Skip to content

fix(core): an import row with several bad cells reports every one in errors (#25582) - #26543

Merged
objectstack-fleet[bot] merged 3 commits into
mainfrom
claude/project-thread-sc5ydc
Oct 27, 2026
Merged

objectstack-fleet[bot] merged 3 commits into
mainfrom
claude/project-thread-sc5ydc

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Requested by Leehom · project thread

Fixes #25582

This closes the card: maintainer ruling 6377844879 letter A ruled proposals 1, 2 and 4, and not 3. Proposals 1 and 2 landed in #25596; this PR is proposal 4, so every ruled proposal has then landed. Proposal 3 (option aliases) is not ruled and nothing here implements it.

Clause-②: yes (widening: a failed import row whose cells did not coerce gains the optional errors list, ImportRowResultSchema.errors; every existing key keeps its value)
Metadata-protocol: none

维护者速读

  • 改了什么:导入时一行有多个坏单元格,行报告新增 errors 列表,按列序列出每个坏单元格(field、code、message);原来的 field / code / error 不变,仍指第一个。
  • 为什么改:以前只报第一个坏单元格,操作员改一格、重导一次才看到下一个,试导时每行要两三轮(/import: common spreadsheet cell shapes fail whole rows (dotted / compact dates, option value case, one error per row) #25582 裁决 A 第 4 条)。
  • 风险与回滚:只是响应里多一个可选键,同步导入、异步任务、试运行、正式提交都一样;回滚即撤销 import-runner.ts 那一处赋值。
  • 先看:packages/core/src/utils/import-runner.ts(行报告赋值处与 ImportRowResult.errors 注释),packages/rest/src/import-row-cell-errors.test.ts(四条路径加两种不带 errors 的失败)。
  • 两分钟验证:pnpm --filter @objectstack/rest exec vitest run src/import-row-cell-errors.test.ts,预期 6 个用例全过;用例里一行同时填坏日期、坏数字、坏选项,结果的 errors 按列序列出这三项。
  • 需要你做的:无,席位落地。

What changes

  • runImport (packages/core/src/utils/import-runner.ts) copies coerceRow's cell errors onto the failed row as errors, one { field, code, message } element per failed cell, in column order. The first-error keys field, code and error are unchanged and are element 0, as ImportRowResultSchema.errors (landed in feat(spec): ImportRowResultSchema declares errors, every cell error of a failed row #26454) states.
  • Filled at the one place a row fails on cell coercion, which comes before the dry-run / commit split, so it reaches the dry run and the commit, on the synchronous POST /api/v1/data/:object/import and on the async job's GET /api/v1/data/import/jobs/:jobId/results (the job stores summary.results and the results route returns the items verbatim).
  • Not filled, per the spec seat's fill rule (6434667735): a validate-verdict row (error is composeValidationMessage(...)), a refused write (toFailedResult / adoptDoorVerdict), and the duplicate / ambiguous / blank match-key rows.
  • ImportRowResult (@objectstack/core) gains the optional errors?: ValidateDataIssue[], documented with that rule.
  • Changeset: @objectstack/core patch.

Why

An operator importing a sheet with several bad cells in a row fixed one cell, re-ran, and only then saw the next. The trial in the card took two to three rounds per row.

Tests

  • packages/core/src/utils/import-runner-cell-errors.test.ts (new, 5 cases, against an ImportProtocolLike double): three bad cells around a good one report all three in column order on the dry run and on the commit (the commit writes nothing), element 0 equals the first-error keys, every element has exactly field / code / message; one bad cell gives a one-element list; a validate-verdict refusal and a refused write carry no errors.
  • packages/rest/src/import-row-cell-errors.test.ts (new, 6 cases, real ObjectQL + protocol + SQLite with sys_import_job): the same row through the sync route and the async job, each on the dry run and the commit; and the absence pin for a dry-run validate verdict and a commit refused write (a missing required field), sync and async.
  • Red first on main (45de8c6cba), tests committed alone as 46ca67cf: core 3 failed / 2 passed, route 4 failed / 2 passed. The four route reds read the first cell only (field: opened, code: invalid_date) with no errors key. The two absence pins pass on main too, as they should.

Acceptance notes

  • A JSON-format route test must register the stored-metadata family (sys_metadata and siblings) for the protocol to hand the route the object's field metadata. Without it every cell passes through uncoerced and the engine's validator answers instead, so a row with bad cells reports a joined validation message and no cell errors. The new route test registers them, as import-year-first-day-option-case.test.ts does. That is test setup, not a product path.
  • Proposal 3 (aliases on options) stays unruled; closing this card does not decide it.

Verification

  • Head 7a40605863 (after merging origin/main; the merge touched no import file).
  • pnpm --filter @objectstack/core test: 123 files / 2793 tests passed; pnpm --filter @objectstack/core typecheck passed (test layer included). Measured at a975cbab, before the merge.
  • pnpm --filter @objectstack/rest test: 325 files, 6489 passed / 326 skipped; pnpm --filter @objectstack/rest typecheck passed. Measured at a975cbab.
  • At 7a40605863: the two new files plus import-year-first-day-option-case, import-dryrun-parity, import-row-schema-drift-20701 and import-job-integration: 6 files / 49 tests passed.
  • Ablation, once, through scripts/ablation-replace.mjs: the errors: fill line deleted (anchor 1 to 0, blob 4983d0e39a26 to 923db228c535), core rebuilt, ablation-dist-preflight --absent passed (marker absent from all 14 built files). Core test 3 failed / 2 passed, route test 4 failed / 2 passed. Restored: blob equals HEAD, git diff HEAD empty; rebuilt, preflight found the marker back in dist/ and the tree clean.
  • node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack derived 61 commands at 7a40605863; all 61 exit 0. Two first ran exit 3 for a missing prerequisite (check-plugin-teardown-shape --self-test on a shallow clone; check:dual-build-cjs-loads before a full build) and were re-run green after git fetch --unshallow and the full build check:type-check-debt performs. --ran: "61 derived famil(ies) accounted for — 61 run, 0 NOT-MEASURED".
  • pnpm exec eslint --no-inline-config packages/core packages/rest: exit 0.
  • Not run locally: test:repo (no repo-project test touched; CI Test Core runs it) and the full dogfood battery (no ! in the title and no narrowing arm).

Human Minimum

  • 1. Schemas start as Zod; packages/spec holds no runtime logic. (No packages/spec change.)
  • 2. Every name I added uses the casing for its kind.
  • 3. No formatter ran; pnpm lint passes (scoped to the touched packages).
  • 4. One issue, one branch off an up-to-date main, one pull request.
  • 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset.
  • 6. packages/spec touched: n/a.
  • 7. No hand edits to content/docs/references/, packages/*/CHANGELOG.md or content/docs/releases/.
  • 8. Title, body and comments are in English (the maintainer brief is in Chinese by convention).

Generated by Claude Code

…errors

The runner copies coerceRow's cell errors onto the failed row as
ImportRowResultSchema.errors, beside the first-error keys, which keep
naming element 0. A validate-verdict row and a refused write carry none.

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

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/core, touching 7 documentable anchor(s).

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

  • content/docs/api/client-sdk.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/api/wire-format.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/data-modeling/fields.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/data-modeling/import-mappings.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it), /:object/import/jobs (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/permissions/system-context.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it), /:object/import/jobs (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/protocol/objectql/state-machine.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it), /:object/import/jobs (route, bridged from symbol runImport — its route source's handler names it))

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

  • content/docs/releases/v12.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/releases/v17/17-6.mdx (via runImport (symbol, a top-level function), /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/releases/v18/18-1.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))

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
  • the SDK route bridge reached 55 of 207 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 — 29 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 185512ccaf6394364a0da1af55cd8f1bfae37869 → packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 185512ccaf6394364a0da1af55cd8f1bfae37869

⚠️ 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 185512ccaf6394364a0da1af55cd8f1bfae37869 → 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: TIER_DEFAULT
Head-sha: 7a406058637b388f1fc1dfc13d7b50069570f497
Local-runs: none

Inputs read: card #25582 (body and all 11 comments, the ruling line 6377844879 letter A — proposals 1, 2, 4; not 3 — the spec seat's fill rule 6434667735, claim 6434841948, dev report 6436104459), PR #26543 (body, file list, net diff origin/main...7a406058 = 4 files, +351/-1), and the head's check-runs, read three times (2026-10-27T07:22Z, 2026-10-27T07:24Z, 2026-10-27T07:25Z). Cited code read with git show at the head; nothing built, run or re-run.

① Derived judgments

Diff: packages/core/src/utils/import-runner.ts (+14/-1), packages/core/src/utils/import-runner-cell-errors.test.ts (new), packages/rest/src/import-row-cell-errors.test.ts (new), .changeset/25582-import-row-every-cell-error.md (new). No packages/spec/**, no governed path.

  1. Accept-set: a failed import row whose cells did not coerce now carries errors — one { field, code, message } per failed cell, column order. RIGHT. The fill is the single coerceRow failure site in runImport (import-runner.ts:1230-1236), inside the per-row loop before the dry-run / commit split (:1282), so dry run and commit, sync route and async job all carry it. The map (errors.map picking field, code, message) projects FieldCoerceError (import-coerce.ts:149, exactly field: string; code: FieldErrorCode; message: string) onto the spec element; field is always set (coerceRow falls back to the column key, import-coerce.ts:892).
  2. Element-0 invariant (ImportRowResultSchema describe, export.zod.ts:410-425: field / code / error name element 0). RIGHT — the same first = errors[0] feeds the three first-error keys and element 0 of the list; the pre-change row object is a strict subset of the new one.
  3. Never on a validate-verdict row, a refused write, or a match-key row (fill rule 6434667735; spec describe "cell-coercion errors ... only source"). RIGHT — the validate-verdict row (:1301-1307, composeValidationMessage), toFailedResult / adoptDoorVerdict (:528, :570, :1217, :1362), DUPLICATE_MATCH_KEY / AMBIGUOUS_MATCH / BLANK_MATCH_KEY (:1238-1262) are all untouched and build no errors key. The pre-scan rowsSharingKey (:892) skips rows with coercion errors and reports nothing. Pinned by the two absence tests in each new file; the existing exact-key pin on refused-write rows (import-row-schema-drift-20701.test.ts:197, ROW_KEYS) stays true because those rows are toFailedResult rows.
  4. Wire parity: sync POST /data/:object/import and async GET /data/import/jobs/:jobId/results. RIGHT — data-action-routes.ts:409 returns summary.results verbatim; :611 stores capImportResults(summary.results) (a filter/slice of the same objects, rest-server.ts:954) in sys_import_job.results (Field.json), and :773-775 returns the stored items verbatim. No ImportRowResultSchema.parse on either path, and at this head the schema declares errors (feat(spec): ImportRowResultSchema declares errors, every cell error of a failed row #26454, 43714cae, an ancestor of origin/main), so a typed reader is not stripped either.
  5. Public surface: @objectstack/core ImportRowResult.errors?: ValidateDataIssue[] (import-runner.ts:96, exported via packages/core/src/index.ts:104). RIGHT, optional and additive. The element type is the spec's ValidateDataIssue (protocol.zod.ts:2456: field, code, message required, rule / severity optional), matching the spec element exactly; the runtime value never carries rule / severity, as the describe requires.
  6. Transitive surfaces the PR prose does not name — noted, not wrong. @objectstack/rest re-exports the core type unchanged (packages/rest/src/index.ts:38), and @objectstack/plugin-auth's IdentityImportRowResult extends ImportRowResult (admin-import-users.ts:361) with rows copied verbatim from runImport (:777, { ...r, row }), so the identity-import endpoint also reports errors on a cell-coercion failure. Same contract (same coerceRow, same fill rule), no schema on that path strips it, and no exact-key pin on it was found. The changeset's "every path that reports rows" is true but lists only the two data-import paths; a consumer reading the third one gets the declared shape. No separate changeset owed: the diff moves no file in either package (AGENTS.md step 3, "a package the diff moves").
  7. Card scope. RIGHT — Fixes #25582: proposals 1 and 2 landed in feat(core): import reads dotted and compact year-first days and matches option values in any case #25596 (9f390945e, card comment 6379902876), proposal 3 is explicitly not ruled (card body, ruling 6377844879), proposal 4 is this PR; the ruling's letter A is then whole. The gates The card this PR closes must claim this branch and Part-of PR must not also close its card are both success on the head; claim 6434841948 names claude/project-thread-sc5ydc.
  8. Nothing accepted before is refused now; no key changes value; ok rows and non-coercion failures are byte-identical to the base. The tests pin that (not.toHaveProperty('errors') on the good row and both absence paths).

Check-runs on 7a406058 at the last read (2026-10-27T07:25Z): 23 success, 8 skipped (opt-in / matrix stubs), 0 failure, 14 still in progress — Build Core, Build & E2E UI, Test Core 1/8..8/8, Type Check UI, Type Check · consumer gates / debt ledger / workspace. Success includes Check Changeset, Governed Surface Queue Guard, Lint · ESLint, Lint · PM dispatch-gates, Lint · CI release & engine gates, Lint · repo & tooling gates, Type Check · source gates, Dogfood Regression Gate, Flag docs affected, Check PR Size, both single-writer / same-issue guards. The verdict below stands on the diff and the card; the in-progress families are the merge queue's to wait on, and a red among them is a new fact this record does not pre-empt.

② Semver level

Changeset .changeset/25582-import-row-every-cell-error.md: '@objectstack/core': patch. RIGHT. The diff publishes one additive optional member on a released package's exported type plus a widening of its runtime output; Metadata-protocol: none is correct (nothing a metadata author writes changes), and under AGENTS.md step 3 none takes patch. Not skip-changeset (core publishes). No ADR-0087 marker owed (not breaking). The changeset body carries both declaration lines line-initial, the ruling ref, and the behaviour sentence a CHANGELOG reader needs.

Clause-②: yes (widening: a failed import row whose cells did not coerce gains the optional errors list ...) on the PR body and the changeset, byte-identical. RIGHT: the arm is widening, which scripts/pm/clause2-line.mjs readArmToken reads from (widening: (the token ends at :, not alphanumeric and not |); the parenthetical that follows is reasoning, not a second arm. Matches the measured diff — accept set grows, nothing narrows — and matches the claim's expectation (6434841948: "yes (widening) — expected: a new optional response member"). Check Changeset is success on the head.

③ Boundary flags

Dev report 6436104459, open_questions: [], out_of_scope_findings: []. Every deviation answered:

Implemented-by: claude/project-thread-sc5ydc
Reviewed-by: fable subagent of session_01R1q2xdtYgU6FKrZ7LgjPCC

VERDICT: PASS

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

席位核对 · ACCEPT · domain:server seat 1 · os-project-manager · session_01R1q2xdtYgU6FKrZ7LgjPCC · 2026-10-27T07:39Z

Head 7a40605863, report 6436104459 (on #25582), claim 6434841948, contract review 6436235757 (TIER_DEFAULT, declaration face, PASS on this head).

item reading
Shape Draft, base main, Fixes #25582. 4 files: packages/core/src/utils/import-runner.ts, a core test, a REST route test, and a patch changeset for @objectstack/core. Clause-②: yes (widening …) and Metadata-protocol: none appear in both the body and the changeset.
Fix Proposal 4 of ruling 6377844879 letter A, under the spec seat's fill rule 6434667735. The one coerceRow failure site copies every cell error into errors as { field, code, message }, in column order. The first-error keys stay as element 0. The site sits before the dry-run/commit split, so sync, async, dry run and commit all carry it. Validate-verdict rows, refused writes and match-key rows carry no errors, and tests pin that.
Contract Review 6436235757: the widening matches the landed spec member (#26454); patch is the right level. Noted, not wrong: plugin-auth's identity import copies runImport rows verbatim (admin-import-users.ts:777), so that endpoint reports errors too, under the same contract.
Evidence CI on head 7a40605863: 39 succeeded, 8 expected skips, none failed or pending; mergeable clean. Red first on main (core 3 red, route 4 red), and the ablation turns all 7 red. Derived gates: 61 of 61 exit 0, with --ran reconciled.
Card This lands the last ruled proposal (3 is unruled), so the card closes with Fixes.
Landing 「按照 pm-dispatch 规则复核过的都授权」 (Leehom, 2026-10-25T11:35Z): ready and auto-merge through the relay, once every check on this head is green or an expected skip.

Generated by Claude Code

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review October 27, 2026 07:41
@objectstack-fleet
objectstack-fleet Bot enabled auto-merge October 27, 2026 07:41
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Oct 27, 2026
Merged via the queue into main with commit 2f21b48 Oct 27, 2026
48 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/project-thread-sc5ydc branch October 27, 2026 08:01
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.

/import: common spreadsheet cell shapes fail whole rows (dotted / compact dates, option value case, one error per row)

2 participants