feat: @indexwright/record — query capture (v0.2.0) - #9
Conversation
Empty commit to open the PR before the work lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implements SPEC §7. `indexwright-record -- npm test` runs a suite with FIRESTORE_EMULATOR_HOST pointed at a pass-through proxy and writes the query shapes it observed to firestore.queries.json. The proxy has to be invisible or it has changed what it was measuring, so it forwards bodies, trailers, and trailers-only errors untouched, and tells HTTP/2 from HTTP/1.1 by the connection preface rather than refusing the latter — the emulator's own data-clearing endpoint is HTTP/1.1, and a suite that calls it would otherwise break by being recorded. Verified against the real emulator: client behaviour through the proxy is byte-identical to client behaviour without it, on the success path and on INVALID_ARGUMENT. Capture needs no gRPC stack. @grpc/grpc-js is a client/server library for services known at build time, not a transparent proxy, and §7 already requires a closed operator vocabulary with unknown enum values counted as unsupported-shape — so the enum table has to be in-tree whichever decoder reads the bytes. What is left is varint and length-delimited parsing over field numbers a released .proto cannot renumber. @indexwright/record therefore ships with no runtime dependencies either. Two skip reasons are not in §7 yet and are added in the following commit: unsupported-encoding for a message compressed with something this package cannot undo, and undecodable-message for bytes that do not parse. Without them a compressed request would be dropped silently, which is the failure §7 exists to prevent. The decoder is tested against RunQueryRequest bytes a real @google-cloud/ firestore client emitted, committed as fixtures so CI needs no Java; the expected shape for each is written by hand rather than generated, so the assertion is about what the wire means and not about what the decoder does. scripts/capture-fixtures.mjs regenerates them against a stub server, with no emulator involved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
§3 justified splitting the family with a gRPC stack that capture then turned out not to need: @grpc/grpc-js implements clients and servers for services known at build time and has no part in a transparent proxy, and §7's closed operator vocabulary requires an in-tree enum table whichever library reads the bytes. The split survives on check, which replays a corpus and needs a real Firestore client — the one dependency §8 exists to keep out of a linter. Also records that @indexwright/record does not depend on indexwright yet. Capture reads no index declarations, so the index model and the json contract are check's needs; declaring it early would put a package in an adopter's tree that nothing imports. Adds two skip reasons the implementation needed and the format did not have. unsupported-encoding covers a message compressed with a codec record cannot undo, and undecodable-message covers bytes that do not parse. Both were silent drops, which is the one outcome §7 is written to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Version bumps, changelogs, and the release path for the second package. @indexwright/record releases from its own tag prefix, record-v*, which the linter's v* filter cannot match and vice versa, so neither workflow can publish the other's package. Its first publish cannot use OIDC — npm will not accept a trusted publisher for a package that does not exist yet — so 0.2.0 needs the same one-shot-token bootstrap 0.1.0 needed, with the trusted publisher configured and the token revoked afterwards. The workflow says so at the step that would otherwise look like it handles the first release. verify-package now covers both tarballs. The pack-and-install harness moved to scripts/lib/tarball.mjs so neither package can quietly get the weaker check; the record side runs the installed bin against a stub upstream and reads the corpus it wrote, because a --version that works proves nothing about whether the thing can capture a query. Also dates the 0.1.1 changelog entry, which had been left as "unreleased" in the commit that released it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesRecord package
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant User
participant indexwrightRecord
participant CaptureProxy
participant FirestoreEmulator
participant Recorder
participant CorpusFile
User->>indexwrightRecord: Start command
indexwrightRecord->>CaptureProxy: Start proxy
User->>CaptureProxy: Send Firestore requests
CaptureProxy->>FirestoreEmulator: Forward traffic
CaptureProxy->>Recorder: Record RunQuery payloads
FirestoreEmulator-->>CaptureProxy: Return responses
CaptureProxy-->>User: Forward responses
indexwrightRecord->>CorpusFile: Write corpus
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
spawn's error escaped run(), so `indexwright-record -- typo` exited on an unhandled rejection with a stack trace rather than saying what was wrong. It is a usage error and exits 2, and no corpus is written: nothing ran, so there is nothing the file would be evidence of. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
test/args.test.js (1)
77-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the installation command in this contract test.
The production message also includes the installation command in
src/args.tsLines 58-59. These assertions only check the package name and executable name. A brokennpm install --save-devinstruction would still pass.Proposed assertion
rejects(['record', '--', 'npm', 'test'], /ships as `@indexwright`\/record/); rejects(['record', '--', 'npm', 'test'], /indexwright-record/); + rejects( + ['record', '--', 'npm', 'test'], + /Install it with "npm install --save-dev `@indexwright`\/record"/, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/args.test.js` around lines 77 - 82, Update the contract test for the split-package “record” verb to also assert the installation command emitted by the argument parser, including the expected npm install --save-dev instruction. Keep the existing package-name and executable-name assertions, and target the message produced by the relevant src/args.ts handling.packages/record/src/decode.ts (1)
168-202: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the filter recursion depth.
readFilterandreadCompositeFilterrecurse once per nesting level of the composite tree. The depth comes from the wire bytes, not from this package. A deeply nestedcomposite_filterchain raisesRangeError: Maximum call stack size exceeded. That error is neitherWireError,UnsupportedShape, norVectorQuery, sodecodeRunQueryrethrows it at Line 101, andrecordRunQueryinpackages/record/src/recorder.tsrethrows it too. The proxy then fails instead of counting a skip.Add a depth limit and convert an over-deep tree into
UnsupportedShape, so the failure stays inside the skip vocabulary.♻️ Proposed depth limit
+/** Firestore rejects filter trees far shallower than this; past it the input is not a query. */ +const MAX_FILTER_DEPTH = 32; + -function readFilter(bytes: Uint8Array): FilterNode { +function readFilter(bytes: Uint8Array, depth = 0): FilterNode { + if (depth > MAX_FILTER_DEPTH) throw new UnsupportedShape('filter tree is nested too deeply'); let node: FilterNode | null = null; for (const field of fields(bytes)) { if (field.kind !== 'bytes') continue; switch (field.number) { case FILTER_COMPOSITE: - node = readCompositeFilter(field.value); + node = readCompositeFilter(field.value, depth + 1); break; case FILTER_FIELD: node = readFieldFilter(field.value); break; case FILTER_UNARY: node = readUnaryFilter(field.value); break; default: break; } } if (node === null) throw new UnsupportedShape('filter holds no recognised variant'); return node; } -function readCompositeFilter(bytes: Uint8Array): FilterNode { +function readCompositeFilter(bytes: Uint8Array, depth: number): FilterNode { let op: CompositeOperator | null = null; const filters: FilterNode[] = []; for (const field of fields(bytes)) { if (field.number === COMPOSITE_OP && field.kind === 'varint') { op = COMPOSITE_OPERATORS.get(enumeration(field.value)) ?? null; } else if (field.number === COMPOSITE_FILTERS && field.kind === 'bytes') { - filters.push(readFilter(field.value)); + filters.push(readFilter(field.value, depth)); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/src/decode.ts` around lines 168 - 202, Add a recursion-depth parameter and a package-appropriate maximum to readFilter and readCompositeFilter, incrementing it for each nested composite level and throwing UnsupportedShape when the limit is exceeded. Propagate the depth through recursive readFilter calls while preserving existing parsing and validation behavior for trees within the limit.packages/record/test/corpus.test.js (2)
81-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe replacement target hardcodes the current
CORPUS_VERSIONand the serialiser indentation.The test imports
CORPUS_VERSIONat Line 8 but matches the literal"corpusVersion": 1. IfCORPUS_VERSIONchanges, or ifserialiseCorpuschanges its spacing,replacematches nothing. The text then stays valid,parseCorpussucceeds, and the failure message points atassert.throwsinstead of the stale literal.Build the document through
JSON.parseso the test tracks the exported constant.♻️ Proposed rewrite
test('an unknown corpusVersion is refused rather than read as far as it goes', () => { - const text = serialiseCorpus(buildCorpus([], [])).replace('"corpusVersion": 1', '"corpusVersion": 2'); - assert.throws(() => parseCorpus(text), (error) => error instanceof CorpusError && /corpusVersion/.test(error.message)); + const document = JSON.parse(serialiseCorpus(buildCorpus([], []))); + document.corpusVersion = CORPUS_VERSION + 1; + assert.throws( + () => parseCorpus(JSON.stringify(document)), + (error) => error instanceof CorpusError && /corpusVersion/.test(error.message), + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/test/corpus.test.js` around lines 81 - 84, Update the unknown-version test around parseCorpus to construct the serialised document with JSON.parse, assign corpusVersion to CORPUS_VERSION + 1, and reserialise it with JSON.stringify before parsing. Remove the hardcoded version and formatting-dependent string replacement while preserving the CorpusError assertion.
150-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the expected error so the atomicity assertion cannot pass for the wrong reason.
assert.throwsaccepts any error here. ATypeErrorraised beforewriteCorpusreaches the temp file would satisfy both this line and Line 151, and the test would still report success. Match the injected message.♻️ Proposed matcher
- assert.throws(() => writeCorpus(path, unserialisable)); + assert.throws(() => writeCorpus(path, unserialisable), /serialisation exploded/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/test/corpus.test.js` around lines 150 - 151, Update the assert.throws call in the writeCorpus atomicity test to match the injected serialization error message, ensuring the expected failure originates from writeCorpus rather than an unrelated TypeError while preserving the existing file-content assertion.packages/record/test/decode.test.js (1)
107-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test repeats the test at Line 68.
The body is identical to
'two spellings of one query collapse to one key'. Both compare the same two fixture keys. The name claims a different property, but no assertion isolates the limit.To test the stated property, add a fixture pair that differs only by
.limit(...), or drop this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/test/decode.test.js` around lines 107 - 113, The test named “limit is not recorded” duplicates the key comparison in “two spellings of one query” and does not verify limit handling. Remove this redundant test, or replace its fixtures with a pair of otherwise identical queries that differ only by .limit(...) and assert their decoded keys are equal.packages/record/test/proxy.test.js (1)
38-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe stub discards the request body, so "passes through unchanged" is only checked for the response.
stubUpstreamdrops every data chunk at Line 39 and records only:path. The test at Line 87 then asserts the response body and the path. It does not assert that the proxy forwarded the request bytes intact. A proxy that re-framed or truncated the request would still pass.Collect the request body in the stub and compare it to the framed fixture.
♻️ Proposed stub change
function stubUpstream({ trailersOnly = false } = {}) { const seen = []; + const bodies = []; const server = createHttp2Server(); server.on('stream', (stream, headers) => { - stream.on('data', () => {}); + const chunks = []; + stream.on('data', (chunk) => chunks.push(chunk)); stream.on('end', () => { seen.push(headers[':path']); + bodies.push(Buffer.concat(chunks)); @@ - return { server, seen }; + return { server, seen, bodies }; }Then assert in the first test:
assert.deepEqual(upstream.seen, ['/google.firestore.v1.Firestore/RunQuery']); + assert.deepEqual(upstream.bodies, [frame(fixtureMessage('a collection group query'))]);Also applies to: 87-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/test/proxy.test.js` around lines 38 - 41, The stubUpstream stream handler currently discards request data, so the test does not verify request-body forwarding. Accumulate the chunks received by the `stream.on('data')` handler, and in the first test’s assertions compare the reconstructed request body with the framed fixture while retaining the existing path and response checks.packages/record/src/proxy.ts (1)
179-218: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDestroy the upstream stream when the client stream closes without an error.
Line 180 destroys
upstreamonly onerror. A client that cancels an RPC ends theServerHttp2StreamwithRST_STREAM, which emitscloseand, depending on the code, noerror. The upstream stream then stays open until the emulator ends it. Over a long suite this keeps cancelledListenandRunQuerystreams alive.Add a
closehandler that destroysupstreamif it is not already finished.♻️ Proposed cleanup
stream.pipe(upstream); stream.on('error', () => upstream.destroy()); + stream.on('close', () => { + if (!upstream.closed) upstream.destroy(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/src/proxy.ts` around lines 179 - 218, Add a close handler for the client stream in the proxy flow alongside the existing error handler, destroying upstream when it has not already finished. Preserve the current error-based cleanup and avoid destroying an already completed upstream stream.packages/record/src/corpus.ts (1)
174-200: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a depth limit in
parseFilter.
parseFilterrecurses once per nesting level of the storedwheretree.parseCorpusis exported public API and reads a file that the tool does not control. A deeply nestedfiltersarray makes the recursion throwRangeError: Maximum call stack size exceededinstead ofCorpusError, so a caller that catchesCorpusErrorsees an unexpected failure.normaliseRootandserialiseFilterat Line 163 recurse over the same tree.A depth counter keeps the refusal inside the documented error type.
♻️ Proposed depth cap
-function parseFilter(value: unknown, at: string): FilterNode { +const MAX_FILTER_DEPTH = 100; + +function parseFilter(value: unknown, at: string, depth = 0): FilterNode { + if (depth > MAX_FILTER_DEPTH) throw new CorpusError(`${at} nests deeper than this format allows`); const node = expectObject(value, at); @@ const filters = expectArray(node['filters'], `${at}.filters`).map((child, index) => - parseFilter(child, `${at}.filters[${index}]`), + parseFilter(child, `${at}.filters[${index}]`, depth + 1), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/src/corpus.ts` around lines 174 - 200, Introduce a depth counter and finite maximum depth for recursive filter processing so excessively nested stored where trees raise CorpusError instead of overflowing the call stack. Update parseFilter to validate the depth before recursing and propagate the incremented depth to child calls; apply the same guard and depth propagation to the corresponding recursive paths in normaliseRoot and serialiseFilter so all three traversals enforce the cap consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-record.yml:
- Around line 55-60: Update the release workflow around the record-v0.2.0
trigger and the final npm publish step so the manually bootstrapped 0.2.0
release is validated and skipped instead of republished. Preserve the existing
publish behavior for subsequent versions, or explicitly exclude the bootstrap
tag from this workflow’s trigger.
- Around line 27-34: Update the setup-node configuration in the release workflow
to disable package-manager caching by setting package-manager-cache to false and
remove the cache: npm option. Keep the existing Node version and
trusted-publishing configuration unchanged.
In `@packages/record/CHANGELOG.md`:
- Around line 14-18: Update the changelog entry for indexwright-record to state
that it runs against the Firestore emulator, which does not enforce composite
indexes, so missing indexes cannot cause the recording suite to fail. Explain
that the recorded firestore.queries.json corpus is intended for the future check
command to detect missing indexes during replay, while preserving the suite
exit-code behavior.
In `@packages/record/README.md`:
- Line 33: Update the fenced CLI output block in the README to specify a
language identifier, using text or console, so the Markdown lint rule is
satisfied.
In `@packages/record/src/cli.ts`:
- Around line 54-59: Update the child-process execution flow around runChild and
its caller run to catch spawn failures such as ENOENT or EACCES, convert them to
the documented exit code 2, and still close capture in the existing finally
path. Ensure the failure is handled as a normal command error so run completes
without an unhandled rejection and already captured traffic can be written to
the corpus.
In `@packages/record/src/proxy.ts`:
- Around line 76-77: Update startCapture and the shared upstream client flow
around client.request() to catch synchronous request failures, fail only the
affected stream, and prevent uncaught exceptions from terminating the recorder.
Track the session’s closed or unusable state and lazily establish a replacement
connection before later requests, reusing the existing upstream connection setup
and error warning behavior.
In `@packages/record/src/recorder.ts`:
- Around line 14-17: Update the DECOMPRESSORS map so both gunzipSync and
inflateSync receive maxOutputLength set to 8 * 1024 * 1024, preserving the
existing catch path that converts oversized decompression failures into
undecodable-message.
In `@src/args.ts`:
- Around line 37-39: Update the command lookup that uses ELSEWHERE to check that
the command is an own property before destructuring or casting its value.
Replace the inherited-property-prone in ELSEWHERE condition in the relevant
argument handling path with an own-property check, preserving the generic
unknown-command error for names such as constructor and toString.
---
Nitpick comments:
In `@packages/record/src/corpus.ts`:
- Around line 174-200: Introduce a depth counter and finite maximum depth for
recursive filter processing so excessively nested stored where trees raise
CorpusError instead of overflowing the call stack. Update parseFilter to
validate the depth before recursing and propagate the incremented depth to child
calls; apply the same guard and depth propagation to the corresponding recursive
paths in normaliseRoot and serialiseFilter so all three traversals enforce the
cap consistently.
In `@packages/record/src/decode.ts`:
- Around line 168-202: Add a recursion-depth parameter and a package-appropriate
maximum to readFilter and readCompositeFilter, incrementing it for each nested
composite level and throwing UnsupportedShape when the limit is exceeded.
Propagate the depth through recursive readFilter calls while preserving existing
parsing and validation behavior for trees within the limit.
In `@packages/record/src/proxy.ts`:
- Around line 179-218: Add a close handler for the client stream in the proxy
flow alongside the existing error handler, destroying upstream when it has not
already finished. Preserve the current error-based cleanup and avoid destroying
an already completed upstream stream.
In `@packages/record/test/corpus.test.js`:
- Around line 81-84: Update the unknown-version test around parseCorpus to
construct the serialised document with JSON.parse, assign corpusVersion to
CORPUS_VERSION + 1, and reserialise it with JSON.stringify before parsing.
Remove the hardcoded version and formatting-dependent string replacement while
preserving the CorpusError assertion.
- Around line 150-151: Update the assert.throws call in the writeCorpus
atomicity test to match the injected serialization error message, ensuring the
expected failure originates from writeCorpus rather than an unrelated TypeError
while preserving the existing file-content assertion.
In `@packages/record/test/decode.test.js`:
- Around line 107-113: The test named “limit is not recorded” duplicates the key
comparison in “two spellings of one query” and does not verify limit handling.
Remove this redundant test, or replace its fixtures with a pair of otherwise
identical queries that differ only by .limit(...) and assert their decoded keys
are equal.
In `@packages/record/test/proxy.test.js`:
- Around line 38-41: The stubUpstream stream handler currently discards request
data, so the test does not verify request-body forwarding. Accumulate the chunks
received by the `stream.on('data')` handler, and in the first test’s assertions
compare the reconstructed request body with the framed fixture while retaining
the existing path and response checks.
In `@test/args.test.js`:
- Around line 77-82: Update the contract test for the split-package “record”
verb to also assert the installation command emitted by the argument parser,
including the expected npm install --save-dev instruction. Keep the existing
package-name and executable-name assertions, and target the message produced by
the relevant src/args.ts handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bcd6d97-78f8-4fba-a19f-8b099c896c44
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.github/workflows/release-record.ymlCHANGELOG.mdREADME.mdSPEC.mdpackage.jsonpackages/record/CHANGELOG.mdpackages/record/LICENSEpackages/record/README.mdpackages/record/package.jsonpackages/record/scripts/capture-fixtures.mjspackages/record/src/args.tspackages/record/src/cli.tspackages/record/src/corpus.tspackages/record/src/decode.tspackages/record/src/index.tspackages/record/src/proxy.tspackages/record/src/recorder.tspackages/record/src/shape.tspackages/record/src/types.tspackages/record/src/version.tspackages/record/src/wire.tspackages/record/test/cli.test.jspackages/record/test/corpus.test.jspackages/record/test/decode.test.jspackages/record/test/fixtures/run-query.jsonpackages/record/test/proxy.test.jspackages/record/test/shape.test.jspackages/record/tsconfig.jsonscripts/lib/tarball.mjsscripts/verify-package.mjsscripts/verify-record-package.mjssrc/args.tstest/args.test.jstsconfig.base.jsontsconfig.json
| - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 | ||
| with: | ||
| # 24, not the engines floor of 22, because trusted publishing needs npm >= 11.5.1 and | ||
| # Node 22 still ships npm 10. The floor is exercised by the ci workflow on every PR. | ||
| node-version: '24' | ||
| # registry-url is deliberately absent; see the comment in release.yml for why setting it | ||
| # breaks the OIDC exchange. | ||
| cache: npm |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate workflow =="
git ls-files | grep -F '.github/workflows/release-record.yml' || true
echo "== file excerpt =="
if [ -f .github/workflows/release-record.yml ]; then
cat -n .github/workflows/release-record.yml | sed -n '1,140p'
fi
echo "== release.yml cache mentions =="
if [ -f .github/workflows/release.yml ]; then
rg -n "package-manager-cache|cache: npm|setup-node|npm publish|oidc|id-token" .github/workflows/release.yml || true
else
echo "release.yml not found"
fi
echo "== search release-record cache/security references =="
rg -n "release-record|package-manager-cache|cache: npm|setup-node@820762786026740c76f36085b0efc47a31fe5020|trusted publishing|Trusted Publishing|npm publish" .github/workflows . 2>/dev/null | head -200Repository: uny/indexwright
Length of output: 5614
🌐 Web query:
npm trusted publishing CI package-manager-cache false setup-node cache npm
💡 Result:
When using npm Trusted Publishing (OIDC) with the actions/setup-node GitHub Action, it is standard practice and highly recommended to set package-manager-cache: false [1][2][3]. The actions/setup-node action automatically enables caching for npm if your package.json contains a packageManager or devEngines.packageManager field set to npm [1][4][5]. In release workflows or those with elevated privileges, disabling this automatic cache is recommended for several reasons: 1. Security (Cache Poisoning): Caching dependencies can introduce security risks, such as cache poisoning, where an attacker might inject malicious code into a high-privilege workflow through the GitHub Actions cache [6]. Disabling the cache mitigates this risk in sensitive environments [1][4][7]. 2. Build Reproducibility: For release builds, you typically want a clean, reproducible environment [3]. Caching can sometimes mask issues by inadvertently using stale or modified tarballs instead of fresh, verified dependencies [3]. Example Configuration: - uses: actions/setup-node@v6 with: node-version: '24' registry-url: 'https://registry.npmjs.org' package-manager-cache: false # Disables automatic caching Note: If you use registry-url, the actions/setup-node action automatically writes a configuration line containing _authToken to your.npmrc [8]. While this is standard for token-based authentication, it can occasionally interfere with the OIDC flow if npm mistakenly detects it as a misconfigured authentication attempt [8]. In such cases, some users manually strip the _authToken line from the.npmrc after the setup-node step [8].
Citations:
- 1: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md
- 2: https://docs.npmjs.com/trusted-publishers/
- 3: https://www.rabinarayanpatra.com/blogs/how-to-enable-npm-trusted-publishing-github-actions-oidc
- 4: https://github.com/actions/setup-node
- 5: https://github.com/actions/setup-node/blob/v6/README.md
- 6: Trusted Publisher documentation actions/setup-node#1445
- 7: https://github.com/marketplace/actions/setup-node-js-environment
- 8: registry-url writes _authToken line that breaks npm Trusted Publisher OIDC when no NODE_AUTH_TOKEN is set actions/setup-node#1551
🌐 Web query:
npmjs.com trusted-publishers package-manager-cache actions/setup-node cache npm
💡 Result:
Trusted publishing and caching in npm workflows are distinct security and performance features that serve different purposes: Trusted publishing is a security feature that allows packages to be published from CI/CD environments (like GitHub Actions) using OpenID Connect (OIDC) instead of long-lived npm tokens [1][2]. By establishing a trust relationship between your CI/CD provider and npm, short-lived, workflow-specific credentials are used for publishing [1][3]. This also enables automatic generation of provenance attestations, which provide cryptographic proof of a package's origin and build process [2][3]. Trusted publishing is configured via the npm website or the npm trust CLI command [4][5]. Caching in npm workflows is a performance feature managed primarily by CI/CD tools like actions/setup-node [6][7]. It stores global package data to speed up dependency installation [8][7]. - Automatic caching: In actions/setup-node, caching for npm is automatically enabled if the package.json file contains a packageManager or devEngines.packageManager field set to npm [6][9]. - Control: This behavior is governed by the package-manager-cache input, which defaults to true [6][10]. You can disable it by setting package-manager-cache: false in your workflow file [8][9]. - Security recommendation: For workflows handling sensitive information or requiring elevated privileges, it is recommended to set package-manager-cache: false if caching is not explicitly required [6][7]. These systems operate independently: trusted publishing secures the delivery of your package to the registry [1][2], while workflow caching optimizes the speed of installing dependencies within your CI/CD environment [6][8].
Citations:
- 1: https://docs.npmjs.com/trusted-publishers/
- 2: https://github.blog/changelog/2025-07-31-npm-trusted-publishing-with-oidc-is-generally-available/
- 3: https://cheatsheetseries.owasp.org/cheatsheets/NPM_Security_Cheat_Sheet.html
- 4: https://docs.npmjs.com/cli/v12/commands/npm-trust/
- 5: https://philna.sh/blog/2026/01/28/trusted-publishing-npm/
- 6: https://github.com/actions/setup-node
- 7: https://github.com/actions/setup-node/blob/v6/README.md
- 8: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md
- 9: https://github.com/marketplace/actions/setup-node-js-environment
- 10: https://github.com/actions/setup-node/tree/refs/heads/main
🌐 Web query:
npmjs.com trusted-publishers docs "package-manager-cache"
💡 Result:
"Trusted publishers" and "package manager cache" refer to two distinct features within the npm ecosystem. Trusted publishing is a security feature for authenticated package publication, while the npm cache is a local utility for managing downloaded package data [1][2]. Trusted Publishers Trusted publishing allows you to publish packages to the npm registry using OpenID Connect (OIDC) authentication instead of relying on long-lived access tokens [1][3]. This feature integrates with CI/CD providers (such as GitHub Actions, GitLab CI/CD, and CircleCI) to create a trust relationship where short-lived, workflow-specific credentials are used for each publish [1][4]. Key aspects include: - Security: It eliminates the risk associated with storing and potentially exfiltrating long-lived npm tokens [1][3][5]. - Provenance: When using trusted publishing, npm automatically generates and publishes provenance attestations, providing verifiable information about how and where a package was built [3][6][4]. - Requirements: It requires npm CLI version 11.5.1 or later and Node.js version 22.14.0 or higher [1][4]. - Configuration: Trusted publishers are configured at the package level on npmjs.com by specifying the CI/CD provider and the specific workflow details (e.g., repository and workflow filename) that are permitted to publish [1][3][5]. npm Cache The npm cache is a local, self-healing directory used by the npm CLI to store package data, HTTP request responses, and other related information to speed up installations [2][7][8]. Key aspects include: - Functionality: The cache is managed via the npm cache command (e.g., npm cache add, npm cache clean, npm cache verify) [2][7]. It is primarily accessed internally by the npm CLI through tools like pacote [2][8]. - Design: It is a content-addressable cache (based on cacache) and is not intended to be a persistent data store [2][7]. Because it is self-healing, manually clearing the cache is typically unnecessary unless you are reclaiming disk space [2][8]. - Location: The default location varies by operating system (e.g., ~/.npm on Posix systems, %LocalAppData%\npm-cache on Windows) and can be configured via the cache setting in your npm configuration [9][7][8].
Citations:
- 1: https://docs.npmjs.com/trusted-publishers/
- 2: https://docs.npmjs.com/cli/v11/commands/npm-cache/
- 3: https://github.blog/changelog/2025-07-31-npm-trusted-publishing-with-oidc-is-generally-available/
- 4: https://www.rabinarayanpatra.com/blogs/how-to-enable-npm-trusted-publishing-github-actions-oidc
- 5: https://lwn.net/Articles/1076205/
- 6: https://docs.npmjs.com/generating-provenance-statements/
- 7: https://fastly.jsdelivr.net/npm/npm@12.0.2/docs/output/commands/npm-cache.html
- 8: https://unpkg.com/npm@10.9.0/docs/output/commands/npm-cache.html
- 9: https://github.com/npm/cli/blob/e09bdc142a4d9fe247927eb82a49720d5370b293/docs/content/using-npm/config.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packageManager / devEngines fields =="
python3 - <<'PY'
import json, pathlib
for p in pathlib.Path('.').rglob('package.json'):
try:
data = json.loads(p.read_text())
except Exception:
continue
if 'packageManager' in data.get('devEngines' if False else data) or 'packageManager' in data or 'devEngines' in data:
print(p)
print(json.dumps(data.get('packageManager'), default=str))
print(json.dumps(data.get('devEngines'), default=str))
PY
echo "== package record package.json =="
if [ -f packages/record/package.json ]; then
cat -n packages/record/package.json
fi
echo "== setup-node advanced docs local =="
if [ -d node_modules/@actions/setup-node ]; then
rg -n "package-manager-cache|npm cache|Trusted Publishing" node_modules/@actions/setup-node README.md docs 2>/dev/null || true
else
echo "node_modules/@actions/setup-node not present"
fiRepository: uny/indexwright
Length of output: 302
🌐 Web query:
Zizmor "caching" "setup-node" "package-manager-cache" npm
💡 Result:
Zizmor’s cache-poisoning audit is designed to detect configurations where GitHub Actions with built-in caching capabilities could be exploited to persist or retrieve malicious artifacts [1]. In the context of actions/setup-node, Zizmor specifically tracks the package-manager-cache input [2]. Background: Starting with actions/setup-node v5, the action introduced automatic caching for npm projects when the package.json file contains a packageManager or devEngines.packageManager field set to npm [3][4]. This behavior is controlled by the package-manager-cache input, which defaults to true [3][5]. Zizmor's Audit Behavior: Zizmor considers the use of automatic caching in sensitive contexts (like publishing workflows) a potential security risk [1]. To satisfy the Zizmor cache-poisoning audit in environments where you need to explicitly disable caching, you must configure the action as follows [1]: - For actions/setup-node: Set package-manager-cache: false explicitly [1][6]. Known Issues and Nuances: There have been reported cases where Zizmor incorrectly flags workflows even when users attempt to disable caching [7][8]. Specifically: 1. Version Compatibility: Older versions of Zizmor may not have been aware of the input fields introduced in actions/setup-node v5 or v6, leading to false positives [7][8]. Ensuring you are using a current version of Zizmor is recommended. 2. Explicit Disabling: If you are explicitly setting cache: '' and package-manager-cache: false, but still receiving a finding, it may be due to how Zizmor evaluates specific input combinations or empty string values in its audit rules [8]. Maintaining the explicit setting of package-manager-cache: false is the standard mitigation recommended by both the Zizmor documentation and the actions/setup-node maintainers for secure operation [1][6]. If you continue to encounter issues after setting package-manager-cache: false, it is recommended to check the Zizmor issue tracker for your specific version, as the tool is actively updated to support evolving configurations in official GitHub actions [8][9].
Citations:
- 1: https://docs.zizmor.sh/audits/
- 2: https://github.com/zizmorcore/zizmor/blob/563b7b25/crates/zizmor/src/audit/cache_poisoning.rs
- 3: Enhance caching in setup-node with automatic package manager detection actions/setup-node#1348
- 4: https://github.com/actions/setup-node
- 5: https://github.com/marketplace/actions/setup-node-js-environment
- 6: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md
- 7: [BUG]:
cache-poisoningrule doesn't support breaking change ofactions/setup-nodev5 zizmorcore/zizmor#1152 - 8: [BUG]: cache-poisoning rule doesn't support breaking change of actions/setup-node v6 zizmorcore/zizmor#1485
- 9: https://github.com/zizmorcore/zizmor
🏁 Script executed:
#!/bin/bash
set -u
echo "== packageManager / devEngines packageManager fields =="
python3 - <<'PY'
import json, pathlib
for p in pathlib.Path('.').rglob('package.json'):
try:
data = json.loads(p.read_text())
except Exception:
continue
pm = data.get('packageManager')
dev_pm = data.get('devEngines', {}).get('packageManager') if isinstance(data.get('devEngines'), dict) else None
if pm or dev_pm:
print(p)
print("packageManager", pm)
print("devEngines.packageManager", dev_pm)
PY
echo "== package record package.json =="
if [ -f packages/record/package.json ]; then
cat -n packages/record/package.json
fi
echo "== workflow diff/stat if available =="
git diff -- .github/workflows/release-record.yml 2>/dev/null | sed -n '1,180p' || true
git diff --stat -- .github/workflows/release-record.yml 2>/dev/null || trueRepository: uny/indexwright
Length of output: 1805
Disable setup-node caching for trusted publishing.
This job runs npm ci and dependency lifecycle scripts before publishing with OIDC credentials. Set package-manager-cache: false and remove cache: npm so setup-node does not restore or save the npm cache for the release job.
Proposed fix
- cache: npm
+ package-manager-cache: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 | |
| with: | |
| # 24, not the engines floor of 22, because trusted publishing needs npm >= 11.5.1 and | |
| # Node 22 still ships npm 10. The floor is exercised by the ci workflow on every PR. | |
| node-version: '24' | |
| # registry-url is deliberately absent; see the comment in release.yml for why setting it | |
| # breaks the OIDC exchange. | |
| cache: npm | |
| - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 | |
| with: | |
| # 24, not the engines floor of 22, because trusted publishing needs npm >= 11.5.1 and | |
| # Node 22 still ships npm 10. The floor is exercised by the ci workflow on every PR. | |
| node-version: '24' | |
| # registry-url is deliberately absent; see the comment in release.yml for why setting it | |
| # breaks the OIDC exchange. | |
| package-manager-cache: false |
🧰 Tools
🪛 zizmor (1.29.0)
[error] 27-27: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release-record.yml around lines 27 - 34, Update the
setup-node configuration in the release workflow to disable package-manager
caching by setting package-manager-cache to false and remove the cache: npm
option. Keep the existing Node version and trusted-publishing configuration
unchanged.
Source: Linters/SAST tools
| # The FIRST publish of this package cannot use that path — npm will not accept a trusted | ||
| # publisher for a package that does not exist yet, so 0.2.0 is bootstrapped with a one-shot | ||
| # token published by hand, the trusted publisher is configured against the published | ||
| # package, and the token is revoked. Every release after that runs here. This is the same | ||
| # sequence `indexwright` went through for 0.1.0 and 0.1.1. | ||
| - run: npm publish -w @indexwright/record --provenance --access public |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate workflow files =="
git ls-files '.github/workflows/*' || true
echo
echo "== release-record.yml =="
fd -a 'release-record\.yml$' . | sed 's#^\./##'
if [ -f .github/workflows/release-record.yml ]; then
nl -ba .github/workflows/release-record.yml | sed -n '1,120p'
fi
echo
echo "== package/workflow matching search =="
rg -n "record-v[0-9]|`@indexwright/record`|npm publish|npm pack|version"| head -200Repository: uny/indexwright
Length of output: 343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== release-record.yml =="
awk '{printf "%5d\t%s\n", NR, $0}' .github/workflows/release-record.yml
echo
echo "== package/config snippets mentioning record package or publish =="
rg -n "`@indexwright/record`|record-v|npm publish|npm pack|version" .github/workflows/release-record.yml .github/workflows 2>/dev/null || true
echo
echo "== workflow run if/condition syntax summary =="
awk '/if:|matches:|push:|tag:|version|npm publish/ {printf "%5d\t%s\n", NR, $0}' .github/workflows/release-record.yml .github/workflows/release.yml .github/workflows/ci.yml 2>/dev/null || trueRepository: uny/indexwright
Length of output: 8842
🌐 Web query:
npm publish already published version immutable error
💡 Result:
The error message "You cannot publish over the previously published version" occurs because the npm registry enforces strict immutability for package versions [1][2]. Once a specific version number has been published to the registry, it is permanently associated with that package and can never be overwritten, modified, or reused—even if you unpublish that specific version later [3][1][2]. This policy is designed to ensure the security, stability, and reproducibility of the package ecosystem [3][2]. Because systems rely on the "name@version" combination as a unique, immutable identifier, changing or reusing a version number would break dependencies and create security risks [3][2]. To resolve this error, you must follow these steps: 1. Increment the version: Update the version number in your package.json file to a new, unique value [1][4]. You can do this automatically using the npm version command [1]: npm version patch # Increments the patch version (e.g., 1.0.0 to 1.0.1) npm version minor # Increments the minor version (e.g., 1.0.0 to 1.1.0) npm version major # Increments the major version (e.g., 1.0.0 to 2.0.0) 2. Publish again: After bumping the version, run npm publish again [1]. If you receive this error for a version number that you do not believe you have published, it is possible that a previous version with that number was published and subsequently unpublished [4][5]. Because npm registry data is immutable, you still cannot reuse that version number [2]. You can verify the history of published versions by running: npm view versions If you need to mark a previously published version as broken or unwanted, the correct practice is to use the npm deprecate command instead of unpublishing [1][2]. This allows you to provide a warning message to users who download that specific version without breaking existing installations [2].
Citations:
- 1: https://www.javascript-package-publishing.com/package-publishing-and-release/npm-registry-publishing-workflows/fixing-npm-publish-403-forbidden-errors/
- 2: https://docs.npmjs.com/policies/unpublish/
- 3: ERR: You cannot publish over the previously published version 1.1.3. npm/npm#10556
- 4: https://stackoverflow.com/questions/55769038/npm-cannot-publish-over-previously-published-version
- 5: [BUG] You cannot publish over previously published.. but this version has never been published npm/cli#5058
Make record-v0.2.0 an explicit bootstrap path.
record-v0.2.0 matches this workflow trigger and passes the version check. The bootstrap publishes @indexwright/record@0.2.0 manually, so the workflow’s final npm publish -w @indexwright/record --provenance --access public retries an already published immutable version and fails.
Add a bootstrap path that validates and skips an already published initial version, or document a tag process that does not invoke this workflow for 0.2.0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release-record.yml around lines 55 - 60, Update the
release workflow around the record-v0.2.0 trigger and the final npm publish step
so the manually bootstrapped 0.2.0 release is validated and skipped instead of
republished. Preserve the existing publish behavior for subsequent versions, or
explicitly exclude the bootstrap tag from this workflow’s trigger.
Eleven verified must-fixes, each reproduced before the change and again after it. Crashes that ended the recorder, and with it the suite it was running: - The upstream HTTP/2 session is shared by every stream. An emulator that restarts or sends GOAWAY leaves it closed, and `client.request` then throws synchronously inside the `stream` handler. Failed the stream instead, with the error listener registered first so that failing it does not become the same crash one step on. - `readFilter` and `parseFilter` recursed once per nesting level with no ceiling, so a 195KB body — far under the 8MiB cap — reached RangeError, which is neither a WireError nor a CorpusError and escaped both. Both now stop at 100 levels and report the skip the vocabulary has for it. Wire reading: - A negative enum is sign-extended to ten bytes. Read as unsigned it was rejected as `undecodable-message`, the reason SPEC §7 reserves for having misread the wire, for a message that parsed perfectly. - Folding that value back exposed the varint reader accepting up to 2^70, where 2^64+5 wraps to EQUAL and puts a query shape nobody issued in the corpus. The tenth byte now contributes bit 63 and nothing above it. - A RunQuery carrying no message at all was counted nowhere, so a query the proxy saw came out looking like one that was never issued. - gunzipSync and inflateSync were unbounded; a few kilobytes expanded to gigabytes before anything decided the message was too large. Addressing: - `[::1]:8080` became `http://::1:8080`, which is not a URL, so an IPv6 emulator could not be reached at all. An unbracketed literal is now a usage error rather than an address to guess at. Corpus: - `parseCorpus` did not check ordering, so a file read and written back could differ in bytes — the diff-stability the sort exists for. - The temp file was named from the pid and written without O_EXCL, which in a shared directory is a symlink another user can plant. - `corpusVersion` is checked before the member set: adding a member is the normal reason to bump it, and the old order blamed a stray field. Tests: - `an interrupted write leaves the previous corpus intact` induced its failure during serialisation, before any filesystem call, so it passed against a non-atomic implementation. Split into one test for each. - Live coverage added for gzip capture, the empty body, IPv6, deep nesting, the varint ceiling, corpus ordering, negative enums, and the planted symlink. 134 tests to 146. Also: `command in ELSEWHERE` matched inherited names, so `indexwright constructor` reported the package as "undefined"; the sensitiveHeaders symbol was dropped by `Object.entries` and its headers became eligible for HPACK indexing; `Intent` was exported as a function's return type but not as a name; the record tarball's SPEC.md links resolved to nothing once installed; and the CHANGELOG justified writing the corpus on a red suite with an index failure the emulator cannot produce. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/record/CHANGELOG.md (1)
23-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the recorder decision, not proxy rejection.
Lines 37-43 state that unsupported requests are forwarded and reported. The proxy does not reject these requests. Replace “the proxy declines” with wording such as “the recorder cannot capture” to prevent an incorrect network-behavior expectation.
Proposed wording
-- Everything the proxy declines is counted under a closed vocabulary and reported on stderr — +- Everything the recorder cannot capture is counted under a closed vocabulary and reported on stderr —🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/CHANGELOG.md` around lines 23 - 26, Update the changelog wording around the closed-vocabulary list to describe recorder capture limitations rather than proxy rejection: replace “the proxy declines” with wording such as “the recorder cannot capture,” while preserving the listed categories and stderr reporting details.
🧹 Nitpick comments (2)
packages/record/test/proxy.test.js (1)
337-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the fixed sleeps with the events they wait for.
The first
setTimeoutwaits for the proxy's upstream session to exist. On a loaded machine the session may not exist yet, and the loop then destroys nothing. The test still passes, because line 371 accepts anullcode, so it silently stops covering the guarded synchronousclient.requestthrow. Thesessionevent is already observed, so it can be awaited directly.♻️ Proposed change to wait on events
const upstream = stubUpstream(); const sessions = []; + const firstSession = new Promise((resolve) => upstream.server.once('session', resolve)); upstream.server.on('session', (session) => sessions.push(session)); const upstreamAddress = await listen(upstream.server); const capture = await startCapture({ upstream: upstreamAddress, onWarning: () => {} }); try { // Wait for the proxy's session to exist, then take the emulator away under it. - await new Promise((resolve) => setTimeout(resolve, 100)); + await firstSession; upstream.server.close(); - for (const session of sessions) session.destroy(); - await new Promise((resolve) => setTimeout(resolve, 100)); + await Promise.all( + sessions.map( + (session) => + new Promise((resolve) => { + session.once('close', resolve); + session.destroy(); + }), + ), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/test/proxy.test.js` around lines 337 - 348, Update the test setup around the sessions listener to await the upstream server’s session event before closing the server and destroying sessions, replacing the initial fixed setTimeout delay. Preserve the existing cleanup and assertions, and replace the later fixed delay only with the relevant connection/close event needed to ensure the proxy has observed the destroyed sessions before continuing.packages/record/test/corpus.test.js (1)
230-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the exclusive-creation guard.
This test proves only that the temp name is no longer derived from the pid. The random name never collides with the planted symlink, so the test also passes if
flag: 'wx'is removed fromwriteFileSyncin packages/record/src/corpus.ts. The second half of the fix is untested.To cover it, make the temp name injectable, then plant a symlink at the name the write will use and assert that
writeCorpusthrowsEEXISTand leaves the victim file unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/record/test/corpus.test.js` around lines 230 - 251, Update the corpus write path and its test to make the temporary filename injectable, using the injected name in writeCorpus. Add coverage that plants a symlink at that exact name, asserts writeCorpus throws an EEXIST error from exclusive creation, and verifies the victim file remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/record/CHANGELOG.md`:
- Around line 23-26: Update the changelog wording around the closed-vocabulary
list to describe recorder capture limitations rather than proxy rejection:
replace “the proxy declines” with wording such as “the recorder cannot capture,”
while preserving the listed categories and stderr reporting details.
---
Nitpick comments:
In `@packages/record/test/corpus.test.js`:
- Around line 230-251: Update the corpus write path and its test to make the
temporary filename injectable, using the injected name in writeCorpus. Add
coverage that plants a symlink at that exact name, asserts writeCorpus throws an
EEXIST error from exclusive creation, and verifies the victim file remains
unchanged.
In `@packages/record/test/proxy.test.js`:
- Around line 337-348: Update the test setup around the sessions listener to
await the upstream server’s session event before closing the server and
destroying sessions, replacing the initial fixed setTimeout delay. Preserve the
existing cleanup and assertions, and replace the later fixed delay only with the
relevant connection/close event needed to ensure the proxy has observed the
destroyed sessions before continuing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a439e63f-6baa-4b73-8ad3-e97e05eccf04
📒 Files selected for processing (15)
packages/record/CHANGELOG.mdpackages/record/README.mdpackages/record/src/cli.tspackages/record/src/corpus.tspackages/record/src/decode.tspackages/record/src/index.tspackages/record/src/proxy.tspackages/record/src/recorder.tspackages/record/src/wire.tspackages/record/test/cli.test.jspackages/record/test/corpus.test.jspackages/record/test/decode.test.jspackages/record/test/proxy.test.jssrc/args.tstest/args.test.js
🚧 Files skipped from review as they are similar to previous changes (8)
- src/args.ts
- packages/record/src/wire.ts
- packages/record/README.md
- test/args.test.js
- packages/record/src/decode.ts
- packages/record/src/cli.ts
- packages/record/src/proxy.ts
- packages/record/src/index.ts
Implements SPEC §7: capture the query shapes a test suite actually issues, as a
corpus. First commit is empty; the work lands on top.
Shape of the change
packages/record— a new npm workspace publishing@indexwright/record.indexwright recordin a lint-only installation says where the verb lives,rather than reporting an unknown command (SPEC §3).
One SPEC premise did not survive the spike
§3 justifies the package split with a gRPC stack:
@grpc/grpc-jsplus protobufdefinitions, on the grounds that hand-writing a decoder for someone else's wire
format is not worth the cost. Capture needs neither.
@grpc/grpc-jsis a client/server library for services known at build time; atransparent proxy is
node:http2and raw frames. And §7 already requires aclosed operator vocabulary with unknown enum values counted as
unsupported-shape, so the enum table has to exist in-tree whichever decoderreads the bytes. What is left is varint and length-delimited field parsing.
The split still holds, on a different premise: v0.3's
checkreplays a corpusand will need
@google-cloud/firestore. §3 is rewritten to rest on that.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
@indexwright/recordpackage andindexwright-recordCLI for capturing Firestore query shapes.Documentation
Chores