From 93a2d299dd432057c21b49fb9f16f545d7b608e1 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Mon, 3 Aug 2026 21:36:34 +0200 Subject: [PATCH 1/2] fix(presets): return None for an unreadable layer in resolve_content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PresetResolver.resolve_content() reads the winning layer (and each composition layer) with a bare read_text(), so a layer file that cannot be read or decoded crashed command registration with a raw OSError/UnicodeDecodeError. The docstring already promises 'Composed content string, or None if not found', and since #3896 collect_all_layers() deliberately tolerates a non-UTF-8 legacy layer — moving the crash here, where both callers (_register_commands and _reconcile_composed_commands) are unguarded. Return None when the winning or base layer cannot be read, treating an unreadable layer like a missing one per the documented contract. Assisted-by: GitHub Copilot (model: claude-fable-5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/presets/__init__.py | 23 ++++++++++++++++++++--- tests/test_presets.py | 25 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index cc5308f3fc..772895a918 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -5540,7 +5540,7 @@ def resolve_content( if not layers: return None - def _read_layer_content(layer: Dict[str, Any]) -> str: + def _read_layer_content(layer: Dict[str, Any]) -> Optional[str]: """Read a layer's raw text, rewriting extension-relative subdir references (agents/, knowledge-base/, etc.) to their installed location when the layer is extension-provided (#2101). @@ -5550,8 +5550,18 @@ def _read_layer_content(layer: Dict[str, Any]) -> str: rewrite when it wins outright above or serves as the composition base below — never as a mid-stack composing (append/prepend/wrap) layer. + + Returns None when the layer cannot be read or decoded: + collect_all_layers deliberately keeps a non-UTF-8 legacy layer + (with its "replace" default) so unrelated commands still + resolve, so the same tolerance must apply here — the documented + contract is "Composed content string, or None if not found", + not a raw UnicodeDecodeError at composition time. """ - text = layer["path"].read_text(encoding="utf-8") + try: + text = layer["path"].read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return None extension_id = layer.get("extension_id") extension_dir = layer.get("extension_dir") if extension_id and extension_dir: @@ -5589,6 +5599,8 @@ def _read_layer_content(layer: Dict[str, Any]) -> str: # Convert to reversed_layers index base_reversed_idx = len(layers) - 1 - base_layer_idx content = _read_layer_content(layers[base_layer_idx]) + if content is None: + return None # Compose only the layers above the base (higher priority = lower index in layers, # higher index in reversed_layers). Process bottom-up from base+1. start_idx = base_reversed_idx + 1 @@ -5632,7 +5644,12 @@ def _split_frontmatter(text: str) -> tuple: # Apply composition layers from bottom to top for layer in reversed_layers[start_idx:]: - layer_content = layer["path"].read_text(encoding="utf-8") + try: + layer_content = layer["path"].read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + # Same tolerance as _read_layer_content: an unreadable layer + # means the composed result cannot be produced. + return None strategy = layer["strategy"] if is_command: diff --git a/tests/test_presets.py b/tests/test_presets.py index 243d13ab55..e7961b085e 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -11353,6 +11353,31 @@ def test_resolve_content_nonexistent(self, project_dir): content = resolver.resolve_content("nonexistent") assert content is None + def test_resolve_content_unreadable_winning_layer_returns_none(self, project_dir): + """An undecodable winning layer must yield None, not a raw traceback. + + ``collect_all_layers`` deliberately keeps a non-UTF-8 legacy command + layer (with its ``replace`` default) so unrelated commands still + resolve. ``resolve_content`` then read that same file without a + boundary, so the tolerated layer crashed with ``UnicodeDecodeError`` + at composition time — reachable from ``specify preset add`` via + ``_register_commands``. The documented contract is "Composed content + string, or None if not found". + """ + presets_dir = project_dir / ".specify" / "presets" + command_path = ( + presets_dir / "legacy-pack" / "commands" / "speckit.legacy.md" + ) + command_path.parent.mkdir(parents=True) + command_path.write_bytes(b"\xff\xfe") + PresetRegistry(presets_dir).add( + "legacy-pack", {"version": "1.0.0", "priority": 10} + ) + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("speckit.legacy", "command") + assert content is None + def test_resolve_content_replace_strategy(self, project_dir, temp_dir, valid_pack_data): """Test resolve_content with default replace strategy.""" manager = PresetManager(project_dir) From c217418da28d722958bbf1c9637b48366ad53608 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:13:20 +0200 Subject: [PATCH 2/2] test: cover the base guard and composing-layer read Review follow-up: add an unreadable replace base beneath a valid composing layer, and a mocked-PermissionError composing layer over a valid base, so every new boundary and both exception types are covered. Assisted-by: GitHub Copilot (model: claude-fable-5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_presets.py | 82 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/tests/test_presets.py b/tests/test_presets.py index e7961b085e..6d55b90c50 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -11378,6 +11378,88 @@ def test_resolve_content_unreadable_winning_layer_returns_none(self, project_dir content = resolver.resolve_content("speckit.legacy", "command") assert content is None + def test_resolve_content_unreadable_base_under_composing_layer( + self, project_dir, temp_dir, valid_pack_data + ): + """An undecodable base beneath a valid composing layer yields None. + + Covers the base-read guard: the winning layer composes (append), so + resolution reads the base layer beneath it — here the core template, + corrupted to non-UTF-8 — and must return None instead of crashing. + """ + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "append-pack", "name": "Append"} + pack_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "append", + }] + } + pack_dir = temp_dir / "append-pack" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text("## Appended Section\n") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + core_spec = project_dir / ".specify" / "templates" / "spec-template.md" + core_spec.write_bytes(b"\xff\xfe") + + resolver = PresetResolver(project_dir) + assert resolver.resolve_content("spec-template") is None + + def test_resolve_content_unreadable_composing_layer( + self, project_dir, temp_dir, valid_pack_data, monkeypatch + ): + """An unreadable composing layer over a valid base yields None. + + Covers the composition-loop read and the ``OSError`` half of the + boundary: the base (core template) reads fine, but the append layer + raises a mocked ``PermissionError`` — mocked so the case also holds + under privileged CI where permission bits are not enforced. + """ + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "append-pack", "name": "Append"} + pack_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "append", + }] + } + pack_dir = temp_dir / "append-pack" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text("## Appended Section\n") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + layer_path = ( + project_dir / ".specify" / "presets" / "append-pack" + / "templates" / "spec-template.md" + ) + assert layer_path.is_file() + original_read_text = Path.read_text + + def failing_read_text(self_path, *args, **kwargs): + if self_path == layer_path: + raise PermissionError(13, "Permission denied") + return original_read_text(self_path, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", failing_read_text) + + resolver = PresetResolver(project_dir) + assert resolver.resolve_content("spec-template") is None + def test_resolve_content_replace_strategy(self, project_dir, temp_dir, valid_pack_data): """Test resolve_content with default replace strategy.""" manager = PresetManager(project_dir)