From fee4a8474a6329318487a78d6ec22b7ccc6ad885 Mon Sep 17 00:00:00 2001 From: Daniel Green Date: Wed, 13 May 2026 21:04:51 -0700 Subject: [PATCH] plan(3166): write plan-3166.md --- plans/plan-3166.children.json | 1 + plans/plan-3166.md | 52 +++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 plans/plan-3166.children.json create mode 100644 plans/plan-3166.md diff --git a/plans/plan-3166.children.json b/plans/plan-3166.children.json new file mode 100644 index 00000000..0637a088 --- /dev/null +++ b/plans/plan-3166.children.json @@ -0,0 +1 @@ +[] \ No newline at end of file diff --git a/plans/plan-3166.md b/plans/plan-3166.md new file mode 100644 index 00000000..c8f49b04 --- /dev/null +++ b/plans/plan-3166.md @@ -0,0 +1,52 @@ +--- +apex_facets: [implementable] +--- + +# Scope reviewer direction-asymmetry: distinguish empty-MG cases + +## Summary +The MG `scope_reviewer` agent in [.conductor/registry/workflows/implement-merge-group.yaml](.conductor/registry/workflows/implement-merge-group.yaml) currently collapses three distinct "zero commits on `feature..mg`" outcomes into a single `empty_merge_group_structural_violation` → `changes_requested` verdict, which then loops to the cap gate with no path to converge. We will replace the single boolean check with a typed three-way disposition (genuine empty / no-op / already-merged-forward) that approves cases B and C and only requests changes for case A. + +## Problem / Motivation +The reviewer's empty-MG check is a single boolean (`commit_count == 0`). That collapses three semantically different situations: + +| Case | What happened | Correct disposition | +|------|--------------|--------------------| +| A. Genuinely empty MG | Classifier/seeder/feature-pr-router routed no work | `changes_requested` | +| B. Nothing-to-do MG | Impl agent decided no code change is needed (spec-only, sibling already did it) | `approved` (no-op rationale) | +| C. Already-merged-forward | Fix landed on `main` between dispatch and review (e.g. via a different PR), so `feature/{id}` already contains it | `approved` (already-satisfied rationale) | + +Reproductions: AB#3157 (stale local refs sibling), AB#3127 first run (cap at cycle 3), AB#3127 second run May 13 (cap at cycle 12 — case C, fix landed on `main` via PR #363's typed-lint sweep). Any work item where the bug got fixed elsewhere will hit the gate forever. Operators have no convergence path. + +## Proposed Approach +Edit the `scope_reviewer` agent prompt and routes in [.conductor/registry/workflows/implement-merge-group.yaml](.conductor/registry/workflows/implement-merge-group.yaml) (lines ~876–1011, especially the empty-MG block at 924–959) to perform three ordered checks before falling through to the structural-violation verdict: + +1. **Case C (already-merged-forward, cheapest signal first).** Run `git log --oneline origin/main..origin/{feature_branch}` (forward direction). If `feature/{id}` has zero commits ahead of `main`, the apex's change is already on `main` (either via a sibling PR or fast-forwarded). Return `verdict: "approved"` with `issues: ["already_satisfied"]` and feedback explaining the apex is already on `main`. +2. **Case B (impl agent declared no-op).** Inspect upstream impl agent outcome by reading the per-task impl PR descriptions / branch state (the impl agent records its rationale in the impl PR body). If every task in `work_item_ids` was completed with a documented "no change required" outcome, return `verdict: "approved"` with `issues: ["no_op"]`. +3. **Case A (genuine empty).** Only if both checks fail does the reviewer return the existing `empty_merge_group_structural_violation` → `changes_requested` verdict. + +Keep the existing structural checks (a)–(d) — branch existence, file-stat zero, dispatched-with-no-tasks — as deterministic guards that run before the case A/B/C disambiguation. The `feedback` field must name which case fired so the operator sees the disposition. + +**Lint update.** [.conductor/registry/tests/lint-scope-reviewer-empty-merge-group.Tests.ps1](.conductor/registry/tests/lint-scope-reviewer-empty-merge-group.Tests.ps1) currently asserts the prompt forbids punting and matches `covered\s+elsewhere`. The lint must be relaxed/extended to (a) still require the case-A guardrail (no green-washing of dispatcher bugs), AND (b) require the new case-B / case-C disposition language and the forward-direction `git log main..feature/` check. + +**Harness coverage.** Add a scenario under [tests/harness/scenarios/](tests/harness/scenarios/) where `feature/{id}` is created from a `main` that already contains the equivalent fix (case C). Assert the pipeline reaches `terminal_apex_satisfied`, not the cap gate. (Optional: a second scenario for case B where the FakeProvider impl agent returns a no-op rationale.) + +**No-op signal plumbing (only if needed).** If the impl agent's no-op rationale is not already reachable from the reviewer's context, add the necessary plumbing — either by reading impl PR bodies via `gh`/`twig` from the reviewer prompt (no schema change required), or by surfacing it through `scope_guidance_loader`. Prefer the prompt-level read to avoid a workflow-schema change. + +## Acceptance Criteria +- [ ] `scope_reviewer` in `implement-merge-group.yaml` returns `approved` with `issues: ["already_satisfied"]` when `git log origin/main..origin/{feature_branch}` shows zero commits ahead of main (case C). +- [ ] `scope_reviewer` returns `approved` with `issues: ["no_op"]` when every dispatched task recorded a no-op outcome and case C does not apply (case B). +- [ ] `scope_reviewer` continues to return `changes_requested` with `empty_merge_group_structural_violation` only for case A (genuine empty — work was dispatched, no commits on MG, AND case C/B both refuted). +- [ ] `feedback` text names which of cases A/B/C fired so operators see the disposition without re-running git. +- [ ] [.conductor/registry/tests/lint-scope-reviewer-empty-merge-group.Tests.ps1](.conductor/registry/tests/lint-scope-reviewer-empty-merge-group.Tests.ps1) is updated to assert the new three-way disposition language while preserving the AB#3064 anti-green-washing guardrail; all existing assertions either pass or are intentionally migrated. +- [ ] A harness scenario reproduces case C (`feature/{id}` branched from a `main` that already contains the fix) and reaches `terminal_apex_satisfied` rather than the scope-revise cap gate. +- [ ] Existing implement-merge-group lint and harness tests continue to pass (no regression of the genuine-empty-MG guardrail from AB#3064). +- [ ] Build passes with zero errors and warnings. +- [ ] All existing tests pass; new tests cover the three-way disposition. + +## Context +- Symptom-side issue: AB#3127 (run #2 surfaced case C at cap 12). +- Sibling reviewer-correctness bug: AB#3157 (stale local refs — already fixed upstream of this work in [.conductor/registry/workflows/implement-merge-group.yaml](.conductor/registry/workflows/implement-merge-group.yaml) lines 819–835). +- Related (re-run idempotency): AB#3165 — pollution makes case C more common but case C exists independently. +- Primary file to change: [.conductor/registry/workflows/implement-merge-group.yaml](.conductor/registry/workflows/implement-merge-group.yaml). +- Lint to update: [.conductor/registry/tests/lint-scope-reviewer-empty-merge-group.Tests.ps1](.conductor/registry/tests/lint-scope-reviewer-empty-merge-group.Tests.ps1).