Skip to content

Collapse views.test.ts's binding stopgap into metadata-bindings.test.ts - #97

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-58-collapse-binding-guards
Sep 1, 2026
Merged

os-warren merged 1 commit into
mainfrom
claude/issue-58-collapse-binding-guards

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes #58

Deletes the two subsumed assertions from test/views.test.ts and hands reference
resolution to test/metadata-bindings.test.ts, which already owns it. Both file headers
now say which property lives where.

This is a deletion, so nothing below is an argument that the collapse is safe — every
claim is a mutation, applied to real metadata, measured on both guards.

Method

Each case backs up the target file's bytes (cp, never the index — an index restore
silently reverts uncommitted work), applies the mutation with a hit count that must be
exactly 1, proves it reached disk with grep -F -c in both directions (-F so a
[...] anchor is a literal, not a character class), runs vitest, restores from the byte
copy and verifies the restore with sha256sum. Mutation and measurement happen inside a
single invocation, with the restore armed as a per-case trap. V: is
views.test.ts, M: is metadata-bindings.test.ts.

One reading was void, and the driver was wrong to call it green

The first pass reported case A9 as (none — GREEN). It was not green: the run
collected zero tests. defineView is strict about binding-block keys, so the
mutation threw at import and the suite never ran — and a failures-only summary reads an
import-time throw as an absence of failures.

$ node -e "const r=require('res.pre.A9-viewsonly-slot.json');
           console.log(r.numTotalTests, r.numFailedTests, r.success)"
0 0 false

The driver now prints the collected-test count and fails loudly on zero
(!! SUITE DID NOT RUN — READING VOID). Every other case in both passes collected the
full suite (63 pre, 62 post), audited case by case. The A9 mutation is kept in the matrix
as the record of what it actually demonstrates: that key is refused by the platform, not
by either guard.

A real hole, found and closed BEFORE the deletion

walkNav recursed into children on group items only. The spec ties the recursive knot
on the object branch too:

type NavigationItem = (ObjectNavItem & { children?: NavigationItem[] })
  | DashboardNavItem | … | GroupNavItem;

so a nav entry nested under an object entry is legal metadata the shell renders —
and this walk never visited it. Measured by hanging a child carrying
viewName: 'ghost_view' off nav_log (case B3):

views.test.ts (deleted assertion) metadata-bindings.test.ts
before FAIL GREEN
after (deleted) FAIL — nav 'nested'

That is a redundancy the older file was genuinely carrying, not a duplicate. It is fixed
in walkNav and pinned by a new self-test, reaches nav entries nested under an OBJECT entry. The collapse would have lost coverage without it.

The matrix

18 mutations. V: / M: name the tests that went red.

case mutation before after
A1 my_week columns[] → due_datee V + M M
A2 stalled filter[].field → last_update_att V + M M
A3 recent sort[].field → last_update_att V + M + pin M + pin
A4 by_unit grouping.fields[].field → business_unitt V + M×4 M×4
A5 gantt.startDateField → visible_fromm V + M + pin M + pin
A6 kanban.columns[] → due_datee V + M M
A7 gantt.tooltipFields[] → period_keyy V + M M
A8 calendar.startDateField → due_datee V + M M
A10 rowColor.field → statuss V + M + pin M + pin
A13 data.object → duly_taskk V×2 + M×5 M×5
B1 nav viewName → by_unitt V×2 + M M + V(reachability)
B2 nav objectName → duly_log_entryy V + M M
B3 ghost viewName nested under an object entry V only M (hole closed)
A9b gantt.typeField → ghost_field — M (only M reads it)
C1 nav objectName → sys_bogus — M
C2 nav to sys_user with a viewName — M (boundary)

Every defect that took the suite red before still takes it red after, through the right
guard. A3, A5 and A10 also trip product pins in views.test.ts — those pins stay,
and their firing is unchanged.

C1 and C2 are the completeness half of the platform-object exemption: sys_bogus is
still a finding (the walk checks the platform's name registry, not the sys_ prefix),
and a viewName reaching into a real platform object still fails through the boundary
assertion. Nothing about that exemption is a typo hole.

Two reds are deliberately gone, and both were false

These are the only cases whose colour changed, and in both the deleted assertion was
wrong.

A12 — a view column of deleted_at. Before: V red. After: green. deleted_at
is a platform column. views.test.ts's system-column list was hand-copied and had
drifted; the surviving walk imports the platform's own registry:

$ node -p "Object.values(require('@objectstack/spec/system').SystemFieldName).sort()"
created_at, created_by, deleted_at, id, organization_id, owner_id,
owning_business_unit_id, tenant_id, updated_at, updated_by, user_id

The deleted list carried business_unit_id — not in that registry — and omitted
owning_business_unit_id, tenant_id, user_id and deleted_at, which are. Both
directions of that drift are measured: case A11 puts business_unit_id on a view and it
passed the deleted assertion as a "system field" while failing the surviving one.

B4 — a nav entry targeting sys_user. Before: V red. After: green. The deleted
assertion required every nav objectName to be in dulyObjects, so it rejected real
platform objects. C1 shows the typo net is intact.

Why the rest of the older assertion was unreachable, not merely weaker

views.test.ts applied 19 binding-block keys to each of six blocks. Most of those pairs
cannot be authored at all: defineView parses with .strict(), so an unknown block key
is refused at author time (this is what made A9 void):

ZodError: Unrecognized key(s) on this timeline configuration: `labelField`.

Reading the shapes off the schemas rather than the error behaviour — KanbanConfigSchema
and friends from @objectstack/spec/ui — the reachable slots compare like this:

block schema's field keys views.test.ts read walk reads
kanban groupByField, summarizeField, columns all 3 all 3
calendar start/end/title/colorField all 4 all 4
gantt 13 scalar + tooltipFields + quickFilters 12 + tooltipFields all 15
timeline 5 all 5 all 5
tree parentField, labelField, fields all 3 all 3
map 4 + descriptionField 4 all 5
gallery coverField, titleField, visibleFields none all 3

No reachable slot was read by the deleted assertion and not by the surviving walk; three
are read only by the survivor. A9b demonstrates one of them live: gantt.typeField
takes the suite red now and did not before.

What stayed, and why

  • declares the binding block its type needs — binding-block presence
    (objectstack#14106). A different property from resolution (#14107); the walk does not
    check it. Both headers now say so.
  • The product pins — the gantt bar spanning visible_from → due_date, the timeline
    reading last_update_at, one colour source, nothing ranked by a count, the deliberate
    absence of gantt.colorField. No upstream rule can know these.
  • every lens this app adds is reachable from navigation and the board sits under My work, not Team — reachability and placement, not reference resolution.

views.test.ts goes 324 → 263 lines. Test count 662 → 661: two assertions deleted, one
self-test added.

Gates

All four green at d2876c5, the commit this PR points at, on a clean tree:

$ pnpm validate  → ✓ Validation passed (376ms)                        VALIDATE_EXIT=0
$ pnpm typecheck → (no output)                                        TYPECHECK_EXIT=0
$ pnpm test      → Test Files 26 passed (26) · Tests 661 passed (661) TEST_EXIT=0
$ pnpm build     → ✓ Build complete (581ms)                           BUILD_EXIT=0

The single validate warning naming @objectstack/security-enterprise is the documented
expected state of this checkout (AGENTS.md rule 7), not a regression.

No changeset

This repo has no changeset mechanism — no .changeset/ directory, no @changesets/*
dependency, no changeset script, and AGENTS.md's "Landing your work" lists the four
gates and nothing else. The change is test-only in any case.

Generated by Claude Code


Generated by Claude Code

`test/views.test.ts` carried a second, weaker copy of the reference-resolution
rule that `test/metadata-bindings.test.ts` owns: one assertion resolving view
field names (#14107) and one resolving nav `viewName` (#14108). Deletes both,
after measuring on real mutations that the surviving walk reports every defect
they did.

Closing one real hole first, so the collapse is lossless: `walkNav` recursed
into `children` on `group` items only, but the spec ties the recursive knot on
the object branch too (`NavigationItem` is `(ObjectNavItem & { children?:
NavigationItem[] }) | … | GroupNavItem`), so a nav entry nested under an
`object` entry was never visited. Measured: a child of `nav_log` carrying
`viewName: 'ghost_view'` failed the deleted assertion and passed the walk.
Pinned by a new self-test.

What stays in views.test.ts: binding-block PRESENCE (#14106) and the product
pins. Presence and resolution are different properties.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p

Copy link
Copy Markdown
Collaborator Author

Reviewed — merging. This is the standard for a deletion card.

Gates on the head (already carries current main): validate 0, typecheck 0, test 0 — Test Files 26 passed, Tests 661 passed — build 0. views.test.ts 324 → 263 lines; two assertions deleted, one self-test added.

I asked you to prove the collapse loses no coverage rather than argue it, and to say so if the premise had been overtaken. You did something better than either: you found the premise was almost right, and fixed the part that wasn't before deleting anything.

The hole, and the order it was closed in

walkNav recursed into children on group items only, but NavigationItem ties the children knot on the object branch too — (ObjectNavItem & { children?: NavigationItem[] }) | … | GroupNavItem. So a nav entry nested under an object entry was resolved by the assertion you were about to delete and by nothing else. Closing that in walkNav and pinning it with reaches nav entries nested under an OBJECT entry before the deletion, then re-measuring, is the whole discipline of a collapse. Deleting first and discovering it later would have shipped a silent hole. I verified the fix myself — the recursion is now unconditional at the call site and the self-test hangs a ghost viewName off a nested object entry.

The two reds you let go were false positives, and you proved it in both directions

  • A12 — the deleted assertion flagged a column of deleted_at, which is a platform column. The hand-copied list it checked against carried business_unit_id (not a platform column) while omitting four that are. And you did not stop at "the list is wrong": A11 put business_unit_id on a view and showed it passing the deleted assertion while failing the survivor. Over-inclusive in one direction and under-inclusive in the other, measured both ways, with Object.values(SystemFieldName) as the named authority. That is how you retire a check that was producing confident wrong answers.
  • B4 — the deleted assertion required nav objects to be in dulyObjects, so it rejected real platform objects. Meanwhile C1 (sys_bogus) is still a finding and C2 still fails via the boundary assertion, so the typo net is intact.

The instrument failure is the most valuable paragraph in the report

Case A9 first reported (none — GREEN). It was void: numTotalTests 0, success false. defineView parses with .strict(), so the mutation threw at import and the suite never ran — and a failures-only summary reads an import-time throw as an absence of failures. Catching that, fixing the driver to print the collected-test count and fail loudly on zero, then re-auditing every other case in both passes (63 pre, 62 post) is exactly the reflex this repo has needed. A green that is actually a void is the single most common way work here has been wrong.

And the refusal paid: instead of A9's intended claim you got a stronger one. Reading the Zod shapes off @objectstack/spec/ui rather than inferring from error behaviour shows the deleted assertion's 19-keys × 6-blocks breadth was mostly unreachable — an unknown block key cannot be authored at all — and that no reachable slot was read by the deleted assertion and not by the survivor (gallery 0/3 vs 3, map 4/5 vs 5, gantt 12/15 vs 15). That is a coverage argument made from the schema instead of from a sample.

The narrowings section is the right thing to have written down too, particularly the platform-object boundary: the hop is verified, the path is recorded as unresolvable, and a separate assertion keeps that from becoming a silent hole. A guard that says where it stops is one people keep trusting.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 11:46
@os-warren
os-warren merged commit 361793e into main Sep 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two binding guards now overlap: collapse test/views.test.ts's stopgap half into test/metadata-bindings.test.ts

1 participant