Repository navigation
test(cli): os explain's key-retention sweep now judges the optional/required TABLE faces - #18538
Merged
os-support-ai merged 2 commits intoSep 16, 2026
Conversation
…ces too The sweep read the `example` face only — the one face `evaluate` reads. The `required` / `optional` tables name keys as well, and no assertion in this file had ever read a row's `name` against a schema, so the half of #16925's defect that lived in the table rows was unguarded: a reader copies a row, the schema silently strips the key, and the query runs unfiltered under an ordinary success. The design work is the row grammar. A row name is read as an ALTERNATION — split on `|`, trim — and judged iff every part is a bare identifier, which makes `view`'s four-slot required row judgeable as four keys instead of waved through as prose. A row that does not fit must be declared in `PROSE_ROWS` (empty today), and a stale declaration fails too. Two techniques, because one entry does not read: `.shape` for the eight bound entries that expose one, and a behavioural probe for `ActionSchema`, which resolves to a pipe. Each entry's test carries an inline control key that must read ABSENT, since `action` has no second opinion. Every row on all nine bound entries names a real key today, so this lands green. Claude-Session: https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3 Co-authored-by: Claude <noreply@anthropic.com>
…y the open one A strict schema refuses the row's key by name; an open one takes the object, strips the key and reports success. The message named only the second, so the verdict read wrong on the eight strict entries. Claude-Session: https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3 Co-authored-by: Claude <noreply@anthropic.com>
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
os-support-ai
marked this pull request as ready for review
September 16, 2026 22:10
os-support-ai
deleted the
claude/issue-17266-explain-key-retention-table-faces
branch
September 16, 2026 22:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #17266
Clause-②: no
What this is
A test-only pin. There is no defect in the tree today — the card says so itself (⛔ "Not a defect in what #16925 shipped — a declared gap in the guard it built") — and that was re-measured here rather than taken on trust: every row on all nine bound entries names a real key. So this lands green.
The key-retention sweep in
packages/cli/test/commands.test.tsreads the EXAMPLE face only, becauseevaluate(key)is the one face it reads. A catalog entry names keys on a second face — itsrequired/optionaltables — and no assertion in that file had ever read a row'snameagainst a schema. (The #3244 pin above it reads one row'stypeSTRING; it never asks whether the row's NAME is a key.) The trap is the same one a step earlier: a reader copies a row, the schema silently strips the key, and the query runs unfiltered under an ordinary success.os explain queryshippedfilters/sorton BOTH faces; only the example half was guarded.The taker was the file itself. The gap was declared in the test's own header, so that header is rewritten in the same diff — it no longer announces a gap that is closed, and it now points at the block that closes it.
The row grammar — the actual design work
A row's
nameis prose until something decides it is a key name. The declared rule:That is deliberately wider than "one identifier", and the width is what earns its keep: it makes
view's required row (list | form | listViews | formViews, four slot names in thenameposition — the card's own blocking example) judgeable as four keys instead of waved through as prose. Measured: all four are realViewSchemakeys.A row that does not fit the grammar is ⛔ not silently skipped — it must be declared in
PROSE_ROWS, and a declaration that stops being needed fails too, so a stale one cannot go on un-judging a row.PROSE_ROWSis empty today, measured: every row on all nine bound entries fits.Two techniques, because one entry does not read
.shapeactionActionSchemaresolves to a pipe (lazySchema(() =the arrowactionObject().refine(...))), and a pipe has no shape — the card predicted this and it holds. The probe never asks whether the VALUE is acceptable, only whether the schema KNOWS the name: anunrecognized_keysissue naming it is ABSENT (a strict object refused it), success with the key gone from the output is ABSENT (an open object stripped it silently), anything else is DECLARED.Measured across all nine entries: the two techniques agree on every row the shape one can see, and the probe returns ABSENT for 7 control names on all 9 entries — including
query, whose open top level answers by DROPPING rather than refusing, which is the exact silent-strip mode this guard exists for.⭐ Each entry's test carries an inline control key that must read ABSENT. That is load-bearing rather than decorative:
actionhas no second opinion, so a technique that answered "declared" to everything would report green over twelve rows and be indistinguishable from a real pass.Premises verified before writing any code
SCHEMAS.object.optional.find(...)in the explain:os explain objectdocumentsownershipas "own" | "extend" — real values are user | org | none #3244 pin, which asserts the row'stypestring. Control words known present in the same file:evaluate(× 6,.example× 4.packages/cli/test/commands.test.tsis the right and only place." ✅ It is the only file in the repository that imports the explain catalog at all (repo-wide grep forcommands/explain, excludingnode_modulesanddist). No sibling pin exists.Ablation — a pin that fails on the wrong fix
Two legs, both run from the committed state, both mutating the source the pin reads (
packages/cli/src/commands/explain.ts), each with a trap restore on absolute paths.HEADblob for that file:09ab4c720ef0a52ffccbf9c19100a307d16568efLeg 1 —
query, the shape technique, silent-drop mode. Table rowwhererewritten tofilters(the historical defect, on the table face only — the example keepswhere).from=1 to=0, afterfrom=0 to=1535ff0bf015b9a757d2a89dabb666cfc3fae7a2f(not equal to HEAD blob)os explain query — every QuerySchema table row names a key the schema declares✗,Tests 1 failed | 49 passed (50)os explain query — example survives QuerySchema with every declared key intactstayed GREEN. That is the card's claim made mechanical: the pre-existing retention assertion cannot see this face.09ab4c72...equals the HEAD blob;git diff HEADon the path EMPTY;git status --porcelainemptyLeg 2 —
action, the probe technique, refuse-by-name mode. Required rowtargetrewritten toflow(the key the entry's own description says does not exist).from=1 to=0, afterfrom=0 to=1279748e6d171a66640fd020e37a24a8934e97ab7(not equal to HEAD blob)os explain action — every ActionSchema table row names a key the schema declares✗,Tests 1 failed | 49 passed (50); both the parse and retention assertions onactionstayed greengit diff HEADon the path EMPTY; porcelain emptyFinal tree state after both legs:
git diff HEADexit 0 (clean). ⛔ No permanent ablation artefact is left behind.No dist leg is owed here: the mutated subject is imported by the test relatively (
from '../src/commands/explain'), so it resolves from source, not through a packageexportsentry — there is no built artefact in the resolution path for it.Verification
Run on
4fb225c6e(the tip this PR opens with).Gate derivation —
node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack, derived from the actual changed path (1 path,packages/cli/test/commands.test.ts), re-derived after the final commit and byte-identical. Reconciled with--rancarrying exit codes:pnpm check:cross-package-test-inputsexit 1 — PRE-EXISTING, ⛔ not this PR's. It fires whereverpackages/spec/distis built and namespackages/cli/test/init-created-files-summary.e2e.test.ts(a file this diff does not touch), reporting thatpackages/clidescendspackages/spec/dist/with no declared glob reaching inside it. Already filed as [finding] check:cross-package-test-inputs passes in CI and fails on a built tree — its verdict is a function of gitignored build state #18353 / [regression] check:cross-package-test-inputs 的裁决取决于被 gitignore 的空目录 packages/spec/dist 存不存在 —— 构建过的工作树一律红,CI 绿只因那一步跑在构建之前(#18340 引入) #18348. ⛔ No new card.pnpm check:dual-build-cjs-loadsexit 3 —PREREQUISITE NOT MET, i.e. NOT MEASURED, ⛔ neither pass nor fail. It reads built output and 12 packages outside this diff's closure have nodist/; only a whole-repopnpm buildsatisfies it, which CI does. This diff touches nosrc/, so it can move nodist/.Beyond the derivation:
pnpm lint(repo-wide,eslint . --no-inline-config)pnpm --filter @objectstack/cli typechecktsc --noEmit+check:test-typecheck)pnpm --filter @objectstack/cli exec vitest run --project unit(the tier this file runs in)⭐ Typecheck coverage of the edited file asserted rather than assumed:
tsc -p tsconfig.test.json --listFilesnamestest/commands.test.ts(the package's plaintsconfig.jsondoes not, which is why thetypecheckscript chainscheck:test-typecheck). Rawtsc -p tsconfig.test.jsonreports 28 errors across 3 OTHER files, all ledgered and shrink-only; zero are intest/commands.test.ts.The
integrationtier is declared to CI: this diff touches no integration-layer file, no spawn entry point (bin/,test/helpers/serve-process.ts) and no driver/kernel boot path, andcommands.test.tslands inunitunderpackages/cli/vitest-tiers.ts(neither SPAWN nor KERNEL).Changeset — measured, not assumed
skip-changesetis the correct disposition, and it was measured rather than reasoned from the path:@objectstack/cli'sfiles[]is["dist", "README.md", "CHANGELOG.md"];npm pack --dry-runshipsdist/,bin/,README.md,CHANGELOG.md,LICENSE,package.json— no test source.SCHEMAS(fromsrc/commands/explain.ts) IS found indist/commands/explain.js, so the grep can find a shipped symbol. Each ofPROSE_ROWS,os_explain_table_face_control_key,__os_explain_table_face_probe__,rowKeyNames-> 0 shipped files.src/cannot movedist.skip-changesetis OWED on this PR and the author was fenced from writing labels by its dispatch.changeset-checkinpr-automation.ymlhas no path-based exemption — it requires either a changeset or that label — so this PR goes red until a seat applies it. Flagged here and in the report rather than applied.Acceptance notes
Out of scope, noted and ⛔ not filed:
os explain query's optional table lists 6 ofQuerySchema's 17 keys. The card declares missing rows out of scope by name and explains why (a missing row is an omission, not an error — nothing an author copies fails or is silently dropped). This PR's grammar judges rows that are PRESENT; it deliberately says nothing about absent ones. Taker: none — stated so the boundary stays legible.view's prose spellsfiltersin a line comment andfilterin the entry description. A code-comment inconsistency with no runtime or authoring reach; the card routes it to [finding]os explain view's example teaches a flat view literal —ViewSchemais a CONTAINER (list/form/listViews/formViews) #15171, which owns that entry. ⛔ Not touched here (editingview's entry is fenced).packages/specschema was relaxed, considered or touched. The new assertion's failure message says so in its own text: a wrong row is corrected on the ROW.The floor question, held
This adds assertion coverage inside a test file that already runs in CI — a blind-spot repair of an EXISTING gate. ⛔ It adds no new required gate, hook or ratchet, no workflow step, no
check:*script, nopackage.jsonscript. The distinction ruling F draws (adding a door that must be passed vs. making an existing door see more) was never reached in the wrong direction; nothing in the delivery required it.Generated by Claude Code