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 279cde0f..6f8cba98 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_23_193530) 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 @@ -478,6 +489,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 97faf507..e40947c6 100644 --- a/engine/app/controllers/coplan/api/v1/base_controller.rb +++ b/engine/app/controllers/coplan/api/v1/base_controller.rb @@ -118,6 +118,55 @@ 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 + + # 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 + # A document's address, absolute, so a caller can hand it straight to # a human. Built from the request rather than from Urls::Canonical # alone: only the request knows the host, and a host that mounted the diff --git a/engine/app/controllers/coplan/api/v1/content_controller.rb b/engine/app/controllers/coplan/api/v1/content_controller.rb index a840f347..0d46023a 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? @@ -44,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] @@ -57,12 +64,16 @@ 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, 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 4c46e314..3d1f5c81 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] @@ -28,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 @@ -100,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 || "" @@ -187,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: { @@ -228,6 +236,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: { @@ -324,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/plans_controller.rb b/engine/app/controllers/coplan/api/v1/plans_controller.rb index ac6f97aa..00661adc 100644 --- a/engine/app/controllers/coplan/api/v1/plans_controller.rb +++ b/engine/app/controllers/coplan/api/v1/plans_controller.rb @@ -27,7 +27,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, @@ -286,6 +290,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..f1903276 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. @@ -42,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 = { @@ -55,9 +62,13 @@ 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 + 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.rb b/engine/app/models/coplan/plan.rb index dc93d54e..07546306 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..f9fe54e4 --- /dev/null +++ b/engine/app/models/coplan/plan_read.rb @@ -0,0 +1,77 @@ +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. + # + # 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 false if reader_type.blank? || reader_id.blank? || revision.blank? + + revision = revision.to_i + scope = where(plan_id: plan.id, reader_type: reader_type, reader_id: reader_id) + now = Time.current + + 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 + + # 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/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 new file mode 100644 index 00000000..4f1d6a70 --- /dev/null +++ b/engine/app/services/coplan/plans/human_edit_guard.rb @@ -0,0 +1,187 @@ +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 + + # 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 + @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/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/engine/app/views/coplan/agent_instructions/show.text.erb b/engine/app/views/coplan/agent_instructions/show.text.erb index 80a09d56..a20bb4db 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. @@ -429,10 +430,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. @@ -648,5 +683,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..09a3e6be --- /dev/null +++ b/spec/requests/api/v1/human_edit_guard_spec.rb @@ -0,0 +1,409 @@ +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 + + # 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 + + 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 + + # 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 + 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/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 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