Start fresh when re-adding a reviewer

engine/app/controllers/coplan/api/v1/review_requests_controller.rb

@@ -35,9 +35,9 @@ def create
         def update
           if ActiveModel::Type::Boolean.new.cast(params[:request_again])
             Reviews::Request.call(plan: @plan, reviewer: @review_request.reviewer, actor: current_user,
-              requirement: params[:requirement].presence || @review_request.requirement, api_token: @api_token)
+              requirement: params[:requirement].presence || @review_request.requirement, api_token: @api_token, request_id: @review_request.id)
           else
-            @plan.with_lock { @review_request.update!(requirement: params.require(:requirement)) }
+            @plan.with_lock { @review_request.reload.update!(requirement: params.require(:requirement)) }
             Reviews::Changed.call(request: @review_request, event_type: "review.requested", actor_token_id: api_token_id,
               payload: { "action" => "requirement_changed", "requirement" => @review_request.requirement })
           end

engine/app/controllers/coplan/review_requests_controller.rb

@@ -1,7 +1,7 @@
 module CoPlan
   class ReviewRequestsController < ApplicationController
     before_action :set_plan
-    before_action :set_review_request, only: [ :update, :destroy, :respond, :compare ]
+    before_action :set_review_request, only: [ :update, :respond, :compare ]
     rescue_from Reviews::Respond::Conflict, with: :render_conflict
     rescue_from ActiveRecord::RecordInvalid, with: :render_invalid
 
@@ -17,10 +17,11 @@ def suggestions
 
     def create
       authorize!(@plan, :update?)
-      attributes = params.expect(review_request: [ :reviewer_id, :requirement ])
+      attributes = params.expect(review_request: [ :reviewer_id, :requirement, :replace_withdrawn ])
       reviewer = User.find(attributes[:reviewer_id])
       request = Reviews::Request.call(plan: @plan, reviewer: reviewer, actor: current_user,
-        requirement: attributes[:requirement].presence || "required", create_only: true)
+        requirement: attributes[:requirement].presence || "required", create_only: true,
+        replace_withdrawn: ActiveModel::Type::Boolean.new.cast(attributes[:replace_withdrawn]))
       render json: Reviews::Serialize.call(request), status: :created
     end
 
@@ -29,9 +30,9 @@ def update
       attributes = params.expect(review_request: [ :requirement, :request_again ])
       if ActiveModel::Type::Boolean.new.cast(attributes[:request_again])
         Reviews::Request.call(plan: @plan, reviewer: @review_request.reviewer, actor: current_user,
-          requirement: attributes[:requirement].presence || @review_request.requirement)
+          requirement: attributes[:requirement].presence || @review_request.requirement, request_id: @review_request.id)
       else
-        @plan.with_lock { @review_request.update!(requirement: attributes.fetch(:requirement)) }
+        @plan.with_lock { @review_request.reload.update!(requirement: attributes.fetch(:requirement)) }
         Reviews::Changed.call(request: @review_request, event_type: "review.requested",
           payload: { "action" => "requirement_changed", "requirement" => @review_request.requirement })
       end
@@ -40,7 +41,12 @@ def update
 
     def destroy
       authorize!(@plan, :update?)
-      Reviews::Withdraw.call(request: @review_request, actor: current_user)
+      if ActiveModel::Type::Boolean.new.cast(params.dig(:review_request, :remove))
+        Reviews::Remove.call(plan: @plan, request_id: params[:id])
+      else
+        set_review_request
+        Reviews::Withdraw.call(request: @review_request, actor: current_user)
+      end
       head :no_content
     end
 

engine/app/javascript/controllers/coplan/review_tray_controller.js

@@ -267,7 +267,7 @@ export default class extends Controller {
     event.preventDefault()
     if (!this.reviewerIdTarget.value) { this.assignmentErrorTarget.textContent = "Choose a person from the results first."; return }
     await this.withBusy(event.target, this.assignmentErrorTarget, async () => {
-      await this.write(this.urlValue, "POST", { review_request: { reviewer_id: this.reviewerIdTarget.value, requirement: event.target.querySelector('[type="checkbox"]').checked ? "required" : "optional" } })
+      await this.write(this.urlValue, "POST", { review_request: { reviewer_id: this.reviewerIdTarget.value, requirement: event.target.querySelector('[type="checkbox"]').checked ? "required" : "optional", replace_withdrawn: true } })
       this.searchTarget.value = ""; this.reviewerIdTarget.value = ""; this.resultsTarget.replaceChildren()
       this.closeAssignment()
       event.target.querySelector('[type="checkbox"]').checked = true
@@ -354,7 +354,7 @@ export default class extends Controller {
 
   requestAgain(event) { this.manage(event, "PATCH", { review_request: { request_again: true } }) }
   changeRequirement(event) { this.manage(event, "PATCH", { review_request: { requirement: event.currentTarget.dataset.requirement } }) }
-  withdraw(event) { this.manage(event, "DELETE") }
+  withdraw(event) { this.manage(event, "DELETE", { review_request: { remove: true } }) }
 
   async manage(event, method, body) {
     if (this.managing) return

engine/app/services/coplan/reviews/remove.rb

@@ -0,0 +1,22 @@
+module CoPlan
+  module Reviews
+    # A later assignment gets a fresh ID, so old response URLs stay invalid.
+    class Remove
+      def self.call(plan:, request_id:)
+        request = plan.with_lock do
+          request = plan.review_requests.find_by(id: request_id)
+          if request
+            unless plan.plan_events.where(event_type: "review_requested", after_value: request.id).exists?
+              RecordRequest.call(request: request)
+            end
+            request.destroy!
+          end
+          request
+        end
+        Changed.call(request: request, event_type: "review.requested",
+          payload: { "action" => "removed", "removed" => true }) if request
+        request
+      end
+    end
+  end
+end

engine/app/services/coplan/reviews/request.rb

@@ -1,27 +1,41 @@
 module CoPlan
   module Reviews
     class Request
-      def self.call(plan:, reviewer:, actor:, requirement: "required", api_token: nil, create_only: false)
-        new(plan:, reviewer:, actor:, requirement:, api_token:, create_only:).call
+      def self.call(plan:, reviewer:, actor:, requirement: "required", api_token: nil, create_only: false, request_id: nil, replace_withdrawn: false)
+        new(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, request_id:, replace_withdrawn:).call
       end
 
-      def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:)
+      def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, request_id:, replace_withdrawn:)
         @plan, @reviewer, @actor, @requirement, @api_token = plan, reviewer, actor, requirement, api_token
         @create_only = create_only
+        @request_id = request_id
+        @replace_withdrawn = replace_withdrawn
       end
 
       def call
         changed = false
+        removed = nil
         request = @plan.with_lock do
-          request = @plan.review_requests.find_or_initialize_by(reviewer: @reviewer)
-          # Retrying a create cannot change a decision, reopen a withdrawal,
-          # or send another nudge. Only an explicit re-request starts a cycle.
+          request = if @request_id
+            @plan.review_requests.where(reviewer: @reviewer).find(@request_id)
+          else
+            @plan.review_requests.find_or_initialize_by(reviewer: @reviewer)
+          end
+          # API create retries preserve every existing task. The web add form
+          # deliberately replaces withdrawn tasks left by earlier removals.
           if @create_only && request.persisted?
             unless ReviewRequest::REQUIREMENTS.include?(@requirement)
               request.errors.add(:requirement, :inclusion)
               raise ActiveRecord::RecordInvalid, request
             end
-            next request
+            next request unless @replace_withdrawn && request.status == "withdrawn"
+
+            unless @plan.plan_events.where(event_type: "review_requested", after_value: request.id).exists?
+              RecordRequest.call(request: request)
+            end
+            request.destroy!
+            removed = request
+            request = @plan.review_requests.new(reviewer: @reviewer)
           end
 
           # Preserve the original request for tasks created before request
@@ -39,6 +53,8 @@ def call
           changed = true
           request
         end
+        Changed.call(request: removed, event_type: "review.requested", actor_token_id: @api_token&.id,
+          payload: { "action" => "removed", "removed" => true }) if removed
         Changed.call(request: request, event_type: "review.requested", actor_token_id: @api_token&.id,
           payload: { "action" => "requested", "requirement" => request.requirement }) if changed
         request

engine/app/views/coplan/agent_instructions/guides/comments.text.erb

@@ -87,4 +87,5 @@ Discussion is the default. An approval means “satisfied with this exact versio
 - The plan author's credential may `GET /api/v1/plans/:plan_id/review_requests/suggestions` for suggested reviewers. Each entry in `suggestions` has `reviewer` (`id`, `name`, `username`), a `reason`, and a suggested `requirement`. A deployment may base them on the files and repositories the plan links to, so link the files a plan will change before asking. The list leaves out the author and people already reviewing, and is empty when the deployment suggests nobody. Suggestions are not requests. When your principal wants the plan reviewed, `POST` each chosen `reviewer.id` with its `requirement`.
 - `PATCH /api/v1/plans/:plan_id/review_requests/:id` accepts `requirement` without reopening the task or starting another review cycle. Use `{ "request_again": true }` when feedback is ready for another look. That preserves earlier responses and notifies the reviewer again. Live events distinguish these actions; see the live guide.
 - `DELETE` the same path withdraws the request and records the withdrawing actor and time in review activity. Earlier request cycles and decisions remain. It clears the reviewer's request nudge, not the author's unread responses. Archiving a plan does not withdraw pending reviews.
+- Removing a reviewer in the web management pane is different from API withdrawal: it deletes their request and past review decisions. Adding them again creates a new pending request with a new ID. Requests to the removed ID return 404; stop using that ID and fetch the current requests. Never retry an old response against a replacement request.
 - Only the requested reviewer's credential may `POST /api/v1/plans/:plan_id/review_requests/:id/respond` with `{ "revision": 4, "decision": "approved", "note": "optional public note", "idempotency_key": "RESPONSE_UUID" }` or `{ "revision": 4, "decision": "changes_requested", "note": "what needs adjustment", "idempotency_key": "RESPONSE_UUID" }`. Change requests create ordinary public comment threads. Agent responses are explicitly attributed to the token's agent acting for its human owner. A stale revision returns 409 with `code: review_version_changed`; re-read before responding. A new response to a withdrawn request returns 409 with `code: review_request_withdrawn`; stop responding unless the author requests another look. Replace `RESPONSE_UUID` with a fresh UUID for each intended response. Retry with the same UUID and identical fields if the HTTP response is lost. Use a new UUID for another review, even for the same version and decision. A keyed retry returns the original result even after a plan edit or withdrawal. It does not complete a newly re-requested task. Without a key, identical fields are treated as a retry. Retries may use a renewed token for the same human principal. The original response keeps its original agent and token attribution. Reusing a key with different fields returns 422. Never manufacture a human sign-off.

engine/app/views/coplan/agent_instructions/guides/live.text.erb

@@ -105,6 +105,7 @@ For `review.requested`, inspect `payload.action`:
 | `requested` | A new request or another look. `requirement` is `required` or `optional`. |
 | `requirement_changed` | Only `requirement` changed. This does not reopen the task or start another review cycle. Do not treat it as a request for another look. |
 | `withdrawn` | The request was withdrawn. Stop working on that request; its earlier decisions remain. The payload also has `withdrawn: true`. |
+| `removed` | The web management pane deleted this request and its review decisions. Stop using its ID. Reassignment creates a new pending request. The payload also has `removed: true`. |
 
 The attach and bridge tools show this review data and point to the review guide. Review events do not authorize content edits or execution.
 

engine/app/views/coplan/reviews/_roster.html.erb

@@ -21,7 +21,7 @@
                   <button type="button" class="menu__item" data-action="coplan--review-tray#changeRequirement coplan--menu#close" data-url="<%= plan_review_request_path(plan, request) %>" data-requirement="<%= requirement == 'required' ? 'optional' : 'required' %>">Make <%= requirement == 'required' ? 'optional' : 'required' %></button>
                   <% if approval && approval.plan_version.revision != plan.current_revision %><button type="button" class="menu__item" data-action="coplan--review-tray#compare coplan--menu#close" data-url="<%= compare_plan_review_request_path(plan, request, response_id: approval.id) %>">Changes since approval</button><% end %>
                   <div class="menu__separator" role="separator"></div>
-                  <button type="button" class="menu__item" data-action="coplan--review-tray#withdraw coplan--menu#close" data-url="<%= plan_review_request_path(plan, request) %>">Remove request</button>
+                  <button type="button" class="menu__item" data-action="coplan--review-tray#withdraw coplan--menu#close" data-url="<%= plan_review_request_path(plan, request) %>">Remove reviewer</button>
                 </div>
               </span>
             <% elsif approval && approval.plan_version.revision != plan.current_revision %>

spec/requests/api/v1/review_requests_spec.rb

@@ -14,6 +14,37 @@ def headers(token)
 
   before { author_token; reviewer_token }
 
+  [ true, false ].each do |keyed|
+    it "cannot replay a removed #{keyed ? 'keyed' : 'legacy'} review into a fresh request", :review_removal do
+      payload = { revision: 1, decision: "changes_requested", note: "Earlier feedback" }
+      payload[:idempotency_key] = SecureRandom.uuid if keyed
+      post respond_api_v1_plan_review_request_path(plan, task), params: payload, headers: headers("reviews-reviewer"), as: :json
+      expect(response).to have_http_status(:created)
+      old_response_id = response.parsed_body.fetch("id")
+      sign_in_as(author)
+      delete plan_review_request_path(plan, task), params: { review_request: { remove: true } }, as: :json
+      expect(response).to have_http_status(:no_content)
+      post api_v1_plan_review_requests_path(plan), params: { reviewer_id: reviewer.id }, headers: headers("reviews-owner"), as: :json
+      expect(response).to have_http_status(:created)
+      fresh = plan.review_requests.find(response.parsed_body.fetch("id"))
+      expect(fresh.id).not_to eq(task.id)
+      expect(response.parsed_body.fetch("responses")).to be_empty
+
+      post respond_api_v1_plan_review_request_path(plan, task), params: payload, headers: headers("reviews-reviewer"), as: :json
+      expect(response).to have_http_status(:not_found)
+      patch api_v1_plan_review_request_path(plan, task), params: { request_again: true }, headers: headers("reviews-owner"), as: :json
+      expect(response).to have_http_status(:not_found)
+      delete api_v1_plan_review_request_path(plan, task), headers: headers("reviews-owner")
+      expect(response).to have_http_status(:not_found)
+      expect(CoPlan::ReviewResponse.exists?(old_response_id)).to be(false)
+      expect(fresh.reload.status).to eq("pending")
+      get api_v1_plan_review_requests_path(plan), headers: headers("reviews-owner")
+      requests = response.parsed_body.fetch("review_requests")
+      expect(requests.map { |request| request.fetch("id") }).to eq([ fresh.id ])
+      expect(requests.first.fetch("responses")).to be_empty
+    end
+  end
+
   [ true, false ].each do |keyed|
     it "replays a #{keyed ? 'keyed' : 'legacy unkeyed'} response through a renewed session token without changing its attribution", :review_retries do
       first_token, first_secret = reviewer_token.mint_session_token!(agent_name: "Originalbot")

spec/requests/review_requests_spec.rb

@@ -6,6 +6,15 @@
   let(:plan) { create(:plan, :published, created_by_user: author) }
   let(:task) { create(:review_request, plan: plan, reviewer: reviewer) }
 
+  it "allows only the plan author to erase a reviewer's decisions", :review_removal do
+    previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved")
+    sign_in_as(reviewer)
+    delete plan_review_request_path(plan, task), params: { review_request: { remove: true } }, as: :json
+    expect(response).to have_http_status(:not_found)
+    expect(previous.reload.decision).to eq("approved")
+    expect(task.reload.status).to eq("approved")
+  end
+
   it "marks an unread note-less review decision as attention until its owner reads it", :review_attention do
     CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: plan.current_revision, decision: "approved")
     notice = author.notifications.find_by!(review_request: task, reason: "review_response")