Skip to content

refactor(evals): delete the dead reporter/telemetry pipeline and publication leftovers - #1174

Merged
aviggiano merged 9 commits into
mainfrom
claude/w20a-evals-dead-code
Sep 29, 2026
Merged

aviggiano merged 9 commits into
mainfrom
claude/w20a-evals-dead-code

Conversation

@aviggiano

@aviggiano aviggiano commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

@ultrafuzz/evals still carried three pipelines that nothing uses. One of them could still end an eval run early.

  • Telemetry pump and reporters. runEvalSuite always built an empty reporter list (const reporters: EvalReporter[] = []), and resolveEvalProvider accepts only none. So NodeTelemetryPump translated and "delivered" journal events to nobody. It still had to succeed on every poll, though, because a failed drain() rejected runEvalSuite before run-summary.json was written. One schema-valid trigger is a node-synced transition without payload.attempt. The event contract makes that field optional (nodeSyncedPayloadSchema), and sync omits it when the runner reported no attempt (workflow-sync.ts node-synced payload). The pump rejected such a transition with EVAL_TELEMETRY_EVENT_UNUSABLE.
  • Automatic eval-history publication. history-publication.ts, its plan and generation schemas, their semantic gates, and the automatic / plan-rows / policy commands of scripts/ci/prepare-eval-history-publication.mjs remained after Remove paid benchmark CI and repair CI gates #1131 deleted the workflows that called them.
  • History extras. eval history --max-age-days (with assertEvalHistoryRecency) remained after ci: remove benchmark history freshness monitor #1112 removed the freshness monitor that used it. Six per-metric history SVGs were rendered and byte-checked even though README.md embeds only the three overview charts.

Root cause

When the external reporter providers and the paid benchmark workflows were removed, the extension points and handoff code they needed were left in place. The pump in particular kept running its fail-closed journal, cursor and lock checks on every watch tick, with no consumer at all.

Change

  1. Telemetry pump and reporter interface deleted. watchEvalRow now repeats three steps: sync, read state.json, sleep. After the loop it reads state.json once more, as main did, so a run that another command (ultrafuzz status, inspect, the dashboard) synchronized to a terminal state during the last sleep is recorded as terminal, not timed out. Deleted with the pump:

    • NodeTelemetryPump, EvalReporter and its row/graph/event types, guardReporter and reporterForReliableDelivery.
    • The telemetry cursor: reader, writer, schema and common-schema definition. Eval runs no longer write telemetry/.
    • The pump's own graph.json read (for reporters' onRowStart), the proper-lockfile dependency, and the utils helpers only the pump used.

    boundedResponseText moves into scoring.ts next to the LLM judge, its only caller. It now takes the real Response type; one scoring test double changed from a plain object to new Response(...).

  2. Automatic history publication deleted. history-publication.ts, its two schemas and gates, and its test are gone. prepare-eval-history-publication.mjs keeps only the exports that validate-modal-benchmark-launch.mjs and prepare-modal-benchmark-cleanup.mjs import, plus the helpers those reach. Its CLI entry, the bundle-summary and plan-row code, and the TypeScript source-constant parser are gone.

  3. eval history --max-age-days and assertEvalHistoryRecency deleted, with their tests.

  4. The six per-metric charts and their renderer deleted. The three README charts render byte-identically. History tests that used a per-metric chart to observe supersession now read the same commit links from quality.svg.

  5. Unused exports deleted:

    • read/write/parseEvalPublicationState, plus the publication-state schema and types (leftovers of the retired eval publish). The CLI json validate eval-schema test now uses the ground-truth schema.
    • assertEvalReportAuthorityRemainedCurrent, isEvalNodeStatus, EVAL_EXPANSION_SOURCE_NODE_KEY and evmbenchSchemaPath.
    • validateBenchmarkControlManifest and the testReportingPolicy test helper are no longer exported; they are used only inside their own modules.

Docs, the default suite comment and the eval run help text no longer promise telemetry. The reporting.artifacts docs, suite comment and type comments now say the block is validated and recorded but that no eval behaviour depends on it; the EvalTarget.sensitivity comment names what private does enforce (ground truth bound to the target's repo and ref, and ULTRAFUZZ_EVAL_JUDGE_ALLOW_PRIVATE_DATA=true before an LLM judge receives it). Net: +249 / −5,717 lines outside the SVGs, plus 4,643 deleted SVG lines.

The execution-policy fingerprint inputs are unchanged. reporting.node_telemetry and reporting.heartbeat_interval_seconds still feed it, and EVAL_EXECUTION_POLICY_REVISION is untouched, so published history cohorts stay comparable.

Deliberately not built (and why)

  • No early stop after N identical consecutive sync failures. The sync-state analysis suggested this, and I decided against it. watchEvalRow already records the failure count, first and last failure times, and the last message on the row as EVAL_ROW_SYNC_FAILED, and --watch-timeout-seconds bounds the watch. An early stop would end the watch while the detached run may still be progressing. In the Modal public worker, eval run returning leads straight to scoring and bundling (or to a failure), and the run root is removed with the sandbox (see the public-worker.ts comment on retained artifacts). So an early stop on a repeated but transient error would end a healthy campaign, which works against the stability priority. It would also make the decision depend on comparing error-message text. The permanent-failure cases come from sync failing on bookkeeping, and the fix for that belongs in sync itself.
  • An invalid graph.json still fails the watch; this PR does not change that. After polling, evalRunExpansion (readStaticNodeIds → assertPlannedGraph) validates the graph for every row that has a state.json, and classifyRecoveryEquivalence does so for terminal rows. Either throw still rejects runEvalSuite before run-summary.json is written. On main the pump's graph read rejected it earlier, during polling; now it happens after polling. Making those readers tolerant is a separate stability change. The deleted watch-level test rejects a present graph that would require telemetry repair is not restored: its subject (telemetry repair) is gone, and the readers' rejection is covered by expansion.test.ts › rejects absent or malformed state and graph evidence and recovery-equivalence.test.ts › rejects contradictory graph kind and model fanout.
  • The suite reporting: block is kept. The suite schema is closed, so removing fields would reject existing suite YAML. node_telemetry still selects the watch default, and two of the fields feed the execution-policy fingerprint. experiment_prefix and artifacts are now documented as validated, with no eval behaviour depending on them.
  • The --provider / resolveEvalProvider shim is kept. Whether to remove it is a separate, contested decision.
  • prepare-eval-history-publication.mjs is not renamed. This keeps the diff to deletions. Its remaining exports validate Modal benchmark control manifests.
  • buildPairwiseChart stays exported, although it was listed as unused. It is called inside charts.ts, and dropping export pulls its existing 142-line body into the diff-limited strict lint.

Verification

Discriminating test 1: runner-publish.test.ts › publishes the run summary for a watched row whose skipped node synced without an attempt. It uses a terminal run whose journal has a contract-valid node-synced skipped event with no payload.attempt.

  • With packages/evals/src and packages/evals/schema reset to origin/main and the new test in place, runEvalSuite rejects: EvalError: node transition evt-4444… must carry a positive canonical payload.attempt, raised from NodeTelemetryPump.drain inside watchEvalRow.
  • On this branch it resolves, with launched: 1, incomplete: 0, and run-summary.json is written.

Regression test 2 (added after review): runner-publish.test.ts › records the terminal status another writer reached while the watch slept past its deadline. One poll runs against a running state with a 1 s deadline and a 1.5 s poll interval, and a timer writes the terminal run 300 ms after that poll, during the sleep.

  • On origin/main (b6dd1da), with the test ported (main's watchEvalRow also needs reporters: []): passes.
  • On the previous branch head (846dcf3, the restored line stashed): fails with final_status: "timed-out" and workflow: { status: "running", terminal: false }.
  • With the restored re-read: passes.
  • Timing: the test compiles the state validator first so the poll starts well inside the deadline. If the first poll started after the 1 s deadline, syncCalls would be 0 and the test would fail loudly. If the read after the poll took more than 300 ms, the test would still pass but would stop discriminating.

Also checked by a throwaway probe (not committed): on this branch an invalid graph.json (a node without logical_id) rejects watchEvalRow with planned graph is schema-invalid, both for a terminal row and for a running row whose watch times out.

What I ran, after pnpm -w build:

  • Full evals vitest suite: 20 files, 384 tests pass, on the final head. I used --testTimeout=180000 --maxWorkers=4 because the host is shared; an earlier round under load 30–40 with the default 5 s timeout hit 9 timeouts and no assertion failures. The new test sets its own 20 s timeout because it sleeps 1.5 s.
  • CLI (node:test), in the first round (the review fixes since then change one line of watchEvalRow, add a test, and edit comments and docs):
    • eval-history.test.ts: pass.
    • json-validate.test.ts › json validate recognizes the pinned eval schema…: pass.
    • cli.test.ts › agent-owned bytes stay identical across validation, sync, aggregation, report, dashboard, and bundle reads: pass, 139 s.
  • evmbench: contracts.test.ts passes (32), first round.
  • pnpm -w test:ci-scripts, first round: 94 pass, 1 fail. The failure is safe-archive-bun.test.ts › repeatedly extracts a synthetic archive without stalling Bun callbacks: 2,000 extractions inside its own 15 s budget. It imports only packages/modal/src/safe-archive.ts, which this PR does not touch. The same test also timed out when I ran it in a pristine origin/main worktree on this host.
  • pnpm -w benchmark:check:prebuilt (eval history --check), first round: passes against the checked-in quality.svg, latest-summary.svg and performance-cost.svg (192 observations). The kept charts are therefore byte-identical.
  • Static checks on the final head, all passing:
    • tsc --noEmit for evals (cli, evmbench and modal in the first round).
    • npx prettier --check and npx eslint on every changed file.
    • CI=1 ESLINT_PLUGIN_DIFF_COMMIT=origin/main pnpm -w lint:strict:ci.
    • pnpm -w knip.
    • node scripts/docs-check.mjs.
    • pnpm install --frozen-lockfile --offline with the updated lockfile (first round).
  • Not run: the full CLI, runtime, modal and dashboard suites. Only comments changed in modal.

Risk / compatibility

  • Breaking CLI surface (recorded under Breaking changes in the changelog):
    • ultrafuzz eval history --max-age-days is gone, and oclif rejects the flag.
    • eval history renders 3 SVGs instead of 9. Render mode does not delete old SVGs in other checkouts; append mode already replaces the whole charts directory.
  • Removed eval schema IDs: telemetry-cursor:1, publication-state:1, history-automatic-publication-plan:1 and history-publication-generation:1. json validate no longer recognizes them, and the eval schema bundle digest changes. Artifact schemas and validatorBuildIdentity() are untouched: that identity hashes only @ultrafuzz/artifacts validator modules and ajv versions. So in-flight campaign runs are unaffected; I checked this by reading the code, not by running an old run.
  • Watch behaviour:
    • The pump's journal and graph checks no longer run on each poll, so a journal record the pump rejected no longer aborts the suite. An invalid graph.json still rejects the watch, now after polling instead of during it (see Deliberately not built).
    • The final state.json re-read after the loop is kept, so the terminal-versus-timed-out classification matches main.
    • A foreign state.json swapped in mid-watch now fails with readStateSafe's message ("run state identity does not match run …") instead of the pump's.
    • Existing <eval-run>/telemetry/*.cursor.json files are ignored.
  • CI script: prepare-eval-history-publication.mjs no longer has a command-line entry. No workflow, script or doc invoked it.
  • Merge order: ci: validate every package on PRs, stop cancelling main runs, and add a global complexity ceiling #1184 (w21) edits the same two lines of packages/cli/test/cli.test.ts (~2582) as this PR's test retitle. Whichever merges second keeps w21's (t) / tempProject(t) and this title.

Refs #462

🤖 Generated with Claude Code

RetriggerConfidence Score: 4/5

The PR does not appear safe to merge while the outstanding watch-state race can prevent a terminal row from being recorded.

Fix All in Claude CodeFindings

  1. P1 Terminal state can be lost ▶
  2. P2 Dashboard is not a syncer ▶
Fix with agent prompt
### Issue 1
packages/evals/src/runner.ts:undefined-622
When a poll has already observed a terminal run, this unconditional second read can overlap with another synchronizer replacing `state.json`. The snapshot reader then throws, so the watched row-and potentially the suite-fails instead of recording the terminal result. Keep the state already observed when it is terminal.

### Issue 2
packages/evals/src/runner.ts:undefined-620
This comment names the dashboard as a writer of `state.json`, but the dashboard only reads run state. The new test repeats that example. It gives maintainers the wrong reason for the final read and makes the behavior harder to understand; use an actual state writer as the example.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR removes the unused eval telemetry/reporter pipeline, automatic history-publication code, retired schemas, the history recency flag, and six per-metric charts. It also restores a final state read after watch polling. The latest change removes the changelog entry for these compatibility changes.

Reviews (3) · Last reviewed commit: "chore: move the changelog entry to the c..."

aviggiano and others added 6 commits September 28, 2026 23:46
… interface

runEvalSuite always built an empty reporter list and resolveEvalProvider
accepts only "none", so NodeTelemetryPump translated and "delivered"
journal events to nobody. It still had to succeed on every poll: a drain
that threw aborted the whole suite before run-summary.json was written.
One schema-valid trigger is a node-synced transition without
payload.attempt, which the event contract allows and sync emits when the
runner reported no attempt; the pump rejected it with
EVAL_TELEMETRY_EVENT_UNUSABLE.

watchEvalRow is now: sync, read state.json, sleep. Deleted with the pump:
EvalReporter and its graph/row types, guardReporter,
reporterForReliableDelivery, the telemetry cursor (reader/writer, schema
and common-schema definition), the eval telemetry/ directory, the
graph.json read the reporter needed, the proper-lockfile dependency, and
utils helpers only the pump used. boundedResponseText moves into
scoring.ts next to the LLM judge, its only caller.

The execution-policy fingerprint inputs are unchanged, so published
history stays comparable.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
#1131 deleted the eval-benchmarks and eval-history-publication workflows,
and docs/how-to/run-evals.md already says there is no automatic history
publisher. Their producer and handoff checks stayed behind:
history-publication.ts, its plan and generation schemas and semantic
gates, and the `automatic` / `plan-rows` / `policy` commands of
scripts/ci/prepare-eval-history-publication.mjs. Nothing runs them.

The script keeps only the exports that validate-modal-benchmark-launch.mjs
and prepare-modal-benchmark-cleanup.mjs import (manifest, pair-config and
policy-checkout validation) and the helpers they reach; its CLI entry, the
bundle-summary and plan-row code, and the TypeScript source-constant
parser go. Tests of the deleted paths go with them; the strict-manifest
reader test now targets readBenchmarkControlManifest, which still uses
that reader.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
The flag and assertEvalHistoryRecency existed for the scheduled
benchmark-history freshness monitor, which #1112 removed. No workflow,
script or doc passes the flag any more.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
README.md embeds only the three overview charts (quality,
latest-summary, performance-cost). The precision, recall, F1,
cumulative-true-positive, wall-clock and cost SVGs were still generated
and byte-checked by `eval history --check`, but nothing links to them.
Their renderer and assets go. The three kept charts render
byte-identically: `eval history --check` passes against the checked-in
SVGs.

Tests that used a per-metric chart to observe supersession or
completeness now read the same commit links and completeness attributes
from the overview charts; the tests of per-metric layout details go.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
- read/write/parseEvalPublicationState, the publication-state schema and
  its common-schema definitions, and the EvalPublicationState types: left
  over from the retired `eval publish`; only tests used them. The CLI
  `json validate` eval-schema test now uses the ground-truth schema.
- assertEvalReportAuthorityRemainedCurrent, isEvalNodeStatus,
  EVAL_EXPANSION_SOURCE_NODE_KEY and evmbenchSchemaPath: no caller in
  any package or script.
- validateBenchmarkControlManifest and the testReportingPolicy test
  helper are only used inside their own modules, so they are no longer
  exported.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@aviggiano
aviggiano requested a review from a team as a code owner September 28, 2026 23:54
Comment thread packages/evals/src/runner.ts
aviggiano and others added 2 commits September 29, 2026 02:51
… a row

Deleting the telemetry pump also deleted the state.json re-read that
followed the final drain. When the watch deadline passed during the last
poll sleep, the row was then classified from the state read before that
sleep. The watch is not the only writer of state.json: `ultrafuzz status`,
`inspect` and the dashboard synchronize the run too. A run one of them
made terminal during that sleep was recorded as timed-out, with an
EVAL_ROW_WATCH_TIMEOUT diagnostic and a `running` workflow lifecycle,
and counted as incomplete. origin/main recorded its real terminal status.

Restore the single read, as origin/main had it.

The new watchEvalRow test polls once with a 1 s deadline and a 1.5 s
sleep, and a timer writes the terminal run 300 ms after that poll. It
passes on origin/main (test ported with `reporters: []`), fails on the
previous branch head with final_status "timed-out", and passes with this
change.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
With the pump gone, eval runs write no telemetry, and the suite's
`reporting.artifacts` policy has no reporter to stream to. Two docs
still said eval runs retain telemetry, and the EvalArtifactPolicy and
EvalTarget.sensitivity comments still described payload streaming and
manifest-only artifact reporting.

The docs and the default suite comment also said "nothing reads" the
artifacts policy. That overstated it: normalizeReporting still computes
it, and `eval plan --json` and the Modal worker still carry it. They now
say no eval behaviour depends on it. The `sensitivity` comment now names
what `private` does enforce: ground truth bound to the target's repo and
ref, and ULTRAFUZZ_EVAL_JUDGE_ALLOW_PRIVATE_DATA before an LLM judge
receives the data.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
diagnostics.push(...finalDrain.warnings);
// Other syncers (`ultrafuzz status`, the dashboard) also write state.json; a
// run they finished during the final sleep is terminal, not timed out.
state = readStateSafe(runRoot, input.record.ultrafuzz_run_id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Terminal state can be lost

When a poll has already observed a terminal run, this unconditional second read can overlap with another synchronizer replacing state.json. The snapshot reader then throws, so the watched row—and potentially the suite—fails instead of recording the terminal result. Keep the state already observed when it is terminal.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/evals/src/runner.ts
Line: 622

Comment:
**Terminal state can be lost**

When a poll has already observed a terminal run, this unconditional second read can overlap with another synchronizer replacing `state.json`. The snapshot reader then throws, so the watched row—and potentially the suite—fails instead of recording the terminal result. Keep the state already observed when it is terminal.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code

await startRowIfReady(true);
const finalDrain = await pump.drain();
diagnostics.push(...finalDrain.warnings);
// Other syncers (`ultrafuzz status`, the dashboard) also write state.json; a

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dashboard is not a syncer

This comment names the dashboard as a writer of state.json, but the dashboard only reads run state. The new test repeats that example. It gives maintainers the wrong reason for the final read and makes the behavior harder to understand; use an actual state writer as the example.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/evals/src/runner.ts
Line: 620

Comment:
**Dashboard is not a syncer**

This comment names the dashboard as a writer of `state.json`, but the dashboard only reads run state. The new test repeats that example. It gives maintainers the wrong reason for the final read and makes the behavior harder to understand; use an actual state writer as the example.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

Every pull request in this batch inserts its entry at the same place in
CHANGELOG.md, so each merge would conflict with the next. The entries are
collected into one changelog update instead.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant