fix(apply): warn when a change is ready to implement with no specs - #1783
fix(apply): warn when a change is ready to implement with no specs#1783clay-good wants to merge 5 commits into
Conversation
Apply gates on the schema's `apply.requires` (tasks) alone, so a change whose tasks file was written ahead of its specs read as ready even though it had no delta specs at all — the state `openspec validate` rejects. Apply was the one surface that green-lit a change every other surface flags, which is how agents end up implementing before the specs exist. Report it as a warning, in the text output and in `--json`, naming both ways out: write the specs, or declare `skip_specs: true`. Blocking would be a policy change; naming the gap is not. Changes that have specs, declare `skip_specs`, or are still blocked on their own required artifacts are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying openspec-docs with
|
| Latest commit: |
5cd26d1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2ec30f23.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-apply-warns-missing-spec.openspec-docs.pages.dev |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthrough
ChangesApply instruction diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to Apply instructions now surface non-blocking missing-spec guidance and complete prerequisite chains while preserving existing apply gating. The updated behavior is covered across JSON, text, schema, and validation scenarios and is ready to merge. Sequence Diagram(s)sequenceDiagram
participant Change
participant generateApplyInstructions
participant collectMissingPrerequisites
participant collectApplyWarnings
participant ApplyOutput
Change->>generateApplyInstructions: request apply instructions
generateApplyInstructions->>collectMissingPrerequisites: resolve required artifact chain
collectMissingPrerequisites-->>generateApplyInstructions: return missing prerequisites
generateApplyInstructions->>collectApplyWarnings: inspect state and spec outputs
collectApplyWarnings-->>generateApplyInstructions: return warnings when delta specs are absent
generateApplyInstructions->>ApplyOutput: include diagnostics in JSON or text output
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR addresses the apply-stage symptom from issue Resolution Implement the sequence enforcement required by issue Full details: Docstring CoverageExplanation Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A schema with no spec-producing artifact must stay quiet, and one whose spec artifact is not called `specs` must still warn - the rule keys off the output path, not the artifact id. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No PR-relevant drift confirmed.
|
os.tmpdir() hands back the short form (C:\Users\RUNNER~1) while the CLI resolves the long one, so the assertion pinned a path that never matched on windows-pwsh. Assert the change-relative tail instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Apply blocks on the schema's `apply.requires` alone, so its message stopped at the first hop: a change holding only a proposal was told "Missing artifacts: tasks" while the specs `tasks` depends on were missing too. Taken literally that is an instruction to write the tracking file straight from the proposal and skip everything between — the failure reported in #834 and #869. Walk `requires` and report the whole set, in build order, as `missingPrerequisites` (text and `--json`). What apply blocks on is unchanged, and the wording leaves conditional artifacts to the schema rather than demanding them. The remedies these messages give are now CLI commands rather than the `openspec-continue-change` skill: `continue` is not in CORE_WORKFLOWS, so on the default profile the old advice named a skill that is never installed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Status: Ready for review.
Closes #834. Related: #869 (same failure seen from Copilot).
What was wrong
Two ways
openspec instructions applytold an agent to skip the artifacts a change is supposed to be built from.1. It reported
readyfor a change with no specs at all. Apply gates on the schema'sapply.requires— forspec-driventhat istasksalone. Nothing checks thattasksown prerequisites were built, so a change whosetasks.mdwas written ahead of its specs came back ready to implement:statussays the specs are missing,openspec validate demo-changefails the change ("Change must have at least one delta … setskip_specs: true"), andarchivewarns about it. Apply was the one surface saying "go ahead" — the surface an agent reads immediately before writing code.2. When it did block, it named only the first hop. A change holding nothing but a proposal got:
Read literally, that is an instruction to write
tasks.mdstraight from the proposal — which lands you back in case 1. And the remedy was a dead end on the default install:continueis not inCORE_WORKFLOWS, so thecoreprofile never installs the skill the message named.How it was fixed
Ready-state warning.
generateApplyInstructionsnow collects warnings about the change itself. One rule today: apply is past its own gate, the schema declares spec-producing artifacts, none produced a file, and the change does not declareskip_specs: true.Blocked-state chain. Walking
requiresfromapply.requiresgives everything still to build, in build order — reported asmissingPrerequisites:Remedies that exist everywhere. Every message in this function now points at
openspec instructions <artifact> --change <name>andopenspec status --change <name>instead of theopenspec-continue-changeskill. The CLI verbs are what the skill runs, and they are there on every profile.Deliberate boundaries:
missingArtifactsstill decidesblocked, so no change that applied before applies any differently now. No deadlock risk and no new policy. Turning the no-specs warning into a hard block is a one-line follow-up, but it is a policy call and belongs to the founder, not to this PR.isSpecsArtifactPathhelper thatskip_specsalready uses — no artifact id is hardcoded, so custom schemas keep working. A schema with no spec-producing artifact getsskip_specsat change creation, so this never fires on one.designis never demanded. It is optional in practice (38 of the 83 archived changes in this repo have adesign.md) and, unlike specs, has no opt-out marker. It appears in the build-order list because the schema declares it — the wording says "build the ones this change needs … the schema says which are conditional" — and when several artifacts are left the remedy stays<artifact>rather than naming the first one, which would point atdesignas often as atspecs.--jsongainsmissingPrerequisites?: string[]andwarnings?: string[]; both documented indocs/agent-contract.md§4.6.Replication / proof
The reproduction above, against the built CLI on this branch:
18 tests across two new files, run against unmodified
src/first to confirm they fail:test/commands/apply-instructions-warnings.test.ts(11) — warns on a ready change with no specs; prints the section above the context files; still warns once every task is done; warns about exactly the stateValidator.validateChangeDeltaSpecsrejects, and stays quiet about exactly the state it accepts (so the message cannot drift from the rule it cites); quiet with specs, quiet withskip_specs, quiet while blocked; quiet for a custom schema that produces no specs; warns for a custom schema whose spec artifact is calledcontracts. 3 failed before the change.test/commands/apply-instructions-blocked.test.ts(7) — names the whole chain; leaves conditional artifacts to the schema; drops the chain line once only the required artifact is left; counts a skippedspecsas built; never namesopenspec-continue-change; no prerequisites once ready; prints the chain under the blocked heading. 6 failed before the change.Full local suite: 4419 passed, 19 failed — 16 vitest 10s timeouts plus one
npm ETIMEDOUTin subprocess-heavy e2e files on a loaded machine, and the two failures that reproduce withsrc/reverted tomain(config-profile,artifact-workflow; sandbox path/permission errors under/var/folders). No assertion failure anywhere near this change. CI is the real check: linux, macOS and windows-pwsh all green.Notes / nits
/opsx:explore) is the same class of failure but starts before any apply call, in the workflow templates. This PR covers every CLI surface an agent hits on the way to implementing; it does not claim the template half.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
openspec instructions applynow warns when changes lack delta specs without declaringskip_specs: true.Documentation