From aa11a16ed9c7ced153ed3f023ded95ef01505c39 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 24 Aug 2026 13:35:44 -0500 Subject: [PATCH 1/2] A hand edit stops the agents until they've read it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everywhere else in the concurrency machinery, a stale agent write is merged past whatever landed in between: the operations path transforms its ranges through the intervening versions and applies anyway, and the content path just asks for a fresh base_revision. That's right for agent-vs-agent races. It is wrong when the intervening edit came from a person — someone who opened the editor and changed the words by hand is the highest-authority input the document gets, and an agent that never saw those words can silently undo them. So human versions are now a fence. If a `human` revision landed after this credential last read the plan, every agent write is refused with a 409 (`code: human_edit_pending`) carrying the person's own diff, their name, and their change summary — an agent told only "you are stale" re-derives; an agent shown the words a person chose can keep them. Proof of reading is a receipt, not an assertion. `base_revision` is a number the caller supplies, so an agent handed a 409 naming the current revision could echo it back and clobber the edit on the retry — the exact accident this is here to prevent. The content-returning endpoints now record a PlanRead per credential, and only a real fetch lifts the fence. A credential that has never read the plan is behind it too, which is the blind-overwrite case. Guarded at all four agent write paths: PUT /content, POST /operations (before any rebase machinery runs), session create, and session commit. Web-UI human edits are untouched; agent-vs-agent staleness keeps the OT rebase it has today. Test churn worth reading rather than skimming: the plan factory builds revision 1 as a human version, so 34 existing specs were writing over an unread human edit and now read first via `agent_has_read`. Separately, the OT-rebase specs in sessions_spec were creating their *intervening* versions as "human" while testing agent-vs-agent merging — those are now local_agent, which is what they always meant. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 1 + app/admin/plan_reads.rb | 41 +++ ...120000_create_coplan_plan_reads.co_plan.rb | 22 ++ db/schema.rb | 15 +- .../coplan/api/v1/base_controller.rb | 40 +++ .../coplan/api/v1/content_controller.rb | 8 + .../coplan/api/v1/operations_controller.rb | 7 + .../coplan/api/v1/plans_controller.rb | 5 + .../coplan/api/v1/sessions_controller.rb | 8 + engine/app/models/coplan/plan.rb | 1 + engine/app/models/coplan/plan_read.rb | 59 ++++ .../services/coplan/plans/human_edit_guard.rb | 156 ++++++++++ .../coplan/agent_instructions/show.text.erb | 37 ++- ...20260824000000_create_coplan_plan_reads.rb | 21 ++ spec/requests/api/v1/content_spec.rb | 8 +- spec/requests/api/v1/human_edit_guard_spec.rb | 283 ++++++++++++++++++ spec/requests/api/v1/operations_spec.rb | 3 + spec/requests/api/v1/sessions_spec.rb | 15 +- spec/support/agent_read_helpers.rb | 19 ++ 19 files changed, 741 insertions(+), 8 deletions(-) create mode 100644 app/admin/plan_reads.rb create mode 100644 db/migrate/20260824120000_create_coplan_plan_reads.co_plan.rb create mode 100644 engine/app/models/coplan/plan_read.rb create mode 100644 engine/app/services/coplan/plans/human_edit_guard.rb create mode 100644 engine/db/migrate/20260824000000_create_coplan_plan_reads.rb create mode 100644 spec/requests/api/v1/human_edit_guard_spec.rb create mode 100644 spec/support/agent_read_helpers.rb diff --git a/AGENTS.md b/AGENTS.md index 9e776935..b2b0bd7f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -123,6 +123,7 @@ it is. - **Editing model**: humans comment, AI agents apply edits via semantic operations (`replace_exact`, `insert_under_heading`, `delete_paragraph_containing`) - **Edit leases**: one agent edits at a time, enforced by a lease with TTL - **Versions are immutable** — every edit creates a new PlanVersion with full provenance +- **A person's hand edit is a fence, not a merge input** — agent-vs-agent staleness gets rebased through intervening versions (OT), but a `human` version blocks every agent write with a 409 (`code: human_edit_pending`) carrying the human's diff, until that credential has actually re-read the plan. Proof of reading is a `PlanRead` receipt, not the caller's `base_revision`. See `Plans::HumanEditGuard`. ## Comment & Review UX diff --git a/app/admin/plan_reads.rb b/app/admin/plan_reads.rb new file mode 100644 index 00000000..f5c27504 --- /dev/null +++ b/app/admin/plan_reads.rb @@ -0,0 +1,41 @@ +# Read receipts. Mostly here to answer one support question: "why is my +# agent getting human_edit_pending?" — compare last_seen_revision against +# the plan's most recent human version. +ActiveAdmin.register CoPlan::PlanRead, as: "PlanRead" do + actions :index, :show + + filter :plan + filter :reader_type, as: :select, collection: CoPlan::PlanRead::READER_TYPES + filter :reader_id + filter :last_seen_revision + filter :last_seen_at + + index do + selectable_column + id_column + column :plan + column :reader_type + column :reader_id + column :last_seen_revision + column("Plan revision") { |read| read.plan.current_revision } + column :last_seen_at + actions + end + + show do + attributes_table do + row :id + row :plan + row :reader_type + row :reader_id + row :last_seen_revision + row("Plan revision") { |read| read.plan.current_revision } + row("Last human revision") do |read| + read.plan.plan_versions.where(actor_type: "human").maximum(:revision) + end + row :last_seen_at + row :created_at + row :updated_at + end + end +end diff --git a/db/migrate/20260824120000_create_coplan_plan_reads.co_plan.rb b/db/migrate/20260824120000_create_coplan_plan_reads.co_plan.rb new file mode 100644 index 00000000..4d57321a --- /dev/null +++ b/db/migrate/20260824120000_create_coplan_plan_reads.co_plan.rb @@ -0,0 +1,22 @@ +# This migration comes from co_plan (originally 20260824000000) +class CreateCoplanPlanReads < ActiveRecord::Migration[8.1] + # Read receipts: the highest revision of a plan a given credential has + # actually fetched content for. This is what makes "the agent hasn't + # seen the human's edit yet" a fact the server can check, rather than a + # number the caller asserts. See Plans::HumanEditGuard. + def change + create_table :coplan_plan_reads, id: { type: :string, limit: 36 } do |t| + t.string :plan_id, limit: 36, null: false + # "api_token" for agents, "user" for hook-authenticated humans. + t.string :reader_type, null: false + t.string :reader_id, limit: 36, null: false + t.integer :last_seen_revision, null: false, default: 0 + t.datetime :last_seen_at, null: false + t.timestamps + end + + add_index :coplan_plan_reads, [ :plan_id, :reader_type, :reader_id ], + unique: true, name: "index_coplan_plan_reads_on_plan_and_reader" + add_foreign_key :coplan_plan_reads, :coplan_plans, column: :plan_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 5322621f..40d2057c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_21_205749) do +ActiveRecord::Schema[8.1].define(version: 2026_08_24_120000) do create_table "active_admin_comments", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.bigint "author_id" t.string "author_type" @@ -279,6 +279,17 @@ t.index ["plan_id"], name: "index_coplan_plan_placements_on_plan_id", unique: true end + create_table "coplan_plan_reads", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| + t.datetime "created_at", null: false + t.datetime "last_seen_at", null: false + t.integer "last_seen_revision", default: 0, null: false + t.string "plan_id", limit: 36, null: false + t.string "reader_id", limit: 36, null: false + t.string "reader_type", null: false + t.datetime "updated_at", null: false + t.index ["plan_id", "reader_type", "reader_id"], name: "index_coplan_plan_reads_on_plan_and_reader", unique: true + end + create_table "coplan_plan_tags", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.datetime "created_at", null: false t.string "plan_id", limit: 36, null: false @@ -442,7 +453,6 @@ t.index ["user_id"], name: "index_coplan_web_push_subscriptions_on_user_id" end - add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" add_foreign_key "coplan_agent_events", "coplan_api_tokens", column: "api_token_id" add_foreign_key "coplan_agent_events", "coplan_plans", column: "plan_id" @@ -478,6 +488,7 @@ add_foreign_key "coplan_plan_placements", "coplan_libraries", column: "library_id" add_foreign_key "coplan_plan_placements", "coplan_plans", column: "plan_id" add_foreign_key "coplan_plan_placements", "coplan_users", column: "placed_by_user_id" + add_foreign_key "coplan_plan_reads", "coplan_plans", column: "plan_id" add_foreign_key "coplan_plan_tags", "coplan_plans", column: "plan_id" add_foreign_key "coplan_plan_tags", "coplan_tags", column: "tag_id" add_foreign_key "coplan_plan_versions", "coplan_api_tokens", column: "api_token_id" diff --git a/engine/app/controllers/coplan/api/v1/base_controller.rb b/engine/app/controllers/coplan/api/v1/base_controller.rb index 7c96d0be..2b33886b 100644 --- a/engine/app/controllers/coplan/api/v1/base_controller.rb +++ b/engine/app/controllers/coplan/api/v1/base_controller.rb @@ -118,6 +118,46 @@ def api_token_id @api_token&.id end + # Which credential is reading/writing, for read receipts. Distinct + # from api_actor_id only in being explicit about the namespace: a + # token id and a user id are both UUIDs, and a receipt earned by one + # must never satisfy the other. + def api_reader_type + @api_token ? "api_token" : "user" + end + + def api_reader_id + api_actor_id + end + + # Call from any endpoint that hands the caller a plan's content. + # This is the only way to earn the right to write over a human's + # edit — see Plans::HumanEditGuard. + def record_plan_read!(plan, revision: nil) + CoPlan::PlanRead.record!( + plan: plan, + reader_type: api_reader_type, + reader_id: api_reader_id, + revision: revision || plan.current_revision + ) + end + + # A hand-written human edit is a hard stop for agents until they've + # pulled it. Hook-authenticated callers are the human themselves, so + # the fence doesn't apply to them. + def guard_human_edits! + return unless @plan + return if api_author_type == "human" + + block = CoPlan::Plans::HumanEditGuard.call( + plan: @plan, + reader_type: api_reader_type, + reader_id: api_reader_id, + base_revision: params[:base_revision].presence&.to_i + ) + render json: block, status: :conflict if block + end + def set_plan @plan = CoPlan::Plan.find_by(id: params[:plan_id] || params[:id]) unless @plan diff --git a/engine/app/controllers/coplan/api/v1/content_controller.rb b/engine/app/controllers/coplan/api/v1/content_controller.rb index a840f347..c383a91a 100644 --- a/engine/app/controllers/coplan/api/v1/content_controller.rb +++ b/engine/app/controllers/coplan/api/v1/content_controller.rb @@ -11,9 +11,15 @@ module V1 # # Optimistic concurrency: caller MUST supply base_revision matching # the plan's current_revision, or the request fails with 409. + # + # Human edits go further than that: if a person has edited by hand + # since this credential last read the plan, the write is refused with + # their diff attached and no amount of base_revision bumping gets + # past it — see Plans::HumanEditGuard. class ContentController < BaseController before_action :set_plan before_action :authorize_plan_access! + before_action :guard_human_edits! def update if params[:content].nil? @@ -57,6 +63,8 @@ def update end version = result[:version] + # The caller has seen this content — it just wrote it. + record_plan_read!(@plan, revision: version.revision) render json: { revision: version.revision, content_sha256: version.content_sha256, diff --git a/engine/app/controllers/coplan/api/v1/operations_controller.rb b/engine/app/controllers/coplan/api/v1/operations_controller.rb index 4c46e314..470f013f 100644 --- a/engine/app/controllers/coplan/api/v1/operations_controller.rb +++ b/engine/app/controllers/coplan/api/v1/operations_controller.rb @@ -4,6 +4,10 @@ module V1 class OperationsController < BaseController before_action :set_plan before_action :authorize_plan_access! + # A stale agent write gets rebased through intervening versions + # below. A stale write over a *human's* hand edit does not — it is + # refused here, before any of that machinery runs. + before_action :guard_human_edits! def create operations = params[:operations] @@ -228,6 +232,9 @@ def commit_version(current_content, result) @plan.comment_threads.mark_out_of_date_for_new_version!(version) + # The caller has seen this content — it just wrote it. + record_plan_read!(@plan, revision: new_revision) + broadcast_plan_update render json: { diff --git a/engine/app/controllers/coplan/api/v1/plans_controller.rb b/engine/app/controllers/coplan/api/v1/plans_controller.rb index 3046b410..7be4f7db 100644 --- a/engine/app/controllers/coplan/api/v1/plans_controller.rb +++ b/engine/app/controllers/coplan/api/v1/plans_controller.rb @@ -24,7 +24,11 @@ def index render json: plans.map { |p| plan_json(p) } end + # Handing over the content is what earns a read receipt, which is + # what lifts a Plans::HumanEditGuard block. Recorded here and in + # #snapshot — the two endpoints that return current_content. def show + record_plan_read!(@plan) render json: plan_json(@plan).merge( current_content: @plan.current_content, current_revision: @plan.current_revision, @@ -283,6 +287,7 @@ def comments end def snapshot + record_plan_read!(@plan) threads = @plan.comment_threads.includes(:comments, :created_by_user).order(created_at: :desc) references = @plan.references.order(created_at: :desc) collaborators = @plan.plan_collaborators.includes(:user) diff --git a/engine/app/controllers/coplan/api/v1/sessions_controller.rb b/engine/app/controllers/coplan/api/v1/sessions_controller.rb index 415a5ca6..16c2444d 100644 --- a/engine/app/controllers/coplan/api/v1/sessions_controller.rb +++ b/engine/app/controllers/coplan/api/v1/sessions_controller.rb @@ -5,6 +5,12 @@ class SessionsController < BaseController before_action :set_plan before_action :authorize_plan_access! before_action :set_session, only: [ :show, :commit ] + # Checked twice on purpose: at #create so an agent can't start + # accumulating operations against content it has never seen, and + # again at #commit because a person may have edited by hand while + # the session was open. Neither is a rebase — a human's hand edit + # stops the write until the agent pulls it. + before_action :guard_human_edits!, only: [ :create, :commit ] # POST /api/v1/plans/:plan_id/sessions # Cloud personas create sessions via direct Ruby service calls, not this endpoint. @@ -55,6 +61,8 @@ def commit response[:revision] = result[:version].revision response[:version_id] = result[:version].id response[:content_sha256] = result[:version].content_sha256 + # The caller has seen this content — it just wrote it. + record_plan_read!(@plan, revision: result[:version].revision) end render json: response diff --git a/engine/app/models/coplan/plan.rb b/engine/app/models/coplan/plan.rb index 48e5bbc2..864724f0 100644 --- a/engine/app/models/coplan/plan.rb +++ b/engine/app/models/coplan/plan.rb @@ -42,6 +42,7 @@ class Plan < ApplicationRecord has_many :plan_tags, dependent: :destroy has_many :tags, through: :plan_tags, source: :tag has_many :plan_viewers, dependent: :destroy + has_many :plan_reads, dependent: :destroy has_many :notifications, dependent: :destroy has_many :references, dependent: :destroy has_many_attached :attachments diff --git a/engine/app/models/coplan/plan_read.rb b/engine/app/models/coplan/plan_read.rb new file mode 100644 index 00000000..fccfd03d --- /dev/null +++ b/engine/app/models/coplan/plan_read.rb @@ -0,0 +1,59 @@ +module CoPlan + # A read receipt: proof that a particular credential has actually pulled + # a plan's content at a particular revision. + # + # Why this exists: `base_revision` is an assertion, not evidence. An agent + # that gets a 409 telling it the plan is now at revision 7 can re-send its + # unchanged body with `base_revision: 7` and wipe out whatever the human + # wrote — the very accident Plans::HumanEditGuard is there to prevent. A + # receipt can only be earned by fetching the content, so "have you seen + # the human's edit?" becomes a question the server can answer. + # + # Keyed per credential, not per person: an agent that mints a fresh token + # for each run starts with no receipts and must read before it writes, + # which is step one of the documented workflow anyway. + class PlanRead < ApplicationRecord + READER_TYPES = %w[api_token user].freeze + + belongs_to :plan + + validates :reader_type, presence: true, inclusion: { in: READER_TYPES } + validates :reader_id, presence: true + validates :last_seen_revision, presence: true + + # Records that `reader` has seen `revision`. Monotonic: a later read of + # an older revision (a version fetch, a cached response) never walks the + # receipt backwards. + def self.record!(plan:, reader_type:, reader_id:, revision:) + return nil if reader_type.blank? || reader_id.blank? || revision.blank? + + record = find_or_initialize_by(plan_id: plan.id, reader_type: reader_type, reader_id: reader_id) + return record if record.persisted? && record.last_seen_revision >= revision.to_i + + record.last_seen_revision = revision.to_i + record.last_seen_at = Time.current + record.save! + record + rescue ActiveRecord::RecordNotUnique + retry + end + + # Highest revision this credential has seen. Zero means "never read it", + # which is deliberately indistinguishable from "read it before anything + # existed" — both mean the reader can't have seen a human's edit. + def self.revision_for(plan:, reader_type:, reader_id:) + return 0 if reader_type.blank? || reader_id.blank? + + where(plan_id: plan.id, reader_type: reader_type, reader_id: reader_id) + .pick(:last_seen_revision) || 0 + end + + def self.ransackable_attributes(auth_object = nil) + %w[id plan_id reader_type reader_id last_seen_revision last_seen_at created_at updated_at] + end + + def self.ransackable_associations(auth_object = nil) + %w[plan] + end + end +end diff --git a/engine/app/services/coplan/plans/human_edit_guard.rb b/engine/app/services/coplan/plans/human_edit_guard.rb new file mode 100644 index 00000000..147df2d4 --- /dev/null +++ b/engine/app/services/coplan/plans/human_edit_guard.rb @@ -0,0 +1,156 @@ +module CoPlan + module Plans + # Human edits are a fence, not a merge input. + # + # The rest of the concurrency machinery treats every revision the same: + # a stale agent write gets transformed through whatever landed in + # between (Plans::TransformRange) and applied anyway. That's the right + # behavior for agent-vs-agent races — two agents editing different + # paragraphs shouldn't block each other. + # + # It is the wrong behavior when the intervening edit came from a person. + # Someone opening the editor and changing the words by hand is the + # highest-authority input the document gets, and an agent that never saw + # those words can silently undo them — rewriting the paragraph the human + # just fixed, restoring the sentence they just cut. So: if a human has + # edited since this caller last actually read the plan, every agent + # write is refused until the caller pulls the new content. + # + # "Actually read" means a read receipt (CoPlan::PlanRead), not a + # `base_revision` the caller asserts — an agent handed a 409 that names + # the current revision could otherwise just echo the number back and + # clobber the edit on the retry. + # + # The refusal carries the human's diff. An agent that's told only "you + # are stale" re-reads and re-derives; an agent that's shown the words a + # person chose can keep them. + class HumanEditGuard + # Blocking is only useful if the message is actionable, and a diff is + # the actionable part — but a wholesale rewrite of a long plan would + # otherwise dump the entire document into an error body. + MAX_DIFF_CHARS = 6_000 + MAX_TOTAL_DIFF_CHARS = 12_000 + # Beyond this, listing every edit stops helping; the caller needs to + # re-read the plan regardless. + MAX_EDITS_LISTED = 10 + + # Returns nil when the write may proceed, or a JSON-ready Hash + # describing the block (render it with status :conflict). + def self.call(plan:, reader_type:, reader_id:, base_revision: nil) + new(plan: plan, reader_type: reader_type, reader_id: reader_id, base_revision: base_revision).call + end + + def initialize(plan:, reader_type:, reader_id:, base_revision: nil) + @plan = plan + @reader_type = reader_type + @reader_id = reader_id + @base_revision = base_revision + end + + # The common case is "no human has touched this plan since you read + # it", and it has to stay cheap: one aggregate over the version index, + # no rows loaded. Only a genuine block pays for fetching versions. + def call + last_human_revision = @plan.plan_versions.where(actor_type: "human").maximum(:revision) + return nil if last_human_revision.nil? + return nil if seen_revision >= last_human_revision + + conflict_payload(last_human_revision, unseen_human_versions) + end + + private + + # content_markdown is a MEDIUMTEXT per row and we never look at it — + # the diff is what the caller needs. Select around it. + def unseen_human_versions + @plan.plan_versions + .where(actor_type: "human") + .where("revision > ?", seen_revision) + .order(revision: :asc) + .select(:id, :plan_id, :revision, :actor_id, :change_summary, :diff_unified, :created_at) + .includes(:actor_user) + .to_a + end + + def seen_revision + @seen_revision ||= PlanRead.revision_for( + plan: @plan, reader_type: @reader_type, reader_id: @reader_id + ) + end + + def conflict_payload(last_human_revision, unseen) + listed = unseen.last(MAX_EDITS_LISTED) + + { + error: error_message(unseen), + code: "human_edit_pending", + current_revision: @plan.current_revision, + last_human_revision: last_human_revision, + last_seen_revision: seen_revision, + base_revision: @base_revision, + human_edits: with_diff_budget(listed), + human_edits_omitted: (unseen.length - listed.length).presence, + resolve: resolve_instructions + }.compact + end + + def error_message(unseen) + editors = unseen.filter_map { |v| v.actor_user&.name }.uniq + who = if editors.length == 1 + editors.first + elsif editors.length > 1 + "#{editors[0..-2].join(", ")} and #{editors.last}" + else + "Someone" + end + + count = unseen.length + edits = count == 1 ? "an edit" : "#{count} edits" + seen = seen_revision.zero? ? "you have never read this plan" : "you last read v#{seen_revision}" + + "Blocked: #{who} edited this plan by hand (#{edits}, now at v#{@plan.current_revision}) and " \ + "#{seen}. A person's direct edit outranks an agent's — it is not something to merge past. " \ + "Read the current content, keep their changes, fold your own work in around them, then write again." + end + + # Diffs are included newest-first so the most recent human intent + # survives the budget; the list is re-sorted into revision order for + # reading. + def with_diff_budget(versions) + budget = MAX_TOTAL_DIFF_CHARS + by_revision = {} + + versions.reverse_each do |version| + diff, budget = clip_diff(version.diff_unified, budget) + by_revision[version.revision] = { + revision: version.revision, + editor: version.actor_user&.name, + edited_at: version.created_at, + change_summary: version.change_summary, + diff: diff + }.compact + end + + by_revision.keys.sort.map { |rev| by_revision[rev] } + end + + def clip_diff(diff, budget) + return [ nil, budget ] if diff.blank? || budget <= 0 + + limit = [ MAX_DIFF_CHARS, budget ].min + if diff.length <= limit + [ diff, budget - diff.length ] + else + [ "#{diff[0, limit]}\n… diff truncated — read the plan for the full text.", budget - limit ] + end + end + + def resolve_instructions + base = CoPlan::Engine.routes.url_helpers.snapshot_api_v1_plan_path(@plan) + "GET #{base} to pull the current content (that read is what lifts this block), " \ + "then re-send your write with base_revision=#{@plan.current_revision}. " \ + "Do not re-send your previous body unchanged — it predates the human's edit." + 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 0f02c44d..17ee0ae9 100644 --- a/engine/app/views/coplan/agent_instructions/show.text.erb +++ b/engine/app/views/coplan/agent_instructions/show.text.erb @@ -32,6 +32,7 @@ Keeping a local Markdown file and syncing whole documents is the intended workfl 2. **Edit** the local file however you like. 3. **Write** — `PUT <%= @base %>/api/v1/plans/$PLAN_ID/content` with the full new content, `base_revision` set to the revision you read, and a specific `change_summary` (it becomes the version's label in the plan history — "Tightened rollout plan per Sam's feedback", not "Update"). 4. **On `409 Conflict`** someone edited in between: re-read the snapshot, re-apply your changes to the fresh content, and PUT again with the new revision. Never retry with the stale `base_revision`. +5. **On `409` with `"code": "human_edit_pending"`** a person edited the plan by hand and you haven't read it since. Their diff is in the response. Read it, keep what they wrote, and only then write again — see [When a person has edited by hand](#when-a-person-has-edited-by-hand). The server diffs your content against the current revision, records granular operations automatically, and preserves comment anchors in unchanged regions. Re-read before every editing session — don't trust a local copy that's more than a few minutes old, and don't keep long-lived local state between tasks. @@ -427,10 +428,44 @@ 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) +- `409 Conflict` with `{"code": "human_edit_pending"}` if a person edited by hand since you last read the plan (see below) - `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. +### When a person has edited by hand + +A direct human edit is the highest-authority input a plan gets. Agent-vs-agent races get merged for you; a human's edit does not — **every agent write is refused until you have pulled their words**: + +```json +{ + "error": "Blocked: Sam edited this plan by hand (an edit, now at v7) and you last read v5. A person's direct edit outranks an agent's — it is not something to merge past. Read the current content, keep their changes, fold your own work in around them, then write again.", + "code": "human_edit_pending", + "current_revision": 7, + "last_human_revision": 7, + "last_seen_revision": 5, + "human_edits": [ + { + "revision": 7, + "editor": "Sam", + "edited_at": "2026-08-24T17:02:11Z", + "change_summary": "Edited in web UI", + "diff": "@@ -12,3 +12,3 @@\n-Ship behind a flag in Q4.\n+Ship behind a flag in Q3 — legal signed off on the earlier date.\n" + } + ], + "resolve": "GET /api/v1/plans/:id/snapshot to pull the current content …" +} +``` + +What to do: + +1. **Read the diff in the response.** It is the person's own wording, and it is what they meant. +2. **`GET .../snapshot`** to pull the current content. That read is what lifts the block — the server tracks which revision your credential has actually fetched, so bumping `base_revision` to the number in the error does *not* get you through. +3. **Rebuild your edit on top of theirs.** Do not re-send the body you already had; it predates their change and re-sending it deletes their words. +4. If your instructions and their edit genuinely conflict, **leave their text alone and raise it in a comment** (`POST /api/v1/plans/:id/comments`) rather than overwriting a decision a person made deliberately. + +This applies to every agent write path — content replacement, operations, and session commits. It does not apply to a plan nobody has hand-edited. + ## 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. @@ -646,5 +681,5 @@ For approved changes, the recommended path is: read the snapshot → edit the ma | 401 | Not authenticated | | 403 | Not authorized for this action | | 404 | Plan not found (or no access) | -| 409 | Edit lease conflict or stale revision | +| 409 | Edit lease conflict, stale revision, or an unread human edit (`code: human_edit_pending`) | | 422 | Validation error or operation failed | diff --git a/engine/db/migrate/20260824000000_create_coplan_plan_reads.rb b/engine/db/migrate/20260824000000_create_coplan_plan_reads.rb new file mode 100644 index 00000000..a1987e65 --- /dev/null +++ b/engine/db/migrate/20260824000000_create_coplan_plan_reads.rb @@ -0,0 +1,21 @@ +class CreateCoplanPlanReads < ActiveRecord::Migration[8.1] + # Read receipts: the highest revision of a plan a given credential has + # actually fetched content for. This is what makes "the agent hasn't + # seen the human's edit yet" a fact the server can check, rather than a + # number the caller asserts. See Plans::HumanEditGuard. + def change + create_table :coplan_plan_reads, id: { type: :string, limit: 36 } do |t| + t.string :plan_id, limit: 36, null: false + # "api_token" for agents, "user" for hook-authenticated humans. + t.string :reader_type, null: false + t.string :reader_id, limit: 36, null: false + t.integer :last_seen_revision, null: false, default: 0 + t.datetime :last_seen_at, null: false + t.timestamps + end + + add_index :coplan_plan_reads, [ :plan_id, :reader_type, :reader_id ], + unique: true, name: "index_coplan_plan_reads_on_plan_and_reader" + add_foreign_key :coplan_plan_reads, :coplan_plans, column: :plan_id + end +end diff --git a/spec/requests/api/v1/content_spec.rb b/spec/requests/api/v1/content_spec.rb index 1a132a97..eb3396c5 100644 --- a/spec/requests/api/v1/content_spec.rb +++ b/spec/requests/api/v1/content_spec.rb @@ -17,7 +17,13 @@ p end - before { alice_token } + before do + alice_token + # Revision 1 is a human version, so the agent has to have read the plan + # before it may write. See the human-edit fence specs for the fence + # itself; these are about everything else. + agent_has_read(plan, alice_token) + end def put_content(body, params: {}) payload = { base_revision: plan.current_revision, content: body }.merge(params) diff --git a/spec/requests/api/v1/human_edit_guard_spec.rb b/spec/requests/api/v1/human_edit_guard_spec.rb new file mode 100644 index 00000000..8453aa19 --- /dev/null +++ b/spec/requests/api/v1/human_edit_guard_spec.rb @@ -0,0 +1,283 @@ +require "rails_helper" + +# A person editing a plan by hand is the highest-authority input the +# document gets. Everywhere else in the concurrency machinery a stale agent +# write gets transformed through intervening versions and applied anyway; +# here it doesn't. These specs pin the difference. +RSpec.describe "Human edit fence", 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\nShip behind a flag in Q4.\n\nOwner: Sam.\n" } + let!(:plan) do + p = CoPlan::Plan.create!(title: "Rollout", created_by_user: alice) + v = CoPlan::PlanVersion.create!( + plan: p, revision: 1, + content_markdown: initial_content, + actor_type: "local_agent", actor_id: alice.id, + operations_json: [] + ) + p.update!(current_plan_version: v, current_revision: 1) + p + end + + before { alice_token } + + # The manual edit a person would make in the web editor. + def hand_edit!(from: nil, to: nil, summary: "Edited in web UI", actor: alice) + CoPlan::Plans::ReplaceContent.call( + plan: plan.reload, + new_content: plan.current_content.sub(from, to), + base_revision: plan.current_revision, + actor_type: "human", + actor_id: actor.id, + change_summary: summary + ) + end + + def put_content(body, revision: nil) + put api_v1_plan_content_path(plan), + params: { base_revision: revision || plan.reload.current_revision, content: body }, + headers: headers, as: :json + end + + describe "PUT /content" do + it "lets an agent write when nobody has hand-edited the plan" do + agent_has_read(plan, alice_token) + + put_content(initial_content + "\nAgent addendum.\n") + + expect(response).to have_http_status(:created) + end + + it "refuses the write when a person edited after the agent last read" do + agent_has_read(plan, alice_token) + hand_edit!(from: "Q4", to: "Q3 — legal signed off on the earlier date") + + expect { + put_content(initial_content + "\nAgent addendum.\n") + }.not_to change(CoPlan::PlanVersion, :count) + + expect(response).to have_http_status(:conflict) + body = JSON.parse(response.body) + expect(body["code"]).to eq("human_edit_pending") + expect(body["last_human_revision"]).to eq(2) + expect(body["last_seen_revision"]).to eq(1) + expect(plan.reload.current_content).to include("Q3 — legal signed off") + end + + it "hands back the human's diff so the agent can keep their wording" do + agent_has_read(plan, alice_token) + hand_edit!(from: "Q4", to: "Q3", summary: "Legal cleared the earlier date") + + put_content(initial_content) + + body = JSON.parse(response.body) + edit = body["human_edits"].sole + expect(edit["revision"]).to eq(2) + expect(edit["editor"]).to eq(alice.name) + expect(edit["change_summary"]).to eq("Legal cleared the earlier date") + expect(edit["diff"]).to include("Q3") + expect(edit["diff"]).to include("Q4") + expect(body["error"]).to include(alice.name) + expect(body["resolve"]).to include("snapshot") + end + + # The whole point of tracking reads rather than trusting base_revision: + # the 409 tells the agent the current revision, and an agent that just + # echoes it back would otherwise wipe out the edit it never saw. + it "is not satisfied by bumping base_revision to the number in the error" do + agent_has_read(plan, alice_token) + hand_edit!(from: "Q4", to: "Q3") + + put_content(initial_content, revision: 2) + + expect(response).to have_http_status(:conflict) + expect(JSON.parse(response.body)["code"]).to eq("human_edit_pending") + expect(plan.reload.current_content).to include("Q3") + end + + it "lifts once the agent actually reads the plan" do + agent_has_read(plan, alice_token) + hand_edit!(from: "Q4", to: "Q3") + put_content(initial_content) + expect(response).to have_http_status(:conflict) + + get snapshot_api_v1_plan_path(plan), headers: headers, as: :json + expect(response).to have_http_status(:ok) + current = JSON.parse(response.body)["current_content"] + + put_content(current + "\nAgent addendum.\n") + + expect(response).to have_http_status(:created) + expect(plan.reload.current_content).to include("Q3") + expect(plan.current_content).to include("Agent addendum.") + end + + it "blocks a credential that has never read the plan at all" do + hand_edit!(from: "Q4", to: "Q3") + + put_content(initial_content) + + expect(response).to have_http_status(:conflict) + body = JSON.parse(response.body) + expect(body["last_seen_revision"]).to eq(0) + expect(body["error"]).to include("never read this plan") + end + + # Receipts are per credential, not per person: a second agent running on + # its own token hasn't seen anything just because the first one did. + it "does not let one agent's read clear the fence for another" do + other_token = create(:api_token, user: alice, raw_token: "test-token-other") + agent_has_read(plan, alice_token) + hand_edit!(from: "Q4", to: "Q3") + get snapshot_api_v1_plan_path(plan), headers: headers, as: :json + + put api_v1_plan_content_path(plan), + params: { base_revision: plan.reload.current_revision, content: initial_content }, + headers: { "Authorization" => "Bearer test-token-other" }, as: :json + + expect(response).to have_http_status(:conflict) + expect(JSON.parse(response.body)["code"]).to eq("human_edit_pending") + expect(other_token.reload).to be_present + end + + it "reports every unseen human edit, not just the last one" do + agent_has_read(plan, alice_token) + hand_edit!(from: "Q4", to: "Q3") + hand_edit!(from: "Owner: Sam.", to: "Owner: Sam and Dana.") + + put_content(initial_content) + + body = JSON.parse(response.body) + expect(body["human_edits"].map { |e| e["revision"] }).to eq([ 2, 3 ]) + expect(body["error"]).to include("2 edits") + end + + it "ignores human edits the agent has already seen" do + hand_edit!(from: "Q4", to: "Q3") + agent_has_read(plan, alice_token) + + put_content(plan.reload.current_content + "\nAgent addendum.\n") + + expect(response).to have_http_status(:created) + end + + # Agent-vs-agent staleness keeps its existing behavior: the operations + # path rebases, the content path asks for a re-read. Neither is a fence. + it "leaves another agent's edit to the ordinary stale-revision path" do + agent_has_read(plan, alice_token) + CoPlan::Plans::ReplaceContent.call( + plan: plan, new_content: initial_content.sub("Q4", "Q2"), + base_revision: 1, actor_type: "local_agent", actor_id: alice.id + ) + + put_content(initial_content, revision: 1) + + expect(response).to have_http_status(:conflict) + body = JSON.parse(response.body) + expect(body["code"]).to be_nil + expect(body["error"]).to match(/Stale/) + end + end + + describe "POST /operations" do + it "refuses to rebase over an unread human edit" do + agent_has_read(plan, alice_token) + hand_edit!(from: "Q4", to: "Q3") + + expect { + post api_v1_plan_operations_path(plan), + params: { + base_revision: 1, + operations: [ { op: "replace_exact", old_text: "Owner: Sam.", new_text: "Owner: Dana.", count: 1 } ] + }, + headers: headers, as: :json + }.not_to change(CoPlan::PlanVersion, :count) + + expect(response).to have_http_status(:conflict) + expect(JSON.parse(response.body)["code"]).to eq("human_edit_pending") + end + end + + describe "sessions" do + it "refuses to open a session against content the agent has not read" do + hand_edit!(from: "Q4", to: "Q3") + + expect { + post api_v1_plan_sessions_path(plan), headers: headers, as: :json + }.not_to change(CoPlan::EditSession, :count) + + expect(response).to have_http_status(:conflict) + expect(JSON.parse(response.body)["code"]).to eq("human_edit_pending") + end + + it "refuses to commit a session a person edited underneath" do + agent_has_read(plan, alice_token) + post api_v1_plan_sessions_path(plan), headers: headers, as: :json + session_id = JSON.parse(response.body)["id"] + + post api_v1_plan_operations_path(plan), + params: { + session_id: session_id, base_revision: 1, + operations: [ { op: "replace_exact", old_text: "Owner: Sam.", new_text: "Owner: Dana.", count: 1 } ] + }, headers: headers, as: :json + expect(response).to have_http_status(:created) + + hand_edit!(from: "Q4", to: "Q3") + + expect { + post commit_api_v1_plan_session_path(plan, session_id), headers: headers, as: :json + }.not_to change(CoPlan::PlanVersion, :count) + + expect(response).to have_http_status(:conflict) + expect(JSON.parse(response.body)["code"]).to eq("human_edit_pending") + expect(plan.reload.current_content).to include("Owner: Sam.") + end + end + + describe "read receipts" do + it "records the revision handed over by GET /plans/:id" do + get api_v1_plan_path(plan), headers: headers, as: :json + + expect(receipt_revision).to eq(1) + end + + it "records the revision an agent writes, since it has seen that content" do + agent_has_read(plan, alice_token) + put_content(initial_content + "\nAgent addendum.\n") + + expect(receipt_revision).to eq(2) + end + + it "never walks backwards" do + agent_has_read(plan, alice_token, revision: 5) + get api_v1_plan_path(plan), headers: headers, as: :json + + expect(receipt_revision).to eq(5) + end + + def receipt_revision + CoPlan::PlanRead.revision_for(plan: plan, reader_type: "api_token", reader_id: alice_token.id) + end + end + + describe "checkbox ticks" do + # Ticking a box in the UI is a person saying something about the plan's + # state, and it goes through the same human-version path as any other + # manual edit. It raises the fence like anything else would. + it "counts as a human edit" do + agent_has_read(plan, alice_token) + CoPlan::Plans::ReplaceContent.call( + plan: plan, new_content: initial_content + "\n- [x] Flag wired up\n", + base_revision: 1, actor_type: "human", actor_id: alice.id, + change_summary: "Checked an item" + ) + + put_content(initial_content) + + expect(response).to have_http_status(:conflict) + expect(JSON.parse(response.body)["code"]).to eq("human_edit_pending") + end + end +end diff --git a/spec/requests/api/v1/operations_spec.rb b/spec/requests/api/v1/operations_spec.rb index 6631352e..7e2e61e1 100644 --- a/spec/requests/api/v1/operations_spec.rb +++ b/spec/requests/api/v1/operations_spec.rb @@ -9,6 +9,9 @@ before do alice_token # ensure token exists + # Revision 1 is a human version, so the agent has to have read the plan + # before it may write (CoPlan::Plans::HumanEditGuard). + agent_has_read(plan, alice_token) CoPlan::EditLease.acquire!( plan: plan, holder_type: "local_agent", diff --git a/spec/requests/api/v1/sessions_spec.rb b/spec/requests/api/v1/sessions_spec.rb index b7847efc..5cc1f0de 100644 --- a/spec/requests/api/v1/sessions_spec.rb +++ b/spec/requests/api/v1/sessions_spec.rb @@ -8,6 +8,11 @@ before do alice_token # ensure token exists + # Revision 1 is a human version, so the agent has to have read the plan + # before it may write (CoPlan::Plans::HumanEditGuard). The intervening + # versions these specs create are agent versions on purpose — OT rebase + # is for agent-vs-agent races; a human's hand edit stops the write. + agent_has_read(plan, alice_token) end describe "POST /api/v1/plans/:plan_id/sessions" do @@ -235,7 +240,7 @@ plan: plan, revision: new_rev, content_markdown: intervening_content, - actor_type: "human", + actor_type: "local_agent", actor_id: alice.id, operations_json: [ { "op" => "replace_exact", @@ -278,7 +283,7 @@ plan: plan, revision: new_rev, content_markdown: intervening_content, - actor_type: "human", + actor_type: "local_agent", actor_id: alice.id, operations_json: [ { "op" => "replace_exact", @@ -313,6 +318,7 @@ p = CoPlan::Plan.create!(title: "Rich Plan", created_by_user: alice) v = CoPlan::PlanVersion.create!(plan: p, revision: 1, content_markdown: rich_content, actor_type: "human", actor_id: alice.id) p.update!(current_plan_version: v, current_revision: 1) + agent_has_read(p, alice_token) p end @@ -323,7 +329,7 @@ def create_intervening_replace(plan, old_text, new_text) new_rev = plan.current_revision + 1 v = CoPlan::PlanVersion.create!( plan: plan, revision: new_rev, - content_markdown: new_content, actor_type: "human", actor_id: alice.id, + content_markdown: new_content, actor_type: "local_agent", actor_id: alice.id, operations_json: [ { "op" => "replace_exact", "old_text" => old_text, "new_text" => new_text, @@ -408,6 +414,7 @@ def create_intervening_replace(plan, old_text, new_text) p = CoPlan::Plan.create!(title: "Rich Plan", created_by_user: alice) v = CoPlan::PlanVersion.create!(plan: p, revision: 1, content_markdown: rich_content, actor_type: "human", actor_id: alice.id) p.update!(current_plan_version: v, current_revision: 1) + agent_has_read(p, alice_token) p end @@ -418,7 +425,7 @@ def create_intervening_replace(plan, old_text, new_text) new_rev = plan.current_revision + 1 v = CoPlan::PlanVersion.create!( plan: plan, revision: new_rev, - content_markdown: new_content, actor_type: "human", actor_id: alice.id, + content_markdown: new_content, actor_type: "local_agent", actor_id: alice.id, operations_json: [ { "op" => "replace_exact", "old_text" => old_text, "new_text" => new_text, diff --git a/spec/support/agent_read_helpers.rb b/spec/support/agent_read_helpers.rb new file mode 100644 index 00000000..925fcb53 --- /dev/null +++ b/spec/support/agent_read_helpers.rb @@ -0,0 +1,19 @@ +# The API refuses an agent write when a person has hand-edited a plan since +# that credential last read it (CoPlan::Plans::HumanEditGuard). The plan +# factory builds revision 1 as a human version, so every spec that writes +# through the agent API has to satisfy the fence. In real life an agent +# satisfies it by reading the plan first, which is exactly what this does. +module AgentReadHelpers + def agent_has_read(plan, token, revision: nil) + CoPlan::PlanRead.record!( + plan: plan, + reader_type: "api_token", + reader_id: token.id, + revision: revision || plan.reload.current_revision + ) + end +end + +RSpec.configure do |config| + config.include AgentReadHelpers +end From 879e37333f831933e14278125a1e5712362f327b Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 24 Aug 2026 18:09:02 -0500 Subject: [PATCH 2/2] Enforce the fence under the plan lock, and on the paths with no request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three holes in the first pass, all found in review. The controller before_action was the only enforcement point, so anything reaching the write another way went straight past it. CommitExpiredSessionJob calls Plans::CommitSession directly: a session opened before a hand edit and then abandoned would get auto-committed and rebased over that edit with nobody in the loop at all — the worst version of the accident this is supposed to prevent. The fence now lives in CommitSession and ReplaceContent themselves, which every route shares, and the job marks a blocked session failed rather than committing it. It was also a check-then-write: the before_action queried, and the write took the plan lock some microseconds later. A human edit landing in that window would be seen by the rebase machinery and merged past. Enforcement is now inside the same locked transaction that creates the version, so nothing can land between the check and the write. The before_action stays as the fast, informative refusal — it just isn't what holds the line. And the read receipt was a load-compare-save, so two overlapping reads on one credential could both load the same row and the slower one save the older revision last — walking the receipt backwards and leaving an agent fenced after it genuinely read the human's edit. Monotonicity now lives in the UPDATE's WHERE clause: one statement, no window. The new specs for the first two fail without their fix (checked by reverting each). The receipt spec pins the contract but can't race a single-threaded suite; that fix is structural, and the spec says so. Co-Authored-By: Claude Opus 5 --- .../coplan/api/v1/base_controller.rb | 9 ++ .../coplan/api/v1/content_controller.rb | 5 +- .../coplan/api/v1/operations_controller.rb | 18 +++ .../coplan/api/v1/sessions_controller.rb | 5 +- .../jobs/coplan/commit_expired_session_job.rb | 8 ++ engine/app/models/coplan/plan_read.rb | 32 ++++- .../services/coplan/plans/commit_session.rb | 21 ++- .../services/coplan/plans/human_edit_guard.rb | 31 +++++ .../services/coplan/plans/replace_content.rb | 19 ++- spec/requests/api/v1/human_edit_guard_spec.rb | 126 ++++++++++++++++++ spec/services/plans/commit_session_spec.rb | 19 ++- 11 files changed, 274 insertions(+), 19 deletions(-) diff --git a/engine/app/controllers/coplan/api/v1/base_controller.rb b/engine/app/controllers/coplan/api/v1/base_controller.rb index 2b33886b..658f349e 100644 --- a/engine/app/controllers/coplan/api/v1/base_controller.rb +++ b/engine/app/controllers/coplan/api/v1/base_controller.rb @@ -158,6 +158,15 @@ def guard_human_edits! render json: block, status: :conflict if block end + # Reader identity to hand a service that writes under the plan lock, + # where the fence is actually enforced. Empty for a human calling + # the API on hook auth — their write *is* a human edit. + def fence_reader + return {} if api_author_type == "human" + + { reader_type: api_reader_type, reader_id: api_reader_id } + end + def set_plan @plan = CoPlan::Plan.find_by(id: params[:plan_id] || params[:id]) unless @plan diff --git a/engine/app/controllers/coplan/api/v1/content_controller.rb b/engine/app/controllers/coplan/api/v1/content_controller.rb index c383a91a..0d46023a 100644 --- a/engine/app/controllers/coplan/api/v1/content_controller.rb +++ b/engine/app/controllers/coplan/api/v1/content_controller.rb @@ -50,7 +50,8 @@ def update agent_name: api_agent_name, api_token_id: api_token_id, change_summary: params[:change_summary], - reason: params[:reason] + reason: params[:reason], + **fence_reader ) if result[:no_op] @@ -71,6 +72,8 @@ def update applied: result[:applied], version_id: version.id }, status: :created + rescue Plans::HumanEditGuard::Blocked => e + render json: e.payload, status: :conflict rescue Plans::ReplaceContent::StaleRevisionError => e render json: { error: e.message, diff --git a/engine/app/controllers/coplan/api/v1/operations_controller.rb b/engine/app/controllers/coplan/api/v1/operations_controller.rb index 470f013f..3d1f5c81 100644 --- a/engine/app/controllers/coplan/api/v1/operations_controller.rb +++ b/engine/app/controllers/coplan/api/v1/operations_controller.rb @@ -32,6 +32,8 @@ def create end rescue Plans::OperationError => e render json: { error: e.message }, status: :unprocessable_content + rescue Plans::HumanEditGuard::Blocked => e + render json: e.payload, status: :conflict end private @@ -104,6 +106,7 @@ def apply_direct(operations, base_revision) ActiveRecord::Base.transaction do @plan.lock! @plan.reload + enforce_human_edit_fence!(base_revision) current_content = @plan.current_content || "" @@ -191,6 +194,7 @@ def create_version_from_operations(operations, base_revision:) ActiveRecord::Base.transaction do @plan.lock! @plan.reload + enforce_human_edit_fence!(base_revision) if @plan.current_revision != base_revision render json: { @@ -331,6 +335,20 @@ def verify_transformed_ranges!(op, transformed_ranges, content) end end + # The before_action gives the caller a fast refusal; this is the one + # that actually holds the line, because it runs under the plan lock + # that the version creation below shares. + def enforce_human_edit_fence!(base_revision) + return if api_author_type == "human" + + Plans::HumanEditGuard.enforce!( + plan: @plan, + reader_type: api_reader_type, + reader_id: api_reader_id, + base_revision: base_revision + ) + end + def broadcast_plan_update Broadcaster.replace_to( @plan, diff --git a/engine/app/controllers/coplan/api/v1/sessions_controller.rb b/engine/app/controllers/coplan/api/v1/sessions_controller.rb index 16c2444d..f1903276 100644 --- a/engine/app/controllers/coplan/api/v1/sessions_controller.rb +++ b/engine/app/controllers/coplan/api/v1/sessions_controller.rb @@ -48,7 +48,8 @@ def commit change_summary: params[:change_summary], actor_id: api_user_id, agent_name: api_agent_name, - api_token_id: api_token_id + api_token_id: api_token_id, + **fence_reader ) response = { @@ -66,6 +67,8 @@ def commit end render json: response + rescue Plans::HumanEditGuard::Blocked => e + render json: e.payload, status: :conflict rescue Plans::CommitSession::SessionNotOpenError => e render json: { error: e.message }, status: :unprocessable_content rescue Plans::CommitSession::StaleSessionError => e diff --git a/engine/app/jobs/coplan/commit_expired_session_job.rb b/engine/app/jobs/coplan/commit_expired_session_job.rb index 5afc4268..1bd9ff16 100644 --- a/engine/app/jobs/coplan/commit_expired_session_job.rb +++ b/engine/app/jobs/coplan/commit_expired_session_job.rb @@ -27,6 +27,14 @@ def perform(session_id:) rescue Plans::CommitSession::SessionNotOpenError # Session was closed concurrently (manual commit/cancel) — nothing to do Rails.logger.info("CommitExpiredSessionJob: session #{session_id} already closed, skipping") + rescue Plans::HumanEditGuard::Blocked => e + # A person edited by hand while this session sat open, and the agent + # that owned it never came back to read them. Auto-committing would + # rebase an abandoned draft straight over their words with nobody in + # the loop to notice — so the session dies instead. The operations + # stay on the row if anyone wants to see what was lost. + session.update!(status: "failed", change_summary: "Auto-commit blocked: #{e.message}") + Rails.logger.warn("CommitExpiredSessionJob: session #{session_id} blocked by an unread human edit") rescue Plans::CommitSession::SessionConflictError, Plans::CommitSession::StaleSessionError, Plans::OperationError => e # Conflict during auto-commit — mark session as failed session.update!(status: "failed", change_summary: "Auto-commit failed: #{e.message}") diff --git a/engine/app/models/coplan/plan_read.rb b/engine/app/models/coplan/plan_read.rb index fccfd03d..f9fe54e4 100644 --- a/engine/app/models/coplan/plan_read.rb +++ b/engine/app/models/coplan/plan_read.rb @@ -24,17 +24,35 @@ class PlanRead < ApplicationRecord # Records that `reader` has seen `revision`. Monotonic: a later read of # an older revision (a version fetch, a cached response) never walks the # receipt backwards. + # + # The advance is a single UPDATE whose WHERE clause *is* the + # monotonicity, rather than a load-compare-save. Two concurrent reads on + # the same credential can otherwise both load the same row and the + # slower one can save the older revision last — walking the receipt + # backwards and leaving an agent fenced after it genuinely read the + # human's edit. def self.record!(plan:, reader_type:, reader_id:, revision:) - return nil if reader_type.blank? || reader_id.blank? || revision.blank? + return false if reader_type.blank? || reader_id.blank? || revision.blank? - record = find_or_initialize_by(plan_id: plan.id, reader_type: reader_type, reader_id: reader_id) - return record if record.persisted? && record.last_seen_revision >= revision.to_i + revision = revision.to_i + scope = where(plan_id: plan.id, reader_type: reader_type, reader_id: reader_id) + now = Time.current - record.last_seen_revision = revision.to_i - record.last_seen_at = Time.current - record.save! - record + advanced = scope.where(last_seen_revision: ...revision) + .update_all(last_seen_revision: revision, last_seen_at: now, updated_at: now) + return true if advanced.positive? + # No rows advanced: either the receipt is already at or past this + # revision, or there is no receipt yet. + return true if scope.exists? + + create!( + plan_id: plan.id, reader_type: reader_type, reader_id: reader_id, + last_seen_revision: revision, last_seen_at: now + ) + true rescue ActiveRecord::RecordNotUnique + # Another request inserted the row first; go around again and take the + # UPDATE path. Terminates: the row now exists. retry end diff --git a/engine/app/services/coplan/plans/commit_session.rb b/engine/app/services/coplan/plans/commit_session.rb index 06bb4af3..d4464c1d 100644 --- a/engine/app/services/coplan/plans/commit_session.rb +++ b/engine/app/services/coplan/plans/commit_session.rb @@ -16,16 +16,22 @@ class SessionNotOpenError < StandardError; end # The committing controller passes the resolved user and agent so the # version is attributed the way comments are; direct Ruby callers # (cloud personas) fall back to the session's own actor fields. - def self.call(session:, change_summary: nil, actor_id: nil, agent_name: nil, api_token_id: nil) - new(session:, change_summary:, actor_id:, agent_name:, api_token_id:).call + def self.call(session:, change_summary: nil, actor_id: nil, agent_name: nil, api_token_id: nil, reader_type: nil, reader_id: nil) + new(session:, change_summary:, actor_id:, agent_name:, api_token_id:, reader_type:, reader_id:).call end - def initialize(session:, change_summary: nil, actor_id: nil, agent_name: nil, api_token_id: nil) + def initialize(session:, change_summary: nil, actor_id: nil, agent_name: nil, api_token_id: nil, reader_type: nil, reader_id: nil) @session = session @change_summary = change_summary || session.change_summary @actor_id = actor_id || session.actor_id @agent_name = agent_name @api_token_id = api_token_id + # Whose reads count for the human-edit fence. A live commit passes + # the credential making the request; the expiry job has no request, + # so it falls back to the session's owner. A human-actor session + # isn't fenced at all. + @reader_type = reader_type || (session.actor_type == "human" ? nil : "api_token") + @reader_id = reader_id || (session.actor_type == "human" ? nil : session.actor_id) end def call @@ -45,6 +51,15 @@ def call plan.lock! + # Under the lock, and on every route into this service — the + # expiry job commits sessions with no request behind them, so a + # controller-level check alone would let an abandoned session + # rebase straight over a hand edit. + Plans::HumanEditGuard.enforce!( + plan: plan, reader_type: @reader_type, reader_id: @reader_id, + base_revision: @session.base_revision + ) + base_revision = @session.base_revision current_revision = plan.current_revision current_content = plan.current_content || "" diff --git a/engine/app/services/coplan/plans/human_edit_guard.rb b/engine/app/services/coplan/plans/human_edit_guard.rb index 147df2d4..4f1d6a70 100644 --- a/engine/app/services/coplan/plans/human_edit_guard.rb +++ b/engine/app/services/coplan/plans/human_edit_guard.rb @@ -34,12 +34,43 @@ class HumanEditGuard # re-read the plan regardless. MAX_EDITS_LISTED = 10 + # Raised by .enforce!. Carries the same payload .call returns, so a + # controller can render it identically wherever the block is caught. + class Blocked < StandardError + attr_reader :payload + + def initialize(payload) + @payload = payload + super(payload[:error]) + end + end + # Returns nil when the write may proceed, or a JSON-ready Hash # describing the block (render it with status :conflict). + # + # Call this early — before a request does any work — for a fast, + # informative refusal. It is NOT the enforcement point: a human edit + # can land between this check and the write. Use .enforce! inside the + # plan-locked transaction for that. def self.call(plan:, reader_type:, reader_id:, base_revision: nil) new(plan: plan, reader_type: reader_type, reader_id: reader_id, base_revision: base_revision).call end + # The enforcement point. Must be called inside the same transaction + # that holds the plan lock and creates the version, so no human + # revision can land between the check and the write — otherwise the + # rebase machinery downstream would happily merge the agent's edit + # past a hand edit that arrived microseconds too late. + # + # A blank reader identity means the caller isn't an agent (a human + # editing in the web UI, a system write); the fence doesn't apply. + def self.enforce!(plan:, reader_type:, reader_id:, base_revision: nil) + return if reader_type.blank? || reader_id.blank? + + payload = call(plan: plan, reader_type: reader_type, reader_id: reader_id, base_revision: base_revision) + raise Blocked, payload if payload + end + def initialize(plan:, reader_type:, reader_id:, base_revision: nil) @plan = plan @reader_type = reader_type diff --git a/engine/app/services/coplan/plans/replace_content.rb b/engine/app/services/coplan/plans/replace_content.rb index f2b09305..8e840ad3 100644 --- a/engine/app/services/coplan/plans/replace_content.rb +++ b/engine/app/services/coplan/plans/replace_content.rb @@ -30,7 +30,7 @@ def initialize(message, current_revision:) # 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:, agent_name: nil, api_token_id: nil, change_summary: nil, reason: nil) + def self.call(plan:, new_content:, base_revision:, actor_type:, actor_id:, agent_name: nil, api_token_id: nil, change_summary: nil, reason: nil, reader_type: nil, reader_id: nil) new( plan: plan, new_content: new_content, @@ -40,11 +40,13 @@ def self.call(plan:, new_content:, base_revision:, actor_type:, actor_id:, agent agent_name: agent_name, api_token_id: api_token_id, change_summary: change_summary, - reason: reason + reason: reason, + reader_type: reader_type, + reader_id: reader_id ).call end - def initialize(plan:, new_content:, base_revision:, actor_type:, actor_id:, agent_name: nil, api_token_id: nil, change_summary: nil, reason: nil) + def initialize(plan:, new_content:, base_revision:, actor_type:, actor_id:, agent_name: nil, api_token_id: nil, change_summary: nil, reason: nil, reader_type: nil, reader_id: nil) @plan = plan # Normalize line endings to LF before diffing. Browser textareas, agents # running on Windows, and copy-paste from various sources commonly emit @@ -61,6 +63,10 @@ def initialize(plan:, new_content:, base_revision:, actor_type:, actor_id:, agen @api_token_id = api_token_id @change_summary = change_summary @reason = reason + # Who is writing, for the human-edit fence. Blank for a human in the + # web UI or a system write — those aren't fenced. + @reader_type = reader_type + @reader_id = reader_id end def call @@ -68,6 +74,13 @@ def call @plan.lock! @plan.reload + # Inside the lock: a human edit committing between an earlier + # check and here would otherwise be merged past. + Plans::HumanEditGuard.enforce!( + plan: @plan, reader_type: @reader_type, reader_id: @reader_id, + base_revision: @base_revision + ) + if @plan.current_revision != @base_revision raise StaleRevisionError.new( "Stale revision. Expected #{@plan.current_revision}, got #{@base_revision}", diff --git a/spec/requests/api/v1/human_edit_guard_spec.rb b/spec/requests/api/v1/human_edit_guard_spec.rb index 8453aa19..09a3e6be 100644 --- a/spec/requests/api/v1/human_edit_guard_spec.rb +++ b/spec/requests/api/v1/human_edit_guard_spec.rb @@ -236,6 +236,85 @@ def put_content(body, revision: nil) end end + # The controller before_action is a fast, informative refusal; it is not + # the enforcement point. These pin the enforcement that runs under the + # plan lock, on the paths that never touch a controller at all. + describe "enforcement under the plan lock" do + it "refuses when the early check passed but a hand edit is there at write time" do + agent_has_read(plan, alice_token) + hand_edit!(from: "Q4", to: "Q3") + + # Stand in for the window: the before_action ran a moment before the + # hand edit committed, so it saw a clean plan and waved the request + # through. Only the check under the plan lock can still catch it. + early_check = true + allow(CoPlan::Plans::HumanEditGuard).to receive(:call).and_wrap_original do |original, **kwargs| + next original.call(**kwargs) unless early_check + + early_check = false + nil + end + + expect { + post api_v1_plan_operations_path(plan), + params: { + base_revision: 1, + operations: [ { op: "replace_exact", old_text: "Owner: Sam.", new_text: "Owner: Dana.", count: 1 } ] + }, + headers: headers, as: :json + }.not_to change(CoPlan::PlanVersion, :count) + + expect(response).to have_http_status(:conflict) + expect(JSON.parse(response.body)["code"]).to eq("human_edit_pending") + expect(plan.reload.current_content).to include("Owner: Sam.") + expect(plan.current_content).to include("Q3") + end + + it "blocks the expiry job from auto-committing over an unread hand edit" do + agent_has_read(plan, alice_token) + session = CoPlan::EditSession.create!( + plan: plan, actor_type: "local_agent", actor_id: alice_token.id, + base_revision: 1, expires_at: 1.minute.ago + ) + applied = CoPlan::Plans::ApplyOperations.call( + content: initial_content, + operations: [ { "op" => "replace_exact", "old_text" => "Owner: Sam.", "new_text" => "Owner: Dana.", "count" => 1 } ] + ) + session.update!(operations_json: applied[:applied], draft_content: applied[:content]) + + hand_edit!(from: "Q4", to: "Q3") + + expect { + CoPlan::CommitExpiredSessionJob.perform_now(session_id: session.id) + }.not_to change(CoPlan::PlanVersion, :count) + + expect(session.reload.status).to eq("failed") + expect(session.change_summary).to match(/Auto-commit blocked/) + expect(plan.reload.current_content).to include("Owner: Sam.") + expect(plan.current_content).to include("Q3") + end + + it "still auto-commits an expired session when no human edited" do + agent_has_read(plan, alice_token) + session = CoPlan::EditSession.create!( + plan: plan, actor_type: "local_agent", actor_id: alice_token.id, + base_revision: 1, expires_at: 1.minute.ago + ) + applied = CoPlan::Plans::ApplyOperations.call( + content: initial_content, + operations: [ { "op" => "replace_exact", "old_text" => "Owner: Sam.", "new_text" => "Owner: Dana.", "count" => 1 } ] + ) + session.update!(operations_json: applied[:applied], draft_content: applied[:content]) + + expect { + CoPlan::CommitExpiredSessionJob.perform_now(session_id: session.id) + }.to change(CoPlan::PlanVersion, :count).by(1) + + expect(session.reload.status).to eq("committed") + expect(plan.reload.current_content).to include("Owner: Dana.") + end + end + describe "read receipts" do it "records the revision handed over by GET /plans/:id" do get api_v1_plan_path(plan), headers: headers, as: :json @@ -257,6 +336,53 @@ def put_content(body, revision: nil) expect(receipt_revision).to eq(5) end + # The reason monotonicity lives in the UPDATE's WHERE clause rather than + # in a load-compare-save is concurrency: two overlapping reads on one + # credential could otherwise both load the same row and the slower one + # save the older revision last. A single-threaded spec can't race that — + # the guarantee is structural, since a guarded UPDATE has no window + # between the compare and the write. What this pins is the contract + # either implementation owes: a lower revision never lands, and doesn't + # cost a write. + it "ignores a lower revision without touching the row" do + agent_has_read(plan, alice_token, revision: 7) + before = CoPlan::PlanRead.find_by(plan_id: plan.id, reader_id: alice_token.id) + + CoPlan::PlanRead.record!( + plan: plan, reader_type: "api_token", reader_id: alice_token.id, revision: 6 + ) + + after = CoPlan::PlanRead.find_by(plan_id: plan.id, reader_id: alice_token.id) + expect(after.last_seen_revision).to eq(7) + expect(after.updated_at).to eq(before.updated_at) + end + + it "creates the receipt exactly once when another request wins the insert" do + raced = false + allow(CoPlan::PlanRead).to receive(:create!).and_wrap_original do |original, **attrs| + unless raced + raced = true + # insert_all bypasses create!, so this stands in for another + # process getting the row in first — the original call below then + # hits the unique index and the retry takes the UPDATE path. + now = Time.current + CoPlan::PlanRead.insert_all([ { + id: SecureRandom.uuid_v7, plan_id: plan.id, + reader_type: "api_token", reader_id: alice_token.id, + last_seen_revision: 3, last_seen_at: now, created_at: now, updated_at: now + } ]) + end + original.call(**attrs) + end + + CoPlan::PlanRead.record!( + plan: plan, reader_type: "api_token", reader_id: alice_token.id, revision: 4 + ) + + expect(CoPlan::PlanRead.where(plan_id: plan.id).count).to eq(1) + expect(receipt_revision).to eq(4) + end + def receipt_revision CoPlan::PlanRead.revision_for(plan: plan, reader_type: "api_token", reader_id: alice_token.id) end diff --git a/spec/services/plans/commit_session_spec.rb b/spec/services/plans/commit_session_spec.rb index f4b81de0..a92fbe7f 100644 --- a/spec/services/plans/commit_session_spec.rb +++ b/spec/services/plans/commit_session_spec.rb @@ -14,7 +14,7 @@ end def build_session(plan:, operations_json: [], draft_content: nil, base_revision: nil, **attrs) - CoPlan::EditSession.create!( + session = CoPlan::EditSession.create!( plan: plan, actor_type: "local_agent", actor_id: SecureRandom.uuid_v7, @@ -25,6 +25,17 @@ def build_session(plan:, operations_json: [], draft_content: nil, base_revision: expires_at: 1.hour.from_now, **attrs ) + # Revision 1 is a human version, and CommitSession fences an agent that + # hasn't read the human's words (Plans::HumanEditGuard). Opening a + # session in real life means the agent read the plan first; these specs + # are about the rebase, so give it the receipt. The intervening versions + # below are agent versions on purpose — OT rebase is for agent-vs-agent + # races; a hand edit stops the commit. + CoPlan::PlanRead.record!( + plan: plan, reader_type: "api_token", reader_id: session.actor_id, + revision: session.base_revision + ) + session end describe "happy path" do @@ -152,7 +163,7 @@ def build_session(plan:, operations_json: [], draft_content: nil, base_revision: end_pos = start_pos + old_text.length CoPlan::PlanVersion.create!( plan: plan, revision: 2, - content_markdown: v2_content, actor_type: "human", actor_id: user.id, + content_markdown: v2_content, actor_type: "local_agent", actor_id: user.id, operations_json: [ { "op" => "replace_exact", "old_text" => old_text, @@ -197,7 +208,7 @@ def build_session(plan:, operations_json: [], draft_content: nil, base_revision: end_pos = start_pos + old_text.length CoPlan::PlanVersion.create!( plan: plan, revision: 2, - content_markdown: v2_content, actor_type: "human", actor_id: user.id, + content_markdown: v2_content, actor_type: "local_agent", actor_id: user.id, operations_json: [ { "op" => "replace_exact", "old_text" => old_text, @@ -249,7 +260,7 @@ def build_session(plan:, operations_json: [], draft_content: nil, base_revision: new_content = current_content + "\n\nRevision #{rev} content." CoPlan::PlanVersion.create!( plan: plan, revision: rev, - content_markdown: new_content, actor_type: "human", actor_id: user.id, + content_markdown: new_content, actor_type: "local_agent", actor_id: user.id, operations_json: [] ) current_content = new_content