🤖 refactor: the todo and review pane tools take the workspace FIFO from SessionLocks by ThomasK33 · Pull Request #7380 · coder/xum

ThomasK33 · 2026-10-24T02:06:04Z

Summary

The todo and review pane tools take the workspace FIFO from SessionLocks. The turn request builder passes its history service's locks in the tool config. It is PR 15 of phase 4. Lock keys and order do not change: the Node layer gives today's workspaceFileLocks.

Order

Plan order: PR 15 comes after PRs 10 to 14.
Landing order: PR 15 lands on main as its own PR, before PR 5 (🤖 tests: put the session lock modules under local/no-host-imports with their 1 pair #7355), PR 8 (🤖 refactor: pinned files, artifact index, workflow runs and journal appends take their locks through SessionLocks #7354), PR 7 and PRs 10 to 14 (recorded as a phase 4 plan note).
Why: PR 15 waits for phase 3 PR 8 (🤖 refactor: move the agent tools onto the HostClock service #7277) and for PR 9's historyService.locks (🤖 refactor: HistoryService takes its session locks from SessionLocks #7372). Both have merged. It uses no code from the PRs it jumps. Phase 5's 5a PR 12 waits for it.

Implementation

common/utils/tools/tools.ts: ToolConfiguration gains an optional sessionLocks. When it is absent, the tools use the Node layer, as for clock and desktopNotifications in configs that tests build by hand.
turnRequestBuilder.ts: the tool config gets sessionLocks: historyService.locks.
tools/todo.ts: writeTodos takes the locks. todo_write passes config.sessionLocks. setTodosForSessionDir(ws, dir, todos, locks = nodeSessionLocks) takes locks, and both app callers pass one:
- propose_plan.ts passes config.sessionLocks as is. The default parameter covers undefined, so createProposePlanTool gains no branch.
- mock/mockAiStreamPlayer.ts passes deps.historyService.locks.
Tests and the call record scenarios keep the default.
tools/review_pane.ts: the read, apply and write of review_pane_update run under locks.workspaceFiles, from config.sessionLocks.
todo.ts and review_pane.ts import no lock module now. The lint pairs for these two files arrive with 🤖 tests: put the session lock modules under local/no-host-imports with their 1 pair #7355. 🤖 tests: put the session lock modules under local/no-host-imports with their 1 pair #7355 drops them when it next merges main.

Validation

On the head 16124bf4be: bun test of tools/todo.test.ts, tools/review_pane.test.ts, tools/propose_plan*.test.ts, mock/, scripts/noHostImports.test.ts and sessionLocks/: 128 pass, 0 fail. make static-check: pass. make crap-check: PASS, 0 violations (a warning on the touched hotspot prepareOrThrow, CC 308 unchanged).
On 3b158d7e39 (the first commit; the second one drops a ?? in propose_plan.ts and adds two comments): make lint-eslint passes. make test gives 27753 pass, 0 fail in the main run, and every other run passes. The tripwire (scripts/perf/loopTestBaseline.ts --runs 3 --base origin/main) reports 0 trips.
In todo.ts and review_pane.ts, rg -nP '(?<![.\w])(acquireProcessFileLock\(|acquireCrossProcessLock\(|inspectCrossProcessLock\(|workspaceUseLeasesFor\(|workspaceFileLocks\b)' prints nothing.

Mutation check

make mutate-check on 16124bf4be: 148 killed, 2 equivalent, 150 survived, 94 not run.

Equivalent (2): the config.sessionLocks ?? nodeSessionLocks lines in todo.ts and review_pane.ts. Each has a mutation-equivalent: marker: sessionLocks is an object or undefined, so ?? and || agree.
Survived (150): every survivor is on a line that this PR does not change. Each is a pre-existing gap for PR 17, listed in 🤖 tests: declined review findings of the phase 4 call-record PRs #7326.
- TurnRequestBuilder.prepareOrThrow (CC 308): 141. The PR adds one property line, sessionLocks: this.dependencies.historyService.locks, with no operator to mutate.
- createProposePlanTool: 6, on lines 35, 47, 60, 69, 91 and 115.
- createReviewPaneUpdateTool: 3, on lines 182, 184 and 206.
Not run (94): past the default budget of 300 mutants per run (mutate_report.py --max-mutants). The scope is large because prepareOrThrow is a CC 308 hotspot.

Risks

None for behavior. In the app, historyService.locks is the Node layer, so every tool uses today's workspaceFileLocks FIFO.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high • Cost: $122.03

… SessionLocks ToolConfiguration gains an optional sessionLocks (absent: the Node layer, for configs that tests build by hand). The turn request builder passes its history service's locks, so todo_write, propose_plan's todo sync and review_pane_update take the workspace FIFO from SessionLocks. setTodosForSessionDir takes locks (Node default), and its two app callers pass one: propose_plan and the mock stream player. todo.ts and review_pane.ts import no lock module now.

…uivalent ?? mutants propose_plan passes config.sessionLocks as is: setTodosForSessionDir's default parameter covers undefined, so the tool gains no branch (the CRAP gate refused CC 15 -> 16). The two remaining config.sessionLocks ?? nodeSessionLocks lines get a mutation-equivalent marker: sessionLocks is an object or undefined, so ?? and || agree.

chatgpt-codex-connector · 2026-10-24T02:06:14Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-24T02:08:24.884096Z	`16124bf`	PR opened
🔒 Security Review	🔄 Running since 2026-10-24T03:08:57.437170Z	`16124bf`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

github-actions · 2026-10-24T02:12:14Z

🟢 e2e stories (Haiku): 14 passed

27 agent steps · 7 replayed from cache · 30 model calls · 207.4k tokens (72% cached)
Spend: $0.10 of the $1.00 cap (proxy ledger)

All 14 tests in 9 files

	Test	Agent	Time
🟢	chatRestart.e2e.ts · 1 passed	2 steps · 4 calls	29.8s
🟢	a chat survives a restart of the server	2 steps · 4 calls	29.8s
🟢	compaction.e2e.ts · 1 passed	3 steps · 7 calls	43.8s
🟢	/compact replaces the old chat with a summary	3 steps · 7 calls	43.8s
🟢	pluginTrustRestart.e2e.ts · 1 passed	4 steps · 2 calls	12.1s
🟢	a trusted plugin hook stays trusted after the server restarts	4 steps · 2 calls	12.1s
🟢	settingsRoundTrip.e2e.ts · 1 passed	3 steps · 4 calls	28.2s
🟢	a chat survives trips through Settings	3 steps · 4 calls	28.2s
🟢	sidebarPerWorkspace.e2e.ts · 1 passed	4 steps · 1 call	4.6s
🟢	a new workspace starts with its right sidebar collapsed while another keeps it open	4 steps · 1 call	4.6s
🟢	smoke.e2e.ts · 1 passed	2 steps · 1 call	5.6s
🟢	the agent opens the playground workspace	2 steps · 1 call	5.6s
🟢	streamErrors.e2e.ts · 1 passed	1 step · 1 call	7.8s
🟢	stream errors show in the chat, and the chat recovers	1 step · 1 call	7.8s
🟢	todoNudge.e2e.ts · 1 passed	2 steps · 4 calls	23.3s
🟢	the agent finishes open TODO items after the turn-end nudge	2 steps · 4 calls	23.3s
🟢	touchAlignment.e2e.ts · 6 passed	6 steps · 6 calls	27.2s
🟢	the Settings close button is level with the title on a touch screen 390px wide	1 step · 1 call	7.7s
🟢	the Settings close button is level with the title on a touch screen 768px wide	1 step · 1 call	4.2s
🟢	Backup settings checkboxes are small and level with their labels on a touch phone	1 step · 1 call	4.4s
🟢	the heartbeat checkbox is small and level with its label on a touch phone	1 step · 1 call	5.2s
🟢	a reply's model and time stay on one line on a touch phone	1 step · 1 call	3.0s
🟢	the workspace actions menu stays on the title row on a touch phone	1 step · 1 call	2.8s

_{e2e 0.20.0 · 3m 5s · web · run artifacts}

🟢 e2e Electron stories (Haiku): 3 passed

3 agent steps · 3 model calls · 8.1k tokens (21% cached)
Spend: $0.02 of the $1.00 cap (proxy ledger)

All 3 tests in 3 files

	Test	Agent	Time
🟢	electron/deepLink.e2e.ts · 1 passed	1 step · 1 call	5.6s
🟢	a deep link of a second launch starts a chat in the running app	1 step · 1 call	5.6s
🟢	electron/menuBar.e2e.ts · 1 passed	1 step · 1 call	7.2s
🟢	the workspace menu bar is the desktop title bar	1 step · 1 call	7.2s
🟢	electron/smoke.e2e.ts · 1 passed	1 step · 1 call	6.1s
🟢	Xum Electron opens the playground workspace	1 step · 1 call	6.1s

_{e2e 0.20.0 · 20.8s · electron · run artifacts}

🟢 e2e agent repros (Haiku): no tests selected

Spend: $0.00 of the $1.00 cap (proxy ledger)

_{e2e 0.20.0 · 5.4s · 2 targets (web, phone) · run artifacts}

ThomasK33 · 2026-10-24T03:08:44Z

@codex security review

ThomasK33 · 2026-10-24T04:09:43Z

Codex's security review on this PR started at about 02:06 UTC on head 16124bf and has not finished. One same-head security retry at 03:08:45 UTC also has not finished after 60 minutes. The code review of the same head is clean. So the same branch (phase4/tools) and head move to a new PR for a fresh review, as for #7354. I close this PR and keep the branch.

…om SessionLocks (coder#7385) Replaces coder#7380 (Codex security review hung there). ## Summary The todo and review pane tools take the workspace FIFO from `SessionLocks`. The turn request builder passes its history service's locks in the tool config. It is PR 15 of phase 4. Lock keys and order do not change: the Node layer gives today's `workspaceFileLocks`. ## Order - Plan order: PR 15 comes after PRs 10 to 14. - Landing order: PR 15 lands on main as its own PR, before PR 5 (coder#7355), PR 8 (coder#7354), PR 7 and PRs 10 to 14 (recorded as a phase 4 plan note). - Why: PR 15 waits for phase 3 PR 8 (coder#7277) and for PR 9's `historyService.locks` (coder#7372). Both have merged. It uses no code from the PRs it jumps. Phase 5's 5a PR 12 waits for it. ## Implementation - `common/utils/tools/tools.ts`: `ToolConfiguration` gains an optional `sessionLocks`. When it is absent, the tools use the Node layer, as for `clock` and `desktopNotifications` in configs that tests build by hand. - `turnRequestBuilder.ts`: the tool config gets `sessionLocks: historyService.locks`. - `tools/todo.ts`: `writeTodos` takes the locks. `todo_write` passes `config.sessionLocks`. `setTodosForSessionDir(ws, dir, todos, locks = nodeSessionLocks)` takes locks, and both app callers pass one: - `propose_plan.ts` passes `config.sessionLocks` as is. The default parameter covers undefined, so `createProposePlanTool` gains no branch. - `mock/mockAiStreamPlayer.ts` passes `deps.historyService.locks`. Tests and the call record scenarios keep the default. - `tools/review_pane.ts`: the read, apply and write of `review_pane_update` run under `locks.workspaceFiles`, from `config.sessionLocks`. - `todo.ts` and `review_pane.ts` import no lock module now. The lint pairs for these two files arrive with coder#7355. coder#7355 drops them when it next merges main. ## Validation - On the head `16124bf4be`: `bun test` of `tools/todo.test.ts`, `tools/review_pane.test.ts`, `tools/propose_plan*.test.ts`, `mock/`, `scripts/noHostImports.test.ts` and `sessionLocks/`: 128 pass, 0 fail. `make static-check`: pass. `make crap-check`: PASS, 0 violations (a warning on the touched hotspot `prepareOrThrow`, CC 308 unchanged). - On `3b158d7e39` (the first commit; the second one drops a `??` in `propose_plan.ts` and adds two comments): `make lint-eslint` passes. `make test` gives 27753 pass, 0 fail in the main run, and every other run passes. The tripwire (`scripts/perf/loopTestBaseline.ts --runs 3 --base origin/main`) reports 0 trips. - In `todo.ts` and `review_pane.ts`, `rg -nP '(?<![.\w])(acquireProcessFileLock\(|acquireCrossProcessLock\(|inspectCrossProcessLock\(|workspaceUseLeasesFor\(|workspaceFileLocks\b)'` prints nothing. ## Mutation check `make mutate-check` on `16124bf4be`: 148 killed, 2 equivalent, 150 survived, 94 not run. - Equivalent (2): the `config.sessionLocks ?? nodeSessionLocks` lines in `todo.ts` and `review_pane.ts`. Each has a `mutation-equivalent:` marker: `sessionLocks` is an object or undefined, so `??` and `||` agree. - Survived (150): every survivor is on a line that this PR does not change. Each is a pre-existing gap for PR 17, listed in coder#7326. - `TurnRequestBuilder.prepareOrThrow` (CC 308): 141. The PR adds one property line, `sessionLocks: this.dependencies.historyService.locks`, with no operator to mutate. - `createProposePlanTool`: 6, on lines 35, 47, 60, 69, 91 and 115. - `createReviewPaneUpdateTool`: 3, on lines 182, 184 and 206. - Not run (94): past the default budget of 300 mutants per run (`mutate_report.py --max-mutants`). The scope is large because `prepareOrThrow` is a CC 308 hotspot. ## Risks - None for behavior. In the app, `historyService.locks` is the Node layer, so every tool uses today's `workspaceFileLocks` FIFO. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$122.03`_

…h their 1 pair (coder#7355) ## Summary Puts the five session lock modules under `local/no-host-imports` for the agent-loop files, with the 1 (file, module) pair that those files still import. It is PR 5 of phase 4. ## Background Phase 4 moves the loop's lock calls behind `SessionLocks`. With the lock modules in `HOST_MODULES`, a loop file that adds a new direct lock import fails lint. The allow map holds today's imports, and the later move PRs remove them: - `agentSession.ts` -> `workspaceUseLeases` (PR 10) The PR first listed 5 pairs. 4 of them are gone, because the PRs that moved those imports merged first: - `historyService.ts` -> `workspaceFileLocks`, `fileLock`: coder#7372 (PR 9). - `tools/todo.ts` -> `workspaceFileLocks` and `tools/review_pane.ts` -> `workspaceFileLocks`: coder#7385 (PR 15, which replaced coder#7380). ## Implementation - `eslint.config.mjs`: `HOST_MODULES` gains `@/node/utils/concurrency/workspaceFileLocks`, `@/node/utils/concurrency/fileLock`, `@/node/utils/concurrency/processLiveness`, `@/node/utils/main/crossProcessLock` and `@/node/services/workspaceUseLeases`. `mutexMap` stays off the list: `tools/file_edit_operation.ts` keeps a pure in-memory map. `HOST_IMPORT_ALLOW` gains the 1 pair. - `.oxlintrc.json`: the same modules and pair. - `scripts/noHostImports.test.ts`: the frozen pairs gain the same 1. - No config block is added, and no source file changes. coder#7381 (PR 8) merged, so this PR now has base main. The branch has one merge of main, which resolves the 3 lint files. ## Validation - `bun test ./scripts/noHostImports.test.ts`: 6 pass, 0 fail. - `make static-check` and `make lint-eslint`: pass. The rule fails a stale allow entry, so the 1 pair is in use. - `make check-oxlint-config`: `.oxlintrc.json` matches `eslint.config.mjs` for every lint file. - `make test` on 1afc6e2: 27804 pass, 1 fail under host load (load average about 13): `backgroundProcessManager.test.ts` ("gives concurrent same-name spawns from two backends distinct directories on a non-host runtime"). The file passes alone on this head (166 of 166). The PR changes no runtime code. - Tripwire (`--runs 3 --base origin/main`): 0 trips. - `make crap-check`: PASS, CC 0 -> 0. `make mutate-check`: PASS with 0 mutants. The PR changes no production function. ## Risks Lint-only. A loop file that later adds a direct lock import fails lint until its PR lists the pair. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$135.57`_

ThomasK33 added 2 commits October 24, 2026 00:53

ThomasK33 mentioned this pull request Oct 24, 2026

🤖 tests: declined review findings of the phase 4 call-record PRs #7326

Open

ThomasK33 closed this Oct 24, 2026

ThomasK33 mentioned this pull request Oct 24, 2026

🤖 refactor: the todo and review pane tools take the workspace FIFO from SessionLocks #7385

Merged

ThomasK33 deleted the phase4/tools branch October 24, 2026 05:05

This was referenced Oct 24, 2026

🤖 tests: put the session lock modules under local/no-host-imports with their 1 pair #7355

Merged

🤖 tests: mutation survivors on lines that phase 4 PRs did not change (moved from #7326) #7475

Closed

Navigation Menu

Repository navigation

🤖 refactor: the todo and review pane tools take the workspace FIFO from SessionLocks - #7380

🤖 refactor: the todo and review pane tools take the workspace FIFO from SessionLocks#7380
ThomasK33 wants to merge 2 commits into
mainfrom
phase4/tools

ThomasK33 commented Oct 24, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 24, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 24, 2026

Uh oh!

ThomasK33 commented Oct 24, 2026

Uh oh!

ThomasK33 commented Oct 24, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ThomasK33 commented Oct 24, 2026

Summary

Order

Implementation

Validation

Mutation check

Risks

Uh oh!

chatgpt-codex-connector Bot commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

github-actions Bot commented Oct 24, 2026

🟢 e2e stories (Haiku): 14 passed

🟢 e2e Electron stories (Haiku): 3 passed

🟢 e2e agent repros (Haiku): no tests selected

Uh oh!

ThomasK33 commented Oct 24, 2026

Uh oh!

ThomasK33 commented Oct 24, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chatgpt-codex-connector Bot commented Oct 24, 2026 •

edited

Loading