feat: support empty Java action arguments#336
Conversation
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove. The PR thoroughly implements the ellipsis placeholder feature following all project guidelines, with proper documentation, test coverage, and full-stack implementation. The minor concern about visitor logic is likely addressed in the generated or omitted code sections given the passing tests and explicit documentation of the AST behavior. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Symptom: microflow Java action parameters with an empty BasicCodeActionParameterValue could not be represented in MDL, so describe/exec round-trips had to either invent a value or fail to parse the generated source. Root cause: the expression grammar had no placeholder expression that could carry an intentionally unbound code-action argument through the parser and builder. Fix: add an ellipsis source expression, emit it for `...`, preserve it through expression formatting, and map it back to an empty BasicCodeActionParameterValue for Java action arguments. Tests: added parser/visitor coverage, builder coverage for placeholder Java action arguments, an MDL doctype fixture, and validation docs for the new syntax.
Symptom: Java actions with microflow-typed parameters roundtripped placeholder arguments as basic empty expressions, which Studio Pro reported as stale Java action arguments. Root cause: the SDK/parser/writer did not model Microflows$MicroflowParameterValue, and the builder treated all ellipsis placeholders as BasicCodeActionParameterValue. Fix: add MicroflowParameterValue support end-to-end and emit it for Java action parameters whose definition is MicroflowType, keeping ellipsis as an empty microflow reference. Tests: added builder and parser regressions and ran make test.
hjotha
force-pushed
the
submit/microflow-ellipsis-placeholder-expression
branch
from
b8ee049 to
a8dee83
Compare
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove the PR. The implementation correctly addresses the need to preserve round-trip symmetry for unbound Java action parameters, follows all required architectural patterns, includes comprehensive tests, and provides appropriate documentation. The minor concerns about LSP/VS Code extension are unlikely to impact functionality given the nature of the feature as an expression placeholder that doesn't introduce new language constructs requiring special editor support. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
ako
left a comment
ako
left a comment
There was a problem hiding this comment.
Why not empty?
The ... token solves a real problem, but MDL already has the right tool for this: the empty keyword.
empty is already:
- A lexer token (
EMPTY: E M P T YinMDLLexer.g4) - A valid
literalinMDLParser.g4— parseable as an expression anywhere a literal is accepted, including call arguments - A
LiteralEmptyAST kind inmdl/ast/ast_expression.go - Already handled by the executor —
expressionToString()returns"empty"forLiteralEmpty, andvalidate_microflow.goalready treats it as a null-like sentinel
So the builder could simply check if lit.Kind == ast.LiteralEmpty instead of isPlaceholderExpression(...), and describe would emit:
$Total = call java action SampleModule.Recalculate(
CompanyId = empty,
RecalculateAll = true,
ItemList = empty
);
This is already valid MDL, already human-readable, and consistent with how = empty is used for uninitialized list variables (declare $Items list of Module.Entity = empty).
The ... choice instead introduced a new lexer token, regenerated the entire ANTLR parser (3000+ lines of churn in mdl_lexer.go / mdl_parser.go), and added syntax with no precedent in MDL — all to solve a problem the existing keyword already covers. CompanyId = ... reads as broken/incomplete code; CompanyId = empty reads as intentional.
Suggested change: drop ELLIPSIS from the grammar, remove the isPlaceholderExpression helper, and map LiteralEmpty → BasicCodeActionParameterValue { Argument: "" } in addCallJavaActionAction. The doctype example, skill docs, and quick-reference entry would all stay — just s/.../empty/.
Symptom: review pointed out that the ellipsis placeholder introduced a new MDL token for a Java-action argument case that the existing `empty` literal already expresses more clearly. Root cause: the feature modeled unbound code-action parameters as a new `...` expression, which created parser churn and diverged from normal MDL expression conventions. Fix: remove the ELLIPSIS token and parser alternative, map `empty` literals in Java-action arguments to empty BasicCodeActionParameterValue/MicroflowParameterValue bindings, and update formatter output, docs, proposal, skill text, and examples. Tests: regenerate the parser, run focused grammar/visitor/executor tests, then make build and make test.
Symptom: the empty Java action argument example used a .test.mdl suffix, so TestMxCheck_DoctypeScripts skipped it. Root cause: doctype CI only discovers plain .mdl files under mdl-examples/doctype-tests. Fix: rename the fixture to .mdl without changing its contents. Tests: mxcli check on the fixture plus make build and make test.
…psis-placeholder-expression # Conflicts: # docs/11-proposals/README.md # mdl/executor/cmd_microflows_builder_calls.go # sdk/mpr/parser_microflow_test.go
AI Code ReviewWe are reviewing a pull request that adds support for the The PR includes:
We must review against the provided checklist. Let's break down the review by section: Overlap & duplication
MDL syntax designThe PR does not add new MDL syntax (like new keywords or statements). It modifies the interpretation of an existing literal ( We must check if the use of
Full-stack consistencyThe PR adds a new feature (the use of
Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
The implementation already uses the existing `empty`/`null` literals for unbound Java action arguments after review feedback rejected a dedicated ellipsis placeholder token. The remaining helper name still used the old placeholder terminology, which made the branch look like it still introduced new syntax. Rename the helper to the Java-action-specific behavior it implements and fix the proposal example path to the actual .mdl fixture name. Tests: - go test ./mdl/executor -run 'TestBuildJavaAction_Empty|TestBuildJavaAction' -count=1 - make build - ./bin/mxcli check mdl-examples/doctype-tests/empty_java_action_argument.mdl - make test
hjotha
changed the title
AI Code ReviewCritical Issues
Moderate Issues
Minor Issues
What Looks Good
Recommendation Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
…psis-placeholder-expression # Conflicts: # sdk/mpr/parser_microflow_test.go
AI Code ReviewWe are reviewing a pull request that adds support for the
Let's go through the checklist: Overlap & duplication
MDL syntax design
But note: the PR documentation states: So the meaning of However, looking at the existing code, the The design guidelines for MDL syntax (in .claude/skills/design-mdl-syntax.md) might have something to say about this. But note that the PR is not introducing a new keyword, it is reusing an existing one. We must check if this overloading is acceptable. The PR argues that it is clearer than introducing a new placeholder token. Since the PR is not adding new syntax (no changes to the grammar), we don't have to update the grammar, AST, visitor, etc. for the syntax itself. However, we do have changes in the executor to handle the The PR does update the quick reference to document this usage. Let's check the quick reference change: it adds a row under "Call from microflow" for "Empty argument". This is acceptable. The PR also updates the write-microflows skill to document the usage. We note that the PR does not change the grammar, so we don't have to check for: However, note that the PR does add a test in the visitor to ensure that the parser accepts Full-stack consistencySince the syntax (the The PR includes: We note that the PR does not add LSP wiring or DESCRIBE roundtrip specifically for this feature? But note: So the DESCRIBE roundtrip is implicitly covered by the formatter changes (which output We see that the PR adds a test in the formatter for formatting an empty Java-action argument (in Also, the PR adds a doctype test file and tests in the executor builder. Therefore, the full stack is covered. Test coverage
We note that the PR does not add any integration tests beyond the unit tests and the doctype test (which is tested via However, the doctype test is in the correct place and is tested. Security & robustness
Scope & atomicity
Code quality
However, we note that the PR does not update any manually maintained lists that might be a maintenance risk? We don't see any such lists in the changes. The design documents (skills, quick reference) are updated to match the implementation. Bugs & correctness
We note that the visitor test expects the But note: in the MDL lexer, both Actually, in the MDL lexer, Looking at the visitor test, it is checking for In the builder test, we see: And in the visitor test, we see: This seems inconsistent. However, note that the visitor test is checking that the parser accepts The PR documentation says: And then in the visitor test, it uses both Let me read the visitor test again: And then it checks that the two empty arguments (CompanyId and ItemList) are of kind This suggests that in the parser, both But wait, the builder test uses This discrepancy might be a bug? Or perhaps the lexer returns the same token for We don't have the lexer in the diff, but note that the PR does not change the lexer. However, the PR does not need to change the lexer because the We must check: in the existing code, how are Without the lexer, we cannot be sure. But note that the PR is not changing the lexer, so we assume that the existing representation is correct. However, the visitor test expects But note: the PR documentation says it uses the The visitor test uses Let's look at the commit that added the Given that the PR is not changing the lexer, and the existing code already handles Therefore, the visitor test is incorrect because it expects However, note that the PR says: And the So the PR author might have intended that in the Java-action argument context, both But the PR documentation only mentions We see that the helper function So the executor treats both as empty. Therefore, the visitor test should accept either? But the visitor test input uses The visitor test input uses the word The test then checks for This is a bug in the test. However, note that the PR might be relying on the fact that the lexer returns the same token for Since we are not changing the lexer, and the PR does not show lexer changes, we must assume that the lexer produces different tokens for Therefore, the visitor test is wrong. But wait: the PR also adds a test in the builder that uses And the formatter test uses an empty string in the BSON and expects the formatter to output So the core functionality is correct. The visitor test is the only place that seems to have an issue. However, note that the visitor test is in the same PR and is testing the parser's acceptance of the We must flag this as a bug in the test. Alternatively, if the lexer does return the same token for Given the information we have, we note the inconsistency and suggest that the visitor test should be fixed to check for However, since we are reviewing the PR and the PR author might have intended to treat But note: the PR description says: "Uses the existing We see that the builder test uses Therefore, the visitor test should be changed to expect However, we are not the author and we are reviewing. We must decide if this is a blocker. Given that the core functionality (executor, formatter, SDK) is tested and works, and the visitor test is only testing the parser's acceptance (which we know works from the builder test), we might consider this a minor issue. But note: the visitor test is failing as written? We don't know because we don't have the lexer. However, if the lexer produces We must assume that the lexer does produce Therefore, the visitor test is incorrect and should be fixed. However, note that the PR also tests The PR could add a separate test for Given the above, we will note the visitor test issue as a minor issue. Alternatively, we can note that the PR does not change the lexer and the existing representation of But since we are not making the change, we will flag it. Summary of findings:
However, note that the PR might have intended to treat We'll consider the visitor test issue as a minor issue that should be fixed, but it does not block the approval because the core functionality is correct and the test is only testing the parser's acceptance (which we know works from the builder test). Alternatively, we can note that the PR author might have a reason (maybe the lexer returns the same token for Since we don't have the lexer, we cannot be sure. But note that the PR does not change the lexer, so the lexer behavior is unchanged. We will assume that the lexer returns different tokens for However, to be safe, we note that the PR has tested the functionality via the builder and formatter tests, and the doctype test. We will recommend that the visitor test be fixed, but we can approve the PR with a note to fix the test. But note: the PR is already written and we are reviewing. We can either request a change to the test or note it as a minor issue and approve. Given that the PR has extensive test coverage elsewhere, we can approve and note the issue in the review. However, the checklist says we should flag issues. We will flag the visitor test as a minor issue. Let's structure the review: Critical Issues: None What Looks Good: Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
PR #336 Review — feat: support empty Java action argumentsGood update. The One implementation detail worth documenting, and one missing test. Minor
In the microflow expression visitor both No writer unit test for
Otherwise looks good to merge. |
3 participants
Summary
Uses the existing
emptyliteral as the MDL representation for intentionally unbound Java-action parameters. This letsdescribeoutput preserve emptyBasicCodeActionParameterValuebindings and letsexecwrite them back without inventing synthetic expression values or adding a new placeholder token.Part of #332.
Changes
empty/nullback to empty code-action parameter values.MicroflowParameterValuereferences.Validation
go test ./mdl/executor -run 'TestBuildJavaAction_Empty|TestBuildJavaAction' -count=1make build./bin/mxcli check mdl-examples/doctype-tests/empty_java_action_argument.mdlmake test