feat(#3996): classify generated-media save failures into safe user-visible reasons

Replace the generic "see debug log for details" materialization and
manifest warnings with fixed, classified sentences: no session
workspace, unwritable/read-only location, deleted root, filename
collision exhaustion, refused save path, and a saved-but-unrecorded
manifest failure that may not render inline. Unclassified causes tell
the user to enable --debug and retry. Every reason is a constant, so
no absolute path, session ID, provider metadata, or raw OS error can
leak into a WarningEvent; the detailed error still reaches the debug
log only. Add workspacemedia.ErrNameExhausted so collision exhaustion
is matchable without echoing the requested path.

Author: aheritier

pkg/runtime/loop.go

@@ -1337,9 +1337,11 @@ func sanitizeToolCallName(name string) string {
 // error (including the workspace root) to the debug log only, and emits a
 // runtime [WarningEvent] carrying nothing but safe display metadata — the
 // exact 1-based failed item index and total batch count, the sanitized MIME
-// type, and the sanitized provider-supplied name (or [fallbackDisplayName]
+// type, the sanitized provider-supplied name (or [fallbackDisplayName]
 // when that name is empty, whitespace-only, or missing — never omitted,
-// exactly like the strip_generated_media.go placeholder) — so the failure
+// exactly like the strip_generated_media.go placeholder), and a fixed
+// classified reason from [mediaSaveFailureReason] (a retry-with-debug
+// hint when the cause is unclassified, never raw error text) — so the failure
 // is observable to the user/caller without leaking the absolute workspace
 // path or a raw OS error (which could contain that path) into a surface a
 // user might paste into a bug report or share screen. Both the name AND the
@@ -1374,8 +1376,8 @@ func (r *LocalRuntime) materializeGeneratedMedia(ctx context.Context, sess *sess
 			if displayName == "" {
 				displayName = fallbackDisplayName
 			}
-			warning := fmt.Sprintf("Failed to save generated media item %d/%d (%s, %s); see debug log for details",
-				i+1, len(media), safeMimeType, displayName)
+			warning := fmt.Sprintf("Failed to save generated media item %d/%d (%s, %s). %s",
+				i+1, len(media), safeMimeType, displayName, mediaSaveFailureReason(err))
 			events.Emit(Warning(chat.TruncateUTF8Bytes(warning, maxPlaceholderOrWarningBytes), agentName))
 		}
 
@@ -1410,7 +1412,7 @@ func (r *LocalRuntime) materializeGeneratedMedia(ctx context.Context, sess *sess
 			slog.DebugContext(ctx, "Failed to record generated media in the manifest; the file was written but may not display inline",
 				"agent", agentName, "session_id", sess.ID, "rel_path", res.RelPath, "error", err)
 			if events != nil {
-				warning := fmt.Sprintf("Saved generated media %s but failed to record it for display; see debug log for details", res.RelPath)
+				warning := fmt.Sprintf("Saved generated media %s but could not record it for display; it may not render inline. %s", res.RelPath, retryWithDebugAdvice)
 				events.Emit(Warning(chat.TruncateUTF8Bytes(warning, maxPlaceholderOrWarningBytes), agentName))
 			}
 		}

pkg/runtime/materialize_generated_media_test.go

@@ -3,11 +3,13 @@ package runtime
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"io/fs"
 	"log/slog"
 	"os"
 	"path/filepath"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 
@@ -268,6 +270,8 @@ func TestMaterializeGeneratedMedia_NoWorkspaceRoot(t *testing.T) {
 	assert.Contains(t, warnings[1].Message, "2/2")
 	assert.Contains(t, warnings[1].Message, "dog.jpg")
 	for _, w := range warnings {
+		assert.Contains(t, w.Message, "No session workspace is available to save into.",
+			"a missing workspace must surface its classified reason")
 		assertSafeWarningMessage(t, w.Message, sess.ID, "")
 	}
 
@@ -298,6 +302,8 @@ func TestMaterializeGeneratedMedia_UnwritableRoot(t *testing.T) {
 	require.Len(t, warnings, 1)
 	assert.Contains(t, warnings[0].Message, "cat.png")
 	assert.Contains(t, warnings[0].Message, "1/1")
+	assert.Contains(t, warnings[0].Message, "The save location no longer exists.",
+		"a deleted workspace root must surface its classified reason")
 	assertSafeWarningMessage(t, warnings[0].Message, sess.ID, root)
 	assertNoFilesUnder(t, dataDir)
 }
@@ -334,6 +340,9 @@ func TestMaterializeGeneratedMedia_PartialSuccess_SingleBatchCall(t *testing.T)
 	assert.Contains(t, warnings[0].Message, "2/2", "the failing item's index/total must reflect its real position in the batch")
 	assert.Contains(t, warnings[0].Message, "dog.jpg")
 	assert.Contains(t, warnings[0].Message, "image/jpeg")
+	assert.Contains(t, warnings[0].Message, retryWithDebugAdvice,
+		"an unclassified failure must carry the retry-with-debug advice")
+	assert.NotContains(t, warnings[0].Message, "injected failure", "the raw error text must never reach the warning")
 	assertSafeWarningMessage(t, warnings[0].Message, sess.ID, root)
 
 	data, err := os.ReadFile(filepath.Join(root, "cat.png"))
@@ -346,6 +355,82 @@ func TestMaterializeGeneratedMedia_PartialSuccess_SingleBatchCall(t *testing.T)
 	require.ErrorIs(t, err, session.ErrGeneratedFileNotFound, "a failed write must never be recorded in the manifest")
 }
 
+// TestMaterializeGeneratedMedia_ClassifiedWriteFailureReasons drives every
+// classified writer-failure category through the workspacemediaWrite seam
+// and proves the per-item warning carries exactly the fixed classified
+// sentence — while the raw error (with its embedded secret path) reaches
+// only the debug log, never the warning.
+func TestMaterializeGeneratedMedia_ClassifiedWriteFailureReasons(t *testing.T) {
+	const secretPath = "/secret/root/cat.png"
+
+	cases := []struct {
+		name       string
+		writeErr   error
+		wantReason string
+	}{
+		{
+			name:       "not writable",
+			writeErr:   fmt.Errorf("claim %q: %w", secretPath, fs.ErrPermission),
+			wantReason: "The save location is not writable.",
+		},
+		{
+			name:       "read-only filesystem",
+			writeErr:   fmt.Errorf("open workspace root: %w", &fs.PathError{Op: "open", Path: secretPath, Err: syscall.EROFS}),
+			wantReason: "The save location is not writable.",
+		},
+		{
+			name:       "collision exhaustion",
+			writeErr:   fmt.Errorf("%w: %q after 10000 attempts", workspacemedia.ErrNameExhausted, secretPath),
+			wantReason: "Every candidate filename is already taken.",
+		},
+		{
+			// The provider-named flow retries ErrPathEscape once under the
+			// generic name; the seam fails both attempts, so the refusal
+			// itself must reach the user as the classified reason.
+			name:       "requested path refused",
+			writeErr:   fmt.Errorf("%w: %q: absolute path", workspacemedia.ErrPathEscape, secretPath),
+			wantReason: "The requested save path was refused.",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			r, _, dataDir := newMediaTestRuntime(t)
+			sess, root := workspaceSession(t, "sess-classified-"+tc.name)
+
+			var logBuf bytes.Buffer
+			prevLogger := slog.Default()
+			slog.SetDefault(slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelDebug})))
+			t.Cleanup(func() { slog.SetDefault(prevLogger) })
+
+			orig := workspacemediaWrite
+			workspacemediaWrite = func(string, string, []byte, string) (workspacemedia.Result, error) {
+				return workspacemedia.Result{}, tc.writeErr
+			}
+			t.Cleanup(func() { workspacemediaWrite = orig })
+
+			sink := &collectingSink{}
+			parts := r.materializeGeneratedMedia(t.Context(), sess, []chat.MediaDelta{
+				{Data: []byte{0x01}, MimeType: "image/png", Name: "cat.png", Size: 1},
+			}, "root", sink)
+
+			assert.Empty(t, parts)
+			warnings := sink.warnings()
+			require.Len(t, warnings, 1)
+			msg := warnings[0].Message
+			assert.Contains(t, msg, "1/1")
+			assert.Contains(t, msg, "cat.png")
+			assert.Contains(t, msg, tc.wantReason)
+			assert.NotContains(t, msg, retryWithDebugAdvice, "a classified failure must show its reason, not the debug fallback")
+			assert.NotContains(t, msg, secretPath, "the warning must never leak a path embedded in the error")
+			assertSafeWarningMessage(t, msg, sess.ID, root)
+
+			assert.Contains(t, logBuf.String(), secretPath, "the detailed error must still reach the debug log")
+			assertNoFilesUnder(t, dataDir)
+		})
+	}
+}
+
 // storeWithoutManifest hides the built-in store's GeneratedMediaManifest
 // implementation: interface embedding only promotes session.Store's own
 // method set, so the type assertion in recordGeneratedFile fails.
@@ -372,7 +457,10 @@ func TestMaterializeGeneratedMedia_ManifestFailureKeepsFileAndWarns(t *testing.T
 	warnings := sink.warnings()
 	require.Len(t, warnings, 1)
 	assert.Contains(t, warnings[0].Message, "cat.png")
-	assert.Contains(t, warnings[0].Message, "record")
+	assert.Contains(t, warnings[0].Message, "could not record it for display")
+	assert.Contains(t, warnings[0].Message, retryWithDebugAdvice,
+		"the manifest cause is unclassified storage internals, so the warning must carry the retry-with-debug advice")
+	assert.NotContains(t, warnings[0].Message, "see debug log")
 	assertBoundedSingleLineUTF8(t, warnings[0].Message)
 }
 

pkg/runtime/media_save_failure.go

@@ -0,0 +1,37 @@
+package runtime
+
+import (
+	"errors"
+	"io/fs"
+	"syscall"
+
+	"github.com/docker/docker-agent/pkg/session"
+	"github.com/docker/docker-agent/pkg/workspacemedia"
+)
+
+// retryWithDebugAdvice is the user-facing fallback for a generated-media
+// save failure with no safe classified reason: it tells the user how to
+// capture the technical details instead of leaking any of them.
+const retryWithDebugAdvice = "Enable --debug and retry to capture technical details."
+
+// mediaSaveFailureReason maps a generated-media save failure to a fixed,
+// user-safe sentence for the runtime WarningEvent. Every return value is a
+// constant: nothing from err — which may embed the absolute workspace
+// root, a requested path, a session ID, or raw OS error text — ever
+// reaches the warning. The detailed error belongs in the debug log only.
+func mediaSaveFailureReason(err error) string {
+	switch {
+	case errors.Is(err, session.ErrWorkingDirUnavailable):
+		return "No session workspace is available to save into."
+	case errors.Is(err, workspacemedia.ErrNameExhausted):
+		return "Every candidate filename is already taken."
+	case errors.Is(err, workspacemedia.ErrPathEscape):
+		return "The requested save path was refused."
+	case errors.Is(err, fs.ErrPermission), errors.Is(err, syscall.EROFS):
+		return "The save location is not writable."
+	case errors.Is(err, fs.ErrNotExist):
+		return "The save location no longer exists."
+	default:
+		return retryWithDebugAdvice
+	}
+}

pkg/runtime/media_save_failure_test.go

@@ -0,0 +1,81 @@
+package runtime
+
+import (
+	"errors"
+	"fmt"
+	"io/fs"
+	"syscall"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/docker/docker-agent/pkg/session"
+	"github.com/docker/docker-agent/pkg/workspacemedia"
+)
+
+// TestMediaSaveFailureReason proves every classified failure maps to its
+// fixed sentence, that anything unclassified falls back to the
+// retry-with-debug advice, and — the redaction contract — that no fragment
+// of the underlying error (absolute paths, session IDs, raw OS error text)
+// ever survives into the returned reason.
+func TestMediaSaveFailureReason(t *testing.T) {
+	t.Parallel()
+
+	const (
+		secretRoot = "/Users/someone/secret-workspace"
+		sessionID  = "sess-1234-secret"
+	)
+
+	cases := []struct {
+		name string
+		err  error
+		want string
+	}{
+		{
+			name: "no workspace provenance",
+			err:  fmt.Errorf("%w: session %s has no workspace root and no parent", session.ErrWorkingDirUnavailable, sessionID),
+			want: "No session workspace is available to save into.",
+		},
+		{
+			name: "filename collision exhaustion",
+			err:  fmt.Errorf("%w: %q after 10000 attempts", workspacemedia.ErrNameExhausted, secretRoot+"/cat.png"),
+			want: "Every candidate filename is already taken.",
+		},
+		{
+			name: "requested path refused",
+			err:  fmt.Errorf("%w: %q: absolute path", workspacemedia.ErrPathEscape, secretRoot),
+			want: "The requested save path was refused.",
+		},
+		{
+			name: "permission denied",
+			err:  &fs.PathError{Op: "open", Path: secretRoot, Err: fs.ErrPermission},
+			want: "The save location is not writable.",
+		},
+		{
+			name: "read-only filesystem",
+			err:  &fs.PathError{Op: "open", Path: secretRoot, Err: syscall.EROFS},
+			want: "The save location is not writable.",
+		},
+		{
+			name: "workspace root gone",
+			err:  fmt.Errorf("open workspace root: %w", &fs.PathError{Op: "open", Path: secretRoot, Err: fs.ErrNotExist}),
+			want: "The save location no longer exists.",
+		},
+		{
+			name: "unclassified error",
+			err:  errors.New("write " + secretRoot + "/tmp-1: device timeout for " + sessionID),
+			want: retryWithDebugAdvice,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			got := mediaSaveFailureReason(tc.err)
+			assert.Equal(t, tc.want, got)
+			assert.NotContains(t, got, secretRoot, "the reason must never echo a path from the error")
+			assert.NotContains(t, got, sessionID, "the reason must never echo a session ID from the error")
+			assert.NotContains(t, got, tc.err.Error(), "the reason must never echo the raw error text")
+		})
+	}
+}

pkg/workspacemedia/writer.go

@@ -63,14 +63,20 @@ type Result struct {
 }
 
 // maxNameAttempts bounds the dash-suffix collision retry so a pathological
-// directory cannot loop forever; exhaustion surfaces as a visible error.
+// directory cannot loop forever; exhaustion surfaces as [ErrNameExhausted].
 const maxNameAttempts = 10000
 
+// ErrNameExhausted classifies collision-suffix exhaustion: every candidate
+// name up to maxNameAttempts already exists. Match with errors.Is when the
+// failure must be explained without echoing the requested path.
+var ErrNameExhausted = errors.New("no free filename after exhausting collision suffixes")
+
 // Write stores data under workspaceRoot at requestedPath, sanitized and
 // collision-avoided per the package contract, and returns the exact
 // workspace-relative path written. Prompt-directed subdirectories in
 // requestedPath are created as needed. A rejected path returns an error
-// matching [ErrPathEscape]; any other failure (unwritable directory, full
+// matching [ErrPathEscape], collision-suffix exhaustion one matching
+// [ErrNameExhausted]; any other failure (unwritable directory, full
 // disk, ...) is returned as-is for the caller to surface.
 func Write(workspaceRoot, requestedPath string, data []byte, mimeType string) (Result, error) {
 	return write(workspaceRoot, requestedPath, bytes.NewReader(data), mimeType)
@@ -146,7 +152,7 @@ func claimAndPublish(root *os.Root, dir, base, ext string, r io.Reader) (string,
 		}
 		return rel, nil
 	}
-	return "", fmt.Errorf("no free name for %q after %d attempts", path.Join(dir, base+ext), maxNameAttempts)
+	return "", fmt.Errorf("%w: %q after %d attempts", ErrNameExhausted, path.Join(dir, base+ext), maxNameAttempts)
 }
 
 // publish writes r to a sibling temp file, syncs it, and renames it over

pkg/workspacemedia/writer_test.go

@@ -227,6 +227,17 @@ func TestWrite_CollisionAfterExtensionCorrection(t *testing.T) {
 	assert.True(t, res.ExtensionCorrected)
 }
 
+func TestWrite_CollisionExhaustionReturnsErrNameExhausted(t *testing.T) {
+	root := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(root, "pic.png"), []byte("x"), 0o644))
+	for n := 1; n < maxNameAttempts; n++ {
+		require.NoError(t, os.WriteFile(filepath.Join(root, fmt.Sprintf("pic-%d.png", n)), []byte("x"), 0o644))
+	}
+
+	_, err := Write(root, "pic.png", pngData, "image/png")
+	require.ErrorIs(t, err, ErrNameExhausted)
+}
+
 func TestWrite_ConcurrentSameNameWriters(t *testing.T) {
 	root := t.TempDir()
 	const writers = 16