fix(templates): report an unreadable extensions.yml instead of skipping hooks silently - #4456
fix(templates): report an unreadable extensions.yml instead of skipping hooks silently#4456avp9-nexus wants to merge 2 commits into
Conversation
…ng hooks silently All ten core command templates told the agent: "If the YAML cannot be parsed or is invalid, skip hook checking silently and continue normally". A `.specify/extensions.yml` with one malformed line was therefore treated exactly like a manifest with no hooks, and a mandatory hook (`optional: false`, as registered by the bundled git extension) was disabled without anything saying so. The agent now tells the user that the manifest could not be read (with the parser error) and that no hooks were checked, then continues as before. Control flow is unchanged; only the silence is removed. Adds tests/test_command_template_hooks.py, which pins the wording at both hook sites of every template that reads extensions.yml.
There was a problem hiding this comment.
🟡 Changes recommended
The regression tests must cover the parser error, mandatory-hook warning, and continuation behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates core command templates to report unreadable .specify/extensions.yml files rather than silently skipping hooks.
Changes:
- Adds explicit parse-error and skipped-hook warnings across ten templates.
- Adds prompt-text regression tests for both hook sites.
File summaries
| File | Description |
|---|---|
tests/test_command_template_hooks.py |
Adds regression checks, but does not cover all required warning and continuation clauses. |
templates/commands/taskstoissues.md |
Reports unreadable hook manifests. |
templates/commands/tasks.md |
Reports unreadable hook manifests. |
templates/commands/specify.md |
Reports unreadable hook manifests. |
templates/commands/plan.md |
Reports unreadable hook manifests. |
templates/commands/implement.md |
Reports unreadable hook manifests. |
templates/commands/converge.md |
Reports unreadable hook manifests. |
templates/commands/constitution.md |
Reports unreadable hook manifests. |
templates/commands/clarify.md |
Reports unreadable hook manifests. |
templates/commands/checklist.md |
Reports unreadable hook manifests. |
templates/commands/analyze.md |
Reports unreadable hook manifests. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| r"^.*If the YAML cannot be parsed or is invalid.*$", re.MULTILINE | ||
| ) | ||
| _SILENT = "skip hook checking silently" | ||
| _REPORTED = ("could not be read", "no hooks were checked") |
There was a problem hiding this comment.
Agreed, and thanks for the concrete list. The test now pins all five clauses (manifest could not be read, parser error included, no hooks were checked, mandatory optional: false hooks named, then continue). I checked it by mutation rather than by reading: removing any one clause from all ten templates fails the ten parametrized cases, restoring them brings the file back to 21 passed. I kept my own wording rather than the suggested changeset because the phrases are a little stricter (they pin the full include the parser error and including any mandatory (optional: false) hooks clauses), but the coverage is the same. As disclosed in the description, this reply was drafted with an AI assistant and reviewed before posting.
The regression test only pinned two of the five clauses the new instruction carries. Dropping the parser error, the mandatory-hook warning or the continuation clause from every template would have left it green. It now pins all five: the manifest could not be read, the parser error is included, no hooks were checked, mandatory (optional: false) hooks are named, and the command then continues. Checked by mutation: removing any one clause from all ten templates fails the ten parametrized cases.
Description
All ten core command templates (
templates/commands/*.md) check.specify/extensions.ymlforhooks.before_*/hooks.after_*entries, and all of them contain this instruction at both hook sites:So a manifest with a single malformed line is treated exactly like a manifest with no hooks. A mandatory hook (
optional: false) is skipped, and nothing tells the user. The bundledgitextension registers two of those (before_constitution,before_specify), so this is reachable from a stock install: corrupt one line ofextensions.ymland/speckit.specifyruns without creating the feature branch, silently."No hooks registered" and "manifest unreadable" should not produce the same silence. #2901 made sure the agent actually runs a mandatory hook instead of only emitting the directive; this PR closes the other way the same hook can vanish.
What changes
.specify/extensions.ymlcould not be read (include the parser error) and that no hooks were checked, including any mandatory (optional: false) hooks registered there, then continue normally (or then continue to the Completion Report where the original line said so).tests/test_command_template_hooks.py: for every template that readsextensions.yml, asserts that the old wording is gone and that each parse-failure line tells the user both facts (manifest could not be read, no hooks were checked). It reads the templates as text because the behaviour lives in the prompt.Testing
uv sync && uv run pytest(equivalent environment: Python 3.14.4,pip install -e .[test], offline sandbox). Full suite: 7737 tests, 196 skipped, 4 failures. The same 4 fail onmain(4a7341a) in the same sandbox and are environmental:test_bash_command_hint_falls_back_to_awk_when_jq_and_python3_broken,test_ps_variant_prefixed_with_powershell_launcher, and the twotest_bundler_referencescases that need the network. None of them reads a template.templates/commands/plan.mdtoHEAD, the twoplan.mdcases fail with the expected message, restored the change, 21 pass again.uv run specify --help(no CLI code touched)AI Disclosure
The finding, the wording of the replacement lines, the test file and this description were drafted with an AI coding assistant (Claude Code) during a read-only review of the repository, and reviewed line by line by the maintainer of this fork before opening the PR. Replies on this PR will be written the same way and disclosed as such.