2a742e13d696b39806a5d8a02f81f84202f4e0a8
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 happy5318
|
bc37675bc2 |
fix(tests): drop unused sys/types imports in #7565 codex commentary test
The test file imports `sys` and `types` but never references either. Ruff F401 flags them. No test or runtime behavior change. - Remove the 2 unused import lines - 11 tests still pass - No new ruff violations |
||
|
happy5318
|
8e31ae9f93 |
# 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` |