-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(cli): surface real --cloud-fork import failure reasons #12388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
89cb35e
d321dfe
a143c7f
7b53ca9
b8c8a10
dbbd2ba
c99ce90
afecbf2
f3f8401
ea9c65f
bb92ad1
5cc68b5
1157c73
36b316b
9a465cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@kilocode/cli": patch | ||
| --- | ||
|
|
||
| Surface the underlying reason when `kilo --cloud-fork` fails to import a cloud session (HTTP status, server message, or fetch error) in both the user-visible message and the DEBUG log stream. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,9 @@ | ||
| import { errorMessage } from "@/util/error" | ||
| import { Log } from "@opencode-ai/core/util/log" | ||
| import { UI } from "@/cli/ui" | ||
|
|
||
| const log = Log.create({ service: "kilocode.cloud-session" }) | ||
|
|
||
| /** | ||
| * Validate --cloud-fork flag combinations and return an error message if invalid. | ||
| */ | ||
|
|
@@ -21,20 +27,50 @@ export function localSessionID(args: { cloudFork?: boolean; session?: string }) | |
| * Import a cloud session to local storage and return the new local session ID. | ||
| * Wraps the SDK's `.kilo.cloud.session.import()` which returns `unknown` due to | ||
| * the OpenAPI spec not typing the response. | ||
| * | ||
| * Throws when the import fails: with the server's error message on an HTTP | ||
| * error, or with "cloud session import returned no session id" when the | ||
| * response was malformed. | ||
| */ | ||
| export async function importCloudSession( | ||
| client: { | ||
| kilo: { | ||
| cloud: { | ||
| session: { | ||
| import: (params: { sessionId: string }) => Promise<{ data?: unknown }> | ||
| import: (params: { sessionId: string }) => Promise<{ data?: unknown; error?: unknown }> | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| sessionId: string, | ||
| ): Promise<string | undefined> { | ||
| ): Promise<string> { | ||
| const result = await client.kilo.cloud.session.import({ sessionId }) | ||
| if (result.error) throw new Error(importErrorReason(result.error)) | ||
| const id = (result.data as Record<string, unknown>)?.id | ||
| return typeof id === "string" ? id : undefined | ||
| if (typeof id !== "string") throw new Error("cloud session import returned no session id") | ||
| return id | ||
| } | ||
|
|
||
| /** | ||
| * Extract a human-readable reason from a failed cloud-session import. | ||
| * The gateway returns errors as `{ error: string }` (400/500), while other | ||
| * SDK error shapes carry `.message`/`.data.message`. Prefer the `error` | ||
| * field so the real server reason reaches the user instead of `[object Object]`. | ||
| */ | ||
| function importErrorReason(error: unknown): string { | ||
| const err = error as { error?: unknown } | ||
| if (typeof err.error === "string" && err.error) return err.error | ||
| return errorMessage(error) | ||
| } | ||
|
|
||
| /** | ||
| * Report a failed cloud-session import: log the cause at DEBUG and surface a | ||
| * human-readable message via `UI.error`. Returns `void` on purpose — it does | ||
| * not throw, so each caller keeps its own deterministic exit semantics | ||
| * (`process.exit` / `exitCode` / `shutdownAndExit` / typed `return`). The | ||
| * caller must still perform that exit after calling this. | ||
| */ | ||
| export function reportCloudImportError(err: unknown): void { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Declaring Reply with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch on the dead-code mismatch — fixed (helper is now Deliberate deviation from the literal With Net: dead code gone, docstring accurate, graceful per-site exits preserved. Happy to switch to |
||
| log.debug("failed to import cloud session", { err }) | ||
| UI.error(`Failed to import session from cloud: ${errorMessage(err)}`) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| import { describe, expect, test, mock } from "bun:test" | ||
| import { importCloudSession, reportCloudImportError } from "../../src/kilocode/cloud-session" | ||
|
|
||
| const errorMock = mock() | ||
| mock.module("@/cli/ui", () => ({ UI: { error: errorMock } })) | ||
|
|
||
| type ImportResult = { data?: unknown; error?: unknown } | ||
|
|
||
| const client = (imp: (params: { sessionId: string }) => Promise<ImportResult>) => | ||
| ({ kilo: { cloud: { session: { import: imp } } } }) as Parameters<typeof importCloudSession>[0] | ||
|
|
||
| describe("importCloudSession", () => { | ||
| test("returns local id on success", async () => { | ||
| const c = client(async () => ({ data: { id: "ses_local" } })) | ||
| const id = await importCloudSession(c, "ses_cloud") | ||
| expect(id).toBe("ses_local") | ||
| }) | ||
|
|
||
| test("throws when server returns HTTP error", async () => { | ||
| const c = client(async () => ({ | ||
| data: undefined, | ||
| error: { name: "GatewayError", message: "session not found", status: 404 }, | ||
| })) | ||
| await expect(importCloudSession(c, "ses_cloud")).rejects.toThrow("session not found") | ||
| }) | ||
|
|
||
| test("throws with the gateway's { error } reason (400/500 contract)", async () => { | ||
| const c = client(async () => ({ | ||
| data: undefined, | ||
| error: { error: "Invalid export data" }, | ||
| })) | ||
| await expect(importCloudSession(c, "ses_cloud")).rejects.toThrow("Invalid export data") | ||
| }) | ||
|
|
||
| test("throws when data.id is missing", async () => { | ||
| const c = client(async () => ({ data: {} })) | ||
| await expect(importCloudSession(c, "ses_cloud")).rejects.toThrow() | ||
| }) | ||
|
|
||
| test("propagates thrown fetch exceptions", async () => { | ||
| const c = client(async () => { | ||
| throw new Error("network down") | ||
| }) | ||
| await expect(importCloudSession(c, "ses_cloud")).rejects.toThrow("network down") | ||
| }) | ||
| }) | ||
|
|
||
| describe("reportCloudImportError", () => { | ||
| test("surfaces the reason via UI.error and does not throw", () => { | ||
| const err = new Error("session not found") | ||
| expect(() => reportCloudImportError(err)).not.toThrow() | ||
| expect(errorMock).toHaveBeenCalledWith("Failed to import session from cloud: session not found") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 itto have Kilo Code address this issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.