From c4b345271a6a0b28d7a4b6f5d8232b544e18393c Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 10 Sep 2026 12:30:26 -0500 Subject: [PATCH 1/6] fix(update-change): draft the requested edit in step 4, write only in step 5 Step 4 told the agent to "Apply the requested edit" while step 5 and the guardrails told it to write only after the user confirms each revision. "Apply" is a write verb in this very document - step 5 is titled "Confirm and apply" - so the same request either wrote immediately or stopped and showed the revision first, depending on which passage the agent weighed. Step 5 is the workflow's only write path, so its confirmation guarantee was unenforceable whenever step 4 governed. Step 4 now drafts and says explicitly that it writes nothing; step 5 claims every write and shows the drafted edit for confirmation. Both delivery surfaces and the committed skill carry the same wording, and a regression test slices step 4 out of each body so no write verb can reappear there. Closes #1836 Co-Authored-By: Claude Opus 5 --- .changeset/update-change-step-four-drafts.md | 5 +++ skills/openspec-update-change/SKILL.md | 7 ++-- src/core/templates/workflows/update-change.ts | 14 ++++--- .../templates/skill-templates-parity.test.ts | 6 +-- test/core/templates/update-change.test.ts | 42 +++++++++++++++++++ 5 files changed, 62 insertions(+), 12 deletions(-) create mode 100644 .changeset/update-change-step-four-drafts.md diff --git a/.changeset/update-change-step-four-drafts.md b/.changeset/update-change-step-four-drafts.md new file mode 100644 index 0000000000..f79b97c0ce --- /dev/null +++ b/.changeset/update-change-step-four-drafts.md @@ -0,0 +1,5 @@ +--- +'@fission-ai/openspec': patch +--- + +Resolve the contradiction that left `/opsx:update`'s only write path without a governing rule. Step 4 told the agent to "Apply the requested edit", while step 5 and the guardrails told it to write only after the user confirms each revision. "Apply" reads as a write verb in that very document — step 5 is titled "Confirm and apply" — so the same `/opsx:update "the design now uses X"` either wrote the edit immediately or stopped and showed the proposed revision first, depending on which passage the agent weighed. Since step 5 is the workflow's only write path, its confirmation guarantee was unenforceable whenever step 4 governed. Step 4 now drafts the edit and says explicitly that it writes nothing, and step 5 states that it performs every write in the workflow and shows the drafted edit for confirmation along with everything else. Both delivery surfaces and the committed skill carry the same wording, and a regression test slices step 4 out of each body so no write verb can reappear there. Fixes #1836. diff --git a/skills/openspec-update-change/SKILL.md b/skills/openspec-update-change/SKILL.md index 24c9f88367..57ce724666 100644 --- a/skills/openspec-update-change/SKILL.md +++ b/skills/openspec-update-change/SKILL.md @@ -56,13 +56,14 @@ Revise a change's existing planning artifacts and keep them coherent. Never edit 4. **Read and reconcile** - Read the artifact(s) the request touches and the change's other existing artifacts. - - Apply the requested edit. Then check every other existing artifact against it - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. + - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - Revise only files that already exist (`existingOutputPaths`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to `/openspec-continue-change` to create them. - - If the change is already coherent, say so and make no edits. + - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** - - Show each proposed revision and why. Write only after the user confirms. + - This step performs every write in this workflow; nothing earlier writes to disk. + - Show each proposed revision and why - including the requested edit drafted in step 4. Write only after the user confirms. - If the user rejects a revision, do not write it - leave that artifact unchanged. - When a substantial rewrite is needed, get that artifact's rules and template first: ```bash diff --git a/src/core/templates/workflows/update-change.ts b/src/core/templates/workflows/update-change.ts index 7700cd8d7e..32d74ec112 100644 --- a/src/core/templates/workflows/update-change.ts +++ b/src/core/templates/workflows/update-change.ts @@ -58,13 +58,14 @@ ${STORE_SELECTION_GUIDANCE} 4. **Read and reconcile** - Read the artifact(s) the request touches and the change's other existing artifacts. - - Apply the requested edit. Then check every other existing artifact against it - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. + - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - Revise only files that already exist (\`existingOutputPaths\`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to \`/opsx:continue\` to create them. - - If the change is already coherent, say so and make no edits. + - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** - - Show each proposed revision and why. Write only after the user confirms. + - This step performs every write in this workflow; nothing earlier writes to disk. + - Show each proposed revision and why - including the requested edit drafted in step 4. Write only after the user confirms. - If the user rejects a revision, do not write it - leave that artifact unchanged. - When a substantial rewrite is needed, get that artifact's rules and template first: \`\`\`bash @@ -149,13 +150,14 @@ ${STORE_SELECTION_GUIDANCE} 4. **Read and reconcile** - Read the artifact(s) the request touches and the change's other existing artifacts. - - Apply the requested edit. Then check every other existing artifact against it - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. + - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - Revise only files that already exist (\`existingOutputPaths\`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to \`/opsx:continue\` to create them. - - If the change is already coherent, say so and make no edits. + - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** - - Show each proposed revision and why. Write only after the user confirms. + - This step performs every write in this workflow; nothing earlier writes to disk. + - Show each proposed revision and why - including the requested edit drafted in step 4. Write only after the user confirms. - If the user rejects a revision, do not write it - leave that artifact unchanged. - When a substantial rewrite is needed, get that artifact's rules and template first: \`\`\`bash diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 4d2e717cae..7fbec2e66f 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -61,8 +61,8 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxProposeSkillTemplate: 'b7215583fefddae0127076465de9b3de9c230f2f1ea9ae6e4fb2a46fe510e8d6', getOpsxProposeCommandTemplate: 'f016c66c2b6115b459751154c76a6270e444d6aee31973bb7cb8c0e6d505fb98', getFeedbackSkillTemplate: 'dabeb5e825b9349abc8156c3e7b8608f27987912a6d9bf47ef29addde6138133', - getUpdateChangeSkillTemplate: '7dc8abc6f64c58bf34d7581ed4ab095a3b7a53cb372349bee2d840db58622819', - getOpsxUpdateCommandTemplate: 'e2388521b22f92f74561df9a0c2f98e1fa4d265af93b5ba26f42fb47a6c5bfed', + getUpdateChangeSkillTemplate: '7c6b19eb12c8182037b1804df5a8de1c26ee8dba31c0dbcd5fa3f19da0c35352', + getOpsxUpdateCommandTemplate: '7c6a8b89cbe333c7c707fe1943938d170d55cac01e060ad1e1616580cd332738', }; const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { @@ -77,7 +77,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-verify-change': 'af9be013dcbe8c6d8f6d9ab10c893fbd03f4c62933c384d82f63894dd0ceb84f', 'openspec-onboard': 'f6f59476acaf5e4d65dbb180da4cef62432612f3cecf207d471a951295e2003a', 'openspec-propose': '679d0f868bed23cfb34a8ecc6b4ba4ff7b88dd7dbaef91563423e98f194f988f', - 'openspec-update-change': '586547406aca94422dfeb3ffedce6c01049429b743f57ce829baa79ebc714d51', + 'openspec-update-change': '4a932cab1a4e5f6d1b57c21d1cd5e3c55197c728bfca66c844fc3b8528238c0e', }; // Intentionally excludes getFeedbackSkillTemplate: this list only models templates diff --git a/test/core/templates/update-change.test.ts b/test/core/templates/update-change.test.ts index cf68f234f5..3a6c5914de 100644 --- a/test/core/templates/update-change.test.ts +++ b/test/core/templates/update-change.test.ts @@ -16,6 +16,16 @@ const bodies: Array<[string, string]> = [ ['command', command.content], ]; +// Slice one numbered step out of a workflow body so an assertion about where a +// rule lives cannot be satisfied by the same words appearing in another step. +function section(body: string, startMarker: string, endMarker: string): string { + const start = body.indexOf(startMarker); + const end = body.indexOf(endMarker, start + startMarker.length); + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + return body.slice(start, end); +} + describe('update-change templates', () => { it('generates the expected skill and command shape (3.1)', () => { expect(skill.name).toBe('openspec-update-change'); @@ -99,6 +109,38 @@ describe('update-change templates', () => { } }); + // Regression for #1836: step 4 said "Apply the requested edit" while step 5 + // and the guardrails said to write only after the user confirms. "Apply" is a + // write verb in this very document - step 5 is titled "Confirm and apply" - + // so the same `/opsx:update "the design now uses X"` either wrote immediately + // or stopped and showed the revision first, depending on which passage the + // agent weighed. Step 4 now drafts; step 5 owns every write. + it('keeps step 4 read-only so step 5 owns every write (#1836)', () => { + for (const [label, body] of bodies) { + const stepFour = section(body, '4. **Read and reconcile**', '5. **Confirm and apply'); + const stepFive = section(body, '5. **Confirm and apply', '6. **Point to the next step'); + + // Step 4 states the edit is drafted, not written. + expect(stepFour, label).toContain('Draft the requested edit'); + expect(stepFour, label).toContain( + 'do not write anything yet - step 5 owns every write' + ); + + // No write verb may authorize a write inside step 4. "Apply"/"Write" are + // the words step 5 uses for the real thing. + expect(stepFour, label).not.toMatch(/\bAppl(y|ies|ied)\b/); + expect(stepFour, label).not.toMatch(/\bWrite\b/); + expect(stepFour, label).not.toContain('make no edits'); + + // Step 5 keeps the gate, and claims the writes explicitly. + expect(stepFive, label).toContain( + 'This step performs every write in this workflow; nothing earlier writes to disk' + ); + expect(stepFive, label).toContain('including the requested edit drafted in step 4'); + expect(stepFive, label).toContain('Write only after the user confirms'); + } + }); + it('confirms every edit and redirects intent changes to /opsx:new', () => { for (const [label, body] of bodies) { expect(body, label).toContain('Write only after the user confirms'); From 368a4606ec043ee7e4bc77d00b4034211816e20d Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 10 Sep 2026 12:35:41 -0500 Subject: [PATCH 2/6] fix(update-change): keep every edit verb out of the write-free step Adversarial review of the first commit found three gaps. Step 4 still opened a bullet with "Revise only files that already exist" - the same shape as the bug, an imperative edit verb inside the step that now declares it writes nothing. It reads as a scoping rule, but "revise" is the write verb everywhere else in this body ("proposed revision", "Which artifacts were revised"). It now says "Propose revisions only to files that already exist". The guard's `not.toMatch(/\bWrite\b/)` was inert and inverted: it was case-sensitive, so it never matched the wording it was meant to pin, and it could not be made case-insensitive because the fix's own text says "do not write anything yet". It now strips that one sanctioned sentence and rejects any remaining form of write or apply, case-insensitively - so lowercase "write the drafted edit now", the dangerous case, is caught. A second assertion rejects any step 4 bullet opening with Revise/Edit/Update/Rewrite. The changeset claimed no write verb could reappear in step 4, which was not what the old guard did. It now states what the guard checks. Also passes the surface label into section() so a marker drift names the surface that broke. Co-Authored-By: Claude Opus 5 --- .changeset/update-change-step-four-drafts.md | 2 +- skills/openspec-update-change/SKILL.md | 2 +- src/core/templates/workflows/update-change.ts | 4 +- .../templates/skill-templates-parity.test.ts | 6 +-- test/core/templates/update-change.test.ts | 54 ++++++++++++++----- 5 files changed, 48 insertions(+), 20 deletions(-) diff --git a/.changeset/update-change-step-four-drafts.md b/.changeset/update-change-step-four-drafts.md index f79b97c0ce..29c41ba589 100644 --- a/.changeset/update-change-step-four-drafts.md +++ b/.changeset/update-change-step-four-drafts.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -Resolve the contradiction that left `/opsx:update`'s only write path without a governing rule. Step 4 told the agent to "Apply the requested edit", while step 5 and the guardrails told it to write only after the user confirms each revision. "Apply" reads as a write verb in that very document — step 5 is titled "Confirm and apply" — so the same `/opsx:update "the design now uses X"` either wrote the edit immediately or stopped and showed the proposed revision first, depending on which passage the agent weighed. Since step 5 is the workflow's only write path, its confirmation guarantee was unenforceable whenever step 4 governed. Step 4 now drafts the edit and says explicitly that it writes nothing, and step 5 states that it performs every write in the workflow and shows the drafted edit for confirmation along with everything else. Both delivery surfaces and the committed skill carry the same wording, and a regression test slices step 4 out of each body so no write verb can reappear there. Fixes #1836. +Resolve the contradiction that left `/opsx:update`'s only write path without a governing rule. Step 4 told the agent to "Apply the requested edit", while step 5 and the guardrails told it to write only after the user confirms each revision. "Apply" reads as a write verb in that very document — step 5 is titled "Confirm and apply" — so the same `/opsx:update "the design now uses X"` either wrote the edit immediately or stopped and showed the proposed revision first, depending on which passage the agent weighed. Since step 5 is the workflow's only write path, its confirmation guarantee was unenforceable whenever step 4 governed. Step 4 now drafts the edit and says explicitly that it writes nothing, and step 5 states that it performs every write in the workflow and shows the drafted edit for confirmation along with everything else. Both delivery surfaces and the committed skill carry the same wording, and a regression test slices step 4 out of each body and rejects any form of "write" or "apply" left in it, so the old wording cannot silently return. Fixes #1836. diff --git a/skills/openspec-update-change/SKILL.md b/skills/openspec-update-change/SKILL.md index 57ce724666..78af419bcd 100644 --- a/skills/openspec-update-change/SKILL.md +++ b/skills/openspec-update-change/SKILL.md @@ -58,7 +58,7 @@ Revise a change's existing planning artifacts and keep them coherent. Never edit - Read the artifact(s) the request touches and the change's other existing artifacts. - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - - Revise only files that already exist (`existingOutputPaths`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to `/openspec-continue-change` to create them. + - Propose revisions only to files that already exist (`existingOutputPaths`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to `/openspec-continue-change` to create them. - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** diff --git a/src/core/templates/workflows/update-change.ts b/src/core/templates/workflows/update-change.ts index 32d74ec112..faac30d79a 100644 --- a/src/core/templates/workflows/update-change.ts +++ b/src/core/templates/workflows/update-change.ts @@ -60,7 +60,7 @@ ${STORE_SELECTION_GUIDANCE} - Read the artifact(s) the request touches and the change's other existing artifacts. - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - - Revise only files that already exist (\`existingOutputPaths\`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to \`/opsx:continue\` to create them. + - Propose revisions only to files that already exist (\`existingOutputPaths\`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to \`/opsx:continue\` to create them. - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** @@ -152,7 +152,7 @@ ${STORE_SELECTION_GUIDANCE} - Read the artifact(s) the request touches and the change's other existing artifacts. - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - - Revise only files that already exist (\`existingOutputPaths\`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to \`/opsx:continue\` to create them. + - Propose revisions only to files that already exist (\`existingOutputPaths\`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to \`/opsx:continue\` to create them. - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 7fbec2e66f..79e31a0f34 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -61,8 +61,8 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxProposeSkillTemplate: 'b7215583fefddae0127076465de9b3de9c230f2f1ea9ae6e4fb2a46fe510e8d6', getOpsxProposeCommandTemplate: 'f016c66c2b6115b459751154c76a6270e444d6aee31973bb7cb8c0e6d505fb98', getFeedbackSkillTemplate: 'dabeb5e825b9349abc8156c3e7b8608f27987912a6d9bf47ef29addde6138133', - getUpdateChangeSkillTemplate: '7c6b19eb12c8182037b1804df5a8de1c26ee8dba31c0dbcd5fa3f19da0c35352', - getOpsxUpdateCommandTemplate: '7c6a8b89cbe333c7c707fe1943938d170d55cac01e060ad1e1616580cd332738', + getUpdateChangeSkillTemplate: 'bee31e5b8760c985a9750284f6031e0b1c31eb2e47deef2e9f68d7a9e2b9092c', + getOpsxUpdateCommandTemplate: 'c6560162b8051aba756d9971b510fc9db750a632be6a3cdf5e12ed60ae685709', }; const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { @@ -77,7 +77,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-verify-change': 'af9be013dcbe8c6d8f6d9ab10c893fbd03f4c62933c384d82f63894dd0ceb84f', 'openspec-onboard': 'f6f59476acaf5e4d65dbb180da4cef62432612f3cecf207d471a951295e2003a', 'openspec-propose': '679d0f868bed23cfb34a8ecc6b4ba4ff7b88dd7dbaef91563423e98f194f988f', - 'openspec-update-change': '4a932cab1a4e5f6d1b57c21d1cd5e3c55197c728bfca66c844fc3b8528238c0e', + 'openspec-update-change': 'dbd4492c5ee34afa0e5456de282467b73ba28bf2adf4b74624ef210f18ed2871', }; // Intentionally excludes getFeedbackSkillTemplate: this list only models templates diff --git a/test/core/templates/update-change.test.ts b/test/core/templates/update-change.test.ts index 3a6c5914de..ad1436c251 100644 --- a/test/core/templates/update-change.test.ts +++ b/test/core/templates/update-change.test.ts @@ -11,6 +11,11 @@ const command = getOpsxUpdateCommandTemplate(); // Both delivery surfaces must carry the same contract; every behavioral // assertion below runs against each body. +// The one sentence in step 4 that is allowed to say "write" - it is what hands +// every write to step 5. +const SANCTIONED_WRITE_MENTION = + 'do not write anything yet - step 5 owns every write'; + const bodies: Array<[string, string]> = [ ['skill', skill.instructions], ['command', command.content], @@ -18,11 +23,16 @@ const bodies: Array<[string, string]> = [ // Slice one numbered step out of a workflow body so an assertion about where a // rule lives cannot be satisfied by the same words appearing in another step. -function section(body: string, startMarker: string, endMarker: string): string { +function section( + body: string, + startMarker: string, + endMarker: string, + label: string +): string { const start = body.indexOf(startMarker); const end = body.indexOf(endMarker, start + startMarker.length); - expect(start).toBeGreaterThanOrEqual(0); - expect(end).toBeGreaterThan(start); + expect(start, label).toBeGreaterThanOrEqual(0); + expect(end, label).toBeGreaterThan(start); return body.slice(start, end); } @@ -117,21 +127,39 @@ describe('update-change templates', () => { // agent weighed. Step 4 now drafts; step 5 owns every write. it('keeps step 4 read-only so step 5 owns every write (#1836)', () => { for (const [label, body] of bodies) { - const stepFour = section(body, '4. **Read and reconcile**', '5. **Confirm and apply'); - const stepFive = section(body, '5. **Confirm and apply', '6. **Point to the next step'); + const stepFour = section( + body, + '4. **Read and reconcile**', + '5. **Confirm and apply', + label + ); + const stepFive = section( + body, + '5. **Confirm and apply', + '6. **Point to the next step', + label + ); // Step 4 states the edit is drafted, not written. expect(stepFour, label).toContain('Draft the requested edit'); - expect(stepFour, label).toContain( - 'do not write anything yet - step 5 owns every write' - ); - - // No write verb may authorize a write inside step 4. "Apply"/"Write" are - // the words step 5 uses for the real thing. - expect(stepFour, label).not.toMatch(/\bAppl(y|ies|ied)\b/); - expect(stepFour, label).not.toMatch(/\bWrite\b/); + expect(stepFour, label).toContain(SANCTIONED_WRITE_MENTION); + + // Step 4 is declared write-free, so the ONLY write/apply words it may + // carry are the ones in the sentence handing writing to step 5. Strip + // that sanctioned sentence and nothing of the kind may remain. The match + // is case-insensitive on purpose: a lowercase `write the drafted edit + // now` is the dangerous regression, and a case-sensitive `\bWrite\b` + // would miss exactly that while tripping on harmless capitalized prose. + const residue = stepFour.replace(SANCTIONED_WRITE_MENTION, ''); + expect(residue, label).not.toMatch(/\bwrit(e|es|ing|ten)\b/i); + expect(residue, label).not.toMatch(/\bappl(y|ies|ied|ying)\b/i); expect(stepFour, label).not.toContain('make no edits'); + // No bullet in step 4 may open with an imperative edit verb: "Revise the + // files ..." reads as the instruction to edit them, which is the shape + // this whole guard exists to keep out. + expect(stepFour, label).not.toMatch(/^\s*-\s*(Revise|Edit|Update|Rewrite)\b/m); + // Step 5 keeps the gate, and claims the writes explicitly. expect(stepFive, label).toContain( 'This step performs every write in this workflow; nothing earlier writes to disk' From 11fc037b889744efb473cfaa805a8993004e9410 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 10 Sep 2026 12:52:53 -0500 Subject: [PATCH 3/6] fix(update-change): let no passage outside step 5 authorize a write Mutation-testing the first guard found it defeatable: 20 of 25 mutations broke the #1836 contract and still passed. The two worst were structural, not lexical - the guard only sliced steps 4 and 5, so an authorization placed in the intro, in step 3, in the Guardrails or in the Output section governed the agent while no assertion ever saw it; and step 5 carried only positive assertions, so its gate could be kept and then exempted in the next sentence, or the whole-body confirmation guardrail deleted outright, with nothing failing. The guard now pins step 4's draft rule and the whole of step 5 verbatim, requires the whole-body guardrail to survive, and scans every other passage for write verbs and their synonyms (commit/save/persist/ overwrite/reapply/flush/emit), verb-free equivalents (perform, carry out, in place, to disk), consent-bypass phrasing, and imperative edit bullets including ones led by an adverb. All 22 mutations are now caught. Two wording corrections came out of the same review. "nothing earlier writes to disk" was false - every openspec invocation persists a telemetry id via the root preAction hook - so step 5 now claims every artifact write instead. Step 4 says "in the conversation, not in files", borrowing explore.ts's phrasing, so "draft" cannot be read as writing a draft file. docs/commands.md carried the same apply-vs-confirm collision two lines above the confirmation bullet, contradicting the worked example directly below it; it now says "Drafts your requested revision". Co-Authored-By: Claude Opus 5 --- .changeset/update-change-step-four-drafts.md | 2 +- docs/commands.md | 2 +- skills/openspec-update-change/SKILL.md | 4 +- src/core/templates/workflows/update-change.ts | 8 +- .../templates/skill-templates-parity.test.ts | 6 +- test/core/templates/update-change.test.ts | 174 ++++++++++++------ 6 files changed, 126 insertions(+), 70 deletions(-) diff --git a/.changeset/update-change-step-four-drafts.md b/.changeset/update-change-step-four-drafts.md index 29c41ba589..988d58d906 100644 --- a/.changeset/update-change-step-four-drafts.md +++ b/.changeset/update-change-step-four-drafts.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -Resolve the contradiction that left `/opsx:update`'s only write path without a governing rule. Step 4 told the agent to "Apply the requested edit", while step 5 and the guardrails told it to write only after the user confirms each revision. "Apply" reads as a write verb in that very document — step 5 is titled "Confirm and apply" — so the same `/opsx:update "the design now uses X"` either wrote the edit immediately or stopped and showed the proposed revision first, depending on which passage the agent weighed. Since step 5 is the workflow's only write path, its confirmation guarantee was unenforceable whenever step 4 governed. Step 4 now drafts the edit and says explicitly that it writes nothing, and step 5 states that it performs every write in the workflow and shows the drafted edit for confirmation along with everything else. Both delivery surfaces and the committed skill carry the same wording, and a regression test slices step 4 out of each body and rejects any form of "write" or "apply" left in it, so the old wording cannot silently return. Fixes #1836. +Resolve the contradiction that left `/opsx:update`'s only write path without a governing rule. Step 4 told the agent to "Apply the requested edit", while step 5 and the guardrails told it to write only after the user confirms each revision — so the same `/opsx:update "the design now uses X"` either wrote immediately or stopped and showed the proposed revision first, depending on which passage the agent weighed. Step 4 now drafts the edit in the conversation and step 5 owns every artifact write, matching the workflow's own specified behavior: propose each revision and apply it only after user confirmation. Fixes #1836. diff --git a/docs/commands.md b/docs/commands.md index 7546dae82d..229b0626e3 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -342,7 +342,7 @@ Revise a change's existing planning artifacts and keep them coherent with one an **What it does:** - Reads the change's artifacts via `openspec status --change --json` -- Applies your requested revision, or reviews the artifacts for contradictions if you didn't name one +- Drafts your requested revision, or reviews the artifacts for contradictions if you didn't name one - Reconciles the other existing artifacts in any direction (a design edit may ripple back to the proposal) - Confirms every edit with you before writing, one artifact at a time - Ends by recommending the next step: `/opsx:continue` (artifacts missing), `/opsx:apply` (carry a revised plan into code), or `/opsx:archive` (all done) diff --git a/skills/openspec-update-change/SKILL.md b/skills/openspec-update-change/SKILL.md index 78af419bcd..24e0c7a688 100644 --- a/skills/openspec-update-change/SKILL.md +++ b/skills/openspec-update-change/SKILL.md @@ -56,13 +56,13 @@ Revise a change's existing planning artifacts and keep them coherent. Never edit 4. **Read and reconcile** - Read the artifact(s) the request touches and the change's other existing artifacts. - - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. + - Draft the requested edit in the conversation, not in files. Work out exactly what it changes; step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - Propose revisions only to files that already exist (`existingOutputPaths`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to `/openspec-continue-change` to create them. - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** - - This step performs every write in this workflow; nothing earlier writes to disk. + - This step performs every artifact write in this workflow; no earlier step edits an artifact. - Show each proposed revision and why - including the requested edit drafted in step 4. Write only after the user confirms. - If the user rejects a revision, do not write it - leave that artifact unchanged. - When a substantial rewrite is needed, get that artifact's rules and template first: diff --git a/src/core/templates/workflows/update-change.ts b/src/core/templates/workflows/update-change.ts index faac30d79a..69e09e7c73 100644 --- a/src/core/templates/workflows/update-change.ts +++ b/src/core/templates/workflows/update-change.ts @@ -58,13 +58,13 @@ ${STORE_SELECTION_GUIDANCE} 4. **Read and reconcile** - Read the artifact(s) the request touches and the change's other existing artifacts. - - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. + - Draft the requested edit in the conversation, not in files. Work out exactly what it changes; step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - Propose revisions only to files that already exist (\`existingOutputPaths\`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to \`/opsx:continue\` to create them. - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** - - This step performs every write in this workflow; nothing earlier writes to disk. + - This step performs every artifact write in this workflow; no earlier step edits an artifact. - Show each proposed revision and why - including the requested edit drafted in step 4. Write only after the user confirms. - If the user rejects a revision, do not write it - leave that artifact unchanged. - When a substantial rewrite is needed, get that artifact's rules and template first: @@ -150,13 +150,13 @@ ${STORE_SELECTION_GUIDANCE} 4. **Read and reconcile** - Read the artifact(s) the request touches and the change's other existing artifacts. - - Draft the requested edit. Work out exactly what it changes, but do not write anything yet - step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. + - Draft the requested edit in the conversation, not in files. Work out exactly what it changes; step 5 owns every write. Then check every other existing artifact against the drafted edit - in ANY direction: an edit to a later artifact may require revising an earlier one, not only the other way around. Build order is a useful reading order, not a constraint on which artifacts may be revised. - Note everything that is now inconsistent, missing, or contradictory. - Propose revisions only to files that already exist (\`existingOutputPaths\`). Do NOT create artifacts that don't exist yet, and do NOT invent new files under a glob artifact - note them and point the user to \`/opsx:continue\` to create them. - If the change is already coherent, say so and propose no revisions. 5. **Confirm and apply, one artifact at a time** - - This step performs every write in this workflow; nothing earlier writes to disk. + - This step performs every artifact write in this workflow; no earlier step edits an artifact. - Show each proposed revision and why - including the requested edit drafted in step 4. Write only after the user confirms. - If the user rejects a revision, do not write it - leave that artifact unchanged. - When a substantial rewrite is needed, get that artifact's rules and template first: diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 79e31a0f34..b25c4a5460 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -61,8 +61,8 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxProposeSkillTemplate: 'b7215583fefddae0127076465de9b3de9c230f2f1ea9ae6e4fb2a46fe510e8d6', getOpsxProposeCommandTemplate: 'f016c66c2b6115b459751154c76a6270e444d6aee31973bb7cb8c0e6d505fb98', getFeedbackSkillTemplate: 'dabeb5e825b9349abc8156c3e7b8608f27987912a6d9bf47ef29addde6138133', - getUpdateChangeSkillTemplate: 'bee31e5b8760c985a9750284f6031e0b1c31eb2e47deef2e9f68d7a9e2b9092c', - getOpsxUpdateCommandTemplate: 'c6560162b8051aba756d9971b510fc9db750a632be6a3cdf5e12ed60ae685709', + getUpdateChangeSkillTemplate: '968e4164ce38258fdab858bbe65ff3f2300b0174a9c35194dc6cacafe4626f61', + getOpsxUpdateCommandTemplate: 'fc3b2ba3977a63e9f7689fef2ba05bb788f909db312818a4844867aefa6837d5', }; const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { @@ -77,7 +77,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-verify-change': 'af9be013dcbe8c6d8f6d9ab10c893fbd03f4c62933c384d82f63894dd0ceb84f', 'openspec-onboard': 'f6f59476acaf5e4d65dbb180da4cef62432612f3cecf207d471a951295e2003a', 'openspec-propose': '679d0f868bed23cfb34a8ecc6b4ba4ff7b88dd7dbaef91563423e98f194f988f', - 'openspec-update-change': 'dbd4492c5ee34afa0e5456de282467b73ba28bf2adf4b74624ef210f18ed2871', + 'openspec-update-change': '832fc53b29546f70dc5b70047864a118f2b84ad86456dcad835ce3e21094811f', }; // Intentionally excludes getFeedbackSkillTemplate: this list only models templates diff --git a/test/core/templates/update-change.test.ts b/test/core/templates/update-change.test.ts index ad1436c251..14085b8335 100644 --- a/test/core/templates/update-change.test.ts +++ b/test/core/templates/update-change.test.ts @@ -11,18 +11,62 @@ const command = getOpsxUpdateCommandTemplate(); // Both delivery surfaces must carry the same contract; every behavioral // assertion below runs against each body. -// The one sentence in step 4 that is allowed to say "write" - it is what hands -// every write to step 5. -const SANCTIONED_WRITE_MENTION = - 'do not write anything yet - step 5 owns every write'; - const bodies: Array<[string, string]> = [ ['skill', skill.instructions], ['command', command.content], ]; -// Slice one numbered step out of a workflow body so an assertion about where a -// rule lives cannot be satisfied by the same words appearing in another step. +// The load-bearing sentence of step 4 and the whole of step 5 are pinned +// verbatim. #1836 happened because a single verb ("Apply") in step 4 silently +// re-answered a question step 5 had already answered, so any reword of either +// passage has to come back through this test and re-argue the contract rather +// than just regenerate a parity hash. +const STEP_FOUR_DRAFT_RULE = + ' - Draft the requested edit in the conversation, not in files. Work out exactly what it changes; step 5 owns every write.'; + +const STEP_FIVE = `5. **Confirm and apply, one artifact at a time** + - This step performs every artifact write in this workflow; no earlier step edits an artifact. + - Show each proposed revision and why - including the requested edit drafted in step 4. Write only after the user confirms. + - If the user rejects a revision, do not write it - leave that artifact unchanged. + - When a substantial rewrite is needed, get that artifact's rules and template first: + \`\`\`bash + openspec instructions "" --change "" --json + \`\`\` + +`; + +// Every mention of writing or applying allowed to live OUTSIDE step 5. Each is +// a scope rule, a hand-off to another workflow, or the gate itself - none +// authorizes a write here. +const SANCTIONED_OUTSIDE_STEP_FIVE = [ + 'step 5 owns every write', + 'Do NOT write to `resolvedOutputPath`', + '- Edit only the concrete files in `existingOutputPaths`; never write to a glob `resolvedOutputPath`.', + 'Confirm every edit with the user before writing.', + '`/opsx:apply`', + 'already applied', +]; + +// Synonyms matter as much as the original verb: "commit the edit", "overwrite +// the artifact", "reapply it" all reintroduce #1836 while dodging a naive +// /\bwrite\b/. No leading \b, so over-/re- prefixed forms are caught too. +const WRITE_VERB = + /(?:over|re)?writ(?:e|es|ing|ten)\b|(?:re)?appl(?:y|ies|ied|ying)\b|\b(?:commit|commits|committing|save|saves|saving|persist|persists|persisting|flush|flushes|flushing|emit|emits|emitting)\b/i; + +// Verb-free ways to say the same thing: "perform the edit", "put it in place", +// "carry it out", anything "to disk". Step 5 is the only passage entitled to +// this vocabulary, and it is excluded before these run. +const WRITE_PHRASE = + /\bperform(?:s|ed|ing)?\b|\bcarr(?:y|ies|ied|ying) out\b|\bin place\b|\bto disk\b/i; + +// An authorization needs no write verb at all - "do it now, without asking" is +// enough. There is no legitimate use of this phrasing in this workflow. +const CONSENT_BYPASS = + /without (?:asking|confirming|confirmation)|do not wait for confirmation|no confirmation (?:is )?(?:needed|required)/i; + +// Slice one region out of a workflow body so an assertion about where a rule +// lives cannot be satisfied by the same words appearing somewhere else. The +// label names the marker, so a renamed heading reports which one went missing. function section( body: string, startMarker: string, @@ -31,11 +75,73 @@ function section( ): string { const start = body.indexOf(startMarker); const end = body.indexOf(endMarker, start + startMarker.length); - expect(start, label).toBeGreaterThanOrEqual(0); - expect(end, label).toBeGreaterThan(start); + expect(start, `${label}: missing marker ${startMarker}`).toBeGreaterThanOrEqual(0); + expect(end, `${label}: missing marker ${endMarker}`).toBeGreaterThan(start); return body.slice(start, end); } +function stepFive(body: string, label: string): string { + return section(body, '5. **Confirm and apply', '6. **Point to the next step', `${label} step 5`); +} + +// Regression for #1836: step 4 said "Apply the requested edit" while step 5 and +// the guardrails said to write only after the user confirms. "Apply" is a write +// verb in this very document - step 5 is titled "Confirm and apply" - so the +// same `/opsx:update "the design now uses X"` either wrote immediately or +// stopped and showed the revision first, depending on which passage the agent +// weighed. Step 5 is the workflow's only gated write path, so its confirmation +// guarantee was unenforceable whenever step 4 governed. +describe('update-change write gate (#1836)', () => { + it('pins the step 4 draft rule and the whole of step 5', () => { + for (const [label, body] of bodies) { + const stepFour = section( + body, + '4. **Read and reconcile**', + '5. **Confirm and apply', + `${label} step 4` + ); + + expect(stepFour, `${label} step 4`).toContain(STEP_FOUR_DRAFT_RULE); + // Verbatim, because an exemption bolted onto the gate ("this does not + // apply to the requested edit") is invisible to any toContain check. + expect(stepFive(body, label), `${label} step 5`).toBe(STEP_FIVE); + } + }); + + it('keeps the whole-body confirmation guardrail', () => { + for (const [label, body] of bodies) { + // Deleting this one line used to break nothing. + expect(body, label).toContain('Confirm every edit with the user before writing.'); + } + }); + + it('lets no passage outside step 5 authorize a write', () => { + for (const [label, body] of bodies) { + // Everything the agent reads except step 5 and the shared store preamble. + // #1836 lived in step 4, but a sentence in the intro, in step 3, in the + // Guardrails or in the Output section would govern the agent just as well + // while sitting outside any single-step slice. + let rest = body + .split(stepFive(body, label)) + .join('\n') + .split(STORE_SELECTION_GUIDANCE) + .join(''); + for (const sanctioned of SANCTIONED_OUTSIDE_STEP_FIVE) { + rest = rest.split(sanctioned).join(''); + } + + expect(rest, `${label}: only step 5 may instruct a write`).not.toMatch(WRITE_VERB); + expect(rest, `${label}: only step 5 may instruct a write`).not.toMatch(WRITE_PHRASE); + expect(rest, `${label}: nothing may waive the confirmation`).not.toMatch(CONSENT_BYPASS); + // A leading adverb ("Immediately revise the files ...") must not disarm + // this - the verb does not have to be the bullet's first token. + expect(rest, `${label}: no imperative edit bullet outside step 5`).not.toMatch( + /^\s*-\s*(?:\w+ly,?\s+)?(?:Revise|Edit|Update|Rewrite)\b/im + ); + } + }); +}); + describe('update-change templates', () => { it('generates the expected skill and command shape (3.1)', () => { expect(skill.name).toBe('openspec-update-change'); @@ -119,56 +225,6 @@ describe('update-change templates', () => { } }); - // Regression for #1836: step 4 said "Apply the requested edit" while step 5 - // and the guardrails said to write only after the user confirms. "Apply" is a - // write verb in this very document - step 5 is titled "Confirm and apply" - - // so the same `/opsx:update "the design now uses X"` either wrote immediately - // or stopped and showed the revision first, depending on which passage the - // agent weighed. Step 4 now drafts; step 5 owns every write. - it('keeps step 4 read-only so step 5 owns every write (#1836)', () => { - for (const [label, body] of bodies) { - const stepFour = section( - body, - '4. **Read and reconcile**', - '5. **Confirm and apply', - label - ); - const stepFive = section( - body, - '5. **Confirm and apply', - '6. **Point to the next step', - label - ); - - // Step 4 states the edit is drafted, not written. - expect(stepFour, label).toContain('Draft the requested edit'); - expect(stepFour, label).toContain(SANCTIONED_WRITE_MENTION); - - // Step 4 is declared write-free, so the ONLY write/apply words it may - // carry are the ones in the sentence handing writing to step 5. Strip - // that sanctioned sentence and nothing of the kind may remain. The match - // is case-insensitive on purpose: a lowercase `write the drafted edit - // now` is the dangerous regression, and a case-sensitive `\bWrite\b` - // would miss exactly that while tripping on harmless capitalized prose. - const residue = stepFour.replace(SANCTIONED_WRITE_MENTION, ''); - expect(residue, label).not.toMatch(/\bwrit(e|es|ing|ten)\b/i); - expect(residue, label).not.toMatch(/\bappl(y|ies|ied|ying)\b/i); - expect(stepFour, label).not.toContain('make no edits'); - - // No bullet in step 4 may open with an imperative edit verb: "Revise the - // files ..." reads as the instruction to edit them, which is the shape - // this whole guard exists to keep out. - expect(stepFour, label).not.toMatch(/^\s*-\s*(Revise|Edit|Update|Rewrite)\b/m); - - // Step 5 keeps the gate, and claims the writes explicitly. - expect(stepFive, label).toContain( - 'This step performs every write in this workflow; nothing earlier writes to disk' - ); - expect(stepFive, label).toContain('including the requested edit drafted in step 4'); - expect(stepFive, label).toContain('Write only after the user confirms'); - } - }); - it('confirms every edit and redirects intent changes to /opsx:new', () => { for (const [label, body] of bodies) { expect(body, label).toContain('Write only after the user confirms'); From 973dc15eb5d94e4bde397e48492c850846398d52 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 10 Sep 2026 13:01:51 -0500 Subject: [PATCH 4/6] test(update-change): catch more imperative edit verbs outside step 5 CodeRabbit noted `- Modify the artifact now` slipped past the imperative-bullet guard. Added Modify, Amend, Patch and Replace, each verified to trip the guard. `Change` is deliberately excluded: step 6 already opens a bullet with "Change already implemented ...". Co-Authored-By: Claude Opus 5 --- test/core/templates/update-change.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/templates/update-change.test.ts b/test/core/templates/update-change.test.ts index 14085b8335..97ce6368a6 100644 --- a/test/core/templates/update-change.test.ts +++ b/test/core/templates/update-change.test.ts @@ -136,7 +136,7 @@ describe('update-change write gate (#1836)', () => { // A leading adverb ("Immediately revise the files ...") must not disarm // this - the verb does not have to be the bullet's first token. expect(rest, `${label}: no imperative edit bullet outside step 5`).not.toMatch( - /^\s*-\s*(?:\w+ly,?\s+)?(?:Revise|Edit|Update|Rewrite)\b/im + /^\s*-\s*(?:\w+ly,?\s+)?(?:Revise|Edit|Update|Rewrite|Modify|Amend|Patch|Replace)\b/im ); } }); From d9ab719af29569d6c3f3ac6b820fa80882ed617c Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 15 Sep 2026 07:57:21 -0500 Subject: [PATCH 5/6] test(update-change): prove the write-gate scan trips in every section The outside-step-5 scan was only ever checked against the real body, so nothing showed it could fail. Injecting a verb-free authorization into the intro, Input, steps 1-2, Output or Guardrails passed on both surfaces (7 of 10 sections). The bare "already applied" allowlist entry also erased "treat the requested edit as already applied" before any check ran. - Move the scan into a function and add a mutation table: one injected authorization per section, asserted on skill and command bodies. - Spell every sanctioned mention in full context. - Flag any mention of the requested edit outside the pinned draft rule. - Widen the consent-bypass filter (needs no / exempt from / skip confirm). Also revert the legacy docs/commands.md edit: docs-lab reference/skills.md is the published page and already states the confirm-then-write contract. Co-Authored-By: Claude Opus 5 --- docs/commands.md | 2 +- test/core/templates/update-change.test.ts | 96 +++++++++++++++++------ 2 files changed, 72 insertions(+), 26 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 229b0626e3..7546dae82d 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -342,7 +342,7 @@ Revise a change's existing planning artifacts and keep them coherent with one an **What it does:** - Reads the change's artifacts via `openspec status --change --json` -- Drafts your requested revision, or reviews the artifacts for contradictions if you didn't name one +- Applies your requested revision, or reviews the artifacts for contradictions if you didn't name one - Reconciles the other existing artifacts in any direction (a design edit may ripple back to the proposal) - Confirms every edit with you before writing, one artifact at a time - Ends by recommending the next step: `/opsx:continue` (artifacts missing), `/opsx:apply` (carry a revised plan into code), or `/opsx:archive` (all done) diff --git a/test/core/templates/update-change.test.ts b/test/core/templates/update-change.test.ts index 97ce6368a6..9571029603 100644 --- a/test/core/templates/update-change.test.ts +++ b/test/core/templates/update-change.test.ts @@ -37,16 +37,26 @@ const STEP_FIVE = `5. **Confirm and apply, one artifact at a time** // Every mention of writing or applying allowed to live OUTSIDE step 5. Each is // a scope rule, a hand-off to another workflow, or the gate itself - none -// authorizes a write here. +// authorizes a write here. Each is spelled in full context: a bare fragment +// such as "already applied" would also erase "treat the requested edit as +// already applied" before any check could see it. const SANCTIONED_OUTSIDE_STEP_FIVE = [ - 'step 5 owns every write', + STEP_FOUR_DRAFT_RULE, + 'that is the starting edit.', 'Do NOT write to `resolvedOutputPath`', '- Edit only the concrete files in `existingOutputPaths`; never write to a glob `resolvedOutputPath`.', 'Confirm every edit with the user before writing.', '`/opsx:apply`', - 'already applied', + '(tasks checked off / already applied)', ]; +// Authorizations need not share any vocabulary with writing ("land the +// requested edit", "it goes straight into the file"), but they must name what +// they authorize. Outside the pinned draft rule and step 3's framing, nothing +// may talk about the requested edit at all. +const REQUESTED_EDIT = + /\brequested (?:edit|revision|change)|\buser's (?:edit|revision|change)|\bstarting edit\b/i; + // Synonyms matter as much as the original verb: "commit the edit", "overwrite // the artifact", "reapply it" all reintroduce #1836 while dodging a naive // /\bwrite\b/. No leading \b, so over-/re- prefixed forms are caught too. @@ -62,7 +72,7 @@ const WRITE_PHRASE = // An authorization needs no write verb at all - "do it now, without asking" is // enough. There is no legitimate use of this phrasing in this workflow. const CONSENT_BYPASS = - /without (?:asking|confirming|confirmation)|do not wait for confirmation|no confirmation (?:is )?(?:needed|required)/i; + /without (?:asking|confirming|confirmation)|do not wait for confirmation|no confirmation (?:is )?(?:needed|required)|needs? no confirm|exempt from (?:the )?confirm|skip(?:s|ping)? (?:the )?confirm/i; // Slice one region out of a workflow body so an assertion about where a rule // lives cannot be satisfied by the same words appearing somewhere else. The @@ -84,6 +94,35 @@ function stepFive(body: string, label: string): string { return section(body, '5. **Confirm and apply', '6. **Point to the next step', `${label} step 5`); } +// Everything the agent reads except step 5 and the shared store preamble. +// #1836 lived in step 4, but a sentence in the intro, in step 3, in the +// Guardrails or in the Output section would govern the agent just as well +// while sitting outside any single-step slice. Returns the checks that tripped. +function writeAuthorizationsOutsideStepFive(body: string, label: string): string[] { + let rest = body + .split(stepFive(body, label)) + .join('\n') + .split(STORE_SELECTION_GUIDANCE) + .join(''); + for (const sanctioned of SANCTIONED_OUTSIDE_STEP_FIVE) { + rest = rest.split(sanctioned).join(''); + } + + const checks: Array<[string, RegExp]> = [ + ['write verb', WRITE_VERB], + ['write phrase', WRITE_PHRASE], + ['consent bypass', CONSENT_BYPASS], + ['names the requested edit', REQUESTED_EDIT], + // A leading adverb ("Immediately revise the files ...") must not disarm + // this - the verb does not have to be the bullet's first token. + [ + 'imperative edit bullet', + /^\s*-\s*(?:\w+ly,?\s+)?(?:Revise|Edit|Update|Rewrite|Modify|Amend|Patch|Replace)\b/im, + ], + ]; + return checks.filter(([, pattern]) => pattern.test(rest)).map(([name]) => name); +} + // Regression for #1836: step 4 said "Apply the requested edit" while step 5 and // the guardrails said to write only after the user confirms. "Apply" is a write // verb in this very document - step 5 is titled "Confirm and apply" - so the @@ -117,27 +156,34 @@ describe('update-change write gate (#1836)', () => { it('lets no passage outside step 5 authorize a write', () => { for (const [label, body] of bodies) { - // Everything the agent reads except step 5 and the shared store preamble. - // #1836 lived in step 4, but a sentence in the intro, in step 3, in the - // Guardrails or in the Output section would govern the agent just as well - // while sitting outside any single-step slice. - let rest = body - .split(stepFive(body, label)) - .join('\n') - .split(STORE_SELECTION_GUIDANCE) - .join(''); - for (const sanctioned of SANCTIONED_OUTSIDE_STEP_FIVE) { - rest = rest.split(sanctioned).join(''); - } - - expect(rest, `${label}: only step 5 may instruct a write`).not.toMatch(WRITE_VERB); - expect(rest, `${label}: only step 5 may instruct a write`).not.toMatch(WRITE_PHRASE); - expect(rest, `${label}: nothing may waive the confirmation`).not.toMatch(CONSENT_BYPASS); - // A leading adverb ("Immediately revise the files ...") must not disarm - // this - the verb does not have to be the bullet's first token. - expect(rest, `${label}: no imperative edit bullet outside step 5`).not.toMatch( - /^\s*-\s*(?:\w+ly,?\s+)?(?:Revise|Edit|Update|Rewrite|Modify|Amend|Patch|Replace)\b/im - ); + expect(writeAuthorizationsOutsideStepFive(body, label), label).toEqual([]); + } + }); + + // The guard above only proves something if it trips. Each line goes into a + // different section of each body (intro, Input, steps 1-4 and 6, Output, + // Guardrails); every one reintroduces #1836 and must be flagged. + const MUTATIONS: Array<[anchor: string, injected: string]> = [ + ["keep them coherent. Never edit code.", 'Land the requested edit right away.'], + ['**Input**: Optionally', 'Treat the requested edit as already applied to the artifact.'], + ['1. **Select the change**', ' - Put the requested edit into the artifact now.'], + ["2. **Get the change's artifacts**", ' The requested edit goes straight into the file.'], + ['3. **Understand the request**', ' - Apply the requested edit immediately.'], + ['4. **Read and reconcile**', ' - Update the artifact with the requested edit now.'], + ['6. **Point to the next step', ' - Save the revisions first.'], + ['**Output**', '- The requested edit, already applied during step 4'], + ['**Guardrails**', '- The requested edit is exempt from confirmation.'], + ['- Confirm every edit with the user before writing.', '- The user\'s revision needs no confirmation.'], + ]; + + it.each(MUTATIONS)('flags a write authorization injected after %s', (anchor, injected) => { + for (const [label, body] of bodies) { + const at = body.indexOf('\n', body.indexOf(anchor)); + expect(body.indexOf(anchor), `${label}: missing anchor`).toBeGreaterThanOrEqual(0); + const mutated = `${body.slice(0, at + 1)}${injected}\n${body.slice(at + 1)}`; + // Still passes the step 5 pin, so only the outside-step-5 scan can catch it. + expect(stepFive(mutated, label), label).toBe(STEP_FIVE); + expect(writeAuthorizationsOutsideStepFive(mutated, label), label).not.toEqual([]); } }); }); From e816eee5f58e03845d876f758d2872b42a070a7f Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 15 Sep 2026 08:22:18 -0500 Subject: [PATCH 6/6] docs(changeset): drop em dashes from the release note Co-Authored-By: Claude Opus 5 --- .changeset/update-change-step-four-drafts.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/update-change-step-four-drafts.md b/.changeset/update-change-step-four-drafts.md index 988d58d906..1d74daa99f 100644 --- a/.changeset/update-change-step-four-drafts.md +++ b/.changeset/update-change-step-four-drafts.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -Resolve the contradiction that left `/opsx:update`'s only write path without a governing rule. Step 4 told the agent to "Apply the requested edit", while step 5 and the guardrails told it to write only after the user confirms each revision — so the same `/opsx:update "the design now uses X"` either wrote immediately or stopped and showed the proposed revision first, depending on which passage the agent weighed. Step 4 now drafts the edit in the conversation and step 5 owns every artifact write, matching the workflow's own specified behavior: propose each revision and apply it only after user confirmation. Fixes #1836. +Resolve the contradiction that left `/opsx:update`'s only write path without a governing rule. Step 4 told the agent to "Apply the requested edit", while step 5 and the guardrails told it to write only after the user confirms each revision, so the same `/opsx:update "the design now uses X"` either wrote immediately or stopped and showed the proposed revision first, depending on which passage the agent weighed. Step 4 now drafts the edit in the conversation and step 5 owns every artifact write, matching the workflow's own specified behavior: propose each revision and apply it only after user confirmation. Fixes #1836.