Reagan/opencode integration#401
Merged
Merged
Conversation
opencode integration with kimi
BrowserCode emits step_finish for both tool-call continuation and final stop states. The adapter now treats usage events separately from terminal completion, and runEngine suppresses duplicate done events defensively while logging enough spawn context to debug missing CLI/PATH failures. Constraint: OpenCode JSON events use step_finish.reason=tool-calls for continuing turns and reason=stop for final completion. Rejected: Treat every step_finish as terminal | causes repeated done states after tool calls. Confidence: high Scope-risk: narrow Directive: Do not emit done from BrowserCode step_finish unless the reason is explicitly stop; clean exits without done are handled by runEngine. Tested: npm run typecheck; npm run lint; npm test
BrowserCode now carries provider/model selection through settings, the runner, session persistence, and the hub/pill UI so sessions can be diagnosed by the exact model they started with. The provider list is constrained to model IDs verified against OpenCode's current Models.dev registry, and provider API-key tests now respect OpenAI-compatible versus Anthropic-compatible endpoints. Constraint: BrowserCode/OpenCode provider support is registry-driven and includes both OpenAI-compatible and Anthropic-compatible providers. Constraint: Raw API keys and tokens must never be logged. Rejected: Keep MiniMax behind the OpenAI-compatible test path | OpenCode models MiniMax through an Anthropic-compatible provider endpoint. Confidence: medium Scope-risk: moderate Directive: Do not add BrowserCode model IDs without checking the current OpenCode/Models.dev provider registry and endpoint protocol. Tested: npm run typecheck Tested: npm run lint Tested: npm test Tested: git diff --check Tested: configured BrowserCode model IDs resolve against https://models.dev/api.json Not-tested: Authenticated live completions for Qwen and MiniMax without user-provided provider keys
Provider tool streams do not all expose equivalent file-write metadata, so the runner now snapshots and watches the actual harness files instead of treating tool-call arguments as proof. This keeps Claude Code, BrowserCode, Codex, and future engines on the same source of truth while preserving skill read/write post-processing. Constraint: BrowserCode currently flattens tool input into tool_result rows, so adapter metadata is incomplete for harness edit detection Rejected: Patch only the BrowserCode adapter | would keep harness reporting dependent on provider-specific event shapes Confidence: high Scope-risk: narrow Directive: Keep harness_edited tied to real file content changes, not model/tool claims Tested: yarn typecheck; yarn eslint src/main/hl/engines/runEngine.ts tests/unit/hl/runEngineHarnessWatcher.test.ts; yarn vitest run tests/unit/hl/runEngineHarnessWatcher.test.ts; yarn test Not-tested: Live packaged Electron run with real BrowserCode provider
Engine adapters and auth flows were each carrying their own subprocess spawning logic, which made Windows shim handling and model attribution easy to drift. Introduce a shared CLI spawn/capture helper, route non-PTY engine CLI calls through it, and pass selected provider/model through a generic run context so BrowserCode is no longer special-cased in the adapter contract. Add a Linux POSIX spawn smoke alongside the existing Windows shim smoke. Constraint: Windows npm shims need resolveCliSpawn while Linux must stay shell-free and PATH-based Rejected: Keep per-adapter spawn helpers | repeats platform-specific quoting and timeout behavior Confidence: high Scope-risk: moderate Directive: Route non-PTY engine CLI subprocesses through cliSpawn so platform behavior stays centralized Tested: yarn typecheck; yarn eslint tests/unit/hl/codexStdinLinux.test.ts tests/unit/hl/codexStdinWindows.test.ts src/main/hl/engines/cliSpawn.ts src/main/hl/engines/pathEnrich.ts; yarn vitest run tests/unit/hl/codexStdinLinux.test.ts tests/unit/hl/codexStdinWindows.test.ts; yarn test Not-tested: Native Linux/Windows smoke execution locally; platform-gated tests run on CI runners
- Drop the standalone BrowserCode model chip from TaskInput and Pill - Embed a provider/model side flyout into EnginePicker on the BrowserCode row - Categorize ConnectionsPane into Providers and Connections sections - Add Test button + mock failure modes (mock:invalid, mock:credits, etc.) for BrowserCode keys - Map raw provider errors to friendly messages (out of credits, invalid key, rate limit, network) - Deep-link unconfigured provider clicks to Settings via focusBrowserCodeProvider IPC - Route settings:open through the in-app SettingsPane overlay instead of the empty standalone window - Disambiguate OpenAI buttons when both API key and ChatGPT OAuth are saved - Hide model badge on Claude Code/Codex sessions; only show for BrowserCode - Stop pill from over-expanding when engine dropdown opens - Fix dashboard z-index so model dropdown renders above stat cards
Comment on lines
+23
to
+28
| return spawn(resolved.command, resolved.args, { | ||
| cwd: opts.cwd, | ||
| env, | ||
| stdio, | ||
| ...resolved.spawnOptions, | ||
| }) as ChildProcessWithoutNullStreams; |
There was a problem hiding this comment.
6 issues found across 44 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/renderer/hub/BrowserCodeModelPicker.tsx">
<violation number="1" location="app/src/renderer/hub/BrowserCodeModelPicker.tsx:282">
P2: Use the provider default model as a fallback in the submenu so the active model is highlighted correctly when `lastModel` is unset.</violation>
</file>
<file name="app/src/main/hl/engines/cliSpawn.ts">
<violation number="1" location="app/src/main/hl/engines/cliSpawn.ts:44">
P1: Timeout logic can hang indefinitely because it only sends `SIGTERM` and never resolves on timeout if the process does not exit.</violation>
</file>
<file name="app/src/main/hl/engines/browsercode/adapter.ts">
<violation number="1" location="app/src/main/hl/engines/browsercode/adapter.ts:93">
P1: Passing the wrapped multi-line prompt via argv can break BrowserCode on Windows `.cmd` shims. Use stdin payload mode instead of `args.push(wrappedPrompt)` for this adapter.</violation>
</file>
<file name="app/src/main/index.ts">
<violation number="1" location="app/src/main/index.ts:1252">
P2: Avoid info-level logging in the `sessions:view-resize` hot path; this handler runs every frame and can cause resize jank and log spam.</violation>
</file>
<file name="app/src/main/settings/apiKeyIpc.ts">
<violation number="1" location="app/src/main/settings/apiKeyIpc.ts:428">
P1: Development/testing backdoor left in production code. The `mockTestResult` function allows anyone to bypass real API key validation by entering `mock:ok` as their key. This should be gated behind a `NODE_ENV` / dev-mode check, or removed entirely before shipping.</violation>
</file>
<file name="app/src/main/hl/engines/installer.ts">
<violation number="1" location="app/src/main/hl/engines/installer.ts:43">
P1: Windows script construction breaks for commands containing `&&`. The browsercode installer command uses `&&` internally, but `openWindowsTerminal` joins all parts with ` & `. Since cmd.exe treats `&&` as a higher-precedence conditional operator, the `echo` line will be split and `bash` will run prematurely as a conditional. Wrap the command and echo in parentheses or use a different composition strategy.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
The reported issues shared a failure mode where engine integration state could be misleading: model defaults were not reflected in the UI, BrowserCode prompts traveled through argv on Windows, timeouts could wait forever, Windows installer command composition was fragile, resize logging ran in a hot path, and production builds still accepted mock API-key checks. The fixes normalize those edges and add targeted regressions around the risky behavior. Constraint: Windows command shims and cmd.exe parsing treat argv and command composition differently from POSIX shells Constraint: Packaged builds must not accept development-only mock API-key results Rejected: Keep BrowserCode prompts in argv | multi-line prompts can break through Windows .cmd shims Rejected: Only send SIGTERM on timeout | children may ignore it and leave callers unresolved Confidence: high Scope-risk: moderate Reversibility: clean Tested: npm test -- tests/unit/hl/cliSpawn.test.ts tests/unit/hl/browsercodeAdapter.test.ts tests/unit/hub/BrowserCodeModelPicker.spec.tsx tests/unit/settings/apiKeyIpc.test.ts Tested: npm run typecheck Tested: npm run lint -- --quiet Tested: git diff --check Not-tested: Manual Windows terminal installer launch on a real Windows host
Collaborator
Author
|
@cubic review |
There was a problem hiding this comment.
5 issues found across 48 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/renderer/hub/EnginePicker.tsx">
<violation number="1" location="app/src/renderer/hub/EnginePicker.tsx:203">
P2: Prevent repeated clicks from re-triggering install/login while that engine is already in progress.</violation>
</file>
<file name="app/src/main/identity/authStore.ts">
<violation number="1" location="app/src/main/identity/authStore.ts:252">
P3: Provider ID is validated with `.trim()` but stored untrimmed, inconsistent with `saveBrowserCodeKey` which trims before using as key. If persisted data contains whitespace in a provider key, it will survive normalization but won't match subsequent lookups.</violation>
</file>
<file name="app/src/main/sessions/BrowserPool.ts">
<violation number="1" location="app/src/main/sessions/BrowserPool.ts:445">
P2: Bounds fitting uses a computed zoom from height instead of the current webContents zoom, so resize centering can drift after window/manual zoom changes.</violation>
</file>
<file name="app/src/main/hl/engines/claude-code/adapter.ts">
<violation number="1" location="app/src/main/hl/engines/claude-code/adapter.ts:74">
P3: Propagate `r.error` here so spawn/timeout failures are not mislabeled as a normal auth failure.</violation>
</file>
<file name="app/src/main/hl/engines/browsercode/adapter.ts">
<violation number="1" location="app/src/main/hl/engines/browsercode/adapter.ts:222">
P2: Tool timing is broken: `pendingTools` is read/deleted but never populated, so `tool_result.ms` is always near zero.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Repeated setup clicks, unnormalized BrowserCode provider keys, stale zoom math, dropped Claude auth transport errors, and BrowserCode tool timing all came from transient state being treated as display-only or optional. The fix records the state at the boundary where the app observes it, then uses that state for UI guards, credential lookup, browser centering, auth errors, and tool durations. Constraint: BrowserCode may emit either start-plus-terminal tool_use events or only a terminal tool_use event Constraint: Browser view centering must preserve user/manual zoom after attach Rejected: Rely on React state alone for click suppression | repeated clicks can arrive before state renders Rejected: Infer BrowserCode duration without a start event | no earlier timestamp exists in that event shape Confidence: high Scope-risk: moderate Reversibility: clean Tested: npm test -- --reporter=dot tests/unit/hub/EnginePicker.spec.tsx tests/unit/sessions/BrowserPool.test.ts tests/unit/hl/browsercodeAdapter.test.ts tests/unit/hl/claudeCodeAdapter.test.ts tests/unit/identity/authStore.test.ts tests/unit/hl/cliSpawn.test.ts Tested: npm run typecheck Tested: npm run lint -- --quiet Tested: git diff --check Not-tested: Manual BrowserCode run with real terminal-only tool event stream
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/hl/engines/browsercode/adapter.ts">
<violation number="1" location="app/src/main/hl/engines/browsercode/adapter.ts:261">
P2: Cancelled tool executions are incorrectly marked as successful (`ok: true`).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic.
BrowserCode already classified cancelled tool_use events as terminal, but the normalized result predicate only rejected error and failed statuses. That made user-cancelled tools look successful in the event stream. Split terminal-state detection from success classification so cancelled/canceled and explicit error payloads produce ok=false while preserving timing cleanup. Constraint: BrowserCode may spell cancellation as cancelled or canceled Rejected: Keep status checks inline | terminal and success semantics drifted when new statuses were added Confidence: high Scope-risk: narrow Reversibility: clean Tested: npm test -- --reporter=dot tests/unit/hl/browsercodeAdapter.test.ts Tested: npm run typecheck Tested: npm run lint -- --quiet Tested: git diff --check
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.
Add browsercode (opencode fork) with many edits.
Currently kimi, qwen, and minimax models are added.
runs via bcode within cli with api key auth
also normalized a few things, such as cli spawning.
and added a spinner that shouldve been in a different pr but it's fine haha
Summary by cubic
Adds a new
browsercodeengine with provider/model selection and API key management, standardizes CLI spawning across platforms, and improves reliability so sessions show the exact model used and failures are easier to diagnose.New Features
browsercodeadapter wrappingbcode run --format json; providers: Moonshot Kimi, Alibaba Qwen, MiniMax; uses our Electron harness (no nativebrowser_execute).modeland hub/logs show engine/model; engine install via IPC forclaude-code,codex,browsercode; pill remembers selected engine; inline “thinking” spinner in Terminal.Refactors
cliSpawn(spawnCli/runCliCapture); PATH enrichment adds~/.bcode/bin; Windows/POSIX shim fixes; stdin-based prompt on Windows; timeouts resolve even if children ignore SIGTERM; Linux spawn smoke in CI.step_finishas done whenreason=stop; dedupe done events and watch actual harness files for edits; map provider errors to friendly messages; production builds disallow BrowserCode mock API-key tests; deterministic setup/parser state (dedupe setup clicks, normalize provider ids/keys, preserve manual-zoom centering, capture Claude auth transport errors, handle terminal-onlytool_usetiming); cancelled/canceledtool_usenow reports as unsuccessful.Written for commit 53d1787. Summary will update on new commits.