fix: persist ACP metadata in SQLite#88724
Conversation
|
Codex review: needs maintainer review before merge. Reviewed May 31, 2026, 1:38 PM ET / 17:38 UTC. Summary PR surface: Source +939, Tests +538, Docs +3, Generated +39. Total +1519 across 36 files. Reproducibility: yes. at source level: current main stores ACP metadata on Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this only after owner review confirms the SQLite schema, legacy doctor migration, and restart/update behavior preserve existing ACP sessions across canonical keys, session IDs, and channel delivery paths. Do we have a high-confidence way to reproduce the issue? Yes at source level: current main stores ACP metadata on Is this the best way to solve the issue? Yes, likely: the shared SQLite store plus doctor migration is the maintainable direction under the repository's database-first policy. The remaining question is upgrade proof, not a better implementation layer. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against a3fa5b6577a2. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +939, Tests +538, Docs +3, Generated +39. Total +1519 across 36 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 077d27f212
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const sessionStore = loadSessionStore(storePath); | ||
| for (const [sessionKey, entry] of Object.entries(sessionStore)) { | ||
| if (!sessionEntryMatchesAcpResumeSessionId(entry, resumeSessionId)) { | ||
| const acp = readAcpSessionMeta({ sessionKey }); |
There was a problem hiding this comment.
Pass the active config when reading ACP resume metadata
When spawnAcpDirect is invoked with a config whose session.store differs from the process-global runtime config, this lookup reads ACP metadata without params.cfg even though the target session store was loaded from params.cfg just above. readAcpSessionMeta then resolves a different store path before accepting rows with a session_id, so a valid resumeSessionId recorded in the configured target store is treated as missing and the spawn is rejected as resume_forbidden. Pass the same cfg/env context used to load sessionStore into the metadata read.
Useful? React with 👍 / 👎.
* fix: persist acp metadata in sqlite * test: align session store acp expectations
* fix: persist acp metadata in sqlite * test: align session store acp expectations
* fix: persist acp metadata in sqlite * test: align session store acp expectations
Summary
sessions.jsonACP metadata and file-backed ACP event ledgers into SQLite, then keep session-store rows discoverable without embeddedacpblocks.Verification
node scripts/run-vitest.mjs src/acp/runtime/session-meta.test.ts src/acp/control-plane/manager.test.ts src/gateway/session-utils.subagent.test.ts src/gateway/server.sessions.reset-cleanup.test.ts src/gateway/server.sessions.delete-lifecycle.test.ts src/commands/sessions.acp-model-display.test.ts src/commands/status.test.ts src/commands/sessions-tail.test.ts extensions/telegram/src/thread-bindings.test.ts src/acp/event-ledger.test.ts src/commands/doctor-state-migrations.test.ts src/config/sessions/sessions.test.ts src/auto-reply/reply/commands-acp.test.ts src/auto-reply/reply/dispatch-from-config.test.ts src/auto-reply/reply/dispatch-from-config.acp-abort.test.ts src/gateway/server.sessions-send.test.ts src/agents/tools/sessions-send-tool.a2a.test.ts src/agents/acp-spawn.test.ts src/auto-reply/reply/routing-policy.test.ts src/auto-reply/reply/dispatch-acp-delivery.test.tspassed, 9 shards / 636 tests.node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core.tsbuildinfopassed.node scripts/run-oxlint.mjs ...on touched ACP/session/gateway/dispatch surfaces passed.pnpm db:kysely:checkpassed.git diff --checkpassed.pnpm check:changeddelegated to Blacksmith Testboxtbx_01kszgt2yn9ydp0z67tzv7eew8, exit 0. GitHub Actions run: https://github.com/openclaw/openclaw/actions/runs/26719270163Real behavior proof
Behavior addressed: ACP metadata and ACP replay/event ledgers now survive restart/update paths through SQLite state instead of session-store JSON or file sidecars.
Real environment tested: Blacksmith Testbox via Crabbox, plus focused local Vitest and type/lint/schema checks in this checkout.
Exact steps or command run after this patch:
pnpm check:changedthrough the repository wrapper, which delegated to Blacksmith Testbox and ran the selected core, coreTests, extensions, extensionTests, and docs lanes.Evidence after fix: Testbox lease
tbx_01kszgt2yn9ydp0z67tzv7eew8completed with exit 0; local focused Vitest suite passed 9 shards / 636 tests.Observed result after fix: ACP metadata reads join SQLite rows to the current session entry and session id, reset/delete cleanup finds canonical metadata for raw legacy keys, ACP session listings work for explicit stores, and legacy JSON/file ACP state migrates into SQLite.
What was not tested: A manual Discord ACP end-to-end run was not performed in this PR; coverage is from unit/integration tests plus the remote changed lane.