diff --git a/.gitignore b/.gitignore index 74365c534..557223cf7 100644 --- a/.gitignore +++ b/.gitignore @@ -38,3 +38,4 @@ scripts/dashboard-audit/out/ pr*-worktree/ *-worktree/ .simard/ +gym_history.db diff --git a/Cargo.lock b/Cargo.lock index 3660d5d62..181215c37 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3883,7 +3883,7 @@ dependencies = [ [[package]] name = "simard" -version = "0.34.0" +version = "0.35.0" dependencies = [ "amplihack-agent-eval", "amplihack-memory", diff --git a/Cargo.toml b/Cargo.toml index 88c3f0e0c..a807622a7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "simard" -version = "0.34.0" +version = "0.35.0" edition = "2024" default-run = "simard" diff --git a/docs/concepts/done-gate-slug-convergence.md b/docs/concepts/done-gate-slug-convergence.md new file mode 100644 index 000000000..d67e9923c --- /dev/null +++ b/docs/concepts/done-gate-slug-convergence.md @@ -0,0 +1,114 @@ +--- +title: "Concept: done-gate slug convergence (one goal → one done-gate PR)" +description: > + Why a completed goal now converges on a SINGLE done-gate PR instead of + accumulating competing done-gate PRs and stale CONFLICTING engineer branches. + The slug-keyed convergence in the goal completion gate — keep the oldest CLEAN + done-gate PR, supersede/close the duplicates (scoped to the bot author AND the + exact sanitized slug), and prune stale out-of-flight branches by logic rather + than by hand. +last_updated: 2026-07-21 +review_schedule: as-needed +owner: simard +doc_type: concept +status: partially implemented +related: + - ./deploy-aware-done-gate.md + - ./gap-scan-backoff-dedup.md + - ./stewardship-mode.md + - ../reference/done-gate-slug-dedup-api.md + - ../reference/completion-evidence-gate-api.md + - ../howto/triage-stale-pull-requests.md + - ../howto/diagnose-a-rejected-goal-completion.md + - ../../src/goal_curation/completion_gate.rs + - ../../src/goal_curation/advance_goal/spawn.rs + - ../../src/goal_curation/advance_goal/goal_session.rs +--- + +# Concept: done-gate slug convergence + +> **Status: partially implemented.** The slug-keyed convergence **logic** +> (`converge_done_gate_prs()`, `sanitize_goal_slug()`, and the ownership-scoped +> supersede decision) is implemented and unit-tested — over an injected +> PR-lister — in +> [`src/goal_curation/completion_gate.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/completion_gate.rs). +> Wiring this decision into the advance-goal spawn/session path +> ([`advance_goal/spawn.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/advance_goal/spawn.rs), +> [`advance_goal/goal_session.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/advance_goal/goal_session.rs)) +> so the done-gate actually converges at runtime is a tracked follow-up and is +> **not yet integrated**. See the +> [done-gate slug dedup API reference](../reference/done-gate-slug-dedup-api.md) +> for the typed surface. + +> Once wired, a completed goal converges on a **single** done-gate PR. The +> convergence **logic dedups by goal slug**: it keeps the oldest `CLEAN` +> done-gate PR and +> supersedes/closes the rest — scoped to PRs authored by Simard's bot **and** +> matching the exact goal slug — while stale `CONFLICTING` engineer branches for +> an out-of-flight goal are pruned by logic instead of accumulating. + +## The problem this solves + +The coin-benchmark-harness goal +(`build-a-local-coin-benchmark-harness-…-09e65e35`) — absent from +`inflight_refs` — accumulated ~8 open PRs with **none merged**: + +- 5 stale `CONFLICTING`/`DIRTY` engineer branches (`#4161`, `#4149`, `#4134`, + `#4101`, `#3190`), and +- 3 competing `CLEAN` done-gate PRs (`#4332`, `#4329`, `#4326`). + +That is **retry churn without delivery**: the done-gate opened a *new* +done-gate PR for the same goal slug each time it fired, and no single PR ever +converged to merged. The fix repairs the dedup/convergence **logic** so the +churn stops — it does **not** hand-close PRs. + +## How convergence works + +When the done-gate fires for a completed goal, it enumerates the open PRs for +that goal via an injected PR-lister and converges them: + +```mermaid +flowchart TD + A[Done-gate fires for goal slug S] --> B[List open PRs authored by bot matching slug S] + B --> C{≥1 CLEAN done-gate PR?} + C -- yes --> D[Keep the OLDEST CLEAN done-gate PR] + D --> E[Supersede/close the remaining bot+slug done-gate PRs] + C -- no --> F[Keep/open one done-gate PR] + B --> G[Prune stale CONFLICTING engineer branches for out-of-flight slug] +``` + +- **Keep the oldest CLEAN.** Among competing done-gate PRs for one slug, the + **oldest `CLEAN`** PR wins (deterministic, minimizes wasted CI). The rest are + superseded/closed with a note pointing at the kept PR. +- **Ownership-scoped supersede.** A PR is only superseded/closed when it is + **both** authored by Simard's bot identity **and** matches the exact goal + slug prefix. Human PRs and unrelated PRs are never touched. +- **Prune stale out-of-flight branches.** `CONFLICTING`/`DIRTY` engineer + branches for a goal no longer in `inflight_refs` are pruned by the same + scoped logic, rather than accumulating. + +## Why this is safe + +- **Slug sanitization.** The goal slug is sanitized to `[a-z0-9-]` before it is + used in any branch name, `gh` argv, or path — no `..`, path separators, or + shell metacharacters. Convergence for one slug can never reach another goal's + PRs. +- **Bot-author scoping.** Supersede/close is restricted to PRs authored by + Simard's bot **and** the exact slug — a double predicate that keeps the fix + from ever closing a human or unrelated PR. +- **Logic, not hand-closing.** The gate repairs the *dedup/convergence logic* + so duplicates stop being created; it does not one-off-close PRs as a + workaround. +- **Idempotent.** Re-running the gate on an already-converged goal is a no-op: + the single kept PR matches, nothing else to supersede. + +## Related + +- [Deploy-aware done-gate](./deploy-aware-done-gate.md) — the completion + evidence gate this convergence sits alongside. +- [Gap-scan dedup & backoff](./gap-scan-backoff-dedup.md) — the sibling dedup + posture on the Observe side. +- [Done-gate slug dedup API reference](../reference/done-gate-slug-dedup-api.md) + — the typed surface, sanitization rules, and edge-case matrix. +- [Triage stale open pull requests](../howto/triage-stale-pull-requests.md) — + the operator runbook for the manual counterpart. diff --git a/docs/concepts/objective-merge-judge-fallback.md b/docs/concepts/objective-merge-judge-fallback.md new file mode 100644 index 000000000..16b81a07f --- /dev/null +++ b/docs/concepts/objective-merge-judge-fallback.md @@ -0,0 +1,157 @@ +--- +title: "Concept: objective merge-judge fallback (converge delivery-ready PRs)" +description: > + Why green, mergeable, non-in-flight rysweet-authored PRs are now actually + merged instead of being re-escalated every Overseer tick. The opt-in + ObjectiveMergeJudge tier that gives build_merge_judge() a non-refusing merge + authority for trusted authors past the objective gates — while the fail-closed + RefusingMergeJudge stays the default — plus the project_ready_prs gate #3/#5 + corrections (trusted-author admission and is_draft hydration) that let those + PRs reach the ready set in the first place. +last_updated: 2026-07-21 +review_schedule: as-needed +owner: simard +doc_type: concept +status: implemented +related: + - ./autonomous-merge-review-gate.md + - ./autonomous-self-merge-sensor.md + - ./draft-pr-merge-exclusion.md + - ../reference/objective-merge-judge-api.md + - ../reference/autonomous-merge-review-gate.md + - ../reference/ready-prs-sensor-api.md + - ../reference/draft-pr-exclusion-gate.md + - ../reference/cross-repo-merge-authority.md + - ../howto/enable-objective-merge-fallback.md + - ../howto/triage-stale-pull-requests.md + - ../../src/stewardship/objective_merge_judge.rs + - ../../src/stewardship/merge_judge.rs + - ../../src/overseer/mod.rs +--- + +# Concept: objective merge-judge fallback + +> **Status: implemented.** The `ObjectiveMergeJudge` tier and the +> `MergeJudgeKind::Objective` selector live in +> [`src/stewardship/objective_merge_judge.rs`](https://github.com/rysweet/Simard/blob/main/src/stewardship/objective_merge_judge.rs) +> and +> [`src/stewardship/merge_judge.rs`](https://github.com/rysweet/Simard/blob/main/src/stewardship/merge_judge.rs); +> the `project_ready_prs` gate #3/#5 corrections live in +> [`src/overseer/mod.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/mod.rs). +> The daemon keeps `RefusingMergeJudge` as the default judge; the objective +> fallback activates **only** when `SIMARD_MERGE_OBJECTIVE_FALLBACK` is set. +> See the [objective merge-judge API reference](../reference/objective-merge-judge-api.md) +> for the typed surface and the +> [enable howto](../howto/enable-objective-merge-fallback.md) to turn it on. + +> Delivery-ready pull requests — green (`mergeStateStatus=CLEAN`), `MERGEABLE`, +> non-draft, rysweet-authored, and owned by no in-flight engineer — now +> **converge to merged** on their own. Previously they were selected (or worse, +> silently dropped before selection) and then re-escalated every Overseer tick +> without ever merging. This concept explains the two-part bug and the additive, +> fail-closed-by-default fix. + +## The problem this solves + +Across many Overseer ticks, ~16 green/mergeable/non-in-flight PRs +(for example `#4389`, `#4344`, `#4145`) stayed unmerged and the delivery +step re-escalated the *same* PRs tick after tick. CI was green: the bottleneck +was the **merge/delivery automation**, not the checks. Two independent defects +compounded: + +### 1. The judge refused everything (`build_merge_judge` fallback) + +The merge authority runs a downstream **merge-judge** as the sole review step +(see the [autonomous-merge review gate](./autonomous-merge-review-gate.md)). +When no reviewer/LLM/recipe provider is wired, +[`build_merge_judge()`](https://github.com/rysweet/Simard/blob/main/src/stewardship/merge_judge.rs) +falls back to **`RefusingMergeJudge`**, which returns `Verdict::NotReady` for +*every* PR. That is the correct **fail-closed** default — a daemon with no review +capability must not merge — but it means every green PR is refused and bounced +back to escalation. Nothing converges. + +The pivot was fail-closed review, **not** `allow_verify_merge` (which is +correctly `true`). Turning off review safety wholesale would be wrong. The fix +instead adds a **narrow, opt-in, still-objective** judgment path. + +### 2. Eligible PRs never reached the ready set (`project_ready_prs` gates) + +Even before the judge, the Overseer's `project_ready_prs` producer silently +dropped eligible PRs: + +- **Gate #3 (engineer-PR requirement)** required an engineer label or an + engineer branch prefix. rysweet-authored non-engineer PRs — exactly the + delivery-ready ones in the escalation loop — failed this gate and were never + projected as ready. +- **Gate #5 (draft filter)** fails **closed** when `isDraft` is absent from the + listing JSON (`None`). If the projection JSON did not hydrate `is_draft`, a + perfectly non-draft PR was excluded as if its draft state were unknown. + +## The fix (additive, fail-closed by default) + +Two coordinated, non-breaking changes restore convergence without weakening the +default safety posture. + +### A non-refusing objective tier — opt-in only + +A new **`ObjectiveMergeJudge`** tier returns `Verdict::Ready` for a PR that is +**authored by a trusted (allowlisted) author** and has **already passed every +objective gate** (CI-green, `MERGEABLE`, base + repo allow-lists, non-draft). +It performs no LLM review; it replaces only the *judgment half* for trusted +authors, and the objective gates remain mandatory and unchanged. + +```mermaid +flowchart TD + A[merge() step 3: build_merge_judge()] --> B{SIMARD_MERGE_OBJECTIVE_FALLBACK set?} + B -- no (default) --> C[RefusingMergeJudge → Verdict::NotReady] + B -- yes --> D{author.login in SIMARD_MERGE_TRUSTED_AUTHORS?} + D -- no --> C + D -- yes, and past objective gates --> E[ObjectiveMergeJudge → Verdict::Ready] + C --> F[Escalate — not merged] + E --> G[Squash-merge] +``` + +The default is unchanged: with `SIMARD_MERGE_OBJECTIVE_FALLBACK` **unset**, +`build_merge_judge()` still returns `RefusingMergeJudge` and the daemon is +fail-closed exactly as before. + +### Corrected selection gates + +- **Gate #3** additionally admits **trusted-author** (allowlisted) PRs even when + they carry no engineer label/branch, so delivery-ready rysweet PRs reach + `ready_prs`. +- **Gate #5** hydrates `is_draft` from the listing JSON so a known non-draft PR + is admitted; the fail-closed `None` semantics are preserved (an *absent* + `isDraft` still excludes, per the + [draft-PR exclusion gate](./draft-pr-merge-exclusion.md)). + +## Why this is safe + +- **Fail-closed default.** Unset env ⇒ `RefusingMergeJudge`. The objective + fallback is strictly opt-in. +- **Objective gates stay mandatory.** The fallback never bypasses CI-green, + `MERGEABLE`, base/repo allow-lists, or the draft exclusion. It replaces only + the review verdict, and only for trusted authors. +- **Authenticated identity, not spoofable text.** Trust is matched against the + authenticated `author.login` (exact equality) — carried in the additive + `PrSnapshot.author_login` field, hydrated from the existing + `gh pr view --json ...,author` call — never a PR title, body, or trailer. An + absent author object hydrates to an empty login and fails closed. +- **No self-merge loop.** The daemon's own bot identity is excluded from the + trusted-author allowlist, preserving the anti-recursion author guard. +- **No override flags.** The merge still runs argv-only `gh` with **no** + `--admin` / `--no-verify`; the human-review label gate and every existing + invariant remain in force. + +## Related + +- [Autonomous-merge review gate (agentic merge-judge)](./autonomous-merge-review-gate.md) + — the review authority this tier plugs into. +- [Autonomous self-merge sensor (`ready_prs` wire)](./autonomous-self-merge-sensor.md) + — the Observe-path sensor that feeds the candidate set. +- [Draft-PR merge exclusion](./draft-pr-merge-exclusion.md) — the fail-closed + draft rule gate #5 preserves. +- [Objective merge-judge API reference](../reference/objective-merge-judge-api.md) + — the typed surface, env config, and edge-case matrix. +- [Enable the objective merge-judge fallback](../howto/enable-objective-merge-fallback.md) + — how to turn it on for a canary and verify convergence. diff --git a/docs/howto/enable-objective-merge-fallback.md b/docs/howto/enable-objective-merge-fallback.md new file mode 100644 index 000000000..b0574bcc0 --- /dev/null +++ b/docs/howto/enable-objective-merge-fallback.md @@ -0,0 +1,133 @@ +--- +title: Enable the objective merge-judge fallback (converge delivery-ready PRs) +description: > + How to turn on the opt-in objective merge-judge tier so Simard actually merges + green, mergeable, non-in-flight, trusted-author PRs instead of re-escalating + them every Overseer tick — set SIMARD_MERGE_OBJECTIVE_FALLBACK and + SIMARD_MERGE_TRUSTED_AUTHORS, canary one repo, verify prs_merged advances, + confirm the fail-closed default, and roll back. +last_updated: 2026-07-21 +review_schedule: as-needed +owner: simard +doc_type: howto +status: implemented +related: + - ../concepts/objective-merge-judge-fallback.md + - ../reference/objective-merge-judge-api.md + - ../reference/autonomous-merge-review-gate.md + - ../reference/ready-prs-sensor-api.md + - ./enable-autonomous-self-merge-canary.md + - ./triage-stale-pull-requests.md + - ./diagnose-merge-pr-verdict-parse-failures.md +--- + +# Enable the objective merge-judge fallback + +> **Goal.** Turn on the opt-in **objective merge-judge** so delivery-ready PRs — +> green (`mergeStateStatus=CLEAN`), `MERGEABLE`, non-draft, authored by a +> **trusted** author, and owned by no in-flight engineer — actually **merge** +> instead of being re-escalated every Overseer tick. The default is fail-closed +> (`RefusingMergeJudge`); this is a deliberate, reversible opt-in. + +For the *why* and the safety model, read +[the objective merge-judge fallback concept](../concepts/objective-merge-judge-fallback.md); +for the typed surface and edge-case matrix, see +[the API reference](../reference/objective-merge-judge-api.md). + +## Before you start + +- Confirm the symptom this addresses: the delivery step **selects** or + **escalates** the same green PRs every tick without merging. Check the + Overseer activity feed / `merge_judge_kind` telemetry — if it reads + `refusing`, no review authority is wired and every green PR is refused. +- Ensure the objective gates you rely on are genuinely green (CI, `MERGEABLE`, + base/repo allow-lists). The fallback replaces only the **judgment** half; it + never bypasses those gates. +- `gh` authenticated as the daemon identity; the daemon's own bot login is + **never** trusted (no self-merge). + +## Steps + +### 1. Choose the trusted authors + +Set the allowlist to the human/owner logins whose green PRs you want landed. +Matching is against the **authenticated `author.login`** (exact, lowercased) — +not any PR text. The default is `rysweet`. + +```bash +export SIMARD_MERGE_TRUSTED_AUTHORS=rysweet +``` + +Multiple authors are comma-separated (`rysweet,other-login`). Entries with +whitespace or `/` are rejected and logged. + +### 2. Enable the fallback + +```bash +export SIMARD_MERGE_OBJECTIVE_FALLBACK=1 +``` + +Accepted truthy values (case-insensitive): `1`, `true`, `yes`, `on`. Anything +else — or unset — keeps `RefusingMergeJudge`. + +### 3. Canary one repo first + +Scope the autonomous-merge repo allowlist to a single repo before a fleet-wide +rollout (see +[Enable autonomous self-merge (canary one repo)](./enable-autonomous-self-merge-canary.md)), +then start (or restart) the daemon so it re-reads the environment. + +For systemd deployments, add the two variables to the unit's environment and +reload: + +```ini +# /etc/systemd/system/simard-overseer.service (drop-in) +[Service] +Environment=SIMARD_MERGE_OBJECTIVE_FALLBACK=1 +Environment=SIMARD_MERGE_TRUSTED_AUTHORS=rysweet +``` + +```bash +sudo systemctl daemon-reload +sudo systemctl restart simard-overseer +``` + +### 4. Verify convergence + +Confirm the tier switched and PRs actually merge: + +- **Telemetry:** `merge_judge_kind` now reports `objective` (was `refusing`). +- **Outcome:** `prs_merged` advances across ticks; the previously re-escalated + green PRs disappear from the delivery/escalation set. +- **Selection:** trusted-author non-engineer PRs now appear in `ready_prs` + (gate #3 admission) and known-non-draft PRs are no longer dropped (gate #5 + `is_draft` hydration). + +```bash +# The green, mergeable, trusted-author PR that used to loop should now be MERGED. +gh pr view -R rysweet/Simard --json state,mergeStateStatus,author,isDraft +``` + +## Verify the fail-closed default still holds + +Unsetting the switch must return the daemon to refusing every PR: + +```bash +unset SIMARD_MERGE_OBJECTIVE_FALLBACK # or set to 0/false +# restart daemon → merge_judge_kind == refusing, no autonomous merges +``` + +## Roll back + +Remove both variables (or set `SIMARD_MERGE_OBJECTIVE_FALLBACK=0`) from the unit +environment and restart. No data migration is involved — the tier selection is +recomputed at boot. + +## Troubleshooting + +| Symptom | Likely cause | Fix | +|---|---|---| +| Still `refusing` after enabling | Trusted set empty/all-invalid, or a recipe-/LLM-backed judge is wired and takes precedence | Check `SIMARD_MERGE_TRUSTED_AUTHORS`; review `tracing` warns for rejected entries | +| Trusted PR still `NotReady` | An objective gate is red (CI, conflict, draft) — the pre-filter blocks upstream | Fix the gate; the judge only runs past objective gates | +| Bot's own PR not merging | Bot identity is excluded by design (no self-merge) | Expected — use a human trusted author | +| Green PR never enters `ready_prs` | Gate #3/#5 — author not trusted or `isDraft` absent from JSON | Add the author to the allowlist; confirm the listing hydrates `isDraft` | diff --git a/docs/reference/done-gate-slug-dedup-api.md b/docs/reference/done-gate-slug-dedup-api.md new file mode 100644 index 000000000..410af7bf4 --- /dev/null +++ b/docs/reference/done-gate-slug-dedup-api.md @@ -0,0 +1,157 @@ +--- +title: Done-gate slug dedup API reference +description: > + The typed surface of the slug-keyed done-gate convergence that makes a + completed goal converge on a single done-gate PR — the injected PR-lister seam, + the sanitize_goal_slug() helper ([a-z0-9-]), the keep-oldest-CLEAN selection, + the bot-author-AND-exact-slug supersede scoping, the stale-CONFLICTING + out-of-flight branch pruning, idempotency, and the edge-case matrix. +last_updated: 2026-07-21 +owner: simard +doc_type: reference +status: partially implemented +related: + - ../concepts/done-gate-slug-convergence.md + - ../concepts/deploy-aware-done-gate.md + - ./completion-evidence-gate-api.md + - ./goal-board-api.md + - ../howto/triage-stale-pull-requests.md + - ../../src/goal_curation/completion_gate.rs + - ../../src/goal_curation/advance_goal/spawn.rs + - ../../src/goal_curation/advance_goal/goal_session.rs +--- + +# Done-gate slug dedup API reference + +> **Status: partially implemented.** The convergence, the `sanitize_goal_slug()` +> helper, and the ownership-scoped supersede decision below are implemented and +> unit-tested (over an injected PR-lister) in +> [`src/goal_curation/completion_gate.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/completion_gate.rs). +> Wiring these decisions into the advance-goal spawn/session path +> ([`advance_goal/spawn.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/advance_goal/spawn.rs), +> [`advance_goal/goal_session.rs`](https://github.com/rysweet/Simard/blob/main/src/goal_curation/advance_goal/goal_session.rs)) +> is a tracked follow-up and is **not yet integrated**. This page specifies the +> typed surface that wiring will consume. + +This reference specifies the API of the slug-keyed done-gate convergence. For +the *why* and the safety narrative, see +[the done-gate slug convergence concept](../concepts/done-gate-slug-convergence.md). + +**One-line summary:** given a goal slug, the done-gate keeps the **oldest +`CLEAN`** bot-authored done-gate PR matching that slug and supersedes/closes the +rest (bot-author **and** exact-slug scoped); it prunes stale +`CONFLICTING`/`DIRTY` engineer branches for out-of-flight slugs by logic. + +## Contents + +- [`PrLister` seam](#prlister-seam) +- [`sanitize_goal_slug()`](#sanitize_goal_slug) +- [`converge_done_gate_prs()`](#converge_done_gate_prs) +- [Supersede scoping](#supersede-scoping) +- [Idempotency](#idempotency) +- [Edge-case matrix](#edge-case-matrix) + +## `PrLister` seam + +Convergence is written against an **injected** PR-lister so it is unit-testable +without live `gh`. Production wires the `gh`-backed implementation. + +```rust +pub struct OpenPr { + pub number: u32, + pub author_login: String, // authenticated GitHub login + pub head_branch: String, // used to match the goal slug + pub merge_state: MergeState, // Clean | Conflicting | Dirty | Unknown + pub created_at: OffsetDateTime, // "oldest CLEAN" tiebreak + pub is_done_gate: bool, // done-gate PR vs. engineer branch +} + +pub trait PrLister: Send + Sync { + /// Open PRs whose head branch carries the given sanitized slug prefix. + fn list_for_slug(&self, repo: &str, slug: &str) -> SimardResult>; +} +``` + +## `sanitize_goal_slug()` + +The goal slug is sanitized **before** any branch/argv/path use. Only +`[a-z0-9-]` survives; everything else (uppercase, whitespace, `.`, `/`, `..`, +shell metacharacters) is stripped or rejected. + +```rust +/// Lowercase; keep only [a-z0-9-]; collapse repeated '-'; trim leading/trailing '-'. +/// Returns None for an empty result (⇒ convergence is skipped, no unscoped close). +pub fn sanitize_goal_slug(raw: &str) -> Option { /* … */ } +``` + +An empty sanitized slug **skips** convergence entirely — the gate never falls +back to an unscoped supersede. + +## `converge_done_gate_prs()` + +```rust +pub struct ConvergeOutcome { + pub kept: Option, // the single surviving done-gate PR + pub superseded: Vec, // done-gate PRs closed as duplicates + pub pruned_branches: Vec,// stale CONFLICTING/DIRTY out-of-flight branches +} + +/// Converge the open PRs for one goal slug onto a single done-gate PR. +/// - keeps the OLDEST Clean done-gate PR authored by `bot_login`, +/// - supersedes/closes the remaining bot-authored, exact-slug done-gate PRs, +/// - prunes stale Conflicting/Dirty engineer branches when the slug is NOT in `inflight`. +/// Pure decision + scoped side-effects; no `--admin`, argv-only `gh`. +pub fn converge_done_gate_prs( + lister: &dyn PrLister, + repo: &str, + slug: &str, // caller passes the sanitized slug + bot_login: &str, + inflight: &InflightRefs, +) -> SimardResult { /* … */ } +``` + +**Selection rule.** Among done-gate PRs (`is_done_gate == true`) with +`merge_state == Clean`, keep the one with the earliest `created_at`. If none is +`Clean`, keep/open a single done-gate PR and supersede any other bot-authored +slug-matching done-gate PRs. + +## Supersede scoping + +A PR is superseded/closed **only** when **all** of the following hold — a +deliberately conjunctive guard so the fix can never touch a human or unrelated +PR: + +1. `pr.author_login == bot_login` (Simard's bot identity), **and** +2. the PR's head branch matches the **exact sanitized slug** prefix, **and** +3. `pr.is_done_gate` (for the supersede path) or it is a stale + `Conflicting`/`Dirty` engineer branch for an **out-of-flight** slug (for the + prune path), **and** +4. it is not the kept PR. + +Closes are argv-only `gh` with a supersede note referencing the kept PR — no +`--admin`, no force. + +## Idempotency + +Re-running convergence on an already-converged goal is a **no-op**: the single +kept PR is re-selected and there is nothing left to supersede or prune. This +makes the gate safe to run on every done-gate tick. + +## Edge-case matrix + +| Situation | Result | +|---|---| +| 3 competing `CLEAN` done-gate PRs, one slug | Oldest kept; other two superseded | +| No `CLEAN` done-gate PR | Keep/open one; supersede other bot+slug done-gate PRs | +| Human-authored PR matching slug | Never touched (author ≠ bot) | +| Bot PR for a **different** slug | Never touched (exact-slug scope) | +| Stale `CONFLICTING` engineer branch, slug **in** flight | Left alone (goal still active) | +| Stale `CONFLICTING` engineer branch, slug **out of** flight | Pruned | +| Empty/invalid slug after sanitization | Convergence skipped (no unscoped close) | +| Already converged (single kept PR) | No-op (idempotent) | + +## Telemetry + +Each convergence emits a structured `tracing` event (OTel) with the kept PR, +superseded set, and pruned branches. No `println!`, no secrets, and the raw +(unsanitized) slug is never used in an argv or path. diff --git a/docs/reference/objective-merge-judge-api.md b/docs/reference/objective-merge-judge-api.md new file mode 100644 index 000000000..72fa5738f --- /dev/null +++ b/docs/reference/objective-merge-judge-api.md @@ -0,0 +1,298 @@ +--- +title: Objective merge-judge fallback API reference +description: > + The typed surface of the opt-in objective merge-judge tier that lets + build_merge_judge() return a non-refusing merge authority for trusted authors + past the objective gates — the ObjectiveMergeJudge type, the + MergeJudgeKind::Objective variant and is_configured(), the + SIMARD_MERGE_OBJECTIVE_FALLBACK and SIMARD_MERGE_TRUSTED_AUTHORS environment + config with hardened parsing, the additive PrSnapshot.author_login field that + gives the judge the authenticated author, the project_ready_prs gate #3 + trusted-author admission and gate #5 is_draft hydration, and the full + fail-closed / edge-case matrix. +last_updated: 2026-07-21 +owner: simard +doc_type: reference +status: implemented +related: + - ../concepts/objective-merge-judge-fallback.md + - ../concepts/autonomous-merge-review-gate.md + - ./autonomous-merge-review-gate.md + - ./ready-prs-sensor-api.md + - ./draft-pr-exclusion-gate.md + - ./cross-repo-merge-authority.md + - ../howto/enable-objective-merge-fallback.md + - ../../src/stewardship/objective_merge_judge.rs + - ../../src/stewardship/merge_judge.rs + - ../../src/stewardship/merge_authority.rs + - ../../src/overseer/mod.rs +--- + +# Objective merge-judge fallback API reference + +> **Status: implemented.** The `ObjectiveMergeJudge`, the +> `MergeJudgeKind::Objective` variant, and the env resolvers below live in +> [`src/stewardship/objective_merge_judge.rs`](https://github.com/rysweet/Simard/blob/main/src/stewardship/objective_merge_judge.rs) +> and +> [`src/stewardship/merge_judge.rs`](https://github.com/rysweet/Simard/blob/main/src/stewardship/merge_judge.rs); +> the `project_ready_prs` gate corrections live in +> [`src/overseer/mod.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/mod.rs). +> The judgment-path selection is covered by unit tests (author-spoof rejection, +> bot self-merge exclusion, default fail-closed, gate admission). + +This reference specifies the API, configuration, and edge-case matrix of the +opt-in objective merge-judge tier. For the *why* and the safety narrative, see +[the objective merge-judge fallback concept](../concepts/objective-merge-judge-fallback.md). + +**One-line summary:** with `SIMARD_MERGE_OBJECTIVE_FALLBACK` set, +`build_merge_judge()` resolves to an `ObjectiveMergeJudge` that returns +`Verdict::Ready` for trusted-author PRs already past the objective gates; +otherwise it stays `RefusingMergeJudge` (fail-closed). + +## Contents + +- [`MergeJudgeKind`](#mergejudgekind) +- [`ObjectiveMergeJudge`](#objectivemergejudge) +- [Required `PrSnapshot` extension](#required-prsnapshot-extension-committed-design) +- [`build_merge_judge()` resolution](#build_merge_judge-resolution) +- [Environment configuration](#environment-configuration) +- [`project_ready_prs` gate corrections](#project_ready_prs-gate-corrections) +- [Edge-case & fail-closed matrix](#edge-case-and-fail-closed-matrix) +- [Telemetry](#telemetry) + +## `MergeJudgeKind` + +An additive enum that names the resolved judgment tier for selection and +telemetry. Existing variants are unchanged; `Objective` is new. + +```rust +/// Which merge-judgment tier build_merge_judge() resolved to. +/// Existing variants (Llm, Recipe, Refusing) are UNCHANGED; `Objective` is new. +/// Keeps the existing `#[serde(rename_all = "snake_case")]` telemetry vocabulary. +pub enum MergeJudgeKind { + /// LlmMergeJudge — production impl backed by an LLM provider (unchanged). + Llm, + /// RecipeMergeJudge — recipe-runner-rs backed impl (unchanged). + Recipe, + /// Fail-closed default: refuses every PR (Verdict::NotReady) (unchanged). + Refusing, + /// Opt-in objective tier: Ready for trusted-author PRs past objective gates. + Objective, +} + +impl MergeJudgeKind { + /// Whether this tier can issue a non-refusal verdict. The existing contract + /// (`Llm | Recipe`) is widened to include `Objective`; drives the dashboard + /// `judge_configured` field and the `merge_judge_kind` telemetry label. + /// Takes `self` by value (the enum is `Copy`), matching the current impl. + pub fn is_configured(self) -> bool { + matches!( + self, + MergeJudgeKind::Llm | MergeJudgeKind::Recipe | MergeJudgeKind::Objective + ) + } +} +``` + +## `ObjectiveMergeJudge` + +A `MergeJudge` implementation that performs **no** LLM/recipe review. It returns +`Verdict::Ready` **only** when both hold: + +1. The PR's authenticated `author.login` — read from `PrSnapshot.author_login` + (see [the required extension below](#required-prsnapshot-extension-committed-design)) — + is in the trusted-author allowlist (exact equality, lowercased), and is + **not** the daemon's own bot identity. +2. The PR has already passed every objective gate upstream (CI-green, + `MERGEABLE`, base + repo allow-lists, non-draft). + +Otherwise it returns `Verdict::NotReady` with a structured reason. + +```rust +pub struct ObjectiveMergeJudge { + trusted_authors: BTreeSet, // lowercased logins; bot identity excluded + bot_login: String, // never trusted (anti self-merge) +} + +impl ObjectiveMergeJudge { + /// Build from the resolved env config. Returns None if the trusted-author + /// set is empty after excluding the bot identity (⇒ caller keeps Refusing). + pub fn from_env(bot_login: &str) -> Option { /* … */ } +} + +impl MergeJudge for ObjectiveMergeJudge { + fn judge( + &self, + pr_number: u32, + repo: &str, + snapshot: &PrSnapshot, + ) -> SimardResult { + // Ready iff the AUTHENTICATED author (snapshot.author_login, see the + // PrSnapshot extension below) is trusted and is not the bot: + // let author = snapshot.author_login.to_lowercase(); + // if !author.is_empty() + // && author != self.bot_login + // && self.trusted_authors.contains(&author) + // => JudgeOutcome { verdict: Verdict::Ready, rationale, blockers: vec![] } + // else + // => JudgeOutcome { verdict: Verdict::NotReady, rationale, blockers } + // An empty author_login (author object missing from the API) fails closed. + } + + /// Required by the `MergeJudge` trait (used by the dashboard without + /// invoking the judge). Reports the objective tier. + fn kind(&self) -> MergeJudgeKind { + MergeJudgeKind::Objective + } +} +``` + +### Required `PrSnapshot` extension (committed design) + +`judge()` receives only `pr_number`, `repo`, and +[`PrSnapshot`](https://github.com/rysweet/Simard/blob/main/src/stewardship/merge_authority.rs), +which today carries **no** author field (`body`, `mergeable`, `review_decision`, +`checks`, `base_ref_name`, `labels`). Because the merge authority consults the +**judge itself** as the sole review step, the judge must return `Verdict::Ready` +on its own — a trusted-author admission in `project_ready_prs` gate #3 only lets +the PR *reach* the ready set; it does not make `RefusingMergeJudge` (or any +judge) say Ready. Gate #3 alone therefore **cannot** deliver the merge. + +The committed design is to **add an `author_login` field to `PrSnapshot`** and +hydrate it from the *existing* `gh pr view` call by adding `author` to its +`--json` field list — no new `gh` invocation, no new token scope: + +```rust +// src/stewardship/merge_authority.rs — additive field (default = "" ⇒ fail-closed) +pub struct PrSnapshot { + pub body: String, + pub mergeable: String, + pub review_decision: String, + pub checks: Vec, + pub base_ref_name: String, + pub labels: Vec, + /// `author.login` from `gh pr view --json ...,author`. Empty when the + /// author object is absent from the API response ⇒ the objective judge + /// fails closed (never Ready). Never sourced from `body`/title/trailers. + pub author_login: String, +} +``` + +The hydration site changes from +`gh pr view --repo --json body,statusCheckRollup,mergeable,reviewDecision,baseRefName,labels` +to `...,labels,author`, parsing `author.login` into the new field. This is +additive and non-breaking: every existing caller keeps compiling (the field +defaults to `""`), and the LLM/recipe/refusing judges simply ignore it. The +judge must **never** infer the author from the spoofable `PrSnapshot.body`. + +> The objective gates are evaluated **before** the judge is consulted (in the +> merge authority / `verify()` objective pre-filter). `ObjectiveMergeJudge` +> assumes they passed; it never re-opens or bypasses them. + +## `build_merge_judge()` resolution + +The resolver is a fail-closed cascade. Its **signature is unchanged** — +`build_merge_judge() -> Box` — because all three callers +(`merge_authority`, `merge_ops`, and `merge_readiness`, the last via +`build_merge_judge().kind()`) depend on it; the resolved tier is read back +through [`MergeJudge::kind()`](#mergejudgekind), not a returned tuple. The +`Objective` branch is inserted **only** ahead of the refusing default and +**only** when explicitly enabled. + +```rust +// Signature UNCHANGED (no MergeJudgeConfig param, no tuple return): the tier is +// read via `.kind()`. Objective is inserted only before the Refusing default. +pub fn build_merge_judge() -> Box { + let repo_root = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); + // 1. Recipe-runner-rs judge, if binary + recipe are available (unchanged). + if let Some(j) = RecipeMergeJudge::new(&repo_root) { + tracing::info!("merge-judge: using recipe-runner-rs backed judge"); + return Box::new(j); + } + // 2. Direct LLM judge, if a provider resolves (unchanged). + if let Ok(provider) = LlmProvider::resolve() { + return Box::new(LlmMergeJudge::new(SessionLlmSubmitter::new(provider))); + } + // 3. Opt-in objective fallback — trusted authors past objective gates. + // `bot_login` comes from the overseer identity (overseer_login()), NOT a + // new config type. from_env() → None ⇒ fall through to Refusing. + if objective_fallback_enabled() { + if let Some(j) = ObjectiveMergeJudge::from_env(overseer_login()) { + return Box::new(j); + } + } + // 4. Fail-closed default (unchanged). NOTE: this fix also replaces the + // existing stray `eprintln!` in this function with `tracing`. + Box::new(RefusingMergeJudge) +} +``` + +## Environment configuration + +Both variables are parsed with the hardened env pattern (trimmed, case-insensitive +boolean, CSV split with per-entry validation). Neither grants new token scopes. + +| Variable | Type | Default | Meaning | +|---|---|---|---| +| `SIMARD_MERGE_OBJECTIVE_FALLBACK` | bool | **off** (unset) | Master switch. `1`/`true`/`yes`/`on` (case-insensitive) enables the objective tier in `build_merge_judge()`. Any other value, or unset, keeps `RefusingMergeJudge`. | +| `SIMARD_MERGE_TRUSTED_AUTHORS` | CSV of logins | `rysweet` | Allowlist of authenticated `author.login`s eligible for `Verdict::Ready`. Compared lowercased, exact-match. The daemon's bot identity is always removed from this set. | + +**Parsing rules (hardened):** + +- Whitespace around the whole value and each CSV entry is trimmed. +- Empty entries are dropped; a value that reduces to an empty set leaves the + daemon on `RefusingMergeJudge`. +- An entry containing whitespace, `/`, or other non-login characters is + **rejected** (logged via `tracing::warn!`, entry skipped) — never used to + build an argv or branch. +- The bot login is excluded even if explicitly listed (no self-merge). + +## `project_ready_prs` gate corrections + +Two narrowing/hydration fixes in `project_ready_prs` (in `src/overseer/mod.rs`) +let delivery-ready PRs reach the ready set. Every other gate (G2 author, +objective gates, MergeJudge) is unchanged. + +### Gate #3 — trusted-author admission + +Previously gate #3 required an engineer label **or** engineer branch prefix. +It now **also** admits a PR whose authenticated `author.login` is in +`SIMARD_MERGE_TRUSTED_AUTHORS`, so rysweet-authored non-engineer PRs are no +longer silently dropped. + +```text +admit if is_engineer_pr(pr) OR trusted_authors.contains(pr.author_login) +``` + +### Gate #5 — `is_draft` hydration + +The projection now hydrates `ProjectionCandidate.is_draft` from the `isDraft` +field of the listing JSON. The fail-closed `Option` semantics are +**preserved**: admit only `Some(false)`; exclude `Some(true)` and `None`. The +bug fixed here is that `is_draft` was previously left `None` for known-non-draft +PRs, causing fail-closed exclusion of eligible PRs. See the +[draft-PR exclusion gate](./draft-pr-exclusion-gate.md). + +## Edge-case and fail-closed matrix + +| Situation | Result | +|---|---| +| `SIMARD_MERGE_OBJECTIVE_FALLBACK` unset | `RefusingMergeJudge` (fail-closed) — no change from before | +| Fallback on, author **not** in allowlist | `Verdict::NotReady` → escalate | +| Fallback on, author **is** the bot identity | Excluded from allowlist → `NotReady` (no self-merge) | +| Fallback on, author object missing from API (`author_login` empty) | Fail-closed → `NotReady` (never Ready on an unverifiable author) | +| Fallback on, spoofed trailer/body claims trusted author | Ignored — only authenticated `author.login` is matched → `NotReady` unless the login itself is trusted | +| Fallback on, trusted author, objective gate fails (red CI, conflict, draft) | Objective pre-filter blocks upstream; judge never returns Ready | +| `SIMARD_MERGE_TRUSTED_AUTHORS` empty / all invalid | Objective tier not built → `RefusingMergeJudge` | +| Recipe/LLM judge wired | Takes precedence; objective tier not consulted | +| Gate #3: rysweet PR, no engineer label | Admitted via trusted-author branch of gate #3 | +| Gate #5: `isDraft` absent from JSON | `None` → excluded (fail-closed preserved) | + +## Telemetry + +- `merge_judge_kind` label (`llm` / `recipe` / `refusing` / `objective`, the + snake_case `serde` tags of `MergeJudgeKind`) is emitted on the merge-judgment + metric so operators can confirm which tier fired. +- Objective-tier `Ready` verdicts and every rejected/invalid trusted-author + entry are recorded with structured `tracing` fields (OTel) — no `println!`, + no secrets, and never the PR body. diff --git a/docs/reference/self-deploy-head-advance-dedup.md b/docs/reference/self-deploy-head-advance-dedup.md new file mode 100644 index 000000000..69a349c5f --- /dev/null +++ b/docs/reference/self-deploy-head-advance-dedup.md @@ -0,0 +1,219 @@ +--- +title: Self-deploy head-advance & per-SHA dedupe API reference +description: > + The additive self-deploy reconcile surface that advances the running head to + the merged base-allowlist head, dedupes redeploys per target SHA to stop + self-deploy thrash, validates the target SHA as lowercase hex before any argv, + and reconciles the not-loaded overseer systemd unit. Documents the extended + file-backed SelfDeployState (last_deploy_target_sha / last_deploy_result), the + head-advance reconcile, the SHA validation helper, the systemd-unit reconcile, + and the SIMARD_OVERSEER_AUTONOMOUS_DEPLOY / SIMARD_OVERSEER_DEPLOY_MIN_INTERVAL_SECS + guards. Addresses #4390, #4387, #4305. +last_updated: 2026-07-21 +owner: simard +doc_type: reference +status: partially implemented +related: + - ../concepts/reconcile-and-self-deploy.md + - ./self-deploy-api.md + - ./self-deploy-source-prep.md + - ./overseer-operator-notifications.md + - ./overseer-tick-details.md + - ../safe-self-update.md + - ../howto/verify-and-roll-back-a-self-deploy.md + - ../howto/run-self-deploy-from-any-directory.md + - ../../src/self_deploy/head_advance.rs + - ../../src/self_deploy/orchestrator.rs + - ../../src/self_deploy/restart.rs + - ../../src/overseer/deploy.rs + - ../../src/overseer/deploy_trigger.rs +--- + +# Self-deploy head-advance & per-SHA dedupe API reference + +> **Status: partially implemented.** The pure decision layer — the extended +> `DeployHeadState` (`last_deploy_target_sha` / `last_deploy_result`), the +> head-advance reconcile decision, the SHA-validation helper, the per-SHA dedupe +> decision, and the systemd-unit-load classification — is implemented and +> unit-tested in +> [`src/self_deploy/head_advance.rs`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/head_advance.rs) +> (re-exported from `src/self_deploy/mod.rs`). The **effectful wiring** that +> consumes these decisions in +> [`src/self_deploy/orchestrator.rs`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/orchestrator.rs), +> [`src/self_deploy/restart.rs`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/restart.rs), +> [`src/overseer/deploy.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/deploy.rs) +> and +> [`src/overseer/deploy_trigger.rs`](https://github.com/rysweet/Simard/blob/main/src/overseer/deploy_trigger.rs) +> is a tracked follow-up and is **not yet integrated** into the running deploy +> loop. The dedupe, head-advance, SHA-rejection, and opt-out decision paths are +> covered by unit tests. This page **extends** the +> [self-deploy API reference](./self-deploy-api.md) and specifies the surface the +> wiring will consume. + +This reference specifies the additive reconcile surface that closes the +merged-but-undeployed head gap (issues +[#4390](https://github.com/rysweet/Simard/issues/4390), +[#4387](https://github.com/rysweet/Simard/issues/4387), +[#4305](https://github.com/rysweet/Simard/issues/4305)). For the rationale and +the end-to-end flow, see +[reconcile-and-self-deploy](../concepts/reconcile-and-self-deploy.md). + +**One-line summary (specified target behavior):** once the wiring lands, the +running head advances to the merged +base-allowlist head; each target SHA is deployed **at most once** (per-SHA +dedupe on top of the existing min-interval anti-thrash); the target SHA is +validated as lowercase hex before any `gh`/`systemctl` argv; and a **not-loaded** +overseer systemd unit is reconciled so deploys become service-managed. + +## The gap this closes + +`simard status` reported `DAEMON/UPTIME unavailable (systemctl: unit not +loaded)` — no service-managed deploy — while the running binary (`0.31.0`) +lagged the merged head (`0.33.1`). Self-deploy could also redeploy the **same** +head repeatedly (thrash). This surface adds three additive guards: +per-SHA dedupe, head-advance reconcile, and systemd-unit reconcile. + +## Contents + +- [`SelfDeployState` (extended)](#selfdeploystate-extended) +- [SHA validation](#sha-validation) +- [Head-advance reconcile](#head-advance-reconcile) +- [Per-SHA dedupe](#per-sha-dedupe) +- [systemd unit-not-loaded reconcile](#systemd-unit-not-loaded-reconcile) +- [Environment configuration](#environment-configuration) +- [Edge-case matrix](#edge-case-matrix) + +## `SelfDeployState` (extended) + +A small file-backed JSON state (mirroring the existing `SelfRelaunchState` in +[`restart.rs`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/restart.rs)) +records the last **target** SHA and its result, enabling per-SHA dedupe and +head-advance reconciliation across restarts. + +```rust +#[derive(serde::Deserialize, serde::Serialize)] +struct SelfDeployState { + /// Last SHA the daemon attempted to deploy TO (40- or 64-char lowercase hex). + last_deploy_target_sha: Option, + /// Outcome of that attempt, for dedupe + operator reporting. + last_deploy_result: DeployResult, + /// Unix seconds of the last attempt (feeds the min-interval anti-thrash). + last_deploy_unix_secs: u64, +} + +#[derive(serde::Deserialize, serde::Serialize, Clone, PartialEq)] +enum DeployResult { + Succeeded, + Failed, + RolledBack, +} +``` + +**Durability contract.** The state file is written `0600`. An **unparseable** +state file is treated as *no known prior deploy* and, combined with the guards +below, results in **no deploy** rather than an unguarded one (fail-closed). + +## SHA validation + +Every target SHA is validated **before** it is placed on any argv (`gh`, +`git`, `systemctl`) or branch/path. + +```rust +/// True iff `s` is a 40- or 64-char all-lowercase hex string (git SHA-1/SHA-256). +/// Rejects uppercase, whitespace, refs, and anything that could inject an argv flag. +pub fn is_valid_deploy_sha(s: &str) -> bool { + (s.len() == 40 || s.len() == 64) && s.bytes().all(|b| b.is_ascii_hexdigit() && !b.is_ascii_uppercase()) +} +``` + +A SHA that fails validation aborts the deploy with a structured `tracing::warn!` +and **no** subprocess is spawned. + +## Head-advance reconcile + +The orchestrator reconciles the **running** head to the **merged +base-allowlist** head from the authenticated remote (`origin/main` root of +trust; see the +[security prerequisites](./self-deploy-api.md#security-prerequisites)). It only +advances to a verified head on the base allow-list — never an arbitrary or fork +ref — and only when that head differs from the running binary's head. + +```text +if merged_head != running_head + && is_valid_deploy_sha(merged_head) + && on_base_allowlist(merged_head) + && not deduped(merged_head) # see below + && min_interval_elapsed() # existing anti-thrash + && autonomous_deploy_enabled() +then deploy_to(merged_head) +``` + +## Per-SHA dedupe + +On top of the existing **min-interval** anti-thrash +(`SIMARD_OVERSEER_DEPLOY_MIN_INTERVAL_SECS`), the daemon **skips a target SHA it +has already successfully deployed**. This prevents redeploying the same head on +every tick (the #4387 self-deploy dedupe requirement). + +```text +deduped(sha) := + state.last_deploy_target_sha == Some(sha) + && state.last_deploy_result == Succeeded +``` + +A `Failed`/`RolledBack` result for the same SHA is **not** deduped — a genuine +retry is still allowed (subject to the min-interval guard), so a transient +failure does not permanently wedge the head. + +## systemd unit-not-loaded reconcile + +When the overseer systemd unit is **not loaded** (the `simard status` +`unit not loaded` condition), the reconcile step detects it and re-establishes +service management using **fixed** `systemctl` subcommands and a **constant** +unit name — no interpolated/user-derived unit strings. + +```rust +const OVERSEER_UNIT: &str = "simard-overseer.service"; + +// Detect: `systemctl --user is-active ` / is-enabled. +// Reconcile: load/enable the unit so the next deploy is service-managed. +``` + +The unit name is a compile-time constant; only fixed subcommands +(`is-active`, `is-enabled`, `restart`) are invoked. No external data is ever +interpolated into a `systemctl` argument. + +## Environment configuration + +These guards reuse the existing autonomous-deploy env surface; **no new opt-in +is required** for head-advance/dedupe (they are additive safety guards on the +already-existing autonomous path). + +| Variable | Type | Default | Meaning | +|---|---|---|---| +| `SIMARD_OVERSEER_AUTONOMOUS_DEPLOY` | bool (opt-out) | on | Set to `0` to disable autonomous drift-triggered deploy entirely. Honored by the head-advance reconcile — with `0`, no head-advance deploy is attempted. | +| `SIMARD_OVERSEER_DEPLOY_MIN_INTERVAL_SECS` | u64 seconds | (existing default) | Minimum wall-clock interval between deploy attempts (existing anti-thrash). Per-SHA dedupe stacks on top of this. | +| `SIMARD_SELF_DEPLOY_REPO` | string | auto-detected | Source repo override (existing; see [source prep](./self-deploy-source-prep.md)). | + +## Edge-case matrix + +| Situation | Result | +|---|---| +| `merged_head == running_head` | No deploy (already at head) | +| Same SHA already `Succeeded` | Deduped — skipped (no thrash) | +| Same SHA previously `Failed` | Retry allowed once min-interval elapses | +| Target SHA not lowercase hex / is a ref | Rejected before argv — no subprocess | +| Head not on base allow-list / from a fork | Rejected — not deployed | +| `SIMARD_OVERSEER_AUTONOMOUS_DEPLOY=0` | No head-advance deploy attempted | +| Min-interval not elapsed | Deferred until the window passes | +| systemd unit not loaded | Reconciled via fixed subcommands + constant unit name | +| `SelfDeployState` file unparseable | Treated as no prior deploy ⇒ fail-closed (no deploy) | + +## Telemetry + +Each reconcile decision (advance, dedupe-skip, SHA-reject, unit-reconcile, +opt-out) emits a structured `tracing` event (OTel) and — per the existing +"notify on every attempt" invariant — every actual deploy attempt notifies the +operator (see +[overseer operator notifications](./overseer-operator-notifications.md)). No +`println!`, no secrets, and SHAs are logged only after validation. diff --git a/mkdocs.yml b/mkdocs.yml index 902245cb4..ec5a87b61 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -101,6 +101,7 @@ nav: - Agentic Merge-Queue + Issue Reasoning (observe/orient): concepts/agentic-merge-queue-reasoning.md - Overseer Agentic Health-Review (self-heal crash-loops): concepts/overseer-agentic-health-review.md - Autonomous-Merge Review Gate (agentic merge-judge): concepts/autonomous-merge-review-gate.md + - Objective Merge-Judge Fallback (converge delivery-ready PRs): concepts/objective-merge-judge-fallback.md - Adaptive Scaling: concepts/adaptive-scaling.md - Pluggable Identity: concepts/pluggable-identity.md - Concierge Identity (Hospitality Design + Ops Software): concepts/concierge-identity.md @@ -121,6 +122,7 @@ nav: - Closing the Procedural-Learning Loop: concepts/procedural-learning-loop.md - Reconcile-and-Self-Deploy: concepts/reconcile-and-self-deploy.md - Deploy-Aware Done-Gate: concepts/deploy-aware-done-gate.md + - Done-Gate Slug Convergence (one goal → one done-gate PR): concepts/done-gate-slug-convergence.md - Closed-Loop Outcome Verification (live-verified done): concepts/closed-loop-outcome-verification.md - Dependency/Overlap-Aware Engineer Scheduling: concepts/dependency-overlap-aware-scheduling.md - Resource-Aware Engineer Admission: concepts/resource-aware-engineer-admission.md @@ -250,6 +252,7 @@ nav: - Fix CI Linker OOM: howto/fix-ci-linker-oom.md - Triage Stale Pull Requests: howto/triage-stale-pull-requests.md - Enable Autonomous Self-Merge (canary one repo): howto/enable-autonomous-self-merge-canary.md + - Enable the Objective Merge-Judge Fallback: howto/enable-objective-merge-fallback.md - Diagnose merge-pr Verdict-Parse Failures: howto/diagnose-merge-pr-verdict-parse-failures.md - Monitor Simard with the TUI: howto/monitor-simard-with-tui.md - Browse the Simard Journal: howto/browse-the-simard-journal.md @@ -321,6 +324,7 @@ nav: - Multi-Binary Self-Update: reference/multi-binary-self-update.md - Self-Deploy API: reference/self-deploy-api.md - Self-Deploy Source Prep & Warm Target Dir: reference/self-deploy-source-prep.md + - Self-Deploy Head-Advance & Per-SHA Dedupe: reference/self-deploy-head-advance-dedup.md - State-Root Resolution: reference/state-root-resolution.md - Operator Read State-Root Contract: reference/operator-read-state-root-contract.md - Runtime Contracts: reference/runtime-contracts.md @@ -405,6 +409,8 @@ nav: - ready_prs Sensor API: reference/ready-prs-sensor-api.md - Agentic Merge-Queue Reasoning API: reference/agentic-merge-queue-reasoning-api.md - Autonomous-Merge Review Gate API: reference/autonomous-merge-review-gate.md + - Objective Merge-Judge Fallback API: reference/objective-merge-judge-api.md + - Done-Gate Slug Dedup API: reference/done-gate-slug-dedup-api.md - "No Point-in-Time Report Docs Scan (pr-verify #8)": reference/no-point-in-time-docs-scan.md - Completion-Evidence Gate API: reference/completion-evidence-gate-api.md - Outcome-Verification API: reference/outcome-verification-api.md diff --git a/src/goal_curation/completion_gate.rs b/src/goal_curation/completion_gate.rs index 361bfe517..b31aa718e 100644 --- a/src/goal_curation/completion_gate.rs +++ b/src/goal_curation/completion_gate.rs @@ -770,5 +770,111 @@ pub fn archive_completed_evidence_aware( (archived, blocked) } +// ════════════════════════════════════════════════════════════════════════════ +// Done-gate slug convergence (#4326/#4329/#4332 churn) — P3 +// ════════════════════════════════════════════════════════════════════════════ +// +// A completed goal can accumulate MULTIPLE competing "done-gate" PRs for the +// same goal slug (retry churn without delivery). The convergence logic below is +// pure: it decides — by goal slug + bot ownership — which single done-gate PR to +// KEEP and which duplicates to SUPERSEDE, so the caller can close/supersede the +// extras via the gated PR-ops path (never a blind hand-close). It NEVER selects +// a human-authored PR or a PR belonging to a different goal slug. +// +// NOTE: this decision helper is not yet wired into the live stewardship loop; +// the effectful close/supersede caller lands as follow-up (#4326/#4329/#4332) +// in a separate, integration-testable change. + +/// Sanitise a goal slug down to the `[a-z0-9-]` alphabet before it is used in a +/// branch name, an argv value, or a path. Lowercases, drops every other byte +/// (so `..`, path separators, spaces, and shell metacharacters can never +/// survive), and trims leading/trailing `-` (so the result can never be parsed +/// as a flag). Idempotent: applying it to its own output is a no-op. +pub fn sanitize_goal_slug(raw: &str) -> String { + let filtered: String = raw + .to_ascii_lowercase() + .bytes() + .filter(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || *b == b'-') + .map(|b| b as char) + .collect(); + filtered.trim_matches('-').to_string() +} + +/// One done-gate PR candidate, projected from `gh pr list` for a completed +/// goal. `slug` is the goal slug the PR was opened for (parsed from its branch +/// or body); `author` is the AUTHENTICATED `author.login`. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DoneGatePr { + pub number: u32, + pub author: String, + pub slug: String, + /// `mergeable` from `gh` — `"MERGEABLE"` marks a CLEAN done-gate candidate. + pub mergeable: String, + /// `createdAt` (ISO-8601) — lexicographic order is chronological, so the + /// OLDEST CLEAN PR is the stable survivor. + pub created_at: String, +} + +/// The convergence decision for one goal slug: the single PR to KEEP and the +/// in-scope duplicates to SUPERSEDE. +#[derive(Clone, Debug, PartialEq, Eq, Default)] +pub struct SlugConvergence { + /// The single surviving done-gate PR (oldest CLEAN in-scope PR), or `None` + /// when no in-scope PR is mergeable (nothing is destroyed in that case). + pub keep: Option, + /// The in-scope duplicates (newer CLEAN + stale CONFLICTING) to supersede. + pub supersede: Vec, +} + +/// Converge the done-gate PRs for `target_slug` (authored by `bot_author`) onto +/// a single survivor. +/// +/// In-scope = a PR whose author matches `bot_author` (case-insensitive) AND +/// whose sanitised slug equals the sanitised `target_slug`. A human PR or a PR +/// for any other slug is never touched. +/// +/// * `keep` = the OLDEST CLEAN (`MERGEABLE`) in-scope PR. +/// * `supersede` = every OTHER in-scope PR (newer clean duplicates + stale +/// conflicting branches). +/// * If NO in-scope PR is mergeable, `keep` is `None` and `supersede` is empty +/// (fail-safe: the only representatives are left for the stale-goal path). +pub fn converge_done_gate_prs( + prs: &[DoneGatePr], + target_slug: &str, + bot_author: &str, +) -> SlugConvergence { + let target = sanitize_goal_slug(target_slug); + let in_scope: Vec<&DoneGatePr> = prs + .iter() + .filter(|p| p.author.eq_ignore_ascii_case(bot_author)) + .filter(|p| sanitize_goal_slug(&p.slug) == target) + .collect(); + + let keeper = in_scope + .iter() + .filter(|p| p.mergeable.eq_ignore_ascii_case("MERGEABLE")) + .min_by(|a, b| { + a.created_at + .cmp(&b.created_at) + .then_with(|| a.number.cmp(&b.number)) + }) + .map(|p| p.number); + + let Some(keep) = keeper else { + return SlugConvergence::default(); + }; + + let supersede = in_scope + .iter() + .map(|p| p.number) + .filter(|n| *n != keep) + .collect(); + + SlugConvergence { + keep: Some(keep), + supersede, + } +} + #[cfg(test)] mod tests; diff --git a/src/goal_curation/mod.rs b/src/goal_curation/mod.rs index db1b3baba..0d5903942 100644 --- a/src/goal_curation/mod.rs +++ b/src/goal_curation/mod.rs @@ -49,12 +49,12 @@ pub use prioritize::{PrioritizationSignals, prioritize}; pub use completion_gate::{ COMPLETION_VERIFICATION_METRIC, CompletionEvidence, CompletionEvidenceGate, CompletionVerdict, - DependencyState, EvidenceSource, FALSE_COMPLETION_RATE_METRIC, GhCliEvidenceSource, - MissingEvidence, VerificationOutcome, archive_completed_evidence_aware, + DependencyState, DoneGatePr, EvidenceSource, FALSE_COMPLETION_RATE_METRIC, GhCliEvidenceSource, + MissingEvidence, SlugConvergence, VerificationOutcome, archive_completed_evidence_aware, archive_completed_with_evidence, classify_from_missing, classify_outcome, - completion_evidence_enabled, error_class_from_missing, false_completion_rate, - has_derivable_signal, is_self_affecting, record_completion_verification, - record_false_completion_rate, + completion_evidence_enabled, converge_done_gate_prs, error_class_from_missing, + false_completion_rate, has_derivable_signal, is_self_affecting, record_completion_verification, + record_false_completion_rate, sanitize_goal_slug, }; pub use no_progress_breaker::{ @@ -105,6 +105,9 @@ mod tests_save_with_removals; // CallerKey dedup instead of accumulating live duplicates. #[cfg(test)] mod tests_snapshot_dedup; +// TDD (Step 7): failing tests for the P3 done-gate slug convergence / dedup. +#[cfg(test)] +mod tests_done_gate_dedup; // Issue #2405: goal decomposition + the typed goal-graph edge model. These // tests pin the durable edge format, the parent-linkage data model, diff --git a/src/goal_curation/tests_done_gate_dedup.rs b/src/goal_curation/tests_done_gate_dedup.rs new file mode 100644 index 000000000..1ef2ae9bf --- /dev/null +++ b/src/goal_curation/tests_done_gate_dedup.rs @@ -0,0 +1,254 @@ +//! TDD (Step 7) — FAILING tests for the P3 done-gate convergence fix. +//! +//! P3: a completed goal accumulates MULTIPLE competing "done-gate" PRs for the +//! same goal slug (retry churn without delivery) — e.g. the coin-benchmark +//! goal's 3 competing CLEAN done-gate PRs plus 5 stale CONFLICTING branches. +//! The fix converges a goal slug onto a SINGLE done-gate PR and prunes the +//! duplicates via LOGIC (never hand-closing PRs). +//! +//! These tests specify the pure convergence + slug-sanitisation contract. RED +//! until the following are implemented in `goal_curation::completion_gate` (and +//! re-exported at `crate::goal_curation`): +//! * `sanitize_goal_slug` +//! * `DoneGatePr`, `SlugConvergence`, `converge_done_gate_prs` +//! +//! Security invariants (see design `security_considerations`): +//! * supersede ONLY bot-authored PRs whose slug EXACTLY matches — never a +//! human's PR and never an unrelated goal; +//! * sanitise the slug to `[a-z0-9-]` before it is used in a branch / argv / +//! path (no `..`, path-sep, or shell metacharacters). +//! +//! Wire-in: `#[cfg(test)] mod tests_done_gate_dedup;` in +//! `src/goal_curation/mod.rs`. + +use crate::goal_curation::completion_gate::{ + DoneGatePr, SlugConvergence, converge_done_gate_prs, sanitize_goal_slug, +}; + +const BOT: &str = "rysweet"; +const SLUG: &str = "build-a-local-coin-benchmark-harness-09e65e35"; + +fn pr(number: u32, author: &str, slug: &str, mergeable: &str, created_at: &str) -> DoneGatePr { + DoneGatePr { + number, + author: author.to_string(), + slug: slug.to_string(), + mergeable: mergeable.to_string(), + created_at: created_at.to_string(), + } +} + +fn clean(number: u32, created_at: &str) -> DoneGatePr { + pr(number, BOT, SLUG, "MERGEABLE", created_at) +} + +fn dirty(number: u32, created_at: &str) -> DoneGatePr { + pr(number, BOT, SLUG, "CONFLICTING", created_at) +} + +// ════════════════════════════════════════════════════════════════════════════ +// 1. sanitize_goal_slug — [a-z0-9-] only, no path/argv metacharacters +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn sanitize_preserves_a_valid_slug() { + assert_eq!(sanitize_goal_slug(SLUG), SLUG); +} + +#[test] +fn sanitize_lowercases() { + assert_eq!( + sanitize_goal_slug("Build-A-Local-Coin"), + "build-a-local-coin" + ); +} + +#[test] +fn sanitize_strips_path_and_shell_metacharacters() { + // `..`, `/`, spaces, and shell metacharacters must be removed so the slug + // can never traverse a path or inject an argv flag. + for (raw, _why) in [ + ("../../etc/passwd", "path traversal"), + ("goal;rm -rf /", "shell metachar"), + ("goal name with spaces", "spaces"), + ("goal/../slug", "embedded traversal"), + ("--flag-like", "argv flag"), + ("slug$(whoami)", "command substitution"), + ] { + let out = sanitize_goal_slug(raw); + assert!( + out.bytes() + .all(|b| matches!(b, b'a'..=b'z' | b'0'..=b'9' | b'-')), + "sanitised {raw:?} => {out:?} must only contain [a-z0-9-]" + ); + assert!( + !out.contains(".."), + "sanitised {raw:?} => {out:?} must not contain '..'" + ); + assert!( + !out.starts_with('-'), + "sanitised {raw:?} => {out:?} must not start with '-'" + ); + } +} + +#[test] +fn sanitize_is_idempotent() { + let once = sanitize_goal_slug("Goal/../Name!!"); + assert_eq!(sanitize_goal_slug(&once), once); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 2. converge_done_gate_prs — keep the OLDEST CLEAN, supersede the rest in scope +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn converges_competing_clean_prs_to_the_oldest() { + // The 3 competing CLEAN done-gate PRs (#4326 oldest, #4329, #4332): keep the + // oldest, supersede the newer duplicates. + let prs = vec![ + clean(4332, "2026-07-20T10:00:00Z"), + clean(4326, "2026-07-18T09:00:00Z"), + clean(4329, "2026-07-19T09:00:00Z"), + ]; + let SlugConvergence { keep, supersede } = converge_done_gate_prs(&prs, SLUG, BOT); + assert_eq!( + keep, + Some(4326), + "the oldest CLEAN PR is the single survivor" + ); + let mut sup = supersede; + sup.sort_unstable(); + assert_eq!( + sup, + vec![4329, 4332], + "the newer CLEAN duplicates are superseded" + ); +} + +#[test] +fn prunes_stale_conflicting_branches_alongside_the_keeper() { + // The 5 stale CONFLICTING branches must be pruned via logic once a CLEAN + // keeper is chosen — not left to accumulate. + let prs = vec![ + clean(4326, "2026-07-18T09:00:00Z"), + dirty(4161, "2026-07-01T09:00:00Z"), + dirty(4149, "2026-07-02T09:00:00Z"), + dirty(4134, "2026-07-03T09:00:00Z"), + ]; + let SlugConvergence { keep, supersede } = converge_done_gate_prs(&prs, SLUG, BOT); + assert_eq!(keep, Some(4326)); + let mut sup = supersede; + sup.sort_unstable(); + assert_eq!( + sup, + vec![4134, 4149, 4161], + "stale conflicting in-scope PRs are pruned" + ); +} + +#[test] +fn single_clean_pr_is_kept_with_nothing_superseded() { + let prs = vec![clean(4326, "2026-07-18T09:00:00Z")]; + let out = converge_done_gate_prs(&prs, SLUG, BOT); + assert_eq!(out.keep, Some(4326)); + assert!( + out.supersede.is_empty(), + "a lone done-gate PR has no duplicates to prune" + ); +} + +#[test] +fn no_clean_pr_keeps_nothing_and_supersedes_nothing() { + // Fail-safe: when NO in-scope PR is mergeable, do not destroy the only + // representatives — leave them for the stale-goal path to handle. + let prs = vec![ + dirty(4161, "2026-07-01T09:00:00Z"), + dirty(4149, "2026-07-02T09:00:00Z"), + ]; + let out = converge_done_gate_prs(&prs, SLUG, BOT); + assert_eq!(out.keep, None); + assert!(out.supersede.is_empty()); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 3. Ownership scoping — never touch a human PR or a different goal slug +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn never_supersedes_a_pr_authored_by_a_human() { + // A same-slug PR by a non-bot author must never be superseded. + let prs = vec![ + clean(4326, "2026-07-18T09:00:00Z"), + pr( + 9001, + "some-human", + SLUG, + "MERGEABLE", + "2026-07-19T09:00:00Z", + ), + ]; + let out = converge_done_gate_prs(&prs, SLUG, BOT); + assert_eq!(out.keep, Some(4326)); + assert!( + !out.supersede.contains(&9001), + "a human-authored PR must never be superseded by the bot's convergence" + ); +} + +#[test] +fn never_supersedes_a_pr_for_a_different_goal_slug() { + let prs = vec![ + clean(4326, "2026-07-18T09:00:00Z"), + pr( + 7777, + BOT, + "some-other-goal-deadbeef", + "CONFLICTING", + "2026-07-10T09:00:00Z", + ), + ]; + let out = converge_done_gate_prs(&prs, SLUG, BOT); + assert_eq!(out.keep, Some(4326)); + assert!( + !out.supersede.contains(&7777), + "an out-of-slug PR must never be superseded" + ); +} + +#[test] +fn does_not_pick_a_different_slug_as_keeper_even_if_older() { + // The keeper must belong to the TARGET slug, not merely be the oldest CLEAN + // PR overall. + let prs = vec![ + pr(1000, BOT, "other-goal", "MERGEABLE", "2026-01-01T00:00:00Z"), // older, wrong slug + clean(4326, "2026-07-18T09:00:00Z"), + ]; + let out = converge_done_gate_prs(&prs, SLUG, BOT); + assert_eq!(out.keep, Some(4326), "keeper must be in the target slug"); + assert!(out.supersede.is_empty()); +} + +#[test] +fn author_match_is_case_insensitive() { + let prs = vec![ + pr(4326, "RySweet", SLUG, "MERGEABLE", "2026-07-18T09:00:00Z"), + pr(4329, "RYSWEET", SLUG, "MERGEABLE", "2026-07-19T09:00:00Z"), + ]; + let out = converge_done_gate_prs(&prs, SLUG, "rysweet"); + assert_eq!(out.keep, Some(4326)); + assert_eq!(out.supersede, vec![4329]); +} + +#[test] +fn slug_is_sanitised_before_matching() { + // A goal slug passed with stray casing/characters still matches PRs whose + // slug is the sanitised canonical form. + let prs = vec![clean(4326, "2026-07-18T09:00:00Z")]; + let out = converge_done_gate_prs(&prs, "Build-A-Local-Coin-Benchmark-Harness-09e65e35", BOT); + assert_eq!( + out.keep, + Some(4326), + "the target slug is sanitised to the canonical form before matching" + ); +} diff --git a/src/operator_commands_dashboard/merge_readiness.rs b/src/operator_commands_dashboard/merge_readiness.rs index b4df40c16..4d9e7e3c7 100644 --- a/src/operator_commands_dashboard/merge_readiness.rs +++ b/src/operator_commands_dashboard/merge_readiness.rs @@ -151,6 +151,7 @@ pub fn build_merge_readiness_response( "judge_kind": match judge_kind { MergeJudgeKind::Llm => "llm", MergeJudgeKind::Recipe => "recipe", + MergeJudgeKind::Objective => "objective", MergeJudgeKind::Refusing => "refusing", }, "base_allowlist": base_allowlist, diff --git a/src/overseer/config.rs b/src/overseer/config.rs index 19a45a3ff..a53998e88 100644 --- a/src/overseer/config.rs +++ b/src/overseer/config.rs @@ -616,6 +616,64 @@ pub fn is_engineer_branch(head: &str) -> bool { .any(|prefix| head.starts_with(prefix)) } +// ─── objective merge-judge fallback + trusted authors (P1 / #4389) ────────── +// +// The merge-judge falls back to `RefusingMergeJudge` (always NotReady) whenever +// no LLM/recipe provider is wired, which stalls every delivery-ready PR. These +// two opt-in knobs let an operator enable an OBJECTIVE last-resort tier that +// issues a Ready verdict for a green PR authored by an explicitly-TRUSTED +// author — the JUDGMENT half only; the objective gates (CI-green, mergeable, +// base/repo allowlists) still run downstream and are never bypassed. + +/// Env var that opts INTO the objective merge-judge fallback. Default OFF +/// (fail-closed): deploying the code must never silently flip merge policy. +pub const SIMARD_MERGE_OBJECTIVE_FALLBACK_ENV: &str = "SIMARD_MERGE_OBJECTIVE_FALLBACK"; + +/// Env var holding the comma-separated allowlist of GitHub logins whose green +/// PRs the objective tier may pass. Unset ⇒ the single documented default +/// [`DEFAULT_MERGE_TRUSTED_AUTHOR`]. +pub const SIMARD_MERGE_TRUSTED_AUTHORS_ENV: &str = "SIMARD_MERGE_TRUSTED_AUTHORS"; + +/// The default trusted author when [`SIMARD_MERGE_TRUSTED_AUTHORS_ENV`] is unset. +pub const DEFAULT_MERGE_TRUSTED_AUTHOR: &str = "rysweet"; + +/// Resolve whether the objective merge-judge fallback is enabled. Default OFF: +/// only an explicit truthy value (`1`/`true`/`yes`/`on`, case/space-insensitive) +/// turns it on; unset, empty, falsey, and garbage all stay OFF (fail-closed). +pub fn merge_objective_fallback_enabled_from(lookup: impl Fn(&str) -> Option) -> bool { + lookup(SIMARD_MERGE_OBJECTIVE_FALLBACK_ENV) + .map(|v| is_truthy(&v)) + .unwrap_or(false) +} + +/// Production entry point: read the real process environment. +pub fn merge_objective_fallback_enabled() -> bool { + merge_objective_fallback_enabled_from(|k| std::env::var(k).ok()) +} + +/// Resolve the trusted-author allowlist for the objective merge-judge tier. +/// Unset ⇒ `["rysweet"]`. A set value is split on commas, trimmed, and empties +/// dropped; an entry containing internal whitespace or a `/` is rejected +/// (defense-in-depth — a GitHub login can contain neither, so such an entry is +/// malformed/injected). +pub fn merge_trusted_authors_from(lookup: impl Fn(&str) -> Option) -> Vec { + match lookup(SIMARD_MERGE_TRUSTED_AUTHORS_ENV) { + None => vec![DEFAULT_MERGE_TRUSTED_AUTHOR.to_string()], + Some(raw) => raw + .split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .filter(|s| !s.contains(char::is_whitespace) && !s.contains('/')) + .map(str::to_string) + .collect(), + } +} + +/// Production entry point: read the real process environment. +pub fn merge_trusted_authors() -> Vec { + merge_trusted_authors_from(|k| std::env::var(k).ok()) +} + // ─── agentic merge-queue reasoning scope (issue #4097) ───────────────────── // // The reasoning-scope gate is DELIBERATELY DISTINCT from the automerge sensor diff --git a/src/overseer/merge_ops.rs b/src/overseer/merge_ops.rs index b774abad2..040a2fc1d 100644 --- a/src/overseer/merge_ops.rs +++ b/src/overseer/merge_ops.rs @@ -686,7 +686,21 @@ impl PrOps for MergePrOps { snapshot: summary.to_snapshot(), }); } - project_ready_prs(&candidates, &self.base_allowlist, overseer_login) + // Trusted-author widening (P1 / #4389) is opt-in: only when the + // objective merge-judge fallback is enabled do we let a trusted author's + // green PR reach the merge chain without an engineer label/branch. Off ⇒ + // pre-P1 engineer-only projection. + let trusted_authors = if crate::overseer::config::merge_objective_fallback_enabled() { + crate::overseer::config::merge_trusted_authors() + } else { + Vec::new() + }; + project_ready_prs( + &candidates, + &self.base_allowlist, + overseer_login, + &trusted_authors, + ) } } @@ -830,6 +844,7 @@ mod tests { checks, base_ref_name: "main".to_string(), labels: Vec::new(), + author_login: "rysweet".to_string(), } } diff --git a/src/overseer/mod.rs b/src/overseer/mod.rs index bfddd838d..71222fdd8 100644 --- a/src/overseer/mod.rs +++ b/src/overseer/mod.rs @@ -75,6 +75,7 @@ pub mod wiring; #[cfg(test)] mod tests_deploy_drift; +// TDD (Step 7): failing tests for the P1 project_ready_prs trusted-author gate. #[cfg(test)] mod tests_diagnosis; #[cfg(test)] @@ -92,6 +93,8 @@ mod tests_memory_recall; #[cfg(test)] mod tests_merge_queue_reasoning; #[cfg(test)] +mod tests_ready_prs_trusted_author; +#[cfg(test)] mod tests_root_cause; #[cfg(test)] mod tests_self_healing; @@ -2659,8 +2662,11 @@ pub struct ProjectionCandidate { /// the Overseer never merges its own artifact; /// 3. the PR proves Simard-origin — it carries the engineer-PR label OR rides an /// engineer-exclusive branch namespace (the same G3 narrowing -/// [`merge_ops`](crate::overseer::merge_ops) applies), so an operator's own -/// review PR sharing the author login is never merged; +/// [`merge_ops`](crate::overseer::merge_ops) applies), OR its author is on the +/// `trusted_authors` allowlist (P1 / #4389: a delivery-ready PR by a trusted +/// author — e.g. `rysweet` — reaches the merge chain even without an engineer +/// label/branch), so an operator's own review PR sharing the author login is +/// never merged; /// 4. it passes the objective gates ([`evaluate_objective_gates`]: base-allowlist /// + `MERGEABLE` + all checks green); /// 5. it is NOT a draft (#4339) — `is_draft == Some(false)`. A draft can never be @@ -2670,23 +2676,33 @@ pub struct ProjectionCandidate { /// This is a pure NARROWING — it can only ever remove candidates. The /// authoritative six-criteria merge-authority gate (with the agentic MergeJudge) /// still runs downstream; this only decides which PRs are even proposed to it. +/// +/// The `trusted_authors` widening at gate #3 NEVER bypasses the anti-recursion +/// author guard (#2), the draft gate (#5), or the objective gates (#4): it only +/// broadens the Simard-origin PROOF, not the objective safety rails. An empty +/// `trusted_authors` reverts to the pre-P1 engineer-label/branch-only policy. pub fn project_ready_prs( candidates: &[ProjectionCandidate], base_allowlist: &[String], overseer_login: &str, + trusted_authors: &[String], ) -> Vec { candidates .iter() .filter(|c| c.reasoned.disposition == PrDisposition::ReadyForMerge) // Anti-recursion author guard: never the overseer bot's own PR. .filter(|c| !c.author_login.eq_ignore_ascii_case(overseer_login)) - // Engineer-PR narrowing: prove Simard-origin (label OR engineer branch). + // Engineer-PR narrowing: prove Simard-origin (label OR engineer branch), + // OR admit a delivery-ready PR from a TRUSTED author (P1 / #4389). .filter(|c| { c.snapshot .labels .iter() .any(|l| config::is_engineer_pr_label(l)) || config::is_engineer_branch(&c.head_ref) + || trusted_authors + .iter() + .any(|t| t.eq_ignore_ascii_case(&c.author_login)) }) // Draft gate (#4339): a draft can never be merged. Admit ONLY a // known-non-draft PR; `Some(true)` and `None` (unknown/absent) are diff --git a/src/overseer/tests_m2.rs b/src/overseer/tests_m2.rs index 6469a4fdd..ec68dd2a8 100644 --- a/src/overseer/tests_m2.rs +++ b/src/overseer/tests_m2.rs @@ -71,6 +71,7 @@ impl PrGhClient for Arc { }], base_ref_name: "main".to_string(), labels: Vec::new(), + author_login: "rysweet".to_string(), }) } fn squash_merge(&self, _repo: &str, _pr: u32) -> crate::error::SimardResult<()> { diff --git a/src/overseer/tests_merge_queue_reasoning.rs b/src/overseer/tests_merge_queue_reasoning.rs index fc935eff2..27d894dbc 100644 --- a/src/overseer/tests_merge_queue_reasoning.rs +++ b/src/overseer/tests_merge_queue_reasoning.rs @@ -121,6 +121,7 @@ fn green_engineer_snapshot() -> PrSnapshot { }], base_ref_name: "main".to_string(), labels: vec![SIMARD_ENGINEER_PR_LABEL.to_string()], + author_login: "engineer-bot".to_string(), } } @@ -641,7 +642,7 @@ fn projection_admits_a_ready_engineer_pr_that_passes_every_gate() { "engineer/4097-abcdef", green_engineer_snapshot(), )]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert_eq!( ready, vec![PrRef { @@ -667,7 +668,7 @@ fn projection_excludes_non_ready_dispositions() { "engineer/x", green_engineer_snapshot(), )]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert!( ready.is_empty(), "only ReadyForMerge is a merge candidate; {disp:?} must never be projected" @@ -687,7 +688,7 @@ fn projection_refuses_the_overseer_bots_own_pr_anti_recursion() { "engineer/x", green_engineer_snapshot(), )]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert!( ready.is_empty(), "the anti-recursion author guard must exclude the overseer bot's own PR" @@ -708,7 +709,7 @@ fn projection_refuses_a_pr_that_is_neither_labeled_nor_on_an_engineer_branch() { "feature/human-typed-branch", snap, )]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert!( ready.is_empty(), "a PR that is neither labeled simard-autonomous nor on an engineer branch is an operator PR — never projected" @@ -739,7 +740,7 @@ fn projection_refuses_a_pr_that_fails_the_objective_gates() { "engineer/x", snap, )]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert!( ready.is_empty(), "a PR failing the objective gates must never be authorized, even with a ReadyForMerge proposal" @@ -761,7 +762,7 @@ fn projection_admits_via_engineer_branch_when_label_is_absent() { "engineer/4097-fallback", snap, )]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert_eq!( ready.len(), 1, @@ -789,7 +790,7 @@ fn projection_excludes_a_draft_pr_even_when_every_other_gate_passes() { green_engineer_snapshot(), ) }]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert!( ready.is_empty(), "a draft PR must never be projected, even when disposition + author + \ @@ -813,7 +814,7 @@ fn projection_admits_identical_non_draft_pr() { green_engineer_snapshot(), ) }]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert_eq!( ready, vec![PrRef { @@ -841,7 +842,7 @@ fn projection_excludes_pr_with_unknown_draft_state_fail_closed() { green_engineer_snapshot(), ) }]; - let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login()); + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); assert!( ready.is_empty(), "unknown draft state must fail closed to exclusion (admit only isDraft==Some(false))" diff --git a/src/overseer/tests_ready_prs_trusted_author.rs b/src/overseer/tests_ready_prs_trusted_author.rs new file mode 100644 index 000000000..477c7a61a --- /dev/null +++ b/src/overseer/tests_ready_prs_trusted_author.rs @@ -0,0 +1,278 @@ +//! TDD (Step 7) — FAILING tests for the P1 `project_ready_prs` selection fix. +//! +//! Secondary root cause of P1: the re-narrowing projection in +//! [`crate::overseer::project_ready_prs`] silently DROPS green, mergeable, +//! rysweet-authored PRs that are neither carrying the engineer-PR label nor on +//! an engineer branch (gate #3), and fails closed when the draft state is +//! absent (gate #5). Delivery-ready PRs authored by a TRUSTED author must reach +//! `ready_prs` so the downstream merge chain can act on them. +//! +//! These tests are RED until `project_ready_prs` gains a `trusted_authors` +//! parameter and admits trusted-author PRs at gate #3 while preserving every +//! existing safety gate (anti-recursion author guard, draft exclusion, +//! objective gates). +//! +//! New signature under test: +//! ```ignore +//! pub fn project_ready_prs( +//! candidates: &[ProjectionCandidate], +//! base_allowlist: &[String], +//! overseer_login: &str, +//! trusted_authors: &[String], +//! ) -> Vec; +//! ``` +//! +//! Wire-in (added by the implementation step): +//! `#[cfg(test)] mod tests_ready_prs_trusted_author;` in `src/overseer/mod.rs`. + +use crate::overseer::config::{self, DEFAULT_OVERSEER_AUTHOR_LOGIN, SIMARD_ENGINEER_PR_LABEL}; +use crate::overseer::{PrDisposition, PrRef, ProjectionCandidate, ReasonedPr, project_ready_prs}; +use crate::stewardship::PrSnapshot; +use crate::stewardship::merge_authority::CheckRollupEntry; + +fn overseer_login() -> String { + DEFAULT_OVERSEER_AUTHOR_LOGIN.to_string() +} + +fn base_allowlist() -> Vec { + vec!["main".to_string()] +} + +fn trusted() -> Vec { + vec!["rysweet".to_string()] +} + +/// A green, mergeable snapshot with NO engineer label (the operator/human-style +/// PR shape that gate #3 currently drops). `author_login` is the field the P1 +/// fix adds to `PrSnapshot`. +fn green_unlabeled_snapshot(author: &str) -> PrSnapshot { + PrSnapshot { + body: String::new(), + mergeable: "MERGEABLE".to_string(), + review_decision: "APPROVED".to_string(), + checks: vec![CheckRollupEntry { + name: "ci".to_string(), + state: "SUCCESS".to_string(), + }], + base_ref_name: "main".to_string(), + labels: vec![], + author_login: author.to_string(), + } +} + +fn candidate( + repo: &str, + pr: u32, + disposition: PrDisposition, + author: &str, + head: &str, + is_draft: Option, + snapshot: PrSnapshot, +) -> ProjectionCandidate { + ProjectionCandidate { + reasoned: ReasonedPr { + repo: repo.to_string(), + pr, + disposition, + rationale: "r".to_string(), + duplicate_of: None, + }, + author_login: author.to_string(), + head_ref: head.to_string(), + snapshot, + is_draft, + } +} + +// ════════════════════════════════════════════════════════════════════════════ +// Gate #3 — admit a TRUSTED-author non-engineer PR (the core P1 unblock) +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn admits_trusted_author_green_pr_without_engineer_label_or_branch() { + // #4389-shaped: green, mergeable, rysweet-authored, NON-engineer branch, no + // simard-autonomous label. Previously dropped by gate #3; must now project. + let cands = vec![candidate( + "rysweet/Simard", + 4389, + PrDisposition::ReadyForMerge, + "rysweet", + "feat/issue-4389-nodeoptions", + Some(false), + green_unlabeled_snapshot("rysweet"), + )]; + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &trusted()); + assert_eq!( + ready, + vec![PrRef { + repo: "rysweet/Simard".to_string(), + pr: 4389, + }], + "a green trusted-author PR must be projected even without engineer label/branch" + ); +} + +#[test] +fn still_refuses_untrusted_author_non_engineer_pr() { + // A non-trusted author on a non-engineer PR is an operator/human PR — never + // projected. Trust widening must be scoped to the allowlist only. + let cands = vec![candidate( + "rysweet/Simard", + 4389, + PrDisposition::ReadyForMerge, + "some-human", + "feat/human-typed", + Some(false), + green_unlabeled_snapshot("some-human"), + )]; + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &trusted()); + assert!( + ready.is_empty(), + "an untrusted, unlabeled, non-engineer PR must NOT be projected" + ); +} + +#[test] +fn preserves_engineer_label_admission_for_untrusted_author() { + // The existing engineer-origin admission must still work even when the + // author is not on the trusted-author allowlist (label proves origin). + let mut snap = green_unlabeled_snapshot("engineer-bot"); + snap.labels = vec![SIMARD_ENGINEER_PR_LABEL.to_string()]; + let cands = vec![candidate( + "rysweet/Simard", + 4123, + PrDisposition::ReadyForMerge, + "engineer-bot", + "engineer/4123-abcdef", + Some(false), + snap, + )]; + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &trusted()); + assert_eq!(ready.len(), 1, "engineer-label admission must be preserved"); +} + +// ════════════════════════════════════════════════════════════════════════════ +// Safety gates preserved — trust NEVER bypasses recursion/draft/objective gates +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn trusted_admission_never_bypasses_anti_recursion_author_guard() { + // Even if the overseer bot login were in the trusted list, its own PR must + // never be projected. + let bot = overseer_login(); + let trusted_with_bot = vec!["rysweet".to_string(), bot.clone()]; + let cands = vec![candidate( + "rysweet/Simard", + 4389, + PrDisposition::ReadyForMerge, + &bot, + "feat/x", + Some(false), + green_unlabeled_snapshot(&bot), + )]; + let ready = project_ready_prs(&cands, &base_allowlist(), &bot, &trusted_with_bot); + assert!( + ready.is_empty(), + "anti-recursion guard must win over trusted-author admission" + ); +} + +#[test] +fn trusted_admission_never_bypasses_objective_gates() { + // A trusted author on a RED / CONFLICTING / off-base PR is still refused — + // the objective tier only replaces the JUDGMENT half. + for mutate in [ + (|s: &mut PrSnapshot| s.mergeable = "CONFLICTING".to_string()) as fn(&mut PrSnapshot), + |s: &mut PrSnapshot| { + s.checks = vec![CheckRollupEntry { + name: "ci".to_string(), + state: "FAILURE".to_string(), + }] + }, + |s: &mut PrSnapshot| s.base_ref_name = "stale-base".to_string(), + ] { + let mut snap = green_unlabeled_snapshot("rysweet"); + mutate(&mut snap); + let cands = vec![candidate( + "rysweet/Simard", + 4389, + PrDisposition::ReadyForMerge, + "rysweet", + "feat/x", + Some(false), + snap, + )]; + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &trusted()); + assert!( + ready.is_empty(), + "objective gates must still exclude non-green/non-mergeable/off-base PRs" + ); + } +} + +#[test] +fn trusted_admission_still_excludes_drafts_fail_closed() { + // Gate #5: a draft can never merge server-side. `Some(true)` and `None` + // (unknown/absent draft state) are both excluded even for a trusted author. + for draft in [Some(true), None] { + let cands = vec![candidate( + "rysweet/Simard", + 4389, + PrDisposition::ReadyForMerge, + "rysweet", + "feat/x", + draft, + green_unlabeled_snapshot("rysweet"), + )]; + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &trusted()); + assert!( + ready.is_empty(), + "draft state {draft:?} must be excluded fail-closed even for a trusted author" + ); + } +} + +#[test] +fn empty_trusted_list_reverts_to_engineer_only_admission() { + // With no trusted authors configured, behaviour is the pre-P1 engineer-only + // policy: a non-engineer PR is dropped. + let cands = vec![candidate( + "rysweet/Simard", + 4389, + PrDisposition::ReadyForMerge, + "rysweet", + "feat/x", + Some(false), + green_unlabeled_snapshot("rysweet"), + )]; + let ready = project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &[]); + assert!( + ready.is_empty(), + "an empty trusted-author allowlist must not admit non-engineer PRs" + ); +} + +#[test] +fn trusted_match_is_case_insensitive_but_exact() { + // GitHub logins compare case-insensitively; a look-alike must not match. + let trusted_list = vec!["rysweet".to_string()]; + let admit = |author: &str| { + let cands = vec![candidate( + "rysweet/Simard", + 4389, + PrDisposition::ReadyForMerge, + author, + "feat/x", + Some(false), + green_unlabeled_snapshot(author), + )]; + !project_ready_prs(&cands, &base_allowlist(), &overseer_login(), &trusted_list).is_empty() + }; + assert!(admit("RySweet"), "case-insensitive login match"); + assert!( + !admit("rysweet-bot"), + "look-alike login must not be trusted" + ); + // Sanity: the config-level engineer-label helper is unaffected by this path. + assert!(config::is_engineer_pr_label(SIMARD_ENGINEER_PR_LABEL)); +} diff --git a/src/overseer/tests_selfmerge_fix.rs b/src/overseer/tests_selfmerge_fix.rs index ea2b973c4..d7e2e3240 100644 --- a/src/overseer/tests_selfmerge_fix.rs +++ b/src/overseer/tests_selfmerge_fix.rs @@ -166,6 +166,7 @@ fn snapshot(mergeable: &str, checks: Vec, labels: Vec) checks, base_ref_name: "main".to_string(), labels, + author_login: "rysweet".to_string(), } } diff --git a/src/self_deploy/head_advance.rs b/src/self_deploy/head_advance.rs new file mode 100644 index 000000000..1c1f11def --- /dev/null +++ b/src/self_deploy/head_advance.rs @@ -0,0 +1,129 @@ +//! Head-advance + per-SHA dedupe for self-deploy (#4305 / #4387 / #4390). +//! +//! The existing time-based [`crate::self_relaunch`] throttle stops *per-tick* +//! thrash but has no memory of *which* head it last deployed, so a merged head +//! that already landed is re-evaluated every cycle (#4387) and a genuinely new +//! merged head is not reliably advanced onto (#4305). This module adds the +//! small, pure, file-backed layer that closes that gap: +//! +//! * [`is_valid_deploy_sha`] — argv-injection guard: only a 40- or 64-char +//! lowercase-hex SHA may ever reach `git`/`systemctl`/`gh` argv. +//! * [`DeployHeadState`] — the durable "last head I deployed + its result", +//! serialised alongside the other self-deploy state (mirrors +//! `SelfRelaunchState`). +//! * [`should_deploy_target_sha`] — per-SHA dedupe: never redeploy a SHA that +//! already SUCCEEDED; a FAILED attempt may retry (the time throttle still +//! guards thrash). +//! * [`needs_head_advance`] — deploy only when the running head differs from +//! the merged head and the merged head is verifiable. +//! * [`classify_unit_load`] / [`should_reconcile_unit`] — turn a +//! `systemctl is-enabled` result into a "unit not loaded → reconcile" +//! decision (mirrors [`crate::self_deploy::restart`]'s present-unit heuristic). +//! +//! All logic here is pure and hermetically unit-tested. It exposes DECISIONS +//! only — the effectful callers (`git rev-parse`, the atomic swap, `systemctl`) +//! in the orchestrator and restart modules are the intended consumers and must +//! invoke these helpers before acting. NOTE: this module is not yet wired into +//! the live self-deploy loop; the runtime integration is tracked as follow-up +//! (#4305 / #4387 / #4390) and lands in a separate, integration-testable change. + +use serde::{Deserialize, Serialize}; + +/// Outcome of the most recent self-deploy attempt for [`DeployHeadState`]. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum DeployResult { + /// The build-from-source deploy landed and verified live. + Succeeded, + /// The deploy attempt failed (and may be retried for the same SHA). + Failed, +} + +/// Durable record of the last head self-deploy acted on. File-backed JSON, +/// mirroring `SelfRelaunchState`, so per-SHA dedupe survives a restart and the +/// daemon does not redeploy an already-succeeded head every tick (#4387). +#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct DeployHeadState { + /// The target commit SHA the last deploy attempt was for. `None` before the + /// first deploy. + #[serde(default)] + pub last_deploy_target_sha: Option, + /// The result of that last attempt. `None` before the first deploy. + #[serde(default)] + pub last_deploy_result: Option, +} + +/// Whether `sha` is a full, argv-safe git object id: exactly 40 (SHA-1) or 64 +/// (SHA-256) characters, every one a **lowercase** hex digit. This rejects +/// uppercase, wrong-length, non-hex, whitespace-padded, and flag-like (`-…`) +/// values before any is passed to `git`/`systemctl`/`gh` — guarding against +/// argv option-injection. Fail-closed: anything not matching is invalid. +pub fn is_valid_deploy_sha(sha: &str) -> bool { + matches!(sha.len(), 40 | 64) + && sha + .bytes() + .all(|b| b.is_ascii_digit() || (b'a'..=b'f').contains(&b)) +} + +/// Decide whether to deploy `candidate_sha` given the last deploy state. +/// +/// * An argv-unsafe candidate is NEVER deployed (fail-closed). +/// * A SHA that already SUCCEEDED is deduped — no redeploy (#4387). +/// * A different merged head always deploys (#4305 head advance). +/// * A SHA whose prior attempt FAILED may retry (the time throttle still +/// prevents per-tick thrash). +pub fn should_deploy_target_sha(state: &DeployHeadState, candidate_sha: &str) -> bool { + if !is_valid_deploy_sha(candidate_sha) { + return false; + } + !matches!( + (state.last_deploy_target_sha.as_deref(), state.last_deploy_result), + (Some(last), Some(DeployResult::Succeeded)) if last == candidate_sha + ) +} + +/// Whether the running head must advance onto the merged head: the merged head +/// must be a verifiable argv-safe SHA AND differ from the running head. An +/// unverifiable / argv-unsafe merged target never triggers an advance +/// (fail-closed). +pub fn needs_head_advance(running_head: &str, merged_head: &str) -> bool { + is_valid_deploy_sha(merged_head) && running_head != merged_head +} + +/// Whether a systemd unit backing the deploy is loaded/known to systemd. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum UnitLoadState { + /// The unit is known to systemd (enabled, static, or disabled). + Loaded, + /// systemd does not know the unit (`not found` / `no such unit` / `not + /// loaded`) — the service-managed deploy path is missing. + NotLoaded, +} + +/// Classify a `systemctl is-enabled ` result into a [`UnitLoadState`]. +/// +/// * A zero exit (`is_enabled_success == true`) means the unit is enabled → +/// [`UnitLoadState::Loaded`]. +/// * A non-zero exit is ambiguous: a KNOWN-but-not-enabled unit (`static`, +/// `disabled`) still exits non-zero but is loaded; only an explicit +/// `not found` / `no such` / `not loaded` output means the unit is absent. +/// Mirrors the `systemd_unit_present` heuristic in +/// [`crate::self_deploy::restart`]. +pub fn classify_unit_load(is_enabled_success: bool, output: &str) -> UnitLoadState { + if is_enabled_success { + return UnitLoadState::Loaded; + } + let lower = output.to_ascii_lowercase(); + if lower.contains("not found") || lower.contains("no such") || lower.contains("not loaded") { + UnitLoadState::NotLoaded + } else { + UnitLoadState::Loaded + } +} + +/// Whether the missing/not-loaded systemd unit path should be reconciled so the +/// deploy becomes service-managed. Only a genuinely absent unit is reconciled; +/// a loaded (even disabled) unit is left alone. +pub fn should_reconcile_unit(state: UnitLoadState) -> bool { + state == UnitLoadState::NotLoaded +} diff --git a/src/self_deploy/mod.rs b/src/self_deploy/mod.rs index 585849fa5..d0ed599ff 100644 --- a/src/self_deploy/mod.rs +++ b/src/self_deploy/mod.rs @@ -23,6 +23,7 @@ pub mod backup; pub mod drift; +pub mod head_advance; pub mod health; pub mod orchestrator; pub mod orphan; @@ -34,6 +35,10 @@ pub use backup::{ProtectiveBackup, take_protective_backup}; pub use drift::{ DeployDrift, DeploySource, GitDeploySource, ReconcileDetector, production_reconcile_detector, }; +pub use head_advance::{ + DeployHeadState, DeployResult, UnitLoadState, classify_unit_load, is_valid_deploy_sha, + needs_head_advance, should_deploy_target_sha, should_reconcile_unit, +}; pub use health::{ BrainsLlmBackedProbe, GoalBoardIntactProbe, MemoryIntactProbe, NoQuarantineProbe, SelfHealthProbes, SelfHealthReport, VersionAdvancedProbe, run_self_health_probe, @@ -61,3 +66,6 @@ mod tests_orphan; mod tests_restart; #[cfg(test)] mod tests_source_prep; +// TDD (Step 7): failing tests for the P2 per-SHA dedupe / head-advance logic. +#[cfg(test)] +mod tests_deploy_dedup; diff --git a/src/self_deploy/tests_deploy_dedup.rs b/src/self_deploy/tests_deploy_dedup.rs new file mode 100644 index 000000000..592a940d7 --- /dev/null +++ b/src/self_deploy/tests_deploy_dedup.rs @@ -0,0 +1,212 @@ +//! TDD (Step 7) — FAILING tests for the P2 merged-but-undeployed fix. +//! +//! P2: the running head does not advance to the merged head, and self-deploy +//! re-fires for the same head (issues #4390 anti-thrash, #4387 dedupe, #4305 +//! land the merged-but-undeployed head). These tests specify the genuinely NEW +//! logic layered on the existing time-based `global_deploy_throttle_allow`: +//! +//! * per-TARGET-SHA dedupe — never re-deploy a SHA that already SUCCEEDED; +//! * head-advance decision — deploy only when running != merged head; +//! * file-backed deploy-head state (mirrors `SelfRelaunchState`) round-trips; +//! * argv-safety — an invalid (non-hex / padded / flag-like) SHA is never +//! deployed (guards `systemctl`/`gh`/`git` argv option-injection); +//! * systemd unit-not-loaded classification + reconcile decision. +//! +//! RED until the following are implemented and re-exported at `self_deploy::`: +//! * `DeployHeadState`, `DeployResult` +//! * `should_deploy_target_sha`, `needs_head_advance`, `is_valid_deploy_sha` +//! * `UnitLoadState`, `classify_unit_load`, `should_reconcile_unit` +//! +//! Wire-in: `#[cfg(test)] mod tests_deploy_dedup;` in `src/self_deploy/mod.rs`. + +use crate::self_deploy::{ + DeployHeadState, DeployResult, UnitLoadState, classify_unit_load, is_valid_deploy_sha, + needs_head_advance, should_deploy_target_sha, should_reconcile_unit, +}; + +const SHA_A: &str = "0123456789abcdef0123456789abcdef01234567"; // 40-hex +const SHA_B: &str = "89abcdef0123456789abcdef0123456789abcdef"; // 40-hex +const SHA_A_256: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; // 64-hex + +// ════════════════════════════════════════════════════════════════════════════ +// 1. is_valid_deploy_sha — 40/64 lowercase hex only (argv-injection guard) +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn accepts_40_and_64_char_lowercase_hex() { + assert!(is_valid_deploy_sha(SHA_A)); + assert!(is_valid_deploy_sha(SHA_A_256)); +} + +#[test] +fn rejects_uppercase_wrong_length_and_non_hex() { + for bad in [ + "", + "abc", + "0123456789ABCDEF0123456789abcdef01234567", // uppercase + "0123456789abcdef0123456789abcdef0123456", // 39 + "0123456789abcdef0123456789abcdef012345678", // 41 + "z123456789abcdef0123456789abcdef01234567", // non-hex 'z' + ] { + assert!(!is_valid_deploy_sha(bad), "must reject invalid SHA {bad:?}"); + } +} + +#[test] +fn rejects_flag_like_and_padded_shas() { + // Leading '-' or surrounding whitespace would let the value be parsed as an + // option or break argv boundaries — never accepted. + for bad in [ + "--upload-pack=evil", + "-0123456789abcdef0123456789abcdef0123456", + " 0123456789abcdef0123456789abcdef01234567", + "0123456789abcdef0123456789abcdef01234567 ", + ] { + assert!( + !is_valid_deploy_sha(bad), + "must reject argv-unsafe SHA {bad:?}" + ); + } +} + +// ════════════════════════════════════════════════════════════════════════════ +// 2. should_deploy_target_sha — per-SHA dedupe (skip already-SUCCEEDED head) +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn deploys_when_no_prior_state() { + let state = DeployHeadState::default(); + assert!(should_deploy_target_sha(&state, SHA_A)); +} + +#[test] +fn skips_a_sha_that_already_succeeded() { + // The #4387 dedupe: a head we already deployed SUCCESSFULLY must not be + // redeployed every tick. + let state = DeployHeadState { + last_deploy_target_sha: Some(SHA_A.to_string()), + last_deploy_result: Some(DeployResult::Succeeded), + }; + assert!( + !should_deploy_target_sha(&state, SHA_A), + "an already-succeeded SHA must be deduped (no redeploy)" + ); +} + +#[test] +fn deploys_a_new_merged_head_even_after_a_prior_success() { + // #4305: once new work merges, the new head must advance. + let state = DeployHeadState { + last_deploy_target_sha: Some(SHA_A.to_string()), + last_deploy_result: Some(DeployResult::Succeeded), + }; + assert!( + should_deploy_target_sha(&state, SHA_B), + "a different merged head must still deploy" + ); +} + +#[test] +fn retries_a_sha_whose_prior_deploy_failed() { + // Dedupe is scoped to SUCCESS only: a FAILED attempt for the same SHA may + // retry (the time-based throttle still prevents per-tick thrash). + let state = DeployHeadState { + last_deploy_target_sha: Some(SHA_A.to_string()), + last_deploy_result: Some(DeployResult::Failed), + }; + assert!( + should_deploy_target_sha(&state, SHA_A), + "a previously-failed SHA is allowed to retry" + ); +} + +#[test] +fn never_deploys_an_invalid_candidate_sha() { + let state = DeployHeadState::default(); + assert!( + !should_deploy_target_sha(&state, "--not-a-sha"), + "an argv-unsafe candidate SHA must never be deployed (fail-closed)" + ); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 3. DeployHeadState — file-backed JSON round-trip (mirrors SelfRelaunchState) +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn deploy_head_state_json_round_trips() { + let state = DeployHeadState { + last_deploy_target_sha: Some(SHA_A.to_string()), + last_deploy_result: Some(DeployResult::Succeeded), + }; + let json = serde_json::to_string(&state).expect("serialises"); + let back: DeployHeadState = serde_json::from_str(&json).expect("deserialises"); + assert_eq!(back.last_deploy_target_sha.as_deref(), Some(SHA_A)); + assert_eq!(back.last_deploy_result, Some(DeployResult::Succeeded)); +} + +#[test] +fn deploy_head_state_default_is_empty() { + let state = DeployHeadState::default(); + assert!(state.last_deploy_target_sha.is_none()); + assert!(state.last_deploy_result.is_none()); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 4. needs_head_advance — reconcile running head to merged head +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn advances_when_running_is_behind_merged() { + assert!(needs_head_advance(SHA_A, SHA_B)); +} + +#[test] +fn no_advance_when_running_equals_merged() { + assert!(!needs_head_advance(SHA_A, SHA_A)); +} + +#[test] +fn no_advance_to_an_invalid_merged_head() { + // Never advance the running head to an unverifiable / argv-unsafe target. + assert!(!needs_head_advance(SHA_A, "--evil")); + assert!(!needs_head_advance(SHA_A, "")); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 5. systemd unit-not-loaded classification + reconcile decision +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn classifies_unit_loaded_on_success() { + assert_eq!(classify_unit_load(true, "enabled"), UnitLoadState::Loaded); +} + +#[test] +fn classifies_unit_not_loaded_on_not_found() { + for out in [ + "Unit simard-ooda.service not loaded", + "not found", + "no such unit", + ] { + assert_eq!( + classify_unit_load(false, out), + UnitLoadState::NotLoaded, + "output {out:?} indicates the unit is not loaded" + ); + } +} + +#[test] +fn classifies_known_but_disabled_unit_as_loaded() { + // A non-zero exit for a KNOWN-but-not-enabled unit (static/disabled) is not + // "not loaded" — matches the existing systemd_unit_present heuristic. + assert_eq!(classify_unit_load(false, "static"), UnitLoadState::Loaded); + assert_eq!(classify_unit_load(false, "disabled"), UnitLoadState::Loaded); +} + +#[test] +fn reconciles_only_when_unit_not_loaded() { + assert!(should_reconcile_unit(UnitLoadState::NotLoaded)); + assert!(!should_reconcile_unit(UnitLoadState::Loaded)); +} diff --git a/src/stewardship/merge_authority.rs b/src/stewardship/merge_authority.rs index e2d317e1d..54f0aad91 100644 --- a/src/stewardship/merge_authority.rs +++ b/src/stewardship/merge_authority.rs @@ -66,6 +66,12 @@ pub struct PrSnapshot { /// human-review gate: a PR carrying /// [`crate::creative_ideas::CREATIVE_IDEA_PR_LABEL`] is never auto-merged. pub labels: Vec, + /// `author.login` from `gh pr view --json ...,author`. The AUTHENTICATED + /// author, used by the objective merge-judge tier (P1 / #4389) to decide + /// whether a green PR is from a TRUSTED author. Fail-closed: an absent + /// author object hydrates to the empty string, which can never match a + /// configured trusted login. + pub author_login: String, } /// One row from `statusCheckRollup`. Both check runs and statuses get @@ -142,6 +148,7 @@ impl OpenPrSummary { checks: self.checks.clone(), base_ref_name: self.base_ref_name.clone(), labels: Vec::new(), + author_login: self.author.clone(), } } } @@ -494,6 +501,13 @@ pub fn parse_pr_view_json(stdout: &[u8]) -> SimardResult { base_ref_name: String, #[serde(default)] labels: Vec, + #[serde(default)] + author: Option, + } + #[derive(serde::Deserialize)] + struct RawAuthor { + #[serde(default)] + login: String, } #[derive(serde::Deserialize)] struct RawLabel { @@ -552,6 +566,7 @@ pub fn parse_pr_view_json(stdout: &[u8]) -> SimardResult { .map(|l| l.name) .filter(|n| !n.is_empty()) .collect(), + author_login: raw.author.map(|a| a.login).unwrap_or_default(), }) } @@ -929,6 +944,7 @@ mod tests { ], base_ref_name: "main".to_string(), labels: Vec::new(), + author_login: "rysweet".to_string(), } } diff --git a/src/stewardship/merge_judge.rs b/src/stewardship/merge_judge.rs index ed5c1c178..6eb22fcfd 100644 --- a/src/stewardship/merge_judge.rs +++ b/src/stewardship/merge_judge.rs @@ -133,6 +133,10 @@ pub enum MergeJudgeKind { /// [`super::recipe_merge_judge::RecipeMergeJudge`] — production impl /// backed by recipe-runner-rs subprocess. Recipe, + /// [`super::objective_merge_judge::ObjectiveMergeJudge`] — the opt-in + /// last-resort tier that passes a green PR from a TRUSTED author when no + /// LLM/recipe provider is wired (P1 / #4389). Off by default. + Objective, /// [`RefusingMergeJudge`] — fallback when no LLM provider is configured. /// When this is the active variant, every merge will be refused; the /// dashboard surfaces this in red so the operator notices. @@ -143,7 +147,31 @@ impl MergeJudgeKind { /// Whether the judge can issue a non-refusal verdict. Mirrors the /// `judge_configured` field in the dashboard JSON contract. pub fn is_configured(self) -> bool { - matches!(self, MergeJudgeKind::Llm | MergeJudgeKind::Recipe) + matches!( + self, + MergeJudgeKind::Llm | MergeJudgeKind::Recipe | MergeJudgeKind::Objective + ) + } +} + +/// Resolve which merge-judge tier is active, in strict preference order: +/// Recipe > LLM > (Objective iff the operator opted in) > Refusing. A real +/// reviewer (recipe/LLM) always wins; the objective tier is the LAST resort and +/// only when `objective_fallback` is on; otherwise the fail-closed +/// [`RefusingMergeJudge`] remains the default. +pub fn resolve_merge_judge_kind( + recipe_available: bool, + llm_available: bool, + objective_fallback: bool, +) -> MergeJudgeKind { + if recipe_available { + MergeJudgeKind::Recipe + } else if llm_available { + MergeJudgeKind::Llm + } else if objective_fallback { + MergeJudgeKind::Objective + } else { + MergeJudgeKind::Refusing } } @@ -370,23 +398,40 @@ fn extract_balanced_objects(s: &str) -> Vec<&str> { // ─────────────────────────── Production constructor ───────────────────────── -/// Build the production merge judge. Resolution order: +/// Build the production merge judge. Resolution order (see +/// [`resolve_merge_judge_kind`]): /// 1. Recipe-runner-rs (if binary and recipe YAML are both available) /// 2. Direct LLM (if an LLM provider is configured) -/// 3. [`RefusingMergeJudge`] (fallback) +/// 3. [`super::objective_merge_judge::ObjectiveMergeJudge`] — ONLY when the +/// operator opted in via `SIMARD_MERGE_OBJECTIVE_FALLBACK` (P1 / #4389) +/// 4. [`RefusingMergeJudge`] (fail-closed default) pub fn build_merge_judge() -> Box { let repo_root = std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")); if let Some(recipe_judge) = super::recipe_merge_judge::RecipeMergeJudge::new(&repo_root) { - eprintln!("[simard] merge-judge: using recipe-runner-rs backed judge"); + tracing::info!( + target: "stewardship::merge_judge", + "merge-judge: using recipe-runner-rs backed judge" + ); return Box::new(recipe_judge); } - match LlmProvider::resolve() { - Ok(provider) => { - let submitter = SessionLlmSubmitter::new(provider); - Box::new(LlmMergeJudge::new(submitter)) - } - Err(_) => Box::new(RefusingMergeJudge), + if let Ok(provider) = LlmProvider::resolve() { + return Box::new(LlmMergeJudge::new(SessionLlmSubmitter::new(provider))); + } + // No recipe, no LLM: use the objective tier ONLY if explicitly opted in, + // else stay fail-closed on the refusing default. + if crate::overseer::config::merge_objective_fallback_enabled() { + let trusted = crate::overseer::config::merge_trusted_authors(); + let bot_login = crate::overseer::config::overseer_author_login(); + tracing::info!( + target: "stewardship::merge_judge", + trusted_authors = ?trusted, + "merge-judge: using OBJECTIVE fallback tier (no recipe/LLM; opt-in enabled)" + ); + return Box::new(super::objective_merge_judge::ObjectiveMergeJudge::new( + trusted, bot_login, + )); } + Box::new(RefusingMergeJudge) } // ─────────────────────────── Tests ────────────────────────────────────────── @@ -403,6 +448,7 @@ mod tests { checks: vec![], base_ref_name: "main".into(), labels: vec![], + author_login: "rysweet".into(), } } diff --git a/src/stewardship/mod.rs b/src/stewardship/mod.rs index 864e2d2cb..1729b9643 100644 --- a/src/stewardship/mod.rs +++ b/src/stewardship/mod.rs @@ -18,6 +18,7 @@ pub mod dedup; pub mod gh_client; pub mod merge_authority; pub mod merge_judge; +pub mod objective_merge_judge; pub mod recipe_merge_judge; pub mod routing; pub mod types; @@ -26,6 +27,9 @@ pub mod types; mod tests; #[cfg(test)] mod tests_extra; +// TDD (Step 7): failing tests for the P1 objective-merge-judge fallback. +#[cfg(test)] +mod tests_objective_merge_judge; pub use dedup::{failure_signature, find_existing, normalize}; pub use gh_client::{GhClient, GhIssue, RealGhClient}; @@ -37,8 +41,9 @@ pub use merge_authority::{ }; pub use merge_judge::{ Blocker, JudgeOutcome, LlmMergeJudge, MergeJudge, MergeJudgeKind, RefusingMergeJudge, Verdict, - build_merge_judge, + build_merge_judge, resolve_merge_judge_kind, }; +pub use objective_merge_judge::ObjectiveMergeJudge; pub use recipe_merge_judge::RecipeMergeJudge; pub use routing::route_failure; pub use types::{OrchestratorRunSummary, StewardshipOutcome, TargetRepo}; diff --git a/src/stewardship/objective_merge_judge.rs b/src/stewardship/objective_merge_judge.rs new file mode 100644 index 000000000..3686975a5 --- /dev/null +++ b/src/stewardship/objective_merge_judge.rs @@ -0,0 +1,103 @@ +//! Objective merge-judge — the opt-in, last-resort JUDGMENT tier (P1 / #4389). +//! +//! The default [`super::merge_judge::RefusingMergeJudge`] always returns +//! `NotReady` whenever no LLM/recipe provider is wired, which stalls every +//! delivery-ready PR and re-escalates the same ones each overseer tick. This +//! module provides an OBJECTIVE alternative the operator can opt into +//! (`SIMARD_MERGE_OBJECTIVE_FALLBACK=1`): it issues a `Ready` verdict for a PR +//! authored by an explicitly-TRUSTED GitHub login, and `NotReady` for anyone +//! else. +//! +//! It replaces ONLY the judgment half. The objective gates (CI-green, +//! `MERGEABLE`, base-branch + repo allowlists) still run in +//! [`super::merge_authority`] downstream and are never bypassed. +//! +//! ## Security invariants (mirrored by `tests_objective_merge_judge.rs`) +//! * Trust is keyed on the AUTHENTICATED `author.login` (exact, case-insensitive +//! equality) — never on a spoofable body/title/trailer. +//! * The overseer bot identity is ALWAYS excluded from a `Ready` verdict, even +//! if it is somehow present in the allowlist (no self-merge loop). +//! * An empty/unknown author (absent `author` object) can never be trusted. +//! * An empty allowlist trusts no one (fail-closed). + +use crate::error::SimardResult; + +use super::merge_authority::PrSnapshot; +use super::merge_judge::{Blocker, JudgeOutcome, MergeJudge, MergeJudgeKind, Verdict}; + +/// Objective merge judge: passes a green PR iff its authenticated author is on +/// the configured trusted-author allowlist (and is not the overseer bot). +pub struct ObjectiveMergeJudge { + trusted_authors: Vec, + bot_login: String, +} + +impl ObjectiveMergeJudge { + /// Construct with the trusted-author allowlist and the overseer bot login + /// to exclude from any `Ready` verdict. + pub fn new(trusted_authors: Vec, bot_login: String) -> Self { + Self { + trusted_authors, + bot_login, + } + } + + /// Whether `author` is a trusted, non-bot, non-empty login. Case-insensitive + /// but EXACT: a padded (`" rysweet"`) or look-alike (`rysweet-bot`) login + /// never matches. + fn is_trusted(&self, author: &str) -> bool { + if author.is_empty() { + return false; + } + if author.eq_ignore_ascii_case(&self.bot_login) { + return false; + } + self.trusted_authors + .iter() + .any(|t| t.eq_ignore_ascii_case(author)) + } +} + +impl MergeJudge for ObjectiveMergeJudge { + fn judge( + &self, + _pr_number: u32, + _repo: &str, + snapshot: &PrSnapshot, + ) -> SimardResult { + let author = snapshot.author_login.as_str(); + if self.is_trusted(author) { + Ok(JudgeOutcome { + verdict: Verdict::Ready, + rationale: format!( + "objective merge-judge: PR authored by trusted author {author:?}; \ + objective gates enforced separately" + ), + blockers: vec![], + }) + } else { + Ok(JudgeOutcome { + verdict: Verdict::NotReady, + rationale: format!( + "objective merge-judge: author {author:?} is not on the trusted-author \ + allowlist (or is the overseer bot / empty)" + ), + blockers: vec![Blocker { + section: "trusted-author".to_string(), + severity: "high".to_string(), + observation: format!( + "author {author:?} is not a configured trusted author for the \ + objective merge-judge fallback" + ), + fix: "Add the author to SIMARD_MERGE_TRUSTED_AUTHORS, or land the PR via a \ + configured LLM/recipe merge-judge or a manual merge-ready review." + .to_string(), + }], + }) + } + } + + fn kind(&self) -> MergeJudgeKind { + MergeJudgeKind::Objective + } +} diff --git a/src/stewardship/tests_objective_merge_judge.rs b/src/stewardship/tests_objective_merge_judge.rs new file mode 100644 index 000000000..040ba637e --- /dev/null +++ b/src/stewardship/tests_objective_merge_judge.rs @@ -0,0 +1,356 @@ +//! TDD (Step 7) — FAILING tests for the P1 objective-merge-judge fallback. +//! +//! These tests are written **before** the implementation and specify the +//! contract for the P1 fix ("green, mergeable, non-in-flight rysweet PRs are +//! selected but never merged"). Root cause: `build_merge_judge()` falls back to +//! [`RefusingMergeJudge`] (always `Verdict::NotReady`) whenever no LLM/recipe +//! provider is wired, so every delivery-ready PR is refused and re-escalated. +//! +//! They are RED until the following are implemented (see the design spec): +//! * `crate::stewardship::objective_merge_judge::ObjectiveMergeJudge` +//! * `crate::stewardship::merge_judge::MergeJudgeKind::Objective` +//! * `crate::stewardship::merge_judge::resolve_merge_judge_kind` +//! * `crate::stewardship::merge_authority::PrSnapshot::author_login` +//! * `crate::overseer::config::merge_objective_fallback_enabled_from` +//! * `crate::overseer::config::merge_trusted_authors_from` +//! +//! Security invariants under test (see design `security_considerations`): +//! * default OFF — env unset => `RefusingMergeJudge`, never the objective tier; +//! * trust is keyed on the AUTHENTICATED `author.login` (exact equality), not +//! on a spoofable body/title/trailer; +//! * the overseer bot identity is excluded from the trusted-author allowlist +//! (no self-merge loop); +//! * the objective tier only replaces the JUDGMENT half — the objective gates +//! (CI-green, mergeable, base/repo allowlists) still run downstream. +//! +//! Wire-in (added by the implementation step): +//! `#[cfg(test)] mod tests_objective_merge_judge;` in `src/stewardship/mod.rs`. + +use crate::stewardship::merge_authority::{PrSnapshot, parse_pr_view_json}; +use crate::stewardship::merge_judge::{ + MergeJudge, MergeJudgeKind, RefusingMergeJudge, Verdict, resolve_merge_judge_kind, +}; +use crate::stewardship::objective_merge_judge::ObjectiveMergeJudge; + +use crate::overseer::config::{ + SIMARD_MERGE_OBJECTIVE_FALLBACK_ENV, SIMARD_MERGE_TRUSTED_AUTHORS_ENV, + merge_objective_fallback_enabled_from, merge_trusted_authors_from, +}; + +/// Test env resolver: fixed key/value pairs, `None` for anything else. Mirrors +/// the `fn env(pairs)` helper the existing `overseer::config` tests use so the +/// hardened `_from(lookup)` seam is exercised without touching the real +/// process environment. +fn env(pairs: &[(&str, &str)]) -> impl Fn(&str) -> Option { + let map: std::collections::HashMap = pairs + .iter() + .map(|(k, v)| ((*k).to_string(), (*v).to_string())) + .collect(); + move |k: &str| map.get(k).cloned() +} + +/// A green, mergeable snapshot authored by `author`. `author_login` is the NEW +/// field the P1 fix adds to `PrSnapshot` (hydrated from the existing +/// `gh pr view --json ...,author` call). +fn green_snapshot_by(author: &str) -> PrSnapshot { + PrSnapshot { + body: "## CI\ngreen\n## Tests\ncovered\n".to_string(), + mergeable: "MERGEABLE".to_string(), + review_decision: "APPROVED".to_string(), + checks: vec![], + base_ref_name: "main".to_string(), + labels: vec![], + author_login: author.to_string(), + } +} + +// ════════════════════════════════════════════════════════════════════════════ +// 1. PrSnapshot gains `author_login`, hydrated from `gh pr view --json ,author` +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn pr_view_json_hydrates_author_login() { + // The judge-layer trust check needs the AUTHENTICATED author, so the + // existing `gh pr view` parse must now carry `author.login`. + let stdout = br#"{ + "body": "b", + "mergeable": "MERGEABLE", + "reviewDecision": "APPROVED", + "statusCheckRollup": [], + "baseRefName": "main", + "labels": [], + "author": { "login": "rysweet" } + }"#; + let snap = parse_pr_view_json(stdout).expect("valid gh pr view JSON parses"); + assert_eq!(snap.author_login, "rysweet"); +} + +#[test] +fn pr_view_json_absent_author_hydrates_empty_fail_closed() { + // A missing author object must fail closed to an EMPTY login — never a + // trusted default — so an unknown author can never match the allowlist. + let stdout = br#"{ "body": "b", "mergeable": "MERGEABLE", "baseRefName": "main" }"#; + let snap = parse_pr_view_json(stdout).expect("parses with defaults"); + assert_eq!( + snap.author_login, "", + "absent author => empty login (fail-closed)" + ); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 2. ObjectiveMergeJudge — the opt-in non-refusing tier +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn objective_judge_reports_objective_kind_and_is_configured() { + // Dashboard renders "Judge: configured (objective)" off this signal. + let judge = ObjectiveMergeJudge::new( + vec!["rysweet".to_string()], + "simard-overseer[bot]".to_string(), + ); + assert_eq!(judge.kind(), MergeJudgeKind::Objective); + assert!(judge.kind().is_configured()); +} + +#[test] +fn objective_judge_passes_trusted_author_green_pr() { + // The whole P1 fix: a green PR from a trusted author gets a READY verdict + // (instead of the RefusingMergeJudge NotReady that stalls delivery). + let judge = ObjectiveMergeJudge::new( + vec!["rysweet".to_string()], + "simard-overseer[bot]".to_string(), + ); + let out = judge + .judge(4389, "rysweet/Simard", &green_snapshot_by("rysweet")) + .expect("objective judge does not error"); + assert_eq!(out.verdict, Verdict::Ready); +} + +#[test] +fn objective_judge_refuses_untrusted_author() { + // Someone not on the allowlist must NOT get a ready verdict even on a green + // PR — trust is the whole gate the objective tier replaces. + let judge = ObjectiveMergeJudge::new( + vec!["rysweet".to_string()], + "simard-overseer[bot]".to_string(), + ); + let out = judge + .judge( + 4389, + "rysweet/Simard", + &green_snapshot_by("some-random-user"), + ) + .expect("objective judge does not error"); + assert_eq!(out.verdict, Verdict::NotReady); + assert!( + !out.blockers.is_empty(), + "refusal must carry an actionable blocker" + ); +} + +#[test] +fn objective_judge_matches_author_case_insensitively_but_exactly() { + // GitHub logins are case-insensitive; `RySweet` == `rysweet`. But a + // look-alike (`rysweet-bot`, `notrysweet`) must NOT match. + let judge = ObjectiveMergeJudge::new( + vec!["rysweet".to_string()], + "simard-overseer[bot]".to_string(), + ); + assert_eq!( + judge + .judge(1, "rysweet/Simard", &green_snapshot_by("RySweet")) + .unwrap() + .verdict, + Verdict::Ready + ); + for imposter in ["rysweet-bot", "notrysweet", "rysweet ", " rysweet"] { + assert_eq!( + judge + .judge(1, "rysweet/Simard", &green_snapshot_by(imposter)) + .unwrap() + .verdict, + Verdict::NotReady, + "look-alike/padded login {imposter:?} must not match the allowlist" + ); + } +} + +#[test] +fn objective_judge_excludes_bot_identity_no_self_merge() { + // Even if the bot login is somehow present in the allowlist, the judge must + // never issue Ready for the overseer bot's own PR (anti self-merge loop). + let bot = "simard-overseer[bot]"; + let judge = ObjectiveMergeJudge::new( + vec!["rysweet".to_string(), bot.to_string()], + bot.to_string(), + ); + let out = judge + .judge(4389, "rysweet/Simard", &green_snapshot_by(bot)) + .expect("objective judge does not error"); + assert_eq!( + out.verdict, + Verdict::NotReady, + "the bot identity is always excluded from a Ready verdict" + ); +} + +#[test] +fn objective_judge_refuses_empty_author_fail_closed() { + // An empty/unknown author (absent `author` object in the listing) can never + // be trusted. + let judge = ObjectiveMergeJudge::new( + vec!["rysweet".to_string()], + "simard-overseer[bot]".to_string(), + ); + assert_eq!( + judge + .judge(1, "rysweet/Simard", &green_snapshot_by("")) + .unwrap() + .verdict, + Verdict::NotReady + ); +} + +#[test] +fn objective_judge_with_empty_allowlist_refuses_everyone() { + // An empty trusted-authors list is fully fail-closed — no one is trusted. + let judge = ObjectiveMergeJudge::new(vec![], "simard-overseer[bot]".to_string()); + assert_eq!( + judge + .judge(1, "rysweet/Simard", &green_snapshot_by("rysweet")) + .unwrap() + .verdict, + Verdict::NotReady + ); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 3. resolve_merge_judge_kind — Recipe > LLM > (Objective iff opt-in) > Refusing +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn judge_resolution_defaults_to_refusing_when_nothing_configured() { + // Default posture: no recipe, no LLM, fallback OFF => RefusingMergeJudge. + let kind = resolve_merge_judge_kind( + /* recipe_available */ false, /* llm_available */ false, + /* objective_fallback */ false, + ); + assert_eq!(kind, MergeJudgeKind::Refusing); + assert!(!kind.is_configured()); +} + +#[test] +fn judge_resolution_uses_objective_only_when_fallback_opted_in() { + // With no recipe/LLM but the opt-in fallback ON, the objective tier is used + // instead of refusing — this is the P1 unblock. + let kind = resolve_merge_judge_kind(false, false, true); + assert_eq!(kind, MergeJudgeKind::Objective); +} + +#[test] +fn judge_resolution_prefers_recipe_then_llm_over_objective() { + // Objective is the LAST-resort fallback: a real reviewer always wins. + assert_eq!( + resolve_merge_judge_kind(true, false, true), + MergeJudgeKind::Recipe + ); + assert_eq!( + resolve_merge_judge_kind(false, true, true), + MergeJudgeKind::Llm + ); + assert_eq!( + resolve_merge_judge_kind(true, true, true), + MergeJudgeKind::Recipe + ); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 4. config: SIMARD_MERGE_OBJECTIVE_FALLBACK / SIMARD_MERGE_TRUSTED_AUTHORS +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn objective_fallback_defaults_off() { + // Unset => OFF (fail-closed): deploying the code must NOT flip merge policy. + assert!(!merge_objective_fallback_enabled_from(env(&[]))); +} + +#[test] +fn objective_fallback_enables_on_truthy_values() { + for v in ["1", "true", "on", "yes", "TRUE", " on "] { + assert!( + merge_objective_fallback_enabled_from(env(&[(SIMARD_MERGE_OBJECTIVE_FALLBACK_ENV, v)])), + "value {v:?} should enable the objective fallback" + ); + } +} + +#[test] +fn objective_fallback_stays_off_on_falsey_or_noise() { + for v in ["0", "false", "off", "no", "", " ", "maybe"] { + assert!( + !merge_objective_fallback_enabled_from(env(&[( + SIMARD_MERGE_OBJECTIVE_FALLBACK_ENV, + v + )])), + "value {v:?} must keep the objective fallback OFF (fail-closed)" + ); + } +} + +#[test] +fn trusted_authors_default_is_rysweet() { + // Unset => the documented default single trusted author. + assert_eq!( + merge_trusted_authors_from(env(&[])), + vec!["rysweet".to_string()] + ); +} + +#[test] +fn trusted_authors_parses_trims_csv() { + let got = merge_trusted_authors_from(env(&[( + SIMARD_MERGE_TRUSTED_AUTHORS_ENV, + " rysweet , second-user ,,third ", + )])); + assert_eq!( + got, + vec![ + "rysweet".to_string(), + "second-user".to_string(), + "third".to_string() + ], + "CSV is split, trimmed, empties dropped" + ); +} + +#[test] +fn trusted_authors_rejects_logins_with_whitespace_or_slash() { + // A GitHub login can never contain an internal space or a '/', so such an + // entry is malformed/injected and must be dropped (defense-in-depth). + let got = merge_trusted_authors_from(env(&[( + SIMARD_MERGE_TRUSTED_AUTHORS_ENV, + "good-user, bad user, owner/repo, ok2", + )])); + assert_eq!( + got, + vec!["good-user".to_string(), "ok2".to_string()], + "internal-space and slash-bearing entries are rejected" + ); +} + +// ════════════════════════════════════════════════════════════════════════════ +// 5. RefusingMergeJudge stays the DEFAULT — regression guard on fail-closed +// ════════════════════════════════════════════════════════════════════════════ + +#[test] +fn refusing_judge_remains_available_and_not_configured() { + // The objective tier must not remove or weaken the refusing default; the + // regression risk is "RefusingMergeJudge ceases to be the default". + let j = RefusingMergeJudge; + assert_eq!(j.kind(), MergeJudgeKind::Refusing); + assert!(!j.kind().is_configured()); + let out = j + .judge(1, "rysweet/Simard", &green_snapshot_by("rysweet")) + .unwrap(); + assert_eq!(out.verdict, Verdict::NotReady); +} diff --git a/tests/objective_merge_delivery_consumer.rs b/tests/objective_merge_delivery_consumer.rs new file mode 100644 index 000000000..dfe88b9d8 --- /dev/null +++ b/tests/objective_merge_delivery_consumer.rs @@ -0,0 +1,245 @@ +//! Outside-in consumer test (Step 13, #4389): exercises the delivery-stall fix +//! exactly as an OPERATOR would — through the public `simard` library boundary, +//! with no knowledge of internals. +//! +//! Scenario 1 (basic user-facing behaviour): an operator opts into the +//! objective merge fallback via `SIMARD_MERGE_OBJECTIVE_FALLBACK` + a +//! trusted-author allowlist; a green PR by a trusted author is now judged +//! `Ready` (previously always `NotReady`, which is the exact stall this PR +//! fixes), while an untrusted author and the overseer bot are refused. +//! +//! Scenario 2 (integration / edge cases): the hardened env parsing plus the +//! P2/P3 decision-layer functions an operator/consumer calls — self-deploy +//! per-SHA dedupe + head-advance (with an argv-injection guard) and done-gate +//! slug convergence. + +use simard::goal_curation::completion_gate::{ + DoneGatePr, converge_done_gate_prs, sanitize_goal_slug, +}; +use simard::overseer::config::{ + SIMARD_MERGE_OBJECTIVE_FALLBACK_ENV, SIMARD_MERGE_TRUSTED_AUTHORS_ENV, + merge_objective_fallback_enabled_from, merge_trusted_authors_from, +}; +use simard::self_deploy::head_advance::{ + DeployHeadState, DeployResult, UnitLoadState, classify_unit_load, is_valid_deploy_sha, + needs_head_advance, should_deploy_target_sha, should_reconcile_unit, +}; +use simard::stewardship::merge_authority::PrSnapshot; +use simard::stewardship::merge_judge::{ + MergeJudge, MergeJudgeKind, Verdict, resolve_merge_judge_kind, +}; +use simard::stewardship::objective_merge_judge::ObjectiveMergeJudge; + +/// Deterministic env resolver so the consumer path is exercised without +/// mutating the real process environment. +fn env(pairs: &[(&str, &str)]) -> impl Fn(&str) -> Option { + let map: std::collections::HashMap = pairs + .iter() + .map(|(k, v)| ((*k).to_string(), (*v).to_string())) + .collect(); + move |k: &str| map.get(k).cloned() +} + +/// A green, mergeable PR snapshot authored by `author` (only `author_login` +/// drives the objective judge; the objective CI/mergeable gates run separately +/// downstream). +fn green_pr_by(author: &str) -> PrSnapshot { + PrSnapshot { + mergeable: "MERGEABLE".to_string(), + author_login: author.to_string(), + ..Default::default() + } +} + +// ── Scenario 1: basic user-facing behaviour ──────────────────────────────── + +#[test] +fn scenario1_operator_optin_lands_green_trusted_author_pr() { + // Operator config: opt in + allowlist rysweet. + let lookup = env(&[ + (SIMARD_MERGE_OBJECTIVE_FALLBACK_ENV, "1"), + (SIMARD_MERGE_TRUSTED_AUTHORS_ENV, "rysweet"), + ]); + + let optin = merge_objective_fallback_enabled_from(&lookup); + let trusted = merge_trusted_authors_from(&lookup); + assert!(optin, "operator opt-in flag must be read as enabled"); + assert_eq!(trusted, vec!["rysweet".to_string()]); + + // With no recipe/LLM provider wired but opt-in on, the resolver must pick + // the Objective tier instead of the fail-closed Refusing default. + let kind = resolve_merge_judge_kind( + /* recipe_available */ false, /* llm_available */ false, optin, + ); + assert_eq!(kind, MergeJudgeKind::Objective); + + // The green PR by the trusted author is now judged Ready — the delivery + // stall (#4389) is fixed. + let judge = ObjectiveMergeJudge::new(trusted, "simard-overseer[bot]".to_string()); + let verdict = judge + .judge(4389, "rysweet/Simard", &green_pr_by("rysweet")) + .expect("judge must not error") + .verdict; + assert_eq!( + verdict, + Verdict::Ready, + "green PR by a trusted author must be Ready under the objective fallback" + ); +} + +#[test] +fn scenario1_untrusted_author_and_bot_are_still_refused() { + let judge = ObjectiveMergeJudge::new( + vec!["rysweet".to_string()], + "simard-overseer[bot]".to_string(), + ); + + // Untrusted human → NotReady. + let untrusted = judge + .judge(1, "rysweet/Simard", &green_pr_by("someone-else")) + .unwrap() + .verdict; + assert_eq!(untrusted, Verdict::NotReady); + + // The overseer bot is excluded even if on the allowlist → no self-merge loop. + let self_judge = ObjectiveMergeJudge::new( + vec!["simard-overseer[bot]".to_string()], + "simard-overseer[bot]".to_string(), + ); + let bot = self_judge + .judge(2, "rysweet/Simard", &green_pr_by("simard-overseer[bot]")) + .unwrap() + .verdict; + assert_eq!(bot, Verdict::NotReady, "bot self-merge must be refused"); + + // Default (no opt-in) stays fail-closed on Refusing. + let default_kind = resolve_merge_judge_kind(false, false, false); + assert_eq!(default_kind, MergeJudgeKind::Refusing); +} + +// ── Scenario 2: integration / edge cases + decision layer ─────────────────── + +#[test] +fn scenario2_hardened_env_parsing_and_precedence() { + // Default: flag unset → off; allowlist unset → default trusted author. + let empty = env(&[]); + assert!(!merge_objective_fallback_enabled_from(&empty)); + assert_eq!( + merge_trusted_authors_from(&empty), + vec!["rysweet".to_string()] + ); + + // CSV is trimmed; whitespace / slash-bearing entries are rejected. + let dirty = env(&[( + SIMARD_MERGE_TRUSTED_AUTHORS_ENV, + " rysweet , bad name , org/team ,octocat ", + )]); + let parsed = merge_trusted_authors_from(&dirty); + assert_eq!(parsed, vec!["rysweet".to_string(), "octocat".to_string()]); + + // Real reviewers always win over the objective fallback even when opted in. + assert_eq!( + resolve_merge_judge_kind(true, false, true), + MergeJudgeKind::Recipe + ); + assert_eq!( + resolve_merge_judge_kind(false, true, true), + MergeJudgeKind::Llm + ); +} + +#[test] +fn scenario2_self_deploy_head_advance_dedupe_and_argv_guard() { + let good = "a".repeat(40); + let other = "b".repeat(40); + + // Argv-injection guard: only full lowercase-hex SHAs are deployable. + assert!(is_valid_deploy_sha(&good)); + assert!(!is_valid_deploy_sha("--exec=rm -rf /")); + assert!(!is_valid_deploy_sha(&good.to_uppercase())); + + // A SHA that already SUCCEEDED is deduped (anti-thrash, #4387)... + let state = DeployHeadState { + last_deploy_target_sha: Some(good.clone()), + last_deploy_result: Some(DeployResult::Succeeded), + }; + assert!(!should_deploy_target_sha(&state, &good)); + // ...but a genuinely new merged head still advances (#4305). + assert!(should_deploy_target_sha(&state, &other)); + assert!(needs_head_advance(&good, &other)); + assert!(!needs_head_advance(&good, &good)); + // An argv-unsafe merged target never triggers an advance (fail-closed). + assert!(!needs_head_advance(&good, "not-a-sha")); + + // A missing systemd unit is reconciled; a loaded one is left alone. + assert_eq!( + classify_unit_load(false, "Unit simard.service not found."), + UnitLoadState::NotLoaded + ); + assert!(should_reconcile_unit(UnitLoadState::NotLoaded)); + assert!(!should_reconcile_unit(classify_unit_load(true, "enabled"))); +} + +#[test] +fn scenario2_done_gate_slug_convergence() { + let slug = "coin-benchmark-harness-09e65e35"; + let bot = "simard-overseer[bot]"; + + let prs = vec![ + DoneGatePr { + number: 4332, + author: bot.to_string(), + slug: slug.to_string(), + mergeable: "MERGEABLE".to_string(), + created_at: "2026-07-20T10:00:00Z".to_string(), + }, + DoneGatePr { + number: 4329, + author: bot.to_string(), + slug: slug.to_string(), + mergeable: "MERGEABLE".to_string(), + created_at: "2026-07-19T10:00:00Z".to_string(), // oldest CLEAN → survivor + }, + DoneGatePr { + number: 4326, + author: bot.to_string(), + slug: slug.to_string(), + mergeable: "CONFLICTING".to_string(), + created_at: "2026-07-18T10:00:00Z".to_string(), + }, + // A human PR and a different-slug PR must never be touched. + DoneGatePr { + number: 999, + author: "rysweet".to_string(), + slug: slug.to_string(), + mergeable: "MERGEABLE".to_string(), + created_at: "2026-07-01T10:00:00Z".to_string(), + }, + DoneGatePr { + number: 888, + author: bot.to_string(), + slug: "some-other-goal".to_string(), + mergeable: "MERGEABLE".to_string(), + created_at: "2026-07-01T10:00:00Z".to_string(), + }, + ]; + + let converge = converge_done_gate_prs(&prs, slug, bot); + assert_eq!( + converge.keep, + Some(4329), + "oldest CLEAN in-scope PR survives" + ); + let mut superseded = converge.supersede.clone(); + superseded.sort_unstable(); + assert_eq!( + superseded, + vec![4326, 4332], + "newer clean + stale conflicting in-scope duplicates are superseded; human/other-slug untouched" + ); + + // Slug sanitisation lowercases and strips shell/path metacharacters and + // whitespace (only [a-z0-9-] survive) before matching. + assert_eq!(sanitize_goal_slug("Coin/Bench $lug!"), "coinbenchlug"); + assert_eq!(sanitize_goal_slug("Coin-Bench-42"), "coin-bench-42"); +}