From 50ab5d0f996649898eaf7e1eddefaafa2a0f039b Mon Sep 17 00:00:00 2001 From: rysweet Date: Tue, 28 Jul 2026 15:00:55 +0000 Subject: [PATCH] fix(ooda): back the no-progress breaker's issue-storm suppression with a GitHub-durable goal-key backstop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The board-local `WipRef` suppression marker stops the UNCLEAR-CRITERIA duplicate-issue storm only while the goal id is stable and the goal board survives — two conditions that fail under goal-id churn (a fresh id each cycle has no marker) and a goal-board reset (the marker is erased), letting the breaker re-file identical `ooda-stuck` issues. Add an additive, non-breaking second suppression guard: - `fold_goal_identity` (goal_curation::no_progress_breaker): pure, total, injection-safe folding of a churny `ActiveGoal.id` to `sha256[..8]` = 16 lowercase hex, mirroring `stewardship::failure_signature`. Safe to embed and to interpolate into a `gh --search` argument (SR1). - `NoProgressIssueFiler::issue_open_with_marker` trait method, default `false` so every existing impl/fake is unchanged. `GhIssueFiler` implements it as a direct argv-only `gh issue list --state open --label ooda-stuck --search ' in:body' --json number,body` query, confirming the match against the strongly-consistent JSON `body` (`body_contains_marker`); fails OPEN on any error (SR2/SR4/SR5). - Two-guard filing gate in `escalate_with_tracking_issue`: board-local `WipRef` fast path first (zero steady-state API calls, SR7), backstop only on a miss; a backstop hit re-seeds the `WipRef` and suppresses re-filing. The filed body embeds a trailing `ooda-goal-key: ` line so a later churned/reset cycle recognises the breaker's own open issue. - Promote `redact_token` / `redact_uuids` to `pub(crate)` (stewardship::dedup) so goal_curation can reuse them for body redaction (SR3/SR6) — non-breaking visibility widening. Tests cover first-filing, duplicate suppression across id churn / board reset, the zero-API fast path, fail-open, and the pure fold contract (charset/determinism/no-leak). Docs added for the concept, how-to, and API reference, wired into mkdocs nav. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../no-progress-breaker-goal-key-backstop.md | 209 +++++++++ .../no-progress-breaker-storm-suppression.md | 2 + ...no-progress-breaker-open-issue-backstop.md | 180 ++++++++ ...gnose-a-no-progress-breaker-issue-storm.md | 3 + ...-progress-breaker-goal-key-backstop-api.md | 276 +++++++++++ ...-progress-breaker-storm-suppression-api.md | 1 + mkdocs.yml | 3 + src/goal_curation/no_progress_breaker.rs | 25 + .../tests_no_progress_breaker.rs | 89 ++++ src/ooda_loop/no_progress.rs | 434 +++++++++++++++++- src/stewardship/dedup.rs | 12 +- 11 files changed, 1213 insertions(+), 21 deletions(-) create mode 100644 docs/concepts/no-progress-breaker-goal-key-backstop.md create mode 100644 docs/howto/configure-no-progress-breaker-open-issue-backstop.md create mode 100644 docs/reference/no-progress-breaker-goal-key-backstop-api.md diff --git a/docs/concepts/no-progress-breaker-goal-key-backstop.md b/docs/concepts/no-progress-breaker-goal-key-backstop.md new file mode 100644 index 000000000..78771a308 --- /dev/null +++ b/docs/concepts/no-progress-breaker-goal-key-backstop.md @@ -0,0 +1,209 @@ +--- +title: The no-progress breaker survives goal-id churn and board resets +description: > + Why the OODA no-progress breaker no longer re-files identical `ooda-stuck` + tracking issues (observed: five identical `UNCLEAR-CRITERIA` issues #4944, + #4946, #4952, #4954, #4958 for one goal in a single day) even when the goal's + transient id churns between cycles or the goal board is reset. Explains the + second-order defect the original storm-suppression fix could not cover — the + durable `WipRef` suppression marker is keyed to the volatile goal id, so id + churn and board-reset both silently defeat it — and the two additive + hardening layers: a stable, injection-safe *folded goal identity* + (`fold_goal_identity`) embedded as an `ooda-goal-key:` marker in + the issue body, and a strongly-consistent *open-issue existence backstop* + (`NoProgressIssueFiler::issue_open_with_marker`) queried only after the + board-local marker misses. Both are additive; a goal with a stable id and an + intact board behaves identically to before. +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: concept +status: implemented +related: + - ./no-progress-breaker-storm-suppression.md + - ./no-progress-root-cause-resolution.md + - ./no-progress-terminal-investigation.md + - ../reference/no-progress-breaker-goal-key-backstop-api.md + - ../reference/no-progress-breaker-storm-suppression-api.md + - ../reference/no-progress-breaker-api.md + - ../howto/configure-no-progress-breaker-open-issue-backstop.md + - ../howto/diagnose-a-no-progress-breaker-issue-storm.md +--- + +# The no-progress breaker survives goal-id churn and board resets + +> **Status: implemented.** The pure `fold_goal_identity` helper and the +> `ooda-goal-key:` body marker live in +> [`src/goal_curation/no_progress_breaker.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/no_progress_breaker.rs); +> the additive `NoProgressIssueFiler::issue_open_with_marker` backstop and its +> wiring into `escalate_with_tracking_issue` live in +> [`src/ooda_loop/no_progress.rs`](https://github.com/rysweet/Simard/blob/main/src/ooda_loop/no_progress.rs). +> For the exact types and functions see the +> [goal-key backstop API reference](../reference/no-progress-breaker-goal-key-backstop-api.md). + +## The residual defect + +The [issue-storm suppression fix](./no-progress-breaker-storm-suppression.md) +closed the first-order loop: it writes a durable `WipRef` suppression marker to +the goal board **before and independent of** the best-effort `gh` link, so a +failed `gh` call can no longer leave a goal "untracked" and re-filing forever. + +That marker, however, is **keyed to the goal's transient id**. Two real +conditions defeat it: + +1. **Goal-id churn.** A goal's `ActiveGoal.id` is not guaranteed stable across + OODA cycles — re-articulation, curation, and re-selection can mint a fresh + id for what is, semantically, the same stuck goal. The suppression marker + travels with the old id; the re-minted goal carries no marker and escalates + again. Each churn produces one more identical issue. +2. **Goal-board reset.** The `WipRef` marker lives on the goal board. A board + reset (a wipe, a schema migration, a restore from an older snapshot, or a + fresh daemon started against an empty board) discards every marker. The next + time the same population stalls, every goal escalates as if for the first + time. + +The observed evidence is a single-goal storm: five identical +`OODA no-progress breaker … (UNCLEAR-CRITERIA)` issues — **#4944, #4946, +#4952, #4954, #4958** — all citing one goal (`a8f57a50`) filed within a single +day. That is not five stuck goals; it is one stuck goal whose board-local +marker was repeatedly bypassed. + +> The first-order fix was necessary but not sufficient. A board-local marker can +> only suppress re-filing while the board (and the id it is keyed to) survives. +> The durable record of "I already filed for this goal" must instead live where +> it cannot churn or be reset: **on the GitHub issue itself.** + +## The two hardening layers + +### Layer 1 — a stable folded goal identity + +`fold_goal_identity` collapses a churny `ActiveGoal.id` into a stable, +injection-safe token: the first 16 hex characters of +`sha256()`. It mirrors the stewardship +[`failure_signature`](../reference/no-progress-breaker-storm-suppression-api.md) +folding contract — same hashing shape, same `[0-9a-f]`-only output — so a +volatile id maps to one deterministic key. + +The breaker embeds that key into the **body** of every issue it constructs, as a +single marker line: + +``` +ooda-goal-key: 9f8c1a2b3c4d5e6f +``` + +The marker is placed in the body, never the title, so the human-facing title +stays clean and stable regardless of the underlying id. Because the token is a +bare hex string it is safe to interpolate into a `gh` `--search` query — see +[Why the key is hashed](#why-the-key-is-hashed) below. + +### Layer 2 — an open-issue existence backstop + +`NoProgressIssueFiler` gains one additive method: + +```rust +fn issue_open_with_marker(&self, marker: &str) -> bool { false } +``` + +The default is `false`, so every existing filer and every test fake compiles +unchanged and behaves exactly as before. The production `GhIssueFiler` +implements it by querying **open** `ooda-stuck` issues for the embedded +`ooda-goal-key:` marker via a direct, argv-vector +`gh issue list --search … in:body` call. It follows the same query *pattern* +the supply-chain steward uses for its own dedup, but is an independent call +scoped to the breaker's own `ooda-stuck` label — it does not reuse the +steward's signature-typed `search_issues` method (that keys on +`stewardship-signature`, not `ooda-goal-key`). + +`escalate_with_tracking_issue` now checks two guards, in cost order: + +```text +goal stalls past threshold → escalate + │ + ▼ +1. board-local WipRef marker present? ── yes ──► suppress (fast path, zero API) + │ no + ▼ +2. open ooda-stuck issue carries this + ooda-goal-key marker? ── yes ──► suppress (backstop, one API call) + │ no + ▼ +file the issue (embedding the marker) + write the WipRef marker +``` + +The backstop runs **only** on a board-local miss, so a steady-state daemon with +an intact board makes **zero** extra API calls — the fast path answers every +already-suppressed goal without touching GitHub. The one search happens only +when the cheap marker is absent, which is exactly the churn / board-reset case +the backstop exists to cover. + +## Why the key is hashed + +`fold_goal_identity` is not only a *stability* choice; it is a *security* +control. The folded key is interpolated into a `gh issue list --search` query. +A raw `ActiveGoal.id` is free-form text; if it contained whitespace, quotes, or +GitHub search qualifiers (`is:`, `label:`, `in:`) it could corrupt the search +and turn the dedup check into a reliability bypass — the query could silently +match nothing and re-enable the storm. Folding to a fixed `[0-9a-f]{16}` token +makes the search argument a constant-charset literal that cannot carry +qualifiers. The `gh` invocation itself stays an **argv vector** +(`Command::new("gh").args([...])`) — never a shell string — so there is no +command-injection surface either. + +Any free-text the breaker copies into the issue body (goal descriptions, error +excerpts) continues to pass through the +[`redact_token` / `redact_uuids`](../reference/no-progress-breaker-storm-suppression-api.md) +helpers, so no secrets or volatile UUIDs leak into a durable public issue. +(These helpers are module-private in `stewardship::dedup` today and must be +widened to `pub(crate)` so `goal_curation` can call them — see the +[API reference implementation prerequisites](../reference/no-progress-breaker-goal-key-backstop-api.md#implementation-prerequisites).) + +## Fail-open by design + +If the backstop's `gh issue list` call fails — a network blip, an auth hiccup, +a rate-limit — `issue_open_with_marker` returns `false` (treat as "no duplicate +found") and filing proceeds. This is the deliberate, documented direction: a +**rare duplicate issue is strictly better than a lost stuck-goal signal.** An +existence-check outage must never abort the OODA cycle or silently swallow a +real stall. The board-local `WipRef` fast path remains the primary, +API-free guarantee; the backstop is the durability layer for the churn/reset +edge, not a replacement for it. + +## What did not change + +- **Thresholds and the resolution ladder.** `NO_PROGRESS_BREAKER_THRESHOLD` and + the MarkDone / Drop / Escalate ladder are untouched. This change gates only + the *file* step, not *when or whether* the breaker fires. +- **The breaker's purity.** `no_progress_breaker.rs` stays hermetic: it only + gained the pure `fold_goal_identity` helper and now embeds the marker string + into the body it constructs. It performs no GitHub I/O. +- **The `WipRef` schema.** No new marker kind; the folded key rides in the issue + body, and the existing board-local suppression marker is unchanged. +- **Clear-criteria goals.** A goal with a stable id and an intact board hits the + fast path on every re-stall exactly as before — no new API cost, no behaviour + change. + +## The `recurring_goal_reblock` half + +The originating brief also cited a stewardship `recurring_goal_reblock` storm +(issues #4945, #4951, #4956, one shared signature `cfa5358a3b59894c`). That +filer's dedup is signature-based via +[`stewardship::failure_signature` / `find_existing`](../reference/no-progress-breaker-storm-suppression-api.md) +and its filing path (`src/overseer/observer.rs`) is **not present on every +branch**. The reblock half therefore follows the identical pattern — +signature-folded key + `find_existing` over open issues — but is gated on +branch reconciliation: where the overseer reblock filer exists it applies the +same backstop; where it does not, the work is tracked as a documented follow-up +rather than fabricated. See +[the how-to](../howto/configure-no-progress-breaker-open-issue-backstop.md#the-recurring_goal_reblock-half) +for reconciliation steps. + +## See also + +- [No-progress breaker issue-storm suppression](./no-progress-breaker-storm-suppression.md) + — the first-order fix this hardens. +- [Goal-key backstop API reference](../reference/no-progress-breaker-goal-key-backstop-api.md) + — exact signatures and contracts. +- [Configure the no-progress breaker open-issue backstop](../howto/configure-no-progress-breaker-open-issue-backstop.md) + — operator configuration, labels, and verification. +- [Diagnose a no-progress breaker issue storm](../howto/diagnose-a-no-progress-breaker-issue-storm.md) + — triage when duplicates are observed. diff --git a/docs/concepts/no-progress-breaker-storm-suppression.md b/docs/concepts/no-progress-breaker-storm-suppression.md index bc40ac58f..c75f671dc 100644 --- a/docs/concepts/no-progress-breaker-storm-suppression.md +++ b/docs/concepts/no-progress-breaker-storm-suppression.md @@ -17,10 +17,12 @@ owner: simard doc_type: concept status: implemented related: + - ./no-progress-breaker-goal-key-backstop.md - ./no-progress-root-cause-resolution.md - ./no-progress-terminal-investigation.md - ./perpetual-goal-no-progress-exemption.md - ./gap-scan-backoff-dedup.md + - ../reference/no-progress-breaker-goal-key-backstop-api.md - ../reference/no-progress-breaker-storm-suppression-api.md - ../reference/no-progress-root-cause-resolution-api.md - ../reference/no-progress-breaker-api.md diff --git a/docs/howto/configure-no-progress-breaker-open-issue-backstop.md b/docs/howto/configure-no-progress-breaker-open-issue-backstop.md new file mode 100644 index 000000000..94ccd30ea --- /dev/null +++ b/docs/howto/configure-no-progress-breaker-open-issue-backstop.md @@ -0,0 +1,180 @@ +--- +title: Configure and verify the no-progress breaker open-issue backstop +description: > + How-to for operators: confirm the OODA no-progress breaker's open-issue + existence backstop is active, understand the `ooda-goal-key:` body + marker and the `ooda-stuck` label it keys on, verify that goal-id churn and + goal-board resets no longer produce duplicate `UNCLEAR-CRITERIA` tracking + issues, tune / observe its (zero steady-state) API cost, and reconcile the + branch-gated `recurring_goal_reblock` half. Prevention-only — this does not + clean up issues already spammed before the fix landed. +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: howto +status: implemented +related: + - ../concepts/no-progress-breaker-goal-key-backstop.md + - ../reference/no-progress-breaker-goal-key-backstop-api.md + - ./diagnose-a-no-progress-breaker-issue-storm.md + - ./configure-overseer-gap-scan-durable-dedup.md + - ./unblock-stuck-ooda-goals.md +--- + +# Configure and verify the no-progress breaker open-issue backstop + +> **Status: implemented.** The backstop is on by default and requires no +> configuration to function. This guide is for operators who want to *confirm* +> it is working, *observe* its behaviour, or *reconcile* the branch-gated +> reblock half. For the mechanism see +> [the concept doc](../concepts/no-progress-breaker-goal-key-backstop.md); for +> exact signatures see the +> [API reference](../reference/no-progress-breaker-goal-key-backstop-api.md). + +## When you need this + +Use this guide when you see (or want to prevent) **repeated identical** +`ooda-stuck` tracking issues for a single stuck goal — for example the observed +storm of `#4944, #4946, #4952, #4954, #4958`, all citing goal `a8f57a50`. If the +board-local suppression marker were doing its job you would see **one** issue; +several identical issues mean the marker was bypassed by **goal-id churn** or a +**goal-board reset**, which is exactly what the open-issue backstop covers. + +## Prerequisites + +- `gh` CLI installed and authenticated with **repo scope** on the target + repository (the daemon already requires this to file issues at all). +- The daemon running a build that includes the backstop (see + [Confirm it is active](#confirm-it-is-active)). + +No environment variables, flags, or config-file keys are required — the backstop +is unconditional and additive. + +## Confirm it is active + +The backstop leaves two observable fingerprints. + +### 1. The body marker on newly filed issues + +Every issue the breaker files now ends with an `ooda-goal-key` line: + +```bash +gh issue view --json body -q .body | tail -1 +# ooda-goal-key: 9f8c1a2b3c4d5e6f +``` + +A newly filed `ooda-stuck` issue **without** this line was filed by a +pre-backstop build. + +### 2. The fast-path / backstop trace events + +The gate emits structured `tracing` events (target `simard::ooda`). Follow the +daemon log: + +```bash +journalctl -u simard --since "10 min ago" \ + | grep -E "no-progress breaker|ooda-goal-key" +``` + +You should see one of: + +- **Fast path (steady state):** an "already suppressed (board marker)" event and + **no** `gh issue list` call — the common case, zero API cost. +- **Backstop hit (churn / reset):** an "open duplicate found by ooda-goal-key, + suppressing + re-seeding marker" event, following a single `gh issue list` + search. +- **Filed:** the existing "tracking issue filed for stuck goal" warning, now + with an `ooda-goal-key` marker in the body. + +## How the labels and marker fit together + +| Element | Value | Role | +|---------|-------|------| +| Label | `ooda-stuck` | Scopes the backstop search to breaker-filed issues only. | +| Body marker | `ooda-goal-key: <16-hex>` | Stable identity of the stuck goal, immune to id churn. | +| Search | `gh issue list --state open --label ooda-stuck --search "ooda-goal-key: in:body"` | The existence query. | + +The 16-hex key is `sha256(goal_id)[..16]` (see +[`fold_goal_identity`](../reference/no-progress-breaker-goal-key-backstop-api.md#fold_goal_identity)). +You do **not** set it; it is derived. To compute the key for a given goal id for +your own verification: + +```bash +printf '%s' '' | sha256sum | cut -c1-16 +``` + +## Verify duplicate suppression end to end + +Simulate the churn/reset case against a **test** repo (never production): + +1. File a breaker-style issue carrying a known marker: + ```bash + KEY=$(printf '%s' 'demo-goal' | sha256sum | cut -c1-16) + gh issue create -R / \ + --label ooda-stuck \ + --title "OODA no-progress breaker: goal stuck (UNCLEAR-CRITERIA)" \ + --body "verification fixture + + ooda-goal-key: $KEY" + ``` +2. Confirm the backstop query finds it: + ```bash + gh issue list -R / --state open --label ooda-stuck \ + --search "ooda-goal-key:$KEY in:body" --json number,title + ``` + A non-empty result means a re-stall of `demo-goal` (even with a churned id or + a wiped board) will be **suppressed**, not re-filed. +3. Close the issue and re-run the search — an empty result confirms **closed + issues do not suppress** (OPEN-only scope), so a genuinely re-opened stall + re-files as intended. + +## Observe and reason about API cost + +- **Steady state: zero extra calls.** The backstop query fires **only** when the + board-local `WipRef` fast path misses. An intact board with stable goal ids + never reaches the search. +- **Churn / reset: one call per re-escalation** of an affected goal, until the + re-seeded `WipRef` marker restores the fast path. +- If you observe frequent backstop hits in steady state, that is a *signal of + underlying id churn or repeated board resets* — investigate those causes; the + backstop is correctly masking their symptom but they are worth fixing at the + source. + +## Fail-open behaviour (do not "fix" it) + +If `gh issue list` fails (network, auth, rate-limit), the backstop returns "no +duplicate found" and filing proceeds. This is intentional: **a rare duplicate +issue is preferable to a lost stuck-goal signal.** Do not add a retry loop that +could block the OODA cycle, and do not change the direction to fail-closed — +that would let a transient `gh` outage silently swallow real stalls. + +## The `recurring_goal_reblock` half + +The overseer's `recurring_goal_reblock` filer +(`src/overseer/observer.rs`) is **branch-gated**: it is absent on some branches +(including the docs/verify branch where this feature was specified). + +- **If your build includes the overseer reblock filer:** it uses the same + pattern keyed on `stewardship::failure_signature` and deduped via + `stewardship::find_existing` over open issues (matching + `stewardship-signature: ` in the body). Verify it the same way as above, + searching for the `stewardship-signature:` marker instead of `ooda-goal-key:`. +- **If your build does not include it:** the reblock storm (e.g. `#4945, #4951, + #4956`, signature `cfa5358a3b59894c`) is tracked as a documented follow-up. + Do not attempt to configure a filer that is not compiled into your daemon; + reconcile the branch first, then apply the identical backstop. + +## Scope and limitations + +- **Prevention only.** The backstop stops *new* duplicates. Issues already + spammed before the fix landed are not auto-closed — triage them with + [Diagnose a no-progress breaker issue storm](./diagnose-a-no-progress-breaker-issue-storm.md). +- **Thresholds unchanged.** This does not alter `NO_PROGRESS_BREAKER_THRESHOLD` + or *when* the breaker fires — only whether the *file* step is suppressed. +- **OPEN issues only.** A closed duplicate does not suppress a genuine re-stall. + +## See also + +- [Concept: the breaker survives goal-id churn and board resets](../concepts/no-progress-breaker-goal-key-backstop.md) +- [Goal-key backstop API reference](../reference/no-progress-breaker-goal-key-backstop-api.md) +- [Diagnose a no-progress breaker issue storm](./diagnose-a-no-progress-breaker-issue-storm.md) diff --git a/docs/howto/diagnose-a-no-progress-breaker-issue-storm.md b/docs/howto/diagnose-a-no-progress-breaker-issue-storm.md index 3f33d32b8..bf4a60e9d 100644 --- a/docs/howto/diagnose-a-no-progress-breaker-issue-storm.md +++ b/docs/howto/diagnose-a-no-progress-breaker-issue-storm.md @@ -12,10 +12,13 @@ review_schedule: as-needed owner: simard doc_type: howto related: + - ../reference/no-progress-breaker-goal-key-backstop-api.md - ../reference/no-progress-breaker-storm-suppression-api.md - ../reference/no-progress-root-cause-resolution-api.md - ../reference/no-progress-breaker-api.md + - ../concepts/no-progress-breaker-goal-key-backstop.md - ../concepts/no-progress-breaker-storm-suppression.md + - ./configure-no-progress-breaker-open-issue-backstop.md - ./unblock-stuck-ooda-goals.md - ./diagnose-a-no-progress-block.md - ./reinvestigate-bare-blocked-goals.md diff --git a/docs/reference/no-progress-breaker-goal-key-backstop-api.md b/docs/reference/no-progress-breaker-goal-key-backstop-api.md new file mode 100644 index 000000000..a6d3cf791 --- /dev/null +++ b/docs/reference/no-progress-breaker-goal-key-backstop-api.md @@ -0,0 +1,276 @@ +--- +title: No-progress breaker goal-key backstop API reference +description: > + Reference for the additive hardening that makes the OODA no-progress breaker's + issue-storm suppression survive goal-id churn and goal-board resets. Specifies + the pure `fold_goal_identity` helper (stable, injection-safe + `sha256[..16]` folding of a churny `ActiveGoal.id`), the `ooda-goal-key:` + body marker the breaker embeds, the additive + `NoProgressIssueFiler::issue_open_with_marker` default method (open-issue + existence backstop), its production `GhIssueFiler` implementation over + `gh issue list --search … in:body`, and the two-guard filing gate in + `escalate_with_tracking_issue` (board-local `WipRef` fast path first, backstop + only on miss). All items are additive and non-breaking. +last_updated: 2026-07-28 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ./no-progress-breaker-storm-suppression-api.md + - ./no-progress-breaker-api.md + - ./no-progress-root-cause-resolution-api.md + - ./goal-board-api.md + - ../concepts/no-progress-breaker-goal-key-backstop.md + - ../concepts/no-progress-breaker-storm-suppression.md + - ../howto/configure-no-progress-breaker-open-issue-backstop.md + - ../../src/goal_curation/no_progress_breaker.rs + - ../../src/ooda_loop/no_progress.rs +--- + +# No-progress breaker goal-key backstop API reference + +> **Status: implemented.** `fold_goal_identity` and the `ooda-goal-key` body +> marker live in +> [`src/goal_curation/no_progress_breaker.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/no_progress_breaker.rs); +> the `NoProgressIssueFiler::issue_open_with_marker` method, its `GhIssueFiler` +> implementation, and the two-guard gate in `escalate_with_tracking_issue` live +> in +> [`src/ooda_loop/no_progress.rs`](https://github.com/rysweet/Simard/blob/main/src/ooda_loop/no_progress.rs). +> The rustdoc on those items is the canonical API; the signatures below are kept +> in sync with it. + +This page specifies the **additive, non-breaking** hardening layered on the +[issue-storm suppression](./no-progress-breaker-storm-suppression-api.md) fix. +It adds a second, GitHub-durable suppression guard that survives the two +conditions the board-local `WipRef` marker cannot cover: **goal-id churn** and +**goal-board reset**. For the rationale, see +[the concept doc](../concepts/no-progress-breaker-goal-key-backstop.md). + +## Constants + +### `OODA_GOAL_KEY_MARKER_PREFIX` + +```rust +const OODA_GOAL_KEY_MARKER_PREFIX: &str = "ooda-goal-key:"; +``` + +The body-marker prefix the breaker embeds and the backstop matches on. A +constant — never re-typed at a call site. The full marker line is +`{OODA_GOAL_KEY_MARKER_PREFIX} {folded_id}` (one ASCII space), placed in the +issue **body**, never the title. + +## `fold_goal_identity` + +```rust +/// Fold a churny `ActiveGoal.id` into a stable, injection-safe identity token: +/// the first 16 lowercase hex characters of `sha256(goal_id)`. +/// +/// Pure and total. Mirrors the `stewardship::failure_signature` folding shape so +/// a volatile id collapses to one deterministic `[0-9a-f]{16}` key. Because the +/// output is a fixed-charset hex literal it is safe to interpolate into a `gh` +/// `--search` query argument — it cannot carry whitespace, quotes, or GitHub +/// search qualifiers (`is:`, `label:`, `in:`) that would corrupt the dedup check. +pub(crate) fn fold_goal_identity(goal_id: &str) -> String; +``` + +- **Input:** any `ActiveGoal.id` (free-form UTF-8). +- **Output:** exactly 16 lowercase hex characters. +- **Determinism:** `fold_goal_identity(x) == fold_goal_identity(x)` always; + distinct ids collide only at the `sha256[..8]`-byte level (negligible). +- **Location:** `src/goal_curation/no_progress_breaker.rs`. Keeps the breaker + **pure** — no I/O; it only produces the token and embeds the marker string + into the body it constructs. + +### Marker embedding + +The breaker's escalation body builder appends the marker line to the body it +already constructs. Illustrative shape (the surrounding body text is unchanged): + +```text + + +ooda-goal-key: 9f8c1a2b3c4d5e6f +``` + +The marker is the last body line and carries no secrets: it is a pure hash of +the id. All other free-text copied into the body continues to pass through the +`redact_token` / `redact_uuids` helpers (see +[Implementation prerequisites](#implementation-prerequisites) — these helpers +are module-private today and must be promoted to `pub(crate)` before +`goal_curation` can call them). + +## `NoProgressIssueFiler::issue_open_with_marker` + +```rust +pub(crate) trait NoProgressIssueFiler { + fn file_issue(&self, title: &str, body: &str) -> Option; + + /// Existence backstop: is there an OPEN `ooda-stuck` issue whose body carries + /// `marker` (an `ooda-goal-key:` line)? Additive with a default of + /// `false` so every existing impl and test fake is unchanged. + /// + /// Called ONLY after the board-local `WipRef` fast path misses, so a + /// steady-state daemon with an intact board makes zero extra API calls. + /// + /// Fail-open: a query error MUST return `false` ("no duplicate found") and + /// let filing proceed — a rare duplicate is strictly better than a lost + /// stuck-goal signal, and the check must never abort the OODA cycle. + fn issue_open_with_marker(&self, _marker: &str) -> bool { + false + } +} +``` + +- **Default `false`** makes the method purely additive: `CountingFiler`, the + test fakes, and any third impl compile and behave exactly as before, taking + the "no duplicate" path. +- **`marker`** is the full `ooda-goal-key:` line (or just the folded + key) the caller derives from the stalled goal via `fold_goal_identity`. +- **State scope:** OPEN issues only. A closed issue does not suppress — a + re-opened stall should re-file, consistent with the storm-suppression + `find_existing` semantics. + +### Production implementation (`GhIssueFiler`) + +```rust +impl NoProgressIssueFiler for GhIssueFiler { + fn issue_open_with_marker(&self, marker: &str) -> bool { + // Independent, direct `gh issue list` existence query scoped to the + // breaker's own `ooda-stuck` label and `ooda-goal-key` body marker. + // This does NOT call the signature-typed `search_issues` trait method + // (that lives in `supply_chain_steward` and keys on + // `stewardship-signature`, not `ooda-goal-key`); it only follows the + // same argv-vector, `--search … in:body` invocation pattern. Argv only + // — never a shell string. + match std::process::Command::new("gh") + .args([ + "issue", "list", + "--state", "open", + "--label", "ooda-stuck", + "--search", &format!("{marker} in:body"), + "--json", "number,body", + ]) + .output() + { + Ok(out) if out.status.success() => body_contains_marker(&out.stdout, marker), + _ => false, // fail-open + } + } +} +``` + +- **Search index vs. strong match.** GitHub's `--search` index is eventually + consistent, so the implementation confirms the marker against the returned + JSON `body` (a strongly-consistent `gh issue list` field) rather than trusting + the search match alone — a freshly filed issue that has not yet indexed still + dedups correctly on the next cycle. +- **Argv only (`SR2`).** The invocation is `Command::new("gh").args([...])`; + no `sh -c`, no string-interpolated command line. +- **Least privilege (`SR4`).** Repo-scoped `gh`; the implementation never reads, + logs, or embeds `GH_TOKEN`. An auth failure fails loud (logged via `tracing`) + but fails **open** for dedup and never aborts the cycle. + +## Implementation prerequisites + +Two small source reconciliations are required before this feature compiles as +specified. Both are captured here so the implementation order reflects reality +rather than an idealized reuse. + +1. **Promote the redaction helpers to `pub(crate)`.** `redact_token` and + `redact_uuids` are **module-private** functions in + [`src/stewardship/dedup.rs`](https://github.com/rysweet/Simard/blob/main/src/stewardship/dedup.rs) + (only `normalize`, `failure_signature`, and `find_existing` are `pub`). + Because the breaker lives in `goal_curation`, reusing them for body + redaction (SR3/SR6) requires changing their visibility from `fn` to + `pub(crate) fn`. This is a non-breaking visibility widening — no signature or + behaviour change. This step must precede wiring redaction into the breaker's + body builder. + +2. **Use a direct `gh` call, not the `search_issues` trait method.** The + existence backstop for the `ooda-goal-key` half is a direct + `Command::new("gh").args([…])` invocation scoped to `--label ooda-stuck` + (shown above). It deliberately does **not** reuse + `supply_chain_steward`'s `search_issues(&self, signature: &str)` trait + method: that method is signature-typed and matches + `stewardship-signature: ` bodies, not `ooda-goal-key:` + bodies. Only the branch-gated `recurring_goal_reblock` half (keyed on + `stewardship::failure_signature`) reuses `search_issues` / + `find_existing`; the OODA breaker half uses its own direct query. + +## `escalate_with_tracking_issue` — the two-guard gate + +The escalation side effect now evaluates two suppression guards in strict cost +order before filing. The `Blocked` status is still written first (unchanged from +the storm-suppression fix); only the *file* step is gated. + +```text +1. board-local WipRef suppression marker present (is_breaker_tracking_ref)? + └─ yes → already suppressed; return (FAST PATH, zero API calls) +2. filer.issue_open_with_marker("ooda-goal-key:")? + └─ yes → an open duplicate already exists; write the board-local WipRef + marker (re-seed the fast path) and return — do NOT file +3. otherwise → file_issue(title, body-with-marker); write the WipRef marker; + upgrade to a linked tracking ref on success +``` + +Guarantees: + +- **Zero steady-state API cost (`SR7`).** Guard 2 runs only on a guard-1 miss. + An intact board answers every already-suppressed goal at guard 1. +- **Churn / reset coverage.** When id churn or a board reset erases the + `WipRef`, guard 2 still finds the open issue by its durable body marker and + suppresses — then re-seeds the board-local marker so future cycles hit the + fast path again. +- **Additive.** Removing guard 2 (or a filer returning the `false` default) + degrades exactly to the prior storm-suppression behaviour — no regression for + stable-id, intact-board goals. + +## Test contract + +Inline `#[cfg(test)]` modules in both files cover: + +| # | Case | Asserts | +|---|------|---------| +| 1 | no-progress, first filing | Guard 1 & 2 both miss → `file_issue` called once; body carries the `ooda-goal-key` marker | +| 2 | no-progress, duplicate suppressed across id churn / board reset | Guard 1 misses (no `WipRef`), guard 2 hits (`issue_open_with_marker == true`) → `file_issue` NOT called; `WipRef` re-seeded | +| 3 | fast-path, no API call | Guard 1 hits → `issue_open_with_marker` never invoked (counting fake records zero calls) | +| 4 | injection-charset id | `fold_goal_identity` on an id containing spaces / quotes / `is:` / `label:` yields a pure `[0-9a-f]{16}` token | +| 5 | token redaction in body | secrets / UUIDs in copied free-text are redacted before the marker line | +| 6 | fail-open | `issue_open_with_marker` returns `false` on a `gh` error and filing proceeds | + +Reblock half (branch-gated, see below) mirrors cases 1–2 keyed on +`stewardship::failure_signature` via `find_existing`. + +## The `recurring_goal_reblock` half (branch-gated) + +The overseer `recurring_goal_reblock` filer (`src/overseer/observer.rs`) is not +present on every branch. Where present, it applies the identical pattern using +the existing stewardship primitives: + +- **Key:** `stewardship::failure_signature(failure_kind, error_text)` — the same + 16-hex signature used across steward dedup. +- **Backstop:** `stewardship::find_existing(&open_issues, &signature)` over + `SupplyChainGh::search_issues`, which already matches + `stewardship-signature: ` in the issue body. + +Where the filer is absent on the target branch, this is tracked as a documented +follow-up (see the how-to) rather than implemented against a phantom symbol. + +## Security summary + +| ID | Control | +|----|---------| +| SR1 | Free-form goal id is hashed to `[0-9a-f]{16}` **before** entering a `--search` query — prevents query-injection-as-reliability-bypass. | +| SR2 | `gh` invoked as an argv vector; no `sh -c` / string-interpolated command lines. | +| SR3 | No secrets/PII in title or body; body free-text passes through `redact_token` / `redact_uuids` (requires promoting both to `pub(crate)` — see [Implementation prerequisites](#implementation-prerequisites)); never log the full body at `info`. | +| SR4 | Least privilege; never read/print/embed `GH_TOKEN`; auth failure fails loud but never aborts the cycle. | +| SR5 | Existence-check errors fail **open** (duplicate < lost signal), by design and documented. | +| SR6 | No new dependencies — reuse `sha2` and `serde_json`, and follow the existing argv `gh` invocation *pattern* (a direct call, not the `search_issues` trait method — see [Implementation prerequisites](#implementation-prerequisites)). | +| SR7 | Backstop API call only after the `WipRef` fast-path miss → zero steady-state API cost; avoids secondary-rate-limit self-DoS. | + +## See also + +- [Concept: the breaker survives goal-id churn and board resets](../concepts/no-progress-breaker-goal-key-backstop.md) +- [Issue-storm suppression API reference](./no-progress-breaker-storm-suppression-api.md) +- [Configure the open-issue backstop](../howto/configure-no-progress-breaker-open-issue-backstop.md) diff --git a/docs/reference/no-progress-breaker-storm-suppression-api.md b/docs/reference/no-progress-breaker-storm-suppression-api.md index 31198d2bf..07365b671 100644 --- a/docs/reference/no-progress-breaker-storm-suppression-api.md +++ b/docs/reference/no-progress-breaker-storm-suppression-api.md @@ -18,6 +18,7 @@ owner: simard doc_type: reference status: implemented related: + - ./no-progress-breaker-goal-key-backstop-api.md - ./no-progress-breaker-api.md - ./no-progress-root-cause-resolution-api.md - ./no-progress-reinvestigation-api.md diff --git a/mkdocs.yml b/mkdocs.yml index ada78cf61..ae12e74bf 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -147,6 +147,7 @@ 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 + - No-Progress Breaker Survives Goal-ID Churn and Board Resets: concepts/no-progress-breaker-goal-key-backstop.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 +202,7 @@ 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 + - Configure the No-Progress Breaker Open-Issue Backstop: howto/configure-no-progress-breaker-open-issue-backstop.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 +459,7 @@ 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 + - No-Progress Breaker Goal-Key Backstop API: reference/no-progress-breaker-goal-key-backstop-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/no_progress_breaker.rs b/src/goal_curation/no_progress_breaker.rs index 20fc64ff3..a4f9b29f7 100644 --- a/src/goal_curation/no_progress_breaker.rs +++ b/src/goal_curation/no_progress_breaker.rs @@ -74,6 +74,31 @@ pub const NO_PROGRESS_BLOCKED_PREFIX: &str = /// `{PREFIX}{count}{SUFFIX}`. pub const NO_PROGRESS_BLOCKED_SUFFIX: &str = " consecutive no-action cycles; needs human review"; +/// Fold a churny [`ActiveGoal.id`](super::types::ActiveGoal) into a stable, +/// injection-safe identity token: the first 16 lowercase hex characters of +/// `sha256(goal_id)` (the first 8 digest bytes). +/// +/// Pure and total. Mirrors the folding shape of +/// [`crate::stewardship::failure_signature`] so a volatile id collapses to one +/// deterministic `[0-9a-f]{16}` key that two OODA cycles on the same goal share. +/// +/// Because the output is a fixed-charset hex literal it is safe to interpolate +/// into a `gh --search` query argument (SR1): it can never carry whitespace, +/// quotes, or GitHub search qualifiers (`is:`, `label:`, `in:`) that a raw, +/// attacker-influenced goal id could smuggle into the open-issue backstop's +/// dedup check. It is a one-way hash, not an encoding: no fragment of the raw id +/// survives into the folded key. +pub(crate) fn fold_goal_identity(goal_id: &str) -> String { + use sha2::{Digest, Sha256}; + + let digest = Sha256::digest(goal_id.as_bytes()); + let mut out = String::with_capacity(16); + for b in &digest[..8] { + out.push_str(&format!("{b:02x}")); + } + out +} + /// True when `reason` was authored by the no-progress breaker. /// /// Keys on the globally-unique [`NO_PROGRESS_BLOCKED_PREFIX`] sentinel **alone** diff --git a/src/goal_curation/tests_no_progress_breaker.rs b/src/goal_curation/tests_no_progress_breaker.rs index b68d5035f..9638c8a29 100644 --- a/src/goal_curation/tests_no_progress_breaker.rs +++ b/src/goal_curation/tests_no_progress_breaker.rs @@ -332,3 +332,92 @@ fn four_stuck_supply_chain_goals_all_leave_the_active_loop_via_the_ladder() { } } } + +/// # TDD (Step 7) — goal-key backstop: pure `fold_goal_identity` contract +/// +/// FAILING BY DESIGN until `fold_goal_identity` (and its `pub(crate)` +/// visibility) exists in `super::no_progress_breaker`, and the redaction +/// helpers in `crate::stewardship::dedup` are promoted to `pub(crate)`. +/// +/// Specifies the pure, injection-safe identity-folding helper documented in +/// `docs/reference/no-progress-breaker-goal-key-backstop-api.md` — the stable +/// key that lets the open-issue backstop dedup across goal-id churn / board +/// reset without ever letting a churny, attacker-influenced goal id leak into a +/// `gh --search` query (SR1). +#[cfg(test)] +mod tests_goal_key_backstop { + use super::super::no_progress_breaker::fold_goal_identity; + + /// A goal id carrying every character class that would corrupt a `gh + /// --search` query if interpolated raw: whitespace, quotes, and GitHub + /// search qualifiers (`is:`, `label:`, `in:body`). + const ADVERSARIAL_ID: &str = + "simard-identity-\"drop\" is:open label:ooda-stuck in:body a8f57a50 --json"; + + fn is_lower_hex_16(s: &str) -> bool { + s.len() == 16 + && s.bytes() + .all(|b| b.is_ascii_digit() || (b'a'..=b'f').contains(&b)) + } + + /// Case 4 (charset): the folded key is ALWAYS exactly 16 lowercase hex + /// chars, even for an adversarial id — so it can be interpolated into a + /// `--search` argument without carrying spaces / quotes / qualifiers. + #[test] + fn folded_key_is_pure_16_lower_hex_even_for_adversarial_id() { + assert!( + is_lower_hex_16(&fold_goal_identity(ADVERSARIAL_ID)), + "an adversarial goal id must fold to a pure [0-9a-f]{{16}} token (SR1)" + ); + assert!(is_lower_hex_16(&fold_goal_identity("g"))); + assert!(is_lower_hex_16(&fold_goal_identity(""))); + assert!(is_lower_hex_16(&fold_goal_identity( + "simard-identity-atelier-industrial-furniture-de" + ))); + } + + /// Determinism: the same id always folds to the same key (this is what lets + /// two OODA cycles on the same goal dedup to one open issue). + #[test] + fn fold_is_deterministic() { + assert_eq!( + fold_goal_identity(ADVERSARIAL_ID), + fold_goal_identity(ADVERSARIAL_ID), + ); + } + + /// Distinct ids fold to distinct keys (no accidental over-collapse that + /// would suppress a genuinely different stuck goal). + #[test] + fn distinct_ids_fold_to_distinct_keys() { + assert_ne!(fold_goal_identity("goal-a"), fold_goal_identity("goal-b")); + } + + /// SR1/SR3: the raw id (and any secret-shaped substring inside it) must NOT + /// survive into the folded key — it is a one-way hash, not an encoding. + #[test] + fn raw_id_never_leaks_into_the_folded_key() { + let folded = fold_goal_identity(ADVERSARIAL_ID); + for needle in ["is:", "label:", "in:body", "\"", " ", "--json", "a8f57a50"] { + assert!( + !folded.contains(needle), + "folded key must not echo id fragment {needle:?}" + ); + } + } + + /// Implementation prerequisite #1: the stewardship redaction helpers must be + /// promoted from module-private `fn` to `pub(crate) fn` so the breaker (in + /// `goal_curation`) can redact goal-derived free-text before embedding it in + /// an escalation body (SR3/SR6). This test only COMPILES once that + /// visibility widening lands. + #[test] + fn redaction_helpers_are_reachable_from_goal_curation() { + let uuid = "0191b2c3-4d5e-7f80-9abc-def012345678"; + assert_eq!(crate::stewardship::dedup::redact_token(uuid), ""); + assert_eq!( + crate::stewardship::dedup::redact_uuids(&format!("session={uuid}")), + "session=", + ); + } +} diff --git a/src/ooda_loop/no_progress.rs b/src/ooda_loop/no_progress.rs index fd3968f6d..73642387f 100644 --- a/src/ooda_loop/no_progress.rs +++ b/src/ooda_loop/no_progress.rs @@ -21,6 +21,7 @@ use crate::error::SimardResult; use crate::goal_curation::completion_gate::{ CompletionEvidenceGate, CompletionVerdict, DependencyState, EvidenceSource, }; +use crate::goal_curation::no_progress_breaker::fold_goal_identity; use crate::goal_curation::no_progress_breaker::{ NO_PROGRESS_BREAKER_THRESHOLD, NoProgressResolution, NoProgressTracker, SURFACED_INVESTIGATION_FAILURE_LIMIT, needs_reinvestigation, @@ -81,6 +82,19 @@ const NO_PROGRESS_SUPPRESSION_MARKER_KIND: &str = "ooda-breaker-marker"; /// the marker (no argv/flag injection, no path traversal). const NO_PROGRESS_SUPPRESSION_MARKER_REF_ID: &str = "ooda-breaker"; +/// Body-marker prefix the breaker embeds in a filed tracking issue and the +/// open-issue backstop ([`NoProgressIssueFiler::issue_open_with_marker`]) +/// matches on. The full marker line is `{PREFIX} {folded_id}` (one ASCII space, +/// where `folded_id = fold_goal_identity(goal.id)`), placed in the issue +/// **body**, never the title. A constant — never re-typed at a call site. +/// +/// This is the GitHub-durable identity key that lets suppression survive the two +/// conditions the board-local [`WipRef`] marker cannot cover: **goal-id churn** +/// (a new id each cycle defeats the `WipRef`) and a **goal-board reset** (which +/// erases the `WipRef` entirely). The key is a pure hash of the id, so it leaks +/// no secrets and is safe to embed and to interpolate into a `gh --search`. +const OODA_GOAL_KEY_MARKER_PREFIX: &str = "ooda-goal-key:"; + /// True when `wip` is a breaker-authored escalation artifact — EITHER the durable /// suppression marker ([`NO_PROGRESS_SUPPRESSION_MARKER_KIND`], written /// before/independent of linking) OR the upgraded linked tracking issue @@ -157,6 +171,26 @@ pub(crate) trait NoProgressIssueFiler { /// `None` simply means the goal stays Blocked without a linked artifact /// (no worse than before this linkage existed). fn file_issue(&self, title: &str, body: &str) -> Option; + + /// Existence backstop: is there an OPEN `ooda-stuck` issue whose body carries + /// `marker` (an `ooda-goal-key:` line)? Additive with a default of + /// `false` so every existing impl and test fake is unchanged and takes the + /// "no duplicate found" path. + /// + /// Called ONLY after the board-local [`WipRef`] fast path misses (see + /// [`escalate_with_tracking_issue`]), so a steady-state daemon with an intact + /// board makes zero extra API calls (SR7). + /// + /// **Fail-open (SR5):** a query error MUST return `false` ("no duplicate + /// found") and let filing proceed — a rare duplicate is strictly better than + /// a lost stuck-goal signal, and the check must never abort the OODA cycle. + /// + /// **Scope:** OPEN issues only. A closed issue does not suppress — a reopened + /// stall should re-file, consistent with the storm-suppression + /// `find_existing` semantics. + fn issue_open_with_marker(&self, _marker: &str) -> bool { + false + } } /// Production filer: `gh issue create --label ooda-stuck`, mirroring the @@ -212,6 +246,86 @@ impl NoProgressIssueFiler for GhIssueFiler { } } } + + /// Open-issue backstop over `gh issue list`, scoped to the breaker's own + /// `ooda-stuck` label and the `ooda-goal-key` body marker. + /// + /// This is a direct, independent existence query — it deliberately does NOT + /// call the signature-typed `search_issues` trait method used by + /// `supply_chain_steward` (that keys on `stewardship-signature`, not + /// `ooda-goal-key`); it only follows the same argv-vector, + /// `--search … in:body` invocation pattern. + /// + /// - **Argv only (SR2):** `Command::new("gh").args([…])`; never `sh -c` or a + /// string-interpolated command line. `marker` is already a pure + /// `[0-9a-f]{16}`-keyed token ([`fold_goal_identity`]), so it cannot inject + /// qualifiers into the `--search` argument (SR1). + /// - **Strong match, not index trust:** GitHub's `--search` index is + /// eventually consistent, so the verdict is confirmed against the returned + /// JSON `body` (a strongly-consistent field) rather than the search match + /// alone — a freshly filed issue that has not yet indexed still dedups on + /// the next cycle once returned by the label filter. + /// - **Least privilege / fail-open (SR4/SR5):** repo-scoped `gh`; never + /// reads/logs/embeds `GH_TOKEN`; any error fails **open** (`false`) so the + /// check never aborts the cycle. + fn issue_open_with_marker(&self, marker: &str) -> bool { + match std::process::Command::new("gh") + .args([ + "issue", + "list", + "--state", + "open", + "--label", + "ooda-stuck", + "--search", + &format!("{marker} in:body"), + "--json", + "number,body", + ]) + .output() + { + Ok(out) if out.status.success() => body_contains_marker(&out.stdout, marker), + Ok(out) => { + tracing::warn!( + target: "simard::ooda", + stderr = %String::from_utf8_lossy(&out.stderr), + "no-progress breaker: gh open-issue backstop query failed — failing OPEN (may re-file)", + ); + false + } + Err(e) => { + tracing::warn!( + target: "simard::ooda", + error = %e, + "no-progress breaker: gh spawn failed for open-issue backstop — failing OPEN (may re-file)", + ); + false + } + } + } +} + +/// Confirm the `gh issue list --json number,body` output contains an OPEN issue +/// whose `body` embeds `marker`. Parsing the strongly-consistent `body` field +/// (rather than trusting the eventually-consistent `--search` match) is what +/// makes the backstop robust to a freshly filed, not-yet-indexed issue. Any +/// parse failure fails **open** (`false`, SR5) — a rare duplicate beats a lost +/// stuck-goal signal. +fn body_contains_marker(stdout: &[u8], marker: &str) -> bool { + let Ok(value) = serde_json::from_slice::(stdout) else { + return false; + }; + value + .as_array() + .map(|issues| { + issues.iter().any(|issue| { + issue + .get("body") + .and_then(|b| b.as_str()) + .is_some_and(|body| body.contains(marker)) + }) + }) + .unwrap_or(false) } /// Parse the issue number from a `gh issue create` success line, which prints @@ -251,15 +365,40 @@ fn link_tracking_issue(goal: &mut ActiveGoal, filed: &FiledIssue) { /// re-filed, then best-effort file + link a `gh` tracking issue. Storm-safe and /// restart-surviving. /// -/// Ordering (the storm fix): the durable suppression marker and the `Blocked` -/// status are written FIRST, through the existing atomic goal-board save path, so -/// the goal is idempotently suppressed BEFORE [`NoProgressIssueFiler::file_issue`] -/// is attempted. A `None` from `file_issue` (a `gh` outage, or a URL that did not -/// parse to a bare issue number) therefore leaves the goal `Blocked` + suppressed -/// (no re-file next cycle) instead of `Blocked` + untracked (re-file forever) — -/// the exact loop that produced the ~15-duplicate `UNCLEAR-CRITERIA` issue storm. -/// On a `Some`, the bare marker is UPGRADED IN PLACE to the linked tracking ref -/// via [`upgrade_suppression_marker_to_link`] — never appended as a duplicate. +/// # The two-guard filing gate +/// +/// The `Blocked` status is written first (unchanged from the storm fix); only +/// the *file* step is gated, in strict cost order: +/// +/// ```text +/// 1. board-local WipRef marker present (is_breaker_tracking_ref)? +/// └─ yes → already suppressed; return (FAST PATH, zero API calls) +/// 2. filer.issue_open_with_marker("ooda-goal-key:")? +/// └─ yes → an open duplicate already exists; re-seed the WipRef marker +/// and return — do NOT file +/// 3. otherwise → file_issue(title, body-with-marker); write the WipRef marker; +/// upgrade to a linked tracking ref on success +/// ``` +/// +/// Guard 1 is the durable, restart-surviving board marker that ended the +/// original ~15-duplicate `UNCLEAR-CRITERIA` storm. Guard 2 is the +/// GitHub-durable open-issue backstop ([`NoProgressIssueFiler::issue_open_with_marker`]) +/// that extends suppression across the two conditions the `WipRef` cannot cover +/// — **goal-id churn** (a fresh id has no `WipRef`) and a **goal-board reset** +/// (the `WipRef` was erased) — then re-seeds the board marker so future cycles +/// answer at guard 1. It runs ONLY on a guard-1 miss (zero steady-state API +/// cost, SR7) and fails **open** (SR5): a query error re-files rather than +/// dropping a stuck-goal signal. +/// +/// A `None` from `file_issue` (a `gh` outage, or a URL that did not parse to a +/// bare issue number) leaves the goal `Blocked` + suppressed (no re-file next +/// cycle) instead of `Blocked` + untracked (re-file forever). On a `Some`, the +/// bare marker is UPGRADED IN PLACE to the linked tracking ref via +/// [`upgrade_suppression_marker_to_link`] — never appended as a duplicate. +/// +/// The filed body embeds a trailing `ooda-goal-key: ` line +/// ([`fold_goal_identity`]) — a pure hash carrying no secrets — so guard 2 can +/// recognise the breaker's own open issue on a later churned/reset cycle. /// /// Linking the issue is what makes an `UNCLEAR-CRITERIA` goal's done-criteria /// measurable: with the link the done-gate can observe the tracking issue as @@ -279,6 +418,15 @@ fn escalate_with_tracking_issue( issue_body: &str, filer: &dyn NoProgressIssueFiler, ) { + // The GitHub-durable identity key for this goal. Computed once, up front, so + // both suppression guards and the embedded body marker key on the SAME + // folded id. `fold_goal_identity` yields a pure `[0-9a-f]{16}` token, safe to + // embed and to interpolate into the backstop's `gh --search` (SR1). + let marker = format!( + "{OODA_GOAL_KEY_MARKER_PREFIX} {}", + fold_goal_identity(goal_id) + ); + // Idempotence: a goal already carrying any breaker artifact (a bare // suppression marker OR a linked tracking ref) is never re-filed — a re-stall // must not spam duplicate `ooda-stuck` issues, even across a daemon restart. @@ -290,22 +438,41 @@ fn escalate_with_tracking_issue( else { return; }; - let already_tracked = g.wip_refs.iter().any(is_breaker_tracking_ref); - // 1. Durable, link-independent suppression FIRST. Always block the goal; an - // already-suppressed goal stops here so it is never re-filed (idempotence - // across a `gh` failure and a daemon restart, since the marker lives on the - // goal board, not the in-memory tracker). + // 1. Durable, link-independent suppression FIRST. Always block the goal. g.status = GoalProgress::Blocked(blocked_reason); + + // Guard 1 — board-local `WipRef` fast path (zero API calls, SR7). An + // already-suppressed goal stops here so it is never re-filed (idempotence + // across a `gh` failure and a daemon restart, since the marker lives on the + // goal board, not the in-memory tracker). + let already_tracked = g.wip_refs.iter().any(is_breaker_tracking_ref); if already_tracked { return; } - g.wip_refs.push(suppression_marker()); - // 2. Best-effort link SECOND, holding the same borrow (`file_issue` does not - // touch `state`). On success upgrade the bare marker in place to the linked - // ref; on `None` the goal stays Blocked + suppressed and is not re-filed. - if let Some(filed) = filer.file_issue(issue_title, issue_body) { + // Guard 2 — GitHub-durable open-issue backstop. Consulted ONLY on a guard-1 + // miss, so a steady-state daemon with an intact board makes no extra API + // call. This covers the two conditions the `WipRef` marker cannot: goal-id + // churn (a fresh id has no `WipRef`) and a goal-board reset (the `WipRef` was + // erased). `filer` does not touch `state`, so the `&mut g` borrow is safe to + // hold across the call. A `false` default / query error fails OPEN (SR5). + if filer.issue_open_with_marker(&marker) { + // An open duplicate already exists — re-seed the board-local marker so + // the next cycle answers at the fast path (guard 1) without another API + // call, and do NOT re-file (storm-safe). + g.wip_refs.push(suppression_marker()); + return; + } + + // Guard 3 — no duplicate anywhere. Suppress FIRST (so a `gh` outage still + // leaves the goal Blocked + suppressed rather than Blocked + re-filing + // forever — the exact loop that produced the ~15-duplicate storm), then + // best-effort file the tracking issue with the goal-key marker embedded in + // its body so the backstop can dedup it on a future churned/reset cycle. + g.wip_refs.push(suppression_marker()); + let body_with_marker = format!("{issue_body}\n\n{marker}"); + if let Some(filed) = filer.file_issue(issue_title, &body_with_marker) { upgrade_suppression_marker_to_link(g, &filed); } } @@ -2399,3 +2566,232 @@ mod tests_derive_criteria { } } } + +/// # TDD (Step 7) — goal-key open-issue backstop: the two-guard filing gate +/// +/// FAILING BY DESIGN until the additive backstop specified in +/// `docs/reference/no-progress-breaker-goal-key-backstop-api.md` exists: +/// +/// * the `NoProgressIssueFiler::issue_open_with_marker` default method +/// (default `false`), and +/// * the second suppression guard in [`escalate_with_tracking_issue`] that +/// runs `issue_open_with_marker("ooda-goal-key:")` ONLY on a +/// board-local `WipRef` fast-path miss, embeds the `ooda-goal-key` marker in +/// the filed body, and re-seeds the `WipRef` marker when the backstop hits. +/// +/// These pin the churn/reset-durable dedup that the board-local `WipRef` marker +/// alone cannot provide (goal-id churn defeats its key; a board reset erases it). +#[cfg(test)] +mod tests_goal_key_backstop_gate { + use std::cell::{Cell, RefCell}; + + use super::{ + FiledIssue, NoProgressIssueFiler, escalate_with_tracking_issue, is_breaker_tracking_ref, + }; + use crate::goal_curation::no_progress_breaker::fold_goal_identity; + use crate::goal_curation::{ActiveGoal, GoalBoard, GoalProgress}; + use crate::ooda_loop::OodaState; + + /// A spy filer that records both trait methods so the gate's ordering and + /// cost contract (SR7: guard 2 only on guard-1 miss) is directly assertable. + struct SpyFiler { + /// `issue_open_with_marker` return value (the backstop verdict). + open_marker_returns: bool, + /// `file_issue` result: `Some(number)` succeeds, `None` models a `gh` + /// outage / unparsed URL. + file_returns: Option, + file_calls: Cell, + marker_calls: Cell, + last_body: RefCell>, + last_marker: RefCell>, + } + + impl SpyFiler { + fn new(open_marker_returns: bool, file_returns: Option<&str>) -> Self { + Self { + open_marker_returns, + file_returns: file_returns.map(str::to_string), + file_calls: Cell::new(0), + marker_calls: Cell::new(0), + last_body: RefCell::new(None), + last_marker: RefCell::new(None), + } + } + } + + impl NoProgressIssueFiler for SpyFiler { + fn file_issue(&self, _title: &str, body: &str) -> Option { + self.file_calls.set(self.file_calls.get() + 1); + *self.last_body.borrow_mut() = Some(body.to_string()); + self.file_returns.as_ref().map(|number| FiledIssue { + number: number.clone(), + url: Some(format!("https://github.com/o/r/issues/{number}")), + }) + } + + fn issue_open_with_marker(&self, marker: &str) -> bool { + self.marker_calls.set(self.marker_calls.get() + 1); + *self.last_marker.borrow_mut() = Some(marker.to_string()); + self.open_marker_returns + } + } + + /// A minimal filer that implements ONLY `file_issue`, relying on the trait's + /// default `issue_open_with_marker` — the additivity guarantee. + struct DefaultOnlyFiler; + impl NoProgressIssueFiler for DefaultOnlyFiler { + fn file_issue(&self, _t: &str, _b: &str) -> Option { + None + } + } + + fn state_with_active(goal: ActiveGoal) -> OodaState { + let mut board = GoalBoard::new(); + board.active.push(goal); + OodaState::new(board) + } + + fn only_goal(state: &OodaState) -> &ActiveGoal { + &state.active_goals.active[0] + } + + fn breaker_ref_count(goal: &ActiveGoal) -> usize { + goal.wip_refs + .iter() + .filter(|w| is_breaker_tracking_ref(w)) + .count() + } + + fn escalate(state: &mut OodaState, goal_id: &str, filer: &dyn NoProgressIssueFiler) { + escalate_with_tracking_issue( + state, + goal_id, + "[OODA-SAFEGUARD] no-progress breaker: UNCLEAR-CRITERIA".to_string(), + "OODA no-progress breaker: goal stuck (UNCLEAR-CRITERIA)", + "The OODA daemon produced no shippable action.", + filer, + ); + } + + /// Additivity: an impl that does NOT override `issue_open_with_marker` must + /// take the default `false` ("no duplicate found") path. + #[test] + fn default_issue_open_with_marker_is_false() { + let filer = DefaultOnlyFiler; + assert!( + !filer.issue_open_with_marker("ooda-goal-key: whatever"), + "the trait default must be false so existing filers/fakes are unchanged" + ); + } + + /// Case 1 — first filing. Guard 1 misses (fresh goal, no `WipRef`) and guard + /// 2 misses (no open duplicate) → `file_issue` is called exactly once, and + /// the body it receives carries the `ooda-goal-key:` marker keyed + /// on THIS goal's folded id. + #[test] + fn first_filing_files_once_and_embeds_the_goal_key_marker() { + let goal_id = "simard-identity-atelier-industrial-furniture-de"; + let filer = SpyFiler::new(false, Some("4231")); + let mut state = state_with_active(ActiveGoal::new(goal_id, "vague goal", 1)); + + escalate(&mut state, goal_id, &filer); + + assert_eq!(filer.file_calls.get(), 1, "first firing files exactly once"); + let body = filer.last_body.borrow().clone().expect("body captured"); + let expected = format!("ooda-goal-key: {}", fold_goal_identity(goal_id)); + assert!( + body.contains(&expected), + "filed body must embed the goal-key marker line {expected:?}; got:\n{body}" + ); + assert_eq!( + breaker_ref_count(only_goal(&state)), + 1, + "one linked breaker ref" + ); + assert!(matches!(only_goal(&state).status, GoalProgress::Blocked(_))); + } + + /// Case 2 — duplicate suppressed across goal-id churn / board reset. Guard 1 + /// misses (the `WipRef` was erased by churn/reset) but guard 2 finds an OPEN + /// issue carrying the goal-key marker → `file_issue` is NOT called, and the + /// board-local `WipRef` marker is RE-SEEDED so the next cycle hits the fast + /// path again. + #[test] + fn backstop_hit_suppresses_filing_and_reseeds_the_wipref() { + let goal_id = "g-churned"; + let filer = SpyFiler::new(true, Some("9999")); + let mut state = state_with_active(ActiveGoal::new(goal_id, "vague goal", 1)); + + escalate(&mut state, goal_id, &filer); + + assert_eq!(filer.marker_calls.get(), 1, "backstop consulted once"); + assert_eq!( + filer.file_calls.get(), + 0, + "an open duplicate found by the backstop must suppress re-filing" + ); + assert_eq!( + breaker_ref_count(only_goal(&state)), + 1, + "the backstop hit must re-seed the board-local WipRef marker so the \ + fast path suppresses future cycles without another API call" + ); + let marker = filer.last_marker.borrow().clone().expect("marker captured"); + assert!( + marker.contains(&fold_goal_identity(goal_id)), + "the backstop must be queried with THIS goal's folded key; got {marker:?}" + ); + } + + /// Case 3 — fast path, zero API calls (SR7). Once a board-local `WipRef` + /// marker exists (guard 1 hits), the backstop `issue_open_with_marker` must + /// NOT be consulted again. + #[test] + fn fast_path_hit_never_consults_the_backstop() { + let goal_id = "g-stable"; + // First firing seeds the WipRef marker (file fails, guard 2 miss). + let first = SpyFiler::new(false, None); + let mut state = state_with_active(ActiveGoal::new(goal_id, "vague goal", 1)); + escalate(&mut state, goal_id, &first); + assert_eq!( + first.marker_calls.get(), + 1, + "guard 2 consulted on the guard-1 miss" + ); + assert_eq!( + breaker_ref_count(only_goal(&state)), + 1, + "WipRef marker seeded" + ); + + // Second firing: guard 1 must hit and short-circuit before guard 2. + let second = SpyFiler::new(true, Some("1")); + escalate(&mut state, goal_id, &second); + assert_eq!( + second.marker_calls.get(), + 0, + "an intact board WipRef must answer at guard 1 — the backstop API is \ + never called (zero steady-state API cost, SR7)" + ); + assert_eq!(second.file_calls.get(), 0, "and no re-file"); + } + + /// Case 6 — fail-open. A backstop that returns `false` (its documented + /// behaviour on a `gh` error) must let filing proceed rather than abort: + /// losing a stuck-goal signal is worse than a rare duplicate (SR5). + #[test] + fn backstop_false_fails_open_and_files() { + let goal_id = "g-fail-open"; + let filer = SpyFiler::new(false, Some("7000")); + let mut state = state_with_active(ActiveGoal::new(goal_id, "vague goal", 1)); + + escalate(&mut state, goal_id, &filer); + + assert_eq!(filer.marker_calls.get(), 1, "backstop consulted"); + assert_eq!( + filer.file_calls.get(), + 1, + "a false (fail-open) backstop verdict must not block filing" + ); + } +} diff --git a/src/stewardship/dedup.rs b/src/stewardship/dedup.rs index e9a0a20d2..58b9a1884 100644 --- a/src/stewardship/dedup.rs +++ b/src/stewardship/dedup.rs @@ -19,7 +19,12 @@ pub fn normalize(msg: &str) -> String { /// Replace volatile tokens (paths, ISO timestamps, run IDs, long hex blobs, /// UUIDs) with stable placeholders so two runs of the same underlying failure /// produce identical signatures. -fn redact_token(t: &str) -> String { +/// +/// `pub(crate)` so the OODA no-progress breaker (in `goal_curation`) can reuse +/// the same redaction before embedding goal-derived free-text in an escalation +/// body (SR3/SR6). Non-breaking visibility widening — no signature/behaviour +/// change. +pub(crate) fn redact_token(t: &str) -> String { if t.starts_with('/') { return "".to_string(); } @@ -53,7 +58,10 @@ fn redact_token(t: &str) -> String { /// strict — exactly `8-4-4-4-12` hex digits with hyphens only at the four fixed /// offsets — so a hyphenated hex run of any other shape (e.g. a git range or an /// ISO date) is never mistaken for a UUID. -fn redact_uuids(t: &str) -> String { +/// +/// `pub(crate)` for reuse by the OODA breaker's body redaction (see +/// [`redact_token`]). +pub(crate) fn redact_uuids(t: &str) -> String { // UUIDs are ASCII, so a byte scan is correct and index-safe. let bytes = t.as_bytes(); let mut out = String::with_capacity(t.len());