Skip to content

fix(driver-turso): escape the aggregation alias instead of gating it, so remote-mode analytics cube queries stop 500ing - #14248

Merged
os-support-ai merged 2 commits into
mainfrom
claude/issue-14113-turso-remote-alias-quoting
Sep 2, 2026
Merged

os-support-ai merged 2 commits into
mainfrom
claude/issue-14113-turso-remote-alias-quoting

Conversation

@claude

@claude claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes #14113

RemoteTransport.aggregate (Turso remote mode) held the aggregation alias to SAFE_IDENTIFIER (/^[a-zA-Z_][a-zA-Z0-9_]*$/). A dot fails that regex. Every analytics measure is named CUBE.MEASURE on the wire and ObjectQLStrategy uses that name verbatim as the aggregation alias, so every cube query reaching this face threw:

RemoteTransport: unsafe identifier rejected: "showcase_delivery.count"

a bare Error with no code and no status, which mapDataError then served as an opaque 500 — for a query that is spelled correctly.

The repair: escape the alias, do not drop the check

The alias is now escaped rather than gated — it may be any string, and the quote character is doubled (" becomes ""), the standard escape inside a quoted SQL identifier. New private helper RemoteTransport.aliasIdentifierSql, placed beside assertSafeIdentifier so the reference-versus-name pair reads as one thing.

⛔ Not "drop the check". The alias reaches the statement raw inside AS "...", so an alias containing a " would close the quoting and continue as grammar. This is the ALIAS half of the distinction #13714 drew one face over, where the same position routes through knex's wrapIdentifier (SqlDriver.aliasIdentifierSql): a qualified reference must be validated, a single output name must be quoted and escaped. AggregationNodeSchema declares alias: z.string() — an output-column key — and the in-memory, MongoDB and post-#13714 SQL faces all project it verbatim. This face was the outlier.

The field and object positions keep assertSafeIdentifier unchanged: those become column and table references, which are grammar.

Verification

Tests execute against a real SQLite-backed libsql stub (makeLibsqlSqliteStub) rather than asserting on a SQL string. That is deliberate and load-bearing: only executing the statement tells "escaped" apart from "broke out" — a string assertion passes on an alias that terminates its own quoting, because the text still looks like a select list.

New suite remote-transport-aggregation-alias-quoting.test.ts, 9 cases:

  • direction 1 — the dotted alias is served: showcase_delivery.count returns the value under the caller's own key; emitted SQL pinned as SELECT count(*) AS "showcase_delivery.count" FROM "showcase_delivery" with an explicit assertion that it is not split into two segments; plus the un-bucketed grouped-cube shape end to end.
  • direction 2 — a " is escaped: won"count compiles to AS "won""count" and runs; bucket"; DROP TABLE showcase_delivery; -- compiles to one inert column name, is returned as a column name, and the table it named still holds all 3 rows.
  • regression controls (the positions this card did not touch): the field position still refuses and sends nothing; the object position still refuses; the groupBy alias position is unchanged; the default alias count_all is byte-identical.
pnpm --filter @objectstack/driver-turso exec vitest run --maxWorkers=2 \
  src/remote-transport-aggregation-alias-quoting.test.ts
  Test Files  1 passed (1)       Tests  9 passed (9)

pnpm --filter @objectstack/driver-turso test
  Test Files  41 passed (41)     Tests  1120 passed (1120)

pnpm --filter @objectstack/driver-turso typecheck        exit 0

typecheck was confirmed to actually read the new test file (tsc --noEmit --listFiles count = 1, not 0), so "typecheck clean" is a measurement over these edits rather than a green over source nothing read.

Reverse verification — direction predicted before it was run

Restoring the pre-fix two lines (this.assertSafeIdentifier(alias); + AS "${alias}"). Predicted: the 3 dotted-alias cases and the 2 quote-escape cases go red by throwing inside the call, not on a comparison; the 3 position controls and the default-alias case stay green.

Measured — 5 failed / 4 passed of 9, case for case as predicted:

× the exact alias the card measured is served, on rows
× compiles to ONE quoted identifier, dot and all
× the un-bucketed cube shape — a grouped measure — is served end to end
× doubles the quote and still runs, returning the value under the literal alias
× an alias that tries to close the quoting and append a statement stays one name

Error: RemoteTransport: unsafe identifier rejected: "showcase_delivery.count"
Error: RemoteTransport: unsafe identifier rejected: "won"count"
Error: RemoteTransport: unsafe identifier rejected: "bucket"; DROP TABLE showcase_delivery; --"

The first of those is the card's reported symptom reproduced verbatim. The mutation was confirmed on disk before the run (anchor counts: gate call 0 to 1, new emission 1 to 0, old emission 0 to 1) and the restore was proven by observation afterwards — git diff HEAD empty and git hash-object byte-identical to the HEAD blob, not by the restore command's exit code. No rebuild leg was needed and that is measured, not assumed: packages/drivers/driver-turso/dist does not exist, so the subject under test resolves from source.

Gates

34 families derived from the real change set by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (not from a hand-written diff list) and run at commit 689d223f. Exit codes captured after redirecting to a file, never through a pipe.

31 of 34 exited 0. The other 3 are NOT MEASURED, not red — each refuses on its own prerequisite because it reads built output the whole workspace has to produce, and each says so in its own verdict line rather than being inferred from the exit code:

gate exit its own verdict line
check-test-completeness.mjs 3 "the local reading for this gate is NOT MEASURED. It is not a red, and there is nothing here to fix" (it wants a saved turbo run test log)
check:dual-build-cjs-loads 3 "PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/ ... This is NOT a pass: nothing was measured"
check:type-check-debt 3 "PREREQUISITE NOT MET" (its --re-measure needs the built workspace closure)

All three are unreachable locally without a full pnpm build of every package, which CI performs; none is affected by the semantics of this diff. Recorded as NOT MEASURED rather than folded into the green count.

Also run beyond the derived set: node scripts/check-nul-bytes.mjs (exit 0, 7781 files) plus a direct control-byte scan of the three changed files (clean).

Out of scope, filed rather than patched

#14235 — the groupBy alias is the second output-name position in this same method and is still gated, while driver-sql escapes it post-#13714. It was deliberately left alone: that position carries a landed pin (#6401, remote-transport-groupby-node.test.ts) asserting the refusal, so reversing it is a judgement, not a mechanical edit. It is also not on the reproducing path — measured, not assumed: ObjectQLStrategy.resolveFieldName resolves a dimension to member.sql or member.split('.')[1], so only the measure arrives dotted. A regression control in the new suite pins the position's current behaviour so it cannot drift silently while that card is open.

Clause-②: yes — the guard stays intact in the field position (and in object); this diff touches the aggregation alias position only, and both untouched positions carry explicit regression controls. The diff does not reach packages/drivers/driver-sql (STOP condition 1 not triggered), and no open PR claims remote-transport.ts (re-checked against the 14 open PRs immediately before push, STOP condition 3).

Dispatch assumptions, measured

  • 2.1 anchor drift — FALSIFIED. SAFE_IDENTIFIER is still at remote-transport.ts:71, unmoved, at base 66ecc50a (main had already advanced past the 1403d943 the order cited).
  • 2.2 confined to driver-turso — HOLDS. Diff is 3 files, all in driver-turso plus the changeset.
  • 2.3 the un-bucketed path is the reproducing one — HOLDS. TursoDriver.supports publishes queryDateGranularity: {} in remote mode, so a bucketed query falls back to find() + in-memory bucketing and never reaches aggregate. The new suite reproduces on the un-bucketed grouped-cube shape.
  • 2.4 remaining un-enveloped refusals — MEASURED, and one remains. After this change the alias position refuses nothing, so there is no un-enveloped refusal left there. But assertSafeIdentifier itself still throws a bare Error (no code, no status) for the field, object, groupBy and DDL positions — so a genuinely unsafe field on this path still reaches the caller as an opaque 500. That is the other half of the driver-sql (PG): sum/avg/min/max over a boolean column throw the raw PostgreSQL 42883 with no ADR-0112 envelope (status undefined) #11455 / A dotted WHERE key escapes as a raw dialect error with the bound literal inlined on Postgres and MySQL — pre-existing, and measured against live servers #8931 shape and it is not closed here: picking its envelope (an injection refusal is not obviously INVALID_QUERY/400) is a judgement no landed evidence pins, and it would change the error surface for DDL and backfill callers too. Reported rather than guessed.

Generated by Claude Code

@github-actions github-actions Bot added the size/m label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 74a32a2edd55f06158effef71b9151f2f2eec36a → packageMentionDocs.

Which tree this was computed on

This run read content/docs from ceff15a3e18b43bc3b7a0676e6b1a74875d3202d — the merge of head 689d223f59823940012d20e0dc679b111d1ab4d9 into base 74a32a2edd55f06158effef71b9151f2f2eec36a, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ceff15a3e18b43bc3b7a0676e6b1a74875d3202d && git checkout ceff15a3e18b43bc3b7a0676e6b1a74875d3202d
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 74a32a2edd55f06158effef71b9151f2f2eec36a 689d223f59823940012d20e0dc679b111d1ab4d9 && git checkout -B drift-repro 74a32a2edd55f06158effef71b9151f2f2eec36a && git merge --no-ff 689d223f59823940012d20e0dc679b111d1ab4d9

node scripts/docs-audit/affected-docs.mjs --json 74a32a2edd55f06158effef71b9151f2f2eec36a

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queue Sep 2, 2026
Merged via the queue into main with commit 443f0d3 Sep 2, 2026
40 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14113-turso-remote-alias-quoting branch September 2, 2026 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

drivers(turso): RemoteTransport refuses a cube-qualified aggregation alias, so every analytics cube query on a remote datasource is an un-enveloped 500

2 participants