feat(storage): add SessionRepository contract - #4662
Conversation
Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 914b2fc1d00e98861ff84fe6f3911591a0527faa. This adds the V1 checkpoint Manifest/object-store contract plus the in-memory SessionRepository conformance implementation and tests. I found two correctness gaps in Fork recovery and target provenance; details are inline.
Validation: clean install; Core and Storage builds; Storage typecheck; focused SessionRepository tests (18/18); full Storage tests (1,114 passed, 10 skipped); Biome, ASF headers, and diff checks; hosted test green. A clean synthetic merge against current main 01fe14bd271c38d65e82ce58d1448b10a5844a46 passed the same focused and full Storage checks. The first local full-suite run exposed an unrelated Node 22.22.1 warning-sensitive child-process test; suppressing the runtime warning made that isolated test and both full suites pass.
Not assessed: the durable backend and multi-process semantics, which this PR explicitly leaves to follow-up.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Generated-by: Codex
likun666661
left a comment
There was a problem hiding this comment.
Reviewed exact head 9b0b438746f99ec028a230be22f424ce411a3f87. The two earlier Fork findings are fixed, and the focused contract suite passes. One remaining contract-level recovery gap is inline.
Generated-by: Codex
* feat(storage): add durable file SessionRepository Generated-by: Codex * fix(storage): harden durable Fork publication Generated-by: Codex * fix(storage): harden durable SessionRepository Generated-by: Codex
Use the pull request merge commit's first parent instead of the stale event base, and verify that its second parent is the requested PR head. Preserve push and dispatch baselines and test refreshed-merge and wrong-checkout behavior. Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 57d3c0ae66e3a32a1c422d0fb7b0b6ad243e382c. The earlier Fork source-retention/materialization and inherited Activation findings are fixed. This head also adds a durable file-backed repository and a CI merge-base correction. I found two remaining issues inline.\n\nValidation: clean Node 24/npm 11 install; Core and Storage builds; focused SessionRepository/FileSessionRepository tests (26/26); full Storage suite (1,122 passed, 10 skipped, 0 failed); CI workflow policy tests (41/41); changed-file Biome, ASF headers, and git diff --check; and a clean patch-preserving merge onto current main dd7d1d595b7f9284e01fe76cf547c979a6d84a0a. For the CI failure, I reproduced locale hygiene failing against the stale event base and passing against the checked-out merge commit first parent.\n\nThe file adapter is still not exported or wired into production, so I did not assess a live Cloud Session consumer.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
…pository-contract
Resolve the PR merge base and checked-out head once for test planning, protocol epoch, locale hygiene and renderer architecture. Preserve push and dispatch behavior and exercise every consumer with real Git history and the workflow shell scripts. Generated-by: Codex
Use the existing process-lifetime file update lease for control-plane mutations. Exercise SIGKILL before and after the state rename, retry the interrupted commit idempotently and prove a fresh revision can still be published without removing lock state. Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 75d43ee1e91928fb48c9bbd9f40dfe4306ee4748. Three P2 findings remain, detailed inline: admitted commit identifiers can poison subsequent durable state reads; failed in-memory materialization deletes an existing destination; and concurrent in-memory Fork claims can reject an already admitted operation. All three were reproduced against the built implementation, without modifying production source or tampering with repository state. This revision is not ready to merge.
Problem and mechanism
The PR adds internal V1 checkpoint manifests, immutable object publication/materialization, Session head CAS, commit receipts, retained Fork source checkpoints and idempotency records, with in-memory and durable local adapters. The file adapter serializes control-plane updates and atomically replaces one state document. The associated CI repair derives one comparison from the actual checked-out merge for planning, protocol, locale and renderer checks.
The problem definition is supported by #2370 and the existing Bundle boundary: Bundle bytes alone do not supply Session identity, conditional publication or recoverable operation records. Keeping immutable bytes separate from the strongly consistent control plane follows the ownership requirements and first principles. Reusing the existing process-lifetime lock and one shared CI comparison is appropriately simple. The implementation is not yet optimal because the three localized contract failures remain; a deeper architecture rewrite is not required.
Previous findings
The earlier Fork source retention/materialization and inherited Activation issues are addressed. Both new real-child-process kill cases pass, including an additional fresh revision after recovery. A loader-only negative control restored the previous directory lock without editing source: the before-rename regression then failed after approximately 10 seconds with io_failure, confirming that the test detects the original failure. The stale-event-base correction now reaches all four CI consumers; the workflow and relevant gate tests pass.
Simplification and test quality
Remove the unconditional cleanup on failed exclusive creation; cleanup needs proof that this invocation created the file. Nonblocking: materializeSessionCheckpointV1 can check the declared size before I/O and omit its redundant Bundle assertReadable call, because materialize already owns exact-reference, size and digest verification. Retain Manifest verification. No other production-code deletion or test deletion is justified by this review. Keep the killed-writer tests and real-Git CI fixture. Add shared adapter regressions for preserving existing destinations, structured commit identities across reopen, and identical/conflicting concurrent Fork claims after source advancement. Those behavioral gaps explain why the current suites are green despite these failures.
Verification and limits
Clean Node 24.18.1/npm install; Core and Storage builds; Storage typecheck; focused repository/lock tests 31/31; full Storage suite 1,148 passed, 8 skipped, zero failures. The parallel CI review ran 239 distinct related tests successfully and checked the actual planner, protocol, locale and renderer commands. Changed-file Biome, ASF headers, Windows skip inventory and diff whitespace checks passed. The exact-head hosted test run 33979478401 is successful. git merge-tree also reports a clean merge with current main 02f97c16d76e644d5b565889701958293ff7b5fb; that synthetic merge was not rebuilt or tested.
Local execution was on macOS, not a Windows or Linux power-loss qualification. Real SIGKILL tests establish process-death recovery, not filesystem power-failure guarantees. Full Cloud Activation/Fork hydration, re-keying and repacking orchestration is explicitly outside this internal PR and was not assessed. The local JSON adapter's scale and broader remote-backend qualification remain follow-up work. Existing GitHub approval/green CI do not clear the three reproduced findings.
Automated review by Codex on behalf of hqhq1025. This is not an independent human review and does not replace one. No source fixes, commits, pushes or merge were performed.
Encode commit identity tuples unambiguously, preserve materialization destinations not created by the caller, and reconcile admitted Fork claims before source-currency validation. Add shared memory/file adapter regressions for special identifiers, existing and concurrent destinations, Fork claim races, partial-write cleanup, and duplicate receipt rejection. Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 958cfb48cc46740500f3bdc0c826a66fcebf5635. I found no remaining P0-P3 issue in this revision.
The latest commit fixes the three prior P2 findings: durable commit identities now use an unambiguous tuple encoding; failed or concurrent in-memory materialization removes a destination only after this call successfully acquired exclusive ownership; and an in-flight Fork claim reconciles an already admitted identical/conflicting operation before applying the final source-currency check. Shared memory/file conformance tests cover all three behaviors. The earlier retained Fork source, target Activation provenance, killed-writer recovery, and refreshed-merge CI comparison fixes also remain intact.
Validation passed with a clean Node 24.18.1/npm 11.19 install, full build:test, focused SessionRepository tests (44/44), the complete Storage suite (1,162 passed, 10 skipped), CI workflow policy tests (42/42), changed-file Biome, locale hygiene, Windows skip inventory, ASF headers, and git diff --check. The exact-head hosted test check is green. A synthetic merge onto current main (eca7778b1aa04ae21d33850cefd2f46ae7c7138a) is clean and preserves the PR patch.
The new repository remains an internal contract/adapter without a production Cloud Session consumer in this PR, so full hydration, re-keying, and remote-backend behavior were not assessed. Local execution was on Linux; native Windows/macOS and filesystem power-loss guarantees were not independently qualified. This feature still requires maintainer product review.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Summary
Implements the V1 Session checkpoint Manifest, immutable-object and SessionRepository contracts for #2370, with a deterministic in-memory conformance implementation and the file-backed local adapter incorporated from #4674.
The ports and adapter remain internal: they are not exported as a public package API or wired into a live Cloud Session consumer. This is not a production Cloud service or complete Activation/Fork orchestration.
Contract
The design keeps three operations distinct:
ImmutableObjectStore.publishandassertReadablepublish and verify immutable Bundle and Manifest bytes.ImmutableObjectStore.materializecopies one verified immutable object to a caller-owned new path, never overwrites it, and enforces a caller-provided byte limit.SessionRepositoryowns Session identity bindings, opaque Repository revisions, head CAS, current and exact checkout, and commit and Fork idempotency semantics.A visible Session head resolves through the V1 checkpoint envelope:
Publication follows the required order: publish and verify the compatibility Bundle; canonically encode, publish, and verify the Manifest; then create the Session or CAS-publish its head.
A first Fork claim verifies the explicit source revision, then persists its exact admitted checkpoint and agent binding. If the source later advances, retry still returns that claim.
materializeSessionCheckpointV1converts the retained V1 Bundle reference into the boundedSessionBundleSourcerequired by the existing file inspect and hydrate boundary. The future Fork owner can therefore hydrate, re-key, or repack the admitted source after a worker restart without re-reading the old head. This PR deliberately does not implement that future orchestration.The contract rejects same-Session Fork targets, requires target and source agent binding to agree, and rejects inherited Activation identity on a Fork-created target.
Local durable adapter
CI correction
The PR also fixes stale event-base comparisons in core CI. Test planning, protocol epoch, locale hygiene and renderer architecture share one validated comparison derived from the actual checked-out PR merge. Push and dispatch behavior is preserved; no check is disabled and no debt budget is relaxed.
Non-goals and follow-up
Refs #2370
Verification
AI use
Tool(s) and scope: Codex contributed to the SessionRepository contract, in-memory and local file-backed implementations, CI baseline repair, tests, and PR description under human contributor ownership.
Checklist
Does this PR entail a change in behavior?