worker: input-artifact resolver and materializer (candidate, not wired) - #72
Open
thegeorgepu wants to merge 5 commits into
Open
thegeorgepu wants to merge 5 commits into
thegeorgepu wants to merge 5 commits into
Conversation
ExecutionSpec.inputArtifacts is `{id, digest}[]`, already digest-bound, and
task.mjs:461 records it verbatim under "no fetch in Wave 1B scope". So the
contract can NAME an input the worker must consume and nothing can deliver
one. This is the missing middle, written so the contract owner has a concrete
thing to accept, amend or reject rather than a description.
DELIBERATELY NOT WIRED. No call site changes, no schema change, no envelope
change. materializeEnvelope and the runVinci spawn path are untouched.
WHY IT IS NOT MERE WIRING. `{id, digest}` establishes IDENTITY, not LOCATION.
A search of vinci-code-cli, vinci-gpu-control, vinci-contracts and
vinci-foundry found no resolver to reuse: the artifacts table says in its own
schema comment that it is "a pointer ledger ONLY", GET /v1/jobs/{id}/artifacts
returns metadata rather than bytes, the worker fetches nothing by digest
anywhere, and foundry verifies digests only AFTER reading an already-local
path. The nearest precedent is `vgc artifacts pull`, and this follows it.
TWO IDENTITIES, CHECKED SEPARATELY. The ledger says "object X holds these
bytes"; the contract says "the expected input is digest Y". They are distinct
claims from distinct authorities, so both are asserted -- letting one pass
because the other did is how a swap survives.
THE WORKER NEVER RESOLVES AN ID ITSELF. The id goes to an authority which
returns a pointer. The on-disk name is derived from the DIGEST, never from
the id or the uri: a filename is a place, and both of those are strings
someone else chose. Materialized 0o400, because a task that can rewrite its
own input can rewrite the evidence of what it was given.
The returned chain -- requested, resolved, downloaded, materialized -- is
exactly what vinci-gpu-control's `input delivery observation` consumes.
Eight mutants against a green baseline; six caught immediately, and the two
survivors are recorded because they were the interesting ones:
* Hashing the in-memory buffer instead of re-reading the file SURVIVED.
That is a real gap, not a cosmetic one: it proves the download twice and
labels the second one a materialization. Closed by making the read-back
injectable purely so the property is testable.
* Mutating writeFileSync's mode SURVIVED because the chmod after the
rename decides the final mode -- the write mode was defence in depth
that defended nothing observable. Removed, leaving one mechanism a test
can see. Mutating the chmod is now caught.
One bug of my own, same class as the module's subject: the digest-mismatch
fixture used a substitute of a different LENGTH, so the truncation check
answered first and that test never reached the guard it names. Earlier-guard
masking, inside the tests for a module whose whole job is catching
substitutions. The substitute is now the same length.
Not established: no live lookup, no live download, no call site, and no
authority route for a per-id pointer. The existing job-scoped artifacts route
returns a list, so a caller must either supply the producing job or the VGC
owner must add a per-id lookup -- `lookup` is injected here precisely so that
decision stays theirs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7
…seam
Four corrections from review, all of which reproduced.
1. PERMISSION BEFORE PUBLICATION. I had concluded the write mode was
useless because a chmod set the final mode. That was the wrong lesson:
write -> rename -> chmod leaves an interval where the FINAL pathname
exists and is still writable, and no test observed that interleaving,
which is exactly why the mutant survived. The ordering was the
load-bearing part and nothing was checking it. Now every narrowing
happens on the TEMP path, identity is verified while the file is still
unpublished, and the rename publishes something already read-only. A
staged file that does not hash correctly never acquires the final name.
2. LOOKUP TAKES id AND digest. Sending only the id would require artifact
ids to be globally unique, and the search behind this module found no
registry semantics establishing that. More than one matching pointer is
now `ambiguous_pointer` -- unresolvable, never pick-the-newest, because
choosing by recency is how the wrong artifact arrives with every digest
check passing.
3. THE READ-BACK SEAM IS NOT CALLER-CONTROLLED. It exists so the
buffer-versus-materialization property is testable, and a caller must
not be able to supply the function that decides what the worker believes
it materialized. The production entry point now enumerates what it
forwards instead of passing an options object through, and a test proves
an injected readBack does not reach the resolver that way.
4. The destination directory is created 0o700 before anything is written
into it.
MUTATION NOTES, because three instruments were wrong before the code was.
* My mutation harness reported all six mutants SURVIVING. It grepped
`head -1` and caught an earlier console.log while hiding the assertion
below it. Re-run on exit code: four caught, two survived. A mutation
harness that cannot see a failure is worse than none, because it
certifies.
* The directory-privacy mutant then survived because the FIXTURE
pre-created the destination with mkdtempSync, which already makes 0o700
directories -- the fixture was doing the thing under test. The dest now
hands the resolver a directory it must create.
* The write-mode and chmod-on-temp mutants each survived individually
because the other still set 0o400. That is genuine defence in depth
rather than dead code -- unlike the earlier chmod-after-rename case --
and the combined mutation IS caught, with "the temp file must be
read-only before it is published".
Still not established: no live lookup, no live download, no call site, and
no authority route for a per-id pointer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7
Two corrections from review, plus a mutant that closed a real gap. 1. THE ALL-OR-NOTHING COMMENT OVERCLAIMED. The loop publishes each artifact as it goes, so a refusal on artifact N leaves 1..N-1 materialized. The property that actually holds is EXECUTION-ATOMIC: no spawn unless every declared input resolved, downloaded, verified and materialized. The residue is verified, immutable and unreferenced -- cleanup debris, not partial execution -- but it is residue, and calling this a transaction was a claim the code does not implement. Narrowed rather than fixed with staging machinery: staging the whole set buys nothing while the execution-atomic property is enforced at the call site, which is where the wiring must test it. A test now asserts the residue exists rather than leaving the reader to assume rollback. 2. AN AUTHORITY THAT CAN SELECT AN OBJECT DOES NOT THEREBY GAIN ARBITRARY NETWORK-FETCH AUTHORITY. The pointer decides where this worker goes, so the caller must declare the storage namespace its downloader is qualified for, and a pointer outside it is refused however well-formed. There is NO permissive default: an omitted allowlist is `no_uri_allowlist`, because a default would make the trust boundary invisible at the call site, which is the one place it has to be visible. Refused cases include another bucket, an http endpoint, a file:// url, the link-local metadata address, and a prefix-adjacent bucket name. MUTATION, with the harness qualified first. Following the doctrine this session earned: the run began with a KNOWN-KILL CONTROL -- make `refuse` a no-op, which must be caught -- so a harness that cannot see failures is detected before it certifies anything. It was caught, then four real mutants ran. Three were caught. The fourth, `startsWith` -> `includes`, SURVIVED: every out-of-namespace case I had written also fails a substring test, so none could tell a prefix check from a substring check. Closed with the namespace appearing as a query parameter and buried in a key -- the two shapes where the difference is the whole attack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7
Adversarial review of ambient capability found a real defect, and it is the
one that matters: the resolver followed a symlink at its staging path.
THE ATTACK, reproduced before the fix. Pre-place a symlink at the
deterministic staging name `${digest}.input.partial` in destDir, pointing at
any file the worker can write. writeFileSync and chmodSync BOTH follow it,
so the victim outside destDir is overwritten with the artifact bytes and
forced to 0o400. renameSync does NOT follow, so it publishes the LINK as the
materialized file. The final digest re-read follows the link and matches --
so the chain reports success, with a correct materialized_digest, while an
arbitrary file has been clobbered and permission-locked and
`materialized_path` points out of the sandbox. Worse for the module's own
claim: the attacker still owns that location, so the "materialized, verified,
immutable" artifact can be rewritten after resolution returns.
Measured: victim content became the artifact bytes, mode 400, and
lstat(materialized_path).isSymbolicLink() was true.
Three independent defences, because one is a single point of failure on a
path that had none:
* the staging name is now unpredictable (randomUUID), so it cannot be
pre-created;
* the write is `flag: "wx"` -- exclusive create fails EEXIST on anything
already there, symlink included, rather than writing through it;
* the published path is lstat-ed and must be a regular file.
MUTATION, harness qualified first with a known-kill control. Y2
(exclusive create) and Y3 (lstat) each SURVIVE individual removal, because
the unpredictable name alone already blocks the attack. That is the
redundant-guards-mask-each-other pattern, so they were removed PAIRWISE:
with only lstat left the attack dies as
`published_path_not_a_regular_file`; with only exclusive-create left it dies
as `staging_path_occupied`; with all three gone the symlink test goes red
with "a file outside destDir must not be written through a symlink". Each
backstop demonstrably contributes rather than being decoration.
Also from the same review:
* Unanchored allowlist prefixes are refused. `s3://vgc-artifacts` without
the delimiter admits `s3://vgc-artifacts@evil.example/x`, which passes
startsWith while a WHATWG parser reads host evil.example and userinfo
vgc-artifacts. A structural rule on configuration, not a URI parser --
the module previously placed no requirement on these entries at all.
* Wrong-typed inputArtifacts raised a bare TypeError, escaping callers
that catch the documented InputArtifactError contract. Now refuses.
* The size ceiling is documented as HONESTY-DEPENDENT: it reads the
pointer's declared size, so a lying authority can under-declare and the
adapter still buffers the real object before this module sees a byte.
maxBytes reaches download as ADVISORY and this module cannot verify the
adapter honoured it. What is enforced is delivered-length equals
declared-length, which bounds materialization, not transfer.
* `download_truncated` renamed `download_length_mismatch`: an oversized
payload is not a truncated one.
The reviewer confirmed the sponsor's URI question directly: `..` and
`%2e%2e` both pass the string check and collapse to the same path under a
real parser. That divergence stands as a known limit of a prefix test; the
structured replacement belongs with the real downloader integration and is
deliberately not built here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7
Contributor
Author
Do not merge yet — merge gateThis is a qualified candidate waiting, not a merge queue entry. Specifically, Lane 1 is expected to bind the requesting WorkOrder and authenticated principal into resolution, which changes The order matters. Merging the primitive first and integrating later means changing a There is a second reason to keep dormant components off Nothing is at risk from waiting: the branch, its SHAs, this review history and CI all preserve it. The reviewed original is at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Candidate only. Nothing calls this. No call-site change, no schema change, no envelope change —
materializeEnvelopeand therunVincispawn path are untouched. Two new files, 873 insertions, 4 ahead ofmainand 0 behind.The question this PR asks
Explicitly not: "does this complete input-artifact delivery?" It does not, and the sections below say exactly which parts are missing and who owns them.
Why it exists
ExecutionSpec.inputArtifactsis{id, digest}[]— already digest-bound, andtask.mjs:461records it verbatim under "no fetch in Wave 1B scope". So the contract can name an input a worker must consume, and nothing can deliver one.{id, digest}establishes identity, not location. A search of vinci-code-cli, vinci-gpu-control, vinci-contracts and vinci-foundry found no resolver to reuse: theartifactstable says in its own schema comment that it is "a pointer ledger ONLY",GET /v1/jobs/{job_id}/artifactsreturns metadata rather than bytes, the worker fetches nothing by digest anywhere, and vinci-foundry verifies digests only after reading an already-local path. The nearest precedent isvgc artifacts pull, and this follows it.What it does
Given
lookup(artifact_id, expected_digest)anddownload(uri)— both injected, because the authority endpoint and object-store client are deployment concerns — it refuses more than one pointer match; treats the ledger digest and the contract digest as two separate claims from two authorities; enforces a declared byte ceiling before fetching; verifies the downloaded bytes; verifies the staged file while it is still unpublished; publishes read-only by atomic rename under a digest-derived name into a0700directory; and re-reads the published path to produce the evidence chain (requested / resolved / downloaded / materialized) that vinci-gpu-control's input-delivery observation consumes.The worker never resolves an id itself. The id goes to an authority which returns a pointer, and the on-disk name comes from the digest — never the id, never the URI, because a filename is a place and both of those are strings someone else chose.
Review disposition: GO-WITH-FINDINGS
Adversarial review of ambient capability found one critical defect, reproduced and fixed in
229bb4c6: a symlink pre-placed at the deterministic staging path was followed by both the write and the chmod, so a file outsidedestDirwas overwritten and forced to0400, whilerenameSync(which does not follow) published the link as the materialized artifact — and the digest re-read followed it and matched. Success reported, correct digest, arbitrary file clobbered,materialized_pathpointing out of the sandbox.Three defences now: unpredictable staging name,
flag: "wx"exclusive create, and anlstatrequiring a regular file at the published path. Removing the second or third individually survives — the random name alone blocks the attack — so they were removed pairwise: with onlylstatleft the attack dies aspublished_path_not_a_regular_file; with only exclusive-create left, asstaging_path_occupied; with all three gone the symlink test goes red. Each backstop demonstrably contributes.Correction to a comment inside the diff
The source calls the materialized residue "immutable". Read it as:
runVincichanges no uid/gid, and a Unix owner can chmod its own0400file writable again. This repo already states the same limitation about its own clean room —vinci/worker/cleanroom.mjs:45("no user separation (child and daemon share a uid and can read each other's files)") and:196("a resolution boundary, not uid isolation"). The comment was left unedited to preserve the exact reviewed subject; this note exists so no reviewer inherits the false claim.Remaining limits, all intentionally downstream
lookupis injected; no route resolves(id, digest)→ one pointer today, and resolution must also check that the bound execution spec for an authenticated principal actually requests that artifact.s3://vgc-artifacts/../../etc/passwdand its%2e%2eencoding both pass and collapse to the same path under a real parser. The downloader should receive a structured bucket/key object, not a generic string.maxBytesis advisory at this layer — a lying pointer can under-declare and the adapter buffers the real object first. The delivered-vs-declared length check remains a separate control; it bounds materialization, not transfer.Full requirements:
vinci-gpu-control→docs/ARTIFACT-RESOLUTION-LANE-1.md. Lane closure record:docs/P0-D-WORKER-LANE-CLOSED.md.Validation
node vinci/test/worker-input-artifacts.mjs— four control groups, all green.node vinci/test/worker-contract-vectors.mjs— PASS (6 vectors, 11 float cases, 22 path-grant cases), unchanged by this diff.Every refusal is paired with a legitimate case through the same call. Mutation rounds began with a known-kill control (make
refusea no-op, which must be caught) after an earlier harness silently reported six false survivors through bad output parsing.Not established
No live lookup, no live download, no call site, no authority route, and no delivery has ever been observed. The reviewed original is preserved unmodified at
worker/input-artifact-resolver@01cb4c2c.🤖 Generated with Claude Code
https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7