Conversation
…tops the pipeline
--issues reads like a run mode ("file issues instead of doing the work") but is
specified as a sink selector ("record deferred findings in the tracker rather than
PLAN.md"). Users hit this by running /do:better --issues or /do:simplify --issues
and getting a full remediation pipeline with per-category PRs.
The combination that delivers the expected behaviour is --scan-only --issues, and
it was underspecified: the disposition rules file an issue for any finding the run
"defers", and a scan-only run remediates nothing, so every finding is deferred --
but that was left to inference. Two runs with identical flags could defensibly file
all findings or none.
Fixes both halves:
- --scan-only and its Phase 2 gate now state that under issue mode every surviving
finding is filed, and that the filed issues are the run's entire output.
- The --issues bullet now leads with what it controls and says outright that it
does not change what the run does, pointing at --scan-only.
- plan-issue-mode.md grows a section covering the scan-only case directly, since
that partial is where the "deferred" wording originates.
- review-config-defaults.md notes that a saved issues: true does not suppress
remediation, and why --issues must not be redefined to imply --scan-only: anyone
with that default set would silently stop getting fixes.
No behaviour change to --issues itself.
… a PR or merging Every reviewer loop pushes its own fix commits as its final step, but nothing downstream verified it happened. An orchestrator that improvised a loop body instead of following it left the fixes local: gh pr create opened a PR carrying only the pre-review commit, every reviewer still reported clean, CI still passed (it was testing the pushed tree, green because it predated the fixes), and the merge gate had nothing to object to. The failure was completely silent. Primary fix in the multi-reviewer wrapper, so it lands once for all seven commands that drive it: series dispatch gains a step 5 asserting the pass's commits reached the remote, pushing with the loops' own one-retry, and recording a new push-failed status when the push still fails. Parallel dispatch gets the same assertion after its single union apply. push-failed is inconclusive and, unlike the other inconclusive statuses, is never excused by ~opt: a non-blocking reviewer's findings are still real fixes, and merging a tree that isn't the reviewed tree is not what ~opt opts out of. Backstops in do:pr at the two moments unpushed work becomes visible damage: Open the PR pushes first if the branch is ahead, and the merge gate refuses to merge while it is, reporting the unpushed SHAs. Both checks compare against the upstream ref specifically (git status says nothing about committed-but-unpushed commits) and skip when there is no upstream, so a never-pushed branch under /do:review or /do:better is unaffected.
…licit refspec, keep hard-errors dirty - The inconclusive rule's trailing '~opt passes are ignored here' contradicted the push-failed carve-out three clauses earlier, so an ~opt reviewer's stranded fixes could still compute clean and merge. Scope the exemption to the statuses it covers. - Push with an explicit refspec: a bare 'git push' under push.default=matching fans out to every same-named local branch, including a release branch that auto-tags. - Skip the assertion when the pass committed nothing, so it never publishes deliberately-unpushed local work on a pass that had no fixes of its own. - A hard-error keeps its status instead of being rewritten to push-failed, which would downgrade the aggregate from dirty to inconclusive and slip past do:pr's abort-before-creating-the-PR gate. - Add the missing push-failed enumeration to do:release's restatement of the rule.
…cal branch name
`git push origin HEAD` resolves <dst> to `refs/heads/<local-branch-name>` and
ignores the upstream, so on a branch whose upstream is named differently it
creates a spurious remote branch, leaves the real PR head stale, and
`@{u}..HEAD` stays non-empty — issue #134's exact failure, reintroduced inside
the guard against it. Reproduced in a scratch repo before fixing. The push target
is now derived from the upstream ref the check already resolves.
Also from this round:
- Run the assertion on hard-error passes rather than asserting they always no-op.
A faithfully-executed loop reverts to its start commit and the check falls
through, but an improvised loop body may have skipped the revert — which is the
case this whole assertion exists for.
- Use `if` instead of a trailing `[ … ] && …`, so the snippet doesn't exit
non-zero on the live path and read as a failed command to the orchestrator.
- Make the test guards structural: a reworded but still-unqualified `~opt`
exemption would have slipped past the literal-string match.
… in the same shell
Two failure modes reproduced in a scratch repo before fixing:
- Splitting the abbrev-ref mangles two real upstream shapes. A branch tracking a
LOCAL branch (`branch.<n>.remote=.`, what `git branch --set-upstream-to=main`
produces) abbreviates to a bare `main`, so both halves of the split return
`main` and the push targets a nonexistent remote — and on such a branch
`@{u}..HEAD` is non-empty by construction, so the assertion fires on a healthy
branch and records a false `push-failed` that blocks the merge. A remote whose
name contains a slash (`up/stream/main`) mis-splits the same way. Derive remote
and branch from `branch.<n>.remote` / `.merge` instead, and treat a local
upstream as "no remote to assert against". Since `.merge` is already fully
qualified the destination is `HEAD:$PUSH_BRANCH`, not `HEAD:refs/heads/…`.
- The push was prescribed in prose OUTSIDE the fenced block that defined
PUSH_REMOTE/PUSH_BRANCH. Spec snippets run as separate Bash invocations where
shell variables do not persist, so it would have executed against the
empty-string remote and reported another false `push-failed`. Fold the push and
its retry into the same block; parallel step 6 now reuses that whole block
rather than restating a push whose variables nothing in that section defines.
Tests made structural: every prescribed push is scanned from raw text (the
in-block occurrences carry no backticks) and must use the derived form.
…n rule self-contained - do:pr's unpushed-commits gate printed the SHAs and left the decision to adjacent prose. Since the failure this whole change guards against IS a step being skimmed past, the snippet now exits non-zero instead of emitting output someone has to interpret. - The `clean` aggregate rule excluded push-failed only by rule ordering (the inconclusive rule matches first). State it explicitly so the rule is true on its own terms — the round-1 CRITICAL in this same file was exactly a later sentence quietly contradicting an earlier carve-out. - Relax three test regexes that pinned incidental formatting (an exact three-space comment gap, an adjacency assumption between a guard and its body, and a 600-char proximity window) so a reformat with no behavior change can't fail them. Rejected from this pass: the claim that `git rev-parse --abbrev-ref --symbolic-full-name` combines conflicting flags and errors. It is a standard idiom and ran correctly here on every upstream shape tested. Also rejected a suggestion to let the prescribed-push scan span newlines, which would make the regex greedy across the retry line and match less precisely than it does now.
…tranding the branch The retry's `git pull --rebase --autostash` can hit a content conflict, and nothing aborted the half-finished rebase. Reproduced end to end: after a concurrent push to the same branch plus an overlapping edit, the block left `rebase in progress`, HEAD detached, the tree conflicted, and the pass's own fix commit reachable only as REBASE_HEAD. That is worse than the state issue #134 describes, because `push-failed` is a continue-signal: the wrapper goes on to the next reviewer, which now runs against a detached, conflicted tree — and that reviewer's own push assertion silently no-ops, since a detached HEAD has no resolvable @{u}, so PUSH_REMOTE is empty and the check skips. The guard built to surface stranded fixes would report nothing. The inner loops carry the same pull-rebase-then-push idiom, but there it is the loop's last act before reporting failure; here execution deliberately continues, which is what turns a recoverable failure into a corrupted tree for every later pass. Abort the rebase on the conflict path and keep the non-zero exit that drives `push-failed`, so the branch is always returned exactly as the reviewer left it.
…e-branch The bare 'false' — the line that makes a failed push observable to the orchestrator — was guarded by nothing. Deleting it left the suite green while flipping the abort path's exit status to 0 (git rebase --abort succeeds), so a genuine push failure would read as success, the pass would never be recorded push-failed, and the stranded commits would reach the merge gate: issue #134 verbatim. The old assertion also matched 'git rebase --abort' anywhere in the file, so moving the abort out of the else-branch and merely naming it in prose kept the suite green with a conflicted rebase left in place. One anchored assertion catches both. Verified by re-running both mutations: each now fails exactly one test.
…eviewer cli-error (#139) * fix: guard possibly-empty array expansions so bash 3.2 can't fake a reviewer cli-error (#138) Stock macOS ships neither timeout(1) nor gtimeout, so TIMEOUT_CMD is legitimately empty there. Under /bin/bash 3.2 a bare "${ARR[@]}" on an empty array is an unset expansion, so with set -u it aborts with 'unbound variable' before the reviewer ever starts. Every file then returns RC=1 with empty output, the loop counts them all as REVIEW_ERRORS, and the pass resolves to cli-error -- a hard error that ~opt does not excuse -- blocking the merge on a PR no reviewer looked at. Switch TIMEOUT_CMD, MODEL_FLAG, and OLLAMA_FLAGS to the ${ARR[@]+"${ARR[@]}"} form across the ollama, local-agent, and enhance loops; document that an absent timeout binary is the common macOS case and a supported configuration rather than a reviewer failure; and add contract tests that fail if an unguarded expansion returns. * address review (claude+antigravity): collapse double-wrapped guards and hoist the rule into one partial The mechanical rewrite in the previous commit wrapped three already-guarded expansions a second time, publishing a competing spelling of the very rule this change exists to teach. Collapse them, and move the five near-identical copies of the explanation into lib/empty-array-expansion.md (the lib/gh-host.md convention) so the next amendment can't land in two files and rot in three. The contract test now scans by pattern instead of a hardcoded array-name list, so a newly introduced optional-argument array is covered the day it lands, and it also rejects the double-wrap slip. Its companion no longer pins the loops' prose wording -- it asserts the shared partial carries the contract and that each loop links to it. Registering the new partial in install.sh/uninstall.sh was caught by the existing curl-installer LIBS allowlist test. * address review (antigravity): catch the quoted form of a double-wrapped guard too ${A[@]+"${A[@]+…}"} is the same slip as ${A[@]+${A[@]+…}} but slipped past the detector, so the contract test allowed exactly one spelling of the mistake it exists to reject.
…nd scope it to the pipeline commands
docs: say that --issues picks a destination, not a run mode, in simplify and config
…tics docs: say plainly that --issues picks a destination and --scan-only stops the pipeline
…t from config, and file issues before the scan-only exit
The pre-PR assertion compares against the upstream ref, so it only ever sees
committed work — but /do:pr's Local Code Review gate never told anyone to commit
its fixes (unlike /do:release's, which does). A gate fix left in the working tree
was therefore invisible to the guard and the PR opened from the pre-fix tree.
The same bullet prescribed 'git push origin {current_branch}', which hardcodes the
local branch name as the destination — the exact property multi-reviewer-loop.md
calls out as #134's failure reintroduced inside its own guard. Derive remote and
ref from branch.<name>.remote/.merge, matching that file.
The scan-only issue-filing instruction sat in the paragraph AFTER 'STOP HERE ...
and exit', so an orchestrator following the gate literally exited before reaching
it and '--scan-only --issues' filed nothing. Fold it into the gate sentence.
…and scope do:pr's -u push to the never-published case
My previous fix copied multi-reviewer-loop.md's config-derived push but left
behind its `[ "$PUSH_REMOTE" = "." ]` guard. On a branch whose upstream is a
local ref (branch.<n>.remote=".", what `git branch --set-upstream-to=main`
produces) @{u} resolves, so the no-upstream carve-out never fires and the push
became `git push . HEAD:refs/heads/main` — silently fast-forwarding the local
default branch onto the feature branch, exiting 0, and emptying @{u}..HEAD. The
guard would then report the branch had reached the remote and open a PR for a
branch never pushed anywhere. Reproduced in a scratch repo both before and after.
Also: `git push -u origin <branch>` rewrites branch.<n>.remote/.merge, so running
it unconditionally re-points an existing upstream at origin/<local-name> and
defeats the config-derived guard at its source. Scope it to the never-published
case. Mirror the lib's `git rebase --abort` on a conflicted retry, since /do:next
and /do:pr-better invoke /do:pr programmatically and would inherit the mid-rebase
tree. Name the push form the review gate should use, rather than leaving the
orchestrator to pick one of the two forms this file forbids.
Tests: broaden the ~opt exemption scan to match by verb stem with a window on both
sides, and add coverage for the local-upstream guard and the scan-only gate
placement (the latter had none, and its regression reads fine in isolation).
…lue, not on whether @{u} resolves
Round 2's split routed on `git rev-parse @{u}` succeeding, but a branch tracking
a local ref resolves @{u} perfectly well — so a local upstream fell through to the
derived-push case, which carries no "." guard, reproducing the same CRITICAL one
section earlier: `git push . HEAD:refs/heads/main` fast-forwards local main, exits
0, empties @{u}..HEAD, and the PR-creation assertion then reads the branch as
pushed. The alternative reading was no better — an orchestrator copying the guard
wholesale dead-ends at Commit and Push printing "REFUSING TO CREATE THE PR" where
no PR is being created, so a local-upstream branch could never be published at all.
Discriminate on branch.<name>.remote being empty or "." instead. Both route to
`git push -u origin <branch>`, which publishes the branch and re-points the local
upstream at the remote — making the later guard defense-in-depth rather than a
dead end. Tests now pin the discriminator itself, not just the -u form's placement.
…se, where the round-1 bug would re-enter
The round-3 assertions covered the first case-split bullet and the discriminator
sentence, but nothing pinned the second bullet's 'never -u'. Reintroducing `git
push -u origin {current_branch}` there — the exact round-1 CRITICAL, where -u
rewrites branch.<n>.remote/.merge and re-points a differently-named or non-origin
upstream at origin/<local-name> — passed all 15 tests in the file. It now fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release v3.27.1
Released: 2026-08-12
Highlights
/do:prpushes anything still outstanding before opening a PR, and it refuses to merge while your branch is ahead of the remote — naming the commits that would have been dropped.--issuesreads as what it is: a destination, not a run mode./do:simplifyand/do:confignow say outright that the flag selects where findings are recorded and does not suppress remediation, and point at--scan-only --issuesfor the audit-and-file run people were actually reaching for.Fixed
/do:prpushes anything outstanding before opening the PR, and the merge gate fails closed when the local branch is ahead of its upstream, listing the commits at risk.@{u}) and config rather than assuming the local branch name matches the remote.cli-error. (fix: guard possibly-empty array expansions so bash 3.2 can't fake a reviewer cli-error #139)Docs
--issuesno longer reads like it stops the run./do:simplify's flag list and/do:config's issue-mode default now state that--issuesselects where findings are recorded and does not suppress remediation, and reference--scan-only --issuesfor the audit-and-file workflow. (--issues reads as a run mode but is a sink selector; --scan-only --issues is underspecified #135, docs: say plainly that --issues picks a destination and --scan-only stops the pipeline #136)--review-withand scoped to the pipeline commands, so commands that never run a reviewer no longer imply one gates their merge.Full Changelog
Full Diff: v3.27.0...v3.27.1