Skip to content

fix(review): salvage frozen correction binding - #2599

Merged
Alan-TheGentleman merged 3 commits into
mainfrom
fix/2132-frozen-correction-binding-salvage
Aug 5, 2026
Merged

fix(review): salvage frozen correction binding#2599
Alan-TheGentleman merged 3 commits into
mainfrom
fix/2132-frozen-correction-binding-salvage

Conversation

@Alan-TheGentleman

@Alan-TheGentleman Alan-TheGentleman commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Linked Issue

Fixes #2132


PR Type

  • type:bug - Bug fix

Summary

  • Salvages Jalmar Villarreal's approved frozen-authority binding fix from fix(review): accept frozen correction-plan binding after applied fix #2240 onto current main.
  • Preserves contributor provenance with -x cherry-picks of only ffff87dfcb834435c719925a8af7f920e7a32a79 and 7dbd6da54831340a0028805b97bfea1ba5787e00.
  • Uses the current v2 status boundary with its required runtime identity and declares the fixture candidate under current scope rules.

Changes

File / Area What Changed
internal/cli/review_status_contract.go Bind CorrectionRequest.TargetIdentity to reviewAuthorityTargetIdentity(result), not the live workspace identity.
internal/cli/review_status_contract_test.go Exercise the regression through negotiated v2 status and state its intentionally narrow scope.

Test Plan

  • Genuine RED: final v2 regression failed on the parent production comparison with operation_failed in the unforecast, live-diverged branch.
  • GREEN: the same regression passes after the one-line frozen-authority correction.
  • Focused correction/status tests, including TestCorrectionNextTransitionAgreesBetweenFinalizeAndRestartStatus and TestNegotiatedStatusUnderUnavailableProcessTempContinuesCompactCorrection.
  • go test ./... -count=1
  • Focused go test -race ./internal/cli ... -count=1
  • go run ./internal/gofmtcheck, go vet ./..., and go build ./...
  • Refusal ratchets, byte-equivalence golden tests, and ./scripts/deadcode-ratchet.sh
  • GOOS=windows GOARCH=amd64 go build ./... and GOOS=darwin GOARCH=arm64 go build ./...
  • Bench declaration test plus driven j51-negotiated-status-correction-continuation: 1 completed, 0 failed.
  • Docker E2E was not run locally; CI will execute its required lane.

Contributor Checklist

  • PR is linked to an approved issue.
  • PR stays below 400 changed lines (+89/-1).
  • Exactly one type:bug label is applied.
  • Unit tests and Go format pass.
  • Benchmark validation completed.
  • Documentation is not required for this internal contract correction.
  • Commits use Conventional Commit messages and contain no Co-Authored-By trailers.

Notes for Reviewers

This is a maintainer salvage of #2240, not a merge or closure of #2240. The two contributor commits above retain Jalmar Villarreal as author and record their original SHA through -x; the third, conventional maintainer commit only updates the test fixture and invocation for current v2 requirements.

The regression proves producer/validator consistency only for the unforecast branch where the live workspace diverges after an uncommitted correction. It does not claim to prove the correctly ordered forecasted bounded-correction flow. That flow already routes to correction_repository_verification_required with targeted validation and is exercised by TestCorrectionNextTransitionAgreesBetweenFinalizeAndRestartStatus and driven bench journey j51.

No qualifying security, integrity, admission, repair, or governance guard was added or changed; .guard-population-baseline.txt is unchanged.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected correction-request validation to use the appropriate authority identity.
    • Improved status handling after an applied but uncommitted fix, including changed-workspace and request-build-error scenarios.
    • Preserved authority state during status evaluation and clarified returned correction reasons.

Jalmar Villarreal and others added 3 commits August 5, 2026 18:19
Fix CodeRabbit finding: defensive nil check on NextTransition before
accessing CorrectionRequest field in regression test.

(cherry picked from commit 7dbd6da)
Copilot AI lite review requested due to automatic review settings August 5, 2026 16:29
@Alan-TheGentleman Alan-TheGentleman added the type:bug Bug fix label Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 786585a6-67e9-47a3-94b0-be4183994bc9

📥 Commits

Reviewing files that changed from the base of the PR and between c18e512 and 4d9ec6c.

📒 Files selected for processing (2)
  • internal/cli/review_status_contract.go
  • internal/cli/review_status_contract_test.go

📝 Walkthrough

Walkthrough

Correction-plan status now validates correction requests against the frozen authority target identity. Regression coverage checks applied uncommitted fixes, workspace identity divergence, reason codes, and read-only authority preservation.

Changes

Correction status authority binding

Layer / File(s) Summary
Authority binding and regression coverage
internal/cli/review_status_contract.go, internal/cli/review_status_contract_test.go
Correction requests use the resolved authority target identity. Tests cover changed and forecasted workspaces after an applied fix and verify stable authority state.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the fix to frozen correction binding.
Linked Issues check ✅ Passed The change binds correction requests to frozen authority identity and adds regression coverage for negotiated status after bounded correction [#2132].
Out of Scope Changes check ✅ Passed The implementation and regression test directly support the linked issue and stated review-status objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/2132-frozen-correction-binding-salvage

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a negotiated review.status contract validation bug where a correction plan request’s TargetIdentity was incorrectly validated against the live workspace identity instead of the frozen (authority) target identity, which can diverge after an uncommitted bounded correction. It also adds a focused regression test reproducing the unforecasted diverged-workspace scenario under the current v2 contract/status behavior.

Changes:

  • Update ReviewTargetStatusResult.Validate() to validate CorrectionRequest.TargetIdentity against reviewAuthorityTargetIdentity(result) (frozen authority identity when present).
  • Add a regression test covering the diverged-workspace (unforecast) path and a forecasted stop-path variant, asserting the correction request binds to the frozen identity and that status remains read-only.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/cli/review_status_contract.go Fixes correction request binding validation to use the frozen authority target identity instead of the live workspace identity.
internal/cli/review_status_contract_test.go Adds regression coverage ensuring negotiated status accepts frozen binding after an applied, uncommitted fix and preserves read-only semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

review status fails before authority evaluation after bounded correction

2 participants