Add schema compiler fuzz engine - #379
Conversation
📝 WalkthroughWalkthroughThe PR adds a private fuzz workspace for deterministic Sury schema compiler testing. It defines typed cases, generates schemas, executes compiler operations, shrinks failures, writes replay artifacts, reports coverage, and exposes campaign and replay commands. ChangesSury fuzzing workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FuzzCLI
participant generateCases
participant runCase
participant SuryCompiler
participant FailureArtifacts
FuzzCLI->>generateCases: Generate seeded compiler cases
generateCases->>runCase: Submit CompilerCase
runCase->>SuryCompiler: Compile schemas and create operation
SuryCompiler-->>runCase: Return operation result
runCase-->>FuzzCLI: Return CaseResult and coverage
FuzzCLI->>FailureArtifacts: Persist minimized failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Spec performance
No significant changes. 1535 unchanged · 40 constant-schema targets skipped · 5 async examples skipped · advisory only |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
packages/fuzz/random.ts (1)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard on length instead of the sampled value.
pickthrows when the selected element isundefined. A genericreadonly T[]can holdundefinedas a valid value, so the error message would then be wrong. Check the array length instead.♻️ Proposed refactor
pick<T>(values: readonly T[]): T { - const value = values[Math.floor(this.next() * values.length)]; - if (value === undefined) throw new Error("Cannot pick from an empty list"); - return value; + if (values.length === 0) throw new Error("Cannot pick from an empty list"); + return values[Math.floor(this.next() * values.length)]!; }🤖 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/fuzz/random.ts` around lines 26 - 30, Update Random.pick to validate values.length before sampling, throwing the existing empty-list error only when the array is empty. Preserve undefined as a valid sampled element and return the selected value unchanged for non-empty arrays.packages/fuzz/schema.ts (1)
311-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
primitiveWitnessreadsNO_WITNESSbefore its declaration.
NO_WITNESSis declared at Line 357, afterprimitiveWitnessat Lines 311-355. The reference is safe today becauseprimitiveWitnessruns only after module evaluation. A future top-level call, or a module-level baseline table, would hit the temporal dead zone. Move theNO_WITNESSdeclaration aboveprimitiveWitness.🤖 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/fuzz/schema.ts` around lines 311 - 357, Move the exported NO_WITNESS declaration above primitiveWitness in the module so every switch branch, including the "never" case, references an initialized binding. Keep primitiveWitness behavior unchanged.packages/fuzz/engine_test.ts (1)
1-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
node:testfor isolation and reporting.The file runs assertions at module top level. The first failure stops all later checks, and the output gives no per-check names.
node:testprovides isolated named cases and a standard reporter. This is optional for a private fuzz workspace.🤖 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/fuzz/engine_test.ts` around lines 1 - 49, Optionally refactor the top-level assertions in engine_test.ts into named node:test cases so each deterministic-generation, mode-coverage, and custom-codec check runs independently with clear reporting. Preserve the existing assertions and test expectations while registering the cases through node:test’s standard test API.packages/fuzz/generate.ts (1)
541-544: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the ternary with identical branches.
Both branches call
random.int(1, 3). The condition has no effect on the value. It still consumes no extra randomness, so removing it does not change generated corpora. If parsers were meant to use a different range, set that range here.♻️ Proposed refactor
- const schemaCount = - operation === "parser" || operation === "asyncParser" - ? random.int(1, 3) - : random.int(1, 3); + const schemaCount = random.int(1, 3);🤖 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/fuzz/generate.ts` around lines 541 - 544, In the schemaCount initialization, remove the redundant operation ternary and assign random.int(1, 3) directly. Preserve the current generated range and behavior for all operation values.packages/fuzz/cli.ts (3)
185-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrint the full error, not only the message.
The top-level handler prints
error.messageand discards the stack. If the CLI itself fails, for example duringloadSuryorreadReplay, the stack identifies the origin. A fuzzing tool needs that detail.♻️ Proposed refactor
main().catch((error: unknown) => { - console.error(error instanceof Error ? error.message : error); + console.error(error); process.exitCode = 2; });🤖 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/fuzz/cli.ts` around lines 185 - 188, Update the top-level main() rejection handler to print the complete Error object, preserving its stack, instead of selecting only error.message; retain the existing fallback for non-Error values and process.exitCode behavior.
82-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck
artifactVersionagainst the supported value.
readReplaytreats any object that has anartifactVersionkey as aFailureArtifact. It never compares the value to1.validateCasestill guards theminimizedcase version, so a mismatch is caught indirectly, but a direct check gives a clearer error for a future artifact format.♻️ Proposed refactor
if (value && typeof value === "object" && "artifactVersion" in value) { const artifact = value as FailureArtifact; + if (artifact.artifactVersion !== 1) { + throw new Error(`Unsupported artifact version ${String(artifact.artifactVersion)}`); + } return {🤖 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/fuzz/cli.ts` around lines 82 - 88, Update the FailureArtifact detection in readReplay to require artifact.artifactVersion to equal the supported version 1, rather than only checking for the artifactVersion key. Keep the existing validateCase and expectedSignature handling unchanged for supported artifacts.
162-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish the replay exit codes.
replayreturns2for bothCHANGEDandNOT REPRODUCED. A caller cannot tell "a different bug appeared" from "the bug is fixed" without parsing stdout. Use distinct codes and document them inpackages/fuzz/README.md.♻️ Proposed refactor
console.log(`${matches ? "REPRODUCED" : "CHANGED"}: ${result.failure.signature}`); console.log(result.failure.message); - return matches ? 1 : 2; + return matches ? 1 : 3; } console.log(`NOT REPRODUCED: ${result.status}`); return 2;🤖 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/fuzz/cli.ts` around lines 162 - 171, Update the replay result handling around the `result.status === "bug"` branch to return a distinct exit code for `CHANGED` versus the `NOT REPRODUCED` path, while preserving the existing reproduced code and status behavior. Document the resulting replay exit-code meanings in the fuzz README so callers can distinguish a changed bug from a fixed bug without parsing output.packages/fuzz/README.md (1)
17-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
test:fuzzscript.The root
package.jsonaddstest:fuzz, which runs the engine self-tests. The README describes onlypnpm fuzz. A contributor who changes the generator needs to know how to run the self-tests.♻️ Proposed addition
```sh pnpm fuzz
+To run the engine self-tests:
+
+sh +pnpm test:fuzz +</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/fuzz/README.mdaround lines 17 - 25, Update the Run section of the
fuzz README to document the root package.json test:fuzz script, adding a concise
command example and identifying it as the engine self-test command while
preserving the existing pnpm fuzz instructions.</details> <!-- cr-comment:v1:fa1e0f04fbd995441122be23 --> </blockquote></details> <details> <summary>packages/fuzz/engine.ts (2)</summary><blockquote> `135-149`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Record `cacheHit` in coverage, or drop the second compilation.** `runCase` compiles each operation twice to detect the operation cache. The resulting `cacheHit` value is returned but never used: `check` in `packages/fuzz/cli.ts` (lines 103-106) only counts the `compiled` status. The extra compilation therefore doubles compile work for 8,000 cases without producing a signal. Add a coverage counter so the check is observable in the report. <details> <summary>♻️ Proposed refactor</summary> ```diff cacheHit = operation === second; + coverage.hit(coverage.outcomes, cacheHit ? "cache:hit" : "cache:miss"); } catch (error) {🤖 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/fuzz/engine.ts` around lines 135 - 149, Record the cacheHit result in coverage within runCase after the two operationFactory calls, using distinct coverage outcomes for cache hits and misses so the value appears in reports. Keep returning cacheHit unchanged and preserve the existing compile error handling.
79-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated timeout error type.
withTimeoutsignals a timeout with the messageTimed out after ${timeoutMs}ms. Line 206 detects the timeout by matching that message prefix. A schema under test could throw an error with the same message and be misclassified as a timeout. A dedicated error class removes the string coupling.♻️ Proposed refactor
+class FuzzTimeoutError extends Error {} + const withTimeout = async <T>(promise: Promise<T>, timeoutMs: number): Promise<T> => { let timer: ReturnType<typeof setTimeout> | undefined; const timeout = new Promise<never>((_, reject) => { - timer = setTimeout(() => reject(new Error(`Timed out after ${timeoutMs}ms`)), timeoutMs); + timer = setTimeout( + () => reject(new FuzzTimeoutError(`Timed out after ${timeoutMs}ms`)), + timeoutMs, + ); });Then replace the message check at line 206 with
error instanceof FuzzTimeoutError.🤖 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/fuzz/engine.ts` around lines 79 - 89, Define a dedicated FuzzTimeoutError class and have withTimeout reject with that error when the timer expires, preserving the timeout duration in its message if needed. Update the timeout handling around the line-206 error check to use error instanceof FuzzTimeoutError instead of matching the error message prefix, while leaving non-timeout errors unchanged.packages/fuzz/package.json (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
workspace:*for the localsurydependency.This workspace already uses pnpm workspaces and
workspace:sury-ppx, sosuryshould link the local package viaworkspace:*;packages/fuzz/package.json:13still usesfile:../sury.♻️ Proposed refactor
"dependencies": { - "sury": "file:../sury" + "sury": "workspace:*" },🤖 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/fuzz/package.json` around lines 12 - 14, Update the sury dependency in packages/fuzz/package.json from the file-based ../sury reference to the pnpm workspace protocol workspace:*, preserving the dependency name and all other package configuration.
🤖 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 `@package.json`:
- Around line 8-9: Update the inner fuzz package script invoked by the root
“fuzz” script to insert an explicit argument separator before the tsx CLI
invocation, preserving the build step and ensuring replay arguments reach cli.ts
without being parsed as pnpm options.
In `@packages/fuzz/engine.ts`:
- Around line 218-229: Update schemaShrinks so the unknown primitive does not
return a candidate identical to its input; preserve the existing simplest
candidates for other primitive schemas and shrink behavior for all remaining
schema kinds. Alternatively, apply identity filtering in caseShrinks so
unchanged candidates are excluded across the full shrinking pipeline.
- Around line 159-165: Update the source validation around
Function.prototype.toString in the callable handling flow to skip the new
Function syntax check when the source represents a native or bound function,
including the “[native code]” form. Preserve the existing coverage.hit and
source failure behavior for genuinely invalid non-native function sources.
- Around line 357-361: Update the filename construction around the artifact path
so the failure signature is sanitized with the same character-replacement logic
as artifact.minimized.id before being passed to writeFileSync. Apply this to the
full artifact.failure.signature, preserving the existing safeId behavior and
output naming structure while preventing path separators or unsafe segments from
affecting the target path.
In `@packages/fuzz/schema.ts`:
- Around line 441-452: Update the refine rendering logic in renderSchema to
apply the same argument defaults as compileSchema: use 1 when rendering
min-length or max-length refinements, and 0 for gte, lte, gt, lt, and length
when ast.argument is missing. Preserve explicit arguments and the existing
rendering for empty, nonEmpty, pattern, and custom refinements.
---
Nitpick comments:
In `@packages/fuzz/cli.ts`:
- Around line 185-188: Update the top-level main() rejection handler to print
the complete Error object, preserving its stack, instead of selecting only
error.message; retain the existing fallback for non-Error values and
process.exitCode behavior.
- Around line 82-88: Update the FailureArtifact detection in readReplay to
require artifact.artifactVersion to equal the supported version 1, rather than
only checking for the artifactVersion key. Keep the existing validateCase and
expectedSignature handling unchanged for supported artifacts.
- Around line 162-171: Update the replay result handling around the
`result.status === "bug"` branch to return a distinct exit code for `CHANGED`
versus the `NOT REPRODUCED` path, while preserving the existing reproduced code
and status behavior. Document the resulting replay exit-code meanings in the
fuzz README so callers can distinguish a changed bug from a fixed bug without
parsing output.
In `@packages/fuzz/engine_test.ts`:
- Around line 1-49: Optionally refactor the top-level assertions in
engine_test.ts into named node:test cases so each deterministic-generation,
mode-coverage, and custom-codec check runs independently with clear reporting.
Preserve the existing assertions and test expectations while registering the
cases through node:test’s standard test API.
In `@packages/fuzz/engine.ts`:
- Around line 135-149: Record the cacheHit result in coverage within runCase
after the two operationFactory calls, using distinct coverage outcomes for cache
hits and misses so the value appears in reports. Keep returning cacheHit
unchanged and preserve the existing compile error handling.
- Around line 79-89: Define a dedicated FuzzTimeoutError class and have
withTimeout reject with that error when the timer expires, preserving the
timeout duration in its message if needed. Update the timeout handling around
the line-206 error check to use error instanceof FuzzTimeoutError instead of
matching the error message prefix, while leaving non-timeout errors unchanged.
In `@packages/fuzz/generate.ts`:
- Around line 541-544: In the schemaCount initialization, remove the redundant
operation ternary and assign random.int(1, 3) directly. Preserve the current
generated range and behavior for all operation values.
In `@packages/fuzz/package.json`:
- Around line 12-14: Update the sury dependency in packages/fuzz/package.json
from the file-based ../sury reference to the pnpm workspace protocol
workspace:*, preserving the dependency name and all other package configuration.
In `@packages/fuzz/random.ts`:
- Around line 26-30: Update Random.pick to validate values.length before
sampling, throwing the existing empty-list error only when the array is empty.
Preserve undefined as a valid sampled element and return the selected value
unchanged for non-empty arrays.
In `@packages/fuzz/README.md`:
- Around line 17-25: Update the Run section of the fuzz README to document the
root package.json test:fuzz script, adding a concise command example and
identifying it as the engine self-test command while preserving the existing
pnpm fuzz instructions.
In `@packages/fuzz/schema.ts`:
- Around line 311-357: Move the exported NO_WITNESS declaration above
primitiveWitness in the module so every switch branch, including the "never"
case, references an initialized binding. Keep primitiveWitness behavior
unchanged.
🪄 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: c33927d8-34c5-45a1-98d4-729c7333cef3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
package.jsonpackages/fuzz/.gitignorepackages/fuzz/README.mdpackages/fuzz/catalog.tspackages/fuzz/cli.tspackages/fuzz/engine.tspackages/fuzz/engine_test.tspackages/fuzz/generate.tspackages/fuzz/package.jsonpackages/fuzz/random.tspackages/fuzz/schema.tspackages/fuzz/tsconfig.jsonpackages/fuzz/types.ts
| "fuzz": "pnpm --filter=fuzz fuzz", | ||
| "test:fuzz": "pnpm --filter=fuzz test", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
pnpm run forward arguments nested filter script with && chained commands
💡 Result:
To forward arguments to a script in pnpm when using chained commands or filters, you must use the double-dash (--) separator [1][2][3]. This separator tells pnpm that any arguments following it should be passed directly to the underlying script or command, rather than being interpreted by pnpm itself [1][3][4]. When dealing with chained commands (e.g., using &&) or filters, it is important to place the -- correctly to ensure the arguments reach the intended subprocess [5][3]. 1. Basic Argument Forwarding To pass arguments to a single script: pnpm run -- Example: pnpm run build -- --minify [5] 2. Chained Commands (&&) When chaining commands with &&, the -- applies only to the specific command immediately preceding it. If you need to pass arguments to multiple scripts in a chain, you must include a -- for each command that requires arguments [5][6]. Example: pnpm run script-a -- arg1 && pnpm run script-b -- arg2 3. Using Filters When using --filter to run a script in specific workspace packages, pnpm treats the arguments differently depending on their position [5][7]. Arguments for the script should be placed after the script name, separated by -- [1][2]. Example: pnpm --filter run -- Troubleshooting Note: If you are passing arguments that might collide with pnpm's own flags (like --filter or --recursive), the -- separator is essential to distinguish between pnpm options and script-level arguments [5][3][8]. If you find that arguments are being dropped or not received by the script, ensure you are using the -- correctly as shown above [3]. If you are chaining pnpm commands, ensure each pnpm command in the chain is properly scoped with its own set of arguments and separators if necessary [6].
Citations:
- 1: https://pnpm.io/cli/run
- 2: https://pnpm.io/10.x/cli/run
- 3: pacquet:
pnpm run <script> -- <args>drops the--separator, so script arguments hit the underlying program pnpm/pnpm#13295 - 4: https://stackoverflow.com/questions/74102152/how-to-pass-arbitrary-argument-with-pnpm
- 5: Run scripts without
--? pnpm/pnpm#3778 - 6: feat: improved exec/filter command chaining pnpm/pnpm#5543
- 7: https://pnpm.io/filtering
- 8: Filtering via -- for the test/run/exec commands pnpm/pnpm#1646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json relevant lines =="
cat -n package.json | sed -n '1,80p'
echo
echo "== locate fuzz package files =="
git ls-files | rg '(^|/)packages/fuzz/(README\.md|package\.json$|cli\.ts$)$|-pkg/fuzz/' || true
echo
echo "== fuzz package package.json =="
packJson="$(git ls-files | rg '(^|/)packages/fuzz/package\.json$' | head -n1)"
if [ -n "${packJson:-}" ]; then
cat -n "$packJson" | sed -n '1,120p'
fi
echo
echo "== fuzz README around replay docs =="
readme="$(git ls-files | rg '(^|/)packages/fuzz/README\.md$' | head -n1)"
if [ -n "${readme:-}" ]; then
cat -n "$readme" | sed -in '25,55p'
else
echo "no packages/fuzz/README.md"
fi
echo
echo "== cli parseCommand definitions/usages =="
cli="$(git ls-files | rg '(^|/)packages/fuzz/cli\.ts$' | head -n1)"
if [ -n "${cli:-}" ]; then
rg -n "parseCommand|program|command|replay|replay <artifact>|replay" "$cli"
fi
echo
echo "== package scripts mentioning fuzz/test/fuzz =="
rg -n '"(fuzz|test:fuzz|test|build:entry)"|fuzz|replay' package.json packages/fuzz || true
echo
echo "== lockfile/engines for pnpm pin =="
if [ -f package.json ]; then
python3 - <<'PY'
import json
p=json.load(open('package.json'))
print("engines:", p.get("engines"))
print("pnpm:", p.get("packageManager") or p.get("pnpm"))
PY
fiRepository: DZakh/sury
Length of output: 1891
🏁 Script executed:
#!/bin/bash
set -u
echo "== packages/fuzz/README.md =="
cat -n packages/fuzz/README.md | sed -n '1,90p'
echo
echo "== packages/fuzz/cli.ts outline/relevant sections =="
wc -l packages/fuzz/cli.ts
cat -n packages/fuzz/cli.ts | sed -n '1,220p'
echo
echo "== lock/pnpm files =="
git ls-files | rg '(^|/)(pnpm-lock\.yaml|pnpm-workspace\.yaml)$' || trueRepository: DZakh/sury
Length of output: 10585
🌐 Web query:
pnpm@9.0.5 --filter script arguments chained commands && pnpm run
💡 Result:
In pnpm, you can chain multiple commands by separating them with shell operators like && or; [1]. When using the --filter flag to target specific packages within a workspace, each segment of your chained command must be scoped appropriately with its own --filter flag [1]. For example, to run a build script in one package and a test script in another, you would structure the command as follows: pnpm --filter packageA build && pnpm --filter packageB test Key points regarding this syntax: 1. Scope: Each pnpm command is independent. When you prepend --filter to a pnpm command, it limits the execution of that specific command to the selected packages [2][3]. The subsequent command in your chain is a fresh execution of pnpm, so it requires its own --filter to target the correct package [1]. 2. Argument Passing: If you need to pass additional arguments to the underlying script being executed (e.g., passing --watch to a build script), place them after the script name [4][5]. If the script command itself uses flags that might conflict with pnpm's own flags, use -- to separate pnpm arguments from script arguments [4][5]. 3. Shell Behavior: Because these are standard shell-level chains (using && or;), the shell handles the sequence [1]. The pnpm-specific filtering happens entirely at the CLI layer for each individual command call [3]. If you find that you are frequently chaining complex commands, it is often cleaner to define a script in your root package.json or use a dedicated task runner like Turborepo, which is designed to handle dependency-aware command orchestration and chaining more natively [1].
Citations:
- 1: feat: improved exec/filter command chaining pnpm/pnpm#5543
- 2: https://pnpm.io/filtering
- 3: https://www.javascript-package-publishing.com/monorepo-architecture-orchestration/pnpm-workspace-filtering/
- 4: https://pnpm.io/cli/run
- 5: https://pnpm.io/10.x/cli/run
🌐 Web query:
pnpm documentation run -- filter filterArgs --filter argument is passed to command
💡 Result:
In pnpm, the --filter option is a command-level argument that must be placed before the command (e.g., run, test, build) to influence pnpm's behavior, such as selecting specific workspace packages [1][2][3]. Arguments intended for the script itself must be placed after the script name [1][4]. The distinction is as follows: 1. Parameters for pnpm: These must be placed before the command (e.g., pnpm --filter run ) [2][3]. 2. Parameters for the script: These must be placed after the script name (e.g., pnpm run -- ) [1][4]. As of recent versions, pnpm explicitly parses and forwards all arguments placed after the script name verbatim to the executed script [5]. Conversely, if you place flags like --filter after the script name, they will be treated as arguments to the script rather than pnpm configuration [1][4]. To ensure --filter is correctly parsed by pnpm, it should never be placed after the script name [3].
Citations:
- 1: https://pnpm.io/cli/run
- 2: https://pnpm.io/filtering
- 3: https://modernjs.dev/guides/troubleshooting/cli
- 4: https://pnpm.io/10.x/cli/run
- 5: fix(run): forward every argument after the script name verbatim pnpm/pnpm#13300
🌐 Web query:
pnpm 9.0.5 source run command process.argv filterArg appendScriptArgs
💡 Result:
In pnpm 9.0.5, the command line argument handling for pnpm run is managed during the initial parsing phase to ensure that arguments intended for the user's script are passed through correctly while pnpm's own flags are consumed [1][2]. Specifically, pnpm uses the nopt library to parse command-line arguments [1][2]. For the run command, which is designated as a specially escaped command (SPECIALLY_ESCAPED_CMDS), pnpm needs to stop parsing its own flags once it reaches the script name [1][2]. Key mechanisms in the source code (within cli/parse-cli-args/src/index.ts) include: 1. getEscapeArgsWithSpecialCases: This function determines the "escape" point for argument parsing [2]. For run, it calculates the index of the script name within process.argv (taking into account whether recursive commands or fallback command patterns are used) and instructs the parser to treat that position as the boundary where pnpm-specific flags end [2]. Everything following this boundary is treated as arguments to be forwarded to the script [3][2]. 2. Argument Forwarding: Once the script name is identified, the parser separates the input arguments [1][2]. Arguments appearing after the script name are preserved in the remaining argument list (argv.remain), which are then passed to the execution layer to be appended to the script's command execution [4][1]. 3. Handling filterArg and other options: While filters (--filter) are processed as pnpm-level options before the script name, they are explicitly handled by the parser to ensure they influence pnpm's behavior (e.g., recursive execution) rather than being passed to the script [4][1]. Recent developments in the codebase, such as those in PR #13300 and related issues, have refined this process to ensure that all arguments after the script name—including those that might otherwise be misinterpreted as pnpm flags—are forwarded verbatim [3][5][6]. This prevents issues where user-provided arguments (like --watch or --if-present) are incorrectly consumed by pnpm's parser instead of being passed to the underlying program [3][6].
Citations:
- 1: https://github.com/pnpm/pnpm/blob/main/cli/parse-cli-args/src/index.ts
- 2: https://github.com/pnpm/pnpm/blob/a751c7f2/cli/parse-cli-args/src/index.ts
- 3: fix(run): forward every argument after the script name verbatim pnpm/pnpm#13300
- 4: https://pnpm.io/cli/run
- 5: pacquet:
pnpm run <script> -- <args>drops the--separator, so script arguments hit the underlying program pnpm/pnpm#13295 - 6: fix(cli): forward every argument of test, start, and stop to the script pnpm/pnpm#13344
🌐 Web query:
pnpm@9.0.5 cli run script string "--filter=sury build:entry && tsx ./cli.ts" arguments remaining
💡 Result:
The command you provided, pnpm --filter=sury build:entry && tsx./cli.ts arguments remaining, is a shell command that combines a pnpm workspace operation with a subsequent execution command [1][2]. Breakdown of the command: 1. pnpm --filter=sury build:entry: This part of the command uses pnpm to execute the build:entry script specifically in the package named (or matching) sury within your workspace [1][2]. The --filter flag restricts the scope of the command to the specified package [1]. 2. &&: This is a shell operator that ensures the second command (tsx./cli.ts) runs only if the first command (pnpm build:entry) completes successfully (exit code 0). 3. tsx./cli.ts arguments remaining: If the build succeeds, this executes your TypeScript file (cli.ts) using the tsx runner, passing arguments remaining as parameters to your script [3][4]. Important considerations: - Argument placement: If you intended to pass arguments to the pnpm script itself rather than the shell command, you would place them after the script name [3][5]. For example: pnpm --filter=sury build:entry -- --my-arg. - Shell context: Because the shell interprets the && operator, the entire sequence is treated as two distinct commands executed in order [1][2]. - Filter syntax: The --filter=sury syntax is valid for selecting a package by name [1][2]. If sury refers to a directory path, it is generally safer to use --filter=./path/to/sury [6][2].
Citations:
- 1: https://pnpm.io/filtering
- 2: https://www.javascript-package-publishing.com/monorepo-architecture-orchestration/pnpm-workspace-filtering/
- 3: https://pnpm.io/cli/run
- 4: https://pnpm.io/10.x/cli/run
- 5: https://pnpm.io/next/cli/run
- 6: https://github.com/pnpm/spec/blob/master/workspace/filtering.md
🌐 Web query:
GitHub pnpm 9.0.5 pnpm-run script command process.argv argv remain script arguments
💡 Result:
In pnpm, arguments passed to pnpm run <script> are forwarded to the underlying script command [1][2]. To ensure arguments are passed correctly—especially those that might be misinterpreted as pnpm flags—use the -- separator [1][2][3]. For example: pnpm run script-name -- --your-flag=value [4] When using the -- separator, pnpm treats everything following it as arguments for the script, ensuring they reach process.argv in the executed command untouched [1][2][5]. Key points regarding argument passing: 1. Handling of --: The -- separator is the standard way to prevent pnpm from attempting to parse subsequent arguments as its own CLI options [1][2]. Without it, if an argument looks like a pnpm flag, it may be consumed or cause errors [3][5]. 2. process.argv: In a Node.js script, process.argv contains the command line arguments. Because pnpm run executes the script command, your script will receive the arguments passed after the script name (or after the -- separator) starting from index 2 of process.argv (or index 1 if you are using node -e) [6][3][7]. 3. Known Issues and Improvements: While argument forwarding is a core feature, some versions have encountered bugs regarding the handling of specific character sequences (like newlines) or the dropping of the -- separator itself [3][7]. These are generally addressed in subsequent patches [5]. If you encounter unexpected behavior where arguments are dropped or misparsed, ensuring you are on a recent version of pnpm is recommended [5]. If you are trying to access arguments inside your package.json scripts, note that pnpm does not automatically provide $pnpm_config_name style variables for custom arguments as some other package managers might for their configuration options [4]. Instead, arguments passed to pnpm run are passed directly to the command defined in the script [1][2]. If you need to map these arguments to environment variables for use inside a script, a common workaround is to use a small wrapper script (e.g., in JavaScript) that parses process.argv and sets process.env before executing your primary command [4].
Citations:
- 1: https://pnpm.io/cli/run
- 2: https://pnpm.io/10.x/cli/run
- 3: pacquet:
pnpm run <script> -- <args>drops the--separator, so script arguments hit the underlying program pnpm/pnpm#13295 - 4: https://stackoverflow.com/questions/78961674/access-command-line-arguments-passed-to-pnpm-run-in-package-json
- 5: fix(cli): forward the argument separator to the script pnpm/pnpm#13303
- 6: pnpm run doesn't pass arguments pnpm/pnpm#1818
- 7: Multi-line arguments don't get forwarded correctly to scripts pnpm/pnpm#7641
🌐 Web query:
pnpm@9.0.5 npm_script_script_args argv forward command arguments
💡 Result:
In pnpm, arguments are forwarded to scripts by placing them after the script name [1][2]. Any options intended for the pnpm run command itself must be placed before the script name, while arguments intended for the executed script are placed after it [1][2]. Regarding the behavior of argv forwarding and environment variables: 1. Argument Forwarding: pnpm is designed to pass arguments occurring after the script name verbatim to the underlying command [3]. While previous versions occasionally encountered issues with flag parsing or the argument separator (--), recent updates have specifically ensured that the separator is correctly forwarded [4] and that arguments after the script name are not incorrectly consumed as pnpm options [3]. 2. npm_config_argv: Historically, pnpm populated the npm_config_argv environment variable to maintain compatibility with tools that relied on npm's behavior [5]. However, this variable is not a standard feature across all package managers and has been reduced or removed in newer ecosystems [5]. In modern pnpm versions, relying on npm_config_argv is discouraged, as pnpm primarily forwards arguments directly to the command line of the script being executed [6]. 3. Passing Arguments: If you need to pass arbitrary flags (e.g., --foo=bar) to your script, simply appending them after the script name (e.g., pnpm run -- --foo=bar) is the standard approach [6]. The use of the -- separator is recommended if your script-specific arguments might conflict with shell parsing or if you wish to ensure clear separation between pnpm arguments and your script's arguments [7][4]. For the most stable behavior in pnpm v9.x, avoid depending on the presence or structure of npm_config_argv, as it is considered a legacy implementation detail [5]. Instead, structure your scripts to accept command-line arguments directly [6].
Citations:
- 1: https://pnpm.io/cli/run
- 2: https://pnpm.io/next/cli/run
- 3: fix(run): forward every argument after the script name verbatim pnpm/pnpm#13300
- 4: fix(cli): forward the argument separator to the script pnpm/pnpm#13303
- 5: https://github.com/orgs/pnpm/discussions/4153
- 6: https://stackoverflow.com/questions/74102152/how-to-pass-arbitrary-argument-with-pnpm
- 7: pnpm run doesn't pass switches in args on to the script pnpm/pnpm#2134
Make replay argument forwarding explicit for the double-hop script.
The documented command pnpm fuzz -- replay packages/fuzz/artifacts/<artifact>.json works at the root because the outer -- reaches the inner fuzz script. The inner package script is pnpm --filter=sury build:entry && tsx ./cli.ts; add a second -- before the tsx invocation so replay arguments cannot be treated as pnpm options and dropped by the && chain.
🤖 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 `@package.json` around lines 8 - 9, Update the inner fuzz package script
invoked by the root “fuzz” script to insert an explicit argument separator
before the tsx CLI invocation, preserving the build step and ensuring replay
arguments reach cli.ts without being parsed as pnpm options.
| try { | ||
| const source = Function.prototype.toString.call(callable); | ||
| new Function(`return (${source})`); | ||
| } catch (error) { | ||
| coverage.hit(coverage.outcomes, "bug:source"); | ||
| return { status: "bug", failure: failure("source", error) }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip the syntax check for native or bound functions.
Function.prototype.toString returns function () { [native code] } for bound and native functions. new Function("return (function () { [native code] })") throws a SyntaxError. The case is then reported as bug:source, which is a false positive. If Sury ever returns a bound operation, the campaign fails without a real compiler defect.
Note on the static analysis hint for line 161: the input is the process's own generated function source, and the expression is a function definition that is never invoked, so this is not an injection sink here.
🐛 Proposed guard
try {
const source = Function.prototype.toString.call(callable);
- new Function(`return (${source})`);
+ if (!source.includes("[native code]")) new Function(`return (${source})`);
} catch (error) {📝 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.
| try { | |
| const source = Function.prototype.toString.call(callable); | |
| new Function(`return (${source})`); | |
| } catch (error) { | |
| coverage.hit(coverage.outcomes, "bug:source"); | |
| return { status: "bug", failure: failure("source", error) }; | |
| } | |
| try { | |
| const source = Function.prototype.toString.call(callable); | |
| if (!source.includes("[native code]")) new Function(`return (${source})`); | |
| } catch (error) { | |
| coverage.hit(coverage.outcomes, "bug:source"); | |
| return { status: "bug", failure: failure("source", error) }; | |
| } |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 161-161: new Function() with dynamic input can execute arbitrary code. Avoid dynamic code evaluation entirely, or use a safe alternative.
(coderabbit.code-injection.new-function-js)
🤖 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/fuzz/engine.ts` around lines 159 - 165, Update the source validation
around Function.prototype.toString in the callable handling flow to skip the new
Function syntax check when the source represents a native or bound function,
including the “[native code]” form. Preserve the existing coverage.hit and
source failure behavior for genuinely invalid non-native function sources.
Source: Linters/SAST tools
| const schemaShrinks = (schema: SchemaAst): SchemaAst[] => { | ||
| const simplest: SchemaAst[] = [ | ||
| { kind: "primitive", name: "string" }, | ||
| { kind: "primitive", name: "unknown" }, | ||
| ]; | ||
| switch (schema.kind) { | ||
| case "primitive": | ||
| return schema.name === "string" ? [] : simplest; | ||
| case "literal": | ||
| case "enum": | ||
| case "instance": | ||
| return simplest; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
schemaShrinks returns the input node for the unknown primitive.
simplest contains { kind: "primitive", name: "unknown" }. Line 225 returns simplest for every primitive except string. For an unknown primitive, the returned list therefore contains a node equal to the input.
shrinkFailure accepts that candidate when it reproduces the target signature, sets changed = true, and restarts the outer loop with an unchanged case. The shrink budget is then consumed by no-op passes, and the loop breaks before it reaches the other schemas in the pipeline. maxAttempts prevents a true infinite loop, but shrinking stops making progress.
🐛 Proposed fix
switch (schema.kind) {
case "primitive":
- return schema.name === "string" ? [] : simplest;
+ return simplest.filter((candidate) => renderSchema(candidate) !== renderSchema(schema));Alternatively, filter identical candidates once in caseShrinks, which also covers other nodes that can shrink to themselves.
🤖 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/fuzz/engine.ts` around lines 218 - 229, Update schemaShrinks so the
unknown primitive does not return a candidate identical to its input; preserve
the existing simplest candidates for other primitive schemas and shrink behavior
for all remaining schema kinds. Alternatively, apply identity filtering in
caseShrinks so unchanged candidates are excluded across the full shrinking
pipeline.
| mkdirSync(directory, { recursive: true }); | ||
| const safeId = artifact.minimized.id.replace(/[^a-zA-Z0-9_.-]+/g, "-"); | ||
| const path = join(directory, `${safeId}-${artifact.failure.signature.replace(":", "-")}.json`); | ||
| writeFileSync(path, `${JSON.stringify(artifact, null, 2)}\n`); | ||
| return path; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Sanitize the signature before it becomes part of the filename.
Line 358 sanitizes artifact.minimized.id, but line 359 inserts artifact.failure.signature into the filename after replacing only the first colon. The signature is ${phase}:${name}, and name comes from the name property of an arbitrary thrown error. A name that contains /, \, or a path segment changes the write target or makes writeFileSync fail with ENOENT, which loses the failure artifact. Apply the same sanitizer to both parts.
🐛 Proposed fix
+const safeSegment = (value: string): string => value.replace(/[^a-zA-Z0-9_.-]+/g, "-");
+
export const writeFailureArtifact = (
directory: string,
artifact: FailureArtifact,
): string => {
mkdirSync(directory, { recursive: true });
- const safeId = artifact.minimized.id.replace(/[^a-zA-Z0-9_.-]+/g, "-");
- const path = join(directory, `${safeId}-${artifact.failure.signature.replace(":", "-")}.json`);
+ const safeId = safeSegment(artifact.minimized.id);
+ const safeSignature = safeSegment(artifact.failure.signature);
+ const path = join(directory, `${safeId}-${safeSignature}.json`);
writeFileSync(path, `${JSON.stringify(artifact, null, 2)}\n`);📝 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.
| mkdirSync(directory, { recursive: true }); | |
| const safeId = artifact.minimized.id.replace(/[^a-zA-Z0-9_.-]+/g, "-"); | |
| const path = join(directory, `${safeId}-${artifact.failure.signature.replace(":", "-")}.json`); | |
| writeFileSync(path, `${JSON.stringify(artifact, null, 2)}\n`); | |
| return path; | |
| const safeSegment = (value: string): string => value.replace(/[^a-zA-Z0-9_.-]+/g, "-"); | |
| export const writeFailureArtifact = ( | |
| directory: string, | |
| artifact: FailureArtifact, | |
| ): string => { | |
| mkdirSync(directory, { recursive: true }); | |
| const safeId = safeSegment(artifact.minimized.id); | |
| const safeSignature = safeSegment(artifact.failure.signature); | |
| const path = join(directory, `${safeId}-${safeSignature}.json`); | |
| writeFileSync(path, `${JSON.stringify(artifact, null, 2)}\n`); | |
| return path; | |
| }; |
🤖 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/fuzz/engine.ts` around lines 357 - 361, Update the filename
construction around the artifact path so the failure signature is sanitized with
the same character-replacement logic as artifact.minimized.id before being
passed to writeFileSync. Apply this to the full artifact.failure.signature,
preserving the existing safeId behavior and output naming structure while
preventing path separators or unsafe segments from affecting the target path.
| case "refine": | ||
| return ast.refinement === "min-length" | ||
| ? `S.minLength(${renderSchema(ast.inner)}, ${ast.argument})` | ||
| : ast.refinement === "max-length" | ||
| ? `S.maxLength(${renderSchema(ast.inner)}, ${ast.argument})` | ||
| : ["gte", "lte", "gt", "lt", "length"].includes(ast.refinement) | ||
| ? `S.${ast.refinement}(${renderSchema(ast.inner)}, ${ast.argument})` | ||
| : ast.refinement === "empty" || ast.refinement === "nonEmpty" | ||
| ? `S.${ast.refinement}(${renderSchema(ast.inner)})` | ||
| : ast.refinement === "pattern" | ||
| ? `S.pattern(${renderSchema(ast.inner)}, /^fuzz/)` | ||
| : `S.refine(${renderSchema(ast.inner)}, <${ast.refinement}>)`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render the same argument defaults that compileSchema applies.
compileSchema substitutes defaults when ast.argument is missing: ?? 1 for minLength and maxLength (Lines 117 and 120) and ?? 0 for gte, lte, gt, lt, and length (Line 127). renderSchema interpolates ast.argument directly. If argument is missing, the rendered snippet contains undefined and does not reproduce the compiled schema. Failure artifacts and CLI output use this text.
🐛 Proposed fix
case "refine":
return ast.refinement === "min-length"
- ? `S.minLength(${renderSchema(ast.inner)}, ${ast.argument})`
+ ? `S.minLength(${renderSchema(ast.inner)}, ${ast.argument ?? 1})`
: ast.refinement === "max-length"
- ? `S.maxLength(${renderSchema(ast.inner)}, ${ast.argument})`
+ ? `S.maxLength(${renderSchema(ast.inner)}, ${ast.argument ?? 1})`
: ["gte", "lte", "gt", "lt", "length"].includes(ast.refinement)
- ? `S.${ast.refinement}(${renderSchema(ast.inner)}, ${ast.argument})`
+ ? `S.${ast.refinement}(${renderSchema(ast.inner)}, ${ast.argument ?? 0})`📝 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.
| case "refine": | |
| return ast.refinement === "min-length" | |
| ? `S.minLength(${renderSchema(ast.inner)}, ${ast.argument})` | |
| : ast.refinement === "max-length" | |
| ? `S.maxLength(${renderSchema(ast.inner)}, ${ast.argument})` | |
| : ["gte", "lte", "gt", "lt", "length"].includes(ast.refinement) | |
| ? `S.${ast.refinement}(${renderSchema(ast.inner)}, ${ast.argument})` | |
| : ast.refinement === "empty" || ast.refinement === "nonEmpty" | |
| ? `S.${ast.refinement}(${renderSchema(ast.inner)})` | |
| : ast.refinement === "pattern" | |
| ? `S.pattern(${renderSchema(ast.inner)}, /^fuzz/)` | |
| : `S.refine(${renderSchema(ast.inner)}, <${ast.refinement}>)`; | |
| case "refine": | |
| return ast.refinement === "min-length" | |
| ? `S.minLength(${renderSchema(ast.inner)}, ${ast.argument ?? 1})` | |
| : ast.refinement === "max-length" | |
| ? `S.maxLength(${renderSchema(ast.inner)}, ${ast.argument ?? 1})` | |
| : ["gte", "lte", "gt", "lt", "length"].includes(ast.refinement) | |
| ? `S.${ast.refinement}(${renderSchema(ast.inner)}, ${ast.argument ?? 0})` | |
| : ast.refinement === "empty" || ast.refinement === "nonEmpty" | |
| ? `S.${ast.refinement}(${renderSchema(ast.inner)})` | |
| : ast.refinement === "pattern" | |
| ? `S.pattern(${renderSchema(ast.inner)}, /^fuzz/)` | |
| : `S.refine(${renderSchema(ast.inner)}, <${ast.refinement}>)`; |
🤖 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/fuzz/schema.ts` around lines 441 - 452, Update the refine rendering
logic in renderSchema to apply the same argument defaults as compileSchema: use
1 when rendering min-length or max-length refinements, and 0 for gte, lte, gt,
lt, and length when ast.argument is missing. Preserve explicit arguments and the
existing rendering for empty, nonEmpty, pattern, and custom refinements.
1 participant
Summary by CodeRabbit
New Features
Documentation