Repository navigation
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: preserve the session workdir when resolving path-backed attachments.
I reviewed the full github/main...HEAD diff, the composer/transcript callers, /file and turn.send path policy, relevant history, backward compatibility, tests, and the Web UI/runtime architecture and naming rules in AGENTS.md, CONTEXT-MAP.md, and ui-web/CONTEXT.md. I found one concrete blocker, marked inline. I did not find weakened tests or additional merge-blocking issues.
Verification:
- Focused Vitest suite: 254 passed across
Dock.test.tsx,ComposerPage.test.tsx, andTranscriptPage.test.tsx. npm run type-check: passed.npm run lint: passed with four pre-existing warnings in untouched files.uv run pytest tests/test_rpc_turn_send.py -x: could not run because global test setup cannot import the optionalraven_everospackage (4 setup errors before test bodies).- Direct resolver reproduction: an absolute picture under a session project outside agent home raises
PermissionErrorwhen the agent-home fence is enabled.
9d3a577 to
d974e40
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the rebased github/main...HEAD diff and the new session-aware attachment fix, including the /file and turn.send callers, shared viewer_root behavior, normal and injected sends, path-policy compatibility, relevant history, and the Web UI/runtime constraints in AGENTS.md, CONTEXT-MAP.md, and ui-web/CONTEXT.md. The previous blocker is fixed and its thread is resolved. I found no weakened tests or new issue worth raising.
Verification:
uv run pytest tests/test_rpc_turn_send.py -x: 62 passed.uv run pytest tests/test_rpc_console.py -k viewer_root -x: 3 passed.- Focused Vitest suite for Dock, ComposerPage, and TranscriptPage: 254 passed.
npm run type-check: passed.- Focused Ruff check: passed.
git diff --check github/main...HEAD: passed.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the delta from the previously clean revision and rechecked the full github/main...HEAD change. The shared session/viewer path rules now live in raven.rpc.files, removing the new console/turn import cycle while preserving the console-facing names, /file behavior, restricted attachment resolution, and normal/injected send paths. I also checked relevant callers and history, backward compatibility, test integrity, and the architecture and repository constraints in AGENTS.md, CONTEXT-MAP.md, and ui-web/CONTEXT.md. I found no new issue worth raising, and my prior thread remains resolved.
Verification:
uv run pytest tests/test_import_cycle_budget.py -x: 4 passed.uv run pytest tests/test_rpc_turn_send.py -x: 62 passed.uv run pytest tests/test_rpc_console.py -k 'viewer_root or file' -x: 16 passed.- Focused Ruff check: passed.
git diff --check github/main...HEAD: passed.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: a fresh upload can resolve to an older same-named session file.
The new review record invalidates my earlier clean stance. I independently confirmed the concrete behavior reported in discussion 4238145333: when the session root and agent home both contain uploads/image.png, viewer_root(session, rel, home) selects the session copy, while fs.upload writes and returns the agent-home copy. The composer can therefore preview the newly uploaded bytes while turn.send silently hands the model the older file. That ordinary, user-visible data substitution must be resolved before merge.
The two additional reviewer blockers about global-listener ownership and the duplicated picture-extension table remain open under their author; I am not re-grading or resolving them. My own earlier attachment-root thread remains correctly resolved.
Verification this turn:
uv run pytest tests/test_rpc_turn_send.py tests/test_rpc_console.py -k 'turn_send or viewer_root or upload' -x: 70 passed.- Focused Vitest suite for Dock, ComposerPage, and globalListeners: 88 passed.
- Direct collision reproduction selected
<session>/uploads/image.pngcontainingOLDover<agent-home>/uploads/image.pngcontainingNEW.
|
Not a blocker -- two smaller items from the acceptance pass on head d36d455 (merged onto main 289426c), beside the three blocking threads. Severity of this one finding, not a verdict on the pull request. The blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
|
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the delta from the blocked revision and rechecked the full github/main...HEAD change, affected callers and history, backward compatibility, test integrity, and the repository architecture and rules in AGENTS.md, CONTEXT-MAP.md, ui-web/CONTEXT.md, and ui-web/CONTRIBUTING.md. The fresh-upload identity bug is fixed both at name allocation and at send time: upload paths avoid the current session's shadow names, and the live page hands turn.send the absolute file fs.upload wrote. The document listeners now belong to state/globalListeners, the picture-extension table is shared through lib, and the two attachment replay notes are covered. I found no new blocker or plain error.
The three open threads were raised by another reviewer and remain for that reviewer to close; my own earlier thread remains resolved.
Verification:
uv run pytest tests/test_rpc_turn_send.py tests/test_rpc_console.py tests/test_rpc_files.py tests/test_import_cycle_budget.py -x: 282 passed.- Focused Vitest suite across 9 affected files: 446 passed.
npm run type-check: passed.- Focused ESLint and Ruff checks: passed.
- Import-direction gate through Vitest: 6 passed.
git diff --check github/main...HEAD: passed.- A direct
nodeinvocation of the Vitest gate was invalid for its runner API; rerunning with the repository test command produced the passing result above.
08b0c99 to
23296bc
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This head rewrites commit history but has the exact same tree object as the previously reviewed clean revision (6d725a38c32db1495c59568a30a9ba6079e80ec2), with an empty 08b0c99a3a86..HEAD content diff. I rechecked the full github/main...HEAD change, affected callers and compatibility, test integrity, and the repository architecture and rules; the upload identity, global-listener ownership, shared picture-extension table, import-cycle repair, and attachment replay fixes remain intact. I found no new blocker or plain error.
The three threads opened by another reviewer now contain that reviewer's re-verification that they are fixed; resolving those threads remains theirs. My own earlier thread remains resolved.
Verification:
uv run pytest tests/test_rpc_turn_send.py tests/test_rpc_files.py tests/test_import_cycle_budget.py -x: 133 passed.- Focused Vitest suite across 6 affected files: 126 passed.
npm run type-check: passed.git diff --check github/main...HEAD: passed.
|
Not a blocker -- three items from the re-acceptance pass on head 23296bc (merged onto main 6126965), beside the blocking thread on Severity of this one finding, not a verdict on the pull request. The blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
|
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: distinguish uploaded attachments from dragged same-spelled session files.
The new cross-session reproduction changes my stance on this unchanged revision. I independently confirmed the shared page-lifetime mapping: after remember("uploads/image.png", "/agent-home/uploads/image.png"), a later mediaPath("uploads/image.png") for a dragged file from another session still returns the agent-home absolute path. Since mediaOf applies that lookup to every attachment path, the newly dragged file can be silently replaced by the earlier upload.
Focused verification remains green but does not cover this collision: npm test -- --run src/lib/uploadPaths.test.ts src/state/session/runtime.inject.test.ts src/chrome/Dock.test.tsx (46 tests passed).
My earlier clean stance is withdrawn; the open uploadPaths thread now blocks this revision from my side.
23296bc to
18486ec
Compare
|
Answers to both board notes; the blocking The note on the d36d455 pass. Both items were fixed in c8c3abd ("close the composer drag-in findings from review"), which your re-acceptance pass covered: a reloaded bubble gives each engine line to one note entry, in order, and The note on the 23296bc pass.
The description now covers both rounds. |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I independently verified the cross-session collision fix: uploads are staged by abs_path, dragged session files retain their own paths, mediaOf preserves what the note records, and the file route admits the absolute upload only when it resolves inside agent-home uploads/. The real Dock send/drop regression covers both same-spelled files. The earlier board notes are addressed by the same absolute staging and the guarded unreadable-session-root probe.
I covered the repository rules (AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and ui-web/CONTEXT.md), the full github/main...HEAD diff, the revision delta, affected callers and history, backward compatibility through the path fallback, test changes (the deleted alias-table unit tests are replaced by an end-to-end Dock regression rather than weakened), and the global-listener and path-fencing architecture constraints.
Verification: 319 backend tests passed; 229 focused UI tests passed; TypeScript type-check and the source-language gate passed. ESLint completed with zero errors and four existing hook warnings; focused Ruff checks passed. My earlier upload-path thread remains resolved; I did not resolve the other reviewer’s thread.
|
No blockers in the reviewed scope. Scope: I reviewed the full PR diff (31 changed files), including tests and documentation, and traced affected callers and contracts at Traced dragstart/drop through staging, attachment notes, mediaOf, normal and injected turn.send, /file, and the shared path resolver. Checked upload/template/direct-chat absolute staging, legacy path fallback, resolved-path confinement, session-root selection, listener ownership, the shared picture-extension table, replay pairing, and both generated RPC clients. The historical upload-identity and listener findings have fixes and re-verification in the existing threads; I am not filing them again. Verification (native Windows, Python 3.12, isolated source snapshots; local JUnit-output options omitted below):
The 22 base failures also fail at the head. The three additional cases stop while creating a symlink with WinError 1314, before reaching their functional assertions; these are a local validation gap, not passing cases. Frontend verification:
|
Dragging a picture from the transcript into the composer attached either nothing (Chrome on Windows and Linux, and Firefox, hand an in-page drag over with no File) or a re-upload of the File that Chrome on macOS names `download`, with no extension, so the sent bubble drew a file chip where the picture should have been. A drop that began in this document and names this origin's /file route in `text/uri-list` now attaches that file where it already is, by path, with nothing read or uploaded; the address wins over any File the drop also carries. A transcript picture still drawn from the upload's cached data URL puts its /file address in the list when it is dragged. An address is taken only when the drag that began here carried the same path from its start, so a site the reader drags from cannot make the composer attach an arbitrary local file. A File that does go up is now named by its type when its name spells no picture extension (download becomes download.png), which covers images dragged in from other sites. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
The composer now attaches a picture dragged out of the transcript by its own absolute path instead of uploading a copy, so the paths _resolve_media receives are not only uploads under agent home. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
…ricted With tools.restrict_to_workspace on, turn.send resolved every media path against agent home and fenced it there. The viewer serves a picture from the conversation's own working directory, so a picture the composer attaches by its own path, from a session pinned outside agent home, showed in the tray and was dropped from the turn without an error. turn.send now asks for that directory the way /file does (console._workspace_root), finds a relative path by the viewer's own rule (viewer_root, which now takes agent home from its caller), and with the setting on admits that directory beside agent home. A path that neither root holds is still refused, and a loop that cannot name the directory leaves agent home to stand in. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
tests/test_import_cycle_budget.py counted 45 modules inside an import cycle against a ceiling of 44. The session-aware resolver had turn.send import viewer_root and the session-root lookup from rpc.methods.console, and the console already reaches turn through rpc.methods.session, so the new edge closed console -> session -> turn -> console; the guard counts a function-local import exactly like a top-level one. viewer_root's rule and the session-root lookup now live in raven/rpc/files.py, which both modules import and which reaches neither. The console keeps its names (viewer_root as a wrapper that reads agent home where fs.upload does, _workspace_root and _UPLOAD_DIR as imports), so its callers and tests are unchanged. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
Name an upload past the session root's own uploads/ as well, so the relative path fs.upload answers can never mean two files: viewer_root reads the session's own root first, and both turn.send and /file follow it, so a same-named file there would answer for the upload and the reader would see one picture while the model was handed another. Register the composer's two page-lifetime drag listeners with the page's other document listeners (ui-web/CONTEXT.md's one place), and teach the order test the two new rows. Move the picture-extension table to lib/, read by the workspace viewer and the composer both, so the tray and the sent bubble cannot disagree about whether a name is a picture. Give each engine path one note entry when a sent message is read back: two entries can name suffixes of one path, and both reading it drew that file twice and lost the other. Pin the session key _media_root hands the resolver with a loop that answers per key, which the shared-root stub could not tell from a dropped one. Co-authored-by: Claude (deepseek-flash) <noreply@anthropic.com>
fs.upload answers with a short path (uploads/<name>) and with the absolute file it wrote. The note keeps the short one -- the bubble renders it and /file re-roots it -- while the turn now receives the absolute file: viewer_root reads the session's own root before agent home, so a same-named file appearing there after the upload could answer for it. lib/uploadPaths keeps the answer by path, page-lifetime and keyed like lib/attachmentCache, and mediaOf maps each path it recovers from the note through it. A path no upload minted is left to viewer_root as before, and so is a note restored after a reload, when the answer is gone. Pin it thrice: the store case shows an upload is remembered, the runtime case shows the turn.send media carries the file, and the turn.send case shows a restricted, pinned session admits the absolute upload path. Co-authored-by: Claude (deepseek-flash) <noreply@anthropic.com>
…wered fs.upload answers uploads/<name> as well as the absolute file it wrote. The composer staged the short one, and the send mapped it back through a table kept for as long as the tab stays open and keyed by that spelling alone. So once an upload had answered uploads/x.png, a different file with that spelling - a session root's own uploads/x.png, dragged in from the workspace pane - reached the turn as the upload. The two messages carried byte-identical notes, so nothing that reads the text could tell them apart. The composer now stages an upload, and a picked template, by the file the server wrote, falling back to the short path only where a host answers none, and files the bubble's bytes under that same path. The note names an upload absolutely and a dragged file by the path it was dragged with, so mediaOf hands the turn the note's paths as written and lib/uploadPaths goes. A sub-agent direct chat writes the same note, so it stages an upload the same way. The comments and docstrings that described the note as carrying the short path, or uploads/<name> as never meaning two files, now say what holds. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
The composer now names an upload by the file fs.upload wrote, so a sent bubble's chip asks /file for that absolute path. viewer_root made agent home the root of uploads/<name> alone, so with tools.restrict_to_workspace on, a session pinned elsewhere was refused the absolute spelling of its own upload: the fence admitted the session's root only. The same 403 met every upload chip after a reload, since a reloaded bubble names the engine's absolute path. viewer_root now decides by where a path lands: inside agent home's uploads/ it is agent home's, spelled relatively or absolutely, and /file asks it about both spellings. Judging by the first segment let uploads/../notes/x.md, and a link left under uploads/, reach the rest of agent home past the fence; both land outside uploads/ and so stay the session's own question. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
fs.upload keeps a name free under the session's own uploads/ as well, and asks with Path.exists, which raises EACCES rather than answering False. A pinned folder the engine may not read - one macOS keeps from it, or another user's directory - so turned every upload into a bare -32603, where main stored the file. Only the root lookup was guarded. A probe that raises now logs and leaves the name to agent home, as a session whose root cannot be resolved already did. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
The comment above the name check said a session that cannot be resolved falls back to the old rule. An empty session resolves without error, to the resolver's policy default - the launch directory under raven serve, <session root>/_ under raven gateway - so a draft's upload is checked against a folder the conversation need not run in. Say so, and say what covers it: the web UI's composer hands every reader the absolute file. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
workspace_root's docstring said an empty key falls through to the resolver's policy default, "which for the gateway is the launch directory". Under raven gateway the policy is per channel, and an empty key has no channel, so it resolves to <session root>/_; the launch directory is raven serve's default. The text moved here from the console with the rest of the rule, so this branch is where it is corrected. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
Four comments this branch added spoke of "the page" for the whole document and of the workspace pane as "the file panel", two senses ui-web/CONTEXT.md retires: a page is a row of state/pages.ts, and the workspace column is a pane. They now say the document and the workspace pane. Comments only. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
FsUploadResult described path as the "workspace-relative path to hand the agent" and abs_path not at all. turn.send looks for a relative path under the conversation's own working directory before agent home, so a same-named file there answers for uploads/<name>; only abs_path names the upload whatever that directory holds, and it is what the web UI's composer now sends. The contract says so in openrpc.json, its Python mirror and both generated clients. Descriptions only. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
"serves a pinned session nothing else of agent home" claims a set the test does not enumerate: it asks /file for four spellings that start in uploads/ and lead out of it, or name a file beside it. Named for the rule those four exercise instead. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
Pass the draft working directory and session context when uploading files or picking templates. Share collision handling across both deposit paths and validate draft directories before naming attachments. Add regressions for session and draft collisions, protected directories, unreadable session roots, and per-call frontend context. Update the RPC contract and both generated clients. Co-authored-by: Codex <noreply@openai.com>
18486ec to
f3b4be1
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new commit preserves attachment identity for draft-selected workdirs and template deposits by passing per-call context and sharing the existing collision-name policy. I traced both callers through the RPC models, handler validation, shared target selection, generated contracts, and the existing absolute-path send/view flow. The new RPC fields are optional, so older callers retain the prior behavior.
I rechecked the full github/main...HEAD diff and the revision delta against the repository rules and relevant context, callers and history, backward compatibility, test strength, and the existing path-policy architecture. The added cases strengthen coverage rather than weakening it, and all review threads are resolved.
Verification: 310 backend tests and 115 affected UI tests passed; web TypeScript type-check, git diff --check, and the source-language gate passed. The TUI RPC drift command could not start because json-schema-to-typescript is not installed in this worktree; direct comparison shows both generated clients carry the same optional session and workdir fields as the OpenRPC schema.
Summary
Dragging a picture from the transcript into the composer now attaches the file shown by that picture. The composer accepts the same-origin /file path when it matches a drag that began in the document. File uploads retain their image extension through the MIME type when the browser supplies a generic filename. The drag listeners live in the global listener registry, and the composer and viewer share the picture-extension table.
Uploads, picked deck templates, and direct-chat uploads are staged by abs_path. Attachment notes preserve that identity through queued sends, history reloads, and changes of session. Dragged files retain their original paths. Reloaded attachment notes pair each engine path with one entry, preventing overlapping suffixes from duplicating an image.
The viewer and turn.send share session-aware path resolution. Under tools.restrict_to_workspace, attachments are admitted from the session directory and agent home. The viewer serves agent-home uploads by their resolved location, including absolute paths, and rejects traversal or symlinks that escape uploads into other agent-home files.
Both fs.upload and deck.templates.pick use one filename allocator. It checks agent home, the runtime-resolved directory, and a draft's selected directory. This covers pinned drafts, the gateway's tui directory, existing sessions, and a process -w override. Unreadable candidate directories are skipped individually while the remaining directories are checked. Draft directories use the same validation as session.create. The optional session/workdir fields are documented in OpenRPC and mirrored in Python and both generated clients.
Type
Verification
The following checks passed after rebasing onto the current main:
uv run --frozen --all-extras pytest tests/test_rpc_console.py tests/test_rpc_deck_templates.py tests/test_rpc_files.py tests/test_rpc_turn_send.py tests/test_rpc_schema_match.py tests/test_import_cycle_budget.py -q -n 0 -x --tb=short: 774 passed.node node_modules/vitest/vitest.mjs run --no-file-parallelism src/app/install.test.ts src/chrome/Dock.test.tsx src/features/composer/ComposerPage.test.tsx src/features/composer/templates.test.tsx src/features/subagents/SubagentsPage.test.tsx src/lib/attachments.test.ts src/state/session/runtime.inject.test.ts scripts/gates/import-direction.test.mjs: 8 files, 238 passed.node node_modules/typescript/bin/tsc --noEmit -p tsconfig.json: passed.node scripts/gen-rpc-client.mjs --check; in ui-tui:node scripts/gen-rpc-types.mjs --check: both clients match the contract.make check-commits check-large-files check-source-language lint-imports: passed; 10 import contracts kept, 0 broken.uv run --frozen --all-extras ty check raven/rpc/files.py raven/rpc/deck_templates.py raven/rpc/methods/console.py raven/rpc/models.py: passed.node node_modules/eslint/bin/eslint.js src/app/install.ts src/app/install.test.ts: passed.git diff --check origin/main...HEAD: passed.Regression cases cover colliding names in agent home and session directories, repeated deposits, draft-directory validation, unreadable directories, per-call frontend context, and the precedence of a process working-directory override. Pre-fix runs reproduced failures for template collisions, pinned-draft uploads, process-override collisions, and the frontend request context.
The RPC contract documents the added optional parameters. This update changes attachment handling and has no new visible control.
Risk
The same-document drag check and resolved-path boundaries remain enforced. The new directory hints affect filename allocation; deposited bytes remain in agent home. Existing callers may omit the added fields. Clients should send abs_path to preserve upload identity if the session directory or its files later change.
Cross-tab drags follow the existing browser-file fallback, and direct chat retains its upload control without a drop target. Validation above covers the affected Python and frontend suites; native Windows and macOS browser behavior was not rerun for this update.
Rollback is reverting the squash commit. There is no data or configuration migration, and previously recorded absolute attachment paths remain usable.
Related Issues
N/A