Repository navigation
🤖 tests: record the options of each session lock call - #7290
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. |
![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: c1f6335ee1
ℹ️ 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".
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 36 model calls · 278.9k tokens (79% cached) All 14 tests in 9 files
e2e 0.20.0 · 3m 9s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.1k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 19.7s · 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 |
c1f6335 to
2b0817b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b0817b6e9
ℹ️ 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".
|
@codex review |
…call-record # Conflicts: # src/node/services/sessionLocks/__fixtures__/callOptions.json # src/node/services/sessionLocks/sessionLocks.callOptions.test.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9650c17be4
ℹ️ 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".
|
@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: 480bb5a516
ℹ️ 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".
|
@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: 5ed6316f5b
ℹ️ 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".
|
Review loop paused at round 4 (non-convergence rule in AGENTS.md). I fixed nothing in this round. The round-4 findings repeat two areas of earlier rounds:
Each fix closed one case, and the next round found the next case of the same kind. So the design of the record is the problem, not one more field or token. The phase lead decides the next step with the maintainer. The third finding (PRRT_kwDOPxxmWM6vgtmT, seeding HistoryService in The three threads stay open until then. |
|
The review loop continues, with a design change of the record (approved by the phase lead). Rounds 1 to 4 kept finding new cases in two areas: a key that the record left out, and an identity that the first-use numbering hid. The change removes both hand-made parts instead of adding one more key or token:
Scratch edits that fail now: the history lock built from a wrong workspace id, and |
|
@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: 71bebea2c1
ℹ️ 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".
|
Review loop paused at round 5. I fixed nothing in this round. The round-5 finding (PRRT_kwDOPxxmWM6vh7aX) is valid and is again in the identity-masking area: the name map gives the 32-character UTF-8 digest (history and refine-apply locks) and the 64-character UTF-16LE digest (mutation gate and use leases) the same name, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7841035d0d
ℹ️ 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".
|
Review loop stopped at round 6, the last round under the cap. I fixed nothing in this round. Round 6 brought two new findings on 7841035: PRRT_kwDOPxxmWM6vjS7Z (the record sorts away the acquire-before-inspect order of |
|
@codex security review |
🛡️ Codex Security ReviewSecurity 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
Adds a test that records each session lock call that a few real scenarios make: the source file of the call, the lock function and all its arguments (every option). A fixture pins today's calls. It is PR 2 of phase 4 (session locks behind one
SessionLocksservice). Each later move PR must keep this fixture byte for byte, so a move that changes a lock path, a timeout, a label or any other option fails.Background
Phase 4 moves the lock calls of 27 files behind
SessionLocks, with no change of behavior. The moved calls keep their options by hand. A changed timeout or path changes behavior only under contention, which normal tests do not reach. This record makes such a change visible in review. This PR covers the 9 files of the first group (history, agent session, turn manager, removal, branch summary, refine, durable journal, todo and review pane tools). PR 3 adds the other 18 files.Implementation
sessionLocks.callOptions.test.tsspies on the lock modules: thefileLockandcrossProcessLocknamespaces,WorkspaceUseLeases.prototype.holdand the sharedworkspaceFileLocks.withLock. In Bun, a spy on a module namespace also reaches the named imports of the callers, so the record sees each call before and after a move.callSite()takes the first source file on the stack and skips theSessionLockslayers, which only forward a call. Calls from test files are left out.sessionLocks.callOptions.scenarios.testHarness.tsruns the scenarios through public entry points: one turn, one refusal of a mutation in a turn, one removal, one refine apply and one fork.AbortSignals become fixed tokens, and any other kind fails the test with an assertion. So no option can drop out of the record (createParentDirectory, for example). This replaced a chosen list of keys in review round 4, because rounds 1, 2 and 4 each found one more key that the list left out.workspaceFiles.withLockkey and theWorkspaceUseLeases.holdworkspace id are arguments, so the record has them too. A move that passes the wrong id fails.callSite()reads the stack-frame path up to.ts:line:col, so a checkout path with spaces (or a drive letter) still parses.<root:turn>,ws-record-turn), and the removal names its memory target keys. The test turns each name into digests with the code's own path functions (historyWriteLockPath,refineApplyLockPath,workspaceMutationLockPath,workspaceUseLockDir,targetMutationLockFilePath), so a digest shows as<hash:history:ws-record-turn>. Each digest scheme has its own name:history(SHA-256 of UTF-8, cut to 32 characters: the history and refine-apply locks),workspace-use(SHA-256 of UTF-16LE: the mutation gate and the use leases) andtarget(memory target keys). The test checks that the paths of one scheme share one digest, and that a digest keeps one name. So a lock built with the other scheme's digest shows the other name (review round 5). A digest or temp path with no name fails the test with a clear message (lock calls with no name: ...). So a lock built from the wrong id or root fails even when it occurs once. This replaced first-use numbering in review round 4, because rounds 2, 3 and 4 each found one more case that the numbering hid. Uuids (lease tokens) are masked. Rows are sorted and deduplicated, so the fixture is stable.HistoryServicewithcreateTestHistoryService().UPDATE_FIXTURE=1 bun test ./src/node/services/sessionLocks/sessionLocks.callOptions.test.ts, thenmake fmt.Validation
loopTestBaseline.ts --runs 3 --base origin/main) on the round-5 head: 0 trips. The round-5 fix changes only names in the test file and the fixture, and the phase lead counts this tripwire for the final head.bun test ./src/node/services/sessionLocks/sessionLocks.callOptions.test.ts: passes, about 1 s, and 10 more runs passed, also 3 repeats in one process.withLockkey intools/todo.tschanged -> the test fails with a diff on that row's key. Reverted.AgentSessionholds the turn lease for a wrong workspace id -> the test fails on theholdrow and on the history lock token. Reverted.historyService.tsbuilds the history write lock from${workspaceId}-other-> the test fails withlock calls with no name: <the three new digests>. Reverted.historyService.tsbuilds the history write lock path from theworkspaceUseLockDirdigest (the UTF-16LE scheme) -> the test fails with a diff fromhistory-<hash:history:...>tohistory-<hash:workspace-use:...>on each history row. Reverted.workspaceUseLeases.tspassescreateParentDirectory: falseto the turn lease'sacquireCrossProcessLock-> the test fails with a diff that adds"createParentDirectory": falseto both lease rows. Reverted. (No call in the scenarios passesfalsetoday, onlyWorkflowRunStore, so the scratch edit adds the option where the review finding said a move could add it. At the turn owner lock inworkspaceTurnManager.tsthe same edit makes the scenario itself fail before the record compare, because that lock directory does not exist yet.)workspaceRemoval.tschanged -> the test fails with a diff on that row. Reverted.make testand its second half (tests/ui/storybook/,tests/ui/domIsolation.test.ts,vscode/src/,scripts/) on the 🤖 tests: record the session lock calls of 18 more files #7291 head 069b820 (which contains this PR) pass exceptWorkspaceMenuBar.test.tsxandscripts/perf/pageTimingCore.test.ts, known failures on this older base (fixed on main by 🤖 tests: make test-unit runs CI's isolated files alone and ends a leaked metadata loop #7305), and the known load flakebackgroundProcessManager.test.ts, which passed alone (163 of 163).scripts/mutate/check.ts, base = this PR's base): PASS with 0 mutants. The PR changes only test files, so no survivor needs a disposition.Known limits of the record
WorkspaceUseLeases.hold()inspects the mutation gate before it acquires the use-lease file. Phase 4 PR 4 adds an explicit order check for that (review round 6).Deferred review findings
#7326 tracks the findings that this PR declined, each with the PR that must close it: the two limits above, seeding HistoryService in
beforeEach(round 4) and Windows path separators (round 2).Risks
Test-only. The record depends on stack traces: if a refactor changes which source file makes a call, the site of a row changes although the options do not. The PR that does so records the fixture again and explains the row change.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$70.87