Corrected the claim that sbx is never on PATH on Windows and restated the rule as never relying on it, across the rules file, Architecture and three skills - #31
Merged
Conversation
… the rule as never relying on it, across the rules file, Architecture and three skills
There was a problem hiding this comment.
Pull request overview
This PR corrects documentation and process guidance around Windows executable resolution for sbx, replacing the inaccurate claim that it is “not on PATH” with the durable rule the codebase relies on: never depend on PATH because sbxPath() checks the install location first on every call and PATH cannot fix stale-environment VS Code windows.
Changes:
- Updates
CLAUDE.mdanddocs/Architecture.mdto state the correct Windows resolution behavior and why PATH cannot be relied upon. - Updates three workflow skills to probe
sbxvia the install location (not PATH) and to usesbx diagnoseas the single readiness probe.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Architecture.md | Restates sbx resolution as “check install location first; PATH may or may not exist and is not relied upon,” with the stale-environment rationale. |
| CLAUDE.md | Replaces “not on PATH” with “never rely on PATH,” aligning the rules with sbxPath()’s per-call resolution behavior. |
| .claude/skills/spec-implement/SKILL.md | Updates the “traps” checklist to the new PATH guidance and reinforces “do not memoize” the resolution. |
| .claude/skills/ext-run-local/SKILL.md | Updates local acceptance prerequisites to probe the install location and use sbx diagnose for readiness checks. |
| .claude/skills/dev-onboard/SKILL.md | Updates onboarding prerequisites to probe the install location rather than PATH and points to sbx diagnose as the consolidated preflight. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Risk & Size
Risk: Low
Size: Small
Documentation and skill text only — no source file changes.
What & Why
CLAUDE.md asserted "
sbxis not on PATH (Windows)", and four other places repeated it.On a v0.31.3 install this is false:
HKCU:\Environmentcarries%LOCALAPPDATA%\DockerSandboxes\bin\, andsbx.exeresolves throughPATH. Whether theinstaller adds that entry or the machine's owner did is not determinable after the fact, so
the rule is restated to be true either way and to say the thing that actually matters:
never rely on PATH —
sbxPath()checks the install location first, on every call, and aPATH entry cannot rescue the case FR-059 was about anyway (a VS Code window already open when
sbx was installed captured its environment beforehand).
Surfaced while building the FR-059 test harness for #29: the "not installed" simulation was a
no-op until both sources were removed, which is what exposed the stale claim.
No spec: this corrects an inaccurate statement about the environment and adjusts process text.
Per CLAUDE.md, specs record product changes only.
Changes
CLAUDE.md— the backend-model bullet now reads "never rely onsbxbeing on PATH", namesthe resolution order, and notes the stale-environment case.
docs/Architecture.md§10 — same correction; states that aPATHentry varies by installand is not relied upon.
.claude/skills/spec-implement— the trap entry restated, plus "do not reintroduce a memo"(the resolution is deliberately uncached since Made a missing prerequisite visible and actionable instead of a silently empty UI, and scoped the host Docker requirement to the custom-image build (FR-059) #29).
.claude/skills/dev-onboard— probe the install location rather than PATH; points atsbx diagnose, which reports binary, daemon and authentication in one call..claude/skills/ext-run-local— same, and records what a missingsbxnow looks like(
⚠ Sandbox not available, readiness node, modal on New Sandbox — FR-059).How to Verify
npm run verify— exits 0 (no source changed; the gate runs anyway pergit-commit-push).grep -rn -i "not on PATH" CLAUDE.md docs/ .claude/ README.md— returns nothing.entry exists, only that the code must not depend on one.