fix(automerge): run do_automerge_tick after the validated set is built #256

Merged

joshtronic merged 1 commit from fix/automerge-cascade-order into master

2026-06-26 01:09:37 +00:00

igor commented

2026-06-26 01:01:23 +00:00

Collaborator

Bug the live demo flushed out on its first run. Every auto-merge gate condition was green for porksicle #60 — your APPROVE, CI success, cleanly mergeable, eligible (agent.json .smoke.url), shadow verdict COMMENT — yet the live tick kept going idle without merging.

Root cause: do_automerge_tick was wired in early (just after the deploy barrier, above the health gate), but it iterates VALIDATED_REPOS_JSON — which the validation sweep builds lower down in the cascade. So it looped over an empty repo set every tick and silently did nothing.

Fix: the deploy barrier stays early (it keys off .deploy state, needs no repo set), but do_automerge_tick moves to right after the validation sweep — same populated set do_review_tick uses. Ordering dependency now documented at both spots.

The unit tests stub VALIDATED_REPOS_JSON, so they pass regardless of cascade position — this was an integration/placement bug, not a logic one. bash -n + check-sync green.

No reviewer. Once this deploys, the live tick will pick up #60 (still approved + green) and run the loop for real — merge → barrier → smoke.

**Bug the live demo flushed out on its first run.** Every auto-merge gate condition was green for porksicle #60 — your APPROVE, CI success, cleanly mergeable, eligible (`agent.json .smoke.url`), shadow verdict COMMENT — yet the live tick kept going idle without merging. **Root cause:** `do_automerge_tick` was wired in **early** (just after the deploy barrier, above the health gate), but it iterates `VALIDATED_REPOS_JSON` — which the **validation sweep builds lower down in the cascade**. So it looped over an *empty* repo set every tick and silently did nothing. **Fix:** the deploy barrier stays early (it keys off `.deploy` state, needs no repo set), but `do_automerge_tick` moves to **right after the validation sweep** — same populated set `do_review_tick` uses. Ordering dependency now documented at both spots. The unit tests stub `VALIDATED_REPOS_JSON`, so they pass regardless of cascade position — this was an integration/placement bug, not a logic one. `bash -n` + `check-sync` green. No reviewer. Once this deploys, the live tick will pick up #60 (still approved + green) and run the loop for real — merge → barrier → smoke.

igor added 1 commit

2026-06-26 01:01:23 +00:00

fix(automerge): run do_automerge_tick AFTER the validation sweep builds the repo set

Lint / check-sync (pull_request) Successful in 4s

Details

Lint / check-sync (push) Successful in 4s

Details

9a414d1bd4

Phase 1 placed do_automerge_tick early in the cascade (right after the deploy
barrier, above the health gate) -- but it iterates VALIDATED_REPOS_JSON, which
the validation sweep doesn't build until lower down. So every tick it looped over
an EMPTY set and silently merged nothing, despite an approved + green + eligible
PR sitting right there.

Surfaced by the first live demo: every gate condition checked TRUE by hand
(approve, CI success, mergeable, eligible, verdict != RC), yet the live tick went
idle without merging -- because the set it scans was empty at that point.

Fix: the deploy barrier stays early (it keys off .deploy state, needs no repo
set), but do_automerge_tick moves to right after the validation sweep, where
VALIDATED_REPOS_JSON is populated -- same set do_review_tick uses. Comments now
state the ordering dependency at both spots.

Unit tests stub VALIDATED_REPOS_JSON so they couldn't catch a cascade-ordering
bug; this is an integration-placement fix. bash -n + check-sync green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor commented

2026-06-26 01:06:02 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 9a414d1b: success

Summary: Moves do_automerge_tick && exit 0 from just-after the deploy barrier (early, above the health gate) to just-after the VALIDATED_REPOS_JSON empty-check, so it iterates a populated validated set instead of an empty one. The core fix is plausible and in scope, but it carries a behavior change the description doesn't surface, and it lands with no regression guard — so I can't clear it for the human cold.

What checks out

The new placement sits immediately after if [ -z "$VALIDATED_REPOS_JSON" ]; then ... exit 0; fi, so at that point the validated set is guaranteed non-empty. That directly addresses the stated "looped over an empty set" bug.
Small, focused, single-issue diff. No CI-config touches. No security surface. CI is success.
Comments updated at both the old and new spots to document the ordering dependency.

What the human should weigh before merging

Cooldown behavior silently changes. The old comment was explicit that auto-merge was placed above the health gate on purpose — "non-model: API + curl, so they run even during a Claude health cooldown." The new placement is below the claude_health_blocked gate. I can't see whether that gate hard-exits or merely skips model sections, but if it exits the tick (which the original design — deliberately hoisting non-model work above it — strongly implies), then auto-merge will no longer run during a Claude health cooldown. That may be an acceptable trade (you fundamentally can't build VALIDATED_REPOS_JSON above the gate either), but it's a real regression of a documented property and the PR description doesn't acknowledge it. Worth a one-line confirmation that delaying merges until cooldown clears is intended.
No regression guard. The author candidly notes the unit tests stub VALIDATED_REPOS_JSON and "pass regardless of cascade position" — which is exactly why this bug shipped. The fix adds nothing to catch a future re-ordering. I understand an ordering bug in a bash cascade is awkward to unit-test, but as-is there's zero protection against the same class of bug recurring. At minimum, consider an assertion/test that do_automerge_tick is invoked only with a non-empty set, or an integration smoke that fails if it's placed above the sweep.

What I couldn't verify from the diff

The body of do_automerge_tick (does it correctly read the populated set?).
Whether claude_health_blocked exits or continues, which determines the cooldown impact above.
That the validation sweep between the two hunks is itself non-model / reliably populates the set on the live tick.

None of these is a confirmed defect, so this isn't a block — but the undocumented cooldown trade-off plus the absent regression guard are enough that a human should glance before merging rather than take an auto-APPROVE.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `9a414d1b`: **success** **Summary:** Moves `do_automerge_tick && exit 0` from just-after the deploy barrier (early, above the health gate) to just-after the `VALIDATED_REPOS_JSON` empty-check, so it iterates a populated validated set instead of an empty one. The core fix is plausible and in scope, but it carries a behavior change the description doesn't surface, and it lands with no regression guard — so I can't clear it for the human cold. **What checks out** - The new placement sits immediately after `if [ -z "$VALIDATED_REPOS_JSON" ]; then ... exit 0; fi`, so at that point the validated set is guaranteed non-empty. That directly addresses the stated "looped over an empty set" bug. - Small, focused, single-issue diff. No CI-config touches. No security surface. CI is `success`. - Comments updated at both the old and new spots to document the ordering dependency. **What the human should weigh before merging** - **Cooldown behavior silently changes.** The *old* comment was explicit that auto-merge was placed above the health gate on purpose — "non-model: API + curl, so they run even during a Claude health cooldown." The new placement is *below* the `claude_health_blocked` gate. I can't see whether that gate hard-exits or merely skips model sections, but if it exits the tick (which the original design — deliberately hoisting non-model work above it — strongly implies), then **auto-merge will no longer run during a Claude health cooldown.** That may be an acceptable trade (you fundamentally can't build `VALIDATED_REPOS_JSON` above the gate either), but it's a real regression of a documented property and the PR description doesn't acknowledge it. Worth a one-line confirmation that delaying merges until cooldown clears is intended. - **No regression guard.** The author candidly notes the unit tests stub `VALIDATED_REPOS_JSON` and "pass regardless of cascade position" — which is exactly why this bug shipped. The fix adds nothing to catch a future re-ordering. I understand an ordering bug in a bash cascade is awkward to unit-test, but as-is there's zero protection against the same class of bug recurring. At minimum, consider an assertion/test that `do_automerge_tick` is invoked only with a non-empty set, or an integration smoke that fails if it's placed above the sweep. **What I couldn't verify from the diff** - The body of `do_automerge_tick` (does it correctly read the populated set?). - Whether `claude_health_blocked` exits or continues, which determines the cooldown impact above. - That the validation sweep between the two hunks is itself non-model / reliably populates the set on the live tick. None of these is a confirmed defect, so this isn't a block — but the undocumented cooldown trade-off plus the absent regression guard are enough that a human should glance before merging rather than take an auto-APPROVE. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-06-26 01:06:02 +00:00

1 minute 10 seconds

igor requested review from joshtronic

2026-06-26 01:06:02 +00:00