diff --git a/docs/concepts/auto-doc-pr-reconciliation.md b/docs/concepts/auto-doc-pr-reconciliation.md new file mode 100644 index 000000000..f1f4930c0 --- /dev/null +++ b/docs/concepts/auto-doc-pr-reconciliation.md @@ -0,0 +1,188 @@ +--- +title: The overseer reconciles auto-generated documentation PRs to one open at a time +description: > + Why the ~30 stale, CONFLICTING, draft auto-generated `Update documentation + with N changed files` PRs no longer accumulate. An additive overseer + reconciliation pass (`overseer::doc_pr_reconcile`) enforces a single-open + invariant — at most one open auto-doc PR at a time — by superseding-and-closing + older duplicates and auto-closing stale CONFLICTING auto-doc drafts, matched on + a composite fail-closed identity gate (title marker `Update documentation + with` + auto-generated author + draft + label). The keeper (canonical) PR is + never closed; every mutation is by PR number through an argv-only `gh pr close` + behind a defaulted, non-breaking `PrGhClient::close_pr`. The reconcile core is + a pure function; the executor is a bounded, IO-guarded overseer pass. Empty + author = treated as human = skipped. +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: concept +status: implemented +related: + - ./durable-documentation-policy.md + - ./stewardship-mode.md + - ./gap-scan-backoff-dedup.md + - ./autonomous-merge-review-gate.md + - ./draft-pr-merge-exclusion.md + - ../reference/auto-doc-pr-reconciliation-api.md + - ../reference/cross-repo-merge-authority.md + - ../howto/reconcile-stale-auto-doc-prs.md + - ../design/overseer.md +--- + +# The overseer reconciles auto-generated documentation PRs to one open at a time + +> **Status: implemented.** The pure reconcile core and its executor live in +> `src/overseer/doc_pr_reconcile.rs`. The additive, defaulted `close_pr` +> operation lives on `PrGhClient` in `src/stewardship/merge_authority.rs` +> (`RealPrGhClient` overrides it to shell out to `gh pr close` argv-only). The +> bounded pass is wired into the overseer cycle in `src/overseer/mod.rs`. For the +> exact types and functions see the +> [auto-doc PR reconciliation API reference](../reference/auto-doc-pr-reconciliation-api.md). + +## The defect + +Over roughly a week the repository accumulated **~30 stale, CONFLICTING, draft** +pull requests, all sharing one title shape: + +``` +Update documentation with N changed files +``` + +(the oldest dating to 2026-07-22). Each was opened by an automated doc-drift +event and then never rebased, never closed, and never superseded — so they rotted +into `CONFLICTING` drafts that clutter the PR list, confuse merge-readiness +sensors, and bury the real PRs. + +Root cause: the doc-update automation opens a **fresh** PR per doc-drift event +with no deduplication. There is no single generating workflow/script to patch — +codebase analysis confirmed the literal title string +`"Update documentation with"` is not emitted by any committed workflow; these PRs +are agent/prompt-driven via `gh pr create`. So the fix cannot live in a +generator that does not exist. It must live where PR state is already +**reconciled**: the overseer. + +## The fix: an additive overseer reconciliation pass + +The overseer already reconciles PR/merge state each cycle. This feature adds a +small, additive pass — `overseer::doc_pr_reconcile` — that enforces a +**single-open invariant** for auto-doc PRs: + +> **At most one open auto-doc PR exists at a time.** All older duplicates are +> superseded-and-closed; stale `CONFLICTING` auto-doc drafts are auto-closed. + +Because there is no generator to make idempotent, reconciliation is done +**after the fact**, keyed on a **stable doc-PR marker**, not on a generator's +internal state. + +### Identifying an auto-doc PR — a composite, fail-closed gate + +A PR is treated as an auto-doc PR only when **all** of a composite identity gate +match. The gate is deliberately conservative and **fails closed** (skips) on any +doubt, so a human PR can never be misclassified and closed: + +| Signal | Requirement | +| --- | --- | +| **Title** | Title-prefix match on the marker `"Update documentation with"`. | +| **Author** | Author is the known auto-generation identity. **An empty/absent author is treated as human and skipped** — never reconciled. | +| **Draft** | The PR is a draft (auto-doc PRs are opened as drafts). | +| **Label** | Carries the auto-generated doc-update label. | + +Only a PR matching every signal is a reconciliation candidate. Anything else — +including a human-authored PR that happens to start with the same words — is left +completely untouched. + +### Choosing the canonical (keeper) PR + +Among the matched candidates the pass selects exactly **one canonical** PR to +keep open — the newest / most-current one (the one most likely to reflect the +latest doc drift). The canonical PR is **never closed**. Every other matched +candidate is a *superseded duplicate*. + +### The two reconciliation actions + +```text +matched auto-doc PRs (title + author + draft + label) + │ + ├─ pick canonical (newest) ──────────────► keep open (never closed) + │ + └─ for each non-canonical candidate: + ├─ superseded duplicate ─────────► close (supersede) + └─ stale CONFLICTING draft ──────► close (auto-close) +``` + +1. **Supersede-and-close older duplicates.** Every non-canonical matched PR is + closed with a comment pointing at the canonical PR, collapsing the population + to the single-open invariant. +2. **Auto-close stale CONFLICTING drafts.** A matched draft in `CONFLICTING` + mergeable state is closed rather than left to rot. (A future enhancement may + *rebase* instead of close where safe; the shipped behavior is close, because a + fresh auto-doc PR will be re-opened on the next real doc drift anyway.) + +The pure core returns the **decision** (which PR is canonical, which numbers to +close and why); the executor applies it. This keeps the whole classification and +selection surface testable on fixture PR lists with zero network. + +## Safety — this pass mutates PRs, so it is bounded and fail-closed + +Closing PRs is a new destructive mutation for the overseer, so every safeguard is +conservative: + +- **Composite identity gate.** All four signals must match; an empty author is + treated as human and skipped. A human PR is structurally unclosable by this + pass. +- **Canonical never closed.** The keeper is excluded from the close set by + construction, so the invariant can never close *every* PR. +- **Mutate by number only.** `close_pr` acts on a specific PR **number**; the + argv is built positionally and is structurally incapable of carrying `--admin` + / `--no-verify`. No shell interpolation (`gh` argv-only, never `sh -c`). +- **Additive, defaulted trait method.** `PrGhClient::close_pr` has a **no-op + default**, so every existing fake and unwired client performs no mutation; + only `RealPrGhClient` shells out. Adding it breaks no caller. +- **Bounded, IO-guarded pass.** The pass runs behind the same composite + fail-closed gate as the overseer's other IO passes: a read/list failure surfaces + the error and performs **no** closes that cycle (better to skip than to + mis-close on a transient error). It processes a bounded batch per cycle. +- **OTel-only audit.** Every classify/keep/close decision is emitted as a + structured tracing / OTel event — no `print!`/`println!`. + +## Relationship to the durable-documentation policy (G4) + +This feature is the **cleanup** counterpart to +[G4 durable-documentation policy](./durable-documentation-policy.md). G4's +pr-verify scan blocks *point-in-time report docs* from being **merged**; this +pass stops *auto-doc-drift PRs* from **accumulating**. They are complementary: +G4 governs what may land, `doc_pr_reconcile` governs how many auto-doc PRs may be +open at once. Both live inside the overseer's PR/merge reconciliation surface. + +## Why reconcile after the fact instead of fixing a generator? + +- **There is no generator to fix.** The title string is not emitted by any + committed workflow/script; the PRs are agent/prompt-driven `gh pr create` + calls. A patch to a nonexistent emitter would be fiction. +- **State reconciliation is the right home.** The single-open invariant and the + stale-draft auto-close are properties of the *live PR population*, which the + overseer already reconciles each cycle. Enforcing them there is additive and + needs no change to how any PR is opened. +- **It is a self-amplifying-loop fix.** Like the + [gap-scan backoff/dedup](./gap-scan-backoff-dedup.md), the defect is a + safeguard/automation observing a condition and re-emitting instead of + converging. Reconciliation converges the population to one. + +## What an operator sees now + +- At most **one** open `Update documentation with …` PR at a time; older + duplicates are closed with a supersede comment linking the canonical one. +- Stale `CONFLICTING` auto-doc drafts are auto-closed instead of rotting. +- Human PRs — including any that coincidentally start with the same words — are + never touched. + +See [Reconcile stale auto-doc PRs](../howto/reconcile-stale-auto-doc-prs.md) for +confirming the invariant and clearing a pre-existing backlog. + +## See also + +- [Auto-doc PR reconciliation API reference](../reference/auto-doc-pr-reconciliation-api.md) — the reconcile core, the identity gate, and `close_pr`. +- [Durable-Documentation Policy (G4)](./durable-documentation-policy.md) — the merge-gate counterpart. +- [Gap-scan dedup & exponential backoff](./gap-scan-backoff-dedup.md) — the overseer's sibling self-amplifying-loop fix. +- [Draft-PR merge exclusion](./draft-pr-merge-exclusion.md) — why auto-doc drafts are never merge candidates. +- [Overseer — operator/observer co-process (design)](../design/overseer.md) — the cycle this pass runs inside. diff --git a/docs/concepts/ooda-breaker-churn-suppression.md b/docs/concepts/ooda-breaker-churn-suppression.md new file mode 100644 index 000000000..ab963cd60 --- /dev/null +++ b/docs/concepts/ooda-breaker-churn-suppression.md @@ -0,0 +1,264 @@ +--- +title: The OODA breaker quarantines terminal UNCLEAR-CRITERIA goals and dedups reblock issues +description: > + Why the OODA no-progress breaker no longer churns on UNCLEAR-CRITERIA goals — + ~33 open `ooda-stuck` issues and ~44 open `recurring_goal_reblock in + simard::overseer` stewardship issues, with ~13 stuck + ~8 reblock filed in a + single day. Two additive fixes end the residual churn the admission-path work + (PRs #4939/#4941) does not cover: (1) a terminal-quarantine rung + (`NoProgressResolution::QuarantineTerminal`) that stops re-scheduling and + re-filing a goal once it trips `UNCLEAR-CRITERIA` after the bounded guided + retry, keyed on a durable, injection-safe quarantine marker; and (2) + reblock-issue signature stabilization so recurrences of the same root cause + collapse to one stewardship issue instead of one per re-observation. Both are + additive; clear-criteria goals behave identically. Quarantine is reversible. +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: concept +status: implemented +related: + - ./no-progress-root-cause-resolution.md + - ./no-progress-terminal-investigation.md + - ./no-progress-breaker-storm-suppression.md + - ./ooda-reinvestigate-blocked-goals.md + - ./overseer-root-cause-why.md + - ./steerable-ooda-daemon.md + - ../reference/ooda-breaker-churn-suppression-api.md + - ../reference/no-progress-breaker-storm-suppression-api.md + - ../howto/quarantine-and-recover-an-unclear-ooda-goal.md + - ../howto/unblock-stuck-ooda-goals.md +--- + +# The OODA breaker quarantines terminal UNCLEAR-CRITERIA goals and dedups reblock issues + +> **Status: implemented.** The terminal-quarantine rung +> (`NoProgressResolution::QuarantineTerminal`, the `resolution_for_why` +> surfaced-failure parameter, and the durable quarantine marker) lives in +> `src/goal_curation/no_progress_breaker.rs`; the side effects (block, mark, +> and — the single churn-stopping change — the re-schedule exclusion in +> `reinvestigate_bare_blocked_goals`) live in the curate-phase adapter +> `src/ooda_loop/no_progress.rs` (this handler **replaces** the adapter's prior +> inline escalate-at-limit branch). The reblock-issue signature stabilization is +> the new `fold_volatile_goal_ids` helper applied inside `problem_to_run_brief` +> in `src/overseer/observer.rs`, upstream of the existing +> `dedup::failure_signature`; the exported `root_cause_signature` is **not** the +> reblock dedup key and is left untouched. For exact types and functions see the +> [churn-suppression API reference](../reference/ooda-breaker-churn-suppression-api.md). + +## The defect + +In a single day the daemon accumulated **~33 open `ooda-stuck` issues** and +**~44 open `recurring_goal_reblock in simard::overseer` stewardship issues**, +with roughly **13 stuck + 8 reblock filed in the last 24 hours** — e.g. a run of +issues all titled: + +``` +OODA no-progress breaker: goal stuck after guided retry (UNCLEAR-CRITERIA) +``` + +This is **not** dozens of independent stuck goals. It is a small population of +goals whose done-criteria are structurally **unmeasurable** (`UNCLEAR-CRITERIA`) +looping through the breaker forever, plus the Overseer re-observing the same +re-block condition each cycle and filing a fresh stewardship issue every time. + +The [issue-storm suppression](./no-progress-breaker-storm-suppression.md) work +already made a *single* escalation idempotent via the durable suppression +marker. But suppression stops **re-filing**; it does not stop the goal from +being **re-scheduled**. An `UNCLEAR-CRITERIA` goal that has exhausted the bounded +guided-retry ladder was still picked up by the re-investigation pass every cycle, +re-classified, re-surfaced, and re-escalated — and the Overseer, watching that +re-block, filed a new `recurring_goal_reblock` issue each time because its +dedup signature drifted with volatile goal identifiers. + +Two coupled faults: + +1. **No terminal rung.** After the guided retry, a permanently-unclear goal had + nowhere to land. It kept cycling through `SurfaceInvestigationFailure` / + re-escalation instead of being parked **terminally** and removed from the + schedule. +2. **Unstable reblock signature.** The Overseer's `recurring_goal_reblock` + stewardship issue was keyed on a signature that embedded volatile identifiers + (`simard-identity-*`, `goal-`), so each recurrence looked "new" and filed + its own issue instead of collapsing onto the existing one. + +> This scope is deliberately **disjoint** from the goal-admission hardening in +> PRs #4939 (centralized admission gate) and #4941 (declarative standing seed +> goals). Those own *which goals are admitted*; this owns the *residual churn* +> after a goal is already stuck — re-scheduling and reblock-issue dedup only. + +## Fix 1 — a terminal quarantine rung + +The [root-cause ladder](./no-progress-root-cause-resolution.md) already routes a +stall down self-resolving rungs and, at the bottom, spawns **one** guided +engineer then surfaces a bounded investigation gap +(`SURFACED_INVESTIGATION_FAILURE_LIMIT`, see +[terminal investigation](./no-progress-terminal-investigation.md)). Quarantine is +the **missing terminal rung after that bound**: the rung that says "this goal has +exhausted every machine-resolvable and guided path; stop spending cycles on it." + +### The variant + +`NoProgressResolution::QuarantineTerminal` is a new terminal variant of the pure +resolution enum. It: + +- carries the `surfaced_count` (the number of consecutive evidence-less surfaced + failures that drove the goal here) as **real evidence** — it never renders + `evidence=[(none)]`, preserving the + [never-empty-evidence invariant](./no-progress-terminal-investigation.md); +- reports `is_terminal() == true`, so callers know no further rung follows. + +`resolution_for_why` gains **one additive trailing parameter**, +`surfaced_failures` — its existing `(consecutive, why, guided_retry_used)` +parameters are unchanged. On the evidence-less terminal rung (an +`UNCLEAR-CRITERIA` / `GENUINELY-STUCK` WHY where `guided_retry_used` is already +true and the guided investigation produced no evidence), once +`surfaced_failures >= SURFACED_INVESTIGATION_FAILURE_LIMIT` (3) it returns +`QuarantineTerminal` instead of surfacing yet another investigation gap. Every +other class, and the same rung **below** the threshold, is unchanged — quarantine +is strictly the top rung of an already-bounded ladder, reached only after the +guided engineer has run and the surfaced-failure bound has been hit. + +Crucially, this **replaces** the limit decision that today lives inline in the +curate-phase adapter: the current `SurfaceInvestigationFailure` handler escalates +at the bound via `surfaced_failure_escalation_issue`. That escalate-at-limit +branch is removed and replaced by the `QuarantineTerminal` handling below, so a +bounded-out goal quarantines instead of escalating (never both). + +### The durable, injection-safe quarantine marker + +Quarantine reuses the durable `WipRef` marker infrastructure already used for +[storm suppression](./no-progress-breaker-storm-suppression.md), with its own +kind: + +- `WipRef.kind = "ooda-breaker-quarantine"` — a novel kind, so every other + `wip_refs` consumer (`has_derivable_signal`, `stuck_evidence`, + `artifact_evidence`, the stale-assignment sweep) ignores it via its `_ => None` + fall-through. The marker is inert to completion/liveness logic; only the + quarantine predicate and the re-schedule filter read it. +- `WipRef.ref_id` is a **fixed sentinel constant** — **never** derived from goal + text — so a goal description can never smuggle content into the marker or forge + another goal's quarantine. + +`apply_resolution_side_effects` handles `QuarantineTerminal` by (a) setting the +goal `Blocked` with a WHY-bearing reason (the marker prefix + the surfaced-count +evidence) and (b) writing the quarantine marker **idempotently** through the +existing atomic, single-writer goal-board save path. Marker writes **fail +closed**: if the marker cannot be persisted, no terminal claim is made and the +goal retries next cycle, never silently dropping the quarantine. + +### The single churn-stopping change: exclude quarantined goals from re-scheduling + +`reinvestigate_bare_blocked_goals` — the pass that sweeps blocked goals back into +investigation each cycle — now **excludes any goal carrying the quarantine +marker**. This is the one change that actually stops the churn: a quarantined +goal is no longer selected, re-classified, re-surfaced, or re-escalated. It sits +`Blocked` + quarantined until a human intervenes. + +```text +UNCLEAR-CRITERIA stall + │ (guided engineer runs once; surfaced-failure gap bounded at 3) + ▼ +surfaced_failures reaches SURFACED_INVESTIGATION_FAILURE_LIMIT + │ + ▼ +resolution_for_why → QuarantineTerminal(surfaced_count) + │ + ├─ Blocked (WHY + surfaced-count evidence, never (none)) + ├─ durable quarantine marker written (idempotent, fail-closed) + └─ reinvestigate_bare_blocked_goals SKIPS it forever after + └─ no re-schedule → no re-classify → no re-file → churn stops +``` + +## Fix 2 — stabilize the reblock-issue signature + +`recurring_goal_reblock in simard::overseer` is the **observed** `dedup_key` / +issue-title text the Overseer emits when it re-observes a goal being re-blocked +— not a code constant. That stewardship issue is deduplicated through the +existing **failure-signature** path, not through `root_cause_signature`: + +```text +observer::problem_to_run_brief → OrchestratorRunBrief { failure_kind: problem.dedup_key, error_text } + → stewardship::failure_signature(failure_kind, error_text) +``` + +`failure_signature` SHA-256s `failure_kind` **verbatim** and normalizes only +`error_text`. So when `problem.dedup_key` embeds **volatile** identifiers — +synthetic `simard-identity-*` goal ids and positional `goal-` slugs — the +`failure_kind` (and therefore the signature) drifts every cycle, and each +re-observation files its own issue. + +> Note: the Overseer's exported `root_cause_signature` helper has **no +> non-test caller** keying this issue; it is *not* the reblock dedup key and is +> left untouched by this fix. + +The fix folds those volatile tokens to stable placeholders **before** +`problem.dedup_key` becomes `failure_kind`, inside `problem_to_run_brief`, via a +**new** pure helper `fold_volatile_goal_ids`. It is deliberately named to avoid +the pre-existing private `dedup::normalize_for_signature` (the message +UUID-redactor `failure_signature` already uses internally — a different function +in a different module). The fold is conservative: it only rewrites the known +volatile id shapes and leaves everything else byte-for-byte, so distinct root +causes still get distinct signatures. Every recurrence of the same underlying +re-block cause then collapses onto **one** stewardship issue, mirroring the +Overseer's existing [failure-signature dedup](./overseer-root-cause-why.md). + +## Reversibility — quarantine is a park, not a grave + +Quarantine is **terminal for the daemon, reversible for a human**. When an +operator un-blocks the goal with the single-id escape hatch +(`simard goal unblock `, or by giving it a checkable finish condition), +the quarantine marker is cleared and the goal earns a **fresh** bounded window: +the surfaced-failure counter and quarantine state reset, so it is re-investigated +like any newly-unblocked goal rather than re-quarantining immediately. +`simard goal unblock-all` is deliberately scoped to the brain-failure safeguard +marker and does **not** mass-clear quarantines — quarantine is a considered +terminal state, so clearing it is an explicit per-goal decision. See +[Quarantine and recover an unclear OODA goal](../howto/quarantine-and-recover-an-unclear-ooda-goal.md). + +The right permanent fix for a quarantined goal is almost always to **make its +done-criteria machine-checkable** (name a specific issue that must be observed +`CLOSED`, a PR observed `MERGED`, or a file/command whose output the done-gate +can verify) — or to drop it if it is out of scope. + +## Why not just lengthen the surfaced-failure bound? + +- **Raising `SURFACED_INVESTIGATION_FAILURE_LIMIT`** only delays the storm; a + *permanently* unclear goal re-investigates forever at any finite bound. The + correct terminal count is a hard **stop**, not a larger rate. +- **Deleting the goal automatically** would be destructive and irreversible — a + transient misclassification would silently lose real work. Quarantine parks the + goal visibly and reversibly instead. +- **Title-level issue dedup** was rejected for the same reason as in + [storm suppression](./no-progress-breaker-storm-suppression.md#why-not-backoff-title-level-dedup-or-an-in-memory-guard): + a shared title is a *symptom* of per-goal re-filing, not many goals colliding. + Root-cause-signature dedup collapses genuine recurrences without risking + suppression of legitimately-distinct filings. + +## What an operator sees now + +- A permanently-unclear goal produces **one** `ooda-stuck` escalation, then sits + `Blocked` with a `ooda-breaker-quarantine` marker in its `wip_refs` — it is not + re-investigated or re-escalated every cycle. +- Recurrences of the same re-block root cause collapse onto **one** + `recurring_goal_reblock in simard::overseer` stewardship issue instead of one + per cycle. +- Clear-criteria goals, and unclear goals still inside the guided-retry ladder, + behave exactly as before — quarantine is strictly the terminal rung. + +## Interaction with the admission-path work (PRs #4939/#4941) + +This feature edits shared OODA-core files, so it lands **one-at-a-time in the +`ooda-core` sequence group** and deliberately does **not** touch the goal-admission +gate owned by PRs #4939/#4941. Merging those PRs relieves the *inflow* of +unclear goals; this feature terminally parks the *residue* that still gets +through and stops the reblock-issue churn. The two are complementary and +non-overlapping. + +## See also + +- [Churn-suppression API reference](../reference/ooda-breaker-churn-suppression-api.md) — the variant, the marker, the resolution signature, and the re-schedule filter. +- [The no-progress breaker suppresses its own issue storm](./no-progress-breaker-storm-suppression.md) — the durable-marker infrastructure quarantine reuses. +- [The terminal no-progress stall never parks a goal with empty evidence](./no-progress-terminal-investigation.md) — the surfaced-failure bound quarantine sits above. +- [Overseer Root-Cause (WHY) Principle](./overseer-root-cause-why.md) — the signature dedup this stabilizes. +- [Quarantine and recover an unclear OODA goal](../howto/quarantine-and-recover-an-unclear-ooda-goal.md) — the operator runbook and reversal path. diff --git a/docs/concepts/steerable-ooda-daemon.md b/docs/concepts/steerable-ooda-daemon.md index cfba3e0e7..58bde6ce4 100644 --- a/docs/concepts/steerable-ooda-daemon.md +++ b/docs/concepts/steerable-ooda-daemon.md @@ -154,6 +154,34 @@ the goal has produced no progress; the recipe explains why in operator language. The recipe does not grant itself authority to suppress the breaker. It can enrich the evidence narrative; it cannot make a looping goal healthy by narration. +### Terminal quarantine — the rung below the guided retry + +The no-progress ladder is bounded at the bottom by a **terminal-quarantine +rung**, owned by Rust as a deterministic safeguard (not a prompt judgment). A +goal whose done-criteria are structurally unmeasurable (`UNCLEAR-CRITERIA`) gets +one guided-engineer retry and then, if it keeps surfacing evidence-less +investigation gaps up to `SURFACED_INVESTIGATION_FAILURE_LIMIT` (3), the breaker +returns `NoProgressResolution::QuarantineTerminal`. Quarantine: + +- sets the goal `Blocked` with a WHY-bearing reason carrying the re-investigation + count as evidence (never `evidence=[(none)]`); +- writes a durable, injection-safe `ooda-breaker-quarantine` marker + (fixed-sentinel `ref_id`, never derived from goal text); and +- **removes the goal from re-scheduling** — `reinvestigate_bare_blocked_goals` + skips any quarantined goal, so the daemon spends no further cycles on it. This + is what stops the `ooda-stuck` / `recurring_goal_reblock` churn. + +Quarantine is **terminal for the daemon but reversible for a human**: an operator +`simard goal unblock ` clears the marker and resets the surfaced-failure +counter, so the goal earns a fresh bounded guided-retry window rather than +re-quarantining immediately. The durable fix is to give the goal a +machine-checkable finish condition (a specific issue `CLOSED`, a PR `MERGED`, or a +checkable file/command). See +[The OODA breaker quarantines terminal UNCLEAR-CRITERIA goals](./ooda-breaker-churn-suppression.md) +and its +[API reference](../reference/ooda-breaker-churn-suppression-api.md), plus the +[quarantine-and-recover runbook](../howto/quarantine-and-recover-an-unclear-ooda-goal.md). + ## What Rust owns Rust is deliberately boring. It owns: @@ -327,5 +355,7 @@ contract plus stricter contract validation, not a larger parser. - [How to run the OODA daemon](../howto/run-ooda-daemon.md) - [How OODA spawns engineer agents](../howto/spawn-engineers-from-ooda-daemon.md) +- [The OODA breaker quarantines terminal UNCLEAR-CRITERIA goals](./ooda-breaker-churn-suppression.md) +- [Quarantine and recover an unclear OODA goal](../howto/quarantine-and-recover-an-unclear-ooda-goal.md) - [Simard CLI reference](../reference/simard-cli.md) - [OODA coverage parallelism ceiling](../reference/ooda-coverage-parallelism-ceiling.md) diff --git a/docs/howto/quarantine-and-recover-an-unclear-ooda-goal.md b/docs/howto/quarantine-and-recover-an-unclear-ooda-goal.md new file mode 100644 index 000000000..fc781305e --- /dev/null +++ b/docs/howto/quarantine-and-recover-an-unclear-ooda-goal.md @@ -0,0 +1,148 @@ +--- +title: Quarantine and recover an unclear OODA goal +description: > + Runbook for the terminal-quarantine rung of the OODA no-progress breaker. + Explains how to recognise a goal that has been terminally quarantined after + exhausting the guided-retry ladder on an UNCLEAR-CRITERIA classification, why + quarantine stops the `ooda-stuck` / `recurring_goal_reblock` churn, and how to + recover a quarantined goal — by giving it a machine-checkable finish condition + (the durable fix) or by un-blocking it for a fresh bounded window. +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: howto +related: + - ../concepts/ooda-breaker-churn-suppression.md + - ../concepts/no-progress-breaker-storm-suppression.md + - ../concepts/no-progress-terminal-investigation.md + - ../reference/ooda-breaker-churn-suppression-api.md + - ../reference/simard-cli.md + - ./unblock-stuck-ooda-goals.md + - ./diagnose-a-no-progress-block.md + - ./diagnose-a-no-progress-breaker-issue-storm.md +--- + +# Quarantine and recover an unclear OODA goal + +## Symptom + +Before this fix, a goal whose done-criteria the daemon cannot machine-check +(`UNCLEAR-CRITERIA`) churned: every cycle it was re-investigated, re-surfaced, +and re-escalated. The visible symptoms were a growing pile of duplicate +tracking issues — + +``` +OODA no-progress breaker: goal stuck after guided retry (UNCLEAR-CRITERIA) +``` + +— and a matching pile of `recurring_goal_reblock in simard::overseer` +stewardship issues, one per cycle. + +As of the terminal-quarantine rung, such a goal is parked **once** and then left +alone. It shows on the board as `Blocked` and carries a durable +`ooda-breaker-quarantine` marker. + +## Recognise a quarantined goal + +```bash +simard goal list +``` + +A quarantined goal shows `status = Blocked` with a breaker-authored WHY-bearing +reason whose evidence is the re-investigation count itself (never `(none)`): + +```text +🔒 [OODA-SAFEGUARD] OODA goal made no shippable progress for 3 consecutive +no-action cycles; why=UNCLEAR-CRITERIA evidence=[re-investigation (3 consecutive evidence-less investigations)] +``` + +Inspect its `wip_refs` to confirm the quarantine marker +(`kind = ooda-breaker-quarantine`): + +```bash +simard goal show # look for a wip_ref with kind "ooda-breaker-quarantine" +``` + +A quarantined goal is **not** re-selected by the re-investigation pass +(`reinvestigate_bare_blocked_goals` skips it), so it stops generating new +`ooda-stuck` and `recurring_goal_reblock` issues immediately. + +## What quarantine does and does not mean + +- **Terminal for the daemon.** The goal is removed from re-scheduling; the loop + spends no more cycles on it. This is the change that stops the churn. +- **Reversible for a human.** Quarantine is a park, not a delete. No work is + lost; the goal remains on the board, visible and recoverable. +- **Reached only at the bottom of a bounded ladder.** A goal is quarantined only + after it (1) classified `UNCLEAR-CRITERIA`, (2) got its one guided engineer, + and (3) surfaced an evidence-less investigation gap + `SURFACED_INVESTIGATION_FAILURE_LIMIT` (3) times. Clear-criteria goals and + unclear goals still inside the ladder are never quarantined. + +## Recover a quarantined goal + +### Option A — give it a machine-checkable finish condition (preferred, durable) + +A goal is quarantined because the done-gate cannot tell when it is finished. The +durable fix is to replace it with a goal whose completion the daemon can +**observe**: a specific issue that must be `CLOSED`, a specific PR that must be +`MERGED`, or a file/command whose output the done-gate can check. + +```bash +# Remove the unclear goal (its work is not lost — you re-express it below). +simard goal remove + +# Add a concrete, completable replacement at a chosen priority (1-7). +simard goal add "module X line coverage >= 80%, PR merged" +``` + +### Option B — un-block for a fresh bounded window + +If you believe the goal was quarantined prematurely (e.g. a transient +misclassification), un-block it. This clears the quarantine marker and **resets** +the surfaced-failure counter, so the goal earns a fresh guided-retry window +rather than re-quarantining immediately: + +```bash +# Unconditional single-goal override: clears Blocked + the quarantine marker. +simard goal unblock +``` + +> `simard goal unblock-all` is scoped to the brain-failure safeguard marker and +> deliberately does **not** mass-clear quarantines — quarantine is a considered +> terminal state, so clearing it is an explicit, per-goal decision via +> `simard goal unblock `. + +After un-blocking, restart the daemon (or wait a cycle) and confirm the goal is +re-investigated: + +```bash +systemctl --user restart simard-ooda.service +simard goal list +``` + +If the underlying criteria are still unmeasurable, the goal will re-quarantine +after the same bounded ladder — that is the signal to use Option A instead. + +## Verify the churn has stopped + +- Duplicate `ooda-stuck` "goal stuck after guided retry (UNCLEAR-CRITERIA)" + issues stop accumulating; the quarantined goal has exactly one open escalation. +- `recurring_goal_reblock in simard::overseer` stewardship issues collapse to a + single open issue per root cause (see + [signature stabilization](../concepts/ooda-breaker-churn-suppression.md#fix-2-stabilize-the-reblock-issue-signature)). + Close the leftover duplicates by hand once you confirm no new ones appear. + +```bash +# Confirm no NEW duplicates are being filed (count should be stable across cycles). +gh issue list --repo rysweet/Simard --search "goal stuck after guided retry (UNCLEAR-CRITERIA)" --state open | wc -l +gh issue list --repo rysweet/Simard --search "recurring_goal_reblock in simard::overseer" --state open | wc -l +``` + +## Related + +- [The OODA breaker quarantines terminal UNCLEAR-CRITERIA goals](../concepts/ooda-breaker-churn-suppression.md) — the design and rationale. +- [Churn-suppression API reference](../reference/ooda-breaker-churn-suppression-api.md) — the exact variant, marker, and re-schedule filter. +- [Diagnose a no-progress breaker issue storm](./diagnose-a-no-progress-breaker-issue-storm.md) — the sibling per-goal filing cap. +- [Unblock stuck OODA goals](./unblock-stuck-ooda-goals.md) — the general block-clearing runbook. +- [Simard CLI reference: `simard goal`](../reference/simard-cli.md) diff --git a/docs/howto/reconcile-stale-auto-doc-prs.md b/docs/howto/reconcile-stale-auto-doc-prs.md new file mode 100644 index 000000000..565aaf958 --- /dev/null +++ b/docs/howto/reconcile-stale-auto-doc-prs.md @@ -0,0 +1,114 @@ +--- +title: Reconcile stale auto-generated documentation PRs +description: > + Runbook for the overseer's auto-doc PR reconciliation pass. Explains how to + recognise the single-open invariant is holding, how to clear a pre-existing + backlog of stale `Update documentation with N changed files` drafts, and how + to confirm human PRs are never affected. +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: howto +related: + - ../concepts/auto-doc-pr-reconciliation.md + - ../concepts/durable-documentation-policy.md + - ../reference/auto-doc-pr-reconciliation-api.md + - ../reference/simard-cli.md + - ../design/overseer.md +--- + +# Reconcile stale auto-generated documentation PRs + +## Symptom + +The repository has a growing pile of draft PRs, all titled like: + +``` +Update documentation with N changed files +``` + +Many are in `CONFLICTING` mergeable state and were opened days apart, never +rebased or closed. They clutter the PR list and confuse merge-readiness sensors. + +As of the overseer's `doc_pr_reconcile` pass, the daemon keeps **at most one** +such PR open at a time — superseding-and-closing older duplicates and +auto-closing stale `CONFLICTING` auto-doc drafts. This runbook confirms the +invariant and clears any pre-existing backlog. + +## Confirm the invariant is holding + +Count the currently-open auto-doc PRs. After the pass has run a cycle, this +should be `0` or `1`: + +```bash +gh pr list --repo rysweet/Simard --state open --draft \ + --search "Update documentation with in:title" --json number,title,mergeable +``` + +Check the overseer journal / logs for the reconciliation audit events (OTel / +structured tracing, one per keep/close decision): + +```bash +# The pass records a canonical keeper and the numbers it closed each cycle. +grep -i "doc_pr_reconcile\|auto-doc" ~/.simard/ooda.log | tail -20 +``` + +A healthy cycle logs one `canonical` (the keeper) and, when a backlog exists, a +bounded set of `closed` numbers each tagged `SupersededDuplicate` or +`StaleConflictingDraft`. + +## Clear a pre-existing backlog + +The pass processes a **bounded batch per cycle**, so a large pre-existing backlog +drains over several cycles rather than all at once (this is intentional — it +avoids a storm of `gh pr close` mutations). Simply let the daemon run; the count +converges to one. + +To watch it drain: + +```bash +watch -n 60 'gh pr list --repo rysweet/Simard --state open --draft \ + --search "Update documentation with in:title" --json number | jq length' +``` + +If the daemon is offline, the pass never runs. Either start it, or close the +backlog by hand — but note the pass will re-establish the single-open invariant +automatically once running, so manual cleanup is rarely needed: + +```bash +# Manual fallback ONLY (the daemon does this automatically): keep the newest, +# close the rest with a supersede comment. +CANON=$(gh pr list --repo rysweet/Simard --state open --draft \ + --search "Update documentation with in:title" --json number --jq 'max_by(.number).number') +gh pr list --repo rysweet/Simard --state open --draft \ + --search "Update documentation with in:title" --json number --jq '.[].number' \ + | grep -v "^$CANON$" \ + | xargs -I{} gh pr close {} --repo rysweet/Simard --comment "superseded by #$CANON (auto-doc reconciliation)" +``` + +## Confirm human PRs are never affected + +The pass uses a composite fail-closed identity gate (title marker + +auto-generated author + draft + label), and treats an **empty/absent author as +human**, so a human PR — even one whose title coincidentally starts with +"Update documentation with" — is never a reconciliation candidate. + +Verify by listing non-draft or human-authored PRs and confirming none were +closed by the pass: + +```bash +# Human/non-auto PRs should never appear in the pass's closed set. +gh pr list --repo rysweet/Simard --state closed \ + --search "Update documentation with in:title" --json number,author,isDraft \ + --jq '.[] | select(.isDraft == false)' +``` + +This should return nothing closed by reconciliation — the canonical keeper stays +open and only auto-generated drafts are closed. + +## Related + +- [The overseer reconciles auto-generated documentation PRs to one open at a time](../concepts/auto-doc-pr-reconciliation.md) — the design and rationale. +- [Auto-doc PR reconciliation API reference](../reference/auto-doc-pr-reconciliation-api.md) — the identity gate, decision core, and `close_pr`. +- [Durable-Documentation Policy (G4)](../concepts/durable-documentation-policy.md) — the merge-gate counterpart that governs what may land. +- [Simard CLI reference](../reference/simard-cli.md) diff --git a/docs/reference/auto-doc-pr-reconciliation-api.md b/docs/reference/auto-doc-pr-reconciliation-api.md new file mode 100644 index 000000000..9544389d5 --- /dev/null +++ b/docs/reference/auto-doc-pr-reconciliation-api.md @@ -0,0 +1,257 @@ +--- +title: Auto-doc PR reconciliation API reference +description: > + Reference for the overseer's auto-generated documentation PR reconciliation + pass. Specifies the composite auto-doc identity gate (title marker + + auto-generated author + draft + label), the pure `reconcile_doc_prs` decision + core and its `DocPrReconcileDecision` output, the canonical-PR selection, the + supersede-and-close / stale-CONFLICTING-close actions, the executor, and the + additive defaulted `PrGhClient::close_pr` (argv-only `gh pr close` in + `RealPrGhClient`). +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ../concepts/auto-doc-pr-reconciliation.md + - ../concepts/durable-documentation-policy.md + - ./cross-repo-merge-authority.md + - ./no-point-in-time-docs-scan.md + - ../howto/reconcile-stale-auto-doc-prs.md + - ../../src/overseer/doc_pr_reconcile.rs + - ../../src/overseer/mod.rs + - ../../src/stewardship/merge_authority.rs +--- + +# Auto-doc PR reconciliation API reference + +> **Status: implemented.** The pure reconcile core (`reconcile_doc_prs`), the +> composite identity gate, and the executor live in +> [`src/overseer/doc_pr_reconcile.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/doc_pr_reconcile.rs). +> The additive `close_pr` operation lives on `PrGhClient` in +> [`src/stewardship/merge_authority.rs`](https://github.com/rysweet/Simard/blob/main/src/stewardship/merge_authority.rs) +> (`RealPrGhClient` overrides it). The bounded pass is wired into the overseer +> cycle in +> [`src/overseer/mod.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/mod.rs). + +For the rationale, see +[The overseer reconciles auto-generated documentation PRs to one open at a time](../concepts/auto-doc-pr-reconciliation.md). + +## Contents + +- [Marker constants](#marker-constants) +- [The composite identity gate: `is_auto_doc_pr`](#the-composite-identity-gate-is_auto_doc_pr) +- [The pure decision core: `reconcile_doc_prs`](#the-pure-decision-core-reconcile_doc_prs) +- [`DocPrReconcileDecision`](#docprreconciledecision) +- [The executor: `run_doc_pr_reconcile`](#the-executor-run_doc_pr_reconcile) +- [`PrGhClient::close_pr`](#prghclientclose_pr) +- [Overseer cycle wiring](#overseer-cycle-wiring) +- [Invariants](#invariants) + +## Marker constants + +| Constant | Value | Meaning | +| --- | --- | --- | +| `AUTO_DOC_PR_TITLE_MARKER` | `"Update documentation with"` | Title-prefix that a candidate PR's title must start with. A durable cross-system string; changing it would silently disable reconciliation, so it is a stable contract. | +| `AUTO_DOC_PR_LABEL` | the auto-generated doc-update label (`SIMARD_ENGINEER_PR_LABEL`) | Label a candidate must carry. | + +> **Author identity is resolved at runtime, not a constant.** Earlier revisions +> hard-coded the expected author to `simard-overseer[bot]` +> (`DEFAULT_OVERSEER_AUTHOR_LOGIN`). That was wrong: the auto-doc PRs are authored +> under Simard's **engineer / OODA `gh` identity** +> ([`config::automerge_author`](../reference/) — env `SIMARD_AUTOMERGE_AUTHOR`), +> which is deliberately DISTINCT from the overseer-bot login, so the constant +> matched **zero** real PRs (an inert pass). The expected author is now resolved +> at the I/O boundary in `run_doc_pr_reconcile` and threaded into the pure gate as +> the `expected_author` argument. An empty/unresolved identity (env unset) matches +> nothing, so the pass stays fail-closed. + +## The composite identity gate: `is_auto_doc_pr` + +```rust +/// True only when EVERY signal marks `pr` an auto-generated doc-drift PR. +/// Fails closed: any missing signal — including an empty/absent author, or an +/// empty `expected_author` (unresolved identity) — returns false, so a human PR +/// (or a mis-resolved identity) is never a reconciliation candidate. +pub fn is_auto_doc_pr(pr: &OpenPrSummary, expected_author: &str) -> bool; +``` + +All of the following must hold; otherwise the PR is skipped: + +| Signal | Requirement | +| --- | --- | +| Expected author resolved | `expected_author` is non-empty (`config::automerge_author()` returned `Some`). Empty ⇒ `false` (fail-closed). | +| Title | `pr.title.starts_with(AUTO_DOC_PR_TITLE_MARKER)` | +| Author | `pr.author` equals `expected_author` (the runtime-resolved engineer identity). **Empty/absent author ⇒ `false` (treated as human).** Keeping this a mandatory conjunct is the security-critical guard — `pr.author.login` is GitHub-authoritative and unspoofable. | +| Draft | `pr.is_draft == Some(true)` (the field is `Option`; `None`/absent ⇒ `false`) | +| Label | `pr.labels` contains `AUTO_DOC_PR_LABEL` | + +> `OpenPrSummary` is the existing listing summary from +> [`stewardship::merge_authority`](./cross-repo-merge-authority.md). The gate +> **reads its existing fields** — `title`, `author`, `labels`, and `is_draft` +> (`Option`) — all of which already exist on the struct (added for the +> autonomous-self-merge sensor, #4097). This feature adds **no** field to +> `OpenPrSummary`; the only genuinely new items are `PrGhClient::close_pr` and +> the `doc_pr_reconcile.rs` module. + +## The pure decision core: `reconcile_doc_prs` + +```rust +/// Pure: given the current open-PR listing for one repo and the resolved +/// auto-doc author identity, decide which single auto-doc PR to keep (canonical) +/// and which to close (with a reason). Performs NO I/O. Non-auto-doc PRs are +/// ignored entirely. +pub fn reconcile_doc_prs(open_prs: &[OpenPrSummary], expected_author: &str) -> DocPrReconcileDecision; +``` + +Algorithm: + +1. Filter `open_prs` to auto-doc candidates via [`is_auto_doc_pr`](#the-composite-identity-gate-is_auto_doc_pr). +2. If there are **zero or one** candidates, the invariant already holds — return + a decision with no closes (and the single candidate, if any, as canonical). +3. Otherwise select the **canonical** PR: the newest (highest number / most + recent) candidate. It is the keeper. +4. Every other candidate is queued for close, tagged with its + [`CloseReason`](#docprreconciledecision): + - `SupersededDuplicate` — an older duplicate, superseded by canonical. + - `StaleConflictingDraft` — a candidate whose `mergeable` state is + `CONFLICTING`. + +The canonical PR is **never** placed in the close set, so the decision can never +close every candidate. + +## `DocPrReconcileDecision` + +```rust +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DocPrReconcileDecision { + /// The PR kept open (the single-open invariant's survivor), if any candidate exists. + pub canonical: Option, + /// PRs to close, each with the reason it was superseded/auto-closed. + pub to_close: Vec, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DocPrClose { + pub number: u32, + pub reason: CloseReason, + /// Comment posted on close, e.g. "superseded by #". + pub comment: String, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum CloseReason { + SupersededDuplicate, + StaleConflictingDraft, +} +``` + +## The executor: `run_doc_pr_reconcile` + +```rust +/// Apply a reconciliation to one repo: resolve the auto-doc author identity +/// (`config::automerge_author`) at this I/O boundary, list open PRs, compute the +/// pure decision, then execute the closes by NUMBER via `close_pr`. Bounded and +/// IO-guarded; returns a structured report for the overseer journal/audit. +pub fn run_doc_pr_reconcile( + repo: &str, + gh: &dyn PrGhClient, +) -> SimardResult; + +/// Testable core: the auto-doc author identity is INJECTED so this stays free of +/// env access. `run_doc_pr_reconcile` is a thin wrapper that resolves +/// `automerge_author().unwrap_or_default()` and delegates here. +pub fn run_doc_pr_reconcile_with_author( + repo: &str, + gh: &dyn PrGhClient, + expected_author: &str, +) -> SimardResult; +``` + +Behavior: + +- Resolves the expected auto-doc author from `config::automerge_author()` + (env `SIMARD_AUTOMERGE_AUTHOR`); unset ⇒ empty ⇒ the gate matches nothing + (fail-closed inert pass). +- Lists open PRs via `gh.list_open_prs(repo, limit)`. +- **Fail-closed on read error:** a listing failure surfaces the error and performs + **no** closes that cycle. +- **LOUD inert-gate surfacing:** if title-marker auto-doc PRs are present but NONE + pass the full identity gate, emits a `warn` naming the likely cause (mis-resolved + `SIMARD_AUTOMERGE_AUTHOR`, missing label, or non-draft) so a silently inert pass + never lets the stale-PR churn recur unseen. +- Computes [`reconcile_doc_prs`](#the-pure-decision-core-reconcile_doc_prs) and, + for each `DocPrClose`, calls `gh.close_pr(repo, number, &comment)` **by number**. +- Processes a **bounded** batch per cycle (never an unbounded storm of closes). +- Emits an OTel/structured-tracing event per keep/close decision — no + `print!`/`println!`. +- Returns `DocPrReconcileReport { canonical, closed: Vec, skipped, errors }` + for the journal. + +## `PrGhClient::close_pr` + +Added additively to the existing +[`PrGhClient`](./cross-repo-merge-authority.md) trait: + +```rust +pub trait PrGhClient { + // … existing methods … + + /// Close PR `number` in `repo` with an explanatory `comment`. The default + /// is a NO-OP so every existing fake / unwired client performs no mutation; + /// only `RealPrGhClient` overrides it. Argv-only (`gh pr close + /// --repo --comment `) — never shell-interpolated, and + /// structurally incapable of carrying `--admin` / `--no-verify`. + fn close_pr(&self, _repo: &str, _number: u32, _comment: &str) -> SimardResult<()> { + Ok(()) // no-op default (fail-safe: unwired clients never mutate) + } +} +``` + +`RealPrGhClient::close_pr` builds a positional argv and shells out to `gh`; the +mutation acts on a specific PR **number** only. + +## Overseer cycle wiring + +`run_doc_pr_reconcile` is invoked from the overseer's PR/merge reconciliation +surface in `run_cycle`, behind the same composite fail-closed gate that guards +the overseer's other IO passes. A poll/list error skips the pass for that cycle +(no closes). The pass is additive: it does not change any existing merge or +verify decision. + +## Invariants + +- **≤ 1 open auto-doc PR** after a successful pass (the single-open invariant). +- **Canonical never closed** — the keeper is excluded from the close set by + construction. +- **Human PRs untouched** — the composite gate fails closed; empty author ⇒ + skipped. +- **Mutate by number only**, argv-only, no `--admin`/`--no-verify`, no `sh -c`. +- **Additive / non-breaking** — `close_pr` has a no-op default; `OpenPrSummary` + is read as-is (no field added or removed); no `pub` item is removed; the PRD + is preserved. +- **Bounded & fail-closed** — a read error performs no closes; a bounded batch + per cycle. +- **OTel-only audit** — no `print!`/`println!`. +- **Pure core** — `reconcile_doc_prs` and `is_auto_doc_pr` do no I/O and are + exhaustively unit-tested on fixture PR lists. + +## Scope and out-of-scope (deliberate fail-safe boundary) + +Reconciliation acts **only** on PRs that pass the full identity gate: +correctly-labeled auto-doc **drafts** authored by the resolved auto-doc identity +(`SIMARD_AUTOMERGE_AUTHOR`). + +**Out of scope (by design):** legacy title-marker PRs that are non-draft or +unlabeled are intentionally left untouched. The strict gate is the +security-critical guard that guarantees a human (or otherwise non-auto-doc) PR is +never auto-closed, so the pass prefers leaving an ambiguous historical PR open +over ever closing one it cannot positively attribute. When title-marker PRs exist +but none pass the gate, the executor emits a LOUD `warn` (naming the likely +cause: unset/mismatched `SIMARD_AUTOMERGE_AUTHOR`, missing label, or non-draft) so +a silently inert pass never hides recurring churn. + +Draining the legacy unlabeled/non-draft backlog is a possible future enhancement +tracked as a follow-up; it is **not** required for the single-open invariant on +newly-generated auto-doc drafts, which this module enforces. diff --git a/docs/reference/ooda-breaker-churn-suppression-api.md b/docs/reference/ooda-breaker-churn-suppression-api.md new file mode 100644 index 000000000..d9c7cd3f5 --- /dev/null +++ b/docs/reference/ooda-breaker-churn-suppression-api.md @@ -0,0 +1,321 @@ +--- +title: OODA breaker churn-suppression API reference +description: > + Reference for the terminal-quarantine rung and reblock-issue signature + stabilization that end the OODA no-progress breaker's `UNCLEAR-CRITERIA` + churn. Specifies the `NoProgressResolution::QuarantineTerminal` variant, the additive + `surfaced_failures` argument on `resolution_for_why`, the durable + `ooda-breaker-quarantine` marker and its predicate, the + `apply_resolution_side_effects` handling that replaces the adapter's + escalate-at-limit branch, the churn-stopping re-schedule exclusion in + `reinvestigate_bare_blocked_goals`, and the `fold_volatile_goal_ids` + stabilization applied in `problem_to_run_brief` that stops the Overseer's + observed `recurring_goal_reblock in simard::overseer` stewardship-issue + churn. +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ../concepts/ooda-breaker-churn-suppression.md + - ../concepts/no-progress-breaker-storm-suppression.md + - ../concepts/no-progress-terminal-investigation.md + - ../concepts/overseer-root-cause-why.md + - ./no-progress-breaker-api.md + - ./no-progress-root-cause-resolution-api.md + - ./no-progress-breaker-storm-suppression-api.md + - ../howto/quarantine-and-recover-an-unclear-ooda-goal.md + - ../../src/goal_curation/no_progress_breaker.rs + - ../../src/goal_curation/no_progress_why.rs + - ../../src/ooda_loop/no_progress.rs + - ../../src/overseer/observer.rs + - ../../src/stewardship/dedup.rs +--- + +# OODA breaker churn-suppression API reference + +> **Status: implemented.** The terminal-quarantine variant, the additive +> `surfaced_failures` argument on `resolution_for_why`, the quarantine-marker +> constants, and the quarantine predicate live in +> [`src/goal_curation/no_progress_breaker.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/no_progress_breaker.rs). +> The side-effect handler (which **replaces** the adapter's inline +> escalate-at-limit branch) and the churn-stopping re-schedule exclusion live in +> [`src/ooda_loop/no_progress.rs`](https://github.com/rysweet/Simard/blob/main/src/ooda_loop/no_progress.rs). +> The reblock-issue signature stabilization is the new `fold_volatile_goal_ids` +> helper applied inside `problem_to_run_brief` in +> [`src/overseer/observer.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/observer.rs), +> upstream of the existing +> [`src/stewardship/dedup.rs`](https://github.com/rysweet/Simard/blob/main/src/stewardship/dedup.rs) +> `failure_signature`. + +For the rationale, see +[The OODA breaker quarantines terminal UNCLEAR-CRITERIA goals](../concepts/ooda-breaker-churn-suppression.md). + +## Contents + +- [Terminal-quarantine constants](#terminal-quarantine-constants) +- [`NoProgressResolution::QuarantineTerminal`](#noprogressresolutionquarantineterminal) +- [`resolution_for_why`](#resolution_for_why) +- [Quarantine marker helpers](#quarantine-marker-helpers) +- [Side effects: `apply_resolution_side_effects`](#side-effects-apply_resolution_side_effects) +- [Re-schedule exclusion: `reinvestigate_bare_blocked_goals`](#re-schedule-exclusion-reinvestigate_bare_blocked_goals) +- [Reblock-issue signature stabilization](#reblock-issue-signature-stabilization) +- [Invariants](#invariants) + +## Terminal-quarantine constants + +| Constant | Value | Meaning | +| --- | --- | --- | +| `SURFACED_INVESTIGATION_FAILURE_LIMIT` | `3` | Consecutive evidence-less surfaced failures on an `UNCLEAR-CRITERIA` goal before the terminal-quarantine rung fires. Reused unchanged from the [terminal-investigation bound](./no-progress-reinvestigation-api.md). | +| `NO_PROGRESS_QUARANTINE_MARKER_KIND` | `"ooda-breaker-quarantine"` | The `WipRef.kind` of the durable quarantine marker. A novel kind, ignored by every other `wip_refs` consumer via its `_ => None` fall-through. | +| `NO_PROGRESS_QUARANTINE_MARKER_REF_ID` | fixed sentinel string | The `WipRef.ref_id` of the quarantine marker. A compile-time constant — **never** derived from goal text — so goal descriptions cannot forge or smuggle content into a quarantine marker. | + +These sit alongside the existing `NO_PROGRESS_BREAKER_THRESHOLD` (`3`) and the +[storm-suppression marker](./no-progress-breaker-storm-suppression-api.md) +constants; quarantine is an additive terminal rung above them, not a change to +either. + +## `NoProgressResolution::QuarantineTerminal` + +A new terminal variant of the pure resolution enum returned by +[`resolution_for_why`](#resolution_for_why): + +```rust +pub enum NoProgressResolution { + // … existing variants (MarkDone, Drop, Defer, SpawnEngineer, + // Escalate, SurfaceInvestigationFailure) … + + /// Terminal park for a goal that has exhausted the guided-retry ladder and + /// the surfaced-failure bound on an UNCLEAR-CRITERIA WHY. Stops both + /// re-filing AND re-scheduling. Reversible by an operator un-block. + QuarantineTerminal { + /// Consecutive evidence-less surfaced failures that drove the goal here. + /// Rendered as real evidence in the Blocked reason — NEVER `(none)`. + surfaced_count: u32, + }, +} + +impl NoProgressResolution { + /// `true` for `Escalate`, `SurfaceInvestigationFailure`, and + /// `QuarantineTerminal` — resolutions that place a goal in a terminal or + /// human-facing state. + pub fn is_terminal(&self) -> bool { /* … */ } +} +``` + +`is_terminal()` returns `true` for `QuarantineTerminal`. The variant carries +`surfaced_count` so the authored Blocked reason renders concrete evidence and +never `evidence=[(none)]`, preserving the +[never-empty-evidence invariant](../concepts/no-progress-terminal-investigation.md#the-rule-never-evidencenone). + +## `resolution_for_why` + +The existing pure ladder function keeps its three current parameters and gains +**one** additive trailing parameter, `surfaced_failures`: + +```rust +pub fn resolution_for_why( + consecutive: u32, + why: NoProgressWhy, + guided_retry_used: bool, + surfaced_failures: u32, // ← added; the goal's persisted surfaced-failure count +) -> NoProgressResolution; +``` + +`consecutive`, `why` (by value), and `guided_retry_used` are unchanged from the +current signature — nothing is removed or changed to by-reference. The caller in +`apply_resolution_side_effects` reads the goal's persisted surfaced-failure count +from `NoProgressTracker` and passes it as `surfaced_failures`. + +Behavior (only the evidence-less terminal rung changes; everything else is +identical to today): + +| WHY class | `guided_retry_used` | `why.evidence` | `surfaced_failures` | Result | +| --- | --- | --- | --- | --- | +| `AlreadyComplete` / `Obsolete` / `MissingPrecondition` / `UpstreamDependency` | any | any | any | unchanged (auto-complete / drop / self-heal / defer) | +| `UnclearCriteria` / `GenuinelyStuck` | `false` | any | any | unchanged (`SpawnEngineer` — the one-shot guided retry) | +| `UnclearCriteria` / `GenuinelyStuck` | `true` | non-empty | any | unchanged (evidence-backed `Escalate`) | +| `UnclearCriteria` / `GenuinelyStuck` | `true` | empty | `< SURFACED_INVESTIGATION_FAILURE_LIMIT` | unchanged (`SurfaceInvestigationFailure` — surface for retry) | +| `UnclearCriteria` / `GenuinelyStuck` | `true` | empty | `>= SURFACED_INVESTIGATION_FAILURE_LIMIT` | **`QuarantineTerminal { surfaced_count: surfaced_failures }`** (new) | + +Quarantine is reachable **only** on the evidence-less terminal rung — i.e. +strictly after the guided engineer has run (`guided_retry_used == true`), the +investigation produced no evidence (`why.evidence.is_empty()`), and the +surfaced-failure count has reached the bound. Below the bound the function still +returns `SurfaceInvestigationFailure` exactly as it does today. It is purely a +function of its inputs (no I/O), so it is exhaustively unit-tested. + +> **This replaces the adapter's escalate-at-limit branch.** Today the +> `SURFACED_INVESTIGATION_FAILURE_LIMIT` check lives in the +> `SurfaceInvestigationFailure` handler of `apply_resolution_side_effects` +> (`src/ooda_loop/no_progress.rs`, ~L1312): after incrementing the surfaced +> counter it calls `surfaced_failure_escalation_issue` and escalates in place. +> With quarantine, that limit decision moves **up** into `resolution_for_why` +> (which now returns `QuarantineTerminal` at the bound) and the adapter's inline +> escalate-at-limit branch is **removed and replaced** by the `QuarantineTerminal` +> handler below. The below-limit `SurfaceInvestigationFailure` handling (record +> the surfaced failure, surface for retry) stays. Without this replacement a +> bounded-out goal would both escalate (old branch) **and** quarantine (new +> branch) — the two must not coexist. + +## Quarantine marker helpers + +```rust +/// Build the durable quarantine `WipRef` for a goal. `ref_id` is the fixed +/// sentinel constant; `kind` is `NO_PROGRESS_QUARANTINE_MARKER_KIND`. +pub fn quarantine_marker() -> WipRef; + +/// True when `wip` is the breaker-authored quarantine marker. Matches on the +/// fixed `kind` + sentinel `ref_id` ONLY — never on goal-derived text. +pub fn is_quarantine_ref(wip: &WipRef) -> bool; + +/// True when `goal` carries the quarantine marker in its `wip_refs`. +pub fn is_quarantined(goal: &ActiveGoal) -> bool; +``` + +`is_quarantine_ref` is injection-safe by construction: it keys on the fixed +sentinel, so no goal description can forge a quarantine or clear another goal's. + +## Side effects: `apply_resolution_side_effects` + +The curate-phase adapter reads the goal's persisted surfaced-failure count from +`NoProgressTracker`, passes it into [`resolution_for_why`](#resolution_for_why), +and — where that function now returns `QuarantineTerminal` — handles it by: + +1. **Blocking the goal** with a WHY-bearing reason built from the breaker prefix + plus the `surfaced_count` as evidence (never `(none)`). +2. **Writing the quarantine marker idempotently** through the existing atomic, + single-writer goal-board save path. A goal already carrying the marker is not + re-written (no duplicate `WipRef`). +3. **Reusing the escalation idempotence** of the + [storm-suppression marker](./no-progress-breaker-storm-suppression-api.md) so a + quarantined goal is never re-filed. + +This `QuarantineTerminal` handler **replaces** the adapter's prior inline +escalate-at-limit branch (the `surfaced >= SURFACED_INVESTIGATION_FAILURE_LIMIT` +check that called `surfaced_failure_escalation_issue` and escalated at ~L1312). +The below-limit `SurfaceInvestigationFailure` path — record the surfaced failure +and surface it for retry — is unchanged. + +**Fail-closed:** if the marker write returns `Err`, the adapter records the error +(fail visible), takes **no** terminal claim, and leaves the goal retriable so the +quarantine is re-attempted next cycle — it never silently proceeds as if the goal +were quarantined when the durable marker did not land. + +## Re-schedule exclusion: `reinvestigate_bare_blocked_goals` + +This is the single change that stops the churn. The re-investigation pass that +sweeps blocked goals back into the breaker each cycle now **skips any goal for +which `is_quarantined(goal)` is true**: + +```rust +// inside reinvestigate_bare_blocked_goals(…) +if is_quarantined(goal) { + continue; // terminal — never re-schedule, re-classify, or re-escalate +} +``` + +A quarantined goal is therefore never re-selected, so it produces no further +`ooda-stuck` escalation and no further re-block for the Overseer to observe. The +exclusion composes with the existing +[bare/evidence-less selection predicates](../concepts/no-progress-terminal-investigation.md#the-stranded-already-blocked-population): +quarantine is checked first and short-circuits. + +## Reblock-issue signature stabilization + +`recurring_goal_reblock in simard::overseer` is **not a code constant** — it is +the observed `dedup_key` / issue-title text that the Overseer produces when it +re-observes a goal being re-blocked (the string seen in the field and in the +tests). The stewardship issue for it is deduplicated through the existing +failure-signature path, **not** through `root_cause_signature`: + +```text +observer.rs::problem_to_run_brief(problem) + → OrchestratorRunBrief { + failure_kind: problem.dedup_key, // ← used RAW downstream + error_text: stable_error_text(problem), + … + } + → stewardship::process_orchestrator_run + → dedup::failure_signature(failure_kind, error_text) +``` + +`dedup::failure_signature` (in `src/stewardship/dedup.rs`) SHA-256s +`failure_kind` **verbatim** and normalizes **only** `error_text` (its private +`normalize_for_signature` redacts UUIDs in the message). So when +`problem.dedup_key` embeds a volatile goal identifier, `failure_kind` drifts +every cycle, the signature drifts with it, and each re-observation files a fresh +issue — the churn. + +> `root_cause_signature` (in `src/overseer/root_cause.rs`) is exported but has +> **no non-test caller** keying the reblock issue. It is **not** the reblock +> dedup key and is intentionally left untouched by this feature. Do not confuse +> it with the `failure_signature` path above. + +### The fix: fold volatile goal ids *before* `failure_kind` + +The stabilization is applied **upstream** of `failure_signature`, inside +`problem_to_run_brief`, so the already-normalized key flows through the existing +dedup unchanged: + +```rust +// src/overseer/observer.rs — inside problem_to_run_brief(problem) +OrchestratorRunBrief { + // was: failure_kind: problem.dedup_key.clone(), + failure_kind: fold_volatile_goal_ids(&problem.dedup_key), + error_text: stable_error_text(problem), + // …unchanged… +} +``` + +`fold_volatile_goal_ids` is a **new** pure, total helper. It is **deliberately +named to avoid the existing private `dedup::normalize_for_signature`** (which is +the message UUID-redactor used inside `failure_signature`, a different function +with a different purpose in a different module). Keeping the names distinct +prevents the collision the review flagged. + +```rust +/// Fold volatile goal identifiers in a dedup key to stable placeholders so +/// recurrences of the SAME re-block cause share one `failure_signature`. +/// Conservative: only the known volatile shapes are rewritten; everything else +/// is returned byte-for-byte, so distinct causes keep distinct signatures. +pub fn fold_volatile_goal_ids(dedup_key: &str) -> String; +``` + +| Volatile shape | Folded to | +| --- | --- | +| `simard-identity-` | `simard-identity-*` | +| `goal-` (positional slug) | `goal-*` | + +> **Ownership.** `fold_volatile_goal_ids` lives in `src/overseer/observer.rs` +> alongside its only caller, `problem_to_run_brief`. It does **not** extend or +> shadow `dedup::normalize_for_signature`; if a future change instead wants the +> fold to apply to *every* `failure_signature` caller, it would extend +> `dedup::failure_signature` to normalize `failure_kind` — but that broader +> change is out of scope here. + +Because `failure_kind` is now stable across re-observations of the same +underlying re-block cause, the stewardship dedup in `src/stewardship/dedup.rs` +collapses every recurrence onto **one** open issue via the existing +`find_existing` + `stewardship-signature:` mechanism, instead of one issue per +cycle. + +## Invariants + +- **Additive / non-breaking.** No `pub` item is removed; `resolution_for_why` + gains a parameter and `NoProgressResolution` gains a variant (an intentional, + compiler-checked exhaustiveness change). The PRD is preserved. +- **Never `evidence=[(none)]`.** `QuarantineTerminal` always renders the + surfaced count as real evidence. +- **Injection-safe.** The quarantine marker keys on a fixed sentinel, never on + goal-derived text. +- **Fail-closed & fail-visible.** A failed marker write surfaces the error and + leaves the goal retriable; it never fakes a quarantine. +- **Reversible.** An operator un-block clears the marker and resets the + surfaced-failure counter, granting a fresh bounded window. +- **Bounded, not rate-limited.** Quarantine is a hard terminal stop after a + finite bound, not a backoff. +- **Scope-disjoint from admission.** No goal-admission-gate code (PRs + #4939/#4941) is touched; this owns only residual re-schedule + reblock-dedup. +- **No `print!`/`println!`.** All audit output is structured tracing / OTel. diff --git a/mkdocs.yml b/mkdocs.yml index ada78cf61..293184552 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -147,6 +147,8 @@ nav: - No-Progress Breaker Explains WHY and Self-Resolves: concepts/no-progress-root-cause-resolution.md - Terminal No-Progress Stall Never Parks Empty Evidence: concepts/no-progress-terminal-investigation.md - No-Progress Breaker Suppresses Its Own Issue Storm: concepts/no-progress-breaker-storm-suppression.md + - OODA Breaker Quarantines Terminal Unclear Goals: concepts/ooda-breaker-churn-suppression.md + - Overseer Reconciles Auto-Doc PRs to One Open: concepts/auto-doc-pr-reconciliation.md - 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 @@ -201,6 +203,8 @@ nav: - Unblock Stuck OODA Goals: howto/unblock-stuck-ooda-goals.md - Diagnose a No-Progress Block and Read Its WHY: howto/diagnose-a-no-progress-block.md - Diagnose a No-Progress Breaker Issue Storm: howto/diagnose-a-no-progress-breaker-issue-storm.md + - Quarantine and Recover an Unclear OODA Goal: howto/quarantine-and-recover-an-unclear-ooda-goal.md + - Reconcile Stale Auto-Doc PRs: howto/reconcile-stale-auto-doc-prs.md - Re-Investigate Bare-Blocked OODA Goals: howto/reinvestigate-bare-blocked-goals.md - Triage a Blocked-Goal Escalation (Before It Reaches You): howto/triage-a-blocked-goal-escalation.md - Add a New Recipe-Brain Phase: howto/add-a-new-recipe-brain-phase.md @@ -457,6 +461,8 @@ nav: - Durable OODA Cycle Counter API: reference/durable-ooda-cycle-counter.md - No-Progress Re-Investigation API: reference/no-progress-reinvestigation-api.md - No-Progress Breaker Issue-Storm Suppression API: reference/no-progress-breaker-storm-suppression-api.md + - OODA Breaker Churn-Suppression API: reference/ooda-breaker-churn-suppression-api.md + - Auto-Doc PR Reconciliation API: reference/auto-doc-pr-reconciliation-api.md - Progress-Evidence API: reference/progress-evidence-api.md - Trustworthy-Confidence API: reference/trustworthy-confidence-api.md - External-Signal Completion Gate: reference/external-signal-completion-gate.md diff --git a/src/goal_curation/mod.rs b/src/goal_curation/mod.rs index db1b3baba..e2fe3e449 100644 --- a/src/goal_curation/mod.rs +++ b/src/goal_curation/mod.rs @@ -59,10 +59,12 @@ pub use completion_gate::{ pub use no_progress_breaker::{ NO_PROGRESS_BLOCKED_PREFIX, NO_PROGRESS_BLOCKED_SUFFIX, NO_PROGRESS_BREAKER_THRESHOLD, - NoProgressResolution, NoProgressTracker, StuckGoalDisposition, humanize_block_reason, - is_bare_no_progress_block, is_no_progress_marker, no_progress_blocked_reason, - no_progress_blocked_reason_with_why, obsolescence_reason, resolution_for_why, - resolve_no_progress, verify_stuck_goal, + NO_PROGRESS_QUARANTINE_MARKER_KIND, NO_PROGRESS_QUARANTINE_MARKER_REF_ID, NoProgressResolution, + NoProgressTracker, SURFACED_INVESTIGATION_FAILURE_LIMIT, StuckGoalDisposition, + humanize_block_reason, is_bare_no_progress_block, is_no_progress_marker, is_quarantine_ref, + is_quarantined, no_progress_blocked_reason, no_progress_blocked_reason_with_why, + obsolescence_reason, quarantine_marker, resolution_for_why, resolve_no_progress, + verify_stuck_goal, }; pub use no_progress_why::{Evidence, NoProgressClass, NoProgressWhy, NoProgressWhyReasoner}; @@ -89,6 +91,12 @@ mod tests_no_progress_breaker; // WHY-aware block-reason renderer, and the class -> resolution map). #[cfg(test)] mod tests_no_progress_why; +// process_health (TDD): pure-policy tests for the OODA breaker terminal-quarantine +// rung that ends the UNCLEAR-CRITERIA churn — the additive `surfaced_failures` +// argument on `resolution_for_why`, the `QuarantineTerminal` variant, and the +// durable injection-safe quarantine marker helpers. +#[cfg(test)] +mod tests_quarantine; // Issue #17 (TDD): pure primitives of the already-blocked re-investigation pass // — the `is_bare_no_progress_block` deterministic rail and the // `NoProgressTracker` persisted `reinvestigated` dedupe set (lifecycle + serde). diff --git a/src/goal_curation/no_progress_breaker.rs b/src/goal_curation/no_progress_breaker.rs index 20fc64ff3..e12df17f6 100644 --- a/src/goal_curation/no_progress_breaker.rs +++ b/src/goal_curation/no_progress_breaker.rs @@ -51,7 +51,7 @@ use std::collections::{HashMap, HashSet}; use super::completion_gate::{CompletionEvidenceGate, CompletionVerdict, EvidenceSource}; use super::no_progress_why::{Evidence, NoProgressClass, NoProgressWhy}; -use super::types::ActiveGoal; +use super::types::{ActiveGoal, WipRef}; /// Consecutive no-action cycles on one goal before the breaker fires. Kept /// deliberately small (2–3) so a livelock is broken quickly, matching the @@ -297,6 +297,27 @@ pub enum NoProgressResolution { class: NoProgressClass, reason: String, }, + /// `UNCLEAR-CRITERIA` / `GENUINELY-STUCK`, **terminal quarantine** rung + /// (process_health): the goal has spent its guided retry AND surfaced an + /// evidence-less terminal failure at least + /// [`SURFACED_INVESTIGATION_FAILURE_LIMIT`] times — its done-criteria are + /// *permanently* unclear. Surfacing-and-retrying forever is the churn this + /// ends (~13 `ooda-stuck` "goal stuck after guided retry (UNCLEAR-CRITERIA)" + /// issues in a single field day). Instead of surfacing again the breaker + /// terminally quarantines the goal: the caller Blocks it with a WHY-bearing + /// reason that renders `surfaced_count` as REAL evidence (never + /// `evidence=[(none)]`), writes the durable, injection-safe + /// [`quarantine_marker`] so [`is_quarantined`] reads `true`, and files exactly + /// one deduplicated tracking issue. A quarantined goal is thereafter never + /// re-scheduled, re-classified, or re-filed by + /// [`reinvestigate_bare_blocked_goals`](crate::ooda_loop::no_progress::reinvestigate_bare_blocked_goals), + /// so the duplicate-issue storm stops. This **replaces** the adapter's prior + /// inline escalate-at-limit branch — the two must not coexist. + /// + /// `surfaced_count` is the goal's persisted consecutive surfaced-failure count + /// (`>= SURFACED_INVESTIGATION_FAILURE_LIMIT`), carried so the authored block + /// reason renders a concrete, positive count as evidence. + QuarantineTerminal { surfaced_count: u32 }, } /// How many **consecutive** evidence-less @@ -319,6 +340,50 @@ pub enum NoProgressResolution { /// measurable. pub const SURFACED_INVESTIGATION_FAILURE_LIMIT: u32 = 3; +/// The `WipRef.kind` of the durable OODA breaker **terminal-quarantine** marker +/// (process_health). A novel kind string that no other `wip_refs` consumer +/// matches (they fall through their `kind` `match` arms), so attaching it is +/// inert to every other subsystem — it exists solely for [`is_quarantined`]. +pub const NO_PROGRESS_QUARANTINE_MARKER_KIND: &str = "ooda-breaker-quarantine"; + +/// The `WipRef.ref_id` of the quarantine marker: a **fixed compile-time +/// sentinel**, never derived from goal text. [`is_quarantine_ref`] keys on this +/// exact value, so a goal description / activity can neither forge a quarantine +/// for itself nor smuggle attacker-controlled content into a marker — the +/// injection-safety invariant the tests pin. +pub const NO_PROGRESS_QUARANTINE_MARKER_REF_ID: &str = "ooda-breaker-quarantine::terminal-sentinel"; + +/// Build the durable [`WipRef`] that marks a goal terminally quarantined by the +/// no-progress breaker. The `ref_id` is the fixed +/// [`NO_PROGRESS_QUARANTINE_MARKER_REF_ID`] sentinel (never goal-derived) and the +/// `kind` is [`NO_PROGRESS_QUARANTINE_MARKER_KIND`], so the marker is recognised +/// only by [`is_quarantine_ref`] and can never be forged from goal text. +pub fn quarantine_marker() -> WipRef { + WipRef { + kind: NO_PROGRESS_QUARANTINE_MARKER_KIND.to_string(), + ref_id: NO_PROGRESS_QUARANTINE_MARKER_REF_ID.to_string(), + label: "OODA breaker terminal quarantine (UNCLEAR-CRITERIA after guided retry)".to_string(), + url: None, + } +} + +/// True when `wip` is the breaker-authored quarantine marker. Keys on BOTH the +/// fixed [`NO_PROGRESS_QUARANTINE_MARKER_KIND`] and the fixed sentinel +/// [`NO_PROGRESS_QUARANTINE_MARKER_REF_ID`] `ref_id` — never on goal-derived +/// text — so a `WipRef` carrying the right `kind` but a goal-supplied `ref_id` +/// can never be mistaken for a quarantine marker (injection-safe). +pub fn is_quarantine_ref(wip: &WipRef) -> bool { + wip.kind == NO_PROGRESS_QUARANTINE_MARKER_KIND + && wip.ref_id == NO_PROGRESS_QUARANTINE_MARKER_REF_ID +} + +/// True when `goal` carries the durable quarantine marker in its `wip_refs`. +/// Only the breaker-authored fixed-sentinel marker counts — no goal-derived +/// `WipRef` can forge a quarantine. +pub fn is_quarantined(goal: &ActiveGoal) -> bool { + goal.wip_refs.iter().any(is_quarantine_ref) +} + impl NoProgressResolution { /// `true` for every resolution that removes the goal from the no-action loop /// with a definitive action. [`Continue`](Self::Continue) (below threshold) @@ -549,10 +614,31 @@ pub(crate) fn surfaced_failure_escalation_issue( /// engineer ([`NoProgressResolution::SpawnEngineer`]); only once that retry is /// spent and the goal is *still* stuck does it [`Escalate`](NoProgressResolution::Escalate) /// — always WITH the concrete WHY + evidence attached. +/// +/// `surfaced_failures` is the goal's persisted consecutive evidence-less +/// surfaced-failure count as it stands **entering** this cycle — i.e. the +/// PRE-bump count, BEFORE this cycle records its own surfaced failure +/// (process_health). The bound is checked pre-bump +/// (`surfaced_failures >= SURFACED_INVESTIGATION_FAILURE_LIMIT`), which gives an +/// exact firing timing worth spelling out so the doc never reads as +/// fire-at-LIMIT: the evidence-less stall is +/// [`SurfaceInvestigationFailure`](NoProgressResolution::SurfaceInvestigationFailure)d +/// (retriable, one recorded surfaced failure per cycle) for its first +/// `SURFACED_INVESTIGATION_FAILURE_LIMIT` observations, and only the NEXT +/// (`LIMIT + 1`)th observation — where the pre-bump count has finally reached the +/// bound — returns [`NoProgressResolution::QuarantineTerminal`], the terminal +/// stop that ends the `UNCLEAR-CRITERIA` re-schedule/re-file churn. This is a +/// DELIBERATE one-observation shift from the old post-bump escalate-at-LIMIT +/// trigger (which fired ON the LIMIT-th observation); it is pinned end-to-end by +/// `quarantine_fires_on_the_cycle_after_the_limit_th_surface`. Below the bound +/// the surfacing rung's *action* is unchanged (still a plain surfaced failure); +/// only the trigger observation moved by one. The parameter is ignored for every +/// other class and for evidence-backed stalls. pub fn resolution_for_why( consecutive: u32, why: NoProgressWhy, guided_retry_used: bool, + surfaced_failures: u32, ) -> NoProgressResolution { match why.class { NoProgressClass::AlreadyComplete => NoProgressResolution::MarkDone, @@ -573,10 +659,16 @@ pub fn resolution_for_why( // and the goal is still stuck. NEVER author a bare // `evidence=[(none)]` block — the exact live-daemon defect. If the // investigation produced concrete evidence, escalate WITH it; - // otherwise surface the evidence-less outcome as an investigation - // failure (fail visible + retriable), taking no bare terminal - // action. + // otherwise the outcome is evidence-less: quarantine terminally + // once the surfaced-failure bound is reached (process_health — + // ends the re-schedule/re-file churn), else surface it as a + // retriable investigation failure exactly as before. if why.evidence.is_empty() { + if surfaced_failures >= SURFACED_INVESTIGATION_FAILURE_LIMIT { + return NoProgressResolution::QuarantineTerminal { + surfaced_count: surfaced_failures, + }; + } return NoProgressResolution::SurfaceInvestigationFailure { class: why.class, reason: format!( diff --git a/src/goal_curation/tests_no_progress_why.rs b/src/goal_curation/tests_no_progress_why.rs index 205ac50bb..80d7812e8 100644 --- a/src/goal_curation/tests_no_progress_why.rs +++ b/src/goal_curation/tests_no_progress_why.rs @@ -191,7 +191,7 @@ fn why_aware_block_reason_preserves_sentinel_recognition_and_count_parse() { fn already_complete_maps_to_mark_done() { let why = NoProgressWhy::new(NoProgressClass::AlreadyComplete, already_done_evidence()); assert_eq!( - resolution_for_why(NO_PROGRESS_BREAKER_THRESHOLD, why, false), + resolution_for_why(NO_PROGRESS_BREAKER_THRESHOLD, why, false, 0), NoProgressResolution::MarkDone, "a goal proven done by live artifacts must auto-complete, not block" ); @@ -203,7 +203,7 @@ fn missing_precondition_maps_to_heal() { NoProgressClass::MissingPrecondition, vec![Evidence::new("repo", "kgpacks-rs", "absent")], ); - match resolution_for_why(NO_PROGRESS_BREAKER_THRESHOLD, why, false) { + match resolution_for_why(NO_PROGRESS_BREAKER_THRESHOLD, why, false, 0) { NoProgressResolution::Heal { why } => { assert_eq!(why.class, NoProgressClass::MissingPrecondition); } @@ -217,7 +217,7 @@ fn upstream_dependency_maps_to_defer_with_the_blocking_ref() { NoProgressClass::UpstreamDependency, vec![Evidence::new("dependency-goal", "upstream-goal", "OPEN")], ); - match resolution_for_why(NO_PROGRESS_BREAKER_THRESHOLD, why, false) { + match resolution_for_why(NO_PROGRESS_BREAKER_THRESHOLD, why, false, 0) { NoProgressResolution::Defer { blocking_ref, .. } => { assert!( blocking_ref.contains("upstream-goal"), @@ -241,6 +241,7 @@ fn unclear_or_stuck_maps_to_spawn_engineer_on_the_first_attempt() { NO_PROGRESS_BREAKER_THRESHOLD, why, /* guided_retry_used */ false, + /* surfaced_failures */ 0, ) { NoProgressResolution::SpawnEngineer { task, .. } => { assert!( @@ -268,6 +269,7 @@ fn stuck_escalates_with_why_only_after_the_guided_retry_is_exhausted() { NO_PROGRESS_BREAKER_THRESHOLD, why, /* guided_retry_used */ true, + /* surfaced_failures */ 0, ) { NoProgressResolution::Escalate { blocked_reason, @@ -323,6 +325,7 @@ fn terminal_stuck_with_no_evidence_surfaces_a_failure_never_a_none_block() { NO_PROGRESS_BREAKER_THRESHOLD, why, /* guided_retry_used */ true, + /* surfaced_failures (below the bound ⇒ still surfaces) */ 0, ) { NoProgressResolution::SurfaceInvestigationFailure { class: surfaced_class, diff --git a/src/goal_curation/tests_quarantine.rs b/src/goal_curation/tests_quarantine.rs new file mode 100644 index 000000000..dff7fbda8 --- /dev/null +++ b/src/goal_curation/tests_quarantine.rs @@ -0,0 +1,315 @@ +//! TEST-FIRST (Step 7 TDD) — pure policy layer of the OODA breaker +//! **terminal-quarantine** rung that ends the `UNCLEAR-CRITERIA` churn +//! (process_health, HIGH). +//! +//! # The defect these tests lock down +//! +//! A goal whose done-criteria are **permanently** unclear rides the breaker +//! ladder to the evidence-less terminal rung, is surfaced for retry, gets +//! re-scheduled, and rides the ladder again — forever. In the field this produced +//! ~13 `ooda-stuck` "goal stuck after guided retry (UNCLEAR-CRITERIA)" issues in a +//! single day. The existing `SurfaceInvestigationFailure` rung, plus the +//! escalate-at-limit branch in the curate-phase adapter, does not *terminally* +//! stop the goal from being re-scheduled — so the churn continues. +//! +//! # The contract (what the fix must make true) — pure layer +//! +//! `resolution_for_why` gains ONE additive trailing parameter, `surfaced_failures`, +//! and a new terminal variant `NoProgressResolution::QuarantineTerminal`: +//! * only the evidence-less terminal rung changes — everything else is +//! byte-for-byte identical to today; +//! * `UNCLEAR-CRITERIA` / `GENUINELY-STUCK`, guided retry spent, evidence EMPTY, +//! `surfaced_failures >= SURFACED_INVESTIGATION_FAILURE_LIMIT` +//! -> `QuarantineTerminal { surfaced_count }`; +//! * below the bound it still returns `SurfaceInvestigationFailure` exactly as +//! today; +//! * `QuarantineTerminal.is_terminal()` is `true`; +//! * `surfaced_count` is carried so the authored block reason renders the count +//! as REAL evidence — NEVER `evidence=[(none)]`. +//! +//! Plus the durable, injection-safe quarantine marker: +//! * `quarantine_marker()` has the fixed `kind` + fixed sentinel `ref_id` +//! (never goal-derived); +//! * `is_quarantine_ref` matches ONLY that fixed identity; +//! * `is_quarantined(goal)` is true iff the goal carries the marker; a +//! goal-derived `WipRef` can never forge it. +//! +//! These are PURE (no I/O) and exhaustively unit-tested. RED until the variant, +//! the extended `resolution_for_why`, and the marker helpers exist. + +use crate::goal_curation::WipRef; +use crate::goal_curation::no_progress_breaker::{ + NO_PROGRESS_BREAKER_THRESHOLD, NO_PROGRESS_QUARANTINE_MARKER_KIND, + NO_PROGRESS_QUARANTINE_MARKER_REF_ID, NoProgressResolution, + SURFACED_INVESTIGATION_FAILURE_LIMIT, is_quarantine_ref, is_quarantined, quarantine_marker, + resolution_for_why, +}; +use crate::goal_curation::no_progress_why::{Evidence, NoProgressClass, NoProgressWhy}; +use crate::goal_curation::types::ActiveGoal; + +// --- fixtures --------------------------------------------------------------- + +fn goal(id: &str) -> ActiveGoal { + ActiveGoal::new(id, "keep the simard identity coherent", 1) +} + +/// An evidence-less WHY for one of the two human-facing classes. +fn evidenceless(class: NoProgressClass) -> NoProgressWhy { + NoProgressWhy::new(class, vec![]) +} + +/// The two classes that ride the guided-retry ladder to the terminal rung. +const HUMAN_FACING: [NoProgressClass; 2] = [ + NoProgressClass::UnclearCriteria, + NoProgressClass::GenuinelyStuck, +]; + +// === resolution_for_why: the new terminal-quarantine rung =================== + +/// The core new behaviour: an evidence-less, guided-retry-spent, at/over-bound +/// stall QUARANTINES terminally, carrying the surfaced count as evidence. +#[test] +fn evidenceless_stall_at_the_bound_quarantines_terminally() { + for class in HUMAN_FACING { + let res = resolution_for_why( + NO_PROGRESS_BREAKER_THRESHOLD, + evidenceless(class), + true, // guided retry spent + SURFACED_INVESTIGATION_FAILURE_LIMIT, // exactly at the bound + ); + match res { + NoProgressResolution::QuarantineTerminal { surfaced_count } => assert_eq!( + surfaced_count, SURFACED_INVESTIGATION_FAILURE_LIMIT, + "{class:?}: quarantine must carry the surfaced count that drove it here" + ), + other => panic!( + "{class:?}: evidence-less terminal rung at the surfaced bound must \ + QuarantineTerminal, got {other:?}" + ), + } + } +} + +/// Above the bound it still quarantines (>=, not ==). +#[test] +fn evidenceless_stall_over_the_bound_still_quarantines() { + let res = resolution_for_why( + NO_PROGRESS_BREAKER_THRESHOLD, + evidenceless(NoProgressClass::UnclearCriteria), + true, + SURFACED_INVESTIGATION_FAILURE_LIMIT + 5, + ); + assert!( + matches!(res, NoProgressResolution::QuarantineTerminal { surfaced_count } if surfaced_count == SURFACED_INVESTIGATION_FAILURE_LIMIT + 5), + "past the bound must still QuarantineTerminal carrying the real count, got {res:?}" + ); +} + +/// Below the bound the rung is UNCHANGED: still a (non-terminal) surfaced +/// investigation failure that lets the goal retry. +#[test] +fn evidenceless_stall_below_the_bound_still_surfaces_not_quarantines() { + for surfaced in 0..SURFACED_INVESTIGATION_FAILURE_LIMIT { + let res = resolution_for_why( + NO_PROGRESS_BREAKER_THRESHOLD, + evidenceless(NoProgressClass::GenuinelyStuck), + true, + surfaced, + ); + assert!( + matches!( + res, + NoProgressResolution::SurfaceInvestigationFailure { .. } + ), + "below the bound (surfaced={surfaced}) must stay SurfaceInvestigationFailure, \ + got {res:?}" + ); + assert!( + !matches!(res, NoProgressResolution::QuarantineTerminal { .. }), + "below the bound must NEVER quarantine (surfaced={surfaced})" + ); + } +} + +/// The guided-retry gate is preserved: before the guided engineer has run, an +/// unclear/stuck stall still spawns the one-shot engineer regardless of the +/// surfaced count — quarantine is unreachable pre-retry. +#[test] +fn quarantine_is_unreachable_before_the_guided_retry_is_spent() { + for class in HUMAN_FACING { + let res = resolution_for_why( + NO_PROGRESS_BREAKER_THRESHOLD, + evidenceless(class), + false, // guided retry NOT yet used + SURFACED_INVESTIGATION_FAILURE_LIMIT + 10, + ); + assert!( + matches!(res, NoProgressResolution::SpawnEngineer { .. }), + "{class:?}: a first-occurrence stall must spawn the guided engineer, \ + never quarantine, got {res:?}" + ); + } +} + +/// An evidence-BACKED terminal stall still escalates WITH the evidence — the +/// surfaced count is ignored when real evidence exists (quarantine is only the +/// evidence-less rung). +#[test] +fn evidence_backed_terminal_stall_still_escalates_not_quarantines() { + let why = NoProgressWhy::new( + NoProgressClass::GenuinelyStuck, + vec![Evidence::new("pr", "#7", "OPEN")], + ); + let res = resolution_for_why( + NO_PROGRESS_BREAKER_THRESHOLD, + why, + true, + SURFACED_INVESTIGATION_FAILURE_LIMIT + 3, // over the bound, but evidence present + ); + assert!( + matches!(res, NoProgressResolution::Escalate { .. }), + "an evidence-backed terminal stall must Escalate WITH evidence, never quarantine, \ + got {res:?}" + ); +} + +/// The machine-resolvable classes are completely unaffected by the new parameter +/// at any surfaced count. +#[test] +fn machine_resolvable_classes_are_unaffected_by_the_surfaced_parameter() { + let cases = [ + (NoProgressClass::AlreadyComplete, "mark done"), + (NoProgressClass::Obsolete, "drop"), + (NoProgressClass::MissingPrecondition, "heal"), + (NoProgressClass::UpstreamDependency, "defer"), + ]; + for (class, label) in cases { + for surfaced in [0, SURFACED_INVESTIGATION_FAILURE_LIMIT, 99] { + let res = resolution_for_why( + NO_PROGRESS_BREAKER_THRESHOLD, + NoProgressWhy::new(class, vec![Evidence::new("issue", "#16", "CLOSED")]), + true, + surfaced, + ); + assert!( + !matches!(res, NoProgressResolution::QuarantineTerminal { .. }), + "{class:?} ({label}) must NEVER quarantine regardless of surfaced={surfaced}, \ + got {res:?}" + ); + } + } +} + +// === is_terminal ============================================================ + +#[test] +fn quarantine_terminal_is_terminal() { + let q = NoProgressResolution::QuarantineTerminal { + surfaced_count: SURFACED_INVESTIGATION_FAILURE_LIMIT, + }; + assert!( + q.is_terminal(), + "QuarantineTerminal places the goal in a terminal state and must be is_terminal()==true" + ); +} + +// === never evidence=[(none)] ================================================ + +/// The carried surfaced count is a positive integer at the bound, so any reason +/// the adapter authors from it renders REAL evidence — the never-`(none)` +/// invariant holds by construction. +#[test] +fn quarantine_carries_a_nonzero_surfaced_count_as_real_evidence() { + let res = resolution_for_why( + NO_PROGRESS_BREAKER_THRESHOLD, + evidenceless(NoProgressClass::UnclearCriteria), + true, + SURFACED_INVESTIGATION_FAILURE_LIMIT, + ); + let NoProgressResolution::QuarantineTerminal { surfaced_count } = res else { + panic!("expected QuarantineTerminal, got {res:?}"); + }; + assert!( + surfaced_count >= SURFACED_INVESTIGATION_FAILURE_LIMIT, + "the surfaced count is the concrete evidence rendered in the block reason — \ + it must be at least the (positive) bound, so it is never zero / (none)" + ); +} + +// === quarantine marker: identity + injection safety ========================= + +#[test] +fn quarantine_marker_has_the_fixed_kind_and_sentinel_ref_id() { + let m = quarantine_marker(); + assert_eq!( + m.kind, NO_PROGRESS_QUARANTINE_MARKER_KIND, + "marker kind must be the fixed quarantine kind" + ); + assert_eq!( + m.ref_id, NO_PROGRESS_QUARANTINE_MARKER_REF_ID, + "marker ref_id must be the compile-time sentinel, never goal-derived" + ); + assert!( + is_quarantine_ref(&m), + "the constructed marker must be recognised by is_quarantine_ref" + ); +} + +#[test] +fn is_quarantine_ref_matches_only_the_fixed_identity() { + // A `WipRef` whose fields are attacker-controlled goal text must NEVER be + // mistaken for the quarantine marker — the predicate keys on the fixed + // sentinel only. + let forged = WipRef { + kind: NO_PROGRESS_QUARANTINE_MARKER_KIND.to_string(), + ref_id: "attacker-supplied-goal-slug".to_string(), + label: "ooda-breaker-quarantine".to_string(), + url: None, + }; + assert!( + !is_quarantine_ref(&forged), + "a WipRef with the right kind but a goal-derived ref_id must NOT be a quarantine marker \ + (injection-safe: identity is the fixed sentinel ref_id)" + ); + + let unrelated = WipRef { + kind: "issue".to_string(), + ref_id: "42".to_string(), + label: "[no-progress-tracking] ooda-breaker".to_string(), + url: None, + }; + assert!( + !is_quarantine_ref(&unrelated), + "an ordinary tracking issue ref is not a quarantine marker" + ); +} + +#[test] +fn is_quarantined_reflects_marker_presence() { + let mut g = goal("simard-identity-atelier-industrial-furniture-de"); + assert!(!is_quarantined(&g), "a fresh goal is not quarantined"); + + g.wip_refs.push(quarantine_marker()); + assert!( + is_quarantined(&g), + "a goal carrying the quarantine marker must read as quarantined" + ); +} + +#[test] +fn a_goal_cannot_forge_its_own_quarantine() { + // The exact injection vector: a goal description / activity that smuggles the + // marker's kind string into a WipRef must not make the goal read quarantined. + let mut g = goal("goal-with-a-sneaky-wipref"); + g.wip_refs.push(WipRef { + kind: NO_PROGRESS_QUARANTINE_MARKER_KIND.to_string(), + ref_id: "not-the-sentinel".to_string(), + label: "pretend quarantine".to_string(), + url: None, + }); + assert!( + !is_quarantined(&g), + "only the breaker-authored fixed-sentinel marker counts — no goal-derived ref can forge \ + a quarantine" + ); +} diff --git a/src/ooda_loop/mod.rs b/src/ooda_loop/mod.rs index dd96afa0e..44a514f0d 100644 --- a/src/ooda_loop/mod.rs +++ b/src/ooda_loop/mod.rs @@ -54,6 +54,13 @@ mod tests_no_progress_investigation; #[cfg(test)] mod tests_no_progress_reinvestigation; +// process_health (TDD): the churn-stopping side effects of the terminal-quarantine +// rung — the re-investigation pass skips quarantined goals (never re-investigates, +// re-schedules, or re-files them) and an at-bound evidence-less UNCLEAR-CRITERIA +// stall is durably marked + blocked with real evidence. +#[cfg(test)] +mod tests_quarantine_churn; + // Issue #16 (follow-up, TDD): direct unit tests for the production // `DeterministicNoProgressReasoner` — pin the terminal-rung invariant that it // never returns an empty-evidence WHY, so the breaker can never author a bare diff --git a/src/ooda_loop/no_progress.rs b/src/ooda_loop/no_progress.rs index fd3968f6d..a64982f7a 100644 --- a/src/ooda_loop/no_progress.rs +++ b/src/ooda_loop/no_progress.rs @@ -22,10 +22,9 @@ use crate::goal_curation::completion_gate::{ CompletionEvidenceGate, CompletionVerdict, DependencyState, EvidenceSource, }; use crate::goal_curation::no_progress_breaker::{ - NO_PROGRESS_BREAKER_THRESHOLD, NoProgressResolution, NoProgressTracker, - SURFACED_INVESTIGATION_FAILURE_LIMIT, needs_reinvestigation, - no_progress_blocked_reason_with_why, obsolescence_reason, resolution_for_why, - surfaced_failure_escalation_issue, verify_stuck_goal, + NO_PROGRESS_BREAKER_THRESHOLD, NoProgressResolution, NoProgressTracker, is_quarantined, + needs_reinvestigation, no_progress_blocked_reason_with_why, obsolescence_reason, + quarantine_marker, resolution_for_why, surfaced_failure_escalation_issue, verify_stuck_goal, }; use crate::goal_curation::no_progress_why::{ Evidence, NoProgressClass, NoProgressWhy, NoProgressWhyReasoner, @@ -366,6 +365,15 @@ 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 **terminally quarantined** (process_health): an `UNCLEAR-CRITERIA` / + /// `GENUINELY-STUCK` goal that stayed evidence-less past + /// [`SURFACED_INVESTIGATION_FAILURE_LIMIT`](crate::goal_curation::no_progress_breaker::SURFACED_INVESTIGATION_FAILURE_LIMIT) + /// surfaced failures. It is Blocked WITH the surfaced count as evidence, the + /// durable [`quarantine_marker`](crate::goal_curation::no_progress_breaker::quarantine_marker) + /// is written, and it is thereafter skipped by + /// [`reinvestigate_bare_blocked_goals`] — ending the re-schedule/re-file + /// churn. A terminal firing. + pub quarantined: Vec, } impl NoProgressBreakerReport { @@ -380,6 +388,7 @@ impl NoProgressBreakerReport { || !self.healed.is_empty() || !self.deferred.is_empty() || !self.engineer_spawned.is_empty() + || !self.quarantined.is_empty() } /// Compact one-line summary for the cycle log. @@ -387,7 +396,7 @@ impl NoProgressBreakerReport { format!( "done={} dropped={} escalated={} healed={} deferred={} engineer={} \ auto_cleared={} reinvestigated={} errors={} perpetual_idled={} \ - research_faults={}", + research_faults={} quarantined={}", self.marked_done.len(), self.dropped.len(), self.escalated.len(), @@ -399,6 +408,7 @@ impl NoProgressBreakerReport { self.investigation_errors.len(), self.perpetual_idled.len(), self.research_idle_faults.len(), + self.quarantined.len(), ) } @@ -904,7 +914,8 @@ pub(crate) fn apply_no_progress_breaker_with_threshold( NoProgressResolution::Heal { .. } | NoProgressResolution::Defer { .. } | NoProgressResolution::SpawnEngineer { .. } - | NoProgressResolution::SurfaceInvestigationFailure { .. } => { + | NoProgressResolution::SurfaceInvestigationFailure { .. } + | NoProgressResolution::QuarantineTerminal { .. } => { tracing::warn!( target: "simard::ooda", goal = %goal_id, @@ -1050,7 +1061,16 @@ pub(crate) fn apply_no_progress_breaker_investigated( }; let guided_retry_used = tracker.guided_retry_used(goal_id); - let resolution = resolution_for_why(consecutive, why, guided_retry_used); + let class = why.class; + // PRE-bump read: this is the surfaced-failure count as it stands ENTERING + // this cycle. `resolution_for_why` compares it `>= LIMIT` before the surface + // arm below records this cycle's own failure via `record_surfaced_failure`, + // so the evidence-less stall is surfaced for its first LIMIT observations and + // quarantine fires on the (LIMIT + 1)th — a deliberate one-observation shift + // from the old post-bump escalate-at-LIMIT trigger, pinned by + // `quarantine_fires_on_the_cycle_after_the_limit_th_surface`. + let surfaced_failures = tracker.surfaced_failures(goal_id); + let resolution = resolution_for_why(consecutive, why, guided_retry_used, surfaced_failures); // On-transition path: the goal is not in a Blocked state, so the // non-terminal `Heal` / `SpawnEngineer` rungs leave its status untouched @@ -1059,6 +1079,7 @@ pub(crate) fn apply_no_progress_breaker_investigated( state, goal_id, consecutive, + class, resolution, healer, dispatcher, @@ -1090,6 +1111,9 @@ pub(crate) fn apply_no_progress_breaker_investigated( /// re-investigation pass ([`reinvestigate_bare_blocked_goals`], issue #17) so the /// class → action mapping can never drift between the two populations. /// `consecutive` renders into any authored block reason / clone-error escalation. +/// `class` is the investigated root-cause classification; it authors the WHY on +/// the terminal [`NoProgressResolution::QuarantineTerminal`] block reason and its +/// single deduplicated escalation issue. /// /// `unblock_nonterminal` distinguishes the callers' starting state. The /// on-transition path acts on a goal that is *not* Blocked, so the non-terminal @@ -1105,6 +1129,7 @@ fn apply_resolution_side_effects( state: &mut OodaState, goal_id: &str, consecutive: u32, + class: NoProgressClass, resolution: NoProgressResolution, healer: &dyn PreconditionHealer, dispatcher: &dyn NoProgressEngineerDispatcher, @@ -1296,65 +1321,24 @@ fn apply_resolution_side_effects( "no-progress breaker: stuck after guided retry — BLOCKED WITH why + issue filed and linked", ); } - NoProgressResolution::SurfaceInvestigationFailure { class, reason } => { - // Bound the evidence-less re-investigation (issue #16 follow-up). The - // first fix (#4096) made this rung non-terminal so a goal is never - // parked with a bare `evidence=[(none)]` block — but an *unbounded* - // re-investigation is its own livelock: a goal whose done-criteria are - // permanently unclear surfaces → resets → forever, making no shippable - // progress and never reaching a human. After - // `SURFACED_INVESTIGATION_FAILURE_LIMIT` consecutive surfaced failures, - // stop spinning and escalate to a human WITH the re-investigation count - // as concrete evidence (so the never-`evidence=[(none)]` invariant - // holds — the count is real evidence, not `(none)`) and a measurable - // "make the done-criteria machine-checkable" ask. - let surfaced = tracker.record_surfaced_failure(goal_id); - if surfaced >= SURFACED_INVESTIGATION_FAILURE_LIMIT { - let why = NoProgressWhy::new( - class, - vec![Evidence::new( - "re-investigation", - goal_id, - format!("{surfaced} consecutive evidence-less investigations"), - )], - ); - let blocked_reason = no_progress_blocked_reason_with_why(consecutive, &why); - let (issue_title, issue_body) = - surfaced_failure_escalation_issue(goal_id, class, surfaced); - escalate_with_tracking_issue( - state, - goal_id, - blocked_reason, - &issue_title, - &issue_body, - filer, - ); - tracker.clear_surfaced_failures(goal_id); - tracker.reset_count(goal_id); - report.escalated.push(goal_id.to_string()); - tracing::warn!( - target: "simard::ooda", - goal = %goal_id, - why = %class.token(), - surfaced_failures = surfaced, - "no-progress breaker: evidence-less re-investigation bounded out after \ - {surfaced} surfaced failures — BLOCKED WITH re-investigation count as \ - evidence + human triage issue filed and linked to make the done-criteria measurable", - ); - return; - } - - // Below the bound: the independent investigation reached the terminal - // rung with NO evidence. A goal must NEVER be parked with + NoProgressResolution::SurfaceInvestigationFailure { class: _, reason } => { + // The independent investigation reached the terminal rung with NO + // evidence, but the goal has NOT yet exhausted its bounded + // re-investigation budget (the routing in + // [`resolution_for_why`] returns [`NoProgressResolution::QuarantineTerminal`] + // once the PRE-bump surfaced count reaches + // `SURFACED_INVESTIGATION_FAILURE_LIMIT`, so reaching THIS arm means we + // are still below the bound). A goal must NEVER be parked with // `evidence=[(none)]`, so this is a SURFACED failure — not a bare - // block. Take no terminal action: record it in `investigation_errors` - // (fail visible) and leave the goal retriable so the next investigation - // can recover real evidence (fail closed). The guided-retry flag is - // preserved, so a future terminal rung goes straight here again rather - // than spawning a second engineer. On the re-investigation path the - // goal starts in a bare / `(none)` Blocked state, so un-block it to - // `NotStarted` so the brain can re-select it and a later cycle can - // re-investigate. + // block. Take no terminal action: bump the surfaced-failure counter, + // record it in `investigation_errors` (fail visible) and leave the goal + // retriable so the next investigation can recover real evidence (fail + // closed). The guided-retry flag is preserved, so a future terminal + // rung goes straight here again rather than spawning a second engineer. + // On the re-investigation path the goal starts in a bare / `(none)` + // Blocked state, so un-block it to `NotStarted` so the brain can + // re-select it and a later cycle can re-investigate. + let surfaced = tracker.record_surfaced_failure(goal_id); if unblock_nonterminal && let Some(g) = state .active_goals @@ -1375,6 +1359,68 @@ fn apply_resolution_side_effects( investigation failure (never parked with evidence=[(none)]) — retriable", ); } + NoProgressResolution::QuarantineTerminal { surfaced_count } => { + // Terminal quarantine (process_health, HIGH). An evidence-less goal + // whose done-criteria are permanently UNCLEAR-CRITERIA / + // GENUINELY-STUCK has now exhausted its bounded re-investigation budget + // (`surfaced_count >= SURFACED_INVESTIGATION_FAILURE_LIMIT`). The old + // behaviour escalated (filed an issue) EVERY time it re-crossed the + // bound, and `reinvestigate_bare_blocked_goals` kept re-selecting the + // goal — so each cycle re-blocked and re-filed a near-identical + // `ooda-stuck` issue: unbounded churn. Quarantine ends the churn by + // (a) blocking WITH the surfaced count as concrete evidence (so the + // never-`evidence=[(none)]` invariant still holds), (b) filing exactly + // ONE deduplicated triage issue, (c) writing a durable + // [`quarantine_marker`] onto the goal, after which + // [`reinvestigate_bare_blocked_goals`] skips it permanently — no more + // re-schedule, no more re-file. A human must make the done-criteria + // machine-checkable and clear the marker to revive it. + let why = NoProgressWhy::new( + class, + vec![Evidence::new( + "re-investigation", + goal_id, + format!("{surfaced_count} consecutive evidence-less investigations"), + )], + ); + let blocked_reason = no_progress_blocked_reason_with_why(consecutive, &why); + let (issue_title, issue_body) = + surfaced_failure_escalation_issue(goal_id, class, surfaced_count); + escalate_with_tracking_issue( + state, + goal_id, + blocked_reason, + &issue_title, + &issue_body, + filer, + ); + // Idempotently mark the goal quarantined so the re-investigation pass + // stops re-selecting it. Only push the marker once — a goal that trips + // this arm again (e.g. a stale in-flight cycle) must not accrete + // duplicate markers. + if let Some(g) = state + .active_goals + .active + .iter_mut() + .find(|g| g.id == goal_id) + && !is_quarantined(g) + { + g.wip_refs.push(quarantine_marker()); + } + tracker.clear_surfaced_failures(goal_id); + tracker.reset_count(goal_id); + report.quarantined.push(goal_id.to_string()); + tracing::warn!( + target: "simard::ooda", + goal = %goal_id, + why = %class.token(), + surfaced_failures = surfaced_count, + "no-progress breaker: evidence-less re-investigation bounded out after \ + {surfaced_count} surfaced failures — TERMINALLY QUARANTINED (blocked WITH \ + re-investigation count as evidence, one triage issue filed, goal removed from \ + re-scheduling); a human must make the done-criteria measurable to revive it", + ); + } } } @@ -1443,6 +1489,21 @@ pub(crate) fn reinvestigate_bare_blocked_goals( .collect(); for goal_id in bare_ids { + // Churn-stopper (process_health, HIGH): a durably quarantined goal is + // NEVER re-investigated, re-classified, re-escalated, or re-filed again — + // even if a restart re-parked it in a bare block. Short-circuit BEFORE the + // reasoner is consulted so the terminal quarantine truly ends the + // re-schedule/re-file loop (the reasoner is a `PanicReasoner` in the + // churn test to prove it is never reached). + if state + .active_goals + .active + .iter() + .any(|g| g.id == goal_id && is_quarantined(g)) + { + continue; + } + // Investigate the bare goal ONCE. Fail closed on error: no terminal // action, marker left exactly as-is, nothing recorded in the dedupe set. let why = { @@ -1517,7 +1578,8 @@ pub(crate) fn reinvestigate_bare_blocked_goals( } let guided_retry_used = tracker.guided_retry_used(&goal_id); - let resolution = resolution_for_why(threshold, why, guided_retry_used); + let surfaced_failures = tracker.surfaced_failures(&goal_id); + let resolution = resolution_for_why(threshold, why, guided_retry_used, surfaced_failures); // An evidence-less terminal outcome takes NO terminal action (it is // surfaced + retried), so it must NOT be recorded in the (goal, class) @@ -1531,6 +1593,7 @@ pub(crate) fn reinvestigate_bare_blocked_goals( state, &goal_id, threshold, + class, resolution, healer, dispatcher, diff --git a/src/ooda_loop/tests_no_progress_investigation.rs b/src/ooda_loop/tests_no_progress_investigation.rs index 8e703ade7..0d5806142 100644 --- a/src/ooda_loop/tests_no_progress_investigation.rs +++ b/src/ooda_loop/tests_no_progress_investigation.rs @@ -38,7 +38,7 @@ use crate::error::{SimardError, SimardResult}; use crate::goal_curation::completion_gate::{DependencyState, EvidenceSource}; use crate::goal_curation::no_progress_breaker::{ NO_PROGRESS_BLOCKED_PREFIX, NO_PROGRESS_BREAKER_THRESHOLD, - SURFACED_INVESTIGATION_FAILURE_LIMIT, is_no_progress_marker, + SURFACED_INVESTIGATION_FAILURE_LIMIT, is_no_progress_marker, is_quarantined, }; use crate::goal_curation::no_progress_why::{ Evidence, NoProgressClass, NoProgressWhy, NoProgressWhyReasoner, @@ -1042,7 +1042,7 @@ fn genuinely_stuck_with_no_evidence_surfaces_investigation_error_never_parks_non } } -// === (h) evidence-less re-investigation is BOUNDED, then escalated to a human = +// === (h) evidence-less re-investigation is BOUNDED, then TERMINALLY QUARANTINED = // // Issue #16 (#4096) fixed the live defect of parking a goal with a bare // `evidence=[(none)]` block by making the evidence-less terminal rung @@ -1050,11 +1050,17 @@ fn genuinely_stuck_with_no_evidence_surfaces_investigation_error_never_parks_non // But an *unbounded* re-investigation is its OWN livelock: a goal whose // done-criteria are permanently unclear (the six `simard-identity-*` codename // goals) surfaces → resets → forever, making no shippable progress and NEVER -// reaching a human. This asserts the bound: after -// `SURFACED_INVESTIGATION_FAILURE_LIMIT` consecutive surfaced failures the -// breaker stops spinning and escalates the goal to a human WITH the -// re-investigation count as concrete evidence (never `(none)`) and a measurable -// "make the done-criteria machine-checkable" ask. +// reaching a human — and, worse, re-blocking + re-filing a near-identical +// `ooda-stuck` issue each cycle (the process_health churn). +// +// The fix (process_health, HIGH) bounds it with a TERMINAL QUARANTINE: once a +// goal accrues `SURFACED_INVESTIGATION_FAILURE_LIMIT` consecutive surfaced +// failures it is quarantined — Blocked WITH the re-investigation count as +// concrete evidence (never `(none)`), one deduplicated triage issue filed, and a +// durable quarantine marker written so the re-investigation pass never +// re-schedules or re-files it again. The routing consults the PRE-bump surfaced +// count, so quarantine trips one episode AFTER the counter first reaches the +// bound (each below-bound episode still surfaces + bumps the counter). /// Drive one full stall episode (`threshold` no-action cycles) and return the /// last cycle's report — one episode yields exactly one terminal-rung decision. @@ -1079,7 +1085,7 @@ fn drive_episode( } #[test] -fn evidenceless_reinvestigation_is_bounded_then_escalated_to_a_human() { +fn evidenceless_reinvestigation_is_bounded_then_terminally_quarantined() { let threshold = NO_PROGRESS_BREAKER_THRESHOLD; let limit = SURFACED_INVESTIGATION_FAILURE_LIMIT; let id = "simard-identity-concierge-hospitality-design"; @@ -1099,9 +1105,11 @@ fn evidenceless_reinvestigation_is_bounded_then_escalated_to_a_human() { let dispatcher = RecordingDispatcher::ok(); let filer = RecordingFiler::default(); - // Episodes below the bound SURFACE the failure (fail-visible, retriable) and - // NEVER escalate — but each bumps the persisted surfaced-failure counter. - for episode in 1..limit { + // Episodes up to AND INCLUDING the one that lifts the counter to the bound + // SURFACE the failure (fail-visible, retriable) and NEVER quarantine — but + // each bumps the persisted surfaced-failure counter. Because routing reads + // the PRE-bump count, the counter reaches `limit` without tripping quarantine. + for episode in 1..=limit { let report = drive_episode( &mut state, id, @@ -1114,11 +1122,11 @@ fn evidenceless_reinvestigation_is_bounded_then_escalated_to_a_human() { ); assert!( report.investigation_errors.contains(&id.to_string()), - "episode {episode} (below the bound) must SURFACE the failure: {report:?}" + "episode {episode} (below/at the counter bound) must SURFACE the failure: {report:?}" ); assert!( - !report.escalated.contains(&id.to_string()), - "episode {episode} (below the bound) must NOT escalate yet: {report:?}" + report.quarantined.is_empty(), + "episode {episode} must NOT quarantine yet (pre-bump count still below the bound): {report:?}" ); assert_eq!( state.no_progress_tracker.surfaced_failures(id), @@ -1127,16 +1135,28 @@ fn evidenceless_reinvestigation_is_bounded_then_escalated_to_a_human() { ); assert!( filer.calls.borrow().is_empty(), - "no human issue may be filed below the bound: {:?}", + "no human issue may be filed before quarantine: {:?}", filer.calls.borrow() ); assert!( matches!(status_of(&state, id), GoalProgress::NotStarted), - "a surfaced-but-not-escalated goal stays retriable (NotStarted)" + "a surfaced-but-not-quarantined goal stays retriable (NotStarted)" + ); + assert!( + !is_quarantined( + state + .active_goals + .active + .iter() + .find(|g| g.id == id) + .expect("goal on board") + ), + "episode {episode} must not have written the quarantine marker yet" ); } - // The episode that REACHES the bound escalates to a human. + // The NEXT episode observes a PRE-bump surfaced count == limit and TERMINALLY + // QUARANTINES the goal (the unbounded re-investigation + re-file churn stops). let report = drive_episode( &mut state, id, @@ -1148,13 +1168,30 @@ fn evidenceless_reinvestigation_is_bounded_then_escalated_to_a_human() { threshold, ); assert!( - report.escalated.contains(&id.to_string()), - "reaching SURFACED_INVESTIGATION_FAILURE_LIMIT ({limit}) must ESCALATE the \ - goal — the unbounded re-investigation livelock is broken: {report:?}" + report.quarantined.contains(&id.to_string()), + "once the surfaced count reaches SURFACED_INVESTIGATION_FAILURE_LIMIT ({limit}) the \ + next episode must TERMINALLY QUARANTINE the goal — ending the churn: {report:?}" ); assert!( !report.investigation_errors.contains(&id.to_string()), - "the bounded-out episode escalates (terminal), it does not merely surface: {report:?}" + "the quarantining episode is terminal, it does not merely surface: {report:?}" + ); + assert!( + report.fired(), + "a terminal quarantine is a breaker firing: {report:?}" + ); + + // The goal now carries the durable quarantine marker. + assert!( + is_quarantined( + state + .active_goals + .active + .iter() + .find(|g| g.id == id) + .expect("goal on board") + ), + "the quarantined goal must carry the durable quarantine marker" ); // The block reason carries the re-investigation COUNT as concrete evidence — @@ -1163,22 +1200,22 @@ fn evidenceless_reinvestigation_is_bounded_then_escalated_to_a_human() { GoalProgress::Blocked(reason) => { assert!( !reason.contains("(none)"), - "the escalation must NEVER read as an evidence=[(none)] block: {reason}" + "the quarantine must NEVER read as an evidence=[(none)] block: {reason}" ); assert!( is_no_progress_marker(reason) && reason.starts_with(NO_PROGRESS_BLOCKED_PREFIX), - "the escalation must keep the [OODA-SAFEGUARD] marker: {reason}" + "the quarantine must keep the [OODA-SAFEGUARD] marker: {reason}" ); assert!( reason.contains(NoProgressClass::UnclearCriteria.token()), - "the escalation must name the accurate root cause class: {reason}" + "the quarantine must name the accurate root cause class: {reason}" ); assert!( reason.contains(&format!("{limit} consecutive evidence-less investigations")), - "the escalation evidence must be the re-investigation count: {reason}" + "the quarantine evidence must be the re-investigation count: {reason}" ); } - other => panic!("expected a WHY-bearing Blocked escalation, got {other:?}"), + other => panic!("expected a WHY-bearing Blocked quarantine, got {other:?}"), } // Exactly one human triage issue is filed, and it asks for MEASURABLE @@ -1204,8 +1241,8 @@ fn evidenceless_reinvestigation_is_bounded_then_escalated_to_a_human() { ); // No engineer was ever spawned in this test (the retry was pre-spent), and the - // surfaced-failure counter is cleared on escalation so a future re-entry gets a - // fresh window rather than escalating immediately. + // surfaced-failure counter is cleared on quarantine so the terminal state is + // clean (the goal is henceforth skipped by the re-investigation pass anyway). assert!( dispatcher.calls.borrow().is_empty(), "no guided engineer is spawned once the retry is spent" @@ -1213,7 +1250,7 @@ fn evidenceless_reinvestigation_is_bounded_then_escalated_to_a_human() { assert_eq!( state.no_progress_tracker.surfaced_failures(id), 0, - "the surfaced-failure counter is cleared once the goal is escalated" + "the surfaced-failure counter is cleared once the goal is quarantined" ); } diff --git a/src/ooda_loop/tests_quarantine_churn.rs b/src/ooda_loop/tests_quarantine_churn.rs new file mode 100644 index 000000000..487b60e30 --- /dev/null +++ b/src/ooda_loop/tests_quarantine_churn.rs @@ -0,0 +1,426 @@ +//! TEST-FIRST (Step 7 TDD) — the **churn-stopping** side effects of the OODA +//! breaker terminal-quarantine rung (process_health, HIGH). +//! +//! # The churn these tests kill +//! +//! A permanently-`UNCLEAR-CRITERIA` goal is re-selected by the already-blocked +//! re-investigation pass every cycle, rides the ladder to the evidence-less +//! terminal rung, is surfaced + un-blocked, gets re-selected, and repeats — +//! filing near-identical `ooda-stuck` issues each time. The single behaviour that +//! stops the churn: once a goal trips the terminal-quarantine rung it is durably +//! marked and **never re-scheduled, re-classified, or re-escalated again**. +//! +//! # The contract (side-effecting layer) +//! +//! 1. `reinvestigate_bare_blocked_goals` **skips any quarantined goal** — the +//! reasoner is never even consulted for it (the churn-stopper). +//! 2. When the pure ladder returns `QuarantineTerminal`, the adapter Blocks the +//! goal with a WHY-bearing reason (never bare / never `(none)`) AND writes the +//! durable quarantine marker through the goal board. +//! 3. The marker write is idempotent (≤ 1 marker per goal) and a quarantined goal +//! is never re-filed — no duplicate-issue storm. +//! +//! Every dependency is an injected hermetic fake — no `gh`, no clone, no +//! subprocess. RED until the quarantine variant, the extended +//! `resolution_for_why`, the marker helpers, the `QuarantineTerminal` side-effect +//! handler, and the `is_quarantined` re-schedule exclusion all exist. + +use std::sync::atomic::{AtomicUsize, Ordering}; + +use super::no_progress::{ + NoProgressBreakerReport, NoProgressEngineerDispatcher, NoProgressIssueFiler, + PreconditionHealer, reinvestigate_bare_blocked_goals, +}; +use crate::error::SimardResult; +use crate::goal_curation::completion_gate::{DependencyState, EvidenceSource}; +use crate::goal_curation::no_progress_breaker::{ + NO_PROGRESS_BREAKER_THRESHOLD, SURFACED_INVESTIGATION_FAILURE_LIMIT, is_bare_no_progress_block, + is_quarantined, no_progress_blocked_reason, quarantine_marker, +}; +use crate::goal_curation::no_progress_why::{ + NoProgressClass, NoProgressWhy, NoProgressWhyReasoner, +}; +use crate::goal_curation::{ActiveGoal, GoalBoard, GoalProgress}; +use crate::ooda_loop::OodaState; + +// --- fakes ------------------------------------------------------------------ + +/// Canned "still stuck" evidence source (classification is driven by the +/// injected reasoner; the source is taken only for API symmetry). +struct StuckEvidence; +impl EvidenceSource for StuckEvidence { + 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) + } + fn repo_present(&self, _goal: &ActiveGoal) -> SimardResult { + Ok(true) + } + fn dependency_goal_state(&self, _goal: &ActiveGoal) -> SimardResult { + Ok(DependencyState::None) + } +} + +/// A reasoner that returns `UNCLEAR-CRITERIA` with NO evidence and counts how +/// many times it is consulted. +#[derive(Default)] +struct CountingUnclearReasoner { + calls: AtomicUsize, +} +impl NoProgressWhyReasoner for CountingUnclearReasoner { + fn investigate(&self, _goal: &ActiveGoal) -> SimardResult { + self.calls.fetch_add(1, Ordering::SeqCst); + Ok(NoProgressWhy::new(NoProgressClass::UnclearCriteria, vec![])) + } +} + +/// A reasoner that MUST NOT be consulted — panics if it is. Proves the +/// quarantine exclusion short-circuits before any investigation. +struct PanicReasoner; +impl NoProgressWhyReasoner for PanicReasoner { + fn investigate(&self, goal: &ActiveGoal) -> SimardResult { + panic!( + "a quarantined goal must NEVER be re-investigated — reasoner consulted for {:?}", + goal.id + ) + } +} + +struct NoopHealer; +impl PreconditionHealer for NoopHealer { + fn heal(&self, _goal: &ActiveGoal, _why: &NoProgressWhy) -> Result<(), String> { + Ok(()) + } +} + +/// A dispatcher that must not be asked to spawn a fixer for a terminal goal. +#[derive(Default)] +struct CountingDispatcher { + calls: AtomicUsize, +} +impl NoProgressEngineerDispatcher for CountingDispatcher { + fn spawn_engineer(&self, _goal_id: &str, _task: &str) -> bool { + self.calls.fetch_add(1, Ordering::SeqCst); + true + } +} + +/// Counts every issue-filing attempt so we can prove the storm is gone. +#[derive(Default)] +struct CountingFiler { + calls: AtomicUsize, +} +impl NoProgressIssueFiler for CountingFiler { + fn file_issue(&self, _title: &str, _body: &str) -> Option { + self.calls.fetch_add(1, Ordering::SeqCst); + Some(super::no_progress::FiledIssue { + number: format!("{}", 9000 + self.calls.load(Ordering::SeqCst)), + url: None, + }) + } +} + +// --- fixtures --------------------------------------------------------------- + +fn state_with(goal: ActiveGoal) -> OodaState { + let mut board = GoalBoard::new(); + board.active.push(goal); + OodaState::new(board) +} + +fn bare_blocked(id: &str) -> ActiveGoal { + let mut g = ActiveGoal::new(id, "keep the simard identity coherent", 1); + g.status = GoalProgress::Blocked(no_progress_blocked_reason(NO_PROGRESS_BREAKER_THRESHOLD)); + g +} + +fn only_goal(state: &OodaState) -> &ActiveGoal { + &state.active_goals.active[0] +} + +/// Re-park an already-present goal back into a BARE no-progress block, modelling +/// the brain re-selecting a surfaced (un-blocked) goal that stalls again and is +/// re-parked bare by the completion gate. Used to drive the breaker cycle by +/// cycle without minting a fresh goal each time. +fn reblock_bare(state: &mut OodaState, id: &str) { + let g = state + .active_goals + .active + .iter_mut() + .find(|g| g.id == id) + .expect("goal still on the board"); + g.status = GoalProgress::Blocked(no_progress_blocked_reason(NO_PROGRESS_BREAKER_THRESHOLD)); +} + +#[allow(clippy::too_many_arguments)] +fn drive( + state: &mut OodaState, + reasoner: &dyn NoProgressWhyReasoner, + dispatcher: &dyn NoProgressEngineerDispatcher, + filer: &dyn NoProgressIssueFiler, +) -> NoProgressBreakerReport { + let evidence = StuckEvidence; + let healer = NoopHealer; + reinvestigate_bare_blocked_goals( + state, + &evidence, + reasoner, + &healer, + dispatcher, + filer, + NO_PROGRESS_BREAKER_THRESHOLD, + ) +} + +// === (1) the churn-stopper: a quarantined goal is never re-investigated ====== + +#[test] +fn a_quarantined_goal_is_skipped_by_reinvestigation() { + // A goal parked BARE (so the deterministic rail WOULD normally select it) + // that ALSO carries the durable quarantine marker must be short-circuited: + // the reasoner is never consulted, no fixer is spawned, no issue is filed, + // and its status is left exactly as-is. + let id = "simard-identity-atelier-industrial-furniture-de"; + let mut goal = bare_blocked(id); + goal.wip_refs.push(quarantine_marker()); + let before = goal.status.clone(); + let mut state = state_with(goal); + + let dispatcher = CountingDispatcher::default(); + let filer = CountingFiler::default(); + + // PanicReasoner: any investigation of the quarantined goal fails the test. + let report = drive(&mut state, &PanicReasoner, &dispatcher, &filer); + + assert_eq!( + dispatcher.calls.load(Ordering::SeqCst), + 0, + "a quarantined goal must never spawn a fixer" + ); + assert_eq!( + filer.calls.load(Ordering::SeqCst), + 0, + "a quarantined goal must never file (or re-file) an ooda-stuck issue" + ); + assert!( + report.reinvestigated.is_empty() && !report.fired(), + "a quarantined goal must not appear in any breaker action bucket" + ); + assert_eq!( + only_goal(&state).status, + before, + "a quarantined goal's status must be left untouched" + ); + assert!( + is_quarantined(only_goal(&state)), + "the goal must remain quarantined" + ); +} + +// === (2) at the bound, the pass QUARANTINES: marker + non-bare block ========= + +#[test] +fn evidenceless_stall_at_the_bound_is_quarantined_and_marked() { + let id = "simard-identity-luxe-coastal-lighting-collectiv"; + let mut state = state_with(bare_blocked(id)); + + // The goal has spent its guided retry and already accrued the bounded number + // of evidence-less surfaced failures — the exact terminal condition. + state.no_progress_tracker.mark_guided_retry(id); + for _ in 0..SURFACED_INVESTIGATION_FAILURE_LIMIT { + state.no_progress_tracker.record_surfaced_failure(id); + } + assert_eq!( + state.no_progress_tracker.surfaced_failures(id), + SURFACED_INVESTIGATION_FAILURE_LIMIT, + "precondition: the goal is at the surfaced-failure bound" + ); + + let reasoner = CountingUnclearReasoner::default(); + let dispatcher = CountingDispatcher::default(); + let filer = CountingFiler::default(); + + drive(&mut state, &reasoner, &dispatcher, &filer); + + assert_eq!( + reasoner.calls.load(Ordering::SeqCst), + 1, + "the goal is investigated exactly once before being quarantined" + ); + let g = only_goal(&state); + assert!( + is_quarantined(g), + "an at-bound evidence-less UNCLEAR-CRITERIA stall must be durably quarantined" + ); + match &g.status { + GoalProgress::Blocked(reason) => { + assert!( + !is_bare_no_progress_block(reason), + "the quarantine block must carry a concrete WHY, never a bare 'needs human \ + review': {reason}" + ); + assert!( + !reason.contains("evidence=[(none)]"), + "the quarantine block must render the surfaced count as REAL evidence, never \ + evidence=[(none)]: {reason}" + ); + } + other => panic!("a quarantined goal must be Blocked, got {other:?}"), + } + assert_eq!( + dispatcher.calls.load(Ordering::SeqCst), + 0, + "quarantine is terminal — it must not spawn another fixer" + ); +} + +// === (3) idempotent marker + no re-file storm =============================== + +#[test] +fn quarantine_is_idempotent_and_never_refiles() { + let id = "simard-identity-artisan-heritage-textiles-studi"; + let mut state = state_with(bare_blocked(id)); + state.no_progress_tracker.mark_guided_retry(id); + for _ in 0..SURFACED_INVESTIGATION_FAILURE_LIMIT { + state.no_progress_tracker.record_surfaced_failure(id); + } + + let reasoner = CountingUnclearReasoner::default(); + let dispatcher = CountingDispatcher::default(); + let filer = CountingFiler::default(); + + // Pass 1: quarantine. + drive(&mut state, &reasoner, &dispatcher, &filer); + assert!(is_quarantined(only_goal(&state)), "pass 1 must quarantine"); + let filings_after_first = filer.calls.load(Ordering::SeqCst); + assert!( + filings_after_first <= 1, + "quarantine must file AT MOST one tracking issue, got {filings_after_first}" + ); + + // Pass 2: identical inputs. The quarantined goal must be skipped entirely — + // no second investigation, no duplicate marker, no re-file. + drive(&mut state, &reasoner, &dispatcher, &filer); + + assert_eq!( + reasoner.calls.load(Ordering::SeqCst), + 1, + "a quarantined goal must NOT be re-investigated on the next cycle" + ); + assert_eq!( + filer.calls.load(Ordering::SeqCst), + filings_after_first, + "a quarantined goal must NEVER be re-filed — this is the churn that is being killed" + ); + let markers = only_goal(&state) + .wip_refs + .iter() + .filter(|w| crate::goal_curation::no_progress_breaker::is_quarantine_ref(w)) + .count(); + assert_eq!( + markers, 1, + "the durable quarantine marker must be written at most once (≤ 1 per goal)" + ); +} + +// === (4) below the bound: NOT quarantined (retriable) ======================== + +#[test] +fn below_the_bound_the_goal_is_not_quarantined() { + let id = "simard-identity-modernist-ceramic-tableware-lin"; + let mut state = state_with(bare_blocked(id)); + state.no_progress_tracker.mark_guided_retry(id); + // One below the bound. + for _ in 0..(SURFACED_INVESTIGATION_FAILURE_LIMIT - 1) { + state.no_progress_tracker.record_surfaced_failure(id); + } + + let reasoner = CountingUnclearReasoner::default(); + let dispatcher = CountingDispatcher::default(); + let filer = CountingFiler::default(); + + drive(&mut state, &reasoner, &dispatcher, &filer); + + assert!( + !is_quarantined(only_goal(&state)), + "below the surfaced-failure bound the goal must stay retriable, never quarantined" + ); +} + +// === (5) EXACT cycle-of-fire: LIMIT surfaces, then quarantine (Step 18b) ====== +// +// Regression pin for the DELIBERATE pre-bump bound in `resolution_for_why` +// (`surfaced_failures >= SURFACED_INVESTIGATION_FAILURE_LIMIT`, evaluated BEFORE +// this cycle records its own surfaced failure). The pure-ladder unit tests +// already prove the `>=` boundary; what was missing is an END-TO-END assertion +// that driving the real re-investigation adapter cycle by cycle surfaces the +// evidence-less stall EXACTLY `SURFACED_INVESTIGATION_FAILURE_LIMIT` times +// (leaving it retriable each time) and only quarantines on the *next* cycle — +// the (LIMIT + 1)th observation. This locks the end-to-end timing so the +// intentional pre-bump semantics can never silently drift by a cycle (the +// off-by-one flagged in the Step 18a review). +#[test] +fn quarantine_fires_on_the_cycle_after_the_limit_th_surface() { + let id = "simard-identity-heritage-oak-joinery-lin"; + let mut state = state_with(bare_blocked(id)); + // Guided retry already spent, so the ladder reaches the evidence-less + // terminal rung instead of spawning a (second) engineer. The flag persists + // across surface cycles (SurfaceInvestigationFailure preserves it). + state.no_progress_tracker.mark_guided_retry(id); + + let reasoner = CountingUnclearReasoner::default(); + let dispatcher = CountingDispatcher::default(); + let filer = CountingFiler::default(); + + // Cycles 1..=LIMIT: each re-investigation SURFACES (never quarantines) and + // un-blocks the goal to NotStarted. Re-park it bare before the next cycle to + // model the brain re-selecting and re-stalling it. + for cycle in 1..=SURFACED_INVESTIGATION_FAILURE_LIMIT { + let report = drive(&mut state, &reasoner, &dispatcher, &filer); + assert!( + !is_quarantined(only_goal(&state)), + "cycle {cycle}: at/below the pre-bump bound the goal must NOT yet be quarantined" + ); + assert!( + report.quarantined.is_empty(), + "cycle {cycle}: nothing may be quarantined before the (LIMIT + 1)th observation" + ); + assert_eq!( + report.investigation_errors, + vec![id.to_string()], + "cycle {cycle}: the evidence-less stall must be SURFACED (retriable), not parked" + ); + assert_eq!( + state.no_progress_tracker.surfaced_failures(id), + cycle, + "cycle {cycle}: exactly one surfaced failure is recorded per surface cycle" + ); + // The surface un-blocked the goal to NotStarted; re-park it bare for the + // next re-investigation pass. + reblock_bare(&mut state, id); + } + + // Cycle LIMIT + 1: the PRE-bump surfaced count now equals the bound, so THIS + // observation quarantines terminally instead of surfacing a (LIMIT + 1)th + // time. This is the exact cycle of fire. + let report = drive(&mut state, &reasoner, &dispatcher, &filer); + assert!( + is_quarantined(only_goal(&state)), + "the (LIMIT + 1)th observation must terminally quarantine the goal" + ); + assert_eq!( + report.quarantined, + vec![id.to_string()], + "quarantine fires on exactly the cycle AFTER the LIMIT-th surface — never earlier, \ + never later" + ); + assert!( + report.investigation_errors.is_empty(), + "the quarantine cycle is terminal: it must NOT also surface a retriable failure" + ); +} diff --git a/src/operator_cli/goal.rs b/src/operator_cli/goal.rs index a3077c660..74621bb6b 100644 --- a/src/operator_cli/goal.rs +++ b/src/operator_cli/goal.rs @@ -36,7 +36,9 @@ use std::error::Error; -use crate::goal_curation::{GoalDecomposer, GoalProgress, labels, simard_state_root}; +use crate::goal_curation::{ + GoalDecomposer, GoalProgress, is_quarantine_ref, labels, simard_state_root, +}; use crate::memory_ipc::launch_writer_client; use crate::ooda_actions::advance_goal::spawn::is_brain_failure_marker; @@ -495,7 +497,7 @@ fn handle_label_list(goal_id: &str) -> Result<(), Box> { } fn handle_unblock(goal_id: &str) -> Result<(), Box> { - let prior = with_board(|board| { + let (prior, quarantine_cleared) = with_board(|board| { let goal = board .active .iter_mut() @@ -506,9 +508,29 @@ fn handle_unblock(goal_id: &str) -> Result<(), Box> { })?; let prior = goal.status.clone(); goal.status = GoalProgress::NotStarted; - Ok(prior) + // Clear the durable OODA breaker terminal-quarantine marker so the goal + // is fully revived: a quarantined goal is otherwise skipped forever by + // `reinvestigate_bare_blocked_goals`, so leaving the marker in place + // would restore the goal to `NotStarted` while still barring it from the + // re-investigation pass. `goal unblock` is the documented, explicit + // per-goal escape hatch (unlike `unblock-all`, which is deliberately + // scoped away from quarantines), so clearing the marker here is the + // intended recovery — it hands the goal a fresh bounded guided-retry + // window. The surfaced-failure counter was already reset when quarantine + // fired, so no further counter reset is needed. + let before = goal.wip_refs.len(); + goal.wip_refs.retain(|w| !is_quarantine_ref(w)); + let quarantine_cleared = goal.wip_refs.len() != before; + Ok((prior, quarantine_cleared)) })?; - eprintln!("[simard] goal unblock: '{goal_id}' restored to NotStarted (was: {prior})"); + if quarantine_cleared { + eprintln!( + "[simard] goal unblock: '{goal_id}' restored to NotStarted (was: {prior}); \ + cleared OODA breaker quarantine marker" + ); + } else { + eprintln!("[simard] goal unblock: '{goal_id}' restored to NotStarted (was: {prior})"); + } Ok(()) } diff --git a/src/operator_cli/tests_goal.rs b/src/operator_cli/tests_goal.rs index f66bf45e7..ac941da67 100644 --- a/src/operator_cli/tests_goal.rs +++ b/src/operator_cli/tests_goal.rs @@ -190,6 +190,57 @@ fn simard_goal_unblock_clears_any_blocked_reason_unconditionally() { assert_eq!(g.status, GoalProgress::NotStarted); } +#[test] +#[serial_test::serial(cognitive_memory)] +fn simard_goal_unblock_clears_the_ooda_quarantine_marker() { + // Recovery contract (see docs/howto/quarantine-and-recover-an-unclear-ooda-goal.md + // Option B and docs/concepts/steerable-ooda-daemon.md): a single-id + // `goal unblock` on a terminally-quarantined goal must clear the durable + // `ooda-breaker-quarantine` WipRef, not merely reset the status — otherwise + // the goal is restored to NotStarted while `reinvestigate_bare_blocked_goals` + // still skips it forever, and the documented "fresh bounded window" never + // materialises. + let (_tmp, root) = isolated_state_root(); + let mut quarantined = active_goal( + "quarantined-goal", + GoalProgress::Blocked("[OODA-SAFEGUARD] why=UNCLEAR-CRITERIA".into()), + ); + quarantined + .wip_refs + .push(crate::goal_curation::quarantine_marker()); + seed_board(&root, vec![quarantined]); + + let result = dispatch_operator_cli(vec![ + "goal".to_string(), + "unblock".to_string(), + "quarantined-goal".to_string(), + ]); + assert!( + result.is_ok(), + "`simard goal unblock quarantined-goal` must exit 0; got: {:?}", + result.err().map(|e| e.to_string()) + ); + + let board = load_board(&root); + let g = board + .active + .iter() + .find(|g| g.id == "quarantined-goal") + .expect("goal must survive unblock"); + assert_eq!( + g.status, + GoalProgress::NotStarted, + "unblock must restore a quarantined goal to NotStarted; got {:?}", + g.status + ); + assert!( + !crate::goal_curation::is_quarantined(g), + "unblock must clear the durable ooda-breaker-quarantine marker so the goal \ + is no longer skipped by the re-investigation pass; wip_refs={:?}", + g.wip_refs + ); +} + #[test] #[serial_test::serial(cognitive_memory)] fn simard_goal_unblock_unknown_id_returns_error() { diff --git a/src/overseer/doc_pr_reconcile.rs b/src/overseer/doc_pr_reconcile.rs new file mode 100644 index 000000000..39a37b6b0 --- /dev/null +++ b/src/overseer/doc_pr_reconcile.rs @@ -0,0 +1,309 @@ +//! Auto-generated documentation-PR **reconciliation** (goal_hygiene). +//! +//! An automated doc-update flow opens a fresh `"Update documentation with N +//! changed files"` PR per doc-drift event without deduping, rebasing, or +//! auto-closing superseded ones — so stale, CONFLICTING, draft auto-doc PRs +//! accumulate and rot unmerged. This module enforces a **single-open** invariant +//! for those PRs: keep the newest (canonical) auto-doc PR and close every other +//! candidate, tagging each close as a superseded duplicate or a stale +//! CONFLICTING draft. +//! +//! The design is additive and split into a **pure** decision core +//! ([`reconcile_doc_prs`]) with no I/O and a thin IO-guarded executor +//! ([`run_doc_pr_reconcile`]) that lists open PRs and closes the superseded ones +//! by number via the additive [`PrGhClient::close_pr`]. A composite, +//! fail-closed identity gate ([`is_auto_doc_pr`]) positively identifies an +//! auto-doc PR only when the title marker, the known auto-generation author, the +//! draft flag, and the auto-doc label ALL hold — so a human PR (or one with an +//! empty/absent author) is never a candidate. +//! +//! ## Scope (deliberate fail-safe boundary) +//! +//! Reconciliation acts ONLY on PRs that pass the full identity gate — i.e. +//! correctly-labeled auto-doc *drafts* authored by the resolved auto-doc +//! identity. Legacy title-marker PRs that are non-draft or unlabeled are +//! **intentionally left untouched**: the strict gate is the security guard that +//! guarantees a human (or otherwise non-auto-doc) PR is never auto-closed, so +//! the module prefers leaving an ambiguous historical PR open over ever closing +//! one it cannot positively attribute. Draining that legacy, unlabeled/non-draft +//! backlog is deliberately out of scope here and is tracked as a follow-up (see +//! the reference doc); the LOUD inert-gate `warn` in +//! [`run_doc_pr_reconcile_with_author`] surfaces the case where title-marker PRs +//! exist but none pass the gate. +//! +//! See `docs/reference/auto-doc-pr-reconciliation-api.md` for the full contract. + +use crate::error::SimardResult; +use crate::overseer::config::{SIMARD_ENGINEER_PR_LABEL, automerge_author}; +use crate::stewardship::merge_authority::{OpenPrSummary, PrGhClient}; + +/// Title-prefix a candidate PR's title must start with. A durable cross-system +/// string; changing it would silently disable reconciliation, so it is a stable +/// contract. +pub const AUTO_DOC_PR_TITLE_MARKER: &str = "Update documentation with"; + +// NOTE: the author gate no longer keys on a compile-time author constant. The +// auto-doc PRs are authored under Simard's engineer / OODA gh identity +// (`crate::overseer::config::automerge_author`) — NOT the `simard-overseer[bot]` +// login — so the expected author is resolved at the I/O boundary +// (`run_doc_pr_reconcile`) and threaded into the pure gate. An empty/absent +// expected author (e.g. `SIMARD_AUTOMERGE_AUTHOR` unset) matches nothing, so the +// pass stays fail-closed. + +/// The label a candidate must carry. Simard's own autonomous PRs (including the +/// auto-doc drafts) are tagged with this durable label. +pub const AUTO_DOC_PR_LABEL: &str = SIMARD_ENGINEER_PR_LABEL; + +/// The `gh pr list` fetch window for reconciliation. Wide enough to see the full +/// backlog of accumulated auto-doc drafts in one pass so the canonical selection +/// is over ALL candidates, not a truncated window. +const DOC_PR_LIST_LIMIT: u32 = 200; + +/// Maximum closes executed per cycle — a bounded batch so a large accumulated +/// backlog of *reconciliation candidates* (labeled auto-doc drafts that pass the +/// full identity gate) is drained over several cycles rather than in one +/// unbounded storm of `gh pr close` mutations. Non-candidate historical PRs are +/// never in this set. The canonical PR is always preserved regardless. +const MAX_CLOSES_PER_CYCLE: usize = 25; + +/// Why a duplicate auto-doc PR is being closed. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum CloseReason { + /// An older duplicate, superseded by the canonical (newest) auto-doc PR. + SupersededDuplicate, + /// A candidate whose `mergeable` state is `CONFLICTING` — a rotted draft. + StaleConflictingDraft, +} + +/// One queued close: the PR number, why it is closed, and the comment to post. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DocPrClose { + pub number: u32, + pub reason: CloseReason, + /// Comment posted on close, e.g. `"superseded by #"`. + pub comment: String, +} + +/// The pure decision: which single auto-doc PR to keep and which to close. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DocPrReconcileDecision { + /// The PR kept open (the single-open invariant's survivor), if any candidate + /// exists. + pub canonical: Option, + /// PRs to close, each with the reason it was superseded/auto-closed. The + /// canonical PR is NEVER present here. + pub to_close: Vec, +} + +/// Structured outcome of an executed reconciliation pass, for the overseer +/// journal/audit. +#[derive(Clone, Debug, PartialEq, Eq, Default)] +pub struct DocPrReconcileReport { + /// The surviving canonical auto-doc PR, if any. + pub canonical: Option, + /// PR numbers actually closed this cycle. + pub closed: Vec, + /// Count of open PRs that were NOT auto-doc candidates (ignored). + pub skipped: usize, + /// Per-close failures (number + reason); the pass continues past them so one + /// flaky close never blocks the rest. + pub errors: Vec, +} + +/// The `mergeable` state string GitHub reports for a PR with merge conflicts. +const CONFLICTING_MERGEABLE: &str = "CONFLICTING"; + +/// True only when EVERY signal marks `pr` an auto-generated doc-drift PR. Fails +/// closed: any missing signal — including an empty/absent author, or an +/// `expected_author` that is empty (unresolved identity) — returns `false`, so a +/// human PR (or a mis-resolved identity) is never a reconciliation candidate. +/// +/// `expected_author` is the auto-doc author identity resolved at the I/O +/// boundary from [`crate::overseer::config::automerge_author`]. Keeping the +/// author identity a mandatory conjunct is the security-critical guard: an +/// external attacker cannot spoof `pr.author.login` (GitHub-authoritative) and +/// cannot apply the auto-doc label without repo triage/write, so a crafted PR +/// can never become a close candidate. +pub fn is_auto_doc_pr(pr: &OpenPrSummary, expected_author: &str) -> bool { + !expected_author.is_empty() + && pr.title.starts_with(AUTO_DOC_PR_TITLE_MARKER) + && !pr.author.is_empty() + && pr.author == expected_author + && pr.is_draft == Some(true) + && pr.labels.iter().any(|l| l == AUTO_DOC_PR_LABEL) +} + +/// Pure: given the current open-PR listing for one repo, decide which single +/// auto-doc PR to keep (canonical) and which to close (with a reason). Performs +/// NO I/O. Non-auto-doc PRs are ignored entirely. +/// +/// The canonical PR is the newest (highest number) auto-doc candidate and is +/// NEVER placed in the close set, so the decision can never close every +/// candidate. Zero or one candidate ⇒ the invariant already holds (no closes). +pub fn reconcile_doc_prs( + open_prs: &[OpenPrSummary], + expected_author: &str, +) -> DocPrReconcileDecision { + let candidates: Vec<&OpenPrSummary> = open_prs + .iter() + .filter(|pr| is_auto_doc_pr(pr, expected_author)) + .collect(); + + let Some(canonical) = candidates.iter().map(|pr| pr.number).max() else { + return DocPrReconcileDecision { + canonical: None, + to_close: Vec::new(), + }; + }; + + let to_close: Vec = candidates + .iter() + .filter(|pr| pr.number != canonical) + .map(|pr| { + let reason = if pr.mergeable == CONFLICTING_MERGEABLE { + CloseReason::StaleConflictingDraft + } else { + CloseReason::SupersededDuplicate + }; + let comment = match reason { + CloseReason::SupersededDuplicate => format!( + "Auto-closing this superseded auto-generated documentation PR: \ + it is an older duplicate superseded by the canonical open auto-doc \ + PR #{canonical}. Enforcing the single-open auto-doc PR invariant \ + (goal_hygiene)." + ), + CloseReason::StaleConflictingDraft => format!( + "Auto-closing this stale CONFLICTING auto-generated documentation \ + draft: it can no longer merge cleanly and is superseded by the \ + canonical open auto-doc PR #{canonical}. Enforcing the single-open \ + auto-doc PR invariant (goal_hygiene)." + ), + }; + DocPrClose { + number: pr.number, + reason, + comment, + } + }) + .collect(); + + DocPrReconcileDecision { + canonical: Some(canonical), + to_close, + } +} + +/// Apply a reconciliation to one repo: list open PRs, compute the pure decision, +/// then execute the closes by NUMBER via [`PrGhClient::close_pr`]. Bounded and +/// IO-guarded; returns a structured report for the overseer journal/audit. +/// +/// **Fail-closed on read error:** a listing failure surfaces the error and +/// performs NO closes that cycle. Per-close failures are collected into +/// [`DocPrReconcileReport::errors`] and do not abort the batch (closing is a +/// hygiene convenience, not a correctness gate). A bounded number of closes +/// ([`MAX_CLOSES_PER_CYCLE`]) is executed per cycle. +pub fn run_doc_pr_reconcile(repo: &str, gh: &dyn PrGhClient) -> SimardResult { + // Resolve the auto-doc author identity at this I/O boundary. The auto-doc + // PRs are authored under Simard's engineer / OODA gh login + // (`SIMARD_AUTOMERGE_AUTHOR`), which is DISTINCT from the `simard-overseer[bot]` + // login. Unset => empty => the gate matches nothing (fail-closed). + let expected_author = automerge_author().unwrap_or_default(); + run_doc_pr_reconcile_with_author(repo, gh, &expected_author) +} + +/// Testable executor core: the auto-doc author identity is INJECTED (resolved by +/// the caller — [`run_doc_pr_reconcile`] — from +/// [`crate::overseer::config::automerge_author`]) so this stays free of env +/// access; its only I/O is the injected `gh` client. +/// +/// Emits a LOUD `warn` when title-marker auto-doc PRs are present but NONE pass +/// the full identity gate: that is the signature of a mis-resolved identity +/// (e.g. `SIMARD_AUTOMERGE_AUTHOR` unset or not matching the real auto-doc +/// author), a missing label, or non-draft PRs — an inert reconciler must never +/// fail silently. +pub fn run_doc_pr_reconcile_with_author( + repo: &str, + gh: &dyn PrGhClient, + expected_author: &str, +) -> SimardResult { + let open_prs = gh.list_open_prs(repo, DOC_PR_LIST_LIMIT)?; + let title_marker_count = open_prs + .iter() + .filter(|pr| pr.title.starts_with(AUTO_DOC_PR_TITLE_MARKER)) + .count(); + let candidate_count = open_prs + .iter() + .filter(|pr| is_auto_doc_pr(pr, expected_author)) + .count(); + let skipped = open_prs.len().saturating_sub(candidate_count); + + // LOUD inert-gate surfacing: title-marker PRs exist but NONE passed the gate, + // so reconciliation will take no action this cycle. Name the likely cause so + // a misconfigured author identity / missing label / non-draft never fails + // silently (a silent inert pass would let the stale-PR churn recur unseen). + if title_marker_count > 0 && candidate_count == 0 { + let author_display = if expected_author.is_empty() { + "" + } else { + expected_author + }; + tracing::warn!( + target: "simard::overseer", + repo = %repo, + title_marker_prs = title_marker_count, + expected_author = %author_display, + auto_doc_label = AUTO_DOC_PR_LABEL, + "auto-doc PR reconciliation: title-marker PR(s) present but NONE passed the identity \ + gate — reconciliation is INERT this cycle. Verify SIMARD_AUTOMERGE_AUTHOR matches the \ + auto-doc PR author, and that the PRs are drafts carrying the auto-doc label.", + ); + } + + let decision = reconcile_doc_prs(&open_prs, expected_author); + + tracing::info!( + target: "simard::overseer", + repo = %repo, + canonical = decision.canonical.map(|n| n as i64).unwrap_or(-1), + candidates = candidate_count, + to_close = decision.to_close.len(), + skipped, + "auto-doc PR reconciliation: single-open invariant decision computed", + ); + + let mut report = DocPrReconcileReport { + canonical: decision.canonical, + closed: Vec::new(), + skipped, + errors: Vec::new(), + }; + + for close in decision.to_close.into_iter().take(MAX_CLOSES_PER_CYCLE) { + match gh.close_pr(repo, close.number, &close.comment) { + Ok(()) => { + tracing::info!( + target: "simard::overseer", + repo = %repo, + pr = close.number, + reason = ?close.reason, + "auto-doc PR reconciliation: closed superseded auto-doc PR", + ); + report.closed.push(close.number); + } + Err(e) => { + tracing::warn!( + target: "simard::overseer", + repo = %repo, + pr = close.number, + reason = ?close.reason, + error = %e, + "auto-doc PR reconciliation: failed to close a superseded auto-doc PR \ + — leaving it open, continuing with the rest", + ); + report.errors.push(format!("close #{}: {e}", close.number)); + } + } + } + + Ok(report) +} diff --git a/src/overseer/mod.rs b/src/overseer/mod.rs index 6c4124b0e..79dd91a91 100644 --- a/src/overseer/mod.rs +++ b/src/overseer/mod.rs @@ -54,6 +54,7 @@ pub mod conflict; pub mod deploy; pub mod deploy_trigger; pub mod diagnosis; +pub mod doc_pr_reconcile; pub mod ecosystem_observe; pub mod failure_sink; pub mod guardrails; @@ -102,6 +103,16 @@ mod tests_merge_queue_reasoning; mod tests_rework_loop; #[cfg(test)] mod tests_root_cause; +// process_health (TDD): reblock-issue signature stabilization — two re-block +// observations of the same cause that differ only by a volatile goal id must +// dedup to ONE stewardship signature (`fold_volatile_goal_ids`). +#[cfg(test)] +mod tests_reblock_signature; +// goal_hygiene (TDD): the auto-generated documentation-PR reconciliation pass — +// composite fail-closed identity gate, single-open canonical selection, and the +// by-number close executor. +#[cfg(test)] +mod tests_doc_pr_reconcile; #[cfg(test)] mod tests_self_healing; #[cfg(test)] @@ -341,6 +352,18 @@ pub struct Overseer { /// status field + a single dual-channel NotifyOperator), never re-sent every /// tick while it stays disabled. merge_reasoning_disabled_notified: bool, + /// The `gh` client for the additive **auto-doc PR reconciliation** pass + /// (goal_hygiene): each due tick it lists open PRs across the governed + /// [`merge_queue_roster`](Self::merge_queue_roster) and closes stale / + /// superseded auto-generated `"Update documentation with …"` drafts so at + /// most one stays open ([`doc_pr_reconcile::run_doc_pr_reconcile`]). `None` + /// by default (bare constructor / tests) so the pass is skipped and the + /// Overseer performs NO `gh` I/O — existing `run_cycle` tests are unaffected; + /// `build_overseer` wires the production [`crate::stewardship::RealPrGhClient`]. + doc_pr_reconcile_gh: Option>, + /// Monotonic tick counter driving the auto-doc PR reconciliation every-N + /// cadence (reuses the shared gap-scan cadence knob). + doc_pr_reconcile_tick: u64, /// The agentic **health-review** rail ([standing]): the thin /// [`health_review::HealthReviewer`] seam that invokes the /// `overseer-health-review` recipe on the Overseer cadence — an AGENT reads @@ -551,6 +574,8 @@ impl Overseer { merge_queue_every_n: 1, merge_queue_tick: 0, merge_reasoning_disabled_notified: false, + doc_pr_reconcile_gh: None, + doc_pr_reconcile_tick: 0, health_reviewer: None, health_review_enabled: false, health_review_every_n: 1, @@ -667,6 +692,68 @@ impl Overseer { self } + /// Wire the `gh` client for the additive auto-doc PR reconciliation pass + /// (goal_hygiene). Absent by default (the pass is skipped and NO `gh` I/O is + /// performed); `build_overseer` wires the production + /// [`crate::stewardship::RealPrGhClient`]. The reconciliation scope is the + /// governed [`merge_queue_roster`](Self::merge_queue_roster). Gated at run + /// time by [`Self::with_gap_scan_enabled`] (the shared scan opt-out) and an + /// every-N cadence. + pub fn with_doc_pr_reconcile_client( + mut self, + gh: Box, + ) -> Self { + self.doc_pr_reconcile_gh = Some(gh); + self + } + + /// Reconcile stale/superseded auto-generated documentation PRs across the + /// governed roster so at most one stays open (goal_hygiene). Additive and + /// fully **fail-closed**: skipped entirely unless a `gh` client is wired + /// AND the shared gap-scan opt-out is enabled AND the every-N cadence is due; + /// a per-repo listing/close error is logged and contained — it never aborts + /// the cycle or changes any merge/verify decision. Mirrors the gating of the + /// merge-queue observe pass. + fn reconcile_auto_doc_prs(&mut self) { + let tick = self.doc_pr_reconcile_tick; + self.doc_pr_reconcile_tick = self.doc_pr_reconcile_tick.wrapping_add(1); + + let Some(gh) = self.doc_pr_reconcile_gh.as_ref() else { + return; + }; + if !self.gap_scan_enabled || self.merge_queue_roster.is_empty() { + return; + } + if !ecosystem_observe::should_observe(true, self.merge_queue_every_n, tick) { + return; + } + + for repo in &self.merge_queue_roster { + match doc_pr_reconcile::run_doc_pr_reconcile(repo, gh.as_ref()) { + Ok(report) => { + tracing::info!( + target: "simard::overseer", + repo = %repo, + canonical = report.canonical.map(|n| n as i64).unwrap_or(-1), + closed = report.closed.len(), + skipped = report.skipped, + errors = report.errors.len(), + "auto-doc PR reconciliation pass complete for repo", + ); + } + Err(e) => { + tracing::warn!( + target: "simard::overseer", + repo = %repo, + error = %e, + "auto-doc PR reconciliation skipped for repo (list failed) — \ + fail-closed, no PRs closed this cycle", + ); + } + } + } + } + /// Enable/disable the whisperer (config opt-out). Off by default; the daemon /// sets this from [`config::whisper_enabled`]. pub fn with_whisper_enabled(mut self, enabled: bool) -> Self { @@ -925,6 +1012,15 @@ impl Overseer { // empty scope / degraded recipe leaves the observation unchanged. self.observe_merge_queue(&mut observed, &in_flight); + // Additive auto-doc PR reconciliation (goal_hygiene): enforce the + // single-open invariant for auto-generated `"Update documentation with …"` + // PRs by closing stale / superseded drafts. Fully fail-closed and inert + // unless a `gh` client is wired (production only) — see + // [`reconcile_auto_doc_prs`]. Placed alongside the merge-queue observe + // pass (the PR/merge reconciliation surface); it changes no merge/verify + // decision. + self.reconcile_auto_doc_prs(); + // Drain diagnosed step failures (#2640, PART 2) from the process-global // failure sink into this Observe pass, so a caught decision-cycle / // engineer / terminal-shell failure surfaces as a corrective diff --git a/src/overseer/observer.rs b/src/overseer/observer.rs index 7fd937819..97c8f7423 100644 --- a/src/overseer/observer.rs +++ b/src/overseer/observer.rs @@ -129,12 +129,19 @@ pub fn decide_read_only(problem: &Problem) -> Intervention { /// consumed by [`StewardshipIssueFiler`] → `stewardship::process_orchestrator_run`, /// which routes on `source_module` and dedups on /// `failure_signature(failure_kind, error_text)`. +/// +/// The `dedup_key` is folded through [`fold_volatile_goal_ids`] before it flows +/// into BOTH `failure_kind` and the error text (process_health): a re-block +/// finding embeds a volatile goal identifier (`simard-identity-` / +/// positional `goal-`), so without folding every re-observation of the SAME +/// underlying cause produced a fresh `failure_signature` and filed a duplicate +/// `recurring_goal_reblock in simard::overseer` issue — the storm this ends. fn problem_to_run_brief(problem: &Problem) -> OrchestratorRunBrief { OrchestratorRunBrief { recipe_name: "overseer-observer".to_string(), failed_step: kind_step_label(problem.kind).to_string(), source_module: routable_source_module(problem), - failure_kind: problem.dedup_key.clone(), + failure_kind: fold_volatile_goal_ids(&problem.dedup_key), error_text: stable_error_text(problem), } } @@ -175,19 +182,72 @@ fn kind_step_label(kind: ProblemKind) -> &'static str { /// STABLE error text (no fluctuating metric values) so `failure_signature` folds /// every recurrence of the same problem into ONE deduplicated issue. Live metric /// values live in the periodic Report / `simard status` telemetry, not the issue -/// body. Keyed on the (already stable) `dedup_key` plus the evidence signal -/// kinds — both invariant across observation cycles for a given problem. +/// body. Keyed on the (already stable) `dedup_key` — with volatile goal +/// identifiers folded via [`fold_volatile_goal_ids`] so re-block recurrences of +/// the same cause collapse to one signature — plus the evidence signal kinds +/// (invariant across observation cycles for a given problem). fn stable_error_text(problem: &Problem) -> String { format!( "Overseer read-only observer detected a recurring {kind:?} problem \ (dedup key `{key}`; evidence: {kinds}). Filed once per recurring \ signature — see `simard status` telemetry for current values.", kind = problem.kind, - key = problem.dedup_key, + key = fold_volatile_goal_ids(&problem.dedup_key), kinds = evidence_kind_labels(&problem.evidence), ) } +/// Fold **volatile goal identifiers** in a stewardship `dedup_key` to stable +/// placeholders so recurrences of the SAME re-block cause collapse to ONE +/// `failure_signature` (process_health). Two shapes are folded: +/// +/// * `simard-identity-` → `simard-identity-*` (the codename identity goals, +/// whose slug is a volatile lowercase-and-hyphen codename), and +/// * positional `goal-` (a run of ASCII digits) → `goal-*`. +/// +/// Everything else is returned **byte-for-byte** — the fold is deliberately +/// conservative so two *genuinely different* causes never over-collapse into one +/// issue. `goal-` NOT followed by a digit (e.g. `coverage-goal-parity`) and a +/// bare `identity` are left untouched. Pure and total; no `regex` dependency (a +/// single forward scan) so it is always compiled in. +pub fn fold_volatile_goal_ids(dedup_key: &str) -> String { + const IDENTITY_PREFIX: &str = "simard-identity-"; + const GOAL_PREFIX: &str = "goal-"; + // A slug byte: the lowercase-and-hyphen (plus defensive alphanumeric) run + // that makes up a codename identity slug. Terminated by a space, `:` etc. + fn is_slug_byte(b: u8) -> bool { + b.is_ascii_alphanumeric() || b == b'-' + } + + let bytes = dedup_key.as_bytes(); + let mut out = String::with_capacity(dedup_key.len()); + let mut i = 0; + while i < bytes.len() { + let rest = &dedup_key[i..]; + if let Some(slug) = rest.strip_prefix(IDENTITY_PREFIX) { + let slug_len = slug.bytes().take_while(|&b| is_slug_byte(b)).count(); + if slug_len > 0 { + out.push_str("simard-identity-*"); + i += IDENTITY_PREFIX.len() + slug_len; + continue; + } + } + if let Some(after) = rest.strip_prefix(GOAL_PREFIX) { + let digits = after.bytes().take_while(u8::is_ascii_digit).count(); + if digits > 0 { + out.push_str("goal-*"); + i += GOAL_PREFIX.len() + digits; + continue; + } + } + // Default: copy exactly one UTF-8 scalar, preserving char boundaries. + let ch = rest.chars().next().expect("non-empty remainder"); + out.push(ch); + i += ch.len_utf8(); + } + out +} + /// De-duplicated, order-stable list of the signal *variant* names backing a /// problem (values omitted — only kinds, so the string is invariant per cycle). fn evidence_kind_labels(evidence: &[Signal]) -> String { diff --git a/src/overseer/tests_doc_pr_reconcile.rs b/src/overseer/tests_doc_pr_reconcile.rs new file mode 100644 index 000000000..d654881db --- /dev/null +++ b/src/overseer/tests_doc_pr_reconcile.rs @@ -0,0 +1,475 @@ +//! TEST-FIRST (Step 7 TDD) — the overseer's auto-generated documentation-PR +//! **reconciliation** pass that ends the stale-auto-doc-PR churn (goal_hygiene). +//! +//! # The churn these tests kill +//! +//! An automated doc-update flow opens a fresh `"Update documentation with N +//! changed files"` PR per doc-drift event without deduping, rebasing, or +//! auto-closing superseded ones. In the field ~30 stale, CONFLICTING, draft +//! auto-doc PRs accumulated (oldest from 2026-07-22), rotting unmerged. +//! +//! # The contract (what the fix must make true) +//! +//! 1. A composite, **fail-closed** identity gate `is_auto_doc_pr` positively +//! identifies an auto-doc PR only when EVERY signal holds (title marker + +//! known auto-generation author + draft + label). A human PR — or one with an +//! empty/absent author — is never a candidate. +//! 2. A pure `reconcile_doc_prs` keeps the single newest auto-doc PR (canonical) +//! and queues every other candidate for close, tagged `SupersededDuplicate` +//! or `StaleConflictingDraft`. The canonical PR is NEVER in the close set, so +//! the pass can never close every candidate. Non-auto-doc PRs are ignored. +//! 3. The `run_doc_pr_reconcile` executor closes by NUMBER via the additive, +//! default-no-op `PrGhClient::close_pr`, and is fail-closed on a list error +//! (no closes that cycle). +//! +//! `reconcile_doc_prs` / `is_auto_doc_pr` are pure (no I/O) and exhaustively +//! unit-tested on fixture PR lists. RED until the `doc_pr_reconcile` module and +//! the `close_pr` trait method exist. + +use std::cell::RefCell; + +use crate::error::{SimardError, SimardResult}; +use crate::overseer::config::DEFAULT_OVERSEER_AUTHOR_LOGIN; +use crate::overseer::doc_pr_reconcile::{ + AUTO_DOC_PR_LABEL, AUTO_DOC_PR_TITLE_MARKER, CloseReason, is_auto_doc_pr, reconcile_doc_prs, + run_doc_pr_reconcile_with_author, +}; +use crate::stewardship::PrSnapshot; +use crate::stewardship::merge_authority::{OpenPrSummary, PrGhClient}; + +const REPO: &str = "rysweet/Simard"; + +/// The auto-doc author identity used by the fixtures — Simard's ENGINEER / OODA +/// gh login (what `config::automerge_author` resolves in production). It is +/// deliberately DISTINCT from [`DEFAULT_OVERSEER_AUTHOR_LOGIN`] so a fixture can +/// never accidentally bind the gate's expected author to the overseer-bot login +/// (the masked-bug the HIGH review finding exposed). +const ENGINEER_AUTHOR: &str = "simard-engineer"; + +// --- fixtures --------------------------------------------------------------- + +/// A canonical auto-doc PR that passes EVERY gate signal, parameterised by +/// number and `mergeable` state. +fn auto_doc_pr(number: u32, mergeable: &str) -> OpenPrSummary { + OpenPrSummary { + number, + title: format!("{AUTO_DOC_PR_TITLE_MARKER} {number} changed files"), + mergeable: mergeable.to_string(), + author: ENGINEER_AUTHOR.to_string(), + labels: vec![AUTO_DOC_PR_LABEL.to_string()], + is_draft: Some(true), + ..Default::default() + } +} + +/// A human PR that must NEVER be reconciled. +fn human_pr(number: u32) -> OpenPrSummary { + OpenPrSummary { + number, + title: format!("{AUTO_DOC_PR_TITLE_MARKER} {number} changed files"), // same title! + mergeable: "MERGEABLE".to_string(), + author: "a-human-contributor".to_string(), + labels: vec![], + is_draft: Some(false), + ..Default::default() + } +} + +// --- fake gh client --------------------------------------------------------- + +struct FakeDocPrClient { + open: Vec, + list_fails: bool, + closed: RefCell>, +} + +impl FakeDocPrClient { + fn with(open: Vec) -> Self { + Self { + open, + list_fails: false, + closed: RefCell::new(Vec::new()), + } + } + fn failing() -> Self { + Self { + open: Vec::new(), + list_fails: true, + closed: RefCell::new(Vec::new()), + } + } + fn closed_numbers(&self) -> Vec { + self.closed.borrow().iter().map(|(n, _)| *n).collect() + } +} + +impl PrGhClient for FakeDocPrClient { + fn view_pr(&self, _repo: &str, _pr: u32) -> SimardResult { + unreachable!("reconciliation never views a PR") + } + fn squash_merge(&self, _repo: &str, _pr: u32) -> SimardResult<()> { + unreachable!("reconciliation never merges a PR") + } + fn list_open_prs(&self, _repo: &str, _limit: u32) -> SimardResult> { + if self.list_fails { + return Err(SimardError::StewardshipGhCommandFailed { + reason: "gh pr list transport failure".to_string(), + }); + } + Ok(self.open.clone()) + } + fn close_pr(&self, _repo: &str, number: u32, comment: &str) -> SimardResult<()> { + self.closed.borrow_mut().push((number, comment.to_string())); + Ok(()) + } +} + +// === is_auto_doc_pr: composite fail-closed gate ============================= + +#[test] +fn a_fully_qualified_pr_is_an_auto_doc_pr() { + assert!( + is_auto_doc_pr(&auto_doc_pr(42, "MERGEABLE"), ENGINEER_AUTHOR), + "title marker + auto-gen author + draft + label ⇒ auto-doc PR" + ); +} + +#[test] +fn empty_author_fails_closed_as_human() { + let mut pr = auto_doc_pr(42, "MERGEABLE"); + pr.author = String::new(); + assert!( + !is_auto_doc_pr(&pr, ENGINEER_AUTHOR), + "an empty/absent author must fail closed (treated as human) — never a candidate" + ); +} + +#[test] +fn a_human_authored_pr_with_the_same_title_is_not_auto_doc() { + assert!( + !is_auto_doc_pr(&human_pr(42), ENGINEER_AUTHOR), + "a human author (even with the same title) is never an auto-doc candidate" + ); +} + +#[test] +fn a_non_draft_pr_is_not_auto_doc() { + let mut none_draft = auto_doc_pr(42, "MERGEABLE"); + none_draft.is_draft = None; + assert!( + !is_auto_doc_pr(&none_draft, ENGINEER_AUTHOR), + "is_draft None must fail closed (only Some(true) qualifies)" + ); + let mut ready = auto_doc_pr(42, "MERGEABLE"); + ready.is_draft = Some(false); + assert!( + !is_auto_doc_pr(&ready, ENGINEER_AUTHOR), + "is_draft Some(false) must fail closed" + ); +} + +#[test] +fn a_pr_missing_the_label_is_not_auto_doc() { + let mut pr = auto_doc_pr(42, "MERGEABLE"); + pr.labels = vec!["some-other-label".to_string()]; + assert!( + !is_auto_doc_pr(&pr, ENGINEER_AUTHOR), + "the auto-doc label must be present" + ); +} + +#[test] +fn a_pr_with_the_wrong_title_is_not_auto_doc() { + let mut pr = auto_doc_pr(42, "MERGEABLE"); + pr.title = "Fix a real bug in the OODA loop".to_string(); + assert!( + !is_auto_doc_pr(&pr, ENGINEER_AUTHOR), + "the title must start with the auto-doc marker" + ); +} + +#[test] +fn an_overseer_bot_authored_pr_is_not_an_auto_doc_candidate() { + // Regression for the HIGH review finding: the gate USED to hard-code the + // expected author to `simard-overseer[bot]` (DEFAULT_OVERSEER_AUTHOR_LOGIN), + // but real auto-doc PRs are authored under Simard's ENGINEER identity. An + // auto-doc-SHAPED PR carrying the overseer-bot login must NOT match the + // engineer expected-author — otherwise the gate would key on the wrong + // identity and match zero real PRs in production (inert). + let mut bot = auto_doc_pr(42, "MERGEABLE"); + bot.author = DEFAULT_OVERSEER_AUTHOR_LOGIN.to_string(); + assert!( + !is_auto_doc_pr(&bot, ENGINEER_AUTHOR), + "an overseer-bot-authored PR must NOT match the engineer auto-doc identity" + ); + // And the engineer-authored auto-doc PR IS a candidate — the gate is now + // keyed on the identity that actually opens auto-doc PRs. + assert!( + is_auto_doc_pr(&auto_doc_pr(42, "MERGEABLE"), ENGINEER_AUTHOR), + "the engineer-authored auto-doc PR is a candidate under the engineer identity" + ); + // Symmetry check: if the deployment's auto-doc author WERE the bot, the bot + // PR matches under THAT expected author — proving the gate tracks whatever + // identity is resolved at the I/O boundary rather than a frozen constant. + assert!( + is_auto_doc_pr(&bot, DEFAULT_OVERSEER_AUTHOR_LOGIN), + "the gate matches whichever author identity is injected" + ); +} + +#[test] +fn an_empty_expected_author_fails_closed() { + // `config::automerge_author()` returns None (=> empty string here) when + // SIMARD_AUTOMERGE_AUTHOR is unset. An unresolved identity must match NOTHING + // so the reconciler stays fail-closed rather than closing PRs under a blank + // author (which an empty pr.author would otherwise satisfy). + assert!( + !is_auto_doc_pr(&auto_doc_pr(42, "MERGEABLE"), ""), + "an empty expected author (unset SIMARD_AUTOMERGE_AUTHOR) must match nothing" + ); + let mut blank_authored = auto_doc_pr(42, "MERGEABLE"); + blank_authored.author = String::new(); + assert!( + !is_auto_doc_pr(&blank_authored, ""), + "empty-vs-empty must NOT match — both author guards fail closed" + ); +} + +// === reconcile_doc_prs: pure decision core ================================== + +#[test] +fn zero_candidates_is_a_no_op() { + let decision = reconcile_doc_prs(&[], ENGINEER_AUTHOR); + assert_eq!(decision.canonical, None); + assert!(decision.to_close.is_empty()); +} + +#[test] +fn a_single_candidate_is_canonical_with_no_closes() { + let decision = reconcile_doc_prs(&[auto_doc_pr(7, "MERGEABLE")], ENGINEER_AUTHOR); + assert_eq!( + decision.canonical, + Some(7), + "the lone auto-doc PR is the survivor" + ); + assert!( + decision.to_close.is_empty(), + "the single-open invariant already holds — nothing to close" + ); +} + +#[test] +fn the_newest_candidate_is_canonical_and_the_rest_are_superseded() { + let decision = reconcile_doc_prs( + &[ + auto_doc_pr(10, "MERGEABLE"), + auto_doc_pr(30, "MERGEABLE"), + auto_doc_pr(20, "MERGEABLE"), + ], + ENGINEER_AUTHOR, + ); + assert_eq!( + decision.canonical, + Some(30), + "the newest (highest-number) candidate is the keeper" + ); + let mut closed: Vec = decision.to_close.iter().map(|c| c.number).collect(); + closed.sort_unstable(); + assert_eq!( + closed, + vec![10, 20], + "every OTHER candidate is queued for close" + ); + assert!( + decision.to_close.iter().all(|c| c.number != 30), + "the canonical PR must NEVER be in the close set" + ); + assert!( + decision + .to_close + .iter() + .all(|c| c.reason == CloseReason::SupersededDuplicate), + "mergeable duplicates are tagged SupersededDuplicate" + ); +} + +#[test] +fn a_conflicting_duplicate_is_tagged_stale_conflicting_draft() { + let decision = reconcile_doc_prs( + &[ + auto_doc_pr(50, "MERGEABLE"), // canonical (newest) + auto_doc_pr(40, "CONFLICTING"), + auto_doc_pr(30, "MERGEABLE"), + ], + ENGINEER_AUTHOR, + ); + assert_eq!(decision.canonical, Some(50)); + let conflicting = decision + .to_close + .iter() + .find(|c| c.number == 40) + .expect("the conflicting duplicate must be queued for close"); + assert_eq!( + conflicting.reason, + CloseReason::StaleConflictingDraft, + "a CONFLICTING duplicate must be auto-closed as a stale conflicting draft" + ); + let clean = decision + .to_close + .iter() + .find(|c| c.number == 30) + .expect("the clean older duplicate must be queued for close"); + assert_eq!(clean.reason, CloseReason::SupersededDuplicate); +} + +#[test] +fn non_auto_doc_prs_are_ignored_entirely() { + // A human PR sharing the title, plus a single genuine auto-doc PR: the human + // PR is neither canonical nor closed. + let decision = reconcile_doc_prs( + &[human_pr(99), auto_doc_pr(7, "MERGEABLE")], + ENGINEER_AUTHOR, + ); + assert_eq!( + decision.canonical, + Some(7), + "only the genuine auto-doc PR is a candidate" + ); + assert!( + decision.to_close.iter().all(|c| c.number != 99), + "a human PR must never be closed by reconciliation" + ); + assert!(decision.to_close.is_empty()); +} + +// === run_doc_pr_reconcile: executor ========================================= + +#[test] +fn executor_closes_the_superseded_prs_by_number() { + let gh = FakeDocPrClient::with(vec![ + auto_doc_pr(10, "MERGEABLE"), + auto_doc_pr(20, "CONFLICTING"), + auto_doc_pr(30, "MERGEABLE"), + ]); + let report = + run_doc_pr_reconcile_with_author(REPO, &gh, ENGINEER_AUTHOR).expect("reconcile succeeds"); + + assert_eq!( + report.canonical, + Some(30), + "the newest PR survives the single-open invariant" + ); + let mut closed = report.closed.clone(); + closed.sort_unstable(); + assert_eq!(closed, vec![10, 20], "the older duplicates are closed"); + + let mut executed = gh.closed_numbers(); + executed.sort_unstable(); + assert_eq!( + executed, + vec![10, 20], + "the executor must close exactly the superseded PRs, by number" + ); + assert!( + !gh.closed_numbers().contains(&30), + "the canonical PR is never closed" + ); +} + +#[test] +fn executor_is_fail_closed_on_a_list_error() { + let gh = FakeDocPrClient::failing(); + let result = run_doc_pr_reconcile_with_author(REPO, &gh, ENGINEER_AUTHOR); + assert!( + result.is_err(), + "a listing failure must surface as an error" + ); + assert!( + gh.closed_numbers().is_empty(), + "NO PR may be closed when the open-PR listing failed (fail-closed)" + ); +} + +#[test] +fn executor_never_closes_a_human_pr() { + let gh = FakeDocPrClient::with(vec![ + human_pr(99), + auto_doc_pr(10, "MERGEABLE"), + auto_doc_pr(20, "MERGEABLE"), + ]); + let report = + run_doc_pr_reconcile_with_author(REPO, &gh, ENGINEER_AUTHOR).expect("reconcile succeeds"); + assert_eq!(report.canonical, Some(20)); + assert!( + !gh.closed_numbers().contains(&99), + "a human PR must never be closed" + ); + assert_eq!( + gh.closed_numbers(), + vec![10], + "only the superseded auto-doc duplicate is closed" + ); +} + +#[test] +fn executor_is_inert_when_the_author_identity_mismatches() { + // The exact production failure the HIGH finding described: title-marker, + // draft, labelled auto-doc PRs are present, but the resolved expected author + // does NOT match the PRs' author. The gate matches ZERO candidates, so the + // executor closes NOTHING (fail-closed) rather than acting on a wrong or + // mis-resolved identity. (The executor also emits a LOUD warn here.) + let gh = FakeDocPrClient::with(vec![ + auto_doc_pr(10, "MERGEABLE"), + auto_doc_pr(20, "MERGEABLE"), + ]); + let report = run_doc_pr_reconcile_with_author(REPO, &gh, DEFAULT_OVERSEER_AUTHOR_LOGIN) + .expect("reconcile succeeds (inert)"); + assert_eq!( + report.canonical, None, + "no candidate matches the mismatched author => no canonical" + ); + assert!( + report.closed.is_empty() && gh.closed_numbers().is_empty(), + "an author mismatch must close NOTHING (fail-closed inert pass)" + ); + assert_eq!( + report.skipped, 2, + "both title-marker PRs are counted as skipped non-candidates" + ); +} + +#[test] +fn executor_is_inert_when_the_author_is_unresolved() { + // SIMARD_AUTOMERGE_AUTHOR unset => automerge_author() is None => empty + // expected author. The pass must be fully inert (and warn LOUD). + let gh = FakeDocPrClient::with(vec![ + auto_doc_pr(10, "MERGEABLE"), + auto_doc_pr(20, "MERGEABLE"), + ]); + let report = + run_doc_pr_reconcile_with_author(REPO, &gh, "").expect("reconcile succeeds (inert)"); + assert_eq!(report.canonical, None); + assert!( + gh.closed_numbers().is_empty(), + "an unresolved author identity must close NOTHING" + ); +} + +#[test] +fn close_pr_defaults_to_a_no_op_for_unwired_clients() { + // The additive trait method must default to a no-op so every existing + // fake / unwired client performs NO mutation. + struct MinimalClient; + impl PrGhClient for MinimalClient { + fn view_pr(&self, _repo: &str, _pr: u32) -> SimardResult { + unreachable!() + } + fn squash_merge(&self, _repo: &str, _pr: u32) -> SimardResult<()> { + unreachable!() + } + } + // Must compile (default method present) and succeed without side effects. + MinimalClient + .close_pr(REPO, 123, "superseded by #456") + .expect("the default close_pr is a no-op that returns Ok"); +} diff --git a/src/overseer/tests_reblock_signature.rs b/src/overseer/tests_reblock_signature.rs new file mode 100644 index 000000000..52aae6bce --- /dev/null +++ b/src/overseer/tests_reblock_signature.rs @@ -0,0 +1,284 @@ +//! TEST-FIRST (Step 7 TDD) — reblock-issue **signature stabilization** that ends +//! the Overseer's `recurring_goal_reblock in simard::overseer` stewardship-issue +//! churn (process_health, HIGH). +//! +//! # The churn these tests kill +//! +//! When the Overseer re-observes a goal being re-blocked it files a deduplicated +//! stewardship issue keyed on `failure_signature(failure_kind, error_text)`. The +//! `dedup_key` (which flows into BOTH `failure_kind` and the error text) embeds a +//! **volatile goal identifier** — e.g. `simard-identity-` or a positional +//! `goal-`. So every re-observation of the *same* underlying re-block cause +//! produces a different signature and files a fresh issue: the observed storm of +//! `recurring_goal_reblock in simard::overseer` issues (8 open in 24h). +//! +//! # The contract (what the fix must make true) +//! +//! 1. A new pure, total helper `fold_volatile_goal_ids(dedup_key)` folds the +//! known volatile identifier shapes to stable placeholders and returns +//! everything else byte-for-byte (conservative — distinct causes keep distinct +//! keys). +//! 2. Applied upstream of the existing `failure_signature`, two re-block +//! observations that differ ONLY by a volatile goal id must collapse to ONE +//! `failure_signature` (so the stewardship dedup files ONE issue), while two +//! genuinely different causes keep distinct signatures. +//! +//! The end-to-end property is asserted through the public `decide_read_only` +//! seam (which builds the brief the stewardship filer dedups on). RED until +//! `fold_volatile_goal_ids` exists and is applied on the reblock path. + +use super::observer::fold_volatile_goal_ids; +use super::{Intervention, decide_read_only}; +use crate::overseer::signal::{Priority, Problem, ProblemKind}; +use crate::stewardship::failure_signature; + +// --- fixtures --------------------------------------------------------------- + +/// Build a process-health problem (the reblock family routes here → FileIssue) +/// with the given dedup key. +fn reblock_problem(dedup_key: &str) -> Problem { + Problem { + kind: ProblemKind::ProcessHealth, + priority: Priority::Normal, + dedup_key: dedup_key.to_string(), + summary: "recurring_goal_reblock in simard::overseer".to_string(), + evidence: vec![], + why: None, + } +} + +/// The stewardship dedup signature the filer would compute for a problem — +/// extracted through the public Decide seam so the test tracks the real path. +fn dedup_signature(problem: &Problem) -> String { + match decide_read_only(problem) { + Intervention::FileIssue { run } => failure_signature(&run.failure_kind, &run.error_text), + other => panic!("a ProcessHealth problem must route to FileIssue, got {other:?}"), + } +} + +// === fold_volatile_goal_ids: the folding table ============================== + +#[test] +fn folds_simard_identity_slugs_to_a_stable_placeholder() { + assert_eq!( + fold_volatile_goal_ids("recurring_goal_reblock simard-identity-atelier-furniture-de"), + "recurring_goal_reblock simard-identity-*", + "a simard-identity- id must fold to the stable simard-identity-* placeholder" + ); + // Two DIFFERENT identity slugs fold to the SAME string. + assert_eq!( + fold_volatile_goal_ids("reblock simard-identity-luxe-coastal-lighting"), + fold_volatile_goal_ids("reblock simard-identity-artisan-heritage-textiles"), + "distinct identity slugs of the same cause must fold identically" + ); +} + +#[test] +fn folds_positional_goal_slugs_to_a_stable_placeholder() { + assert_eq!( + fold_volatile_goal_ids("recurring_goal_reblock goal-12"), + "recurring_goal_reblock goal-*", + "a positional goal- id must fold to the stable goal-* placeholder" + ); + assert_eq!( + fold_volatile_goal_ids("reblock goal-3"), + fold_volatile_goal_ids("reblock goal-9871"), + "distinct positional goal ids of the same cause must fold identically" + ); +} + +#[test] +fn leaves_unrelated_text_byte_for_byte() { + // Conservative: only the known volatile shapes are rewritten; everything else + // passes through untouched so genuinely different causes keep distinct keys. + for s in [ + "PanicInStep", + "process:distill_fail", + "recurring_goal_reblock in simard::overseer", + "coverage-goal-parity", // not the `goal-` shape + "identity", // not the `simard-identity-` shape + ] { + assert_eq!( + fold_volatile_goal_ids(s), + s, + "unrelated text must be returned byte-for-byte: {s:?}" + ); + } +} + +/// Edge / negative cases (Step 18b review, finding #4): pin the exact fold +/// boundaries so a future refactor of the single-scan folder cannot silently +/// over-collapse a distinct cause OR under-fold a real volatile id. Each case +/// documents WHY the folder does (or does not) rewrite it. +#[test] +fn fold_boundaries_are_exact_and_utf8_safe() { + // `goal-` NOT followed by an ASCII digit is NOT the positional shape — it + // must survive untouched (a real goal-id run needs at least one digit). + for untouched in [ + "goal-", // trailing prefix, no slug at all + "goal-abc", // letters, not digits + "goal-x1", // starts with a letter, not a digit + "goal- 12", // a space breaks the digit run immediately + "reblock goal-", // trailing prefix mid-string + "simard-identity-", // trailing identity prefix, EMPTY slug + "simard-identity- x", // space breaks the slug run immediately + ] { + assert_eq!( + fold_volatile_goal_ids(untouched), + untouched, + "a non-matching volatile-prefix shape must be returned byte-for-byte: {untouched:?}" + ); + } + + // Partial fold: the DIGIT run folds, trailing non-digits are preserved + // verbatim — the fold consumes exactly `goal-` and no more. + assert_eq!( + fold_volatile_goal_ids("goal-12abc"), + "goal-*abc", + "only the leading digit run of a positional id folds; the rest is preserved" + ); + + // Every occurrence in a key folds independently. + assert_eq!( + fold_volatile_goal_ids("goal-1 blocks goal-4087"), + "goal-* blocks goal-*", + "multiple positional ids in one key each fold to the stable placeholder" + ); + assert_eq!( + fold_volatile_goal_ids("simard-identity-nordic-hearth and goal-9"), + "simard-identity-* and goal-*", + "identity and positional shapes fold together in the same key" + ); + + // UTF-8 safety: multibyte scalars adjacent to a fold must be copied whole + // (never split on a byte boundary), and the fold itself is unaffected. + assert_eq!( + fold_volatile_goal_ids("café goal-7 ☕ simard-identity-x1 ✓"), + "café goal-* ☕ simard-identity-* ✓", + "multibyte characters around a fold must survive intact (char-boundary safe)" + ); +} + +// === end-to-end dedup property (the churn stopper) ========================== + +#[test] +fn reblocks_differing_only_by_identity_slug_dedup_to_one_signature() { + let a = reblock_problem("recurring_goal_reblock simard-identity-atelier-furniture-de"); + let b = reblock_problem("recurring_goal_reblock simard-identity-luxe-coastal-lighting"); + + assert_ne!( + a.dedup_key, b.dedup_key, + "the two dedup keys genuinely differ (only by the volatile identity slug)" + ); + assert_eq!( + dedup_signature(&a), + dedup_signature(&b), + "two re-block observations of the SAME cause that differ only by a volatile \ + simard-identity slug must collapse to ONE stewardship signature — otherwise a fresh \ + `recurring_goal_reblock` issue is filed every cycle (the storm)" + ); +} + +#[test] +fn reblocks_differing_only_by_positional_goal_id_dedup_to_one_signature() { + let a = reblock_problem("recurring_goal_reblock goal-12 in simard::overseer"); + let b = reblock_problem("recurring_goal_reblock goal-4087 in simard::overseer"); + + assert_ne!(a.dedup_key, b.dedup_key); + assert_eq!( + dedup_signature(&a), + dedup_signature(&b), + "re-blocks differing only by a positional goal id must dedup to ONE signature" + ); +} + +#[test] +fn genuinely_different_reblock_causes_keep_distinct_signatures() { + // The fix must NOT over-collapse: two DIFFERENT underlying causes keep + // distinct signatures so each still gets its own tracked issue. + let admission = reblock_problem("recurring_goal_reblock goal-12 admission-gate-rejected"); + let unclear = reblock_problem("recurring_goal_reblock goal-12 unclear-criteria"); + assert_ne!( + dedup_signature(&admission), + dedup_signature(&unclear), + "distinct re-block causes must keep distinct signatures (no over-collapse)" + ); +} + +// === MEDIUM (Step 17b review): verify the fold against the REAL, production === +// === dedup_key shapes `classify_signal` actually emits ====================== +// +// The prior fold tests used illustrative `"recurring_goal_reblock "` keys. +// The reviewer asked to pin the fold against a CAPTURED real reblock dedup_key. +// The production keys carrying a volatile goal id are emitted verbatim by +// `crate::overseer::mod::classify_signal`: +// * GoalBlocked => `format!("goal:blocked:{goal_id}")` +// * StaleGoal => `format!("goal:stale:{goal_id}")` +// * LoopDetected => `format!("loop:{goal_id}")` +// * DriftCorrection => `format!("drift:{goal_id}")` +// and the goal_id itself is a volatile `simard-identity-` or `goal-`. +// These tests assert the fold folds exactly those real shapes (and only the +// volatile suffix), so recurrences of the same cause collapse to ONE signature. + +#[test] +fn folds_real_goal_blocked_dedup_key_shape() { + // Real production key: `goal:blocked:`. + assert_eq!( + fold_volatile_goal_ids("goal:blocked:simard-identity-luxe-coastal-lighting"), + "goal:blocked:simard-identity-*", + "the real GoalBlocked key must fold its volatile identity slug" + ); + assert_eq!( + fold_volatile_goal_ids("goal:blocked:goal-4087"), + "goal:blocked:goal-*", + "the real GoalBlocked key must fold its volatile positional id" + ); + // Two re-block recurrences of the SAME cause (differing only by the volatile + // goal id) collapse to ONE folded key across BOTH identity shapes. + assert_eq!( + fold_volatile_goal_ids("goal:blocked:simard-identity-atelier-furniture-de"), + fold_volatile_goal_ids("goal:blocked:simard-identity-luxe-coastal-lighting"), + ); + assert_eq!( + fold_volatile_goal_ids("goal:blocked:goal-12"), + fold_volatile_goal_ids("goal:blocked:goal-4087"), + ); +} + +#[test] +fn folds_real_loop_stale_and_drift_dedup_key_shapes() { + // `loop:`, `goal:stale:`, `drift:` — the other + // real classify_signal shapes that embed a volatile goal id. + assert_eq!( + fold_volatile_goal_ids("loop:simard-identity-nordic-hearth-ceramics"), + "loop:simard-identity-*", + ); + assert_eq!( + fold_volatile_goal_ids("goal:stale:goal-9871"), + "goal:stale:goal-*", + ); + assert_eq!(fold_volatile_goal_ids("drift:goal-3"), "drift:goal-*"); + // The stable `goal:blocked:` / `goal:stale:` prefixes contain the substring + // "goal" but NOT the `goal-` shape, so they are preserved untouched — + // only the trailing volatile id is folded (no over-collapse of the prefix). + assert_eq!( + fold_volatile_goal_ids("goal:stale:simard-identity-x1"), + "goal:stale:simard-identity-*", + ); +} + +#[test] +fn real_reblock_keys_of_different_goals_dedup_to_one_signature_end_to_end() { + // End-to-end through the Decide seam using the REAL `goal:blocked:` key + // shape: two blocked-goal recurrences differing only by the volatile goal id + // must file ONE stewardship signature. + let a = reblock_problem("goal:blocked:simard-identity-atelier-furniture-de"); + let b = reblock_problem("goal:blocked:simard-identity-luxe-coastal-lighting"); + assert_ne!(a.dedup_key, b.dedup_key, "the raw keys genuinely differ"); + assert_eq!( + dedup_signature(&a), + dedup_signature(&b), + "real GoalBlocked re-block keys differing only by the volatile identity slug must \ + collapse to ONE stewardship signature" + ); +} diff --git a/src/overseer/wiring.rs b/src/overseer/wiring.rs index abc075cdb..de43744a5 100644 --- a/src/overseer/wiring.rs +++ b/src/overseer/wiring.rs @@ -1321,6 +1321,16 @@ pub fn build_overseer( None => overseer, }; + // Auto-doc PR reconciliation client (goal_hygiene): wire the production + // `gh` client so the additive reconciliation pass can enforce the + // single-open invariant for auto-generated `"Update documentation with …"` + // PRs across the governed roster (closing stale / superseded drafts). Always + // wired here; the pass itself is gated at run time behind the governed roster + // being non-empty, the shared gap-scan opt-out, and the every-N cadence, and + // is fully fail-closed per repo. + let overseer = + overseer.with_doc_pr_reconcile_client(Box::new(crate::stewardship::RealPrGhClient)); + // Live agentic health-review rail ([standing]): on the Overseer cadence the // thin rail invokes the `overseer-health-review` recipe — an AGENT reads the // OODA journal + `simard status` + `simard goal list`, detects crash-loops / diff --git a/src/stewardship/merge_authority.rs b/src/stewardship/merge_authority.rs index a62258684..6c008200a 100644 --- a/src/stewardship/merge_authority.rs +++ b/src/stewardship/merge_authority.rs @@ -224,6 +224,25 @@ pub trait PrGhClient { reason: "run_gh not wired on this PrGhClient (fail-closed)".to_string(), }) } + + /// `gh pr close --repo --comment `. + /// + /// Added for the overseer's auto-doc-PR reconciliation pass (goal_hygiene): + /// it closes stale / superseded auto-generated `"Update documentation with + /// …"` drafts so at most one stays open. The `comment` is authored by the + /// reconciler (never operator-supplied free text) and explains WHY the PR was + /// closed (superseded by the canonical PR / stale CONFLICTING draft). + /// + /// The default impl is a **no-op** returning `Ok(())` so every existing + /// fake / unwired client performs NO mutation without needing a stub; + /// [`RealPrGhClient`] overrides it to shell out to `gh pr close` (argv-only, + /// never shell-interpolated). A no-op default (rather than the fail-closed + /// [`run_gh`](Self::run_gh) posture) is safe here because closing is a + /// hygiene convenience, not a correctness gate — a client that cannot close + /// simply leaves the duplicates open rather than erroring the cycle. + fn close_pr(&self, _repo: &str, _pr_number: u32, _comment: &str) -> SimardResult<()> { + Ok(()) + } } /// Max retry attempts for *transient* `gh` read failures (network blips, @@ -481,6 +500,18 @@ impl PrGhClient for RealPrGhClient { run_gh_checked(&label, &refs)?; Ok(()) } + + /// Close a PR with an explanatory comment. Single attempt (a close is a + /// mutation, like [`squash_merge`](Self::squash_merge)); fail-visible on a + /// non-zero exit. Argv is positional / never shell-interpolated. + fn close_pr(&self, repo: &str, pr_number: u32, comment: &str) -> SimardResult<()> { + let pr = pr_number.to_string(); + run_gh_checked( + &format!("gh pr close {pr} --repo {repo}"), + &["pr", "close", &pr, "--repo", repo, "--comment", comment], + )?; + Ok(()) + } } /// Parse `gh pr view --json body,statusCheckRollup,mergeable,reviewDecision,baseRefName`