chore: oc server related improvements#194
Conversation
Unify native-host and oc serve process startup through one internal launcher so cwd/env/cors behavior stays consistent. Also allow additional native-host extension IDs via CALY_NATIVE_HOST_EXTENSION_IDS for sideloaded installs.
Add native-host status diagnostics and tighten direct native-host dispatch so subcommands route through Commander. Also add oc proxy workdir overrides via --cwd/--workdir with shared workspace defaults and explicit override handling.
Document native-host status command and new oc --cwd/--workdir behavior. Clarify default shared workspace behavior and current-directory override options.
🦋 Changeset detectedLatest commit: 460470f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant CLI as CLI Implementation
participant Manifest as Manifest File
participant Wrapper as Wrapper File
participant Registry as Windows Registry
participant Extension as Extension Origins
Client->>CLI: showNativeHostStatus()
CLI->>Manifest: Check manifest path exists
Manifest-->>CLI: manifest exists/missing
CLI->>Wrapper: Check wrapper executable exists
Wrapper-->>CLI: wrapper exists/missing
CLI->>Registry: Query native messaging registry key
Registry-->>CLI: registry config exists/missing
CLI->>Manifest: Read manifest allowed_origins
Manifest-->>CLI: origins list
CLI->>Extension: Verify extension IDs in origins
Extension-->>CLI: match/mismatch detected
CLI->>Client: Report status + warning if mismatches
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/cli/src/commands/opencode/index.ts (2)
246-252: Silent failure when--workdirhas no value argument.If a user provides
--workdiras the last argument without a path, the code silently continues without settingexplicitWorkdir. This could confuse users who expect the option to work.Consider logging a warning when the value is missing:
♻️ Suggested improvement
if (arg === '--workdir') { const next = passThroughArgs[i + 1]; if (next) { explicitWorkdir = next; i++; + } else { + log.warn('--workdir requires a path argument; ignoring.'); } continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/opencode/index.ts` around lines 246 - 252, The parsing branch handling the "--workdir" option silently ignores the flag when it's the last arg (no value); update the block that checks if (arg === '--workdir') to detect when passThroughArgs[i + 1] is undefined and emit a clear warning (e.g., console.warn or the CLI logger) stating that "--workdir" was provided without a value, instead of just continuing; if desired, also consider failing early or showing usage, but at minimum log the missing-value warning before continuing, while keeping the existing behavior of setting explicitWorkdir = next when next is present.
19-19: Consider adding type annotation toprogramparameter.The
programparameter lacks an explicit type annotation. Per coding guidelines, explicit typing is preferred over implicitany.♻️ Suggested fix
-async function registerOpencodeCommands(program) { +async function registerOpencodeCommands(program: import('commander').Command) {Or import the type at the top of the file:
import type { Command } from 'commander'; // ... async function registerOpencodeCommands(program: Command) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/opencode/index.ts` at line 19, The function registerOpencodeCommands has an untyped parameter "program"; add an explicit type annotation (e.g., program: Command) by importing the Command type from 'commander' at the top of the file (import type { Command } from 'commander') and update the function signature to async function registerOpencodeCommands(program: Command) { ... }; also update any local references or tests that call registerOpencodeCommands if they rely on the implicit any to ensure type compatibility.packages/cli/src/commands/opencode/implementation.ts (2)
102-212: Pre-existing concern: Process ownership verification.Based on learnings from a previous review,
killProcessOnPort()kills any process on the port without verifying ownership. The recommended solution was to persist the server's PID to a file (~/.calycode/opencode/server.pid) and only kill processes with tracked PIDs. This issue predates this PR but may be worth addressing in a follow-up to prevent accidentally killing unrelated processes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/opencode/implementation.ts` around lines 102 - 212, killProcessOnPort currently kills any process listening on the target port; change it to read the persisted PID at ~/.calycode/opencode/server.pid (create helper like getPersistedServerPid or readPersistedPid) and only attempt to kill PIDs that match the persisted PID(s); in Windows branch (netstat parsing) and Unix branch (fuser/lsof parsing) compare discovered PIDs against the persisted PID and skip/klog any non-matching processes, and if no discovered PID matches the persisted one return false and log that the tracked server PID was not found; ensure writing the PID when starting the server (e.g., saveServerPid) and handle missing/invalid PID file gracefully.
566-575: Minor redundancy in args construction for logging.The
argsarray on line 566 is constructed solely for logging purposes, whilelaunchOpencodeServerinternally constructs its own args. This is acceptable for debugging but could be simplified by havinglaunchOpencodeServerreturn or log the args it uses.The refactoring to use the shared helper is a good improvement for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/opencode/implementation.ts` around lines 566 - 575, The local construction of the args array (const args = [..., getCorsArgs(extraOrigins)]) is redundant because launchOpencodeServer builds its own args; remove that local args construction and logging, and instead expose the arg-builder from launchOpencodeServer (either by returning the args when calling launchOpencodeServer or by creating a shared helper like buildOpencodeArgs/getOpencodeArgs) so the caller can log the exact args and launchOpencodeServer can reuse the same arg-builder; update the call site to use getOpencodeArgs(...) for logger.log and pass the same args or rely on launchOpencodeServer returning them, preserving existing logger.log lines (use getCalycodeOpencodeConfigDir, getOpencodeWorkingDir and getCorsArgs names to locate usages).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/index.ts`:
- Around line 10-17: The native-host detection in index.ts (using commandIndex,
args and isDirectNativeHostInvocation) is missing the chrome-extension://
argument check present in index-bundled.ts; update the detection logic to also
detect a chromeExtensionArg (e.g., any arg starting with "chrome-extension://")
and route those invocations to startNativeHost() just like the bundled
entrypoint, ensuring args with chrome-extension:// are handled before falling
through to program.parseAsync().
---
Nitpick comments:
In `@packages/cli/src/commands/opencode/implementation.ts`:
- Around line 102-212: killProcessOnPort currently kills any process listening
on the target port; change it to read the persisted PID at
~/.calycode/opencode/server.pid (create helper like getPersistedServerPid or
readPersistedPid) and only attempt to kill PIDs that match the persisted PID(s);
in Windows branch (netstat parsing) and Unix branch (fuser/lsof parsing) compare
discovered PIDs against the persisted PID and skip/klog any non-matching
processes, and if no discovered PID matches the persisted one return false and
log that the tracked server PID was not found; ensure writing the PID when
starting the server (e.g., saveServerPid) and handle missing/invalid PID file
gracefully.
- Around line 566-575: The local construction of the args array (const args =
[..., getCorsArgs(extraOrigins)]) is redundant because launchOpencodeServer
builds its own args; remove that local args construction and logging, and
instead expose the arg-builder from launchOpencodeServer (either by returning
the args when calling launchOpencodeServer or by creating a shared helper like
buildOpencodeArgs/getOpencodeArgs) so the caller can log the exact args and
launchOpencodeServer can reuse the same arg-builder; update the call site to use
getOpencodeArgs(...) for logger.log and pass the same args or rely on
launchOpencodeServer returning them, preserving existing logger.log lines (use
getCalycodeOpencodeConfigDir, getOpencodeWorkingDir and getCorsArgs names to
locate usages).
In `@packages/cli/src/commands/opencode/index.ts`:
- Around line 246-252: The parsing branch handling the "--workdir" option
silently ignores the flag when it's the last arg (no value); update the block
that checks if (arg === '--workdir') to detect when passThroughArgs[i + 1] is
undefined and emit a clear warning (e.g., console.warn or the CLI logger)
stating that "--workdir" was provided without a value, instead of just
continuing; if desired, also consider failing early or showing usage, but at
minimum log the missing-value warning before continuing, while keeping the
existing behavior of setting explicitWorkdir = next when next is present.
- Line 19: The function registerOpencodeCommands has an untyped parameter
"program"; add an explicit type annotation (e.g., program: Command) by importing
the Command type from 'commander' at the top of the file (import type { Command
} from 'commander') and update the function signature to async function
registerOpencodeCommands(program: Command) { ... }; also update any local
references or tests that call registerOpencodeCommands if they rely on the
implicit any to ensure type compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec664a7b-3325-4cdb-aaeb-7041ecd19590
📒 Files selected for processing (9)
.changeset/hot-worms-juggle.md.changeset/yummy-trains-boil.mddocs/commands/oc-native-host.mddocs/commands/oc.mddocs/guides/opencode-setup.mdpackages/cli/src/commands/opencode/implementation.tspackages/cli/src/commands/opencode/index.tspackages/cli/src/index-bundled.tspackages/cli/src/index.ts
|
|
||
| const commandIndex = Math.max(args.lastIndexOf('opencode'), args.lastIndexOf('oc')); | ||
| const isDirectNativeHostInvocation = | ||
| commandIndex >= 0 && | ||
| args[commandIndex + 1] === 'native-host' && | ||
| commandIndex + 2 >= args.length; | ||
|
|
||
| if (isDirectNativeHostInvocation) { |
There was a problem hiding this comment.
Missing chrome-extension:// argument handling compared to bundled entry point.
The index-bundled.ts file (lines 42-47 in the relevant snippet) includes an additional check for chrome-extension:// arguments that routes those invocations directly to startNativeHost(). This file lacks that check, meaning:
- Bundled binary:
chrome-extension://...args →startNativeHost() - This file:
chrome-extension://...args →program.parseAsync()(Commander)
This could cause inconsistent behavior when the native messaging host is invoked by Chrome on different platforms. If the non-bundled entry point is ever used as the native host target, Chrome's invocation would fail.
Consider adding the same chromeExtensionArg check here for consistency:
🔧 Suggested fix
const args = process.argv;
exitIfLegacyXanoInvocation(args);
+const chromeExtensionArg = args.find((arg) => arg.startsWith('chrome-extension://'));
const commandIndex = Math.max(args.lastIndexOf('opencode'), args.lastIndexOf('oc'));
const isDirectNativeHostInvocation =
commandIndex >= 0 &&
args[commandIndex + 1] === 'native-host' &&
commandIndex + 2 >= args.length;
-if (isDirectNativeHostInvocation) {
+if (chromeExtensionArg || isDirectNativeHostInvocation) {
startNativeHost();
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/index.ts` around lines 10 - 17, The native-host detection in
index.ts (using commandIndex, args and isDirectNativeHostInvocation) is missing
the chrome-extension:// argument check present in index-bundled.ts; update the
detection logic to also detect a chromeExtensionArg (e.g., any arg starting with
"chrome-extension://") and route those invocations to startNativeHost() just
like the bundled entrypoint, ensuring args with chrome-extension:// are handled
before falling through to program.parseAsync().
1 participant
Summary by CodeRabbit
New Features
--cwdand--workdiroptions to control OpenCode proxy working directory.native-host statussubcommand to verify native host configuration.Documentation
Chores