Follow-ups from apm-review-panel on #1313 (adopt byte-identical files)

[danielmeppiel]

Tracking the deferred recommendations from the apm-review-panel synthesis on #1313. Each one was raised by 1-3 panelists; none block the merge.

1. Observability: IntegrationResult.files_adopted counter

Convergence: cli-logging-expert + devx-ux-expert.
Adopt-only package runs emit (files unchanged) in the install summary because services.py:347 computes _total_integrated from files_integrated + skill_created + sub_skills_promoted -- adopted files are absent from all three. The catch-22 recovery is invisible to humans and to AI agents reading --verbose traces.

Suggested change:

• Add files_adopted: int = 0 to IntegrationResult (mirrors the existing sub_skills_promoted field).
• Increment at each is_content_identical_to_source call site instead of a bare append + continue.
• Fold into _total_integrated and emit |-- N files adopted (byte-identical) under --verbose.

2. End-to-end test rigor: exercise the real policy-blocked catch-22

Convergence: devx-ux-expert + test-coverage-expert.
tests/integration/test_silent_adopt_existing_files_e2e.py::test_required_packages_deployed_passes_after_lockfile_wipe re-installs with --no-policy, which bypasses the very gate that defines the catch-22. The current test cannot fail even on unfixed main.

Suggested change: add a third apm install run (without --no-policy) and assert returncode == 0; or add an integration-with-fixtures-tier test that keeps the lockfile intact with deployed_files=[] and exercises the real policy_gate -> integrate ordering. Document the one-time apm install --no-policy recovery command for users already in the catch-22.

3. Refactor: BaseIntegrator.try_adopt_identical helper

From: python-architect.
The 5-line if self.is_content_identical_to_source(t, s): target_paths.append(t); continue pattern is repeated verbatim at 7 call sites across agent_integrator, instruction_integrator, prompt_integrator, command_integrator, and (after #1313) hook_integrator. Exceeds the project's abstract-at-3-call-sites threshold.

Sketch:

@staticmethod
def try_adopt_identical(target_path: Path, source_path: Path, target_paths: list[Path]) -> bool:
    if BaseIntegrator.is_content_identical_to_source(target_path, source_path):
        target_paths.append(target_path)
        return True
    return False

Each call site collapses to if self.try_adopt_identical(...): continue. Pure refactor, no behavior change.

(Architect explicitly recommends against the heavier tri-state decide_file_action -> Literal['adopt','skip','write'] shape until adopt logic grows beyond byte-equality -- speed and simplicity over complexity.)

4. Naming symmetry: SkillIntegrator._dirs_equal

From: python-architect (nit).
Rename SkillIntegrator._dirs_equal to is_skill_dir_identical_to_source so the symmetry with BaseIntegrator.is_content_identical_to_source is visible without having to discover the parallel.

5. Error UX: hint the --no-policy escape

From: devx-ux-expert.
The required-packages-deployed violation message at policy_gate.py:153 lists missing packages but offers no concrete next action for users in the catch-22. Append:

Hint: re-run with --no-policy to bypass this check and repair the lockfile, then reinstall normally.

6. CommandIntegrator copilot-target unit test

From: test-coverage-expert (nit).
tests/unit/integration/test_content_identical_adopt.py:239 has a comment claiming CommandIntegrator adopt is dead-code because of the format transformer. False: the adopt check fires before the format dispatch for the primary copilot target (which is a direct .prompt.md copy). Add a TestCommandIntegratorAdopt mirroring TestPromptIntegratorAdopt for the copilot target and remove the misleading comment.

7. Growth-narrative framing

From: oss-growth-hacker.
Frame the next release post around 'self-healing governance' -- a real enterprise consumer (zava-storefront) reproduced a permanent install lockout and the policy engine now repairs itself. Lead with the named-user citation; the trust arc converts skeptics of heavy policy tooling into adopters.

---

Originating PR: #1313

[github-actions]

Triage decision

accept

Proposed labels

theme/governance
area/cli
area/testing
type/refactor
status/accepted

Milestone

null (cleanup tracking; maintainer should attach to the appropriate patch milestone)

Suggested next action

Pick items by size: items 4 (rename) and 5 (error hint) are one-liners that can land in any PR touching those files; items 1 and 3 go together as a single IntegrationResult + BaseIntegrator PR; items 2 and 6 go together as a test-coverage PR.

Suggested issue comment

Thanks for capturing these deferred items so cleanly -- the numbered list and code sketches make it easy to pick them up incrementally.

A quick orientation for anyone picking these up:

- Items 4 (`_dirs_equal` rename) and 5 (`--no-policy` error hint) are the lowest-friction starting points -- both are one- or two-line changes in a single file and can ride in any PR that already touches those modules.
- Items 1 (`files_adopted` counter) and 3 (`BaseIntegrator.try_adopt_identical`) are complementary and worth landing together: the counter makes the adoption visible in `--verbose` output; the helper eliminates the 7-site duplication that was the root cause of the adoption being invisible in the first place.
- Items 2 and 6 are test discipline improvements. Item 2 is the more impactful one -- the current e2e test cannot fail even on an unfixed main, so adding the third `apm install` run (without `--no-policy`) is the real regression guard. Item 6 removes a misleading comment and adds the missing `TestCommandIntegratorAdopt` coverage.
- Item 7 (growth-narrative framing) is a release-notes / blog-post note for the maintainer, not a code task.

Triaged as `type/refactor` + `theme/governance` (policy gate UX and file-adoption observability are governance-surface concerns). `status/accepted` -- design is fully specified in the issue body with code sketches; no separate design phase needed. Assign to whichever patch milestone fits your current sprint.

Per-lens notes (collapsed)

DevX UX Expert -- User-Need Reviewer

Items 1 and 5 are the user-visible wins. Item 1 closes a silent-success footgun: adopt-only installs currently print (files unchanged) even when files were matched byte-for-byte against source -- the correct status is invisible to humans and to AI agents reading --verbose traces. Adding N files adopted (byte-identical) under --verbose restores the transparency that a package manager user expects. Item 5 turns a dead-end required-packages-deployed policy violation into an actionable recovery message (re-run with --no-policy to bypass this check and repair the lockfile, then reinstall normally) -- exactly the npm ERR! ... run npm install --legacy-peer-deps pattern that reduces support load. Items 2 and 6 are test discipline improvements that protect these user-visible paths from regression.

Supply Chain Security Expert -- Risk-Surface Reviewer

Item 5 touches policy_gate.py:153 (the required-packages-deployed enforcement gate). The proposed change appends a hint string to the violation message; it does not weaken enforcement or expose a bypass -- the --no-policy flag already exists and is documented. File adoption logic (is_content_identical_to_source) is a governance integrity check: byte-identity is the correctness signal that APM uses to track policy-compliant state. The issue's primary concern is the observability and UX of governance enforcement flows, which maps cleanly to theme/governance. No new attack surface is introduced by any of the 7 items.

OSS Growth Hacker -- Contributor-Tone Reviewer

Not activated -- author (danielmeppiel) is a COLLABORATOR with significant prior interaction; this is a structured technical follow-up from a completed code review, not a first-time contributor pattern requiring tone adjustment.

Python Architect -- Architecture Reviewer

The BaseIntegrator.try_adopt_identical sketch (item 3) is clean: @staticmethod, returns bool, call sites collapse to if self.try_adopt_identical(...): continue. The project's own "abstract-at-3-call-sites" threshold is cited and exceeded (7 call sites across 5 integrators). IntegrationResult.files_adopted: int = 0 (item 1) mirrors the existing sub_skills_promoted field pattern -- no contract break. The rename in item 4 is a pure identifier rename with no behavioral change. No status/needs-design warranted; the design for each item is fully specified with code sketches, argument for the specific implementation, and explicit scope boundaries. Feasibility is high for all items.

Doc Writer -- Documentation Reviewer

Not activated -- all 7 items are code-level changes (counters, refactors, tests, error messages). Item 7 is growth-narrative framing for a release post, not a change to docs/src/content/docs/. No new user-facing doc pages are implied by the code work in items 1-6.

APM CEO -- Triage Arbiter

All 7 items are concrete, implementable, and well-scoped post-review cleanup -- the design for each is already specified, so no separate design phase is needed. Primary theme: theme/governance (policy gate UX, file adoption observability). The refactoring character dominates (items 3, 4 are pure refactors; items 1, 5 are minor additive improvements within the refactoring sweep; items 2, 6 are test corrections). status/accepted. No priority label (cleanup, not blocking) and no milestone assignment from the panel -- a maintainer should attach to whichever patch milestone fits the current sprint.

{
  "decision": "accept",
  "decision_detail": "",
  "theme": "theme/governance",
  "areas": ["area/cli", "area/testing"],
  "type": "type/refactor",
  "status": "status/accepted",
  "priority": null,
  "preserved_labels": [],
  "milestone": null,
  "next_action": "Group items 1+3 into one PR (IntegrationResult counter + BaseIntegrator helper), items 2+6 into a test-coverage PR, and items 4+5 as opportunistic one-liners in any PR touching those files.",
  "comment_markdown": "Thanks for capturing these deferred items so cleanly -- the numbered list and code sketches make it easy to pick them up incrementally.\n\nA quick orientation for anyone picking these up:\n\n- Items 4 and 5 are the lowest-friction starting points -- both are one- or two-line changes in a single file.\n- Items 1 and 3 are complementary and worth landing together: the counter makes adoption visible in --verbose output; the helper eliminates the 7-site duplication.\n- Items 2 and 6 are test discipline improvements.\n- Item 7 is a release-notes note for the maintainer, not a code task.\n\nTriaged as type/refactor + theme/governance. status/accepted -- design is fully specified; no separate design phase needed."
}

---

Triage status: agentic proposal pending human ratification.
Silence is approval. Maintainers can:

• Override any label or milestone above by editing it directly --
  human edits are authoritative and will not be reverted on
  subsequent runs.
• Re-trigger triage by applying the status/needs-triage label, or
  by removing status/triaged to enroll the issue in the next
  daily sweep.

Posted by the Triage Panel workflow. See .apm/skills/apm-triage-panel for the panel skill.

Note

🔒 Integrity filter blocked 23 items

The following items were blocked because they don't meet the GitHub integrity level.

• #1120 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1122 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1130 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1138 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1156 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1157 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1209 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1210 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1220 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1222 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1225 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1231 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1260 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1266 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1273 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1279 search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• ... and 7 more items

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by Triage Panel · ● 1.1M · ◷