fix(web): six create paths ask the status field's type before defaulting it (BUG-3078) (#1360)

Six create paths independently read `statusField.options[0]` into a new item's
`status` with no type check. `options` survives a retype — the premise BUG-3057,
BUG-3067, BUG-3068 and BUG-3074 all turn on — so on a collection whose `status`
was retyped to `multi_select`, `number` or `checkbox` while keeping its options,
every one of them sent a scalar into a field that cannot hold it and the server
refused the create. Not a degraded create: no item, an error, and no route to a
good outcome. Quick capture simply stopped working on that collection.

The sites were QuickCaptureSheet, the Sidebar quick-add, EditorBubbleMenu, and
the collection page's three create paths. One of the six sits INSIDE the
function BUG-3057 fixed, six lines above its `laneWriteValue` call: the
conversion landed on the lane value and not on the status default directly
above it.

All six now call one `createDefaultFields`, which routes the default through
`laneWriteValue` — the same converter the drag and quick-create paths use, so
this cannot drift from them. `createDefaultCallers.test.ts` pins that there is
one copy, in the style `laneKeyCallers.test.ts` already uses for the sibling
question about a lane key.

CONVERT WHERE POSSIBLE, OMIT ONLY WHERE NOT, and the asymmetry is measured
rather than assumed. Against the real create handler, on a retyped collection
whose stale scalar default also survived:

    supplied  {"status":"open"}    -> 400, "must be an array of strings"
    omitted   {}                   -> 201, stored {"status":"open"}
    converted {"status":["open"]}  -> 201, stored {"status":["open"]}
    omitted, no stale default      -> 201, stored {}

The omit arm lands but stores an unchecked scalar, because
`ValidateFieldsDetailed` assigns a schema default and continues past
`validateFieldType`. So omitting everywhere would have traded a loud refusal for
a silent corrupt write on exactly the collections this bug is about. Converting
is the only arm that both lands and stores a correctly-typed value; omit is kept
for `number` and `checkbox`, where no conversion exists and the alternative is
no item at all. That server-side hole is filed separately — it is reachable
today by any caller that creates without a status, so this routes onto an
existing path rather than opening one.

Codex round 1 found that `multi_relation` still went out as a scalar:
`laneWriteValue` passes a relation key straight through BY DESIGN, because a
lane key for a relation is already the target's item id. This caller holds no
lane key — it holds a leftover `select` option, which is never an id — so the
same string is a good value there and a bad one here, and the refusal belongs
in this module rather than in `laneWriteValue`, which would break the lane path
it was written for. `relation` is the same class and was found by asking what
else reaches that branch; both refusals were measured (400 `"open" does not
name an item in collection "tasks"`, and 400 `must be an array of strings`).

Evidence: 19 tests over the four arms and the six-caller guard. Counterfactual:
reverting the six callers to origin/main fails all eight guard assertions and
leaves both of its controls green. Mutation 6/6 killed, none build-broken — including a mutant that fixes only
the `multi_relation` codex named and leaves `relation` — and
one of them, dropping the `ok` check, SURVIVED the first version of these tests:
`laneWriteValue`'s refusal carries no `value`, so the mutant assigned `undefined`
and `toEqual({})` treats an undefined-valued key as absent. The omit cases now
assert key ABSENCE via `Object.keys`, which is the contract they meant all along. The guard keys
on the assignment shape rather than one spelling, since three sites wrote
`f => f.key` and three `(f) => f.key`; it carries a positive control over all
four spellings the sites used, so it cannot silently become a regex that matches
nothing. Full web suite 2954/2954, svelte-check 0 errors.

Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
This commit is contained in:
xarmian
2026-09-14 08:05:16 -04:00
committed by GitHub
parent 02240532de
commit d2de0536c2
7 changed files with 316 additions and 41 deletions
@@ -0,0 +1,86 @@
// Node-project test (no DOM): SOURCE-level guard that every create path takes
// its status default from `createDefaults` rather than reading `options[0]`
// itself (BUG-3078).
//
// This is the defect's own shape, so it is the thing worth pinning. The bug was
// not a wrong line — it was SIX copies of one question ("what status should a
// new item start with?") in six files, none of which asked whether the field
// could hold the answer. A behaviour test at one site says nothing about the
// other five, and a mutant that re-inlines the read at a site with no behaviour
// test of its own is invisible to every assertion in the suite. The claim "there
// is one copy" is a fact about the SOURCE, so it is asserted where it lives —
// the same shape as `laneKeyCallers.test.ts`, which exists for the sibling
// question about a lane KEY.
import { describe, it, expect } from 'vitest';
import { readFileSync } from 'node:fs';
import { fileURLToPath } from 'node:url';
const read = (rel: string) => readFileSync(fileURLToPath(new URL(rel, import.meta.url)), 'utf8');
/** Every surface that creates an item with a default status. */
const CALLERS: { name: string; rel: string; expectedCalls: number }[] = [
{ name: 'QuickCaptureSheet', rel: '../components/layout/QuickCaptureSheet.svelte', expectedCalls: 1 },
{ name: 'Sidebar quick-add', rel: '../components/layout/Sidebar.svelte', expectedCalls: 1 },
{ name: 'EditorBubbleMenu', rel: '../components/editor/EditorBubbleMenu.svelte', expectedCalls: 1 },
// Three separate create paths in one file: "Untitled", create-in-lane, and
// quick-create. Counted rather than merely detected, so removing two of the
// three still fails.
{ name: 'collection page', rel: '../../routes/[username]/[workspace]/[collection]/+page.svelte', expectedCalls: 3 },
];
/**
* The shape the fix removed, written to match the VARIANTS rather than the one
* spelling that happened to be there.
*
* Three of the six wrote `f => f.key` and three wrote `(f) => f.key`; two used a
* local named `defaultFields` and one used `fields`. A guard keyed on one
* spelling would have passed on the unfixed tree for half the sites, which is
* the same mistake the code made. This keys on the ASSIGNMENT — anything
* reading an `options[0]` off a status-ish field def into a result object.
*/
const INLINE_READ = /\.options\s*(\?\.)?\[\s*0\s*\]/;
describe('every create path takes its status default from createDefaults', () => {
for (const caller of CALLERS) {
it(`${caller.name} calls createDefaultFields ${caller.expectedCalls}x`, () => {
const src = read(caller.rel);
expect(src, `${caller.name} should import createDefaultFields`).toContain(
"from '$lib/collections/createDefaults'",
);
const calls = src.match(/createDefaultFields\s*\(/g) ?? [];
expect(calls.length, `${caller.name} call count`).toBe(caller.expectedCalls);
});
it(`${caller.name} reads no options[0] of its own`, () => {
expect(INLINE_READ.test(read(caller.rel)), `${caller.name} has an inline options[0]`).toBe(
false,
);
});
}
it('the inline-read pattern matches every spelling the six sites used', () => {
// Positive control. Without this the guard above could be a regex that
// matches nothing, which passes on any tree and proves nothing — the
// failure mode that makes an absence assertion vacuous.
const spellings = [
"defaultFields.status = statusField.options[0];",
"fields.status = field.options[0];",
"defaultFields.status = statusField.options?.[0];",
"defaultFields.status = statusField.options[ 0 ];",
];
for (const s of spellings) {
expect(INLINE_READ.test(s), s).toBe(true);
}
});
it('the inline-read pattern does not match the module that legitimately reads options', () => {
// Negative control, and a real one: `createDefaults.ts` and
// `laneWriteValue.ts` both index `options`, and a guard that flagged them
// would have to be weakened until it flagged nothing.
expect(INLINE_READ.test('const first = statusField?.options?.[0];')).toBe(true);
expect(read('./createDefaults.ts')).toContain('options?.[0]');
// ...which is exactly why the guard is scoped to the CALLERS list above
// and never swept across the tree.
expect(CALLERS.some((c) => c.rel.includes('createDefaults'))).toBe(false);
});
});
@@ -0,0 +1,121 @@
// Node-project test: the status default a create starts with (BUG-3078).
//
// The four arms below are the four outcomes MEASURED against the real create
// handler before this module was written, not four cases imagined from the
// types. On a collection whose `status` is `multi_select` with a stale scalar
// default that survived the retype:
//
// supplied `{"status":"open"}` -> 400, "must be an array of strings"
// omitted `{}` -> 201, stored {"status":"open"} (unchecked)
// converted `{"status":["open"]}` -> 201, stored {"status":["open"]}
// omitted, no stale default -> 201, stored {}
//
// That is why this converts where it can and omits only where it cannot: the
// converted write is the one arm that both lands AND stores a correctly-typed
// value. The probe and its numbers are on the BUG-3078 trail.
import { describe, it, expect } from 'vitest';
import { createDefaultFields } from './createDefaults';
import { laneWriteValue } from './laneWriteValue';
import type { Collection } from '$lib/types';
const coll = (statusField: Record<string, unknown> | null): Collection =>
({
slug: 'tasks',
name: 'Tasks',
schema: JSON.stringify({ fields: statusField ? [statusField] : [] }),
settings: '{}',
}) as unknown as Collection;
describe('createDefaultFields', () => {
it('uses the first option for a select — the case that was never broken', () => {
expect(createDefaultFields(coll({ key: 'status', type: 'select', options: ['open', 'done'] })))
.toEqual({ status: 'open' });
});
it('CONVERTS for a multi_select instead of omitting', () => {
// The arm the server accepts and stores correctly typed. Omitting here
// would fall through to the schema default, which is assigned WITHOUT a
// type check — so omitting would be strictly worse when that default is
// a stale scalar.
expect(
createDefaultFields(coll({ key: 'status', type: 'multi_select', options: ['open', 'done'] })),
).toEqual({ status: ['open'] });
});
it('OMITS for number and checkbox, where no conversion exists', () => {
// Reachable only because `options` survives a retype; a field that was
// always a number has none and never reached the old code either.
//
// KEY ABSENCE, not `toEqual({})`. A mutant that drops the `ok` check
// assigns `undefined` — `laneWriteValue`'s refusal carries no `value` —
// and `toEqual` treats an undefined-valued key as absent, so it SURVIVED
// the first version of this test. `Object.keys` is what tells the two
// apart. It matters beyond the mutant only for a future caller that
// spreads or enumerates the result: today's callers `JSON.stringify` it,
// which drops an undefined value, so the wire payload is identical and
// the distinction is the module's contract rather than an observed bug.
for (const type of ['number', 'checkbox']) {
const got = createDefaultFields(coll({ key: 'status', type, options: ['open', 'done'] }));
expect(Object.keys(got), type).toEqual([]);
expect('status' in got, type).toBe(false);
}
});
it('OMITS for relation and multi_relation, whose values are item ids', () => {
// Codex round 1 found `multi_relation`; `relation` is the same class and
// came from asking what else reaches that branch. Both refusals were
// measured against the create handler, not reasoned about:
// relation -> 400 "open" does not name an item in collection "tasks"
// multi_relation -> 400 must be an array of strings (item IDs, refs, …)
// There is no honest conversion for either: wrapping as ['open'] clears
// the shape check and is then refused by referent validation, because a
// leftover select option names no item.
for (const type of ['relation', 'multi_relation']) {
const got = createDefaultFields(
coll({ key: 'status', type, collection: 'tasks', options: ['open', 'done'] }),
);
expect(Object.keys(got), type).toEqual([]);
}
});
it('does NOT let laneWriteValue decide the reference case, because it cannot', () => {
// Pins the asymmetry rather than the outcome, so a later "simplification"
// that deletes the REFERENCE_TYPES guard and leans on laneWriteValue
// fails here with the reason attached. `laneWriteValue` passes a relation
// key straight through BY DESIGN — a lane key for a relation is already
// the target's item id. `options[0]` is not a lane key and never an id.
const relField = { key: 'status', type: 'relation', collection: 'tasks', options: ['open'] };
expect(laneWriteValue(relField as never, 'open')).toEqual({ ok: true, value: 'open' });
expect(Object.keys(createDefaultFields(coll(relField)))).toEqual([]);
});
it('omits when the field declares no options at all', () => {
expect(Object.keys(createDefaultFields(coll({ key: 'status', type: 'select' })))).toEqual([]);
expect(
Object.keys(createDefaultFields(coll({ key: 'status', type: 'select', options: [] }))),
).toEqual([]);
});
it('omits when there is no status field, and when there is no collection', () => {
expect(createDefaultFields(coll(null))).toEqual({});
expect(createDefaultFields(coll({ key: 'priority', type: 'select', options: ['high'] }))).toEqual({});
expect(createDefaultFields(null)).toEqual({});
expect(createDefaultFields(undefined)).toEqual({});
});
it('survives a malformed schema rather than throwing', () => {
// `parseSchema` swallows the parse error and returns defaults; a create
// path that threw here would be a worse failure than the one being fixed.
expect(createDefaultFields({ schema: 'not json', settings: '{}' } as unknown as Collection))
.toEqual({});
});
it('returns a FRESH object each call', () => {
// Callers mutate the result (they add lane values to it), so a shared
// object would leak one create's lane value into the next create.
const c = coll({ key: 'status', type: 'select', options: ['open'] });
const a = createDefaultFields(c);
(a as Record<string, unknown>).leaked = true;
expect(createDefaultFields(c)).toEqual({ status: 'open' });
});
});
+90
View File
@@ -0,0 +1,90 @@
// The `status` a create should start with (BUG-3078).
//
// Six create paths independently did this:
//
// const statusField = schema.fields.find((f) => f.key === 'status');
// if (statusField?.options?.length) {
// defaultFields.status = statusField.options[0];
// }
//
// Nothing asked whether `status` could still HOLD a string. `options` survives a
// retype — the premise BUG-3057, BUG-3067, BUG-3068 and BUG-3074 all turn on —
// so on a collection whose `status` was retyped to `multi_select`, `number` or
// `checkbox` while keeping its options, every one of those sites sent a scalar
// into a field that cannot hold it and the server REFUSED the create outright.
// Not a degraded create: no item, an error toast, and no route to a good
// outcome. Quick capture simply stopped working on that collection.
//
// ONE MODULE for the same reason `laneKeyCallers.test.ts` exists: six sites
// asking one question in six places is how the question comes to have six
// answers. `createDefaultCallers.test.ts` pins that they all call this.
//
// WHAT EACH ARM DOES, and why it is not uniform:
//
// * The conversion is `laneWriteValue`'s, not a second copy of it. For a
// `multi_select` it yields `['open']` rather than `'open'`, which the server
// ACCEPTS and stores correctly typed (measured against the create handler,
// not assumed — see the BUG-3078 trail for the four-arm probe).
// * Where `laneWriteValue` REFUSES — `number`, `checkbox` — the key is OMITTED
// and the server applies the collection's own schema default. Omitting is
// the only remaining move: a create has no lane to refuse into, and failing
// the create is the behaviour being fixed.
// * REFERENCE-SHAPED types are omitted HERE rather than by `laneWriteValue`,
// and that asymmetry is the one thing in this module that is not obvious.
// `laneWriteValue` answers "can this LANE KEY become a value", and a lane
// key for a relation is ALREADY the target's item id — its own comment says
// so — which is why it passes `relation` straight through. This caller does
// not hold a lane key. It holds `options[0]`, a leftover `select` option,
// which is never an item id. So the same string is a good value there and a
// bad one here, and changing `laneWriteValue` to refuse it would break the
// lane path it was written for.
//
// Both arms measured against the create handler rather than reasoned about:
// `relation` answers 400 `"open" does not name an item in collection
// "tasks"`, and `multi_relation` answers 400 `must be an array of strings
// (item IDs, refs, or exact titles)`. There is no honest conversion for
// either — wrapping it as `['open']` passes the shape check and is then
// refused by referent validation, because the option names no item — so the
// key is omitted and the collection's own default applies.
//
// Codex found `multi_relation`; `relation` is the same class and was found
// by asking what else reaches that branch (CONVE-18).
//
// A CAVEAT THE OMIT ARM INHERITS, recorded because it is invisible from here.
// `ValidateFieldsDetailed` assigns a schema default and skips its own type
// check, so a collection whose stale SCALAR default also survived the retype
// stores that scalar unvalidated when the key is omitted. That is a server-side
// hole with its own item, reachable today by any caller that creates without a
// status (CLI, MCP, plain API) — this module routes onto it rather than opening
// it. It is still strictly better than the refusal it replaces, because the
// alternative is no item at all.
import type { Collection } from '$lib/types';
import { parseSchema } from '$lib/types';
import { laneWriteValue } from './laneWriteValue';
/** Field types whose values are item references, never a declared option. */
const REFERENCE_TYPES = new Set(['relation', 'multi_relation']);
/**
* The `fields` object a new item should be created with.
*
* Returns an EMPTY object rather than omitting `status` selectively, so callers
* keep their existing `JSON.stringify(defaultFields)` shape and no caller has to
* know which arm it landed on.
*/
export function createDefaultFields(collection: Collection | null | undefined): Record<string, unknown> {
const defaults: Record<string, unknown> = {};
if (!collection) return defaults;
const statusField = parseSchema(collection).fields.find((f) => f.key === 'status');
const first = statusField?.options?.[0];
if (first === undefined) return defaults;
// See the header: a stale `select` option is never an item id, so no
// reference-shaped field can be defaulted from one.
if (REFERENCE_TYPES.has(statusField?.type ?? '')) return defaults;
const write = laneWriteValue(statusField, first);
if (write.ok) defaults.status = write.value;
return defaults;
}
@@ -1,7 +1,8 @@
<script lang="ts">
import type { Editor } from '@tiptap/core';
import type { Collection, Item } from '$lib/types';
import { parseSchema, isAgentCollection } from '$lib/types';
import { isAgentCollection } from '$lib/types';
import { createDefaultFields } from '$lib/collections/createDefaults';
import { api, isPlanLimitError, planLimitMessage } from '$lib/api/client';
import { toastStore } from '$lib/stores/toast.svelte';
import { localIndex } from '$lib/stores/localIndex.svelte';
@@ -178,15 +179,9 @@
errorMsg = '';
}
// BUG-3078: the status default goes through the declared type.
function getDefaultFields(collection: Collection): Record<string, any> {
const schema = parseSchema(collection);
const fields: Record<string, any> = {};
for (const field of schema.fields) {
if (field.key === 'status' && field.options && field.options.length > 0) {
fields.status = field.options[0];
}
}
return fields;
return createDefaultFields(collection);
}
async function handleCreate() {
@@ -19,7 +19,8 @@
import { collectionStore } from '$lib/stores/collections.svelte';
import { uiStore } from '$lib/stores/ui.svelte';
import { toastStore } from '$lib/stores/toast.svelte';
import { parseSchema, parseSettings, itemUrlId, isAgentCollection } from '$lib/types';
import { parseSettings, itemUrlId, isAgentCollection } from '$lib/types';
import { createDefaultFields } from '$lib/collections/createDefaults';
import DockedSheet from '$lib/components/layout/DockedSheet.svelte';
let {
@@ -68,13 +69,9 @@
submitting = true;
const t = title.trim();
try {
const schema = parseSchema(coll);
const settings = parseSettings(coll);
const defaultFields: Record<string, any> = {};
const statusField = schema.fields.find((f) => f.key === 'status');
if (statusField?.options?.length) {
defaultFields.status = statusField.options[0];
}
// BUG-3078: the status default goes through the declared type.
const defaultFields: Record<string, any> = createDefaultFields(coll);
const item = await api.items.create(wsSlug, coll.slug, {
title: t,
content: settings.content_template || '',
+4 -7
View File
@@ -10,7 +10,8 @@
import { authStore } from '$lib/stores/auth.svelte';
import { goto } from '$app/navigation';
import { api, isPlanLimitError, planLimitMessage } from '$lib/api/client';
import { parseSchema, parseSettings, itemUrlId, isAgentCollection } from '$lib/types';
import { parseSettings, itemUrlId, isAgentCollection } from '$lib/types';
import { createDefaultFields } from '$lib/collections/createDefaults';
import { getActiveKey } from '$lib/nav/destinations';
import type { Collection } from '$lib/types';
import { toastStore } from '$lib/stores/toast.svelte';
@@ -161,13 +162,9 @@
const title = quickAddTitle.trim();
cancelQuickAdd();
try {
const schema = parseSchema(coll);
const settings = parseSettings(coll);
const defaultFields: Record<string, any> = {};
const statusField = schema.fields.find(f => f.key === 'status');
if (statusField?.options?.length) {
defaultFields.status = statusField.options[0];
}
// BUG-3078: the status default goes through the declared type.
const defaultFields: Record<string, any> = createDefaultFields(coll);
const item = await api.items.create(wsSlug, coll.slug, {
title,
content: settings.content_template || '',
@@ -9,6 +9,7 @@
import { plansProgressToMap, fetchCollectionProgress } from '$lib/collections/progressMerge';
import { resolveRenameNavTarget, resolveSyncRenameTarget } from '$lib/collections/renameNav';
import { laneWriteValue, laneWriteRefusalMessage } from '$lib/collections/laneWriteValue';
import { createDefaultFields } from '$lib/collections/createDefaults';
import BoardView from '$lib/components/collections/BoardView.svelte';
import ListView from '$lib/components/collections/ListView.svelte';
import TableView from '$lib/components/collections/TableView.svelte';
@@ -1914,12 +1915,8 @@
if (!wsSlug || !collSlug || creatingNew) return;
creatingNew = true;
try {
const schema = collection ? parseSchema(collection) : { fields: [] };
const defaultFields: Record<string, any> = {};
const statusField = schema.fields.find(f => f.key === 'status');
if (statusField?.options?.length) {
defaultFields.status = statusField.options[0];
}
// BUG-3078: the status default goes through the declared type.
const defaultFields: Record<string, any> = createDefaultFields(collection);
const item = await api.items.create(wsSlug, collSlug, {
title: 'Untitled',
content: '',
@@ -1968,12 +1965,8 @@
const trimmed = title.trim();
if (!trimmed) return null;
try {
const schema = collection ? parseSchema(collection) : { fields: [] };
const defaultFields: Record<string, any> = {};
const statusField = schema.fields.find((f) => f.key === 'status');
if (statusField?.options?.length) {
defaultFields.status = statusField.options[0];
}
// BUG-3078: the status default goes through the declared type.
const defaultFields: Record<string, any> = createDefaultFields(collection);
// Pre-fill the lane's group field (status, or a custom
// board_group_by select) so the item opens in this lane — converted
// through the declared type, for the reason on the drag path
@@ -2114,12 +2107,8 @@
if (!title || !wsSlug || !collSlug || creatingNew) return;
creatingNew = true;
try {
const schema = collection ? parseSchema(collection) : { fields: [] };
const defaultFields: Record<string, any> = {};
const statusField = schema.fields.find(f => f.key === 'status');
if (statusField?.options?.length) {
defaultFields.status = statusField.options[0];
}
// BUG-3078: the status default goes through the declared type.
const defaultFields: Record<string, any> = createDefaultFields(collection);
// Epoch before create: brand-new id, never fenced (BUG-2098).
const epoch = localIndex.scopeEpochFor(wsSlug);
const item = await api.items.create(wsSlug, collSlug, {