From 566677ff58ccd2357eee588e5fa7bea3fa8c4fe1 Mon Sep 17 00:00:00 2001 From: serafin-garcia Date: Wed, 16 Sep 2026 17:33:27 +0000 Subject: [PATCH] PRD-7872 Prevent silent concurrent replace loss (#4430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 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 --- skills/core/open-knowledge/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/core/open-knowledge/SKILL.md b/skills/core/open-knowledge/SKILL.md index f3c0d26..1b379fb 100644 --- a/skills/core/open-knowledge/SKILL.md +++ b/skills/core/open-knowledge/SKILL.md @@ -91,7 +91,7 @@ Every `.md` / `.mdx` needs YAML frontmatter — `title` + `description` required ## Conflict-aware writes -A 409 `doc-in-conflict` freezes writes and carries `conflict.kind` + `resolutionOptions`. A flush-time `stale-external-write` 409 means the edit reached collaborative recovery but not disk; resolve and re-read before retrying. Detect with `conflicts`, not `exec`; kinds and full flow in `references/conflict-resolution.md`. +`doc-in-conflict` freezes writes; `stale-external-write` means recovery missed disk. Detect both with `conflicts`, not `exec`; see `references/conflict-resolution.md`. `concurrent-overwrite-refused` is not conflict state: wait its bound and retry or use `append`/`prepend`/`edit`. ## Anti-patterns — the top offenders