From fa737d8cb8b8e71f8c9921e59fb5bae3b2322be1 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Tue, 12 May 2026 13:33:36 +0100 Subject: [PATCH 1/5] fix(policy): layer dependencies.require through extends: org (closes #1201) From 2bcb23217d07d8a74dc256f8f005248ccfdb7b17 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Tue, 12 May 2026 13:56:09 +0100 Subject: [PATCH 2/5] fix(policy): layer dependencies.require/deny through extends: org (closes #1201) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + src/apm_cli/commands/policy.py | 4 +- src/apm_cli/policy/discovery.py | 8 +-- src/apm_cli/policy/inheritance.py | 28 +++++++- src/apm_cli/policy/matcher.py | 2 +- src/apm_cli/policy/parser.py | 10 ++- src/apm_cli/policy/policy_checks.py | 12 ++-- src/apm_cli/policy/schema.py | 20 ++++-- .../policy/test_chain_discovery_shared.py | 2 +- tests/unit/policy/test_inheritance.py | 69 ++++++++++++++++++- tests/unit/policy/test_parser.py | 3 +- tests/unit/policy/test_schema.py | 6 +- 12 files changed, 138 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f16280684..cbd4facc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `extends: org` now correctly layers `dependencies.require` and `dependencies.deny` from the parent policy when the child omits the `dependencies:` block entirely; `None` signals "no opinion" (transparent) while `[]` signals explicit override. (#1290) - Pin `Path.home()` under unit tests via a session-scoped autouse conftest fixture, fixing 56 Windows runner failures on the new `windows-2025-vs2026` GitHub-hosted image where `USERPROFILE`/`HOMEDRIVE`+`HOMEPATH` are not seeded for pytest workers; also patch the `_check_and_notify_updates` import binding in the disabled-self-update test so it no longer races on the version-check cache. (#1270) - `apm install` now works on macOS git 2.53.0 (Homebrew): bare-cache commands switch to `--git-dir` to satisfy the `safe.bareRepository=explicit` default; fetched SHAs are pinned as synthetic refs so `git clone --local --shared` no longer silently omits them. (#1268) - Set the unit-test hermetic HOME at conftest import time so a single xdist worker on the `windows-2025-vs2026` runner can no longer race fixture setup and re-trigger the 53 `Path.home()` failures the session-scoped autouse fixture was supposed to prevent. (#1271) diff --git a/src/apm_cli/commands/policy.py b/src/apm_cli/commands/policy.py index add68cb9a..e3edc8bbb 100644 --- a/src/apm_cli/commands/policy.py +++ b/src/apm_cli/commands/policy.py @@ -86,9 +86,9 @@ def _allow_count(value: tuple | None) -> int: return -1 if value is None else len(value) return { - "dependencies_deny": len(policy.dependencies.deny), + "dependencies_deny": len(policy.dependencies.effective_deny), "dependencies_allow": _allow_count(policy.dependencies.allow), - "dependencies_require": len(policy.dependencies.require), + "dependencies_require": len(policy.dependencies.effective_require), "mcp_deny": len(policy.mcp.deny), "mcp_allow": _allow_count(policy.mcp.allow), "mcp_transports_allowed": _allow_count(policy.mcp.transport.allow), diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index e88f65c57..ef6f20e03 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -1049,8 +1049,8 @@ def _opt_list(val: tuple[str, ...] | None) -> list | None: "cache": {"ttl": policy.cache.ttl}, "dependencies": { "allow": _opt_list(policy.dependencies.allow), - "deny": list(policy.dependencies.deny), - "require": list(policy.dependencies.require), + "deny": list(policy.dependencies.effective_deny), + "require": list(policy.dependencies.effective_require), "require_resolution": policy.dependencies.require_resolution, "max_depth": policy.dependencies.max_depth, }, @@ -1104,9 +1104,9 @@ def _is_policy_empty(policy: ApmPolicy) -> bool: beyond the permissive defaults. """ return ( - not policy.dependencies.deny + not policy.dependencies.effective_deny and policy.dependencies.allow is None - and not policy.dependencies.require + and not policy.dependencies.effective_require and not policy.mcp.deny and policy.mcp.allow is None and policy.mcp.transport.allow is None diff --git a/src/apm_cli/policy/inheritance.py b/src/apm_cli/policy/inheritance.py index d5f3024cc..3b2c3f534 100644 --- a/src/apm_cli/policy/inheritance.py +++ b/src/apm_cli/policy/inheritance.py @@ -137,9 +137,9 @@ def _merge_cache(parent: PolicyCache, child: PolicyCache) -> PolicyCache: def _merge_dependencies(parent: DependencyPolicy, child: DependencyPolicy) -> DependencyPolicy: return DependencyPolicy( - deny=_union(parent.deny, child.deny), + deny=_merge_list_field(parent.deny, child.deny), allow=_intersect_allow(parent.allow, child.allow), - require=_union(parent.require, child.require), + require=_merge_list_field(parent.require, child.require), require_resolution=_escalate( _RESOLUTION_LEVELS, parent.require_resolution, child.require_resolution ), @@ -211,6 +211,30 @@ def _merge_unmanaged_files( # --------------------------------------------------------------------------- +def _merge_list_field( + parent: tuple[str, ...] | None, + child: tuple[str, ...] | None, +) -> tuple[str, ...] | None: + """Merge a deny/require list field with None-transparency and union. + + * ``child is None`` -- no opinion; parent flows through (transparent). + * ``child`` is empty -- explicit empty; overrides parent with ``()``. + * both truthy -- union (child adds, never removes from parent). + + Always returns a ``tuple`` or ``None``; never a bare list. + """ + if child is None: + # Transparent: parent flows through. Normalise to tuple if non-None + # so callers always receive a uniform type regardless of how parent + # was constructed in tests (list vs tuple). + return _union((), parent) if parent is not None else None + if not child: + return () # explicit empty: override parent + if parent is None or not parent: + return _union((), child) # parent has nothing; child wins + return _union(parent, child) # both have values: union + + def _union(a: tuple[str, ...], b: tuple[str, ...]) -> tuple[str, ...]: """Deduplicated union preserving first-seen order.""" seen: set[str] = set() diff --git a/src/apm_cli/policy/matcher.py b/src/apm_cli/policy/matcher.py index cf40e491d..db0be46c0 100644 --- a/src/apm_cli/policy/matcher.py +++ b/src/apm_cli/policy/matcher.py @@ -73,7 +73,7 @@ def check_dependency_allowed( policy: DependencyPolicy, ) -> tuple[bool, str]: """Check if a dependency is allowed by policy.""" - return _check_allow_deny(canonical_ref, policy.allow, policy.deny) + return _check_allow_deny(canonical_ref, policy.allow, policy.effective_deny) def check_mcp_allowed( diff --git a/src/apm_cli/policy/parser.py b/src/apm_cli/policy/parser.py index 8093716c9..ceb1451aa 100644 --- a/src/apm_cli/policy/parser.py +++ b/src/apm_cli/policy/parser.py @@ -163,11 +163,15 @@ def _build_policy(data: dict) -> ApmPolicy: ttl=cache_data.get("ttl", PolicyCache.ttl), ) - deps_data = data.get("dependencies") or {} + _raw_deps = data.get("dependencies") + deps_data = _raw_deps if isinstance(_raw_deps, dict) else {} + _deps_absent = _raw_deps is None dependencies = DependencyPolicy( allow=_parse_allow(deps_data.get("allow")), - deny=_parse_tuple(deps_data.get("deny")), - require=_parse_tuple(deps_data.get("require")), + deny=None if (_deps_absent or "deny" not in deps_data) else _parse_tuple(deps_data["deny"]), + require=None + if (_deps_absent or "require" not in deps_data) + else _parse_tuple(deps_data["require"]), require_resolution=deps_data.get("require_resolution", DependencyPolicy.require_resolution), max_depth=deps_data.get("max_depth", DependencyPolicy.max_depth), ) diff --git a/src/apm_cli/policy/policy_checks.py b/src/apm_cli/policy/policy_checks.py index 6fcbb863e..6780ae3a6 100644 --- a/src/apm_cli/policy/policy_checks.py +++ b/src/apm_cli/policy/policy_checks.py @@ -111,7 +111,7 @@ def _check_dependency_denylist( """Check 2: no dependency matches policy deny list.""" from .matcher import check_dependency_allowed - if not policy.deny: + if not policy.effective_deny: return CheckResult( name="dependency-denylist", passed=True, @@ -144,7 +144,7 @@ def _check_required_packages( policy: DependencyPolicy, ) -> CheckResult: """Check 3: every required package is in manifest deps.""" - if not policy.require: + if not policy.effective_require: return CheckResult( name="required-packages", passed=True, @@ -153,7 +153,7 @@ def _check_required_packages( dep_names = {dep.get_canonical_dependency_string().split("#")[0] for dep in deps} missing: list[str] = [] - for req in policy.require: + for req in policy.effective_require: pkg_name = req.split("#")[0] if pkg_name not in dep_names: missing.append(pkg_name) @@ -178,7 +178,7 @@ def _check_required_packages_deployed( policy: DependencyPolicy, ) -> CheckResult: """Check 4: required packages appear in lockfile with deployed files.""" - if not policy.require or lock is None: + if not policy.effective_require or lock is None: return CheckResult( name="required-packages-deployed", passed=True, @@ -188,7 +188,7 @@ def _check_required_packages_deployed( dep_names = {dep.get_canonical_dependency_string().split("#")[0] for dep in deps} lock_by_name = {locked.get_unique_key(): locked for _key, locked in lock.dependencies.items()} not_deployed: list[str] = [] - for req in policy.require: + for req in policy.effective_require: pkg_name = req.split("#")[0] if pkg_name not in dep_names: continue # not in manifest -- check 3 handles this @@ -218,7 +218,7 @@ def _check_required_package_version( policy: DependencyPolicy, ) -> CheckResult: """Check 5: required packages with version pins match per resolution strategy.""" - pinned = [(r, r.split("#", 1)) for r in policy.require if "#" in r] + pinned = [(r, r.split("#", 1)) for r in policy.effective_require if "#" in r] if not pinned or lock is None: return CheckResult( name="required-package-version", diff --git a/src/apm_cli/policy/schema.py b/src/apm_cli/policy/schema.py index 4b17c9e28..07315efcb 100644 --- a/src/apm_cli/policy/schema.py +++ b/src/apm_cli/policy/schema.py @@ -7,8 +7,10 @@ * ``()`` -- "explicitly empty" (after merge: nothing is allowed). * ``(...)`` -- "allow only matching patterns". -Deny/require lists use ``Tuple[str, ...]`` (never ``None``) because -they accumulate via union -- an absent key simply means "nothing to add". +Deny/require list semantics: + * ``None`` -- "no opinion" (transparent during inheritance merge). + * ``()`` -- "explicitly empty" (overrides parent in merge). + * ``(...)`` -- union-merged with parent during inheritance. """ from __future__ import annotations @@ -29,11 +31,21 @@ class DependencyPolicy: """Rules governing which APM dependencies are permitted.""" allow: tuple[str, ...] | None = None - deny: tuple[str, ...] = () - require: tuple[str, ...] = () + deny: tuple[str, ...] | None = None # None = no opinion; () = explicit empty + require: tuple[str, ...] | None = None # None = no opinion; () = explicit empty require_resolution: str = "project-wins" # project-wins | policy-wins | block max_depth: int = 50 + @property + def effective_deny(self) -> tuple[str, ...]: + """Resolved deny list for runtime checks (None -> ()).""" + return self.deny if self.deny is not None else () + + @property + def effective_require(self) -> tuple[str, ...]: + """Resolved require list for runtime checks (None -> ()).""" + return self.require if self.require is not None else () + @dataclass(frozen=True) class McpTransportPolicy: diff --git a/tests/unit/policy/test_chain_discovery_shared.py b/tests/unit/policy/test_chain_discovery_shared.py index 24279e244..3293408c5 100644 --- a/tests/unit/policy/test_chain_discovery_shared.py +++ b/tests/unit/policy/test_chain_discovery_shared.py @@ -34,7 +34,7 @@ # -- Helpers --------------------------------------------------------------- -def _make_policy(*, enforcement="warn", extends=None, deny=()): +def _make_policy(*, enforcement="warn", extends=None, deny=None): """Build a minimal ApmPolicy for testing.""" return ApmPolicy( enforcement=enforcement, diff --git a/tests/unit/policy/test_inheritance.py b/tests/unit/policy/test_inheritance.py index 6872d7fd4..4e3602cb3 100644 --- a/tests/unit/policy/test_inheritance.py +++ b/tests/unit/policy/test_inheritance.py @@ -156,6 +156,72 @@ def test_deduplication(self): self.assertEqual(result.dependencies.require, ("contoso/hooks",)) +class TestDependencyTransparency(unittest.TestCase): + """Child omitting dependencies block is transparent for deny/require (fixes #1201).""" + + def test_parent_require_child_omits_deps_block(self): + """Parent require + child omits dependencies entirely -> require flows through.""" + result = merge_policies( + ApmPolicy(dependencies=DependencyPolicy(require=("contoso/hooks",))), + ApmPolicy(), # child omits dependencies -> require=None + ) + self.assertEqual(result.dependencies.require, ("contoso/hooks",)) + + def test_parent_deny_child_omits_deps_block(self): + """Parent deny + child omits dependencies entirely -> deny flows through.""" + result = merge_policies( + ApmPolicy(dependencies=DependencyPolicy(deny=("evil/*",))), + ApmPolicy(), # child omits dependencies -> deny=None + ) + self.assertEqual(result.dependencies.deny, ("evil/*",)) + + def test_parent_require_child_explicit_empty_require(self): + """Child explicit empty require=() overrides parent (AC#2).""" + result = merge_policies( + ApmPolicy(dependencies=DependencyPolicy(require=("contoso/hooks",))), + ApmPolicy(dependencies=DependencyPolicy(require=())), + ) + self.assertEqual(result.dependencies.require, ()) + + def test_parent_deny_child_explicit_empty_deny(self): + """Child explicit empty deny=() overrides parent.""" + result = merge_policies( + ApmPolicy(dependencies=DependencyPolicy(deny=("evil/*",))), + ApmPolicy(dependencies=DependencyPolicy(deny=())), + ) + self.assertEqual(result.dependencies.deny, ()) + + def test_three_level_chain_require_transparency(self): + """Enterprise require -> org omits -> repo omits -> require preserved.""" + result = resolve_policy_chain( + [ + ApmPolicy(dependencies=DependencyPolicy(require=("contoso/core",))), + ApmPolicy(), # org omits + ApmPolicy(), # repo omits + ] + ) + self.assertEqual(result.dependencies.require, ("contoso/core",)) + + def test_three_level_chain_deny_transparency(self): + """Enterprise deny -> org omits -> repo omits -> deny preserved.""" + result = resolve_policy_chain( + [ + ApmPolicy(dependencies=DependencyPolicy(deny=("banned/*",))), + ApmPolicy(), # org omits + ApmPolicy(), # repo omits + ] + ) + self.assertEqual(result.dependencies.deny, ("banned/*",)) + + def test_both_none_merged_none(self): + """Both parent and child omit dependencies -> None (no opinion).""" + result = merge_policies(ApmPolicy(), ApmPolicy()) + self.assertIsNone(result.dependencies.deny) + self.assertIsNone(result.dependencies.require) + self.assertEqual(result.dependencies.effective_deny, ()) + self.assertEqual(result.dependencies.effective_require, ()) + + class TestRequireResolutionEscalation(unittest.TestCase): """require_resolution: project-wins < policy-wins < block.""" @@ -601,7 +667,8 @@ def test_both_defaults(self): result = merge_policies(ApmPolicy(), ApmPolicy()) self.assertEqual(result.enforcement, "warn") self.assertEqual(result.cache.ttl, 3600) - self.assertEqual(result.dependencies.deny, ()) + self.assertIsNone(result.dependencies.deny) # None = no opinion from either side + self.assertEqual(result.dependencies.effective_deny, ()) self.assertIsNone(result.dependencies.allow) def test_extends_cleared_after_merge(self): diff --git a/tests/unit/policy/test_parser.py b/tests/unit/policy/test_parser.py index 8fe9e8399..ac534fb9f 100644 --- a/tests/unit/policy/test_parser.py +++ b/tests/unit/policy/test_parser.py @@ -274,7 +274,8 @@ def test_nested_missing_sections_use_defaults(self): """) policy, warnings = load_policy(yaml_str) # noqa: RUF059 self.assertEqual(policy.dependencies.allow, ("org/*",)) - self.assertEqual(policy.dependencies.deny, ()) + self.assertIsNone(policy.dependencies.deny) + self.assertEqual(policy.dependencies.effective_deny, ()) self.assertEqual(policy.dependencies.max_depth, 50) self.assertEqual(policy.mcp.self_defined, "warn") diff --git a/tests/unit/policy/test_schema.py b/tests/unit/policy/test_schema.py index 6eea415aa..d26818185 100644 --- a/tests/unit/policy/test_schema.py +++ b/tests/unit/policy/test_schema.py @@ -39,8 +39,10 @@ class TestDependencyPolicyDefaults(unittest.TestCase): def test_defaults(self): dep = DependencyPolicy() self.assertIsNone(dep.allow) - self.assertEqual(dep.deny, ()) - self.assertEqual(dep.require, ()) + self.assertIsNone(dep.deny) + self.assertIsNone(dep.require) + self.assertEqual(dep.effective_deny, ()) + self.assertEqual(dep.effective_require, ()) self.assertEqual(dep.require_resolution, "project-wins") self.assertEqual(dep.max_depth, 50) From 0804caedab939e9d58dce8aff4a728f8a9a097aa Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Tue, 12 May 2026 14:17:23 +0100 Subject: [PATCH 3/5] fix(policy): three Copilot review follow-ups for tri-state deny/require MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../content/docs/reference/policy-schema.md | 14 ++++-- .../.apm/skills/apm-usage/governance.md | 4 +- src/apm_cli/policy/discovery.py | 4 +- src/apm_cli/policy/parser.py | 6 ++- tests/unit/policy/test_discovery.py | 48 +++++++++++++++++++ tests/unit/policy/test_parser.py | 20 ++++++++ 6 files changed, 86 insertions(+), 10 deletions(-) diff --git a/docs/src/content/docs/reference/policy-schema.md b/docs/src/content/docs/reference/policy-schema.md index 155a1a537..d5393e044 100644 --- a/docs/src/content/docs/reference/policy-schema.md +++ b/docs/src/content/docs/reference/policy-schema.md @@ -67,8 +67,8 @@ Rules over the `dependencies:` and `mcp:` blocks declared in consumer `apm.yml` | Field | Type | Default | Notes | |----------------------|-----------------------|------------------|---------------------------------------------------------------------------------------------| | `allow` | list of patterns or null | `null` | `null` = no opinion. `[]` = nothing allowed. `[...]` = only these. | -| `deny` | list of patterns | `[]` | Always wins over `allow`. | -| `require` | list of refs | `[]` | Packages every consumer manifest must include. | +| `deny` | list of patterns or null | `null` | `null` = no opinion (transparent during merge). `[]` = explicitly empty. Always wins over `allow`. | +| `require` | list of refs or null | `null` | `null` = no opinion (transparent during merge). `[]` = explicitly empty. Packages every consumer manifest must include. | | `require_resolution` | enum | `project-wins` | `project-wins` / `policy-wins` / `block` -- how to resolve version conflicts on required packages. | | `max_depth` | integer | `50` | Maximum transitive dependency depth. Must be `> 0`. | @@ -135,7 +135,7 @@ The child can tighten the parent but never relax it: | `fetch_failure` | Child overrides if set. | | `cache.ttl` | `min(parent, child)`. | | `*.allow` lists | Set intersection. `null` is transparent (no opinion). | -| `*.deny` / `require` lists | Union, deduplicated, parent order preserved. | +| `*.deny` / `require` lists | Union, deduplicated, parent order preserved. Omitting the field (or setting it to `null`) is transparent — the parent value passes through unchanged. `[]` is an explicit empty override. | | `dependencies.max_depth` | `min(parent, child)`. | | `dependencies.require_resolution` | Stricter wins (`block` > `policy-wins` > `project-wins`). | | `mcp.self_defined` | Stricter wins (`deny` > `warn` > `allow`). | @@ -157,7 +157,13 @@ For every `allow:` field, the three states are distinct: | `[]` | "explicitly nothing" | Intersects to nothing downstream. | | `[...]` | "only these patterns" | Intersected with child list. | -`deny` and `require` lists are always tuples (omitted = empty); they accumulate by union during merge. +`deny` and `require` lists support the same three-state semantics as `allow`: + +| Value | Meaning | Inheritance behavior | +|----------|----------------------------|----------------------------------------------------------| +| omitted / `null` | "no opinion" | Transparent during merge — parent value passes through. | +| `[]` | "explicitly empty" | Overrides parent; no entries accumulate. | +| `[...]` | "these entries" | Unioned with parent list (parent order preserved). | ## Complete example diff --git a/packages/apm-guide/.apm/skills/apm-usage/governance.md b/packages/apm-guide/.apm/skills/apm-usage/governance.md index 04c5e14c4..dd0defd3d 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/governance.md +++ b/packages/apm-guide/.apm/skills/apm-usage/governance.md @@ -85,8 +85,8 @@ Child policies can only tighten parent policies, never relax them: |-------|-----------| | `enforcement` | Escalates: `off` < `warn` < `block` | | Allow lists | Intersection (child narrows parent) | -| Deny lists | Union (child adds to parent) | -| `require` | Union (combines required packages) | +| Deny lists | Union (child adds to parent). Omitting or `null` = transparent; `[]` = explicit empty override. | +| `require` | Union (combines required packages). Omitting or `null` = transparent; `[]` = explicit empty override. | | `max_depth` | `min(parent, child)` | | `mcp.self_defined` | Escalates: `allow` < `warn` < `deny` | | `source_attribution` | `parent OR child` (either enables) | diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index ef6f20e03..b2ccd6e10 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -1049,8 +1049,8 @@ def _opt_list(val: tuple[str, ...] | None) -> list | None: "cache": {"ttl": policy.cache.ttl}, "dependencies": { "allow": _opt_list(policy.dependencies.allow), - "deny": list(policy.dependencies.effective_deny), - "require": list(policy.dependencies.effective_require), + "deny": _opt_list(policy.dependencies.deny), + "require": _opt_list(policy.dependencies.require), "require_resolution": policy.dependencies.require_resolution, "max_depth": policy.dependencies.max_depth, }, diff --git a/src/apm_cli/policy/parser.py b/src/apm_cli/policy/parser.py index ceb1451aa..ab7bc3169 100644 --- a/src/apm_cli/policy/parser.py +++ b/src/apm_cli/policy/parser.py @@ -168,9 +168,11 @@ def _build_policy(data: dict) -> ApmPolicy: _deps_absent = _raw_deps is None dependencies = DependencyPolicy( allow=_parse_allow(deps_data.get("allow")), - deny=None if (_deps_absent or "deny" not in deps_data) else _parse_tuple(deps_data["deny"]), + deny=None + if (_deps_absent or "deny" not in deps_data or deps_data["deny"] is None) + else _parse_tuple(deps_data["deny"]), require=None - if (_deps_absent or "require" not in deps_data) + if (_deps_absent or "require" not in deps_data or deps_data["require"] is None) else _parse_tuple(deps_data["require"]), require_resolution=deps_data.get("require_resolution", DependencyPolicy.require_resolution), max_depth=deps_data.get("max_depth", DependencyPolicy.max_depth), diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index acc87f465..2f804239b 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -261,6 +261,54 @@ def test_get_cache_dir(self): expected = root.resolve() / "apm_modules" / ".policy-cache" self.assertEqual(_get_cache_dir(root), expected) + def test_round_trip_preserves_none_deny_and_require(self): + """Cache write→read must preserve deny=None/require=None (tri-state Fix 1). + + A policy with no dependencies: block must survive a cache round-trip + as None, not collapse to () which would prevent parent inheritance. + """ + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + repo_ref = "contoso/.github" + + # Policy with no dependencies: block → deny=None, require=None + policy, _ = load_policy("name: p\nversion: '1'\nenforcement: warn\n") + self.assertIsNone(policy.dependencies.deny) + self.assertIsNone(policy.dependencies.require) + + _write_cache(repo_ref, policy, root) + result = _read_cache(repo_ref, root) + + self.assertIsNotNone(result) + self.assertIsNone( + result.policy.dependencies.deny, + "deny must survive cache round-trip as None, not collapse to ()", + ) + self.assertIsNone( + result.policy.dependencies.require, + "require must survive cache round-trip as None, not collapse to ()", + ) + + def test_round_trip_preserves_explicit_empty_deny(self): + """Cache round-trip must preserve deny=() (explicit empty override).""" + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + repo_ref = "contoso/.github" + + yaml_str = "name: p\nversion: '1'\nenforcement: warn\ndependencies:\n deny: []\n" + policy, _ = load_policy(yaml_str) + self.assertEqual(policy.dependencies.deny, ()) + + _write_cache(repo_ref, policy, root) + result = _read_cache(repo_ref, root) + + self.assertIsNotNone(result) + self.assertEqual( + result.policy.dependencies.deny, + (), + "deny=[] must survive cache round-trip as () (explicit empty)", + ) + class TestFetchGithubContents(unittest.TestCase): """Test _fetch_github_contents with mocked requests.""" diff --git a/tests/unit/policy/test_parser.py b/tests/unit/policy/test_parser.py index ac534fb9f..87f61be0c 100644 --- a/tests/unit/policy/test_parser.py +++ b/tests/unit/policy/test_parser.py @@ -326,6 +326,26 @@ def test_omitted_unmanaged_files_yields_none_action(self): self.assertEqual(policy.unmanaged_files.effective_action, "ignore") self.assertEqual(policy.unmanaged_files.directories, ()) + def test_absent_dependencies_block_gives_none_deny_and_require(self): + """Entirely absent dependencies: block -> deny=None, require=None (Fix 2).""" + policy, _ = load_policy("name: p\nversion: '1'\nenforcement: warn\n") + self.assertIsNone(policy.dependencies.deny, "absent block must yield deny=None") + self.assertIsNone(policy.dependencies.require, "absent block must yield require=None") + + def test_yaml_null_deny_gives_none(self): + """YAML 'deny: null' (or bare 'deny:') must be treated as no opinion, not empty list (Fix 2).""" + yaml_str = "name: p\nversion: '1'\nenforcement: warn\ndependencies:\n deny:\n require:\n" + policy, _ = load_policy(yaml_str) + self.assertIsNone(policy.dependencies.deny, "deny: null must yield None, not ()") + self.assertIsNone(policy.dependencies.require, "require: null must yield None, not ()") + + def test_explicit_empty_deny_list_gives_empty_tuple(self): + """Explicit 'deny: []' must give () (explicit empty override), not None.""" + yaml_str = "name: p\nversion: '1'\nenforcement: warn\ndependencies:\n deny: []\n require: []\n" + policy, _ = load_policy(yaml_str) + self.assertEqual(policy.dependencies.deny, (), "deny: [] must yield ()") + self.assertEqual(policy.dependencies.require, (), "require: [] must yield ()") + class TestLoadPolicyFromFile(unittest.TestCase): """Test load_policy from a file path.""" From 98026c781d56c35d18bab8d0781d5432470593c1 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Tue, 12 May 2026 14:18:50 +0100 Subject: [PATCH 4/5] style: ruff format test_parser.py --- tests/unit/policy/test_parser.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/policy/test_parser.py b/tests/unit/policy/test_parser.py index 87f61be0c..3b4bbffb4 100644 --- a/tests/unit/policy/test_parser.py +++ b/tests/unit/policy/test_parser.py @@ -341,7 +341,9 @@ def test_yaml_null_deny_gives_none(self): def test_explicit_empty_deny_list_gives_empty_tuple(self): """Explicit 'deny: []' must give () (explicit empty override), not None.""" - yaml_str = "name: p\nversion: '1'\nenforcement: warn\ndependencies:\n deny: []\n require: []\n" + yaml_str = ( + "name: p\nversion: '1'\nenforcement: warn\ndependencies:\n deny: []\n require: []\n" + ) policy, _ = load_policy(yaml_str) self.assertEqual(policy.dependencies.deny, (), "deny: [] must yield ()") self.assertEqual(policy.dependencies.require, (), "require: [] must yield ()") From 44144f3f0e893b7944bedccac8872da4954676af Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Tue, 12 May 2026 14:33:32 +0100 Subject: [PATCH 5/5] fix: resolve 4th Copilot review round -- ASCII, docstring, heading accuracy - 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). --- .../content/docs/reference/policy-schema.md | 10 +++--- .../.apm/skills/apm-usage/governance.md | 34 ++++++++++--------- src/apm_cli/policy/inheritance.py | 7 ++-- tests/unit/policy/test_discovery.py | 6 ++-- 4 files changed, 32 insertions(+), 25 deletions(-) diff --git a/docs/src/content/docs/reference/policy-schema.md b/docs/src/content/docs/reference/policy-schema.md index d5393e044..2d9c569a0 100644 --- a/docs/src/content/docs/reference/policy-schema.md +++ b/docs/src/content/docs/reference/policy-schema.md @@ -125,9 +125,11 @@ Files in primitive target directories that are not recorded in `apm.lock.yaml`. For supply-chain safety, `extends:` references are pinned to the **leaf policy's host** -- a policy fetched from `github.com` cannot extend one on `evil.example.com`. -### Merge rules (tighten-only) +### Merge rules -The child can tighten the parent but never relax it: +Most fields tighten as the policy chain descends. The exceptions are +`deny`/`require` lists, where a child may use `[]` to explicitly clear an +inherited list (see the tri-state table below). | Field family | Merge rule | |-----------------------------|----------------------------------------------------------------------------------| @@ -135,7 +137,7 @@ The child can tighten the parent but never relax it: | `fetch_failure` | Child overrides if set. | | `cache.ttl` | `min(parent, child)`. | | `*.allow` lists | Set intersection. `null` is transparent (no opinion). | -| `*.deny` / `require` lists | Union, deduplicated, parent order preserved. Omitting the field (or setting it to `null`) is transparent — the parent value passes through unchanged. `[]` is an explicit empty override. | +| `*.deny` / `require` lists | Union, deduplicated, parent order preserved. Omitting the field (or setting it to `null`) is transparent -- the parent value passes through unchanged. `[]` is an explicit empty override. | | `dependencies.max_depth` | `min(parent, child)`. | | `dependencies.require_resolution` | Stricter wins (`block` > `policy-wins` > `project-wins`). | | `mcp.self_defined` | Stricter wins (`deny` > `warn` > `allow`). | @@ -161,7 +163,7 @@ For every `allow:` field, the three states are distinct: | Value | Meaning | Inheritance behavior | |----------|----------------------------|----------------------------------------------------------| -| omitted / `null` | "no opinion" | Transparent during merge — parent value passes through. | +| omitted / `null` | "no opinion" | Transparent during merge -- parent value passes through. | | `[]` | "explicitly empty" | Overrides parent; no entries accumulate. | | `[...]` | "these entries" | Unioned with parent list (parent order preserved). | diff --git a/packages/apm-guide/.apm/skills/apm-usage/governance.md b/packages/apm-guide/.apm/skills/apm-usage/governance.md index dd0defd3d..9f231a312 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/governance.md +++ b/packages/apm-guide/.apm/skills/apm-usage/governance.md @@ -77,9 +77,11 @@ undeclared local content at install / audit time. | `warn` | Violations reported but do not fail | | `block` | Violations abort `apm install` (exit 1) AND fail `apm audit --ci` | -## Inheritance rules (tighten-only) +## Inheritance rules -Child policies can only tighten parent policies, never relax them: +Most fields tighten as the policy chain descends. The exception is `deny` and +`require` lists: a child policy may use `[]` to explicitly clear an inherited +list (removing entries the parent set). All other fields obey the rules below: | Field | Merge rule | |-------|-----------| @@ -138,7 +140,7 @@ apm audit --ci --policy https://... # remote policy URL **Note:** Install-time policy enforcement (issue #827) is in active development. The behaviour described below reflects the shipping design. -**Non-goal — structured output:** install-time enforcement does NOT emit JSON or SARIF. Output is human-readable terminal text only. For machine-readable policy reports use `apm audit --ci --format json` or `apm audit --ci --format sarif`. +**Non-goal -- structured output:** install-time enforcement does NOT emit JSON or SARIF. Output is human-readable terminal text only. For machine-readable policy reports use `apm audit --ci --format json` or `apm audit --ci --format sarif`. ### 1. What APM policy is @@ -149,18 +151,18 @@ may use. This section covers how that contract is enforced at `apm install` time ### 2. Discovery and applicability APM auto-discovers policy from `/.github/apm-policy.yml` for any GitHub -remote — both `github.com` and GitHub Enterprise (GHE). Non-GitHub remotes (ADO, +remote -- both `github.com` and GitHub Enterprise (GHE). Non-GitHub remotes (ADO, GitLab, plain git) currently fall through with no policy applied; tracked as a follow-up. Repositories with no detectable git remote (unpacked bundles, temp dirs) emit an explicit "could not determine org" line and skip discovery. -The `--policy ` flag is **audit-only today** — it works on +The `--policy ` flag is **audit-only today** -- it works on `apm audit --ci` but is not yet wired through `apm install`. ### 3. Inheritance and composition Policy resolves through the chain: enterprise hub -> org -> repo override. -The merge is **tighten-only** (see "Inheritance rules" above). +The merge follows "Inheritance rules" above (most fields tighten; deny/require lists support explicit `[]` override). **Multi-level extends:** install-time enforcement and `apm audit --ci` both resolve the full `extends:` chain up to `MAX_CHAIN_DEPTH = 5`. Cycles are @@ -179,12 +181,12 @@ merges what it resolved and emits a `Policy chain incomplete` warning. | Command | Behaviour | |---------|-----------| -| `apm install` | NEW — gate runs after resolve, before integration / target writes | -| `apm install ` | NEW — snapshot apm.yml, run gate, rollback on block | -| `apm install --mcp` | NEW — dedicated MCP preflight | -| `apm deps update` | NEW — runs the install pipeline, so the same gate applies | -| `apm install --dry-run` | NEW — read-only preflight; renders "would be blocked" | -| `apm audit --ci` | Existing — same checks against on-disk manifest + lockfile | +| `apm install` | NEW -- gate runs after resolve, before integration / target writes | +| `apm install ` | NEW -- snapshot apm.yml, run gate, rollback on block | +| `apm install --mcp` | NEW -- dedicated MCP preflight | +| `apm deps update` | NEW -- runs the install pipeline, so the same gate applies | +| `apm install --dry-run` | NEW -- read-only preflight; renders "would be blocked" | +| `apm audit --ci` | Existing -- same checks against on-disk manifest + lockfile | `pack` and `bundle` are out of scope (author-side, not dependency consumers). @@ -194,9 +196,9 @@ merges what it resolved and emits a `Policy chain incomplete` warning. `require_resolution: project-wins` has a narrow semantic: - Downgrades **version-pin mismatches** on required packages to warnings only. -- Does **NOT** downgrade missing required packages — those still block under +- Does **NOT** downgrade missing required packages -- those still block under `enforcement: block`. -- Does **NOT** override an inherited org `deny` — parent deny always wins. +- Does **NOT** override an inherited org `deny` -- parent deny always wins. ### 7. CLI examples @@ -385,7 +387,7 @@ Violation classes: | `transitive_mcp` | MCP server pulled in by a transitive dep, blocked by `mcp.deny` / `transport` / `self_defined` | Remove offending dep, request policy update, or set `mcp.trust_transitive: true` | Full message text per outcome and per class lives in -`docs/src/content/docs/enterprise/policy-reference.md` §10. Violation messages +`docs/src/content/docs/enterprise/policy-reference.md` section10. Violation messages flow through `InstallLogger.policy_violation`; under `block` they print inline as `[x]` errors and exit `1`. @@ -399,7 +401,7 @@ Checklist to publish a policy: 3. Set `enforcement: warn` first. Let CI surface diagnostics across consuming repos for one cycle without breaking installs. 4. When the warn-cycle is clean, switch to `enforcement: block`. Communicate - the change — `apm install` will start failing for non-compliant repos. + the change -- `apm install` will start failing for non-compliant repos. 5. Use `extends:` for team-specific overrides on top of the org baseline rather than forking the file. diff --git a/src/apm_cli/policy/inheritance.py b/src/apm_cli/policy/inheritance.py index 3b2c3f534..cb2b67b40 100644 --- a/src/apm_cli/policy/inheritance.py +++ b/src/apm_cli/policy/inheritance.py @@ -218,8 +218,11 @@ def _merge_list_field( """Merge a deny/require list field with None-transparency and union. * ``child is None`` -- no opinion; parent flows through (transparent). - * ``child`` is empty -- explicit empty; overrides parent with ``()``. - * both truthy -- union (child adds, never removes from parent). + * ``child`` is empty -- explicit empty override; clears parent entries, + returning ``()``. Child can use ``[]`` in YAML to clear an inherited + deny/require list. + * both truthy -- union; child entries are added to parent entries + (deduped, parent order preserved). Always returns a ``tuple`` or ``None``; never a bare list. """ diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index 2f804239b..bc9f3af03 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -1,4 +1,4 @@ -"""Tests for apm_cli.policy.discovery — policy auto-discovery engine.""" +"""Tests for apm_cli.policy.discovery -- policy auto-discovery engine.""" from __future__ import annotations @@ -262,7 +262,7 @@ def test_get_cache_dir(self): self.assertEqual(_get_cache_dir(root), expected) def test_round_trip_preserves_none_deny_and_require(self): - """Cache write→read must preserve deny=None/require=None (tri-state Fix 1). + """Cache write->read must preserve deny=None/require=None (tri-state Fix 1). A policy with no dependencies: block must survive a cache round-trip as None, not collapse to () which would prevent parent inheritance. @@ -271,7 +271,7 @@ def test_round_trip_preserves_none_deny_and_require(self): root = Path(tmpdir) repo_ref = "contoso/.github" - # Policy with no dependencies: block → deny=None, require=None + # Policy with no dependencies: block -> deny=None, require=None policy, _ = load_policy("name: p\nversion: '1'\nenforcement: warn\n") self.assertIsNone(policy.dependencies.deny) self.assertIsNone(policy.dependencies.require)