Skip to content

fix(cli): os lint --eval --json reports an unscorable stack instead of crashing - #15576

Merged
os-litant merged 2 commits into
mainfrom
claude/issue-14974-lint-eval-json-error-face
Sep 4, 2026
Merged

os-litant merged 2 commits into
mainfrom
claude/issue-14974-lint-eval-json-error-face

Conversation

@os-litant

Copy link
Copy Markdown
Contributor

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 --json face.

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:

os lint --eval --json --generator ./g-getter.mjs
exit 1 · stdout 0 BYTES · stderr "    Error: poison getter"

JSON.parse on that stdout throws Unexpected end of JSON input — there is no document at all. Eval mode dispatches above the project-lint try and returns, so that mode's catch-all JSON exit can never see it; and lint.ts hand-rolls json as a plain Flags.boolean rather than oclif's enableJsonFlag (zero occurrences anywhere in packages/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 try wrapped only stack = await options.generate(...); the const score = scoreMetadata(stack); on the next line sat outside it. A generator that threw was captured into generationError, 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.ok is 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 unprotected normalizeStackInput. That would have been incomplete, and the measurement says so: scoreMetadata walks the stack twice, and both walks are reachable throw sites.

poison placement throw site frame
top-level key normalizeStackInput's { ...input } spread score.ts:66
one level down ObjectStackDefinitionSchema.safeParse — the shallow spread copied the child by reference, so the getter fires inside zod score.ts:69

Both driven, with stack traces at --stack-trace-limit=100. A guard around normalizeStackInput alone leaves the second one escaping.

So the guard goes around the whole scoreMetadata call, in metadata-eval.ts. Three reasons, in order of weight:

  1. It covers both walks, and every future one inside scoreMetadata.
  2. It is where the promise is made and where the channel lives — generationError is populated a dozen lines up, by the throwing-generator path this failure now joins.
  3. Zero blast radius on the other caller. scoreMetadata has a second caller, os lint --score at lint.ts:613, which already sits inside the project-lint try and so already has a JSON net. Changing scoreMetadata itself would have moved that mode too.

The after-shape, same invocation, same published entry

exit 1 · stdout 3551 bytes · JSON.parse OK · stderr 0 bytes
{ "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:

  • the case fails (passed: false), the report says ok: false, and the process still exits non-zero;
  • the cause is named, on the per-case channel that already existed;
  • the score is 0 / F / valid: false, deliberately not scoreMetadata({}). The throwing-generator path substitutes an empty stack, and an empty stack scores 100 / A / valid: true (pinned in score.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 caught pins 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 generationError absent and schemaErrors greater than zero — that is what fails it. Six of those are pinned as an it.each in the e2e file.

Ablation

Predicted before running: reverting metadata-eval.ts to its pre-fix blob turns the two poison drives red and leaves all fourteen control rows byte-identical.

  • Mutation proven on disk before any reading: injected marker count 0, removed text count 1, and the mutated blob hash equal to the base blob (2b05139d5c9).
  • Ablated result: both poison drives back to 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.
  • Restore proven: blob hash equal to the HEAD blob (7986fe93ee2), git diff HEAD empty, git status empty. Run under a trap ... EXIT INT TERM with an absolute path.
  • No rebuild between legs, and that is itself the proof the measured path is src/ through tsx: the ablated behaviour changed with no build step in between.

Bump: patch, argued rather than assumed

The WHICH LEVEL rule reserves at least minor for "a purely additive widening of a published package's public surface (a new exported symbol on an index, a new accepted key or value)", and says "a fix( that changes no public surface stays patch."

None of the three widening forms is present. No new exported symbol: the sentinel is a module-private function, and src/lint/metadata-eval.ts is not on the package index at all — @objectstack/cli exports only . and ./console. No new accepted key or value: the --generator module contract is untouched and the same generators are accepted. No new key on the payload: generationError is a pre-existing optional field on MetadataEvalCaseResult, 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 fixture getter throws, and it failed: let stack = c.fixture is 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, since os lint --eval passes 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 conversions branch fenced by #14015; #15547; #15549; #15550.

Verification

  • pnpm --filter @objectstack/cli typecheck — green, including check: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-wide eslint . --no-inline-config, exit 0. Not narrowed.
  • 51 gate families, re-derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack on a non-stale tree, all green at a79148386ac. Two returned exit 3 (PREREQUISITE NOT MET, an unbuilt workspace) on the first pass, which is NOT MEASURED rather than red; both green after turbo 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

… 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
@github-actions github-actions Bot added the size/m label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

4 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
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 22 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 66086d06ad8d688c7c1a505dbe008acc53d64637 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from d4a5da13b8bcd8e180f4e7781bd6e81bbb1b0cc0 — the merge of head a79148386ac5ad366049566789f50c5fb4df5f27 into base 66086d06ad8d688c7c1a505dbe008acc53d64637, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# 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

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants