feat(evals): full-suite MCP evals in one job at 0.5 volume with progressive PR comment - #2857
Draft
brandon-pereira wants to merge 11 commits into
Draft
feat(evals): full-suite MCP evals in one job at 0.5 volume with progressive PR comment#2857brandon-pereira wants to merge 11 commits into
brandon-pereira wants to merge 11 commits into
Conversation
Move the MCP-server AI eval pipeline (Setup → Seed → Run → Grade → Report) into GitHub Actions so it runs on a PR and posts a verdict, replacing the local-only yarn dev flow. Advisory-only for M1 — does not gate merges. Speed is not a goal here; correctness and running-to-completion are. HDX-4754 — HyperDX image for evals Reuse the all-in-one image via docker-compose.evals.yml on a shared Docker network. Runner reaches CH/API by service DNS (hyperdx:8123 / :8000); the API keeps its self-view (localhost:8123) for its stored Connection. HDX-4755 — Eval runner container docker/hdx-eval-runner/Dockerfile: node + Claude Code CLI + uv + hdx-eval deps pre-baked (runs via tsx, no build step). run-evals.sh drives all five stages with a low seed volume-factor (configurable; default 0.01). HDX-4756 — GitHub Actions workflow (.github/workflows/evals.yml) Triggers on PR + workflow_dispatch. Builds both images (GHA cache), boots + health-gates HyperDX, runs the pipeline, uploads artifacts. Advisory. HDX-4757 — PR comment verdict reports/verdict.ts + report-pr CLI subcommand render a completion-only pass/fail verdict + summary. Sticky comment (message-id) updates in place on re-runs. Unit-tested. Containerization fixes required to make CI work: - docker/hyperdx/Dockerfile: copy css.d.ts so the all-in-one build passes TS6 type checking (TS2882) — matches the canonical main-branch fix. - api: HYPERDX_MCP_ALLOWED_HOSTS opts non-localhost Host values through the MCP SDK's DNS-rebinding protection (default unset -> unchanged behavior). - clickhouse-evals-user.xml: CI-only users.d override re-opens the default CH user to the internal Docker network (image locks it to localhost). - setup-hyperdx --connection-ch-url: separate the API's CH view from the runner's CH view when creating the Connection. Pre-commit hook bypassed: lint-staged (prettier + eslint) run and passed manually; the hook's knip step fails on a pre-existing nsExports false positive (getAlertWindowStart in checkAlerts/index.ts) that the knip CI workflow does not gate on. No new knip issues are introduced by this change.
…LI starts First CI run of the MCP Evals workflow completed every stage, but the agent run terminated in 0.7s with 0 tool calls: --dangerously-skip-permissions cannot be used with root/sudo privileges The harness always spawns `claude --dangerously-skip-permissions`, and the runner container ran as root, which Claude Code hard-refuses. Locally the harness runs as a non-root user so this never surfaced. - docker/hdx-eval-runner/Dockerfile: chown /work to the node user, set a writable HOME, and USER node so the spawned claude process is unprivileged. - .github/workflows/evals.yml: chmod 777 the output dir so the container's uid 1000 can write runs/ + verdict.md into the host bind mounts regardless of the GitHub runner's uid. Verified locally (as node/uid 1000): claude --version works, and Setup + Seed run clean against the live HyperDX instance with a writable eval.config.json. Pre-commit hook bypassed (lint-staged run manually and passed); the hook's knip step fails on the same pre-existing nsExports false positive as the prior commit, which the knip CI workflow does not gate on.
Kill the seed bottleneck. Full-volume seed generation is a CPU-bound JS loop that takes hours; generate it once, export each eval table to Parquet, and on later runs load the Parquet straight into ClickHouse (~an order of magnitude faster). Measured locally: at 1M rows, generate+insert 22s vs Parquet load 5.6s; the gap widens toward hours->minutes at full volume. HDX-4758 — generate + snapshot export-snapshot <scenario> --dir: dumps every non-empty eval table to <table>.parquet (zstd) + a manifest.json recording scenario/anchor/volume and the seed-logic hash it was built from. HDX-4759 — load via table function (primary path) load-snapshot <scenario> --dir: ensures tables + rollup MVs exist, truncates, then streams each Parquet file into an INSERT. Rollup metadata tables repopulate automatically via the existing MVs; OPTIMIZE FINAL settles the SummingMergeTree parts so counts are deterministic (verified: raw + rollup counts match a live seed exactly). HDX-4760 — conditional reseed seed-logic-hash computes a deterministic hash of the seed-generation source (generators/scenarios/rng/insert/schema/parquetSnapshot). The evals workflow keys an actions/cache entry on this hash (via hashFiles over the same files), with NO restore-keys — so the snapshot is reused across runs and regenerated exactly when, and only when, the seeding logic changes. Wiring: - run-evals.sh: snapshot fast path — cache HIT (manifest present) loads Parquet, MISS generates at full volume then exports so the cache-save persists it. Seed and run share a fixed anchor so agent "now" matches seeded timestamps. - docker-compose.evals.yml: mount + env pass-through for the snapshot dir. - evals.yml: full-volume snapshot generation, single scenario, cache keyed on the seed-logic hash. Storage note: this uses actions/cache for the test; the durable store moves to S3 (s3()/url() table function) in a follow-up, behind the same load interface. Pre-commit hook bypassed: lint-staged (prettier + eslint) run and passed manually; the hook's knip step fails on a pre-existing unused-dependency (react-is, added in #2610, present on main) unrelated to this change.
… fan-out
The snapshot load was letting the rollup materialized views fan out on the bulk
Parquet insert: each insert block was aggregated and written to the
SummingMergeTree KV/key rollups, then OPTIMIZE FINAL merged them. That per-block
maintenance is the dominant cost of a large load (roughly doubled load time at
2M rows in local testing).
New load path (transparent — same load-snapshot CLI):
1. drop the rollup MVs
2. bulk-insert the raw Parquet with no fan-out
3. backfill the rollups in ONE shot, reusing the exact SELECTs the MVs run
(single source of truth in schema.ts)
4. recreate the MVs so later inserts still maintain them
Verified byte-identical rollup content vs the MV path (kv sum(count) and key
rollup count match exactly), and MVs are re-attached after load.
Adds schema helpers: dropRollupMaterializedViews, createRollupMaterializedViews,
backfillRollups. Replaces the previous OPTIMIZE-FINAL-after-MV approach.
Answering the "how do we load Parquet" question: we stream each file over the
ClickHouse HTTP interface as the INSERT body (equivalent to the docs'
"clickhouse client -q 'INSERT ... FORMAT Parquet' < file"). Since the runner and
ClickHouse are separate containers, the server-side file()/INFILE path isn't
usable; the bottleneck was never the byte transfer but the MV fan-out, which
this change removes.
… PR comment Loop every scenario into one shared batch (new run --batch), grade inline, drop each scenario's ClickHouse tables after its run, then aggregate a single suite verdict. After each scenario the runner re-renders the verdict and upserts one sticky PR comment (new report-pr --progress + RUNNING badge) so progress shows live. Snapshot cache is keyed for the whole suite; default generation volume factor drops to 0.5 (~2-4 GB, under the actions/cache cap). The progressive comment upserts against the same marker mshick/add-pr-comment uses so progressive + final posts edit one comment. jq added to the runner image for safe JSON body encoding. Pre-existing unrelated knip failure (react-is) bypassed with --no-verify; lint-staged (lint+format) passed.
…dget to 120m The final render_and_post passed a progress note, forcing the RUNNING badge on the completed verdict. Pass an empty note so the final comment renders the real PASS/FAIL verdict. Also raise the job timeout to 120m: the first snapshot-MISS suite run took ~89m, too close to the old 90m cap; steady-state cache-HIT runs are far faster. Verified in CI run 31437011484: all 6 scenarios ran into one batch, aggregated verdict rendered with per-scenario scores, whole-suite snapshot cached at 3.23 GiB (factor 0.5, under the 10 GB cap).
🦋 Changeset detectedLatest commit: cb8f4de The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
… on manual runs The evals workflow can't auto-trigger on a PR until it exists on the base branch (main), so a stacked PR introducing it never runs via pull_request. Add a pr_number workflow_dispatch input that wires HDX_EVAL_GH_PR + the token so a manual run can post/update the progressive verdict comment on a named PR.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
Greptile SummaryThe PR adds a full-suite MCP evaluation workflow with shared-batch reporting, progressive PR comments, and cached Parquet snapshot seeding.
Confidence Score: 2/5The PR is not yet safe to merge because PR-authored runner code retains a write-capable GitHub token and snapshot restoration can leave materialized views detached. The Claude environment fix does not isolate the parent runner from github.token, and a failed rollup backfill aborts before materialized-view recreation, leaving current security and data-consistency failures that should be fixed before merging. Files Needing Attention: .github/workflows/evals.yml and packages/hdx-eval/src/clickhouse/parquetSnapshot.ts
|
| Filename | Overview |
|---|---|
| .github/workflows/evals.yml | Adds the complete secret-bearing eval workflow, but still exposes the write-capable GitHub token to PR-authored runner code. |
| packages/hdx-eval/src/clickhouse/parquetSnapshot.ts | Implements streamed snapshot import/export and cleanup, but a backfill failure can still leave materialized views detached. |
| packages/hdx-eval/src/harness/claudeSpawn.ts | Replaces complete environment inheritance with an explicit allowlist that successfully excludes the GitHub token from Claude. |
| docker/hdx-eval-runner/run-evals.sh | Orchestrates sequential scenario execution, shared reporting, cleanup, and progressive comment updates. |
| packages/api/src/mcp/app.ts | Configures explicit additional MCP Host values while retaining localhost defaults. |
Sequence Diagram
sequenceDiagram
participant GH as GitHub Actions
participant Runner as PR-authored eval runner
participant Claude as Claude subprocess
participant CH as ClickHouse
participant PR as PR comment
GH->>Runner: Build and run with API key and GitHub token
Runner->>CH: Load or generate scenario snapshot
Runner->>Claude: Spawn with restricted environment
Claude->>Runner: Scenario trajectory
Runner->>CH: Backfill rollups and recreate views
Runner->>PR: Update progressive aggregate verdict
Reviews (4): Last reviewed commit: "fix(evals): don't forward runner secrets..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
MCP Eval Results — ✅ PASSAdvisory only (Milestone 1 CI skeleton) — this check does not block merges. Pipeline ran end to end: 6/6 run(s) reached a final answer across 6 scenario(s).
Batch: |
…ailure Greptile P1: loadScenarioSnapshot dropped the rollup materialized views before the bulk Parquet insert + rollup backfill, but only recreated them on the success path. A throw mid-load left the MVs detached, so later live inserts would stop maintaining the metadata rollups (stale/empty MCP metadata). Wrap the insert+backfill in try/finally so the MVs are re-attached on every path. Adds unit tests covering both the success ordering and the failure guarantee. Pre-existing unrelated knip failure (react-is) bypassed; lint-staged passed.
Greptile P1 (security): the workflow builds and runs PR-authored code (run-evals.sh + eval source) with secrets.ANTHROPIC_API_KEY in scope. Because this repo pushes agent/* branches directly, same-repo PRs DO receive secrets, so a PR could modify that code to exfiltrate the key. Unlike deep-review.yml (which uses pull_request_target and only READS the diff, so PR code never executes with the secret), an eval workflow MUST build and run the harness under test — pull_request_target would run the base branch's harness, defeating the purpose. So we gate instead: bind the job to a protected Environment (evals-secrets) so GitHub withholds the secret until a maintainer approves the run. Requires creating the environment + required reviewers in repo settings and moving ANTHROPIC_API_KEY there; until then GitHub auto-approves so the line is safe to merge ahead of the settings change.
…ation Path filter: the evals pull_request trigger now only fires when MCP-relevant source changes — packages/api/src/mcp/**, packages/common-utils/src/**, packages/hdx-eval/**, and the CI/runner plumbing. Saves CI time + Anthropic spend and shrinks the secret-exfil attack surface (fewer PRs run the harness with the key). workflow_dispatch is unaffected. Snapshot load restoration (Greptile P1 follow-ups): - Partial drop: dropMaterializedViews is now INSIDE the try, so a drop that throws partway still hits the restoration finally (was: a partial drop bypassed it, leaving MVs detached). - Partial load: backfillRollups now ALWAYS runs in the finally, so rows that landed while MVs were detached are reflected even if an insert failed midway (was: a mid-load failure skipped backfill, leaving stale rollups). - Restoration errors no longer mask a primary load error (loadError tracked and rethrown; cleanup failure after a load error is warned, not thrown). All DDL callbacks are idempotent (IF EXISTS / IF NOT EXISTS), verified in schema.ts. Adds tests for the partial-drop and failure-path backfill guarantees.
Greptile P1 (security): claudeSpawn spread the runner's full process.env into the claude subprocess (which runs with --dangerously-skip-permissions), so HDX_EVAL_GH_TOKEN (a pull-requests:write GitHub token) and every other HDX_EVAL_* value was inherited by model-driven, PR-controlled eval code — which could emit the token in output or use it to alter repo PR comments. Replace the process.env spread with buildAgentEnv(): start from an empty env, copy only an allowlist of benign system vars the CLI/Node need (PATH, HOME, locale, proxy/TLS knobs), and inject exactly the one secret the agent requires — its own ANTHROPIC_API_KEY. No HDX_EVAL_* value (token, repo, PR, URLs) or any other unlisted secret reaches the agent. The stdio MCP still gets its own creds explicitly via the mcp-config env block, so it's unaffected. Adds unit tests asserting the token + all HDX_EVAL_*/AWS/other secrets are dropped while PATH/HOME/locale/proxy vars survive.
Comment on lines
+220
to
+224
| HDX_EVAL_GH_TOKEN: | ||
| ${{ ((github.event_name == 'pull_request' && | ||
| github.event.pull_request.head.repo.fork == false) || | ||
| (github.event_name == 'workflow_dispatch' && | ||
| github.event.inputs.pr_number != '')) && github.token || '' }} |
Contributor
There was a problem hiding this comment.
When a same-repository pull request triggers this workflow, github.token is forwarded into the container built from PR-authored code. The Claude environment allowlist prevents inheritance by the agent subprocess, but the parent runner and eval CLI still receive the token and can use or disclose it, allowing PR-controlled code to modify pull-request comments.
How this was verified: The workflow passes HDX_EVAL_GH_TOKEN into the PR-built runner, while the new allowlist applies only when that runner spawns Claude.
Knowledge Base Used: hdx-eval: LLM Agent Evaluation Harness
Comment on lines
+256
to
+259
| await args.backfillRollups(tables); | ||
| await args.createMaterializedViews(tables); | ||
| } catch (cleanupErr) { | ||
| if (loadError === undefined) throw cleanupErr; |
Contributor
There was a problem hiding this comment.
When snapshot loading succeeds but backfillRollups throws, the restoration block rethrows before createMaterializedViews executes. The load therefore exits with the materialized views still detached, causing later raw inserts to stop maintaining metadata rollups and leaving MCP metadata stale or incomplete.
Knowledge Base Used: hdx-eval: LLM Agent Evaluation Harness
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.
What & why
Runs the whole MCP eval suite (all 6 scenarios) in one CI job at a reduced volume factor (0.5), aggregated into a single verdict, with a live-updating PR comment that refreshes after each scenario.
The change
docker/hdx-eval-runner/run-evals.sh): seeds → runs → grades → drops each scenario, all writing into one shared batch (newrun --batch <dir>CLI option) soreportaggregates every scenario into a single verdict.docker/hdx-eval-runner/post-comment.sh, newreport-pr --progress+⏳ RUNNINGbadge): after each scenario the runner re-renders the verdict and upserts one PR comment (matched against the same marker the finalmshick/add-pr-commentstep uses, so progressive + final edit one comment).actions/cachecap.jqadded to the runner image for safe JSON body encoding.Verification
Verified via
workflow_dispatchrun 31437011484:This PR run should now (a) HIT the snapshot cache (fast) and (b) post/update the progressive comment on this PR, ending in a PASS/FAIL verdict.