fix(artifact): refuse file and directory ZIP collisions

CHANGELOG.md

@@ -4,6 +4,7 @@ Each release of Research-Kit, newest first. The version is `KIT_VERSION` in
 `research-kit/lib/core.mjs`; a release is tagged `v<version>` (ADR-0116).
 
 ## Unreleased
+- `artifact validate` rejects ZIP entries where a file is also an ancestor directory of another entry, before extraction, with a stable `ZIP-PATH-COLLISION` failure (#362).
 - Fixed `AGENTS.md` corpus description to use evergreen relative references rather than stale hardcoded entry counts (#299).
 
 - The suite's "every CLI refuses a flag it does not know" table now covers `measure`,

docs/ARCHITECTURE.md

@@ -177,7 +177,7 @@ is an open decision (2026-09-30):
 | `r29-workbook-linkage-validator.mjs` | Cross-reference validator for the R29 reviewer-workbook linkage register: 24 task ids, six expected reference kinds (task/gold/source/calibration/warm/lock), and a status reduction that takes the worst of `PASS < INCOMPLETE < FAIL < REOPEN`. **Zero imports** — it is 58 lines of pure functions over `{ register, catalog, pointer }` passed in as arguments, so it touches no filesystem, no network and no clock, and the caller owns every input. That is also why it needed no sealed-record fixture: its tests synthesise a register into a temp directory rather than shipping one. A revoked lock pointer reduces to `REOPEN` rather than `FAIL`, which is the distinction the rank order exists for — a withdrawn approval is not the same as a broken link. | ADR-0029 |
 | `config.mjs` | A **compatibility shim**, nine lines, re-exporting `machine.mjs`'s config surface so an older import path keeps working. It adds nothing and decides nothing. Listed because it was the one module in `lib/` this map did not mention — found by counting files against rows during the 2026-09-20 reconciliation, which is the only way that kind of omission surfaces. | ADR-0002 |
 | `archive.mjs` | **One container format**: `buildZip(entries)` → bytes, `writeZip(file, entries)` → `{ file, bytes, entries }` (written whole or not at all, through `writeBytes`, since 2026-09-28), `crc32`, `entryName`. ZIP with deflate (store when deflating would only grow the payload), written because the kit has no dependencies and cannot shell out to a `zip` binary that may not exist. Knows the container and nothing else — what goes inside a bundle is `lib/audit.mjs`'s. Refuses entry names that are not plain relative paths (an archive must not unpack outside its folder) and an archive with no entries. See ADR-0019. **Changes no name it is handed** (2026-09-28): `entryName` refuses a backslash - it translated one into a separator, which stored `a\b.md` as `a/b.md` while the package manifest still declared the original. | ADR-0019 |
-| `artifact-zip.mjs` | **Reading a ZIP somebody else wrote** — the opposite job from `archive.mjs`, and a separate file on purpose: a writer that also parses hostile input grows a trust boundary in the middle of itself. `openZip(bytes)` → `{ entries, names, problems, has, read }`, plus `checkEntryName`, `readCentralDirectory`, `inspectEntries`, `readEntry` and the exported `ZIP_LIMITS` (entry count, total and per-entry uncompressed bytes, ratio + floor, name length) so tests drive the refusal paths with small fixtures instead of real bombs. **The order of operations is the security property**: every structural refusal — traversal, absolute, drive letter, backslash, NUL, duplicate, case collision, symlink/special entry, encryption, unknown method, counts, sizes, ratio — is decided from the **central directory before one byte is inflated**. The directory is attacker-controlled too, so it is never the only defence: `inflateRawSync` runs with `maxOutputLength`, making the ceiling the runtime's to enforce rather than a number compared against a field the attacker wrote. Names are judged as raw text, never `path.normalize`'d first — normalising would let `a/../../b` become `../b` and then be judged, when the honest answer is that `..` has no business in a research artifact. Touches no filesystem: entries come back as buffers. ZIP64 is refused rather than half-read. | ADR-0032 |
+| `artifact-zip.mjs` | **Reading a ZIP somebody else wrote** — the opposite job from `archive.mjs`, and a separate file on purpose: a writer that also parses hostile input grows a trust boundary in the middle of itself. `openZip(bytes)` → `{ entries, names, problems, has, read }`, plus `checkEntryName`, `readCentralDirectory`, `inspectEntries`, `readEntry` and the exported `ZIP_LIMITS` (entry count, total and per-entry uncompressed bytes, ratio + floor, name length) so tests drive the refusal paths with small fixtures instead of real bombs. **The order of operations is the security property**: every structural refusal — traversal, absolute, drive letter, backslash, NUL, duplicate, case collision, file/directory-prefix collision, symlink/special entry, encryption, unknown method, counts, sizes, ratio — is decided from the **central directory before one byte is inflated**. The directory is attacker-controlled too, so it is never the only defence: `inflateRawSync` runs with `maxOutputLength`, making the ceiling the runtime's to enforce rather than a number compared against a field the attacker wrote. Names are judged as raw text, never `path.normalize`'d first — normalising would let `a/../../b` become `../b` and then be judged, when the honest answer is that `..` has no business in a research artifact. Touches no filesystem: entries come back as buffers. ZIP64 is refused rather than half-read. | ADR-0032 |
 | `artifact-validator.mjs` | **Is this package valid, and does it authorize a build** — two questions, kept apart. `validateArtifact({ file, expectedClientRef })` → `{ status, buildAuthorized, packageId, clientRef, workflowRunId, state, errors, warnings }`, where `status: 'PASS'` means internally consistent and says *nothing* about permission. Offline and read-only by construction: no transport imported, `process.env` never read, nothing from the package executed, no shell, and the only write is a `fs.mkdtemp` copy of `project/` removed in a `finally` — needed because the chain and handoff checks are the kit's existing code (`readCorpus`, `verifyHandoff`) and reuse beats a second implementation of provenance verification nobody tested. Stable error codes (`ZIP-*`, `MANIFEST-*`, `FILE-*`, `LEDGER-*`, `CAPTURE-*`, `AUTHORIZATION-INCONSISTENT`, `SECRET-DETECTED`, `FORMAT-UNSUPPORTED`, `CLIENT-REF-MISMATCH`) and **never a secret value in an error** — the report names the pattern and the line. `authorizationProblems()` is the one implementation of the build rule; the schema encodes it a second time and when the schema catches it first the rule is still reported in its own words. An unsupported format **major** is `INCOMPLETE`, not invalid. A crash is `BLOCKED`, never `PASS`. Its report carries `reviewedBy` (`review.by`, or `undeclared` for a 1.0.0 package) and judges nothing by it (ADR-0074). Reads format majors **1 and 2** (2026-09-30, ADR-0107): a 1.x `HUMAN_REVIEW_REQUIRED` is reported as `REVIEW_REQUIRED`, and `eraProblems` refuses a value from the other major by name - a 2.x package with the old state or `review.by: "human"`, or a 1.x one with the new state. A binary body (NUL in its first 8000 bytes) is not matched against the credential patterns, as a body over 512 KiB is not (ADR-0128, 2026-10-02): `docs/` travels whole, and one vendored DLL carrying the PEM marker it parses would have failed a package with no secret in it. | ADR-0032 |
 | `artifact.mjs` | **Packaging a project without changing it, and deriving every authorization field.** `createArtifact` → `{ bytes, name, manifest, entries, derived }`; `writeArtifact` writes then **re-validates with the consumer's own validator** before reporting success; its bytes go through `writeBytes`, so re-creating a package at a name that already holds one cannot trade a good package for a truncated one (2026-09-28). There is no `--build-authorized` and no option that reaches an authorization field: `deriveState(root)` runs the real gate with `env` defaulting to `{}` so a packaging shell's override cannot become a property of a package that outlives it. `mapClassified` and `briefReviewed` have honest machine answers; `findingsReviewState` answers the third **negatively** — a `Finding` byte-identical to what `firstFinding` re-extracts from the capture was demonstrably never rewritten, which detects the unreviewed case without grading the reviewed one (ADR-0013's line, uncrossed). It repeats the collector's own call exactly - the page body after the front-matter, falling back to the page's `title` or `url` from that front-matter - since 2026-09-27: run over the whole file, the extractor read the front-matter as the page, and a URL-only corpus nobody had touched (one JSON capture, run 36287211468) was packaged `findingsReviewed: true`. `packageName` never carries the topic (an artifact listing is world-readable on a public repository) and falls back to `run-<workflowRunId>`; `checkClientRef` refuses rather than sanitises. Inventory sorted by ZIP path, manifest and digest appended **last**, digest over the exact manifest bytes. README-FIRST in a package that is not yet approved prints the preflight and brief commands and **one `create` line carrying the package's own identity** (`repackageArgs`, 2026-09-27): the manifest is written once, and the package gave its reviewer no way to get one that says `APPROVED_BRIEF`. Commands are spelled by `homeCommand` (ADR-0050). `createArtifact` checks the identity against the manifest schema's own `source` rules before deriving or writing anything (`sourceProblem`, 2026-09-27): `create --commit abc` wrote a package, then reported it did not validate, and left it on disk. **`review.by`** (format 1.1.0, ADR-0074) is the reviewer's declaration read from `BRIEF.md` by `reviewedBy`: `agent` or `undeclared` (`human` was retired in format 2.0.0, ADR-0107). Not an approval condition. README-FIRST no longer says "a human reviewed it"; it names the declared reviewer, or says none was declared. `collectProjectFiles` and the explicit ledger add refuse (`OUTSIDE_PROJECT`) any path whose real location is outside the project: it followed links and packed outside files, a private key included (ADR-0076). The same walk asks the consumer's own `checkEntryName` about every name it is about to store and refuses the run before a byte is written (`UNPACKAGEABLE_NAME`, 2026-09-28): `entryName` translates a backslash into a path separator, so a file named `a\b.md` travelled as `a/b.md` - a directory invented out of a name - while the manifest kept the original, and only the read-back caught the contradiction, after the package had been written. The same refusal covers two files differing only in case (2026-09-28), with the reader's lowercase comparison: the reader's ZIP-CASE-COLLISION fired only after the write. Format **2.0.0** (2026-09-30, ADR-0107): a collected package waiting for review says `REVIEW_REQUIRED`, and README-FIRST gives the review to the agent alone.  collectProjectFiles refuses (`UNPACKAGEABLE_FILE`) what packaging cannot read to an end - a FIFO, socket or device, and a folder link back into its own ancestry (judged by real path, so a link to a sibling folder inside the project still packages) - before a byte is read (2026-10-01). **A reviewed brief is complete and current** (2026-10-03, output-reliability audit G1): `deriveState` approves only when every judged section is present and answered (`judgedSection`) and a stamped brief's inputs hash matches the corpus (`draftStamp`, `briefInputsHash`); `briefState` alone read a missing section as answered, so a drafted brief with no review sections derived APPROVED_BRIEF on the repository's own corpus, and a brief whose evidence changed after the draft stayed authorized beside hygiene's brief-stale warning. A brief with no stamp predates ADR-0055 - the root's own and ten decision briefs - and keeps its approval: its currency cannot be checked, `hygiene/brief-unstamped` says so (ADR-0138), a redraft with --force stamps it. **README-FIRST in every state and the collection summary preserve the producer evaluation warnings** through `renderGateWarnings` (ADR-0143). Authorization fields and machine package formats are unchanged. `bin/artifact.mjs` refuses a value flag given no value by name, exit 3, before anything is read or written (2026-10-11, #280): `create` had asked only whether a required flag was present, so a bare `--ref` went into the manifest as "true", a bare `--run-id` as 1 and a bare `--output` wrote a package named `true` that re-validated PASS; a bare `validate --file` tried to open a file called `true`. | ADR-0032, ADR-0031, ADR-0052, ADR-0143 |
 | `mcp.mjs` | **The collector as a Model Context Protocol server**, and the first module in this repository built from an APPROVED BRIEF rather than from a plan - every design choice cites an E-row in `docs/decisions/2026-09-21-agent-interface/`. `handle(message, deps)` is a pure function of a JSON-RPC message plus injected transport, so the whole surface is tested offline. **stdio, not Streamable HTTP**: semantics are identical on both, so the difference is operational, and remote costs the entire OAuth 2.1 chain. **Two tools, not one** - `collect` returns a run id immediately, `fetch_corpus` returns a link - because a server cannot initiate a request, so the alternative is one call held open for minutes and an agent that loses a paid run when its process dies. **A `resource_link`, never embedded bytes**: the spec defines embedding but states no maximum size, so a link is the defensible option. **No tool takes a token** and a test asserts no parameter is credential-shaped. `PROTOCOL_VERSION` is pinned and a mismatch answers `-32022` with `data.supported`, the same discipline as pinning `X-GitHub-Api-Version`. `createStdioLoop` serialises chunk processing through one promise chain - overlapping `data` events were mutating the buffer concurrently, which a test caught. **DUAL-ERA**: `SUPPORTED_VERSIONS` is `['2026-07-28', '2025-11-25', '2025-06-18']` (2025-06-18 since 2026-09-27, ADR-0049: a client on the official SDK up to 1.24.0 asks for it and refuses 2025-11-25, and nothing 2025-11-25 changed touches this server; 2025-03-26 is not served, because the results carry `structuredContent` and `resource_link`) and `initialize` is implemented beside `server/discover`, because the specification is ahead of every shipped client - the official SDK at v1.30.0 declares `2025-11-25` as its latest and answered the modern-only first version with `-32601: unknown method initialize`. `initialize` **echoes** a serveable requested version and falls back to **legacy** rather than to this server's preference, because a client throws on a version it does not support. **Arguments that fail a tool's schema are a tool result** (`isError: true`, the reason as text) since 2026-09-27 - SEP-1303 in 2025-11-25 says input validation errors should be Tool Execution Errors, so the model can correct itself; they were JSON-RPC -32602, which a client raises as an exception. An unknown tool stays -32602. `collect`'s `topic` carries `minLength: 1`, and `validateArgs` enforces `minLength` after trimming (2026-09-27): an empty topic had passed and reached GitHub, which answered HTTP 422. `fetch_corpus` checks `out_dir` with `usableOutDir` before it reads the run (2026-09-27), so an unusable folder is an `OUT_DIR` tool error, not `UNKNOWN`. `fetch_corpus` returns `reviewedBy`, and its text says review is for an agent or a person (ADR-0074). `createStdioLoop` bounds a line at `MAX_LINE` (4 MiB, 2026-09-28): a line past it is refused with -32600 as soon as it passes the cap and dropped through its newline, and the next message is served - the buffer had grown without limit until a newline came, so a client that sent none could grow the server until it died. **No request is lost at end of input** (break-test PR #145, 2026-09-29): `exitWhenSettled` waits for every call already read before exiting (bounded by `END_GRACE_MS`), and a final request without a trailing newline is served on `end` - both had been dropped with exit 0. `SERVER_INFO.version` is `KIT_VERSION` (ADR-0116); it had announced a fixed 1.0.0. **The approved fetch response directs the builder to README-FIRST and the brief for warnings and known unknowns** (ADR-0143). It forwards no new machine warning field and derives no new authorization. `validateArgs` checks allowed keys by own membership and enforces the declared arrays of strings: malformed query/URL values and inherited Object property names become schema tool errors before credential lookup or dispatch. Omitted or empty arrays, and valid string arrays, keep their existing normalization. | ADR-0034, ADR-0049, ADR-0143 |

research-kit/README.md

@@ -563,7 +563,7 @@ node research-kit/bin/selftest.mjs            # all of it
 node research-kit/bin/selftest.mjs gate hook  # just these files
 ```
 
-1789 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means
+1791 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means
 the assertions settled (ADR-0021), and each test is raced against a watchdog
 (`RESEARCH_KIT_TEST_TIMEOUT`, default 60s).
 

research-kit/lib/artifact-zip.mjs

@@ -212,6 +212,8 @@ export function inspectEntries(entries, { limits = ZIP_LIMITS } = {}) {
   const problems = [];
   const seen = new Map();          // exact name -> count
   const folded = new Map();        // lowercased name -> first exact name that produced it
+  const files = new Set();
+  const directories = new Set();
   let totalBytes = 0;
 
   for (const entry of entries) {
@@ -242,7 +244,12 @@ export function inspectEntries(entries, { limits = ZIP_LIMITS } = {}) {
       continue;
     }
 
-    if (entry.isDirectory) continue;   // carries no payload and declares no size worth counting
+    if (entry.isDirectory) {
+      directories.add(entry.name.replace(/\/$/, ''));
+      continue;   // carries no payload and declares no size worth counting
+    }
+
+    files.add(entry.name);
 
     const count = (seen.get(entry.name) ?? 0) + 1;
     seen.set(entry.name, count);
@@ -281,6 +288,31 @@ export function inspectEntries(entries, { limits = ZIP_LIMITS } = {}) {
     }
   }
 
+  // An archive may not use one path as both a regular file and the directory
+  // containing another entry. Otherwise materialization order decides whether mkdir
+  // or writeFileSync throws, turning a readable contradictory package into BLOCKED.
+  for (const file of files) {
+    if (directories.has(file)) {
+      problems.push({
+        code: 'ZIP-PATH-COLLISION',
+        detail: `file "${file}" is also declared as a directory`,
+        path: file,
+      });
+    }
+    let slash = file.indexOf('/');
+    while (slash !== -1) {
+      const parent = file.slice(0, slash);
+      if (files.has(parent)) {
+        problems.push({
+          code: 'ZIP-PATH-COLLISION',
+          detail: `file "${parent}" is also a directory prefix of "${file}"`,
+          path: file,
+        });
+      }
+      slash = file.indexOf('/', slash + 1);
+    }
+  }
+
   if (totalBytes > limits.maxTotalBytes) {
     problems.push({ code: 'ZIP-SIZE-LIMIT', detail: `the package declares ${totalBytes} uncompressed bytes, over the ${limits.maxTotalBytes} limit` });
   }