Skip to content

fix(ci): unbreak bullock verdict script quoting - #2488

Merged
ethicnology merged 2 commits into
developfrom
fix/bullock-verdict-script-quoting
Jul 21, 2026
Merged

fix(ci): unbreak bullock verdict script quoting#2488
ethicnology merged 2 commits into
developfrom
fix/bullock-verdict-script-quoting

Conversation

@ethicnology

Copy link
Copy Markdown
Member

Two commits, independently revertable:

  1. fix(ci): unbreak bullock verdict script quoting — the actual bug that made every @bullock summon fail red.
  2. ci(bullock): harden feedback and CI-file guardrails — three hardenings found while auditing the workflow to root-cause the bug.

An apostrophe inside ${question:-word} was treated as a bash quoting
char even under double quotes, leaving the last single-quote in the
88-line script unclosed. bash -n failed with 'unexpected EOF' on every
Bullock run, so the whole 'Act on Bullock's verdict' step died at
parse time before any instruction ran — including the clarifying
comment this exact line was meant to post, so summons that should
have gotten a question instead got a silent red run.

Reword to avoid the apostrophe; no behavior change once it parses.
Three independent hardenings found during an audit of the workflow:

- Step-level (not just job-level) timeout on the Claude step. The
  job's 60min timeout CANCELS the job, and the verdict-acting step is
  guarded by !cancelled(), so a job timeout was a silent red run with
  no comment. A step timeout FAILS the step instead, which degrades
  cleanly into the existing no-valid-verdict comment path.

- Widen the CI-file backstop from .github/workflows/ to all of
  .github/. GITHUB_TOKEN can push .github/actions/ composite actions
  even though it can't push workflows, and those actions execute in
  future CI runs with secrets present. Bullock has no legitimate
  reason to touch any CI file, so block it mechanically instead of
  relying only on the prompt's SECURITY section.

- Skip the job when the summoning comment is from a Bot. GITHUB_TOKEN
  comments never retrigger workflows (GitHub anti-recursion), but
  GitHub-App comments do — a claude.yml review quoting "@bullock"
  would otherwise spin a runner and die red at the access gate.
@ethicnology ethicnology self-assigned this Jul 21, 2026
@ethicnology
ethicnology merged commit e7702a9 into develop Jul 21, 2026
3 checks passed
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 5m 19s —— View job


Review of PR #2488 — bullock verdict script quoting fix + hardenings

  • Gather context (PR diff / workflow file)
  • Review .github/workflows/bullock.yml for correctness, security, regressions
  • Post review summary

Note on scope: the runner has a depth-1 checkout of the merge commit and network fetches are sandboxed, so I couldn't diff against the pre-merge state; I reviewed the full merged workflow, concentrating on the areas this PR touched (verdict script, feedback paths, .github/ guardrails). I also can't post formal inline review comments (GitHub App limitation), so findings are here with line references.

Verdict

The headline fix is correct, and the hardenings are sound. The quoting bug is real bash behavior: inside "${var:-word}", bash treats a single quote in word as a quoting character even within double quotes, so an apostrophe in the fallback text left an unterminated quote and the whole script died with exit 2 at EOF. The rewritten line at bullock.yml:337 avoids apostrophes in the fallback and the NB comment (334–336) documents the trap for future editors. 👍

The .github/ guardrail is mechanically solid: git status --porcelain -- .github (line 358) catches both tracked edits and untracked additions before staging, and git add -A -- ':(exclude).github' (line 367) is consistent with it (both pathspecs match at the directory boundary). The rm -rf .bullock before staging correctly un-stages a sentinel Claude might have git add-ed, since the later git add -A re-syncs the index with the working tree.

Findings

1. [Low/Med — correctness] Fail-closed gap: a valid-but-non-object verdict crashes the script instead of posting the feedback commentbullock.yml:316

The validity check is jq empty, which accepts any valid JSON — a bare string, number, boolean, or array. If Claude writes e.g. "true" or [{"sufficient": true}], the check passes, and then jq -r '.sufficient' at line 323 errors ("Cannot index string/array…"). Under set -euo pipefail the command substitution failure kills the script — a red run with no comment posted, which is exactly the silent-failure mode this step's fail-closed design (and this PR) is meant to eliminate. Suggested fix — require an object in the same guard:

if [ ! -f .bullock/verdict.json ] || ! jq -e 'type == "object"' .bullock/verdict.json >/dev/null 2>&1; then

Fix this →

2. [Low — robustness] Late failures are still silent-redbullock.yml:379, bullock.yml:403

git push and gh pr create run under set -e with no fallback comment. If the push is rejected (e.g. a future ruleset/branch protection on bullock/*) or PR creation fails, the summoner gets a red run with no feedback — the same UX this PR fixed for the verdict path. A || { comment "🐂 Bullock failed to push/open the PR — see the run log."; exit 1; } on each would close the loop.

3. [Nit] Multi-line question escapes the markdown blockquotebullock.yml:337

Only the first line of ${question} gets the > prefix; subsequent lines render outside the quote. Cosmetic — question=$(jq -r ... | sed 's/^/> /')-style prefixing would fix it, or force it single-line like summary.

4. [Nit] cut -c1-72 is byte-orientedbullock.yml:326

GNU cut -c counts bytes, so a summary with a multibyte character spanning the 72-byte boundary is truncated mid-sequence, producing invalid UTF-8 in the commit subject/PR title. Unlikely given the conventional-commit prompt, but cut -c → awk/parameter-expansion truncation would be safer.

Security assessment (no blockers)

  • Comment body reaches shell only via env: (INSTRUCTION, line 92) — no inline ${{ }} interpolation into run: blocks with attacker-influenced content; usernames/repo names interpolated inline are safe charsets.
  • The slug pipeline (lines 174–180) strips everything outside [a-z0-9] before xargs, so quote-bearing instructions can't break xargs or produce unsafe branch names.
  • The write-access gate fails closed (404 from the permission endpoint fails the step), and comments are posted with GITHUB_TOKEN, which can't retrigger workflows — no self-summoning loop.
  • Blocking all of .github/ (not just workflows/) at commit time is the right call: the composite action and CI config execute with secrets in future runs, and the mechanical guard doesn't depend on the prompt being obeyed.

Only finding 1 is worth a follow-up commit; the rest are polish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant