fix(server): match project copy errors by _tag instead of instanceof - #43999
Open
miladsoroush wants to merge 1 commit into
Open
fix(server): match project copy errors by _tag instead of instanceof#43999miladsoroush wants to merge 1 commit into
miladsoroush wants to merge 1 commit into
Conversation
### Issue for this PR Closes anomalyco#43995 Related to anomalyco#40613, which reports the same symptom (forceRequired false when it should be true) from a different cause — the regex in git.ts only matches English git output. The two are independent: this PR doesn't address the locale case, and fixing the locale case wouldn't address this one. ### Type of change - [x] Bug fix - [ ] New feature - [ ] Refactor / code improvement - [ ] Documentation ### What does this PR do? `project-copy.ts` identifies errors with `instanceof`, which compares class identity. If `@opencode-ai/core` is ever loaded twice in one process, the service constructs the error from one copy of the module and the handler tests it against the other, so every check returns false while the code still looks correct. I hit this in a workspace that path-maps the core specifiers. It won't reproduce on a stock checkout, and since core isn't published, a normal install can't produce two copies — so I'm not claiming this is currently broken for anyone else. The reason I think it's still worth changing: `_tag` comparison doesn't depend on identity, it's already the pattern used in ~52 places in this codebase (including error handling, e.g. `error._tag === "FileSystemError"`), and this handler is the outlier. Six checks changed, plus the now-unused `Git` import removed. No behaviour change when only one copy of core is loaded. ### Impact when the checks do fail - `forceRequired` is dropped from the 400 response, so a client can't tell the user to retry with force after a failed project-copy delete. The operation looks like a plain failure. - All five ProjectCopy errors fall through to `error.message`, surfacing raw git text ("fatal: '...' contains modified or untracked files") instead of the formatted messages. Both fail silently — `instanceof` returning false doesn't throw, so nothing surfaces except the degraded response. ### How did you verify your code works? `bun test test/server/project-copy.test.ts` in the server package — the "lists directories and manages git worktree copies" case asserts `forceRequired: true` after deleting a dirty worktree. It passes before and after on a stock checkout. In my path-mapped setup it failed before and passes now. `tsgo --noEmit` on `packages/server` is clean. ### Screenshots / recordings N/A, not a UI change. ### Checklist - [x] I have tested my changes locally - [x] I have not included unrelated changes in this PR
Contributor
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
Contributor
|
The following comment was made by an LLM, it may be inaccurate: Based on my search, I found one potentially related PR: Related PR (not a duplicate):
Why it's related:
The description notes these are independent issues — fixing one wouldn't address the other — so they are complementary rather than duplicative. |
miladsoroush
changed the title
Author
fixed. |
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
1 participant
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.
Issue for this PR
Closes #43995
Related to #40613, which reports the same symptom (forceRequired false when it should be true) from a different cause — the regex in git.ts only matches English git output. The two are independent: this PR doesn't address the locale case, and fixing the locale case wouldn't address this one.
Type of change
What does this PR do?
project-copy.tsidentifies errors withinstanceof, which compares class identity. If@opencode-ai/coreis ever loaded twice in one process, the service constructs the error from one copy of the module and the handler tests it against the other, so every check returns false while the code still looks correct.I hit this in a workspace that path-maps the core specifiers. It won't reproduce on a stock checkout, and since core isn't published, a normal install can't produce two copies — so I'm not claiming this is currently broken for anyone else. The reason I think it's still worth changing:
_tagcomparison doesn't depend on identity, it's already the pattern used in ~52 places in this codebase (including error handling, e.g.error._tag === "FileSystemError"), and this handler is the outlier.Six checks changed, plus the now-unused
Gitimport removed. No behaviour change when only one copy of core is loaded.Impact when the checks do fail
forceRequiredis dropped from the 400 response, so a client can't tell the user to retry with force after a failed project-copy delete. The operation looks like a plain failure.error.message, surfacing raw git text ("fatal: '...' contains modified or untracked files") instead of the formatted messages.Both fail silently —
instanceofreturning false doesn't throw, so nothing surfaces except the degraded response.How did you verify your code works?
From
packages/opencode:bun test test/server/project-copy.test.ts— the "lists directories and manages git worktree copies" case assertsforceRequired: trueafter deleting a dirty worktree. It passes before and after on a stock checkout. In my path-mapped setup it failed before and passes now.tsgo --noEmitonpackages/serveris clean.Screenshots / recordings
N/A, not a UI change.
Checklist