Skip to content

refactor(sessions): route get-reply-run session refresh through the accessor#104880

Merged
obviyus merged 1 commit into
mainfrom
core/session-accessor-reroute
Jul 12, 2026
Merged

refactor(sessions): route get-reply-run session refresh through the accessor#104880
obviyus merged 1 commit into
mainfrom
core/session-accessor-reroute

Conversation

@obviyus

@obviyus obviyus commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

src/config/sessions/session-accessor.ts is the storage-neutral session boundary (ownership contract #88838), and most historical bypasses were already rerouted by #98236/#101179. One non-doctor whole-store read remained in the reply pipeline: runPreparedReply (src/auto-reply/reply/get-reply-run.ts) still imported resolveSessionStoreEntry from sessions/store.js and alias-scanned the in-memory session map to refresh the entry before onSessionPrepared. That is the layout-encoding pattern (whole Record + map-shape alias walk) the accessor exists to hide, and it kept get-reply-run.ts on the session-accessor debt baseline.

This is seam 8 of #104219 (session store accessor bypass), scoped to the residual mechanical caller. Re-verification against current main showed the seam is mostly complete upstream; the remaining bypasses are named owners (doctor whole-store scan/repair, state-migrations.ts) or shipped public surfaces (library.ts root exports, plugin-sdk/session-store-runtime.ts) that need their own compatibility decision, not a mechanical reroute.

Why This Change Was Made

The refresh now reads the working set by exact key and falls back to the accessor disk read, matching the two sibling refresh sites in the same function (get-reply-run.ts:852, :865-867) and the exact-key write at :842:

  • sessionStore[sessionKey] — the reply working set is exact-keyed everywhere else in runPreparedReply, so the old alias walk over the map had no live dependency.
  • loadSessionEntry({ storePath, sessionKey, readConsistency: "latest" }) — the accessor read performs full canonical-key and legacy-alias resolution internally (session-accessor.sqlite.ts readSessionEntryRow → collectSessionEntryLookupKeys), so legacy lowercase-alias repair is preserved whenever a store path is known.
  • Prepared sessionEntry remains the terminal fallback, unchanged.

The dead store.runtime.js mock in get-reply-run.media-only.test.ts is deleted (prod stopped importing it before this change; verified zero updateSessionStore references on main). The session-accessor debt baseline is regenerated because the ratchet fails on improvements (get-reply-run.ts drops off the debt list).

User Impact

None intended — pure boundary refactor; same reads, same fallback order for live states. Reply handling, session refresh, and onSessionPrepared payloads behave identically. Maintainer impact: the reply pipeline no longer imports whole-store file APIs, and the debt ratchet shrinks by one file.

Evidence

  • Sibling-idiom proof: exact-key reads/writes at src/auto-reply/reply/get-reply-run.ts:842,852,865-867 predate this change; the replaced call was the only alias-walking access in the function.
  • Alias-resolution parity: loadSessionEntry → loadSqliteSessionEntry → readSessionEntryRow builds plural lookup keys (collectSessionEntryLookupKeys), covering normalized + folded-legacy candidates the deleted resolveSessionStoreEntry handled.
  • git diff --numstat: +11/−17 (net −6), no new accessor surface, no behavior branches added.
  • Boundary guard: pnpm lint:tmp:session-accessor-boundary passes with the regenerated baseline (one-line deletion: get-reply-run.ts leaves the debt list).
  • Remote (Blacksmith Testbox): pnpm check:changed exit 0 on commit f6a6496; the six covering shards (incl. full auto-reply-reply: 141 files / 3,071 tests, and gateway-server) pass in isolated reruns on the same lease. The two failing PR checks are current-main breakage, reproduced on clean origin/main dfa580e9 — see the CI-triage comment.

Real behavior proof (ephemeral gateway, real Telegram channel path)

Built this branch (f6a64961427), ran a real gateway (temp OPENCLAW_STATE_DIR, loopback) with a mock Telegram Bot API (:18931) and mock OpenAI-completions provider (:18932), and drove two consecutive real reply turns through the Telegram channel into runPreparedReply:

  • Both turns delivered replies (sendMessage texts session-proof-reply-1, session-proof-reply-2 captured from the Bot API mock — ground truth).
  • One sessions row for the chat after both turns (SQLite dump of agents/main/agent/openclaw-agent.sqlite): sessionKey agent:main:telegram:group:-100777001, same sessionId ef4af902-… across both run traces, updatedAt advanced 1783824157995 → 1783824162754.
  • Gateway debug trace shows both turns' session turn created / embedded run start / message processed … outcome=completed on the same sessionId — the refreshed prepared-session binding (the changed accessor-backed read) held across turns.

Part of #104219 (seam 8: session store accessor bypass — residual mechanical caller; doctor and public-surface bypasses excluded as named owners).

@obviyus
refactor(sessions): route get-reply-run session refresh through the a…
f6a6496 
…ccessor

Drops the last non-doctor whole-store resolveSessionStoreEntry read in the
reply pipeline; exact working-set key plus accessor disk read match the
sibling refresh sites in runPreparedReply. Part of #104219 (seam 8).
@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: XS maintainer Maintainer-authored PR labels Jul 12, 2026
@obviyus

obviyus commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@obviyus

obviyus commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

CI triage: the two failing checks are current main breakage, not this PR.

Both failures reproduce on a clean origin/main checkout at dfa580e9 ("fix(plugins): complete session catalog contracts") on a Blacksmith Testbox:

  • check-additional-runtime-topology-architecture → pnpm check:madge-import-cycles reports the same single cycle through the src/plugins/** barrel ending at src/sessions/session-state-events.ts ("Madge import cycle check: 1 cycle(s)"), byte-identical to this PR's failure.
  • checks-node-compact-small-1 → test/scripts/plugin-sdk-surface-report.test.ts fails the same two budget-pin assertions on main (public callable exports 5298 > 5297 and the deprecated-count mismatch behind "expected { exports: 10595, …(2) } to deeply equal { exports: 10595, …(2) }").

This PR's diff is src/auto-reply/reply/get-reply-run.ts, its test, and the session-accessor debt baseline — no src/plugins/** imports and no plugin-SDK export surface, so it cannot move either check. Per repo policy the unrelated fix stays separate; the breakage appears to have landed with the session-catalog-contracts commit on main and will clear here on the next rebase/rerun once main is fixed.

Passing proof for this PR's own surface: pnpm check:changed exit 0, and the six shards that cover the change (incl. full auto-reply-reply: 141 files / 3,071 tests, and gateway-server) pass in isolated reruns on the same Testbox lease at head f6a6496.

@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. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jul 12, 2026
@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 11, 2026, 10:53 PM ET / July 12, 2026, 02:53 UTC.

Summary
The PR routes prepared-reply session refresh through exact-key working-set access plus the session accessor, removes an obsolete session-store mock, and shrinks the accessor-debt baseline.

PR surface: Source +6, Tests -11, Other -1. Total -6 across 3 files.

Reproducibility: not applicable. this is an architectural refactor rather than a reported failing behavior, and equivalence is established through source invariants plus runtime proof.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #104219
Summary: This PR is the bounded candidate implementation for seam 8 of the canonical core-hardening tracker.

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:

  • none.

Risk before merge

  • [P1] The branch is behind current main, so exact-head checks and the final merge result should be refreshed before landing; no code-level compatibility defect was found.

Maintainer options:

  1. Decide the mitigation before merge
    Run the normal exact-head merge gates and land this narrow accessor-boundary cleanup without expanding session APIs, compatibility paths, or product behavior.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No automated repair is needed; the protected-label PR should proceed through ordinary maintainer review and exact-head merge gating.

Security
Cleared: The focused TypeScript, test, and JSON-baseline changes add no dependency, secret, permission, download, build, or supply-chain concern.

Review details

Best possible solution:

Run the normal exact-head merge gates and land this narrow accessor-boundary cleanup without expanding session APIs, compatibility paths, or product behavior.

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

Not applicable; this is an architectural refactor rather than a reported failing behavior, and equivalence is established through source invariants plus runtime proof.

Is this the best way to solve the issue?

Yes; routing the residual caller through the existing accessor is the narrowest maintainable solution and preserves both the exact-key working set and legacy alias resolution.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5e064bb7b4b7.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR now includes after-fix gateway execution showing two completed reply turns, outbound delivery, one stable SQLite session identity, and advancing persisted state.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR now includes after-fix gateway execution showing two completed reply turns, outbound delivery, one stable SQLite session identity, and advancing persisted state.
  • 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:

  • P3: This is a low-risk internal boundary cleanup with no intended user-visible behavior change.
  • 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 (logs): The PR now includes after-fix gateway execution showing two completed reply turns, outbound delivery, one stable SQLite session identity, and advancing persisted state.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR now includes after-fix gateway execution showing two completed reply turns, outbound delivery, one stable SQLite session identity, and advancing persisted state.
Evidence reviewed

PR surface:

Source +6, Tests -11, Other -1. Total -6 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 11 5 +6
Tests 1 0 11 -11
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 0 1 -1
Total 3 11 17 -6

What I checked:

  • Focused unique patch: The merge-base diff contains only the intended runtime lookup replacement, obsolete mock removal, and one debt-baseline deletion; current main still retains the bypass. (src/auto-reply/reply/get-reply-run.ts:1100, f6a649614276)
  • Canonical working-set invariant: Reply-session initialization writes the committed entry at the resolver-normalized key, returns that cloned store view, and the mutable reply handle continuously writes the active exact session key. (src/config/sessions/session-accessor.ts:2281, 5e064bb7b4b7)
  • Accessor alias parity: The SQLite accessor gathers trimmed, normalized, folded-legacy, and structurally normalized candidate keys, then selects the entry with the same resolveSessionStoreEntry contract used by the deleted caller. (src/config/sessions/session-accessor.sqlite.ts:2428, 5e064bb7b4b7)
  • Real behavior proof: The updated PR body reports two after-fix reply turns through a real gateway and Telegram channel path, with both outbound messages captured, one stable SQLite session identity, advancing updatedAt, and completed run traces. (f6a649614276)
  • Accessor architecture provenance: The affected reply and accessor paths were established and repeatedly adopted by the merged SQLite storage and accessor-reroute work, including the storage flip and prior residual-caller cleanup. (src/config/sessions/session-accessor.ts:1084, 0a8e3604ba24)
  • Live PR state: The reviewed head is unchanged, non-draft, mergeable, and behind current main; it has no human reviews and retains the protected maintainer label. (f6a649614276)

Likely related people:

  • jalehman: Authored the merged SQLite session/transcript storage flip and multiple accessor-boundary reroutes that define the ownership model used by this PR. (role: accessor architecture contributor; confidence: high; commits: 0a8e3604ba24, 4e2a80cac34d; files: src/config/sessions/session-accessor.ts, src/config/sessions/session-accessor.sqlite.ts, src/auto-reply/reply/get-reply-run.ts)
  • steipete: Has numerous recent merged changes in get-reply-run.ts and the session accessor, making them a strong routing candidate for integration review. (role: recent reply and session area contributor; confidence: medium; commits: 49b5b862acf3, 04f945fabe47; files: src/auto-reply/reply/get-reply-run.ts, src/config/sessions/session-accessor.ts)
  • vincentkoc: Most recently repaired SQLite terminal freshness in the accessor path, directly adjacent to the latest-read behavior used by this PR. (role: recent session accessor contributor; confidence: medium; commits: 2f946506bb89; files: src/config/sessions/session-accessor.ts, src/config/sessions/session-accessor.sqlite.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 (1 earlier review cycle)
  • reviewed 2026-07-12T02:11:06.688Z sha f6a6496 :: needs real behavior proof before merge. :: none

@obviyus

obviyus commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Real behavior proof added (PR body Evidence updated).

Ephemeral gateway built from this branch (f6a6496), real Telegram channel path (mock Bot API + mock OpenAI provider on loopback, temp OPENCLAW_STATE_DIR), two consecutive reply turns in one chat:

  • Bot API mock captured both outbound replies: session-proof-reply-1, session-proof-reply-2.
  • SQLite session store after both turns: one row for agent:main:telegram:group:-100777001, same sessionId (ef4af902-…) in both turns' run traces, updatedAt advanced between turns (1783824157995 → 1783824162754).
  • Gateway debug log shows session turn created → embedded run start → message processed … outcome=completed on that sessionId for both turns — the refreshed prepared-session binding (the accessor-backed read this PR introduces) exercised end to end on the real reply path.

@clawsweeper re-review

@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 12, 2026
This was referenced Jul 12, 2026
Merged
Closed
@obviyus
obviyus merged commit 596603f into main Jul 12, 2026
176 of 191 checks passed
@obviyus
obviyus deleted the core/session-accessor-reroute branch July 12, 2026 04:36
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

maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. 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: XS 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

@obviyus