Repository navigation
PRD-7872 Prevent silent concurrent replace loss (#4430) - #29
Merged
Merged
Conversation
* PRD-7872 Prevent silent concurrent replace loss
* fix(open-knowledge): address concurrent replace review
* test(open-knowledge): keep refusal identities private
* test(open-knowledge): keep divergence probe in one session
* fix(open-knowledge): close concurrent replace review gaps
* fix(open-knowledge): address concurrent replace review
* fix(open-knowledge): tighten concurrent replace review fixes
* fix(open-knowledge): enforce replace admission accounting
* fix(open-knowledge): bind replace admission checks
* test(open-knowledge): reject callback-hoisted admission checks
* fix(open-knowledge): close concurrent replace review gaps
* fix(open-knowledge): surface advisory parser failures
* fix(open-knowledge): preserve malformed advisory signals
* fix(open-knowledge): declare malformed advisory fallback
* test(open-knowledge): close advisory review gaps
* docs(open-knowledge): complete advisory warning guidance
* test(open-knowledge): cover generic advisory fallback
* docs(open-knowledge): describe generic advisory fallback
* docs: clarify unidentified advisory fallback
* test: pin shared batch checkpoint allowance
* test: harden batch checkpoint budget coverage
* test: pin checkpoint outcome metrics
* fix: address concurrent replace review findings
* fix: align replace recovery and shutdown contracts
* test: align packed refusal recovery assertion
* fix: harden concurrent replace recovery
* fix: close remaining concurrent replace review gaps
* Harden PRD-7872 shutdown and recovery contracts
* Complete PRD-7872 shutdown review follow-up
* Close PRD-7872 shutdown review gaps
* Close shutdown review findings
* Cover shutdown head diagnostics
* Address concurrent replace review findings
* Pin writer-identity threading at every agent-write spine call
The `agentId` argument that drives the concurrent-replace refusal is an
optional 7th parameter, so a spine call that omits it typechecks clean and
silently stops recording the write as a peer. Merging origin/main exercised
exactly that: PRD-8617 (#4441) moved the lint-fix handler out of
api-extension.ts into http/lint-write-routes.ts, and the threading would have
been dropped at the new site with no gate to catch it.
Mirrors the existing agent-write-loss-detect-coverage.test.ts, including its
planted-negative and positive-control cases. Removing session.agentId from
lint-write-routes.ts fails the new test naming 'lint-write-routes.ts:349'.
* Stamp external-editor recency inside the observers that see a change
The concurrent-replace guard armed on any connection-origin transaction
reaching afterAllTransactions, including the SyncStep2 frame a client
sends on connect, which mutates nothing. A harness that connected a
client and then seeded the doc with position: replace got a 409, which
red-lined roughly 28 integration files and 35 e2e tests that this branch
never touched.
The stamp also sat after the observer.dispatch call, so a dispatch that
threw left the transaction committed with the stamp unset.
Move the stamp into observer A and observer B, gated on a connection
origin and placed after their sync and paired-write early returns. Both
observers fire only when their type actually changed, and both run
during transaction cleanup ahead of the dispatch, so the same move
fixes the over-arming and the throw-skips-the-stamp halves.
Both observers are required. Rich-text edits arrive on the XmlFragment
and only reach Y.Text through observer A's sync write, which observer B
early-returns on, so reusing the Y.Text recency would drop rich-text
protection. Removing either stamp reds its own arm of the existing
'recent source and rich-text edits are protected' test.
* Fail the concurrent-replace guard closed on a backward clock step
Bound the recency predicates by magnitude so a future-dated peer stamp
refuses the replace instead of being deleted and admitted. A clock step
smaller than the window now fails closed. A larger one still admits, so a
big jump cannot take agent replace offline for the length of the jump.
Defer the frontmatter edit-surface counter until after the refusal check,
so a replace that is about to be refused no longer overcounts mcp-write
during exactly the collision bursts the metric is read for. The refusal
check keeps its position after frontmatter validation, where a malformed
payload still returns the more actionable 400.
Guard the ts-morph writer-identity census with a negative sweep over
packages/{app,cli,desktop}/src, so a production call site appearing
outside the census walk root fails rather than going unseen.
* Make the concurrent-replace refusal actionable and countable
The refusal keys on elapsed time and position, never on payload content,
so the old "re-read it and retry" copy named an action that cannot clear
the gate. Reword all four customer-facing copies to name the wait and the
exempt positions, and carry the bound on the wire as a retryAfterSeconds
extension (plus a Retry-After header on the HTTP surface) so the batch and
ACP lanes, which never see headers, can read it too.
Count the refusal on the default-on reconciliation metrics snapshot via the
single logConcurrentOverwriteRefusal funnel, so an operator on defaults can
see refusal rate without grepping logs.
Give the ACP refusal a structured data discriminator, so a retryable refusal
is no longer indistinguishable on the wire from an unclassified handler throw
sharing code -32603.
Warn when a request agentId fails validation instead of silently demoting it
onto the default writer key, matching the existing mcp-http precedent, and
state in the docs that the peer half of the guard distinguishes writers by
agentId.
* Derive the guard-window wait from the constant it mirrors
doc-edge-blank-runs waited a bare 2100 ms to outlast the concurrent-replace
window, restating a module-private 2_000 in another package with nothing
tying the two together. Moving the window would have red a test whose
subject is trailing-blank-run fidelity, reporting only a 409.
Export CONCURRENT_REPLACE_WINDOW_MS through the server barrel and compute
the wait from it. Exporting the TypeScript symbol does not put the window
on the wire, so the value stays out of the client contract.
The wait itself stays. The test performs a real editor change before the
agent replace, so it legitimately arms the guard.
* Pin the refusal's retry contract and the agentId validation signal
The 409 gained a Retry-After header and a retryAfterSeconds body extension,
but nothing asserted either, and the exact-keys assertion on the problem
body did not list the new member, so the HTTP surface was both unpinned and
about to red. Assert the header, the extension value, and the full key set
in one place.
parseAgentBodyFields now warns when a supplied agentId fails validation
instead of silently demoting it onto the default writer key. Cover the two
rejection shapes, the silent cases, and the cardinality rule that the raw
id never reaches the log payload.
Sort the barrel export so biome's organizeImports is satisfied.
* Pin the admitting half of the backward-clock bound
The magnitude bound in hasRecentPeer and isRecent had coverage on only
its refusing side: the existing test steps the clock back 1 ms and
asserts the peer replace is refused. The deliberate other half — a step
at or beyond the window still admits, so a large backward step cannot
take agent replace offline for its duration — had no test, and the
obvious simplification of dropping Math.abs for a plain negative check
kept the suite green.
The new sibling derives its step from CONCURRENT_REPLACE_WINDOW_MS
rather than restating 2000, so a window change moves the boundary the
test probes. Boundary-proved: stepping one millisecond further forward,
to just inside the window, reds it.
* Suppress only the peer predicate when no agent identity was supplied
assertConcurrentReplaceAllowed opened with an all-or-nothing early
return on an absent agentId, so threading a genuine absence through it
would have taken the editor-recency half offline as well. Two shipped
surfaces already promise the opposite: mcp/tools/write.ts says "Peers
are told apart by agent identity, so a caller that sends none is refused
only against editor changes", and docs/content/reference/mcp.mdx says "A
recent change from an external editor still refuses them, so the guard
is partly rather than wholly inactive on that path".
Move the absence disjunct off the early return and onto the peer
predicate it actually guards. Nothing is added: the editor predicate and
the throw are untouched, and the guard still returns early for every
non-replace position.
No behavior changes yet, because every production caller still passes
the post-default sentinel. The identity threading that makes absence
reachable lands next.
* Thread the supplied writer id, not the defaulted one, into the guard
extractAgentIdentity collapses an absent body agentId onto a sentinel at
`const agentId = fields.writerId ?? 'claude-1';`, and four agent-write
route handlers passed that post-default value to applyAgentMarkdownWrite
as the concurrent-replace guard's peer key. Two callers that both sent no
identity therefore looked like one agent to the recency map, and a caller
that sent none armed a refusal against every identified agent that
followed inside the window. The HTTP surface says agentId is optional, so
this refused writes it had promised to accept.
parseAgentBodyFields already computes the presence-bearing writerId one
layer down and extractAgentIdentity already discarded it. Surface it on
the return type and pass it at the four sites in handleAgentWrite,
handleAgentWriteBatch, handleAgentWriteMd and handleAgentPatch. Where an
identity was supplied the value is byte-identical to session.agentId, so
those requests are unchanged. Where none was supplied the guard now
receives undefined, and both the record and the hasRecentPeer halves fall
silent, leaving the editor-recency half to refuse as documented.
The sentinel itself is untouched: it stays load-bearing for attribution
and presence, and the two call sites that pass a genuinely supplied
session.agentId, in acp/thread-manager.ts and http/lint-write-routes.ts,
are already correct and stay as they are.
Known red, deliberate: agent-write-agent-id-coverage.test.ts pins by AST
shape that every spine call passes a property access named agentId, which
the four fixed sites no longer do. Widening that predicate belongs to the
owner of that file.
* Stop three stress specs from tripping the concurrent-replace guard
Each of these specs armed the guard from its own setup and then asserted
on something unrelated, so the refusal surfaced as a thrown setup error
rather than a subject.
asset-click-dispatch seeds a one-heading document and then clicks the
editor body. The click lands below the last block, which is where
TrailingAffordance appends a paragraph, and that real editor change arms
the editor-recency half for every replaceDoc the tests issue next. Click
the heading instead. The caret still lands in the seeded document and the
editor is still focused for the drop in P9.11, which is the only test
that depends on the click at all.
multi-agent-presence raced two distinct agents at Promise.all on one
document, which the guard is designed to refuse: one of the two writes
must lose. Serialize them with a wait derived from
CONCURRENT_REPLACE_WINDOW_MS. No assertion is lost, because the badge
poll already ran after both writes had completed, and presence outlives
the gap by an order of magnitude. Its two navigation tests each wrote to
the same document twice with a freshly randomized id, so the second write
registered a new agent rather than refreshing the first. Hoist the id
into one const per document so the refresh does what it was added to do.
jsx-unregistered-ime-concurrent starts an IME composition and then drives
a server replace through it, so the composition itself is the editor
change that arms the guard. Wait out the window between the two. The
write stays at position replace, the composition stays open, and every
assertion is unchanged.
* Stop the refusal naming a position the endpoint cannot accept
The shared detail told the caller to "use append, prepend, or patch",
but `patch` is not a `position` value any agent-write surface accepts:
the enum is append | prepend | replace, and the patch operation is a
separate endpoint with a separate body shape. A caller following the
sentence literally gets a 400. The same delta's other two copies of the
escape-hatch sentence, in mcp.mdx and the write tool description, say
`edit` instead, so the three disagreed with each other as well.
Split the constant. CONCURRENT_OVERWRITE_REFUSED_DETAIL is now the part
that is true everywhere — the refusal and the retry — and the positions
clause moves into CONCURRENT_OVERWRITE_REFUSED_DETAIL_WITH_POSITIONS,
used by the two surfaces that are reached through a `position` field:
the single-write HTTP handler and the batch handler. ACP's handleFsWrite
has no position parameter, so it keeps the base detail rather than
advertising an escape hatch its caller cannot reach.
The changeset drops `patch` for the same reason and names the two
positions that are genuinely never refused.
* Carry the retry bound onto the MCP write tool and pin it to the window
The refusal's retryAfterSeconds reached HTTP as a body field plus a
Retry-After header, batch as a per-result field, and ACP as structured
error data. The MCP write tool, which is the loop this guard was built
for, flattened the failure to `${error} (${detail})` and dropped the
number. So the one caller that cannot read a response header or an error
envelope was the one caller told nothing about how long to wait, while
mcp.mdx stated the response carries the bound.
Append the bound to the tool's error text when the result carries one,
and correct the reference page to say what each surface actually
delivers, quoting the tool's literal suffix.
The bound itself was a standalone 3 tied to the 2 000 ms guard window by
nothing but coincidence, and every assertion on it pinned the literal, so
moving the window would leave the advertised wait silently short. The new
test pins the relation instead: the advertised retry must cover the window
that decides when the refusal clears.
* Warn on every unusable agentId, not only a rejected string
parseAgentBodyFields only reached the warn when body.agentId was a
non-empty string that failed validation. A number, a boolean, null, an
array, an object or an empty string took the `typeof === 'string'`
guard's else branch and vanished with no signal, so the misconfiguration
most likely to be a client bug — sending `{"agentId": 42}` — was the one
shape that demoted silently. The old test pinned that silence.
Classify the rejection once and branch on the classification: not-a-string,
empty, too-long, charset. The warn now fires for every supplied-but-
unusable value and reports `reason` plus `agentIdType` in place of the
old agentIdLength/agentIdCharsetOk pair, which said nothing useful about
a non-string. Absent stays silent, because absent is the documented
anonymous case rather than a mistake. The rejected value is still never
logged, now checked for an object-shaped one too.
The message claimed the request falls back to the unattributed default
writer. That is true of extractAgentIdentity, and false of the MCP path,
which has no default writer at all, so half of this parser's production
callers were described wrongly. It now states only what this function
does: the value was discarded before writer-identity resolution.
Widen the spine census predicate in the same pass. Threading the supplied
writer id (b0af0f4093) replaced four `session.agentId` property accesses
with a `fields.writerId` local, which the AST-shape assertion rejected;
it now admits either shape at a spine call, and the file walker admits
.tsx so a spine call cannot hide in one.
* test(ok): retry concurrent replacements by outcome
* test(ok): retry post-conflict replacement
* fix(ok): refuse an unidentified replace against a recent identified peer
Threading the undefaulted writer id removed the peer refusal for every pair
where exactly one side supplies an agentId. At the delta's base all four spine
calls passed session.agentId, which extractAgentIdentity resolved to 'claude-1',
so the predicate always ran. Once the raw id reached it, an anonymous caller
short-circuited the check before hasRecentPeer was consulted, and an identified
agent's whole-document replace could be destroyed by the next caller that omits
agentId, with a 200 and nothing in the response or the counters marking it.
hasRecentPeer now accepts string | undefined and assertConcurrentReplaceAllowed
consults it unconditionally. A caller with no usable identity can never be shown
to be the recorded writer, which is the property hasRecentPeer already tests.
The record side still skips an anonymous writer, so the two-anonymous-callers
exemption and the anonymous-then-identified ordering are unchanged. All four
rows of the matrix are pinned; the identified-then-anonymous row is new and
failed with "expected 200 to be 409" before the change.
The raw id also sat one argument away from AgentWriteLossDetect.writerId, which
carries the defaulted value, and the census matched on the name alone. It is now
a branded RawWriterId named suppliedWriterId, so passing the defaulted id at that
position fails tsc rather than reading as conforming.
mcp.mdx and the changeset stated a narrower exemption than the code holds and
now state the asymmetry.
* test(ok): bound the concurrent-replace retries to the guard window
116ea5edad and 633bd85845 replaced a CONCURRENT_REPLACE_WINDOW_MS-derived wait
with a 10s poll and recorded no cause, so the specs tolerated the refusal for
five times the documented window with no assertion on attempts or elapsed time.
A regression that widened the effective clearance anywhere under 10s would have
passed silently.
Measured the post-resolution write in conflict-authority-reconcile with an
attempt and elapsed counter: firstStatus=200 attempts=1 elapsedMs=5..12 across
all three cases, so the guard is already clear when that assertion runs and the
10s budget was never exercised. The preceding conflict-clear poll is what spends
the window, and it can in principle return sooner, so a bounded poll stays. Its
budget is now CONCURRENT_REPLACE_WINDOW_MS * 2 on all three sites.
The two stress specs keyed their retry off an error message owned by
_helpers/fixtures.ts, a file outside the change with no exported constant, so a
reword there would have made both blocks rethrow on the first expected 409. Both
throwers now attach err.status the way test-harness.ts already does, and the
retry decision goes through isConcurrentOverwriteRefusal.
* fix(ok): never record an identity-less writer as a replace peer
Removing the `agentId !== undefined` short-circuit last round was correct, but
it exposed a second half that had been hidden behind it. `hasRecentPeer`
compares each recorded key against the caller's own, and no string equals
`undefined`, so an unidentified caller now matches every recorded entry. The
lint-fix route records `'principal-anonymous'` for any anonymous actor whose
fix changes the document, on a `'patch'` write that `record()` does not gate by
position. An anonymous `POST /api/lint/fix` therefore refused the next
anonymous `POST /api/agent-write-md` replace for the rest of the window, which
is the exemption `mcp.mdx` and the changeset both promise.
The refusal now lives in the identity constructor rather than at one call site.
`asRawWriterId` was an exported unchecked cast, so the brand added to make a
wrong identity a compile error accepted the sentinel as readily as a real id,
and the ts-morph census backing it matches the argument's spelling rather than
its origin. Construction is module-private now, with two ways out: the parse
boundary, which brands a caller-supplied id after validation, and
`sessionWriterId`, which returns undefined for `ANONYMOUS_WRITER_ID`. Every
session-derived surface inherits the rule, not just the lint route.
The brand also drops its `unique symbol` tag and its folded `| undefined` for
the `string & { readonly __brand }` shape the five brands in `packages/core`
use, with optionality at the parameter. That retype leaves the guard's own unit
test the one file contradicting the signature, since `packages/server/tsconfig`
excludes `**/*.test.ts` and vitest does not typecheck, so its nine call sites
now construct the parameter the way production callers must.
`mcp.mdx:93` states the action the rule turns on and names the subject of each
claim rather than opening on a pronoun. Its scope is narrower than the earlier
wording: a lint fix on a project with a principal is recorded under that
principal, so the exemption covers a writer with no identity of its own rather
than any write that omits `agentId`.
* test(ok): discriminate the stress retry on the problem type, not the status
`agent-write-routes` dispatches both `ConcurrentOverwriteRefusedError` and
`DocInConflictError` to 409 from one catch, so a predicate keyed on the status
alone retried a disk-versus-CRDT conflict to budget exhaustion and reported it
as a poll timeout naming no cause. Both throwers now attach the body's `type`
beside the status they already carried, and the predicate reads that. It stays
a structured-field match, with no return to the message string an untouched
helper owns, and the URN is pinned at compile time against the canonical
`ProblemType` union so a rename of the wire contract fails `tsc`.
The walk-root guard moves out of the identity census into a file neither census
owns. It asserted that no `applyAgentMarkdownWrite` call site exists outside the
walked root for either census, which made the loss-detect census's coverage a
property of a neighbouring file that could be edited away without a signal. A
new assertion beside it holds every spine census to that one root by deriving
the census set from disk, so a third census joins the guarantee on arrival.
* test(ok): close concurrent writer review gaps
* test(ok): scope the identified-lint row to the case it covers
The identified row planted a directory where principal.json belongs, copied
from the anonymous sibling where that is the mechanism. The row supplies
agentId, so extract-actor-identity returns at the agent branch before the
principal is consulted, and the replace half never reads the principal at
all. Trimming it to .ok also moves the row onto a project that has a
principal, which is the production shape and which nothing else in the file
exercised. Under a mutation that lets a loaded principal override a supplied
agentId, the planted fixture passes and the trimmed one fails.
The suite title in the renamed spine-walk-root file now carries the family
words its filename does, matching its four siblings.
* docs(ok): drop the tab-pane anchor from the principal link
The heading the link targeted sits inside the first pane of a persisted
Tabs block on what-open-knowledge-writes.mdx, and fumadocs-ui 16.1.0 mounts
the inactive pane with display:none and resolves only tab-id hashes. A
reader carrying the CLI selection landed on a hidden section. Both panes
carry the principal.json row, so the page link resolves for either
selection, and every other anchor into that page targets a heading outside
the Tabs block.
* fix(ok): clarify concurrent write guidance (PRD-7872)
Correct the MCP write description so unidentified callers are described consistently with the implemented refusal rule. Add a mutation-proven unit table showing that append, prepend, and patch still succeed while a peer replace in the same window is refused.
* fix(ok): scope the agentId guidance to the door that carries it (PRD-7872)
The write tool description told the calling model to send a stable agentId,
but the tool declares no such field. docTargetShape carries path, content,
extension, template, frontmatter and position, and no MCP tool declares an
agentId schema property, so a nested one is stripped by zod's default with
nothing reported back. The value the tool sends is identity.connectionId,
minted per connection on both transports before the first tool call.
The description now says the server supplies that identity, and keeps the
refusal rule: a writer with no identity is never recorded, and is still
refused by a recent identified writer as well as by an editor change.
The mcp.mdx paragraph carried the same unqualified imperative. agentId is a
real optional body field on the HTTP agent-write schemas, so it names both
audiences now rather than dropping the rule.
* fix(ok): clarify MCP replace refusal guidance (PRD-7872)
Keep the MCP tool description focused on cases reachable over MCP. The server always supplies a per-connection identity, while recent external editor changes can still refuse a replace.
* fix(ok): clarify connected editor refusal (PRD-7872)
Describe the live connection-origin editor change accurately in the MCP tool metadata and reference docs.
* test(ok): poll the guard window instead of sleeping past it (PRD-7872)
The trailing-run replace test cleared its own concurrent-overwrite window
with a fixed CONCURRENT_REPLACE_WINDOW_MS + 100 sleep measured from a
settle() on the local client's own ytext. The guard compares against
lastExternalEditorChangeMs, stamped server-side when the connection-origin
transaction is observed, so the 100 ms margin had to absorb the WebSocket
round trip and server-side processing on top of scheduler jitter. When it
did not, the replace was refused with a 409 the test did not catch and the
failure surfaced under the trailing-run test's name.
Poll the replace until it is not refused, bounded by
CONCURRENT_REPLACE_WINDOW_MS * 2, matching the sibling site in
conflict-authority-reconcile.test.ts. A refused replace is asserted before
the write is recorded, so it writes nothing and retrying is side-effect
free. The replace still executes and the trailing-run convergence
assertions still run against it.
Verified: shrinking the poll bound to 200 ms fails with "pollUntil timed
out ... waiting for the replace of <doc> to clear the concurrent-overwrite
guard window", confirming the guard is armed at the first attempt and the
retry is load-bearing.
* docs(ok): clarify overwrite refusal recovery (PRD-7872)
* docs(ok): keep project skill within size gate (PRD-7872)
* docs(ok): disambiguate overwrite recovery (PRD-7872)
* fix(ok): tighten overwrite refusal contracts (PRD-7872)
* docs(ok): clarify MCP overwrite identity (PRD-7872)
* docs(ok): distinguish MCP transport identity (PRD-7872)
* docs(ok): make MCP peer guidance explicit (PRD-7872)
* docs(ok): document MCP identity collisions (PRD-7872)
* docs(ok): define forwarded MCP identity format (PRD-7872)
* docs(ok): unify writer identity rules (PRD-7872)
* docs(ok): clarify writer identity fallbacks (PRD-7872)
* docs(ok): generalize overwrite peer identity (PRD-7872)
* docs(ok): scope overwrite self-exemption (PRD-7872)
* docs(ok): scope MCP overwrite self-exemption (PRD-7872)
---------
GitOrigin-RevId: 5d3e77e806bb0297aa6c795af6959bb1eed749d5
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
1 participant
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.
Copybara-translated 1 Inkeep OSS change. Rebase-merge this PR so the prepared commit lands directly on public main.
Linear: this mirror replays a change that already merged upstream, so it must not drive ticket status.
skip PRD-7872