fix(policy): inherit parent unmanaged_files when child omits block

[abhinavgautam01]

Fixes #1198

Description

Child policies that use extends: but leave out unmanaged_files: were parsed as if they had the schema defaults (action / directories), so merge treated them as an explicit ignore posture and org unmanaged_files.action: deny no longer applied in repo-scoped apm audit --policy.
UnmanagedFilesPolicy now uses None for “no opinion” (omitted or empty unmanaged_files), the YAML parser only sets fields when keys are present, and _merge_unmanaged_files returns the parent unchanged when the child is fully transparent—matching how allow-lists use None during inheritance. Downstream checks and diagnostics treat None like permissive/default where appropriate. Regression test covers the org + child repo override scenario from the issue.

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)
  uv run pytest tests/unit/policy/ tests/integration/test_policy_discovery_e2e.py (533 passed, 15 skipped).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

[abhinavgautam01]

ping @danielmeppiel

[github-actions]

APM Review Panel: ship_with_followups

Fixes silent org-policy inheritance breakage and malformed-YAML validation for unmanaged_files; both halves matter to enterprise admins enforcing multi-team policy hierarchies. (#1248)

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

This PR closes two related bugs in the unmanaged_files policy path: a hard validation failure when the field is set to a non-mapping YAML value, and a silent inheritance bypass where a child policy omitting the block was treated as an explicit empty override rather than a transparent pass-through. The second fix is the higher-value change -- it directly restores the 'governed by policy' contract for every enterprise team running an org-level unmanaged_files deny rule. All active panelists found the core logic correct and the None/None sentinel idiom appropriate; there are no blocking findings.

Panel signals converge on two actionable gaps before or shortly after merge: (1) the CHANGELOG entry documents only the validation rejection and must be extended to surface the inheritance fix -- devx-ux, doc-writer, and oss-growth-hacker all call this out independently, making it the strongest cross-persona signal in the review; (2) the error message for the malformed-YAML case gives no recovery hint, which devx-ux and cli-logging-expert both flag as a recommended fix. The test-coverage-expert identifies a missing unit test for the partial-None merge branch (child sets directories but defers action to parent), which is the only regression-trap gap on a governed-by-policy surface and ranks above the opinion-only nits.

The supply-chain finding on child-clears-parent-deny is pre-existing behavior not introduced by this PR; it is a real governance gap worth a follow-up issue but is not a reason to hold this merge. The python-architect's effective_directories consistency recommendation and the doc drift on policy-schema.md are both clean follow-ups. No panelist dissented on ship-readiness.

Dissent. cli-logging-expert classified the error-message gap as a nit; devx-ux classified the same finding as recommended. I side with devx-ux: the error message is the only user-facing feedback path for a type mismatch and 'failure mode is the product' applies here.

Aligned with: Governed by Policy -- org-level unmanaged_files rules now correctly propagate to child policies that omit the block, restoring the inheritance contract the principle depends on. Secure by Default -- validation now rejects malformed unmanaged_files values instead of silently no-oping, closing a footgun that could mask misconfigured deny policies; pre-existing child-clears-parent-deny gap remains and needs a follow-up issue. Pragmatic as npm -- error message for the YAML type mismatch should include a recovery example (got X, expected Y plus corrective snippet), which is the npm/cargo ergonomic baseline.

Growth signal. Both fixes directly serve the enterprise-admin persona that APM's 'governed by policy' pillar targets. The CHANGELOG should surface both halves to ensure affected teams find the inheritance fix in release notes. A short FAQ entry -- 'Why is my child policy not inheriting unmanaged_files?' -- would convert confused enterprise users searching that exact phrase into trust, and is low-cost to add post-merge.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	None/None sentinel is idiomatic and merge logic is correct; one recommended consistency gap on effective_directories, one nit on coerce helper placement.
CLI Logging Expert	0	0	1	Error message is consistent with existing parser conventions; or () guards preserve correct zero-count reporting; no output regressions found.
DevX UX Expert	0	2	1	Error message lacks recovery hint; CHANGELOG omits the transparency fix that is the core user story; both are recommended fixes before ship.
Supply Chain Security Expert	0	1	1	Transparency fix correctly closes silent org-deny downgrade (#1198); one recommended finding on child-clears-parent-deny surface.
OSS Growth Hacker	0	1	1	Footgun fix strengthens the 'governed by policy' proposition; CHANGELOG entry covers the validation half but misses the transparency/inheritance fix that matters most to enterprise admins.
Doc Writer	0	2	1	CHANGELOG omits the inheritance fix; policy-schema.md documents directories default as [] but the PR changes not-expressed to None, creating doc drift on tri-state semantics.
Test Coverage Expert	0	1	0	Core behaviour changes (mapping-rejection, transparency, None defaults) are well-defended; one partial-None combination in _merge_unmanaged_files lacks an explicit assertion.

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

Top 5 follow-ups

1. [DevX UX Expert] Extend CHANGELOG entry to document the child-policy inheritance fix, not just the validation rejection. -- Three panelists (devx-ux, doc-writer, oss-growth-hacker) independently flagged this gap. Enterprise admins affected by the silent inheritance breakage will search release notes for this fix and currently will not find it. Highest cross-persona signal in the review.
2. [Test Coverage Expert] Add test_child_directories_only_inherits_parent_action covering the partial-None merge branch (child.directories set, child.action=None, parent.action non-None). -- Missing test on a governed-by-policy surface: a future refactor of _coerce_unmanaged_action_for_escalate could silently drop the parent action with no assertion to catch it.
3. [DevX UX Expert] Add recovery hint to the 'unmanaged_files must be a YAML mapping' error message (got: type + value; expected block syntax example). -- User has no self-correction path from the current message. devx-ux and cli-logging-expert both flag this; 'failure mode is the product' applies for a YAML type error that a new adopter will hit.
4. [Doc Writer] Update policy-schema.md to document the tri-state semantics for unmanaged_files.directories (omitted=transparent, []=all dirs, [...]=scoped). -- The docs currently say default is [] but the PR changes not-expressed to None. Silent doc drift on a field enterprise admins configure explicitly.
5. [Supply Chain Security Expert] Open a follow-up issue on child-policy clearing org-level deny list with deny: [] -- add a governance guard or document as explicit design choice in enterprise/security.md. -- Pre-existing behavior not introduced by this PR, but the passing test proves the gap is real: a repo-level policy can silently widen the org-enforced deny surface with no audit event. Needs a decision, not a hold.

Architecture

classDiagram
    direction LR
    class ApmPolicy {
        <<DataClass>>
        +unmanaged_files UnmanagedFilesPolicy
    }
    class UnmanagedFilesPolicy {
        <<DataClass>>
        +action str | None
        +directories tuple | None
        +effective_action() str
    }
    class DependencyPolicy {
        <<DataClass>>
        +deny tuple | None
        +require tuple | None
        +effective_deny() tuple
        +effective_require() tuple
    }
    class _merge_unmanaged_files {
        <<Pure>>
    }
    class _coerce_unmanaged_action_for_escalate {
        <<Pure>>
    }
    class _coerce_unmanaged_directories_for_union {
        <<Pure>>
    }
    class merge_policies {
        <<Facade>>
    }
    ApmPolicy *-- UnmanagedFilesPolicy : contains
    merge_policies ..> _merge_unmanaged_files : delegates
    _merge_unmanaged_files ..> _coerce_unmanaged_action_for_escalate : uses
    _merge_unmanaged_files ..> _coerce_unmanaged_directories_for_union : uses
    _merge_unmanaged_files ..> UnmanagedFilesPolicy : returns
    note for UnmanagedFilesPolicy "Null Object: (None, None) = no opinion, transparent in merge chain"
    note for DependencyPolicy "Prior art: same None-transparent pattern, effective_deny / effective_require already encapsulated"
    class UnmanagedFilesPolicy:::touched
    class _merge_unmanaged_files:::touched
    class _coerce_unmanaged_action_for_escalate:::touched
    class _coerce_unmanaged_directories_for_union:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["YAML input: apm-policy.yml"]) --> B["validate_policy()"]
    B --> C{"uf is not None and not dict?"}
    C -- yes --> ERR["errors: unmanaged_files must be a YAML mapping"]
    C -- no --> D["_build_policy()"]
    D --> E{"data.get('unmanaged_files') is None?"}
    E -- yes --> F["UnmanagedFilesPolicy(action=None, directories=None) -- no-opinion sentinel"]
    E -- no --> G{"'directories' in uf_data?"}
    G -- yes --> H["directories = _parse_tuple(uf_data['directories'])"]
    G -- no --> I["directories = None -- key absent, transparent"]
    H --> J["UnmanagedFilesPolicy(action, directories)"]
    I --> J
    F --> K(["ApmPolicy.unmanaged_files"])
    J --> K
    K --> L["merge_policies()"]
    L --> M["_merge_unmanaged_files(parent, child)"]
    M --> N{"child.action is None AND child.directories is None?"}
    N -- yes --> O["return parent unchanged -- transparent child"]
    N -- no --> P{"child.action is None?"}
    P -- yes --> Q["eff_action_raw = parent.action"]
    P -- no --> R["_escalate(LEVELS, coerce(parent.action), child.action)"]
    Q --> S{"child.directories is None?"}
    R --> S
    S -- yes --> T["eff_dirs = parent.directories"]
    S -- no --> U["_union(coerce(parent.directories), child.directories)"]
    T --> V["Materialise: None->ignore / None->()"]
    U --> V
    V --> W["UnmanagedFilesPolicy(action=eff_action, directories=eff_dirs_out)"]
    O --> X(["Merged ApmPolicy"])
    W --> X

Loading

Recommendation

Merge is clear -- no blocking findings, core logic is correct, and both bugs are well-defended by existing tests. Before merging, extend the CHANGELOG to surface the inheritance fix (highest cross-persona signal) and add the recovery hint to the error message (two panelists, user-facing regression). The partial-None test case and doc drift on policy-schema.md are the priority post-merge items. Track the pre-existing child-clears-deny governance gap in a dedicated issue.

---

Full per-persona findings

Python Architect

• [recommended] No effective_directories property mirrors effective_action; callers use inline or () instead at src/apm_cli/policy/schema.py
schema.py adds UnmanagedFilesPolicy.effective_action (None -> 'ignore') but does not add a symmetric effective_directories property (None -> ()). Two call sites in commands/policy.py:97 and discovery.py:1083 use raw inline directories or () guard. This breaks the established pattern in DependencyPolicy where None-coercion is encapsulated on the dataclass. Adding effective_directories closes the surface and matches DependencyPolicy's contract.
  Suggested: @property / def effective_directories(self) -> tuple[str, ...]: / return self.directories if self.directories is not None else ()

• [nit] _coerce_unmanaged_action_for_escalate and _coerce_unmanaged_directories_for_union are each called exactly once and could be inline at src/apm_cli/policy/inheritance.py
  Both helpers are private, single-expression, called from one location. Named helpers add doc value but the same clarity could be achieved with a brief inline comment.

• [nit] Design pattern annotation: None/None sentinel and partial-None merge are well-applied Null Object + transparent-merge patterns consistent with the codebase.
  Consistent with DependencyPolicy.deny=None semantics. No action required.

CLI Logging Expert

• [nit] Error message lacks a fix hint -- user cannot self-correct without reading docs at src/apm_cli/policy/parser.py:146
  APM message rule: 'Include the fix'. 'unmanaged_files must be a YAML mapping' tells the user what is wrong but not what to do. This is a novel YAML type error and users hitting it are likely confused about YAML syntax.
  Suggested: "unmanaged_files must be a YAML mapping (got list or scalar); use 'unmanaged_files:\n action: ...' block syntax"

DevX UX Expert

• [recommended] Error message 'unmanaged_files must be a YAML mapping' gives no recovery path at src/apm_cli/policy/parser.py:146
  A user who writes unmanaged_files: deny receives the error but has no idea (a) what value was rejected or (b) what the correct YAML shape looks like. npm, cargo, and pip all print 'got X, expected Y' plus a corrective example.
  Suggested: errors.append(f"unmanaged_files must be a YAML mapping (got: {type(uf).__name__!r} {uf!r}); expected e.g.:\n unmanaged_files:\n action: deny\n directories:\n - src/")

• [recommended] CHANGELOG entry documents only the validation rejection; the inheritance transparency fix (the core user story) is absent at CHANGELOG.md
  The PR fixes two bugs: (1) validation rejection and (2) child-policy transparency. The CHANGELOG only mentions (1). A user who was silently affected by broken inheritance will not find this fix in the release notes.
  Suggested: Add: '- A project-level policy that omits unmanaged_files: now correctly inherits the org policy block; previously the silent default directories=() caused the child to override the parent, discarding the org's action: deny. (fix(policy): inherit parent unmanaged_files when child omits block #1248)'

• [nit] Scalar directories: /tmp silently coerces to empty tuple instead of raising a validation error at src/apm_cli/policy/parser.py:148
_parse_tuple returns () for any non-list value. A user writing directories: src/ gets an empty deny-list with no feedback. Now that the PR adds type-checking for the top-level key, applying the same check to directories would be consistent.
  Suggested: Inside the isinstance(uf, dict) branch add: dirs = uf.get('directories'); if dirs is not None and not isinstance(dirs, list): errors.append(f"unmanaged_files.directories must be a YAML list, got {type(dirs).__name__!r}")

Supply Chain Security Expert

• [recommended] Child policy can clear org-level deny list with deny: [] -- no governance guard or audit event at src/apm_cli/policy/inheritance.py:259
_merge_list_field semantics intentionally allow child=() (YAML deny: []) to fully override and clear a parent deny list. A repo-level policy can silently remove org-enforced deny rules without any escalation check, audit log, or policy-governance gate. The allow field uses _intersect_allow which enforces least-privilege; deny lacks an equivalent anti-widening guard.
  Suggested: Add a governance check: if parent.deny is non-empty and child.deny == (), emit a PolicyValidationError or warning. Alternatively document this in enterprise/security.md as an explicit design choice.
  Proof (passed): tests/unit/policy/test_inheritance.py::TestDependencyTransparency::test_parent_deny_child_explicit_empty_deny -- proves: a child policy with deny=[] silently clears the parent org-level deny list with no error or warning [secure-by-default, governed-by-policy]

• [nit] _merge_unmanaged_files returns the parent object directly (aliasing) rather than a copy at src/apm_cli/policy/inheritance.py:210
  When child is fully transparent, the parent UnmanagedFilesPolicy instance is returned directly. Frozen dataclasses make this safe today but relaxing the frozen constraint would make this a latent bug.

OSS Growth Hacker

• [recommended] CHANGELOG entry omits the child-policy transparency fix -- the higher-value story for enterprise adopters at CHANGELOG.md
  The entry only mentions the validation rejection. The second fix -- child policies that omit unmanaged_files now correctly inherit -- is the change enterprise admins running multi-team policy hierarchies care about most.
  Suggested: Add: '- Child policies that omit unmanaged_files now correctly inherit from the parent instead of being treated as an explicit empty override, fixing silent policy transparency breakage in multi-team org configurations. (fix(policy): inherit parent unmanaged_files when child omits block #1248)'

• [nit] CHANGELOG entry uses passive 'rejects ... instead of treating' -- a concrete before/after phrasing lands harder at CHANGELOG.md
  High-signal CHANGELOG entries tell a user what was broken and what changed.
  Suggested: 'Policy validation now raises a clear error when unmanaged_files is set to a list or scalar instead of a YAML mapping, replacing silent no-op behavior that masked misconfigured policies. (fix(policy): inherit parent unmanaged_files when child omits block #1248)'

Doc Writer

• [recommended] CHANGELOG entry omits the inheritance fix ([bug] policy inheritance: child without unmanaged_files block silently downgrades parent's action: deny #1198 transparency bug) at CHANGELOG.md
  The PR makes two distinct user-visible changes: (1) validation rejects non-mapping values, and (2) a child policy omitting unmanaged_files now correctly inherits the org-level setting. The CHANGELOG entry covers only (1).
  Suggested: Extend: '- Policy inheritance: a child policy omitting unmanaged_files now correctly inherits the org-level setting instead of silently dropping it. Fixes [bug] policy inheritance: child without unmanaged_files block silently downgrades parent's action: deny #1198.'

• [recommended] policy-schema.md documents unmanaged_files.directories default as [] but the PR changes not-expressed to None (transparent) at docs/src/content/docs/reference/policy-schema.md:114
  After this PR, the internal representation distinguishes None (not expressed, transparent during merge) from () / [] (explicitly empty). The tri-state semantics are documented for *.allow lists in the same file but are absent for unmanaged_files.directories. Code semantics changed; docs were not updated.
  Suggested: Add tri-state table: omitted (transparent, parent passes through) / [] (all dirs) / [...] (only these dirs). Update Default column from [] to omitted.

• [nit] enterprise/policy-reference.md unmanaged_files section does not document the omit-vs-empty distinction at docs/src/content/docs/enterprise/policy-reference.md:292
  Lines 292-315 describe unmanaged_files.directories without mentioning that omitting the block is transparent to inheritance.
  Suggested: Add: 'Omitting unmanaged_files entirely is transparent during inheritance; a child policy that does not declare this block inherits the org-level setting unchanged. See Policy Schema: unmanaged_files for the full tri-state table.'

Test Coverage Expert

• [recommended] Partial-None merge path (child.directories set, child.action=None, parent.action non-None) has no dedicated test at tests/unit/policy/test_inheritance.py
_merge_unmanaged_files has four logical branches. The case 'child adds a directory but defers action decision to parent' when parent.action is non-None has no test. A future refactor of _coerce_unmanaged_action_for_escalate could silently drop the parent action with no assertion to catch it.
  Suggested: Add test_child_directories_only_inherits_parent_action: parent=(action='deny', dirs=('.github',)), child=(action=None, dirs=('.rules',)) -> assert result.action=='deny' and both dirs present.
  Proof (missing): tests/unit/policy/test_inheritance.py::test_child_directories_only_inherits_parent_action -- proves: when child specifies only directories (action=None) the parent non-None action is preserved, not silently dropped [governed-by-policy]

Auth Expert -- inactive

No auth-relevant files touched: src/apm_cli/core/auth.py, token_manager.py, azure_cli.py, github_downloader.py, and related credential/host files are all unchanged. PR is scoped to policy validation and inheritance logic only.

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

Note

🔒 Integrity filter blocked 2 items

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

• #1248 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• fix(policy): inherit parent unmanaged_files when child omits block #1248 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

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

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

Generated by PR Review Panel for issue #1248 · ● 3.4M · ◷