skills: fix gcode's export remediation; add cad-viewer test suite; repair skill deps - #335
Merged
earthtojake merged 7 commits intoAug 26, 2026
Conversation
… skill deps Finding-driven cleanup across the skills: The gcode skill's remediation text -- emitted to agents at runtime -- told them to run 'python scripts/step --kind part <input> --stl <output>.stl', but the cad skill has no scripts/step and no --kind flag. The real command is 'python scripts/export <input> --stl <output>.stl'. Fixed in gcode_tool.py, SKILL.md and slicer-backends.md. The cad-viewer skill was the only one without a test suite. It now has one: static pins on the packaged layout, the documented start command and the default port, plus a live smoke that boots on a free port and asserts /__cad/server answers with the local-fs backend on that port. implicit-cad had no requirements.txt even though its documented snapshot CLI hard-imports playwright; it now ships one, and dxf's gains the playwright it was missing. snapshot_core's missing-playwright error no longer points agents at another skill's manifest.
CreateProcess cannot spawn the bare 'npm' name -- it is an .cmd shim there. Resolve it via shutil.which (skipping the live smoke when npm is absent) so the suite runs, rather than errors, on Windows.
…utput The Windows runner has no CAD python toolchain for the launcher's runtime probe to find, so the smoke died in require_cadgen_runtime before serving. The suite boots the SERVE surface, not the build toolchain -- set VIEWER_CAD_BACKEND_VALIDATED=1 (the same escape hatch test_server_startup uses) and include the process's captured output when it exits early.
Windows re-grabs the ephemeral probe port between our close() and the viewer's bind, so 'already in use' is a race to retry with the next free port, not a broken start command. The early-exit diagnostic now raises a dedicated error carrying the captured output.
Windows NAT drivers reserve blocks of the ephemeral range where connect() to a closed port fails without refusal, which the launcher's occupancy probe must read as occupied -- so every ephemeral candidate reported busy. Probe ports now come from a low, non-ephemeral band.
Owner
|
Looks like windows CI/CD is failing |
earthtojake
added a commit
to warun7/text-to-cad
that referenced
this pull request
Aug 26, 2026
The launcher's port_is_free connected to the port and counted only ConnectionRefusedError as free. On Windows a connect to a closed port routinely fails some other way (Hyper-V/WSL port exclusions, loopback filtering, refusals arriving as timeouts), so every port read as occupied and the launcher refused to start with a false "already in use" error. Found by earthtojake#335's Windows smoke test, where four random ports Python had just bound and released all failed the connect probe. Bind instead: it is the same operation the server is about to perform, so the probe cannot disagree with reality. EADDRINUSE and EACCES (Windows's answer for excluded port ranges) keep the friendly rerun-with---port message; any other error counts as free, because the probe exists only for that message and the server's own bind stays authoritative. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
|
Merged, thanks for this and for chasing the Windows failure as far as you did. The failure was in our launcher, not your test. |
earthtojake
pushed a commit
that referenced
this pull request
Aug 26, 2026
Source ref: develop Source commit: cce04de Target branch: main Previous target: 8f9a7d7 Release base: 8f9a7d7 Previous source: 96675ba Included commits since previous source: cce04de Merge pull request #337 from earthtojake/release/0.4.28 c3f3856 Release 0.4.28 c7e2a7c Merge pull request #305 from warun7/fix/viewer-worker-deadlock-and-timeouts 2b65d4f Merge branch 'develop' into fix/viewer-worker-deadlock-and-timeouts 6f0265d Merge pull request #335 from warun7/fix/skill-remediations-and-coverage 1e4aea1 Merge branch 'develop' into fix/skill-remediations-and-coverage 1f75ced Merge pull request #336 from earthtojake/claude/port-probe-bind 3236a5c viewer: probe port availability by binding, not connecting 99a806f tests: pick viewer-smoke ports outside the ephemeral range 5633b65 tests: call the module-level drain helper directly 788bb5d tests: retire a busy candidate port instead of failing the viewer smoke 7306fbe tests: skip the cadgen probe in the viewer start smoke, surface its output 603e812 tests: resolve npm through PATH for the viewer start smoke on Windows 0b64fa3 skills: point gcode at the real cad export CLI; cover cad-viewer; fix skill deps 24e9d28 viewer: restore run_cadgen_cold's terminal error return 3150457 tests: drive the stderr drainer from a real subprocess pipe dbeea4f viewer: kill the CAD worker and cold subprocess on idleness, not wall clock 06bf1b3 viewer: add worker and cold process timeouts and stream large assets
Contributor
Author
|
yeah that makes sense |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Skill-level fixes and coverage gaps found in review.
gcode remediation text pointed agents at a command that does not exist
The remediation emitted at runtime said
python scripts/step --kind part <input> --stl <output>.stl- but the cad skill has noscripts/stepand never had a--kindflag, so every STEP/DXF/URDF/SDF slicing flow got misdirected exactly when it needed help. The real CLI isscripts/export <input> --stl <output>.stl(gen sources and imported STEP/STP alike). Fixed ingcode_tool.py,SKILL.md:123andreferences/slicer-backends.md.cad-viewer was the only skill without a test suite
New
tests/python/skills/cad-viewer/test_packaged_viewer.py:startscript + shim, Python launcher, requirements pinning the vendored cadgen) and on SKILL.md documenting the start command and port 3245npm --prefix scripts/viewer run starton a free port and asserts/__cad/serveranswers JSON withbackend=local-fson that exact portSkill dependency repairs
skills/implicit-cad/shipped no requirements.txt although its documented snapshot CLI hard-imports playwright -> one added (--editable ./scripts/packages/cadgen+ playwright), matching sdf/srdf/urdf.skills/dxf/requirements.txtwas missing playwright for the same reason -> added.snapshot_core.py's missing-playwright error told users to "Install the CAD skill requirements" - another skill's manifest. Now says "the invoking skill's own requirements.txt".Dropped from scope since the scan: daemon
-stdin inline handling is already documented incadgen_daemon/client.py.Verification
scripts/test/test-python.sh: all 15 suites OK, including the new cad-viewer suite (5 tests) wired into the runner via itstests/python/skills/<skill>directory