Fix Bencher reporting permanently broken on pushes to main#3148
Fix Bencher reporting permanently broken on pushes to main#3148alexeyr-ci2 wants to merge 1 commit intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughGitHub Actions workflow updated to compute a single Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant Bencher as Bencher CLI
participant API as Git/GitHub API
alt push to main
Note over GHA: START_POINT_ARGS = (empty)
else pull_request / workflow_dispatch
Note over GHA: START_POINT_ARGS = --start-point ... --start-point-hash HASH ...
end
GHA->>Bencher: run_bencher(START_POINT_ARGS)
Bencher->>API: request baseline (may include HASH)
API-->>Bencher: response (baseline or not found)
alt Baseline found
Bencher-->>GHA: success (exit 0)
else Baseline not found (stderr indicates pinned hash missing)
Bencher-->>GHA: stderr captured, non-zero exit
GHA->>GHA: compute retry args (strip --start-point-hash if present)
alt retry args differ
GHA->>Bencher: run_bencher(retry args)
Bencher->>API: request baseline (without HASH)
API-->>Bencher: response (latest baseline)
Bencher-->>GHA: success/fail
end
end
GHA->>GHA: print stderr, export BENCHER_EXIT_CODE
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Code ReviewOverall: This is a well-targeted fix. The root cause (passing CorrectnessRoot cause fix — correct. For Retry logic — good improvement over the previous "swallow 404" approach. Instead of silently ignoring failures, the PR now retries with a degraded-but-functional baseline. The guard
Concerns1. Fragile error-message matching for retry trigger (line 647) grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"The retry only fires when Bencher's stderr contains exactly these two substrings. If Bencher ever changes its error format, the retry silently stops working and the old 404-swallowing behaviour is gone too — meaning the run just fails. Consider adding a comment citing the specific Bencher error string you observed (e.g. 2. Unquoted $sp_args \The 3. RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//')This works correctly for the current single-line string format and hex SHA values. It's worth noting it could leave a double space in the resulting string, but bencher's CLI treats that as equivalent to a single space so it's not a bug. Minor
Summary: The fix is correct and the retry logic is a meaningful improvement. The main fragility is that the retry trigger depends on Bencher's error message text staying stable — worth a comment to future-proof it. Otherwise, ready to merge with that minor note addressed. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # Bencher (e.g., the base commit was a docs-only change skipped by | ||
| # paths-ignore), retry without --start-point-hash so the report | ||
| # still gets stored using the latest available baseline. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"; then |
There was a problem hiding this comment.
The retry condition depends on Bencher's exact stderr wording staying stable. If the error format changes, the retry silently stops working (and without the old 404-swallowing fallback, the workflow just fails).
Consider pinning the expected message in a comment, e.g.:
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"; then | |
| # Bencher emits e.g. "Head Version '<sha>' not found" when the pinned hash | |
| # isn't in its DB. Grep for both fragments so a message-format change | |
| # surfaces clearly rather than silently skipping the retry. | |
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"; then |
| # Wrap bencher run in a function so we can retry with different | ||
| # start-point args if the pinned hash isn't found in Bencher. | ||
| run_bencher() { | ||
| local sp_args="$1" |
There was a problem hiding this comment.
The # shellcheck disable=SC2086 on line 592 is correct — word-splitting $sp_args is intentional so the multi-word arg string is split into individual flags. It's worth adding a brief inline note so the next reader understands why it must be unquoted:
| local sp_args="$1" | |
| local sp_args="$1" | |
| # $sp_args must be unquoted so the shell word-splits the multi-flag | |
| # string into individual arguments (e.g. "--start-point main --start-point-hash abc123"). | |
| # shellcheck disable=SC2086 |
alexeyr-ci2
force-pushed
the
alexeyr/2546/Fix-benchmark-workflow
branch
from
a72ff8d to
8c45b36
Compare
Code ReviewOverviewThis PR correctly identifies and fixes the root cause of cascading Bencher failures on main pushes: passing `--start-point main --start-point-hash ` when main is the base branch created a lookup for a hash that often did not exist (docs-only commits skipped by `paths-ignore`), causing a 404 and permanently breaking subsequent runs. The refactoring to a `run_bencher()` function to eliminate the ~30-line command duplication is a clear improvement. Issues[Medium] Retry logic removes the old regression-masking guard The old code explicitly prevented masking real regressions by checking for the absence of regression indicators before swallowing an error. The new retry block has no equivalent guard. While a regression alert and a "Head Version not found" error are unlikely to co-occur in a single Bencher run, the original defensive check was explicit about never retrying when regression indicators are present. Without it, if Bencher ever emits both in one run, the retry could produce an exit-0 result that masks a real regression. Consider adding back the guard before the retry block: && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"[Low] `--start-point-reset` silently added to `workflow_dispatch` non-main branches The old dispatch case had `EXTRA_ARGS=""` (no `--start-point-reset`); the new code includes it in both dispatch hash paths. This changes Bencher's behavior for manually-dispatched branch runs — if the branch already exists in Bencher, it will now be reset to main's state. If intentional (and it probably is correct), a brief comment explaining the reasoning would help. [Low] `sed`-based hash stripping is fragile `RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//')` This works for the current arg format but would silently fail to strip (and skip the retry) if the Bencher flag is ever renamed. A bash array (`declare -a SP_ARGS=(...)`) would make the manipulation explicit and avoid the word-splitting workaround entirely, at the cost of slightly more verbose construction code. What looks good
|
| # Bencher emits: 'Head Version (..., Some(GitHash("<sha>"))) not found' | ||
| # We grep for both fragments so a format change surfaces clearly | ||
| # rather than silently skipping the retry. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"; then |
There was a problem hiding this comment.
The old error-handling block included an explicit guard to prevent retrying (and potentially masking) actual regression alerts:
# OLD
&& ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"While it's probably not physically possible for Bencher to emit both a "Head Version not found" error and a regression alert in the same run (it wouldn't reach the comparison stage if the hash is missing), restoring this guard would make the safety property explicit again. Suggest:
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"; then | |
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR" && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"; then |
| # We grep for both fragments so a format change surfaces clearly | ||
| # rather than silently skipping the retry. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"; then | ||
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//') |
There was a problem hiding this comment.
This sed approach works today, but if Bencher ever renames --start-point-hash the substitution silently no-ops, $RETRY_ARGS == $START_POINT_ARGS, and the retry is quietly skipped with no warning. The comment on line 652–653 already notes the concern about format changes surfacing clearly — worth applying the same logic here.
A small improvement: echo + sed can be replaced with bash parameter expansion which is both faster and avoids a subshell:
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//') | |
| RETRY_ARGS="${START_POINT_ARGS/--start-point-hash [^ ]*/}" |
[^ ]* isn't a POSIX extended regex here but rather a glob/pattern, so use ${var/pattern/replacement} — though note bash glob patterns don't support [^ ]. In practice the simpler approach is fine:
RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/ --start-point-hash [^ ]*//')The leading space before --start-point-hash avoids leaving a double-space artefact in the retry args, which is cosmetically cleaner (no functional impact since the shell collapses whitespace during word-splitting).
| START_POINT_HASH=$(gh api "repos/${{ github.repository }}/compare/main...$BRANCH" --jq '.merge_base_commit.sha' || true) | ||
| if [ -n "$START_POINT_HASH" ]; then | ||
| echo "Found merge-base via API: $START_POINT_HASH" | ||
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" |
There was a problem hiding this comment.
This is a silent behavior change from the old code: the dispatch case previously had EXTRA_ARGS="" (no --start-point-reset), so manually-dispatched runs on a branch that already existed in Bencher would append to its history rather than resetting it. The new code adds --start-point-reset here (and on line 580 for the no-hash path), so now Bencher will reset the branch to main's state on every manual dispatch.
This is probably the correct behavior (consistent with how PRs work), but a one-line comment would make the intentionality clear:
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" | |
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" # reset so repeated dispatches don't accumulate stale history |
alexeyr-ci2
force-pushed
the
alexeyr/2546/Fix-benchmark-workflow
branch
from
8c45b36 to
8a321e2
Compare
Code ReviewOverviewThis PR correctly fixes a cascading Bencher reporting failure on pushes to The refactor into Issues1. Behavior change: In the original code, 2. Two-grep "Head Version … not found" check is fragile grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"These are independent searches — they could match on completely different lines. If Bencher's output ever includes "Head Version" in a success message and "not found" anywhere else, the retry fires incorrectly. A combined pattern would be safer: grep -qE "Head Version.*not found" "$BENCHER_STDERR"3. Minor: RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//')This leaves What's Good
|
| # Bencher emits: 'Head Version (..., Some(GitHash("<sha>"))) not found' | ||
| # We grep for both fragments so a format change surfaces clearly | ||
| # rather than silently skipping the retry. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"; then |
There was a problem hiding this comment.
These two independent grep calls could match on different lines — e.g., "Head Version" in one output line and "not found" in a completely unrelated error message. Consider combining them into a single pattern:
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version" "$BENCHER_STDERR" && grep -q "not found" "$BENCHER_STDERR"; then | |
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -qE "Head Version.*not found" "$BENCHER_STDERR"; then |
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" | ||
| else | ||
| echo "⚠️ Could not find merge-base with main via GitHub API, continuing without hash" | ||
| START_POINT_ARGS="--start-point main --start-point-clone-thresholds --start-point-reset" |
There was a problem hiding this comment.
Potential behavior change from original code: The original workflow_dispatch path had EXTRA_ARGS="", so --start-point-reset was never passed for dispatch events. Both new sub-cases here now include --start-point-reset, which tells Bencher to reset the branch's accumulated history to match main on every run. For a feature branch benchmarked multiple times via dispatch to build up a trending baseline, this would wipe it each time. Was this intentional? If not, --start-point-reset should be removed from these two lines.
There was a problem hiding this comment.
Yes, it's intentional and not worth commenting every flag.
alexeyr-ci2
force-pushed
the
alexeyr/2546/Fix-benchmark-workflow
branch
2 times, most recently
from
9aa20e4 to
b6c4ada
Compare
| # Bencher emits: 'Head Version (..., Some(GitHash("<sha>"))) not found' | ||
| # We grep for both fragments so a format change surfaces clearly | ||
| # rather than silently skipping the retry. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version.*not found" "$BENCHER_STDERR" && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"; then |
There was a problem hiding this comment.
The comment says two separate greps make format changes "surface clearly", but the real risk is the opposite: two independent greps can match on different lines, creating a false-positive retry. A combined pattern is both safer and still surfaces format changes:
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version.*not found" "$BENCHER_STDERR" && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"; then | |
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -qE "Head Version.*not found" "$BENCHER_STDERR" && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"; then |
If Bencher's error format ever changes, the combined pattern still stops matching — you don't lose the diagnostic value, and you eliminate the false-positive risk.
There was a problem hiding this comment.
The current version already has a single grep to prevent matching on different lines.
| START_POINT_HASH=$(gh api "repos/${{ github.repository }}/compare/main...$BRANCH" --jq '.merge_base_commit.sha' || true) | ||
| if [ -n "$START_POINT_HASH" ]; then | ||
| echo "Found merge-base via API: $START_POINT_HASH" | ||
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" |
There was a problem hiding this comment.
Behavior change from old code: the previous dispatch case used EXTRA_ARGS="", so --start-point-reset was never passed for workflow_dispatch runs. This now resets the Bencher branch state to main on every dispatch run. For a branch benchmarked multiple times via dispatch to build up history, that history gets wiped each run.
Is this intentional? If so, worth a brief comment here explaining why dispatch should match PR behavior.
Code ReviewOverviewThe fix is correct and well-reasoned. Passing The Issues[Medium] The old code used [Low] RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//')When What's Good
|
| START_POINT_HASH=$(gh api "repos/${{ github.repository }}/compare/main...$BRANCH" --jq '.merge_base_commit.sha' || true) | ||
| if [ -n "$START_POINT_HASH" ]; then | ||
| echo "Found merge-base via API: $START_POINT_HASH" | ||
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" |
There was a problem hiding this comment.
Behavior change from old code: The old dispatch path had EXTRA_ARGS="" — --start-point-reset was never passed for dispatch events. Adding it here means every workflow_dispatch run on a non-main branch will reset that branch's Bencher history to main's latest state on each invocation.
If the intent is to always start fresh from main for dispatch runs (consistent with PR behavior), this is correct and intentional. But if someone uses workflow_dispatch to benchmark a long-running branch iteratively, this will wipe their accumulated data each time. Worth a comment either way — e.g.:
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" | |
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" |
(No code change needed if intentional — just add a comment confirming --start-point-reset is deliberate for dispatch.)
| # We grep for both fragments so a format change surfaces clearly | ||
| # rather than silently skipping the retry. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version.*not found" "$BENCHER_STDERR" && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"; then | ||
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//') |
There was a problem hiding this comment.
When --start-point-hash <sha> appears in the middle of $START_POINT_ARGS, the substitution leaves a double-space (e.g. --start-point main --start-point-clone-thresholds). Not a functional bug — shell word-splitting collapses it — but a trailing space in the pattern is cleaner:
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//') | |
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]* //') |
There was a problem hiding this comment.
Yes, it's intentional and already replied to.
|
View the full report in the job summary. |
## Summary - Applies `prettier --write` to five tracked docs files that are currently out of compliance on `main`. - No content changes — only whitespace/table-alignment fixes produced by Prettier. ## Why this is needed The formatting drift landed via a docs-only merge to `main`. The `lint-js-and-ruby.yml` workflow is conditionally skipped for docs-only commits on `main` (see the job condition at [`.github/workflows/lint-js-and-ruby.yml:79-87`](https://github.com/shakacode/react_on_rails/blob/main/.github/workflows/lint-js-and-ruby.yml#L79-L87)), so the check never ran on merge. It surfaces on every subsequent code-touching PR because the lint job runs for those (e.g. [#3151](#3151), [#3148](#3148), [#3142](#3142), [#3097](#3097)). Merging this unblocks the `build` → `Check formatting` step on those PRs. ## Test plan - [x] `pnpm start format.listDifferent` passes for the five files - [x] Diff is purely whitespace — no link targets, code blocks, or prose changed 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Improved formatting across API reference, core concepts, migration, and RSC docs: refined table layouts, adjusted column alignment, and standardized emphasis for clearer warnings. * Updated link text and phrasing for several guides for clearer navigation. * Reformatted code examples and JSX/Route snippets for more compact, consistent presentation and improved readability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| START_POINT="$GITHUB_BASE_REF" | ||
| START_POINT_HASH="${{ github.event.pull_request.base.sha }}" | ||
| EXTRA_ARGS="--start-point-reset" | ||
| START_POINT_ARGS="--start-point $GITHUB_BASE_REF --start-point-hash ${{ github.event.pull_request.base.sha }} --start-point-clone-thresholds --start-point-reset" |
There was a problem hiding this comment.
The START_POINT_ARGS string for PR events embeds $GITHUB_BASE_REF and ${{ github.event.pull_request.base.sha }} directly into a space-delimited arg string. This is fine for typical branch names, but GITHUB_BASE_REF values like feature/my branch (with a space) would corrupt the arg list when word-split inside run_bencher. The same applies to the workflow_dispatch path with $BRANCH.
In practice, GitHub branch names almost never contain spaces and the SHA is always hex, so this is a very low risk. Just worth documenting.
| START_POINT_ARGS="--start-point $GITHUB_BASE_REF --start-point-hash ${{ github.event.pull_request.base.sha }} --start-point-clone-thresholds --start-point-reset" | |
| START_POINT_ARGS="--start-point $GITHUB_BASE_REF --start-point-hash ${{ github.event.pull_request.base.sha }} --start-point-clone-thresholds --start-point-reset" |
(No code change needed unless you want to add a comment noting the branch-name assumption.)
| # We grep for both fragments so a format change surfaces clearly | ||
| # rather than silently skipping the retry. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version.*not found" "$BENCHER_STDERR" && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"; then | ||
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//') |
There was a problem hiding this comment.
The sed strip leaves a double-space when --start-point-hash sits between two other flags:
"--start-point main --start-point-hash abc123 --start-point-clone-thresholds"
^ sed leaves two spaces here
This is harmless because the unquoted $sp_args expansion inside run_bencher collapses consecutive whitespace via normal word-splitting. Just noting it so future readers don't think there's a bug.
A slightly cleaner alternative if you ever want it:
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//') | |
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*[[:space:]]*//') |
Code ReviewOverviewThis PR correctly diagnoses and fixes a cascading failure in the Bencher reporting workflow: for push-to-main events, the old code passed What's Good
Issues / Suggestions1. Retry is silently skipped if Bencher changes its error message format The retry guard uses: grep -q "Head Version.*not found" "$BENCHER_STDERR"The comment says "we grep for both fragments so a format change surfaces clearly," but the only observable effect of a format change is that the retry is skipped and the run fails — not a very clear signal. Consider adding a fallback log line when the exit code is non-zero but the error pattern doesn't match, so it's obvious why the retry didn't fire: elif [ $BENCHER_EXIT_CODE -ne 0 ]; then
echo "ℹ️ bencher failed but error pattern didn't match retry criteria — not retrying"
fi2. Double-space left by 3. Branch names with spaces in SummaryWell-reasoned fix that addresses the root cause rather than papering over it. The behavioral changes (retry logic, correct scoping of |
The benchmark workflow passed --start-point main --start-point-hash <github.event.before> for push-to-main events. Since main IS the base branch, Bencher tried to look up a version of main at the "before" hash — which often didn't exist (e.g., docs-only commits skipped by paths-ignore). This caused a 404, the report was never stored, and subsequent pushes also failed because their "before" hash was also missing. This cascading failure meant no main data was stored after the first version (Jan 18). Fix: don't pass --start-point args for pushes to main (thresholds are defined inline via --threshold-* args). For PRs/dispatch where the start-point hash may be missing, retry without --start-point-hash so the report still gets stored using the latest available baseline. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alexeyr-ci2
force-pushed
the
alexeyr/2546/Fix-benchmark-workflow
branch
from
b6c4ada to
f4cc4c8
Compare
Greptile SummaryThis PR fixes a cascading Bencher reporting failure on pushes to main by removing the erroneous Confidence Score: 5/5Safe to merge — the fix correctly eliminates the root cause of the cascading Bencher 404s and replaces error suppression with an active retry strategy. All changes are confined to the CI benchmark workflow. The root cause analysis is accurate, the retry guard (checking for no regression alerts before retrying) prevents masking real regressions, and word-splitting on $sp_args is intentional and well-documented. No P0/P1 findings. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Benchmark step starts] --> B{github.event_name?}
B -->|push| C[BRANCH=main\nSTART_POINT_ARGS=empty]
B -->|pull_request| D[BRANCH=HEAD_REF\nSTART_POINT_ARGS=--start-point BASE_REF --start-point-hash BASE_SHA\n--start-point-clone-thresholds --start-point-reset]
B -->|workflow_dispatch| E{BRANCH == main?}
E -->|yes| F[START_POINT_ARGS=empty]
E -->|no| G[Fetch merge-base SHA via GitHub API]
G -->|found| H[START_POINT_ARGS=--start-point main --start-point-hash MERGE_BASE\n--start-point-clone-thresholds --start-point-reset]
G -->|not found| I[START_POINT_ARGS=--start-point main\n--start-point-clone-thresholds --start-point-reset]
C & F --> J[run_bencher with empty start-point args]
D & H & I --> J
J --> K{bencher exit code?}
K -->|0| L[✅ Success — write HTML report]
K -->|non-zero| M{Head Version not found\nin stderr AND no alerts?}
M -->|no| N[❌ Fail workflow]
M -->|yes| O{--start-point-hash\npresent in args?}
O -->|no| N
O -->|yes| P[Strip --start-point-hash\nfrom args, retry]
P --> Q{Retry exit code?}
Q -->|0| L
Q -->|non-zero| N
Reviews (1): Last reviewed commit: "Fix Bencher reporting permanently broken..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Code Review: Fix Bencher reporting for pushes to main - The fix is correct and well-motivated. Root cause and fix are both clearly explained. See inline comments for specifics. |
| START_POINT_HASH=$(gh api "repos/${{ github.repository }}/compare/main...$BRANCH" --jq '.merge_base_commit.sha' || true) | ||
| if [ -n "$START_POINT_HASH" ]; then | ||
| echo "Found merge-base via API: $START_POINT_HASH" | ||
| START_POINT_ARGS="--start-point main --start-point-hash $START_POINT_HASH --start-point-clone-thresholds --start-point-reset" |
There was a problem hiding this comment.
Behavior change for workflow_dispatch: The old code used EXTRA_ARGS="" for workflow_dispatch — no --start-point-reset. The new code adds --start-point-reset here and on line 580. This aligns workflow_dispatch with PR behavior, which is likely correct, but it isn't mentioned in the PR description. Could you confirm this is intentional?
| # Bencher emits: 'Head Version (..., Some(GitHash("<sha>"))) not found' | ||
| # when the pinned hash isn't in its DB. A single combined pattern | ||
| # avoids false-positive matches across unrelated lines. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version.*not found" "$BENCHER_STDERR" && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"; then |
There was a problem hiding this comment.
Fragile error-message coupling: The retry is gated on "Head Version.*not found", which is Bencher's current internal error string. If Bencher changes this message in a future release, the retry will silently stop working and the old cascading-404 failure will return — without any obvious signal that it regressed.
Suggestion: add a comment here with a link to the Bencher CLI source or changelog entry that documents this error format, so it's easy to re-check after a Bencher version bump.
| # when the pinned hash isn't in its DB. A single combined pattern | ||
| # avoids false-positive matches across unrelated lines. | ||
| if [ $BENCHER_EXIT_CODE -ne 0 ] && grep -q "Head Version.*not found" "$BENCHER_STDERR" && ! grep -qiE "alert|threshold violation|boundary violation" "$BENCHER_STDERR"; then | ||
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//') |
There was a problem hiding this comment.
Minor: double space after removal. s/--start-point-hash [^ ]*/ removes the flag+value but leaves a double space in the result (e.g. "--start-point main --start-point-clone-thresholds ..."). Functionally harmless — word-splitting eats it and the RETRY_ARGS != START_POINT_ARGS guard still fires — but including the trailing space in the pattern keeps things tidy:
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]*//') | |
| RETRY_ARGS=$(echo "$START_POINT_ARGS" | sed 's/--start-point-hash [^ ]* //') |
| run_bencher() { | ||
| local sp_args="$1" | ||
| # Intentional word-splitting: sp_args contains multiple flags | ||
| # (e.g. "--start-point main --start-point-hash abc123") that must | ||
| # be split into separate argv entries. | ||
| # shellcheck disable=SC2086 | ||
| bencher run \ | ||
| --project react-on-rails-t8a9ncxo \ | ||
| --token '${{ secrets.BENCHER_API_TOKEN }}' \ | ||
| --branch "$BRANCH" \ | ||
| $sp_args \ | ||
| --testbed github-actions \ |
There was a problem hiding this comment.
Suggestion: use an array instead of intentional word-splitting. The shellcheck disable=SC2086 escape hatch works for the current inputs (branch names and SHA hashes never contain spaces), but it's fragile for future changes and suppresses a useful lint rule broadly.
read -ra builds a real array and handles the empty-string case (push-to-main where START_POINT_ARGS="") correctly — an empty input produces a zero-element array, so "${sp_args[@]}" expands to nothing:
| run_bencher() { | |
| local sp_args="$1" | |
| # Intentional word-splitting: sp_args contains multiple flags | |
| # (e.g. "--start-point main --start-point-hash abc123") that must | |
| # be split into separate argv entries. | |
| # shellcheck disable=SC2086 | |
| bencher run \ | |
| --project react-on-rails-t8a9ncxo \ | |
| --token '${{ secrets.BENCHER_API_TOKEN }}' \ | |
| --branch "$BRANCH" \ | |
| $sp_args \ | |
| --testbed github-actions \ | |
| run_bencher() { | |
| local -a sp_args | |
| read -ra sp_args <<< "$1" | |
| bencher run \ | |
| --project react-on-rails-t8a9ncxo \ | |
| --token '${{ secrets.BENCHER_API_TOKEN }}' \ | |
| --branch "$BRANCH" \ | |
| "${sp_args[@]}" \ |
2 participants
Summary
The benchmark workflow passed
--start-point main --start-point-hash <github.event.before>for push-to-main events. Since main IS the base branch, Bencher tried to look up a version of main at the "before" hash — which often didn't exist (e.g., docs-only commits skipped by paths-ignore). This caused a 404, the report was never stored, and subsequent pushes also failed because their "before" hash was also missing. This cascading failure meant no main data was stored between the first version (Jan 18) and the renaming ofmastertomain(Mar 21), and then many more runs were missing.Fix: don't pass
--start-pointargs for pushes to main (thresholds are defined inline via--threshold-*args). For PRs/dispatch where the start-point hash may be missing, retry without--start-point-hashso the report still gets stored using the latest available baseline.Pull Request checklist
- [ ] Add/update test to cover these changes- [ ] Update documentation- [ ] Update CHANGELOG fileOther Information
Rebuilt historical data on Bencher starting from a month ago (earlier data is not available; in principle it could be done by rerunning benchmarks on old commits, but it would take a long time and require re-deleting the current data).
Summary by CodeRabbit