feat: AST flow inspection, parameter prompts, and Copilot follow-ups#25
feat: AST flow inspection, parameter prompts, and Copilot follow-ups#25Sahilbhatane wants to merge 9 commits intoouterbounds:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a single-pass Python AST inspection pipeline to reliably detect Metaflow flow files and extract Parameter(...) metadata before running/spinning flows, plus updates the extension UX (prompts + safer command construction) and expands test coverage.
Changes:
- Introduces shared Python AST utilities and a new
inspectFlowFile.pyJSON interface (syntaxOk,hasFlowSpec,parameters,error). - Updates the VS Code extension to gate run/spin on inspection results, prompt for parameters, and build shell-quoted commands (including hyphenated flag names).
- Adds Node tests and Python fixtures covering inspection, parameter extraction (including
AnnAssign), and command quoting/validation.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/metaflow_flow_ast.py |
Shared AST analysis: FlowSpec detection + Parameter(...) extraction. |
scripts/inspectFlowFile.py |
New JSON-emitting inspector script for the extension/tests. |
scripts/extractFlowParameters.py |
Refactors extractor to reuse shared AST helpers. |
parameterPrompt.js |
Uses inspector output for parameters and adds prompting/validation flow. |
pythonRunner.js |
Adds PYTHON / python→python3 fallback and sync runner for tests. |
flowDetection.js |
Centralizes editor validation + “inspection allows run” predicates/errors. |
buildRunCommand.js |
Updates flag-name validation (allows hyphens) and quoting utilities. |
extension.js |
Wires inspection + prompts into run/spin; adds error surfacing around command build/send. |
test/buildRunCommand.test.js |
Tests quoting, shell detection, CLI name validation, and command building. |
test/extractParameters.test.js |
Tests AST parameter extraction behavior including annotated assignment. |
test/inspectFlowFile.test.js |
Tests unified inspection JSON output and parity with extractor output. |
test/flowDetection.test.js |
Tests flowInspectAllowsRun predicate behavior. |
test_fixtures/sample_flow.py |
Fixture: typical FlowSpec with multiple parameters. |
test_fixtures/not_a_flow.py |
Fixture: non-flow file. |
test_fixtures/no_params_flow.py |
Fixture: flow with no parameters. |
test_fixtures/syntax_error_flow.py |
Fixture: invalid Python syntax for inspector error path. |
test_fixtures/ann_assign_flow.py |
Fixture: annotated assignment parameter extraction. |
examples/hello_flow.py |
Manual testing example for prompts + spin. |
README.md |
Documents FlowSpec validation, smart prompts, limitations, and test command. |
package.json |
Adds npm test script using Node’s built-in test runner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const shell = getShellType(); | ||
| const terminal = getOrCreateTerminal(); | ||
| terminal.show(); | ||
| terminal.sendText(`cd ${quoteForTerminal(fileDir, shell)}`); |
There was a problem hiding this comment.
On Windows when the integrated terminal shell is cmd.exe, cd will not switch drives (e.g. C: -> D:) unless /d is used. This can cause the subsequent python ... command to run in the wrong directory for flows located on a different drive. Consider emitting cd /d <dir> for shell === 'cmd' (or separately sending a <drive>: command before cd).
| terminal.sendText(`cd ${quoteForTerminal(fileDir, shell)}`); | |
| const cdCommand = | |
| shell === 'cmd' | |
| ? `cd /d ${quoteForTerminal(fileDir, shell)}` | |
| : `cd ${quoteForTerminal(fileDir, shell)}`; | |
| terminal.sendText(cdCommand); |
| if (flowInspectAllowsRun(inspectResult)) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
reportFlowInspectFailure can throw when called with null/undefined because it unconditionally reads inspectResult.syntaxOk/hasFlowSpec after flowInspectAllowsRun returns false. Since flowInspectAllowsRun explicitly accepts falsy inputs (and tests cover null), add an early guard like “if (!inspectResult) { showErrorMessage(...); return true; }” to keep the helper total-order safe.
| if (!inspectResult) { | |
| vscode.window.showErrorMessage( | |
| 'Flow inspection failed.' | |
| ); | |
| return true; | |
| } |
| execFilePythonChain( | ||
| [INSPECT_SCRIPT, filePath], | ||
| { timeout: 10000 }, | ||
| (err, stdout, stderr) => { | ||
| if (err) { | ||
| console.error('inspectFlowFile failed:', stderr || err.message); | ||
| resolve({ | ||
| syntaxOk: false, | ||
| hasFlowSpec: false, | ||
| parameters: [], | ||
| error: (stderr && String(stderr).trim()) || err.message || 'Failed to inspect flow file.', | ||
| }); |
There was a problem hiding this comment.
execFilePythonChain is invoked without an encoding, so Node will return stdout/stderr as Buffers; this relies on implicit string coercion in JSON.parse and error formatting. Pass encoding: 'utf8' in the options (or make execFilePythonChain default an encoding) so parsing/logging is deterministic and matches the function’s JSDoc types.
| /** | ||
| * Run a Python binary with args; on missing interpreter, try next candidate. | ||
| * @param {string[]} args | ||
| * @param {import('child_process').ExecFileOptionsWithStringEncoding} opts | ||
| * @param {(err: Error | null, stdout: string, stderr: string) => void} callback | ||
| */ | ||
| function execFilePythonChain(args, opts, callback) { | ||
| const chain = pythonCandidates(); | ||
| function attempt(i) { | ||
| if (i >= chain.length) { | ||
| callback( | ||
| new Error( | ||
| 'No Python interpreter found. Install Python or set the PYTHON environment variable.' | ||
| ), | ||
| '', | ||
| '' | ||
| ); | ||
| return; | ||
| } | ||
| execFile(chain[i], args, opts, (err, stdout, stderr) => { | ||
| if (err && isMissingInterpreterError(err)) { | ||
| attempt(i + 1); | ||
| return; | ||
| } | ||
| callback(err, stdout, stderr); | ||
| }); |
There was a problem hiding this comment.
The JSDoc for execFilePythonChain types opts as ExecFileOptionsWithStringEncoding and the callback stdout/stderr as strings, but the function doesn't enforce/provide a default encoding. This mismatch can lead to callers unexpectedly receiving Buffers. Either loosen the types to allow Buffer outputs or set a default encoding inside execFilePythonChain when none is provided.
Implements Improve Flow File Detection Before Command Execution (#22) and aligns with the attached plan: one Python AST pass via
inspectFlowFile.py/metaflow_flow_ast.pyto enforce valid syntax, aFlowSpecsubclass, andParameter(...)extraction before run and spin. Smart parameter prompts and shell-safepython … run --…building remain as in PR #23.What changed
scripts/metaflow_flow_ast.py– Shared AST:analyze_flow_source, FlowSpec detection,Parameterextraction (includingast.AnnAssign).scripts/inspectFlowFile.py– Single JSON result for the extension (syntaxOk,hasFlowSpec,parameters,error).parameterPrompt.js+pythonRunner.js– Run inspect withPYTHONorpython→python3fallback; prompts preserve submitted values (empty strings and spaces) for flags.flowDetection.js– Editor/language checks +reportFlowInspectFailure(JSDoc clarified).buildRunCommand.js– CLI flag names allow hyphenated Click-style options; still reject=, spaces, etc.extension.js–try/catcharoundbuildRunCommand+ terminal send so failures surface as VS Code errors.execFilePythonSyncfor CI/Linux; fixtureann_assign_flow.py; updated CLI name tests.How to test
npm testexamples/hello_flow.py— Ctrl+Alt+R / Ctrl+Alt+SFollow-ups (unchanged)
IncludeFileparameters (README limitations)