feat(policy): declarable integrity keys (require_hashes, fail_on_drift)

[danielmeppiel]

Problem

apm-policy.yml can express several governance floors (audit-on-install, registry routing, scanner restrictions), but it cannot yet declare two integrity guarantees that the install/audit pipeline already enforces operationally:

1. A team that wants reproducible, tamper-evident installs has no way to require that every locked dependency carries a content hash. Today a missing hash passes silently.
2. A team that wants apm audit to be a hard CI/local gate on workspace drift has to remember --ci; a bare apm audit renders drift but exits 0.

Both behaviors exist in the code today -- they just are not declarable as policy.

What changed: two keys, and only two

Both are additive, optional, and default off, nested under the existing security: namespace (no new top-level namespace, no top-level audit:):

• security.integrity.require_hashes (bool, default off) -- require lockfile content hashes for all installs. A missing/empty hash on a non-local lockfile entry is a failure (fail-closed), not a silent pass.
• security.audit.fail_on_drift (bool, default off) -- make a bare apm audit exit non-zero when workspace content drifts from the lockfile. Sits alongside the existing security.audit.on_install.

Why only two

Every field in the policy schema maps 1:1 to a concrete apm check that enforces it today. require_hashes asserts hash-presence on the freshly-built lockfile entry (the install pipeline already computes + records hashes -- no second hashing pass). fail_on_drift reuses the existing drift detection and only changes the exit code -- it does not add a second drift pass. Keys for signatures / provenance / sbom are deliberately not added here (deferred to #1777): a key that validates but enforces nothing is a false-assurance liability.

Fail-closed behavior

• require_hashes: true + any non-local locked entry with an empty/missing content_hash -> install fails closed (offenders named). Local dependencies are exempt. No-op when the key is off, --no-policy, or no policy resolves.
• fail_on_drift: true + detected drift -> apm audit escalates exit code to 1. Default-off preserves today's advisory (rendered, exit 0) behavior.

Both keys carry through policy inheritance with logical-OR / tighten-not-relax semantics: once a parent enables a key a child cannot relax it, and a child silent on the key preserves the parent's value (None-transparency). The structural merge coverage guard from #1791 stays green.

Test seams (TDD, red first)

1. Golden parse-equivalence -- parse every fixture under tests/fixtures/policy/, canonicalize, and assert equality with a checked-in golden snapshot, proving old policies parse identically after the new keys are added (the additive / non-breaking claim).
2. None-transparency on merge -- a child silent on require_hashes / fail_on_drift does not override a parent that set them.
3. Fail-closed -- a lockfile entry with a missing/empty hash fails under require_hashes: true (and passes when present); fail_on_drift: true makes a drifted workspace exit non-zero while default-off preserves current behavior.

New on/off fixtures per key were added; a mutation-break was confirmed (forcing require_hashes to treat a missing hash as a pass fails the suite) and restored.

Validation evidence (all green)

uv run --extra dev ruff check src/ tests/            # All checks passed
uv run --extra dev ruff format --check src/ tests/   # 1270 files already formatted
uv run --extra dev python -m pylint --disable=all --enable=R0801 \
  --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/   # 10.00/10
bash scripts/lint-auth-signals.sh                    # auth-signal lint clean
uv run --extra dev pytest tests/unit -k "policy or audit or inheritance or integrity or drift or require_hashes" -q
  # 1950 passed, 1 xfailed

All source files stay under 2450 lines; ASCII only.

Docs

• docs/src/content/docs/reference/policy-schema.md: documented both keys (default off; what each enforces) + merge rules.
• packages/apm-guide/.apm/skills/apm-usage/governance.md: integrity/drift governance subsection.
• CHANGELOG.md: Unreleased ### Added entry.

Spec citation (OpenAPM v0.1)

Both keys are now first-class normative requirements in the OpenAPM v0.1 specification (maintainer governance call: author the citation, not a waiver):

• req-pl-013 (security.integrity.require_hashes, MUST, governance) and req-pl-014 (security.audit.fail_on_drift, MUST, governance) land under a new Section 6.8 "Integrity controls", with a non-normative 6.3.6 security field reference, two req-pl-006 merge-table rows (logical OR / tighten-not-relax), an Appendix C index row each, and a requirements-manifest entry each. Statement count 87 -> 89 (84 MUST, 5 SHOULD).
• The apm-spec-guardian adversarial panel returned fold_and_ship (shocked-meter avg 7.75/10, zero blocking across all four reviewers). The single convergent recommended finding -- bind the phrase "recorded integrity hash" to the concrete content_hash lockfile field the implementation checks -- was folded.

Cross-PR count reconcile: this PR bumps the shared spec count sites (Section 1.3 count sentence, Appendix C trailer, Appendix D revision history) from base 87 -> 89. The sibling spec-citation PR #1793 edits the same shared sites independently. Whichever of #1793 / #1794 merges second will need a trivial reconcile: bump the cumulative count to the union total and take the union of the added Appendix C / manifest rows. Authored independently; no count coordination attempted between the two PRs.

Part of #1774

[Copilot]

⚠️ Not ready to approve

The current implementation can leak credentials in error output and has edge cases where policy-enabled drift/hash enforcement can silently fail open.

Pull request overview

This PR makes two existing integrity behaviors declarable in apm-policy.yml under the existing security: namespace: (1) requiring lockfile content hashes for non-local dependencies (security.integrity.require_hashes) and (2) making apm audit exit non-zero on drift (security.audit.fail_on_drift), with tighten-not-relax inheritance semantics and accompanying tests/docs.

Changes:

• Extend policy schema + parsing + inheritance merge to include security.integrity.require_hashes and security.audit.fail_on_drift (both default-off, tighten-only).
• Enforce require_hashes in the install pipeline by reading the freshly-written lockfile and failing closed when hashes are missing.
• Make apm audit optionally gate on drift via policy, plus add golden-fixture parse-equivalence + targeted unit tests and docs/changelog updates.

File summaries

File	Description
src/apm_cli/policy/schema.py	Adds AuditPolicy.fail_on_drift and new IntegrityPolicy under SecurityPolicy.
src/apm_cli/policy/parser.py	Validates/parses the two new policy keys.
src/apm_cli/policy/inheritance.py	OR-merges the new boolean keys to enforce tighten-not-relax inheritance.
src/apm_cli/install/integrity.py	New integrity enforcement helpers (unhashed_dependencies, enforce_require_hashes).
src/apm_cli/install/pipeline.py	Hooks require_hashes enforcement after lockfile generation.
src/apm_cli/commands/audit.py	Adds _resolve_fail_on_drift() and escalates apm audit exit code on drift when enabled.
tests/unit/install/test_require_hashes.py	Unit tests for fail-closed hash requirement logic.
tests/unit/test_audit_fail_on_drift.py	Unit tests for apm audit exit code escalation when drift is detected.
tests/unit/policy/test_integrity_keys.py	Schema defaults, parsing, validation, and inheritance-tightening tests for the new keys.
tests/unit/policy/test_policy_golden.py	Golden snapshot-based parse-equivalence test over policy fixtures.
tests/fixtures/policy/golden/parsed-policies.json	Updated golden snapshot to include the new default-off fields.
tests/fixtures/policy/apm-policy-*.yml	Adds fixtures covering on/off/combined scenarios for the new keys.
docs/src/content/docs/reference/policy-schema.md	Documents the two new policy keys and their inheritance behavior.
packages/apm-guide/.apm/skills/apm-usage/governance.md	Updates governance guidance with integrity/drift enforcement keys.
CHANGELOG.md	Adds an Unreleased entry describing the new policy keys.

Copilot's findings

• Files reviewed: 19/19 changed files
• Comments generated: 4

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.

[danielmeppiel]

APM Review Panel: ship_now

Declarable integrity keys land a clean, default-off policy surface with zero blocking findings and strong unit coverage -- ship now, track polish post-merge.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

Nine specialists reviewed this PR; none raised a blocking finding. The change is additive, default-off, and wired into an existing enforcement pipeline that already passed 4 review folds. The remaining recommended items cluster into cheap polish: friendlier stderr diagnostics (cli-logging + devx-ux converge on the same ask), two documentation gaps on fail-closed edge cases (doc-writer), two integration-tier tests that would belt-and-suspender the existing 37-test unit suite (test-coverage-expert), and a private-helper relocation agreed by python-architect, auth-expert, and supply-chain-security (three panelists independently flagged _redact_url_credentials coupling -- strong signal, but zero risk today). All of these are trackable as a follow-up PR without regressing any user promise: the unit tests already lock the fail-closed contracts, the code paths are thin boolean gates, and the feature is opt-in. The out-of-band Spec Conformance gate (Mode B silent-extension detector) is a governance decision for the maintainer at merge time -- it tests whether the new policy keys require an OpenAPM spec amendment or waiver, not whether the code is correct. It does not alter the code-quality ship stance.

On test evidence: both test-coverage-expert findings carry evidence blocks with outcome:passed at unit tier (proving the user promises hold on this commit) and outcome:missing at integration tier (proving no fixture-level end-to-end test exists yet). The unit assertions are sound -- they test the actual gate logic, not a mocked boundary -- so the user promises are locked. The missing integration tests are additive coverage on secure_by_default/governed_by_policy surfaces; per the weighting rubric they rank above opinion-only recommended findings but below any blocking defect. Given zero blocking defects and the thin gate logic (15 lines for require_hashes, 3 lines for fail_on_drift), the integration gap does not block merge -- it is the highest-priority follow-up.

Dissent. Three panelists (python-architect, auth-expert, supply-chain-security) independently flagged the _redact_url_credentials cross-module import as a coupling smell. All classified it recommended or nit -- none blocking. The convergence elevates signal but does not elevate severity: the function works correctly today and has a single additional call site. I side with tracking it as the cheapest follow-up (a file move, no logic change). The supply-chain-security nit about the regex-fallback in the except branch is theoretically valid but the triggering condition (urlparse raises on a string containing user:token@) is degenerate; I weight it below the integration-test follow-ups.

Aligned with: Secure by default -- require_hashes enforces fail-closed on missing hashes AND unreadable lockfile, credential redaction tested; default-off means no silent behavior change for existing users, but once opted in the posture is strictly fail-closed. Governed by policy -- both keys are declarable, inherit via logical-OR/tighten-not-relax, and respect the --no-policy escape: declare intent in policy, enforce in pipeline, escape via explicit opt-out. Pragmatic as npm -- default-off preserves today's advisory UX; users who never touch the policy file see zero change, users who opt in get CI-grade enforcement with no wrapper scripts.

Growth signal. Strong adoption story for security-conscious teams: "Reproducible, tamper-evident installs -- declared in policy, enforced in CI, zero wrapper scripts." Worth a Supply-Chain Governance section in the next release announcement. The two-key design (require_hashes for install, fail_on_drift for audit) gives a natural progressive-disclosure ramp: start with audit visibility, then tighten to install-time enforcement.

Panel summary

Persona	B	R	N	Takeaway
Python architect	0	1	1	Clean additive architecture; one coupling smell (private cross-module import) worth relocating to utils.
Cli logging expert	0	1	1	Output UX solid; one recommended: silent exit-code escalation in audit needs a stderr hint for CI users.
Devx ux expert	0	3	2	Default-off keys nest well under security:; error wording needs one-action fix hint; fail_on_drift/--ci overlap should be doc-clarified.
Supply chain security expert	0	0	3	Solid fail-closed design; local exemption safe; fail-open on drift policy discovery defensible for an opt-in escalation key.
Oss growth hacker	0	0	2	Strong story angle for supply-chain governance adoption; CHANGELOG and docs self-serve ready.
Auth expert	0	0	1	Credential redaction in the fail-closed path is correct and tested; no auth/token changes; no leak vectors.
Doc writer	0	2	1	Docs accurate on core behavior; two undocumented fail-closed edges plus a minor cross-link gap.
Test coverage expert	0	2	0	37 unit tests pass; critical-surface gates tested at unit tier; integration-tier coverage is the top additive gap.
Performance expert	0	0	3	No install wall-time regression; default-off path is ~0; enabled path re-reads lockfile (minor reuse opportunity).

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

Top 5 follow-ups

1. [Test coverage expert] Add integration-with-fixtures test for require_hashes fail-closed (real lockfile, real exit code) -- outcome:missing on a secure_by_default surface; locks the user promise against future refactors that move the call site.
2. [Test coverage expert] Add integration-with-fixtures test for fail_on_drift exit-code escalation (real drifted workspace, real exit code) -- outcome:missing on a governed_by_policy surface; same rationale: thin gate but no fixture guards it today.
3. [Cli logging expert] Emit a stderr hint when fail_on_drift escalates exit 0 to 1 (name the policy key and how to disable) -- CI users see advisory output but the job fails with invisible cause; one line closes the debuggability gap.
4. [Doc writer] Document the two fail-closed edges in policy-schema.md: unreadable lockfile + drift-check-could-not-run escalation -- code enforces two triggers not yet described in docs; security-relevant edges should be self-serve discoverable.
5. [Python architect] Relocate _redact_url_credentials to a shared utils module and make it public -- three panelists flagged independently; prevents coupling compounding at the next call site addition.

Architecture

classDiagram
    direction LR
    class ApmPolicy {
        <<ValueObject>>
        +security SecurityPolicy
    }
    class SecurityPolicy {
        <<ValueObject>>
        +audit AuditPolicy
        +integrity IntegrityPolicy
    }
    class AuditPolicy {
        <<ValueObject>>
        +on_install str or None
        +external tuple or None
        +scanners tuple or None
        +fail_on_drift bool
    }
    class IntegrityPolicy {
        <<ValueObject>>
        +require_hashes bool
    }
    class enforce_require_hashes {
        <<Pure>>
        +enforce_require_hashes(deps, enabled) None
        +unhashed_dependencies(deps) list
    }
    class PolicyViolationError {
        <<Exception>>
    }
    class _enforce_require_hashes_pipeline {
        <<IOBoundary>>
        +_enforce_require_hashes(ctx) None
    }
    class _resolve_fail_on_drift_audit {
        <<IOBoundary>>
        +_resolve_fail_on_drift(project_root) bool
    }
    ApmPolicy *-- SecurityPolicy
    SecurityPolicy *-- AuditPolicy
    SecurityPolicy *-- IntegrityPolicy
    _enforce_require_hashes_pipeline ..> IntegrityPolicy : reads policy
    _enforce_require_hashes_pipeline ..> enforce_require_hashes : delegates
    _enforce_require_hashes_pipeline ..> PolicyViolationError : raises
    _resolve_fail_on_drift_audit ..> AuditPolicy : reads policy
    enforce_require_hashes ..> IntegrityPolicy : parameterized by enabled
    note for SecurityPolicy "Composite of frozen siblings;\neach sub-policy merges independently"
    note for enforce_require_hashes "Pure decision fn;\nno I/O, no side effects"
    class IntegrityPolicy:::touched
    class AuditPolicy:::touched
    class enforce_require_hashes:::touched
    class _enforce_require_hashes_pipeline:::touched
    class _resolve_fail_on_drift_audit:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm install CLI"] --> B["run_install_pipeline()\nsrc/apm_cli/install/pipeline.py"]
    B --> C["LockfileBuilder.build_and_save()\n[FS] writes apm.lock.yaml"]
    C --> D{"_enforce_require_hashes(ctx)\npolicy disabled or key off?"}
    D -->|"no-op"| E["continue install"]
    D -->|"enabled"| F["[FS] LockFile.read(lockfile_path)"]
    F --> G{"lockfile is None?"}
    G -->|"yes"| H["raise PolicyViolationError\nFAIL CLOSED"]
    G -->|"no"| I["enforce_require_hashes(deps, enabled=True)\nsrc/apm_cli/install/integrity.py"]
    I --> J{"unhashed_dependencies empty?"}
    J -->|"yes"| E
    J -->|"no"| K["raise RuntimeError -> PolicyViolationError\nFAIL CLOSED"]

    L["apm audit CLI"] --> M["_audit_content_scan()\nsrc/apm_cli/commands/audit.py"]
    M --> N{"drift_failed and exit_code == 0?"}
    N -->|"no"| O["return exit_code as-is"]
    N -->|"yes"| P["_resolve_fail_on_drift(project_root)"]
    P --> Q{"APM_POLICY_DISABLE or discovery error?"}
    Q -->|"yes"| R["return False -- FAIL OPEN"]
    Q -->|"no"| S{"policy.security.audit.fail_on_drift?"}
    S -->|"False"| O
    S -->|"True"| T["exit_code = 1"]

Loading

Recommendation

Zero blocking findings across 9 specialists. Unit coverage locks both user promises on this commit (37 tests, 1.18s, all green). The remaining recommended items are polish and additive coverage -- none alter behavior, none risk regression, and all are cheaper as a focused follow-up PR than as merge-blocking scope creep on a feature that is already default-off and fail-closed-correct. The maintainer should address the out-of-band Spec Conformance gate as a governance decision (spec citation or waiver) orthogonal to code quality. Ship now; open a follow-up issue tracking the 5 items above.

---

Full per-persona findings

Python architect

• [recommended] _redact_url_credentials is a private function imported cross-module -- relocate to shared utils at src/apm_cli/install/integrity.py:52
  install/integrity.py imports _redact_url_credentials from install/mcp/registry.py. A private helper reused across module boundaries couples integrity enforcement to the MCP registry; the function is general-purpose URL sanitization with no MCP logic. Extracting now (2 call sites) prevents compounding coupling.
  Suggested: Move _redact_url_credentials to src/apm_cli/utils/url.py, make it public, and import from there in registry.py and integrity.py.
• [nit] enforce_require_hashes raises RuntimeError rather than a domain exception at src/apm_cli/install/integrity.py:55
  Generic RuntimeError wrapped into PolicyViolationError by the pipeline. Fine today (single caller); define HashIntegrityError if a second caller appears.

Cli logging expert

• [recommended] Silent exit-code escalation needs a stderr hint at src/apm_cli/commands/audit.py:979
  When fail_on_drift escalates exit 0 to 1 there is no stderr message explaining why. A CI user sees advisory-looking findings but the job fails with an invisible cause. Emit a parallel stderr line after setting exit_code=1, e.g. an info line naming security.audit.fail_on_drift.
• [nit] Error message: prefer the exact command at src/apm_cli/install/integrity.py:56
  "Re-run the install..." reads better as "Re-run apm install to regenerate the lockfile with hashes."

Devx ux expert

• [recommended] fail_on_drift error should name the policy key that triggered it at src/apm_cli/commands/audit.py:979
  The user sees exit 1 from apm audit with no indication a policy made the advisory check a hard gate. Add a one-line diagnostic naming security.audit.fail_on_drift and where to disable it.
• [recommended] require_hashes error should give the exact remediation command at src/apm_cli/install/integrity.py:54
  Be explicit about the regenerate command; the intermediate RuntimeError also leaks framework-smell if surfaced raw -- consider raising PolicyViolationError directly or a domain error.
• [recommended] Document fail_on_drift vs --ci relationship in apm audit --help
  Two ways to gate on drift (flag vs policy); --help should cross-mention the policy key for discoverability.
• [nit] integrity vs audit nesting asymmetry is fine; a schema comment helps at docs/src/content/docs/reference/policy-schema.md
  require_hashes under security.integrity, fail_on_drift under security.audit -- semantically correct and already noted in docs.
• [nit] Consider logging which policy file activated require_hashes on failure at src/apm_cli/install/pipeline.py:345
  Naming the ancestor policy file/URL in the error would speed debugging in inheritance chains.

Supply chain security expert

• [nit] _redact_url_credentials fallback returns the original URL on parse error -- theoretical leak on malformed URLs at src/apm_cli/install/mcp/registry.py:58
except (ValueError, TypeError): return url could emit user:token@ if urlparse fails but the string still has credentials. Extremely unlikely; defense-in-depth would regex-strip ://[^@]+@ before the fallback return.
  Suggested: regex fallback strip of ://[^@]+@ before returning the raw URL in the except branch.
  Proof (passed): tests/unit/install/test_require_hashes.py::test_enabled_redacts_credentials_in_message -- proves: standard user:token@host redaction works; the fallback path is reachable only with degenerate URLs.
• [nit] _resolve_fail_on_drift fails open on discovery exception -- defensible but worth a log line at src/apm_cli/commands/audit.py:799
  A bare except Exception: return False swallows policy-discovery failures, reverting to advisory. Correct for an opt-in default-off escalation key, but an operator who opted in gets zero signal the gate was bypassed; a debug/warning log adds observability without changing behavior.
  Suggested: logger.debug/warning in the except branch.
• [nit] No direct test of the _resolve_fail_on_drift exception-handling branch at tests/unit/test_audit_fail_on_drift.py
  Tests mock the return value rather than exercising the swallow logic. A test patching discover_policy_with_chain to raise and asserting False would lock the fail-open contract.
  Suggested: add a test that patches discover_policy_with_chain to raise and asserts False.

Oss growth hacker

• [nit] CHANGELOG entry is dense for a release note at CHANGELOG.md:10
  Two features in one sentence; for the eventual release note consider splitting with a why-you-care framing. Fine for the raw CHANGELOG.
• [nit] Add a one-liner CI snippet to the new section at docs/src/content/docs/reference/policy-schema.md:200
  Show the CI payoff: a single apm audit line noting it exits non-zero on drift when fail_on_drift is enabled closes the loop for a security engineer.

Auth expert

• [nit] Re-export a canonical redactor rather than importing a private helper cross-package at src/apm_cli/install/integrity.py:54
_redact_url_credentials is private in install/mcp/registry.py; importing across sub-packages couples unrelated modules. A thin re-export in utils/ improves discoverability. Functionally correct today -- pure hygiene.

Doc writer

• [recommended] require_hashes docs omit the unreadable/missing-lockfile fail-closed case at docs/src/content/docs/reference/policy-schema.md:167
  pipeline._enforce_require_hashes raises PolicyViolationError when the freshly-written lockfile cannot be read -- a second fail-closed trigger distinct from missing-hash, and the more security-relevant one. Docs describe only the missing-hash trigger.
  Suggested: append "An unreadable or missing lockfile also fails the install closed -- never a silent pass."
• [recommended] fail_on_drift gating surface is narrower in docs than in code at docs/src/content/docs/reference/policy-schema.md:166
  audit.py escalates on any drift-check failure (covers detected drift and a drift check that could not run), while an advisory cache-miss skip does not gate. Docs frame the trigger only as "when drift is detected."
  Suggested: note that any drift-check failure escalates the exit code (including a check that could not run), but an advisory cache-miss skip does not gate.
• [nit] governance.md duplicates the reference field tables without a cross-link at packages/apm-guide/.apm/skills/apm-usage/governance.md:93
  Duplication is defensible but a cross-reference to policy-schema.md keeps the two from drifting.
  Suggested: add a pointer to reference/policy-schema.md as authoritative.

Test coverage expert

• [recommended] No integration test exercises require_hashes fail-closed via a real lockfile
  The install pipeline floor is integration-with-fixtures; unit tests mock LockFile.read. Production logic is a thin 15-line gate and the mocked seam is already validated by the existing install integration suite. Risk is low but non-zero: a future refactor could move the call site unnoticed. Zero matches for "require_hashes" in tests/integration/.
  Suggested: add a parametrized integration test invoking apm install with an unhashed entry + require_hashes:true asserting non-zero exit.
  Proof (passed): tests/unit/install/test_require_hashes.py::TestPipelineEnforceRequireHashes::test_unreadable_lockfile_fails_closed_when_enabled -- proves: install fails closed when require_hashes is on and the lockfile is unreadable [Secure by default]
  self.assertRaises(PolicyViolationError)
  Proof (missing): tests/integration/test_install_require_hashes.py::test_require_hashes_blocks_install_on_unhashed_entry -- proves: a user with require_hashes enabled cannot install a package whose lockfile entry has no content_hash [Secure by default,Governed by policy]
  assert result.exit_code != 0 and 'require_hashes' in result.stderr
• [recommended] No integration test exercises fail_on_drift exit-code escalation via a real audit invocation
  The audit exit-code floor is integration-with-fixtures; unit tests mock _resolve_fail_on_drift and _check_drift. Existing integration tests cover scanner exit codes but none activate fail_on_drift. The gate is 3 lines and the drift pipeline underneath is integration-tested. Risk is low. Zero matches for "fail_on_drift" in tests/integration/.
  Suggested: add a test creating a drifted fixture workspace + fail_on_drift:true asserting SystemExit != 0.
  Proof (passed): tests/unit/test_audit_fail_on_drift.py::test_drift_with_fail_on_drift_exits_nonzero -- proves: apm audit exits non-zero on drift when fail_on_drift is enabled [Secure by default,Governed by policy]
  assert _run(tmp_path, fail_on_drift=True) != 0
  Proof (missing): tests/integration/test_audit_fail_on_drift.py::test_fail_on_drift_policy_exits_nonzero_on_real_drift -- proves: a user running bare apm audit with fail_on_drift sees non-zero exit on workspace drift [Secure by default,Governed by policy]
  assert result.exit_code != 0

Performance expert

• [nit] Re-reading the lockfile from disk when the builder already holds entries in memory at src/apm_cli/install/pipeline.py:_enforce_require_hashes
  The gate calls LockFile.read on the just-written file (~50-200us). The builder already has the entries in memory; passing them avoids a redundant read. Reading the persisted file also gives a verify-what-was-written guarantee -- defensible. Nit only.
• [nit] Default-off fast path is correctly negligible at src/apm_cli/install/pipeline.py:_enforce_require_hashes
  Disabled key: at most 3 getattr + a bool check before return. ~0 cost.
• [nit] Policy discovery gated behind drift_failed -- the clean path is untouched at src/apm_cli/commands/audit.py:967
  _resolve_fail_on_drift only runs when drift_failed and exit_code==0; the common no-drift path never invokes policy discovery. Correct.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

[danielmeppiel]

APM Spec Guardian: fold_and_ship

Scope: editorial-patch; diff = +120/-6 lines across 4 file(s). Shocked-meter avg: 7.75/10.

[+] The panel converges on fold_and_ship: shocked-meter average 7.75/10 with zero blocking findings across all four reviewers. PR #1794 authors the OpenAPM v0.1 spec citation for the two declarable integrity keys (security.integrity.require_hashes -> req-pl-013, security.audit.fail_on_drift -> req-pl-014), both MUST in the governance conformance class under a new Section 6.8. The one convergent recommended finding -- binding the under-specified phrase "recorded integrity hash" to the concrete lockfile field the implementation actually checks -- has been folded in 62258eb7. Remaining items are single-panel polish deferred to v0.1.1; one ordering recommendation is rejected because it would make the spec contradict the implementation.

Convergence

Panel	Verdict	Shocked	New B	New R	New N
Spec Oci Editor	ship	8/10	0	1	1
Spec Swagger Editor	ship	8/10	0	1	2
Spec Pkgmgr Editor	ship	7/10	0	1	1
Spec Tag Architect	ship	8/10	0	2	1

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

Convergent themes (flagged by 2+ panels)

• ct-1 -- "recorded integrity hash" was not bound to a concrete lockfile field (supporting: pkg-001, oci-r1-001) -- FOLDED in 62258eb7.

Fold now (1 item)

1. [ct-1 / fidelity] 6.8 (req-pl-013) + 6.3.6 -- Bind the phrase "recorded integrity hash" to the content_hash lockfile field that src/apm_cli/install/integrity.py inspects for every non-local entry, so two conformant implementations cannot diverge on which field satisfies the fail-closed predicate. (The panels proposed a per-source-type table; that was narrowed to the single content_hash field because the implementation checks exactly one field uniformly -- a per-source-type table would over-specify beyond the code.)
   Success criterion: req-pl-013 and 6.3.6 name content_hash; orphan_check stays 89-aligned; spec pytest green

Defer to v0.1.1

• [sw-rec-r1-1 / testability] 6.8 (req-pl-014) -- add an informative note that conformance suites must exercise both the enabled (non-zero on drift) and default (report-only) code paths.
• [tag-r02 / layering] 6.3.6 -- the non-normative field reference restates the logical-OR merge rule that already lives in the req-pl-006 merge table; collapse to a pure cross-reference for single-source-of-truth.
• [pkg-n01 / precision] 6.3.6 -- prefer the exact field name local_deployed_file_hashes over the generic "deployed-file hashes".
• [oci-n1-001 / traceability] 6.8 + 6.3.6 -- cross-reference the lockfile trust-anchor requirement (req-lk-012) from the local-dep exemption clause.
• [sw-nit-r1-1 + tag-n02 / housekeeping] Appendix D 0.1.3 -- after the sibling spec-citation PR (feat(audit): surface unmanaged artifacts with reason, type, and deny-conflict #1793) lands, strip the transient cross-PR coordination NOTE from the revision-history cell.

Rejected findings

• tag-r01 -- The recommendation to add an ordering MUST ("evaluated after dependency resolution and BEFORE file materialization") is rejected: the require_hashes enforcement in src/apm_cli/install/pipeline.py runs AFTER the integrate/materialization phase, so the spec deliberately says "at the point of the check" with no ordering claim. Adding a pre-materialization ordering MUST would make the normative text false against the implementation -- the opposite of the spec-vs-code fidelity this panel exists to protect.

Linter handoff: All 11 guardian linter checks pass on the post-fold artifact: anchor regex (89 strict anchors), manifest/Appendix-C/pytest-marker 4-way alignment, count sites consistent at 89 (84 MUST, 5 SHOULD) across Section 1.3, Appendix C trailer, and Appendix D, and the 6.8 -> 6.9 renumber introduces zero new broken links on the rendered slug model. Check 11 (modified .py files) is expected for this code+spec PR and is covered by the concurrent apm-review-panel run.

---

Full per-panel findings

Spec Oci Editor -- shocked_meter 8/10, confidence high

Summary: Clean additive editorial patch. The req-pl-013 fail-closed posture is sound (absent/unreadable lockfile trips the gate, non-local deps checked, local deps exempt via deployed-file hashes). The req-pl-014 advisory-skip-does-not-gate is a correct fail-closed posture. One recommended finding on per-source-type field binding for "recorded integrity hash" (now folded); one nit on cross-referencing the local-dep trust anchor.

New recommended findings (1)

• [oci-r1-001] 6.8 (req-pl-013) -- "recorded integrity hash" did not bind to a concrete lockfile field; a literal reading could let git-sourced entries pass while gating only registry entries.
  Recommended fix: Name the field(s) that satisfy the predicate. (Folded as content_hash in 62258eb.)

New nit findings (1)

• [oci-n1-001] Local-dep exemption rationale does not cross-reference req-lk-012/req-lk-017 -- fix: append (see [req-lk-012]). (Deferred v0.1.1.)

Preserved strengths confirmed

• Hash-envelope anchoring (req-lk-016) intact; fail-closed extraction posture (req-lk-013) still ordered before integration; supply-chain threat-to-req mapping remains comprehensive.

Spec Swagger Editor -- shocked_meter 8/10, confidence high

Summary: Clean additive editorial-patch. Both new governance MUST anchors are correctly enumerated across all count sites (Section 1.3, Section 6.9 trailer, Section 11.3.4 Governance, Appendix C + trailer) and the requirements manifest. Cross-references to the new Section 6.8 resolve; the 6.8 -> 6.9 renumber is fully propagated. RFC 2119 keyword discipline is sound.

New recommended findings (1)

• [sw-rec-r1-1] 6.8 (req-pl-014) -- the final clause binds an obligation on the default (off) state, so the requirement is only fully tested by exercising both on- and off-paths; suggest an informative testing note.
  Recommended fix: Add an informative "exercise both paths" note. (Deferred v0.1.1.)

New nit findings (2)

• [sw-nit-r1-1] Appendix D 0.1.3 row ends with a transient sibling-PR NOTE that becomes noise post-merge. (Deferred v0.1.1.)
• [sw-nit-r1-2] 6.3.6 uses lowercase "fails closed" informatively, shadowing the normative MUST. (No change required.)

Preserved strengths confirmed

• Count-site consistency (89 / 84-MUST / 5-SHOULD); conformance-class enumeration completeness; monotonic anchor numbering; cross-reference integrity after renumber; RFC 2119 discipline.

Spec Pkgmgr Editor -- shocked_meter 7/10, confidence high

Summary: Clean additive editorial-patch. The two new merge-table rows exactly match the inheritance.py boolean-OR (tighten-not-relax) semantics. The req-pl-014 drift definition aligns with the existing lockfile drift model. One recommended finding on field-binding for "recorded integrity hash" (now folded); one nit on field-name precision.

New recommended findings (1)

• [pkg-001] 6.8 (req-pl-013) -- "recorded integrity hash" not bound to a specific lockfile field per source type; ambiguity could let two compliant-by-reading implementations diverge.
  Recommended fix: Name the field(s) constituting the integrity hash. (Folded as content_hash in 62258eb.)

New nit findings (1)

• [pkg-n01] "deployed-file hashes" is ambiguous vs the schema term local_deployed_file_hashes. (Deferred v0.1.1.)

Preserved strengths confirmed

• Lockfile determinism model intact; merge-table semantics match code exactly; reserved-slot discipline maintained; producer/consumer/governance class separation preserved.

Spec Tag Architect -- shocked_meter 8/10, confidence high

Summary: Clean additive editorial-patch. The two keys are layered correctly: non-normative 6.3.6 defers all normative force to a new Section 6.8 inserted before the conformance trailer (now 6.9). Cross-references, Appendix C rows, manifest entries, and counts are consistent. The security.integrity / security.audit namespaces leave clean forward-compatibility room for deferred signatures/provenance/SBOM work.

New recommended findings (2)

• [tag-r01] 6.8 (req-pl-013) -- recommends naming the phase boundary for the check (after resolution, before file materialization). (Rejected -- contradicts the implementation, which enforces after integrate; see Rejected findings above.)
• [tag-r02] 6.3.6 -- the field reference restates merge semantics that already live in the req-pl-006 table; defer to the table for single-source-of-truth. (Deferred v0.1.1.)

New nit findings (1)

• [tag-n01] 6.8 opening repeats "default-off"/"opt-in" already stated in 6.3.6. (Deferred v0.1.1.)

Preserved strengths confirmed

• Manifest stays in sync with Appendix C; section-number cross-references consistent after renumber; extension-model namespace leaves clean v0.2 room; non-normative 6.3.6 defers normative force to 6.8; tighten-not-relax merge invariant upheld.

_{This panel is advisory. It does not block merge. Re-apply the spec-review label after addressing feedback to re-run.}

[danielmeppiel]

APM Review Panel: ship_now

Zero blocking findings across 9 panelists; additive default-OFF security gates with fail-closed semantics, spec-backed citations, full CI green, and 35+2 tests. Ship now with non-blocking polish tracked.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All nine specialists converged: this is a clean, additive, default-OFF pair of security policy keys that fail closed when enabled, merge by logical OR (no downgrade path), and carry zero auth-surface impact. The supply-chain-security expert affirmatively certified the fail-closed semantics and the OR-merge inheritance model. The auth expert certified no credential/token surface was touched. The test-coverage expert confirmed solid unit coverage while noting the absence of integration-tier tests -- but explicitly marked both findings NOT blocking given the additive, default-off nature of the feature. The single highest-signal hardening item is the env-var truthy-vs-eq1 inconsistency flagged by supply-chain (audit.py:793): this is a real fail-open footgun but only reachable when a user explicitly sets APM_POLICY_DISABLE to a non-empty value other than '1', and the gate itself is default-OFF, so the blast radius is narrow and post-merge fixable. The cross-module private-import smell (python-architect) is a maintenance hazard but not a correctness or security defect. The audit-exit-visibility gap (cli-logging-expert, devx-ux-expert) is a UX polish item that does not change observable correctness. Prior gates (apm-review-panel ship_now, apm-spec-guardian fold_and_ship with shocked_meter 7.75, CI fully green) all confirm this code is production-ready. No panelist disagreed on shippability; the only question is whether the env-var inconsistency warranted blocking -- the supply-chain expert explicitly did NOT block, and I concur: the feature is off-by-default, the env-var is a debug escape hatch, and the fix is a one-line alignment best landed as an immediate follow-up.

Dissent. No substantive disagreement between panelists. The only potential tension -- whether the env-var truthy check warranted blocking -- was resolved by the supply-chain expert themselves marking it explicitly NOT blocking, and by the narrow blast radius (default-off feature, debug escape hatch, one-line fix). The test-coverage expert's missing-integration findings carry secure-by-default principle weight, but both findings were explicitly self-classified as non-blocking given the additive default-off nature; I concur and rank them fourth in follow-up priority.

Aligned with: secure-by-default -- both keys default OFF (no existing user impacted) and fail closed when enabled (missing hash = hard stop, indeterminate drift = escalated exit); governed-by-policy -- declarable in apm.yml under the security namespace, inheritable via OR-merge, spec-backed by req-pl-013/014; portability-by-manifest -- policy state lives in the manifest and is version-controllable; breaking-change discipline -- purely additive, default-off, no existing behaviour altered.

Growth signal. Release story angle: declarable, spec-backed supply-chain integrity in two YAML lines -- default-off, tighten-only inheritance. Target the enterprise-security audience; reinforce the spec-is-the-contract narrative with req-pl-013/014 citations.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Clean additive design; frozen-dataclass schema, OR-merge inheritance, fail-closed enforcement. One cross-module private-import is the only structural smell.
CLI Logging Expert	0	1	1	Install fail-closed errors route correctly through PolicyViolationError + CommandLogger; audit exit-code escalation is silent in text mode.
DevX UX Expert	0	2	1	Two well-scoped opt-in default-off booleans; naming/merge/defaults align with conventional package-manager mental models.
Supply Chain Security Expert	0	1	1	Both keys correctly fail-closed and default-OFF; gates on missing/empty hash and unreadable lockfile and indeterminate drift; OR-merge prevents downgrade.
OSS Growth Hacker	0	0	1	Clean additive PR that strengthens the governed-by-policy / secure-by-default-opt-in positioning; CHANGELOG is capability-framed.
Auth Expert	0	0	0	pipeline.py change is auth-adjacent by location but touches no authentication behaviour; no token/credential surface read or logged.
Doc Writer	0	2	2	Docs/spec accurately describe the code; spec count math 87->89 internally consistent. Minor completeness/consistency gaps only.
Test Coverage Expert	0	2	0	Solid unit coverage (35 tests); no integration-tier test exercises the full install pipeline or audit command end-to-end. Recommended follow-up.
Performance Expert	0	0	1	Effectively zero-cost on the default-OFF path; when ON, an avoidable lockfile re-parse but otherwise a pure O(n) string-presence scan. No hot-path regression.

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

Top 5 follow-ups

1. [Supply Chain Security Expert] Align APM_POLICY_DISABLE env-var check to == '1' -- audit.py:793 uses truthy os.environ.get instead of == '1' like the rest of the codebase; any non-empty value (including '0' or 'false') disables the gate, an accidental fail-open footgun. One-line fix, highest signal.
2. [Python Architect] Promote _redact_url_credentials to a public util -- cross-module import of a private helper from .mcp.registry into install/integrity.py couples the integrity gate to a private function in an unrelated sub-package; a rename there silently breaks the error path. Promote to apm_cli.utils or inline.
3. [CLI Logging Expert] Emit a verdict line on audit exit-code escalation in text mode -- when fail_on_drift flips exit 0 to 1 in text format, no final line explains the escalation; a terminal user sees drift output but no verdict. One stderr line closes the gap.
4. [Test Coverage Expert] Add integration tests for require_hashes and fail_on_drift end-to-end -- 35 unit tests prove logic with mocked boundaries; integration tests through the real install pipeline and audit command would catch call-site wiring drift. Non-blocking given default-off, but valuable as adoption grows.
5. [Doc Writer] Sync policy-schema.md and the published JSON Schema -- reference docs omit the indeterminate-drift escalation behaviour the spec and code implement, and the published JSON Schema does not yet encode the two new keys. Cosmetic consistency gap, not a correctness issue.

Architecture

classDiagram
    direction LR
    class ApmPolicy {
      <<ValueObject>>
      +security SecurityPolicy
    }
    class SecurityPolicy {
      <<ValueObject>>
      +audit AuditPolicy
      +integrity IntegrityPolicy
    }
    class AuditPolicy {
      <<ValueObject>>
      +on_install str
      +fail_on_drift bool
    }
    class IntegrityPolicy {
      <<ValueObject>>
      +require_hashes bool
    }
    class PolicyViolationError {
      <<Exception>>
    }
    class enforce_require_hashes {
      <<Pure>>
      +__call__(deps, enabled) None
    }
    class unhashed_dependencies {
      <<Pure>>
      +__call__(deps) list
    }
    class _enforce_require_hashes_pipeline {
      <<IOBoundary>>
      +__call__(ctx) None
    }
    class _resolve_fail_on_drift {
      <<IOBoundary>>
      +__call__(project_root) bool
    }
    class LockFile {
      +read(path) LockFile
      +get_package_dependencies() list
    }
    class LockedDependency {
      <<ValueObject>>
      +source str
      +content_hash str
      +repo_url str
    }
    ApmPolicy *-- SecurityPolicy
    SecurityPolicy *-- AuditPolicy
    SecurityPolicy *-- IntegrityPolicy
    _enforce_require_hashes_pipeline ..> LockFile : reads
    _enforce_require_hashes_pipeline ..> enforce_require_hashes : delegates
    _enforce_require_hashes_pipeline ..> PolicyViolationError : raises
    enforce_require_hashes ..> unhashed_dependencies : delegates
    enforce_require_hashes ..> LockedDependency : inspects
    _resolve_fail_on_drift ..> ApmPolicy : reads
    note for IntegrityPolicy "OR-merge in inheritance: once true stays true"
    note for AuditPolicy "fail_on_drift: OR-merge, governs audit exit code"
    class IntegrityPolicy:::touched
    class AuditPolicy:::touched
    class enforce_require_hashes:::touched
    class unhashed_dependencies:::touched
    class _enforce_require_hashes_pipeline:::touched
    class _resolve_fail_on_drift:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm install (CLI entry)"] --> B["run_install_pipeline"]
    B --> C["LockfileBuilder.build_and_save [FS]"]
    C --> D{"policy.security.integrity.require_hashes?"}
    D -- off/no-policy --> E["continue to deploy phase"]
    D -- on --> F["_enforce_require_hashes(ctx) [FS]"]
    F --> G["LockFile.read(lockfile_path) [I/O]"]
    G -- None --> H["raise PolicyViolationError (fail-closed)"]
    G -- ok --> I["enforce_require_hashes(deps, enabled=True)"]
    I --> J{"unhashed_dependencies empty?"}
    J -- yes --> E
    J -- no --> K["raise RuntimeError -> PolicyViolationError"]
    K --> L["pipeline error handler renders + exits 1"]
    H --> L
    M["apm audit (CLI entry)"] --> N["_audit_content_scan"]
    N --> O{"drift_failed and exit_code==0?"}
    O -- no --> P["return exit_code"]
    O -- yes --> Q["_resolve_fail_on_drift(project_root) [I/O]"]
    Q --> R{"policy.security.audit.fail_on_drift?"}
    R -- True --> S["exit_code = 1"]
    R -- False/error --> P
    S --> P

Loading

sequenceDiagram
    participant User
    participant CLI as apm install
    participant Pipeline as run_install_pipeline
    participant Lock as LockfileBuilder
    participant Gate as _enforce_require_hashes
    participant Integrity as enforce_require_hashes
    User->>CLI: apm install
    CLI->>Pipeline: run_install_pipeline(ctx)
    Pipeline->>Lock: build_and_save()
    Lock-->>Pipeline: lockfile written
    Pipeline->>Gate: _enforce_require_hashes(ctx)
    Gate->>Gate: check policy enabled
    alt require_hashes OFF
        Gate-->>Pipeline: no-op return
    else require_hashes ON
        Gate->>Gate: LockFile.read(path)
        Gate->>Integrity: enforce_require_hashes(deps, enabled=True)
        alt all hashed
            Integrity-->>Gate: return (pass)
            Gate-->>Pipeline: return
        else missing hashes
            Integrity-->>Gate: raise RuntimeError
            Gate-->>Pipeline: raise PolicyViolationError
            Pipeline-->>CLI: exit 1
        end
    end
    Pipeline-->>CLI: continue deploy

Loading

Recommendation

Ship now. Zero blocking findings from any of the 9 panelists. Both keys are additive, default-OFF, fail-closed, spec-backed, and covered by 35 unit + 2 spec-conformance tests with full CI green. The recommended follow-ups above are non-blocking polish items that do not change the ship decision and can land as a fast-follow PR within the same release cycle.

---

Full per-persona findings

Python Architect

• [recommended] Cross-module import of private helper _redact_url_credentials at src/apm_cli/install/integrity.py:52
  integrity.py imports a private function (leading underscore) from .mcp.registry, an unrelated sub-package; a rename or refactor there silently breaks the error path, and there is no public contract guaranteeing stability.
  Suggested: Promote _redact_url_credentials to a public utility (e.g. apm_cli.utils.url.redact_url_credentials) or inline the trivial implementation.
• [nit] Docstring references the re-export path for PolicyViolationError at src/apm_cli/install/pipeline.py:325
  The canonical definition lives in apm_cli.install.errors; pointing at the back-compat re-export may confuse a reader grepping for the source of truth.
• [nit] fail_on_drift placement asymmetry at src/apm_cli/policy/schema.py:194
fail_on_drift is a field on AuditPolicy while require_hashes gets its own new IntegrityPolicy; the asymmetry is intentional but worth one clarifying inline comment.

CLI Logging Expert

• [recommended] Silent exit-code escalation in text-mode audit at src/apm_cli/commands/audit.py:979
  When fail_on_drift flips the exit code 0->1 in text format, no message explains the escalation; the verdict (pass/fail) should be the last thing printed.
  Suggested: Append a one-liner after setting exit_code=1, e.g. [x] drift detected -- exit escalated by security.audit.fail_on_drift.
• [nit] Fix hint circular in lockfile-unreadable case at src/apm_cli/install/pipeline.py:348
  The message says re-run install while the user is already inside install; consider "delete the corrupt lockfile and re-run" or drop the hint.

DevX UX Expert

• [recommended] Exit-code help text omits policy-driven drift escalation at src/apm_cli/commands/audit.py:1199
  The --help docstring says drift-only is advisory (exit 0); with fail_on_drift enabled that same drift yields exit 1, but --help is silent.
  Suggested: Note that exit 1 also covers policy-gated drift.
• [recommended] Error message suggests re-running the install that just failed at src/apm_cli/install/integrity.py:55
  Enforcement runs inside the same install pipeline that just wrote the lockfile; "re-run install" can loop. The dep-name list is good; the recovery action needs to be achievable on the first try.
• [nit] Table column key path mismatch at docs/src/content/docs/reference/policy-schema.md:168
  The row reads integrity.require_hashes but other rows are relative to the security section; add the security. prefix or note the implicit scope.

Supply Chain Security Expert

• [recommended] _resolve_fail_on_drift uses a truthy check for APM_POLICY_DISABLE at src/apm_cli/commands/audit.py:793
  The rest of the codebase tests == "1"; bare truthy means any non-empty value (e.g. '0', 'false') disables the gate -- an accidental fail-open footgun.
  Suggested: Align to == "1".
• [nit] _resolve_fail_on_drift swallows all exceptions returning False at src/apm_cli/commands/audit.py:799
  The design intent (transient failure must not convert advisory drift into a hard failure) is documented; a logging.debug line would let operators diagnose a permanently-open gate. Explicitly not blocking.

OSS Growth Hacker

• [nit] CHANGELOG entry is dense at CHANGELOG.md:10
  For the release narrative, consider a one-sentence capability hook before the detail so it is quotable by security-minded adopters.

Auth Expert

No findings. pipeline.py change is auth-adjacent by location but touches no authentication behaviour; the new call site operates purely on lockfile hash-presence and policy, never reading, writing, passing, or logging credentials. The error path uses _redact_url_credentials to sanitise URLs.

Doc Writer

• [recommended] policy-schema.md fail_on_drift description omits the indeterminate-drift escalation at docs/src/content/docs/reference/policy-schema.md:166
  audit.py escalates on drift_check.passed is False, which covers both detected drift AND a drift check that could not run; the reference page should be at least as complete as the normative spec on observable exit behaviour.
• [recommended] Published policy-v0.1.schema.json does not encode the two new keys at docs/src/content/docs/specs/openapm-v0.1.md:1335
  openapm-v0.1.md now asserts these as v0.1 governance MUSTs while the companion JSON Schema omits them; the runtime validator already enforces the keys, so this is a corpus-consistency gap, not a runtime defect.
  Suggested: Add the keys to the JSON Schema or note its encoding is advisory/deferred.
• [nit] YAML comment drops the non-local qualifier at docs/src/content/docs/reference/policy-schema.md:203
  integrity.py skips source == "local" entries; the unqualified comment could read as if local deps are also gated.
• [nit] Exit-status wording inconsistent at docs/src/content/docs/reference/policy-schema.md:210
  "escalates the exit code to 1" is correct only when there are no content findings; the spec's "non-zero" phrasing is the precise contract.

Test Coverage Expert

• [recommended] Install pipeline lacks an integration test for the require_hashes fail-closed gate
  Unit tests exercise the gate with mocked LockFile.read; no integration test wires a real policy fixture through run_install_pipeline with a lockfile missing a hash to prove the gate fires at the correct phase.
  Proof (missing at): tests/integration/test_install_require_hashes_e2e.py -- proves: install fails closed when a non-local lockfile entry lacks a content hash and require_hashes is on [secure-by-default]
• [recommended] Audit command lacks an integration test for fail_on_drift exit-code escalation
  Unit tests mock _resolve_fail_on_drift; no integration test exercises apm audit end-to-end with fail_on_drift enabled and a drifted workspace to confirm the non-zero exit propagates.
  Proof (missing at): tests/integration/test_audit_fail_on_drift_e2e.py -- proves: apm audit exits non-zero when workspace content drifts and fail_on_drift is enabled end-to-end [secure-by-default,governed-by-policy]

Performance Expert

• [nit] Avoidable lockfile re-parse when require_hashes is ON at src/apm_cli/install/pipeline.py:342
  The gate re-parses the YAML lockfile from disk immediately after LockfileBuilder just wrote it; the resolved deps are already in memory. Cosmetic today (default-off, page-cache-hot) but material if the key is ever promoted to default-ON.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}