diff --git a/docs/concepts/gap-scan-backoff-dedup.md b/docs/concepts/gap-scan-backoff-dedup.md index 4f8b73998..e883d3b52 100644 --- a/docs/concepts/gap-scan-backoff-dedup.md +++ b/docs/concepts/gap-scan-backoff-dedup.md @@ -8,14 +8,18 @@ description: > fixed-window dedup was insufficient, how exponential backoff rate-limits without ever permanently silencing a genuinely recurring gap, how this makes the Overseer ACT on a gap once rather than observe it forever (meta bugs - #4255 / #4126), and the planned cross-process open-issue check (future work). -last_updated: 2026-07-17 + #4255 / #4126), and the stable, content-addressed gap signature — the + foundation that a future durable cross-process open-issue check will build on + to survive daemon restarts. +last_updated: 2026-07-25 review_schedule: as-needed owner: simard doc_type: concept status: reference related: - ../reference/overseer-backoff-gate-api.md + - ../reference/overseer-gap-durable-dedup.md + - ../howto/configure-gap-durable-dedup.md - ../howto/configure-overseer-gap-scan-backoff.md - ../reference/overseer-workstream-gap-scan.md - ../reference/overseer-recipe-launch-idempotency.md @@ -84,8 +88,10 @@ The fix ([BackoffGate reference](../reference/overseer-backoff-gate-api.md)) is deliberately additive — it adds a new primitive rather than mutating the existing `WhisperGate`, so every current caller is untouched. An in-process -exponential-backoff gate guards the gap-cover act path, with a cross-process -open-issue equivalence check planned as a follow-on (future work). +exponential-backoff gate guards the gap-cover act path. A stable, +content-addressed gap signature (shipped here) lays the foundation for a +durable cross-process open-issue equivalence check — the follow-on that will let +the guarantee survive daemon restarts. ### Exponential BackoffGate (in-process) — implemented @@ -111,12 +117,30 @@ covering issue per distinct gap for the life of the process. ### Open-issue equivalence check (cross-process) — future work The BackoffGate is in-memory, so a daemon restart forgets its state and a cold -gate could re-file a duplicate that is already open on GitHub. A planned -follow-on would, before launching a gap-cover recipe, do a **best-effort** GitHub -query (reusing the existing `stewardship::dedup` helpers) for an already-open -**equivalent** issue and skip the launch if one exists (failing toward surfacing -on any API error). This cross-process layer is **not** part of #4186; it is -documented here as intended direction only. +gate could re-file a duplicate that is already open on GitHub. The deeper cause +of the observed `[stewardship] workstream_gap:*` flood (e.g. #4671, #4680, +#4685; OODA-stuck #4689) was that the gap signature was keyed +per **run** (`originating-run: overseer-`), so the in-process gate key +churned every run and any GitHub-side search would never match across runs. + +This change fixes the **root cause** by making the signature a **stable, +content-addressed slug** (derived from trusted identifiers, not the run id), so +the in-process gate now collapses a recurring gap to a single notification +**within a running daemon**, and the slug is a valid, restart-safe join key. +That stable signature is the **prerequisite** for a durable cross-process check. + +The durable check itself is **future work**: before notifying/filing, the +Overseer would run a GitHub query (reusing the existing `stewardship::dedup` +helpers and `find_existing` on the `stewardship-signature:` body marker) for an +already-open **equivalent** issue and reuse/skip if one exists, failing loud on +any `gh` error. The gap-notification path (`act_flag_workstream_gaps`) does +**not** perform this query today — it applies the in-process `WhisperGate` and +notifies the operator. The proven durable pattern lives on the sibling +stewardship filing seam (`stewardship::process_orchestrator_run`); wiring it +onto the gap path is the follow-on this stable signature enables, tracked in +[#4717](https://github.com/rysweet/Simard/issues/4717). See the +[gap-filing dedup reference](../reference/overseer-gap-durable-dedup.md) +and the [how-to](../howto/configure-gap-durable-dedup.md). ## How this makes the Overseer ACT (not just observe) @@ -146,7 +170,9 @@ protect. - **One open covering issue per distinct gap within a process.** The in-process gate holds duplicate coverage plans across ticks; the cross-process case - (restarts) is the planned open-issue check (future work). + (restarts / multiple daemons) is not yet closed — the stable signature shipped + here is the foundation for the future durable + [open-issue check](../reference/overseer-gap-durable-dedup.md). - **Never permanently silent.** The window is capped and resets after silence; a genuinely recurring gap always re-surfaces. - **Additive / non-breaking.** A new `BackoffGate` primitive; existing diff --git a/docs/howto/configure-gap-durable-dedup.md b/docs/howto/configure-gap-durable-dedup.md new file mode 100644 index 000000000..86114e8b8 --- /dev/null +++ b/docs/howto/configure-gap-durable-dedup.md @@ -0,0 +1,162 @@ +--- +title: Configure and verify gap dedup +description: > + How to operate and verify the Overseer's stable-signature gap dedup for + workstream-gap notifications — confirm a recurring gap is deduped to one + operator notification within a running daemon, read the + overseer::gap_scan flagged/suppressed logs, understand that a daemon restart + resets the in-process gate, and check the bounded GapCategory taxonomy that + keeps gap signatures stable and dedupable. +last_updated: 2026-07-25 +review_schedule: as-needed +owner: simard +doc_type: howto +related: + - ../reference/overseer-gap-durable-dedup.md + - ../reference/overseer-workstream-gap-scan.md + - ../reference/overseer-backoff-gate-api.md + - ../concepts/gap-scan-backoff-dedup.md + - ./review-overseer-workstream-gaps.md + - ./configure-overseer-gap-scan-backoff.md + - ./file-stewardship-issues-from-orchestrator-runs.md +--- + +# Configure and verify gap dedup + +The Overseer flags uncovered backlog work — uncovered goals, high-signal open +issues, and unaddressed telemetry anomalies — on its recurring gap-scan and +**notifies the operator** (email + Signal) about it. This rail makes the gap's +dedup signature a **stable, content-addressed** slug (instead of a per-run hash), +so a recurring gap is deduped to **one notification within a running daemon** +rather than re-notified every tick. This is the root-cause fix behind the +near-duplicate `[stewardship] workstream_gap:*` noise (observed on e.g. #4671, +#4680, #4685). + +For the data model, signature grammar, and guarantees, see the +[gap-filing dedup reference](../reference/overseer-gap-durable-dedup.md). + +> **Scope.** The gap-notification path dedupes via the **in-process** +> `WhisperGate` and notifies the operator; it does **not** create GitHub issues +> and is **not restart-safe on its own** — a daemon restart resets the gate. A +> durable, GitHub-sourced cross-process check is scoped as follow-on work +> ([#4717](https://github.com/rysweet/Simard/issues/4717)) and is +> **not** wired on this path yet; see the reference doc's *Future work* section. + +## What changed for operators + +- **Before:** the gap signature was derived per run + (`originating-run: overseer-`), so every restart/re-run minted a fresh + key and the in-process gate could not collapse a recurring gap → the scan + re-notified a near-duplicate gap every tick. +- **Now:** the signature is a **stable, content-addressed** slug, so the + in-process gate collapses a recurring gap to **one notification per dedup + window** for the life of the daemon. +- **Restart behaviour:** the in-process gate is memory-resident, so a restart + resets it and the first post-restart tick may re-notify. Cross-restart dedup + awaits the durable check (future work). + +There is **nothing to turn on** — stable-signature dedup is always active on the +gap path whenever the Overseer runs. The existing knobs still apply: + +| Env var | What it does | Default | +|---|---|---| +| `SIMARD_OVERSEER_GAP_SCAN` | Falsey (`0`/`false`/`no`/`off`) turns the gap-scan off entirely | on | +| `SIMARD_OVERSEER_GAP_SCAN_EVERY_N` | Run the scan every *Nth* tick | `1` | + +See [configure the gap-scan backoff](./configure-overseer-gap-scan-backoff.md) +for the in-process dedup window. + +## Prerequisites + +- The acting Overseer is enabled (`SIMARD_OVERSEER_ENABLED` unset or truthy). +- An operator notifier is configured (email and/or Signal) so gap notifications + have somewhere to go. + +## Verify: a recurring gap is deduped within a run + +This is the core acceptance check for the workstream-gap dedup rail. + +1. With a standing, uncovered gap present, let the Overseer run one gap-scan tick + and confirm it notifies **once** (a `flagged>=1` info line): + + ```text + INFO overseer::gap_scan flagged=1 suppressed=0 + overseer recorded uncovered backlog work and notified the operator + ``` + +2. On the **next** tick within the dedup window, confirm the **same** gap is + suppressed rather than re-notified — the count moves to `suppressed`: + + ```text + DEBUG overseer::gap_scan flagged=0 suppressed=1 + overseer gap-scan: every observed gap is within the dedup window (suppressed) + ``` + +3. `flagged=0 suppressed=1` for the recurring gap is the proof the stable + signature deduped it. (A daemon restart between steps 1 and 2 resets the gate + and may re-notify — that is expected until the durable check lands.) + +## Read the logs + +The gap path emits structured `tracing` + OTel only (no `print!`/`println!`), on +`target: "overseer::gap_scan"`: + +| Field | Meaning | +|---|---| +| `flagged` | Fresh gaps notified this tick | +| `suppressed` | Gaps dropped by the in-process gate or by a malformed signature | +| `dispatched` / `all_sent` | Operator-notification delivery status | + +## Malformed signatures are dropped (injection defense) + +A gap whose signature is not a valid restricted slug +(`^[a-z0-9][a-z0-9:_#.\-/]{0,200}$`) is **dropped at the filing seam** and counted +as suppressed — it never reaches an operator notification: + +```text +WARN overseer::gap_scan category="goal" + overseer gap-scan: dropping a gap with a malformed dedup signature + (outside the bounded taxonomy) +``` + +This is deliberate: signatures come from trusted identifiers only, so a malformed +one signals a bug or an injection attempt, not a real gap. + +## The bounded taxonomy (why signatures are now stable) + +Duplicates used to slip through because free-form titles drifted between ticks. +Each gap resolves to a bounded `GapCategory` variant with a stable slug that +anchors the signature: + +| Gap kind | `GapCategory` | Signature prefix | +|---|---|---| +| Uncovered p1/p2 goal | `GoalUncovered` | `goal:` | +| High-signal open issue | `IssueUncovered` | `issue:#` | +| Unaddressed anomaly | `AnomalyUnaddressed` | `anomaly:` | + +`GapCategory` is a closed enum of exactly these three kinds, so a gap's signature +is stable across ticks. The fix did not add kinds — it made the signature a +stable, content-addressed slug (instead of a per-run hash) so the in-process gate +recognises the same gap across ticks. This change is additive. + +## Common pitfalls + +- **A duplicate notification appeared right after a restart.** Expected — the + in-process gate is reset on restart. Cross-restart dedup is future work. +- **Two notifications for the "same" gap.** Confirm the gaps carry the **same** + signature. If the signatures differ, the gap resolved to two distinct keys + (e.g. two different goal ids) — correct behaviour, not a dedup miss. +- **A gap I expected was never notified.** Check for a `dropping a gap with a + malformed dedup signature` WARN — a signature outside the bounded taxonomy is + dropped by design. + +## See also + +- [Gap-filing dedup reference](../reference/overseer-gap-durable-dedup.md) + — signature grammar, the in-process flow, and the scoped durable follow-on. +- [Review the Overseer's workstream gaps](./review-overseer-workstream-gaps.md) + — where the gaps surface and how to respond. +- [Gap-scan dedup & exponential backoff](../concepts/gap-scan-backoff-dedup.md) + — the in-process gate this stable signature feeds. +- [File stewardship issues from orchestrator runs](./file-stewardship-issues-from-orchestrator-runs.md) + — the sibling loop whose durable dedup flow the future gap check would mirror. diff --git a/docs/howto/review-overseer-workstream-gaps.md b/docs/howto/review-overseer-workstream-gaps.md index a8b8d40c9..1429da577 100644 --- a/docs/howto/review-overseer-workstream-gaps.md +++ b/docs/howto/review-overseer-workstream-gaps.md @@ -12,6 +12,8 @@ owner: simard doc_type: howto related: - ../reference/overseer-workstream-gap-scan.md + - ../reference/overseer-gap-durable-dedup.md + - ../howto/configure-gap-durable-dedup.md - ./watch-overseer-activity.md - ../reference/overseer-activity-feed.md - ../design/overseer.md diff --git a/docs/index.md b/docs/index.md index f67411e1e..ad8dae1b9 100644 --- a/docs/index.md +++ b/docs/index.md @@ -167,6 +167,8 @@ Bare `simard` prints the unified help text instead of attempting a hidden enviro - [Canary gate isolation and self-deploy convergence reference](./reference/canary-gate-convergence.md) - The #4440 root-cause repair that acts on those diagnostics so a healthy candidate self-deploys and `DeployDrift` returns to 0: per-gate `self_relaunch::gate` tracing/OTel spans in `verify_canary`, the additive `RelaunchConfig.canary_env` narrow allow-list plus `scrub_gate_env` that supplies a gate's legitimately-missing signal without weakening fail-closed semantics, and the self-deploy loop advancing past the stuck target SHA. See the [convergence runbook](./howto/converge-a-stuck-red-canary-self-deploy.md). - [How to review the Overseer's workstream gaps](./howto/review-overseer-workstream-gaps.md) - Read, act on, and tune the Overseer's recurring "what workstreams are we missing?" gap-scan — the uncovered high-priority goals, high-signal issues, and unaddressed anomalies it flags each tick, where the deduped notification appears, and the `SIMARD_OVERSEER_GAP_SCAN` knobs (#2630). - [Overseer workstream gap-scan reference](./reference/overseer-workstream-gap-scan.md) - The additive Observe→Orient→Act gap-scan: the `Signal::WorkstreamGap`/`GapItem`/`ProblemKind::WorkstreamCoverage` model, the coverage-set detection contract, the deduped NotifyOperator act path, the `SIMARD_OVERSEER_GAP_SCAN` configuration, and the additive `OverseerTickReport.workstream_gaps_detected` counter (#2630). +- [Overseer gap-filing dedup reference](./reference/overseer-gap-durable-dedup.md) - The stable, content-addressed gap signature that stops the Overseer re-notifying a near-duplicate gap every tick within a running daemon: the bounded `GapCategory` taxonomy (`GoalUncovered`/`IssueUncovered`/`AnomalyUnaddressed`), the slug-validated `stewardship-signature:` key that replaces the old per-run hash, the in-process `WhisperGate` dedup + operator notification on the gap path, and the injection-defense slug validation enforced at the filing seam. The durable, restart-safe GitHub-side open-issue check is scoped as the follow-on this signature enables. +- [How to configure and verify gap dedup](./howto/configure-gap-durable-dedup.md) - Confirm a recurring gap is deduped to one operator notification within a running daemon, read the `overseer::gap_scan` `flagged`/`suppressed` logs, understand that a daemon restart resets the in-process gate, and check the bounded taxonomy that keeps gap signatures stable and dedupable. - [Overseer recipe-launch idempotency reference](./reference/overseer-recipe-launch-idempotency.md) - The launcher-level rail that makes `AmplihackRecipeRunner::spawn` idempotent per task signature: the pure `recipe_signature` normalization (`target_repo` + `task_description`, trim/lowercase/whitespace-collapse, `\u{1F}` separator), the reap-then-dedup order, the fail-visible `overseer::recipe` suppressed-launch warning, the shared-handle `probe` semantics, and the injectable `ChildSpawner`/`SpawnedChild` test seam — so a still-blocked signature no longer spawns a byte-identical `smart-orchestrator` every tick (#4125). - [Overseer root-cause ("WHY") API reference](./reference/overseer-root-cause-why-api.md) - The `RootCause`/`CauseCandidate`/`Confidence`/`Likelihood`/`CauseSource` model and additive `Problem.why` field, the pure `root_cause::analyze` analyzer with `PriorOccurrence`/`root_cause_signature`, the `Remediation`/`RemediationClass` classification on `PlannedIntervention`, the `Overseer::with_memory` recall+store seam, the `decide_blocked_goal` recurrence routing, the `goal_blocked_with_why` WHY notification constructor, the `ProblemEntry` activity-feed rows, and the extended `OverseerTickReport`/`OverseerTotals` counters (#2635). - [How to configure and observe the Overseer root-cause principle](./howto/configure-overseer-root-cause-why.md) - Read the WHY and the root-cause/symptom label in the `overseer::root_cause` traces, the `OverseerTickReport` counters, the activity-feed `problem_entries` rows, and the `goal-blocked` notification; understand the always-on/no-opt-out contract and the graceful memory-recall degrade; and verify the feature end-to-end with injected fakes (#2635). diff --git a/docs/reference/overseer-gap-durable-dedup.md b/docs/reference/overseer-gap-durable-dedup.md new file mode 100644 index 000000000..c2a1ef735 --- /dev/null +++ b/docs/reference/overseer-gap-durable-dedup.md @@ -0,0 +1,245 @@ +--- +title: Overseer gap-filing dedup reference +description: > + The stable, content-addressed gap signature on the Overseer stewardship + gap-notification path — the bounded GapCategory taxonomy + (GoalUncovered / IssueUncovered / AnomalyUnaddressed), the slug-validated + signature grammar that replaces the old per-run hash, the in-process + WhisperGate dedup + operator notification the gap path actually performs, the + injection-defense slug validation enforced at the filing seam, and the durable + cross-process open-issue check this signature is the foundation for (scoped as + follow-on work, not yet wired on the gap path). +last_updated: 2026-07-25 +review_schedule: as-needed +owner: simard +doc_type: reference +related: + - ./overseer-workstream-gap-scan.md + - ./overseer-backoff-gate-api.md + - ./stewardship-api.md + - ../concepts/gap-scan-backoff-dedup.md + - ../howto/configure-gap-durable-dedup.md + - ../howto/file-stewardship-issues-from-orchestrator-runs.md + - ../howto/review-overseer-workstream-gaps.md + - ../design/overseer.md +--- + +# Overseer gap-filing dedup reference + +The acting **Overseer** watches for uncovered backlog work — uncovered goals, +high-signal open issues, and unaddressed telemetry anomalies — and flags it on +the recurring gap-scan. Before this rail the gap signature was derived **per +run** (`originating-run: overseer-`), so every daemon start or re-run +minted a **fresh** signature. The only dedup guard on the gap path is the +in-process [`WhisperGate` / `BackoffGate`](./overseer-backoff-gate-api.md), which +keys on that signature; a churning per-run key meant the same gap re-surfaced as +noise, and the near-duplicate `[stewardship] workstream_gap:*` flood was observed +downstream (e.g. #4671, #4680, #4685). + +This rail fixes the **root cause**: it makes the signature a **stable, +content-addressed** slug (derived from the gap's trusted identifiers, not the run +id). With a stable key, the in-process gate now collapses a recurring gap to a +**single operator notification within a running daemon**, and the slug is a +valid, restart-safe join key that a future durable check can search on. + +> **Scope — read this first.** What ships here is the **stable signature**, the +> **bounded `GapCategory` taxonomy**, and **injection-defense slug validation**. +> The gap-notification path (`act_flag_workstream_gaps`, +> `src/overseer/mod.rs`) applies the in-process `WhisperGate` and **notifies the +> operator** (email + Signal); it does **not** call `gh.search_issues`, +> `find_existing`, or `create_issue`, and it is **not restart-safe on its own** +> (the gate is in memory and is wiped on restart). The durable, GitHub-sourced +> open-issue check described under [Future work](#future-work-the-durable-cross-process-check) +> is **not yet wired** on the gap path; the stable signature is the prerequisite +> that makes it viable. The proven durable pattern currently lives on the sibling +> stewardship filing seam (`stewardship::process_orchestrator_run`). + +> **Modules:** taxonomy + signature grammar `src/overseer/signal.rs` +> (`GapCategory`, `GapItem::signature`, `has_valid_dedup_signature`); batch key +> and filing seam `src/overseer/mod.rs` (`workstream_gap_key`, +> `act_flag_workstream_gaps`); detectors `src/overseer/sensor.rs` +> (`detect_workstream_gaps`). The durable pattern this signature is designed to +> feed lives in `src/stewardship/gh_client.rs` and `src/stewardship/mod.rs` +> (`find_existing`, `process_orchestrator_run`). Hermetic tests +> `src/overseer/signal.rs` (`gap_dedup_key_tests`), +> `src/stewardship/tests_extra.rs`. + +## At a glance + +| You want to… | Use | +|---|---| +| Understand why a recurring gap stopped re-notifying every tick | This page — the stable signature + in-process gate | +| See the stable key that dedupes a gap | `GapItem.signature` → `stewardship-signature: workstream-gap:` | +| Know which gap kinds are deduped | The bounded [`GapCategory` taxonomy](#the-bounded-gapcategory-taxonomy) | +| Confirm a recurring gap was deduped, not re-notified | `overseer::gap_scan` `flagged=…`/`suppressed=…` info log + `ActOutcome::WorkstreamGapsFlagged` | +| Operate / verify it | [How to configure and verify gap dedup](../howto/configure-gap-durable-dedup.md) | + +## The bounded `GapCategory` taxonomy + +Free-form gap titles let the same underlying condition render slightly different +strings on each tick, so no two dedup keys matched. The taxonomy replaces +free-form detection with a **bounded enum**; every variant carries a short, +stable `.label()` slug that anchors the signature. + +| `GapCategory` variant | `.label()` | Signature prefix | Filed for | +|---|---|---|---| +| `GoalUncovered` | `goal` | `goal:` | A p1/p2 active goal with no engineer and no PR | +| `IssueUncovered` | `issue` | `issue:#` | A high-signal open issue with no PR / workstream | +| `AnomalyUnaddressed` | `anomaly` | `anomaly:` | A live telemetry anomaly with no fix in flight | + +The taxonomy is a **bounded, closed enum** (`src/overseer/signal.rs`), so a gap +can only ever resolve to one of these three stable `.label()` slugs — the same +underlying condition renders an **identical** signature on every tick. Every +exhaustive `match` on `GapCategory` already covers these three arms; **none was +renamed**. + +## The dedup key (signature) grammar + +Each `GapItem` carries a `signature`: a **stable, constrained slug** built from +**trusted identifiers only** (a goal id, `repo#number`, or an anomaly slug) — +never from hostile free text. Identical inputs yield an identical signature, so a +recurring gap dedupes to at most one live notification per daemon. + +The act path wraps the per-gap signature in a filing key: + +```text +stewardship-signature: workstream-gap: +``` + +`workstream_gap_key` composes the batch key deterministically (sorted, deduped) +so a multi-gap tick is stable regardless of detection order: + +```rust +// src/overseer/mod.rs +fn workstream_gap_key(gaps: &[GapItem]) -> String { + let mut sigs: Vec<&str> = gaps.iter().map(|g| g.signature.as_str()).collect(); + sigs.sort_unstable(); + sigs.dedup(); + format!("workstream-gap:{}", sigs.join("|")) +} +``` + +### Signature slug validation (injection defense) + +Signatures are validated **at construction** in `signal.rs` and re-checked at the +filing seam (`has_valid_dedup_signature` in `act_flag_workstream_gaps`), so a +malformed or hostile identifier can never reach a notification body — or a +future `gh search` argument. A valid signature matches: + +```text +^[a-z0-9][a-z0-9:_#.\-/]{0,200}$ +``` + +Any identifier that would violate this (control characters, spaces, shell +metacharacters, over-length) is rejected at the boundary and the gap is dropped +(counted as suppressed so the observability contract stays exact). This makes any +downstream search query **inert**: a goal id such as `g-1; rm -rf ~` cannot +survive slug validation, so it can never inject into a `gh issue list --search` +term when the durable check is wired. + +## What the gap path does today + +The gap-notification act path (`act_flag_workstream_gaps`) runs, per tick: + +```text +for each detected gap: + 1. has_valid_dedup_signature(gap) # IV-1: drop a malformed/hostile signature + └─ invalid → count suppressed, continue (never notified) + 2. WhisperGate.peek(sig) # in-process dedup (no gh call) + └─ SuppressDuplicate/Cap → count suppressed, continue + └─ Deliver → collect as a FRESH gap +if any fresh gaps: + 3. notifier.notify(workstream_gap) # ONE consolidated operator notification + # (email + Signal), never create_issue + 4. WhisperGate.commit(sig) # record the fresh notification in-process +``` + +Key properties **as implemented**: + +- **Notification-only.** Routine gap observations notify the operator; they do + **not** create GitHub issues or stewardship backlog items on this path. +- **Deduped within a process.** The stable signature makes the in-process gate + collapse a recurring gap to one notification per dedup window (default 900 s). +- **Not restart-safe on its own.** The `WhisperGate` lives in memory, so a daemon + restart resets it. Cross-restart dedup depends on the future durable check + below. +- **Fail-closed at the seam.** A malformed signature is dropped **before** it can + reach a notification body — never rendered into operator-facing text. + +## Future work: the durable cross-process check + +The stable signature exists so a durable, restart-safe check can be layered in +**without** re-deriving a key. The intended follow-on mirrors the proven +[`stewardship::process_orchestrator_run`](./stewardship-api.md) flow: + +```text +for each detected gap (after the in-process gate): + gh.search_issues(repo, sig) # DURABLE open-issue equivalence check + └─ Err(e) → FAIL LOUD: propagate, file nothing + find_existing(&issues, sig) # match `stewardship-signature: ` + └─ Some(issue) → reuse/comment/skip (no new issue) + └─ None → gh.create_issue(repo, title, body) +``` + +When wired, this would upgrade the guarantee to *"at most one open issue per +distinct gap signature across restarts and daemons"* and must **fail loud** — a +`gh` search error stops filing rather than falling back to a blind +`create_issue`, which would reintroduce exactly the flood this work prevents. +**This flow is not implemented on the gap-notification path today; it is +tracked in [#4717](https://github.com/rysweet/Simard/issues/4717).** + +## Structured observability + +The gap path emits structured `tracing` + OTel only — **no** +`print!`/`println!`. On `target: "overseer::gap_scan"`: + +| Field | Meaning | +|---|---| +| `flagged` | Fresh gaps notified this tick | +| `suppressed` | Gaps dropped by the in-process `WhisperGate` or by a malformed signature | +| `dispatched` / `all_sent` | Operator-notification delivery status | + +The `ActOutcome::WorkstreamGapsFlagged { flagged, suppressed }` outcome and the +`OverseerTickReport.workstream_gaps_detected` / `…_suppressed` counters are +unchanged and additive. + +## Guarantees + +- **At most one live notification per distinct gap signature within a running + daemon** (in-process, `WhisperGate`-sourced). Cross-restart / cross-daemon + dedup is **not** yet guaranteed — see [Future work](#future-work-the-durable-cross-process-check). +- **Never permanently silent.** The in-process gate window is capped and resets + after silence; a genuinely recurring gap always re-surfaces. +- **Injection-safe.** A gap whose signature is not a valid restricted slug is + dropped at the seam, so no hostile identifier reaches a notification body or a + future search term. +- **Additive / non-breaking.** Only the signature's *value* changes (unstable + per-run hash → stable per-gap slug); the `stewardship-signature:` marker field, + the notification structure, and every existing caller are untouched. Pre-fix + issues carry the old per-run hash and do not retro-dedupe. + +## Security notes + +| Risk | Mitigation | +|---|---| +| Search-query / notification injection via an unconstrained signature | Slug-validate the signature at construction **and** at the seam (`^[a-z0-9][a-z0-9:_#.\-/]{0,200}$`); drop on failure | +| Fail-open regression re-introducing the flood (when the durable check is wired) | Propagate `search_issues` errors; never default to `create_issue` | +| Self-DoS from a fast detector loop | Mandatory `WhisperGate` pre-filter before any external call | + +No credential handling lives on this path: the future durable check would let +`gh` supply its own token, which is never read, logged, or embedded. + +## Related + +- [Overseer workstream gap-scan reference](./overseer-workstream-gap-scan.md) — + the `GapItem`/`GapCategory`/`Signal::WorkstreamGap` data model this extends. +- [Gap-scan dedup & exponential backoff](../concepts/gap-scan-backoff-dedup.md) + — the in-process `BackoffGate` this signature stabilizes. +- [Overseer backoff-gate API reference](./overseer-backoff-gate-api.md) — the + in-process gate the gap path uses. +- [Goal Stewardship — Orchestrator Failure API reference](./stewardship-api.md) + — the `search_issues → find_existing → create_issue` flow the future durable + check would mirror. +- [How to configure and verify gap dedup](../howto/configure-gap-durable-dedup.md). +- PRD: `Specs/ProductArchitecture.md` § *Stewardship Mode* / § *Goal + Stewardship Mode*. diff --git a/docs/reference/overseer-workstream-gap-scan.md b/docs/reference/overseer-workstream-gap-scan.md index d09ddf283..9b05b4788 100644 --- a/docs/reference/overseer-workstream-gap-scan.md +++ b/docs/reference/overseer-workstream-gap-scan.md @@ -16,6 +16,7 @@ owner: simard doc_type: reference related: - ./overseer-activity-feed.md + - ./overseer-gap-durable-dedup.md - ./overseer-self-observation-stability.md - ../design/overseer.md - ../howto/review-overseer-workstream-gaps.md @@ -83,7 +84,7 @@ gap reaches a person exactly once without creating recursive tracking work. | See the gaps the Overseer flagged | Dashboard **Overseer** tab / TUI **Overseer** pane / `simard status` → **OVERSEER** — each tick's line reads e.g. `flagged 2 workstream gaps` | | Read the gaps as JSON | `GET /api/overseer` → `data.recent[].report.workstream_gaps_detected` / `…_suppressed` | | Get told when a genuine gap appears | The deduped operator notification (email + Signal), kind `workstream-gap` | -| Find the filed gap issues | GitHub issues in `rysweet/Simard` opened by the Overseer's identity (`simard-overseer[bot]`) with the `workstream-gap` signature | +| See what covers a gap | The gated coverage `LaunchRecipe` (`WORKSTREAM_COVERAGE_GROUP`) the gap-scan decides to since #4128 — the gap scan itself files **no** GitHub issue | | Turn the scan up, down, or off | `SIMARD_OVERSEER_GAP_SCAN` + `SIMARD_OVERSEER_GAP_SCAN_EVERY_N` (see [Configuration](#configuration)) | ## What counts as a gap @@ -170,7 +171,8 @@ pub struct GapItem { /// is uncovered (e.g. "p1 goal with no engineer and no PR"). pub why_it_matters: String, /// Stable dedup signature for this gap (see the signature grammar below). - /// Used to de-duplicate notifications and filed issues across ticks. + /// Used to de-duplicate operator notifications (and coverage launches) across + /// ticks. pub signature: String, } @@ -263,8 +265,9 @@ contract: ## Signature grammar and de-duplication -Every gap carries a **stable signature** so the same recurring gap is notified -and filed **at most once**, matching the existing M1 dedup behaviour. The +Every gap carries a **stable signature** so the same recurring gap is acted on +(notified, or covered by a launch) **at most once** per window within a running +daemon, matching the existing M1 dedup behaviour. The signature is a restricted slug — only `[A-Za-z0-9_\-#/:]`, never raw titles, quotes, or newlines (input-validation guideline **V3**): @@ -274,20 +277,29 @@ quotes, or newlines (input-validation guideline **V3**): | `IssueUncovered` | `issue:/#` | `issue:rysweet/Simard#2630` | | `AnomalyUnaddressed` | `anomaly:` | `anomaly:distill_parse_fail` | -The dedup key committed to the gate is `workstream-gap:`. De-dup runs -in **two layers**, so neither a fast tick cadence nor a restart floods the -operator: - -1. **Gate layer.** Before acting on a gap, `act_flag_workstream_gaps` **peeks** the - `WhisperGate` for `workstream-gap:`; if already committed within the - window it records a *suppressed* outcome and skips. On success it **commits** - the key. The gate **fails closed** on an identity/read error (guardrail - **A2**) — an indeterminate gate suppresses rather than risks a duplicate. -2. **Stewardship layer.** The filed issue still goes through - `stewardship::{failure_signature, find_existing}`, so even across gate resets a - matching open issue is updated, not duplicated. - -The result: **one deduped item per recurring gap signature**, not one per tick. +The dedup key committed to the gate is `workstream-gap:`. De-duplication +is **in-process** on both act paths, so a fast tick cadence does not flood the +operator or the backlog: + +1. **Notify path (`act_flag_workstream_gaps`).** Before notifying, it **peeks** the + in-process `WhisperGate` (`gap_gate`) for `workstream-gap:`; if + already committed within the window it records a *suppressed* outcome and + skips. On success it **commits** the key. The gate **fails closed** on an + identity/read error (guardrail **A2**) — an indeterminate gate suppresses + rather than risks a duplicate. This path **only notifies the operator**; it + does **not** file or upsert a GitHub issue. +2. **Coverage path (`LaunchRecipe`, the #4128 default).** An equivalent covering + launch is **held** while one is already in flight (`inflight_investigations`) + and, after it completes, suppressed within a growing bounded window by the + in-process exponential `coverage_backoff` gate (keyed by `recipe_dedup_key`). + +Both gates are **in-memory**, so their state is per-process. A durable, +cross-process GitHub open-issue equivalence check that would survive a daemon +restart is **future work**, tracked in +[#4717](https://github.com/rysweet/Simard/issues/4717); see the +[gap-filing dedup reference](./overseer-gap-durable-dedup.md). The result today is +**one deduped item per recurring gap signature within a running daemon**, not one +per tick. ## Act path — the coverage closing edge (issue #4128, D3b) @@ -317,28 +329,23 @@ The result: **one deduped item per recurring gap signature**, not one per tick. When it is invoked directly, `act_flag_workstream_gaps` (`src/overseer/mod.rs`) acts through the Overseer's **existing** escalation -machinery — the same paths `goal_health` and M1 use — with no new bypass: +machinery — the same notifier `goal_health` and M1 use — with no new bypass: -1. **De-dupe** each gap via the gate (above); suppressed gaps are counted, not - acted on. +1. **De-dupe** each gap via the in-process `WhisperGate` (above); suppressed gaps + are counted, not acted on. A gap whose signature is not a valid bounded slug is + dropped and counted as suppressed. 2. **NotifyOperator.** Emit **one consolidated** `OperatorNotification::workstream_gap(count, top_gaps)` on **both** channels (email + Signal) through the `DualChannelNotifier` — a notification is never silently dropped; an unconfigured channel is `Queued` and logged. -3. **FileIssue.** File a **deduped** stewardship issue per gap signature through - the same `IssueFiler` path M1 uses - (`stewardship::process_orchestrator_run` + `find_existing`). The gap briefs - carry `source_module = "overseer"`, which matches no routing keyword; the - stewardship router's **default-repo fallback** therefore routes them to - `rysweet/Simard` (the `DEFAULT_TARGET_REPO` constant) and logs the fallback - with `tracing::warn!`. This is what lets the gap-scan actually file/upsert one - rolling tracking issue per gap signature every tick, rather than failing with - `overseer intervention failed … flag_workstream_gaps … cannot route - source-module 'overseer'`. - -Both the notify and the file happen as **side effects of this one act** — exactly -as `goal_health`'s escalate notifies both channels from a single intervention. -The act then returns **one** summarising `ActOutcome` (see + +That is the **entire** side effect of this act: it **notifies the operator only**. +It does **not** file or upsert a GitHub issue, and it does **not** call +`stewardship::process_orchestrator_run` / `find_existing`. (An `IssueFiler` / +`FileIssue` path does exist on the Overseer, but it is reached by the +`QualityRegression` CI-failure-cluster problem, **not** by the gap scan.) The act +then returns **one** summarising +`ActOutcome::WorkstreamGapsFlagged { flagged, suppressed }` (see [Tick counters and totals](#tick-counters-and-totals)); it does **not** return a separate `Escalated` / `IssueFiled` outcome, so gap activity is counted only on the gap-scan's own dedicated counters, never on the generic ones. @@ -536,9 +543,11 @@ pub fn gap_scan_every_n() -> u64; The scan honours the Overseer's shared cadence (`SIMARD_OVERSEER_INTERVAL_SECS`, 15-minute default, clamped to a 60 s floor) — `EVERY_N` multiplies that interval -rather than introducing a second clock. There is no per-scan state file: dedup -lives in the shared `WhisperGate` and in GitHub issues (durable findings are -issues or code, never committed snapshot docs). +rather than introducing a second clock. There is no per-scan state file: notify +-path dedup lives in the in-process `WhisperGate` and coverage-path dedup in the +in-process in-flight / exponential-backoff guards; durable findings are issues or +code, never committed snapshot docs. A durable GitHub-side open-issue check is +future work ([#4717](https://github.com/rysweet/Simard/issues/4717)). ## Using cognitive memory (best-effort) @@ -557,11 +566,15 @@ It never blocks a tick and never writes. - **Genuine gaps only.** Every candidate is deduped against the coverage set (in-flight refs ∪ open PRs); blocked goals are delegated to `goal_health`, so the scan never re-flags work already in motion. -- **Deduped delivery.** Two-layer dedup (gate + stewardship) yields **one** - notification and **one** issue per recurring gap signature — never a flood. -- **Reuses existing plumbing.** Notify and file go through the same - `DualChannelNotifier` / `IssueFiler` paths `goal_health` / M1 use — same - escalation, same gates, no `--admin`, no `--no-verify`, no new bypass. Since +- **Deduped delivery.** In-process dedup on the notify path (the `WhisperGate` + `gap_gate`) yields **one** operator notification per recurring gap signature, + and the coverage path's in-flight + exponential-backoff guards yield **one** + covering launch per signature — never a flood. Neither path files a GitHub + issue today; a durable cross-process check is future work + ([#4717](https://github.com/rysweet/Simard/issues/4717)). +- **Reuses existing plumbing.** The notify path goes through the same + `DualChannelNotifier` `goal_health` / M1 use — same escalation, same gates, no + `--admin`, no `--no-verify`, no new bypass. Since issue #4128 a coverage gap decides to a **gated closing-edge** `LaunchRecipe` (tagged `WORKSTREAM_COVERAGE_GROUP`): it is admitted only through the existing launch gate, **fails closed** without a distinct steward identity, and is @@ -581,7 +594,7 @@ It never blocks a tick and never writes. ## Security notes External GitHub issue and PR text is **untrusted input** that flows into -notifications, filed-issue bodies, and `gh` reads. The gap-scan therefore: +notifications, launched coverage `task_description`s, and `gh` reads. The gap-scan therefore: - **A1/A2:** exposes the new counter only on the already `require_auth`-gated `/api/overseer` (no new/unauthenticated route); `act_flag_workstream_gaps` **fails @@ -601,7 +614,7 @@ notifications, filed-issue bodies, and `gh` reads. The gap-scan therefore: - **S1/S2/S3:** the only launch the gap-scan adds — the issue #4128 coverage closing edge — stays **behind the existing launch gate** plus a fail-closed steward-identity guard and in-flight dedup (no unguarded auto-launch path); - two-layer dedup prevents notify/issue floods; the `SIMARD_OVERSEER_GAP_SCAN` + in-process dedup prevents notification and duplicate-launch floods; the `SIMARD_OVERSEER_GAP_SCAN` kill-switch is honoured (its opt-out holds the coverage launch too). ## Testing @@ -636,6 +649,8 @@ built on synthetic pictures — no network, no real `gh`, no clock dependence: - [Overseer design](../design/overseer.md) — the meta-OODA loop, the capability/guardrail model, and the `goal_health` pattern this scan mirrors. - [Stewardship API](./stewardship-api.md) — the deduped issue-filing path - (`failure_signature` / `find_existing`) reused here. + (`failure_signature` / `find_existing`) used by the Overseer's + `QualityRegression` CI-cluster path; the gap scan does **not** file issues but + shares the same signature-stability philosophy. - [No-progress breaker API](./no-progress-breaker-api.md) — the "needs human review" marker delegated to `goal_health` rather than re-flagged as a gap. diff --git a/mkdocs.yml b/mkdocs.yml index e26b9d50a..2bc3d53c3 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -282,6 +282,7 @@ nav: - Review Overseer Workstream Gaps: howto/review-overseer-workstream-gaps.md - Diagnose Recurring Cognitive-Memory Signature: howto/diagnose-recurring-cognitive-memory-signature.md - Configure Overseer Gap-Scan Backoff: howto/configure-overseer-gap-scan-backoff.md + - Configure & Verify Gap Dedup: howto/configure-gap-durable-dedup.md - Configure Overseer Root-Cause Principle: howto/configure-overseer-root-cause-why.md - Configure Cognitive-Thread Scheduling: howto/configure-cognitive-thread-scheduling.md - Add a New Cognitive Thread: howto/add-a-new-cognitive-thread.md @@ -320,6 +321,7 @@ nav: - Overseer Signal JSON-RPC Transport: reference/overseer-signal-jsonrpc-transport.md - Overseer Workstream Gap-Scan: reference/overseer-workstream-gap-scan.md - Overseer BackoffGate & Gap-Scan Dedup: reference/overseer-backoff-gate-api.md + - Overseer Gap-Filing Dedup: reference/overseer-gap-durable-dedup.md - Overseer Root-Cause (WHY) API: reference/overseer-root-cause-why-api.md - Overseer Self-Observation Stability: reference/overseer-self-observation-stability.md - simard-engineer-step CLI: reference/simard-engineer-step.md diff --git a/src/overseer/mod.rs b/src/overseer/mod.rs index b1d5c76c3..d0adecfd8 100644 --- a/src/overseer/mod.rs +++ b/src/overseer/mod.rs @@ -125,8 +125,8 @@ pub use sensor::{ blocked_goals_from_board, in_flight_from_board, observed_from_snapshot, run_observer_cycle, }; pub use signal::{ - CauseCandidate, CauseSource, Confidence, GapCategory, GapItem, Likelihood, Priority, Problem, - ProblemKind, RootCause, Signal, signals_from, + CauseCandidate, CauseSource, Confidence, GAP_DEDUP_KEY_PREFIX, GapCategory, GapItem, + Likelihood, Priority, Problem, ProblemKind, RootCause, Signal, signals_from, }; pub use whisper_ops::{ MeetingHandoffWhisperSink, WhisperRecord, WhisperSink, WhisperUrgency, compose_whisper_note, @@ -1968,7 +1968,23 @@ impl Overseer { let mut fresh: Vec = Vec::new(); let mut suppressed = 0usize; for g in gaps { - let sig = format!("workstream-gap:{}", g.signature); + // Enforce the bounded-taxonomy contract at the filing seam: a gap + // whose signature is not a stable, restricted slug (never expected + // from the sensor, which builds every signature from trusted + // identifiers) is dropped rather than allowed to inflate a + // notification or a dedup search query (IV-1). Counted as suppressed + // so the observability contract stays exact. + if !g.has_valid_dedup_signature() { + tracing::warn!( + target: "overseer::gap_scan", + category = g.category.label(), + "overseer gap-scan: dropping a gap with a malformed dedup signature \ + (outside the bounded taxonomy)" + ); + suppressed += 1; + continue; + } + let sig = g.dedup_key(); match self.gap_gate.peek(&sig, now) { WhisperDecision::Deliver => fresh.push(g.clone()), WhisperDecision::SuppressDuplicate | WhisperDecision::SuppressCapReached => { @@ -1999,7 +2015,7 @@ impl Overseer { let notification = OperatorNotification::workstream_gap(fresh.len(), &fresh); let report = notifier.notify(¬ification); for g in &fresh { - let sig = format!("workstream-gap:{}", g.signature); + let sig = g.dedup_key(); self.gap_gate.commit(&sig, now); } @@ -2453,7 +2469,7 @@ fn workstream_gap_key(gaps: &[GapItem]) -> String { let mut sigs: Vec<&str> = gaps.iter().map(|g| g.signature.as_str()).collect(); sigs.sort_unstable(); sigs.dedup(); - format!("workstream-gap:{}", sigs.join("|")) + format!("{}{}", GAP_DEDUP_KEY_PREFIX, sigs.join("|")) } /// Orient: fold `Signal`s into ranked, deduplicated `Problem`s. Dedups against diff --git a/src/overseer/sensor.rs b/src/overseer/sensor.rs index e9abf08e5..b564ade2b 100644 --- a/src/overseer/sensor.rs +++ b/src/overseer/sensor.rs @@ -18,6 +18,7 @@ //! - File deduped issues: `stewardship::process_orchestrator_run` (via //! [`StewardshipIssueFiler`](crate::overseer::observer::StewardshipIssueFiler)). +use std::collections::HashSet; use std::sync::Arc; use std::time::{Duration, Instant}; @@ -319,7 +320,11 @@ pub fn detect_workstream_gaps( anomalies: &[String], coverage: &[String], ) -> Vec { - let is_covered = |sig: &str| coverage.iter().any(|c| c == sig); + // Build the coverage set once for O(1) membership tests. Each of the three + // candidate loops below probes it per item; a linear `coverage.iter().any` + // would be O(candidates x coverage) every observer cycle. + let covered: HashSet<&str> = coverage.iter().map(String::as_str).collect(); + let is_covered = |sig: &str| covered.contains(sig); let mut gaps: Vec = Vec::new(); // 1. Goal board — uncovered high-priority goals. @@ -424,10 +429,11 @@ fn goal_has_active_workstream(goal: &ActiveGoal) -> bool { /// Bound a rendered gap field to [`MAX_GAP_FIELD_LEN`] `char`s (trimmed). fn truncate_field(s: &str) -> String { let s = s.trim(); - if s.chars().count() <= MAX_GAP_FIELD_LEN { - s.to_string() - } else { - s.chars().take(MAX_GAP_FIELD_LEN).collect() + // Short-circuit at the bound: stop scanning at the (MAX_GAP_FIELD_LEN)th + // char instead of counting every char of an arbitrarily long board field. + match s.char_indices().nth(MAX_GAP_FIELD_LEN) { + None => s.to_string(), + Some((byte_idx, _)) => s[..byte_idx].to_string(), } } diff --git a/src/overseer/signal.rs b/src/overseer/signal.rs index 8a4fe1a96..c17574304 100644 --- a/src/overseer/signal.rs +++ b/src/overseer/signal.rs @@ -155,6 +155,20 @@ impl GapCategory { Self::AnomalyUnaddressed => "anomaly", } } + + /// The stable prefix every [`GapItem::signature`] of this category MUST begin + /// with (`goal:` / `issue:` / `anomaly:`). Part of the bounded gap taxonomy: + /// the sensor builds each signature as ``, so a + /// recurring gap of the same kind always collapses onto one dedup key rather + /// than a free-form, per-tick-unstable title. Kept in lock-step with + /// [`GapCategory::label`] so provenance and signature never drift. + pub fn signature_prefix(&self) -> &'static str { + match self { + Self::GoalUncovered => "goal:", + Self::IssueUncovered => "issue:", + Self::AnomalyUnaddressed => "anomaly:", + } + } } /// One genuine backlog-coverage gap: a specific piece of important work that is @@ -180,6 +194,71 @@ pub struct GapItem { pub signature: String, } +/// The single canonical prefix for a workstream-gap dedup key. The per-gap key +/// is `` and the combined per-tick key +/// (`overseer::mod::workstream_gap_key`) is ``, +/// so both filing seams agree on one stable, content-addressed token. +pub const GAP_DEDUP_KEY_PREFIX: &str = "workstream-gap:"; + +/// Upper bound on a gap signature's length, matching the sensor's field bound +/// (`sensor::MAX_GAP_FIELD_LEN` plus the short category prefix). A signature is +/// only ever built from trusted identifiers; this bound guarantees it stays a +/// small, safe token even for exotic ids (IV-1: dedup-search-injection defense). +pub const MAX_GAP_SIGNATURE_LEN: usize = 200; + +impl GapItem { + /// This gap's stable, restart-durable dedup key: `workstream-gap:`. + /// + /// The key is content-addressed on trusted identifiers only, so the SAME + /// uncovered work always yields the SAME key across ticks — and across daemon + /// restarts. The gap-scan notifier collapses a recurring gap onto this key + /// (one operator notification per window) and a downstream filer can search an + /// existing open issue by embedding it, instead of emitting a fresh, + /// free-form-titled issue every detection tick. Centralising the key here (vs. + /// inlining `format!("workstream-gap:{}", sig)` at each seam) stops the filing + /// paths from silently drifting apart. + pub fn dedup_key(&self) -> String { + format!("{}{}", GAP_DEDUP_KEY_PREFIX, self.signature) + } + + /// True when `signature` upholds the construction contract the sensor + /// guarantees: it begins with this gap's [`GapCategory::signature_prefix`] and + /// is a bounded slug in the restricted alphabet (see + /// [`is_bounded_signature_slug`]). The notifier guards on this so a malformed + /// signature can never inflate a notification, an issue body, or a dedup + /// search query — the bounded taxonomy is enforced at the filing seam, not + /// merely assumed. + pub fn has_valid_dedup_signature(&self) -> bool { + self.signature.starts_with(self.category.signature_prefix()) + && is_bounded_signature_slug(&self.signature) + } +} + +/// Validate that a gap signature is a bounded slug: non-empty, at most +/// [`MAX_GAP_SIGNATURE_LEN`] bytes (which equals characters for the ASCII-only +/// alphabet enforced here), beginning with an ASCII alphanumeric, and +/// composed solely of the restricted alphabet `[A-Za-z0-9:_#./-]`. This is the +/// IV-1 dedup-search-injection defense: no whitespace, quoting, or shell/search +/// metacharacter can appear in a key that is later embedded in a `gh` search +/// query or an issue body, regardless of how exotic the source identifier is. +pub fn is_bounded_signature_slug(sig: &str) -> bool { + // `sig.len()` is the UTF-8 byte length. Because the character check below + // admits only ASCII bytes (alphanumerics + the restricted separators), a + // slug that passes this function is pure ASCII, so its byte length equals + // its `char` count — the `MAX_GAP_SIGNATURE_LEN` bound is therefore an exact + // character bound for every *valid* slug. Checking bytes up front also + // cheaply rejects an over-long multi-byte input before the per-char scan. + if sig.is_empty() || sig.len() > MAX_GAP_SIGNATURE_LEN { + return false; + } + // Single pass: an ASCII alphanumeric is allowed anywhere; the restricted + // separators are allowed only after the first char (so the slug must open + // with an alphanumeric, never a separator). + sig.char_indices().all(|(i, c)| { + c.is_ascii_alphanumeric() || (i > 0 && matches!(c, ':' | '_' | '#' | '.' | '/' | '-')) + }) +} + /// Coarse relative importance. `Ord` sorts ascending so `Critical` comes first, /// mirroring `crate::cognitive_threads::Priority`. #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] @@ -965,3 +1044,100 @@ mod describe_tests { ); } } + +#[cfg(test)] +mod gap_dedup_key_tests { + //! Contract for the bounded workstream-gap taxonomy (Problem 2, issue #4687): + //! every gap carries a stable, restart-durable, injection-safe dedup key so a + //! recurring gap collapses onto one key instead of flooding the tracker with + //! near-duplicate, free-form-titled issues on every daemon restart. + use super::*; + + fn gap(category: GapCategory, signature: &str) -> GapItem { + GapItem { + category, + ref_id: "ref".to_string(), + title: "title".to_string(), + why_it_matters: "why".to_string(), + signature: signature.to_string(), + } + } + + #[test] + fn dedup_key_is_the_stable_prefixed_signature() { + let g = gap(GapCategory::GoalUncovered, "goal:g-hot"); + assert_eq!(g.dedup_key(), "workstream-gap:goal:g-hot"); + // The centralised prefix is the single source of truth for both seams. + assert!(g.dedup_key().starts_with(GAP_DEDUP_KEY_PREFIX)); + } + + #[test] + fn identical_uncovered_work_yields_an_identical_key_across_ticks() { + // Content-addressed on trusted identifiers only: a re-detection of the + // same gap (a fresh GapItem, as after a daemon restart) produces the same + // key, so the notifier/filer deduplicates it instead of re-filing. + let first = gap(GapCategory::IssueUncovered, "issue:rysweet/simard#4687"); + let after_restart = gap(GapCategory::IssueUncovered, "issue:rysweet/simard#4687"); + assert_eq!(first.dedup_key(), after_restart.dedup_key()); + } + + #[test] + fn category_prefix_tracks_the_label() { + for (cat, prefix, label) in [ + (GapCategory::GoalUncovered, "goal:", "goal"), + (GapCategory::IssueUncovered, "issue:", "issue"), + (GapCategory::AnomalyUnaddressed, "anomaly:", "anomaly"), + ] { + assert_eq!(cat.signature_prefix(), prefix); + assert_eq!(cat.signature_prefix().trim_end_matches(':'), cat.label()); + assert_eq!(cat.label(), label); + } + } + + #[test] + fn well_formed_gaps_from_every_category_are_valid() { + assert!(gap(GapCategory::GoalUncovered, "goal:g-hot").has_valid_dedup_signature()); + assert!( + gap(GapCategory::IssueUncovered, "issue:rysweet/simard#4687") + .has_valid_dedup_signature() + ); + assert!( + gap( + GapCategory::AnomalyUnaddressed, + "anomaly:distill_parse_fail_rate_high", + ) + .has_valid_dedup_signature() + ); + } + + #[test] + fn a_signature_missing_its_category_prefix_is_rejected() { + // A goal gap whose signature does not start with `goal:` breaks the + // bounded-taxonomy contract and must not be treated as dedupable. + assert!( + !gap(GapCategory::GoalUncovered, "issue:rysweet/simard#1").has_valid_dedup_signature() + ); + } + + #[test] + fn injection_and_unbounded_signatures_are_rejected() { + // IV-1: a signature carrying whitespace / search or shell metacharacters + // (as free-form text would) is not a bounded slug and is rejected before + // it can reach a `gh` search query or an issue body. + assert!(!is_bounded_signature_slug("")); + assert!(!is_bounded_signature_slug("goal:has space")); + assert!(!is_bounded_signature_slug("goal:\"quoted\"")); + assert!(!is_bounded_signature_slug("goal:a`b")); + assert!(!is_bounded_signature_slug(":leading-colon")); + assert!(!is_bounded_signature_slug(&format!( + "goal:{}", + "x".repeat(MAX_GAP_SIGNATURE_LEN) + ))); + // The legitimate trusted-slug shapes stay valid. + assert!(is_bounded_signature_slug("goal:g-hot")); + assert!(is_bounded_signature_slug("issue:rysweet/simard#4687")); + assert!(is_bounded_signature_slug( + "anomaly:distill_parse_fail_rate_high" + )); + } +} diff --git a/src/stewardship/gh_client.rs b/src/stewardship/gh_client.rs index a6cbda99f..5099a5e84 100644 --- a/src/stewardship/gh_client.rs +++ b/src/stewardship/gh_client.rs @@ -487,6 +487,95 @@ mod tests { ); } + /// A stateful `gh issue list` model of GitHub's eventually-consistent issue + /// search: filed issues are visible to the strongly-consistent + /// [`IssueListQuery::RecentOpen`] scan *immediately*, but the full-text + /// [`IssueListQuery::Signature`] index lags for the entire observation window + /// (it never surfaces the just-filed issue). This is the exact condition that + /// produced the 28-near-duplicate stewardship issue storm (rysweet/Simard + /// #4671-#4686): every sweep within the multi-minute index window saw an + /// empty signature search. + struct IndexLagList { + /// Issues already filed (what a strongly-consistent scan would return). + recent: RefCell>, + signature_queries: Cell, + recent_queries: Cell, + } + + impl IndexLagList { + fn new() -> Self { + Self { + recent: RefCell::new(Vec::new()), + signature_queries: Cell::new(0), + recent_queries: Cell::new(0), + } + } + fn run(&self, query: &IssueListQuery) -> Result, SimardError> { + match query { + // The search index lags for the whole window: always empty. + IssueListQuery::Signature(_) => { + self.signature_queries.set(self.signature_queries.get() + 1); + Ok(Vec::new()) + } + // Strongly consistent: returns everything filed so far. + IssueListQuery::RecentOpen(_) => { + self.recent_queries.set(self.recent_queries.get() + 1); + Ok(self.recent.borrow().clone()) + } + } + } + fn file(&self, issue: GhIssue) { + self.recent.borrow_mut().push(issue); + } + } + + /// Regression for the stewardship duplicate-issue *storm* (rysweet/Simard + /// #4671-#4686): many detection sweeps of the SAME failure within GitHub's + /// search-index window must collapse to exactly ONE filed issue, not one per + /// sweep. This drives the real [`resolve_dedup_candidates`] dedup — mirroring + /// the file-if-no-match decision `process_orchestrator_run` makes — across a + /// window in which the full-text index never catches up. The + /// strongly-consistent recent-open scan is what keeps the count at one; if + /// that fallback were removed each sweep would see an empty search and file + /// afresh, reproducing the flood. + #[test] + fn resolve_collapses_a_full_window_of_sweeps_to_a_single_filed_issue() { + let sig = "cafef00dcafef00d"; + let gh = IndexLagList::new(); + let sweeps = 12; + let mut filed = 0u64; + + for _ in 0..sweeps { + let candidates = resolve_dedup_candidates(|q| gh.run(q), sig).unwrap(); + if super::find_existing(&candidates, sig).is_none() { + // No open issue carries this signature yet — file exactly one. + filed += 1; + gh.file(issue(4671, sig)); + } + } + + assert_eq!( + filed, 1, + "a full index-lag window of {sweeps} identical detections must file ONE issue, \ + not one per sweep (the #4671-#4686 storm)" + ); + assert_eq!( + gh.recent.borrow().len(), + 1, + "exactly one tracking issue exists after the whole window" + ); + assert_eq!( + gh.signature_queries.get(), + sweeps, + "every sweep still runs the fast signature search first" + ); + assert_eq!( + gh.recent_queries.get(), + sweeps, + "every empty search must fall back to the strongly-consistent recent scan" + ); + } + #[test] fn resolve_returns_no_match_when_neither_query_has_signature() { let sig = "cafef00dcafef00d"; diff --git a/src/stewardship/tests_extra.rs b/src/stewardship/tests_extra.rs index ab09c2411..1930292b2 100644 --- a/src/stewardship/tests_extra.rs +++ b/src/stewardship/tests_extra.rs @@ -362,6 +362,106 @@ fn process_run_routes_overseer_to_default_and_dedups() { ); } +// ─────────────────────────── Storm regression (multi-sweep dedup) ─────────── + +/// A stateful `GhClient` that models the daemon's real dedup surface once the +/// search-index-lag fallback is in play: a filed tracking issue is visible to a +/// subsequent `search_issues` call (as the strongly-consistent recent-open scan +/// makes it), so re-observing the same failure matches instead of re-filing. +/// Unlike the seeded [`FakeGhClient`], this one accumulates created issues and +/// answers searches from that live store — the shape a full observation window +/// actually sees. +#[derive(Default)] +struct StatefulGhClient { + issues: Mutex>, + create_calls: Mutex, + next_number: Mutex, +} + +impl StatefulGhClient { + fn new() -> Self { + Self { + issues: Mutex::new(Vec::new()), + create_calls: Mutex::new(0), + next_number: Mutex::new(4671), + } + } + fn create_call_count(&self) -> usize { + *self.create_calls.lock().unwrap() + } + fn open_issue_count(&self) -> usize { + self.issues.lock().unwrap().len() + } +} + +impl GhClient for StatefulGhClient { + fn search_issues(&self, _repo: &str, signature: &str) -> Result, SimardError> { + let needle = format!("stewardship-signature: {signature}"); + Ok(self + .issues + .lock() + .unwrap() + .iter() + .filter(|i| i.body.contains(&needle)) + .cloned() + .collect()) + } + fn create_issue(&self, repo: &str, title: &str, body: &str) -> Result { + *self.create_calls.lock().unwrap() += 1; + let mut num = self.next_number.lock().unwrap(); + let issue = GhIssue { + number: *num, + url: format!("https://github.com/{repo}/issues/{num}"), + title: title.to_string(), + body: body.to_string(), + }; + *num += 1; + self.issues.lock().unwrap().push(issue.clone()); + Ok(issue) + } +} + +/// End-to-end regression for the stewardship issue *storm* (rysweet/Simard +/// #4671-#4686): a whole observation window of identical detections routed +/// through [`process_orchestrator_run`] must produce exactly ONE open issue — +/// the first sweep files, every later sweep dedups to `MatchedExisting`. This is +/// the public-API expression of the "duplicate detections within a window +/// produce at most one open issue" contract; it would fail the moment the +/// file-if-no-match dedup guard regressed and the loop filed per-tick. +#[test] +fn process_run_files_one_issue_across_a_full_window_of_identical_detections() { + let gh = StatefulGhClient::new(); + let run = sample_run(); + + let mut filed_new = 0usize; + let mut matched_existing = 0usize; + for _ in 0..15 { + match process_orchestrator_run(&run, &gh).unwrap() { + StewardshipOutcome::FiledNew { .. } => filed_new += 1, + StewardshipOutcome::MatchedExisting { .. } => matched_existing += 1, + } + } + + assert_eq!( + filed_new, 1, + "only the first sweep files a new tracking issue" + ); + assert_eq!( + matched_existing, 14, + "every subsequent sweep dedups to the existing issue" + ); + assert_eq!( + gh.create_call_count(), + 1, + "exactly one `create_issue` across the whole window — no duplicate flood" + ); + assert_eq!( + gh.open_issue_count(), + 1, + "exactly one open tracking issue remains after the window" + ); +} + // ─────────────────────────── Input validation ─────────────────────────── #[test]