Skip to content

plan: root #3166 #371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PolyphonyRequiem merged 1 commit into from
May 14, 2026
Merged

plan: root #3166 #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions plans/plan-3166.children.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[]
52 changes: 52 additions & 0 deletions plans/plan-3166.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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).