fix(update-change): draft the requested edit in step 4, write only in step 5

.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, 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.

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 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.
-   - 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.
+   - 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**
-   - Show each proposed revision and why. Write only after the user confirms.
+   - 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

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 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.
-   - 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.
+   - 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**
-   - Show each proposed revision and why. Write only after the user confirms.
+   - 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
@@ -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 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.
-   - 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.
+   - 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**
-   - Show each proposed revision and why. Write only after the user confirms.
+   - 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

test/core/templates/skill-templates-parity.test.ts

@@ -61,8 +61,8 @@ const EXPECTED_FUNCTION_HASHES: Record<string, string> = {
   getOpsxProposeSkillTemplate: 'b7215583fefddae0127076465de9b3de9c230f2f1ea9ae6e4fb2a46fe510e8d6',
   getOpsxProposeCommandTemplate: 'f016c66c2b6115b459751154c76a6270e444d6aee31973bb7cb8c0e6d505fb98',
   getFeedbackSkillTemplate: 'dabeb5e825b9349abc8156c3e7b8608f27987912a6d9bf47ef29addde6138133',
-  getUpdateChangeSkillTemplate: '7dc8abc6f64c58bf34d7581ed4ab095a3b7a53cb372349bee2d840db58622819',
-  getOpsxUpdateCommandTemplate: 'e2388521b22f92f74561df9a0c2f98e1fa4d265af93b5ba26f42fb47a6c5bfed',
+  getUpdateChangeSkillTemplate: '968e4164ce38258fdab858bbe65ff3f2300b0174a9c35194dc6cacafe4626f61',
+  getOpsxUpdateCommandTemplate: 'fc3b2ba3977a63e9f7689fef2ba05bb788f909db312818a4844867aefa6837d5',
 };
 
 const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record<string, string> = {
@@ -77,7 +77,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record<string, string> = {
   'openspec-verify-change': 'af9be013dcbe8c6d8f6d9ab10c893fbd03f4c62933c384d82f63894dd0ceb84f',
   'openspec-onboard': 'f6f59476acaf5e4d65dbb180da4cef62432612f3cecf207d471a951295e2003a',
   'openspec-propose': '679d0f868bed23cfb34a8ecc6b4ba4ff7b88dd7dbaef91563423e98f194f988f',
-  'openspec-update-change': '586547406aca94422dfeb3ffedce6c01049429b743f57ce829baa79ebc714d51',
+  'openspec-update-change': '832fc53b29546f70dc5b70047864a118f2b84ad86456dcad835ce3e21094811f',
 };
 
 // Intentionally excludes getFeedbackSkillTemplate: this list only models templates

test/core/templates/update-change.test.ts

@@ -16,6 +16,178 @@ const bodies: Array<[string, string]> = [
   ['command', command.content],
 ];
 
+// 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 "<artifact-id>" --change "<name>" --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. 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_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`',
+  '(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.
+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)|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
+// label names the marker, so a renamed heading reports which one went missing.
+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, `${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`);
+}
+
+// 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
+// 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) {
+      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([]);
+    }
+  });
+});
+
 describe('update-change templates', () => {
   it('generates the expected skill and command shape (3.1)', () => {
     expect(skill.name).toBe('openspec-update-change');