feat: circuits distribution#1243
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds Noir circuit build and artifact management: TypeScript CLI to compile/package circuits, a git-backed push/pull script, npm scripts and docs, root Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CLI as NoirCircuitBuilder (scripts/build-circuits.ts)
participant ArtifRepo as Git Remote (circuit-artifacts branch)
participant GHA as GitHub Actions (download-circuits)
participant Release as create-github-release
Dev->>CLI: npm run build:circuits (build artifacts, compute source hash)
CLI->>CLI: compile circuits, generate checksums & manifest
Dev->>ArtifRepo: npm run store:circuits push (commit dist + SOURCE_HASH)
ArtifRepo->>ArtifRepo: store artifacts on circuits branch
Note over GHA,ArtifRepo: On release workflow
GHA->>ArtifRepo: fetch/arcive origin/circuit-artifacts
ArtifRepo-->>GHA: archive + SOURCE_HASH (or not found)
GHA->>Release: set outputs found / source_hash
Release->>Release: include noir-circuits asset and Source hash in final release notes if found
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/release-test.yml:
- Around line 84-91: The workflow is reading github.event.inputs.version which
is undefined for push triggers; replace uses of github.event.inputs.version with
the pre-defined env.VERSION (or assign VERSION=${{ env.VERSION }}) wherever
VERSION is set or consumed (the shell assignment VERSION=..., the release notes
generation, and the release action fields tag_name and name) so the script and
the release action use env.VERSION consistently instead of
github.event.inputs.version.
In `@scripts/circuit-artifacts.ts`:
- Around line 55-68: The pull() function should (1) quote the DIST variable in
the git archive/tar shell invocation used by runV to safely handle paths with
spaces (reference DIST and runV in pull()), and (2) stop assuming every git
fetch failure means the branch is missing—capture the thrown error in the catch
block (reference run and BRANCH in pull()), log or include the actual error
message and exit with a generic "failed to fetch" message, or inspect the error
text to only treat explicit "branch not found" as that case (mirror the more
robust handling used in push()).
- Around line 44-50: The catch around the git operations (calls to run and runV
that perform `git commit`/`git push`) incorrectly conflates all failures as "No
changes"; update the catch in the try block that wraps run(`git commit...`) and
runV(...) to inspect the thrown error (or its message/exit code) and only treat
it as "no changes" when it clearly indicates nothing to commit (e.g., contains
"nothing to commit" / "no changes added to commit" or equivalent); for any other
error (git config/permissions/network), log the full error (including
error.message) with context (hash, BRANCH) and rethrow or exit non-zero so real
failures aren't masked. Ensure references to run, runV, hash, BRANCH and tmp are
preserved when adding the more specific handling and logging.
🧹 Nitpick comments (3)
.gitignore (1)
10-10: Trailing whitespace ondistentry.There's a trailing space after
distwhich, while harmless for gitignore functionality, is inconsistent with the other entries.Suggested fix
-dist +distscripts/circuit-artifacts.ts (1)
16-16: Functions are markedasyncbut don't useawait.Both
push()andpull()are declared asasyncbut use synchronousexecSynccalls exclusively. The CLI dispatch doesn't await them either. This works because the functions are actually synchronous, but it's misleading.Suggested fix: Remove async keywords
-async function push() { +function push() {-async function pull() { +function pull() {Also applies to: 55-55, 70-73
scripts/build-circuits.ts (1)
342-342: No validation for--envargument values.The
--envargument splits by comma and casts directly toEnvironment[]without validating that the values are actually valid environments (insecureorproduction). Invalid values like--env foo,barwould be silently accepted, resulting in no circuits being discovered.Suggested fix: Validate environment values
- else if (arg === '--env') options.environments = args[++i]?.split(',') as Environment[] + else if (arg === '--env') { + const envs = args[++i]?.split(',') + const valid = Object.values(ENVIRONMENTS) as string[] + const invalid = envs?.filter(e => !valid.includes(e)) + if (invalid?.length) { + console.error(`❌ Invalid environment(s): ${invalid.join(', ')}. Valid: ${valid.join(', ')}`) + process.exit(1) + } + options.environments = envs as Environment[] + }
ctrlc03
force-pushed
the
feat/circuits-distribution
branch
from
6a9bd64 to
f907ae9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.github/workflows/releases.yml:
- Around line 333-345: The Pull circuit artifacts step sets found=true
prematurely; update the step (the "Pull circuit artifacts" run block) to
validate the extracted dist/circuits layout and SOURCE_HASH before echoing
"found=true": after git archive extracts into dist/circuits, check for required
files/directories (e.g., expected circuit files or index manifest produced by
scripts/build-circuits.ts), compute or read the artifact hash from the extracted
artifacts and compare it to SOURCE_HASH, and only echo "found=true" to
GITHUB_OUTPUT if layout and hash match; otherwise echo "found=false" (and
optionally fail with a clear error) so downstream steps don't run on
malformed/mismatched artifacts.
In `@scripts/build-circuits.ts`:
- Around line 336-338: The --verbose flag is parsed into options.verbose but
never used; either remove its parsing/documentation or wire it to actual verbose
behavior. Locate the options object and where CLI flags are handled (references:
options.verbose and the argument-parsing block in build-circuits.ts) and
implement one of two fixes: 1) Remove the '-v'/'--verbose' branch and any
user-facing docs mentioning verbose, or 2) Use options.verbose to enable more
detailed logging (e.g., pass it into logging/helpers or conditionally increase
log level in functions that call build circuits or checksum routines) so the
flag has an effect across the script.
- Around line 305-321: The manifest currently builds artifact paths using
directory/name assumptions in generateManifest; instead, look up each circuit's
real artifact names from the discovered circuit metadata (e.g., use properties
on each circuit object such as packageName or artifacts produced by
discoverCircuits()) and construct artifacts.json as
`${environment}/${packageName}/${packageName}.json`; also respect the skip-vk
flag by omitting the vk entry when this.options.skipVk (or the equivalent
option) is true, otherwise include vk as
`${environment}/${packageName}/${packageName}.vk`; update generateManifest to
map circuits to use those real names and conditional vk inclusion.
- Around line 272-290: computeSourceHash/hashDir currently only updates the hash
with file names and contents, so renaming directories can leave the hash
unchanged; update hashDir to include directory names (or the file's relative
path) when iterating entries—e.g., before recursing into a directory or hashing
a file, call hash.update with the entry's relative path (or the joined relative
path from the circuit root) so directory structure is incorporated into the
final hash returned by computeSourceHash.
- Around line 143-188: The build currently treats a missing ${packageName}.json
as a warning; change buildCircuit so it fails loudly instead of returning an
incomplete CompiledCircuit: when jsonFile or targetDir is not found after
checking targetDirs, throw an Error (include circuit.environment and
circuit.name in the message) rather than logging and returning result; keep the
existing successful-path behavior (setting result.artifacts.json, checksums,
generating VK via generateVk when !this.options.skipVk) and ensure callers of
buildCircuit will observe the thrown error so CI fails.
- Around line 190-204: In generateVk, don't swallow errors silently: change the
empty catch to catch (err) and log the failure (including jsonFile and
packageName) and the error details (e.g., console.error(`Failed to generate VK
for ${jsonFile} (${packageName}):`, err)); optionally rethrow if you want the
build to fail instead of returning null; ensure the catch references the
generateVk function so you can find and update it.
In `@scripts/circuit-artifacts.ts`:
- Around line 40-41: The cpSync call is copying the DIST directory itself into
tmp (creating tmp/circuits/...), causing nested paths; change the copy to copy
DIST's contents into tmp instead. Locate the cpSync(DIST, tmp, { recursive: true
}) invocation and replace it so the copy operation targets the contents of DIST
(e.g., copy join(DIST, '.') or iterate DIST's entries and cpSync each into tmp)
so that files land directly under tmp (preserving the expected dist/circuits/...
layout) and keep the subsequent writeFileSync(join(tmp, 'SOURCE_HASH'), hash)
unchanged.
ctrlc03
force-pushed
the
feat/circuits-distribution
branch
from
18aaf7f to
55eedc1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 9-10: The .gitignore entry "dist␠" has a trailing space so it
won't match the directory; remove the trailing space from the "dist" line (or
replace it with "dist/") to ensure the build output directory is actually
ignored and verify there is no trailing whitespace on that line.
🧹 Nitpick comments (4)
scripts/README.md (2)
195-196: Documentation shows only two groups but code defines three.The
--groupoption documentation lists onlydkg,threshold, butbuild-circuits.tsalso definesAGGREGATION: 'recursive_aggregation'as a valid group. Consider updating the documentation to include all available groups.Options: - --group <groups> Circuit groups (comma-separated: dkg,threshold) + --group <groups> Circuit groups (comma-separated: dkg,threshold,recursive_aggregation)
233-238: Inconsistent command usage between sections.Line 237 uses
pnpm tsx scripts/circuit-artifacts.ts pushdirectly, while lines 218-219 use the npm script aliaspnpm store:circuits push. For consistency and maintainability, prefer the npm script alias throughout.1. **Local**: Build circuits and push to branch ```bash pnpm build:circuits - pnpm tsx scripts/circuit-artifacts.ts push + pnpm store:circuits push</blockquote></details> <details> <summary>scripts/build-circuits.ts (2)</summary><blockquote> `329-331`: **CLI argument parsing can access undefined array index.** When `--group`, `--circuit`, or `-o`/`--output` is the last argument without a value, `args[++i]` returns `undefined`. This silently sets invalid options rather than erroring. <details> <summary>🛡️ Proposed defensive fix</summary> ```diff - else if (arg === '--group') options.groups = args[++i]?.split(',') as CircuitGroup[] - else if (arg === '--circuit') (options.circuits ??= []).push(args[++i]) - else if (arg === '-o' || arg === '--output') options.outputDir = resolve(args[++i]) + else if (arg === '--group') { + const val = args[++i] + if (!val) { console.error('--group requires a value'); process.exit(1) } + options.groups = val.split(',') as CircuitGroup[] + } + else if (arg === '--circuit') { + const val = args[++i] + if (!val) { console.error('--circuit requires a value'); process.exit(1) } + (options.circuits ??= []).push(val) + } + else if (arg === '-o' || arg === '--output') { + const val = args[++i] + if (!val) { console.error('--output requires a value'); process.exit(1) } + options.outputDir = resolve(val) + }
222-232: Path sanitization regex may not cover all path separators consistently.The regex uses
[/\\]for separators but the replacement uses hardcoded forward slashes. On Windows, mixed separators in the output could occur. Also, silently ignoring errors here could hide issues during debugging.Consider at minimum logging a warning when sanitization fails:
} catch { - // Ignore errors + // Sanitization is best-effort; file may be read-only or malformed }
hmzakhalid
approved these changes
Feb 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #1235
Summary by CodeRabbit
New Features
Documentation
Chores