fix(mcp): expose the disk-commit boundary when a mutating tool is abandoned
A tool call is bounded by a transport deadline; when it fires, boundHandler
answers the client and lets the handler keep running. agents.AtomicWriteFile
takes no context, so a handler that had already reached its rename committed to
disk anyway — and the caller was told only "the work may still complete in the
background, so treat any side effect as unknown".
That single message covered two opposite outcomes. A client reading it as
"nothing happened" retried, or fell back to its own editor, and applied the same
logical change twice.
Split the two states and make both observable:
- Every disk commit registers in a durable ledger BEFORE the write and is
stamped terminal immediately after it, so a receipt exists for any mutation
that is still reachable.
- commitFileMutation adds a cancellation gate between registration and the
write. Stopping there is a guarantee that nothing was written, not a guess;
the remaining window is one atomic rename wide and is covered by an
in_flight receipt.
- The abandoned-call error now reports what actually landed — committed with
path and new_sha, not_applied, or genuinely unknown — plus a machine-readable
mutation_commit={...} tail. Read tools keep the original wording.
- Edit responses carry disk_status and graph_status as independent fields:
bytes reaching disk and the graph catching up are different questions with
different failure modes.
- mutation_status (facade: change/receipt) queries a receipt by id,
mutation_id, or path for 30 minutes, refreshing a still-pending graph state
rather than answering from a stale snapshot.
- edit_file / write_file / edit_symbol take an optional mutation_id
idempotency key, mirroring batch_edit's transaction_id: the same key with the
identical edit replays the original result instead of writing twice, and a
different edit under the same key is refused.
rename_symbol and the batch symbol/file edits register in the same ledger; a
coordinated rename now stops at its first cancelled write instead of attempting
every remaining file.
The agent preset's tools/list ceiling is re-based 28200 -> 28400 for the
mutation_id parameter on the two edit tools (148 bytes measured); the preset
itself did not grow.
Fixes #548
This commit is contained in:
+32
-2
@@ -286,6 +286,7 @@ over a very large tree, or `ask` against a slow local model.
|
||||
| `edit_symbol` | Edit a symbol's source directly by ID — no Read needed. Line-ending tolerant: an LF-authored `old_source` matches a CRLF file (and vice versa) and the replacement adopts the file's endings (`eol_normalized: true` rides on the response). Optional `base_sha` content-hash guard refuses the write when the on-disk SHA has drifted; every success carries `new_sha` so the next edit can pipeline without re-reading |
|
||||
| `edit_file` | Edit any file (markdown, config, spec, template, source) by exact string replacement — accepts absolute paths or repo-rooted paths. Line-ending tolerant: an LF-authored `old_string` matches a CRLF file (and vice versa) and the replacement is written with the file's own endings (`eol_normalized: true` rides on the response). Same `base_sha` / `new_sha` drift guard. Kills Read-before-Edit for files not in the graph |
|
||||
| `write_file` | Create or overwrite any file — atomic temp+rename, re-indexes on write. Same `base_sha` / `new_sha` drift guard |
|
||||
| `mutation_status` | What a file mutation actually did, after the fact. Reports `disk_status` (`committed` / `not_applied` / `failed` / `in_flight`) separately from `graph_status` (`fresh` / `pending` / `stale` / `failed`), selected by `receipt`, `mutation_id`, or `path`. Use it instead of retrying when an edit call was abandoned at its deadline |
|
||||
| `rename_symbol` | Coordinated multi-file rename with all references — definition, graph usages, receiver lines, and test names that embed the old identifier. Replacement is whole-identifier, so renaming `Get` leaves `GetUser` intact. Every target line is re-verified against disk and every affected file is parse-gated before anything is written, so the rename lands completely or is refused; `dry_run: true` returns the identical edit list without writing. Successful responses carry `status` (`applied` / `would_apply` / `no_edits`) plus per-file `bytes_written` / `new_sha` / `reindexed`. An existing unindexed target returns a structured `symbol_not_indexed` error only when the configured extractor anchors the requested declaration. Its `safe_fallback.request` is a guarded exact edit of that declaration line (`scope: declaration_only`); same-file and cross-file references remain explicitly unproven, and the refusal itself writes no bytes |
|
||||
| `move_symbol` | Relocate a function / method / type / variable / const to another file. Cross-package moves rewrite every qualified reference, drop the source import, add the target import, synthesise the target file if missing. Go for now |
|
||||
| `inline_symbol` | Replace every callsite of a trivial single-statement / single-expression callee with the body — refuses cleanly on defer, spawn, close-over-scope, multi-return, or side-effecting arg. `delete_after: true` removes the declaration. Go for now |
|
||||
@@ -355,8 +356,37 @@ Contract details that matter for an evidence workflow:
|
||||
disk observation.
|
||||
|
||||
Whether the graph caught up with the write is a separate question, answered by
|
||||
the `reindexed` / `reindex_pending` / `reindex_generation` fields every mutation
|
||||
already returns.
|
||||
`graph_status` (and the `reindexed` / `reindex_pending` / `reindex_generation`
|
||||
fields behind it) on every mutation response. Whether the write happened *at
|
||||
all* is a third question — see the next section.
|
||||
|
||||
### Mutating tools and the transport deadline
|
||||
|
||||
A tool call is bounded (`GORTEX_MCP_TOOL_TIMEOUT`, default 60s). When a handler
|
||||
outruns that budget the transport is released and the handler keeps running, so
|
||||
a write can land *after* the client was answered. The mutating tools make that
|
||||
observable instead of leaving it unknown:
|
||||
|
||||
- **Before the disk commit**, a cancelled request is refused and nothing is
|
||||
written. The response says `disk_status=not_applied` and retrying is safe.
|
||||
- **After the disk commit**, the abandoned-call error names what landed —
|
||||
path, `new_sha`, and a receipt id — and carries a machine-readable
|
||||
`mutation_commit={...}` tail. Re-applying the edit would duplicate it.
|
||||
- **Either way**, the receipt is queryable for 30 minutes with
|
||||
`mutation_status` (facade: `change` with `operation: "receipt"`), which
|
||||
reports the disk state and the graph-freshness state independently.
|
||||
- `edit_file` / `write_file` / `edit_symbol` accept an optional `mutation_id`
|
||||
idempotency key. Retrying with the same key and the identical edit replays
|
||||
the original result without writing again; reusing it for a different edit is
|
||||
refused. `batch_edit` has the equivalent `transaction_id`.
|
||||
|
||||
Successful edit responses carry `disk_status`, `graph_status`, and
|
||||
`mutation_receipt` alongside the existing `new_sha` / `reindexed` fields.
|
||||
|
||||
`physical_evidence` and `disk_status` answer different questions and compose:
|
||||
the first attests *what bytes* are on disk, the second whether the write
|
||||
happened at all. A call abandoned before its response returns no evidence block
|
||||
— only the receipt — which is exactly when `disk_status` is the field you need.
|
||||
|
||||
## Agent-optimized (token efficiency)
|
||||
|
||||
|
||||
@@ -398,6 +398,10 @@ func (s *Server) mutationReceiptState(id string) (mutationReindexOutcome, bool)
|
||||
// unnecessary source re-read.
|
||||
func (s *Server) attachMutationFreshness(resp map[string]any, relPath, absPath string, outcome mutationReindexOutcome) {
|
||||
resp["reindexed"] = outcome.Reindexed
|
||||
// graph_status is the freshness half of the mutation contract, named so it
|
||||
// reads next to disk_status (mutation_commit.go) rather than having to be
|
||||
// inferred from the reindexed / reindex_pending / reindex_error triple.
|
||||
resp["graph_status"] = graphStatusFor(outcome)
|
||||
if outcome.Generation > 0 {
|
||||
resp["reindex_generation"] = outcome.Generation
|
||||
}
|
||||
|
||||
@@ -342,6 +342,14 @@ func facadeOperationSpecs() []facadeOperationSpec {
|
||||
"batch": "batch_edit", "docs": "generate_docs", "export_graph": "export_graph", "file": "edit_file", "scaffold": "scaffold",
|
||||
"skill": "generate_skill", "symbol": "edit_symbol", "write": "write_file",
|
||||
})
|
||||
// mutation_status answers "did my edit land?", which is a read about
|
||||
// change state — it cannot ride the edit facade, whose operations are all
|
||||
// local_write (facades hold one effect class each, see
|
||||
// TestFacadeEffectBoundaryParity).
|
||||
specs = append(specs, facadeOperationSpec{
|
||||
Facade: "change", Operation: "receipt", Legacy: "mutation_status",
|
||||
Effect: facadeEffectRead,
|
||||
})
|
||||
// generate_wiki can call an LLM when enhance=true. Keep the ordinary edit
|
||||
// authorization boundary local-only; enhanced generation remains available
|
||||
// through the explicit legacy compatibility surface.
|
||||
|
||||
@@ -34,7 +34,7 @@ func TestFacadeRegistryCoversRegisteredLegacyCatalog(t *testing.T) {
|
||||
}
|
||||
}
|
||||
require.Empty(t, missing, "every registered legacy tool must map into facade-v1")
|
||||
require.Len(t, srv.facades.byLegacy, 178, "facade-v1 migration table must cover the full legacy catalog")
|
||||
require.Len(t, srv.facades.byLegacy, 179, "facade-v1 migration table must cover the full legacy catalog")
|
||||
require.Len(t, facadeToolNames(), 21)
|
||||
for _, name := range facadeToolNames() {
|
||||
if name == "capabilities" {
|
||||
|
||||
@@ -0,0 +1,559 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/sha256"
|
||||
"encoding/hex"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"sort"
|
||||
"strings"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"time"
|
||||
|
||||
"github.com/zzet/gortex/internal/agents"
|
||||
)
|
||||
|
||||
// The commit ledger exists because a tool call has two independent terminal
|
||||
// states and the transport only ever reported one of them.
|
||||
//
|
||||
// boundHandler answers the client when the deadline fires and lets the handler
|
||||
// keep running (tool_deadline.go). For a read that is harmless. For a write it
|
||||
// was not: agents.AtomicWriteFile takes no context, so a handler that had
|
||||
// already reached the rename committed to disk regardless, and the caller was
|
||||
// told only "treat any side effect as unknown". A client that then retried, or
|
||||
// fell back to its own editor, applied the same logical change twice.
|
||||
//
|
||||
// Every disk commit therefore registers here BEFORE the write and is stamped
|
||||
// terminal immediately after it. The record outlives the request, so:
|
||||
//
|
||||
// - the abandoned response can name what actually landed instead of guessing
|
||||
// (mutationCommitNote, read by boundToolHandler),
|
||||
// - a client that lost the response can ask (mutation_status),
|
||||
// - a client that supplied a mutation_id gets the original result replayed
|
||||
// rather than writing twice.
|
||||
//
|
||||
// Disk state and graph state are deliberately separate fields. "The bytes are
|
||||
// on disk" and "the graph has caught up" are different questions with different
|
||||
// failure modes, and collapsing them is what made the original error message
|
||||
// unusable.
|
||||
const (
|
||||
// mutationDiskNotApplied means the handler stopped before the write. This
|
||||
// is a guarantee, not a guess: the ledger entry is created ahead of the
|
||||
// cancellation check, so a record can only reach this state by taking the
|
||||
// branch that skips agents.AtomicWriteFile entirely.
|
||||
mutationDiskNotApplied = "not_applied"
|
||||
// mutationDiskInFlight means the write was entered but has not reported
|
||||
// back. A caller observing this must query rather than retry.
|
||||
mutationDiskInFlight = "in_flight"
|
||||
// mutationDiskCommitted means the atomic rename returned success.
|
||||
mutationDiskCommitted = "committed"
|
||||
// mutationDiskFailed means the write was attempted and errored. Nothing was
|
||||
// renamed into place, so the target still holds its previous content.
|
||||
mutationDiskFailed = "failed"
|
||||
)
|
||||
|
||||
const (
|
||||
mutationGraphPending = "pending"
|
||||
mutationGraphFresh = "fresh"
|
||||
mutationGraphStale = "stale"
|
||||
mutationGraphFailed = "failed"
|
||||
)
|
||||
|
||||
const (
|
||||
// mutationCommitRetention outlives the transport deadline by a wide margin:
|
||||
// a client only learns it needs the receipt after its call was abandoned,
|
||||
// and it may spend several turns reasoning before it asks.
|
||||
mutationCommitRetention = 30 * time.Minute
|
||||
// maxMutationCommits bounds the ledger for a long-lived daemon doing bulk
|
||||
// edits. Oldest-first eviction keeps the entries a recovering client is
|
||||
// most likely to want.
|
||||
maxMutationCommits = 512
|
||||
// maxMutationCommitListing caps an unfiltered mutation_status listing.
|
||||
maxMutationCommitListing = 20
|
||||
)
|
||||
|
||||
// mutationIDParamDescription is shared across the three single-file mutating
|
||||
// tools for the same reason the physical-evidence blurbs are (mutation_evidence.go):
|
||||
// the cold tools/list byte ceiling is measured, and three copies of one sentence
|
||||
// is pure tax.
|
||||
const mutationIDParamDescription = "Optional idempotency key. Retrying with the same key and the identical edit " +
|
||||
"replays the first result instead of writing twice — the safe way to retry an abandoned call. " +
|
||||
"A different edit under the same key is refused."
|
||||
|
||||
var mutationCommitSequence atomic.Uint64
|
||||
|
||||
// errMutationNotApplied is returned by commitFileMutation when the request was
|
||||
// cancelled before the disk commit. Callers surface it as a terminal refusal so
|
||||
// the client knows retrying is safe.
|
||||
var errMutationNotApplied = errors.New("mutation not applied")
|
||||
|
||||
type mutationCommitRecord struct {
|
||||
mu sync.RWMutex
|
||||
|
||||
id string
|
||||
tool string
|
||||
key string
|
||||
relPath string
|
||||
absPath string
|
||||
// fingerprint identifies the logical edit this record was created for. It
|
||||
// is only consulted when key != "", to reject a mutation_id reused for a
|
||||
// different payload.
|
||||
fingerprint string
|
||||
|
||||
disk string
|
||||
graph string
|
||||
|
||||
newSHA string
|
||||
bytesWritten int
|
||||
reindexReceipt string
|
||||
errText string
|
||||
|
||||
startedAt time.Time
|
||||
committedAt time.Time
|
||||
|
||||
// response is the successful tool payload, retained only for records that
|
||||
// carry a caller-chosen mutation_id. Without a key there is nothing to
|
||||
// replay against, so ordinary edits pay no memory for this.
|
||||
response map[string]any
|
||||
}
|
||||
|
||||
type mutationCommitSnapshot struct {
|
||||
Receipt string `json:"receipt"`
|
||||
Tool string `json:"tool,omitempty"`
|
||||
MutationID string `json:"mutation_id,omitempty"`
|
||||
Path string `json:"path,omitempty"`
|
||||
DiskStatus string `json:"disk_status"`
|
||||
GraphStatus string `json:"graph_status"`
|
||||
NewSHA string `json:"new_sha,omitempty"`
|
||||
BytesWritten int `json:"bytes_written,omitempty"`
|
||||
Error string `json:"error,omitempty"`
|
||||
StartedAt string `json:"started_at,omitempty"`
|
||||
CommittedAt string `json:"committed_at,omitempty"`
|
||||
}
|
||||
|
||||
func (r *mutationCommitRecord) snapshot() mutationCommitSnapshot {
|
||||
r.mu.RLock()
|
||||
defer r.mu.RUnlock()
|
||||
snap := mutationCommitSnapshot{
|
||||
Receipt: r.id,
|
||||
Tool: r.tool,
|
||||
MutationID: r.key,
|
||||
Path: r.relPath,
|
||||
DiskStatus: r.disk,
|
||||
GraphStatus: r.graph,
|
||||
NewSHA: r.newSHA,
|
||||
BytesWritten: r.bytesWritten,
|
||||
Error: r.errText,
|
||||
}
|
||||
if !r.startedAt.IsZero() {
|
||||
snap.StartedAt = r.startedAt.UTC().Format(time.RFC3339Nano)
|
||||
}
|
||||
if !r.committedAt.IsZero() {
|
||||
snap.CommittedAt = r.committedAt.UTC().Format(time.RFC3339Nano)
|
||||
}
|
||||
return snap
|
||||
}
|
||||
|
||||
func (r *mutationCommitRecord) diskStatus() string {
|
||||
r.mu.RLock()
|
||||
defer r.mu.RUnlock()
|
||||
return r.disk
|
||||
}
|
||||
|
||||
func (r *mutationCommitRecord) markNotApplied(err error) {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
r.disk = mutationDiskNotApplied
|
||||
r.graph = mutationGraphStale
|
||||
if err != nil {
|
||||
r.errText = err.Error()
|
||||
}
|
||||
}
|
||||
|
||||
func (r *mutationCommitRecord) markFailed(err error) {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
r.disk = mutationDiskFailed
|
||||
r.graph = mutationGraphStale
|
||||
if err != nil {
|
||||
r.errText = err.Error()
|
||||
}
|
||||
}
|
||||
|
||||
func (r *mutationCommitRecord) markCommitted(newSHA string, bytesWritten int) {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
r.disk = mutationDiskCommitted
|
||||
r.newSHA = newSHA
|
||||
r.bytesWritten = bytesWritten
|
||||
r.committedAt = time.Now()
|
||||
}
|
||||
|
||||
// recordGraph stamps the graph half of the receipt from the freshness outcome
|
||||
// the reindex path produced. It never touches the disk half: a failed reindex
|
||||
// does not un-write a committed file, and conflating the two is the reporting
|
||||
// bug this ledger exists to fix.
|
||||
func (r *mutationCommitRecord) recordGraph(outcome mutationReindexOutcome) {
|
||||
if r == nil {
|
||||
return
|
||||
}
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
r.graph = graphStatusFor(outcome)
|
||||
r.reindexReceipt = outcome.Receipt
|
||||
}
|
||||
|
||||
// retainResponse stores the successful payload for idempotent replay. Only
|
||||
// keyed records retain it — see mutationCommitRecord.response.
|
||||
func (r *mutationCommitRecord) retainResponse(resp map[string]any) {
|
||||
if r == nil {
|
||||
return
|
||||
}
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
if r.key == "" {
|
||||
return
|
||||
}
|
||||
clone := make(map[string]any, len(resp)+1)
|
||||
for key, value := range resp {
|
||||
clone[key] = value
|
||||
}
|
||||
r.response = clone
|
||||
}
|
||||
|
||||
func (r *mutationCommitRecord) replayResponse() (map[string]any, bool) {
|
||||
r.mu.RLock()
|
||||
defer r.mu.RUnlock()
|
||||
if r.response == nil {
|
||||
return nil, false
|
||||
}
|
||||
clone := make(map[string]any, len(r.response)+1)
|
||||
for key, value := range r.response {
|
||||
clone[key] = value
|
||||
}
|
||||
return clone, true
|
||||
}
|
||||
|
||||
func (r *mutationCommitRecord) pendingReindexReceipt() string {
|
||||
r.mu.RLock()
|
||||
defer r.mu.RUnlock()
|
||||
if r.graph != mutationGraphPending {
|
||||
return ""
|
||||
}
|
||||
return r.reindexReceipt
|
||||
}
|
||||
|
||||
// graphStatusFor collapses a reindex outcome into the graph half of a receipt.
|
||||
func graphStatusFor(outcome mutationReindexOutcome) string {
|
||||
switch {
|
||||
case outcome.Err != nil:
|
||||
return mutationGraphFailed
|
||||
case outcome.Pending:
|
||||
return mutationGraphPending
|
||||
case outcome.Reindexed:
|
||||
return mutationGraphFresh
|
||||
default:
|
||||
return mutationGraphStale
|
||||
}
|
||||
}
|
||||
|
||||
// mutationCommitLedger is the daemon-lifetime store. It is a plain guarded map
|
||||
// rather than a sync.Map because eviction needs a total order over entries, and
|
||||
// bounding it matters more here than lock-free reads on a path that already
|
||||
// performs a filesystem write.
|
||||
type mutationCommitLedger struct {
|
||||
mu sync.Mutex
|
||||
byID map[string]*mutationCommitRecord
|
||||
byKey map[string]*mutationCommitRecord
|
||||
ordered []*mutationCommitRecord
|
||||
}
|
||||
|
||||
func (l *mutationCommitLedger) put(record *mutationCommitRecord) {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
if l.byID == nil {
|
||||
l.byID = make(map[string]*mutationCommitRecord)
|
||||
l.byKey = make(map[string]*mutationCommitRecord)
|
||||
}
|
||||
l.evictLocked(time.Now())
|
||||
l.byID[record.id] = record
|
||||
if record.key != "" {
|
||||
l.byKey[record.key] = record
|
||||
}
|
||||
l.ordered = append(l.ordered, record)
|
||||
}
|
||||
|
||||
// evictLocked drops expired entries first, then the oldest survivors if the
|
||||
// ledger is still over its cap.
|
||||
func (l *mutationCommitLedger) evictLocked(now time.Time) {
|
||||
keep := l.ordered[:0]
|
||||
for _, record := range l.ordered {
|
||||
record.mu.RLock()
|
||||
expired := now.Sub(record.startedAt) > mutationCommitRetention
|
||||
record.mu.RUnlock()
|
||||
if expired {
|
||||
delete(l.byID, record.id)
|
||||
if record.key != "" && l.byKey[record.key] == record {
|
||||
delete(l.byKey, record.key)
|
||||
}
|
||||
continue
|
||||
}
|
||||
keep = append(keep, record)
|
||||
}
|
||||
l.ordered = keep
|
||||
for len(l.ordered) >= maxMutationCommits {
|
||||
oldest := l.ordered[0]
|
||||
l.ordered = l.ordered[1:]
|
||||
delete(l.byID, oldest.id)
|
||||
if oldest.key != "" && l.byKey[oldest.key] == oldest {
|
||||
delete(l.byKey, oldest.key)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func (l *mutationCommitLedger) byReceipt(id string) (*mutationCommitRecord, bool) {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
record, ok := l.byID[id]
|
||||
return record, ok
|
||||
}
|
||||
|
||||
func (l *mutationCommitLedger) byMutationID(key string) (*mutationCommitRecord, bool) {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
record, ok := l.byKey[key]
|
||||
return record, ok
|
||||
}
|
||||
|
||||
// recentForPath returns the newest record touching relPath. Path recovery is
|
||||
// what a client has left when it lost the response entirely and therefore never
|
||||
// saw a receipt id.
|
||||
func (l *mutationCommitLedger) recentForPath(relPath string) (*mutationCommitRecord, bool) {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
for i := len(l.ordered) - 1; i >= 0; i-- {
|
||||
record := l.ordered[i]
|
||||
record.mu.RLock()
|
||||
match := record.relPath == relPath || record.absPath == relPath
|
||||
record.mu.RUnlock()
|
||||
if match {
|
||||
return record, true
|
||||
}
|
||||
}
|
||||
return nil, false
|
||||
}
|
||||
|
||||
func (l *mutationCommitLedger) recent(limit int) []*mutationCommitRecord {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
if limit <= 0 || limit > len(l.ordered) {
|
||||
limit = len(l.ordered)
|
||||
}
|
||||
out := make([]*mutationCommitRecord, 0, limit)
|
||||
for i := len(l.ordered) - 1; i >= 0 && len(out) < limit; i-- {
|
||||
out = append(out, l.ordered[i])
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// mutationCommitNote is the per-request channel between a mutating handler and
|
||||
// the deadline wrapper that may answer for it. boundHandler installs one on
|
||||
// every call context; only handlers that reach a disk commit ever write to it.
|
||||
type mutationCommitNote struct {
|
||||
mu sync.Mutex
|
||||
records []*mutationCommitRecord
|
||||
}
|
||||
|
||||
type mutationCommitNoteKey struct{}
|
||||
|
||||
func withMutationCommitNote(ctx context.Context) (context.Context, *mutationCommitNote) {
|
||||
note := &mutationCommitNote{}
|
||||
return context.WithValue(ctx, mutationCommitNoteKey{}, note), note
|
||||
}
|
||||
|
||||
func mutationCommitNoteFrom(ctx context.Context) *mutationCommitNote {
|
||||
note, _ := ctx.Value(mutationCommitNoteKey{}).(*mutationCommitNote)
|
||||
return note
|
||||
}
|
||||
|
||||
func (n *mutationCommitNote) observe(record *mutationCommitRecord) {
|
||||
if n == nil || record == nil {
|
||||
return
|
||||
}
|
||||
n.mu.Lock()
|
||||
defer n.mu.Unlock()
|
||||
n.records = append(n.records, record)
|
||||
}
|
||||
|
||||
// mutationCommitVerdict is what the deadline wrapper reports. It is read at
|
||||
// abandon time from a handler that is still running, so it is deliberately
|
||||
// conservative: anything short of a confirmed terminal state reads as unknown.
|
||||
type mutationCommitVerdict struct {
|
||||
// Status is one of "", "not_applied", "unknown", or "committed".
|
||||
Status string `json:"disk_status"`
|
||||
Snapshots []mutationCommitSnapshot `json:"mutations,omitempty"`
|
||||
}
|
||||
|
||||
func (n *mutationCommitNote) verdict() mutationCommitVerdict {
|
||||
if n == nil {
|
||||
return mutationCommitVerdict{}
|
||||
}
|
||||
n.mu.Lock()
|
||||
records := append([]*mutationCommitRecord(nil), n.records...)
|
||||
n.mu.Unlock()
|
||||
if len(records) == 0 {
|
||||
return mutationCommitVerdict{}
|
||||
}
|
||||
verdict := mutationCommitVerdict{Status: mutationDiskNotApplied}
|
||||
for _, record := range records {
|
||||
snap := record.snapshot()
|
||||
verdict.Snapshots = append(verdict.Snapshots, snap)
|
||||
switch snap.DiskStatus {
|
||||
case mutationDiskCommitted:
|
||||
verdict.Status = mutationDiskCommitted
|
||||
case mutationDiskInFlight:
|
||||
if verdict.Status != mutationDiskCommitted {
|
||||
verdict.Status = mutationDiskInFlight
|
||||
}
|
||||
}
|
||||
}
|
||||
return verdict
|
||||
}
|
||||
|
||||
// beginMutationCommit registers an in-flight disk commit and publishes it to
|
||||
// the request's note. It must be called before the cancellation check that
|
||||
// guards the write, so that a record exists for every path on which a write is
|
||||
// still reachable.
|
||||
func (s *Server) beginMutationCommit(ctx context.Context, tool, mutationID, fingerprint, relPath, absPath string) *mutationCommitRecord {
|
||||
record := &mutationCommitRecord{
|
||||
id: fmt.Sprintf("commit-%d", mutationCommitSequence.Add(1)),
|
||||
tool: tool,
|
||||
key: mutationID,
|
||||
fingerprint: fingerprint,
|
||||
relPath: relPath,
|
||||
absPath: absPath,
|
||||
disk: mutationDiskInFlight,
|
||||
graph: mutationGraphStale,
|
||||
startedAt: time.Now(),
|
||||
}
|
||||
s.mutationCommits.put(record)
|
||||
mutationCommitNoteFrom(ctx).observe(record)
|
||||
return record
|
||||
}
|
||||
|
||||
// commitFileMutation is the single guarded disk-commit primitive for the
|
||||
// single-file mutating tools. Every caller gets three things it did not have
|
||||
// before: a refusal when the request is already dead, a durable receipt, and a
|
||||
// note the deadline wrapper can read if it answers first.
|
||||
func (s *Server) commitFileMutation(
|
||||
ctx context.Context,
|
||||
tool, mutationID, fingerprint, relPath, absPath string,
|
||||
data []byte,
|
||||
perm os.FileMode,
|
||||
) (*mutationCommitRecord, error) {
|
||||
record := s.beginMutationCommit(ctx, tool, mutationID, fingerprint, relPath, absPath)
|
||||
if s.mutationPreCommitHook != nil {
|
||||
s.mutationPreCommitHook(record)
|
||||
}
|
||||
// The cancellation gate. Before this line no bytes can have been written,
|
||||
// so refusing here is the "cancellation guarantees no mutation" half of the
|
||||
// contract. After it the window is one atomic rename wide, and that window
|
||||
// is exactly what the in_flight receipt covers.
|
||||
if err := ctx.Err(); err != nil {
|
||||
record.markNotApplied(err)
|
||||
return record, fmt.Errorf("%w: %w", errMutationNotApplied, err)
|
||||
}
|
||||
if err := agents.AtomicWriteFile(absPath, data, perm); err != nil {
|
||||
record.markFailed(err)
|
||||
return record, err
|
||||
}
|
||||
record.markCommitted(gitBlobSHA(data), len(data))
|
||||
return record, nil
|
||||
}
|
||||
|
||||
// mutationNotAppliedMessage is the terminal refusal for a write that was
|
||||
// cancelled before it started. It says "nothing changed" plainly, because that
|
||||
// is the one case where a client may retry without checking anything.
|
||||
func mutationNotAppliedMessage(what string, record *mutationCommitRecord, err error) string {
|
||||
return fmt.Sprintf(
|
||||
"%s cancelled before the disk commit — nothing was written and the file is unchanged (disk_status=not_applied, receipt=%s): %v. "+
|
||||
"Retrying is safe.",
|
||||
what, record.id, err)
|
||||
}
|
||||
|
||||
// attachMutationCommit puts the disk half of the receipt on a success payload.
|
||||
// The graph half rides on the same response via attachMutationFreshness.
|
||||
func attachMutationCommit(resp map[string]any, record *mutationCommitRecord) {
|
||||
if record == nil {
|
||||
return
|
||||
}
|
||||
resp["mutation_receipt"] = record.id
|
||||
resp["disk_status"] = record.diskStatus()
|
||||
if record.key != "" {
|
||||
resp["mutation_id"] = record.key
|
||||
}
|
||||
}
|
||||
|
||||
// mutationFingerprint identifies one logical mutation. It is only ever compared
|
||||
// against another fingerprint stored under the same caller-chosen mutation_id,
|
||||
// so it does not need to be globally unique — it needs to catch a key reused
|
||||
// for a different edit.
|
||||
func mutationFingerprint(tool string, parts ...string) string {
|
||||
sum := sha256.Sum256([]byte(tool + "\x00" + strings.Join(parts, "\x00")))
|
||||
return hex.EncodeToString(sum[:16])
|
||||
}
|
||||
|
||||
// replayMutation resolves a caller-supplied mutation_id against the ledger.
|
||||
//
|
||||
// Reusing a key with the SAME payload replays the original result instead of
|
||||
// writing again — this is what makes a retry after an abandoned call safe.
|
||||
// Reusing it with a DIFFERENT payload is a caller bug and is refused rather
|
||||
// than silently treated as a new edit, matching atomic_batch_edit.
|
||||
func (s *Server) replayMutation(mutationID, fingerprint string) (map[string]any, string, bool) {
|
||||
if mutationID == "" {
|
||||
return nil, "", false
|
||||
}
|
||||
record, ok := s.mutationCommits.byMutationID(mutationID)
|
||||
if !ok {
|
||||
return nil, "", false
|
||||
}
|
||||
record.mu.RLock()
|
||||
storedFingerprint := record.fingerprint
|
||||
disk := record.disk
|
||||
record.mu.RUnlock()
|
||||
if storedFingerprint != fingerprint {
|
||||
return nil, fmt.Sprintf(
|
||||
"mutation_id %q was already used for a different edit — reuse it only to retry the identical mutation, or choose a new id",
|
||||
mutationID), true
|
||||
}
|
||||
if disk != mutationDiskCommitted {
|
||||
// A prior attempt under this key did not commit. Nothing to replay:
|
||||
// let the caller through so the edit can actually land.
|
||||
return nil, "", false
|
||||
}
|
||||
resp, ok := record.replayResponse()
|
||||
if !ok {
|
||||
snap := record.snapshot()
|
||||
encoded, _ := json.Marshal(snap)
|
||||
return nil, fmt.Sprintf(
|
||||
"mutation_id %q already committed but its response was not retained; query mutation_status for the receipt: %s",
|
||||
mutationID, encoded), true
|
||||
}
|
||||
resp["replayed"] = true
|
||||
resp["mutation_receipt"] = record.id
|
||||
return resp, "", true
|
||||
}
|
||||
|
||||
// mutationCommitListing renders records for the mutation_status tool.
|
||||
func mutationCommitListing(records []*mutationCommitRecord) []mutationCommitSnapshot {
|
||||
out := make([]mutationCommitSnapshot, 0, len(records))
|
||||
for _, record := range records {
|
||||
out = append(out, record.snapshot())
|
||||
}
|
||||
sort.SliceStable(out, func(i, j int) bool { return out[i].StartedAt > out[j].StartedAt })
|
||||
return out
|
||||
}
|
||||
@@ -0,0 +1,543 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"github.com/zzet/gortex/internal/indexer"
|
||||
)
|
||||
|
||||
// resultJSON parses a successful tool payload.
|
||||
func resultJSON(t *testing.T, res *mcp.CallToolResult) map[string]any {
|
||||
t.Helper()
|
||||
require.NotNil(t, res)
|
||||
require.False(t, res.IsError, "expected success, got: %s", resultText(res))
|
||||
var payload map[string]any
|
||||
require.NoError(t, json.Unmarshal([]byte(resultText(res)), &payload))
|
||||
return payload
|
||||
}
|
||||
|
||||
// newMutationServer builds the smallest Server the single-file mutating tools
|
||||
// need: session bookkeeping, no indexer. Without an indexer the graph half of
|
||||
// every receipt reports "stale", which is exactly the separation under test.
|
||||
func newMutationServer() *Server {
|
||||
return &Server{session: &sessionState{}}
|
||||
}
|
||||
|
||||
func writeFileRequest(args map[string]any) mcp.CallToolRequest {
|
||||
req := mcp.CallToolRequest{}
|
||||
req.Params.Name = "write_file"
|
||||
req.Params.Arguments = args
|
||||
return req
|
||||
}
|
||||
|
||||
func editFileRequest(args map[string]any) mcp.CallToolRequest {
|
||||
req := mcp.CallToolRequest{}
|
||||
req.Params.Name = "edit_file"
|
||||
req.Params.Arguments = args
|
||||
return req
|
||||
}
|
||||
|
||||
// --- Criterion: timeout BEFORE the write applies nothing -------------------
|
||||
|
||||
// A cancelled request must not reach the disk. The handler takes its
|
||||
// cancellation gate between registering the commit and calling AtomicWriteFile,
|
||||
// so the refusal is a guarantee rather than a race the caller has to reason
|
||||
// about.
|
||||
func TestWriteFileCancelledBeforeCommitAppliesNothing(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "never_written.go")
|
||||
|
||||
s := newMutationServer()
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
cancel()
|
||||
|
||||
res, err := s.handleWriteFile(ctx, writeFileRequest(map[string]any{
|
||||
"path": target, "content": "package p\n",
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
require.True(t, res.IsError)
|
||||
|
||||
_, statErr := os.Stat(target)
|
||||
require.True(t, os.IsNotExist(statErr), "a cancelled write must not create the file")
|
||||
}
|
||||
|
||||
// The gate that matters sits AFTER the mutation lock, not only in front of it.
|
||||
// A deadline that lands while the handler is reading, matching, or parse-gating
|
||||
// leaves it holding the lock with a dead context — exactly the state
|
||||
// boundHandler leaves a detached handler in — and it must still refuse rather
|
||||
// than commit. mutationPreCommitHook is the only way into that window.
|
||||
func TestEditFileCancelledInsideCommitWindowWritesNothing(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "guarded.txt")
|
||||
require.NoError(t, os.WriteFile(target, []byte("alpha\n"), 0o644))
|
||||
|
||||
s := newMutationServer()
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
var reached bool
|
||||
s.mutationPreCommitHook = func(record *mutationCommitRecord) {
|
||||
reached = true
|
||||
// The record exists before the gate, so a client can always ask what
|
||||
// happened even for a mutation that never starts.
|
||||
require.Equal(t, mutationDiskInFlight, record.diskStatus())
|
||||
cancel()
|
||||
}
|
||||
|
||||
res, err := s.handleEditFile(ctx, editFileRequest(map[string]any{
|
||||
"path": target, "old_string": "alpha", "new_string": "beta",
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
require.True(t, reached, "the handler must reach the commit window")
|
||||
require.True(t, res.IsError)
|
||||
require.Contains(t, resultText(res), "disk_status=not_applied")
|
||||
require.Contains(t, resultText(res), "Retrying is safe")
|
||||
|
||||
content, readErr := os.ReadFile(target)
|
||||
require.NoError(t, readErr)
|
||||
require.Equal(t, "alpha\n", string(content), "the file must be untouched")
|
||||
|
||||
record, ok := s.mutationCommits.recentForPath(target)
|
||||
require.True(t, ok, "the refusal must still be recorded")
|
||||
require.Equal(t, mutationDiskNotApplied, record.diskStatus())
|
||||
}
|
||||
|
||||
// The same window for write_file, where the target does not exist yet: a
|
||||
// cancelled create must leave no file behind at all.
|
||||
func TestWriteFileCancelledInsideCommitWindowCreatesNothing(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "not_created.go")
|
||||
|
||||
s := newMutationServer()
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
s.mutationPreCommitHook = func(*mutationCommitRecord) { cancel() }
|
||||
|
||||
res, err := s.handleWriteFile(ctx, writeFileRequest(map[string]any{
|
||||
"path": target, "content": "package p\n",
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
require.True(t, res.IsError)
|
||||
|
||||
_, statErr := os.Stat(target)
|
||||
require.True(t, os.IsNotExist(statErr), "a cancelled create must not leave a file")
|
||||
}
|
||||
|
||||
// --- Criterion: timeout AFTER the write reports the commit -----------------
|
||||
|
||||
// The regression from the issue. boundHandler answers at the deadline while the
|
||||
// handler keeps running; once that handler has committed, the answer must say
|
||||
// so instead of "treat any side effect as unknown".
|
||||
func TestAbandonedCallReportsCommittedDiskWrite(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "landed.go")
|
||||
|
||||
s := newMutationServer()
|
||||
s.ToolCallTimeout = 150 * time.Millisecond
|
||||
|
||||
committed := make(chan struct{})
|
||||
handler := func(ctx context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) {
|
||||
record, err := s.commitFileMutation(ctx, "write_file", "", "fp", "landed.go", target,
|
||||
[]byte("package p\n"), 0o644)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, mutationDiskCommitted, record.diskStatus())
|
||||
close(committed)
|
||||
// Stand in for the post-commit graph refresh that outran the budget:
|
||||
// mutationReindexState can fall through to a synchronous, contextless
|
||||
// IncrementalReindexRepo, which is where the reporter's 59s went.
|
||||
time.Sleep(500 * time.Millisecond)
|
||||
return mcp.NewToolResultText("applied"), nil
|
||||
}
|
||||
|
||||
req := mcp.CallToolRequest{}
|
||||
req.Params.Name = "write_file"
|
||||
res, err := s.boundToolHandler(handler)(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
require.True(t, res.IsError)
|
||||
|
||||
<-committed
|
||||
text := resultText(res)
|
||||
require.Contains(t, text, "WAS COMMITTED")
|
||||
require.Contains(t, text, "Do NOT retry")
|
||||
require.NotContains(t, text, "treat any side effect as unknown")
|
||||
|
||||
// The structured tail is what a client parses.
|
||||
_, encoded, found := strings.Cut(text, "mutation_commit=")
|
||||
require.True(t, found, "abandoned response must carry a machine-readable receipt")
|
||||
var verdict mutationCommitVerdict
|
||||
require.NoError(t, json.Unmarshal([]byte(encoded), &verdict))
|
||||
require.Equal(t, mutationDiskCommitted, verdict.Status)
|
||||
require.Len(t, verdict.Snapshots, 1)
|
||||
require.Equal(t, "landed.go", verdict.Snapshots[0].Path)
|
||||
require.NotEmpty(t, verdict.Snapshots[0].NewSHA)
|
||||
|
||||
// And the receipt it names must resolve.
|
||||
record, ok := s.mutationCommits.byReceipt(verdict.Snapshots[0].Receipt)
|
||||
require.True(t, ok)
|
||||
require.Equal(t, mutationDiskCommitted, record.diskStatus())
|
||||
}
|
||||
|
||||
// An abandoned handler that refused before its write must be reported as such,
|
||||
// so the client knows a retry is safe.
|
||||
func TestAbandonedCallReportsNotAppliedWrite(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "refused.go")
|
||||
|
||||
s := newMutationServer()
|
||||
s.ToolCallTimeout = 100 * time.Millisecond
|
||||
|
||||
handler := func(ctx context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) {
|
||||
<-ctx.Done() // the deadline fires first
|
||||
_, err := s.commitFileMutation(ctx, "write_file", "", "fp", "refused.go", target,
|
||||
[]byte("package p\n"), 0o644)
|
||||
require.Error(t, err)
|
||||
return mcp.NewToolResultText("unreachable"), nil
|
||||
}
|
||||
|
||||
req := mcp.CallToolRequest{}
|
||||
req.Params.Name = "write_file"
|
||||
res, err := s.boundToolHandler(handler)(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
require.True(t, res.IsError)
|
||||
|
||||
// The handler races the response; wait for it to reach its gate.
|
||||
require.Eventually(t, func() bool {
|
||||
record, ok := s.mutationCommits.recentForPath("refused.go")
|
||||
return ok && record.diskStatus() == mutationDiskNotApplied
|
||||
}, 2*time.Second, 10*time.Millisecond)
|
||||
|
||||
_, statErr := os.Stat(target)
|
||||
require.True(t, os.IsNotExist(statErr), "the refused write must not exist")
|
||||
}
|
||||
|
||||
// A read tool must keep the original liveness wording — the commit machinery
|
||||
// adds nothing when nothing was mutated.
|
||||
func TestAbandonedReadToolKeepsGenericMessage(t *testing.T) {
|
||||
s := newMutationServer()
|
||||
s.ToolCallTimeout = 80 * time.Millisecond
|
||||
handler := func(ctx context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) {
|
||||
<-ctx.Done()
|
||||
return mcp.NewToolResultText("late"), nil
|
||||
}
|
||||
req := mcp.CallToolRequest{}
|
||||
req.Params.Name = "search_symbols"
|
||||
|
||||
res, err := s.boundToolHandler(handler)(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
require.True(t, res.IsError)
|
||||
text := resultText(res)
|
||||
require.Contains(t, text, "treat any side effect as unknown")
|
||||
require.NotContains(t, text, "mutation_commit=")
|
||||
}
|
||||
|
||||
// --- Criterion: the response separates disk state from graph state ---------
|
||||
|
||||
func TestWriteFileResponseSeparatesDiskAndGraphStatus(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "fresh.txt")
|
||||
|
||||
s := newMutationServer()
|
||||
res, err := s.handleWriteFile(context.Background(), writeFileRequest(map[string]any{
|
||||
"path": target, "content": "hello\n",
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
|
||||
payload := resultJSON(t, res)
|
||||
require.Equal(t, mutationDiskCommitted, payload["disk_status"])
|
||||
require.NotEmpty(t, payload["mutation_receipt"])
|
||||
// No indexer is attached, so the graph genuinely cannot be refreshed. That
|
||||
// is reported as stale — separately from the committed disk write, which is
|
||||
// the distinction the issue asks for.
|
||||
require.Equal(t, mutationGraphStale, payload["graph_status"])
|
||||
require.NotEmpty(t, payload["new_sha"])
|
||||
}
|
||||
|
||||
// --- Criterion: a queryable receipt ----------------------------------------
|
||||
|
||||
func TestMutationStatusResolvesReceiptPathAndListing(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "queried.txt")
|
||||
|
||||
s := newMutationServer()
|
||||
writeRes, err := s.handleWriteFile(context.Background(), writeFileRequest(map[string]any{
|
||||
"path": target, "content": "content\n",
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
receipt, _ := resultJSON(t, writeRes)["mutation_receipt"].(string)
|
||||
require.NotEmpty(t, receipt)
|
||||
|
||||
status := func(args map[string]any) map[string]any {
|
||||
req := mcp.CallToolRequest{}
|
||||
req.Params.Name = "mutation_status"
|
||||
req.Params.Arguments = args
|
||||
res, statusErr := s.handleMutationStatus(context.Background(), req)
|
||||
require.NoError(t, statusErr)
|
||||
return resultJSON(t, res)
|
||||
}
|
||||
|
||||
byReceipt := status(map[string]any{"receipt": receipt})
|
||||
require.Equal(t, true, byReceipt["found"])
|
||||
require.Equal(t, mutationDiskCommitted, byReceipt["disk_status"])
|
||||
require.Equal(t, false, byReceipt["retry_safe"])
|
||||
require.Contains(t, byReceipt["guidance"], "do not re-apply")
|
||||
|
||||
byPath := status(map[string]any{"path": target})
|
||||
require.Equal(t, receipt, byPath["receipt"])
|
||||
|
||||
listing := status(map[string]any{})
|
||||
require.Equal(t, float64(1), listing["count"])
|
||||
|
||||
unknown := status(map[string]any{"path": filepath.Join(dir, "never.txt")})
|
||||
require.Equal(t, false, unknown["found"])
|
||||
require.Equal(t, "unrecorded", unknown["disk_status"])
|
||||
}
|
||||
|
||||
func TestMutationStatusUnknownReceiptIsAnError(t *testing.T) {
|
||||
s := newMutationServer()
|
||||
req := mcp.CallToolRequest{}
|
||||
req.Params.Name = "mutation_status"
|
||||
req.Params.Arguments = map[string]any{"receipt": "commit-does-not-exist"}
|
||||
res, err := s.handleMutationStatus(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
require.True(t, res.IsError)
|
||||
require.Contains(t, resultText(res), "no mutation receipt")
|
||||
}
|
||||
|
||||
// --- Criterion: retrying after a timeout is idempotent ---------------------
|
||||
|
||||
// The concrete failure from the issue: a call is abandoned, the client retries,
|
||||
// and the same logical change lands twice. With a mutation_id the retry replays
|
||||
// the original result and writes nothing.
|
||||
func TestEditFileRetryWithMutationIDDoesNotDuplicate(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "fields.go")
|
||||
require.NoError(t, os.WriteFile(target, []byte("type T struct {\n\ta int\n}\n"), 0o644))
|
||||
|
||||
s := newMutationServer()
|
||||
args := map[string]any{
|
||||
"path": target,
|
||||
"old_string": "\ta int\n",
|
||||
"new_string": "\ta int\n\texistenceSet map[string]bool\n",
|
||||
"mutation_id": "add-existence-set",
|
||||
}
|
||||
|
||||
first, err := s.handleEditFile(context.Background(), editFileRequest(args))
|
||||
require.NoError(t, err)
|
||||
firstPayload := resultJSON(t, first)
|
||||
require.Equal(t, "applied", firstPayload["status"])
|
||||
require.Nil(t, firstPayload["replayed"])
|
||||
|
||||
afterFirst, err := os.ReadFile(target)
|
||||
require.NoError(t, err)
|
||||
|
||||
// The client never saw the response and retries the identical edit.
|
||||
second, err := s.handleEditFile(context.Background(), editFileRequest(args))
|
||||
require.NoError(t, err)
|
||||
secondPayload := resultJSON(t, second)
|
||||
require.Equal(t, true, secondPayload["replayed"])
|
||||
require.Equal(t, firstPayload["new_sha"], secondPayload["new_sha"])
|
||||
require.Equal(t, firstPayload["mutation_receipt"], secondPayload["mutation_receipt"])
|
||||
|
||||
afterSecond, err := os.ReadFile(target)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, string(afterFirst), string(afterSecond))
|
||||
require.Equal(t, 1, strings.Count(string(afterSecond), "existenceSet"),
|
||||
"the retry must not duplicate the field")
|
||||
}
|
||||
|
||||
// Without a key the tools stay as they were — the retry re-applies. This is the
|
||||
// documented boundary of the guarantee, asserted so it cannot drift silently.
|
||||
func TestWriteFileRetryWithoutMutationIDStillWrites(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "unkeyed.txt")
|
||||
|
||||
s := newMutationServer()
|
||||
req := writeFileRequest(map[string]any{"path": target, "content": "one\n"})
|
||||
_, err := s.handleWriteFile(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
|
||||
second, err := s.handleWriteFile(context.Background(), writeFileRequest(map[string]any{
|
||||
"path": target, "content": "two\n",
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
require.Nil(t, resultJSON(t, second)["replayed"])
|
||||
|
||||
content, err := os.ReadFile(target)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "two\n", string(content))
|
||||
}
|
||||
|
||||
// A key reused for a different edit is a caller bug, not a retry. Treating it
|
||||
// as a new mutation would silently drop one of the two edits.
|
||||
func TestMutationIDReusedForDifferentEditIsRefused(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "conflict.txt")
|
||||
require.NoError(t, os.WriteFile(target, []byte("a\nb\n"), 0o644))
|
||||
|
||||
s := newMutationServer()
|
||||
_, err := s.handleEditFile(context.Background(), editFileRequest(map[string]any{
|
||||
"path": target, "old_string": "a", "new_string": "A", "mutation_id": "k1",
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
|
||||
res, err := s.handleEditFile(context.Background(), editFileRequest(map[string]any{
|
||||
"path": target, "old_string": "b", "new_string": "B", "mutation_id": "k1",
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
require.True(t, res.IsError)
|
||||
require.Contains(t, resultText(res), "already used for a different edit")
|
||||
|
||||
content, err := os.ReadFile(target)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "A\nb\n", string(content), "the refused edit must not have been applied")
|
||||
}
|
||||
|
||||
// Concurrent retries of one keyed mutation must write once. The path lock
|
||||
// serialises them; the replay check runs inside it.
|
||||
func TestConcurrentKeyedRetriesWriteOnce(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
target := filepath.Join(dir, "race.txt")
|
||||
require.NoError(t, os.WriteFile(target, []byte("x\n"), 0o644))
|
||||
|
||||
s := newMutationServer()
|
||||
args := map[string]any{
|
||||
"path": target, "old_string": "x", "new_string": "x\ny", "mutation_id": "once",
|
||||
}
|
||||
|
||||
const callers = 8
|
||||
var wg sync.WaitGroup
|
||||
results := make([]map[string]any, callers)
|
||||
for i := range callers {
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
res, callErr := s.handleEditFile(context.Background(), editFileRequest(args))
|
||||
if callErr == nil && res != nil && !res.IsError {
|
||||
var payload map[string]any
|
||||
if json.Unmarshal([]byte(resultText(res)), &payload) == nil {
|
||||
results[i] = payload
|
||||
}
|
||||
}
|
||||
}()
|
||||
}
|
||||
wg.Wait()
|
||||
|
||||
applied := 0
|
||||
for _, payload := range results {
|
||||
require.NotNil(t, payload)
|
||||
if payload["replayed"] == nil {
|
||||
applied++
|
||||
}
|
||||
}
|
||||
require.Equal(t, 1, applied, "exactly one caller may perform the write")
|
||||
|
||||
content, err := os.ReadFile(target)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "x\ny\n", string(content))
|
||||
}
|
||||
|
||||
// --- Ledger mechanics ------------------------------------------------------
|
||||
|
||||
// A pending graph refresh must not read as pending forever: mutation_status
|
||||
// re-reads the underlying watcher receipt before answering.
|
||||
func TestMutationStatusRefreshesPendingGraphStatus(t *testing.T) {
|
||||
s := newMutationServer()
|
||||
record := s.beginMutationCommit(context.Background(), "edit_file", "", "fp", "pkg/a.go", "/abs/pkg/a.go")
|
||||
record.markCommitted("sha", 10)
|
||||
|
||||
ticket := &mutationReceipt{id: "mutation-test", generation: 7, done: make(chan struct{})}
|
||||
s.mutationReceipts.Store(ticket.id, ticket)
|
||||
record.recordGraph(mutationReindexOutcome{Pending: true, Receipt: ticket.id, Generation: 7})
|
||||
require.Equal(t, mutationGraphPending, record.snapshot().GraphStatus)
|
||||
|
||||
// The watcher finishes after the response was already sent.
|
||||
ticket.result = indexer.MutationResult{RequestedGeneration: 7, AppliedGeneration: 7, Reindexed: true}
|
||||
ticket.completed = true
|
||||
close(ticket.done)
|
||||
|
||||
payload := s.mutationStatusPayload(record)
|
||||
require.Equal(t, mutationGraphFresh, payload["graph_status"])
|
||||
require.Equal(t, mutationDiskCommitted, payload["disk_status"])
|
||||
}
|
||||
|
||||
func TestMutationLedgerEvictsOldestPastCap(t *testing.T) {
|
||||
s := newMutationServer()
|
||||
for i := range maxMutationCommits + 10 {
|
||||
s.beginMutationCommit(context.Background(), "write_file", "", "fp",
|
||||
fmt.Sprintf("f%d.go", i), fmt.Sprintf("/abs/f%d.go", i))
|
||||
}
|
||||
s.mutationCommits.mu.Lock()
|
||||
held := len(s.mutationCommits.ordered)
|
||||
indexed := len(s.mutationCommits.byID)
|
||||
s.mutationCommits.mu.Unlock()
|
||||
|
||||
require.LessOrEqual(t, held, maxMutationCommits)
|
||||
require.Equal(t, held, indexed, "the id index must not outlive the ordered list")
|
||||
|
||||
_, ok := s.mutationCommits.recentForPath("f0.go")
|
||||
require.False(t, ok, "the oldest record must have been evicted")
|
||||
_, ok = s.mutationCommits.recentForPath(fmt.Sprintf("f%d.go", maxMutationCommits+9))
|
||||
require.True(t, ok, "the newest record must survive")
|
||||
}
|
||||
|
||||
func TestGraphStatusForCoversEveryOutcome(t *testing.T) {
|
||||
require.Equal(t, mutationGraphFailed, graphStatusFor(mutationReindexOutcome{Err: context.Canceled}))
|
||||
require.Equal(t, mutationGraphPending, graphStatusFor(mutationReindexOutcome{Pending: true}))
|
||||
require.Equal(t, mutationGraphFresh, graphStatusFor(mutationReindexOutcome{Reindexed: true}))
|
||||
require.Equal(t, mutationGraphStale, graphStatusFor(mutationReindexOutcome{}))
|
||||
}
|
||||
|
||||
// --- Multi-file writes -----------------------------------------------------
|
||||
|
||||
// A coordinated rename must not keep writing files after its request is dead.
|
||||
// Grinding on would spread a half-applied rename across more files than
|
||||
// necessary; stopping leaves receipts that say exactly how far it got.
|
||||
func TestRenameStopsWritingOnceCancelled(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
writes := make([]*renameFileWrite, 0, 3)
|
||||
for _, name := range []string{"a.go", "b.go", "c.go"} {
|
||||
abs := filepath.Join(dir, name)
|
||||
require.NoError(t, os.WriteFile(abs, []byte("package p // old\n"), 0o644))
|
||||
writes = append(writes, &renameFileWrite{
|
||||
RelPath: name, AbsPath: abs, New: []byte("package p // new\n"), NewSHA: "sha-" + name,
|
||||
})
|
||||
}
|
||||
|
||||
s := newMutationServer()
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
// Let the first file commit, then kill the request inside the second.
|
||||
var seen int
|
||||
s.mutationPreCommitHook = func(*mutationCommitRecord) {
|
||||
seen++
|
||||
if seen == 2 {
|
||||
cancel()
|
||||
}
|
||||
}
|
||||
|
||||
results := s.commitRenameWrites(ctx, writes)
|
||||
require.Len(t, results, 2, "the rename must stop instead of attempting every remaining file")
|
||||
require.Equal(t, "applied", results[0]["status"])
|
||||
require.Equal(t, mutationDiskCommitted, results[0]["disk_status"])
|
||||
require.Equal(t, "failed", results[1]["status"])
|
||||
require.Equal(t, mutationDiskNotApplied, results[1]["disk_status"])
|
||||
|
||||
first, err := os.ReadFile(writes[0].AbsPath)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "package p // new\n", string(first))
|
||||
for _, w := range writes[1:] {
|
||||
content, readErr := os.ReadFile(w.AbsPath)
|
||||
require.NoError(t, readErr)
|
||||
require.Equal(t, "package p // old\n", string(content), "%s must be untouched", w.RelPath)
|
||||
}
|
||||
}
|
||||
@@ -2,13 +2,13 @@ package mcp
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"sort"
|
||||
"strings"
|
||||
"unicode/utf8"
|
||||
|
||||
"github.com/zzet/gortex/internal/agents"
|
||||
"github.com/zzet/gortex/internal/indexer"
|
||||
)
|
||||
|
||||
@@ -422,19 +422,30 @@ func (s *Server) commitRenameWrites(ctx context.Context, writes []*renameFileWri
|
||||
if info, err := os.Stat(w.AbsPath); err == nil {
|
||||
perm = info.Mode().Perm()
|
||||
}
|
||||
if err := agents.AtomicWriteFile(w.AbsPath, w.New, perm); err != nil {
|
||||
// A multi-file rename stops at the first cancelled write rather than
|
||||
// grinding through the rest: every remaining file would be refused
|
||||
// anyway, and the receipts already recorded say exactly how far the
|
||||
// rename got.
|
||||
commit, err := s.commitFileMutation(ctx, "rename_symbol", "", "", w.RelPath, w.AbsPath, w.New, perm)
|
||||
if err != nil {
|
||||
entry["status"] = "failed"
|
||||
entry["error"] = err.Error()
|
||||
attachMutationCommit(entry, commit)
|
||||
results = append(results, entry)
|
||||
if errors.Is(err, errMutationNotApplied) {
|
||||
return results
|
||||
}
|
||||
continue
|
||||
}
|
||||
entry["status"] = "applied"
|
||||
sess.recordModified(w.RelPath)
|
||||
outcome := s.mutationReindexState(ctx, w.AbsPath)
|
||||
commit.recordGraph(outcome)
|
||||
if outcome.Err != nil {
|
||||
entry["reindex_error"] = outcome.Err.Error()
|
||||
}
|
||||
s.attachMutationFreshness(entry, w.RelPath, w.AbsPath, outcome)
|
||||
attachMutationCommit(entry, commit)
|
||||
results = append(results, entry)
|
||||
}
|
||||
return results
|
||||
|
||||
@@ -96,6 +96,7 @@ var defaultToolScopes = map[string]ToolScope{
|
||||
"edit_symbol": ScopeRepo,
|
||||
"edit_file": ScopeRepo,
|
||||
"write_file": ScopeRepo,
|
||||
"mutation_status": ScopeRepo,
|
||||
"rename_symbol": ScopeRepo,
|
||||
"scaffold": ScopeRepo,
|
||||
"suggest_pattern": ScopeRepo,
|
||||
|
||||
@@ -365,6 +365,22 @@ type Server struct {
|
||||
mutationReindexWait time.Duration
|
||||
mutationSafetyWait time.Duration
|
||||
|
||||
// mutationCommits is the durable disk-commit ledger for the single-file
|
||||
// mutating tools (mutation_commit.go). It answers the question the
|
||||
// transport cannot: when a tool call is abandoned at its deadline, did the
|
||||
// bytes actually land? The zero value is usable, so directly-constructed
|
||||
// test and embedded servers need no constructor wiring.
|
||||
mutationCommits mutationCommitLedger
|
||||
|
||||
// mutationPreCommitHook is a fault-injection seam, nil in production. It
|
||||
// fires between registering a commit and the cancellation gate that guards
|
||||
// the write — the window a real deadline hits when it lands after the
|
||||
// mutation lock was taken but before the bytes are renamed into place.
|
||||
// That window cannot be entered from outside: the handler crosses it in
|
||||
// microseconds, so without this seam the gate is untestable and could
|
||||
// regress unnoticed.
|
||||
mutationPreCommitHook func(*mutationCommitRecord)
|
||||
|
||||
// batchTransactions holds daemon-lifetime delivery receipts for atomic
|
||||
// batch edits. sync.Map's zero value keeps directly-constructed test and
|
||||
// embedded servers usable without constructor wiring. The write/remove
|
||||
|
||||
@@ -2,6 +2,7 @@ package mcp
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
|
||||
"fmt"
|
||||
@@ -108,7 +109,7 @@ func boundHandler[Req, Res any](
|
||||
kind, name string,
|
||||
h func(context.Context, Req) (Res, error),
|
||||
busy func(stuck int64, timeout time.Duration) (Res, error),
|
||||
expired func(timeout time.Duration) (Res, error),
|
||||
expired func(timeout time.Duration, note *mutationCommitNote) (Res, error),
|
||||
panicked func(recovered any) (Res, error),
|
||||
) func(context.Context, Req) (Res, error) {
|
||||
return func(ctx context.Context, req Req) (Res, error) {
|
||||
@@ -134,6 +135,11 @@ func boundHandler[Req, Res any](
|
||||
|
||||
callCtx, cancel := context.WithTimeout(ctx, timeout)
|
||||
defer cancel()
|
||||
// Every call carries a commit note. A read handler never writes to it
|
||||
// and pays one context value; a mutating handler stamps its disk commit
|
||||
// there, which is the only way this frame can say what landed after it
|
||||
// has stopped waiting for the handler.
|
||||
callCtx, commitNote := withMutationCommitNote(callCtx)
|
||||
|
||||
type outcome struct {
|
||||
res Res
|
||||
@@ -193,7 +199,7 @@ func boundHandler[Req, Res any](
|
||||
zap.Duration("elapsed", time.Since(started)),
|
||||
zap.Int64("abandoned_in_flight", stuck))
|
||||
}
|
||||
return expired(timeout)
|
||||
return expired(timeout, commitNote)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -220,6 +226,48 @@ func abandonedMessage(what, name string, timeout time.Duration) string {
|
||||
what, name, timeout)
|
||||
}
|
||||
|
||||
// abandonedToolMessage is abandonedMessage plus what the handler managed to do
|
||||
// to the filesystem before this frame gave up on it.
|
||||
//
|
||||
// The generic "treat any side effect as unknown" wording is correct only when
|
||||
// nothing is known. Once a disk commit has been confirmed it is actively
|
||||
// harmful: a client that reads it as "nothing happened" retries, or falls back
|
||||
// to its own editor, and applies the same logical change twice. So the three
|
||||
// states are reported separately and the JSON tail is machine-readable.
|
||||
func abandonedToolMessage(name string, timeout time.Duration, verdict mutationCommitVerdict) string {
|
||||
base := abandonedMessage("tool", name, timeout)
|
||||
switch verdict.Status {
|
||||
case "":
|
||||
// No mutating handler registered a commit, so nothing was written by
|
||||
// this call. Read tools land here, and so does a write refused during
|
||||
// validation.
|
||||
return base
|
||||
case mutationDiskNotApplied:
|
||||
base = fmt.Sprintf(
|
||||
"tool %q exceeded its %s deadline and was abandoned so the session stays responsive. "+
|
||||
"No filesystem change was applied — the handler stopped at its cancellation gate before the disk commit, "+
|
||||
"so retrying is safe. This usually means the graph is busy (indexing, enrichment, or a slow store) — "+
|
||||
"retry, or check `gortex daemon status`.",
|
||||
name, timeout)
|
||||
case mutationDiskCommitted:
|
||||
base = fmt.Sprintf(
|
||||
"tool %q exceeded its %s deadline, but its filesystem change WAS COMMITTED before the deadline fired. "+
|
||||
"Do NOT retry this edit and do NOT apply it by another route — you would duplicate it. "+
|
||||
"Only the graph refresh is still outstanding; the files below are already on disk with the listed SHAs.",
|
||||
name, timeout)
|
||||
case mutationDiskInFlight:
|
||||
base = fmt.Sprintf(
|
||||
"tool %q exceeded its %s deadline while a filesystem write was in progress, so the outcome is genuinely unknown. "+
|
||||
"Do not retry blindly: query the receipt below (`mutation_status`, or `edit` with operation:\"receipt\") "+
|
||||
"to learn whether the bytes landed.",
|
||||
name, timeout)
|
||||
}
|
||||
if encoded, err := json.Marshal(verdict); err == nil {
|
||||
base += "\nmutation_commit=" + string(encoded)
|
||||
}
|
||||
return base
|
||||
}
|
||||
|
||||
func (s *Server) boundToolHandler(h mcpserver.ToolHandlerFunc) mcpserver.ToolHandlerFunc {
|
||||
return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
|
||||
name := req.Params.Name
|
||||
@@ -227,8 +275,8 @@ func (s *Server) boundToolHandler(h mcpserver.ToolHandlerFunc) mcpserver.ToolHan
|
||||
func(stuck int64, timeout time.Duration) (*mcp.CallToolResult, error) {
|
||||
return mcp.NewToolResultError(busyMessage("tool calls", stuck, timeout)), nil
|
||||
},
|
||||
func(timeout time.Duration) (*mcp.CallToolResult, error) {
|
||||
return mcp.NewToolResultError(abandonedMessage("tool", name, timeout)), nil
|
||||
func(timeout time.Duration, note *mutationCommitNote) (*mcp.CallToolResult, error) {
|
||||
return mcp.NewToolResultError(abandonedToolMessage(name, timeout, note.verdict())), nil
|
||||
},
|
||||
func(r any) (*mcp.CallToolResult, error) {
|
||||
return mcp.NewToolResultError(fmt.Sprintf("tool %q internal error: %v", name, r)), nil
|
||||
@@ -270,7 +318,7 @@ func (s *Server) boundResourceHandler(uri string, h mcpserver.ResourceHandlerFun
|
||||
func(stuck int64, timeout time.Duration) ([]mcp.ResourceContents, error) {
|
||||
return nil, errors.New(busyMessage("resource reads", stuck, timeout))
|
||||
},
|
||||
func(timeout time.Duration) ([]mcp.ResourceContents, error) {
|
||||
func(timeout time.Duration, _ *mutationCommitNote) ([]mcp.ResourceContents, error) {
|
||||
return nil, errors.New(abandonedMessage("resource", uri, timeout))
|
||||
},
|
||||
func(r any) ([]mcp.ResourceContents, error) {
|
||||
@@ -301,7 +349,7 @@ func (s *Server) boundPromptHandler(name string, h mcpserver.PromptHandlerFunc)
|
||||
func(stuck int64, timeout time.Duration) (*mcp.GetPromptResult, error) {
|
||||
return nil, errors.New(busyMessage("prompt requests", stuck, timeout))
|
||||
},
|
||||
func(timeout time.Duration) (*mcp.GetPromptResult, error) {
|
||||
func(timeout time.Duration, _ *mutationCommitNote) (*mcp.GetPromptResult, error) {
|
||||
return nil, errors.New(abandonedMessage("prompt", name, timeout))
|
||||
},
|
||||
func(r any) (*mcp.GetPromptResult, error) {
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"bufio"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
@@ -12,7 +13,6 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
"github.com/zzet/gortex/internal/agents"
|
||||
"github.com/zzet/gortex/internal/elide"
|
||||
"github.com/zzet/gortex/internal/graph"
|
||||
"github.com/zzet/gortex/internal/indexer"
|
||||
@@ -127,6 +127,7 @@ func (s *Server) registerCodingTools() {
|
||||
mcp.WithBoolean("dry_run", mcp.Description("Validate the edit and return a unified-diff preview without writing (status: would_apply). Use to review the change before committing it.")),
|
||||
mcp.WithBoolean("physical_evidence", mcp.Description(mutationEvidenceParamDescription)),
|
||||
mcp.WithString("digest", mcp.Description(evidenceDigestParamDescription)),
|
||||
mcp.WithString("mutation_id", mcp.Description(mutationIDParamDescription)),
|
||||
),
|
||||
s.handleEditSymbol,
|
||||
)
|
||||
@@ -163,6 +164,7 @@ func (s *Server) registerCodingTools() {
|
||||
mcp.WithBoolean("allow_parse_errors", mcp.Description("Bypass the pre-write parse gate. By default an edit that would introduce new tree-sitter parse errors (leaving the file more syntactically broken than before) is refused before the atomic write; set true to write anyway.")),
|
||||
mcp.WithBoolean("physical_evidence", mcp.Description(mutationEvidenceParamDescription)),
|
||||
mcp.WithString("digest", mcp.Description(evidenceDigestParamDescription)),
|
||||
mcp.WithString("mutation_id", mcp.Description(mutationIDParamDescription)),
|
||||
),
|
||||
s.handleEditFile,
|
||||
)
|
||||
@@ -177,10 +179,21 @@ func (s *Server) registerCodingTools() {
|
||||
mcp.WithBoolean("allow_parse_errors", mcp.Description("Bypass the pre-write parse gate. By default a write that would introduce new tree-sitter parse errors (relative to the prior content, or any error in a brand-new file) is refused before the atomic write; set true to write anyway.")),
|
||||
mcp.WithBoolean("physical_evidence", mcp.Description(mutationEvidenceParamDescription)),
|
||||
mcp.WithString("digest", mcp.Description(evidenceDigestParamDescription)),
|
||||
mcp.WithString("mutation_id", mcp.Description(mutationIDParamDescription)),
|
||||
),
|
||||
s.handleWriteFile,
|
||||
)
|
||||
|
||||
s.addTool(
|
||||
mcp.NewTool("mutation_status",
|
||||
mcp.WithDescription("Reports what a file mutation actually did, after the fact. Use it when an edit_file / write_file / edit_symbol call was abandoned at its deadline (\"the work may still complete in the background\"): instead of retrying blindly, ask here. Returns disk_status (committed / not_applied / failed / in_flight) separately from graph_status (fresh / pending / stale / failed), because the bytes reaching disk and the graph catching up are different questions. disk_status=committed means the edit is already applied — re-applying it by any route would duplicate it. Select a record by receipt, by mutation_id, or by path; with no argument it lists the most recent mutations. Receipts are retained for 30 minutes."),
|
||||
mcp.WithString("receipt", mcp.Description("Mutation receipt id from an edit response (`mutation_receipt`) or from the `mutation_commit=` block on an abandoned-call error.")),
|
||||
mcp.WithString("mutation_id", mcp.Description("Caller-chosen idempotency key passed to the original edit.")),
|
||||
mcp.WithString("path", mcp.Description("File path — returns the most recent mutation recorded for it. Use when the response was lost entirely and no receipt id was ever seen.")),
|
||||
),
|
||||
s.handleMutationStatus,
|
||||
)
|
||||
|
||||
s.addTool(
|
||||
mcp.NewTool("rename_symbol",
|
||||
mcp.WithDescription("Applies a coordinated multi-file rename for a symbol. Rewrites the definition, every graph usage (calls / references / instantiates), receiver lines when renaming a type, and test-function names that embed the old identifier. Returns status (applied / would_apply / no_edits), the {file, line, old_text, new_text, confidence, reason} edit list, and per-file {bytes_written, new_sha, reindexed}. An existing but unindexed target returns a structured symbol_not_indexed error only when the configured extractor anchors the requested declaration; its guarded edit/file fallback is an exact declaration-line edit and does not claim reference completeness. The refusal itself writes nothing. Every affected line is re-verified against disk and parse-gated before anything is written, so the rename either lands completely or is refused. Pass dry_run=true to preview the identical edit list without writing."),
|
||||
@@ -2824,6 +2837,7 @@ func (s *Server) handleEditSymbol(ctx context.Context, req mcp.CallToolRequest)
|
||||
if evidenceRequested && dryRun {
|
||||
return mcp.NewToolResultError(errEvidenceDryRun), nil
|
||||
}
|
||||
mutationID := strings.TrimSpace(req.GetString("mutation_id", ""))
|
||||
|
||||
if oldSource == newSource {
|
||||
return mcp.NewToolResultError("old_source and new_source are identical"), nil
|
||||
@@ -2849,6 +2863,17 @@ func (s *Server) handleEditSymbol(ctx context.Context, req mcp.CallToolRequest)
|
||||
}
|
||||
defer releaseMutation()
|
||||
|
||||
// Replay under the path lock, before the snapshot read: once the edit has
|
||||
// landed old_source no longer matches, so an un-replayed retry would be
|
||||
// refused rather than answered with the original result.
|
||||
fingerprint := mutationFingerprint("edit_symbol", id, absPath, oldSource, newSource)
|
||||
if replay, conflict, matched := s.replayMutation(mutationID, fingerprint); matched {
|
||||
if conflict != "" {
|
||||
return mcp.NewToolResultError(conflict), nil
|
||||
}
|
||||
return s.respondJSONOrTOON(ctx, req, replay)
|
||||
}
|
||||
|
||||
// Read the entire file ONCE — both the drift check and the
|
||||
// patch operate on the same byte snapshot so a concurrent
|
||||
// writer cannot wedge a diff between the SHA we accept and the
|
||||
@@ -2989,14 +3014,19 @@ func (s *Server) handleEditSymbol(ctx context.Context, req mcp.CallToolRequest)
|
||||
if info, statErr := os.Stat(absPath); statErr == nil {
|
||||
perm = info.Mode().Perm()
|
||||
}
|
||||
if err := agents.AtomicWriteFile(absPath, newContentBytes, perm); err != nil {
|
||||
return mcp.NewToolResultError(fmt.Sprintf("could not write file: %v", err)), nil
|
||||
commit, writeErr := s.commitFileMutation(ctx, "edit_symbol", mutationID, fingerprint, node.FilePath, absPath, newContentBytes, perm)
|
||||
if writeErr != nil {
|
||||
if errors.Is(writeErr, errMutationNotApplied) {
|
||||
return mcp.NewToolResultError(mutationNotAppliedMessage("edit", commit, writeErr)), nil
|
||||
}
|
||||
return mcp.NewToolResultError(fmt.Sprintf("could not write file: %v", writeErr)), nil
|
||||
}
|
||||
sess := s.sessionFor(ctx)
|
||||
sess.recordModified(node.FilePath)
|
||||
sess.recordSymbol(id)
|
||||
|
||||
reindexOutcome := s.mutationReindexState(ctx, absPath)
|
||||
commit.recordGraph(reindexOutcome)
|
||||
|
||||
// Count lines changed.
|
||||
oldLines := strings.Count(oldSource, "\n") + 1
|
||||
@@ -3021,6 +3051,10 @@ func (s *Server) handleEditSymbol(ctx context.Context, req mcp.CallToolRequest)
|
||||
if evidenceRequested {
|
||||
s.attachMutationPhysicalEvidence(resp, absPath, content, true)
|
||||
}
|
||||
attachMutationCommit(resp, commit)
|
||||
// Retained last so a replayed retry returns the complete original payload,
|
||||
// physical evidence included.
|
||||
commit.retainResponse(resp)
|
||||
return s.respondJSONOrTOON(ctx, req, resp)
|
||||
}
|
||||
|
||||
|
||||
@@ -14,7 +14,6 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
"github.com/zzet/gortex/internal/agents"
|
||||
"github.com/zzet/gortex/internal/analysis"
|
||||
"github.com/zzet/gortex/internal/audit"
|
||||
"github.com/zzet/gortex/internal/blame"
|
||||
@@ -4069,7 +4068,8 @@ func (s *Server) applyBatchSymbolEdit(ctx context.Context, edit batchEditItem, w
|
||||
if info, statErr := os.Stat(absPath); statErr == nil {
|
||||
perm = info.Mode().Perm()
|
||||
}
|
||||
if writeErr := agents.AtomicWriteFile(absPath, []byte(newContent), perm); writeErr != nil {
|
||||
commit, writeErr := s.commitFileMutation(ctx, "batch_edit", "", "", node.FilePath, absPath, []byte(newContent), perm)
|
||||
if writeErr != nil {
|
||||
res.Status, res.Error = "failed", fmt.Sprintf("could not write file: %v", writeErr)
|
||||
return res
|
||||
}
|
||||
@@ -4077,6 +4077,7 @@ func (s *Server) applyBatchSymbolEdit(ctx context.Context, edit batchEditItem, w
|
||||
sess.recordModified(node.FilePath)
|
||||
sess.recordSymbol(edit.SymbolID)
|
||||
reindexOutcome := s.mutationReindexState(ctx, absPath)
|
||||
commit.recordGraph(reindexOutcome)
|
||||
res.Reindexed, res.ReindexPending = reindexOutcome.Reindexed, reindexOutcome.Pending
|
||||
res.ReindexReceipt = reindexOutcome.Receipt
|
||||
res.ReindexGeneration = reindexOutcome.Generation
|
||||
@@ -4162,12 +4163,14 @@ func (s *Server) applyBatchFileEdit(ctx context.Context, edit batchEditItem, wri
|
||||
if info, statErr := os.Stat(absPath); statErr == nil {
|
||||
perm = info.Mode().Perm()
|
||||
}
|
||||
if writeErr := agents.AtomicWriteFile(absPath, []byte(newContent), perm); writeErr != nil {
|
||||
commit, writeErr := s.commitFileMutation(ctx, "batch_edit", "", "", relPath, absPath, []byte(newContent), perm)
|
||||
if writeErr != nil {
|
||||
res.Status, res.Error = "failed", fmt.Sprintf("could not write file: %v", writeErr)
|
||||
return res
|
||||
}
|
||||
s.sessionFor(ctx).recordModified(relPath)
|
||||
reindexOutcome := s.mutationReindexState(ctx, absPath)
|
||||
commit.recordGraph(reindexOutcome)
|
||||
res.Reindexed, res.ReindexPending = reindexOutcome.Reindexed, reindexOutcome.Pending
|
||||
res.ReindexReceipt = reindexOutcome.Receipt
|
||||
res.ReindexGeneration = reindexOutcome.Generation
|
||||
|
||||
@@ -11,13 +11,13 @@ import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"sort"
|
||||
"strconv"
|
||||
"strings"
|
||||
"time"
|
||||
"unicode/utf8"
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
|
||||
"github.com/zzet/gortex/internal/agents"
|
||||
"github.com/zzet/gortex/internal/elide"
|
||||
"github.com/zzet/gortex/internal/graph"
|
||||
"github.com/zzet/gortex/internal/indexer"
|
||||
@@ -758,6 +758,7 @@ func (s *Server) handleEditFile(ctx context.Context, req mcp.CallToolRequest) (*
|
||||
if evidenceRequested && dryRun {
|
||||
return mcp.NewToolResultError(errEvidenceDryRun), nil
|
||||
}
|
||||
mutationID := strings.TrimSpace(req.GetString("mutation_id", ""))
|
||||
|
||||
absPath, relPath, resolveErr := s.resolveFilePath(rawPath)
|
||||
if resolveErr != nil {
|
||||
@@ -769,6 +770,17 @@ func (s *Server) handleEditFile(ctx context.Context, req mcp.CallToolRequest) (*
|
||||
}
|
||||
defer releaseMutation()
|
||||
|
||||
// Replay before reading the file: a landed edit has already consumed
|
||||
// old_string, so an un-replayed retry would fail the match check with a
|
||||
// confusing "not found" instead of returning the original result.
|
||||
fingerprint := mutationFingerprint("edit_file", absPath, oldString, newString, strconv.FormatBool(replaceAll))
|
||||
if replay, conflict, matched := s.replayMutation(mutationID, fingerprint); matched {
|
||||
if conflict != "" {
|
||||
return mcp.NewToolResultError(conflict), nil
|
||||
}
|
||||
return s.respondJSONOrTOON(ctx, req, replay)
|
||||
}
|
||||
|
||||
content, err := os.ReadFile(absPath)
|
||||
if err != nil {
|
||||
return mcp.NewToolResultError(fmt.Sprintf("could not read file: %v", err)), nil
|
||||
@@ -870,14 +882,19 @@ func (s *Server) handleEditFile(ctx context.Context, req mcp.CallToolRequest) (*
|
||||
if info, err := os.Stat(absPath); err == nil {
|
||||
perm = info.Mode().Perm()
|
||||
}
|
||||
if err := agents.AtomicWriteFile(absPath, newContentBytes, perm); err != nil {
|
||||
return mcp.NewToolResultError(fmt.Sprintf("could not write file: %v", err)), nil
|
||||
commit, writeErr := s.commitFileMutation(ctx, "edit_file", mutationID, fingerprint, relPath, absPath, newContentBytes, perm)
|
||||
if writeErr != nil {
|
||||
if errors.Is(writeErr, errMutationNotApplied) {
|
||||
return mcp.NewToolResultError(mutationNotAppliedMessage("edit", commit, writeErr)), nil
|
||||
}
|
||||
return mcp.NewToolResultError(fmt.Sprintf("could not write file: %v", writeErr)), nil
|
||||
}
|
||||
|
||||
sess := s.sessionFor(ctx)
|
||||
sess.recordModified(relPath)
|
||||
|
||||
reindexOutcome := s.mutationReindexState(ctx, absPath)
|
||||
commit.recordGraph(reindexOutcome)
|
||||
|
||||
resp := map[string]any{
|
||||
"path": relPath,
|
||||
@@ -899,6 +916,10 @@ func (s *Server) handleEditFile(ctx context.Context, req mcp.CallToolRequest) (*
|
||||
if evidenceRequested {
|
||||
s.attachMutationPhysicalEvidence(resp, absPath, content, true)
|
||||
}
|
||||
attachMutationCommit(resp, commit)
|
||||
// Retained last so a replayed retry returns the complete original payload,
|
||||
// physical evidence included.
|
||||
commit.retainResponse(resp)
|
||||
return s.respondJSONOrTOON(ctx, req, resp)
|
||||
}
|
||||
|
||||
@@ -920,6 +941,7 @@ func (s *Server) handleWriteFile(ctx context.Context, req mcp.CallToolRequest) (
|
||||
if evidenceRequested && dryRun {
|
||||
return mcp.NewToolResultError(errEvidenceDryRun), nil
|
||||
}
|
||||
mutationID := strings.TrimSpace(req.GetString("mutation_id", ""))
|
||||
|
||||
absPath, relPath, resolveErr := s.resolveFilePath(rawPath)
|
||||
if resolveErr != nil {
|
||||
@@ -931,6 +953,17 @@ func (s *Server) handleWriteFile(ctx context.Context, req mcp.CallToolRequest) (
|
||||
}
|
||||
defer releaseMutation()
|
||||
|
||||
// Idempotency runs under the path lock so a retry that races the original
|
||||
// call waits for it and then sees a terminal record, rather than both
|
||||
// deciding independently that they are the first.
|
||||
fingerprint := mutationFingerprint("write_file", absPath, content)
|
||||
if replay, conflict, matched := s.replayMutation(mutationID, fingerprint); matched {
|
||||
if conflict != "" {
|
||||
return mcp.NewToolResultError(conflict), nil
|
||||
}
|
||||
return s.respondJSONOrTOON(ctx, req, replay)
|
||||
}
|
||||
|
||||
status := "created"
|
||||
perm := os.FileMode(0o644)
|
||||
fileExists := false
|
||||
@@ -1012,14 +1045,19 @@ func (s *Server) handleWriteFile(ctx context.Context, req mcp.CallToolRequest) (
|
||||
return s.respondJSONOrTOON(ctx, req, preview)
|
||||
}
|
||||
|
||||
if err := agents.AtomicWriteFile(absPath, contentBytes, perm); err != nil {
|
||||
return mcp.NewToolResultError(fmt.Sprintf("could not write file: %v", err)), nil
|
||||
commit, writeErr := s.commitFileMutation(ctx, "write_file", mutationID, fingerprint, relPath, absPath, contentBytes, perm)
|
||||
if writeErr != nil {
|
||||
if errors.Is(writeErr, errMutationNotApplied) {
|
||||
return mcp.NewToolResultError(mutationNotAppliedMessage("write", commit, writeErr)), nil
|
||||
}
|
||||
return mcp.NewToolResultError(fmt.Sprintf("could not write file: %v", writeErr)), nil
|
||||
}
|
||||
|
||||
sess := s.sessionFor(ctx)
|
||||
sess.recordModified(relPath)
|
||||
|
||||
reindexOutcome := s.mutationReindexState(ctx, absPath)
|
||||
commit.recordGraph(reindexOutcome)
|
||||
|
||||
resp := map[string]any{
|
||||
"path": relPath,
|
||||
@@ -1037,6 +1075,10 @@ func (s *Server) handleWriteFile(ctx context.Context, req mcp.CallToolRequest) (
|
||||
if evidenceRequested {
|
||||
s.attachMutationPhysicalEvidence(resp, absPath, priorContent, fileExists)
|
||||
}
|
||||
attachMutationCommit(resp, commit)
|
||||
// Retained last so a replayed retry returns the complete original payload,
|
||||
// physical evidence included.
|
||||
commit.retainResponse(resp)
|
||||
return s.respondJSONOrTOON(ctx, req, resp)
|
||||
}
|
||||
|
||||
|
||||
@@ -62,6 +62,10 @@ const (
|
||||
// slack. Note the blurbs are shared constants and the schema compactor
|
||||
// is not monotonic in description length — a shorter blurb measured
|
||||
// *larger* here (28551), so shrink by measuring, never by eyeballing.
|
||||
//
|
||||
// The `mutation_id` idempotency key on the same two floor tools then took
|
||||
// another 148 bytes (28527 → 28675), sharing one blurb constant for the same
|
||||
// reason. The ceiling still holds; the remaining slack is ~175 bytes.
|
||||
const agentPresetByteCeiling = 28850
|
||||
|
||||
// localizationPresetByteCeiling is the hard budget for the diet
|
||||
|
||||
@@ -0,0 +1,126 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"context"
|
||||
"strings"
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
)
|
||||
|
||||
// handleMutationStatus answers "what actually happened to my edit?" after a
|
||||
// response was lost — the recovery path for a tool call the deadline wrapper
|
||||
// abandoned (see mutation_commit.go).
|
||||
//
|
||||
// It is read-only. Its whole job is to turn the two unknowns a timed-out client
|
||||
// is left holding into two independent facts: whether the bytes reached disk,
|
||||
// and whether the graph has caught up with them.
|
||||
func (s *Server) handleMutationStatus(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
|
||||
receipt := strings.TrimSpace(req.GetString("receipt", ""))
|
||||
mutationID := strings.TrimSpace(req.GetString("mutation_id", ""))
|
||||
rawPath := strings.TrimSpace(req.GetString("path", ""))
|
||||
|
||||
switch {
|
||||
case receipt != "":
|
||||
record, ok := s.mutationCommits.byReceipt(receipt)
|
||||
if !ok {
|
||||
return mcp.NewToolResultError(
|
||||
"no mutation receipt " + receipt + " — receipts are kept for " + mutationCommitRetention.String() +
|
||||
"; query by path instead, or read the file to see its current state"), nil
|
||||
}
|
||||
return s.respondJSONOrTOON(ctx, req, s.mutationStatusPayload(record))
|
||||
|
||||
case mutationID != "":
|
||||
record, ok := s.mutationCommits.byMutationID(mutationID)
|
||||
if !ok {
|
||||
return mcp.NewToolResultError("no mutation recorded for mutation_id " + mutationID), nil
|
||||
}
|
||||
return s.respondJSONOrTOON(ctx, req, s.mutationStatusPayload(record))
|
||||
|
||||
case rawPath != "":
|
||||
// Path lookup is the last resort a client has when it lost the whole
|
||||
// response and therefore never saw a receipt id. Resolution failure is
|
||||
// not fatal here: the raw spelling is still matched against the ledger.
|
||||
lookup := rawPath
|
||||
if absPath, relPath, err := s.resolveFilePath(rawPath); err == nil {
|
||||
if record, ok := s.mutationCommits.recentForPath(relPath); ok {
|
||||
return s.respondJSONOrTOON(ctx, req, s.mutationStatusPayload(record))
|
||||
}
|
||||
lookup = absPath
|
||||
}
|
||||
record, ok := s.mutationCommits.recentForPath(lookup)
|
||||
if !ok {
|
||||
return s.respondJSONOrTOON(ctx, req, map[string]any{
|
||||
"path": rawPath,
|
||||
"found": false,
|
||||
"disk_status": "unrecorded",
|
||||
"note": "this daemon has no record of a mutation to that path — it was never attempted, " +
|
||||
"it was applied by another process, or the receipt has aged out",
|
||||
})
|
||||
}
|
||||
return s.respondJSONOrTOON(ctx, req, s.mutationStatusPayload(record))
|
||||
}
|
||||
|
||||
records := s.mutationCommits.recent(maxMutationCommitListing)
|
||||
return s.respondJSONOrTOON(ctx, req, map[string]any{
|
||||
"mutations": mutationCommitListing(records),
|
||||
"count": len(records),
|
||||
"note": "most recent first; pass receipt, mutation_id, or path to select one",
|
||||
})
|
||||
}
|
||||
|
||||
// mutationStatusPayload renders one record, refreshing the graph half if the
|
||||
// reindex it was waiting on has since finished. Without the refresh every
|
||||
// pending receipt would read as pending forever, which is precisely the stale
|
||||
// answer this tool exists to avoid.
|
||||
func (s *Server) mutationStatusPayload(record *mutationCommitRecord) map[string]any {
|
||||
if pending := record.pendingReindexReceipt(); pending != "" {
|
||||
if outcome, ok := s.mutationReceiptState(pending); ok {
|
||||
record.recordGraph(outcome)
|
||||
}
|
||||
}
|
||||
snap := record.snapshot()
|
||||
payload := map[string]any{
|
||||
"found": true,
|
||||
"receipt": snap.Receipt,
|
||||
"tool": snap.Tool,
|
||||
"path": snap.Path,
|
||||
"disk_status": snap.DiskStatus,
|
||||
"graph_status": snap.GraphStatus,
|
||||
}
|
||||
if snap.MutationID != "" {
|
||||
payload["mutation_id"] = snap.MutationID
|
||||
}
|
||||
if snap.NewSHA != "" {
|
||||
payload["new_sha"] = snap.NewSHA
|
||||
}
|
||||
if snap.BytesWritten > 0 {
|
||||
payload["bytes_written"] = snap.BytesWritten
|
||||
}
|
||||
if snap.Error != "" {
|
||||
payload["error"] = snap.Error
|
||||
}
|
||||
if snap.StartedAt != "" {
|
||||
payload["started_at"] = snap.StartedAt
|
||||
}
|
||||
if snap.CommittedAt != "" {
|
||||
payload["committed_at"] = snap.CommittedAt
|
||||
}
|
||||
payload["retry_safe"] = snap.DiskStatus == mutationDiskNotApplied || snap.DiskStatus == mutationDiskFailed
|
||||
payload["guidance"] = mutationStatusGuidance(snap.DiskStatus)
|
||||
return payload
|
||||
}
|
||||
|
||||
func mutationStatusGuidance(disk string) string {
|
||||
switch disk {
|
||||
case mutationDiskCommitted:
|
||||
return "the bytes are on disk — do not re-apply this edit; if graph_status is not \"fresh\" the graph is still catching up"
|
||||
case mutationDiskNotApplied:
|
||||
return "nothing was written — the file is unchanged and retrying is safe"
|
||||
case mutationDiskFailed:
|
||||
return "the write was attempted and failed — the file still holds its previous content, so retrying is safe"
|
||||
case mutationDiskInFlight:
|
||||
return "the write has not reported back yet — poll this receipt rather than retrying"
|
||||
default:
|
||||
return ""
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user