Remote-host attachments sharing a sanitized name silently overwrite each other (post-sanitization basename collision)

[postoso]

Version: oracle 0.18.0 (main @ 083bba7e), remote-host path (ORACLE_REMOTE_HOST / browser.remoteHost)

Related to #385, but a distinct mechanism on a different path. #385 is the local browser upload path, where attachmentDataTransfer.ts builds each browser File from path.basename(). This one happens server-side, before the browser is involved at all, and survives any client-side guard on original basenames.

What happens

sanitizeName in src/remote/server.ts:722 maps every character outside [a-zA-Z0-9._-] to _:

function sanitizeName(raw: string): string {
  return raw.replace(/[^a-zA-Z0-9._-]/g, "_");
}

At src/remote/server.ts:229-238 that sanitized name becomes the write path, and each attachment entry then records that path:

const safeName = sanitizeName(attachment.fileName ?? `attachment-${index + 1}`);
const filePath = path.join(attachmentDir, safeName);
await writeFile(filePath, Buffer.from(attachment.contentBase64, "base64"));
attachments.push({ path: filePath, ... });

Two attachments whose names differ only in characters the regex folds therefore resolve to the same filePath. writeFile overwrites the first payload, and both pushed entries point at the surviving file. The fallback loop at :249-258 has the same shape.

Concretely, a b.txt and a_b.txt both sanitize to a_b.txt.

Why it is easy to miss

The names are genuinely distinct on the client, so any collision check on original basenames passes. The divergence only appears after sanitization, on the server, by which point the write has already happened. There is no warning and no count mismatch, so the run reports the full number of files.

Impact

Same class as #385: silent content loss, with the surviving file wearing a plausible name. A caller sees the expected file count and a successful run while one attachment's content was never sent. The recurring-basename case that makes #385 painful in multi-directory packs applies here too, widened to any pair of names differing only in sanitized characters (spaces being the common one).

Suggested fix

Collision-free path allocation at the write site, rather than a client-side check. Two reasons to prefer the server side:

1. A client-side guard on sanitized names requires the client to model the server's sanitization rule, which breaks the moment that regex changes.
2. Version-mismatched clients would never receive a client-side fix, whereas server-side allocation protects every caller.

Suffixing on collision (a_b.txt, a_b-2.txt) or allocating per-attachment subdirectories both work. Erroring is also defensible for consistency with the #385 guard, though the names here are legitimately distinct from the user's point of view, which argues for allocation over rejection.

Not attempted

I did not run a live remote-host session against this; the report is from reading main at 083bba7e. The mechanism is visible in source and the two loops are unambiguous, but a live confirmation would be worth having before anyone builds on it.

[clawsweeper]

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Reviewed August 22, 2026, 7:56 PM ET / 23:56 UTC.

Summary
Current main still maps distinct remote attachment names to the same staged path after sanitization, so the data-loss report remains valid. The open linked PR provides a focused, proof-backed fix and should be reviewed rather than duplicating repair work.

Reproducibility: yes. —source provides a high-confidence path: submit two remote attachments whose filenames sanitize to the same value and inspect the staged files passed to the browser runner. The linked PR also reports a real before/after localhost remote-service run using that exact case.

Root-cause cluster
Relationship: canonical
Canonical: #387
Summary: This issue is the canonical server-side staging collision report; the browser upload collision work is adjacent rather than the same write path.

Members:

• fixed_by_candidate: fix(serve): stage remote attachments under unique paths #389 - The open PR explicitly targets both server-side staging loops described here and supplies corresponding proof and regression coverage.
• adjacent_distinct: Upload-path attachments sharing a basename silently drop one file's content (no warning, summary still reports full file count) #385 - It concerns same-basename loss at the browser upload boundary rather than the remote server's post-sanitization temporary-file allocation.
• adjacent_distinct: fix(browser): reject colliding attachment basenames before upload #388 - It implements the separate browser-side guard for the upload-path issue and does not replace the server-side staging fix.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Next step
An open PR already uses closing syntax for this issue and supplies the focused repair, so a second automated fix PR would duplicate active work.

Review details

Best possible solution:

Land a narrow server-side structural allocation that gives every primary and fallback attachment a distinct staging path while preserving its sanitized display basename.

Do we have a high-confidence way to reproduce the issue?

Yes—source provides a high-confidence path: submit two remote attachments whose filenames sanitize to the same value and inspect the staged files passed to the browser runner. The linked PR also reports a real before/after localhost remote-service run using that exact case.

Is this the best way to solve the issue?

Yes—the linked PR's per-attachment staging directories are a narrow server-side solution that protects all clients and both primary and fallback loops without requiring clients to duplicate the sanitization policy.

AGENTS.md: found and applied where relevant.

Remaining risk / open question:

• Until the linked PR lands, remote-host users can receive a successful file count after one staged payload has been overwritten.

Codex review notes: model internal, reasoning high; reviewed against 083bba7e61f4.

Label changes

Label justifications:

• P1: Remote-host attachment handling can silently discard one caller-provided payload while reporting a successful run.
• impact:data-loss: The overwrite replaces one attachment's staged content with another without an error or count mismatch.

Evidence reviewed

What I checked:

• Current primary staging collision: The primary loop sanitizes each provided filename and uses that result as the only path component; two names such as a b.txt and a_b.txt therefore reach the same writeFile target. (src/remote/server.ts:231, 083bba7e61f4)
• Fallback staging collision: The fallback-submission loop repeats the same sanitize-then-write-to-shared-directory pattern, so it has the same overwrite behavior. (src/remote/server.ts:249, 083bba7e61f4)
• Sanitization rule: The sanitizer folds every character outside the allowlist to _, making post-sanitization basename collisions deterministic. (src/remote/server.ts:722, 083bba7e61f4)
• Existing remote boundary test seam: The remote-server test already drives a real localhost client/server boundary and inspects staged attachment paths and contents inside the injected browser runner, which is a focused regression-test location. (tests/remote/server.test.ts:29, 083bba7e61f4)
• Linked implementation candidate: The open PR fix(serve): stage remote attachments under unique paths #389 explicitly closes this issue, covers both loops, and supplies before/after localhost remote-service evidence showing two preserved sentinels under distinct index-scoped paths. (c0218d6fef0e)
• Feature-history routing: Recent history on the central remote-server path includes Peter Steinberger's current-main change fix(browser): make Chrome cookie sync opt-in (#372); broader history also includes rtl-ai's remote server tab-cleanup work. (src/remote/server.ts:214, 3a185f55918a)

Likely related people:

• Peter Steinberger: Recent current-main history for the remote server path includes the cookie-sync and service behavior update, and he is the dominant contributor in the sampled path history. (role: recent area contributor; confidence: high; commits: 3a185f55918a, 6b17e6db0cae; files: src/remote/server.ts, tests/remote/server.test.ts)
• rtl-ai: Contributed a merged remote-server behavior change in the sampled history, making them a useful secondary routing contact for serve-path behavior. (role: recent adjacent contributor; confidence: medium; commits: 653c621b7ff7; files: src/remote/server.ts)

How this review workflow works

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.