Repository navigation
test: open, seed and read the agent-session record store through one test harness - #23986
Merged
Merged
Conversation
…harness Tests that open the durable agent-session record store, seed it, or read back what it persisted now go through agent-session-record-store-test-harness.ts instead of calling AgentSessionRecordStore.open or touching agent-sessions.json themselves. A later change that moves the store into the chat database then changes the harness instead of every test. No production code changes. Tests whose subject is the JSON file itself (its .bak recovery, salvage, schema versions, permissions, and what older builds read back) keep reading and writing the file directly; the storage move rewrites or deletes them.
The harness took the store's own folder, so each caller picked one (join(root, 'store'), or 'agent-sessions' where a test read the store the runtime owns). A later change that moves the store into the state directory's journal database could not tell those apart, and would have had to edit every caller again. Every harness function now takes the state directory, the one the test's journal database and recovery capsule already live in, and keeps the store in the same subfolder the runtime uses. Callers pass that directory; store-only tests pass their temp directory unchanged. Format tests that share a directory with harness calls take the file path from testAgentSessionStoreFilePath. The folder name moves from a private constant in the runtime to AGENT_SESSION_STORE_DIR_NAME beside the store's file name, so the harness shares it without importing the runtime. Its value and every path built from it are unchanged.
Contributor
Author
Review status: ready for review (not merged)The problem. The planned storage move (step D) moves the agent-session record store out of What changes for users. Nothing. It is a test-only change. The one production edit moves the private Fixed during review (ed88d25)
Deferred (P3, not worth changing here)
Verified
Not verified
|
brennanb2025
marked this pull request as ready for review
September 30, 2026 03:54
 |
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (81)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds a shared test harness for the agent-session record store and migrates tests across mobile, native chat, runtime, RPC, and end-to-end areas to use it. The harness provides helpers to open stores, seed and edit persisted data, read persisted contents, and locate store files. The runtime now uses an exported constant for the store directory name. Test scenarios and assertions are generally unchanged. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The shared harness centralizes test-store setup without an established production behavior change or weakened persistence checks. No actionable merge-blocking issue remains in the supplied evidence. 🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 29.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 50 files. (31 skipped: 31 over the file limit.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Production constant move —
AGENT_SESSION_STORE_DIR_NAME = 'agent-sessions'moves from the runtime's privateRECORD_STORE_DIR_NAMEintoagent-session-record-store-file.ts;structured-agent-session-runtime.tsnow imports it. Same literal, same paths built byhasPersistedStructuredAgentSessionStoreandinstallOnJournal, so no behavior change. - New test harness (
src/main/runtime/agent-session-record-store-test-harness.ts) — one place toopen/seed/seedFromNewerBuild/read/readText/editthe store plus its file path. Each function takes the host's state directory and owns theagent-sessionssubfolder inside it, composed through the runtime's own constant andagentSessionStorePath. - 78 test/support files migrated — store opens and JSON reads now go through the harness; the two tests that assert on the running app's store (secret-never-reached-disk, spawn-token descendants) now read through the harness instead of a hardcoded path.
- Intentionally-left format tests — the JSON/
.bak/schema/permission suites and the mixed format cases keep reading the file directly; the PR body enumerates them for step D.
Verification in this run: read the full diff; ran the store-adjacent suites (record-store, tab-table, legacy-handoff-lease, import-mismatch, send-open-stale-turn, spawn-token-descendants, claude-root-exit) — 67/67 pass; pnpm tc:node passes; audit:anti-slop clean on the changed production files. An independent pass over every migrated call site found no case where the store's new <stateDirectory>/agent-sessions location diverges from a journal/capsule/copy/raw-read base used in the same test.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
7 of 9 tasks
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
1 participant
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.
ELI5
The app keeps a small durable list of every chat's session ownership (the "agent-session record store"). Today it lives in a file called
agent-sessions.json. A planned change moves it into the chat database the app already has. About 75 test files open, fill or read that store directly, so that move would have to edit every one of them in the same PR. This PR is a test-only step first: every one of those tests now goes through one small test harness, so the storage move only has to change the harness.This PR is step D0 of: R0 #23964 → L′-a #23978 → D0 (this) → D.
agent-sessions.jsoninto the chat database.What Changed
Nothing a user sees or does is different. The only production edit moves the private
'agent-sessions'folder-name constant from the runtime (structured-agent-session-runtime.ts) intoagent-session-record-store-file.tsasAGENT_SESSION_STORE_DIR_NAME, so the harness can share it without importing the runtime. Same value, same paths, no behaviour change. Everything else is test files and test-support files.New file
src/main/runtime/agent-session-record-store-test-harness.ts, built from what the existing tests actually do. Every function takes the host's state directory: the directory the chat database and the recovery file also live in, which is what the running app passes around. The harness owns theagent-sessionssubfolder inside it exactly as the running app does, so the storage move can open the store on the same database the test already uses. Callers pass that state directory instead of a store-onlystoresubfolder; tests that have no other state pass their temp directory as before.openTestAgentSessionRecordStore(stateDirectory, { hostId? }): open, or reopen, the store (122 call sites that calledAgentSessionRecordStore.open({ directory, hostId: 'local' })now call this).seedTestAgentSessionRecordStore(stateDirectory, { records }): leave records behind as an earlier run of the app would, before anything opens the store (replaces 4 hand-written copies of the JSON file layout).readPersistedTestAgentSessionStore(stateDirectory): read back what the store committed, typed (records, operation rows, retired keys, chat tabs).readPersistedTestAgentSessionStoreText(stateDirectory): the same as text, for "this secret never reached disk" and "this action wrote nothing" checks.editPersistedTestAgentSessionStore(stateDirectory, edit): change the committed state behind the store's back, as an older build or a damaged disk would (used by the older-build lease fixture and the record-quarantine test).seedTestAgentSessionStoreFromNewerBuild(stateDirectory): a store written by a newer build, which this build reads but never writes.testAgentSessionStoreFilePath(stateDirectory): where the store's file is, only for the format tests listed below that share a directory with harness calls.Two tests now read the store the running app itself writes, instead of reaching into its folder by hand: the integration test that checks a secret never reached disk, and the spawn-token descendants test that checks nothing is killed. Before, they named the app's
agent-sessionsfolder directly, so after the storage move they would have read an empty separate store and passed without checking anything.Migrated: 78 test and test-support files, including the shared host, rest, queued-message and restart-interruption rigs, the older-build lease fixture, the two tests/e2e unit tests that use the store as ordinary setup, and the two mobile tests that open it. Each test asserts exactly what it did before; only the way it reaches the store changed.
Left as they are on purpose, because the JSON file's format is itself what they test, so the storage move deletes or rewrites them rather than keeping them through a harness:
agent-session-backup-recovery.test.ts,agent-session-unreadable-record-salvage.test.ts,agent-session-record-store-security.test.ts,agent-session-record-unsupported-schema.test.ts,agent-session-recovery-publish-fault.test.ts(the.bakcopy, salvage, file permissions, schema versions, the save/load functions).tests/e2e/cross-version-wire/agent-session-death-evidence-downgrade.unit.test.tsandagent-session-unproven-release-downgrade.unit.test.ts(an older release reads the file this build wrote).structured-agent-session-runtime.test.ts(checks the production path the runtime opens)..bakcases inagent-session-record-store.test.ts; the byte comparisons of legacy lease values inagent-session-record-store-legacy-handoff-lease.test.ts(it opens the store and finds the file through the harness because it shares a directory with the older-build lease fixture, but still reads the JSON file); the tab table read from an older store, a stale record field, and a malformed table inagent-session-record-store-tab-table.test.ts; "an older build drops the table" instructured-chat-tab-table.test.ts; thevisibleSessionIdscheck instructured-agent-session-rollback-compatibility.test.ts; the unreadable-launch-payload case (which rewrites both the file and its.bak) inagent-launch-replay.test.ts; and the file-rename fault injection instructured-agent-session-settled-attach-retry.test.ts.Why
The storage move is one reviewable change only if it does not also carry about 800 lines of mechanical edits across 78 test files. Doing the mechanical part first, with no behaviour change, means the move's diff shows only what actually changes: how the harness opens, seeds and reads the store.
Alternative considered: change each test inside the storage-move PR. Rejected because it buries the real change under unrelated edits, and a reviewer cannot tell a mechanical edit from a changed assertion.
Alternative considered: also route the format tests above through the harness. Rejected because their assertions are about the JSON file (its bytes, its
.bak, what an older release parses), so no harness can keep them meaningful once the file is no longer written. Listing them here tells the storage move exactly what it must delete or rewrite.No "simulate an unreadable store" helper was added: every test that does that is a JSON or
.bakformat test from the list above, and the storage move changes what an unreadable store does (it is reported and the app carries on, rather than refusing to open).Linked Issue
None. Precursor for the planned record-store move (step D above).
Visual Proof
N/A: test-only change, nothing visible or interactive changed.
Testing
Ran every affected test file explicitly (103 files in the root suite: every file that touches the store, plus every consumer of the shared rigs and fixtures) before and after.
fb67d5d7c3f): 863 tests passed. One cross-version file hit a 45 s archive-extraction timeout in the parallel batch and passed alone (17/17).structured-agent-session-refusal-retry.test.ts, a 90 s test on a heavily loaded machine) passes alone in 17 s, the same as the origin/main copy of that file (18.5 s). Its only change is the one-line store open.structured-agent-session-claude-compact-stop.test.ts, a 1 s wait;structured-agent-session-refusal-retry.test.ts, the same 90 s test as above) and both pass alone.oxlinton all changed files andpnpm run check:code-quality:changed: clean.pnpm tc:nodepasses.Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Security, cross-platform, SSH, mobile, backwards compatibility and performance: not affected, since the only production edit moves a constant without changing its value or any path built from it. The harness builds paths with
path.jointhrough the existing store helpers.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)