Repository navigation
fix(rest): an import row names its column from the error envelope, and the driver-string branch stops calling a declared field unknown - #20905
Conversation
…d the driver-string branch stops calling a declared field unknown toFailedResult read `field` only off a ValidationError finding, so a row the engine's declared-field door refused (INVALID_FIELD, the column in the envelope's `field`, bare names in `fields`) named no column on the dry run or on the commit, while POST /data/:object named it. The row now takes the envelope's `field` when no finding names one; a finding still wins. mapDataError's driver-string branch answered a missing column with "Unknown field", but it only sees the driver's text, and the engine refuses an undeclared key before any driver runs, so what reaches it is usually a DECLARED field whose column is missing (schema drift). It now says the table has no column for the field and names the reconcile step. code, status, field and object are unchanged. Claude-Session: https://claude.ai/code/session_01VvcEokUG1tvVxkceYfR5XB Co-authored-by: Claude <noreply@anthropic.com>
…409 names; changeset Claude-Session: https://claude.ai/code/session_01VvcEokUG1tvVxkceYfR5XB Co-authored-by: Claude <noreply@anthropic.com>
…f casting them The two `as any` casts grew check:query-options-erasure's test surface 236 -> 238; the options are plain EngineQueryOptions. Claude-Session: https://claude.ai/code/session_01VvcEokUG1tvVxkceYfR5XB Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift CheckThis PR changes 1 package(s): 8 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 4a16d887b3f89b64ca36815bed56c3fea12dc1be && git checkout 4a16d887b3f89b64ca36815bed56c3fea12dc1be
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 33b6e8bece79e70156dc2c1880813f8d88af61d4 901010d4d8a577742867e4f6384c7e231746e76b && git checkout -B drift-repro 33b6e8bece79e70156dc2c1880813f8d88af61d4 && git merge --no-ff 901010d4d8a577742867e4f6384c7e231746e76b
node scripts/docs-audit/affected-docs.mjs --json 33b6e8bece79e70156dc2c1880813f8d88af61d4
|
Contract reviewServed-tier: Inputs read: card #20701 body and all ten comments (governing: ① Derived judgments(a)
(b) The driver-string branch — right; code, status, field, object verified unchanged. The body literal's (c) Item 1 not delivered — each premise holds on the merge-base.
(d) Changeset ( ② Semver level
③ Boundary flagsDev
Dev
Check-runs on Implemented-by: VERDICT: PASS Generated by Claude Code |
…he create door answers (objectstack-ai#20941) Part of objectstack-ai#20701 Clause-②: no Item 4 of the REST half of objectstack-ai#20701 (the schema-drift row), as triage's answer `5917724406` scopes it. Item 1, the drop signal on the row report, is not in this PR: it waits on objectstack-ai#20922, which waits on objectstack-ai#20921. objectstack-ai#20701 remains open. ## What changed `toFailedResult` (`packages/rest/src/import-runner.ts`) now classifies a write error through `mapDataError`, the mapper `POST /api/v1/data/:object` uses, for the same object. When the mapper's verdict is `INVALID_FIELD`, the row carries the door's `code`, `field` and sentence. Every other verdict leaves the row as it was. So a declared field whose database column is missing (metadata and the physical schema have drifted) now fails the commit row, and the async job's rows, the way the create door fails it: `INVALID_FIELD`, `field` set, and the sentence "The database table of object 'X' has no column for field 'f'. If the object declares 'f', its database schema has drifted from the metadata: run 'os migrate' to reconcile." Before, the row carried the driver's code (`SQLITE_ERROR`), the driver's text and no `field`. The sync route and the async job worker both run through `runImport`, so one function covers both. A field-level finding still wins over the door, as it does for `code`. An undeclared key reads exactly as before, because the door relays the engine's own `INVALID_FIELD` envelope in the engine's own words. `packages/rest/src/error-response.ts` is not edited: `mapDataError` is exported, side-effect free and callable as it stands. ## Repro (SQLite `:memory:`, real ObjectQL + `ObjectStackProtocolImplementation` + RestServer routes) Drift is made the way it happens: the object declares `late` after the DDL ran, with no re-sync. | surface | before (base `67c1b11a20`) | after (`132e93c05b`) | |:--|:--|:--| | `POST /api/v1/data/:object` with `late` | `400 INVALID_FIELD`, `field: 'late'`, the drift sentence | unchanged | | import commit, insert | failed, `SQLITE_ERROR`, "table proj_probe has no column named late", no `field` | failed, `INVALID_FIELD`, `field: 'late'`, the door's sentence | | import commit, upsert onto an existing row | failed, `SQLITE_ERROR`, "no such column: late", no `field` | failed, `INVALID_FIELD`, `field: 'late'`, the door's sentence | | async job, `GET .../import/jobs/:jobId/results` | failed, `SQLITE_ERROR`, the driver's text, no `field` | failed, `INVALID_FIELD`, `field: 'late'`, the door's sentence | | import dry run | ok, created | ok, created (unchanged, see Acceptance notes) | | unique conflict row (control) | `UNIQUE_VIOLATION`, `field: 'code'`, the engine's sentence | byte-identical | | NOT NULL row (control) | `SQLITE_CONSTRAINT_NOTNULL`, "must is required.", no `field` | byte-identical | | writable row beside the drifted one (control) | ok, created | ok, created | ## Why only the `INVALID_FIELD` verdict The claim's controls say a unique-conflict row and a NOT NULL row keep today's answers. The door answers both in other words, so routing the whole row through the mapper would move them: - **Unique conflict.** Row: `UNIQUE_VIOLATION`, `field`, and the engine's sentence ("Duplicate record refused on 'X': a unique constraint on 'code' already holds this value. No record was written."). Door: the same `code` and `field`, but the curated sentence "A record with this code already exists" (no period), with the engine's sentence moved to `developerMessage`. - **NOT NULL.** Row: the driver's code (`SQLITE_CONSTRAINT_NOTNULL` on SQLite), "must is required.", no `field`. Door: `400 VALIDATION_FAILED` with a `required` finding for `must`, "must is required" and a drift `hint`. These differences are named in the report on objectstack-ai#20701. This PR does not change them. ## Other dialects (read from source, NOT MEASURED) The mapper's driver-string branch recognises SQLite `has no column named c` and `no such column: c`, MySQL `Unknown column 'c'` (the pattern is not anchored to `in 'field list'`, so an update or where-clause spelling matches too), and Postgres through `matchMissingColumnOfRelation`, which matches only the write-path template `column "c" of relation "t" does not exist`. The Postgres read-path spelling `column "c" does not exist` is not matched, so an upsert match lookup on a drifted column in Postgres would keep the row's old answer. The mapper is not widened here. ## Pins `packages/rest/src/import-row-schema-drift-20701.test.ts`, 5 cases through the public doors: the insert commit row, the upsert (update path) commit row and the async job's results row each equal the create door's `{ code, field, error }` and carry none of the driver's words; a writable row beside the drifted one is created (control); the unique-conflict and NOT NULL rows keep their answers (controls). The control pins say "unchanged by this PR", not "right": a card that converges them updates them on purpose. ## Verification (head `132e93c05b`) Heavy steps ran under `scripts/pm/os-verify-lock.sh`. - **Dependency closure** `turbo run build --filter='@objectstack/rest^...'`: 24 tasks successful. - **Pins plus the neighbouring import suites** (the new file, `import-row-report-field-20701`, `import-runner-unique-violation-row`, `import-runner-error-sanitize`, `import-job-integration`, `import-integration`, `import-dryrun-parity`, `import-runner-bulk`, and the throwaway probe): `Test Files 9 passed (9)`, `Tests 160 passed (160)`. - **Package suite** `pnpm --filter @objectstack/rest test`: `Test Files 243 passed (243)`, `Tests 4858 passed | 114 skipped (4972)`. The `--maxWorkers=2` I passed after a bare `--` was dropped by vitest, so this ran with the default worker count; it is still the whole `local` project. The package's one `repo`-project test reads none of the changed files. - **Typecheck** `pnpm --filter @objectstack/rest typecheck`: exit 0, `check:test-typecheck: OK`; `tsc -p tsconfig.test.json --listFilesOnly` lists the new pin file. - **Ablation**, with the change committed first, through `scripts/ablation-replace.mjs` WRAP mode. The subject is read from `packages/rest/src` by relative import, so no `dist/` is involved. Anchor `const door = mapDataError(err, objectName).body;` replaced by an empty door, 1 to 0; blob `e8012ff2402a` to `bf0b6d366a5c`. Result over the new file and the objectstack-ai#20905 pin file: 3 failed, 11 passed. The three are the insert, upsert and async-job drift cells, each back at `SQLITE_ERROR` with the driver's text; the controls and the objectstack-ai#20905 pins stayed green, as predicted. Restored: `blob == HEAD (e8012ff)`, `git diff HEAD` empty. - **Gates**: `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` at `132e93c05b` derives 61 commands. Each ran with its exit code recorded before any pipe, and `--ran` reads "61 derived famil(ies) accounted for — 60 run, 1 NOT-MEASURED". NOT MEASURED: `check:dual-build-cjs-loads`, `PREREQUISITE NOT MET` (exit 3), because it needs every package's `dist/`, which is the full build CI runs. `check:dts-closure`, `check:sourcemap-no-sources-content` and `check:type-check-debt` were re-run after building `@objectstack/rest` and the six packages the debt re-measure needs, and read 0. - **Lint**, narrowed with proof at `132e93c05b`. (1) eslint's own config lints the two `.ts` files in this diff, and `isPathIgnored` is true for the changeset `.md`, so those two files are the diff's whole lint population. (2) `eslint --no-inline-config --format json` over them: `files=2 errors=0 warnings=0`. (3) `calculateConfigForFile` shows `parserOptions.project` and `projectService` null on both, so type-aware linting is off and this diff cannot change eslint's verdict on any file it does not touch. - Control-byte self-scan over the three changed files: clean. ## Acceptance notes - **The dry run still previews a drifted column as ok.** `engine.validate` reads the metadata, never the table, so the preview promises a row the commit then fails. That is the card's own path ("an import preview promises only rows the commit will write"), and the report names it for the seat. The new pin file does not vouch for the dry-run reading. - **A sandboxed producer that throws `INVALID_FIELD` with no field-level finding** now gets the door's answer on its row too, which is the business sentence the sandbox carried rather than the QuickJS debug wrapper. Read from source (`classifyDataError`'s sandbox unwrap), NOT MEASURED. - **Rows whose door verdict is not `INVALID_FIELD` keep `sanitizeRowError`.** That covers the two controls above, and any driver text `sanitizeRowError` passes through because it does not read as SQL. The door's answer for those was not measured here. --- _Generated by [Claude Code](https://claude.ai/code/session_01VvcEokUG1tvVxkceYfR5XB)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Part of #20701
Clause-②: no
The REST half of the #20701 split, delivered in part. Two of its three items land here. The third, the drop signal on the import row report, cannot be read from the seam this package reads, so it goes back to the seat with the extraction it needs (see "Not in this PR"). #20701 remains open.
What changed
toFailedResult(packages/rest/src/import-runner.ts) readfieldonly off aValidationErrorfinding. The engine's declared-field door throwsINVALID_FIELD/ 400 with the column in the envelope'sfieldmember and the bare names infields(strings, not findings). So a row that door refused named no column, on the dry run and on the commit alike, whilePOST /api/v1/data/:objectnamed it. The row now takes the envelope'sfieldwhen no finding names one. A finding still wins, as it does forcode.DuplicateRecordErrorcarriesfieldtoo, so a unique-conflict row now names the column the409names.mapDataError's missing-column branch (packages/rest/src/error-response.ts) sees only the driver's text, not the object's field map. The engine refuses an undeclared write key itself before any driver runs, in its own words. So what reaches this branch is usually a field the object DECLARES whose column is missing (schema drift). The sentence now reads "The database table of object 'X' has no column for field 'f'. If the object declares 'f', its database schema has drifted from the metadata: run 'os migrate' to reconcile."code,status,fieldandobjectare unchanged. The engine's own "Unknown field" sentence for an undeclared key is untouched, and a pin holds that control.packages/rest/src/import-row-report-field-20701.test.ts, 9 cases on real ObjectQL +ObjectStackProtocolImplementation+ SQLite:memory:, through the public doors.@objectstack/restpatch.Clause-②: no, re-read against this diff. The claim'syes (widening)was written for the drop-signal key, which is not in this diff.fieldis an existing optional key ofImportRowResultSchema, now filled in more cases. The sentence change moves no key and no accept set. No error code is minted.Readings
Base
ca5408c629(before) and head901010d4d8(after). Real ObjectQL + protocol + SQLite:memory:, non-system caller. The "before" column is from a throwaway probe (never committed); the unique-conflict "before" is ablation leg A below, which restores the old read.POST /api/v1/data/:objectnopeINVALID_FIELD, nofield→field: 'nope'400 INVALID_FIELD,field: 'nope'(unchanged)uniquevalue already heldvalidatedoes not touch the driver)UNIQUE_VIOLATION, nofield→field: 'code'409 UNIQUE_VIOLATION,field: 'code'(unchanged)late, table has no columnvalidatedoes not touch the driver)SQLITE_ERRORwith the driver's text; not re-measured after, and the driver's error carries nofieldfor this PR to read (see Acceptance notes)400 INVALID_FIELD"Unknown field 'late' on object 'proj'" → "The database table of object 'proj' has no column for field 'late'. ..."doubled201+droppedFieldsreasoncomputedro201+droppedFieldsreasonreadonlytitle201Not in this PR: the drop signal, and why
The card's first item reads: "The import row report carries the drop signal, in the dry run and in the commit alike, read from the engine's
droppedFields". This package cannot read it on the dry run, and on the commit's bulk path it cannot place it on a row.p.validateData, theImportProtocolLikeseam typed byValidateDataRequest/ValidateDataResponsefrom@objectstack/spec/api.ObjectQL.validatereports drops through itsonFieldsDroppedoption. ButObjectStackProtocolImplementation.validateData(packages/metadata-protocol/src/protocol.ts) forwards onlymodeandcontext, andValidateDataResponseSchemahas nodroppedFieldsmember. Measured at base for a formula key and a readonly key: the response is{ object, mode, valid, results, posture }, with nothing about the drop.insertManyData, whosedroppedFieldsis the batch-level union by design. Its docblock andObjectQL.insertMany's both say why: the union cannot be resolved back to rows, and a reconstruction at a call site names rows that are not at fault. The per-row writes (createData,updateData) report per row.So both shapes need work outside
packages/rest:insertMany, plus the protocol and spec halves of the dry run.sys_import_jobfor the async path.Either shape also owes a declaration in the spec's
ImportRowResultSchema/ImportResponseSchema. The options, costs and a recommendation are in the report comment on #20701.Verification (head
901010d4d8)All runs on this worktree, SQLite
:memory:, heavy steps underscripts/pm/os-verify-lock.sh.import-row-report-field-20701.test.ts) at901010d4d8:Test Files 1 passed (1),Tests 9 passed (9), andpnpm --filter @objectstack/rest typecheckexit 0 (check:test-typecheck: OK;tsc --listFilesovertsconfig.test.jsonlists the pin file).pnpm --filter @objectstack/rest exec vitest run --project local --maxWorkers=2) at6eb88d7028, the merge oforigin/main:Test Files 238 passed (238),Tests 4658 passed | 103 skipped (4761). The one commit after it removes twoas anycasts from the pin file, and the pin file and the typecheck were re-run on it (above). The package's onerepo-project test does not read the files changed here.scripts/ablation-replace.mjsWRAP mode (the anchor must hit, the blob must move, and the restore is proven asblob == HEADplus an emptygit diff HEAD). The subject is read frompackages/rest/srcby relative import, so nodist/is involved.import-runner.ts: anchor: e?.field;1 to 0, blob60e13a17efecto6ba5cf4c20cd. The row goes back to readingfieldoff a finding only. Result: 4 failed, 5 passed. The four are the unknown-column cells (insert and upsert), the row-versus-door cell and the unique-conflict cell, each missingfield. Restored:blob == HEAD (60e13a17efec),git diff HEADempty.error-response.ts: anchorhas no column for field1 to 0, blob2db9a02d90f4toa427b6b19471. The sentence says "Unknown field" again. Result: 1 failed (the drift cell), 7 passed. Restored:blob == HEAD (2db9a02d90f4),git diff HEADempty.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commandsat901010d4d8derives 62 commands. Each was run with its exit code recorded before any pipe.--ranreads "62 derived famil(ies) accounted for — 60 run, 2 NOT-MEASURED". NOT MEASURED:check:dual-build-cjs-loadsandcheck:type-check-debt, bothPREREQUISITE NOT MET(exit 3), because each needs every package'sdist/, which is the full build CI runs. The first pass at6eb88d7028caught one real red,check:query-options-erasure(test surface 236 to 238, the pin file's two casts);901010d4d8fixes it and the gate reads 236 again..tsfiles in this diff, andisPathIgnoredis true for the changeset.md, so those three files are the diff's whole lint population.eslint --no-inline-config --format jsonover them at901010d4d8:files=3 errors=0 warnings=0.calculateConfigForFileshowsparserOptions.projectandprojectServicenull on all three, so type-aware linting is off: this diff cannot change eslint's verdict on any file it does not touch.ca5408c629: a throwaway probe (never committed) produced the "before" cells in the table above.Acceptance notes
latereadscode: 'SQLITE_ERROR', error "table proj has no column named late", while the create door answers400 INVALID_FIELDwith the sentence above. The dry run was not measured;validatedoes not touch the driver, so it cannot see the missing column. This is the same row-versus-door agreement family as rest(import): a column for a formula field passes the dry run, then fails the row at commit with the driver's SQL error, where the create door answers 400 INVALID_FIELD #20701, reached only through schema drift, and it is left for the seat to fold into that card's remaining half. Not filed.ImportRowResult.warnings(packages/rest/src/import-runner.ts) is absent fromImportRowResultSchemain@objectstack/spec/api, so the typed client (client.data.import, typed by the spec'sImportResponse) cannot see it. That matters for where the drop signal is declared. Noted, not filed.Part of #20701, notFixes, because the drop signal is not here. The Clause-② line isno, not the claim'syes (widening), because the widening key is not here either.Generated by Claude Code