diff --git a/.changeset/update-change-step-four-drafts.md b/.changeset/update-change-step-four-drafts.md new file mode 100644 index 0000000000..1d74daa99f --- /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, 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/skills/openspec-update-change/SKILL.md b/skills/openspec-update-change/SKILL.md index 24c9f88367..24e0c7a688 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 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 diff --git a/src/core/templates/workflows/update-change.ts b/src/core/templates/workflows/update-change.ts index 7700cd8d7e..69e09e7c73 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 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 diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 4d2e717cae..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: '7dc8abc6f64c58bf34d7581ed4ab095a3b7a53cb372349bee2d840db58622819', - getOpsxUpdateCommandTemplate: 'e2388521b22f92f74561df9a0c2f98e1fa4d265af93b5ba26f42fb47a6c5bfed', + 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': '586547406aca94422dfeb3ffedce6c01049429b743f57ce829baa79ebc714d51', + '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 cf68f234f5..9571029603 100644 --- a/test/core/templates/update-change.test.ts +++ b/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 "" --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. 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');