Repository navigation
refactor(review): delete orphaned review authority after the SDD clean break #2423
Description
Activity
Real-world reproduction: legacy binding is reachable and lacks repository_context
Environment: gentle-ai 2.2.4, Linux x86_64, OpenCode 1.18.12
Minimal reproduction (fictional project, Calculator.kt + CalculatorTest.kt, 2 commits on a feature branch):
git init -b main
commit 1: scaffold (base)
git checkout -b feature/change
commit 2: add a method + test
gentle-ai review start --base-ref --committed-only=true
Observed:
- The direct/legacy route creates the lineage, but its state contains no repository_context (review-state.json → current_snapshot has base_tree/candidate_tree/paths but the artifact subject exposes no repository_context field). The CLI hint confirms this: "this response's selected lenses require the frozen Git trees, changed-path manifest, and artifact subjects, which only the negotiated contract form returns".
- Querying the negotiated facade afterward (gentle-ai review status --contract gentle-ai.review-integration/v2 --next-transition) ignores the existing legacy lineage and derives a new target from the current workspace, offering review.start again — it never emits a collect transition with repository_context for the frozen legacy lineage.
- The reviewer lens (e.g. review-reliability) that binds to gentle-ai review inspect-candidate fails with repository_context_unavailable / incomplete inspection, because the legacy binding lacks the token the template requires. The capture admission then rejects the result: "reviewer artifact admission incomplete: reviewer did not report completed candidate inspection".
Impact: a user following the documented compatibility path (--base-ref, explicitly "compatibility-supported for explicit/manual non-negotiated callers" in the review execution contract) hits a dead end: the legacy review can be started, but no lens can complete inspection and the negotiated facade never picks up the frozen lineage. The workaround I found was to not use the legacy route at all; the frozen review is effectively blocked until a released fix lands.
This suggests the "inert today" gap is actually reachable through a normal, documented usage path, which may matter for the legacy-retirement timeline in this issue.
@danizafa this is an excellent report and it corrects us on a claim we made in this very issue. Thank you for the minimal reproduction; having the exact sequence and the three observed effects separated is what made it verifiable in minutes rather than hours.
You are right, and the "inert today" framing above is wrong.
Confirmed in code:
RepositoryContextis populated ininternal/cli/review_start_contract.go, which is the negotiated path only. The direct route does not set it. The OpenCode reviewer plugin then requires that field in every binding shape it accepts, so a lens bound to a legacy lineage refuses before inspection, exactly as your third observation describes.We deferred the switch-removal work in this wave on the reasoning that the missing
repository_contextwas only reachable at the switch-on plus negotiated intersection. Your reproduction shows it is reachable through a route the execution contract explicitly calls compatibility-supported for explicit and manual non-negotiated callers. That is a documented path, and it dead-ends. The deferral itself still stands for its own reasons, but the rationale printed here needs correcting and now is.Your second observation is the one that worries me most, and it is the sharper of the three: the negotiated facade does not pick up an existing frozen legacy lineage. It derives a new target from the workspace and offers
review.startagain. So the recovery path cannot see the thing that needs recovering, which is the same impossible-loop shape we have been removing elsewhere. A review that can be started and never completed is bad; one whose recovery route is blind to it is worse.What happens now. Tracked separately so it does not ride on a wave-landing issue. Two layers, in the order they will land:
- The legacy start route refuses up front when it cannot produce a completable review, naming the negotiated form as the exit, so nobody else spends your afternoon discovering this. Small, and it stands alone.
- The negotiated facade recognises an existing legacy lineage rather than deriving past it, and the deeper
repository_contextwork lands with the switch-removal successor change.
Your workaround is the right one meanwhile: stay on the negotiated form. If you want the reviews you already started to be recoverable rather than abandoned, say so, because that changes how we sequence the second layer.
Thanks again. Reports that arrive with the cause already isolated are worth several that do not.
- changed the title
[-]feat(review): land RDD root simplification Wave 7 (legacy retirement)[/-][+]refactor(review): delete orphaned review authority after the SDD clean break[/+]on Aug 22, 2026 This approved issue is now the global cleanup follow-up after #3564. The earlier Wave 7 compatibility deferrals are superseded: old formats and authority representations are not migrated or dual-read, while the current atomic compact-v2 lifecycle remains only where a named production path and test prove it is live. Work starts from the post-#3564 tree and deletes by causal closure, including tests, assets, docs, refusals, ratchets, and stale issue or PR roots. This avoids creating a duplicate tracker and gives the deferred #3417 cleanup one durable owner.
Read-only follow-up audit after closing #1596 and #3643 found one post-atomic cleanup slice for this tracker, not a new selector-free recovery bug. On origin/main 26d7cee, j59 covers sibling-worktree isolation and explicit-lineage continuation has unit and driven coverage, but no active journey pairs a same-worktree restart with both required outcomes: selector-free STATUS remains preflight-only, while STATUS with the preserved lineage returns the exact pending collect transition. More urgently, README-linked docs and shipped Claude/OpenCode sdd-apply assets still recommend the retired public FINALIZE phase. Suggested post-#3564 slice: remove that stale guidance, document restart continuation with explicit lineage/revision/target, and add the paired negative selector-free plus positive explicit-lineage journey. No implementation has started because this issue explicitly begins from the completed post-#3564 tree.
I'm taking the scoped post-#3564 follow-up described in the latest audit comment: remove stale FINALIZE guidance from README-linked docs and shipped Claude/OpenCode sdd-apply assets, document restart continuation with explicit lineage/revision/target, and first map whether the paired selector-free/explicit-lineage journey is still missing on current main. I'll keep this as a narrow slice and won't attempt the full cleanup tracker.
Pre-flight Checklist
Outcome
Delete every review/RDD production surface outside SDD that becomes orphaned after #3564 removes its final consumer. Retain only code with a named live path in the current atomic compact-v2 lifecycle.
This is deletion, not migration. Historical formats, versions, bindings, receipts, recovery shells, switches, and compatibility readers carry no lifecycle authority and are not recognized by the replacement implementation.
Dependency
This work begins after #3564 lands its complete SDD cleanup. The post-#3564 source tree is the only valid reachability baseline. Pre-#3564 references do not justify retaining a provider whose final consumer is scheduled for deletion.
Maintainer Decisions
D1. No backward compatibility
Old review formats and authority representations are unsupported. A replaced version fails closed with one actionable instruction to start a fresh atomic review. There is no dual reader, compatibility translator, migration, record rewrite, or legacy fallback.
D2. Preserve only the live atomic lifecycle
The retained review lifecycle is:
Fresh STATUS/FINALIZE routing, exact consent, immutable candidate binding, reviewer admission, bounded correction, validator evidence, and terminal burn remain only when they are reachable from this lifecycle.
Approval leaves no terminal receipt, lineage, witness, tombstone, mirror, gate authority, or durable review history. Git remains the durable project history.
D3. Delete by causal closure
When a consumer is deleted, delete its newly unreachable providers, direct tests, fixtures, schemas, help text, refusal continuations, ratchet entries, manifests, docs, and compile references in the same causal work unit. Do not baseline newly dead code and do not preserve a helper solely because a test calls it.
D4. Evidence required for retention
Every retained review symbol must have all three:
Anything missing one of these is deletion scope unless a separate approved issue owns a concrete current use.
Scope
After #3564 merges, inventory and delete orphaned review/RDD code outside
internal/sddstatus, including as applicable:Out of Scope
Acceptance Criteria
go test ./...passes at repository root, along with vet, format, refusal/dead-code ratchets, platform builds, and CI-equivalent driven review journeys.Delivery Strategy
Use stacked-to-main, deletion-first slices by causal boundary. Each slice must be independently buildable and reviewable, and tests stay with the behavior they prove. If one causal deletion exceeds 400 changed lines, record the exact reason before applying
size:exception; do not split a consumer from the providers that become dead because of its removal.References