Skip to content

Commit a83d4cf

Browse files
kartsan03kartsan03
authored
fix(extensions): reject hook commands that are not strings (#5146)
* fix(extensions): reject hook commands that are not strings `ExtensionManifest` only checked that a hook's `command` was truthy, so `command: [speckit.x.hello]` or a mapping passed validation. `extension add` then crashed in `HookExecutor.register_hooks` with `TypeError: unhashable type: 'list'`, after the extension directory and the agent skill had already been written. Apply the rule extension events already use (#17): a hook command must be a non-blank string, so the manifest is rejected with a `ValidationError` before anything is installed. Refs #5080 Assisted-by: Codex CLI (model: gpt-6.1-sol, autonomous) Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) * fix(extensions): point the hook command comment at validate_events() `#17` read as a GitHub issue reference, but that issue is about Gemini CLI. Refs #5080 Assisted-by: Claude Code (model: claude-opus-5-5, autonomous)
1 parent 246b9d0 commit a83d4cf

3 files changed

Lines changed: 54 additions & 2 deletions

File tree

‎src/specify_cli/extensions/__init__.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -432,9 +432,13 @@ def _validate(self):
432432
f"Invalid hook '{hook_name}': "
433433
"expected a mapping or list of mappings"
434434
)
435-
if not entry.get("command"):
435+
command = entry.get("command")
436+
# #5080: same rule as validate_events() in events/__init__.py.
437+
# A truthy non-string (e.g. command: [foo]) passed a bare
438+
# truthiness check and crashed hook registration mid-install.
439+
if not isinstance(command, str) or not command.strip():
436440
raise ValidationError(
437-
f"Hook '{hook_name}' missing required 'command' field"
441+
f"Hook '{hook_name}' missing required 'command' string"
438442
)
439443
if "priority" in entry:
440444
priority = entry["priority"]

‎tests/specify_cli/extensions/test_command_add.py‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,34 @@
4040
class TestExtensionAddCLI:
4141
"""CLI tests for ``specify extension add``."""
4242

43+
@pytest.mark.parametrize("command", [
44+
pytest.param(["speckit.test-ext.hello"], id="list"),
45+
pytest.param({"name": "speckit.test-ext.hello"}, id="mapping"),
46+
])
47+
def test_add_dev_rejects_invalid_hook_command_before_writing_files(
48+
self, extension_dir, project_dir, command
49+
):
50+
"""Malformed hook commands must leave no installed artifacts (#5080)."""
51+
manifest_path = extension_dir / "extension.yml"
52+
data = yaml.safe_load(manifest_path.read_text(encoding="utf-8"))
53+
data["hooks"]["after_tasks"]["command"] = command
54+
manifest_path.write_text(yaml.safe_dump(data), encoding="utf-8")
55+
agent_dir = project_dir / ".claude"
56+
(agent_dir / "skills").mkdir(parents=True)
57+
before = set(agent_dir.rglob("*"))
58+
59+
with patch.object(Path, "cwd", return_value=project_dir):
60+
result = CliRunner().invoke(app, ["extension", "add", "--dev", str(extension_dir)])
61+
62+
assert result.exit_code == 1, result.output
63+
assert isinstance(result.exception, SystemExit), repr(result.exception)
64+
output = strip_ansi(result.output)
65+
assert "after_tasks" in output and "'command'" in output, output
66+
assert "Traceback" not in output
67+
assert not (project_dir / ".specify/extensions/test-ext").exists()
68+
assert set(agent_dir.rglob("*")) == before
69+
assert ExtensionRegistry(project_dir / ".specify/extensions").get("test-ext") is None
70+
4371
def test_add_dev_links_copilot_agent_when_supported(
4472
self, extension_dir, project_dir, temp_dir
4573
):

‎tests/test_extensions.py‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -878,6 +878,26 @@ def test_non_dict_hook_entry_raises_validation_error(self, temp_dir, valid_manif
878878
with pytest.raises(ValidationError, match="Invalid hook 'after_tasks'"):
879879
ExtensionManifest(manifest_path)
880880

881+
@pytest.mark.parametrize("as_list", [False, True])
882+
@pytest.mark.parametrize("command", [
883+
pytest.param(["speckit.test-ext.hello"], id="list"),
884+
pytest.param({"name": "speckit.test-ext.hello"}, id="mapping"),
885+
pytest.param(123, id="integer"),
886+
pytest.param(True, id="boolean"),
887+
pytest.param(" ", id="blank"),
888+
])
889+
def test_hook_command_must_be_a_string(
890+
self, temp_dir, valid_manifest_data, command, as_list
891+
):
892+
"""Reject non-string commands before hook registration can fail (#5080)."""
893+
entry = {"command": command}
894+
valid_manifest_data["hooks"]["after_tasks"] = [entry] if as_list else entry
895+
manifest_path = temp_dir / "extension.yml"
896+
manifest_path.write_text(yaml.safe_dump(valid_manifest_data), encoding="utf-8")
897+
898+
with pytest.raises(ValidationError, match="Hook 'after_tasks' missing required 'command' string"):
899+
ExtensionManifest(manifest_path)
900+
881901
def test_hook_single_mapping_still_accepted(self, extension_dir):
882902
"""Existing single-mapping hook manifests parse unchanged (regression)."""
883903
manifest_path = extension_dir / "extension.yml"

0 commit comments

Comments
 (0)