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. %> -