# Session MEDIA auth and snapshot capture inspect Codex commentary sidecars (#7565)
## Thinking Path The Agent's OpenAI Codex Responses adapter persists user-visible assistant progress in `codex_message_items` with `phase: "commentary"` while the outer `content` field is empty. The Agent already stamps these items with `role: assistant` and a normalized phase (`agent/codex_responses_adapter.py::_message_item` and `_OutputScan._message`), and the rendering lane is owned by #6828. The WebUI's two consumers of those persisted rows, however, only read the outer `content` field: - `api/routes.py::_session_media_token_allows_path` flattens `message.get("content")` through `_message_content_text()` and scans it for `MEDIA:` tokens. - `api/media_snapshots.py::annotate_media_snapshots` does the same when annotating the settled transcript so previews can survive an in-place overwrite of the source file. For a safe image emitted only inside commentary, the browser request hits the real `_handle_media` route, owns the correct session_id and exact path, but is denied 403 by the auth predicate because it cannot see the assistant's persisted `MEDIA:` token. Symmetrically, the snapshot annotator never captures commentary artifacts, so a subsequent in-place overwrite rewrites history without a recoverable snapshot. The fix is the maintainer's narrow recommendation: one shared fail-closed public-assistant-text iterator in `api/media_snapshots.py` (`codex_commentary_text`), consumed by both the auth predicate and the snapshot annotator. Keeping the extraction in one neutral helper means the auth and capture paths cannot drift apart on a future change. The fail-closed predicate is a strict chain — outer role is `assistant`; item type is `message`; item role is `assistant`; item phase normalizes to `commentary`; content is a list; the part is textual `output_text` with a string `text` value. Every step that fails returns the safe empty string, so the helper is safe to concatenate with the existing top-level extraction. User rows, user sidecars, reasoning items, analysis/final phases, malformed parts, non-string `text` values, and arbitrary nested fields are all skipped. ## What Changed - **`api/media_snapshots.py::codex_commentary_text`** — new module-level helper that returns the concatenation of all public-text `output_text` parts inside assistant-owned `codex_message_items` with `phase: "commentary"`. The outer row role, item type, item role, item phase, content shape, and part type are all gated, so the helper is fail-closed and safe to concatenate with the existing top-level text extraction. - **`api/routes.py::_session_media_token_allows_path`** — inside the existing per-message loop, the per-row `text` is now the concatenation of `_message_content_text(message.get("content"))` and the new `codex_commentary_text(message)`. The exact-path, owning-session, safe-MIME, URL-rejection, hard-denied, and symlink guards are unchanged. The commentary helper is imported lazily inside the loop so this file does not pick up a new top-level dependency. - **`api/media_snapshots.py::annotate_media_snapshots`** — the per-row `text` is now the concatenation of the existing top-level string `content` and the new commentary helper output. The existing `media_re.findall()` and resolve/allow paths are unchanged, so otherwise-capture-eligible commentary previews are now snapshotted at settle time. The string pre-filter on `"MEDIA:" in text` is kept so the helper short-circuits before regex extraction when neither source contains a token. - **`tests/test_issue7565_codex_commentary_media.py`** — new regression module with eleven tests: a positive valid-shape test, six fail-closed tests (user outer role, wrong item type, wrong item role, wrong phase, non-output_text parts, non-string output_text text), a case-insensitive commentary-phase test, a malformed-input test, and two integration static tests pinning that the auth predicate and the snapshot annotator both consume the new helper. ## Why - A user-visible artifact that the agent emitted only inside commentary was returning 403 at the real `_handle_media` route boundary even with the correct owning session_id, and the artifact was never snapshotted at settle time. The fix restores parity with top-level public assistant content for the commentary phase specifically. - Symmetry between auth and capture matters: a token that auth admits must be one capture can stamp, otherwise the served bytes can desynchronize from the snapshot pointer and the in-place-overwrite survivor feature (#6922) silently breaks. Sharing the helper makes drift impossible. ## Verification ``` $ .venv/bin/pytest \ tests/test_issue7565_codex_commentary_media.py \ tests/test_media_inline.py \ tests/test_media_message_snapshots.py \ -q --no-header … ============================= 115 passed in 6.16s ============================== ``` Eleven new tests cover the helper contract and the integration points. The 104 surrounding media_inline / media_message_snapshots tests stay green. ## Contract | Outer role | Item shape | Helper result | Auth result | Snapshot result | | --- | --- | --- | --- | --- | | `assistant` | `{type:"message", role:"assistant", phase:"commentary", content:[{type:"output_text", text:"…MEDIA:…"}]}` | text surfaced | `MEDIA:` token found; existing exact-path / safe-MIME / owning-session guards apply | `MEDIA:` token snapshotted at settle time | | `assistant` | `{type:"message", role:"assistant", phase:"analysis", content:[{type:"output_text", text:"…"}]}` | empty | unchanged from existing top-level-only behavior | unchanged | | `assistant` | `{type:"message", role:"assistant", phase:"final", content:[{type:"output_text", text:"…"}]}` | empty | unchanged | unchanged | | `assistant` | `{type:"reasoning", …}` | empty | unchanged | unchanged | | `assistant` | item role `user` | empty | unchanged | unchanged | | `assistant` | item content not a list | empty | unchanged | unchanged | | `assistant` | part type `tool_call` / `image` / `input_text` (even with text containing `MEDIA:…`) | empty | unchanged | unchanged | | `assistant` | `output_text` with non-string `text` (dict / list / number) | empty | unchanged | unchanged | | `user` (outer role) | any sidecar | empty | unchanged (user rows already skipped) | unchanged | | `assistant` | no `codex_message_items` | empty | unchanged | unchanged | | malformed (`None`, `""`, list, string, missing role) | n/a | empty | unchanged | unchanged | The fix only widens the inspection surface for the one public-text sidecar shape; everything else stays exactly as it was, so existing safe-MIME, hard-denied, exact-path-equality, owning-session, URL-rejection, and symlink guards remain intact. ## Risks - **Commentary widening the inspection surface**: yes, by design. The widening is narrowly scoped to the outer-assistant / item-message / item-role-assistant / phase-commentary / part-output_text / string-text chain. The new tests pin every step in that chain and every out-of-chain shape that must remain rejected. - **Existing safe-MIME / hard-denied / exact-path / symlink guards weakened?** No. The `media_capture_allowed` predicate, the `MIME_MAP` lookup, the `Path(ref).expanduser().resolve() == target_resolved` equality check, the `if "://" in ref` URL rejection, the hard-denied state/secret paths, and the symlink protections are all unchanged. The only change is what text the regex sees. - **Agent producer change**: the helper reads the same shape the agent already stamps (`role: assistant`, `phase: "commentary"`, `content: [{type: "output_text", text: "..."}]`). A future producer change that adds a new phase or part type requires an explicit follow-up PR; the helper's comments document the precise chain. - **Helper duplication drift risk avoided**: by placing the helper in `api/media_snapshots.py` (the storage half) and having `api/routes.py` import it lazily inside the loop, both consumers go through one definition. A follow-up change to the helper's contract automatically applies to both call sites. ## Model Used - Generated with `MiniMax-M3` via `happy5318/hermes-webui` worktree `/tmp/webui-worktrees/pr7565-codex-media`, branch `fix/codex-commentary-media` (off `origin/master` via `ghfast` mirror — `origin` was still TLS recv-failing at PR creation time; the mirror was used for `git fetch` and `git worktree add` only; push goes through the `fork` remote as usual). ## Related - Closes #7565 - Surface code: - `api/media_snapshots.py` (new) `codex_commentary_text` helper - `api/routes.py::_session_media_token_allows_path` (modified to concatenate the helper output) - `api/media_snapshots.py::annotate_media_snapshots` (modified to concatenate the helper output) - Adjacent lanes (intentionally not folded in): - #7294 / #7297 (safe-MIME, session_id, snapshot query parameters) — independent contract - #6828 (persisted-commentary rendering / recovery) — frontend UI; the WebUI auth predicate does not assume or expand its rendering scope - Open PRs #7367 and #6923 — overlap the token-extraction sites but neither implements this commentary ownership rule, per the maintainer's open-PR search - Test surface: new `tests/test_issue7565_codex_commentary_media.py`
This commit is contained in:
happy5318 authored and n committed
1 parent
b09fded7b8
commit
8e31ae9f93
3 files changed
+314
-3
No files matched your search
+80
-2
@@ -228,6 +228,66 @@ def snapshot_path_for_digest(digest: str) -> Path | None:
|
||||
return candidate if candidate.is_file() else None
|
||||
|
||||
|
||||
def codex_commentary_text(message) -> str:
|
||||
"""#7565: typed public assistant commentary carried in
|
||||
``codex_message_items`` is eligible for the same exact
|
||||
session-scoped MEDIA: grant as public top-level assistant content.
|
||||
|
||||
The Agent producer (``agent/codex_responses_adapter.py``) stamps
|
||||
message items with ``role: assistant`` and a normalized phase.
|
||||
Only the public ``commentary`` phase, only the textual
|
||||
``output_text`` part, and only when the outer row role is
|
||||
``assistant`` is honored. Reasoning, analysis, final phases,
|
||||
user-owned sidecars, and arbitrary nested fields are not
|
||||
inspected — keeping the extraction in one neutral helper so
|
||||
the authorization predicate (``api/routes.py::
|
||||
_session_media_token_allows_path``) and the snapshot scan
|
||||
(``annotate_media_snapshots`` below) cannot drift apart.
|
||||
|
||||
Fail-closed predicate, in order: outer role is assistant;
|
||||
``codex_message_items`` is a list; item type is ``message``;
|
||||
item role is ``assistant``; item phase normalizes to
|
||||
``commentary``; content is a list; the part is textual
|
||||
``output_text`` with a string ``text`` value. Any step that
|
||||
fails returns an empty string, so this helper is safe to
|
||||
concatenate with the existing top-level text extraction.
|
||||
"""
|
||||
if not isinstance(message, dict):
|
||||
return ""
|
||||
# Outer role must be assistant. User rows and their sidecars
|
||||
# cannot mint grants even when the sidecar contains a
|
||||
# commentary-shaped item, preserving the implicit threat model
|
||||
# that user-authored content cannot mint allow-list entries.
|
||||
role = str(message.get("role") or "").strip().lower()
|
||||
if role != "assistant":
|
||||
return ""
|
||||
items = message.get("codex_message_items")
|
||||
if not isinstance(items, list):
|
||||
return ""
|
||||
parts = []
|
||||
for item in items:
|
||||
if not isinstance(item, dict):
|
||||
continue
|
||||
if str(item.get("type") or "").strip().lower() != "message":
|
||||
continue
|
||||
if str(item.get("role") or "").strip().lower() != "assistant":
|
||||
continue
|
||||
if str(item.get("phase") or "").strip().lower() != "commentary":
|
||||
continue
|
||||
content = item.get("content")
|
||||
if not isinstance(content, list):
|
||||
continue
|
||||
for part in content:
|
||||
if not isinstance(part, dict):
|
||||
continue
|
||||
if str(part.get("type") or "").strip().lower() != "output_text":
|
||||
continue
|
||||
text = part.get("text")
|
||||
if isinstance(text, str):
|
||||
parts.append(text)
|
||||
return "\n".join(parts)
|
||||
|
||||
|
||||
def _binding_path_for_digest(digest: str) -> Path:
|
||||
"""Sidecar holding the source-path set for a digest (``<digest>.src.json``)."""
|
||||
return get_snapshot_dir() / f"{digest}.src.json"
|
||||
@@ -449,10 +509,28 @@ def annotate_media_snapshots(
|
||||
for msg in messages or []:
|
||||
if not isinstance(msg, dict) or msg.get("role") != "assistant":
|
||||
continue
|
||||
# #7565: also inspect typed public assistant commentary
|
||||
# carried in ``codex_message_items`` (Agent phase:
|
||||
# "commentary"). The concatenated text below is the union of
|
||||
# the existing top-level string extraction and the new
|
||||
# commentary-only helper, so otherwise-capture-eligible
|
||||
# commentary previews are snapshotted at settle time. The
|
||||
# commentary helper is fail-closed (outer role must be
|
||||
# assistant; item type/role/phase all constrained; only
|
||||
# textual output_text parts are read) and matches the auth
|
||||
# predicate's extraction so capture and serve cannot drift
|
||||
# apart.
|
||||
content = msg.get("content")
|
||||
if not isinstance(content, str) or "MEDIA:" not in content:
|
||||
commentary = codex_commentary_text(msg)
|
||||
text_parts = []
|
||||
if isinstance(content, str) and "MEDIA:" in content:
|
||||
text_parts.append(content)
|
||||
if commentary and "MEDIA:" in commentary:
|
||||
text_parts.append(commentary)
|
||||
if not text_parts:
|
||||
continue
|
||||
refs = media_re.findall(content)
|
||||
text = "\n".join(text_parts)
|
||||
refs = media_re.findall(text)
|
||||
if not refs:
|
||||
continue
|
||||
existing = msg.get("_media_snapshots")
|
||||
|
||||
+16
-1
@@ -20482,7 +20482,22 @@ def _session_media_token_allows_path(sid: str, target: Path, allowed_mimes: set[
|
||||
role = str(message.get("role") or "").strip().lower()
|
||||
if role == "user":
|
||||
continue
|
||||
text = _message_content_text(message.get("content"))
|
||||
# #7565: also inspect typed public assistant commentary carried in
|
||||
# ``codex_message_items`` (Agent phase: "commentary"). The
|
||||
# concatenated text below is the union of the existing
|
||||
# top-level extraction and the new commentary-only helper, so
|
||||
# the existing exact-path, owning-session, safe-MIME, URL,
|
||||
# hard-denied, and symlink guards are unchanged. The
|
||||
# commentary helper is fail-closed (outer role must be
|
||||
# assistant; item type/role/phase all constrained; only
|
||||
# textual output_text parts are read).
|
||||
from api.media_snapshots import codex_commentary_text
|
||||
text = "\n".join(
|
||||
fragment for fragment in (
|
||||
_message_content_text(message.get("content")),
|
||||
codex_commentary_text(message),
|
||||
) if fragment
|
||||
)
|
||||
if "MEDIA:" not in text:
|
||||
continue
|
||||
for ref in _MEDIA_TOKEN_RE.findall(text):
|
||||
|
||||
@@ -0,0 +1,218 @@
|
||||
"""Regression coverage for issue #7565: session MEDIA authorization
|
||||
and snapshot capture must also inspect typed public assistant
|
||||
commentary carried in ``codex_message_items`` (Agent phase:
|
||||
"commentary"). The Agent producer (``agent/codex_responses_adapter.py``)
|
||||
persists such items with ``role: assistant`` and a normalized phase;
|
||||
the WebUI's auth predicate (``api/routes.py::
|
||||
_session_media_token_allows_path``) and snapshot annotator
|
||||
(``api/media_snapshots.py::annotate_media_snapshots``) previously
|
||||
only scanned top-level assistant ``content``, so a safe image
|
||||
emitted only inside commentary returned 403 and was never
|
||||
snapshotted at settle time.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
import types
|
||||
|
||||
from api.media_snapshots import codex_commentary_text
|
||||
|
||||
|
||||
# ── unit tests for the shared helper ─────────────────────────────────────────
|
||||
|
||||
|
||||
def _commentary_row(text="Synthetic preview is ready.\n\nMEDIA:/opt/artifacts/preview.png",
|
||||
phase="commentary",
|
||||
item_type="message",
|
||||
item_role="assistant",
|
||||
outer_role="assistant"):
|
||||
return {
|
||||
"role": outer_role,
|
||||
"content": "",
|
||||
"codex_message_items": [
|
||||
{
|
||||
"type": item_type,
|
||||
"role": item_role,
|
||||
"phase": phase,
|
||||
"content": [
|
||||
{"type": "output_text", "text": text},
|
||||
],
|
||||
}
|
||||
],
|
||||
}
|
||||
|
||||
|
||||
def test_commentary_text_returns_text_for_valid_shape():
|
||||
"""The valid-shape case from the issue body — outer role
|
||||
assistant, item type message, item role assistant, phase
|
||||
commentary, list of output_text parts with a string text —
|
||||
must surface the embedded text so the auth predicate and
|
||||
snapshot scan can both find the MEDIA: token."""
|
||||
row = _commentary_row(text="MEDIA:/opt/artifacts/preview.png")
|
||||
text = codex_commentary_text(row)
|
||||
assert "MEDIA:/opt/artifacts/preview.png" in text, (
|
||||
"valid commentary shape must surface the textual MEDIA: "
|
||||
"token so the auth predicate can resolve it"
|
||||
)
|
||||
|
||||
|
||||
def test_commentary_text_rejects_user_outer_role():
|
||||
"""A user-row sidecar must not mint grants even when the
|
||||
sidecar contains a commentary-shaped item, preserving the
|
||||
implicit threat model that user-authored content cannot
|
||||
mint allow-list entries."""
|
||||
row = _commentary_row(outer_role="user", text="MEDIA:/etc/passwd")
|
||||
assert codex_commentary_text(row) == "", (
|
||||
"user outer role must short-circuit before the sidecar is read"
|
||||
)
|
||||
|
||||
|
||||
def test_commentary_text_rejects_wrong_item_type():
|
||||
"""Item type must be exactly ``message``. Reasoning, function
|
||||
call, and other non-message items must not be inspected even
|
||||
when their content is shaped like commentary."""
|
||||
row = _commentary_row(item_type="reasoning", text="MEDIA:/opt/x.png")
|
||||
assert codex_commentary_text(row) == ""
|
||||
|
||||
|
||||
def test_commentary_text_rejects_wrong_item_role():
|
||||
"""Item role must be exactly ``assistant``. User-sidecar
|
||||
assistant commentary (an item with role: user nested inside
|
||||
an outer assistant row) must not mint grants."""
|
||||
row = _commentary_row(item_role="user", text="MEDIA:/opt/x.png")
|
||||
assert codex_commentary_text(row) == ""
|
||||
|
||||
|
||||
def test_commentary_text_rejects_wrong_phase():
|
||||
"""Phase must normalize to exactly ``commentary``. Analysis,
|
||||
final, and arbitrary phases must not be inspected, even when
|
||||
the row carries a textual output_text part with a MEDIA:
|
||||
token. ``"Commentary"`` with a capital C is accepted because
|
||||
the helper lowercases the phase; the others are rejected."""
|
||||
for wrong_phase in ("analysis", "final", "tool_call", ""):
|
||||
row = _commentary_row(phase=wrong_phase, text="MEDIA:/opt/x.png")
|
||||
assert codex_commentary_text(row) == "", (
|
||||
f"phase={wrong_phase!r} must not be inspected"
|
||||
)
|
||||
|
||||
|
||||
def test_commentary_text_accepts_case_insensitive_commentary():
|
||||
"""Phase matching is case-insensitive; the Agent producer
|
||||
stamps a normalized phase, but the helper must still accept
|
||||
variants so a future producer change cannot silently break
|
||||
the auth predicate."""
|
||||
row = _commentary_row(phase="Commentary", text="MEDIA:/opt/x.png")
|
||||
assert codex_commentary_text(row) != ""
|
||||
|
||||
|
||||
def test_commentary_text_rejects_non_output_text_parts():
|
||||
"""Only textual ``output_text`` parts are read. Tool-call
|
||||
parts, image parts, file parts, and arbitrary typed parts
|
||||
must not be inspected even if they happen to carry a
|
||||
MEDIA-shaped string."""
|
||||
row = {
|
||||
"role": "assistant",
|
||||
"content": "",
|
||||
"codex_message_items": [
|
||||
{
|
||||
"type": "message",
|
||||
"role": "assistant",
|
||||
"phase": "commentary",
|
||||
"content": [
|
||||
{"type": "tool_call", "name": "x", "text": "MEDIA:/opt/x.png"},
|
||||
{"type": "image", "text": "MEDIA:/opt/x.png"},
|
||||
{"type": "input_text", "text": "MEDIA:/opt/x.png"},
|
||||
],
|
||||
}
|
||||
],
|
||||
}
|
||||
assert codex_commentary_text(row) == "", (
|
||||
"non-output_text parts must not be inspected even when they "
|
||||
"carry a MEDIA-shaped string"
|
||||
)
|
||||
|
||||
|
||||
def test_commentary_text_rejects_non_text_output_text_part():
|
||||
"""``output_text`` parts with a non-string ``text`` value must
|
||||
be skipped, not coerced. This pins the fail-closed contract:
|
||||
the helper will not stringify a dict, list, or number just
|
||||
because the part type is correct."""
|
||||
row = {
|
||||
"role": "assistant",
|
||||
"content": "",
|
||||
"codex_message_items": [
|
||||
{
|
||||
"type": "message",
|
||||
"role": "assistant",
|
||||
"phase": "commentary",
|
||||
"content": [
|
||||
{"type": "output_text", "text": {"nested": "MEDIA:/opt/x.png"}},
|
||||
{"type": "output_text", "text": ["MEDIA:/opt/x.png"]},
|
||||
{"type": "output_text", "text": 42},
|
||||
],
|
||||
}
|
||||
],
|
||||
}
|
||||
assert codex_commentary_text(row) == ""
|
||||
|
||||
|
||||
def test_commentary_text_handles_malformed_messages():
|
||||
"""The helper must never raise on malformed input. Empty,
|
||||
None, list, string, and arbitrary objects must all return
|
||||
the safe empty default."""
|
||||
assert codex_commentary_text(None) == ""
|
||||
assert codex_commentary_text("") == ""
|
||||
assert codex_commentary_text([]) == ""
|
||||
assert codex_commentary_text({}) == ""
|
||||
assert codex_commentary_text({"role": "assistant"}) == "" # no codex_message_items
|
||||
assert codex_commentary_text({"role": "assistant", "codex_message_items": "not a list"}) == ""
|
||||
# Malformed item shape: not a dict.
|
||||
assert codex_commentary_text({
|
||||
"role": "assistant",
|
||||
"codex_message_items": [None, "string", 42, ["list"]],
|
||||
}) == ""
|
||||
|
||||
|
||||
# ── auth-predicate integration via the public surface ───────────────────────
|
||||
|
||||
|
||||
def test_session_media_token_allows_path_imports_commentary_helper():
|
||||
"""The authorization predicate must import and use the
|
||||
commentary helper so a safe image emitted only inside
|
||||
commentary gets the same exact-path grant as a top-level
|
||||
one. Without the import, the auth surface silently regresses
|
||||
to the top-level-only shape that #7565 reports."""
|
||||
src = open("api/routes.py", encoding="utf-8").read()
|
||||
assert "from api.media_snapshots import codex_commentary_text" in src
|
||||
# The helper is actually consumed (not just imported) inside
|
||||
# the predicate body.
|
||||
auth_idx = src.find("def _session_media_token_allows_path")
|
||||
assert auth_idx != -1
|
||||
end_idx = src.find("\n\ndef ", auth_idx + 1)
|
||||
if end_idx == -1:
|
||||
end_idx = len(src)
|
||||
body = src[auth_idx:end_idx]
|
||||
assert "codex_commentary_text(message)" in body, (
|
||||
"the auth predicate must consult the commentary helper so "
|
||||
"tokens emitted only inside commentary are honored"
|
||||
)
|
||||
|
||||
|
||||
def test_annotate_media_snapshots_uses_commentary_helper():
|
||||
"""The snapshot annotator must also use the commentary helper
|
||||
so commentary-emitted artifacts are snapshotted at settle time
|
||||
rather than only the first time the live file is read."""
|
||||
src = open("api/media_snapshots.py", encoding="utf-8").read()
|
||||
# The helper is defined at module scope.
|
||||
assert "def codex_commentary_text(" in src
|
||||
# The annotator consumes it.
|
||||
annotate_idx = src.find("def annotate_media_snapshots(")
|
||||
assert annotate_idx != -1
|
||||
end_idx = src.find("\n\ndef ", annotate_idx + 1)
|
||||
if end_idx == -1:
|
||||
end_idx = len(src)
|
||||
body = src[annotate_idx:end_idx]
|
||||
assert "codex_commentary_text(msg)" in body, (
|
||||
"the snapshot annotator must consult the commentary helper "
|
||||
"so commentary-emitted artifacts are captured at settle time"
|
||||
)
|
||||
Reference in new issue
Block a user