fix(cli): surface real --cloud-fork import failure reasons - #12388
Conversation
Add unit tests pinning the behavior of importCloudSession's HTTP/parse handling. Two cases are red against the current implementation (a server HTTP error and a missing local id are silently swallowed as undefined instead of surfaced), one confirms the success path, and one guards that raw fetch exceptions keep propagating. The two failing cases will go green once importCloudSession is taught to throw on error.
importCloudSession previously returned undefined on any failure, silently swallowing the server's HTTP error (401/403/404/500) or a missing local id. Teach it to throw instead: with the server's error message on an HTTP error, or a clear message when the response is malformed. The four cloud-fork call sites keep branching on a missing id, but now reach that branch via catch rather than via a swallowed undefined, so the failure reason can be surfaced by callers.
… entry The Kilo-mirror thread entry swallowed import errors via a .catch(() => undefined) that returned empty, hiding the real 401/403/404/500 cause. Replace it with a try/catch that logs the underlying error at debug level and prints it to stderr, so a failed --cloud-fork import now reports why it failed instead of failing silently.
The three shared upstream cloud-fork entry points (attach, run, tui thread) each swallowed import failures via a .catch(() => undefined) that lost the underlying 401/403/404/500 or network error. Wrap each call in a try/catch that logs the cause at debug level and prints it to stderr, preserving each site's existing exit semantics (process.exit / shutdownAndExit / early return). This keeps the diff to shared opencode files scoped inside the existing kilocode_change blocks.
…port errors Adds the patch changeset describing the user-visible fix (failed --cloud-fork imports now report the underlying reason in both the user-visible message and the DEBUG log stream), and adds the required kilocode_change markers around the new Log/errorMessage imports and module-level loggers in the shared upstream files (run.ts, attach.ts) so the annotation CI check passes.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * error, or with "cloud session import returned no session id" when the | ||
| * response was malformed. | ||
| */ | ||
| export async function importCloudSession( |
There was a problem hiding this comment.
SUGGESTION: Consider consolidating the duplicated try/catch pattern
The try/catch + log.debug("failed to import cloud session", { err }) + UI.error(\Failed to import session from cloud: ${errorMessage(err)}`) sequence is repeated near-verbatim across four call sites, three of which are shared upstream files (run.ts, attach.ts, thread.ts). Since each site's only real difference is the exit mechanism (process.exit, process.exitCode, shutdownAndExit), a small helper here (e.g. importCloudSessionOrThrowthat logs before rethrowing, or a shareddescribeCloudImportError(err)formatter) could shrink the diff each shared file carries against upstream — this repo's stated top priority for files outsidekilocode`-named paths.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Done — addressed in commit 7138f93. Extracted a shared
eportCloudImportError(err)\ helper in \packages/opencode/src/kilocode/cloud-session.ts\ that logs the failure at DEBUG, prints the real reason via \UI.error, then rethrows. The four call sites now delegate to it and keep their own exit semantics, so the three shared upstream files only carry a single marked import instead of a duplicated try/catch — shrinking the diff against upstream as suggested.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of commit The two previously-flagged threads on Files Reviewed in This Update (2 files)
Previous Review Summaries (5 snapshots, latest commit 1157c73)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1157c73)Status: No Issues Found | Recommendation: Merge Incremental review of commit The two previously-flagged threads on Files Reviewed in This Update (2 files)
Previous review (commit f3f8401)Status: No Issues Found | Recommendation: Merge Incremental review of commit The two previously-flagged threads on Files Reviewed in This Update (2 files)
Previous review (commit c99ce90)Status: No Issues Found | Recommendation: Merge The previously flagged WARNING ( Files Reviewed in This Update (2 files)
Previous review (commit 7138f93)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed in This Update (7 files)
The previously flagged SUGGESTION (consolidate duplicated try/catch/log/UI.error across the four call sites) was addressed in commit Fix these issues in Kilo Cloud Previous review (commit b8c8a10)Status: 1 Issue Found | Recommendation: Merge (optional cleanup suggested) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (7 files)
Core logic in Reviewed by claude-sonnet-5 · Input: 8 · Output: 851 · Cached: 133.3K Review guidance: REVIEW.md from base branch |
Resolves the three merge conflicts from the upstream TUI extraction into packages/tui: - src/cli/cmd/run.ts: keep the Kilo cli.run logger, drop the removed runtimeTask dynamic import, repoint errorMessage to @opencode-ai/tui/util/error. - src/kilocode/cli/cmd/tui/thread.ts: keep the Kilo.tui.thread logger, adopt upstream's @opencode-ai/tui TuiInput/StartInput types, repoint errorMessage. - src/cli/cmd/tui/attach.ts: accept upstream's deletion (attach moved to src/cli/cmd/attach.ts) and re-apply the cloud-fork import error-surfacing fix in the new location. - src/cli/cmd/tui.ts: add the Log namespace import required by the cloud-fork fix auto-merged from the old shared tui/thread.ts. All kilocode_change markers preserved; typecheck, annotation check, and cloud-session tests pass.
| * its own exit semantics (`process.exit` / `exitCode` / `shutdownAndExit` / | ||
| * typed `return`). The return type `never` reflects that it always rethrows. | ||
| */ | ||
| export function reportCloudImportError(err: unknown): void { |
There was a problem hiding this comment.
WARNING: reportCloudImportError is typed void but the docstring says its return type should be never since it always rethrows — and that mismatch has a real effect: every one of the four call sites (run.ts, attach.ts, tui.ts, kilocode/cli/cmd/tui/thread.ts) calls this then follows with process.exit(1) / process.exitCode = 1; return / shutdownAndExit(...), which is now unreachable dead code — the throw err fires first. The intended per-site exit semantics no longer run explicitly; instead the caller's async handler rejects and the exit code/behavior depends on how that rejection is handled upstream (or isn't, for the plain cmd()-registered handlers like attach.ts and tui.ts which have no wrapping try/catch or global rejection handler), rather than the clean, deterministic exit the docstring claims to preserve.
Declaring reportCloudImportError(err: unknown): never (matching the existing die/dieInteractive pattern in run.ts) would make TypeScript flag the now-dead lines after each call, or the call sites could just drop the redundant process.exit(1) etc. and rely on the throw, matching this repo's other never-typed helpers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Good catch on the dead-code mismatch — fixed (helper is now void, call sites keep their own exits, docstring matches).
Deliberate deviation from the literal : never: a throw would (1) drop tui.ts's shutdownAndExit TUI-worker teardown and orphan the subprocess, (2) break kilo-mirror/thread.ts's { ok: false } contract that attach() relies on, and (3) double-print via src/index.ts:148's top-level catch.
With void + per-site exit, behavior is identical to the earlier inline version: single message, process.exit/exitCode/shutdownAndExit all still run, no dead code.
Net: dead code gone, docstring accurate, graceful per-site exits preserved.
Happy to switch to : never if you prefer the throw style, but I'd first guard the worker teardown and avoid the double-print.
Extract a shared reportCloudImportError helper in kilocode/cloud-session.ts so the four cloud-fork import sites surface the real failure (HTTP status/body or fetch error) consistently via UI.error plus a DEBUG log, instead of duplicating that logic at each call site.
7138f93 to
c99ce90
Compare
|
Heads up on the main merge #12329 ("repair cloud session imports") while this branch was open — it changed the gateway's error contract so failures now return { error: "..." } for 400, 500, and the existing 404. Our old errorMessage(result.error) only reads .message, so it would've printed [object Object] and hidden the real reason. In commit f3f8401 I switched importCloudSession to read result.error.error (via importErrorReason(), falling back to errorMessage), added a test for the new shape, and kept all four call-site exits unchanged. |
* fix(tui): prevent home wordmark corruption in height-constrained terminals (Kilo-Org#13069) * feat(prompt): mode-specific input placeholders (Kilo-Org#12388) * fix(tui): keep /share available to copy existing link (Kilo-Org#12532) * fix(tui): dismiss dialogs with ctrl+c (Kilo-Org#12884) * fix(app): terminal resize * fix(console): translations * fix(app): terminal PTY buffer carryover * fix(app): notifications on child sessions * Revert "feat(desktop): add WSL backend mode (Kilo-Org#12914)" This reverts commit 213a872. * release: v1.1.58 * refactor: kilo compat for v1.1.58 --------- Co-authored-by: Kit Langton <kit.langton@gmail.com> Co-authored-by: Adam <2363879+adamdotdevin@users.noreply.github.com> Co-authored-by: opencode <opencode@sst.dev>
…oud-session imports Take upstream/main's pattern of removing top-level static imports for Kilo implementations (createKiloClient, cloud-session, run-auto, headless, KiloRun, KiloTuiThreadDaemon) and letting them be dynamically imported inside handlers. Add reportCloudImportError to each handler's existing dynamic cloud-session import so the cloud-fork error display from issue Kilo-Org#12381 still resolves the exported symbol.
606cbfc to
9a465cb
Compare
|
Thanks for the fix! merged. |
* fix(tui): prevent home wordmark corruption in height-constrained terminals (Kilo-Org#13069) * feat(prompt): mode-specific input placeholders (Kilo-Org#12388) * fix(tui): keep /share available to copy existing link (Kilo-Org#12532) * fix(tui): dismiss dialogs with ctrl+c (Kilo-Org#12884) * fix(app): terminal resize * fix(console): translations * fix(app): terminal PTY buffer carryover * fix(app): notifications on child sessions * Revert "feat(desktop): add WSL backend mode (Kilo-Org#12914)" This reverts commit e63699f. * release: v1.1.58 * refactor: kilo compat for v1.1.58 --------- Co-authored-by: Kit Langton <kit.langton@gmail.com> Co-authored-by: Adam <2363879+adamdotdevin@users.noreply.github.com> Co-authored-by: opencode <opencode@sst.dev>
…12388) * test(cli): add failing importCloudSession regression tests Add unit tests pinning the behavior of importCloudSession's HTTP/parse handling. Two cases are red against the current implementation (a server HTTP error and a missing local id are silently swallowed as undefined instead of surfaced), one confirms the success path, and one guards that raw fetch exceptions keep propagating. The two failing cases will go green once importCloudSession is taught to throw on error. * fix(cli): throw on cloud session import failures importCloudSession previously returned undefined on any failure, silently swallowing the server's HTTP error (401/403/404/500) or a missing local id. Teach it to throw instead: with the server's error message on an HTTP error, or a clear message when the response is malformed. The four cloud-fork call sites keep branching on a missing id, but now reach that branch via catch rather than via a swallowed undefined, so the failure reason can be surfaced by callers. * fix(kilocode): surface cloud-fork import failure reason in TUI thread entry The Kilo-mirror thread entry swallowed import errors via a .catch(() => undefined) that returned empty, hiding the real 401/403/404/500 cause. Replace it with a try/catch that logs the underlying error at debug level and prints it to stderr, so a failed --cloud-fork import now reports why it failed instead of failing silently. * fix(cli): surface cloud-fork import failure reason at shared call sites The three shared upstream cloud-fork entry points (attach, run, tui thread) each swallowed import failures via a .catch(() => undefined) that lost the underlying 401/403/404/500 or network error. Wrap each call in a try/catch that logs the cause at debug level and prints it to stderr, preserving each site's existing exit semantics (process.exit / shutdownAndExit / early return). This keeps the diff to shared opencode files scoped inside the existing kilocode_change blocks. * docs: add changeset and kilocode_change annotations for cloud-fork import errors Adds the patch changeset describing the user-visible fix (failed --cloud-fork imports now report the underlying reason in both the user-visible message and the DEBUG log stream), and adds the required kilocode_change markers around the new Log/errorMessage imports and module-level loggers in the shared upstream files (run.ts, attach.ts) so the annotation CI check passes. * refactor(cli): dedupe cloud-fork import error reporting Extract a shared reportCloudImportError helper in kilocode/cloud-session.ts so the four cloud-fork import sites surface the real failure (HTTP status/body or fetch error) consistently via UI.error plus a DEBUG log, instead of duplicating that logic at each call site. * fix(cli): extract { error } reason from cloud-session import failures * fix(cli): reorder reportCloudImportError last in cloud-session import * chore: add run.ts import fix
2 participants
Issue
Fixes #12381
Context
kilo --cloud-forkimports a cloud session before the TUI/run flow starts, but a failed import currently fails silently — the user getsFailed to import session from cloudwith no reason (401/403/404/500, malformed response, or network error). This PR surfaces the real reason in both the user-visible message and theDEBUGlog stream (--print-logs --log-level DEBUG), as the issue requests.Note on the issue's framing: the reporter blamed the call-site
.catch(() => undefined)and proposed logging there. That is incomplete — the.catchonly fires on real network exceptions. On an HTTP error the SDK (defaultthrowOnError: false) returns{ data: undefined, error: <body> }andimportCloudSessionread onlyresult.data, swallowing the error internally before any.catchran. So the proposed patch would have fixed only the uncommon path. The fix is pushed intoimportCloudSessionitself.Implementation
Root cause:
importCloudSession(src/kilocode/cloud-session.ts) returnedundefinedon any failure instead of throwing. It now throws with the server's error message on HTTP failure, orcloud session import returned no session idwhen the response is malformed:importErrorReason(error)prefers the gateway'serrorfield ({ error: string }) and falls back toerrorMessagefor other SDK error shapes.All four call sites replace
.catch(() => undefined)with atry/catchthat delegates to a sharedreportCloudImportError(err)helper (see below) and then keeps its existing exit semantics. The helper lives insrc/kilocode/cloud-session.ts(Kilo-owned, no marker):The helper returns
voidon purpose — it reports (DEBUG log +UI.error) but does not throw or exit, so each caller keeps its own deterministic exit semantics (no dead code, no reliance on an unhandled rejection to terminate). In particulartui.tsstill runs its gracefulshutdownAndExit(...)andrun.tsstill runsprocess.exit(1). The helper owns its own logger (kilocode.cloud-session), so each call site no longer needs its ownLog/errorMessageimport orconst log. This was extracted in a follow-up commit (c99ce908d4) per review feedback, shrinking the diff on the three shared upstream files. The small trade-off: the per-site service tags (kilo.tui.thread,tui.attach,cli.run,tui.thread) are no longer on the import-failure log line — acceptable for #12381, where surfacing the reason is what matters.src/kilocode/cli/cmd/tui/thread.ts(Kilo-mirror)exitCode=1; return { ok:false }src/cli/cmd/attach.tsexitCode=1; returnsrc/cli/cmd/run.tsprocess.exit(1)src/cli/cmd/tui.ts(shared)shutdownAndExit(...)kilocode); the 3 shared sites' import of@/kilocode/cloud-sessionis individually marked so the annotation check passes.createKiloClienttothrowOnError: true(too large a blast radius), and the server-sidelogErrorinkilo-gateway.ts(runs in the daemon, never reaches the TUI log stream).Screenshots / Video
How to Test
Manual/local verification
bun test ./test/kilocode/cloud-session.test.ts— 6 pass (5importCloudSessioncases: success + server HTTP error + missingdata.id+ propagated fetch exception; plus 1reportCloudImportErrorcase asserting it rethrows and surfaces the reason viaUI.error). Executed by the agent.bun test ./test/kilocode/cloud-session-schema.test.ts— pass (sibling schema function in the same source file untouched). Executed by the agent.bun run typecheckfrompackages/opencode/— passes with 0 errors (the pre-existingsrc/session/prompt.ts:1310TS2322was resolved by the upstream merge this branch is based on). Executed by the agent.bun run script/check-opencode-annotations.ts(repo root) — exit 0; every Kilo-specific change in shared upstream files is marked. Executed by the agent.Reviewer test steps
fix/cloud-import-error-reportingand runbun install(links the new@opencode-ai/tuiworkspace package pulled in by the upstream merge).packages/opencode/, runbun test ./test/kilocode/cloud-session.test.tsand confirm 6 pass.kilo --session ses_XXXX --cloud-fork --print-logs --log-level DEBUG....: session not found) instead of the bareFailed to import session from cloud.DEBUGlog stream containsfailed to import cloud sessionwith the full error object (including HTTP status) for diagnosis.Blocked checks and substitute verification
Checklist
Fixes #12381Get in Touch
@TRAVIX26 Discord