fix(policy): layer dependencies.require through extends: org (closes #1201)#1290
fix(policy): layer dependencies.require through extends: org (closes #1201)#1290sergio-sisternes-epam wants to merge 5 commits into
Conversation
…oses #1201) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
Fixes policy inheritance so dependencies.require / dependencies.deny correctly flow from parent policies when a child policy uses extends: org and omits the dependencies: block, by introducing tri-state list semantics (None = transparent, ()/[] = explicit override, non-empty = union).
Changes:
- Update
DependencyPolicyto allowdeny/requireto beNoneand addeffective_*accessors for runtime checks. - Implement
None-transparent merge behavior fordeny/requireduring inheritance resolution and adjust parsing to distinguish absent keys. - Update policy enforcement code paths and unit tests, and add a changelog entry.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/policy/schema.py |
Changes deny/require defaults to None and adds effective_deny / effective_require. |
src/apm_cli/policy/parser.py |
Parses dependencies.deny / dependencies.require as None when absent to enable transparency. |
src/apm_cli/policy/inheritance.py |
Adds _merge_list_field() and uses it to merge deny/require with None transparency. |
src/apm_cli/policy/policy_checks.py |
Switches deny/require checks to use effective_* lists. |
src/apm_cli/policy/matcher.py |
Uses effective_deny for dependency deny evaluation. |
src/apm_cli/policy/discovery.py |
Serializes dependencies using effective_* (but see review comment re: cache semantics). |
src/apm_cli/commands/policy.py |
Counts dependencies using effective_* lists. |
tests/unit/policy/test_schema.py |
Updates default assertions for new tri-state semantics. |
tests/unit/policy/test_parser.py |
Updates parsing expectations for missing deny. |
tests/unit/policy/test_inheritance.py |
Adds tests covering transparency/override for deny/require. |
tests/unit/policy/test_chain_discovery_shared.py |
Updates helper default deny value to None. |
CHANGELOG.md |
Adds an Unreleased fix entry for extends: org layering behavior. |
Fix 1 (cache round-trip): _policy_to_dict in discovery.py was using effective_deny/effective_require which collapses None→(), meaning a policy cached without a dependencies: block would reload as () and stop inheriting from its parent. Now uses the raw .deny/.require fields and _opt_list(), preserving None through the cache round-trip. Fix 2 (YAML null): 'deny:' / 'require:' with a bare or explicit null value were reaching _parse_tuple(None) which returned (). Added an explicit 'or deps_data[field] is None' guard so null is treated the same as key absent (→ None, transparent). Fix 3 (docs): Updated policy-schema.md table (deny/require now show null = no opinion, [] = explicit empty), the merge-rule row, and the deny/require semantics section to match the tri-state model. Updated governance.md used by agents. Tests added: - test_parser.py: 3 new cases for absent block, YAML null, explicit [] - test_discovery.py: 2 new cache round-trip cases (None preserved, explicit-empty () preserved)
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Clean None-transparency extension; follows established unmanaged_files precedent exactly; no architectural faults found. |
| CLI Logging Expert | 0 | 0 | 0 | CLI output surface is minimal and correct; effective_deny/effective_require fix prevents zero-count false negatives in policy summaries. No output regressions. |
| DevX UX Expert | 0 | 0 | 0 | No CLI surface touched; policy inheritance fix is internal-only. Ship. |
| Supply Chain Security Expert | 0 | 1 | 1 | Core fix is a security improvement; explicit-empty deny override contradicts tighten-only invariant and warrants governance clarification. |
| OSS Growth Hacker | 0 | 1 | 0 | Silent governance regression fixed; ship with a changelog story targeting org admins. |
| Doc Writer | 0 | 1 | 1 | One doc drift found: policy-schema.md line 160 asserts omitted deny/require = empty tuple, which contradicts the new None=transparent semantic introduced by this PR. |
| Test Coverage Expert | 0 | 2 | 0 | Unit tests for the core fix are thorough; two integration-tier gaps remain: test_policy_merge_with_repo_override does not assert require flows through, and no parser test for deny: [] producing explicit empty (). |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc Writer] Update policy-schema.md line 160 to reflect the three-state model (omitted = transparent, [] = explicit override, [...] = union-merged with parent). -- The current docs actively contradict the new behavior; org admins reading the reference will misunderstand the semantics of omitting the dependencies block. This is the highest-priority post-merge task.
- [Supply Chain Security Expert] Clarify the "tighten but never relax" invariant in inheritance.py to document that explicit deny: [] is a valid repo-level override, and add a governance warning in the schema reference. -- The passing test proves deny: [] clears the org deny list. The docstring says this should never happen. The contradiction is a trap for future maintainers and org admins.
- [Test Coverage Expert] Add assertion to test_policy_merge_with_repo_override: assertIn("DevExpGbb/required-standards", merged.dependencies.require). -- The integration test loads a fixture with require entries but never asserts they flow through. A regression in the transparency logic would pass this test silently -- exactly the scenario this PR fixes.
- [Test Coverage Expert] Add parser unit test: deny: [] in YAML produces () (empty tuple) and not None. -- The three-state distinction (absent vs. explicit empty) is the semantic load-bearing change in this PR. Without a parser test for case (c), the distinction can silently regress.
- [OSS Growth Hacker] Reframe the CHANGELOG entry to lead with user impact: "org-level dependencies.require and deny rules are no longer silently dropped for repos that use extends: org without an explicit dependencies block (closes extends: org also fails to layer dependencies.require from parent (same root as #1198) #1201)." -- The current entry leads with the internal mechanism (None vs. ()). Org admins scanning the changelog will not recognize this as the fix they were waiting for.
Architecture
classDiagram
direction LR
class DependencyPolicy {
<<ValueObject>>
+allow tuple~str~ | None
+deny tuple~str~ | None
+require tuple~str~ | None
+require_resolution str
+max_depth int
+effective_deny() tuple~str~
+effective_require() tuple~str~
}
class ApmPolicy {
<<ValueObject>>
+dependencies DependencyPolicy
+extends str | None
+enforcement str
}
class _build_policy {
<<IOBoundary>>
+__call__(data dict) ApmPolicy
}
class _merge_dependencies {
<<Pure>>
+__call__(parent, child) DependencyPolicy
}
class _merge_list_field {
<<Pure>>
+__call__(parent, child) tuple | None
}
class policy_checks {
<<Consumer>>
+_check_dependency_denylist()
+_check_required_packages()
}
class matcher {
<<Consumer>>
+check_dependency_allowed()
}
class discovery {
<<Consumer>>
+_opt_list()
+_is_policy_empty()
}
ApmPolicy *-- DependencyPolicy : contains
_build_policy ..> DependencyPolicy : constructs
_merge_dependencies ..> DependencyPolicy : returns
_merge_dependencies *-- _merge_list_field : delegates deny/require
policy_checks ..> DependencyPolicy : reads effective_deny/require
matcher ..> DependencyPolicy : reads effective_deny
discovery ..> DependencyPolicy : reads effective_deny/require
note for _merge_list_field "None-transparency pattern:
None=no opinion, ()=explicit override,
truthy=union (same as unmanaged_files fix #1198)"
note for DependencyPolicy "frozen=True; effective_deny/require
are runtime views; None stored internally"
class DependencyPolicy:::touched
class _merge_list_field:::touched
class _merge_dependencies:::touched
class _build_policy:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["YAML policy file"]) -->|"data.get('dependencies')"| B["[I/O] parser._build_policy"]
B --> C{"_raw_deps is None?"}
C -->|"yes: block absent"| D["deny=None, require=None"]
C -->|"no: block present"| E{"'deny' in deps_data?"}
E -->|"no"| F["deny=None"]
E -->|"yes"| G["deny=_parse_tuple(deps_data['deny'])"]
F --> H["DependencyPolicy(deny=None|tuple, require=None|tuple)"]
D --> H
G --> H
H --> I["inheritance._merge_dependencies(parent, child)"]
I --> J["_merge_list_field(parent.deny, child.deny)"]
J --> K{"child is None?"}
K -->|"yes: transparent"| L["return parent (or None if parent also None)"]
K -->|"no, child empty ()"| M["return () -- explicit override"]
K -->|"no, child truthy"| N["return union(parent, child)"]
L --> O["merged DependencyPolicy"]
M --> O
N --> O
O --> P["effective_deny property: None -> ()"]
O --> Q["effective_require property: None -> ()"]
P --> R["policy_checks._check_dependency_denylist"]
Q --> S["policy_checks._check_required_packages"]
P --> T["matcher.check_dependency_allowed"]
P --> U["discovery._is_policy_empty"]
Q --> U
Recommendation
Merge this PR. The core fix is correct, well-tested at the unit level, and closes a silent governance regression that directly affects org admins. The five follow-ups above are post-merge tasks, not blockers: the doc drift in policy-schema.md is the most urgent (patch it in the same release), the two missing test assertions are one-line additions (bundle into a fast follow-up commit or add before merge if the PR author is available), and the CHANGELOG reframe should land before the release note goes out. The deny: [] governance question is a design clarification, not a defect in this PR.
Full per-persona findings
Python Architect
-
[nit] _merge_list_field defined after its call site in _merge_dependencies at
src/apm_cli/policy/inheritance.py:214
Python resolves names at call time so this is correct, but the module convention (helpers before callers) is inverted here. Readers scanning top-to-bottom see the call at line ~140 and must scroll to line 214 to find the definition.
Suggested: Move the _merge_list_field block (lines 214-240) to just before _merge_dependencies (line ~137). -
[nit] _union((), parent) in None-transparent branch adds an unnecessary dedup pass at
src/apm_cli/policy/inheritance.py:226
When child is None and parent is not None, the code returns _union((), parent). DependencyPolicy is frozen=True and its field type is tuple[str, ...] | None, so parent is always a tuple in production. The guard conflates a test-quality concern with production logic.
Suggested: Either enforce tuple type at DependencyPolicy construction (parser already does this) so the normalization is unnecessary, or add an assertion and return parent directly.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
-
[recommended] deny: [] in a child policy silently clears the parent deny list, violating the tighten-but-never-relax invariant at
src/apm_cli/policy/inheritance.py:225
inheritance.py states "Each level can tighten but never relax the parent." However, _merge_list_field returns () when child=(), unconditionally overriding a non-empty parent deny list. A repo author who writes "deny: []" in their apm-policy.yml will silently bypass all org-level blocked packages.
Suggested: For deny lists, treat explicit empty as "no opinion" (same as None) rather than "clear parent". If intentional, remove "tighten but never relax" from the module docstring and add a governance warning that repo policies can clear org deny lists with "deny: []".
Proof (passed):tests/unit/policy/test_inheritance.py::test_parent_deny_child_explicit_empty_deny-- proves: A repo policy with deny:[] clears the org deny list in the merged result. [governed-by-policy,secure-by-default] -
[nit] No integration-with-fixtures test covers the full parse -> merge -> check_dependency_allowed pipeline for the extends:org None-transparency path
Unit tests cover _merge_list_field and parser separately. No test loads a real org YAML with deny entries, merges a repo YAML that omits dependencies:, then runs check_dependency_allowed against the merged result end-to-end.
Suggested: Add a test in tests/integration/ that loads org-policy.yml as parent, a repo YAML with no dependencies: block as child, and asserts "test-blocked/pkg" is denied by check_dependency_allowed.
OSS Growth Hacker
- [recommended] CHANGELOG entry should call out the user impact, not just the internal mechanism
The primary adoption segment for policy features is org admins. A changelog entry that leads with the internal fix ("None = no opinion pattern") instead of the user-facing symptom ("org-level deny/require rules were silently dropped") converts zero existing users to upgraders.
Suggested: Reframe: "fix: org-level dependencies.require and dependencies.deny rules are no longer silently dropped for repos that use extends: org without an explicit dependencies block (closes extends: org also fails to layer dependencies.require from parent (same root as #1198) #1201)."
Auth Expert -- inactive
No auth files touched; PR only changes policy inheritance logic (DependencyPolicy sentinel values and merge semantics) with no effect on token management, credential resolution, host classification, or AuthResolver behavior.
Doc Writer
-
[recommended] policy-schema.md line 160 contradicts the new None=transparent semantic for deny/require lists at
docs/src/content/docs/reference/policy-schema.md:160
The docs currently state: "deny and require lists are always tuples (omitted = empty); they accumulate by union during merge." This is now incorrect -- omitted is no longer equivalent to empty. A user reading the reference will wrongly believe omitting the dependencies block overrides the parent with an empty list.
Suggested: Replace with a three-state table mirroring the allow-list table above it: omitted = no opinion (transparent), [] = explicitly empty, [...] = union-merged with parent. Also update the Default column for deny and require from [] to "omitted (null)". -
[nit] CHANGELOG entry uses Python-internal detail (None vs ()) that may confuse YAML authors
YAML users think in terms of omitted keys vs empty lists, not Python None. The entry is accurate but implementation-leaning.
Suggested: Reword to: "omitting the dependencies: block signals no opinion (transparent) while an explicit empty list signals intentional override."
Test Coverage Expert
-
[recommended] Integration merge test does not assert org require flows through when repo omits the require key at
tests/integration/test_policy_discovery_e2e.py:146
test_policy_merge_with_repo_override loads org-policy.yml (which has require: [DevExpGbb/required-standards]) and a repo YAML that omits require, merges them, but does NOT assert merged.dependencies.require contains "DevExpGbb/required-standards". A transparency regression would pass this test silently.
Suggested: Add after existing deny assertions:self.assertIn("DevExpGbb/required-standards", merged.dependencies.require)
Proof (missing at integration-with-fixtures):tests/integration/test_policy_discovery_e2e.py::TestPolicyMerging::test_policy_merge_with_repo_override-- proves: When a repo policy extends: org and omits the require block, the org's required packages still appear in the merged policy. [governed-by-policy,secure-by-default] -
[recommended] Parser has no test for "deny: []" in YAML producing explicit-empty () rather than None at
tests/unit/policy/test_parser.py
Three parser cases exist: (a) dependencies absent -> deny=None, (b) dependencies present but deny absent -> deny=None, (c) deny: [] present and empty -> deny=(). Case (c) -- the explicit-empty override signal -- has no test. Without it, the distinction between absent and explicit-empty can silently break.
Suggested:def test_explicit_empty_deny_produces_empty_tuple(self): yaml_str = "name: p\ndependencies:\n deny: []\n" policy, _ = load_policy(yaml_str) self.assertEqual(policy.dependencies.deny, ()) self.assertIsNotNone(policy.dependencies.deny)
Proof (missing at unit):
tests/unit/policy/test_parser.py::TestLoadPolicyFromString::test_explicit_empty_deny_produces_empty_tuple-- proves: A policy YAML with "deny: []" explicitly overrides the parent deny list with empty rather than being transparent. [governed-by-policy]
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 #1290 · ● 4M · ◷
github-actions
Bot
removed
the
panel-review
…curacy - Replace all non-ASCII characters (em dash U+2014, right-arrow U+2192, section sign U+00A7) in test_discovery.py, policy-schema.md, and governance.md with printable ASCII equivalents ( -- , ->, section) - Correct _merge_list_field docstring: third bullet now accurately describes all three cases (transparent, explicit-empty-override, union) instead of the misleading 'never removes from parent' - Replace 'Merge rules (tighten-only)' headings with unqualified 'Merge rules' + a prose note that deny/require allow [] empty override; same fix applied to the companion governance.md heading and the inline 'tighten-only' reference in section 3 Fixes review threads from PR #1290 (4th round).
Description
Fix policy inheritance merge so that
dependencies.requireanddependencies.denyare correctly layered from the org floor when a repo policy usesextends: org. Uses the sameNone= "no opinion" /()= "explicitly empty" pattern established in theunmanaged_filesfix (#1198).Fixes #1201
Changes
schema.py:deny/requiredefaults changed from()toNone; addedeffective_deny/effective_requirepropertiesparser.py: Detects absent vs present-but-empty dependency blocks, preservesNonefor absentinheritance.py: New_merge_list_field()helper with None-transparency;_merge_dependencies()rewrittenpolicy_checks.py,matcher.py,discovery.py,policy.py: All consumers updated to useeffective_deny/effective_requireCHANGELOG.md: Fixed entry under[Unreleased]Type of change
Testing
TestDependencyTransparency: 8 new tests covering None transparency, explicit empty override, mixed mergestest_both_defaults,test_nested_missing_sections_use_defaults,_make_policyhelper