Skip to content

fix(rest): the import template stars a column only when the import refuses a blank in it - #20904

Merged
os-justin merged 3 commits into
mainfrom
claude/issue-18386-template-option-default
Sep 30, 2026
Merged

os-justin merged 3 commits into
mainfrom
claude/issue-18386-template-option-default

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Part of #18386

Clause-②: no

The import template now stars a column when a blank in it is refused on insert, as far as the metadata can tell. Every default the metadata declares counts, while a before* hook, requiredWhen, and a default that fails at insert time cannot be seen from it. isTemplateRequired used to read only defaultValue. The engine also fills a blank field from the option marked default: true (ObjectQL.resolveOptionDefault), treats defaultValue: null as no default, and never runs required on system, readonly or autonumber fields. Separately, each dropdown's errorTitle is now cut to Excel's 32 characters; its error text was already cut to 255.

Why the engine's rule is copied here, not called. resolveOptionDefault and applyFieldDefaults are private members of ObjectQL, and @objectstack/objectql is only a devDependency of @objectstack/rest. So templateInsertDefault copies the rule, and import-template-route.test.ts checks the copy against the engine. For 19 field shapes, the template's own header is starred exactly when a blank cell sent through the real import door is refused. When the blank is filled instead, the stored value is the default the copy predicts.

Pins.

  • A required select with a default: true option gets an unstarred header, and its instructions row says No. A blank cell imported through the xlsx import door stores the default.
  • A 46-character label produces an errorTitle of 32 characters.

Ablation. With the fix committed, the old predicate (def?.required === true && def.defaultValue === undefined) was put back through scripts/ablation-replace.mjs: anchor 1 → 0, blob c5f1888b → 24d5a851.

  • Result: 7 failed and 81 passed. The parity test named 11 shapes where the template and the engine disagreed.
  • Restore: the blob equals HEAD again and git diff HEAD is empty.

Local verification at daf0aa8 (after merging main):

  • @objectstack/rest full local test project: 238 files, 4664 passed, 106 skipped.
  • typecheck: green, with 0 debt in the test layer.
  • 60 derived gate commands: 58 exit 0. 2 are NOT MEASURED and left to CI: check:dual-build-cjs-loads and check:type-check-debt both need a whole-repo build (PREREQUISITE NOT MET).
  • eslint on the 3 changed .ts files: 0 errors, 0 warnings. The changeset is outside eslint's files globs. The config enables no type-aware linting, so this diff cannot change the lint result of any file it does not touch.

Acceptance notes

  • What a template cannot see. Metadata cannot tell the template about two things that fill or refuse a blank field: a before* hook that fills it, and an expression default that fails to evaluate at insert time (the engine then leaves the field unset and logs a warning). The template counts every default an author declared, if it is not empty, as filling the field.
  • Picklist fields. On main, nothing at runtime resolves a picklist reference into options yet, so the template's object read and the engine both see no options, and they agree. When resolution lands, it has to reach both, or a picklist option marked default: true would remove the star from a column the engine still refuses.
  • Truncation. The 32-character cut counts UTF-16 code units, like the existing 255 cut. A label whose 32nd unit falls inside a surrogate pair would keep half of that pair. No shipped label reaches that.

Generated by Claude Code

…nsert-time default

isTemplateRequired read only defaultValue, while the engine also fills a
blank field from the option marked default: true. Mirror the engine's rule
(templateInsertDefault) and pin the mirror against the real import door.
Cap each dropdown's errorTitle at Excel's 32 characters.

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

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

7 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 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 — 15 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 33b6e8bece79e70156dc2c1880813f8d88af61d4 → packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 33b6e8bece79e70156dc2c1880813f8d88af61d4

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

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

Contract review

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

PR #20904 (draft, Part of #18386), head daf0aa882a = merge of origin/main 33b6e8bece into cc2615825f; merge base with main is 33b6e8bece, so the net diff is exactly the PR's four files: packages/rest/src/import-template.ts (+87/-7), packages/rest/src/import-template.test.ts (+90), packages/rest/src/import-template-route.test.ts (+124), .changeset/18386-template-required-option-default.md (+12). Inputs read: the card body and all 20 comments (readings 5914688255, claim 5914714137, dev report 5915980985, seat ruling 5916009192), the PR body and file list, the net diff, the engine and validator at this head, and the head's check-runs. Nothing built, run or re-run.

① Derived judgments

The predicate, judged case by case against the engine at this head. isTemplateRequired (import-template.ts) now answers: not required === true → false; system === true / readonly === true / type === 'autonumber' → false; otherwise starred exactly when templateInsertDefault(def) is empty for required. Each leg against the engine:

  • defaultValue present and not null wins — RIGHT. ObjectQL.applyFieldDefaults (engine.ts:5789) gates on f.defaultValue == null, so '' is a real default and null is none; the template's def.defaultValue != null is the same test.
  • defaultValue: null → falls through to the option list — RIGHT (same gate; parity shape default_value_null refused, default_value_null_option_default accepted and stored c).
  • option default: true on a single-valued field → first marked value; on a multi-valued field → every marked value as an array — RIGHT. resolveOptionDefault (engine.ts:5733) reads only the canonical default === true, skips an option whose value is undefined or null, ignores non-object options, and returns marked or marked[0] by declaredMultiValued (engine.ts:1441), which is isMultiValueField({ type, multiple: multiple === true }) from @objectstack/spec/data. The template's templateInsertDefault calls the same spec predicate with the same two-key shape, so multiselect, checkboxes, tags and select + multiple: true agree by construction; option_default_select_multiple stores ['b'], option_default_multiselect stores ['a','c'], option_default_first_wins stores a.
  • string options (options: ['a','b']) declare no default — RIGHT (both sides continue on a non-object entry).
  • option default on a non-option type (text with options) fills — RIGHT; the engine is deliberately type-agnostic (its own docblock), and the parity shape option_default_any_type pins it.
  • '' default, and [] default on a multi-valued field, stay starred — RIGHT. The validator's isEmptyForRequired (record-validator.ts:301) is isMissing (undefined, null, or a string whose trim() is empty) or, for isMultiValueField(def), an empty array; the template's private copy is the same three-clause test, trim included. A '' default on a non-text stored type reaches null through normalizeBlankTypedValues (record-validator.ts:642) and is refused the same way.
  • system / readonly / autonumber never starred — RIGHT. validateRecord in insert mode skips def.system || def.readonly (record-validator.ts:1706) and validateOne exempts autonumber from the required check (record-validator.ts:885). Without ?fields=, templateColumns already drops all three through TEMPLATE_COLUMN_EXCLUSIONS, so the exemption is reachable only through an explicit ?fields= list — exactly what the code comment says, and what makes the instructions sentence hold for a starred ?fields= column (before this PR such a column was starred while the import accepted its blank).
  • Strictness: the template tests === true where the validator tests truthiness (def.system || def.readonly, def.required). Equal for Zod-parsed registry metadata, where these keys are booleans; recorded as a spelling difference, not a drift.
  • The schema the predicate reads is the registry object after translateMetaItem → translateMetadataDocument → translateObject (i18n-resolver.ts:2767), which copies each field as { ...def } and each option as { ...opt, label }: defaultValue, required, system, readonly, type, multiple and every option's default survive a zh-CN request. RIGHT — no localization drift.

What metadata cannot see (the converse). "Starred exactly when the import refuses a blank" holds in the starred ⇒ refused direction for every metadata-declared default; the refused ⇒ starred direction has four exceptions, none introduced by this PR: (a) a before* hook that fills the field (starred, then accepted); (b) an expression default that fails to evaluate (engine leaves it unset with a warn, then refuses); (c) a current_user token default with no acting user (if (execCtx?.userId != null) at engine.ts, else unset, then refused); (d) a requiredWhen rule that holds at insert. The PR body's Acceptance notes name (a) and (b); (c) and (d) are unnamed. The load-bearing author-shown sentence claims only the first direction (below), so this is an over-breadth in the PR body and in the isTemplateRequired docblock ("exactly when"), true for static required: true plus declared defaults, on main when this lands.

The parity test's power. import-template-route.test.ts registers 19 objects (pty_*), reads each template through the real route with ?fields=f, asserts the header star, the instructions D5 Yes/No and isTemplateRequired agree, then sends { f: '' } through the real POST /data/:object/import, requires code: 'required' on refusal, and on acceptance requires the stored value to equal templateInsertDefault's prediction. The 19 shapes cover every leg above (plain, defaultValue, false, option default on select / first-wins / multiselect / select+multiple / non-option type, no marked option, marked option without value, non-canonical 'true' flag, null, null+option, defaultValue beats option, '', [], readonly, system, autonumber). It is a property test through the door the copy is copying, so a change on either side that moves any of these 19 verdicts is caught. The ablation numbers in the PR body (7 red / 81 green, 11 disagreements) are the dev's, not re-run here; they are internally consistent (11 = 5 option-default shapes + null/''/[] + readonly/system/autonumber). Not covered: a picklist-bound field (no options at this head, see ③), and the four converse cases above, which no metadata-only test can express.

errorTitle cut. errorTitle: c.header.slice(0, 32), error: c.howToFill.slice(0, 255) via two new constants — RIGHT: 32 and 255 are Excel's data-validation dialog limits, the error cut existed before as a literal 255. The unit test pins a 46-character label to a 32-character title and a short title written whole. slice counts UTF-16 code units, as the PR body says.

Instructions sheet, each sentence about starred columns against the new predicate.

  • EN "Columns marked * are required: a row that leaves one of them blank is refused." / ZH "带 * 的列为必填:其中任一列留空的行会被拒绝。" — TRUE for every starred column under the new predicate (the 19-shape parity is starred ⇒ refused with no disagreement); the one way it can fail is a before* hook that fills the field, which no metadata declares. This is what the machine verification 5914688255 found false on showcase_task.status (required, backlog marked default: true — confirmed at examples/app-showcase/src/data/objects/task.object.ts:44-51); with this diff that column is unstarred and the sentence holds.
  • Required column Yes/No (是/否) is c.required ? text.required : text.optional, and c.required is the same predicate — the row for a required select with a marked option says No, pinned in both locales by the describeTemplateColumns test and at D6 by the route test.
  • The example-row note and the read-projection note are untouched and unaffected.

Changeset .changeset/18386-template-required-option-default.md, sentence by sentence. Title (" * only when the import refuses a row that leaves it blank"): true in the direction it claims, hook caveat aside. Bullet 1 (option default: true → no star, instructions say not required, blank imports as the marked option): TRUE, pinned by the TICKET route test (['Title *','Status'], D6 = No, stored backlog). Bullet 2 (defaultValue: null now starred unless an option is marked; '' and [] on a multi-valued field now starred): TRUE, and "now" is right — the old predicate defaultValue === undefined left all three unstarred. Bullet 3 (system / readonly / autonumber named in ?fields= no longer starred): TRUE. Bullet 4 (title cut to 32, Excel's longest): TRUE. Clause-②: no: right, see ②.

PR body, remaining sentences. "isTemplateRequired used to read only defaultValue" TRUE. "treats defaultValue: null as no default, and never runs required on system, readonly or autonumber fields" TRUE (for autonumber the required clause is exempted inside validateOne). "private members of ObjectQL" TRUE (both declared private); "@objectstack/objectql is only a devDependency of @objectstack/rest" TRUE (packages/rest/package.json line 45 under devDependencies at line 41; import-template.ts is not re-exported from src/index.ts, and the package's only exports entry is .). "19 field shapes" TRUE. Pins TRUE. Local verification, gate counts and the eslint reasoning are the dev's claims; the head's check-runs are the verdict on them. Acceptance note on picklists: TRUE at this head — outside packages/spec no source file names picklist (only an engine comment), and the resolver's own comment says a picklist-bound field "is served with its list's options resolved onto it" while resolving nothing itself, so template and engine both see no options today; when resolution lands it must reach both, as the note says. UTF-16 note TRUE.

Public surface. Three new exports and two new optional keys on TemplateFieldDef are package-internal (not on the entry); no query parameter, header, status or error code moves; the fixed behaviour belongs to the template=true feature whose minor changeset is still unreleased on origin/main. No accept set moves.

② Semver level

@objectstack/rest patch, Clause-②: no — matches what the diff publishes: a bug fix inside an unreleased feature of a released package, no export added to the package entry, no surface widened or narrowed. The PR body carries the same Clause-②: no line. Right level.

Clause-②: no

③ Boundary flags

Dev report 5915980985 (os-dev-report, status done):

  • Deviation — the system / readonly / autonumber exemption goes one step past the claim's wording. ANSWERED: right, and required — without it a required ?fields= column of those kinds is starred while the import accepts its blank, which is the exact defect class the round fixes; the parity shapes readonly, system, autonumber measure it. The seat accepted it in 5916009192; this review agrees on its own reading of validateRecord.
  • open_questions[0] — the unreleased minor changeset .changeset/18386-export-import-template.md says " * after each field that is required and has no default value". ANSWERED: A (leave it), with two edge cases recorded where that sentence is loose: a required field with defaultValue: '' (or [] on a multi-valued field) has a default value and is starred; a required system / readonly / autonumber field named in ?fields= has no default value and is not starred. Both changesets sit in .changeset/ on origin/main and compile into the same release, and the patch entry states each case; the file is outside this PR's surface. No edit owed here.
  • out_of_scope_findings[0] — picklist resolution must reach both the template's object read and the engine's schema. Verified true at this head (no runtime resolution exists); the seat's 5916009192 routes it to picklist metadata kind — runtime: resolve picklist → options when serving field metadata, validate writes against the resolved set, apply package-level extensions (phase 1 of objectstack#18164) #19519 by pointer. ESCALATED by that pointer; the parity test is the right place for a picklist shape.
  • out_of_scope_findings[1] — before* hooks and failing expression defaults. ANSWERED: true; add to the same class a current_user default with no acting user (engine leaves it unset) and a requiredWhen rule — the instructions sentence claims only starred ⇒ refused, so none of these falsifies it.
  • out_of_scope_findings[2] — UTF-16 surrogate at the 32nd unit. ANSWERED: true, unreachable by any shipped label; noted.

Card thread: the machine-verification risks 2 (showcase grants nobody allowExport, so no one can download the template there) and 3 (a zh-CN template imported under an en request answers invalid_option) in 5914688255 are outside this PR's file surface and were routed by the seat to a decision card and to objectui#9600; nothing in this diff changes either. The claim 5914714137 scope items 1-3 are each delivered and pinned. Acceptance 6 remains the re-run of the machine verification after landing, per the maintainer's 「A,机器验证通过就直接关卡」.

Check-runs on daf0aa882a, read last, 38 runs deduped to 34 names keeping the newest started_at: 29 success, 5 skipped (Auto Label, Build Docs, Check PR Size, Console Pin Gate, Packed-tarball smoke (opt-in)), 0 failed, none still running. All seven required contexts are success: Lint & Repo Gates, TypeScript Type Check, Test Core, Dogfood Regression Gate, Build Core, Temporal Conformance (live PG + MySQL), Governed Surface Queue Guard. Check Changeset and Part-of PR must not also close its card are success. Not governed (no governed path in the file list); 320 changed lines.

Implemented-by: claude/issue-18386-template-option-default
Reviewed-by: session_01Sfe5YjBLwB9J3y8fvm2xq1

VERDICT: PASS

Adopted and posted by domain:spec seat 5 (session_01Sfe5YjBLwB9J3y8fvm2xq1) · 2026-09-30T17:26Z · rendered by the seat's at-tier review subagent on this head. The seat read its served tier family from the subagent transcript before posting.

  • "exactly when": the seat rewrote the PR body's opening sentence so it holds as far as the metadata can tell, and names what the template cannot see. The isTemplateRequired docblock keeps its wording, because it is package-internal and is bounded by the same caveat the PR body now states. There is no new head.
  • Not governed. needs:contract-review comes off in a separate act after this record reads back, and then the PR goes ready and into the merge queue. After landing, the feat(rest): 导出接口新增 ?template=true —— 输出只含「可填列」的 xlsx 导入模板 #18386 machine verification runs again, and the card closes on a pass per the maintainer's 「A,机器验证通过就直接关卡」.

Generated by Claude Code

@os-justin
os-justin marked this pull request as ready for review September 30, 2026 17:28
@os-justin
os-justin enabled auto-merge September 30, 2026 17:28
@os-justin
os-justin added this pull request to the merge queue Sep 30, 2026
Merged via the queue into main with commit 6f1f1c1 Sep 30, 2026
50 checks passed
@os-justin
os-justin deleted the claude/issue-18386-template-option-default branch September 30, 2026 17:53
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.

2 participants