From 235d0785f04473557a808ba3604869ca7cf54305 Mon Sep 17 00:00:00 2001 From: rysweet Date: Thu, 23 Jul 2026 15:05:27 +0000 Subject: [PATCH 1/2] fix(ooda): stop no-progress breaker livelock + escalate deploy-gate-converging PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem 1 (#4497, #4499, #4504, #4508, #4509, #4474, #4472) — the OODA no-progress / re-orientation breaker was livelocking: a still-blocked goal re-fired every overseer tick, spamming near-duplicate `ooda-stuck` tracking issues while the goal never converged. Fix, in src/ooda_loop/no_progress.rs: - breaker_signature(goal_id): deterministic per-goal dedup key (sha256("ooda-no-progress\n"+goal_id)[..8]) over the UN-redacted goal_id, so two distinct goals never collide (unlike failure_signature's UUID/hex redaction). - NoProgressIssueFiler::find_open_tracking_issue: read-only, fail-closed remote search-before-create (GhIssueFiler lists open `ooda-stuck` issues and scans bodies for the `ooda-signature:` marker). escalate_with_tracking_issue now reuses a live wip_ref, else re-links a matching remote issue, else files one embedding the marker — guaranteeing <=1 open issue per goal across re-orient AND process restart. Repairs the broken issue-filing/escalation path (#4472/#4474): a gh outage logs at error and keeps the goal Blocked, never aborting the cycle. - Skip-once guard: goal_is_sentinel_blocked() skips a goal still standing Blocked with the no-progress sentinel BEFORE any re-orient/escalation, so an already-escalated goal is not re-fired. It re-admits the instant the block is lifted (operator or agentic reasoner); a re-stall re-escalates idempotently against the existing signature. NoProgressBreakerReport.halted records the escalated-and-skipped goals for observability. Problem 2 (#4505) — the overseer's verify-and-merge escalation ignored a green/mergeable/non-draft PR that converges the very deploy gate blocking every self-deploy. Fix, in src/overseer: - config: CONVERGES_GATE_PR_LABEL ("converges-gate") + is_converges_gate_label (whole-string, spoof-resistant). - prioritize_gate_converging_prs(authorized, candidates, deploy_drift): pure, set-preserving stable partition that, only under an active DeployDrift, surfaces a labelled gate-converging PR FIRST within the already-authorized ready set. Never widens authority, never injects an unauthorized candidate, identity when drift is None. Threaded through PrOps::project_reasoned_ready_prs (trait/impl/fake) and the overseer tick call-site via ObservedState.deploy_drift; #4505 is surfaced through the existing VerifyAndMergePr path (no unsafe auto-merge). Additive / non-breaking; PRD preserved; no Bridge naming; structured tracing + OTel only (no stray print!/println!). New tests (tests_no_progress_livelock_dedup, tests_deploy_gate_escalation) plus concept docs; full lib suite green (9256 passed), fmt + clippy clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../deploy-gate-converging-pr-escalation.md | 289 ++++++++++++ docs/concepts/no-progress-livelock-dedup.md | 350 +++++++++++++++ docs/concepts/steerable-ooda-daemon.md | 1 + .../agentic-observe-orient-merge-queue.md | 1 + mkdocs.yml | 2 + src/ooda_loop/mod.rs | 9 + src/ooda_loop/no_progress.rs | 243 ++++++++++- .../tests_no_progress_livelock_dedup.rs | 413 ++++++++++++++++++ src/overseer/capabilities.rs | 1 + src/overseer/config.rs | 25 ++ src/overseer/merge_ops.rs | 12 +- src/overseer/mod.rs | 69 ++- src/overseer/tests_deploy_gate_escalation.rs | 228 ++++++++++ 13 files changed, 1629 insertions(+), 14 deletions(-) create mode 100644 docs/concepts/deploy-gate-converging-pr-escalation.md create mode 100644 docs/concepts/no-progress-livelock-dedup.md create mode 100644 src/ooda_loop/tests_no_progress_livelock_dedup.rs create mode 100644 src/overseer/tests_deploy_gate_escalation.rs diff --git a/docs/concepts/deploy-gate-converging-pr-escalation.md b/docs/concepts/deploy-gate-converging-pr-escalation.md new file mode 100644 index 000000000..9a21c6595 --- /dev/null +++ b/docs/concepts/deploy-gate-converging-pr-escalation.md @@ -0,0 +1,289 @@ +--- +title: The Overseer escalates the PR that converges the active deploy gate +description: > + How the Overseer's verify-and-merge escalation stopped ignoring a green, mergeable, + non-draft PR that fixes the very deploy gate blocking every self-deploy. When + `ObservedState.deploy_drift` is present (red-canary / DeployDrift active), the ready-PR + ranking now surfaces a gate-converging PR (e.g. #4505) FIRST as a `VerifyAndMergePr` + candidate via the existing opt-in merge path — a set-preserving re-ordering that never + widens merge authority and never auto-merges unsafely. +last_updated: 2026-07-23 +review_schedule: as-needed +owner: simard +doc_type: concept +status: implemented +issue: 4505 +related: + - ../design/agentic-observe-orient-merge-queue.md + - ./autonomous-self-merge-sensor.md + - ./autonomous-merge-review-gate.md + - ./reconcile-and-self-deploy.md + - ./deploy-aware-done-gate.md +--- + +# The Overseer escalates the PR that converges the active deploy gate + +> **Status: implemented (PR #4505, DeployDrift / red-canary context).** When a +> deploy gate is red (unit-test canary failing) and `DeployDrift` shows the +> running binary is behind merged `main`, the Overseer now **surfaces a green, +> mergeable, non-draft PR that converges that gate** as a verify-and-merge +> candidate — instead of escalating only unrelated PRs while the blocker +> persists. Primary sources: +> [`src/overseer/mod.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/mod.rs) +> (`project_ready_prs`, `decide`, deploy-drift wiring) and +> [`src/overseer/merge_ops.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/merge_ops.rs) +> (`survey_ready_prs`). + +## The defect this fixes + +PR `rysweet/Simard#4505` — *"fix(self-deploy): converge red-canary gate (env +isolation + …)"* — was **green** (checks `SUCCESS: 18`), `MERGEABLE`, non-draft, +authored by the Simard automerge author, and it **converged the self-deploy +red-canary gate that was blocking every self-deploy**. Yet from 07:55→13:14Z +every Overseer tick reported: + +```text +deploy … failed — deploy_gate: red canary (unit-test exit status 101) +DeployDrift: running binary 1 commit behind merged main +``` + +…and the Overseer only ever escalated verify-and-merge for **#4440** and +**#4398** — never **#4505**, the PR that would *clear the very gate it kept +failing on*. The daemon was deadlocked against its own fix: the change that would +turn the canary green sat un-escalated while the red canary blocked deploys. + +Root cause: the ready-PR **candidate selection did not correlate the active +deploy blocker to a PR that resolves it.** `project_ready_prs` filtered and +emitted eligible PRs, but nothing gave a *gate-converging* PR any priority — so +when the candidate window was dominated by other eligible PRs, the one PR that +mattered for the standing `DeployDrift` blocker was never surfaced for +escalation. + +## The fix, in one sentence + +When `DeployDrift` is active, **re-order** the already-authorized ready-PR set so +a PR that converges the active deploy gate is surfaced first as a +`VerifyAndMergePr` candidate — a *permutation*, never an expansion, of the merge +set. + +## What "converges the gate" means (correlation, not authorization) + +A candidate PR is treated as **gate-converging** only when it clears a concrete, +objective **minimum anchor** — never on title/branch heuristics alone: + +- the PR is in the **same repo** as the active `DeployDrift` (`rysweet/Simard`); +- **it proves Simard-origin** — its `PrSnapshot.labels` carry the durable + `simard-autonomous` marker, matched whole-string by + [`is_engineer_pr_label`](https://github.com/rysweet/Simard/blob/main/src/overseer/config.rs) + (`SIMARD_ENGINEER_PR_LABEL`), or its `head_ref` rides an engineer-exclusive + branch namespace (`is_engineer_branch`) — the same G3 narrowing + `project_ready_prs` already applies; +- it is **green + `MERGEABLE` + non-draft** (already required by the objective + gates); and +- **it carries an explicit, whole-string `converges-gate` label** — the concrete + per-PR predicate that identifies *which* Simard-origin PR resolves the standing + deploy gate. This label is the objective anchor for correlation: it is stamped + by the engineer on the gate-fixing PR at `gh pr create` time and matched + exactly (like `simard-autonomous`), so a look-alike (`converges-gate-ish`) + never qualifies. The `DeployDrift` must also be active + (`ObservedState.deploy_drift = Some`) for the label to have any ranking effect. + +The `converges-gate` label is the **sole** correlation predicate: there is no +title/branch heuristic and no `target_commit` match in the ranking. Title text +(e.g. a `fix(self-deploy)` / red-canary hint) has **no** ranking effect — a PR +without the explicit label is never promoted. + +> **Why a label and not a commit match.** The ready-PR projection +> (`ProjectionCandidate` / `PrSnapshot`) carries `labels`, `head_ref`, +> `mergeable`, checks and base — but **no PR head SHA** — so a candidate cannot be +> matched to `DeployDriftObservation.target_commit` at ranking time. The explicit +> `converges-gate` label is therefore the minimum objective, spoofing-resistant +> predicate that is actually available in the projected state. + +Correlation is **advisory ordering only**. It changes *which* authorized PR is +looked at first; it never makes an unauthorized PR eligible. A PR must already +have passed every objective gate (author allowlist + `simard-autonomous` +label/engineer-branch + green + `MERGEABLE` + non-draft + base allowlist) to be +in the set that gets re-ordered. + +## The ranking seam + +A new **pure** helper sits beside `project_ready_prs` and is a set-preserving +permutation of its output: + +```rust +/// Re-order `authorized` so any PR that converges the active deploy gate sorts +/// first, WITHOUT adding or removing any element. Pure and O(n); performs +/// no network I/O. Returns exactly the same multiset it was given. +/// +/// The input `authorized` set has already passed every objective/authority gate +/// (author allowlist + Simard-origin + green + `MERGEABLE` + non-draft + base +/// allowlist) via `project_ready_prs`, so origin is NOT re-checked here. Within +/// that set, a PR is "gate-converging" iff its matching `ProjectionCandidate` +/// carries the explicit whole-string `converges-gate` label +/// ([`config::is_converges_gate_label`]). There is no PR head SHA in the +/// projection, so no `target_commit` match is attempted, and there is no +/// title/branch heuristic — the label is the sole, spoofing-resistant anchor. +/// +/// `deploy_drift` is `ObservedState.deploy_drift`: `None` ⇒ identity (no +/// re-ordering). +pub fn prioritize_gate_converging_prs( + authorized: &[PrRef], + candidates: &[ProjectionCandidate], + deploy_drift: Option<&DeployDriftObservation>, +) -> Vec; +``` + +The `decide()` / `project_ready_prs` call-site threads +`ObservedState.deploy_drift` into the ranking so the gate-converging PR is +surfaced/prioritised as a `VerifyAndMergePr` candidate **alongside** #4440 / +#4398 — through the *same* path they already use. + +```text +observe → ObservedState { ready_prs, deploy_drift: Some(DeployDrift{..}), .. } + │ + project_ready_prs (author guard + engineer-PR + draft + objective gates) + │ ← unchanged authorized set + prioritize_gate_converging_prs(ready_prs, candidates, deploy_drift) + │ ← permutation: gate-converging PR first + decide → Intervention::VerifyAndMergePr { repo, pr: 4505 } + │ + allow_verify_merge gate + MergeJudge + anti-recursion author guard + │ (opt-in; unchanged) + gh pr merge --squash --delete-branch (NO --admin / NO --no-verify) +``` + +## Authorization is unchanged (the safety invariant) + +This feature is deliberately a **re-ordering of an already-authorized set**. It +does **not**: + +- widen `MergeAuthority` / `RiskClass::MergeAuthority`; +- change the opt-in `allow_verify_merge` gate (verify-and-merge still escalates + only when the operator has enabled it); +- bypass `MergeJudge`, the objective gates (`MERGEABLE` + all checks green + + base allowlist), the draft gate, or the anti-recursion author guard; +- use `--admin` or `--no-verify`; +- auto-merge. The gate-converging PR is surfaced through the **existing** + `VerifyAndMergePr` path exactly as #4440 / #4398 are — an operator-gated + verify-and-merge, not a silent self-merge. + +A unit test asserts the ranking output is a **permutation of its input** +(`sorted(out) == sorted(in)`), so no PR can ever *enter* the merge set through +this seam. + +> **Self-deploy recursion guard preserved.** The agent must still refuse to merge +> its own gate-fixing PR when the anti-recursion author guard would forbid it; +> this ranking never relaxes that guard. Surfacing a candidate is not merging it. + +## API summary + +| Symbol | Location | Role | +| --- | --- | --- | +| `prioritize_gate_converging_prs` | `src/overseer/mod.rs` | Pure, set-preserving re-ordering; gate-converging PR first when drift is active. | +| `ObservedState.deploy_drift` | `src/overseer/capabilities.rs` | The active `DeployDriftObservation`; `None` ⇒ identity ranking. | +| `DeployDriftObservation` | `src/overseer/capabilities.rs` | `{ target_commit, behind_commits }` — the standing blocker to converge on. | +| `project_ready_prs` | `src/overseer/mod.rs` | Unchanged authorized-set projection; its output is what gets re-ordered. | +| `Intervention::VerifyAndMergePr` | `src/overseer/intervention.rs` | The existing opt-in merge path the candidate is surfaced through. | + +## Configuration + +No new environment variables or config keys. Behavior depends only on state +already observed, plus one new **repo-level label convention** (`converges-gate`, +analogous to the existing `ooda-stuck` / `simard-autonomous` labels): + +| Existing setting | Effect on this feature | +| --- | --- | +| `allow_verify_merge` (opt-in) | Must be enabled for any verify-and-merge escalation, gate-converging or not — unchanged. | +| `SIMARD_AUTOMERGE_AUTHOR` | The candidate must be authored by this login (existing `survey_ready_prs` requirement). | +| `SIMARD_AUTOMERGE_REPOS` / base allowlist | The candidate's repo must pass the existing objective base-allowlist gate. | +| DeployDrift observer wired | When `ObservedState.deploy_drift` is `Some`, gate-converging ranking activates; when `None`, ranking is the identity. | +| `converges-gate` label (on the gate-fixing PR) | The explicit whole-string label the engineer stamps on the PR that resolves the standing deploy gate; it is the objective per-PR anchor the ranking keys on. No label ⇒ no gate-converging prioritisation (ranking falls back to identity). | + +## Examples + +### Before — the deadlock + +```text +07:55Z tick: deploy failed — deploy_gate: red canary (exit 101) + DeployDrift: 1 commit behind merged main + ready_prs = [#4440, #4398] ← #4505 present but not surfaced + escalate VerifyAndMergePr #4440 + escalate VerifyAndMergePr #4398 + (#4505 — the gate fix — never escalated) +…13:14Z: same red canary, same DeployDrift, still no #4505 +``` + +### After — the gate-converging PR is surfaced first + +```text +tick: DeployDrift active (target_commit=abc123, behind=1) + authorized ready_prs = [#4440, #4398, #4505] + #4505 carries labels [simard-autonomous, converges-gate] ← objective anchor + prioritize_gate_converging_prs → [#4505, #4440, #4398] + decide → VerifyAndMergePr #4505 (opt-in gate + MergeJudge + author guard) + → gh pr merge --squash --delete-branch (no --admin / no --no-verify) + red canary clears; DeployDrift resolves on next deploy +``` + +Expected structured log lines (no `print!`/`println!`): + +```text +[overseer::merge] deploy-gate-converging PR prioritised for verify-and-merge repo=rysweet/Simard pr=4505 behind=1 +[overseer::merge] verify-and-merge escalation repo=rysweet/Simard pr=4505 +``` + +## Verifying the behavior + +Tests in `src/overseer/tests_deploy_gate_escalation.rs` assert: + +- **`deploy_gate_converging_pr_is_ranked_first_under_active_drift`** — with + `DeployDrift` active and a green/mergeable/non-draft gate-converging PR in the + authorized set, that PR is ranked first (ahead of #4440 / #4398) so it becomes a + `VerifyAndMergePr` candidate. +- **`ranking_is_a_set_preserving_permutation_no_authority_widening`** — + `prioritize_gate_converging_prs` output is a permutation of its input + (`sorted(out) == sorted(in)`); it never adds or drops a PR. +- **`a_converging_candidate_not_in_the_authorized_set_is_never_injected`** — a + gate-converging candidate that is not already in the authorized set is never + promoted into it; the ranking only re-orders authorized PRs. +- **`no_drift_leaves_order_unchanged_even_with_a_converging_label`** — with + `deploy_drift = None` the ranking is the identity even when a `converges-gate` + label is present; ordinary escalation is unaffected. +- **`without_the_converges_gate_label_no_pr_is_promoted`** — only a PR carrying + the explicit whole-string `converges-gate` label qualifies as gate-converging. + A PR that merely has a `fix(self-deploy)` title but lacks the label is **not** + ranked first; the label is the sole predicate — title/branch text has no + ranking effect. + +Quality gates that must stay green: `scan_no_stray_prints` and +`scan_no_bridge_naming`. + +## Why this is safe + +- **Additive / non-breaking.** A new pure helper plus a minimal `decide()` + call-site edit; `project_ready_prs` and the merge gates are unchanged. The PRD + is preserved. +- **Set-preserving.** The seam can only *re-order* an authorized set, proven by a + permutation test — it can never widen who is eligible to merge. +- **Opt-in preserved.** Verify-and-merge still requires `allow_verify_merge`; a + gate-converging PR is surfaced, not auto-merged. +- **High-confidence correlation.** Gate-converging status requires a concrete + objective anchor — Simard-origin (the durable `simard-autonomous` label or an + engineer branch) plus an explicit whole-string `converges-gate` label — never + text heuristics alone, so no PR can impersonate the gate fixer with a look-alike + title. +- **Minimal surface.** P2 touches one pure helper and one call-site to avoid + merge conflicts with concurrent worktrees on the large `mod.rs`. + +## See also + +- [Design: agentic observe/orient merge-queue + issue reasoning](../design/agentic-observe-orient-merge-queue.md) + — the `reasoned_prs → ready_prs` re-narrowing this ranking sits behind. +- [Autonomous self-merge sensor](./autonomous-self-merge-sensor.md) — the + `survey_ready_prs` author-scoped sensor. +- [Autonomous-merge review gate](./autonomous-merge-review-gate.md) — the + `MergeJudge` authorization this never bypasses. +- [Reconcile-and-self-deploy](./reconcile-and-self-deploy.md) and + [Deploy-aware done-gate](./deploy-aware-done-gate.md) — the DeployDrift / + red-canary context. diff --git a/docs/concepts/no-progress-livelock-dedup.md b/docs/concepts/no-progress-livelock-dedup.md new file mode 100644 index 000000000..2819726a4 --- /dev/null +++ b/docs/concepts/no-progress-livelock-dedup.md @@ -0,0 +1,350 @@ +--- +title: The no-progress breaker escalates once and halts, instead of livelocking on re-orient +description: > + How the OODA no-progress / re-orientation breaker stopped spamming near-duplicate + `ooda-stuck` tracking issues for a still-blocked goal. Dedup is now sourced from a + remote, restart-durable signature (`ooda-signature:` embedded in the issue body, + computed per-goal over the un-redacted `goal_id` via a dedicated + `no_progress::breaker_signature` helper) rather than + the volatile in-memory `wip_refs` that `roll_to_new_cycle` clears on every re-orient. + After a guided retry still leaves the goal Blocked, the breaker escalates exactly once and + the goal — still standing Blocked with the no-progress sentinel — is skipped on subsequent + ticks (no re-orientation, no duplicate issue) until its block is lifted, repairing the + broken escalation + issue-filing path. +last_updated: 2026-07-23 +review_schedule: as-needed +owner: simard +doc_type: concept +status: implemented +issue: 4497 +related: + - ./blocked-goal-escalation-backoff.md + - ./no-progress-root-cause-resolution.md + - ./no-progress-terminal-investigation.md + - ./steerable-ooda-daemon.md + - ../reference/whisper-gate-backoff-api.md +--- + +# The no-progress breaker escalates once and halts + +> **Status: implemented (issues #4497, #4499, #4504, #4508, #4509, #4474, #4472).** +> A still-blocked goal now produces **at most one open `ooda-stuck` tracking +> issue**, and a goal that stays blocked after a guided retry is **escalated once +> and then skipped** (its re-orientation is suppressed while it stands Blocked +> with the no-progress sentinel) instead of re-firing on every Overseer tick. +> Primary source: +> [`src/ooda_loop/no_progress.rs`](https://github.com/rysweet/Simard/blob/main/src/ooda_loop/no_progress.rs) +> (`breaker_signature`, `escalate_with_tracking_issue`, `NoProgressIssueFiler`, +> `GhIssueFiler`, and the `goal_is_sentinel_blocked` skip guard). + +## The defect this fixes + +The OODA no-progress / re-orientation breaker was **livelocking**. In one ~6h +window (07:05–12:45Z) it auto-filed five near-duplicate operator-facing tracking +issues for the *same* two still-blocked goals: + +- `#4497`, `#4499`, `#4504`, `#4508` — all titled + *"OODA no-progress breaker: goal stuck after guided retry (UNCLEAR-CRITERIA)"* +- `#4509` — *"higher-order re-orientation livelock — standing"* + +while goals `4d27c91a` and `7f5afcca` stayed Blocked and never converged. Two +companion issues showed the breaker's own escalation machinery was defective: + +- `#4474` — *"no-progress breaker escalation is broken"* +- `#4472` — *"breaker cannot file its operator-facing tracking issue"* + +The root cause was two-fold: + +1. **Dedup was in-memory only.** `escalate_with_tracking_issue` deduped by + scanning the goal's in-memory `wip_refs` for a breaker-authored tracking ref + (`is_breaker_tracking_ref`). But `roll_to_new_cycle` **clears `wip_refs`** on + every re-orient, so the dedup memory was wiped each cycle. The next tick saw a + goal with empty `wip_refs`, concluded "no tracking issue yet", and filed + another one. Restarts had the same effect. The remote issue list — the actual + source of truth — was never consulted. + +2. **No terminal boundary.** After a guided retry still left a goal Blocked, the + breaker kept **re-orienting the same goal every tick** with no backoff and no + terminal state, so it re-entered the escalation path indefinitely. + +The result: one perpetually-blocked goal produced an unbounded stream of +duplicate `ooda-stuck` issues, burying the operator and never converging — a +self-inflicted denial-of-service on the issue tracker. + +## The fix, in one sentence + +Make dedup **remote and restart-durable** (a signature the issue body carries), +and make a post-guided-retry stall **skip**: escalate exactly once, then skip +that goal while it stands Blocked with the no-progress sentinel — until its block +is lifted. + +## Part 1 — remote, restart-durable dedup + +### The signature + +Each blocked goal gets a deterministic **per-goal** dedup signature, computed with +the **existing** stewardship hashing convention (16-hex first-8-bytes-of-SHA-256), +so no new hashing primitive is introduced: + +```rust +// src/ooda_loop/no_progress.rs +let signature = breaker_signature(goal_id); +``` + +`breaker_signature` returns a stable 16-hex-character key +(`sha256("ooda-no-progress\n" || goal_id)[..8]`). It is: + +- **Deterministic** — the same `goal_id` always yields the same signature. +- **Per-goal-distinct** — two *different* `goal_id`s yield two *different* + signatures. +- **Stable across re-orient and restart** — it depends only on `goal_id`, not on + volatile `wip_refs` or process memory. + +> **Design constraint — do NOT reuse `failure_signature` verbatim here.** +> `stewardship::dedup::failure_signature` first runs its argument through +> `normalize_for_signature` → `redact_token`, which deliberately folds volatile +> identifiers: any all-hex token of length ≥ 7 collapses to `` and any +> canonical UUID collapses to ``. Goal IDs are exactly such tokens (the +> real ones observed in this livelock — `4d27c91a`, `7f5afcca` — are 8-char hex), +> so passing a `goal_id` through `failure_signature` would redact **every** goal +> to the *same* placeholder and therefore hash **all goals to one shared +> signature** — collapsing the "per-goal" guarantee and over-deduping a second +> genuinely-stuck goal against the first goal's issue. The signature here must be +> computed over the **un-redacted** `goal_id`. `breaker_signature` reuses the +> *hashing format and marker convention* directly over `goal_id` — **not** +> `failure_signature`'s redaction pipeline. + +The signature is embedded in the tracking issue's body as a machine-readable +marker (mirroring stewardship's `stewardship-signature:` convention, but as a +single whitespace-delimited token so it survives body reflow): + +```text +ooda-signature:3f9a1c77b0e42d18 +``` + +### Search-before-create + +`NoProgressIssueFiler` gains a **read-only** lookup that consults the remote +issue list before creating anything: + +```rust +pub(crate) trait NoProgressIssueFiler { + fn file_issue(&self, title: &str, body: &str) -> Option; + + /// Return the open `ooda-stuck` tracking issue whose body embeds + /// `ooda-signature:`, if one already exists. Read-only; fail-closed + /// (returns `None` on any error, and the caller then treats the goal as + /// still Blocked without filing — never files a duplicate on uncertainty). + fn find_open_tracking_issue(&self, signature: &str) -> Option; +} +``` + +The production `GhIssueFiler` implements it with an **argv-only** listing — never +a shell string — then scans each returned body for the marker locally (a +substring search is used rather than `--search` so the match is exact and +independent of GitHub's search tokenizer): + +```bash +gh issue list --state open --label ooda-stuck --limit 200 \ + --json number,url,body +``` + +If the list command fails, `GhIssueFiler` logs the failure with +`tracing::error!(target: "simard::ooda", …)` and returns `None`; it **never +aborts the cycle** (this is the direct repair of `#4472` / `#4474`). + +### The escalation check order + +`escalate_with_tracking_issue` applies its checks in this order, guaranteeing +**≤ 1 open `ooda-stuck` issue per blocked goal**: + +1. **Live `wip_ref` check** — if the current in-memory goal still carries a + breaker-authored tracking ref, reuse it. Cheap fast path. +2. **Remote signature search** — call `find_open_tracking_issue(signature)`. If a + matching open issue exists, re-link it to the goal and file nothing. This + survives the `wip_refs` wipe and process restarts. +3. **File** — only when both of the above miss, `file_issue` creates one and + embeds the `ooda-signature:` marker; the result is linked back to the goal via + `link_tracking_issue`. + +The *skip-once* boundary (Part 2) is enforced one level up, in the breaker loop, +**before** `escalate_with_tracking_issue` is called at all: a goal already +standing Blocked with the no-progress sentinel is `continue`d past, so an +already-escalated goal never re-enters the escalation path. + +> **Migration note.** Tracking issues filed *before* this change do not carry the +> `ooda-signature:` marker, so they will not be matched by the remote search on +> the first cycle after upgrade. The result is at most **one** additional +> "file-new-once" per affected goal; from then on the new issue's marker makes the +> goal idempotent. This bounded one-time cost is acceptable and requires no +> back-fill. + +## Part 2 — skip-once after guided retry + +`NoProgressBreakerReport` gains an additive field recording which goals the +breaker escalated-and-skipped this tick (for observability and tests): + +```rust +pub(crate) struct NoProgressBreakerReport { + // … existing fields … + /// Goal IDs the breaker escalated exactly once this tick and will now skip + /// while they stand Blocked with the no-progress sentinel — populated at each + /// escalation site so a still-blocked goal is not re-oriented next tick. + pub halted: Vec, +} +``` + +The mechanism is deliberately keyed on the goal's **live block status**, not a +persisted "halted forever" flag. Before it does any work for a goal, the breaker +loop calls `goal_is_sentinel_blocked(state, goal_id)` — true when the goal +currently stands `Blocked(reason)` with a reason that `is_no_progress_marker` +recognizes as the breaker's own sentinel — and if so `continue`s past the goal +entirely (no counter bump, no re-orient, no escalation): + +```text +Blocked (no guided retry yet) + │ breaker fires → re-orient once, set guided_retry_used + ▼ +Blocked (guided_retry_used = true) + │ still Blocked next visit → escalate ONCE + │ (file/reuse ooda-stuck issue), report.halted.push(goal) + │ the goal is left Blocked with the no-progress sentinel + ▼ +Sentinel-Blocked + │ subsequent ticks: goal_is_sentinel_blocked → continue (skip): + │ NO re-orient, NO new issue + ▼ +(re-admitted the moment the goal leaves sentinel-Blocked — an operator + unblock, or the agentic reasoner's roll_to_new_cycle lifting the block) +``` + +**Why status-based, not a persisted flag.** Keying on the live block status means +the skip lifts automatically the instant the goal is unblocked — whether an +operator re-scopes it or the agentic reasoner re-orients it — with no separate +"clear the halt" bookkeeping to get wrong. The remote signature dedup +(Part 1) is what still prevents a *re-stall* from filing a **duplicate** issue: +an unblocked-then-re-stalled goal re-escalates idempotently against its existing +`ooda-signature:` marker. + +Key properties: + +- **Escalate exactly once.** The transition files or reuses a single tracking + issue, then the goal is skipped while sentinel-Blocked; it does not re-escalate + on later ticks. +- **No re-orient thrash.** A sentinel-Blocked goal is skipped *before* + `roll_to_new_cycle`, so the breaker stops wiping `wip_refs` and stops + re-entering the escalation path — killing the livelock at its source. +- **Reuses existing accounting.** The skip reuses the existing `guided_retry_used` + / `surfaced_failures` accounting rather than introducing a parallel timer, and + composes with the per-signature + [blocked-goal escalation backoff](./blocked-goal-escalation-backoff.md). +- **Not permanent silence.** The skip is gated on the *live* sentinel-Blocked + status: the moment the goal leaves that status (operator unblock or agentic + re-orient), it becomes actionable again. + +## API summary + +| Symbol | Location | Role | +| --- | --- | --- | +| `breaker_signature(goal_id)` | `src/ooda_loop/no_progress.rs` | Deterministic, **per-goal**, restart-durable dedup key over the **un-redacted** `goal_id` (reuses the SHA-256 format, not `failure_signature`'s redaction). | +| `ooda-signature:` | issue body marker | Remote dedup source of truth (single whitespace-delimited token). | +| `NoProgressIssueFiler::find_open_tracking_issue` | `src/ooda_loop/no_progress.rs` | Read-only, fail-closed remote list-and-scan before create. | +| `GhIssueFiler` | `src/ooda_loop/no_progress.rs` | argv-only `gh issue list … --json number,url,body`, marker matched locally; `tracing::error!` on failure, never aborts. | +| `escalate_with_tracking_issue` | `src/ooda_loop/no_progress.rs` | live-ref → remote-search → file. | +| `NoProgressBreakerReport.halted` | `src/ooda_loop/no_progress.rs` | Goal IDs escalated-once-and-skipped this tick. | +| `goal_is_sentinel_blocked` | `src/ooda_loop/no_progress.rs` | Skip guard: `continue`s past a goal still standing Blocked with the no-progress sentinel, before any escalation. | +| `is_no_progress_marker` | `src/goal_curation/no_progress_breaker.rs` | Recognizes the breaker's own Blocked-reason sentinel that the skip guard keys on. | + +## Configuration + +This feature is **on by default** and has no new environment variables or config +keys. It reuses the existing `gh` authentication and the standard OODA daemon +setup (see [Keeping the OODA daemon steerable](./steerable-ooda-daemon.md) for +`SIMARD_HOME` and related settings). The `ooda-stuck` label must exist in the +target repository, exactly as before this change. + +## Examples + +### Healthy behavior — one issue, then quiet + +A goal that becomes and stays blocked: + +```text +tick N : goal 4d27c91a Blocked, guided retry used → escalate + no open ooda-signature:3f9a1c77b0e42d18 found → file #4600 +tick N+1 : goal 4d27c91a still sentinel-Blocked → skip (no-op) +tick N+2 : goal 4d27c91a still sentinel-Blocked → skip (no-op) +… +operator closes #4600 and unblocks (re-scopes the goal) +tick N+k : no longer sentinel-Blocked → goal re-orients normally +``` + +Expected log lines (structured tracing; no `print!`/`println!`): + +```text +[simard::ooda] no-progress breaker: tracking issue filed for stuck goal issue=4600 +[simard::ooda] no-progress breaker: stuck after guided retry — BLOCKED WITH why + issue filed and linked (halted, escalated once) goal=4d27c91a +[simard::ooda] no-progress breaker: goal already escalated (sentinel-blocked) — skipping (escalated once) goal=4d27c91a +``` + +### Dedup after a re-orient / restart + +```text +tick N : file #4600 (marker ooda-signature:3f9a1c77b0e42d18) +--- daemon restart; wip_refs empty --- +tick N+1 : live wip_ref check MISSES (memory cleared) + remote search HITS #4600 → re-link, NO new issue +``` + +This is exactly the path that previously produced `#4499`/`#4504`/`#4508` and no +longer does. + +## Verifying the behavior + +Unit tests (inline `#[cfg(test)]` in `no_progress.rs` plus the +`tests_no_progress*` / `tests_no_progress_breaker` suites) assert: + +- **Dedup survives re-orient** — clearing `wip_refs` between ticks matches the + existing remote issue, not a second file. +- **Dedup survives restart** — a fresh filer whose remote list already contains + the marker re-links the existing issue instead of filing. +- **Distinct goals get distinct signatures** — two different `goal_id`s (e.g. the + 8-char-hex `4d27c91a` and `7f5afcca`) produce two *different* `ooda-signature` + markers, so a second stuck goal is never deduped against the first goal's issue. + (Regression guard for the `failure_signature`-redaction collision pitfall.) +- **Single escalation + skip** — a goal still Blocked after a guided retry + escalates once (recorded in `report.halted`) and later ticks are skipped while + it stands sentinel-Blocked. +- **Filer-failure path** — when `gh` fails, the filer returns `None`, logs at + `error`, and the cycle continues (goal stays Blocked). No panic (repairs + `#4472`/`#4474`). +- **Re-admit on unblock** — a still-blocked goal that an operator or the agentic + reasoner unblocks becomes re-orientable again; a re-stall re-escalates + idempotently against its existing `ooda-signature:` marker (no duplicate issue). + +Quality gates that must stay green: `scan_no_stray_prints` (structured tracing + +OpenTelemetry only) and `scan_no_bridge_naming`. + +## Why this is safe + +- **Additive / non-breaking.** New trait method, new signature helper, new report + field, and a status-based skip guard; existing filing behavior is preserved for + the common case. The PRD is unchanged. +- **Fail-closed dedup.** On any `gh` list error the breaker does *not* file a + duplicate; the goal simply stays Blocked, no worse than before the search + existed. +- **Bounded one-time migration cost.** Pre-existing unmarked issues cost at most + one extra "file-new-once" per goal, then are bounded by the marker. +- **No permanent silence.** The skip re-admits on a concrete state change (the + goal leaves sentinel-Blocked via operator unblock or agentic re-orient), so a + goal that becomes actionable is never stranded. +- **No command injection.** All `gh` invocations use `Command::new("gh").args([…])` + argv vectors — never `sh -c` or string-built commands. + +## See also + +- [Blocked-goal escalation backoff](./blocked-goal-escalation-backoff.md) — the + per-signature backoff this composes with. +- [No-progress breaker explains WHY and self-resolves](./no-progress-root-cause-resolution.md) +- [Terminal no-progress stall never parks empty evidence](./no-progress-terminal-investigation.md) +- [Keeping the OODA daemon steerable](./steerable-ooda-daemon.md) diff --git a/docs/concepts/steerable-ooda-daemon.md b/docs/concepts/steerable-ooda-daemon.md index cfba3e0e7..03c6fd03a 100644 --- a/docs/concepts/steerable-ooda-daemon.md +++ b/docs/concepts/steerable-ooda-daemon.md @@ -329,3 +329,4 @@ contract plus stricter contract validation, not a larger parser. - [How OODA spawns engineer agents](../howto/spawn-engineers-from-ooda-daemon.md) - [Simard CLI reference](../reference/simard-cli.md) - [OODA coverage parallelism ceiling](../reference/ooda-coverage-parallelism-ceiling.md) +- [No-progress breaker escalates once, then halts (livelock dedup)](./no-progress-livelock-dedup.md) diff --git a/docs/design/agentic-observe-orient-merge-queue.md b/docs/design/agentic-observe-orient-merge-queue.md index 710fe94b4..7ede75829 100644 --- a/docs/design/agentic-observe-orient-merge-queue.md +++ b/docs/design/agentic-observe-orient-merge-queue.md @@ -501,3 +501,4 @@ journalctl --user -u simard-ooda | grep 'merge reasoning DISABLED' - [Concept: autonomous self-merge sensor](../concepts/autonomous-self-merge-sensor.md) — the retired imperative sensor - [Reference: cross-repo merge authority](../reference/cross-repo-merge-authority.md) — the unchanged action gate - [How to triage stale pull requests](../howto/triage-stale-pull-requests.md) +- [Concept: Overseer escalates the deploy-gate-converging PR](../concepts/deploy-gate-converging-pr-escalation.md) — the DeployDrift-aware ranking that sits behind this re-narrowing diff --git a/mkdocs.yml b/mkdocs.yml index b8d2a8a85..7361fb52f 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -145,6 +145,8 @@ nav: - Overseer Goal-Board Health: concepts/overseer-goal-board-health.md - Stable Goal-Session Identity: concepts/stable-goal-session-identity.md - Blocked-Goal Escalation Backoff: concepts/blocked-goal-escalation-backoff.md + - No-Progress Breaker Escalates Once, Then Halts (livelock dedup): concepts/no-progress-livelock-dedup.md + - Overseer Escalates the Deploy-Gate-Converging PR: concepts/deploy-gate-converging-pr-escalation.md - PID-Reuse-Safe Subordinate Reaper: concepts/pid-reuse-safe-subordinate-reaper.md - Coverage Comment Transient Resilience: concepts/coverage-comment-transient-resilience.md - Overseer Root-Cause (WHY) Principle: concepts/overseer-root-cause-why.md diff --git a/src/ooda_loop/mod.rs b/src/ooda_loop/mod.rs index f9cf9e7fe..e959f6d92 100644 --- a/src/ooda_loop/mod.rs +++ b/src/ooda_loop/mod.rs @@ -39,6 +39,15 @@ mod tests_types; #[cfg(test)] mod tests_no_progress; +// Issue #4497 (TDD): the OODA no-progress / re-orientation LIVELOCK fix — a +// still-blocked goal must file at most one `ooda-stuck` tracking issue via a +// remote signature search-before-create that survives re-orient/restart, and +// must escalate exactly once then be skipped while it stands Blocked with the +// no-progress sentinel (no re-orientation, no duplicate issue) until its block +// is lifted (#4499/#4504/#4508/#4509/#4474/#4472). +#[cfg(test)] +mod tests_no_progress_livelock_dedup; + // Issue #16 (TDD): integration tests for the agentic root-cause upgrade — the // investigated adapter routes a stuck goal down the self-resolving ladder and // only ever authors a human block WITH the concrete why + evidence attached. diff --git a/src/ooda_loop/no_progress.rs b/src/ooda_loop/no_progress.rs index a9eed6369..569a9f4a1 100644 --- a/src/ooda_loop/no_progress.rs +++ b/src/ooda_loop/no_progress.rs @@ -23,7 +23,7 @@ use crate::goal_curation::completion_gate::{ }; use crate::goal_curation::no_progress_breaker::{ NO_PROGRESS_BREAKER_THRESHOLD, NoProgressResolution, NoProgressTracker, - SURFACED_INVESTIGATION_FAILURE_LIMIT, needs_reinvestigation, + SURFACED_INVESTIGATION_FAILURE_LIMIT, is_no_progress_marker, needs_reinvestigation, no_progress_blocked_reason_with_why, obsolescence_reason, resolution_for_why, surfaced_failure_escalation_issue, verify_stuck_goal, }; @@ -68,6 +68,30 @@ fn is_breaker_tracking_ref(wip: &WipRef) -> bool { && wip.label.starts_with(NO_PROGRESS_TRACKING_LABEL_PREFIX) } +/// True when `goal_id` currently stands **Blocked with the no-progress sentinel** +/// — i.e. the breaker has already escalated it and that block is still standing +/// (issue #4497). Such a goal is skipped by the breaker's outcome loop so it +/// escalates EXACTLY ONCE and does not re-fire (re-file a duplicate `ooda-stuck` +/// issue) every tick while its block stands. +/// +/// The guard is deliberately keyed on the LIVE block status, not a persisted +/// "halted forever" flag: when an operator OR the agentic reasoner +/// ([`ActiveGoal::roll_to_new_cycle`]) lifts the block (status → non-Blocked), +/// the goal becomes eligible again — but the remote signature dedup in +/// [`escalate_with_tracking_issue`] still prevents a duplicate tracking issue, so +/// a re-stall re-blocks idempotently instead of spamming. +fn goal_is_sentinel_blocked(state: &OodaState, goal_id: &str) -> bool { + state + .active_goals + .active + .iter() + .find(|g| g.id == goal_id) + .is_some_and(|g| match &g.status { + GoalProgress::Blocked(reason) => is_no_progress_marker(reason), + _ => false, + }) +} + /// A tracking issue the breaker successfully filed for an escalated goal. /// /// Returned by [`NoProgressIssueFiler::file_issue`] so the caller can link the @@ -83,6 +107,40 @@ pub(crate) struct FiledIssue { pub url: Option, } +/// The `ooda-signature:` body marker prefix the breaker embeds in every +/// `ooda-stuck` tracking issue it files, and greps for when searching before +/// creating (issue #4497). It is a single whitespace-delimited token so a plain +/// substring / token scan of an issue body finds it, mirroring the proven +/// `stewardship-signature:` convention in [`crate::stewardship::dedup`]. +const OODA_SIGNATURE_MARKER: &str = "ooda-signature:"; + +/// A stable, per-goal dedup key for the no-progress breaker's tracking issue +/// (issue #4497). It is the durable identity of "this goal's `ooda-stuck` +/// tracking issue": the first 8 bytes of `sha256("ooda-no-progress" || "\n" || +/// goal_id)` rendered as 16 lowercase hex chars. +/// +/// Reuses the sha256-prefix **shape** of +/// [`crate::stewardship::dedup::failure_signature`] (so the marker is +/// indistinguishable in form from the stewardship convention) but deliberately +/// hashes the goal id VERBATIM rather than through the volatile-token redactor: +/// a goal id is a stable identity, and folding an embedded UUID/hex run would +/// wrongly collapse two DISTINCT goals onto one tracking issue. The result is +/// therefore identical across re-orientation and process restart (deterministic +/// on `goal_id`) yet distinct per goal — exactly what a search-before-create +/// dedup key needs. +pub(crate) fn breaker_signature(goal_id: &str) -> String { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + hasher.update(b"ooda-no-progress\n"); + hasher.update(goal_id.trim().as_bytes()); + let digest = hasher.finalize(); + let mut out = String::with_capacity(16); + for b in &digest[..8] { + out.push_str(&format!("{b:02x}")); + } + out +} + /// Files a tracking issue for a goal the breaker escalated. Injected so tests /// exercise the escalation path without shelling out to `gh`. pub(crate) trait NoProgressIssueFiler { @@ -96,6 +154,26 @@ pub(crate) trait NoProgressIssueFiler { /// `None` simply means the goal stays Blocked without a linked artifact /// (no worse than before this linkage existed). fn file_issue(&self, title: &str, body: &str) -> Option; + + /// **Search before create** (issue #4497): return the OPEN `ooda-stuck` + /// tracking issue whose body embeds `ooda-signature:`, or `None` + /// when none is open (or the remote is unreachable). + /// + /// This is the dedup **source of truth**: the remote open-issue list, NOT the + /// volatile in-memory `wip_refs` link. Keying dedup on the remote (matched by + /// the goal's stable [`breaker_signature`]) is what stops a re-orientation + /// (which clears the in-memory link) or a restart from resurrecting the + /// duplicate-filing livelock. It MUST be read-only and **fail closed** — + /// return `None` on any `gh`/parse error so a transient outage never blocks + /// the cycle (the escalation still marks the goal Blocked; it simply files no + /// linked artifact, no worse than an outage of [`file_issue`] itself). + /// + /// The default returns `None` (no remote dedup) so a filer with no remote + /// store — every legacy test fake — keeps its prior create-always behaviour; + /// [`GhIssueFiler`] and the remote-modelling test fakes override it. + fn find_open_tracking_issue(&self, _signature: &str) -> Option { + None + } } /// Production filer: `gh issue create --label ooda-stuck`, mirroring the @@ -151,6 +229,84 @@ impl NoProgressIssueFiler for GhIssueFiler { } } } + + fn find_open_tracking_issue(&self, signature: &str) -> Option { + // Read-only search-before-create against the REMOTE open-issue list — the + // dedup source of truth (issue #4497). Fail closed: any spawn/exit/parse + // error returns `None` so a transient `gh` outage can never abort the + // cycle (the caller still Blocks the goal; it just files no linked issue). + let out = match std::process::Command::new("gh") + .args([ + "issue", + "list", + "--state", + "open", + "--label", + "ooda-stuck", + "--limit", + "200", + "--json", + "number,url,body", + ]) + .output() + { + Ok(out) if out.status.success() => out, + Ok(out) => { + tracing::error!( + target: "simard::ooda", + stderr = %String::from_utf8_lossy(&out.stderr), + "no-progress breaker: gh issue list failed during dedup search (failing closed)", + ); + return None; + } + Err(e) => { + tracing::error!( + target: "simard::ooda", + error = %e, + "no-progress breaker: gh spawn failed during dedup search (failing closed)", + ); + return None; + } + }; + + #[derive(serde::Deserialize)] + struct GhListedIssue { + number: u64, + #[serde(default)] + url: Option, + #[serde(default)] + body: String, + } + + let issues: Vec = match serde_json::from_slice(&out.stdout) { + Ok(issues) => issues, + Err(e) => { + tracing::error!( + target: "simard::ooda", + error = %e, + "no-progress breaker: could not parse gh issue list JSON during dedup (failing closed)", + ); + return None; + } + }; + + let needle = format!("{OODA_SIGNATURE_MARKER}{signature}"); + issues + .into_iter() + .find(|i| body_has_signature(&i.body, &needle)) + .map(|i| FiledIssue { + number: i.number.to_string(), + url: i.url, + }) + } +} + +/// True when `body` embeds the `ooda-signature:` `needle` as a whole +/// whitespace-delimited token (issue #4497). Token-level (not a bare +/// `contains`) so a signature can never be matched as a prefix of a longer +/// token, keeping dedup exact. +fn body_has_signature(body: &str, needle: &str) -> bool { + body.split_whitespace().any(|t| t == needle) } /// Parse the issue number from a `gh issue create` success line, which prints @@ -207,8 +363,11 @@ fn escalate_with_tracking_issue( issue_body: &str, filer: &dyn NoProgressIssueFiler, ) { - // Idempotence: never file a second tracking issue for a goal already linked - // to one (a re-stall must not spam duplicate `ooda-stuck` issues). + // The goal's durable dedup identity (stable across re-orient / restart). + let signature = breaker_signature(goal_id); + + // First-line idempotence: never re-file for a goal that already carries its + // breaker tracking ref in memory this cycle. let already_tracked = state .active_goals .active @@ -218,8 +377,25 @@ fn escalate_with_tracking_issue( let filed = if already_tracked { None + } else if let Some(existing) = filer.find_open_tracking_issue(&signature) { + // Search-before-create (issue #4497): a prior cycle or process already + // filed this goal's tracking issue. Re-link that standing issue instead + // of spamming a duplicate — this is what defeats the re-orient/restart + // livelock that clears the in-memory ref. + tracing::info!( + target: "simard::ooda", + goal = %goal_id, + issue = %existing.number, + signature = %signature, + "no-progress breaker: standing ooda-stuck issue found via signature — re-linking (no duplicate filed)", + ); + Some(existing) } else { - filer.file_issue(issue_title, issue_body) + // No standing issue: file one, embedding the `ooda-signature:` body + // marker so the next search-before-create (after a re-orient / restart) + // can find it and dedup. + let body = format!("{issue_body}\n\n{OODA_SIGNATURE_MARKER}{signature}"); + filer.file_issue(issue_title, &body) }; if let Some(g) = state @@ -291,6 +467,18 @@ pub(crate) struct NoProgressBreakerReport { /// SIGNAL. It stays **fail-closed**: the goal is never blocked/killed/parked, /// and this is deliberately NOT a [`fired`](Self::fired) firing. pub research_idle_faults: Vec, + /// Goals the breaker **terminally halted** this pass (issue #4497): escalated + /// to a human exactly once and recorded in the tracker's halt set so their + /// subsequent no-action cycles are skipped rather than re-escalated. This is + /// the fix for the observed livelock where a single still-blocked goal spammed + /// five near-duplicate `ooda-stuck` tracking issues in ~6h + /// (#4497/#4499/#4504/#4508/#4509) because each agentic re-orientation cleared + /// its in-memory tracking ref and the next threshold cycle re-filed. Recorded + /// on the SAME pass as the escalation (so it accompanies the `escalated` + /// entry); a goal already halted on entry is silently skipped and does NOT + /// re-appear here. Not itself a [`fired`](Self::fired) — the escalation that + /// accompanies it is. + pub halted: Vec, } impl NoProgressBreakerReport { @@ -312,7 +500,7 @@ impl NoProgressBreakerReport { format!( "done={} dropped={} escalated={} healed={} deferred={} engineer={} \ auto_cleared={} reinvestigated={} errors={} perpetual_idled={} \ - research_faults={}", + research_faults={} halted={}", self.marked_done.len(), self.dropped.len(), self.escalated.len(), @@ -324,6 +512,7 @@ impl NoProgressBreakerReport { self.investigation_errors.len(), self.perpetual_idled.len(), self.research_idle_faults.len(), + self.halted.len(), ) } @@ -744,6 +933,21 @@ pub(crate) fn apply_no_progress_breaker_with_threshold( continue; } + // Terminal-halt guard (issue #4497): a goal already escalated by the + // breaker still stands Blocked with the sentinel, so skip it entirely — + // no counter bump, no re-escalation, no duplicate tracking issue — while + // that block stands. This is what stops the observed re-fire livelock. An + // operator or the agentic reasoner lifting the block re-admits the goal; + // the remote signature dedup then keeps a re-stall from filing a duplicate. + if goal_is_sentinel_blocked(state, goal_id) { + tracing::debug!( + target: "simard::ooda", + goal = %goal_id, + "no-progress breaker: goal already escalated (sentinel-blocked) — skipping (escalated once)", + ); + continue; + } + // Standing/perpetual exemption (issue #2589) + never-idle research rail // (issue #4399), unified in `apply_standing_idle` (which calls the shared // pure `classify_standing_idle`) so the two breaker sites can never drift. @@ -814,10 +1018,14 @@ pub(crate) fn apply_no_progress_breaker_with_threshold( filer, ); report.escalated.push(goal_id.to_string()); + // Terminal halt (issue #4497): record the goal as escalated-once so + // the sentinel-block guard skips its later no-action cycles instead + // of re-firing every tick. + report.halted.push(goal_id.to_string()); tracing::warn!( target: "simard::ooda", goal = %goal_id, - "no-progress breaker: unresolved after threshold — BLOCKED + tracking issue filed and linked", + "no-progress breaker: unresolved after threshold — BLOCKED + tracking issue filed and linked (halted, escalated once)", ); } // The base breaker's ladder ([`resolve_no_progress`]) only yields the @@ -933,6 +1141,22 @@ pub(crate) fn apply_no_progress_breaker_investigated( continue; } + // Terminal-halt guard (issue #4497): a goal already escalated by the + // breaker still stands Blocked with the sentinel, so skip it — no counter + // bump, no re-investigation, no re-escalation — while that block stands. + // This stops the observed re-fire livelock in the production (investigated) + // path. An operator or the agentic reasoner lifting the block re-admits the + // goal; the remote signature dedup then keeps a re-stall from filing a + // duplicate tracking issue. + if goal_is_sentinel_blocked(state, goal_id) { + tracing::debug!( + target: "simard::ooda", + goal = %goal_id, + "no-progress breaker: goal already escalated (sentinel-blocked) — skipping (escalated once)", + ); + continue; + } + // Standing/perpetual exemption (issue #2589) + never-idle research rail // (issue #4399) run BEFORE investigation, via the SAME `apply_standing_idle` // helper as the non-investigated site so the reasoner is never consulted for @@ -1123,11 +1347,12 @@ fn apply_resolution_side_effects( ); tracker.reset_count(goal_id); report.escalated.push(goal_id.to_string()); + report.halted.push(goal_id.to_string()); tracing::error!( target: "simard::ooda", goal = %goal_id, error = %err, - "no-progress breaker: precondition heal FAILED — escalating WITH why", + "no-progress breaker: precondition heal FAILED — escalating WITH why (halted, escalated once)", ); } } @@ -1215,10 +1440,11 @@ fn apply_resolution_side_effects( ); tracker.reset_count(goal_id); report.escalated.push(goal_id.to_string()); + report.halted.push(goal_id.to_string()); tracing::warn!( target: "simard::ooda", goal = %goal_id, - "no-progress breaker: stuck after guided retry — BLOCKED WITH why + issue filed and linked", + "no-progress breaker: stuck after guided retry — BLOCKED WITH why + issue filed and linked (halted, escalated once)", ); } NoProgressResolution::SurfaceInvestigationFailure { class, reason } => { @@ -1257,6 +1483,7 @@ fn apply_resolution_side_effects( tracker.clear_surfaced_failures(goal_id); tracker.reset_count(goal_id); report.escalated.push(goal_id.to_string()); + report.halted.push(goal_id.to_string()); tracing::warn!( target: "simard::ooda", goal = %goal_id, diff --git a/src/ooda_loop/tests_no_progress_livelock_dedup.rs b/src/ooda_loop/tests_no_progress_livelock_dedup.rs new file mode 100644 index 000000000..2aae86eb9 --- /dev/null +++ b/src/ooda_loop/tests_no_progress_livelock_dedup.rs @@ -0,0 +1,413 @@ +//! TDD (Step 7, RED) — the OODA no-progress / re-orientation **livelock** fix +//! (issues #4497 / #4499 / #4504 / #4508 / #4509, plus the defective escalation +//! path #4474 / #4472). +//! +//! ROOT CAUSE these tests kill: the breaker dedups a still-blocked goal's +//! `ooda-stuck` tracking issue ONLY on in-memory `wip_refs` +//! ([`super::no_progress::is_breaker_tracking_ref`]). A re-orientation +//! ([`crate::goal_curation::ActiveGoal::roll_to_new_cycle`]) clears `wip_refs`, +//! so the very next threshold cycle sees "no tracked issue" and files a fresh +//! duplicate — the observed behaviour where ONE still-blocked goal spammed five +//! near-identical tracking issues (#4497/#4499/#4504/#4508/#4509) in ~6h while +//! never converging. +//! +//! TARGET contract (these tests reference API that does NOT exist yet, so the +//! crate test build FAILS to compile until the feature lands — that compile +//! failure IS the RED state of red→green→refactor): +//! +//! 1. `breaker_signature(goal_id)` — a deterministic, per-goal dedup key +//! (reusing the proven `stewardship::dedup::failure_signature` convention) +//! that is STABLE across re-orient and process restart, so it is the +//! durable identity of "this goal's ooda-stuck tracking issue". +//! 2. `NoProgressIssueFiler::find_open_tracking_issue(signature)` — a +//! read-only, fail-closed REMOTE search-before-create. The remote open-issue +//! list (not the volatile in-memory `wip_refs`) is the dedup source of +//! truth, so a re-orient/restart can never resurrect the duplicate-filing +//! livelock. +//! 3. `NoProgressBreakerReport.halted` — after a goal's guided retry is spent +//! and it is STILL blocked, the breaker escalates EXACTLY ONCE and then +//! terminally halts re-orientation for that goal (recorded here), instead of +//! re-firing/re-orienting every overseer tick. +//! +//! Everything is hermetic: an injected in-memory filer fake models the remote +//! issue store; no `gh`, no network. + +use std::cell::RefCell; + +use super::no_progress::{ + FiledIssue, NoProgressBreakerReport, NoProgressIssueFiler, + apply_no_progress_breaker_with_threshold, breaker_signature, +}; +use crate::error::SimardResult; +use crate::goal_curation::completion_gate::EvidenceSource; +use crate::goal_curation::no_progress_breaker::{ + NO_PROGRESS_BREAKER_THRESHOLD, is_no_progress_marker, +}; +use crate::goal_curation::{ActiveGoal, GoalBoard, GoalProgress, WipRef}; +use crate::ooda_loop::{ActionKind, ActionOutcome, OodaState, PlannedAction}; + +// ─────────────────────────── fixtures ────────────────────────────────────── + +/// Canned evidence source: no completion evidence at all, so a stalled goal is +/// genuinely stuck (the breaker cannot certify it done / obsolete). +struct NoEvidence; +impl EvidenceSource for NoEvidence { + fn any_pr_merged(&self, _goal: &ActiveGoal) -> SimardResult { + Ok(false) + } + fn issue_closed(&self, _goal: &ActiveGoal) -> SimardResult { + Ok(false) + } + fn is_deployed(&self, _goal: &ActiveGoal) -> SimardResult { + Ok(false) + } +} + +/// In-memory filer modelling the REMOTE `ooda-stuck` issue store. Every filed +/// issue is remembered under the `ooda-signature:` marker embedded in its +/// body, and [`find_open_tracking_issue`](NoProgressIssueFiler::find_open_tracking_issue) +/// searches that store — exactly the search-before-create the production +/// `GhIssueFiler` performs against `gh issue list`. This lets the test prove the +/// remote store (not in-memory `wip_refs`) is the dedup source of truth across a +/// re-orient / restart. +#[derive(Default)] +struct RemoteFiler { + /// Every `file_issue` call's (title, body) — length asserts "filed once". + files: RefCell>, + /// Every `find_open_tracking_issue` call's signature — proves search-first. + searches: RefCell>, + /// The remote store: signature → the issue standing open for it. + remote: RefCell>, + /// When true every remote op returns `None` (models a `gh` outage) so the + /// fail-closed path can be exercised. + offline: bool, +} + +impl RemoteFiler { + fn offline() -> Self { + Self { + offline: true, + ..Self::default() + } + } + + /// Seed the remote store with a pre-existing tracking issue for `signature`, + /// modelling a process that ALREADY filed the issue in a prior run (restart + /// dedup). + fn with_existing(signature: &str, number: &str) -> Self { + let f = Self::default(); + f.remote.borrow_mut().push(( + signature.to_string(), + FiledIssue { + number: number.to_string(), + url: None, + }, + )); + f + } +} + +impl NoProgressIssueFiler for RemoteFiler { + fn file_issue(&self, title: &str, body: &str) -> Option { + if self.offline { + return None; + } + self.files + .borrow_mut() + .push((title.to_string(), body.to_string())); + // The signature marker the breaker MUST embed so a later search can find + // this exact issue. The marker is `ooda-signature:` on its own body + // token — the same body-marker/search convention `stewardship` uses. + let signature = body + .split_whitespace() + .find_map(|t| t.strip_prefix("ooda-signature:")) + .expect("breaker must embed an `ooda-signature:` body marker so dedup can match") + .to_string(); + let number = format!("{}", 4500 + self.files.borrow().len()); + let filed = FiledIssue { + number: number.clone(), + url: None, + }; + self.remote.borrow_mut().push((signature, filed.clone())); + Some(filed) + } + + fn find_open_tracking_issue(&self, signature: &str) -> Option { + self.searches.borrow_mut().push(signature.to_string()); + if self.offline { + return None; + } + self.remote + .borrow() + .iter() + .find(|(sig, _)| sig == signature) + .map(|(_, filed)| filed.clone()) + } +} + +/// A stuck, self-affecting (repo=None) goal parked at 0% with one open, +/// never-merging PR so it is genuinely stuck (no derivable completion signal). +fn stuck_goal(id: &str) -> ActiveGoal { + let mut g = ActiveGoal::new(id, "converge the red-canary deploy gate", 1); + g.status = GoalProgress::NotStarted; + g.wip_refs = vec![WipRef { + kind: "pr".to_string(), + ref_id: "7".to_string(), + label: "PR #7".to_string(), + url: None, + }]; + g +} + +fn state_with(goal: ActiveGoal) -> OodaState { + let mut board = GoalBoard::new(); + board.active.push(goal); + OodaState::new(board) +} + +fn no_action_outcome(goal_id: &str) -> ActionOutcome { + ActionOutcome { + action: PlannedAction { + kind: ActionKind::AdvanceGoal, + goal_id: Some(goal_id.to_string()), + description: "advance".to_string(), + }, + success: true, + detail: format!("no-action: I'll verify concretely next cycle (goal '{goal_id}')"), + } +} + +/// Drive `goal_id` through exactly `threshold` no-action cycles so the breaker +/// fires once, returning the firing cycle's report. +fn drive_to_escalation( + state: &mut OodaState, + goal_id: &str, + evidence: &dyn EvidenceSource, + filer: &dyn NoProgressIssueFiler, + threshold: u32, +) -> NoProgressBreakerReport { + let mut last = NoProgressBreakerReport::default(); + for _ in 0..threshold { + last = apply_no_progress_breaker_with_threshold( + state, + &[no_action_outcome(goal_id)], + evidence, + filer, + threshold, + ); + } + last +} + +/// Simulate the per-goal re-orientation the agentic reasoner performs +/// (`ActiveGoal::roll_to_new_cycle`): drop the goal's tracked refs so the OLD +/// in-memory-only dedup is defeated. The remote store is untouched — that is the +/// whole point of the fix. +fn reorient(state: &mut OodaState, goal_id: &str) { + if let Some(g) = state + .active_goals + .active + .iter_mut() + .find(|g| g.id == goal_id) + { + g.wip_refs.clear(); + g.status = GoalProgress::NotStarted; + } +} + +// ─────────────────────────── tests ───────────────────────────────────────── + +#[test] +fn breaker_signature_is_deterministic_per_goal() { + // Same goal id ⇒ identical signature across independent calls (so a re-orient + // or a restart recomputes the SAME dedup key and matches the standing issue). + let a = breaker_signature("simard-identity-4d27c91a"); + let b = breaker_signature("simard-identity-4d27c91a"); + assert_eq!(a, b, "signature must be stable for a fixed goal id"); + assert!(!a.is_empty(), "signature must be non-empty"); +} + +#[test] +fn breaker_signature_is_distinct_across_goals() { + // Two different goals must never collide onto one tracking issue. + let a = breaker_signature("goal-4d27c91a"); + let b = breaker_signature("goal-7f5afcca"); + assert_ne!(a, b, "distinct goals must get distinct signatures"); +} + +#[test] +fn a_still_blocked_goal_files_at_most_one_issue_across_reorient() { + // THE #4497 livelock: escalate once → re-orient (clears wip_refs) → escalate + // again. The second escalation must find the standing remote issue via the + // signature and NOT file a duplicate, so `file_issue` is called exactly ONCE + // total even though the goal re-stalled after losing its in-memory link. + let threshold = NO_PROGRESS_BREAKER_THRESHOLD; + let filer = RemoteFiler::default(); + let evidence = NoEvidence; + let mut state = state_with(stuck_goal("simard-identity-4d27c91a")); + + let first = drive_to_escalation( + &mut state, + "simard-identity-4d27c91a", + &evidence, + &filer, + threshold, + ); + assert_eq!( + first.escalated, + vec!["simard-identity-4d27c91a".to_string()], + "the stuck goal escalates on the first threshold firing" + ); + assert_eq!( + filer.files.borrow().len(), + 1, + "the first escalation files exactly one ooda-stuck issue" + ); + match &state.active_goals.active[0].status { + GoalProgress::Blocked(reason) => assert!( + is_no_progress_marker(reason), + "escalated goal must carry the no-progress sentinel: {reason}" + ), + other => panic!("expected Blocked, got {other:?}"), + } + + // Re-orient wipes the in-memory tracking ref (the OLD dedup's only source). + reorient(&mut state, "simard-identity-4d27c91a"); + + // Re-stall to the threshold again. + let _ = drive_to_escalation( + &mut state, + "simard-identity-4d27c91a", + &evidence, + &filer, + threshold, + ); + + assert_eq!( + filer.files.borrow().len(), + 1, + "no duplicate issue: remote search-before-create dedups across re-orient (#4497)" + ); + assert!( + !filer.searches.borrow().is_empty(), + "the breaker must consult find_open_tracking_issue BEFORE filing" + ); + let expected_sig = breaker_signature("simard-identity-4d27c91a"); + assert!( + filer.searches.borrow().contains(&expected_sig), + "the remote search must key on the goal's stable breaker_signature" + ); +} + +#[test] +fn dedup_survives_process_restart() { + // A restart drops the in-memory tracker/state entirely. A fresh state whose + // goal re-stalls must still match the tracking issue a PRIOR process filed + // (seeded in the remote), proving the remote store — not in-process memory — + // is the dedup source of truth. + let threshold = NO_PROGRESS_BREAKER_THRESHOLD; + let signature = breaker_signature("simard-identity-7f5afcca"); + let filer = RemoteFiler::with_existing(&signature, "4497"); + let evidence = NoEvidence; + let mut fresh_state = state_with(stuck_goal("simard-identity-7f5afcca")); + + let _ = drive_to_escalation( + &mut fresh_state, + "simard-identity-7f5afcca", + &evidence, + &filer, + threshold, + ); + + assert!( + filer.files.borrow().is_empty(), + "a goal whose issue was filed by a prior process must NOT re-file after restart" + ); + assert!( + filer.searches.borrow().contains(&signature), + "restart dedup must go through the remote signature search" + ); +} + +#[test] +fn escalation_halts_reorientation_after_it_fires_once() { + // After the breaker escalates a still-blocked goal it must record the goal as + // terminally HALTED (escalate once, stop re-orienting) rather than re-firing + // every tick. Subsequent no-action ticks are no-ops: no new escalation, no + // second issue. + let threshold = NO_PROGRESS_BREAKER_THRESHOLD; + let filer = RemoteFiler::default(); + let evidence = NoEvidence; + let mut state = state_with(stuck_goal("simard-identity-4d27c91a")); + + let report = drive_to_escalation( + &mut state, + "simard-identity-4d27c91a", + &evidence, + &filer, + threshold, + ); + assert!( + report + .halted + .contains(&"simard-identity-4d27c91a".to_string()), + "a goal escalated after its guided retry must be marked halted (terminal), not re-orient forever" + ); + + // Keep ticking: a halted goal never produces a second escalation or issue. + for _ in 0..(threshold * 2) { + let r = apply_no_progress_breaker_with_threshold( + &mut state, + &[no_action_outcome("simard-identity-4d27c91a")], + &evidence, + &filer, + threshold, + ); + assert!( + r.escalated.is_empty(), + "a halted goal must not re-escalate on later ticks" + ); + } + assert_eq!( + filer.files.borrow().len(), + 1, + "a halted goal files exactly one tracking issue, ever" + ); +} + +#[test] +fn filer_outage_keeps_goal_blocked_and_never_aborts_the_cycle() { + // Repairs #4472/#4474: when the remote is unreachable (search AND create + // return None) the breaker must fail CLOSED — the goal stays Blocked with the + // sentinel and the cycle returns normally (no panic, no propagated error), + // rather than the escalation path blowing up or spamming. + let threshold = NO_PROGRESS_BREAKER_THRESHOLD; + let filer = RemoteFiler::offline(); + let evidence = NoEvidence; + let mut state = state_with(stuck_goal("simard-identity-4d27c91a")); + + let report = drive_to_escalation( + &mut state, + "simard-identity-4d27c91a", + &evidence, + &filer, + threshold, + ); + + assert_eq!( + report.escalated, + vec!["simard-identity-4d27c91a".to_string()], + "the goal is still escalated (blocked) even when the filer is offline" + ); + assert!( + filer.files.borrow().is_empty(), + "an offline filer files nothing (it returned None), yet must not abort the cycle" + ); + match &state.active_goals.active[0].status { + GoalProgress::Blocked(reason) => assert!( + is_no_progress_marker(reason), + "goal stays Blocked with the no-progress sentinel despite the filer outage: {reason}" + ), + other => panic!("expected Blocked, got {other:?}"), + } +} diff --git a/src/overseer/capabilities.rs b/src/overseer/capabilities.rs index 1441c9e61..2f671fccf 100644 --- a/src/overseer/capabilities.rs +++ b/src/overseer/capabilities.rs @@ -573,6 +573,7 @@ pub trait PrOps { &self, _reasoned: &[ReasonedPr], _overseer_login: &str, + _deploy_drift: Option<&DeployDriftObservation>, ) -> Vec { Vec::new() } diff --git a/src/overseer/config.rs b/src/overseer/config.rs index 19a45a3ff..08ca2c3e1 100644 --- a/src/overseer/config.rs +++ b/src/overseer/config.rs @@ -583,6 +583,23 @@ pub fn automerge_author() -> Option { /// (`not-simard-autonomous`, `simard-autonomous-ish`) through the gate. pub const SIMARD_ENGINEER_PR_LABEL: &str = "simard-autonomous"; +/// The EXPLICIT, whole-string objective anchor marking a Simard-origin PR as one +/// that **converges the active deploy gate** (issue #4505). When a red-canary / +/// DeployDrift blocker is active — the running binary is behind merged `main` +/// because the self-deploy canary is red — the overseer's escalation ranking +/// ([`prioritize_gate_converging_prs`](crate::overseer::prioritize_gate_converging_prs)) +/// surfaces the PR carrying this label FIRST, so a green, mergeable PR that fixes +/// the very gate the deploy keeps failing on is escalated-to-merge instead of +/// sitting idle behind ordinary ready PRs. +/// +/// Matched EXACTLY (whole-string, case-sensitive) by [`is_converges_gate_label`]: +/// a title/branch/author heuristic (or a loose substring match) could be spoofed +/// by free text, so ONLY this durable label promotes a PR. Distinct from +/// [`SIMARD_ENGINEER_PR_LABEL`] — a PR must still prove Simard-origin AND pass the +/// full six-criteria merge-authority gate; this label only re-orders the +/// already-authorized set. +pub const CONVERGES_GATE_PR_LABEL: &str = "converges-gate"; + /// The Rust-deterministic, engineer-EXCLUSIVE head-branch namespaces. These are /// code-generated and NEVER hand-typed by a human operator, so a head branch /// under one of them is proof-of-Simard-origin. They are the SECONDARY @@ -606,6 +623,14 @@ pub fn is_engineer_pr_label(label: &str) -> bool { label == SIMARD_ENGINEER_PR_LABEL } +/// True iff `label` is EXACTLY the deploy-gate-converging marker +/// [`CONVERGES_GATE_PR_LABEL`] (issue #4505). Whole-string, case-sensitive — a +/// substring or loose match would let a spoofed look-alike promote a PR ahead of +/// the merge queue under active DeployDrift. +pub fn is_converges_gate_label(label: &str) -> bool { + label == CONVERGES_GATE_PR_LABEL +} + /// True iff `head` rides a Rust-deterministic, engineer-only branch namespace /// (see [`ENGINEER_BRANCH_PREFIXES`]). Anchored with `starts_with` so a /// look-alike like `engineerish/…` never matches, and — because every prefix is diff --git a/src/overseer/merge_ops.rs b/src/overseer/merge_ops.rs index b774abad2..94154cec9 100644 --- a/src/overseer/merge_ops.rs +++ b/src/overseer/merge_ops.rs @@ -633,9 +633,12 @@ impl PrOps for MergePrOps { &self, reasoned: &[crate::overseer::capabilities::ReasonedPr], overseer_login: &str, + deploy_drift: Option<&crate::overseer::capabilities::DeployDriftObservation>, ) -> Vec { use crate::overseer::capabilities::PrDisposition; - use crate::overseer::{ProjectionCandidate, project_ready_prs}; + use crate::overseer::{ + ProjectionCandidate, prioritize_gate_converging_prs, project_ready_prs, + }; use std::collections::BTreeMap; let ready: Vec<&crate::overseer::capabilities::ReasonedPr> = reasoned @@ -686,7 +689,12 @@ impl PrOps for MergePrOps { snapshot: summary.to_snapshot(), }); } - project_ready_prs(&candidates, &self.base_allowlist, overseer_login) + let authorized = project_ready_prs(&candidates, &self.base_allowlist, overseer_login); + // Escalation ranking (issue #4505): under an ACTIVE DeployDrift blocker, + // surface a deploy-gate-converging PR FIRST so it is escalated-to-merge + // instead of sitting behind ordinary ready PRs. Pure re-ordering of the + // already-authorized set — never widens authority. + prioritize_gate_converging_prs(&authorized, &candidates, deploy_drift) } } diff --git a/src/overseer/mod.rs b/src/overseer/mod.rs index 90bfe15c1..596185871 100644 --- a/src/overseer/mod.rs +++ b/src/overseer/mod.rs @@ -76,6 +76,8 @@ pub mod wiring; #[cfg(test)] mod tests_deploy_drift; #[cfg(test)] +mod tests_deploy_gate_escalation; +#[cfg(test)] mod tests_diagnosis; #[cfg(test)] mod tests_escalation_triage; @@ -1241,10 +1243,11 @@ impl Overseer { // deterministic survey), the ONLY thing that drives `PrReadyToMerge` into // the gated merge chain. An agent can never widen merge authorization. let overseer_login = self.recursion.author_login.clone(); - let projected = self - .caps - .prs - .project_reasoned_ready_prs(&observed.reasoned_prs, &overseer_login); + let projected = self.caps.prs.project_reasoned_ready_prs( + &observed.reasoned_prs, + &overseer_login, + observed.deploy_drift.as_ref(), + ); for pr in projected { if !observed.ready_prs.contains(&pr) { observed.ready_prs.push(pr); @@ -2781,6 +2784,63 @@ pub fn project_ready_prs( .collect() } +/// Re-rank the ALREADY-AUTHORIZED merge set so a PR that converges the active +/// deploy gate is surfaced FIRST (issue #4505). This is the fix for the observed +/// escalation gap: while a red-canary / DeployDrift blocker is active (the running +/// binary is behind merged `main` because the self-deploy canary is red), the +/// overseer keeps failing on the very gate that a green, mergeable, non-draft PR +/// (e.g. #4505) would converge — yet that PR sat idle for hours because the +/// escalation candidate set treated it as just another ready PR. +/// +/// Contract (deliberately narrow so it can NEVER widen merge authority): +/// * **Set-preserving permutation.** The result is a re-ordering of `authorized` +/// ONLY — same multiset in, same multiset out. It can never add, remove, or +/// fabricate authorization for any PR; the six-criteria merge-authority gate +/// still runs downstream unchanged. An `authorized` entry with no matching +/// `candidate` is preserved in place. +/// * **Scoped to an active blocker.** With `deploy_drift == None` the function is +/// the identity — no gate to converge means no promotion, even for a PR that +/// carries the label. +/// * **Explicit label only.** A PR is treated as gate-converging iff its +/// [`ProjectionCandidate::snapshot`] carries the whole-string +/// [`CONVERGES_GATE_PR_LABEL`](crate::overseer::config::CONVERGES_GATE_PR_LABEL); +/// title/branch/author heuristics never promote a PR. +/// * **Never injects an unauthorized PR.** Only members of `authorized` appear in +/// the output — a converging candidate absent from `authorized` (it failed the +/// projection) is never pulled in. Authorization stays owned by +/// [`project_ready_prs`]. +/// +/// The promotion is a STABLE partition: converging PRs keep their relative order +/// and precede the non-converging PRs, which also keep theirs. +pub fn prioritize_gate_converging_prs( + authorized: &[PrRef], + candidates: &[ProjectionCandidate], + deploy_drift: Option<&capabilities::DeployDriftObservation>, +) -> Vec { + // Scoped to an active DeployDrift blocker: no drift ⇒ identity (no promotion). + if deploy_drift.is_none() { + return authorized.to_vec(); + } + + let converges = |pr: &PrRef| -> bool { + candidates.iter().any(|c| { + c.reasoned.repo == pr.repo + && c.reasoned.pr == pr.pr + && c.snapshot + .labels + .iter() + .any(|l| config::is_converges_gate_label(l)) + }) + }; + + // Stable partition: gate-converging first (original order), then the rest + // (original order). A pure re-ordering of the authorized set — never a widen. + let mut ranked: Vec = Vec::with_capacity(authorized.len()); + ranked.extend(authorized.iter().filter(|pr| converges(pr)).cloned()); + ranked.extend(authorized.iter().filter(|pr| !converges(pr)).cloned()); + ranked +} + /// Decide: choose one `Intervention` for a `Problem`. Illustrative routing; a /// production Overseer would use a prompt-driven reasoner with this deterministic /// mapping as its floor (mirroring `OodaDecideBrain`'s deterministic fallback). @@ -4442,6 +4502,7 @@ mod tests { &self, _reasoned: &[capabilities::ReasonedPr], _overseer_login: &str, + _deploy_drift: Option<&capabilities::DeployDriftObservation>, ) -> Vec { self.projected.clone() } diff --git a/src/overseer/tests_deploy_gate_escalation.rs b/src/overseer/tests_deploy_gate_escalation.rs new file mode 100644 index 000000000..544a0e271 --- /dev/null +++ b/src/overseer/tests_deploy_gate_escalation.rs @@ -0,0 +1,228 @@ +//! TDD (Step 7, RED) — the overseer **verify-and-merge escalation gap** for a +//! deploy-gate-converging PR (issue #4505 / DeployDrift / red-canary). +//! +//! ROOT CAUSE these tests kill: while a red-canary / DeployDrift blocker is +//! active (the running binary is behind merged `main` because the self-deploy +//! canary is red), the overseer keeps failing on the very gate that a green, +//! MERGEABLE, non-draft PR (#4505) would converge — yet that PR is never +//! surfaced/prioritised for escalation. The escalation candidate set treats it +//! as just another ready PR, so #4440/#4398 are escalated while the PR that +//! actually unblocks the deploy sits idle for hours. +//! +//! TARGET contract (references API that does NOT exist yet ⇒ the crate test +//! build FAILS to compile until the feature lands — the RED state): +//! +//! * `prioritize_gate_converging_prs(authorized, candidates, deploy_drift)` — +//! a PURE, SET-PRESERVING permutation of the already-authorized `ready_prs` +//! that, WHEN DeployDrift is active, ranks the PR that converges the active +//! deploy gate FIRST (so Decide surfaces it as a `VerifyAndMergePr` candidate +//! alongside #4440/#4398). It is a re-ordering ONLY: it can never add, +//! remove, or fabricate authorization for any PR — the six-criteria +//! merge-authority gate still runs downstream unchanged. +//! * `config::CONVERGES_GATE_PR_LABEL` (`"converges-gate"`) — the EXPLICIT, +//! whole-string objective anchor that marks a Simard-origin PR as converging +//! the deploy gate. A label (not a title/branch heuristic and not a per-PR +//! head-SHA match, which `PrSnapshot` does not carry) is required so the +//! ranking can never be spoofed by free text. +//! +//! Pure-function tests: no fakes, no network — the whole point of keeping the +//! ranking a deterministic rail beside `project_ready_prs`. + +use crate::overseer::capabilities::{DeployDriftObservation, PrDisposition, PrRef, ReasonedPr}; +use crate::overseer::config::{CONVERGES_GATE_PR_LABEL, SIMARD_ENGINEER_PR_LABEL}; +use crate::overseer::{ProjectionCandidate, prioritize_gate_converging_prs}; +use crate::stewardship::PrSnapshot; +use crate::stewardship::merge_authority::CheckRollupEntry; + +// ─────────────────────────── builders ────────────────────────────────────── + +/// A green, MERGEABLE, non-draft engineer snapshot carrying `labels`. +fn green_snapshot(labels: Vec) -> PrSnapshot { + PrSnapshot { + body: String::new(), + mergeable: "MERGEABLE".to_string(), + review_decision: "APPROVED".to_string(), + checks: vec![CheckRollupEntry { + name: "ci".to_string(), + state: "SUCCESS".to_string(), + }], + base_ref_name: "main".to_string(), + labels, + } +} + +fn candidate(repo: &str, pr: u32, labels: Vec) -> ProjectionCandidate { + ProjectionCandidate { + reasoned: ReasonedPr { + repo: repo.to_string(), + pr, + disposition: PrDisposition::ReadyForMerge, + rationale: "r".to_string(), + duplicate_of: None, + }, + author_login: "rysweet".to_string(), + head_ref: format!("engineer/{pr}-abcdef"), + is_draft: Some(false), + snapshot: green_snapshot(labels), + } +} + +/// An engineer PR that ALSO carries the `converges-gate` anchor. +fn gate_converging_candidate(repo: &str, pr: u32) -> ProjectionCandidate { + candidate( + repo, + pr, + vec![ + SIMARD_ENGINEER_PR_LABEL.to_string(), + CONVERGES_GATE_PR_LABEL.to_string(), + ], + ) +} + +/// An ordinary engineer PR (Simard-origin) that does NOT converge the gate. +fn ordinary_candidate(repo: &str, pr: u32) -> ProjectionCandidate { + candidate(repo, pr, vec![SIMARD_ENGINEER_PR_LABEL.to_string()]) +} + +fn pr_ref(repo: &str, pr: u32) -> PrRef { + PrRef { + repo: repo.to_string(), + pr, + } +} + +fn active_drift() -> DeployDriftObservation { + DeployDriftObservation { + target_commit: "deadbeefcafe".to_string(), + behind_commits: 1, + } +} + +/// Multiset equality independent of order — the "set-preserving permutation" +/// invariant (no PR added, removed, or duplicated). +fn same_set(a: &[PrRef], b: &[PrRef]) -> bool { + let mut a: Vec<_> = a.to_vec(); + let mut b: Vec<_> = b.to_vec(); + a.sort_by(|x, y| (x.repo.as_str(), x.pr).cmp(&(y.repo.as_str(), y.pr))); + b.sort_by(|x, y| (x.repo.as_str(), x.pr).cmp(&(y.repo.as_str(), y.pr))); + a == b +} + +// ─────────────────────────── tests ───────────────────────────────────────── + +const REPO: &str = "rysweet/Simard"; + +#[test] +fn deploy_gate_converging_pr_is_ranked_first_under_active_drift() { + // #4505 converges the gate; #4440/#4398 are ordinary ready PRs. With + // DeployDrift active, the converging PR must come FIRST so it is surfaced as + // an escalate-to-merge candidate alongside the others. + let authorized = vec![pr_ref(REPO, 4440), pr_ref(REPO, 4398), pr_ref(REPO, 4505)]; + let candidates = vec![ + ordinary_candidate(REPO, 4440), + ordinary_candidate(REPO, 4398), + gate_converging_candidate(REPO, 4505), + ]; + + let ranked = prioritize_gate_converging_prs(&authorized, &candidates, Some(&active_drift())); + + assert_eq!( + ranked.first(), + Some(&pr_ref(REPO, 4505)), + "the deploy-gate-converging PR (#4505) must be ranked first under active DeployDrift" + ); + assert_eq!( + &ranked[1..], + &[pr_ref(REPO, 4440), pr_ref(REPO, 4398)], + "non-converging PRs keep their original relative order (stable partition)" + ); +} + +#[test] +fn ranking_is_a_set_preserving_permutation_no_authority_widening() { + // The ranking is a re-ordering ONLY: identical multiset in, identical multiset + // out. It can NEVER introduce or drop an authorization. + let authorized = vec![pr_ref(REPO, 4440), pr_ref(REPO, 4398), pr_ref(REPO, 4505)]; + let candidates = vec![ + ordinary_candidate(REPO, 4440), + ordinary_candidate(REPO, 4398), + gate_converging_candidate(REPO, 4505), + ]; + + let ranked = prioritize_gate_converging_prs(&authorized, &candidates, Some(&active_drift())); + + assert_eq!( + ranked.len(), + authorized.len(), + "ranking must not change the number of authorized PRs" + ); + assert!( + same_set(&ranked, &authorized), + "ranking must be a permutation of the authorized set (no widening, no drop)" + ); +} + +#[test] +fn no_drift_leaves_order_unchanged_even_with_a_converging_label() { + // The gate-first promotion is scoped to an ACTIVE blocker. With no + // DeployDrift, the helper is the identity even if a PR carries the + // converges-gate label — ranking never re-orders when there is no gate to + // converge. + let authorized = vec![pr_ref(REPO, 4440), pr_ref(REPO, 4505)]; + let candidates = vec![ + ordinary_candidate(REPO, 4440), + gate_converging_candidate(REPO, 4505), + ]; + + let ranked = prioritize_gate_converging_prs(&authorized, &candidates, None); + + assert_eq!( + ranked, authorized, + "with no active DeployDrift the ranking is the identity (order preserved)" + ); +} + +#[test] +fn without_the_converges_gate_label_no_pr_is_promoted() { + // Safety: a PR is treated as gate-converging ONLY via the explicit + // whole-string `converges-gate` label. With drift active but NO candidate + // carrying the label, nothing is promoted — title/branch/author heuristics + // alone must never fabricate a gate-converging ranking. + let authorized = vec![pr_ref(REPO, 4440), pr_ref(REPO, 4398)]; + let candidates = vec![ + ordinary_candidate(REPO, 4440), + ordinary_candidate(REPO, 4398), + ]; + + let ranked = prioritize_gate_converging_prs(&authorized, &candidates, Some(&active_drift())); + + assert_eq!( + ranked, authorized, + "no converges-gate label ⇒ order is unchanged (no heuristic promotion)" + ); +} + +#[test] +fn a_converging_candidate_not_in_the_authorized_set_is_never_injected() { + // The helper reorders ONLY the already-authorized set. A converging candidate + // that failed the authorization projection (absent from `authorized`) must + // never be pulled into the result — authorization stays owned by + // `project_ready_prs`. + let authorized = vec![pr_ref(REPO, 4440)]; + let candidates = vec![ + ordinary_candidate(REPO, 4440), + // #4505 converges the gate but is NOT authorized this cycle. + gate_converging_candidate(REPO, 4505), + ]; + + let ranked = prioritize_gate_converging_prs(&authorized, &candidates, Some(&active_drift())); + + assert_eq!( + ranked, authorized, + "an unauthorized converging PR must never be injected by the ranking" + ); + assert!( + !ranked.contains(&pr_ref(REPO, 4505)), + "authorization is owned by project_ready_prs, not by the gate ranking" + ); +} From 23c582e695a40443125e3bf91f087d2f8109090a Mon Sep 17 00:00:00 2001 From: rysweet Date: Fri, 24 Jul 2026 05:31:42 +0000 Subject: [PATCH 2/2] fix(ooda): preserve breaker tracking ref across roll_to_new_cycle (#4509) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the ratified in-memory complement to the remote signature dedup (Step 16 code/philosophy review, PR #4515 blocker B1). `roll_to_new_cycle` previously wiped ALL wip_refs, so the breaker's tracking-issue ref was lost on every in-process re-orient and duplicate dedup relied solely on the remote `gh` search-before-create (fails during a `gh` outage). Now it preserves exactly the tracking-issue ref — a durable `issue` RECORD (not-live per `has_live_in_flight_ref`, so it neither suppresses the never-idle fault nor admits an overlapping engineer) — so an in-process re-orient dedups IO-free from memory. The remote signature search remains the fallback for a true process restart (in-memory state genuinely gone). Belt-and-suspenders, not a replacement. - Centralise `NO_PROGRESS_TRACKING_LABEL_PREFIX` + add `WipRef::is_no_progress_tracking` beside `WipRef` (single home shared with `no_progress::is_breaker_tracking_ref`; no duplicated magic string). - `roll_to_new_cycle` retains the tracking ref; drops all live refs as before. - Tests: `roll_to_new_cycle_preserves_breaker_tracking_ref_but_drops_live_refs` (types), `roll_to_new_cycle_preserves_in_memory_dedup_across_reorient` (livelock suite: proves the remote is NOT consulted after a real roll). - Docs: update no-progress-livelock-dedup concept to describe the dual (in-memory-preserve + remote-durable) mechanism. cargo fmt + clippy --all-targets -D warnings clean; goal_curation/ooda_loop/ ooda_brain suites green. Refs #4509 #4497 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/concepts/no-progress-livelock-dedup.md | 64 ++++++++--- src/goal_curation/mod.rs | 1 + src/goal_curation/types.rs | 106 ++++++++++++++++-- src/ooda_loop/no_progress.rs | 15 ++- .../tests_no_progress_livelock_dedup.rs | 88 ++++++++++++++- 5 files changed, 240 insertions(+), 34 deletions(-) diff --git a/docs/concepts/no-progress-livelock-dedup.md b/docs/concepts/no-progress-livelock-dedup.md index 2819726a4..0088973c2 100644 --- a/docs/concepts/no-progress-livelock-dedup.md +++ b/docs/concepts/no-progress-livelock-dedup.md @@ -5,8 +5,9 @@ description: > `ooda-stuck` tracking issues for a still-blocked goal. Dedup is now sourced from a remote, restart-durable signature (`ooda-signature:` embedded in the issue body, computed per-goal over the un-redacted `goal_id` via a dedicated - `no_progress::breaker_signature` helper) rather than - the volatile in-memory `wip_refs` that `roll_to_new_cycle` clears on every re-orient. + `no_progress::breaker_signature` helper), backed by an IO-free complement — the + breaker's tracking-issue `wip_ref` is now PRESERVED across `roll_to_new_cycle` + (issue #4509), so an in-process re-orient dedups from memory without a `gh` call. After a guided retry still leaves the goal Blocked, the breaker escalates exactly once and the goal — still standing Blocked with the no-progress sentinel — is skipped on subsequent ticks (no re-orientation, no duplicate issue) until its block is lifted, repairing the @@ -57,12 +58,20 @@ The root cause was two-fold: 1. **Dedup was in-memory only.** `escalate_with_tracking_issue` deduped by scanning the goal's in-memory `wip_refs` for a breaker-authored tracking ref - (`is_breaker_tracking_ref`). But `roll_to_new_cycle` **clears `wip_refs`** on + (`is_breaker_tracking_ref`). But `roll_to_new_cycle` **cleared `wip_refs`** on every re-orient, so the dedup memory was wiped each cycle. The next tick saw a goal with empty `wip_refs`, concluded "no tracking issue yet", and filed another one. Restarts had the same effect. The remote issue list — the actual source of truth — was never consulted. + The fix attacks this on **two** independent fronts (issue #4509): the remote + signature search below makes dedup durable even when the in-memory link is + genuinely gone (process restart), **and** `roll_to_new_cycle` now + *preserves* the breaker's tracking-issue ref (a durable `issue` RECORD, not + live work — see `WipRef::is_no_progress_tracking` in `src/goal_curation/types.rs`) so an **in-process** + re-orient keeps deduping from memory, IO-free, with no dependence on `gh` + availability. + 2. **No terminal boundary.** After a guided retry still left a goal Blocked, the breaker kept **re-orienting the same goal every tick** with no backoff and no terminal state, so it re-entered the escalation path indefinitely. @@ -73,10 +82,11 @@ self-inflicted denial-of-service on the issue tracker. ## The fix, in one sentence -Make dedup **remote and restart-durable** (a signature the issue body carries), -and make a post-guided-retry stall **skip**: escalate exactly once, then skip -that goal while it stands Blocked with the no-progress sentinel — until its block -is lifted. +Make dedup **remote and restart-durable** (a signature the issue body carries) +**and preserve the breaker's tracking-issue ref across `roll_to_new_cycle`** so +an in-process re-orient dedups IO-free from memory, and make a post-guided-retry +stall **skip**: escalate exactly once, then skip that goal while it stands +Blocked with the no-progress sentinel — until its block is lifted. ## Part 1 — remote, restart-durable dedup @@ -159,10 +169,13 @@ aborts the cycle** (this is the direct repair of `#4472` / `#4474`). **≤ 1 open `ooda-stuck` issue per blocked goal**: 1. **Live `wip_ref` check** — if the current in-memory goal still carries a - breaker-authored tracking ref, reuse it. Cheap fast path. + breaker-authored tracking ref, reuse it (no remote call). Because + `roll_to_new_cycle` now **preserves** that ref (issue #4509), this fast path + survives an in-process re-orient — the remote search is only needed after a + true process restart, when in-memory state is genuinely gone. 2. **Remote signature search** — call `find_open_tracking_issue(signature)`. If a matching open issue exists, re-link it to the goal and file nothing. This - survives the `wip_refs` wipe and process restarts. + survives process restarts (and any path where the in-memory ref was lost). 3. **File** — only when both of the above miss, `file_issue` creates one and embeds the `ooda-signature:` marker; the result is linked back to the goal via `link_tracking_issue`. @@ -221,10 +234,11 @@ Sentinel-Blocked **Why status-based, not a persisted flag.** Keying on the live block status means the skip lifts automatically the instant the goal is unblocked — whether an operator re-scopes it or the agentic reasoner re-orients it — with no separate -"clear the halt" bookkeeping to get wrong. The remote signature dedup -(Part 1) is what still prevents a *re-stall* from filing a **duplicate** issue: -an unblocked-then-re-stalled goal re-escalates idempotently against its existing -`ooda-signature:` marker. +"clear the halt" bookkeeping to get wrong. Dedup (Part 1) is what still prevents +a *re-stall* from filing a **duplicate** issue: the preserved in-memory tracking +ref covers an in-process re-orient IO-free, and the remote `ooda-signature:` +marker covers a restart — so an unblocked-then-re-stalled goal re-escalates +idempotently either way. Key properties: @@ -254,6 +268,7 @@ Key properties: | `NoProgressBreakerReport.halted` | `src/ooda_loop/no_progress.rs` | Goal IDs escalated-once-and-skipped this tick. | | `goal_is_sentinel_blocked` | `src/ooda_loop/no_progress.rs` | Skip guard: `continue`s past a goal still standing Blocked with the no-progress sentinel, before any escalation. | | `is_no_progress_marker` | `src/goal_curation/no_progress_breaker.rs` | Recognizes the breaker's own Blocked-reason sentinel that the skip guard keys on. | +| `WipRef::is_no_progress_tracking` | `src/goal_curation/types.rs` | Marks the breaker's tracking-issue ref; `roll_to_new_cycle` **preserves** exactly this ref so in-memory dedup survives an in-process re-orient (IO-free, `gh`-independent). Prefix constant `NO_PROGRESS_TRACKING_LABEL_PREFIX` lives here (single home shared with `no_progress.rs`). | ## Configuration @@ -291,21 +306,34 @@ Expected log lines (structured tracing; no `print!`/`println!`): ```text tick N : file #4600 (marker ooda-signature:3f9a1c77b0e42d18) +--- in-process re-orient: roll_to_new_cycle PRESERVES the tracking ref --- +tick N+1 : live wip_ref check HITS (ref survived the roll) → re-link + in memory, NO remote call, NO new issue --- daemon restart; wip_refs empty --- -tick N+1 : live wip_ref check MISSES (memory cleared) +tick N+2 : live wip_ref check MISSES (memory cleared by restart) remote search HITS #4600 → re-link, NO new issue ``` This is exactly the path that previously produced `#4499`/`#4504`/`#4508` and no -longer does. +longer does. Note the two distinct dedup sources: an **in-process** re-orient is +handled IO-free by the preserved in-memory ref (issue #4509); a **restart** — the +only path that genuinely loses the in-memory ref — falls back to the remote +signature search. ## Verifying the behavior -Unit tests (inline `#[cfg(test)]` in `no_progress.rs` plus the +Unit tests (inline `#[cfg(test)]` in `no_progress.rs` and `types.rs` plus the `tests_no_progress*` / `tests_no_progress_breaker` suites) assert: -- **Dedup survives re-orient** — clearing `wip_refs` between ticks matches the - existing remote issue, not a second file. +- **Dedup survives an in-process re-orient (in-memory)** — the real + `roll_to_new_cycle` preserves the breaker tracking ref, so a re-stall dedups + from memory **without** consulting the remote + (`roll_to_new_cycle_preserves_in_memory_dedup_across_reorient`, plus + `roll_to_new_cycle_preserves_breaker_tracking_ref_but_drops_live_refs` in + `types.rs`). +- **Dedup survives loss of the in-memory link (remote)** — when `wip_refs` are + gone (process restart), the remote signature search matches the existing issue, + not a second file. - **Dedup survives restart** — a fresh filer whose remote list already contains the marker re-links the existing issue instead of filing. - **Distinct goals get distinct signatures** — two different `goal_id`s (e.g. the diff --git a/src/goal_curation/mod.rs b/src/goal_curation/mod.rs index db1b3baba..255245b54 100644 --- a/src/goal_curation/mod.rs +++ b/src/goal_curation/mod.rs @@ -34,6 +34,7 @@ pub use operations::{ seed_board_from_seed_goals, seed_default_board, simard_state_root, update_goal_progress, update_goal_progress_with_evidence, verify_goal_carryover, write_goal_carryover, }; +pub(crate) use types::NO_PROGRESS_TRACKING_LABEL_PREFIX; pub use types::{ ActiveGoal, BacklogItem, CARRYOVER_CONCEPT, GoalBoard, GoalCarryoverRecord, GoalEdge, GoalEdgeType, GoalNode, GoalProgress, MAX_ACTIVE_GOALS, STANDING_MARKER_PREFIX, WipRef, diff --git a/src/goal_curation/types.rs b/src/goal_curation/types.rs index aa335ab0d..b5f381f0e 100644 --- a/src/goal_curation/types.rs +++ b/src/goal_curation/types.rs @@ -153,6 +153,15 @@ fn contains_phrase_on_word_boundary(haystack_lower: &str, phrase: &str) -> bool false } +/// Label prefix on the no-progress breaker's tracking-issue [`WipRef`] — the +/// link an escalated goal carries to its `ooda-stuck` tracking issue (issue +/// #4497/#4509). Centralised here beside [`WipRef`] (rather than living only in +/// [`crate::ooda_loop::no_progress`]) so [`ActiveGoal::roll_to_new_cycle`] can +/// PRESERVE this one ref across a re-orient/roll without depending on the +/// breaker module — the durable, IO-free half of the breaker's duplicate-issue +/// dedup. Both sides MUST agree on this exact prefix, so it has a single home. +pub(crate) const NO_PROGRESS_TRACKING_LABEL_PREFIX: &str = "[no-progress-tracking] "; + /// A reference to work-in-progress associated with a goal. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct WipRef { @@ -167,6 +176,23 @@ pub struct WipRef { pub url: Option, } +impl WipRef { + /// True when this is the no-progress breaker's tracking-issue link (an + /// `issue` ref whose label carries [`NO_PROGRESS_TRACKING_LABEL_PREFIX`]). + /// + /// This is the one `wip_ref` [`ActiveGoal::roll_to_new_cycle`] deliberately + /// keeps: it is a durable RECORD, not live in-flight work + /// ([`ActiveGoal::has_live_in_flight_ref`] treats `issue` refs as not-live), + /// so preserving it neither suppresses the never-idle fault nor spawns an + /// overlapping engineer — but it lets the breaker's in-memory + /// `already_tracked` fast-path dedup survive a re-orient/roll IO-free, + /// instead of relying on the remote `gh` search-before-create. + pub(crate) fn is_no_progress_tracking(&self) -> bool { + self.kind.eq_ignore_ascii_case("issue") + && self.label.starts_with(NO_PROGRESS_TRACKING_LABEL_PREFIX) + } +} + /// An active goal on the board. Active goals are limited to `MAX_ACTIVE_GOALS`. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct ActiveGoal { @@ -357,14 +383,15 @@ impl ActiveGoal { /// Roll a standing/perpetual goal into a fresh cycle after its current unit /// of work finishes, instead of terminating it (issue #2580). Resets the /// goal to an actionable, re-dispatchable state: status back to - /// `NotStarted`, assignment cleared, and stale work-in-progress refs + /// `NotStarted`, assignment cleared, and stale live work-in-progress refs /// dropped so the next OODA cycle re-enters the spawn path. The /// standing-goal description marker (and thus [`is_perpetual`]) is - /// preserved. + /// preserved, and so is the no-progress breaker's tracking-issue ref (see + /// below). /// - /// **`wip_refs.clear()` is load-bearing, not cosmetic** — and therefore this - /// must only be called once the goal's current unit of work is genuinely - /// finished. Those refs feed the Overseer/Orient dedup set + /// **Dropping the live `wip_refs` is load-bearing, not cosmetic** — and + /// therefore this must only be called once the goal's current unit of work + /// is genuinely finished. Those refs feed the Overseer/Orient dedup set /// (`overseer::sensor::in_flight_from_board`, "so the Overseer never fights /// an engineer already on a case"), engineer-admission control /// (`ooda_brain::depended_on`), and the no-progress completion gate's @@ -375,11 +402,28 @@ impl ActiveGoal { /// research goal still holding a live PR/branch/session is treated as /// in-flight progress and is NOT rolled (issue #4399, crusty finding 1). /// + /// **Exception — the breaker tracking ref is preserved** (issue #4497/#4509). + /// A `wip_ref` matching [`WipRef::is_no_progress_tracking`] is kept across the + /// roll: it is a durable RECORD (`issue` kind — not-live per + /// `has_live_in_flight_ref`, so it neither suppresses the never-idle fault nor + /// admits an overlapping engineer), and keeping it lets the breaker's in-memory + /// duplicate-issue dedup survive a re-orient/roll IO-free, instead of relying + /// on the remote `gh` search-before-create when a re-stall re-escalates. + /// /// [`is_perpetual`]: ActiveGoal::is_perpetual pub fn roll_to_new_cycle(&mut self) { self.status = GoalProgress::NotStarted; self.assigned_to = None; - self.wip_refs.clear(); + // Drop stale live work-in-progress refs (PR/branch/session/engineer) so + // the next cycle re-enters the spawn path, but PRESERVE the no-progress + // breaker's tracking-issue ref (issue #4497/#4509). That ref is a durable + // RECORD, not live work (`has_live_in_flight_ref` treats `issue` as + // not-live), so keeping it neither suppresses the never-idle fault nor + // spawns an overlapping engineer — while letting the breaker's in-memory + // `already_tracked` dedup survive this roll/re-orient IO-free, instead of + // depending on the remote `gh` search-before-create (which is unavailable + // during a `gh` outage). Belt-and-suspenders with the remote signature. + self.wip_refs.retain(WipRef::is_no_progress_tracking); self.current_activity = Some("standing goal — finished a unit of work; rolled to a fresh cycle".to_string()); } @@ -873,7 +917,55 @@ mod tests { ); } - // ── has_live_in_flight_ref: the never-idle preservation guard (#4399) ── + #[test] + fn roll_to_new_cycle_preserves_breaker_tracking_ref_but_drops_live_refs() { + // The no-progress breaker's tracking-issue ref is a durable RECORD that + // must survive a re-orient/roll so the in-memory duplicate-issue dedup + // works IO-free across the roll (issue #4497/#4509) — while all live + // in-flight refs are still dropped so the next cycle re-enters the spawn + // path and no overlapping engineer is admitted. + let mut g = sample_goal(); + g.description = "Research cognition. STANDING PERPETUAL goal.".to_string(); + g.status = GoalProgress::Blocked("ooda-no-progress".to_string()); + g.assigned_to = Some("engineer-x".to_string()); + let tracking = WipRef { + kind: "issue".to_string(), + ref_id: "4231".to_string(), + label: format!("{NO_PROGRESS_TRACKING_LABEL_PREFIX}#4231"), + url: Some("https://example/issues/4231".to_string()), + }; + g.wip_refs = vec![ + WipRef { + kind: "pr".to_string(), + ref_id: "1".to_string(), + label: "old".to_string(), + url: None, + }, + WipRef { + kind: "branch".to_string(), + ref_id: "feat/x".to_string(), + label: "old".to_string(), + url: None, + }, + tracking.clone(), + ]; + + g.roll_to_new_cycle(); + + assert_eq!(g.status, GoalProgress::NotStarted); + assert_eq!(g.assigned_to, None); + assert_eq!( + g.wip_refs, + vec![tracking], + "only the breaker tracking-issue ref must survive the roll" + ); + // A preserved `issue` ref is a not-live record: it must NOT read as live + // in-flight work (else it would suppress the never-idle fault forever). + assert!( + !g.has_live_in_flight_ref(), + "a preserved tracking-issue ref must not count as live in-flight work" + ); + } // // `wip_refs` is load-bearing — the Overseer dedup set (sensor.rs), // engineer-admission control (ooda_brain), and the completion-gate's diff --git a/src/ooda_loop/no_progress.rs b/src/ooda_loop/no_progress.rs index 569a9f4a1..70f4047b4 100644 --- a/src/ooda_loop/no_progress.rs +++ b/src/ooda_loop/no_progress.rs @@ -30,7 +30,9 @@ use crate::goal_curation::no_progress_breaker::{ use crate::goal_curation::no_progress_why::{ Evidence, NoProgressClass, NoProgressWhy, NoProgressWhyReasoner, }; -use crate::goal_curation::{ActiveGoal, GoalBoard, GoalProgress, WipRef}; +use crate::goal_curation::{ + ActiveGoal, GoalBoard, GoalProgress, NO_PROGRESS_TRACKING_LABEL_PREFIX, WipRef, +}; use crate::ooda_actions::outcome_made_no_progress; use crate::ooda_loop::{ActionOutcome, OodaState}; @@ -59,13 +61,16 @@ fn is_breaker_defer_ref(wip: &WipRef) -> bool { /// synthetic `simard-identity-*` goals into a checkable criterion. /// * **Idempotence.** A goal already carrying its breaker tracking issue is /// never re-filed, so a re-stall can never spam duplicate `ooda-stuck` issues. -const NO_PROGRESS_TRACKING_LABEL_PREFIX: &str = "[no-progress-tracking] "; - +/// +/// The prefix is defined once beside [`WipRef`] +/// ([`crate::goal_curation::NO_PROGRESS_TRACKING_LABEL_PREFIX`]) so +/// [`ActiveGoal::roll_to_new_cycle`] can preserve this ref across a re-orient/roll +/// (the durable, IO-free half of the dedup) without a duplicated magic string. +/// /// True when `wip` is a breaker-authored tracking-issue link (the escalation /// artifact authored by [`link_tracking_issue`]). fn is_breaker_tracking_ref(wip: &WipRef) -> bool { - wip.kind.eq_ignore_ascii_case("issue") - && wip.label.starts_with(NO_PROGRESS_TRACKING_LABEL_PREFIX) + wip.is_no_progress_tracking() } /// True when `goal_id` currently stands **Blocked with the no-progress sentinel** diff --git a/src/ooda_loop/tests_no_progress_livelock_dedup.rs b/src/ooda_loop/tests_no_progress_livelock_dedup.rs index 2aae86eb9..3d33cc132 100644 --- a/src/ooda_loop/tests_no_progress_livelock_dedup.rs +++ b/src/ooda_loop/tests_no_progress_livelock_dedup.rs @@ -199,10 +199,14 @@ fn drive_to_escalation( last } -/// Simulate the per-goal re-orientation the agentic reasoner performs -/// (`ActiveGoal::roll_to_new_cycle`): drop the goal's tracked refs so the OLD -/// in-memory-only dedup is defeated. The remote store is untouched — that is the -/// whole point of the fix. +/// Simulate LOSS of the in-memory tracking link — the goal's `wip_refs` are +/// dropped WITHOUT a matching remote loss, as happens on a process restart (or, +/// before the #4509 preservation fix, on every `roll_to_new_cycle`). This +/// exercises the REMOTE search-before-create fallback: the durable dedup source +/// of truth when the in-memory link is gone. The remote store is untouched — +/// that is the whole point. (The in-process re-orient path, where the link now +/// SURVIVES the roll, is covered by +/// `roll_to_new_cycle_preserves_in_memory_dedup_across_reorient`.) fn reorient(state: &mut OodaState, goal_id: &str) { if let Some(g) = state .active_goals @@ -299,6 +303,82 @@ fn a_still_blocked_goal_files_at_most_one_issue_across_reorient() { ); } +#[test] +fn roll_to_new_cycle_preserves_in_memory_dedup_across_reorient() { + // The #4509 durable, IO-free complement to the remote search: an in-PROCESS + // re-orientation calls the REAL `ActiveGoal::roll_to_new_cycle`, which now + // PRESERVES the breaker's tracking-issue ref. So on the re-stall the + // in-memory `already_tracked` fast-path dedups WITHOUT ever consulting the + // remote — no duplicate issue AND no dependence on `gh` availability. + let threshold = NO_PROGRESS_BREAKER_THRESHOLD; + let filer = RemoteFiler::default(); + let evidence = NoEvidence; + let mut state = state_with(stuck_goal("simard-identity-4d27c91a")); + + let _ = drive_to_escalation( + &mut state, + "simard-identity-4d27c91a", + &evidence, + &filer, + threshold, + ); + assert_eq!( + filer.files.borrow().len(), + 1, + "the first escalation files exactly one ooda-stuck issue" + ); + let searches_after_first = filer.searches.borrow().len(); + assert_eq!( + state.active_goals.active[0] + .wip_refs + .iter() + .filter(|w| w.kind.eq_ignore_ascii_case("issue")) + .count(), + 1, + "the escalated goal carries its linked tracking issue ref" + ); + + // Real in-process re-orientation: lifts the block AND preserves the tracking + // ref (drops the stale live PR ref). + state.active_goals.active[0].roll_to_new_cycle(); + assert_eq!( + state.active_goals.active[0] + .wip_refs + .iter() + .filter(|w| w.kind.eq_ignore_ascii_case("issue")) + .count(), + 1, + "roll_to_new_cycle must PRESERVE the breaker tracking-issue ref" + ); + assert!( + !state.active_goals.active[0] + .wip_refs + .iter() + .any(|w| w.kind.eq_ignore_ascii_case("pr")), + "roll_to_new_cycle must still DROP the stale live PR ref" + ); + + // Re-stall to the threshold again. + let _ = drive_to_escalation( + &mut state, + "simard-identity-4d27c91a", + &evidence, + &filer, + threshold, + ); + + assert_eq!( + filer.files.borrow().len(), + 1, + "no duplicate issue: the preserved in-memory ref dedups across the roll (#4509)" + ); + assert_eq!( + filer.searches.borrow().len(), + searches_after_first, + "the remote search is NOT consulted after a roll — in-memory dedup short-circuits (IO-free, gh-independent)" + ); +} + #[test] fn dedup_survives_process_restart() { // A restart drops the in-memory tracker/state entirely. A fresh state whose