trending/OpenSpec
1
0
Fork 0
mirror of https://github.com/Fission-AI/OpenSpec.git synced 2026-10-02 05:24:34 +08:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* docs(changeset): drop em dashes from the release note

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Clay Good
2026-09-16 16:35:49 +00:00
committed by GitHub
co-authored by Claude Opus 5
parent 8fc65b7f70
commit fede536c27
5 changed files with 195 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/update-change-step-four-drafts.md
+5
View File
@@ -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
+5 -4
View File
@@ -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
+10 -8
View File
@@ -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
+3 -3
View File
@@ -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
+172
View File
@@ -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');