From ed9b185814f42981f2ba23a9a040d63bc9a587ab Mon Sep 17 00:00:00 2001 From: Sean Prashad <13009507+seanprashad@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:24:15 -0400 Subject: [PATCH 1/5] Restore removed reviewers when re-added --- .../coplan/review_tray_controller.js | 10 +- spec/system/plan_reviews_spec.rb | 96 +++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/engine/app/javascript/controllers/coplan/review_tray_controller.js b/engine/app/javascript/controllers/coplan/review_tray_controller.js index e818fb8b..10ee02f7 100644 --- a/engine/app/javascript/controllers/coplan/review_tray_controller.js +++ b/engine/app/javascript/controllers/coplan/review_tray_controller.js @@ -267,7 +267,15 @@ 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" } }) + const requirement = event.target.querySelector('[type="checkbox"]').checked ? "required" : "optional" + const request = await this.write(this.urlValue, "POST", { review_request: { reviewer_id: this.reviewerIdTarget.value, requirement } }) + // Create retries preserve withdrawn tasks. Re-adding someone here is + // an explicit new request; reopen it through the existing update API. + // If that result is lost, the next create returns the pending task + // and does not send another request or notification. + if (request.status === "withdrawn") { + await this.write(`${this.urlValue}/${encodeURIComponent(request.id)}`, "PATCH", { review_request: { request_again: true, requirement } }) + } this.searchTarget.value = ""; this.reviewerIdTarget.value = ""; this.resultsTarget.replaceChildren() this.closeAssignment() event.target.querySelector('[type="checkbox"]').checked = true diff --git a/spec/system/plan_reviews_spec.rb b/spec/system/plan_reviews_spec.rb index 47fa5804..4feeaf46 100644 --- a/spec/system/plan_reviews_spec.rb +++ b/spec/system/plan_reviews_spec.rb @@ -66,6 +66,102 @@ def lose_next_review_result end end + [ "light", "dark" ].each do |theme| + it "re-adds a removed reviewer through the assignment pane in #{theme} theme", :review_readdition do + approval = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved") + sign_in_browser(author) + page.execute_script("document.documentElement.dataset.theme = arguments[0]", theme) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") do + click_button "Actions for Plan Reviewer", enable_aria_label: true + click_button "Remove request" + expect(page).to have_no_css("[data-review-assignments] .review-person") + click_button "Add reviewer", exact: true + end + expect(task.reload.status).to eq("withdrawn") + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + uncheck "Required" + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + within(".review-management[open] [data-review-assignments]") do + expect(page).to have_css(".review-person", text: "Plan Reviewer") + expect(page).to have_css(".review-person__requirement", text: "Optional") + expect(page).to have_css(".review-state", text: "Pending") + end + expect(task.reload).to have_attributes(status: "pending", requirement: "optional") + expect(plan.review_requests.where(reviewer: reviewer).count).to eq(1) + expect(task.review_responses.pluck(:id)).to eq([ approval.id ]) + expect(task.notifications.unread.where(user: reviewer, reason: "review_requested").count).to eq(1) + page.save_screenshot(Rails.root.join("tmp/review-preview/readded-reviewer-#{theme}.png")) + sign_in_browser(reviewer) + expect(page).to have_css(".review-tray", text: "Your review is requested") + end + end + + [ "lost", "rejected" ].each do |failure| + it "keeps re-adding a reviewer retryable after a #{failure} update result", :review_readdition do + CoPlan::Reviews::Withdraw.call(request: task, actor: author) + sign_in_browser(author) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") { click_button "Add reviewer", exact: true } + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + page.execute_script(<<~JS, failure) + const originalFetch = window.fetch.bind(window) + let interrupt = true + const failure = arguments[0] + window.fetch = async (url, options) => { + if (interrupt && options?.method === 'PATCH' && String(url).includes('/review_requests/')) { + interrupt = false + if (failure === 'rejected') return new Response(JSON.stringify({error: 'Please retry this request'}), {status: 422, headers: {'Content-Type': 'application/json'}}) + await originalFetch(url, options) + throw new TypeError('Lost the result after the server reopened the task') + } + return originalFetch(url, options) + } + JS + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + expect(page).to have_css(".review-assignment [role='alert']", text: failure == "lost" ? "Couldn't reach CoPlan" : "Please retry this request") + expect(page).to have_field("Find a reviewer", with: reviewer.name) + expect(task.reload.status).to eq(failure == "lost" ? "pending" : "withdrawn") + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + within(".review-management[open] [data-review-assignments]") do + expect(page).to have_css(".review-person", text: reviewer.name) + expect(page).to have_css(".review-state", text: "Pending") + end + expect(task.reload.status).to eq("pending") + expect(plan.review_requests.where(reviewer: reviewer).count).to eq(1) + expect(plan.plan_events.where(event_type: "review_requested", after_value: task.id).count).to eq(2) + expect(task.notifications.unread.where(user: reviewer, reason: "review_requested").count).to eq(1) + end + end + + it "leaves an already active review unchanged when a selected re-add is submitted", :review_readdition do + CoPlan::Reviews::Withdraw.call(request: task, actor: author) + sign_in_browser(author) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") { click_button "Add reviewer", exact: true } + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + # Another tab restores and completes this request after the person was + # selected. Submitting the stale add form must not reset that decision. + restored = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, requirement: "optional") + approval = CoPlan::Reviews::Respond.call(request: restored, actor: reviewer, revision: 1, decision: "approved") + counts = [ plan.plan_events.where(event_type: "review_requested").count, CoPlan::Notification.count ] + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + within(".review-management[open] [data-review-assignments]") do + expect(page).to have_css(".review-person", text: reviewer.name) + expect(page).to have_css(".review-state", text: "Approved") + expect(page).to have_css(".review-person__requirement", text: "Optional") + end + expect(task.reload).to have_attributes(status: "approved", requirement: "optional") + expect(task.review_responses.pluck(:id)).to eq([ approval.id ]) + expect([ plan.plan_events.where(event_type: "review_requested").count, CoPlan::Notification.count ]).to eq(counts) + end + it "offers equal review choices and preserves cancelled feedback without moving the reader" do within(".review-tray__actions") do expect(page).to have_button("Request changes") From 729a7c6e031889d151b8d21c5f8d0d1200fd1416 Mon Sep 17 00:00:00 2001 From: Sean Prashad <13009507+seanprashad@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:42:19 -0400 Subject: [PATCH 2/5] Start fresh after removing a reviewer --- .../api/v1/review_requests_controller.rb | 4 +- .../coplan/review_requests_controller.rb | 13 +- .../coplan/review_tray_controller.js | 2 +- engine/app/models/coplan/plan_event.rb | 2 +- engine/app/services/coplan/reviews/changed.rb | 10 +- engine/app/services/coplan/reviews/remove.rb | 34 ++++++ engine/app/services/coplan/reviews/request.rb | 13 +- .../agent_instructions/guides/api.text.erb | 2 + .../guides/comments.text.erb | 1 + .../agent_instructions/guides/live.text.erb | 1 + .../views/coplan/plans/_event_item.html.erb | 2 +- .../app/views/coplan/reviews/_roster.html.erb | 2 +- spec/requests/api/v1/review_requests_spec.rb | 31 +++++ spec/requests/review_requests_spec.rb | 40 +++++++ spec/services/coplan/reviews/remove_spec.rb | 112 ++++++++++++++++++ spec/system/plan_reviews_spec.rb | 71 ++++++++--- 16 files changed, 307 insertions(+), 33 deletions(-) create mode 100644 engine/app/services/coplan/reviews/remove.rb create mode 100644 spec/services/coplan/reviews/remove_spec.rb diff --git a/engine/app/controllers/coplan/api/v1/review_requests_controller.rb b/engine/app/controllers/coplan/api/v1/review_requests_controller.rb index c9b9d4da..35b75c4f 100644 --- a/engine/app/controllers/coplan/api/v1/review_requests_controller.rb +++ b/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 diff --git a/engine/app/controllers/coplan/review_requests_controller.rb b/engine/app/controllers/coplan/review_requests_controller.rb index f62be26d..52733b65 100644 --- a/engine/app/controllers/coplan/review_requests_controller.rb +++ b/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 @@ -29,9 +29,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 +40,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 diff --git a/engine/app/javascript/controllers/coplan/review_tray_controller.js b/engine/app/javascript/controllers/coplan/review_tray_controller.js index 10ee02f7..b4c0d9fe 100644 --- a/engine/app/javascript/controllers/coplan/review_tray_controller.js +++ b/engine/app/javascript/controllers/coplan/review_tray_controller.js @@ -362,7 +362,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 diff --git a/engine/app/models/coplan/plan_event.rb b/engine/app/models/coplan/plan_event.rb index da2e3e1e..f728a09f 100644 --- a/engine/app/models/coplan/plan_event.rb +++ b/engine/app/models/coplan/plan_event.rb @@ -9,7 +9,7 @@ module CoPlan # metadata-only changes). # # Records are append-only — never updated, never destroyed except through - # the parent plan's cascade. + # the parent plan's cascade or explicit removal of a reviewer's history. class PlanEvent < ApplicationRecord # Mirrors PlanVersion::ACTOR_TYPES so history items from either source can # be rendered uniformly. diff --git a/engine/app/services/coplan/reviews/changed.rb b/engine/app/services/coplan/reviews/changed.rb index 14d76fe7..089903dd 100644 --- a/engine/app/services/coplan/reviews/changed.rb +++ b/engine/app/services/coplan/reviews/changed.rb @@ -11,8 +11,14 @@ def initialize(request:, event_type:, actor_token_id:, payload:) def call request, event_type, actor_token_id, payload = @request, @event_type, @actor_token_id, @payload - AgentEvents::Publish.call(plan: request.plan, event_type: event_type, actor_token_id: actor_token_id, - payload: { "review_request_id" => request.id, "reviewer_id" => request.reviewer_id }.merge(payload)) + request.plan.with_lock do + # A response can commit just before removal, then reach this fan-out + # afterward. Do not put an erased decision back in an agent's inbox. + if payload["action"] == "removed" || request.plan.review_requests.exists?(request.id) + AgentEvents::Publish.call(plan: request.plan, event_type: event_type, actor_token_id: actor_token_id, + payload: { "review_request_id" => request.id, "reviewer_id" => request.reviewer_id }.merge(payload)) + end + end Notifications::BroadcastBadges.call(user_ids: [ request.reviewer_id, request.plan.created_by_user_id ]) # No viewer-relative HTML or CSRF tokens on a shared stream. Each # open tab fetches its own controls, preserving its current draft. diff --git a/engine/app/services/coplan/reviews/remove.rb b/engine/app/services/coplan/reviews/remove.rb new file mode 100644 index 00000000..83d6bef3 --- /dev/null +++ b/engine/app/services/coplan/reviews/remove.rb @@ -0,0 +1,34 @@ +module CoPlan + module Reviews + # Removing a reviewer ends the request's identity. A later assignment gets + # a new ID, so delayed responses to the old request cannot complete it. + # API withdrawal is separate and continues to preserve review history. + class Remove + def self.call(plan:, request_id:) + removed_events = [] + request = plan.with_lock do + request = plan.review_requests.find_by(id: request_id) + next unless request + + events = plan.plan_events.where(event_type: %w[review_requested review_withdrawn], after_value: request.id) + removed_events = events.to_a + events.delete_all + event_ids = AgentEvent.where(plan: plan, event_type: %w[review.requested review.responded]).pluck(:id, :payload).filter_map do |id, payload| + id if payload.to_h["review_request_id"] == request.id + end + AgentEvent.where(id: event_ids).delete_all + request.destroy! + request + end + if request + removed_events.each { |event| Broadcaster.remove_to(plan, target: ActionView::RecordIdentifier.dom_id(event)) } + count = plan.plan_versions.count + plan.plan_events.count + Broadcaster.replace_to(plan, target: "history-count", + html: ApplicationController.helpers.content_tag(:span, ApplicationController.helpers.pluralize(count, "entry"), class: "section-count", id: "history-count")) + Changed.call(request: request, event_type: "review.requested", payload: { "action" => "removed", "removed" => true }) + end + request + end + end + end +end diff --git a/engine/app/services/coplan/reviews/request.rb b/engine/app/services/coplan/reviews/request.rb index 5e646a28..9e5b971d 100644 --- a/engine/app/services/coplan/reviews/request.rb +++ b/engine/app/services/coplan/reviews/request.rb @@ -1,19 +1,24 @@ 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) + new(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, request_id:).call end - def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:) + def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, request_id:) @plan, @reviewer, @actor, @requirement, @api_token = plan, reviewer, actor, requirement, api_token @create_only = create_only + @request_id = request_id end def call changed = false request = @plan.with_lock do - request = @plan.review_requests.find_or_initialize_by(reviewer: @reviewer) + request = if @request_id + @plan.review_requests.where(reviewer: @reviewer).find(@request_id) + else + @plan.review_requests.find_or_initialize_by(reviewer: @reviewer) + end # Retrying a create cannot change a decision, reopen a withdrawal, # or send another nudge. Only an explicit re-request starts a cycle. if @create_only && request.persisted? diff --git a/engine/app/views/coplan/agent_instructions/guides/api.text.erb b/engine/app/views/coplan/agent_instructions/guides/api.text.erb index d8087c87..24d86ef6 100644 --- a/engine/app/views/coplan/agent_instructions/guides/api.text.erb +++ b/engine/app/views/coplan/agent_instructions/guides/api.text.erb @@ -82,6 +82,8 @@ return 429. Invalid names return 422 with `invalid_request`. Creating a review request for an existing reviewer returns the current task with status 201. It leaves the requirement, decision, history, and notifications unchanged. Use `PATCH` to change the requirement or `request_again: true` to request another look. +Removing a reviewer in the web management pane deletes their request and review decisions. Re-adding them creates a fresh pending request with a new ID. The removed request ID returns 404, including for old response retries. API withdrawal remains separate and preserves history. + Include `idempotency_key` (a fresh UUID) for each intended review response. Reuse that UUID and identical fields when retrying a lost response; its returned `id` is the UUID. Use a new UUID for a new decision, including another review of the same version. A keyed retry returns the original recorded result even after the plan changes or the task is withdrawn. It preserves any newly pending review task. Without a key, an identical response is 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. A key reused for different fields returns 422. A review response returns 409 with `code: review_version_changed` for a stale revision. 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. diff --git a/engine/app/views/coplan/agent_instructions/guides/comments.text.erb b/engine/app/views/coplan/agent_instructions/guides/comments.text.erb index 8ae64f43..1a255732 100644 --- a/engine/app/views/coplan/agent_instructions/guides/comments.text.erb +++ b/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. diff --git a/engine/app/views/coplan/agent_instructions/guides/live.text.erb b/engine/app/views/coplan/agent_instructions/guides/live.text.erb index c682ed2c..7761d69c 100644 --- a/engine/app/views/coplan/agent_instructions/guides/live.text.erb +++ b/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. diff --git a/engine/app/views/coplan/plans/_event_item.html.erb b/engine/app/views/coplan/plans/_event_item.html.erb index a4f75cfe..847e4a73 100644 --- a/engine/app/views/coplan/plans/_event_item.html.erb +++ b/engine/app/views/coplan/plans/_event_item.html.erb @@ -2,7 +2,7 @@ _version_item.html.erb. Non-interactive (no turbo-frame target) since there's no separate diff to display — the event payload itself is the diff. %> -
+
<%= event.field || event.event_type %>
<% if event.actor_user %> diff --git a/engine/app/views/coplan/reviews/_roster.html.erb b/engine/app/views/coplan/reviews/_roster.html.erb index 2092a34b..898fbca8 100644 --- a/engine/app/views/coplan/reviews/_roster.html.erb +++ b/engine/app/views/coplan/reviews/_roster.html.erb @@ -21,7 +21,7 @@ <% if approval && approval.plan_version.revision != plan.current_revision %><% end %> - +
<% elsif approval && approval.plan_version.revision != plan.current_revision %> diff --git a/spec/requests/api/v1/review_requests_spec.rb b/spec/requests/api/v1/review_requests_spec.rb index 3e0a02aa..595bb8b1 100644 --- a/spec/requests/api/v1/review_requests_spec.rb +++ b/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") diff --git a/spec/requests/review_requests_spec.rb b/spec/requests/review_requests_spec.rb index 4a4c9baa..62c88cab 100644 --- a/spec/requests/review_requests_spec.rb +++ b/spec/requests/review_requests_spec.rb @@ -6,6 +6,46 @@ let(:plan) { create(:plan, :published, created_by_user: author) } let(:task) { create(:review_request, plan: plan, reviewer: reviewer) } + [ true, false ].each do |keyed| + it "rejects old #{keyed ? 'keyed' : 'legacy'} responses after removing and re-adding a reviewer", :review_removal do + payload = { revision: 1, decision: "approved" } + payload[:idempotency_key] = SecureRandom.uuid if keyed + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, **payload) + 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) + expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) + post plan_review_requests_path(plan), params: { review_request: { reviewer_id: reviewer.id } }, 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) + + sign_in_as(reviewer) + post respond_plan_review_request_path(plan, task), params: { review_response: payload }, as: :json + expect(response).to have_http_status(:not_found) + sign_in_as(author) + patch plan_review_request_path(plan, task), params: { review_request: { request_again: true } }, as: :json + expect(response).to have_http_status(:not_found) + delete plan_review_request_path(plan, task), params: { review_request: { remove: true } }, as: :json + expect(response).to have_http_status(:no_content) + expect(fresh.reload.status).to eq("pending") + expect(fresh.review_responses).to be_empty + get plan_review_requests_path(plan) + expect(Nokogiri::HTML(response.body).css(".review-log__event").map { |node| node.text.squish }).to eq([ + "#{author.name} requested a review from #{reviewer.name}" + ]) + end + end + + 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") diff --git a/spec/services/coplan/reviews/remove_spec.rb b/spec/services/coplan/reviews/remove_spec.rb new file mode 100644 index 00000000..ced77aee --- /dev/null +++ b/spec/services/coplan/reviews/remove_spec.rb @@ -0,0 +1,112 @@ +require "rails_helper" + +RSpec.describe CoPlan::Reviews::Remove do + let(:plan) { create(:plan, :published) } + let(:reviewer) { create(:coplan_user) } + let(:task) { CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user) } + + %w[approved changes_requested].each do |decision| + it "erases a removed reviewer's #{decision} decisions and starts a new pending request" do + watcher = create(:api_token, user: plan.created_by_user) + CoPlan::AgentSession.create!(plan: plan, api_token: watcher, state: "watching", agent_name: "Authorbot") + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, + decision: decision, note: decision == "changes_requested" ? "Earlier feedback" : nil) + other = CoPlan::Reviews::Request.call(plan: plan, reviewer: create(:coplan_user), actor: plan.created_by_user) + other_response = CoPlan::Reviews::Respond.call(request: other, actor: other.reviewer, revision: 1, decision: "approved") + + described_class.call(plan: plan, request_id: task.id) + + expect(CoPlan::ReviewRequest.exists?(task.id)).to be(false) + expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) + expect(plan.notifications.where(review_request_id: task.id)).to be_empty + expect(plan.plan_events.where(after_value: task.id)).to be_empty + expect(watcher.agent_events.where(event_type: "review.responded").map { |event| event.payload["review_request_id"] }).to eq([ other.id ]) + expect(watcher.agent_events.last.payload).to include("action" => "removed", "review_request_id" => task.id) + expect(other_response.reload.decision).to eq("approved") + + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, create_only: true) + expect(fresh.id).not_to eq(task.id) + expect(fresh.status).to eq("pending") + expect(fresh.review_responses).to be_empty + expect(fresh.latest_approval).to be_nil + end + end + + it "does not remove or reset the replacement when an old removal is retried" do + old_id = task.id + described_class.call(plan: plan, request_id: old_id) + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, create_only: true) + approval = CoPlan::Reviews::Respond.call(request: fresh, actor: reviewer, revision: 1, decision: "approved") + expect { described_class.call(plan: plan, request_id: old_id) }.not_to change(CoPlan::Notification, :count) + expect(fresh.reload.status).to eq("approved") + expect(fresh.review_responses).to eq([ approval ]) + end + + it "rejects an old re-request already in flight when the request is removed" do + old_id = task.id + described_class.call(plan: plan, request_id: old_id) + expect { + CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, request_id: old_id) + }.to raise_error(ActiveRecord::RecordNotFound) + expect(plan.review_requests).to be_empty + end + + it "does not erase a request from another plan" do + task + expect(described_class.call(plan: create(:plan, :published), request_id: task.id)).to be_nil + expect(task.reload.status).to eq("pending") + end + + it "does not republish an erased response when its post-commit fan-out runs late" do + watcher = create(:api_token, user: plan.created_by_user) + CoPlan::AgentSession.create!(plan: plan, api_token: watcher, state: "watching", agent_name: "Authorbot") + stale_request = task + described_class.call(plan: plan, request_id: task.id) + CoPlan::Reviews::Changed.call(request: stale_request, event_type: "review.responded", payload: { "decision" => "approved" }) + expect(watcher.agent_events.where(event_type: "review.responded")).to be_empty + end + + describe "requests already waiting for the plan lock" do + self.use_transactional_tests = false + after { truncate_tables(*(ActiveRecord::Base.connection.tables - %w[schema_migrations ar_internal_metadata])) } + + %i[respond request_again].each do |action| + it "rejects an old #{action} after removal and reassignment commit" do + old_id = task.id + reviewer_id = reviewer.id + ready = Queue.new + worker = nil + fresh = nil + plan.with_lock do + worker = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + stale = CoPlan::ReviewRequest.find(old_id) + ready << true + begin + if action == :respond + CoPlan::Reviews::Respond.call(request: stale, actor: stale.reviewer, revision: 1, decision: "approved") + else + CoPlan::Reviews::Request.call(plan: stale.plan, reviewer: stale.reviewer, + actor: stale.requested_by_user, request_id: old_id) + end + rescue ActiveRecord::RecordNotFound => error + error + end + end + end + Timeout.timeout(10) { ready.pop } + described_class.call(plan: plan, request_id: old_id) + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: CoPlan::User.find(reviewer_id), + actor: plan.created_by_user, create_only: true) + end + expect(worker.join(10)).to be_present + expect(worker.value).to be_a(ActiveRecord::RecordNotFound) + expect(fresh.reload.status).to eq("pending") + expect(fresh.review_responses).to be_empty + expect(plan.review_requests.pluck(:id)).to eq([ fresh.id ]) + ensure + worker&.kill if worker&.alive? + end + end + end +end diff --git a/spec/system/plan_reviews_spec.rb b/spec/system/plan_reviews_spec.rb index 4feeaf46..8c296630 100644 --- a/spec/system/plan_reviews_spec.rb +++ b/spec/system/plan_reviews_spec.rb @@ -40,15 +40,9 @@ def lose_next_review_result before { sign_in_browser(reviewer, open_sections: !RSpec.current_example.metadata[:collapsed_support]) } - it "keeps a withdrawn request between its two request cycles", :review_withdrawals do + it "keeps an API-withdrawn request between its two request cycles", :review_withdrawals do sign_in_browser(author) - click_button "More actions" - within("#plan-menu") { click_button "Manage reviewers" } - within(".review-management[open]") do - click_button "Actions for Plan Reviewer", enable_aria_label: true - click_button "Remove request" - expect(page).to have_no_css("[data-review-assignments] .review-person") - end + CoPlan::Reviews::Withdraw.call(request: task, actor: author) expect(task.reload.status).to eq("withdrawn") CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author) visit plan_page_path(plan) @@ -66,20 +60,22 @@ def lose_next_review_result end end - [ "light", "dark" ].each do |theme| - it "re-adds a removed reviewer through the assignment pane in #{theme} theme", :review_readdition do - approval = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved") + [ "light", "dark" ].product(%w[approved changes_requested]).each do |theme, decision| + it "starts fresh after removing a #{decision} reviewer in #{theme} theme", :review_readdition do + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: decision, + note: decision == "changes_requested" ? "Earlier review feedback" : nil) sign_in_browser(author) page.execute_script("document.documentElement.dataset.theme = arguments[0]", theme) click_button "More actions" within("#plan-menu") { click_button "Manage reviewers" } within(".review-management[open]") do click_button "Actions for Plan Reviewer", enable_aria_label: true - click_button "Remove request" + click_button "Remove reviewer" expect(page).to have_no_css("[data-review-assignments] .review-person") click_button "Add reviewer", exact: true end - expect(task.reload.status).to eq("withdrawn") + expect(CoPlan::ReviewRequest.exists?(task.id)).to be(false) + expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) fill_in "Find a reviewer", with: reviewer.name find(".review-search-result", text: reviewer.name).click uncheck "Required" @@ -89,17 +85,58 @@ def lose_next_review_result expect(page).to have_css(".review-person__requirement", text: "Optional") expect(page).to have_css(".review-state", text: "Pending") end - expect(task.reload).to have_attributes(status: "pending", requirement: "optional") + fresh = plan.review_requests.find_by!(reviewer: reviewer) + expect(fresh.id).not_to eq(task.id) + expect(fresh).to have_attributes(status: "pending", requirement: "optional") expect(plan.review_requests.where(reviewer: reviewer).count).to eq(1) - expect(task.review_responses.pluck(:id)).to eq([ approval.id ]) - expect(task.notifications.unread.where(user: reviewer, reason: "review_requested").count).to eq(1) + expect(fresh.review_responses).to be_empty + expect(fresh.notifications.unread.where(user: reviewer, reason: "review_requested").count).to eq(1) page.save_screenshot(Rails.root.join("tmp/review-preview/readded-reviewer-#{theme}.png")) + within(".review-management[open]") { click_button "Close reviews", enable_aria_label: true } + within("#footnote-approvals") { click_button "View activity", exact: false } + within(".review-activity-dialog[open]") do + expect(all(".review-log__event").map { |entry| entry.text.squish }).to eq([ "Plan Author requested a review from Plan Reviewer" ]) + expect(page).to have_no_text("Earlier review feedback") + end + page.save_screenshot(Rails.root.join("tmp/review-preview/fresh-review-history-#{theme}.png")) sign_in_browser(reviewer) expect(page).to have_css(".review-tray", text: "Your review is requested") end end [ "lost", "rejected" ].each do |failure| + it "keeps removal retryable after a #{failure} result", :review_readdition do + sign_in_browser(author) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + page.execute_script(<<~JS, failure) + const originalFetch = window.fetch.bind(window) + const failure = arguments[0] + // Also lose the socket invalidation so the stale row can be retried. + window.Turbo.StreamActions['coplan-review-changed'] = function () {} + let interrupt = true + window.fetch = async (url, options) => { + if (interrupt && options?.method === 'DELETE' && String(url).includes('/review_requests/')) { + interrupt = false + if (failure === 'rejected') return new Response(JSON.stringify({error: 'Please retry removal'}), {status: 422, headers: {'Content-Type': 'application/json'}}) + await originalFetch(url, options) + throw new TypeError('Lost the successful removal result') + } + return originalFetch(url, options) + } + JS + within(".review-management[open]") do + click_button "Actions for Plan Reviewer", enable_aria_label: true + click_button "Remove reviewer" + expect(page).to have_css("[role='alert']", text: failure == "lost" ? "Couldn't reach CoPlan" : "Please retry removal") + expect(CoPlan::ReviewRequest.exists?(task.id)).to eq(failure == "rejected") + click_button "Actions for Plan Reviewer", enable_aria_label: true + click_button "Remove reviewer" + expect(page).to have_no_css("[data-review-assignments] .review-person") + end + expect(CoPlan::ReviewRequest.exists?(task.id)).to be(false) + end + it "keeps re-adding a reviewer retryable after a #{failure} update result", :review_readdition do CoPlan::Reviews::Withdraw.call(request: task, actor: author) sign_in_browser(author) @@ -188,7 +225,7 @@ def lose_next_review_result expect(page).to have_field("Public review comment", with: "Can we clarify this boundary?", enable_aria_label: true) end - [ [ "Request another review", false ], [ "Remove request", true ] ].each do |action, failed| + [ [ "Request another review", false ], [ "Remove reviewer", true ] ].each do |action, failed| it "keeps reviewer-update progress visible for #{action}", :review_interactions do sign_in_browser(author) click_button "More actions" From d46388f0f73514d1f038568dbce0054c4524cb36 Mon Sep 17 00:00:00 2001 From: Sean Prashad <13009507+seanprashad@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:53:28 -0400 Subject: [PATCH 3/5] Limit resets to explicit reviewer reassignment --- .../coplan/review_requests_controller.rb | 5 +- .../coplan/review_tray_controller.js | 10 +-- engine/app/models/coplan/plan_event.rb | 2 +- engine/app/services/coplan/reviews/changed.rb | 10 +-- engine/app/services/coplan/reviews/remove.rb | 25 +----- engine/app/services/coplan/reviews/request.rb | 16 ++-- .../agent_instructions/guides/api.text.erb | 2 - .../views/coplan/plans/_event_item.html.erb | 2 +- spec/requests/review_requests_spec.rb | 31 ------- spec/services/coplan/reviews/remove_spec.rb | 34 ++------ spec/system/plan_reviews_spec.rb | 87 ++++--------------- 11 files changed, 47 insertions(+), 177 deletions(-) diff --git a/engine/app/controllers/coplan/review_requests_controller.rb b/engine/app/controllers/coplan/review_requests_controller.rb index 52733b65..4417b0f4 100644 --- a/engine/app/controllers/coplan/review_requests_controller.rb +++ b/engine/app/controllers/coplan/review_requests_controller.rb @@ -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 diff --git a/engine/app/javascript/controllers/coplan/review_tray_controller.js b/engine/app/javascript/controllers/coplan/review_tray_controller.js index b4c0d9fe..4adabcdf 100644 --- a/engine/app/javascript/controllers/coplan/review_tray_controller.js +++ b/engine/app/javascript/controllers/coplan/review_tray_controller.js @@ -267,15 +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 () => { - const requirement = event.target.querySelector('[type="checkbox"]').checked ? "required" : "optional" - const request = await this.write(this.urlValue, "POST", { review_request: { reviewer_id: this.reviewerIdTarget.value, requirement } }) - // Create retries preserve withdrawn tasks. Re-adding someone here is - // an explicit new request; reopen it through the existing update API. - // If that result is lost, the next create returns the pending task - // and does not send another request or notification. - if (request.status === "withdrawn") { - await this.write(`${this.urlValue}/${encodeURIComponent(request.id)}`, "PATCH", { review_request: { request_again: true, requirement } }) - } + 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 diff --git a/engine/app/models/coplan/plan_event.rb b/engine/app/models/coplan/plan_event.rb index f728a09f..da2e3e1e 100644 --- a/engine/app/models/coplan/plan_event.rb +++ b/engine/app/models/coplan/plan_event.rb @@ -9,7 +9,7 @@ module CoPlan # metadata-only changes). # # Records are append-only — never updated, never destroyed except through - # the parent plan's cascade or explicit removal of a reviewer's history. + # the parent plan's cascade. class PlanEvent < ApplicationRecord # Mirrors PlanVersion::ACTOR_TYPES so history items from either source can # be rendered uniformly. diff --git a/engine/app/services/coplan/reviews/changed.rb b/engine/app/services/coplan/reviews/changed.rb index 089903dd..14d76fe7 100644 --- a/engine/app/services/coplan/reviews/changed.rb +++ b/engine/app/services/coplan/reviews/changed.rb @@ -11,14 +11,8 @@ def initialize(request:, event_type:, actor_token_id:, payload:) def call request, event_type, actor_token_id, payload = @request, @event_type, @actor_token_id, @payload - request.plan.with_lock do - # A response can commit just before removal, then reach this fan-out - # afterward. Do not put an erased decision back in an agent's inbox. - if payload["action"] == "removed" || request.plan.review_requests.exists?(request.id) - AgentEvents::Publish.call(plan: request.plan, event_type: event_type, actor_token_id: actor_token_id, - payload: { "review_request_id" => request.id, "reviewer_id" => request.reviewer_id }.merge(payload)) - end - end + AgentEvents::Publish.call(plan: request.plan, event_type: event_type, actor_token_id: actor_token_id, + payload: { "review_request_id" => request.id, "reviewer_id" => request.reviewer_id }.merge(payload)) Notifications::BroadcastBadges.call(user_ids: [ request.reviewer_id, request.plan.created_by_user_id ]) # No viewer-relative HTML or CSRF tokens on a shared stream. Each # open tab fetches its own controls, preserving its current draft. diff --git a/engine/app/services/coplan/reviews/remove.rb b/engine/app/services/coplan/reviews/remove.rb index 83d6bef3..0fc88e3f 100644 --- a/engine/app/services/coplan/reviews/remove.rb +++ b/engine/app/services/coplan/reviews/remove.rb @@ -1,32 +1,15 @@ module CoPlan module Reviews - # Removing a reviewer ends the request's identity. A later assignment gets - # a new ID, so delayed responses to the old request cannot complete it. - # API withdrawal is separate and continues to preserve review history. + # A later assignment gets a fresh ID, so old response URLs stay invalid. class Remove def self.call(plan:, request_id:) - removed_events = [] request = plan.with_lock do request = plan.review_requests.find_by(id: request_id) - next unless request - - events = plan.plan_events.where(event_type: %w[review_requested review_withdrawn], after_value: request.id) - removed_events = events.to_a - events.delete_all - event_ids = AgentEvent.where(plan: plan, event_type: %w[review.requested review.responded]).pluck(:id, :payload).filter_map do |id, payload| - id if payload.to_h["review_request_id"] == request.id - end - AgentEvent.where(id: event_ids).delete_all - request.destroy! + request&.destroy! request end - if request - removed_events.each { |event| Broadcaster.remove_to(plan, target: ActionView::RecordIdentifier.dom_id(event)) } - count = plan.plan_versions.count + plan.plan_events.count - Broadcaster.replace_to(plan, target: "history-count", - html: ApplicationController.helpers.content_tag(:span, ApplicationController.helpers.pluralize(count, "entry"), class: "section-count", id: "history-count")) - Changed.call(request: request, event_type: "review.requested", payload: { "action" => "removed", "removed" => true }) - end + Changed.call(request: request, event_type: "review.requested", + payload: { "action" => "removed", "removed" => true }) if request request end end diff --git a/engine/app/services/coplan/reviews/request.rb b/engine/app/services/coplan/reviews/request.rb index 9e5b971d..f777fbd3 100644 --- a/engine/app/services/coplan/reviews/request.rb +++ b/engine/app/services/coplan/reviews/request.rb @@ -1,14 +1,15 @@ module CoPlan module Reviews class Request - def self.call(plan:, reviewer:, actor:, requirement: "required", api_token: nil, create_only: false, request_id: nil) - new(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, request_id:).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:, request_id:) + 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 @@ -19,14 +20,17 @@ def call else @plan.review_requests.find_or_initialize_by(reviewer: @reviewer) end - # Retrying a create cannot change a decision, reopen a withdrawal, - # or send another nudge. Only an explicit re-request starts a cycle. + # 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" + + request.destroy! + request = @plan.review_requests.new(reviewer: @reviewer) end # Preserve the original request for tasks created before request diff --git a/engine/app/views/coplan/agent_instructions/guides/api.text.erb b/engine/app/views/coplan/agent_instructions/guides/api.text.erb index 24d86ef6..d8087c87 100644 --- a/engine/app/views/coplan/agent_instructions/guides/api.text.erb +++ b/engine/app/views/coplan/agent_instructions/guides/api.text.erb @@ -82,8 +82,6 @@ return 429. Invalid names return 422 with `invalid_request`. Creating a review request for an existing reviewer returns the current task with status 201. It leaves the requirement, decision, history, and notifications unchanged. Use `PATCH` to change the requirement or `request_again: true` to request another look. -Removing a reviewer in the web management pane deletes their request and review decisions. Re-adding them creates a fresh pending request with a new ID. The removed request ID returns 404, including for old response retries. API withdrawal remains separate and preserves history. - Include `idempotency_key` (a fresh UUID) for each intended review response. Reuse that UUID and identical fields when retrying a lost response; its returned `id` is the UUID. Use a new UUID for a new decision, including another review of the same version. A keyed retry returns the original recorded result even after the plan changes or the task is withdrawn. It preserves any newly pending review task. Without a key, an identical response is 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. A key reused for different fields returns 422. A review response returns 409 with `code: review_version_changed` for a stale revision. 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. diff --git a/engine/app/views/coplan/plans/_event_item.html.erb b/engine/app/views/coplan/plans/_event_item.html.erb index 847e4a73..a4f75cfe 100644 --- a/engine/app/views/coplan/plans/_event_item.html.erb +++ b/engine/app/views/coplan/plans/_event_item.html.erb @@ -2,7 +2,7 @@ _version_item.html.erb. Non-interactive (no turbo-frame target) since there's no separate diff to display — the event payload itself is the diff. %> -
+
<%= event.field || event.event_type %>
<% if event.actor_user %> diff --git a/spec/requests/review_requests_spec.rb b/spec/requests/review_requests_spec.rb index 62c88cab..6ac79639 100644 --- a/spec/requests/review_requests_spec.rb +++ b/spec/requests/review_requests_spec.rb @@ -6,37 +6,6 @@ let(:plan) { create(:plan, :published, created_by_user: author) } let(:task) { create(:review_request, plan: plan, reviewer: reviewer) } - [ true, false ].each do |keyed| - it "rejects old #{keyed ? 'keyed' : 'legacy'} responses after removing and re-adding a reviewer", :review_removal do - payload = { revision: 1, decision: "approved" } - payload[:idempotency_key] = SecureRandom.uuid if keyed - previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, **payload) - 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) - expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) - post plan_review_requests_path(plan), params: { review_request: { reviewer_id: reviewer.id } }, 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) - - sign_in_as(reviewer) - post respond_plan_review_request_path(plan, task), params: { review_response: payload }, as: :json - expect(response).to have_http_status(:not_found) - sign_in_as(author) - patch plan_review_request_path(plan, task), params: { review_request: { request_again: true } }, as: :json - expect(response).to have_http_status(:not_found) - delete plan_review_request_path(plan, task), params: { review_request: { remove: true } }, as: :json - expect(response).to have_http_status(:no_content) - expect(fresh.reload.status).to eq("pending") - expect(fresh.review_responses).to be_empty - get plan_review_requests_path(plan) - expect(Nokogiri::HTML(response.body).css(".review-log__event").map { |node| node.text.squish }).to eq([ - "#{author.name} requested a review from #{reviewer.name}" - ]) - end - end - 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) diff --git a/spec/services/coplan/reviews/remove_spec.rb b/spec/services/coplan/reviews/remove_spec.rb index ced77aee..7c4678e7 100644 --- a/spec/services/coplan/reviews/remove_spec.rb +++ b/spec/services/coplan/reviews/remove_spec.rb @@ -7,8 +7,6 @@ %w[approved changes_requested].each do |decision| it "erases a removed reviewer's #{decision} decisions and starts a new pending request" do - watcher = create(:api_token, user: plan.created_by_user) - CoPlan::AgentSession.create!(plan: plan, api_token: watcher, state: "watching", agent_name: "Authorbot") previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: decision, note: decision == "changes_requested" ? "Earlier feedback" : nil) other = CoPlan::Reviews::Request.call(plan: plan, reviewer: create(:coplan_user), actor: plan.created_by_user) @@ -19,9 +17,6 @@ expect(CoPlan::ReviewRequest.exists?(task.id)).to be(false) expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) expect(plan.notifications.where(review_request_id: task.id)).to be_empty - expect(plan.plan_events.where(after_value: task.id)).to be_empty - expect(watcher.agent_events.where(event_type: "review.responded").map { |event| event.payload["review_request_id"] }).to eq([ other.id ]) - expect(watcher.agent_events.last.payload).to include("action" => "removed", "review_request_id" => task.id) expect(other_response.reload.decision).to eq("approved") fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, create_only: true) @@ -42,28 +37,15 @@ expect(fresh.review_responses).to eq([ approval ]) end - it "rejects an old re-request already in flight when the request is removed" do - old_id = task.id - described_class.call(plan: plan, request_id: old_id) + it "keeps the old review if a replacement request is invalid" do + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved") + CoPlan::Reviews::Withdraw.call(request: task, actor: plan.created_by_user) expect { - CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, request_id: old_id) - }.to raise_error(ActiveRecord::RecordNotFound) - expect(plan.review_requests).to be_empty - end - - it "does not erase a request from another plan" do - task - expect(described_class.call(plan: create(:plan, :published), request_id: task.id)).to be_nil - expect(task.reload.status).to eq("pending") - end - - it "does not republish an erased response when its post-commit fan-out runs late" do - watcher = create(:api_token, user: plan.created_by_user) - CoPlan::AgentSession.create!(plan: plan, api_token: watcher, state: "watching", agent_name: "Authorbot") - stale_request = task - described_class.call(plan: plan, request_id: task.id) - CoPlan::Reviews::Changed.call(request: stale_request, event_type: "review.responded", payload: { "decision" => "approved" }) - expect(watcher.agent_events.where(event_type: "review.responded")).to be_empty + CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, + requirement: "invalid", create_only: true, replace_withdrawn: true) + }.to raise_error(ActiveRecord::RecordInvalid) + expect(previous.reload.decision).to eq("approved") + expect(task.reload.status).to eq("withdrawn") end describe "requests already waiting for the plan lock" do diff --git a/spec/system/plan_reviews_spec.rb b/spec/system/plan_reviews_spec.rb index 8c296630..20d5f0a8 100644 --- a/spec/system/plan_reviews_spec.rb +++ b/spec/system/plan_reviews_spec.rb @@ -60,7 +60,7 @@ def lose_next_review_result end end - [ "light", "dark" ].product(%w[approved changes_requested]).each do |theme, decision| + [ [ "light", "approved" ], [ "dark", "changes_requested" ] ].each do |theme, decision| it "starts fresh after removing a #{decision} reviewer in #{theme} theme", :review_readdition do previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: decision, note: decision == "changes_requested" ? "Earlier review feedback" : nil) @@ -104,75 +104,22 @@ def lose_next_review_result end end - [ "lost", "rejected" ].each do |failure| - it "keeps removal retryable after a #{failure} result", :review_readdition do - sign_in_browser(author) - click_button "More actions" - within("#plan-menu") { click_button "Manage reviewers" } - page.execute_script(<<~JS, failure) - const originalFetch = window.fetch.bind(window) - const failure = arguments[0] - // Also lose the socket invalidation so the stale row can be retried. - window.Turbo.StreamActions['coplan-review-changed'] = function () {} - let interrupt = true - window.fetch = async (url, options) => { - if (interrupt && options?.method === 'DELETE' && String(url).includes('/review_requests/')) { - interrupt = false - if (failure === 'rejected') return new Response(JSON.stringify({error: 'Please retry removal'}), {status: 422, headers: {'Content-Type': 'application/json'}}) - await originalFetch(url, options) - throw new TypeError('Lost the successful removal result') - } - return originalFetch(url, options) - } - JS - within(".review-management[open]") do - click_button "Actions for Plan Reviewer", enable_aria_label: true - click_button "Remove reviewer" - expect(page).to have_css("[role='alert']", text: failure == "lost" ? "Couldn't reach CoPlan" : "Please retry removal") - expect(CoPlan::ReviewRequest.exists?(task.id)).to eq(failure == "rejected") - click_button "Actions for Plan Reviewer", enable_aria_label: true - click_button "Remove reviewer" - expect(page).to have_no_css("[data-review-assignments] .review-person") - end - expect(CoPlan::ReviewRequest.exists?(task.id)).to be(false) - end - - it "keeps re-adding a reviewer retryable after a #{failure} update result", :review_readdition do - CoPlan::Reviews::Withdraw.call(request: task, actor: author) - sign_in_browser(author) - click_button "More actions" - within("#plan-menu") { click_button "Manage reviewers" } - within(".review-management[open]") { click_button "Add reviewer", exact: true } - fill_in "Find a reviewer", with: reviewer.name - find(".review-search-result", text: reviewer.name).click - page.execute_script(<<~JS, failure) - const originalFetch = window.fetch.bind(window) - let interrupt = true - const failure = arguments[0] - window.fetch = async (url, options) => { - if (interrupt && options?.method === 'PATCH' && String(url).includes('/review_requests/')) { - interrupt = false - if (failure === 'rejected') return new Response(JSON.stringify({error: 'Please retry this request'}), {status: 422, headers: {'Content-Type': 'application/json'}}) - await originalFetch(url, options) - throw new TypeError('Lost the result after the server reopened the task') - } - return originalFetch(url, options) - } - JS - within(".review-assignment-form") { click_button "Add reviewer", exact: true } - expect(page).to have_css(".review-assignment [role='alert']", text: failure == "lost" ? "Couldn't reach CoPlan" : "Please retry this request") - expect(page).to have_field("Find a reviewer", with: reviewer.name) - expect(task.reload.status).to eq(failure == "lost" ? "pending" : "withdrawn") - within(".review-assignment-form") { click_button "Add reviewer", exact: true } - within(".review-management[open] [data-review-assignments]") do - expect(page).to have_css(".review-person", text: reviewer.name) - expect(page).to have_css(".review-state", text: "Pending") - end - expect(task.reload.status).to eq("pending") - expect(plan.review_requests.where(reviewer: reviewer).count).to eq(1) - expect(plan.plan_events.where(event_type: "review_requested", after_value: task.id).count).to eq(2) - expect(task.notifications.unread.where(user: reviewer, reason: "review_requested").count).to eq(1) - end + it "re-adds a reviewer removed before permanent removal was supported", :review_readdition do + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved") + CoPlan::Reviews::Withdraw.call(request: task, actor: author) + sign_in_browser(author) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") { click_button "Add reviewer", exact: true } + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + expect(page).to have_css(".review-person", text: reviewer.name) + expect(page).to have_css(".review-state", text: "Pending") + fresh = plan.review_requests.find_by!(reviewer: reviewer) + expect(fresh.id).not_to eq(task.id) + expect(fresh.review_responses).to be_empty + expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) end it "leaves an already active review unchanged when a selected re-add is submitted", :review_readdition do From f46f097117b6e364499c6ed55aa9ae4beacd35e8 Mon Sep 17 00:00:00 2001 From: Sean Prashad <13009507+seanprashad@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:33:27 -0400 Subject: [PATCH 4/5] Exercise reviewer removal with real request history Signed-off-by: Sean Prashad <13009507+seanprashad@users.noreply.github.com> --- spec/system/plan_reviews_spec.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/spec/system/plan_reviews_spec.rb b/spec/system/plan_reviews_spec.rb index 20d5f0a8..dcfbbb05 100644 --- a/spec/system/plan_reviews_spec.rb +++ b/spec/system/plan_reviews_spec.rb @@ -62,7 +62,9 @@ def lose_next_review_result [ [ "light", "approved" ], [ "dark", "changes_requested" ] ].each do |theme, decision| it "starts fresh after removing a #{decision} reviewer in #{theme} theme", :review_readdition do - previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: decision, + task.destroy! + requested = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author) + previous = CoPlan::Reviews::Respond.call(request: requested, actor: reviewer, revision: 1, decision: decision, note: decision == "changes_requested" ? "Earlier review feedback" : nil) sign_in_browser(author) page.execute_script("document.documentElement.dataset.theme = arguments[0]", theme) @@ -74,7 +76,7 @@ def lose_next_review_result expect(page).to have_no_css("[data-review-assignments] .review-person") click_button "Add reviewer", exact: true end - expect(CoPlan::ReviewRequest.exists?(task.id)).to be(false) + expect(CoPlan::ReviewRequest.exists?(requested.id)).to be(false) expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) fill_in "Find a reviewer", with: reviewer.name find(".review-search-result", text: reviewer.name).click @@ -86,7 +88,7 @@ def lose_next_review_result expect(page).to have_css(".review-state", text: "Pending") end fresh = plan.review_requests.find_by!(reviewer: reviewer) - expect(fresh.id).not_to eq(task.id) + expect(fresh.id).not_to eq(requested.id) expect(fresh).to have_attributes(status: "pending", requirement: "optional") expect(plan.review_requests.where(reviewer: reviewer).count).to eq(1) expect(fresh.review_responses).to be_empty @@ -95,8 +97,9 @@ def lose_next_review_result within(".review-management[open]") { click_button "Close reviews", enable_aria_label: true } within("#footnote-approvals") { click_button "View activity", exact: false } within(".review-activity-dialog[open]") do - expect(all(".review-log__event").map { |entry| entry.text.squish }).to eq([ "Plan Author requested a review from Plan Reviewer" ]) + expect(all(".review-log__event").map { |entry| entry.text.squish }).to eq([ "Plan Author requested a review from Plan Reviewer" ] * 2) expect(page).to have_no_text("Earlier review feedback") + expect(plan.plan_events.where(event_type: "review_requested").pluck(:after_value)).to contain_exactly(requested.id, fresh.id) end page.save_screenshot(Rails.root.join("tmp/review-preview/fresh-review-history-#{theme}.png")) sign_in_browser(reviewer) From b235f4fe811107dbe5f728ca5940fd105d84ddec Mon Sep 17 00:00:00 2001 From: Sean Prashad <13009507+seanprashad@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:44:09 -0400 Subject: [PATCH 5/5] Preserve legacy request audit and removal events Signed-off-by: Sean Prashad <13009507+seanprashad@users.noreply.github.com> --- engine/app/services/coplan/reviews/remove.rb | 7 +++++- engine/app/services/coplan/reviews/request.rb | 7 ++++++ spec/services/coplan/reviews/remove_spec.rb | 24 +++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/engine/app/services/coplan/reviews/remove.rb b/engine/app/services/coplan/reviews/remove.rb index 0fc88e3f..e269863b 100644 --- a/engine/app/services/coplan/reviews/remove.rb +++ b/engine/app/services/coplan/reviews/remove.rb @@ -5,7 +5,12 @@ class Remove def self.call(plan:, request_id:) request = plan.with_lock do request = plan.review_requests.find_by(id: request_id) - request&.destroy! + 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", diff --git a/engine/app/services/coplan/reviews/request.rb b/engine/app/services/coplan/reviews/request.rb index f777fbd3..5f028d60 100644 --- a/engine/app/services/coplan/reviews/request.rb +++ b/engine/app/services/coplan/reviews/request.rb @@ -14,6 +14,7 @@ def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, def call changed = false + removed = nil request = @plan.with_lock do request = if @request_id @plan.review_requests.where(reviewer: @reviewer).find(@request_id) @@ -29,7 +30,11 @@ def call end 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 @@ -48,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 diff --git a/spec/services/coplan/reviews/remove_spec.rb b/spec/services/coplan/reviews/remove_spec.rb index 7c4678e7..201b9d9d 100644 --- a/spec/services/coplan/reviews/remove_spec.rb +++ b/spec/services/coplan/reviews/remove_spec.rb @@ -37,6 +37,30 @@ expect(fresh.review_responses).to eq([ approval ]) end + it "preserves a legacy request's audit before deleting the task" do + legacy = create(:review_request, plan: plan, reviewer: reviewer) + expect(plan.plan_events.where(event_type: "review_requested")).to be_empty + described_class.call(plan: plan, request_id: legacy.id) + event = plan.plan_events.find_by!(event_type: "review_requested", after_value: legacy.id) + expect(event).to have_attributes(actor_id: legacy.requested_by_user_id, created_at: legacy.requested_at) + expect(event.metadata).to include("reviewer_name" => reviewer.name, "revision" => 1) + expect(CoPlan::ReviewRequest.exists?(legacy.id)).to be(false) + end + + it "announces the old identity's removal when replacing a withdrawn legacy request" do + legacy = create(:review_request, plan: plan, reviewer: reviewer, status: "withdrawn") + watcher = create(:api_token, user: reviewer) + CoPlan::AgentSession.create!(plan: plan, api_token: watcher, state: "watching", agent_name: "Watcher") + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, + create_only: true, replace_withdrawn: true) + payloads = watcher.agent_events.where(event_type: "review.requested").order(:created_at).map(&:payload) + expect(payloads.size).to eq(2) + expect(payloads.first).to include("action" => "removed", "removed" => true, "review_request_id" => legacy.id) + expect(payloads.last).to include("action" => "requested", "review_request_id" => fresh.id) + expect(plan.plan_events.where(event_type: "review_requested").pluck(:after_value)).to contain_exactly(legacy.id, fresh.id) + expect(fresh.status).to eq("pending") + end + it "keeps the old review if a replacement request is invalid" do previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved") CoPlan::Reviews::Withdraw.call(request: task, actor: plan.created_by_user)