Skip to content

fix(sqlite): support Node 23.0–23.10 runtimes lacking StatementSync.columns()#90035

Closed
sjf wants to merge 1 commit into
mainfrom
fix/sqlite-columns-node-23-gap
Closed

fix(sqlite): support Node 23.0–23.10 runtimes lacking StatementSync.columns()#90035
sjf wants to merge 1 commit into
mainfrom
fix/sqlite-columns-node-23-gap

Conversation

@sjf

@sjf sjf commented Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Fixes #90007. After upgrading to 2026.6.1, the post-update openclaw doctor step crashed on Node 23.0–23.10 with:

TypeError: statement.columns is not a function

across the plugin install index, task registry sidecar, and task flow sidecar migrations.

Root cause

StatementSync.prototype.columns() (node:sqlite) was Added in: v23.11.0, v22.16.0 (per the Node docs). Because Node 22 is LTS and got the backport, the timeline is non-linear: .columns() exists on 22.16+ (so all of our engines: >=22.19.0 floor), on 23.11+, and on 24+ — but not on 23.0 through 23.10. The reporter's Node 23.9.0 lands squarely in that hole, yet satisfies the declared engine, so it is a supported runtime.

Both SQLite execute paths called statement.columns() unconditionally to decide between .all() (result-returning) and .run():

  • src/infra/kysely-sync.ts — the sync helper used by every state migration
  • src/infra/kysely-node-sqlite.ts — the Kysely driver executeQuery

Fix

Two changes, both centered in src/infra/kysely-node-sqlite.ts:

1. Make the .all()-vs-.run() decision version-robust (statementReturnsRows). Feature-detect .columns(); on runtimes that have it (Node 22.16+/23.11+/24+) behavior is unchanged. When it's absent (Node 23.0–23.10), classify result-returning statements from the typed Kysely operation node — no SQL string parsing:

  • SelectQueryNode → rows
  • Insert/Update/DeleteQueryNode → rows iff a RETURNING clause is present (node.returning != null)
  • anything else → no rows (the dialect only ever raw-executes transaction-control statements — begin/commit/rollback/savepoint)

This is exact for every query OpenClaw runs through this dialect: all callers use Kysely builders, and the dialect itself only raw-executes transaction control.

2. Coalesced the two duplicated execute paths to remove the duplication. statementReturnsRows has to be shared by both call sites, and the surrounding bodies were already near-identical copies (prepare → row check → all()/run() → insert metadata). Rather than duplicate the new logic across both, the async driver's executeQuery and the sync helper's executeCompiledSqliteQuerySync are now unified into a single executeCompiledQuerySync, and both delegate to it. This also reconciles a pre-existing inconsistency where the two paths detected inserts differently (raw SQL startsWith("insert") vs InsertQueryNode.is) — they now share InsertQueryNode.is. Net result: kysely-sync.ts shrinks (its bespoke body is deleted), and the row-detection logic lives in exactly one place.

Verification

Reproduced and verified against a standalone Node v23.9.0 (the reporter's runtime) and local Node v26:

Node 23.9.0 (columns absent) Node 26 (columns present)
Before fix TypeError: statement.columns is not a function ✅ works
After fix ✅ all queries pass ✅ all queries pass

Exercised through both the sync helper (executeSqliteQuerySync, the path the migrations use) and the async Kysely driver: insert + insertId, select, INSERT … RETURNING, update, delete, takeFirst, transaction rollback, savepoints, and streaming — identical results on both runtimes.

New regression tests simulate a node:sqlite build without StatementSync.columns() (a Proxy that hides the method), so CI on Node 24 still exercises the fallback classifier through both the sync and driver entry points.

Local commands (normal source checkout):

pnpm tsgo:core && pnpm tsgo:core:test
node scripts/run-oxlint.mjs src/infra/kysely-node-sqlite.ts src/infra/kysely-sync.ts src/infra/kysely-node-sqlite.test.ts
node scripts/check-kysely-guardrails.mjs
pnpm test src/infra/kysely-node-sqlite.test.ts src/infra/kysely-sync.types.test.ts \
          src/plugins/installed-plugin-index-store.test.ts src/commands/doctor-state-migrations.test.ts

All green (kysely infra 7, types 1, doctor-state-migrations 51, installed-plugin-index 14).

Thanks to @TreyLawrence for the precise report and reproduction.

@openclaw-barnacle openclaw-barnacle Bot added size: M maintainer Maintainer-authored PR labels Jun 3, 2026
@sjf

sjf commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

clawsweeper Bot commented Jun 3, 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:

@clawsweeper

clawsweeper Bot commented Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 8, 2026, 6:00 PM ET / 22:00 UTC.

Summary
The PR centralizes node:sqlite Kysely execution, feature-detects StatementSync.columns(), and adds hidden-columns regression tests for builder and sync-helper paths.

PR surface: Source -2, Tests +82. Total +80 across 3 files.

Reproducibility: yes. from source: current main unconditionally calls statement.columns(), and PR head can be inspected to show raw RawNode row queries fall through to run() when that method is hidden. I did not run a live Node 23.9 repro in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until stronger real behavior proof 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 hidden-columns() coverage for raw SELECT, PRAGMA reads, and raw INSERT ... RETURNING or document a maintainer-approved narrowed contract.
  • [P2] Post redacted terminal output or logs from the current head on Node 23.9 after the fallback repair.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body self-reports Node 23.9 and Node 26 verification, but it does not include redacted terminal output, logs, copied live output, or a linked artifact from the current head. 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] On Node 23.0-23.10, raw Kysely SELECT, PRAGMA, or raw INSERT ... RETURNING through this dialect would be treated as non-row-returning and call run(), even though existing adjacent tests treat those raw row queries as supported.
  • [P1] The PR's Node 23.9 verification is self-reported without redacted terminal output, logs, or a linked artifact from the current head, so maintainers cannot independently check the claimed real-runtime pass.

Maintainer options:

  1. Fix raw row fallback before merge (recommended)
    Extend the missing-columns() fallback and tests so raw SELECT, PRAGMA reads, and raw RETURNING queries still return rows on Node 23.0-23.10.
  2. Accept a narrower raw-query contract
    Maintainers can explicitly decide that raw row-returning SQL is unsupported on the Node 23 fallback path, but the PR body and tests should state that compatibility limit before merge.
  3. Pause for current-head proof
    Hold the PR until the author or a maintainer posts redacted Node 23.9 output from the current head after the fallback decision is resolved.

Next step before merge

  • [P1] Human follow-up is needed because the PR has a narrow compatibility defect and the contributor still needs to provide real Node 23 proof from their setup; automation cannot supply that contributor proof.

Security
Cleared: The diff is limited to TypeScript SQLite execution logic and colocated tests; it does not add dependencies, workflows, package metadata, secret handling, or downloaded code.

Review findings

  • [P2] Preserve raw row queries when columns() is missing — src/infra/kysely-node-sqlite.ts:213
Review details

Best possible solution:

Keep the shared executor, preserve raw row-returning SQL behavior or explicitly narrow that contract, add hidden-columns coverage for that path, then post redacted Node 23.9 proof before merge.

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

Yes from source: current main unconditionally calls statement.columns(), and PR head can be inspected to show raw RawNode row queries fall through to run() when that method is hidden. I did not run a live Node 23.9 repro in this read-only review.

Is this the best way to solve the issue?

No as-is. The shared SQLite/Kysely executor is the right layer, but the best fix must preserve raw row-returning behavior or make a maintainer-approved contract change before merge.

Full review comments:

  • [P2] Preserve raw row queries when columns() is missing — src/infra/kysely-node-sqlite.ts:213
    When statement.columns is unavailable, Kysely raw SQL compiles as RawNode, so this fallback returns false and calls statement.run(). Existing adjacent tests treat raw SELECT and raw INSERT ... RETURNING as row-returning behavior, so please keep that behavior covered in the hidden-columns() path before merge.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 6c4fb997e52e.

Label changes

Label justifications:

  • P1: The linked bug is a latest-release post-update doctor crash in a supported Node runtime that can break upgrade and state migration workflows.
  • merge-risk: 🚨 compatibility: The PR changes the core SQLite compatibility path for supported Node 23 runtimes and currently narrows raw row-returning query behavior when columns() is absent.
  • 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 stronger real behavior proof before merge: The PR body self-reports Node 23.9 and Node 26 verification, but it does not include redacted terminal output, logs, copied live output, or a linked artifact from the current head. 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 -2, Tests +82. Total +80 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 51 53 -2
Tests 1 82 0 +82
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 133 53 +80

What I checked:

  • Repository policy read: The full root AGENTS.md was read; its dependency-contract, SQLite state, compatibility, and real-behavior-proof review rules apply to this PR. No scoped AGENTS.md owns src/infra, and the only maintainer note found was Telegram-specific. (AGENTS.md:1, 6c4fb997e52e)
  • Current main async failure path: Current main prepares SQL and unconditionally calls stmt.columns() before choosing all() versus run() in the async Kysely driver. (src/infra/kysely-node-sqlite.ts:157, 6c4fb997e52e)
  • Current main sync failure path: Current main also calls statement.columns() unconditionally in the sync helper used by state and migration paths. (src/infra/kysely-sync.ts:35, 6c4fb997e52e)
  • PR fallback gap: At PR head, statementReturnsRows returns false for operation nodes outside Select/Insert/Update/Delete, so Kysely raw SQL row queries fall through to statement.run() when columns() is missing. (src/infra/kysely-node-sqlite.ts:213, e4b9ba94c7a9)
  • Existing raw row behavior: The adjacent dialect tests already assert raw sql SELECT and raw INSERT ... RETURNING return rows, but the new hidden-columns tests cover only builder select/returning plus sync builder select. (src/infra/kysely-node-sqlite.test.ts:23, e4b9ba94c7a9)
  • Node dependency contract: Official Node docs for v23.10 list statement.sourceSQL but not statement.columns(), while v23.11 adds statement.columns() and v22.19 includes it from v22.16; this matches the Node 23.0-23.10 compatibility gap.

Likely related people:

  • Shakker: Current-main blame on the SQLite Kysely helper and adjacent tests points to commit da40134 touching these files shortly before this review. (role: recent area contributor; confidence: medium; commits: da401341b685; files: src/infra/kysely-node-sqlite.ts, src/infra/kysely-sync.ts, src/infra/kysely-node-sqlite.test.ts)
  • Vincent Koc: The v2026.6.1 release-baseline commit includes the SQLite helper and test files implicated by the Node 23 columns() failure. (role: release-baseline contributor; confidence: medium; commits: 2e08f0f4221f; files: src/infra/kysely-node-sqlite.ts, src/infra/kysely-sync.ts, src/infra/kysely-node-sqlite.test.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.

@sjf
sjf force-pushed the fix/sqlite-columns-node-23-gap branch from deeef96 to a986080 Compare June 3, 2026 22:27
@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. P1 High-priority user-facing bug, regression, or broken workflow. labels Jun 3, 2026
@sjf
sjf force-pushed the fix/sqlite-columns-node-23-gap branch 2 times, most recently from 364f16e to 3e3c59b Compare June 3, 2026 23:14
@sjf
sjf requested a review from steipete June 3, 2026 23:19
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed 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. labels Jun 3, 2026
@sjf
sjf force-pushed the fix/sqlite-columns-node-23-gap branch 2 times, most recently from bc6b809 to 6ec319a Compare June 3, 2026 23:26
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Jun 3, 2026
@sjf

sjf commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

clawsweeper Bot commented Jun 3, 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:

@clawsweeper clawsweeper Bot mentioned this pull request Jun 4, 2026
Closed
@sjf @claude
fix(sqlite): support Node 23.0–23.10 runtimes lacking StatementSync.c…
e4b9ba9 
…olumns()

node:sqlite added StatementSync.columns() in v22.16/v23.11, but it is absent on
Node 23.0–23.10 — runtimes that still satisfy engines (>=22.19.0). The Kysely
sync and driver execute paths called it unconditionally, so a post-upgrade
`openclaw doctor` crashed the plugin install index and task registry/flow
sidecar migrations with "statement.columns is not a function".

Centralize a single executeCompiledQuerySync that feature-detects columns() and,
when absent, classifies result-returning statements from the compiled query
shape. Collapses the previously duplicated sync/driver execute logic into one
path.

Fixes #90007

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@sjf
sjf force-pushed the fix/sqlite-columns-node-23-gap branch from 6ec319a to e4b9ba9 Compare June 8, 2026 21:49
@sjf

sjf commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

clawsweeper Bot commented Jun 8, 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:

@sjf sjf closed this Jun 9, 2026
This was referenced Jul 4, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steipete steipete Awaiting requested review from steipete

Assignees

No one assigned

Labels

maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

1 participant

@sjf