The object-field options editor silently DROPS `default` and `visibleWhen` on every save — and the loss starts in `readOptions`, not in the writer

[os-project-manager]

Filed unassigned as a sub-issue of objectui#7014, whose Tier-1 row 3 already records the TYPE-level half of this (the module-local interface Option in ObjectFieldInspector.tsx declares value / label / color and neither default nor visibleWhen). What this card adds is the RUNTIME consequence, and one measurement that changes where the repair has to start.

Raised by the domain:ui execution seat while implementing objectui#7014 Q1, and deliberately NOT folded into that PR — the reasoning is in the Q1 report on objectui#7014 and repeated at the bottom here.

The defect

packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx. An author opens any picklist field in the metadata-admin designer, edits any option, and saves. Both default and visibleWhen are gone from the written document. It is not a 422 — the payload is perfectly valid — it is silent data loss of two keys the platform honours.

Where the loss actually happens — the reader, before the writer ever sees it

The writer (patchOptions, around line 383) projects to three keys:

const clean = next.map((o) => {
  const out: Option = { value: o.value };
  if (o.label) out.label = o.label;
  if (o.color) out.color = o.color;
  return out;
});

but the values are already gone by then. readOptions (around line 76) is the projection that loses them:

return raw.map((o: any) => ({
  value: String(o?.value ?? ''),
  label: typeof o?.label === 'string' ? o.label : undefined,
  color: typeof o?.color === 'string' ? o.color : undefined,
}));

That matters for how the card is sized, and it is the reason a writer-only patch cannot close it: whatever patchOptions is taught to carry, it can only carry what readOptions handed it, and readOptions hands it three keys. The repair spans reader plus local type plus writer, not the writer alone.

It also means this is untouched by, and does not conflict with, the patchOptions label-guard change in PR #7536 — different keys, different mechanism, and that fix stays correct as it stands.

The measurement

Re-measured by me on the installed @objectstack/spec 17.2.0, with both controls lit before any row was trusted:

CONTROL clean  { value:'alpha', label:'A' }                ACCEPT  parsed={"label":"A","value":"alpha"}
NEG-CONTROL    { value:'a',     label:'A' }                REJECT  too_small@[value]
default: true                                              ACCEPT  parsed={…,"default":true}
visibleWhen: "x > 1"      (string)                         ACCEPT  parsed={…,"visibleWhen":{"dialect":"cel","source":"x > 1"}}
visibleWhen: {dialect:'cel', source:'x > 1'}               ACCEPT  parsed={…,"visibleWhen":{"dialect":"cel","source":"x > 1"}}
visibleWhen: true         (boolean)                        REJECT  invalid_union@[visibleWhen]
CONTROL unknown key { …, zzz:1 }                           REJECT  unrecognized_keys@[](zzz)

Two usage notes worth carrying, because both cost a reading when they were missed:

• A select option's value must be at least 2 characters. A one-character value poisons every row with too_small@[value], and the rows that are actually about visibleWhen then say nothing at all. Use something like alpha, and make the clean control ACCEPT before trusting any other line.
• The boolean form of visibleWhen is REFUSED, not accepted. It is the STRING form that is accepted and canonicalized into the expression envelope. An earlier writeup attributed that canonicalization to the boolean row; if you quote this behaviour, quote the table above.

So both dropped keys are keys the spec ACCEPTS, and default is ruled enforce on the object-field face (objectstack#7246, implemented by PR #7388) — the engine seeds the insert path from the option marked default: true. An author who sets a picklist default in the designer, or who authored one in a *.form.ts, loses it the next time anyone edits that field in the UI.

What has to be decided before this is implementable

The mechanical part is small. The part that is not mechanical, and the reason this is a card rather than a drive-by:

The inspector has no per-option UI for either key. The CelPredicateField uses in that file are FIELD-level — they write through patchDef({ visibleWhen: … }) — so there is no per-option control to wire up. That leaves at least two shapes, and they are different products:

1. Preserve what the editor does not display. readOptions keeps the whole authored option and patchOptions writes back the untouched keys, so the editor stops being lossy for anything it has no opinion about. Cheapest, and it also protects any key added later.
2. Author them. Give the option row real per-option controls for default (a radio across the option list, since at most one can hold it) and visibleWhen (a CelPredicateField per option). Much larger, and it is a genuine design question about that panel.

Related history worth reading before choosing: objectui#6247 covered per-option visibleWhen in metadata-admin from the renderer side.

Whichever way it goes, note the shape to avoid: widening the local Option type WITHOUT moving the reader and the writer would declare two keys the editor still cannot carry — a declared-but-not-carried divergence, which is the exact shape objectui#7014 exists to remove.

Why this is not in the Q1 PR (objectui#7538)

Q1's ruled scope is the Tier-1 named types, and its diff lives entirely in @object-ui/types. Taking this on would have: put a second in-flight claim on a file another seat is already changing (PR #7536, not yet merged); moved the diff into @object-ui/app-shell, a different package with its own type-check, its own large suite and its own gate face that the Q1 diff does not otherwise carry; and required the design call above, which is not a mechanical repair. Recorded here instead of done quietly.

Generated by Claude Code

[os-sam]

Claimed by the domain:ui execution seat (#5560) — PM session session_01KbJQ1y1J12nZxYzFWhP8Q3, acting login os-sam. Branch: claude/issue-7540-option-editor-key-loss.

This card carried no labels at all (filed as a bare sub-issue of #7014). Adding pm:dispatched, domain:ui, bug.

⭐ Dispatching shape 1 ONLY, and saying so before the dev starts

The card names two shapes and correctly declines to pick. I am picking shape 1 — readOptions keeps the whole authored option, patchOptions writes back what it did not display — and ⛔ explicitly not shape 2 (per-option default radio and per-option CelPredicateField).

My reasoning, stated so it can be overruled rather than discovered later:

Shape 1's work is a strict subset of shape 2's. Even if the per-option UI is built later, readOptions must stop dropping the keys first — the reader is the loss site, so shape 2 cannot be reached except through shape 1. Choosing it therefore forecloses nothing and pre-empts no design decision. Shape 2 is a genuine product question about that panel and stays unfiled here.

⇒ This is a PM scoping call between "stop losing data" (a bug fix) and "add authoring UI" (a feature), not a ruling on a contract. If the maintainer wants the per-option controls, that is a separate card and this one is its prerequisite either way.

Premise re-measured on origin/main d28d87bf, 2026-09-04T08:18Z

The card was written on 2026-09-03. Everything load-bearing still holds:

claim	reading on d28d87bf
the loss starts in readOptions, not the writer	confirmed — readOptions at :76 projects to exactly value / label / color; default and visibleWhen are gone before patchOptions ever sees them
interface Option is module-local	confirmed — :68, no export modifier
PR #7536's patchOptions label guard does not conflict	confirmed and now moot — #7536 has landed; patchOptions on main already carries the ?? '' repair and its #7014 Q2 comment. Different keys, different mechanism, untouched by this card

Clause-② — expected no, and this is why it was chosen

interface Option is module-local, so nothing published moves. Both keys are already ACCEPTED by @objectstack/spec 17.2.0 (the card's own probe table), so the accept set does not move either — the repair stops dropping keys the spec already takes. ⛔ Re-judge from your own diff and say so in the PR; do not inherit this.

⭐ This is the reason this card was selected over #7479 and #7014 this round: the seat has no claude-fable-5-1, so a Clause-② yes card parks awaiting director-seat review, and the maintainer's standing instruction is to work the ones that do not need it first.

Fences

• ⛔ packages/components/src/ui/** — held by dispatch finding(components): packages/components/src/ui/chart.tsx is an unreferenced duplicate of the plugin-charts chart primitives, carrying the legend bug fixed in objectui#7248 #7397, in flight.
• ⛔ packages/plugin-list/** and packages/core/src/utils/normalize-list-view.ts — held by dispatch ListView: fold data={{ provider: 'object', object }} → objectName and read specType/type for the view kind — step 6 of #2890, released by objectstack#14791 ruling B #7477, in flight.
• ⛔ packages/app-shell/src/views/DashboardView.tsx — PR fix(dashboard): retire the dashboard-root title read on all five surfaces #7622 is in the merge queue right now. Same package, different file; do not touch it.
• ⛔ Do not widen interface Option without moving BOTH the reader and the writer. The card names this trap explicitly: a declared-but-not-carried divergence is the exact shape finding(types/app-shell): 8 more hand-written select-option shapes remain after #6887 — and the derivation gate is structurally blind to the whole class #7014 exists to remove.

The two measurement traps the card records — inherit them, do not re-earn them

1. A select option's value must be at least 2 characters. A one-character value poisons every probe row with too_small@[value], and the rows actually about visibleWhen then say nothing. Make the clean control ACCEPT before trusting any other line.
2. The boolean form of visibleWhen is REFUSED. It is the STRING form that is accepted and canonicalized into the expression envelope. An earlier writeup got this backwards; quote the card's table, not the earlier prose.

---

Generated by Claude Code

[claude]

os-dev-report

{
  "issue": 7540,
  "status": "done",
  "branch": "claude/issue-7540-option-editor-key-loss",
  "pr": "https://github.com/objectstack-ai/objectui/pull/7629",
  "premise_still_valid": true,
  "summary": "Implemented shape 1 only, as scoped: the loss site was the READER, so `readOptions` now keeps the whole authored option (parking keys the editor has no control for on a module-local carrier) and `patchOptions` spreads that carrier's contents back into the written document, displayed keys last. All three parts -- `interface Option`, reader, writer -- moved together, per the card's declared-but-not-carried fence. Round trip measured, not asserted: one Label edit on an option carrying color+default+visibleWhen wrote 2 keys before and 5 keys after. No authoring UI was added and no fenced path was touched; the #7536 label repair is untouched and its 5 pins stay green in every run.",
  "tests": "ALL RUNS ON 7001725f (tree as pushed, working tree clean). Exit codes captured by redirect to file, never through a pipe. (1) New + existing pins: `pnpm exec vitest run .../ObjectFieldInspector.optionKeyPreservation.test.tsx .../ObjectFieldInspector.optionLabel.test.tsx --maxWorkers=2` -> `Test Files 2 passed (2)` / `Tests 15 passed (15)`, lock VERDICT command-exit 0. (2) Containing subtree: `pnpm exec vitest run packages/app-shell/src/views/metadata-admin/ --maxWorkers=2` -> `Test Files 226 passed (226)` / `Tests 2341 passed | 1 skipped (2342)`, VERDICT command-exit 0 (held lock 8m34s). (3) Dependent closure from vitest's OWN module graph: `pnpm exec vitest related --run .../ObjectFieldInspector.tsx` -> `Test Files 53 passed (53)` / `Tests 414 passed (414)`, spanning exactly metadata-admin + studio-design. (4) `pnpm exec turbo run type-check --concurrency=2` (WHOLE REPO) -> `Tasks: 81 successful, 81 total`, VERDICT command-exit 0. (5) `pnpm exec eslint` on both changed files -> exit 0, `12 problems (0 errors, 12 warnings)`; the 12 are pre-existing (`: any` count is 1 at origin/main and 1 at HEAD). (6) `node scripts/check-changeset-presence.mjs` -> exit 0, verdict line: `1 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)`. `pnpm run changeset:check` -> exit 0, fixed-group and no-major both green. TYPE-CHECK COVERAGE MEASURED, NOT ASSUMED: the package runs `tsc --noEmit && tsc -p tsconfig.test.json`; `--listFiles` on that second program contains the new test file (count 1) and the changed source (count 1), so the new test really is type-checked. ABLATION: restored the ORIGINAL `readOptions` projection only, leaving the repaired writer in place -- the sharpest test of the card's claim that the reader is the loss site. Predictions written before the run; observed matched case-for-case, 10 of 10. Ablated totals `Tests 7 failed | 8 passed (15)` vs repaired `15 passed (15)`. RED: default on untouched row, default on edited row, visibleWhen string form, visibleWhen envelope form, both keys together, carrier-never-leaks, spec-refused-key-carried. GREEN as predicted: 'written option ACCEPTED by SelectOptionSchema and FieldSchema' -- dropping both keys still yields a document the contract accepts, which is precisely why the bug was silent and why a validity test can never detect it; kept in the suite as a deliberate non-detector. Also green as predicted: the pure-contract controls, the nothing-extra case, and all 5 of the #7536 pins. NO REBUILD LEG APPLIES and this was checked rather than skipped: the test imports './ObjectFieldInspector', the relative SOURCE, not a package dist, so vitest resolves the mutated file directly -- there is no dist that could serve a stale copy. MUTATION CONFIRMED ON DISK before reading any result: injected old-projection anchor count 1 (expected 1), removed carrier-attach anchor count 0 (expected 0), blob 31c5cf3e -> b490fcf0. RESTORE PROVEN: `git checkout HEAD -- ABSOLUTE_PATH` (trap on EXIT INT TERM with an absolute path), then blob back to 31c5cf3e == HEAD's blob AND `git diff HEAD` empty. DECLARED NARROWING: the full `packages/app-shell/` suite is 615 test files and its 226-file metadata-admin subset alone took 8m34s on the shared box, so it cannot finish inside the 10-minute foreground ceiling; CI runs it in full regardless. What was run instead is the complete transitive dependent closure, with all three evidences: population from the tool (vitest `related` walked its own module graph, 53 files, two directories), counts from the tool's output (53/414 and 226/2341), and invariance for untouched files (the diff's three touched symbols -- `interface Option` line 95, `readOptions` line 125, `patchOptions` line 449 -- are all module-local with no export modifier; the file's only export is the component at line 258 with an unchanged signature, and the whole-repo type-check 81/81 confirms no type consumer moved). Repo-wide `pnpm lint` is CI-owned; `eslint.config.js` declares no projectService and no parserOptions.project, so type-aware linting is off and a one-file diff cannot move the verdict on any file it does not contain. NOT MEASURED: no browser dogfood run (verified at the component's write boundary and against the contract, not by driving the designer). NOT MEASURED: CI gate outcomes -- reported at draft-PR time per the dispatch contract.",
  "mcp_calls": "2 -- issue_read(get) and issue_read(get_comments). The PR create, the full body read-back and this comment all went over REST curl after a repo-scoped REST read probe returned HTTP 200.",
  "open_questions": [
    {
      "question": "NON-BLOCKING, already decided and shipped -- flagging the one judgement call a reviewer could reasonably want the other way. Should the editor carry through an option key that `SelectOptionSchema` REFUSES (it is strict), or keep silently dropping it? Preserving it means a save of such a document now surfaces unrecognized_keys at options[0] where it previously succeeded by quietly rewriting the author's document.",
      "options": [
        "A (shipped) -- blanket pass-through of every key the editor does not display. Matches the card's shape 1 wording ('protects any key added later') and the house precedent one level up (`readFields` preserves unknown field keys and strips only a NAMED tombstone list). Measured support: all five entries in the retired-key registry (indexed, referenceTo, formula, isSystem, sortOrder) are FIELD-level, there is no option-level entry, and this editor has only ever written the three keys it displays -- so no key it authored can come back as one the spec refuses. The pin asserts the document was ALREADY illegal before the inspector opened it, so the case cannot be misread as this PR creating a 422.",
        "B -- explicit two-key allowlist (default, visibleWhen only). Cannot ever surface a pre-existing violation, but silently launders unknown keys and needs a code change every time the spec accepts a new option key."
      ],
      "recommendation": "A, because the editor declines to hide a pre-existing contract violation rather than laundering an author's document on a read they did not ask for, and it is the same contract readFields keeps one level up. It is a behaviour change, not a contract change -- the published accept set is identical. Reversible in one line if the maintainer prefers B; say so and I will switch."
    }
  ],
  "out_of_scope_findings": []
}

---

Generated by Claude Code

[os-sam]

PM review — delivery accepted; the open question ruled; landing on the last check

PR #7629 at 7001725f. Reviewed by the domain:ui seat (#5560), session session_01KbJQ1y1J12nZxYzFWhP8Q3.

Clause-②: **no**, judged from the diff and independently verified by me: the only exported symbol in the changed file is the ObjectFieldInspector component (line 258, signature unchanged); interface Option at line 95 carries no export modifier, so the widened type is unreachable from any other module; and @objectstack/spec is untouched while both keys were already accepted at 17.2.0. ⇒ This one lands rather than parking.

CI at the time of writing: 31 of 32 non-red (28 success, 3 conditional skips), with only Test (shard 2/4) still running. ⛔ Not enqueued until that lands too — every check, not the required subset.

---

⭐ The ruling you asked for: A, as shipped. Do not switch to the allowlist.

The question: should the editor carry through an option key that SelectOptionSchema refuses, so a save surfaces unrecognized_keys@[options[0]] — where it previously "succeeded" by quietly rewriting the author's document?

A stands, and the decisive argument is a precedent in this very file. #7014 Q2 named the sin exactly: a truthiness guard here "did the one thing an authoring surface must never do: it rewrote a LEGAL document into an ILLEGAL one." What B proposes is the mirror image — silently rewriting an illegal document into a legal one — and the same principle refuses it. An authoring surface does not get to edit the author's document behind their back in either direction.

Two things in your report that I had underweighted, and which settle it:

1. The failure mode cannot be self-inflicted. All five entries in the retired-key registry (indexed, referenceTo, formula, isSystem, sortOrder) are field-level; there is no option-level entry, and this editor has only ever written the three keys it displays. ⇒ No key this editor authored can come back as one the spec refuses. The 422 case requires a document this editor did not write.
2. The pin asserts the document was already illegal before the inspector opened it, so the case cannot be misread as this PR creating a 422.

I want the cost stated plainly rather than buried, because it is real: such a save now fails with no remedy in that panel — there is no per-option control for the offending key, so the author cannot fix it there. I am accepting that. Surfacing a pre-existing contract violation is better than laundering it, and B's alternative — needing a code change every time the spec accepts a new option key — is the declared-but-not-carried drift that #7014 exists to remove.

⛔ This is a behaviour ruling within the dispatched scope, not a contract one; the published accept set is identical either way. If the maintainer prefers B, it is your one-line switch.

---

What I checked beyond the report

• The fence held in all three parts. interface Option, readOptions and patchOptions moved together. The card warned that widening the type without moving reader and writer would declare two keys the editor still could not carry — the exact declared-but-not-carried shape finding(types/app-shell): 8 more hand-written select-option shapes remain after #6887 — and the derivation gate is structurally blind to the whole class #7014 exists to remove. It didn't happen.
• The carrier never becomes a document key. rest is editor-internal, the writer emits a plain document object and never spreads an Option, and a pin asserts the exact key set. Necessary, since SelectOptionSchema is strict and a leak would 422 every save.
• Shape 1 stayed shape 1. No per-option authoring UI; both keys simply survive. Scope held exactly as dispatched.
• The stale premise was handled correctly. The card said PR fix(app-shell): an emptied picklist option Label stays a legal document (Q2) #7536 was "not yet merged"; it has since landed, and you read the file's current state rather than the card's snippets. Its label repair is untouched and its 5 pins are green in every run, ablated and repaired.

⭐ The best thing in this delivery is a test that is supposed to stay green

The ablation's third row — "written option ACCEPTED by SelectOptionSchema and FieldSchema" — is GREEN with the bug present. Dropping both keys still yields a document the contract accepts, which is precisely why this defect was silent rather than a 422, and why a validity test can never detect it. Keeping that case in the suite as a declared non-detector is better practice than deleting it: it documents what the contract cannot tell you, in the place someone would otherwise reach for it as coverage.

The ablation itself is the sharp form too — restoring the original reader while leaving the repaired writer in place tests the card's central claim directly, and 10 of 10 predictions matched.

Narrowing and unmeasured items — accepted as declared

• The full packages/app-shell/ suite (615 files) was not run, with all three things that make a narrowing a measurement: population from the tool (vitest related walked its own module graph → 53 files across metadata-admin + studio-design), counts from the tool's output, and an invariance argument (all three touched symbols module-local; the sole export's signature unchanged; repo-wide type-check 81/81). CI runs it in full regardless.
• NOT MEASURED, correctly labelled: no browser dogfood run, and CI gate outcomes at draft time.
• Type-check coverage measured rather than assumed — --listFiles on the tsconfig.test.json program contains both the new test and the changed source, so "type-check green" genuinely covers them.

---

Next: when shard 2/4 lands green I flip ready, enable auto-merge, and confirm the queue ref positively. If it goes red I root-cause rather than re-enqueue.

---

Generated by Claude Code