feat: improve native host reliability#196
Conversation
Allow start/restart native-messaging commands to pass an OpenCode version override and validate version format before spawning npx. This lets the extension control tested versions while keeping the pinned default as fallback.
🦋 Changeset detectedLatest commit: d85fb7a 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 native-host extension discovery for Chromium browsers, adds OpenCode version pinning with optional runtime overrides, implements platform-specific native host setup with manifest registration, and propagates version selection through server launch and native messaging. ChangesNative Host Extension Discovery and OpenCode Version Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/commands/opencode/implementation.ts (1)
1437-1463:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ocVersionis validated but never propagated bysetupOpencode.
setupOpencode({ ocVersion })resolves and warns about the version, but the resolved value isn't forwarded tosetupNativeHostRegistrationor persisted anywhere; the wrapper script emitted insetup.tsdoesn't include--oc-version, and the native-host's fallback parser (parseOcVersionFromArgv(process.argv)at line 623) reads from the host process's own argv. Net effect:caly-xano oc init --oc-version 1.2.3validates the input and then silently throws it away — every subsequent server start will resolve to the default (or whatever the extension passes).If
--oc-versiononinitis meant to pin the version for the installed native host, bake it into the wrapper script (e.g. append--oc-version <value>to the generated.bat/.sh); otherwise remove the option fromoc initto avoid misleading users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/opencode/implementation.ts` around lines 1437 - 1463, setupOpencode currently resolves ocVersion but doesn't pass it on or bake it into the generated wrapper, so init validates but discards the value; modify setupOpencode to forward the resolvedVersion into setupNativeHostRegistration (e.g. change call to setupNativeHostRegistration(extensionIds, resolvedVersion)) and update the wrapper-generation logic in setup.ts to append `--oc-version <resolvedVersion>` into the emitted .sh/.bat commands so the native host receives the pinned version (this ensures parseOcVersionFromArgv(process.argv) will see the value). If the CLI should not support persisting ocVersion, instead remove the ocVersion option from setupOpencode and callers to avoid the misleading behavior.packages/cli/src/commands/opencode/index.ts (1)
41-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
initandserveactions do not receive parent--oc-versionoption — they declare only(options)and try to readoptions.ocVersion.In Commander.js v14.0.0, action handler callbacks receive
(options, command)whereoptionscontains only that subcommand's own parsed options. Parent/global options must be accessed viacommand.parent?.opts()orcommand.optsWithGlobals(). The--oc-versionoption is registered on theocparent command (line 32), but theinit(lines 41–47) andserve(lines 174–180) actions only declare theoptionsparameter and attempt to readoptions.ocVersion, which will always beundefined.Result:
caly-xano oc init --oc-version 1.14.41andcaly-xano oc serve --oc-version 1.14.41both passocVersion: undefinedto their handlers, silently falling back to defaults. Therunaction (line 206) already demonstrates the correct pattern withcommand.parent?.opts()?.ocVersion.Add the
commandparameter to both action callbacks and access parent options viacommand.parent?.opts()?.ocVersionorcommand.optsWithGlobals().ocVersion.Proposed fix using
command.parent?.opts()opencodeNamespace .command('init') .description( 'Initialize OpenCode native host integration and configuration for use with the CalyCode extension.', ) .option('-f, --force', 'Force overwrite existing configuration files') .option('--skip-config', 'Skip installing OpenCode configuration templates') - .action(async (options) => { + .action(async (options, command) => { await setupOpencode({ force: options.force, skipConfig: options.skipConfig, - ocVersion: options.ocVersion, + ocVersion: command.parent?.opts()?.ocVersion, }); }); @@ opencodeNamespace .command('serve') .description('Serve the OpenCode AI server locally.') .option('--port <port>', 'Port to run the OpenCode server on (default: 4096)') .option('-d, --detach', 'Run the server in the background (detached mode)') - .action(async (options) => { + .action(async (options, command) => { await serveOpencode({ port: options.port ? parseInt(options.port, 10) : undefined, detach: options.detach, - ocVersion: options.ocVersion, + ocVersion: command.parent?.opts()?.ocVersion, }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/opencode/index.ts` around lines 41 - 47, The init and serve action handlers call setupOpencode({ ocVersion: options.ocVersion }) but only declare (options) so they never see the parent --oc-version; change both action callbacks to accept the second parameter (commonly named command) and read the parent/global option via command.parent?.opts()?.ocVersion (or command.optsWithGlobals().ocVersion) when passing ocVersion into setupOpencode and similar calls so the parent oc command's --oc-version value is used.
🧹 Nitpick comments (7)
packages/cli/src/utils/host-constants.ts (1)
22-22: ⚡ Quick winUse proper type definition instead of inline type cast.
The type cast
as 'strict' | 'balanced' | 'name-only'is a workaround. Consider defining a proper type for the mode:♻️ Proposed improvement
+type ExtensionDiscoveryMode = 'strict' | 'balanced' | 'name-only'; + export const HOST_APP_INFO = { name: 'CalyCode Xano CLI', description: 'CalyCode Xano CLI Native Host', reverseAppId: 'com.calycode.cli', appId: 'cli.calycode.com', version: '1.0.0', url: 'https://calycode.com/xano', // Known extension IDs (fast-path allowlist) extensionId: 'hadkkdmpcmllbkfopioopcmeapjchpbm', allowedExtensionIds: [ 'hadkkdmpcmllbkfopioopcmeapjchpbm', // Production (Chrome Web Store) 'lnhipaeaeiegnlokhokfokndgadkohfe', // Development (unpacked) ], extensionDiscovery: { extensionName: '@calycode | Extension', trustedAuthorPatterns: ['calycode', '@calycode', 'Mihály `@calycode`'], trustedHomepagePrefixes: [ 'https://extension.calycode.com', 'https://www.extension.calycode.com', ], requireNativeMessagingPermission: true, - mode: 'balanced' as 'strict' | 'balanced' | 'name-only', + mode: 'balanced' as const satisfies ExtensionDiscoveryMode, }, };Note: You may also export
ExtensionDiscoveryModeif it's needed elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/host-constants.ts` at line 22, Replace the inline type assertion on the "mode" value with a proper named type: define an exported type alias (e.g. ExtensionDiscoveryMode = 'strict' | 'balanced' | 'name-only') and use it for the mode property instead of the cast; update the constant's declaration (the "mode" property) to be typed as ExtensionDiscoveryMode so callers and consumers can reuse the exported type throughout the codebase.packages/cli/src/commands/opencode/native-host/discovery.ts (1)
351-372: ⚖️ Poor tradeoffVersion selection uses modification time instead of semantic versioning.
The
getLatestExtensionVersionPathfunction selects the "latest" extension version by sorting directories by modification time (newest first). While this may work in practice, Chrome extensions use semantic versioning in their directory names (e.g.,1.0.0_0). Consider whether semantic version comparison would be more accurate than mtime-based selection.This is likely acceptable for production use, but worth noting that:
- An older version directory touched more recently would be selected
- Development scenarios with manual file changes could yield unexpected results
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/opencode/native-host/discovery.ts` around lines 351 - 372, getLatestExtensionVersionPath currently chooses the newest directory by mtime which can pick an older semantic version touched recently; update it to parse semantic versions from the directory names (e.g., "1.2.3" or "1.2.3_0") and sort using proper semver comparison (use a semver library or implement a comparator that extracts numeric version parts) to pick the highest version from versionDirs, and keep the mtime-based fallback only if no valid semantic version can be parsed; reference getLatestExtensionVersionPath and the versionDirs array when making the change.packages/cli/src/commands/opencode/implementation.ts (1)
621-681: 💤 Low valueIndentation regression inside
startServer.The newly added
try { … }block uses 4-space indentation while the rest of the file is on 3 spaces (see surroundingif (serverProc)andserverProc.on(...)calls). Formatting only — please align with the existing style so diffs/reviews aren't noisy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/opencode/implementation.ts` around lines 621 - 681, The new try block inside startServer is indented with 4 spaces, causing an indentation regression versus the file's 3-space style; reformat the try { … } block (including its nested lines and the catch) to use 3-space indentation to match surrounding code (e.g., the startServer function, serverProc event handlers, and waitForServerReady logic) so the diff only contains logical changes.packages/cli/src/commands/opencode/native-host/setup.ts (4)
68-82: 💤 Low valueReplace
Record<string, any>with an explicit manifest type.Per the coding guidelines,
anyshould be avoided and object shapes defined via interfaces. The manifest schema is fixed (Chrome native messaging spec), so an explicit interface gives accurate types to bothcreateNativeHostManifestandwriteNativeHostManifests. As per coding guidelines: "Use interfaces for defining object shapes, types for unions and aliases" and "Prefer explicit typing overanytype in TypeScript".Proposed fix
+interface NativeHostManifest { + name: string; + description: string; + path: string; + type: 'stdio'; + allowed_origins: string[]; +} + -function createNativeHostManifest(manifestExePath: string, allowedExtensionIds: string[]): Record<string, any> { +function createNativeHostManifest(manifestExePath: string, allowedExtensionIds: string[]): NativeHostManifest { @@ function writeNativeHostManifests( platform: NodeJS.Platform, targets: NativeHostBrowserTarget[], - manifestContent: Record<string, any>, + manifestContent: NativeHostManifest, ): NativeHostBrowserTarget[] {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/opencode/native-host/setup.ts` around lines 68 - 82, Create an explicit interface (e.g., NativeHostManifest) matching the Chrome native messaging manifest schema (fields: name: string, description: string, path: string, type: 'stdio' | string, allowed_origins: string[] and any other optional manifest fields), then replace the uses of Record<string, any> with this interface: update the return type of createNativeHostManifest to NativeHostManifest and change the manifestContent parameter type in writeNativeHostManifests to NativeHostManifest (and adjust any related local variables/returns to use NativeHostManifest). Ensure imports/exports are added if needed and keep the runtime shape identical to the current implementation.
230-258: ⚡ Quick winStatus only validates the first target's manifest; other browsers' mismatches are silently ignored.
firstManifestPath = nativeHostTargets[0]?.manifestPathonly reads one manifest. If browser 0 isn't installed but Chrome and Brave are, you'll either show(none)or validate a stale Chromium manifest and miss a drift in the others. Iterate over targets whose manifest exists and compare each one'sallowed_originsagainstexpectedOrigins, recommending refresh if any of them drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/opencode/native-host/setup.ts` around lines 230 - 258, The current check only reads nativeHostTargets[0]?.manifestPath (firstManifestPath) and validates its manifestAllowedOrigins against expectedOrigins, which misses drifts in other targets; update the logic in the setup flow to iterate over nativeHostTargets, for each target with an existing manifestPath read and JSON-parse its allowed_origins (like manifestAllowedOrigins), compute missingOrigins per-target (expectedOrigins.filter(...) against that manifest's allowed_origins), collect any targets with missing origins, and if any exist push a descriptive line (including target identifier e.g., target.name or manifestPath) to lines and warn/recommend refresh (same message used now); keep the existing extensionResolution.ids and summary lines but ensure the per-target manifest checks replace the single-first-manifest check so no target's mismatch is silently ignored.
142-187: 💤 Low valueAdd JSDoc for the exported
setupNativeHostRegistrationAPI.This is a public, exported entry point used from
implementation.ts(and described inshowNativeHostStatusflows). The coding guidelines require JSDoc on public APIs with documented parameters/return values. Same applies toshowNativeHostStatusbelow. As per coding guidelines: "Add JSDoc comments for all public APIs and complex functions" and "Document all function parameters with types in JSDoc comments".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/opencode/native-host/setup.ts` around lines 142 - 187, Add a JSDoc comment for the exported function setupNativeHostRegistration describing its purpose, the parameter extensionIds?: string[] (explain it is an optional array of extension IDs and how it is used), and the return type Promise<SetupNativeHostResult> (describe the manifestExePath and extensionResolution fields). Also add matching JSDoc for the related exported showNativeHostStatus function. Ensure the JSDoc includes parameter types, descriptions, and a `@returns` block and place the comments immediately above each function declaration (setupNativeHostRegistration and showNativeHostStatus) so tooling and consumers (e.g., implementation.ts) pick up the documentation.
219-219: ⚡ Quick winReplace
execSyncwithspawnSync/execFileSyncfor the registry probe.OpenGrep flagged the interpolated command string here. While
target.registryKeyoriginates from internaltargets.ts(low risk today), the file already usesspawnwith an argument array for the matchingreg addcall (lines 115-118); usingspawnSync('reg', ['query', target.registryKey, '/ve'], …)here keeps the read path consistent and immune to future regressions if the key ever becomes user-influenced.Proposed fix
-import { spawn, execSync } from 'node:child_process'; +import { spawn, spawnSync } from 'node:child_process'; @@ - let registryConfigured = false; - try { - execSync(`reg query "${target.registryKey}" /ve`, { stdio: 'ignore', windowsHide: true }); - registryConfigured = true; - } catch { - registryConfigured = false; - } + const probe = spawnSync('reg', ['query', target.registryKey, '/ve'], { + stdio: 'ignore', + windowsHide: true, + }); + const registryConfigured = probe.status === 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/opencode/native-host/setup.ts` at line 219, The current execSync call that probes the registry using an interpolated command string should be changed to a safe argument-based child process call: replace execSync(`reg query "${target.registryKey}" /ve`, { stdio: 'ignore', windowsHide: true }) with a spawnSync (or execFileSync) invocation that passes 'reg' and an args array like ['query', target.registryKey, '/ve'] and the same options ({ stdio: 'ignore', windowsHide: true }); also ensure you import/require spawnSync from 'child_process' if not already present and follow the same pattern used for the existing reg add spawn calls around the reg-add logic (the lines that use spawn for reg add) to keep the registry read path consistent and safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/commands/oc-native-host.md`:
- Around line 54-56: Remove the stray closing code fence (```) that follows the
line "Run 'caly-xano <command> --help' for detailed usage." — either delete that
trailing ``` so the two lines remain plain text, or move the "Run 'caly-xano
<command> --help'..." line into the previous `term` block (before its closing
fence) so the fenced block is properly balanced; look for the trailing
triple-backtick and the text "Run 'caly-xano <command> --help'..." to locate and
fix it.
In `@packages/cli/esbuild.config.ts`:
- Around line 10-13: The defineEnvValue helper returns the literal string
'undefined' when process.env[name] is missing; change it so missing env vars are
emitted as an empty string so downstream parsers treat them as absent—update
defineEnvValue(name: string) to return JSON.stringify(value) when present and
JSON.stringify('') (i.e., an empty string) when value === undefined; this fixes
parseListEnv('undefined') and resolveEnvWithBuildFallback returning the string
'undefined' by ensuring they receive an empty value instead.
In `@packages/cli/src/commands/opencode/native-host/discovery.ts`:
- Line 572: Add a JSDoc comment above the public function
resolveWriteAllBrowserManifests describing its purpose, behavior, inputs and
outputs: state that it takes no parameters, returns a boolean indicating whether
browser manifests should be written, describe any side effects or conditions it
checks, and include `@returns` {boolean} and a short usage example or note about
when it is used; place this comment directly above the
resolveWriteAllBrowserManifests declaration so it documents the public API.
- Around line 456-465: The balanced-mode branch (config.mode === 'balanced')
accepts if trustSignals >= 1 OR hasNativeMessagingPermission but computes
confidence solely from trustSignals, which lets an extension with 0 trustSignals
+ native messaging be accepted at low confidence; decide which behavior you want
and implement it: either (A) require at least one trust signal to accept (change
accepted to trustSignals >= 1 || (trustSignals >= 1 &&
hasNativeMessagingPermission) — i.e., remove the OR with permission), or (B)
treat native messaging as an additional trust signal by computing an
effectiveSignals value (e.g., effectiveSignals = trustSignals +
(hasNativeMessagingPermission ? 1 : 0)), then set accepted = effectiveSignals >=
1 and compute confidence from effectiveSignals (high if >=2, medium if =1, low
if 0); update the code that sets accepted and confidence accordingly
(referencing config.mode, accepted, confidence, trustSignals,
hasNativeMessagingPermission).
- Line 515: Add a JSDoc comment above the public function
resolveAllowedExtensionIds describing its purpose, the providedIds parameter
(type and optional nature) and the structure of the ResolveExtensionIdsResult
return value; include `@param` for providedIds and `@returns` describing what
keys/fields the ResolveExtensionIdsResult contains and any behavior (e.g.,
defaults when providedIds is undefined) so the public API is properly documented
for consumers and tooling.
In `@packages/cli/src/commands/opencode/native-host/setup.ts`:
- Around line 23-47: The two unconditional Windows warnings should only show for
non-bundled (dev) runs: move the log.warn calls into the else branch that
handles !isBundled so they only execute when using the batch wrapper in
development. Update the code around isBundled (the branch that builds
wrapperContent for process.execPath vs process.argv[1]) to emit log.warn('Note:
Development mode on Windows uses a batch file wrapper.') and log.warn('If Native
Messaging fails, try building the bundled exe instead.') inside the else branch,
leaving wrapperPath creation, fs.writeFileSync, and the success log.info calls
as-is; ensure wrapperPath is still returned for both branches.
---
Outside diff comments:
In `@packages/cli/src/commands/opencode/implementation.ts`:
- Around line 1437-1463: setupOpencode currently resolves ocVersion but doesn't
pass it on or bake it into the generated wrapper, so init validates but discards
the value; modify setupOpencode to forward the resolvedVersion into
setupNativeHostRegistration (e.g. change call to
setupNativeHostRegistration(extensionIds, resolvedVersion)) and update the
wrapper-generation logic in setup.ts to append `--oc-version <resolvedVersion>`
into the emitted .sh/.bat commands so the native host receives the pinned
version (this ensures parseOcVersionFromArgv(process.argv) will see the value).
If the CLI should not support persisting ocVersion, instead remove the ocVersion
option from setupOpencode and callers to avoid the misleading behavior.
In `@packages/cli/src/commands/opencode/index.ts`:
- Around line 41-47: The init and serve action handlers call setupOpencode({
ocVersion: options.ocVersion }) but only declare (options) so they never see the
parent --oc-version; change both action callbacks to accept the second parameter
(commonly named command) and read the parent/global option via
command.parent?.opts()?.ocVersion (or command.optsWithGlobals().ocVersion) when
passing ocVersion into setupOpencode and similar calls so the parent oc
command's --oc-version value is used.
---
Nitpick comments:
In `@packages/cli/src/commands/opencode/implementation.ts`:
- Around line 621-681: The new try block inside startServer is indented with 4
spaces, causing an indentation regression versus the file's 3-space style;
reformat the try { … } block (including its nested lines and the catch) to use
3-space indentation to match surrounding code (e.g., the startServer function,
serverProc event handlers, and waitForServerReady logic) so the diff only
contains logical changes.
In `@packages/cli/src/commands/opencode/native-host/discovery.ts`:
- Around line 351-372: getLatestExtensionVersionPath currently chooses the
newest directory by mtime which can pick an older semantic version touched
recently; update it to parse semantic versions from the directory names (e.g.,
"1.2.3" or "1.2.3_0") and sort using proper semver comparison (use a semver
library or implement a comparator that extracts numeric version parts) to pick
the highest version from versionDirs, and keep the mtime-based fallback only if
no valid semantic version can be parsed; reference getLatestExtensionVersionPath
and the versionDirs array when making the change.
In `@packages/cli/src/commands/opencode/native-host/setup.ts`:
- Around line 68-82: Create an explicit interface (e.g., NativeHostManifest)
matching the Chrome native messaging manifest schema (fields: name: string,
description: string, path: string, type: 'stdio' | string, allowed_origins:
string[] and any other optional manifest fields), then replace the uses of
Record<string, any> with this interface: update the return type of
createNativeHostManifest to NativeHostManifest and change the manifestContent
parameter type in writeNativeHostManifests to NativeHostManifest (and adjust any
related local variables/returns to use NativeHostManifest). Ensure
imports/exports are added if needed and keep the runtime shape identical to the
current implementation.
- Around line 230-258: The current check only reads
nativeHostTargets[0]?.manifestPath (firstManifestPath) and validates its
manifestAllowedOrigins against expectedOrigins, which misses drifts in other
targets; update the logic in the setup flow to iterate over nativeHostTargets,
for each target with an existing manifestPath read and JSON-parse its
allowed_origins (like manifestAllowedOrigins), compute missingOrigins per-target
(expectedOrigins.filter(...) against that manifest's allowed_origins), collect
any targets with missing origins, and if any exist push a descriptive line
(including target identifier e.g., target.name or manifestPath) to lines and
warn/recommend refresh (same message used now); keep the existing
extensionResolution.ids and summary lines but ensure the per-target manifest
checks replace the single-first-manifest check so no target's mismatch is
silently ignored.
- Around line 142-187: Add a JSDoc comment for the exported function
setupNativeHostRegistration describing its purpose, the parameter extensionIds?:
string[] (explain it is an optional array of extension IDs and how it is used),
and the return type Promise<SetupNativeHostResult> (describe the manifestExePath
and extensionResolution fields). Also add matching JSDoc for the related
exported showNativeHostStatus function. Ensure the JSDoc includes parameter
types, descriptions, and a `@returns` block and place the comments immediately
above each function declaration (setupNativeHostRegistration and
showNativeHostStatus) so tooling and consumers (e.g., implementation.ts) pick up
the documentation.
- Line 219: The current execSync call that probes the registry using an
interpolated command string should be changed to a safe argument-based child
process call: replace execSync(`reg query "${target.registryKey}" /ve`, { stdio:
'ignore', windowsHide: true }) with a spawnSync (or execFileSync) invocation
that passes 'reg' and an args array like ['query', target.registryKey, '/ve']
and the same options ({ stdio: 'ignore', windowsHide: true }); also ensure you
import/require spawnSync from 'child_process' if not already present and follow
the same pattern used for the existing reg add spawn calls around the reg-add
logic (the lines that use spawn for reg add) to keep the registry read path
consistent and safe.
In `@packages/cli/src/utils/host-constants.ts`:
- Line 22: Replace the inline type assertion on the "mode" value with a proper
named type: define an exported type alias (e.g. ExtensionDiscoveryMode =
'strict' | 'balanced' | 'name-only') and use it for the mode property instead of
the cast; update the constant's declaration (the "mode" property) to be typed as
ExtensionDiscoveryMode so callers and consumers can reuse the exported type
throughout the codebase.
🪄 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: a5e72690-a7b0-477c-b9d2-af26447559b1
📒 Files selected for processing (11)
.changeset/calm-trains-wash.mddocs/commands/oc-init.mddocs/commands/oc-native-host.mdpackages/cli/esbuild.config.tspackages/cli/src/commands/opencode/implementation.tspackages/cli/src/commands/opencode/index.tspackages/cli/src/commands/opencode/native-host/discovery.tspackages/cli/src/commands/opencode/native-host/setup.tspackages/cli/src/commands/opencode/native-host/targets.tspackages/cli/src/index-bundled.tspackages/cli/src/utils/host-constants.ts
| Run 'caly-xano <command> --help' for detailed usage. | ||
| https://github.com/calycode/xano-tools | https://links.calycode.com/discord | ||
| ``` No newline at end of file | ||
| ``` |
There was a problem hiding this comment.
Remove extraneous closing code fence.
Lines 54-56 appear to have a stray closing code fence at line 56. The content "Run 'caly-xano..." typically appears inside documentation sections, not as a separate fenced block. Either:
- Remove the closing ``` at line 56 if lines 54-55 should be plain text, or
- Move lines 54-55 inside the previous
termblock (before line 52's closing fence)
📝 Proposed fix
Run 'caly-xano <command> --help' for detailed usage.
https://github.com/calycode/xano-tools | https://links.calycode.com/discord
-```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Run 'caly-xano <command> --help' for detailed usage. | |
| https://github.com/calycode/xano-tools | https://links.calycode.com/discord | |
| ``` | |
| \ No newline at end of file | |
| ``` | |
| Run 'caly-xano <command> --help' for detailed usage. | |
| https://github.com/calycode/xano-tools | https://links.calycode.com/discord |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/commands/oc-native-host.md` around lines 54 - 56, Remove the stray
closing code fence (```) that follows the line "Run 'caly-xano <command> --help'
for detailed usage." — either delete that trailing ``` so the two lines remain
plain text, or move the "Run 'caly-xano <command> --help'..." line into the
previous `term` block (before its closing fence) so the fenced block is properly
balanced; look for the trailing triple-backtick and the text "Run 'caly-xano
<command> --help'..." to locate and fix it.
| function defineEnvValue(name: string): string { | ||
| const value = process.env[name]; | ||
| return value === undefined ? 'undefined' : JSON.stringify(value); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how discovery.ts handles potentially 'undefined' string values
rg -n -A5 -B2 'BUILD_OC_DEFAULTS\.' packages/cli/src/commands/opencode/native-host/discovery.tsRepository: calycode/xano-tools
Length of output: 2742
🏁 Script executed:
#!/bin/bash
# Get context around defineEnvValue function
cat -n packages/cli/esbuild.config.ts | head -30Repository: calycode/xano-tools
Length of output: 1921
🏁 Script executed:
#!/bin/bash
# Find resolveEnvWithBuildFallback and related helpers
rg -n "resolveEnvWithBuildFallback|parseListEnvWithBuildFallback|parseBooleanEnvWithBuildFallback|defineEnvValue" packages/cli/src/commands/opencode/native-host/discovery.ts | head -20Repository: calycode/xano-tools
Length of output: 976
🏁 Script executed:
#!/bin/bash
# Find where defineEnvValue is defined and used
rg -n "defineEnvValue" packages/cli/Repository: calycode/xano-tools
Length of output: 1563
🏁 Script executed:
#!/bin/bash
# Get resolveEnvWithBuildFallback implementation
sed -n '88,100p' packages/cli/src/commands/opencode/native-host/discovery.tsRepository: calycode/xano-tools
Length of output: 399
🏁 Script executed:
#!/bin/bash
# Find BUILD_OC_DEFAULTS definition
rg -n "BUILD_OC_DEFAULTS\s*=" packages/cli/src/commands/opencode/native-host/discovery.ts -A 20Repository: calycode/xano-tools
Length of output: 1020
🏁 Script executed:
#!/bin/bash
# Get parseBooleanEnvWithBuildFallback and parseListEnvWithBuildFallback implementations
sed -n '102,125p' packages/cli/src/commands/opencode/native-host/discovery.tsRepository: calycode/xano-tools
Length of output: 721
🏁 Script executed:
#!/bin/bash
# Check if string 'undefined' is handled specially anywhere in discovery.ts
rg -n "'undefined'|\"undefined\"" packages/cli/src/commands/opencode/native-host/discovery.tsRepository: calycode/xano-tools
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find parseBooleanEnv and parseListEnv functions
rg -n "function parseBooleanEnv|function parseListEnv" packages/cli/src/commands/opencode/native-host/discovery.ts -A 10Repository: calycode/xano-tools
Length of output: 1599
🏁 Script executed:
#!/bin/bash
# Check imports to see if these parsing functions come from elsewhere
head -30 packages/cli/src/commands/opencode/native-host/discovery.tsRepository: calycode/xano-tools
Length of output: 1213
🏁 Script executed:
#!/bin/bash
# See how parseListEnvWithBuildFallback results are used - specifically the trusted* patterns
sed -n '168,190p' packages/cli/src/commands/opencode/native-host/discovery.tsRepository: calycode/xano-tools
Length of output: 967
🏁 Script executed:
#!/bin/bash
# Check if there are any filters for 'undefined' string value anywhere
rg -n "isValidExtensionId|filter|Boolean" packages/cli/src/commands/opencode/native-host/discovery.ts | head -20Repository: calycode/xano-tools
Length of output: 1131
Fix defineEnvValue to properly indicate missing environment variables.
The function returns the string 'undefined' when an environment variable is missing, but downstream parsing functions treat non-empty strings as valid values. This causes parseListEnv('undefined') to return ['undefined'] and resolveEnvWithBuildFallback to return the string 'undefined' instead of undefined, bypassing intended fallbacks.
Return an empty string '' instead of 'undefined' when a variable is missing. This allows existing parsing functions to correctly treat it as absent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/esbuild.config.ts` around lines 10 - 13, The defineEnvValue
helper returns the literal string 'undefined' when process.env[name] is missing;
change it so missing env vars are emitted as an empty string so downstream
parsers treat them as absent—update defineEnvValue(name: string) to return
JSON.stringify(value) when present and JSON.stringify('') (i.e., an empty
string) when value === undefined; this fixes parseListEnv('undefined') and
resolveEnvWithBuildFallback returning the string 'undefined' by ensuring they
receive an empty value instead.
| if (config.mode === 'name-only') { | ||
| accepted = true; | ||
| confidence = 'low'; | ||
| } else if (config.mode === 'strict') { | ||
| accepted = trustSignals >= 2; | ||
| confidence = accepted ? 'high' : 'low'; | ||
| } else { | ||
| accepted = trustSignals >= 1 || hasNativeMessagingPermission; | ||
| confidence = trustSignals >= 2 ? 'high' : trustSignals >= 1 ? 'medium' : 'low'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Clarify balanced mode acceptance criteria for extensions with only native messaging permission.
In balanced mode (lines 463-464), an extension is accepted if it has either 1+ trust signals or the native messaging permission, but the confidence is assigned based only on trust signal count. This means:
- An extension with 0 trust signals but native messaging permission is accepted with low confidence
- This is more permissive than requiring at least one trust signal
Confirm this behavior is intentional. If the native messaging permission alone should not be sufficient for acceptance, consider:
- accepted = trustSignals >= 1 || hasNativeMessagingPermission;
+ accepted = trustSignals >= 1;
confidence = trustSignals >= 2 ? 'high' : trustSignals >= 1 ? 'medium' : 'low';Alternatively, if the permission should count as a trust signal, adjust the confidence calculation accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/opencode/native-host/discovery.ts` around lines 456
- 465, The balanced-mode branch (config.mode === 'balanced') accepts if
trustSignals >= 1 OR hasNativeMessagingPermission but computes confidence solely
from trustSignals, which lets an extension with 0 trustSignals + native
messaging be accepted at low confidence; decide which behavior you want and
implement it: either (A) require at least one trust signal to accept (change
accepted to trustSignals >= 1 || (trustSignals >= 1 &&
hasNativeMessagingPermission) — i.e., remove the OR with permission), or (B)
treat native messaging as an additional trust signal by computing an
effectiveSignals value (e.g., effectiveSignals = trustSignals +
(hasNativeMessagingPermission ? 1 : 0)), then set accepted = effectiveSignals >=
1 and compute confidence from effectiveSignals (high if >=2, medium if =1, low
if 0); update the code that sets accepted and confidence accordingly
(referencing config.mode, accepted, confidence, trustSignals,
hasNativeMessagingPermission).
| }; | ||
| } | ||
|
|
||
| function resolveAllowedExtensionIds(providedIds?: string[]): ResolveExtensionIdsResult { |
There was a problem hiding this comment.
Add JSDoc documentation for public API function.
The resolveAllowedExtensionIds function is a public API but lacks JSDoc comments. According to coding guidelines, all public APIs must have JSDoc documentation including parameter and return value descriptions.
📝 Proposed documentation
+/**
+ * Resolve the list of allowed extension IDs for the native host.
+ *
+ * `@param` providedIds - Optional array of manually specified extension IDs. If provided,
+ * these IDs are validated and returned without discovery.
+ * `@returns` Result object containing the resolved extension IDs, matched candidates,
+ * and the source of the IDs (manual, discovery mode, or discovery+known).
+ * `@throws` Error if any provided ID is invalid (must match Chrome extension ID format).
+ */
function resolveAllowedExtensionIds(providedIds?: string[]): ResolveExtensionIdsResult {As per coding guidelines, public APIs must be documented with JSDoc comments including parameter and return value descriptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/opencode/native-host/discovery.ts` at line 515, Add
a JSDoc comment above the public function resolveAllowedExtensionIds describing
its purpose, the providedIds parameter (type and optional nature) and the
structure of the ResolveExtensionIdsResult return value; include `@param` for
providedIds and `@returns` describing what keys/fields the
ResolveExtensionIdsResult contains and any behavior (e.g., defaults when
providedIds is undefined) so the public API is properly documented for consumers
and tooling.
| }; | ||
| } | ||
|
|
||
| function resolveWriteAllBrowserManifests(): boolean { |
There was a problem hiding this comment.
Add JSDoc documentation for public API function.
The resolveWriteAllBrowserManifests function is a public API but lacks JSDoc comments.
📝 Proposed documentation
+/**
+ * Resolve whether to write native host manifests for all supported browsers.
+ *
+ * `@returns` true if manifests should be written for all browsers, false otherwise.
+ * Reads from CALY_OC_WRITE_ALL_BROWSER_MANIFESTS or CALY_BUILD_OC_WRITE_ALL_BROWSER_MANIFESTS.
+ */
function resolveWriteAllBrowserManifests(): boolean {As per coding guidelines, public APIs must be documented with JSDoc comments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function resolveWriteAllBrowserManifests(): boolean { | |
| /** | |
| * Resolve whether to write native host manifests for all supported browsers. | |
| * | |
| * `@returns` true if manifests should be written for all browsers, false otherwise. | |
| * Reads from CALY_OC_WRITE_ALL_BROWSER_MANIFESTS or CALY_BUILD_OC_WRITE_ALL_BROWSER_MANIFESTS. | |
| */ | |
| function resolveWriteAllBrowserManifests(): boolean { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/opencode/native-host/discovery.ts` at line 572, Add
a JSDoc comment above the public function resolveWriteAllBrowserManifests
describing its purpose, behavior, inputs and outputs: state that it takes no
parameters, returns a boolean indicating whether browser manifests should be
written, describe any side effects or conditions it checks, and include `@returns`
{boolean} and a short usage example or note about when it is used; place this
comment directly above the resolveWriteAllBrowserManifests declaration so it
documents the public API.
| if (isWin) { | ||
| const wrapperDir = path.join(homeDir, '.calycode', 'bin'); | ||
| if (!fs.existsSync(wrapperDir)) { | ||
| fs.mkdirSync(wrapperDir, { recursive: true }); | ||
| } | ||
|
|
||
| const wrapperPath = path.join(wrapperDir, 'calycode-host.bat'); | ||
| let wrapperContent = ''; | ||
| const isBundled = process.execPath.toLowerCase().endsWith('caly.exe') || (process as any).pkg; | ||
|
|
||
| if (isBundled) { | ||
| wrapperContent = `@echo off\r\n`; | ||
| wrapperContent += `"${process.execPath}" opencode native-host %*\r\n`; | ||
| } else { | ||
| wrapperContent = `@echo off\r\n`; | ||
| wrapperContent += `"${process.execPath}" "${process.argv[1]}" opencode native-host %*\r\n`; | ||
| } | ||
|
|
||
| fs.writeFileSync(wrapperPath, wrapperContent); | ||
| log.info(`Created wrapper script: ${wrapperPath}`); | ||
| log.info(`Wrapper content: ${wrapperContent.trim()}`); | ||
| log.warn('Note: Development mode on Windows uses a batch file wrapper.'); | ||
| log.warn('If Native Messaging fails, try building the bundled exe instead.'); | ||
| return wrapperPath; | ||
| } |
There was a problem hiding this comment.
Dev-mode warning is shown unconditionally on Windows, even for bundled installs.
log.warn('Note: Development mode on Windows uses a batch file wrapper.') and the follow-up suggestion to "try building the bundled exe instead" fire after both the isBundled and else branches, so users running the bundled caly.exe are incorrectly told they're in development mode and prompted to rebuild. Move these warnings inside the else (non-bundled) branch.
Proposed fix
if (isBundled) {
wrapperContent = `@echo off\r\n`;
wrapperContent += `"${process.execPath}" opencode native-host %*\r\n`;
} else {
wrapperContent = `@echo off\r\n`;
wrapperContent += `"${process.execPath}" "${process.argv[1]}" opencode native-host %*\r\n`;
}
fs.writeFileSync(wrapperPath, wrapperContent);
log.info(`Created wrapper script: ${wrapperPath}`);
log.info(`Wrapper content: ${wrapperContent.trim()}`);
- log.warn('Note: Development mode on Windows uses a batch file wrapper.');
- log.warn('If Native Messaging fails, try building the bundled exe instead.');
+ if (!isBundled) {
+ log.warn('Note: Development mode on Windows uses a batch file wrapper.');
+ log.warn('If Native Messaging fails, try building the bundled exe instead.');
+ }
return wrapperPath;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isWin) { | |
| const wrapperDir = path.join(homeDir, '.calycode', 'bin'); | |
| if (!fs.existsSync(wrapperDir)) { | |
| fs.mkdirSync(wrapperDir, { recursive: true }); | |
| } | |
| const wrapperPath = path.join(wrapperDir, 'calycode-host.bat'); | |
| let wrapperContent = ''; | |
| const isBundled = process.execPath.toLowerCase().endsWith('caly.exe') || (process as any).pkg; | |
| if (isBundled) { | |
| wrapperContent = `@echo off\r\n`; | |
| wrapperContent += `"${process.execPath}" opencode native-host %*\r\n`; | |
| } else { | |
| wrapperContent = `@echo off\r\n`; | |
| wrapperContent += `"${process.execPath}" "${process.argv[1]}" opencode native-host %*\r\n`; | |
| } | |
| fs.writeFileSync(wrapperPath, wrapperContent); | |
| log.info(`Created wrapper script: ${wrapperPath}`); | |
| log.info(`Wrapper content: ${wrapperContent.trim()}`); | |
| log.warn('Note: Development mode on Windows uses a batch file wrapper.'); | |
| log.warn('If Native Messaging fails, try building the bundled exe instead.'); | |
| return wrapperPath; | |
| } | |
| if (isWin) { | |
| const wrapperDir = path.join(homeDir, '.calycode', 'bin'); | |
| if (!fs.existsSync(wrapperDir)) { | |
| fs.mkdirSync(wrapperDir, { recursive: true }); | |
| } | |
| const wrapperPath = path.join(wrapperDir, 'calycode-host.bat'); | |
| let wrapperContent = ''; | |
| const isBundled = process.execPath.toLowerCase().endsWith('caly.exe') || (process as any).pkg; | |
| if (isBundled) { | |
| wrapperContent = `@echo off\r\n`; | |
| wrapperContent += `"${process.execPath}" opencode native-host %*\r\n`; | |
| } else { | |
| wrapperContent = `@echo off\r\n`; | |
| wrapperContent += `"${process.execPath}" "${process.argv[1]}" opencode native-host %*\r\n`; | |
| } | |
| fs.writeFileSync(wrapperPath, wrapperContent); | |
| log.info(`Created wrapper script: ${wrapperPath}`); | |
| log.info(`Wrapper content: ${wrapperContent.trim()}`); | |
| if (!isBundled) { | |
| log.warn('Note: Development mode on Windows uses a batch file wrapper.'); | |
| log.warn('If Native Messaging fails, try building the bundled exe instead.'); | |
| } | |
| return wrapperPath; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/opencode/native-host/setup.ts` around lines 23 -
47, The two unconditional Windows warnings should only show for non-bundled
(dev) runs: move the log.warn calls into the else branch that handles !isBundled
so they only execute when using the batch wrapper in development. Update the
code around isBundled (the branch that builds wrapperContent for
process.execPath vs process.argv[1]) to emit log.warn('Note: Development mode on
Windows uses a batch file wrapper.') and log.warn('If Native Messaging fails,
try building the bundled exe instead.') inside the else branch, leaving
wrapperPath creation, fs.writeFileSync, and the success log.info calls as-is;
ensure wrapperPath is still returned for both branches.
|
Addressed the review feedback in this branch and pushed commit d85fb7a. Implemented fixes include:
Validation run: |
1 participant
Summary by CodeRabbit
Release Notes
New Features
--oc-versionCLI option to specify OpenCode versions at runtimeDocumentation
oc initdocumentation with configuration precedence and available build-time/runtime variablesoc native-hostcommand documentation with subcommand details