sync: merge upstream v1.1.13 into integration #292
Merged
Conversation
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
…ce code duplication across tools
Co-authored-by: Aiden Cline <[email protected]>
Resolved conflicts: - .gitignore: merged both .loop* and *.bun-build patterns - bun.lock: kept fork's @shuvcode/desktop name, updated version to 1.1.13 - packages/app/src/app.tsx: merged shuv.ai hostname, __OPENCODE__.serverUrl support, and query param priority - packages/app/src/components/prompt-input.tsx: kept upstream onClick/onMouseEnter pattern (mobile-friendly) - packages/desktop/src-tauri/src/lib.rs: kept shuv.ai branding, merged configured server URL health check logic - sdks/vscode/package.json: kept fork's shuvcode name and latitudes-dev publisher, updated version to 1.1.13
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const filepath = typeof metadata["filepath"] === "string" ? metadata["filepath"] : "" | ||
| const diff = typeof metadata["diff"] === "string" ? metadata["diff"] : "" | ||
|
|
||
| const content = await Bun.file(filepath).text() |
There was a problem hiding this comment.
The file read operation await Bun.file(filepath).text() can throw an error if the file doesn't exist, is not readable, or the path is invalid. This should be wrapped in a try-catch block to handle potential errors gracefully.
Suggested change
| const content = await Bun.file(filepath).text() | |
| try { | |
| const content = await Bun.file(filepath).text() | |
| const newContent = getNewContent(content, diff) | |
| if (newContent) { | |
| this.connection.writeTextFile({ | |
| sessionId: sessionId, | |
| path: filepath, | |
| content: newContent, | |
| }) | |
| } | |
| } catch (error) { | |
| log.error("Failed to read or apply diff to file", { filepath, error }) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/opencode/src/acp/agent.ts
Line: 114:114
Comment:
The file read operation `await Bun.file(filepath).text()` can throw an error if the file doesn't exist, is not readable, or the path is invalid. This should be wrapped in a try-catch block to handle potential errors gracefully.
```suggestion
try {
const content = await Bun.file(filepath).text()
const newContent = getNewContent(content, diff)
if (newContent) {
this.connection.writeTextFile({
sessionId: sessionId,
path: filepath,
content: newContent,
})
}
} catch (error) {
log.error("Failed to read or apply diff to file", { filepath, error })
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+448
to
+451
| let escaped_url = url.replace('\\', "\\\\").replace('"', "\\\""); | ||
| let _ = window.eval(format!( | ||
| "window.__SHUVCODE__.serverUrl = \"{escaped_url}\"; window.__OPENCODE__.serverUrl = \"{escaped_url}\";", | ||
| )); |
There was a problem hiding this comment.
The URL escaping logic here may not correctly handle all special characters. The replace calls handle backslashes and quotes, but URLs can contain other characters that need escaping (e.g., single quotes, newlines, or other control characters). Consider using a proper URL encoding/escaping function or JSON serialization to ensure the URL is safely injected into JavaScript.
Additionally, injecting URLs directly into JavaScript via eval could be vulnerable to injection attacks if the URL contains malicious content. Consider using a safer alternative like postMessage or Tauri's state management.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/src-tauri/src/lib.rs
Line: 448:451
Comment:
The URL escaping logic here may not correctly handle all special characters. The `replace` calls handle backslashes and quotes, but URLs can contain other characters that need escaping (e.g., single quotes, newlines, or other control characters). Consider using a proper URL encoding/escaping function or JSON serialization to ensure the URL is safely injected into JavaScript.
Additionally, injecting URLs directly into JavaScript via `eval` could be vulnerable to injection attacks if the URL contains malicious content. Consider using a safer alternative like `postMessage` or Tauri's state management.
How can I resolve this? If you propose a fix, please make it concise.
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
Syncs shuvcode fork with upstream opencode v1.1.13, bringing desktop app improvements, tool enhancements, and bug fixes.
Changes
Features
Fixes
Refactoring
Internal
Breaking Changes
None
Testing
Greptile Overview
Greptile Summary
This PR syncs the shuvcode fork with upstream opencode v1.1.13, bringing substantial improvements across desktop functionality, tool enhancements, and bug fixes.
Key Changes
Desktop Application Enhancements:
Core Tool Refactoring:
assertExternalDirectoryutility, eliminating code duplication across Edit, Write, Read, Glob, and Grep tools.Search & File Operations:
--followand--hiddenflags added to ripgrep).Built-in Codex Plugin:
CodexAuthPlugin) provides ChatGPT Pro/Plus OAuth authentication with PKCE flow.opencode-openai-codex-authplugin for better reliability.ACP Agent Enhancement:
Provider & Plugin System:
sessionIDparameter tochat.system.transformhook for session-aware system message transformations.accountIdfor organization subscriptions.TUI Fixes:
props.reftoonMount.LSP Enhancement:
Integration Quality
The changes integrate cleanly with the existing codebase architecture. The refactoring to shared utilities (external-directory validation) improves maintainability and reduces the risk of inconsistent behavior across tools. The new Codex plugin follows established plugin patterns and is properly integrated as an internal plugin alongside existing authentication providers.
Issues Found
Two issues were identified that should be addressed:
ACP Agent File Operations: Missing error handling when reading files for diff application (line 114 of
acp/agent.ts). File read can fail if the file doesn't exist or is not readable.URL Injection Security: The Rust code that injects the configured server URL into JavaScript uses basic string escaping that may not handle all special characters safely (line 448-451 of
lib.rs). Consider using proper JSON serialization or a safer injection method.These are not critical blockers but should be fixed to prevent potential runtime errors and security issues.
Confidence Score: 4/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant User participant DesktopApp as Desktop App (Tauri) participant Store as Settings Store participant Server as Remote/Local Server participant UI as React UI Note over User,UI: Desktop App Launch Flow with Default Server User->>DesktopApp: Launch App DesktopApp->>Store: get_default_server_url() alt Default Server URL Configured Store-->>DesktopApp: Return server URL DesktopApp->>Server: check_server_health(url) alt Server is Healthy Server-->>DesktopApp: Health check success DesktopApp->>UI: Inject serverUrl via window.__SHUVCODE__ DesktopApp->>UI: Set serverReady = true UI->>Server: Connect to configured server else Server Not Reachable Server-->>DesktopApp: Health check failed DesktopApp->>User: Show dialog: Retry or Start Local? alt User Chooses Retry User-->>DesktopApp: Retry DesktopApp->>Server: check_server_health(url) again else User Chooses Start Local User-->>DesktopApp: Start Local DesktopApp->>DesktopApp: spawn_sidecar() on port DesktopApp->>UI: Set serverReady = true UI->>Server: Connect to local server end end else No Default Server Configured Store-->>DesktopApp: Return None DesktopApp->>DesktopApp: Check if port already running alt Port Not Running DesktopApp->>DesktopApp: spawn_sidecar() on port end DesktopApp->>UI: Set serverReady = true UI->>Server: Connect to local server end Note over User,UI: User Configures Default Server User->>UI: Open Server Selection Dialog UI->>DesktopApp: getDefaultServerUrl() DesktopApp->>Store: get_default_server_url() Store-->>UI: Current default URL (or null) User->>UI: Set Current as Default UI->>DesktopApp: setDefaultServerUrl(url) DesktopApp->>Store: set(DEFAULT_SERVER_URL_KEY, url) Store->>Store: save() Store-->>UI: Success Note over User: Requires app restart to take effect