feat(coding-agent)!: file mutation coordinator with bounded Goal recovery - #2482
feat(coding-agent)!: file mutation coordinator with bounded Goal recovery#2482MarkAronov wants to merge 1 commit into
Conversation
Building a hashline rejection recorded the file's current content in the session snapshot store and reported that tag in the error, so re-sending the identical edit with the reported tag was accepted even though the model had never read the changed file. Inject a non-minting SnapshotStore decorator at the only Patcher construction site. Reads delegate to the session store so drift recovery is unchanged; record() computes the tag without retaining it, leaving recordHashlineSnapshot as the single writer of provenance. Refs bastani-inc#2329
|
Thanks for calling this out and measuring the current recovery behavior. Please proceed with the hybrid approach. Preserve the existing same-session merge-with-warning behavior when the hashline engine can apply a stale edit cleanly. Keep cross-session snapshot rejection strict, and convert cases the engine cannot safely recover into the typed Please also preserve #2496’s same-snapshot parallel batching. Those compatible sibling calls should remain one planned mutation rather than being treated as stale follow-ups. Writer and attempt identity should remain diagnostic only and must not grant ownership or permission. Since #2496 changed the same |
|
Rebased onto current main, and #2496's batching is preserved. Worth saying why, since it is load-bearing: siblings are merged into one Two smaller things I hit while wiring, both of which touch your point 1. One canonical key, or two? There are currently two. My preference is to keep both keys and document which is used where, on the grounds that unifying them means changing how Does "replaced" mean identity or content? Delete-and-recreate with different content already trips the guard. Delete-and-recreate with identical content passes every check we have, and if the replacement is a symlink pointing elsewhere, the canonical key now resolves to a different file than the one I have added a |
Related: #2329
What is in this diff
Building a hashline rejection minted provenance that defeated the rejection.
Patcher's mismatch path records the file's current content into the session snapshot storebefore constructing the error, then reports that tag to the model as
actualFileHash. Sincethe tag now names content the store has seen, re-sending the identical edit quoting it was
accepted, even though the model had never read the changed file. In practice that means a
worker whose edit was correctly rejected could retry once and silently overwrite whatever the
other writer had just put there.
The engine is vendored (
DO NOT EDIT), so the fix is injected at the singlenew Patcher(...)construction site: a
SnapshotStoredecorator whose reads delegate to the session store andwhose
record()computes the tag without retaining it.That is safe because every record the patcher would make is either unreachable or redundant
when it is driven by the edit tool:
commit()'s no-op branch never runs, becauseedit.tsfilters no-op sections and returnsbefore
applyis called.commit()'s post-write record is duplicated byrecordHashlineSnapshotinedit.ts, whichis also what populates the display-header map the patcher bypasses. That call remains the
single writer of provenance.
Reads still delegate, so
Recoveryresolves stale tags against the real per-session historyand in-session drift recovery is unchanged.
Tests
Two cases in
test/non-minting-snapshot-store.test.ts:stale tag, take the live hash out of the rejection message, retry the identical edit quoting
it. This test fails without the fix and passes with it.
RECOVERY_EXTERNAL_WARNING. This one passes either way by design: it is a guard proving thedecorator did not break drift recovery, not a regression test.
Local results:
npm run checkclean including the shrinkwrap gate; the four hashline andmutation-queue suites green (43 tests). The remaining local failures are pre-existing and
environmental on this machine (a Windows
tarpath issue in the release-publisher contract, astale untracked
dist/predating an upstream sync, and several bash/RPC concurrency testswhose failing identity changes between runs and which pass in isolation). CI on this PR is the
real check, which is part of why it is open early.
Still to come
FileMutationConflict, wired toedit, plus an opaque per-attempt UUID from the subagent runtime for correlation.writewill require a prior same-session observation beforeoverwriting an existing file, and will use exclusive-create semantics for new files.
WriteOperationsgains a read capability, so custom implementations will need updating.Migration will be documented in the changelog and in
docs/tools.md.max_turns, records conflict andrepair outcomes in the existing goal ledger, and moves to
needs_humanwith evidence whenthe budget is exhausted. Scoped to Goal; other workflows receive the typed conflict through
their current error path unchanged.
Open question
@flora131 one thing I would like to settle before wiring the edit tool, since I measured
something I did not expect.
When a section tag is stale, recovery replays the edit onto the snapshot the tag names and
applies the result to the live file with zero fuzz and three lines of context. Measured on a
40-line file with an edit at line 20: an external change at line 1 lands, merged with a
recovery warning, while a change at line 18 is rejected. So for same-session staleness,
proximity decides rather than staleness itself. Cross-session tags are unaffected and still
rejected outright.
Point 4 of your outline says "Reject changed or replaced targets with a typed
FileMutationConflict". Read literally that would also reject the distant-drift case the engine
currently merges. My preference is to preserve the merge-with-warning behaviour for
same-session drift and convert only the cases the engine already rejects, since the merge path
looks like deliberate engine design and removing it would make solo sessions stricter than they
are today. If you would rather have strict rejection whenever coordination is active, I would
gate it on the requesting identity being present so plain sessions keep current behaviour.
Scope boundaries
This guarantee covers the built-in
editandwritetools on ordinary local files. Shellwrites, external programs, other Atomic processes, selectors, archives, and custom backends
remain outside it unless they route through the same coordinator.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.