fix(apply): fail with exit 1 when apply instructions are blocked#1250
fix(apply): fail with exit 1 when apply instructions are blocked#1250lawsonoates wants to merge 3 commits into
Conversation
Exit with code 1 when apply instructions are blocked, add a spec-driven hint when delta specs are absent, and extend tests plus the tmp-init fixture so blocked and ready apply paths are covered.
Expect only tasks in the missing-artifacts message and assert that missing delta specs do not block apply while the default schema still requires tasks alone.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesApply instructions blocked state and spec-driven hint
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
PR #1250 Review: blocked apply exits non-zero
Reviewed: 2026-06-24 23:18 AEST
PR: #1250
Author: @lawsonoates
Head: 9315bd1cdb32447f15f9d735eed77a969eeec914
Issue: #1212, spec-driven fast-path workflow silently leaves specs stale
Verdict
Changes requested.
The exit-code hardening is good and should be kept, but the PR no longer fixes the root #1212 path. The final commit explicitly preserves the existing spec-driven apply contract of apply.requires: [tasks] and adds a regression test proving that a change with proposal.md, design.md, and tasks.md but no specs/ is still considered ready to apply.
That is exactly the unsafe state described in #1212: fast-path generation can stop at apply requirements, opsx:apply proceeds, and archive later has no delta specs to sync.
What changed
src/commands/workflow/instructions.tsapplyInstructionsCommand()now setsprocess.exitCode = 1when generated apply instructions areblocked.- JSON and text output both still print before the non-zero exit is set.
generateApplyInstructions()includes a spec-driven delta-spec hint only whenspecsis already inmissingArtifacts.
test/commands/artifact-workflow.test.ts- Blocked apply now expects exit code 1.
- New regression asserts missing delta specs do not block apply when tasks exist.
test/fixtures/tmp-init/openspec/changes/c1/tasks.md- Added a simple tasks fixture so existing apply JSON path is ready.
Verification
Ran in /tmp/openspec-pr1250:
pnpm install --frozen-lockfile
pnpm exec vitest run test/commands/artifact-workflow.test.tsResult: 1 file, 60 tests passed.
Review findings
Blocking: missing delta specs still pass apply for spec-driven changes
The final PR state leaves schemas/spec-driven/schema.yaml as:
apply:
requires: [tasks]generateApplyInstructions() derives missingArtifacts exclusively from that apply.requires list. Since specs is not required, this branch never runs for the #1212 failure state:
const specsHint =
missingArtifacts.includes('specs') && context.schemaName === 'spec-driven'The new test makes this explicit:
await createTestChange('missing-specs-apply', ['proposal', 'design', 'tasks']);
expect(result.exitCode).toBe(0);
expect(json.state).toBe('ready');
expect(json.missingArtifacts).toBeUndefined();That means the unsafe fast path remains green. Exit-code hardening only helps when the command is already blocked, but #1212 is about the command failing to block.
Recommended fix
Minimum acceptable fix for this PR:
- Add
specstoschemas/spec-driven/schema.yamlapply.requires, likelyrequires: [specs, tasks]. - Flip the missing-specs test to expect blocked state, exit code 1,
missingArtifacts: ['specs'], and the delta-spec hint. - Keep the existing exit-code hardening.
Follow-up still worth tracking separately:
- Archive should also guard against spec-driven changes with no delta specs before it cleanly reports “No delta specs”. That is the last line of defense if the apply fast path regresses again.
Notes
This is small and close. I would not merge as-is because it creates a false sense that #1212 is fixed while preserving the exact missing-specs apply path.
Add specs to apply.requires so fast-path workflows cannot stop at tasks alone. Update apply instruction tests to expect blocked state when delta specs are missing.
|
@alfred-openspec implemented your recommendations |
2 participants
Summary
Fix for #1212.
Hardens the apply path so missing required artifacts are treated as a hard failure instead of a soft warning the agent can ignore.
Changes
openspec instructions applynow sets exit code 1 when apply is blocked (missing required artifacts per the schema'sapply.requires)specsis among the missing artifacts, pointing authors to create delta specs underchanges/<name>/specs/before implementationspecsis inapply.requires)tmp-initfixture withtasks.mdso the apply JSON e2e test exercises a ready change, not a blocked oneBehavior
Before: Apply could report
Blockedbut still exit 0, making it easy for agents/scripts to proceed anyway.After: Blocked apply exits 1 and includes clearer guidance when delta specs are missing.
Testing
pnpm exec vitest run test/commands/artifact-workflow.test.tspnpm test(full suite)Notes
I was unable to reproduce the exact failure described in #1212. On a greenfield playground project, the
propose → apply → archivepath created delta specs during propose, and archive synced them intoopenspec/specs/as expected.A follow-up could add
specsto the defaultapply.requiresif others can reproduce the skip-delta-specs behaviour.Summary by CodeRabbit
applycommand now returns a non-zero exit status when apply instructions are blocked, improving CI/script reliability.