fix(policy): carry all ApmPolicy fields through inheritance merge#1791
Merged
Conversation
merge_policies omitted fetch_failure, registry_source, and bin_deploy when reconstructing ApmPolicy, so a policy that extends another silently reset those three fields to defaults. Add _merge_fetch_failure, _merge_registry_source, and _merge_bin_deploy with tighten-not-relax semantics and wire them into merge_policies. Add a structural coverage guard so any future ApmPolicy field forgotten in the merge fails a test. Part of #1774 (prerequisite for #1778) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a correctness bug in policy inheritance merging where merge_policies reconstructed ApmPolicy but unintentionally dropped fetch_failure, registry_source, and bin_deploy, causing inherited policies to revert those sections to defaults.
Changes:
- Extend
merge_policiesto carry and restrict-only mergefetch_failure,registry_source, andbin_deploy. - Add targeted unit tests for each new merge behavior plus a structural “field coverage” guard to prevent future omitted-field regressions.
- Add a CHANGELOG entry documenting the fix.
Show a summary per file
| File | Description |
|---|---|
| src/apm_cli/policy/inheritance.py | Adds merge helpers for fetch_failure, registry_source, and bin_deploy and wires them into merge_policies. |
| tests/unit/policy/test_inheritance.py | Adds behavioral tests for the new merge semantics and a structural guard ensuring merge completeness across ApmPolicy fields. |
| CHANGELOG.md | Documents the inheritance fix in the Unreleased “Fixed” section. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
|
|
||
| ### Fixed | ||
|
|
||
| - Policy inheritance no longer drops `fetch_failure`, `registry_source`, and `bin_deploy` when a policy `extends` another; these fields now carry and tighten through the merge like sibling sections. (closes #1778) (#1791) |
This was referenced Jun 16, 2026
danielmeppiel
added a commit
that referenced
this pull request
Jun 16, 2026
…l-transparency, classifier precedence Fold three Copilot review findings on the unmanaged-artifacts check: - Symlink-dir traversal guard (security): switch the scan from Path.rglob (which can recurse into directory symlinks) to os.walk(followlinks=False) -- the house pattern from security/gate.py -- so a symlinked directory resolving outside the workspace is never traversed. The file-symlink escape guard now applies only to file symlinks. Adds a regression test asserting a symlinked dir's contents never surface. - exclude null-transparency: parser now mirrors the deny/require pattern so `exclude: null` (or an absent key) -> None (transparent in merge), `exclude: []` -> () (explicit override), `exclude: [..]` -> tuple. Previously `null` collapsed to (), breaking the documented inheritance semantics. Still passes the #1791 coverage guard. - Classifier precedence: explicit filename conventions (.agent.md / .instructions.md / mcp.json / SKILL.md) now win before directory-segment hints, and MCP detection is narrowed to known config filenames / a `.mcp/` root -- so a path under a dir merely named `mcp` (e.g. .github/agents/mcp/rogue.agent.md) classifies as agent, not mcp. Strict TDD (red tests first), mutation-break verified on each new assert. ASCII-only, report-only. Part of #1774 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added a commit
that referenced
this pull request
Jun 16, 2026
…conflict (#1793) * feat(audit): surface unmanaged artifacts with reason, type, and deny-conflict Extend the existing _check_unmanaged_files policy check (one unified report, not a parallel scan engine) so apm audit answers "what is here that APM did not put here?": - Enrich each finding with a factual reason ("not tracked in apm.lock.yaml") and a deny-conflict note ("matches deny rule (<pattern>)") when the path matches the policy's own dependencies.deny / mcp.deny. - Lazily classify the primitive type (skill/agent/instruction/mcp) of the already-flagged files only -- never the whole tree. - Surface deny-conflicts through a shared first_matching_pattern matcher reused by the dependency/MCP deny-list checks (no second matcher). - Add unmanaged_files.exclude (glob allow-list) to suppress known harness-managed paths; carried through inheritance merge as a union. - Guard traversal against symlinks escaping the workspace. This is drift / divergence visibility, not supply-chain-attack prevention -- the lockfile is hand-editable YAML. Report only; APM never removes or blocks a flagged file. Part of #1774 Closes #1775 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(changelog): record PR number for unmanaged-artifacts feature Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(audit): address PR #1793 review -- symlink-dir guard, exclude null-transparency, classifier precedence Fold three Copilot review findings on the unmanaged-artifacts check: - Symlink-dir traversal guard (security): switch the scan from Path.rglob (which can recurse into directory symlinks) to os.walk(followlinks=False) -- the house pattern from security/gate.py -- so a symlinked directory resolving outside the workspace is never traversed. The file-symlink escape guard now applies only to file symlinks. Adds a regression test asserting a symlinked dir's contents never surface. - exclude null-transparency: parser now mirrors the deny/require pattern so `exclude: null` (or an absent key) -> None (transparent in merge), `exclude: []` -> () (explicit override), `exclude: [..]` -> tuple. Previously `null` collapsed to (), breaking the documented inheritance semantics. Still passes the #1791 coverage guard. - Classifier precedence: explicit filename conventions (.agent.md / .instructions.md / mcp.json / SKILL.md) now win before directory-segment hints, and MCP detection is narrowed to known config filenames / a `.mcp/` root -- so a path under a dir merely named `mcp` (e.g. .github/agents/mcp/rogue.agent.md) classifies as agent, not mcp. Strict TDD (red tests first), mutation-break verified on each new assert. ASCII-only, report-only. Part of #1774 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(audit): correct deny-rule doc example, add next-action hint, harden type trap Fold the apm-review-panel advisory on the unmanaged-files audit: - docs(policy-reference): the deny-conflict example claimed `matches deny rule (rogue*)` for `.claude/skills/rogue/SKILL.md`, but the matcher fully anchors patterns (`rogue*` -> `^rogue[^/]*$`) and the conflict check tests only the rel path and basename -- neither starts with `rogue`, so that line could never be emitted. Use `**/rogue/**`, which matches via the `/rogue/` segment so the documented output is reproducible. - docs(policy-reference): add `.kiro` to the documented default governance directories so the list matches `_DEFAULT_GOVERNANCE_DIRS`; note the `mcp` directory-name narrowing on the type-tag bullet. - feat(policy): append one next-action hint to a non-empty unmanaged-files report (track with `apm install <ref>` / suppress via `unmanaged_files.exclude`) so a flagged file is self-resolving. Covered by a new regression-trap test (mutation-break verified). - docs(policy-schema): correct the `exclude` default to `null` (null-transparent in the extends merge) to match the model. - test(policy): tighten the type-tag assertion to `[type: agent]` so the trap fails if the tag is dropped (the path itself contains `agent`). - docs(code): clarify in the docstring that the dependency deny side is defaults-inclusive (`effective_deny`) while MCP uses raw `mcp.deny`, and document the basename fallback intent. Report-only drift-visibility behavior is unchanged; the traversal and matcher logic is untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(spec): cite req-pl-015 for unmanaged-artifact surfacing Author OpenAPM v0.1 requirement req-pl-015 (Section 6.3.5, governance MUST) codifying the unmanaged-artifact surfacing behavior added in this PR, resolving the Mode B spec-conformance gate honestly (spec is the contract) rather than waiving it. req-pl-015 governs reporting COMPLETENESS only: a governance implementation evaluating policy over a populated primitive target tree MUST surface every file under a managed primitive target directory that is neither recorded in apm.lock.yaml nor matched by a configured unmanaged_files.exclude glob, each carrying its unmanaged reason, a dependency/MCP deny-conflict note where applicable, and a determinable inferred primitive type; an excluded path MUST NOT be surfaced. Enforcement stays governed by unmanaged_files.action -- this is not an enforcement claim. Citation ritual (all sites edited for orphan_check + linter-check-6): - Section 6.3.5 anchor + normative prose. - Section 6.4 merge table: unmanaged_files.exclude row (union, dedup, parent order preserved -- matches inheritance._union). - Section 1.3 + Appendix C count 87 -> 88 (83 MUST); Appendix C row; Section 6.8 governance trailer; Appendix D erratum row. - Manifest entry after req-pl-012; regenerated CONFORMANCE.{md,json}. - New @pytest.mark.req("req-pl-015") behavioral test binding the spec MUST to _check_unmanaged_files (reason, type, deny-conflict, exclude). Cross-PR note: sibling spec-citation PR #1794 also edits the shared count sites; whichever merges second needs a trivial count reconcile. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(spec): fold guardian findings on req-pl-015 (sub-clauses, absent-type, additive exclude) Restructure req-pl-015 into lettered sub-clauses (a)/(b)/(c) so each obligation is individually citable; add the absent-type omission MUST; clarify the deny-conflict note is supplemental enrichment; cross-ref the exclude matcher to Section 6.5 (no new dialect pin); qualify the 6.4 merge-table exclude row as additive union (child cannot clear parent; null and [] both preserve parent); relabel the Appendix D row as a semver-zero normative addition (not an informative erratum); reserve req-pl-013/014 in the manifest for concurrent in-flight work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(spec): fold guardian round-2 editorial polish on req-pl-015 Apply the three zero-risk fold-now items from the apm-spec-guardian round-2 synthesis (ship_decision=fold_and_ship, shocked_meter_avg=8.0, zero blocking): state the exclude-merge dedup key is byte-exact on each pattern's UTF-8 string; remove an orphan line break in sub-clause (b); replace the em-dash in the (b) reason clause with a parenthetical. No normative, anchor, count, or conformance-test change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fold(review-panel): clarify exclude additive-merge in reference docs; type-annotate locals apm-review-panel re-run folds (in-scope, non-blocking): - [recommended/doc-writer] policy-schema.md + policy-reference.md exclude merge rows now state additive-only: null AND [] both preserve the parent list (unlike deny/require, a child cannot clear an inherited exclude), matching the openapm-v0.1 6.4 merge-table row and _merge_unmanaged_files. - [nit/python-architect] annotate deployed/deployed_dir_prefixes locals as set[str] / list[str] in _check_unmanaged_files. Declined (noted): doc-writer (a) absolute-framing carve-out nit -- guardian already passed the absolute wording; the 'when it evaluates policy' preamble covers symlink-escape/scan-cap degraded modes; low value vs reopening blessed spec prose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
merge_policiesinsrc/apm_cli/policy/inheritance.pyreconstructs anApmPolicy(...)but omitted three fields that exist on the dataclass:fetch_failure,registry_source, andbin_deploy. As a result, whenever a policy inherits viaextends, those three fields were silently reset to their defaults instead of being carried and merged. This is a live correctness bug today (an org floor onbin_deploy.deny_allorregistry_source.allow_non_registrywould vanish at the repo level) and a structural landmine for every future top-level policy field.Fix
Added three section-merge helpers consistent with the existing tighten-not-relax style and wired them into
merge_policies:_merge_fetch_failure- escalates on awarn < blockladder (block wins; a silent child cannot relax a parent'sblock)._merge_registry_source-requireis union-merged (child can add mandated registries, never drop a parent's);allow_non_registryis AND-merged (once any ancestor setsFalse, a child cannot relax it)._merge_bin_deploy-deny_allOR-merges (any ancestor's kill-switch sticks);denyis union-merged.Test seams added
TestFetchFailureEscalation,TestRegistrySourceMerge,TestBinDeployMerge): a parent that sets each non-default field, merged with a silent child, preserves the parent value; and a child that tightens each wins, while a child cannot relax a parent floor.TestMergeFieldCoverageGuard): builds a parent whose every mergeable field is non-default, merges with a default child, and asserts no field reverted to its default - so a future field added toApmPolicybut forgotten inmerge_policiesMUST fail. A companion meta-test asserts the sample covers exactlyfields(ApmPolicy)minus{name, version, extends}.Validation evidence
bin_deployfrom the merge fails both the behavioral tests and the structural guard; restored.ruff check,ruff format --check,pylint R0801(10.00/10), andscripts/lint-auth-signals.shall clean. ASCII-only.Part of #1774 (prerequisite for #1778)