Fix dev-tools-update path validation: expand ~ before checking paths - #939
Merged
Merged
Conversation
The dev-tools-update handler validated opencode/amicode repo paths with fs.statSync without expanding a leading ~, so the default settings paths (~/harmoniqs/opencode, ~/harmoniqs/amicode) always failed validation even when correct. The dev-tools-rebuild handler already expanded tildes; this brings dev-tools-update in line. TDD: added tests exercising tilde-prefixed paths via a real symlink under the home directory, confirmed RED (fs.statSync fails on a literal ~), then applied the fix and confirmed GREEN.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Blurring either Developer Tools path field re-sends BOTH current path values (sendUpdate() always includes both), and the dev-tools-update handler unconditionally ran a real 'bun run build' + an unconfirmed 'workbench.action.reloadWindow' whenever the amicode path was valid and non-empty — so even an unrelated edit to the opencode field, or just re-blurring an unchanged value, silently rebuilt and reloaded the whole window. Committing a path is now validate-only for both fields: opencode still swaps the binary + restarts just the server (lightweight, non-disruptive, left as-is), and amicode now only checks the path looks like a real repo. Clearing the amicode path to empty still clears any devAssetRoot override — that's removing a setting, not building one. Building is now exclusively an explicit action via the existing 'Rebuild Locally' / 'Rebuild from Main' buttons (a separate dev-tools-rebuild message, already self-sufficient and unaffected by this change). TDD: added a test asserting a valid amicodePath commit produces exactly one synchronous status reply with no building/reloadNeeded and no devAssetRoot config write. Confirmed RED against the old code (it eagerly posts building:true before any real build attempt completes, which is what the interim message the test caught was). Applied the fix, confirmed GREEN (41/41). Side benefit: this also fixes a latent issue in the existing tilde-expansion tests, which incidentally spawned a real, doomed 'bun run build' child process against a fake temp repo before this fix.
#943) dev-tools-update's candidate list for resolving a fork checkout's built opencode binary only ever checked an unsuffixed dist/opencode/bin/opencode path — but every real fork build produces a platform-suffixed directory instead (dist/opencode-darwin-arm64/bin/opencode, etc., per opencode_binary.ts's SUPPORTED list). So a genuinely valid repo path was rejected with 'Binary not found at this path' exactly like an actual typo, and correcting a bad path to a real one just re-ran the same broken check and got the same wrong answer — which reads as 'it doesn't update.' This predates both of this PR's earlier commits (introduced in the original Developer Tools feature, untouched since); the recent flicker fix only changed how the symptom *looks* (steady wrong answer instead of a flicker back to the same wrong answer), not the underlying defect. dev-tools-rebuild's own post-build resolve step had a second, separately incomplete candidate list — darwin-only, missing both Linux targets. Extracted one shared findForkedOpencodeBinary() helper in opencode_binary.ts, built from the canonical SUPPORTED platform list, used by both handlers. It also fixes a latent bug in the original inline loop: mutating a shared reply object across iterations meant an earlier candidate that existed-but-wasn't-executable could permanently mark opencodeValid false even if a later candidate resolved successfully — the new helper returns immediately on the first real match, sidestepping that entirely. TDD: added unit tests for the new helper (platform-suffixed resolution, legacy-layout fallback, not-found vs not-executable, the direct-binary-path case, and the earlier-bad-candidate-doesn't-shadow-a-later-good-one regression) — confirmed RED (function didn't exist), then GREEN (12/12). Added an integration test in chat_bridge.test.ts reproducing the exact reported bug (a fork checkout with only a platform-suffixed layout, like the actual ~/harmoniqs/opencode checkout used to develop this fix) — confirmed RED against the old code, GREEN after wiring both handlers to the shared helper. Full extension suite: 3079/3079 pass, typecheck clean.
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.
Summary
Three fixes to the extension-side
dev-tools-update/dev-tools-rebuildhandlers inchat_bridge.ts, all part of a broader settings-dialog bug sweep paired with harmoniqs/opencode#318.1. Path validation false negatives (tilde expansion)
The handler validated the opencode/amicode repo paths with
fs.statSyncbut never expanded a leading~, so the default settings paths (~/harmoniqs/opencode,~/harmoniqs/amicode) always failed validation even when they pointed at a real, valid repo — Node'sfsmodule doesn't resolve~on its own. The siblingdev-tools-rebuildhandler already expanded tildes; this bringsdev-tools-updatein line with it.Fix: Added
.replace(/^~/, os.homedir())to both path reads.2. Committing a path silently triggered a real build + window reload (#941)
Blurring either path field re-sends BOTH current path values (the app's
sendUpdate()always includes both), and the handler unconditionally ran a realbun run build+ an unconfirmedworkbench.action.reloadWindowwhenever the amicode path was valid and non-empty. So even an unrelated edit to the opencode field — or just re-blurring an unchanged, already-valid amicode path — silently rebuilt and reloaded the whole window with no confirmation.Fix: Committing the amicode path is now validate-only, same as the opencode path already was. Building is exclusively an explicit action via the existing "Rebuild Locally" / "Rebuild from Main" buttons.
3. A valid opencode repo path was rejected identically to a typo (#943)
The candidate list for resolving a fork checkout's built binary only ever checked an unsuffixed
dist/opencode/bin/opencodepath — but every real fork build produces a platform-suffixed directory instead (dist/opencode-darwin-arm64/bin/opencode, etc.). So a genuinely valid repo path was rejected with "Binary not found at this path" exactly like an actual typo — correcting a bad path to a real one just re-ran the same broken check and got the same wrong answer, which reads as "it doesn't update." This predates this PR entirely (introduced in the original Developer Tools feature).A second, separately-incomplete candidate list existed in
dev-tools-rebuild's post-build resolve step (darwin-only, missing both Linux targets).Fix: Extracted one shared
findForkedOpencodeBinary()helper inopencode_binary.ts, built from the canonicalSUPPORTEDplatform list, used by both handlers. Also fixes a latent bug in the original inline loop: mutating a shared reply object across iterations meant an earlier existing-but-not-executable candidate could permanently markopencodeValidfalse even if a later candidate resolved successfully.Testing (TDD)
Each fix has its own failing-first test:
building/reloadNeededand nodevAssetRootconfig write.findForkedOpencodeBinary(platform-suffixed resolution, legacy-layout fallback, not-found vs. not-executable, direct-binary-path, and the earlier-bad-candidate-doesn't-shadow-a-later-good-one regression), plus an integration test reproducing the exact reported bug — a fork checkout with only a platform-suffixed layout, matching a real~/harmoniqs/opencodecheckout.All confirmed RED before their fix, GREEN after.
Side benefit: the auto-build removal also fixes a latent issue in the tilde-expansion tests, which incidentally spawned a real, doomed
bun run buildchild process against a fake temp repo before that fix.Companion PR
This is the extension-side half of a broader settings-dialog bug sweep. The app-side fixes (rebuild label, rebuild status flags, validation flicker, permissions-tab relocation, header alignment, and the companion dead-code cleanup) are in the paired fork PR: harmoniqs/opencode#318.