Repository navigation
fix(cli): os lint --eval --json reports an unscorable stack instead of crashing - #15576
Conversation
… of crashing
`runMetadataEval`'s docblock promised totality — "Never throws — generation
failures become failed cases" — and the promise was false as written. Its `try`
wrapped only `options.generate(...)`; the `scoreMetadata(stack)` call that
follows sat outside it. A generator that THREW became a failed case; one that
RETURNED a value nobody could walk escaped the harness, out through an eval
branch that dispatches above the project-lint `try`, and out of a command whose
`json` flag is hand-rolled so no framework envelope catches it either. Driven on
the published entry: exit 1, stdout 0 bytes, stderr " Error: poison getter".
Scoring walks the stack twice and both walks are driven: `normalizeStackInput`
spreads the top level, and the schema parse walks everything below it. Guarding
the whole `scoreMetadata` call covers both — and every future one — where a
guard around the normalizer alone would have left the nested case escaping. It
also leaves `scoreMetadata`'s other caller (`os lint --score`) untouched; that
one already sits inside the command's catch-all JSON exit.
The failure routes through the per-case `generationError` channel the throwing
generator already used, so the report exit that was always there emits its JSON,
names the cause and still exits non-zero. Nothing new appears on the `--json`
face; the exit is simply reachable now.
The failed case is scored 0/F/`valid: false`, deliberately not `scoreMetadata({})`
— an empty stack scores 100/A/valid, and stamping that on a stack nobody could
parse would replace a loud crash with a quiet wrong answer.
Part of #14974
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
…nt-eval-json-error-face
📓 Docs Drift Check4 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
Coarse fallback — 22 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin d4a5da13b8bcd8e180f4e7781bd6e81bbb1b0cc0 && git checkout d4a5da13b8bcd8e180f4e7781bd6e81bbb1b0cc0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 66086d06ad8d688c7c1a505dbe008acc53d64637 a79148386ac5ad366049566789f50c5fb4df5f27 && git checkout -B drift-repro 66086d06ad8d688c7c1a505dbe008acc53d64637 && git merge --no-ff a79148386ac5ad366049566789f50c5fb4df5f27
node scripts/docs-audit/affected-docs.mjs --json 66086d06ad8d688c7c1a505dbe008acc53d64637 |
Fixes #14974
Phase 2 of #14974, executing the PM's ruling: option C — make the eval path total, so the failure becomes a per-case failure the EXISTING JSON exit already reports. Nothing new appears on the
--jsonface.The before-shape, driven on the published entry
packages/cli/bin/run.js, with a generator that returns (does not throw) a value whose property enumeration throws:JSON.parseon that stdout throwsUnexpected end of JSON input— there is no document at all. Eval mode dispatches above the project-linttryand returns, so that mode's catch-all JSON exit can never see it; andlint.tshand-rollsjsonas a plainFlags.booleanrather than oclif'senableJsonFlag(zero occurrences anywhere inpackages/cli/src), so there is no framework envelope underneath either.Why this is enforcement, not a contract move
Two docblocks already promised totality in writing:
metadata-eval.ts— "Never throws — generation failures become failed cases."scoreMetadata— "Pure and deterministic."The first was false as written. Its
trywrapped onlystack = await options.generate(...); theconst score = scoreMetadata(stack);on the next line sat outside it. A generator that threw was captured intogenerationError, exactly as documented; one that returned a poisonous value was not. That gap was the entire reachable class.The report exit already emits JSON and already exits 1 when
report.okis false. This change makes it reachable where it was not. No new key, no new payload shape, no widened accept set.The site was measured, not assumed
Phase 1 proposed guarding
score.ts's unprotectednormalizeStackInput. That would have been incomplete, and the measurement says so:scoreMetadatawalks the stack twice, and both walks are reachable throw sites.normalizeStackInput's{ ...input }spreadscore.ts:66ObjectStackDefinitionSchema.safeParse— the shallow spread copied the child by reference, so the getter fires inside zodscore.ts:69Both driven, with stack traces at
--stack-trace-limit=100. A guard aroundnormalizeStackInputalone leaves the second one escaping.So the guard goes around the whole
scoreMetadatacall, inmetadata-eval.ts. Three reasons, in order of weight:scoreMetadata.generationErroris populated a dozen lines up, by the throwing-generator path this failure now joins.scoreMetadatahas a second caller,os lint --scoreatlint.ts:613, which already sits inside the project-linttryand so already has a JSON net. ChangingscoreMetadataitself would have moved that mode too.The after-shape, same invocation, same published entry
{ "ok": false, "passed": 0, "failed": 5, "meanScore": 0, "results": [{ "id": "invoice_with_line_items", "generationError": "Failed to score the generated stack: poison getter", "passed": false, "score": { "score": 0, "grade": "F", "valid": false } }] }The trap the ruling named, and how it is kept shut
A guard that swallowed the throw and scored a poisoned stack as passing would be worse than the crash — a loud failure turned into a quiet wrong answer. Three things keep it out, and the third is the one that is easy to miss:
passed: false), the report saysok: false, and the process still exits non-zero;0 / F / valid: false, deliberately notscoreMetadata({}). The throwing-generator path substitutes an empty stack, and an empty stack scores 100 / A /valid: true(pinned inscore.test.ts). Reusing it here would have stamped a clean-looking verdict on a stack nobody could parse. As the ruling put it: an unnormalizable stack is not a stack.a silent swallow would be caughtpins that negative directly, in both the unit and the e2e file.The nine negative controls are unchanged, and that is measured
Phase 1's evidence that the class is narrow. All re-driven on this branch, and every one still answers exactly as before: a generator that throws; six off-shape plain-JSON shapes; 200k-deep nesting; a bare string; a missing file; a path pointing at a directory; a syntax error; no default export; plus the offline baseline (exit 0, 5/5 pass).
⛔ The one that matters most for this fix: ordinary bad metadata is still SCORED, never rerouted into the failure channel. An off-shape stack comes back with
generationErrorabsent andschemaErrorsgreater than zero — that is what fails it. Six of those are pinned as anit.eachin the e2e file.Ablation
Predicted before running: reverting
metadata-eval.tsto its pre-fix blob turns the two poison drives red and leaves all fourteen control rows byte-identical.2b05139d5c9).exit 1 · stdout 0 bytes; the control fingerprint diff shows exactly two changed rows out of sixteen — the two poison ones. 8 of the new assertions went red.7986fe93ee2),git diff HEADempty,git statusempty. Run under atrap ... EXIT INT TERMwith an absolute path.src/through tsx: the ablated behaviour changed with no build step in between.Bump:
patch, argued rather than assumedThe
WHICH LEVELrule reserves at leastminorfor "a purely additive widening of a published package's public surface (a new exported symbol on anindex, a new accepted key or value)", and says "afix(that changes no public surface stayspatch."None of the three widening forms is present. No new exported symbol: the sentinel is a module-private function, and
src/lint/metadata-eval.tsis not on the package index at all —@objectstack/cliexports only.and./console. No new accepted key or value: the--generatormodule contract is untouched and the same generators are accepted. No new key on the payload:generationErroris a pre-existing optional field onMetadataEvalCaseResult, already emitted for the throwing-generator path.What changed is that an exit which was already there is now reachable for one more class of failure. That is a bug fix.
One thing found and deliberately not widened
An early draft of the offline test used a case object whose own
fixturegetter throws, and it failed:let stack = c.fixtureis a read of the caller's own case entry, outside every guard. That is a broken argument rather than a failed case — there is no id to report it under — and no in-repo caller can reach it, sinceos lint --evalpasses a static corpus. Rather than over-claim, the docblock now states the boundary explicitly instead of implying totality it does not have. Over-claiming in a docblock is the defect this card is about, so it seemed the wrong place to repeat it.Scope
One PR, one defect. Not merged with any sibling, per the ruling. Deliberately untouched and still open: the
conversionsbranch fenced by #14015; #15547; #15549; #15550.Verification
pnpm --filter @objectstack/cli typecheck— green, includingcheck:test-typecheck(3 files / 28 errors held in the shrink-only ledger, unchanged; the new test file adds none). Both new test files confirmed present in the tsc program via--listFiles, so "typecheck is clean" actually covers them.pnpm --filter @objectstack/cli exec vitest run— the WHOLE package, not just the touched suites: 252 files, 2934 passed, 6 expected-fail, exit 0 (VERDICT command-exit 0, held the shared verify lock 1133s).pnpm lint— the full repo-wideeslint . --no-inline-config, exit 0. Not narrowed.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackon a non-stale tree, all green ata79148386ac. Two returned exit 3 (PREREQUISITE NOT MET, an unbuilt workspace) on the first pass, which isNOT MEASUREDrather than red; both green afterturbo run build.Every exit code above was captured by redirect-then-capture, never read through a pipe, and each gate's verdict is quoted from the line the gate itself prints.
🤖 Generated with Claude Code
https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
Generated by Claude Code