Repository navigation
[code-quality] Report missing command errors - #399
Pedro Henrique Penna (ppenna) merged 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused change preserves existing command-exit handling and adds regression coverage, with no identified blocking issues.
Review effort: Balanced
Findings: None
What changed in this PR
Makes subprocess startup failures use the repository-standard ScriptError instead of exposing raw OSError tracebacks.
Changes:
- Translates startup errors while preserving the original exception as the cause.
- Adds a regression test for a missing executable.
| File | Description |
|---|---|
scripts/test_nvx_tools.py |
Tests missing-executable error reporting. |
scripts/nvx_tools/common.py |
Wraps subprocess startup errors in ScriptError. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3da1ff5 to
7e9997c
Compare
7e9997c to
1dff06c
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes preserve existing exit-status behavior, include regression coverage, and have no identified blocking issues.
0 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
run_checked and run_capture are the shared subprocess helpers of the
build, CI, source-collection, and packaging commands. run_checked
reports a command that exits nonzero with a ScriptError that names the
command, and run_capture returns the exit status for require_success
to report. Neither handled a command that cannot start at all. For
such a command, subprocess.run raises OSError, for example when the
executable is missing or is not executable or when the working
directory does not exist, and the OSError escaped both helpers.
Callers therefore received a raw OSError instead of the ScriptError of
every other command failure, and the error's text alone does not
identify the command. nvx.py printed that text as the whole error, so
a missing git during materialize-kernel-provenance-inputs read
error: [Errno 2] No such file or directory: 'git'
on Linux and, because the Windows error names no file,
error: [WinError 2] The system cannot find the file specified
on Windows.
Both helpers now raise a ScriptError that names the full command and
the OS error, with the OSError chained as its cause:
error: failed to run command git ls-tree -r -z --name-only HEAD -- kernel/config-microvm kernel/patches: [WinError 2] The system cannot find the file specified
A command that starts is reported as before, whatever its exit status.
A regression test runs both helpers on a missing executable and checks
the message and the chained FileNotFoundError.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bff752cc-6f55-46e9-bdd1-8d680ce8d103
1dff06c to
87406b0
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused change preserves completed-command behavior, includes regression coverage, and has no identified blocking issues.
0 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Summary
common.run_checkedandcommon.run_captureare the shared subprocess helpers of the build, CI, source-collection, and packaging commands. When a command cannot start (its executable is missing or not executable, or its working directory does not exist),subprocess.runraisesOSError. That error escaped both helpers instead of becoming the repository-standardScriptError, and its text alone does not identify the command. For example,nvx.pyreported a missinggitduringmaterialize-kernel-provenance-inputsas:Both helpers now raise a
ScriptErrorthat names the full command and the OS error, with theOSErrorchained as its cause:A command that starts is reported as before, whatever its exit status. A regression test runs both helpers on a missing executable and checks the message and the chained
FileNotFoundError. Without thecommon.pychange, the test fails for both helpers.This is a novel candidate: recent code-quality PRs #361, #343, and #310 addressed managed-exec manifests, Alpine package manifests, and benchmark float validation respectively; active PRs #362 and #365 concern package-lock hashing and path-existence reuse, not process-start errors. No tracked issue or active PR covers this shared command boundary.
Scope
Changed files:
scripts/nvx_tools/common.pyscripts/test_nvx_tools.pyTotal changed lines: 29 added, 6 deleted. No dependency, public API/CLI/ABI, gitlink, or OpenVMM change was made.
Validation
All runs used
7e9997condevatd68859d.Local (Windows):
python -m unittest scripts.test_nvx_tools.SharedCommandTests -v: passed.validate-nvxcommand set (compileall, the nine unittest suites with 741 tests,test_adversarial.py,test_hosts.py, and the CLI--helpchecks): passed.test_windows_curl_shim_retries_http_500needsrustconPATH; it passed once a toolchain was provided.ruff check,ruff format --check,pyright --pythonplatform Linux,pyright --pythonplatform Windows, andgit diff --check: passed.Remote runners: each used an isolated depth-1 checkout that mirrors CI (NVX
d68859dplus the bundled7e9997c, OpenVMMa41cf73). The checkouts were removed afterwards.validate-nvxnvx.py verifygit(before → after)azure-kvm-1linux-kvm--helpOK[Errno 2] No such file or directory: 'git'→failed to run command git ls-tree … : [Errno 2] …azure-azlinux-1linux-mshv--helpOKazure-kvm-1azure-windows-1windows-whp--helpOK[WinError 2] The system cannot find the file specified→failed to run command git ls-tree … : [WinError 2] …On each runner, both helpers were also run against real processes:
ScriptError, with theOSErrorsubclass as the cause.command failed with exit 3: …), andrun_capturereturning status 3 behave as before.gitavailable,materialize-kernel-provenance-inputssucceeds.