Skip to content

refactor(sessions): move inbound meta, goals, and delivery reads behind the session accessor#101180

Merged
jalehman merged 8 commits into
mainfrom
b62.1/split-brain-accessor-closure
Jul 7, 2026
Merged

refactor(sessions): move inbound meta, goals, and delivery reads behind the session accessor#101180
jalehman merged 8 commits into
mainfrom
b62.1/split-brain-accessor-closure

Conversation

@jalehman

@jalehman jalehman commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Four runtime surfaces still read/write the JSON session store directly through store.ts, bypassing the session accessor (#88838): inbound session meta + last-route recording (channels core and the plugin SDK, on every channel message), session goals, delivery-info snapshot reads, and compaction-count reconciliation after subscribed runs. Anything behind the accessor can swap storage backends; these four cannot — they would split-brain against a non-file backend.

Why This Change Was Made

Moves the persistence of all four surfaces behind the accessor, file-backed, with zero behavior change:

  • Two named accessor methods, recordInboundSessionMeta and updateSessionLastRoute, own inbound meta persistence. File-backed implementations delegate to the existing store.ts bodies, preserving createIfMissing upsert, preserve-activity merges (inbound meta must not refresh updatedAt — idle reset depends on it), legacy-key normalization, and cache ownership byte-identically.
  • Plugin SDK facades keep their exported names/signatures (recordSessionMetaFromInbound, updateLastRoute) — internals route through the accessor; no SDK contract change.
  • goals.ts and the compaction-count handler use accessor entry get/patch; delivery-info.ts reads through a new narrow openSessionEntryReadView — a raw borrowed keyed view (exact-key probes stay cheap property reads; enumeration defers until a normalized fallback is actually needed), which listSessionEntries now also delegates to.
  • Migrated files are added to the boundary guard lists; the guard's legacy-name sets gain readSessionStoreSnapshot, recordSessionMetaFromInbound, and updateLastRoute so new bypasses of these operations fail CI.

User Impact

None — behavior-preserving refactor. Channel routing metadata, goal tracking, delivery-info resolution, and compaction counts behave identically; delivery lookups keep their exact-key fast path (guarded by a restored enumeration-trap test).

Evidence

  • Accessor tests: 68/68 pass, including 4 new (preserve-activity for both methods, createIfMissing:false, detached returns) and a raw-probe contract test for the read view (exact key hits; folded alias returns undefined; entries() enumerates).
  • Laziness mutation-verified: forcing the normalized-index build makes the restored "does not enumerate the store when an exact routable key is present" test fail; shipped code passes.
  • Suites green: delivery-info (24), goals, channels session, outbound-session, compaction handler, goal-tools, session-key normalization (now proven through the accessor boundary), both plugin-SDK facade tests.
  • tsgo core + core-test lanes exit 0; import-cycle check 0 cycles; pnpm build green with no INEFFECTIVE_DYNAMIC_IMPORT (the inbound lazy boundary re-export changed).
  • Plugin SDK API baseline regenerated via the sanctioned generator — export names/signatures unchanged (declaration source moved); flagged for maintainer sign-off.
  • node scripts/check-session-accessor-boundary.mjs passes; structured review (codex/gpt-5.5) clean after the delivery-laziness fix, zero remaining findings.

Real behavior proof (A/B equivalence vs merge-base)

An identical scenario was run against isolated stores at this head and at the merge-base (e80e8a2b673); all four normalized state dumps are byte-identical (one sha256 across two runs per side), with 36/36 raw-byte assertions passing on both sides — demonstrating zero behavior change on all four rerouted surfaces.

  • Preserve-activity (the key contract): after an inbound meta write that demonstrably lands new metadata on an existing entry, the serialized updatedAt token is byte-identical — idle-reset timing cannot drift. Proven three ways: inbound meta, updateLastRoute (whose base implementation is also preserve-activity per its Session idle/daily reset never fires: updateLastRoute() bumps updatedAt before freshness check #49515 contract comment — asserted identically on both sides), and through the legacy-alias collapse path.
  • Inbound meta/last-route semantics: createIfMissing upsert, createIfMissing: false null-return, and structural alias-key collapse to the canonical key with seeded origin markers preserved through the merge.
  • Delivery-info resolution: seeded exact/alias/routable/non-routable entry combinations resolve with the routable > exact > freshest preference order preserved.
  • Compaction-count reconciliation: max-only advance (0→3; a late 3→1 write leaves it at 3) with updatedAt monotonicity.
  • Plugin SDK facade: recordSessionMetaFromInbound/updateLastRoute still resolve as callable exports from session-store-runtime and config-runtime.

Scope disclosure: the inbound leg drives the plugin-SDK facade exports — the exact seam channel plugins call in production — because every fully-real channel path requires a live network transport; goals and delivery-info legs are direct-seam probes against the isolated store for the same reason. The rerouted persistence code executes for real in every leg.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation scripts Repository scripts agents Agent runtime and tooling size: L maintainer Maintainer-authored PR labels Jul 6, 2026
@jalehman
jalehman force-pushed the b62.1/split-brain-accessor-closure branch from 33cc0e0 to 5356a95 Compare July 6, 2026 20:33
@jalehman

jalehman commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

ClawSweeper status: review started.

I am starting a fresh review of this pull request: refactor(sessions): move inbound meta, goals, and delivery reads behind the session accessor This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jul 6, 2026
@clawsweeper

clawsweeper Bot commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 6, 2026, 8:41 PM ET / 00:41 UTC.

Summary
The branch adds session-accessor read/write seams and reroutes inbound metadata, last-route updates, goals, delivery-info reads, compaction reconciliation, SDK facades, and boundary guard baselines through that accessor.

PR surface: Source +71, Tests +122, Generated 0, Other +11. Total +204 across 20 files.

Reproducibility: not applicable. this is a behavior-preserving refactor PR, not a bug report with a failing user path. The review used source, diff, tests in the PR, live PR state, and related tracker context.

Review metrics: 3 noteworthy metrics.

  • SDK Facade Exports: 2 rerouted, 0 renamed or removed. The PR touches shipped plugin SDK names, so unchanged signatures still need compatibility proof.
  • Accessor Operations: 3 added. recordInboundSessionMeta, updateSessionLastRoute, and openSessionEntryReadView become new storage-neutral seams future backends must implement consistently.
  • Boundary Guard Names: 3 legacy helpers added. Adding readSessionStoreSnapshot, recordSessionMetaFromInbound, and updateLastRoute changes the guard's CI enforcement surface.

Stored data model
Persistent data-model change detected: serialized state: scripts/lib/session-accessor-debt-baseline.json, serialized state: src/channels/session.test.ts, serialized state: src/config/sessions/delivery-info.test.ts, serialized state: src/config/sessions/delivery-info.ts, serialized state: src/config/sessions/goals.ts, serialized state: src/config/sessions/inbound.runtime.ts, and 11 more. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: partial_overlap
Canonical: #88838
Summary: This PR is a narrow accessor-adoption slice under the canonical session/transcript SQLite accessor-seam tracker; sibling slices and broad storage-flip work overlap but do not safely replace this branch yet.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Rebase onto current main and rerun the focused accessor, delivery-info, goals, compaction, plugin SDK API, and boundary-guard checks on the exact head.
  • Keep the SDK export/signature compatibility proof visible for maintainer signoff.

Risk before merge

  • [P1] The PR reroutes shipped plugin SDK facade names to accessor-backed implementations; subtle behavior drift could affect plugins even though names and signatures stay unchanged.
  • [P1] Session-state behavior is compatibility-sensitive because inbound metadata, last-route persistence, goals, delivery lookup, and compaction counts must stay semantically identical while moving behind the accessor.
  • [P1] The head is behind current main, and main changed src/config/sessions/store.ts after the PR base; the exact merge result should be revalidated before landing.

Maintainer options:

  1. Rebase And Validate Exact Head (recommended)
    Rebase onto current main and rerun the focused accessor, SDK API, delivery-info, goals, compaction, and boundary-guard checks before merge.
  2. Require SDK Compatibility Signoff
    Treat the unchanged SDK export names as a compatibility surface and verify regenerated baselines plus runtime behavior before landing.
  3. Fold Into Broader Flip
    If maintainers want one storage-flip lane, move this PR's unique migrations into that branch and close this slice only after replacement proof exists.

Next step before merge

  • [P2] Protected maintainer-labeled session storage seam work needs human sequencing plus rebase validation; no narrow automated repair is identified.

Maintainer decision needed

  • Question: Should this focused accessor-adoption slice land independently after rebase validation, or be folded into the broader session/transcript storage-flip lane?
  • Rationale: The code follows the accessor-boundary direction, but the maintainer label, canonical migration tracker, SDK facade touch, and behind-base store.ts change make sequencing a human ownership decision.
  • Likely owner: jalehman — jalehman owns the tracker text and the recent sequence of accessor-seam PRs that this slice belongs to.
  • Options:
    • Land Focused Slice (recommended): Rebase onto current main, rerun the focused accessor, delivery-info, goals, compaction, SDK API, and boundary-guard checks, then review this PR as an independent seam migration.
    • Fold Into Storage Flip: Move the unique accessor migrations into the broader SQLite flip branch and supersede this PR only after the replacement branch contains equivalent proof.
    • Pause For Tracker Direction: Keep this PR open but defer merge work until maintainers confirm the active implementation lane for the canonical migration tracker.

Security
Cleared: No concrete security or supply-chain concern was found; the diff changes TypeScript session accessors/callers, tests, generated SDK hash metadata, and a local guard script without new dependencies, permissions, secrets handling, or downloaded code.

Review details

Best possible solution:

Land this focused accessor slice after rebase and exact-head validation if maintainers want separate seam adoption; otherwise carry its unique migrations into the broader storage-flip branch before superseding it.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this is a behavior-preserving refactor PR, not a bug report with a failing user path. The review used source, diff, tests in the PR, live PR state, and related tracker context.

Is this the best way to solve the issue?

Yes: the accessor boundary is the right layer for this storage-neutral cleanup, and preserving the SDK-facing names is the safer compatibility shape. It still needs maintainer sequencing and exact-head validation before merge.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 74bb2ad7a533.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body now includes after-change A/B equivalence proof against the merge-base with isolated stores, byte-identical normalized state dumps, and 36/36 raw-byte assertions over the rerouted surfaces; maintainers should still ask for exact rebased-head validation.

Label justifications:

  • P2: This is an important behavior-preserving session-state refactor with limited immediate user blast radius.
  • merge-risk: 🚨 compatibility: The diff reroutes shipped plugin SDK facade exports, so subtle signature or behavior drift could affect plugins despite unchanged names.
  • merge-risk: 🚨 session-state: The diff moves session metadata, route, goal, delivery, and compaction reads/writes behind a new seam where semantic drift could stale or mis-associate session state.
  • merge-risk: 🚨 automation: The diff changes the session-accessor boundary guard and debt baseline, which can affect CI enforcement after merge.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body now includes after-change A/B equivalence proof against the merge-base with isolated stores, byte-identical normalized state dumps, and 36/36 raw-byte assertions over the rerouted surfaces; maintainers should still ask for exact rebased-head validation.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body now includes after-change A/B equivalence proof against the merge-base with isolated stores, byte-identical normalized state dumps, and 36/36 raw-byte assertions over the rerouted surfaces; maintainers should still ask for exact rebased-head validation.
Evidence reviewed

PR surface:

Source +71, Tests +122, Generated 0, Other +11. Total +204 across 20 files.

View PR surface stats
Area Files Added Removed Net
Source 11 183 112 +71
Tests 6 178 56 +122
Docs 0 0 0 0
Config 0 0 0 0
Generated 1 2 2 0
Other 2 14 3 +11
Total 20 377 173 +204

What I checked:

  • Repository policy read: Root AGENTS.md and scoped guides for plugin SDK, agents, channels, plugins, outbound, scripts, and docs were read; their SDK-boundary, session-state, accessor, generated-artifact, and validation guidance shaped the review. (AGENTS.md:1, 74bb2ad7a533)
  • Live PR state: Live GitHub state shows the PR is open, non-draft, mergeable but behind current main, at head 52c9e7e, and protected by the maintainer label. (52c9e7ec5107)
  • Accessor boundary implementation: The PR head adds a borrowed raw read view plus recordInboundSessionMeta and updateSessionLastRoute accessors, keeping the new storage-neutral boundary in session-accessor.ts rather than adding direct store readers to callers. (src/config/sessions/session-accessor.ts:950, 52c9e7ec5107)
  • File-backed behavior preserved: The new inbound and last-route accessors delegate to the existing file-backed store functions, whose current-main implementations preserve createIfMissing, alias canonicalization, detached returns, and preserve-activity timestamp behavior. (src/config/sessions/store.ts:1922, 74bb2ad7a533)
  • Delivery lookup invariant: Delivery-info lookup now uses openSessionEntryReadView but keeps raw exact-key probes before fallback enumeration, and the test suite includes an enumeration-trap check for the cheap exact-key path. (src/config/sessions/delivery-info.ts:96, 52c9e7ec5107)
  • SDK contract preservation: Plugin SDK facades continue exporting recordSessionMetaFromInbound and updateLastRoute as the shipped names, while their implementations route to the accessor-backed functions. (src/plugin-sdk/session-store-runtime.ts:230, 52c9e7ec5107)

Likely related people:

  • jalehman: Josh Lehman authored this PR, the canonical accessor tracker, the sibling accessor slice, and many merged session-accessor migration PRs found in live GitHub history. (role: feature owner and recent area contributor; confidence: high; commits: 58aa59eaf805, f47515f02e51, 52c9e7ec5107; files: src/config/sessions/session-accessor.ts, src/config/sessions/delivery-info.ts, src/plugin-sdk/session-store-runtime.ts)
  • steipete: Peter Steinberger authored the current-main store.ts session-rotation fix that landed after this PR's base, making him relevant for exact-merge validation of the delegated file-backed behavior. (role: recent adjacent contributor; confidence: medium; commits: 254b819d7d8f; files: src/config/sessions/store.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (2 earlier review cycles)
  • reviewed 2026-07-06T21:21:15.013Z sha 5356a95 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-06T22:19:47.091Z sha 52c9e7e :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. label Jul 6, 2026
@jalehman
jalehman force-pushed the b62.1/split-brain-accessor-closure branch from 5356a95 to 52c9e7e Compare July 6, 2026 22:02
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 6, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 6, 2026
Merged
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jul 7, 2026
@jalehman
jalehman merged commit cb0d8a1 into main Jul 7, 2026
124 of 126 checks passed
@jalehman
jalehman deleted the b62.1/split-brain-accessor-closure branch July 7, 2026 01:07
@github-actions github-actions Bot mentioned this pull request Jul 7, 2026
Open
This was referenced Jul 7, 2026
Merged
Merged
Closed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 7, 2026
@jalehman
refactor(sessions): move inbound meta, goals, and delivery reads behi…
f326cfe 
…nd the session accessor (openclaw#101180)
@clawsweeper clawsweeper Bot mentioned this pull request Jul 7, 2026
Merged
giodl73-repo pushed a commit to giodl73-repo/openclaw that referenced this pull request Jul 8, 2026
@jalehman @giodl73-repo
refactor(sessions): move inbound meta, goals, and delivery reads behi…
91ddd02 
…nd the session accessor (openclaw#101180)
@clawsweeper clawsweeper Bot mentioned this pull request Jul 11, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling docs Improvements or additions to documentation maintainer Maintainer-authored PR merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. scripts Repository scripts size: L status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jalehman