fix(tools): pass terminal stdin marker to ruff and clarify path exclusions

Author: hazyhaar

internal/tools/format_on_write.go

@@ -71,12 +71,13 @@ func (result formatOnWriteResult) notice(relativePath string) string {
 //
 // STDIN ADAPTERS READ THE WRITTEN BYTES ON STDIN AND PRINT THE FORMATTED RESULT
 // TO STDOUT. That route lets filenameFlag carry the logical destination path,
-// so any rule written against the file's own path (.clang-format-ignore, an
-// .editorconfig section, rustfmt's ignore) still resolves against the file the
-// caller named instead of a random staging name. Physical adapters have no
-// such option: they are handed a private staging copy inside an owner-only
-// sibling directory (including auxiliary files) and keep the destination
-// directory as their working directory.
+// so a rule written against the file's own path is aimed at the name the
+// caller used instead of a random staging name. Aiming the rule at that name
+// does not make a path exclusion apply to a file that does not exist yet; see
+// formatterCommands. Physical adapters have no such option: they are handed a
+// private staging copy inside an owner-only sibling directory (including
+// auxiliary files) and keep the destination directory as their working
+// directory.
 type formatterAdapter struct {
 	argv []string
 	// stdin selects the stdin/stdout route. When false the private staging path
@@ -86,14 +87,28 @@ type formatterAdapter struct {
 	// stdin adapter. It is emitted as "--flag=<target>" and empty when the
 	// formatter resolves its configuration from the working directory alone.
 	filenameFlag string
+	// stdinArg is appended after filenameFlag, and must not also be stored in
+	// argv: a "-" kept in argv would be emitted before the filename flag.
+	// For Ruff, "-" serves as an explicit, defensive marker ensuring input is read
+	// from standard input when invoked as `ruff format --stdin-filename=<path> -`.
+	// Empty for formatters that read stdin when given no path.
+	stdinArg string
 }
 
 // formatterCommands maps a file extension to its formatter. A missing binary
 // silently skips formatting.
+//
+// PATH EXCLUSIONS DO NOT APPLY ON THE FIRST WRITE OF A NEW FILE. The stdin
+// adapters pass the logical destination, but a formatter that consults its
+// ignore file only when that path already exists (clang-format and
+// .clang-format-ignore) formats a not-yet-created file even when the path
+// matches an exclusion. An overwrite still finds the previous file at that
+// path, so the same rule can apply. TestFormatOnWritePathIgnoreDoesNotApplyToNewFiles
+// pins this split; it is the contract, not an oversight.
 var formatterCommands = map[string]formatterAdapter{
 	".go":    {argv: []string{"gofmt"}, stdin: true},
 	".rs":    {argv: []string{"rustfmt"}, stdin: true},
-	".py":    {argv: []string{"ruff", "format", "--quiet"}, stdin: true, filenameFlag: "--stdin-filename"},
+	".py":    {argv: []string{"ruff", "format", "--quiet"}, stdin: true, filenameFlag: "--stdin-filename", stdinArg: "-"},
 	".ts":    {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"},
 	".tsx":   {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"},
 	".js":    {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"},
@@ -142,11 +157,11 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon
 // maybeFormatWrittenFileScoped runs the configured formatter over a transient
 // copy of the written bytes (stdin, or a private staging file) and returns the
 // bytes to publish. The destination is never opened or rewritten here. If
-// absolutePath does not resolve inside one of the scope's write roots, the
-// formatting is refused and the written bytes pass through unchanged: a
-// formatter must not be able to read or write outside the same roots the tool
-// itself is confined to. The formatter's working directory is pinned inside the
-// matched root for the same reason.
+// absolutePath does not resolve inside one of the scope's write roots,
+// formatting is refused and the written bytes pass through unchanged. That
+// refusal is the pre-launch path check in openFormattedFileRoot. The
+// formatter's working directory is a path string derived from that check, not
+// a directory handle the subprocess is bound to.
 func maybeFormatWrittenFileScoped(ctx context.Context, workspaceRoot string, scope PathScope, absolutePath string, writtenContent string) formatOnWriteResult {
 	unformatted := formatOnWriteResult{Content: writtenContent}
 	if !formatOnWriteEnabled() {
@@ -172,10 +187,19 @@ func maybeFormatWrittenFileScoped(ctx context.Context, workspaceRoot string, sco
 	return formatWithStaging(ctx, adapter, binaryPath, absolutePath, writtenContent, workDir)
 }
 
-// openFormattedFileRoot resolves absolutePath against the scope's write roots
-// and opens the first root that contains it. It returns the descriptor-bound
-// root and the path relative to it, or an error when the path lies outside
-// every allowed root.
+// openFormattedFileRoot checks, before a formatter subprocess is started, that
+// absolutePath resolves inside one of the scope's write roots. It resolves
+// each configured root, applies a lexical prefix check to the candidate path
+// (previously validated by resolveScopedTargetPath), and opens the first root
+// containing it. It returns that root and the path relative to it, or an error
+// when the path lies outside every allowed root.
+//
+// The caller does not use the root as a handle for formatter I/O. It joins
+// root.Name() with the relative directory and passes that string as
+// exec.Cmd.Dir. The check is therefore a pre-launch lexical path check, not
+// descriptor-level confinement of the subprocess: a symlink or Windows junction
+// swapped onto a component of that directory between the check and process start
+// is not excluded, and there is no portable way to hand Cmd.Dir a directory handle.
 func openFormattedFileRoot(workspaceRoot string, scope PathScope, absolutePath string) (*os.Root, string, error) {
 	roots, err := scopedRoots(workspaceRoot, scope)
 	if err != nil {
@@ -273,6 +297,9 @@ func formatWithStdin(ctx context.Context, adapter formatterAdapter, binaryPath,
 	if adapter.filenameFlag != "" {
 		arguments = append(arguments, adapter.filenameFlag+"="+absolutePath)
 	}
+	if adapter.stdinArg != "" {
+		arguments = append(arguments, adapter.stdinArg)
+	}
 	formatter := exec.CommandContext(formatCtx, binaryPath, arguments...)
 	formatter.Dir = dir
 	formatter.Stdin = strings.NewReader(writtenContent)

internal/tools/format_on_write_test.go

@@ -229,7 +229,9 @@ func TestFormatOnWritePrettierUsesDestinationFilename(t *testing.T) {
 //
 // A stdin adapter receives the logical destination through its filename flag,
 // so a .clang-format-ignore pattern, an .editorconfig section, or rustfmt's
-// ignore still matches "vendor/lib.hintfmt" rather than a random staging name.
+// ignore is aimed at "vendor/lib.hintfmt" rather than a random staging name.
+// Whether the formatter consults that rule for a path that does not exist yet
+// is a separate contract; see TestFormatOnWritePathIgnoreDoesNotApplyToNewFiles.
 func TestFormatOnWritePassesLogicalDestinationToStdinFormatter(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("fake formatter shim is a POSIX script")
@@ -265,6 +267,85 @@ func TestFormatOnWritePassesLogicalDestinationToStdinFormatter(t *testing.T) {
 	}
 }
 
+// PATH EXCLUSIONS DO NOT APPLY TO A FILE THAT DOES NOT EXIST YET.
+//
+// clang-format consults .clang-format-ignore only when --assume-filename names
+// a path that is already on disk. Formatting runs before publication, so the
+// first write of a new file is formatted even when that path would match an
+// exclusion. An overwrite still finds the previous file: the formatter prints
+// nothing, and the empty-output guard keeps the written bytes. Depositing the
+// bytes at the destination before the formatter runs would make the subsequent
+// exclusive create fail, so this split is the contract.
+func TestFormatOnWritePathIgnoreDoesNotApplyToNewFiles(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("fake formatter shim is a POSIX script")
+	}
+	t.Setenv("ZERO_FORMAT_ON_WRITE", "1")
+	installExistenceGatedFormatter(t, ".ignfmt", "ignfmt", "--assume-filename")
+
+	dir := t.TempDir()
+	target := filepath.Join(dir, "vendor", "lib.ignfmt")
+	if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil {
+		t.Fatal(err)
+	}
+
+	const written = "unformatted\n"
+	created := maybeFormatWrittenFile(context.Background(), target, written)
+	if created.Content != "REFORMATTED\n" {
+		t.Fatalf("a new file must be formatted even when its path would be ignored, got %q", created.Content)
+	}
+	if _, err := os.Stat(target); !os.IsNotExist(err) {
+		t.Fatalf("formatting a new file must not create it ahead of publication: %v", err)
+	}
+
+	if err := os.WriteFile(target, []byte("previous\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	overwritten := maybeFormatWrittenFile(context.Background(), target, written)
+	if overwritten.Content != written {
+		t.Fatalf("an existing ignored path must keep the written bytes, got %q", overwritten.Content)
+	}
+	onDisk, err := os.ReadFile(target)
+	if err != nil || string(onDisk) != "previous\n" {
+		t.Fatalf("formatter must not rewrite the destination, got %q, %v", onDisk, err)
+	}
+}
+
+// installExistenceGatedFormatter registers a stdin formatter that honours a
+// path exclusion only when the assumed path already exists, matching
+// clang-format's .clang-format-ignore behaviour (where existing ignored files
+// have their input returned unchanged). A missing path is reformatted.
+func installExistenceGatedFormatter(t *testing.T, extension, name, filenameFlag string) {
+	t.Helper()
+	directory := t.TempDir()
+	script := "#!/bin/sh\n" +
+		"path=\n" +
+		"for arg in \"$@\"; do\n" +
+		"  case \"$arg\" in\n" +
+		"    " + filenameFlag + "=*) path=${arg#" + filenameFlag + "=} ;;\n" +
+		"  esac\n" +
+		"done\n" +
+		"if [ -n \"$path\" ] && [ -e \"$path\" ]; then\n" +
+		"  cat\n" +
+		"  exit 0\n" +
+		"fi\n" +
+		"cat >/dev/null\n" +
+		"printf 'REFORMATTED\\n'\n"
+	if err := os.WriteFile(filepath.Join(directory, name), []byte(script), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH"))
+	previous, existed := formatterCommands[extension]
+	formatterCommands[extension] = formatterAdapter{argv: []string{name}, stdin: true, filenameFlag: filenameFlag}
+	t.Cleanup(func() {
+		if existed {
+			formatterCommands[extension] = previous
+			return
+		}
+		delete(formatterCommands, extension)
+	})
+}
+
 // THE PRODUCTION TABLE MUST CARRY THE HINTS, NOT ONLY THE TEST SEAM.
 func TestFormatterFilenameHints(t *testing.T) {
 	want := map[string]string{
@@ -676,6 +757,68 @@ func installFakePhysicalFormatter(t *testing.T, extension, name, record string)
 	})
 }
 
+// RUFF MUST READ STDIN WITH AN EXPLICIT TERMINAL MARKER.
+//
+// The stdin route passes an explicit "-" marker after --stdin-filename,
+// ensuring Ruff strictly reads input from stdin as documented.
+func TestFormatOnWriteRuffPassesTerminalStdinMarker(t *testing.T) {
+	t.Setenv("ZERO_FORMAT_ON_WRITE", "1")
+	adapter := formatterCommands[".py"]
+	if !adapter.stdin || adapter.filenameFlag != "--stdin-filename" || adapter.stdinArg != "-" {
+		t.Fatalf("ruff adapter = %+v, want stdin with --stdin-filename and stdinArg %q", adapter, "-")
+	}
+
+	directory := t.TempDir()
+	binaryName := "ruff" + formatterScriptExtension()
+	script := "#!/bin/sh\ncat\n"
+	if runtime.GOOS == "windows" {
+		script = "@echo off\r\nexit /b 0\r\n"
+	}
+	if err := os.WriteFile(filepath.Join(directory, binaryName), []byte(script), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	var seen *exec.Cmd
+	previous := formatterCommandObserver
+	formatterCommandObserver = func(command *exec.Cmd) { seen = command }
+	t.Cleanup(func() { formatterCommandObserver = previous })
+
+	dir := t.TempDir()
+	target := filepath.Join(dir, "pkg", "new_file.py")
+	if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	_ = maybeFormatWrittenFile(context.Background(), target, "x=1\n")
+	if seen == nil {
+		t.Fatal("ruff was not executed")
+	}
+	args := seen.Args
+	if len(args) == 0 || args[len(args)-1] != "-" {
+		t.Fatalf("ruff execution args %q must end with the stdin marker \"-\"", args)
+	}
+	dashes := 0
+	flagAt := -1
+	wantFlag := "--stdin-filename=" + target
+	for i, arg := range args {
+		if arg == "-" {
+			dashes++
+		}
+		if arg == wantFlag {
+			flagAt = i
+		}
+		if arg == target {
+			t.Fatalf("ruff execution args %q pass the path as a file operand; the stdin route must not", args)
+		}
+	}
+	if dashes != 1 {
+		t.Fatalf("ruff execution args %q must contain exactly one stdin marker", args)
+	}
+	if flagAt < 0 || flagAt >= len(args)-1 {
+		t.Fatalf("ruff execution args %q must carry %q before the terminal \"-\"", args, wantFlag)
+	}
+}
+
 func TestFormatOnWriteRuffUsesLogicalDestinationForWriteAndEdit(t *testing.T) {
 	if _, err := exec.LookPath("ruff"); err != nil {
 		t.Skip("ruff not installed")