[deregtd]

User scenario

An agent needs to run tests against a GitHub or GitLab repository in a Linux container. The container needs the complete immutable base commit, but it must not receive the user's Git credentials or unrestricted repository access.

Today the DO can lazily fetch repository objects for individual file reads. That is useful in DO mode, but fetching a whole checkout that way is not the native Git bootstrap a container needs.

What changes

The existing typed gitPull(oids, options) API now returns a GitPullTransfer: a one-shot, backpressured raw PACK stream and an independent terminal status(). GitHub/GitLab gatekeepers fetch and strip protocol framing using credentials they already own. The overseer consumes the stream through its bounded, validating DO cache decoder and checks requested-object presence. The transfer status preserves useful upload-pack errors across Workers RPC, which can otherwise replace stream exceptions with a generic disconnect.

The same capability supports a complete, unfiltered depth-one pack for a native Git consumer without imposing the DO cache's size limit. This PR provides that capability, not the container consumer: container hydration ships separately. It does not itself create a container or synchronize later edits.

Boundary and dependencies

• Provider authentication stays in the provider gatekeeper, including configured Cloudflare Access credentials for self-hosted GitLab. Consumers receive repository bytes, not credentials.
• Gatekeeper.gitPull is kernel-facing, not exposed through gadget sessions. The existing kernel routes pulls from recorded provenance: advertised commits, objects referenced by objects populated by that gatekeeper, or previously populated objects evicted from cache. The shared composer checks oid syntax and composes typed options; it does not validate advertisement history. The authority boundary is the kernel's selection, not a shared advertised-commit validator.
• A native-bootstrap consumer must preserve that boundary by binding its bootstrap capability to an authorized immutable commit, not accepting arbitrary guest-supplied oids, Git commands, ref discovery, or clone URLs. It owns its transfer/storage limits and shallow-boundary recording.
• The shared API adds an optional streaming capability; existing providers need not implement it.
• No container runtime, workspace replication, lifecycle, attachment, or OT changes.
• Targets main independently. The separate replica bootstrap in Synchronize integration worktree edits with canonical DO workspaces #774 is the intended container consumer.

Verification

Before the latest push, 1,463 tests and five affected package typechecks passed. Real workerd RPC regressions cover upload-pack ERR, sideband-fatal, and truncated responses, in addition to provider, cache, and framing regressions. No manual deployment or CI rerun occurred.

Extracted from #729. This PR does not enable or deploy the Linux Container integration.

[github-actions]

Preview: pr769-ddr-split-git-cedb31f5

https://pr769-ddr-split-git-cedb31f5-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

[ask-bonk]

LGTM!

github run

[deregtd]

Reviewed all 13 changed files at 3ba8e8b against 7b0ac75, including exact-commit framing, repository/project scoping, credential refresh/revocation and streaming transport. No actionable findings.

Validation: 13 kit tests (including an actual native Git shallow-fetch fixture), 4 GitHub Workers tests and 16 GitLab Workers tests passed. The current required Build and test, Lint and CLAssistant checks pass. Provider requests were mocked in the Workers tests; I did not exercise live GitHub or GitLab accounts.

[ask-bonk]

LGTM!

github run

[Maximo-Guk]

🤖 GPT bot comment

P2 Preserve upload-pack errors across Workers RPC — packages/gatekeeper-kit/src/git-transport.ts:370 (packages/gatekeeper-kit/src/git-transport.ts#L370)
Returning the demuxed stream directly removes the previous mechanism that preserved protocol failures across RPC. Workers RPC replaces stream exceptions with a generic disconnect: I reproduced an HTTP 200 response containing ERR upload-pack: not our ref …, and the consumer received only “Network connection lost.”
Consequently, unavailable commits and server-side fatal errors lose their actionable cause in the kernel’s pull error and logs. The previous implementation explicitly preserved these failures, and its regression test was removed. Retain an RPC-visible terminal status/error channel alongside the stream.

[kentonv]

🤖 Written by an AI agent:

[low · bug] Upstream git fetch errors are lost across RPC; pull failures now show a generic stream disconnect

packages/workshop-backend/src/overseer.ts:4705

Requested by: 🤖 AI summary agent (noticed while summarizing the PR)

Before this PR, pullGitObjectsIntoCache passed an onFailure callback to demuxGitFetchResponse and rethrew that error when cache.consumePack() failed. Its comment explained why: "A failed pack stream reaches consumePack() across RPC only as a premature disconnect". A test also covered this ("reports why the fetch failed, not how consumePack saw it end").

Now the demuxed stream is returned from the gatekeeper facet over RPC and read in the overseer. If upstream sends ERR upload-pack: not our ref …, a band-3 fatal: …, or a truncated response, the gatekeeper-side stream errors with a useful message. But consumePackFromGatekeeper only sees something like "ReadableStream disconnected prematurely". That generic text then becomes the Last error: in ensureGitObjects's "every connection that could provide it failed" message shown to the user, and appears in the git.pull.source.failed log. The onFailure hook and its test were removed with no replacement.

The failure still fails closed (nothing is silently accepted), so this only hurts diagnostics.

Suggested fix: have the gatekeeper log the demux failure on its side, or validate the pack's framing before returning (e.g. buffer until the packfile section starts so an ERR pkt makes gitPull() itself reject). Mid-stream band-3 errors would still need gatekeeper-side logging.

[kentonv]

The agent-written bug report above seems worth fixing if it's easy but it's not clear to me whether it's easy.

Otherwise the change seems fine although in review I noticed some problems with our streaming pack decoding -- not introduced in this PR, so no need to fix here, but worth noting:

• We apparently still enforce MAX_GIT_PACK_BYTES even though we are streaming now. We shouldn't need to limit anymore.
• Apparently any blob in the pack that exceeds the 8MB storage limit is held in memory instead, just in case a later file uses it as a reference. That seems... kinda backwards? Such later files are probably also over the storage limit, maybe they should all just be dropped? Or perhaps we should just implement chunking and solve the problem entirely...

[ask-bonk]

LGTM!

github run

[clanky-the-clanker]

🤖 AI code review

💬 Approved with comments — No blocking or warning findings. One INFO note: the raw upload-pack body cap was removed on the cache-ingestion path, and the DO decoder's pack-size limit does not count the non-pack bytes it discards.

Findings: 1 info

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #769.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## INFO (1)

### [Code Quality] The cache-ingestion path lost its raw upload-pack transfer cap (`pullGitPack` now passes `Infinity`). Band-2 progress and skipped sections are not counted by the decoder's `maxPackSize`. `maxBytes` is now unused outside tests. Consider passing a framing cap on the overseer path or removing the parameter.
- **File:** packages/gatekeeper-kit/src/git-transport.ts:384
- **Issue:** The cache-ingestion path lost its raw upload-pack transfer cap (`pullGitPack` now passes `Infinity`). Band-2 progress and skipped sections are not counted by the decoder's `maxPackSize`. `maxBytes` is now unused outside tests. Consider passing a framing cap on the overseer path or removing the parameter.

---

📋 Code Quality: 💬 1 comment

The refactor is sound. One low-impact note about the raw transfer cap removed from the cache-ingestion path.

Recommendation: 💬 Optional Nits

• [INFO] The cache-ingestion path lost its raw upload-pack transfer cap (pullGitPack now passes Infinity). Band-2 progress and skipped sections are not counted by the decoder's maxPackSize. maxBytes is now unused outside tests. Consider passing a framing cap on the overseer path or removing the parameter. (packages/gatekeeper-kit/src/git-transport.ts:384)

View full review

Raw transfer cap removed from the cache path (packages/gatekeeper-kit/src/git-transport.ts, pullGitPack / demuxGitFetchResponse)

• pullGitPack now calls demuxGitFetchResponse(response.body, Infinity, onTerminal), and MAX_GIT_FETCH_BYTES has been deleted. The maxBytes parameter is still there, but no production code passes it.
• The DO cache decoder still caps pack bytes with maxPackSize in consumePackFromGatekeeper. That cap counts only band-1 pack data.
• Band-2 progress frames and skipped pre-packfile sections are dropped without being counted. A misbehaving upstream, such as a self-hosted GitLab instance, could therefore stream unbounded non-pack bytes to the overseer without hitting any limit.
• The upstream is authenticated, so the impact is low.
• Fix options: pass a generous framing cap on the overseer-bound path (native consumers stay unbounded), or remove the unused parameter.

Checks that found nothing

• Status ordering: the overseer checks status() after a drain to EOF. That is safe because the demux stream reports complete before it closes.
• Cancel/dispose: the first terminal state wins, so a late failure after cancel cannot overwrite canceled. An unclaimed stream is cancelled when the transfer is disposed.
• Provenance: provenance still comes from consumePackFromGatekeeper(gatekeeperId, ...).
• Tests: coverage is thorough. It includes real-workerd RPC error propagation (ERR, sideband-fatal, truncated responses) and the filtered-blob carve-out.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

No exploitable issues found in the changed code.

• Credentials: provider credentials stay in each gatekeeper's upload-pack fetch closure. Only a PACK stream and a status capability cross RPC.
• Authority: gitPull is still kernel-facing. Provenance is still attributed through consumePackFromGatekeeper(gatekeeperId, ...).
• DO cache bounds: the DO cache keeps its pack and object size limits through the decoder.
• Transfer lifecycle: the one-shot stream claim and cancel-on-dispose are sound.
• Status messages: status.message carries upload-pack error text, which contains no secrets.

---

📖 AGENTS.md: ✅ LGTM

No AGENTS.md concerns.

---

⚡ Performance: ✅ LGTM

No performance concerns. The pack stream applies backpressure (highWaterMark: 0) and is never buffered.

---

📝 Documentation: ✅ LGTM

The README, USAGE and plans updates match the new GitPullTransfer contract. The new exported members of workshop-shared all have doc comments.

---

📘 How the review decision works

• [BLOCKING] findings (dangerous, exploitable or outage-risk problems) make the decision "significant concerns".
• [WARNING] findings: any with production impact, or 4+ that each state a correctness, reliability, security or production impact → "minor issues"; otherwise → "approved with comments". Docs wording, style, test-typing, observability and process warnings don't count toward the 4.
• [INFO], [SUGGESTION], [QUESTION] never count against the change.
• Findings in test files and justified exceptions (e.g. // intentional:) are rated lower.
• The decision is advisory: it never approves, blocks or requests changes on this pull request.

_{Reviewed commit: cd8fb41d}

[ask-bonk]

LGTM!

github run

[ask-bonk]

LGTM!

github run

[github-actions]

Eval results

Verdict: ⚪ Unchanged. No task moved beyond what 10 runs can tell apart from noise.

Task	Score	Δ score	Fisher test	Cache hits	Avg min	Avg steps
change-calendar	100% → 90%	−10 pp	p = 1.00	95% → 96% +1 pp	3.7 → 3.6	26.7 → 30.6
chess	90% → 100%	+10 pp	p = 1.00	98% → 97% −1 pp	8.5 → 9.0	53.6 → 54.6
incident-desk	100% → 90%	−10 pp	p = 1.00	96% 0 pp	4.1 → 4.8	32.6 → 34.4
worker-logs	100% → 90%	−10 pp	p = 1.00	96% → 94% −2 pp	5.4 → 3.9	27.9 → 19.6

Failed checks

Task	Check	Failed
change-calendar	t4 names-the-booked-windows-the-new-rules-reject	0/10 → 1/10
chess	t1 starts-from-the-standard-position	1/10 → 0/10
chess	t1 agrees-with-the-oracle-on-the-hard-positions	1/10 → 0/10
chess	t1 agrees-with-the-oracle-on-perft-positions	1/10 → 0/10
chess	t1 agrees-with-the-oracle-through-random-games	1/10 → 0/10
chess	t1 the-game-is-shared-across-connections	1/10 → 0/10
incident-desk	t1 opens-acknowledges-and-resolves-in-order	0/10 → 1/10
incident-desk	t1 simultaneous-acknowledges-yield-exactly-one-owner	0/10 → 1/10
incident-desk	t1 simultaneous-opens-of-one-id-admit-exactly-one	0/10 → 1/10
worker-logs	t1 hourly-buckets-cover-every-hour-including-empty-ones	0/10 → 1/10
worker-logs	t1 ranges-are-half-open-and-filter-by-worker	0/10 → 1/10

Run · trajectories and raw results

[ask-bonk]

LGTM!

github run

[ask-bonk]

🔬 Eval runs review

Performance

With 10 comparable runs per task, change-calendar, incident-desk and worker-logs each went from 100% to 90%, while chess went from 90% to 100%; comparison.json calls these pass-rate changes unchanged, with p=1 throughout. Approximate mean costs moved main → PR: calendar $0.022 → $0.025, incident desk $0.023 → $0.027, worker logs $0.023 → $0.017, and chess $0.051 → $0.052; worker logs also fell from 27.9 to 19.6 steps and 5.4 to 3.9 minutes, partly because one run stopped after turn 1. Chess remained the largest step/cost sink, including PGN escaping-repair detours, while worker logs alone showed significant cache deterioration: hits 95.62% → 94.40%, breaks 0.0108% → 0.0127%.

⚪ VERDICT: NO REGRESSION FROM THIS PR

The diff changes Git PACK transfer and ingestion, which neither side’s trajectories exercises through remote Git pulls, so the isolated model mistakes and mixed performance shifts do not establish a regression caused by this PR.

Triage

Failure modes

• Incorrect spacing diagnosis · change-calendar 1/10 · model error · this PR: no — Trial 8, turn 4’s executeCode retrieved the correct windows, but the final reply incorrectly added mw-101 and mw-103 as TOO_CLOSE despite their 45-hour gap. Main passed 10/10; its trial 8 computed the rejection list programmatically.
• Successful inserts reported as duplicates · incident-desk 1/10 · model error · this PR: no — Trial 3, turn 1’s writeFile(server.js) treated any rowsWritten !== 1 as DUPLICATE_ID. The checks read back successfully inserted incidents, but every open reported failure, preventing the acknowledgement race checks from proceeding; this was not evidence of double ownership.
• Malformed hourly timestamps · worker-logs 1/10 · model error · this PR: no — Trial 8, turn 1’s writeFile(server.js) replaced the seconds/milliseconds suffix with :00:00Z, producing timestamps such as 2027-03-09T14:00:00:00Z. The verifier’s timestamp normalization then threw Invalid time value; main’s passing implementations returned valid ISO strings.

Tool errors

• editFile: No matching text was found in the file · change-calendar 0 → 1, chess 7 → 5, incident-desk 1 → 1, worker-logs 2 → 0 · model error — Agents supplied overescaped or mismatched search text. Main chess trial 6 corrected doubled regex backslashes after grep; candidate chess trial 7 similarly retried newline escaping.
• editFile: Validation failed · change-calendar 2 → 5, chess 2 → 0, incident-desk 3 → 3, worker-logs 0 → 1 · model error — Calls omitted required fields or used replacement code as a property name. Candidate calendar trial 4 omitted replacement; incident-desk trial 8 repeatedly omitted filename, then corrected it.
• editFile: Multiple matches were found · chess 1 → 1, incident-desk 1 → 8 · model error — Search strings lacked unique context despite the parameter’s explicit requirement. Candidate incident-desk trial 1, turn 1 issued three identical this.incident(id) replacements after grep had shown three occurrences, then recovered with surrounding context.
• readFile: File does not exist · change-calendar 0 → 2, incident-desk 0 → 4 · model error — Agents tried reading server.js and client.js immediately after creating empty gadgets, although the existing prompt and tool description explain that non-blueprint gadgets have no files.
• executeCode: This step already made code changes · worker-logs 0 → 1 · tool design — Trial 10, turn 3 batched a README edit with execution. The unchanged tool description omits this scheduling restriction; the error explained recovery, and execution succeeded in the next step.

Prompt cache

• worker-logs · cache hits 95.62% → 94.40% · breaks 0.0108% → 0.0127% · this PR: no — Hit p=0.043 and break p=0.029, but no prefix-reset step stands out: both sides start with 7,220 cache-read tokens and retain cached prefixes across turns. At turn 2, main trial 1 reads 15,845 tokens and writes 851; candidate trial 4 reads 13,972 and writes 1,760 as new conversation content arrives, with only three uncached tokens in each. The shorter candidate trajectories provide less reuse, not evidence that this Git-only diff introduced cache misses.

What to do

• Optional: add packages/workshop-evals/evals/git-bootstrap.eval.ts with mocked upload-pack responses and an authorized worktree mount/read, covering the changed stream/status path rather than relying on these unrelated gadget-building tasks.
• Optional: in packages/workshop-backend/src/agent.ts, add to EXECUTE_CODE_TOOL_DESCRIPTION: “Do not batch file edits and executeCode in one step; execute the code in your next step.”

github run