fix(flue-review): hydrate from the GitHub tarball instead of JS git - #2386
Conversation
isomorphic-git pack indexing in the workflow DO stopped completing once the repo's shallow pack passed ~16MB, silently stalling every review in the hydrating stage until the watchdog killed it. The tarball of the PR head SHA needs no git objects at all: stream it through the runtime's native gzip DecompressionStream and untar into the workspace (ustar + GNU longname + pax paths, symlinks included, archive root stripped). The diff was already API-fetched; nothing downstream used the git repo. Also keeps the hydration stage/R2 instrumentation added while diagnosing: every stage and R2 operation logs start/end so the next stall identifies itself.
|
github-actions
Bot
added
size/L
review/needs-review
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 06ffa4f | Aug 08 2026, 09:57 PM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 06ffa4f | Aug 08 2026, 09:56 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 06ffa4f | Aug 08 2026, 09:56 PM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is the right fix: the outage was caused by pure-JS git pack indexing stalling as the repo grew, and the reviewer only ever needed the file tree at the PR head plus the canonical diff. Fetching the GitHub tarball and streaming it through the runtime-native DecompressionStream"gzip") is simpler, faster, and removes the problematic code path entirely. It fits the Cloudflare Shell model (no exec, just workspace files).
What I checked:
- Full diff and the two changed files (
review.tsandcloudflare-shell.ts). - Call sites and the existing test suite under
infra/flue-review/test. - AGENTS.md conventions (comments, tests, scope, etc.).
Headline conclusion: the fix works, but the new tar parser needs path-traversal hardening and a regression test. I also found stale/narrative comments left over from the incident.
Blocking-ish concerns
- Tar path traversal (
untarInto) — the code trusts archive entry paths. A tarball entry whose path contains..or an absolute path could write outside/repo; symlink targets are passed through verbatim and pose the same risk. GitHub tarballs are unlikely to contain such entries, but the parser is the boundary between an external PR and the review workspace, so it should reject escapes explicitly. - Missing automated regression test —
infra/flue-review/testalready uses vitest, yet the new parser has no test. A crafted tarball covering regular files, directories, symlinks, GNU longname, pax, zero-byte entries, and an entry with..should be exercised in CI so a parser regression cannot silently take the bot down again.
Non-blocking
- The top-of-file comment in
review.tsstill describes JS-git hydration. - The "Temporary ... (2026-08-08 review-stall incident)" comments are incident narrative; they will age poorly and should be trimmed to a one-line rationale or removed.
No Lingui/Tailwind/SQL/query-count issues are touched by this infra-only change.
Findings
-
[needs fixing]
infra/flue-review/.flue/workflows/review.ts:271untarIntobuilds the destination path by joiningdestDirwith the archive-providedrelative, but it never validates thatrelativestays insidedestDir. A tarball entry named../foo,/etc/passwd, ordir/../../barwould be written wherever the Workspace resolves it. The same applies to symlink targets passed straight toworkspace.symlink. The Workspace may or may not normalize.., so the parser must defend the boundary itself.const relative = stripRoot(fullName); if (!relative) continue; if (relative.startsWith("/") || relative.split("/").includes("..")) { throw new Error(`tar entry path escapes workspace: ${fullName}`); } const dest = `${destDir}/${relative}`;For symlinks, also validate that the resolved target stays under
destDir(reject absolute targets and targets with..that escape). -
[needs fixing]
infra/flue-review/.flue/workflows/review.ts:175This package already has a vitest suite (
infra/flue-review/test/*.test.ts), but the new tar parser has no automated test. AGENTS.md requires failing-test-first for bug fixes, and a parser regression here would silently re-break every review. Add a unit test that constructs a small tarball (regular files, directories, symlinks, zero-byte file, GNU longname entry, pax path entry) and asserts that the mocked Workspace receives the expected normalized paths and bytes — including a negative case that rejects a..path. -
[suggestion]
infra/flue-review/.flue/workflows/review.ts:5The header comment still says hydration is done "via JS git" with a shallow clone. The PR replaces that with the GitHub tarball, so this comment is now misleading.
// (DO SQLite + R2 for large files) from the GitHub tarball of the PR head, and -
[suggestion]
infra/flue-review/.flue/workflows/review.ts:159Incident-date narrative in a comment will go stale quickly. AGENTS.md says comments are for future readers, not reviewers or incident timelines. Trim to the non-obvious rationale (these logs intentionally bracket stages so a hang appears as an unmatched
startline) and remove the date/incident references. -
[suggestion]
infra/flue-review/.flue/workflows/review.ts:291This comment is mainly historical narrative about the 2026-08-08 outage. Future readers need the "why" (JS git pack indexing stalls on large repos; tarball avoids it), not the date. Trim the incident references.
-
[suggestion]
infra/flue-review/.flue/sandboxes/cloudflare-shell.ts:223The "Temporary R2 diagnostics (2026-08-08 review-stall incident)" comment references a specific incident date and frames the code as temporary. If it is meant to be removed soon, consider tracking that in an issue/comment in the PR rather than the code; if it is staying, describe the operator-facing behavior without the incident narrative.
There was a problem hiding this comment.
Pull request overview
This PR fixes flue-review workflow “hydrating” stalls by replacing JS git-based hydration with a GitHub tarball download + streaming untar into the durable Workspace, and adds additional hydration/R2 instrumentation to make future stalls diagnosable.
Changes:
- Replace isomorphic-git shallow clone/fetch/checkout with
GET /repos/{owner}/{repo}/tarball/{headSha}hydration. - Stream-decompress (
DecompressionStream("gzip")) and incrementally untar into the Workspace (including longname + pax path overrides). - Add structured logging around hydration steps and R2 bucket operations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| infra/flue-review/.flue/workflows/review.ts | Switch hydration to GitHub tarball + add untar implementation and hydration step logging. |
| infra/flue-review/.flue/sandboxes/cloudflare-shell.ts | Add R2 operation instrumentation wrapper used by the Workspace. |
Suppressed comments (1)
infra/flue-review/.flue/workflows/review.ts:277
- Symlink entries are created with an unvalidated
linkTarget. Absolute or traversal targets (e.g.../../...) can create links that point outside the hydrated repo, which is risky for any later file reads/writes that may follow symlinks.
} else if (type === "2") {
await ensureDir(parentOf(dest));
await workspace.symlink(linkTarget, dest);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const relative = stripRoot(fullName); | ||
| if (!relative) continue; | ||
| const dest = `${destDir}/${relative}`; | ||
|
|
| // Temporary hydration diagnostics (2026-08-08 review-stall incident): brackets | ||
| // every stage so a hang shows as a start line with no matching end line. R2 | ||
| // operations are instrumented in the shared getDefaultWorkspace. Remove once | ||
| // the stall is diagnosed. |
| const url = `https://api.github.com/repos/${payload.owner}/${payload.repo}/tarball/${payload.headSha}`; | ||
| const response = await fetch(url, { | ||
| headers: { "User-Agent": "emdash-flue-review", Accept: "application/vnd.github+json" }, | ||
| }); |
| // Temporary R2 diagnostics (2026-08-08 review-stall incident). Wrapped once and | ||
| // cached per underlying bucket: Workspace fingerprints construction options per | ||
| // storage, so every construction site must receive the identical instance. |
github-actions
Bot
added
review/awaiting-author
Review findings on the tarball hydration: the parser is the boundary between an externally-supplied archive and the workspace, so entry paths and symlink targets now reject absolute paths and any traversal that would resolve outside the destination. The parser moves to lib/untar.ts behind a narrow target interface with a vitest suite covering files, directories, symlinks, GNU longname, pax paths, truncation, and the escape rejections. Stale JS-git and incident narrative trimmed from comments.
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
The approach remains sound: replacing isomorphic-git pack indexing with a GitHub tarball fetch directly fixes the deterministic hydration stall and removes the problematic code path. This re-review confirms all concerns from the previous round have been addressed.
What changed since the last review:
- The tar parser is now extracted into
infra/flue-review/.flue/lib/untar.ts, with explicit path and symlink-target validation that rejects absolute paths and any..sequence that escapes the destination. - A regression test suite (
infra/flue-review/test/untar.test.ts) covers regular files, directories, symlinks, zero-byte files, GNU longname, pax path overrides, ustar prefix, path-traversal rejections, absolute-path rejections, symlink-target escapes, and truncated-archive handling. - Incident-narrative and JS-git references in comments have been trimmed to evergreen rationale.
I statically re-read the new untar.ts, review.ts, and cloudflare-shell.ts, checked for leftover incident narrative, and confirmed the tests are present. The trust-boundary validation, the idempotent HYDRATED marker, R2 instrumentation, and the streaming gzip-to-tar pipeline all look correct. The comment style now matches AGENTS.md guidance. No Lingui/Tailwind/SQL/query-count/content-table concerns are touched by this infra-only change.
All blocking items from the prior review are resolved; I have no further findings.
github-actions
Bot
added
review/approved
There was a problem hiding this comment.
The approach is sound and directly fixes the outage: replacing isomorphic-git's pure-JS pack indexing with a streaming GitHub tarball fetch removes the deterministic hydration stall and simplifies the worker. This re-review confirms the prior round's concerns have been addressed — the tar parser is now isolated, path/symlink validation is present, tests cover the common archive formats and traversal attacks, and the incident narrative in comments has been trimmed.
I statically re-read infra/flue-review/.flue/lib/untar.ts, .flue/workflows/review.ts, .flue/sandboxes/cloudflare-shell.ts, and test/untar.test.ts. No core EmDash conventions are touched (no Lingui/Tailwind/SQL/content-table changes). Two parser gaps remain:
- The untar parser buffers an entire entry's content before writing, and it never caps the declared
size. A malformed or merely very large tar entry can exhaust the Durable Object's memory duringneed(padded). Given that this parser is explicitly framed as a trust boundary, it should reject entries above a safe ceiling. - The pax parser only reads the
pathkeyword, notlinkpath. GitHub tarballs use pax extended headers for symlinks whose targets exceed the 100-byte ustarlinknamefield, so a long symlink target would be silently truncated/wrong.
Both are straightforward to close and don't change the overall fix.
|
|
||
| const rawName = readCString(header.subarray(0, 100)); | ||
| const prefix = readCString(header.subarray(345, 500)); | ||
| const size = parseInt(readCString(header.subarray(124, 136)).trim() || "0", 8); |
There was a problem hiding this comment.
[needs fixing] The octal size is parsed but never bounded, and need(padded) below buffers the entire entry before writeFileBytes. A malformed header—or a legitimate large binary—can declare a multi-gigabyte size and exhaust the DO's memory before the write happens. Add an explicit max-entry-size guard after parsing size.
| const size = parseInt(readCString(header.subarray(124, 136)).trim() || "0", 8); | |
| const size = parseInt(readCString(header.subarray(124, 136)).trim() || "0", 8); | |
| // Guard against runaway memory use: one entry is buffered at a time. | |
| const MAX_ENTRY_BYTES = 128 * 1024 * 1024; | |
| if (size > MAX_ENTRY_BYTES) { | |
| throw new Error(`tar entry too large: ${size} bytes`); | |
| } |
| for (const line of text.split("\n")) { | ||
| const eq = line.indexOf("="); | ||
| if (eq > 0 && line.slice(line.indexOf(" ") + 1, eq) === "path") { | ||
| pendingPaxPath = line.slice(eq + 1); |
There was a problem hiding this comment.
[suggestion] The pax parser only honors the path keyword, so a symlink target longer than 100 bytes (stored by GitHub tarballs in a pax linkpath record) falls back to the truncated ustar linkTarget field. Track linkpath the same way as path and use it when present.
| pendingPaxPath = line.slice(eq + 1); | |
| // pax records: "<len> key=value\n" | |
| const text = decoder.decode(content); | |
| for (const line of text.split("\n")) { | |
| const eq = line.indexOf("="); | |
| if (eq > 0) { | |
| const key = line.slice(line.indexOf(" ") + 1, eq); | |
| if (key === "path") pendingPaxPath = line.slice(eq + 1); | |
| if (key === "linkpath") pendingPaxLink = line.slice(eq + 1); | |
| } | |
| } |
What does this PR do?
Fixes the repo-wide review-bot outage (every review since ~16:50 UTC today silently stalling in
hydratinguntil watchdog timeout): replaces isomorphic-git hydration with the GitHub tarball API.Root cause: hydration shallow-cloned the repo with JS git inside the workflow Durable Object. isomorphic-git's pack indexing (pure-JS inflate of the whole packfile) stopped completing once emdash's shallow pack crossed ~16MB — today's merges (two ~9k-line translation catalogs among them) pushed it over. Silent stall: no error, no logs, watchdog kill, deterministic on every PR and every worker build. July's intermittent
FLueErrorfailures were the near-threshold era of the same cliff. Eliminated en route, each by direct test: trace collection (#2384 — innocent, can re-land), R2 availability (packfile put: 610ms), GitHub pack serving (native clone: 7s), PR content, and run concurrency.The fix: the reviewer never needed git — it needs the files at the PR head plus the unified diff, which was already API-fetched. Hydration now fetches
GET /repos/{o}/{r}/tarball/{headSha}(head SHAs of fork PRs are reachable in the base repo), streams through the runtime-nativeDecompressionStream("gzip"), and untars into the workspace: ustar + GNU longname + pax path handling, symlinks preserved, archive root stripped, one entry buffered at a time. Mode bits dropped (no shell, nothing executes). Hydration now completes in ~2s where the old path ran forever.Also retained: the stage/R2 instrumentation added during diagnosis (every hydration stage and R2 op logs start/end), so the next stall identifies itself instead of costing a day.
Verification — the worker is already running this code (deployed ahead of the PR per maintainer instruction, given the bot cannot review anything while down): parser validated in Node against the real tarball with a byte-for-byte path-set comparison vs system tar (3,273 entries, zero missing/extra, symlinks intact), then end-to-end in production — the bot reviewed #2375 successfully at 21:31 UTC on exactly this code. This PR's own review below is the fix reviewing itself.
Closes #
Type of change
Checklist
pnpm typecheckpasses — n/a-with-note: the package's known pre-existing env-dependent baseline; the changed file introduces no new errorspnpm lintpasses — oxlint clean on changed filespnpm testpasses — n/a: package has no unit suite for the workflow; verification was the byte-exact Node parser comparison + the production self-test abovepnpm formathas been runAI-generated code disclosure
Screenshots / test output
Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
fix/flue-review-tarball-hydration. Updated automatically when the playground redeploys.