fix(templates): make the schema instruction field authoritative for artifact creation#1405
Conversation
📝 WalkthroughWalkthroughUpdated propose, continue, and fast-forward workflows to treat schema ChangesSchema Instruction Authority
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant OpenSpecInstructions
participant Workflow
participant DelegatedSkillOrCommand
participant ArtifactFile
OpenSpecInstructions->>Workflow: provide artifact instruction
Workflow->>DelegatedSkillOrCommand: invoke delegated creator
DelegatedSkillOrCommand->>ArtifactFile: create artifact at resolvedOutputPath
Workflow->>ArtifactFile: verify resolved output exists
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The template change looks right and isolated verification passes, but the PR only updates self-maintained hash baselines. Please add a focused assertion across the skill and command variants that the instruction field is authoritative, delegated creation is invoked, and the old Common artifact patterns shortcut is absent, so #777's behavior is pinned rather than accepting any regenerated prompt.
|
@alfred-openspec addressed in 310d257: added a focused test ( |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-review at 310d257: the new six-variant assertion now pins instruction authority, delegation at the creation step, output verification, and removal of the hard-coded artifact shortcut. Clean regeneration, focused tests, build, and the full exact-head CI matrix all pass.
…rtifact creation The continue-change skill and command embedded hard-coded spec-driven artifact patterns that agents followed instead of the schema's instruction field whenever a custom schema reused familiar artifact names, so schemas could not delegate artifact creation to their own skills. Drop the hard-coded patterns, state that the instruction field is authoritative, and tell both continue and ff workflows to invoke a skill when the instruction delegates to one. Fixes #777 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ep and in propose Adversarial review findings: the propose workflow shared the same creation loop and pre-fix wording as ff, and the numbered creation steps still commanded a direct write before the agent ever reached the delegation guideline. Add the delegation conditional at the point of creation in propose, continue, and ff (skill and command variants), add the authoritative-instruction bullets to propose, and verify the artifact exists after a delegated skill runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r in tests Address alfred's review: the hash baselines alone accepted any regenerated prompt, so add a focused parity assertion covering all six variants (propose/continue/ff x skill/command) that the instruction field is the authoritative guidance, delegated creation is invoked and verified at the creation step and restated in the guidelines, and the old "Common artifact patterns" shortcut stays gone. The test fails against the pre-fix templates. Also fix an ordering contradiction the adversarial review surfaced: the continue-change delegation bullet preceded the dependency-read bullet and said "instead of following the bullets below", telling agents to skip dependency reads that the guardrails require. It now mirrors propose/ff: read dependencies first, then delegate "instead of writing the file yourself" - making the sentence identical across all six variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
310d257 to
78759e1
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The rebase preserves dependency-first delegation and output verification across all six propose, continue, and fast-forward variants, including the focused behavior assertion. Exact-head build, clean skill regeneration, 11 parity tests, and the full cross-platform CI/CodeQL matrix pass.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@skills/openspec-continue-change/SKILL.md`:
- Around line 71-72: Update the delegated artifact flow in the instruction-field
handling to resolve the schema-selected concrete output path from
resolvedOutputPath before checking file existence. Verify that exact resolved
file rather than testing the glob pattern literally, while preserving the
existing delegation and artifact-generation behavior.
🪄 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: f4fde62e-60ba-44b0-8870-f72759c6b2d4
📒 Files selected for processing (8)
.changeset/instruction-field-authority.mdskills/openspec-continue-change/SKILL.mdskills/openspec-ff-change/SKILL.mdskills/openspec-propose/SKILL.mdsrc/core/templates/workflows/continue-change.tssrc/core/templates/workflows/ff-change.tssrc/core/templates/workflows/propose.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/instruction-field-authority.md
- src/core/templates/workflows/continue-change.ts
- src/core/templates/workflows/propose.ts
- src/core/templates/workflows/ff-change.ts
- test/core/templates/skill-templates-parity.test.ts
| - If the `instruction` field delegates creation to a specific skill or command, invoke it to produce the artifact instead of writing the file yourself, then verify the artifact file exists at `resolvedOutputPath` | ||
| - Otherwise use `template` as the structure - fill in its sections |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Resolve glob outputs before verifying creation.
resolvedOutputPath may be a glob, not a concrete filename. A delegated artifact should first resolve the schema-selected concrete path, then verify that exact file exists; otherwise a literal existence check can fail for valid glob-based schemas.
🤖 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 `@skills/openspec-continue-change/SKILL.md` around lines 71 - 72, Update the
delegated artifact flow in the instruction-field handling to resolve the
schema-selected concrete output path from resolvedOutputPath before checking
file existence. Verify that exact resolved file rather than testing the glob
pattern literally, while preserving the existing delegation and
artifact-generation behavior.
2 participants
Status: LGTM — full suite green locally (104 files / 1,995 tests; the 17 known environment-only
zsh-installerfailures are addressed by #1400 and don't affect CI), hashes independently re-verified, zerogenerate:skillsdrift, and five adversarial review passes applied across the three commits. Alfred's requested change (a focused behavioral test, not just hash baselines) is addressed in the third commit.What was wrong: The
openspec-continue-changeskill and/opsx:continuecommand embedded hard-coded one-line "common artifact patterns" forproposal.md,specs/,design.md, andtasks.md, with the schema'sinstructionfield relegated to "other schemas." Since most custom schemas reuse those familiar artifact names, agents followed the hard-coded shortcuts and ignored the custominstruction— so a schema could not delegate artifact creation to its own skill (e.g. Flokay's"You MUST use the flokay:design skill"was treated as prose). Theproposeandffworkflows shared the same artifact-creation loop with no cue that a skill reference means invoke that skill.How it was fixed: Template text only — no CLI or schema behavior changes.
continue-change(skill + command): hard-coded spec-driven pattern block removed, replaced with "theinstructionfield is the authoritative guidance, even for familiar artifact names" plus a skill-delegation directive.propose,continue, andff(skill + command variants): the delegation conditional is stated at the artifact-creation step itself — "if theinstructionfield delegates creation to a specific skill or command, invoke it to produce the artifact instead of writing the file yourself, then verify the artifact file exists atresolvedOutputPath" — not just in the trailing guidelines, so an agent executing steps in order sees it before writing anything. The sentence is identical across all six variants, and incontinueit follows the dependency-read bullet (an adversarial pass caught the earlier ordering telling agents to skip dependency reads that the guardrails require).skill-templates-parity.test.tsnow asserts, for all six variants, that theinstructionfield is the authoritative guidance, that delegated creation is invoked and verified at the creation step and restated in the guidelines, and thatCommon artifact patternsis absent — so openspec-continue-change hard-coded artifact patterns override schema instruction field #777's behavior is pinned independently of the hash baselines.skills/openspec-{continue-change,ff-change,propose}/SKILL.md, updated the golden hashes, added a patch changeset.Why removing the pattern block is safe: every removed one-liner is already covered — in far more detail — by the spec-driven schema's own
instructionfields inschemas/spec-driven/schema.yaml(verified line by line; the one item not in the schema, "ask the user if unclear," survives as an existing guardrail in both templates). The delegation directive is conditional, and the default schema never references a skill, so the default workflow is untouched.Proof:
main: 3 tests fail, including the new one) and passes with them.npm run build && npm run generate:skillsproduces zero drift against the committedskills/files;npx vitest run— 1,995 passing across 104 files including all parity suites.dist/and matched; hash churn is exactly the 3 touched workflow files (6 function hashes + 3 generated-skill hashes), nothing else moved.Notes:
skill:convention for the instruction field is a design change and intentionally out of scope.onboardandupdate-changehave related-but-different hard-coded assumptions; left for a follow-up since they are guided flows, not theinstruction-driven creation loop openspec-continue-change hard-coded artifact patterns override schema instruction field #777 describes. Theproposeintro's "proposal.md (what & why), design.md (how), tasks.md" announcement line is likewise pre-existing follow-up material.Fixes #777
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
instructionguidance is now authoritative for artifact creation, replacing any default “common artifact patterns” behavior.Tests
instructionand no longer includes the old “common artifact patterns” shortcut.