refactor(jsonutil): centralize streaming atomic replacement

cmd/entire/cli/agent/opencode/cli_commands.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"strings"
@@ -29,49 +30,19 @@ type openCodeExportError struct {
 func (e *openCodeExportError) Error() string { return e.message }
 func (e *openCodeExportError) Unwrap() error { return e.cause }
 
-// runOpenCodeExportToFile runs `opencode export <sessionID>` and redirects stdout
-// to outputPath. This avoids pipe/stdout capture truncation bugs in some opencode versions.
-//
-// outputName is relative to root, the shared .entire root, and must be a staging
-// name, never a live transcript: `opencode export` can fail after writing a
-// partial payload, and can exit 0 having written nothing at all. Callers own the
-// validate-then-install step — see fetchAndCacheExport, which is the only caller
-// and stages under .entire/tmp.
-//
-// opencode never sees the name: it inherits the already-opened file as stdout, so
-// the root's containment covers the whole write even though the payload is
-// produced by another process.
-//
-// The fsync before close is what makes the caller's rename durable: without it
-// some filesystems can surface the rename as complete while the file is still
-// empty after a hard crash, which would destroy the transcript the staging exists
-// to protect. Same reasoning as jsonutil.WriteFileAtomic.
-func runOpenCodeExportToFile(ctx context.Context, root *os.Root, sessionID, outputName string) (retErr error) {
+// runOpenCodeExport runs `opencode export <sessionID>` and streams stdout to
+// output. The caller owns durable, validated publication of those bytes.
+func runOpenCodeExport(ctx context.Context, sessionID string, output io.Writer) error {
 	ctx, cancel := context.WithTimeout(ctx, openCodeCommandTimeout)
 	defer cancel()
 
-	file, err := root.OpenFile(outputName, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600)
-	if err != nil {
-		return fmt.Errorf("failed to create export file: %w", err)
-	}
-	defer func() {
-		if closeErr := file.Close(); closeErr != nil && retErr == nil {
-			retErr = fmt.Errorf("failed to close export file: %w", closeErr)
-		}
-	}()
-
 	cmd := exec.CommandContext(ctx, "opencode", "export", sessionID)
-	cmd.Stdout = file
+	cmd.Stdout = output
 	var stderr bytes.Buffer
 	cmd.Stderr = &stderr
 	if runErr := cmd.Run(); runErr != nil {
 		return classifyOpenCodeExportError(ctx, runErr, stderr.String(), sessionID)
 	}
-
-	if syncErr := file.Sync(); syncErr != nil {
-		return fmt.Errorf("failed to flush export file: %w", syncErr)
-	}
-
 	return nil
 }
 

cmd/entire/cli/agent/opencode/cli_commands_test.go

@@ -1,6 +1,7 @@
 package opencode
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"os"
@@ -88,36 +89,27 @@ func TestClassifyOpenCodeExportError_Timeout(t *testing.T) {
 	}
 }
 
-// TestRunOpenCodeExportToFile_MissingBinary pins the classification of the most
-// common failure. The runner writes to a staging path the caller owns, so the
-// preservation of the live transcript is covered by the fetchAndCacheExport tests
-// in lifecycle_test.go, not here.
-func TestRunOpenCodeExportToFile_MissingBinary(t *testing.T) {
+// TestRunOpenCodeExport_MissingBinary pins the classification of the most common
+// failure. Publication is covered by the fetchAndCacheExport tests.
+func TestRunOpenCodeExport_MissingBinary(t *testing.T) {
 	// No t.Parallel: t.Setenv.
-	root := mustOpenRoot(t, t.TempDir())
-	const staged = ".export-ses_cached.json-1"
-
 	// Empty PATH makes the export fail deterministically without an opencode binary.
 	t.Setenv("PATH", "")
 
-	err := runOpenCodeExportToFile(context.Background(), root, "ses_cached", staged)
+	err := runOpenCodeExport(context.Background(), "ses_cached", &bytes.Buffer{})
 	if err == nil {
 		t.Fatal("expected export to fail with no opencode on PATH")
 	}
 	if !errors.Is(err, exec.ErrNotFound) {
-		t.Fatalf("runOpenCodeExportToFile error = %v, want exec.ErrNotFound", err)
+		t.Fatalf("runOpenCodeExport error = %v, want exec.ErrNotFound", err)
 	}
 }
 
-func TestRunOpenCodeExportToFile_WritesStdoutToPath(t *testing.T) {
+func TestRunOpenCodeExport_WritesStdout(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("stub opencode is a shell script")
 	}
 	// No t.Parallel: t.Setenv.
-	dir := t.TempDir()
-	root := mustOpenRoot(t, dir)
-	const staged = ".export-ses_ok.json-1"
-
 	const export = `{"info":{"id":"ses_ok"},"messages":[]}`
 	stubDir := t.TempDir()
 	script := "#!/bin/sh\nprintf '%s' '" + export + "'\n"
@@ -126,55 +118,12 @@ func TestRunOpenCodeExportToFile_WritesStdoutToPath(t *testing.T) {
 	}
 	t.Setenv("PATH", stubDir)
 
-	if err := runOpenCodeExportToFile(context.Background(), root, "ses_ok", staged); err != nil {
-		t.Fatalf("runOpenCodeExportToFile failed: %v", err)
+	var output bytes.Buffer
+	if err := runOpenCodeExport(context.Background(), "ses_ok", &output); err != nil {
+		t.Fatalf("runOpenCodeExport failed: %v", err)
 	}
-
-	got, err := os.ReadFile(filepath.Join(dir, staged))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if string(got) != export {
-		t.Fatalf("exported transcript = %q, want %q", string(got), export)
-	}
-}
-
-func TestRenameOverExisting_ReplacesDestination(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-	root := mustOpenRoot(t, dir)
-	const staged = ".export-ses_x.json-1"
-	const dest = "ses_x.json"
-	if err := os.WriteFile(filepath.Join(dir, staged), []byte("fresh"), 0o600); err != nil {
-		t.Fatal(err)
-	}
-	if err := os.WriteFile(filepath.Join(dir, dest), []byte("stale"), 0o600); err != nil {
-		t.Fatal(err)
-	}
-
-	if err := renameOverExisting(root, staged, dest); err != nil {
-		t.Fatalf("renameOverExisting failed: %v", err)
-	}
-	got, err := os.ReadFile(filepath.Join(dir, dest))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if string(got) != "fresh" {
-		t.Fatalf("destination = %q, want %q", string(got), "fresh")
-	}
-	if _, err := os.Stat(filepath.Join(dir, staged)); !os.IsNotExist(err) {
-		t.Errorf("staged file still present after rename: %v", err)
-	}
-}
-
-func TestIsRenameContention_NonSharingErrorsAreNotRetried(t *testing.T) {
-	t.Parallel()
-
-	// On POSIX this is always false; on Windows only sharing/access violations
-	// qualify. A plain ENOENT must never be retried on either.
-	if isRenameContention(os.ErrNotExist) {
-		t.Error("isRenameContention(ErrNotExist) = true, want false")
+	if output.String() != export {
+		t.Fatalf("exported transcript = %q, want %q", output.String(), export)
 	}
 }
 
@@ -187,15 +136,3 @@ func TestFormatOpenCodeErrorDetail_Truncates(t *testing.T) {
 		t.Fatalf("formatOpenCodeErrorDetail = %q, want %q", detail, want)
 	}
 }
-
-// mustOpenRoot opens dir as an os.Root, standing in for the shared .entire root
-// the production callers pass.
-func mustOpenRoot(t *testing.T, dir string) *os.Root {
-	t.Helper()
-	root, err := os.OpenRoot(dir)
-	if err != nil {
-		t.Fatalf("os.OpenRoot(%s): %v", dir, err)
-	}
-	t.Cleanup(func() { _ = root.Close() })
-	return root
-}

cmd/entire/cli/agent/opencode/lifecycle.go

@@ -3,6 +3,7 @@ package opencode
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -13,13 +14,14 @@ import (
 
 	"github.com/entireio/cli/cmd/entire/cli/agent"
 	"github.com/entireio/cli/cmd/entire/cli/entiredir"
+	"github.com/entireio/cli/cmd/entire/cli/jsonutil"
 	"github.com/entireio/cli/cmd/entire/cli/logging"
 	"github.com/entireio/cli/cmd/entire/cli/osroot"
 	"github.com/entireio/cli/cmd/entire/cli/paths"
 	"github.com/entireio/cli/cmd/entire/cli/validation"
 )
 
-var runOpenCodeExportToFileFn = runOpenCodeExportToFile
+var runOpenCodeExportFn = runOpenCodeExport
 
 // Compile-time assertion that OpenCode can inject context into the model.
 var _ agent.ContextInjector = (*OpenCodeAgent)(nil)
@@ -214,10 +216,7 @@ func (a *OpenCodeAgent) fetchAndCacheExport(ctx context.Context, sessionID strin
 		repoRoot = "."
 	}
 
-	// Every read, write, and rename below goes through the shared .entire root.
-	// The absolute paths are still needed alongside it, because `opencode export`
-	// is a separate process that takes a path, and the transcript path is handed
-	// back to callers that pass it across the same boundary.
+	// File operations stay confined to .entire; callers receive an absolute path.
 	root, err := entiredir.OpenAt(repoRoot)
 	if err != nil {
 		return "", fmt.Errorf("open %s for export cache: %w", paths.EntireDir, err)
@@ -240,57 +239,46 @@ func (a *OpenCodeAgent) fetchAndCacheExport(ctx context.Context, sessionID strin
 		return "", fmt.Errorf("failed to create temp dir: %w", err)
 	}
 
-	// Export to a staging file and move it into place only once the bytes are
-	// known good. tmpFile is frequently the ONLY local copy of the session — the
-	// turn-end hook writes it and nothing condenses it into a checkpoint until
-	// the user commits — and every caller re-exports over a possibly-populated
-	// path (PrepareTranscript on every turn end, FetchTranscript on attach).
-	// Writing in place means a missing binary, a rejected session, a timeout, or
-	// an `opencode export` that exits 0 with truncated output replaces a good
-	// transcript with nothing or with garbage. Garbage is the worse of the two:
-	// attach's os.Stat branch accepts whatever is at this path and treats
-	// PrepareTranscript's failure as best-effort, so a corrupt file is used
-	// silently while a missing one at least falls through to a re-fetch.
-	staged, err := stageExportPath(root, entireTmpName, sessionID)
+	// The cache may be the session's only local transcript until checkpointing;
+	// publish only a complete export that has passed JSON validation.
+	err = jsonutil.WriteFileAtomicStreamIn(
+		ctx,
+		root,
+		tmpName,
+		0o600,
+		func(writer io.Writer) error {
+			return runOpenCodeExportFn(ctx, sessionID, writer)
+		},
+		func(reader io.Reader) error {
+			return validateOpenCodeExport(ctx, sessionID, reader)
+		},
+	)
 	if err != nil {
-		return "", err
-	}
-	stagedAbs := filepath.Join(entireDirAbs, filepath.FromSlash(staged))
-	keepStaged := false
-	defer func() {
-		if !keepStaged {
-			_ = root.Remove(staged) //nolint:errcheck // best-effort cleanup of a staging file we are abandoning
+		var publishErr *jsonutil.PublishError
+		if errors.As(err, &publishErr) {
+			return "", fmt.Errorf("failed to install export file (export saved at %s): %w", publishErr.StagedPath, err)
 		}
-	}()
-
-	if err := runOpenCodeExportToFileFn(ctx, root, sessionID, staged); err != nil {
-		return "", err
+		return "", err //nolint:wrapcheck // preserve producer and validator error classification
 	}
 
-	data, err := entiredir.ReadFile(root, staged)
+	return tmpFile, nil
+}
+
+func validateOpenCodeExport(ctx context.Context, sessionID string, reader io.Reader) error {
+	data, err := io.ReadAll(reader)
 	if err != nil {
-		return "", fmt.Errorf("failed to read export file: %w", err)
+		return fmt.Errorf("failed to read export file: %w", err)
 	}
-
-	if !json.Valid(data) {
-		logging.Debug(logging.WithComponent(ctx, "lifecycle"),
-			"opencode export file contained invalid JSON",
-			slog.Int("bytes", len(data)),
-			slog.String("path", stagedAbs),
-		)
-		return "", &openCodeExportError{
-			message: fmt.Sprintf("OpenCode returned invalid transcript data for session %q. Try updating OpenCode and running the command again.", sessionID),
-		}
+	if json.Valid(data) {
+		return nil
 	}
 
-	if err := renameOverExisting(root, staged, tmpName); err != nil {
-		// The staged export is intact and validated; keep it rather than delete a
-		// transcript we may be the last holder of, and name it so the user can
-		// recover it by hand.
-		keepStaged = true
-		return "", fmt.Errorf("failed to install export file (export saved at %s): %w", stagedAbs, err)
+	logging.Debug(logging.WithComponent(ctx, "lifecycle"),
+		"opencode export file contained invalid JSON",
+		slog.Int("bytes", len(data)),
+		slog.String("session_id", sessionID),
+	)
+	return &openCodeExportError{
+		message: fmt.Sprintf("OpenCode returned invalid transcript data for session %q. Try updating OpenCode and running the command again.", sessionID),
 	}
-	keepStaged = true
-
-	return tmpFile, nil
 }