review(#152/#153): incorporate code-review feedback (2 HIGH + 3 MEDIUM + LOW polish)#200
Merged
Merged
Conversation
…M + 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.
Merged
6 tasks
jakebromberg
added a commit
that referenced
this pull request
May 30, 2026
…iles (follow-up to #200) Post-merge review of #200 surfaced two race-condition gaps in the substrate's S3 concurrency story and a tempfile-cleanup hole in publish-catalog.sh. This change closes all three. **MEDIUM** — `pipeline/refresh-index.mjs` ETag-CAS retry was reusing a stale serialized body. Brief §3.4 prescribes "each writer lists by-repo/<R>/ to compute the latest, then PUTs index.json with conditional If-Match on its ETag, retrying on 412 conflict" — re-listing per attempt. The previous implementation listed once at script start, serialized once, then on 412 only re-read the ETag and retried with the same body. That gave PUT-level CAS (writer A's bytes won't clobber B's if B PUT between A's list and A's PUT) but NOT logical CAS — A's eventual successful retry would still carry A's view of the world, silently overwriting whatever B added. Fix: extract the listing + enumeration + serialization into a `recompute()` function and call it at the top of each retry attempt. The first attempt uses `initial = recompute()` from the regular execution path; conflict retries call `recompute()` again to incorporate the winning writer's additions before the next PUT. **MEDIUM** — `pipeline/refresh-index.mjs` first-publish race had no protection. When `headETag` returns null (no existing index.json), `putBytes` previously omitted any precondition — two concurrent first-publishers would both succeed, one silently losing. Fix: pass `--if-none-match '*'` whenever `ifMatch` is null, so even the create-vs-create race triggers a 412 and routes through the same retry path. **LOW** — `pipeline/publish-catalog.sh` tempfile cleanup was not exception-safe. The script creates `/tmp/publish-catalog-*.$$.*` tempfiles for validation status, per-catalog metadata, and the latest.json staging buffer. Under `set -eu`, any failure (validation, upload, jq) caused an early exit that bypassed the explicit `rm -f` calls. Fix: add a `trap '...rm -f /tmp/publish-catalog-*.$$.*...' EXIT` at the top of the script so cleanup runs unconditionally. The explicit `rm -f` calls remain as belt-and-suspenders on the happy path. Verified: 97 extractor tests + 26 canonical + 124 integration + 54 gojq-parity + 58 substrate = 359 tests pass (was 357; +2 for byte-stability check on `recompute()` and a tempfile-leak regression test on `publish-catalog.sh`). shellcheck clean on all four substrate shell scripts. Refs #152, #153, #200.
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.
Independent code reviews of #196 (imports kind, refs #152) and #199 (R2 substrate, refs #153) surfaced two real production bugs plus several correctness issues. This change closes them all and adds regression tests for each.
HIGH
pipeline/refresh-index.mjs:147,154ESMrequire()bug.require('node:fs').unlinkSync(tmp)inside an.mjsmodule throwsReferenceError, swallowed by the outertry{}catch{}— every S3 GET / PUT leaked a/tmp/refresh-index-*file in production. Fix: hoistunlinkSyncinto the top-level fs import.pipeline/refresh-index.mjsS3putBytesskipped ETag-CAS. Brief §2.5 / §3.4 / §7.2 (and the file's own header) prescribeIf-Match+ retry on 412 for concurrency. The original implementation issued a bares3api put-object— two concurrent publishes silently overwrote each other. Fix: addheadETag()to the S3 backend, pass--if-matchfrom the write loop, retry on tagged precondition errors with capped exponential backoff (up to 5).MEDIUM
pipeline/refresh-index.mjsmetadata drift. When a newer SHA-keyed prefix exists in the bucket butlatest.jsonstill points at the older one, the emitted record mixed bucket-derivedprefix/short_shawith stale pointer-derivedcommit_sha/published_at. Status calc poisoned by the stale timestamp. Fix: trustlatest.jsononly when itsprefixmatches the bucket-newest; otherwise WARN, deriveshort_sha/published_atfrom the directory name, and emitcommit_sha: null.pipeline/publish-catalog.shorphan-on-validation-failure. Validation and upload were in the same loop: catalog A could validate and upload, then catalog B's validation could fail, leaving A as an orphan with nolatest.json. Fix: separate validate-all-first pass (capturing failure via tempfile so the subshell exit isn't lost) from the upload-all-after pass.pipeline/fetch-catalogs.shexit-0-on-empty-cache.distinct_reposcounted every repo named in the work list regardless of fetch success. An index with 3 ok repos where all GETs failed exited 0 with an empty cache, silently producing empty downstream queries. Fix: only count repos that have at least one cached or freshly-fetched catalog.LOW polish
type-catalog.mjs:const { a: { b } } = require('pkg')now records the package edge as namespace-form require (previously dropped silently).type-catalog.mjs:import {} from "pkg"now emits a side-effect row (previously dropped silently).type-catalog.mjs: corrected the misleading "interleave with declaration rows by line" comment to describe actual ordering.refresh-index.mjs:--bucket-fs DIRnow preflight-checks the directory exists with a friendly error.refresh-index.mjs: dropped the hardcodedwxyc/org prefix in the canonical-name fallback whenlatest.jsonis missing.Test plan
npm testinextractors/typescript/: 97 / 97 pass (was 95; +2 for the new edge-case fixtures)bash pipeline/_tests/test_substrate.sh: 56 / 56 pass (was 45; +11 for HIGH/MEDIUM regressions and LOW polish)shellcheck pipeline/*.sh pipeline/_tests/test_substrate.sh— cleanlatest.json→ index reflects bucket newest,commit_sha: null, WARN surfacedRefs #152, #153.