Skip to content

Commit fe3732e

Browse files
marcelsafinCopilot
marcelsafin
and
Copilot
authored
fix(presets): return None for an unreadable layer in resolve_content (#3959)
* fix(presets): return None for an unreadable layer in resolve_content 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> * 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 3d4f71c commit fe3732e

2 files changed

Lines changed: 127 additions & 3 deletions

File tree

‎src/specify_cli/presets/__init__.py‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5576,7 +5576,7 @@ def resolve_content(
55765576
if not layers:
55775577
return None
55785578

5579-
def _read_layer_content(layer: Dict[str, Any]) -> str:
5579+
def _read_layer_content(layer: Dict[str, Any]) -> Optional[str]:
55805580
"""Read a layer's raw text, rewriting extension-relative subdir
55815581
references (agents/, knowledge-base/, etc.) to their installed
55825582
location when the layer is extension-provided (#2101).
@@ -5586,8 +5586,18 @@ def _read_layer_content(layer: Dict[str, Any]) -> str:
55865586
rewrite when it wins outright above or serves as the
55875587
composition base below — never as a mid-stack composing
55885588
(append/prepend/wrap) layer.
5589+
5590+
Returns None when the layer cannot be read or decoded:
5591+
collect_all_layers deliberately keeps a non-UTF-8 legacy layer
5592+
(with its "replace" default) so unrelated commands still
5593+
resolve, so the same tolerance must apply here — the documented
5594+
contract is "Composed content string, or None if not found",
5595+
not a raw UnicodeDecodeError at composition time.
55895596
"""
5590-
text = layer["path"].read_text(encoding="utf-8")
5597+
try:
5598+
text = layer["path"].read_text(encoding="utf-8")
5599+
except (OSError, UnicodeDecodeError):
5600+
return None
55915601
extension_id = layer.get("extension_id")
55925602
extension_dir = layer.get("extension_dir")
55935603
if extension_id and extension_dir:
@@ -5625,6 +5635,8 @@ def _read_layer_content(layer: Dict[str, Any]) -> str:
56255635
# Convert to reversed_layers index
56265636
base_reversed_idx = len(layers) - 1 - base_layer_idx
56275637
content = _read_layer_content(layers[base_layer_idx])
5638+
if content is None:
5639+
return None
56285640
# Compose only the layers above the base (higher priority = lower index in layers,
56295641
# higher index in reversed_layers). Process bottom-up from base+1.
56305642
start_idx = base_reversed_idx + 1
@@ -5668,7 +5680,12 @@ def _split_frontmatter(text: str) -> tuple:
56685680

56695681
# Apply composition layers from bottom to top
56705682
for layer in reversed_layers[start_idx:]:
5671-
layer_content = layer["path"].read_text(encoding="utf-8")
5683+
try:
5684+
layer_content = layer["path"].read_text(encoding="utf-8")
5685+
except (OSError, UnicodeDecodeError):
5686+
# Same tolerance as _read_layer_content: an unreadable layer
5687+
# means the composed result cannot be produced.
5688+
return None
56725689
strategy = layer["strategy"]
56735690

56745691
if is_command:

‎tests/test_presets.py‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11425,6 +11425,113 @@ def test_resolve_content_nonexistent(self, project_dir):
1142511425
content = resolver.resolve_content("nonexistent")
1142611426
assert content is None
1142711427

11428+
def test_resolve_content_unreadable_winning_layer_returns_none(self, project_dir):
11429+
"""An undecodable winning layer must yield None, not a raw traceback.
11430+
11431+
``collect_all_layers`` deliberately keeps a non-UTF-8 legacy command
11432+
layer (with its ``replace`` default) so unrelated commands still
11433+
resolve. ``resolve_content`` then read that same file without a
11434+
boundary, so the tolerated layer crashed with ``UnicodeDecodeError``
11435+
at composition time — reachable from ``specify preset add`` via
11436+
``_register_commands``. The documented contract is "Composed content
11437+
string, or None if not found".
11438+
"""
11439+
presets_dir = project_dir / ".specify" / "presets"
11440+
command_path = (
11441+
presets_dir / "legacy-pack" / "commands" / "speckit.legacy.md"
11442+
)
11443+
command_path.parent.mkdir(parents=True)
11444+
command_path.write_bytes(b"\xff\xfe")
11445+
PresetRegistry(presets_dir).add(
11446+
"legacy-pack", {"version": "1.0.0", "priority": 10}
11447+
)
11448+
11449+
resolver = PresetResolver(project_dir)
11450+
content = resolver.resolve_content("speckit.legacy", "command")
11451+
assert content is None
11452+
11453+
def test_resolve_content_unreadable_base_under_composing_layer(
11454+
self, project_dir, temp_dir, valid_pack_data
11455+
):
11456+
"""An undecodable base beneath a valid composing layer yields None.
11457+
11458+
Covers the base-read guard: the winning layer composes (append), so
11459+
resolution reads the base layer beneath it — here the core template,
11460+
corrupted to non-UTF-8 — and must return None instead of crashing.
11461+
"""
11462+
pack_data = {**valid_pack_data}
11463+
pack_data["preset"] = {**valid_pack_data["preset"], "id": "append-pack", "name": "Append"}
11464+
pack_data["provides"] = {
11465+
"templates": [{
11466+
"type": "template",
11467+
"name": "spec-template",
11468+
"file": "templates/spec-template.md",
11469+
"strategy": "append",
11470+
}]
11471+
}
11472+
pack_dir = temp_dir / "append-pack"
11473+
pack_dir.mkdir()
11474+
with open(pack_dir / "preset.yml", 'w') as f:
11475+
yaml.dump(pack_data, f)
11476+
(pack_dir / "templates").mkdir()
11477+
(pack_dir / "templates" / "spec-template.md").write_text("## Appended Section\n")
11478+
11479+
manager = PresetManager(project_dir)
11480+
manager.install_from_directory(pack_dir, "0.1.5")
11481+
11482+
core_spec = project_dir / ".specify" / "templates" / "spec-template.md"
11483+
core_spec.write_bytes(b"\xff\xfe")
11484+
11485+
resolver = PresetResolver(project_dir)
11486+
assert resolver.resolve_content("spec-template") is None
11487+
11488+
def test_resolve_content_unreadable_composing_layer(
11489+
self, project_dir, temp_dir, valid_pack_data, monkeypatch
11490+
):
11491+
"""An unreadable composing layer over a valid base yields None.
11492+
11493+
Covers the composition-loop read and the ``OSError`` half of the
11494+
boundary: the base (core template) reads fine, but the append layer
11495+
raises a mocked ``PermissionError`` — mocked so the case also holds
11496+
under privileged CI where permission bits are not enforced.
11497+
"""
11498+
pack_data = {**valid_pack_data}
11499+
pack_data["preset"] = {**valid_pack_data["preset"], "id": "append-pack", "name": "Append"}
11500+
pack_data["provides"] = {
11501+
"templates": [{
11502+
"type": "template",
11503+
"name": "spec-template",
11504+
"file": "templates/spec-template.md",
11505+
"strategy": "append",
11506+
}]
11507+
}
11508+
pack_dir = temp_dir / "append-pack"
11509+
pack_dir.mkdir()
11510+
with open(pack_dir / "preset.yml", 'w') as f:
11511+
yaml.dump(pack_data, f)
11512+
(pack_dir / "templates").mkdir()
11513+
(pack_dir / "templates" / "spec-template.md").write_text("## Appended Section\n")
11514+
11515+
manager = PresetManager(project_dir)
11516+
manager.install_from_directory(pack_dir, "0.1.5")
11517+
11518+
layer_path = (
11519+
project_dir / ".specify" / "presets" / "append-pack"
11520+
/ "templates" / "spec-template.md"
11521+
)
11522+
assert layer_path.is_file()
11523+
original_read_text = Path.read_text
11524+
11525+
def failing_read_text(self_path, *args, **kwargs):
11526+
if self_path == layer_path:
11527+
raise PermissionError(13, "Permission denied")
11528+
return original_read_text(self_path, *args, **kwargs)
11529+
11530+
monkeypatch.setattr(Path, "read_text", failing_read_text)
11531+
11532+
resolver = PresetResolver(project_dir)
11533+
assert resolver.resolve_content("spec-template") is None
11534+
1142811535
def test_resolve_content_replace_strategy(self, project_dir, temp_dir, valid_pack_data):
1142911536
"""Test resolve_content with default replace strategy."""
1143011537
manager = PresetManager(project_dir)

0 commit comments

Comments
 (0)