ci(town-crier): producer workflow (announce + resolve on close)#4
Conversation
This comment has been minimized.
This comment has been minimized.
Goosterhof
left a comment
There was a problem hiding this comment.
Verdict: concerns (no blockers) — 0 blockers / 2 concerns / 0 nits. CI green (check 8.4/8.5 pass; announce self-test passed, resolve skipped as expected).
Actions-security headline: clean. No injection vector — PR-author-controlled title/requester reach the shell through env: and are referenced as quoted --arg values inside jq, never interpolated into a run: string. Trigger is plain pull_request, not pull_request_target. Token is ${{ secrets.TOWN_CRIER_TOKEN }}, not hardcoded. No third-party actions to SHA-pin. This is the right shape.
Two non-blocking concerns and one confirmation of @jasperboerhof's own unlabeled note, all inline.
Findings
- Concern — no
permissions:block; the workflow inherits the broad repo default token for a job that only POSTs to an external bus (announcejob). - Concern —
curl -fsShard-fails the step on any bus non-2xx/outage, reddening the PR's checks; the bus is fail-open by design. - Confirm (MINOR) — @jasperboerhof's
unlabeled-gap finding holds.
Automated war-room agent review — posted because this PR carries the Agent Review Requested label.
Goosterhof
left a comment
There was a problem hiding this comment.
Round 2 — since bec01bc7: 2 addressed, 0 new, 0 still open. Verdict: approve-worthy (0 blockers / 0 concerns / 0 nits). CI green (check 8.4/8.5 pass).
Both standing concerns from the prior round are closed by this head:
permissions:block — addressed.permissions: {}at line 16 drops the token to zero scopes, which is correct: neither job touchesGITHUB_TOKEN(they auth to the bus withTOWN_CRIER_TOKEN). Stronger than thecontents: readI'd have settled for.- Fail-open — addressed.
continue-on-error: trueon both theannounceandresolvesteps, plus--max-time 10on both curls. A bus cold-start or 5xx now leaves the step red but the job green, so a town-crier hiccup no longer reds the PR's checks.curl -fsSstill exits non-zero on a bus error, butcontinue-on-erroris the right containment — the failure is visible in the run log without blocking merge.
Actions-security posture unchanged and clean: PR-author-controlled title/requester reach the shell only through env: and as quoted jq --arg values, never interpolated into a run: string; trigger is plain pull_request, not pull_request_target; token is ${{ secrets.TOWN_CRIER_TOKEN }}, not hardcoded; no third-party actions to SHA-pin.
One residual, unchanged and out of scope for an approve: the unlabeled-gap MINOR (@jasperboerhof's own note, which I confirmed last round) — types: [labeled, closed] means removing the label before close leaves the bus thread open. Author-acknowledged; not a blocker.
Automated war-room agent review — posted because this PR carries the Agent Review Requested label.
Goosterhof
left a comment
There was a problem hiding this comment.
Round 3 — since a788feb: config-error-loud/bus-open split — correct. Verdict: approve-worthy (0 blockers / 0 concerns / 0 nits). CI green (check 8.4/8.5 pass).
The single new commit replaces the prior round's blanket continue-on-error: true with an in-script split that distinguishes the two failure modes — which is exactly what closes the swallow-integration-zero trap I'd have worried about with a blanket tolerance:
- Config error fails LOUD. Missing
TOWN_CRIER_URL/TOWN_CRIER_TOKENhits::error::+exit 1(announce-pr.yml:43-44,:71-72), reddening the job so a misprovisioned repo in the rollout is discoverable. - Bus hiccup fails OPEN. With the secret present, a
curl -fsSnon-2xx/timeout falls to|| echo "::warning::"(:57,:79), which exits 0 — step stays green, so a Fly cold-start or 5xx never reds a contributor's checks.
Removing continue-on-error and gating loud-vs-open purely in-script is the right call: a blanket continue-on-error would have swallowed the config exit 1 too. Anchored inline at the new guard.
Actions-security posture unchanged and clean: PR-author-controlled title/requester reach the shell only via env: and as quoted jq --arg values, never interpolated into a run: string; trigger is plain pull_request, not pull_request_target; token is ${{ secrets.TOWN_CRIER_TOKEN }}; permissions: {} holds; no third-party actions to SHA-pin.
The unlabeled-gap MINOR (@jasperboerhof's own note, confirmed in prior rounds) remains the one residual — types: [labeled, closed] leaves a label-removed-before-close thread open on the bus. Author-acknowledged, out of scope for this approve.
Automated war-room agent review — posted because this PR carries the Agent Review Requested label.
Review Loop · 9/10 · PASSkendo-report-tool #4 · AC anchor: PR description: town-crier producer workflow (announce on label, resolve on close) · head Tip Reviewed the single new GitHub Actions workflow (announce-pr.yml) as the primary surface — injection vector, permissions, the two-mode failure policy, and the resolve-job close gate. The diff is clean: PR-author-controlled TITLE/REQUESTER reach the script only as quoted jq --arg values (no shell splice), permissions:{} correctly zeroes the unused GITHUB_TOKEN, and the config-error-loud / bus-hiccup-fail-open split is implemented exactly as documented. The one real correctness gap (a label-removed-before-close PR never fires /resolve, leaking an open bus thread) is already a colleague-confirmed MINOR thread on the PR, so it is not mine to re-derive or count. No findings — clean against the review checklist. Actionmerge-ready |
Adds the town-crier producer workflow — announce on the
Agent Review Requestedlabel, resolve on PR close (merged/closed).The bus's resolve-by-
pr_urlendpoint is deployed (script-development/town-crier#14) and this repo'sTOWN_CRIER_URLvar +TOWN_CRIER_TOKENsecret are already provisioned, so it's ready to merge. Part of the producer rollout — keeps merged PRs from sittingopenon the bus.🤖 Generated with Claude Code