Skip to content

Fix: Harden sandbox chat attachments, session history, and stream lifecycle#39

Merged
joeqian10 merged 5 commits intomasterfrom
fix/chat_attachment
Mar 19, 2026
Merged

Fix: Harden sandbox chat attachments, session history, and stream lifecycle#39
joeqian10 merged 5 commits intomasterfrom
fix/chat_attachment

Conversation

@joeqian10
Copy link
Contributor

Description

This PR improves spoon-bot behavior in sandbox chat by fixing stale request leakage, making attachment handling workspace-safe, and preserving structured attachment context across session/history flows.

What this PR changes

  • makes anti-loop think() wrapping request-scoped instead of stacking across requests
  • restores the agent’s base think() implementation after process, stream, and process_with_thinking
  • cancels background run() tasks when a streamed request is cancelled or the stream is closed
  • avoids persisting partial/cancelled streamed output into session history
  • adds support for structured attachments alongside media in websocket and REST chat entrypoints
  • allows attachment-only chat requests and injects attachment context into the request when needed
  • validates attachment/media paths against the sandbox workspace instead of accepting arbitrary local files
  • validates imported session attachment/media metadata before persisting it
  • persists attachments and media in session history instead of flattening everything to plain text
  • rehydrates session history back into runtime memory with attachment context preserved
  • logs warnings when persisted attachment/media refs are dropped during history sync
  • improves multimodal/media error visibility by no longer silently swallowing invalid file refs

Why

Before this change, sandbox conversations could drift in a few bad ways:

  • a new request could inherit anti-loop prompt state from a previous request
  • cancelled streams could keep running in the background
  • attachment metadata could be available for the current request but get lost in later turns, reconnects, or session import/export
  • gateway callers could pass arbitrary existing local paths as media / attachments
  • invalid file refs could quietly degrade into text-only behavior

Together, these issues made sandbox attachment-based answers unreliable and could cause cross-request task leakage.

Expected result

  • stopping or disconnecting a stream no longer leaves the previous task running
  • a new chat starts from clean per-request state
  • attachment-only sandbox requests work consistently
  • structured attachment context survives session save/export/import/history hydration
  • only real files inside the configured workspace are accepted as attachment/media inputs
  • invalid attachment refs fail fast instead of degrading silently

@veithly
Copy link
Contributor

veithly commented Mar 19, 2026

@codex review it

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

_stream_sse(agent, processed_message, None, False, session_key=session_key),

P1 Badge Pass the new thinking argument in voice streaming

After _stream_sse() gained the new attachments positional parameter, this streaming call was not updated. POST /v1/agent/voice/chat with stream=true will now raise TypeError (missing 1 required positional argument: 'thinking') before any bytes are sent, so voice streaming is completely broken on this path.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@joeqian10
Copy link
Contributor Author

@codex review again and resolve conversations

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 04724ebf2b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@joeqian10
Copy link
Contributor Author

@codex review again

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 289ff98a99

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +1050 to +1054
extras["media"] = _validate_media_paths(_string_list_from_any(extras.get("media")))
if "attachments" in extras:
extras["attachments"] = _validate_attachment_paths(
_normalize_attachment_refs(extras.get("attachments"))
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Validate imported attachment refs before clearing the session

These new import-time validations run after session.messages.clear() has already executed a few lines above. That means an import with one bad media or attachments path now returns an error but leaves the existing session emptied in memory, so a failed session.import can destroy the user's current history. Build and validate the replacement message list first, then swap it into the live session only after validation succeeds.

Useful? React with 👍 / 👎.

@joeqian10
Copy link
Contributor Author

@codex review and list all the issues at one time

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

except Exception as e:
logger.error(f"Session import failed: {e}")
return {
"success": False,
"error": f"Import failed: {e}",

P2 Badge Stop swallowing session.import validation errors

This except Exception converts bad media/attachments imports into a normal {"success": false} response instead of the standard WS error frame. handle_request() only emits type: "error" / INVALID_PARAMS when a ValueError escapes, so clients that rely on WS error semantics will miss invalid import failures and can treat the import as successful. This happens for any session.import payload containing an out-of-workspace or missing file reference.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +151 to +155
resolved = _resolve_workspace_file(path)
if resolved is None:
invalid.append(path)
continue
resolved_media.append(str(resolved))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve portable media URIs instead of host paths

By replacing each accepted media entry with str(resolved), the request loses the original /workspace/... URI and stores a host-specific absolute path instead. AgentLoop persists that list verbatim and session.export returns it verbatim, while session.import revalidates media against the current workspace root; as a result, a session exported from one sandbox/workspace will fail to import into another even when the same file exists there. This breaks round-tripping for any conversation that included media.

Useful? React with 👍 / 👎.

@joeqian10 joeqian10 merged commit 8e5b4d2 into master Mar 19, 2026
1 of 2 checks passed
@joeqian10 joeqian10 deleted the fix/chat_attachment branch March 19, 2026 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants