feat: add lifecycle hooks for openspec/config.yaml#1102
Closed
519926695 wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete lifecycle hook system for OpenSpec workflows. It adds optional ChangesConfig YAML Hooks Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/core/project-config-hooks.test.ts (1)
78-85: ⚡ Quick winConsider adding explicit null checks for consistency.
Lines 79-80 and 84-85 use the non-null assertion operator (
result!) without first asserting thatresultis not null, unlike the pattern established in lines 71-72. While the tests will pass, adding an explicitexpect(result).not.toBeNull();check before accessing properties improves test clarity and provides better error messages ifparseHooksFieldunexpectedly returns null.✨ Suggested consistency improvement
it('should parse a hook entry with only instruction', () => { const result = parseHooksField({ 'pre-apply': { instruction: 'Check tasks' } }); + expect(result).not.toBeNull(); expect(result!['pre-apply']).toEqual({ instruction: 'Check tasks' }); }); it('should parse a hook entry with only run', () => { const result = parseHooksField({ 'pre-archive': { run: './scripts/check.sh' } }); + expect(result).not.toBeNull(); expect(result!['pre-archive']).toEqual({ run: './scripts/check.sh' }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/project-config-hooks.test.ts` around lines 78 - 85, The two tests calling parseHooksField ('should parse a hook entry with only instruction' and 'should parse a hook entry with only run') use the non-null assertion operator result! without asserting result is not null; add an explicit expect(result).not.toBeNull() before accessing result (in the tests that reference parseHooksField for 'pre-apply' and 'pre-archive') so the tests mirror the null-check pattern used elsewhere and yield clearer failures if parseHooksField returns null.openspec/changes/archive/2026-05-18-add-config-yaml-hooks/design.md (1)
78-92: 💤 Low valueConsider adding a language identifier to the code block.
The code block starting at line 78 lacks a language identifier. While this pseudo-code outline is clear without it, adding
textorplaintextwould satisfy markdown best practices.📝 Optional fix
-``` +```text 1. Call: openspec hooks get pre-<workflow> --json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/archive/2026-05-18-add-config-yaml-hooks/design.md` around lines 78 - 92, The markdown code block in design.md showing the hook execution steps (the block containing "Call: openspec hooks get pre-<workflow> --json" through "Follow the instruction as a post-workflow action") lacks a language identifier; update that fenced code block to include a language tag such as "text" or "plaintext" (e.g., change ``` to ```text) so the snippet follows Markdown best practices while preserving the existing pseudo-code and line ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/archive/2026-05-18-add-config-yaml-hooks/proposal.md`:
- Line 28: The proposal incorrectly names the CLI command as "openspec hooks run
<event>"; update the text to use the consistent command name "openspec hooks get
<event>" (replace the string "openspec hooks run <event>" with "openspec hooks
get <event>") so it matches D2 and the rest of the specs and tooling references
to the hooks command.
In `@openspec/changes/archive/2026-05-18-add-config-yaml-hooks/tasks.md`:
- Line 11: Update the checklist entry text to fix the typo "resilientely" →
"resiliently" in the task description that mentions parseHooksField in
src/core/project-config.ts; ensure the revised line still references
VALID_HOOK_EVENTS and the behavior (validates keys, warns on unknown keys,
parses instruction/run fields) and search the file for any other occurrences of
the misspelling to correct them as well.
In `@src/core/project-config.ts`:
- Line 254: The function readProjectConfig currently returns null when hasValid
is false, which collapses an explicit empty hooks object; modify the return
logic around hasValid/result so that if the original hooks field was explicitly
provided (even if empty) readProjectConfig preserves and returns an empty object
for hooks instead of null. Concretely, detect whether the incoming/parsed hooks
value was present (e.g., a variable like rawHooks or the original parsed config)
and change the final return (where it currently says "return hasValid ? result :
null") to return result (or {} for hooks) when the hooks key existed but had no
valid entries, otherwise return null; reference hasValid, result, and
readProjectConfig to locate where to change the conditional.
- Around line 197-202: The current early-return treats raw === null the same as
missing input; change the check so that if raw === null you emit a warning about
an explicit null for the 'hooks' field and then return null, while preserving
the existing behavior for raw === undefined. Locate the code that inspects the
variable raw in this module (the block that currently does "if (raw === null ||
raw === undefined) return null;") and split it into two cases: one that logs a
clear warning like "Invalid 'hooks' field in config: explicit null provided"
when raw === null, and the other that simply returns null when raw ===
undefined.
In `@src/core/templates/workflows/propose.ts`:
- Around line 34-38: The handling of pre-propose hook results is ambiguous when
both "run" and "instruction" are present: ensure in the hook-parsing logic that
"instruction" is only applied if the "run" command either is not provided or
completes successfully; specifically, in the code path that parses the JSON
result (checking exists, run, instruction), if exists === true and run is set
then execute run and on non-zero exit immediately halt and return the error (do
NOT apply instruction), and only if run exits with code 0 (or if run was absent)
then apply the instruction binding for the workflow. Reference the JSON fields
"exists", "run", and "instruction" and the pre-propose hook execution branch
when updating the logic.
---
Nitpick comments:
In `@openspec/changes/archive/2026-05-18-add-config-yaml-hooks/design.md`:
- Around line 78-92: The markdown code block in design.md showing the hook
execution steps (the block containing "Call: openspec hooks get pre-<workflow>
--json" through "Follow the instruction as a post-workflow action") lacks a
language identifier; update that fenced code block to include a language tag
such as "text" or "plaintext" (e.g., change ``` to ```text) so the snippet
follows Markdown best practices while preserving the existing pseudo-code and
line ordering.
In `@test/core/project-config-hooks.test.ts`:
- Around line 78-85: The two tests calling parseHooksField ('should parse a hook
entry with only instruction' and 'should parse a hook entry with only run') use
the non-null assertion operator result! without asserting result is not null;
add an explicit expect(result).not.toBeNull() before accessing result (in the
tests that reference parseHooksField for 'pre-apply' and 'pre-archive') so the
tests mirror the null-check pattern used elsewhere and yield clearer failures if
parseHooksField returns null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c13f9036-4e8f-4afc-8341-0a2cbdd553c4
📒 Files selected for processing (19)
openspec/changes/archive/2026-05-18-add-config-yaml-hooks/.openspec.yamlopenspec/changes/archive/2026-05-18-add-config-yaml-hooks/design.mdopenspec/changes/archive/2026-05-18-add-config-yaml-hooks/proposal.mdopenspec/changes/archive/2026-05-18-add-config-yaml-hooks/specs/config-loading/spec.mdopenspec/changes/archive/2026-05-18-add-config-yaml-hooks/specs/config-yaml-hooks/spec.mdopenspec/changes/archive/2026-05-18-add-config-yaml-hooks/specs/skill-hook-execution/spec.mdopenspec/changes/archive/2026-05-18-add-config-yaml-hooks/tasks.mdopenspec/specs/config-loading/spec.mdopenspec/specs/config-yaml-hooks/spec.mdopenspec/specs/skill-hook-execution/spec.mdsrc/cli/index.tssrc/core/project-config.tssrc/core/templates/workflows/apply-change.tssrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/explore.tssrc/core/templates/workflows/propose.tstest/commands/hooks.test.tstest/core/project-config-hooks.test.tstest/core/templates/skill-templates-parity.test.ts
|
|
||
| - `src/core/project-config.ts` — add `HookConfig`, `WorkflowHooks`, and `HooksConfig` types; extend `ProjectConfigSchema` and `readProjectConfig`. | ||
| - Skill SKILL.md files (`openspec-propose`, `openspec-explore`, `openspec-apply-change`, `openspec-archive-change`, `opsx:*` aliases) — add hook-read and hook-enforcement steps. | ||
| - New CLI subcommand or helper (e.g., `openspec hooks run <event>`) to surface hook config to skills in a structured way. |
Contributor
There was a problem hiding this comment.
Command name inconsistency: use "openspec hooks get" to match the design.
Line 28 references openspec hooks run <event> but the design document (D2) and all specs consistently specify openspec hooks get <event> as the actual command being implemented. This should be corrected to avoid confusion.
📝 Proposed fix
-- New CLI subcommand or helper (e.g., `openspec hooks run <event>`) to surface hook config to skills in a structured way.
+- New CLI subcommand `openspec hooks get <event>` to surface hook config to skills in a structured way.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - New CLI subcommand or helper (e.g., `openspec hooks run <event>`) to surface hook config to skills in a structured way. | |
| - New CLI subcommand `openspec hooks get <event>` to surface hook config to skills in a structured way. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/archive/2026-05-18-add-config-yaml-hooks/proposal.md` at
line 28, The proposal incorrectly names the CLI command as "openspec hooks run
<event>"; update the text to use the consistent command name "openspec hooks get
<event>" (replace the string "openspec hooks run <event>" with "openspec hooks
get <event>") so it matches D2 and the rest of the specs and tooling references
to the hooks command.
|
|
||
| ## 2. Config Parsing | ||
|
|
||
| - [x] 2.1 Add `parseHooksField` function in `src/core/project-config.ts` that validates each key against `VALID_HOOK_EVENTS`, warns on unknown keys, and parses each entry's `instruction`/`run` fields resilientely |
Contributor
There was a problem hiding this comment.
Fix typo: "resilientely" should be "resiliently".
📝 Proposed fix
-- [x] 2.1 Add `parseHooksField` function in `src/core/project-config.ts` that validates each key against `VALID_HOOK_EVENTS`, warns on unknown keys, and parses each entry's `instruction`/`run` fields resilientely
+- [x] 2.1 Add `parseHooksField` function in `src/core/project-config.ts` that validates each key against `VALID_HOOK_EVENTS`, warns on unknown keys, and parses each entry's `instruction`/`run` fields resiliently📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [x] 2.1 Add `parseHooksField` function in `src/core/project-config.ts` that validates each key against `VALID_HOOK_EVENTS`, warns on unknown keys, and parses each entry's `instruction`/`run` fields resilientely | |
| - [x] 2.1 Add `parseHooksField` function in `src/core/project-config.ts` that validates each key against `VALID_HOOK_EVENTS`, warns on unknown keys, and parses each entry's `instruction`/`run` fields resiliently |
🧰 Tools
🪛 LanguageTool
[grammar] ~11-~11: Ensure spelling is correct
Context: ...each entry's instruction/run fields resilientely - [x] 2.2 Call parseHooksField in `re...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/archive/2026-05-18-add-config-yaml-hooks/tasks.md` at line
11, Update the checklist entry text to fix the typo "resilientely" →
"resiliently" in the task description that mentions parseHooksField in
src/core/project-config.ts; ensure the revised line still references
VALID_HOOK_EVENTS and the behavior (validates keys, warns on unknown keys,
parses instruction/run fields) and search the file for any other occurrences of
the misspelling to correct them as well.
Comment on lines
+197
to
+202
| if (raw === null || raw === undefined) return null; | ||
|
|
||
| if (typeof raw !== 'object' || Array.isArray(raw)) { | ||
| console.warn(`Invalid 'hooks' field in config (must be object)`); | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Log a warning when hooks is explicitly null.
Line 197 treats null the same as missing input. If the YAML contains hooks: null, that’s a user-provided invalid type and should emit a warning for troubleshooting.
Suggested fix
export function parseHooksField(raw: unknown): HooksConfig | null {
- if (raw === null || raw === undefined) return null;
+ if (raw === undefined) return null;
+ if (raw === null) {
+ console.warn(`Invalid 'hooks' field in config (must be object)`);
+ return null;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/project-config.ts` around lines 197 - 202, The current early-return
treats raw === null the same as missing input; change the check so that if raw
=== null you emit a warning about an explicit null for the 'hooks' field and
then return null, while preserving the existing behavior for raw === undefined.
Locate the code that inspects the variable raw in this module (the block that
currently does "if (raw === null || raw === undefined) return null;") and split
it into two cases: one that logs a clear warning like "Invalid 'hooks' field in
config: explicit null provided" when raw === null, and the other that simply
returns null when raw === undefined.
| hasValid = true; | ||
| } | ||
|
|
||
| return hasValid ? result : null; |
Contributor
There was a problem hiding this comment.
Empty hooks: {} is collapsed to undefined instead of preserving empty config.
Line 254 returns null when no valid entries were found, which makes an explicit empty hooks object disappear in readProjectConfig. This breaks the documented contract for hooks: {}.
Suggested fix
export function parseHooksField(raw: unknown): HooksConfig | null {
if (raw === null || raw === undefined) return null;
@@
- const result: HooksConfig = {};
+ const result: HooksConfig = {};
+ const rawEntries = Object.entries(raw as Record<string, unknown>);
+ if (rawEntries.length === 0) {
+ return {};
+ }
let hasValid = false;
- for (const [key, value] of Object.entries(raw as Record<string, unknown>)) {
+ for (const [key, value] of rawEntries) {
@@
return hasValid ? result : null;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/project-config.ts` at line 254, The function readProjectConfig
currently returns null when hasValid is false, which collapses an explicit empty
hooks object; modify the return logic around hasValid/result so that if the
original hooks field was explicitly provided (even if empty) readProjectConfig
preserves and returns an empty object for hooks instead of null. Concretely,
detect whether the incoming/parsed hooks value was present (e.g., a variable
like rawHooks or the original parsed config) and change the final return (where
it currently says "return hasValid ? result : null") to return result (or {} for
hooks) when the hooks key existed but had no valid entries, otherwise return
null; reference hasValid, result, and readProjectConfig to locate where to
change the conditional.
Comment on lines
+34
to
+38
| Parse the JSON result: | ||
| - If \`exists: false\` or if the command is unavailable (non-zero exit) — proceed normally. | ||
| - If \`exists: true\` and \`run\` is set — execute the \`run\` command via Bash. If it exits with a non-zero code, **HALT**: report "pre-propose hook failed (exit code N): <stderr>" and do NOT continue. | ||
| - If \`exists: true\` and \`instruction\` is set — treat the instruction text as a binding directive for this entire workflow. | ||
|
|
Contributor
There was a problem hiding this comment.
Pre-hook “both fields set” ordering is ambiguous vs required behavior.
Line 36/172 says run failure must halt, but Line 37/173 independently instructs applying instruction. Please make the instruction bullet explicitly conditional on successful run when both are present, to match the hook-execution spec and avoid accidental directive application on failed pre-hooks.
Suggested wording tweak
- - If `exists: true` and `instruction` is set — treat the instruction text as a binding directive for this entire workflow.
+ - If `exists: true` and `instruction` is set:
+ - when `run` is not set, apply it immediately as a binding directive;
+ - when both `run` and `instruction` are set, apply `instruction` only after `run` succeeds.Also applies to: 170-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/templates/workflows/propose.ts` around lines 34 - 38, The handling
of pre-propose hook results is ambiguous when both "run" and "instruction" are
present: ensure in the hook-parsing logic that "instruction" is only applied if
the "run" command either is not provided or completes successfully;
specifically, in the code path that parses the JSON result (checking exists,
run, instruction), if exists === true and run is set then execute run and on
non-zero exit immediately halt and return the error (do NOT apply instruction),
and only if run exits with code 0 (or if run was absent) then apply the
instruction binding for the workflow. Reference the JSON fields "exists", "run",
and "instruction" and the pre-propose hook execution branch when updating the
logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a hooks field to openspec/config.yaml for declaring pre/post lifecycle automation on the four core workflows: propose, explore, apply, and archive
Introduces openspec hooks get [--json] CLI command so skills can query hook config as structured JSON
Updates all four workflow skill templates (propose, explore, apply, archive) to enforce pre-hooks as blocking gates and post-hooks as non-blocking warnings
What's New
Config Schema
hooks:
pre-archive:
run: ./scripts/check-completeness.sh
post-archive:
instruction: |
Review the archived change and consolidate the error log.
run: ./scripts/notify-slack.sh
Each entry supports optional instruction (injected into AI reasoning context) and/or run (shell command). Eight valid event keys: pre/post-propose, pre/post-explore, pre/post-apply, pre/post-archive.
New CLI Command
openspec hooks get post-archive --json
→ { "event": "post-archive", "instruction": "...", "run": "...", "exists": true }
Returns exists: false gracefully when no hook is configured or no config file exists.
Skill Enforcement
All four workflow skills and their opsx:* aliases now:
Call openspec hooks get pre- --json before starting — halt on non-zero run exit
Inject instruction text as a binding directive into active reasoning
Call openspec hooks get post- --json after completing — surface run failures as warnings only
If openspec hooks get is unavailable (older CLI), skills degrade gracefully and proceed normally.
Test Plan
test/core/project-config-hooks.test.ts — 19 unit tests for parseHooksField and readProjectConfig hooks integration
test/commands/hooks.test.ts — 6 CLI integration tests for openspec hooks get
test/core/templates/skill-templates-parity.test.ts — SHA-256 hashes updated for all modified templates
Full test suite: 78 test files / 1536 tests passing
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
openspec hooks get <event>with--jsonoutput to inspect configured hooks.