From 7becc6073fa822446936ad2246e8f828bb86c860 Mon Sep 17 00:00:00 2001 From: Bez Hermoso Date: Thu, 9 Jul 2026 13:42:35 -0700 Subject: [PATCH 1/2] Add line range qualifier to replace_exact and send checkbox source line Co-Authored-By: Claude Fable 5 --- .../controllers/coplan/plans_controller.rb | 15 ++++- engine/app/helpers/coplan/markdown_helper.rb | 12 ++-- .../controllers/coplan/checkbox_controller.js | 12 ++-- .../services/coplan/plans/commit_session.rb | 2 + .../coplan/plans/position_resolver.rb | 56 +++++++++++++++++++ .../coplan/agent_instructions/show.text.erb | 16 ++++++ 6 files changed, 104 insertions(+), 9 deletions(-) diff --git a/engine/app/controllers/coplan/plans_controller.rb b/engine/app/controllers/coplan/plans_controller.rb index 48ec8648..ce0082cc 100644 --- a/engine/app/controllers/coplan/plans_controller.rb +++ b/engine/app/controllers/coplan/plans_controller.rb @@ -138,6 +138,17 @@ def toggle_checkbox return end + # Optional source line number scoping the replacement to a single-line + # box, so identical task lines elsewhere in the document can't collide. + line = nil + if params[:line].present? + line = Integer(params[:line], exception: false) + if line.nil? || line < 1 + render json: { error: "line must be a positive integer" }, status: :unprocessable_content + return + end + end + ActiveRecord::Base.transaction do @plan.lock! @plan.reload @@ -148,9 +159,11 @@ def toggle_checkbox end current_content = @plan.current_content || "" + operation = { "op" => "replace_exact", "old_text" => old_text, "new_text" => new_text } + operation["lines"] = line if line result = Plans::ApplyOperations.call( content: current_content, - operations: [{ "op" => "replace_exact", "old_text" => old_text, "new_text" => new_text }] + operations: [operation] ) new_revision = @plan.current_revision + 1 diff --git a/engine/app/helpers/coplan/markdown_helper.rb b/engine/app/helpers/coplan/markdown_helper.rb index f7029415..a026e2a3 100644 --- a/engine/app/helpers/coplan/markdown_helper.rb +++ b/engine/app/helpers/coplan/markdown_helper.rb @@ -14,7 +14,7 @@ module MarkdownHelper details summary ].freeze - ALLOWED_ATTRIBUTES = %w[id class href src alt title type checked disabled data-line-text data-action data-coplan--checkbox-target data-mention-username].freeze + ALLOWED_ATTRIBUTES = %w[id class href src alt title type checked disabled data-line data-line-text data-action data-coplan--checkbox-target data-mention-username].freeze # Matches `[@username](mention:username)` where the bracket text and link # target encode the same username. Username allows letters, digits, dots, @@ -78,13 +78,14 @@ def make_checkboxes_interactive(html, content) task_lines = extract_task_lines(content) checkboxes.each_with_index do |cb, i| - line_text = task_lines[i] + line_number, line_text = task_lines[i] next unless line_text cb.remove_attribute("disabled") cb["data-action"] = "coplan--checkbox#toggle" cb["data-coplan--checkbox-target"] = "checkbox" cb["data-line-text"] = line_text + cb["data-line"] = line_number.to_s li = cb.parent next unless li&.name == "li" @@ -102,17 +103,20 @@ def make_checkboxes_interactive(html, content) doc.to_html end + # Returns [1-based line number, rstripped line text] pairs for each task + # line, in document order. Fenced lines are counted (so numbering stays + # accurate) but never treated as task lines. def extract_task_lines(content) lines = [] in_fence = false - content.to_s.each_line do |line| + content.to_s.each_line.with_index(1) do |line, line_number| stripped = line.rstrip if stripped.match?(/\A(`{3,}|~{3,})/) in_fence = !in_fence next end next if in_fence - lines << stripped if stripped.match?(/^\s*[*+-]\s+\[[ xX]\]\s/) + lines << [line_number, stripped] if stripped.match?(/^\s*[*+-]\s+\[[ xX]\]\s/) end lines end diff --git a/engine/app/javascript/controllers/coplan/checkbox_controller.js b/engine/app/javascript/controllers/coplan/checkbox_controller.js index dc3be388..2a5878de 100644 --- a/engine/app/javascript/controllers/coplan/checkbox_controller.js +++ b/engine/app/javascript/controllers/coplan/checkbox_controller.js @@ -16,14 +16,17 @@ export default class extends Controller { ? lineText.replace(/([*+-]\s+)\[[ ]\]/, "$1[x]") : lineText.replace(/([*+-]\s+)\[[xX]\]/, "$1[ ]") + // Source line number disambiguates duplicate task-line text server-side. + const line = parseInt(checkbox.dataset.line, 10) + // Optimistic UI: update immediately checkbox.dataset.lineText = newText this.inflight = true - this.#sendToggle({ checkbox, oldText, newText, nowChecked, retried: false }) + this.#sendToggle({ checkbox, oldText, newText, line, nowChecked, retried: false }) } - #sendToggle({ checkbox, oldText, newText, nowChecked, retried }) { + #sendToggle({ checkbox, oldText, newText, line, nowChecked, retried }) { const token = document.querySelector('meta[name="csrf-token"]')?.content fetch(this.toggleUrlValue, { @@ -36,7 +39,8 @@ export default class extends Controller { body: JSON.stringify({ old_text: oldText, new_text: newText, - base_revision: this.revisionValue + base_revision: this.revisionValue, + ...(Number.isInteger(line) && line >= 1 ? { line } : {}) }) }).then(response => { if (response.ok) { @@ -50,7 +54,7 @@ export default class extends Controller { if (data.current_revision) { this.revisionValue = data.current_revision } - this.#sendToggle({ checkbox, oldText, newText, nowChecked, retried: true }) + this.#sendToggle({ checkbox, oldText, newText, line, nowChecked, retried: true }) }) } else { this.#revert(checkbox, oldText, nowChecked) diff --git a/engine/app/services/coplan/plans/commit_session.rb b/engine/app/services/coplan/plans/commit_session.rb index 9cd94e60..974dc75d 100644 --- a/engine/app/services/coplan/plans/commit_session.rb +++ b/engine/app/services/coplan/plans/commit_session.rb @@ -65,6 +65,8 @@ def call rebased_ops = [] @session.operations_json.each do |op_data| op_data = op_data.transform_keys(&:to_s) + # "lines" is intentionally absent: this fallback re-resolves against + # *current* content, where a base-revision line box would be wrong. semantic_keys = %w[op old_text new_text heading content needle occurrence replace_all count new_content include_heading] semantic_op = op_data.slice(*semantic_keys) diff --git a/engine/app/services/coplan/plans/position_resolver.rb b/engine/app/services/coplan/plans/position_resolver.rb index 72caf829..a376a173 100644 --- a/engine/app/services/coplan/plans/position_resolver.rb +++ b/engine/app/services/coplan/plans/position_resolver.rb @@ -38,12 +38,25 @@ def resolve_replace_exact occurrence = (@op["occurrence"] || 1).to_i raise OperationError, "replace_exact: occurrence must be >= 1, got #{occurrence}" if occurrence < 1 + line_box = parse_line_box + ranges = find_all_occurrences(old_text) if ranges.empty? raise OperationError, "replace_exact found 0 occurrences of the specified text" end + if line_box + box = char_range_for_lines(*line_box) + total = ranges.length + # Only occurrences fully contained in the box survive; the + # occurrence/replace_all/count rules below then index within it. + ranges = ranges.select { |s, e| s >= box[0] && e <= box[1] } + if ranges.empty? + raise OperationError, "replace_exact found #{total} occurrence(s) of the text, but none within lines #{line_box[0]}..#{line_box[1]}" + end + end + if replace_all Resolution.new(op: "replace_exact", ranges: ranges) else @@ -108,6 +121,49 @@ def resolve_delete_paragraph_containing Resolution.new(op: "delete_paragraph_containing", ranges: ranges) end + # Optional "lines" qualifier: a 1-based inclusive line range ("boxed + # context") that scopes replace_exact matching. Accepts an Integer + # (single-line box) or a two-element [start, end] pair. Multi-line + # old_text is fine as long as the match fits inside the box. + def parse_line_box + raw = @op["lines"] + return nil if raw.nil? + + bounds = raw.is_a?(Array) ? raw : [raw, raw] + unless bounds.length == 2 && bounds.all?(Integer) + raise OperationError, "replace_exact: 'lines' must be an integer line number or a [start, end] pair of integers" + end + + start_line, end_line = bounds + if start_line < 1 || end_line < start_line + raise OperationError, "replace_exact: 'lines' must satisfy 1 <= start <= end, got #{start_line}..#{end_line}" + end + + [start_line, end_line] + end + + # Character range [start, end) covering the given 1-based inclusive + # line range, including the end line's terminator so a match may end + # at the end of that line. + def char_range_for_lines(start_line, end_line) + total = 0 + box_start = nil + box_end = nil + pos = 0 + @content.each_line do |line| + total += 1 + box_start = pos if total == start_line + pos += line.length + box_end = pos if total == end_line + end + + if box_start.nil? || box_end.nil? + raise OperationError, "replace_exact: lines #{start_line}..#{end_line} out of range (document has #{total} lines)" + end + + [box_start, box_end] + end + def find_all_occurrences(text) ranges = [] start_pos = 0 diff --git a/engine/app/views/coplan/agent_instructions/show.text.erb b/engine/app/views/coplan/agent_instructions/show.text.erb index 42b19e1f..4fc3cf68 100644 --- a/engine/app/views/coplan/agent_instructions/show.text.erb +++ b/engine/app/views/coplan/agent_instructions/show.text.erb @@ -290,6 +290,22 @@ Find and replace exact text. Fails if text not found or count exceeded. } ``` +Optional `lines` qualifier: a 1-based inclusive line range scoping the match — an +integer (single line) or a `[start, end]` pair. Use it to disambiguate text that +appears more than once (only occurrences fully inside the range are considered; +`occurrence`/`replace_all`/`count` then apply within it). Multi-line `old_text` +is allowed as long as the match fits inside the range. Fails if the text isn't +found within the range. + +```json +{ + "op": "replace_exact", + "old_text": "- [ ] Write tests", + "new_text": "- [x] Write tests", + "lines": 12 +} +``` + ### insert_under_heading Insert content after a markdown heading. Fails if heading not found or ambiguous. From 83ab9223cfee7aa6d3e71d2542d99427bee06350 Mon Sep 17 00:00:00 2001 From: Bez Hermoso Date: Thu, 9 Jul 2026 13:42:39 -0700 Subject: [PATCH 2/2] Add specs for the lines qualifier and checkbox line targeting Co-Authored-By: Claude Fable 5 --- .../coplan/markdown_helper_checkbox_spec.rb | 36 ++++++ spec/requests/toggle_checkbox_spec.rb | 83 ++++++++++++ spec/services/plans/apply_operations_spec.rb | 24 ++++ spec/services/plans/position_resolver_spec.rb | 120 ++++++++++++++++++ 4 files changed, 263 insertions(+) diff --git a/spec/helpers/coplan/markdown_helper_checkbox_spec.rb b/spec/helpers/coplan/markdown_helper_checkbox_spec.rb index f3dfd56a..ec89f650 100644 --- a/spec/helpers/coplan/markdown_helper_checkbox_spec.rb +++ b/spec/helpers/coplan/markdown_helper_checkbox_spec.rb @@ -96,4 +96,40 @@ expect(nested["data-line-text"]).to eq(" - [ ] Nested child") end end + + describe "#render_markdown data-line attribute" do + it "sets data-line to the 1-based source line number" do + md = "# Heading\n\n- [ ] First\n- [x] Second" + html = helper.render_markdown(md) + doc = Nokogiri::HTML::DocumentFragment.parse(html) + checkboxes = doc.css('input[type="checkbox"]') + expect(checkboxes.map { |cb| cb["data-line"] }).to eq(%w[3 4]) + end + + it "keeps line numbers accurate across fenced code blocks" do + md = "```\n- [ ] Fake checkbox\n```\n- [ ] Real checkbox" + html = helper.render_markdown(md) + doc = Nokogiri::HTML::DocumentFragment.parse(html) + checkboxes = doc.css('input[type="checkbox"]') + expect(checkboxes.length).to eq(1) + expect(checkboxes[0]["data-line"]).to eq("4") + end + + it "keeps line numbers accurate across multiple lists and duplicates" do + md = "- [ ] TODO\n\nSome text\n\n- [ ] TODO\n- [ ] Other" + html = helper.render_markdown(md) + doc = Nokogiri::HTML::DocumentFragment.parse(html) + checkboxes = doc.css('input[type="checkbox"]') + expect(checkboxes.map { |cb| cb["data-line"] }).to eq(%w[1 5 6]) + expect(checkboxes[0]["data-line-text"]).to eq(checkboxes[1]["data-line-text"]) + end + + it "numbers nested tasks by their own source lines" do + md = "- [ ] Parent\n - [ ] Nested child" + html = helper.render_markdown(md) + doc = Nokogiri::HTML::DocumentFragment.parse(html) + nested = doc.css('input[type="checkbox"]').find { |cb| cb["data-line-text"]&.include?("Nested") } + expect(nested["data-line"]).to eq("2") + end + end end diff --git a/spec/requests/toggle_checkbox_spec.rb b/spec/requests/toggle_checkbox_spec.rb index 7ca59e3e..f766533e 100644 --- a/spec/requests/toggle_checkbox_spec.rb +++ b/spec/requests/toggle_checkbox_spec.rb @@ -111,4 +111,87 @@ expect(response).to have_http_status(:ok) end end + + describe "PATCH toggle_checkbox with duplicate task lines" do + before do + plan.current_plan_version.update!( + content_markdown: "# Tasks\n\n- [ ] TODO\n- [ ] TODO\n- [ ] Deploy to staging\n- [ ] Deploy" + ) + end + + it "toggles the checkbox on the given line, leaving its duplicate untouched" do + patch toggle_checkbox_plan_path(plan), params: { + old_text: "- [ ] TODO", + new_text: "- [x] TODO", + base_revision: plan.current_revision, + line: 4 + }, as: :json + + expect(response).to have_http_status(:ok) + plan.reload + expect(plan.current_content).to eq("# Tasks\n\n- [ ] TODO\n- [x] TODO\n- [ ] Deploy to staging\n- [ ] Deploy") + end + + it "toggles a line whose text is a prefix of another task line" do + patch toggle_checkbox_plan_path(plan), params: { + old_text: "- [ ] Deploy", + new_text: "- [x] Deploy", + base_revision: plan.current_revision, + line: 6 + }, as: :json + + expect(response).to have_http_status(:ok) + plan.reload + expect(plan.current_content).to eq("# Tasks\n\n- [ ] TODO\n- [ ] TODO\n- [ ] Deploy to staging\n- [x] Deploy") + end + + it "returns 422 when the text is not on the given line" do + patch toggle_checkbox_plan_path(plan), params: { + old_text: "- [ ] TODO", + new_text: "- [x] TODO", + base_revision: plan.current_revision, + line: 5 + }, as: :json + + expect(response).to have_http_status(:unprocessable_content) + plan.reload + expect(plan.current_content).not_to include("- [x] TODO") + end + + it "returns 422 without a line param when duplicates exist (regression guard)" do + patch toggle_checkbox_plan_path(plan), params: { + old_text: "- [ ] TODO", + new_text: "- [x] TODO", + base_revision: plan.current_revision + }, as: :json + + expect(response).to have_http_status(:unprocessable_content) + end + + it "returns 422 for a non-integer line param" do + patch toggle_checkbox_plan_path(plan), params: { + old_text: "- [ ] TODO", + new_text: "- [x] TODO", + base_revision: plan.current_revision, + line: "abc" + }, as: :json + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body["error"]).to include("line must be a positive integer") + end + + it "records the lines qualifier in the version's operations_json" do + patch toggle_checkbox_plan_path(plan), params: { + old_text: "- [ ] TODO", + new_text: "- [x] TODO", + base_revision: plan.current_revision, + line: 3 + }, as: :json + + expect(response).to have_http_status(:ok) + op = plan.reload.current_plan_version.operations_json.first + expect(op["lines"]).to eq(3) + expect(op["resolved_range"]).to be_present + end + end end diff --git a/spec/services/plans/apply_operations_spec.rb b/spec/services/plans/apply_operations_spec.rb index b7611601..71bf3a14 100644 --- a/spec/services/plans/apply_operations_spec.rb +++ b/spec/services/plans/apply_operations_spec.rb @@ -45,6 +45,30 @@ ) }.to raise_error(CoPlan::Plans::OperationError, /requires 'old_text'/) end + + it "scopes replacement with a lines qualifier and records it in applied data" do + content = "- [ ] TODO\n- [ ] TODO\n- [ ] Other" + result = CoPlan::Plans::ApplyOperations.call( + content: content, + operations: [{ "op" => "replace_exact", "old_text" => "- [ ] TODO", "new_text" => "- [x] TODO", "lines" => 2 }] + ) + expect(result[:content]).to eq("- [ ] TODO\n- [x] TODO\n- [ ] Other") + + applied = result[:applied].first + expect(applied["lines"]).to eq(2) + expect(applied["resolved_range"]).to eq([11, 21]) + expect(applied["new_range"]).to eq([11, 21]) + expect(applied["delta"]).to eq(0) + end + + it "replaces multi-line old_text within a [start, end] lines box" do + content = "intro\nalpha\nbeta\nalpha\nbeta" + result = CoPlan::Plans::ApplyOperations.call( + content: content, + operations: [{ "op" => "replace_exact", "old_text" => "alpha\nbeta", "new_text" => "gamma", "lines" => [4, 5] }] + ) + expect(result[:content]).to eq("intro\nalpha\nbeta\ngamma") + end end describe "insert_under_heading" do diff --git a/spec/services/plans/position_resolver_spec.rb b/spec/services/plans/position_resolver_spec.rb index b8043557..c6aa6837 100644 --- a/spec/services/plans/position_resolver_spec.rb +++ b/spec/services/plans/position_resolver_spec.rb @@ -209,6 +209,126 @@ expect(content[result.ranges.first[0]...result.ranges.first[1]]).to eq("world") end end + + describe "lines qualifier" do + let(:content) { "# Tasks\n- [ ] TODO\nsome text\n- [ ] TODO\n- [ ] Deploy to staging\n- [ ] Deploy" } + + describe "single-line box disambiguates duplicate lines" do + let(:operation) { { op: "replace_exact", old_text: "- [ ] TODO", new_text: "- [x] TODO", lines: 4 } } + + it "resolves only the occurrence on that line" do + result = resolve + expect(result.ranges.length).to eq(1) + range = result.ranges.first + expect(range[0]).to eq(content.index("some text\n- [ ] TODO") + "some text\n".length) + expect(content[range[0]...range[1]]).to eq("- [ ] TODO") + end + end + + describe "prefix collision" do + let(:operation) { { op: "replace_exact", old_text: "- [ ] Deploy", new_text: "- [x] Deploy", lines: 6 } } + + it "resolves the standalone line, not the longer line containing the prefix" do + result = resolve + expect(result.ranges.length).to eq(1) + expect(result.ranges.first[0]).to eq(content.rindex("- [ ] Deploy")) + end + end + + describe "match on last line without trailing newline" do + let(:content) { "- [ ] TODO\n- [ ] TODO" } + let(:operation) { { op: "replace_exact", old_text: "- [ ] TODO", new_text: "- [x] TODO", lines: 2 } } + + it "resolves the last-line occurrence" do + result = resolve + expect(result.ranges.first).to eq([11, 21]) + end + end + + describe "text exists globally but not within the box" do + let(:operation) { { op: "replace_exact", old_text: "- [ ] TODO", new_text: "- [x] TODO", lines: 3 } } + + it "raises with the global count and box" do + expect { resolve }.to raise_error(CoPlan::Plans::OperationError, /found 2 occurrence\(s\) of the text, but none within lines 3\.\.3/) + end + end + + describe "multi-line old_text within a multi-line box" do + let(:operation) { { op: "replace_exact", old_text: "- [ ] TODO\nsome text", new_text: "replaced", lines: [2, 3] } } + + it "resolves the multi-line match" do + result = resolve + range = result.ranges.first + expect(content[range[0]...range[1]]).to eq("- [ ] TODO\nsome text") + end + end + + describe "old_text straddling the box boundary" do + let(:operation) { { op: "replace_exact", old_text: "- [ ] TODO\nsome text", new_text: "replaced", lines: [2, 2] } } + + it "is filtered out and raises" do + expect { resolve }.to raise_error(CoPlan::Plans::OperationError, /none within lines 2\.\.2/) + end + end + + describe "composes with occurrence" do + let(:content) { "foo foo\nfoo foo foo\nfoo" } + let(:operation) { { op: "replace_exact", old_text: "foo", new_text: "bar", lines: 2, occurrence: 2 } } + + it "indexes occurrences within the box" do + result = resolve + expect(result.ranges.first).to eq([12, 15]) + end + end + + describe "composes with replace_all" do + let(:content) { "foo foo\nfoo foo foo\nfoo" } + let(:operation) { { op: "replace_exact", old_text: "foo", new_text: "bar", lines: [2, 2], replace_all: true } } + + it "replaces only in-box occurrences" do + result = resolve + expect(result.ranges).to eq([[8, 11], [12, 15], [16, 19]]) + end + end + + describe "still ambiguous within the box" do + let(:content) { "foo foo\nbar" } + let(:operation) { { op: "replace_exact", old_text: "foo", new_text: "baz", lines: 1 } } + + it "raises the existing ambiguity error" do + expect { resolve }.to raise_error(CoPlan::Plans::OperationError, /found 2 occurrences, expected at most 1/) + end + end + + describe "box beyond end of document" do + let(:operation) { { op: "replace_exact", old_text: "- [ ] TODO", new_text: "- [x] TODO", lines: 99 } } + + it "raises out of range" do + expect { resolve }.to raise_error(CoPlan::Plans::OperationError, /lines 99\.\.99 out of range \(document has 6 lines\)/) + end + end + + describe "invalid values" do + [0, -1, "3", [2], [2, 3, 4], ["2", "3"], [3, 2], { start: 1 }].each do |invalid| + it "rejects #{invalid.inspect}" do + operation = { op: "replace_exact", old_text: "- [ ] TODO", new_text: "- [x] TODO", lines: invalid } + expect { + described_class.call(content: content, operation: operation) + }.to raise_error(CoPlan::Plans::OperationError, /'lines' must/) + end + end + end + + describe "fenced duplicate vs real task line" do + let(:content) { "```\n- [ ] TODO\n```\n- [ ] TODO" } + let(:operation) { { op: "replace_exact", old_text: "- [ ] TODO", new_text: "- [x] TODO", lines: 4 } } + + it "targets the real task line outside the fence" do + result = resolve + expect(result.ranges.first[0]).to eq(content.rindex("- [ ] TODO")) + end + end + end end describe "insert_under_heading" do