Conversation
|
Caution Review failedThe pull request is closed. WalkthroughActivate lint flow, simplify build output-directory creation and logging, add a JIT symbol registry and reference-to-serializable resolution, introduce a generic JSON exporter, extend surface-rule serialization to resolve references and negation, expose ReferenceNode.ToSerializable, wire project symbols into lang, and update an example surface rules file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as cmd/lint
participant FS as Filesystem
participant Project as project.Builder
participant Lang as lang (JitSymbols)
User->>CLI: mms lint <path>
CLI->>FS: Stat(path)
alt path is file
CLI->>FS: ReadFile(path)
CLI->>Project: AddFile(content)
else path is dir
CLI->>CLI: Log diagnostic (dir case)
end
CLI->>Project: BuildFsLike()
Project->>Lang: lang.JitSymbols = p.Symbols()
Project-->>CLI: build diagnostics
CLI->>CLI: Log diagnostics
sequenceDiagram
autonumber
participant SR as SurfaceRules
participant Reg as lang.JitSymbols
participant Spec as ReferenceNode
participant Export as exporter(...)
SR->>Spec: SerializeConditional(node)
alt node is Reference
SR->>Reg: getSymbolValue(namespace,name)
Reg-->>SR: Serializable value or nil
SR->>SR: wrap resolved value in surface rule structure
else direct node
SR->>SR: serialize node directly
end
SR->>Export: exporter(serializer, rootPath, ext)
Export-->>SR: *lib.FileTreeLike (JSON files)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/build.go (1)
68-72: Critical: Remove stale error check.Lines 69-72 check
errafter callingBuildFsLike, butBuildFsLikedoesn't return an error—only*lib.FileTreeLike. Theerrbeing checked here is stale state from thefs.Statcall on line 48, which is already handled on lines 50-53.This creates two problems:
- The error message "Error exporting project" is misleading—it refers to a stat operation, not export
- If a stat error occurred, execution should have already returned at line 52
Apply this diff to remove the dead error check:
fsLike := project.BuildFsLike(outFile) - if err != nil { - log.Println("Error exporting project:", err) - return - } if err = mkdirIfNotExists(outFile); err != nil {cmd/lint.go (1)
56-81: Remove dead commented code.Lines 56-81 contain a large block of commented-out code that duplicates the active implementation above. Dead code reduces maintainability and should be removed.
Apply this diff:
for _, diag := range project.AllDiagnostics() { log.Println(diag) } - project.BuildFsLike(".") - - // - //stat, err := fs.Stat(os.DirFS("."), inFile) - // - //if err != nil { - // log.Println("Error building project.", err) - // return - //} - //if stat.IsDir() { - // log.Println("Project is a directory") - //} else { - // if content, err := os.ReadFile(inFile); err != nil { - // log.Println("Error building project:") - // } else { - // f := project.AddFile(inFile, string(content)) - // f.Parse() - // } - //} - // - //if len(project.Diagnostics()) > 0 { - // for _, diag := range project.Diagnostics() { - // log.Println(diag) - // } - //} else { - // log.Println("Project has no errors or warnings") - //} - }
🧹 Nitpick comments (1)
cmd/lint.go (1)
54-54: Recommended: Remove unnecessary BuildFsLike call.
BuildFsLikeis invoked but its result is discarded. For a lint command, building a filesystem-like structure is unnecessary—only parsing and diagnostic collection matters.Additionally, this call triggers the side effect of setting
lang.JitSymbols(see project/Project.go line 63), which isn't needed for linting.Apply this diff to remove the unnecessary call:
for _, diag := range project.AllDiagnostics() { log.Println(diag) } - - project.BuildFsLike(".") -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/build.go(1 hunks)cmd/lint.go(2 hunks)examples/test_files/surface_rules.mms(1 hunks)lang/Dimensions.go(1 hunks)lang/Registries.go(1 hunks)lang/SurfaceRules.go(2 hunks)lang/spec/List.go(0 hunks)lang/spec/Reference.go(1 hunks)project/Project.go(2 hunks)
💤 Files with no reviewable changes (1)
- lang/spec/List.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.windsurfrules)
**/*.go: Keep all imports at the top of Go files; never insert imports mid-file
Run gofmt/goimports and go vet before committing or proposing changes
Use Project.Diagnostics() method instead of accessing a Diagnostics field
Files:
lang/spec/Reference.gocmd/build.goproject/Project.golang/Registries.gocmd/lint.golang/SurfaceRules.golang/Dimensions.go
🧬 Code graph analysis (5)
project/Project.go (1)
lang/Registries.go (1)
JitSymbols(12-12)
lang/Registries.go (3)
lang/ast/Namespace.go (1)
Namespace(8-11)lang/ast/Symbol.go (1)
Symbol(48-63)lib/IsNilInterface.go (1)
IsNilInterface(5-11)
cmd/lint.go (1)
project/Project.go (1)
NewProject(18-23)
lang/SurfaceRules.go (6)
lang/spec/List.go (2)
ListNode(119-123)NewListSpec(67-69)lang/ast/Symbol.go (3)
Symbol(48-63)SymbolSurfaceCondition(44-44)SymbolSurfaceRule(43-43)lib/IsNilInterface.go (1)
IsNilInterface(5-11)lang/spec/Reference.go (2)
NewReferenceSpec(28-30)ReferenceNode(142-147)lang/spec/Condition.go (2)
NewConditionSpec(28-33)ConditionalNode(250-257)lang/ast/Namespace.go (1)
Namespace(8-11)
lang/Dimensions.go (2)
wasm/mms.d.ts (1)
FileTreeLike(24-27)lib/FileTreeLike.go (1)
NewDirLike(140-147)
🔇 Additional comments (1)
cmd/build.go (1)
74-76: LGTM: Good refactoring of directory creation.Consolidating the directory existence check into
mkdirIfNotExistsimproves code clarity and reusability.
| if stat.IsDir() { | ||
| log.Println("Project is a directory") | ||
| } else { | ||
| if content, err := os.ReadFile(inFile); err != nil { | ||
| log.Println("Error reading project:", err) | ||
| } else { | ||
| _, err := project.AddFile(inFile, string(content)) | ||
|
|
||
| if err != nil { | ||
| log.Println("Error parsing project:", err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Major: Implement directory traversal for lint.
When stat.IsDir() is true, the code logs "Project is a directory" but doesn't actually process any files within that directory. This means lint is effectively a no-op for directory inputs.
Consider implementing recursive directory traversal:
if stat.IsDir() {
entries, err := os.ReadDir(inFile)
if err != nil {
log.Println("Error reading directory:", err)
return
}
for _, entry := range entries {
if !entry.IsDir() && strings.HasSuffix(entry.Name(), ".mms") {
filePath := path.Join(inFile, entry.Name())
content, err := os.ReadFile(filePath)
if err != nil {
log.Println("Error reading file:", err)
continue
}
_, err = project.AddFile(filePath, string(content))
if err != nil {
log.Println("Error parsing file:", err)
}
}
}
} else {Or use filepath.WalkDir for recursive traversal if subdirectories should also be processed.
🤖 Prompt for AI Agents
In cmd/lint.go around lines 37 to 49 the branch for stat.IsDir() only logs
"Project is a directory" and does not process any files; replace that no-op with
directory traversal that finds and reads .mms files (recursively if desired) and
calls project.AddFile for each file. Use filepath.WalkDir (or os.ReadDir for
non-recursive) to iterate entries, skip directories, filter by ".mms" suffix,
build the full path, read the file contents, handle and log read/parse errors
but continue processing other files, and ensure you pass the correct full file
path and file contents into project.AddFile.
| var val any = nil | ||
| if n.Value != nil && n.Value.ToSerializable() != nil { | ||
| val = n.Value.ToSerializable() | ||
| if !lib.IsNilInterface(n.Value) { | ||
| if ref, ok := n.Value.(*spec.ReferenceNode); ok { | ||
| val = getSymbolValue(ref.Namespace, ref.Name) | ||
| } else if !lib.IsNilInterface(n.Value.ToSerializable()) { | ||
| val = n.Value.ToSerializable() | ||
| } |
There was a problem hiding this comment.
Restore fallback serialization when value references miss the registry.
If getSymbolValue can’t resolve the referenced symbol, val stays nil, dropping the rule body. Previously we still emitted the reference string, so this is a regression. Fall back to ref.ToSerializable() when resolution fails.
Apply this diff:
- if ref, ok := n.Value.(*spec.ReferenceNode); ok {
- val = getSymbolValue(ref.Namespace, ref.Name)
- } else if !lib.IsNilInterface(n.Value.ToSerializable()) {
+ if ref, ok := n.Value.(*spec.ReferenceNode); ok {
+ if resolved := getSymbolValue(ref.Namespace, ref.Name); resolved != nil {
+ val = resolved
+ } else {
+ val = ref.ToSerializable()
+ }
+ } else if !lib.IsNilInterface(n.Value.ToSerializable()) {📝 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.
| var val any = nil | |
| if n.Value != nil && n.Value.ToSerializable() != nil { | |
| val = n.Value.ToSerializable() | |
| if !lib.IsNilInterface(n.Value) { | |
| if ref, ok := n.Value.(*spec.ReferenceNode); ok { | |
| val = getSymbolValue(ref.Namespace, ref.Name) | |
| } else if !lib.IsNilInterface(n.Value.ToSerializable()) { | |
| val = n.Value.ToSerializable() | |
| } | |
| var val any = nil | |
| if !lib.IsNilInterface(n.Value) { | |
| if ref, ok := n.Value.(*spec.ReferenceNode); ok { | |
| if resolved := getSymbolValue(ref.Namespace, ref.Name); resolved != nil { | |
| val = resolved | |
| } else { | |
| val = ref.ToSerializable() | |
| } | |
| } else if !lib.IsNilInterface(n.Value.ToSerializable()) { | |
| val = n.Value.ToSerializable() | |
| } |
🤖 Prompt for AI Agents
In lang/SurfaceRules.go around lines 393 to 399, when handling n.Value as a
*spec.ReferenceNode the code calls getSymbolValue(ref.Namespace, ref.Name) but
if that fails val remains nil and the rule body is dropped; update the branch so
that if getSymbolValue cannot resolve the symbol (i.e., returns nil or not ok),
you fall back to using ref.ToSerializable() as the serialized value; ensure the
existing else-if that uses n.Value.ToSerializable() remains for non-reference
values.
| func (p *Project) BuildFsLike(root string) *lib.FileTreeLike { | ||
| rootDir := lib.NewDirLike(root, nil) | ||
|
|
||
| lang.JitSymbols = p.Symbols() |
There was a problem hiding this comment.
Critical: Avoid global mutable state for symbols.
Setting lang.JitSymbols (a package-level global) introduces several risks:
- Race conditions: Concurrent calls to
BuildFsLikeon different projects will race, corrupting each other's symbol state - Testing difficulties: Global state persists between test cases, causing non-deterministic failures
- Hidden dependencies: Downstream code relying on
JitSymbolsbecomes implicitly coupled toBuildFsLikehaving been called first
Consider refactoring to pass symbols explicitly:
-func (p *Project) BuildFsLike(root string) *lib.FileTreeLike {
+func (p *Project) BuildFsLike(root string) *lib.FileTreeLike {
rootDir := lib.NewDirLike(root, nil)
- lang.JitSymbols = p.Symbols()
-
for ns, decls := range p.symbols {
- nsDir := rootDir.MkDir(ns, nil)
+ nsDir := rootDir.MkDir(ns, nil)
for name, decl := range decls.AllDecls() {
- declFs := decl.ToFileTreeLike(name)
+ declFs := decl.ToFileTreeLike(name, p.Symbols())
if declFs != nil {
nsDir.AddChild(declFs)
}
}
}
return rootDir
}Then update ToFileTreeLike methods to accept symbols as a parameter rather than accessing the global. This eliminates the global state while preserving functionality.
📝 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.
| lang.JitSymbols = p.Symbols() | |
| func (p *Project) BuildFsLike(root string) *lib.FileTreeLike { | |
| rootDir := lib.NewDirLike(root, nil) | |
| for ns, decls := range p.symbols { | |
| nsDir := rootDir.MkDir(ns, nil) | |
| for name, decl := range decls.AllDecls() { | |
| declFs := decl.ToFileTreeLike(name, p.Symbols()) | |
| if declFs != nil { | |
| nsDir.AddChild(declFs) | |
| } | |
| } | |
| } | |
| return rootDir | |
| } |
🤖 Prompt for AI Agents
In project/Project.go around line 63, remove the assignment to the package-level
lang.JitSymbols global and instead have BuildFsLike produce/return the symbols
(or a symbol container) as a value; change all ToFileTreeLike methods to accept
the symbols value as an explicit parameter and update their call sites to pass
the returned symbols through, eliminating reads/writes to lang.JitSymbols;
propagate the new parameter through callers, update tests to
construct/passthrough symbols per-test, and remove or mark lang.JitSymbols
deprecated so no code relies on mutable global state.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Examples