Skip to content

fix(refinery): funnel direct→pr auto-promote through the single mr PR surface (tk-kcr8v6)#77

Open
zook-bot wants to merge 1 commit into
mainfrom
polecat/tk-kcr8v6
Open

fix(refinery): funnel direct→pr auto-promote through the single mr PR surface (tk-kcr8v6)#77
zook-bot wants to merge 1 commit into
mainfrom
polecat/tk-kcr8v6

Conversation

@zook-bot

@zook-bot zook-bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Problem

When direct push to a protected branch is rejected (GH013), the refinery's
auto-promote path at formulas/mol-refinery-patrol.toml:411-423 flips
MERGE_STRATEGY=mr and is supposed to fall through to the mr block — but in
practice the refinery agent has skipped the gate twice in a row.

Live evidence

2026-05-25:

Both close reasons: "auto-promoted direct→pr (main protected, GH013); PR
#NN opened with rebased branch ff to origin/main".

Operator caught both on 2026-05-25, recovered by drafting the PRs and
hand-filing review beads (tk-m76f0h, tk-26oacb) routed to
gc-toolkit/gc-toolkit.polecat-codex.

Root cause

The formula's prose at line 416 says:

# Fall through to the "If MERGE_STRATEGY = 'mr'" block below.

And at line 426-428:

**If `$MERGE_STRATEGY` was just flipped to `mr` (auto-promotion fired),
skip steps 2-5 below and execute the "If MERGE_STRATEGY = 'mr'" block
instead. The rebased `temp` branch is still available for the PR push.**

This is correct prose, but the gate steps (lines 525-528 setting
PR_DRAFT_FLAG, lines 621-644 dispatching the codex review bead) live ~110
lines downstream and across two visually-similar shell blocks. The agent
has skipped them on both auto-promote firings — apparently shortcutting
to gh pr create + close once the rebased branch is pushed and the PR
URL is in hand.

Proposed fix (one of)

A. Inline the gate at the auto-promote site. Move the draft-flag check
and the review-bead dispatch into the auto-promote stanza itself so
the fall-through doesn't have to traverse the whole mr block. The mr
block keeps its existing logic for the bead-metadata=mr case.

B. Restructure so steps 2-5 of mr are the only PR-creation surface.
The auto-promote stanza becomes "set MERGE_STRATEGY=mr, reset hard,
GOTO mr-step-1" with no PR creation outside the mr block. Cleaner, but
requires the prose to be unambiguous that the auto-promote case must
re-enter the mr flow at step 1 (push), not at step 2 (create PR), since
the rebased branch hasn't been pushed yet.

B is the cleaner architecture (single source of truth for PR creation);
A is the lower-risk patch. Pick one based on appetite.

Acceptance criteria

  • A new refinery pour that hits GH013 on direct push opens its PR as draft
    AND files a codex review bead with the standard metadata (task_kind=review,
    pr_url, pr_number, work_bead, fix_target_pool, gc.routed_to).
  • The review_gate=codex default still gates the auto-promote path; setting
    review_gate="" (empty) disables it parallel to the mr path.
  • Regression coverage: a test or rehearsal exercising the auto-promote
    branch confirms both gate steps fire.

Implementation notes

Implemented: funnel direct→pr auto-promote through the single mr PR-creation surface (Option B). Auto-promote arm only flips MERGE_STRATEGY=mr and resets; direct-success tail (verify/close/cleanup/auto-ff) guarded with [ "$MERGE_STRATEGY" = "direct" ] so it no-ops after promotion (no false merged-close, temp survives for the PR). Added regression doctor check check-refinery-autopromote-review-gate (passes on fix, fails on pre-fix). All 6 pack doctor checks pass; TOML + bash -n valid. Branch polecat/tk-kcr8v6, target main.

Refinery handoff

  • Issue: tk-kcr8v6
  • Source branch: polecat/tk-kcr8v6 → Target: main
  • Rebased clean on main (ff); auto-promoted direct→pr (GH013 protected main).
  • Validation: all 6 pack doctor checks pass on the fix; the new check-refinery-autopromote-review-gate fails on pre-fix main (confirmed discriminating); TOML + bash -n valid.

… surface (tk-kcr8v6)

When a direct push to a protected branch is rejected (GH013), merge-push
auto-promotes to merge_strategy=pr and is supposed to fall through to the
"mr" block, which opens the PR as --draft (under review_gate=codex) and
dispatches a codex review bead. The refinery agent skipped that
fall-through twice (PR #68/tk-sma1xw and PR #69/tk-d8jhjb both opened
non-draft with no review bead) — the gate steps lived ~110 lines
downstream across two visually-similar shell blocks, so the long-distance
fall-through was unreliable and the agent shortcut to a bare `gh pr
create` + close.

Fix (Option B — single PR-creation surface, hardened):
- The auto-promote arm now only flips MERGE_STRATEGY=mr and resets; an
  explicit comment + echo state that it creates no PR and closes no bead,
  and that the mr block is the sole remaining handoff.
- The direct-success tail (verify / merged-close / cleanup / auto-ff) is
  each wrapped in `[ "$MERGE_STRATEGY" = "direct" ]`. After an
  auto-promotion (MERGE_STRATEGY=mr) every one no-ops, so the bead is not
  falsely closed as "merged" and `temp` survives for the PR push. The
  delete_merged_branches step is folded into the guard as explicit shell
  rather than trailing prose.
- The mr block is documented as the single PR surface serving both
  originally-mr work and auto-promoted work; the draft flag and review
  dispatch (both keyed on review_gate=codex) are the gate, and routing
  all PR creation through here is what guarantees the gate fires for
  auto-promoted work.

This is purely a gc-toolkit local delta (the auto-promote path itself is
local), so the base-artifact-collision snapshot is intentionally
untouched.

Regression coverage: doctor/check-refinery-autopromote-review-gate
statically pins, in the merge-push bash (comments + line-continuations
folded, if/else arms depth-tracked):
  1. the auto-promote arm funnels (MERGE_STRATEGY=mr) and contains no
     `gh pr create` / `gc bd close`;
  2. the direct merged-close is wrapped in the MERGE_STRATEGY=direct
     guard;
  3. PR_DRAFT_FLAG="--draft" is gated on review_gate=codex;
  4. the task_kind=review dispatch is gated on review_gate=codex;
  5. exactly one `gh pr create` exists (single surface).

Validation: the new check passes on this fix and fails (exit 2) on the
pre-fix origin/main formula, flagging the unguarded merged-close. All six
pack doctor checks pass; the formula parses as TOML; the new guarded
blocks pass `bash -n`.

@zook-bot zook-bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking findings.

Validation performed:

  • Reviewed the formula control-flow changes and the new static doctor check.
  • gc formula show mol-refinery-patrol --json parsed successfully.
  • bash -n doctor/check-refinery-autopromote-review-gate/run.sh passed.
  • GC_PACK_DIR=. ./doctor/check-refinery-autopromote-review-gate/run.sh passed on this branch.
  • All six doctor/*/run.sh checks passed with GC_PACK_DIR=..
  • The new doctor check fails against origin/main with the expected unguarded direct-close finding, so it discriminates the regression.
  • GitHub reports no branch checks for this PR.

@zook-bot
zook-bot marked this pull request as ready for review May 29, 2026 21:38
@zook-bot
zook-bot marked this pull request as draft June 5, 2026 20:09
@zook-bot

zook-bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Back to draft to flag follow-up (operator, 2026-06-05). The formula control-flow fix is sound — auto-promote arm only flips strategy; direct-success tail guarded with [ "$MERGE_STRATEGY" = "direct" ]; single mr PR-creation surface. Keep that.

The follow-up is the 258-line awk doctor check: it pins the exact textual structure of the merge-push bash (if/else depth, guard strings) and will false-positive on any future refactor — it polices the prose, not the behavior. The gate also being enforced in formula prose (an LLM following a 110-line fall-through) is why the bypass happened (#68/#69). Rework options: (a) replace the check with a behavioral post-condition (after merge-push, assert: protected-branch PR ⇒ draft AND review bead exists), or (b) push the invariant into gc's refinery logic (gascity bead). Do not merge until the check is reworked or dropped.

@zook-bot
zook-bot marked this pull request as ready for review June 12, 2026 06:56
zook-bot added a commit that referenced this pull request Jul 21, 2026
…one (tk-vpx62)

Second arm of tk-vpx62 (mayor scope extension, 2026-07-21). The first arm
handled a PR whose anchor is stranded; this one handles a PR whose anchor is
GONE.

Every close-on-land path — merge-skill.sh, this reconciler, the refinery patrol
— enumerates from the BEAD and reads its pr_number. Sound while the bead
outlives the PR, which close-on-land guarantees. But a PR whose bead is CLOSED
is not merely unhandled, it is unseen: no scan starts anywhere that reaches it,
so it never lands, never escalates, never times out. It does not read as broken,
it reads as absent. The pre-#163 close-on-publish model created that state by
design, and the 7 PRs it stranded (#171 #170 #166 #108 #88 #80 #77) sat
untouched for weeks — found only by a human cross-checking `gh pr list` against
the ledger by hand.

So reconcile from the other side too: enumerate open PRs, subtract every PR
number any LIVE bead references, report the remainder. Verified against the live
ledger: flags exactly those 7 and correctly excludes in-flight PR#195.

Detection and surfacing ONLY, per the mayor's scope note — it never merges,
closes, or reopens an anchorless PR. Disposition stays an operator call.

Convergence, since a patrol pass runs on every idle wake:
- Escalate once, bounded by an anchorless_flagged marker written to the closed
  bead BEFORE the mail (stamp-first, mirroring the merged path's close-first).
  A failed stamp means no bound, so it does not mail — a delayed escalation is
  recoverable, a mail storm is not.
- A PR with NO bead in any state is logged but never mailed: nothing durable
  exists to record that we already said it.
- A failed ledger read is empty, not "[]", and the scan fails CLOSED on it —
  treating "could not read" as "nothing is tracked" would flag every open PR at
  once.
- Anchor resolution picks the OLDEST bead carrying merge_result, so the operator
  is pointed at the bead that OPENED the PR rather than a review bead or a later
  "address findings" child (several closed beads routinely name one PR); the
  mail lists the whole set regardless.

Zero gating anchors is no longer an early exit — that was precisely the state
where this scan matters most (no live anchors WITH open PRs is the stranded
shape).

Tests: 24 new hermetic assertions (62 total, all pass); verified 13 of them FAIL
against the pre-arm script while all 38 pre-existing ones still pass. Sibling
suites green (merge-skill 36, one-anchor-per-pr 13, pre-open-resolve 16,
signoff-anchor-resolution 14, signoff-anchor-failclosed 14,
reconcile-graduated-convoys 14, reconcile-rig-checkouts 8, husk-guard 16).
bash -n + shellcheck clean at warning severity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant