ci: track test suite durations in Bencher#12404
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThree new Node.js CLI scripts measure command timing, extract metrics from pnpm summaries, and merge Bencher result files. The ChangesBencher Benchmark Integration
Sequence Diagram(s)sequenceDiagram
participant TestCI as Test/Pacquet CI
participant measure as measure-command.mjs
participant pnpmExtract as bencher-result-from-pnpm-summary.mjs
participant merge as merge-bencher-results.mjs
participant artifacts as GitHub Artifacts
participant BencherUpload as ci-performance-bencher-upload
participant BencherAPI as Bencher API
TestCI->>measure: wrap test execution
measure-->>TestCI: .bench/tests.all.json
alt full_tests enabled
TestCI->>pnpmExtract: extract CLI e2e duration
pnpmExtract-->>TestCI: .bench/tests.cli.json
TestCI->>merge: concatenate results
merge-->>TestCI: merged results + metadata
end
TestCI->>artifacts: upload ci-performance-pnpm artifact
artifacts-->>BencherUpload: workflow_run trigger on CI success
BencherUpload->>artifacts: download ci-performance-* artifacts
BencherUpload->>BencherUpload: validate metadata + testbed + filenames
BencherUpload->>BencherAPI: bencher run --upload (if BENCHER_API_TOKEN set)
BencherAPI-->>BencherUpload: results published to Bencher
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8049b11 to
635910c
Compare
Code Review by Qodo
1. Untrusted PR results uploaded
|
PR Summary by QodoCI: Upload pnpm/pacquet test suite durations to Bencher WalkthroughsDescription• Measure CI test suite durations and emit Bencher-compatible hyperfine JSON results. • Upload pnpm/pacquet/pnpr test durations to Bencher when BENCHER_API_TOKEN is configured. • Extract pnpm CLI e2e duration from --report-summary output without rerunning tests. Diagramgraph TD
CI[".github/workflows/ci.yml"] --> TEST[".github/workflows/test.yml"]
TEST --> MEASURE[".github/scripts/measure-command.mjs"] --> ALL[(".bench/tests.*.json")] --> MERGE[".github/scripts/merge-bencher-results.mjs"] --> BENCHERCLI["Bencher CLI"] --> BENCHER{{"Bencher Cloud"}}
TEST --> SUMMARY[("pnpm-exec-summary.json")] --> EXTRACT[".github/scripts/bencher-result-from-pnpm-summary.mjs"] --> CLI[(".bench/tests.cli.json")] --> MERGE
PACQ[".github/workflows/pacquet-ci.yml"] --> MEASURE --> RUST[(".bench/*-tests-all.json")] --> BENCHERCLI
subgraph Legend
direction LR
_wf[Workflow] ~~~ _scr["Script"] ~~~ _file[(Result file)] ~~~ _ext{{External}}
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Use hyperfine directly in CI
2. Derive durations from GitHub Actions timings (API/job metadata)
Recommendation: The PR’s approach (lightweight Node scripts producing shell_hyperfine-compatible JSON, plus guarded Bencher uploads) is a good fit: it keeps CI dependencies minimal, works cross-platform, and reuses pnpm --report-summary to avoid rerunning CLI e2e tests. If future needs expand to richer statistics (multiple samples, variance), consider switching to hyperfine for the measured steps. File ChangesOther (8)
|
Code Review by Qodo
1. Missing inputs silently skipped
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/measure-command.mjs:
- Around line 9-10: The scripts have a path traversal vulnerability where the
unsanitized `output` argument can escape the `.bench/` directory (e.g., via
`--output ../../file` or absolute paths). At each of the three locations, you
must: (1) resolve the `output` argument to an absolute path, (2) validate that
the resolved path is contained within the workspace's `.bench/` directory by
checking that the resolved path starts with the .bench directory path, (3) throw
an error if the path escapes .bench, and (4) use the validated absolute path
when calling mkdir in `.github/scripts/measure-command.mjs` lines 9-10,
`.github/scripts/bencher-result-from-pnpm-summary.mjs` lines 26-27, and
`.github/scripts/merge-bencher-results.mjs` lines 22-23. Apply the identical
validation and boundary-check pattern at all three sites before creating or
writing files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bc87be62-25a2-443e-bbc0-4e0da2e31441
📒 Files selected for processing (8)
.github/scripts/bencher-result-from-pnpm-summary.mjs.github/scripts/measure-command.mjs.github/scripts/merge-bencher-results.mjs.github/workflows/ci.yml.github/workflows/pacquet-ci.yml.github/workflows/test.ymljustfilepackage.json
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Code review by qodo was updated up to the latest commit 651b5d2 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
184-198: ⚡ Quick winConsider validating platform and node version inputs against supported testbed patterns.
The testbed name
pnpm.${platform_slug}.node${node_major}is constructed from workflow inputs without validation. The downstream Bencher upload workflow only acceptspnpm.(ubuntu|windows).node(22|24|26)patterns. If the calling workflow passesmacos-latestor an unsupported Node version, the artifact will upload successfully here but fail during the Bencher upload step.Consider adding early validation to fail fast with a clear error message:
🛡️ Suggested validation check
- name: Stage Bencher test durations if: steps.test-scope.outputs.full_tests == 'true' env: NODE_VERSION: ${{ inputs.node }} PLATFORM: ${{ inputs.platform }} shell: bash run: | platform_slug=${PLATFORM%-latest} node_major=${NODE_VERSION%%.*} + + # Validate against supported testbed patterns + if [[ ! "$platform_slug" =~ ^(ubuntu|windows)$ ]]; then + echo "::error::Unsupported platform '$platform_slug' for pnpm benchmarks (expected ubuntu or windows)" + exit 1 + fi + if [[ ! "$node_major" =~ ^(22|24|26)$ ]]; then + echo "::error::Unsupported Node major version '$node_major' for pnpm benchmarks (expected 22, 24, or 26)" + exit 1 + fi + artifact_dir=.bench-artifact/pnpm mkdir -p "$artifact_dir"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 184 - 198, The testbed name construction using platform_slug and node_major lacks validation against the supported patterns required by the downstream Bencher upload workflow. Add validation checks after extracting platform_slug from PLATFORM and node_major from NODE_VERSION to ensure platform_slug matches either "ubuntu" or "windows" and node_major matches one of "22", "24", or "26". If either validation fails, exit with a clear error message that specifies which value is unsupported and what the allowed values are, so failures happen immediately rather than during the later Bencher upload step.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci-performance-bencher-upload.yml:
- Around line 167-179: The Rust case in the workflow currently allows the job to
succeed even when the metadata is malformed or incomplete (missing reports
field, empty reports, or wrong number of reports). Before iterating through the
reports in the `rust)` case statement, add validation to fail closed by checking
that the reports field exists and contains exactly two entries with the exact
filenames pacquet-tests-all.json and pnpr-tests-all.json. Extract the reports
array from the metadata, verify its length is exactly 2, verify each report's
filename matches one of the expected names, and exit with status 1 if any
validation fails. This ensures the job fails rather than silently succeeding
with incomplete data when the producer contract is violated.
In @.github/workflows/ci.yml:
- Around line 32-34: The current implementation uses newline-delimited path
handling with find and mapfile -t, which creates an argument injection
vulnerability if any filename contains a newline character. This allows crafted
paths to be split into multiple argv tokens, with a fragment starting with --
being misinterpreted by tar as an option. Replace the newline-delimited approach
on lines 32 and 33 with null-delimited path handling by using find with the
-print0 flag and mapfile with the -d '' (null delimiter) option. This ensures
that paths containing newlines are treated as single arguments and prevents
argument injection attacks in the tar command on line 34.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 184-198: The testbed name construction using platform_slug and
node_major lacks validation against the supported patterns required by the
downstream Bencher upload workflow. Add validation checks after extracting
platform_slug from PLATFORM and node_major from NODE_VERSION to ensure
platform_slug matches either "ubuntu" or "windows" and node_major matches one of
"22", "24", or "26". If either validation fails, exit with a clear error message
that specifies which value is unsupported and what the allowed values are, so
failures happen immediately rather than during the later Bencher upload step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f9e8f72-8808-44c0-967f-a6350d1d8ee9
📒 Files selected for processing (9)
.github/scripts/bencher-result-from-pnpm-summary.mjs.github/scripts/measure-command.mjs.github/scripts/merge-bencher-results.mjs.github/workflows/ci-performance-bencher-upload.yml.github/workflows/ci.yml.github/workflows/pacquet-ci.yml.github/workflows/test.ymljustfilepackage.json
🚧 Files skipped from review as they are similar to previous changes (4)
- justfile
- .github/scripts/merge-bencher-results.mjs
- .github/scripts/measure-command.mjs
- .github/scripts/bencher-result-from-pnpm-summary.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/bench-output-path.mjs:
- Line 7: The boundary check in the condition on line 7 only validates lexical
path traversal and does not account for symlinks, which can be used to escape
the intended directory. After the existing lexical validations for empty paths,
parent directory references, and absolute paths, you must resolve the actual
filesystem path (following any symlinks) and verify that the resolved path is
within the intended .bench directory. Use a function like realpath() to resolve
symlinks and then check that the resolved path starts with the intended
directory's resolved path to ensure it cannot escape outside the boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3794480f-6870-45b3-b1d8-a1e7696799bf
📒 Files selected for processing (10)
.github/scripts/bench-output-path.mjs.github/scripts/bencher-result-from-pnpm-summary.mjs.github/scripts/measure-command.mjs.github/scripts/merge-bencher-results.mjs.github/workflows/ci-performance-bencher-upload.yml.github/workflows/ci.yml.github/workflows/pacquet-ci.yml.github/workflows/test.ymljustfilepackage.json
🚧 Files skipped from review as they are similar to previous changes (8)
- justfile
- .github/scripts/merge-bencher-results.mjs
- .github/workflows/test.yml
- .github/scripts/bencher-result-from-pnpm-summary.mjs
- package.json
- .github/workflows/pacquet-ci.yml
- .github/scripts/measure-command.mjs
- .github/workflows/ci-performance-bencher-upload.yml
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Code review by qodo was updated up to the latest commit e138d22 |
This was referenced Jun 14, 2026
2 tasks
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.
Summary
Adds CI duration tracking for the
pnpm-ci-performanceBencher project.Tracked Rust testbeds and benchmarks:
pacquet.ubuntu,pacquet.windows,pacquet.macos->tests.allpnpr.ubuntu,pnpr.windows,pnpr.macos->tests.allTracked pnpm testbeds and benchmarks for full test runs:
pnpm.ubuntu.node22,pnpm.ubuntu.node24,pnpm.ubuntu.node26->tests.all,tests.clipnpm.windows.node22,pnpm.windows.node24,pnpm.windows.node26->tests.all,tests.cliThe test workflows produce Bencher-compatible JSON artifacts without receiving
BENCHER_API_TOKEN. A separateworkflow_runworkflow downloads those artifacts only for same-repository runs, validates their metadata, and uploads from trusted workflow code using the existingBENCHER_API_TOKENsecret. The pnpm CLI e2e duration is extracted frompnpm run --report-summaryoutput during the same full-test execution, so the CLI e2e suite is not run a second time.Validation
actionlint .github/workflows/ci.yml .github/workflows/test.yml .github/workflows/pacquet-ci.yml .github/workflows/ci-performance-bencher-upload.ymlgit diff --checknode --check .github/scripts/bench-output-path.mjs && node --check .github/scripts/measure-command.mjs && node --check .github/scripts/bencher-result-from-pnpm-summary.mjs && node --check .github/scripts/merge-bencher-results.mjstests.all+tests.cli.reports.bench/**.github/scripts/merge-bencher-results.mjsjust --dry-run test-pacquetjust --dry-run test-pnprgit pushWritten by an agent (Codex, GPT-5).
Summary by CodeRabbit
Chores