Make run/list language-aware and fix Windows run-command quoting#106
Conversation
Two gaps found while testing the language PRs (#100-#105): - discover_scripts() only globbed .py, so 'reverse-api-engineer run' and 'list' reported 'No Python scripts found' for every non-Python client, and the run path always executed the shared-venv Python interpreter. Discovery now covers all supported extensions (single source of truth in utils.OUTPUT_LANGUAGE_EXTENSIONS, shared with BaseEngineer), excludes build dirs and the vendored cJSON sources, and the run command dispatches per language - node/npx tsx/go run/mvn exec:exec/ dotnet run/php/ruby, plus compile-then-execute for C - with a clear error when the required tool is missing from PATH. - The Java/C#/PHP/Ruby/C run commands quoted paths with POSIX-only shlex.quote, which cmd.exe/PowerShell parse incorrectly for paths with spaces. BaseEngineer._quote_path() now uses subprocess.list2cmdline on Windows and shlex.quote elsewhere. Also asks the C partial for -Wall -Wextra-clean code (generated client had unused-parameter warnings in live testing). Verified live: run --json executed real generated Go/PHP/C/C#/Ruby clients from earlier agent sessions end-to-end (C compiling first).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if sys.platform == "win32": | ||
| return subprocess.list2cmdline([str(path)]) |
There was a problem hiding this comment.
list2cmdline() applies Windows C-runtime/cmd.exe quoting, but the returned text is interpolated into a shell command that may run in PowerShell. A valid path containing PowerShell metacharacters such as $ or a backtick can therefore be expanded or escaped, causing the generated run command to use the wrong path or fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/reverse_api/base_engineer.py
Line: 485-486
Comment:
**PowerShell Paths Remain Unsafe**
`list2cmdline()` applies Windows C-runtime/cmd.exe quoting, but the returned text is interpolated into a shell command that may run in PowerShell. A valid path containing PowerShell metacharacters such as `$` or a backtick can therefore be expanded or escaped, causing the generated run command to use the wrong path or fail.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Acknowledged — this is a real boundary, but not one that quoting can close: PowerShell expands
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6544087f2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if f.is_file() and f.suffix == ".py" and f.name not in exclude_files: | ||
| if f.is_file() and f.suffix in SCRIPT_EXTENSIONS and f.name not in exclude_files: | ||
| scripts.append(f) | ||
| scripts = [s for s in scripts if not any(part in exclude_dirs for part in s.parts)] |
There was a problem hiding this comment.
When a custom output directory contains one of these newly added names, this filters out every generated client because s.parts includes the entire path, not just children below the scripts directory; for example output_dir=/tmp/target makes /tmp/target/scripts/<run>/api_client.go look like it is inside an excluded build dir and run/--ls report no scripts. Since this function only iterates direct children of scripts_dir, compare relative parts or remove this filter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 21fc967 — reproduced with output_dir=/tmp/target/... returning zero scripts. Since iterdir() doesn't recurse and non-files are already skipped, the parts-based filter could only ever misfire on parent components, so it's removed entirely (with a comment explaining why no explicit build-dir filter is needed). Regression test added.
| ValueError: For unsupported extensions, or script_args with a Java | ||
| client (mvn exec's program arguments are fixed in the pom). | ||
| """ | ||
| d = script.parent |
There was a problem hiding this comment.
If the configured output_dir is relative, discover_scripts() returns a relative script, but both non-Python runners execute the argv with cwd=script.parent; e.g. out/scripts/<run>/api_client.js is then resolved by node from out/scripts/<run> as out/scripts/<run>/out/scripts/<run>/api_client.js, so every non-Python client fails to start. Build commands from script.resolve() or pass only the filename once the cwd is changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 21fc967 — build_script_commands() now resolves the script path before building argv, so commands stay absolute regardless of a relative output_dir, and cwd=script.parent can no longer re-anchor them. Verified live with a relative path + node subprocess, plus a regression test.
- The missing-toolchain message contained 'not found', which _classify_error's heuristics remap to engine_failure, overriding the config_invalid hint. Reworded so the intended kind survives. - Unit tests for _quote_path on both platforms (list2cmdline quoting on win32, shlex on POSIX) and for spaced paths surviving intact in the dispatch argv lists. Live matrix via the CLI: go/php/c/csharp/ruby clients from the language-PR test sessions plus hand-written js/ts clients all ran with exit 0, including --file selection, --ls listing, args passthrough (node), the Java script-args misuse error, and the missing-tool config_invalid error under a stripped PATH.
…s, document quoting boundary - discover_scripts filtered on every component of the full path, so a custom output_dir under a directory named like a build dir (e.g. /tmp/target/...) excluded all scripts. iterdir() doesn't recurse and non-files are already skipped, so the parts-based filter could only misfire - removed it. - build_script_commands now resolves the script path before building argv: callers run with cwd=script.parent, which would re-anchor a relative path (from a relative output_dir) and fail to start. - _quote_path docstring now states the Windows quoting boundary precisely: list2cmdline is cmd.exe/CreateProcess-safe; PowerShell additionally expands $/backtick inside double quotes and no quoting satisfies both shells for such paths.
1 participant
Fixes the outstanding issues found while testing the six output-language PRs (#100–#105).
1.
run/listwere Python-only (found on #100, affected every language)discover_scripts()only globbed.py, soreverse-api-engineer run <run_id>failed with "No Python scripts found" for JS/TS/Go/Java/C#/PHP/Ruby/C clients, and the run path always executed the shared-venv Python interpreter.utils.OUTPUT_LANGUAGE_EXTENSIONSas the single source of truth (BaseEngineerreuses it), and excludes build dirs (node_modules,bin,obj,target) and the vendoredcJSON.c/cJSON.h.build_script_commands()dispatches per language:node/npx tsx/go run/mvn -q -f pom.xml compile exec:exec/dotnet run --project/php/ruby, and compile-then-execute for C. Missing toolchains produce a clear error naming the required binary instead of a crash. Java rejects extra script args explicitly (mvn exec's program args are fixed in the pom); C# passes them after--.--json/--json-stream) and interactive run paths use the dispatch; the Python venv flow is unchanged.2. Windows quoting (flagged on #101–#105)
The Java/C#/PHP/Ruby/C run commands quoted paths with POSIX-only
shlex.quote, which cmd.exe/PowerShell parse incorrectly for paths containing spaces.BaseEngineer._quote_path()now usessubprocess.list2cmdlineon Windows andshlex.quoteelsewhere.3. C compiler warnings (found on #105, minor)
The C partial now asks for
-Wall -Wextra-clean code — the live-tested client had unused-parameter warnings.Testing
run --jsonexecuted the Go, PHP, C (compile step included), C#, and Ruby clients end-to-end with exit 0. (The Java run surfaced a runtime fragility in that one generated artifact's second example call — the dispatch itself correctly ran Maven and propagated the exit code.)🤖 Generated with Claude Code
https://claude.ai/code/session_01KpXzPMFT1X8YCZQcxaFYVE
Summary by cubic
Make
runandlistwork across all output languages and fix Windows path quoting so commands run reliably. Discovery is now robust across directories, and commands use absolute paths to run correctly from any working directory.cJSON.*,__init__.py). It no longer misfires when parent paths contain build-dir names;utils.OUTPUT_LANGUAGE_EXTENSIONSis the single source of truth.node,npx tsx,go run,mvn -q -f pom.xml compile exec:exec,dotnet run --project,php,ruby; C compiles then runs). Commands use absolute paths and run withcwd=script.parent. Clear missing-tool errors; in machine mode they areconfig_invalid. Java rejects extra args; C# forwards args after--.subprocess.list2cmdlineon Windows andshlex.quoteelsewhere. Documented PowerShell$/backtick limits; cmd.exe-safe quoting is the baseline.Written for commit 21fc967. Summary will update on new commits.
Greptile Summary
This PR makes script discovery and execution aware of every supported output language. The main changes are:
Confidence Score: 5/5
The changed flow looks mergeable after a small PowerShell quoting cleanup.
src/reverse_api/base_engineer.py
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Make run/list language-aware and quote r..." | Re-trigger Greptile