From 58fe5d20229c5205023b67aa097a9fbb98d278ac Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Tue, 25 Aug 2026 01:01:58 -0300 Subject: [PATCH 1/2] fix(plan-lifecycle): closed-plan terminal step repair and edit regression guard A finished plan with a non-terminal step deadlocked: step required ongoing while archive required every step terminal, and reopening destroyed the closure proof. step now accepts terminal targets on a finished plan. edit now refuses a body that regresses a terminal step status, closing the stale-draft revert race the provenance digest cannot see. --- docs/PLAN.md | 18 +- .../skills/productivity/plan-manager/SKILL.md | 8 +- .../plan-manager/references/plan-contract.md | 13 +- .../plan-manager/scripts/plan.mjs | 27 ++- .../productivity/plan-workspace/SKILL.md | 4 +- .../references/plan-md-template.md | 18 +- scripts/tests/plan-cli.mjs | 159 ++++++++++++++++++ 7 files changed, 229 insertions(+), 18 deletions(-) diff --git a/docs/PLAN.md b/docs/PLAN.md index a112fdc9..19bf5c0f 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -126,8 +126,8 @@ The Steps table uses this exact header: Every Steps row must be terminal before the closing pull request merges. Once that merge closes the issue as completed, the derived state is `finished` and -step mutation is no longer legal. Post-merge work belongs to a named follow-up -plan. +step mutation is limited to terminal repair of a closed-but-unarchived record. +Post-merge work belongs to a named follow-up plan. No Steps `Files` cell names the plan's own issue reference. Writing lifecycle state into the record is the CLI's job, not an implementation step. @@ -196,10 +196,10 @@ resolves the target repository from that checkout's GitHub remote. | `plan.mjs claim ` | Take single-writer ownership of an existing plan: assign the acting login, stay idempotent when it already owns the plan, and refuse when another login does. | | `plan.mjs show [--body]` | Print the header strip, then `reviews: plan= code=`. With `--body`, print only the body to stdout and send both metadata lines to stderr, header first. | | `plan.mjs export ` | Write the issue body verbatim to `plan-.md` inside the scratch directory `git rev-parse --git-path docks-review` resolves, creating it mode 0700 when missing, and print the absolute path. | -| `plan.mjs edit --file ` | Validate the file as the plan record, refuse on any failed check, replace the issue body, and print the header strip and the changed lines. | +| `plan.mjs edit --file ` | Validate the file as the plan record, refuse on any failed check or any regression of a matching terminal step to a different non-terminal status, instruct re-export, replace the issue body, and print the header strip and the changed lines. | | `plan.mjs check >` | Validate a v3 record and print the pass result. | | `plan.mjs status [--reason ]` | Validate and apply one open-status transition, then replace all phase labels with the target phase label. Refuse closed issues. | -| `plan.mjs step ` | Rewrite one Steps `Status` cell after checking the plan state and dependencies. | +| `plan.mjs step ` | Rewrite one Steps `Status` cell after checking dependencies; require an open `ongoing` plan, or a `finished` plan when the target status is terminal (`done` or `skipped`) for repair. | | `plan.mjs list [--status ]` | Print `\t#\t` for every issue labelled `plan`, deriving `unlabelled`, `finished`, `retired`, and `duplicate` rather than reading them from the body. | | `plan.mjs next` | Print startable open plans, using the queue when it is present and valid. | | `plan.mjs archive <issue>` | Verify terminal steps, the latest trusted code-review result (with legacy body fallback only when no trusted comment record exists), completed closure, and an eligible merged closing pull request; strip stale phase labels and write no status. | @@ -249,6 +249,12 @@ remaining window but does not close it, because the read and the edit are separate API calls. A conflict is not an error to retry blindly: re-read the record, re-apply the intent, and run `plan.mjs check <issue>` before continuing. +`step` requires an open `ongoing` plan, except that a `finished` plan accepts a +step mutation when the target status is terminal (`done` or `skipped`). This +repairs a closed-but-unarchived record without creating a new closure event. +Never reopen the issue for this repair: `archive` trusts only the latest +closure, so reopening would discard the eligible closure proof. + An export copy is a snapshot of one body revision, not a live view. The `step` and `edit` commands rewrite body bytes. A status change also rewrites body bytes when it adds or clears the blocked reason. These body writes supersede every @@ -265,6 +271,10 @@ re-export. The `claim`, `archive`, and `retire` commands do not rewrite body bytes. A successful phase-only status change writes labels only and leaves the body and digest valid. The guard compares body bytes, not the issue timestamp. +`edit` also refuses an incoming body that moves any matching step id from a +terminal remote status (`done` or `skipped`) to a different non-terminal +status. Re-export the current body and re-apply the intended edit. + Re-export immediately before every body edit. Edit the export. Run `plan.mjs check <issue>`. Delete the export and its `.origin` sidecar. Never carry an edit across an intervening body write. diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md b/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md index 064ac2a0..5a7ea372 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md @@ -4,8 +4,8 @@ description: "Use when a goal may need the six-phase plan flow: decide, draft, r user-invocable: true metadata: pattern: tool-wrapper - updated: "2026-08-24" - content_hash: "9ba61ce6b78db45126838e43e8dc1c4033f0c6aad29b8eb559e6371ad5a31f18" + updated: "2026-08-25" + content_hash: "f51725f1a98263c28ae64f217751538a9fe841b27f464e4c67d785dc46e91dc9" --- # Plan Manager @@ -225,10 +225,10 @@ Archive proof rules live in the contract reference. | `claim <issue>` | resolve the acting login, `gh issue edit <n> --add-assignee @me` when unassigned; idempotent for the owner, refuses a foreign owner without writing | `plan #<n> claimed: <login>` | | `show <issue> [--body]` | header strip, then per-kind verdicts from latest trusted comments with legacy fallback only when none exists; `--body` puts the record alone on stdout and both metadata lines on stderr | header strip, then `reviews: plan=<pass\|repair\|blocked\|none> code=<pass\|fixes-required\|blocked\|none>` | | `export <issue>` | `export` writes the body to the worktree-aware `docks-review` directory. It writes its SHA-256 digest to `<file>.origin` with mode `0600`. | the absolute export path | -| `edit <issue> --file <path>` | `edit` runs 13 checks. It requires provenance for the current body. It refreshes the digest before the remote body write. It then replaces the body. | header strip, then `changed: <k> line(s)` and the changed lines as `-old` / `+new` | +| `edit <issue> --file <path>` | `edit` runs 13 checks. It requires provenance for the current body and refuses a file that moves any matching step id from a terminal remote status (`done` or `skipped`) to a different non-terminal status; re-export and re-apply the edit. It refreshes the digest before the remote body write, then replaces the body. | header strip, then `changed: <k> line(s)` and the changed lines as `-old` / `+new` | | `check <issue \| --file <path>>` | 13 checks | `plan check passed: #<n>` or `plan check passed: <path>` | | `status <issue> <status> [--reason <text>]` | `status` requires an open issue. It validates and updates its phase label. It keeps a leading `Blocked:` line only for blocked status. | `plan #<n> status: <old> -> <new>` | -| `step <issue> <step-id> <status>` | rewrite one Steps `Status` cell | `plan #<n> step <id>: <old> -> <new>` | +| `step <issue> <step-id> <status>` | rewrite one Steps `Status` cell; requires an open `ongoing` plan, or a `finished` plan when the target status is terminal (`done` or `skipped`) for repair | `plan #<n> step <id>: <old> -> <new>` | | `list [--status <s>]` | list plan issues and derive status from phase label for open work or from `state` + `stateReason` when closed; open issues first, then closed; each group sorted by ascending number | `<status>\t#<n>\t<title>` per line | | `next` | queue-aware startable plans from `docs/PLAN-QUEUE.md` (`Plan` cell holds the issue number); falls back to every `planned` plan on a missing or malformed queue, warning on stderr | `#<n>` per line | | `archive <issue>` | require completed closure, terminal steps, a trusted latest `Code-review: pass` comment (or legacy body pass only when no trusted code record exists), and a merged closing pull request into the target repository's default branch; remove any stale phase label without writing status | `plan #<n> finished (closed by <pr-url>)` | diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md b/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md index 13f66ca6..91a2d673 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md @@ -136,8 +136,9 @@ template placeholder `_Not researched yet._`. to a declared id. A bare `step 3` is invalid. Every Steps row must be terminal before the pull request carrying -`Closes #<issue>` merges. Once that merge closes the issue, step mutation is no -longer available. Post-merge work belongs to a named follow-up plan. +`Closes #<issue>` merges. After that merge closes the issue, step mutation is +limited to terminal repair of a closed-but-unarchived record. Post-merge work +belongs to a named follow-up plan. ## Acceptance table - exact header @@ -271,6 +272,11 @@ it never enters the export/edit body cycle. A conflict is not permission to retry blindly. Re-read the issue, re-apply the intended change, and run `plan.mjs check <issue>` before continuing. +`step` requires an open `ongoing` plan, except that a `finished` plan accepts a +step mutation when the target status is terminal (`done` or `skipped`). This +repairs a closed-but-unarchived record without creating a new closure event. +Never reopen the issue for this repair: `archive` trusts only the latest +closure, so reopening would discard the eligible closure proof. `export` writes the body to `<git-dir>/docks-review/plan-<n>.md`. It writes the body digest to `plan-<n>.md.origin` as one lowercase SHA-256 line. The sidecar @@ -281,6 +287,9 @@ sidecar, an unreadable digest, or a digest from a superseded body revision. After validation, `edit` refreshes the digest before the remote body write. A local sidecar failure fails closed and requires one re-export. A phase-only status change leaves the body and sidecar valid. +`edit` also refuses an incoming body that moves any matching step id from a +terminal remote status (`done` or `skipped`) to a different non-terminal +status. Re-export the current body and re-apply the intended edit. For every body edit, export the record. Edit the export. Run `plan.mjs check <issue>`. Delete the export and its `.origin` sidecar. diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index 4eccccf2..76db6760 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -825,7 +825,7 @@ function editPlan(args) { if (!value) fail('edit requires an issue and --file'); const options = parseOptions(flags, new Set(['--file'])); if (!options['--file']) fail('edit requires an issue and --file'); - const { issue, record } = readPlanIssue(value, true); + const { issue, parsed, record } = readPlanIssue(value, true); const file = options['--file']; const after = fs.readFileSync(file, 'utf8'); // Every body edit runs export, edit, check, delete, so a file with no sidecar was derived from bytes this @@ -842,6 +842,24 @@ function editPlan(args) { } const failures = checkPlan(after, issue); if (failures.length > 0) fail(failures.map((message) => `${file}: ${message}`).join('\n')); + // The provenance digest proves the remote body is the one exported from, not that the FILE reflects it: + // an intervening export refreshes the sidecar, so a stale draft can pass the digest check and silently + // revert step statuses the CLI advanced in between. A terminal cell never legally regresses + // (STEP_TRANSITIONS closes `done` and `skipped`), so refuse any matching step id moving away from one. + const stepStatuses = (text) => { + const table = parseRows(sectionMap(text).sections.get('Steps') ?? '', STEPS_HEADER, STEPS_SEPARATOR, 8); + return new Map(table.rows.map(({ cells }) => [cells[1], unquoteCode(cells[6])])); + }; + const remoteSteps = stepStatuses(parsed.body); + const regressed = [...stepStatuses(after)] + .filter(([id, status]) => { + const remote = remoteSteps.get(id); + return new Set(['done', 'skipped']).has(remote) && status !== remote && !new Set(['done', 'skipped']).has(status); + }) + .map(([id, status]) => `${id} (${remoteSteps.get(id)} -> ${status})`); + if (regressed.length > 0) { + fail(`status regression: step ${regressed.join(', ')} is terminal on #${issue.number} but the file reverts it; re-export and re-apply the edit`); + } const changes = changedLines(issue.body, after); // Stage provenance before the remote write. A failed sidecar write then costs one re-export; the reverse // order would leave a local digest naming a body GitHub already replaced, which reads as a valid export. @@ -902,7 +920,12 @@ function setStepStatus(args) { if (!STEP_STATUSES.has(target)) fail(`unknown step status: ${target}`); const { issue, parsed, record } = readPlanIssue(value, true); requireValidPlanIssue(issue); - if (record.status !== 'ongoing') fail(`plan status is ${record.status}; expected ongoing`); + // A plan whose issue closed with a step still non-terminal deadlocks: `step` wanted `ongoing` while + // `archive` wants every step terminal. Reopening to repair destroys the closure proof (only the latest + // closure event counts), so a `finished` plan accepts exactly the terminal repairs archive needs - a body + // edit on the closed issue, which creates no closure event and never touches GitHub state. + const terminalRepair = record.status === 'finished' && new Set(['done', 'skipped']).has(target); + if (record.status !== 'ongoing' && !terminalRepair) fail(`plan status is ${record.status}; expected ongoing`); const { sections } = sectionMap(parsed.body); const table = parseRows(sections.get('Steps') ?? '', STEPS_HEADER, STEPS_SEPARATOR, 8); const row = table.rows.find(({ cells }) => cells[1] === stepId); diff --git a/plugins/plan-lifecycle/skills/productivity/plan-workspace/SKILL.md b/plugins/plan-lifecycle/skills/productivity/plan-workspace/SKILL.md index bb18d04e..f645bf2f 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-workspace/SKILL.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-workspace/SKILL.md @@ -4,8 +4,8 @@ description: "Use when bootstrapping, migrating, auditing, or explicitly refresh user-invocable: true metadata: pattern: tool-wrapper - updated: "2026-08-24" - content_hash: "49b0f7d76b40059a871d7807f9b51f6322879a392f287d351d1c64669403f5f8" + updated: "2026-08-25" + content_hash: "457e5d91cb38f0cc95ceb4c40947f6065e2ff017a46c08e7c813cf0d3c63f4d3" --- # Plans Workspace diff --git a/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/plan-md-template.md b/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/plan-md-template.md index 339146d0..b8bb9420 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/plan-md-template.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/plan-md-template.md @@ -130,8 +130,8 @@ The Steps table uses this exact header: Every Steps row must be terminal before the closing pull request merges. Once that merge closes the issue as completed, the derived state is `finished` and -step mutation is no longer legal. Post-merge work belongs to a named follow-up -plan. +step mutation is limited to terminal repair of a closed-but-unarchived record. +Post-merge work belongs to a named follow-up plan. No Steps `Files` cell names the plan's own issue reference. Writing lifecycle state into the record is the CLI's job, not an implementation step. @@ -200,10 +200,10 @@ resolves the target repository from that checkout's GitHub remote. | `plan.mjs claim <issue>` | Take single-writer ownership of an existing plan: assign the acting login, stay idempotent when it already owns the plan, and refuse when another login does. | | `plan.mjs show <issue> [--body]` | Print the header strip, then `reviews: plan=<pass\|repair\|blocked\|none> code=<pass\|fixes-required\|blocked\|none>`. With `--body`, print only the body to stdout and send both metadata lines to stderr, header first. | | `plan.mjs export <issue>` | Write the issue body verbatim to `plan-<issue>.md` inside the scratch directory `git rev-parse --git-path docks-review` resolves, creating it mode 0700 when missing, and print the absolute path. | -| `plan.mjs edit <issue> --file <path>` | Validate the file as the plan record, refuse on any failed check, replace the issue body, and print the header strip and the changed lines. | +| `plan.mjs edit <issue> --file <path>` | Validate the file as the plan record, refuse on any failed check or any regression of a matching terminal step to a different non-terminal status, instruct re-export, replace the issue body, and print the header strip and the changed lines. | | `plan.mjs check <issue \| --file <path>>` | Validate a v3 record and print the pass result. | | `plan.mjs status <issue> <status> [--reason <text>]` | Validate and apply one open-status transition, then replace all phase labels with the target phase label. Refuse closed issues. | -| `plan.mjs step <issue> <step-id> <status>` | Rewrite one Steps `Status` cell after checking the plan state and dependencies. | +| `plan.mjs step <issue> <step-id> <status>` | Rewrite one Steps `Status` cell after checking dependencies; require an open `ongoing` plan, or a `finished` plan when the target status is terminal (`done` or `skipped`) for repair. | | `plan.mjs list [--status <s>]` | Print `<status>\t#<issue>\t<title>` for every issue labelled `plan`, deriving `unlabelled`, `finished`, `retired`, and `duplicate` rather than reading them from the body. | | `plan.mjs next` | Print startable open plans, using the queue when it is present and valid. | | `plan.mjs archive <issue>` | Verify terminal steps, the latest trusted code-review result (with legacy body fallback only when no trusted comment record exists), completed closure, and an eligible merged closing pull request; strip stale phase labels and write no status. | @@ -253,6 +253,12 @@ remaining window but does not close it, because the read and the edit are separate API calls. A conflict is not an error to retry blindly: re-read the record, re-apply the intent, and run `plan.mjs check <issue>` before continuing. +`step` requires an open `ongoing` plan, except that a `finished` plan accepts a +step mutation when the target status is terminal (`done` or `skipped`). This +repairs a closed-but-unarchived record without creating a new closure event. +Never reopen the issue for this repair: `archive` trusts only the latest +closure, so reopening would discard the eligible closure proof. + An export copy is a snapshot of one body revision, not a live view. The `step` and `edit` commands rewrite body bytes. A status change also rewrites body bytes when it adds or clears the blocked reason. These body writes supersede every @@ -269,6 +275,10 @@ re-export. The `claim`, `archive`, and `retire` commands do not rewrite body bytes. A successful phase-only status change writes labels only and leaves the body and digest valid. The guard compares body bytes, not the issue timestamp. +`edit` also refuses an incoming body that moves any matching step id from a +terminal remote status (`done` or `skipped`) to a different non-terminal +status. Re-export the current body and re-apply the intended edit. + Re-export immediately before every body edit. Edit the export. Run `plan.mjs check <issue>`. Delete the export and its `.origin` sidecar. Never carry an edit across an intervening body write. diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index 9d109e03..e3343d55 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -1026,6 +1026,83 @@ try { 'a second consecutive edit of the same exported file must update the issue', ); + const terminalRegressionNumber = createPlan('terminal-step-regression-edit'); + makeValid(terminalRegressionNumber); + updateIssue(terminalRegressionNumber, (entry) => { + entry.body = replaceStepStatus(entry.body, 'done'); + }); + const beforeTerminalRegressionEdit = issue(terminalRegressionNumber).body; + const terminalRegressionExport = run('export', String(terminalRegressionNumber)); + expectSuccess(terminalRegressionExport, 'terminal step regression edit export'); + const terminalRegressionExportPath = terminalRegressionExport.stdout.trim(); + fs.writeFileSync( + terminalRegressionExportPath, + replaceStepStatus(fs.readFileSync(terminalRegressionExportPath, 'utf8'), 'planned'), + ); + const terminalRegressionBodyWritesBefore = loadState().calls.filter( + (call) => + call[0] === 'issue' && + call[1] === 'edit' && + call[2] === String(terminalRegressionNumber) && + call.includes('--body-file'), + ).length; + const terminalRegressionEdit = run( + 'edit', + String(terminalRegressionNumber), + '--file', + terminalRegressionExportPath, + ); + assert.equal(terminalRegressionEdit.status, 1, 'a file must not regress a terminal remote step'); + assert.equal( + terminalRegressionEdit.stderr.trim(), + `status regression: step implement_contract (done -> planned) is terminal on #${terminalRegressionNumber} but the file reverts it; re-export and re-apply the edit`, + ); + assert.equal( + issue(terminalRegressionNumber).body, + beforeTerminalRegressionEdit, + 'a terminal step regression must leave the remote body unchanged', + ); + assert.equal( + loadState().calls.filter( + (call) => + call[0] === 'issue' && + call[1] === 'edit' && + call[2] === String(terminalRegressionNumber) && + call.includes('--body-file'), + ).length, + terminalRegressionBodyWritesBefore, + 'a terminal step regression must fail before the body write', + ); + + const terminalStepRow = + '| 1 | implement_contract | Implement the contract | src/example.mjs | - | `local` | `done` | command exits 0 |'; + const addedStepRow = + '| 2 | extend_contract | Extend the contract | src/extra.mjs | - | `local` | `planned` | command exits 0 |'; + const terminalPreservingBody = beforeTerminalRegressionEdit.replace( + terminalStepRow, + `${terminalStepRow}\n${addedStepRow}`, + ); + assert.notEqual( + terminalPreservingBody, + beforeTerminalRegressionEdit, + 'the non-regressing edit fixture must add a step row', + ); + fs.writeFileSync(terminalRegressionExportPath, terminalPreservingBody); + const terminalPreservingEdit = run( + 'edit', + String(terminalRegressionNumber), + '--file', + terminalRegressionExportPath, + ); + expectSuccess(terminalPreservingEdit, 'terminal-preserving row addition edit'); + assert.equal( + issue(terminalRegressionNumber).body, + terminalPreservingBody, + 'adding a row while preserving terminal cells must update the remote body', + ); + assert.match(issue(terminalRegressionNumber).body, /\| `done` \| command exits 0 \|/); + assert.match(issue(terminalRegressionNumber).body, /\| 2 \| extend_contract .+ \| `planned` \| command exits 0 \|/); + const staleExportNumber = createPlan('stale-export-edit'); makeValid(staleExportNumber); setIssueStatus(staleExportNumber, 'ongoing'); @@ -1443,6 +1520,88 @@ try { } } } + + const finishedStepNumber = createPlan('finished-step-repair'); + makeValid(finishedStepNumber); + setIssueStatus(finishedStepNumber, 'ongoing'); + updateIssue(finishedStepNumber, (entry) => { + entry.body = replaceStepStatus(entry.body, 'in-flight'); + entry.state = 'CLOSED'; + entry.stateReason = 'COMPLETED'; + }); + const finishedStepBefore = issue(finishedStepNumber); + const finishedStepCallsBefore = loadState().calls.length; + const finishedStepRepair = run('step', String(finishedStepNumber), 'implement_contract', 'done'); + expectSuccess(finishedStepRepair, 'finished plan terminal step repair'); + assert.equal( + finishedStepRepair.stdout.trim(), + `plan #${finishedStepNumber} step implement_contract: in-flight -> done`, + ); + const finishedStepAfter = issue(finishedStepNumber); + assert.match(finishedStepAfter.body, /\| `done` \| command exits 0 \|/); + assert.equal(finishedStepAfter.state, finishedStepBefore.state, 'terminal repair must not reopen the issue'); + assert.equal( + finishedStepAfter.stateReason, + finishedStepBefore.stateReason, + 'terminal repair must preserve the completed state reason', + ); + assert.deepEqual( + finishedStepAfter.labels, + finishedStepBefore.labels, + 'terminal repair must not mutate the plan labels', + ); + const finishedStepCalls = loadState().calls.slice(finishedStepCallsBefore); + assert.equal( + finishedStepCalls.filter( + (call) => + call[0] === 'issue' && + call[1] === 'edit' && + call[2] === String(finishedStepNumber) && + call.includes('--body-file'), + ).length, + 1, + 'terminal repair must record exactly one body edit', + ); + assert.equal( + finishedStepCalls.some( + (call) => call[0] === 'issue' && new Set(['close', 'reopen']).has(call[1]), + ), + false, + 'terminal repair must not create another issue state transition', + ); + + const finishedStepBodyBeforeRefusal = finishedStepAfter.body; + const finishedStepBodyWritesBeforeRefusal = loadState().calls.filter( + (call) => + call[0] === 'issue' && + call[1] === 'edit' && + call[2] === String(finishedStepNumber) && + call.includes('--body-file'), + ).length; + const finishedNonterminalStep = run( + 'step', + String(finishedStepNumber), + 'implement_contract', + 'in-flight', + ); + assert.equal(finishedNonterminalStep.status, 1, 'a finished plan must reject a non-terminal step target'); + assert.equal(finishedNonterminalStep.stderr.trim(), 'plan status is finished; expected ongoing'); + assert.equal( + issue(finishedStepNumber).body, + finishedStepBodyBeforeRefusal, + 'refusing a non-terminal repair must leave the closed issue body unchanged', + ); + assert.equal( + loadState().calls.filter( + (call) => + call[0] === 'issue' && + call[1] === 'edit' && + call[2] === String(finishedStepNumber) && + call.includes('--body-file'), + ).length, + finishedStepBodyWritesBeforeRefusal, + 'a non-terminal target on a finished plan must fail before the body write', + ); const malformedStepNumber = createPlan('malformed-step'); makeValid(malformedStepNumber); setIssueStatus(malformedStepNumber, 'ongoing'); From aa62e70cb7e56cc9a747b15b238af45f68f99c3f Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Tue, 25 Aug 2026 01:06:57 -0300 Subject: [PATCH 2/2] style(tests): apply biome formatting to plan-cli additions --- scripts/tests/plan-cli.mjs | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index e3343d55..cf079485 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -1046,12 +1046,7 @@ try { call[2] === String(terminalRegressionNumber) && call.includes('--body-file'), ).length; - const terminalRegressionEdit = run( - 'edit', - String(terminalRegressionNumber), - '--file', - terminalRegressionExportPath, - ); + const terminalRegressionEdit = run('edit', String(terminalRegressionNumber), '--file', terminalRegressionExportPath); assert.equal(terminalRegressionEdit.status, 1, 'a file must not regress a terminal remote step'); assert.equal( terminalRegressionEdit.stderr.trim(), @@ -1088,12 +1083,7 @@ try { 'the non-regressing edit fixture must add a step row', ); fs.writeFileSync(terminalRegressionExportPath, terminalPreservingBody); - const terminalPreservingEdit = run( - 'edit', - String(terminalRegressionNumber), - '--file', - terminalRegressionExportPath, - ); + const terminalPreservingEdit = run('edit', String(terminalRegressionNumber), '--file', terminalRegressionExportPath); expectSuccess(terminalPreservingEdit, 'terminal-preserving row addition edit'); assert.equal( issue(terminalRegressionNumber).body, @@ -1563,9 +1553,7 @@ try { 'terminal repair must record exactly one body edit', ); assert.equal( - finishedStepCalls.some( - (call) => call[0] === 'issue' && new Set(['close', 'reopen']).has(call[1]), - ), + finishedStepCalls.some((call) => call[0] === 'issue' && new Set(['close', 'reopen']).has(call[1])), false, 'terminal repair must not create another issue state transition', ); @@ -1578,12 +1566,7 @@ try { call[2] === String(finishedStepNumber) && call.includes('--body-file'), ).length; - const finishedNonterminalStep = run( - 'step', - String(finishedStepNumber), - 'implement_contract', - 'in-flight', - ); + const finishedNonterminalStep = run('step', String(finishedStepNumber), 'implement_contract', 'in-flight'); assert.equal(finishedNonterminalStep.status, 1, 'a finished plan must reject a non-terminal step target'); assert.equal(finishedNonterminalStep.stderr.trim(), 'plan status is finished; expected ongoing'); assert.equal(