review(#152/#153): incorporate code-review feedback (2 HIGH + 3 MEDIUM + LOW polish)

Independent reviews of PR #196 (imports kind) and PR #199 (R2 substrate) surfaced two real bugs in the substrate's S3 path plus several MEDIUM-severity correctness issues. This change closes them all and adds regression tests for each.

**HIGH** — `pipeline/refresh-index.mjs:147,154`: `require('node:fs').unlinkSync(tmp)` inside an `.mjs` ESM module throws `ReferenceError`, silently swallowed by the surrounding `try{}catch{}`. Every S3 GET / PUT leaked a `/tmp/refresh-index-*` file in production. Hermetic tests didn't exercise the S3 path so the bug never surfaced. Fix: hoist `unlinkSync` into the top-level `import { ... } from 'node:fs'`.

**HIGH** — `pipeline/refresh-index.mjs` S3 `putBytes`: the file's own header (line 35) declares `If-Match on the current ETag; retry on 412`, and brief §2.5 / §3.4 / §7.2 prescribe ETag-CAS. The implementation was a bare `s3api put-object` with no preconditions and no retry — two concurrent publishes would silently lose one writer's view. Fix: add `headETag()` to the S3 backend; `putBytes({ifMatch})` passes `--if-match` and translates 412 to a tagged `__precondition` error; the write loop reads the ETag, attempts the PUT, and on precondition failure retries with capped exponential backoff (up to 5 attempts).

**MEDIUM** — `pipeline/refresh-index.mjs`: when a newer SHA-keyed prefix exists in the bucket but `latest.json` still points at the older one (interrupted publish, partial restore, manual upload), the emitted index record mixed `prefix` and `short_sha` from the bucket-derived newest with `commit_sha` and `published_at` read from the stale pointer. The mismatched `published_at` also poisoned the `status` calculation. Fix: trust `latest.json`'s metadata only when its `prefix` field actually matches the chosen `latestPrefix`. Otherwise log a WARN, derive `short_sha` + `published_at` from the prefix directory name, and emit `commit_sha: null`. New `refresh-index.mjs: latest.json points at older prefix than bucket newest (drift)` test exercises this.

**MEDIUM** — `pipeline/publish-catalog.sh`: validation and upload were interleaved in a single `while ... | read` loop. If catalog A validated and uploaded, then catalog B failed validation, the script exited 1 leaving A as an orphan under the SHA-keyed prefix with no `latest.json` ever written — and the next `refresh-index` run would treat the orphan as the canonical "latest." Fix: split into a validate-all-first pass (record failure to a tempfile so the subshell `exit` doesn't get swallowed) then an upload-all-after pass; the bucket stays untouched if any catalog fails validation. New `publish-catalog.sh: mixed-validity batch uploads NOTHING (no orphan SHA prefix)` test exercises this.

**MEDIUM** — `pipeline/fetch-catalogs.sh`: the `distinct_repos` counter was incremented for every repo named in the work list (i.e., every ok-status repo in the index) regardless of whether any catalog was actually present in the cache afterwards. An index with 3 ok repos where all GETs failed would exit 0 with an empty cache and `Summary: 3 repos / 0 fetched / 0 cached / 3 failed`. Downstream `jq -s 'map(.entries) | add'` then silently produced empty output. Fix: rename the counter to `present_repos` and only `echo` to its tempfile after a successful cache hit or fetch. New `fetch-catalogs.sh: exits 1 when every fetch fails` test exercises this.

**LOW polish** rolled into the same change:

- `extractors/typescript/type-catalog.mjs:524`: `const { a: { b } } = require('pkg')` previously emitted zero rows (every element was `continue`d as a nested binding, then the explicit `return` short-circuited the bare-call fallback) so the consumer edge to the package was completely lost. Now falls through to the namespace-form `pushRequire({ name: '*', imported_as: null })` so the package edge survives. Fixture `12-nested-require.ts` + matching assertion added.
- `extractors/typescript/type-catalog.mjs:467`: `import {} from "pkg"` (empty named-imports clause — semantically a side-effect import) now emits a side-effect row instead of zero rows. Fixture `11-empty-named.ts` + matching assertion added.
- `extractors/typescript/type-catalog.mjs:404-409`: corrected the misleading "interleave with declaration rows by line" comment to describe actual ordering (top-level imports first in source-line order; declarations + dynamic-import + require rows after, in AST-walk order). Both halves remain deterministic, which is what the contract depends on.
- `pipeline/refresh-index.mjs`: `--bucket-fs DIR` now preflight-checks the directory exists with a friendly error message instead of crashing with a raw `ENOENT mkdir` stack inside `putBytes`.
- `pipeline/refresh-index.mjs:196`: dropped the hardcoded `wxyc/` org prefix in the canonical-name fallback. When `latest.json` is missing or unparseable, `canonicalRepo` now falls back to `pathSegment` alone — keeps the substrate org-agnostic.

Verified: 97 extractor tests + 26 canonical + 124 integration + 54 gojq-parity + 56 substrate = 357 tests pass. shellcheck on the four substrate shell scripts is clean.

Refs #152, #153.

Author: jakebromberg

extractors/typescript/fixtures-imports/11-empty-named.ts

@@ -0,0 +1,4 @@
+// `import {} from "pkg"` — empty named-imports clause. Semantically a
+// side-effect import; the consumer edge to "@wxyc/empty-named" must still
+// be recorded.
+import {} from '@wxyc/empty-named';

extractors/typescript/fixtures-imports/12-nested-require.ts

@@ -0,0 +1,6 @@
+// `const { a: { b } } = require('pkg')` — every destructured binding is a
+// nested pattern. The previous extractor lost the specifier entirely;
+// after the fix it falls through to the namespace-form bare require row.
+const { a: { b } } = require('@wxyc/nested-require');
+
+export const _b = b;

extractors/typescript/test/imports.test.mjs

@@ -63,8 +63,8 @@ test('with --include-imports, entry count grows by exactly the expected import c
   const cat = catalogWithImports();
   const baseline = catalogWithoutImports();
   const importRows = cat.entries.filter((e) => e.kind === 'import');
-  assert.equal(importRows.length, 26, `unexpected import row count: ${importRows.length}`);
-  assert.equal(cat.entries.length, baseline.entries.length + 26);
+  assert.equal(importRows.length, 28, `unexpected import row count: ${importRows.length}`);
+  assert.equal(cat.entries.length, baseline.entries.length + 28);
 });
 
 test('filtering out kind:"import" matches the no-flag catalog byte-for-byte', () => {
@@ -252,6 +252,33 @@ test('relative and absolute specifiers record origin_package=null, resolution="r
   ]);
 });
 
+test('empty-named: `import {} from "pkg"` emits a single side-effect row', () => {
+  // Empty named-imports clause is semantically equivalent to a side-effect
+  // import — the consumer-edge to the package must still be recorded.
+  const rows = importsFor(catalogWithImports(), '11-empty-named.ts');
+  assert.deepEqual(rows, [
+    {
+      name: null, imported_as: null, import_form: 'side-effect',
+      origin_specifier: '@wxyc/empty-named', origin_package: '@wxyc/empty-named',
+      origin_resolution: 'bare-specifier', type_only: false,
+    },
+  ]);
+});
+
+test('nested-require: all-nested destructure falls through to namespace require so the package edge survives', () => {
+  // `const { a: { b } } = require('pkg')` — every element is a nested
+  // binding pattern. Previously dropped silently; now records the package
+  // edge with name="*" and no local alias.
+  const rows = importsFor(catalogWithImports(), '12-nested-require.ts');
+  assert.deepEqual(rows, [
+    {
+      name: '*', imported_as: null, import_form: 'require',
+      origin_specifier: '@wxyc/nested-require', origin_package: '@wxyc/nested-require',
+      origin_resolution: 'bare-specifier', type_only: false,
+    },
+  ]);
+});
+
 test('mixed: default + named bindings from one statement emit in source order', () => {
   const rows = importsFor(catalogWithImports(), '10-mixed.ts');
   assert.deepEqual(rows, [

extractors/typescript/type-catalog.mjs

@@ -402,9 +402,12 @@ function extractFromFile(filePath, pkgName, pkgRoot, { collectImports, includeIm
   const rawImports = collectImports ? [] : null;
 
   // Per-symbol `kind: "import"` rows for the type-catalog (gated on
-  // --include-imports). One row per imported binding; pushed in walk order
-  // so they interleave with declaration rows by line — keeps the catalog's
-  // existing "walk order = output order" determinism contract intact.
+  // --include-imports). Top-level static imports + re-exports are emitted
+  // here, ahead of visit(sf) — so within a file the row order is:
+  //   1) every top-level import/re-export in source-line order, then
+  //   2) declaration + dynamic-import + require rows in AST-walk order.
+  // Both halves are deterministic, which is what the catalog contract
+  // actually depends on.
   function emitImportRowsForDecl(stmt, specifier, importForm) {
     const declTypeOnly = importForm === 're-export'
       ? stmt.isTypeOnly === true
@@ -447,12 +450,15 @@ function extractFromFile(filePath, pkgName, pkgRoot, { collectImports, includeIm
       pushImport({ name: null, imported_as: null, import_form: 'side-effect', type_only: false });
       return;
     }
+    let emittedAny = false;
     if (clause.name) {
       pushImport({ name: 'default', imported_as: clause.name.text, import_form: 'default', type_only: declTypeOnly });
+      emittedAny = true;
     }
     const nb = clause.namedBindings;
     if (nb && ts.isNamespaceImport(nb)) {
       pushImport({ name: '*', imported_as: nb.name.text, import_form: 'namespace', type_only: declTypeOnly });
+      emittedAny = true;
     } else if (nb && ts.isNamedImports(nb)) {
       for (const spec of nb.elements) {
         const localName = spec.name.text;
@@ -463,8 +469,15 @@ function extractFromFile(filePath, pkgName, pkgRoot, { collectImports, includeIm
           import_form: 'named',
           type_only: declTypeOnly || spec.isTypeOnly === true,
         });
+        emittedAny = true;
       }
     }
+    // `import {} from "pkg"` parses as a named-imports clause with zero
+    // elements — semantically equivalent to a side-effect import. Treat it
+    // as such so the consumer edge to `pkg` is still recorded.
+    if (!emittedAny) {
+      pushImport({ name: null, imported_as: null, import_form: 'side-effect', type_only: false });
+    }
   }
 
   // Dynamic `import(spec)` and `require(spec)` rows. Both live inside visit()
@@ -504,17 +517,21 @@ function extractFromFile(filePath, pkgName, pkgRoot, { collectImports, includeIm
     const parent = callNode.parent;
     if (parent && ts.isVariableDeclaration(parent)) {
       if (ts.isObjectBindingPattern(parent.name)) {
+        let emittedAny = false;
         for (const el of parent.name.elements) {
           if (!ts.isIdentifier(el.name)) continue; // skip nested binding patterns in v1
           const localName = el.name.text;
           const originName = el.propertyName && ts.isIdentifier(el.propertyName)
             ? el.propertyName.text
             : localName;
           pushRequire({ name: originName, imported_as: localName });
+          emittedAny = true;
         }
-        return;
-      }
-      if (ts.isIdentifier(parent.name)) {
+        if (emittedAny) return;
+        // Destructure consists entirely of nested binding patterns
+        // (`const {a:{b}} = require('pkg')`). Fall through to the bare-call
+        // form so at least the package consumer edge is recorded.
+      } else if (ts.isIdentifier(parent.name)) {
         pushRequire({ name: '*', imported_as: parent.name.text });
         return;
       }

pipeline/_tests/test_substrate.sh

@@ -142,6 +142,19 @@ assert_nonzero_exit "$rc" "malformed index exits nonzero"
 assert_contains "$out" "malformed index.json" "error message mentions malformed"
 rm -rf "$SCRATCH" "$BAD_MOCK"
 
+echo "=== fetch-catalogs.sh: exits 1 when every fetch fails (regression for review feedback) ==="
+SCRATCH=$(mk_scratch)
+DEAD_MOCK=$(mk_scratch)
+# Build a "bucket" that has an index pointing at keys that don't exist.
+cp "$MOCK/index.json" "$DEAD_MOCK/"
+# Intentionally omit by-repo/* — every GET will fail.
+out=$(AUDIT_BUCKET_URL="file://$DEAD_MOCK" AUDIT_LOCAL_CACHE="$SCRATCH" \
+  bash "$PIPELINE/fetch-catalogs.sh" 2>&1)
+rc=$?
+assert_nonzero_exit "$rc" "all-failed fetch exits nonzero"
+assert_contains "$out" "0 repos present" "summary reports zero present repos"
+rm -rf "$SCRATCH" "$DEAD_MOCK"
+
 echo "=== fetch-catalogs.sh: sha256 mismatch handled ==="
 SCRATCH=$(mk_scratch)
 TAMPER_MOCK=$(mk_scratch)
@@ -192,6 +205,45 @@ assert_eq "0d733bd04c183bd79e59774d8e9a7cf20cd46463abfb8f8489491fb8030dce63" "$a
   "rebuilt sha256 matches the canonical fixture value"
 rm -rf "$SCRATCH"
 
+echo "=== refresh-index.mjs: --bucket-fs preflight rejects nonexistent dir ==="
+out=$(node "$PIPELINE/refresh-index.mjs" --bucket-fs /tmp/does-not-exist-xyz123 2>&1)
+rc=$?
+assert_nonzero_exit "$rc" "nonexistent dir exits nonzero"
+assert_contains "$out" "not a directory" "error message names the missing dir"
+
+echo "=== refresh-index.mjs: latest.json points at older prefix than bucket newest (drift) ==="
+SCRATCH=$(mk_scratch)
+cp -r "$MOCK/." "$SCRATCH/"
+rm -f "$SCRATCH/index.json"
+# Plant a future snapshot under fake-repo-a (must use hex chars for the short_sha).
+mkdir -p "$SCRATCH/by-repo/fake-repo-a/2099-12-31T23-59-59Z_aabbcc1"
+cat > "$SCRATCH/by-repo/fake-repo-a/2099-12-31T23-59-59Z_aabbcc1/type-catalog.json" <<'JSON'
+{"schema_version":"1.1","extractor":{"language":"typescript","name":"type-catalog","version":"0.5.0"},"entries":[]}
+JSON
+# Don't update latest.json — that's the drift scenario.
+out=$(node "$PIPELINE/refresh-index.mjs" --bucket-fs "$SCRATCH" 2>&1)
+assert_contains "$out" "but bucket newest is" "drift warning surfaced"
+prefix=$(jq -r '.repos[] | select(.path_segment == "fake-repo-a") | .latest.prefix' "$SCRATCH/index.json")
+assert_eq "by-repo/fake-repo-a/2099-12-31T23-59-59Z_aabbcc1/" "$prefix" \
+  "rebuilt prefix reflects bucket newest (not stale latest.json)"
+short_sha=$(jq -r '.repos[] | select(.path_segment == "fake-repo-a") | .latest.short_sha' "$SCRATCH/index.json")
+assert_eq "aabbcc1" "$short_sha" "short_sha derived from prefix directory name"
+commit_sha=$(jq -r '.repos[] | select(.path_segment == "fake-repo-a") | .latest.commit_sha' "$SCRATCH/index.json")
+assert_eq "null" "$commit_sha" "commit_sha null when stale latest.json can't be trusted"
+rm -rf "$SCRATCH"
+
+echo "=== refresh-index.mjs: canonical repo name falls back to path-segment alone (no org guess) ==="
+SCRATCH=$(mk_scratch)
+mkdir -p "$SCRATCH/by-repo/org-less-repo/2026-05-30T10-00-00Z_aaa1111"
+cat > "$SCRATCH/by-repo/org-less-repo/2026-05-30T10-00-00Z_aaa1111/type-catalog.json" <<'JSON'
+{"schema_version":"1.1","extractor":{"language":"typescript","name":"type-catalog","version":"0.5.0"},"entries":[]}
+JSON
+# Intentionally no latest.json — forces the fallback path.
+node "$PIPELINE/refresh-index.mjs" --bucket-fs "$SCRATCH" >/dev/null 2>&1
+canonical=$(jq -r '.repos[0].repo' "$SCRATCH/index.json")
+assert_eq "org-less-repo" "$canonical" "canonical name == path_segment when latest.json absent (no wxyc/ guess)"
+rm -rf "$SCRATCH"
+
 echo "=== refresh-index.mjs: --known-repos surfaces missing repos ==="
 SCRATCH=$(mk_scratch)
 cp -r "$MOCK/." "$SCRATCH/"
@@ -311,6 +363,29 @@ assert_nonzero_exit "$rc" "bare-array publish exits nonzero"
 assert_contains "$out" "REFUSED" "refusal message emitted"
 rm -rf "$PUB_BUCKET" "$PUB_CAT"
 
+echo "=== publish-catalog.sh: mixed-validity batch uploads NOTHING (no orphan SHA prefix) ==="
+PUB_BUCKET=$(mk_scratch)
+PUB_CAT=$(mk_scratch)
+# One good catalog, one bare-array (refused). Pre-fix: the good one
+# uploaded before the bad one's validation failed — leaving an orphan.
+cat > "$PUB_CAT/type-catalog.json" <<'JSON'
+{"schema_version":"1.1","extractor":{"language":"typescript","name":"type-catalog","version":"0.5.0"},"entries":[]}
+JSON
+printf '[{"name":"x"}]\n' > "$PUB_CAT/function-catalog.json"
+out=$(bash "$PIPELINE/publish-catalog.sh" \
+  --repo wxyc/mixed --sha bbbbbbb1234567 --catalogs-dir "$PUB_CAT" \
+  --bucket-fs "$PUB_BUCKET" --skip-refresh 2>&1)
+rc=$?
+assert_nonzero_exit "$rc" "mixed-batch publish exits nonzero"
+if [ -d "$PUB_BUCKET/by-repo/wxyc-mixed" ]; then
+  FAIL=$((FAIL + 1))
+  echo "  ✗ orphan SHA prefix created in the bucket"
+else
+  PASS=$((PASS + 1))
+  echo "  ✓ bucket untouched on validation failure"
+fi
+rm -rf "$PUB_BUCKET" "$PUB_CAT"
+
 echo "=== publish-catalog.sh: --skip-refresh leaves index alone ==="
 PUB_BUCKET=$(mk_scratch)
 PUB_CAT=$(mk_scratch)

pipeline/fetch-catalogs.sh

@@ -155,21 +155,24 @@ fi
 new_count=0
 cached_count=0
 fail_count=0
-seen_repos_file="$cache_dir/.fetch-seen-repos.tmp"
-: > "$seen_repos_file"
+# Only repos with at least one cached-or-fetched catalog count as "present
+# in cache." A repo whose every catalog fails to fetch does NOT count —
+# otherwise the script would exit 0 with an empty cache and downstream
+# `jq -s 'map(.entries) | add' */latest.json` would silently produce zero
+# results.
+present_repos_file="$cache_dir/.fetch-present-repos.tmp"
+: > "$present_repos_file"
 
 log "[2/3] Fetching catalog objects from $bucket_url"
 
 while IFS=$'\t' read -r repo key expected_sha; do
-  echo "$repo" >> "$seen_repos_file"
-  # Flatten path-segment: index already maps repo -> path_segment, but the
-  # key contains the segment, so we can just store under cache_dir/<key>.
   dest="$cache_dir/$key"
   if [ -f "$dest" ]; then
     actual_sha=$(sha256_of "$dest")
     if [ "$actual_sha" = "$expected_sha" ]; then
       log "  cached  $repo  $(basename "$key")"
       cached_count=$((cached_count + 1))
+      echo "$repo" >> "$present_repos_file"
       continue
     fi
   fi
@@ -178,6 +181,7 @@ while IFS=$'\t' read -r repo key expected_sha; do
     if [ "$actual_sha" = "$expected_sha" ]; then
       log "  fetched $repo  $(basename "$key")"
       new_count=$((new_count + 1))
+      echo "$repo" >> "$present_repos_file"
     else
       echo "  SHA MISMATCH $repo  $(basename "$key"): index=$expected_sha actual=$actual_sha" >&2
       rm -f "$dest"
@@ -191,13 +195,13 @@ done <<EOF
 $work_list
 EOF
 
-distinct_repos=$(sort -u "$seen_repos_file" | wc -l | tr -d ' ')
-rm -f "$seen_repos_file"
+present_repos=$(sort -u "$present_repos_file" | wc -l | tr -d ' ')
+rm -f "$present_repos_file"
 
-log "[3/3] Summary: $distinct_repos repos / $new_count fetched / $cached_count cached / $fail_count failed"
+log "[3/3] Summary: $present_repos repos present / $new_count fetched / $cached_count cached / $fail_count failed"
 log "Cache: $cache_dir"
 
-if [ "$distinct_repos" -eq 0 ]; then
+if [ "$present_repos" -eq 0 ]; then
   exit 1
 fi
 exit 0

pipeline/publish-catalog.sh

@@ -120,13 +120,18 @@ if [ -z "$catalog_files" ]; then
   exit 1
 fi
 
-echo "[1/3] Validating and uploading catalogs..." >&2
-
-# IFS protects against catalog names with spaces (shouldn't happen but cheap).
+echo "[1/4] Validating every catalog before any upload..." >&2
+
+# Validate all files first — only after every catalog passes do we touch
+# the bucket. Prevents the orphan-upload case where file A validates+uploads
+# then file B fails validation, leaving A stranded under the SHA prefix
+# with no latest.json. The subshell `exit 1` inside the pipe-then-read loop
+# wouldn't terminate the script, so we record failure to a tempfile and
+# check it after the loop.
+val_status="/tmp/publish-catalog-val.$$.status"
+: > "$val_status"
 echo "$catalog_files" | while IFS= read -r path; do
   filename=$(basename "$path")
-  kind="${filename%.json}"
-  # Schema check: must be the v1.1 wrapper object with the required fields.
   if ! jq -e '
         type == "object"
         and (.schema_version | type == "string")
@@ -136,15 +141,26 @@ echo "$catalog_files" | while IFS= read -r path; do
              or (.nodes | type == "array"))
       ' "$path" >/dev/null 2>&1; then
     echo "  REFUSED: $filename is not a v1.1 wrapper-shaped catalog" >&2
-    exit 1
+    echo "fail" >> "$val_status"
   fi
+done
+if [ -s "$val_status" ]; then
+  rm -f "$val_status"
+  echo "publish-catalog.sh: validation failed; nothing uploaded" >&2
+  exit 1
+fi
+rm -f "$val_status"
+
+echo "[2/4] Uploading catalogs..." >&2
+echo "$catalog_files" | while IFS= read -r path; do
+  filename=$(basename "$path")
   upload "$path" "${prefix}/${filename}"
   echo "  uploaded ${prefix}/${filename}" >&2
 done
 
-# We re-derive metadata in a second pass because the `while ... | read` loop
-# above runs in a subshell, so any vars set in there don't survive.
-echo "[2/3] Building latest.json pointer..." >&2
+# We re-derive metadata in a separate pass because the `while ... | read`
+# loop above runs in a subshell, so any vars set in there don't survive.
+echo "[3/4] Building latest.json pointer..." >&2
 echo "$catalog_files" | while IFS= read -r path; do
   filename=$(basename "$path")
   kind="${filename%.json}"
@@ -173,7 +189,7 @@ upload "$latest_tmp" "by-repo/${path_segment}/latest.json"
 echo "  wrote by-repo/${path_segment}/latest.json" >&2
 rm -f "$latest_tmp" "/tmp/publish-catalog-meta.$$.txt"
 
-echo "[3/3] Refreshing index.json..." >&2
+echo "[4/4] Refreshing index.json..." >&2
 if [ "$skip_refresh" -eq 1 ]; then
   echo "  skipped (--skip-refresh)" >&2
 else