From 6ea8fd45ea8acda20cfbe32beeca982f3b59a36e Mon Sep 17 00:00:00 2001 From: Alexander Ivanov Date: Tue, 18 Aug 2026 09:21:04 +0300 Subject: [PATCH 1/2] Add change-scoped Rollback (active + archived) and opt-in checkpoint retention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related gaps: Rollback only ever targeted one process at a time, and nothing offered Rollback for an already-archived change even though archiving never touches checkpoint/journal data. A task-scoped alternative (per-checklist-item rollback via a SQLite diff store) was discussed and rejected — see ADR 0008 for the full reasoning (unsolved attribution problem, native-dependency risk, and a fail-closed-vs-warn-and-restore philosophy mismatch with the rest of this codebase). rollbackChangeCheckpoints() aggregates every finalized checkpoint for a Change into one restore: each file's target is its content from the earliest checkpoint that touched it, conflict-checked against each file's latest known post-run hash — same fail-closed, all-or-nothing semantics as single-process rollback, no new storage. Works identically for active and archived changes. Also adds openspec-ui.checkpointRetentionDays (VS Code setting, default 0 = keep forever, unchanged from every prior version) since Rollback now reaches further back into a Change's history and unbounded growth was worth addressing — pruning is opt-in and its Rollback-availability consequence is disclosed, not silent. Co-Authored-By: Claude Sonnet 5 --- README.md | 6 +- ...08-change-scoped-rollback-and-retention.md | 105 ++++++++++++++++ .../change-scoped-rollback/proposal.md | 99 +++++++++++++++ .../specs/persistent-workbench-runs/spec.md | 56 +++++++++ .../specs/vscode-extension/spec.md | 30 +++++ .../changes/change-scoped-rollback/tasks.md | 116 ++++++++++++++++++ packages/core/package.json | 2 +- packages/core/src/checkpoint.test.ts | 70 +++++++++++ packages/core/src/checkpoint.ts | 78 ++++++++++-- packages/core/src/process-scheduler.test.ts | 12 ++ packages/core/src/process-scheduler.ts | 18 +++ packages/core/src/workbench-recovery.test.ts | 38 ++++++ packages/core/src/workbench-recovery.ts | 50 ++++++++ packages/extension/CHANGELOG.md | 15 +++ packages/extension/README.md | 16 +++ packages/extension/package.json | 17 ++- packages/extension/src/commands.test.ts | 99 +++++++++++++++ packages/extension/src/commands.ts | 25 ++++ packages/extension/src/extension.ts | 11 ++ .../src/implementation-sessions.test.ts | 56 +++++++++ .../extension/src/implementation-sessions.ts | 43 +++++++ packages/server/package.json | 2 +- packages/server/src/recovery-rest.ts | 13 ++ packages/server/src/server.test.ts | 35 ++++++ packages/server/src/server.ts | 5 + 25 files changed, 1000 insertions(+), 17 deletions(-) create mode 100644 docs/adr/0008-change-scoped-rollback-and-retention.md create mode 100644 openspec/changes/change-scoped-rollback/proposal.md create mode 100644 openspec/changes/change-scoped-rollback/specs/persistent-workbench-runs/spec.md create mode 100644 openspec/changes/change-scoped-rollback/specs/vscode-extension/spec.md create mode 100644 openspec/changes/change-scoped-rollback/tasks.md diff --git a/README.md b/README.md index 12c0ba2f..0b0b86f4 100644 --- a/README.md +++ b/README.md @@ -162,9 +162,9 @@ release artifact. Current release versions are: | Package | Version | Release role | | --- | ---: | --- | -| `@openspec-ui/core` | 0.18.0 | Shared behavior and persistence contract | -| `openspec-ui-vscode` | 0.12.1 | VS Code delivery | -| `@openspec-ui/server` | 1.7.0 | Standalone server delivery | +| `@openspec-ui/core` | 0.19.0 | Shared behavior and persistence contract | +| `openspec-ui-vscode` | 0.14.0 | VS Code delivery | +| `@openspec-ui/server` | 1.8.0 | Standalone server delivery | | `@openspec-ui/webui` | 1.8.0 | Shared browser UI | | `@openspec-ui/cli` | 0.1.0 | CI merge-gate delivery | diff --git a/docs/adr/0008-change-scoped-rollback-and-retention.md b/docs/adr/0008-change-scoped-rollback-and-retention.md new file mode 100644 index 00000000..ad39a6ac --- /dev/null +++ b/docs/adr/0008-change-scoped-rollback-and-retention.md @@ -0,0 +1,105 @@ +# 0008: Change-Scoped Rollback Instead of Task-Scoped, With Opt-In Retention + +Status: Accepted + +Date: 2026-08-18 + +## Context + +Rollback (ADR 0004) restores exactly the files one process's checkpoint +touched, and is currently only offered per-process, only for active +changes (no code path checked archive status either way — it simply +never came up). Two related asks came up in review: + +1. Should Rollback be scoped to individual `tasks.md` checklist items, + so undoing "just task 3" doesn't also undo tasks 1 and 2 from the same + run? +2. Should Rollback be available for archived changes too, and directly + from the change itself rather than only by finding the right entry in + the Processes list? + +For (1), a concrete proposal was floated: a SQLite database recording a +diff per task, with a retention-period fork and a "restore with a +merge-risk warning" UX on rollback. + +## Decision + +1. **Rollback is scoped to the whole Change, not to individual tasks.** + `rollbackChangeCheckpoints` (`packages/core/src/checkpoint.ts`) + aggregates every finalized checkpoint recorded against a `changeName`: + each file's restore target is its content from the *earliest* + checkpoint that touched it (undo everything ever done under this + Change); the conflict check compares against each file's *latest* + known `afterHash`. Same fail-closed, all-or-nothing semantics as + single-process rollback — any conflict refuses the entire restore, not + just the conflicting file. No new storage: this is a pure aggregation + over checkpoints the system already captures. +2. **Rejected the SQLite/per-task-diff proposal.** The actually hard part + of task-scoped rollback is attribution (which files belong to which + task), and no storage technology changes that — it needs incremental, + per-checkbox-transition checkpoints, a materially bigger change to the + capture path than anything storage-related. Separately, a native + SQLite dependency in a VS Code extension carries real packaging risk + (native addon build-per-platform, or a slower WASM fallback) that + nothing else in this codebase's dependency-free, plain-JSON journal + architecture (ADR 0004, ADR 0006) has taken on. And "restore with a + merge-risk warning" is a materially more permissive philosophy than + the fail-closed rollback this codebase has used everywhere else. + Change-scoped rollback answers the same underlying want ("undo what I + did to this change") without any of the above. +3. **Rollback works identically for archived changes.** Archiving + (`archiveChange()`, a thin `openspec archive` CLI wrapper scoped to + `openspec/changes/`) never touches checkpoint/journal data — it was + already unblocked at the data layer before this change, just not + exposed as a command. `openspec-ui.rollbackChange` takes a + `ChangeTreeItem` from either the Changes or Archive tree, no + archived-state guard. +4. **Retention is opt-in, off by default.** `openspec-ui.checkpointRetentionDays` + (VS Code setting; `0` or negative — the default — keeps checkpoint/ + process history forever, unchanged from every prior version). A + positive value prunes processes/checkpoints older than N days once, on + the next extension activation, via `WorkbenchProcessScheduler.removeBefore`. + Pruning a process makes Rollback (single-process and whole-Change) + permanently unavailable for it — this is disclosed in the setting's + own description and in the extension README, not silent. +5. **Scoped to the VS Code extension for this first pass.** The + automatic on-activation pruning has no standalone-app equivalent (the + standalone server has no settings/config system to read a retention + value from); `WorkbenchRecoveryService.rollbackChange` exists in core + and is exposed over REST for API completeness, but nothing calls it + automatically outside VS Code. Same pattern already used by + `repo-bootstrap-snippets`. + +## Rejected Alternatives + +### SQLite-backed per-task diff store + +See point 2 above — rejected on both the attribution problem (unsolved +by a storage change) and the dependency-risk/philosophy mismatch with +the rest of this codebase. + +### Warn-and-restore-anyway on rollback conflict + +Rejected — every other rollback path in this codebase (`rollbackCheckpoint`, +and now `rollbackChangeCheckpoints`) refuses the entire restore on any +conflict. A permissive exception for this one path would be a surprising, +undocumented inconsistency. + +### Automatic retention pruning in the standalone server too + +Rejected for this pass — the standalone server has no persistent +settings mechanism to source a retention value from, and building one +is out of scope for what was actually asked (a VS Code setting). + +## Consequences + +- Rollback's mental model becomes "per-process" and "per-Change" (not + per-task) — simpler than task-scoped rollback would have been, at the + cost of not being able to undo a single task's contribution in + isolation when multiple tasks were touched by the same process run. +- No new dependency, no new persisted data shape — `rollbackChangeCheckpoints` + is pure aggregation over the existing checkpoint format. +- Checkpoint/process history still grows unboundedly by default, exactly + as before this change; operators who want bounded growth must opt in + explicitly via `openspec-ui.checkpointRetentionDays` and accept the + Rollback-availability trade-off that comes with it. diff --git a/openspec/changes/change-scoped-rollback/proposal.md b/openspec/changes/change-scoped-rollback/proposal.md new file mode 100644 index 00000000..20a6a3c3 --- /dev/null +++ b/openspec/changes/change-scoped-rollback/proposal.md @@ -0,0 +1,99 @@ +## Why + +Two related gaps raised in review: (1) Rollback only targets one +process's checkpoint at a time — undoing a Change that took several +implement runs means finding and rolling back each one individually, in +the right order; (2) nothing in the UI offers Rollback for an already- +archived Change, even though archiving never actually touches +checkpoint/journal data (`archiveChange()` is a thin `openspec archive` +CLI wrapper scoped to `openspec/changes/`) — it was already unblocked at +the data layer, just never exposed as a command. + +A task-scoped alternative (rolling back a single `tasks.md` checklist +item) was discussed and explicitly rejected — see ADR 0008 for the full +reasoning (attribution problem unsolved by storage choice; SQLite +dependency risk in a VS Code extension; fail-closed-vs-warn-and-proceed +philosophy mismatch). + +Also closes a related, previously undiscussed gap: checkpoint/process +history has never had any retention mechanism in the VS Code extension +(the core `WorkbenchRecoveryService.cleanupBefore` exists but is only +reachable through the standalone server's REST API, which the extension +doesn't use in its primary direct-import mode) — now that Rollback +reaches further back in a Change's history, unbounded growth is worth +addressing with an explicit, opt-in setting. + +## What Changes + +- `packages/core/src/checkpoint.ts`: new `rollbackChangeCheckpoints()` + aggregates every finalized checkpoint for a Change into one restore — + each file's target is its content from the *earliest* checkpoint that + touched it, conflict-checked against each file's *latest* known + post-run hash. Same fail-closed, all-or-nothing semantics as + single-process rollback. Refactored `rollbackCheckpoint()` to share the + new `restoreFiles()` primitive instead of duplicating the conflict- + check/restore loop (behavior-preserving). +- `packages/core/src/workbench-recovery.ts` (`WorkbenchRecoveryService`, + used by the server/standalone) and `packages/extension/src/ + implementation-sessions.ts` (`ImplementationSessionManager`, used by + the extension's primary direct-import mode) both gain + `changeRollbackDetails(changeName)` (preview: process/file counts, for + a confirmation prompt) and `rollbackChange(changeName)` (perform it), + built on the shared core aggregation function. +- New `openspec-ui.rollbackChange` command, offered on a `ChangeTreeItem` + from **either** the Changes or Archive tree (no archived-state guard — + archiving never touched this data). +- New `openspec-ui.checkpointRetentionDays` VS Code setting (default `0` + = keep forever, unchanged from every prior version). A positive value + prunes processes/checkpoints older than N days once, on the next + extension activation. `WorkbenchProcessScheduler` gains `removeBefore()` + (in-place mutation, unlike `WorkbenchRecoveryService.cleanupBefore`'s + whole-instance replacement — chosen because the extension's scheduler + instance is shared by reference across several tree providers and the + chat participant, so replacing it wholesale would require a bigger, + unrelated refactor). Pruning makes Rollback permanently unavailable for + the pruned process(es) — disclosed in the setting description and + README, not silent. +- `packages/server/src/recovery-rest.ts`: `POST /api/processes/rollback- + change` for API completeness/parity with the extension's capability — + no new standalone UI button for it in this pass (see "Impact"). + +## Capabilities + +### New Capabilities + +(none) + +### Modified Capabilities + +- `persistent-workbench-runs`: Rollback can now target an entire Change, + not just one process; adds an opt-in, VS Code-only retention setting. +- `vscode-extension`: Changes and Archive trees offer a "Rollback Change" + action. + +## Impact + +- `packages/core/src/checkpoint.ts` (`restoreFiles`, `RestoreEntry`, + `rollbackChangeCheckpoints`), `checkpoint.test.ts`. +- `packages/core/src/workbench-recovery.ts` (`changeRollbackDetails`, + `rollbackChange`), `workbench-recovery.test.ts`. +- `packages/core/src/process-scheduler.ts` (`removeBefore`), + `process-scheduler.test.ts`. +- `packages/extension/src/implementation-sessions.ts` + (`changeRollbackDetails`, `rollbackChange`, `dropSessions`), + `implementation-sessions.test.ts`. +- `packages/extension/src/commands.ts` (`openspec-ui.rollbackChange`), + `commands.test.ts`. +- `packages/extension/src/extension.ts` (retention pruning at + activation, reading the new setting). +- `packages/extension/package.json` (command, context-menu entries on + both `openspec-ui.activeChange`/`openspec-ui.archivedChange`, + `openspec-ui.checkpointRetentionDays` configuration property). +- `packages/server/src/recovery-rest.ts`, `server.ts`, + `recovery-rest.test.ts` (or `server.test.ts`, wherever the existing + process-rollback endpoint is tested) — REST parity only, no automatic + retention pruning on the server (no settings/config system to source a + value from) and no new standalone webui button — deliberate scope + decision for this pass, same pattern `repo-bootstrap-snippets` already + used ("VS Code only for this first pass"). +- `docs/adr/0008-change-scoped-rollback-and-retention.md` (new). diff --git a/openspec/changes/change-scoped-rollback/specs/persistent-workbench-runs/spec.md b/openspec/changes/change-scoped-rollback/specs/persistent-workbench-runs/spec.md new file mode 100644 index 00000000..db8f7632 --- /dev/null +++ b/openspec/changes/change-scoped-rollback/specs/persistent-workbench-runs/spec.md @@ -0,0 +1,56 @@ +## ADDED Requirements + +### Requirement: Rollback can target an entire Change + +The core SHALL offer, alongside per-process rollback, a rollback that +aggregates every finalized checkpoint recorded against a Change name +into a single restore. Each file's restore target SHALL be its content +from the earliest checkpoint that touched it; the conflict check SHALL +compare current file content against each file's latest known post-run +hash. Any conflict SHALL refuse the entire restore, not just the +conflicting file — the same fail-closed, all-or-nothing semantics as +single-process rollback. This capability SHALL be available identically +whether the Change is currently active or already archived. + +#### Scenario: Restoring a Change with multiple implement runs + +- **WHEN** a Change was implemented across two sequential runs and the + user requests a Change-level rollback +- **THEN** every file either run touched is restored to its content from + before the earlier of the two runs + +#### Scenario: A file changed outside any known run + +- **WHEN** a Change-level rollback is requested and a file's current + content does not match its latest known post-run hash +- **THEN** the service reports a conflict and restores no files at all + +#### Scenario: Rollback for an archived Change + +- **WHEN** a Change-level rollback is requested for a Change that has + since been archived +- **THEN** the restore proceeds identically to an active Change — archive + status plays no part in eligibility + +### Requirement: Opt-in checkpoint retention via extension setting + +The VS Code extension SHALL expose a setting controlling automatic +checkpoint/process retention, defaulting to keeping history forever +(unchanged from prior behavior). When configured with a positive number +of days, the extension SHALL prune processes and their checkpoint +sessions older than that cutoff once, at extension activation. Pruning a +process SHALL make both single-process and Change-level rollback +permanently unavailable for it, and this consequence SHALL be disclosed +in the setting's own description. + +#### Scenario: Default behavior is unchanged + +- **WHEN** the retention setting is left at its default +- **THEN** no process or checkpoint data is ever automatically pruned + +#### Scenario: A positive retention period prunes old history + +- **WHEN** the setting is configured to a positive number of days and the + extension activates +- **THEN** processes created before the cutoff, and their checkpoint + sessions, are removed together diff --git a/openspec/changes/change-scoped-rollback/specs/vscode-extension/spec.md b/openspec/changes/change-scoped-rollback/specs/vscode-extension/spec.md new file mode 100644 index 00000000..621f7618 --- /dev/null +++ b/openspec/changes/change-scoped-rollback/specs/vscode-extension/spec.md @@ -0,0 +1,30 @@ +## ADDED Requirements + +### Requirement: Changes and Archive trees offer whole-Change rollback + +A "Rollback Change" action SHALL be available on a Change item in either +the "Changes" or "Archive" tree view. Selecting it, when at least one +rollback-eligible process exists for that Change, SHALL show a +confirmation naming the affected file and process counts before +proceeding; when no rollback-eligible process exists, the system SHALL +report that instead of prompting for confirmation. + +#### Scenario: Rollback from the Changes tree + +- **WHEN** the user selects "Rollback Change" on an active Change with + rollback-eligible processes +- **THEN** a confirmation shows the affected file and process counts +- **AND** confirming restores those files and refreshes the trees + +#### Scenario: Rollback from the Archive tree + +- **WHEN** the user selects "Rollback Change" on an archived Change with + rollback-eligible processes +- **THEN** the same confirmation and restore behavior applies, unmodified + by archive status + +#### Scenario: No rollback-eligible processes + +- **WHEN** the user selects "Rollback Change" on a Change with no + rollback-eligible processes +- **THEN** the system reports this without showing a confirmation dialog diff --git a/openspec/changes/change-scoped-rollback/tasks.md b/openspec/changes/change-scoped-rollback/tasks.md new file mode 100644 index 00000000..19dc11f1 --- /dev/null +++ b/openspec/changes/change-scoped-rollback/tasks.md @@ -0,0 +1,116 @@ +## 1. Core: aggregated restore primitive + +- [x] 1.1 `checkpoint.ts`: `RestoreEntry`, `restoreFiles()` (shared + conflict-check/restore loop); `rollbackCheckpoint()` refactored to + build entries from a single checkpoint and delegate — behavior + unchanged, confirmed by the pre-existing tests still passing unmodified. + `rollbackChangeCheckpoints()` aggregates a list of finalized + checkpoints: earliest-before per file, latest-afterHash per file for + the conflict check. +- [x] 1.2 `checkpoint.test.ts`: multi-checkpoint aggregation (earliest + wins for restore target, order-independent), conflict refuses the + whole restore, empty-list and not-finalized error cases. 9/9 core + checkpoint tests passing (4 pre-existing + 5 new). + +## 2. Core: WorkbenchRecoveryService (server/standalone) + +- [x] 2.1 `workbench-recovery.ts`: `changeRollbackCandidates()` (private, + shared by both new methods — active or archived, no archive-status + check since archiving never touches this data), `changeRollbackDetails()` + (process/file-count preview), `rollbackChange()`. +- [x] 2.2 `workbench-recovery.test.ts`: rolls back two processes for one + Change across two checkpoints to the earliest state; throws for a + Change with nothing eligible. 5/5 passing (3 pre-existing + 2 new). + +## 3. Core: WorkbenchProcessScheduler retention primitive + +- [x] 3.1 `process-scheduler.ts`: `removeBefore(cutoff)` — in-place Map + mutation (not instance replacement, unlike + `WorkbenchRecoveryService.cleanupBefore`), because the extension's + scheduler instance is captured by reference across several tree + providers and the chat participant. +- [x] 3.2 `process-scheduler.test.ts`: drops only processes created + before the cutoff, in place. 6/6 passing (5 pre-existing + 1 new). + +## 4. Extension: ImplementationSessionManager mirror + +- [x] 4.1 `implementation-sessions.ts`: `changeRollbackCandidates()` + (private), `changeRollbackDetails()`, `rollbackChange()` — same + eligibility rule and aggregation as core's mirror, duplicated here + because the extension's primary mode keeps its own session map instead + of going through `WorkbenchRecoveryService`. `dropSessions()` for + retention pruning. +- [x] 4.2 `implementation-sessions.test.ts`: rolls back two runs for one + Change to the earliest state; throws when nothing eligible; + `dropSessions` removes a session from participating in later rollback + calls. 6/6 passing (3 pre-existing + 3 new). + +## 5. Extension: command, menu, retention setting + +- [x] 5.1 `commands.ts`: `openspec-ui.rollbackChange` — takes a + `ChangeTreeItem` from either tree, previews via + `changeRollbackDetails`, confirms, calls `rollbackChange`, same + conflict/error handling pattern as `openspec-ui.rollbackProcess`. +- [x] 5.2 `package.json`: command registration; context-menu entries for + both `openspec-ui.activeChange` and `openspec-ui.archivedChange` + (single `when` clause, same pattern as `deleteChange`); + `openspec-ui.checkpointRetentionDays` configuration property (default + `0`, description discloses the Rollback-availability consequence of + pruning). +- [x] 5.3 `extension.ts`: reads the setting after + `implementationSessions.restore()`; if positive, computes the cutoff + and calls `scheduler.removeBefore()` + `implementationSessions.dropSessions()`. + Relies on the already-registered `scheduler.onDidChange(persistRuns)` + subscription to persist the pruned state — no separate persist call. +- [x] 5.4 `commands.test.ts`: 5 new tests for `openspec-ui.rollbackChange` + (confirmed rollback, no-eligible-processes reporting without a + confirmation prompt, declined confirmation, conflict reporting, works + identically for an archived item). Extended `makeDeps()`'s + `implementationSessions` mock with `changeRollbackDetails`/ + `rollbackChange`. 46/46 extension command tests passing. + +## 6. Server: REST parity (no automatic pruning, no new webui UI) + +- [x] 6.1 `recovery-rest.ts`: `handleChangeRollbackRequest` — `POST + /api/processes/rollback-change`, body `{ cwd, changeName }`, same + authorization/cwd-policy pattern as the existing per-process rollback + handler. +- [x] 6.2 `server.ts`: route registration. +- [x] 6.3 `server.test.ts`: real end-to-end test — two real checkpoints + captured/finalized against a real temp workspace, seeded into a real + journal, `POST /api/processes/rollback-change` restores to the + earliest state. 33/33 server tests passing. +- [x] 6.4 No standalone webui button and no automatic retention pruning + on the server in this pass — documented as a deliberate scope decision + in `proposal.md`, matching the `repo-bootstrap-snippets` precedent. + +## 7. Docs, ADR, verification, versioning, smoke test + +- [x] 7.1 `docs/adr/0008-change-scoped-rollback-and-retention.md` — the + SQLite-per-task-diff proposal, the fail-closed-vs-warn-and-restore + question, and the retention-scope decision, all recorded with their + reasoning. +- [x] 7.2 `packages/extension/README.md`: Features bullet for "Rollback + Change"; Settings section entry for + `openspec-ui.checkpointRetentionDays` with an explicit **Warning** that + pruned processes lose Rollback availability permanently and that the + default is to keep everything forever (per user's explicit + documentation request). +- [x] 7.3 `npm run typecheck && npm run lint && npm run test` passes for + `packages/core` (162/162), `packages/server` (39/39), + `packages/extension` (97/97). Re-run `npm run verify` after + `git add`/commit of all new files. +- [x] 7.4 Bumped `package.json` versions (minor — new capability, no + contract break): `@openspec-ui/core` 0.18.0 → 0.19.0, `openspec-ui-vscode` + 0.12.1 → 0.14.0 (0.13.0 deliberately skipped — already claimed by the + concurrent `repo-bootstrap-tree-ui` branch/PR, to avoid an entirely + foreseeable version collision), `@openspec-ui/server` 1.7.0 → 1.8.0. + Extension `CHANGELOG.md` entry, root `README.md` version table. +- [x] 7.5 Manual smoke test: real Extension Host run + (`npm run test:integration`, 6/6 passing); real temp-workspace check + via `tsx` with two sequential real checkpoints against real files — + `rollbackChangeCheckpoints` correctly restored `v2` back to `v0`, not + through mocks. Server-side real end-to-end coverage also added in + `server.test.ts` (task 6.3). +- [x] 7.6 `openspec change validate --strict change-scoped-rollback` + passes. diff --git a/packages/core/package.json b/packages/core/package.json index 870f03bd..07b6b186 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,7 +1,7 @@ { "name": "@openspec-ui/core", "private": true, - "version": "0.18.0", + "version": "0.19.0", "type": "module", "main": "src/index.ts", "exports": { diff --git a/packages/core/src/checkpoint.test.ts b/packages/core/src/checkpoint.test.ts index 77c96d36..84fb9008 100644 --- a/packages/core/src/checkpoint.test.ts +++ b/packages/core/src/checkpoint.test.ts @@ -7,6 +7,7 @@ import { deserializeCheckpoint, finalizeCheckpoint, rollbackCheckpoint, + rollbackChangeCheckpoints, serializeCheckpoint, } from "./checkpoint.js"; @@ -93,3 +94,72 @@ describe("Workbench checkpoints", () => { expect(() => deserializeCheckpoint(serialized)).toThrow("Invalid checkpoint path"); }); }); + +describe("rollbackChangeCheckpoints", () => { + it("restores each file to its state before the earliest checkpoint that touched it", async () => { + const root = await temporaryRoot(); + await writeFile(path.join(root, "shared.txt"), "v0"); + await writeFile(path.join(root, "only-in-first.txt"), "orig1"); + + const checkpoint1 = await captureCheckpoint(root); + await writeFile(path.join(root, "shared.txt"), "v1"); + await rm(path.join(root, "only-in-first.txt")); + await finalizeCheckpoint(checkpoint1); + + const checkpoint2 = await captureCheckpoint(root); + await writeFile(path.join(root, "shared.txt"), "v2"); + await writeFile(path.join(root, "only-in-second.txt"), "new2"); + await finalizeCheckpoint(checkpoint2); + + const result = await rollbackChangeCheckpoints([checkpoint1, checkpoint2]); + + expect(result.conflicts).toEqual([]); + expect(result.restored.sort()).toEqual(["only-in-first.txt", "only-in-second.txt", "shared.txt"]); + expect(await readFile(path.join(root, "shared.txt"), "utf8")).toBe("v0"); + expect(await readFile(path.join(root, "only-in-first.txt"), "utf8")).toBe("orig1"); + await expect(readFile(path.join(root, "only-in-second.txt"))).rejects.toThrow(); + }); + + it("restores correctly regardless of array order (sorts by checkpoint.createdAt)", async () => { + const root = await temporaryRoot(); + await writeFile(path.join(root, "shared.txt"), "v0"); + const checkpoint1 = await captureCheckpoint(root); + await writeFile(path.join(root, "shared.txt"), "v1"); + await finalizeCheckpoint(checkpoint1); + const checkpoint2 = await captureCheckpoint(root); + await writeFile(path.join(root, "shared.txt"), "v2"); + await finalizeCheckpoint(checkpoint2); + + const result = await rollbackChangeCheckpoints([checkpoint2, checkpoint1]); + + expect(result.conflicts).toEqual([]); + expect(await readFile(path.join(root, "shared.txt"), "utf8")).toBe("v0"); + }); + + it("refuses the entire restore when any file was changed after the latest known state", async () => { + const root = await temporaryRoot(); + await writeFile(path.join(root, "a.txt"), "a0"); + await writeFile(path.join(root, "b.txt"), "b0"); + const checkpoint = await captureCheckpoint(root); + await writeFile(path.join(root, "a.txt"), "a1"); + await writeFile(path.join(root, "b.txt"), "b1"); + await finalizeCheckpoint(checkpoint); + await writeFile(path.join(root, "b.txt"), "user changed it later"); + + const result = await rollbackChangeCheckpoints([checkpoint]); + + expect(result).toEqual({ restored: [], conflicts: ["b.txt"] }); + expect(await readFile(path.join(root, "a.txt"), "utf8")).toBe("a1"); + expect(await readFile(path.join(root, "b.txt"), "utf8")).toBe("user changed it later"); + }); + + it("throws for an empty checkpoint list", async () => { + await expect(rollbackChangeCheckpoints([])).rejects.toThrow("No checkpoints to roll back"); + }); + + it("throws if a checkpoint was never finalized", async () => { + const root = await temporaryRoot(); + const checkpoint = await captureCheckpoint(root); + await expect(rollbackChangeCheckpoints([checkpoint])).rejects.toThrow("Checkpoint must be finalized before rollback"); + }); +}); diff --git a/packages/core/src/checkpoint.ts b/packages/core/src/checkpoint.ts index a975ba83..8c7460a3 100644 --- a/packages/core/src/checkpoint.ts +++ b/packages/core/src/checkpoint.ts @@ -228,26 +228,82 @@ async function currentHash(filePath: string): Promise { } } -export async function rollbackCheckpoint(checkpoint: WorkbenchCheckpoint): Promise { - if (!checkpoint.after || !checkpoint.delta) throw new Error("Checkpoint must be finalized before rollback"); +/** One file's target restore state: `beforeContent` undefined means the + * file should not exist after restore (delete). `expectedCurrentHash` is + * the hash the file is expected to have on disk right now — a mismatch + * means something changed it after the checkpoint(s) this restore is + * based on, and the whole restore is refused (fail-closed), matching + * `rollbackCheckpoint`'s existing per-process behavior. */ +export interface RestoreEntry { + path: string; + beforeContent?: Buffer; + expectedCurrentHash?: string; +} + +export async function restoreFiles(root: string, entries: RestoreEntry[]): Promise { const conflicts: string[] = []; - for (const delta of checkpoint.delta) { - const absolutePath = path.join(checkpoint.root, delta.path); - if (await currentHash(absolutePath) !== delta.afterHash) conflicts.push(delta.path); + for (const entry of entries) { + const absolutePath = path.join(root, entry.path); + if (await currentHash(absolutePath) !== entry.expectedCurrentHash) conflicts.push(entry.path); } if (conflicts.length > 0) return { restored: [], conflicts }; const restored: string[] = []; - for (const delta of checkpoint.delta) { - const absolutePath = path.join(checkpoint.root, delta.path); - const beforeFile = checkpoint.before.get(delta.path); - if (!beforeFile) { + for (const entry of entries) { + const absolutePath = path.join(root, entry.path); + if (!entry.beforeContent) { await rm(absolutePath, { force: true }); } else { await mkdir(path.dirname(absolutePath), { recursive: true }); - await writeFile(absolutePath, beforeFile.content); + await writeFile(absolutePath, entry.beforeContent); } - restored.push(delta.path); + restored.push(entry.path); } return { restored, conflicts: [] }; } + +export async function rollbackCheckpoint(checkpoint: WorkbenchCheckpoint): Promise { + if (!checkpoint.after || !checkpoint.delta) throw new Error("Checkpoint must be finalized before rollback"); + const entries: RestoreEntry[] = checkpoint.delta.map((delta) => ({ + path: delta.path, + beforeContent: checkpoint.before.get(delta.path)?.content, + expectedCurrentHash: delta.afterHash, + })); + return restoreFiles(checkpoint.root, entries); +} + +/** Aggregates every finalized checkpoint belonging to one Change into a + * single restore: each file's target is its content from the *earliest* + * checkpoint that touched it (i.e. "undo everything ever done under this + * Change"), and the conflict check compares against each file's *latest* + * known `afterHash` — the most recent state this restore actually knows + * about. Same fail-closed, all-or-nothing semantics as + * `rollbackCheckpoint`: any conflict refuses the entire restore, not just + * the conflicting file. Callers (`WorkbenchRecoveryService`, + * `ImplementationSessionManager`) are responsible for selecting which + * checkpoints belong to a Change and are rollback-eligible — this + * function only does the aggregation + restore. */ +export async function rollbackChangeCheckpoints(checkpoints: WorkbenchCheckpoint[]): Promise { + if (checkpoints.length === 0) throw new Error("No checkpoints to roll back"); + const ordered = [...checkpoints].sort((a, b) => Date.parse(a.createdAt) - Date.parse(b.createdAt)); + const root = ordered[0]!.root; + + const earliestBefore = new Map(); + const latestAfterHash = new Map(); + for (const checkpoint of ordered) { + if (!checkpoint.after || !checkpoint.delta) throw new Error("Checkpoint must be finalized before rollback"); + for (const delta of checkpoint.delta) { + if (!earliestBefore.has(delta.path)) { + earliestBefore.set(delta.path, checkpoint.before.get(delta.path)?.content); + } + latestAfterHash.set(delta.path, delta.afterHash); + } + } + + const entries: RestoreEntry[] = [...latestAfterHash.keys()].sort().map((filePath) => ({ + path: filePath, + beforeContent: earliestBefore.get(filePath), + expectedCurrentHash: latestAfterHash.get(filePath), + })); + return restoreFiles(root, entries); +} diff --git a/packages/core/src/process-scheduler.test.ts b/packages/core/src/process-scheduler.test.ts index 3da0406b..295ec0a7 100644 --- a/packages/core/src/process-scheduler.test.ts +++ b/packages/core/src/process-scheduler.test.ts @@ -102,6 +102,18 @@ describe("WorkbenchProcessScheduler", () => { expect(scheduler.list()[0]).toMatchObject({ state: "rolled-back", summary: "one file restored" }); }); + it("removeBefore drops only processes created before the cutoff, in place", () => { + const scheduler = new WorkbenchProcessScheduler([ + { id: "old", operation: "review", mutating: false, state: "completed", createdAt: "2026-01-01T00:00:00.000Z" }, + { id: "new", operation: "review", mutating: false, state: "completed", createdAt: "2026-08-01T00:00:00.000Z" }, + ]); + + const removed = scheduler.removeBefore(new Date("2026-07-01T00:00:00.000Z")); + + expect(removed).toEqual(["old"]); + expect(scheduler.list().map((process) => process.id)).toEqual(["new"]); + }); + it("restores unfinished processes as interrupted", () => { const scheduler = new WorkbenchProcessScheduler([{ id: "active", diff --git a/packages/core/src/process-scheduler.ts b/packages/core/src/process-scheduler.ts index 7a737272..b8f78a05 100644 --- a/packages/core/src/process-scheduler.ts +++ b/packages/core/src/process-scheduler.ts @@ -92,6 +92,24 @@ export class WorkbenchProcessScheduler { return true; } + /** Removes processes created before `cutoff` in place, returning their + * ids — for retention (`openspec-ui.checkpointRetentionDays`). Prefer + * this over reconstructing the scheduler when the instance is shared + * across other long-lived references (VS Code extension activation + * wires this scheduler into several tree providers and the chat + * participant by reference). */ + removeBefore(cutoff: Date): string[] { + const removed: string[] = []; + for (const [id, process] of this.processes) { + if (Date.parse(process.createdAt) < cutoff.getTime()) { + this.processes.delete(id); + removed.push(id); + } + } + if (removed.length > 0) this.emit(); + return removed; + } + start(options: StartProcessOptions): WorkbenchProcessHandle { const id = options.id ?? crypto.randomUUID(); if (this.processes.has(id)) throw new Error(`Workbench process already exists: ${id}`); diff --git a/packages/core/src/workbench-recovery.test.ts b/packages/core/src/workbench-recovery.test.ts index 1e7e529e..38567e96 100644 --- a/packages/core/src/workbench-recovery.test.ts +++ b/packages/core/src/workbench-recovery.test.ts @@ -60,6 +60,44 @@ describe("WorkbenchRecoveryService", () => { expect(service.list()[0]?.state).toBe("rolled-back"); }); + it("rolls back every process for a change across multiple checkpoints, to the earliest state", async () => { + const root = await createRoot(); + const filePath = path.join(root, "shared.txt"); + await writeFile(filePath, "v0", "utf8"); + const checkpoint1 = await captureCheckpoint(root); + await writeFile(filePath, "v1", "utf8"); + await finalizeCheckpoint(checkpoint1); + const checkpoint2 = await captureCheckpoint(root); + await writeFile(filePath, "v2", "utf8"); + await finalizeCheckpoint(checkpoint2); + await new WorkbenchRunJournal(root).save({ + processes: [ + { id: "run-1", operation: "implement", changeName: "demo-change", mutating: true, state: "completed", createdAt: "2026-08-01T00:00:00.000Z" }, + { id: "run-2", operation: "implement", changeName: "demo-change", mutating: true, state: "completed", createdAt: "2026-08-02T00:00:00.000Z" }, + ], + checkpointSessions: [ + { processId: "run-1", changeName: "demo-change", checkpoint: serializeCheckpoint(checkpoint1) }, + { processId: "run-2", changeName: "demo-change", checkpoint: serializeCheckpoint(checkpoint2) }, + ], + }); + + const service = await WorkbenchRecoveryService.open(root); + expect(service.changeRollbackDetails("demo-change")).toEqual({ processCount: 2, fileCount: 1 }); + + await expect(service.rollbackChange("demo-change")).resolves.toEqual({ restored: ["shared.txt"], conflicts: [] }); + expect(await readFile(filePath, "utf8")).toBe("v0"); + expect(service.list().map((process) => process.state)).toEqual(["rolled-back", "rolled-back"]); + }); + + it("throws for a change with no rollback-eligible processes", async () => { + const root = await createRoot(); + const service = await WorkbenchRecoveryService.open(root); + await expect(service.rollbackChange("nonexistent")).rejects.toThrow( + 'No rollback-eligible processes for change "nonexistent"', + ); + expect(service.changeRollbackDetails("nonexistent")).toBeUndefined(); + }); + it("cleans old processes and their checkpoint sessions together", async () => { const root = await createRoot(); const checkpoint = await captureCheckpoint(root); diff --git a/packages/core/src/workbench-recovery.ts b/packages/core/src/workbench-recovery.ts index b970d693..4adfb53f 100644 --- a/packages/core/src/workbench-recovery.ts +++ b/packages/core/src/workbench-recovery.ts @@ -2,6 +2,7 @@ import { deserializeCheckpoint, finalizeCheckpoint, rollbackCheckpoint, + rollbackChangeCheckpoints, serializeCheckpoint, type CheckpointCoverage, type CheckpointDelta, @@ -82,6 +83,55 @@ export class WorkbenchRecoveryService { return result; } + /** Every rollback-eligible process belonging to `changeName`, active or + * archived — archive status lives entirely in OpenSpec's own change + * folders, never in the checkpoint/journal system, so it plays no part + * in eligibility here (see `rollbackChange`'s own JSDoc). */ + private changeRollbackCandidates(changeName: string): WorkbenchProcess[] { + return [...this.sessions.values()] + .filter((session) => session.changeName === changeName && session.checkpoint.after && session.checkpoint.delta) + .map((session) => this.scheduler.list().find((process) => process.id === session.processId)) + .filter((process): process is WorkbenchProcess => + process !== undefined && ["completed", "failed", "interrupted"].includes(process.state)); + } + + /** File count and process count a `rollbackChange` call for this + * changeName would affect, without performing the restore — for a + * confirmation prompt before the destructive call. */ + changeRollbackDetails(changeName: string): { processCount: number; fileCount: number } | undefined { + const processes = this.changeRollbackCandidates(changeName); + if (processes.length === 0) return undefined; + const paths = new Set(); + for (const process of processes) { + const checkpoint = this.sessions.get(process.id)?.checkpoint; + for (const delta of checkpoint?.delta ?? []) paths.add(delta.path); + } + return { processCount: processes.length, fileCount: paths.size }; + } + + /** Rolls back every process ever run against `changeName` — active or + * already archived — restoring every touched file to its state before + * the earliest of those runs. Archiving a change never touches + * checkpoint/journal data (it's a thin `openspec archive` CLI wrapper + * scoped to `openspec/changes/`), so nothing here needs an + * archived-change guard; this is deliberately the simpler alternative + * to task-scoped rollback, discussed and chosen over a new + * SQLite-backed per-task diff store — see + * openspec/changes/change-scoped-rollback/proposal.md. */ + async rollbackChange(changeName: string): Promise { + const processes = this.changeRollbackCandidates(changeName); + if (processes.length === 0) throw new Error(`No rollback-eligible processes for change "${changeName}"`); + const checkpoints = processes.map((process) => this.sessions.get(process.id)!.checkpoint); + const result = await rollbackChangeCheckpoints(checkpoints); + if (result.conflicts.length === 0) { + for (const process of processes) { + this.scheduler.markRolledBack(process.id, `change "${changeName}" rolled back: ${result.restored.length} files restored`); + } + await this.persist(); + } + return result; + } + async cleanupBefore(cutoff: Date): Promise { if (Number.isNaN(cutoff.getTime())) throw new Error("Invalid cleanup cutoff"); const current = this.scheduler.list(); diff --git a/packages/extension/CHANGELOG.md b/packages/extension/CHANGELOG.md index 55894330..d3537769 100644 --- a/packages/extension/CHANGELOG.md +++ b/packages/extension/CHANGELOG.md @@ -1,5 +1,20 @@ # Changelog +## 0.14.0 + +- Added "Rollback Change" on a Change item (in either the Changes or + Archive tree): rolls back every process ever run against that Change, + restoring every touched file to its state before the earliest of those + runs — works identically for active and archived changes. Same fail- + closed behavior as single-process rollback: any file changed outside + what the system knows about refuses the entire restore. +- Added `openspec-ui.checkpointRetentionDays` setting (default `0` = + keep forever, unchanged from prior versions). A positive value prunes + process/checkpoint history older than that many days once, on the next + window reload — pruning permanently removes Rollback availability for + the pruned processes, disclosed in the setting description and in this + README. + ## 0.12.1 - "Delete Task" is no longer offered for a task marked done, even in an diff --git a/packages/extension/README.md b/packages/extension/README.md index cf164bdc..859e62d0 100644 --- a/packages/extension/README.md +++ b/packages/extension/README.md @@ -27,6 +27,12 @@ notifications while keeping lifecycle and security behavior in VS Code Agent implementation sessions, finish them for review, and roll back only files changed by a run. Rollback refuses to overwrite later edits and discloses files omitted by checkpoint size limits. +- "Rollback Change" on a Change item (in either the Changes or Archive tree) + rolls back every process ever run against that Change, restoring every + touched file to its state before the earliest of those runs — works + identically for active and archived changes. Same fail-closed behavior as + single-process rollback: any file changed outside what the system knows + about refuses the entire restore, not just that one file. - Open the Process Dashboard with Workspace root and Change directory filled from the current VS Code workspace. The default message-bridge dashboard follows the active VS Code color theme, including dark and high-contrast @@ -116,6 +122,16 @@ how this differs from the `@openspec` Chat Participant above. directly and uses an in-process message bridge. Because the optional localhost shell is a cross-origin iframe, it retains the standalone palette instead of inheriting VS Code theme variables. +- `openspec-ui.checkpointRetentionDays`: days to keep process/checkpoint + history before it's pruned, once, on the next window reload. Defaults to + `0`, which keeps everything forever — unchanged from every prior version. + A positive number prunes anything older than that many days. + + > **Warning:** once a process is pruned this way, both single-process + > Rollback and "Rollback Change" become permanently unavailable for it — + > pruning itself cannot be undone. Leave this at `0` (the default) unless + > you specifically want bounded history growth and have accepted that + > trade-off. ## Development diff --git a/packages/extension/package.json b/packages/extension/package.json index 038943c9..db1a058d 100644 --- a/packages/extension/package.json +++ b/packages/extension/package.json @@ -3,7 +3,7 @@ "displayName": "OpenSpec Workbench", "description": "Native OpenSpec workflow with lifecycle controls, VS Code Chat, persistent processes, checkpoints, and conflict-safe rollback.", "publisher": "openspec-ui", - "version": "0.12.1", + "version": "0.14.0", "icon": "media/icon.png", "license": "MIT", "repository": { @@ -147,6 +147,11 @@ "command": "openspec-ui.rollbackProcess", "title": "OpenSpec UI: Rollback Process Changes", "icon": "$(discard)" + }, + { + "command": "openspec-ui.rollbackChange", + "title": "OpenSpec UI: Rollback Change", + "icon": "$(discard)" } ], "viewsContainers": { @@ -262,6 +267,11 @@ "when": "viewItem == openspec-ui.archivedChange", "group": "openspec@2" }, + { + "command": "openspec-ui.rollbackChange", + "when": "viewItem == openspec-ui.activeChange || viewItem == openspec-ui.archivedChange", + "group": "openspec@3" + }, { "command": "openspec-ui.customizeTemplate", "when": "viewItem == openspec-ui.builtInTemplate", @@ -311,6 +321,11 @@ "type": "boolean", "default": false, "description": "Run an optional local REST/WS server (same package as the standalone tool) and point the AI panel webview at it instead of the default in-process message bridge." + }, + "openspec-ui.checkpointRetentionDays": { + "type": "number", + "default": 0, + "description": "Days to keep process/checkpoint history before it's pruned on the next window reload. 0 or a negative number keeps everything forever (the default, unchanged from prior versions). WARNING: once a process is pruned, Rollback (both single-process and whole-Change) is permanently unavailable for it — pruning is not itself reversible." } } }, diff --git a/packages/extension/src/commands.test.ts b/packages/extension/src/commands.test.ts index ecb286e9..f6b4e960 100644 --- a/packages/extension/src/commands.test.ts +++ b/packages/extension/src/commands.test.ts @@ -97,6 +97,8 @@ function makeDeps(overrides: Partial[1]> = { getDelta: vi.fn(() => undefined), getCoverage: vi.fn(() => undefined), rollback: vi.fn(), + changeRollbackDetails: vi.fn(() => undefined), + rollbackChange: vi.fn(), }; return { getWorkspaceRoot: () => "/workspace/repo", @@ -137,6 +139,7 @@ describe("registerCommands", () => { "openspec-ui.listSpecsSummary", "openspec-ui.openAiPanel", "openspec-ui.reviewDiff", + "openspec-ui.rollbackChange", ]), ); }); @@ -184,6 +187,102 @@ describe("registerCommands", () => { expect(deps.refreshTrees).toHaveBeenCalledTimes(2); }); + describe("openspec-ui.rollbackChange", () => { + const changeItem = { changeName: "demo-change", archived: false }; + + it("rolls back after confirmation and refreshes trees", async () => { + const deps = makeDeps(); + (deps.implementationSessions.changeRollbackDetails as ReturnType).mockReturnValue({ + processCount: 2, + fileCount: 3, + }); + (deps.implementationSessions.rollbackChange as ReturnType).mockResolvedValue({ + restored: ["a.txt", "b.txt", "c.txt"], + conflicts: [], + }); + vscodeMock.window.showWarningMessage.mockResolvedValue("Rollback"); + registerCommands(makeContext() as unknown as import("vscode").ExtensionContext, deps); + + await vscodeMock._registeredCommands.get("openspec-ui.rollbackChange")?.(changeItem); + + expect(deps.implementationSessions.rollbackChange).toHaveBeenCalledWith("demo-change"); + expect(deps.refreshTrees).toHaveBeenCalled(); + }); + + it("reports no rollback-eligible processes instead of showing a confirmation", async () => { + const deps = makeDeps(); + registerCommands(makeContext() as unknown as import("vscode").ExtensionContext, deps); + + await vscodeMock._registeredCommands.get("openspec-ui.rollbackChange")?.(changeItem); + + expect(vscodeMock.window.showWarningMessage).toHaveBeenCalledWith( + expect.stringContaining("no rollback-eligible processes"), + ); + expect(vscodeMock.window.showWarningMessage).not.toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ modal: true }), + expect.anything(), + ); + expect(deps.implementationSessions.rollbackChange).not.toHaveBeenCalled(); + }); + + it("does not roll back when the confirmation is declined", async () => { + const deps = makeDeps(); + (deps.implementationSessions.changeRollbackDetails as ReturnType).mockReturnValue({ + processCount: 1, + fileCount: 1, + }); + vscodeMock.window.showWarningMessage.mockResolvedValue(undefined); + registerCommands(makeContext() as unknown as import("vscode").ExtensionContext, deps); + + await vscodeMock._registeredCommands.get("openspec-ui.rollbackChange")?.(changeItem); + + expect(deps.implementationSessions.rollbackChange).not.toHaveBeenCalled(); + }); + + it("reports conflicts as an error instead of refreshing trees", async () => { + const deps = makeDeps(); + (deps.implementationSessions.changeRollbackDetails as ReturnType).mockReturnValue({ + processCount: 1, + fileCount: 1, + }); + (deps.implementationSessions.rollbackChange as ReturnType).mockResolvedValue({ + restored: [], + conflicts: ["a.txt"], + }); + vscodeMock.window.showWarningMessage.mockResolvedValue("Rollback"); + registerCommands(makeContext() as unknown as import("vscode").ExtensionContext, deps); + + await vscodeMock._registeredCommands.get("openspec-ui.rollbackChange")?.(changeItem); + + expect(vscodeMock.window.showErrorMessage).toHaveBeenCalledWith( + expect.stringContaining("a.txt"), + ); + expect(deps.refreshTrees).not.toHaveBeenCalled(); + }); + + it("works the same for an archived change item", async () => { + const deps = makeDeps(); + (deps.implementationSessions.changeRollbackDetails as ReturnType).mockReturnValue({ + processCount: 1, + fileCount: 1, + }); + (deps.implementationSessions.rollbackChange as ReturnType).mockResolvedValue({ + restored: ["a.txt"], + conflicts: [], + }); + vscodeMock.window.showWarningMessage.mockResolvedValue("Rollback"); + registerCommands(makeContext() as unknown as import("vscode").ExtensionContext, deps); + + await vscodeMock._registeredCommands.get("openspec-ui.rollbackChange")?.({ + changeName: "archived-change", + archived: true, + }); + + expect(deps.implementationSessions.rollbackChange).toHaveBeenCalledWith("archived-change"); + }); + }); + it("openspec-ui.status: picks a change, runs direct OpenSpec status flow, and reveals the panel", async () => { listChangesMock.mockResolvedValue({ changes: [{ name: "shared-ui", completedTasks: 1, totalTasks: 17, lastModified: "t", status: "in-progress" }], diff --git a/packages/extension/src/commands.ts b/packages/extension/src/commands.ts index d40c726a..b607f441 100644 --- a/packages/extension/src/commands.ts +++ b/packages/extension/src/commands.ts @@ -664,6 +664,31 @@ export function registerCommands(context: vscode.ExtensionContext, deps: Command await showCommandError("rollback", error); } }), + vscode.commands.registerCommand("openspec-ui.rollbackChange", async (item?: ChangeTreeItem) => { + if (!item) return; + const details = deps.implementationSessions.changeRollbackDetails(item.changeName); + if (!details) { + void vscode.window.showWarningMessage(`OpenSpec UI: no rollback-eligible processes for ${item.changeName}.`); + return; + } + const answer = await vscode.window.showWarningMessage( + `Rollback ${item.changeName}? This restores ${details.fileCount} file${details.fileCount === 1 ? "" : "s"} across ${details.processCount} process${details.processCount === 1 ? "" : "es"} to their state before this change was ever implemented.`, + { modal: true }, + "Rollback", + ); + if (answer !== "Rollback") return; + try { + const result = await deps.implementationSessions.rollbackChange(item.changeName); + if (result.conflicts.length > 0) { + void vscode.window.showErrorMessage(`OpenSpec UI: rollback blocked by later changes: ${result.conflicts.join(", ")}`); + return; + } + deps.refreshTrees(); + void vscode.window.showInformationMessage(`OpenSpec UI: restored ${result.restored.length} files.`); + } catch (error) { + await showCommandError("rollback change", error); + } + }), vscode.commands.registerCommand("openspec-ui.cancelProcess", (item?: { process?: { id?: string } }) => { if (item?.process?.id) deps.implementationSessions.cancel(item.process.id); }), diff --git a/packages/extension/src/extension.ts b/packages/extension/src/extension.ts index 67038802..a26880fe 100644 --- a/packages/extension/src/extension.ts +++ b/packages/extension/src/extension.ts @@ -69,6 +69,17 @@ export async function activate(context: vscode.ExtensionContext): Promise("checkpointRetentionDays", 0); + if (retentionDays > 0) { + const cutoff = new Date(Date.now() - retentionDays * 24 * 60 * 60 * 1000); + implementationSessions.dropSessions(scheduler.removeBefore(cutoff)); + } const processesTree = new ProcessesTreeProvider(scheduler); context.subscriptions.push( processesTree, diff --git a/packages/extension/src/implementation-sessions.test.ts b/packages/extension/src/implementation-sessions.test.ts index c1bf4ff2..c4f345ae 100644 --- a/packages/extension/src/implementation-sessions.test.ts +++ b/packages/extension/src/implementation-sessions.test.ts @@ -95,4 +95,60 @@ describe("ImplementationSessionManager", () => { expect((await manager.rollback(process.id)).conflicts).toEqual([]); expect(await readFile(filePath, "utf8")).toBe("before"); }); + + it("rolls back every session for a change across two runs, to the earliest state", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "openspec-session-")); + roots.push(root); + const filePath = path.join(root, "shared.txt"); + await writeFile(filePath, "v0"); + const scheduler = new WorkbenchProcessScheduler(); + const manager = new ImplementationSessionManager(scheduler); + + await manager.run(root, { + operation: "implement", + changeName: "demo", + mutating: true, + execute: async () => { await writeFile(filePath, "v1"); }, + }); + await manager.run(root, { + operation: "implement", + changeName: "demo", + mutating: true, + execute: async () => { await writeFile(filePath, "v2"); }, + }); + + expect(manager.changeRollbackDetails("demo")).toEqual({ processCount: 2, fileCount: 1 }); + await expect(manager.rollbackChange("demo")).resolves.toEqual({ restored: ["shared.txt"], conflicts: [] }); + expect(await readFile(filePath, "utf8")).toBe("v0"); + }); + + it("throws rollbackChange for a change with no rollback-eligible sessions", async () => { + const scheduler = new WorkbenchProcessScheduler(); + const manager = new ImplementationSessionManager(scheduler); + await expect(manager.rollbackChange("nonexistent")).rejects.toThrow( + 'No rollback-eligible processes for change "nonexistent"', + ); + expect(manager.changeRollbackDetails("nonexistent")).toBeUndefined(); + }); + + it("dropSessions removes a session so it no longer participates in change rollback", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "openspec-session-")); + roots.push(root); + const filePath = path.join(root, "code.ts"); + await writeFile(filePath, "before"); + const scheduler = new WorkbenchProcessScheduler(); + const manager = new ImplementationSessionManager(scheduler); + + const process = await manager.run(root, { + operation: "implement", + changeName: "demo", + mutating: true, + execute: async () => { await writeFile(filePath, "after"); }, + }); + + manager.dropSessions([process.id]); + + expect(manager.getDelta(process.id)).toBeUndefined(); + expect(manager.changeRollbackDetails("demo")).toBeUndefined(); + }); }); diff --git a/packages/extension/src/implementation-sessions.ts b/packages/extension/src/implementation-sessions.ts index cfe8837c..1bd14dea 100644 --- a/packages/extension/src/implementation-sessions.ts +++ b/packages/extension/src/implementation-sessions.ts @@ -3,10 +3,12 @@ import { deserializeCheckpoint, finalizeCheckpoint, rollbackCheckpoint, + rollbackChangeCheckpoints, serializeCheckpoint, type CheckpointDelta, type CheckpointCoverage, type PersistedCheckpointSession, + type RollbackResult, type StartProcessOptions, type WorkbenchCheckpoint, type WorkbenchProcess, @@ -144,6 +146,47 @@ export class ImplementationSessionManager { return result; } + /** Every rollback-eligible session belonging to `changeName`, active or + * archived — same eligibility rule as `WorkbenchRecoveryService`'s + * mirror of this method (`@openspec-ui/core`), duplicated here because + * the extension's primary mode keeps its own session map instead of + * going through that service (see module header). */ + private changeRollbackCandidates(changeName: string): CheckpointSession[] { + return [...this.sessions.values()] + .filter((session) => session.changeName === changeName && session.delta) + .map((session) => ({ session, process: this.scheduler.list().find((candidate) => candidate.id === session.processId) })) + .filter((entry): entry is { session: CheckpointSession; process: WorkbenchProcess } => + entry.process !== undefined && ["completed", "failed", "interrupted"].includes(entry.process.state)) + .map((entry) => entry.session); + } + + changeRollbackDetails(changeName: string): { processCount: number; fileCount: number } | undefined { + const sessions = this.changeRollbackCandidates(changeName); + if (sessions.length === 0) return undefined; + const paths = new Set(); + for (const session of sessions) for (const delta of session.delta ?? []) paths.add(delta.path); + return { processCount: sessions.length, fileCount: paths.size }; + } + + async rollbackChange(changeName: string): Promise { + const sessions = this.changeRollbackCandidates(changeName); + if (sessions.length === 0) throw new Error(`No rollback-eligible processes for change "${changeName}"`); + const result = await rollbackChangeCheckpoints(sessions.map((session) => session.checkpoint)); + if (result.conflicts.length === 0) { + for (const session of sessions) { + this.scheduler.markRolledBack(session.processId, `change "${changeName}" rolled back: ${result.restored.length} files restored`); + } + } + return result; + } + + /** Retention pruning (`openspec-ui.checkpointRetentionDays`) — drops + * sessions whose process was removed from the scheduler by + * `WorkbenchProcessScheduler.removeBefore`. */ + dropSessions(processIds: readonly string[]): void { + for (const id of processIds) this.sessions.delete(id); + } + getDelta(processId: string): CheckpointDelta[] | undefined { return this.sessions.get(processId)?.delta?.map((item) => ({ ...item })); } diff --git a/packages/server/package.json b/packages/server/package.json index e6f41174..2bd76066 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -1,7 +1,7 @@ { "name": "@openspec-ui/server", "private": true, - "version": "1.7.0", + "version": "1.8.0", "type": "module", "main": "src/index.ts", "dependencies": { diff --git a/packages/server/src/recovery-rest.ts b/packages/server/src/recovery-rest.ts index b3e2225c..42e343f8 100644 --- a/packages/server/src/recovery-rest.ts +++ b/packages/server/src/recovery-rest.ts @@ -12,6 +12,7 @@ interface RecoveryRequest { cwd: string; processId?: string; cutoff?: string; + changeName?: string; } export type RecoveryServiceResolver = (cwd: string) => Promise; @@ -111,6 +112,18 @@ export function handleProcessRollbackRequest( }); } +export function handleChangeRollbackRequest( + req: IncomingMessage, + res: ServerResponse, + policy: RestRequestPolicy, + resolveService: RecoveryServiceResolver, +): Promise { + return withRecoveryRequest(req, res, policy, resolveService, (request, service) => { + if (!request.changeName) throw new Error("changeName is required"); + return service.rollbackChange(request.changeName); + }); +} + export function handleProcessesCleanupRequest( req: IncomingMessage, res: ServerResponse, diff --git a/packages/server/src/server.test.ts b/packages/server/src/server.test.ts index 56860434..3cac46f3 100644 --- a/packages/server/src/server.test.ts +++ b/packages/server/src/server.test.ts @@ -12,6 +12,9 @@ import WebSocket from "ws"; import { OpenSpecCliCompatibilityError, WorkbenchRunJournal, + captureCheckpoint, + finalizeCheckpoint, + serializeCheckpoint, type AgentRunner, type Command, type Event, @@ -606,6 +609,38 @@ describe("server — REST /api/status", () => { expect((await new WorkbenchRunJournal(cwd).load()).processes).toEqual([]); }); + it("rolls back every process for a change across two checkpoints, to the earliest state", async () => { + const cwd = await createTempWorkspace(); + const filePath = path.join(cwd, "shared.txt"); + await writeFile(filePath, "v0", "utf8"); + const checkpoint1 = await captureCheckpoint(cwd); + await writeFile(filePath, "v1", "utf8"); + await finalizeCheckpoint(checkpoint1); + const checkpoint2 = await captureCheckpoint(cwd); + await writeFile(filePath, "v2", "utf8"); + await finalizeCheckpoint(checkpoint2); + await new WorkbenchRunJournal(cwd).save({ + processes: [ + { id: "run-1", operation: "implement", changeName: "demo-change", mutating: true, state: "completed", createdAt: "2026-08-01T00:00:00.000Z" }, + { id: "run-2", operation: "implement", changeName: "demo-change", mutating: true, state: "completed", createdAt: "2026-08-02T00:00:00.000Z" }, + ], + checkpointSessions: [ + { processId: "run-1", changeName: "demo-change", checkpoint: serializeCheckpoint(checkpoint1) }, + { processId: "run-2", changeName: "demo-change", checkpoint: serializeCheckpoint(checkpoint2) }, + ], + }); + + const response = await fetch(`${baseUrl}/api/processes/rollback-change`, { + method: "POST", + headers: JSON_HEADERS, + body: JSON.stringify({ cwd, changeName: "demo-change" }), + }); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ restored: ["shared.txt"], conflicts: [] }); + expect(await readFile(filePath, "utf8")).toBe("v0"); + }); + it("returns actionable diagnostics without replacing a future run journal", async () => { const cwd = await createTempWorkspace(); const journal = new WorkbenchRunJournal(cwd); diff --git a/packages/server/src/server.ts b/packages/server/src/server.ts index d54aef1e..60fd7dc7 100644 --- a/packages/server/src/server.ts +++ b/packages/server/src/server.ts @@ -25,6 +25,7 @@ import { import { handleSocketMessage } from "./websocket.js"; import { tryServeStatic, type StaticAssetPaths } from "./static.js"; import { + handleChangeRollbackRequest, handleProcessDetailsRequest, handleProcessRollbackRequest, handleProcessesCleanupRequest, @@ -177,6 +178,10 @@ export function createServer(options: ServerOptions): OpenSpecUiServer { void handleProcessRollbackRequest(req, res, requestPolicy, resolveRecoveryService); return; } + if (req.method === "POST" && req.url === "/api/processes/rollback-change") { + void handleChangeRollbackRequest(req, res, requestPolicy, resolveRecoveryService); + return; + } if (req.method === "POST" && req.url === "/api/processes/cleanup") { void handleProcessesCleanupRequest(req, res, requestPolicy, resolveRecoveryService); return; From 3dcb679d95aa057e9fcc4402b8a8b818704d85f1 Mon Sep 17 00:00:00 2001 From: Alexander Ivanov Date: Tue, 18 Aug 2026 12:49:14 +0300 Subject: [PATCH 2/2] Fix race condition in rollbackChangeCheckpoints ordering (CI-caught) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed on a fast runner: two checkpoints captured back to back can share the same millisecond in createdAt (new Date().toISOString() resolution), and sorting by Date.parse(createdAt) then leaves tied entries in their original array position via V8's stable sort rather than a meaningful chronological one — silently aggregating before/ after state from the wrong checkpoint. Caught as a spurious conflict in the order-independence test; in a real near-simultaneous scenario it could instead silently restore the wrong content. Fixed by removing the internal sort: the function now trusts caller-provided order (earliest-first) instead of re-deriving it from a lossy timestamp. Both real callers already provide correct order for free via Map iteration order (insertion order = capture order). Rewrote the affected test to match: correct order still works, out-of-order input now fails closed with a conflict instead of silently restoring the wrong content. Co-Authored-By: Claude Sonnet 5 --- .../changes/change-scoped-rollback/tasks.md | 28 +++++++++++++++++++ packages/core/src/checkpoint.test.ts | 19 ++++++++++--- packages/core/src/checkpoint.ts | 17 ++++++++--- 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/openspec/changes/change-scoped-rollback/tasks.md b/openspec/changes/change-scoped-rollback/tasks.md index 19dc11f1..01400350 100644 --- a/openspec/changes/change-scoped-rollback/tasks.md +++ b/openspec/changes/change-scoped-rollback/tasks.md @@ -114,3 +114,31 @@ `server.test.ts` (task 6.3). - [x] 7.6 `openspec change validate --strict change-scoped-rollback` passes. + +## 8. CI-caught race condition, fixed post-review + +- [x] 8.1 A CI run on a fast runner failed `checkpoint.test.ts`'s + order-independence test with `expected ['shared.txt'] to deeply equal + []` — a genuine bug, not flakiness: `rollbackChangeCheckpoints` sorted + input checkpoints by `Date.parse(checkpoint.createdAt)`, and + `createdAt` (`new Date().toISOString()`) only has millisecond + resolution. Two checkpoints captured back to back can tie, and V8's + stable sort then leaves tied entries in their *original* array + position rather than a meaningful chronological one — silently + aggregating `before`/`after` state from the wrong checkpoint and + surfacing as a spurious conflict (or, worse in a real scenario, a + silently wrong restore instead of a caught conflict). +- [x] 8.2 Fixed by removing the internal sort entirely: the function now + documents and trusts caller-provided order (earliest-first) instead of + re-deriving it from a lossy timestamp. Both real callers + (`WorkbenchRecoveryService.changeRollbackCandidates`, + `ImplementationSessionManager.changeRollbackCandidates`) already + provide correct order for free — they build their checkpoint list from + a `Map`'s iteration order, which is insertion order, which is capture + order. +- [x] 8.3 `checkpoint.test.ts`'s order test rewritten to match the new + contract: correct (earliest-first) order still restores correctly; + passing checkpoints out of order now fails closed with a conflict + rather than silently restoring the wrong content. Re-ran + `packages/core`'s full suite (162/162) plus a standalone run of + `checkpoint.test.ts` (9/9) and `workbench-recovery.test.ts` (5/5). diff --git a/packages/core/src/checkpoint.test.ts b/packages/core/src/checkpoint.test.ts index 84fb9008..b5126632 100644 --- a/packages/core/src/checkpoint.test.ts +++ b/packages/core/src/checkpoint.test.ts @@ -120,7 +120,12 @@ describe("rollbackChangeCheckpoints", () => { await expect(readFile(path.join(root, "only-in-second.txt"))).rejects.toThrow(); }); - it("restores correctly regardless of array order (sorts by checkpoint.createdAt)", async () => { + it("trusts the caller's array order rather than re-sorting by createdAt", async () => { + // createdAt has millisecond resolution and can tie between two + // checkpoints captured back to back (confirmed live on a fast CI + // runner) — sorting by it would then be unreliable with no error. + // The function trusts input order instead; both real callers already + // provide it for free via Map iteration order (insertion order). const root = await temporaryRoot(); await writeFile(path.join(root, "shared.txt"), "v0"); const checkpoint1 = await captureCheckpoint(root); @@ -130,10 +135,16 @@ describe("rollbackChangeCheckpoints", () => { await writeFile(path.join(root, "shared.txt"), "v2"); await finalizeCheckpoint(checkpoint2); - const result = await rollbackChangeCheckpoints([checkpoint2, checkpoint1]); - - expect(result.conflicts).toEqual([]); + const inOrder = await rollbackChangeCheckpoints([checkpoint1, checkpoint2]); + expect(inOrder.conflicts).toEqual([]); expect(await readFile(path.join(root, "shared.txt"), "utf8")).toBe("v0"); + + // Passing them out of order doesn't silently restore the wrong + // content — it fails closed with a conflict, same as any other + // unexpected on-disk state. + await writeFile(path.join(root, "shared.txt"), "v2"); + const outOfOrder = await rollbackChangeCheckpoints([checkpoint2, checkpoint1]); + expect(outOfOrder.conflicts).toEqual(["shared.txt"]); }); it("refuses the entire restore when any file was changed after the latest known state", async () => { diff --git a/packages/core/src/checkpoint.ts b/packages/core/src/checkpoint.ts index 8c7460a3..8a00c8f5 100644 --- a/packages/core/src/checkpoint.ts +++ b/packages/core/src/checkpoint.ts @@ -282,15 +282,24 @@ export async function rollbackCheckpoint(checkpoint: WorkbenchCheckpoint): Promi * the conflicting file. Callers (`WorkbenchRecoveryService`, * `ImplementationSessionManager`) are responsible for selecting which * checkpoints belong to a Change and are rollback-eligible — this - * function only does the aggregation + restore. */ + * function only does the aggregation + restore. + * + * `checkpoints` MUST already be in chronological (earliest-first) order. + * This deliberately does not re-sort by `createdAt`: that field has + * millisecond resolution and two checkpoints captured in the same + * millisecond (confirmed live — a fast CI runner tripped this) would tie, + * making a sort-based order silently wrong with no error. Both real + * callers already provide correct order for free — they build this list + * from a `Map`'s iteration order, which is insertion order, which is + * capture order — so trusting the caller is both simpler and more + * reliable than re-deriving order from a lossy timestamp. */ export async function rollbackChangeCheckpoints(checkpoints: WorkbenchCheckpoint[]): Promise { if (checkpoints.length === 0) throw new Error("No checkpoints to roll back"); - const ordered = [...checkpoints].sort((a, b) => Date.parse(a.createdAt) - Date.parse(b.createdAt)); - const root = ordered[0]!.root; + const root = checkpoints[0]!.root; const earliestBefore = new Map(); const latestAfterHash = new Map(); - for (const checkpoint of ordered) { + for (const checkpoint of checkpoints) { if (!checkpoint.after || !checkpoint.delta) throw new Error("Checkpoint must be finalized before rollback"); for (const delta of checkpoint.delta) { if (!earliestBefore.has(delta.path)) {