From f8db865357f47386ec777f5937c004d6423d3748 Mon Sep 17 00:00:00 2001 From: Fixture Date: Mon, 24 Aug 2026 13:32:33 -0700 Subject: [PATCH 1/2] fix(premerge): scope 4c's verify review to the polish, not the whole stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `### 4c — VERIFY` dispatched `Skill("code-review", " ")` while its own prose claimed the call was "scoped to the refactor". `` is the stack PR, so nothing was scoped: the verify step re-reviewed the entire stack and the reviewer treated that whole diff as its scope. 4c is not a discovery pass — it asks "did the polish do harm". Asking a critic "what is wrong with this stack" answers from an unbounded set, so any blocker that sample rolled — including one present all along that the green gate had not sampled — reached the third table arm as "a blocker the polish introduced" and reverted a correct, behaviour-preserving refactor. The scope now comes from the checkout rather than from an argument. 4b commits the polish and deliberately does not push it; 4c reviews with a level and no target, so the reviewer's own default `git diff @{upstream}...HEAD` — upstream still at the pre-simplify SHA, via Phase 0c's `push -u` — resolves to exactly the polish commit. A new `premerge-simplify-push` fence then pushes. Passing the range explicitly was tried and rejected. On `o48-xhigh-v1` (the cell `claude-opus-5` xhigh resolves to, and `xhigh` is the default level) `finderBudgetHint` is false, so `o30` never parses the target and it reaches the model as prose. An ignored target does not degrade to a wider review: it falls back to that same default, which on an already-pushed branch is an empty range over a clean tree — zero findings and a vacuous green from the one phase that exists to catch a bad polish. The push runs regardless of `--no-post-simplify-review`; only the review, triage and gate are gated on it. It is the only push of the simplify commit, and both the command file and RFC 0021 document that flag as costing the verification, never the polish. Also here: - a third arm for 4b exiting non-zero (wrong branch, untracked files, absent allowed set, stray files), each of which leaves the lens edits uncommitted in a dirty tree where the empty-range fallback would sweep them in; - an arm for 4b's `COMMIT=none REASON=no-edits` early exit; - RFC 0021 A3 C8 amended: "a two-SHA range is not in that surface" was read off the review prompt's enumeration rather than executed. `o30` accepts only no-target or a range and returns on every other shape — a range is the only target form that code parses. The amendment records the bound as well as the retraction, which is why 4c passes no target at all; - rationale extracted to references/ so the body stays on its pinned size ceiling rather than raising it (#779). Closes #795 --- docs/rfc/0021-premerge-stack-integration.md | 65 +++++++-- .../uberdev/skills/premerge-pipeline/SKILL.md | 136 +++++++++--------- .../references/common-mistakes.md | 36 ++++- .../references/phase-contracts.md | 59 ++++++++ tests/premerge.test.sh | 96 +++++++++++++ 5 files changed, 310 insertions(+), 82 deletions(-) diff --git a/docs/rfc/0021-premerge-stack-integration.md b/docs/rfc/0021-premerge-stack-integration.md index 4659ecb5..1e5aaaea 100644 --- a/docs/rfc/0021-premerge-stack-integration.md +++ b/docs/rfc/0021-premerge-stack-integration.md @@ -647,10 +647,12 @@ findings, and the second round's six were all in text the first round's fixes ha just written — the amendment reproducing, on itself, the defect it documents. Two of those were structural rather than cosmetic: the design named `refuted` as a gate-closing state while defining a producer only for `fixed`, and it prescribed -reviewing a two-SHA range through a dispatch surface that accepts only a PR, a -branch or a path. A design with a state nothing can produce and a keystone +reviewing a two-SHA range through a dispatch surface believed to accept only a +PR, a branch or a path. A design with a state nothing can produce and a keystone nothing can invoke is a sketch, and a sketch attracts "you did not specify X" -without bound. +without bound. (The second of those two was itself wrong about the reviewer — +see C8's 2026-08-24 amendment. The *shape* of the criticism held: the keystone +was asserted, not executed. Executing it is what found the range regex.) So what A3 ratifies is the diagnosis above — every pointer in it executed and confirmed — plus the constraint set below. Each constraint is a way the obvious @@ -721,11 +723,51 @@ carried as a retained backstop. **C8 — the incremental review needs a dispatch that exists.** The only reviewer the pipeline has is the built-in `code-review` skill (`SKILL.md:584`), whose -accepted targets are a PR number, a branch, or a path; the no-target default is -`git diff @{upstream}...HEAD` plus the working tree. A two-SHA range is not in -that surface. C1 needs a path set derived from the repair diff, a locally-arranged -checkout the default picks up, or an extension to the target surface — named -explicitly, because "review the delta" is not yet an invocation. +Phase 0 enumerates a PR number, a branch, or a path; the no-target default is +`git diff @{upstream}...HEAD` plus the working tree. + +> **Amended 2026-08-24 (#795).** This constraint originally read *"a two-SHA range +> is not in that surface"*. That is **false**, and it was asserted from the prompt's +> enumeration rather than executed against the invocation path. Read out of the +> shipped binary (`2.1.241`), in three parts, because only the first two support the +> retraction and the third bounds it: +> +> 1. **The parser passes anything through.** `hLs` splits the post-level remainder +> on whitespace and `qau` joins every token back into ONE free-text target +> string. No shape is rejected. +> 2. **The CLI carries an explicit range path.** `o30` accepts either no target — in +> which case it uses `@{upstream}...HEAD` — or a target of at most 256 chars +> matching `^[@\w][@\w./~^-]*\.\.\.?[@\w][@\w./~^-]*$`, on which it runs +> `git diff --no-ext-diff --no-textconv --numstat --end-of-options --`, +> and it `return`s on every other shape. A range is the *only* target form that +> code parses at all; a PR number, a branch and a path all fall straight through +> it. "Not in that surface" is exactly backwards. +> 3. **On the cell /premerge actually runs, that path is dead.** `o30` is reached +> only from `n30`, which returns immediately unless the cell sets +> `finderBudgetHint`. `O_e` — the constructor every non-Sonnet cell is built +> from — sets it **false**, and `claude-opus-5` xhigh resolves to +> `O_e("o48-xhigh-v1")`. `PREMERGE_DEFAULT_LEVEL` is `xhigh`. So on the default +> cell nothing in the CLI parses the target; it reaches the model as prose, under +> a Phase 0 that says *"review that target instead. Treat this diff as the review +> scope."* +> +> Net: a range is a legitimate, recognised target shape, and on the default cell it +> is **interpreted rather than enforced**. `### 4c — VERIFY` therefore does **not** +> pass one. An ignored target does not degrade to a wider review — it degrades to +> the no-target default, which on an already-pushed branch is an empty range, a +> clean tree and zero findings: a vacuous green from the one phase that exists to +> catch a bad polish. 4c instead leaves the polish commit unpushed until after the +> review, so that default resolves to exactly the polish and no target is needed. +> +> What survives of C8 is the narrower point C1 actually needs: a range scopes the +> **review**, but the findings that come back carry `file` and `line` and no hunk +> provenance, so per-finding attribution to *"text the previous repair wrote"* is +> still not purchasable this way. + +C1 still needs a path set derived from the repair diff, a locally-arranged +checkout the default picks up, or per-finding provenance — named explicitly, +because "review the delta *and know which findings are about it*" is still not +yet an invocation. **On the narrow verifier.** The instinct behind C4 — that checking must be cheaper than discovering, and structurally unable to emit new findings — is sound @@ -986,9 +1028,10 @@ rediscovering them. landed in text the previous repair *wrote*; what is measured is whether it landed in a **file** that repair modified. A genuinely new defect four hundred lines from the repair hunk counts as growth. The precise version needs diff-hunk -mapping against a review pointed at one repair's delta, and C8 records that no -such dispatch exists — the only reviewer this pipeline has accepts a PR, a -branch or a path, and a two-SHA range is not in that surface. So the proxy errs +mapping against a review pointed at one repair's delta. Pointing a review at that +delta is available — C8's amendment records the range target form, which `### 4c` +deliberately does not use — but findings carry `file`, `line` and no provenance, so +the mapping still has nothing to key on. So the proxy errs toward stopping, which is what the two-round requirement exists to absorb. Making it precise belongs to the C1–C8 work, not to a follow-up on this one. diff --git a/plugins/uberdev/skills/premerge-pipeline/SKILL.md b/plugins/uberdev/skills/premerge-pipeline/SKILL.md index 6a55260c..3d3cdcf3 100644 --- a/plugins/uberdev/skills/premerge-pipeline/SKILL.md +++ b/plugins/uberdev/skills/premerge-pipeline/SKILL.md @@ -1448,8 +1448,10 @@ None of the five stops the loop, and none of them is `0.00`. Reading `-` as zero *we did not look* into *we looked and it was fine*. **Deliberately the coarse proxy.** The precise question — did this finding land in text -the previous repair *wrote* — needs diff-hunk mapping nothing here produces (A3 C8: the -reviewer takes a PR, a branch or a path, not a two-SHA range). File membership counts a +the previous repair *wrote* — needs per-finding hunk provenance, and the reviewer returns +`file` and `line` and nothing that says which hunk a line came from. (A3 C8's second +reason — *"a two-SHA range is not in that surface"* — is **retracted**; see its amendment, +and note that a range would have bought scope, never provenance.) File membership counts a new finding anywhere in a touched file, so it errs toward stopping, which the two-round requirement absorbs. Do not build the hunk-mapping version to "fix" this; build it, if ever, as the C1–C8 design A3 is waiting for. @@ -1921,7 +1923,7 @@ $PREMERGE_RUN_DIR/simplify-scope-.allowed ``` Then commit with the fence below, which is the Phase 2a guard with that file as -its assigned list. Push. +its assigned list. **Do not push** — `### 4c` step 2 does, after the review. ```bash uberdev-executable origin=premerge-simplify-commit set -u @@ -1978,13 +1980,9 @@ fi git add -u || exit 2 git commit -m "refactor(premerge): apply simplify lenses to the stack" >/dev/null || exit 2 -PREMERGE_PUSH_ERR="$(git push origin "$PREMERGE_BRANCH" 2>&1 1>/dev/null)" || { - # Bounded and defused first — references/phase-contracts.md, `### What a failed push is allowed to say`. - PREMERGE_PUSH_ERR="$(printf '%s' "${PREMERGE_PUSH_ERR//PREMERGE/PRE-MERGE}" | tr -s '\n\r\t' ' ' | cut -c1-200)" - printf 'error: pushing the simplify commit to %s failed: %s\n' \ - "$PREMERGE_BRANCH" "$PREMERGE_PUSH_ERR" >&2 - exit 2 -} +# COMMITS AND DOES NOT PUSH: while `origin/` is still one commit behind, +# the reviewer's no-target default IS the polish, which is `### 4c`'s whole scope +# mechanism. `### 4c` step 2 pushes. printf 'PREMERGE SIMPLIFY COMMIT=%s ATTEMPT=%s\n' "$(git rev-parse HEAD)" "$PREMERGE_ATTEMPT" >&2 ``` @@ -2018,20 +2016,58 @@ is written about.)* ### 4c — VERIFY -**Runs unless `--no-post-simplify-review`.** This is the phase the shipped -pipeline was missing entirely. - -Phase 4 is the last thing to touch the branch, and every gate in the run was -computed *before* it. So a refactor that reddened the suite, or changed behaviour -while claiming not to, or introduced a fresh blocker, would ship inside a stack -whose summary says `clean gate green` — and Phase 5's `chore(release)` commit -would then attest to code nothing ever verified. +**Steps 1, 3 and 4 run unless `--no-post-simplify-review`. Step 2 — the push — +runs regardless**: it is the only push of the simplify commit, and both +`commands/premerge.md` and RFC 0021 promise that flag costs the *verification*, +never the polish itself. + +Branch on what `### 4b` reported, and there is no fourth arm: + +- **`COMMIT=none REASON=no-edits`** — skip 4c entirely and go to Phase 5, leaving + `PREMERGE_ATTEMPT` where it is. Nothing was applied, so there is nothing to + push, verify or revert. +- **a non-zero exit** — 4b refused (wrong branch, untracked files, absent allowed + set, stray files), and each of those leaves the lens edits uncommitted in a + dirty tree. **Stop the run; do not verify.** The reviewer's empty-range + fallback would otherwise pull those scope-rejected edits into the review. +- **`COMMIT=`** — run a full attempt cycle at `PREMERGE_ATTEMPT + 1`: not just + the gate, the whole Phase 1 → Phase 2 → Phase 3 sequence, as the loop runs it. + +1. `Skill("code-review", "")` — **a level and NOTHING ELSE. No target + argument at all, and never the stack PR number.** 4c asks *"did the polish do + harm"*; pointed at the stack PR it asks *"what is wrong with this stack"* and a + critic answers that from an unbounded set, so a blocker it rolls in a file the + polish never touched reverts a correct refactor (#795). The scope comes from the + **checkout**: the reviewer's no-target default is `git diff @{upstream}...HEAD`, + `### 4b` left the polish unpushed and Phase 0c set upstream tracking, so that + default resolves to exactly the polish commit. **Do not "improve" this by passing + the range explicitly** — `references/common-mistakes.md`, *"Handing 4c a target + argument"*, records why an ignored target degrades to an EMPTY review. + +2. **Push the polish** — after step 1's review, on the runs where step 1 ran: + +```bash uberdev-executable origin=premerge-simplify-push +set -u +PREMERGE_BRANCH="${PREMERGE_BRANCH:?PREMERGE_BRANCH must be prefixed onto this fence by the orchestrator}" +# A fence is a fresh shell and cannot assume the checkout did not move under it. +PREMERGE_HEAD_BRANCH="$(git symbolic-ref -q --short HEAD)" || PREMERGE_HEAD_BRANCH="" +[ "$PREMERGE_HEAD_BRANCH" = "$PREMERGE_BRANCH" ] || { + printf 'error: expected to be on %s but HEAD is %s; refusing to push the simplify pass\n' \ + "$PREMERGE_BRANCH" "${PREMERGE_HEAD_BRANCH:-(detached)}" >&2 + exit 2 +} +PREMERGE_PUSH_ERR="$(git push origin "$PREMERGE_BRANCH" 2>&1 1>/dev/null)" || { + # Bounded and defused first — references/phase-contracts.md, `### What a failed push is allowed to say`. + PREMERGE_PUSH_ERR="$(printf '%s' "${PREMERGE_PUSH_ERR//PREMERGE/PRE-MERGE}" | tr -s '\n\r\t' ' ' | cut -c1-200)" + printf 'error: pushing the simplify commit to %s failed: %s\n' \ + "$PREMERGE_BRANCH" "$PREMERGE_PUSH_ERR" >&2 + exit 2 +} +printf 'PREMERGE SIMPLIFY PUSHED=%s\n' "$(git rev-parse HEAD)" >&2 +``` -**Run a full attempt cycle at `PREMERGE_ATTEMPT + 1`.** Not just the gate — the -whole Phase 1 → Phase 2 → Phase 3 sequence, exactly as the loop runs it: -1. `Skill("code-review", " ")` again, scoped to the refactor. -2. The **Phase 2 triage fence** at the new attempt index. This is the step that +3. The **Phase 2 triage fence** at the new attempt index. This is the step that is easy to skip and fatal to skip: `plan` is what stamps `head_sha`, and the simplify commit has already moved `HEAD`. Re-gating without re-stamping compares the pre-simplify evidence against the post-simplify SHA, which is @@ -2039,7 +2075,7 @@ whole Phase 1 → Phase 2 → Phase 3 sequence, exactly as the loop runs it: single run and revert every correct, behaviour-preserving polish pass. A self-check that always fails is worse than none: it trains its reader to ignore it. -3. The **Phase 3 gate fence** at the new attempt index with `PREMERGE_PUSHED=1`. +4. The **Phase 3 gate fence** at the new attempt index with `PREMERGE_PUSHED=1`. Report the post-simplify CI state as a row of its own in the summary, not as an update to the pre-simplify one — two different SHAs were measured and the operator should see both. @@ -2049,13 +2085,7 @@ attempt index 4c planned, so its `defer` and its summary describe the tree that was actually verified rather than the one before the polish. **Read the gate verdict from `gate-.json` directly — do not route this -through `converge`.** Two reasons, and the second is the load-bearing one. The -repair budget may already be spent, and `converge` refuses an attempt past it -(`bad_attempt`, exit 74), which would turn a successful verify into a crash. And -`converge` would answer the wrong question anyway: the predecessor attempt gated -**green**, so its blocker set is empty, so a blocker the polish introduced is -`appeared > 0` with `fixed == 0` — `STOP_REGRESSED` on the first call, on every -run, which parks the exact commit this phase exists to remove. The branch is: +through `converge`.** The branch is: | Verify outcome | Action | |---|---| @@ -2063,44 +2093,14 @@ run, which parks the exact commit this phase exists to remove. The branch is: | `not_green`, and **every** reason is waitable — `ci=pending`, `ci=no_checks_after_push`, `ci=unknown`, `mergeable=unknown` | wait `PREMERGE_CI_SETTLE_SECS`, re-run **this same** Phase 3 gate fence (same attempt index, `PREMERGE_PUSHED=1`), and take this table again. At most `PREMERGE_WAIT_CI_CEILING` re-probes; reaching the ceiling is `not_green` for `ci_unsettled` — the state `converge` would call `STOP_UNREADABLE` — i.e. the row below | | `not_green` for **any** other reason — a blocker the polish introduced, `ci=red`, `mergeable=CONFLICTING`, `stale_evidence` | **revert the simplify commit**, push, and report `simplify=reverted ()` | -The arms are disjoint and exhaustive by construction: the verdict is `green` or -`not_green`, and a not-green reason set either sits entirely inside the waitable -set or it does not. Count the re-probes in the controller — 4c does not call -`converge`, so it writes no `converge.jsonl` row for `_count_wait_rows` to find, -and an uncounted wait is an unbounded loop under a command that promises it will -not loop forever. - -**The wait arm is not politeness; without it the verify is a coin flip that -always lands the same way.** Step 3 gates with `PREMERGE_PUSHED=1` immediately -after pushing the simplify commit, and by `### The CI settle window` above -`gh pr checks` reports nothing at all for the first 10–30 seconds after a push. -So the first probe answers `not_green REASONS=ci=no_checks_after_push` on -essentially every run, and reverting on that throws away a correct, -behaviour-preserving polish because the build had not started yet — the same -"a self-check that always fails is worse than none" hazard step 2 warns about. -Everywhere else in the pipeline those reasons are explicitly waitable; 4c does -not get to be the one gate reader that treats them as a verdict. - -**And there is no "repair it" arm, deliberately.** Simplify is optional polish, -so its failure mode is `undo`, not `debug`: routing a broken polish back into the -loop spends the repair budget — when any is left — on a refactor nobody asked -for, and on the failure it would most often be handed, a blocker the polish -itself introduced, `converge` stops with `STOP_REGRESSED` before one fixer is -dispatched. So the run would park the broken polish either way, which is what -both this file's `## The order within one attempt` ("un-does itself if it did -harm") and the command's own guarantee ("reverted, not debugged") forbid. - -The revert rule is the point. **A stack that was green before the polish and -broken after it does not need a human to debug the polish at 3am — it needs the -polish gone.** `git revert --no-edit ` restores the last state that -actually passed a gate, and the un-applied findings are already on their way to -issues, so nothing is lost except a refactor that was not safe to ship. Report -the verify reason alongside `simplify=reverted`: the revert removes the polish, -not the reason, and a `mergeable=CONFLICTING` that appeared while Phase 4 ran is -still true of the stack afterwards. - -Never revert past the fix commits. They are the loop's product and they are what -made the stack green in the first place. +The arms are disjoint and exhaustive. **Count the re-probes in the controller**: 4c +writes no `converge.jsonl` row for `_count_wait_rows`, so a wait is else unbounded. + +**Never revert past the fix commits** — they are the loop's product and what made the +stack green. Report the verify reason alongside `simplify=reverted`: the revert +removes the polish, not the reason. + +`references/phase-contracts.md` `### Phase 4c` carries the reasoning for all three. --- diff --git a/plugins/uberdev/skills/premerge-pipeline/references/common-mistakes.md b/plugins/uberdev/skills/premerge-pipeline/references/common-mistakes.md index 0f39f6ce..4188d39f 100644 --- a/plugins/uberdev/skills/premerge-pipeline/references/common-mistakes.md +++ b/plugins/uberdev/skills/premerge-pipeline/references/common-mistakes.md @@ -90,9 +90,12 @@ twice and no stop is the count condition doing its job, not the detector failing over-counts: a new finding anywhere in a touched file counts as growth. It is a signal, not a proof, and that is deliberate — it is also why the stop needs two consecutive eligible rounds rather than one. The precise version needs diff-hunk -mapping against a review pointed at one repair's delta, and RFC 0021 A3 C8 records -that no such dispatch exists yet — building it here would replace a working coarse -signal with a keystone nothing can invoke. +mapping against a review pointed at one repair's delta. Pointing a review at that +delta is available in principle — a two-SHA range is a recognised target shape +(RFC 0021 A3 C8, as amended 2026-08-24) — but **`### 4c` deliberately does not use +one**; see *"Handing 4c a target argument"* below for why. And a range would not +help here regardless: the findings it returns carry no hunk provenance, so the +mapping has nothing to key on. **Reading "the round is not measured" as "the round repaired nothing".** The one inference that is worth more than the ratio's precision, because it is the case @@ -214,3 +217,30 @@ only correct writer. **Skipping the accounted-for gate in 0c.** It is the substitute for the assertion `/premerge` deliberately does not run. Removing it leaves the pack with no completeness proof at all. + +**Handing 4c a target argument.** `### 4c — VERIFY` dispatches +`Skill("code-review", "")` with **no target**, and the omission is the +mechanism, not an oversight. It is tempting to "fix" it by naming the polish +explicitly — the stack PR (what it used to do, #795) or the `..` range. +Both are wrong, and the range is wrong in the more dangerous direction. + +A range *is* a shape the CLI recognises: `o30` accepts either no target or one +matching `^[@\w][@\w./~^-]*\.\.\.?[@\w][@\w./~^-]*$` and returns on everything else. +But `o30` runs only when the review cell sets `finderBudgetHint`, and `O_e` sets +that **false** for `o48-xhigh-v1` — the cell `claude-opus-5` xhigh resolves to, and +`xhigh` is `PREMERGE_DEFAULT_LEVEL`. On /premerge's own default cell nothing parses +the target; it reaches the model as prose (RFC 0021 A3 C8, as amended). + +**An ignored target does not degrade to a wider review. It degrades to an empty +one.** It falls back to the no-target default, `git diff @{upstream}...HEAD` — and +if 4b had pushed, `@{upstream}` would equal `HEAD`, so the range is empty, the tree +is clean, `git diff HEAD` is empty too, and the review returns zero findings. The +gate reads `verdict=green`, Phase 5 stamps `chore(release)`, and the summary says +the stack is green *after* simplify. That is a **vacuous green from the one phase +whose entire job is to catch a bad polish** — strictly worse than the full-stack +review it replaced, which at least looked at something. + +Leaving 4b's commit unpushed is what makes the default correct instead of empty: +`@{upstream}` is still the pre-simplify SHA, so the reviewer's *first* command +returns exactly the polish. Do not add a target, and do not move the push back into +`### 4b`. `tests/premerge.test.sh` P23 locks both halves. diff --git a/plugins/uberdev/skills/premerge-pipeline/references/phase-contracts.md b/plugins/uberdev/skills/premerge-pipeline/references/phase-contracts.md index da1f45c1..a17adf62 100644 --- a/plugins/uberdev/skills/premerge-pipeline/references/phase-contracts.md +++ b/plugins/uberdev/skills/premerge-pipeline/references/phase-contracts.md @@ -180,3 +180,62 @@ that was never going to change. `PREMERGE_WAIT_CI_CEILING` bounds the waiting. A check that never settles is not a reason to loop forever, and after that many re-probes the loop calls the evidence unreadable and stops — which is a *not-green* stop, never a pass. + + +--- + +### Phase 4c + +Extracted from `SKILL.md` `### 4c — VERIFY` (#795): the instructions and the table +stay in the body, the reasoning lives here. + +**Why the phase exists at all.** Phase 4 is the last thing to touch the branch, and +every gate in the run was computed *before* it. So a refactor that reddened the +suite, or changed behaviour while claiming not to, or introduced a fresh blocker, +would ship inside a stack whose summary says `clean gate green` — and Phase 5's +`chore(release)` commit would then attest to code nothing ever verified. + +**Why the verdict is read from `gate-.json` and not through `converge`.** Two +reasons, and the second is the load-bearing one. The repair budget may already be +spent, and `converge` refuses an attempt past it (`bad_attempt`, exit 74), which +would turn a successful verify into a crash. And `converge` would answer the wrong +question anyway: the predecessor attempt gated **green**, so its blocker set is +empty, so a blocker the polish introduced is `appeared > 0` with `fixed == 0` — +`STOP_REGRESSED` on the first call, on every run, which parks the exact commit this +phase exists to remove. + +**Why the arms are exhaustive.** The verdict is `green` or `not_green`, and a +not-green reason set either sits entirely inside the waitable set or it does not. + +**The wait arm is not politeness; without it the verify is a coin flip that +always lands the same way.** Step 4 gates with `PREMERGE_PUSHED=1` shortly after +step 2 pushed the simplify commit — only the step 3 triage dispatch sits between +them — and by `### The CI settle window` `gh pr checks` reports nothing at all for +the first 10–30 seconds after a push. So the first probe answers +`not_green REASONS=ci=no_checks_after_push` on essentially every run, and +reverting on that throws away a correct, behaviour-preserving polish because the +build had not started yet — the same "a self-check that always fails is worse +than none" hazard step 3 warns about. +Everywhere else in the pipeline those reasons are explicitly waitable; 4c does +not get to be the one gate reader that treats them as a verdict. + +**And there is no "repair it" arm, deliberately.** Simplify is optional polish, +so its failure mode is `undo`, not `debug`: routing a broken polish back into the +loop spends the repair budget — when any is left — on a refactor nobody asked +for, and on the failure it would most often be handed, a blocker the polish +itself introduced, `converge` stops with `STOP_REGRESSED` before one fixer is +dispatched. So the run would park the broken polish either way, which is what +both `SKILL.md`'s `## The order within one attempt` ("un-does itself if it did +harm") and the command's own guarantee ("reverted, not debugged") forbid. + +The revert rule is the point. **A stack that was green before the polish and +broken after it does not need a human to debug the polish at 3am — it needs the +polish gone.** `git revert --no-edit ` restores the last state that +actually passed a gate, and the un-applied findings are already on their way to +issues, so nothing is lost except a refactor that was not safe to ship. Report +the verify reason alongside `simplify=reverted`: the revert removes the polish, +not the reason, and a `mergeable=CONFLICTING` that appeared while Phase 4 ran is +still true of the stack afterwards. + +Never revert past the fix commits. They are the loop's product and they are what +made the stack green in the first place. diff --git a/tests/premerge.test.sh b/tests/premerge.test.sh index 9e6c4be3..5166e716 100644 --- a/tests/premerge.test.sh +++ b/tests/premerge.test.sh @@ -1438,6 +1438,102 @@ else echo " FAIL P22c: the append detector cannot see an append — P22b is vacuous"; FAIL=$((FAIL + 1)) fi +echo "== P23: the 4c verify review is scoped to the polish, not the stack (#795) ==" +# 4c asks "did the polish do harm", never "what else is wrong with the stack". +# Pointed at the stack PR it re-samples an unbounded finding set, and any blocker +# it happens to roll -- including one present all along that the green gate simply +# did not sample -- reads as "a blocker the polish introduced" and REVERTS a +# correct, behaviour-preserving refactor. +# +# The scope comes from the CHECKOUT, not from a target argument: 4b commits and +# does NOT push, so the reviewer's no-target default `git diff @{upstream}...HEAD` +# resolves to exactly the polish. Passing a range instead would be interpreted +# rather than enforced on the default cell, and an IGNORED target falls back to +# that same default -- which on an already-pushed branch is EMPTY. That is the +# trap these rows exist to keep shut: the failure is a vacuous green, not a wider +# review. +SIMPLIFY_PUSH_FENCE="$(fence_body "$SKILL" premerge-simplify-push)" +if [ -n "$SIMPLIFY_PUSH_FENCE" ]; then + echo " PASS P23: the simplify-push fence extracts non-empty"; PASS=$((PASS + 1)) +else + echo " FAIL P23: the simplify-push fence extracted empty - the origin tag moved"; FAIL=$((FAIL + 1)) +fi +assert_grep "$SKILL" '^```bash uberdev-executable origin=premerge-simplify-push$' \ + "P23: the simplify-push fence carries exactly its documented origin tag" +# THE LOAD-BEARING ROW. The commit fence must not push: the unpushed window IS +# 4c's review scope. A push here re-creates the empty-default vacuous green. +# assert_not_in / assert_in, NOT assert_no_grep / assert_grep: a fence body is a +# STRING, and handing it to a helper that greps a FILE makes the row vacuous -- +# grep cannot open it, reports no match, and a "must not contain" row passes for +# the wrong reason. The anti-vacuity mutant below is what caught that here. +assert_not_in "$SIMPLIFY_FENCE" 'push origin' \ + "P23: the simplify-COMMIT fence does not push - the unpushed window is the scope" +assert_in "$SIMPLIFY_PUSH_FENCE" 'push origin "$PREMERGE_BRANCH"' \ + "P23: the simplify-PUSH fence is the one that pushes" +# THE PROSE HALF. The fence-body row above cannot see 4b's SURROUNDING TEXT, and a +# leftover "Push." sentence next to a fence that must not push is a contradiction a +# controller resolves by pushing -- which closes the unpushed window and hands 4c an +# empty review. Scoped to the 4b section, extracted by heading, not the whole file. +P23_4B="$(awk '/^### 4b — Apply the behaviour-preserving findings$/{f=1;next} /^### 4b-defer/{f=0} f' "$SKILL")" +if [ -z "$P23_4B" ]; then + echo " FAIL P23: could not extract the 4b section - its heading moved"; FAIL=$((FAIL + 1)) +else + echo " PASS P23: the 4b section extracts non-empty"; PASS=$((PASS + 1)) +fi +assert_not_in "$P23_4B" '(^|[^-])[Pp]ush\.' \ + "P23: 4b's prose does not tell the controller to push" +assert_in "$P23_4B" 'Do not push' \ + "P23: 4b's prose says the opposite, in words" +# THE FLAG. The push is the ONLY push of the simplify commit, so it must not sit +# behind --no-post-simplify-review: that flag is documented as costing the +# verification, never the polish. Without this row the polish is committed locally +# and never reaches the stack PR on `/premerge --no-post-simplify-review`. +assert_fixed "$SKILL" 'Step 2 — the push —' \ + "P23: 4c distinguishes the flag-gated steps from the push" +assert_fixed "$SKILL" 'runs regardless' \ + "P23: and says the push runs regardless of --no-post-simplify-review" +# 4b's non-zero exits leave the lens edits UNCOMMITTED in a dirty tree, where the +# reviewer's empty-range fallback would sweep them into the review. +assert_fixed "$SKILL" 'a non-zero exit' \ + "P23: 4c has an arm for 4b refusing, not just for 4b finding nothing to do" +# 4c passes a level and no target. Both halves matter: the old bare-PR call is +# gone, and nothing reintroduced a target of any shape. +P23_BARE_PR_RE='code-review", " "' +assert_no_grep "$SKILL" "$P23_BARE_PR_RE" \ + "P23: 4c no longer dispatches the verify review at the bare stack PR" +assert_fixed "$SKILL" 'Skill("code-review", "")' \ + "P23: 4c dispatches with a level and no target argument" +assert_fixed "$SKILL" 'No target' \ + "P23: and says so in words, so the next editor does not helpfully add one" +# The no-edits arm: 4b can exit before making a commit, and 4c must skip rather +# than verify a commit that was never made. +assert_fixed "$SKILL" 'COMMIT=none REASON=no-edits`** — skip 4c entirely' \ + "P23: 4c has an explicit skip arm for 4b's no-edits early exit" +# The pointer 4c hands the next editor must resolve. A "do not do X, see Y for why" +# whose Y does not exist is how X gets re-added by someone acting in good faith. +assert_fixed "$SKILL_MISTAKES" 'Handing 4c a target argument' \ + "P23: the mistake entry 4c points at actually exists" +assert_fixed "$SKILL_MISTAKES" 'It degrades to an empty' \ + "P23: and it records the empty-review failure direction, not a wider-review one" +# The retraction of the false range claim stays retracted. +assert_no_grep "$SKILL" 'not a two-SHA range' \ + "P23: the false 'ranges are not a target form' claim is gone" +# ANTI-VACUITY, one per assert_no_grep above, each re-reading the SAME pattern +# rather than re-typing it. Herestrings, not pipes (EPIPE guard). +P23_PUSH_RE='push origin' +P23_PUSH_MUTANT='PREMERGE_PUSH_ERR="$(git push origin "$PREMERGE_BRANCH" 2>&1 1>/dev/null)" || {' +if grep -qE -e "$P23_PUSH_RE" <<<"$P23_PUSH_MUTANT"; then + echo " PASS P23: the push detector sees a push when there is one"; PASS=$((PASS + 1)) +else + echo " FAIL P23: the push detector cannot see a push - the row above is vacuous"; FAIL=$((FAIL + 1)) +fi +P23_MUTANT='1. `Skill("code-review", " ")` again, scoped to the refactor.' +if grep -qE -e "$P23_BARE_PR_RE" <<<"$P23_MUTANT"; then + echo " PASS P23: the bare-PR detector sees the old call when it is present"; PASS=$((PASS + 1)) +else + echo " FAIL P23: the bare-PR detector cannot see the old call - the row above is vacuous"; FAIL=$((FAIL + 1)) +fi + echo "" echo "== Summary ==" echo " passed: $PASS" From 063009d3361dfc183a395eb26269333f77e088d8 Mon Sep 17 00:00:00 2001 From: Fixture Date: Mon, 24 Aug 2026 13:32:36 -0700 Subject: [PATCH 2/2] chore(release): v0.56.2 --- .claude-plugin/marketplace.json | 2 +- CHANGELOG.md | 73 ++++++++++++++++++++++ README.md | 2 +- plugins/uberdev/.claude-plugin/plugin.json | 2 +- tests/goal.test.sh | 4 +- tests/solve-claim.test.sh | 4 +- 6 files changed, 80 insertions(+), 7 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 467c6141..0dc4ef6a 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -9,7 +9,7 @@ "name": "uberdev", "source": "./plugins/uberdev", "description": "GitHub workflow commands + full Superpowers skill suite (brainstorm with visual companion, write-plan, execute-plan, subagent-driven-dev wave-based parallel execution, finish-branch, systematic-debugging, TDD, worktrees, parallel-agents, verification, code-review pair, writing-skills, using-uberdev primer) and PR review agents. /solve and /turbo dispatch autonomous agents via a platform-aware dispatch backend (Workflow-native solver fleets by default, with detached wezterm / background transports opt-in — cross-platform on macOS, Linux, WSL2, native Windows); /issue creates well-investigated, deduped issues.", - "version": "0.56.1", + "version": "0.56.2", "category": "workflow", "tags": [ "github", diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a6b883b..37680398 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,79 @@ All notable changes to UberDev are documented here. The format is based on [Keep a Changelog 1.1.0](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.56.2] — 2026-08-24 + +### Fixed — `/premerge` 4c reviewed the whole stack, so a resampled blocker reverted a correct refactor (#795) + +`### 4c — VERIFY` dispatched `Skill("code-review", " ")` while +its own prose claimed the call was *"scoped to the refactor"*. `` is +the stack PR, so nothing was scoped: the verify step re-reviewed the entire stack +and the built-in reviewer treated that whole diff as its scope. + +4c is not a discovery pass — it asks *"did the polish do harm"*. Asking a critic +*"what is wrong with this stack"* answers from an unbounded set, so any blocker +that sample happened to roll — including one present all along that the green gate +had simply not sampled — reached the table's third arm as *"a blocker the polish +introduced"* and **reverted a correct, behaviour-preserving refactor**, reporting +`simplify=reverted` to an operator whose polish was fine. + +**The scope now comes from the checkout, not from an argument.** `### 4b` commits +the polish and deliberately does **not** push it; `### 4c` reviews with a level and +no target at all, so the reviewer's own default — `git diff @{upstream}...HEAD`, +with upstream still at the pre-simplify SHA — resolves to exactly the polish +commit. 4c then pushes (a new `premerge-simplify-push` fence, carrying the branch +assertion and the bounded stderr capture the old inline push had) and gates. + +That push runs **regardless of `--no-post-simplify-review`**, and only the review, +triage and gate steps are gated on it. Moving the push into 4c without that carve-out +would have put the *only* push of the simplify commit behind the flag: on +`/premerge --no-post-simplify-review --no-bump`, or on any repo without the version +ratchet, the polish would be committed locally, never pushed, absent from the stack +PR, and destroyed by the next `git reset --hard origin/...`. Both `commands/premerge.md` +and RFC 0021 document that flag as costing the verification, never the polish. + +4c also gained a third arm for `### 4b` **exiting non-zero** (wrong branch, untracked +files, absent allowed set, stray files). Each of those leaves the lens edits +uncommitted in a dirty tree, where the reviewer's empty-range fallback would sweep +the scope-rejected edits into the review; the arm stops the run instead. + +Passing the range explicitly was tried first and rejected. On the default cell the +target is prose the model may or may not honour, and an **ignored target does not +degrade to a wider review — it degrades to an empty one**: it falls back to that +same default, which on an already-pushed branch is an empty range over a clean +tree, so zero findings and a vacuous green from the phase whose whole job is to +catch a bad polish. Leaving the commit unpushed is what makes the default correct +rather than empty. + +Also fixed while here: 4c had no arm for `### 4b`'s `COMMIT=none REASON=no-edits` +early exit, so a lens pass that applied nothing sent 4c to verify a commit that was +never made. It now skips to Phase 5. + +### Fixed — a claim about the built-in reviewer that was asserted rather than executed + +RFC 0021 A3 **C8** recorded *"a two-SHA range is not in that surface"*, and +`## Phase 3b — CONVERGE` plus `references/common-mistakes.md` both repeated it. +It had been read off the review prompt's own enumeration rather than executed +against the invocation path. Read out of the shipped binary (`2.1.241`): + +- the argument parser (`hLs` → `qau`) joins every post-level token into one + free-text target and rejects no shape; +- `o30` takes either no target or one matching + `^[@\w][@\w./~^-]*\.\.\.?[@\w][@\w./~^-]*$` and returns on everything else — so + a range is the **only** target form that code parses; a PR number, a branch and + a path all fall through it. "Not in that surface" is backwards. + +The bound matters as much as the retraction, and is recorded with it: `o30` runs +only when the cell sets `finderBudgetHint`, which `O_e` sets **false** for +`o48-xhigh-v1` — the cell `claude-opus-5` xhigh resolves to, and `xhigh` is +`PREMERGE_DEFAULT_LEVEL`. On /premerge's own default cell the CLI never parses the +target at all. That bound is why 4c passes no target rather than a range. + +What survives of C8 is the point the growth ratio actually needs: a range would +scope the *review*, but findings come back carrying `file` and `line` and no hunk +provenance — so file-membership stays the honest proxy. C8 is amended in place +rather than rewritten, and the two downstream copies now say the same thing. + ## [0.56.1] — 2026-08-24 ### Changed — the project rule file is `CLAUDE.md`, not `AGENTS.md` diff --git a/README.md b/README.md index 30db5aae..1086aabb 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ **Personal Claude Code marketplace — opinionated GitHub-workflow slash commands.** -[![Version](https://img.shields.io/badge/version-0.56.1-blue)](./CHANGELOG.md) +[![Version](https://img.shields.io/badge/version-0.56.2-blue)](./CHANGELOG.md) [![License](https://img.shields.io/badge/license-MIT-green)](./LICENSE) [![Claude Code](https://img.shields.io/badge/Claude%20Code-plugin-8B5CF6)](https://docs.claude.com/en/docs/claude-code/plugins) [![Repo Agnostic](https://img.shields.io/badge/repo--agnostic-yes-success)](#configuration) diff --git a/plugins/uberdev/.claude-plugin/plugin.json b/plugins/uberdev/.claude-plugin/plugin.json index b3ac2d79..62d926f1 100644 --- a/plugins/uberdev/.claude-plugin/plugin.json +++ b/plugins/uberdev/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "uberdev", - "version": "0.56.1", + "version": "0.56.2", "description": "Production-grade Claude Code plugin: parallel-fanout PR review, autonomous GitHub issue triage and resolution via Workflow-native solver fleets (one worktree-isolated agent per issue; detached wezterm / background transports remain opt-in; cross-platform — macOS, Linux, WSL2, native Windows), brainstorm/plan/execute workflows with wave-based subagent dispatch.", "author": { "name": "TheFJK", diff --git a/tests/goal.test.sh b/tests/goal.test.sh index 1d65735c..17ad1ab7 100755 --- a/tests/goal.test.sh +++ b/tests/goal.test.sh @@ -505,8 +505,8 @@ assert_no_grep "$GOAL_LIB" '\bbash -c' "G19.no assert_grep "$GOAL_CMD" '--i-know-what-im-doing' "G19.r12-mentioned-once" echo -echo "== G20: version bump locked (0.56.1) ==" -assert_version_bump "$REPO_ROOT" "0.56.1" +echo "== G20: version bump locked (0.56.2) ==" +assert_version_bump "$REPO_ROOT" "0.56.2" assert_no_grep "$REPO_ROOT/tests/solve-claim.test.sh" '0\.30\.0' "G20.solve-claim-no-old-version" assert_grep "$GOAL_P0" 'uberdev_dispatch_resolve_env' "G20b.phase0-wires-resolve-env (#175 SSOT anchor)" diff --git a/tests/solve-claim.test.sh b/tests/solve-claim.test.sh index e78215e8..b34e34f0 100755 --- a/tests/solve-claim.test.sh +++ b/tests/solve-claim.test.sh @@ -46,8 +46,8 @@ source "$REPO_ROOT/tests/_lib_assert_structural.sh" || { echo "FATAL: _lib_asser # Both lines below are read by bump-version.sh as literal anchors. The echo text # and the assert_version_bump argument must carry the CURRENT version, and both # must be bumped together every release. -echo "== Version bump 0.56.0 -> 0.56.1 propagated ==" -assert_version_bump "$REPO_ROOT" "0.56.1" +echo "== Version bump 0.56.1 -> 0.56.2 propagated ==" +assert_version_bump "$REPO_ROOT" "0.56.2" # --- #123 B1: closing-keyword regex left-anchor ------------------------------- # Kept because it EXECUTES the regex rather than only asserting its presence,