feat: fix codex runner, add --override-doc grading, port skillgym quiz cases to help-conformance bench#1176
Conversation
…cases The 2026-07-09 evaluation of scripts/help-conformance-bench.mjs found it structurally right but broken for the codex runner (two bugs), thin on coverage (4 cases), sequential, and unable to grade a draft help rewrite without a rebuild. - Fix runCodex: (1) codex exec reads stdin until EOF when not attached to a TTY, and execFile never closes the child's stdin, so every codex call hung until RUN_TIMEOUT_MS with empty output — close stdin right after spawn. (2) `-o outFile` writes the same final JSON that codex also prints to stdout, so concatenating both produced two back-to-back JSON objects that broke every JSON.parse candidate and silently zeroed extractCommands() — prefer the clean -o payload, fall back to stdout only when it's empty. - Add `--override-doc <topicId>=<path>` (repeatable): loads a topic's text from a file instead of shelling out to `node bin/agent-device.mjs help <topic>`, so a draft help rewrite can be A/B graded with zero rebuild. - Port three cases from test/skillgym/suites/agent-device-smoke-suite.ts (settle-diff-is-observation, sample-output-settled-diff-next-target, sample-output-not-settled-needs-observe) as self-contained "next-command quiz" cases, generalizing the scorer to support regex matchers/forbidden patterns alongside the existing named expectations. Fixture output text matches the CURRENT settle rendering in src/commands/interaction/output.ts, including the "unchanged interactive (N):" tail added by #1167/#1172. - Parallelize the runner x case matrix with a concurrency cap (HELP_BENCH_CONCURRENCY, default 4); results still print in the original matrix order. - Extend test/skillgym/README.md's existing pointer to this bench with the new flags. Validated with real LLM calls (both runners, all 7 cases, 14 calls, ~$0.25 total): 13/14 pass; the one fail (claude-haiku-4-5 on dogfood-mode) is a genuine model miss (returned an empty command plan asking for the app name instead of committing to a generic plan), not a bench bug. `--override-doc` demonstrated live: stripping the dogfood doc's evidence-command examples regresses codex:gpt-5.4-mini from 3/3 to 2/3 on the same case, showing the flag both loads and changes grading.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Independent adversarial review, done in a scratch worktree off origin/feat/help-iteration-bench (47dd67c). Verified by reading the diff, exercising the script directly (built the CLI, ran dry-run + live invocations), and re-running the gates.
Findings, severity-ranked
1. HIGH — --override-doc for --help:first30 skips the real 30-line truncation, invalidating the A/B comparison for every case
loadDoc for the real path does firstLines(await cliHelp(['--help']), 30), but the override path is just (await readFile(overridePath, 'utf8')).trim() — no firstLines(text, 30) applied. Every one of the 7 cases lists --help:first30 as its first (or only) doc, so this is the primary, not edge-case, code path.
Reproduced directly on the branch:
$ node scripts/help-conformance-bench.mjs --case raw-first-screen-bluesky \
--override-doc "--help:first30=/tmp/big-override-doc.txt" --dry-run --runner claude:claude-haiku-4-5
with a 49-line override file — the generated prompt contained line 31 and line 49, i.e. everything past the 30-line cap the live --help path always enforces. A draft doc under 30 lines "passes" cheaper than it should relative to the real thing, and a draft over 30 lines gets graded on content a real user would never see truncated — exactly the apples-to-oranges failure this flag exists to prevent. Suggest applying the same firstLines(text, 30) to the override branch when docId === '--help:first30' (or, more generally, run the override text through whatever post-processing the real path would have applied for that doc id).
2. MEDIUM — unknown/mistyped --override-doc topicId is silently ignored, no error or warning
overrideDocs.get(docId) is only consulted for doc ids the selected cases actually request; an override for a topic id that matches none of them (typo, wrong case id, stale flag left over from a renamed case) is simply never read. Reproduced: --override-doc "totally-bogus-topic=/tmp/x.txt" --dry-run exits 0 with no message, and the real doc is used instead. For a flag whose whole purpose is "trust me, I graded the draft," a silent no-op on typo is a real footgun. Suggest logging a warning (or hard-erroring) for any --override-doc key left unconsumed after loadDocs runs.
3. LOW — malformed/missing override file surfaces as a raw unhandled stack trace
readFile(overridePath, 'utf8') in loadDoc isn't wrapped in try/catch, so a typo'd path (or a trailing-= empty path) throws an ENOENT straight through main() as a Node stack trace rather than a clean Error: ... message. Acceptable for a local dev tool per the task's stated bar, but a one-line try/catch with a clearer message would be nicer.
4. LOW — repeated --override-doc for the same topic id (last-wins) is undocumented
Confirmed via Map.set semantics and a live test (two --override-doc --help:first30=... flags, second one wins). Not a bug, just worth a line in the README/error text since it's not obvious from the flag name alone.
Verified sound (no issues found)
- codex stdin fix:
pending.child?.stdin?.end()runs synchronously right afterexecFileAsync(...)returns and before anyawait, so it applies to every code path (success, non-zero exit, timeout) — not just a happy path.pending.childis populated synchronously becauseutil.promisify's custom executor forchild_process.execFileassignspromise.childbefore returning, andexecFilespawns synchronously — no race. - codex output sourcing: now prefers the
-ooutFile and only falls back tostdoutwhen the file read throws or is empty after.trim(). No undefined-deref path in any of the observed failure modes (missing file, empty file, orpendingitself rejecting on timeout — caught two levels up inrunCaseRawOutput, which never rethrows). Also fixed a real latent collision: the-ofilename now includes a random suffix in addition toDate.now(), which matters now that same-model codex calls can run concurrently. - Ported quiz fixtures: checked byte-for-byte against
src/commands/interaction/output.ts(unchanged by this PR) —Tapped @eN (x, y),Filled N chars(char count verified against the literal fixture text, e.g. "callstack" → 9),settled after Nms: +A -R (~U unchanged), theunchanged interactive (N):tail with= @eN [role] "label"lines, and lowercasehint:+ the exactNEVER_SETTLED_HINTstring all match current source exactly. Diffed against the original skillgym cases intest/skillgym/suites/agent-device-smoke-suite.tsthese were ported from — the old fixtures used genuinely stale shapes (Filled id="x" with "y",settled:true refsGeneration: N,Changed:, capitalizedHint:), confirming this is a real fix, not a lateral rewrite. - Regex matchers: word-boundary usage checked adversarially (
\bclose\bcorrectly rejectsdisclose,--settle\bcorrectly rejects--settled);RAW_COORDINATE_TARGETis copied verbatim from the source suite. Case 3's loosening from the original'swait\s+stableto a barewait\bis intentional, not a regression — it now also acceptswait text .../wait <selector>per the CLI's actualwaitcommand variants, matching the case's own hint text ("wait for specific content, or take a fresh snapshot," not just "wait stable"). - Parallelization:
mapWithConcurrencycaps concurrency correctly atHELP_BENCH_CONCURRENCY(default 4), no race on the sharednextIndexcounter (noawaitbetween read and increment), and results are written back toresults[current]by original index so matrix-order printing is preserved regardless of completion order. A rejected entry can't take down the run becauserunCaseRawOutputcatches every runner error internally and returns a result object instead of throwing. - Scope: diff touches only
scripts/help-conformance-bench.mjsandtest/skillgym/README.md, confirmed viagit diff origin/main...origin/feat/help-iteration-bench --stat. - Gates:
lint,typecheck,format:check,check:layering, andfallow audit --base origin/mainall pass clean in an isolated worktree. Full CI is green (the one previously-pendingSmoke Testsjob finished passing). - Live end-to-end check: ran
node scripts/help-conformance-bench.mjs --case settle-diff-is-observation --runner claude:claude-haiku-4-5for real on the branch →PASS claude:claude-haiku-4-5 settle-diff-is-observation 8/8, with the model correctly answeringagent-device close. Confirms the tool executes end-to-end and the ported fixture is realistic enough for a small model to reason over correctly.
Verdict
Needs changes before merge: finding #1 is a confirmed, reproducible bug that undermines the stated purpose of --override-doc for the doc id every case actually uses. #2 is a real footgun worth at least a warning. Everything else (the codex fixes, the ported fixtures, the parallelization, the scope) is solid and verified.
…ad overrides Review findings on the initial version (all reproduced): - HIGH: an override for the --help:first30 doc id skipped the live path's firstLines(text, 30) cap, so a 49-line draft leaked lines 31-49 into the prompt — grading content a live run never shows, on the doc id every case uses. loadDoc now splits source (live shell-out vs override file) from post-processing, and the post-processing applies to both, so an override differs ONLY in where the text comes from. - MEDIUM: an --override-doc topic id no selected case uses was silently ignored (exit 0, real doc graded). Now fails fast listing the valid doc ids for the selection. - LOW: a missing override file threw a raw ENOENT stack trace; expected failures now print one clean Error line. Added --help usage text that documents last-wins semantics for repeated same-topic overrides and the post-processing parity. Guard tests (scripts/__tests__/help-conformance-bench.test.ts, wired into the unit-core vitest project by explicit path): a 49-line fixture whose prompt must keep line 30 and drop line 31, unknown-topic fail-fast with valid ids listed, clean no-stack error for a missing file, and last-wins for repeated overrides. All spawn the script in --dry-run with every required doc overridden, so they need no LLM calls and no built CLI. Live re-validation: a 33-line override of --help:first30 whose lines 31-33 instruct the model to emit a sentinel command; neither claude-haiku-4-5 nor codex:gpt-5.4-mini emitted it (both scored 4/4, matching the live-doc baseline), proving the cap applies end-to-end.
|
Thanks for the thorough review — all four findings reproduced and fixed in ca56396, severity order:
The guard tests live in Gates re-run clean (lint, typecheck, format:check, check:layering, fallow audit — now 4 changed files). PR body updated with the post-fix evidence. |
|
Re-review of ca56396: the actionable findings are resolved. Override and live doc sources now share postProcessDoc, so --help:first30 preserves the same 30-line projection; unused override IDs fail fast; unreadable files produce a focused error; and the new unit-core tests reproduce each reviewed failure. The updated scope is coherent, all 21 checks pass, and no device evidence is required for this tooling-only change. No actionable blockers remain. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
Follow-up to the 2026-07-09 evaluation of
scripts/help-conformance-bench.mjs, which found it structurally right but: (a) broken for the codex runner, (b) only 4 cases, (c) a sequential runner loop, (d) no way to grade a draft help rewrite without rebuilding.runCodex):codex execreads stdin until EOF when it isn't attached to a TTY, and Node'sexecFilenever closes the child's stdin pipe on its own — every codex call blocked on "Reading additional input from stdin..." until it hitRUN_TIMEOUT_MS(90s) and reported empty/error output. Fixed by ending the child's stdin right after spawn.-o outFilewrites the same final JSON message that codex also prints to stdout when not attached to a TTY. The old code concatenatedstdout + '\n' + lastMessage, producing two back-to-back JSON objects that broke everyJSON.parsecandidate inparseJsonPayload, silently zeroingextractCommands()on every codex case. Fixed to prefer the clean-opayload and fall back to stdout only when it's empty.--override-doc <topicId>=<path>(repeatable; last occurrence per topic wins): loads a topic's text from a file instead of shelling out tonode bin/agent-device.mjs help <topic>for that one topic, so a draft help rewrite can be A/B graded with zero rebuild. Per review: override text goes through the same post-processing as the live source (the--help:first30doc id stays capped to its first 30 lines), an override topic id no selected case uses fails fast listing the valid doc ids, and a missing/unreadable override file prints one clean error line instead of a stack trace.--helpprints usage documenting all of this.test/skillgym/suites/agent-device-smoke-suite.ts(settle-diff-is-observation,sample-output-settled-diff-next-target,sample-output-not-settled-needs-observe) as self-contained "next-command quiz" cases. Generalized the scorer (resolveChecks) so a case can declare regexmatchers/forbiddenpatterns alongside the existing namedexpectations. Fixture output text was rewritten to match the current settle rendering insrc/commands/interaction/output.ts(confirmed againstformatSettleText/formatSettleTailLines/success-text.ts, not the stale wording in the original skillgym fixtures), including the"unchanged interactive (N):"tail added by feat: include unchanged interactive refs in settle output #1167/fix: make settle tail list real actionable targets #1172, and the real"Tapped @eN (...)"/"Filled N chars"/ lowercase"hint: ..."message shapes.HELP_BENCH_CONCURRENCY, default 4); results still print in the original matrix order for readability.--cases/--caseand--runners/--runnerfilters already existed — no change needed there.test/skillgym/README.mdwith the new flags and override semantics.scripts/__tests__/help-conformance-bench.test.ts, wired into theunit-corevitest project by explicit path so the stray unmatchedscripts/integration-progress-model.test.tsstays out of scope): a 49-line override fixture whose prompt must keep line 30 and drop line 31; unknown-topic fail-fast with valid ids listed; clean no-stack error on a missing file; last-wins for repeated same-topic overrides. All run the script in--dry-runwith every required doc overridden — no LLM calls, no built CLI needed.Review response (severity-ranked findings, all addressed in ca56396)
firstLines(text, 30):loadDocnow splits source (loadDocSource: live shell-out vs override file) from post-processing (postProcessDoc), and post-processing applies to both, so an override differs only in where the text comes from. Re-ran the reviewer's exact repro (49-line override of--help:first30, dry-run): prompt now contains line 30 but not line 31 or line 49. Locked in by unit test.assertOverrideDocIdsfails fast before any doc loads:Error: --override-doc topic id(s) not used by the selected cases: totally-bogus-topic. Valid doc ids: --help:first30.(exit 1, no report written). Locked in by unit test.Error: --override-doc file for "--help:first30" is not readable: /nope/missing.txt (ENOENT). Locked in by unit test (asserts no stack frames on stderr).--helpusage text and the README pointer. Locked in by unit test.Evidence: live truncation proof (post-fix, 2 LLM calls)
Honeypot demonstration: overrode
--help:first30with a 33-line file — the real--helpfirst 30 lines plus lines 31–33 instructing the model that its plan MUST begin with the sentinelagent-device frobnicate-xyzzy --confirm:Neither model emitted the sentinel and both scored 4/4 — identical to the live-doc baseline, as expected since the capped override is byte-identical to the live first 30 lines. Combined with the dry-run byte-check (line 31 absent from the prompt), this proves the cap applies end-to-end.
Evidence: real bench run (both runners, all 7 cases, 14 LLM calls, ~$0.25)
13/14 pass. The one fail is a genuine model behavior, not a bench bug:
claude-haiku-4-5returnedcommands: []and asked a clarifying question instead of committing to a generic dogfood plan — a real signal about that model's hedging on underspecified tasks, confirmed in the raw transcript.Evidence:
--override-docgraded delta (real A/B)Same
dogfood-modecase with thedogfoodhelp text replaced by a draft stripped of its "Evidence commands"/"Evidence rules" sections:codex:gpt-5.4-miniregressed 3/3 → 2/3 once the evidence-command examples were stripped — the zero-rebuild A/B workflow this flag exists for.Test plan
pnpm build,pnpm lint,pnpm typecheck,pnpm format:check,pnpm check:layeringall cleannpx fallow audit --base origin/main— no issues in the 4 changed filesnpx vitest run scripts/__tests__/help-conformance-bench.test.ts— 4/4 passing (each ~150 ms, inside the slow-test budget)node scripts/help-conformance-bench.mjs --dry-run— all 14 runner×case prompts build correctly, no LLM callsDeliberately skipped / deferred
HELP_BENCH_CONCURRENCYonly) — not requested, kept minimal.scripts/integration-progress-model.test.tsis unmatched by any vitest project (pre-existing); left out of scope rather than silently waking it in this PR.