ci: post sticky PR comment with fallow audit findings

[jrusso1020]

Stacked on #949. Merge that first.

What

Adds a sticky PR comment from the Fallow audit job so reviewers see findings inline instead of digging through CI logs.

How

The Fallow audit job (added in #942) now runs in four steps:

1. Run audit + capture markdown — bunx fallow audit ... --fail-on-issues --format pr-comment-github > /tmp/fallow-comment.md with set +e. Captures the markdown output and the audit exit code. Includes a non-empty guard so transient fallow crashes don't post a blank sticky.
2. Post sticky comment (findings) — marocchino/sticky-pull-request-comment@v2.9.1, only when exit_code != '0'. Uses fallow's built-in <!-- fallow-id: fallow-results --> sentinel as the header so each subsequent run updates the same comment.
3. Remove stale sticky comment (clean run) — same action with delete: true, only when exit_code == '0'. Clears any previous "findings" sticky after a follow-up commit fixes the issue.
4. Fail if audit found issues — re-emits the captured exit code so the job still gates the PR on new findings.

The sticky-comment steps both have continue-on-error: true so fork PRs (which run with a read-only GITHUB_TOKEN regardless of workflow permissions:) don't fail the whole job on a comment they can't post; the audit gate still fires via step 4.

Permissions

pull-requests: write is scoped to the fallow job only — moved out of the workflow-level permissions: block (per Vance's review). Build, lint, test, smoke jobs etc. keep the workflow's read-only default. Job-level permissions override the workflow block, so this scope-down is one extra permissions: stanza, no other changes needed.

What the comment looks like

When there are findings: a collapsible markdown table listing severity, rule, file:line, and description — same content fallow normally writes to stdout.

When there are no findings: no comment is posted, and any stale "findings" sticky from a previous run is deleted.

Test plan

• YAML parses
• Local: bunx fallow audit --fail-on-issues --format pr-comment-github emits markdown to stdout AND exits 1 when findings exist
• Job-level permissions: block correctly overrides workflow-level (verified against GitHub Actions docs)
• Verify in CI: this PR adds workflow lines only — should fall through the "clean run" branch and delete any prior fallow comment (creating none, since none exists yet)
• Verify on a follow-up PR that intentionally introduces a finding: comment should appear

Follow-ups

• If sticky comments prove too coarse, can swap to per-line inline review comments (--format review-github) — bigger UX shift

[jrusso1020]

• ci: post sticky PR comment with fallow audit findings #954 👈 (View in Graphite)
• refactor: delete orphan declarations flagged by fallow #949
• ci: run fallow audit in lefthook pre-commit #948
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[vanceingalls]

Workflow change is small, surgical, and well-commented. Three steps (run + capture, post sticky, re-fail) is the right decomposition and the sentinel-matching with fallow's <!-- fallow-id: fallow-results --> is the correct sticky pattern.

Audited: .github/workflows/ci.yml (entire file, end-to-end at 3d59ebb7); verified marocchino/sticky-pull-request-comment@52423e01 resolves to tag v2.9.1 and oven-sh/setup-bun@0c5077e5 resolves to v2; verified fallow ^2.75.0 is in root package.json so bunx fallow resolves from node_modules (PR-body claim holds).

Strengths

• The "Remove stale sticky comment (clean run)" step (ci.yml:135-140) is the right call — without it, a PR that goes red then green would leave the findings comment around forever and reviewers would think the PR is still broken. Nice catch.
• set +e + capturing $? to GITHUB_OUTPUT (ci.yml:121-126) is the cleanest way to keep the audit gating while still letting the sticky-post step run. Cleaner than relying on continue-on-error at the step level.
• The top-of-file comment on the pull-requests: write widening (ci.yml:5-9) is exactly the kind of forward-looking note that pays off later. It names the future split point (fork PRs → separate workflow) before someone else has to figure it out.

Findings

• important — ci.yml:9 (pull-requests: write at the workflow level). This grants every job in ci.yml PR-write tokens — Build, Lint, Format, Typecheck, Test, both smoke jobs, CLI smoke, filesize, semantic-pr-title. Only fallow needs it. The blast-radius concern is: any of those jobs runs bun install against user-controlled package.json (lifecycle scripts) or bun run build against user-controlled source, all under a write-scoped token. The in-diff comment acknowledges this and defers; my push is that the right shape is available now — move permissions: to the job level inside fallow: only and revert the top-level to pull-requests: read. That keeps the audit-comment poster working without expanding any other job's authority. Job-level permissions: override the workflow-level block. One-line fix, no other tradeoffs.
• nit — ci.yml:127-133 posts /tmp/fallow-comment.md unconditionally on non-zero exit, but if bunx fallow crashes before producing meaningful markdown, the file may be empty or partial. Cheap guard: if [ ! -s /tmp/fallow-comment.md ]; then echo "fallow produced no output" > /tmp/fallow-comment.md; fi after the audit. Avoids posting a blank sticky on infra failure.
• nit — ci.yml:124-125 would benefit from set -o pipefail in case fallow's output is ever piped (today it isn't; this is paranoia against future edits to the line).
• nit — fork PRs: with the pull_request trigger (not _target), GitHub gives forks a read-only token regardless of the workflow permissions: block, so the sticky-post step will fail for fork contributors. Not a security issue (correct direction of failure), but worth a one-line graceful degradation — continue-on-error: true on the two sticky-comment steps so a fork PR doesn't fail CI on a comment it can't post. The audit gate still fires via "Fail if audit found issues."

Notes

• Test-plan checkbox 3 ("Verify in CI: this PR should post a 'No findings' comment on itself") — given the "Remove stale sticky comment (clean run)" branch, a no-findings PR shouldn't post a comment; it should delete a (nonexistent) one. The verification on this PR is "no fallow comment appears." Worth nudging the description for clarity.

Verdict: APPROVE
Reasoning: The behavior change is correct and the design is sound. The workflow-level pull-requests: write is an important call-out the author already flagged for follow-up; scoping it to the fallow job in this PR would be one line and worth doing before merge, but it isn't a blocker given there's no fork trigger here.

Review by Vai

[miguel-heygen]

APPROVE — well-structured CI workflow.

Verified:

• Exit code capture pattern (set +e → $? → GITHUB_OUTPUT) is correct
• marocchino/sticky-pull-request-comment@52423e01640425a022ef5fd42c6fb5f633a02728 SHA matches v2.9.1
• Sticky comment lifecycle is right: post on findings, delete on clean run — prevents stale "0 findings" comments
• --format pr-comment-github outputs to file, read via path: — avoids ${{ }} injection vectors
• No security concern: fallow output is from static analysis, not untrusted PR metadata

Non-blocking note: pull-requests: write is workflow-wide. The inline comment documents why this is acceptable. If fork PRs ever become a concern, moving fallow to its own workflow would scope the permission.

[jrusso1020]

Thanks @vanceingalls @miguel-heygen — addressed all four points:

Fixed

• 🔧 Important — pull-requests: write scope: moved from workflow-level to job-level on fallow: only. The rest of `ci.yml` (Build, Lint, Format, Typecheck, Test, smoke jobs, etc.) keeps the workflow-level pull-requests: read default. Job-level permissions override workflow-level, so this is the clean one-stanza scope-down you suggested.
• 🔧 Nit — empty markdown guard: added if [ ! -s /tmp/fallow-comment.md ]; then echo "fallow produced no output — see the job logs above." > /tmp/fallow-comment.md; fi after the audit. No more blank stickies on transient crashes.
• 🔧 Nit — fork PR graceful degradation: continue-on-error: true on both sticky-comment steps. Fork PRs (read-only GITHUB_TOKEN regardless of permissions: block) won't fail the whole job on a comment they can't post; the audit gate at the end still fires.
• 📝 Note — test plan wording: PR body updated. Clean-run behavior is now correctly described as "no comment posted; any stale finding-sticky from a previous run is deleted."

Skipped

• set -o pipefail — fallow's output isn't piped today; paranoia for a future change isn't worth the line.

Force-pushed @ 7c1098da.

The base branch was changed.

[miguel-heygen]

Re-reviewed — APPROVE.

All prior feedback addressed in the force-push:

• pull-requests: write scoped to job level (not workflow-wide) — addresses the least-privilege concern
• Empty markdown guard added (if [ ! -s /tmp/fallow-comment.md ]) — handles fallow crash edge case
• continue-on-error: true on both sticky-comment steps — graceful degradation for fork PRs
• PR description updated to match actual behavior

CI all green including fallow audit, full regression suite, typecheck, lint, build.

[vanceingalls]

Re-review on a67436b4. All three prior findings ADDRESSED — including the important one. Diff is clean: top-level permissions: pull-requests: read, job-level pull-requests: write scoped to fallow: only.

Findings status

• important (workflow-level pull-requests: write) — ADDRESSED. .github/workflows/ci.yml:4-6 is now permissions: contents: read / pull-requests: read. ci.yml:88-90 adds job-level permissions: contents: read / pull-requests: write on fallow: only, with a comment that names exactly the right reason ("the rest of ci.yml keeps the workflow-level pull-requests: read default so build / lint / test tokens can't post or modify PR comments"). Build, Lint, Format, Typecheck, Test, smoke jobs, filesize, semantic-pr-title now run with read-only PR scope. Blast-radius pinched as recommended.
• nit (empty/partial sticky markdown guard) — ADDRESSED. ci.yml:113-115: if [ ! -s /tmp/fallow-comment.md ]; then echo "fallow produced no output — see the job logs above." > /tmp/fallow-comment.md; fi. Good fallback message too — points reviewers at the log rather than dropping a blank comment.
• nit (fork-PR fail-soft on sticky-comment steps) — ADDRESSED. ci.yml:121 and ci.yml:130 both have continue-on-error: true on the two sticky-comment uses-blocks. Fork PRs won't fail CI on a comment they can't post; the audit gate (Fail if audit found issues step) still runs and gates the build.

New code worth a callout

• ci.yml:128-134 — "Remove stale sticky comment (clean run)" using delete: true is the right pattern; a PR that goes red→green now sheds its findings comment rather than carrying a stale one indefinitely. (Pre-existing, kept from the prior review SHA; still strong.)

CI status (verbatim, head a67436b4)

All required checks SUCCESS: Build, Lint, Format, Typecheck, Test, Test: runtime contract, CLI smoke (required), Smoke: global install, Fallow audit, File size check, regression-shards (8 shards), preview-regression, player-perf, regression, Semantic PR title, Detect changes, Preflight (lint + format), Graphite / mergeability_check. No FAILURE conclusions. mergeStateStatus: BLOCKED is review-required (this approval clears it), not CI.

baseRefName: main — no stacked-PR CI gap.

Verdict: APPROVE
Reasoning: Important finding addressed exactly as recommended (job-level scoping); both nits cleanly handled. CI fully green on main base. Ship it.

Review by Vai (re-review)