Repository navigation
refactor(talk): centralize typed voice session queries - #139103
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 5, 2026, 9:16 AM ET / 13:16 UTC. ClawSweeper reviewWhat this changesCentralizes Talk voice-session database queries in the existing persistence module, uses typed query builders, and adds rollback and retry coverage. Merge readiness⛔ Blocked before merge - 2 items remain Current main still contains the duplicated queries, so this maintainer-owned refactor remains useful. No introduced correctness defect was found. Priority: P3 Review scores
Verification
How this fits togetherTalk stores per-agent voice-call records used by client requests, transcript persistence, and abandoned-call recovery. The persistence module translates those operations into SQLite queries while the lifecycle module enforces session ownership and close behavior. flowchart TD
A[Talk client requests] --> B[Voice session lifecycle]
C[Abandoned call recovery] --> B
B --> D[Ownership and state checks]
D --> E[Talk persistence module]
E --> F[Per-agent SQLite database]
F --> E
E --> G[Transcript and close outcomes]
Before merge
Agent review detailsSecurityNone. PR surfaceSource +21, Tests +58, Other 0. Total +79 across 4 files. View PR surface stats
Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep one Talk persistence owner using the shared typed builder while preserving native execution, transaction custody, record format, and eager recovery snapshots. Do we have a high-confidence way to reproduce the issue? Not applicable: this is a behavior-preserving refactor, and source inspection confirms the duplicated queries on current main. Is this the best way to solve the issue? Yes at the owner boundary: reusing the existing store and compile/bind helper removes duplicated SQL without switching to an executor that changes native error or iteration behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d37ed30158b5. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
The two remaining review items and the rank-up move concern access to the same pinned dependency contract. I inspected the installed Kysely 0.29.5 implementation directly: insert-values-parser preserves column/value correspondence; update-set-parser retains the supplied update entries; default-query-compiler visits bound values in SQL order and pushes their original values into the parameter list; the SQLite compiler emits The native statement is still prepared before those readers evaluate the record. Talk keeps native get/all/run and the caller-supplied transaction handle; the UPSERT updates only value_json and updated_at. Direct baseline/candidate native proof covered preparation and binding order, blob/expiry preservation, rejected write rollback/retry, agent isolation, ambiguous lookup and stale-snapshot boundaries. The real built Gateway transcript, duplicate/close, restart and post-close refusal checks passed, as did the focused tests and complete changed gate. Four independent review reports are clean, and hosted CI is green on the unchanged head. This resolves the reviewer-side dependency access gap without a production change or policy exception. No Codex protocol/runtime dependency is involved in this query refactor. |
|
Merged via squash.
|
* commit '3e4becea5e2ce38ede69e8ecf5302805ecbfc09c': refactor: simplify outbound message execution context (openclaw#139107) fix(models): keep ordinary chat selections session-scoped (openclaw#139232) docs: slim root agent instructions and clarify testing scope (openclaw#139236) test(agents): give the sibling-drain completion wait a tolerance on slow runners (openclaw#139176) refactor(workboard): use canonical queries for card-tree writes (openclaw#139106) fix(canvas): recognize versioned macOS nodes (openclaw#138958) fix: load GitHub connections without a default agent (openclaw#139222) fix(memory): explain unsafe legacy host-event path recovery (openclaw#139195) refactor(talk): centralize typed voice session queries (openclaw#139103) fix(cli): preserve command dispatch and structured errors (openclaw#139067) refactor(agents): consolidate reply failure summaries (openclaw#139175) fix(auth): keep an unset default model unchanged during login (openclaw#139218) perf(media): reuse validated attachment read handles (openclaw#139168) refactor(channels): simplify account configuration updates (openclaw#139096) refactor(android): remove unused updater export root (openclaw#139198) fix(gateway): stop admitting post-ready producers during close (openclaw#139092) fix(gateway): expose agent hook completion outcomes (openclaw#139155) fix(config): return the saved configuration after plugin install commits (openclaw#139203) fix(android): reduce repeated retries to unreachable gateways (openclaw#138732)
What Problem This Solves
Talk voice sessions constructed the same cache queries in both the lifecycle module and the persistence module. Point reads were duplicated, and recovery code owned its own SQL and row assertions.
Why This Change Was Made
Keep all five ordinary queries in the existing Talk store, using the generated agent-database types and shared Kysely compile/bind helpers. The lifecycle code now asks the store for its records. Native
get, eagerall, andrunexecution retain the existing database/error owner and supplied transaction handle.The UPSERT still updates only JSON and update time, preserving blob/expiry columns. Native preparation still precedes payload getters and JSON serialization. Recovery takes its full candidate snapshot before asynchronous work; the legacy lookup still requires a unique open session. Production: +58/-37 (net +21); tests: +58; four row assertions removed. No schema, config, public API, retention, or PostgreSQL change.
Evidence
Two early Gateway harness failures involved native row prototypes and JSON-mode error output; both are retained with their cleanup receipts. Only those task-local comparisons were corrected. The single runtime build was reused after comment-only guard annotations were proved to emit identical JavaScript; all 5,597 recorded build hashes remained unchanged. No external voice provider, audio transport, or performance claim.