Skip to content

feat(storage): add SessionRepository contract - #4662

Merged
MicroGery merged 11 commits into
mainfrom
codex/2370-session-repository-contract
Sep 7, 2026
Merged

feat(storage): add SessionRepository contract#4662
MicroGery merged 11 commits into
mainfrom
codex/2370-session-repository-contract

Conversation

@MicroGery

@MicroGery MicroGery commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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.publish and assertReadable publish and verify immutable Bundle and Manifest bytes.
  • ImmutableObjectStore.materialize copies one verified immutable object to a caller-owned new path, never overwrites it, and enforces a caller-provided byte limit.
  • SessionRepository owns 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:

Session head
  -> immutable SessionCheckpointManifestV1
       -> compatibilityBundle: immutable verified #2369 Bundle

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. materializeSessionCheckpointV1 converts the retained V1 Bundle reference into the bounded SessionBundleSource required 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

  • Stores immutable objects using no-replace publication, streaming digest/size verification and directory durability barriers.
  • Stores Session heads, revision allocation, commit receipts and Fork records in one validated, atomically replaced control-plane document.
  • Serializes control-plane mutations with the existing process-lifetime file-update lease. A killed writer releases the native lease, allowing the next writer to reclaim its ownerless marker. This is a local repository write lock, not a Cloud Session Activation lease; unidentified legacy directory locks are not stolen.
  • Covers real child-process SIGKILL before and after state-file replacement, idempotent commit recovery, and a subsequent fresh revision without manual lock cleanup.

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

  • Add the local durable Repository and Immutable Object Store adapter from feat(storage): add durable file SessionRepository #4674.
  • Wire this Repository into Cloud Session Activation and Fork orchestration.
  • Implement target state re-keying, repacking, and full Bundle hydration in the owning Fork work.
  • Add a remote storage backend and broader backend-specific crash/concurrency qualification.

Refs #2370

Verification

  • Core and Storage builds and Storage typecheck.
  • Focused SessionRepository, FileSessionRepository and shared file-update-lock tests: 31 passed.
  • New killed-writer recovery cases repeated 10 times: 20/20 passed. The before-rename case fails on the old implementation with a 10-second lock timeout.
  • Full local Storage suite: 1,149 passed, 8 skipped, 0 failed.
  • Shared CI baseline repair: 138 related contract tests and the real historical failing merge checked locally; the subsequent hosted run passed both locale and renderer gates.
  • Changed-file Biome, Windows skip inventory, clean-checkout ASF headers and diff whitespace checks passed.
  • Full hosted CI for the latest push remains to be confirmed.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — it adds the internal Cloud Session checkpoint Manifest, immutable object publication and materialization, revision CAS, Fork idempotency contract and local durable adapter described above, and corrects core CI comparison baselines.

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 3, 2026

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/storage/src/session-repository.ts
Comment thread packages/storage/src/session-repository.ts

@likun666661 likun666661 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/storage/src/session-repository.ts
* 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
@github-actions github-actions Bot added effort/XXL Over 2500 readable lines and removed effort/XL Under 2500 readable lines labels Sep 5, 2026
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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/storage/src/file-session-repository.ts Outdated
Comment thread .github/workflows/ci.yml Outdated
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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/storage/src/file-session-repository.ts Outdated
Comment thread packages/storage/src/session-repository.ts
Comment thread packages/storage/src/session-repository.ts Outdated
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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MicroGery
MicroGery merged commit 492ff80 into main Sep 7, 2026
1 check passed
@MicroGery
MicroGery deleted the codex/2370-session-repository-contract branch September 7, 2026 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants