You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Harden the scripting language against inconsistent value conversion, invalid control flow, unbounded recursion, and loss of source diagnostics, while aligning editor analysis with runtime validation.
Validate complete inputs before execution; correct nested return/break/continue propagation and preserve function argument types and local assignments.
Bound parser nesting, expression-tree depth, and active function/script calls; preserve cancellation and unwind scopes/source context.
Preserve source locations, call sites, original failures, and exit-code categories. Incorrect function arity is a usage error.
Keep JSON null, numeric truthiness, string concatenation, and decimal JSON roundtrips consistent across value origins.
Recognize document-local functions and nested commands/options in LSP; fix case-sensitive variable references and hover ranges.
Add a 384-case value-origin matrix covering raw and shell-constructed JSON, plus runtime, process, scope, diagnostics, and editor regression tests.
Document language behavior, limits, and the testing strategy.
Boundaries / Follow-ups
This remains a trusted local shell, not a sandbox for untrusted scripts. Process cancellation for jq and overall execution budgets remain follow-ups. LSP function discovery is document-wide, variable analysis is not runtime-scope-aware, and existing equality semantics remain unchanged.
Serialize shell and MCP execution and invalidate destructive confirmations after context changes. Preserve replayable interactive history while omitting MCP command echo and history entries.
Write exports through temporary files, stop pagination at the requested limit, dispose iterators, and spool CSV exports to disk. Stream CSV imports with CsvHelper and report malformed record line numbers.
Add regression tests and document history policy, context sharing, and export guarantees. Offline validation: 2315 passed, 2 interactive-console tests skipped.
Retain original error states, failure categories, source files and function/script call sites across execution boundaries. Capture function definition sources and include source diagnostics in human, JSON and redacted diagnostic-log output. Reuse runtime control-flow validation in LSP with exclusive-end ranges. Add regression tests and document the diagnostic contract.
Validation: 2399 offline tests passed, 2 interactive-console tests skipped; app and test builds warning-free.
Evaluate the return expression before setting return state, and clear stale return values for bare return. Cover a multi-statement inner function and unreachable code after the outer return.
Validation: 63 focused tests passed; app and test projects built successfully.
Exclude OperationCanceledException from positional wrapping in blocks, loops and exec so host cancellation and timeout handling retain their original exception type. Add deterministic cancellation-during-execution tests across files and function calls, verifying neutral results, cancellation diagnostics and scope/source restoration. Document cancellation semantics.
Validation: 2430 offline tests passed; 2 interactive-console tests skipped. App and test projects built without warnings; git diff --check clean.
Keep a decimal suffix or exponent when serializing shell doubles to JSON scalars, objects, and arrays so integral decimal values do not become integer arithmetic after reconstruction.
Expand the value-origin matrix to cover raw and shell-constructed JSON. Add repeated reconstruction, signed zero, double boundary, and non-finite value tests. Update language documentation and contribution guidance.
Validation: 2889 offline tests passed, 2 interactive tests skipped. Warning-free builds, no editor diagnostics, and clean git diff --check.
The overall line coverage in commit 52e5930 in the dev/mkrueger/script-... branch is 64%. The line coverage in commit c614f52 in the main branch is 63%.
Show a line coverage summary of the most impacted files.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
LSP variable completion still de-duplicates case-insensitively, which breaks the newly documented case-sensitive variable model by hiding distinct names (e.g., $value vs $Value).
Pull request overview
This PR strengthens CosmosDBShell’s scripting runtime and language-server analysis by enforcing full preflight validation before execution, bounding parse/execution resources, preserving source-level diagnostics and exit-code categorization, and aligning value semantics across JSON/shell origins. It also extends import/export operational hardening and adds broad regression coverage plus updated documentation.
Variable completion currently de-duplicates variable names case-insensitively (via seen), which hides distinct variables like $value vs $Value. This conflicts with the new case-sensitive variable semantics and the LSP’s case-sensitive symbol/hover behavior.
Addressed the suppressed Copilot completion finding in ec3e971: variable names are now de-duplicated with StringComparer.Ordinal, preserving distinct case-sensitive names while retaining case-insensitive prefix discovery. A regression checks both labels and insertion text directly. Merge 9c66fec brings forward the two validated MCP/export follow-ups from #207 without duplicating their commits. All nine inline threads have individual replies and are resolved. Validation: 2,894 offline tests passed, two interactive-console skips, 96 focused integration-of-changes tests passed, both projects built without warnings.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The LSP semantic analyzer currently records assignment RHS variable occurrences before the LHS, which can misidentify the “definition” span for self-referential assignments and break hover/go-to-definition accuracy.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
In the LSP semantic analyzer, assignment statements currently analyze the RHS before recording the LHS variable. For a self-referential assignment like $x = $x + 1, this causes the RHS $x to be treated as the variable definition ("first occurrence"), and the LHS becomes a reference, which breaks hover/go-to-definition and reference classification.
Addressed the second review suppressed finding in 4459e40: the semantic analyzer now records the assignment target before visiting the RHS. Self-referential assignments keep their definition at the LHS, and reassignments preserve an existing definition. Added two regressions checking symbol location, reference classification, and lookup at the RHS. All 74 workspace/hover tests pass, and both projects build without warnings. This finding had no inline thread; the nine existing threads remain resolved.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
It introduces broad, cross-cutting runtime + parser + LSP semantic changes where small edge-case regressions could have high user impact despite strong test additions.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Duplicate-parameter validation reports the diagnostic span on the function name token, not on the function definition/parameter list. This makes editor highlighting misleading (the error is about a parameter, but the underline lands on the name). Consider using the full function-definition span until parameter token spans are available.
Structured errors with positional script context are only handled specially in machine mode. In normal output, the earlier RenderUser branch runs first (batch failures have one) and prints only the command's generic failure text, so the PositionalException attached by CommandStatement never surfaces its file/line/call trace. This means a failing batch invoked from a script still loses the human-readable source diagnostics this change promises. Handle positional StructuredErrorCommandState errors in the human path as well while preserving the structured result/renderer.
JSonPathExpression is still omitted from the expression visitor, so variable occurrences such as $source.left produce no variable symbol/reference at all. As a result hover and reference lookup do not benefit from the new case-sensitive handling for a common variable form; record the base variable (excluding the path suffix) when visiting this expression type.
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
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.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Summary
Harden the scripting language against inconsistent value conversion, invalid control flow, unbounded recursion, and loss of source diagnostics, while aligning editor analysis with runtime validation.
Boundaries / Follow-ups
This remains a trusted local shell, not a sandbox for untrusted scripts. Process cancellation for jq and overall execution budgets remain follow-ups. LSP function discovery is document-wide, variable analysis is not runtime-scope-aware, and existing equality semantics remain unchanged.