fix(review): accept frozen correction-plan binding after applied fix - #2240
Closed
Jalmar01 wants to merge 8 commits into
Closed
fix(review): accept frozen correction-plan binding after applied fix#2240Jalmar01 wants to merge 8 commits into
Jalmar01 wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCorrection status now uses the authority-specific target identity. Target status promotes eligible approved scope-changed recovery candidates. Regression coverage validates frozen bindings and read-only status behavior after bounded corrections. ChangesCorrection and recovery status
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/review_status_contract_test.go`:
- Around line 1147-1150: Update the transition validation in the review status
test to assign status.NextTransition to transition and validate that transition
and its ReasonCode before accessing transition.CorrectionRequest. Only read
CorrectionRequest after confirming transition is non-nil, while preserving the
existing failure diagnostic output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e4027d7-dca2-40ab-a5e9-d5a0a82493ef
📒 Files selected for processing (2)
internal/cli/review_status_contract.gointernal/cli/review_status_contract_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/reviewtransaction/target_status.go`:
- Around line 303-330: In the scope-change promotion block, replace the
re-sliced `scopeChangedCandidates[:1]` assignment with a fresh one-element slice
containing `sc`, so updating `promoted[0].correctionRecovery` and
`promoted[0].recoveryDisposition` cannot mutate `scopeChangedCandidates[0]`.
Keep the existing promotion conditions and candidate replacement behavior
unchanged.
- Around line 281-302: Refactor the duplicate “sole governing authority” logic
in the surrounding target-status selection flow into a shared helper, and use it
from both this block and Block B. Add an explicit len(candidates) > 0 guard here
to match Block B, then remove the unreachable issue `#1826` fallback that checks
len(candidates) == 0 with len(approvedScopeRecovery) == 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f30b1b4-8211-413b-8be9-6ee263048e78
📒 Files selected for processing (1)
internal/reviewtransaction/target_status.go
Jalmar01
pushed a commit
to Jalmar01/gentle-ai
that referenced
this pull request
Aug 3, 2026
…x slice aliasing
- Extract isSoleCorrectionRecoveryGoverning helper used by both
approvedScopeRecovery and scopeChangedCandidates promotion blocks
- Add explicit len(candidates) > 0 guard to approvedScopeRecovery block
- Fix scope-changed promotion: use fresh slice []targetStatusCandidate{sc}
instead of scopeChangedCandidates[:1] to avoid mutating original
- Remove unreachable fallback (issue Gentleman-Programming#1826) — covered by first block
Addresses CodeRabbit findings on PR Gentleman-Programming#2240 (target_status.go).
added 4 commits
August 3, 2026 14:28
Fix CodeRabbit finding: defensive nil check on NextTransition before accessing CorrectionRequest field in regression test.
…ority in STATUS Issue Gentleman-Programming#2345: STATUS was selecting a historical correction-required authority instead of the approved delivery with canonical receipt when a small follow-up touched the same tracked files. This inflated original_changed_lines (11619 vs actual small diff) and incorrectly required recovery authorization. Fix mirrors START command precedence (compact_store.go:1095-1099): - recoveryCandidates (approved scope-changed deliveries) govern over claimants (correction-required authorities) - Promotes approvedScopeRecovery when it's the sole governing authority - Also promotes scopeChangedCandidates with canonical receipts when only correction-required claimants exist in candidates All existing TestAssessTargetStatus* tests pass (27 tests).
…x slice aliasing
- Extract isSoleCorrectionRecoveryGoverning helper used by both
approvedScopeRecovery and scopeChangedCandidates promotion blocks
- Add explicit len(candidates) > 0 guard to approvedScopeRecovery block
- Fix scope-changed promotion: use fresh slice []targetStatusCandidate{sc}
instead of scopeChangedCandidates[:1] to avoid mutating original
- Remove unreachable fallback (issue Gentleman-Programming#1826) — covered by first block
Addresses CodeRabbit findings on PR Gentleman-Programming#2240 (target_status.go).
Jalmar01
force-pushed
the
fix/2132-review-status-post-correction
branch
from
August 3, 2026 19:29
79771dc to
c9692a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/reviewtransaction/target_status.go`:
- Around line 567-576: Update isSoleCorrectionRecoveryGoverning to require every
candidate’s state to be StateCorrectionRequired, in addition to the existing
correctionRecovery, retry, and escalation checks. Return false for invalidated,
approved, or other states so only compact StateCorrectionRequired candidates can
be promoted over.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ccbd3cd7-5b40-4697-a97d-10c40444f45f
📒 Files selected for processing (3)
internal/cli/review_status_contract.gointernal/cli/review_status_contract_test.gointernal/reviewtransaction/target_status.go
Comment on lines
+567
to
+576
| func isSoleCorrectionRecoveryGoverning(candidates []targetStatusCandidate) bool { | ||
| for _, c := range candidates { | ||
| if !c.correctionRecovery || c.finalVerificationRetry != nil { | ||
| return false | ||
| } | ||
| if c.recoveryDisposition == RecoveryEscalated { | ||
| return false | ||
| } | ||
| } | ||
| return true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict promotion inputs to StateCorrectionRequired candidates.
This predicate accepts invalidated and approved scope recovery candidates because they set correctionRecovery. Lines 285 and 293 can then replace those governing candidates with another approved scope-changed candidate.
Require every promoted-over candidate to be a compact StateCorrectionRequired candidate.
Proposed fix
func isSoleCorrectionRecoveryGoverning(candidates []targetStatusCandidate) bool {
for _, c := range candidates {
- if !c.correctionRecovery || c.finalVerificationRetry != nil {
+ if c.compact == nil || c.compact.State.State != StateCorrectionRequired ||
+ !c.correctionRecovery || c.finalVerificationRetry != nil {
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func isSoleCorrectionRecoveryGoverning(candidates []targetStatusCandidate) bool { | |
| for _, c := range candidates { | |
| if !c.correctionRecovery || c.finalVerificationRetry != nil { | |
| return false | |
| } | |
| if c.recoveryDisposition == RecoveryEscalated { | |
| return false | |
| } | |
| } | |
| return true | |
| func isSoleCorrectionRecoveryGoverning(candidates []targetStatusCandidate) bool { | |
| for _, c := range candidates { | |
| if c.compact == nil || c.compact.State.State != StateCorrectionRequired || | |
| !c.correctionRecovery || c.finalVerificationRetry != nil { | |
| return false | |
| } | |
| if c.recoveryDisposition == RecoveryEscalated { | |
| return false | |
| } | |
| } | |
| return true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/reviewtransaction/target_status.go` around lines 567 - 576, Update
isSoleCorrectionRecoveryGoverning to require every candidate’s state to be
StateCorrectionRequired, in addition to the existing correctionRecovery, retry,
and escalation checks. Return false for invalidated, approved, or other states
so only compact StateCorrectionRequired candidates can be promoted over.
added 4 commits
August 3, 2026 14:38
…ity check isSoleCorrectionRecoveryGoverning now validates c.compact.State.State == StateCorrectionRequired, per CodeRabbit review. Only compact StateCorrectionRequired candidates can be promoted over by an approved recovery candidate. Addresses CodeRabbit finding on PR Gentleman-Programming#2240 (target_status.go:567-576).
…change Issue Gentleman-Programming#2349: STATUS was advertising escalated recovery for accounting-only escalations (correction passed but budget exceeded), but RecoverCompactAuthority rejected it because the target was byte-identical to the predecessor. Fix: validateCompactRecoveryEdge now skips the target-changed check for accounting-only escalations (compactAccountingOnlyEscalation returns true), matching the behavior of STATUS which offers this recovery via the accounting-only path. This makes STATUS and Recover consistent for accounting-only escalations.
…n-eligibility Issue Gentleman-Programming#2340: review finalize rejected --next-transition and --action-eligibility with contract v2, even though review status accepts v2. The error was also misclassified as operation_outcome_unknown instead of a clean validation error. Changes: - Update error message to allow v1 OR v2 contract - Require v2 specifically when --next-transition or --action-eligibility is used - Use reviewPreflightError for proper error classification - Update test to expect new message format Now finalize --next-transition --contract v2 works like status --next-transition --contract v2.
Fix potential 63-char SHA-256 issue when first byte is 0x00. fmt.Sprintf with %x drops leading zero bytes; hex.EncodeToString preserves all bytes. This addresses the root cause of issue Gentleman-Programming#2252 where negotiated review.start emitted a target with 63 hex chars instead of 64.
18 tasks
Contributor
|
Thanks for the original diagnosis and fix. The valid #2132 work was salvaged and merged through #2599 as I am closing this PR because its branch later accumulated six unrelated commits for #2345, #2349, #2340, and #2252. Some of those changes are now stale or unsafe against current main, and retaining them would make the review scope misleading. The focused replacement contains only the frozen correction-binding fix and its v2 regression. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Scope
In scope (this PR):
ReviewTargetStatusResult.Validate(line 375) to compare against the frozen authority identity viareviewAuthorityTargetIdentity, restoringcorrection_plan_requiredafter a bounded correction with an uncommitted workspace.TestCorrectionPlanStatusAcceptsFrozenBindingAfterAppliedFix(2 subtests, platform-agnostic) covering both branches of the fallback rule.Out of scope (deliberate decisions):
CorrectionPlanRequest.TargetIdentityto the live workspace identity — larger blast radius, would weaken the plan's binding to the reviewed candidate.review_operation_contract.go:869-877still emitsretry_safe: true/next_action: retryfor deterministic contract-internal validation failures. Known limitation, documented, not fixed here. If a deterministic failure occurs, the consumer still gets misleading retry advice; a follow-up change is required.gentle-ai.review-integration/v2contract is unchanged.🔗 Linked Issue
Closes #2132
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)📝 Summary
After a bounded correction in an active negotiated review, the read-only
review statusoperation failed deterministically in thepre_nativephase withoperation_failed/retry_safe: true/next_action: retry. Root cause:ReviewTargetStatusResult.Validatecompared the correction-plan request'sTargetIdentity(bound to the frozen reviewed candidate) against the live workspace identity, which diverges the moment the bounded fix lands uncommitted. The fix compares against the frozen authority identity via the existingreviewAuthorityTargetIdentityhelper (falling back toTargetIdentitywhen absent), restoringcorrection_plan_requiredafter a bounded correction. No schema change, no new helper.📂 Changes
internal/cli/review_status_contract.goreviewAuthorityTargetIdentity(result)instead ofresult.TargetIdentityinternal/cli/review_status_contract_test.goTestCorrectionPlanStatusAcceptsFrozenBindingAfterAppliedFix— 2 platform-agnostic subtests (no forecast + changed workspace →correction_plan_required; forecast + request-build error →corrected_candidate_unavailable)🧪 Test Plan
go build ./internal/cli/...— cleango test ./internal/cli -run TestCorrectionPlanStatusAcceptsFrozenBindingAfterAppliedFix -count=1 -v— PASS 2/2 (fresh)go test ./internal/cli— ok (233.7s)go test ./internal/reviewtransaction— ok (176.3s)go run ./internal/gofmtcheck— exit 0go vet ./...— clean✅ Contributor Checklist
status:approvedtype:buglabel applied — pending maintainer (contributor cannot add labels)Co-Authored-BytrailersPending maintainer actions
type:buglabel to this PR (contributor cannot apply labels — GraphQL 403 is policy)💬 Notes for Reviewers
The correction-plan request binds to the frozen reviewed candidate (
correction_plan_request.go:71). The contract result exposesAuthorityTargetIdentityonly when it differs from the liveTargetIdentity(review_status_contract.go:195-198), so the fallback keeps unchanged-workspace fixtures green. A diverged-identity + request-build-error path is unreachable through the facade (it routes tocorrection_repository_verification_requiredwithout a CorrectionRequest), so the test covers the reachablecorrected_candidate_unavailableshape with an unchanged candidate.Summary by CodeRabbit