From 2a5d702c097396fdd0e6e18658425ceca7c72c48 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Wed, 29 Apr 2026 14:32:53 -0400 Subject: [PATCH 1/2] Add PUT /api/v1/plans/:id/content for full content replacement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent-friendly edit path: read the plan, edit markdown locally, PUT it back. Server diffs against current revision via Diff::LCS, decomposes into replace_exact ops (preserving comment anchors in unchanged regions via existing OT), and creates a new immutable PlanVersion. - Plans::DiffToOperations — line-level LCS diff to ordered ops with _pre_resolved_ranges and cumulative-delta-aware positions - Plans::ReplaceContent — orchestrator: locks plan, validates base_revision, normalizes CRLF to LF, applies ops, creates version, marks anchors out-of-date - Api::V1::ContentController — thin controller, optimistic concurrency via 409 - Disable wrap_parameters on API base controller (was silently nesting body under params[:content]) - Allow empty old_text in apply_replace_exact when _pre_resolved_ranges is set - Fix delta calculation to use range slice (not old_text.length) so mismatched caller-supplied old_text can't corrupt OT metadata - Update agent-instructions doc to feature this as the primary edit path Diff service is line-level for two reasons: anchors in unchanged regions survive intact via Plans::TransformRange, and operations_json stays compact. Tests cover: roundtrip property (incl. 30-iteration fuzz), single/multi-hunk shapes, append/insert/delete at every position, trailing-newline variants, unicode, very long single-line content, repeated identical lines (LCS ambiguity), shared prefix/suffix, contiguous delete-then-insert, large files with disjoint hunks, CRLF normalization, RoundtripFailureError, stale revision 409, anchor preservation (before/inside/after change), and a delta-calculation regression for the operations API surface. Amp-Thread-ID: https://ampcode.com/threads/T-019dda2a-85b2-706b-b5f8-67601e9dcf58 Co-authored-by: Amp --- .../coplan/api/v1/base_controller.rb | 7 + .../coplan/api/v1/content_controller.rb | 65 +++ .../services/coplan/plans/apply_operations.rb | 25 +- .../coplan/plans/diff_to_operations.rb | 147 +++++++ .../services/coplan/plans/replace_content.rb | 130 ++++++ .../coplan/agent_instructions/show.text.erb | 50 ++- engine/config/routes.rb | 1 + spec/requests/api/v1/content_spec.rb | 127 ++++++ spec/services/plans/apply_operations_spec.rb | 25 ++ .../services/plans/diff_to_operations_spec.rb | 399 ++++++++++++++++++ spec/services/plans/replace_content_spec.rb | 284 +++++++++++++ 11 files changed, 1254 insertions(+), 6 deletions(-) create mode 100644 engine/app/controllers/coplan/api/v1/content_controller.rb create mode 100644 engine/app/services/coplan/plans/diff_to_operations.rb create mode 100644 engine/app/services/coplan/plans/replace_content.rb create mode 100644 spec/requests/api/v1/content_spec.rb create mode 100644 spec/services/plans/diff_to_operations_spec.rb create mode 100644 spec/services/plans/replace_content_spec.rb diff --git a/engine/app/controllers/coplan/api/v1/base_controller.rb b/engine/app/controllers/coplan/api/v1/base_controller.rb index 4542c36..9f16bdc 100644 --- a/engine/app/controllers/coplan/api/v1/base_controller.rb +++ b/engine/app/controllers/coplan/api/v1/base_controller.rb @@ -2,6 +2,13 @@ module CoPlan module Api module V1 class BaseController < ActionController::API + # Disable Rails' wrap_parameters middleware: it auto-wraps the + # JSON body under the controller's resource name, which collides + # with body params that share that name (e.g. PUT /content with + # `{ "content": "..." }` would silently nest the body under + # params[:content]). + wrap_parameters false + before_action :authenticate_api! after_action :set_agent_instructions_header diff --git a/engine/app/controllers/coplan/api/v1/content_controller.rb b/engine/app/controllers/coplan/api/v1/content_controller.rb new file mode 100644 index 0000000..bf853b3 --- /dev/null +++ b/engine/app/controllers/coplan/api/v1/content_controller.rb @@ -0,0 +1,65 @@ +module CoPlan + module Api + module V1 + # PUT /api/v1/plans/:plan_id/content — replace plan content wholesale. + # + # The agent-friendly edit path: read the plan, edit the markdown + # locally, then PUT the updated content back. The server diffs + # against the current revision, decomposes into operations (so + # comment anchors in unchanged regions survive via OT), and creates + # a new immutable PlanVersion. + # + # Optimistic concurrency: caller MUST supply base_revision matching + # the plan's current_revision, or the request fails with 409. + class ContentController < BaseController + before_action :set_plan + before_action :authorize_plan_access! + + def update + if params[:content].nil? + return render json: { error: "content is required" }, status: :unprocessable_content + end + + base_revision = params[:base_revision]&.to_i + unless base_revision.present? + return render json: { error: "base_revision is required" }, status: :unprocessable_content + end + + result = Plans::ReplaceContent.call( + plan: @plan, + new_content: params[:content].to_s, + base_revision: base_revision, + actor_type: api_author_type, + actor_id: api_actor_id, + change_summary: params[:change_summary], + reason: params[:reason] + ) + + if result[:no_op] + render json: { + revision: @plan.current_revision, + applied: 0, + no_op: true + }, status: :ok + return + end + + version = result[:version] + render json: { + revision: version.revision, + content_sha256: version.content_sha256, + applied: result[:applied], + version_id: version.id + }, status: :created + rescue Plans::ReplaceContent::StaleRevisionError => e + render json: { + error: e.message, + current_revision: e.current_revision + }, status: :conflict + rescue ActiveRecord::RecordInvalid => e + render json: { error: e.record.errors.full_messages.join(", ") }, status: :unprocessable_content + end + end + end + end +end diff --git a/engine/app/services/coplan/plans/apply_operations.rb b/engine/app/services/coplan/plans/apply_operations.rb index 932d2da..d0e1996 100644 --- a/engine/app/services/coplan/plans/apply_operations.rb +++ b/engine/app/services/coplan/plans/apply_operations.rb @@ -38,9 +38,18 @@ def apply_replace_exact(op, index) old_text = op["old_text"] new_text = op["new_text"] - raise OperationError, "Operation #{index}: replace_exact requires 'old_text'" if old_text.blank? + # old_text may be empty ONLY when _pre_resolved_ranges is supplied + # (e.g. pure insertions emitted by Plans::DiffToOperations). Without + # pre-resolved ranges, PositionResolver has nothing to search for. + if old_text.blank? && !op.key?("_pre_resolved_ranges") + raise OperationError, "Operation #{index}: replace_exact requires 'old_text'" + end raise OperationError, "Operation #{index}: replace_exact requires 'new_text'" if new_text.nil? + # Coerce to string so length/delta arithmetic below is always safe + # — clients supplying _pre_resolved_ranges may omit old_text entirely. + old_text = old_text.to_s + ranges = if op.key?("_pre_resolved_ranges") op["_pre_resolved_ranges"] else @@ -59,7 +68,14 @@ def apply_replace_exact(op, index) @content = @content[0...adjusted_start] + new_text + @content[adjusted_end..] - delta = new_text.length - old_text.length + # Delta is computed from the actual range slice, NOT from + # old_text.length. Otherwise a client supplying mismatched + # `_pre_resolved_ranges` and `old_text` (e.g. ranges=[[0,100]], + # old_text="") would corrupt cumulative_delta and persist + # broken positional metadata into the new PlanVersion's + # operations_json — silently breaking all future OT transforms + # through this version. + delta = new_text.length - (range[1] - range[0]) replacements << { "resolved_range" => range, "new_range" => [range[0], range[0] + new_text.length], @@ -74,7 +90,10 @@ def apply_replace_exact(op, index) range = ranges[0] @content = @content[0...range[0]] + new_text + @content[range[1]..] - delta = new_text.length - old_text.length + # See comment above — delta MUST be computed from the actual + # range being replaced, not from the (potentially mismatched) + # `old_text` supplied by the caller. + delta = new_text.length - (range[1] - range[0]) applied_data["resolved_range"] = range applied_data["new_range"] = [range[0], range[0] + new_text.length] applied_data["delta"] = delta diff --git a/engine/app/services/coplan/plans/diff_to_operations.rb b/engine/app/services/coplan/plans/diff_to_operations.rb new file mode 100644 index 0000000..9b24617 --- /dev/null +++ b/engine/app/services/coplan/plans/diff_to_operations.rb @@ -0,0 +1,147 @@ +require "diff/lcs" + +module CoPlan + module Plans + # Converts (old_content, new_content) into an ordered array of replace_exact + # operations whose application reproduces new_content exactly. Each op is + # emitted with `_pre_resolved_ranges` already set (in the coordinate system + # of the content state immediately BEFORE the op is applied), so it can be + # fed straight into Plans::ApplyOperations without re-resolving positions. + # + # Granularity is line-level: consecutive non-equal lines in the LCS diff + # are grouped into a single hunk, which becomes one replace_exact op. This + # is the right granularity for two reasons: + # + # 1. Anchors in unchanged regions survive intact via the existing OT + # engine (Plans::TransformRange) — only anchors that overlap a + # changed hunk get marked out-of-date. + # 2. The resulting operations_json on the new PlanVersion stays compact + # (one entry per hunk, not one per character), which keeps OT + # transforms cheap when later edits rebase through this version. + # + # Operations are emitted in left-to-right order over the OLD content, and + # each `_pre_resolved_ranges` accounts for cumulative deltas from prior + # ops in the sequence — so applying them via ApplyOperations one after + # another produces correct positional metadata on every op. + class DiffToOperations + def self.call(old_content:, new_content:) + new(old_content: old_content, new_content: new_content).call + end + + def initialize(old_content:, new_content:) + @old_content = old_content || "" + @new_content = new_content || "" + end + + def call + return [] if @old_content == @new_content + + old_lines = @old_content.lines + new_lines = @new_content.lines + + # offsets[i] = character offset of the start of line i (offsets[len] = total chars). + # Positions throughout the codebase (anchor_start/anchor_end, resolved_range, etc.) + # are character offsets, NOT byte offsets — so unicode is handled correctly. + old_offsets = build_line_offsets(old_lines) + new_offsets = build_line_offsets(new_lines) + + sdiff = Diff::LCS.sdiff(old_lines, new_lines) + hunks = group_hunks(sdiff) + + cumulative_delta = 0 + + hunks.map do |hunk| + old_start, old_end = char_range(hunk[:old_lines], hunk[:old_anchor], old_offsets) + new_start, new_end = char_range(hunk[:new_lines], hunk[:new_anchor], new_offsets) + + old_text = @old_content[old_start...old_end] || "" + new_text = @new_content[new_start...new_end] || "" + + # Shift positions to account for prior ops' deltas. Because hunks + # are emitted left-to-right and don't overlap, all prior ops are + # strictly before this one — a simple cumulative shift is exact. + adjusted_start = old_start + cumulative_delta + adjusted_end = old_end + cumulative_delta + + op = { + "op" => "replace_exact", + "old_text" => old_text, + "new_text" => new_text, + "_pre_resolved_ranges" => [[adjusted_start, adjusted_end]] + } + + cumulative_delta += new_text.length - old_text.length + op + end + end + + private + + def build_line_offsets(lines) + offsets = [0] + running = 0 + lines.each do |line| + running += line.length + offsets << running + end + offsets + end + + # Groups consecutive non-"=" sdiff entries into hunks. Each hunk + # records the line indexes it touches on each side. Pure insertions + # have an empty :old_lines list; pure deletions have an empty + # :new_lines list — those use the recorded anchor as a zero-width + # insertion point in that side. + def group_hunks(sdiff) + hunks = [] + current_old = [] + current_new = [] + current_old_anchor = nil + current_new_anchor = nil + in_hunk = false + + flush = lambda do + if in_hunk + hunks << { + old_lines: current_old.any? ? (current_old.min..current_old.max).to_a : [], + new_lines: current_new.any? ? (current_new.min..current_new.max).to_a : [], + old_anchor: current_old_anchor, + new_anchor: current_new_anchor + } + current_old = [] + current_new = [] + current_old_anchor = nil + current_new_anchor = nil + in_hunk = false + end + end + + sdiff.each do |ctx| + if ctx.action == "=" + flush.call + next + end + + in_hunk = true + current_old_anchor ||= ctx.old_position + current_new_anchor ||= ctx.new_position + current_old << ctx.old_position if %w[- !].include?(ctx.action) + current_new << ctx.new_position if %w[+ !].include?(ctx.action) + end + + flush.call + hunks + end + + # Returns [start, end] character offsets for a hunk's line list. For a + # pure insertion/deletion (empty line list), uses the anchor as a + # zero-width range at offsets[anchor]. + def char_range(line_indexes, anchor, offsets) + return [offsets[anchor], offsets[anchor]] if line_indexes.empty? + first = line_indexes.first + last = line_indexes.last + [offsets[first], offsets[last + 1]] + end + end + end +end diff --git a/engine/app/services/coplan/plans/replace_content.rb b/engine/app/services/coplan/plans/replace_content.rb new file mode 100644 index 0000000..040c668 --- /dev/null +++ b/engine/app/services/coplan/plans/replace_content.rb @@ -0,0 +1,130 @@ +module CoPlan + module Plans + # Replaces a plan's content wholesale by diffing the supplied new_content + # against the plan's current_content, decomposing the diff into a series + # of replace_exact operations, applying them through Plans::ApplyOperations + # (so each gets resolved_range/new_range positional metadata), and + # persisting the result as a new immutable PlanVersion. + # + # Why this matters: agents are great at producing whole files. Letting + # them PUT the entire updated markdown is much simpler than scripting + # surgical operations, and the line-level diff preserves comment anchors + # in unchanged regions via the existing OT engine. + # + # Concurrency model: optimistic via base_revision. If the plan's + # current_revision has advanced beyond the supplied base_revision, this + # raises StaleRevisionError — the caller should re-read the plan and + # rebase their edits. We don't auto-rebase: a wholesale rewrite that + # didn't see intervening edits would silently clobber them. + class ReplaceContent + class StaleRevisionError < StandardError + attr_reader :current_revision + def initialize(message, current_revision:) + super(message) + @current_revision = current_revision + end + end + + # Raised when DiffToOperations + ApplyOperations don't reproduce the + # caller's new_content exactly — indicates a bug in the diff pipeline. + # Surfaces as a 500 (rather than silently persisting wrong content). + class RoundtripFailureError < StandardError; end + + def self.call(plan:, new_content:, base_revision:, actor_type:, actor_id:, change_summary: nil, reason: nil) + new( + plan: plan, + new_content: new_content, + base_revision: base_revision, + actor_type: actor_type, + actor_id: actor_id, + change_summary: change_summary, + reason: reason + ).call + end + + def initialize(plan:, new_content:, base_revision:, actor_type:, actor_id:, change_summary: nil, reason: nil) + @plan = plan + # Normalize line endings to LF before diffing. Browser textareas, agents + # running on Windows, and copy-paste from various sources commonly emit + # CRLF (`\r\n`). If the stored current_content is LF and the inbound + # new_content is CRLF, every line would diff as changed — producing + # a single wholesale-rewrite op that destroys all comment anchors and + # bloats operations_json. Stripping `\r` keeps the diff focused on + # actual content changes. + @new_content = (new_content || "").delete("\r") + @base_revision = base_revision + @actor_type = actor_type + @actor_id = actor_id + @change_summary = change_summary + @reason = reason + end + + def call + ActiveRecord::Base.transaction do + @plan.lock! + @plan.reload + + if @plan.current_revision != @base_revision + raise StaleRevisionError.new( + "Stale revision. Expected #{@plan.current_revision}, got #{@base_revision}", + current_revision: @plan.current_revision + ) + end + + current_content = @plan.current_content || "" + + # No-op: content is identical → no version, no broadcasts. + if current_content == @new_content + return { version: nil, plan: @plan, applied: 0, no_op: true } + end + + ops = Plans::DiffToOperations.call( + old_content: current_content, + new_content: @new_content + ) + + result = Plans::ApplyOperations.call(content: current_content, operations: ops) + + # Sanity: ApplyOperations must produce exactly the requested content. + # If this ever fires, DiffToOperations has a bug — better to fail + # loudly than silently corrupt the version. + unless result[:content] == @new_content + raise RoundtripFailureError, "DiffToOperations roundtrip failure for plan #{@plan.id}" + end + + new_revision = @plan.current_revision + 1 + diff = Diffy::Diff.new(current_content, @new_content).to_s + + version = PlanVersion.create!( + plan: @plan, + revision: new_revision, + content_markdown: @new_content, + actor_type: @actor_type, + actor_id: @actor_id, + change_summary: @change_summary, + diff_unified: diff.presence, + operations_json: result[:applied], + base_revision: @base_revision, + reason: @reason + ) + + @plan.update!( + current_plan_version: version, + current_revision: new_revision + ) + + @plan.comment_threads.mark_out_of_date_for_new_version!(version) + + Broadcaster.replace_to( + @plan, + target: "plan-header", + partial: "coplan/plans/header", + locals: { plan: @plan } + ) + + { version: version, plan: @plan, applied: result[:applied].length, no_op: false } + end + end + end + end +end diff --git a/engine/app/views/coplan/agent_instructions/show.text.erb b/engine/app/views/coplan/agent_instructions/show.text.erb index de0ffe7..42b19e1 100644 --- a/engine/app/views/coplan/agent_instructions/show.text.erb +++ b/engine/app/views/coplan/agent_instructions/show.text.erb @@ -182,9 +182,44 @@ These are auto-extracted with both the key and title preserved. "<%= @base %>/api/v1/plans/$PLAN_ID/comments" | jq . ``` -## Editing Plans (Lease + Operations) +## Editing Plans (Recommended: Full Content Replacement) -Editing requires three steps: acquire lease → apply operations → release lease. +The simplest and recommended way to edit a plan is to read its current content, edit the markdown locally, and PUT the entire updated content back. The server diffs your version against the current revision, decomposes the change into granular operations, and creates a new immutable PlanVersion. Comment anchors in unchanged regions are preserved automatically. + +**No lease required.** Optimistic concurrency is enforced via `base_revision`. + +### Replace Plan Content + +```bash +# 1. Read the current plan to get its content and revision +PLAN_DATA=$(<%= @curl %> "<%= @base %>/api/v1/plans/$PLAN_ID") +CURRENT_REVISION=$(echo "$PLAN_DATA" | jq -r '.current_revision') +echo "$PLAN_DATA" | jq -r '.current_content' > /tmp/plan.md + +# 2. Edit /tmp/plan.md locally however you like + +# 3. PUT the new content back +<%= @curl %> -X PUT \ + -H "Content-Type: application/json" \ + -d "$(jq -n \ + --arg content "$(cat /tmp/plan.md)" \ + --argjson rev "$CURRENT_REVISION" \ + --arg summary "Rewrote goals section" \ + '{base_revision: $rev, content: $content, change_summary: $summary}')" \ + "<%= @base %>/api/v1/plans/$PLAN_ID/content" | jq . +``` + +Returns: +- `201 Created` with `{revision, content_sha256, applied, version_id}` on success +- `200 OK` with `{no_op: true}` if the content is unchanged +- `409 Conflict` with `{current_revision}` if `base_revision` is stale (re-read the plan and retry) +- `422 Unprocessable Content` if `content` or `base_revision` is missing or invalid + +This is the right tool for any non-trivial edit: rewriting a section, adding multiple paragraphs, restructuring headings, applying many small fixes at once, or doing a wholesale rewrite. + +## Editing Plans (Advanced: Lease + Operations) + +For surgical, targeted edits where you want to express the change as a single operation (and keep the diff minimal), use the lease + operations path. Editing requires three steps: acquire lease → apply operations → release lease. ### 1. Acquire Edit Lease @@ -346,10 +381,19 @@ Review each open comment thread and categorize it: ### 3. Apply Edits -For approved changes: acquire lease → apply operations → release lease → resolve threads. +For approved changes, the recommended path is: read the snapshot → edit the markdown locally → `PUT /api/v1/plans/:id/content` → resolve the addressed threads. Use the lease + operations path only for surgical, single-operation edits. ## Typical Workflow +### Recommended (full content replacement) + +1. **Read** the plan: `GET /api/v1/plans/:id/snapshot` +2. **Edit** the markdown locally +3. **Replace** content: `PUT /api/v1/plans/:id/content` with `{base_revision, content, change_summary}` +4. **Comment** on changes: `POST /api/v1/plans/:id/comments` + +### Advanced (surgical operations) + 1. **Read** the plan: `GET /api/v1/plans/:id/snapshot` 2. **Acquire lease**: `POST /api/v1/plans/:id/lease` 3. **Apply operations**: `POST /api/v1/plans/:id/operations` (can call multiple times while lease is held) diff --git a/engine/config/routes.rb b/engine/config/routes.rb index 5304f00..c3eab8f 100644 --- a/engine/config/routes.rb +++ b/engine/config/routes.rb @@ -35,6 +35,7 @@ get :versions, on: :member get :comments, on: :member get :snapshot, on: :member + resource :content, only: [:update], controller: "content" resource :lease, only: [:create, :update, :destroy], controller: "leases" resources :operations, only: [:create] resources :sessions, only: [:create, :show], controller: "sessions" do diff --git a/spec/requests/api/v1/content_spec.rb b/spec/requests/api/v1/content_spec.rb new file mode 100644 index 0000000..aa5b50e --- /dev/null +++ b/spec/requests/api/v1/content_spec.rb @@ -0,0 +1,127 @@ +require "rails_helper" + +RSpec.describe "Api::V1::Content", type: :request do + let(:alice) { create(:coplan_user, :admin) } + let(:alice_token) { create(:api_token, user: alice, raw_token: "test-token-alice") } + let(:headers) { { "Authorization" => "Bearer test-token-alice" } } + let(:initial_content) { "# Plan\n\nSection one.\n\nSection two.\n" } + let!(:plan) do + p = CoPlan::Plan.create!(title: "Test", created_by_user: alice, status: "considering") + v = CoPlan::PlanVersion.create!( + plan: p, revision: 1, + content_markdown: initial_content, + actor_type: "human", actor_id: alice.id, + operations_json: [] + ) + p.update!(current_plan_version: v, current_revision: 1) + p + end + + before { alice_token } + + def put_content(body, params: {}) + payload = { base_revision: plan.current_revision, content: body }.merge(params) + put api_v1_plan_content_path(plan), params: payload, headers: headers, as: :json + end + + describe "happy path" do + it "creates a new version with the supplied content" do + new_body = initial_content.sub("Section one.", "Section ONE rewritten.") + + expect { put_content(new_body) }.to change(CoPlan::PlanVersion, :count).by(1) + expect(response).to have_http_status(:created) + + body = JSON.parse(response.body) + expect(body["revision"]).to eq(2) + expect(body["applied"]).to be > 0 + expect(body["version_id"]).to be_present + + expect(plan.reload.current_content).to eq(new_body) + expect(plan.current_revision).to eq(2) + end + + it "returns 200 ok with no_op:true when content is unchanged" do + expect { put_content(initial_content) }.not_to change(CoPlan::PlanVersion, :count) + expect(response).to have_http_status(:ok) + body = JSON.parse(response.body) + expect(body["no_op"]).to be true + expect(body["applied"]).to eq(0) + expect(body["revision"]).to eq(1) + end + + it "accepts change_summary and persists it on the version" do + new_body = initial_content + "\nappended.\n" + put_content(new_body, params: { change_summary: "added appendix" }) + expect(response).to have_http_status(:created) + + version = plan.plan_versions.find_by(revision: 2) + expect(version.change_summary).to eq("added appendix") + end + end + + describe "validation" do + it "returns 422 when base_revision is missing" do + put api_v1_plan_content_path(plan), + params: { content: "anything" }, headers: headers, as: :json + expect(response).to have_http_status(:unprocessable_content) + expect(JSON.parse(response.body)["error"]).to match(/base_revision/) + end + + it "returns 422 when content key is missing entirely" do + put api_v1_plan_content_path(plan), + params: { base_revision: 1 }, headers: headers, as: :json + expect(response).to have_http_status(:unprocessable_content) + expect(JSON.parse(response.body)["error"]).to match(/content/) + end + + it "rejects empty content with 422 (PlanVersion requires content_markdown)" do + put_content("") + expect(response).to have_http_status(:unprocessable_content) + expect(plan.reload.current_revision).to eq(1) + end + end + + describe "concurrency" do + it "returns 409 with current_revision when base_revision is stale" do + put api_v1_plan_content_path(plan), + params: { base_revision: 999, content: "anything" }, + headers: headers, as: :json + expect(response).to have_http_status(:conflict) + body = JSON.parse(response.body) + expect(body["error"]).to match(/Stale/) + expect(body["current_revision"]).to eq(1) + end + end + + describe "auth" do + it "returns 401 without bearer token" do + put api_v1_plan_content_path(plan), + params: { base_revision: 1, content: "x" }, as: :json + expect(response).to have_http_status(:unauthorized) + end + end + + describe "anchor preservation through full content replacement" do + let!(:thread) do + anchor_text = "Section one." + CoPlan::CommentThread.create!( + plan: plan, plan_version: plan.current_plan_version, + created_by_user: alice, + anchor_text: anchor_text, + anchor_revision: 1, + anchor_start: initial_content.index(anchor_text), + anchor_end: initial_content.index(anchor_text) + anchor_text.length, + status: "todo" + ) + end + + it "shifts unaffected anchors and marks overlapping ones out-of-date" do + new_body = initial_content.sub("Section one.", "Section ONE rewritten with more text.") + put_content(new_body) + expect(response).to have_http_status(:created) + + thread.reload + expect(thread.out_of_date).to be true + end + end +end diff --git a/spec/services/plans/apply_operations_spec.rb b/spec/services/plans/apply_operations_spec.rb index 9294154..b761160 100644 --- a/spec/services/plans/apply_operations_spec.rb +++ b/spec/services/plans/apply_operations_spec.rb @@ -385,5 +385,30 @@ expect(second_applied["new_range"]).to eq([10, 13]) expect(second_applied["delta"]).to eq(-4) end + + # Regression: delta MUST be computed from the actual range slice, not + # from a (possibly mismatched) caller-supplied old_text. Otherwise a + # client passing _pre_resolved_ranges with empty/stale old_text could + # corrupt cumulative_delta and persist broken positional metadata that + # silently breaks all future OT transforms through this version. + it "computes delta from the resolved range, not from old_text length" do + content = "0123456789ABCDEFGHIJ" # 20 chars + result = CoPlan::Plans::ApplyOperations.call( + content: content, + operations: [ + { + "op" => "replace_exact", + "old_text" => "", # intentionally wrong / empty + "new_text" => "x", + "_pre_resolved_ranges" => [[0, 10]] + } + ] + ) + + expect(result[:content]).to eq("xABCDEFGHIJ") + expect(result[:applied][0]["delta"]).to eq(-9) # 1 - (10 - 0), NOT 1 - 0 + expect(result[:applied][0]["resolved_range"]).to eq([0, 10]) + expect(result[:applied][0]["new_range"]).to eq([0, 1]) + end end end diff --git a/spec/services/plans/diff_to_operations_spec.rb b/spec/services/plans/diff_to_operations_spec.rb new file mode 100644 index 0000000..f03c87a --- /dev/null +++ b/spec/services/plans/diff_to_operations_spec.rb @@ -0,0 +1,399 @@ +require "rails_helper" + +RSpec.describe CoPlan::Plans::DiffToOperations do + # Property: applying the produced ops to old_content via ApplyOperations + # MUST yield new_content exactly. This is the hard invariant the diffing + # logic exists to satisfy. + def assert_roundtrip(old_content, new_content) + ops = described_class.call(old_content: old_content, new_content: new_content) + result = CoPlan::Plans::ApplyOperations.call(content: old_content, operations: ops) + expect(result[:content]).to eq(new_content), -> { + "Roundtrip failed.\nOld:\n#{old_content.inspect}\nNew:\n#{new_content.inspect}\n" \ + "Got:\n#{result[:content].inspect}\nOps:\n#{ops.inspect}" + } + [ops, result] + end + + describe "no-op cases" do + it "returns [] when content is identical" do + expect(described_class.call(old_content: "abc", new_content: "abc")).to eq([]) + end + + it "returns [] when both contents are empty" do + expect(described_class.call(old_content: "", new_content: "")).to eq([]) + end + + it "handles nil inputs as empty" do + expect(described_class.call(old_content: nil, new_content: nil)).to eq([]) + end + end + + describe "single hunk" do + it "produces one op when a single line is changed in the middle" do + old = "alpha\nbeta\ngamma\n" + new = "alpha\nBETA\ngamma\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["op"]).to eq("replace_exact") + expect(ops[0]["old_text"]).to eq("beta\n") + expect(ops[0]["new_text"]).to eq("BETA\n") + expect(ops[0]["_pre_resolved_ranges"]).to eq([[6, 11]]) + end + + it "produces one op for an append at end of file" do + old = "line one\nline two\n" + new = "line one\nline two\nline three\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("") + expect(ops[0]["new_text"]).to eq("line three\n") + expect(ops[0]["_pre_resolved_ranges"]).to eq([[old.length, old.length]]) + end + + it "produces one op for an insert at start of file" do + old = "line two\nline three\n" + new = "line one\nline two\nline three\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("") + expect(ops[0]["new_text"]).to eq("line one\n") + expect(ops[0]["_pre_resolved_ranges"]).to eq([[0, 0]]) + end + + it "produces one op for an insert in the middle" do + old = "alpha\ngamma\n" + new = "alpha\nbeta\ngamma\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("") + expect(ops[0]["new_text"]).to eq("beta\n") + expect(ops[0]["_pre_resolved_ranges"]).to eq([[6, 6]]) + end + + it "produces one op for a deletion in the middle" do + old = "alpha\nbeta\ngamma\n" + new = "alpha\ngamma\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("beta\n") + expect(ops[0]["new_text"]).to eq("") + expect(ops[0]["_pre_resolved_ranges"]).to eq([[6, 11]]) + end + + it "produces one op for a deletion at end of file" do + old = "a\nb\nc\n" + new = "a\nb\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("c\n") + expect(ops[0]["new_text"]).to eq("") + end + + it "produces one op for a deletion at start of file" do + old = "a\nb\nc\n" + new = "b\nc\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("a\n") + expect(ops[0]["new_text"]).to eq("") + expect(ops[0]["_pre_resolved_ranges"]).to eq([[0, 2]]) + end + + it "groups multiple consecutive changed lines into one hunk" do + old = "header\na\nb\nc\nfooter\n" + new = "header\nA\nB\nC\nfooter\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("a\nb\nc\n") + expect(ops[0]["new_text"]).to eq("A\nB\nC\n") + end + + it "groups a delete + insert pair into one hunk when they're contiguous" do + old = "header\nold1\nold2\nfooter\n" + new = "header\nnew1\nnew2\nnew3\nfooter\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + end + end + + describe "multiple disjoint hunks" do + it "produces independent ops for non-adjacent changes" do + old = "a\nb\nc\nd\ne\nf\ng\n" + new = "a\nB\nc\nd\nE\nf\ng\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(2) + expect(ops[0]["old_text"]).to eq("b\n") + expect(ops[0]["new_text"]).to eq("B\n") + expect(ops[1]["old_text"]).to eq("e\n") + expect(ops[1]["new_text"]).to eq("E\n") + end + + it "shifts later ops' ranges to account for prior ops' delta growth" do + old = "a\nb\nc\n" + new = "AAAAAA\nb\nC\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(2) + # First op replaces "a\n" (positions 0..2) with "AAAAAA\n" (delta = +5) + expect(ops[0]["_pre_resolved_ranges"]).to eq([[0, 2]]) + # Second op replaces "c\n" — originally at positions 4..6 in old, + # but after op 1 those positions are at 9..11 in the working content. + expect(ops[1]["_pre_resolved_ranges"]).to eq([[9, 11]]) + end + + it "shifts later ops' ranges to account for prior ops' delta shrinkage" do + old = "aaaaaa\nb\nc\n" + new = "X\nb\nC\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(2) + expect(ops[0]["_pre_resolved_ranges"]).to eq([[0, 7]]) + # Original "c\n" was at positions 9..11; after op 1 (delta = -5) they're at 4..6. + expect(ops[1]["_pre_resolved_ranges"]).to eq([[4, 6]]) + end + + it "handles insert + change + delete in one document" do + old = "header\nkeep1\nold_change\nkeep2\nold_delete\nfooter\n" + new = "INSERTED\nheader\nkeep1\nnew_change\nkeep2\nfooter\n" + assert_roundtrip(old, new) + end + end + + describe "edge cases" do + it "works when old is empty" do + ops, _ = assert_roundtrip("", "alpha\nbeta\n") + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("") + expect(ops[0]["new_text"]).to eq("alpha\nbeta\n") + end + + it "works when new is empty" do + ops, _ = assert_roundtrip("alpha\nbeta\n", "") + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("alpha\nbeta\n") + expect(ops[0]["new_text"]).to eq("") + end + + it "preserves trailing-newline absence in old" do + old = "a\nb" # no trailing newline + new = "a\nB" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("b") + expect(ops[0]["new_text"]).to eq("B") + end + + it "preserves trailing-newline addition" do + assert_roundtrip("a\nb", "a\nb\n") + end + + it "preserves trailing-newline removal" do + assert_roundtrip("a\nb\n", "a\nb") + end + + it "handles unicode content correctly (positions are character-based)" do + old = "café\n☕ coffee\nend\n" + new = "café\n☕ tea\nend\n" + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to eq("☕ coffee\n") + expect(ops[0]["new_text"]).to eq("☕ tea\n") + end + + it "handles a fully replaced single-line file" do + ops, _ = assert_roundtrip("hello", "world") + expect(ops.length).to eq(1) + end + + it "handles realistic markdown rewrite of a section" do + old = <<~MD + # Plan + + ## Goals + + We should use unit tests. + + ## Timeline + + Q1 2026 delivery. + MD + new = <<~MD + # Plan + + ## Goals + + We should use integration tests with full coverage. + Add CI gating on the coverage threshold. + + ## Timeline + + Q1 2026 delivery. + MD + ops, _ = assert_roundtrip(old, new) + # The Goals body changed; everything else is unchanged → one hunk + expect(ops.length).to eq(1) + expect(ops[0]["old_text"]).to include("unit tests") + expect(ops[0]["new_text"]).to include("integration tests with full coverage") + expect(ops[0]["new_text"]).to include("CI gating") + end + end + + describe "operations metadata after applying via ApplyOperations" do + it "produces resolved_range / new_range / delta on the applied output" do + old = "alpha\nbeta\ngamma\n" + new = "alpha\nBETA\ngamma\n" + ops, result = assert_roundtrip(old, new) + applied = result[:applied] + expect(applied.length).to eq(1) + expect(applied[0]["resolved_range"]).to eq([6, 11]) + expect(applied[0]["new_range"]).to eq([6, 11]) + expect(applied[0]["delta"]).to eq(0) + end + + it "produces correct positional metadata across multiple ops" do + old = "a\nb\nc\n" + new = "AAAAAA\nb\nC\n" + _, result = assert_roundtrip(old, new) + applied = result[:applied] + expect(applied.length).to eq(2) + expect(applied[0]["resolved_range"]).to eq([0, 2]) + expect(applied[0]["new_range"]).to eq([0, 7]) + expect(applied[0]["delta"]).to eq(5) + expect(applied[1]["resolved_range"]).to eq([9, 11]) + expect(applied[1]["new_range"]).to eq([9, 11]) + expect(applied[1]["delta"]).to eq(0) + end + end + + describe "OT compatibility (anchor preservation through generated ops)" do + # The whole point of fine-grained hunks is that anchors in unchanged + # regions can be transformed forward by Plans::TransformRange. + it "lets an anchor before all changes survive unchanged" do + old = "alpha beta gamma\nMIDDLE\nepsilon zeta\n" + new = "alpha beta gamma\nMIDDLE_CHANGED\nepsilon zeta\n" + ops, _ = assert_roundtrip(old, new) + + # Anchor on "alpha" at [0, 5] — unchanged, should survive + version = double(operations_json: CoPlan::Plans::ApplyOperations.call(content: old, operations: ops)[:applied]) + transformed = CoPlan::Plans::TransformRange.transform_through_versions([0, 5], [version]) + expect(transformed).to eq([0, 5]) + end + + it "shifts an anchor after the change by the delta" do + old = "a\nb\nc\n" # 6 chars + new = "AAAAAA\nb\nc\n" # 11 chars + ops, _ = assert_roundtrip(old, new) + + # Anchor on "c" at [4, 5] in old — should shift +5 to [9, 10] in new + applied = CoPlan::Plans::ApplyOperations.call(content: old, operations: ops)[:applied] + version = double(operations_json: applied) + transformed = CoPlan::Plans::TransformRange.transform_through_versions([4, 5], [version]) + expect(transformed).to eq([9, 10]) + end + + it "marks an anchor inside the changed region as conflicting" do + old = "a\nbeta\nc\n" + new = "a\nGAMMA\nc\n" + ops, _ = assert_roundtrip(old, new) + + # Anchor on "beta" at [2, 6] — overlaps the changed range, must conflict + applied = CoPlan::Plans::ApplyOperations.call(content: old, operations: ops)[:applied] + version = double(operations_json: applied) + expect { + CoPlan::Plans::TransformRange.transform_through_versions([2, 6], [version]) + }.to raise_error(CoPlan::Plans::TransformRange::Conflict) + end + end + + describe "tricky LCS alignments" do + # When old has repeated identical lines, LCS has multiple valid alignments; + # whichever it picks, the produced ops MUST roundtrip exactly. + it "roundtrips when old has many repeated identical lines (insertion)" do + old = "x\nx\nx\nx\nx\n" + new = "x\nx\nNEW\nx\nx\nx\n" + assert_roundtrip(old, new) + end + + it "roundtrips when old has many repeated identical lines (deletion)" do + old = "x\nx\nx\nx\nx\n" + new = "x\nx\nx\nx\n" + assert_roundtrip(old, new) + end + + it "roundtrips when lines share a long common prefix" do + old = "prefix_one\nprefix_two\nprefix_three\n" + new = "prefix_one\nprefix_TWO_changed\nprefix_three\n" + assert_roundtrip(old, new) + end + + it "roundtrips when lines share a common suffix" do + old = "alpha_end\nbeta_end\ngamma_end\n" + new = "alpha_end\nBETA_end\ngamma_end\n" + assert_roundtrip(old, new) + end + + it "roundtrips a contiguous delete-then-insert at the same position" do + old = "header\nold_a\nold_b\nfooter\n" + new = "header\nfooter\nnew_a\nnew_b\n" + assert_roundtrip(old, new) + end + + it "handles mixed delete + change + insert in one hunk" do + old = "h\nremove1\nremove2\nchange_me\nfooter\n" + new = "h\nchanged\ninsert1\ninsert2\nfooter\n" + assert_roundtrip(old, new) + end + + it "roundtrips when many small disjoint hunks span a large file" do + old_lines = Array.new(50) { |i| "line_#{i}\n" } + new_lines = old_lines.dup + [3, 11, 27, 41].each { |i| new_lines[i] = "CHANGED_#{i}\n" } + assert_roundtrip(old_lines.join, new_lines.join) + end + end + + describe "very long single-line content" do + it "roundtrips a 100k-character single line" do + old = "x" * 100_000 + new = "y" * 100_000 + ops, _ = assert_roundtrip(old, new) + expect(ops.length).to eq(1) + end + + it "roundtrips a long paragraph with mid-line word change" do + paragraph = "word " * 5_000 # 25k chars, single line + old = paragraph + new = paragraph.sub("word ", "WORD ") + # No newlines: this is one logical line, so the entire paragraph + # diffs as a single replacement. That's fine — we just need exactness. + assert_roundtrip(old, new) + end + end + + describe "stress: random rewrites roundtrip" do + # Small fuzz check: random line-level edits must always roundtrip. + it "roundtrips for many random old/new pairs" do + srand(42) # deterministic + 30.times do + old_lines = Array.new(rand(0..15)) { "line_#{rand(1000)}\n" } + new_lines = old_lines.dup + + # Apply a few random mutations + rand(0..5).times do + op = %i[insert delete change].sample + case op + when :insert + idx = rand(0..new_lines.length) + new_lines.insert(idx, "ins_#{rand(1000)}\n") + when :delete + next if new_lines.empty? + new_lines.delete_at(rand(new_lines.length)) + when :change + next if new_lines.empty? + new_lines[rand(new_lines.length)] = "chg_#{rand(1000)}\n" + end + end + + assert_roundtrip(old_lines.join, new_lines.join) + end + end + end +end diff --git a/spec/services/plans/replace_content_spec.rb b/spec/services/plans/replace_content_spec.rb new file mode 100644 index 0000000..e3d28db --- /dev/null +++ b/spec/services/plans/replace_content_spec.rb @@ -0,0 +1,284 @@ +require "rails_helper" + +RSpec.describe CoPlan::Plans::ReplaceContent do + let(:user) { create(:coplan_user) } + let(:initial_content) do + <<~MD + # My Plan + + ## Goals + + We should use unit tests. + + ## Timeline + + Q1 2026 delivery. + MD + end + let!(:plan) do + plan = CoPlan::Plan.create!(title: "Test Plan", created_by_user: user) + version = CoPlan::PlanVersion.create!( + plan: plan, revision: 1, + content_markdown: initial_content, + actor_type: "human", actor_id: user.id, + operations_json: [] + ) + plan.update!(current_plan_version: version, current_revision: 1) + plan + end + + describe "happy path" do + let(:new_content) { initial_content.sub("unit tests", "integration tests") } + + it "creates a new PlanVersion with the new content" do + expect { + described_class.call( + plan: plan, + new_content: new_content, + base_revision: 1, + actor_type: "local_agent", + actor_id: user.id + ) + }.to change(CoPlan::PlanVersion, :count).by(1) + + version = plan.reload.current_plan_version + expect(version.content_markdown).to eq(new_content) + expect(version.revision).to eq(2) + expect(version.base_revision).to eq(1) + expect(version.actor_type).to eq("local_agent") + end + + it "stores positional metadata in operations_json so OT can rebase later anchors" do + result = described_class.call( + plan: plan, + new_content: new_content, + base_revision: 1, + actor_type: "local_agent", + actor_id: user.id + ) + + ops_json = result[:version].operations_json + expect(ops_json).to be_an(Array).and(be_present) + ops_json.each do |op| + expect(op).to have_key("resolved_range") + expect(op).to have_key("new_range") + end + end + + it "computes a diff_unified summary" do + result = described_class.call( + plan: plan, + new_content: new_content, + base_revision: 1, + actor_type: "local_agent", + actor_id: user.id + ) + + expect(result[:version].diff_unified).to include("-We should use unit tests.") + expect(result[:version].diff_unified).to include("+We should use integration tests.") + end + + it "carries change_summary onto the version" do + result = described_class.call( + plan: plan, + new_content: new_content, + base_revision: 1, + actor_type: "local_agent", + actor_id: user.id, + change_summary: "Switched to integration tests" + ) + + expect(result[:version].change_summary).to eq("Switched to integration tests") + end + + it "is a no-op when content is unchanged" do + result = described_class.call( + plan: plan, + new_content: initial_content, + base_revision: 1, + actor_type: "local_agent", + actor_id: user.id + ) + + expect(result[:no_op]).to be true + expect(result[:version]).to be_nil + expect(plan.reload.current_revision).to eq(1) + expect(CoPlan::PlanVersion.where(plan: plan).count).to eq(1) + end + end + + describe "stale revision" do + it "raises StaleRevisionError when base_revision lags current_revision" do + # Bump current_revision by adding a version + v2 = CoPlan::PlanVersion.create!( + plan: plan, revision: 2, + content_markdown: initial_content + "\nappended\n", + actor_type: "human", actor_id: user.id, + operations_json: [] + ) + plan.update!(current_plan_version: v2, current_revision: 2) + + expect { + described_class.call( + plan: plan, + new_content: "ignored", + base_revision: 1, + actor_type: "local_agent", + actor_id: user.id + ) + }.to raise_error(CoPlan::Plans::ReplaceContent::StaleRevisionError) do |err| + expect(err.current_revision).to eq(2) + end + end + + it "does not create a version when stale" do + v2 = CoPlan::PlanVersion.create!( + plan: plan, revision: 2, + content_markdown: initial_content + "\nappended\n", + actor_type: "human", actor_id: user.id, + operations_json: [] + ) + plan.update!(current_plan_version: v2, current_revision: 2) + + expect { + begin + described_class.call( + plan: plan, new_content: "ignored", base_revision: 1, + actor_type: "local_agent", actor_id: user.id + ) + rescue CoPlan::Plans::ReplaceContent::StaleRevisionError + # expected + end + }.not_to change(CoPlan::PlanVersion, :count) + end + end + + describe "line ending normalization" do + it "normalizes CRLF in new_content to LF before diffing" do + # Stored content uses LF; agent submits CRLF (e.g. from Windows or a textarea). + # Without normalization, every line would diff as changed and a single + # wholesale-rewrite op would destroy all comment anchors. + crlf_content = initial_content.gsub("\n", "\r\n") + result = described_class.call( + plan: plan, new_content: crlf_content, base_revision: 1, + actor_type: "local_agent", actor_id: user.id + ) + + # Pure line-ending difference: should be detected as no-op since the + # normalized content matches the stored LF content exactly. + expect(result[:no_op]).to be true + expect(result[:version]).to be_nil + end + + it "normalizes CRLF and only emits ops for actual content changes" do + crlf_with_one_change = initial_content + .sub("unit tests", "integration tests") + .gsub("\n", "\r\n") + + result = described_class.call( + plan: plan, new_content: crlf_with_one_change, base_revision: 1, + actor_type: "local_agent", actor_id: user.id + ) + + # Only the unit→integration change should produce an op, not every line. + expect(result[:no_op]).to be false + expect(result[:applied]).to eq(1) + expect(result[:version].content_markdown).not_to include("\r") + expect(result[:version].content_markdown).to include("integration tests") + end + end + + describe "roundtrip safety" do + it "raises RoundtripFailureError if ApplyOperations doesn't reproduce new_content" do + # Force a failure by stubbing DiffToOperations to return ops that don't match. + allow(CoPlan::Plans::DiffToOperations).to receive(:call).and_return([]) + + new_content = initial_content + "\nappended\n" + expect { + described_class.call( + plan: plan, new_content: new_content, base_revision: 1, + actor_type: "local_agent", actor_id: user.id + ) + }.to raise_error(CoPlan::Plans::ReplaceContent::RoundtripFailureError, /roundtrip failure/) + + expect(plan.reload.current_revision).to eq(1) + expect(CoPlan::PlanVersion.where(plan: plan).count).to eq(1) + end + end + + describe "comment thread anchor preservation" do + let!(:thread_before) do + CoPlan::CommentThread.create!( + plan: plan, + plan_version: plan.current_plan_version, + created_by_user: user, + anchor_text: "My Plan", + anchor_revision: 1, + anchor_start: initial_content.index("My Plan"), + anchor_end: initial_content.index("My Plan") + "My Plan".length, + status: "todo" + ) + end + + let!(:thread_inside_change) do + CoPlan::CommentThread.create!( + plan: plan, + plan_version: plan.current_plan_version, + created_by_user: user, + anchor_text: "unit tests", + anchor_revision: 1, + anchor_start: initial_content.index("unit tests"), + anchor_end: initial_content.index("unit tests") + "unit tests".length, + status: "todo" + ) + end + + let!(:thread_after_change) do + CoPlan::CommentThread.create!( + plan: plan, + plan_version: plan.current_plan_version, + created_by_user: user, + anchor_text: "Q1 2026 delivery.", + anchor_revision: 1, + anchor_start: initial_content.index("Q1 2026 delivery."), + anchor_end: initial_content.index("Q1 2026 delivery.") + "Q1 2026 delivery.".length, + status: "todo" + ) + end + + it "keeps anchors before the change intact" do + new_content = initial_content.sub("unit tests", "integration tests with full coverage") + described_class.call( + plan: plan, new_content: new_content, base_revision: 1, + actor_type: "local_agent", actor_id: user.id + ) + + thread_before.reload + expect(thread_before.out_of_date).to be false + expect(new_content[thread_before.anchor_start...thread_before.anchor_end]).to eq("My Plan") + end + + it "marks anchors that overlap the change as out-of-date" do + new_content = initial_content.sub("unit tests", "integration tests") + described_class.call( + plan: plan, new_content: new_content, base_revision: 1, + actor_type: "local_agent", actor_id: user.id + ) + + thread_inside_change.reload + expect(thread_inside_change.out_of_date).to be true + end + + it "shifts anchors after the change by the delta" do + new_content = initial_content.sub("unit tests", "integration tests with full coverage") + described_class.call( + plan: plan, new_content: new_content, base_revision: 1, + actor_type: "local_agent", actor_id: user.id + ) + + thread_after_change.reload + expect(thread_after_change.out_of_date).to be false + expect(new_content[thread_after_change.anchor_start...thread_after_change.anchor_end]).to eq("Q1 2026 delivery.") + end + end +end From 6366456e5e0bbb1f0f1d4f125bee6dfca5942805 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Wed, 29 Apr 2026 14:47:48 -0400 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../coplan/api/v1/content_controller.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/engine/app/controllers/coplan/api/v1/content_controller.rb b/engine/app/controllers/coplan/api/v1/content_controller.rb index bf853b3..965c21e 100644 --- a/engine/app/controllers/coplan/api/v1/content_controller.rb +++ b/engine/app/controllers/coplan/api/v1/content_controller.rb @@ -20,11 +20,21 @@ def update return render json: { error: "content is required" }, status: :unprocessable_content end - base_revision = params[:base_revision]&.to_i - unless base_revision.present? + raw_base_revision = params[:base_revision] + if raw_base_revision.blank? return render json: { error: "base_revision is required" }, status: :unprocessable_content end + begin + base_revision = Integer(raw_base_revision) + rescue ArgumentError, TypeError + return render json: { error: "base_revision must be a positive integer" }, status: :unprocessable_content + end + + if base_revision <= 0 + return render json: { error: "base_revision must be a positive integer" }, status: :unprocessable_content + end + result = Plans::ReplaceContent.call( plan: @plan, new_content: params[:content].to_s,