Skip to content

fix(driver-turso,driver-sql): a business-key upsert keeps the stored id on the remote face, and both faces answer the stored row (#21166) - #21184

Merged
objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/issue-21166-remote-upsert-id-insert-only
Oct 1, 2026
Merged

objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/issue-21166-remote-upsert-id-insert-only

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Fixes #21166
Clause-②: no

What this does

Remote face (driver-turso). TursoDriver.upsert's remote branch now passes SqlDriver.insertOnlyUpsertColumns(object) to RemoteTransport.upsert. This is the same list the local faces build their merge set from, so it names id, created_at and the auto_number columns. Before, the branch passed only the autonumber columns, from a lookup of its own (remoteAutoNumberColumns, deleted here). The merge set then carried "id" = excluded."id", and an upsert keyed on a business column replaced the stored row's primary key. This is the re-key the SqlDriver.upsert docblock forbids: "id is insert-only … the moment conflictKeys names a business key the merged row's identity is silently replaced". There is now one list and no second copy. A remote object never renames a column, because remoteTableFor refuses a renaming column map before any statement. So the list's physical names are the names the transport writes.

The answer (both faces). With id insert-only, the remote read-back by the payload's id stopped finding the merged row and answered the payload instead. The local face already behaved that way: H3 below. Both read-backs now look the landed row up by its conflict-key values, which identify it on both legs. When a conflict key is empty in the payload, nothing can have matched, so the row was inserted and the read is by id. On the default ['id'] target, both readings are the same query.

Hypotheses, measured

  • H1: confirmed. I reproduced it on origin/main f3b16fc2f with the card's table on the libsql-sqlite stub (turso-local-remote-upsert-identity-parity.test.ts, run before the fix: 8 failed | 5 passed):
    • On the remote face, upsert({ id: 'row-NEW', email: a, title: 'edited' }, ['email']) stored id: 'row-NEW'.
    • upsert({ email: b, title: 'edited too' }, ['email']) stored id: 'u9ZJdBf6xIsx0URs', a minted nanoid.
    • The _id alias stored 'row-ALIAS'.
    • The local face kept row-a and row-b.
  • H2: the raise rule is NOT met. runImport's writeMode: 'upsert' never reaches driver.upsert:
    • It finds the record by matchFields through findExisting → p.findData (packages/core/src/utils/import-runner.ts:574, :583).
    • Then it updates by id, p.updateData({ id: target.id }) (:941), or it creates through p.createData (:965), createManyData or insertManyData (:757).
    • ImportProtocolLike (:132) declares no upsert member, and bulk-write.ts has zero upsert references.
    • The connector pull hands artifact.upsertKey to that runner as matchFields (packages/services/service-automation/src/connector-pull.ts:291, :413).
    • The only in-repo driver.upsert caller is still LifecycleService with ['id'] (packages/objectql/src/lifecycle/lifecycle-service.ts:1405). The sandbox ql.upsert falls back to insert, because the engine has no upsert.
  • H3: measured on SQLite and on a private live PostgreSQL 16. It is the same list's consequence, so it is fixed here. SqlDriver.upsert stored os21166_seed and answered id: 'os21166_new': the read by the payload's id missed, and || toUpsert answered the payload. That is the failing ablation leg A1 below, on both cells. MySQL was NOT MEASURED, because no server is in this container. The code path is dialect-independent.

File-surface increment (declared). The claim listed sql-driver.ts as read-only unless lifted. H3's ruling arm adds three things:

  • the read-back in packages/drivers/driver-sql/src/sql-driver.ts (nothing else in that file);
  • one pin in sql-driver-upsert-conflict-target-dialects.test.ts;
  • @objectstack/driver-sql: patch beside @objectstack/driver-turso: patch in the changeset.

Pins

  • driver-turso/src/turso-local-remote-upsert-identity-parity.test.ts, 13 cases on both faces:
    • the card's table: each stored id is kept, the merge still writes title, and the answer names the stored id;
    • the _id alias;
    • created_at is insert-only, the shared list's second member;
    • controls: an id-keyed upsert, by default and with ['id'] named, and a business-key upsert that inserts;
    • a parity case that compares the two faces with each other and with the right answer.
  • driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts adds one case to the existing pins that keep the merged row's identity. It runs on the SQLite and live-PG cells: the answer names the stored id for a supplied and for a minted payload id, and the insert-leg answer is the control.

Ablations

Each fix was committed first. Each mutation went through scripts/ablation-replace.mjs, which proved it on disk, and was restored to a HEAD-equal blob.

leg mutation red green
A1 local read-back back to where('id', toUpsert.id) (driver-sql rebuilt; ablation-dist-preflight saw the marker in 2 dist files, then --absent after the restore build) turso pins: the 3 local answer cases + parity (4 failed); driver-sql pin on sqlite and live postgres (2 failed, PG answered 'os21166_new') every remote case
A2 remote set filtered back to autonumber-only the 3 remote stored-id cases, remote created_at, parity (5 failed) local face, controls
A2b only created_at dropped from the remote set remote created_at alone (1 failed; stored '2001-01-01…') the rest
A3 remote read-back back to "id" = ? the 3 remote answer cases + parity (4 failed; answered 'row-NEW') remote stored-id cases

Tests (head f80af709ec, merge of origin/main fbcc05f40; all under os-verify-lock.sh)

The test runs below ran on the fix commit 7a597948b3. The merge brought only plugin-audit and docs files, so they were not re-run. The gate union ran on f80af709ec.

  • driver-sql build exit 0. Closure pnpm --filter '@objectstack/driver-turso^...' build exit 0.
  • driver-turso full vitest run: 83 passed (83) files, 2231 passed | 33 skipped.
  • driver-sql full suite in two halves of 109 and 108 files: 104 passed | 5 skipped, 1440 passed | 100 skipped; 102 passed | 6 skipped, 1915 passed | 88 skipped.
  • The 8 driver-sql files that call .upsert( with OS_TEST_POSTGRES_URL on a private PG 16: 8 passed, 125 passed | 3 skipped, live postgres RAN, live mysql NOT RUN.
  • driver-sqlite-wasm (extends SqlDriver) upsert file: 3 passed.
  • driver-turso and driver-sql typecheck exit 0. --listFiles includes both test files.
  • Driver conformance ledger, before (f3b16fc2f) and after: identical, 50 covered cell(s), 0 in the DEBT ledger, 0 exempt.
  • Gate union, re-derived with no paths on f80af709ec: 63 commands plus 4 roster gates named as ⛔ for these paths. All exit 0. check:dual-build-cjs-loads and check:lean-entry-closure first exited 3 (PREREQUISITE NOT MET) and exited 0 after the builds they name. dispatch-gates --ran: 63 derived, 63 run, 0 NOT-MEASURED, 0 UNRUN.

Acceptance notes

  • Pin sweep. No other test pinned the payload answer. driver-memory already answers the stored id on a business-key merge (memory-unique-constraint.test.ts:174 expects '1'), so the SQL faces now agree with it.
  • The answer pin sits in the sqlite + pg sweep, so the MySQL cell does not run it. The MySQL read-back takes the same branch.
  • A measured cross-tenant behaviour of business-key upserts on the local face goes to the seat in the report. It was not changed here.

Generated by Claude Code

claude added 2 commits October 1, 2026 13:39
…id on the remote face, and both faces answer the stored row

The remote upsert names SqlDriver.insertOnlyUpsertColumns to the
transport instead of an autonumber-only lookup of its own, so the merge
set leaves id and created_at alone as the local faces do. Both
read-backs look the landed row up by its conflict-key values, so a merge
on a business key answers the stored row instead of the payload.

Claude-Session: https://claude.ai/code/session_01Ujdtvqs7ree7WyQmEDwEnG
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added the size/m label Oct 1, 2026
@github-actions github-actions Bot added documentation Improvements or additions to documentation tests labels Oct 1, 2026
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/driver-sql, @objectstack/driver-turso, touching 2 documentable anchor(s).

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx (via TursoDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx (via TursoDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx (via TursoDriver (symbol, a top-level class))

⛔ 1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17/17-5.mdx (via TursoDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 54 of 206 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 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.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 14 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 454bbb6866f64da574a29dcc1e0be320bff50189 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 4a17d99b957b492a3ec744247b55c58d48f8f1b7 — the merge of head f80af709ecacafdce2ae2a95af1d0336be1c7f62 into base 454bbb6866f64da574a29dcc1e0be320bff50189, 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 4a17d99b957b492a3ec744247b55c58d48f8f1b7 && git checkout 4a17d99b957b492a3ec744247b55c58d48f8f1b7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 454bbb6866f64da574a29dcc1e0be320bff50189 f80af709ecacafdce2ae2a95af1d0336be1c7f62 && git checkout -B drift-repro 454bbb6866f64da574a29dcc1e0be320bff50189 && git merge --no-ff f80af709ecacafdce2ae2a95af1d0336be1c7f62

node scripts/docs-audit/affected-docs.mjs --json 454bbb6866f64da574a29dcc1e0be320bff50189

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 454bbb6866f64da574a29dcc1e0be320bff50189 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: f80af709ecacafdce2ae2a95af1d0336be1c7f62
Local-runs: none

Read: card #21166 (body, triage 5930765705, claim 5932434922, dev report 5933523104), PR #21184 (body, the six-file list, the net diff against main at merge base fbcc05f40), the 31 check-runs on the head, and the surrounding source at the head through git show on the fetched ref. The contract is SqlDriver.upsert's #8622 docblock: id is insert-only, and a merge on a business key never re-keys the row.

① Derived judgments

Accept set: no call is newly accepted or newly refused. Each change the diff implies, judged:

  1. One list on the remote face (turso-driver.ts): right. TursoDriver.upsert's remote branch now hands RemoteTransport.upsert the spread of SqlDriver.insertOnlyUpsertColumns(object): created_at, the physical id column and every auto_number column. The private remoteAutoNumberColumns lookup is deleted and no reference to it survives at the head; the only insert-only spellings left in driver-turso/src are the transport's parameter and this one call site. remoteTableFor refuses a renaming column map before any remote statement, so the list's physical names are the names the transport writes.
  2. Remote merge set (remote-transport.ts): right. The merge set is every payload column minus the merge keys minus the insert-only list; when it empties the statement is DO NOTHING, so a merge whose only non-key columns are insert-only leaves the stored row alone instead of restamping created_at. That is the contract's intent and it moves no refusal. The insert leg still writes every column, id included.
  3. Remote read-back: right. The landed row is read by the conflict-key values when the payload carries every one of them (neither undefined nor null), else by id. The SELECT serialises the values with the same serializeValue the INSERT used, and carries no tenant scope, exactly as the old WHERE "id" = ? did not. libsql is SQLite semantics: the ON CONFLICT target must be an arbiter unique index over exactly the key columns (the unbacked case is already refused), so after a statement that did not throw, the key values name at most one row, and it is the one the statement inserted or merged into.
  4. Local read-back, the declared H3 increment (sql-driver.ts): right, on the dialect reading below. sent is the write-column-mapped, storage-form row of the last attempt (the same object stampInsertTimestamps and stampUpsertUpdatedAt mutate, so it holds what was sent); getBuilder is a raw knex builder, so where(k, sent[k]) compares the physical column with the value the statement sent; applyTenantScope is applied as before.
    • SQLite and PostgreSQL. The server honours ON CONFLICT (keys) only against a unique index over exactly those columns (refuseUnbackedConflictTarget otherwise). The insert leg writes the key values; the merge leg matched on them and, since the key columns are not insert-only, re-writes them with the equal excluded values. So the landed row carries the sent key values and no other row can. The read names the landed row.
    • MySQL. assertConflictTargetHonoured refuses a caller-named target that no index covers exactly, and refuses a rival non-primary unique key as ambiguous; the primary key is never a rival, so ON DUPLICATE KEY UPDATE may land on the primary-key row rather than the named key's row. Even then the merge set writes the sent key values onto the landed row (they are merge columns), and a second collision on the key during that UPDATE raises a unique violation rather than landing silently. The read by the key values therefore names the landed row on this dialect too. Reasoned from the code, not measured: the dev's MySQL cell did not run, and the new driver-sql pin sits in the sqlite + pg sweep (ON_CONFLICT_DIALECTS), so the MySQL cell never executes it.
    • A NULL conflict key, alone or as one member of a composite key. NULL never conflicts under the unique indexes this driver creates, on any of the three dialects, so the statement inserted and the row carries toUpsert.id; matchedOn tests every key for undefined and null and falls back to the id read. Right.
    • A composite key with every member present. The read ANDs all members; the covering index is over exactly that set; one row. Right.
    • A tenant-scoped object. A per-tenant unique key is only ever backed as a composite that includes the tenant column, so a target naming the business column alone is refused on every dialect, and a composite target carries the tenant column (stamped by injectTenantOnInsert) into the read. A globally unique key names at most one row installation-wide. The read is scoped to options.tenantId (field = tenant OR field IS NULL), as the old read was; an admin write that names another tenant in the payload still misses and answers the payload, unchanged by this PR and not this card's. The case in which a globally unique key matches a row another tenant owns is the security finding the report names: escalated in ③, not restated here.
    • Could the read answer a row other than the landed one? No case found. The only miss is the pre-existing tenant-scope miss above, which answers the payload exactly as before. Two unchanged edges, named for the record: mergeKeys are the caller's spellings while sent is keyed by physical names, but a renaming column map on a conflict key never reaches the read because onConflict(mergeKeys) is emitted verbatim and the server refuses the unknown column first; and the read is on object, not the rotation writeTable, exactly as on main.
  5. H2, the raise rule: confirmed from the code, not met; the card stays p2. ImportProtocolLike (import-runner.ts) declares findData, createData, updateData, createManyData, insertManyData and no upsert member; findExisting matches by matchFields through findData, and the write is updateData({ id: target.id }) or a create. connector-pull.ts:289-292 folds artifact.upsertKey into matchFields for that runner. Non-test .upsert( callers at the head outside the drivers: lifecycle-service.ts:1405 with ['id']; body-runner.ts:780 on the sandbox ql, not the driver; knowledge-service.ts on a search adapter. No in-repo producer reaches driver.upsert with a business key.
  6. Public surface: unchanged. remoteAutoNumberColumns was private; insertOnlyUpsertColumns is the pre-existing protected member, signature untouched; SqlDriver.upsert, TursoDriver.upsert and RemoteTransport.upsert keep their signatures. The answer now names the stored row on a business-key merge, which is the documented contract's content, not a surface move.
  7. Pins: right. turso-local-remote-upsert-identity-parity.test.ts: the card's table on both faces (stored id kept, title merged, the answer names the stored id), the _id alias, created_at on the merge leg, the id-keyed and insert-leg controls, and the face-to-face parity case. sql-driver-upsert-conflict-target-dialects.test.ts: one answer pin on BACKED (email: unique), supplied and minted payload ids, with the insert leg as the control, on the sqlite and live pg cells.

② Semver level

.changeset/21166-remote-upsert-id-insert-only.md: @objectstack/driver-turso: patch and @objectstack/driver-sql: patch. Both packages are public and released (17.5.0, private: false, access: public), so skip-changeset would have been wrong and patch is the level a bug fix in a released package takes. Nothing authorable is removed or renamed and no export is added, so no migration text and no ADR-0087 marker is owed. Clause-②: no is in the PR body and in the changeset body, and it is right: the accept set did not move, no call is newly refused or newly admitted, and neither (widening) nor (narrowing) applies. The body carries no model identifier and names update() as the deliberate re-key path. Check Changeset on the head: success.

③ Boundary flags

  • File-surface increment (dev flag). The claim held sql-driver.ts read-only unless lifted, and separately ruled that the local returned-id observation is fixed here only if it is the same list's consequence. It is: id on the insert-only list is exactly what makes the payload's id name no row on the merge leg, so the read by that id missed and the fallback answered the payload. The increment is the read-back only, one pin, and the driver-sql changeset line, all declared in the PR body. Answered: accepted.
  • MySQL NOT MEASURED (dev flag). Answered by the dialect reading in ①.4; the new driver-sql pin does not run on the MySQL cell by construction, and Temporal Conformance (live PG + MySQL) is in progress and is not this pin. Residual for the seat, not a FAIL: an answer pin on a MySQL cell is owed when a server is at hand.
  • Raise rule (dev flag). Not met, confirmed in ①.5; the card stays priority:p2.
  • open_questions. Empty; nothing to answer.
  • out_of_scope_findings[0]. The security finding the report names. Escalated to the seat to file as a security finding. It is not changed by this PR, this record does not judge it, and its mechanism is deliberately not restated here.
  • Check-runs on the head (31). 12 success: Auto Label, Check Changeset, Check Documentation Links, Check PR Size, filter, Flag docs affected by code changes, Governed Surface Queue Guard, No other open PR may claim the same issue, No other open PR may claim the same single-writer path, Part-of PR must not also close its card, The card this PR closes must claim this branch, Type Check · source gates. 3 skipped by path or opt-in: Build Docs, Console Pin Gate, Packed-tarball smoke (opt-in). 16 in progress, each of them not a verdict: Build Core; Dogfood Regression Gate (1/3), (2/3), (3/3); Dogfood Verify CLI; Lint & Repo Gates; Temporal Conformance (live PG + MySQL); Test Core (1/6), (2/6), (3/6), (4/6), (5/6), (6/6); Type Check · consumer gates; Type Check · debt ledger; Type Check · workspace. The landing waits for those; this record does not, and they were not polled.
  • Docs Drift Check (advisory, 5933491736). Three hand-written pages name TursoDriver; the change restores the documented contract rather than changing it, so no doc edit is owed and the release-owned page stays untouched.
  • Governance. No governed surface in the six-file list; the head repo is the base repo, not a fork.

Implemented-by: claude/issue-21166-remote-upsert-id-insert-only
Reviewed-by: session_01Ujdtvqs7ree7WyQmEDwEnG

VERDICT: PASS


Generated by Claude Code

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review October 1, 2026 14:45
@objectstack-fleet
objectstack-fleet Bot enabled auto-merge October 1, 2026 14:45
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Oct 1, 2026
Merged via the queue into main with commit ebdb6f2 Oct 1, 2026
36 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/issue-21166-remote-upsert-id-insert-only branch October 1, 2026 15:08
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

2 participants