feat(autofix): replace inline reviewdog with /autofix ChatOps button#1458
Conversation
… Apply" The sticky summary comment was stating "Posted formatting suggestions inline. Click Apply suggestion on each" even when reviewdog landed zero inline review comments — typical case: the formatter touched lines outside the PR's added range, so `-filter-mode=added` (correctly) filtered everything out. The script unconditionally set `posted=true` after running reviewdog regardless of whether any comments were actually created, leaving the user staring at a sticky that promised buttons that didn't exist. The publish job now snapshots the count of `github-actions[bot]` review comments before and after reviewdog. If the delta is zero, surface a new `diff-no-overlap` UI state that tells the user plainly: "Formatter found fixable issues, but they're on lines outside this PR's added range — there's nothing to click here. Run locally: npm run lint:fix && npm run format." Plus a matching `gitnexus/autofix` Check Run conclusion (still neutral, distinct title) so agents reading `gh pr checks` see the same signal. Three states are now machine-distinguishable in the sticky's gitnexus-autofix JSON block: suggestions-posted (delta > 0), diff-no-overlap (delta == 0), skipped-too-large (>3k lines).
Pivot the PR autofix UX from per-line reviewdog suggestions to a single
slash-command button. Contributors comment `/autofix` on the PR; a new
trusted workflow downloads the existing autofix patch artifact, applies
it to the PR head, and pushes a commit back.
Why:
- 3K+ diffs hit GitHub's review-comment API 406 limit -> dead end.
- Diffs where the formatter touches lines outside the PR's added range
("no-overlap") get filtered by reviewdog's -filter-mode=added -> dead
end (PR #1457 patched the lying sticky but the underlying UX gap
remained).
- Per-line click-Apply-suggestion is high-friction for big diffs and
easy to apply unevenly.
- A single `git apply` + push works at any size and lands fixes
atomically.
Changes:
- pr-autofix-publish.yml: remove `Install reviewdog` and
`Post inline suggestions` steps. Collapse three sticky states
(suggestions-posted, diff-no-overlap, skipped-too-large) into one
(fixes-available). Bump JSON schema v1 -> v2 with `apply_command`
field; all v1 fields preserved.
- pr-autofix-apply.yml (new): triggers on issue_comment with body
`/autofix`, validates body via strict regex, validates commenter
has write/admin/maintain or is the PR author, locates latest
successful pr-autofix run for PR head SHA, downloads artifact,
applies patch, pushes commit. Reacts +1/-1/eyes on triggering
comment per outcome. Idempotent (`git apply --check --reverse`
detects already-applied state).
- CONTRIBUTING.md: document v2 schema and the /autofix flow,
including the maintainer-edit requirement for fork PR pushes.
Trust posture: apply workflow runs from default-branch code only,
under issue_comment trigger. Comment body and author login flow
through env vars and pattern-matched, never interpolated into shell.
Permission gate (write/admin/maintain OR PR author) before any
artifact fetch. Fork PRs require "Allow edits by maintainers"
(GitHub-native; we don't bypass).
Net YAML: -139 lines in publish.yml, +260 in apply.yml. Removes
reviewdog binary pin and the entire review-comment API surface.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Two findings from the Codex adversarial review of the autofix ChatOps pivot. Both are localized YAML changes that close trust gaps the pivot inherited from the original PR #1446 design. U1 — Cross-verify metadata against workflow_run authority (.github/workflows/pr-autofix-publish.yml): Previously the trusted publisher accepted pr_number, head_sha, and head_repo from metadata.json after only an allowlist regex. A fork-controlled `npm run lint:fix` could have written a syntactically valid metadata.json referencing another PR/SHA, redirecting the write-scoped sticky/check-run onto an attacker-chosen target. New `Verify metadata against workflow_run authority` step compares artifact-claimed identity against: - github.event.workflow_run.head_sha - github.event.workflow_run.head_repository.full_name - workflow_run.pull_requests[].number (within-repo PRs) - gh api commits/{sha}/pulls fallback (fork PRs, where pull_requests[] is empty) Fail closed on mismatch — no sticky, no check-run, no override. U2 — Lease-protected push in apply workflow (.github/workflows/pr-autofix-apply.yml): Previously the apply step pushed `HEAD:${HEAD_REF}` plain. A force- push between resolve (Step 5) and push (Step 9) would silently fast-forward an older commit graph over the contributor's newer state. Push now uses `--force-with-lease=refs/heads/${HEAD_REF}:${HEAD_SHA}` against the SHA resolved earlier. Distinct `lease-failed` result code + retry-message reply, separated from `push-failed` (fork without maintainer-edit) so contributors can diagnose the actual cause. Plan: docs/plans/2026-05-09-005-fix-autofix-codex-adversarial-findings-plan.md (local-only per repo convention). Trust posture preserved: no new permissions, no new workflows, no contract change. JSON v2 schema unchanged. CodeQL js/server-side- request-forgery and template-injection posture unchanged — all new inputs flow via env vars and pattern-matched.
…eckout actions/checkout's default behavior writes the GITHUB_TOKEN into .git/config as an extraheader. The token then sits on disk in the checkout directory — an actions/upload-artifact step on that directory would leak it. We don't upload, but zizmor's credential-persistence lint correctly flags the latent risk. Set persist-credentials: false on the Checkout PR head step. Provide push auth inline via `git -c http.extraheader="Authorization: Basic <base64-of-x-access-token:TOKEN>"` so the credential never lands on disk and never appears in process listings (the URL form https://x-access-token:TOKEN@… is rejected here because it leaks via ps and git remote -v). Push lease semantics from U2 unchanged — same --force-with-lease against the resolved HEAD_SHA, same lease-failed/push-failed/stale result codes.
ce-code-review surfaced 15 findings on PR #1458; this commit applies the 7 with concrete fixes (#1, #2, #3, #4, #5, #9, #13). Five P2 findings (#6, #7, #8, #10, #12) are recorded as residual actionable work for follow-up; two advisory items (#11, #14) skipped. #1 — applied_run_id schema drift (CONTRIBUTING.md): v2 docs claimed `state: applied` enum value and an `applied_run_id` field that no code path emits. Trimmed docs to match what the workflow actually writes (state: fixes-available; v1 field set as superset). Implementing the apply-side sticky upsert that would populate `applied_run_id` is deferred — cleaner than carrying a contract claim with no code. #2 — result= unset between idempotency probe and lease push (pr-autofix-apply.yml): After `git apply --check` passed, an early non-zero exit from `git config` / `git apply` / `git add` / `git commit` left `result=` unset, sending the user to the `*` "unexpected state (`unknown`)" arm. Wrapped the apply/commit phase in a single if-test that sets `result=apply-failed` on any failure. New React-and-reply branch surfaces an actionable message. #3 — permission lookup conflated transient API failures with denial (pr-autofix-apply.yml): `gh api … 2>/dev/null || echo "none"` swallowed 5xx, 429 secondary rate-limit, and network failures, surfacing them as a public 👎 refusal to legitimate maintainers. Now distinguishes 404 (genuine non-collaborator) from other API failures via stderr match. New `allowed=api-failed` state triggers a 😕 reaction with a "transient API failure, retry" reply instead of a misleading refusal. #4 — lease-failure grep missed git's "remote rejected" / branch- deleted phrasings (pr-autofix-apply.yml): Real lease failures got classified as `push-failed` → user told to enable maintainer-edit, which won't help. Expanded regex to match `remote rejected` and `! [rejected]`. #5 — broken bullet continuation in CONTRIBUTING.md release-candidate section: rejoined the split bullet so it renders correctly. #9 — base64 GITHUB_TOKEN bypassed GitHub's secret-masker (pr-autofix-apply.yml): Added `::add-mask::${auth_header}` immediately after construction so any subsequent log line (set -x, GIT_TRACE) gets *** redacted. #13 — misleading schema-bump comment in pr-autofix-publish.yml: Comment claimed all v1 fields preserved exactly, but the `state` enum was redefined v1→v2. Updated to make the migration path explicit (v1 readers see unfamiliar schema, fall back to prose). Residual actionable work (deferred to follow-up): #6 locate step gh api retry; #7 artifact-expired graceful fallback; #8 re-entrancy comment-spam guard; #10 producer-still-running UX; #12 gh_retry wrapper for apply.yml. Validations: yaml.safe_load OK, check-workflow-concurrency.py OK.
#8, #10, #12) Pulls the deferred items from the previous review pass into this PR so the workflow ships with full reliability + UX coverage rather than follow-up debt. #6 + #12 — gh_retry wrapper on idempotent GETs in apply.yml: Permission lookup, PR metadata fetch, and workflow-run lookup are now wrapped in the same gh_retry helper publish.yml uses (3 attempts, linear backoff). Reaction/comment POSTs remain unwrapped (retrying POST would dupe the resource). #10 — producer-still-running UX: The locate step now distinguishes three cases via `found_status` output: success (proceed), in-progress / queued / pending / waiting (reply ⏳ "wait for autofix run to finish"), not-found (reply 🤔 "push a commit"), api-failed (reply⚠️ "transient API failure"). The "no successful autofix run" message no longer fires immediately after a fresh push while the producer is still mid-run. #7 — artifact-expired graceful fallback: actions/download-artifact gains `continue-on-error: true`. The apply step distinguishes patch-file-missing (artifact expired, 1-day retention elapsed) from patch-file-zero-bytes (formatter found nothing). New `result=artifact-expired` case + ⏳ "push a new commit to regenerate" reply. #8 — re-entrancy loop guard: After checkout but before applying, check if HEAD itself is a github-actions[bot] `chore(autofix)` commit. If so, refuse to re-apply (`result=loop-prevented`) with a 🔁 reply telling the user to push a human-authored commit or revert before retrying. Prevents formatter-config-drift loops where an automated agent watching the sticky could pump arbitrary apply commits. Net effect: every code path in apply.yml now sets a meaningful `result=` that maps to a specific user-facing reaction + reply. The `*` "unexpected state (unknown)" arm becomes truly unreachable in normal operation. Validations: yaml.safe_load OK, check-workflow-concurrency.py OK.
|
Claude finished @magyargergo's task in 9m 5s —— View job PR #1458 Security Review — feat(autofix): replace inline reviewdog with /autofix ChatOps buttonTasks
Review bar for this PRAgainst GUARDRAILS.md (least-privilege, no secrets, security-sensitive config requires human sign-off) and CONTRIBUTING.md CI/security standards, this PR must clear:
Current PR state
Branch hygiene assessmentVerdict: merge-from-main commit present but harmless and merge-safe. The merge commit Understanding of the changeWhy reviewdog was removed: Reviewdog's Why Why Why applying and pushing is more sensitive than suggestions: The old flow could only post cosmetic suggestions; rejecting them was free. This flow writes commits directly to PR branches with Why artifact identity validation is mandatory: The artifact's Why Why fork PRs need "Allow edits by maintainers": GitHub's GITHUB_TOKEN push authorization for cross-repo refs requires the PR author to have granted maintainer-edit access. The workflow cannot bypass this; it fails loud with a clear reply when the push is rejected. Findings[Low] Stale reviewdog header comments in changed and related workflow files
[Low] No patch-path constraint on
|
| Workflow | Permissions | Justified by |
|---|---|---|
pr-autofix.yml (producer) |
{} (none) |
Fork code execution; no API writes needed |
pr-autofix-publish.yml |
pull-requests: write, actions: read, checks: write |
PR comment upsert, artifact download, check-run creation |
pr-autofix-apply.yml |
pull-requests: write, contents: write, actions: read |
PR comment + reaction, push commit, artifact download |
pr-autofix-apply.yml does not have checks: write (correct — apply creates no check runs). No custom PAT, no id-token, no OIDC, no secrets.* beyond GITHUB_TOKEN. GITHUB_TOKEN is not echoed; set -x is absent from all run: blocks.
Injection / security tooling assessment
Clean on latest head. Every untrusted value flows through env vars:
github.event.comment.body→$BODY(not interpolated into shell source)github.event.comment.user.login→$COMMENTER(allowlisted before URL use)github.event.issue.number→$PR(numeric, only used ingh apiURL)- All
metadata.jsonfields → allowlist-validated before$GITHUB_OUTPUT; cross-verified againstworkflow_runauthority HEAD_REF,HEAD_REPO,HEAD_SHA→ validated regex; used via env vars not${{ }}interpolation in shell
No eval, no unsafe heredocs, no unquoted variable expansions, no gh api paths constructed from untrusted strings without validation. JSON built with jq --arg/--argjson, not string concatenation.
Tooling status: Workflow Lint (which runs actionlint + zizmor) ✅. CodeQL ✅. Gitleaks ✅. Dependency Review ✅. The GHAS credential-persistence finding is resolved (persist-credentials: false at line 344; zizmor passes).
Race / re-entrancy assessment
Well-handled. Concurrency key ${{ github.workflow }}-${{ github.event.pull_request.number || github.event.issue.number }} serializes apply runs per PR; cancel-in-progress: false queues a second /autofix rather than cancelling the first (correct — the second invocation hits the already-applied idempotency guard).
Producer-still-running: distinguished via in-flight run status check with in_progress/queued/pending/waiting. Instructs user to wait rather than regenerate. Artifact-expired: clearly differentiated from empty patch. The --force-with-lease catches the apply run that raced against a force-push. Re-entrant loop is bounded: HEAD must be a human commit for the loop guard to clear.
The one remaining theoretical loop path — an AI agent watching stickies and auto-commenting /autofix on each new fixes-available — is mitigated by the loop guard and by the sticky including head_sha + run_id so a well-implemented agent can detect staleness. There is no bot auto-comment /autofix, so no self-triggering.
Reviewdog removal assessment
Complete. The Install reviewdog and Post inline suggestions steps are absent from pr-autofix-publish.yml. reviewdog/action-setup is not referenced. No REVIEWDOG_GITHUB_API_TOKEN env var. The 3K review-comment API cap is no longer relevant. suggestions-posted, diff-no-overlap, and skipped-too-large are absent from all runtime code paths. CONTRIBUTING.md describes only /autofix. PR #1457's no-overlap state is superseded.
The stale references at pr-autofix-publish.yml lines 7, 17, 52 are comments only (header documentation and a permission annotation), not executable paths. They do not cause old behavior to run.
Validation / CI assessment
All relevant static checks pass on latest head. issue_comment workflows cannot be exercised in PR checks (GitHub only runs default-branch copies of issue_comment workflows post-merge), so end-to-end validation is necessarily post-merge manual. The PR body documents a complete manual matrix (11 scenarios) covering: small/large/no-overlap diffs, repeated /autofix, unauthorized user, no-run, in-progress, expired artifact, fork with/without maintainer edits, stale head. Rollback is a standard git revert of the apply commit (small blast radius); the workflow itself can be disabled in the GitHub Actions settings page if needed. This is adequate for a privileged ChatOps workflow where the scope is narrow (push commits to PR branches only when an authorized user explicitly requests it).
Hidden Unicode / YAML hygiene assessment
No bidirectional controls (U+202A–U+202E, U+2066–U+2069) found in any changed file. Non-ASCII characters present are emoji (✅, 👀, 👎, ⚠️, 🚫, 🔁, ⏳, ❓) appearing exclusively in YAML step name strings and gh api -f body="..." user-facing reply messages. None appear in shell code, GitHub expressions (${{ }}), regex patterns, permission blocks, JSON schema markers, or security comments. The CONTRIBUTING.md non-ASCII is emoji in the PR-type table header (🚀, 🐛, 🏎️), which is pre-existing and benign.
Elegance / maintainability assessment
The state machine is clean and exhaustive: every result= value is explicitly handled in the final case with a distinct reaction + reply. The * fallback correctly catches unexpected values rather than silently succeeding. The gh_retry helper is consistently repeated (unavoidable in YAML run: blocks which have no shared scope) with a clear comment explaining the repetition. The auth-via-inline-header approach (-c http.extraheader) is slightly more complex than a token URL but is the correct pattern to avoid token leakage in process listings and remote configs; the accompanying ::add-mask:: call is a good defensive extra step. Comment density is high and targeted at the trust-boundary decisions, not at obvious mechanics. Future maintainers have enough context to understand why each security control is present.
Final verdict
Production-ready with minor follow-ups.
The core trust-boundary architecture is correctly implemented: the untrusted producer never receives write credentials, the trusted publisher treats artifact metadata as hostile data and cross-verifies against workflow_run authority, the trusted apply workflow runs only default-branch code, and every fork-controlled string is allowlist-validated before use in git or GitHub API operations. The two key mechanisms — --force-with-lease with exact SHA and persist-credentials: false with inline auth — are both in place and correctly structured. All static security checks (actionlint, zizmor, CodeQL, Gitleaks, Dependency Review) pass on the latest head.
The two findings above are both [Low] severity with no merge-blocking impact: (1) stale reviewdog documentation in workflow headers is misleading for future maintainers but has zero runtime effect; (2) the absence of patch-path constraints on .github/workflows/ is inherent to the ChatOps apply pattern, not a regression, and requires social engineering of a write-access user to exploit. Both can be addressed as post-merge follow-ups.
The branch hygiene is merge-safe: the merge-from-main commit is harmless (GitHub's diff shows exactly 3 autofix-related files) and PR #1457's supersession is intentional and clean.
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 8488 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
…ing .github/ Two follow-up findings on PR #1458: #1 — Stale reviewdog references in workflow header comments: pr-autofix-publish.yml's header still described the removed inline- suggestion path ("posts inline review-comment suggestions to the PR using `reviewdog`", "Reviewdog reporter: github-pr-review reads $REVIEWDOG_GITHUB_API_TOKEN…"). The Check Run permissions comment enumerated the old outcomes (clean / suggestions-posted / skipped-too-large) instead of the current set (clean / fixes- available). pr-autofix.yml's header described the trusted job as posting "inline review-comment suggestions" and the changed_lines comment referenced the dead 3000-line cap. Refreshed all three to describe the actual sticky + Check Run + /autofix flow. #2 — Reject patches touching .github/ (sensitive-paths guard): Theoretical supply-chain vector: a malicious PR could ship a custom prettier/ESLint config that reformats workflow YAML, dependabot.yml, or CODEOWNERS. The producer would capture those edits in autofix.patch; a maintainer running `/autofix` would push them under `contents: write` without human review. The default GITHUB_TOKEN lacks the `workflows` scope so workflow-file pushes would fail at the platform layer anyway, but as a generic `push-failed` (which misleads users into enabling maintainer-edit). Reject early with a specific reason. Match runs against the patch with grep on `^(diff --git|---|+++) [ab]?/?\.github/`. New `result=sensitive-paths` case + 🛑 reply telling the user to apply .github/ formatter changes manually. Documented the constraint in CONTRIBUTING.md under the /autofix section so contributors aren't surprised when the workflow refuses a patch that includes formatter changes to workflow files. Validations: yaml.safe_load OK, check-workflow-concurrency.py OK.
2 participants
Summary
Pivot the PR autofix UX from per-line reviewdog inline suggestions to a single ChatOps button. Contributors comment
/autofixon a PR; a new trusted workflow downloads the existing autofix patch artifact, applies it to the PR head, and pushes a commit back.Why
The current pipeline has three failure modes that all reduce to "the sticky tells the user to do something but the actionable thing isn't usable":
-filter-mode=added→ reviewdog posts nothing. PR fix(autofix): verify reviewdog actually posted before claiming "click Apply" #1457 fixed the lying sticky, but the UX dead-end remained.A single
git apply+ push works at any size and lands the fixes atomically. This is the standard ChatOps pattern (Dependabot's@dependabot rebase, Mergify's@mergify rebase).What changed
.github/workflows/pr-autofix-publish.yml: removedInstall reviewdogandPost inline suggestionssteps. Collapsed three sticky states (suggestions-posted,diff-no-overlap,skipped-too-large) into one (fixes-available). Schema bumped v1 → v2 with newapply_commandfield; all v1 fields preserved..github/workflows/pr-autofix-apply.yml(new): listens forissue_commentwith body matching^/autofix\s*$, validates commenter haswrite/admin/maintainOR is PR author, locates latest successfulpr-autofix.ymlrun for PR head SHA, downloads artifact, applies patch, pushes commit. Reacts 👀/✅/👎 on the triggering comment per outcome. Idempotent (git apply --check --reversedetects already-applied state).CONTRIBUTING.md: documented v2 schema and the/autofixflow, including the Allow edits by maintainers requirement for fork PRs.Trust posture
issue_commenttrigger.Test plan
Post-merge manual matrix on test PRs against
main:fixes-available+/autofixinstruction. Comment/autofix→ bot reacts 👀, then ✅, push lands aschore(autofix): ...commit.paths-ignorefor one snapshot temporarily) → noskipped-too-largestate;/autofixworks./autofix --foo(extra args) → silently ignored (no reaction).please don't /autofix this→ silently ignored./autofix→ 👎 + refusal reply.gh pr checks <pr> --json name,conclusion,output | jq '.[] | select(.name == "gitnexus/autofix")'returns the new neutral title.jq '.schema'readsgitnexus.pr-autofix/v2,.apply_command == "/autofix".Notes
no-overlapstate introduced in fix(autofix): verify reviewdog actually posted before claiming "click Apply" #1457 becomes dead code (the new state machine has nono-overlap). Consider closing fix(autofix): verify reviewdog actually posted before claiming "click Apply" #1457 or rebasing it on top of this.🤖 Generated with Claude Code
Supersedes
Closes #1457 — this PR includes that commit (
e1f0fe7e) as a base and pivots beyond it. Theno-overlapstate #1457 introduced becomes dead code with the inline reviewdog removal, but the commit history is preserved.Codex adversarial review (post-pivot follow-up)
Codex flagged two trust gaps in the pivot. Both addressed in commit
a4e1b220:pr-autofix-publish.ymlnow cross-verifies artifact identity (pr_number,head_sha,head_repo) againstgithub.event.workflow_runauthority before any sticky/check-run side effect. Fork-PR fallback usesgh api commits/{sha}/pulls.pr-autofix-apply.ymlnow pushes with--force-with-lease=refs/heads/${HEAD_REF}:${HEAD_SHA}against the resolved SHA. Distinctlease-failedresult code + retry-message reply (separate frompush-failedfor fork-maintainer-edit so contributors can diagnose the actual cause).Residual review findings - all resolved in commit a7dcd4f
All five P2 items from ce-code-review (#6 gh_retry on locate, #7 artifact-expired fallback, #8 re-entrancy loop guard, #10 producer-still-running UX, #12 gh_retry wrapper port) landed in this PR. Every code path in pr-autofix-apply.yml now sets a meaningful
result=that maps to a specific user-facing reaction + reply.