diff --git a/AGENTS.md b/AGENTS.md index 8ff776d..004eb2c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,6 +5,11 @@ replacing the Claude app for daily use. Rust/Axum backend on the desktop, Kotlin/Compose Android app, WireGuard + pinned self-signed TLS + bearer token between them. +**`TRANSCRIPT_RENDERING.md` is the record of the transcript work** -- +measurements, techniques, the harness, and the ordered list of what is +next. Read it before touching anything under `Markdown*.kt`, +`Transcript*.kt` or `SessionScreen.kt`'s list. + **`PLAN.md` is the design source of truth.** Read it before building or changing anything structural. It records every decision with its date, its rationale, and the alternatives that were rejected and why — keep that habit @@ -75,9 +80,12 @@ repo is in PLAN.md's "Backend layout" section. and `isImageRef` on the phone tell the two apart; keep those lists level. The phone attaches from the photo picker, the file chooser and Android's share sheet (`Share.kt`; the manifest's SEND filter), all - through one `attach` path in `SessionScreen`. Files exist only on the - server's machine -- see PLAN.md's "Transport" for what that means for - remote sessions. + through one `attach` path in `SessionScreen`, streamed both from the + phone and onto disk. A file for a session on another machine is also + copied there during the upload (setup's `attachmentsDir`, else the + session's cwd, else home) and the driver names that path, read from + the `.remote` marker beside the file -- PLAN.md's "Transport" has + the reasoning. - `server/` — Rust backend (`ai-server`). `main.rs` bootstraps (TLS, the auth layer, token/QR enrollment, wg0 binding), `routes.rs` has the HTTP table in its module doc comment, `auth.rs` the bearer-token middleware, diff --git a/PLAN.md b/PLAN.md index 2ded77a..ec55550 100644 --- a/PLAN.md +++ b/PLAN.md @@ -720,11 +720,17 @@ host) and **hosts**. The manager runs at most one llama-server per remote filesystem, so there is no `scp` step to get wrong. - **Any other file is told to the session by path** (2026-09-03: a trace, a log, a zip -- things a model cannot be shown and the CLI can read). - The upload stays under the session's `attachments/` and the message - ends with `Attached file: /abs/path`. That directory exists only on the - machine running this server, so a file attached to a remote (ssh) - session names a path that is not there. Shipping it is not built; the - one host in use runs its sessions locally. Images are unaffected. + The upload is streamed to disk under the session's `attachments/` on + this machine -- the transcript references it there and the phone can + fetch it -- and the message ends with `Attached file: /abs/path`. For a + session on another machine the upload also copies the file there, in + the same request, over one `ssh` invocation (`cat` from stdin, then + `pwd -P` so the answer is the absolute path the CLI is told). It lands + in the setup's `attachmentsDir` if set, else the session's cwd, else + the login home; the resolved remote path is recorded beside the file + (`.remote`) and is what the driver names. A copy that fails fails + the upload, so no message ever names a file that is not there. Images + are unaffected: they ride the message as base64. ### Usage limits (Claude) @@ -1103,10 +1109,9 @@ window just fills. command is the identical one wrapped in `ssh -T`, with every argument shell-quoted). Attachment shipping turned out to be unnecessary for images — they ride the stdio JSONL as base64 in both directions, so - nothing needs `scp` — and became necessary again on 2026-09-03 for - files, which are attached by path (see "Transport" above). Still - outstanding: file shipping for remote sessions, and remote - llama-server with its port forward, which comes with phase 4. + nothing needs `scp` — and was built on 2026-09-03 for files, which + are attached by path (see "Transport" above). Still outstanding: + remote llama-server with its port forward, which comes with phase 4. Two things learned doing it: a remote session inherits ssh's non-login PATH, which is narrower than an interactive shell's (point `command` at an absolute path if a CLI isn't found), and the remote command is run diff --git a/TRANSCRIPT_RENDERING.md b/TRANSCRIPT_RENDERING.md new file mode 100644 index 0000000..de60f3c --- /dev/null +++ b/TRANSCRIPT_RENDERING.md @@ -0,0 +1,189 @@ +# Transcript rendering: what was learned, and what is next + +Written 2026-09-03 at the end of a week of work on the session screen's +transcript, so the next session can start from here rather than from a +compacted context. `AGENTS.md` holds the one-paragraph conventions; this is +the longer record: the measurements that drove each decision, the +techniques that worked, the ones that did not, and the order to do the rest +in. `PLAN.md` remains the design source of truth; nothing here contradicts +it. + +## The goal, and where it stands + +A reply of any length must scroll at the phone's 120Hz without a bump, and +must keep doing so while the reply is still streaming in. Measured on the +Pixel 9 Pro XL by Bryan, the transcript went from visible stalls at long +replies and at lists of links to "I have to actually try to feel any +bumps". The remaining work is finish and extensibility rather than +performance. + +## The architecture, as built + +Everything below lives under `app/androidApp/src/main/kotlin/com/example/aiapp/`. + +**Rows become units, and units are bounded.** `TranscriptUnits.kt` turns a +transcript row into the things the lazy list actually holds. An assistant +reply is not one unit: it is one unit per piece of its markdown, so the +list composes and draws a paragraph, a fence, a table or one bullet at a +time. The reason is the draw phase: a row's display list holds every glyph +of it and is re-recorded whenever drawing is invalidated, and the lazy list +composes an item whole in the frame it scrolls into. The tallest single +row still being drawn before this was 36,982px, twenty-five screens in one +message. Long user messages are sliced the same way (`UserChunk`), through +the shared `cardPiece` modifier that draws one card in lazy-list pieces. + +**One parse per message, addressed by piece.** `MarkdownPieces.kt`'s +`Piece(block, item)` is an address into the message's single parse tree, +not a substring: `block` indexes the root's children and `item` one +`LIST_ITEM` of a top-level list. Cutting was originally done by +re-parsing substrings, which cost a parse per piece and broke reference +links defined at the foot of a message. `ParsedReplies` caches the parse +and the piece list per text (`of`, `piecesOf`), warmed off the composing +thread by `TranscriptItems.warm`. The parser is still intellij-markdown via +the mikepenz renderer; the renderer's `Markdown()` composable is kept only +as the provider of its `Local*` environment (`MarkdownRoot` in +`Markdown.kt`), and `MarkdownElement` dispatches a whole block through our +component table. + +**Lists are drawn an item at a time, by us.** The renderer has no element +for a single list item, so `MarkdownListItem` draws one: marker, then the +item's children, nested lists recursing through `MarkdownList`. The +marker is drawn in one place on purpose; styled bullets per depth go +there. + +**Links are spans, not nodes.** `MarkdownLinks.kt`. Compose turns every +`LinkAnnotation` into a layout node (clipped, focusable, hoverable, +clickable, outline recomputed from the text layout). A paragraph of eight +links was nine nodes, and measured against the same paragraphs with each +link replaced by plain words it cost 26.3ms worst measure against 5.2ms, +1.7x the place time. That was the bump at a reply's list of sources. +`LinkedText` builds the annotated string with the renderer's own inline +builder but answers links itself: colour, underline, a string annotation +carrying the URL, and one tap detector for the whole text that asks the +layout which glyph is under the finger. Hit-testing must check the glyph +on either side of the returned caret, because `getOffsetForPosition` +returns the nearest boundary; taps on the right half of a glyph otherwise +open nothing. Headings need the `ATX_CONTENT`/`SETEXT_CONTENT` child, since +the inline builder draws nothing for a node type it does not know (a week +of blank headings). Tables go through `LinkedTable`/`LinkedTableRow` so +cells get the same treatment. + +**Text draws on the platform directly.** A paragraph without an image +skips the renderer's `MarkdownText`, which charges every paragraph for the +possibility of inline images (placement callback, derived inline-content +map, semantics group, size animation). Paragraphs that contain an image +still take the renderer's path. + +**Tables spread or scroll without subcomposition.** The renderer used +`BoxWithConstraints` to decide; `LinkedTable` uses +`fillMaxWidth().horizontalScroll().layout { }` -- `horizontalScroll` +passes `minWidth` through and lifts `maxWidth` to infinity, so the inner +layout reads `minWidth` as the room available and takes +`max(minWidth, columns * cellWidth)`. + +**A streaming reply is reparsed one block at a time.** `LiveParse` in +`Markdown.kt` freezes every finished top-level block with its parse and +reparses only the tail block per delta. Markdown's block rules make later +text unable to alter an earlier block, with the single exception of a +late reference definition, which is accepted. Measured on a 58-word stream +of list, fence, table and quote: 47 tail reparses at 1.7ms mean. A +single-list stream still reparses per delta because the whole list is one +tail block. + +**Expansion anchors the edge that was tapped, and the list never moves +under the reader** except when pinned to the bottom with new content +arriving. Those two rules are in `ScrollAnchor.kt` and `TranscriptList.kt` +and are the reason several tempting simplifications were rejected. + +## Techniques and harness + +- **`app/ui-sandbox.sh`** starts a second `ai-server` against a sandbox + home with the echo driver, so nothing touches real sessions. + `spawn [title]` makes an echo session and prints its id; `send SID text` + or `send SID @file` sends into it; `api /path [curl args]` is an + authenticated request. Restarting it regenerates the config but keeps + enrolled tokens. +- **The echo driver is the test rig** (`server/src/session/echo.rs`, the + list at the top of the file). `/stream N`, `/mixed N`, `/table N`, + `/tools N gap`, `/ask`, `/peer`, `/compact`, `/slow`, `/bash command` + each produce a shape the real CLI produces only when it feels like it. + Build what a UI test needs into it rather than spending model turns. +- **`app/transcript-bench.sh`** is the standard measurement: restart, open + the first session, scroll, print the render report. The report is what + the "Copy render timings" button copies and also logs + (`adb logcat -d -s ai-app:I`), and it includes the last crash's stack + (`CrashLog.kt`), which is how a crash on the phone reaches a session + here. +- **`DebugStats`/`FrameStats`** time our own phases (`record: one block`, + `measure: the app root`) and count events (`markdown reparsed while + streaming`, `markdown cut into pieces`). Add a counter before guessing. +- **`app/trace-draw.sh`** names what a scrolling frame spends inside the + framework, via `atrace` text output, no trace processor needed. It is + how the link-node cost was attributed. +- **`app/debug-transcript.sh`** loads a real Claude Code conversation onto + the emulator; two faults were invisible on fixtures and obvious on it. + Real transcripts are private: fixtures stay in `/tmp`, never in the repo. +- **`ui-trace`** reads the screen as text. Bounds print as + `x1,y1..x2,y2`; unanchored `-m` patterns match labels, anchored ones do + not. A row taller than the viewport reports clipped bounds, so compare + screenshots for that case. +- **Emulator frame times are not app measurements.** Software rendering + puts the stock Settings app at 60ms of UI-thread traversal per frame. + Costs of operations in milliseconds rank correctly; smoothness itself is + judged on the phone. +- **System Tracing on the phone does not work on GrapheneOS.** Its + Categories list is empty because the tracing daemon builds it by running + `atrace --list_categories`, which returns nothing there, and a recorded + trace contains zero ftrace events: no app sections, no frames, no + scheduling. Callstack sampling records, but the app's profiler config + unwinds one process shard in four. GrapheneOS issues 2206 and 6094 are + open on exactly this. Until they close, phone numbers come from the + render report and from Bryan noticing. +- **Compose `DropdownMenu` in an edge-to-edge activity** needs + `PopupProperties(clippingEnabled = false)` or it opens a status bar's + height away from its anchor (`~/.claude/TOOLCHAIN.md`). +- **highlights 1.1.0's shell lexer** returns a span whose end precedes its + start for `x '*/a/*'`; `ToolInput.kt` drops such spans. The fix belongs + upstream and has not been filed. + +## Rejected, and why + +- **Writing our own markdown renderer.** Rejected in favour of keeping + the intellij-markdown parser and the library's inline builder while + owning block dispatch and the leaf composables. The parser is the hard + part and is not the slow part; everything that was slow lived in the + composables, which are now ours. +- **Re-parsing substrings per piece.** Cost a parse per piece and broke + foot-of-message reference links. Replaced by addressed pieces of one + parse. +- **Animated or timing-dependent corrections.** Anything the reader could + catch at 120Hz is a bug; corrections must be structurally impossible to + see. + +## What is next, in order + +1. **Styled markers per depth.** `MarkdownListItem`'s `Marker` is the one + place bullets and numbers are drawn; give it a glyph per depth and the + list's own colour. Bryan asked whether the architecture allows it; it + does, and it is a small change. +2. **Syntax highlighting inside fences.** The highlights lexer used for + tool commands can colour code blocks too; route the fence composable + through the same table as `ToolInput.kt`, keep the inverted-range + guard, and measure a long fence before and after, since a highlighted + fence is one `Text` with many spans. +3. **Drop `MarkdownRoot`'s dependence on the library's `Markdown()`.** + It exists only to provide `LocalMarkdown*`. Providing those locals + directly removes the last library composable from the hot path and + frees the way for a different parser later. +4. **Paragraphs with images** still take the renderer's `MarkdownText`. + Draw the image as its own piece below the paragraph instead, then the + text leaf covers every paragraph. +5. **Per-item units for a streaming list.** A single-list stream reparses + the whole list per delta; freezing finished items would make a + forty-item list stream like forty paragraphs. +6. **Regression runs.** Run `transcript-bench.sh` before and after any + change to the files above and paste the report into the commit. The + numbers to watch are the worst `record: one block` and the draw phase + share in the accounting line. +7. **Tooling debt.** AGP 9.4.0 is available (lint warns). File the + highlights range bug upstream with the one-line repro. diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt index c9f9427..051cc71 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt @@ -32,8 +32,12 @@ fun requestFromServer( path: String, method: String = "GET", jsonBody: String? = null, - /** Raw request body as content-type to bytes -- the upload path. */ - binaryBody: Pair? = null, + /** + * A request body written as it is produced -- the upload path. Content type, and a writer + * handed the connection's stream. Sent chunked, since what a writer will produce is not known + * up front and the point is that a file never sits whole in memory on this side. + */ + streamBody: Pair Unit>? = null, readTimeoutMs: Int = READ_TIMEOUT_MS, readBody: (HttpURLConnection) -> T, ): T { @@ -48,10 +52,11 @@ fun requestFromServer( connection.doOutput = true connection.setRequestProperty("Content-Type", "application/json") connection.outputStream.use { it.write(jsonBody.encodeToByteArray()) } - } else if (binaryBody != null) { + } else if (streamBody != null) { connection.doOutput = true - connection.setRequestProperty("Content-Type", binaryBody.first) - connection.outputStream.use { it.write(binaryBody.second) } + connection.setChunkedStreamingMode(0) + connection.setRequestProperty("Content-Type", streamBody.first) + connection.outputStream.use(streamBody.second) } if (connection.responseCode !in 200..299) { val detail = connection.errorStream?.bufferedReader()?.readText()?.trim() @@ -381,12 +386,17 @@ data class SshDetails( val address: String, val port: Int? = null, val identityFile: String? = null, + /** + * Where files attached from here land on that machine; null for the session's own directory. + */ + val attachmentsDir: String? = null, ) private fun SshDetails.toJson() = JSONObject().put("address", address).apply { if (port != null) put("port", port) if (!identityFile.isNullOrBlank()) put("identityFile", identityFile) + if (!attachmentsDir.isNullOrBlank()) put("attachmentsDir", attachmentsDir) } /** What a machine turns out to have, without saving anything. */ @@ -540,16 +550,16 @@ fun setSessionCwd(settings: ServerSettings, sessionId: String, cwd: String) { } /** - * Uploads one attachment; the returned id goes into [sendMessage]. [name] is what the server keeps - * a file under and tells the session; for an image it is ignored, since the model is shown the - * picture rather than told its name. + * Uploads one attachment, streamed by [write]; the returned id goes into [sendMessage]. [name] is + * what the server keeps a file under and tells the session; for an image it is ignored, since the + * model is shown the picture rather than told its name. */ fun uploadAttachment( settings: ServerSettings, sessionId: String, - bytes: ByteArray, mime: String, name: String, + write: (java.io.OutputStream) -> Unit, ): String { val boundary = "----aiapp-${System.currentTimeMillis()}" // The header is a line: a quote or a line break in the name would end it early. @@ -564,8 +574,16 @@ fun uploadAttachment( settings, "/sessions/$sessionId/attachments", method = "POST", - binaryBody = "multipart/form-data; boundary=$boundary" to (head + bytes + tail), - readTimeoutMs = 60000, + streamBody = + "multipart/form-data; boundary=$boundary" to + { out -> + out.write(head) + write(out) + out.write(tail) + }, + // Long: a trace is hundreds of megabytes, and the server copies it on to a remote + // machine before answering. + readTimeoutMs = 600000, ) { connection -> connection.jsonObject().getString("id") } diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Attachments.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Attachments.kt index b53eda2..6c659c7 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Attachments.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Attachments.kt @@ -33,7 +33,7 @@ suspend fun uploadPickedImage( maxEdge: Int?, ): String { val (bytes, mime) = readForUpload(context, uri, maxEdge) - return uploadAttachment(settings, sessionId, bytes, mime, "image") + return uploadAttachment(settings, sessionId, mime, "image") { it.write(bytes) } } /** @@ -53,26 +53,34 @@ suspend fun uploadPicked( if (mime != null && mime.startsWith("image/")) { return uploadPickedImage(context, settings, sessionId, uri, maxEdge) } - val bytes = readAll(resolver, uri) - return uploadAttachment( - settings, - sessionId, - bytes, - mime ?: "application/octet-stream", - displayName(resolver, uri), - ) + // Opened before the request starts, so a provider that refuses says so here and not from + // inside the connection; then streamed, since a trace or a log is bigger than this process + // should hold at once. + val source = openSource(resolver, uri) + val name = displayName(resolver, uri) + return uploadAttachment(settings, sessionId, mime ?: "application/octet-stream", name) { out -> + try { + source.use { it.copyTo(out, COPY_BUFFER) } + } catch (e: java.io.IOException) { + // Either side of the copy can fail; the message names the file, which is the + // part the reader can do something about. + throw ApiException("couldn't send $name: ${e.message}", e) + } + } } +private const val COPY_BUFFER = 64 * 1024 + /** - * Everything at [uri], or the failure as the kind the composer reports beside the message. + * A stream of [uri], or the refusal as the kind the composer reports beside the message. * * A share arrives with whatever access the other app granted, and a provider that refuses says so * with a `SecurityException`; a file gone between the pick and the read is an `IOException`. Both * are things the reader can act on, so neither is left to end the process. */ -private fun readAll(resolver: ContentResolver, uri: Uri): ByteArray = +private fun openSource(resolver: ContentResolver, uri: Uri): java.io.InputStream = try { - resolver.openInputStream(uri)?.use { it.readBytes() } + resolver.openInputStream(uri) ?: throw ApiException("couldn't read ${uri.lastPathSegment ?: uri}: nothing there") } catch (e: SecurityException) { throw ApiException("couldn't read ${uri.lastPathSegment ?: uri}: no access to it") @@ -80,6 +88,10 @@ private fun readAll(resolver: ContentResolver, uri: Uri): ByteArray = throw ApiException("couldn't read ${uri.lastPathSegment ?: uri}: ${e.message}") } +/** Everything at [uri]; an image is decoded whole anyway, so it is read whole. */ +private fun readAll(resolver: ContentResolver, uri: Uri): ByteArray = + openSource(resolver, uri).use { it.readBytes() } + /** * The name a document provider shows for [uri]. The last path segment is the fallback because a * provider's own id for a file is usually a number, which says nothing to the session. diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/SetupsScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/SetupsScreen.kt index ace30b0..23c2dc0 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SetupsScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SetupsScreen.kt @@ -236,6 +236,7 @@ private fun AddSetupDialog( var name by remember { mutableStateOf("") } var address by remember { mutableStateOf("") } var identity by remember { mutableStateOf("") } + var attachmentsDir by remember { mutableStateOf("") } var tested by remember { mutableStateOf(null) } var testing by remember { mutableStateOf(false) } @@ -249,6 +250,7 @@ private fun AddSetupDialog( address = host, port = typedPort, identityFile = identity.trim().ifEmpty { null }, + attachmentsDir = attachmentsDir.trim().ifEmpty { null }, ) } @@ -285,6 +287,15 @@ private fun AddSetupDialog( label = { Text("Key path on the backend") }, singleLine = true, ) + // Where a file attached from the phone lands on that machine. Blank means the + // session's own directory, which is what most people want and what needs no + // path typed on a phone. + OutlinedTextField( + value = attachmentsDir, + onValueChange = { attachmentsDir = it }, + label = { Text("Folder for attached files (optional)") }, + singleLine = true, + ) tested?.let { Spacer(Modifier.height(8.dp)) Text(it, style = MaterialTheme.typography.bodySmall) diff --git a/server/src/config.rs b/server/src/config.rs index a9f89ba..9760d8f 100644 --- a/server/src/config.rs +++ b/server/src/config.rs @@ -106,6 +106,12 @@ pub struct SshConfig { /// Extra `-o` settings, each written as `Key=value`. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub options: Vec, + /// Where a file attached from the phone is put on this machine so the + /// session can read it. Absent means the session's own working + /// directory, or the login home for a session that has none. A `~` + /// prefix is the remote home. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub attachments_dir: Option, } /// Which translator runs a session. A new one is a new driver behind the @@ -436,6 +442,7 @@ mod tests { port: Some(2222), identity_file: None, options: Vec::new(), + attachments_dir: None, }), providers: vec![ProviderConfig { name: "claude-cli".to_string(), diff --git a/server/src/routes.rs b/server/src/routes.rs index 2e30fa6..114a5b5 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -267,6 +267,9 @@ struct SshRequest { identity_file: Option, #[serde(default)] options: Vec, + /// Where attached files land on that machine; see `SshConfig`. + #[serde(default)] + attachments_dir: Option, } impl SshRequest { @@ -288,6 +291,15 @@ impl SshRequest { .iter() .filter_map(|o| crate::setups::tidy(o)) .collect(), + // Not `tidy`: that expands `~` to *this* machine's home, and + // this path is on the other one. The remote shell expands it + // there (`ssh::quote_path`). + attachments_dir: self + .attachments_dir + .as_deref() + .map(str::trim) + .filter(|dir| !dir.is_empty()) + .map(std::path::PathBuf::from), }) } } @@ -1279,21 +1291,32 @@ async fn compact( Ok(StatusCode::NO_CONTENT) } -/// The most one attachment may be. A day of `perfetto` is under a -/// gigabyte; a phone photo is a few megabytes; this is the room between. -const ATTACHMENT_LIMIT: usize = 1024 * 1024 * 1024; +/// The most one attachment may be. Streamed to disk, so this bounds the +/// session directory rather than memory; a day of `perfetto` is under a +/// gigabyte, and this leaves room for a few of them. +const ATTACHMENT_LIMIT: usize = 4 * 1024 * 1024 * 1024; /// Accepts one file (any multipart field) and stores it under the /// session; the returned id goes into a later `/message`'s attachmentIds. /// An image is later shown to the model, anything else is named to it by /// path -- see `ClaudeDriver::send_user_message`. +/// +/// Written to disk as it arrives rather than collected first: a trace is +/// bigger than this process should hold, and the phone streams it for the +/// same reason. Under a `.part` name until it is whole, so a tunnel that +/// drops mid-upload leaves nothing a message could reference. +/// +/// A file for a session on another machine is copied there too, because +/// the path the session is told has to exist where the session runs. The +/// copy is part of the upload: if it fails, the upload fails and says so, +/// rather than a message later naming a file that is not there. async fn upload_attachment( State(manager): State>, UrlPath(id): UrlPath, mut multipart: axum::extract::Multipart, ) -> Result, ApiError> { let session = lookup(&manager, &id)?; - let field = multipart + let mut field = multipart .next_field() .await .map_err(|err| ApiError::BadRequest(format!("bad upload: {err}")))? @@ -1303,16 +1326,115 @@ async fn upload_attachment( .unwrap_or("application/octet-stream") .to_string(); let file_name = field.file_name().map(str::to_string); - let bytes = field - .bytes() - .await - .map_err(|err| ApiError::BadRequest(format!("upload read failed: {err}")))?; - let name = session - .save_attachment(&bytes, &content_type, file_name.as_deref()) + let (name, path) = session + .new_attachment(&content_type, file_name.as_deref()) .map_err(bad_request)?; + let part = path.with_file_name(format!("{name}.part")); + let received: Result<(), ApiError> = async { + use tokio::io::AsyncWriteExt; + let mut file = tokio::fs::File::create(&part) + .await + .with_context(|| format!("create {}", part.display()))?; + while let Some(chunk) = field + .chunk() + .await + .map_err(|err| ApiError::BadRequest(format!("upload read failed: {err}")))? + { + file.write_all(&chunk) + .await + .with_context(|| format!("write {}", part.display()))?; + } + file.flush() + .await + .with_context(|| format!("finish {}", part.display()))?; + Ok(()) + } + .await; + if let Err(err) = received { + let _ = tokio::fs::remove_file(&part).await; + return Err(err); + } + tokio::fs::rename(&part, &path) + .await + .with_context(|| format!("name {}", path.display())) + .map_err(ApiError::Internal)?; + + // Images are not copied: they ride the message itself as base64. + if crate::media::media_type_for(&name).is_none() + && let Some((ssh, cwd)) = manager.remote_of(&id) + { + match ship_attachment(&ssh, cwd.as_deref(), &path, &name).await { + Ok(remote) => { + tokio::fs::write(remote_marker(&path), remote) + .await + .with_context(|| format!("record where {name} went")) + .map_err(ApiError::Internal)?; + } + Err(err) => { + let _ = tokio::fs::remove_file(&path).await; + return Err(ApiError::BadRequest(format!( + "{name} reached the server but couldn't be copied to {}: {err:#}", + ssh.address + ))); + } + } + } Ok(axum::Json(serde_json::json!({ "id": name }))) } +/// Copies `local` to the machine `ssh` names, into the configured +/// attachments directory, else `cwd`, else the login home, and returns the +/// absolute path it has there. +/// +/// One `ssh` invocation does the copy and answers the path: the file goes +/// over stdin to `cat`, and `pwd -P` afterwards resolves whatever the +/// directory was written as -- a `~`, a relative name, a symlink -- into +/// the path the session will be told, which is the one a CLI's file tools +/// take. `scp` would need a second round trip for that answer. +async fn ship_attachment( + ssh: &crate::config::SshConfig, + cwd: Option<&Path>, + local: &Path, + name: &str, +) -> anyhow::Result { + let dir = ssh.attachments_dir.as_deref().or(cwd); + let mut script = String::new(); + if let Some(dir) = dir { + let dir = crate::ssh::quote_path(&dir.to_string_lossy()); + // Created if missing: a configured directory may not exist yet, + // and a session's own cwd already does, so this costs it nothing. + script.push_str(&format!("mkdir -p {dir} && cd {dir} && ")); + } + script.push_str(&format!("cat > {} && pwd -P", crate::ssh::quote(name))); + let source = std::fs::File::open(local).with_context(|| format!("open {}", local.display()))?; + let mut command = tokio::process::Command::from(crate::ssh::command( + Some(ssh), + "sh", + &["-c".to_string(), script], + None, + )); + command + .stdin(source) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()); + let output = command.output().await.context("run ssh")?; + if !output.status.success() { + anyhow::bail!("{}", String::from_utf8_lossy(&output.stderr).trim()); + } + let dir = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if dir.is_empty() { + anyhow::bail!("the remote shell did not say where it put the file"); + } + Ok(format!("{dir}/{name}")) +} + +/// Where the remote path of a shipped attachment is recorded, beside it. +/// Read by `ClaudeDriver`'s `attachment_path`; removed with the session. +fn remote_marker(local: &Path) -> std::path::PathBuf { + let name = local.file_name().unwrap_or_default().to_string_lossy(); + local.with_file_name(format!("{name}.remote")) +} + /// Serves a session's stored files -- both `files/` (images produced by /// tools) and `attachments/` (uploaded from the phone), by the id events /// and uploads reference. diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index 674dea5..106f110 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -1220,6 +1220,14 @@ fn attachment_path(session_dir: &Path, id: &str) -> Result { anyhow::bail!("invalid attachment id"); } let path = session_dir.join("attachments").join(id); + // A file copied to the session's own machine is named where it landed + // there -- `routes::upload_attachment` writes that down beside it -- + // because the path has to be one the CLI can open, not one this server + // can. + let shipped = path.with_file_name(format!("{id}.remote")); + if let Ok(remote) = std::fs::read_to_string(&shipped) { + return Ok(PathBuf::from(remote.trim())); + } std::fs::canonicalize(&path).with_context(|| format!("find {}", path.display())) } @@ -1244,6 +1252,30 @@ fn attachment_block(session_dir: &Path, id: &str) -> Result { mod tests { use super::*; + #[test] + fn an_attachment_is_named_where_the_cli_can_open_it() { + let dir = tempfile::tempdir().expect("temp dir"); + let attachments = dir.path().join("attachments"); + std::fs::create_dir(&attachments).unwrap(); + std::fs::write(attachments.join("ab12-x.bin"), b"x").unwrap(); + assert_eq!( + attachment_path(dir.path(), "ab12-x.bin").unwrap(), + attachments.join("ab12-x.bin").canonicalize().unwrap() + ); + // Shipped to the session's machine: the path there, not here. + std::fs::write( + attachments.join("ab12-x.bin.remote"), + "/home/t/in/ab12-x.bin\n", + ) + .unwrap(); + assert_eq!( + attachment_path(dir.path(), "ab12-x.bin").unwrap(), + PathBuf::from("/home/t/in/ab12-x.bin") + ); + assert!(attachment_path(dir.path(), "../config.ron").is_err()); + assert!(attachment_path(dir.path(), "missing.bin").is_err()); + } + /// Drives real CLI output lines through the reader and collects what /// came out, which is the only way to check the wiring between "the /// CLI said this" and "the transcript records that". diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index 3a682de..a3db9f2 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -485,9 +485,11 @@ impl LiveSession { .expect("transcript lives in the session dir") } - /// Stores one uploaded attachment, returning the id `POST /message` - /// references it by. Removed with the session directory on delete -- - /// the same path out as everything else in it. + /// Reserves the name and path for one uploaded attachment; the caller + /// writes the bytes, since a trace is bigger than this should hold. + /// The name is the id `POST /message` references it by. Removed with + /// the session directory on delete -- the same path out as everything + /// else in it. /// /// An image is named `.` and nothing else, since the /// model is shown the picture rather than told its name. Anything else @@ -496,21 +498,19 @@ impl LiveSession { /// more to it than `3f9a…` would. The name is cleaned to characters a /// path and a URL both take unquoted, and the hex keeps two uploads of /// the same name apart. `AttachmentRef` documents the two shapes. - pub fn save_attachment( + pub fn new_attachment( &self, - bytes: &[u8], content_type: &str, file_name: Option<&str>, - ) -> Result { + ) -> Result<(AttachmentRef, PathBuf)> { let name = match crate::media::extension_for(content_type) { Some(extension) => format!("{}.{extension}", random_hex()), None => format!("{}-{}", random_hex(), safe_file_name(file_name)), }; let dir = self.dir().join("attachments"); wg_app_link::private::create_dir(&dir)?; - std::fs::write(dir.join(&name), bytes) - .with_context(|| format!("write attachment {name}"))?; - Ok(name) + let path = dir.join(&name); + Ok((name, path)) } /// `setup_name` and `cwd` are passed in rather than read from the @@ -983,6 +983,20 @@ impl SessionManager { /// direction, and it exists for the same delete the phone offers a /// toggle for: removing a session here can also remove the machine's /// own transcript of it, and only the server knows which file that is. + /// The machine a session runs on when that is not this one, with the + /// session's working directory there: what an upload needs to put a + /// file where the session can read it. `None` for a local session. + pub fn remote_of(&self, id: &str) -> Option<(crate::config::SshConfig, Option)> { + let inner = self.inner.read().unwrap(); + let meta = inner.config.sessions.iter().find(|meta| meta.id == id)?; + let setup = inner + .config + .setups + .iter() + .find(|setup| setup.id == meta.setup)?; + Some((setup.ssh.clone()?, meta.cwd.clone())) + } + pub fn foreign_transcript(&self, id: &str) -> Option<(String, String)> { let inner = self.inner.read().unwrap(); let meta = inner.config.sessions.iter().find(|meta| meta.id == id)?; @@ -1802,7 +1816,9 @@ fn safe_file_name(name: Option<&str>) -> String { }) .collect(); let cleaned = cleaned.replace("..", "_").trim_matches('.').to_string(); - if cleaned.is_empty() { + // Nothing a person would recognise as a name is left: say so rather + // than store a file called `_`. + if !cleaned.chars().any(|c| c.is_ascii_alphanumeric()) { return "file".to_string(); } let excess = cleaned.chars().count().saturating_sub(FILE_NAME_LIMIT); @@ -2393,6 +2409,25 @@ async fn pump( #[cfg(test)] mod tests { use super::*; + + #[test] + fn a_file_name_is_reduced_to_what_an_id_may_hold() { + assert_eq!( + safe_file_name(Some("trace komodo (1).perfetto-trace")), + "trace_komodo__1_.perfetto-trace" + ); + let hostile = safe_file_name(Some("../../etc/passwd")); + assert!( + !hostile.contains('/') && !hostile.contains(".."), + "{hostile}" + ); + assert_eq!(safe_file_name(Some("...")), "file"); + assert_eq!(safe_file_name(None), "file"); + let long = "x".repeat(200) + ".pftrace"; + let kept = safe_file_name(Some(&long)); + assert_eq!(kept.len(), FILE_NAME_LIMIT); + assert!(kept.ends_with(".pftrace")); + } use std::time::Duration; fn echo_spec() -> SpawnSpec { diff --git a/server/src/ssh.rs b/server/src/ssh.rs index 6a1bc50..890852d 100644 --- a/server/src/ssh.rs +++ b/server/src/ssh.rs @@ -121,7 +121,7 @@ fn remote_script(program: &str, args: &[String], cwd: Option<&Path>) -> String { /// `~user` is deliberately not handled: there is no portable expansion for /// it, and inventing one would mean guessing another account's home /// directory. It stays literal and fails with the shell's own message. -fn quote_path(path: &str) -> String { +pub(crate) fn quote_path(path: &str) -> String { if path == "~" { return "\"$HOME\"".to_string(); } @@ -137,7 +137,7 @@ fn quote_path(path: &str) -> String { /// names, and prompts-as-arguments are all attacker-adjacent input in a /// server whose whole job is running commands, and unquoted they would be /// shell syntax rather than data. -fn quote(word: &str) -> String { +pub(crate) fn quote(word: &str) -> String { // Inside single quotes every character is literal except `'` itself, // which is closed, escaped, and reopened. format!("'{}'", word.replace('\'', r"'\''")) @@ -168,6 +168,7 @@ mod tests { port: None, identity_file: None, options: vec![], + attachments_dir: None, } } @@ -190,6 +191,7 @@ mod tests { port: Some(2222), identity_file: Some("/home/me/.ssh/id_ai".into()), options: vec!["StrictHostKeyChecking=accept-new".to_string()], + attachments_dir: None, }; let rendered = argv(&command( Some(&ssh), diff --git a/server/src/usage.rs b/server/src/usage.rs index d43771c..929f855 100644 --- a/server/src/usage.rs +++ b/server/src/usage.rs @@ -423,6 +423,7 @@ mod tests { port: None, identity_file: None, options: vec!["ConnectTimeout=1".to_string()], + attachments_dir: None, }), providers: vec![crate::config::ProviderConfig { name: "claude-cli".to_string(),