Part of #2224 (3/3): OVERSEER_ALERT on pipeline-branch divergence with merged-PR signatures

[jwbron]

Summary

Defense-in-depth guardrail (Phase 4, item 3 of 3) for #2224, follow-on to #2222.

When a pipeline branch is materially ahead of base AND the ahead-commits contain merged-PR subject signatures (`(#NNNN)`), the pipeline-watcher emits an `OVERSEER_ALERT` so operators see the contamination shape from #2222 in real time rather than only at PR open.

Why

A real pipeline branch grows by refine/plan/implement/state-file commits authored by agents — none of those carry a `(#NNNN)` suffix in the subject. When that signature appears alongside material divergence, the branch has absorbed merged-main commits — the exact failure mode that produced #2220 (the contaminated PR for #2137). The signature heuristic is intentionally cheap and false-positive-tolerant; per the issue, we'd "rather over-alert than miss another contaminated PR."

Changes

• orchestrator/routes/pipelines.py:

  • New `BRANCH_DIVERGENCE_THRESHOLD = 20` and `_BRANCH_DIVERGENCE_PR_RE = re.compile(r"\(#\d+\)")`.
  • New helper `_check_branch_divergence_for_alert(...)` runs `git rev-list --count` then `git log --pretty=format:%H%x09%s` and filters to subjects matching the PR-signature regex. All errors logged-and-swallowed.
  • New helper `_publish_branch_divergence_alert(...)` mirrors `_publish_consensus_timeout_alert` (Replace auto-choice consensus-failure decision with OVERSEER_ALERT (protocol change) #2264): publishes `OVERSEER_ALERT` with offending SHAs in metadata, ties back to Investigate why pipeline branch ate main commits + carried stale prior-run history #2222 in the body.
  • Wired into `_health_monitor_poll` (every 30s tick) with a per-pipeline `divergence_alerted_shas` dedupe set so each offending commit produces at most one alert per pipeline run.

• orchestrator/tests/test_branch_divergence_alert.py: 13 tests covering: not-ahead, at-threshold (no alert), above-threshold without signatures (no alert), above-threshold with signatures (alert), all best-effort error paths (rev-list/log fails, subprocess timeout), branch == base no-op, threshold override, alert content + metadata, message-store-unavailable graceful skip, message-store exception swallow, offender-list truncation in body.

Scope

This is only PR 3 of 3 for issue #2224. PR 1 (gateway-side bare-rebase block) is in #2282; PR 2 (end-of-pipeline rebase) is next.

Test plan

• New unit tests pass (13/13).
• `ruff check` clean.
• CI green.
• Manual sanity: confirm alerts surface in operator-visible OVERSEER_ALERT flow.

[jwbron]

Verified: #2222 regression test (test_rebase_does_not_contaminate_when_base_fetch_silently_failed) still passes on this branch.

[egg-reviewer]

Reviewed orchestrator/routes/pipelines.py and orchestrator/tests/test_branch_divergence_alert.py end-to-end. Ran the new test file (13/13 pass) and tests/test_pipelines_routes.py (23/23 pass) to confirm no regressions in the surrounding area.

Verdict: no blocking issues. The helper + publisher functions correctly mirror _publish_consensus_timeout_alert (#2264), the from_role/to_role/MessageType wiring matches the existing OVERSEER_ALERT consumers, and best-effort error swallowing is consistent with the rest of this file. Inputs into the subprocess come from controlled internal sources and are passed as list args, so no shell-injection surface.

Several non-blocking observations follow.

Non-blocking

1. Detection latency is bounded by phase transitions, not the 30 s tick

The detector polls origin/<pipeline_branch>..origin/<base> every 30 s, but the polling thread never git fetches. Worktree fetches in this orchestrator only happen at:

• pipeline start (_sync_worktree_with_remote, line 13353),
• phase boundaries (line 14600),
• a handful of resume / signal paths.

Agents push their commits from inside their own containers via the gateway; the orchestrator's local origin/<pipeline_branch> only refreshes when the orchestrator itself fetches. So contamination introduced mid-phase (by a long-running implement phase) won't be detected by the polling tick — the next tick after the phase end fetch is the earliest it can fire.

The PR description says "operators see the contamination shape from #2222 in real time rather than only at PR open." In practice, "real time" here is "phase-boundary granularity," which can be many minutes to hours, not 30 s. That's still strictly better than detecting at PR open, but the framing oversells it.

Two options if you want true 30 s granularity:

• have _health_monitor_poll issue a cheap spawner.gateway.fetch_worktree_branch(...) immediately before the divergence check (best-effort, log-and-swallow), or
• accept the limitation and update the PR description / module comment to say "phase-boundary detection" rather than "real time."

Defense-in-depth is the goal and PR 1 (#2282) is the primary gate, so I don't think this blocks merge — but the next reader of _check_branch_divergence_for_alert will reasonably assume the staleness was considered.

2. Duplicate git rev-list --count between helper and caller

_check_branch_divergence_for_alert already runs rev-list --count (lines 9910-9920) and computes ahead, but throws it away. _health_monitor_poll then re-runs the same command inline (lines 13829-13846) just to put ahead into the alert body. This is:

• one extra subprocess per offender batch (3 git invocations per tick that fires, vs. 2),
• racy (the count can change between calls — if a push lands between them, the alert body's ahead_count and offending_shas come from different snapshots),
• a separate copy of the git -c core.hooksPath=/dev/null -c safe.directory=... boilerplate that has to stay in sync with the helper.

Suggested fix: change the helper to return tuple[int, list[tuple[str, str]]] (or attach ahead_count to a small dataclass) and drop the inline subprocess.run block entirely. The dedupe filter still works on the offenders list; the count just rides along.

3. Inline subprocess returncode is unchecked

In _health_monitor_poll lines 13829-13850, the second subprocess.run is parsed without checking returncode:

try:
    _ahead = int((_ahead_proc.stdout or "0").strip() or "0")
except ValueError:
    _ahead = 0

If git fails (rc != 0) or hits the 15 s timeout (caught by the outer except Exception at DEBUG), the alert path still proceeds with _ahead = 0. The published body will then say "is 0 commits ahead of origin/main and contains N commit(s)", which is internally contradictory and will confuse a reader. This goes away if you do (2) above.

4. Re-introduced contamination is silently suppressed

divergence_alerted_shas is a per-pipeline-run set keyed only by SHA. If contamination is alerted, then corrected (e.g., git push --force to a clean branch), then re-introduced with the same SHAs (e.g., the agent re-runs the same bad rebase), the second occurrence is filtered out and produces no alert.

That's an unusual sequence, but it directly defeats the "rather over-alert than miss another contaminated PR" stance from the issue. If you care about that case, drop the dedupe set when the offenders list goes empty (i.e., reset the set whenever _check_branch_divergence_for_alert returns [] while above-threshold), or only suppress for some short window rather than for the whole pipeline lifetime.

5. _health_monitor_poll integration is uncovered

The 13 new tests exercise _check_branch_divergence_for_alert and _publish_branch_divergence_alert in isolation, but nothing covers the polling-thread integration block at lines 13804-13865 — specifically the dedupe-via-divergence_alerted_shas, the inline subprocess.run, the _div_pipeline = store.load_pipeline(pipeline_id) re-load, or the conditional that skips the alert when new_offenders is empty. A regression in any of those is invisible to this test file.

A small focused test that drives the closure (e.g., builds the closure variables and calls the loop body once with patched subprocess.run and a fake store) would cover the integration without standing up a full pipeline. Not strictly required, but the dedupe logic in particular is the kind of thing that'll silently break.

6. Test naming / assertion nits

• test_returns_empty_when_below_threshold (line 110) actually exercises the at-threshold case (ahead == BRANCH_DIVERGENCE_THRESHOLD), not below. The docstring "Threshold is exclusive — at-threshold count returns empty" is correct; the test name disagrees. Either rename to test_returns_empty_when_at_threshold or add an explicit below-threshold case (e.g. ahead = THRESHOLD - 1).
• assert "abc1234"[:12] in msg.body (line 222) — "abc1234" is 7 chars, so [:12] is a no-op. The slice was presumably copy-pasted from the body's f"{sha[:12]} ..."; just write assert "abc1234" in msg.body.

7. ImportError fallback is uncovered

_publish_branch_divergence_alert has the from message_store import ... / from ..message_store import ... two-step import (lines 9996-10002) mirroring _publish_consensus_timeout_alert. Neither path is tested; if both imports failed the outer except Exception would catch it, but the swallow path for the import-failure case isn't exercised. Tracking with the consensus-timeout publisher (which has the same gap) is fine — just noting it.

8. BRANCH_DIVERGENCE_THRESHOLD = 20 is hardcoded

Module-level constant with no environment/config override. For long-lived branches with many legitimate refine/plan/implement commits + state-file commits, 20 is reachable in normal operation; for tiny single-task pipelines, 20 is essentially "always alert." Per the issue's "rather over-alert" stance the default is fine, but exposing this through PipelineConfig (or even just os.environ) would let operators tune false-positive rate without a code change.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Pushed d4f89f0 (on top of aafadbf after a main merge). Addressed:

1. Detection latency framing — Updated the module-level comment in orchestrator/routes/pipelines.py to call out explicitly that the polling thread does not fetch, so contamination introduced mid-phase is detected at the next phase boundary's fetch (not within 30 s). Phrased as "phase-boundary granularity, not real time" per your suggestion. Did not edit the PR description (gated to PR author).

2. Duplicate git rev-list --count — _check_branch_divergence_for_alert now returns tuple[int, list[tuple[str, str]]] so the caller drops its inline subprocess.run block entirely. The ahead count and the offender list now come from a single git snapshot.

3. Inline subprocess returncode unchecked — Goes away with (2) — the caller no longer runs git directly. _check_branch_divergence_for_alert already short-circuits on count.returncode != 0 and on ValueError, and now surfaces (0, []) in those error paths so the publisher is never invoked with a contradictory body.

4. Re-introduced contamination silently suppressed — _branch_divergence_tick clears divergence_alerted_shas whenever the offender list goes empty, so a corrected-then-re-introduced SHA fires again. Covered by test_re_introduction_after_reset_re_fires (3 ticks: fire → clear → re-fire).

5. _health_monitor_poll integration uncovered — Extracted the polling-tick logic into a new _branch_divergence_tick(...) helper so the closure body is now ~6 lines that call it. New TestBranchDivergenceTick class drives the helper through 8 scenarios: first-tick publish, dedupe across ticks, partial overlap, empty-offenders reset, re-introduction re-fires, missing branch/base skip, load_pipeline exception swallow, and per-tick re-load picking up branch updates.

6. Test naming / assertion nits — test_returns_empty_when_below_threshold → renamed to test_returns_empty_when_at_threshold (matches body), and a new test_returns_empty_when_below_threshold exercises a real below-threshold count. Fixed assert "abc1234"[:12] in msg.body → assert "abc1234" in msg.body.

Skipped per your own framing:

• Phase 6: Release preparation #7 ImportError fallback — you explicitly noted tracking this with the consensus-timeout publisher is fine. Agreed; no separate fix here.
• Merge Phase 1.5 docs into main #8 BRANCH_DIVERGENCE_THRESHOLD = 20 — you noted the current default is fine per the issue's over-alert stance. Leaving for a follow-up if operators want to tune false-positive rate.

Test counts after fixes: 22 in test_branch_divergence_alert.py (was 13), 23 in test_pipelines_routes.py (unchanged). All pass locally.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta from 7c72f825 → d4f89f0. Functional change is squarely in commit f4544b21 (the second Merge branch 'main' and the final merge are no-ops for the divergence-alert files). Ran the new test suite locally (22/22 pass) and traced the new helper end-to-end against _run_concurrent_phase's polling closure.

Verdict: no blocking issues. Previous review's non-blocking observations are properly addressed.

Resolution of previous feedback

#	Item	Resolution
1	Detection latency framing	Module-level comment now explicitly says "phase-boundary granularity, not real time" with the polling-thread-doesn't-fetch caveat (lines 9856-9864). Good.
2	Duplicate git rev-list --count	_check_branch_divergence_for_alert now returns tuple[int, list[tuple[str, str]]] (line 9879). Caller drops its inline subprocess.run block entirely. Single git snapshot, no race.
3	Inline subprocess returncode unchecked	Goes away with #2. The inline block is gone.
4	Re-introduced contamination silently suppressed	_branch_divergence_tick clears alerted_shas when not offenders (line 10073-10074). Covered by test_re_introduction_after_reset_re_fires.
5	_health_monitor_poll integration uncovered	New _branch_divergence_tick(...) helper extracted (lines 10043-10091); TestBranchDivergenceTick class adds 8 integration tests covering dedupe, partial overlap, reset-on-clear, re-introduction, missing-branch skip, store exception swallow, and per-tick re-load.
6	Test naming / assertion nits	test_returns_empty_when_at_threshold (renamed); new test_returns_empty_when_below_threshold exercises 19 vs threshold=20; assert "abc1234" in msg.body (slice removed).

Items #7 (ImportError fallback) and #8 (BRANCH_DIVERGENCE_THRESHOLD config) were explicitly framed as acceptable to defer in the previous review — no objection to skipping them.

Verification of new code

• _check_branch_divergence_for_alert return contract: All eight return paths produce a 2-tuple — confirmed by reading every return and the test matrix. The ahead value is preserved when the branch is below threshold or when the log call fails after a successful rev-list, which is what allows the caller to drop its second subprocess.
• _branch_divergence_tick dedupe logic: The clear branch (if not offenders and alerted_shas: alerted_shas.clear()) only runs when offenders is empty and the set is non-empty, so it's idempotent. The new-offenders filter then runs on a (possibly cleared) set — correct, since clearing makes everything new again, which matches the over-alert posture.
• Integration wiring: _run_concurrent_phase allocates divergence_alerted_shas once per pipeline run and passes it to the polling closure. Only the polling thread reads/writes it — no concurrent access concerns.
• Tests: All 22 pass on Python 3.14.4 / pytest 9.0.3. The new TestBranchDivergenceTick integration tests use MagicMock(spec=...) for store + targeted patches for the helper/publisher, which is the right level of mocking for an integration helper.

Non-blocking

Reset-on-empty conflates "below-threshold" with transient git error

_check_branch_divergence_for_alert returns (0, []) for several distinct cases, e.g.:

• branch genuinely not ahead of base,
• below-threshold,
• rev-list returncode != 0,
• subprocess.TimeoutExpired,
• ValueError parsing the count.

_branch_divergence_tick then treats any of those as "contamination cleared" and resets alerted_shas. So a transient git timeout mid-polling will flush the dedupe set, and the same SHAs will re-fire on the next successful tick. Per the issue's "rather over-alert than miss" stance this is consistent — but the next reader of _branch_divergence_tick will reasonably assume the reset only fires when contamination is genuinely gone, not on observability hiccups. A one-line comment ("Note: transient git errors also flush the dedupe set; this is intentional per #2224's over-alert posture") would save them five minutes of head-scratching.

PR description still says "real time"

The producer noted it's gated to PR author. The module-level comment is the authoritative spot now ("phase-boundary granularity, not real time"), so this is purely cosmetic. Worth a one-word edit at merge time if convenient — "real time" remains the literal text in the body of this PR.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. Pushed 12675f5 to address the first non-blocking observation:

Reset-on-empty conflates "below-threshold" with transient git error — Added a four-line inline comment in _branch_divergence_tick at the alerted_shas.clear() site explaining that transient git errors in _check_branch_divergence_for_alert also surface as offenders == [] and therefore flush the dedupe set, and that this is intentional per #2224's "rather over-alert than miss" posture. The behavior is unchanged.

PR description still says "real time" — This one I can't fix from the bot account: gh pr edit is gated to PR author and the gateway returns Edit denied: PR #2290 is not owned by james-in-a-box or configured user (author: jwbron). The module-level comment in orchestrator/routes/pipelines.py (lines 9856-9864) is now the authoritative phrasing ("phase-boundary granularity, not real time") so the PR-body wording is purely cosmetic at this point — flagging here for @jwbron to optionally reword at merge time.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta from d4f89f0b → 12675f59. Single commit, four lines added, no functional change.

Verdict: no blocking issues.

Resolution of previous feedback

#	Item	Resolution
1	Reset-on-empty conflates "below-threshold" with transient git error	Inline comment added at _branch_divergence_tick line 10073-10078 explaining that transient errors in _check_branch_divergence_for_alert surface as offenders == [] and therefore flush the dedupe set, with explicit reference to #2224's over-alert posture. Behavior unchanged.
2	PR description still says "real time"	Producer correctly identified this as gated to the PR author (gh pr edit denied by gateway). Module-level comment at lines 9856-9864 is the authoritative phrasing now ("phase-boundary granularity, not real time"). Acceptable handoff for @jwbron to optionally reword at merge time.

Verification

• Comment accuracy: Traced every error return in _check_branch_divergence_for_alert (lines 9873-9958):

  • count is None (rev-list timeout/OSError): returns (0, []) — offenders empty ✓
  • count.returncode != 0: returns (0, []) — offenders empty ✓
  • ValueError parsing count: returns (0, []) — offenders empty ✓
  • log is None or log.returncode != 0: returns (ahead, []) — offenders empty ✓ (note: ahead > 0 here)

  All four error paths have empty offenders, so the comment's claim "transient git errors ... also surface as offenders == []" is accurate. The reset-on-empty branch (if not offenders and alerted_shas) fires on not offenders regardless of ahead, so the dedupe-flush semantics described in the comment match the code.

• Tests: 22/22 in orchestrator/tests/test_branch_divergence_alert.py still pass. No test changes needed since the delta is comment-only.

• Diff scope: git diff d4f89f0b..12675f59 --stat shows exactly one file modified (orchestrator/routes/pipelines.py, +4/-0). No collateral edits.

Non-blocking

Commit message slightly overspecifies the error surface

The commit message says transient errors "fail (count returncode != 0, parse error, log timeout) and surface as (0, [])". The log timeout case actually returns (ahead, []) where ahead > threshold, not (0, []). The inline comment in the code correctly hedges to "surface as offenders == []" without claiming the tuple is always (0, []), so this inaccuracy stays out of the codebase. Purely cosmetic — flagging only because the next person reading commit history might be momentarily confused. No action needed.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.