diff --git a/CHANGELOG.md b/CHANGELOG.md index 894797c4f..545f1165c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `Unknown target` error suggestions no longer advertise the `agent-skills` meta-target, which `apm targets` intentionally omits from its table. The canonical set still accepts `agent-skills` via `--target` and `apm.yml`, but the recovery path printed on errors now matches what the discovery command actually lists. (#1215) - `apm pack` no longer hardcodes `pack.target` into bundles; bundles are target-agnostic and `apm install ` resolves the consumer target from project context and wires bundle `.mcp.json` servers per target via `MCPIntegrator`. (#1217) - Multi-account Git Credential Manager users: APM now selects the right GitHub account automatically per repository (no account-picker prompt) when `credential.useHttpPath = true` is set. Existing single-account setups are unaffected. (#1226) +- Policy inheritance now fails closed: child policies that omit `unmanaged_files` inherit the parent's action instead of silently defaulting to `ignore`. (#1253) - Protocol-fallback port warnings are now deduplicated across parallel download workers via a threading lock, so each (host, repo, port) triple warns exactly once. (#1238) - `apm install --global ` no longer rejects absolute local paths at user scope (regression introduced by #1149); restores the post-#937 contract that only relative local paths are ambiguous at user scope. (#1247) - Realigned the failing integration suite with current product contracts: copilot-target detection requires `.github/copilot-instructions.md` (post-#1154), `apm marketplace build` is removed in favor of `apm pack`, ADO virtual collections use the SUBDIRECTORY layout (post-#1094), and the `repo:` apm.yml key is replaced by `git:`. (#1247) diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 70044081b..e88f65c57 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -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.effective_action == "ignore" ) diff --git a/src/apm_cli/policy/inheritance.py b/src/apm_cli/policy/inheritance.py index 0229883f1..d5f3024cc 100644 --- a/src/apm_cli/policy/inheritance.py +++ b/src/apm_cli/policy/inheritance.py @@ -194,8 +194,14 @@ def _merge_manifest(parent: ManifestPolicy, child: ManifestPolicy) -> ManifestPo def _merge_unmanaged_files( parent: UnmanagedFilesPolicy, child: UnmanagedFilesPolicy ) -> UnmanagedFilesPolicy: + if child.action is None: + merged_action = parent.action + elif parent.action is None: + merged_action = child.action + else: + merged_action = _escalate(_UNMANAGED_ACTION_LEVELS, parent.action, child.action) return UnmanagedFilesPolicy( - action=_escalate(_UNMANAGED_ACTION_LEVELS, parent.action, child.action), + action=merged_action, directories=_union(parent.directories, child.directories), ) diff --git a/src/apm_cli/policy/parser.py b/src/apm_cli/policy/parser.py index 4a2f77ef8..8093716c9 100644 --- a/src/apm_cli/policy/parser.py +++ b/src/apm_cli/policy/parser.py @@ -210,7 +210,7 @@ def _build_policy(data: dict) -> ApmPolicy: uf_data = data.get("unmanaged_files") or {} unmanaged_files = UnmanagedFilesPolicy( - action=uf_data.get("action", UnmanagedFilesPolicy.action), + action=uf_data.get("action"), # None when absent -> "no opinion" directories=_parse_tuple(uf_data.get("directories")), ) diff --git a/src/apm_cli/policy/policy_checks.py b/src/apm_cli/policy/policy_checks.py index 899b7d1c3..6fcbb863e 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.effective_action == "ignore": return CheckResult( name="unmanaged-files", passed=True, @@ -752,7 +752,7 @@ def _check_unmanaged_files( message="No unmanaged files in governance directories", ) - if policy.action == "warn": + if policy.effective_action == "warn": 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..4b17c9e28 100644 --- a/src/apm_cli/policy/schema.py +++ b/src/apm_cli/policy/schema.py @@ -91,9 +91,14 @@ class ManifestPolicy: class UnmanagedFilesPolicy: """Rules for files not tracked in apm.lock.""" - action: str = "ignore" # ignore | warn | deny + action: str | None = None # None = no opinion; "ignore" | "warn" | "deny" directories: tuple[str, ...] = () + @property + def effective_action(self) -> str: + """Resolved action for runtime checks (None -> 'ignore').""" + return self.action if self.action is not None else "ignore" + @dataclass(frozen=True) class ApmPolicy: diff --git a/tests/unit/policy/test_fixtures.py b/tests/unit/policy/test_fixtures.py index 0fe76160d..4818f4c9d 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.assertEqual(policy.unmanaged_files.action, None) + self.assertEqual(policy.unmanaged_files.effective_action, "ignore") 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..6872d7fd4 100644 --- a/tests/unit/policy/test_inheritance.py +++ b/tests/unit/policy/test_inheritance.py @@ -425,6 +425,73 @@ def test_directories_dedup(self): self.assertEqual(result.unmanaged_files.directories, (".prompts",)) +class TestUnmanagedFilesTransparency(unittest.TestCase): + """Child omitting unmanaged_files is transparent (fixes #1198).""" + + def test_parent_deny_child_omits_block(self): + """Parent deny + child omits -> merged deny.""" + result = merge_policies( + ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action="deny")), + ApmPolicy(), # child omits unmanaged_files entirely -> action=None + ) + self.assertEqual(result.unmanaged_files.action, "deny") + + def test_parent_deny_child_explicit_ignore(self): + """Parent deny + child explicitly sets ignore -> merged deny (escalation).""" + result = merge_policies( + ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action="deny")), + ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action="ignore")), + ) + self.assertEqual(result.unmanaged_files.action, "deny") + + def test_parent_warn_child_explicit_deny(self): + """Parent warn + child explicitly sets deny -> merged deny (child tightens).""" + result = merge_policies( + ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action="warn")), + ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action="deny")), + ) + self.assertEqual(result.unmanaged_files.action, "deny") + + def test_parent_ignore_child_omits(self): + """Parent ignore + child omits -> merged ignore.""" + result = merge_policies( + ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action="ignore")), + ApmPolicy(), + ) + self.assertEqual(result.unmanaged_files.action, "ignore") + + def test_parent_none_child_omits(self): + """Both omit -> merged None (no opinion).""" + result = merge_policies( + ApmPolicy(), + ApmPolicy(), + ) + self.assertIsNone(result.unmanaged_files.action) + self.assertEqual(result.unmanaged_files.effective_action, "ignore") + + def test_directories_inherited_when_child_omits(self): + """Parent directories preserved when child omits the block.""" + result = merge_policies( + ApmPolicy( + unmanaged_files=UnmanagedFilesPolicy(action="deny", directories=(".github", "docs")) + ), + ApmPolicy(), # child omits + ) + self.assertEqual(result.unmanaged_files.action, "deny") + self.assertEqual(sorted(result.unmanaged_files.directories), [".github", "docs"]) + + def test_three_level_chain_transparency(self): + """Enterprise deny -> org omits -> repo omits -> deny preserved.""" + result = resolve_policy_chain( + [ + ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action="deny")), + ApmPolicy(), # org omits + ApmPolicy(), # repo omits + ] + ) + self.assertEqual(result.unmanaged_files.action, "deny") + + 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..8fe9e8399 100644 --- a/tests/unit/policy/test_parser.py +++ b/tests/unit/policy/test_parser.py @@ -318,6 +318,13 @@ def test_long_yaml_string_does_not_crash(self): self.assertEqual(policy.version, "1.0") self.assertEqual(policy.enforcement, "off") + def test_omitted_unmanaged_files_yields_none_action(self): + """Absent unmanaged_files block -> action is None (no opinion).""" + policy, _ = load_policy("name: test\nenforcement: warn\n") + self.assertIsNone(policy.unmanaged_files.action) + self.assertEqual(policy.unmanaged_files.effective_action, "ignore") + self.assertEqual(policy.unmanaged_files.directories, ()) + class TestLoadPolicyFromFile(unittest.TestCase): """Test load_policy from a file path.""" diff --git a/tests/unit/policy/test_policy_checks.py b/tests/unit/policy/test_policy_checks.py index eef7ff12c..08396403b 100644 --- a/tests/unit/policy/test_policy_checks.py +++ b/tests/unit/policy/test_policy_checks.py @@ -9,6 +9,7 @@ import yaml from apm_cli.models.apm_package import clear_apm_yml_cache +from apm_cli.policy.inheritance import merge_policies from apm_cli.policy.models import CheckResult, CIAuditResult # noqa: F401 from apm_cli.policy.policy_checks import ( _check_compilation_strategy, @@ -653,6 +654,53 @@ def test_rglob_cap_skips_check(self, tmp_path, monkeypatch): assert result.passed assert "capped" in result.message.lower() + def test_action_none_resolves_to_ignore(self, tmp_path): + """action=None standalone: effective_action resolves to 'ignore', check passes.""" + policy = UnmanagedFilesPolicy(action=None) + result = _check_unmanaged_files(tmp_path, None, policy) + assert result.passed + + def test_action_none_inherits_parent_deny(self, tmp_path): + """action=None + parent deny: merge propagates deny to child.""" + (tmp_path / ".github" / "agents").mkdir(parents=True) + (tmp_path / ".github" / "agents" / "rogue.md").write_text("rogue", encoding="utf-8") + parent = ApmPolicy( + unmanaged_files=UnmanagedFilesPolicy(action="deny", directories=[".github/agents"]) + ) + child = ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action=None)) + merged = merge_policies(parent, child) + lock = _make_lockfile([{"repo_url": "org/pkg", "deployed_files": []}]) + result = _check_unmanaged_files(tmp_path, lock, merged.unmanaged_files) + assert not result.passed + + def test_rogue_file_caught_parent_deny_child_omits_block(self, tmp_path): + """Rogue file is caught when parent says deny but child omits unmanaged_files block.""" + (tmp_path / ".github" / "agents").mkdir(parents=True) + (tmp_path / ".github" / "agents" / "rogue.md").write_text("rogue", encoding="utf-8") + parent = ApmPolicy( + unmanaged_files=UnmanagedFilesPolicy(action="deny", directories=[".github/agents"]) + ) + child = ApmPolicy() # child omits unmanaged_files entirely (defaults to action=None) + merged = merge_policies(parent, child) + lock = _make_lockfile([{"repo_url": "org/pkg", "deployed_files": []}]) + result = _check_unmanaged_files(tmp_path, lock, merged.unmanaged_files) + assert not result.passed + assert ".github/agents/rogue.md" in result.details + + def test_integration_chain_deny_propagates(self, tmp_path): + """Integration chain: parent deny + child omits block -> merge -> check catches rogue file.""" + (tmp_path / ".github" / "agents").mkdir(parents=True) + (tmp_path / ".github" / "agents" / "rogue.md").write_text("rogue", encoding="utf-8") + parent_policy = ApmPolicy( + unmanaged_files=UnmanagedFilesPolicy(action="deny", directories=[".github/agents"]) + ) + child_policy = ApmPolicy(unmanaged_files=UnmanagedFilesPolicy(action=None)) + merged = merge_policies(parent_policy, child_policy) + lock = _make_lockfile([{"repo_url": "org/pkg", "deployed_files": []}]) + result = _check_unmanaged_files(tmp_path, lock, merged.unmanaged_files) + assert not result.passed + assert ".github/agents/rogue.md" in result.details + # -- Integration: run_policy_checks --------------------------------- diff --git a/tests/unit/policy/test_schema.py b/tests/unit/policy/test_schema.py index 1677e5210..6eea415aa 100644 --- a/tests/unit/policy/test_schema.py +++ b/tests/unit/policy/test_schema.py @@ -96,7 +96,8 @@ class TestUnmanagedFilesPolicyDefaults(unittest.TestCase): def test_defaults(self): uf = UnmanagedFilesPolicy() - self.assertEqual(uf.action, "ignore") + self.assertIsNone(uf.action) + self.assertEqual(uf.effective_action, "ignore") self.assertEqual(uf.directories, ())