diff --git a/CHANGELOG.md b/CHANGELOG.md index f16280684..586379303 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `apm marketplace add` accepts GitLab-class hosts; unsupported generic hosts now show separate recovery hints for GHES (`GITHUB_HOST`) and self-managed GitLab instead of only `GITHUB_HOST`. (#1149) - Realigned the 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:`. (#1257, #1261, #1264) - `triage-panel` scheduled sweep now paginates oldest-first via GitHub MCP `search_issues` with `-label:status/triaged sort:created-asc`, so daily runs drain the untriaged backlog instead of processing one issue per cron tick. (#1193, #1194) +- Policy validation rejects `unmanaged_files` values that are not YAML mappings (lists or scalars) instead of treating them as an empty block. (#1248) ## [0.12.4] - 2026-05-07 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 e88f65c57..4acc1b1ae 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 ()), }, } diff --git a/src/apm_cli/policy/inheritance.py b/src/apm_cli/policy/inheritance.py index d5f3024cc..0b6fd88bd 100644 --- a/src/apm_cli/policy/inheritance.py +++ b/src/apm_cli/policy/inheritance.py @@ -191,19 +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: + """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: - merged_action = parent.action - elif parent.action is None: - merged_action = child.action + eff_action_raw = parent.action else: - merged_action = _escalate(_UNMANAGED_ACTION_LEVELS, parent.action, child.action) - return UnmanagedFilesPolicy( - action=merged_action, - directories=_union(parent.directories, child.directories), - ) + 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 8093716c9..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( @@ -208,11 +210,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"), # None when absent -> "no opinion" - 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 = 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/schema.py b/src/apm_cli/policy/schema.py index 4b17c9e28..37ef61ecd 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 | None = None # None = no opinion; "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 @property def effective_action(self) -> str: diff --git a/tests/unit/policy/test_fixtures.py b/tests/unit/policy/test_fixtures.py index 4818f4c9d..67c70d8f9 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, None) + self.assertIsNone(policy.unmanaged_files.action) + self.assertIsNone(policy.unmanaged_files.directories) self.assertEqual(policy.unmanaged_files.effective_action, "ignore") def test_repo_override_has_extends(self): diff --git a/tests/unit/policy/test_inheritance.py b/tests/unit/policy/test_inheritance.py index 6872d7fd4..8bac31730 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 TestUnmanagedFilesTransparency(unittest.TestCase): """Child omitting unmanaged_files is transparent (fixes #1198).""" diff --git a/tests/unit/policy/test_parser.py b/tests/unit/policy/test_parser.py index 8fe9e8399..5b9a7dbab 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) @@ -218,6 +224,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(""" @@ -323,7 +331,7 @@ def test_omitted_unmanaged_files_yields_none_action(self): 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, ()) + self.assertIsNone(policy.unmanaged_files.directories) class TestLoadPolicyFromFile(unittest.TestCase): diff --git a/tests/unit/policy/test_schema.py b/tests/unit/policy/test_schema.py index 6eea415aa..e204e9af9 100644 --- a/tests/unit/policy/test_schema.py +++ b/tests/unit/policy/test_schema.py @@ -97,8 +97,8 @@ class TestUnmanagedFilesPolicyDefaults(unittest.TestCase): def test_defaults(self): uf = UnmanagedFilesPolicy() self.assertIsNone(uf.action) + self.assertIsNone(uf.directories) self.assertEqual(uf.effective_action, "ignore") - self.assertEqual(uf.directories, ()) class TestApmPolicyDefaults(unittest.TestCase):