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/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/.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/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/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/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" 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,