Repository navigation
🤖 refactor: init records, session files and the plan migration take their locks from SessionLocks - #7378
Merged
Conversation
…ir locks from SessionLocks - InitStateManager(config, locks?) takes the init lease probe, the init-record lock, its probe and the write barrier from SessionLocks. core.ts keeps the Node default. - SessionFileManager(config, fileName, locks?) takes the workspace FIFO from SessionLocks. eventStore.ts and the other callers keep the default. - The plan migration lock goes through SessionLocks. resolvePlanFileLocation, markRemotePlanMigratedForDeletion and importLegacyRemotePlan take an optional locks. AgentSession and WorkspaceService pass their history service's locks at the plan's five sites. Other callers keep the default. Paths, timeouts, labels and texts do not change.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
xum/src/node/services/initStateManager.ts
Lines 141 to 146 in dc17e33
Pass the injected locks into the init status store
When a caller supplies non-default SessionLocks, the record claim and release barrier use this.locks, but this constructor still creates an EventStore whose SessionFileManager silently uses nodeSessionLocks. If clearInMemoryState() runs after the fire-and-forget start write passes shouldWrite but before it lands, releaseRecordClaim() drains the wrong workspace FIFO and can release the record lock before that write finishes, allowing a stale running record or deleted session directory to reappear. Thread the same lock set through EventStore and SessionFileManager.
AGENTS.md reference: AGENTS.md:L178-L178
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 32 model calls · 233.4k tokens (72% cached) All 14 tests in 9 files
e2e 0.20.0 · 2m 51s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.1k tokens (21% cached) All 3 tests in 3 files
e2e 0.20.0 · 21.1s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 5.3s · 2 targets (web, phone) · run artifacts |
…ks too Codex (round 1): findImportableLegacyPlanPath runs the automatic migration under the migration lock, but it took no locks argument, so WorkspaceService's import offer and rename check used the default while its clear and import paths used historyService.locks. With a non-default layer the two lock domains could overlap. The probe now takes an optional locks, and both WorkspaceService callers pass this.historyService.locks.
Member
Author
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 544b8a43df
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ode default The init status store's SessionFileManager now takes InitStateManager's locks, so releaseRecordClaim drains the same workspace FIFO as the status writes. The planLocation.ts exports, EventStore and SessionFileManager take locks with no default: every caller passes its history service's locks, and ReviewStateService and the call-options scenario pass nodeSessionLocks with a reason.
…locks the plan read takes
Member
Author
|
Reply to the Codex review on dc17e33 (review 5636321927), finding "Pass the injected locks into the init status store" ( Fixed in 8b5fd7a (afe126d updates one test context). The same commit also closes the round 2 finding on
Production behavior does not change: |
ThomasK33
added a commit
that referenced
this pull request
Oct 24, 2026
Main's #7378 added the same import to the harness, so the merge declared it twice.
This was referenced Oct 24, 2026
Closed
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…n SessionFiles (coder#7391) ## Summary 5a PR 12 of phase 5 (Loop host I/O) of the agent-loop refactor. The todo tools and the review pane tools read and write their session files (`todos.json`, `assistedReview.json`) through the session's `SessionFiles`, the history layer's file service of phase 2. So 4 frozen pairs of `local/no-host-imports` leave the allow map. The map goes from 10 pairs in 5 files to 6 pairs in 3 files. Behavior does not change: the Node layer calls the same `fs/promises` functions and the same `writeFileAtomic`. ## Order This PR waited for phase 4 PR 15 (coder#7385, merged as dab0392), which gave these tools the workspace FIFO from `SessionLocks`. Phase 2 PR 7 (coder#7312) is merged. Plan order is PR 1 to PR 14, so it keeps its place. ## Implementation - `tools.ts`: `ToolConfiguration` gains an optional `sessionFiles`. `turnRequestBuilder.ts` fills it from the `HistoryService`. A config without it gets the Node layer, as configs that tests build by hand do today. - `todo.ts` and `review_pane.ts`: every read, `mkdir`, atomic write and remove of the session file goes through `config.sessionFiles`, with the Node layer as the default. - `todos/todoStorage.ts` and `reviewPane/assistedReviewStorage.ts`: the readers take the files as a last argument with no default, and so does `setTodosForSessionDir`. So the type checker lists every caller. `propose_plan.ts`, `agentSession.ts` and the mock stream player pass the session's files. The callers outside the agent loop (`workspaceService.ts`, `goalStatusBoard.ts`) pass `nodeSessionFiles` explicitly. - The todo reminder (`TodoListReminderSource`) and the turn-end todo nudge (`needsTodoTurnEndNudge`) read the list through the session's files, as `todo_write` writes it. - `eslint.config.mjs`, `.oxlintrc.json` and `scripts/noHostImports.test.ts`: the 4 pairs (`todo.ts` and `review_pane.ts`, each with `fs/promises` and `@/node/utils/writeFileAtomic`) leave the allow map and the frozen list. ## Validation - 2 new cases, one in `todo.test.ts` and one in `review_pane.test.ts`, run the tools on the memory `SessionFiles`. Each reads the state back from the memory volume and checks that the temp folder on disk stays empty. With main's `todo.ts` and `review_pane.ts`, both new cases fail. With this PR, both pass. - The plan's list (`todo.test.ts`, `review_pane.test.ts`, `propose_plan.test.ts`, `services/mock/`, `scripts/noHostImports.test.ts`): 70 pass, 0 fail. - `make static-check` and `make lint-eslint`. - `make test` (host suite lock) on head 7c75dc7: `bun test src` had 27807 pass, 7 skip, 0 fail. The ui line had 729 pass, 1 skip, 0 fail. The shared scripts/storybook/vscode run and all isolated files passed with 0 fail. Loop-test tripwire, 3 runs against main: 0 trips. - The branch first merged main dab0392 with the merge base 16124bf (the coder#7385 head that merged). The diff against main had the same changes as the diff against 16124bf. The one exception was `.oxlintrc.json`, where main already had the formatter layout, so only the 4 allow entries changed. - Round 1 (d9e4781, e860bc2): the todo reminder and the turn-end nudge read through the session's files, and the 3 helpers lose their Node default. 2 new cases run on memory `SessionFiles` (the reminder through `getToolsForModel`, the nudge of a session with memory history files). With the readers of the previous head, both fail. With the fix, both pass. The plan's list plus `agentNotifications/`, `agentSession.turnStopHooks`, `sessionLocks/`, `goalStatusBoard`, `workspaceService.metadata`, `workspaceService.sendMessage`, `tools.test` and `tools.modelOnlyNotifications`: 383 pass, 0 fail. `make static-check`, `make lint-eslint`, `make crap-check`, `make dry-check`. - Round 4 (ac9d932, 8a92b12) merges main a59ebb3 (coder#7378 and coder#7355). coder#7378 added a `nodeSessionLocks` import to the lock scenarios harness, so the merge declared it twice, and CI's Static Checks failed. The harness now imports it once. The allow map keeps coder#7355's new pair. On the merge: `make static-check`, `make lint-eslint`, `make crap-check`, and the round 1 test list: 383 pass, 0 fail. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$148.96`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=148.96 -->
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…rvice (coder#7392) ## Summary 5a PR 13 of phase 5 (Loop host I/O) of the agent-loop refactor. Agent-loop code builds its runtimes through a new `RuntimeFactory` service, so the in-memory loop can give its `FakeRuntime` in place of a concrete runtime. So 5 frozen pairs of `local/no-host-imports` leave the allow map. On main (with 5a PR 12, coder#7391) the map has 6 pairs in 3 files. With this PR, 1 pair is left: `agentSession.ts @/node/services/workspaceUseLeases`, which coder#7355 added. Behavior does not change: the Node layer calls today's functions. ## Order This PR waited for phase 4 PR 16 (coder#7378, merged as 6004b7c) and phase 3 PR 5 (coder#7274, merged), which change the same functions of `agentSession.ts`. coder#7391 landed first, and this PR merges it. They conflicted only in the allow map (`eslint.config.mjs`, `.oxlintrc.json`, `scripts/noHostImports.test.ts`). ## Implementation - `runtimeFactory/runtimeFactoryService.ts`: the `RuntimeFactory` interface. Its members are `createRuntime`, `createRuntimeForWorkspace`, `createRuntimeContextForWorkspace`, `createMultiProjectRuntime` (the multi-project branch of `AIService.createWorkspaceRuntimeContext`) and `createLocalRuntime`. Each has the signature of today's function. The module has only type imports, so `di/tags.ts` can import it with no cycle. `createRuntime` and `createLocalRuntime` have no caller yet. 5a PR 14 moves the reached runtime callers (`agentResolution.ts`, `continuousCompactionSummary.ts`, `planReviewService.ts`, `workspaceProjectRepos.ts`) onto them. - `runtimeFactory/nodeRuntimeFactory.ts`: the Node layer, which calls today's functions. `StoresLive` provides it under `RuntimeFactoryTag`. - `di/layers/core.ts` and `turnRequestBuilder.ts`: `AILive` passes the factory to `AIService` through `LoopHostServices`. - `aiService.ts`: every runtime that `AIService` builds goes through `this.runtimeFactory`. A hand-built `AIService` without one gets the Node layer. - `agentSession.ts` and `contextManagement/strategies/tokenBudget.ts`: they build their runtimes through `aiService.runtimeFactory`, with the Node layer as the default for test doubles of `AIService`. - `runtimeFactory/fakeRuntimeFactory.testHarness.ts`: `fakeRuntimeFactory` and `RuntimeFactoryFake` give the memory host's `FakeRuntime` for every member. - `eslint.config.mjs`, `.oxlintrc.json` and `scripts/noHostImports.test.ts`: the 5 pairs (`agentSession.ts`, `aiService.ts` and `tokenBudget.ts` with `@/node/runtime/runtimeHelpers`, and `aiService.ts` with `@/node/runtime/runtimeFactory` and `@/node/runtime/multiProjectRuntime`) leave the allow map and the frozen list. ## Validation - `aiService.test.ts`: a new case gives `AIService` a `fakeRuntimeFactory` and checks that the workspace runtime context holds the memory host's runtime. With main's `aiService.ts`, the case fails. With this PR, it passes. - The plan's list (`aiService.test.ts`, `agentSession.pinnedBudget.test.ts`, `contextManagement/`, `runtime/runtimeHelpers.test.ts`, `scripts/noHostImports.test.ts`): 255 pass, 0 fail. - `make static-check` and `make lint-eslint`. - `make test` (host suite lock) on head f72b3e9: `bun test src` had 27807 pass, 7 skip, 1 fail. The failure was `sshAskpass` "handles two sequential prompts without ignoring the second", a timing flake in code that this PR does not touch: the file passed 10 of 10 reruns. The rest of `make test` (the shared scripts/storybook/vscode run: 729 pass, 1 skip, 0 fail, and the 18 isolated files: 0 fail) passed. Loop-test tripwire, 3 runs against main: 0 trips. - The branch merged main 6004b7c with a merge base that holds coder#7378's merged head afe126d and the main that the branch merged before. Each file's diff against main equaled its diff against that base. - Then it merges main a59ebb3 (coder#7355). The allow map conflicted, and the resolution removes exactly the 5 pairs of this PR and keeps coder#7355's new pair. The other 11 files keep the same diff against main. On that head (d7c3c41): `make static-check`, `make lint-eslint`, `make crap-check`, the plan's list (255 pass, 0 fail) and the proof above. - Round 2 (3639619) merges main 646ce43 (coder#7391). The allow map conflicted, and the resolution again removes exactly the 5 pairs of this PR, so 1 pair is left. On that head: `make static-check`, `make lint-eslint`, `make crap-check`, and the plan's list with the tests of coder#7391 (todo, review_pane, propose_plan, mock/, turnStopHooks, tools.modelOnlyNotifications): 360 pass, 0 fail. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$151.37`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=151.37 -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The init-record locks, the session file FIFO and the plan migration lock go through
SessionLocks. It is PR 16 of phase 4. Every newlocksargument is optional, and its default is the Node layer, so the app's lock paths, timeouts, labels and texts do not change.Order
historyService.locks(🤖 refactor: HistoryService takes its session locks from SessionLocks #7372), which has merged. It uses no code from the PRs it jumps. Phase 5's 5a PR 13 waits for it.Implementation
initStateManager.ts:new InitStateManager(config, locks = nodeSessionLocks). These calls go throughlocks:useLeasesFor(config).isHeld(ws, "init"))acquireCrossProcessLock) and its probe (inspectCrossProcessLock)workspaceFiles.withLock)core.tskeepsnew InitStateManager(config).utils/sessionFile.ts:new SessionFileManager(config, fileName, locks = nodeSessionLocks).eventStore.tsand the other callers keep the default.utils/runtime/planLocation.ts: the migration lock islocks.acquireProcessFileLock.resolvePlanFileLocation,markRemotePlanMigratedForDeletionandimportLegacyRemotePlantake an optionallocksand pass it throughmigrateRemotePlantowithMigrationLock. The privatewithMigrationLock(config, id, locks, fn)takeslocksbeforefn, so the migration body keeps its indentation.Callers that pass
this.historyService.locks(the plan's sites):agentSession.ts:resolvePlanFileLocationfor the post-compaction attachments.workspaceService.ts:importLegacyRemotePlan, bothmarkRemotePlanMigratedForDeletioncalls, andgetPlanFileLocation'sresolvePlanFileLocation.All other callers keep the default. They are
findImportableLegacyPlanPath, which the plan does not list, and the calls intaskService.ts,workspaceOperations.ts,planReviewService.tsandturnRequestBuilder.ts. In the app,historyService.locksis the same Node layer, so this changes no behavior.Validation
All on the head (
dc17e3374e, this diff on main4770fa63e0):bun testofinitStateManager.test.ts,workspaceLeasesRepro.test.ts,workspaceService.remotePlanNamespace.test.ts,workspaceService.planStorageRepro.test.ts,planReviewService.ensureSnapshot.test.tsandsessionLocks/: 173 pass, 0 fail. The call record fixture is unchanged, and its test passes.make static-check,make lint-eslint: pass.make test: 27753 pass, 0 fail in the main run, and every other run passes.scripts/perf/loopTestBaseline.ts --runs 3 --base origin/main): 0 trips.make crap-check: PASS, 0 violations.core.tsandeventStore.tsdo not change. In the 3 moved files,rg -nP '(?<![.\w])(acquireProcessFileLock\(|acquireCrossProcessLock\(|inspectCrossProcessLock\(|workspaceUseLeasesFor\(|workspaceFileLocks\b)'prints nothing.Mutation check
make mutate-check: 49 mutants, 21 killed, 28 survived. Every survivor is on a line that this PR does not change. Each one is a pre-existing gap for PR 17, listed in #7326.AgentSession.buildAttachmentsFromContextInitStateManagerconstructor,readUnownedRunningInit,acquireRecordLock,releaseRecordClaim,isRecordLockHeldWorkspaceService.importLegacyPlan,markRemotePlanMigratedForRemoval,deletePlanFilesOfMetadatamarkRemotePlanMigratedForDeletion,migrateRemotePlan(planLocation.ts)Risks
historyService.locksin the app is the Node layer.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$122.03