Repository navigation
ci(cli): add a report-only CLI benchmark for pull requests - #7363
Prashansa-K wants to merge 16 commits into
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…s to bench-cli Adds a PR workload manifest, replay-server profile redirection and scenario control, per-run API call counts from traces, and a Markdown summary renderer. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Binds the replay server to loopback, routes the merge-base install through the dependency firewall, and renders a notice when no report was produced. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e v3 cut Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews were available. Code inspection confirms eight distinct findings: four minor reporting/failure-handling issues and four nits. No critical or major findings; no duplicates.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/scripts/bench-cli.ts:373 |
error-handling |
codex | A failed traced launch loses its exit code when no usable spans are decoded, allowing a timing verdict despite that failed invocation. |
| 🟡 MINOR | apps/cli/scripts/bench-report.ts:396 |
correctness |
codex | Unavailable API measurements become zero endpoint counts, producing false endpoint additions or removals. |
| 🟡 MINOR | .github/workflows/cli-bench.yml:54 |
ci |
codex | A branch-binary build failure fails the benchmark job and suppresses its explanatory summary, contrary to the documented report-only behavior. |
| 🟡 MINOR | .github/workflows/cli-bench.yml:102 |
error-handling |
claude | The benchmark runs after replay-server startup fails and then reports a misleading secondary failure. |
| ⚪ NIT | .github/workflows/cli-bench.yml:75 |
ci |
claude | The merge-base dependency install omits HUSKY=0, allowing hook installation to modify Git configuration shared with the main checkout. |
| ⚪ NIT | apps/cli/scripts/bench-summary.ts:181 |
error-handling |
claude | Report read and schema-decoding errors are silently presented as a missing report. |
| ⚪ NIT | apps/cli/scripts/bench-summary.ts:111 |
correctness |
claude | The phase-summary sentence incorrectly says no workload had a timing verdict whenever any workload is not comparable and no phase rows exist. |
| ⚪ NIT | apps/cli/scripts/bench-cli.ts:204 |
error-handling |
claude | Workload validation fails with a plain string, unlike the tagged error used by the new replay-control helper. |
Stats
Claude findings: 5 · Codex findings: 3 · Confirmed: 8 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Keep the job green when the branch build or the replay server fails, skip the dependent steps, and name the failed step in the summary. Set HUSKY=0 for the merge-base install. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Record the traced launch exit code independently of span decoding, skip endpoint changes when a build has no API measurement, separate absent from unreadable reports, fix the phase-summary wording, and raise a tagged workload validation error. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Lead with the answer, show commands and rounded timings, and describe each flag in a sentence. The comment variant adds the marker and a re-run hint. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The first run on a pull request from this repository posts one marker comment; a maintainer's exact /bench comment re-runs the head commit and updates it. Pull request code runs in a read-only job with no secrets, and a separate job posts the sanitized markdown. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
bench-pr keeps the shared setup action and firewall token; bench-command installs the toolchain with its cache off and dependencies from the public registry. Both call a new local action for the shared steps. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Reject /bench on forks before reacting, and collapse the two benchmark jobs back into one that uses the shared setup action and firewall token for both triggers, with the dependency cache off for /bench. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Limit the pull_request trigger and /bench to develop for now. /bench also rejects stacked pull requests based on another branch. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The resolve and comment jobs checked out the default branch, which lacks the cli-bench scripts until this change merges. Checkout now uses github.sha: default branch for /bench comments, the merge commit for pull_request runs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
CLI benchmark: no changes foundCommit
API calls: unchanged for all 7 commands. Binary size: 133.8 MB, unchanged. Details: timing ranges, API calls per command, slowest phasesTiming ranges
API calls per command
Slowest phasesNo command moved beyond noise, so there are no phase deltas. Comment |
Coly010
left a comment
There was a problem hiding this comment.
requesting changes for one thing, the rest is small
-
i think
/benchmight open up develop's cache to PR code. you've already called out the runner's cache token in the description, and i agree that for same-repo authors on normalpull_requestruns it's a wash. where i'm less sure is theissue_commentpath. that run is in develop's scope, and the bench job checks out the PR head and runs its own copy of.github/actions/setup, sodependency-cache: falseonly holds if the PR's action.yml honours it (and the build steps are in develop's cache scope either way). the release jobs inrelease.yml/release-shared.ymlrestore develop-scope caches onubuntu-latest, and the pnpm store one falls back to apnpm-store-files-<os>-<arch>-prefix match. so a same-repo branch (a renovate one included, if someone types/benchon it) could plant a store that the next release restores without it ever being reviewed. i haven't tried it end to end, but pushing a branch normally can't get anything into a release jobwould it work to keep
/benchbut have it re-run the PR's own benchmark run, instead of benchmarking inside the comment run? something like:resolvestays as it is onissue_comment(develop's code, same permission checks), finds the latestpull_requestrun of this workflow for the PR's current head, and re-runs it through the API (needsactions: writeonresolveonly)- a re-run keeps the original run's context, so the cache stays scoped to the PR and a fork's token stays read-only
- the comment job can upsert whenever
github.run_attempt > 1, which covers/benchand the re-run button in the Actions tab - if the latest push didn't touch the paths filter there's no run for the current head, so
/benchcould just reply saying so
that way the
issue_commentpath only ever runs develop's code, and PR code never gets develop's cache scope. i'm fairly sure aGITHUB_TOKENcan trigger a re-run, but it's worth confirming with one run -
smaller one:
runLaunchinbench-cli.tshas no timeout, so one hung launch sits until the 30 min job timeout and the summary loses every workload that already finished. anEffect.timeoutper launch that records a non-zero exit would land in the existing not-comparable path
happy to be told i'm missing something on 1. otherwise, it looks good to me!
/bench no longer checks out or runs pull request code. It re-runs the pull_request workflow run for the current head commit, which benchmarks and updates the comment, and replies on the pull request when nothing can be re-run. Later pushes skip while the comment reports a result. The comment keeps a collapsed list of earlier runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fixed the PR based on the two suggestions. Added in-depth description in the PR description.
|
Adds a non-blocking job that compares the CLI build of each ready pull request into
developwith a build of thedevelopcommit it branches from, without Docker, a database, or the network. It writes a job summary that leads with the answer ("no changes found" or "N things to check"), then one plain-words line per flag (including when a command that made no Management API calls starts making them), a before/after timing table per command, API call counts, binary size, and a collapsed section with timing ranges, per-command API calls, and the slowest phases for commands that moved.The first push to a pull request from this repository benchmarks and posts one comment with the same report, tagged with a hidden marker. Later pushes skip the benchmark while that comment reports a result, and the job summary says so. Pull requests from forks have a read-only token, so they get the job summary only and benchmark every push.
A maintainer can comment
/benchto run the benchmark again on the pull request's current head commit. The command re-runs that commit's workflow run (the same as "Re-run all jobs" in the Actions UI), and the re-run updates the comment in place. An eyes reaction on the/benchcomment means GitHub accepted the re-run; the result arrives as the updated benchmark comment, whether it succeeded or failed. If nothing can be re-run, the pull request gets a reply saying why: a benchmark is already running, the run is older than 30 days, GitHub refused the re-run, or there is no run for the commit. The comment keeps a collapsed "Previous runs" list (commit, time, headline, link; up to 20) in a hidden marker. Only the repository owner and people with write access can run it, and only on open, non-draft pull requests intodevelopfrom this repository. The whole comment must be exactly/bench; any other comment is ignored. Comment-triggered workflows run from the default branch, so/benchstarts working only after this merges todevelop.A pull request that changes none of the benchmarked paths, or more than the 300 files GitHub's path filter inspects, gets no run, so
/benchreplies that there is none.apps/cli/scripts/bench-cli.tsgains--workloads <manifest>and--api-url. Every launch gets a profile file pointing at a local replay server (the profile is the CLI's only API redirect), and the harness resets the server's request log and scenario before each launch. API calls are counted from HTTP client spans in the trace and cross-checked against the server's request log.apps/cli/scripts/bench-report.tsholds the report schemas and pure analysis;bench-summary.tsrenders the Markdown for the job summary and the comment;replay-control.tsis the client for the replay server's control API.apps/cli/bench/workloads.pr.jsonlists the workloads. They were chosen from command usage data, ranked and filtered to commands that need no Docker, database, network, or prompt; the method, query, and caveats are inapps/cli/docs/tracing-monitoring.md.apps/cli-e2e/scripts/replay-serve.tsstarts the existing replay server standalone..github/workflows/cli-bench.ymland.github/scripts/cli-bench/are new. No existing workflow or action is edited.The
/benchjob never checks out or runs pull request code: it runs default branch code and only asks GitHub to re-run the existingpull_requestrun, so benchmarks always execute in the pull request's own run and cache scope, like any other pull request job. Permissions are split by job: the job that decides whether to benchmark is read-only, the/benchjob is the only one withactions: write, the job that builds and runs pull request code has read-only permissions, and a separate job, the onlypull_requestjob with comment-write permission, downloads the rendered markdown, sanitizes it, and posts it; it never runs pull request code. Whether the workflow token may call the re-run endpoint is verified on the first/benchafter merge; a refusal is reported in the reply.Pull request runs now measure the pull request head commit instead of the test merge commit, so the commit shown in the report is the one that was benchmarked.
🤖 Generated with Claude Code