diff --git a/cmd/provider/use.go b/cmd/provider/use.go index 9df4280db..2b045dcf4 100644 --- a/cmd/provider/use.go +++ b/cmd/provider/use.go @@ -1,6 +1,7 @@ package provider import ( + "bytes" "context" "fmt" "io" @@ -9,6 +10,7 @@ import ( "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/client/clientimplementation" "github.com/devsy-org/devsy/pkg/config" + cliErrors "github.com/devsy-org/devsy/pkg/errors" "github.com/devsy-org/devsy/pkg/log" options2 "github.com/devsy-org/devsy/pkg/options" provider2 "github.com/devsy-org/devsy/pkg/provider" @@ -221,6 +223,10 @@ func initProvider( provider *provider2.ProviderConfig, stdout, stderr io.Writer, ) error { + // Capture the sub-binary's stderr in parallel with forwarding it to the + // regular log sink so that errors.Classify has the real provider output + // to fingerprint, not just an opaque "exit status 1". + stderrBuf := &bytes.Buffer{} err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, Name: "init", @@ -229,10 +235,13 @@ func initProvider( Options: devsyConfig.ProviderOptions(provider.Name), Config: provider, Stdout: stdout, - Stderr: stderr, + Stderr: io.MultiWriter(stderr, stderrBuf), }) if err != nil { - return fmt.Errorf("init: %w", err) + return cliErrors.Classify(fmt.Errorf("init: %w", err), cliErrors.ClassifyContext{ + Provider: provider.Name, + Stderr: stderrBuf.String(), + }) } if devsyConfig.Current().Providers == nil { devsyConfig.Current().Providers = map[string]*config.ProviderConfig{} diff --git a/cmd/root.go b/cmd/root.go index c68d3a0bd..d6d53fd14 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -1,6 +1,7 @@ package cmd import ( + "fmt" "os" "os/exec" @@ -18,6 +19,7 @@ import ( "github.com/devsy-org/devsy/cmd/up" "github.com/devsy-org/devsy/cmd/use" "github.com/devsy-org/devsy/pkg/config" + cliErrors "github.com/devsy-org/devsy/pkg/errors" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/telemetry" "github.com/go-logr/logr" @@ -26,6 +28,18 @@ import ( "k8s.io/klog/v2" ) +const ( + logOutputJSON = "json" + logOutputLogfmt = "logfmt" +) + +// isMachineLogFormat reports whether the configured --log-output mode produces +// a structured, machine-parseable stream (json or logfmt). Callers use this to +// suppress decorative human-readable affordances that would corrupt the stream. +func isMachineLogFormat(format string) bool { + return format == logOutputJSON || format == logOutputLogfmt +} + // Execute adds all child commands to the root command and sets flags appropriately. // This is called by main.main(). It only needs to happen once to the rootCmd. func Execute() { @@ -47,14 +61,22 @@ func Execute() { os.Exit(execExitErr.ExitCode()) } - if globalFlags.Debug { - log.Errorf("%+v", err) - } else { - if rootCmd.Annotations == nil || - rootCmd.Annotations[agent.AgentExecutedAnnotation] != config.BoolTrue { - log.Error("Try using -v or --debug flag to see more verbose output") + cliErr := cliErrors.Classify(err, cliErrors.ClassifyContext{}) + // Always emit the error through zap so the configured log encoder + // (json/logfmt/text) governs the wire format. JSONError preserves + // the full err.Error() chain in the top-level "msg" field and ships + // the structured CLIError under "cliError" for the desktop IPC. + log.JSONError(cliErr) + // In human-friendly text mode, follow up with hint/doc affordances + // that don't fit cleanly into the zap line. These extras are + // suppressed in machine-readable modes so log streams stay parseable. + if !isMachineLogFormat(globalFlags.LogOutput) { + if cliErr.Hint != "" { + fmt.Fprintf(os.Stderr, "Hint: %s\n", cliErr.Hint) + } + if cliErr.DocURL != "" { + fmt.Fprintf(os.Stderr, "See: %s\n", cliErr.DocURL) } - log.Errorf("%v", err) } os.Exit(1) } diff --git a/desktop/electron.vite.config.ts b/desktop/electron.vite.config.ts index c847310a0..c954b4406 100644 --- a/desktop/electron.vite.config.ts +++ b/desktop/electron.vite.config.ts @@ -32,6 +32,7 @@ export default defineConfig({ resolve: { alias: { $lib: resolve(__dirname, "src/renderer/src/lib"), + $shared: resolve(__dirname, "src/shared"), }, }, build: { diff --git a/desktop/src/main/__tests__/cli.test.ts b/desktop/src/main/__tests__/cli.test.ts index 8ea522d9d..20f0e6c9e 100644 --- a/desktop/src/main/__tests__/cli.test.ts +++ b/desktop/src/main/__tests__/cli.test.ts @@ -35,7 +35,7 @@ describe("CliRunner", () => { expect(result).toEqual([{ id: "ws-1" }]) expect(mockExecFile).toHaveBeenCalledWith( "/usr/local/bin/devsy", - ["list", "--skip-pro", "--result-format", "json"], + ["list", "--skip-pro", "--result-format", "json", "--log-output", "json"], expect.objectContaining({ env: expect.any(Object) }), expect.any(Function), ) @@ -59,6 +59,44 @@ describe("CliRunner", () => { await expect(cli.run(["list"])).rejects.toThrow("workspace not found") }) + + it("extracts cliError from a zap JSON stderr line and attaches it to the thrown Error", async () => { + const mockExecFile = vi.mocked(execFile) as unknown as ReturnType< + typeof vi.fn + > + const cliErrorPayload = { + code: "AWS_PROFILE_MISSING", + message: "AWS credentials are not configured.", + hint: "Set AWS_PROFILE or create ~/.aws/credentials.", + docUrl: "https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html", + provider: "aws", + cause: "init: exit status 1: failed to get shared config profile, default", + } + const stderrLine = JSON.stringify({ + level: "error", + ts: "2026-05-25T06:02:47.423-0500", + msg: cliErrorPayload.message, + cliError: cliErrorPayload, + }) + mockExecFile.mockImplementation( + (_cmd: string, _args: string[], _opts: unknown, callback: Function) => { + const error = new Error("Command failed") as Error & { + code: number + stderr: string + } + error.code = 1 + error.stderr = `noise before\n${stderrLine}\n` + callback(error, { stdout: "", stderr: error.stderr }) + }, + ) + + const rejection = await cli + .run(["provider", "set-options", "aws"]) + .catch((e) => e as Error & { cliError?: typeof cliErrorPayload }) + expect(rejection).toBeInstanceOf(Error) + expect(rejection.cliError).toEqual(cliErrorPayload) + expect(rejection.message).toBe(cliErrorPayload.message) + }) }) describe("runRaw", () => { @@ -92,7 +130,7 @@ describe("CliRunner", () => { await jsCli.run(["list"]) expect(mockExecFile).toHaveBeenCalledWith( "node", - ["/tmp/mock.cjs", "list", "--result-format", "json"], + ["/tmp/mock.cjs", "list", "--result-format", "json", "--log-output", "json"], expect.objectContaining({ env: expect.any(Object) }), expect.any(Function), ) diff --git a/desktop/src/main/cli.ts b/desktop/src/main/cli.ts index 0d2061b81..a5fe42a17 100644 --- a/desktop/src/main/cli.ts +++ b/desktop/src/main/cli.ts @@ -2,10 +2,73 @@ import type { ChildProcess } from "node:child_process" import { execFile as execFileCb, spawn } from "node:child_process" import { createInterface } from "node:readline" import { promisify } from "node:util" +import type { CLIError, CliLogLine } from "../shared/cli-error.js" const execFile = promisify(execFileCb) const MAX_CONCURRENT = 50 +export interface StreamLine { + /** Raw line text from the CLI (already ANSI-stripped upstream is not assumed). */ + raw: string + /** Parsed zap JSON log line, if the line was valid JSON. */ + parsed?: CliLogLine + /** Convenience: the structured cliError extracted from a parsed line, if any. */ + cliError?: CLIError + /** Log level from the parsed line, if available. */ + level?: "info" | "warn" | "error" +} + +/** + * Coerce an arbitrary zap level string into the narrow set the IPC payload uses. + * Unknown levels map to "info". + */ +function normalizeLevel(level: unknown): "info" | "warn" | "error" | undefined { + if (typeof level !== "string") return undefined + const lower = level.toLowerCase() + if (lower === "warn" || lower === "warning") return "warn" + if ( + lower === "error" || + lower === "fatal" || + lower === "panic" || + lower === "dpanic" + ) + return "error" + return "info" +} + +/** + * Walk a multi-line stderr blob, parse each line as a zap JSON record, and + * return the last `cliError` field encountered. Used by the non-streaming + * `run`/`runRaw` paths where the entire stderr is delivered after exit. + */ +function extractCliErrorFromStderr(stderr: string): CLIError | undefined { + let found: CLIError | undefined + for (const line of stderr.split(/\r?\n/)) { + const parsed = parseStderrLine(line) + if (parsed?.cliError) found = parsed.cliError + } + return found +} + +/** + * Parse a single stderr line as a zap JSON record. Returns undefined when the + * line is not valid JSON or not a plain object — callers should treat the line + * as opaque text in that case. + */ +function parseStderrLine(line: string): CliLogLine | undefined { + const trimmed = line.trim() + if (!trimmed.startsWith("{")) return undefined + try { + const obj = JSON.parse(trimmed) as unknown + if (obj && typeof obj === "object" && !Array.isArray(obj)) { + return obj as CliLogLine + } + } catch { + // not JSON — fall through + } + return undefined +} + /** * On macOS, GUI apps (including Electron) inherit a minimal PATH that excludes * /usr/local/bin and /opt/homebrew/bin. This means tools like docker, git, etc. @@ -71,7 +134,14 @@ export class CliRunner { async run(args: string[]): Promise { await this.acquire() try { - const fullArgs = [...this.prefixArgs, ...args, "--result-format", "json"] + const fullArgs = [ + ...this.prefixArgs, + ...args, + "--result-format", + "json", + "--log-output", + "json", + ] const { stdout } = await execFile(this.execPath, fullArgs, { env: this.env, }) @@ -88,7 +158,7 @@ export class CliRunner { try { const { stdout } = await execFile( this.execPath, - [...this.prefixArgs, ...args], + [...this.prefixArgs, ...args, "--log-output", "json"], { env: this.env }, ) return stdout @@ -103,16 +173,24 @@ export class CliRunner { async runStreaming( args: string[], - onLine: (line: string, stream: "stdout" | "stderr") => void, - onExit: (code: number) => void, + onLine: ( + line: string, + stream: "stdout" | "stderr", + meta?: StreamLine, + ) => void, + onExit: (code: number, cliError?: CLIError) => void, ): Promise { await this.acquire() - const child = spawn(this.execPath, [...this.prefixArgs, ...args], { - env: this.env, - }) + const child = spawn( + this.execPath, + [...this.prefixArgs, ...args, "--log-output", "json"], + { env: this.env }, + ) this.activeChildren.add(child) + let lastCliError: CLIError | undefined + if (child.stdout) { const rl = createInterface({ input: child.stdout }) rl.on("line", (line) => onLine(line, "stdout")) @@ -120,13 +198,25 @@ export class CliRunner { if (child.stderr) { const rl = createInterface({ input: child.stderr }) - rl.on("line", (line) => onLine(line, "stderr")) + rl.on("line", (line) => { + const parsed = parseStderrLine(line) + if (parsed?.cliError) { + lastCliError = parsed.cliError + } + const meta: StreamLine = { + raw: line, + parsed, + cliError: parsed?.cliError, + level: normalizeLevel(parsed?.level), + } + onLine(line, "stderr", meta) + }) } child.on("close", (code) => { this.activeChildren.delete(child) this.release() - onExit(code ?? -1) + onExit(code ?? -1, lastCliError) }) return child @@ -149,12 +239,18 @@ export class CliRunner { return join(resourcesPath, "bin", binaryName) } - private wrapError(error: unknown): Error { + private wrapError(error: unknown): Error & { cliError?: CLIError } { if (error instanceof Error && "stderr" in error) { const stderr = CliRunner.stripAnsi( String((error as { stderr: string }).stderr), ) - return new Error(this.sanitizeMessage(stderr || error.message)) + const cliError = extractCliErrorFromStderr(stderr) + const message = cliError + ? cliError.message + : this.sanitizeMessage(stderr || error.message) + const wrapped = new Error(message) as Error & { cliError?: CLIError } + if (cliError) wrapped.cliError = cliError + return wrapped } return error instanceof Error ? new Error(this.sanitizeMessage(error.message)) diff --git a/desktop/src/main/ipc.ts b/desktop/src/main/ipc.ts index 0caff7ec0..a2a721dda 100644 --- a/desktop/src/main/ipc.ts +++ b/desktop/src/main/ipc.ts @@ -6,6 +6,7 @@ import { join } from "node:path" import { promisify } from "node:util" import type { BrowserWindow } from "electron" import { ipcMain } from "electron" +import type { CLIError } from "../shared/cli-error.js" import { trackEvent } from "./analytics.js" import type { CliRunner } from "./cli.js" import type { LogStore } from "./log-store.js" @@ -111,8 +112,19 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M await cli.runRaw(["provider", "use", args.name]) }) + // Returns an envelope rather than throwing so a structured cliError survives + // the IPC boundary. Electron's structured-clone only preserves + // name/message/stack/cause on Error instances and drops arbitrary + // own-properties, so a thrown Error with a .cliError attached would lose it. ipcMain.handle("provider_init", async (_event, args: { name: string }) => { - await cli.runRaw(["provider", "set-options", args.name]) + try { + await cli.runRaw(["provider", "set-options", args.name]) + return { ok: true } as const + } catch (err) { + const cliError = (err as { cliError?: CLIError }).cliError + const message = err instanceof Error ? err.message : String(err) + return { ok: false, message, cliError } as const + } }) ipcMain.handle( @@ -123,15 +135,16 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M await cli.runStreaming( ["provider", "set-options", args.name], - (line) => { + (line, _stream, meta) => { const formatted = formatLogLine(line) win?.webContents.send("command-progress", { commandId: cmdId, message: formatted, + level: meta?.level, done: false, }) }, - (code) => { + (code, cliError) => { const exitMsg = formatLogLine( `Exit code: ${code}`, code === 0 ? "INFO" : "ERROR", @@ -139,6 +152,8 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M win?.webContents.send("command-progress", { commandId: cmdId, message: exitMsg, + level: code === 0 ? "info" : "error", + cliError, done: true, }) }, diff --git a/desktop/src/renderer/src/lib/components/ErrorCard.svelte b/desktop/src/renderer/src/lib/components/ErrorCard.svelte new file mode 100644 index 000000000..32d922a3e --- /dev/null +++ b/desktop/src/renderer/src/lib/components/ErrorCard.svelte @@ -0,0 +1,46 @@ + + + diff --git a/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte b/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte index 46e5b99fd..cfa6cc030 100644 --- a/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte +++ b/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte @@ -10,6 +10,8 @@ import * as Sheet from "$lib/components/ui/sheet/index.js" import * as ButtonGroup from "$lib/components/ui/button-group/index.js" import { Spinner } from "$lib/components/ui/spinner/index.js" import ConfirmDialog from "$lib/components/layout/ConfirmDialog.svelte" +import ErrorCard from "$lib/components/ErrorCard.svelte" +import type { CLIError } from "$shared/cli-error.js" import { providerUse, providerUpdate, @@ -46,6 +48,7 @@ let renaming = $state(false) let renameValue = $state("") let renameSaving = $state(false) let initializing = $state(false) +let initError = $state(null) let loadedFor = $state(null) let isDirty = $derived.by(() => { @@ -135,15 +138,43 @@ async function handleUpdate() { } } +/** + * Pull a structured CLIError off a thrown IPC error if the main process + * attached one. Electron's structured-clone IPC preserves additional own + * properties on Error objects, so a `cliError` field set in cli.ts/wrapError + * survives the bridge. + */ +function extractCliError(err: unknown): CLIError | null { + if (err && typeof err === "object" && "cliError" in err) { + const candidate = (err as { cliError?: unknown }).cliError + if (candidate && typeof candidate === "object" && "code" in candidate && "message" in candidate) { + return candidate as CLIError + } + } + return null +} + async function handleInitialize() { initializing = true + initError = null try { await providerInit(provider.name) const updated = await providerList() providers.set(updated) toasts.success(`Initialized ${provider.name}`) } catch (err) { - toasts.error(`Failed to initialize: ${extractErrorMessage(err)}`) + const cliError = extractCliError(err) + if (cliError) { + initError = cliError + } else { + initError = { + code: "UNKNOWN", + message: + err instanceof Error + ? err.message + : `Failed to initialize ${provider.name}.`, + } + } } finally { initializing = false } @@ -271,6 +302,12 @@ async function handleSaveOptions() { + {#if initError} +
+ +
+ {/if} +
{#if loading}

Loading options...

diff --git a/desktop/src/renderer/src/lib/components/provider/ProviderWizard.svelte b/desktop/src/renderer/src/lib/components/provider/ProviderWizard.svelte index 08c7725e4..569f481a4 100644 --- a/desktop/src/renderer/src/lib/components/provider/ProviderWizard.svelte +++ b/desktop/src/renderer/src/lib/components/provider/ProviderWizard.svelte @@ -12,6 +12,8 @@ import { Progress } from "$lib/components/ui/progress/index.js" import { Spinner } from "$lib/components/ui/spinner/index.js" import ProviderIcon from "./ProviderIcon.svelte" import LogTable from "$lib/components/log/LogTable.svelte" +import ErrorCard from "$lib/components/ErrorCard.svelte" +import type { CLIError } from "$shared/cli-error.js" import { providerAdd, providerInitStreaming, @@ -78,7 +80,8 @@ let saving = $state(false) let initCommandId = $state(null) let initLines = $state([]) let initRunning = $state(false) -let initError = $state("") +let initError = $state(null) +let initStartError = $state("") // Event listener cleanup let unlisten: UnlistenFn | null = null @@ -132,7 +135,8 @@ function reset() { initCommandId = null initLines = [] initRunning = false - initError = "" + initError = null + initStartError = "" } $effect(() => { @@ -155,7 +159,11 @@ onMount(async () => { if (isCommandSuccess(progress.message)) { refreshAndComplete() } else { - initError = "Initialization failed. You can retry or skip this step." + initError = + progress.cliError ?? { + code: "UNKNOWN", + message: "Provider initialization failed.", + } } } } @@ -262,13 +270,14 @@ async function handleConfigure() { async function startInit() { initRunning = true - initError = "" + initError = null + initStartError = "" initLines = [] try { initCommandId = await providerInitStreaming(providerName) } catch (err) { initRunning = false - initError = `Failed to start initialization: ${extractErrorMessage(err)}` + initStartError = `Failed to start initialization: ${err instanceof Error ? err.message : String(err)}` } } @@ -506,17 +515,34 @@ function handleDone() {

- {#if initError} + {#if initStartError} - {initError} + {initStartError} {/if} + {#if initError} + + {/if} + {#if initLines.length > 0} -
- -
+ {#if initError} +
+ + Show logs + +
+ +
+
+ {:else} +
+ +
+ {/if} {:else if initRunning}
diff --git a/desktop/src/renderer/src/lib/ipc/commands.ts b/desktop/src/renderer/src/lib/ipc/commands.ts index e4cc9ec97..0961940cd 100644 --- a/desktop/src/renderer/src/lib/ipc/commands.ts +++ b/desktop/src/renderer/src/lib/ipc/commands.ts @@ -87,7 +87,16 @@ export async function providerUse(name: string): Promise { } export async function providerInit(name: string): Promise { - return invoke("provider_init", { name }) + const result = await invoke< + | { ok: true } + | { ok: false; message: string; cliError?: import("$shared/cli-error.js").CLIError } + >("provider_init", { name }) + if (result.ok) return + const err = new Error(result.message) as Error & { + cliError?: import("$shared/cli-error.js").CLIError + } + if (result.cliError) err.cliError = result.cliError + throw err } export async function providerInitStreaming(name: string): Promise { diff --git a/desktop/src/renderer/src/lib/types/index.ts b/desktop/src/renderer/src/lib/types/index.ts index 42fcfb598..d3101fd4a 100644 --- a/desktop/src/renderer/src/lib/types/index.ts +++ b/desktop/src/renderer/src/lib/types/index.ts @@ -101,7 +101,14 @@ export interface Context { export interface CommandProgress { commandId: string message: string + /** Optional log level; populated when the underlying stderr line was JSON. */ + level?: "info" | "warn" | "error" done: boolean + /** + * Structured CLI error. Present only on the final (done: true) event when the + * CLI emitted a `cliError` field on its zap JSON output. + */ + cliError?: import("../../../../shared/cli-error.js").CLIError } export interface AuditEntry { diff --git a/desktop/src/renderer/src/lib/utils/error.ts b/desktop/src/renderer/src/lib/utils/error.ts index 30a256e6e..c68b81ef9 100644 --- a/desktop/src/renderer/src/lib/utils/error.ts +++ b/desktop/src/renderer/src/lib/utils/error.ts @@ -1,16 +1,32 @@ +import type { CLIError } from "$shared/cli-error.js" + /** - * Extract a human-readable message from CLI error output. - * The CLI errors contain a FATAL line with the actual message after the file reference. - * Pattern: '... FATAL : ' - * Falls back to the raw string if no FATAL line is found. + * Extract a structured CLIError from a thrown IPC error, if the main process + * attached one in cli.ts/wrapError. Electron's structured-clone-based IPC + * preserves the own-property across the renderer boundary. + */ +export function extractCliError(err: unknown): CLIError | null { + if (err && typeof err === "object" && "cliError" in err) { + const candidate = (err as { cliError?: unknown }).cliError + if ( + candidate && + typeof candidate === "object" && + "code" in candidate && + "message" in candidate + ) { + return candidate as CLIError + } + } + return null +} + +/** + * Human-readable message for toast/inline string display. Prefers the + * structured `cliError.message`; falls back to the raw Error.message for + * non-CLI errors (IPC bridge failures, renderer-thrown errors, etc.). */ export function extractErrorMessage(err: unknown): string { - const raw = err instanceof Error ? err.message : String(err) - // Match: FATAL : - const fatalMatch = raw.match(/FATAL\s+\S+:\d+\s+(.+)/i) - if (fatalMatch) return fatalMatch[1].trim() - // Fallback: try to get the last meaningful segment after 'Error:' - const parts = raw.split("Error:") - const last = parts[parts.length - 1]?.trim() - return last || raw + const cliError = extractCliError(err) + if (cliError) return cliError.message + return err instanceof Error ? err.message : String(err) } diff --git a/desktop/src/renderer/tsconfig.json b/desktop/src/renderer/tsconfig.json index eac2032cf..1e5f7b1b8 100644 --- a/desktop/src/renderer/tsconfig.json +++ b/desktop/src/renderer/tsconfig.json @@ -14,7 +14,8 @@ "verbatimModuleSyntax": false, "types": ["vite/client"], "paths": { - "$lib/*": ["./src/lib/*"] + "$lib/*": ["./src/lib/*"], + "$shared/*": ["../shared/*"] } }, "include": ["src/**/*.ts", "src/**/*.svelte"] diff --git a/desktop/src/shared/cli-error.ts b/desktop/src/shared/cli-error.ts new file mode 100644 index 000000000..28e5a5485 --- /dev/null +++ b/desktop/src/shared/cli-error.ts @@ -0,0 +1,38 @@ +/** + * Structured CLI error contract. + * + * The Go CLI emits a final zap JSON log line on stderr containing a `cliError` + * field shaped exactly like {@link CLIError}. The field name and its children + * are an immutable wire contract — see + * `docs/superpowers/specs/2026-05-25-structured-cli-errors-design.md`. + * + * This type is the single source of truth shared between the Electron main + * process (which parses CLI stderr) and the renderer (which displays errors). + */ +export interface CLIError { + /** Stable machine-readable code, e.g. "AWS_PROFILE_MISSING", "UNKNOWN". */ + code: string + /** One-line user-facing summary. */ + message: string + /** Actionable next step for the user. */ + hint?: string + /** Optional link to documentation about this error. */ + docUrl?: string + /** Optional provider name ("aws", "docker", etc.) the error originated from. */ + provider?: string + /** Original error text from the underlying failure; useful for debugging. */ + cause?: string +} + +/** + * Shape of a zap JSON log line as emitted on stderr by the CLI when + * `--log-output json` is set. The `cliError` field is present only on the + * final error event. + */ +export interface CliLogLine { + level?: "debug" | "info" | "warn" | "error" | "panic" | "fatal" | string + ts?: string + msg?: string + cliError?: CLIError + [key: string]: unknown +} diff --git a/desktop/tsconfig.json b/desktop/tsconfig.json index d904c4ec1..50309b1c0 100644 --- a/desktop/tsconfig.json +++ b/desktop/tsconfig.json @@ -8,5 +8,5 @@ "skipLibCheck": true, "outDir": "dist" }, - "include": ["src/main/**/*", "src/preload/**/*"] + "include": ["src/main/**/*", "src/preload/**/*", "src/shared/**/*"] } diff --git a/pkg/client/clientimplementation/proxy_client.go b/pkg/client/clientimplementation/proxy_client.go index a7198d466..1edfbff15 100644 --- a/pkg/client/clientimplementation/proxy_client.go +++ b/pkg/client/clientimplementation/proxy_client.go @@ -89,13 +89,20 @@ func (e *proxyExecutor) execute(ctx context.Context, params execParams) error { }) } -// executeWithJSONLog runs a command with JSON log streaming. +// executeWithJSONLog runs a command with JSON log streaming. After the +// sub-process exits we close the pipe and block on the scanner goroutine +// so every buffered log line is forwarded through zap before the caller +// can observe completion. Without this drain, callers that propagate the +// error and exit (e.g. cmd/root.go calls os.Exit on failure) race the +// goroutine and can lose the tail of the sub-process stderr — which is +// where the actionable error message usually lives. func (e *proxyExecutor) executeWithJSONLog(ctx context.Context, params execParams) error { - writer, _ := log.PipeJSONStream() - defer func() { _ = writer.Close() }() - + writer, done := log.PipeJSONStream() params.stderr = writer - return e.execute(ctx, params) + err := e.execute(ctx, params) + _ = writer.Close() + <-done + return err } func (s *proxyClient) Lock(ctx context.Context) error { diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go new file mode 100644 index 000000000..53a6dbf72 --- /dev/null +++ b/pkg/errors/errors.go @@ -0,0 +1,209 @@ +// Package errors provides structured CLI error classification. +// +// CLIError is the single typed error surface returned from the CLI to its +// callers (terminal users and the desktop app over IPC). Classify inspects a +// raw Go error and any captured sub-process stderr against a fingerprint +// table (see patterns.go) and produces a CLIError with a user-facing message, +// an actionable hint, and a documentation link. +// +// The JSON shape of CLIError — and in particular the field names code, +// message, hint, docUrl, provider, and cause — is the IPC contract with the +// desktop app and MUST NOT change without coordinated changes in both repos. +package errors + +import ( + "encoding/json" + + "go.uber.org/zap/zapcore" +) + +// Code is the stable, machine-readable identifier for a class of CLI errors. +// The string values are part of the IPC contract. +type Code string + +const ( + CodeAWSProfileMissing Code = "AWS_PROFILE_MISSING" + //nolint:gosec // G101 false positive: this is an error-code identifier, not a credential. + CodeAWSCredsInvalid Code = "AWS_CREDS_INVALID" + CodeAWSRegionMissing Code = "AWS_REGION_MISSING" + CodeDockerNotRunning Code = "DOCKER_NOT_RUNNING" + CodeDockerPermDenied Code = "DOCKER_PERMISSION_DENIED" + CodeKubeConfigMissing Code = "KUBE_CONFIG_MISSING" + CodeKubeUnreachable Code = "KUBE_UNREACHABLE" + CodePodmanSocket Code = "PODMAN_SOCKET_UNAVAILABLE" + CodeProviderInitFailed Code = "PROVIDER_INIT_FAILED" + CodeUnknown Code = "UNKNOWN" +) + +// CLIError is a structured error suitable for both terminal rendering and +// JSON-IPC transport to the desktop app. +type CLIError struct { + Code Code `json:"code"` + Message string `json:"message"` + Hint string `json:"hint,omitempty"` + DocURL string `json:"docUrl,omitempty"` + Provider string `json:"provider,omitempty"` + Cause string `json:"cause,omitempty"` + + // wrapped preserves the original error chain for errors.Is/As. + wrapped error `json:"-"` +} + +// Error returns the user-facing one-line summary. +func (e *CLIError) Error() string { + if e == nil { + return "" + } + return e.Message +} + +// Unwrap returns the original error so errors.Is / errors.As continue to work. +func (e *CLIError) Unwrap() error { + if e == nil { + return nil + } + return e.wrapped +} + +// MarshalJSON ensures stable field ordering and matches the IPC contract. +func (e *CLIError) MarshalJSON() ([]byte, error) { + type wire struct { + Code Code `json:"code"` + Message string `json:"message"` + Hint string `json:"hint,omitempty"` + DocURL string `json:"docUrl,omitempty"` + Provider string `json:"provider,omitempty"` + Cause string `json:"cause,omitempty"` + } + return json.Marshal(wire{ + Code: e.Code, + Message: e.Message, + Hint: e.Hint, + DocURL: e.DocURL, + Provider: e.Provider, + Cause: e.Cause, + }) +} + +// MarshalLogObject teaches zap to encode a *CLIError as a structured JSON +// object (instead of falling back to .Error()). Field names must stay in +// lock-step with MarshalJSON and the IPC contract documented at the top of +// this file. +func (e *CLIError) MarshalLogObject(enc zapcore.ObjectEncoder) error { + if e == nil { + return nil + } + enc.AddString("code", string(e.Code)) + enc.AddString("message", e.Message) + if e.Hint != "" { + enc.AddString("hint", e.Hint) + } + if e.DocURL != "" { + enc.AddString("docUrl", e.DocURL) + } + if e.Provider != "" { + enc.AddString("provider", e.Provider) + } + if e.Cause != "" { + enc.AddString("cause", e.Cause) + } + return nil +} + +// ClassifyContext carries optional information that helps Classify produce a +// more specific CLIError. Both fields are optional. +type ClassifyContext struct { + // Provider is the name of the provider the error originated from + // (e.g. "aws", "docker"). Empty when not applicable. + Provider string + + // Stderr is the captured stderr of a sub-process invocation, if any. + // Many sub-binaries (provider plugins) exit with "exit status 1" while + // the actionable detail lives in stderr. + Stderr string +} + +// Classify maps a Go error and optional sub-process stderr to a typed +// CLIError. It always returns a non-nil *CLIError when err is non-nil. +// Returns nil only when err is nil. +// +// The classifier walks the fingerprint table in declaration order; the first +// matching pattern wins. Unmatched errors fall through to CodeUnknown with +// the original error text as the message. +func Classify(err error, ctx ClassifyContext) *CLIError { + if err == nil { + return nil + } + if cliErr, ok := err.(*CLIError); ok { + return cloneCLIErrorWithContext(cliErr, ctx) + } + + cause := buildCause(err, ctx.Stderr) + haystack := err.Error() + if ctx.Stderr != "" { + haystack = haystack + "\n" + ctx.Stderr + } + + for _, p := range patterns { + if p.matches(haystack) { + return &CLIError{ + Code: p.Code, + Message: p.Message, + Hint: p.Hint, + DocURL: p.DocURL, + Provider: ctx.Provider, + Cause: cause, + wrapped: err, + } + } + } + + // Provider-init catch-all: surface a generic provider-init error when a + // Provider is set in the context but no specific fingerprint matched. + if ctx.Provider != "" { + return &CLIError{ + Code: CodeProviderInitFailed, + Message: "Provider initialization failed.", + Hint: "Re-run with --debug for the original error, or check the provider configuration.", + Provider: ctx.Provider, + Cause: cause, + wrapped: err, + } + } + + return &CLIError{ + Code: CodeUnknown, + Message: err.Error(), + Provider: ctx.Provider, + Cause: cause, + wrapped: err, + } +} + +// cloneCLIErrorWithContext returns a copy of cliErr with Provider/Cause +// filled in from ctx when missing. The input pointer is never mutated; this +// prevents callers' shared CLIError instances from drifting when wrapped at +// multiple layers. +func cloneCLIErrorWithContext(cliErr *CLIError, ctx ClassifyContext) *CLIError { + clone := *cliErr + if clone.Provider == "" { + clone.Provider = ctx.Provider + } + if clone.Cause == "" { + clone.Cause = buildCause(cliErr, ctx.Stderr) + } + return &clone +} + +// buildCause assembles a cause string from the Go error and any captured +// stderr. Both are included because the Go error is usually opaque +// ("exit status 1") while the sub-process stderr carries the real detail. +func buildCause(err error, stderr string) string { + if err == nil { + return stderr + } + if stderr == "" { + return err.Error() + } + return err.Error() + ": " + stderr +} diff --git a/pkg/errors/errors_test.go b/pkg/errors/errors_test.go new file mode 100644 index 000000000..9389be9db --- /dev/null +++ b/pkg/errors/errors_test.go @@ -0,0 +1,202 @@ +package errors + +import ( + "encoding/json" + stderrs "errors" + "fmt" + "testing" +) + +const ( + testExitErr = "exit status 1" + testProviderAWS = "aws" +) + +func TestClassify_NilReturnsNil(t *testing.T) { + if got := Classify(nil, ClassifyContext{}); got != nil { + t.Fatalf("Classify(nil) = %v, want nil", got) + } +} + +//nolint:funlen // table-driven test; each fingerprint case is a single row. +func TestClassify_Fingerprints(t *testing.T) { + cases := []struct { + name string + errText string + stderr string + want Code + }{ + // Positives — one per fingerprint. + { + "aws profile missing", + "init: exit status 1", + "failed to get shared config profile, default", + CodeAWSProfileMissing, + }, + { + "aws creds invalid (token)", + testExitErr, + "InvalidClientTokenId: The security token included in the request is invalid.", + CodeAWSCredsInvalid, + }, + {"aws creds invalid (sig)", testExitErr, "SignatureDoesNotMatch", CodeAWSCredsInvalid}, + { + "aws region missing", + testExitErr, + "MissingRegion: could not find region configuration", + CodeAWSRegionMissing, + }, + {"aws region missing alt", testExitErr, "could not find region", CodeAWSRegionMissing}, + { + "docker not running", + testExitErr, + "Cannot connect to the Docker daemon at unix:///var/run/docker.sock", + CodeDockerNotRunning, + }, + { + "docker permission denied", + testExitErr, + "permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock", + CodeDockerPermDenied, + }, + { + "kube config missing", + testExitErr, + "stat /home/user/.kube/config: no such file or directory", + CodeKubeConfigMissing, + }, + { + "kube unreachable refused", + testExitErr, + "dial tcp 127.0.0.1:6443: connection refused", + CodeKubeUnreachable, + }, + { + "podman socket", + testExitErr, + "podman.sock: connect: no such file or directory", + CodePodmanSocket, + }, + + // Negatives — error text that should NOT match these fingerprints. + {"unrelated text", "something else entirely went wrong", "", CodeUnknown}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := fmt.Errorf("%s", tc.errText) + got := Classify(err, ClassifyContext{Stderr: tc.stderr}) + if got == nil { + t.Fatalf("Classify returned nil") + } + if got.Code != tc.want { + t.Fatalf("Classify code = %q, want %q (cause=%q)", got.Code, tc.want, got.Cause) + } + }) + } +} + +func TestClassify_ProviderCatchAll(t *testing.T) { + err := fmt.Errorf("init: exit status 1") + got := Classify(err, ClassifyContext{ + Provider: testProviderAWS, + Stderr: "totally unrecognised output", + }) + if got.Code != CodeProviderInitFailed { + t.Fatalf("Code = %q, want %q", got.Code, CodeProviderInitFailed) + } + if got.Provider != testProviderAWS { + t.Fatalf("Provider = %q, want aws", got.Provider) + } +} + +func TestClassify_UnknownNoProvider(t *testing.T) { + err := fmt.Errorf("kaboom") + got := Classify(err, ClassifyContext{}) + if got.Code != CodeUnknown { + t.Fatalf("Code = %q, want UNKNOWN", got.Code) + } + if got.Message != "kaboom" { + t.Fatalf("Message = %q, want %q", got.Message, "kaboom") + } +} + +func TestClassify_PreservesExistingCLIError(t *testing.T) { + original := &CLIError{Code: CodeAWSProfileMissing, Message: "x"} + got := Classify(original, ClassifyContext{Provider: testProviderAWS}) + if got.Code != original.Code || got.Message != original.Message { + t.Fatalf("Classify lost fields from input CLIError: got %+v", got) + } + if got.Provider != testProviderAWS { + t.Fatalf("Provider not filled; got %q", got.Provider) + } +} + +func TestCLIError_UnwrapPreservesChain(t *testing.T) { + sentinel := stderrs.New("sentinel") + wrapped := fmt.Errorf("init: %w", sentinel) + cliErr := Classify(wrapped, ClassifyContext{}) + if !stderrs.Is(cliErr, sentinel) { + t.Fatalf("errors.Is should find sentinel through CLIError chain") + } +} + +func TestCLIError_MarshalJSONSnapshot(t *testing.T) { + e := &CLIError{ + Code: CodeAWSProfileMissing, + Message: "AWS credentials are not configured.", + Hint: "Set AWS_PROFILE or create ~/.aws/credentials.", + DocURL: "https://example.invalid/aws", + Provider: testProviderAWS, + Cause: "init: exit status 1: failed to get shared config profile, default", + } + b, err := json.Marshal(e) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + want := `{"code":"AWS_PROFILE_MISSING",` + + `"message":"AWS credentials are not configured.",` + + `"hint":"Set AWS_PROFILE or create ~/.aws/credentials.",` + + `"docUrl":"https://example.invalid/aws",` + + `"provider":"aws",` + + `"cause":"init: exit status 1: failed to get shared config profile, default"}` + if string(b) != want { + t.Fatalf("JSON mismatch.\n got: %s\nwant: %s", b, want) + } +} + +func TestCLIError_MarshalJSONOmitsEmpties(t *testing.T) { + e := &CLIError{Code: CodeUnknown, Message: "oops"} + b, err := json.Marshal(e) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + want := `{"code":"UNKNOWN","message":"oops"}` + if string(b) != want { + t.Fatalf("JSON mismatch.\n got: %s\nwant: %s", b, want) + } +} + +func TestClassify_DoesNotMutateInputCLIError(t *testing.T) { + original := &CLIError{Code: CodeAWSProfileMissing, Message: "msg"} + got := Classify(original, ClassifyContext{Provider: "aws", Stderr: "stderr-extra"}) + if got == original { + t.Fatalf("Classify returned the input pointer; expected a clone") + } + if original.Provider != "" { + t.Fatalf("input Provider was mutated: %q", original.Provider) + } + if original.Cause != "" { + t.Fatalf("input Cause was mutated: %q", original.Cause) + } + if got.Provider != "aws" { + t.Fatalf("clone Provider = %q, want %q", got.Provider, "aws") + } +} + +func TestClassify_NoPanicOnArbitraryInput(t *testing.T) { + inputs := []string{"", "\x00\x01", "random\nbytes", testExitErr} + for _, in := range inputs { + _ = Classify(fmt.Errorf("%s", in), ClassifyContext{Stderr: in}) + } +} diff --git a/pkg/errors/patterns.go b/pkg/errors/patterns.go new file mode 100644 index 000000000..1bec66c82 --- /dev/null +++ b/pkg/errors/patterns.go @@ -0,0 +1,92 @@ +package errors + +import ( + "regexp" + "strings" +) + +// pattern is one row of the fingerprint table. Exactly one of Substring or +// Regex is set. Walked in declaration order; first match wins. +type pattern struct { + Substring string + Regex *regexp.Regexp + Code Code + Message string + Hint string + DocURL string +} + +func (p pattern) matches(s string) bool { + if p.Regex != nil { + return p.Regex.MatchString(s) + } + return strings.Contains(s, p.Substring) +} + +// re is a small helper that panics at init time on a bad pattern. +func re(expr string) *regexp.Regexp { + return regexp.MustCompile(expr) +} + +// patterns is the canonical fingerprint table. ORDER MATTERS — more specific +// patterns appear before more generic ones. +// +// Each row's Code is part of the IPC contract; do not rename existing codes. +var patterns = []pattern{ + { + Substring: "failed to get shared config profile", + Code: CodeAWSProfileMissing, + Message: "AWS credentials are not configured.", + Hint: "Set AWS_PROFILE to an existing profile or create ~/.aws/credentials.", + DocURL: "https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html", + }, + { + Regex: re(`InvalidClientTokenId|SignatureDoesNotMatch`), + Code: CodeAWSCredsInvalid, + Message: "AWS credentials are invalid or expired.", + Hint: "Refresh your AWS credentials (e.g. `aws sso login`) and try again.", + DocURL: "https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html", + }, + { + Regex: re(`MissingRegion|could not find region`), + Code: CodeAWSRegionMissing, + Message: "AWS region is not set.", + Hint: "Set AWS_REGION or configure a default region with `aws configure`.", + DocURL: "https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html", + }, + { + Regex: re(`permission denied.*docker\.sock`), + Code: CodeDockerPermDenied, + Message: "Your user cannot access the Docker socket.", + Hint: "Add your user to the `docker` group, then log out and back in.", + DocURL: "https://docs.docker.com/engine/install/linux-postinstall/", + }, + { + Substring: "Cannot connect to the Docker daemon", + Code: CodeDockerNotRunning, + Message: "Docker is not running.", + Hint: "Start Docker Desktop or run `sudo systemctl start docker`.", + DocURL: "https://docs.docker.com/config/daemon/start/", + }, + { + Regex: re(`stat .*\.kube/config: no such file`), + Code: CodeKubeConfigMissing, + Message: "Kubernetes config not found.", + Hint: "Set KUBECONFIG to an existing kubeconfig file or place one at ~/.kube/config.", + DocURL: "https://kubernetes.io/docs/concepts/configuration/organize-cluster-access-kubeconfig/", + }, + { + Regex: re(`connection refused.*:6443|dial tcp.*:6443`), + Code: CodeKubeUnreachable, + Message: "Cannot reach the Kubernetes API server.", + Hint: "Check that your cluster is running and that your kubeconfig points to the right endpoint.", + DocURL: "https://kubernetes.io/docs/tasks/access-application-cluster/access-cluster/", + }, + { + Substring: "podman.sock: connect: no such file", + Code: CodePodmanSocket, + Message: "Podman socket is unavailable.", + Hint: "Start the Podman service with `systemctl --user start podman.socket`.", + DocURL: "https://docs.podman.io/en/latest/markdown/podman-system-service.1.html", + }, +} diff --git a/pkg/log/logger.go b/pkg/log/logger.go index d54776534..8b642cbdc 100644 --- a/pkg/log/logger.go +++ b/pkg/log/logger.go @@ -4,6 +4,7 @@ import ( "os" "sync/atomic" + cliErrors "github.com/devsy-org/devsy/pkg/errors" "go.uber.org/zap" "go.uber.org/zap/zapcore" "golang.org/x/term" @@ -107,6 +108,28 @@ func Warn(args ...any) { sugar.Load().Warn(args...) } func Error(args ...any) { sugar.Load().Error(args...) } func Fatal(args ...any) { sugar.Load().Fatal(args...) } +// JSONError writes a single structured zap entry carrying a *CLIError under +// the "cliError" field. The desktop IPC layer parses this field by name. +// +// The top-level "msg" is set to the original error chain (when available) so +// that log consumers grepping the textual message still see the underlying +// cause. The friendly, user-facing summary remains in cliError.message and is +// what the desktop UI surfaces. +func JSONError(cliErr *cliErrors.CLIError) { + if cliErr == nil { + return + } + msg := cliErr.Message + if wrapped := cliErr.Unwrap(); wrapped != nil { + if s := wrapped.Error(); s != "" { + msg = s + } + } else if cliErr.Cause != "" { + msg = cliErr.Cause + } + sugar.Load().Desugar().Error(msg, zap.Object("cliError", cliErr)) +} + func Debugw(msg string, keysAndValues ...any) { sugar.Load().Debugw(msg, keysAndValues...) } func Infow(msg string, keysAndValues ...any) { sugar.Load().Infow(msg, keysAndValues...) } func Warnw(msg string, keysAndValues ...any) { sugar.Load().Warnw(msg, keysAndValues...) }