fix(ooda): idempotent done-gate PR emission — one PR per goal (#4166/#4189)

[rysweet]

Problem

The OODA/overseer done-gate opened a fresh done-gate PR every cycle for a goal whose engineer had exited leaving an open, unmerged PR — flooding the repo (3× coin-benchmark #4326/#4329/#4332, 4× kgpacks-parity #4324/#4328/#4330/#4333). Goal→engineer dispatch had no idempotency keyed on durable goal identity.

Tracking issues: #4166, #4189.

Fix

Make done-gate emission idempotent on stable goal identity (never the goal title):

• Durable primary guard — new goal_pr_emissions ledger (typed-OODA schema v1→v2, forward-only, additive):
  • record_goal_pr_emission upsert (ON CONFLICT(goal_key) DO UPDATE)
  • indexed find_open_goal_pr_emission open-lookup
  • UNIQUE(repo, pr_number) turns a second goal claiming the same PR into a visible conflict, not a silent duplicate
  • rows are never deleted — a completed PR transitions state, so an emission outlives its engineer
• Advisory secondary seam — pure, total, no-panic dedup bricks (goal_dedup_key, parse_goal_key_trailer, find_open_pr_for_goal) + a GoalPrRef DTO and a PrGhClient::list_open_goal_prs default, so a gh open-PR reconciliation can adopt a pre-existing PR by the Simard-Goal-Key: body trailer (primary) or the engineer/{key}- branch convention (fallback).

Safety / hygiene

• Additive and non-breaking: the default list_open_goal_prs returns empty, existing fakes compile unchanged; distinct goals are unaffected.
• Structured tracing + OTel only — no stray print!/println!, no Bridge naming.

Tests

• 6 ledger contract tests (upsert idempotency, open-only lookup, UNIQUE(repo,pr_number) conflict, migration idempotent across reopen)
• 27 dedup unit tests (key determinism + boundary-safety, trailer parsing, PR matching precedence + branch boundary guard)
• 3-case integration test: same-goal cycles emit exactly one PR; distinct goals each dispatch; a reopened goal dispatches again after its previous PR closes

Full lib suite green (8889 passed), cargo fmt --check clean, cargo clippy --all-targets --all-features -D warnings clean.

Docs

Concept, how-to runbook, and ledger API reference, all linked from the index and mkdocs nav.

Fixes #4166
Fixes #4189

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[github-actions]

📊 Coverage Summary

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

Module	Lines	Covered	Coverage
Total	186094	155756	83.7%

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

[rysweet]

✅ Verification: merge-ready endorsement (done-gate idempotency)

Validated against the OODA done-gate idempotency requirements. This PR fully resolves the near-duplicate done-gate PR churn (#4328/#4330/#4333 kgpacks-rs; #4326/#4329/#4332 COIN).

Verification results

• Grep-clean: no Bridge naming; no stray print!/println!/eprintln! in new code (tracing/OTel only). ✔
• Idempotency surface correct: third dispatch guard in the dispatch_spawn_engineer guard chain, keyed on a durable per-goal emission ledger (goal_pr_emissions, schema v2) plus advisory live-PR lookup via stewardship::merge_authority::list_open_prs. ✔
• Regression test tests/done_gate_dedup_integration.rs:
  • same_goal_across_two_cycles_emits_exactly_one_pr — second cycle is an idempotent no-op; tracked PR stays fix(ooda): make the local COIN benchmark harness goal machine-checkable (delivered by merged PRs #4171/#2763) #4326 (never re-emitted). ✔
  • distinct_goals_each_emit_their_own_pr — distinct goal keys emit independently. ✔
  • reopened_goal_dispatches_again_after_previous_pr_closes — re-dispatch allowed once the prior PR is no longer open. ✔
• CI: all required checks green (cargo-audit/deny/vet, coverage, e2e-dashboard, install-real, npm-audit, pre-commit, scripts-tests, GitGuardian). ✔
• Additive / non-breaking; PRD preserved. ✔

Only outstanding item: reviewDecision is empty (unreviewed). Requesting a maintainer review/approval to land. No competing PR should be opened — this is the canonical fix. Symptom duplicate PRs (#4328/#4330/#4333, #4326/#4329/#4332) self-resolve once this merges.

[rysweet]

📐 DB design review (Step 5c) — schema validated, one doc-drift nit

Validated the goal_pr_emissions ledger (schema v2) against the OODA done-gate idempotency requirements. Schema, migration, constraints, and index are correct and minimal — the goal_key PK enforces ≤1 emission per goal-identity, UNIQUE(repo, pr_number) makes concurrent re-record TOCTOU-safe, rows transition state instead of being deleted, and the migration is forward-only + idempotent + zero-backfill. No new DB work required.

⚠ Non-blocking doc drift — the design markdown in this PR describes a schema that differs from the implemented migration in ledger.rs (the authoritative source):

Aspect	Markdown (doc)	Migration (actual)
created_at column	present	absent (only updated_at)
open index	partial WHERE state='open'	composite (goal_key, state)
index name	idx_goal_pr_emissions_open	goal_pr_emissions_open_idx

Suggest aligning the markdown to the implementation (drop created_at, document the composite index + real name) — or, if created_at is genuinely wanted, add it to the migration. Correctness is unaffected; does not block merge. Please do not add a second v2 migration to fix this.

[rysweet]

🔒 Security requirements review (Step 5d) — no blocking findings

Reviewed against the OODA done-gate idempotency trust boundaries: SQLite ledger, the gh subprocess + its JSON output, and PR-body trailers/branch names authored by spawned engineers.

AuthZ: simard-autonomous label is a filter, not authorization — a match must also carry the Simard-Goal-Key: trailer or engineer/{goal-key}- branch before it suppresses emission. Guard never gains merge/close authority (stays in merge_authority). No new credentials (reuses ambient gh auth). ✔

Input validation: trailer parse is total/no-panic, line-anchored, case-sensitive, exact [0-9a-f]{16}; u32::try_from(pr_number) rejects overflow/negative; frame_identity_field makes the (goal_id, repo)→SHA-256 encoding injective (a newline can't be confused with the field boundary); malformed ledger state fails closed via InvalidQuery. ✔

Data protection: SQL fully parameterized (params![], no format! interpolation); TransactionBehavior::Immediate + UNIQUE(repo, pr_number) blocks TOCTOU double-claim; no secrets/PII in columns or logs (structured tracing/OTel only). ✔

Subprocess: Command::new("gh").args(&[...]) — direct argv exec, no shell, so no injection surface; fails open with a warn on gh outage. ✔

Residual risk (Low, accepted): a spoofed Simard-Goal-Key: trailer on a labeled PR could cause denial-of-emission (suppress a legit PR) — but blast radius is suppression only (never merge/close of attacker content), requires repo write, is surfaced in the OODA log, and self-heals next cycle. If ever escalated, mitigate by cross-checking head-branch + author identity — do not weaken fail-open.

Verdict: merge-ready from a security standpoint. No code changes required.

[rysweet]

✅ Step 8 (Implement) — validation: #4334 fully resolves the done-gate duplication; merge-ready

Per the OODA-core done-gate idempotency brief (Problem #2; #4166/#4189), this PR is the correct and complete fix. A competing/duplicate implementation was deliberately not opened (the anti-pattern this PR exists to prevent); the parallel workstream instead validated #4334 and is driving it to merge.

Requirement coverage

• Idempotent emission, ≤1 open PR per (goal, done-gate): durable primary guard goal_pr_emissions ledger — record_goal_pr_emission upsert (ON CONFLICT(goal_key) DO UPDATE) + indexed find_open_goal_pr_emission, UNIQUE(repo, pr_number) surfacing a second-goal claim as a visible conflict, rows never deleted (state transitions instead, so an emission outlives its engineer / survives restart).
• Advisory live-PR lookup: pure/total dedup bricks goal_dedup_key, parse_goal_key_trailer, find_open_pr_for_goal + GoalPrRef/list_open_goal_prs seam adopting a pre-existing PR by the Simard-Goal-Key: trailer (primary) or engineer/{key}- branch (fallback).
• Regression test proves the no-op: tests/done_gate_dedup_integration.rs::same_goal_across_two_cycles_emits_exactly_one_pr asserts cycle 2 does not dispatch/emit a second PR and the tracked open PR stays fix(ooda): make the local COIN benchmark harness goal machine-checkable (delivered by merged PRs #4171/#2763) #4326; distinct_goals_each_emit_their_own_pr confirms distinct goals are unaffected; reopened_goal_dispatches_again_after_previous_pr_closes confirms the guard suppresses only while a PR is OPEN.

Hygiene (grep-clean on added src/tests): no Bridge naming; no stray print!/println!. Additive / non-breaking (default list_open_goal_prs returns empty; existing fakes compile unchanged; PRD preserved). Structured tracing + OTel only.

Status: MERGEABLE / mergeStateStatus CLEAN, all required checks green (coverage, pre-commit, install-real, e2e-dashboard, cargo-audit/deny/vet, npm-audit, scripts-tests). reviewDecision is empty (unreviewed) — self-approve is blocked (author == reviewer), so this needs one operator/reviewer approval to land.

Recommendation: approve & merge #4334. Once landed, the duplicate symptom PRs (#4328/#4330/#4333 kgpacks-parity; #4326/#4329/#4332 coin-benchmark) can be closed as operator cleanup — they are symptoms, not fix targets.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[rysweet]

Crusty review — PRIORITY root-cause, but needs rebase + scope-check before it can merge.

This is the keystone for the done-gate churn: without idempotent one-PR-per-goal emission, the OODA loop opened a flood of near-duplicate done-gate PRs (currently ~9 open for the COIN and kgpacks-rs goals alone — #4332/#4329/#4326/#4322, #4330/#4333/#4324/#4328). Landing this stems the flood at the source, after which most of those per-goal PRs can be closed rather than merged.

Two blockers before merge:

1. DIRTY — rebase onto current main. This branch is 9 days stale and now conflicts. Main has since gained feat(goal): 'set-done-gate' operator command to bind a machine-checkable finish line #4510 (set-done-gate operator command + goal_done_gate_pins.json store) and fix(overseer): durable GitHub-side dedup for gap-scan issue filing (#4717) #4742 (durable GitHub-side gap-scan dedup), both touching adjacent OODA/overseer emission code. Please rebase and reconcile the idempotency key against fix(overseer): durable GitHub-side dedup for gap-scan issue filing (#4717) #4742's stewardship-signature dedup so we don't end up with two overlapping "don't re-emit" gates.

2. Scope check — +1722/-3 across 12 files is large for "idempotent emission." A focused idempotency guard (one PR per goal, keyed by goal id / dedup_key, checked before emit) should be far smaller. Please confirm the extra volume isn't docs/scaffolding that violates the point-in-time-doc policy, and trim to the load-bearing change.

Once rebased, reconciled with #4742, and scoped down: crusty → merge-ready → CI green → merge. This one should jump the queue — it makes the rest of the cluster closable.