Skip to content

fix(benchmarks): make an unpublishable smoke generation loud instead of green - #1013

Merged
aviggiano merged 3 commits into
mainfrom
fix/1012-benchmark-history-publication-relay
Aug 31, 2026
Merged

aviggiano merged 3 commits into
mainfrom
fix/1012-benchmark-history-publication-relay

Conversation

@aviggiano

@aviggiano aviggiano commented Aug 31, 2026 •

Copy link
Copy Markdown
Collaborator

Closes the observability half of #1012.

Benchmark history published nothing for 19 days while the producer workflow reported success on nearly every push to main. The publication honesty gate was refusing correctly the whole time — cleanup_incomplete_run ran in 29 of 29 green push runs since 08-13, i.e. a 100% failure rate reported as 100% success. Nothing in the repo measured whether history was advancing.

The dependency that caused the final phase is already resolved out-of-band, at the same commit, so no cohort re-pin and no cohort_fingerprint change. This PR is about making the next occurrence impossible to miss.

What is here

1. Pre-compute cohort reachability gate — scripts/ci/verify-cohort-reachability.mjs (new), wired into eval-benchmarks.yml before any Modal image build or sandbox launch.

Proves every pinned target and every .gitmodules entry at that pinned revision is reachable anonymously, which is exactly what the sandbox can do. Deliberately given no token: an authenticated probe reads a private dependency successfully and reports the cohort healthy, defeating the gate. Exit 1 is a named anonymous denial and blocks; exit 2 is the gate's own "proved nothing" verdict (resolver blip, 403 throttle) and warns without blocking, so a DNS hiccup cannot be published as a private dependency.

Against the broken cohort it produced, in 2 s:

UNREACHABLE very-liquid-vaults-foundry lib/console3 https://github.com/aviggiano/console3
  cannot be cloned anonymously: fatal: could not read Username for 'https://github.com'

It now exits 0 on both lanes (smoke: 16 probes; full: 40 targets / 137 probes, 7 s). Uses raw.githubusercontent.com + codeload rather than the REST API, because the anonymous API ceiling is 60 req/hr/IP and the full lane needs ~137 lookups — and raising that ceiling requires the token that would defeat the check.

2. Scheduled freshness monitor — scripts/ci/assert-benchmark-history-freshness.mjs + .github/workflows/benchmark-history-freshness.yml (new). The repo previously contained zero cron: triggers. Imports the canonical parser from packages/evals/dist rather than re-deriving the rule or hand-copying schema constants. Opens/updates a single tracking issue instead of filing a duplicate daily.

3. eval history --max-age-days — opt-in recency assertion. --check previously validated integrity and chart byte-equality but had no recency check at all, exiting 0 on 19.5-day-old data. Default behaviour is byte-identical, so the existing benchmark:history:check gate on main is untouched. A future-dated observation fails rather than silently passing, so one bad producer clock cannot blind the monitor permanently.

4. Both silent no-ops now annotate — ::warning:: plus $GITHUB_STEP_SUMMARY from the classifier and from qualify. No ready/eligible verdict changes, and tests now pin that so a future change cannot quietly weaken the gate. Loud, not red: a legitimate operational soft-fail still must not turn main red — escalation is the freshness monitor's job.

5. Worker diagnostics — the worker built git exited 1: fatal: Authentication failed for '…' and dropped it at the serialization boundary, so result.json carried only diagnostic_code. It now reaches the collected artifact through the existing eval-failure-diagnostics channel; the log grammar is deliberately not extended, because assertSanitizedModalCollectedFiles asserts before staging and one illegal line discards the pair's entire artifact set. Two latent instances of exactly that were closed: a sanitizer that could emit 1002 bytes against the collector's 1000-byte cap, and reportCorroborationFailure writing a line matching neither production.

Verification

Rebased onto ab3c03f4 (#1005, "adopt external static-analysis gates"), which landed while this was being written and touches public-worker.ts — its change there was dead-export removal, so no semantic conflict. All gates below were run on the rebased tree, including the three gates #1005 introduced that this branch had never been checked against.

Gate Result
bun test scripts/ci/*.test.ts 161 pass / 0 fail (18 files; was 138 / 17)
packages/modal vitest 637 pass / 34 files
packages/evals vitest 431 pass / 22 files
pnpm -w lint clean
pnpm -w lint:strict (--max-warnings 0) clean
pnpm -w knip (--treat-config-hints-as-errors) clean
pnpm -w format:check clean
all 5 workflow YAMLs parse; no runner.* in any job-level env:

End-to-end on the real history: eval history --check still exits 0; --max-age-days 7 exits 1 naming the 19.59-day age. The sandbox's exact anonymous recursive clone now reproduces green (SUBMODULE-UPDATE-RC=0, lib/console3 → 401bd220…).

New tests were mutation-checked rather than trusted — reverting the truncation fix fails the new bound tests, and a degenerate "drop everything" implementation also fails, so content survival is pinned too.

Second commit is unrelated, and main is currently red without it

main fails ci-config.test.ts > runs the release validation lanes the policy script selects, on pull requests too. I verified this on pristine origin/main with none of this branch's files present: #1005 wired external-static-analysis into release-validation.needs and release-gates.needs but left the assertions expecting the previous two- and three-element arrays. Both live in one test, so the first failure masked the second. ci.yml is the intentional side; the assertions were stale. Fixed in a separate commit so it can be cherry-picked or split out — without it this PR's CI is unreadable.

Draft, because

  • Phase C is unmasked, not fixed. WORKFLOW_SUBMISSION_FAILED: workflow run link event does not match its journal entry has not been exercised since 08-27 because the lane died earlier in preparation. Expect it on the first run reaching the eval stage.
  • The reachability gate is deliberately blocking, so the next push turns this workflow red if a dependency is unreachable. That is the intent — 4 seconds red beats 4 hours green-and-empty — but it is a judgment call worth ratifying before merge.
  • The freshness monitor fires immediately on merge, since the real data is stale until a complete generation publishes. Also correct, also worth knowing.
  • Merge-velocity cancellation is untouched (43 of 62 push runs cancelled; 28 of those leaked launched sandboxes because their cleanup job was cancelled too). Needs a trigger/concurrency decision, tracked in Benchmark history stalled 2026-08-12 → 08-31: a four-cause relay behind 100% green checks #1012.
  • Acceptance is not complete until one run lands all three smoke pairs with a validated public-results.json.

🤖 Generated with Claude Code

Greptile Summary

This PR makes silent benchmark-publication failures observable while preserving the existing publication eligibility policy.

  • Adds anonymous cohort reachability checks before launching benchmark work.
  • Adds scheduled benchmark-history freshness monitoring and an opt-in CLI recency assertion.
  • Carries bounded, sanitized worker failure diagnostics into collected artifacts.
  • Annotates publication no-ops and updates workflow-policy assertions.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains in the eligible follow-up review scope.

No blocking failure remains.

Important Files Changed

Filename Overview
scripts/ci/verify-cohort-reachability.mjs Adds bounded anonymous repository and top-level submodule reachability classification with explicit reachable, denied, and inconclusive outcomes.
.github/workflows/eval-benchmarks.yml Runs the reachability gate before credential validation, image construction, or sandbox launch.
.github/workflows/benchmark-history-freshness.yml Adds scheduled and manually configurable freshness checks with a deduplicated tracking-issue workflow.
packages/evals/src/history.ts Adds canonical newest-observation recency validation, including empty-history and future-timestamp rejection.
packages/cli/src/commands/eval/history.ts Exposes the recency assertion through an opt-in maximum-age flag without changing the default history check.
packages/modal/src/public-worker.ts Propagates sanitized command and eval diagnostics through the existing worker artifact channel.
packages/modal/src/worker-diagnostics.ts Centralizes bounded diagnostic serialization and aligns producer limits with collector validation.
.github/workflows/eval-history-publication.yml Adds step summaries distinguishing deliberate publication refusals from unreached readiness verdicts.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Push[Push or manual benchmark run] --> Reachability[Anonymous cohort reachability gate]
  Reachability -->|Denied| Block[Fail before sandbox launch]
  Reachability -->|Reachable or inconclusive| Generate[Generate benchmark results]
  Generate --> Qualify[Qualification and publication readiness]
  Qualify -->|Ready| Publish[Advance benchmark history]
  Qualify -->|Refused| Annotate[Keep operational soft-fail green and annotate]
  Publish --> Freshness[Scheduled freshness monitor]
  Annotate --> Freshness
  Freshness -->|Fresh| Healthy[Monitor succeeds]
  Freshness -->|Stale or unevaluable| Issue[Open or update tracking issue]
Loading

Reviews (2): Last reviewed commit: "fix(ci): satisfy the static-analysis gat..." | Re-trigger Greptile

aviggiano and others added 2 commits August 31, 2026 18:02
…of green (#1012)

Benchmark history published nothing between 2026-08-12 and 2026-08-31 while
the producer workflow reported success on nearly every push. The publication
gate refused correctly throughout; what was missing was any signal that it had.

Add a tokenless cohort reachability gate that runs before Modal spend, so a
dependency that is not anonymously cloneable fails in seconds naming the URL
rather than after a four-hour run that ends unpublishable. Add the first
scheduled freshness monitor and an opt-in `eval history --max-age-days`, so a
history that stops advancing is detectable at all. Annotate both silent
publication no-ops without changing any readiness verdict. Carry the failing
command's reason into the collected artifact through the existing
eval-failure-diagnostics channel, and close two paths where one
collector-illegal log line discarded a pair's whole artifact set.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…is job

#1005 wired `external-static-analysis` into `release-validation.needs` and
`release-gates.needs` but left the assertions in ci-config.test.ts expecting the
previous two- and three-element arrays, so `main` is red on
"runs the release validation lanes the policy script selects, on pull requests
too". Both assertions live in one test, so the first failure masked the second.

Unrelated to the rest of this branch; needed for its CI to be readable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aviggiano
aviggiano force-pushed the fix/1012-benchmark-history-publication-relay branch from 03cfea1 to 464cb3b Compare August 31, 2026 18:14
@aviggiano
aviggiano marked this pull request as ready for review August 31, 2026 19:33
Two independent classes of failure, both surfaced by gates that landed after
this branch was written.

Strict lint (ours). `eslint-plugin-diff` scopes the strict ruleset to changed
lines, so new code gets rules existing code is grandfathered against. Replace
console with process.stdout/stderr.write, remove non-null assertions in favour
of guards that name what was missing, split two over-long functions, and move
the new recency tests into their own file so history.test.ts returns to being
untouched rather than dragging its pre-existing 2131 lines into max-lines.

Workflow linters (inherited). Super-Linter lints only changed files but lints
each in full, and the repo carried no linter config, so stock yamllint defaults
applied to lines nobody wrote: eval-benchmarks.yml alone has 146 lines over 80
columns. Any workflow edit by anyone was failing. Add a config that relaxes
only what prettier already owns or GitHub mandates. Suppress zizmor's
dangerous-triggers on the publication workflow's workflow_run, whose trust
model rejects non-default-branch and fork producers before anything executes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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