Add validation for type declarations and schema generation#976
Open
moose-code wants to merge 2 commits intomainfrom
Open
Add validation for type declarations and schema generation#976moose-code wants to merge 2 commits intomainfrom
moose-code wants to merge 2 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces stricter validation and error handling across the CLI package. Changes include prototype-pollution protection for entity fields, explicit duplicate detection for event names, enhanced type declaration validation with cycle detection and parameter uniqueness checks, and improved error propagation in ABI type processing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
DZakh
requested changes
Feb 23, 2026
Member
DZakh
left a comment
DZakh
left a comment
There was a problem hiding this comment.
This looks good, but I'd like to have a test for every case which was fixed. I see there are tests in type_schema.rs, but it uses internal API which is not very convenient to review + more subject to break on any internal change. Ideally to have the tests accepting schema.graphql code as a string, parsing it and verifing that it either works correctly, or we get a specific error message 👍
- Fix potential infinite loop in TypeDeclMulti::to_rescript_schema when type declarations have cyclic dependencies. The while loop now detects lack of progress and returns a clear error instead of hanging forever. - Add validation in TypeDeclMulti::new: reject empty declarations and duplicate type names. - Add validation in TypeDecl::new: reject duplicate type parameters. - Add event name uniqueness validation in Contract::new to catch duplicate event names within a contract early. - Add __proto__ field name check in schema validation to prevent prototype pollution issues in the JavaScript runtime. - All new validations include corresponding unit tests. https://claude.ai/code/session_01BwrGigSvKsk4MR43QhkSrx
moose-code
force-pushed
the
claude/code-review-improvements-TdLog
branch
from
ba2b68f to
4259dd6
Compare
![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: 1
🧹 Nitpick comments (1)
packages/cli/src/config_parsing/entity_parsing.rs (1)
174-187: Make__proto__violation output deterministic.
self.entitiesis aHashMap, so the joined violation list can be emitted in varying orders. Sorting keeps error messages stable and avoids flaky snapshots/log comparisons.♻️ Suggested patch
fn check_field_names_for_proto(self) -> anyhow::Result<Self> { let mut violations: Vec<String> = vec![]; for entity in self.entities.values() { for field in &entity.fields { if field.name == "__proto__" { violations.push(format!("{}.{}", entity.name, field.name)); } } } + violations.sort(); + violations.dedup(); if !violations.is_empty() { return Err(anyhow!( "Schema contains fields named '__proto__' which can cause prototype \ pollution issues: {}. Please rename these fields.", violations.join(", ") )); } Ok(self) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/config_parsing/entity_parsing.rs` around lines 174 - 187, The error message can be non-deterministic because self.entities is a HashMap; collect the offending field identifiers into the violations Vec<String> as you do now, then sort the violations (call sort() on violations) before building the anyhow! error string so the joined list is stable; update the code around the violations variable in entity_parsing.rs (the loop over self.entities, the violations Vec, and the Err(...) return) to sort violations prior to violations.join(", ").
🤖 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/src/type_schema.rs`:
- Around line 155-169: The duplicate-parameter check in TypeSchema::new is
case-sensitive, so parameters like "T" and "t" slip through; normalize parameter
names (e.g., to_lowercase()) before inserting into seen_params and building
duplicate_params. Concretely, map parameters to a normalized list (let
normalized = parameters.iter().map(|p| p.to_lowercase()).collect::<Vec<_>>()),
then run the existing seen_params/duplicate_params logic against normalized
instead of the raw parameters; keep the stored parameters unchanged unless you
intend to canonicalize them elsewhere.
---
Nitpick comments:
In `@packages/cli/src/config_parsing/entity_parsing.rs`:
- Around line 174-187: The error message can be non-deterministic because
self.entities is a HashMap; collect the offending field identifiers into the
violations Vec<String> as you do now, then sort the violations (call sort() on
violations) before building the anyhow! error string so the joined list is
stable; update the code around the violations variable in entity_parsing.rs (the
loop over self.entities, the violations Vec, and the Err(...) return) to sort
violations prior to violations.join(", ").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 501db211-08a7-47e8-a851-61acc6327eeb
📒 Files selected for processing (5)
packages/cli/src/config_parsing/entity_parsing.rspackages/cli/src/config_parsing/system_config.rspackages/cli/src/fuel/abi.rspackages/cli/src/hbs_templating/codegen_templates.rspackages/cli/src/type_schema.rs
Comment on lines
+155
to
+169
| pub fn new(name: String, type_expr: TypeExpr, parameters: Vec<String>) -> Result<Self> { | ||
| let mut seen_params: HashSet<String> = HashSet::new(); | ||
| let mut duplicate_params: Vec<String> = vec![]; | ||
| for param in ¶meters { | ||
| if !seen_params.insert(param.clone()) { | ||
| duplicate_params.push(param.clone()); | ||
| } | ||
| } | ||
| if !duplicate_params.is_empty() { | ||
| return Err(anyhow!( | ||
| "Type declaration '{}' has duplicate parameters: {}", | ||
| name, | ||
| duplicate_params.join(", ") | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize generic parameter names before duplicate checks.
Duplicate detection is currently case-sensitive, but parameters are lowercased when rendered (Line 194). Inputs like ["T", "t"] can pass validation and still emit conflicting generics.
🐛 Suggested patch
pub fn new(name: String, type_expr: TypeExpr, parameters: Vec<String>) -> Result<Self> {
let mut seen_params: HashSet<String> = HashSet::new();
let mut duplicate_params: Vec<String> = vec![];
for param in ¶meters {
- if !seen_params.insert(param.clone()) {
+ let normalized = param.to_lowercase();
+ if !seen_params.insert(normalized) {
duplicate_params.push(param.clone());
}
}
if !duplicate_params.is_empty() {
return Err(anyhow!(📝 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.
Suggested change
| pub fn new(name: String, type_expr: TypeExpr, parameters: Vec<String>) -> Result<Self> { | |
| let mut seen_params: HashSet<String> = HashSet::new(); | |
| let mut duplicate_params: Vec<String> = vec![]; | |
| for param in ¶meters { | |
| if !seen_params.insert(param.clone()) { | |
| duplicate_params.push(param.clone()); | |
| } | |
| } | |
| if !duplicate_params.is_empty() { | |
| return Err(anyhow!( | |
| "Type declaration '{}' has duplicate parameters: {}", | |
| name, | |
| duplicate_params.join(", ") | |
| )); | |
| } | |
| pub fn new(name: String, type_expr: TypeExpr, parameters: Vec<String>) -> Result<Self> { | |
| let mut seen_params: HashSet<String> = HashSet::new(); | |
| let mut duplicate_params: Vec<String> = vec![]; | |
| for param in ¶meters { | |
| let normalized = param.to_lowercase(); | |
| if !seen_params.insert(normalized) { | |
| duplicate_params.push(param.clone()); | |
| } | |
| } | |
| if !duplicate_params.is_empty() { | |
| return Err(anyhow!( | |
| "Type declaration '{}' has duplicate parameters: {}", | |
| name, | |
| duplicate_params.join(", ") | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/type_schema.rs` around lines 155 - 169, The
duplicate-parameter check in TypeSchema::new is case-sensitive, so parameters
like "T" and "t" slip through; normalize parameter names (e.g., to_lowercase())
before inserting into seen_params and building duplicate_params. Concretely, map
parameters to a normalized list (let normalized = parameters.iter().map(|p|
p.to_lowercase()).collect::<Vec<_>>()), then run the existing
seen_params/duplicate_params logic against normalized instead of the raw
parameters; keep the stored parameters unchanged unless you intend to
canonicalize them elsewhere.
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.
Summary
This PR adds comprehensive validation to type declarations and schema generation, converting several TODO comments into implemented validation logic. The changes improve error handling by detecting invalid configurations early and providing clear error messages.
Key Changes
TypeDeclMulti validation:
new()to returnResult<Self>instead ofSelfTypeDecl validation:
new()to returnResult<Self>instead ofSelfTopological sort improvements:
to_rescript_schema()to returnResult<String>instead ofStringSchema validation:
check_field_names_for_proto()to detect__proto__field names that could cause prototype pollution issuesContract event validation:
Contract::new()Updated all call sites: Modified all test cases and production code to handle the new
Resultreturn types with.unwrap()or?operators as appropriateNotable Implementation Details
anyhow!()for consistent error handling across the codebasehttps://claude.ai/code/session_01BwrGigSvKsk4MR43QhkSrx
Summary by CodeRabbit
New Features
Bug Fixes