Composer attachments & references: image paste, drag-drop, file mentions (Editor UX 7/7) (SebaBoler/vanguard#362)

[pawelkrystkiewicz]

Part of #362

Automated implementation of #362 by Vanguard.

Spec conformance (partial delivery)

This PR does not cover the full spec. Deferred sections are unchecked below.

Spec files:

• apps/desktop/src/features/task/ChatPane.tsx
• src/api/complete.ts
• apps/desktop/src-tauri/src/sidecar.rs

Tests:

• attach-chip-lifecycle (apps/desktop/src/features/task/ChatPane.test.tsx)
• mention-autocomplete (apps/desktop/src/features/task/ChatPane.test.tsx)
• prompt-inlines-mention (src/api/complete.test.ts)

Acceptance criteria:

• image-paste: Pasted image becomes a removable chip; sent as an image content block; unsupported provider errors inline before send
• drag-drop: Dropped text files inline as fenced blocks (64KB cap); binaries rejected with notice
• file-mentions: @ opens tracked-file autocomplete; selected mention inlines file content into the prompt
• bounded-payload: Total inlined content capped at 256KB with a clear error above

Conformance gap detail

The implementer ended (turn cap or timeout) before signalling the task complete.

Proof of work: PASS

• command: pnpm install --frozen-lockfile && pnpm run typecheck && pnpm test
• exit code: 0
• sha256(output): 5ab4a5750f8066348c54a763a4f3a256065b91bf7ab574e04a5093bc974800d5

output tail

�[2m Test Files �[22m �[1m�[32m105 passed�[39m�[22m�[2m | �[22m�[33m2 skipped�[39m�[90m (107)�[39m
�[2m      Tests �[22m �[1m�[32m1522 passed�[39m�[22m�[2m | �[22m�[33m10 skipped�[39m�[90m (1532)�[39m
�[2m   Start at �[22m 16:50:50
�[2m   Duration �[22m 12.35s�[2m (transform 726ms, setup 0ms, import 2.89s, tests 4.02s, environment 4ms)�[22m

[WARN] The "pnpm" field in package.json is no longer read by pnpm. The following keys were ignored: "pnpm.onlyBuiltDependencies". See https://pnpm.io/settings for the new home of each setting.
[WARN] The "pnpm" field in package.json is no longer read by pnpm. The following keys were ignored: "pnpm.onlyBuiltDependencies". See https://pnpm.io/settings for the new home of each setting.
[WARN] The "pnpm" field in package.json is no longer read by pnpm. The following keys were ignored: "pnpm.onlyBuiltDependencies". See https://pnpm.io/settings for the new home of each setting.
�[90mstderr�[2m | src/pipeline/visual-proof.test.ts�[2m > �[22m�[2mresolveAndRunVisualProof�[2m > �[22m�[2msynthesizes a FAIL result (never undefined) when a configured proof throws
�[22m�[39mvisual proof execution failed (recorded as FAIL): Error: sandbox container crashed
    at Object.exec �[90m(/workspace/�[39msrc/pipeline/visual-proof.test.ts:315:15�[90m)�[39m
    at runVisualProof �[90m(/workspace/�[39msrc/pipeline/visual-proof.ts:73:21�[90m)�[39m
    at resolveAndRunVisualProof �[90m(/workspace/�[39msrc/pipeline/visual-proof.ts:148:18�[90m)�[39m
    at �[90m/workspace/�[39msrc/pipeline/visual-proof.test.ts:318:20

�[90mstderr�[2m | src/agents/claude-stream.test.ts�[2m > �[22m�[2mrunClaudeCli�[2m > �[22m�[2msalvages a cut stream (exit 0 + assistant turns + session, no result) as an incomplete resumable run
�[22m�[39mvanguard: claude stream ended without a result event (exit 0, 1 turns) — treating the stage as incomplete and resumable

�[90mstderr�[2m | src/agents/codex.test.ts�[2m > �[22m�[2mCodexProvider�[2m > �[22m�[2mthrows AgentError when no parseable output is produced
�[22m�[39mcodex produced no parseable output (exit 1)
--- codex stdout ---
fatal crash output
--- codex stderr ---


�[90mstderr�[2m | src/agents/codex.test.ts�[2m > �[22m�[2mCodexProvider�[2m > �[22m�[2mthrows "without a result" (not "no parseable output") when only thread.started is received
�[22m�[39mcodex exited without a result (exit 0)
--- codex stdout ---
{"type":"thread.started","thread_id":"x"}
--- codex stderr ---


�[90mstderr�[2m | src/agents/codex.test.ts�[2m > �[22m�[2mCodexProvider�[2m > �[22m�[2mthrows AgentError when turn.completed is missing
�[22m�[39mcodex exited without a result (exit 0)
--- codex stdout ---
{"type":"thread.started","thread_id":"codex-thread-1"}
{"type":"item.completed","item":{"type":"agent_message","text":"answer"}}
--- codex stderr ---

[pawelkrystkiewicz]

Vanguard Review

Reviewed by claude-code @ 1ecffdf:

Review summary

I reviewed the change adversarially. The backend __complete inlining path (file fences + image content blocks) is clean, correctly validated, and well-tested. But this diff delivers only half the spec, and one mandatory AC had no enforcement anywhere.

Fixed in this pass

• bounded-payload AC (256KB total) was enforced nowhere. MAX_INLINE_TOTAL_BYTES was exported but dead — the frontend enforcer is absent from this diff, and the backend (the actual trust boundary, per repo-files.ts's own "must be re-validated here" reasoning) trusted client-supplied content of any length. Added a boundary check in complete.ts that refuses over-limit sends before hitting the model, plus a test. typecheck + tests green.

Findings I could not fix (out-of-scope subsystems / would be unverifiable)

1. Spec-required frontend + Rust entirely missing. apps/desktop/src/features/task/ChatPane.tsx and apps/desktop/src-tauri/src/sidecar.rs (both required: true) are absent, as are the attach-chip-lifecycle and mention-autocomplete tests (ChatPane.test.tsx). No PR body declares deferral. ACs image-paste, drag-drop, and the file-mentions/chip UX are therefore unverified.

2. repo-files.ts is unreachable dead code as shipped. listRepoFiles/readRepoFile are imported into src/sidecar/deps.ts but never called; SidecarDeps has no listFiles/readFile method and the sidecar dispatch loop (sidecar.ts) has no handler. The @-mention autocomplete/read path has no backend entry point, so the mention-autocomplete test and file-mentions AC cannot pass against this slice.

3. resolveRepoFile — the only path-traversal guard against webview-supplied mention text — has zero tests. Security-critical and untested. (The traversal logic itself looks sound; the middle ${sep}..${sep} check is dead after normalize, but harmless.)

These are coverage gaps requiring the frontend/IPC slice, not local edits — flagging per the mandatory-spec rule rather than half-wiring an unverifiable protocol method.

{"findings":[{"severity":"high","kind":"correctness","title":"Spec-required frontend (ChatPane.tsx) and Rust (sidecar.rs) files and their mandatory tests (attach-chip-lifecycle, mention-autocomplete) are absent with no deferral note","evidence":"spec_manifest marks ChatPane.tsx and sidecar.rs required:true and both ChatPane.test.tsx tests required:true; none appear in the diff or repo grep, so ACs image-paste/drag-drop/file-mentions are unverified"},{"severity":"high","kind":"correctness","title":"repo-files.ts (@-mention list/read) is unreachable: imported into deps.ts but no SidecarDeps method or dispatch handler wires it","evidence":"grep shows listRepoFiles/readRepoFile only referenced at src/sidecar/deps.ts:4 (import) with no call site; SidecarDeps in src/sidecar/sidecar.ts exposes only listTasks/fetchSpec — the mention-autocomplete test and file-mentions AC have no backend entry point"},{"severity":"high","kind":"security","title":"resolveRepoFile path-traversal guard for webview-supplied mention paths has no unit tests","evidence":"src/tasks/repo-files.ts:55 is the sole re-validation of untrusted mention text ('reads with the app's ambient FS rights') but no src/tasks/repo-files.test.ts exists"}]}

[pawelkrystkiewicz]

Vanguard Review

I've reviewed the diff against the existing wire.ts contract. Here are my findings.

Blocking / high-value findings

1. Arbitrary file read via unvalidated image path in __complete (security)

src/api/complete.ts — buildPrompt reads the image path with no containment check:

for (const img of images) {
  const data = readFileSync(img.path).toString('base64');
  ...
}

coerceAttachments accepts any kind:'image' entry with a path that is merely typeof … === 'string' — no validation that it points inside the draft assets dir (or anywhere in particular). So a request such as { attachments: [{ kind: 'image', path: '/etc/passwd' }] } (or ~/.ssh/id_ed25519, .env, etc.) is read verbatim and base64-inlined into the prompt sent to the model — an arbitrary-file-read / exfiltration primitive.

This is inconsistent with the security posture the rest of this same PR establishes: drafts.rs::write_asset re-validates the name ("a smuggled ../ … must never reach join"), and repo-files.ts::resolveRepoFile re-validates the mention path ("arrives from the webview and must be re-validated here"). The image read path has none of that defense-in-depth. The path should be constrained to the draft assets directory (canonicalize and assert it lives under .vanguard/drafts/<id>-assets/) before readFileSync.

2. Image attachments bypass every size cap (bounded-payload AC gap / unbounded growth)

The bounded-payload acceptance criterion claims "Total inlined content capped at 256KB." That accounting only covers content (text), never images:

• Renderer addImageFile (ChatPane.tsx) applies no size check — only addTextFile checks MAX_ATTACHMENT_BYTES.
• Host inlineByteTotal (complete.ts) sums only fileAttachments (kind:'file' with string content); images contribute zero.
• buildPrompt then readFileSyncs each image whole (plus ~33% base64 growth), with no per-image or count cap.

A large pasted image (or many) sails past both the 64KB and 256KB ceilings and past the MAX_INLINE_TOTAL_BYTES guard at the trust boundary. At minimum, cap image bytes and count them toward the total; the AC as written is not actually enforced for images.

Non-blocking findings

3. Duplicate apiListRepoFiles fetch (double IPC per project)

TaskDraftScreen.tsx wires the mention-file load twice: once appended to the existing model-loading effect, and again as a brand-new effect immediately after — both keyed on [project] with identical bodies:

    void apiListRepoFiles(project).then((r) => setMentionFiles(r.files)).catch(() => setMentionFiles([]));
  }, [project]);

  useEffect(() => {
    void apiListRepoFiles(project).then((r) => setMentionFiles(r.files)).catch(() => setMentionFiles([]));
  }, [project]);

This shells git ls-files twice on every project change. Looks like a copy-paste slip — remove one.

4. Binary-detection literal is unverifiable in the diff and untested

ChatPane.tsx::addTextFile:

if (content.includes(' ')) {  // comment says: "A NUL byte is the cheapest reliable 'this is binary' signal"

The character inside the quotes renders as a space, not an obvious \u0000. If it is genuinely a space (not a NUL), every text file containing a space is rejected as "binary," breaking the primary drag-drop-text path. No test exercises acceptance of a small text file through this check (the oversize test rejects on size before reaching it), so a regression here would pass CI. Please confirm this is '\u0000' and add a test that a normal spaced text file is accepted and a NUL-containing one is rejected.

5. readRepoFile reads any repo-relative path, not only tracked files

resolveRepoFile blocks traversal/absolute/\0, but readRepoFile never checks that rel is actually tracked. The doc comment says "Read one tracked file," yet a webview-supplied mention like @.env or @.vanguard/config.json (untracked secrets living in the repo) resolves and is read/inlined. It also follows symlinks (a tracked link -> /etc/passwd would be read). Given the PR treats the webview as untrusted for the path grammar, consider also gating on the git ls-files set (or git check-ignore) before reading.

6. modelSupportsImages is a substring heuristic

/claude|sonnet|opus|haiku/i will (a) misclassify a custom provider model whose name merely contains "claude" (e.g. a proxy that is text-only) as image-capable, and (b) block images for any image-capable built-in that doesn't match those tokens (e.g. meridian from WIRE_PROVIDER_NAMES). Low impact, but the image-support decision would be more robust keyed off provider capabilities than a name regex.

[pawelkrystkiewicz]

Vanguard Review

I have enough grounding. The PR is not applied to the working tree (base is clean), so I reviewed the diff directly and confirmed the relevant base behavior (drafts::delete, the symlink guards the author added elsewhere, the composer send path).

Review — #370

1. (High) Pasted-image asset directories are never deleted — unbounded growth in every repo

drafts::write_asset creates <repo>/.vanguard/drafts/<id>-assets/ and writes image files into it (drafts.rs, new assets_dir/write_asset), but drafts::delete is unchanged:

pub fn delete(repo: &Path, id: &str) -> Result<(), String> {
    ...
    match fs::remove_file(drafts_dir(repo).join(format!("{id}.json"))) { ... } // only <id>.json
}

Deleting a draft removes its JSON but orphans <id>-assets/ and every image in it, forever. Since S10 drafts are explicitly deletable (the whole subsystem exists "to kill the un-removable file"), this reintroduces an unbounded on-disk leak keyed on the very operation meant to clean up. delete should also remove_dir_all the assets dir (with the same symlink guard write_asset uses).

2. (Medium) Orphaned assets on failed/retried sends

In TaskDraftScreen.resolveAttachments, images are persisted to disk before the 256KB total is checked:

const path = await writeDraftAsset(project, id, `${crypto.randomUUID()}.${ext}`, bytes);
out.push({ kind: 'image', path, ... });
...
if (total > MAX_INLINE_TOTAL_BYTES) { throw new Error(...); } // after the write

Every send mints a fresh <uuid>.ext, so a send that throws the 256KB error — or any retry after an earlier failure — leaves the previously written image on disk with no reference and no cleanup path (compounding finding #1). Consider writing assets only after the size check passes, or cleaning up on the failure branch.

3. (Medium, security) readRepoFile follows symlinks and reads any in-repo path, not just tracked files

resolveRepoFile (src/tasks/repo-files.ts) does only a lexical ../absolute check, then readFile(join(repoPath, norm)):

const norm = normalize(rel);
if (norm === '..' || norm.startsWith(`..${sep}`) || norm.includes(`${sep}..${sep}`)) { throw ... }
return join(repoPath, norm);

Two gaps, both inconsistent with the symlink discipline the author applied everywhere else (drafts.rs's assert_leaf_not_symlink, and complete.ts using realpathSync for image containment):

• Symlink escape: git ls-files lists tracked symlinks. A repo containing notes.md -> /etc/passwd (or -> ../../secret) passes the lexical check, and readFile follows the link out of the checkout. @notes.md then inlines an arbitrary host file into the prompt.
• Untracked/secret reads: the mention text is free-form (the user can type @.env even though autocomplete only offers tracked files), and there is no check that the path is actually tracked. @.env inlines repo secrets.

Because the completion can target a custom-provider baseUrl, inlined content is an exfiltration channel. Add a realpathSync-based containment check (canonical path must stay under realpathSync(repoPath)), matching the image path's treatment.

4. (Low, likely rendering artifact — verify) Binary-guard literal

ChatPane.addTextFile shows:

// A NUL byte is the cheapest reliable "this is binary" signal...
if (content.includes(' ')) {

The comment says NUL byte but the diff shows a space. This is probably a \u0000 that rendered as a space in transport — but please confirm the source literal is '\u0000'. If it is literally a space, every text file containing a space is rejected as "binary." Note the added tests don't cover this: no test asserts a normal text file successfully becomes a chip (the oversize test uses 'x'.repeat(...), which has no spaces), so a wrong literal here would pass CI silently. Worth an explicit "small text file attaches" test.

5. (Low) No client-side cap on image size

addImageFile reads any image via readAsDataURL and writeDraftAsset ships it as Array.from(bytes) — a JSON array of one number per byte over IPC — with rejection only later in core at MAX_IMAGE_BYTES (5MB). A large paste balloons renderer memory and IPC payload before the eventual inline error. A client-side size check mirroring MAX_ATTACHMENT_BYTES for text would fail fast.

---

Findings #1 and #3 are the ones I'd block on; #2 compounds #1. #4 is a verification ask, not a confirmed defect.

[pawelkrystkiewicz]

Vanguard Review

Diff-grounded claims confirmed. Here's my review.

Review: PR #370 — Composer attachments & references

1. bounded-payload AC is not enforced for images — aggregate image bytes are unbounded

The stated acceptance criterion is "Total inlined content capped at 256KB." But the total-payload guard only counts text attachments:

• src/api/complete.ts — inlineByteTotal() → fileAttachments() filters to kind === 'file', so images are excluded from the MAX_INLINE_TOTAL_BYTES check.
• apps/desktop/.../TaskDraftScreen.tsx — resolveAttachments() computes total from a.content only; images are only checked individually against MAX_IMAGE_BYTES (5MB).

There is no cap on the number of images or on their aggregate size. A user can paste N images (each ≤5MB) and the whole set is base64-encoded into a single prompt with no ceiling — e.g. 50 images ≈ 250MB into one __complete turn. This directly contradicts the "bounded-payload" AC and is an unbounded-growth / memory risk on the send path (buildPrompt builds the full content[] array in memory, readFileSync(...).toString('base64') per image). Recommend a total-image-bytes and/or image-count cap enforced in runComplete (trust boundary), mirroring the text total.

2. Attachments are lost on a send failure — the inline error is unactionable

ChatPane.send() clears attachments synchronously right after invoking onSend:

if (attachments.length > 0) onSend(text, attachments);
else onSend(text);
setAttachments([]);   // cleared before async resolution runs

onSend → TaskDraftScreen.send kicks off async resolveAttachments. When that throws (256KB ceiling, oversize image), the .catch restores only the text (setComposerText(text)); the image/dropped-file chips are already gone. So the exact error the code raises — "Attached content is too large … Remove a file or mention and try again" — instructs the user to remove a file that no longer exists in the composer. They must re-paste/re-drop everything. The restore contract should also restore the attachment chips (or defer clearing them until resolution succeeds).

3. readRepoFile reads any in-repo path, not just tracked files — contradicts the "tracked file" contract

Every docstring frames this as reading tracked files (src/tasks/repo-files.ts header: "reads one tracked file"; sidecar.rs: "Read one tracked file for mention inlining"). But readRepoFile/resolveRepoFile enforce only lexical containment + symlink canonicalization — there is no git ls-files membership check. Since the mention text arrives free-form from the webview (not constrained to the autocomplete list), a user can type @.env, @.git/config, or any gitignored secret and it will be inlined into the prompt — then shipped to whatever baseUrl a custom provider is configured with (a real egress channel). The autocomplete deliberately surfaces only tracked files, setting the expectation that only those are reachable; the read path doesn't match that boundary. If "tracked only" is the intended security model, readRepoFile should verify membership (e.g. git ls-files --error-unmatch <path>) rather than reading any containable path.

4. Persisted image assets are dead after send but never cleaned until draft delete

resolveAttachments persists each pasted image via writeDraftAsset to <id>-assets/<uuid>.<ext> and forwards the path. __complete reads the file once, base64-inlines it into the prompt, and the transcript stores text only (messages: {role, content: string}) — the file is never referenced again. Nothing cleans it up except drafts::delete. So every image-bearing send permanently accumulates a file under <id>-assets/ for the life of the draft (repeated sends of the same image write fresh UUID files each time). Given the file serves no purpose after the single read, consider deleting it post-send, or documenting why it must persist. As written it's a slow on-disk leak per long-lived draft.

5. Minor

• Truncation silently dropped: readRepoFile returns truncated, and the wire doc says "the caller warns and inlines the head," but resolveAttachments ignores the flag — an over-cap mention is silently inlined as a partial file with no notice to the user.
• Image-model support is client-only: modelSupportsImages (a name-regex heuristic, /claude|sonnet|opus|haiku/i) gates send in ChatPane, but __complete performs no equivalent check. A custom provider whose model name happens to contain "opus" but is text-only will pass the client guard and fail at the API instead of inline. Heuristic is acceptable for UX, but note there's no server-side backstop.

The Rust path-safety work (safe_asset_name, symlink guards in write_asset/delete, set_asset_root overwriting renderer input) and the realpath-containment for both image and repo-file reads are solid and well-tested — no issues there.

Findings 1–4 are actionable and worth addressing before merge; #1 and #3 are the ones that undercut a stated AC / security-model claim.

[pawelkrystkiewicz]

Vanguard Review

I've reviewed the diff. The path-traversal, symlink, and asset-root containment work is genuinely well done (realpath containment on both the image root and mention reads, tracked-file membership gating, symlink guards on write/delete). My findings are correctness/perf/UX gaps, not holes in that hardening.

Findings

1. Aggregate image cap (MAX_IMAGE_TOTAL_BYTES) is enforced only server-side → orphaned assets + cleared chips on multi-image sends (medium)

src/api/complete.ts buildPrompt enforces MAX_IMAGE_TOTAL_BYTES (15MB aggregate). But the client never does — TaskDraftScreen.tsx imports only MAX_INLINE_TOTAL_BYTES and MAX_IMAGE_BYTES, and resolveAttachments checks images against the per-image cap only:

if (bytes.length > MAX_IMAGE_BYTES) { throw ... }   // per-image only

Consequence: paste 4×4MB images (each < 5MB, so each passes), resolveAttachments reaches the end and persists all four to disk via writeDraftAsset, resolves successfully, and ChatPane.send clears the chips (accept(true)). Only then does __complete reject with "images exceed the 15MB total attachment limit" — surfaced as a completion error, not a resolution failure. The user now has: chips gone (can't retry without re-pasting), four orphaned files under <id>-assets/, and an error that says nothing about which images to drop. This is exactly the failure mode the MAX_IMAGE_BYTES client mirror comment says it wants to avoid ("fail fast instead of shipping megabytes over IPC only to be refused in __complete") — but the aggregate case was missed. Add the aggregate check to resolveAttachments before persisting, so the throw happens pre-disk-write and keeps the chips.

2. Every @token in prose triggers a server-side git subprocess spawn (medium, perf)

resolveAttachments scans the full sent text and calls apiReadRepoFile for every @token, sequentially:

for (const m of text.matchAll(/(?:^|\s)@([\w./-]+)/g)) {
  ...
  await apiReadRepoFile(project, rel);   // sequential
}

Each call reaches src/tasks/repo-files.ts readRepoFile, which shells git ls-files --error-unmatch (and a second git for the offer list is separate). The regex matches any @word after whitespace, so ordinary prose — "cc @john", "@me on this", email handles — spawns a git process per token, awaited serially. A message with N @-tokens = N sequential process spawns added to send latency, and the untracked ones all take the --error-unmatch nonzero-exit path (thrown + caught + skipped). This is a real latency/DoS-ish surface for casual @ usage. Consider resolving mentions concurrently (Promise.all) and/or intersecting the matched tokens against the already-loaded mentionFiles set before shelling out, so only plausible tracked paths hit git.

3. modelSupportsImages loose substring match can false-positive (low)

export function modelSupportsImages(model: string): boolean {
  return /claude|sonnet|opus|haiku/i.test(model);
}

A custom-provider model whose name merely contains one of these tokens (e.g. a proxy named gpt-opus-router, my-claude-gateway) is treated as image-capable, so a pasted image is sent to a provider that can't read it — the exact "unsupported provider errors inline before send" acceptance criterion this guards. Anchoring to known model families or matching a stricter prefix would tighten it. Low severity since it only misfires on adversarially/oddly named custom models.

---

None of these are security-blocking; #1 and #2 are the ones worth addressing before merge. If the team considers them acceptable given the per-draft self-cleaning asset lifecycle and typical @ usage, that's a reasonable call — but they should be a conscious one.

[pawelkrystkiewicz]

Vanguard Review

Based on my review of the diff against the codebase, here are the actionable findings.

Findings

1. (Correctness, blocking) Attachment-failure persistNow is unguarded against a mid-flight draft switch — can corrupt a different draft

In the new resolveAttachments(...) .catch in TaskDraftScreen.tsx:

.catch((err) => {
  if (id === activeIdRef.current) {          // guarded
    dispatch({ type: 'fail', ... });
    setComposerText(text);
  }
  persistNow(id, {                            // NOT guarded
    chat: draftRef.current.chat.at(-1)?.role === 'user' ? draftRef.current.chat.slice(0, -1) : draftRef.current.chat,
    composerText: text,
  });
  ...
})

persistNow mutates and writes draftRef.current (TaskDraftScreen.tsx:477-481). resolveAttachments awaits IPC (apiReadRepoFile, writeDraftAsset), and a draft switch in that window repoints draftRef.current to a different draft (TaskDraftScreen.tsx:232, draftRef.current = { ...entry.data }). The .catch then:

• writes the now-active draft's chat (minus a trailing user turn) and the failed send's composerText into file id, and
• clobbers the active draft's in-memory ref.

The success path deliberately avoids this: for the non-active case it uses writer.update(id, d => …) operating on the file (TaskDraftScreen.tsx:557), and all draftRef mutations are gated on id === activeIdRef.current. The new error path breaks that convention. Fix: gate the persistNow on id === activeIdRef.current, or rewrite it as writer.update(id, d => …) like the late-reply branch.

2. (Unbounded growth) Per-send image assets leak for any draft that is filed rather than deleted

resolveAttachments persists a fresh crypto.randomUUID().<ext> asset on every image send, and the only reclamation is drafts::delete removing <id>-assets/ (drafts.rs). But the common terminal action is createTask, which sets archived: true and never deletes the draft (TaskDraftScreen.tsx:667). Every pasted image on a filed draft therefore stays on disk forever, and each Stop-then-retry writes an additional orphan. The code comment claims growth is "bounded per draft and self-cleans on delete" — that's only true for the delete path, not the archive path, which is the normal end state. Consider content-hash dedupe, or reclaiming <id>-assets/ on archive.

3. (Performance) Image bytes are sent over IPC as a JSON integer array

writeDraftAsset marshals bytes as bytes: Array.from(bytes) (ipc.ts). A 5 MB image (the MAX_IMAGE_BYTES ceiling) serializes to a ~15–20 MB JSON array that both sides must stringify/parse on every paste-send. Base64 (or a byte-oriented Tauri channel) would be an order of magnitude cheaper.

4. (Minor) truncated is silently dropped on mention inlining

resolveAttachments does const { content } = await apiReadRepoFile(...) and discards truncated. A tracked file over MAX_ATTACHMENT_BYTES is inlined head-only with no notice, contradicting the wire doc ("the caller warns and inlines the head"). Surface the truncation to the user.

5. (Very minor) Fenced inlining is not fence-safe

inlineAttachments wraps file content in a bare ``` fence (complete.ts). Content containing a ``` line breaks out of the block. Not a security issue (content is user-owned), but it can garble the prompt for legitimate markdown/code files; a longer/backtick-counted fence would be more robust.

---

Findings 1–3 are worth addressing before merge; 4–5 are polish. The security posture of the new read paths (tracked-only membership check, realpath containment for both mentions and image assetRoot, sidecar-stamped trusted root overriding renderer input) is sound and well-tested.

[pawelkrystkiewicz]

Vanguard Review

I reviewed the diff focusing on the security-critical paths (image containment, tracked-file membership, path traversal, symlink handling) and the composer/attachment lifecycle. The security boundaries are handled well and well-tested — trusted assetRoot stamped Rust-side over renderer input, realpath containment on both image reads and mention reads, git ls-files --error-unmatch membership gating before any read, lexical traversal rejection, and symlink guards on the Rust asset writes. No arbitrary-file-read or traversal escape is reachable that I can find.

A few non-blocking, actionable items:

Findings (non-blocking)

1. truncated is silently discarded when inlining a mention — contradicts the documented contract

In TaskDraftScreen.tsx resolveAttachments:

const { content } = await apiReadRepoFile(project, rel);
out.push({ kind: 'file', path: rel, content });

The truncated field is destructured away. But src/wire.ts / repo-files.ts document ReadRepoFileResult.truncated as "the caller warns and inlines the head." The caller does not warn — a tracked file over MAX_ATTACHMENT_BYTES (64KB) is inlined head-only with no user-visible notice, so the model silently sees a partial file. Either surface a notice (a chip/warning) or update the doc comment to match reality.

2. Math.max(0, ...runs) spread over untrusted content in inlineAttachments (src/api/complete.ts)

const longest = Math.max(0, ...[...f.content.matchAll(/`+/g)].map((m) => m[0].length));

complete.ts is the trust boundary and only enforces the aggregate inline cap (MAX_INLINE_TOTAL_BYTES, 256KB) — not a per-file cap. A single file attachment near 256KB densely packed with backtick runs yields ~100K+ matches, and Math.max(...spread) on an array that large can hit V8's argument-count limit and throw. It's inside the try in runComplete, so it degrades to an error response rather than a crash — but a legitimately large fenced-code file would then fail to inline instead of completing. Prefer a reduce over the spread to make this input-size-independent.

3. Chips/images are cleared on resolution success, before apiComplete runs — a failed completion loses them for retry

In send (TaskDraftScreen.tsx):

.then(async (resolved) => {
  if (!cancelledCalls.current.has(callId)) accept(true); // clears chips
  const cfg = await readAppConfig(project);
  ...
  const res = await apiComplete(...);

accept(true) (which clears the composer chips in ChatPane) fires as soon as attachment resolution succeeds, before the completion is sent. If apiComplete then fails and the failure path restores composerText for retry, the pasted image / dropped-file chips are already gone, so the retry silently sends text-only. Intentional per the r3 comment (chips clear on resolution, not send), but worth confirming the retry-loses-attachment case is acceptable.

---

None of these block. The traversal/containment/symlink/membership security model and the draft-switch race handling (the r5 tests) are sound.

No blocking findings.

[pawelkrystkiewicz]

Vanguard Review

I've reviewed the diff against the base files (the PR is not applied to the working tree, so src/api/complete.ts and drafts.rs on disk are the pre-PR baseline — I reviewed the diff as the source of truth, cross-checked against the existing helpers it reuses).

Findings

1. delete() can now leave a draft permanently undeletable (regression) — apps/desktop/src-tauri/src/drafts.rs

delete gates JSON removal on asset cleanup succeeding:

pub fn delete(repo: &Path, id: &str) -> Result<(), String> {
    safe_name(id)?;
    assert_not_symlink(repo)?;
    clear_assets(repo, id)?;          // <-- hard gate, `?` returns early on any error
    match fs::remove_file(drafts_dir(repo).join(format!("{id}.json"))) { ... }
}

clear_assets returns Err when the assets dir is a symlink (assert_leaf_not_symlink) or when remove_dir_all hits a real IO error (permission denied, a locked/undeletable child, Windows sharing violation, a hostile clone committing <id>-assets as a symlink). In all of those cases delete now returns Err before removing <id>.json, so the draft the user explicitly asked to delete survives.

This directly undercuts the invariant the whole safe_name leniency exists to protect ("anything list could have returned must be deletable"; AC6 "every draft visible and deletable"). Previously delete always removed the JSON.

Fix: make asset cleanup best-effort within delete (e.g. let _ = clear_assets(repo, id);) or remove the JSON first, then attempt asset cleanup. There is a test that delete removes assets on the happy path, but none asserting the draft is still deletable when asset cleanup fails.

2. Truncated mention content is silently inlined — apps/desktop/src/features/task/TaskDraftScreen.tsx

readRepoFile caps a file at MAX_ATTACHMENT_BYTES and returns truncated: true, and the wire doc promises "the caller warns and inlines the head." But resolveAttachments discards the flag:

const { content } = await apiReadRepoFile(project, rel);
out.push({ kind: 'file', path: rel, content });

A >64KB tracked file is inlined as just its head with no notice to the user or the model, which can silently mislead the completion (the model treats a truncated file as complete). At minimum surface truncated in the UI, or annotate the fenced block ("… truncated at 64KB").

3. Chips clear on resolution success, before completion success — ChatPane.tsx / TaskDraftScreen.tsx

accept(true) fires as soon as resolveAttachments resolves; the chips are then cleared while readAppConfig + apiComplete still run. If the completion itself fails (network hang, provider error), the chips are already gone and the persisted image assets are orphaned on disk until the draft is deleted — the user must re-paste to retry. The stated intent (review r3) is "keep chips for retry," which only holds for resolution failures, not completion failures. Consider deferring the chip-clear until the turn actually lands, or documenting this as intentional.

4. modelSupportsImages name heuristic is bidirectionally wrong — ChatPane.tsx

return /claude|sonnet|opus|haiku/i.test(model);

• False negative: a genuinely multimodal custom provider (e.g. an Anthropic-compatible proxy model not named claude*, or a gpt-4o-class model) is treated as text-only, so the user is blocked from ever attaching an image with an inline "can't read images" error.
• False positive: a text-only custom model whose name happens to contain opus/claude is allowed, and the image is dropped/erred only downstream.

Since custom providers are user-configured with arbitrary baseUrl/model names, this heuristic will misfire. Not blocking, but worth a comment acknowledging the limitation or a capability flag instead of a name regex.

Notes (verified OK)

• Image path containment (buildPrompt) is sound: assetRoot is stamped Rust-side over any renderer value (set_asset_root), realpathSync resolves symlinks before the startsWith(root + sep) check, so a link out of .vanguard/drafts is refused; images with no trusted root are refused outright.
• readRepoFile correctly orders membership (git ls-files --error-unmatch) → realpath containment, closing both the untracked-secret (@.env) and tracked-symlink-out-of-repo exfiltration vectors; well covered by repo-files.test.ts.
• Fence-safety in inlineAttachments (fence longer than the longest backtick run) prevents prompt-injection breakout; tested.
• writable_id vs safe_name split for assets is consistent — assets can only be written for strict ids, so no lenient-only draft can accumulate an uncleanable asset dir.
• Server-side re-check of MAX_INLINE_TOTAL_BYTES at the __complete trust boundary is present and tested.

[pawelkrystkiewicz]

Vanguard Review

I've reviewed the diff statically (the PR changes aren't applied to the working tree, so I assessed the diff as text against the surrounding code I could read: src/wire.ts and the pre-diff src/api/complete.ts).

Assessment

The security-critical paths are well-constructed and clearly hardened across multiple prior review rounds. I verified each of the guard claims against the diff:

Image containment (src/api/complete.ts buildPrompt) — Correct. realpathSync canonicalizes both the trusted root and each image path, and the check real !== root && !real.startsWith(root + sep) is sound (the + sep closes the sibling-prefix bypass, e.g. .../drafts-evil). A missing assetRoot hard-fails before any read. The root is stamped Rust-side (set_asset_root) over renderer input, mirroring set_base_url, so the renderer can't choose what it's checked against. Synchronous throws from buildPrompt land inside runComplete's try, so they surface as { error }, not a crash.

Mention read path (src/tasks/repo-files.ts) — Correct and defense-in-depth: lexical traversal/absolute/NUL rejection → git ls-files --error-unmatch membership (blocks @.env/gitignored secrets) → realpath containment (blocks a tracked symlink pointing out of the checkout). The exfiltration concern (inlining a secret into a prompt bound for a custom-provider baseUrl) is closed at three layers.

Rust asset lifecycle (drafts.rs) — safe_asset_name blocks separators/../dotfiles; symlink guards on both dir and leaf. delete correctly removes the JSON first and does asset cleanup best-effort after, preserving the AC6 "listable ⇒ deletable" invariant even when the assets dir is a hostile symlink.

Bounded payload — Enforced on both sides of the trust boundary: client (resolveAttachments) and server (inlineByteTotal + per/aggregate image caps in buildPrompt). Fence-safety in inlineAttachments prevents fenced-block breakout.

Non-blocking observations (author's discretion, not required for merge)

1. Mentions beyond REPO_FILES_CAP (2000) are silently un-inlined. resolveAttachments only reads paths present in mentionFiles (tracked.has(rel)), and mentionFiles is the capped/sorted slice. On a monorepo, a user can @-mention a genuinely tracked file that sits past the cap; it won't autocomplete and won't inline, with no notice — the server's membership re-validation is never reached because the client never issues the read. apiListRepoFiles returns capped, but TaskDraftScreen discards it. Consider surfacing the capped state, or attempting the read for any @token matching the tracked-path grammar and letting the server be the authority.

2. assetRoot is the whole .vanguard/drafts tree, not the specific <id>-assets/. Containment permits an image path pointing at any file under drafts/ (other drafts' JSON, other drafts' assets). This is not a meaningful escalation (all are the user's own local drafts, and a hostile renderer already controls the whole request), but a per-send <id>-assets root would be tighter if the id were threadable into set_asset_root.

3. Mention reads are sequential (await apiReadRepoFile in a for loop). Fine for the expected handful of deduped mentions; only worth noting if mention counts ever grow.

None of these block the change.

No blocking findings.