fix(ooda,overseer): terminal-quarantine breaker churn + single-open auto-doc PR reconciliation (Problems #1/#5) - #4964
Conversation
Automatic checkpoint to preserve work in progress. Tests and implementation saved before refactoring phase.
rysweet
left a comment
rysweet
left a comment
There was a problem hiding this comment.
Step 17b — Comprehensive Code Review
Scope reviewed: implementation commit f78c26cc0 (Problems #1 & #5). 23 files, +3169/−109. Docs + tests + source.
Verification run during review:
cargo check --tests→ clean (exit 0).- Targeted tests all green: quarantine churn (36),
doc_pr_reconcile(15), reblock signature (8).
Overall this is high-quality, well-documented, philosophy-aligned work: pure/IO separation, fail-closed gating, injection-safe durable markers, no print!/println!, no Bridge naming, additive/non-breaking. One HIGH-severity correctness bug makes Problem #5 inert in production, plus a few medium/minor items below.
🔴 HIGH — Problem #5 identity gate pins the WRONG author ⇒ reconciliation is a silent no-op in production
src/overseer/doc_pr_reconcile.rs:35 sets AUTO_DOC_PR_AUTHOR = DEFAULT_OVERSEER_AUTHOR_LOGIN ("simard-overseer[bot]"), and is_auto_doc_pr (line 104) requires pr.author == AUTO_DOC_PR_AUTHOR.
But the real "Update documentation with …" PRs are not authored by the overseer bot. Verified live against this repo:
The codebase’s own config invariant confirms this is the wrong identity: src/overseer/config.rs:510-516 documents that the engineer/goal-advance author (SIMARD_AUTOMERGE_AUTHOR) is DISTINCT from OVERSEER_AUTHOR_LOGIN (the overseer-bot recursion identity) — different logins by design. The auto-doc PRs carry SIMARD_ENGINEER_PR_LABEL (simard-autonomous), which config.rs:574 says is "stamped on EVERY engineer / goal-advance PR" — i.e. they are engineer-authored, never overseer-bot-authored.
Impact: is_auto_doc_pr returns false for every real auto-doc PR, so run_doc_pr_reconcile finds zero candidates and closes nothing. Problem #5 does not actually fix the stale-PR churn in production.
Why tests don’t catch it (test-masks-bug): tests_doc_pr_reconcile.rs:50 builds its auto-doc fixture with author: AUTO_DOC_PR_AUTHOR.to_string() — the fixture is bound to the same constant under test, so no test ever exercises the real-world author. The 15 green tests are internally consistent but validate nothing about the production identity.
Recommended fix (pick one):
- Resolve the author from the configured engineer identity — reuse
config::automerge_author()(envSIMARD_AUTOMERGE_AUTHOR) rather than hardcoding the overseer-bot default; when unset, fall back to the compositetitle-marker + simard-autonomous label + draftgate, which already fail-closes against human PRs (a human PR won’t carrysimard-autonomous). - Or drop the author-equality clause entirely and rely on
title marker + SIMARD_ENGINEER_PR_LABEL + is_draft==Some(true). The label alone is the codebase’s own "this is Simard’s own PR" signal, so it is both correct and sufficient; the current author pin is redundant and actively breaks the gate.
🟠 MEDIUM — Inert-gate failure is silent (no LOUD signal)
Because the pass fail-closes to "0 candidates" and only emits tracing::info! with candidates=0, an identity-gate mismatch (the HIGH finding) produces no warning — exactly why this shipped unnoticed. Per TRUST/zero-BS, a mismatch should be LOUD. Recommend: if open_prs contains ≥1 PR whose title starts with AUTO_DOC_PR_TITLE_MARKER but zero pass is_auto_doc_pr, emit a tracing::warn! ("N title-marker PRs present but none passed the identity gate — reconciliation inert, check author/label config") and add a test asserting that warn path. This turns a silent no-op into an observable defect.
🟠 MEDIUM (verify) — fold_volatile_goal_ids shapes must match real dedup keys
fold_volatile_goal_ids (observer.rs:213) only folds two shapes: simard-identity-<slug> and positional goal-<n>. The dedup relies on the actual production Problem.dedup_key embedding exactly one of these. Tests assert against hand-written shapes ("recurring_goal_reblock simard-identity-…", "…goal-12") rather than a captured real key. Please confirm against a real recurring_goal_reblock dedup_key that the volatile portion matches one of the two folded forms; if the real key embeds the goal id in any other shape (e.g. a UUID, goal_id=<x>, or a ::-joined signature), the fold silently no-ops and dedup does not happen. Add one test built from a captured production key to pin this.
🟡 LOW — Quarantine bound is off-by-one vs. the prior escalate timing
resolution_for_why (no_progress_breaker.rs:657) checks the pre-bump surfaced_failures >= LIMIT, while the SurfaceInvestigationFailure arm bumps the counter afterward. Net effect with LIMIT=3: the goal surfaces 3 times, then quarantines on the 4th terminal pass — whereas the old inline branch escalated on the 3rd. One extra surfaced cycle before terminal action. Likely intentional (and surfaced_count renders as exactly LIMIT), but the docstring says "once the surfaced-failure bound is reached," which reads as fire-at-3. Please either adjust to fire when surfaced_failures + 1 >= LIMIT or tighten the docstring to state the pre-bump semantics, and add a test that pins the exact cycle count at which quarantine fires.
🟡 LOW (style) — let _ = class;
no_progress.rs:1335: the SurfaceInvestigationFailure { class, reason } arm now discards class via let _ = class;. Prefer binding it out in the pattern (SurfaceInvestigationFailure { class: _, reason }) so the discard is expressed at the match site rather than as a statement.
🟡 LOW — close_pr default no-op can hide a mis-wired client
merge_authority.rs: PrGhClient::close_pr defaults to Ok(()) (no-op). The rationale (hygiene, not a correctness gate) is sound and documented, but combined with the HIGH finding it means a client that silently can’t close leaves duplicates open with no signal. The MEDIUM telemetry suggestion above also covers this. No change strictly required.
✅ Strengths (Problem #1)
QuarantineTerminalcorrectly classified terminal byis_terminal(); the quarantined-goal short-circuit inreinvestigate_bare_blocked_goalsruns before the reasoner, and the churn test proves it via aPanicReasoner.- Injection-safe marker: fixed compile-time
ref_idsentinel +kind, never goal-derived;is_quarantine_refkeys on both. Idempotent push (!is_quarantined(g)guard) prevents marker accretion. - Never authors a bare
evidence=[(none)]block — the surfaced count is rendered as real evidence.
Checklist
- Code quality and standards — strong; no
print!/println!, noBridgenaming. - Test coverage — broad and green, but the Problem #5 tests mask the HIGH author bug (fixtures bound to the constant under test).
- No TODOs, stubs, or swallowed exceptions — none found; per-close errors are collected and logged, not swallowed.
- No unimplemented functions — none.
- Logic correctness — HIGH bug: Problem #5 gate is inert in production.
- Edge-case handling — fail-closed on empty author,
is_draft None, list errors, zero/one candidate; bounded closes; canonical never closed.
Verdict: REQUEST CHANGES — fix the HIGH author-gate bug (and add the LOUD-inert telemetry + a real-key reblock test) before merge. Everything else is minor.
rysweet
left a comment
rysweet
left a comment
There was a problem hiding this comment.
Step 17c — Security Review (MANDATORY)
Scope: implementation commit f78c26cc0 (Problems #1 & #5). Source files only (docs excluded). Read-only analysis performed via the security-review specialist.
Security checklist
- Verify all security requirements met — met
- Check for any new vulnerabilities — none found
- Confirm sensitive data handling — no secrets logged/exposed
- Review authentication/authorization — fail-closed identity gate, no priv-esc
- Check for injection vulnerabilities — none (argv-only
ghexec, no shell)
Findings by category
1. Injection / untrusted-input (doc_pr_reconcile.rs) — ✅ CLEAN.
The only mutating command is RealPrGhClient::close_pr (merge_authority.rs:507-514) → run_gh_checked (:337-354), which uses std::process::Command::new("gh").args(args) — positional argv, no shell. The format!("gh pr close …") at :509 is a log label only, not executed. Attacker-influenceable strings (PR title, labels, branch names) are never interpolated into any command, path, or shell. The close comment (doc_pr_reconcile.rs:136-147) interpolates only the canonical PR number (u32). The close set is computed purely from structured OpenPrSummary fields and executed by pr.number (:197), so a crafted title/label cannot redirect a close to the wrong PR.
2. Authorization / identity confusion — ✅ CLEAN.
is_auto_doc_pr (doc_pr_reconcile.rs:101-107) is a fail-closed AND of four signals: title prefix + author == "simard-overseer[bot]" + is_draft == Some(true) + label "simard-autonomous". An external attacker cannot spoof author.login (GitHub-authoritative) nor apply the simard-autonomous label without repo triage/write. Empty/absent author fails closed (:103). A human/attacker PR can never become a candidate, be closed, or be selected as canonical. merge_authority.rs only adds fields + close_pr; no privilege boundary crossed.
3. Destructive-action safety — ✅ CLEAN.
Canonical = candidates…map(number).max() (:119), explicitly excluded from the close set (:128). 0/1 candidates ⇒ zero closes. Deterministic; provably cannot close all candidates nor a non-candidate (human) PR. Bounded: MAX_CLOSES_PER_CYCLE = 25 (:49), DOC_PR_LIST_LIMIT = 200 (:44). Triple runtime gate (overseer/mod.rs:1016, :709-745): wired gh client + gap_scan_enabled + non-empty roster + every-N cadence; disabled by default (None, :577). Per-repo list failure fails closed (no closes); per-close failure is contained.
4. Quarantine-marker integrity (no_progress_breaker.rs/no_progress.rs) — ✅ CLEAN.
Marker key is a fixed compile-time sentinel: NO_PROGRESS_QUARANTINE_MARKER_KIND/_REF_ID (no_progress_breaker.rs:346,354). quarantine_marker() (:361-368) never derives kind/ref_id from goal text; is_quarantine_ref (:377-380) matches BOTH fixed constants. Goal text/evidence/why-strings cannot forge or collide a marker. wip_refs is populated only by internal code + local board JSON, never attacker-crafted issue/PR bodies. Neither denial-of-progress nor quarantine evasion is reachable. Marker write is idempotent (no_progress.rs:1400-1408, guarded by !is_quarantined(g)) — no unbounded accretion.
5. Command execution / secrets / unsafe — ✅ CLEAN.
No unsafe blocks, no sh -c, only the one argv-based gh pr close. No new network calls beyond gh (self-authenticating). No tokens/PR bodies/credentials logged — tracing emits only repo, PR numbers, counts, reasons, error strings.
6. DoS / resource — ✅ bounded. 200-PR list window, 25 closes/cycle, idempotent single marker.
⚠️ Security-adjacent guidance (forward-looking, not a defect)
The known functional gap (author gate on simard-overseer[bot] may match zero real auto-doc PRs) is currently fail-safe: it can only cause under-action (nothing closed), never wrongful closes. Security caveat for the fix: do NOT remediate by loosening the gate to title-marker + label only. Dropping the exact-author conjunct (doc_pr_reconcile.rs:104) would collapse safety onto the simard-autonomous label alone — anyone with repo triage/write could then apply that label to a crafted draft PR titled "Update documentation with…" and get arbitrary PRs auto-closed. Keep the author-identity check as a mandatory conjunct in any future relaxation (resolve the correct author via config::automerge_author() rather than removing the check).
Security verdict: ✅ PASS
No exploitable security issues found across all six categories: no injection (argv-only exec), no spoofable-identity path (fail-closed 4-signal gate), no destructive logic error (canonical=max, bounded batch, triple gate + fail-closed reads), no marker forgery/collision (fixed sentinels), no secret exposure, no unsafe.
|
🟢 SECURITY (injection) — supplementary evidence: |
rysweet
left a comment
rysweet
left a comment
There was a problem hiding this comment.
Step 17b — Comprehensive Code Review
Reviewed all 23 files on review/ooda-churn-autodoc-step17b-1785249692 (base main, +3169/-109). Findings verified against the live repo state via gh pr list, not just the diff.
Verdict: Request changes. Problem #1 (terminal quarantine) is solid and well-tested. Problem #5 (auto-doc PR reconciliation), as written, will not act on any real PR — the identity gate's author constant does not match the actual author of the live auto-doc PRs, and the test suite masks this by hardcoding the expected author.
🔴 HIGH — Problem #5 identity gate is inert against production data
is_auto_doc_pr (src/overseer/doc_pr_reconcile.rs) requires:
pr.author == AUTO_DOC_PR_AUTHOR // = DEFAULT_OVERSEER_AUTHOR_LOGIN = "simard-overseer[bot]"But every live auto-doc PR is authored by rysweet, not the bot. Verified:
$ gh pr list --search "Update documentation with in:title" --json number,author,isDraft,labels,mergeable
4826 author=rysweet draft=True mergeable=MERGEABLE labels=[simard-autonomous]
4784 author=rysweet draft=True mergeable=CONFLICTING labels=[simard-autonomous]
4612 author=rysweet draft=True mergeable=CONFLICTING labels=[]
4528 author=rysweet draft=False mergeable=CONFLICTING labels=[]
... (19 candidates total, all author=rysweet)
Consequences of the 4-way AND gate against this real data:
- Author conjunct fails for all 19 →
reconcile_doc_prsfinds zero candidates → selects no canonical → closes nothing. The single-open invariant is never enforced in production; Problem #5 is a no-op. - Even if author matched: many historical drafts carry no
simard-autonomouslabel (#4612, #4567, #4528, #4490, …) and some areis_draft=false(#4528, #4457). The gate would still exclude exactly the accumulated backlog the module docstring claims to "drain over several cycles."
The PR summary under-states this as "one HIGH-severity correctness issue on the identity gate." Against live data it means the feature does nothing.
Fix options:
- Match
pr.authoragainst the identity that actually opens these PRs (whateveroverseer_author_login()/ the publish PAT resolves to — hererysweet), not a hard-coded bot login. - Relax the label conjunct to a non-gating signal (title-marker + author + draft is already a strong positive ID; treat missing label as still-a-candidate), or key on the auto-doc branch pattern.
- Add a fixture built from a real
gh pr listrow (authorrysweet, no label) so the suite fails-closed against production shapes.
🔴 HIGH (tied) — Test blind spot hides the above
tests_doc_pr_reconcile.rs::auto_doc_pr fabricates candidates with author: AUTO_DOC_PR_AUTHOR and labels: vec![AUTO_DOC_PR_LABEL]. The suite is green precisely because it never exercises the production author/label combination. All 15 doc-PR tests pass while the feature matches nothing live. Add a production-shaped fixture (see fix #3 above).
🟡 MEDIUM — Off-by-one shift in the quarantine trigger
Old code escalated when the post-bump surfaced count reached SURFACED_INVESTIGATION_FAILURE_LIMIT (3) — i.e. on the 3rd surfaced failure. New code routes on the pre-bump count (tracker.surfaced_failures(goal_id) read before the surface arm calls record_surfaced_failure), so QuarantineTerminal fires only once the stored count is already ≥3 — i.e. on the 4th surfaced failure. Net: one extra surface → re-block → (dedup-suppressed) cycle before terminal quarantine.
Not a correctness bug (the suppression marker prevents a duplicate issue on the extra cycle), but it moves the terminal trigger by one cycle and slightly weakens the churn-reduction intent. The docstring's "byte-for-byte unchanged below the bound" is true for the surface arm but not for the trigger point. Either route on surfaced_failures + 1 >= LIMIT to preserve the original point, or document the shift.
🟢 LOW — reblock fold coverage is intentionally narrow (confirm against live signature)
fold_volatile_goal_ids folds only simard-identity-<slug> (confirmed a real shape — e.g. simard-identity-atelier-industrial-furniture-de) and positional goal-<digits>. Any reblock dedup_key embedding a volatile id in another shape (bare hex board id, overseer-goal-<ts> slugs seen in meeting_ops.rs) would not fold and could still churn. Per requirement R1, confirm the production reblock dedup_key behind signature cfa5358a3b59894c actually uses one of the two folded shapes; otherwise the reblock half is also inert. The design is appropriately conservative — this is a verification ask, not a code defect.
🟢 LOW — mergeable == "CONFLICTING" ignores transient UNKNOWN
CloseReason classification treats a still-computing UNKNOWN PR as SupersededDuplicate. Harmless (both reasons close, and a duplicate should close regardless), so cosmetic only — noting for completeness.
✅ Strengths (verified)
- Breaker core stays pure (R4 satisfied).
quarantine_marker/is_quarantine_refkey on a fixed compile-time sentinelref_id, never goal-derived — a goal cannot forge its own quarantine (injection-safe); tests pin this. - "Exactly one issue" dedup is sound.
escalate_with_tracking_issueblocks then short-circuits on an existing suppression marker that lives on the goal board (survives restart); the quarantine arm pushes the marker idempotently behind!is_quarantined(g). - Churn short-circuit proven.
reinvestigate_bare_blocked_goalsskips quarantined goals before consulting the reasoner, andtests_quarantine_churn.rsuses aPanicReasonerto prove the reasoner is never reached — a strong regression guard. - Reconcile is fail-closed and injection-safe.
close_prdefault is a no-op;RealPrGhClientshells argv-only (no shell interpolation). The pass is gated (client wired + gap-scan opt-in + non-empty roster + every-N cadence),max()can never close all candidates, and closes are bounded byMAX_CLOSES_PER_CYCLE. Per-close errors are collected into the report, not silently swallowed. - No new TODOs / stubs /
println!/ swallowed errors in production code; logging istracing-only (R5 satisfied). Remainingunwrap!/panic!/eprintln!are all in test code or pre-existing.
Checklist
- Code quality and standards — good; pure/IO split is clean
- Test coverage — adequate for Problem #1; blind spot for Problem #5 (see HIGH)
- No TODOs, stubs, or swallowed exceptions in new production code
- No unimplemented functions
- Logic correctness — Problem #5 gate does not match production data (HIGH, blocking)
- Edge case handling — quarantine idempotence, canonical never-closed, restart survival all covered
Bottom line: merge-blocking on the Problem #5 identity gate. Land the quarantine half; fix the auto-doc author gate (and add a production-shaped fixture) before Problem #5 can be considered delivered.
Step 17c — Security Review (read-only specialist)Security-focused pass over all 23 changed files on ✅ Verdict: SECURITY-CLEAN — no Critical/High findings, non-blocking.
Notes
Bottom line: No security changes required to merge. Subprocess execution is injection-safe (argv-only), the destructive auto-close is anchored on an unforgeable bot-author login, sentinels are forgery-proof compile-time constants, and no secrets leak. |
…gate + reblock verify) Addresses the REQUEST-CHANGES findings from the Step 17b code review and the security/philosophy reviews on PR #4964. HIGH (correctness, blocking) — auto-doc PR gate was inert in production: `is_auto_doc_pr` hard-coded the expected author to `AUTO_DOC_PR_AUTHOR` (= `simard-overseer[bot]`), but real auto-doc PRs are authored under Simard's engineer/OODA gh identity (`config::automerge_author`, distinct by design). The gate matched ZERO real PRs. Fix: remove the constant, make `is_auto_doc_pr` / `reconcile_doc_prs` take an `expected_author`, and resolve it at the I/O boundary in `run_doc_pr_reconcile` via `automerge_author()`. The author conjunct stays mandatory (security review's requirement — author.login is GitHub-authoritative/unspoofable); unset identity => empty => fail-closed inert. Split the executor into a thin env-resolving `run_doc_pr_reconcile` wrapper and a testable `run_doc_pr_reconcile_with_author` core. MEDIUM — silent inert gate: `run_doc_pr_reconcile_with_author` now emits a LOUD `warn` when title-marker PRs are present but NONE pass the identity gate, naming the likely cause so an inert pass never fails silently. MEDIUM — reblock fold verification: added tests pinning `fold_volatile_goal_ids` against the REAL production dedup_key shapes `classify_signal` emits (`goal:blocked:<id>`, `goal:stale:<id>`, `loop:<id>`, `drift:<id>`), incl. an end-to-end dedup assertion through the Decide seam. LOW — cleaned `let _ = class;` (use `class: _` in the match arm). Tests: fixed the test-masks-bug (fixtures bound author to the gate's own constant). Fixtures now use a realistic engineer author distinct from the overseer bot; added regression tests: overseer-bot author is NOT a candidate, empty expected author fails closed, and the executor is inert (no closes) on author mismatch / unresolved identity. Also updated the API reference doc. Verified: cargo check/clippy/fmt clean; targeted suites green (28 doc_pr_reconcile/reblock + 159 no_progress/quarantine). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Step 17d — Philosophy Guardian ReviewAssessed Compliance checklist
VerdictNOT COMPLIANT — changes required, driven entirely by the Zero-BS failure.
Strengths worth preserving: no new stubs/TODOs/ Posted as evidence for Step 17d. Filed as a COMMENT (GitHub blocks non-COMMENT self-reviews for the PR author). |
Step 17e — Blocking Issues AddressedAll REQUEST-CHANGES findings from the Step 17b code review (plus the security/philosophy review notes) are now fixed. Pushed as
Test-masks-bug (root cause of the HIGH slipping through)Fixtures bound
Verification
The security review's guidance is honored: the fix keeps the author conjunct (resolving via |
Finding #1 (quarantine off-by-one timing): the pre-bump surfaced-failure bound in resolution_for_why is deliberate and unit-tested, but nothing pinned the end-to-end cycle of fire. Add a regression test that drives reinvestigate_bare_blocked_goals cycle by cycle and asserts the evidence-less stall surfaces EXACTLY SURFACED_INVESTIGATION_FAILURE_LIMIT times (retriable each cycle) and terminally quarantines only on the (LIMIT + 1)th observation. Finding #4 (fold_volatile_goal_ids negative coverage): add explicit edge / negative cases — goal- without a digit, trailing goal-/simard-identity- prefixes with empty slugs, partial digit-run folds (goal-12abc -> goal-*abc), multi-occurrence folds, and UTF-8 multibyte char-boundary safety. No production behavior change; tests only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Step 18a — Analyze Review Feedback (MANDATORY)Analyzed all Step 17 feedback (17b code review · 17c security · 17d philosophy) against the actual fix commit Disposition of every blocking / noted finding
Residual (NON-BLOCKING) — one item worth a follow-upLabel + draft prong of the original HIGH is not addressed.
This is a defensible fail-safe (a strict gate avoids ever closing a non-auto-doc PR), so it is not merge-blocking. Recommendation: either (a) narrow the docstring claim to "drain correctly-labeled draft duplicates," or (b) file a follow-up to handle legacy unlabeled/non-draft PRs on the auto-doc branch pattern. Operational note (delivery dependency)The fix is fail-closed by design, so Problem #5 only acts in production if VerdictProceed — all blocking (HIGH) findings resolved; security-clean; philosophy note addressed. Remaining items are one non-blocking safety/docstring residual and one operational reminder. No further code changes required to merge; the two recommendations are follow-ups, not gates. |
Step 18b: implement the one actionable non-blocking residual from the Step 18a review analysis. The auto-doc PR reconciler's identity gate (is_auto_doc_pr) intentionally acts only on correctly-labeled auto-doc drafts authored by the resolved auto-doc identity — a security-motivated fail-safe (Step 17c) so a human/non-auto-doc PR is never auto-closed. Make that scope boundary explicit instead of implying the whole doc-PR backlog is drained: add a Scope section to the module docstring and the reference doc, and tighten the MAX_CLOSES_PER_CYCLE wording to 'backlog of reconciliation candidates'. Broadening the gate to sweep legacy unlabeled/non-draft PRs would reintroduce the spoofing risk, so it is tracked as follow-up #4965 rather than implemented here. No behavior change (doc-comment/reference only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Step 18b — Implement Review Feedback (MANDATORY)Per the Step 18a disposition, all blocking (HIGH/MED) findings were already resolved by
What I implemented (commit
|
…`goal unblock` Quality-audit finding on PR #4964 (terminal-quarantine churn suppression). The runbook (Option B) and the concept/API docs all state that `simard goal unblock <goal-id>` "clears the quarantine marker" so a terminally-quarantined UNCLEAR-CRITERIA goal earns a fresh bounded window. The code never implemented it: `handle_unblock` only reset status to NotStarted, leaving the durable `ooda-breaker-quarantine` WipRef in place. No code path anywhere cleared the marker, so a "revived" goal was restored to NotStarted while `reinvestigate_bare_blocked_goals` still skipped it forever — the documented recovery contract was unimplemented. Fixes: - `handle_unblock` now retains-out the quarantine marker (via `is_quarantine_ref`) so a single-id unblock fully revives the goal, and logs when a marker was cleared. `unblock-all` is intentionally left untouched (scoped to the brain-failure safeguard marker only). - Corrected `docs/concepts/ooda-breaker-churn-suppression.md`, which contradicted the runbook by claiming `simard goal unblock-all` also clears quarantines; it does not. - Added regression test `simard_goal_unblock_clears_the_ooda_quarantine_marker`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ep 18a #2/#6) Tighten the `resolution_for_why` `surfaced_failures` docstring and the ooda read-site comment to state the pre-bump bound semantics precisely: the evidence-less stall is surfaced (retriable) for its first LIMIT observations and terminal quarantine fires on the (LIMIT + 1)th observation — a deliberate one-observation shift from the old post-bump escalate-at-LIMIT trigger, pinned end-to-end by `quarantine_fires_on_the_cycle_after_the_limit_th_surface`. Removes the misleading "byte-for-byte unchanged below the bound" / "once it reaches LIMIT" phrasing that read as fire-at-LIMIT (Step 18a review findings on no_progress_breaker.rs and ooda_loop/no_progress.rs). Doc-only; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📊 Coverage Summary
Coverage data from CI run. Test files matching |
rysweet
added
the
simard-autonomous
rysweet
left a comment
rysweet
left a comment
There was a problem hiding this comment.
Step 17b — Comprehensive Code Review
Verdict: APPROVE (no blocking issues). Reviewed all 8 non-test source files, the pure/IO split, wiring, recovery path, and ran the targeted suites. The two churn fixes are additive, fail-closed, and well-tested.
Verification performed
cargo test --lib quarantine→ 38 passedcargo test --lib -- doc_pr_reconcile→ 19 passedcargo test --lib -- reblock→ 12 passed- Traced the full error/churn path:
resolution_for_why→apply_resolution_side_effects→reinvestigate_bare_blocked_goalsshort-circuit →goal unblockrecovery.
Strengths (confirmed correct)
- Injection-safety is real.
quarantine_markeruses a fixed compile-time sentinelref_id(never goal-derived);is_quarantine_refkeys on bothkindand the sentinel, so goal text can neither forge a quarantine nor smuggle content into a marker. Pinned by tests. - Single-open invariant cannot close everything.
reconcile_doc_prsselects canonical =max(number)and explicitly excludes it fromto_close; zero/one candidate ⇒ no closes. - Identity gate is fail-closed and spoof-resistant.
is_auto_doc_prrequires title marker AND non-empty resolved author ANDauthor == expected_authorANDis_draft == Some(true)AND the auto-doc label — every conjunct mandatory. Empty/unresolved author matches nothing. GitHub-authoritativeauthor.login+ label (needs triage/write) make a crafted external PR a non-candidate. - Churn actually ends. The re-investigation pass short-circuits quarantined goals before consulting the reasoner (verified by the
PanicReasonertest), and the marker is durable (persisted inwip_refs), so a restart that re-parks a goal bare still skips it. - Never
evidence=[(none)].QuarantineTerminalblocks WITH the surfaced count rendered as concrete evidence; the non-zero-count invariant is pinned. - Recovery path exists and is tested.
goal unblockclears the marker (wip_refs.retain(!is_quarantine_ref)) and restoresNotStarted;unblock-allis deliberately scoped away. - No TODOs, stubs, unimplemented functions, or swallowed exceptions. Per-close failures are collected into
report.errorsand logged (fail-visible), read errors are fail-closed with no mutations.
Non-blocking findings
1. [Info / deployment-assumption] Reconciler effectiveness hinges on SIMARD_AUTOMERGE_AUTHOR matching the real auto-doc PR author.
The flow that creates "Update documentation with …" PRs is not in this repo (confirmed by grep — no pr create with that title). If, in production, those PRs are authored by simard-overseer[bot] (or any login other than automerge_author()), the gate never matches and reconciliation is permanently INERT. This is well-mitigated: run_doc_pr_reconcile_with_author emits a LOUD warn naming the exact cause when title-marker PRs exist but none pass the gate. Action: confirm the actual auto-doc PR author login equals SIMARD_AUTOMERGE_AUTHOR in the deployed env before relying on this pass, and consider an ops alert on that warn.
2. [Nit] DocPrReconcileReport::skipped is noisy.
skipped = open_prs.len() - candidate_count counts all unrelated open PRs (up to the 200 fetch window), not just title-marker-but-rejected ones. The logged number will be dominated by unrelated PRs and won't isolate the interesting "looked like an auto-doc PR but failed a conjunct" case. Consider reporting title_marker_count - candidate_count (already computed) as the meaningful skip signal.
3. [Info, no action] record_progress clears the surfaced-failure counter but intentionally not the quarantine marker.
Only goal unblock clears the marker — documented and correct. Noting for completeness so a future reader doesn't "fix" it.
Checklist
- Code quality and standards — consistent with existing
operator_cli(eprintln!) andoverseer(tracing) conventions - Test coverage adequate — 69 new targeted tests, all green
- No TODOs, stubs, or swallowed exceptions
- No unimplemented functions
- Logic correctness — quarantine timing shift is deliberate and pinned; invariants hold
- Edge case handling — empty/unresolved author, zero/one candidate, list error, per-close error, restart re-park, idempotent re-fire all covered
Step 17c — Security ReviewVerdict: APPROVE — no security-blocking issues found. Reviewed the full change set with a security lens: command construction, authorization/PR-close authority, injection surfaces, sensitive-data handling, and fail-closed behavior. All security-invariant tests pass (45 targeted, incl. the injection-safety and never-close-a-human-PR guards). Requirements verified ✔1. Injection vulnerabilities — none (command execution). 2. Authorization / PR-close authority — fail-closed, spoof-resistant. 3. Injection-safe quarantine marker. 4. Single-open invariant — provably cannot over-close. 5. Sensitive-data handling — clean. 6. Non-blocking security observations
Conclusion: The change is injection-safe (argv-only), fail-closed on the authorization gate and on read errors, and free of sensitive-data exposure. No security fix is warranted. |
Step 17d — Philosophy Guardian ReviewVerdict: ✅ COMPLIANT — no philosophy blockers. Reviewed the two changed subsystems ( Compliance checklist
Notes (non-blocking)
Outcome: Philosophy-compliant. No changes required. |
Step 18a — Analyze Review Feedback (MANDATORY)Analyzed the latest Step 17 review round against branch HEAD
Both reviews evaluated the current HEAD (post-fix: Disposition of every finding in this round
ConclusionNo actionable findings in this round. Both the security and philosophy reviews are clean approvals of the current HEAD; the only observations are two non-blocking, deployment-config notes (least-privilege token scope, All previously-blocking findings (17b HIGH author-gate, MED inert-gate telemetry, MED reblock-key verification, LOW timing docstring, LOW style) remain resolved by the commits already on this branch. Verdict: Proceed — no code changes required from Step 18a. Nothing to implement in 18b beyond the already-filed follow-up #4965. |
Step 18b — Implement Review Feedback (MANDATORY)Implementing the disposition from Step 18a against branch HEAD Actionable code changes required: noneEvery review finding across the full 17b→17d rounds is already resolved by commits on this branch. Re-verified each item still holds at HEAD:
Non-blocking items — dispositioned, no source change
Verification
Verdict: Step 18b complete — zero code changes required. All actionable feedback was already implemented on the branch; the only open items are non-blocking and tracked in #4965. PR remains MERGEABLE. |
…o-progress escalation storm (#4973) * wip: checkpoint after implementation (steps 7-8) Automatic checkpoint to preserve work in progress. Tests and implementation saved before refactoring phase. * fix(overseer): resolve Step 17e blocking review findings (Problem #5 gate + reblock verify) Addresses the REQUEST-CHANGES findings from the Step 17b code review and the security/philosophy reviews on PR #4964. HIGH (correctness, blocking) — auto-doc PR gate was inert in production: `is_auto_doc_pr` hard-coded the expected author to `AUTO_DOC_PR_AUTHOR` (= `simard-overseer[bot]`), but real auto-doc PRs are authored under Simard's engineer/OODA gh identity (`config::automerge_author`, distinct by design). The gate matched ZERO real PRs. Fix: remove the constant, make `is_auto_doc_pr` / `reconcile_doc_prs` take an `expected_author`, and resolve it at the I/O boundary in `run_doc_pr_reconcile` via `automerge_author()`. The author conjunct stays mandatory (security review's requirement — author.login is GitHub-authoritative/unspoofable); unset identity => empty => fail-closed inert. Split the executor into a thin env-resolving `run_doc_pr_reconcile` wrapper and a testable `run_doc_pr_reconcile_with_author` core. MEDIUM — silent inert gate: `run_doc_pr_reconcile_with_author` now emits a LOUD `warn` when title-marker PRs are present but NONE pass the identity gate, naming the likely cause so an inert pass never fails silently. MEDIUM — reblock fold verification: added tests pinning `fold_volatile_goal_ids` against the REAL production dedup_key shapes `classify_signal` emits (`goal:blocked:<id>`, `goal:stale:<id>`, `loop:<id>`, `drift:<id>`), incl. an end-to-end dedup assertion through the Decide seam. LOW — cleaned `let _ = class;` (use `class: _` in the match arm). Tests: fixed the test-masks-bug (fixtures bound author to the gate's own constant). Fixtures now use a realistic engineer author distinct from the overseer bot; added regression tests: overseer-bot author is NOT a candidate, empty expected author fails closed, and the executor is inert (no closes) on author mismatch / unresolved identity. Also updated the API reference doc. Verified: cargo check/clippy/fmt clean; targeted suites green (28 doc_pr_reconcile/reblock + 159 no_progress/quarantine). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(ooda,overseer): pin Step 18a review findings #1 and #4 Finding #1 (quarantine off-by-one timing): the pre-bump surfaced-failure bound in resolution_for_why is deliberate and unit-tested, but nothing pinned the end-to-end cycle of fire. Add a regression test that drives reinvestigate_bare_blocked_goals cycle by cycle and asserts the evidence-less stall surfaces EXACTLY SURFACED_INVESTIGATION_FAILURE_LIMIT times (retriable each cycle) and terminally quarantines only on the (LIMIT + 1)th observation. Finding #4 (fold_volatile_goal_ids negative coverage): add explicit edge / negative cases — goal- without a digit, trailing goal-/simard-identity- prefixes with empty slugs, partial digit-run folds (goal-12abc -> goal-*abc), multi-occurrence folds, and UTF-8 multibyte char-boundary safety. No production behavior change; tests only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(overseer): document deliberate auto-doc reconcile scope boundary Step 18b: implement the one actionable non-blocking residual from the Step 18a review analysis. The auto-doc PR reconciler's identity gate (is_auto_doc_pr) intentionally acts only on correctly-labeled auto-doc drafts authored by the resolved auto-doc identity — a security-motivated fail-safe (Step 17c) so a human/non-auto-doc PR is never auto-closed. Make that scope boundary explicit instead of implying the whole doc-PR backlog is drained: add a Scope section to the module docstring and the reference doc, and tighten the MAX_CLOSES_PER_CYCLE wording to 'backlog of reconciliation candidates'. Broadening the gate to sweep legacy unlabeled/non-draft PRs would reintroduce the spoofing risk, so it is tracked as follow-up #4965 rather than implemented here. No behavior change (doc-comment/reference only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(ooda,docs): implement the documented quarantine recovery path in `goal unblock` Quality-audit finding on PR #4964 (terminal-quarantine churn suppression). The runbook (Option B) and the concept/API docs all state that `simard goal unblock <goal-id>` "clears the quarantine marker" so a terminally-quarantined UNCLEAR-CRITERIA goal earns a fresh bounded window. The code never implemented it: `handle_unblock` only reset status to NotStarted, leaving the durable `ooda-breaker-quarantine` WipRef in place. No code path anywhere cleared the marker, so a "revived" goal was restored to NotStarted while `reinvestigate_bare_blocked_goals` still skipped it forever — the documented recovery contract was unimplemented. Fixes: - `handle_unblock` now retains-out the quarantine marker (via `is_quarantine_ref`) so a single-id unblock fully revives the goal, and logs when a marker was cleared. `unblock-all` is intentionally left untouched (scoped to the brain-failure safeguard marker only). - Corrected `docs/concepts/ooda-breaker-churn-suppression.md`, which contradicted the runbook by claiming `simard goal unblock-all` also clears quarantines; it does not. - Added regression test `simard_goal_unblock_clears_the_ooda_quarantine_marker`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(ooda): clarify deliberate pre-bump quarantine trigger timing (Step 18a #2/#6) Tighten the `resolution_for_why` `surfaced_failures` docstring and the ooda read-site comment to state the pre-bump bound semantics precisely: the evidence-less stall is surfaced (retriable) for its first LIMIT observations and terminal quarantine fires on the (LIMIT + 1)th observation — a deliberate one-observation shift from the old post-bump escalate-at-LIMIT trigger, pinned end-to-end by `quarantine_fires_on_the_cycle_after_the_limit_th_surface`. Removes the misleading "byte-for-byte unchanged below the bound" / "once it reaches LIMIT" phrasing that read as fire-at-LIMIT (Step 18a review findings on no_progress_breaker.rs and ooda_loop/no_progress.rs). Doc-only; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(goal): filter tombstones on the OODA board READ path (stop escalation storm) The `simard ooda run` daemon loads its goal board via `load_goal_board` -> `read_latest_snapshot`, which read the latest `goal-board:snapshot` fact from cognitive memory and returned it **without applying `filter_tombstoned`**. That filter only ran on the operator-command daemon path (`simard goal list`). Consequence: a goal that was removed and tombstoned still lingered inside an older snapshot fact, so every OODA cycle re-materialised it into `state.active_goals`, the no-progress breaker saw it as blocked-with-no-progress, and filed a fresh duplicate escalation issue — then re-persisted the dead goal on the next save-merge. This is why `simard goal list` showed the goal gone while the OODA loop kept resurrecting it and storming ~1 GitHub issue every 90 minutes (`OODA no-progress breaker: goal stuck ... UNCLEAR-CRITERIA`), immune to 30 days of downstream band-aids (dedup guards, done-gate pins, terminal rungs) that never touched this load-path gap. Fix: apply `filter_tombstoned` inside `read_latest_snapshot`, the shared read used by BOTH `load_goal_board` (OODA cycle) and the `save_goal_board` merge-read, so a tombstoned goal can never be read back from memory on any path. Single defensive point covering every caller (cycle, journal, creative-ideas, meeting-backend, status). Adds a regression test: a snapshot containing a tombstoned goal id is filtered out of the loaded board while live goals survive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Additive, non-breaking implementation of the two churn fixes from the Step 2c requirements:
UNCLEAR-CRITERIA/GENUINELY-STUCKgoal exhausts its bounded re-investigation budget (SURFACED_INVESTIGATION_FAILURE_LIMIT), the breaker returns the newNoProgressResolution::QuarantineTerminal, blocks the goal with the surfaced count as real evidence (neverevidence=[(none)]), files exactly one deduplicated tracking issue, and writes a durable, injection-safequarantine_marker(WipRef).reinvestigate_bare_blocked_goalsshort-circuits quarantined goals before consulting the reasoner, ending the re-schedule/re-file storm. Also stabilizes therecurring_goal_reblockstewardship signature viafold_volatile_goal_ids(foldssimard-identity-<slug>and positionalgoal-<n>to stable placeholders).overseer::doc_pr_reconcilemodule enforcing a single-open invariant for auto-generated"Update documentation with …"PRs: a pure decision core (reconcile_doc_prs) + a fail-closed, bounded IO executor (run_doc_pr_reconcile) that closes superseded / stale-CONFLICTING drafts via the additivePrGhClient::close_pr. Gated behind the governed roster, the shared gap-scan opt-out, and an every-N cadence.Validation
cargo check --tests— clean (exit 0).Notes
This PR is opened to host the mandatory Step 17b comprehensive code review. See the review comments for findings (one HIGH-severity correctness issue on the Problem #5 identity gate).
Out of scope (merge escalations only): #4939, #4941, #4926.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com