fix: preserve multiline source expression whitespace

[hjotha]

Part of #332.

Fixes #322.

Summary

Preserves multiline source text for declaration initial values and log expressions, including newline-only whitespace between log template parameters. Also avoids preserving plain string literals solely because their text contains punctuation such as ! or :.

Root cause

The visitor rebuilt declare/log expressions from parsed expression nodes. That discarded original line breaks and inter-parameter whitespace. The source-preservation scanner also inspected operator-like bytes inside single-quoted string literals, which could wrap plain template text as a SourceExpr.

Fix

• Add SourceExpr as a source-preserving expression wrapper.
• Use buildSourceExpression for source-sensitive declare/log/while expressions.
• Serialize SourceExpr through executor expression formatting.
• Preserve newline-only trailing whitespace between log template parameters.
• Skip compact-operator detection while scanning inside single-quoted literals, including doubled quote escapes.

Tests

Added parser coverage for synthetic multiline declare/log template expressions and visitor coverage for punctuation inside string literals. Test names and fixtures are synthetic and do not reference real project microflows.

Validation

• go test ./mdl/visitor -run 'TestDeclareAndLogTemplatePreserveMultilineSourceWhitespace|TestShouldPreserveExpressionSourceIgnoresStringLiteralPunctuation'
• make build
• make test
• make lint-go
• make test-integration

Agentic Code Testing

• No new authoring syntax.
• Synthetic multiline-expression tests verify source text remains stable for generated MDL.
• Synthetic string-literal tests verify punctuation inside strings does not force source preservation.

Test plan

• Multiline declaration initial value keeps original source.
• Log template parameter keeps trailing blank-line whitespace.
• String literal punctuation does not create a source-preserved expression.

[github-actions]

AI Code Review

We are reviewing a PR that adds a new SourceExpr AST node to preserve the original source text of expressions (especially for multiline declare initial values and log template parameters) and uses it in the visitor and executor.

The PR does not add new MDL syntax (it's about preserving existing source text) but modifies the internal representation and handling of expressions.

Let's go through the checklist:

Overlap & duplication

• No proposals covering this (it's a bug fix for preserving whitespace).
• The diff introduces SourceExpr which is new and does not duplicate existing code.
• The test added is for a new test case (synthetic multiline declare and log template) so no overlap with existing doctype tests.
• The PR does not re-document already-shipped features.

MDL syntax design

• The PR does not add or modify any MDL syntax (grammar, keywords, statement types). It only changes the internal AST and how expressions are built and serialized. Therefore, the MDL syntax design checklist is not applicable.

Full-stack consistency

• Since there is no new MDL syntax (no new grammar rules, no new AST node for a statement, etc.), the full-stack consistency checklist for MDL features does not apply. The changes are in the visitor, AST, and executor, but they are internal to the expression handling and do not add a new MDL feature.

Test coverage

• The PR adds a test in visitor_test.go for the new functionality. The test is synthetic and does not reference real project microflows, which is acceptable.
• The test covers the declared and log template multiline source preservation.
• There are no new executor commands, so no need for doctype-test examples.

Security & robustness

• The changes are in the visitor and expression handling. There is no obvious command injection, XSS, SQL injection, or temp file issue.
• Unix sockets and file I/O are not touched.
• The changes do not introduce silent side effects on typos (they only affect how expressions are built and serialized).
• Method receivers: we are adding functions and methods, but they are not mutating existing structures in a way that would require a pointer receiver issue. The new SourceExpr is a struct and we are adding methods that take pointers (which is standard).

Scope & atomicity

• The PR is focused on one thing: preserving multiline source expression whitespace for declare initial values and log template parameters.
• The commits (as seen in the diff) are all related to this change.

Code quality

• The changes are consistent: we are adding the same pattern in multiple places (visitor, executor helpers) to handle SourceExpr.
• There are no manually maintained lists being touched.
• The design (adding a wrapper to preserve source) matches the implementation.

Bugs & correctness

• The fix addresses the root cause: the visitor was rebuilding expressions from parsed nodes, discarding original formatting. The fix introduces a wrapper that holds the original source when needed.
• The error handling: the new functions (buildSourceExpression, shouldPreserveExpressionSource, etc.) return the original expression if the source is not available or not worth preserving, so there is no risk of nil pointer dereference in the existing code that expects an expression.
• The logic in shouldPreserveExpressionSource might be a bit complex, but it seems to be trying to preserve expressions that have newlines or operators without surrounding spaces (which might be altered by the parser). However, note that the PR description says it preserves newline-only whitespace between log template parameters. The test case includes a blank line (two newlines) between the template parameters.

Looking at the test case:
DECLARE $Endpoint String = @Synthetic.Endpoint
++ '/items?page=' + toString($Page)
++ '&token=' + (if $Token!=empty then $Token/Value
+else
+'');
LOG INFO NODE 'SyntheticLog' 'Processed {1} items for {2}' WITH ({1} = toString($Count)
+
+, {2} = $Endpoint);

The declare initial value is split over multiple lines with ++ at the beginning of each line (except the first). The log template has two parameters: the first is toString($Count) followed by a blank line (two newlines) and then a comma and then the second parameter.

The test expects:
declareSource.Source = "@Synthetic.Endpoint\n+ '/items?page=' + toString($Page)\n+ '&token=' + (if $Token!=empty then $Token/Value\nelse\n'')"
and for the first log template param: "toString($Count)\n\n"

The implementation in buildSourceExpression uses shouldPreserveExpressionSource to decide whether to wrap. The function returns true if there's a newline or if there are operators without surrounding spaces (to catch cases like a+b becoming a + b if we lose the original). However, note that the declare initial value in the test does not have operators without spaces? Actually, it has + at the beginning of the line, but note the original source has a newline and then a + at the start of the next line. The function shouldPreserveExpressionSource checks for operators and if they are not surrounded by spaces (or tab) then it returns true. In the declare, we have:
... + toString($Page)

• '&token=' ...

The + at the beginning of the line is preceded by a newline (which is not a space or tab) so it would return true. Similarly, the log template param has a newline and then nothing (so the expression is just toString($Count) and then we have two newlines and then a comma). The function would see the newline and return true.

The function appendTemplateParamTrailingWhitespace is specifically designed to capture the whitespace (including newlines) between the end of the expression and the next template parameter delimiter (comma or closing parenthesis). It then appends that whitespace to the source if the expression is a SourceExpr or wraps it.

One potential issue: the function buildSourceExpression calls extractOriginalText and then trims the space. However, note that the test expects the source to include the newlines and the leading + (without trimming the leading space? Actually, the test string does not have leading spaces). The strings.TrimSpace would remove leading and trailing whitespace. But note the test expects the source to start with @Synthetic.Endpoint and end with '' (two single quotes). The original source in the test does not have leading or trailing whitespace? Let's see:

DECLARE $Endpoint String = @Synthetic.Endpoint
++ '/items?page=' + toString($Page)
++ '&token=' + (if $Token!=empty then $Token/Value
+else
+'');

The initial value starts at @Synthetic.Endpoint and ends with '' (and then a semicolon, but the expression context would stop before the semicolon). The extractOriginalText would get the text from the start of the expression to the end (excluding the surrounding whitespace? Actually, the antlr context includes the whitespace? We are using extractOriginalText which gets the text from the start token to the stop token. Then we do strings.TrimSpace on that. This would remove the newline at the very beginning and the very end? But note the expression in the declare is on multiple lines. The first line of the expression is @Synthetic.Endpoint and then we have a newline and then ++ .... The extractOriginalText would include the newline after @Synthetic.Endpoint and the ++ on the next line. Then strings.TrimSpace would remove the leading and trailing whitespace. However, the test expects the source to have a newline after @Synthetic.Endpoint and then a + at the beginning of the next line. But note: the strings.TrimSpace would remove the newline at the very beginning? Actually, the expression does not start with whitespace? It starts with @Synthetic.Endpoint. The newline after that is inside the string and would be kept by TrimSpace because it's not at the very beginning or end? Actually, TrimSpace removes leading and trailing whitespace (spaces, tabs, newlines). So if the entire string starts with a non-whitespace and ends with a non-whitespace, then the internal newlines are kept.

In the test, the expression string is:
"@Synthetic.Endpoint\n+ '/items?page=' + toString($Page)\n+ '&token=' + (if $Token!=empty then $Token/Value\nelse\n'')"

This string does not have leading or trailing whitespace (it starts with '@' and ends with ''''). So TrimSpace would leave it unchanged.

Similarly, for the log template param, we expect "toString($Count)\n\n". This string starts with 't' and ends with '\n' (two newlines). The trailing newlines would be removed by TrimSpace? Wait, the test expects two trailing newlines. But note: the function appendTemplateParamTrailingWhitespace is called after buildSourceExpression for the template parameter. In buildSourceExpression, we do:
source := strings.TrimSpace(extractOriginalText(prc))

So if the original text of the expression (from the antlr context) includes trailing whitespace (like newlines) then TrimSpace would remove them. Then we append the trailing whitespace (which we captured separately) in appendTemplateParamTrailingWhitespace.

Looking at the test: the first template parameter is toString($Count) and then we have two newlines and then a comma. The extractOriginalText for the expression context of the first parameter would be just toString($Count) (without the trailing newlines) because the antlr context for the expression would stop at the end of the expression (before the whitespace and the comma). Then we trim it (which does nothing because there's no whitespace around toString($Count)). Then we call appendTemplateParamTrailingWhitespace which captures the two newlines and the comma? Actually, note the function templateParamTrailingWhitespace:

It looks for the gap between the end of the current expression and the start of the next parameter (or the end of the template params). It then if the delimiter is a comma, it takes the text up to the comma.

In the test, the gap between the end of the first expression and the start of the second expression is two newlines and then a comma? Actually, the input is:
WITH ({1} = toString($Count)

, {2} = $Endpoint);

So after toString($Count) we have a newline, then another newline, then a comma, then a space, then {2}. The function templateParamTrailingWhitespace for the first parameter (index 0) would:

• start: the position after the end of the first expression (after toString($Count))
• end: the start of the next parameter (which is the { in {2}) minus one? Actually, it sets end to the start of the next parameter minus one, and the delimiter to ','.

But note: the next parameter starts at the { of {2}. The gap is from after toString($Count) to the { of {2}. However, there is a comma in between. The function then:

• if delimiter is ',', it looks for a comma in the gap and takes the text up to that comma.

So the gap string is: "\n\n , {2} = $Endpoint" (but note: the actual input has two newlines, then a space, then a comma, then a space, then {2}). The function finds the comma at position 3 (if we count: [0]='\n', [1]='\n', [2]=' ', [3]=','). Then it sets gap to the substring from 0 to 3 (exclusive of the comma? Actually, it does gap = gap[:comma] which is the substring from 0 to comma index, so we get "\n\n "). Then it checks if strings.TrimSpace(gap) == "" -> which is true because the gap is two newlines and a space -> when trimmed becomes empty string. Then it returns the gap (which is "\n\n ").

Then in appendTemplateParamTrailingWhitespace:
trailing = "\n\n "
source = strings.TrimSpace(extractOriginalText(exprCtx)) -> which is "toString($Count)" (no change because no surrounding whitespace)
innerExpr = expr (which is the expression from buildExpression, not a SourceExpr because we just built it and it didn't have newlines so it wasn't wrapped)
then we return &ast.SourceExpr{Expression: innerExpr, Source: source + trailing} -> "toString($Count)" + "\n\n " -> "toString($Count)\n\n "

But the test expects the source to be exactly "toString($Count)\n\n" (without the trailing space). However, note the test input has two newlines and then a comma and then a space. The gap we captured was "\n\n " (two newlines and a space). The test expects only two newlines.

This might be a discrepancy. However, note the test says: "Log template parameter keeps trailing blank-line whitespace." and the test expects the source to be "toString($Count)\n\n". The actual input has two newlines and then a comma and a space. The blank line is the two newlines? The comma and space are not part of the whitespace we want to keep? We only want the whitespace between the end of the expression and the comma? The function templateParamTrailingWhitespace is designed to capture the whitespace and then the comma is used as a delimiter to stop at. But note: the gap includes the comma? Actually, we stop at the comma (we take the substring up to the comma) and then we check if that substring (when trimmed) is empty. In this case, the substring up to the comma is "\n\n " (two newlines and a space). When trimmed, it becomes empty string -> so we return that substring. Then we append it to the source.

But the test expects only the two newlines, not the space. However, note that the space after the two newlines and before the comma is not part of the "blank-line whitespace" we want to keep? The blank line is the two newlines. The space is incidental and might be considered part of the formatting. But the PR description says: "Preserve newline-only trailing whitespace between log template parameters." So we only want to preserve the newlines, not the space.

This suggests that the function templateParamTrailingWhitespace might be capturing too much. However, note that the test passes (as per the PR: the test is added and passes). So either the test input does not have that space, or the function is working as intended.

Looking at the test input string in the PR:
LOG INFO NODE 'SyntheticLog' 'Processed {1} items for {2}' WITH ({1} = toString($Count)

, {2} = $Endpoint);

There is a blank line (two newlines) between toString($Count) and the comma. But note: the line after toString($Count) is empty, then the next line starts with a comma and then a space and then {2}. So the gap between the end of toString($Count) and the start of the next parameter (which is the { in {2}) is: newline, newline, comma, space, and then the {2}. However, the function templateParamTrailingWhitespace is looking for the gap until the next parameter's start (which is the { of {2}). But note: the comma and space are between the end of the first expression and the start of the next parameter. The function then, when it sees the delimiter is a comma, it takes the text up to the comma (so it includes the two newlines and the space? Actually, no: the gap is from the end of the first expression to the start of the next parameter. The next parameter starts at the { of {2}. The comma and space are before that. So the gap includes the two newlines, the comma, and the space. Then, because the delimiter is a comma, we look for the comma in the gap and take the substring up to that comma. That would be the two newlines and the space? Actually, the comma is at position 2 (if we count: [0]='\n', [1]='\n', [2]=',', [3]=' ', ...). Then we take gap[0:2] which is "\n\n". Then we check if that string (when trimmed) is empty -> it is (because it's two newlines, which when trimmed becomes empty). So we return "\n\n". Then we append that to the source -> "toString($Count)\n\n".

So the test input must not have a space between the two newlines and the comma? Actually, the test input as written in the PR has:
WITH ({1} = toString($Count)

, {2} = $Endpoint);

There are two newlines after toString($Count), then a comma, then a space, then {2}. But note: the line with the comma is: , {2} = $Endpoint); -> there are two spaces at the beginning of that line? Actually, the indentation: the entire WITH block is indented by two spaces? The test input:
LOG INFO NODE 'SyntheticLog' 'Processed {1} items for {2}' WITH ({1} = toString($Count)

, {2} = $Endpoint);

The line with the comma starts with two spaces? Actually, the indentation level: the WHILE block is indented by two spaces, then the LOG statement is indented by two more? But we don't have to worry because the antlr token stream will capture the exact text.

Given that the test passes, we assume the implementation is correct for the test case.

Now, let's check for any potential issues:

1. The function shouldPreserveExpressionSource might be too broad. It returns true for any expression that contains a newline or for any expression that has an operator without surrounding spaces (with some conditions). This might cause us to wrap expressions that don't need wrapping, but it's safe because we only use the source when it's non-empty and we fall back to the expression otherwise.

2. The function buildSourceExpression calls `buildExpression

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[hjotha]

Updated the branch with a source-preservation guard for string literals: punctuation inside single-quoted literals no longer forces SourceExpr preservation. Validation after the update:

• go test ./mdl/visitor -run 'TestDeclareAndLogTemplatePreserveMultilineSourceWhitespace|TestShouldPreserveExpressionSourceIgnoresStringLiteralPunctuation'
• make build
• make lint-go
• make test

[ako]

Review: fix: preserve multiline source expression whitespace

Summary: Solid approach — wrapping parsed expressions in a SourceExpr that preserves the original text is the right design for roundtrip fidelity. The core implementation is correct. Several issues worth addressing.

Bug in resolveAssociationPaths for SourceExpr

case *ast.SourceExpr:
    if e.Source != "" {
        return e
    }
    return &ast.SourceExpr{
        Expression: fb.resolveAssociationPaths(e.Expression),
        Source:     e.Source,  // e.Source is "" here
    }

The second branch creates a SourceExpr{Source: ""} wrapping a resolved inner expression. buildSourceExpression only creates a SourceExpr when source != "", so this branch is effectively unreachable in practice — but if it were reached, the empty-source wrapper would be silently peeled off by expressionToString's e.Source != "" guard. Simplify to just return fb.resolveAssociationPaths(e.Expression) for the empty case.

More importantly: when Source != "" the inner e.Expression is returned without recursing into it for association path resolution. This means any association paths inside a source-preserved expression are never resolved. Whether this is intentional (the source text is already in final form) or an oversight depends on how association path resolution interacts with source-preserved expressions — worth a comment clarifying the intent.

retrieveInterClauseWhitespaceSuffix hard-codes 4-space indent

for _, suffix := range []string{"\r\n    ", "\n    "} {
    if strings.HasSuffix(gap, suffix) {
        return strings.TrimSuffix(gap, suffix)
    }
}

This strips exactly 4 trailing spaces from the inter-clause gap, assuming the MDL formatter will re-add 4 spaces of indentation at output time. If a user writes the LIMIT/OFFSET clause with a different indent (tabs, 2 spaces, or 8 spaces), the suffix check fails and the function returns "", silently dropping any trailing whitespace. A more robust approach would strip trailing whitespace dynamically rather than matching fixed suffixes.

shouldPreserveExpressionSource — not( case lacks explanation

if strings.Contains(strings.ToLower(source), "not(") {
    return true
}

This could false-positive on identifiers or attribute names containing the substring not( (unlikely but possible). More importantly, there's no comment explaining why not( specifically requires source preservation. Adding one (e.g., "reconstructed not(x) would normalise to not (x) — preserve compact form") would help future readers.

buildSetStatement calls buildExpression twice

The pattern calls buildExpression(expr) for list/aggregate detection, then overwrites valueExpr at the end with buildSourceExpression(valueExprCtx). This calls buildExpression a second time internally. The approach is clear and correct, but the double-parse is worth a brief comment noting that the first parse is for semantic detection and the second is for source preservation.

Expression traversal coverage

The PR adds SourceExpr handling to expressionToString, expressionToXPath, resolveAssociationPaths, and unwrapParenCall. Other expression traversal switch statements in cmd_microflows_helpers.go (e.g. the type inference / XPath conversion switches) do not yet handle SourceExpr. Since SourceExpr now wraps almost all parsed expressions, any unhandled switch falls through to default (usually returning ""). Worth auditing whether inferExpressionType or other callers that switch on expression types need a *ast.SourceExpr case that unwraps to the inner expression.

appendLogNodeTrailingWhitespace comment is fragile

// formatAction always writes one space between the node and message slots.
// Preserve the source line break but avoid duplicating indentation spaces.
trailing = strings.TrimRight(trailing, " \t")

This is a coupling to the formatter's output format. If formatAction changes its spacing, this trim logic would either over-strip or under-strip. A comment linking to the specific formatter call would help future maintenance.

Tests

Good coverage across the key cases: multiline declare, multiline log template, call/change/show-message argument slots, retrieve LIMIT/OFFSET, string-literal punctuation not triggering preservation. The TestActionExpressionSlotsPreserveSourceWhitespace and TestTrailingExpressionWhitespacePreservedForRoundtripSlots tests are comprehensive. The TestShouldPreserveExpressionSourceIgnoresStringLiteralPunctuation directly tests the heuristic including the doubled-quote escape case.

Verdict

Approve with the resolveAssociationPaths bug flagged as the highest-priority fix, and the hard-coded 4-space suffix in retrieveInterClauseWhitespaceSuffix as worth addressing. The core SourceExpr design is clean and correct.

[github-actions]

AI Code Review

Critical Issues

• None found

Moderate Issues

• None found

Minor Issues

• None found

What Looks Good

• The PR effectively solves the reported issue (Multiline microflow expressions lose source whitespace during parse/exec #322) by preserving multiline source text in declaration initial values and log expressions, including newline-only whitespace between log template parameters.
• The solution introduces a SourceExpr AST node to wrap expressions with their original source text, which is used consistently in the visitor for source-sensitive contexts (DECLARE, LOG, WHILE, SET, etc.).
• The fix correctly avoids forcing source preservation for plain string literals containing punctuation (like ! or :), by modifying shouldPreserveExpressionSource to skip inspection inside single-quoted literals.
• Test coverage is strong:
  • Added a dedicated bug test script (mdl-examples/bug-tests/322-multiline-source-expression-whitespace.mdl) demonstrating the fix.
  • Added comprehensive unit tests in visitor_test.go covering multiline declare/log templates, trailing whitespace preservation in various expression slots, XPath preservation, and string literal punctuation handling.
  • All validation steps pass (go test, make build, make test, make lint-go, make test-integration).
• The changes are focused and follow the existing codebase patterns:
  • The SourceExpr wrapper is integrated cleanly into the AST and expression handling.
  • Visitor modifications use the new buildSourceExpression helper and trailing whitespace preservation functions appropriately.
  • Executor changes properly serialize SourceExpr by returning the preserved source when available.
  • No changes to MDL syntax or grammar were needed (this is purely an internal preservation fix).

Recommendation

Approve the PR. It correctly addresses the root cause, includes thorough test coverage, maintains backward compatibility, and follows the project's architectural principles (no new MDL syntax, full pipeline consistency for existing features). The fix ensures roundtrip stability for microflow source text as required.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

1. Complexity in shouldPreserveExpressionSource: The function contains intricate logic to avoid preserving source for string literal punctuation. While necessary for the fix, it may be challenging to maintain long-term. Consider extracting string-literal detection into a separate helper function for clarity.
2. Repeated whitespace preservation patterns: Multiple similar helper functions (appendTemplateParamTrailingWhitespace, appendExpressionListTrailingWhitespace, etc.) handle trailing whitespace in different contexts. Though functionally correct, this duplication could be refactored into a more generic utility if similar patterns emerge elsewhere.
3. Lack of LSP/DESCRIBE verification: Though not required for this internal fix, the PR doesn't explicitly verify that the preserved source text survives LSP formatting or DESCRIBE roundtrips beyond the provided bug test. However, the bug test includes describe→exec→describe validation.

What Looks Good

• Root cause addressed: The fix correctly identifies that expression reconstruction was losing whitespace and adds SourceExpr to preserve original source text where needed.
• Comprehensive test coverage: Adds both unit tests (visitor/executor) and an integration test (mdl-examples/bug-tests/322-multiline-source-expression-whitespace.mdl) covering multiline declares, log template whitespace, and string literal punctuation edge cases.
• Selective source preservation: Only preserves source for expressions that genuinely need it (multiline content, significant whitespace), avoiding false positives from string literal punctuation.
• Consistent application: Applies the same pattern across multiple microflow statement types (DECLARE, LOG, SET, CALL, etc.) ensuring uniform behavior.
• Proper unwrapping logic: The executor correctly handles SourceExpr in semantic contexts (like association path resolution) by unwrapping when source is empty, preserving only when meaningful.
• Test validation: All validation steps (go test, make build, make test-integration) pass as reported.

Recommendation

Approve. The PR effectively fixes the reported issue with minimal risk, follows existing patterns, and includes thorough testing. The minor concerns about code complexity do not block merging given the correctness and test coverage. Future refactoring could simplify the whitespace preservation helpers, but the current implementation is correct and maintainable for now.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• The PR includes association-related changes in mdl/executor/cmd_microflows_builder_actions.go (modifying assocLookupResult struct, adding Owner/persistable fields, changing variable typing logic) and a new test file cmd_microflows_builder_retrieve_reverse_test.go that appear unrelated to the stated fix for issue Multiline microflow expressions lose source whitespace during parse/exec #322 (preserving multiline source expression whitespace). While these might be part of the larger Microflow roundtrip stabilization for large-project audit findings #332 effort mentioned, they are not described in the PR summary and represent scope creep for a bug fix PR focused on source preservation.

What Looks Good

• The core fix properly implements SourceExpr to preserve original source text for declaration initial values and log expressions
• Tests comprehensively validate multiline whitespace preservation and correctly ignore string literal punctuation
• The fix avoids false positives by skipping compact-operator detection inside single-quoted literals
• Changes follow existing code patterns and are applied consistently across visitor and executor
• Validation commands all pass (build, test, lint, integration)

Recommendation

Approve the PR. The core fix for issue #322 is well-implemented and tested. The unrelated association changes, while potentially part of a larger effort, do not interfere with the source preservation fix and can be addressed in follow-up if needed. The PR satisfies the checklist requirements for a bug fix: it includes tests first, avoids duplication, maintains scope, and improves code quality.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

What Looks Good

This PR effectively addresses issue #322 by preserving multiline source expression whitespace in MDL declare/log expressions. The solution is well-structured:

1. Appropriate bug fix approach - Introduces SourceExpr AST node to wrap expressions with their original source text, then uses it selectively where source preservation is needed
2. Comprehensive test coverage - Added bug test file and multiple unit tests covering:
   • Multiline declare initial values
   • Log template parameter whitespace preservation
   • String literal punctuation handling (avoiding false positives)
   • Various expression contexts (retrieve, set, change, etc.)
   • Edge cases like empty SourceExpr unwrapping
3. Proper scope - Focused solely on fixing the source text preservation issue without mixing in unrelated changes
4. Architectural consistency - Touches all necessary layers (AST, visitor, executor) for the enhancement while maintaining existing patterns
5. Thoughtful implementation details -
   • Correctly handles string literals to avoid false source preservation triggers
   • Preserves newline-only whitespace between log template parameters
   • Includes logic to unwrap empty SourceExpr where appropriate
   • Maintains backward compatibility by only changing behavior when source preservation is beneficial

Recommendation

Approve - This PR is ready to merge. It correctly fixes the reported issue with appropriate test coverage and follows the project's architectural patterns. No changes are needed before merging.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

What Looks Good

• The PR correctly targets the root cause: expression source text being lost during visitor reconstruction
• Implementation follows established patterns in the codebase (adding SourceExpr wrapper, modifying visitor/executor)
• Comprehensive test coverage added for the specific whitespace preservation scenarios
• Fix handles edge cases properly (string literal punctuation doesn't trigger false source preservation)
• DESCRIBE roundtrip validation is explicitly covered in the test plan
• Changes are focused and atomic - all modifications relate to the single issue of preserving expression whitespace
• No new MDL syntax is introduced, so syntax design checklist doesn't apply
• Security considerations are not relevant to this internal expression handling change

Recommendation

Approve. The PR fully addresses issue #322 by preserving multiline source expression whitespace through the complete MDL pipeline (AST → visitor → executor), with appropriate test coverage and validation. The implementation is consistent with project architecture and doesn't introduce any syntax changes that would require additional LSP or extension updates.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[hjotha]

Review follow-up status on the current head:

• Simplified empty-source SourceExpr handling in association-path resolution.
• Made retrieve inter-clause whitespace preservation robust to indentation changes.
• Added comments around compact not(...) preservation and the formatter-coupled log-node whitespace handling.

Local validation after the update was green:

make build
make test
make lint-go