Repository navigation
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jan Milde <janmilde47@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
85d4ae5 to
6a8f694
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jan Milde <janmilde47@gmail.com>
4bbf2a6 to
101c82a
Compare
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @JJM1986, thank you for your contribution to SkillSpector — we really appreciate the time you put into this! A few items need attention before it can be merged; details below.
This PR makes two Windows test-portability fixes: explicit pytest ids for the long-string parametrization in tests/nodes/analyzers/test_json_container_ownership.py, and a newline-aware size assertion in tests/nodes/test_build_context.py. Its second commit (101c82ab, "feat(cli): add offline Runway project exporter") also adds an unrelated production feature: a skillspector runway-project CLI command, a new skillspector.runway_projects module, a README section and tests. I checked the two test fixes by reading the code, and both are correct. All 19 ids match the 19 parameter sets. _MAX_JSON_QUOTE_CONTAINER_CHARS is 65,536, so the auto-generated test ID would exceed Windows' 32,767-character limit when pytest sets PYTEST_CURRENT_TEST. write_text() writes CRLF on Windows, so the os.linesep expectation is right. Both changes behave the same as before on POSIX. The Runway commit is the blocker.
Findings
-
[Blocking] An unrelated feature is bundled into this test-portability PR (
src/skillspector/cli.py:3092,src/skillspector/runway_projects.py:1,tests/unit/test_runway_projects.py:1,README.md:147).- The new command exports prompt sheets for Runway video generation from hard-coded templates ("The Legends of Automation", "Cerberus Wolf: Drone Runaway"). The templates cite an "attached video brief" that is not in the repository.
- It is unrelated to both Windows compatibility and skill scanning, and there is no linked issue.
- It adds a public CLI command and shipped package code to a security tool. This contradicts the PR description ("No source behavior was changed; the changes are limited to test portability").
- Existing commands are unaffected, because the change only adds a command and an import. That is not a reason to include it here.
- The new files lack the SPDX header that CONTRIBUTING.md requires for new source files.
- The README section is inserted just before the existing
#### Size limitssubsection, which now reads as part of the Runway section.
Please drop commit
101c82ab, for example by resetting the branch to6a8f6941and force-pushing, so the PR contains only the Windows test fixes. If you want to propose this feature, please open an issue first. -
[Non-blocking]
tests/nodes/test_build_context.py:1378: optional. The new expectation is correct, but the test file's contents still depend on the platform. Writing it byte-exact (payload.write_bytes(b"ordinary metadata\n")) and keepinglen(b"ordinary metadata\n")would make the test identical on every platform. -
[Non-blocking]
tests/nodes/analyzers/test_json_container_ownership.py:89: optional. A separateids=[...]list can silently mislabel cases if entries are reordered later.pytest.param(value, id="...")keeps each ID next to its case.
Tests/CI: All checks pass at 101c82ab (DCO, lint, test-unit, docker-smoke, OpenCode). CI runs only on Ubuntu with Python 3.12, so the Windows behavior itself is not exercised. Related: #636 (same author) makes separate Windows fixes under scripts/ and does not touch the same files. Once the Runway commit is removed, the remaining two test changes look ready to approve.
Decision: Changes Requested (reviewed head 101c82ab02ccffbfce7119e31e7ba1c1a60e3a31)
| raise typer.Exit(code=1) | ||
|
|
||
|
|
||
| @app.command("runway-project") |
There was a problem hiding this comment.
[Blocking] This runway-project command generates Runway video-prompt sheets from built-in templates. It is an unrelated product feature, as are the skillspector.runway_projects import and the RunwayFormat enum it needs, and none of it belongs in a Windows test-suite fix. It adds a public CLI command to a security scanner without an issue or design discussion. It also contradicts the PR description ("No source behavior was changed"). Please drop commit 101c82ab from this PR.
| @@ -0,0 +1,179 @@ | |||
| """Offline Runway video prompt projects. | |||
There was a problem hiding this comment.
[Blocking] This new module ships in the package but has nothing to do with scanning skills. It contains hard-coded "Cerberus Wolf" and "Legends of Automation" video prompts that cite an "attached video brief" not present in the repository. It also lacks the SPDX header that CONTRIBUTING.md requires for new source files. Please remove it together with tests/unit/test_runway_projects.py.
| skillspector scan ./my-skill.zip | ||
| ``` | ||
|
|
||
| ### Runway video projects |
There was a problem hiding this comment.
[Blocking] Please remove this section along with the rest of the Runway feature. Where it sits now, it also splits the Basic Usage docs. The existing #### Size limits subsection, which documents the caps on remote and archive inputs, now appears as a child of "Runway video projects".
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@JJM1986 thank you for running the suite on Windows and fixing what you found.
Value and readiness
This is a status re-review of 101c82a against current main (530d3c5). The branch has not changed since the 28 September review, and it merges cleanly into current main. main has not picked up either Windows fix, so both are still useful:
- the 19
idsstill match the 19 parametrized cases; _MAX_JSON_QUOTE_CONTAINER_CHARSis still 65,536;- the merged
test_build_context.pystill writes its payload withwrite_text.
The only blocker is still the unrelated Runway commit. Once that commit is dropped, the two test fixes are ready to approve.
Material findings
Prior findings:
- Blocker: commit
101c82abadds an unrelatedrunway-projectcommand, therunway_projects.pymodule, a README section and tests (src/skillspector/cli.py:3092,src/skillspector/runway_projects.py:1,README.md:147; open threads). Still open. Please reset the branch to6a8f6941and force-push. - Optional: write the test payload byte-exact with
write_bytesintests/nodes/test_build_context.py. Still open. - Optional: use
pytest.param(..., id=...)instead of a separateidslist. Still open.
I found nothing new.
PIC tradeoffs
None identified.
Verification and gaps
git merge-treeagainst530d3c5is clean. In the merged tree the parametrize decorator has 19 cases and 19 ids, andimport osis present for theos.linesepassertion.- CI passed at this head on 25 September (DCO, lint, test-unit, docker-smoke, OpenCode). That run used an older
main, so CI should re-run after the branch is updated. CI runs only on Ubuntu, so it does not exercise Windows. - The author has not responded on the PR in the 15 days since the review.
Windows test execution exposed two portability issues that prevented the suite from running reliably. This change keeps pytest's generated environment metadata below Windows' 32,767-character limit and makes the excluded-artifact size assertion account for platform newline encoding.
Validation:
No source behavior was changed; the changes are limited to test portability and deterministic parametrization.