feat(tools/compiler): introduce artifacts manifest, and strict the types#42
feat(tools/compiler): introduce artifacts manifest, and strict the types#42
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR introduces an artifact manifest system for the Compact CLI to detect and handle structure mismatches when switching between flattened and hierarchical artifact output layouts. It refactors compiler logic into modular services, strengthens type safety with enums for compiler versions and flags, and adds interactive prompting for resolving mismatched structures or a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Compiler as CompactCompiler
participant EnvVal as EnvironmentValidator
participant FileDisc as FileDiscovery
participant ManifestSvc as ManifestService
participant CompilerSvc as CompilerService
participant UI as UIService
CLI->>Compiler: compile(options)
rect rgb(200, 220, 255)
note over Compiler,UI: Environment Validation
Compiler->>EnvVal: validate(version)
EnvVal-->>Compiler: { compactToolVersion, compactcVersion }
Compiler->>UI: displayEnvInfo(...)
end
rect rgb(200, 235, 200)
note over Compiler,ManifestSvc: Structure Mismatch Check
Compiler->>ManifestSvc: checkMismatch(requestedStructure)
ManifestSvc-->>Compiler: existingManifest | null
alt Mismatch Detected
Compiler-->>CLI: throw StructureMismatchError
CLI->>CLI: promptConfirmation()
alt User Confirms
CLI->>Compiler: cleanAndCompile()
Compiler->>ManifestSvc: cleanOutputDirectory()
else User Declines
CLI-->>CLI: exit
end
end
end
rect rgb(255, 240, 200)
note over Compiler,FileDisc: File Discovery & Compilation
Compiler->>FileDisc: getCompactFiles(srcDir)
FileDisc-->>Compiler: [ file1.compact, file2.compact ]
Compiler->>UI: showCompilationStart(fileCount)
loop For Each File
Compiler->>CompilerSvc: compileFile(file, flags, version)
CompilerSvc-->>Compiler: { stdout, stderr }
Compiler->>Compiler: trackArtifacts()
end
end
rect rgb(240, 200, 255)
note over Compiler,ManifestSvc: Manifest Generation & Write
Compiler->>Compiler: buildManifest(artifacts, timing, metadata)
Compiler->>ManifestSvc: write(manifest)
ManifestSvc-->>Compiler: void
Compiler-->>CLI: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
0xisk
force-pushed
the
feat/artifacts-structure
branch
from
2a4b976 to
61a9922
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
packages/cli/test/runCompiler.test.ts (1)
12-20: Missing test coverage forStructureMismatchErrorhandling.The mock correctly re-exports
StructureMismatchError, but there are no tests verifying thehandleStructureMismatchbehavior introduced inrunCompiler.ts. Consider adding tests for:
- Interactive prompt flow when
process.stdin.isTTYis true- Non-TTY exit with
--forceguidance- User confirmation leading to
cleanAndCompile- User rejection leading to graceful exit
packages/cli/src/services/FileDiscovery.ts (1)
64-68: Silent failure on missing directory may mask configuration errors.When the source directory doesn't exist or is inaccessible, this returns an empty array and logs to stderr. This could mask misconfiguration issues (e.g., typo in
srcDir). Consider throwing an error for the top-level directory failure while keeping graceful handling for subdirectories.} catch (err) { // biome-ignore lint/suspicious/noConsole: Needed to display error and dir path console.error(`Failed to read dir: ${dir}`, err); - return []; + throw err; }Alternatively, if silent failure is intentional for flexibility, document this behavior explicitly in the JSDoc.
packages/cli/src/services/UIService.ts (2)
28-34: Directconsole.logusage without lint suppression.Other services in this PR use
// biome-ignore lint/suspicious/noConsole:comments. For consistency, either add the suppression comment here or consider whether this is intentional for UIService (since it's explicitly a UI output service).printOutput(output: string, colorFn: (text: string) => string): void { const lines = output .split('\n') .filter((line) => line.trim() !== '') .map((line) => ` ${line}`); + // biome-ignore lint/suspicious/noConsole: UIService is responsible for console output console.log(colorFn(lines.join('\n'))); },
55-66: Consider reusing ora instance or using simpler output.Each method creates a new
ora()instance just to call.info()or.warn()once. While functionally correct, this is slightly inefficient. Consider either:
- Creating a single shared spinner instance
- Using
console.logwith chalk directly for non-spinner outputThis is a minor optimization and the current approach works fine. Defer if preferred.
packages/cli/src/services/ManifestService.ts (2)
45-55: Unsafe JSON parsing and silent error swallowing.Two concerns:
JSON.parseresult is cast toArtifactManifestwithout validation. Malformed or tampered manifests could cause downstream issues.- The catch block silently returns
nullfor all errors (parse errors, permission errors), making debugging difficult.Consider at minimum logging parse errors:
async read(): Promise<ArtifactManifest | null> { try { if (!existsSync(this.manifestPath)) { return null; } const content = await readFile(this.manifestPath, 'utf-8'); return JSON.parse(content) as ArtifactManifest; - } catch { + } catch (error) { + // Log parse/read errors for debugging; return null to allow fresh compilation + console.warn(`Failed to read manifest: ${error}`); return null; } }For validation, consider adding a schema check (e.g., with zod) or at least verifying
structurefield exists.
85-89: Redundant existence check beforerm.The
{ force: true }option already handles non-existent paths gracefully, making theexistsSynccheck unnecessary.async cleanOutputDirectory(): Promise<void> { - if (existsSync(this.outDir)) { - await rm(this.outDir, { recursive: true, force: true }); - } + await rm(this.outDir, { recursive: true, force: true }); }packages/cli/src/services/EnvironmentValidator.ts (1)
117-123: Command construction produces double space when version is omitted.When
versionis undefined,versionFlagbecomes an empty string, resulting in"compact compile --version"(note the double space). While this likely works, it's inconsistent with typical CLI invocation patterns.async getCompactcVersion(version?: string): Promise<string> { - const versionFlag = version ? `+${version}` : ''; - const { stdout } = await this.execFn( - `compact compile ${versionFlag} --version`, - ); + const command = version + ? `compact compile +${version} --version` + : `compact compile --version`; + const { stdout } = await this.execFn(command); return stdout.trim(); }packages/cli/src/types/manifest.ts (1)
25-28: NodeVersion type will require maintenance as new Node.js versions release.The hardcoded union
'18' | '20' | '21' | '22' | '23' | '24' | '25'will become stale. Consider deriving this from a const array (similar toCOMPACTC_VERSIONSin config.ts) for easier maintenance, or document that this list needs periodic updates.+// In config.ts or here +export const NODE_VERSIONS = ['18', '20', '21', '22', '23', '24', '25'] as const; + /** * Supported Node.js major versions. */ -export type NodeVersion = '18' | '20' | '21' | '22' | '23' | '24' | '25'; +export type NodeVersion = (typeof NODE_VERSIONS)[number];packages/cli/src/Compiler.ts (2)
456-462: Platform and nodeVersion may produce values outside the defined types.
nodeVersion: The regex^v(\d+)captures the major version, but platforms like Deno or Bun would have different version formats. Additionally, future Node.js versions (e.g., v26) aren't in theNodeVersiontype.platform: Combinations like'freebsd-x64'or'linux-riscv64'would be cast toPlatformdespite not being in the union.Since these are metadata fields, this may be acceptable, but the type assertions could mask unexpected values.
Consider making these optional with runtime validation:
// Get Node.js major version - const nodeVersion = process.version.match(/^v(\d+)/)?.[1] as - | NodeVersion - | undefined; + const nodeVersionMatch = process.version.match(/^v(\d+)/)?.[1]; + const nodeVersion = nodeVersionMatch as NodeVersion | undefined; // Get platform identifier - const platform = `${process.platform}-${process.arch}` as Platform; + const platformId = `${process.platform}-${process.arch}`; + const platform = platformId as Platform | undefined;Or validate against allowed values before assigning.
550-555:cleanAndCompilecallscompile()which validates environment redundantly.When
cleanAndCompile()is called (after catchingStructureMismatchErrorfrom an initialcompile()attempt), the secondcompile()call will re-runvalidateEnvironment(). This is wasteful since the environment was just validated.Consider extracting the core compilation logic or passing validated version info:
async cleanAndCompile(): Promise<void> { const spinner = ora(); spinner.info(chalk.yellow('[COMPILE] Cleaning existing artifacts...')); await this.cleanOutputDirectory(); - await this.compile(); + // Consider: pass previously validated versions or extract inner compilation logic + await this.compile(); }Alternatively, this may be acceptable if compilation is infrequent and the overhead is minimal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/cli/README.md(7 hunks)packages/cli/src/Compiler.ts(13 hunks)packages/cli/src/config.ts(1 hunks)packages/cli/src/runCompiler.ts(3 hunks)packages/cli/src/services/CompilerService.ts(1 hunks)packages/cli/src/services/EnvironmentValidator.ts(1 hunks)packages/cli/src/services/FileDiscovery.ts(1 hunks)packages/cli/src/services/ManifestService.ts(1 hunks)packages/cli/src/services/UIService.ts(1 hunks)packages/cli/src/types/manifest.ts(1 hunks)packages/cli/test/Compiler.test.ts(28 hunks)packages/cli/test/runCompiler.test.ts(1 hunks)packages/simulator/test/fixtures/sample-contracts/SampleZOwnable.compact(2 hunks)packages/simulator/test/fixtures/sample-contracts/Simple.compact(1 hunks)packages/simulator/test/fixtures/sample-contracts/Witness.compact(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
packages/cli/src/runCompiler.ts (1)
packages/cli/src/types/manifest.ts (1)
StructureMismatchError(115-130)
packages/cli/src/services/ManifestService.ts (2)
packages/cli/src/config.ts (1)
DEFAULT_OUT_DIR(10-10)packages/cli/src/types/manifest.ts (2)
MANIFEST_FILENAME(106-106)ArtifactManifest(68-103)
packages/cli/src/services/UIService.ts (1)
packages/cli/src/config.ts (2)
CompactToolVersion(41-41)CompactcVersion(38-38)
packages/cli/src/services/CompilerService.ts (4)
packages/cli/src/services/EnvironmentValidator.ts (1)
ExecFunction(39-41)packages/cli/src/config.ts (2)
DEFAULT_SRC_DIR(7-7)DEFAULT_OUT_DIR(10-10)packages/cli/src/types/manifest.ts (1)
CompilerFlag(55-60)packages/cli/src/types/errors.ts (1)
CompilationError(58-73)
packages/cli/src/services/EnvironmentValidator.ts (2)
packages/cli/src/config.ts (4)
CompactToolVersion(41-41)CompactcVersion(38-38)MIN_COMPACT_TOOL_VERSION(30-31)MAX_COMPACTC_VERSION(34-35)packages/cli/src/types/errors.ts (1)
CompactCliNotFoundError(38-48)
packages/cli/src/services/FileDiscovery.ts (1)
packages/cli/src/config.ts (1)
DEFAULT_SRC_DIR(7-7)
packages/cli/src/Compiler.ts (4)
packages/cli/src/types/manifest.ts (4)
CompilerFlag(55-60)StructureMismatchError(115-130)NodeVersion(28-28)Platform(34-40)packages/cli/src/config.ts (4)
CompactcVersion(38-38)DEFAULT_SRC_DIR(7-7)DEFAULT_OUT_DIR(10-10)CompactToolVersion(41-41)packages/cli/src/services/ManifestService.ts (1)
ManifestService(21-90)packages/cli/src/services/UIService.ts (1)
UIService(15-107)
packages/cli/src/types/manifest.ts (1)
packages/cli/src/config.ts (2)
CompactcVersion(38-38)CompactToolVersion(41-41)
🔇 Additional comments (28)
packages/simulator/test/fixtures/sample-contracts/Simple.compact (1)
7-7: LGTM!The exported ledger declaration properly exposes
_valfor thesetValandgetValcircuits. This aligns with the test fixture's purpose of providing observable state.packages/simulator/test/fixtures/sample-contracts/SampleZOwnable.compact (2)
9-14: LGTM!The ledger exports (
_ownerCommitment,_counter,_instanceSalt) are appropriately exposed for test fixture purposes, enabling external verification of ownership state.
41-44: Formatting changes look fine.These are whitespace/style adjustments with no behavioral impact.
Also applies to: 49-49, 54-54, 57-58
packages/cli/src/config.ts (1)
16-21: LGTM!Good use of
as constfor type derivation, creating a single source of truth for supported versions. The derived typesCompactcVersionandCompactToolVersionenable compile-time validation of version strings.Also applies to: 33-35, 37-41
packages/cli/README.md (2)
90-132: LGTM!Comprehensive documentation of the new structure mismatch detection feature, including the manifest file format, interactive prompts, and
--forceflag behavior for CI/CD environments.
244-264: LGTM!The updated type documentation accurately reflects the new
CompilerFlag[]array type,CompactcVersionunion type, andforceoption inCompilerOptions.packages/cli/src/runCompiler.ts (4)
125-138: LGTM!The
promptConfirmationutility correctly implements a simple yes/no prompt usingreadline. It properly closes the interface after receiving input and normalizes the response case-insensitively.
56-60: LGTM!Good placement of
StructureMismatchErrorhandling before the generic error handler, allowing specialized interactive handling while falling through to standard error paths for other cases.
269-273: LGTM!The
--force, -fflag is correctly documented in the usage help, matching the README and issue requirements.
104-112: The review comment is unnecessary.cleanAndCompile()explicitly callscleanOutputDirectory()beforecompile(), which removes the entire output directory including the manifest file. Whencompile()subsequently callscheckMismatch(), no manifest exists to check, so noStructureMismatchErroris thrown. The design prevents re-checking without requiring the--forceflag to be added.Likely an incorrect or invalid review comment.
packages/simulator/test/fixtures/sample-contracts/Witness.compact (1)
1-32: LGTM!The test fixture additions are well-structured. The new witness declarations with arguments (
wit_secretFieldPlusArg,wit_secretUintPlusArgs) correctly match the signatures expected by the existing circuits, expanding test coverage for parameterized witnesses.packages/cli/src/services/FileDiscovery.ts (1)
41-63: Clean recursive implementation with parallel processing.The use of
readdirwithwithFileTypes,Promise.allfor parallelism, andflat()for result aggregation is idiomatic and efficient.packages/cli/src/services/CompilerService.ts (1)
110-127: Solid error handling with cause preservation.The error handling correctly extracts the message from both
Errorinstances and non-Error values, and wraps them inCompilationErrorwith the file path and original cause for debugging.packages/cli/src/services/ManifestService.ts (2)
62-64:write()assumes output directory exists.If
outDirdoesn't exist,writeFilewill throwENOENT. The compilation process likely creates the directory, butManifestService.write()could be called independently.Verify that callers always ensure the directory exists before calling
write(), or consider adding:+ import { mkdir } from 'node:fs/promises'; + async write(manifest: ArtifactManifest): Promise<void> { + await mkdir(this.outDir, { recursive: true }); await writeFile(this.manifestPath, JSON.stringify(manifest, null, 2)); }
72-80: Clean mismatch detection logic.The method correctly reads the existing manifest and returns it only when a structure mismatch is detected, enabling the caller to handle the mismatch scenario (prompt user, force clean, etc.).
packages/cli/src/services/EnvironmentValidator.ts (4)
178-181: Unsafe type assertion for version strings.The returned version strings are cast to
CompactToolVersionandCompactcVersionwithout validating they're actually in the allowed set. If the CLI returns a version not in the predefined lists (e.g., a newer patch version), consumers may receive unexpected values while TypeScript assumes they're valid.Consider either:
- Validating against the allowed versions before casting
- Changing return types to
stringand letting consumers handle validation- Documenting this as intentional (versions outside the list are allowed at runtime)
19-30: LGTM!The
compareVersionshelper correctly handles semver comparison with support for versions of different lengths by defaulting missing parts to 0.
56-66: LGTM!Good use of dependency injection for the exec function, enabling testability while providing a sensible default.
158-176: LGTM!The version validation logic appropriately warns for outdated tools and errors for unsupported compiler versions. The error messages provide clear guidance on resolution steps.
packages/cli/src/types/manifest.ts (2)
68-103: LGTM!The
ArtifactManifestinterface is well-documented with JSDoc comments and appropriate use of optional fields for backward compatibility. The discriminated union forartifactsbased on structure type is a good design choice.
115-129: LGTM!
StructureMismatchErrorcorrectly extendsError, sets thenameproperty for proper error identification, and exposes both structure values as readonly properties for programmatic handling.packages/cli/test/Compiler.test.ts (3)
1140-1168: LGTM!Good test coverage for the new
--forceand-fflags, including interaction with other flags and the default behavior.
1257-1293: LGTM!The
checkMismatchtests properly cover the three cases: no manifest, matching structure, and mismatching structure.
1295-1316: LGTM!The
cleanOutputDirectorytests correctly verify both the happy path (directory exists) and edge case (directory doesn't exist).packages/cli/src/Compiler.ts (4)
235-248: Type assertions bypass type safety.Both
version(line 238) andflags(line 247) are cast to their strict types without validation. If a user passes an invalid flag like--invalid-flag, it will be typed asCompilerFlagdespite not being a valid value.Consider either:
- Validating inputs against allowed values and throwing on invalid flags
- Using looser types (
string[]) for the parsed values- Accepting this as intentional to allow pass-through of unknown flags to the underlying compiler
375-404: LGTM!The structure mismatch handling logic correctly:
- Checks for existing manifest with different structure
- Auto-cleans when
--forceis specified- Throws
StructureMismatchErrorfor CLI to handle interactivelyThis aligns well with the PR objectives from Issue #41.
427-447: LGTM!Artifact tracking correctly handles both hierarchical (grouped by subdirectory) and flattened (simple array) structures, building the appropriate data shape for the manifest.
464-479: LGTM!The manifest is written with comprehensive metadata including timing, platform info, compiler flags, and the compiled artifacts list. The conditional structure for
artifactscorrectly matches the requested structure type.
0xisk
force-pushed
the
feat/artifacts-structure
branch
from
c72b7a6 to
95c03e0
Compare
0xisk
changed the title
0xisk
changed the title
| // Check for shell metacharacters and embedded quotes | ||
| if (/[;&|`$(){}[\]<>'"\\]/.test(normalized)) { | ||
| throw new CompilationError( | ||
| `Invalid file path: contains unsafe characters: ${filePath}`, | ||
| filePath, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is unnecessary when using the execFile API (which is the default). No shell is spawned that can interpret these characters.
| // Check for shell metacharacters and embedded quotes | |
| if (/[;&|`$(){}[\]<>'"\\]/.test(normalized)) { | |
| throw new CompilationError( | |
| `Invalid file path: contains unsafe characters: ${filePath}`, | |
| filePath, | |
| ); | |
| } |
| args.push(validatedInputPath, normalizedOutputDir); | ||
|
|
||
| try { | ||
| return await this.execFileFn('compact', args); |
There was a problem hiding this comment.
It's safer if we use absolute executable paths. The standard install paths for compact can be found here https://github.com/midnightntwrk/compact-export/blob/bc6c7b94a25305821c4f66460fa25dd423f838ea/dist-workspace.toml#L42C1-L43C75
There was a problem hiding this comment.
Good point actually, could you review it? 39ea434 (#42)
|
@emnul could you please review this PR after the fixes? |
| * Standard install paths for the Compact CLI. | ||
| * Based on the dist-workspace.toml install-path configuration: | ||
| * - $XDG_BIN_HOME/ | ||
| * - $XDG_DATA_HOME/../bin (typically ~/.local/bin) | ||
| * - ~/.local/bin | ||
| * | ||
| * @see https://github.com/midnightntwrk/compact-export/blob/main/dist-workspace.toml | ||
| */ |
| | `createdAt` | `string` | ISO 8601 timestamp (e.g., `"2025-12-11T10:09:46.023Z"`) | | ||
| | `buildDuration` | `number` | Total compilation duration in milliseconds | | ||
| | `nodeVersion` | `NodeVersion` | Node.js major version (`"18"` \| `"20"` \| `"21"` \| `"22"` \| `"23"` \| `"24"` \| `"25"`) | | ||
| | `platform` | `Platform` | Platform identifier (`"linux-x64"` \| `"linux-arm64"` \| `"darwin-x64"` \| `"darwin-arm64"` \| `"win32-x64"` \| `"win32-arm64"`) | |
There was a problem hiding this comment.
I suggest only listing supported platforms to avoid confusion.
The supported platforms are "aarch64-apple-darwin", "x86_64-apple-darwin", "x86_64-unknown-linux-musl"
| "createdAt": "2025-12-11T10:35:09.916Z", | ||
| "buildDuration": 2445, | ||
| "nodeVersion": "22", | ||
| "platform": "linux-x64", |
There was a problem hiding this comment.
See comment above on supported platforms
emnul
left a comment
emnul
left a comment
There was a problem hiding this comment.
Looks good! Just a few minor comments
2 participants
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #41
artifacts/manifest.jsonfile that describes the compilation output details.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
New Features
--force/-fflag to automatically clean conflicting artifacts on structure mismatchesDocumentation
✏️ Tip: You can customize this high-level summary in your review settings.