diff --git a/CHANGELOG.md b/CHANGELOG.md index 6672b8593..8d8f5f6cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,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 `unmanaged_files`:** Child policies that omit the block (or use an empty mapping) under `extends:` inherit the parent org settings transparently, fixing silent downgrades such as losing `action: deny`; values that are not YAML mappings (lists or scalars) are rejected with a recovery hint. (#1198, #1248) ## [0.12.4] - 2026-05-07 diff --git a/src/apm_cli/commands/policy.py b/src/apm_cli/commands/policy.py index e3edc8bbb..6cf90ff21 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 b2ccd6e10..97815df2c 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 cb2b67b40..f732b1d73 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 ab7bc3169..f58d1abc2 100644 --- a/src/apm_cli/policy/parser.py +++ b/src/apm_cli/policy/parser.py @@ -140,9 +140,18 @@ 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 " + f"(got {type(uf).__name__} {uf!r}); use a block, for example:\n" + " unmanaged_files:\n" + " action: deny\n" + " directories:\n" + " - .github/instructions" + ) + elif isinstance(uf, dict): action = uf.get("action") if action is not None and action not in _VALID_UNMANAGED_ACTION: errors.append( @@ -214,11 +223,14 @@ 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 07315efcb..3ff3cb7d0 100644 --- a/src/apm_cli/policy/schema.py +++ b/src/apm_cli/policy/schema.py @@ -101,10 +101,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/integration/conftest.py b/tests/integration/conftest.py index ef23a849d..ae85db3b3 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -25,6 +25,29 @@ import pytest +_REPO_ROOT = Path(__file__).resolve().parent.parent.parent + + +@pytest.fixture(autouse=True) +def _integration_process_cwd_guard(): + """Keep process cwd valid across integration tests on POSIX workers. + + If a test changes the working directory into a directory that is later + deleted without leaving first, the kernel leaves the process parked on a + detached inode and :func:`os.getcwd` raises :exc:`FileNotFoundError`. + That poisons the rest of an xdist worker (merge-queue CI runs + ``-n 2 --dist loadgroup``). + """ + try: + os.getcwd() + except (FileNotFoundError, OSError): + os.chdir(_REPO_ROOT) + yield + try: + os.getcwd() + except (FileNotFoundError, OSError): + os.chdir(_REPO_ROOT) + def make_copilot_project(tmp_path: Path, name: str = "test-project") -> Path: """Create a temp project with a valid copilot signal. diff --git a/tests/integration/test_registry.py b/tests/integration/test_registry.py index dcf765e82..abab11426 100644 --- a/tests/integration/test_registry.py +++ b/tests/integration/test_registry.py @@ -59,6 +59,14 @@ def teardown_method(self): if sys.platform == "win32": time.sleep(0.1) + # Leave the temp tree before unlinking it. Otherwise cwd can still + # reference the directory inode and os.getcwd() raises FileNotFoundError + # on POSIX — breaking later tests on the same xdist worker. + try: + os.chdir(tempfile.gettempdir()) + except (FileNotFoundError, OSError): + pass + # First, try the standard cleanup try: self.test_dir.cleanup() 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 4e3602cb3..df017fb5f 100644 --- a/tests/unit/policy/test_inheritance.py +++ b/tests/unit/policy/test_inheritance.py @@ -490,6 +490,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 3b4bbffb4..e6b8ff6fd 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(""" @@ -324,7 +332,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) def test_absent_dependencies_block_gives_none_deny_and_require(self): """Entirely absent dependencies: block -> deny=None, require=None (Fix 2).""" diff --git a/tests/unit/policy/test_schema.py b/tests/unit/policy/test_schema.py index d26818185..cab6bfdf9 100644 --- a/tests/unit/policy/test_schema.py +++ b/tests/unit/policy/test_schema.py @@ -99,8 +99,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):