refactor(tools/cli): execlude *.mock.compact contracts from builder#68
refactor(tools/cli): execlude *.mock.compact contracts from builder#68
*.mock.compact contracts from builder#68Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces file exclusion and dry-run capabilities to the CLI compiler. It adds glob pattern matching for excluding files, extends CLI options with Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI / CompactCompiler
participant Parser as Argument Parser
participant Discovery as FileDiscovery
participant UIService as UIService
participant Compiler as Compilation Engine
User->>CLI: Invoke with --exclude and --dry-run flags
CLI->>Parser: parseArgs(args)
Parser->>Parser: Extract exclude patterns and dryRun flag
CLI->>Discovery: new FileDiscovery(srcDir, exclude patterns)
Discovery->>Discovery: Filter .compact files via isExcluded()
alt Dry-run mode active
CLI->>UIService: showDryRun(filtered files, targetDir)
UIService->>User: Display preview of files to compile
else Dry-run mode inactive
CLI->>Compiler: compile()
Compiler->>Compiler: Process filtered files
Compiler->>User: Generate output
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/README.md (1)
183-185:⚠️ Potential issue | 🟡 MinorStale "Copy and clean" step description
Line 184 still reads "Copy and clean
.compactfiles todist/" but the cleanup (removal ofMock*.compact) was removed in this PR. The step now only copies.📝 Proposed fix
-4. Copy and clean `.compact` files to `dist/` +4. Copy `.compact` files to `dist/`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/README.md` around lines 183 - 185, Update the README step that currently reads "Copy and clean `.compact` files to `dist/`" to reflect the new behavior: change the wording to "Copy `.compact` files to `dist/`" (or similar phrasing) to remove mention of cleanup/removal of Mock*.compact, referencing the entry in the CLI README where the artifact copy steps are listed so the documentation matches the PR changes.packages/cli/src/Builder.ts (1)
116-125:⚠️ Potential issue | 🟠 Major
--dry-runflag is silently ignored for all build steps
compile()returns early whendryRunistrue, butbuild()unconditionally proceeds to run all three shell steps (tsc, artifact copy,.compactcopy). A user passing--dry-runtocompact-builderwill see the dry-run preview from the compiler, then immediately trigger a real TypeScript compilation and file copy — the opposite of what the flag promises.The README states the builder "Accepts all compiler options except
--skip-zk", which explicitly advertises--dry-runsupport.🐛 Proposed fix
public async build(): Promise<void> { const compiler = new CompactCompiler(this.options); await compiler.compile(); + if (this.options.dryRun) { + return; // dry-run: compiler already showed the preview; skip build steps + } + for (const [index, step] of this.steps.entries()) { await this.executeStep(step, index, this.steps.length); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/Builder.ts` around lines 116 - 125, The build method currently always runs executeStep over this.steps even when the CompactCompiler.compile() returned early for a dry run; update build to check the dry-run flag and skip executing real build steps when dryRun is true: after creating the CompactCompiler and awaiting compiler.compile(), inspect the same dry-run option (this.options.dryRun or compiler.dryRun if available) and return early (or skip the for loop) so executeStep(...) is not invoked during a dry run; reference the build method, CompactCompiler.compile, and executeStep to locate and change the logic.packages/cli/src/Compiler.ts (1)
922-930:⚠️ Potential issue | 🟡 Minor
[COMPILE]prefix shown in dry-run mode when no files are foundWhen
dryRunistrueandcompactFiles.length === 0(either because the directory is truly empty, or because every file matched an--excludepattern),showNoFilesis called before the dry-run branch is reached. This emits:⚠ [COMPILE] No .compact files found in .The
[COMPILE]prefix implies compilation was attempted, which is misleading during a dry-run. When all files are excluded the message is also indistinguishable from "no files exist", making it impossible to verify that--excludepatterns are correct via--dry-run.🐛 Proposed fix – move the dry-run gate before showNoFiles
- if (compactFiles.length === 0) { - UIService.showNoFiles(this.options.targetDir); - return; - } - if (this.options.dryRun) { UIService.showDryRun(compactFiles, this.options.targetDir); return; } + if (compactFiles.length === 0) { + UIService.showNoFiles(this.options.targetDir); + return; + } + UIService.showCompilationStart(compactFiles.length, this.options.targetDir);
showDryRunalready handles the zero-file case gracefully ("Would compile 0 file(s)") and keeps the[DRY-RUN]prefix throughout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/Compiler.ts` around lines 922 - 930, The dry-run branch must be evaluated before the "no files" check so UIService.showDryRun handles the zero-file case and uses the correct "[DRY-RUN]" prefix; in the method where compactFiles is checked (in Compiler.ts near the block referencing compactFiles, this.options.dryRun, UIService.showNoFiles and UIService.showDryRun) move the if (this.options.dryRun) { UIService.showDryRun(compactFiles, this.options.targetDir); return; } check to occur before the if (compactFiles.length === 0) { UIService.showNoFiles(...); return; } so dry-run always takes precedence and short-circuits before showing the "no files" compile message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/README.md`:
- Around line 130-135: The fenced code block showing the example output (the
block containing "ℹ [DRY-RUN] Would compile 2 file(s):" and the two file lines)
lacks a language specifier which triggers markdownlint MD040; update that fenced
block to include a language tag such as text or console (e.g., replace ``` with
```text) so the linter is satisfied while keeping the existing contents
unchanged.
In `@packages/cli/src/Builder.ts`:
- Around line 78-82: The current shell task in Builder that runs 'find src -name
"*.compact" -exec cp {} dist/' ignores this.options.exclude and flattens files;
replace that shell task with a TypeScript copy that uses the already-filtered
list from FileDiscovery (the same list CompactCompiler uses) so excluded
patterns are respected, create target subdirectories under dist preserving each
file's relative path (avoid name collisions), and ensure directories are created
before writing; update the Builder task definition to call this new copy routine
instead of the shell command.
---
Outside diff comments:
In `@packages/cli/README.md`:
- Around line 183-185: Update the README step that currently reads "Copy and
clean `.compact` files to `dist/`" to reflect the new behavior: change the
wording to "Copy `.compact` files to `dist/`" (or similar phrasing) to remove
mention of cleanup/removal of Mock*.compact, referencing the entry in the CLI
README where the artifact copy steps are listed so the documentation matches the
PR changes.
In `@packages/cli/src/Builder.ts`:
- Around line 116-125: The build method currently always runs executeStep over
this.steps even when the CompactCompiler.compile() returned early for a dry run;
update build to check the dry-run flag and skip executing real build steps when
dryRun is true: after creating the CompactCompiler and awaiting
compiler.compile(), inspect the same dry-run option (this.options.dryRun or
compiler.dryRun if available) and return early (or skip the for loop) so
executeStep(...) is not invoked during a dry run; reference the build method,
CompactCompiler.compile, and executeStep to locate and change the logic.
In `@packages/cli/src/Compiler.ts`:
- Around line 922-930: The dry-run branch must be evaluated before the "no
files" check so UIService.showDryRun handles the zero-file case and uses the
correct "[DRY-RUN]" prefix; in the method where compactFiles is checked (in
Compiler.ts near the block referencing compactFiles, this.options.dryRun,
UIService.showNoFiles and UIService.showDryRun) move the if
(this.options.dryRun) { UIService.showDryRun(compactFiles,
this.options.targetDir); return; } check to occur before the if
(compactFiles.length === 0) { UIService.showNoFiles(...); return; } so dry-run
always takes precedence and short-circuits before showing the "no files" compile
message.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/Builder.tspackages/cli/src/Compiler.tspackages/cli/test/Compiler.test.ts
💤 Files with no reviewable changes (1)
- packages/cli/package.json
| **Example output:** | ||
| ``` | ||
| ℹ [DRY-RUN] Would compile 2 file(s): | ||
| Token.compact | ||
| AccessControl.compact | ||
| ``` |
There was a problem hiding this comment.
Fenced code block missing language specifier
The markdownlint MD040 warning here is valid — the example output block has no language tag. Use text (or console) to satisfy the linter.
📝 Proposed fix
-```
+```text
ℹ [DRY-RUN] Would compile 2 file(s):
Token.compact
AccessControl.compact</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
**Example output:**
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 131-131: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/README.md` around lines 130 - 135, The fenced code block showing
the example output (the block containing "ℹ [DRY-RUN] Would compile 2 file(s):"
and the two file lines) lacks a language specifier which triggers markdownlint
MD040; update that fenced block to include a language tag such as text or
console (e.g., replace ``` with ```text) so the linter is satisfied while
keeping the existing contents unchanged.
| { | ||
| cmd: 'mkdir -p dist && find src -type f -name "*.compact" -exec cp {} dist/ \\; 2>/dev/null && rm dist/Mock*.compact 2>/dev/null || true', | ||
| msg: 'Copying and cleaning .compact files', | ||
| cmd: 'mkdir -p dist && find src -type f -name "*.compact" -exec cp {} dist/ \\; 2>/dev/null || true', | ||
| msg: 'Copying .compact files', | ||
| shell: '/bin/bash', | ||
| }, |
There was a problem hiding this comment.
--exclude patterns are not honoured by the .compact copy step
The shell command at line 79 copies every .compact file found under src/ regardless of this.options.exclude. Exclude patterns are propagated to CompactCompiler (so mock files aren't compiled), but they never reach this step, so the excluded source files are still written to dist/. The old rm dist/Mock*.compact cleanup that kept them out of dist/ was removed without an equivalent filter here, leaving dist/ populated with the very mock contracts issue #66 aimed to exclude.
Additionally, because find ... -exec cp {} dist/ flattens all .compact files into a single directory, same-named files from different subdirectories (e.g., src/access/Token.compact and src/token/Token.compact) would silently overwrite each other.
The cleanest fix is to perform the copy in TypeScript, reusing the already-filtered file list from FileDiscovery, rather than relying on a raw shell command.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/Builder.ts` around lines 78 - 82, The current shell task in
Builder that runs 'find src -name "*.compact" -exec cp {} dist/' ignores
this.options.exclude and flattens files; replace that shell task with a
TypeScript copy that uses the already-filtered list from FileDiscovery (the same
list CompactCompiler uses) so excluded patterns are respected, create target
subdirectories under dist preserving each file's relative path (avoid name
collisions), and ensure directories are created before writing; update the
Builder task definition to call this new copy routine instead of the shell
command.
There was a problem hiding this comment.
🧩 Analysis chain
@emnul, agreed — the copy step needs to be fixed. I'll plan the TypeScript-based replacement now.
Task: Fix .compact copy step to respect exclude patterns and preserve directory structure
- Add
copyCompactFiles()private method toCompactBuilderusingFileDiscovery - Replace the raw shell copy step with a call to this new method
- Preserve relative paths under
dist/to avoid name collisions
🏁 Script executed:
#!/bin/bash
# Check current imports and FileDiscovery.getCompactFiles signature
rg -n "getCompactFiles\|DEFAULT_SRC_DIR" packages/cli/src/Compiler.ts | head -40
rg -n "^import" packages/cli/src/Builder.ts
Length of output: 296
🏁 Script executed:
#!/bin/bash
rg -n "getCompactFiles\|DEFAULT_SRC_DIR\|class FileDiscovery" packages/cli/src/Compiler.ts | head -40
rg -n "^import" packages/cli/src/Builder.ts
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Get FileDiscovery class, getCompactFiles, and DEFAULT_SRC_DIR from Compiler.ts
ast-grep --pattern 'class FileDiscovery {
$$$
}' packages/cli/src/Compiler.ts
rg -n "DEFAULT_SRC_DIR|getCompactFiles|srcDir" packages/cli/src/Compiler.ts | head -30
Length of output: 7098
@emnul, here's the concrete fix. The plan replaces the raw shell copy step with a TypeScript method that reuses FileDiscovery (honoring exclude patterns) and preserves each file's relative path under dist/ to prevent name collisions.
Task: Fix .compact copy step in Builder.ts
- Import
FileDiscovery+ Node.jsfs/pathutilities - Extend step type to support an async
fncallback alongsidecmd - Replace 3rd shell step with a
fnpointing to a newcopyCompactFiles()method - Update
executeStepto dispatch correctly onfnvscmd - Add private
copyCompactFiles()usingFileDiscoverywithexcludepatterns
Failed to handle agent chat message. Please try again.
| ``` | ||
|
|
||
| Accepts all compiler options except `--skip-zk` (builds always include ZK proofs). | ||
| Accepts all compiler options except `--skip-zk` (builds always include ZK proofs). Use `--exclude` to skip mock contracts or test files during the build. |
There was a problem hiding this comment.
Correct me if I'm wrong here, but I'm not seeing any logic that filters --skip-zk for the Builder CLI. The docs should be updated or that logic should be added if we want to enforce this requirement
| { | ||
| cmd: 'mkdir -p dist && find src -type f -name "*.compact" -exec cp {} dist/ \\; 2>/dev/null && rm dist/Mock*.compact 2>/dev/null || true', | ||
| msg: 'Copying and cleaning .compact files', | ||
| cmd: 'mkdir -p dist && find src -type f -name "*.compact" -exec cp {} dist/ \\; 2>/dev/null || true', | ||
| msg: 'Copying .compact files', | ||
| shell: '/bin/bash', | ||
| }, |
| } else if (args[i] === '--exclude') { | ||
| const valueExists = | ||
| i + 1 < args.length && !args[i + 1].startsWith('--'); | ||
| if (valueExists) { | ||
| options.exclude = options.exclude ?? []; | ||
| options.exclude.push(args[i + 1]); | ||
| i++; | ||
| } else { | ||
| throw new Error('--exclude flag requires a glob pattern'); | ||
| } | ||
| } else if (args[i] === '--include') { | ||
| const valueExists = | ||
| i + 1 < args.length && !args[i + 1].startsWith('--'); | ||
| if (valueExists) { | ||
| options.include = options.include ?? []; | ||
| options.include.push(args[i + 1]); | ||
| i++; | ||
| } else { | ||
| throw new Error('--include flag requires a glob pattern'); | ||
| } |
There was a problem hiding this comment.
Maybe it's about time we use a cli parser package like yargs instead of hand rolling it with this logic monster
| /** | ||
| * Returns true if path matches the glob pattern. | ||
| * Supports ** (match zero or more path segments) and * (match within segment). | ||
| * | ||
| * @param path - The file path to test (use forward slashes) | ||
| * @param pattern - Glob pattern to match against | ||
| * @returns true if the path matches the pattern | ||
| * @example | ||
| * ```typescript | ||
| * // Pattern string: two asterisks, slash, asterisk, .mock.compact (backslash in code escapes slash for JSDoc) | ||
| * matchGlob('foo/bar.mock.compact', '**\/*.mock.compact'); // true | ||
| * matchGlob('bar.mock.compact', '*.mock.compact'); // true | ||
| * matchGlob('test/unit/foo.compact', '**\/test/**'); // true | ||
| * ``` | ||
| */ | ||
| export function matchGlob(path: string, pattern: string): boolean { | ||
| const re = globToRegExp(pattern); | ||
| return re.test(path); | ||
| } | ||
|
|
||
| /** | ||
| * Converts a glob pattern to a RegExp. | ||
| * Supports: | ||
| * - `**` matches zero or more path segments | ||
| * - `*` matches any characters except `/` | ||
| * | ||
| * Does NOT support: | ||
| * - `?` (single character wildcard) | ||
| * - Brace expansion `{a,b}` | ||
| * - Negation `!pattern` | ||
| * | ||
| * @param pattern - Glob pattern to convert | ||
| * @returns RegExp that matches paths according to the glob pattern | ||
| * @example | ||
| * ```typescript | ||
| * // Pattern string: two asterisks, slash, asterisk, .mock.compact (backslash in code escapes slash for JSDoc) | ||
| * const re = globToRegExp('**\/*.mock.compact'); | ||
| * re.test('foo/bar.mock.compact'); // true | ||
| * re.test('bar.mock.compact'); // true (** matches zero segments) | ||
| * ``` | ||
| */ | ||
| // Placeholders for glob-to-regex (private-use Unicode, not in patterns) | ||
| const GLOB_STAR_STAR = '\uE001'; | ||
| const GLOB_STAR = '\uE002'; | ||
| const GLOB_STAR_STAR_END = '\uE003'; | ||
|
|
||
| export function globToRegExp(pattern: string): RegExp { | ||
| const escaped = pattern | ||
| .replace(/[.+^${}()|[\]\\]/g, '\\$&') | ||
| .replace(/\*\*\//g, GLOB_STAR_STAR) | ||
| .replace(/\/\*\*$/g, `/${GLOB_STAR_STAR_END}`) | ||
| .replace(/\*\*/g, GLOB_STAR_STAR) | ||
| .replace(/\*/g, GLOB_STAR); | ||
| const reStr = escaped | ||
| .replace(new RegExp(GLOB_STAR_STAR, 'g'), '(?:[^/]*/)*') | ||
| .replace(new RegExp(GLOB_STAR, 'g'), '[^/]*') | ||
| .replace(new RegExp(GLOB_STAR_STAR_END, 'g'), '.*'); | ||
| return new RegExp(`^${reStr}$`); | ||
| } |
There was a problem hiding this comment.
This feels like an overly complex solution when we could instead use node's built-in import { matchesGlob } from 'node:path'; in isIncluded
| /** | ||
| * Displays dry-run output showing which files would be compiled. | ||
| * | ||
| * @param files - Array of file paths that would be compiled | ||
| * @param targetDir - Optional target directory being compiled | ||
| * @example | ||
| * ```typescript | ||
| * UIService.showDryRun(['Token.compact', 'AccessControl.compact']); | ||
| * // Output: | ||
| * // [DRY-RUN] Would compile 2 file(s): | ||
| * // Token.compact | ||
| * // AccessControl.compact | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
It should be noted that --dry-run skips environment validation
| it('should create instance with all parameters', () => { | ||
| compiler = new CompactCompiler( | ||
| { | ||
| flags: '--skip-zk', | ||
| targetDir: 'security', | ||
| version: '0.26.0', | ||
| hierarchical: true, | ||
| srcDir: 'contracts', | ||
| outDir: 'build', | ||
| exclude: ['**/*.mock.compact'], | ||
| include: ['**/test/**'], |
There was a problem hiding this comment.
Shouldn't we also have dryRun and verbose here?
| expect(compiler.testOptions.exclude).toEqual(['**/*.mock.compact']); | ||
| expect(compiler.testOptions.include).toEqual(['**/test/**']); |
| ); | ||
| }); | ||
|
|
||
| it('should parse --exclude flag', () => { |
There was a problem hiding this comment.
What should the behavior be if the include and exclude globs are the same?
| }); | ||
| }); | ||
|
|
||
| describe('CompactCompiler', () => { |
There was a problem hiding this comment.
No E2E tests for the --include flag in CompactCompiler. It's a minor concern, but would surface any internal wiring issues
| ); | ||
|
|
||
| showDryRunSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
| }); | |
| }); | |
| it('should pass targetDir to showDryRun', async () => { | |
| mockReaddir.mockResolvedValue([ | |
| { name: 'MyToken.compact', isFile: () => true, isDirectory: () => false }, | |
| ] as any); | |
| const showDryRunSpy = vi | |
| .spyOn(UIService, 'showDryRun') | |
| .mockImplementation(() => {}); | |
| compiler = new CompactCompiler( | |
| { dryRun: true, targetDir: 'security' }, | |
| mockExec, | |
| ); | |
| await compiler.compile(); | |
| expect(showDryRunSpy).toHaveBeenCalledWith(['MyToken.compact'], 'security'); | |
| showDryRunSpy.mockRestore(); | |
| }); |
we don't validate that this.options.targetDir flows through to showDryRun
2 participants
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #66
Motivation: for large projects like Lunarswap, it is very time confusing to wait for the builder to compile with ZK all the mock contracts, because they are only meant for testing.
--exclude(glob patterns to skip files, e.g. mocks/tests) and--dry-run(list files that would be compiled, no compile). ExportedmatchGlobandglobToRegExpfor pattern checks.rm dist/Mock*.compactstep; exclusion is handled via--excludeon the compiler.exports; CLI-only, no library import surface. README documents--excludeand--dry-run, and the Programmatic API section was removed.--dry-runparsing/behavior, andshowDryRunoutput.PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
Release Notes
New Features
--excludeCLI flag to filter files by glob patterns (repeatable option)--dry-runCLI flag to preview compilation output without executingDocumentation