From e6b7e4cc3abb06352b2da694a6d56321db68a613 Mon Sep 17 00:00:00 2001 From: choi138 Date: Fri, 11 Sep 2026 10:29:54 +0900 Subject: [PATCH] fix(update-change): draft the requested edit in step 4, write in step 5 Step 4 said "Apply the requested edit" while step 5 said "Write only after the user confirms" and the guardrails said "Confirm every edit with the user before writing", with nothing stating which governs. "Apply" reads as a write verb in this document: step 5 is itself titled "Confirm and apply", and step 4's closing bullet ("say so and make no edits") only parses if step 4 is the editing stage. So `/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 write path, so its confirmation guarantee was unenforceable whenever step 4 governed. Step 4 now drafts and step 5 still owns the write. Both delivery surfaces and the committed skill carry the same wording, and the contract is pinned by tests on each surface. Fixes #1836 --- .changeset/update-change-step-4-drafts.md | 5 ++++ 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 | 30 +++++++++++++++++++ 5 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 .changeset/update-change-step-4-drafts.md diff --git a/.changeset/update-change-step-4-drafts.md b/.changeset/update-change-step-4-drafts.md new file mode 100644 index 0000000000..1edfec8a32 --- /dev/null +++ b/.changeset/update-change-step-4-drafts.md @@ -0,0 +1,5 @@ +--- +'@fission-ai/openspec': patch +--- + +Fix the update-change workflow contradicting its own confirmation gate. Step 4 said to apply the requested edit while step 5 said to write only after the user confirms, and "apply" reads as a write verb in that document - step 5 is itself titled "Confirm and apply". Step 4 now drafts the edit and step 5 remains the only write path, so `/opsx:update "the design now uses X"` always shows the revision before writing it. Both delivery surfaces carry the same wording and it is now pinned by tests. diff --git a/skills/openspec-update-change/SKILL.md b/skills/openspec-update-change/SKILL.md index 24c9f88367..b1e813a587 100644 --- a/skills/openspec-update-change/SKILL.md +++ b/skills/openspec-update-change/SKILL.md @@ -56,7 +56,7 @@ 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. 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. - 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. diff --git a/src/core/templates/workflows/update-change.ts b/src/core/templates/workflows/update-change.ts index 7700cd8d7e..fa62fcf6a0 100644 --- a/src/core/templates/workflows/update-change.ts +++ b/src/core/templates/workflows/update-change.ts @@ -58,7 +58,7 @@ ${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. 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. - 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. @@ -149,7 +149,7 @@ ${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. 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. - 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. diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 4d2e717cae..fad7507b2a 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: 'af3c7dca6aeab8b3dfe54789f8d1570be564053d50749022091e1def59ffb9c3', + getOpsxUpdateCommandTemplate: '7808077fca5bc5985d463c3b3ef9235d4f1088c09417ab6258e5daad37701c89', }; 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': 'bd16c9ea9a8ade956f97a5a6390402c87c642a6d459bb186f991cc1d57554179', }; // 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..2398b23411 100644 --- a/test/core/templates/update-change.test.ts +++ b/test/core/templates/update-change.test.ts @@ -118,4 +118,34 @@ describe('update-change templates', () => { expect(newRecommendation, label).toBeGreaterThan(newAvailabilityCheck); } }); + + // Regression for #1836: step 4 said "Apply the requested edit" while step 5 + // said "Write only after the user confirms". "Apply" reads as a write verb in + // this document - step 5 is itself titled "Confirm and apply" - so the same + // request either wrote immediately or showed the revision first, depending on + // which passage the agent weighed. Step 5 is the only write path; step 4 + // drafts. + it('keeps step 4 non-writing so step 5 owns the only write (#1836)', () => { + for (const [label, body] of bodies) { + const start = body.indexOf('4. **Read and reconcile**'); + const end = body.indexOf('5. **Confirm and apply, one artifact at a time**'); + + expect(start, label).toBeGreaterThanOrEqual(0); + expect(end, label).toBeGreaterThan(start); + + const step = body.slice(start, end); + expect(step, label).toContain('Draft the requested edit.'); + expect(step, label).not.toContain('Apply the requested edit'); + } + }); + + it('orders the draft before the confirmed write (#1836)', () => { + for (const [label, body] of bodies) { + const draft = body.indexOf('Draft the requested edit.'); + const confirmedWrite = body.indexOf('Write only after the user confirms.'); + + expect(draft, label).toBeGreaterThanOrEqual(0); + expect(confirmedWrite, label).toBeGreaterThan(draft); + } + }); });