feat(#3996): classify generated-media save failures into safe user-visible reasons - #4025
Draft
aheritier wants to merge 4 commits into
Draft
feat(#3996): classify generated-media save failures into safe user-visible reasons#4025aheritier wants to merge 4 commits into
aheritier wants to merge 4 commits into
Conversation
Extend the shared chat.MessageDelta streaming pipeline into persistence: recordAssistantMessage materializes every streamed media delta through pkg/workspacemedia into the owning session's workspace (the effective WorkingDir, resolved parent-chain-aware via session.ResolveWorkingDir) and appends a document part carrying only an owner-qualified, workspace-relative reference (ArtifactRoot=workspace) plus MIME/name/size — never raw bytes — so session JSON never carries generated image base64. The owning session ID is recorded on the part so copies into branched/forked sessions keep naming the original owner; a provider display name the writer refuses falls back to a generic generated-N rather than losing the item, and when no workspace root is available every item fails with a per-item warning — there is deliberately no other target location, so generated files never land outside the workspace. A default message transform (BuiltinStripGeneratedMedia) strips these parts from outgoing provider history on later turns, replacing each with a short, stable placeholder (position/count, sanitized name, MIME) so a media-only reply never becomes an empty turn; it runs before strip_unsupported_modalities, and compaction counts a Content/MultiContent text mirror once. Branch/fork owner qualification, transform order, and per-item placeholder behavior are covered by tests.
Provider-supplied display names and MIME types are untrusted model output; this hardens every place they are stored or shown: - Every stored or displayed display name now goes through the shared chat.SanitizeDisplayName helper (introduced with the Gemini request diagnostics), so control characters, path separators, traversal-like sequences, and angle brackets can never forge an XML/tag boundary in harness prompts, and every field is bounded by MaxSanitizedFieldBytes (128, UTF-8-safe) via chat.TruncateUTF8Bytes. - sanitizeMimeType trims control characters, enforces the same field bound, and requires conservative RFC 6838 type/subtype syntax, falling back to application/octet-stream; only the safe MIME is ever persisted. - Materialization warnings and strip-time placeholders are capped at 512 bytes after formatting, fall back to canonical display names, and never include paths or raw OS errors; harness prompt interpolation runs both sanitizers first, closing an XML/role injection path. - strip_unsupported_modalities skips generated-media parts independently of transform registration order, so a reordering can never silently drop a generated artifact before its placeholder replaces it. Tests cover the classification chain end-to-end: bounded/overlong metadata, empty-name/MIME fallbacks, multi-artifact placeholders per provider converter, ownerless marker preservation, harness injection inertness, and genuine single-call batch partial success.
Add a generated-media manifest to the session stores (migration 028_add_generated_media_manifest_table on the SQLite store, an in-memory map on the memory store) recording owner session + final relative path + sanitized MIME + creation time, written by materialization only after a successful workspace write. Resolution must verify references against it, so tampered session JSON can never select an arbitrary workspace file such as .env; the store API also rejects absolute/traversal/backslash/NUL path shapes on both add and lookup. The table has no FK to sessions (materialization can precede the lazily persisted session row); DeleteSession prunes it explicitly. The persisted part keeps carrying ArtifactRoot=workspace plus the exact final workspace-relative path the writer returned; strip/no-resend predicates still key on a non-empty ArtifactPath, and a reference whose root kind is empty/unknown stays unresolvable by design. No workspace root (or an unwritable one) keeps the existing per-item sanitized warning contract with no data-dir fallback, preserving the turn's text and surviving siblings. Writer MIME/extension corrections now surface a bounded notice naming the final path; collision suffixes are persisted verbatim. Prompt-directed naming and the out-of-workspace confirmation flow are deliberately not part of this slice, and the TUI resolver is not yet wired to the manifest/workspace root. Also harden session.ResolveWorkingDir: stored roots must be exactly clean (filepath.Clean(dir) == dir), rejecting absolute values smuggling .. segments.
…sible 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.
aheritier
added
area/docs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Classifies generated-media save failures into safe user-visible reasons, with runtime/session persistence, provider placeholders, and documentation/tests.
Why
Users need actionable failures when generated media cannot be persisted, without leaking filesystem or provider internals.
Validation
task test; materialization, failure-classification, manifest, migration, and provider tests.Test instructions
Run:
task testGenerate media while forcing its save operation to fail. Expected: the user sees a safe, actionable classified reason without filesystem or provider internals, and the session remains recoverable.