fix(tools): say when format-on-write ran out of time

internal/tools/edit_file.go

@@ -160,7 +160,8 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any
 	// Optional format-on-write (ZERO_FORMAT_ON_WRITE). Must run BEFORE the
 	// FileTracker re-baseline: recording pre-format content would make the very
 	// next edit look like an external modification and trip the conflict guard.
-	updated = maybeFormatWrittenFile(ctx, absolutePath, updated)
+	formatting := maybeFormatWrittenFile(ctx, absolutePath, updated)
+	updated = formatting.Content
 	// Re-baseline to the content we just wrote so subsequent edits in this session
 	// compare against the current on-disk state, not the pre-edit version.
 	newInfo, _ := os.Stat(absolutePath)
@@ -193,6 +194,7 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any
 		suffix = "s"
 	}
 	summary := fmt.Sprintf("Successfully edited %s (replaced %d occurrence%s).", relativePath, replacedCount, suffix)
+	summary += formatting.notice(relativePath)
 	summary += inlineDiagnostics(ctx, options, absolutePath, relativePath)
 	result := okResult(summary)
 	result.ChangedFiles = []string{relativePath}

internal/tools/format_on_write.go

@@ -2,6 +2,7 @@ package tools
 
 import (
 	"context"
+	"errors"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -22,8 +23,59 @@ import (
 // the conflict guard.
 
 // formatOnWriteTimeout bounds one formatter run; a wedged formatter must never
-// hang a tool call. On timeout the unformatted write stands.
-const formatOnWriteTimeout = 10 * time.Second
+// hang a tool call. On timeout the unformatted write stands, and the caller
+// says so: see formatOnWriteResult.
+//
+// A var rather than a const so a test can shorten it. Nothing outside a test
+// assigns to it, and the deadline path was previously unreachable in a test at
+// any speed, which is part of why it went unnoticed that it reported nothing.
+var formatOnWriteTimeout = 10 * time.Second
+
+// formatOnWriteResult is the content after formatting, plus whether the
+// formatting that was supposed to happen actually did.
+//
+// A TIMEOUT IS NOT THE SAME KIND OF MISS AS THE OTHERS. Every other way this
+// falls back is a standing fact about the environment: the toggle is off, the
+// extension has no formatter, the binary is not installed. Those are silent on
+// purpose, because nothing is wrong and saying so on every write would be
+// noise. A deadline firing is different: formatting was configured, available
+// and expected, and the file was written unformatted anyway, on a machine that
+// was merely slow. Left silent, the caller believes it wrote canonical style
+// and finds out from a CI format check it cannot see, which is the thing this
+// feature exists to prevent.
+type formatOnWriteResult struct {
+	Content string
+	// Formatter is the binary that was run, named in the notice so the user can
+	// tell a slow gofmt from a slow prettier.
+	Formatter string
+	TimedOut  bool
+	// RestoreFailed means the file on disk is not known to hold Content.
+	//
+	// These formatters edit in place, so one that is killed or fails partway
+	// can leave the target truncated or half-rewritten: what a dead
+	// `prettier --write` leaves behind is not the input and not the output.
+	// Returning the written bytes while disk holds something else would put the
+	// tracker baseline, the diff preview and the file itself into three
+	// different states, so the failure paths write the bytes back. When even
+	// that fails the user has to hear about it: it is their file.
+	RestoreFailed bool
+}
+
+// notice is the line appended to the tool summary when formatting was expected
+// and did not happen, and empty in every other case.
+func (result formatOnWriteResult) notice(relativePath string) string {
+	if result.RestoreFailed {
+		return "\n\nWARNING: " + relativePath + " may not hold what was written. " +
+			result.Formatter + " was interrupted while rewriting it in place and the " +
+			"content could not be written back. Re-read the file before trusting it."
+	}
+	if !result.TimedOut {
+		return ""
+	}
+	return "\n\nNote: " + relativePath + " was written but not formatted: " +
+		result.Formatter + " did not finish within " + formatOnWriteTimeout.String() +
+		". The file holds exactly what was written, so a project format check may still flag it."
+}
 
 // formatterCommands maps a file extension to the formatter argv; the file path
 // is appended as the final argument. Only in-place, config-respecting,
@@ -69,18 +121,20 @@ func formatOnWriteEnabled() bool {
 // enabled and on PATH) and returns the file's content afterwards. Best-effort
 // throughout: any failure — no formatter, formatter error, timeout, unreadable
 // result — returns writtenContent so the caller's state matches the last write
-// it performed itself.
-func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenContent string) string {
+// it performed itself. Only the timeout is reported back, for the reason on
+// formatOnWriteResult.
+func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenContent string) formatOnWriteResult {
+	unformatted := formatOnWriteResult{Content: writtenContent}
 	if !formatOnWriteEnabled() {
-		return writtenContent
+		return unformatted
 	}
 	command, ok := formatterCommands[strings.ToLower(filepath.Ext(absolutePath))]
 	if !ok {
-		return writtenContent
+		return unformatted
 	}
 	binaryPath, err := exec.LookPath(command[0])
 	if err != nil {
-		return writtenContent
+		return unformatted
 	}
 	formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout)
 	defer cancel()
@@ -89,11 +143,35 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon
 	formatter.Dir = filepath.Dir(absolutePath)
 	formatter.Stdin = strings.NewReader("")
 	if err := formatter.Run(); err != nil {
-		return writtenContent
+		unformatted.Formatter = command[0]
+		// THE FORMATTER EDITS IN PLACE, SO A FAILED RUN CAN LEAVE THE FILE
+		// NEITHER FORMATTED NOR AS WRITTEN. Killed by the deadline or by the
+		// caller, or exiting partway through its own rewrite, the target can hold
+		// a truncation. Returning the written bytes on top of that would leave the
+		// tracker baseline and the diff preview describing a file that is not on
+		// disk, which is a worse failure than the missing formatting: the next
+		// edit compares against content the file does not have.
+		//
+		// Written back unconditionally on this path rather than only when the
+		// bytes differ. Comparing first means reading the file to find out, and a
+		// read that fails leaves the same ambiguity this exists to remove.
+		if restoreErr := os.WriteFile(absolutePath, []byte(writtenContent), 0o644); restoreErr != nil {
+			unformatted.RestoreFailed = true
+		}
+		// OUR deadline, not the caller's cancellation and not the formatter's own
+		// exit status. A cancelled tool call is already being reported as
+		// cancelled, and a formatter that ran and refused the file usually means
+		// content it could not parse, which the write itself does not promise to
+		// fix. Neither is this notice's business; the restore above is, for all
+		// three.
+		if errors.Is(formatCtx.Err(), context.DeadlineExceeded) && ctx.Err() == nil {
+			unformatted.TimedOut = true
+		}
+		return unformatted
 	}
 	formatted, err := os.ReadFile(absolutePath)
 	if err != nil {
-		return writtenContent
+		return unformatted
 	}
-	return string(formatted)
+	return formatOnWriteResult{Content: string(formatted), Formatter: command[0]}
 }

internal/tools/format_on_write_test.go

@@ -97,9 +97,12 @@ func TestFormatOnWriteFormatsAndKeepsTrackerConsistent(t *testing.T) {
 
 func TestFormatOnWriteSkipsUnknownExtensions(t *testing.T) {
 	t.Setenv("ZERO_FORMAT_ON_WRITE", "1")
-	content := maybeFormatWrittenFile(context.Background(), filepath.Join(t.TempDir(), "notes.xyz"), "raw   text")
-	if content != "raw   text" {
-		t.Fatalf("unknown extension must pass through: %q", content)
+	formatting := maybeFormatWrittenFile(context.Background(), filepath.Join(t.TempDir(), "notes.xyz"), "raw   text")
+	if formatting.Content != "raw   text" {
+		t.Fatalf("unknown extension must pass through: %q", formatting.Content)
+	}
+	if notice := formatting.notice("notes.xyz"); notice != "" {
+		t.Fatalf("an extension with no formatter is not a miss worth reporting, got %q", notice)
 	}
 }
 
@@ -111,8 +114,11 @@ func TestFormatOnWriteFormatterLookupFailure(t *testing.T) {
 	if err := os.WriteFile(targetPath, []byte(uglyContent), 0o644); err != nil {
 		t.Fatal(err)
 	}
-	content := maybeFormatWrittenFile(context.Background(), targetPath, uglyContent)
-	if content != uglyContent {
-		t.Fatalf("missing formatter must return written content, got %q", content)
+	formatting := maybeFormatWrittenFile(context.Background(), targetPath, uglyContent)
+	if formatting.Content != uglyContent {
+		t.Fatalf("missing formatter must return written content, got %q", formatting.Content)
+	}
+	if notice := formatting.notice("a.go"); notice != "" {
+		t.Fatalf("an uninstalled formatter is a standing fact, not a miss worth reporting, got %q", notice)
 	}
 }