feat(skill): add /fixer skill - #2158
Conversation
Solve the N lowest-numbered open issues end-to-end, one at a time, from open issue to merged PR, then drain any parked PRs with /sweep and /resolve. Prevents the compounding merge conflicts seen in previous batch runs via six invariants: one branch per issue always cut from a freshly fetched origin/main, merge before cutting the next branch so each branch is born containing all prior fixes, at most one PR open at a time, never rebase, park rather than stall, and a mechanical integrity check that verifies every line a PR authored survives any catch-up merge. The branch-per-issue rule also avoids stacked PRs, which this repo cannot use safely because deleteBranchOnMerge closes dependents instead of retargeting.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| mkdir -p .codegraph/fixer | ||
| REPO=$(cat .codegraph/fixer/repo) | ||
| # PR is the parked PR being updated; set it from the current drain iteration |
There was a problem hiding this comment.
When an issue is abandoned or its PR is parked, this block still records STATUS="merged", causing the final report to misrepresent the result and preventing parked PRs from being added to the drain queue.
There was a problem hiding this comment.
Fixed — step 2b now writes abandoned to .codegraph/fixer/outcome when an issue is closed as already-fixed/invalid/duplicate, 2f writes parked at the exact point the 3-round convergence cap is hit, and the merge step writes merged only after gh pr merge succeeds. 2g now reads STATUS from that outcome file (erroring if it's unexpectedly unset) instead of hardcoding STATUS="merged".
| echo "G2 PASS: no unanswered reviewer comments" | ||
| else | ||
| echo "G2 FAIL: $UNANSWERED unanswered Greptile comment(s)"; GATE_FAIL=1 | ||
| fi | ||
|
|
||
| # --- G3: branch contains origin/main (required by strict:true) --- | ||
| git fetch origin main || { echo "ERROR: git fetch failed"; exit 1; } | ||
| if git merge-base --is-ancestor origin/main HEAD; then | ||
| echo "G3 PASS: up to date with main" | ||
| else |
There was a problem hiding this comment.
When a PR has unresolved Claude, human, or review-body feedback but no unanswered top-level Greptile inline comment, G2 passes and permits an admin merge with that feedback still unresolved.
There was a problem hiding this comment.
Fixed — G2 now checks inline comments from any reviewer (not just Greptile), and additionally requires a post-review issue-comment reply for any top-level review body (Claude's CHANGES_REQUESTED/COMMENT summary, Greptile's summary, or a human's), since those have no path/line and were previously invisible to the gate entirely.
| fi | ||
| echo "fixer: $MERGED/$TOTAL issues merged (${PCT}%)" | ||
| jq -r '.issues[] | " #\(.issue) \(.status) \(if .pr then "PR #\(.pr)" else "no PR" end)"' .codegraph/fixer/state.json | ||
| ``` | ||
|
|
||
| Output a summary table: | ||
|
|
||
| | Issue | PR | Branch | Status | CI | Greptile | Notes | | ||
| |-------|----|--------|--------|----|----------|-------| |
There was a problem hiding this comment.
When a catch-up merge drops an authored line while identical or containing text remains in another changed file, the deduplicated global substring check still passes, allowing the branch to merge after losing part of the PR's behavior.
There was a problem hiding this comment.
Fixed — the integrity check now records each authored line together with the specific file it came from (tab-separated) and verifies survival within that same file, rather than against one combined haystack of every changed file. A line dropped from file A can no longer be masked by identical/containing text surviving in file B.
| mkdir -p .codegraph/fixer | ||
| # Merge main into the parked branch. A non-zero exit means conflicts, which is a | ||
| # signal to run /resolve rather than a failure of this skill. | ||
| if git merge origin/main -m "merge: bring branch up to date with main"; then |
There was a problem hiding this comment.
When execution stops after state.json records an outcome but before the separate queue rewrite completes, the next run still selects that completed issue from queue.json[0]; the documented status filter is not implemented in the bash blocks, so the issue is processed again and can produce a duplicate branch or PR.
There was a problem hiding this comment.
Fixed — added a resume-safety loop at the top of Phase 2 that drops any queue head already recorded with a terminal status (merged/parked/abandoned) in state.json before the loop ever reads it, closing the gap where a crash between the state.json write and the queue.json shift in 2g could cause the next run to reprocess a completed issue.
Greptile SummaryAdds the
Confidence Score: 3/5The PR is not yet safe to merge because unresolved review-body feedback can still be treated as acknowledged before an administrative merge. G2 associates review bodies with self-authored issue comments using only timestamp windows; for the latest review, any later non-ping comment satisfies the gate even when it addresses unrelated work. Files Needing Attention: .claude/skills/fixer/SKILL.md Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Queue[Build issue queue] --> Resume[Reconcile persisted state]
Resume --> Branch[Cut branch from origin/main]
Branch --> Solve[Implement and verify issue]
Solve --> PR[Open pull request]
PR --> Gate{All merge gates pass?}
Gate -->|Yes| Merge[Merge and record outcome]
Gate -->|No, rounds remain| Solve
Gate -->|No, cap reached| Park[Record parked PR]
Merge --> Next{Queue empty?}
Park --> Next
Next -->|No| Branch
Next -->|Yes| Drain[Drain parked PRs]
Drain --> Report[Final report]
Reviews (5): Last reviewed commit: "fix: reconstruct missing parked.txt entr..." | Re-trigger Greptile |
If a run stopped between recording an outcome in state.json and shifting queue.json in 2g, the queue head could still point at an issue already marked merged/parked/abandoned, causing a resumed run to reprocess it and potentially open a duplicate branch or PR.
G2 only checked whether Greptile's own inline comments had replies, so a PR with unresolved Claude, human, or top-level review-body feedback could still pass the gate and be merged with --admin while that feedback sat unaddressed. G2 now covers inline comments from any reviewer and requires a post-review issue-comment reply for non-empty review bodies.
The check built one combined haystack across every changed file, so a line dropped from file A could still pass if identical or containing text happened to survive in file B — masking a real loss of authored behavior. Each authored line is now recorded and verified against the specific file it came from.
…2158) Step 2g always wrote STATUS="merged" regardless of what happened to the issue, so a parked or abandoned PR was still recorded as merged — misreporting the final table and never entering the parked.txt drain queue. Outcome is now written at the point each path is actually decided (abandoned in 2b, parked once the convergence-round cap is hit in 2f, merged right after `gh pr merge` succeeds) and 2g reads it instead of assuming.
| '[.[] | select(.created_at > $since) | select(.user.login != $reviewer)] | length') | ||
| [ "$REPLY_AFTER" -eq 0 ] && UNANSWERED=$((UNANSWERED + 1)) |
There was a problem hiding this comment.
When any bot, contributor, or the fixer posts an issue-level comment after a review, the temporal REPLY_AFTER test treats it as an acknowledgment of every earlier review body, allowing an administrative merge with unresolved human, Claude, or Greptile feedback.
There was a problem hiding this comment.
Fixed — G2's REPLY_AFTER check now only counts a comment as answering a top-level review body if it was posted by our own account (gh api user); any other bot/contributor/reviewer commenting afterward no longer satisfies it, and a bare @greptileai/@claude re-trigger ping is explicitly excluded since it isn't a substantive acknowledgment.
| fi | ||
| echo "fixer: $MERGED/$TOTAL issues merged (${PCT}%)" | ||
| jq -r '.issues[] | " #\(.issue) \(.status) \(if .pr then "PR #\(.pr)" else "no PR" end)"' .codegraph/fixer/state.json | ||
| ``` |
There was a problem hiding this comment.
When a catch-up merge removes an authored line but leaves a longer line containing the same text in that file, grep -qF still succeeds, causing I4 to report success and permit the branch to proceed after authored behavior was dropped.
There was a problem hiding this comment.
Fixed — the I4 check now uses whole-line matching (grep -qxF, with leading whitespace stripped from the file's lines to match how the authored line was normalised when recorded) instead of substring matching. A removed authored line can no longer be masked by a longer line in the same file that merely contains its text.
| echo "ERROR: HEAD does not contain origin/main — invariant I1/I2 violated, stop and investigate" | ||
| exit 1 | ||
| fi | ||
| ``` | ||
|
|
||
| ### 2b. Understand the issue | ||
|
|
||
| Read the issue in full, including its comments and any linked issues or PRs: | ||
|
|
||
| ```bash | ||
| mkdir -p .codegraph/fixer | ||
| REPO=$(cat .codegraph/fixer/repo) | ||
| ISSUE=$(jq -r '.[0].issue' .codegraph/fixer/queue.json) | ||
| gh issue view "$ISSUE" --repo "$REPO" --comments | ||
| ``` |
There was a problem hiding this comment.
When execution stops after writing parked but before recording the terminal state, resume processes the same queue head without clearing outcome or round; the stale state then skips a mergeable replacement PR as already parked or exhausts its convergence budget prematurely.
There was a problem hiding this comment.
Fixed — step 2a now clears .codegraph/fixer/outcome, round, current-pr, and gate-fail before cutting a fresh branch. This closes the crash window where a park decision (or its round count) was written but the run stopped before 2g recorded it in state.json — the next attempt on that issue now starts with clean scratch state instead of inheriting the stale verdict/count.
- reset stale outcome/round/current-pr/gate-fail scratch state before cutting a fresh branch, so a crash after parking (but before 2g records it) can't leak a stale verdict or round count into a replacement PR for the same issue - G2 review gate: only our own substantive reply counts as answering a top-level review body; any other bot/contributor comment, or a bare @greptileai/@claude re-trigger ping, no longer satisfies the check - I4 integrity check: use whole-line match instead of substring match, so a removed authored line can no longer be masked by a longer line in the same file that merely contains its text
| else | ||
| echo "ERROR: I4 FAIL — $MISSING authored line(s) vanished in the merge." |
There was a problem hiding this comment.
When a PR adds the same normalized line in multiple locations of one file and a catch-up merge drops one occurrence, sort -u records only one baseline entry and the existence-only grep -qxF check passes on the surviving occurrence, allowing the branch to proceed after authored behavior was silently removed.
There was a problem hiding this comment.
Fixed — the I4 baseline now records each (file, line) pair's occurrence count via sort | uniq -c instead of sort -u, and verification checks that count survives (grep -xcF compared against the recorded count) rather than just checking existence. A dropped copy of a duplicated authored line is now caught even when another copy of that same line survives in the same file.
A single ME-authored reply posted after every review previously satisfied all of them by account+timestamp alone, even reviews it never addressed. Each review body now requires its own reply strictly before the next review's submission (or before now, for the latest).
…2158) sort -u collapsed a line the PR added twice in one file to a single baseline entry, so a catch-up merge that dropped one copy still passed the existence-only check on the surviving copy. The baseline now records each (file, line) pair's occurrence count via sort | uniq -c, and verification requires that count to survive, not just presence.
|
Addressed the summary's second gap ("the review gate can still accept unrelated author comments as acknowledgments" / "associates comments with review bodies only by account and timestamp"): G2 now gives each review body its own acknowledgment window — [that review's submitted_at, the next review's submitted_at) — so a single reply posted after every review can no longer retroactively satisfy an earlier review it never addressed. Each review now needs its own reply landing in its own window. |
| git fetch origin main || { echo "ERROR: git fetch failed"; exit 1; } | ||
|
|
||
| # Capture every line this PR authored, relative to its own merge base, BEFORE touching | ||
| # main. Each line is recorded together with the file it came from and how many times it | ||
| # was added in that file (tab-separated: file, count, line) — checking a line's exact |
There was a problem hiding this comment.
When execution stops after state.json records a parked outcome but before the PR is appended to parked.txt, resume drops the terminal queue entry without reconstructing the drain list, causing that parked PR to remain open and never enter Phase 3.
There was a problem hiding this comment.
Fixed — the resume-dedup loop at the top of Phase 2 now reads the terminal issue's status directly, and when it's parked, reconstructs the missing parked.txt entry from state.json's own pr field (idempotently — skips if already present) before dropping the stale queue head. A parked PR whose state.json record landed but whose parked.txt append didn't can no longer be silently excluded from Phase: Drain Parked PRs.
… head (#2158) 2g writes state.json's terminal record and parked.txt's append separately. A stop between them left a parked PR's state.json entry in place but its parked.txt entry missing, so the resume-dedup loop dropped that queue head as resolved without ever queuing the PR for Phase: Drain Parked PRs — it stayed open, forgotten, forever. The dedup loop now reconstructs the missing parked.txt entry from state.json's own pr field before dropping a parked queue head.
|
Re: the summary's note that G2 "still infers acknowledgment from commenter identity and timing rather than a reply tied to the feedback" — this PR already tightened G2 twice (own-account-only, per-review time windows so one late reply can't retroactively satisfy an earlier review). Going further to verify a reply's content actually addresses that specific review would need an LLM to judge relevance between the review body and the candidate reply — GitHub gives no stronger structural signal for top-level review bodies (no reply-threading API, unlike inline comments). That's a semantic-verification sub-step, not a bash-gate tweak, so it's a design change beyond this PR's mandate. Tracked as follow-up: #2160. |
|
This round's finding restates the same G2 temporal-correlation limitation from the previous round rather than naming a new gap — it's already tracked as #2160 with the reasoning for why closing it fully requires semantic verification (an LLM judging reply-to-review relevance) rather than a further bash/jq heuristic. G2 has now been tightened twice in this PR (own-account-only, then per-review time windows), which is the practical ceiling for a mechanical check given GitHub's API has no reply-threading mechanism for top-level review bodies. Not re-triggering again on an unchanged concern — see #2160 for the follow-up. |
1 participant
Summary
/fixer, a skill that takes the N lowest-numbered open issues (default 10) from open issue to merged PR, one at a timeorigin/main; the PR is merged before the next branch is cut, so branches never go stale and never stack/sweepand/resolveagainst parked PRs until each is merged or reported for human reviewWhy
A previous 10-issue batch run collapsed under compounding merge conflicts: issues were solved on a shared branch, so each PR's diff grew to contain all prior fixes, and because the
Mainruleset requires branches to be up to date withmain(strict: true), each merge left every other open branch stale — paying for the same conflict resolution repeatedly./fixeris built around six invariants (documented in the skill) that make that failure mode structurally impossible rather than just less likely.Verification
lint-skill.shandsmoke-test-skill.sh: 0 errors, 0 warnings, 22/22 bash blocks passbash -n--start-from <N>was mistaken for the issue count)gh issue listdataTest plan
/fixer --dry-runto confirm the queue and plan look right with no side effects/fixer 2for a small real run before trusting it for a full batch of 10