Feat/integrate debugger

[rvhelden]

The PR for integrating the new nushell debugger protocol into vscode.
With this PR comes all the features of the original repo https://github.com/rvhelden/vscode-nushell-dap

[rvhelden]

@fdncred Here is the PR for the debugger in vscode

[fdncred]

wow, that's a lot of changes. LOL

[fdncred]

Notes from my clanker.

Lint, both typechecks, compile and the 33 unit tests pass on d3b563b. The comments below come from reading through the debugger code. The examples for script-signatures.ts and split-args.ts come from running those helpers under node with the inputs shown.

The resolve.ts change is probably the one to settle first, since it affects language server and terminal users who never open the debugger. Most of the rest is the launch-time signature parsing. With some common signatures it skips the argument prompt when the script needs arguments, and with others it shows the prompt when the script needs none.

[rvhelden]

Also reverted much of the unrelated changes to keep it as small as possible

[fdncred]

let's do one more review to make sure things look in order and look at the ci issue as well.

[fdncred]

Notes from my clanker, second pass at 61f7edb.

Earlier review threads

All nine are addressed. Three of them have a follow-up, posted inline.

• resolve.ts bad setting: the PATH fallback and the warning are back. Follow-up: the warning now fires from every caller, including quietNushell, so one F5 can show it twice.
• script-signatures.ts (three threads: space-separated params, [^\]]* and nested modules, dashed optionals and comments): moving to ast --json fixes all of them, and only top-level defs are read now. Follow-up: a def with an attribute in front (@example, @category, @search-terms) comes back wrapped in AttributeBlock and gets skipped. That brings back the original bug, no prompt and main runs without its argument.
• entry-point.ts relative program: absoluteProgram resolves it. Follow-up: a relative cwd is used as the base for program, but it goes to the adapter unresolved.
• split-args.ts: --name="hello world" x now gives ['--name=hello world', 'x'].
• restart-on-save.ts: only manual saves of the session's program restart now.
• pinned-variable.ts: pinVariable/unpinVariable bump generation. Follow-up (low): the first fetch in visualizeVariable isn't guarded.
• descriptor-factory.ts: throws CancellationError, so only the resolver's message shows.

CI failure

The build failure on 61f7edb has nothing to do with the PR. GitHub builds the Docker image for lannonbr/vsce-action@master before the job's steps run, and its FROM node:20-slim pull got 504 Gateway Timeout from auth.docker.io three times in a row. Checkout, npm install, typecheck:webview, test:unit and vsce package were all skipped, so this head has never had a real CI run. Re-running the job should fix it. Since this comes from a fork, the re-run may need maintainer approval like the earlier action_required run did.

On 61f7edb locally: npm run lint, tsc -p client --noEmit, npm run typecheck:webview, npm run test:unit (35 pass), npm run compile and vsce package all succeed, and the .vsix has out/debug/ir-panel.html, visualize-panel.html and visualize-panel.webview.js.

Optional, and it could be its own PR: the workflow now runs actions/setup-node, so - run: npx @vscode/vsce package could replace lannonbr/vsce-action@master. That drops the Docker Hub pull from every run and stops depending on an unpinned @master.

Minor, no inline comment

• shared/html.ts makeNonce uses Math.random(). crypto.randomBytes(16).toString('base64') is the usual choice for a CSP nonce.
• test:unit passes a glob to node --test, which needs Node 21 or newer. CI pins 22, but CONTRIBUTING doesn't mention it, and on Node 20 the command fails.
• The Show IR button sits in the title bar of every Nushell editor, even with no debug session or with a nu older than 0.116. Adding inDebugMode to its when clause would hide it outside debugging, if that's what you want.
• The comment on LOCALS_REFERENCE in visualize-request.ts points at "the fork's crates/nu-dap/src/state.rs". nu-dap is upstream now, so the comment could point there.
• In restart-on-save.ts, the isNuDocument check is redundant once isSessionProgram matches the path.
• const nushell = async () => ensureNushell(); in extension.ts wraps a sync call in a promise. Meanwhile configuration-provider.ts imports resolveNushell directly instead of using the injected resolver, so the debugger gets nu two different ways.
• On main, the not-found error told the user to run "Nushell: Start Language Server" after installing. The new message dropped that. A setting change restarts the server, but installing nu onto PATH doesn't.

[fdncred]

A def with an attribute in front of it doesn't come back as a Call. Its expr.expr is {"AttributeBlock": {"attributes": [...], "item": {"expr": {"Call": ...}}}}, so field(..., 'Call') is undefined and the def is skipped.

With nu 0.116.2:

@example "greet" { main 1 }
def main [x: int] {}

readDefinitions returns []. No main is found and there are no defs to pick from, so askForEntryPoint returns the config unchanged. main runs without x, which is the failure the earlier signature threads were about. @example, @category and @search-terms are common on def main in newer scripts.

Unwrapping first would cover it:

const expr = field(field(first, 'expr'), 'expr');
const block = field(expr, 'AttributeBlock');
const call = field(block !== undefined ? field(field(block, 'item'), 'expr') : expr, 'Call');

A fixture case for it would be good too.

[fdncred]

slice(1, -1) expects the Signature span to end at ]. When the def declares input/output types, the span covers those as well. With nu 0.116.2:

• def main [x: int]: nothing -> int { $x } gives the signature x: int]: nothing -> in
• a multi-line signature ending ]: nothing -> nothing { gives \n name: string # the name\n]: nothing -> nothin

The prompt then reads Requires arguments: main [x: int]: nothing -> in]. Cutting at the last ] before the : (or rebuilding the label from required_positional names) would fix it.

[fdncred]

The fallback is back, thanks. But the warning now lives inside resolveNushell, so every caller shows it, including quietNushell in configuration-provider.ts, whose comment says it doesn't report. With a stale setting, one F5 calls resolveNushell from quietNushell and again from ensureNushell in the descriptor factory, and the same warning comes up twice. Opening a terminal and every change to the setting each show it again.

source on the found result (and the NushellSource type) is never read. That looks like the place this was meant to go. Have resolveNushell return something like { kind: 'found', path, fellBackFrom?: configured } without side effects, and let ensureNushell decide whether to warn. Then quietNushell really is quiet.

[fdncred]

This runs on every change to nushellExecutablePath. The Settings UI writes the value as you type, so typing /usr/local/bin/nu produces several changes with partial paths. Each one runs which, shows the "not found ... Falling back" warning, and stops and restarts the language server on PATH nu.

It also starts the server when the user had stopped it with "Nushell: Stop Language Server", since restartLanguageServer calls startLanguageServer either way.

Two overlapping events can also race. The second handler's stopLanguageServer returns right away because client is already cleared, so it starts a client. The first handler then calls startLanguageServer and shows "Nushell Language Server is already running."

Restarting only when client was running, debouncing a bit, and skipping when the resolved path hasn't changed would handle all three.

[fdncred]

A relative cwd gets resolved against the workspace folder here, but only to build program. config.cwd itself goes to the adapter unchanged, and DebugAdapterExecutable is created without a cwd option, so nu --dap resolves a relative cwd against the extension host's working directory. "cwd": "scripts" would then set $env.PWD to something unrelated to the workspace.

Setting config.cwd = path.resolve(folder.uri.fsPath, config.cwd) here (when there's a folder) would keep the two consistent.

[fdncred]

args: [] counts as "args given", so the prompt is skipped. The debug example in the README includes "args": [], so anyone who copies it gets no prompt for def main [name: string], and main fails on the missing argument.

Treating an empty array the same as a missing one (if (Array.isArray(config.args) && config.args.length > 0)) matches what people mean by []. If you want a way to force no prompt, removing args from the README example is the other option.

[fdncred]

Low priority. VS Code calls resolveDebugConfigurationWithSubstitutedVariables before createDebugAdapterDescriptor, so on a nu older than 0.116 the user picks an entry point and types arguments, and only then gets "Debugging is disabled because it needs Nushell 0.116.0". Running the version check in the configuration provider, before resolveEntryPoint, would stop the launch before any prompts.

[fdncred]

Low priority, same kind of race as the generation thread. This fetch has no guard. Visualize $big (slow), then $small (fast) before the first answer comes back. $small paints, then $big arrives and paints over it, and trackLive pins $big. Taking a generation number before the await and dropping a stale answer would handle it. pinned-variable.ts could export a helper for that.

[fdncred]

Low priority. Nothing listens for error on stdin. If nu exits before it reads the whole script, the write fails with EPIPE and Node raises it as an uncaught exception in the extension host. The try/catch around this doesn't see it. I reproduced it by passing /usr/bin/true as nu with a 5 MB script. readDefinitions still returns undefined, but the uncaught EPIPE shows up too. A real nu only hits this if it dies early. parse.child.stdin?.on('error', () => {}) before end is enough.

[fdncred]

Nushell has no name=value syntax for positionals. Typing this at the prompt passes the literal string name=world as name, and count=3 fails to parse as an int. Something like # Try: world 3 --verbose --tag=release would work.