bench: pre-run correctness fixes — promote sink read-back, audit-signal integrity, grader/merge/env guards (Closes #977)#978
Merged
Conversation
…al integrity, grader/merge/env guards (#977) The reviewer-promotion path now reads the promoted content back from its sink (entity description via datahub_get_entity, page summary via the portal list) after the API verify; an API-confirmed apply whose sink does not show the change is a harness error, never a reported promotion. Cold-start claude-cli episodes with successful tool calls but no dps_ handle fail loudly (matching the S1-S3 pipeline), and a failed audit read-back is recorded per attempt as audit_read_error and totaled as a coverage-integrity warning instead of silently zeroing enrichment coverage. The entity grader matches aliases on word boundaries, so a wrong-alias veto like "East" no longer fires inside "at least" and "West" no longer matches inside "southwest". Supersede metrics gate the duplicate check on the update episode having executed its correction capture: a missed correction is measured on the new update-capture rate (and fails pass^k) instead of inflating the duplicate rate with capture noise. -merge refuses to fold passes from different LLM providers into one scorecard; the claude-cli child env strips ANTHROPIC_AUTH_TOKEN and the Bedrock/Vertex switches alongside ANTHROPIC_API_KEY; MCPCalls is derived from the deduplicated tool_use id set; a parsed episode with tool calls but zero token usage warns loudly. bench-lifecycle and bench-supersede write into per-run timestamped directories and benchrun refuses an existing -out in every producing mode, so no run's results can ever be overwritten; their report targets take RESULTS=. The README's cache-token verification claim is corrected to pending until the first real claude-cli run on the current parser.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #978 +/- ##
=======================================
Coverage 89.05% 89.06%
=======================================
Files 424 424
Lines 48598 48598
=======================================
+ Hits 43280 43283 +3
+ Misses 3545 3543 -2
+ Partials 1773 1772 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…cognitive-complexity gate (#977) The cold-start Aggregate folds lesson and attempt accumulation through foldLesson/foldAttempt helpers, bringing it back under the gocognit 15 ceiling CI's harness lint enforces. make verify now runs bench-test and the new bench-lint (a full-module golangci-lint of bench/, mirroring CI's Harness module checks job). bench/ is a separate Go module the root lint and test targets never reach, and the root lint's --new-from-patch scoping cannot see a finding anchored on an unchanged line, so a bench-only finding previously surfaced first in CI. The bench README distinguishes these pure module checks (in verify) from the stack-dependent bench run targets (kept out of verify).
…ure arbiter, audit-signal parity, run-dir atomicity (#977) BaselineCompatible refuses a baseline that scored duplicates but carries no update-capture data: such a baseline predates the update-capture gating of the duplicate rate, its duplicate numbers are definitionally inflated, and gating against it could absorb a real supersede regression. The supersede stage verifies the correction capture BEFORE spending the post-update recall episode: a miss skips the recall (no staleness dilution of update correctness, no wasted paid episode) and still fails pass^k. When the transcript is inconclusive — a claude-cli stream can drop the paired tool_result of a call that really executed — the insights API is the arbiter, with the teach-stage insight excluded by ID because the skew-widened since window can reach back past it. The test fakes now honor the since parameter exactly as the admin API does and stamp insights with real creation times. The lifecycle claude-cli path gains the same audit-signal integrity the cold-start path has: successful tool calls with no dps_ handle are a harness error, and a failed audit read-back is recorded per episode as audit_read_error and totaled as a summary warning. The reviewer's sink read-back window is configurable end to end (-sink-timeout through both runners), and its failure message states that the promotion may be live despite the error so the run is treated as contaminated rather than retried in place. Per-run output directories carry a PID suffix and are created with a plain mkdir, so two runs launched in the same second abort loudly instead of silently sharing a directory. The overwrite-guard comment scopes its claim to the timestamped-run modes and documents the S1-S3 per-arm slot contract that bench-compare consumes. docs/reference/benchmarks.md documents the RESULTS= report invocation and the per-run directories.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #977
Hardens the bench harness ahead of the first real cold-start knowledge-growth run (#958, #963): promotion is verified all the way to the sink, the coverage and audit signals cannot silently degrade, metric redefinitions refuse incomparable baselines, and the grader, merge, environment, and output-isolation guards close the remaining paths by which a paid run could produce wrong, unattributable, or destroyable numbers. The bench module's checks also now run locally in
make verify, mirroring CI's harness job.Promotion is verified at the sink, not only in the platform's own records
promote.Reviewer.Apply(bench/internal/promote/promote.go) follows the insights/changesets API verify with a sink read-back: for a datahub-sink promotion it reads the entity throughdatahub_get_entityand requires the effective description to carry the applied fact; for a page-sink promotion it lists the portal knowledge pages and requires a page with the applied slug and summary. An API-confirmed apply whose sink does not show the change is a harness error, never a reported promotion, so a silent sink-write loss (the resulting_state defect class fixed in v1.94.1) aborts the run before episodes are spent instead of publishing a flat learning curve misattributed to agent behavior. The read-back window is configurable end to end (-sink-timeout, default 15s, plumbed through the lifecycle, supersede, and cold-start runners), and its failure message states that the promotion may be live despite the error — approve and apply already succeeded — so the run is treated as contaminated rather than retried in place. Covered byTestReviewerApplySinkReadBack(both sinks, both the landed and lost-write cases) and exercised against the live platform by the scripted smokes.Audit-signal integrity on both claude-cli paths
A claude-cli episode that reports successful MCP calls but surfaces no
dps_handle is a harness error in both the cold-start and S5 lifecycle runners (matching the S1-S3 pipeline's contract): with no handle a successful data call is impossible, so the inconsistency — a stream-shape drift or an unparseable platform_info result — surfaces instead of contributing unflagged zeros. A failed audit read-back on a graded attempt is recorded per attempt asaudit_read_errorin results.json, totaled asaudit_read_failures, and printed as an integrity warning in the human summary of both suites — enrichment coverage climbing is a documented cold-start pass criterion, so audit-signal loss is visible in the kept artifact rather than only in a log line. Covered by no-handle and audit-loss tests in both packages.The duplicate rate measures the supersede gate, not capture noise — and old baselines are refused
The S5 supersede stage verifies the correction capture before spending the post-update recall episode (
bench/internal/lifecycle/runner.go): a missed correction skips the recall (no staleness dilution of update correctness, no wasted paid episode), is measured on the newupdate_capture_rate, fails pass^k, and leaves the duplicate outcome unscored — with no correction on the platform the supersede gate never ran, so counting the attempt as duplicated would inflate the duplicate rate by up to the capture-miss rate, more than the regression gate's 5-point tolerance. When the transcript is inconclusive (a claude-cli stream can drop the paired tool_result of a call that really executed), the insights API is the arbiter, with the teach-stage insight excluded by ID because the skew-widenedsincewindow can reach back past it.BaselineCompatiblerefuses a baseline that scored duplicates but carries no update-capture data: such a baseline predates this gating, its duplicate numbers are definitionally inflated, and gating against it could absorb a real supersede regression. The isolated supersede sub-benchmark reports a per-protocolupdate_capture_missedcount. Covered byTestRunSupersedeUpdateCaptureMiss,TestCorrectionCapturedAPIArbiter, and the extendedTestBaselineCompatible; the test fakes now honor thesinceparameter exactly as the admin API does.Entity grading matches aliases on word boundaries
grade.Entity(bench/internal/grade/grade.go) matches aliases and wrong-alias vetoes on word boundaries (letters, digits, and underscores are word characters; dots remain separators, sobench.ordersstill matches insidememory.bench.orders). A correct answer phrased "West, at least by net revenue" is no longer vetoed because "at least" contains "east", and "West" no longer matches inside "southwest". Covered byTestEntityWordBoundary.Merge, subscription, and parser guards
-mergerefuses to fold passes from different LLM providers into one scorecard (sameConfigcompares the provider, not the exact CLI version, mirroringBaselineCompatible), so an anthropic pass and a claude-cli pass of the same model can no longer blend into a k=2 artifact the regression gate itself declares incomparable. The claude-cli child environment stripsANTHROPIC_AUTH_TOKEN,CLAUDE_CODE_USE_BEDROCK, andCLAUDE_CODE_USE_VERTEXalongsideANTHROPIC_API_KEY, so no exported metered credential can silently bill a subscription run.MCPCallsis derived from the deduplicated tool_use id set, keeping it consistent with the success/error tallies and preserving the audit read-back's overcount detection under replayed stream events. An episode that drove tools but reports zero token usage warns loudly, so a stream-shape change in a claude release cannot silently zero a run's self-reported cost basis.Every run's output is kept
bench-lifecycleandbench-supersedewrite into per-run timestamped directories underbuild/bench-results/(matchingbench-cold-start); all three run-dir names carry a PID suffix and are created with a plainmkdir, so two runs launched in the same second abort loudly instead of silently sharing a directory.benchrunrefuses an existing-outin all three timestamped-run modes, and the report targets takeRESULTS=with a listing of available run dirs when unset. The S1-S3 task mode deliberately keeps its fixed per-arm slot (results-<arm>.json) — the contractbench-compareand the CI regression gate consume — and the guard's documentation says so explicitly.Bench module checks run locally
make verifynow runsbench-testand the newbench-lint(a full-module golangci-lint ofbench/, mirroring CI's Harness module checks job).bench/is a separate Go module the root lint and test targets never reach, and the root lint's patch scoping cannot see a finding anchored on an unchanged line, so a bench-only finding previously surfaced first in CI. The cold-startAggregatefolds lesson and attempt accumulation through helpers, keeping it under the gocognit ceiling that lint enforces.Documentation
bench/README.mddocuments the sink read-back, the update-capture gating of the duplicate rate, the new pass-criteria warnings, theRESULTS=report usage, and which bench targets belong inverify(pure module checks) versus not (stack-dependent runs).docs/reference/benchmarks.mddocuments the per-run directories andRESULTS=invocation. The claude-cli cache-token section states the real-run confirmation is pending: every kept claude-cli results file predates the current parser, so the first real run on it supplies that verification, and the zero-usage warning makes a silent field move loud in the meantime.