fix: preserve nested agent discovery (closes #2692) - #2721
fix: preserve nested agent discovery (closes #2692)#2721Daniel Meppiel (danielmeppiel) wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 3
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/apm_cli/integration/agent_integrator.py — The warning message is not very actionable: it explains what was ignored, but doesn't tell the user… |
|
docs/src/content/docs/producer/author-primitives/instructions-and-agents.md — The target matrix has a duplicate grok-build row, and the second one still uses the old <name>… |
|
CHANGELOG.md — Changelog entries should follow the repo's one-line-per-PR format and end with the PR number in… |
What changed in this PR
This PR addresses agent discovery and deployment correctness by preserving nested agent directory structure end-to-end (plugin staging -> target install), tightening classification of plain Markdown files as agents, and surfacing non-agent sibling resources via warnings so they are not silently dropped or misclassified.
Changes:
- Preserve declared
agents/<agent>/...bundle paths when staging plugin artifacts into.apm/agents/and when deploying to target agent directories. - Treat plain
*.mdas an agent only when it contains non-emptynameanddescriptionYAML frontmatter; avoid misclassifying docs/templates as agents. - Emit warnings listing non-agent files found under
.apm/agents/that will not be deployed as agents.
| File | Description |
|---|---|
src/apm_cli/deps/plugin_parser.py |
Preserves relative bundle subdirectories when staging declared agent directories into .apm/agents/. |
src/apm_cli/integration/agent_integrator.py |
Adds plain-Markdown agent frontmatter guard, preserves nested relative paths during deployment, and warns on ignored non-agent resources. |
tests/unit/test_plugin_parser.py |
Adds regression test ensuring staged .apm/agents/<bundle>/... preserves nested resources. |
tests/unit/integration/test_agent_integrator.py |
Updates/extends tests for frontmatter-based classification and nested bundle deployment behavior + warning emission. |
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md |
Documents nested agent path preservation and the plain-Markdown frontmatter requirement. |
docs/src/content/docs/producer/author-primitives/instructions-and-agents.md |
Updates agent authoring docs to describe nested paths, plain .md requirements, and warning behavior. |
CHANGELOG.md |
Adds an Unreleased “Fixed” entry describing the behavior change. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| message = ( | ||
| f"Ignored {len(relative)} non-agent file(s) under .apm/agents; " | ||
| "only *.agent.md files and plain Markdown files with name and " | ||
| "description frontmatter are deployable." | ||
| ) |
| ### Fixed | ||
|
|
||
| - Agent discovery now preserves nested agent paths, validates plain Markdown | ||
| definitions by `name` and `description` frontmatter, and warns when sibling | ||
| resources cannot be deployed. (closes #2692) |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 3 | 0 | Preserve reporting independently of deployment authorization and avoid repeated scans. |
| CLI Logging Expert | 1 | 2 | 0 | Sanitize ignored-resource diagnostics, emit once, and provide remediation. |
| DevX UX Expert | 0 | 2 | 0 | Make warnings singular and actionable; remove conflicting target docs. |
| Supply Chain Security Expert | 0 | 2 | 0 | Preserve warning coverage and sanitize untrusted paths. |
| OSS Growth Hacker | 0 | 2 | 0 | Clarify target docs and add a copyable nested-agent example. |
| Doc Writer | 0 | 1 | 1 | Synchronize the canonical target matrix and remove the duplicate row. |
| Test Coverage Expert | 0 | 3 | 0 | Fix the stale failing test and add a cross-module regression. |
| Performance Expert | 0 | 1 | 0 | Classify files once per package rather than once per target. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Reconcile the failing plain-Markdown discovery test with the new frontmatter contract -- the existing integration test fails on HEAD.
- **[CLI Logging Expert] (blocking-severity) ** Sanitize package names and reported paths and add an injection regression test -- package-controlled control characters can corrupt terminal output.
- [Test Coverage Expert] Add a cross-module marketplace regression -- the normalize-to-deploy portability promise lacks an integration guard.
- [Python Architect] Separate safe reporting inventory from
DeployableSourcePlanand emit one actionable warning per package -- the Markdown-only plan hides non-Markdown siblings. - [Performance Expert] Discover and classify package files once -- repeated work scales per target.
Architecture
classDiagram
class PluginParser
class BaseIntegrator
class AgentIntegrator
class DeployableSourcePlan
class DiagnosticCollector
BaseIntegrator <|-- AgentIntegrator
PluginParser ..> AgentIntegrator : stages nested inputs
AgentIntegrator ..> DeployableSourcePlan : authorizes agents
AgentIntegrator ..> DiagnosticCollector : records warnings
flowchart TD
A[Plugin manifest] --> B[Plugin parser stages nested agents]
B --> C[Package-level agent discovery]
C --> D[Deployable source plan]
C --> E[Ignored resource inventory]
D --> F[Per-target deployment]
E --> G[One sanitized actionable warning]
Recommendation
Before shipping, reconcile the failing integration test, sanitize all package-controlled terminal fields, and decouple reporting from the Markdown-only deployment plan. Retain a cross-module regression and single-pass package inventory as durable guardrails.
Full per-persona findings
Python Architect
- [recommended] Do not filter reporting-only resources through the deployable source plan at
src/apm_cli/integration/agent_integrator.py:127. - [recommended] Emit the ignored-resource warning once per package.
- [recommended] Add a static guard for the agent-admission authority if this change establishes or centralizes a durable owner.
CLI Logging Expert
- [blocking] Sanitize ignored-resource package names and file paths before rendering at
src/apm_cli/integration/agent_integrator.py:130. - [recommended] Emit the ignored-resource warning only once per package.
- [recommended] Include remediation in the warning.
DevX UX Expert
- [recommended] Emit one actionable ignored-resource warning per package.
- [recommended] Remove the conflicting duplicate Grok Build output-path row.
Supply Chain Security Expert
- [recommended] Production installs omit non-Markdown sibling resources from the warning.
- [recommended] Sanitize package-controlled paths before rendering warnings.
OSS Growth Hacker
- [recommended] Remove the conflicting duplicate Grok Build row.
- [recommended] Add a copyable nested-agent source-to-target example.
Auth Expert -- inactive
Changes affect agent discovery only, not authentication behavior.
Doc Writer
- [recommended] Update
docs/src/content/docs/reference/targets-matrix.mdfrom flat<name>paths to<relative-name>paths. - [nit] Delete the duplicate Grok Build row.
Test Coverage Expert
- [recommended] Update the existing failing plain-Markdown integration test.
- [recommended] Add a cross-module marketplace regression for nested deployment and warnings.
- [recommended] Add Scenario Evidence to the PR body.
Performance Expert
- [recommended] Discover and classify agent files once per package, not once per target.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 0 | Core architecture is sound; retarget the stale performance reference. |
| CLI Logging Expert | 0 | 1 | 0 | Propagate verbose mode to uninstall reintegration diagnostics. |
| DevX UX Expert | 0 | 0 | 0 | The install experience is coherent. |
| Supply Chain Security Expert | 0 | 1 | 0 | Align source-plan admission with agent classification. |
| OSS Growth Hacker | 0 | 0 | 0 | No growth concerns remain. |
| Doc Writer | 0 | 1 | 0 | Correct the Grok Build relative path. |
| Test Coverage Expert | 0 | 1 | 0 | Add Scenario Evidence to the PR body. |
| Performance Expert | 0 | 0 | 0 | Scaling is acceptable. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 4 follow-ups
- [Supply Chain Security Expert] Make
DeployableSourcePlanauthorize only agent files accepted byAgentIntegrator-- source-only Markdown should not influence enforcement. - [CLI Logging Expert] Pass
logger.verboseinto uninstall reintegration diagnostics -- verbose guidance should expose ignored paths consistently. - [Python Architect] Retarget references to the deleted algorithmic-patterns agent file -- stale links weaken future performance reviews.
- [Doc Writer] Change the Grok Build convention to
<relative-name>-- the canonical matrix should document nested deployment.
Recommendation
The core change is ready for maintainer consideration. Fold the source-plan alignment first, then take the verbose, stale-reference, target-matrix, and Scenario Evidence corrections as bounded cleanups.
Full per-persona findings
Python Architect
- [recommended] Retarget stale references to
.github/agents/algorithmic-patterns.agent.mdin the performance scan workflow and agent.
CLI Logging Expert
- [recommended] Construct uninstall reintegration diagnostics with
verbose=logger.verbose.
DevX UX Expert
No findings.
Supply Chain Security Expert
- [recommended] Populate authorized agent paths through
AgentIntegratorclassification instead of every Markdown file.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
No authentication surface changed.
Doc Writer
- [recommended] Replace
.grok/agents/*.mdwith.grok/agents/<relative-name>.md.
Test Coverage Expert
- [recommended] Add Scenario Evidence mapping to the PR body.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 1 | 0 | 0 | Remove Kiro's parallel relative-identity helper. |
| CLI Logging Expert | 0 | 0 | 1 | Avoid redundant verbose-mode guidance. |
| DevX UX Expert | 0 | 0 | 0 | No concerns. |
| Supply Chain Security Expert | 0 | 0 | 0 | No concerns. |
| OSS Growth Hacker | 0 | 0 | 0 | No concerns. |
| Doc Writer | 0 | 1 | 0 | Scope one Scenario Evidence claim to .apm/agents/. |
| Test Coverage Expert | 0 | 0 | 0 | Behavioral surfaces are covered. |
| Performance Expert | 0 | 0 | 0 | No performance concerns. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- **[Python Architect] (blocking-severity) ** Route Kiro through
get_target_filename_for_target, remove_kiro_agent_relpath, and extend AC36 plus its architecture test -- relative identity needs one owner. - [Doc Writer] Scope the Scenario Evidence claim to
.apm/agents/-- the narrower wording matches the test. - [CLI Logging Expert] Rephrase verbose guidance -- already-verbose users should not be told to enable it.
Recommendation
Address the canonical relative-identity split in this PR. Fold the documentation clarification and warning polish into the same pass.
Full per-persona findings
Python Architect
- [blocking] Kiro computes relative identity through
_kiro_agent_relpathinstead of the canonical_source_agent_relpathpath.
CLI Logging Expert
- [nit] Rephrase the warning's verbose guidance.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
No authentication surface changed.
Doc Writer
- [recommended] Scope the security-scan Scenario Evidence row to
.apm/agents/.
Test Coverage Expert
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Canonical agent identity routing and dual guardrails are coherent. |
| CLI Logging Expert | 0 | 0 | 0 | No terminal-output concerns remain. |
| DevX UX Expert | 0 | 0 | 0 | No DevX concerns remain. |
| Supply Chain Security Expert | 0 | 0 | 0 | No supply-chain security concerns remain. |
| OSS Growth Hacker | 0 | 0 | 0 | No growth concerns remain. |
| Doc Writer | 0 | 0 | 0 | Documentation matches the final behavior. |
| Test Coverage Expert | 0 | 0 | 0 | Behavioral changes have regression and cross-module coverage. |
| Performance Expert | 0 | 0 | 0 | Package-scoped inventory scaling is acceptable. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
class InstallServices
class PrimitiveIntegration
class BaseIntegrator
class AgentIntegrator
class DeployableSourcePlan
class DiagnosticCollector
BaseIntegrator <|-- AgentIntegrator
InstallServices ..> PrimitiveIntegration : prepares once
PrimitiveIntegration ..> AgentIntegrator : inventories package
DeployableSourcePlan ..> AgentIntegrator : uses canonical admission
AgentIntegrator ..> DiagnosticCollector : records warnings
flowchart TD
A[Plugin agent sources] --> B[AgentIntegrator canonical admission]
B --> C[DeployableSourcePlan security scan]
B --> D[Package-scoped inventory]
D --> E[Shared target loop]
E --> F[Canonical relative identity]
F --> G[Copilot Claude Kiro and other targets]
Recommendation
Ship at HEAD 02d5aabca900785470fa83abaf95c791526a3c4f; no follow-up work is recommended.
Folded in this run
- (copilot) Make the ignored-resource warning actionable -- resolved in
2e56ddce9f. - (copilot) Remove the duplicate Grok Build target row -- resolved in
ce2e47f2d9. - (copilot) Use the changelog PR-reference format -- resolved in
ce2e47f2d9. - (panel) Sanitize package-controlled diagnostic fields -- resolved in
2e56ddce9f. - (panel) Report non-Markdown resources outside deployment authorization -- resolved in
2e56ddce9f. - (panel) Prepare agent inventory once per package -- resolved in
e19e4683b1. - (panel) Update the stale frontmatter integration contract -- resolved in
2e56ddce9f. - (panel) Add cross-module nested bundle regression coverage -- resolved in
2e56ddce9f. - (panel) Add the canonical owner and dual architecture guardrails -- resolved in
2e56ddce9f. - (panel) Synchronize target-path documentation and add a mapping example -- resolved in
ce2e47f2d9. - (panel) Align agent security-scan admission with
AgentIntegrator-- resolved in95de107e53. - (panel) Preserve verbose uninstall diagnostic details -- resolved in
95de107e53. - (panel) Retarget stale performance catalogue references -- resolved in
95de107e53. - (panel) Document Grok Build relative agent paths -- resolved in
95de107e53. - (panel) Route Kiro relative identity through the canonical owner -- resolved in
02d5aabca9. - (panel) Clarify verbose warning wording -- resolved in
02d5aabca9. - (panel) Add and scope Scenario Evidence in the PR body -- resolved at
02d5aabca9.
Copilot signals reviewed
src/apm_cli/integration/agent_integrator.py:133-- LEGIT: warning lacked a remediation step (resolved in2e56ddce9f).docs/src/content/docs/producer/author-primitives/instructions-and-agents.md:232-- LEGIT: duplicate target row contradicted relative paths (resolved ince2e47f2d9).CHANGELOG.md-- LEGIT: entry did not use the PR-reference format (resolved ince2e47f2d9).
Regression-trap evidence (mutation-break gate)
test_prepare_agent_files_sanitizes_ignored_resource_diagnostic-- removed diagnostic sanitization; test failed as expected; guard restored.test_nested_agent_bundle_filters_resources_and_preserves_agent_path-- restored reporting through deployment authorization; test failed as expected; guard restored.test_agent_files_are_prepared_once_for_multiple_targets-- removed package-scoped preparation; test failed as expected; guard restored.test_nested_agent_bundle_maps_and_deploys_to_multiple_targets-- flattened source relative identity; test failed as expected; guard restored.test_agent_source_admission_and_inventory_have_single_owner-- removed AC36; test failed as expected; guard restored.test_non_agent_markdown_is_not_in_authorized_agent_scan-- restored all-Markdown scan admission; test failed as expected; guard restored.test_uninstall_reintegration_preserves_user_scope_and_denies_bin_trust-- removed verbose propagation; test failed as expected; guard restored.test_kiro_agents_nested_path_identity_derivationand the architecture owner test -- restored parallel Kiro identity; both failed as expected; guard restored.
Lint contract
uv run --frozen --extra dev ruff check src/ tests/ and uv run --frozen --extra dev ruff format --check src/ tests/ passed. Pylint R0801, YAML I/O, file length, relative-path, auth, architecture, repository self-audit, and spec-conformance checks also passed.
CI
Run 33308442153 is green after 2 CI fix iterations; all checks succeeded or were intentionally skipped.
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2721 | 02d5aab |
ship_now | 4 | 17 | 0 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
4 outer iterations; 2 Copilot rounds. Final panel stance: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect, CLI Logging Expert, DevX UX Expert, Supply Chain Security Expert, OSS Growth Hacker, Doc Writer, Test Coverage Expert, and Performance Expert returned no findings. Auth Expert was inactive because no authentication surface changed.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
02d5aab to
e6a8423
Compare
|
Rebased onto current main at Pre-rebase head: Conflicting paths resolved (faithful merge of both intents):
Regression-trap tests re-verified post-rebase (mutation-break gate):
Lint contract: Post-push mergeability: Ready for maintainer review. |
e6a8423 to
f938848
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Prepare agent sources once per package, preserve warning coverage outside the deployable source plan, and sanitize package-controlled diagnostics. Add regression and architecture guardrails to address the review panel follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Align the target matrix, warning guidance, package guide, and changelog with the nested-path contract and actionable resource warning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract package-scoped primitive preparation and integration hints from the size-limited services module, and keep the existing diagnostic-owner mutation test targeted at its intended wrapper. Addresses CI failures from run 33306131435. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refresh the architecture instruction hash and remove the stale generated agent so the repository self-audit matches the current local primitive inventory. apm-spec-waiver: Restores existing agent bundle deployment semantics without adding a new manifest contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Route security scan admission through AgentIntegrator, preserve verbose uninstall diagnostics, and synchronize the remaining target and performance-reference documentation. Addresses the final review panel follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Route Kiro through AgentIntegrator's canonical relative-path owner and extend AC36 so a target-specific identity helper cannot recreate split authority. Addresses the terminal Python Architect follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
f938848 to
d00dc87
Compare
Preserves declared agent subdirectory structure through plugin normalization and target deployment, recognizes plain Markdown agents only when they carry non-empty name and description frontmatter, and emits one actionable warning for non-agent sibling resources instead of silently dropping or misclassifying them.
Architecture classification: new-owner.
AgentIntegrator.prepare_agent_files,_is_plain_md_agent, and_source_agent_relpathcanonically own agent admission, relative identity, and package-level inventory. AC36 and its architecture test prevent split authority.Scenario Evidence
tests/integration/test_marketplace_plugin_integration.py::TestPluginIntegration::test_nested_agent_bundle_maps_and_deploys_to_multiple_targets.apm/agents/without valid agent frontmatter stays outside agent deployment and the built-in security scan.tests/unit/install/test_security_scan_scope.py::test_non_agent_markdown_is_not_in_authorized_agent_scantests/unit/integration/test_agent_integrator.py::TestAgentIntegrator::test_prepare_agent_files_sanitizes_ignored_resource_diagnostictests/integration/test_install_services_orchestration.py::TestIntegratePackagePrimitives::test_agent_files_are_prepared_once_for_multiple_targetstests/unit/test_surviving_deps_reintegration.py::test_uninstall_reintegration_preserves_user_scope_and_denies_bin_trustHow to test
The regression tests were mutation-proved by removing each production or architecture guard, observing the corresponding test fail, and restoring the guard.
Closes #2692