Skip to content

refactor(sessions): route new session store bypasses through the accessor#101179

Merged
jalehman merged 5 commits into
mainfrom
b62.3/route-new-bypass-sites
Jul 7, 2026
Merged

refactor(sessions): route new session store bypasses through the accessor#101179
jalehman merged 5 commits into
mainfrom
b62.3/route-new-bypass-sites

Conversation

@jalehman

@jalehman jalehman commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Recent features added session/transcript persistence that bypasses the session accessor (#88838's storage-neutral boundary): thread-compaction results persist via direct updateSessionStore in src/gateway/server-methods/sessions.ts (#98510), the cron isolated-agent guard read uses raw loadSessionStore({skipCache:true}) (#98510), and session fork writes transcript JSONL files via raw fs.writeFile (#100814). Direct store/file calls in runtime code couple these features to the file layout and dodge the boundary guard.

Why This Change Was Made

The accessor header states the contract: new runtime callers use the accessor, not store.ts. This PR re-expresses the new call sites through the accessor with zero behavior change — persistence only, no product-logic movement:

  • Gateway thread-compaction persists through applySessionPatchProjection (the same API sessions.patch from the same PR already uses), preserving the exact guard semantics (sessionId match, lifecycleRevision match, work-start error check, alias promote/prune, boolean persisted result).
  • The cron guard read uses loadSessionEntry({readConsistency: "latest"}) — the accessor already models the skip-cache need; freshness semantics unchanged.
  • Fork transcript creation routes through a narrow new accessor method createForkedSessionTranscript (file-backed; same wx/0600/naming semantics; the SQLite mapping is documented on the method). Net −35 LOC in the fork runtime.
  • One fix(compaction): avoid cached usage overcount #99864 readSessionTranscriptIndex call in session-utils.fs.ts is legitimately file-tier (operates on an already-resolved artifact path inside the file backend the reader seam delegates to) — documented with a contract comment instead of rewired.

Migrated files are added to the boundary guard's enforcement lists so they cannot regress.

User Impact

None — behavior-preserving refactor. Thread compaction, cron session admission, and session fork behave identically; existing tests for compaction guards, fork on-disk bytes, and cron lifecycle all pass unchanged.

Evidence

  • New behavior tests: src/cron/isolated-agent/session.latest-read.test.ts (guard read observes latest persisted row past a still-current cache snapshot), src/config/sessions/session-accessor.fork-transcript.test.ts (fork header lineage + ordered record copy).
  • Existing suites green: server.sessions.compaction.test.ts (36 tests incl. stale-persist rejection and persistence-failure terminal events), server.sessions.create.test.ts, session-fork.runtime.test.ts + session-fork.test.ts, cron session.test.ts, run.session-lifecycle.test.ts, delivery-dispatch.double-announce.test.ts.
  • node scripts/check-session-accessor-boundary.mjs passes; tsgo core + core-test lanes clean; oxfmt/oxlint clean.
  • Structured review (codex/gpt-5.5) clean, zero findings.

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

An identical scenario was run against a real gateway process (isolated temp state, no live models) at this head (64a65136efa) and at the merge-base (e80e8a2b673); the normalized on-disk state dumps (session store + transcript bytes) are byte-identical across two runs per side, with 29/29 raw-byte assertions passing on both sides — demonstrating zero behavior change on all three rerouted paths.

  • Fork: real sessions.create fork over WebSocket; forked transcript verified on disk — <timestamp>_<sessionId>.jsonl naming, mode 0600, parentSession lineage header, message records copied in order and byte-equal to the parent's, store entry referencing the fork.
  • Compaction persist: real offline compaction through a loopback mock of the OpenAI responses API; bookkeeping verified on disk (legacy alias promoted to canonical key, compactionCount advanced, token/budget fields deleted, checkpoint + compaction record written). Negative probe: a second OS process rewrites lifecycleRevision while the model response is held in flight — the guarded persist rejects with session-changed and applies nothing.
  • Cron latest-read: a second OS process rewrites the session row with identical byte length and restored mtime so the in-process store cache verifiably still serves the stale snapshot; the guard read returns the just-persisted row.

Scope disclosure: the cron leg invokes the production guard function (loadCronSessionEntryLatest) directly rather than waiting on the full scheduler runtime; the cross-process freshness contract is what it proves, identically on both sides.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime scripts Repository scripts size: M maintainer Maintainer-authored PR labels Jul 6, 2026
@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): route new session store bypasses through the 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 mentioned this pull request Jul 6, 2026
Merged
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. 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:39 PM ET / 00:39 UTC.

Summary
The branch reroutes compaction persistence, cron latest-row reads, and fork transcript writes through session-accessor APIs, adds focused tests, and ratchets the boundary guard baseline.

PR surface: Source +56, Tests +129, Other +2. Total +187 across 11 files.

Reproducibility: not applicable. this is a behavior-preserving refactor, not a bug report with a failing reproduction path. The review evidence is source-path comparison, focused tests in the diff, current-main inspection, and the PR body's A/B runtime proof.

Review metrics: 2 noteworthy metrics.

  • Runtime accessor bypasses: 3 rerouted. Compaction persistence, cron latest reads, and fork transcript writes are session-state paths where behavior equivalence matters before merge.
  • Boundary guard ratchet: 2 migrated-list additions, 2 baseline reductions. The guard and baseline changes determine whether future direct store or transcript bypasses are caught by automation.

Stored data model
Persistent data-model change detected: serialized state: scripts/lib/session-accessor-debt-baseline.json, serialized state: src/auto-reply/reply/session-fork.runtime.ts, serialized state: src/config/sessions/session-accessor.fork-transcript.test.ts, serialized state: src/config/sessions/session-accessor.ts, serialized state: src/config/sessions/transcript-header.ts, serialized state: src/cron/isolated-agent/session.latest-read.test.ts, and 7 more. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #88838
Summary: This PR is a narrow candidate slice for the canonical session/transcript accessor migration tracker, with adjacent overlap from recent session-management, fork, compaction, and guard-ratchet work.

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: 🦞 diamond lobster
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:

  • Refresh the branch against current main and keep the session-accessor boundary guard plus focused session-state checks green on the exact merge head.

Risk before merge

  • [P1] The diff changes stored session/transcript persistence paths for compaction, cron admission checks, and fork creation; a wrong projection could stale or mis-associate session state even though the intended behavior is unchanged.
  • [P1] The diff changes the session-accessor boundary guard and debt baseline, so the exact merge head should keep the guard green and still catch new bypasses.
  • [P1] The live PR is mergeable but behind current main, so maintainers should review the refreshed merge result rather than treating the old exact head as final.

Maintainer options:

  1. Refresh and land exact head (recommended)
    Rebase or merge current main, rerun the session-accessor guard and relevant session-state checks, then land if the maintainer review stays clean.
  2. Accept the existing proof
    A maintainer can decide that the current A/B proof and green checks are enough despite the branch being behind.
  3. Fold into storage tracker
    Pause this PR if maintainers prefer one combined session/transcript storage cleanup under the canonical tracker.

Next step before merge

  • [P2] Protected maintainer-labeled PR with session-state and guard-automation merge risk; maintainers should review the refreshed exact head rather than queue a repair.

Maintainer decision needed

  • Question: Should this accessor-boundary slice land separately after refresh, or be folded into the broader session/transcript storage lane?
  • Rationale: The patch has no concrete defect, but the protected maintainer label and the open storage-seam tracker make the final sequencing and ownership call a human session-state decision.
  • Likely owner: jalehman — They own the tracker and guard-ratchet context most directly and can judge whether this slice should land separately.
  • Options:
    • Land as a narrow slice (recommended): Rebase or otherwise refresh the branch, confirm exact-head guard and session-state proof, and land this focused cleanup independently.
    • Fold into the broader lane: Close or pause this branch and carry the same call-site cleanup inside the active session/transcript accessor or SQLite storage work.
    • Ask for more proof: Require a linked artifact or fuller terminal transcript before maintainer approval if body-level A/B proof is not enough for this surface.

Security
Cleared: No concrete security or supply-chain concern was found; the diff changes TypeScript session persistence helpers, tests, and a local boundary guard without new dependencies, workflows, secrets, or package-resolution changes.

Review details

Best possible solution:

Land the narrow accessor cleanup after maintainer review on the current base, with the boundary guard and focused session-state proof still green on the exact merge head.

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

Not applicable: this is a behavior-preserving refactor, not a bug report with a failing reproduction path. The review evidence is source-path comparison, focused tests in the diff, current-main inspection, and the PR body's A/B runtime proof.

Is this the best way to solve the issue?

Yes: the accessor boundary is the right owner for these runtime persistence call sites, and I did not find a concrete patch defect. The safer alternative is not a different implementation but refreshing the branch and confirming the same proof on the current merge head.

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 describes an after-change real gateway A/B equivalence run at the exact head for fork, compaction, and cron latest-read behavior, including on-disk state checks and assertion counts.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body now describes an after-change real gateway A/B equivalence run at the exact head for fork, compaction, and cron latest-read behavior, including on-disk state checks and assertion counts.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a bounded but compatibility-sensitive session-state refactor with maintainer-review gates, not an emergency outage.
  • merge-risk: 🚨 session-state: The diff changes stored session/transcript persistence paths for compaction, cron admission checks, and fork creation.
  • merge-risk: 🚨 automation: The diff changes the session-accessor boundary guard and debt baseline that CI uses to catch future bypasses.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body now describes an after-change real gateway A/B equivalence run at the exact head for fork, compaction, and cron latest-read behavior, including on-disk state checks and assertion counts.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body now describes an after-change real gateway A/B equivalence run at the exact head for fork, compaction, and cron latest-read behavior, including on-disk state checks and assertion counts.
Evidence reviewed

PR surface:

Source +56, Tests +129, Other +2. Total +187 across 11 files.

View PR surface stats
Area Files Added Removed Net
Source 6 163 107 +56
Tests 3 129 0 +129
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 2 4 2 +2
Total 11 296 109 +187

What I checked:

  • Repository policy read: Root AGENTS.md and scoped scripts, gateway, gateway/server-methods, and test guides were read fully enough for the touched paths; the review applied the session-state, accessor-boundary, and automation-risk guidance. (AGENTS.md:1, 74bb2ad7a533)
  • Current main still needs the cleanup: Current main still has the direct compaction updateSessionStore calls and fork transcript file-writing helpers, so the central refactor is not already implemented on main. (src/gateway/server-methods/sessions.ts:2555, 74bb2ad7a533)
  • Gateway compaction projection: At the PR head, sessions.compact resolves alias migration and terminal compaction persistence through applySessionPatchProjection while preserving sessionId, lifecycleRevision, and work-start guards. (src/gateway/server-methods/sessions.ts:2557, 64a65136efad)
  • Fork transcript accessor: The PR adds createForkedSessionTranscript to centralize fork transcript identity, header lineage, create-only mode, and JSONL serialization under the session accessor boundary. (src/config/sessions/session-accessor.ts:1363, 64a65136efad)
  • Fork runtime caller: The fork runtime delegates both branched and header-only transcript creation to createForkedSessionTranscript while keeping branch record and leaf-control selection local. (src/auto-reply/reply/session-fork.runtime.ts:268, 64a65136efad)
  • Cron latest read through accessor: loadCronSessionEntryLatest now calls loadSessionEntry with readConsistency: latest, which maps to skipCache inside the accessor and is covered by a cache-staleness regression test. (src/cron/isolated-agent/session.ts:125, 64a65136efad)

Likely related people:

  • jalehman: They authored the canonical accessor tracker and the merged boundary debt-ratchet PR, and this branch continues the same session-accessor guard and seam work. (role: feature owner and recent area contributor; confidence: high; commits: c7295e417d5d, 64a65136efad; files: scripts/check-session-accessor-boundary.mjs, scripts/lib/session-accessor-debt-baseline.json, src/config/sessions/session-accessor.ts)
  • steipete: They authored the merged session-management PRs that introduced the compaction, cron, and fork surfaces this refactor is cleaning up. (role: recent adjacent contributor; confidence: medium; commits: 6df0fb818d67, 88f1ec38d4a5; files: src/gateway/server-methods/sessions.ts, src/cron/isolated-agent/session.ts, src/auto-reply/reply/session-fork.runtime.ts)
  • LZY3538: They authored recent merged compaction work touching the session usage and transcript utility paths referenced by this PR's file-tier comment. (role: adjacent compaction contributor; confidence: medium; commits: e6ec74c25430; files: src/gateway/session-utils.fs.ts, src/gateway/server-methods/sessions.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:23:37.555Z sha 56dafb8 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-06T22:21:06.042Z sha 64a6513 :: needs real behavior proof before merge. :: none

@jalehman
jalehman force-pushed the b62.3/route-new-bypass-sites branch from 56dafb8 to 64a6513 Compare July 6, 2026 21:58
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 7, 2026
@jalehman
jalehman merged commit 4e2a80c into main Jul 7, 2026
129 of 137 checks passed
@jalehman
jalehman deleted the b62.3/route-new-bypass-sites branch July 7, 2026 01:05
@github-actions github-actions Bot mentioned this pull request Jul 7, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 7, 2026
@jalehman
refactor(sessions): route new session store bypasses through the acce…
c3a04e7 
…ssor (openclaw#101179)
@jalehman jalehman 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): route new session store bypasses through the acce…
49a3ad5 
…ssor (openclaw#101179)
@obviyus obviyus mentioned this pull request Jul 12, 2026
Merged
@obviyus obviyus mentioned this pull request Jul 12, 2026
Closed
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

gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. 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: M 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