Skip to content

Commit 3c4896c

Browse files
DaubnerFDaubnerF
committed
Merge remote-tracking branch 'upstream/main' into fix/1939-scoped-command-lists
2 parents 9736c1d + bb6cbee commit 3c4896c

36 files changed

Lines changed: 2182 additions & 193 deletions

‎.github/workflows/e2e.yml‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,18 @@ jobs:
162162
if: github.event_name != 'pull_request' || steps.e2e-marker.outputs.cache-hit != 'true'
163163
run: TEST_FILE=restart-persistence.test USE_MOCK=true xvfb-run -a pnpm --filter @roo-code/vscode-e2e exec node ./out/runTest.js
164164

165+
- name: Upload restart-persistence failure artifacts
166+
# runTest.js preserves a failed scenario workspace (phase results,
167+
# VS Code logs, task files) under apps/vscode-e2e/test-failures/
168+
# instead of deleting it, so the interrupted-write state from
169+
# issue #2130-style failures stays inspectable.
170+
if: failure()
171+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
172+
with:
173+
name: restart-persistence-failure
174+
path: apps/vscode-e2e/test-failures/
175+
if-no-files-found: ignore
176+
165177
- name: Explain skipped mocked E2E pass marker
166178
if: steps.e2e-marker.outputs.cache-hit != 'true' && steps.run-e2e.outcome == 'success' && steps.vscode-fallback.outputs.used == 'true'
167179
run: echo "Skipping mocked E2E pass marker because tests ran against a stale cached VS Code binary (VS Code download endpoints unreachable)."

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ AGENTS.local.md
2828
apps/vscode-e2e/playwright-report-electron/
2929
apps/vscode-e2e/test-results-electron/
3030
apps/vscode-e2e/heap-results/
31+
apps/vscode-e2e/test-failures/
3132

3233
# Docs
3334
docs/_site/

‎apps/vscode-e2e/src/restart/scenarioWorkspace.ts‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,56 @@ export async function removeScenarioWorkspace(scenarioWorkspace: ScenarioWorkspa
5656

5757
await fs.rm(root, { recursive: true, force: true })
5858
}
59+
60+
const FAILURE_ARTIFACTS_DIR = "test-failures"
61+
62+
/**
63+
* Copies the diagnosable parts of a failed scenario workspace into a stable
64+
* directory inside the checkout, then removes the temporary scenario root.
65+
* CI uploads that directory as a run artifact, so the failure that used to be
66+
* deleted in the cleanup path stays inspectable: phase results, VS Code logs,
67+
* and the extension's global storage with the task files.
68+
*
69+
* The directory is keyed by the unique scenario root name and cleared before
70+
* copying, so retried runs cannot mix artifacts from different failures.
71+
*/
72+
export async function preserveScenarioWorkspace(scenarioWorkspace: ScenarioWorkspace): Promise<string> {
73+
const destinationRoot = path.resolve(
74+
__dirname,
75+
"../../",
76+
FAILURE_ARTIFACTS_DIR,
77+
path.basename(scenarioWorkspace.root),
78+
)
79+
await fs.rm(destinationRoot, { recursive: true, force: true })
80+
await fs.mkdir(destinationRoot, { recursive: true })
81+
82+
const selections: Array<{ label: string; source: string; destination: string }> = [
83+
{
84+
label: "phase results",
85+
source: scenarioWorkspace.results,
86+
destination: path.join(destinationRoot, "phase-results"),
87+
},
88+
{
89+
label: "VS Code logs",
90+
source: path.join(scenarioWorkspace.userData, "logs"),
91+
destination: path.join(destinationRoot, "vscode-logs"),
92+
},
93+
{
94+
label: "task files",
95+
source: path.join(scenarioWorkspace.userData, "User", "globalStorage"),
96+
destination: path.join(destinationRoot, "global-storage"),
97+
},
98+
]
99+
100+
for (const selection of selections) {
101+
try {
102+
await fs.cp(selection.source, selection.destination, { recursive: true, force: true })
103+
} catch (error) {
104+
// A missing subdirectory must not hide the other artifacts.
105+
console.error(`Failed to preserve ${selection.label} from ${selection.source}:`, error)
106+
}
107+
}
108+
109+
await removeScenarioWorkspace(scenarioWorkspace)
110+
return destinationRoot
111+
}

‎apps/vscode-e2e/src/runTest.ts‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@ import { addSearchFilesResultFixtures } from "./fixtures/search-files"
2121
import { addSubtaskFixtures } from "./fixtures/subtasks"
2222
import { addUseMcpToolResultFixtures } from "./fixtures/use-mcp-tool"
2323
import { addWriteToFileResultFixtures } from "./fixtures/write-to-file"
24-
import { createScenarioWorkspace, removeScenarioWorkspace } from "./restart/scenarioWorkspace"
24+
import {
25+
createScenarioWorkspace,
26+
preserveScenarioWorkspace,
27+
removeScenarioWorkspace,
28+
} from "./restart/scenarioWorkspace"
2529
import { runRestartScenario } from "./restart/vscodeCoordinator"
2630

2731
function getCliFlagValue(flag: string) {
@@ -95,6 +99,7 @@ async function main() {
9599

96100
let testWorkspace: string | undefined
97101
let scenarioWorkspace: Awaited<ReturnType<typeof createScenarioWorkspace>> | undefined
102+
let runFailed = false
98103

99104
try {
100105
// Create a temporary workspace folder for regular tests. Restart scenarios own
@@ -218,14 +223,27 @@ async function main() {
218223
})
219224
}
220225
} catch (error) {
226+
runFailed = true
221227
console.error("Failed to run tests", error)
222228
process.exitCode = 1
223229
} finally {
224230
if (testWorkspace) {
225231
await fs.rm(testWorkspace, { recursive: true, force: true })
226232
}
227233
if (scenarioWorkspace) {
228-
await removeScenarioWorkspace(scenarioWorkspace)
234+
// A failed restart scenario must stay inspectable: the phase
235+
// results, VS Code logs, and task files are copied to a stable
236+
// directory for CI artifact upload before the temp root goes away.
237+
if (runFailed) {
238+
try {
239+
const preservedAt = await preserveScenarioWorkspace(scenarioWorkspace)
240+
console.error(`Restart scenario workspace preserved for failure inspection: ${preservedAt}`)
241+
} catch (preserveError) {
242+
console.error("Failed to preserve the restart scenario workspace:", preserveError)
243+
}
244+
} else {
245+
await removeScenarioWorkspace(scenarioWorkspace)
246+
}
229247
}
230248
await mock?.stop()
231249
}

‎apps/vscode-e2e/src/suite/restart-persistence.test.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,13 @@ async function runCreate(api: RooCodeAPI): Promise<void> {
5454
await waitUntilCompleted({ api, taskId })
5555
assert.strictEqual(sawMarker, true, `Completion should include ${MARKER}`)
5656

57-
// TaskCompleted fires before the final saveClineMessages() write is
58-
// necessarily durable on disk. Wait until the completion marker is
59-
// persisted in ui_messages.json before quitting, so the verify phase
60-
// never reads a task directory whose final write has not landed yet.
57+
// TaskCompleted and the UI marker do not acknowledge all three files.
58+
// Check both persisted histories and the store's published history item.
59+
// TaskHistoryStore publishes newly created cache entries only after writing them.
6160
const createdTaskId: string = taskId
6261
await waitFor(() => api.hasTaskUiMessageText(createdTaskId, MARKER))
62+
await waitForMarkedCompletion(api, createdTaskId)
63+
await waitFor(async () => (await api.getTaskHistoryItem(createdTaskId)) !== undefined)
6364

6465
const result: PhaseResult = {
6566
version: PHASE_RESULT_VERSION,

‎docs/architecture/file-replacement-restart-model.md‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
# File replacement and restart model
2+
3+
`scripts/check-file-replacement-restart.ts` is an independent bounded submodel
4+
under `pnpm lifecycle:model-check`. It preserves the completion-readiness model's
5+
separate scope. It checks three files, one initial publication per file, one later
6+
replacement of any one file, and one process crash at every publication or
7+
replacement boundary. Exploration runs to a fixed point with a 10,000-state cap.
8+
9+
## Contract and production mapping
10+
11+
| Model action | Production boundary |
12+
| ---------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
13+
| `publish-ui`, `publish-api`, `publish-history` | Successful writes of `ui_messages.json`, `api_conversation_history.json`, and `history_item.json` |
14+
| `acknowledge-all-three-files` | Explicit readiness conjunction in the restart E2E: `hasTaskUiMessageText`, `hasTaskApiConversationHistorySequence`, and `getTaskHistoryItem`. For a newly created task, `TaskHistoryStore.upsertCore` publishes its cache entry after `writeTaskFile` succeeds. This is a test readiness boundary, not a new public runtime event. |
15+
| `stage-replacement-*` | `safeWriteJson` writes and flushes a `.new_*.tmp` file under the advisory lock |
16+
| `atomic-commit` | Single temp-to-canonical rename |
17+
| `eperm-backup-rename`, `fallback-commit` | Windows-compatible EPERM fallback: canonical-to-backup, then temp-to-canonical |
18+
| `process-crash` | Process exits without rollback, cleanup, or lock release |
19+
| `lock-becomes-stale` | `proper-lockfile` staleness check using the shared 31-second window |
20+
| `fresh-host-startup`, `locked-reader-recovery` | `TaskHistoryStore.reconcile` recovers a missing history record before stat/read; `readFileWithMissingRetry` invokes the same recovery after a second ENOENT for UI/API reads |
21+
| `recovery-io-failure` | Recovery scan, access, stat, or restore fails; rejection preserves committed backups for a later retry |
22+
| `explicit-delete-under-lock` | `deleteJsonFile` removes matching replacements/backups before the canonical file, serialized with writers and recovery; store deletion remains best effort |
23+
24+
The UI marker can become visible while history publication is still pending.
25+
`Task.saveClineMessages` persists UI messages before updating metadata, and logs
26+
metadata failures independently. Neither `TaskCompleted` nor UI-marker visibility
27+
promises that all three files exist. The model explicitly reaches that partial
28+
publication state and permits interruption there. It requires restart discovery
29+
only after the readiness conjunction, not after an unfinished first write.
30+
31+
After acknowledgment, the latest committed version of each required file must
32+
remain in either the canonical file or its backup until explicit deletion.
33+
Completed fresh reads must return those committed versions and find the history
34+
record without a new write. A failed recovery must preserve every file version.
35+
Successful explicit deletion must leave no recoverable version to resurrect.
36+
The model requires reachable atomic and fallback successes, restart recovery,
37+
recovery after failure, partial publication, and reads after deletion.
38+
39+
The checker first runs the old two-rename protocol without reader recovery and
40+
requires a #2130 counterexample. It then exhaustively checks the fixed protocol.
41+
This is an abstract protocol check, not execution of the production writer.
42+
43+
## Refinement evidence
44+
45+
`src/utils/__tests__/safeWriteJson.interruption.test.ts` runs the real bundled
46+
writer in a child process, terminates it with SIGKILL, and exercises both commit
47+
paths before and after the replacement rename for all three filenames. Initial
48+
publication uses the production writer. UI/API tests call production readers;
49+
history-record cases initialize a fresh `TaskHistoryStore` before any recovery
50+
write. One case leaves the crash lock's timestamp unchanged, proving the fresh
51+
store can wait for staleness instead of requiring the test to age the lock.
52+
53+
`safeWriteJson.test.ts` injects directory scan, access, backup stat, and restore
54+
failures, checking backup preservation and retry. The store deletion tests cover
55+
single and batch deletion, leftover cleanup, unrelated-file preservation, and
56+
cleanup failure retaining the canonical copy. The existing two-extension-host
57+
restart E2E remains the production activation/rehydration boundary check.
58+
59+
## Assumptions and limits
60+
61+
This is process-crash safety on filesystems with atomic same-directory rename,
62+
not power-loss safety. Parent directories are not fsynced. Locks are advisory;
63+
all writers, recoverers, and history-file deleters must use the shared protocol.
64+
Live holders refresh their locks and must not be mistaken for crashed holders;
65+
filesystem clock behavior and lock compromise are outside the model. Recovery
66+
waits up to 35 lock retries at one-second intervals, then reports a lock failure.
67+
An indefinitely active writer or persistent filesystem fault can still prevent
68+
recovery; no liveness guarantee is claimed under such faults.
69+
70+
Recovery is abstracted as a locked restore and cleanup; repeated crashes during
71+
recovery, multiple orphaned versions, external file removal, arbitrary overlapping
72+
writers, and deletion interrupted between individual unlinks are not exhaustively
73+
modeled. Focused tests cover newest-backup selection and filesystem failures.
74+
Deletion does not prohibit an explicit later creation of a task at the same path
75+
or a stale writer that deliberately publishes new data after deletion. The
76+
no-resurrection property concerns recovery from leftovers after successful
77+
deletion. Lifecycle status/delegation reducers and completion-event semantics
78+
are unchanged.

‎docs/architecture/task-lifecycle-model.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,14 @@ CI runs `pnpm lifecycle:model-check` in `.github/workflows/code-qa.yml` after li
206206

207207
## Gap audit
208208

209+
The independent [file replacement/restart model](./file-replacement-restart-model.md)
210+
checks the process-crash replacement protocol separately from completion readiness.
211+
It models three task files, initial publication, one replacement, one crash,
212+
locked recovery on fresh reads/startup, and successful explicit deletion. Its
213+
contract, production mapping, old-protocol counterexample, refinement tests, and
214+
filesystem/lock assumptions are documented there. A passing completion-readiness
215+
model alone does not establish this file-level contract.
216+
209217
This section is a summary tracker. Issue links are historical provenance rather than specifications. Coverage status uses these evidence classes:
210218

211219
The [Task lifecycle verification GAP report](./task-lifecycle-gap-report.md) is the authoritative register. It holds the exhaustive repository inventory, the ranked stable burn-down register, the source-based methodology, and the follow-up checklist. This page remains the executable model-suite specification.

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
"test:agent-evals:unit": "node --import tsx --test scripts/agent-evals/*.test.ts",
2121
"agent-evals:check-types": "tsc -p scripts/agent-evals/tsconfig.json",
2222
"test:mutation-ci": "node --test scripts/stryker-diff.test.mjs",
23-
"lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-provider-handoff-scheduler.ts && pnpm cleanup-protocol:model-check && pnpm parser-scope:model-check && tsx scripts/check-completion-persistence.ts && tsx scripts/check-delegated-mode-readers.ts && pnpm tool-turn:model-check",
23+
"lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-provider-handoff-scheduler.ts && pnpm cleanup-protocol:model-check && pnpm parser-scope:model-check && tsx scripts/check-completion-persistence.ts && tsx scripts/check-file-replacement-restart.ts && tsx scripts/check-delegated-mode-readers.ts && pnpm tool-turn:model-check",
2424
"webview-recovery:model-check": "node --import tsx scripts/check-webview-recovery.ts",
2525
"webview-recovery:check-types": "tsc -p scripts/tsconfig.webview-recovery.json",
2626
"fanout-protocol:model-check": "tsx scripts/check-task-fanout-protocol.ts",

0 commit comments

Comments
 (0)