Skip to content

fix(groom): stop build_pr full-cloning the target repo to file a bail issue #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattmillerai merged 1 commit into from
Aug 4, 2026
+64 −2
Merged

fix(groom): stop build_pr full-cloning the target repo to file a bail issue #129

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 64 additions & 2 deletions .github/workflows/groom.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2237,7 +2237,13 @@ jobs:
# never uploaded its result simply fails its own build_pr cell (fail-fast off).
if: ${{ !cancelled() && needs.gate.outputs.should_run == 'true' && needs.build_select.outputs.have_build == 'true' }}
runs-on: ubuntu-latest
timeout-minutes: 15
# 15 -> 20. Headroom only, and deliberately the LAST line of defence rather
# than the fix: the 2026-08-04 cancellation was a hung fetch that produced
# zero bytes, so a bigger budget alone would just have bought a longer hang.
# The bounded two-attempt checkout below is what actually converts that stall
# into a recovery. This also stops build_pr from being tighter than `build`
# (30 min), which does strictly more work over the same checkout.
timeout-minutes: 20
permissions:
contents: read
strategy:
Expand Down Expand Up @@ -2282,12 +2288,68 @@ jobs:
name: groom-build-${{ matrix.idx }}
path: /tmp/build

- name: Decide whether a working tree is needed
# The bail path (agent bailed / oversized patch / CI-privileged path /
# withheld output) only calls `gh issue create` — it never touches a git
# worktree. Reading the status HERE lets the checkout below be skipped
# entirely on that path, instead of full-cloning a large monorepo to file
# an issue. That is not a hypothetical saving: on 2026-08-04 both build
# cells bailed, and the idx-1 cell still spent its whole 15-minute budget
# in `git fetch` and was cancelled before it could file, dropping a
# CONFIRMED finding that the ledger then never suppressed.
id: plan
run: |
set -euo pipefail
# Default CLOSED: an unreadable/absent result.json means the apply step
# 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)

@github-actions github-actions Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 [ "$status" = "patched" ]; then
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."

@github-actions github-actions Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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).

fi
echo "status=$status" >> "$GITHUB_OUTPUT"

@github-actions github-actions Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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).


# Two bounded attempts, NOT one unbounded one. A server-side pack
# negotiation can stall with zero bytes on the wire: on 2026-08-04 this
# fetch sat silent for 15 minutes and was killed by the job timeout, while
# the sibling cell ran the identical fetch 70 seconds earlier in 50s.
#
# A step `timeout-minutes` alone does NOT buy a retry — exceeding it fails
# the step (and the job); checkout's own 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 is `continue-on-error` and attempt 2 re-runs it on a
# fresh connection. Two 6-minute attempts fit inside the 20-minute job
# budget with room for the push and the PR open.
- name: Checkout target repo (as the bot, for the push)
# persist-credentials stays ON (default) with the bot token so the branch
# push authenticates as the bot — this is the credentialed job by design.
id: checkout_target
if: steps.plan.outputs.needs_worktree == 'true'
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
continue-on-error: true

@github-actions github-actions Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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).

timeout-minutes: 6

@github-actions github-actions Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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).

with:
# Shallow: everything downstream needs only the default-branch tip —
# `checkout -b`, `git apply --index`, then `git push origin <branch>`
# (pushing a new branch from a shallow clone is fine, the base commit is
# already on the remote). Full history was never read, and on a repo of
# this size `fetch-depth: 0` turns a bounded cost into a variable one.
fetch-depth: 1
path: repo
token: ${{ steps.bot_token.outputs.token }}

- name: Checkout target repo (second attempt after a stalled fetch)

@github-actions github-actions Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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).

# 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' }}

@github-actions github-actions Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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).

uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
timeout-minutes: 6
with:
fetch-depth: 0
fetch-depth: 1
path: repo
token: ${{ steps.bot_token.outputs.token }}

Expand Down
Loading