Repository navigation
ci: run perf jobs on bamboo - #36
Conversation
📝 WalkthroughWalkthroughThe changes add local performance tasks and two GitHub Actions workflows. The workflows record Valgrind instruction counts, compare pull-request changes with the merge base, publish main-branch history, upload reports, update pull-request comments, and enforce regression gates. ChangesPerformance automation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant GitHubActions
participant ValgrindTak
participant ArtifactStorage
participant PullRequestComment
PullRequest->>GitHubActions: Trigger perf-pr workflow
GitHubActions->>ValgrindTak: Measure PR head and merge base
ValgrindTak-->>GitHubActions: Return report and gate status
GitHubActions->>ArtifactStorage: Upload report and status
ArtifactStorage-->>GitHubActions: Download report artifact
GitHubActions->>PullRequestComment: Create or update sticky comment
GitHubActions-->>PullRequest: Report success or fail the gate
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThe PR adds dedicated performance workflows for main and eligible pull requests, backed by reusable mise tasks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (6): Last reviewed commit: "ci: build perf binaries on bamboo" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/perf.yml (2)
32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister
bamboo-perfwith actionlint in both workflows. Both files trigger the same actionlintrunner-labelfalse positive becausebamboo-perfis a self-hosted label that isn't declared to actionlint. Add it once toactionlint.yamlto fix both findings.
.github/workflows/perf.yml#L32-L42: no per-file change needed onceactionlint.yamllistsbamboo-perf; this is the root-cause site to reference when adding the config..github/workflows/perf-pr.yml#L39-L39: same fix applies here onceactionlint.yamlis updated.🔧 Proposed actionlint.yaml addition
self-hosted-runner: labels: - bamboo-perfAs per static analysis hints, actionlint flagged
label "bamboo-perf" is unknownon both files.🤖 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/perf.yml around lines 32 - 42, Add the self-hosted runner label bamboo-perf to actionlint.yaml under self-hosted-runner.labels. No direct changes are needed at .github/workflows/perf.yml lines 32-42 or .github/workflows/perf-pr.yml line 39; both runner-label findings are resolved by the shared actionlint configuration.Source: Linters/SAST tools
43-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the checkout/toolchain/instrumentation setup between the two workflows. Both files duplicate the same checkout,
Swatinem/rust-cache,jdx/mise-action, valgrind install, andTAK_RUNNERvalue. Because this PR's comparisons depend on main and pull-request runs using identical instrumentation, letting these two blocks drift (for example, a valgrind version bump landing in only one file) would silently invalidate the instruction-count comparison instead of failing loudly.
.github/workflows/perf.yml#L43-L65: extract this setup sequence into a reusable composite action (or a shared workflow) that both files call..github/workflows/perf-pr.yml#L46-L73: replace this duplicated sequence with a call to the same composite action.🤖 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/perf.yml around lines 43 - 65, Extract the duplicated checkout, Swatinem/rust-cache, jdx/mise-action, valgrind installation, and TAK_RUNNER setup into one reusable composite action or shared workflow. Update .github/workflows/perf.yml lines 43-65 and .github/workflows/perf-pr.yml lines 46-73 to invoke that shared setup, preserving identical instrumentation and configuration in both workflows.
🤖 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/perf-pr.yml:
- Around line 33-58: Before allowing the measure job to run external pull
requests on bamboo-perf, verify maintainer approval, ephemeral per-job runner
cleanup, and isolation from trusted workflows and secrets; if these controls are
unavailable, gate or remove the untrusted PR trigger or move measurement to a
GitHub-hosted runner.
---
Nitpick comments:
In @.github/workflows/perf.yml:
- Around line 32-42: Add the self-hosted runner label bamboo-perf to
actionlint.yaml under self-hosted-runner.labels. No direct changes are needed at
.github/workflows/perf.yml lines 32-42 or .github/workflows/perf-pr.yml line 39;
both runner-label findings are resolved by the shared actionlint configuration.
- Around line 43-65: Extract the duplicated checkout, Swatinem/rust-cache,
jdx/mise-action, valgrind installation, and TAK_RUNNER setup into one reusable
composite action or shared workflow. Update .github/workflows/perf.yml lines
43-65 and .github/workflows/perf-pr.yml lines 46-73 to invoke that shared setup,
preserving identical instrumentation and configuration in both workflows.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fcf76300-b107-4e24-a46f-9fe58e59dcce
📒 Files selected for processing (3)
.github/workflows/perf-pr.yml.github/workflows/perf.ymlmise.toml
Instruction counts
2 benchmark(s) above the 1% gate: Measured on the base but not here — a benchmark that stops running also stops gating: Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3ac139e. Configure here.
| report: | ||
| name: Report and gate | ||
| needs: measure | ||
| if: always() && github.event.pull_request.user.login == 'jdx' |
There was a problem hiding this comment.
Report runs after cancelled measure
Medium Severity
With cancel-in-progress: true, a superseded measure run is cancelled while report still starts because of always(). That job then tries to download the tak-report artifact from a run that often never uploaded it, so the check fails even though a newer workflow run may succeed.
Reviewed by Cursor Bugbot for commit 3ac139e. Configure here.
1 participant
Summary
perfandperf:recordmise tasks for Tak itselfbamboo-perfrunnerValidation
actionlint(withbamboo-perfregistered as the expected custom label)mise tasks lsgit diff --checkmainbaseline queued on the disposable runnerAI-assisted — Tool: Codex; model: unavailable; version: unavailable.
Note
Medium Risk
Introduces self-hosted CI that writes git notes on main and executes PR-supplied code on bamboo-perf, though PR measure jobs are read-only and commenting is isolated on GitHub-hosted runners.
Overview
Adds instruction-count performance CI on the dedicated
bamboo-perfrunner, withmisetasksperfandperf:record(release build +tak run/tak run --record).perfonmainmeasures each merge (and optionalworkflow_dispatch), records viaperf:record, and pushes results torefs/notes/takonly onmain. Builds run withouttarget/cache and install valgrind so counts stay comparable on one runner class (TAK_RUNNERpinned).perf-prcompares the PR head (not the merge commit) against the merge-base withtak compare, fails the check on regressions, and posts/updates a sticky PR comment from a separatereportjob onubuntu-latestso PR code never runs with write tokens. PR measurements stay local (no notes push). Workflows are currently gated to PRs from userjdx; actionlint registers thebamboo-perflabel.Reviewed by Cursor Bugbot for commit 4e13307. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit