fix(groom): stop build_pr full-cloning the target repo to file a bail issue - #129
Conversation
… issue A build_pr matrix cell on a large downstream caller spent its entire 15-minute budget inside `git fetch` and was cancelled, dropping a CONFIRMED finding that never reached the dedup ledger — so the next sweep pays to re-derive it. Three things were wrong, in increasing order of how much they cost: 1. build_pr checked out the target repo UNCONDITIONALLY, before reading result.json. That cell had already bailed (patch over the size cap), so its only remaining work was `gh issue create` — the bail branch `sys.exit(0)`s before touching git. It full-cloned a multi-GB monorepo to write an issue. Now gated on status == "patched", defaulting closed: an unreadable result.json means the apply step takes the bail branch anyway, so skipping the worktree cannot strand a patch. 2. `fetch-depth: 0` fetched all history, branches and tags when nothing downstream reads history — `checkout -b`, `git apply --index`, `git push origin <branch>`. Shallow now; the base commit is already on the remote, so pushing a new branch from a shallow clone is fine. 3. Nothing bounded the fetch. A step `timeout-minutes` alone would not have helped: exceeding it fails the step, and checkout's internal retry only covers git commands that exit non-zero inside a live step, never one the runner killed. So attempt 1 is `continue-on-error` + bounded, and a second attempt re-runs it on a fresh connection. The stall is not "the repo is slow" — the sibling cell ran the identical fetch 70 seconds earlier in 50s, and the failing one emitted zero bytes for 15 minutes. It is a server-side pack-negotiation hang, so the job timeout (15 -> 20) is headroom only, not the fix.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
|
Merging unreviewed. Blast radius: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
| echo "needs_worktree=false" >> "$GITHUB_OUTPUT" | ||
| echo "::notice::build ${{ matrix.idx }} status=$status — no patch to apply, skipping the target-repo checkout." | ||
| fi | ||
| echo "status=$status" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🟡 Medium — status is read from /tmp/build/result.json, an artifact produced by the credential-free builder agent, and written to $GITHUB_OUTPUT with no sanitization; a value containing a newline appends extra key=value lines and, since duplicate keys are last-wins, could override the needs_worktree=false written two lines above and force the credentialed checkout (a value with no = fails the step outright). The same unsanitized value is interpolated into the ::notice:: workflow command on line 2312, where a newline breaks out of the annotation and can forge further workflow commands in the public log. Today the producer only ever writes the literals patched/bail, so validating against that two-value allowlist before echoing makes the guarantee local instead of dependent on a distant job. Raised by 6 of 8 reviewers (gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, kimi-k3-max adversarial, kimi-k3-max edge-case, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| path: repo | ||
| token: ${{ steps.bot_token.outputs.token }} | ||
|
|
||
| - name: Checkout target repo (second attempt after a stalled fetch) |
There was a problem hiding this comment.
🟡 Medium — The retry checks out into the same path: repo that attempt 1 may have left half-populated after the runner SIGKILLed it mid-fetch (unborn HEAD, temp packfiles, stale .git/*.lock). actions/checkout will first try git remote set-url / git clean -ffdx / git reset --hard against that tree and only re-clones if that fallback triggers, so the retry can fail deterministically — or burn its 6-minute budget on cleanup — in exactly the stalled-fetch scenario it was added for. An explicit rm -rf repo step between the attempts (or a distinct path for attempt 2) makes the recovery independent of that behavior. Raised by 5 of 8 reviewers (gemini-3.1-pro adversarial, kimi-k3-max adversarial, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-max edge-case).
| # will take the bail branch (`result.get("status") != "patched"`), so the | ||
| # worktree is genuinely not needed. Skipping it cannot strand a patch — | ||
| # if the status is unreadable there is no patch to apply. | ||
| status=$(jq -r '.status // "bail"' /tmp/build/result.json 2>/dev/null || echo bail) |
There was a problem hiding this comment.
🟡 Medium — The "Default CLOSED … cannot strand a patch" justification does not hold: the apply step does an unguarded open() + json.load() on the same /tmp/build/result.json, so an absent or truncated artifact raises FileNotFoundError/JSONDecodeError before result.get("status") != "patched" is ever evaluated. The step dies without running gh issue create or writing a ledger marker — the dropped-CONFIRMED-finding outcome this change exists to prevent. Wrap the apply-step load in a try/except that falls through to the bail branch, and consider dropping 2>/dev/null here so a corrupt artifact is distinguishable from a genuine agent bail. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case).
| if: steps.plan.outputs.needs_worktree == 'true' | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| continue-on-error: true | ||
| timeout-minutes: 6 |
There was a problem hiding this comment.
🟢 Low — timeout-minutes: 6 is a hard ceiling, not a stall detector — it cannot tell a fetch producing zero bytes from one that is slow but progressing. A caller repo whose checkout legitimately takes longer than 6 minutes previously succeeded inside the job budget and now fails both attempts, turning a slow success into a guaranteed cell failure with no bail issue and no ledger marker. The fetch-depth: 0 → 1 change makes this much less likely, but giving attempt 2 a larger budget than attempt 1 would remove the cliff entirely. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).
| - name: Checkout target repo (second attempt after a stalled fetch) | ||
| # No continue-on-error: if a fresh connection stalls too, fail the cell | ||
| # loudly rather than falling through to an apply step with no worktree. | ||
| if: ${{ steps.plan.outputs.needs_worktree == 'true' && steps.checkout_target.outcome == 'failure' }} |
There was a problem hiding this comment.
🟢 Low — Failing loudly when both attempts stall still drops the finding: the job dies before Apply patch -> open PR, so no gh issue create runs and no ledger marker is written, and the CONFIRMED finding is re-proposed on every subsequent run. The bail path is now hardened against that outcome while the patched path is not; an if: failure() step that files the bail issue would close the gap. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
| id: checkout_target | ||
| if: steps.plan.outputs.needs_worktree == 'true' | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| continue-on-error: true |
There was a problem hiding this comment.
🟢 Low — actions/checkout declares post-if: success(), so a step killed by timeout-minutes never runs its credential-cleanup post step — the bot token written by attempt 1 as http.https://github.com/.extraheader stays in repo/.git/config, and continue-on-error: true is what keeps the job running with it present. Impact is bounded by the ephemeral runner and the short-lived app token, but pairing the wipe suggested for the retry with an explicit rm -rf repo would remove the stale credential too. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
| echo "needs_worktree=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "needs_worktree=false" >> "$GITHUB_OUTPUT" | ||
| echo "::notice::build ${{ matrix.idx }} status=$status — no patch to apply, skipping the target-repo checkout." |
There was a problem hiding this comment.
⚪ Nit — ${{ matrix.idx }} is interpolated straight into the shell body of a run: block, which is the template-injection pattern zizmor flags and which the rest of this job deliberately avoids (the apply step passes the same value via env: IDX:). The value is workflow-generated so it is not exploitable; passing it through env: just keeps the convention consistent. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
2 participants
ELI-5
When groom's robot writes a code change, a second job pushes it and opens the PR. That job always downloaded the entire history of the target repo first — even when there was no code change to push and its only job was to write a GitHub issue. On a huge repo that download can randomly hang. It did: the download sat there for 15 minutes producing nothing, the job's timer ran out, and the finding it was supposed to record was silently lost. Now it only downloads when there's actually a patch to apply, downloads far less, and retries once if the download stalls.
What happened
A
build_prmatrix cell on a large downstream caller was cancelled at its 15-minute job timeout, sitting in this fetch the whole time with zero output:Not one
* [new branch]line, so it hung in server-side pack negotiation before any ref landed. The sibling cell ran the identical fetch 70 seconds earlier and finished it in 50s — the 15-minute budget was already ~13x the observed need. This is a stall, not slowness, which is why a bigger budget is not the fix.Cost: that cell had already decided to file a bail issue, so the finding never got filed, never entered the dedup ledger, and the next sweep re-derives it from scratch and pays the finder + verifier for it again.
Changes
planstep readsresult.jsonand the target-repo checkout is nowif: needs_worktree == 'true'. The bail branch (result.get("status") != "patched") callsfile_issue()andsys.exit(0)s before any git call, so it never needed a worktree — it was full-cloning a multi-GB monorepo in order to rungh issue create. Defaults closed: an unreadable or absentresult.jsonyieldsbail, which is exactly the path that needs no worktree, so skipping it cannot strand a patch.fetch-depth: 0→1. Nothing downstream reads history — the apply step doescheckout -b,git apply --index,git push origin <branch>. Pushing a new branch from a shallow clone is fine because the base commit is already on the remote.timeout-minuteson its own would not have recovered this: exceeding it fails the step and the job, and checkout's internal retry only covers git commands that exit non-zero inside a step that is still alive — never a step the runner killed. So attempt 1 iscontinue-on-error: true+timeout-minutes: 6, and a second attempt re-runs it on a fresh connection. The second has nocontinue-on-error, so a genuine two-strike failure is loud rather than falling through to an apply step with no worktree.timeout-minutes: 15 → 20.** Headroom only, explicitly commented as the last line of defence rather than the fix. It also stopsbuild_prbeing tighter thanbuild` (30 min), which does strictly more work over the same checkout.Verification
python3 -c "yaml.safe_load(...)"parses;build_prstep order confirmed programmatically:plan→ checkout(if, continue-on-error, t=6) → retry checkout(if, t=6) → labels → apply.actionlintreports 9 findings, all pre-existing onmainand all on lines this PR does not touch (github.job_workflow_sha/job.workflow_shaare real contexts its schema lacks). Zero new findings.if result.get("status") != "patched": file_issue(...); sys.exit(0)— no git invocation upstream of that exit, and the_groom_assetscheckout thatfile_issueimports the ledger from is deliberately not gated.