From 5de2b57fa493337d11ffbe01da6d34863ea8ab68 Mon Sep 17 00:00:00 2001 From: abhinavgautam01 Date: Sun, 10 May 2026 22:29:42 +0530 Subject: [PATCH 1/2] fix(policy): inherit parent unmanaged_files when child omits block Omitted or empty unmanaged_files on an extending policy is transparent (None/None), so org action: deny and directories are not replaced by defaults that looked like explicit ignore. --- CHANGELOG.md | 1 + src/apm_cli/commands/policy.py | 2 +- src/apm_cli/policy/discovery.py | 4 +-- src/apm_cli/policy/inheritance.py | 38 ++++++++++++++++++++++++--- src/apm_cli/policy/parser.py | 17 ++++++++---- src/apm_cli/policy/policy_checks.py | 2 +- src/apm_cli/policy/schema.py | 16 ++++++++--- tests/unit/policy/test_fixtures.py | 3 ++- tests/unit/policy/test_inheritance.py | 27 +++++++++++++++++++ tests/unit/policy/test_parser.py | 2 ++ tests/unit/policy/test_schema.py | 4 +-- 11 files changed, 97 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c14c14ced..843bfa6c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Policy inheritance:** a child policy that uses `extends:` but omits `unmanaged_files:` no longer silently downgrades the parent org `action: deny`; the child block is treated as fully transparent (same semantics as allow-list `None` in merge). (#1198) - `apm marketplace add` accepts GitLab-class hosts (`gitlab.com` and self-managed instances configured via `GITLAB_HOST` / `APM_GITLAB_HOSTS`); unsupported generic hosts now show separate recovery hints for GHES (`GITHUB_HOST`) and self-managed GitLab instead of only `GITHUB_HOST`. (#1149) - **GitLab monorepo marketplaces:** `apm install plugin@marketplace` now resolves plugins whose sources live in a subdirectory of the marketplace repository on GitLab-class hosts (`gitlab.com` and self-managed GitLab when classified as GitLab), matching explicit `git:` + `path:` semantics without requiring that hand-written object form. (#1149) - `apm install` now rejects unsupported flat-format `dependencies` (e.g. `dependencies: [owner/repo]`) with a clear error and structured-format hint instead of silently ignoring them; the resolver also surfaces `ValueError` from malformed transitive manifests as warnings instead of swallowing them. (#1189) diff --git a/src/apm_cli/commands/policy.py b/src/apm_cli/commands/policy.py index add68cb9a..dbc140f79 100644 --- a/src/apm_cli/commands/policy.py +++ b/src/apm_cli/commands/policy.py @@ -94,7 +94,7 @@ def _allow_count(value: tuple | None) -> int: "mcp_transports_allowed": _allow_count(policy.mcp.transport.allow), "compilation_targets_allowed": _allow_count(policy.compilation.target.allow), "manifest_required_fields": len(policy.manifest.required_fields), - "unmanaged_files_directories": len(policy.unmanaged_files.directories), + "unmanaged_files_directories": len(policy.unmanaged_files.directories or ()), } diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 70044081b..d7c95f207 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -1080,7 +1080,7 @@ def _opt_list(val: tuple[str, ...] | None) -> list | None: }, "unmanaged_files": { "action": policy.unmanaged_files.action, - "directories": list(policy.unmanaged_files.directories), + "directories": list(policy.unmanaged_files.directories or ()), }, } @@ -1114,7 +1114,7 @@ def _is_policy_empty(policy: ApmPolicy) -> bool: and not policy.manifest.required_fields and policy.manifest.scripts == "allow" and policy.manifest.content_types is None - and policy.unmanaged_files.action == "ignore" + and policy.unmanaged_files.action in (None, "ignore") ) diff --git a/src/apm_cli/policy/inheritance.py b/src/apm_cli/policy/inheritance.py index 0229883f1..0b6fd88bd 100644 --- a/src/apm_cli/policy/inheritance.py +++ b/src/apm_cli/policy/inheritance.py @@ -191,13 +191,43 @@ def _merge_manifest(parent: ManifestPolicy, child: ManifestPolicy) -> ManifestPo ) +def _coerce_unmanaged_action_for_escalate(value: str | None) -> str: + """Treat ``None`` as the weakest rung when comparing two concrete opinions.""" + return "ignore" if value is None else value + + +def _coerce_unmanaged_directories_for_union(value: tuple[str, ...] | None) -> tuple[str, ...]: + return () if value is None else value + + def _merge_unmanaged_files( parent: UnmanagedFilesPolicy, child: UnmanagedFilesPolicy ) -> UnmanagedFilesPolicy: - return UnmanagedFilesPolicy( - action=_escalate(_UNMANAGED_ACTION_LEVELS, parent.action, child.action), - directories=_union(parent.directories, child.directories), - ) + """Merge unmanaged-files policy; omitted child block is transparent (#1198).""" + if child.action is None and child.directories is None: + return parent + + if child.action is None: + eff_action_raw = parent.action + else: + eff_action_raw = _escalate( + _UNMANAGED_ACTION_LEVELS, + _coerce_unmanaged_action_for_escalate(parent.action), + child.action, + ) + + if child.directories is None: + eff_dirs = parent.directories + else: + eff_dirs = _union( + _coerce_unmanaged_directories_for_union(parent.directories), + child.directories, + ) + + eff_action = eff_action_raw if eff_action_raw is not None else "ignore" + eff_dirs_out: tuple[str, ...] = () if eff_dirs is None else eff_dirs + + return UnmanagedFilesPolicy(action=eff_action, directories=eff_dirs_out) # --------------------------------------------------------------------------- diff --git a/src/apm_cli/policy/parser.py b/src/apm_cli/policy/parser.py index 4a2f77ef8..f81dcabcb 100644 --- a/src/apm_cli/policy/parser.py +++ b/src/apm_cli/policy/parser.py @@ -208,11 +208,18 @@ def _build_policy(data: dict) -> ApmPolicy: require_explicit_includes=bool(manifest_data.get("require_explicit_includes", False)), ) - uf_data = data.get("unmanaged_files") or {} - unmanaged_files = UnmanagedFilesPolicy( - action=uf_data.get("action", UnmanagedFilesPolicy.action), - directories=_parse_tuple(uf_data.get("directories")), - ) + raw_uf = data.get("unmanaged_files") + if raw_uf is None: + unmanaged_files = UnmanagedFilesPolicy(action=None, directories=None) + else: + uf_data = {} if not isinstance(raw_uf, dict) else raw_uf + action = uf_data.get("action") + directories = ( + _parse_tuple(uf_data.get("directories")) + if "directories" in uf_data + else None + ) + unmanaged_files = UnmanagedFilesPolicy(action=action, directories=directories) return ApmPolicy( name=data.get("name", "") or "", diff --git a/src/apm_cli/policy/policy_checks.py b/src/apm_cli/policy/policy_checks.py index 899b7d1c3..c5c45efd7 100644 --- a/src/apm_cli/policy/policy_checks.py +++ b/src/apm_cli/policy/policy_checks.py @@ -688,7 +688,7 @@ def _check_unmanaged_files( policy: UnmanagedFilesPolicy, ) -> CheckResult: """Check 16: no untracked files in governance directories.""" - if policy.action == "ignore": + if policy.action in (None, "ignore"): return CheckResult( name="unmanaged-files", passed=True, diff --git a/src/apm_cli/policy/schema.py b/src/apm_cli/policy/schema.py index a1b4b4b9b..f55e38264 100644 --- a/src/apm_cli/policy/schema.py +++ b/src/apm_cli/policy/schema.py @@ -89,10 +89,20 @@ class ManifestPolicy: @dataclass(frozen=True) class UnmanagedFilesPolicy: - """Rules for files not tracked in apm.lock.""" + """Rules for files not tracked in apm.lock. - action: str = "ignore" # ignore | warn | deny - directories: tuple[str, ...] = () + ``action=None`` and ``directories=None`` together mean the policy file + expressed no ``unmanaged_files:`` section (or an empty mapping) — during + :func:`~apm_cli.policy.inheritance.merge_policies` the child is transparent + and the parent block is inherited unchanged. + + When either field is set (including ``directories=()`` with a declared + ``directories`` key), the merge applies escalation / union rules. + ``action`` is then one of ``ignore`` | ``warn`` | ``deny``. + """ + + action: str | None = None # None | ignore | warn | deny + directories: tuple[str, ...] | None = None # None -> no opinion; () explicit @dataclass(frozen=True) diff --git a/tests/unit/policy/test_fixtures.py b/tests/unit/policy/test_fixtures.py index 0fe76160d..8ed44d5f3 100644 --- a/tests/unit/policy/test_fixtures.py +++ b/tests/unit/policy/test_fixtures.py @@ -52,7 +52,8 @@ def test_minimal_policy_defaults(self): self.assertEqual(policy.name, "minimal") self.assertEqual(policy.enforcement, "warn") self.assertEqual(policy.dependencies.require_resolution, "project-wins") - self.assertEqual(policy.unmanaged_files.action, "ignore") + self.assertIsNone(policy.unmanaged_files.action) + self.assertIsNone(policy.unmanaged_files.directories) def test_repo_override_has_extends(self): """Repo override fixture declares extends=org.""" diff --git a/tests/unit/policy/test_inheritance.py b/tests/unit/policy/test_inheritance.py index 1e936c57d..a353d2225 100644 --- a/tests/unit/policy/test_inheritance.py +++ b/tests/unit/policy/test_inheritance.py @@ -424,6 +424,33 @@ def test_directories_dedup(self): ) self.assertEqual(result.unmanaged_files.directories, (".prompts",)) + def test_child_omitting_unmanaged_files_inherits_parent_issue_1198(self): + """extends: child without unmanaged_files must not downgrade org deny.""" + parent = ApmPolicy( + unmanaged_files=UnmanagedFilesPolicy( + action="deny", + directories=( + ".github/instructions", + ".github/agents", + ".github/hooks", + ), + ), + ) + child = ApmPolicy( + dependencies=DependencyPolicy(deny=("**/some-pattern",)), + unmanaged_files=UnmanagedFilesPolicy(action=None, directories=None), + ) + result = merge_policies(parent, child) + self.assertEqual(result.unmanaged_files.action, "deny") + self.assertEqual( + result.unmanaged_files.directories, + ( + ".github/instructions", + ".github/agents", + ".github/hooks", + ), + ) + class TestResolvePolicyChain(unittest.TestCase): """Full chain resolution with three levels.""" diff --git a/tests/unit/policy/test_parser.py b/tests/unit/policy/test_parser.py index 0333e35fd..04595cad3 100644 --- a/tests/unit/policy/test_parser.py +++ b/tests/unit/policy/test_parser.py @@ -218,6 +218,8 @@ def test_minimal_policy(self): self.assertIsNone(policy.dependencies.allow) self.assertEqual(policy.dependencies.max_depth, 50) self.assertFalse(policy.manifest.require_explicit_includes) + self.assertIsNone(policy.unmanaged_files.action) + self.assertIsNone(policy.unmanaged_files.directories) def test_require_explicit_includes_true(self): yaml_str = textwrap.dedent(""" diff --git a/tests/unit/policy/test_schema.py b/tests/unit/policy/test_schema.py index 1677e5210..a3b66bf35 100644 --- a/tests/unit/policy/test_schema.py +++ b/tests/unit/policy/test_schema.py @@ -96,8 +96,8 @@ class TestUnmanagedFilesPolicyDefaults(unittest.TestCase): def test_defaults(self): uf = UnmanagedFilesPolicy() - self.assertEqual(uf.action, "ignore") - self.assertEqual(uf.directories, ()) + self.assertIsNone(uf.action) + self.assertIsNone(uf.directories) class TestApmPolicyDefaults(unittest.TestCase): From 215fcb03d1e8949d7353ff0f4b603e1e92e30be4 Mon Sep 17 00:00:00 2001 From: abhinavgautam01 Date: Tue, 12 May 2026 10:02:55 +0530 Subject: [PATCH 2/2] fix(policy): address Copilot review for unmanaged_files --- CHANGELOG.md | 2 +- src/apm_cli/policy/parser.py | 8 +++++--- src/apm_cli/policy/schema.py | 2 +- tests/unit/policy/test_parser.py | 6 ++++++ 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 843bfa6c3..77bafc89c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **Policy inheritance:** a child policy that uses `extends:` but omits `unmanaged_files:` no longer silently downgrades the parent org `action: deny`; the child block is treated as fully transparent (same semantics as allow-list `None` in merge). (#1198) +- **Policy inheritance:** a child policy that uses `extends:` but omits `unmanaged_files:` no longer silently downgrades the parent org `action: deny`; the child block is treated as fully transparent (same semantics as allow-list `None` in merge). (#1248) - `apm marketplace add` accepts GitLab-class hosts (`gitlab.com` and self-managed instances configured via `GITLAB_HOST` / `APM_GITLAB_HOSTS`); unsupported generic hosts now show separate recovery hints for GHES (`GITHUB_HOST`) and self-managed GitLab instead of only `GITHUB_HOST`. (#1149) - **GitLab monorepo marketplaces:** `apm install plugin@marketplace` now resolves plugins whose sources live in a subdirectory of the marketplace repository on GitLab-class hosts (`gitlab.com` and self-managed GitLab when classified as GitLab), matching explicit `git:` + `path:` semantics without requiring that hand-written object form. (#1149) - `apm install` now rejects unsupported flat-format `dependencies` (e.g. `dependencies: [owner/repo]`) with a clear error and structured-format hint instead of silently ignoring them; the resolver also surfaces `ValueError` from malformed transitive manifests as warnings instead of swallowing them. (#1189) diff --git a/src/apm_cli/policy/parser.py b/src/apm_cli/policy/parser.py index f81dcabcb..d0423ae07 100644 --- a/src/apm_cli/policy/parser.py +++ b/src/apm_cli/policy/parser.py @@ -140,9 +140,11 @@ def validate_policy(data: dict) -> tuple[list[str], list[str]]: if rei is not None and not isinstance(rei, bool): errors.append(f"manifest.require_explicit_includes must be a boolean, got '{rei}'") - # unmanaged_files.action + # unmanaged_files uf = data.get("unmanaged_files") - if isinstance(uf, dict): + if uf is not None and not isinstance(uf, dict): + errors.append("unmanaged_files must be a YAML mapping") + elif isinstance(uf, dict): action = uf.get("action") if action is not None and action not in _VALID_UNMANAGED_ACTION: errors.append( @@ -212,7 +214,7 @@ def _build_policy(data: dict) -> ApmPolicy: if raw_uf is None: unmanaged_files = UnmanagedFilesPolicy(action=None, directories=None) else: - uf_data = {} if not isinstance(raw_uf, dict) else raw_uf + uf_data = raw_uf action = uf_data.get("action") directories = ( _parse_tuple(uf_data.get("directories")) diff --git a/src/apm_cli/policy/schema.py b/src/apm_cli/policy/schema.py index f55e38264..29d8ccc88 100644 --- a/src/apm_cli/policy/schema.py +++ b/src/apm_cli/policy/schema.py @@ -92,7 +92,7 @@ class UnmanagedFilesPolicy: """Rules for files not tracked in apm.lock. ``action=None`` and ``directories=None`` together mean the policy file - expressed no ``unmanaged_files:`` section (or an empty mapping) — during + expressed no ``unmanaged_files:`` section (or an empty mapping); during :func:`~apm_cli.policy.inheritance.merge_policies` the child is transparent and the parent block is inherited unchanged. diff --git a/tests/unit/policy/test_parser.py b/tests/unit/policy/test_parser.py index 04595cad3..9d2e98c30 100644 --- a/tests/unit/policy/test_parser.py +++ b/tests/unit/policy/test_parser.py @@ -73,6 +73,12 @@ def test_invalid_unmanaged_action(self): self.assertEqual(len(errors), 1) self.assertIn("unmanaged_files.action", errors[0]) + def test_unmanaged_files_must_be_mapping(self): + for bad in ([], ["x"], "warn", 1): + errors, warnings = validate_policy({"unmanaged_files": bad}) # noqa: RUF059 + self.assertEqual(len(errors), 1, repr(bad)) + self.assertIn("unmanaged_files must be a YAML mapping", errors[0]) + def test_negative_cache_ttl(self): errors, warnings = validate_policy({"cache": {"ttl": -1}}) # noqa: RUF059 self.assertEqual(len(errors), 1)