A hand edit stops the agents until they've read it - #201
Merged
HamptonMakes merged 3 commits intoAug 26, 2026
Conversation
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 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa11a16ed9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 <noreply@anthropic.com>
Two conflicts, both additive: * base_controller.rb — main added `plan_web_url` in the same private block where the fence added its reader-identity helpers. Kept both. * db/schema.rb — the version line. Regenerated from migrations rather than hand-merged, so main's BackfillPlanSlugs and the plan_reads table both land; the dump is now main's schema plus that one table.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Everywhere else in the concurrency machinery, a stale agent write gets merged past whatever landed in between.
POST /operationstransforms its ranges through the intervening versions and applies anyway;PUT /contentjust asks for a freshbase_revision. That's the right behavior for agent-vs-agent races — two agents editing different paragraphs shouldn't block each other.It's the wrong behavior when the intervening edit came from a person. Someone who opens the editor and changes the words by hand is the highest-authority input the document gets, and today an agent that never saw those words can silently undo them — rewriting the paragraph they just fixed, restoring the sentence they just cut. Worse, the existing 409 says only
"Stale revision. Expected 7, got 5", so the obvious next move is to re-send the same body withbase_revision: 7and wipe the edit.What this does
A
humanversion is now a fence, not a merge input. If one landed after this credential last read the plan, every agent write is refused — no rebase, no partial apply — with the person's own diff attached:{ "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": "…", "change_summary": "Legal cleared the earlier date", "diff": "-Ship behind a flag in Q4.\n+Ship behind a flag in Q3 — legal signed off…" }], "resolve": "GET /api/v1/plans/:id/snapshot to pull the current content…" }An agent told only "you are stale" re-derives. An agent shown the words a person chose can keep them. Every unseen human edit is listed with its diff, newest-first under a 12KB budget so the freshest human intent always survives truncation.
Why a read receipt and not
base_revisionbase_revisionis an assertion the caller supplies. Tracking reads is what makes "have you seen this?" a question the server can answer:GET /plans/:idand/snapshotnow write aCoPlan::PlanRead— highest revision fetched, keyed per credential, monotonic. Only a real fetch lifts the fence. There's a spec pinning exactly the bypass this closes (is not satisfied by bumping base_revision to the number in the error).A credential that has never read the plan sits behind the fence too (
last_seen_revision: 0→ "you have never read this plan"). Writing wholesale content to a document you've never opened is the blind-overwrite case, and reading first is step one of the documented workflow anyway.Receipts are per credential, not per person — an agent minting a fresh token per run starts empty and must read. Two agents on separate tokens don't clear the fence for each other.
Scope
Guarded at all four agent write paths:
PUT /content,POST /operations(before any rebase machinery runs), session create (so an agent can't accumulate ops against content it never saw), and session commit (a person may edit while the session is open). Web-UI human editing is untouched — human-vs-human already has its conflict screen. Agent-vs-agent staleness keeps the OT rebase it has today.Checkbox ticks count:
Plans::ToggleCheckboxwrites a real human version, and a tick is a person saying something about the plan's state. Cost is one cheap re-read.Reviewer notes
humanversion, so every agent-write spec was writing over an unread human edit. They now read first viaagent_has_read(spec/support/agent_read_helpers.rb).sessions_spec.rbwere creating their intervening versions as"human"while testing agent-vs-agent merging. Those are nowlocal_agent— which is what they always meant, and the change makes the specs test what their names claim.maximum(:revision)), no rows loaded. Only a genuine block pays to fetch versions, and it selects aroundcontent_markdown(MEDIUMTEXT per row) since only the diff is needed./agent-instructionsgains a "When a person has edited by hand" section — the response tells agents to keep the human's text and raise a comment rather than overwrite a decision someone made deliberately.PlanReadmodel gets an ActiveAdmin page, per AGENTS.md, so "why is my agent blocked?" is answerable without a console.Testing
Full suite green: 1791 examples, 0 failures (including system specs). 17 new specs in
spec/requests/api/v1/human_edit_guard_spec.rbcover the fence, the no-bypass property, receipt behavior, and the agent-vs-agent path staying lenient.rubocopclean.Migration adds
coplan_plan_reads(engine + host copy).Not included
The plan page shows nothing about a hand edit currently fencing agents. Easy to add a header marker if that's wanted.
🤖 Generated with Claude Code