Repository navigation
🤖 refactor: removal, refine and the durable journal take their locks from SessionLocks - #7404
Merged
Merged
Conversation
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. |
Contributor
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 31 model calls · 218.7k tokens (70% cached) All 14 tests in 9 files
e2e 0.20.0 · 3m 11s · 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 · 18.8s · 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 |
…e their locks from SessionLocks withRemovalLocks, RefineService's apply and staging locks, the branch-summary pending marker and DurableEventJournal's blob and append locks go through SessionLocks. WorkspaceService and AgentSession pass their history service's locks. RefineService uses its own history service's. The process-wide journal registry and the CLIs keep the Node default.
…ir history service's locks to the journal
ThomasK33
force-pushed
the
phase4/removal
branch
from
October 24, 2026 10:21
f4aac55 to
afd879b
Compare
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. |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: afd879b137
ℹ️ 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".
…lock The marker reader discovers a held marker with fs.stat, which an injected memory SessionLocks never creates. The correct fix needs a held-lock probe on every SessionLocks layer, so this PR drops the marker surface and a follow-up phase 4 PR moves the writer and the reader together.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88c4b16bd2
ℹ️ 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".
… lock from the history service's locks Its partner, RefineService's apply lock on the same path, already does, so a non-Node SessionLocks must see both or the reset and the refine apply stop contending. Moved here from PR 13.
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. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
ThomasK33
changed the title
This was referenced Oct 24, 2026
Merged
Closed
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…ck from SessionLocks (coder#7423) ## Summary The target mutation lock and the refinement rollback lock take their file lock from `SessionLocks`. It is PR 13 of phase 4. Lock paths, the 2 s waits, order and messages do not change: the Node layer gives today's `acquireProcessFileLock`. The in-process mutexes stay in their modules. ## Order - PR 12 (coder#7404), which the plan puts below it, has merged, so the base is main. PR 14 stacks on it in the native stack. - Phase 5b PR 5 waits for it. ## Implementation - `refinement/targetMutationLocks.ts`: `withTargetMutationLock` and `withTargetMutationLocks` take an optional last `locks` argument (Node default) for the file lock. - `refinement/refinementRollback.ts`: `rollbackRefinement` takes `locks` in its options for the rollback lock and the target locks. `acquireRollbackFileLock(sessionDir, locks)` defaults to the Node layer. - `workspaceRemoval.ts`: the removal target lock and `rollbackRemovalTombstoneIfOwned` use the caller's `locks`. - `workspaceService.ts`: the 2 tombstone rollbacks pass `this.historyService.locks`. The plan also lists the refine serialization lock here (`workspaceService.ts:4478`). PR 12 (coder#7404) moved it in its round 5, because Codex found it split from its partner lock. - `sessionUsageService.ts`: the headless usage target lock passes `this.historyService.locks`. The 9 other callers of the target lock (plan claim 2.1.1) keep the default with no edit: `reviewStateService.ts`, `mcpAppResultStore.ts`, `memoryService.ts`, `tools/agent_skill_write.ts`, `tools/agent_skill_delete.ts`, `devToolsService.ts`, `creationDraftService.ts`, `draftService.ts` and `backup/adapters.ts`. The debug CLI `cli/debug/refinements.ts` has no service, so its rollback keeps the default too. Files beyond the plan's list, each a pass-through of locks that a caller already has (landing note 14): - `tools/refinement_rollback.ts`, `toolAssembly.ts`, `turnRequestBuilder.ts`: the `refinement_rollback` tool passes locks to `rollbackRefinement`. `turnRequestBuilder` puts `historyService.locks` in the tool assembly's sandbox context. Why: the tool is a production caller of the new option, and its turn has a history service. - `tools/refinement_rollback.test.ts`: a new case that gives the tool a context whose `locks` record each acquire, and checks that the rollback lock comes from them. Revert proof: without `locks: ctx.locks` in the tool, the case fails. ## Validation On the stack head `2456cafda9` (PR 13 on PR 12's final head `4eabff0f7b`), every gate passed: - `make static-check` and `make lint-eslint` pass. `make crap-check`: PASS, 0 violations, 5 warnings on touched hotspots with unchanged CC (total CC of the changed files 5117 -> 5118). - `bun test` of `refinement/`, `workspaceRemoval.test.ts`, `backup/`, `sessionUsageService*.test.ts`, `toolAssembly.test.ts`, `turnRequestBuilder.test.ts` and `tools/refinement_rollback.test.ts`: 686 pass, 0 fail. - The tripwire (`scripts/perf/loopTestBaseline.ts --runs 3 --base 4eabff0`): 0 trips. - `make test`: 27851 pass, 0 fail in the main run, and every other run passes. The head `41aab3e5b4` is the same 3 commits rebased on main `6887934d00` (after coder#7404 and coder#7417). Its diff against main has the 9 files above. On this head, `make static-check` passes and the plan tests give 686 pass, 0 fail. ## Mutation check `make mutate-check` (base `214ec69d6a`, so PR 12's lines are in scope too): 187 killed, 113 survived, 0 without a test, 466 not run. No survivor is on a line that this PR changes. I list them in coder#7326 for PR 17: - `prepareOrThrow` (`turnRequestBuilder.ts`): 45. The PR adds one `locks` field. - `RefineService.applyLocked`: 22, `RefineService.runLocked`: 12, and `cacheAuditCommand`: 20. These are PR 12's, already listed. - `rollbackRefinement`: 9, and `acquireRollbackFileLock`: 1 (`refinementRollback.ts`). - `withTargetMutationLocks`: 2, and `createRefinementRollbackTool`: 2. - Not run (466): past the default budget of 300 mutants per run. ## Risks - None for behavior: in the app, `historyService.locks` is `nodeSessionLocks`, which calls the same `acquireProcessFileLock` with the same arguments. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$163.55`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=163.55 -->
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…through the RuntimeFactory (coder#7420) ## Summary 5b PR 8c of phase 5 (Loop host I/O) of the agent-loop refactor. The agent definitions service builds its host-local runtimes through the session's `RuntimeFactory` (claim 3.3) instead of `new LocalRuntime`. This completes 5b PR 8. Behavior does not change: on the host, every caller gets the Node layer (`nodeRuntimeFactory`), whose `createLocalRuntime` is `new LocalRuntime`. ## Order Base main. 5b PR 8a (coder#7415, `Runtime.location`) and 5b PR 8b (coder#7417, the agent skills service) merged. When coder#7417 merged, the native stack rebased this PR onto main 6887934 as head 73d36a5. This PR replaces the temporary `nodeRuntimeFactory` that 8b passes to `resolveGlobalRuntime` in `agentDefinitionsService.ts`. ## Implementation - `agentDefinitionsService.ts`: `discoverAgentDefinitions`, `readAgentDefinition`, `resolveAgentBody`, `resolveAgentDefinition` and `resolveAgentFrontmatter` take a required `runtimeFactory` (`ReadAgentDefinitionOptions`). The plugin container scans and the global scan build their host runtime through it. `resolveHeadlessAgentDefinition` takes the factory as an argument. - `resolveAgentInheritanceChain.ts` and `resolveNodeAgentAiSettings.ts` (`NodeAgentDefinitionContext`) carry a required `runtimeFactory` to the base-definition reads. - Callers in the loop (`agentResolution.ts`, `turnContextAssembler.ts`, `turnRequestBuilder.ts`, `continuousCompactionSummary.ts`) pass the session's factory. - Callers outside the loop (`taskService.ts`, `workspaceService.ts`, `workspaceTurnManager.ts`, `gitPatchArtifactService.ts`, `memoryConsolidationService.ts`, `agentPlugins/composition.ts`) pass `nodeRuntimeFactory` explicitly, with a comment. The tests change in 8 files: the new case below, and `runtimeFactory` in the calls that typecheck listed. As in 8b, plugin discovery and its containment checks still read the process filesystem next to the factory's runtime. Item 12 of the phase 5 follow-ups on coder#7255 tracks it, with the 5b proof PR as its trigger. `resolveAgentDiscoveryContext` (the ORPC entry point) still builds its runtime with the free `createRuntimeForWorkspace` and `createRuntime`. Item 11 of the phase 5 follow-ups on coder#7255 tracks it, with the 5b proof PR as its trigger. ## Validation - `agentDefinitionsService.test.ts`: a new case "resolves plugin containers through the host-local runtime of the RuntimeFactory it was given". The factory's host runtime resolves `~/` to a temp home that holds a plugin agent. With the scans on `new LocalRuntime`, the case fails: discovery reads the real home. With this PR, it passes. - After this PR, `rg -n 'new LocalRuntime|instanceof (LocalBaseRuntime|DevcontainerRuntime|RemoteRuntime)'` prints nothing for the 5 files of 5b PR 8 (the plan's done check). - The changed callers' tests with `agentDefinitions/`, `agentSkills/`, `workflows/` and the task, turn-manager, git-patch and continuous-compaction tests: 2922 pass, 0 fail. - `make static-check`, `make lint-eslint`, `make crap-check` and `make dry-check` (dry-check with the marker of 8b). - `make test` (host suite lock) on head bd0dae3: `bun test src` had 27881 pass, 7 skip, 0 fail. The shared scripts/storybook/vscode run had 729 pass, 1 skip, 0 fail, and the 18 isolated files had 0 fail. Loop-test tripwire, 3 runs against 8b's head 63b6943: 0 trips. - Head bc53832 merges 8b's head 63b6943, which brings main bc1fecf (coder#7406, which touches no file of this PR). - On head 73d36a5 (after the restack onto main 6887934, which brings coder#7404 and coder#7418): typecheck passes, and `make test` (host suite lock) passed: `bun test src` had 27898 pass, 7 skip, 0 fail, the shared run had 729 pass, 1 skip, 0 fail, and the 18 isolated files had 0 fail. Loop-test tripwire, 3 runs against main: 0 trips. The diff against main equals the pre-restack diff against 8b, apart from line offsets in `workspaceService.ts`. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$183.62`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=183.62 -->
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…ad-only probe from SessionLocks (coder#7427) ## Summary The branch-summary pending marker takes its lock from `SessionLocks`, and its reader finds a held marker through a new read-only probe, `SessionLocks.processFileLockExists`. It is PR 18 of phase 4 (landing note 15). In the app nothing changes: the Node layer's probe is the same single `fs.stat` of the lock file that the reader makes today, and the writer and reader get `acquireProcessFileLock` with today's arguments. ## Why PR 12 (coder#7404) first moved the marker's writer and reader to `SessionLocks`. Codex found that the reader found a held marker with `fs.stat` on the lock file, which an injected memory `SessionLocks` never creates. So the reader stopped seeing the writer in that layer. coder#7404 left the marker as it is on main, and this PR moves the writer and the reader together, with a probe that every layer answers. Tracked in coder#7326. The existing API does not cover the probe. Both the Node and the memory `acquireProcessFileLock` assert `timeoutMs > 0`, so a try-acquire with timeout 0 is refused. A short try-acquire would also create the lock's folder and lock file on every send and contend with the writer, which changes what the writer sees. ## Order - PR 14 (coder#7425), which it stacks on, has merged, so the base is main. It is the last phase 4 PR. - Phase 5b PR 9 waits for it if its turn reaches `awaitPendingBranchSummary`. ## Implementation - `sessionLocks/sessionLocks.ts`: `SessionLocks` gains `processFileLockExists(lockPath)`: true while any holder (this process or another, live or dead) has the lock. It never takes or reclaims the lock. - `utils/concurrency/fileLock.ts`: `processFileLockExists`, one `fs.stat` of the lock path. `nodeSessionLocks.ts` forwards to it. - `sessionLocks/memorySessionLocks.testHarness.ts`: the memory layer answers from its lock table. A dead holder stays until an acquire reclaims it, as its lock file would on the disk. - `sessionLocks/foreignHolders.testHarness.ts`: a fake foreign holder, live or dead, makes the probe true. Otherwise the inner layer answers. - `sessionLocks/sessionLocks.contract.testHarness.ts`: a contract case for all 4 layers (Node, memory, and each with foreign holders): own, live and dead holders, and the probe takes no lock. - `branchSummary.ts`: `startAbandonedBranchSummaryInBackground` takes `locks` in its input (Node default) for the writer. `awaitPendingBranchSummary` takes `locks` (Node default) for the probe and the wait. - `agentSession.ts` and `workspaceService.ts`: they pass `this.historyService.locks` to the reader and the writer. ## Validation On the stack head `d24c5642c4` (PR 18 on PR 14's stack head `c0831c924e`), every gate passed: - `make static-check` and `make lint-eslint` pass. `make crap-check`: PASS, 0 violations, 9 warnings on touched hotspots (total CC of the changed files 13439 -> 13446). - `bun test` of `sessionLocks/`, `branchSummary.test.ts`, `agentSession*.test.ts`, `workspaceService.*.test.ts` and `utils/concurrency/`: 2668 pass, 0 fail. - The tripwire (`scripts/perf/loopTestBaseline.ts --runs 3 --base c0831c924e`): 0 trips. - `make test`: 27903 pass, 0 fail in the main run, and every other run passes. The head `6a6948c03e` is the same 2 commits rebased on main `be78b8fed3` (after coder#7425). Its diff against main has the 10 files above, 103 insertions and 25 deletions. On this head, `make static-check` passes, and `sessionLocks/`, `branchSummary.test.ts` and `utils/concurrency/` give 191 pass, 0 fail. ## Tests - `branchSummary.test.ts`: the r48 cross-process wait runs on the Node layer and on the memory layer, through one helper. The memory case is new. - Revert proofs, each run once: - The reader goes back to `fs.stat`: the memory r48 case fails. - The writer goes back to `nodeSessionLocks`: the memory r48 case fails. - The foreign-holder probe drops its own table: the contract case fails on the 2 foreign-holder layers. ## Mutation check `make mutate-check BASE=<PR 14 head>`: 103 killed, 197 survived, 287 not run. Every survivor is in `AgentSession.prepareMessage` (CC 421), where the PR adds one argument, and the 300-mutant budget runs out there. So I ran a second, scoped check on a scratch commit that leaves `agentSession.ts` as on main: 38 killed, 81 survived, 1 equivalent, 0 without a test, 13 not run. No survivor of either run is on a line that this PR changes. I list them in coder#7326 for PR 17: - `WorkspaceService.fork`: 78. The PR adds one `locks` field. - `awaitPendingBranchSummary`: 2, and `startAbandonedBranchSummaryInBackground`: 1. - The equivalent is the writer's `input.locks ?? nodeSessionLocks`, with a `mutation-equivalent` marker. ## Risks - None for behavior: in the app, `historyService.locks` is `nodeSessionLocks`. Its probe is the same `fs.stat` as before, and its acquire calls the same `acquireProcessFileLock` with the same arguments. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$167.23`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=167.23 -->
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…rite through SessionFiles and HostFiles (coder#7426) ## Summary 5b PR 4 of phase 5 (Loop host I/O) of the agent-loop refactor. The durable event journal and the workspace removal tombstones read and write through the injected file services instead of `fs/promises`: - `Journal`, `BlobStore` and `DurableEventJournal` use the `SessionFiles` of their journal (phase 2). - The removal tombstone (`<rootDir>/locks/workspace-removed-<id>.json`) is not a session file, so `workspaceRemoval.ts` publishes and checks it through `HostFiles` (5a PR 5). The removal also deletes the session dir through it. - `HistoryService` passes its own `SessionFiles` to `isWorkspaceRemovalTombstoned` (claim 3.5). The check needs only `access`, which both services have. Behavior does not change: every default is the Node layer (`nodeSessionFiles`, `nodeHostFiles`), which calls the same `fs/promises` functions and `writeFileAtomic`. ## Order Plan order: stack 5b is PR 1 to PR 9. This PR waits for phase 4 PR 12 (coder#7404, merged), which moved the journal and removal lock calls onto `SessionLocks`. It uses no code of 5b PRs 3 and 5, so it opens on its own, with base main. ## Implementation - `utils/journal/journal.ts`: `JournalOptions.files` (absent: `nodeSessionFiles`, like `locks`). `mkdir`, `appendFile`, `readFile`, `stat` and `open` go through it. - `utils/journal/blobStore.ts`: the constructor takes `files` (default `nodeSessionFiles`). All 8 file calls go through it. - `utils/journal/durableEventJournal.ts`: the constructor takes `files` after `locks`, with the same default, and gives it to its `Journal` and `BlobStore`. The two callers that have a history service (`cli/debug/cache-audit.ts`, `replay/replayFixture.ts`) pass `historyService.files`, as they pass its locks. The process-wide registry (`sharedDurableEventJournal`) keeps the Node layer, as phase 4 did for its locks. - `services/workspaceRemoval.ts`: `isWorkspaceRemovalTombstoned` takes an optional files argument (default `nodeHostFiles`). `removeSessionDirUnderMemoryLocks` and `sealSubAgentForRemovalUnderMemoryLocks` take `files?: HostFiles` beside `locks`, and `publishRemovalTombstone` writes through it. The rollback, lease and startup-heal reads keep `fs/promises`: the plan names only the access, the rm and the publish. - `services/historyService.ts`: its 9 tombstone checks pass `this.files`. The plan estimated about 420 lines. This PR adds 163 and removes 39, because only the callers that already hold a history service pass their own files. ## Validation - `durableEventJournal.test.ts`: a new case builds the journal on memory `SessionFiles`. The rows, the blob and the mention index (its `stat`) live in the memory volume, and the temp dir on disk holds no journal file and no blob folder. With main's journal files, the case fails (ENOENT on the memory read). - `workspaceRemoval.test.ts`: a new case removes a session dir through a memory `HostFiles`. The tombstone exists in memory and not on disk, the memory session dir is gone, and a same-named dir on disk stays. With main's `workspaceRemoval.ts`, the case fails. - `historyService.test.ts`: a new case gives HistoryService memory `SessionFiles` with a tombstone in the memory volume only. `writePartial` refuses with "was removed". With main's `historyService.ts` and `workspaceRemoval.ts`, the write succeeds and the case fails. - The plan's tests (`utils/journal/`, `workspaceRemoval.test.ts`, `historyService.test.ts`) plus `replay/`, `refinement/` and `sessionLocks.callOptions.test.ts`: 577 pass, 0 fail. - `make static-check`, `make lint-eslint`, `make crap-check` and `make dry-check` pass. `make lint-eslint` first flagged an await of a non-promise in the new `workspaceRemoval.test.ts` case, and a test-only commit fixed it. - `make test` (host suite lock) on head 5e59214: `bun test src` had 27900 pass, 7 skip, 0 fail. The shared scripts/storybook/vscode run had 729 pass, 1 skip, 0 fail, and the isolated files had 0 fail. Loop-test tripwire, 3 runs against main: 0 trips. - After the lint fix and a merge of main b4ea1e4: `make static-check` passes, and `utils/journal/`, `workspaceRemoval.test.ts` and `historyService.test.ts` have 424 pass, 0 fail. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$189.95`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=189.95 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 26, 2026
…rker, subagent artifacts and tool configs (coder#7506) ## Summary No-behavior-change refactor for the readiness-advisor follow-ups of phase 4 PRs 13, 14 and 18 in coder#7326. The branch-summary marker, the four subagent artifact writers, the patch-apply watermark and `ToolConfiguration` now require the caller's `SessionLocks` instead of falling back to the Node layer, and the two skill tools pass the tool config's locks to the target mutation lock. Two production sites that built a `ToolConfiguration` without locks now pass `historyService.locks`. In the app every one of these resolves to `nodeSessionLocks`, so the locks taken do not change. Refs coder#7326 ## Items | coder#7326 item | Result | | ---------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | PR 18 (coder#7427) advisor 1: `startAbandonedBranchSummaryInBackground` and `awaitPendingBranchSummary` fall back to `nodeSessionLocks` | Done. Both take an optional `marker: BranchSummaryMarker` (`{ sessionDir, locks }`) instead of `sessionDir?` plus an optional or defaulted `locks`. The locks are needed only for the marker, so a caller that names the marker must name its locks, and the calls without a session dir stay as they are. | | PR 18 (coder#7427) advisor 2: the call-record branch-summary scenario uses the default locks | Done. `oneFork` passes `historyService.locks` to the writer and the reader. The recorded fixture does not change. | | PR 14 (coder#7425) advisor 1: the 4 artifact functions keep an optional `locks` | Done. `locks` is required in `subagentFailureArtifacts.ts`, `subagentReportArtifacts.ts`, `subagentTranscriptArtifacts.ts` and `subagentGitPatchArtifacts.ts`, and in `applyProjectPatch` (`taskGitPatchEngine.ts`), which passes it through. Every production call already passed locks. The test calls pass `nodeSessionLocks`. | | PR 14 (coder#7425) advisor 2: `ToolConfiguration.sessionLocks` is optional | Done. It is required. `createTestToolConfig` supplies `nodeSessionLocks`. Two production configs left it out and now pass `historyService.locks`: the skill-write config of `RefineService` and the IPC bash tool of `WorkspaceService`. `todo.ts` and `review_pane.ts` drop their `?? nodeSessionLocks`. | | PR 13 (coder#7423) advisor: 9 callers of the target mutation lock take the Node default | Partly done. `agent_skill_write.ts` and `agent_skill_delete.ts` pass `config.sessionLocks` (6 sites), with a test each. The other 7 (`memoryService.ts`, `reviewStateService.ts`, `mcpAppResultStore.ts`, `devToolsService.ts`, `draftService.ts`, `creationDraftService.ts`, `backup/adapters.ts`) have no `SessionLocks` in scope: each needs a new constructor dependency and about 300 test constructions, for no change in the app. Reasoned close in coder#7326. | | PR 12 (coder#7404) advisor: the durable journal callers | Not in this PR. They touch the files of coder#7485, which is still open. | ## Validation - The two new tests ("takes the target lock from the config's session locks", in `agent_skill_write.test.ts` and `agent_skill_delete.test.ts`) fail when the tools drop `config.sessionLocks` and pass with it. - The other changes are type-level: `tsc` fails on any caller that leaves the locks out. - `sessionLocks.callOptions.test.ts` passes with the fixture unchanged. - `git merge-tree` of this head with each open `turn-engine/*` PR and with coder#7491 gives no conflict that this PR causes. coder#7488 conflicts with main in `streamManager.ts`, a file that this PR does not change. - Mutation gate: the final `make mutate-check` run (`--max-mutants 900`, base e803893) found 287 survivors, all in `AgentSession.prepareMessage`. That is the hotspot that coder#7475 lists, and this PR changes only one argument there. I did not chase them. The run stopped at its time budget with 438 mutants not run, so it did not reach the other changed functions. `MUTATION_MODE` is `warn`. ## Risks Low. In the app the injected locks are `nodeSessionLocks` everywhere, so no lock path or lock kind changes. Most of the diff is test call sites that now name `nodeSessionLocks`. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$209.75`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=209.75 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 26, 2026
… the session's layers (coder#7507) ## Summary This PR closes the PR 12 (coder#7404) journal follow-ups of coder#7326 and mirrors coder#7485's `createParentDir` option in the foreign-holder test layer. It is stacked on PR B. App behavior does not change: in the app, the history's file and lock layers are the same objects as the Node layers, so every call still gets the same shared journal instance. The skill tools write their refinement rows through the tool config's layers. `RefineService` reads its seq baseline and its staged rows through its history's layers. The skill-write config of `RefineService` now carries the history's session files. A test graph on memory layers therefore writes and reads one journal, the one that `aiService.durableEventJournalFor` uses. Refs coder#7326 ## Items | coder#7326 item | Result | | --- | --- | | PR 12 advisor: `appendRefinementEvent` writers of `sharedDurableEventJournal` (the skill tools) | Done. `RefinementEmitArgs` gets `layers`, and `appendRefinementEventFromTool` passes the tool config's `sessionFiles` and `sessionLocks`. The `sessionLocks` field became required in PR B. | | PR 12 advisor: `refineService.ts:1245` reads the shared journal | Done. `readMaxJournalSeq` and `listStagedRefinementRows` both read through `journalLayers()`, which returns the history's files and locks. `listRefinements` takes optional `layers` for this caller, so the baseline and the staged rows come from one journal. The skill-write config of `RefineService` sets `sessionFiles: this.historyService.files`, so its rows go to that same journal. | | PR 12 advisor: `aiService.durableEventJournalFor` | Already done on main: it passes `historyService.files` and `historyService.locks`. No code in this PR. | | PR 12 advisor: `sandboxHostService.journalFor` | Reasoned close. `SandboxHostService` has no history service and no session layers in scope. The tests build it with `new SandboxHostService()`, so routing it needs a new constructor dependency. One hazard stays, and only in tests: on memory layers, `aiService` and the sandbox host would get two journal instances, and that split would break seq ordering. No test does that today. I searched every test that uses `SessionFilesMemory`, `SessionLocksMemory` or the in-memory loop for the sandbox host. The only match, `agentSession.turnStopHooks.test.ts`, replays a canned `code_execution` part and has no sandbox host. | | PR 12 advisor: `memoryService.ts:2739` (`appendRefinementEvent`) | Reasoned close. `MemoryService` has no `SessionLocks` or `SessionFiles` in scope. This is the same reason as the PR 13 target-lock callers: a new constructor dependency, and about 46 test constructions. | | PR 12 advisor: `refinementRollback` (`rollbackRefinement`'s journal and its `listRefinements`) | Reasoned close. `rollbackRefinement` has `opts.locks` but no files layer, and the registry needs both. Its two callers (`tools/refinement_rollback.ts` and `cli/debug/refinements.ts`) would need a new option through read, validate, apply and append. In the app nothing changes. On memory layers, a rollback would read the Node journal while the skill rows sit in the memory one. No test runs a rollback on memory layers today. | | PR 12 advisor: `replayVerifySession` (`replay/replayVerify.ts`) | Reasoned close. No history service is in scope. It reads the journal of the session dir that its caller names, with `new DurableEventJournal(params.sessionDir)`. | | coder#7485 option in the test layers | Done. The memory layer already honors `createParentDir` (coder#7485). The foreign-holder layer's `acquireProcessFileLock` now probes the inner lock when `createParentDir` is false and a fake holder sits on the path. A missing parent then fails with ENOENT, as the Node lock does before it meets any holder. A lock that the probe takes is released at once. The probe asks the inner layer, not the disk, so it also holds on a memory `HostFiles` volume. | ## Validation - The new test "a tool row takes its journal locks from the tool config's session locks" (`refinementJournal.test.ts`) fails when `appendRefinementEventFromTool` drops `layers`. - The new contract test "foreign: a missing parent wins over a live holder for a file lock without createParentDir" fails without the probe. Its second half (the parent comes back, and the live holder refuses with today's text) fails if the wrapper always hands the call to the inner lock. - This PR does not change `journal.ts`, `durableEventJournal.ts`, `blobStore.ts` or `fileLock.ts`. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$209.75`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=209.75 -->
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
Workspace removal, the refine service and the durable event journal take their locks from
SessionLocks. It is PR 12 of phase 4. Lock paths, timeouts, order and messages do not change: the Node layer gives today'sacquireProcessFileLock.The branch-summary pending marker is not in this PR. It keeps its direct
acquireProcessFileLock. Phase 4 PR 18 moves its writer and its reader together (see Scope).Order
historyService.locksfrom PR 9 (🤖 refactor: HistoryService takes its session locks from SessionLocks #7372), which has merged. Phase 5b PR 4 waits for it.Implementation
workspaceRemoval.ts:withRemovalLockstakes the refine and history locks from an optionallocks(Node default).removeSessionDirUnderMemoryLocksandsealSubAgentForRemovalUnderMemoryLocksforward it.workspaceService.ts: its 2 removal calls passthis.historyService.locks.acquireRefineSerializationLocktakes its file lock fromthis.historyService.lockstoo. Why: it locks the same path as the refine service's apply lock, so both must use one layer or a reset stops contending with a refine apply. The plan had this line in PR 13. Codex found the split on round 4, so round 5 moves it here.refinement/refineService.ts: the apply lock and the staging lock usethis.historyService.locks.utils/journal/durableEventJournal.ts: the constructor takeslocks(Node default) for the blob lock and passes it to itsJournalfor the append lock.Files beyond the plan's list, each a pass-through of locks that a caller already has (landing note 14):
cli/debug/cache-audit.tsandreplay/replayFixture.ts: the journal gets the history service's locks that each one already builds. Why: they are production callers with a service in scope.Callers that keep the Node default, because no service is in scope: the process-wide journal registry (
sharedDurableEventJournal),replayVerifySession, and the call-record scenarios, so the PR 2 and 3 fixture stays unchanged.Scope: the branch-summary marker moves to PR 18
The plan lists
branchSummary.tsfor this PR. Codex found a real defect in the first version (thread onbranchSummary.ts): the marker reader finds a held marker withfs.staton the lock file. An injected memorySessionLocksnever creates that file, so the reader stops seeing the writer. The correct fix needs a held-lock probe on everySessionLockslayer. By AGENTS.md ("remove the surface from this PR instead of growing it"), this PR leavesbranchSummary.ts,agentSession.tsand the branch-summary call inworkspaceService.tsas they are on main. Phase 4 PR 18 delivers the marker move with the probe. The finding is tracked in #7326.Validation
On the head
4eabff0f7b(PR 11 merged on main as214ec69d6a, plus this PR), every gate passed:make static-checkandmake lint-eslintpass.make crap-check: PASS, 0 violations, 3 warnings on touched hotspots (base214ec69d6a, total CC of the changed files 4233 -> 4234).bun testofworkspaceRemoval.test.ts,refinement/,branchSummary.test.ts,utils/journal/,workspaceService.*.test.ts,replay/andcli/debug/cache-audit.test.ts: 1409 pass, 0 fail.scripts/perf/loopTestBaseline.ts --runs 3 --base origin/main): 0 trips.make test: 0 fail.The plan adds no test for this PR: the moved files keep their tests, which pass with no edit. The refine serialization line is a one-argument pass-through with no test of its own: in the app, both sides of the lock are
nodeSessionLocks.Mutation check
make mutate-check(base214ec69d6a): 130 killed, 99 survived, 1 equivalent, 0 without a test, 93 not run. No survivor is on a line that this PR changes. I list them in #7326 for PR 17:removeSingleWorkspaceUnlocked(workspaceService.ts): 35.RefineService.applyLocked: 22, andRefineService.runLocked: 12.cacheAuditCommand(cli/debug/cache-audit.ts): 20.removeSessionDirUnderMemoryLocks(workspaceRemoval.ts): 10.Risks
historyService.locksisnodeSessionLocks, which calls the sameacquireProcessFileLockwith the same arguments.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$161.10