fix(overseer): durable GitHub-side dedup for gap-scan issue filing (#4717)

[rysweet]

Problem

The Overseer's stewardship gap-filing had no durable dedup. The in-process WhisperGate resets to empty on process boot, so a recurring backlog-coverage gap was re-filed on every restart — the observed bursts of near-identical [stewardship] workstream_gap:* issues (e.g. #4726–#4730 filed within 6 seconds). Issue #4717 scoped the fix; duplicates filed after it proved it had not yet landed.

Fix

Durable GitHub-side open-issue dedup, keyed on a stable per-gap signature, run before any gh issue create:

• GapItem::dedup_key() — stable, shell-safe workstream-gap:<signature> marker (sanitized to a slug even for a hostile signature).
• Overseer::with_gap_issue_client(Box<dyn GhClient>) — wires the durable gh seam; unwired keeps the prior notify-only behavior.
• act_flag_workstream_gaps — queries GitHub for an OPEN issue carrying the gap's marker and skips creation on a match (survives a restart); files one deduped issue per genuinely-new gap with the marker on its own body line; fails loud (creates nothing) if the gh search errors. The in-process gate still pre-filters same-cycle bursts before any gh call.
• Line-bounded match (open_issue_has_marker) — never a bare substring, so a prefix-colliding longer key can't swallow a genuinely-new gap.
• ActOutcome::WorkstreamGapsFlagged gains reused_existing, threaded into the tick report / activity totals (workstream_gaps_reused_existing) and the daemon tick log.
• Daemon wires RealGhClient so the durable check lands in production.

Additive / non-breaking; PRD preserved; no Bridge naming; structured tracing + OTel only (no stray print!/println!).

Tests

New hermetic tests (fake gh seam, no network) cover: file-once, durable-skip, restart durability, prefix-collision, fail-closed on search error, and in-process same-cycle burst suppression. All 34 gap-scan + 727 overseer + 151 stewardship lib tests pass; cargo fmt, release clippy -D warnings, and the full --all-targets --all-features --locked clippy gate pass.

Docs: new reference + how-to pages, wired into mkdocs.yml nav and cross-linked.

Closes #4717

[github-actions]

📊 Coverage Summary

Generated by cargo llvm-cov --workspace --summary-only (nightly, excluding test files)

Module	Lines	Covered	Coverage
Total	199035	167638	84.2%

_{Coverage data from CI run. Test files matching tests?/ are excluded from line counts.}

[rysweet]

Comprehensive Code Review — durable GitHub-side gap-scan dedup (#4717)

Scope reviewed: src/overseer/mod.rs, src/overseer/signal.rs, src/overseer/wiring.rs, src/overseer/activity.rs, src/operator_commands_ooda/daemon/mod.rs, src/overseer/tests_gap_scan.rs, docs.
Status at review: mergeStateStatus=CLEAN, mergeable=MERGEABLE, all required checks green (coverage, pre-commit, cargo-audit/deny/vet, e2e-dashboard, install-real).

Verdict: Approve with minor, non-blocking comments. This is a well-factored, additive, non-breaking implementation of requirement #5 (overseer stewardship flooding). Logic is correct, the seam is opt-in, and test coverage of the durable-dedup contract is thorough.

Checklist

• Code quality & standards — idiomatic seam (Option<Box<dyn GhClient>>), None preserves prior notify-only behavior.
• Test coverage adequate — hermetic FakeGhClient covers fresh-file, durable skip (reused_existing), and the fail-loud search path.
• No TODOs / stubs / swallowed exceptions — errors are surfaced via OverseerError::Capability, not swallowed.
• No unimplemented functions.
• Logic correctness — see notes below; core paths correct.
• Edge-case handling — line-bounded marker match + shell-safe slug are strong touches.
• Structured tracing only (no print!/println! in new code).

Strengths

1. Line-bounded marker match (open_issue_has_marker): matching a whole stewardship-signature: <key> line rather than body.contains(...) correctly avoids a longer key (…g-hot-extra) being swallowed by a shorter prefix (…g-hot). Genuinely important correctness detail.
2. Shell-safe slug (sanitize_signature_slug): defense-in-depth so the key can never break an argv or spill onto a second body line — satisfies the injection-guard intent.
3. Dedicated reused_existing counter threaded cleanly through ActOutcome → OverseerTickReport → OverseerTotals → daemon log line: good, non-overlapping observability distinct from the in-window suppressed.

Findings / discussion

F1 — Fail-closed on search error diverges from the fail-open-notify intent (MEDIUM, discussion).
On a gh search failure the Act path propagates OverseerError via ? before notifier.notify(...), so for that tick the operator gets neither a filed issue nor the consolidated notification for those gaps. The fail-loud-never-blind-create choice is correct and prevents the #4717 burst regression, but the session requirement (A6) asked that dedup faults "degrade to notifying (fail-open) so genuine new gaps are never silently dropped." Consider a hybrid that satisfies both: on a search error, skip durable dedup + create but still send the operator notification. As written the gap is deferred (loudly, and retried next sweep — not permanently dropped), so this is defensible, but the trade-off between #4717 (fail-closed create) and A6 (fail-open notify) should be a conscious, documented decision.

F2 — Two derivations of the "same" key (LOW / nit).
Durable dedup keys on GapItem::dedup_key() = workstream-gap:<sanitize_signature_slug(signature)>, but the in-process gate commits/checks on format!("workstream-gap:{}", g.signature) (raw signature). The detector guarantees slug-safe signatures today so they coincide, but the two paths compute the key two different ways. Suggest keying the gate off dedup_key() too (single source of truth) to prevent future drift.

F3 — Create-loop partial failure (LOW).
The consolidated notification is sent for all of to_file, then issues are created in a loop where create_issue(...)? returns on the k-th failure, and gap_gate.commit runs only after the loop. On a mid-loop create failure: the operator was already notified for gaps that did not file, and no gate entries are committed. Cross-cycle correctness is preserved by the durable GitHub dedup (already-created issues are found and skipped on the next sweep), so this is minor — but consider committing the gate per-gap as each issue is filed.

F4 — Two parallel "search open issues by signature" implementations (design note).
This PR adds durable dedup via GhClient::search_issues + open_issue_has_marker, while stewardship::dedup::find_existing already implements open-issue-by-signature dedup used elsewhere. Functionally equivalent. To avoid drift, consider consolidating the gap-scan path onto stewardship::dedup::find_existing (or documenting why a separate marker-line scheme is intentional here).

None of F1–F4 block merge. F1 is worth a one-line reply confirming the fail-closed-create / notify trade-off is intentional.

[rysweet]

🔒 Step 17c — Security Review (mandatory gate)

Read-only security review focused on exploitable vulnerabilities in the dedup implementation. Verdict: Safe to merge with one recommended hardening (Finding 1, MEDIUM, non-blocking).

Architectural fact that neutralizes most vectors

gh is invoked via Command::new(...).args(...) — a direct execvp, no shell — and the issue body is streamed over stdin (--body-file -). No sh -c, no string-built command line. This eliminates the classic shell-injection surface.

Category results

#	Category	Result
1	Command / shell injection	✅ No finding — no shell spawned; the "shell-safe slug" is redundant defense-in-depth
2	gh argument injection (leading -/--)	✅ No finding — untrusted values are flag-values; --search value is prefixed with constant stewardship-signature:
3	GitHub search-query injection	✅ No finding — slug strips all whitespace [A-Za-z0-9:._/#-]; reuse decision is exact per-line equality
4	Marker spoofing into stored markdown	⚠️ Finding 1 (MEDIUM)
5	Sensitive-data leak	✅ No finding — tracing logs key/number/url only; body never logged; token never logged
6	Authorization / issue hijack / TOCTOU	✅ No finding — reuse path only increments a counter; never writes to the matched issue
7	Fail-open vs fail-closed	✅ Search error propagates via ? → fail-closed (safe direction)
8	ReDoS	✅ No finding — lines() + trim() == needle, linear, no regex

Finding 1 — Untrusted gap fields not newline-sanitized adjacent to the dedup marker (marker spoofing / dedup evasion)

Severity: MEDIUM · Confidence: 7/10
Where: src/overseer/mod.rs body construction (~L989-998) + src/overseer/sensor.rs truncate_field (~L424-432) + open_issue_has_marker (~L1051-1056)

The dedup key is slugified to a single line, but the body interpolates title / ref_id / why_it_matters raw on lines adjacent to the stewardship-signature: {key} marker. truncate_field applies only .trim() + a 120-char cap and preserves interior newlines. A field carrying an embedded newline, e.g.:

real description\nstewardship-signature: workstream-gap:goal:<victim-goal-id>

plants a second forged marker line for an unrelated key. Since open_issue_has_marker matches any body line equal to the marker, the overseer will later reuse the poisoned issue and skip filing the legitimate tracking issue — a durable dedup-evasion/suppression that survives restarts.

Impact is bounded: integrity/availability of the overseer's own auto-filed issues only. The reuse path performs no write to the matched issue, so there is no RCE, no data breach, and no unauthorized-write/issue-hijack escalation. Exploitability depends on whether an external actor can inject a newline into a goal description or telemetry-anomaly string (not confirmed here → confidence 7).

Minimal fix (non-blocking): strip newlines/control chars from the three interpolated fields in truncate_field (or at body-assembly time), or restructure the marker so free-text body content cannot forge it (e.g., dedicated label / fenced structured block).

Checklist

• Security requirements verified — no shell, stdin body, hardcoded repo
• New vulnerabilities checked — 1 MEDIUM (marker spoofing), non-blocking
• Sensitive data handling confirmed — no secret/token/body leakage in logs
• AuthZ reviewed — reuse path is read-only w.r.t. matched issues; no hijack
• Injection review — command/arg/search-query injection all ✅ no finding

No merge-blocking security issues. Recommend addressing Finding 1 as a fast follow (newline-strip the interpolated body fields).

[rysweet]

🛡️ Philosophy Guardian Review (Step 17d)

Compliance assessment of the durable gap-scan dedup seam (issue #4717) against the amplihack philosophy. Verdict: PASS — non-blocking notes only.

Compliance checklist

• Ruthless simplicity achieved — The Act path is a linear read: query GitHub → skip on marker match → otherwise notify + file. Helpers (dedup_key, sanitize_signature_slug, open_issue_has_marker) are small, pure, single-purpose functions. No speculative machinery.
• Bricks & studs pattern followed — The seam is a clean brick: the Overseer depends on the GhClient trait (the stud), never a concrete client. with_gap_issue_client(...) is the single wiring point; the contract is explicit and swappable (the hermetic tests inject a fake through the same stud).
• Zero-BS implementation — No stubs, no faked APIs, no swallowed exceptions. The opposite of swallowing: a gh search failure fails loud (map_err → OverseerError::Capability) and files nothing, and create_issue errors propagate identically. No unwrap() on external I/O.
• No over-engineering — The seam is an Option<Box<dyn GhClient>> that is None by default and degrades to the exact prior notify-only behavior. A dedicated reused_existing counter (vs. overloading issues_filed) is warranted, not gold-plating. open_issue_has_marker is a line-bounded match — deliberately not a broader substring engine.
• Clean module boundaries — Responsibilities are well-partitioned: signal.rs owns GapItem + dedup_key; overseer/mod.rs owns orchestration + the marker match; the GhClient trait owns GitHub access; activity.rs/daemon own the counter surface. No cross-layer leakage.

Non-blocking philosophy notes

1. Single source of truth for the dedup key (DRY). The file gate commits format!("workstream-gap:{}", g.signature) (raw signature) while the durable lookup uses g.dedup_key() (sanitized workstream-gap:<slug>). Two derivations of the "same" identity is a latent divergence risk. Consider routing both through dedup_key() so the in-process gate and the GitHub-side marker can never drift. (Mirrors code-review finding F2.)
2. Repeated gate-commit loop. for g in &fresh { self.gap_gate.commit(...) } appears in the all-reused branch and the file branch. A single post-decision commit point would remove the duplication and the risk of one branch being updated without the other.

Both notes are quality/maintainability observations, not philosophy violations. The change is additive, reversible, and honest about failure — it aligns with ruthless simplicity and zero-BS. Approved on philosophy grounds.