Repository navigation
🤖 refactor: callers pass their session locks to the branch-summary marker, subagent artifacts and tool configs - #7506
Conversation
…er, subagent artifacts and tool configs (#7326)
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. |
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 32 model calls · 224.7k tokens (76% cached) All 14 tests in 9 files
e2e 0.20.0 · 3m 6s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.0k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 21.0s · 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 |
|
@codex review |
|
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". |
🛡️ 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. |
Summary
No-behavior-change refactor for the readiness-advisor follow-ups of phase 4 PRs 13, 14 and 18 in #7326. The branch-summary marker, the four subagent artifact writers, the patch-apply watermark and
ToolConfigurationnow require the caller'sSessionLocksinstead 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 aToolConfigurationwithout locks now passhistoryService.locks. In the app every one of these resolves tonodeSessionLocks, so the locks taken do not change.Refs #7326
Items
startAbandonedBranchSummaryInBackgroundandawaitPendingBranchSummaryfall back tonodeSessionLocksmarker: BranchSummaryMarker({ sessionDir, locks }) instead ofsessionDir?plus an optional or defaultedlocks. 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.oneForkpasseshistoryService.locksto the writer and the reader. The recorded fixture does not change.lockslocksis required insubagentFailureArtifacts.ts,subagentReportArtifacts.ts,subagentTranscriptArtifacts.tsandsubagentGitPatchArtifacts.ts, and inapplyProjectPatch(taskGitPatchEngine.ts), which passes it through. Every production call already passed locks. The test calls passnodeSessionLocks.ToolConfiguration.sessionLocksis optionalcreateTestToolConfigsuppliesnodeSessionLocks. Two production configs left it out and now passhistoryService.locks: the skill-write config ofRefineServiceand the IPC bash tool ofWorkspaceService.todo.tsandreview_pane.tsdrop their?? nodeSessionLocks.agent_skill_write.tsandagent_skill_delete.tspassconfig.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 noSessionLocksin scope: each needs a new constructor dependency and about 300 test constructions, for no change in the app. Reasoned close in #7326.Validation
The two new tests ("takes the target lock from the config's session locks", in
agent_skill_write.test.tsandagent_skill_delete.test.ts) fail when the tools dropconfig.sessionLocksand pass with it.The other changes are type-level:
tscfails on any caller that leaves the locks out.sessionLocks.callOptions.test.tspasses with the fixture unchanged.git merge-treeof this head with each openturn-engine/*PR and with 🤖 refactor: read the session clock for notification polls and clutter scans #7491 gives no conflict that this PR causes. 🤖 test: pin the turn lifecycle on the real StreamManager stack #7488 conflicts with main instreamManager.ts, a file that this PR does not change.Mutation gate: the final
make mutate-checkrun (--max-mutants 900, base e803893) found 287 survivors, all inAgentSession.prepareMessage. That is the hotspot that 🤖 tests: mutation survivors on lines that phase 4 PRs did not change (moved from #7326) #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_MODEiswarn.Risks
Low. In the app the injected locks are
nodeSessionLockseverywhere, so no lock path or lock kind changes. Most of the diff is test call sites that now namenodeSessionLocks.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$209.75