Repository navigation
docs(skill-inspector): document the Windows stdout form for scan reports - #1017
bharathvemula wants to merge 3 commits into
Conversation
Fixes NVIDIA#1015: step 2's --output command always fails on Windows because safe file output is unsupported on that platform (exit code 2, no report written). Label the existing command as POSIX and add the PowerShell stdout-redirect form, noting that a non-zero exit from --output on Windows is a platform refusal with no partial report to inspect. AI-assistance disclosure: this change was prepared with AI assistance. Signed-off-by: Satya Sai Bharath Vemula <vemula.sai8@gmail.com>
yashrajp22
left a comment
There was a problem hiding this comment.
One Windows report-path issue remains in the new example. This is a source review with exact-comparison Greptile adjudication; native Windows/PowerShell execution, CLI/scanner tests, and source/wheel verification were not performed.
| rather than UTF-16: | ||
|
|
||
| ```powershell | ||
| skillspector scan "$TARGET" --no-llm --format json | Set-Content -Encoding utf8 "$env:TEMP\skill-inspector-report.json" |
There was a problem hiding this comment.
Could you pass the report destination with -LiteralPath? If $env:TEMP contains square brackets, such as C:\Temp[1], this quoted positional argument still binds to wildcard-aware -Path. The scan can finish successfully but the report is not saved in the intended directory (or is written to another matching path), leaving step 3 without the expected report. Set-Content -Encoding utf8 -LiteralPath "$env:TEMP\skill-inspector-report.json" handles these valid Windows paths in both PowerShell 5.1 and newer versions. See the Set-Content parameter contract.
A quoted positional path still binds to wildcard-aware -Path, so a $env:TEMP containing square brackets (e.g. C:\Temp[1]) can send the report elsewhere. -LiteralPath handles these valid Windows paths in both PowerShell 5.1 and newer versions. Addresses review feedback on NVIDIA#1017.
A quoted positional path still binds to wildcard-aware -Path, so a $env:TEMP containing square brackets (e.g. C:\Temp[1]) can send the report elsewhere. -LiteralPath handles these valid Windows paths in both PowerShell 5.1 and newer versions. Addresses review feedback on NVIDIA#1017. Signed-off-by: Bharath Vemula <vemula.sai8@gmail.com>
e93ceca to
e554641
Compare
|
@yashrajp22 — the |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Decision: Approve.
Value and readiness
The Windows alternative correctly avoids the secure file-output platform refusal and gives the agent a report path to inspect. The current example includes -LiteralPath, addressing the earlier review's valid TEMP-path wildcard concern.
Material findings
No remaining material source blocker found in the documentation change. skills/skill-inspector/SKILL.md:45-55 separates the POSIX file-output command from the Windows stdout pipeline, selects UTF-8 encoding, and explains why a refused --output call leaves no partial report. The current target-branch file_output.py:30-45 confirms the fail-closed platform contract. The other reviewer's Changes Requested state and now-outdated unresolved thread still require their resolution before merge.
PIC tradeoffs
None identified. The manual stdout path follows the documented platform escape hatch. PowerShell 5.1 writes a UTF-8 BOM with this encoding, as #1015 already records; consumers must accept that encoding when parsing the resulting report. Broader per-target filename changes are outside this focused fix.
Verification and gaps
Read the complete one-file diff and pinned skill, linked #1015, all comments/reviews and thread state. Checked the current target-branch secure-output implementation because the docs branch's older exact head predates that module. All six attached checks report success in run 42165271519. No native Windows or PowerShell execution was performed.
Static source review only. No contributor code, tests, snippets, imports, builds, setup, hooks, scanner invocations or model calls were executed.
Reviewed head: e554641f0380f077de99777cdbde948d6eccfe01.
|
Tested the proposed command on Windows PowerShell 5.1 (5.1.26100, the default
A form that is BOM-free on both 5.1 and 7+, tested here (first bytes [IO.File]::WriteAllText("$env:TEMP\skill-inspector-report.json", ((skillspector scan "$TARGET" --no-llm --format json) -join "`n"))( |
Set-Content -Encoding utf8 writes a UTF-8 BOM on PowerShell 5.1, so the report fails to parse in Python and Node. Use the BOM-free [IO.File]::WriteAllText(...) form tested by KAYOKG on PowerShell 5.1 (first bytes 7B 0A, parses in Python and Node), and note -Encoding utf8NoBOM as a PowerShell 6+ alternative. Signed-off-by: Bharath Vemula <vemula.sai8@gmail.com>
|
@yashrajp22 — the BOM issue raised by @KAYOKG is addressed: the documented Windows form now uses the BOM-free |
Fixes #1015.
Step 2 of the bundled skill-inspector skill told every platform to run
skillspector scan "$TARGET" --no-llm --format json --output /tmp/skill-inspector-report.json. On Windows that command always fails (exit code 2) because safe file output is unsupported there (src/skillspector/file_output.pyfails closed whenO_NOFOLLOW/O_DIRECTORY/dir_fdsupport is absent), and no report is ever written — so an agent following step 2 literally looks for a file that was never created.This change:
--outputcommand as the POSIX form.--outputand redirect stdout to a file outside the scan target with the BOM-free[IO.File]::WriteAllText(...)form (plain>writes UTF-16 JSON on PowerShell 5.1, which JSON parsers reject, andSet-Content -Encoding utf8writes a UTF-8 BOM on PowerShell 5.1, which Python and Node JSON parsers also reject). TheWriteAllTextform is BOM-free on both PowerShell 5.1 and 7+ (tested by KAYOKG;-Encoding utf8NoBOMis a PowerShell 6+ alternative).--outputcommand on Windows is a platform refusal, not a scan failure: no partial report exists, so the agent should not look for one or retry the same command.Deliberately out of scope: the issue's secondary suggestion (per-target report filenames) — the issue itself marks it lower-confidence, and it may already be covered by #984.
Verification: docs-only change to
skills/skill-inspector/SKILL.md; no code paths touched. The BOM-free form was tested by KAYOKG on Windows PowerShell 5.1 (first bytes7B 0A, parses in Python and Node); the snippet in the doc was verified byte-identical to his tested command. No Windows PowerShell was available on the Linux VM used for this edit, so no Windows test was run there. The Windows platform gate was confirmed against current upstream main (require_secure_file_output()raises the exact error text quoted in #1015).AI-assistance disclosure: this change was prepared with AI assistance. Commit signed off per DCO.