fix(install): explain skipped symlinked agent sources (closes #2913)

[Daniel Meppiel (danielmeppiel)]

fix(install): explain skipped symlinked agent sources

TL;DR

apm install now reports skipped symlinked agent sources with the source path, the reason, and a real-file remedy. The warning is collected once during source planning, rather than repeated for each target. Source authorization, deployed files, and existing local-path dependency dereferencing are unchanged.

Closes #2913.

Important

This is the maintainer-approved diagnostic-only fix, not a symlink-acceptance change. Source planning still excludes symlink files and does not traverse symlink directories. Legacy integrator discovery may still enumerate/read rejected candidates before authorization filtering; this PR does not claim to fix that separate behavior.

Problem (WHY)

• A project or installed Git package with .apm/agents -> ../agents deployed zero agents without explaining why. Real-filesystem regressions reproduced both missing warnings before the fix; both real-directory controls passed.
• The report explicitly identifies the fallback requirement: "Failing that, the install has to say something." The diagnostic supplies the missing source and corrective action, following "Add what the agent lacks, omit what it knows".

Approach (WHAT)

• Report from the existing has_symlink_component rejection branch.
• Thread an optional callback only through agent-source discovery.
• Use the existing DiagnosticCollector and deferred warning renderer.
• Preserve lexical, package-relative source paths so the warning names the link, not its resolved target; sanitize package and path labels for terminal output.

Implementation (HOW)

File	Change
src/apm_cli/install/deployable_source_plan.py	Optional reporting at the existing rejection guard; no second detector, additional walk, or authorization decision. Wording says "Symlinked agent sources are not deployed."
src/apm_cli/install/services.py	Forward package name and collector into the one source plan shared by selected targets.
tests/unit/install/test_agent_symlink_diagnostics.py	Twenty component cases cover CLI install/reinstall, both service routes, real-directory controls, links, target gating and printable diagnostics; service cases pin the narrowed wording.
tests/spec_conformance/test_manifest_reqs.py	Two real agent-link scan/materialization regressions for existing req-sc-015; paired warning assertions are APM behavior, not a new spec mandate.
CONFORMANCE.json, CONFORMANCE.md	Regenerate the requirement's evidence count from three to five cases.
docs/src/content/docs/producer/author-primitives/instructions-and-agents.md	Document skip, remedy and unchanged contained local-path copying.
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md	Keep the independently usable usage reference aligned.
docs/src/content/docs/consumer/install-packages.md	Add a troubleshooting entry, authoring cross-link and third-party-package guidance; do not edit the cache as a workaround.
CHANGELOG.md	Add one Unreleased fix entry.

Architecture classification: owner-extension, observability only. The deterministic gate identifies Authorized deployable source paths at exact base e38261c5db4d893d6ddebc3925742e4e3bd2ba74 and head aa95cf98543492cfe4d83d325659e456138409ff. DeployableSourcePlan remains the sole authorization owner; integrators consume its unchanged path set. Existing install-deployment-source-plan static guard and architecture baseline/mutation assertions pass. No authority is centralized, split or rerouted.

Diagrams

The highlighted interaction is new; the rejected path stays outside the shared deployment plan.

sequenceDiagram
    participant C as Install command
    participant S as install.services
    participant P as DeployableSourcePlan
    participant D as DiagnosticCollector
    participant A as AgentIntegrator
    C->>S: integrate package primitives
    S->>P: create for all selected targets
    P->>P: has_symlink_component rejects agent source
    rect rgb(255, 247, 200)
        P->>D: warn with lexical path and real-file remedy
    end
    P-->>S: unchanged authorized source paths
    S->>A: integrate for each selected target with plan
    A-->>S: unchanged deployed files
    S-->>C: integration result
    C->>D: render deferred summary
    Note over P,A: No authorization or deployment expansion

Loading

Trade-offs

• Warn instead of dereference. Git-backed and own-project agent symlinks remain unsupported, including contained links. This preserves the approved boundary and makes the omission actionable.
• Source-level rather than per-agent counts. Source planning reports a rejected directory without traversing it; the warning does not claim to know its contents.
• Known residual, deferred hardening. Legacy AgentIntegrator.find_agent_files can enumerate/read candidates under a symlinked agents root before authorization filtering. Empty deployed-file assertions do not prove absence of reads. Routing discovery through the canonical authorized source set, with a no-read regression, is a separate follow-up outside the expressly approved diagnostic-only scope. No duplicate detector was added here.
• Bounded scope. Existing validated contained local-path copying is untouched. No new root agents/ layout, README edit, normative spec edit, transport change or lockfile expansion.
• Installed-package boundary. Git-package regressions start with real symlinks in apm_modules; they do not exercise network acquisition.

Benefits

1. One warning for a rejected agent source even with two selected targets.
2. Default and verbose CLI install/reinstall both explain the skip.
3. Real-directory controls still deploy exactly one agent per target.

Validation

Hosted CI is green at aa95cf98543492cfe4d83d325659e456138409ff, with no retry on this head: CI, spec conformance, aggregate gate. All 18 checks concluded SUCCESS, NEUTRAL or SKIPPED. GitHub reports MERGEABLE / BLOCKED; passing checks and the advisory panel are not policy approval or merge permission.

Two full panel passes selected and verified the wording/docs folds. Final quality stance is ship_with_followups, solely for the separate legacy-discovery hardening noted above. Copilot inline 3973081759 was answered in-thread and resolved with that scope distinction. The suppressed Windows runtime-skip suggestion was declined under repository test conventions and observed Windows success.

Local validation and mutation evidence

Current-head command:

uv run --frozen --extra dev pytest --no-cov -q \
  tests/unit/install/test_agent_symlink_diagnostics.py \
  tests/spec_conformance/test_manifest_reqs.py::test_agent_skip_diagnostic_preserves_authorized_scan_and_materialization \
  tests/integration/test_architecture_owner_rule_mutations.py \
  -k 'symlink or source_plan or source-plan'

26 passed, 339 deselected in 16.72s

Related diagnostic, security-scan-scope, local-copy and paired conformance cases after the fold:

52 passed in 3.06s

All seven current canonical lint gates plus architecture boundaries passed before push. Architecture boundaries also passed again at the exact pushed head. The three grep/awk guards used equivalent macOS-compatible expressions.

Regression probe	Result
Original missing-warning reproduction	2 failed; 2 real-directory controls passed
Narrowed warning assertion before wording fold	2 failed, 2 passed, 16 deselected in 2.21s
Remove callback after folds, across all 22 new cases	19 failed, 3 passed in 2.25s; callback restored
Original service diagnostics-wiring mutation	2 failed, 2 passed, 14 deselected in 0.88s; wiring restored

Historical FIX-wave evidence, not substituted for current-head execution: 315 targeted cases passed; quality suite 63 passed; full conformance 204 passed/2 pre-existing skips; 122 requirement anchors aligned. Initial Mode B failure was fixed with genuine req-sc-015 test coverage and regenerated statements, not a waiver.

At previous head 068766eef4c16330716d739f5baab7823bd77a5d, CI run 34406073853 hit one Linux Shard 2 HTTP-thread semaphore timeout (job 102649234979). One failed-job retry passed without code changes; stale aggregate 34406073846 was refreshed once. The same HTTP test passed immediately after all new diagnostics in one process (21 passed in 1.51s). That provenance is retained; current-head runs passed without retries.

Scenario Evidence

These are real-filesystem/command component tests, not mocked security decisions.

#	Scenario (user promise)	Principle(s)	Test proving it	Type
1	Own-project install/reinstall explains the skip in default and verbose mode	DevX	tests/unit/install/test_agent_symlink_diagnostics.py::test_own_project_cli_reports_symlink_skip_on_install_and_reinstall	integration-with-fixtures
2	Both service routes warn once across targets, without extra deployment or ledger entries	Multi-harness support, Secure by default	tests/unit/install/test_agent_symlink_diagnostics.py::test_agent_source_skip_is_actionable_without_deployment_expansion	integration-with-fixtures
3	Contained, external and dangling links stay rejected while real sources remain authorized	Secure by default	tests/unit/install/test_agent_symlink_diagnostics.py::test_plan_reports_rejected_links_without_traversing_them	integration-with-fixtures
4	Unselected agents stay quiet and labels remain printable	DevX	tests/unit/install/test_agent_symlink_diagnostics.py::test_agent_skip_diagnostics_are_target_gated_and_printable	integration-with-fixtures
5	A warned symlink cannot enter the authorized security scan or deployed results	Secure by default	tests/spec_conformance/test_manifest_reqs.py::test_agent_skip_diagnostic_preserves_authorized_scan_and_materialization (req-sc-015)	integration-with-fixtures

How to test

• Run uv run --frozen --extra dev pytest --no-cov -q tests/unit/install/test_agent_symlink_diagnostics.py; all twenty cases should pass.
• Create the issue's .apm/agents -> ../agents own-project layout and run apm install --target claude; expect the source-path warning and no agent deployment.
• Repeat with --verbose, then replace the link with real agent files under .apm/agents/; expect the warning to disappear and the agent to deploy.
• Run uv run --frozen --extra dev pytest --no-cov -q tests/unit/install/test_security_scan_scope.py tests/unit/install/test_local_content_symlink_deref.py to confirm existing exclusions and local-copy behavior.

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com

[Copilot]

Copilot review overview

🟡 Changes recommended

Symlinked sources may still be traversed, and symlink tests need portable Windows handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	src/​apm_cli/​install/​deployable_source_plan.py — This warning claims symlinked directories are not traversed, but the agent path is still walked…

What changed in this PR

Adds actionable warnings when symlinked agent sources are skipped during installation while preserving existing security filtering.

Changes:

• Threads diagnostics through source planning.
• Adds symlink rejection and target-gating tests.
• Updates authoring documentation.

File	Summary
tests/​unit/​install/​test_agent_symlink_diagnostics.py	Adds filesystem and CLI regression coverage.
src/​apm_cli/​install/​services.py	Forwards diagnostic context into planning.
src/​apm_cli/​install/​deployable_source_plan.py	Reports rejected symlinked sources.
packages/​apm-guide/​.apm/​skills/​apm-usage/​package-authoring.md	Updates authoring guidance.
docs/​src/​content/​docs/​producer/​author-primitives/​instructions-and-agents.md	Documents symlink behavior and remediation.

Suppressed comments (1)

tests/unit/install/test_agent_symlink_diagnostics.py:212

• This module is selected by the windows_compat gate, but the new test cases create symlinks without handling OSError/NotImplementedError. On a Windows runner without symlink privileges, the setup fails before the assertions; the repository's other symlink tests skip in this situation (for example, tests/unit/marketplace/test_marketplace_audit.py:427-431). Guard the symlink setup with a shared skip helper so the cross-platform gate remains portable.

@pytest.mark.windows_compat

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_with_followups

Truthful diagnostic-only symlink skip guidance, with unchanged deployment behavior.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The selected warning and documentation folds are complete at aa95cf98543492cfe4d83d325659e456138409ff. Real-filesystem evidence protects the narrowed sentence and exact deployed sets. Architecture and supply-chain reviewers converge on recording legacy discovery reads as a separate hardening follow-up, not changing this diagnostic-only PR.

Aligned with: Secure by default, governed by policy, pragmatic as npm.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	0	Canonical owner retained; record legacy-discovery residual.
CLI Logging Expert	0	0	0	Accurate, actionable, ASCII-safe deferred warning.
DevX UX Expert	0	0	0	Clear remedy, once per source, unchanged exit behavior.
Supply Chain Security	0	1	0	No source/deployment expansion; no no-read claim.
OSS Growth Hacker	0	0	0	Changelog and consumer cross-link folded.
Doc Writer	0	0	1	Guidance accurate; suggested closer anchor is unnecessary.
Test Coverage	0	0	0	Exact-head consumer tests and callback mutation protect the change.
Performance Expert	0	0	0	No new walk, I/O or transport work.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 1 follow-up

1. [Supply Chain Security / Python Architect] Legacy AgentIntegrator.find_agent_files can enumerate/read rejected sources before authorization filtering. This is explicitly recorded in the PR trade-offs and deferred: a separate hardening change should consume the canonical authorized source set and add a no-read regression.

Architecture

flowchart TD
  A[apm install CLI] --> B[services.integrate_package_primitives]
  B --> C[DeployableSourcePlan.create]
  C --> D{_is_safe_source_path}
  D -- contained real path --> E[add to plan.paths]
  D -- has_symlink_component --> F[on_symlink=warn_agent_symlink]
  F --> G[FS DiagnosticCollector.warn sanitized rel path]
  E --> H[AgentIntegrator.integrate_agents_for_target]
  H --> I[FS deploy authorized real files only]
  G --> J[render_summary at command end]
  I --> J

Loading

Recommendation

No remaining selected in-scope code changes. The known legacy-discovery residual is recorded in the PR body and below. Keep the existing consumer #agents link: the troubleshooting bullet already gives the complete remedy, and adding another heading would fragment the short Layout paragraph. The separate hardening follow-up must not introduce another symlink detector.

---

Full per-persona findings

Python Architect

• [recommended] Record pre-authorization discovery reads as an out-of-scope follow-up. The callback remains observability at the canonical guard.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security

• [recommended] The no-deployment warning does not mean no reads. Record the residual in the PR trade-offs and track future canonical authorized-only discovery separately.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

No credentials, host classification or remote authorization behavior changed.

Doc Writer

• [nit] A dedicated remedy heading would provide a closer anchor. CEO does not select it: the existing section anchor is correct and the consumer bullet already contains the full remedy.

Test Coverage

No missing-coverage findings. Exact-head command: diagnostic module, paired conformance cases, and existing architecture mutation module selected with -k 'symlink or source_plan or source-plan': 26 passed, 339 deselected in 16.72s. These tests prove warning and deployment parity, not absence of legacy discovery reads.

Performance Expert

No findings.

_{This panel is advisory. It does not block merge.}

Reservations carried from strategic-alignment

• User expressly approved diagnostic-only; retain source-plan symlink exclusion and exact deployed sets. Do NOT restore remote dereferencing or loosen has_symlink_component; earlier strategic proposal superseded. -- Addressed: callback observes unchanged rejection; final fold only narrows wording.
• Leave existing contained local-dependency preprocessing intact; req-sc-015 exclusion is normative, new warning itself is not a spec mandate. -- Addressed: local-copy behavior unchanged and tested; conformance assertions distinguish normative exclusion from diagnostic UX.
• No duplicate symlink detector; observe existing canonical guard with diagnostics once per source, avoiding per-target noise. -- Addressed: one plan callback shared across selected targets.

Folded in this run

• (Copilot + panel) Remove global no-traversal wording and pin accurate no-deployment sentence for both services routes -- aa95cf98543492cfe4d83d325659e456138409ff.
• (Panel) Clarify callback's symlink-only observer contract, local-copy docs, consumer remedy cross-link and Unreleased changelog -- same commit.
• (Panel) Record residual legacy discovery reads -- current PR trade-offs.

Copilot signals reviewed

• Inline 3973081759 (review 5159932411): LEGIT wording overclaim, fixed in aa95cf985. Replied in-thread and resolved with the explicit scope distinction; no-read hardening is deferred.
• Review 5159932411, suppressed Windows symlink skip suggestion: NOT-LEGIT for the supported current test contract; hosted Windows passed, and adding runtime privilege skips without a concrete failure would weaken the regression.

Deferred (out-of-scope follow-ups)

• Legacy AgentIntegrator before-classification reads: the approved scope is actionable diagnostics with no change to deployment/discovery behavior. Route future discovery through the canonical authorized source set with a no-read regression in a separately reviewed hardening change. No follow-up issue was opened in this run.

Regression-trap evidence (mutation-break gate)

• Remove the reporting callback from the existing symlink rejection branch: 19 failed, 3 passed in 2.25s across all 22 new cases; callback restored.
• New narrowed-sentence assertion before the wording fold: 2 failed, 2 passed, 16 deselected in 2.21s; the real-directory controls passed.
• Original FIX-wave service-wiring and source-plan mutation evidence is retained in the PR body.

Lint contract

All seven current canonical lint gates plus architecture boundaries passed before push, including Ruff over src/, tests/ and architecture scripts, formatting, pylint R0801, YAML I/O, 2100-line and portable-path guards, and auth signals. Architecture boundaries passed again on exact head aa95cf985.

CI

All 18 checks concluded SUCCESS, NEUTRAL or SKIPPED on aa95cf985, with zero CI recovery iterations or retries on this head: CI, spec, aggregate.

Historical FIX-wave provenance retained: previous head 068766eef had one HTTP-thread semaphore timeout in run 34406073853 job 102649234979, one successful failed-job retry, and one stale aggregate refresh (34406073846). These are not current-head evidence.

Mergeability status

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#2932	aa95cf985	ship_with_followups	2	3	1	2	green	MERGEABLE	BLOCKED	Repository policy remains unsatisfied; no approval or merge permission claimed.

Convergence

Two outer iterations and two Copilot rounds. Selected in-scope folds are complete; one explicitly out-of-scope hardening follow-up remains. Terminal driver status is blocked by repository policy, not a claim of full merge-readiness. No auto-merge or fabricated approval.

[Toon Verwerft (veewee)]

Just wondering: Why don't you allow symlinks withing the same repository root?
What security issues does this involve?

The way i see it this is binding the project shipping the agents to apm, whilst there are package manager tools as well.
I don't want to be copy/pasting the agents for multiple target AI platforms cause that's what this package is supposed to solve.

Looking forward to your vision.

(Btw: we are taking the same approach for skills, which just works. So disallowing it for agents and not for skills might not be very consistent)