Skip to content

Bench: bench pull pulls each task's image by pinned digest and puts its base tree, from the image, in the held-out store (#748 slice 4) [PROTECTED: owner merges] - #947

Merged
jayden1711 merged 8 commits into
mainfrom
bench-slice4-pull
Oct 21, 2026
Merged

jayden1711 merged 8 commits into
mainfrom
bench-slice4-pull

Conversation

@jayden1711

@jayden1711 jayden1711 commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner

Intent: the rest of slice 4 of #748, now based on main (it was stacked on #929, which has merged). bench pull --tasks smoke|baseline pulls that task set's images by their pinned digests, never a tag, and reads each digest back. It then puts each task's base tree in the held-out store: a bare remote with one commit (no upstream history) and a clone of it. The tree is copied out of the task's own image's /testbed through the bench's container runner, with no upstream fetch. The README gets step 10 and an ordered list of the owner's steps; a test holds each block equal to its setup step.

Risk class: protected. It touches scripts/** (scripts/bench/, L3 per #748: the owner merges). It changes no src/ file.

Part of #748. #929 has merged; the base is main now, and main is merged in (d2a549f).

Round 5 (head 1f30402): what changed for the round-4 delta

  • CI fix: CI was red because of this PR (shard 3/4, Node 22 and 24; measured by the evaluator in CI). The no-wall-clock-durations guard (Clocks: dashboard listing caches use a monotonic injectable clock, so a wall-clock step cannot keep or cut a cache window; lint new Date.now() durations in tests (v0.5.3) #582) flagged Date.now() durations in test/bench-pull-budget.test.ts and test/bench-pull-pack.test.ts. Both now use performance.now(). The guard's ALLOWED list is unchanged.
  • Walk time, corrected:
    • README step 10 no longer says "at most about 3 s".
    • It now says the walk reads at most a million entries and inflates at most 1 GiB, and gives the longest measured runs: 3.5 to 4.6 s for a 900,000-link chain of tiny deltas, a 14.4 MB pack (measured in review), and 2.63 s for a million tiny objects (measured).
    • A bound in seconds for every shape is inferred.
  • Non-blocking, done: the packDeclaredBytes doc comment and README step 10 now say that index-pack's memory follows the budget, but its time doesn't. 19 chained 50 MiB deltas, a 51 KB pack, took it 18.6 s at 106,152 KiB, about 109 MB (measured in review). Its time is bounded by the 30-minute git timeout.
  • origin/main 9f9533a is merged in (be1167c, signed off). It merged without conflicts.
  • Verification (measured):
    • the 9 bench test files plus no-wall-clock-durations: # tests 100, # pass 100;
    • the round-7 scan: 1/1;
    • typecheck clean; denylist 1421 files clean; DCO all signed off.
  • Fail-without: the guard itself, red in CI at 410ef53 (measured in CI by the evaluator). I didn't re-run it locally.

Round 4 (head 410ef53): what changed for the third CHANGES NEEDED verdict

The round-1 to round-3 builder wasn't reachable, so I continued its work. This round is one commit, 410ef53.

  1. One budget for the walk, charged before anything is inflated (by design).
    • packDeclaredBytes charges TREE_CAPS.treeBytes (1 GiB) with:
      • 1 KiB an entry (ENTRY_COST). The whole count is charged before any entry is read;
      • the bytes each entry's data inflates to, a delta's own data included;
      • each delta's target size.
    • Each charge is made before that data is inflated, so the walk never inflates more than the budget. Its memory, and index-pack's, are bounded by the budget. Corrected in round 5: index-pack's time is not (18.6 s for 19 chained 50 MiB deltas in review); the 30-minute git timeout bounds it.
    • Data over 1 MiB is inflated as a stream and dropped; only its first 32 bytes are kept. One object, or one delta's data, is still capped at 256 MiB.
    • The walk now runs in baseTree, before anything is made in the store. It returns { decoded, spent }.
  2. Host git on one thread. GIT_HARDENED adds -c pack.threads=1 to every host git, and index-pack runs with --threads=1. fetch's own index-pack and pack-objects get pack.threads=1 too (inferred: git passes -c settings to the git processes it starts).
  3. The entry charge comes from my adversarial pass, not the verdict.
    • 18 million distinct 4-byte blobs make a 288,000,032-byte pack (base64 384,000,044 bytes, under the cap).
    • At 92a8de8 the walk took it in 49.66 s, and index-pack --threads=1 peaked at 1,409,888 KB (measured).
    • Its count alone now refuses it, at once (measured).
  4. Optional, done: the walk checks that the trailer is the pack's SHA-1, and that an OFS_DELTA's base starts at or after the first entry and before the delta itself.
  5. bench pull's own peak is stated, not lowered.
    • Just under the copy-out cap it peaked at 1,509 to 1,894 MiB over 6 runs at this head (measured).
    • It is the same for 383 MiB of A, which the walk refuses at once (1,511 and 1,894 MiB), as for the review's well-formed 2×143 MiB pack (1,509, 1,892, 1,891 and 1,892 MiB).
    • So the peak is in the copy-out itself, before the walk: buffers, the joined buffer, the string and the decoded pack. Whether a run reaches about 1.9 GB depends on when V8 collects (inferred).
    • I tried dropping the chunk array before the string is made (container.ts). It made no measurable difference (1,891 and 1,509 MiB with it), so I reverted it. Lowering the peak needs a streamed copy-out (see "What remains").
  6. Claims corrected:
    • README step 10, rewritten with the measured figures;
    • "Compromised component", "Not defended" and the changelog fragment;
    • in the round-3 notes below, "tracks the 1 GiB total cap" and "about 1.5 GB" are marked wrong.
    • The round-3 "255 MiB for four side by side" did not reproduce in review (1,030,716 KB, 4 of 4 runs, default threads). My re-run with four identical deltas gave 261,056 KB with default threads (measured). The figures below are all --threads=1.

Measured (scratch probes, stand-ins only; git 2.43.0, 16 CPUs, Node 22.23.2). First, the review's packs and mine:

Pack At 92a8de8 At 410ef53
The review's dd-1150 (300,070,794 B: 1,150 deltas of 256 MiB zero data), the walk accepted after 302.6 s, 1.2 GB (review) refused in 0.49 s, 398 MiB
The same, through baseTree not done in 600 s (review) refused in 3.0 s; node 1,887 MiB, from the copy-out
The review's copy8-33x31 (12,501,787 B: 33 valid deltas of 248 MiB data each), the walk accepted after 11.5 s (review) refused in 0.35 s, 122 MiB
The same, through baseTree 1,718,312 KB (review) refused in 0.5 s, 154,392 KB
18 million 4-byte blobs (288,000,032 B), the walk accepted after 49.66 s refused in under 0.01 s
The review's 1,115-byte pack-d refused refused in under 0.01 s

Then shapes the walk admits, against what it charges:

Shape Charged (bytes) Walk index-pack --threads=1 peak
a 250 MiB zero base and one 250 MiB delta made only of inserts (252 MiB of data) 788,498,182 0.22 s 773,828 KB (the highest)
that pair plus a random 255 MiB blob: a 268,579,471-byte pack, near both caps 1,055,886,086 0.60 s 773,420 KB
a 200 MiB base and a chain of two such insert deltas 1,051,881,690 0.38 s 619,336 KB
a 1 MiB base and a chain of four 250 MiB deltas 1,049,631,727 < 0.01 s 516,912 KB (1,028,992 KB with default threads)
a 250 MiB base and a chain of three 250 MiB deltas 1,048,664,135 0.19 s 516,044 KB (1,027,552 KB with default threads)
four zero blobs of 255 MiB 1,069,551,616 0.59 s 264,888 KB
a million 4-byte blobs 1,028,000,000 2.63 s (the longest) 81,312 KB
950 zero blobs of 1.001 MiB (the streamed path) 998,116,550 1.06 s 4,992 KB
  • So with one thread, index-pack's highest measured peak is 756 MiB, for 752 MiB charged. On these shapes it stayed near or under the charge, so it is at most about the 1 GiB budget (inferred for other shapes).
  • The walk's longest admitted run here was 2.63 s. Corrected in round 5: a 900,000-link chain of tiny deltas took it 3.5 to 4.6 s in review.
  • bench pull and index-pack together, on the near-both-caps pack: sampled every 0.1 s, at most 1,746 MiB (node 1,050 + git 696) over 2 runs (measured; a short spike could fall between samples).

Round 3 (head 92a8de8): what changed for the second CHANGES NEEDED verdict

  1. index-pack's memory is bounded by design.
    • pull.ts packDeclaredBytes walks the pack before git index-pack sees it. It reads:
      • the signature, version and count;
      • each entry's type and size;
      • each delta's target size, from its delta header;
      • each entry's zlib data, inflated once and never past its declared size, to find where it ends;
      • that nothing but the trailer follows the last entry.
    • It refuses any whole object or delta target over 256 MiB (new cap: TREE_CAPS.objectBytes), a total over 1 GiB, and any pack that isn't well formed. Nothing is kept and the container is removed.
    • The evaluator's 1,115-byte pack (a delta declaring 1536 MiB) is refused in 2 ms before git runs (measured, on the evaluator's own pack-d.pack). It used to make index-pack peak at 1.58 GB.
    • Admitted packs under the caps (measured with crafted packs): index-pack peaked at 255 MiB for one or four side-by-side 250 MiB deltas, and at 1005 MiB for a chain of four (1001 MiB declared). Its peak tracks the 1 GiB total cap. Wrong, corrected in round 4: with git's default threads it grew with the host's CPUs.
    • The check on what git read, after index-pack, stays as a second check.
  2. Claims corrected:
    • in "Compromised component", the memory peaks: bench pull at about 1.5 GB just under the copy-out cap (measured in review; wrong: 1,891 MiB in review, 1,509 to 1,894 MiB in round 4), and the measured index-pack peaks;
    • "Not defended";
    • README step 10, which now says what is checked before index-pack and gives the measured peaks.
    • I did not stream the copy-out. I state its real peak instead: streaming would change Sent and the exec path, and isn't cheap.
  3. Non-blocking, done:
    • "Checks removed or weakened" lists each removed or replaced test assertion;
    • the snapshot(plain) check after the probe is restored;
    • the size table is current;
    • "What remains" has the slice-7 cap line.

Round 2 (head e5dfe1e): what changed for the CHANGES NEEDED verdict

  1. The copy-out is bounded.
    • TaskRunner.exec takes a byte cap (stdout and stderr together). The async docker runner ends the CLI (SIGKILL) at the first byte past it and keeps nothing; the exec throws OutputTooLarge. This is checked after a stand-in call too.
    • A result too long for one string no longer throws inside the close handler. It is refused, so the caller's finally (the container's removal) always runs.
    • bench pull caps:
      • the probe at 64 KiB;
      • the pack at 384 MiB of base64;
      • what the pack decodes to at 1 GiB (cat-file --batch-all-objects sizes, summed), checked before anything is fetched or checked out.
    • Past a cap the task is refused, nothing is kept and the container is removed (measured, tests). Both caps are guesses at what Verified's largest tree needs; a tree over one is refused, naming the cap.
  2. Content mode is gone: an image with no /testbed/.git is refused (FAIL at that key, nothing copied out, container removed; measured). A kept tree's record must be the git, HEAD-checked kind; an older content record is refused (measured).
  3. The journal.
    • benchDenied now has /var/log/journal and /run/log/journal (measured, test). A tightening.
    • The README's log hint now says to read the journal on screen only, and never to save or copy its output anywhere.
    • What dockerd logs about a pull is a guess: no stand-in can measure what a real dockerd writes.
  4. "Compromised component" is corrected (below).
  5. Non-blocking, done: baseTree refuses a task image that isn't its own pinned digest reference (<name>@sha256:<64 hex> from pins.ts) before docker run. I didn't change container.ts's IMAGE check itself: test/bench.test.ts fixtures run swebench/example:latest, and the driver's own path takes images only from loadTasks (inferred).
  6. Non-blocking, done: the README says a submodule gitlink becomes an empty folder.

The bench pull tests now share their fixtures (test/bench-pull-fixtures.ts). The content-mode PASS test is replaced by the refusal test.

What remains (proposals, not in this PR)

  • A content-hash fallback for an image with no /testbed/.git: a loosening, for the owner to decide. It would bind nothing to base_commit.
  • The digest-collection gap (below), unchanged.
  • container.ts IMAGE requiring a digest for every run: that needs the driver fixtures in test/bench.test.ts changed to digest references. Slice 7 (bench run) is the natural place.
  • caps.ts capsFromArgs defaulting --tasks to smoke, while bench pull requires it: settle when bench run lands.
  • Slice 7: cap bench run's commands. Give every TaskRunner.exec that bench run's task commands make (the test relay, syncTree) a byte cap. Uncapped output is still buffered whole: 600 MiB peaked at 1,273 MiB (measured in review).
  • Streaming the copy-out instead of buffering it, to lower bench pull's peak of 1,509 to 1,894 MiB just under the cap (measured, round 4).

What changed

  • scripts/bench/pull.ts (new).

    • pullTasks: the tasks come from loadTasks (store only). Each image goes through pullImage, which sends only docker pull <name>@sha256:<digest> and then image inspect to read the digest back. Then loadTasks runs again, and the command refuses if images.json changed during the pulls. Refusals name the key only.
    • baseTree: one throwaway container per task, through Docker.start (run --network none --cap-drop ALL, no mount, limits read back) and TaskRunner.exec (docker exec only).
      • Git metadata in /testbed: HEAD must be base_commit. The pack holds base_commit's own commit object (none of its parents) and exactly its tree's objects. On the host, that commit must hash to base_commit and name the tree, so the tree is bound by hash, not by the container's word.
      • No git metadata: refused (round 2).
      • Host build: index-pack (which hashes every object), one root commit with a fixed author and date, and a fresh bare remote that fetches only that commit. That remote must hold exactly the reachable objects (count checked) and pass fsck --connectivity-only. Then a clone with origin set to the bare remote, and a 0600 tree.json. Everything is built in a temporary folder and renamed to trees/<key>/, never over anything.
      • Rerun: a tree already there is kept only when it is the same task from the same image and still checks out. Anything else is refused and left as it is.
    • Host git runs with no config of its own (GIT_CONFIG_GLOBAL=/dev/null, NOSYSTEM), with hooks off, fsmonitor off, and protocol.allow=never except file.
  • scripts/bench/download.ts: pullImage now throws PullRefused (stage pull or read back, plus the exit status). Its messages are unchanged. This lets bench pull report a failure without docker's stderr, which names the image.

  • scripts/bench/cli.ts:

    • bench pull takes exactly --tasks smoke or --tasks baseline. Anything else, including no flag, exits 2 before any docker call.
    • The usage text, the smoke plan's text and the bench images "next" line are updated.
  • scripts/bench/README.md:

    • New step 10.
    • New section "The owner's steps, in order", each step labelled done, measured or inferred.
    • Step 7 names an open gap (below).
    • Step 5 can now be redone: it moves the old clone aside under a fresh name, since a fixed .old breaks the second time. It is redone after every pins.json change, because the bench reads its pins from that clone.
    • The data layout, the smoke plan and "What is here" are updated.
  • changelog.d/947-bench-pull-base-trees.md: the release note, under "Guardrail changes: what each one does". The changelog.d backstop in test/changelog.test.ts passes 1/1 and test/changelog-fragments-selfcheck.test.ts passes 2/2 (measured). I didn't run test/changelog-fragments.test.ts locally: it spawns release.mjs with the host PATH, so CI runs it.

Design choices (inferred; no new loosening)

  • --tasks is required, with no default. The brief said to default to smoke only if the README's smoke plan says so. The plan writes bench run --tasks smoke with the flag, so I failed closed. Note: caps.ts capsFromArgs (for bench run, still to come) defaults --tasks to smoke. That's a mismatch to settle when bench run lands; this PR does not change caps.ts.
  • The trees live in the held-out store (trees/<key>/), not under runs/. The store is denied to the agents' sandbox, and 50 base trees would reveal the task set. bench run (still to come) copies one task's tree into that task's folder.
  • The base tree is base_commit's committed tree, not /testbed's work tree. An image's setup may edit tracked files; the stand-in /testbed test covers that case.

Open gap found while writing the owner's steps (not built here; needs a decision)

No command collects the image digests. pins.json image_digests needs at least 100 digests, including every selected task's, and nothing in the repo produces them. Typing image names would put instance ids into the bench user's shell history. So bench images and bench pull cannot run on the real machine until this is decided. One option is a bench digests command that runs the bench images registry lookups over every row of the pinned dataset and prints only the sorted set. The README now says this in step 7 and in the owner's step I.

How it was verified

Round 4 (head 410ef53)

  • Fail-without (measured):
    • I wrote test/bench-pull-budget.test.ts with the Write tool as a NEW file in a fresh worktree at 92a8de8. It is byte-identical to the head's (sha256 eff52ce0…).
    • There, node --import ./dist/test/isolate.js --test dist/test/bench-pull-budget.test.js (temp HOME, TMPDIR and state, stand-ins first on PATH) gave # tests 6, # pass 1, # fail 5:
      • many valid large deltas: Missing expected rejection. The round-3 walk inflated about 8 GiB in 8.6 s and took the pack;
      • many tiny entries: Missing expected rejection.;
      • the trailer and the offset: Missing expected rejection: a wrong trailer;
      • one thread: index-pack on one thread. The argv ended … index-pack --stdin;
      • the README: no pack.threads=1.
    • The one that passes there is the streamed path's size checks. I kept it as coverage: round 3's one-piece inflate held the same sizes.
  • After (measured):
    • bench-pull-budget, bench-pull-pack, bench-pull, bench-pull-bounds, bench-images, bench-select, bench-select-lookalike, bench-cli and bench-pins: # tests 98, # pass 98;
    • the round-7 scan in bench.test.ts, by name: 1/1;
    • changelog.test.ts and changelog-fragments-selfcheck.test.ts: 29/29;
    • merge-policy weakening backstop: 2/2;
    • typecheck clean; denylist 1415 files clean; DCO all signed off.
  • Against main (measured): git merge-tree --write-tree origin/main HEAD against main 9f9533a is clean (tree b0f6d339). I didn't merge main in; the brief didn't ask for it.
  • Not run locally: the rest of test/bench.test.ts and the full suite. CI runs both.

Round 3 (head 92a8de8)

  • Fail-without (measured): I wrote test/bench-pull-pack.test.ts first, as a NEW file in a fresh worktree at e5dfe1e, byte-identical to the head's (sha256 921b6559…). There, node --import ./dist/test/isolate.js --test dist/test/bench-pull-pack.test.js gave # tests 5, # fail 5:
    • the walk tests (3): typeof packDeclaredBytes was 'undefined';
    • the delta bomb through bench pull: no "declares … before it is indexed" refusal;
    • the README: no "before index-pack".
  • After (measured):
    • bench-pull-pack, bench-pull, bench-pull-bounds, bench-images, bench-select, bench-select-lookalike, bench-cli and bench-pins: # tests 92, # pass 92;
    • by name in bench.test.ts: the round-7 scan and six container tests, 7/7;
    • merge-policy weakening backstop: 2/2;
    • changelog.d backstop: 1/1;
    • fragments selfcheck: 2/2;
    • typecheck clean; denylist 1414 files clean; DCO all signed off.
  • Scratch measurements (measured):
    • /usr/bin/time -f %M git index-pack --stdin on crafted packs: 255 MiB for 1×250 MiB, 255 MiB for 4×250 MiB side by side (not reproduced in review: 1,030,716 KB), 1005 MiB for a chain of 4×250 MiB;
    • the walk on each: 1 to 2 ms;
    • the walk on the evaluator's pack-d.pack: refused in 2 ms.

Round 2 (head e5dfe1e)

  • Fail-without (measured): I wrote test/bench-pull-bounds.test.ts and test/bench-pull-fixtures.ts first, as NEW files in a fresh worktree at 54894eb. Both are byte-identical to this head's (sha256 e1b24dfe… and 860faae6…). There, node --import ./dist/test/isolate.js --test dist/test/bench-pull-bounds.test.js gave # tests 7, # pass 0, # fail 7, each on behaviour, not on a missing module:
    • the copy-out cap: exit 0, not 1;
    • TaskRunner.exec: missing expected rejection;
    • the tree cap: exit 0;
    • no .git: exit 0;
    • a tag image: missing expected rejection;
    • the journal: false;
    • the README: no /var/log/journal.
  • After (measured):
    • bench-pull, bench-pull-bounds, bench-images, bench-select, bench-select-lookalike, bench-cli and bench-pins: # tests 87, # pass 87;
    • six bench.test.ts container tests by name (the task runner sends docker exec…, limits, rootless, daemon, Read denials): 6/6;
    • round-7 scan: 1/1;
    • merge-policy weakening backstop: 2/2;
    • changelog.d backstop: 1/1;
    • fragments selfcheck: 2/2;
    • typecheck clean; denylist 1413 files clean; DCO all signed off.
  • The real async path at the evaluator's sizes (measured). A scratch script ran TaskRunner.exec against a stand-in docker script that prints N zero bytes:
    • 600 MiB with the 384 MiB cap: rejected, "went over 402653184 bytes"; 969 ms; peak RSS growth 386 MiB;
    • 256 MiB with the cap: resolved whole;
    • 600 MiB with no cap: rejected, "output was too long to read". No crash: before this, the same case threw ERR_STRING_TOO_LONG.

Round 1

  • Fail-without (measured): I wrote test/bench-pull.test.ts first, then ran it at Bench: bench images writes each task's pinned image reference from the store, never typed (#748 slice 4) [PROTECTED: owner merges] #929's head 49b01df. That ran in a fresh worktree with only that new file added, byte-identical to this PR's (same sha256 763170ef…). Result: # tests 17, # pass 0, # fail 17. Of those, 14 fail on pull is not a function, 1 on Cannot find module …/dist/scripts/bench/pull.js, 1 on the help text (no pull command), and 1 on the README has the owner steps.
  • After (measured): node --import ./dist/test/isolate.js --test dist/test/bench-pull.test.js dist/test/bench-images.test.js dist/test/bench-select.test.js dist/test/bench-select-lookalike.test.js dist/test/bench-cli.test.js dist/test/bench-pins.test.js gave # tests 81, # pass 81, # fail 0.
  • Also measured:
    • the bench source scan (--test-name-pattern="round 7: no raw format" on dist/test/bench.test.js): 1/1 pass;
    • merge-policy.test.ts "weakening backstop": 2/2 pass;
    • npm run typecheck: clean;
    • npm run check:denylist: 1400 files clean;
    • node scripts/check-dco.mjs origin/bench-slice4-images HEAD: all signed off.
  • Not run locally: the rest of test/bench.test.ts and the full suite. CI runs both.
  • Stand-ins only (measured by reading the tests):
    • a temp bench root, synthetic ids (example__proj-NNNN) and a stand-in docker call. No daemon, image, network, model or real store;
    • host git and sh run only on temp fixture repos, with no config of their own;
    • the in-container scripts run with sh on a stand-in /testbed. A snapshot of every path's size and mtime shows they change nothing there.

New dependencies

None.

Package size

0 bytes of delta: measured at 49b01df in round 1, inferred since.

  • At head 410ef53, npm pack --dry-run --json gives size 1481905, unpackedSize 4963126, entryCount 493, the same as at 92a8de8 (measured).
  • No packed path is under dist/scripts, scripts/bench, test/ or changelog.d (count 0, measured).
  • Every file this PR changes is under those paths, so main d2a549f packs the same bytes (inferred).

Per file, in lines (measured, git diff --numstat origin/main...HEAD at 410ef53):

File Lines Packed
scripts/bench/pull.ts +562 no
scripts/bench/README.md +161/-10 no
scripts/bench/cli.ts +81/-4 no
scripts/bench/container.ts +63/-15 no
scripts/bench/download.ts +17/-2 no
scripts/bench/agent.ts +6 no
changelog.d/947-bench-pull-base-trees.md +8 no
test/bench-pull.test.ts +460 no
test/bench-pull-fixtures.ts +222 no
test/bench-pull-pack.test.ts +161 no
test/bench-pull-budget.test.ts +212 no
test/bench-pull-bounds.test.ts +125 no

What this can never do

  • Pull by tag. Only a reference that pins.ts pinnedImage built reaches pullImage, and its argv check refuses anything but pull <name>@sha256:<64 hex>. Every pulled argv is asserted in the tests (measured).
  • Keep an image whose digest doesn't read back as the pinned one. Tested: another pinned digest, an unpinned one, and the right digest under another task's name (measured).
  • Fetch upstream.
    • No git fetch, clone or network goes to anything but a local path: protocol.allow=never, file only.
    • The source comes from the image, inside a --network none container (measured: argv; inferred: no route in a real container).
  • Keep upstream history.
    • The bare remote takes only the new root commit by fetch, and must hold exactly its reachable objects (measured: a full-history pack leaves no parent, old blob or upstream commit in the remote or the clone).
  • Trust the container's word for the base tree (git mode). base_commit's commit object must hash to base_commit and name the tree (measured: a pack with the right tree but no commit is refused).
  • Write over anything in the store. A temporary folder is renamed onto an empty folder made exclusively just before. A link, another task's tree or another image's tree is refused and left as it is (measured).
  • Print a task. Messages are built from keys. docker's stderr is never passed on. The test asserts that raw output, before the withholding backstop, has no __, _1776_, sweb.eval or [task withheld] (measured across 7 scenarios).
  • Show the agents the task set. The trees live in the held-out store, which the agents' sandbox denies (inferred: unchanged slice-1 deny list).

Compromised component

  • An agent in a bench run (sandboxed, as the bench user):
    • it can read this code, which holds no task. It can't read the store, where the trees and tree.json live, and it has no docker (no socket, no DOCKER_*);
    • bench pull runs before any agent run (inferred: no driver code calls it yet).
  • A malicious or wrong task image (pinned by digest):
    • it controls /testbed and the container's git, and could lie about HEAD;
    • in git mode the host binds the tree to base_commit by hash, so a lie is refused. Faking it would need a SHA-1 collision on a chosen commit (inferred);
    • with no .git it is refused (round 2; measured);
    • a huge copy-out: as of 54894eb there was no bound. The evaluator measured 256 MiB taken whole and 600 MiB crashing the process with the container left running, and a 261 KB pack becoming a 256 MiB file in the clone.
      • Now the copy-out is capped as it arrives: the CLI is ended, nothing is kept and the container is removed.
      • bench pull's own memory (measured in review):
        • over the cap: about the cap (459 MiB peak for 600 MiB);
        • just under the cap: 1,509 to 1,894 MiB over 6 runs (measured, round 4; the review measured 1,891 MiB in round 3). The output is held as buffers, a joined buffer, a string and the decoded pack at once. It is not streamed. The peak is the same when the walk refuses the pack at once, so it is in the copy-out, before the walk and before git.
      • The pack is walked before git index-pack sees it, against one budget (rounds 3 and 4).
        • Each entry is charged 1 KiB, the bytes its data inflates to (a delta's own data too) and, for a delta, its target. The total is capped at 1 GiB, charged before the data is inflated. One object, or one delta's data, is capped at 256 MiB.
        • This bounds by design what round 2 measured: a 1,115-byte delta pack made index-pack peak at 1.58 GB. That pack is now refused before git runs (measured, on the evaluator's own pack file and in a test).
        • It also bounds what round 3's review measured: a 300 MB pack of delta data took the walk 302.6 s, and a 12.5 MB pack of 33 valid deltas made index-pack peak at up to 1.96 GB. Both are now refused in under 0.5 s (measured, round 4).
        • What an admitted pack can still cost (measured with crafted packs, round 4; git 2.43.0, 16 CPUs):
          • host git runs on one thread (pack.threads=1, index-pack --threads=1);
          • index-pack's highest peak was 773,828 KB, for a delta the walk charged 752 MiB. At most about the 1 GiB budget (inferred for other shapes);
          • the walk's longest measured runs: 3.5 to 4.6 s for a 900,000-link chain of tiny OFS deltas, a 14.4 MB pack (measured in review), and 3.1 to 5.8 s for the same chain on a loaded host (measured in the round-5 check); 2.63 s for a million tiny blobs (measured). The walk reads at most a million entries and inflates at most 1 GiB; any bound in seconds is inferred;
          • index-pack's CPU time is bounded by the 30-minute git timeout;
          • disk: the pack (at most 288 MiB), then the tree, at most 1 GiB decoded.
        • Host worst case: about 1.9 GB, in bench pull's copy-out, before git runs (measured). bench pull and index-pack together, on a pack near both caps, sampled at most 1,746 MiB (measured).
    • the daemon's log: the image names it pulls may land in the system journal (a guess). The agents' sandbox now denies /var/log/journal and /run/log/journal;
    • it could send extra objects; they are dropped by the fetch-only-reachable step (measured).
  • The registry: it is reached only through docker pull by digest, and the digest is read back (as in Bench: pinned dataset and image downloads, verified before use (#748 slice 2) [PROTECTED: owner merges] #863).
  • Someone with write access to the store (the bench user outside the sandbox): they could already edit anything there. A kept tree is re-verified against its own record and repo, which catches mistakes, not this attacker.
  • Someone with push access to this repo: they could change the scripts. That is a reviewed scripts/** change, and the owner merges.
  • The coordinator, the dashboard, other instances, the hub viewer: none of them imports scripts/bench (inferred: nothing under src/ does).

Adversarial pass

Each attack runs as a fixture test, and each result is measured.

Attack Result
bench pull, --tasks, --tasks all, --tasks constructor, --tasks=smoke, an extra argument, an instance id, an image name exit 2, no docker call, no __ in the output
images.json missing, an unpinned digest, a tag, another task's image refused before any docker call
Not the bench user; the store at 0755; a rootful daemon refused before any pull
Read-back gives another pinned digest, an unpinned one, or the digest under another name refused at that key; no container started
docker pull fails, with stderr naming the image refused; stderr not printed
images.json rewritten during the pulls (another pinned digest under the same name, which loadTasks alone would take) refused; no tree built
HEAD ≠ base_commit refused; nothing kept; container removed; tree never copied out
Probe exits non-zero, gives bad ids, gives a missing tree id, or answers neither git nor none refused; nothing kept
Pack is another commit's tree; the right tree without the commit; another repo's history; not base64; not a pack; empty; cut short; exec exit 2 refused; nothing kept; no temp folder left
Pack carries the whole upstream history kept as one root commit; no parent, old blob or upstream commit in the remote or the clone
trees/<key> is a link; trees/ itself is a link refused; nothing made through it
Existing tree from another image; a record that disagrees with its repo refused; left as it is (and the good one is still kept)
The real in-container scripts on a stand-in /testbed with a dirty work tree HEAD's committed tree only; no parent; /testbed unchanged
Round 2: a copy-out over its cap (the real pack against a 64-byte cap; 4 MiB against 1 MiB) refused, naming the cap; nothing kept; container removed
Round 2: a stand-in docker binary printing 64 MiB on the real async path, 1 MiB cap rejected; the CLI ended
Round 2: a 4 MiB zero blob (a small pack) against a 1 MiB decoded cap; then against 64 MiB refused before any checkout, nothing kept; taken under the larger cap
Round 2: no /testbed/.git refused; nothing copied out; container removed
Round 2: a kept tree whose record says content mode refused; left as it is
Round 2: task image name:latest, a bare name, or name:latest@digest refused before docker run
Round 2: the lane's deny list holds /var/log/journal and /run/log/journal
Round 3: a delta bomb of a few KB declaring 300 MiB, through bench pull refused before it is indexed; nothing kept; container removed
Round 3: the evaluator's 1,115-byte pack (a delta declaring 1536 MiB) refused by the walk in 2 ms, before git
Round 3: a total one byte over treeBytes; one object over objectBytes; a delta whose target is over (its own data is tiny) refused before it is indexed; exactly at the cap is taken
Round 3: empty, no signature, version 4, fewer entries than the count, trailing bytes, type 5, a size that isn't what it inflates to, cut-short or non-zlib data, a cut-short delta header refused as not well formed, before it is indexed
Round 3: the walk on packs git made and on a crafted delta pack the same per-object sizes git reads (cross-checked with cat-file --batch-all-objects)
Round 4: 33 valid deltas, each a 31 MiB target of one-byte copies (248 MiB of its own data), at the real caps refused before it is indexed, in under 1 s (test)
Round 4: the review's dd-1150 and copy8-33x31 packs refused: the walk in 0.49 s and 0.35 s, baseTree in 3.0 s and 0.5 s; container removed, nothing kept
Round 4: 18 million 4-byte blobs; 2,000 against a 1 MiB budget refused at once, on the count (test for the second)
Round 4: admitted shapes meant to be worst: insert-only deltas, chains, side by side, a million tiny blobs, 950 blobs just over 1 MiB, a pack near both caps index-pack --threads=1 at most 773,828 KB; the walk at most 2.63 s
Round 4: over 1 MiB (the streamed path), data that inflates past, short of, or is cut short of its size refused as not well formed (test)
Round 4: a wrong trailer; an OFS base before the first entry, at offset 0, or past the pack start refused (test); a base at the first entry is taken
Round 4: every git that bench pull runs, recorded through a wrapper each has -c pack.threads=1; index-pack has --threads=1 (test)
Round 4: dropping the copy-out's chunks before the string is made no measurable change to the peak; reverted

What I changed because of it:

  • Round 1: at first the host trusted the container's printed tree id for HEAD. I added base_commit's commit object to the pack and the host-side hash check; the "right tree without the commit" case covers it (inferred: I didn't weaken the code to watch it fail). I also added the clone check when a tree is kept, and the trees-folder-is-a-link case.
  • Round 2: after adding the cap, an uncapped exec that overflowed came back as status 0 with empty output, which a caller could read as success (measured with the scratch script). It is now refused too.
  • Round 3: I measured a chain of deltas as well as side-by-side ones. The chain is the shape that makes index-pack's peak track the total (1005 MiB for 1001 MiB declared), so the total cap, not just the per-object cap, is what bounds it.
  • Round 4: the per-entry charge, from the tiny-blob attack (49.66 s and 1.41 GB at 92a8de8). I also reverted a change to container.ts that measured as no change.

Not defended:

  • memory under the caps (measured, round 4): bench pull peaks at up to 1,894 MiB just under the copy-out cap, in the copy-out (buffers, a string and the pack at once). index-pack --threads=1 peaked at most at 773,828 KB on the shapes tried, about what the walk charged (at most about the 1 GiB budget, inferred). Streaming the copy-out, or lower caps, would lower the first;
  • the caps are guesses (inferred): a legitimate tree over them is refused, failing closed;
  • the scripts need git, sh, printf and base64 in the image. A missing tool fails closed;
  • symlinks in base_commit's own tree are kept as links; nothing in this PR follows them. That is a note for slice 7, which copies the tree.

Checks removed or weakened

No check, lint or guard is removed or loosened. These test assertions were removed or replaced, in this PR's own new test file:

  • Round 2, test/bench-pull.test.ts:
    • the test no git metadata in /testbed: its content hash is recorded instead … expected PASS for a content-mode image. It is replaced by a refusal test (test/bench-pull-bounds.test.ts: FAIL, nothing kept, nothing copied out, container removed). That is a tightening, and the round-1 verdict required it;
    • the CONTAINER_SCRIPTS.content assertions in the in-container scripts test were removed: the code they tested is deleted;
    • assert.deepEqual(snapshot(plain), plainBefore) was dropped. Restored in round 3, as a check that the probe leaves a /testbed with no git metadata unchanged;
    • the fixtures moved to test/bench-pull-fixtures.ts with no assertion changed. The stand-in now strips a :tag when it looks up an image name, so it can answer the tag-image test instead of crashing.
  • Round 3: none removed. The new test/bench-pull-pack.test.ts is additive.
  • Round 4: no check, lint or guard is removed or loosened. The walk only refuses more: every charge is at least round 3's, and the new checks only add refusals (inferred from the code). Changes to this PR's own test/bench-pull-pack.test.ts:
    • its walk calls are awaited (the walk is async now) and read .decoded;
    • "exactly at the cap: taken" now sits at the walk's charge (spent) instead of git's decoded total, because the budget now charges more than the decoded total. One byte under is still refused, and decoded is still checked against git's own sizes;
    • the "delta header cut short" case gets a real base blob first. Its old base offset (1) now fails the new offset check before the header is read, so without a base it would no longer reach the header;
    • one assertion message now says "not only its delta data".
    • The total's refusal message changed from "declares objects that decode to over N bytes" to "is too much to inflate and decode: it declares over N bytes …". The round-2 pattern in test/bench-pull-bounds.test.ts (/decode.*over N bytes/) still matches it, unchanged.
  • Round 5: no check, lint or guard is removed or loosened. The guard no-wall-clock-durations and its ALLOWED list are unchanged. Test-line changes in this PR's own files:

Owner action

Review. The owner's one-time steps, from creating wl-bench to bench pull, are in the README section "The owner's steps, in order". Step I (the image digests) is blocked on the decision above.

…ts base tree, from the image, in the store

The rest of slice 4 of #748 (step 4 of the smoke run plan): images.json had no consumer that pulled, and no
task had a base tree, a local clone with no upstream history or a local bare remote.

bench pull --tasks smoke|baseline (the flag is required, as bench run will take it; no default) takes the
tasks and their references from the store only (loadTasks), pulls each through pullImage (docker pull
<name>@sha256:<digest>, never a tag) and reads the digest back; then reads images.json again and stops if it
changed. pullImage now throws PullRefused with its stage, so bench pull can say what failed without docker's
words, which name the image.

Each task's base tree comes from its own image's /testbed, through the bench's container runner (docker run
--network none, limits read back, TaskRunner exec only), never from upstream. With git metadata, HEAD must be
base_commit; base_commit's own commit object and exactly its tree's objects are packed out, and on the host the
commit must hash to base_commit and name that tree. With none, /testbed's files are hashed into a throwaway
repo in the container and that tree is recorded as the content hash, saying HEAD was not checked. On the host:
one root commit (fixed author and date) in a fresh bare remote that holds exactly its objects, a clone with that
remote as origin, and a 0600 tree.json, built in a temporary folder and renamed to trees/<key>/ in the held-out
store, never over anything. A tree already there is kept only when it is the same task from the same image and
still checks out. It prints keys and counts only.

The README gets step 10, the owner's steps in order with each block held equal to its setup step by a test,
the open gap of collecting the image digests, and a step 5 that can be redone after each pins change.

Test (fails without): test/bench-pull.test.ts (17 of 17 fail at 49b01df: no pull command, no pull module, no
owner steps in the README).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
… from its image (#947)

The release note for the rest of #748 slice 4, as a fragment (changelog.d), never in CHANGELOG.md.

Test (fails without): none, notes only; the changelog.d backstop in test/changelog.test.ts and
test/changelog-fragments-selfcheck.test.ts pass with it.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Base automatically changed from bench-slice4-images to main October 21, 2026 04:59
@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (final check, head 54894eb)

Reviewed head 54894ebef38b91c7de3e937cafff8f030306c912, base main d2a549f4, merge base 49b01df8 (#929's head).

The pull side and the git-mode binding hold up. Three things don't: the copy-out has no size bound, content mode passes with no binding, and the pulls may write instance ids into a journal that the agents can read.

1. Held-out secrecy

Check Result
No task id or image name in the output Met (measured). test/bench-pull.test.ts 17/17 pass at the head, including the 7-scenario leak test. My own probes also printed none.
docker's own error text never printed Met (measured, test 5. inferred from pull.ts:66-71 and :324-327). PullRefused is mapped to stage and exit status. Errors from Docker.start that aren't Refused or LimitsNotApplied become a fixed message.
Trees stay in the held-out store, which the sandbox denies Met (measured). A probe calling benchDenied() at the head gave heldOut denied=true trees under a denied folder=true.
The daemon's own log Open, see required change 3. bench pull is the first command that hands task image names to the bench user's rootless dockerd. benchDenied() has no journal path (measured: journal denied=false). The README itself sends the owner to journalctl --user -u docker to see what failed. Whether dockerd logs the reference of a successful pull at info level (image pulled ... remote=<name>@<digest>) is a guess, likely with the containerd image store. A failed pull's error is more likely to be logged (guess).

2. Binding (probes with a stand-in docker call, host git, temp store under my scratch dir)

Attack Result
Wrong commit object, or the right tree without the commit Refused (measured, tests 11 and 12). In git mode, base_commit's own commit object must hash and name the tree. Forging one needs a SHA-1 collision on a chosen commit (inferred).
Extra objects or upstream history in the pack Dropped (measured, test 12). The fetch takes only what is reachable, and the object count is checked.
A .git/hooks/post-checkout entry in the tree (built with mktree) Refused (measured): Refused: building its bare remote and clone failed. git's verify_path stops the checkout.
A symlink in the tree (evil -> /etc/passwd, up -> ../../../../../../..) Accepted (measured). The clone keeps both as links. In git mode they can only be base_commit's own, which is legitimate. In content mode the image chooses them. Nothing in this PR follows them (inferred). Note for slice 7, which copies the tree.
A submodule gitlink with .gitmodules url = file:///... Accepted (measured): clone/sub is an empty folder and nothing is fetched. The README doesn't say that submodule contents are missing (non-blocking).
A pack bomb Accepted, no bound (measured). A 261,149-byte pack (348,200 base64) became a 268,435,456-byte file in trees/t01/clone, about 1028:1. In git mode the tree is base_commit's, so the image can't inject one. In content mode it can. Unreachable extra objects are still inflated by index-pack (inferred).
Copy-out size Not bounded (measured) through the real async path: TaskRunner.exec with a stand-in docker binary. 256 MiB of stdout was taken whole (stdout chars=268435456, RSS grew by about 770 MiB). 600 MiB crashed the process with an uncaught ERR_STRING_TOO_LONG, thrown in spawnDockerAsync's close handler. That crash skips finally { docker.stop(c) }, so the sleep infinity container is left running (inferred).
Copy-out time Bounded (inferred from code). Probe 5 min, pack and content 60 min, docker run 10 min, each host git call 30 min.
Content mode (no .git) No binding to base_commit (inferred, and the PR body's "Not defended" says so). It is disclosed in the PASS line ("HEAD not checked against base_commit"), the README and tree.json (head_checked: false). But it still prints PASS. Nothing says that an image picks this path just by having no /testbed/.git. See required change 2.

3. No-tag pulls

  • Met (measured). Test 2 asserts that every docker argv containing the image name matches name@sha256:<64 hex> (pull, image inspect, run).
  • Read-back of another pinned digest, an unpinned digest, or the digest under another name is refused (test 4, measured).
  • An images.json swapped during the pulls is refused (test 8, measured).
  • Trees are built from the in-memory tasks that the re-read confirmed (inferred).
  • Observation, non-blocking: container.ts IMAGE (line 49) still allows a :tag on run. So the no-tag property of run rests on loadTasks building task.image, not on the argv check (inferred).

4. README owner steps A to L

  • Accurate against the code (inferred from reading):
    • step 10 describes pull.ts as it is;
    • each block equals its setup step (measured, test 16);
    • K's labels are right.
  • Shell history (inferred):
    • no block types an instance id or an image name;
    • H types the dataset file name (a sha256), B the bench user's name, and E a clone URL.
  • Step I (inferred):
    • "blocked" is stated honestly in I, in step 7 ("Collecting the image digests is not built yet (an open gap)") and in the smoke plan's step 4;
    • it says a decision is needed before J.

5. Fail-without (re-measured)

  • Base run (measured):
    • setup: test/bench-pull.test.ts written as a NEW file into a fresh worktree at 49b01df8, byte-identical to the head's (sha256 763170ef…bdd both);
    • command: node --import ./dist/test/isolate.js --test dist/test/bench-pull.test.js;
    • result: # tests 17, # pass 0, # fail 17.
  • Why they fail (measured):
    • 14 fail on pull is not a function and 1 on Cannot find module …/dist/scripts/bench/pull.js: a missing symbol, unavoidable for a new command;
    • test 1 fails on an assertion: the usage has no pull;
    • test 16 fails on the README has the owner steps.
  • At the head (measured): 17/17 pass.

6. Other checks

  • typecheck: clean at the head (measured).
  • check:denylist: 1401 files clean (measured).
  • Package size: delta 0 (measured with npm pack --dry-run --json at the head and at 49b01df). Both give size 1470176, unpackedSize 4926629, entryCount 491.
  • critical.mjs: #947 self-merge-eligible (6 files) (measured). That disagrees with the PR's own [PROTECTED: owner merges], its needs-owner label, and Bench: gated-merge benchmark on SWE-bench Verified (smoke, baseline, per-release rerun) in scripts/bench/ #748's "The owner merges" (L3 scripts/**). The PR's stricter marking stands; the classifier gap belongs to the orchestrator.
  • Protection markings: the title, **Risk class:** protected, needs-owner, ## What this can never do, ## Compromised component and ## Adversarial pass are all present (measured).
  • Compromised component:
    • its "a huge pack ... denial of service only (inferred)" is right that nothing is accepted;
    • it should add that the crash case leaves the container running (inferred).
  • Commits: 2, both Signed-off-by (measured). No attribution trailers (measured).
  • Scope:
    • no change to package.json, package-lock.json, src/ or .claude/ (measured);
    • no new dependencies;
    • nothing beyond slice 4 except the README's owner-steps list and step 5's redo wording. Both serve this slice.
  • Owner's real state: the tests use a temp HOME, a stand-in docker and PATH=/nonexistent for the CLI, so they can't touch real state (inferred from reading the test file and the isolate preload it requires).
  • git merge-tree --write-tree origin/main HEAD: clean, tree 19acdf82 (measured).
  • CI at the head: run 39545411032 has headSha 54894eb, completed/success. Every required job passes and the systemd jobs are skipped (measured, gh pr checks 947). Not rerun.

Required changes

  1. Bound the copy-out (principle 1).
    • Refuse, cleanly, a copy-out whose stdout passes a fixed byte cap, and remove the container. The cap goes in the exec path that copyOut uses, or in a TaskRunner option. Today 256 MiB is taken whole and 600 MiB crashes the process with the container left running.
    • Before the clone checkout, refuse a pack or tree whose decoded size, or total object size, passes a cap. For example, sum cat-file --batch-check='%(objectsize)' over what the remote holds.
    • Add a test for each: a stand-in docker that prints past the cap is refused and its container removed; a small pack that inflates past the tree cap is refused with nothing kept.
  2. Content mode fails closed (principle 1).
    • An image with no /testbed/.git currently gets PASS with a tree nothing binds to base_commit. Taking that path takes nothing more than deleting .git.
    • SWE-bench's own images keep /testbed as a git checkout, since its harness runs git there (inferred).
    • Refuse content mode by default (FAIL at that key, nothing kept), with a test.
    • If a fallback is wanted, it is a new loosening and the owner must decide it; this PR shouldn't ship it as a PASS.
  3. Deny the journal to the agents, or measure that it holds no image name.
    • This PR is the first to hand task image names to the bench user's dockerd. benchDenied() doesn't cover the journal (measured), and the README points at journalctl --user -u docker for failures.
    • Fail-closed fix, a tightening: add /var/log/journal and /run/log/journal to benchDenied, with a test.
    • Alternatively, state as an open gap in the README and the PR body that this is unmeasured, for the owner to decide before the first real bench pull.
  4. Correct the "Compromised component" huge-pack line to match the measurement above: no size bound, and a crash that leaves the container. If change 1 lands, describe the cap instead.

Non-blocking:

  • the README could say that submodule contents are missing (gitlinks become empty folders);
  • container.ts IMAGE could refuse tags on run.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…adata, and the lane denies the journal

Review round 2 of #947 (principle 1, fail closed).

The copy-out had no bound: 256 MiB of stdout was taken whole, and 600 MiB crashed the process in the async
docker runner's close handler, skipping the container's removal. TaskRunner.exec now takes a byte cap: the async
runner ends the CLI at the first byte past it and keeps nothing (OutputTooLarge), and a result too long for one
string is refused instead of thrown in an event handler. bench pull caps the probe (64 KiB) and the pack (384 MiB
of base64), and caps what the pack decodes to (1 GiB, every object summed) before anything is fetched or checked
out: a 261 KB pack had become a 256 MiB file. Past a cap the task is refused, nothing kept, the container removed.

An image with no /testbed/.git is now refused: the content-hash path bound nothing to base_commit, and an image
took it just by having no .git. A fallback would be a loosening for the owner. A kept tree's record must be the
git, HEAD-checked kind.

bench pull is the first command that hands task image names to the bench user's dockerd, which logs to the system
journal; the agents' lane now denies /var/log/journal and /run/log/journal, and the README's log hint says to read
it on screen only. baseTree also refuses a task image that isn't its own pinned digest reference before docker run.

The bench pull tests share their fixtures (test/bench-pull-fixtures.ts).

Test (fails without): test/bench-pull-bounds.test.ts (7 of 7 fail at 54894eb, each on behaviour).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Round 2: head e5dfe1e (main d2a549f merged in)

All four required changes are made, plus both non-blocking notes. The PR body's "Round 2" section has the detail.

  1. The copy-out is bounded.
    • TaskRunner.exec takes a byte cap. The async runner ends the CLI at the first byte past it and keeps nothing.
    • A result too long for one string is refused, not thrown in the close handler.
    • bench pull caps the probe at 64 KiB, the pack at 384 MiB of base64, and what the pack decodes to at 1 GiB, summed over every object and checked before any fetch or checkout. Past a cap the task is refused, nothing is kept and the container is removed (measured, tests).
    • On the real async path, against a stand-in docker script (measured):
      • 600 MiB is refused at the cap, with peak RSS growth of 386 MiB;
      • 600 MiB with no cap is refused and no longer crashes.
    • Both caps are guesses.
  2. No /testbed/.git is refused. Nothing is copied out and the container is removed (measured). The fallback is listed as a proposal in "What remains".
  3. The journal. benchDenied now holds /var/log/journal and /run/log/journal (measured). The README hint is on-screen only: never save or copy the output. What dockerd logs is a guess: no stand-in can measure it.
  4. "Compromised component" is corrected for the huge-pack and crash cases, now describing the caps and the index-pack residual.
  5. Non-blocking:
    • baseTree refuses a task image that isn't its own pinned digest reference before docker run (measured). container.ts IMAGE itself is unchanged; that would mean changing the test/bench.test.ts driver fixtures, and it is listed for slice 7.
    • The README now notes that submodules become empty folders.

Fail-without (measured): test/bench-pull-bounds.test.ts and test/bench-pull-fixtures.ts were written as new files into a worktree at 54894eb. They are byte-identical to the head's. Result: # tests 7, # fail 7, each failing on behaviour.

After (measured):

  • 87/87 across bench-pull, bench-pull-bounds, bench-images, bench-select, bench-select-lookalike, bench-cli and bench-pins;
  • the six container tests in bench.test.ts;
  • the round-7 scan, the weakening backstop and the changelog.d checks all pass;
  • typecheck clean; denylist 1413 files clean; DCO all signed off.

@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (delta final check, head e5dfe1e)

Reviewed head e5dfe1e160d1264f1d7490e0a1dd3fbcd92e97f6, previous head 54894ebe, main d2a549f4. The round's fixes hold: every probe from the last verdict now refuses cleanly and removes its container. What's left is a correction to two claims. A crafted delta pack of 1,115 bytes makes the host's index-pack use 1.5 GiB of memory before any check runs, and the body and README name only CPU for that step.

1. The findings of the last verdict

Finding Result
Copy-out crash and unbounded copy-out Fixed (measured). My probe ran the real async spawn in TaskRunner.exec against a stand-in docker binary, through baseTree with the default TREE_CAPS. 600 MiB on stdout was refused at the 384 MiB cap (went over 402653184 bytes) in 0.7 s. Node's peak RSS was 459 MiB, the container was removed and trees/ was empty. With no cap at all (maxOutBytes undefined), 600 MiB now throws refused: this docker command's output was too long to read; it no longer crashes. The container's removal is in finally (pull.ts:358-360).
Caps As stated (measured from code and tests): probe 64 KiB, pack 384 MiB of base64, decoded size 1 GiB. The decoded size is checked after index-pack and the binding check, and before commit-tree, fetch and clone (pull.ts:302-310).
Pack bomb (zeros) Fixed (measured). 1100 MiB of zeros in a 1,121,272-byte pack was refused with decodes to over 1073741824 bytes: refused before any checkout. Nothing was kept and the container was removed. 256 MiB of zeros (a 261,104-byte pack) is under the 1 GiB cap and is taken, as designed: the tree is base_commit's, bound by hash.
Is the decoded-size check itself bounded? Yes (inferred from code). cat-file --batch-all-objects --batch-check=%(objectsize) prints one short line per object. That output is capped by the git helper's maxBuffer (256 MiB) and its 30-minute timeout, and an overflow throws, which counts as Infinity and is refused. For a delta, %(objectsize) reads the size from the delta's header without rebuilding the object (inferred from git's documented behaviour).
index-pack cost Only CPU is named; memory is not (measured). See required change 1.
Content mode Removed (measured). CONTENT and source: 'content' are gone. A /testbed with no .git is refused before any copy-out (bounds test 4). A kept tree.json with source: 'content', head_checked: false is refused with not ... bound to base_commit (bench-pull.test.ts kept-tree test, which passes).
Journal Fixed (measured). benchDenied holds /var/log/journal and /run/log/journal (agent.ts JOURNALS), and so does benchLane's denyRead (bounds test 6). README line 404 now says to read the log on screen only and never save or copy journal output.
Non-pinned image before docker run Fixed (measured). baseTree runs requirePinned(task.ref) first. It then requires task.image === name@digest, matching DIGEST_REF. name:latest, a bare name and name:latest@digest are all refused, with zero run calls (bounds test 5).

2. Fail-without (re-measured)

  • Base run (measured):
    • setup: test/bench-pull-bounds.test.ts and test/bench-pull-fixtures.ts written as NEW files, with the Write tool, into a fresh worktree at 54894ebe. They are byte-identical to the head's (sha256 e1b24dfe… and 860faae6… both);
    • command: node --import ./dist/test/isolate.js --test dist/test/bench-pull-bounds.test.js;
    • result: # tests 7, # fail 7.
  • Each fails on behaviour (measured):
    • tests 1, 3 and 4: expected: 1 actual: 0 (pull passed);
    • tests 2 and 5: Missing expected rejection;
    • test 6: '/var/log/journal' expected: true actual: false;
    • test 7: the README match.
  • At the head (measured): 87/87 pass across bench-pull, bench-pull-bounds, bench-cli, bench-images, bench-pins, bench-select and bench-select-lookalike. The TaskRunner tests in bench.test.ts also pass, 13/13 (name-filtered: task runner, hidden fields, wall-clock, tar stream, stdin:). Every run used a temp HOME, TMPDIR and state. The CLI tests set PATH=/nonexistent.

3. Weakening gate: changed or removed existing test lines

test/bench-pull.test.ts, removed:

  • The fixtures (rows, pins, stand-in docker, cliDeps) moved to test/bench-pull-fixtures.ts. This is a refactor; no assertion changed (measured by diff). The stand-in now strips a :tag when it looks up a name, and uses ?? '' instead of !. It answers a tagged image instead of crashing, which the new image-reference test needs. That is a stand-in's tolerance, not a loosened assertion.
  • The test no git metadata in /testbed: its content hash is recorded instead ... expected PASS for a content-mode image. It is replaced by a refusal (bounds test 4: FAIL, nothing kept, nothing copied out, container removed). That is a tightening, and the last verdict required it.
  • In the container's own scripts test, the CONTAINER_SCRIPTS.content assertions were removed. The code they tested is deleted; not a loosening.
  • In the same test, assert.deepEqual(snapshot(plain), plainBefore) was removed (non-blocking).
    • It is the one removed assertion still about live code: the probe still runs on plain, and that plain is left unchanged is no longer asserted.
    • The probe's other branch is still covered by snapshot(testbed), and the none branch only echoes (inferred), so the risk is low.
    • Restoring it is one line. The body's "Checks removed or weakened: None" should list these removals.

test/bench-pull.test.ts, added: the kept-tree test now also refuses a content-mode record (a tightening).

No other existing test was changed. The other test files in the two-dot diff come from the merge of main. The merge commit 39e67dd9 has tree 19acdf82, identical to git merge-tree --write-tree d2a549f4 54894ebe (measured).

4. Other callers of TaskRunner.exec (check 6)

Production callers (grep, measured):

  • pull.ts capped(): probe and pack, with caps.
  • TaskRunner.syncTree: no cap; its output is tar's (small).
  • The driver's ctx.exec, handed to runTask. No production runTask exists yet; only test/bench.test.ts calls ctx.exec.exec.

With no cap, behaviour is unchanged except in one case: output too long for one string now throws refused: ... too long to read instead of crashing in the close handler. The 13 bench.test.ts runner tests pass (measured).

Uncapped output is still buffered whole: 600 MiB gave 1,273 MiB peak RSS (measured). That is a note for slice 7: bench run's test commands should pass a cap. Non-blocking.

5. Body claims against the code

  • "What this can never do": every item still holds (inferred from reading; the cited tests pass, measured).
  • "Compromised component": two lines are wrong or incomplete; see required changes 1 and 2.
  • "What remains":
    • the content-hash fallback is correctly framed as a loosening for the owner;
    • the IMAGE tag check is deferred to slice 7, which is reasonable since baseTree now refuses non-digest references itself.

6. Other checks

  • typecheck: clean at the head (measured).
  • check:denylist: 1413 files clean (measured).
  • Package size: npm pack --dry-run at the head gives size 1,481,905, unpacked 4,963,126, entries 493 (measured). No scripts/bench or changelog.d file is in the package (measured: 0 matching paths), so this PR adds 0 bytes (inferred).
  • git merge-tree --write-tree origin/main e5dfe1e1: clean, tree fc9facc3 (measured). origin/main is d2a549f4, an ancestor of the head (measured).
  • Commits: all four first-parent commits since 49b01df8 carry Signed-off-by and no Co-Authored-By (measured).
  • Protection marking: unchanged. The title has [PROTECTED: owner merges] and the PR carries the needs-owner label (measured).
  • CI at e5dfe1e: 14 jobs have passed and the systemd jobs are skipped. test shard 4/4 (ubuntu-latest, 22) and test shard 3/4 (ubuntu-latest, 24) are still pending (measured, gh pr checks 947). Not rerun.

Required changes

  1. Name index-pack's memory cost, measured (principle 5); better, bound it (principle 4).
    • The measurement: I crafted a pack with a 1 MiB zero blob and one OFS_DELTA object. The delta's target is 1536 MiB, built from 1 MiB copy instructions at two bytes each. The pack is 1,115 bytes.
      • Run alone, git index-pack on it peaked at 1,578,756 KB RSS in 3.7 s (measured, /usr/bin/time -v).
      • Sent as a task's pack through baseTree, the whole run peaked at 1,577,720 KB (index-pack is the child). Only after that was it refused (not a pack holding its tree). Container removed, nothing kept (measured).
    • How it scales: memory grows with the delta's declared target size, so a pack a few KB larger can ask for an allocation the size of the host's RAM (inferred). Whether that ends as git dying on a failed allocation or as an OOM kill depends on the host (guess). The 1 GiB decoded-size check runs only after index-pack, so it can't prevent this.
    • The claims to correct:
      • the PR body's "Compromised component" ("could cost CPU time ... but no disk beyond the pack");
      • the body's "Not defended";
      • README line 396 ("checked before anything is fetched or checked out, since a small pack can inflate a thousandfold"): the host index-pack inflates it first, and a delta inflates far more than a thousandfold in memory.
    • Preferred fix (by design): refuse before index-pack. One way is to walk the pack's entry headers: a whole object's size is in its header, and a delta's target size is in the first bytes of its inflated delta. Sum them against treeBytes. Another is to run the host index-pack under an address-space limit derived from the caps.
    • If the builder judges a bound out of scope, the measured residual must be named accurately in all three places, for the owner.
  2. Correct "Memory peaks at about the cap, 384 MiB (measured)" (principle 5).
    • That holds when the copy-out goes over the cap: 459 MiB peak for 600 MiB (measured).
    • A copy-out just under the cap is held as buffers, a string and the decoded pack together: 383 MiB of output peaked at 1,511 MiB RSS (measured).
    • State the under-cap peak too.

Non-blocking:

  • list the removed test assertions (section 3) under "Checks removed or weakened";
  • restore the snapshot(plain) check after the probe;
  • the change table still says test/bench-pull.test.ts | +687;
  • slice 7: give bench run's TaskRunner.exec calls a byte cap.

Probes (scratch only, stand-in docker, temp store): probe2.mjs and fake-docker.mjs in the evaluator's scratch folder work/eval-947-d2/.

…the caps before git indexes it

Review round 3 of #947. The decoded-size cap ran after git index-pack, which builds each object in memory: a
crafted 1,115-byte delta pack made it peak at 1.58 GB before any check (measured in review).

packDeclaredBytes walks the pack itself before index-pack sees it: the signature, version and count, each entry's
type and size, a delta's target size from its delta header, each entry's zlib data inflated once (never past its
declared size) to find where it ends, and nothing after the last entry but the trailer. A whole object or delta
target over 256 MiB, or a total over 1 GiB, is refused, nothing kept, the container removed; so is any pack that
isn't well formed. With crafted packs under the caps, index-pack peaked at 255 MiB for side-by-side deltas and
1005 MiB for a chain declaring 1001 MiB (measured), so its peak tracks the total cap.

The README and the release note say what is checked before index-pack and the measured peaks, including bench
pull's own (about 1.5 GB just under the copy-out cap, measured in review). The probe test again checks that a
/testbed with no git metadata is unchanged by the probe.

Test (fails without): test/bench-pull-pack.test.ts (5 of 5 fail at e5dfe1e: no pack walk, a delta bomb reaching
index-pack, the README).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Round 3: head 92a8de8

  1. index-pack's memory is bounded by design.
    • packDeclaredBytes walks the pack before git sees it: each whole object's size from its header, and each delta's target size from its delta header. It refuses any object over 256 MiB, a total over 1 GiB, and any pack that isn't well formed.
    • Your 1,115-byte pack-d.pack is refused in 2 ms, before git runs (measured).
    • Admitted packs (measured with crafted packs, /usr/bin/time -f %M): index-pack peaked at 255 MiB for 1 or 4 side-by-side 250 MiB deltas, and at 1005 MiB for a chain of 4 (1001 MiB declared). Its peak tracks the 1 GiB total cap.
  2. Claims corrected:
    • "Compromised component", "Not defended" and README step 10 now give the measured peaks;
    • bench pull's own peak is stated as about 1.5 GB just under the copy-out cap (your measurement). I did not stream the copy-out; that is listed in "What remains".
  3. Non-blocking, done:
    • "Checks removed or weakened" lists each removed or replaced test assertion;
    • snapshot(plain) is restored;
    • the size table is current;
    • "What remains" has the slice-7 cap line.

Fail-without (measured): test/bench-pull-pack.test.ts was written as a new file into a worktree at e5dfe1e (sha256 921b6559…, same as the head's). Result: # tests 5, # fail 5.

After (measured):

  • 92/92 across the eight bench-pull/bench-* test files;
  • the round-7 scan plus six container tests by name: 7/7;
  • weakening backstop 2/2, changelog.d 1/1, selfcheck 2/2;
  • typecheck clean; denylist 1414 files clean; DCO all signed off.

@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (delta final check, head 92a8de8)

I reviewed head 92a8de80d86b795993cf72aad7d31b7f66a71db8, previous head e5dfe1e1, main d2a549f4.

The round's fix works for the shape that was reported: the 1,115-byte delta bomb is now refused before git runs, and the walker is solid against malformed packs. But the new claims about memory and time under the caps don't hold:

  • index-pack resolves deltas on several threads, so its peak grows with the host's CPU count, not with the 1 GiB total;
  • a delta's own data is not counted, so the walker can be made to inflate for minutes;
  • bench pull's own peak, just under the cap, is about 1.9 GB for a well-formed pack, not 1.5 GB.

All probes ran in scratch only: a stand-in docker binary, temp HOME, TMPDIR and store, and host git in temp folders. No real docker, no network.

1. The finding of the last verdict, and the walker

Check Result
The round-2 delta bomb (eval-947-d2/delta/pack-d.pack, 1,115 bytes, 1536 MiB target) Fixed (measured). The walk refused it in 1.6 ms: declares an object of over 268435456 bytes: refused before it is indexed. Through baseTree (stand-in docker, default TREE_CAPS), the whole run took 0.43 s and peaked at 75,200 KB (it was 1,577,720 KB). The container was removed and trees/ stayed empty.
zlib data that inflates to MORE than declared Bounded (measured). A blob declaring 5 bytes whose stream inflates to 1 GiB was refused in 0.7 ms. One declaring 200 MiB was refused in 150 ms, peaking at 278 MiB (maxOutputLength stops at the declared size). Delta data declaring 6 bytes that inflates to 1 GiB was refused in 1.7 ms.
OFS_DELTA vs REF_DELTA Both handled (measured). Each refuses a 1536 MiB target. REF_DELTA at exactly 256 MiB is taken, and at 257 MiB refused. Five REF_DELTAs of 250 MiB are refused on the 1 GiB total.
Negative or huge varints Refused (measured). An entry size of about 2^67 (9 continuation bytes) and one of 2^53+1 are refused on the object cap; 10 continuation bytes are refused as malformed. A delta target of 2^63 is refused on the cap, an 11-byte one as malformed, and an 11-byte OFS offset as malformed. Sizes are sums of non-negative terms, so none can go negative (inferred). An OFS offset that points before the pack start passes the walk; git rejects it (inferred).
Truncation Refused (measured). I cut a valid 3-entry pack at every length from 0 to 3093 bytes: 3094 refused as Refused, 0 accepted, 0 other errors, slowest 6 ms. I also flipped 1 to 4 random bytes in its entries 20,000 times: 19,977 refused, 23 accepted (sizes still consistent), 0 non-Refused errors, slowest 9 ms.
Count mismatch Refused (measured): count 0 with one entry (trailing bytes); count 2, and 0xFFFFFFFF, with one entry (fewer entries than the count). Count 0 with no entries is accepted, total 0; the binding check refuses it later (inferred).
Wrong trailer checksum Not checked by the walk (measured): accepted, total 5. git index-pack then refuses it with pack is corrupted (SHA1 mismatch) at 4 MB RSS (measured). The body only claims "nothing but the trailer follows the last entry", which is true. Non-blocking.
Delta base sizes that lie Safe (measured). The walk ignores a delta's base size, and git catches a lie. Base size 1 for a 1 MiB base: failed to apply delta, 5 MB RSS. A 1 MiB target whose copies make 200 MiB: delta replay has gone wild, 6 MB RSS. In a chain, a second delta that claims a 1 MiB base when its base is 250 MiB: refused, 261 MB RSS.
Walker vs git on real packs Equal (measured, the head's tests: gitSizes cross-check).
Is the walker's own time bounded? Only by the pack size; and it is not stated (measured). See required change 2.
Is index-pack's memory tracked by the 1 GiB total? No (measured). See required change 1.

2. Corrected claims vs measurement

bench pull's own peak just under the copy-out cap:

  • Not a pack (measured). 383 MiB of A gave a node peak of 1,512 MiB; the walk refused it with "no pack signature". That matches the stated "about 1.5 GB".
  • A well-formed pack just under the cap (measured):
    • the pack: two random 143.4 MiB blobs, 300,777,530 bytes, base64 401,036,708 bytes, under the 402,653,184 cap;
    • node peaked at 1,891 MiB and the process tree at 1,936,760 KB;
    • it was refused at the binding check in 8.3 s;
    • the walk inflates each object while the copy-out's buffers and decoded pack are still held.
  • So "about 1.5 GB" is low by about 400 MiB. See required change 3.

index-pack peaks:

  • The builder's shape (measured): a 1 MiB base and four side-by-side 250 MiB OFS_DELTAs, built as in the head's own deltaBomb. It gave 1,030,716, 1,030,636, 1,030,452 and 1,030,580 KB in 4 of 4 runs, with git 2.43.0 on 16 CPUs. That does not reproduce the body's "255 MiB for four side by side". One such delta alone gave 262,452 KB, which matches.
  • A shape whose peak goes well over the total (measured):
    • the pack: a 1-byte base and 33 valid OFS_DELTAs. Each has a 31 MiB target written as 1-byte copies at 8 bytes each (opcode 0xff), so 248 MiB of delta data. The pack is 12,501,787 bytes;
    • the walk accepts it: declared 1,072,693,249, under 1 GiB, in 11.5 s;
    • git index-pack --stdin accepts it, with these peaks:
Threads Peak RSS Runs
default (8 on this host, inferred) 1,859,488 / 860,628 / 1,956,380 / 1,716,424 / 1,840,952 / 1,879,440 KB 6
--threads=16 3,265,032 KB 1
--threads=1 289,404 / 289,368 / 289,512 KB, about 24.5 s each 3
  • through baseTree the process tree peaked at 1,718,312 KB, with node at 872 MiB, before the binding refusal. The container was removed and nothing was kept.
  • Why (inferred): each thread holds a delta's own data, up to 256 MiB, plus its target, and the walk doesn't count delta data.
  • On bigger hosts (inferred from git's source, not measured): git's default thread count is half the CPUs, capped at 20. A 40-CPU host would run 20 threads, about 5 GB on this shape.
  • So these claims are wrong:
    • "its peak tracks the 1 GiB total cap";
    • "Host worst case ... about 2.5 GB";
    • "Not defended: index-pack can still peak at about the 1 GiB total cap";
    • the matching README step 10 lines.

3. Restored check and weakening gate

  • The restored check (measured). const plainBefore = snapshot(plain) and assert.deepEqual(snapshot(plain), plainBefore, ...) are back, around the probe in the container's own scripts test, and it passes at the head.
  • The round's only change to an existing test is those 2 added lines; nothing was removed (measured: git diff e5dfe1e1 92a8de80 -- test/).
  • Against main (measured, gh pr view --json files). Every test file in this PR is new (-0 deletions), so no existing test line of main is changed.
  • The body now lists the round-2 removals under "Checks removed or weakened", each with its reason. I checked them against last round's section 3 and they are accurate. None is a loosening without the owner's OK.
  • No guard, lint or check was weakened (measured by diff).

4. Fail-without (re-measured)

  • Setup: test/bench-pull-pack.test.ts written as a NEW file, with the Write tool, into a fresh worktree at e5dfe1e1. Its sha256 is 921b6559…, identical to the head's.
  • Command: node --import ./dist/test/isolate.js --test dist/test/bench-pull-pack.test.js, with a temp HOME, TMPDIR and state, and stand-ins first on PATH.
  • Base result (measured): # tests 5, # fail 5. Each fails on behaviour:
    • tests 1, 3 and 4: expected: 'function' actual: 'undefined' (no walk exists);
    • test 2: FAIL t01 what came out of /testbed is not a pack holding its tree. The 300 MiB delta bomb went through index-pack first;
    • test 5: the README match.
  • At the head (measured): 92/92 across bench-pull-pack, bench-pull, bench-pull-bounds, bench-cli, bench-images, bench-pins, bench-select and bench-select-lookalike.

5. Other checks

  • Typecheck: clean at the head (measured).
  • check:denylist: 1414 files clean (measured).
  • Package size: no file changed this round is in package.json files (scripts/bench, test, changelog.d), so the package delta is 0 bytes (inferred from the files list).
  • git merge-tree --write-tree origin/main 92a8de80: clean, tree ddc788be (measured). origin/main d2a549f4 is an ancestor of the head (measured).
  • Commit 92a8de80: signed off, with no Co-Authored-By or "Generated with" line (measured).
  • Protection marking: unchanged. The title says [PROTECTED: owner merges] and the PR carries the needs-owner label (measured).
  • CI at the head (measured, gh pr checks 947, not rerun):
    • passed: agentshield, dco, denylist, desktop, new-deps, push-gate, scan ×2, scope, and test shards 2/4 (22, 24), 3/4 (22, 24) and 4/4 (24);
    • skipped: the real-systemd jobs;
    • pending: test shard 1/4 (22), 1/4 (24) and 4/4 (22).

Required changes

  1. Make index-pack's peak follow the caps by design (principle 4), then correct the claims (principle 5).
    • The measurement: an admitted 12.5 MB pack makes index-pack peak at 1.72 to 1.96 GB with git's default threads on this 16-CPU host, and at 3.27 GB with 16 threads (measured). With --threads=1 it peaks at 283 MiB (measured).
    • One by-design fix: run it as index-pack --threads=1 --stdin. The cost is time: 24.5 s against 13 to 21 s on this shape (measured).
    • With or without that, count each delta's own data (its entry size) against a budget too; see change 2.
    • Then re-measure, and correct:
      • "Compromised component" ("tracks the 1 GiB total cap", "Host worst case ... about 2.5 GB");
      • "Not defended";
      • README step 10;
      • the "255 MiB for four side by side" figure, which gave 1,030,716 KB here in 4 of 4 runs. Say what host and git version each figure is from.
  2. Bound the walk's own time, or state it (principles 4 and 5).
    • A delta's own data is inflated up to 256 MiB but counts 0 toward the caps.
    • The measurement: 1,150 deltas, each with 256 MiB of zero delta data (declared base 0, target 0), make a 300,070,794-byte pack, under the copy-out cap. The walk accepted it (total=1) after 302.6 s, peaking at 1,200,320 KB (measured).
    • Through baseTree: the same pack did not finish within 600 s, the walk followed by index-pack's first pass (measured, one run with other load on the machine).
    • The body's "Its CPU time is bounded by the 30-minute git timeout" covers index-pack only. The walk runs in-process before git, with no timeout.
    • Preferred fix: sum every entry's inflated size, delta data included, against a budget (for example, the total cap). That bounds both the walk and index-pack's first pass. Otherwise, state the measured cost.
  3. Correct bench pull's own peak just under the cap (principle 5).
    • It is about 1.5 GB only when the copy-out is not a pack (1,512 MiB, measured).
    • A well-formed pack just under the cap peaked at 1,891 MiB, because the walk inflates objects while the copy-out is still held (measured).
    • During the 1,150-delta walk, ps showed the node process at 1,955,156 KB (measured, sampled).
    • Either state the higher figure, or drop the base64 string and chunk buffers before the walk.

Non-blocking:

  • the walk doesn't verify the trailer's SHA-1 (git does, measured);
  • an OFS offset pointing before the pack start passes the walk (git rejects it, inferred).

Probes (scratch only): walk-probe.mjs, probe3.mjs and run-node.sh in the evaluator's scratch folder work/eval-947-d3/. They reuse work/eval-947-d2/fake-docker.mjs and delta/pack-d.pack.

…, and runs host git on one thread

Review of round 3 measured three gaps under the caps. A delta's own data counted nothing, so a 300 MB pack of
zero delta data took the walk 302.6 s, and 33 valid deltas of 248 MiB data each (12.5 MB pack) passed the walk
and made index-pack peak at 0.86 to 1.96 GB with git's default threads on 16 CPUs (3.27 GB with 16, 283 MiB
with one). The builder also measured 18 million 4-byte blobs (a 288 MB pack): 49.7 s in the walk, 1.41 GB in
index-pack with one thread.

The walk (packDeclaredBytes) now charges one budget, TREE_CAPS.treeBytes: 1 KiB an entry (the count is charged
before any entry is read), the bytes each entry's data inflates to, a delta's own data included, and each
delta's target. Each charge is made before the data is inflated, so the walk never inflates past the budget.
Data over 1 MiB is inflated as a stream and dropped, keeping only its first 32 bytes. The walk also checks the
trailer's SHA-1 and that an OFS_DELTA's base starts inside the pack. It now runs in baseTree, before anything is
made in the store. Host git runs with pack.threads=1, and index-pack with --threads=1.

The README's step 10 and the changelog fragment are corrected with the measured figures.

Test (fails without): test/bench-pull-budget.test.ts, written as a new file in a worktree at 92a8de8: 6 tests,
5 fail (many valid large deltas not refused; tiny entries not refused; a wrong trailer taken; index-pack run
without --threads=1; the README's round-3 claims), 1 passes (the streamed path's size checks, kept as coverage).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (delta final check, head 410ef53)

I reviewed head 410ef53196c559634055589fb9c9a2654e2105a6 against the previous reviewed head 92a8de80. Main is 9f9533aa, which the branch hasn't merged.

The round fixes all three findings of the round-3 delta. The walk now has one budget, charged before anything is inflated; host git runs on one thread; and the bench pull peak is now stated, not lowered. I couldn't find an admitted pack that pushes index-pack past the memory the body states. But two things are left:

  • CI is red because of this PR: both runs of test shard 3/4 fail on no-wall-clock-durations;
  • the new walk-time figure is low: "at most about 3 s" in the README. An admitted shape took 3.5 to 4.6 s.

All probes ran in scratch only: an in-process stand-in docker (the review's fake-docker.mjs), and temp HOME, TMPDIR, state and store. Host git ran only in temp folders. No real docker, no network.

1. The round-3 findings

Finding Result
index-pack memory grew with the thread count Fixed (measured). GIT_HARDENED now passes -c pack.threads=1 to every host git, and index-pack runs with --threads=1. The head's test records every git argv and checks both. At 92a8de8 that test fails with index-pack ... --stdin and no --threads=1. The review's copy8-33x31.pack (12,501,787 B) no longer reaches git: the walk refuses it in 380 ms at 121 MiB. Through baseTree the run took 0.7 s with node at 149 MiB; the container was removed and trees/ stayed empty.
The walk's time was not bounded by the caps Fixed (measured). Every entry's inflated data, delta data included, is charged before it is inflated, along with each delta's target and 1 KiB an entry. Walk results on the review's packs: dd-1150.pack (300,070,794 B; 302.6 s before) refused in 0.72 s at 396 MiB; dd-100.pack (27.0 s before) refused in 1.23 s at 130 MiB; round 2's pack-d.pack refused in 3.5 ms. Through baseTree, dd-1150 was refused in 3.6 s. Container removed, trees/ empty.
The bench pull peak figure Stated, and it matches (measured). The review's valid-2x143.pack through baseTree peaked node at 1,510 and 1,509 MiB in 2 runs, and dd-1150 at 1,506 MiB. All three are inside the body's "1,509 to 1,894 MiB over 6 runs".

2. Hunting for an admitted pack past the stated time or memory

My packs are in work/delta-947-r4/tmp/, built by mkpacks.mjs. index-pack ran as git -c pack.threads=1 index-pack --threads=1 --stdin, timed with /usr/bin/time (git 2.43.0).

Shape Walk index-pack --threads=1
Deltas whose base is itself a large delta, so pruned bases are rebuilt: a 50 MiB zero base, a chain of 19 OFS deltas of 50 MiB (each copies its base and inserts a byte), and a 1-byte leaf delta on every link. 52,193 B pack taken, spent 1,048,616,905, in 46 ms exit 0, 18.61 s, 106,224 KB
The same with 9 links of 100 MiB. 102,632 B pack taken in 118 ms exit 0, 9.85 s, 208,868 KB
Many tiny entries: a chain of 900,000 OFS deltas, each a 4-byte target. 14,400,045 B pack taken, spent 930,601,028, in 4.62 s, 3.49 s and 3.49 s (3 runs), 137 to 154 MiB exit 0, 1.85 s, 172,700 KB
The same, through baseTree — refused at the binding check after 6.2 s in all; node peaked at 185 MiB. Container removed, nothing kept
zlib data that inflates to almost nothing: one 4-byte blob whose stream is 280 MB of empty fixed-Huffman blocks (224 million of them). 280,000,048 B pack taken, spent 1,028, in 1.67 s at 337 MiB exit 0, 2.27 s, 3,828 KB
zlib with a window over 32 KiB (CINFO=8), and with FDICT set both refused as not well formed, in 1.9 ms and 1.5 ms —

What this shows:

  • Memory: every admitted shape kept index-pack --threads=1 at or under 209 MB. That fits the body's "at most about the 1 GiB budget" (measured on these shapes; inferred for others).
  • index-pack time: the rebuild shape takes index-pack 18.6 s for a 51 KB pack. That is within the stated bound, the 30-minute git timeout (measured).
    • Why it is slow (inferred from git's base-cache pruning): the links are bigger than half of git's 96 MiB delta base cache, so pruned bases are rebuilt from the root. That cost grows with the square of the budget over the cache size, not with what was charged.
    • So the doc comment on packDeclaredBytes ("what index-pack hashes and builds, are bounded by it") is not strictly true for what index-pack builds. Non-blocking.
  • Walk time: the 900,000-link chain took the walk 3.5 to 4.6 s. That is over the README's "The walk itself takes at most about 3 s (2.63 s for a million tiny objects, measured)". See required change 2.

3. Changed lines in this PR's own test/bench-pull-pack.test.ts (weakening gate)

From git diff 92a8de80 410ef531 -- test/ (measured). The only existing test file changed is bench-pull-pack.test.ts; bench-pull-budget.test.ts is new.

  • The Walk type and every call: made async (await f(...), .decoded, assert.rejects). Each assertion is the same.
  • "Exactly at the cap: taken": the cap is now spent, not gitSizes, and there is a new assert.ok(spent >= decoded). Since spent >= decoded, the at-cap and cap-minus-one checks are at least as strict as before (inferred).
  • The "delta header cut short" case: it now has a real base, entryHeader(6, 1) with ofs(blob.length), where before it had entryHeader(6, 3) and ofs(1).
    • The old case declared 3 bytes for 1 byte of data, so it was refused on its size and never reached the delta header. The new one reaches it, and with the round's new offset check, ofs(1) would now be refused anyway (inferred from the code).
    • This is a fix, not a loosening.
  • The message "a delta's target counts, not its delta data" became "...not only its delta data". The regex and the assertion are the same.

None of these is loosened. Against main, every test file in the PR is still new, so no line of main's tests changes (measured, gh pr view --json files).

4. Fail-without (re-measured)

  • Setup: test/bench-pull-budget.test.ts was written as a NEW file, with the Write tool, into a fresh worktree at 92a8de80. Its sha256 is eff52ce0…, identical to the head's. The changed bench-pull-pack.test.ts already exists at 92a8de8, so its fail-without is builder-measured.
  • Command: node --import ./dist/test/isolate.js --test dist/test/bench-pull-budget.test.js, with a temp HOME, TMPDIR and state.
  • Base result (measured): # tests 6, # fail 5. Each failure is on behaviour:
    • 1: Missing expected rejection. (33 large deltas taken);
    • 2: Missing expected rejection. (tiny entries taken);
    • 3: Missing expected rejection: a wrong trailer;
    • 5: index-pack on one thread, with an actual of ... index-pack --stdin;
    • 6: the README match.
    • Test 4 (the streamed path's size checks) passes at the base. The commit message says so: it is kept as coverage.
  • At the head (measured): 98/98 across bench-pull-budget, bench-pull-pack, bench-pull, bench-pull-bounds, bench-cli, bench-images, bench-pins, bench-select and bench-select-lookalike.

5. Other checks

  • Typecheck: clean at the head (measured).
  • check:denylist: 1415 files clean (measured).
  • Package size: this round changes only scripts/bench, test and changelog.d. None is in package.json files, so the delta is 0 bytes (inferred from the files list).
  • git merge-tree --write-tree origin/main 410ef531: clean, tree b0f6d339 (measured). origin/main 9f9533aa is not an ancestor of the head (measured).
  • Commit 410ef531: signed off, with no Co-Authored-By or "Generated with" line (measured).
  • CI at the head (measured, gh pr checks 947, not rerun):
    • failed: test shard 3/4 (ubuntu-latest, 22) and (ubuntu-latest, 24). Both fail one test, no test file measures a new duration on the wall clock (Date.now() minus a start), with 'test/bench-pull-budget.test.ts: 1, allowed 0' and 'test/bench-pull-pack.test.ts: 1, allowed 0' (from the job logs). I reproduced it locally at the head with dist/test/no-wall-clock-durations.test.js: the same two lines (measured);
    • passed: agentshield, dco, denylist, desktop, new-deps, push-gate, scope, scan ×2, and shards 1/4, 2/4 and 4/4 on 22 and 24;
    • skipped: the real-systemd jobs.
    • The bench-pull-pack.test.ts instance (Date.now() - started, line 120) was already there at 92a8de8, and so was the guard (measured, git grep and ls-tree). Round 3's review missed it. This round adds a second one, at bench-pull-budget.test.ts:134.

Required changes

  1. Make CI green: measure the two durations with performance.now() (principle 7: a guard is red).
    • The places: test/bench-pull-budget.test.ts:132-134 and test/bench-pull-pack.test.ts:113,120.
    • Don't add them to ALLOWED in test/no-wall-clock-durations.test.ts. That would be an exception to a guard, a loosening under principle 9, and it needs the owner.
  2. Correct the walk-time figure (principle 5).
    • README step 10 says "The walk itself takes at most about 3 s (2.63 s for a million tiny objects, measured)".
    • An admitted chain of 900,000 tiny OFS deltas (14.4 MB pack, spent 930,601,028) took the walk 4.62, 3.49 and 3.49 s here (measured).
    • Either state the measured figure with its shape, or say it is about 5 s on this host and label the bound inferred.
    • If the "Adversarial pass" table or "Compromised component" generalises "the walk took at most 2.63 s" beyond the shapes tried, label it the same way.

Non-blocking:

  • The packDeclaredBytes doc comment says "what index-pack hashes and builds" is bounded by the budget. With links over half of git's base cache, index-pack rebuilds pruned bases: 18.6 s for an admitted 51 KB pack (measured), work that grows faster than the charge (inferred). Memory stays low (106 MB) and time stays inside the 30-minute timeout, so this is only wording: say that hashing and memory follow the budget, and time is bounded by the timeout.

Probes (scratch only): r4-probe.mjs, mkpacks.mjs and run-node.sh in the evaluator's scratch folder work/delta-947-r4/. They reuse round 3's packs in place (work/eval-947-d3/tmp/) and work/eval-947-d2/fake-docker.mjs.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…ime claims match what was measured

The no-wall-clock-durations guard (#582) flagged two elapsed times read off Date.now() in this PR's new tests
(bench-pull-budget and bench-pull-pack), which turned CI shard 3/4 red. They now use performance.now(), which
an NTP step can't move. The guard's ALLOWED list is unchanged.

Review of round 4 measured a 900,000-link chain of tiny deltas (a 14.4 MB pack) taking the walk 3.5 to 4.6 s,
over the README's "at most about 3 s", and 19 chained 50 MiB deltas (51 KB) making index-pack --threads=1
rebuild bases for 18.6 s at 106 MB. README step 10 and packDeclaredBytes' doc now give those figures: the walk
reads at most a million entries and inflates at most 1 GiB, a bound in seconds is inferred, and index-pack's
memory follows the budget while its time is bounded by the 30-minute git timeout.

Test (fails without): test/no-wall-clock-durations.test.ts, red in CI on 410ef53 (shard 3/4, Node 22 and 24).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (delta final check, head 1f30402; PR body only)

I reviewed head 1f30402b5858e463cf964c9623a311c29659d5d1 against the previous reviewed head 410ef531. Main is 9f9533aa, which the head has merged (be1167c7).

The code is done. CI is green, the guard passes with its ALLOWED list unchanged, and the README and doc-comment figures match what I re-measured. One claim is left in the PR body: "Compromised component" still says "the walk took at most 2.63 s". Round 4 asked for that to be corrected too. It needs a body edit only, not a new head.

All probes ran in scratch only: temp HOME, TMPDIR, state and store, with stand-ins (exit 1) first on PATH. Host git ran only in temp folders. No real docker, no network pulls, and the real bench store was never touched.

1. The round-4 findings

Finding Result
1. CI red from no-wall-clock-durations Fixed (measured). Both places now use performance.now(): test/bench-pull-budget.test.ts:132-135 and test/bench-pull-pack.test.ts:113-121. git diff origin/main 1f30402b -- test/no-wall-clock-durations.test.ts src/ .github/ is empty, so ALLOWED is unchanged against main. The guard passes locally at the head and in CI (see section 6).
2. Walk-time figure in README step 10 Fixed in the README (measured); one body line left. README step 10 drops "at most about 3 s". It now gives the longest measured runs with their shapes, and labels a bound in seconds inferred. Body line "Round 4 ... The walk's longest admitted run here was 2.63 s" carries a round-5 correction. But "Compromised component", under "What an admitted pack can still cost", still says "the walk took at most 2.63 s" with no correction. See required change 1.
3. (non-blocking) packDeclaredBytes doc comment Done (measured). It now says index-pack's memory follows the budget and its time does not, and that the 30-minute git timeout bounds the time. README step 10 says the same.

2. Figures re-measured on the round-4 packs, in place (no copies)

I ran the round-4 evaluator's r4-probe.mjs against the head's dist, on the packs in work/delta-947-r4/tmp/. The host was loaded: load average 12 to 15 on 16 CPUs (measured, uptime).

Claim (README step 10 / doc comment) Re-measured at 1f30402
Walk: 3.5 to 4.6 s for a chain of 900,000 tiny deltas, a 14.4 MB pack deepchain-900k.pack (14,400,045 B): taken, spent 930,601,028, in 3.10 s, 5.78 s and 3.60 s (3 runs), at 134 to 162 MiB. The README labels these as the longest runs measured in review, not as a bound, and says a bound in seconds is inferred. So the 5.78 s run, on a loaded host, contradicts no claim (inferred).
index-pack --threads=1: 18.6 s at 106 MB for 19 chained 50 MiB deltas, a 51 KB pack chainleaves-19x50.pack (52,193 B): exit 0, 18.23 s, maxrss 106,152 KB (GNU time's KB are KiB, so 103.7 MiB or 108.7 MB). The figure matches. Walking the same pack: taken in 70 ms.
The walk reads at most a million entries pull.ts:162-163: spent = count * ENTRY_COST (1,024), refused if over treeBytes (1 GiB), before any entry is read. That allows at most 1,048,576 entries, so "a million" is a rounded figure (inferred from the code).

3. The merge commit

  • git merge-tree --write-tree 410ef531 9f9533aa gives b0f6d339043d8d248a27c2fcd9aa79153a477d46. That equals be1167c7^{tree}, so the merge re-derives exactly and has no hand edits (measured).
  • git merge-tree --write-tree origin/main 1f30402b is clean: tree 403d0ff3, the head's own tree. origin/main is still 9f9533aa (measured).

4. Fail-without (re-measured)

  • The builder's fail-without is the existing guard, not a new test file. So I built a fresh worktree at 410ef531, with no file written, and ran node --import ./dist/test/isolate.js --test dist/test/no-wall-clock-durations.test.js in a temp env.
  • Result at 410ef53 (measured): # fail 1, with 'test/bench-pull-budget.test.ts: 1, allowed 0' and 'test/bench-pull-pack.test.ts: 1, allowed 0'.
  • At the head (measured): it passes. Run together with the 9 bench files (bench-pull-budget, bench-pull-pack, bench-pull, bench-pull-bounds, bench-cli, bench-images, bench-pins, bench-select and bench-select-lookalike), the result is # tests 100, # pass 100, # fail 0. That matches the body.

5. Weakening gate: changed test lines this round

This round changes lines in two test files. Both are new against main, so no line of main's tests changes (measured, git diff --name-only origin/main 1f30402b).

  • bench-pull-budget.test.ts: Date.now() became performance.now(), and ms is now Math.round(performance.now() - started). The threshold is unchanged at < 20_000. Rounding can only push a value up to the limit, never under it, so this is not looser (inferred).
  • bench-pull-pack.test.ts: Date.now() became performance.now(). The threshold is unchanged at < 30_000, and so is the message.

None is loosened. A cosmetic nit: bench-pull-pack.test.ts:121 is missing a space in 30_000,'refused.

6. Other checks

  • Typecheck: clean at the head (measured, npm run typecheck).
  • check:denylist: denylist: 1421 files clean (measured).
  • Package size: 0 bytes against main (inferred). The PR touches only changelog.d/, scripts/bench/ and test/, and none of them is in package.json files (measured, the list and git diff --name-only).
  • Commits: 1f30402b and be1167c7 are signed off, with no Co-Authored-By or "Generated with" line (measured).
  • Protection: the PR is marked protected, with the title, needs-owner and the risk class. This round touches no src/ file and no guard file (measured).
  • CI at the head (measured, gh pr checks 947, not rerun):
    • passed: test shards 1/4 to 4/4 on Node 22 and 24, including shard 3/4 on both; also agentshield, dco, denylist, desktop, new-deps, push-gate, scope and scan ×2;
    • also passed: the aggregate test (ubuntu-latest, 22) and test (ubuntu-latest, 24) jobs;
    • skipped: the real-systemd jobs, bootstrap and the sign-in probe;
    • nothing failed and nothing is pending.

Required changes

  1. PR body only (principle 5): correct the walk-time line in "Compromised component".
    • Under "What an admitted pack can still cost", it says "the walk took at most 2.63 s". That is the generalisation round 4's change 2 asked to correct.
    • Measured against it: a 900,000-link chain took the walk 3.5 to 4.6 s in round 4, and 3.10 to 5.78 s here on a loaded host.
    • Make it match README step 10: the longest measured runs with their shapes, and a bound in seconds labelled inferred.
    • No code change and no new head are needed.

Non-blocking:

  • "Checks removed or weakened" has no round-5 entry for the Date.now() to performance.now() lines. They aren't loosened, but the section lists every other round's test-line changes.
  • "106 MB" is GNU time's 106,152 KiB, which is 108.7 MB. Also, the README could note the 5.78 s run on a loaded host. Neither is needed.

Probes (scratch only): run-node.sh in the evaluator's scratch folder work/delta-947-r5/. It ran round 4's work/delta-947-r4/r4-probe.mjs against the head's dist, on round 4's packs in place.

@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: PASS (delta final check follow-up, head 1f30402)

The head is still 1f30402b5858e463cf964c9623a311c29659d5d1 (measured, gh pr view 947 --json headRefOid). I re-read the live PR body and checked the three body fixes. The code checks in my previous comment still stand, since the head is unchanged: CI green, 100/100 tests, typecheck, denylist and the merge re-derivation.

Item Result (measured, from the live body)
"Compromised component" walk-time line, the required change Fixed. "The walk took at most 2.63 s" is gone. The line now gives the longest measured runs with their shapes: 3.5 to 4.6 s in review, 3.1 to 5.8 s on a loaded host, and 2.63 s for a million tiny blobs. It labels any bound in seconds inferred. That matches README step 10 and my measurements.
A Round 5 bullet under "Checks removed or weakened" Added. It lists both Date.now() to performance.now() changes, with thresholds unchanged (20 s and 30 s), and says the guard's ALLOWED list is unchanged. That matches the diff.
The 106,152 KiB figure Fixed in the body. It now reads "106,152 KiB, about 109 MB". 106,152 × 1,024 = 108.7 MB.

Non-blocking, no new head needed:

  • The Round 5 bullet says Math.round is "for the message". In the code, the rounded ms also feeds the < 20_000 check. Rounding can only push a value up to the limit, so this is not a loosening (inferred).
  • The figures mix two review runs: 18.6 s comes from round 4, and 106,152 KiB from round 5 (round 4 measured 106,224 KB). Both were measured.
  • README step 10 at the head still says "106 MB". I noted that as non-blocking before.
  • The adversarial-pass table row for round 4 still says "the walk at most 2.63 s". It is scoped to round 4's shapes, which the row lists, so it is accurate as it stands (inferred).

No required changes remain.

@jayden1711
jayden1711 merged commit 40ef312 into main Oct 21, 2026
27 of 28 checks passed
@jayden1711
jayden1711 deleted the bench-slice4-pull branch October 21, 2026 08:48
jayden1711 added a commit that referenced this pull request Oct 21, 2026
No conflict. #947 brought scripts/bench/pull.ts's git helper, a raw execFileSync('git', [...GIT_HARDENED, ...args],
{ env: GIT_ENV }) that the runner scan refuses (measured on this merge before the fix: "every git call in src/ and
scripts/ goes through the runner" fails with "scripts/bench/pull.ts: starts git 1 time(s) (not on the list)").
It now runs through gitExecFileSync with both lists kept: GIT_HARDENED as arguments after the runner's, and GIT_ENV
as the env, over which the runner sets its own settings last (gitExecFileSync applies gitEnv). Not added to
OUTSIDE_THE_RUNNER, which is never-grow.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
jayden1711 added a commit that referenced this pull request Oct 21, 2026
main brought #947 and #899. One conflict, in src/review.ts: the
HARNESS_CONFIG block from #899 (its JSDoc, HARNESS_CONFIG and the
never-shrink HARNESS_CONFIG_GLOBS) and this PR's decision-record block
(DECISION_FILES, DECISION_DIRS, DECISION_RECORD_GLOBS and onePartPerGlob)
were both added after hidesSubmodules. Both are kept whole: this PR's
block first, then the one from #899, then SELF_MODIFICATION as before.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant