Repository navigation
Conversation
Server-owned skills were sent as one base64 JSON document per tree, re-read and re-hashed synchronously on every catalog resolution, and fully re-downloaded after any edit. Attachments were read whole on the server and buffered twice on the daemon. - Skill trees use a v2 hash over per-file (path, mode, size, sha256), shared by server and daemon via @bb/host-daemon-contract/skill-tree. - SkillTreeRegistry caches per-file hashes keyed by (dev, ino, size, mtime, ctime) with a racy-timestamp guard, and keeps metadata-only manifests that drop superseded hashes. - GET /internal/skills/tree/:hash returns the manifest; POST .../content streams the requested files concatenated, refusing files whose stat changed since hashing. - The daemon reuses files it already stores by sha256, downloads only the missing ones in one streamed request, verifies size and hash per file, and keeps only metadata in its tree memo. - Attachments stream into a hidden temp file with size checks and are published with a no-clobber hard link. - HOST_DAEMON_PROTOCOL_VERSION 253. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ransfers - copyVerifiedFile opened its source stream before awaiting the destination open, so a missing source could emit an unhandled error and crash the daemon when the destination open failed. The source is now read lazily. - writeVerifiedFile ignored bytesWritten, so a short write (for example under a file-size limit) published a truncated file that passed verification. Each chunk is now written fully. - SkillTreeRegistry took size and mode from the first stat even when hashing retried after the file changed, producing manifests the content route could not satisfy. Metadata now comes from the stat the hash matched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stat-keyed cache saved about 20 ms per catalog resolution for 36 MiB of skills and about 1-2 ms at typical sizes, but its invalidation rules were the most error-prone part of the change. Every resolution now hashes each file again, streamed and async. Manifests by hash stay, so the content route can still refuse files that changed after hashing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The server read each attachment fully and copied it into the response; the daemon collected every chunk and then copied them into a second buffer before writing. A 34 MiB attachment peaked at about 65 MiB of live memory across both sides. The internal attachment route now uses the existing streaming file response. The daemon streams the body into a hidden temp file in the thread's Attachments directory, enforcing the size cap and expected size and writing each chunk fully. The finished file is published with a no-clobber hard link (copy fallback), and the temp file is always removed. Authorization, limits, cleanup and reference rewriting are unchanged. The wire format is unchanged, so no protocol bump is needed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…stream-and-consolidate-skill-and-attachment-tran-thr_tjj6vvmme2 # Conflicts: # apps/host-daemon/src/verified-file-transfer.ts
ymichael
changed the title
ymichael
changed the base branch from
main
to
bb/stream-attachment-transfers-thr_tjj6vvmme2
October 26, 2026 19:25
ymichael
added a commit
that referenced
this pull request
Oct 26, 2026
) ## Human comments ## What was wrong The internal attachment route read each attachment fully on the server (`readAttachment`) and copied it into a new `Uint8Array` for the response. The daemon then collected every chunk into an array and copied them into a second full buffer before writing the file. With the 35 MiB attachment cap, one attachment held several full copies in memory at once. In a measurement, a 34 MiB attachment peaked at about 65 MiB of live memory, at least. ## What changed - **Server** (`apps/server/src/internal/session.ts`): `/internal/session/project-attachment-content` now returns the existing streaming `createAttachmentContentResponse`, which reads in 64 KiB chunks, sends `content-length`, and refuses files that change mid-read. Authorization (session, thread, project, assigned host) is unchanged. - **Daemon** (`apps/host-daemon/src/verified-file-transfer.ts`, new): - `writeVerifiedFile` streams a source into a newly created file. It enforces the byte cap and expected size and writes each chunk fully. On any failure it cancels the source and removes the partial file. - `hashFile` and `publishFileWithoutReplacing` (hard link, no-clobber copy fallback) support publishing. - **Daemon** (`command-handlers/prompt-attachments.ts`, `server-client.ts`): - `fetchProjectAttachment` returns the response body stream. `content-length` is still checked before reading. - Staging writes to a hidden `.bb-attachment-<uuid>.partial` in the thread's `Attachments` directory, then publishes it under the usual name or a `-N` suffix. An identical existing file is detected by hash. The temp file is always removed. - **No wire change**, so no `HOST_DAEMON_PROTOCOL_VERSION` bump. An older daemon reads the streamed response like the old one, and this daemon reads an old server's full response as a stream. - This is the attachment half of #6330, which is now stacked on this PR and covers only the skill-tree transfer. ## How you verified - **Tests:** - New: an interrupted attachment stream fails with `attachment_unavailable` and leaves no partial file, and an attachment whose declared length differs is rejected before its body is read (the body is cancelled). - Existing staging tests (renaming, identical-file reuse, cleanup on runtime failure, size mismatch) pass with streamed content. - **Commands:** `turbo run typecheck --filter=@bb/host-daemon --filter=@bb/server`; vitest for `server-client`, `thread-dispatch`, `command-router`, `command-dispatch` and `internal-project-attachment-content`. - **Memory**, measured with the real server on loopback HTTP plus the real daemon client and staging code: a 34 MiB attachment peaked at ≥ 64.7 MiB of live heap before and 1.7 MiB after, sampled with forced GC. - **Manual repro:** under `ulimit -f`, the old write loop reported 4,096 bytes written with 512 on disk. The full-write loop fails with `EFBIG` and leaves no file. - **Live run** with a real Codex thread on a `pnpm start:worktree` instance: a 30 MiB attachment was staged with a matching sha256. 🤖 Generated with [Claude Code](https://claude.com/claude-code) > AGENT GENERATED Co-authored-by: review <review@local> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Base automatically changed from
bb/stream-attachment-transfers-thr_tjj6vvmme2
to
main
October 26, 2026 19:33
…date-skill-and-attachment-tran-thr_tjj6vvmme2 # Conflicts: # apps/host-daemon/src/verified-file-transfer.ts
ymichael
deleted the
bb/stream-and-consolidate-skill-and-attachment-tran-thr_tjj6vvmme2
branch
October 26, 2026 19:46
ymichael
pushed a commit
that referenced
this pull request
Oct 26, 2026
…sets (#6267) ## Human comments ## What was wrong A skill folder larger than 10 MiB could prevent every thread from starting, even when most of its size was supporting assets rather than instructions. The server included the skill in the catalog, but the daemon rejected the required tree with `Skill tree exceeds max byte count 10485760`. Registry installation separately rejected the same folders with `Skill tree exceeds installation limits`. ## What changed - Remove the whole-folder byte cap from daemon skill collection, streamed manifest validation, and server registry installation. - Preserve file-count, directory-count, depth, path, symlink, and content-integrity checks. Instruction-file read limits are unchanged. - Apply the fix on top of #6330's streamed transfers. Large skills still cost disk space, hashing time, and bandwidth; this change does not impose a replacement byte budget. - Bump the host daemon protocol from 253 to 254 so connected machines update instead of continuing to reject oversized skills. ## How you verified - Reproduced the streamed-tree regression against current main: an 11 MiB asset failed with `Skill tree exceeds max byte count 10485760`. The same regression passes with this change and verifies the complete staged bytes. - Added registry installation coverage for an 11 MiB supporting asset. - Verified through the isolated worktree app with a real Codex thread: the original failure is visible in its history, and the updated streaming implementation completes a new turn with `STREAMING_OK`. The staged asset is 11,534,336 bytes and its SHA-256 matches the source. - Host daemon tests: 76 files, 841 passed, 12 skipped. Host daemon contract tests: 5 files, 77 passed. - Focused server skills, public project skills, and internal skill-tree tests: 9 files, 133 passed. - Turbo typechecks for the server, daemon, and daemon contract pass. Production worktree build, formatting checks, and `git diff --check` pass. Fixes #6240 > AGENT GENERATED Co-authored-by: review <review@local>
ymichael
added a commit
that referenced
this pull request
Oct 26, 2026
…read (#6335) ## Human comments ## What was wrong `host.write_file` without `expectedSha256` overwrote its target in place, so a crash or full disk partway through left a truncated file. `host.read_file_relative` loaded whole files with no size limit and sent them as one base64 WebSocket message. Both bugs came from duplicated paths. There were two whole-file reads: `read_file_relative` was built for the status-folder server, removed in June, and its only remaining caller was the provider-skill read. There were also three hand-written copies of the temp-file, hash-check, rename save, so a fix in one never reached the others. ## What changed - **One atomic save:** new dependency-free package `@bb/file-transfer`. `verified-file-transfer.ts` (from #6332/#6330) moved into it unchanged, apart from the `durable` and `mode` arguments below. A new `replaceFile` does the whole save: 1. check the expected hash; 2. write and fsync a fixed-length `.bb-write-<uuid>` temp file next to the target; 3. put it in place: - **no hash:** rename over the target; - **`expectedSha256: null`:** no-clobber link; - **a hash:** re-check, then rename. - **Who uses it:** `writeHostFile` keeps only root confinement, parent creation and error mapping. The server-owned skill save in `skill-listing.ts` calls `replaceFile` directly instead of its own copy. - **Modes:** an existing file keeps its mode exactly. A requested mode on a new file goes through `open()`, so the umask applies, as on main. - **Read-only folders:** an unconditional write falls back to overwriting in place, as on main. A hash-checked save fails with `EACCES`, as on main. - **fsync:** `writeVerifiedFile` takes `durable` (user saves fsync; attachments and skill trees don't) and an explicit create-or-exact `mode`. - **One whole-file read:** removed `host.read_file_relative`. The skill read now uses `host.read_file`, which has the 25 MB (10 MB image) cap. - **Symlinked skill folders:** `host.read_file` gains a required `followRootSymlink` boolean, matching `host.list_files`, so linked skill folders still resolve (#5556). Skill reads pass `true`; every other caller passes `false`. The schema rejects `true` with a git `ref` or without a `rootPath`. - **Server-side path checks:** the server rejects absolute, `..` and dotfile skill paths before calling the host, using the same check server-owned skills already used. - **Errors:** an oversized skill file now returns 413 instead of 502. - **Wire change:** `HOST_DAEMON_PROTOCOL_VERSION` goes from 255 to 256, for the removed command and the new required field. `host.write_file`'s fields and results are unchanged. - **Docs:** updated the symlinked-skill paragraph in `docs/debugging-and-qa.md`. - **Behaviour note:** saves replace the file, so hard links, xattrs and ACLs on an overwritten file aren't carried over. Hash-checked saves already worked this way. ## How you verified - **New tests:** - `@bb/file-transfer`: - an interrupted write leaves the original intact; - read-only folder: an unconditional write lands in place, a hash-checked save is refused; - a requested mode respects the umask; - a 240-character filename saves. - daemon: an oversized read under a followed root is rejected. - server: - oversized skill file → 413; - absolute, `..` and dotfile skill paths are rejected without calling the host; - server-owned skill save writes on a fresh revision and returns 409 on a stale one. - Each failed before its fix. - **Commands:** - `pnpm exec turbo run typecheck lint --filter='...[origin/main]'`: 133/133 tasks pass. - `pnpm exec turbo run test` for `@bb/file-transfer` (7), `@bb/host-daemon-contract` (76), `@bb/host-daemon` (833, 12 skipped) and `@bb/server` (3655 of 3656). The one server failure is a timeout in `builtin-plugins.test.ts`, which nothing on this branch touches. A different test in that file timed out on each full run, both under heavy machine load, and that file's 28 tests pass when run alone. - **Manual:** a `pnpm start:worktree` instance with a fixture project, checked by a script of 24 CLI/API checks, all passing: - symlinked skill reads, the 27 MB → 413 case, rejected paths; - daemon- and server-saved skills: fresh save lands, stale save returns 409, no temp files left behind; - `bb file write`: unconditional, stale hash, read-only folder; - the umask and long-filename cases. > AGENT GENERATED 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: review <review@local> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
ymichael
added a commit
that referenced
this pull request
Oct 27, 2026
…d file transfer (#6331) ## Human comments ## What was wrong Two server-to-daemon downloads still held the whole payload in memory, and they skipped the verification that #6330 added for skills and attachments. - **Daemon self-update** read the host package with `response.arrayBuffer()`. It checked the sha256 only if the server sent `x-bb-artifact-sha256`. It then wrote `bb-app-update-<pid>.tgz` with `writeFile`, which is not atomic and leaves a partial file behind if the daemon crashes mid-write. - **The install route.** `GET /install/bb-app.tgz` called `readFile` on the whole tarball for every request. - **Plugin host artifacts.** `fetchPluginHostArtifact` collected chunks into one buffer of up to `HOST_ARTIFACT_MAX_BYTES` (256 MiB), so the cache held two copies during a write. Every worker start or bridge launch also read the whole cached `host.mjs` back and re-hashed it with `readFile`. ## What changed Both downloads now go through `writeVerifiedFile` from `@bb/file-transfer` (added in #6330, moved into the package by #6335); no parallel download code is added. **Self-update** (`apps/host-daemon/src/protocol-self-update.ts`) - Streams into `<dataDir>/host-package-download/bb-app.tgz.partial` through `writeVerifiedFile` with a sha256 and an expected size. On success it renames the file to `bb-app.tgz` and runs `npm install -g` on it. The rename also gives npm the `.tgz` name it uses to recognize a tarball. - The download directory is cleared before each attempt and after it, so a crash mid-download can't leave tarballs behind. - **The digest is now required.** A 200 response without a valid `x-bb-artifact-sha256` fails the attempt and nothing is installed. - `content-length` becomes the expected size when present. There is also a 1 GiB cap that rejects an oversized `content-length` before any bytes are read. **Install route** (`apps/server/src/server.ts`, `apps/server/src/internal/host-artifact-response.ts`) - `GET /install/bb-app.tgz` streams the file through the existing `hostArtifactFileResponse`, which now takes the response headers as a parameter. The plugin artifact route passes its own headers. - Status codes, headers (`etag`, `x-bb-artifact-sha256`, `content-length`, `content-type`, `cache-control`), the 304 path and the response bytes are all unchanged. - If the cached tarball is missing or has a different size than recorded, the route returns the existing `host_package_unavailable` 500. **Plugin host artifacts** (`server-client.ts`, `node-artifact-cache.ts`, `plugin-host-artifact-cache.ts`, `plugin-host-manager.ts`) - `fetchPluginHostArtifact` returns the response body as a stream after the status and `content-length` checks. `readHostArtifactBytes` is deleted. - The cache writes `.staged-<uuid>.tmp` through `writeVerifiedFile` with the digest, the byte length and the 256 MiB cap, then renames it into place. - It still retries a verification failure once. Network errors still propagate. Partial files are always removed. - **No re-hash of unchanged cache files.** A verified cache file is remembered by `(dev, ino, size, mtimeNs, ctimeNs)`, mirroring #6330's `SkillTreeRegistry`. The same racy-timestamp guard applies: a file whose mtime or ctime is within 2 s of now is not remembered. A changed stat key triggers a full re-hash; if that fails, the file is deleted and downloaded again. - Memo entries are dropped when stale digests are pruned, so there is at most one per plugin. ### Compatibility (no `HOST_DAEMON_PROTOCOL_VERSION` bump) The wire format is unchanged: same routes, request headers, response headers and bytes. - **Self-update across versions.** The download only starts when `server.protocolVersion > HOST_DAEMON_PROTOCOL_VERSION`; an older or equal server is refused before any download. So this daemon only ever downloads from servers built after this change. Every server since #2969 (2026-09-02) sends `x-bb-artifact-sha256` and `content-length` on the 200, so requiring the digest cannot strand a daemon that is updating from a newer server. - **Older daemons against the new server.** They get byte-identical responses with the same headers. Daemons from before #2969 ignore the digest. Later ones verify it when present and send `if-none-match`, which still gets a 304. `install.sh` and the Windows installer download the same URL. - **Plugin artifacts.** The server route is unchanged. The server's `compress()` middleware gzips `text/javascript` and removes `content-length`. The daemon already tolerated a missing `content-length`, and it still verifies the exact byte length and digest from the session payload. - **Forward-compatibility limit.** The 1 GiB self-update cap is now built into shipped daemons. The host package is about 1.4 MB today, but a package over 1 GiB would need a manual install on older daemons. ## How you verified **Tests** - New `apps/host-daemon/src/node-artifact-cache.test.ts`: - streamed multi-chunk download - corrupt download: retried once, then fails with no files left - corrupt download followed by an intact retry - oversized endless stream: reading stops at the expected length and the generator is closed - interrupted stream: the error propagates without a retry and no files are left - stat-key memo: hashes inside the racy window, stops hashing after it, and a same-size tamper is re-hashed and re-downloaded - `apps/host-daemon/src/protocol-self-update.test.ts` now uses server-shaped responses and adds: - one table of failure cases (digest mismatch, missing digest header, body longer or shorter than `content-length`, connection reset mid-body). Each asserts no install and no files left in the data dir. - an over-cap `content-length` that cancels the body before it is read - leftovers from an interrupted earlier attempt are cleared - the installed file is the verified `bb-app.tgz`, never the `.partial` - `apps/server/test/app/skeleton.test.ts` asserts the streamed `content-length`, and that a cached tarball whose size changed returns `host_package_unavailable`. **Commands** - `pnpm exec turbo run typecheck test --filter=@bb/host-daemon --filter=@bb/server`: - typecheck passes for both packages - host-daemon: 849 passed, 12 skipped - server: 3692 passed, 1 failed, in `builtin-plugins.test.ts` (builtin plugin hot-reload, which this PR doesn't touch). It failed under load average ~23. Run alone, a different test in that file failed once, then the whole file passed twice. - `oxfmt --check` and `oxlint` are clean on the changed daemon and test files. **Memory, measured on the real code paths** - **Server side:** the real `createApp` install route (test harness) and the real `registerInternalPluginHostArtifactRoutes` behind `compress()`, served over HTTP with `@hono/node-server`. - **Daemon side:** a separate Node process running the real `createProtocolSelfUpdater` (stub `installTarball`; `/install/version` rewritten to protocol+1) and the real `createServerClient().fetchPluginHostArtifact` with `ensureCachedPluginHostArtifact`. - **Payloads:** the real files are this machine's cached host tarball (1.4 MB) and the provider-claude-code `host.mjs` (2.1 MB). The 64 and 256 MiB payloads are those same files repeated. - **Method:** "before" swaps in this PR's base versions of the 8 changed source files. Each figure is the median of 3 fresh processes. Daemon figures are `process.resourceUsage().maxRSS` growth during one download. Server figures are peak `arrayBuffers` growth. | payload | | self-update daemon peak RSS | install route server arrayBuffers | plugin download daemon peak RSS | cached artifact reuse, per call | |---|---|---|---|---|---| | real | before | +13.2 MiB | +2.7 MiB | +24.1 MiB | 1.77 ms (re-reads and re-hashes 2.1 MB) | | real | after | +11.9 MiB | +2.9 MiB | +23.0 MiB | 0.29 ms (stat only) | | 64 MiB | before | +225.2 MiB | +112.5 MiB | +166.9 MiB | 31 ms | | 64 MiB | after | +64.7 MiB | +23.0 MiB | +62.6 MiB | 0.28 ms | | 256 MiB | before | +809.8 MiB | +485.2 MiB | +572.6 MiB | 105 ms | | 256 MiB | after | +76.0 MiB | +30.9 MiB | +63.4 MiB | 0.23 ms | - **At today's sizes** the memory difference is within noise. The visible gain is cached artifact reuse, which no longer reads and hashes the file on every worker start. - **At larger sizes** the old paths grow with the payload and the new ones stay flat. The remaining ~64–76 MiB at 64 and 256 MiB is fetch/undici buffering on loopback, not file contents. - The first reuse call after a download still hashes once, because the racy guard keeps a freshly written file out of the memo. **Not changed** - `apps/host-daemon/src/server-move/download.ts` has its own streamed, verified download for server moves. It overlaps with `writeVerifiedFile` and could be folded in separately. - `bb-app-artifact.ts` still reads the packed tarball once per server process to compute its digest. That happens at build time, not per request. > AGENT GENERATED Co-authored-by: review <review@local> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human comments
What was wrong
Server-owned skills (global, plugin and built-in) went to the host daemon as one JSON document with every file base64-encoded. The server re-read the tree, and the daemon parsed, decoded, hashed and rewrote it, holding the whole tree in memory at each step. The daemon also kept a per-tree cache of file contents with no byte budget. Any edit, even one line of SKILL.md, re-sent the entire tree including large assets. The server computed tree hashes by synchronously reading every file on each thread start and turn submit.
Follows #6332 (merged), which added attachment streaming and the shared
verified-file-transfer.tshelper. This PR covers only the skill-tree transfer and extends that helper.What changed
HOST_DAEMON_PROTOCOL_VERSION252 → 253):@bb/host-daemon-contract/skill-treefor both server and daemon.GET /internal/skills/tree/:hashreturns a manifest (path, mode, size, sha256 per file).POST /internal/skills/tree/:hash/contentstreams the requested files back to back. The base64 tree response is removed.SkillTreeRegistry.readTreehashes files asynchronously with streamed reads. If a file changes while it is being hashed, it retries up to 3 times and takes size and mode from the stat the hash matched.manifest.json. The collected-tree cache holds metadata only, bounded by tree and file counts.How you verified
Tests:
Manual repro: a 200 MiB file appended mid-hash. The old code recorded the pre-append size; the fix records the final size.
Commands:
turbo run typecheckandturbo run testfor@bb/host-daemon-contract,@bb/host-daemonand@bb/server.Benchmark: real server on loopback HTTP plus the real daemon client and staging code. "Before" was measured on the base commit.
Live run on a
pnpm start:worktreeinstance with a real Codex thread:🤖 Generated with Claude Code