Skip to content

Bench: pinned dataset and image downloads, verified before use (#748 slice 2) [PROTECTED: owner merges] - #863

Merged
jayden1711 merged 17 commits into
mainfrom
issue-748-bench-pins
Oct 21, 2026
Merged

jayden1711 merged 17 commits into
mainfrom
issue-748-bench-pins

Conversation

@jayden1711

@jayden1711 jayden1711 commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner

Intent: slice 2 of #748. It adds bench download, which fetches the SWE-bench Verified file at a pinned revision and sha256 into the 0700 held-out store, and it checks the bytes before anything is kept. Task images are pulled by pinned digest only. Every pin lives in one data file, scripts/bench/pins.json, and nothing else sets one. No engine change: only scripts/bench/**, one new test file and a CHANGELOG bullet.

Stacked on #851 (slice 1, branch issue-748-bench-cli), so the diff against main shows #851's commits too. Merge this after #851. This PR's own commits:

Risk class: protected. It touches scripts/**, and the owner merges.

Refs #748 (a slice; it doesn't close the issue).

What it does

  • scripts/bench/pins.json is the only place a pin lives. It holds:

    • dataset: base, path, revision, sha256, max_bytes and hosts;
    • image_digests: digests only.
      The dataset is unpinned (null) for now, and the digest list is empty. So bench download refuses and fetches nothing, and no image can be pulled, until the owner records the pins.
  • scripts/bench/pins.ts parses that file against a closed schema. It refuses the whole file on:

    • an unknown key;
    • a revision without a sha256, or the reverse;
    • non-https, credentials, a port or a query in the url;
    • a path that climbs out of its folder;
    • 1 to 99 digests (a selection, not a set), unsorted or repeated digests;
    • anything shaped like an instance id (owner__repo-N) or an instance image name (_1776_), checked on both the raw text and the parsed value, so a JSON escape doesn't hide one.

    The url is base + revision + "/" + path, so it is at the pinned revision by construction. loadPins() reads only PINS_FILE. DatasetPin, Pins and ImageRef can only be built inside the module (a module-private token), are frozen, and are recorded in a module-private WeakSet. requirePinned refuses anything outside that set.

  • scripts/bench/download.ts downloadDataset works in this order:

    1. It checks the pin is genuine and runs the preflight's held-out check. heldOutCheck moved, unchanged, from cli.ts to store.ts; cli.ts re-exports it.
    2. A matching file already at dataset/<sha256>-<name> is kept and nothing is fetched. A file that doesn't match, or a link at that name, is refused and left in place.
    3. It fetches into memory, over https only, through at most 5 redirects. Each redirect must go to a pinned host or its subdomain, with no credentials and no port. It reads at most max_bytes, and the sha256 must match. Nothing is made or written before this point.
    4. It makes each store folder at 0700, one at a time, with no link, then runs the check again.
    5. It writes an O_CREAT|O_EXCL|O_NOFOLLOW 0600 temporary file, fsyncs it and reads it back through a fresh O_NOFOLLOW descriptor. It renames the file only if the read-back matches; any failure removes the temporary file.

    The fetcher is injectable. The default is Node's own fetch with redirect: 'manual'; the redirect checks are in our code, not the fetcher's.

  • download.ts pullImage sends only docker pull <name>@sha256:<64 hex>, then docker image inspect --format {{json .RepoDigests}} <same> to read the digest back. checkImageArgs refuses every other argv. The argv is built only from an ImageRef, which pinnedImage makes only from a digest in the pins: no tag, no port, no option. Docker gets container.ts dockerEnv (credential-free, with an absolute DOCKER_HOST). container.ts changed only to export spawnDocker.

  • cli.ts: bench download takes no argument (download --sha256 x exits 2), as the bench user only. main returns number | Promise<number>, and the script entry handles both.

  • README: a new step 7, "Still to come" updated.

  • Release note: changelog.d/863-bench-pinned-downloads.md, under "Guardrail changes: what each one does". Since the changelog.d conversion (Release notes: one changelog.d file per change, moved into the dated section by the changelog step [PROTECTED: owner merges] #870, merged at 8e74e4e through bench: the bench command and bench preflight, a pass/fail line per check with no secret (#748 slice 1) [PROTECTED: owner merges] #851's 6441aba), CHANGELOG.md is unchanged here: its diff against 6441aba is empty (measured). The fragment's five note lines are byte-identical to the five lines this PR added to CHANGELOG.md before (sha256 compared, measured).

How it was verified

  • Fail-without (measured): I wrote test/bench-pins.test.ts first, built it and ran it on the unchanged tree (9ab22b4) with node --import ./dist/test/isolate.js --test dist/test/bench-pins.test.js: # tests 13 / # pass 0 / # fail 13 at that time.

    • The adversarial-pass test, and the prototype-made-pin case added to test 3, failed on my first version of pins.ts (# tests 14 / # pass 12 / # fail 2) before the fix.
    • Correction: the file had 14 tests at f4fc274, not 13 as the first commit message says. The reviewer measured 14 of 14 failing on 9ab22b4.
  • Round 1 (measured): the three new round-1 tests failed on b61f0fa (# tests 17 / # pass 14 / # fail 3):

    • a repeated image_digests key parsed;
    • a %5F%5F base parsed;
    • docker was sent pull evil.example.net/other@sha256:<unpinned>.

    All 17 pass at 3cbe2df.

  • After round 1 and the merge of bench: the bench command and bench preflight, a pass/fail line per check with no secret (#748 slice 1) [PROTECTED: owner merges] #851 4313ef8 (main d9855a9), measured at 2a3dcab: bench-pins + bench-cli + bench + changelog: # tests 107 / # pass 107 / # fail 0. npm run typecheck: exit 0. npm run check:denylist: "1347 files clean". DCO: all commits signed off.

  • After (measured): node --import ./dist/test/isolate.js --test dist/test/bench-pins.test.js dist/test/bench-cli.test.js dist/test/bench.test.js: # tests 94 / # pass 94 / # fail 0. After two more redirect cases were added, bench-pins alone gave # tests 14 / # pass 14. dist/test/changelog.test.js: 10/10 pass.

  • npm run typecheck: exit 0 (measured). npm run check:denylist: "denylist: 1339 files clean" (measured). node scripts/check-dco.mjs origin/main HEAD: "DCO: all commits signed off" (measured). The push gate reported "2 commit(s) and 1 tip(s) clean" (measured).

  • No network, no docker, no real state (inferred from the test code, which I read):

    • every fetch goes through an in-memory stand-in that records URLs;
    • every docker call goes through a recording function, with PATH /nonexistent;
    • every store is under the isolate preload's temp machine, and the file imports ./helpers.js, which enforces the preload.
    • The real httpsFetcher is not exercised by any test, since that would need the network.
  • Not run locally: fullsuite.sh. Some suite files set PATH to /usr/bin without stand-ins for every host tool, and this PR touches none of what they cover. CI runs the full suite.

New dependencies

None. It uses node:crypto, node:fs, node:path and node:url, plus Node's built-in fetch for the real fetcher.

What this can never do

Each item is inferred from the code and backed by the named test (measured).

Scope: each "never" holds against input from outside this process's code: the pins file, the network, the filesystem, the command line and the environment. It does not hold against code running in the same process. Such code could swap a built-in (the reviewer measured that swapping Array.prototype.includes makes pinnedImage build a genuine ImageRef for an unpinned digest), or call spawnDocker or fs directly. The bench runs only its own code in this process (inferred).

  • Keep or write a dataset byte that doesn't match the pinned sha256. The hash is checked in memory before anything is made, again in writeVerified before the write, and on read-back before the rename (test "bytes that do not match…").
  • Make the store, a folder or a temporary file when the download fails or doesn't match (same test, plus "a connection cut" and "over the cap").
  • Write through a link, or follow one: a linked state, store, dataset/ folder or file name is refused, and nothing appears where the link points (two tests). The temporary file is O_EXCL|O_NOFOLLOW.
  • Chmod or replace a store that fails the held-out check. A 0750 store stays 0750 and nothing is fetched.
  • Fetch over http, from an unpinned host, with credentials or a port in the url, or through more than 5 redirects (redirect test, 12 refused cases).
  • Take a pin from anywhere but scripts/bench/pins.json: no flag, argument or variable. A hand-made, copied or prototype-made pin is refused, and a parsed one can't be changed (frozen). This holds against input from outside this process's code; see Scope.
  • Send docker anything but pull <name>@sha256:… or image inspect --format {{json .RepoDigests}} <name>@sha256:…, through pullImage. The argv recorded in the test is exactly those two. Same-process code can still call spawnDocker directly; see Scope.
  • Pull by tag, or by a digest that isn't pinned, given a reference from outside this process's code. A swapped built-in in the same process defeats it, as the reviewer measured; see Scope.
  • Put an instance id or instance image name in the pins file. The parser refuses it, even escaped.

Compromised component

  • An agent in a bench run (sandboxed, as the bench user):
    • The pins file is readable in the clone. It holds no task, image name or selection, only a dataset location and a digest set of 0 or at least 100 (inferred).
    • Its sandbox denies the held-out store (slice 1, unchanged), so it can't swap the store between this code's checks.
    • Its Edit tools are confined to the run folder (README, unchanged).
    • bench download runs before any agent run, not during one (inferred: no driver code calls it).
  • The download server, or a man in the middle on a pinned host: can deny service, or serve different bytes. Different bytes fail the sha256 and nothing is written. A redirect elsewhere is refused. Memory is capped by max_bytes (256 MiB in the repo's file).
  • A registry serving a different image for a digest: docker verifies content against the digest (inferred, Docker's content addressing), and we read the digest back with image inspect.
  • Someone with push access, or a PR author: could change pins.json to a hostile url and a matching sha256. That is a reviewed change to a protected path, which the owner merges. The parser still refuses non-https, unknown keys and task-shaped text, but cannot judge whether a pinned file is the right one. That is the owner's review.
  • Another local user: the store is 0700 and the file 0600, created with O_EXCL. The download refuses a store another uid owns.
  • The coordinator, dashboard, hub viewer and other instances: not involved. Nothing in src/ changed, and scripts/bench/ isn't packed (see Package size).

Adversarial pass

Every attack below is measured by a test in test/bench-pins.test.ts unless marked otherwise.

  • A pin built past the constructor. Object.create(DatasetPin.prototype) plus the fields passes instanceof. My first version only checked instanceof, so this attack got through (test failed, measured). Fixed: genuine pins are recorded in a module-private WeakSet, and requirePinned checks that set. Now refused.

  • Adding a digest after parsing. pins.digests was a mutable Set, so this got through. Fixed: it is now a frozen array. The pin objects and hosts are frozen too, and writes throw TypeError.

  • An instance id behind a JSON escape (django__django-11099). The raw-text scan missed it, so this got through. Fixed: the scan now also runs on the parsed value.

  • Round 1 (the reviewer's findings, all fixed by design, each test shown failing on b61f0fa):

    • A repeated JSON key hid an unchecked copy. The file must now equal its canonical form, JSON.stringify(pins, null, 2) plus a newline.
    • A percent-escaped id in base got through. base must now equal new URL(base).href, and each path segment is a plain name.
    • A swapped ImageRef.prototype.toString changed the pull argv. The argv now comes from the reference's own frozen fields, and the prototypes are frozen.
    • The test fixtures named a real instance. They now use the synthetic example__proj-4242. A grep of the whole branch diff, README included, finds no other.
  • Redirect tricks. All refused:

    • https://evil<host>;
    • https://<host>.elsewhere.example.net;
    • backslash urls;
    • elsewhere\@<host>;
    • http, credentials, a port, no Location;
    • a redirect loop: 6 requests, then refused.
  • Image references. All refused:

    • a tag only;
    • a tag beside a digest;
    • upper case, a short digest, sha512;
    • localhost:5000/…;
    • a leading space, a trailing newline;
    • --platform=x@…;
    • an unpinned digest;
    • any image while the digest list is empty.

    Docker argv refused: pull --platform, pull --all-tags, image rm, run, tag, an inspect format other than RepoDigests.

  • Store attacks. All refused, with nothing fetched:

    • a 0750 store;
    • a linked state;
    • a linked store;
    • a linked dataset/;
    • a link at the file name (the file it names is untouched);
    • a wrong file already at the name (left as it is).
  • A pin override from the command line: download --sha256 <x> exits 2, and nothing is fetched.

  • A leak through shell history (design choice, inferred): no command takes an image name as an argument, because the bench user's shell history isn't on the sandbox's read-deny list and an image name carries its instance id. Task selection (slice 3) will hand references over from the store.

  • Not defended (inferred): a process running as the bench user that swaps a store folder for a link between makeStore's checks and the rename. Node has no openat, so each step is checked by path. A final check after the rename catches a swapped store, but the bytes would already be there. Only the bench user can do this, the agents' sandbox denies the store, and the download runs before agents do.

Package size

Delta: 0 bytes (measured). I ran npm pack --dry-run --json (through npmw.sh) on this branch, and on a separate worktree at main db2e3df (this branch's merge base with main), each after npx tsc:

  • both: 477 entries, 4,732,443 bytes unpacked, 1,409,618 bytes packed;
  • dist/web isn't built in either, and neither side touches web/.

Per file: scripts/bench/{pins.json,pins.ts,download.ts,cli.ts,container.ts,store.ts,README.md}, test/bench-pins.test.ts and CHANGELOG.md are all not packed. The pack list has no entry under scripts/bench, dist/scripts, dist/test, test/ or CHANGELOG (measured), so every one is +0. No budget change.

For the owner

  • Pinning the dataset is your call, and needs your OK to download (slice plan), on the bench machine only. Record revision (40 hex) and sha256 in pins.json in a reviewed change. Then run bench download as wl-bench.
    • The base and path in the file are my guess at the upstream location (princeton-nlp/SWE-bench_Verified, data/test-00000-of-00001.parquet). Check them when you pin.
    • The hosts (huggingface.co, hf.co) are a guess at where its downloads redirect. If a redirect is refused, it names the host.
  • Format (inferred): if the upstream file is parquet, store.ts parseDataset (JSON lines) can't read it. Slice 3 (selection) has to settle that, with no new dependency. This PR only stores the verified bytes.
  • Image digests: the list is empty. Filling it (a set of at least 100, the whole dataset's images, never a selection) and handing each task's name@digest over from the held-out store belong to slices 3 and 4.
  • Proposed, not done here: container.ts's IMAGE pattern for docker create/run still accepts a tag. test/bench.test.ts uses swebench/example:latest. Tightening it to digest-only (an ImageRef) belongs in slice 4, where task containers get their images from pinnedImage. I left it as is, since this lane owns only new test files and it isn't a loosening.

…ne per check and no secret (#748)

The owner set up the bench user on 2026-10-18 and runs the remaining steps once there is a command to check them.
`node dist/scripts/bench/cli.js preflight` (a `bench` wrapper in the README) runs the driver's model-free,
container-free checks as the bench user and prints PASS, FAIL or SKIP for each: bench user, home, bench root,
held-out store (absent, or a 0700 folder of this user, never a link), research-on-demand off, the claude pin, the
claude.ai login (method and plan only), the srt probe, a rootless daemon through an absolute DOCKER_HOST, and the
container limits. It exits 0 only when every check passes. When the user or home check fails nothing else runs: as
another user it would ask that user's claude about that user's login and probe that user's home.

It starts no container, makes no model call, downloads nothing and keeps nothing on disk. Every printed line goes
through scrub(): values of variables that may hold a credential or switch the login, token-shaped strings and email
addresses are redacted, control characters dropped, each detail capped.

requireBenchUser now refuses root (uid 0 or the name root) even when the bench user variable names it: before, setting
it to root made root the bench user.

The README's setup gains the srt install from a checksum-pinned tarball, the bench clone at a pinned commit, the
wrapper, and the preflight run as `sudo -iu wl-bench` then `bench preflight`.

Test (fails without): test/bench-cli.test.ts. Written first and run on the unchanged tree: 9 of 9 fail (8 with
ERR_MODULE_NOT_FOUND for scripts/bench/cli.js, the root test with "Missing expected exception").

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Main's v0.5.3 heading is still dated; the bullet goes there per the lane rule (one unreleased section, no v0.5.4).

Test (fails without): none; a changelog line only.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…LOG bullet into v0.5.3 (unreleased)

Main's v0.5.3 is undated again and its generated PR lists are gone until the release (54b3eb1). The conflict was
this branch's bullet inside those lists: they are dropped as on main, and the bullet is now a hand-written note at
the end of "Guardrail changes: what each one does", above the section's closing empty "### Guardrail changes". No
code change.

Test (fails without): none new; test/changelog.test.ts, bench-cli and bench pass on the merged tree (85 of 85).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…, secrets scrubbed whatever cut them, README steps fail closed

The #851 round-1 review measured four gaps, each fixed by design:
- The held-out check lstat'ed only the store itself, so a linked <root>/state passed, made or not, and 0500, 02700,
  01700 and 04700 passed as "0700". Every folder from the bench root down is now lstat'ed (a link anywhere fails,
  whether or not the store is made), the real path must equal the written one, and the mode must be exactly 0700.
- The driver's checks cut a child's stderr at 200 or 300 characters before the preflight saw it, so a secret
  straddling the cut printed its first characters. main() now wraps the one output sink in scrub(), which runs on
  the full line before any cut, and scrub() also redacts every piece of 6 or more characters that starts or ends a
  secret value, so a value cut short anywhere upstream prints nothing of itself. Paths and socket addresses are
  redacted whole only. An email needs a letters-only last label, so `sandbox-runtime@0.0.79` stays readable.
- README step 4 ran `npm install` on a checksum mismatch or with no openssl when pasted as a block. The install is
  now chained on the check, with --ignore-scripts. Step 5's placeholders held an apostrophe and a `<` (bash waited
  for a quote, or read a redirect); they are plain words now, and every command is chained on the pinned commit.
- test/bench-cli.test.ts imports ./helpers.js, the isolation-preload guard.
Also: requireBenchUser refuses an unknown uid (-1, not an integer) and a root effective uid.

Test (fails without): test/bench-cli.test.ts. The five new tests, written first and run on e8d99f4: 5 of 14 fail
(uid -1 "Missing expected exception"; linked state "'PASS' !== 'FAIL'"; a piece of the secret printed;
step 5 "unexpected EOF while looking for matching `''"; step 4 "a checksum mismatch installed srt"), the 9 others pass.
The secret test's canary is a plainly fake, low-entropy value; with it, on e8d99f4 the piece "benchc" printed.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…ELOG bullets

Main's #855 bullet and this branch's #851 bullet were appended at the same spot, the end of "Guardrail changes: what
each one does" in v0.5.3 (unreleased). Both are kept, main's first, neither reworded. No code change.

Test (fails without): none new; test/changelog.test.ts passes on the merged tree.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…test runs its bash in its own temp folder

Round-2 review notes. `git clone` refuses a non-empty ~/src/bench-engine and the chained step then stops, so the
README says to move an earlier clone aside first. The step-4 test's bash now starts in the test's temp base, so
nothing the block does before its own `cd` lands in the test runner's folder.

Test (fails without): none; a README line and a test's working folder. bench-cli, bench and changelog pass (90 of 90).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…ore, and images are pulled by digest only

Slice 2 of #748. The smoke run needs the SWE-bench Verified file and the task images on the bench machine, and
both must be exactly what was reviewed: a moved branch, a changed file or a retagged image would change the
benchmark without anyone seeing it.

- scripts/bench/pins.json is the one place a pin lives: the dataset's revision and sha256, and a set of image
  digests. pins.ts parses it on a closed schema. An unknown key, a half pin, or anything shaped like an instance
  id or an instance image name refuses the whole file, even behind a JSON escape: the agents can read this clone.
  Only pins.ts builds a pin, frozen, and the download and the pull refuse anything it didn't build (a module-private
  WeakSet, so a copy or an object made on the class's prototype is refused too). The dataset is unpinned (null)
  until the owner records it, and until then bench download fetches nothing.
- download.ts downloadDataset: refuses a store that fails the preflight's held-out check (moved from cli.ts to
  store.ts so both use it), fetches into memory over https from the pinned hosts only (at most five redirects, each
  checked), checks the sha256, and only then makes the store (0700, one folder at a time, no link) and writes a new
  O_EXCL|O_NOFOLLOW 0600 temporary file. It reads that file back and renames it only when the read-back matches.
  It never follows a link, and never chmods or replaces what it finds.
- download.ts pullImage: `docker pull <name>@sha256:<digest>` then `docker image inspect` to read the digest
  back, nothing else (checkImageArgs), with the driver's credential-free docker environment. The argv is built
  only from an ImageRef, which pinnedImage makes from a digest in the pins and never from a tag.
- cli.ts: `bench download`, which takes no argument (no flag sets a pin).

Test (fails without): test/bench-pins.test.ts, 13 of 13 tests failed on the tree before this change; the two
adversarial-pass tests added after (a prototype-made pin, a mutated digest list, an escaped instance id) failed on
the first version of pins.ts.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
One bullet at the start of v0.5.3's "Guardrail changes: what each one does", so it doesn't collide with the
bullets other PRs append at the end.

Test (fails without): none; a changelog entry (test/changelog.test.ts passes with it).

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

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (round 1/2)

Independent final review of head b61f0fab. Only the two commits on top of #851's 9ab22b4f were reviewed: f4fc274d and b61f0fab. Probes used stand-ins only: the injectable fetcher, a recording docker function, and temp dirs with a temp HOME and PATH=/nonexistent. There was no network, no docker and no default fetcher.

Acceptance (slice 2 scope)

Item Result Evidence
sha256 verified before anything lands met measured: a mismatch, a cut connection or a body over the cap leaves no bench root (test 6). Recorded fs calls show the first mkdir only after fetchPinned returns.
temp file O_EXCL|O_NOFOLLOW 0600, rename only after read-back met measured: recorded openSync flags 131265 = O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, mode 0600. Order: write, fsync, open O_RDONLY|O_NOFOLLOW, read, rename. With read-back bytes flipped there is no rename, the temp is removed and dataset/ is empty.
store folders 0700, one at a time, never through a link met measured: 6 mkdirSync calls, each {mode: 0o700}, none recursive. A linked state, evals, store or dataset/ is refused, with nothing fetched and nothing written behind the link.
wrong file / link at the name refused met measured: wrong bytes, a 0644 file with the right bytes, a folder, a link and a dangling link are all refused and left in place.
redirects ≤5, pinned hosts, size cap met measured: 5 redirects then 200 is accepted (6 requests). 6 redirects are refused after 6 requests. Refused: %2e in the host, scheme-relative, IP, trailing dot, host in path/query/userinfo, file:, data:, punycode lookalike. An endless body stops at 17 × 4 KiB chunks for a 64 KiB cap.
pins: unknown keys, half pins, bad hex, non-https, credentials, port, query, traversal, unsorted/invalid digests partly measured: every direct case is refused. Duplicate JSON keys get around the closed schema (required change 1).
pins: instance-id-shaped text, escaped forms partly measured: plain and _ forms are refused. A percent-escaped id in base is accepted (required change 2).
WeakSet gate refuses hand-built pins met measured: refused for {...pin}, structuredClone, a JSON round-trip, Object.create(DatasetPin.prototype), Object.create(genuinePin), a Proxy, Reflect.construct, a subclass, and new Pins/new ImageRef with a fresh symbol. Nothing was fetched.
images: name@sha256:<64 hex> with a pinned digest; argv only pull / image inspect partly measured: tags, name:tag@sha256, upper-case hex or algorithm, whitespace (space, tab, CR, LF), --/-q options, ports, IPv6 and unpinned digests are all refused, and 12 argv shapes are refused. A swapped ImageRef.prototype.toString makes pullImage send pull for an unpinned digest (required change 3).
no held-out leakage not met measured: test/bench-pins.test.ts adds a real SWE-bench Verified instance id and its real image name (required change 4).

Fail-without (method b)

measured: in a fresh worktree at 9ab22b4f I wrote test/bench-pins.test.ts (cmp-identical to the head's copy), then ran npx tsc (6 type errors, emitted) and node --import ./dist/test/isolate.js --test dist/test/bench-pins.test.js: # tests 14 / # pass 0 / # fail 14. Every test failed with ERR_MODULE_NOT_FOUND ... dist/scripts/bench/pins.js, which is unavoidable because the modules are new. On the head: bench-pins 14/14, and bench-pins + bench-cli + bench + changelog 104/104. The commit message says 13 of 13; the file now has 14 tests.

Other checks

  • npm run typecheck: exit 0. npm run check:denylist: "1339 files clean" (measured).
  • Size: npm pack --dry-run --json gives 477 entries, 1,409,618 packed and 4,732,443 unpacked on both the head and 9ab22b4f. Delta 0 (measured). scripts/bench, dist/test and CHANGELOG are not packed (measured: package.json files).
  • CHANGELOG: the bullet is under ## v0.5.3 (unreleased) › "Guardrail changes: what each one does" (measured).
  • Commits: both are signed off, with no Co-Authored-By or attribution trailer (measured).
  • Protection: touches scripts/**. The title, **Risk class:** protected, the needs-owner label and both sections are present (measured). No src/ change. Nothing installed changes, so there is no route an existing install had closed (inferred).
  • Durability rule: met (inferred from the code). The protections are by design: a module-private token plus a WeakSet for opaque, frozen pins; a digest-typed regex parser; verify-before-rename with O_EXCL/O_NOFOLLOW. There is no lint.
  • CI: only scan (gitleaks) ran. The PR is CONFLICTING (measured: mergeStateStatus DIRTY), so the pull_request CI workflow never ran. git merge-tree origin/main b61f0fab gives a conflict in CHANGELOG.md only, and 9ab22b4f (bench: the bench command and bench preflight, a pass/fail line per check with no secret (#748 slice 1) [PROTECTED: owner merges] #851) has the same conflict (measured).

Required changes

  1. Duplicate JSON keys get around the closed schema (measured). JSON.parse keeps the last copy of a key, so an earlier copy is never checked. These two files both parse:

    • {"dataset": {...}, "image_digests": [<4 digests>], "image_digests": []} (a hidden 4-digest selection);
    • {"dataset": {"selection": ["t01","t02"], "free_text": "anything"}, "dataset": {...}, "image_digests": []}.

    The PR body, README and commit say an unknown key or 1–99 digests refuse the whole file. They don't when the key is repeated. Fix by design: require the text to equal its canonical form, for example text === JSON.stringify(parsed, null, 2) + '\n', and reformat pins.json to match. Then nothing in the file's bytes goes unparsed. Add both cases to the tests.

  2. A percent-escaped instance id in dataset.base is accepted (measured). https://<host>/django%5F%5Fdjango-11099/ parses: the raw-text scan and the JSON.stringify scan both see %5F. Fix by design: hold each segment of base's path to PATH_PART, as path already is. That refuses % and ... Also require base === new URL(base).href, so :443 and /a/../b/ (both accepted now, measured) can't show up either. Add a test.

  3. The image argv comes from a prototype method (measured). With ImageRef.prototype.toString replaced, pullImage(genuineRef) sent docker pull evil.example.net/other@sha256:<unpinned>. The inspect read-back fails afterwards, but by then the pull has gone out. This needs code in the same process, so the risk is low (inferred). Still, the "can never pull a digest that isn't pinned" claim doesn't hold. Fix by design: build the argv from the frozen own fields (`${ref.name}@${ref.digest}`) and Object.freeze the three classes' prototypes. Add a test.

  4. A real instance id is in the test fixtures (measured). test/bench-pins.test.ts lines 127, 128 and 175 use django__django-11099 and sweb.eval.x86_64.django_1776_django-11099, a real SWE-bench Verified instance and its image name. The agents can read this clone. The repo's convention is a synthetic id (example__proj-4242, test/bench.test.ts:350). Use example__proj-4242 and sweb.eval.x86_64.example_1776_proj-4242; both still match TASK_SHAPE.

  5. CI has not run (measured). After bench: the bench command and bench preflight, a pass/fail line per check with no secret (#748 slice 1) [PROTECTED: owner merges] #851 resolves its CHANGELOG conflict, merge origin/main into this branch (no rebase) so the pull_request workflow runs, then report its result.

Notes (non-blocking)

  • pinnedImage accepts localhost/x@<pinned> and any registry domain. Docker treats localhost as a registry. The content is still checked by digest (inferred, Docker content addressing). Consider pinning the registry in slice 4.
  • A set of 100 or more digests can't prove it isn't a selection padded with filler (inferred). That rests on the owner's review of pins.json.
  • requirePinned doesn't check the kind of pin. A genuine Pins or ImageRef passed as a DatasetPin, or a DatasetPin passed to pullImage, still fails closed downstream (measured).
  • httpsFetcher with redirect: 'manual' is expected to hand back the 3xx with its Location in Node/undici (inferred). If it doesn't, the download fails closed on the status.

…ge argv from frozen fields, no real task id in tests

#863 round 1. Four holes the review measured, each closed by design:
- A repeated key got past the closed schema (JSON.parse keeps the last copy, so an earlier copy, such as a hidden
  4-digest selection, went unchecked). The pins file must now equal its canonical form, JSON.stringify(pins, null, 2)
  and a newline, so every byte of it is what was parsed and checked. pins.json is reformatted to it.
- A percent-escaped instance id in dataset.base got past the scan. base must now be written exactly as the url it
  is (no :443, no dot segment, no upper-case host), and each of its path segments is held to the same plain-name
  rule as dataset.path, so no percent escape is left.
- pullImage built the docker argv through ImageRef's toString, which a swapped prototype method could change: a
  pull for an unpinned digest went out before the read-back refused it. The argv now comes from the reference's own
  frozen fields, and the three pin classes' prototypes are frozen.
- test/bench-pins.test.ts named a real SWE-bench instance and its image name. They are now the repo's synthetic
  example__proj-4242 and sweb.eval.x86_64.example_1776_proj-4242. A grep of the whole branch diff against main for
  instance-id shapes and the task repos' names finds no other.

The first commit's message said 13 of 13 tests failed without it; that was the count when it was written. The file
had 14 tests at that commit (14 of 14 fail on 9ab22b4, the reviewer measured), and has 17 now.

Test (fails without): test/bench-pins.test.ts's three round-1 tests failed on b61f0fa (# tests 17 / # pass 14 /
# fail 3): a repeated image_digests parsed, a %5F%5F base parsed, and docker was sent
`pull evil.example.net/other@sha256:<unpinned>`.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…-bench-pins: keep both CHANGELOG bullets, the bench download one first in its list

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

Copy link
Copy Markdown
Owner Author

Round 1 fixes (head 2a3dcab)

I wrote each test first and ran it on b61f0fa's code: # tests 17 / # pass 14 / # fail 3 (measured). Each one failed on the attack itself.

  1. Repeated keys (3cbe2df). parsePins now refuses any text but JSON.stringify(parsed, null, 2) + '\n', checked after both instance-id scans. pins.json is reformatted to that form.
    • Tested: both of your repeated-key files, plus compact JSON, a missing final newline, an extra newline and extra spacing. All refused with "canonical" (measured).
  2. A percent-escaped base (3cbe2df). base must equal new URL(base).href, and each of its path segments must match PATH_PART.
    • Tested and refused (measured): %5F%5F, %2F, :443, /a/../b/, /a/./b/, //, and an upper-case host.
  3. Prototype method (3cbe2df). pullImage builds the argv from `${ref.name}@${ref.digest}` (the own frozen fields), and DatasetPin, Pins and ImageRef prototypes are frozen.
    • The test swaps toString by assignment and by defineProperty. On b61f0fa, docker was sent pull evil.example.net/other@sha256:<unpinned> (measured). Now both calls carry the pinned reference, and the three prototypes are frozen (measured).
  4. Real instance id. The three test sites now use example__proj-4242 and sweb.eval.x86_64.example_1776_proj-4242, which still match TASK_SHAPE.
    • I grepped the whole branch diff against origin/main (slice 1 and the README included). Patterns: __…-N, _1776_, <task repo>-NNN, 11099, and the task repos' names.
    • The only hits are the two synthetic constants, the TASK_SHAPE regex and its comment, and the test's negative regex (measured).
  5. CI. I merged bench: the bench command and bench preflight, a pass/fail line per check with no secret (#748 slice 1) [PROTECTED: owner merges] #851's 4313ef8 (which carries main d9855a9) with git merge --no-commit. The CHANGELOG conflict is resolved keeping both bullets, with this PR's first in "Guardrail changes: what each one does". Merge commit: 2a3dcab. CI was not checked before I handed back.

Count correction: the first commit message's "13 of 13" was the count when I wrote it. The file had 14 tests at f4fc274, as you measured. It has 17 now. The PR body is corrected too.

After the merge (measured, 2a3dcab):

  • bench-pins + bench-cli + bench + changelog: # tests 107 / # pass 107 / # fail 0;
  • npm run typecheck: exit 0;
  • npm run check:denylist: "1347 files clean";
  • check-dco.mjs origin/main HEAD: all signed off.

Not done: the non-blocking notes (pinning the registry, a kind check in requirePinned) are left for slice 4. requirePinned already fails closed downstream (your measurement).

@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: PASS (round 2, with notes)

Head 2a3dcab3. I reviewed the fix commit 3cbe2dfb and the merge 2a3dcab3, which brings in #851 4313ef82 and through it main d9855a91. Probes used stand-ins only: no network, no docker, temp HOME and PATH=/nonexistent.

The four round-1 items

# Item Result
1 Repeated keys and non-canonical forms fixed (measured)
2 Percent-escaped base fixed (measured)
3 Prototype swap of the image argv fixed (measured)
4 Real instance ids fixed (measured)
  1. Repeated keys and non-canonical forms. All of these are refused:

    • both round-1 repeated-key files;
    • a repeated key laid out in exact canonical spacing, at the top level and inside dataset;
    • CRLF, a BOM, tab or 4-space indentation, a trailing space, compact JSON;
    • no final newline, two final newlines, a leading newline;
    • a and \/ escapes, 1e6, 1048576.0, [ ], a comment;
    • __proto__.

    Reordered keys are accepted, and nothing is hidden by that. The repo's pins.json is byte-equal to its canonical form.

  2. Percent-escaped base. All of these are refused:

    • %5F%5F (upper and lower hex), %5F1776%5F, %2F, %64;
    • :443, :0443, an empty port;
    • /a/../b/, /a/%2e%2e/b/, /a/./b/, //;
    • an upper-case or mixed-case host, an upper-case scheme, a trailing dot;
    • ~, a space, a backslash, #, ?, % in the host.

    A root base and a pinned subdomain are still accepted.

  3. Prototype swap. All three prototypes are frozen. Refused:

    • assigning toString and defineProperty of toString;
    • setPrototypeOf on an instance and on ImageRef.prototype;
    • reassigning ImageRef.prototype;
    • defineProperty of name/digest/url on an instance.

    After all of those, and with an Object.prototype.name getter plus a swapped String.prototype.toString, the recorded argv is still exactly pull/inspect on the pinned reference.

  4. Real instance ids. git diff origin/main...2a3dcab3 has no added line shaped like an instance id or _1776_ except TASK_SHAPE, its comment, the synthetic example__proj-4242 and its image constant, and the test's negative regex. There are no SWE-bench repo names. The whole head tree has only synthetic ids.

Merge 2a3dcab3

  • measured: git merge-tree --write-tree 3cbe2dfb 4313ef82 conflicts only in CHANGELOG.md. The merge commit's tree differs from that auto-merge result only in CHANGELOG.md.
    • Both bullets are kept.
    • The resolution also rewrites one line of this PR's own bullet to mention the canonical form. The text is accurate, but it is a content edit inside the merge commit.
  • measured: 4313ef82 is origin/issue-748-bench-cli and contains d9855a91 (origin/main). merge-tree origin/main 2a3dcab3 is clean, and the PR is MERGEABLE.

Fail-without (method b)

measured: in a fresh worktree at b61f0fab I applied the head's test/bench-pins.test.ts (cmp-identical), then ran npx tsc and node --import ./dist/test/isolate.js --test dist/test/bench-pins.test.js: # tests 17 / # pass 14 / # fail 3. Each new test fails on the attack itself:

  • 5: "Missing expected exception: a repeated image_digests";
  • 6: "Missing expected exception (Error): a percent-escaped instance id";
  • 7: the argv was evil.example.net/other@sha256:a838….

On the head, bench-pins + changelog passed 27/27.

Other checks

  • npm run typecheck: exit 0. npm run check:denylist: "1347 files clean" (measured).
  • 3cbe2dfb and 2a3dcab3 are signed off, with no attribution trailers (measured).
  • Size: the branch changes only CHANGELOG.md, scripts/bench/**, test/bench-cli.test.ts and test/bench-pins.test.ts. None of them is in package.json files (measured), so the delta is 0 (inferred). Round 1 measured 0 by pack.
  • CI at review time (measured): every finished check passed (test shards 1/4@22, 2/4@22, 4/4@22, 2/4@24, 3/4@24, 4/4@24, desktop, agentshield, push-gate, denylist, dco, scope, new-deps, scan). Shards 3/4@22 and 1/4@24 were still pending.

Notes (non-blocking)

  • measured: with Array.prototype.includes swapped in-process, pinnedImage makes a genuine ImageRef for an unpinned digest. That is the round-1 class (an in-process method swap), reached through a builtin rather than this module's prototypes. A JS module can't close it: code in the same process can also call the exported spawnDocker directly (inferred). Suggest the body's "can never" claims say "from outside this process's code".
  • The merge commit's CHANGELOG rewording (above) is harmless. Next time, reword in a separate commit.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…-bench-pins: keep both CHANGELOG bullets, the bench download one first in its list

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
… to changelog.d/851-bench-preflight.md

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…GELOG.md as the base's, #863's note moved to changelog.d/863-bench-pinned-downloads.md

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
jayden1711 added a commit that referenced this pull request Oct 21, 2026
…is the base #884 already built on

#863 (8e74e4e) merged as squash a5df225, so its six bench files came back as add/add and content
conflicts against #884, which descends from 8e74e4e. Main's side of each conflicting file is
byte-identical to 8e74e4e's (measured: git rev-parse blob ids), so each hunk is #863's original
line next to #884's extension of it. Every hunk keeps #884's side, and each resolved file is
byte-identical to #884's head 170e70a: scripts/bench/README.md, cli.ts, download.ts, pins.json,
pins.ts and test/bench-pins.test.ts. Every #863 pin and check is kept. src/merge-policy-weakening.ts
merged cleanly: main's entries plus #884's SELECTION_RULES line.

Test (fails without): none new; this is a merge with no behaviour change over 170e70a.

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