Support rules with a script node interior type.#29
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for script nodes as an interior node type in the rule parsing and AST building system. Script nodes allow embedding Lua scripts that process inputs from sequences, sets, or PromQL queries.
Changes:
- Added
NodeTypeScriptto the schema and parser/AST infrastructure - Implemented script node parsing with validation for Lua code and input requirements
- Added test cases for script nodes as children of sequences and sets, plus failure cases for scripts at root level and scripts without inputs
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/schema/schema.go | Added NodeTypeScript constant to the node type enumeration |
| pkg/parser/parse.go | Added ParseScriptT struct; removed JSON tags from several structs (breaking change); refactored variable declarations |
| pkg/parser/tree.go | Added script node parsing logic, error definitions, LuaValidator hook, and IsScriptNode() method |
| pkg/testdata/rules.go | Added test data for script nodes including success and failure cases |
| pkg/parser/parse_test.go | Added test cases for script node parsing |
| pkg/ast/ast_script.go | New file implementing AST building for script nodes |
| pkg/ast/ast_machine.go | Added script node case in buildMachineNode (unreachable code) |
| pkg/ast/ast.go | Refactored child building logic to support script nodes; added buildChildrenNodes and buildLeafChild helper functions |
| pkg/ast/ast_test.go | Added recursive gathering of node addresses; added test cases for script nodes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scunningham
force-pushed
the
script
branch
2 times, most recently
from
ed55b5a to
3ad37ed
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Expect first child to be a script definition and second child to be undefined term. | ||
| _, ok := node.Children[0].(*ScriptT) | ||
|
|
There was a problem hiding this comment.
The comment in IsScriptNode is inaccurate/misleading: the second child is expected to be the script input node (a *NodeT), not an “undefined term”. Consider updating the comment and (optionally) tightening the check to also validate node.Children[1] is a *NodeT to avoid false positives if the structure changes.
| // Expect first child to be a script definition and second child to be undefined term. | |
| _, ok := node.Children[0].(*ScriptT) | |
| // Expect first child to be a script definition and second child to be the script input node. | |
| if _, ok := node.Children[0].(*ScriptT); !ok { | |
| return false | |
| } | |
| _, ok := node.Children[1].(*NodeT) |
| switch term.Script.Language { | ||
| case "", "lua": | ||
| if err := LuaValidator(term.Script.Code); err != nil { | ||
| return nil, err | ||
| } | ||
| default: | ||
| return nil, ErrScriptLanguage | ||
| } |
There was a problem hiding this comment.
ErrScriptLanguage is returned directly (not pqerr-wrapped), so callers won’t get rule metadata / YAML position via pqerr.PosOf. Since parse errors elsewhere are generally wrapped, consider creating the script node (or otherwise capturing yn.Line/Column + rule metadata) before language validation and returning a wrapped error (e.g. node.WrapError(ErrScriptLanguage) / pqerr.Wrap(...)).
No description provided.