fix: policy inheritance preserves parent unmanaged_files when child omits block (closes #1198)#1253
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…1253) Child policies that omit the unmanaged_files block no longer silently downgrade the parent's action. The omission is now treated as 'no opinion' (None) rather than an explicit 'ignore', so the parent's setting passes through unchanged during merge. Closes #1198 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a policy inheritance bug where a child policy that uses extends: but omits the unmanaged_files: block could inadvertently weaken the parent posture. The change introduces an explicit “unset / no opinion” state for unmanaged_files.action, ensuring inheritance preserves the parent’s effective action unless the child explicitly sets one.
Changes:
- Model
UnmanagedFilesPolicy.actionasstr | None(whereNonemeans “no opinion”) and addeffective_actionfor runtime behavior. - Update policy parsing and merging so omitted
unmanaged_filesis transparent during inheritance. - Expand unit test coverage for defaults, parsing, and multi-level inheritance; add a changelog entry.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/policy/test_schema.py | Updates default expectations for UnmanagedFilesPolicy (action is None, effective_action == "ignore"). |
| tests/unit/policy/test_parser.py | Adds coverage asserting omitted unmanaged_files yields action=None and preserves runtime default via effective_action. |
| tests/unit/policy/test_inheritance.py | Adds targeted regression tests for transparent inheritance when the child omits unmanaged_files. |
| tests/unit/policy/test_fixtures.py | Updates fixture default assertions to match new action=None model semantics. |
| src/apm_cli/policy/schema.py | Changes action default to None and introduces effective_action property. |
| src/apm_cli/policy/policy_checks.py | Uses effective_action when deciding whether the unmanaged-files check is disabled. |
| src/apm_cli/policy/parser.py | Parses omitted unmanaged_files.action as None rather than defaulting to "ignore". |
| src/apm_cli/policy/inheritance.py | Merges unmanaged_files.action with explicit handling for None (“no opinion”). |
| src/apm_cli/policy/discovery.py | Serializes/emptiness-checks using effective_action to keep behavior consistent when action is None. |
| CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the inheritance behavior correction. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Clean None-sentinel approach, frozen dataclass invariant sound; one raw .action call at policy_checks.py:755 left un-migrated -- safe today but a maintenance trap. |
| CLI Logging Expert | 0 | 1 | 1 | No logging regressions; one recommended: serialization conflates null/ignore; one nit: no verbose hint on silent parent-deny inheritance. |
| DevX UX Expert | 1 | 2 | 1 | CHANGELOG buries a de-facto breaking behavior change; serialization lies about user intent; no runtime migration signal on upgrade. |
| Supply Chain Security Expert | 0 | 1 | 1 | Governance downgrade fix is correct; explicit action: null silently bypasses schema enum validation and should be rejected or documented. |
| OSS Growth Hacker | 0 | 2 | 1 | Silent policy downgrade fix is a high-trust signal for enterprise/platform teams; CHANGELOG entry is technically correct but undersells the governance reliability story. |
| Doc Writer | 0 | 3 | 2 | Inheritance table and two schema references need updating; CHANGELOG entry uses jargon and omits the closed issue number. |
| Test Coverage Expert | 2 | 1 | 0 | Check 16 unit regression-trap missing (action=None path); no integration fixture for full parent-deny/child-omits pipeline; _policy_to_dict round-trip omits action=None case. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] (blocking-severity) Add unit regression trap:
test_pass_none_action_short_circuits_checkintest_policy_checks.py-- action=None must not trigger Check 16 scan -- Missing outcome on governed-by-policy + secure-by-default surface. Ifeffective_actionis refactored, Check 16 silently reverts. This is the highest-priority gap: the PR's core invariant has no CI guard. - [Test Coverage Expert] (blocking-severity) Add integration fixture test:
test_parent_deny_child_omits_unmanaged_check_firesintest_policy_discovery_e2e.py-- real YAML, real directory, rogue file, assert check fires -- Five-module governance chain with no end-to-end fixture. Cross-module governance flows require integration-with-fixtures tier; missing here is a regression-trap gap on the governed-by-policy principle. - [DevX UX Expert] (blocking-severity) Promote CHANGELOG entry to Breaking Changes section with migration callout and oss-growth-hacker reframe -- Users with child-omits + parent-deny configs will see silent audit failures after upgrade with no YAML changes. No migration note = trust-breaking surprise. Minimum: section rename + one-sentence migration line + "closes [bug] policy inheritance: child without unmanaged_files block silently downgrades parent's action: deny #1198".
- [CLI Logging Expert] Revert discovery.py:1082 serialization to raw
policy.unmanaged_files.action; keep_is_policy_emptychange -- Three-panelist convergence. Writingeffective_actionto disk misrepresents user intent inapm policy showand risks re-introducing explicitignoreinto cached policies that re-enter the inheritance chain, inverting the fix. - [Python Architect] Change
policy_checks.py:755frompolicy.action == "warn"topolicy.effective_action == "warn"-- Mixed.action/.effective_actionin a single function is a maintenance trap on a governed-by-policy surface. Change is one token; cost of deferral is a confusing anti-pattern.
Architecture
classDiagram
direction TB
class ApmPolicy {
<<ValueObject>>
+name str
+enforcement str
+unmanaged_files UnmanagedFilesPolicy
}
class UnmanagedFilesPolicy {
<<ValueObject>>
+action str | None
+directories tuple
+effective_action() str
}
class ManifestPolicy {
<<ValueObject>>
+scripts str
+required_fields tuple
}
class _merge_unmanaged_files {
<<PureFunction>>
+__call__(parent, child) UnmanagedFilesPolicy
}
class _escalate {
<<PureFunction>>
+__call__(levels, parent_val, child_val) str
}
class _build_policy {
<<IOBoundary>>
+__call__(data) ApmPolicy
}
class _check_unmanaged_files {
<<PureFunction>>
+__call__(project_root, lock, policy) CheckResult
}
ApmPolicy *-- UnmanagedFilesPolicy
ApmPolicy *-- ManifestPolicy
_merge_unmanaged_files ..> UnmanagedFilesPolicy : creates
_merge_unmanaged_files ..> _escalate : delegates to
_build_policy ..> UnmanagedFilesPolicy : instantiates
_check_unmanaged_files ..> UnmanagedFilesPolicy : reads
note for UnmanagedFilesPolicy "None sentinel = no-opinion; effective_action() resolves None->ignore at call sites"
note for _merge_unmanaged_files "Chain of Responsibility: child.action is None -> inherit parent; both set -> escalate"
class UnmanagedFilesPolicy:::touched
class _merge_unmanaged_files:::touched
class _build_policy:::touched
class _check_unmanaged_files:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["[I/O] load_policy(yaml_str)\nparser.py"] --> B["_build_policy(data)\nparser.py:210"]
B --> C["uf_data.get('action')\n-> None when absent\nparser.py:211"]
C --> D["UnmanagedFilesPolicy(action=None)\nschema.py:91"]
D --> E{"merge_policies?\ninheritance.py"}
E -- "child.action is None" --> F["merged_action = parent.action\ninheritance.py:198"]
E -- "parent.action is None" --> G["merged_action = child.action\ninheritance.py:200"]
E -- "both non-None" --> H["_escalate(...)\ninheritance.py:202"]
E -- "no merge" --> I["UnmanagedFilesPolicy as-is"]
F --> J["UnmanagedFilesPolicy(action=merged)\ninheritance.py:203"]
G --> J
H --> J
I --> K{"call site"}
J --> K
K -- "discovery.py:1079" --> L["[I/O] .effective_action\ndiscovery.py:1079"]
K -- "discovery.py:1114" --> M[".effective_action == 'ignore'\ndiscovery.py:1114"]
K -- "policy_checks.py:688" --> N[".effective_action == 'ignore'\npolicy_checks.py:688 UPDATED"]
K -- "policy_checks.py:755" --> O[".action == 'warn'\npolicy_checks.py:755 RAW not updated"]
N -- "True (incl. None->ignore)" --> P["CheckResult passed=True early exit"]
O -- "True" --> Q["CheckResult warn, passed=True"]
O -- "False" --> R["CheckResult deny, passed=False"]
style O fill:#ffe0e0,stroke:#c00
sequenceDiagram
participant User as apm audit
participant Parser as parser.py
participant Inherit as inheritance.py
participant Schema as UnmanagedFilesPolicy
participant Checks as policy_checks.py
User->>Parser: load_policy(child_yaml -- no unmanaged_files)
Parser->>Schema: UnmanagedFilesPolicy(action=None)
Note over Parser,Schema: Before: action defaulted to 'ignore'. After: action=None (no opinion)
User->>Inherit: merge_policies(parent_deny, child_none)
Inherit->>Inherit: child.action is None -> merged=parent.action ('deny')
Inherit->>Schema: UnmanagedFilesPolicy(action='deny')
Schema-->>Inherit: frozen instance
Inherit-->>User: ApmPolicy(unmanaged_files.action='deny')
User->>Checks: _check_unmanaged_files(root, lock, policy)
Checks->>Schema: policy.effective_action == 'ignore'? -> False
Checks->>Schema: policy.action == 'warn'? -> False
Checks-->>User: CheckResult(passed=False, deny enforced)
Note over Checks: Before fix: child action=None->effective 'ignore', early-exit fired, deny silently dropped
Recommendation
Hold for four targeted changes before merge: (1) unit regression-trap test for Check 16 with action=None, (2) integration fixture test for the full parent-deny/child-omits pipeline, (3) revert discovery.py:1082 serialization to raw action, (4) promote the CHANGELOG entry to Breaking Changes with a migration line and "closes #1198". All four are small in LOC -- the two tests are the main work -- and all four are required to make the governance promise machine-verifiable and the upgrade experience non-breaking for current users. Once those land, this PR is a clean ship and the "fails closed" narrative is credible.
Full per-persona findings
Python Architect
-
[recommended]
policy_checks.py:755still uses rawpolicy.action == 'warn'instead ofeffective_actionatsrc/apm_cli/policy/policy_checks.py:755
The PR migrated the== 'ignore'guard at line 688 toeffective_action, which handles None->ignore correctly and gates all downstream branches. Line 755 is only reachable when effective_action != 'ignore' (i.e., action is 'warn' or 'deny', never None), so the current logic is accidentally safe. But mixing.actionand.effective_actionin the same function is a maintenance trap: a future refactor of the early-exit gate, or a new sentinel value, could let None fall through to the 'warn' branch and silently produce a passing CheckResult instead of deny.
Suggested: Changeif policy.action == "warn":toif policy.effective_action == "warn":for consistency with line 688.
Proof (missing at unit):tests/unit/policy/test_inheritance.py-- proves: No test verifies the warn branch in _check_unmanaged_files executes via effective_action. [governed-by-policy] -
[nit]
effective_actioncould be@functools.cached_propertysince the dataclass is frozen atsrc/apm_cli/policy/schema.py:97
UnmanagedFilesPolicy isfrozen=True, so action is immutable. The property recomputes a trivial conditional on every call. A@cached_propertymakes the immutability contract explicit and aligns with the dataclass-as-value-object pattern already used across schema.py.
Suggested: Replace@propertywith@functools.cached_propertyand addfrom functools import cached_property.
CLI Logging Expert
-
[recommended] Serialization emits
effective_action('ignore') instead of raw action (null), erasing the 'never set' signal in cached policy files atsrc/apm_cli/policy/discovery.py:1082
discovery.py:1082 writeseffective_actioninto the YAML persisted to disk. A user inspecting the cached policy file will seeaction: ignoreeven though they never wrote that field. This violates the output UX contract that serialized files should faithfully reflect what the user configured. It also makes it impossible to distinguish an explicitignorefrom an absent block when reading the cache.
Suggested: Revert this hunk topolicy.unmanaged_files.action. The_is_policy_emptychange on line 1117 is correct and should stay. -
[nit] No
--verbosehint when inheritance silently promotes a parent deny/warn to the merged policy atsrc/apm_cli/policy/inheritance.py:197
Whenchild.action is Noneandparent.actionis 'warn' or 'deny', a user whose Check 16 unexpectedly fires has no verbose-mode breadcrumb to explain why.
Suggested: After resolvingmerged_action, emit a verbose log whenchild.action is None and parent.action not in (None, 'ignore').
DevX UX Expert
-
[blocking] Silent behavior change deserves a Breaking Changes entry + migration callout, not a one-liner in Fixed at
CHANGELOG.md:53
A user who hadchild omits unmanaged_files+parent: action: denywill, after upgrading, start seeing governance checks fail on runs where they previously passed -- with zero YAML changes on their part. That is a de-facto breaking change regardless of the bug label. The current one-liner under### Fixedwill be missed by users skimming the changelog.
Suggested: Move or annotate under### Breaking Changeswith: "If your child policy omitsunmanaged_filesand your parent setsaction: deny, enforcement now applies. Addunmanaged_files: {action: ignore}to your child policy to preserve prior behavior." -
[recommended] Serialization emits
action: ignorefor fields the user never set, misrepresenting intent inapm policy showoutput atsrc/apm_cli/policy/discovery.py:1082
discovery.py:1082 serializeseffective_actionwhich resolves None to 'ignore'. A user who never wroteunmanaged_files:will seeaction: ignorein output, implying they explicitly configured it.
Suggested: Serializepolicy.unmanaged_files.action(rawstr | None) and emit asnull/omit when None. -
[recommended] No runtime migration signal: users silently promoted to deny enforcement get no warning on first
apm auditafter upgrade atsrc/apm_cli/policy/policy_checks.py:691
When effective action shifts from ignore to deny due to this fix, the user sees governance failures with no contextual hint the root cause is a tool upgrade. A one-time[warn]at check time whenpolicy.unmanaged_files.action is Nonebuteffective_action != 'ignore'would satisfy the 'Failure mode is the product' north star. -
[nit]
effective_actionproperty docstring does not explain the inheritance context atsrc/apm_cli/policy/schema.py:86
Suggested: Expand to: "Resolved action for runtime enforcement. Returns the explicitly set action, or 'ignore' when no action was set (transparent in inheritance; parent value takes precedence during merge). Do not use for serialization -- serialize the rawactionfield instead."
Supply Chain Security Expert
-
[recommended] Explicit
action: nullin YAML silently bypasses schema enum validation and is treated as 'no opinion' atsrc/apm_cli/policy/parser.py
The validator guardif action is not None and action not in _VALID_UNMANAGED_ACTIONmeans a child policy containingaction: nullpasses validation without error. Not exploitable for governance downgrade (None inherits parent deny), but it is an undocumented input pathway that bypasses the explicit enum check.
Suggested: Invalidate_policy, change to key-presence check:if 'action' in uf and uf['action'] not in _VALID_UNMANAGED_ACTION-- this rejects explicitnullwith a clear schema error. -
[nit]
_merge_unmanaged_filesresult action can be None after merge if both parent and child have action=None; worth a comment atsrc/apm_cli/policy/inheritance.py
A fully-None merged action is semantically 'no governance applied'; worth a comment noting that None-merged action implies no enterprise policy was set and runtime defaults to 'ignore' viaeffective_action.
OSS Growth Hacker
-
[recommended] CHANGELOG entry undersells the governance trust signal for enterprise adopters at
CHANGELOG.md
The fix prevents a silent deny->ignore downgrade -- the class of bug that causes security teams to reject tools. Framing as 'APM now fails closed on policy inheritance' converts a bug fix into a trust-building moment.
Suggested: "Policy inheritance now fails closed: a child policy omittingunmanaged_files:inherits the parent org'saction: denyrather than silently downgrading it to ignore. Multi-team layered policies (org -> repo) now behave as authored. (fix: policy inheritance preserves parent unmanaged_files when child omits block (closes #1198) #1253, closes [bug] policy inheritance: child without unmanaged_files block silently downgrades parent's action: deny #1198)" -
[recommended] This fix is release-note and community-callout worthy -- enterprise multi-team policy layering is a differentiator
APM's org->repo->child policy inheritance model directly answers the Why do we need a GitHub token? #1 ask from platform/ops teams: 'can I enforce org-wide rules that repo teams cannot accidentally override?' This bug undermined that guarantee.
Suggested: Ship a short release callout: "0.12.5 hardens policy governance -- org-level deny policies now survive child inheritance even when child omits the block." -
[nit] Missing 'closes [bug] policy inheritance: child without unmanaged_files block silently downgrades parent's action: deny #1198' in CHANGELOG entry at
CHANGELOG.md
The issue number ([bug] policy inheritance: child without unmanaged_files block silently downgrades parent's action: deny #1198) is referenced in the PR title but absent from the CHANGELOG entry.
Doc Writer
-
[recommended] Inheritance merge table does not document the 'absent block = no opinion' semantic at
docs/src/content/docs/enterprise/policy-reference.md:423
The merge table showsEscalates: ignore < warn < denybut says nothing about None/absent. A child-policy author who omits the block has no documentation telling them their parent'saction: denyis preserved.
Suggested: Add note: "An absentunmanaged_filesblock is treated as no-opinion: the parent setting is preserved without escalation." -
[recommended] CHANGELOG entry uses 'transparent' without defining it; typical users will not know the policy inheritance model at
CHANGELOG.md:53
'Transparent' is a systems-programming term. A policy author will not understand what 'absent blocks are treated as transparent' means.
Suggested: Replace "absent blocks are now treated as transparent ("no opinion")" with "omitting the block now preserves the parent setting unchanged (previously it silently reset toignore)". -
[nit] CHANGELOG entry references PR fix: policy inheritance preserves parent unmanaged_files when child omits block (closes #1198) #1253 but not the closed issue [bug] policy inheritance: child without unmanaged_files block silently downgrades parent's action: deny #1198 at
CHANGELOG.md:53
Suggested: Append(fixes #1198)or change to(#1198, #1253). -
[recommended]
packages/apm-guidegovernance skill showsaction: ignoreas explicit default, masking the new 'omit = no opinion' behavior atpackages/apm-guide/.apm/skills/apm-usage/governance.md:56
A child-policy author copying the canonical policy template will inadvertently pinaction: ignoreand override a parent'sdeny.
Suggested:# action: ignore # ignore | warn | deny -- omit to inherit parent setting -
[nit] policy-reference.md schema block shows
action: ignoreas default; comment should note omit-to-inherit for child policies atdocs/src/content/docs/enterprise/policy-reference.md:55
Suggested:# ignore | warn | deny; omit in child policies to inherit parent setting
Test Coverage Expert
-
[blocking] No test exercises Check 16 (
_check_unmanaged_files) when action=None -- the exact regression path this PR fixes attests/unit/policy/test_policy_checks.py
PR changes policy_checks.py Check 16 frompolicy.action == 'ignore'topolicy.effective_action == 'ignore'. Grepped test_policy_checks.py for 'effective_action' and 'action=None' -- zero hits. All existing Check 16 tests use explicit action values. Ifeffective_actionis removed or its default changed, Check 16 silently reverts.
Proof (missing at unit):tests/unit/policy/test_policy_checks.py::test_pass_none_action_short_circuits_check-- proves: When child policy omits unmanaged_files (action=None), Check 16 does not scan or fail -- the governance enforcement change does not silently revert on a future refactor [governed-by-policy, secure-by-default] -
[blocking] No integration-with-fixtures test runs the full parent-deny/child-omits pipeline through run_policy_checks with real YAML files at
tests/integration/test_policy_discovery_e2e.py
The fix touches 5 modules in a chain. Unit tests cover each in isolation. Grepped tests/integration/test_policy_discovery_e2e.py for 'effective_action', 'action=None', '_check_unmanaged': zero hits. Per the tier-floor matrix, a cross-module governance-enforcement flow requires integration-with-fixtures tier.
Proof (missing at integration-with-fixtures):tests/integration/test_policy_discovery_e2e.py::test_parent_deny_child_omits_unmanaged_check_fires-- proves: When org policy says deny on unmanaged files and repo policy omits the block, the policy check pipeline correctly fires on a rogue file -- governance promise enforced end-to-end [governed-by-policy, secure-by-default] -
[recommended]
_policy_to_dict/_serialize_policyround-trip does not include unmanaged_files with action=None attests/unit/policy/test_cache_merged_effective.py
discovery.py serialization now useseffective_action. No round-trip test setsaction=Noneand verifies the deserialized policy still hasaction=None(not 'ignore'). If_policy_to_dictwere changed to write the effective value, a cached policy could re-enter the inheritance chain with an explicit 'ignore' that overrides parent deny.
Proof (missing at unit):tests/unit/policy/test_cache_merged_effective.py::test_none_unmanaged_action_round_trip-- proves: A policy with action=None survives cache serialization with action still None -- not promoted to 'ignore' which would prevent parent deny from propagating through cache [governed-by-policy]
Auth Expert -- inactive
No auth files touched; PR changes policy inheritance schema and merge logic only.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1253 · ● 2.8M · ◷
- Serialize raw action (not effective_action) in _policy_to_dict - Use effective_action in _check_unmanaged_files warn branch - Add regression-trap tests for effective_action in policy checks - Clarify CHANGELOG entry for #1253 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #1198
Problem
When a child policy declares
extends: orgbut omitsunmanaged_files:, the merged policy silently downgrades the parent'saction: denyto the model default (ignore). The child's omission is not treated as "no opinion" but as an explicitignore.Approach
Distinguish "field not set" from "field explicitly set to ignore" in
UnmanagedFilesPolicy. When the child has no opinion, the merge returns the parent's value unchanged.Draft PR -- implementation in progress.