Skip to content

refactor: add session accessor seam#88840

Closed
jalehman wants to merge 14 commits into
mainfrom
clawdbot-9c3/session-accessor-seam
Closed

refactor: add session accessor seam#88840
jalehman wants to merge 14 commits into
mainfrom
clawdbot-9c3/session-accessor-seam

Conversation

@jalehman

@jalehman jalehman commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

What problem does this PR solve?

  • Adds the first additive slice for Track core session/transcript SQLite migration via accessor seam #88838: a file-backed session/transcript accessor seam that can later be implemented by SQLite without changing caller-facing domain shapes.
  • Keeps runtime storage behavior unchanged: the new seam delegates to the current file-backed session store and transcript JSONL helpers.
  • Adds a raw transcript event append helper that uses the existing transcript write lock and FIFO, so the seam does not introduce an unsafe write path.
  • Intentionally out of scope: SQLite schema/store modules, runtime storage flip, doctor import, broad caller migration, and plugin SDK behavior changes.
  • Success means reviewers can validate the seam shape and safety rules before later PRs use it as the SQLite migration boundary.
  • Review focus: seam shape, transcript locking/path resolution, normalized session-key handling, and whether the additive boundary is small enough for the first migration slice.

Why does this matter now?

The core session/transcript SQLite migration is broad because many callers currently depend on file-era shapes. This creates a stable internal boundary first so storage changes and caller modernization can land separately.

What is the intended outcome?

A behavior-neutral internal seam over current file storage that later SQLite store modules can satisfy.

What is intentionally out of scope?

SQLite schema/store modules, runtime storage flip, doctor migration, and subsystem caller modernization.

What does success look like?

The seam is additive, tested against file storage behavior, and safe to build subsequent migration PRs on.

What should reviewers focus on?

Transcript write locking, topic/session path resolution, normalized session keys, metadata-only insert behavior, and whether the seam avoids runtime dual-read/fallback behavior.

Linked context

Which issue does this close?

Does not close an issue.

Which issues, PRs, or discussions are related?

Refs #88838.
Related #79902.

Was this requested by a maintainer or owner?

Yes. This follows Josh's Path 3 SQLite decomposition plan for the core session/transcript migration.

Path 3 implementation map

This PR is the PR 3.1a base seam for the broader #88838 SQLite migration. The current open PRs in the stack are:

Section PR Status Notes
PR 3.1a: canonical accessor seam #88840 Open This PR; base for the 3.1b caller slices.
PR 3.1b: gateway entry/list/describe/get #89120 Open Routes gateway session entry reads through the seam.
PR 3.1b: transcript readers #89121 Open Routes storage-neutral transcript readers through the facade.
PR 3.1b: cron/infra/commands #89122 Open Routes command/status/health read paths through the seam; 3.2 writer race remains tracked outside this PR.
PR 3.1b: transcript writers #89123 Open Routes core transcript writer helpers through the seam.
PR 3.1b: auto-reply/agent runtime #89124 Open Routes agent/runtime session writes through the seam.
PR 3.1b: plugin SDK compatibility Not open yet In validation/backport Patch/preserveActivity and SQLite resolver findings are still being backported before PR creation.
PR 3.1b: bundled plugin consumers Not open yet In validation/backport Narrow bundled plugin callers are being moved off whole-store mutation; remaining whole-store compatibility is a design item.
PR 3.2: SQLite store foundation Not open yet In blocker resolution Additive SQLite foundation exists on a branch, with path parity and writer race fixes still being finalized.
Storage flip / doctor migration Not open yet Future phase Explicitly out of scope for this PR and the current 3.1b caller slices.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Adds an internal file-backed accessor seam for the core session/transcript SQLite migration plan.
  • Real environment tested: Local OpenClaw source checkout in a Codex worktree on macOS.
  • Exact steps or command run after this patch:
    • node scripts/run-vitest.mjs src/config/sessions/session-accessor.test.ts src/config/sessions/transcript.test.ts
    • pnpm exec oxfmt --check src/config/sessions/session-accessor.ts src/config/sessions/session-accessor.test.ts src/config/sessions/transcript-append.ts
    • node scripts/run-oxlint.mjs src/config/sessions/session-accessor.ts src/config/sessions/session-accessor.test.ts src/config/sessions/transcript-append.ts
    • pnpm tsgo:core
    • pnpm tsgo:core:test
    • /Users/phaedrus/Projects/prompts/skills/autoreview/scripts/autoreview --mode local
  • Evidence after fix: Focused Vitest passed 2 files / 32 tests; oxfmt, oxlint, core/test typechecks passed; autoreview reported no accepted/actionable findings.
  • Observed result after fix: Session accessor reads/writes file-backed entries, creates durable session ids for metadata-only inserts, preserves topic transcript paths, persists normalized session keys, creates transcript files with 0600 mode, and appends raw transcript events through the existing lock/FIFO.
  • What was not tested: Full runtime session migration, SQLite-backed implementation, doctor import, broad gateway/agent/SDK caller migration, and live end-to-end OpenClaw runs.
  • Proof limitations or environment constraints: This PR is intentionally additive and does not flip storage behavior.
  • Before evidence (optional but encouraged): Existing runtime used direct file-store/transcript helper paths without a dedicated storage-agnostic seam for the migration.

Tests and validation

Which commands did you run?

  • node scripts/run-vitest.mjs src/config/sessions/session-accessor.test.ts src/config/sessions/transcript.test.ts
  • pnpm exec oxfmt --check src/config/sessions/session-accessor.ts src/config/sessions/session-accessor.test.ts src/config/sessions/transcript-append.ts
  • node scripts/run-oxlint.mjs src/config/sessions/session-accessor.ts src/config/sessions/session-accessor.test.ts src/config/sessions/transcript-append.ts
  • pnpm tsgo:core
  • pnpm tsgo:core:test
  • /Users/phaedrus/Projects/prompts/skills/autoreview/scripts/autoreview --mode local

What regression coverage was added or updated?

Added src/config/sessions/session-accessor.test.ts covering file-backed load/list/upsert, metadata-only insert ids, transcript event load/append, secure transcript file creation, topic fallback paths, and normalized session-key persistence.

What failed before this fix, if known?

Autoreview caught several seam risks before this PR was opened: unsafe fallback session ids, insecure raw transcript file creation, missing topic fallback handling, cache mutation outside the writer, non-canonical session-key persistence, and raw appends bypassing the transcript lock/FIFO. Those were fixed before opening this PR.

If no test was added, why not?

Tests were added.

Risk checklist

Did user-visible behavior change? (Yes/No)

No.

Did config, environment, or migration behavior change? (Yes/No)

No.

Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)

No.

What is the highest-risk area?

The new raw transcript event append helper could become a reusable write path, so it must preserve existing transcript locking and file-permission behavior.

How is that risk mitigated?

The helper routes through the existing transcript append FIFO and session write lock, creates new files with 0600, and has focused regression coverage.

Current review state

What is the next action?

Maintainer review of the additive seam shape and safety constraints.

What is still waiting on author, maintainer, CI, or external proof?

CI and maintainer review. The broader migration remains tracked in #88838.

Which bot or reviewer comments were addressed?

No PR comments yet. Local autoreview findings were addressed before PR creation.

@openclaw-barnacle openclaw-barnacle Bot added size: M maintainer Maintainer-authored PR labels Jun 1, 2026
@jalehman jalehman mentioned this pull request Jun 1, 2026
Closed
@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 4, 2026, 4:47 PM ET / 20:47 UTC.

Summary
The branch adds a file-backed session/transcript accessor seam, a raw transcript append helper, a lifecycle cleanup helper, focused tests, and an SDK API baseline hash refresh.

PR surface: Source +819, Tests +552, Generated 0. Total +1371 across 7 files.

Reproducibility: not applicable. this is a refactor PR, not a bug report. The merge blockers are source-visible: the public SDK export at src/plugin-sdk/session-store-runtime.ts:25 and the contributor proof gate.

Review metrics: 1 noteworthy metric.

  • Public SDK surface: 1 helper and 2 types added. The published plugin-sdk/session-store-runtime subpath becomes a third-party plugin contract, so this API expansion needs maintainer approval before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Add redacted real OpenClaw runtime/session proof after the patch; terminal output or logs are enough if they show the changed path.
  • Remove or separately justify the public SDK lifecycle cleanup export with maintainer-approved docs and compatibility coverage.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists focused Vitest, lint, typecheck, and local autoreview output, but no redacted terminal output, logs, recording, or linked artifact from a real OpenClaw runtime/session workflow after the patch. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The new public SDK export would lock in lifecycle deletion/archive semantics before maintainers decide whether that belongs in plugin API.
  • [P1] The seam is the session/transcript storage migration boundary; wrong transcript path or cleanup semantics can carry into dependent 3.1b/3.2 PRs.
  • [P1] External contributor proof is still tests, lint, typecheck, and autoreview only; no redacted real OpenClaw runtime/session workflow is shown.

Maintainer options:

  1. Keep cleanup internal before merge (recommended)
    Remove cleanupSessionLifecycleArtifacts and its types from openclaw/plugin-sdk/session-store-runtime, or split them into a separate approved SDK change with docs, baseline, and compatibility tests.
  2. Accept the SDK expansion explicitly
    Maintainers can choose to expose lifecycle deletion/archive APIs now, but the PR should document that contract and why plugin authors should own this operation before merge.
  3. Pause the migration base
    If this base seam is no longer the chosen Path 3 boundary, pause this PR and keep Track core session/transcript SQLite migration via accessor seam #88838 as the canonical planning item.

Next step before merge

  • [P1] Needs maintainer/API-boundary decision and contributor real behavior proof; this is not a safe automated repair lane while public SDK lifecycle semantics are unresolved.

Security
Cleared: No concrete secret, dependency, CI, package-resolution, or third-party code-execution regression was found; the remaining risk is SDK compatibility and session-state ownership.

Review findings

  • [P1] Keep lifecycle cleanup out of the public SDK — src/plugin-sdk/session-store-runtime.ts:25
Review details

Best possible solution:

Keep the file-backed accessor seam internal and behavior-neutral, remove or defer the SDK lifecycle cleanup export unless maintainers approve it separately, then require redacted runtime/session proof before merge.

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

Not applicable; this is a refactor PR, not a bug report. The merge blockers are source-visible: the public SDK export at src/plugin-sdk/session-store-runtime.ts:25 and the contributor proof gate.

Is this the best way to solve the issue?

No; the internal accessor seam is a plausible first slice, but adding lifecycle cleanup to the public SDK in this slice is not the best boundary. Keep that helper internal or split it into a maintainer-approved SDK API PR.

Full review comments:

  • [P1] Keep lifecycle cleanup out of the public SDK — src/plugin-sdk/session-store-runtime.ts:25
    openclaw/plugin-sdk/session-store-runtime is a published plugin subpath, and this PR says plugin SDK behavior is out of scope. Exporting cleanupSessionLifecycleArtifacts here promotes a row-deletion/transcript-archive operation into third-party API before maintainers approve that lifecycle contract. Please keep it internal to the session seam, or split the SDK API into a separate approved change with docs, baseline, and compatibility coverage.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 3d168074b473.

Label changes

Label justifications:

  • P2: This is a normal-priority session/transcript migration refactor with important but bounded merge risk.
  • merge-risk: 🚨 compatibility: The PR expands a published plugin SDK subpath with lifecycle cleanup API that third-party plugins could start depending on.
  • merge-risk: 🚨 session-state: The new seam and cleanup helper operate on session entries and transcript artifacts, so incorrect contracts can affect stored session history during the SQLite migration.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists focused Vitest, lint, typecheck, and local autoreview output, but no redacted terminal output, logs, recording, or linked artifact from a real OpenClaw runtime/session workflow after the patch. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +819, Tests +552, Generated 0. Total +1371 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 5 821 2 +819
Tests 1 552 0 +552
Docs 0 0 0 0
Config 0 0 0 0
Generated 1 2 2 0
Other 0 0 0 0
Total 7 1375 4 +1371

What I checked:

  • Repository review policy: Root policy treats plugin APIs, auth/session state, persisted preferences, migrations, setup, startup checks, and fallback behavior as compatibility-sensitive merge risk, and requires whole-decision-surface review rather than diff-only review. (AGENTS.md:30, cc296f3a4600)
  • Scoped SDK policy: The scoped SDK guide says src/plugin-sdk is the public contract between plugins and core, that changes can affect bundled and third-party plugins, and that implementation convenience should not be exposed unless intentionally promoted as a supported public contract. (src/plugin-sdk/AGENTS.md:1, cc296f3a4600)
  • Public SDK export added: The PR head exports cleanupSessionLifecycleArtifacts and its two public types from openclaw/plugin-sdk/session-store-runtime, a published SDK subpath. (src/plugin-sdk/session-store-runtime.ts:25, f2ad875a5c9b)
  • Cleanup helper mutates session state: The exported helper accepts a store path, session-key prefix, transcript marker, and age threshold, then removes matching session entries and archives transcript artifacts. (src/config/sessions/store.ts:196, f2ad875a5c9b)
  • Cleanup behavior is covered but still internal-contract heavy: The new accessor test covers scoped lifecycle deletion and transcript archiving behavior, which is useful implementation proof but does not answer whether this should become public SDK API. (src/config/sessions/session-accessor.test.ts:329, f2ad875a5c9b)
  • Current main does not already contain the seam: A current-main search for session-accessor, SessionAccessScope, cleanupSessionLifecycleArtifacts, and appendSessionTranscriptEvent found no matching implementation, so the PR is not obsolete on main. (3d168074b473)

Likely related people:

  • steipete: Peter Steinberger is the current blame owner for the main session-store-runtime SDK facade and dominates recent session-store/session-SDK history sampled for this review. (role: recent area contributor; confidence: high; commits: 57ec0b236f6e, a71b810e43f8; files: src/plugin-sdk/session-store-runtime.ts, src/config/sessions/store.ts)
  • jalehman: Josh Lehman owns this Path 3 stack in the PR context and also appears in prior merged session-store cache work, so routing the migration-boundary decision to him is reasonable beyond this PR authorship. (role: feature and adjacent session-store contributor; confidence: medium; commits: 175c77017115, 1212328c8da1; files: src/config/sessions/store.ts)
  • shakkernerd: Shakker recently split the session store loader from maintenance and isolated session store test cleanup state in the same central session-store area. (role: adjacent session-store refactor contributor; confidence: medium; commits: c2e93c76bd43, 73073a91bb15; files: src/config/sessions/store.ts)
  • vincentkoc: Vincent Koc authored the latest release plugin SDK baseline refresh, which is relevant because this PR changes SDK API baseline hashes. (role: plugin SDK baseline adjacent contributor; confidence: medium; commits: 2e08f0f4221f; files: docs/.generated/plugin-sdk-api-baseline.sha256)
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.

@clawsweeper clawsweeper Bot added 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. P2 Normal backlog priority with limited blast radius. 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. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. and removed 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. labels Jun 1, 2026
@steipete steipete self-assigned this Jun 1, 2026
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. 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 Jun 1, 2026
@jalehman jalehman mentioned this pull request Jun 1, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jun 1, 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. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 1, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 1, 2026
Merged
@jalehman
jalehman marked this pull request as draft June 1, 2026 18:06
@jalehman jalehman mentioned this pull request Jun 1, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jun 2, 2026
Closed
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation size: XL and removed size: L labels Jun 3, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 3, 2026
@jalehman
jalehman marked this pull request as ready for review June 4, 2026 20:31
@jalehman
jalehman force-pushed the clawdbot-9c3/session-accessor-seam branch from f18f99c to b39023f Compare June 4, 2026 20:31
@clawsweeper clawsweeper Bot mentioned this pull request Jun 4, 2026
Merged
@jalehman jalehman mentioned this pull request Jun 4, 2026
Merged
@jalehman

jalehman commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #90463, which bundles this accessor seam with its first gateway production consumer. That keeps the branch-by-abstraction landing unit reviewable while avoiding a temporary dead-code allowlist entry for the intentionally-new seam.

@jalehman jalehman closed this Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

docs Improvements or additions to documentation maintainer Maintainer-authored PR 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. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XL status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jalehman @steipete