feat(evals): empirically test the delegation pipeline — binary before/after + measured native baseline (#1670) - #1671
Merged
Merged
Conversation
…, #1669 merged) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-7, task 009) Replace the provisional self-reported kill-probes with a mechanically-run, diff-scoped mutation gate so the code-quality A/B's adequacy score is MEASURED, not claimed (DR-7 fail-honest). - grade.ts: add gradeAdequacy, which reuses the repo's production kill-probe (runProbe / check_test_adequacy, "mutation-testing-at-N=1") verbatim — no hand-rolled mutation engine. It synthesizes the diff base per produced impl by initializing a throwaway git repo, committing the task stub (impl.stub.ts) as the base commit, and placing the produced impl + its tests as the working tree, so the diff-scope = stub→impl. The probe reverts the impl source hunks to the stub, re-runs the produced tests, and reports killed (non-vacuous) vs survived (vacuous). The throwaway repo is always torn down. Score recorded alongside the existing oracle/typecheck/wroteTests metrics (additive) and rendered in both report tables. - grade.ts: refactored into import-safe exported functions guarded by a main() entry so the test can import without running the script. - grade.test.ts: discrimination test drives the REAL gate over a real throwaway repo (genuine suite -> killed/1, vacuous -> survived/0); characterization test pins the existing cells byte-identical to results.json. - vitest.config.ts: narrow the docs exclude to the run-artifact dirs (docs/**/runs/**) and include the eval grader's own test so the mechanical gate can actually be run by npm run test:run (and by the kill-probe). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…005) parseTaskBlocks/extractDescriptionSpan matched only `### Task` (3 hashes), so extractTaskRiskTier silently dropped tiers on the ~7 of 11 docs/specs authored entirely with `#### Task` headers. Port the heading + id handling to the SoT dispatch parser (parse-task-stamps.ts): accept both `###` and `####` and the broader id token (T-NN / TNN / bare number / dotted N.M), without regressing the legacy 3-hash / T-NN form (parenthetical-id 3-hash specs still parse — unlike a verbatim SoT copy). Templates aligned on h3 (`### Task`), the depth every plan-authoring gate requires (check_plan_coverage, check_provenance_chain, check_task_decomposition); the gate additionally tolerates legacy h4 for the existing corpus. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (DR-5, HIGH/boundary)
Drive real headless Claude Code (claude -p --output-format stream-json) so it treats a spec as its plan and delegates per-subagent, then parse the transcript for per-subagent model, tool behavior, and token spend. Proven on two live runs (both fixtures are faithful trims of real captures). FINDING (retires NATIVE_FLAT_MODEL='opus'): native CC subagents INHERIT the session model — all subagents ran on claude-sonnet-5, the session model; native routes a FLAT model (whatever --model selected), not a mix. result.modelUsage held exactly one key in both runs. Robustness: per-MESSAGE subagent model attribution is unreliable across runs (in one variant the subagents' assistant messages weren't streamed to the parent transcript). The robust signal is result.modelUsage; resolveSubagentModels back-fills the sole session model (modelSource='session-single', a measured inference) and leaves multi-model sessions unresolved rather than guessing. Fail-honest (DR-7): zero-subagent transcripts route to a BLOCKED record with no modelDistribution — never a fabricated/modeled substitute; records stamped + honesty-checked through the Task-001 provenance module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ks 003+004) Drive prepare_delegation through each of four built reference binaries' real MCP tool surface (spawned `<binary> mcp` stdio server — never the pure TS function) over the stamped docs/specs/ corpus, and diff the returned taskClassifications. Replaces the prior MODELED benchmark (pure-function call, the #1669 sin) with a mechanical, binary-boundary measurement. - exp1-binary-driver.ts: pure diff core (diffClassifications — symmetric + complete), corpus loader, MCP invocation, provenance stamping, CSV emission, and a reproducible runExp1/main entry. - exp1-binary-driver.test.ts: property tests on the diff core + provenance/CSV unit tests + a real-binary fixture-pair integration test (skip-when-absent). - binaries.provenance.json: 4 binaries stamped via stampProvenance (task-001 shape), released-window confounds enumerated (#1669 fix, #1659 confound). - exp1-before-after.csv: per-(pair,spec,task) before/after diffs, both pairs. Result (measured, DR-7): causal pair = 90/124 tasks changed tier/verification; released pair = 90/124 (with #1659 co-resident as documented confound). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ing (DR-4/DR-7, task 010) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hanical grading (DR-4/DR-7)
…DR-7, task 007) MECHANICS.md documents the four DR-3 spike questions (spec-as-plan drive, per-subagent model+token capture, identical-task-set comparison, measured native distribution) and the DR-7 fail-honest/SDK-fallback branch. exp2-native-baseline.csv: 6 measured subagent rows across 2 real claude -p runs, all claude-sonnet-5, inheritsSingleModel=true — retires the NATIVE_FLAT_MODEL='opus' assumption. Provenance-stamped (Task 001). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… 011) generate_charts.py (pure-stdlib SVG, mirroring the bifrost reference which is itself stdlib not matplotlib) reads the 3 committed CSVs and emits 3 committed, byte-deterministic SVGs with no network: - chart-exp1-verification-depth.svg (before/after tier + per-check counts; 90/124 changed; causal==released) - chart-exp2-native-distribution.svg (6/6 subagents claude-sonnet-5; distinct=1; retires opus assumption) - chart-exp3-correctness-vs-process.svg (oracle clean null E=N; durable tests E 12/12 vs opus-N 3/6, sonnet-N 0/6) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bifrost-style dated benchmark: environment/provenance table, 3 embedded figures with descriptive alt-text, executed conclusions for verification depth (Exp1: 90/124, causal==released), model selection (Exp2: native inherits session model, opus assumption retired), and correctness under under-specification (Exp3: clean null, durable-tests E 12/12 vs N 3/6·0/6). All 13 links/figures resolve to committed files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…DR-6/DR-7, task 013) Lift the 'directional/pending' framing only for claims #1670 now backs, keep explicit caveats on claims still modeled: - 1636 corpus: banner -> 'superseded by executed #1670'; inline pointers at Dimension 1 (native baseline measured, Exp2) + Dimension 2 (E-vs-H0 executed, Exp1); explicit STILL-MODELED caveat on the H1 heuristic ceiling + the model x risk-tier cross-tab (not re-executed). - quality-ab ANALYSIS: banner -> 'executed and confirmed by #1670'; process delta reproduced under mechanical mutation gate, correctness null extended to under-spec; honest standing scope caveat kept (note-level steer, fully-spec, small n). Limitation bullets (spec-completeness, self-reported kill-probe) updated to resolved-by-Exp3. Both cross-link the new benchmark; all links resolve. (RESULTS.md/grade.ts banner left as-is: accurate for the fully-specified round, out of task scope.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tive CSV Adversarial review (verdict PASS, no HIGH) raised 1 MEDIUM + 5 LOW honesty/ hygiene findings; applied the cheap high-value ones (a provenance-honesty feature should not ship over-claims): - Exp2 model-selection conclusion softened to match the N=2 spike: both runs pinned --model sonnet, so it shows native did not OVERRIDE the pin per-subagent, not that native has no default routing; 'not a model-cost optimizer' marked directional (leans on the still-modeled deterministic arm). (benchmark + MECHANICS) - Exp3 durable-test delta reframed as an E-REGIME effect (E prompt also asks for a test file; N does not) — the steer's clean own-contribution is the mechanical adequacy of the tests E wrote. (benchmark) - exp2-native-baseline.csv now DERIVED from the fixtures by a committed, tested generator (emit-baseline-csv.ts) that stamps each row via stampProvenance + a --check drift mode — closes the reproducibility gap; MECHANICS claim now true. - Exp3 Environment row relabelled 'steer pin / no binary executed' (Exp3 runs no exarchos binary). - parseTaskBlocks 'lockstep' comment softened (parsers converge on numeric-id corpus; differ on non-task-header rejection). Verified: root+MCP tsc 0, CSV --check byte-identical, charts deterministic, all doc links resolve. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds executable Exp 1 binary validation, a headless native delegation baseline, under-specified quality A/B evaluation with mechanical adequacy grading, shared provenance validation, task-heading parsing fixes, captured evaluation runs, deterministic charts, fixtures, and test coverage. ChangesEvaluation infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EvalRunner
participant ClaudeOrBinary
participant ParserOrMCP
participant Grader
participant ArtifactWriter
EvalRunner->>ClaudeOrBinary: execute delegated evaluation
ClaudeOrBinary->>ParserOrMCP: emit transcript or task classifications
ParserOrMCP->>Grader: provide observed results
Grader->>ArtifactWriter: write measured, blocked, CSV, or chart artifacts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
docs/evals/native-baseline/emit-baseline-csv.ts (1)
95-103: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOnly
descriptionis CSV-quoted; other free-text columns aren't.
s.status,dist.attributionMode, andNOTE[s.modelSource]are interpolated raw into the CSV row. None of today's values contain a comma, so the file is valid now, but a future edit toNOTE's wording (e.g. adding a clarifying comma) would silently corrupt column alignment for downstream CSV readers.♻️ Proposed fix: quote all free-text fields uniformly
- String(s.toolUses), s.status ?? '', String(r.subagents.length), - String(dist.distinctModelCount), String(dist.inheritsSingleModel), dist.attributionMode, - num(cost), 'measured', provenance.binaryTag, provenance.gitSha, provenance.date, - provenance.modelIds.join('|'), NOTE[s.modelSource] ?? '', + String(s.toolUses), q(s.status ?? ''), String(r.subagents.length), + String(dist.distinctModelCount), String(dist.inheritsSingleModel), q(dist.attributionMode), + num(cost), 'measured', provenance.binaryTag, provenance.gitSha, provenance.date, + provenance.modelIds.join('|'), q(NOTE[s.modelSource] ?? ''),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/native-baseline/emit-baseline-csv.ts` around lines 95 - 103, Quote all free-text CSV fields consistently in the row assembled by the baseline emission logic, including s.status, dist.attributionMode, and NOTE[s.modelSource], using the same q helper already applied to s.description. Also apply q to any other textual values that may contain commas, such as provenance.modelIds.join('|'), while leaving numeric fields unchanged.docs/evals/data/2026-07-09/generate_charts.py (1)
192-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead statement — leftover disabled gridline draw.
lines.append(line(...)) if False else Nonenever executes and reads like an accidentally-left debug toggle. Either wire it up or drop it to avoid confusing future editors of this generator.🧹 Proposed cleanup
lines.append(line(x - 4, yy, x, yy, stroke=FG, width=1.0)) - lines.append(line(x, yy, x + 9999, yy, stroke=FG, width=0.4, dash="2 4")) if False else None lines.append(text(x - 8, yy + 4, fmt(t), size=11, anchor="end"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/data/2026-07-09/generate_charts.py` around lines 192 - 196, Remove the unreachable conditional gridline statement inside the tick loop of the chart-generation function, leaving only the active tick mark and label rendering; do not alter the behavior of the surrounding `ticks` iteration.docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.test.ts (1)
1-2: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a NodeNext-compatible relative specifier, or document the runner exception.
This Vitest file imports
./implwithout an extension. Under strict NodeNext ESM, compiled relative imports should use./impl.js; Vitest may mask this during tests. Verify the captured-artifact toolchain before merge.As per coding guidelines, TypeScript files must use ESM modules compatible with NodeNext resolution and Node ≥20.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.test.ts` around lines 1 - 2, The Vitest test imports parseDuration from the extensionless specifier "./impl", which is incompatible with strict NodeNext ESM resolution. Update the import to use the NodeNext-compatible "./impl.js" specifier, or document and verify an explicit runner exception if the artifact toolchain requires the extensionless form.Source: Coding guidelines
docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/test.ts (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the benchmark harness exception to the Vitest convention.
This required self-executing
test.tsconflicts with the repository rule requiring co-located*.test.tsfiles using Vitest. Sincerun-underspec.tsexplicitly requirestest.tsand executes it withtsx, document this exception or add a thin launcher around a conventional Vitest test; apply the same decision to the token-bucket harness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/test.ts` around lines 1 - 5, Document the intentional self-executing benchmark harness exception in test.ts, noting that run-underspec.ts invokes it via tsx instead of Vitest; apply the same documentation or launcher approach to the token-bucket harness, using their distinctive entrypoints to keep the decision consistent.Source: Coding guidelines
docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts (1)
147-179: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReal-wall-clock timing assertions can flake under load.
Both
recordAsynccases wait real150/200ms viasetTimeoutthen assert exact refill boundaries (e.g.tryRemove(2) === falseright after a 150ms wait at 10 tokens/sec, which leaves only ~50ms of slack before a 3rd token would refill and flip the assertion). Under CI scheduler jitter or resource contention, the actual elapsed time can exceed the margin, making these "high-tier integration" cases flaky. AFakeClock-only suite (already used for the deterministic cases above) would remove this risk without losing behavioral coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts` around lines 147 - 179, Replace the real-time waits and assertions in both recordAsync cases with deterministic FakeClock-based advancement, using the existing FakeClock setup and Clock injection pattern from the surrounding tests. Advance time by precisely the refill intervals needed to verify token availability and capacity capping, and remove setTimeout-based waits and real Date.now clocks while preserving coverage of real clock collaborator behavior only if a separate non-flaky approach exists.docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/impl.ts (1)
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the unit table readonly.
Record<string, number>leaves the object mutable, contrary to the repository’s readonly-by-default TypeScript standard. UseReadonly<Record<string, number>>or a readonly literal map.Suggested fix
- const UNIT_MS: Record<string, number> = { + const UNIT_MS: Readonly<Record<string, number>> = {Also applies to: 31-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/impl.ts` around lines 12 - 18, Make the UNIT_MS unit map immutable by declaring it as Readonly<Record<string, number>> or using a readonly literal-map type, and apply the same change to the corresponding unit table referenced in the additional location.Source: Coding guidelines
docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/impl.ts (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the unit table readonly.
Record<string, number>leaves this map mutable, conflicting with the repository’s readonly-by-default TypeScript standard. UseReadonly<Record<string, number>>or a readonly literal map.Suggested fix
-const UNIT_TO_MS: Record<string, number> = { +const UNIT_TO_MS: Readonly<Record<string, number>> = {Also applies to: 30-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/impl.ts` around lines 3 - 9, Make the UNIT_TO_MS constant readonly by changing its type to Readonly<Record<string, number>> or using a readonly literal-map definition, and apply the same adjustment to the additional occurrence referenced by the review.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/evals/native-baseline/emit-baseline-csv.ts`:
- Around line 89-104: Replace the single-model cost lookup in the baseline
row-generation logic with a sum of costUSD across every entry in
r.sessionModelUsage, and use that aggregate session cost consistently for
emitted rows. Update the cost calculation near the subagent loop so mixed-model
sessions are fully represented rather than keyed only by r.subagents[0].model.
In `@docs/evals/native-baseline/harness.ts`:
- Around line 601-618: Add a finite timeout to the `execFile` invocation inside
`spawnClaude`, using the harness’s existing timeout configuration or a clearly
defined default. Preserve the current stdout, stderr, and exit-code handling,
including mapping timeout termination to a non-success exit code, so hung Claude
processes resolve promptly and callers such as `runNativeBaseline` do not block
indefinitely.
In `@docs/evals/quality-ab/grade.ts`:
- Around line 59-60: Update the TSX and TSC command setup and their
consumers—gradeOracle, gradeTypecheck, and makeEvalRunTests—to use a
Windows-safe launcher, such as invoking the package entrypoints through node or
enabling shell execution for execFileSync; do not rely on appending .cmd to the
existing paths.
In `@docs/evals/quality-ab/run-underspec.ts`:
- Around line 277-281: Resumed cells lose the precise resolved model ID because
the existing-output path in the dispatch logic returns modelId as null, causing
captureCell to fall back to cell.model. Persist the resolved model ID alongside
impl.ts when writing a fresh result, then read and validate that sidecar in the
existing-output branch and return it as dispatch.modelId; retain the alias
fallback only when the metadata is unavailable or invalid.
In `@docs/evals/quality-ab/runs/underspec/opus/csv-line__N__r2/impl.ts`:
- Around line 29-34: Update the CSV parsing loop handling characters after a
closing quote so trailing text is appended to the current value instead of
skipped; ensure malformed inputs such as `"a"b` produce `["ab"]`, while
preserving normal quoted-field parsing behavior.
In `@docs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r1/impl.ts`:
- Around line 29-40: Update the quoted-field parsing logic following the value
push so text between a closing quote and the next comma is retained rather than
skipped. In the parser’s quoted-field branch, append the trailing characters to
the current field value (or otherwise include them when constructing the field)
before consuming the separator, ensuring malformed input such as `"a"b` produces
the documented combined field value.
In `@docs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r1/impl.ts`:
- Around line 28-35: Update the CSV parsing loop after a quoted field closes to
consume immediately following non-comma text as part of the same field, rather
than breaking at the closing quote. In the relevant parsing function, append
malformed trailing characters such as the b in `"a"b,c` to the current value,
then continue until the comma or end of line so the result is `["ab", "c"]`.
In `@docs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r2/impl.ts`:
- Around line 29-34: Update the CSV parsing logic after a closing quote to
preserve intervening non-separator text by appending each trailing character to
the current field rather than discarding it. Use the parser’s existing field
accumulator and index handling so inputs like `"a"b,c` produce `["ab", "c"]`,
while still advancing to the next comma or end of line.
In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r2/impl.ts`:
- Around line 51-58: Update refill() so a backward clock reading does not modify
lastRefillMs; return immediately when now < lastRefillMs. Only update the
baseline for a stalled clock reading (now === lastRefillMs), preserving the
latest forward timestamp and preventing already-observed intervals from being
refilled again.
In `@servers/exarchos-mcp/src/evals/benchmarks/exp1-binary-driver.ts`:
- Around line 423-426: Make parseEnvelope resilient to malformed response text
by catching JSON.parse failures and returning an empty envelope object (or
equivalent fallback). Preserve the existing successful parsing behavior so
env.success !== true reaches runExp1’s blocked-result handling instead of
throwing and aborting the run; update parseEnvelope accordingly.
- Around line 408-436: Validate the binary response at the MCP boundary using
Zod instead of the unchecked cast in parseEnvelope. Define schemas for the
envelope, nested data, task classifications, and content entries, parse the
selected text with the envelope schema, and use the validated result in
toSnapshots so malformed fields such as riskTier are rejected before diff/CSV
processing; handle JSON or Zod parse failures explicitly.
- Around line 447-525: Move client.connect(transport) inside the try/finally in
runArmOverCorpus, ensuring the existing client.close() cleanup executes for
handshake and startup failures; keep result processing unchanged.
- Around line 452-458: Ensure the temporary directory created by fs.mkdtempSync
in the benchmark execution flow is removed after the StdioClientTransport run
completes, including when errors occur. Track stateDir through the relevant
execution and cleanup logic, and add recursive force removal in a finally block,
matching main()'s workRoot cleanup pattern.
---
Nitpick comments:
In `@docs/evals/data/2026-07-09/generate_charts.py`:
- Around line 192-196: Remove the unreachable conditional gridline statement
inside the tick loop of the chart-generation function, leaving only the active
tick mark and label rendering; do not alter the behavior of the surrounding
`ticks` iteration.
In `@docs/evals/native-baseline/emit-baseline-csv.ts`:
- Around line 95-103: Quote all free-text CSV fields consistently in the row
assembled by the baseline emission logic, including s.status,
dist.attributionMode, and NOTE[s.modelSource], using the same q helper already
applied to s.description. Also apply q to any other textual values that may
contain commas, such as provenance.modelIds.join('|'), while leaving numeric
fields unchanged.
In `@docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/test.ts`:
- Around line 1-5: Document the intentional self-executing benchmark harness
exception in test.ts, noting that run-underspec.ts invokes it via tsx instead of
Vitest; apply the same documentation or launcher approach to the token-bucket
harness, using their distinctive entrypoints to keep the decision consistent.
In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/impl.ts`:
- Around line 12-18: Make the UNIT_MS unit map immutable by declaring it as
Readonly<Record<string, number>> or using a readonly literal-map type, and apply
the same change to the corresponding unit table referenced in the additional
location.
In
`@docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.test.ts`:
- Around line 1-2: The Vitest test imports parseDuration from the extensionless
specifier "./impl", which is incompatible with strict NodeNext ESM resolution.
Update the import to use the NodeNext-compatible "./impl.js" specifier, or
document and verify an explicit runner exception if the artifact toolchain
requires the extensionless form.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/impl.ts`:
- Around line 3-9: Make the UNIT_TO_MS constant readonly by changing its type to
Readonly<Record<string, number>> or using a readonly literal-map definition, and
apply the same adjustment to the additional occurrence referenced by the review.
In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts`:
- Around line 147-179: Replace the real-time waits and assertions in both
recordAsync cases with deterministic FakeClock-based advancement, using the
existing FakeClock setup and Clock injection pattern from the surrounding tests.
Advance time by precisely the refill intervals needed to verify token
availability and capacity capping, and remove setTimeout-based waits and real
Date.now clocks while preserving coverage of real clock collaborator behavior
only if a separate non-flaky approach exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95e9b745-9e14-426d-aae3-0154f5f558af
⛔ Files ignored due to path filters (18)
docs/evals/2026-07-09-1636-plan-format-corpus.mdis excluded by!**/*.mddocs/evals/2026-07-09-1670-delegation-pipeline-empirical.mdis excluded by!**/*.mddocs/evals/data/2026-07-09/chart-exp1-verification-depth.svgis excluded by!**/*.svgdocs/evals/data/2026-07-09/chart-exp2-native-distribution.svgis excluded by!**/*.svgdocs/evals/data/2026-07-09/chart-exp3-correctness-vs-process.svgis excluded by!**/*.svgdocs/evals/data/2026-07-09/exp1-before-after.csvis excluded by!**/*.csvdocs/evals/data/2026-07-09/exp2-native-baseline.csvis excluded by!**/*.csvdocs/evals/data/2026-07-09/exp3-underspec-ab.csvis excluded by!**/*.csvdocs/evals/native-baseline/MECHANICS.mdis excluded by!**/*.mddocs/evals/quality-ab/ANALYSIS.mdis excluded by!**/*.mddocs/evals/quality-ab/tasks/csv-line/SPEC.underspec.mdis excluded by!**/*.mddocs/evals/quality-ab/tasks/parse-duration/SPEC.underspec.mdis excluded by!**/*.mddocs/evals/quality-ab/tasks/token-bucket/SPEC.underspec.mdis excluded by!**/*.mddocs/specs/2026-07-09-1670-delegation-empirical-testing.mdis excluded by!**/*.mdskills-src/plan/references/spec-template.mdis excluded by!**/*.mdskills-src/plan/references/task-template.mdis excluded by!**/*.mdskills/standard/plan/references/spec-template.mdis excluded by!**/*.md,!skills/**skills/standard/plan/references/task-template.mdis excluded by!**/*.md,!skills/**
📒 Files selected for processing (58)
docs/evals/data/2026-07-09/binaries.provenance.jsondocs/evals/data/2026-07-09/generate_charts.pydocs/evals/native-baseline/emit-baseline-csv.tsdocs/evals/native-baseline/fixtures/delegation-sonnet-3subagents.jsonldocs/evals/native-baseline/fixtures/delegation-sonnet-notification-only.jsonldocs/evals/native-baseline/fixtures/no-delegation-direct-answer.jsonldocs/evals/native-baseline/harness.test.tsdocs/evals/native-baseline/harness.tsdocs/evals/quality-ab/grade.test.tsdocs/evals/quality-ab/grade.tsdocs/evals/quality-ab/run-underspec.test.tsdocs/evals/quality-ab/run-underspec.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__N__r2/impl.test.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r1/impl.test.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.test.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r2/impl.tsservers/exarchos-mcp/src/evals/benchmarks/exp1-binary-driver.test.tsservers/exarchos-mcp/src/evals/benchmarks/exp1-binary-driver.tsservers/exarchos-mcp/src/evals/provenance.test.tsservers/exarchos-mcp/src/evals/provenance.tsservers/exarchos-mcp/src/orchestrate/task-decomposition.test.tsservers/exarchos-mcp/src/orchestrate/task-decomposition.tsvitest.config.ts
rsalus
marked this pull request as ready for review
July 10, 2026 20:04
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
… the steer Review #1670 finding: the first Exp3 run asked ONLY the E arm for a durable test (N got bare 'implement it'), confounding 'was steered' with 'was told to test'. Corrected: buildUserPrompt now gives BOTH arms the identical impl+test output contract; the E arm alone carries buildVerificationNote. The only cross-arm variable is the steer's CONTENT, so the measured contrast becomes test ADEQUACY (mutation kills), not test PRESENCE. Unit tests updated to assert the symmetric test request + that the post-contract suffix is byte-identical across arms. (Live re-run of the 24 cells lands the regenerated data separately.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tent adds nothing Re-ran the 24-cell E-vs-N under-spec A/B with the SYMMETRIC prompt (both arms implement+test, only the verification steer varies), opus+sonnet, real claude -p, mechanical mutation grading. Result: correctness E=N 0.921 (identical every cell) wrote tests E 12/12 · N 12/12 mutation-adequate E 12/12 · N 12/12 The first run's durable-test 'win' (E 12/12 vs opus-N 3/6, sonnet-N 0/6) was ENTIRELY the prompt asymmetry — hold the test request constant and it vanishes. So the verification steer's CONTENT shows no measured correctness or test-adequacy benefit on this corpus (scope: small tasks, n=2, adequacy gate saturates at 6/6). Doc updates: - benchmark: reframed to one conclusive result (Exp1 depth), one deferred (Exp2 model selection, blocked on #1672 + unpinned-harness gap), one clean null (Exp3). New Exp3 figure/caption; Exp2 figure demoted to 'does not measure native routing'. - ANALYSIS.md: process-delta claim REFUTED (was a prompt artifact), correctness null still holds; 1636 corpus: native-baseline 'measured' claim retracted (Exp2 deferred), Dimension-1 routing flagged as defect #1672. - exp3 CSV + 24 regenerated run dirs (both arms now carry test.ts); exp3 chart regenerated (both panels now tie, data-driven annotation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/test.ts (1)
149-181: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a monotonic clock for this integration test.
Date.now()can move backward or jump, which makes the refill timing assertions flaky; switchSystemClocktoperformance.now()or another monotonic source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/test.ts` around lines 149 - 181, Use a monotonic time source for the integration test: update SystemClock.now() to return performance.now() instead of Date.now(), preserving the Clock interface and existing test behavior.
🟡 Minor comments (9)
docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/impl.ts-32-37 (1)
32-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unsafe numeric results.
Number(amountStr)can lose precision, and multiplication can overflow even when the input syntax is valid. Add safe-integer validation before returning the total.Proposed fix
totalMs += amount * unitMs; + if (!Number.isSafeInteger(totalMs)) { + throw new RangeError('parseDuration: duration exceeds the safe integer range'); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/impl.ts` around lines 32 - 37, Validate each parsed amount and the accumulated total for safe-integer bounds in the duration parsing function before returning the result; reject inputs when Number(amountStr) is not a safe integer or when amount * unitMs or the updated totalMs is not safe, rather than returning an imprecise or overflowing duration.docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/impl.ts-43-46 (1)
43-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unsafe accumulated totals.
Each
amountmay be safe whileamount * unitMsor the sum is not. The function can therefore return a rounded or infinite duration.Proposed fix
totalMs += amount * unitMs; + if (!Number.isSafeInteger(totalMs)) { + throw new RangeError('parseDuration: duration exceeds the safe integer range'); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/impl.ts` around lines 43 - 46, Validate each converted segment and the running total in the duration parsing function before updating totalMs, rejecting values when amount * unitMs or the accumulated sum is non-finite or exceeds Number.MAX_SAFE_INTEGER; preserve the existing invalid-input behavior and ensure no unsafe rounded or infinite duration is returned.docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r1/impl.ts-41-43 (1)
41-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unsafe millisecond totals.
Large numeric segments can be rounded or become
Infinity, causing an incorrect result instead of an error. CheckNumber.isSafeInteger(total)after accumulation and throw aRangeError.Proposed fix
total += amount * UNIT_TO_MS[unit]; + if (!Number.isSafeInteger(total)) { + throw new RangeError('parseDuration: duration exceeds the safe integer range'); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r1/impl.ts` around lines 41 - 43, In the duration accumulation logic of parseDuration, validate total immediately after adding amount * UNIT_TO_MS[unit] by checking Number.isSafeInteger(total); throw a RangeError when the total is not a safe integer.docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/impl.ts-30-32 (1)
30-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unsafe accumulated totals.
Valid-looking inputs can exceed JavaScript’s exact integer range during multiplication or summation. The current implementation returns a rounded value instead of failing.
Proposed fix
totalMs += amount * UNIT_MS[unit]; + if (!Number.isSafeInteger(totalMs)) { + throw new RangeError('parseDuration: duration exceeds the safe integer range'); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/impl.ts` around lines 30 - 32, Reject duration results that are not safe integers: in the parsing logic using amount, unit, and totalMs, validate each computed unit contribution and the accumulated total with Number.isSafeInteger before assigning or returning them, and throw the existing invalid-input error when either check fails.docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts-140-149 (1)
140-149: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a monotonic clock for this integration test.
Date.now()can move backward or jump, which makes this real-timesetTimeoutcheck flaky and clashes with the monotonicClockcontract. Switch toperformance.now()or keep this path underFakeClock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts` around lines 140 - 149, Update realClockIntegrationTest to use a monotonic time source by replacing Date.now() in the realClock implementation with performance.now(), while preserving the Clock interface and existing timing checks.docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r2/impl.ts-35-40 (1)
35-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the failed-removal contract comment.
A failed removal does not necessarily leave the bucket unchanged:
refill()runs first and can update both tokens and the timestamp. This behavior is also exercised bysonnet/token-bucket__N__r1/test.tsat Lines 93-96. Clarify that failure does not consume tokens, rather than claiming the entire bucket is unchanged.Suggested wording
- * removal itself succeeds). Returns true and consumes the tokens if enough - * are available; otherwise returns false and leaves the bucket unchanged. + * removal itself succeeds). Returns true and consumes the tokens if enough + * are available; otherwise returns false without consuming tokens.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r2/impl.ts` around lines 35 - 40, Update the method comment for the token-removal function to state that a failed removal does not consume tokens, while acknowledging that the preceding refill may update the bucket’s token count and timestamp; remove the claim that the entire bucket remains unchanged.docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r1/impl.ts-45-48 (1)
45-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject durations that overflow
Number.MAX_SAFE_INTEGER.Number(match[1])and theamount * UNIT_MS[unit]addition can silently lose precision or reachInfinityon very large inputs; guard the parsed amount and running total before updatingtotal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r1/impl.ts` around lines 45 - 48, Reject unsafe or non-finite duration values in the duration parser around the amount conversion and total update: validate the parsed amount from match[1] with Number.isSafeInteger and ensure the multiplication and new running total remain finite and within Number.MAX_SAFE_INTEGER before assigning total. Return the parser’s existing invalid-duration result when any validation fails.docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/impl.ts-36-44 (1)
36-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unsafe increments and totals in
parseDuration.Number.isSafeInteger(amount)only covers the parsed chunk;amount * UNIT_MS[unit]and the running sum can still exceedNumber.MAX_SAFE_INTEGER, so large inputs lose precision. Reject overflow or switch to a wider numeric type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/impl.ts` around lines 36 - 44, Update parseDuration to validate each millisecond increment and the accumulated total, not just the parsed amount. In the amount parsing and total accumulation logic, reject values when amount * UNIT_MS[unit] or the resulting total exceeds Number.MAX_SAFE_INTEGER, or use a wider numeric representation consistently; preserve the existing error behavior and clear overflow messaging.docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/test.ts-69-69 (1)
69-69: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a type guard for unexpected thrown values.
Line 69 assumes every thrown value is an
Error; anullorundefinedthrow can crash the failure reporter while handling the original failure. Useerr instanceof Error ? err.message : String(err).As per path instructions, strict TypeScript code should use
unknownwith type guards instead of unsafe casts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/test.ts` at line 69, The unexpected-error reporter in the parseDuration test should not cast thrown values to Error, since JavaScript permits null, undefined, and other non-Error values. Update the catch handling to treat the value as unknown and use an Error type guard, formatting with err.message for Error instances and String(err) otherwise.Source: Path instructions
🧹 Nitpick comments (10)
docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/impl.ts (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the unit table readonly.
Use a readonly lookup type to prevent accidental mutation of parser configuration.
As per path instructions, TypeScript types should use a readonly default.
- const UNIT_MS: Record<string, number> = { + const UNIT_MS: Readonly<Record<string, number>> = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/impl.ts` around lines 3 - 9, The UNIT_MS lookup table is mutable but should be immutable parser configuration. Update the UNIT_MS declaration to use a readonly record type, preserving its existing keys and values.Source: Path instructions
docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/impl.ts (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the unit table readonly.
This lookup is never mutated. Use a readonly type to follow the repository’s TypeScript standard and prevent accidental reassignment.
As per path instructions, TypeScript types should use a readonly default.
- const UNIT_MS: Record<string, number> = { + const UNIT_MS: Readonly<Record<string, number>> = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/impl.ts` around lines 7 - 13, UNIT_MS is a constant lookup that should not be mutable. Update its type declaration to use a readonly record while preserving the existing unit mappings and values.Source: Path instructions
docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.ts (1)
48-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the numeric-precision comment.
\d+guarantees integer syntax, not exactNumberrepresentation. The safe-integer guard—notNumberitself—provides the precision boundary.As per path instructions,
docs/**requires accuracy and completeness checks.Proposed wording
- // `\d+` guarantees a non-negative integer; Number is exact for these. + // `\d+` guarantees a non-negative integer; the guard below rejects values + // that Number cannot represent exactly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.ts` around lines 48 - 49, Update the numeric-precision comment near the accumulation in the duration parser to state that \d+ only guarantees non-negative integer syntax, while the safe-integer validation establishes the Number precision boundary; retain or reference the existing guard rather than claiming Number is always exact.Source: Path instructions
docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/impl.ts (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the unit table readonly.
Use a readonly lookup type because this table is configuration data and should not be mutable.
As per path instructions, TypeScript types should use a readonly default.
-const UNIT_MS: Record<string, number> = { +const UNIT_MS: Readonly<Record<string, number>> = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/impl.ts` around lines 3 - 9, Make the UNIT_MS lookup table immutable by declaring it with a readonly TypeScript record type, preserving its existing unit mappings and values.Source: Path instructions
docs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r2/test.ts (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign this test with the repository’s Vitest convention.
This is a custom
test.tsscript with its own assertion and reporting framework. Rename it toimpl.test.tsand use Vitest, or verify that the evaluation harness intentionally runs it outside normal test discovery.As per coding guidelines, TypeScript tests must use the
foo.test.tspattern and import utilities fromvitest. As per path instructions, the same TypeScript test convention applies to this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r2/test.ts` around lines 5 - 16, Align this test with repository conventions by renaming test.ts to impl.test.ts and replacing the custom checks/assertEqual reporting with Vitest imports and test/expect assertions; if the evaluation harness intentionally runs this script directly, verify and document that exception instead.Sources: Coding guidelines, Path instructions
docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/test.ts (1)
7-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign this test with the repository’s Vitest convention.
This
test.tsfile uses a custom runner andprocess.exit, so it will not be discovered as a standardfoo.test.tstest. Rename it besideimpl.tsasimpl.test.tsand use Vitest assertions, or verify that captured artifacts are intentionally executed outside the repository test runner.As per coding guidelines, TypeScript tests must use the
foo.test.tspattern and import utilities fromvitest. As per path instructions, the same TypeScript test convention applies to this path.Also applies to: 63-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/test.ts` around lines 7 - 28, Replace the custom runner in test.ts with a standard Vitest test: rename the file to impl.test.ts beside impl.ts, import test utilities from vitest, and convert check and its pass/fail bookkeeping into Vitest test cases and assertions. Remove process.exit and ensure csvParseLine behavior is validated through the repository’s normal test runner.Sources: Coding guidelines, Path instructions
docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts (1)
3-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign this test with the repository’s Vitest convention.
This custom
test.tsrunner manually exits the process, so it is not a standard Vitest test module. Rename it toimpl.test.tsand use Vitest, or verify that the captured evaluation harness intentionally executes it directly.As per coding guidelines, TypeScript tests must use the
foo.test.tspattern and import utilities fromvitest. As per path instructions, the same TypeScript test convention applies to this path.Also applies to: 160-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts` around lines 3 - 43, Replace the custom runner in FakeClock and its related checks with a standard Vitest test module: rename the file to impl.test.ts, import test utilities from vitest, and convert check/expectThrows usage to Vitest assertions and test cases. If the evaluation harness requires direct execution, instead verify and document that exception while preserving the repository’s required foo.test.ts convention.Sources: Coding guidelines, Path instructions
docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/test.ts (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign this test with the repository’s Vitest convention.
This file is named
test.ts, manually tracks pass/fail state, and callsprocess.exit. Rename it toimpl.test.tsand use Vitest, or verify that this captured evaluation script is intentionally excluded from normal test discovery.As per coding guidelines, TypeScript tests must use the
foo.test.tspattern and import utilities fromvitest. As per path instructions, the same TypeScript test convention applies to this path.Also applies to: 71-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/test.ts` around lines 1 - 5, Rename the test file to impl.test.ts and convert its manual pass/fail and process.exit logic to Vitest assertions, importing the required utilities from vitest. Preserve the existing OkCase, ThrowCase, and Case coverage while using standard test/describe or it blocks; only leave the current structure if this evaluation script is explicitly excluded from test discovery.Sources: Coding guidelines, Path instructions
docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r2/test.ts (1)
7-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign this test with the repository’s Vitest convention.
This file is named
test.ts, implements its own assertion runner, and exits the process directly. Rename it toimpl.test.tsand use Vitest, or verify that the benchmark harness explicitly executes these captured scripts and excludes them from normal test discovery.As per coding guidelines, TypeScript tests must use the
foo.test.tspattern and import utilities fromvitest. As per path instructions, the same TypeScript test convention applies to this path.Also applies to: 78-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r2/test.ts` around lines 7 - 16, The test uses a nonstandard filename and custom assertion runner instead of the repository’s Vitest convention. Rename it to impl.test.ts, replace arrEq/check and direct process exits with Vitest imports and assertions, and structure the cases as Vitest tests; alternatively, explicitly document and configure the benchmark harness to execute and exclude this captured script.Sources: Coding guidelines, Path instructions
docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/test.ts (1)
1-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign this test with the repository’s Vitest convention.
This file is named
test.ts, implements a custom assertion runner, and callsprocess.exit. Rename it toimpl.test.tsand use Vitest, or verify that the captured evaluation harness intentionally executes it directly.As per coding guidelines, TypeScript tests must use the
foo.test.tspattern and import utilities fromvitest. As per path instructions, the same TypeScript test convention applies to this path.Also applies to: 184-197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/test.ts` around lines 1 - 40, Align the TokenBucket test with the repository’s Vitest convention: rename the file to impl.test.ts, replace the custom check/expectThrows runner and process.exit usage with Vitest imports and assertions, and structure the cases with describe/it or test. Preserve the existing FakeClock and coverage while using Vitest’s expect(...).toThrow assertions for failures.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r1/test.ts`:
- Around line 16-24: Update expectThrows to accept the required exception
constructor and verify the caught value is an instance of that class, recording
a failure when the wrong error type is thrown. Adjust malformed-duration test
calls to require SyntaxError and the non-string input test call to require
TypeError, covering all corresponding cases.
In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/test.ts`:
- Around line 51-67: The invalid-input tests lack coverage for safe-integer
overflow. In the invalid-input section near the existing checkThrows calls, add
an assertion that parseDuration('9007199254740992ms') throws, verifying totals
beyond Number.MAX_SAFE_INTEGER are rejected with RangeError.
- Around line 15-28: Update checkThrows to accept an expected exception
constructor and verify the caught error is an instance of that class, marking
the check failed when no error or the wrong class is thrown. Update every
checkThrows call in the test, including cases covering malformed input,
non-string input, and unsafe totals, to specify the required exception class.
In `@docs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r1/test.ts`:
- Around line 17-25: Update the throws helper to accept and assert the expected
error class, distinguishing RangeError from TypeError; revise all call sites,
including the cases around the token-bucket validation checks, to specify the
required constructor and fail when a different exception type is thrown.
- Around line 169-179: Remove the immediate post-construction assertion from
runIntegrationTests, since it relies on less than 100ms of real elapsed time.
Keep that boundary check in the deterministic FakeClock tests, and retain only
robust real-time assertions after the explicit 500ms delay.
In `@docs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r2/impl.ts`:
- Around line 54-60: Validate that the initial clock reading and every
subsequent reading used by the token bucket are finite numbers before performing
arithmetic. Update the constructor and the refill/consumption logic near the
later clock access (including the methods around lines 92–100) to reject NaN and
Infinity, preserving bucket state and throwing a TypeError consistent with the
existing clock validation.
- Around line 72-84: tryRemove must reject requests exceeding the bucket
capacity before applying TokenBucket.EPSILON. Add an explicit count > capacity
check that returns false (or otherwise preserves the documented invariant), then
perform the existing refill and tolerance-based availability check; add a
regression test using capacity 0.9999999995 and tryRemove(1) to verify it fails.
In `@docs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r2/test.ts`:
- Around line 56-66: Update assertThrows to accept an expected error constructor
and verify the caught value is an instance of that class, failing with a clear
message when the type differs; update all helper call sites, including the tests
around lines 186–216, to expect RangeError for numeric validation and TypeError
for malformed clocks.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/impl.ts`:
- Line 13: Update the duration parser built around SEGMENT_RE to trim leading
and trailing whitespace, permit whitespace-only gaps between duration segments,
and adjust cursor validation accordingly while still rejecting any
non-whitespace garbage. Ensure inputs such as “1h 30m” and “ 2d ” parse
successfully, including the related parsing and validation logic.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/test.ts`:
- Around line 50-55: Update the parseDuration assertions to match the oracle’s
whitespace rules: retain the invalid “1h x30m” case, add positive checks for “1h
30m” and “ 2d ”, and replace the negative leading-space assertion in the test
block with coverage for accepted leading/trailing and inter-segment whitespace.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/test.ts`:
- Line 70: The test incorrectly expects parseDuration to reject whitespace
between valid segments. Replace the negative check for “space between segments
throws” with a positive assertion accepting parseDuration('1h 30m'), and add
assertions confirming leading and trailing whitespace are also accepted.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/test.ts`:
- Around line 37-38: Update the duration test cases to treat trailing and
inter-segment whitespace as valid: change `1m ` to a non-throwing case and add
positive cases for `1h 30m` and ` 2d `. Keep `1 m` as invalid only if
whitespace within a segment is intentionally unsupported.
In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r2/test.ts`:
- Around line 30-38: The helper assertThrows currently accepts any exception
type, so it cannot verify the TokenBucket contract’s distinct validation errors.
Update assertThrows to accept an expected error class and assert the caught
exception is an instance of it, then update all call sites, including the
validations around the TokenBucket tests, to explicitly require RangeError or
TypeError as appropriate.
- Around line 171-185: Update the test named “clock is only read via injected
Clock, not wall-clock” to verify behavioral dependence on injected time: use a
capacity-1 bucket, drain it, advance only a mutable injected-clock value, then
assert the next token removal succeeds. Retain assertions that construction and
operations consult the injected Clock, ensuring refill logic cannot rely on
Date.now() or performance.now().
---
Outside diff comments:
In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/test.ts`:
- Around line 149-181: Use a monotonic time source for the integration test:
update SystemClock.now() to return performance.now() instead of Date.now(),
preserving the Clock interface and existing test behavior.
---
Minor comments:
In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r1/impl.ts`:
- Around line 45-48: Reject unsafe or non-finite duration values in the duration
parser around the amount conversion and total update: validate the parsed amount
from match[1] with Number.isSafeInteger and ensure the multiplication and new
running total remain finite and within Number.MAX_SAFE_INTEGER before assigning
total. Return the parser’s existing invalid-duration result when any validation
fails.
In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/impl.ts`:
- Around line 36-44: Update parseDuration to validate each millisecond increment
and the accumulated total, not just the parsed amount. In the amount parsing and
total accumulation logic, reject values when amount * UNIT_MS[unit] or the
resulting total exceeds Number.MAX_SAFE_INTEGER, or use a wider numeric
representation consistently; preserve the existing error behavior and clear
overflow messaging.
In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r1/impl.ts`:
- Around line 41-43: In the duration accumulation logic of parseDuration,
validate total immediately after adding amount * UNIT_TO_MS[unit] by checking
Number.isSafeInteger(total); throw a RangeError when the total is not a safe
integer.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/impl.ts`:
- Around line 43-46: Validate each converted segment and the running total in
the duration parsing function before updating totalMs, rejecting values when
amount * unitMs or the accumulated sum is non-finite or exceeds
Number.MAX_SAFE_INTEGER; preserve the existing invalid-input behavior and ensure
no unsafe rounded or infinite duration is returned.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/impl.ts`:
- Around line 32-37: Validate each parsed amount and the accumulated total for
safe-integer bounds in the duration parsing function before returning the
result; reject inputs when Number(amountStr) is not a safe integer or when
amount * unitMs or the updated totalMs is not safe, rather than returning an
imprecise or overflowing duration.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/test.ts`:
- Line 69: The unexpected-error reporter in the parseDuration test should not
cast thrown values to Error, since JavaScript permits null, undefined, and other
non-Error values. Update the catch handling to treat the value as unknown and
use an Error type guard, formatting with err.message for Error instances and
String(err) otherwise.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/impl.ts`:
- Around line 30-32: Reject duration results that are not safe integers: in the
parsing logic using amount, unit, and totalMs, validate each computed unit
contribution and the accumulated total with Number.isSafeInteger before
assigning or returning them, and throw the existing invalid-input error when
either check fails.
In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts`:
- Around line 140-149: Update realClockIntegrationTest to use a monotonic time
source by replacing Date.now() in the realClock implementation with
performance.now(), while preserving the Clock interface and existing timing
checks.
In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r2/impl.ts`:
- Around line 35-40: Update the method comment for the token-removal function to
state that a failed removal does not consume tokens, while acknowledging that
the preceding refill may update the bucket’s token count and timestamp; remove
the claim that the entire bucket remains unchanged.
---
Nitpick comments:
In `@docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/test.ts`:
- Around line 7-28: Replace the custom runner in test.ts with a standard Vitest
test: rename the file to impl.test.ts beside impl.ts, import test utilities from
vitest, and convert check and its pass/fail bookkeeping into Vitest test cases
and assertions. Remove process.exit and ensure csvParseLine behavior is
validated through the repository’s normal test runner.
In `@docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r2/test.ts`:
- Around line 7-16: The test uses a nonstandard filename and custom assertion
runner instead of the repository’s Vitest convention. Rename it to impl.test.ts,
replace arrEq/check and direct process exits with Vitest imports and assertions,
and structure the cases as Vitest tests; alternatively, explicitly document and
configure the benchmark harness to execute and exclude this captured script.
In `@docs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.ts`:
- Around line 48-49: Update the numeric-precision comment near the accumulation
in the duration parser to state that \d+ only guarantees non-negative integer
syntax, while the safe-integer validation establishes the Number precision
boundary; retain or reference the existing guard rather than claiming Number is
always exact.
In `@docs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r2/test.ts`:
- Around line 5-16: Align this test with repository conventions by renaming
test.ts to impl.test.ts and replacing the custom checks/assertEqual reporting
with Vitest imports and test/expect assertions; if the evaluation harness
intentionally runs this script directly, verify and document that exception
instead.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/impl.ts`:
- Around line 7-13: UNIT_MS is a constant lookup that should not be mutable.
Update its type declaration to use a readonly record while preserving the
existing unit mappings and values.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/impl.ts`:
- Around line 3-9: Make the UNIT_MS lookup table immutable by declaring it with
a readonly TypeScript record type, preserving its existing unit mappings and
values.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/impl.ts`:
- Around line 3-9: The UNIT_MS lookup table is mutable but should be immutable
parser configuration. Update the UNIT_MS declaration to use a readonly record
type, preserving its existing keys and values.
In `@docs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/test.ts`:
- Around line 1-5: Rename the test file to impl.test.ts and convert its manual
pass/fail and process.exit logic to Vitest assertions, importing the required
utilities from vitest. Preserve the existing OkCase, ThrowCase, and Case
coverage while using standard test/describe or it blocks; only leave the current
structure if this evaluation script is explicitly excluded from test discovery.
In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.ts`:
- Around line 3-43: Replace the custom runner in FakeClock and its related
checks with a standard Vitest test module: rename the file to impl.test.ts,
import test utilities from vitest, and convert check/expectThrows usage to
Vitest assertions and test cases. If the evaluation harness requires direct
execution, instead verify and document that exception while preserving the
repository’s required foo.test.ts convention.
In `@docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/test.ts`:
- Around line 1-40: Align the TokenBucket test with the repository’s Vitest
convention: rename the file to impl.test.ts, replace the custom
check/expectThrows runner and process.exit usage with Vitest imports and
assertions, and structure the cases with describe/it or test. Preserve the
existing FakeClock and coverage while using Vitest’s expect(...).toThrow
assertions for failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d69280c3-a3ba-48ae-a501-8edd0c2c9cde
⛔ Files ignored due to path filters (5)
docs/evals/2026-07-09-1636-plan-format-corpus.mdis excluded by!**/*.mddocs/evals/2026-07-09-1670-delegation-pipeline-empirical.mdis excluded by!**/*.mddocs/evals/data/2026-07-09/chart-exp3-correctness-vs-process.svgis excluded by!**/*.svgdocs/evals/data/2026-07-09/exp3-underspec-ab.csvis excluded by!**/*.csvdocs/evals/quality-ab/ANALYSIS.mdis excluded by!**/*.md
📒 Files selected for processing (51)
docs/evals/data/2026-07-09/generate_charts.pydocs/evals/quality-ab/run-underspec.test.tsdocs/evals/quality-ab/run-underspec.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__N__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/csv-line__N__r2/test.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/parse-duration__N__r2/test.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__N__r1/test.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/opus/token-bucket__N__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/parse-duration__N__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r1/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r1/test.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r2/impl.tsdocs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r2/test.ts
✅ Files skipped from review due to trivial changes (3)
- docs/evals/quality-ab/runs/underspec/opus/csv-line__N__r2/test.ts
- docs/evals/quality-ab/runs/underspec/opus/csv-line__N__r1/test.ts
- docs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r2/test.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- docs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r2/impl.ts
- docs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r1/test.ts
- docs/evals/quality-ab/runs/underspec/opus/csv-line__N__r2/impl.ts
- docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__N__r1/impl.ts
- docs/evals/quality-ab/runs/underspec/opus/csv-line__N__r1/impl.ts
- docs/evals/quality-ab/runs/underspec/sonnet/csv-line__E__r2/impl.ts
- docs/evals/quality-ab/runs/underspec/sonnet/csv-line__N__r1/impl.ts
- docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r2/impl.ts
- docs/evals/quality-ab/runs/underspec/opus/token-bucket__E__r1/impl.ts
- docs/evals/quality-ab/runs/underspec/opus/csv-line__E__r1/impl.ts
- docs/evals/quality-ab/runs/underspec/opus/token-bucket__N__r1/impl.ts
- docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r2/impl.ts
- docs/evals/quality-ab/run-underspec.test.ts
- docs/evals/quality-ab/runs/underspec/opus/token-bucket__N__r2/impl.ts
- docs/evals/quality-ab/runs/underspec/sonnet/token-bucket__E__r1/impl.ts
- docs/evals/data/2026-07-09/generate_charts.py
- docs/evals/quality-ab/run-underspec.ts
Windows Unit (Root) was red on the new eval harnesses (subprocess grading spawns tsx/git; npm .cmd shims don't spawn on win32). Guarded the subprocess-grading suites with skipIf(win32) — dev-only Linux tooling, covered on the Linux lane. CodeRabbit authored-harness fixes (the generated runs/underspec/** comments are raw experiment data and intentionally untouched): - exp1-binary-driver: validate the MCP envelope with Zod + fail-honest parse (malformed/off-schema payload → blocked result, not an uncaught throw that crashes the whole run); move client.connect() inside try + rm the temp stateDir in finally (no orphaned process/dir on handshake failure or reruns). - native-baseline/harness: add a 300s timeout to the spawnClaude execFile so a hung claude -p degrades to a BLOCKED record (preserves fail-honest). - emit-baseline-csv: sum session costUSD across all models (mixed-model sessions no longer silently under-report; single-model unchanged → CSV byte-identical). Verified: root+MCP tsc 0; exp1-driver 13/1-skip; Linux eval tests 55 pass; emit-baseline-csv --check byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
docs/evals/**/runs/** is committed raw experiment output (model-generated impl/test files, graded as-is by the harness), not authored code — same class as the existing skills/ and batch-baselines/ render exclusions. Editing them would falsify the measured results; excluding them from CodeRabbit stops the generated cells from being reviewed as hand-written code (root cause of 19 of the 29 review comments on this PR). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ntry)
Sentry flagged (HIGH) that extractFiles still hardcoded /^###\s/ as the
section terminator — the same 3-hash-only class DR-5 fixed for parseTaskBlocks,
but this sibling was missed. On the majority-#### corpus a '#### <Sub>' header
after **Files:** was not recognized as a terminator, so a backtick path in a
later sub-section could be swept into the file list. Switched to the existing
TASK_DEPTH_HEADING (/^#{3,4}\s/). Regression test kill-probed (RED on old regex).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+479
to
+488
| corpus: readonly CorpusSpecTasks[], | ||
| serverRoot: string, | ||
| ): Promise<Map<string, SpecRunResult>> { | ||
| const stateDir = fs.mkdtempSync(path.join(path.dirname(serverRoot), `exp1-state-${ref.label}-`)); | ||
| const transport = new StdioClientTransport({ | ||
| command: ref.binaryPath, | ||
| args: ['mcp'], | ||
| cwd: serverRoot, | ||
| env: { ...process.env, WORKFLOW_STATE_DIR: stateDir }, | ||
| }); |
There was a problem hiding this comment.
Bug: The toCsv function doesn't escape commas in field values. Error messages containing commas, when written to the CSV, will break the column structure and corrupt the file.
Severity: MEDIUM
Suggested Fix
Modify the toCsv function to properly handle fields that may contain commas, according to RFC 4180. This typically involves checking each field for commas, line breaks, or double quotes, and enclosing the entire field in double quotes if any are present. Any existing double quotes within the field must also be escaped (usually by doubling them).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: servers/exarchos-mcp/src/evals/benchmarks/exp1-binary-driver.ts#L479-L488
Potential issue: The `toCsv` function joins CSV fields with commas without any escaping.
When a spec dispatch fails, a `detail` field containing an error message is generated
and placed in the `beforeSequence` and `afterSequence` columns. Error messages, such as
those from Zod validation or JSON parsing, can plausibly contain commas (e.g.,
`malformed JSON from binary: ...`). These unescaped commas will be interpreted as column
delimiters by CSV parsers, corrupting the data file and causing downstream tools to fail
or misinterpret the data.
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
1 participant
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.
Summary
Re-runs the #1636 delegation-pipeline benchmark properly — every modeled number replaced with an executed one, and every claimed benefit held to isolate-one-variable rigor. The prior study (PR #1669) modeled the classifier, assumed the native model baseline, and tested only fully-specified tasks; its conclusions were PROVISIONAL. Executing them honestly, only one of the three claimed benefits survives measurement.
Three experiments — one conclusive, one deferred, one clean null:
--model sonnet, which forces a flat per-subagent result, so it cannot measure native routing (Claude Code plan mode / dynamic workflows do dispatch models per subtask). It proved only the measurement harness. Blocked until exarchos's own routing defect — Model routing ignores the risk-tier classifier — collapses to flat opus (less differentiated than native plan mode / dynamic workflows) #1672 (model selection decoupled from the risk classifier, collapses to opus) — is fixed and the harness observes native's real unpinned dispatch.Net: the pipeline's demonstrated value narrows to verification-depth calibration (Exp 1). Filed #1672 for the model-routing defect Exp 2 surfaced.
Closes #1670.
Changes
Production (one change, DR-5):
servers/exarchos-mcp/src/orchestrate/task-decomposition.ts— portparseTaskBlocks/extractTaskRiskTierto parse the majority-4-hash corpus (####) as well as legacy 3-hash; regression-tested + no regression across the 3 consumers. Plan/task templates aligned.Experiment harnesses + shared helper:
servers/exarchos-mcp/src/evals/provenance.ts— shared provenance + fail-honest helper (DR-7).servers/exarchos-mcp/src/evals/benchmarks/exp1-binary-driver.ts— Exp 1: drivesprepare_delegationthrough each spawned MCP binary; causal + released pairs.docs/evals/native-baseline/{harness.ts,emit-baseline-csv.ts}— Exp 2:claude -ptranscript parser + a committed, tested, provenance-stamped CSV generator (with--check).docs/evals/quality-ab/{run-underspec.ts,grade.ts,tasks/*/SPEC.underspec.md}— Exp 3: under-specified variants + symmetric-prompt A/B (both arms implement+test) + mechanical diff-scoped mutation grading.Published results (DR-6):
docs/evals/2026-07-09-1670-delegation-pipeline-empirical.md— dated benchmark: provenance table, 3 embedded SVG figures with alt-text, executed conclusions (one conclusive, one deferred, one null).docs/evals/data/2026-07-09/— 3 raw CSVs +binaries.provenance.json+generate_charts.py(pure-stdlib, deterministic, no network) + 3 committed SVGs.docs/evals/native-baseline/MECHANICS.md— Exp 2 spike write-up (with the pinned-spike scope limit).docs/evals/2026-07-09-1636-plan-format-corpus.md+docs/evals/quality-ab/ANALYSIS.md— provisional caveats superseded for now-executed claims; the process-delta claim refuted (prompt artifact), correctness-null confirmed, native-baseline left open (deferred).Test Plan
tsc --noEmit✅ · MCPtsc --noEmit✅claude -p, 0 blocked; regenerated CSV + run dirs + chartpython3 generate_charts.pyruns clean, no network, byte-deterministic; 3 SVGs well-formed XMLemit-baseline-csv.ts --check— native CSV byte-identical to the fixtures🤖 Generated with Claude Code