Skip to content

feat(coding-agent)!: file mutation coordinator with bounded Goal recovery - #2482

Draft
MarkAronov wants to merge 1 commit into
bastani-inc:mainfrom
MarkAronov:feat/file-mutation-coordinator
Draft

feat(coding-agent)!: file mutation coordinator with bounded Goal recovery#2482
MarkAronov wants to merge 1 commit into
bastani-inc:mainfrom
MarkAronov:feat/file-mutation-coordinator

Conversation

@MarkAronov

@MarkAronov MarkAronov commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Draft: 1 of 4 commits. Only the provenance fix is in the diff so far. The coordinator,
the write changes, and the Goal recovery policy follow. Opened early for CI signal and to
keep @flora131 in the loop; one design question is still open below.

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 store
before constructing the error, then reports that tag to the model as actualFileHash. Since
the 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 single new Patcher(...)
construction site: a SnapshotStore decorator whose reads delegate to the session store and
whose 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, because edit.ts filters no-op sections and returns
    before apply is called.
  • commit()'s post-write record is duplicated by recordHashlineSnapshot in edit.ts, which
    is also what populates the display-header map the patcher bypasses. That call remains the
    single writer of provenance.
  • The mismatch record is the bug.

Reads still delegate, so Recovery resolves stale tags against the real per-session history
and in-session drift recovery is unchanged.

Tests

Two cases in test/non-minting-snapshot-store.test.ts:

  1. A rejection does not mint a usable tag. Read, change the file out of band, edit with the
    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.
  2. Recovery still works for a tag the session did record, asserted against
    RECOVERY_EXTERNAL_WARNING. This one passes either way by design: it is a guard proving the
    decorator did not break drift recovery, not a regression test.

Local results: npm run check clean including the shrinkwrap gate; the four hashline and
mutation-queue suites green (43 tests). The remaining local failures are pre-existing and
environmental on this machine (a Windows tar path issue in the release-publisher contract, a
stale untracked dist/ predating an upstream sync, and several bash/RPC concurrency tests
whose 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

  • Commit 2: the file mutation coordinator and a typed FileMutationConflict, wired to
    edit, plus an opaque per-attempt UUID from the subagent runtime for correlation.
  • Commit 3, breaking: write will require a prior same-session observation before
    overwriting an existing file, and will use exclusive-create semantics for new files.
    WriteOperations gains a read capability, so custom implementations will need updating.
    Migration will be documented in the changelog and in docs/tools.md.
  • Commit 4: Goal gains a conflict budget separate from max_turns, records conflict and
    repair outcomes in the existing goal ledger, and moves to needs_human with evidence when
    the 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 edit and write tools on ordinary local files. Shell
writes, external programs, other Atomic processes, selectors, archives, and custom backends
remain outside it unless they route through the same coordinator.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

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
@flora131

Copy link
Copy Markdown
Collaborator

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 FileMutationConflict path. Goal should then require a fresh read and replan, use the separate conflict budget, and move to needs_human with evidence when that budget is exhausted.

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 edit.ts path after this draft’s base, please rebase onto current main before wiring the coordinator and the remaining commits.

@MarkAronov

Copy link
Copy Markdown
Contributor Author

Rebased onto current main, and #2496's batching is preserved. Worth saying why, since it is load-bearing: siblings are merged into one Patch before prepare runs, so every section in a batch prepares against the same live content and the existing staleness loop already runs once over the merged set. The guard goes exactly where that loop is, so compatible siblings stay one planned mutation. Adding a per-call check before takeCompatible absorbs them would break it, so I have deliberately not done that.

Two smaller things I hit while wiring, both of which touch your point 1.

One canonical key, or two? There are currently two. withFileMutationQueue keys by realpath, so a symlink and its target share a queue. The hashline snapshot store keys by the normalized absolute path and does not resolve symlinks. That is fine for edit, which compares text, but it decides write: the prior-observation check asks the store whether this session recorded this content for this path, and asking under the realpath key when the read recorded under the lexical one misses and reports a false "no prior observation". Not hypothetical, since AGENTS.md suggests symlinking packages/workflows into ~/.atomic/agent/extensions/, which is the same shape as the path in the original report.

My preference is to keep both keys and document which is used where, on the grounds that unifying them means changing how read, search and write record snapshots, which widens a PR that already carries a breaking change. Happy to unify instead if you would rather have literally one key.

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 prepare read. Catching that needs inode identity compared across the prepare-to-write window.

I have added a target_missing reason for the narrower case, a target removed between prepare and write, which currently surfaces as a bare ENOENT rather than anything countable. My preference is to treat "replaced" as content-replaced and leave identity-replacement as a documented non-guarantee next to shell writes and external programs, rather than add stat plumbing to this PR. Say if you want identity checked and I will scope it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants