Open
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository 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 a new CLI package for OpenZeppelin contracts, centralizes Zod schemas into Changes
Sequence DiagramsequenceDiagram
actor User as User (CLI)
participant CLI as index.ts (Entrypoint)
participant Parser as cli-adapter.ts<br/>(Parser)
participant Registry as registry.ts<br/>(Registry)
participant Generator as Wizard Module<br/>(print function)
participant Stdout as stdout
User->>CLI: contracts-cli solidity-erc20 --name=MyToken
CLI->>CLI: Parse process.argv
alt --help or no args
CLI->>Registry: Get available commands
CLI->>Parser: generateHelp(...)
CLI->>Stdout: Print usage
CLI->>CLI: Exit 0
else Valid command
CLI->>Registry: registry[command]
Registry-->>CLI: RegistryEntry
alt --help detected
CLI->>Parser: generateHelp(command, schema)
CLI->>Stdout: Print command help
CLI->>CLI: Exit 0
else Normal execution
CLI->>Parser: parseArgsFromSchema(schema, argv)
Parser->>Parser: Introspect schema shape
Parser->>Parser: Extract flags metadata
Parser->>Parser: Parse argv into nested object
Parser->>Parser: Validate via z.object(schema)
Parser-->>CLI: Parsed options
CLI->>Registry: entry.run(argv)
Registry->>Generator: print(parsed_options)
Generator-->>Registry: Code string
Registry-->>CLI: Code string
CLI->>Stdout: Write code
CLI->>CLI: Exit 0
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans 70+ files with heterogeneous changes: a new CLI package with non-trivial schema introspection/parsing logic, systematic schema renaming/reorganization affecting 40+ MCP tool files, package export changes introducing breaking updates, and multiple configuration/workflow updates. While many MCP changes are repetitive (import swaps), the CLI adapter requires careful logic review, and the schema refactoring introduces breaking API changes warranting thorough validation. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/common/src/ai/descriptions/uniswap-hooks.ts (1)
48-50: Consider removing hardcoded leading spaces in generated bullets.The current
- ...formatting can produce overly indented help text depending on renderer; using plain-is usually safer.✂️ Suggested adjustment
- hook: `The name of the Uniswap hook. Available hooks:\n${Object.entries(byHooksDescriptions) - .map(([name, desc]) => ` - ${name}: ${desc}`) + hook: `The name of the Uniswap hook. Available hooks:\n${Object.entries(byHooksDescriptions) + .map(([name, desc]) => `- ${name}: ${desc}`) .join('\n')}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/ai/descriptions/uniswap-hooks.ts` around lines 48 - 50, The hook help string in the hook property builds bullets with hardcoded leading spaces (" - ${name}: ${desc}") which can cause excessive indentation; update the template in the hook construction that maps over byHooksDescriptions to use a plain "- ${name}: ${desc}" (no leading spaces) so bullets render consistently across consumers (look for the hook template that references byHooksDescriptions and change the mapped string and join accordingly).packages/common/src/ai/schemas/cairo.ts (1)
109-111: Optional: deduplicate repeated common field picks.
upgradeable/info/macrosare repeated across several schemas; extracting a small shared object would reduce maintenance overhead.Also applies to: 117-119, 138-142, 151-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/ai/schemas/cairo.ts` around lines 109 - 111, Several schemas repeatedly pick cairoCommonSchema.upgradeable, .info, and .macros; extract these into a single shared object (e.g., const cairoSharedFields = { upgradeable: cairoCommonSchema.upgradeable, info: cairoCommonSchema.info, macros: cairoCommonSchema.macros }) and replace the repeated property lists by spreading or merging cairoSharedFields into each schema (locations referencing cairoCommonSchema.upgradeable/info/macros such as the blocks around the current picks and the other occurrences you noted). Update any exports/imports as needed so all schemas use the new cairoSharedFields to avoid duplication and reduce maintenance overhead.packages/common/package.json (1)
9-17: Consider addingtypesfield to exports for TypeScript ESM consumers.While
typesVersionsworks for many cases, modern TypeScript ESM projects may benefit from explicittypesconditions in the exports field for better type resolution:"exports": { ".": { "types": "./dist/index.d.ts", "default": "./dist/index.js" }, "./schemas": { "types": "./dist/ai/schemas/index.d.ts", "default": "./dist/ai/schemas/index.js" } }This provides more explicit type resolution for consumers using
moduleResolution: "bundler"or"node16".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/package.json` around lines 9 - 17, Update the package.json "exports" entries to include explicit "types" conditions alongside the current JS paths so TypeScript ESM consumers can find declarations; specifically change the "." export to an object with "types": "dist/index.d.ts" and "default": "dist/index.js", and change the "./schemas" export to an object with "types": "dist/ai/schemas/index.d.ts" and "default": "dist/ai/schemas/index.js" (keep or remove typesVersions as desired). Locate and modify the "exports" block and ensure the referenced declaration files exist at the specified paths.packages/cli/src/cli-adapter.ts (1)
117-121: Consider explicit number validation for better error messages.
Number(value)returnsNaNfor non-numeric strings. While Zod'ssafeParsewill catch this, the error message may be confusing (e.g., "Expected number, received nan"). Consider validating before conversion:🔧 Suggested improvement
function parseFlagValue(spec: FlagSpec, value: string): string | number | boolean { - if (spec.type === 'number') return Number(value); + if (spec.type === 'number') { + const num = Number(value); + if (Number.isNaN(num)) { + throw new Error(`Invalid number for --${spec.name}: ${value}`); + } + return num; + } if (spec.hasLiteralFalse && value === 'false') return false; return value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli-adapter.ts` around lines 117 - 121, The parseFlagValue function currently converts numeric flags with Number(value) which yields NaN for invalid numeric input; update parseFlagValue to explicitly validate the string before conversion when spec.type === 'number' (e.g., using a numeric-regex or Number.isFinite(Number(value))) and if invalid, surface a clear, descriptive error (include the flag identifier from FlagSpec if available) instead of returning NaN—otherwise safely return the parsed number; keep the checks around spec.hasLiteralFalse unchanged.packages/cli/src/cli.test.ts (1)
54-73: Registry completeness test provides good coverage.This test ensures every
kinddiscovered in core wizard packages has a corresponding CLI registry entry. The approach of parsingkind.tsfiles directly is clever for catching missing commands.Consider adding a try-catch around the
readFilecall (line 62) to provide a clearer error message if akind.tsfile is missing for a core directory, rather than letting the test fail with a generic ENOENT error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli.test.ts` around lines 54 - 73, Wrap the readFile call that loads each core package's kind.ts in a try-catch to surface a clearer failure when a file is missing: around the readFile(join(PACKAGES_CORE_PATH, coreDir, 'src', 'kind.ts'), 'utf-8') invocation in the test that uses readdir/coreDirs, catch errors and call t.fail or throw a new Error that includes the coreDir and the full path (and the original error message) so the test output states which core language is missing its kind.ts; keep the rest of the logic using kindsFromSource and coreKindToCommand unchanged.packages/common/src/ai/schemas/schemas.test.ts (1)
42-70: Potential for botht.fail()andt.pass()to be called in nested schema validation.When a nested
ZodObjectfield fails validation in the recursive call (line 59),t.fail()is called and that recursive frame returns, but the outer loop continues viacontinueat line 60. If all remaining top-level fields pass,t.pass()at line 69 will also be called. Depending on AVA's behavior, this may mask failures or cause unexpected test outcomes.Consider tracking failure state and returning early from the outer function:
♻️ Proposed fix
function assertAllFieldsHaveDescriptions( t: { pass: () => void; fail: (msg: string) => void }, shape: z.ZodRawShape, prefix = '', -) { +): boolean { for (const [key, schema] of Object.entries(shape)) { const zodSchema = schema as z.ZodTypeAny; const fullKey = prefix ? `${prefix}.${key}` : key; // Unwrap optional to check inner type let innerSchema = zodSchema; if (innerSchema._def.typeName === 'ZodOptional') { innerSchema = innerSchema._def.innerType as z.ZodTypeAny; } // For ZodObject fields, recurse into inner fields (the object itself may or may not have a description) if (innerSchema._def.typeName === 'ZodObject') { - assertAllFieldsHaveDescriptions(t, (innerSchema as z.ZodObject<z.ZodRawShape>).shape, fullKey); - continue; + if (!assertAllFieldsHaveDescriptions(t, (innerSchema as z.ZodObject<z.ZodRawShape>).shape, fullKey)) { + return false; + } + continue; } const desc = zodSchema.description ?? zodSchema._def?.innerType?.description; if (!desc) { t.fail(`Field "${fullKey}" is missing a .describe() string`); - return; + return false; } } t.pass(); + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/ai/schemas/schemas.test.ts` around lines 42 - 70, The recursive assertAllFieldsHaveDescriptions may call t.fail() in a nested call but still proceed to call t.pass() at the end; change the function to short-circuit on failures by returning a boolean (or throw) from assertAllFieldsHaveDescriptions so callers can detect failure: when you call assertAllFieldsHaveDescriptions recursively from inside the ZodObject branch, check its return value and immediately return false if it failed; similarly, whenever you call t.fail(...) return false right away; only call t.pass() when the function is about to return true (no failures). Update the assertAllFieldsHaveDescriptions signature and all call sites accordingly (use the function name assertAllFieldsHaveDescriptions and the recursive call site currently invoking it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/changeset/SKILL.md:
- Around line 26-28: Replace the inline code spans that contain internal
trailing spaces (e.g., the `` `- ` `` example) with lint-safe wording: write the
first bullet as a plain sentence (no leading dash), describe subsequent items
using a literal hyphen dash but not wrapped in an inline code span (use "- "
wording), and specify breaking changes using "- **Breaking change**:" (or "-
**Breaking changes**:" with sub-items indented using " - "). Update the text
around "First line", "Subsequent lines", and "Breaking changes" in SKILL.md to
remove any backticked examples that include internal spaces and instead use the
plain text phrasing described above so MD038 is not triggered.
In `@packages/cli/src/cli-adapter.ts`:
- Around line 21-35: unwrapSchema currently relies on internal Zod internals
(_def.innerType and _def.schema) which will break on Zod 4; add integration
tests for the unwrapSchema function that cover ZodOptional, ZodNullable,
ZodDefault and ZodEffects inputs (using the unwrapSchema symbol) to lock
expected behavior into CI, and replace the internal _def accesses before
upgrading by refactoring unwrapSchema to use only public Zod APIs or documented
helper methods (do the tests first so CI will catch regressions when you
refactor or upgrade).
In `@scripts/check-circular-deps.mjs`:
- Around line 25-27: The catch block that currently swallows all errors when
reading/parsing package.json should not ignore malformed-JSON or other real
errors; update the error handling around the package.json read/JSON.parse (the
try/catch that currently has "catch { // no package.json in this dir }") to only
treat "file not found" (ENOENT) as a silent case and rethrow or log any other
errors (e.g., JSON parse errors) including the filename and error details so
invalid package.json is surfaced rather than hidden.
---
Nitpick comments:
In `@packages/cli/src/cli-adapter.ts`:
- Around line 117-121: The parseFlagValue function currently converts numeric
flags with Number(value) which yields NaN for invalid numeric input; update
parseFlagValue to explicitly validate the string before conversion when
spec.type === 'number' (e.g., using a numeric-regex or
Number.isFinite(Number(value))) and if invalid, surface a clear, descriptive
error (include the flag identifier from FlagSpec if available) instead of
returning NaN—otherwise safely return the parsed number; keep the checks around
spec.hasLiteralFalse unchanged.
In `@packages/cli/src/cli.test.ts`:
- Around line 54-73: Wrap the readFile call that loads each core package's
kind.ts in a try-catch to surface a clearer failure when a file is missing:
around the readFile(join(PACKAGES_CORE_PATH, coreDir, 'src', 'kind.ts'),
'utf-8') invocation in the test that uses readdir/coreDirs, catch errors and
call t.fail or throw a new Error that includes the coreDir and the full path
(and the original error message) so the test output states which core language
is missing its kind.ts; keep the rest of the logic using kindsFromSource and
coreKindToCommand unchanged.
In `@packages/common/package.json`:
- Around line 9-17: Update the package.json "exports" entries to include
explicit "types" conditions alongside the current JS paths so TypeScript ESM
consumers can find declarations; specifically change the "." export to an object
with "types": "dist/index.d.ts" and "default": "dist/index.js", and change the
"./schemas" export to an object with "types": "dist/ai/schemas/index.d.ts" and
"default": "dist/ai/schemas/index.js" (keep or remove typesVersions as desired).
Locate and modify the "exports" block and ensure the referenced declaration
files exist at the specified paths.
In `@packages/common/src/ai/descriptions/uniswap-hooks.ts`:
- Around line 48-50: The hook help string in the hook property builds bullets
with hardcoded leading spaces (" - ${name}: ${desc}") which can cause
excessive indentation; update the template in the hook construction that maps
over byHooksDescriptions to use a plain "- ${name}: ${desc}" (no leading spaces)
so bullets render consistently across consumers (look for the hook template that
references byHooksDescriptions and change the mapped string and join
accordingly).
In `@packages/common/src/ai/schemas/cairo.ts`:
- Around line 109-111: Several schemas repeatedly pick
cairoCommonSchema.upgradeable, .info, and .macros; extract these into a single
shared object (e.g., const cairoSharedFields = { upgradeable:
cairoCommonSchema.upgradeable, info: cairoCommonSchema.info, macros:
cairoCommonSchema.macros }) and replace the repeated property lists by spreading
or merging cairoSharedFields into each schema (locations referencing
cairoCommonSchema.upgradeable/info/macros such as the blocks around the current
picks and the other occurrences you noted). Update any exports/imports as needed
so all schemas use the new cairoSharedFields to avoid duplication and reduce
maintenance overhead.
In `@packages/common/src/ai/schemas/schemas.test.ts`:
- Around line 42-70: The recursive assertAllFieldsHaveDescriptions may call
t.fail() in a nested call but still proceed to call t.pass() at the end; change
the function to short-circuit on failures by returning a boolean (or throw) from
assertAllFieldsHaveDescriptions so callers can detect failure: when you call
assertAllFieldsHaveDescriptions recursively from inside the ZodObject branch,
check its return value and immediately return false if it failed; similarly,
whenever you call t.fail(...) return false right away; only call t.pass() when
the function is about to return true (no failures). Update the
assertAllFieldsHaveDescriptions signature and all call sites accordingly (use
the function name assertAllFieldsHaveDescriptions and the recursive call site
currently invoking it).
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d7376eb-94a5-4066-bb18-417d2f9ed1fc
⛔ Files ignored due to path filters (2)
packages/cli/src/cli.test.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (79)
.changeset/add-cli-package.md.changeset/mcp-use-common-schemas.md.changeset/move-schemas-to-common.md.claude/skills/changeset/SKILL.md.github/workflows/test.yml.gitignorepackage.jsonpackages/cli/LICENSEpackages/cli/NOTICEpackages/cli/ava.config.jspackages/cli/package.jsonpackages/cli/src/cli-adapter.tspackages/cli/src/cli-options.test.tspackages/cli/src/cli.test.tspackages/cli/src/cli.test.ts.mdpackages/cli/src/index.tspackages/cli/src/registry.tspackages/cli/tsconfig.jsonpackages/common/ava.config.jspackages/common/package.jsonpackages/common/src/ai/descriptions/uniswap-hooks.tspackages/common/src/ai/schemas/cairo.tspackages/common/src/ai/schemas/confidential.tspackages/common/src/ai/schemas/index.tspackages/common/src/ai/schemas/schemas.test.tspackages/common/src/ai/schemas/solidity.tspackages/common/src/ai/schemas/stellar.tspackages/common/src/ai/schemas/stylus.tspackages/common/src/ai/schemas/uniswap-hooks.tspackages/common/tsconfig.jsonpackages/mcp/src/cairo/tools/account.test.tspackages/mcp/src/cairo/tools/account.tspackages/mcp/src/cairo/tools/custom.test.tspackages/mcp/src/cairo/tools/custom.tspackages/mcp/src/cairo/tools/erc1155.test.tspackages/mcp/src/cairo/tools/erc1155.tspackages/mcp/src/cairo/tools/erc20.test.tspackages/mcp/src/cairo/tools/erc20.tspackages/mcp/src/cairo/tools/erc721.test.tspackages/mcp/src/cairo/tools/erc721.tspackages/mcp/src/cairo/tools/governor.test.tspackages/mcp/src/cairo/tools/governor.tspackages/mcp/src/cairo/tools/multisig.test.tspackages/mcp/src/cairo/tools/multisig.tspackages/mcp/src/cairo/tools/vesting.test.tspackages/mcp/src/cairo/tools/vesting.tspackages/mcp/src/confidential/tools/erc7984.test.tspackages/mcp/src/confidential/tools/erc7984.tspackages/mcp/src/solidity/tools/account.test.tspackages/mcp/src/solidity/tools/account.tspackages/mcp/src/solidity/tools/custom.test.tspackages/mcp/src/solidity/tools/custom.tspackages/mcp/src/solidity/tools/erc1155.test.tspackages/mcp/src/solidity/tools/erc1155.tspackages/mcp/src/solidity/tools/erc20.test.tspackages/mcp/src/solidity/tools/erc20.tspackages/mcp/src/solidity/tools/erc721.test.tspackages/mcp/src/solidity/tools/erc721.tspackages/mcp/src/solidity/tools/governor.test.tspackages/mcp/src/solidity/tools/governor.tspackages/mcp/src/solidity/tools/rwa.test.tspackages/mcp/src/solidity/tools/rwa.tspackages/mcp/src/solidity/tools/stablecoin.test.tspackages/mcp/src/solidity/tools/stablecoin.tspackages/mcp/src/stellar/tools/fungible.test.tspackages/mcp/src/stellar/tools/fungible.tspackages/mcp/src/stellar/tools/non-fungible.test.tspackages/mcp/src/stellar/tools/non-fungible.tspackages/mcp/src/stellar/tools/stablecoin.test.tspackages/mcp/src/stellar/tools/stablecoin.tspackages/mcp/src/stylus/tools/erc1155.test.tspackages/mcp/src/stylus/tools/erc1155.tspackages/mcp/src/stylus/tools/erc20.test.tspackages/mcp/src/stylus/tools/erc20.tspackages/mcp/src/stylus/tools/erc721.test.tspackages/mcp/src/stylus/tools/erc721.tspackages/mcp/src/uniswap-hooks/tools/hooks.test.tspackages/mcp/src/uniswap-hooks/tools/hooks.tsscripts/check-circular-deps.mjs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
10-10: Optional wording polish for list consistency/readability.This is correct as written; if you want to satisfy the style warning, you can vary the sentence start (e.g., “
packages/cliprovides the CLI interface”).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 10, Update the list item for `packages/cli` to use consistent wording by rephrasing it to something like "`packages/cli` provides the CLI" (or similar phrasing such as "contains the CLI interface") to satisfy the style warning while preserving meaning; edit the `packages/cli` list entry in CONTRIBUTING.md to use the new wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CONTRIBUTING.md`:
- Line 10: Update the list item for `packages/cli` to use consistent wording by
rephrasing it to something like "`packages/cli` provides the CLI" (or similar
phrasing such as "contains the CLI interface") to satisfy the style warning
while preserving meaning; edit the `packages/cli` list entry in CONTRIBUTING.md
to use the new wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ee03e646-12c8-4430-9a31-c8967fd6fbd2
📒 Files selected for processing (5)
.claude/skills/changeset/SKILL.mdCONTRIBUTING.mdREADME.mdpackages/cli/README.mdscripts/check-circular-deps.mjs
✅ Files skipped from review due to trivial changes (3)
- packages/cli/README.md
- scripts/check-circular-deps.mjs
- .claude/skills/changeset/SKILL.md
Member
|
CodeRabbit pointed out this issue I think deserves attention:
|
ernestognw
approved these changes
Mar 30, 2026
Member
ernestognw
left a comment
ernestognw
left a comment
There was a problem hiding this comment.
LGTM aside from the CodeRabbit comment I pointed out
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
Member
Author
|
@SocketSecurity ignore-all |
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
3 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.
Adds a new
@openzeppelin/contracts-clipackage that generates smart contracts from the command line for all supported languages and contract kinds.Schemas are shared with the MCP package via a new
@openzeppelin/wizard-common/schemassubpath export.