diff --git a/src/commands/auth.ts b/src/commands/auth.ts index ed2bfe7..020b630 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -9,13 +9,19 @@ import type { AppContext } from "../core/context.js"; import { cmdLogin, cmdLogout, type LoginOpts } from "./login.js"; import { MODELS_PATH, REFRESH_PATH } from "../core/transport.js"; +import { HttpError, errorHint, errorMessage } from "../core/errors.js"; +import { isApiKeyToken } from "../core/auth.js"; import { box, titledBox, hyperlink, orange, green, darkBlue, brightWhite, lightBlue } from "../ui/box.js"; import { CLOUD } from "../ui/logo.js"; import { theme } from "../ui/theme.js"; +import { formatErrorLine } from "../ui/error_line.js"; -/** A long-lived API token (PAT) starts with `aek_`; otherwise it's a session token. */ +/** A long-lived API token (PAT) starts with `aek_`; otherwise it's a session + * token. Thin wrapper so callers of THIS module keep importing `isApiToken` + * from here — the actual `aek_` prefix check is canonical in core/auth.ts's + * isApiKeyToken (shared with transport.ts's refreshSession). */ export function isApiToken(token: string | null | undefined): boolean { - return typeof token === "string" && token.startsWith("aek_"); + return isApiKeyToken(token); } function mask(t: string): string { @@ -35,37 +41,61 @@ function centeredCloud(): string { // ── Branded status panel ── -async function renderAuthBox(ctx: AppContext): Promise { +// Exported so tests can render the panel directly against a fake AppContext +// without going through cmdAuth's stdout write. +export async function renderAuthBox(ctx: AppContext): Promise { const t = await ctx.tokens.get(); if (!t) return renderLoggedOut(); // Fetch tier info for display let tier = ""; let defaultModel = ""; + // A 401/403 here means the SERVER actively rejected this token (expired or + // revoked session) — that is a different situation from "server + // unreachable" and must not be swallowed the same way, or `status` would + // claim "Authenticated" for a dead session (the stale-AETHER_TOKEN case PR + // #47 fixed elsewhere). Any other failure (no HttpError, or a 5xx) is a + // genuine network/server problem: keep the existing silent local-only + // fallback for those. + let sessionExpired = false; try { const cat = await ctx.api.getJson<{ tier?: string; default?: string }>(MODELS_PATH); tier = cat.tier ?? ""; defaultModel = cat.default ?? ""; - } catch { - // Server unreachable — still show what we know locally. + } catch (err) { + if (err instanceof HttpError && (err.status === 401 || err.status === 403)) { + sessionExpired = true; + } + // Otherwise: server unreachable — still show what we know locally. } const kind = isApiToken(t) ? "API key" : "session token"; + const header = sessionExpired + ? theme.yellow("⚠") + " " + theme.bold("Aether Agent — Session expired") + : theme.iceBlue("☁") + " " + theme.bold("Aether Agent — Authenticated"); const lines: string[] = [ "", - theme.iceBlue("\u2601") + " " + theme.bold("Aether Agent \u2014 Authenticated"), + header, "", // No Account row: there is no account endpoint yet, and a hardcoded - // "(fetching\u2026)" that never resolves is a fake loading state (PR #47 UX). + // "(fetching…)" that never resolves is a fake loading state (PR #47 UX). " " + theme.dim("Token:") + " " + theme.bold(mask(t)) + " " + theme.dim(`(${kind})`), " " + theme.dim("API:") + " " + ctx.cfg.baseUrl.replace("https://", ""), ]; - if (tier) { - lines.push(" " + theme.dim("Tier:") + " " + (tier === "free" ? theme.dim(tier) : theme.cyan(tier))); - } - if (defaultModel) { - lines.push(" " + theme.dim("Default:") + " " + defaultModel); + if (sessionExpired) { + lines.push( + "", + " " + theme.yellow("Server rejected this token — sign in again:"), + " " + theme.bold(theme.cyan("aether auth login")), + ); + } else { + if (tier) { + lines.push(" " + theme.dim("Tier:") + " " + (tier === "free" ? theme.dim(tier) : theme.cyan(tier))); + } + if (defaultModel) { + lines.push(" " + theme.dim("Default:") + " " + defaultModel); + } } lines.push( @@ -78,7 +108,8 @@ async function renderAuthBox(ctx: AppContext): Promise { "", ); - return [centeredCloud(), "", titledBox(lines, "Authenticated", { width: BOX_W })].join("\n"); + const title = sessionExpired ? "Session expired" : "Authenticated"; + return [centeredCloud(), "", titledBox(lines, title, { width: BOX_W })].join("\n"); } // ── Logged-out welcome panel ── @@ -140,6 +171,13 @@ export async function cmdAuth( case "logout": return cmdLogout(ctx); case "status": { + // LOOP-06: renderAuthBox's first move is a network round-trip to + // /models (no cache to fall back on for the CLI's one-shot status + // command), and nothing was written to stdout until the whole thing + // resolved — up to DEFAULT_REQUEST_TIMEOUT_MS of silence on a slow + // connection, "the REPL just looks hung" (the exact class PR #47 fixed + // for slash.ts's catalog fetch). Match that convention here. + process.stdout.write(theme.dim("checking session…\n")); const panel = await renderAuthBox(ctx); process.stdout.write(panel + "\n"); return 0; @@ -152,8 +190,11 @@ export async function cmdAuth( printAuthHelp(); return 0; default: { - // Bare `aether auth` — show the branded panel. + // Bare `aether auth` — show the branded panel. Same LOOP-06 loading + // line as the "status" branch above — this hits renderAuthBox's same + // /models call. if (sub === "") { + process.stdout.write(theme.dim("checking session…\n")); const panel = await renderAuthBox(ctx); process.stdout.write("\n" + panel + "\n\n"); return 0; @@ -208,10 +249,17 @@ async function authRefresh(ctx: AppContext): Promise { process.stdout.write("✓ Session refreshed.\n"); return 0; } - process.stderr.write("✗ Refresh failed — try: aether auth login\n"); + // Same formatErrorLine convention as the catch below — a 200 with no + // session_token is still a refresh failure and must read the same as + // one caught via a thrown error, not as the one bare ✗ line left in + // this function (LOOP-06 round 3). + process.stderr.write(formatErrorLine("Refresh failed", { hint: "run `aether auth login`" })); return 1; } catch (err) { - process.stderr.write(`\u2717 ${err instanceof Error ? err.message : String(err)}\n`); + // formatErrorLine (LOOP-06) owns the glyph/hint/separator convention, + // same as chat.ts's printError, so a failed /auth/refresh reads like + // any other auth-adjacent error instead of bare, uncolored text. + process.stderr.write(formatErrorLine(errorMessage(err), { hint: errorHint(err, ctx.cfg.baseUrl) })); return 1; } } diff --git a/src/commands/chat.ts b/src/commands/chat.ts index 0cac1e9..43d2c15 100644 --- a/src/commands/chat.ts +++ b/src/commands/chat.ts @@ -7,10 +7,11 @@ import { StringDecoder } from "node:string_decoder"; import type { AppContext, GlobalFlags } from "../core/context.js"; import { theme, errTheme } from "../ui/theme.js"; import { buildChatRequest } from "../core/envelope.js"; -import { CHAT_STREAM_PATH, CHAT_PATH } from "../core/transport.js"; +import { CHAT_STREAM_PATH, CHAT_PATH, defaultStreamTimeoutMs } from "../core/transport.js"; import { decodeSse } from "../core/stream.js"; import { Renderer } from "../core/render.js"; -import { StreamUnavailableError, errorHint, isAbortError } from "../core/errors.js"; +import { StreamIncompleteError, StreamUnavailableError, errorHint, isAbortError } from "../core/errors.js"; +import { formatErrorLine } from "../ui/error_line.js"; import { appendCustody } from "../core/custody.js"; import { handleSlash, primeCatalog } from "./slash.js"; import { applyPromptMode } from "./prompt_modes.js"; @@ -88,7 +89,10 @@ export async function resolveBackend(ctx: AppContext): Promise { * `signal` cancels the turn client-side (stream AND the fail-soft fallback) — * orchestrator runs inherit cancelability through this same seam. Throws * ChatTurnError if the server streamed an `error` frame, so callers can exit - * non-zero instead of treating a rendered "✗ msg" as a successful turn. + * non-zero instead of treating a rendered "✗ msg" as a successful turn. Also + * throws StreamIncompleteError if the stream ends without ever sending a + * terminal `done` or `error` frame (LOOP-06 round 3) — a clean-looking + * premature close must not render as a successful turn either. * `onPulsePaint` fires after every thinking-pulse repaint (see * ThinkingPulseOptions.onPaint) so the REPL can re-sync its own input-line * redraw — typing ahead during the pre-first-token window would otherwise @@ -143,6 +147,13 @@ async function runCloudTurn( }); pulse.start(); let sawError: string | null = null; + // Whether a terminal `done` or `error` frame was actually observed. decodeSse's + // for-await loop exits normally (no throw) once the underlying byte stream + // ends — including a premature-but-clean close (proxy/load-balancer time-box, + // etc.) that never sent either. Without this, such a close renders whatever + // partial output arrived and returns as if the turn completed successfully + // (LOOP-06 round 3). + let sawTerminal = false; try { const stream = await ctx.api.stream(CHAT_STREAM_PATH, req, signal); for await (const frame of decodeSse(stream)) { @@ -154,15 +165,21 @@ async function runCloudTurn( // locally (best-effort, never breaks the chat). if (frame.type === "custody") appendCustody(frame.custody); if (frame.type === "error") sawError = frame.msg; + if (frame.type === "error" || frame.type === "done") sawTerminal = true; onFrame?.(frame); renderer.frame(frame); } if (sawError) throw new ChatTurnError(sawError); + if (!sawTerminal) throw new StreamIncompleteError(); } catch (err) { if (err instanceof StreamUnavailableError) { // Contract fail-soft: fall back to the non-streaming request/response. - // Same signal — the fallback leg is cancelable too (arena AT-3d). - const r = await ctx.api.postJson(CHAT_PATH, req, signal); + // Same signal — the fallback leg is cancelable too (arena AT-3d). A + // full LLM turn can legitimately run long, so this explicitly opts + // into stream()'s own generous bound instead of request()'s 30s + // metadata-call default (LOOP-01/LOOP-06 round-1) — otherwise a + // perfectly healthy but slow completion would be killed early. + const r = await ctx.api.postJson(CHAT_PATH, req, signal, defaultStreamTimeoutMs()); pulse.stop(); process.stdout.write((r.response ?? "") + "\n"); if (ctx.flags.audit && r.commitment_hash) { @@ -957,11 +974,8 @@ async function replLines(ctx: AppContext): Promise { function printError(err: unknown, baseUrl: string): void { const msg = err instanceof Error ? err.message : String(err); - process.stderr.write(`\n${errTheme.red("✗")} ${msg}\n`); - const hint = errorHint(err, baseUrl); - if (hint) process.stderr.write(errTheme.dim(` ⤷ ${hint}`) + "\n"); - // Trailing blank line: the REPL reprints its prompt right after this, and - // without the separator the dim hint and the prompt fuse into one line - // (the "⤷ run `aether auth login` … [user]_:" mess from PR #47's report). - process.stderr.write("\n"); + // formatErrorLine (LOOP-06) owns the glyph/hint/separator convention so + // this reads identically to a server-streamed error frame's Renderer.error + // — see src/ui/error_line.ts for why both paths must agree. + process.stderr.write(formatErrorLine(msg, { hint: errorHint(err, baseUrl) })); } diff --git a/src/commands/login.ts b/src/commands/login.ts index 8c63e71..a4b5e30 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -13,6 +13,8 @@ import { LOGOUT_PATH } from "../core/transport.js"; import { requestDeviceCode, pollForToken } from "../core/device.js"; import { openBrowser } from "../core/browser.js"; import { theme } from "../ui/theme.js"; +import { errorHint, errorMessage } from "../core/errors.js"; +import { formatErrorLine } from "../ui/error_line.js"; export interface LoginOpts { token?: string; @@ -75,7 +77,10 @@ export async function cmdLogin(ctx: AppContext, opts: LoginOpts): Promise. @@ -420,7 +426,7 @@ async function select( /** Shared by showPicker/select once a target item is resolved: lock check, * restart warning, and the y/N gate that produces the caller's restart signal. */ -async function confirmSwitch( +export async function confirmSwitch( ctx: AppContext, out: Writable, item: CatalogItem, @@ -428,7 +434,11 @@ async function confirmSwitch( tier: string, ): Promise<{ model?: string; agent?: string } | null> { if (!item.available) { - out.write(`${item.id} is locked on tier ${tier}\n`); + // Same dim styling + "check: /tier or `aether models`" pointer as + // httpStatusHint(403) (errors.ts) — a tier lock reached via the picker + // must read the same as the functionally identical 403 reached over the + // wire, not as unstyled text with no next step. LOOP-06. + out.write(theme.dim(`${item.id} is locked on tier ${tier} — check: /tier or \`aether models\`\n`)); return null; } out.write( diff --git a/src/commands/slash_media.ts b/src/commands/slash_media.ts index eb1803c..c2883fb 100644 --- a/src/commands/slash_media.ts +++ b/src/commands/slash_media.ts @@ -8,6 +8,8 @@ import type { Writable } from "node:stream"; import type { AppContext } from "../core/context.js"; import { theme } from "../ui/theme.js"; +import { sanitizeTerm } from "../ui/text.js"; +import { hintFor } from "../core/error_hints.js"; import { basename } from "node:path"; import { existsSync as fsExistsSync } from "node:fs"; import { @@ -54,6 +56,24 @@ function parseSlashFlags(raw: string, _kind: MediaKind): { prompt: string; flags const SF = theme.dim; +// LOOP-01 round 2: dispatchGeneration()/downloadMediaFile() already throw a +// classified HttpError (401/402/403/5xx) or the shared network-outage/timeout +// errors — every catch block in this file used to print only the raw +// `err.message` with no actionable next step. This mirrors chat.ts's +// printError / workflow.ts's fail() but writes to the slash command's own +// `out` stream instead of stderr, and preserves each call site's original +// indentation. +function writeErr(out: Writable, err: unknown, indent = ""): void { + // Sanitized: dispatchGeneration/downloadMediaFile can fail on a hostile + // server-controlled media_url, whose bytes would otherwise land in + // err.message verbatim (same OSC/CSI-injection class formatErrorLine and + // sanitizeServerText close everywhere else this diff touched). + const msg = err instanceof Error ? err.message : String(err); + out.write(`${indent}✗ ${sanitizeTerm(msg)}\n`); + const hint = hintFor(err); + if (hint) out.write(SF(`${indent} ⤷ ${hint}\n`)); +} + export async function photogenSlash(ctx: AppContext, out: Writable, arg: string, _framed: boolean): Promise { const { prompt, flags } = parseSlashFlags(arg, "image"); if (!prompt) { @@ -86,7 +106,7 @@ export async function photogenSlash(ctx: AppContext, out: Writable, arg: string, } else { out.write(SF(" no media URL returned\n")); } - } catch (err) { out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); } + } catch (err) { writeErr(out, err); } } } @@ -104,7 +124,7 @@ export async function reframeSlash(ctx: AppContext, out: Writable, arg: string): out.write(`${theme.iceBlue("↓")} #${entry.index} ${entry.filename}\n`); _lastMediaUrl = resp.media_url; _lastMediaModel = "vision_gpt_image2"; } - } catch (err) { out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); } + } catch (err) { writeErr(out, err); } } export async function videogenSlash(ctx: AppContext, out: Writable, arg: string, cinematic: boolean): Promise { @@ -124,7 +144,7 @@ export async function videogenSlash(ctx: AppContext, out: Writable, arg: string, out.write(`${theme.iceBlue("↓")} #${entry.index} ${entry.filename}\n ${SF(resp.media_url)}\n\n`); _lastMediaUrl = resp.media_url; _lastMediaModel = modelKey; _lastMediaKind = kind; } - } catch (err) { out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); } + } catch (err) { writeErr(out, err); } } export async function animateSlash(ctx: AppContext, out: Writable, arg: string): Promise { @@ -148,7 +168,7 @@ export async function animateSlash(ctx: AppContext, out: Writable, arg: string): out.write(`${theme.iceBlue("↓")} #${entry.index} ${entry.filename}\n`); _lastMediaUrl = resp.media_url; _lastMediaModel = "vision_seedance"; _lastMediaKind = "video"; } - } catch (err) { out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); } + } catch (err) { writeErr(out, err); } } export async function recutSlash(ctx: AppContext, out: Writable, arg: string): Promise { @@ -165,7 +185,7 @@ export async function recutSlash(ctx: AppContext, out: Writable, arg: string): P out.write(`${theme.iceBlue("↓")} #${entry.index} ${entry.filename}\n`); _lastMediaUrl = resp.media_url; } - } catch (err) { out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); } + } catch (err) { writeErr(out, err); } } export async function outputSlash(_ctx: AppContext, out: Writable, arg: string): Promise { @@ -177,7 +197,7 @@ export async function outputSlash(_ctx: AppContext, out: Writable, arg: string): const entry = findOutput(ref); if (!entry) { out.write(SF(` no output matching "${ref}"\n`)); return; } try { openOutput(entry); out.write(`${theme.iceBlue("→")} opened ${entry.filename}\n`); } - catch (err) { out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); } + catch (err) { writeErr(out, err); } return; } if (sub === "clean" || sub === "clear") { out.write(SF(` cleared ${clearOutput()} entries\n`)); return; } @@ -239,7 +259,7 @@ export async function storyboardSlash(ctx: AppContext, out: Writable, arg: strin out.write(SF("\n/storyboard --generate to generate all keyframes\n")); out.write(SF("/storyboard --animate to animate the sequence\n")); out.write(SF("/storyboard --render full pipeline\n")); - } catch (err) { out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); } + } catch (err) { writeErr(out, err); } } function sbFlagParse(raw: string): { scenes?: number; style?: string } { @@ -280,7 +300,7 @@ async function sbRender(ctx: AppContext, sb: Storyboard, phase: string, out: Wri recordOutput({ model: mk, prompt: s.keyframe_prompt, kind: "image", filepath: fp, filename: basename(fp), url: resp.media_url, timestamp: new Date().toISOString(), flags: {} }); out.write(` ${theme.iceBlue("✓")} scene ${s.index}\n`); } - } catch (err) { out.write(` ✗ ${err instanceof Error ? err.message : String(err)}\n`); } + } catch (err) { writeErr(out, err, " "); } } sb.status = "keyframes_generated"; saveStoryboard(sb); } @@ -296,7 +316,7 @@ async function sbRender(ctx: AppContext, sb: Storyboard, phase: string, out: Wri recordOutput({ model: "vision_seedance", prompt: s.animation_prompt, kind: "video", filepath: fp, filename: basename(fp), url: resp.media_url, timestamp: new Date().toISOString(), flags: {} }); out.write(` ${theme.iceBlue("✓")} scene ${s.index}\n`); } - } catch (err) { out.write(` ✗ ${err instanceof Error ? err.message : String(err)}\n`); } + } catch (err) { writeErr(out, err, " "); } } sb.status = "animated"; saveStoryboard(sb); } diff --git a/src/commands/vault.ts b/src/commands/vault.ts index 264632d..e73969a 100644 --- a/src/commands/vault.ts +++ b/src/commands/vault.ts @@ -4,6 +4,7 @@ import type { AppContext } from "../core/context.js"; import { fail as coreFail } from "../core/errors.js"; +import { hintFor } from "../core/error_hints.js"; import { getVaultList, browseVault, getSpacesUsage, getSpacesContent, deleteSpacesFile, @@ -273,6 +274,12 @@ function renderSearchResults(r: VaultSearchResponse, query: string): void { } } +// LOOP-01 round 2: this used to hand every failure (401, 402 out-of-balance, +// 403 plan/tier, 5xx, network outage) the SAME hardcoded "are you logged in?" +// hint, even though downloadFile/uploadFile/deleteSpacesFile etc. already +// throw a properly-classified HttpError. hintFor() (the no-baseUrl sibling of +// errors.errorHint — see its doc comment) now picks the right wording per +// status, matching the fix already applied to workflow.ts's `workflowNew`. function fail(err: unknown): number { - return coreFail(err, "are you logged in? run: aether auth login"); + return coreFail(err, hintFor(err) ?? undefined); } \ No newline at end of file diff --git a/src/commands/workflow.ts b/src/commands/workflow.ts index 25e81a9..ed5fac8 100644 --- a/src/commands/workflow.ts +++ b/src/commands/workflow.ts @@ -185,8 +185,15 @@ async function workflowSave(ctx: AppContext, name?: string, templateArg?: string } wf = { ...WORKFLOW_TEMPLATES[n - 1]!.workflow, name: name || WORKFLOW_TEMPLATES[n - 1]!.name }; } else if (name) { - // Try loading existing workflow from vault, or create from template by name - wf = await getWorkflow(ctx.api, name); + // Try loading existing workflow from vault, or create from template by name. + // getWorkflow now propagates a real failure (LOOP-01 round 3) instead of + // swallowing it to null, so this needs its own try/catch — otherwise a + // 401/5xx here would escape past this function's own try block below + // (which only starts once wf is resolved) all the way to main.ts's bare + // top-level catch, printing a hint-less raw error message. + try { + wf = await getWorkflow(ctx.api, name); + } catch (err) { return fail(err); } if (!wf) { // Check if name matches a template id const tpl = WORKFLOW_TEMPLATES.find(t => t.id === name); @@ -380,5 +387,5 @@ async function workflowStatus(ctx: AppContext): Promise { // ── Helpers ────────────────────────────────────── function fail(err: unknown): number { - return coreFail(err, "are you logged in? run: aether auth login"); + return coreFail(err, hintFor(err) ?? undefined); } diff --git a/src/core/auth.ts b/src/core/auth.ts index 4046e36..691d09c 100644 --- a/src/core/auth.ts +++ b/src/core/auth.ts @@ -21,7 +21,7 @@ import { writeFileSync, } from "node:fs"; import { configDir } from "./config.js"; -import { LOGIN_PATH, isCredentialSafeUrl } from "./transport.js"; +import { LOGIN_PATH, defaultRequestTimeoutMs, isCredentialSafeUrl, sanitizeServerText } from "./transport.js"; export interface TokenStore { get(): Promise; @@ -91,6 +91,47 @@ export function defaultTokenStore(): TokenStore { return new FileTokenStore(); } +/** + * A long-lived API token (PAT) starts with `aek_`; a session token (minted by + * /auth/login or /auth/refresh) doesn't. The one canonical definition of that + * prefix, shared by transport.ts's refreshSession() (an `aek_` token never + * expires, so a 401 on one is never retried) and commands/auth.ts's + * isApiToken (status/token display) — previously hand-duplicated in both + * places, risking silent drift if the prefix scheme ever changes (LOOP-01 + * round 2; mirrors this file's own tokenStoreForInjected, added for the same + * "one canonical decision" reason). + */ +export function isApiKeyToken(token: string | null | undefined): boolean { + return typeof token === "string" && token.startsWith("aek_"); +} + +/** + * The one canonical "injected token -> TokenStore" decision, shared by every + * surface that resolves an embedded/injected session token (tokenStoreFromEnv + * for the CLI entry, AetherClient's constructor for library embedders) so the + * choice can't silently drift into hand-maintained copies (LOOP-01 round 1). + * + * An empty/whitespace token counts as unset and falls back to the file store. + * + * `persistOnLogin` is the one axis that legitimately differs by surface: + * - true (EnvOverrideTokenStore): an explicit login's fresh token persists to + * disk too, so a NEW process (no env override) still sees it — this is the + * CLI-entry fix for PR #47's "✓ Logged in" evaporating with the process. + * - false (StaticTokenStore): the token stays in-process only. AetherClient is + * an embeddable library surface (desktop in-process embed, Aether AI on the + * web) whose consumers explicitly must NOT have a `.login()` call clobber + * the standalone CLI's independent on-disk session — see StaticTokenStore's + * doc comment below. + */ +export function tokenStoreForInjected( + injected: string | undefined | null, + opts: { persistOnLogin: boolean }, +): TokenStore { + const t = (injected ?? "").trim(); + if (!t) return defaultTokenStore(); + return opts.persistOnLogin ? new EnvOverrideTokenStore(t, defaultTokenStore()) : new StaticTokenStore(t); +} + /** * Token store for a running CLI process, choosing the source by environment. * @@ -99,13 +140,14 @@ export function defaultTokenStore(): TokenStore { * store, so a user already signed into AetherCloud who opens a terminal is * authenticated as that same account and is NEVER asked to re-run * `aether auth login`. With no env token (a standalone CLI user), it falls back - * to the file store, so the normal login flow is unaffected. An empty/whitespace - * value counts as unset. Mirrors AetherClient's embedded-token resolution so the - * interactive REPL and the universal chat client agree on auth. + * to the file store, so the normal login flow is unaffected. Mirrors + * AetherClient's embedded-token resolution (both go through + * tokenStoreForInjected) so the interactive REPL and the universal chat client + * agree on auth reads; only the login-persistence axis differs (see + * tokenStoreForInjected's doc comment). */ export function tokenStoreFromEnv(env: NodeJS.ProcessEnv = process.env): TokenStore { - const injected = (env["AETHER_TOKEN"] ?? "").trim(); - return injected ? new EnvOverrideTokenStore(injected, defaultTokenStore()) : defaultTokenStore(); + return tokenStoreForInjected(env["AETHER_TOKEN"], { persistOnLogin: true }); } /** @@ -179,6 +221,14 @@ export async function loginWithPassword( creds: { username: string; password: string; licenseKey?: string }, ): Promise { if (!isCredentialSafeUrl(baseUrl)) throw new Error("login refused: insecure transport"); + // Bounded on its own: this runs before any token exists, so it can't go + // through ApiClient.request()'s default timeout — without one, a stalled + // /auth/login response would hang the headless `--username/--password` flow + // (login.ts's CI/scripts path) forever with no other cancellation mechanism. + // Shares AETHER_REQUEST_TIMEOUT_MS with ApiClient so one env var controls + // both; 0 (explicitly disabled) is honored rather than passing a + // zero-length AbortSignal.timeout(), which would abort immediately. + const timeoutMs = defaultRequestTimeoutMs(); const res = await fetch(baseUrl.replace(/\/$/, "") + LOGIN_PATH, { method: "POST", headers: { "Content-Type": "application/json", Accept: "application/json" }, @@ -187,6 +237,7 @@ export async function loginWithPassword( password: creds.password, license_key: creds.licenseKey ?? null, }), + ...(timeoutMs > 0 ? { signal: AbortSignal.timeout(timeoutMs) } : {}), }); let body: LoginResponseBody; try { @@ -195,11 +246,27 @@ export async function loginWithPassword( throw new Error(`login failed (HTTP ${res.status})`); } if (!res.ok || !body.authenticated || !body.session_token) { - throw new Error(`login failed: ${body.reason ?? `HTTP ${res.status}`}`); + // body.reason is raw JSON from an un-authenticated POST /auth/login + // response — a compromised/misconfigured backend (or a self-hosted dev + // server) could otherwise inject raw control bytes, including OSC 52 + // clipboard-hijack sequences, since login.ts's headless catch writes + // this message straight to stderr with no sanitization of its own + // (LOOP-06 round 2). Same strip-and-cap treatment toHttpError applies to + // every OTHER server-text path, via the shared sanitizeServerText(). + const reason = + typeof body.reason === "string" && body.reason.trim() ? sanitizeServerText(body.reason) : undefined; + throw new Error(`login failed: ${reason ?? `HTTP ${res.status}`}`); } await store.set(body.session_token); const result: LoginResult = {}; - if (body.plan != null) result.plan = body.plan; - if (body.commitment_hash != null) result.commitmentHash = body.commitment_hash; + // Same server-controlled-text hazard as `reason` above, just on the SUCCESS + // path: login.ts:74 writes `plan` straight to stdout + // (`✓ Logged in (plan: ${r.plan}).`) with no sanitization of its own, so a + // compromised/misconfigured backend's `plan`/`commitment_hash` fields need + // the same treatment before they leave loginWithPassword. + if (typeof body.plan === "string" && body.plan.trim()) result.plan = sanitizeServerText(body.plan); + if (typeof body.commitment_hash === "string" && body.commitment_hash.trim()) { + result.commitmentHash = sanitizeServerText(body.commitment_hash); + } return result; } diff --git a/src/core/bench.ts b/src/core/bench.ts index 1c35be1..37ee5c3 100644 --- a/src/core/bench.ts +++ b/src/core/bench.ts @@ -1,7 +1,7 @@ // UVT /bench — performance profiling + optimization. Orchestrator-gated. import type { ApiClient } from "./transport.js"; -import { AGENT_BENCH_PATH } from "./transport.js"; +import { AGENT_BENCH_PATH, defaultStreamTimeoutMs } from "./transport.js"; export interface BenchRequest { agent: string; @@ -22,8 +22,15 @@ export async function runBenchmark( agent: string, target: string, ): Promise { - return api.postJson(AGENT_BENCH_PATH, { - agent, - target, - } as BenchRequest); + // Blocks server-side until the profiling + optimization pass completes + // (returns patches/profiles, not a job handle) — same class of long-running + // call as chat's non-streaming fallback, so it opts into stream()'s own + // generous bound instead of request()'s 30s metadata-call default + // (LOOP-01/LOOP-06 round-1). + return api.postJson( + AGENT_BENCH_PATH, + { agent, target } as BenchRequest, + undefined, + defaultStreamTimeoutMs(), + ); } diff --git a/src/core/brain_cloud.ts b/src/core/brain_cloud.ts index d5cac8f..342fac1 100644 --- a/src/core/brain_cloud.ts +++ b/src/core/brain_cloud.ts @@ -13,10 +13,10 @@ import type { Brain, TaskCommand } from "./brain.js"; import { EventQueue } from "./brain.js"; import type { BrainEvent } from "./brain_protocol.js"; import type { ApiClient } from "./transport.js"; -import { CHAT_STREAM_PATH, CHAT_PATH } from "./transport.js"; +import { CHAT_STREAM_PATH, CHAT_PATH, defaultStreamTimeoutMs } from "./transport.js"; import { buildChatRequest } from "./envelope.js"; import { decodeSse, type StreamFrame } from "./stream.js"; -import { StreamUnavailableError } from "./errors.js"; +import { StreamIncompleteError, StreamUnavailableError } from "./errors.js"; import { appendCustody } from "./custody.js"; import { hintFor } from "./error_hints.js"; @@ -41,14 +41,18 @@ export class CloudBrain implements Brain { try { const stream = await this.api.stream(CHAT_STREAM_PATH, req); // The terminal done must be ground truth, never fabricated success - // (CONTRACTS.md invariant 5): a streamed error or a user abort ends the - // run ok:false. Custody receipts persist here too — the server stores + // (CONTRACTS.md invariant 5): a streamed error, a user abort, or the + // stream ending without ever sending a terminal done/error frame (a + // clean-looking premature close — LOOP-06 round 3) all end the run + // ok:false. Custody receipts persist here too — the server stores // nothing; the client-held log is the only copy. let failed: string | null = null; + let sawDone = false; for await (const frame of decodeSse(stream)) { if (this.aborted) break; if (frame.type === "custody") appendCustody(frame.custody); if (frame.type === "error") failed = frame.msg; + if (frame.type === "done") sawDone = true; const ev = mapFrame(frame); if (ev) queue.push(ev); } @@ -56,14 +60,19 @@ export class CloudBrain implements Brain { queue.push({ type: "done", ok: false, result: failed, remaining: 0, reason: "" }); } else if (this.aborted) { queue.push({ type: "done", ok: false, result: "aborted", remaining: 0, reason: "" }); + } else if (!sawDone) { + queue.push({ type: "done", ok: false, result: withHint(new StreamIncompleteError()), remaining: 0, reason: "" }); } else { queue.push({ type: "done", ok: true, result: "", remaining: 0, reason: "" }); } } catch (err) { if (err instanceof StreamUnavailableError) { - // Fail-soft: non-streaming fallback (contract `{"stream": false}`). + // Fail-soft: non-streaming fallback (contract `{"stream": false}`). A + // full LLM turn can legitimately run long, so this opts into + // stream()'s own generous bound rather than request()'s 30s + // metadata-call default (LOOP-01/LOOP-06 round-1). try { - const r = await this.api.postJson<{ response?: string }>(CHAT_PATH, req); + const r = await this.api.postJson<{ response?: string }>(CHAT_PATH, req, undefined, defaultStreamTimeoutMs()); queue.push({ type: "monologue", text: r.response ?? "", depth: 0 }); queue.push({ type: "done", ok: true, result: r.response ?? "", remaining: 0, reason: "" }); } catch (e2) { diff --git a/src/core/client.ts b/src/core/client.ts index 1d51e58..e2be751 100644 --- a/src/core/client.ts +++ b/src/core/client.ts @@ -8,13 +8,12 @@ // AETHER_BASE_URL, so the desktop routes its chat through this without owning // any transport logic. -import { ApiClient, CHAT_STREAM_PATH, CHAT_PATH, MODELS_PATH } from "./transport.js"; +import { ApiClient, CHAT_STREAM_PATH, CHAT_PATH, MODELS_PATH, defaultStreamTimeoutMs } from "./transport.js"; import { decodeSse, type StreamFrame } from "./stream.js"; import { buildChatRequest } from "./envelope.js"; import { StreamUnavailableError } from "./errors.js"; import { - defaultTokenStore, - StaticTokenStore, + tokenStoreForInjected, loginWithPassword, type LoginResult, type TokenStore, @@ -49,8 +48,13 @@ export class AetherClient { this.baseUrl = opts.baseUrl ?? process.env["AETHER_BASE_URL"] ?? loadConfig().baseUrl; const envToken = process.env["AETHER_TOKEN"]; const injected = opts.token ?? envToken; - this.tokens = - opts.tokenStore ?? (injected ? new StaticTokenStore(injected) : defaultTokenStore()); + // In-process only (persistOnLogin: false): AetherClient is an embeddable + // library surface (desktop in-process embed, Aether AI on the web), so an + // explicit login() must not clobber the standalone CLI's on-disk session. + // Shares the injected-token decision with tokenStoreFromEnv via + // tokenStoreForInjected so the two surfaces can't silently diverge + // (LOOP-01 round 1). + this.tokens = opts.tokenStore ?? tokenStoreForInjected(injected, { persistOnLogin: false }); this.api = new ApiClient(this.baseUrl, this.tokens); } @@ -76,7 +80,10 @@ export class AetherClient { for await (const frame of decodeSse(stream)) yield frame; } catch (err) { if (err instanceof StreamUnavailableError) { - const r = await this.api.postJson<{ response?: string }>(CHAT_PATH, req); + // A full LLM turn can legitimately run long, so this opts into + // stream()'s own generous bound rather than request()'s 30s + // metadata-call default (LOOP-01/LOOP-06 round-1). + const r = await this.api.postJson<{ response?: string }>(CHAT_PATH, req, undefined, defaultStreamTimeoutMs()); yield { type: "delta", text: r.response ?? "" }; yield { type: "done", uvt: 0, cents: 0 }; return; diff --git a/src/core/device.ts b/src/core/device.ts index 12db174..d779172 100644 --- a/src/core/device.ts +++ b/src/core/device.ts @@ -4,6 +4,8 @@ // server hands back an `aek_` API key. Same flow as `gh auth login`. import { ApiClient, DEVICE_CODE_PATH, DEVICE_TOKEN_PATH } from "./transport.js"; +import { HttpError } from "./errors.js"; +import { errTheme } from "../ui/theme.js"; export interface DeviceCode { device_code: string; @@ -41,6 +43,13 @@ export async function requestDeviceCode(api: ApiClient): Promise { return api.postJson(DEVICE_CODE_PATH, { client_id: "aether-cli" }); } +// Consecutive non-HTTP polling failures (network down, DNS failure, timeout, +// a malformed/bodyless response) before we tell the user something looks +// wrong. A single blip shouldn't interrupt "waiting for approval", but a +// sustained outage must not read as silent waiting for the whole expires_in +// window. +const NETWORK_WARN_THRESHOLD = 3; + /** * Poll the token endpoint until the user approves in the portal. Returns the raw * `aek_` token. `sleep` is injected so the loop is testable. Honors `slow_down` @@ -53,15 +62,40 @@ export async function pollForToken( ): Promise { let interval = code.interval; const deadline = Date.now() + code.expires_in * 1000; + let consecutiveNetworkErrors = 0; while (Date.now() < deadline) { await sleep(interval * 1000); let resp: PollResponse; try { resp = await api.postJson(DEVICE_TOKEN_PATH, { device_code: code.device_code }); + consecutiveNetworkErrors = 0; } catch (err) { - // The poll endpoint returns 400 with an `error` body for pending/slow_down/ - // expired; postJson throws HttpError carrying that body. - resp = (err as { body?: PollResponse }).body ?? { error: "authorization_pending" }; + // The poll endpoint returns 400 with a string `error` body for pending/ + // slow_down/expired (RFC 8628); postJson throws HttpError carrying that + // body — that's the ONLY case that should be folded into ordinary + // polling state. Any OTHER thrown error (network down, DNS failure, + // timeout, a malformed/bodyless response, or a non-400 HttpError such + // as a 5xx that merely happens to carry a parseable JSON object body, + // e.g. FastAPI's `{"detail": "..."}`) is a real problem, not "user + // hasn't approved yet" — don't silently reclassify it as + // authorization_pending, or a genuine outage reads as normal polling + // for the whole expires_in window. + if ( + err instanceof HttpError && + err.status === 400 && + typeof (err.body as Record | undefined)?.["error"] === "string" + ) { + resp = err.body as PollResponse; + consecutiveNetworkErrors = 0; + } else { + consecutiveNetworkErrors++; + if (consecutiveNetworkErrors === NETWORK_WARN_THRESHOLD) { + process.stderr.write( + errTheme.dim("⚠ can't reach the server — still trying… (check your connection)\n"), + ); + } + continue; + } } const action = classifyPoll(resp); if (action === "ready") return resp.access_token as string; @@ -69,5 +103,10 @@ export async function pollForToken( if (action === "expired") throw new Error("login timed out — run `aether auth login` again"); if (action === "slow_down") interval += 5; } + if (consecutiveNetworkErrors >= NETWORK_WARN_THRESHOLD) { + throw new Error( + "login timed out — couldn't reach the server while waiting for approval; check your connection and try again", + ); + } throw new Error("login timed out — run `aether auth login` again"); } diff --git a/src/core/error_hints.ts b/src/core/error_hints.ts index 1f19f33..5d143f5 100644 --- a/src/core/error_hints.ts +++ b/src/core/error_hints.ts @@ -2,25 +2,54 @@ // The REPL's printError appends this as a dim second line, so "✗ HTTP 401" // becomes recoverable instead of a dead end. -import { HttpError, InsecureTransportError, StreamTimeoutError, httpStatusHint } from "./errors.js"; +import { + HttpError, + InsecureTransportError, + NETWORK_CODES, + httpStatusHint, + nonHttpErrorHint, + isAbortError as coreIsAbortError, +} from "./errors.js"; /** One-line recovery hint for a thrown error, or null when there's nothing - * actionable to add. */ + * actionable to add. + * + * The baseUrl-independent shapes (malformed response, stream/request + * timeout, incomplete stream) and the 401/402/403/429 wording are shared + * with errors.errorHint via nonHttpErrorHint()/httpStatusHint() (see their + * own doc comments). Only the 5xx and network-failure branches below stay + * separate per-surface: errorHint (REPL) knows the configured baseUrl and + * names it; hintFor (one-shot CLI / embedders, via the public `hintFor` + * export in index.ts) has no baseUrl to hand it and stays generic instead. + * Don't "fix" that divergence by trying to merge the wording without also + * plumbing a baseUrl through hintFor's public signature. */ export function hintFor(err: unknown): string | null { + const shared = nonHttpErrorHint(err); + if (shared !== null) return shared; if (err instanceof HttpError) { + // Deliberately worded differently from errorHint's >=500 branch (no + // baseUrl available here) — see the module-level note above. if (err.status >= 500) return "server hiccup — retry, or /doctor to check connectivity"; // Shared wording with errors.errorHint so the streamed-turn path and the // slash path never show two different hints for the same status. return httpStatusHint(err.status); } - if (err instanceof StreamTimeoutError) { - return "the stream went quiet - retry, or /doctor to check connectivity"; - } if (err instanceof InsecureTransportError) return "set AETHER_BASE_URL to an https endpoint"; if (err instanceof Error) { if (err.name === "AbortError") return null; // user-initiated, already explained const m = err.message; + // undici puts the failure code on err.cause.code (not in the message + // text) for most real fetch failures — e.g. `new Error("request to host + // failed", { cause: { code: "ECONNREFUSED" } })`. The message-substring + // checks below only catch errors that happen to embed the code in their + // text, which errors.errorHint's own test suite shows is not the shape + // undici actually throws. Check cause.code first so this class of error + // isn't silently missed (LOOP-06 round 3) — mirrors errors.errorHint. + const code = (err.cause as { code?: string } | undefined)?.code ?? ""; + // Deliberately worded differently from errorHint's network branch (no + // baseUrl to name here) — see the module-level note above. if ( + NETWORK_CODES.has(code) || m.includes("fetch failed") || m.includes("ECONNREFUSED") || m.includes("ENOTFOUND") || @@ -33,7 +62,9 @@ export function hintFor(err: unknown): string | null { return null; } -/** True when the error is a user-initiated turn abort (Ctrl+C). */ -export function isAbortError(err: unknown): boolean { - return err instanceof Error && err.name === "AbortError"; -} +/** True when the error is a user-initiated turn abort (Ctrl+C). Re-exports + * errors.isAbortError (rather than maintaining a second, narrower check) + * so there is exactly one abort-detection implementation to keep correct — + * this used to be a separate, narrower copy that only checked `err.name`, + * missing the cause-wrapped/regex shapes below. */ +export const isAbortError = coreIsAbortError; diff --git a/src/core/errors.ts b/src/core/errors.ts index ab9ed13..eda2984 100644 --- a/src/core/errors.ts +++ b/src/core/errors.ts @@ -44,6 +44,35 @@ export class StreamTimeoutError extends Error { } } +/** + * A chat/agent SSE stream ended (the underlying byte stream closed normally, + * no throw) without ever delivering a terminal `done` or `error` frame. + * Distinct from StreamTimeoutError (which fires on an IDLE gap while the + * connection is still open): this is a clean-looking close — e.g. a + * proxy/load-balancer that time-boxes the response and drops the socket well + * within the idle window — that otherwise produces zero complaint and would + * render a partial answer as a fully successful turn (LOOP-06 round 3). + */ +export class StreamIncompleteError extends Error { + constructor() { + super("the connection ended before the server finished responding"); + this.name = "StreamIncompleteError"; + } +} + +/** + * A non-streaming authed call (getJson/postJson/deleteJson) got no response + * within its bound. Unlike stream()'s StreamTimeoutError, there's no partial + * data involved — the whole request is unresolved. Distinct name so it can't + * be confused with a genuine stream timeout in logs/tests. + */ +export class RequestTimeoutError extends Error { + constructor(public timeoutMs: number) { + super(`request timed out after ${Math.round(timeoutMs / 1000)}s with no response`); + this.name = "RequestTimeoutError"; + } +} + /** * Refused to send the session token because the base URL is not a secure * transport (non-https to a non-loopback host). Prevents a cleartext credential @@ -71,9 +100,28 @@ export class HttpError extends Error { } } +/** + * The server returned a 2xx status but the body wasn't parseable JSON — it + * said "ok" without giving usable data. Extends HttpError (not a bare Error) + * so existing `instanceof HttpError` classification still applies, but keeps + * its own name/message so callers that used to get a silent `undefined` (and + * then crashed on their own property access, e.g. `cat.models` in slash.ts's + * getCatalog or `r.session_token` in auth.ts's authRefresh) instead get one + * coherent, hintable error. + */ +export class MalformedResponseError extends HttpError { + constructor(status: number) { + super(status, "malformed response from server"); + this.name = "MalformedResponseError"; + } +} + // Network-cause codes that mean "the server never answered" (undici surfaces -// these on err.cause.code for fetch failures). -const NETWORK_CODES = new Set([ +// these on err.cause.code for fetch failures). Exported so error_hints.hintFor +// can check the same set instead of pattern-matching err.message substrings +// only — undici puts the code on err.cause.code, not in the message text +// (LOOP-06 round 3). +export const NETWORK_CODES = new Set([ "ECONNREFUSED", "ENOTFOUND", "ETIMEDOUT", @@ -111,12 +159,38 @@ export function httpStatusHint(status: number): string | null { return null; } +/** + * The ONE wording for the baseUrl-independent thrown-error shapes (malformed + * response, stream timeout, incomplete stream, request timeout) — shared by + * errorHint (REPL) and error_hints.hintFor (embedders/one-shot CLI). These + * hints never mention baseUrl, unlike the HttpError-5xx and network-outage + * branches each caller keeps separately (see errorHint's/hintFor's own doc + * comments for why those two stay per-surface) — so hand-duplicating THESE + * branches instead of sharing them serves no purpose and risks exactly the + * kind of silent wording drift the RequestTimeoutError branch had picked up + * (this function used to be two copies with two different strings for it). + * Checked before the generic HttpError branch in both callers + * (MalformedResponseError extends HttpError): a malformed 2xx body isn't one + * of the auth/plan statuses httpStatusHint knows about, so the generic + * branch would otherwise return null and print the message with no + * actionable next step at all. + */ +export function nonHttpErrorHint(err: unknown): string | null { + if (err instanceof MalformedResponseError) return "retry, or /doctor to check connectivity"; + if (err instanceof StreamTimeoutError) return "the stream went quiet - retry, or /doctor to check connectivity"; + if (err instanceof StreamIncompleteError) return "retry, or /doctor to check connectivity"; + if (err instanceof RequestTimeoutError) return "the request went quiet - retry, or /doctor to check connectivity"; + return null; +} + /** * One actionable next step for a failed turn, or null when there is nothing * better to say than the error itself. Pure — safe to unit test without a * network or a TTY. Consumed under the ✗ line as a dim hint. */ export function errorHint(err: unknown, baseUrl: string): string | null { + const shared = nonHttpErrorHint(err); + if (shared !== null) return shared; if (err instanceof HttpError) { if (err.status >= 500) return `the server at ${baseUrl} had a problem — try again shortly`; return httpStatusHint(err.status); diff --git a/src/core/port.ts b/src/core/port.ts index a318f92..94e5d13 100644 --- a/src/core/port.ts +++ b/src/core/port.ts @@ -3,7 +3,7 @@ import { existsSync, readFileSync, writeFileSync, statSync, mkdirSync, readdirSync } from "node:fs"; import { join, basename } from "node:path"; import type { ApiClient } from "./transport.js"; -import { UVT_PORT_PATH } from "./transport.js"; +import { UVT_PORT_PATH, defaultStreamTimeoutMs } from "./transport.js"; export interface PortRequest { files: Array<{ path: string; content: string }>; @@ -26,11 +26,13 @@ export async function portCode( targetLanguage: string, sourceLanguage?: string, ): Promise { + // Deep multi-file translation, same class as dispatchGeneration (vision.ts) + // — opt into the 120s stream-class timeout instead of the 30s request default. return api.postJson(UVT_PORT_PATH, { files, target_language: targetLanguage, source_language: sourceLanguage, - } as PortRequest); + } as PortRequest, undefined, defaultStreamTimeoutMs()); } // ── File I/O helpers ──────────────────────── diff --git a/src/core/render.ts b/src/core/render.ts index 13c25d9..ea55cd4 100644 --- a/src/core/render.ts +++ b/src/core/render.ts @@ -15,6 +15,7 @@ import { TaskLedger } from "../ui/ledger.js"; import { errTheme } from "../ui/theme.js"; import { sanitizeTerm } from "../ui/text.js"; import { MdStream } from "../ui/md_stream.js"; +import { formatErrorLine } from "../ui/error_line.js"; export interface RenderOptions { json: boolean; @@ -159,12 +160,10 @@ export class Renderer { // glyph goes to stderr, or scrollback interleaves the two out of order. const rest = this.md.flush(); if (rest) this.out.write(rest); - // errTheme.red keeps the glyph consistently red regardless of stdout's - // TTY-ness (it's keyed off stderr); sanitize the server-sourced fields so - // a hostile/buggy server can't smuggle OSC/CSI through an error message. - this.err.write(`\n${errTheme.red("✗")} ${sanitizeTerm(f.msg)}`); - if (f.errorCode) this.err.write(` [${sanitizeTerm(f.errorCode)}]`); - if (f.refId) this.err.write(` (ref ${sanitizeTerm(f.refId)})`); - this.err.write("\n"); + // formatErrorLine (LOOP-06) owns the glyph/hint/separator convention so + // a mid-stream server error reads identically to chat.ts's printError + // for a client-caught one (e.g. the same "session expired" moment no + // longer looks different depending on which path caught it). + this.err.write(formatErrorLine(f.msg, { errorCode: f.errorCode, refId: f.refId })); } } diff --git a/src/core/scaffold.ts b/src/core/scaffold.ts index 6e2d076..fc7120a 100644 --- a/src/core/scaffold.ts +++ b/src/core/scaffold.ts @@ -2,7 +2,7 @@ // Sends template type + name to backend; backend fills via locked-down LLM prompt. import type { ApiClient } from "./transport.js"; -import { UVT_SCAFFOLD_PATH } from "./transport.js"; +import { UVT_SCAFFOLD_PATH, defaultStreamTimeoutMs } from "./transport.js"; export type ScaffoldType = "component" | "route" | "module"; @@ -23,11 +23,13 @@ export async function generateScaffold( name: string, language = "typescript", ): Promise { + // LLM-backed generation, same class as dispatchGeneration (vision.ts) — + // opt into the 120s stream-class timeout instead of the 30s request default. return api.postJson(UVT_SCAFFOLD_PATH, { type, name, language, - } as ScaffoldRequest); + } as ScaffoldRequest, undefined, defaultStreamTimeoutMs()); } // ── Validation ────────────────────────────── diff --git a/src/core/test_drive.ts b/src/core/test_drive.ts index b0ec140..398caed 100644 --- a/src/core/test_drive.ts +++ b/src/core/test_drive.ts @@ -1,7 +1,7 @@ // UVT /test-drive — autonomous TDD loop. Orchestrator-gated. import type { ApiClient } from "./transport.js"; -import { AGENT_TEST_DRIVE_PATH } from "./transport.js"; +import { AGENT_TEST_DRIVE_PATH, defaultStreamTimeoutMs } from "./transport.js"; import { execSync } from "node:child_process"; export interface TestDriveRequest { @@ -32,12 +32,17 @@ export async function startTestDrive( cwd: string, testCmd?: string, ): Promise { - return api.postJson(AGENT_TEST_DRIVE_PATH, { - agent, - target, - cwd, - test_cmd: testCmd, - } as TestDriveRequest); + // Blocks server-side through the whole iterate-until-green loop (returns + // final_result/patches, not a job handle) — same class of long-running + // call as chat's non-streaming fallback, so it opts into stream()'s own + // generous bound instead of request()'s 30s metadata-call default + // (LOOP-01/LOOP-06 round-1). + return api.postJson( + AGENT_TEST_DRIVE_PATH, + { agent, target, cwd, test_cmd: testCmd } as TestDriveRequest, + undefined, + defaultStreamTimeoutMs(), + ); } export function runTests(testCmd = "npx jest --json 2>&1"): TestResult { diff --git a/src/core/transport.ts b/src/core/transport.ts index 7203679..26301cd 100644 --- a/src/core/transport.ts +++ b/src/core/transport.ts @@ -3,8 +3,15 @@ // // Paths are constants so they change in one place. -import { HttpError, InsecureTransportError, StreamTimeoutError, StreamUnavailableError } from "./errors.js"; -import type { TokenStore } from "./auth.js"; +import { + HttpError, + InsecureTransportError, + MalformedResponseError, + RequestTimeoutError, + StreamTimeoutError, + StreamUnavailableError, +} from "./errors.js"; +import { isApiKeyToken, type TokenStore } from "./auth.js"; /** * Is `base` a transport we will attach the session token to? https is allowed to @@ -25,6 +32,26 @@ export function isCredentialSafeUrl(base: string): boolean { return host === "localhost" || host === "127.0.0.1" || host === "::1"; } +/** + * Does `target` share `baseUrl`'s origin (scheme+host+port)? getBinary() uses + * this — NOT isCredentialSafeUrl() — to decide whether the live session + * bearer token should be attached to a caller-supplied absolute URL at all. + * isCredentialSafeUrl() only checks scheme (any https host passes); reusing + * it here would attach the token to ANY https host, including a third-party + * host a server response (or a compromised/malicious one) pointed at, leaking + * the token to it. A target that isn't the API's own origin gets no + * Authorization header, regardless of its own scheme. + */ +export function isSameOrigin(target: string, baseUrl: string): boolean { + try { + const t = new URL(target); + const b = new URL(baseUrl); + return t.protocol === b.protocol && t.host === b.host; + } catch { + return false; + } +} + // Aether API routes. export const CHAT_STREAM_PATH = "/agent/chat/stream"; // standard chat SSE export const CHAT_PATH = "/agent/chat"; // non-streaming fail-soft fallback @@ -86,6 +113,11 @@ export const PROJECT_FROM_WORKFLOW_PLAN_PATH = "/project/from-workflow/plan"; export const PROJECT_FROM_WORKFLOW_FINALIZE_PATH = "/project/from-workflow/finalize"; export const DEFAULT_STREAM_TIMEOUT_MS = 120_000; +// Non-streaming authed calls (getJson/postJson/deleteJson) — /models, /tier, +// auth status/refresh, device-poll, etc. Much shorter than the stream default +// since these are single request/response round-trips, not a long-lived SSE +// body: a stalled connection here should fail fast, not sit for 2 minutes. +export const DEFAULT_REQUEST_TIMEOUT_MS = 30_000; export interface StreamOptions { signal?: AbortSignal; @@ -112,7 +144,7 @@ export class ApiClient { */ private async refreshSession(failedPath: string, usedToken: string | null): Promise { if (failedPath.startsWith("/auth/")) return false; - if (!usedToken || usedToken.startsWith("aek_")) return false; + if (!usedToken || isApiKeyToken(usedToken)) return false; // Same fail-closed rule as authHeaders(): never POST a session token over // an insecure transport — not even to refresh it. if (!isCredentialSafeUrl(this.baseUrl)) return false; @@ -164,7 +196,6 @@ export class ApiClient { return this.baseUrl.replace(/\/$/, "") + path; } - // Kept as the stable seam vision.ts borrows (via cast) for its own fetches. // Internal retry paths pass the token used by that exact attempt; callers // that omit it read the current token from the store. private async authHeaders(token?: string | null): Promise> { @@ -257,56 +288,273 @@ export class ApiClient { } } - async postJson(path: string, body: unknown, signal?: AbortSignal): Promise { - return this.request("POST", path, { body, signal }); + /** `timeoutMs` overrides the default bound (AETHER_REQUEST_TIMEOUT_MS, 30s; + * 0 disables it) — most callers should omit it. It exists for the rare + * non-streaming call that legitimately runs long (e.g. chat.ts's/ + * brain_cloud.ts's/client.ts's CHAT_PATH fallback for a full LLM turn, + * which passes stream()'s own 120s-class bound instead of the 30s default + * meant for metadata/auth calls). */ + async postJson(path: string, body: unknown, signal?: AbortSignal, timeoutMs?: number): Promise { + return this.request("POST", path, { body, signal, timeoutMs }); } - async getJson(path: string, signal?: AbortSignal): Promise { - return this.request("GET", path, { signal }); + async getJson(path: string, signal?: AbortSignal, timeoutMs?: number): Promise { + return this.request("GET", path, { signal, timeoutMs }); + } + + async deleteJson(path: string, signal?: AbortSignal, timeoutMs?: number): Promise { + return this.request("DELETE", path, { signal, timeoutMs }); + } + + /** + * Authed GET for binary/streaming reads — vault file downloads, media asset + * downloads. Goes through the same refresh-on-401 retry as request()/ + * stream(), and throws HttpError (not a plain Error) on a non-2xx response + * so errorHint()/hintFor() can classify it (the seam vault.ts and vision.ts + * used to bypass via a private-member cast — see git history). Accepts + * either a path relative to this.baseUrl OR an absolute http(s) URL, since + * media assets can live on a different host than the API itself. + * + * The bearer token is attached ONLY when `target` shares baseUrl's origin + * (isSameOrigin — scheme+host+port), never merely because it's https: a + * media asset on a different host (a presumably presigned or public CDN) + * has no business receiving the user's live session token just because + * that host also happens to be https. A cross-origin target is fetched + * unauthenticated instead of failing closed — refusing the whole download + * would be worse than simply not sending credentials it was never entitled + * to. + * + * `timeoutMs` (default AETHER_REQUEST_TIMEOUT_MS, 30s; 0 disables) bounds + * the connect/response-headers phase, same as request() — this previously + * had NO timeout at all. Once headers arrive, the body is wrapped with the + * same idle/quiet-period timeout stream() uses (withIdleTimeout) rather + * than a flat overall cap, so a large-but-healthy download can't be killed + * mid-flight just for taking a while. + */ + async getBinary(pathOrUrl: string, signal?: AbortSignal, timeoutMs?: number): Promise { + const target = /^https?:\/\//i.test(pathOrUrl) ? pathOrUrl : this.url(pathOrUrl); + const attachAuth = isSameOrigin(target, this.baseUrl); + const effTimeoutMs = normalizeTimeoutMs(timeoutMs ?? defaultRequestTimeoutMs()); + const net = new AbortController(); + const releaseNet = (): void => net.abort(); + if (signal) { + if (signal.aborted) releaseNet(); + else signal.addEventListener("abort", releaseNet, { once: true }); + } + const cleanup = (): void => signal?.removeEventListener("abort", releaseNet); + try { + let used: string | null = null; + const send = async (): Promise => { + let headers: Record = {}; + if (attachAuth) { + used = await this.tokens.get(); + headers = await this.authHeaders(used); + } + return raceAgainst( + fetch(target, { headers, signal: net.signal }), + signal, + effTimeoutMs, + () => new RequestTimeoutError(effTimeoutMs), + ); + }; + let res = await send(); + if (res.status === 401 && (await this.refreshSession(pathOrUrl, used))) { + void res.body?.cancel().catch(() => {}); + res = await send(); + } + if (!res.ok) throw await toHttpError(res); + if (!res.body) { + cleanup(); + return res; + } + const wrapped = toReadableStream( + withIdleTimeout(res.body as unknown as AsyncIterable, signal, effTimeoutMs, releaseNet, cleanup), + ); + return new Response(wrapped, { status: res.status, statusText: res.statusText, headers: res.headers }); + } catch (err) { + releaseNet(); + cleanup(); + // `target` can be a server-controlled absolute URL (media_url in a + // generation response) with embedded control bytes. Every OTHER thrown + // error here is a typed class this module controls the wording of; + // fetch()'s OWN url-parse failure is a plain TypeError that echoes the + // raw `target` string verbatim in err.message — the one path an OSC/CSI + // sequence from a hostile backend could still reach the terminal + // unsanitized. Strip it the same way every other server-supplied string + // in this module is stripped (sanitizeServerText). + if ( + err instanceof Error && + !(err instanceof HttpError) && + !(err instanceof RequestTimeoutError) && + !(err instanceof InsecureTransportError) && + !(err instanceof StreamTimeoutError) && + !(err instanceof StreamUnavailableError) && + err.name !== "AbortError" + ) { + throw new Error(sanitizeServerText(err.message)); + } + throw err; + } } - async deleteJson(path: string, signal?: AbortSignal): Promise { - return this.request("DELETE", path, { signal }); + /** + * Authed multipart POST — vault file upload. Same refresh-on-401 retry and + * HttpError classification as request(). Content-Type is left for fetch() + * itself to set (so it can add the multipart boundary); only the bearer + * header, if any, is layered on top of the caller's FormData body. + * + * `timeoutMs` (default AETHER_REQUEST_TIMEOUT_MS, 30s; 0 disables) bounds + * the request the same way request() does — this previously had NO timeout + * at all, so a stalled upload connection would hang forever. + */ + async postForm(path: string, form: FormData, signal?: AbortSignal, timeoutMs?: number): Promise { + const effTimeoutMs = normalizeTimeoutMs(timeoutMs ?? defaultRequestTimeoutMs()); + const net = new AbortController(); + const releaseNet = (): void => net.abort(); + if (signal) { + if (signal.aborted) releaseNet(); + else signal.addEventListener("abort", releaseNet, { once: true }); + } + try { + let used: string | null = null; + const send = async (): Promise => { + used = await this.tokens.get(); + return raceAgainst( + fetch(this.url(path), { + method: "POST", + headers: await this.authHeaders(used), + body: form, + signal: net.signal, + }), + signal, + effTimeoutMs, + () => new RequestTimeoutError(effTimeoutMs), + ); + }; + let res = await send(); + if (res.status === 401 && (await this.refreshSession(path, used))) { + void res.body?.cancel().catch(() => {}); + res = await send(); + } + if (!res.ok) throw await toHttpError(res); + return await parseOkBody(res); + } finally { + releaseNet(); + signal?.removeEventListener("abort", releaseNet); + } } private async request( method: string, path: string, - opts: { body?: unknown; signal?: AbortSignal } = {}, + opts: { body?: unknown; signal?: AbortSignal; timeoutMs?: number } = {}, ): Promise { - let used: string | null = null; - const send = async (): Promise => { - used = await this.tokens.get(); - return fetch(this.url(path), { - method, - headers: { - ...(opts.body !== undefined ? { "Content-Type": "application/json" } : {}), - Accept: "application/json", - ...(await this.authHeaders(used)), - }, - ...(opts.body !== undefined ? { body: JSON.stringify(opts.body) } : {}), - ...(opts.signal ? { signal: opts.signal } : {}), - }); - }; - let res = await send(); - if (res.status === 401 && (await this.refreshSession(path, used))) { - // Release the rejected response before retrying (see stream()). - void res.body?.cancel().catch(() => {}); - res = await send(); + // `?? ` (not `||`) so an explicit 0 (disabled) from a caller survives — + // only an OMITTED timeoutMs falls back to the env-driven default. + const timeoutMs = normalizeTimeoutMs(opts.timeoutMs ?? defaultRequestTimeoutMs()); + const signal = opts.signal; + // Bounded by default (AETHER_REQUEST_TIMEOUT_MS, 30s): unlike stream(), + // this had NO timeout at all — a silently-dropped connection to /models, + // /auth/refresh, or the device-poll endpoint would hang forever with + // nothing but the caller's own (often absent) AbortSignal to save it. + // `net` is a SEPARATE controller from the caller's own `signal` (mirrors + // stream()) so releasing the socket on timeout can never be mistaken for + // the caller's own abort. + const net = new AbortController(); + const releaseNet = (): void => net.abort(); + if (signal) { + if (signal.aborted) releaseNet(); + else signal.addEventListener("abort", releaseNet, { once: true }); } - if (!res.ok) throw await toHttpError(res); - // Mirrors toHttpError()'s own defensive res.json() below: a 2xx response - // can still have an empty or non-JSON body (e.g. 204 No Content from the - // new deleteJson()), which would otherwise throw an uncaught SyntaxError - // here instead of letting the caller's own domain check report it. try { - return (await res.json()) as T; - } catch { - return undefined as T; + let used: string | null = null; + const send = async (): Promise => { + used = await this.tokens.get(); + return raceAgainst( + fetch(this.url(path), { + method, + headers: { + ...(opts.body !== undefined ? { "Content-Type": "application/json" } : {}), + Accept: "application/json", + ...(await this.authHeaders(used)), + }, + ...(opts.body !== undefined ? { body: JSON.stringify(opts.body) } : {}), + signal: net.signal, + }), + signal, + timeoutMs, + () => new RequestTimeoutError(timeoutMs), + ); + }; + let res = await send(); + if (res.status === 401 && (await this.refreshSession(path, used))) { + // Release the rejected response before retrying (see stream()). + void res.body?.cancel().catch(() => {}); + res = await send(); + } + if (!res.ok) throw await toHttpError(res); + return await parseOkBody(res); + } finally { + releaseNet(); + signal?.removeEventListener("abort", releaseNet); } } } +/** + * Parse a 2xx response body for request()/postForm(). An EMPTY body (e.g. 204 + * No Content from deleteJson()) is a legitimate "no data" response and yields + * `undefined` — but a NON-empty body that still fails to parse as JSON means + * the server said "ok" and then didn't give usable data (including a + * connection dropped mid-response, leaving truncated JSON), which is a real + * problem the caller's own domain check can't be expected to catch (it + * assumes `T`, not `T | undefined`). That case throws a typed + * MalformedResponseError instead of silently becoming `undefined as T`, so it + * flows through errorHint()/hintFor() like any other server-side failure + * instead of surfacing as a raw property-access TypeError one layer up + * (e.g. slash.ts's getCatalog -> `cat.models`, auth.ts's authRefresh -> + * `r.session_token`). + * + * Reads the raw text FIRST and only treats a genuinely empty (or + * whitespace-only) body as "no content" — classifying by the JSON.parse + * error's message instead (e.g. matching "Unexpected end of JSON input") + * would also match a truncated-but-nonempty body cut short mid-object, which + * is exactly the dropped-connection case this fix exists to catch, not a + * legitimate empty response. + */ +async function parseOkBody(res: Response): Promise { + let text: string; + try { + text = await res.text(); + } catch { + throw new MalformedResponseError(res.status); + } + if (!text.trim()) return undefined as T; + try { + return JSON.parse(text) as T; + } catch { + throw new MalformedResponseError(res.status); + } +} + +/** + * Server-controlled text lands raw in the terminal: strip C0+C1 control chars + * (ESC, single-byte CSI, newlines) and cap the length. Printable residue of a + * stripped escape (e.g. "[31m") is harmless text. + * + * The one canonical treatment for any raw server-supplied string that may be + * embedded in an Error message a caller prints to the terminal — shared by + * toHttpError (below, every non-2xx response) and loginWithPassword's + * `reason` field (auth.ts), which used to build its thrown Error directly + * from the unsanitized, uncapped field, bypassing this exact protection + * (LOOP-06 round 2: an OSC 52 clipboard-hijack payload in a login failure + * `reason` would otherwise reach the terminal verbatim via login.ts's + * headless catch). + */ +export function sanitizeServerText(v: string): string { + return v.replace(/[\x00-\x1f\x7f-\x9f]+/g, " ").trim().slice(0, 200); +} + async function toHttpError(res: Response): Promise { let body: unknown; try { @@ -322,10 +570,7 @@ async function toHttpError(res: Response): Promise { for (const k of ["message", "detail", "reason", "error"]) { const v = (body as Record)[k]; if (typeof v === "string" && v.trim()) { - // Server-controlled text lands raw in the terminal: strip C0+C1 control - // chars (ESC, single-byte CSI, newlines) and cap the length. Printable - // residue of a stripped escape (e.g. "[31m") is harmless text. - msg = `HTTP ${res.status}: ${v.replace(/[\x00-\x1f\x7f-\x9f]+/g, " ").trim().slice(0, 200)}`; + msg = `HTTP ${res.status}: ${sanitizeServerText(v)}`; break; } } @@ -360,19 +605,47 @@ function normalizeTimeoutMs(ms: number): number { return Number.isFinite(ms) && ms > 0 ? Math.floor(ms) : 0; } +/** Exported so tests can pin AETHER_REQUEST_TIMEOUT_MS parsing without a live request. */ +export function defaultRequestTimeoutMs(): number { + const raw = process.env["AETHER_REQUEST_TIMEOUT_MS"]; + if (raw == null || raw.trim() === "") return DEFAULT_REQUEST_TIMEOUT_MS; + const parsed = Number(raw); + // 0 is a valid "disabled" value (see normalizeTimeoutMs) — only fall back to + // the default when the env var is missing or genuinely invalid, never + // silently discard an explicit 0. + if (parsed === 0) return 0; + return normalizeTimeoutMs(parsed) || DEFAULT_REQUEST_TIMEOUT_MS; +} + /** Races `promise` against the caller's own abort and an independent timeout * timer. Each loses its race with its own error (the caller's real - * AbortError, or a fresh StreamTimeoutError) — there's no shared mutable - * "reason" field for the two to race over, so neither can be mistaken for - * the other regardless of which fires first. */ -function raceAgainst(promise: Promise, signal: AbortSignal | undefined, timeoutMs: number): Promise { - if (signal?.aborted) return Promise.reject(abortError(signal)); + * AbortError, or `onTimeout()`'s error — StreamTimeoutError for stream(), + * RequestTimeoutError for request()) — there's no shared mutable "reason" + * field for the two to race over, so neither can be mistaken for the other + * regardless of which fires first. */ +function raceAgainst( + promise: Promise, + signal: AbortSignal | undefined, + timeoutMs: number, + onTimeout: () => unknown = () => new StreamTimeoutError(timeoutMs), +): Promise { + if (signal?.aborted) { + // `promise` (e.g. fetch(net.signal), already argument-evaluated by the + // caller before raceAgainst runs) is being discarded in favor of one + // consistent abortError(signal) result below -- but it can still go on + // to reject on its own (net.signal was wired to the same abort). Always + // attach a swallow-only catch so THAT rejection can never surface as an + // unhandled promise rejection (mirrors withIdleTimeout's iterator.return() + // handling below). + promise.catch(() => {}); + return Promise.reject(abortError(signal)); + } const racers: Promise[] = [promise]; let timer: ReturnType | undefined; if (timeoutMs > 0) { racers.push( new Promise((_, reject) => { - timer = setTimeout(() => reject(new StreamTimeoutError(timeoutMs)), timeoutMs); + timer = setTimeout(() => reject(onTimeout()), timeoutMs); timer.unref?.(); }), ); @@ -419,3 +692,26 @@ async function* withIdleTimeout( iterator.return?.()?.catch(() => {}); } } + +/** + * Wrap an async iterable (withIdleTimeout()'s output) as a Web ReadableStream + * so getBinary() can hand back a genuine Response whose `.body` still behaves + * like a normal fetch body for its callers (vault.ts/vision.ts pipe it + * straight to disk). A pull() rejection — an idle-timeout or abort surfacing + * from the wrapped iterator — errors the stream instead of hanging it; per + * the Streams spec a rejected pull() automatically errors the stream with + * that reason, so no separate try/catch is needed here. + */ +function toReadableStream(iterable: AsyncIterable): ReadableStream { + const iterator = iterable[Symbol.asyncIterator](); + return new ReadableStream({ + async pull(controller) { + const next = await iterator.next(); + if (next.done) controller.close(); + else controller.enqueue(next.value); + }, + async cancel(reason) { + await iterator.return?.(reason); + }, + }); +} diff --git a/src/core/vault.ts b/src/core/vault.ts index 40a71ad..ded36ef 100644 --- a/src/core/vault.ts +++ b/src/core/vault.ts @@ -1,9 +1,11 @@ // src/core/vault.ts — vault API client, same pattern as github.ts // -// Every function wraps a single ApiClient call. Download/delete use raw fetch() -// because ApiClient doesn't expose those methods yet. Same auth header pattern. +// Every function wraps a single ApiClient call. Download/upload/delete go +// through ApiClient's authed getBinary()/postForm()/deleteJson() (not raw +// fetch()) so they get the same refresh-on-401 retry and HttpError +// classification as every other call. -import { ApiClient, isCredentialSafeUrl } from "./transport.js"; +import { ApiClient, defaultStreamTimeoutMs } from "./transport.js"; import { VAULT_LIST_PATH, VAULT_BROWSE_PATH, VAULT_SPACES_LIST_PATH, VAULT_SPACES_USAGE_PATH, @@ -14,7 +16,6 @@ import { VAULT_NOTES_OUTLINKS_PATH, VAULT_NOTES_TREE_PATH, AGENT_VAULT_SNAPSHOT_PATH, AGENT_VAULT_SLASH_PATH, } from "./transport.js"; -import { InsecureTransportError } from "./errors.js"; // ── Response types ──────────────────────────────── @@ -89,33 +90,6 @@ export interface VaultSlashResponse { command: string; content: string; note_count: number; ok: boolean; error?: string; } -// ── Helpers ────────────────────────────────────── - -/** Extract Bearer token from ApiClient's token store — same auth as every other call. */ -async function _bearerToken(api: ApiClient): Promise { - // ApiClient keeps TokenStore as private `tokens`. Access via internal cast. - try { - const t = (api as unknown as { tokens: { get: () => Promise } }).tokens; - return await t.get(); - } catch { - return null; - } -} - -function _baseUrl(api: ApiClient): string { - const b = (api as unknown as { baseUrl: string }).baseUrl; - return b.replace(/\/$/, ""); -} - -async function _authHeaders(api: ApiClient): Promise> { - const t = await _bearerToken(api); - if (!t) return {}; - // Fail closed: never put the bearer on an insecure transport, same as ApiClient.authHeaders. - const base = _baseUrl(api); - if (!isCredentialSafeUrl(base)) throw new InsecureTransportError(base); - return { Authorization: `Bearer ${t}` }; -} - // ── Vault list / browse ────────────────────────── export async function getVaultList(api: ApiClient): Promise { @@ -137,45 +111,60 @@ export async function getSpacesUsage(api: ApiClient): Promise return api.getJson(VAULT_SPACES_USAGE_PATH); } -export async function downloadSpacesFile(api: ApiClient, filename: string): Promise { - const headers = await _authHeaders(api); - const res = await fetch( - _baseUrl(api) + VAULT_SPACES_DOWNLOAD_PATH + "/" + encodeURIComponent(filename), - { headers }, - ); - if (!res.ok) throw new Error(`download failed: HTTP ${res.status}`); - return res.arrayBuffer(); -} - -/** Upload a local file to the vault. Uses multipart form upload. */ +/** + * Upload a local file to the vault. Uses multipart form upload. + * + * The file is handed to FormData as an fs.openAsBlob() Blob — backed by the + * open file handle itself — instead of fs.readFileSync() + new Blob([data]). + * The old path buffered the ENTIRE file into a Buffer and then copied it a + * second time into an in-memory Blob before any bytes went over the wire; + * openAsBlob() defers reading to whenever the multipart encoder actually + * streams the part out, so a large vault upload no longer holds the whole + * file in memory twice (mirrors downloadFile()'s stream-straight-to-disk fix + * for the download side, 1d33357). + * + * Passes defaultStreamTimeoutMs() (120s) as postForm()'s timeoutMs instead of + * letting it fall back to the 30s metadata-call default (AETHER_REQUEST_TIMEOUT_MS). + * postForm() bounds the ENTIRE request — connect + send-body + wait-for-response + * — with one flat timer; unlike getBinary()/stream(), it can't split that into + * a connect-phase timeout plus a separate idle timeout on the body, because + * fetch() only resolves once the *outgoing* multipart body has been fully sent + * and exposes no hook to observe upload progress in between. A flat 30s bound + * meant any upload whose transfer legitimately took longer (a large file, a + * slow/mobile connection) would always fail with RequestTimeoutError even while + * data was actively flowing. A file upload's duration profile is far closer to + * a full LLM turn (client.ts's/workflow.ts's CHAT_PATH fallback, which already + * passes this same stream-class timeout) than to a /models lookup, so it gets + * the same treatment. This is a partial fix, not the ideal connect-vs-idle + * split: an upload on a very slow link can still hit the flat 120s cap. + */ export async function uploadFile( api: ApiClient, filePath: string, ): Promise<{ key: string; filename: string; size: number; content_type: string }> { const fs = await import("node:fs"); const path = await import("node:path"); - const data = fs.readFileSync(filePath); const filename = path.basename(filePath); + const blob = await fs.openAsBlob(filePath); const formData = new FormData(); - formData.append("file", new Blob([data]), filename); - const headers = await _authHeaders(api); - const res = await fetch(_baseUrl(api) + VAULT_SPACES_UPLOAD_PATH, { - method: "POST", headers, body: formData, - }); - if (!res.ok) { - const errBody = await res.text().catch(() => ""); - throw new Error(`upload failed: HTTP ${res.status}${errBody ? " — " + errBody.slice(0, 200) : ""}`); - } - return res.json(); -} - -/** Download a vault file and save it to a local path. Returns the output path. */ + formData.append("file", blob, filename); + return api.postForm(VAULT_SPACES_UPLOAD_PATH, formData, undefined, defaultStreamTimeoutMs()); +} + +/** + * Download a vault file and save it to a local path. Streams the response + * body straight to disk (no full-file buffering) so a large or misbehaving + * download can't grow the CLI process's memory unbounded. Returns the + * output path. + */ export async function downloadFile(api: ApiClient, filename: string, outputPath: string): Promise { - const buffer = await downloadSpacesFile(api, filename); + const res = await api.getBinary(VAULT_SPACES_DOWNLOAD_PATH + "/" + encodeURIComponent(filename)); + if (!res.body) throw new Error("download failed: empty body"); const fs = await import("node:fs"); const path = await import("node:path"); + const { pipeline } = await import("node:stream/promises"); const dir = path.dirname(outputPath); fs.mkdirSync(dir, { recursive: true }); - fs.writeFileSync(outputPath, Buffer.from(buffer)); + await pipeline(res.body as unknown as NodeJS.ReadableStream, fs.createWriteStream(outputPath)); return outputPath; } @@ -188,13 +177,7 @@ export async function getSpacesContent( export async function deleteSpacesFile( api: ApiClient, filename: string, ): Promise<{ success: boolean; deleted: string }> { - const headers = await _authHeaders(api); - const res = await fetch( - _baseUrl(api) + VAULT_SPACES_DELETE_PATH + "/" + encodeURIComponent(filename), - { method: "DELETE", headers }, - ); - if (!res.ok) throw new Error(`delete failed: HTTP ${res.status}`); - return res.json() as Promise<{ success: boolean; deleted: string }>; + return api.deleteJson(VAULT_SPACES_DELETE_PATH + "/" + encodeURIComponent(filename)); } // ── Notes search ──────────────────────────────── diff --git a/src/core/vision.ts b/src/core/vision.ts index 120495b..8408be7 100644 --- a/src/core/vision.ts +++ b/src/core/vision.ts @@ -4,8 +4,7 @@ // download helpers with streaming fetch, output manager with persistent log. // No terminal I/O. Every function wraps a single concept. -import { ApiClient, isCredentialSafeUrl } from "./transport.js"; -import { InsecureTransportError } from "./errors.js"; +import { ApiClient, defaultStreamTimeoutMs } from "./transport.js"; import type { CatalogItem } from "../types.js"; import { createWriteStream, mkdirSync, readdirSync, statSync, existsSync, readFileSync, writeFileSync } from "node:fs"; import { basename, join } from "node:path"; @@ -190,13 +189,17 @@ export function buildMediaPrompt(prompt: string, flags: GenFlags, _kind: MediaKi export async function dispatchGeneration( api: ApiClient, prompt: string, modelKey: string, flags: GenFlags, ): Promise { + // Same generation-class call as CHAT_PATH elsewhere (chat.ts/brain_cloud.ts/ + // client.ts) — media generation can legitimately run well past the 30s + // default request timeout, so it opts into the same 120s stream-class bound + // instead (undefined signal: this call isn't user-cancelable mid-flight). return api.postJson("/agent/chat", { query: prompt, forced_model_key: modelKey, media_mode: true, mode: "plan", ...(flags.ref ? { ref_image: flags.ref } : {}), - }); + }, undefined, defaultStreamTimeoutMs()); } // ═════════════════════════════════════════════════════════════════════ @@ -210,12 +213,6 @@ export function ensureOutputDir(): string { return OUTPUT_DIR; } -async function authHeaders(api: ApiClient): Promise> { - return (api as unknown as { - authHeaders: () => Promise> - }).authHeaders(); -} - function mediaExt(modelKey: string, kind: MediaKind): string { if (kind === "video") return ".mp4"; if (kind === "3d") return ".glb"; @@ -227,15 +224,12 @@ export async function downloadMediaFile( api: ApiClient, url: string, destDir: string, modelKey: string, kind: MediaKind, label?: string, ): Promise { - const headers = await authHeaders(api); - // Only enforce the credential-safe-transport guard when a bearer token is - // actually about to be attached — mirrors transport.ts's authHeaders()/ - // vault.ts's _authHeaders() token-conditional pattern. An anonymous session - // or a non-loopback self-hosted media host has nothing to leak, so it - // shouldn't hard-fail the download. - if ("Authorization" in headers && !isCredentialSafeUrl(url)) throw new InsecureTransportError(url); - const resp = await fetch(url, { headers }); - if (!resp.ok) throw new Error(`download failed: HTTP ${resp.status}`); + // getBinary() attaches the same bearer token as every other authed call + // (with the same refresh-on-401 retry) and, when a token is actually about + // to be attached, fail-closes if `url` isn't a credential-safe transport — + // an anonymous session or a non-loopback self-hosted media host has + // nothing to leak, so that case shouldn't hard-fail the download. + const resp = await api.getBinary(url); if (!resp.body) throw new Error("download failed: empty body"); let filename = label ? label.replace(/[^a-zA-Z0-9._-]/g, "_") : undefined; @@ -404,10 +398,12 @@ export async function parseStoryboard( ): Promise { const style = options?.style ?? "cinematic"; const content = sourceType === "script_file" ? readFileSync(source, "utf-8") : source; + // Same generation-class call as dispatchGeneration above — opt into the + // 120s stream-class timeout, not the 30s request default. const resp = await api.postJson("/agent/chat", { query: `STORYBOARD REQUEST:\n\n${content}`, forced_model_key: "sonnet", mode: "plan", - }); + }, undefined, defaultStreamTimeoutMs()); const raw = resp.response || resp.text || ""; const scenes = parseScenes(raw); const title = sourceType === "prompt" diff --git a/src/core/workflow.ts b/src/core/workflow.ts index 3a50c51..40d86d2 100644 --- a/src/core/workflow.ts +++ b/src/core/workflow.ts @@ -5,6 +5,7 @@ import { ApiClient } from "./transport.js"; import { PROJECT_FROM_WORKFLOW_ASSESS_PATH, PROJECT_FROM_WORKFLOW_BRAINSTORM_PATH, PROJECT_FROM_WORKFLOW_PLAN_PATH, PROJECT_FROM_WORKFLOW_FINALIZE_PATH, + defaultStreamTimeoutMs, } from "./transport.js"; import { listSpaces, getSpacesContent, deleteSpacesFile, uploadFile, downloadFile } from "./vault.js"; @@ -277,9 +278,15 @@ export function createFenceParser(): FenceParser { } // ── Project Conversion API Wrappers ────────────── +// These block server-side on completed LLM generation (an assessment, +// brainstorm round, plan, or finalized project — not a job handle), the same +// class of long-running call as chat's non-streaming fallback. Each opts +// into stream()'s own generous bound instead of request()'s 30s +// metadata-call default (LOOP-01/LOOP-06 round-1) so a healthy but slow +// generation isn't killed early. export async function assessWorkflow(api: ApiClient, workflow: Workflow): Promise { - return api.postJson(PROJECT_FROM_WORKFLOW_ASSESS_PATH, { workflow }); + return api.postJson(PROJECT_FROM_WORKFLOW_ASSESS_PATH, { workflow }, undefined, defaultStreamTimeoutMs()); } export async function brainstormWorkflow( @@ -289,7 +296,7 @@ export async function brainstormWorkflow( ): Promise { return api.postJson(PROJECT_FROM_WORKFLOW_BRAINSTORM_PATH, { workflow, qa_history: qaHistory, next_index: nextIndex, - }); + }, undefined, defaultStreamTimeoutMs()); } export async function planWorkflow( @@ -297,7 +304,7 @@ export async function planWorkflow( ): Promise { return api.postJson(PROJECT_FROM_WORKFLOW_PLAN_PATH, { workflow, brainstorm_summary: brainstormSummary, mode, - }); + }, undefined, defaultStreamTimeoutMs()); } export async function finalizeWorkflow( @@ -305,42 +312,47 @@ export async function finalizeWorkflow( ): Promise { return api.postJson(PROJECT_FROM_WORKFLOW_FINALIZE_PATH, { workflow, plan_md: planMd, edited_by_user: false, - }); + }, undefined, defaultStreamTimeoutMs()); } // ── Vault-Based Workflow CRUD ──────────────────── const WF_EXT = ".aetherflow.json"; +// listSpaces/getSpacesContent/deleteSpacesFile (vault.ts) never throw for a +// legitimate empty-result case (an empty vault is a 200 with `files: []`; a +// missing file's absence shows up as `content: null`/`binary`, not an +// exception) — so a thrown error here is always a REAL failure (expired +// session, network outage, 5xx). These used to swallow that into an empty +// list / null / false, which the command layer then reported as "no +// workflows" / "not found" / "delete failed", hiding e.g. a 401 session +// expiry with zero indication to run `aether auth login`. Let it propagate +// instead — every call site already wraps these in its own try/catch that +// calls fail(err) (see src/commands/workflow.ts), same as vault.ts's direct +// (uncaught) use of getSpacesContent/deleteSpacesFile. export async function listWorkflows(api: ApiClient): Promise { - try { - const r = await listSpaces(api); - return r.files - .filter(f => f.filename.endsWith(WF_EXT)) - .map(f => ({ - name: f.filename.slice(0, -WF_EXT.length), - filename: f.filename, - size: f.size, - lastModified: f.last_modified, - })); - } catch { return []; } + const r = await listSpaces(api); + return r.files + .filter(f => f.filename.endsWith(WF_EXT)) + .map(f => ({ + name: f.filename.slice(0, -WF_EXT.length), + filename: f.filename, + size: f.size, + lastModified: f.last_modified, + })); } export async function getWorkflow(api: ApiClient, name: string): Promise { const filename = name.endsWith(WF_EXT) ? name : name + WF_EXT; - try { - const r = await getSpacesContent(api, filename); - if (r.binary || !r.content) return null; - return JSON.parse(r.content) as Workflow; - } catch { return null; } + const r = await getSpacesContent(api, filename); + if (r.binary || !r.content) return null; + return JSON.parse(r.content) as Workflow; } export async function deleteWorkflow(api: ApiClient, name: string): Promise { const filename = name.endsWith(WF_EXT) ? name : name + WF_EXT; - try { - await deleteSpacesFile(api, filename); - return true; - } catch { return false; } + await deleteSpacesFile(api, filename); + return true; } /** Save a workflow object to vault by writing JSON to temp file, uploading it. */ diff --git a/src/ui/error_line.ts b/src/ui/error_line.ts new file mode 100644 index 0000000..8cf35ae --- /dev/null +++ b/src/ui/error_line.ts @@ -0,0 +1,40 @@ +// src/ui/error_line.ts — the ONE way a "✗ " error line is written to a +// terminal, shared by chat.ts's printError (client-caught errors: network, +// timeouts, malformed responses) and render.ts's Renderer.error (a +// server-streamed `error` SSE frame mid-turn). Before this, the same +// "session expired" moment read differently depending on which path caught +// it: printError added a dim " ⤷ hint" line plus the trailing blank-line +// separator PR #47 added specifically to stop the hint fusing with the next +// prompt; Renderer.error had neither. LOOP-06. +import { errTheme } from "./theme.js"; +import { sanitizeTerm } from "./text.js"; + +export interface ErrorLineOptions { + /** Dim actionable next step, printed on its own indented line. */ + hint?: string | null; + /** Machine-readable error code (server-streamed errors only, e.g. `f.errorCode`). */ + errorCode?: string; + /** Server-assigned correlation id for support/logs (`f.refId`). */ + refId?: string; +} + +/** + * Compose one "✗ [code] (ref id)" line, an optional dim hint line, and + * a trailing blank-line separator — ready to `write()` verbatim to stderr. + * Every piece of text is sanitized: mandatory for server-streamed fields + * (msg/errorCode/refId can carry attacker-controlled OSC/CSI), and a safe + * no-op for locally-generated client error messages/hints — so both call + * sites get identical defenses instead of only the one that used to bother. + */ +export function formatErrorLine(msg: string, opts: ErrorLineOptions = {}): string { + let head = `${errTheme.red("✗")} ${sanitizeTerm(msg)}`; + if (opts.errorCode) head += ` [${sanitizeTerm(opts.errorCode)}]`; + if (opts.refId) head += ` (ref ${sanitizeTerm(opts.refId)})`; + let out = `\n${head}\n`; + if (opts.hint) out += errTheme.dim(` ⤷ ${sanitizeTerm(opts.hint)}`) + "\n"; + // Trailing separator: without it, a REPL that reprints its prompt right + // after this line fuses onto the hint/message (the exact "⤷ run `aether + // auth login` … [user]_:" mess PR #47 fixed for printError alone). + out += "\n"; + return out; +} diff --git a/src/ui/model_picker.ts b/src/ui/model_picker.ts index 3a5590e..1c583d2 100644 --- a/src/ui/model_picker.ts +++ b/src/ui/model_picker.ts @@ -163,15 +163,20 @@ const DIM_ORB = theme.dim("\u25cb"); // ○ * Launch an interactive model/orchestrator picker. * * Temporarily removes the REPL's data listener, renders the menu, - * processes arrow keys, and returns the selected item or null (cancelled). + * processes arrow keys, and returns the selected item; null on a deliberate + * cancel (Escape/no models/non-TTY) — the caller is expected to print its own + * "kept current session" message for null. Returns undefined on an internal + * key-handler fault: this function has ALREADY written its own distinct + * diagnostic in that case, so the caller must NOT also print a generic + * message (that used to produce two back-to-back lines for one failure). * Restores the REPL listener before resolving. */ export async function pickModel( items: CatalogItem[], out: Writable, -): Promise { +): Promise { if (items.length === 0) { - out.write("no models available.\n"); + out.write(theme.dim("no models available.") + "\n"); return null; } @@ -184,7 +189,7 @@ export async function pickModel( const groups = groupItems(items); const flat = flattenGroups(groups); if (flat.length === 0) { - out.write("no models available.\n"); + out.write(theme.dim("no models available.") + "\n"); return null; } @@ -240,9 +245,15 @@ export async function pickModel( } } catch { // If anything throws in the key handler (render, decode), bail out - // and restore the REPL listeners so the session isn't bricked. + // and restore the REPL listeners so the session isn't bricked. Write + // a distinct diagnostic first, then resolve undefined (not null) so + // the caller (slash.ts's showPicker) can tell this apart from a + // deliberate Escape and skip ITS OWN generic "kept current session." + // message — resolving null there produced two back-to-back lines + // for a single fault. cleanup(); - resolve(null); + out.write(theme.dim(" picker error — kept current session.") + "\n"); + resolve(undefined); } }; diff --git a/test/abort.test.ts b/test/abort.test.ts index 46c1a7d..83d7f86 100644 --- a/test/abort.test.ts +++ b/test/abort.test.ts @@ -62,13 +62,27 @@ test("the abort signal reaches fetch on the streaming leg", async () => { test("the abort signal reaches fetch on the fallback leg too", async () => { const capture: { signal?: AbortSignal | null } = {}; const real = globalThis.fetch; - globalThis.fetch = stubFetch(capture); + // Hangs (rather than resolving immediately) so there's a genuine in-flight + // window to abort during — request()'s internal `net` controller is only + // wired to the caller's abort event while the call is still pending; once + // it settles, that listener is already detached (see request()'s finally). + globalThis.fetch = (async (_url: unknown, init?: RequestInit) => { + capture.signal = init?.signal; + return new Promise(() => {}); + }) as typeof globalThis.fetch; try { const api = new ApiClient("https://example.test", tokens); const ac = new AbortController(); - const r = await api.postJson<{ response: string }>(CHAT_PATH, {}, ac.signal); - assert.equal(r.response, "ok"); - assert.equal(capture.signal, ac.signal); + const pending = api.postJson<{ response: string }>(CHAT_PATH, {}, ac.signal); + ac.abort(); + await assert.rejects(pending, (err: unknown) => isAbortError(err)); + // request() (LOOP-01/LOOP-06 round-1: bounded-by-default timeout) now + // wires the caller's signal to its OWN internal `net` AbortController, + // same reasoning as the streaming leg above — so we assert the + // behavioral guarantee (aborting the caller's controller aborts whatever + // signal fetch got), not reference identity. + assert.ok(capture.signal, "fetch should have received a signal"); + assert.equal(capture.signal?.aborted, true); } finally { globalThis.fetch = real; } diff --git a/test/auth.test.ts b/test/auth.test.ts index 21d2899..854adaa 100644 --- a/test/auth.test.ts +++ b/test/auth.test.ts @@ -4,7 +4,36 @@ import { mkdtempSync, rmSync, existsSync, readFileSync, statSync } from "node:fs import { tmpdir } from "node:os"; import { join } from "node:path"; import { isApiToken } from "../src/commands/auth.js"; -import { FileTokenStore } from "../src/core/auth.js"; +import { FileTokenStore, StaticTokenStore, loginWithPassword, isApiKeyToken } from "../src/core/auth.js"; + +const REQUEST_TIMEOUT_ENV_KEY = "AETHER_REQUEST_TIMEOUT_MS"; + +function setRequestTimeoutMs(value: string | undefined): () => void { + const original = process.env[REQUEST_TIMEOUT_ENV_KEY]; + if (value === undefined) delete process.env[REQUEST_TIMEOUT_ENV_KEY]; + else process.env[REQUEST_TIMEOUT_ENV_KEY] = value; + return () => { + if (original === undefined) delete process.env[REQUEST_TIMEOUT_ENV_KEY]; + else process.env[REQUEST_TIMEOUT_ENV_KEY] = original; + }; +} + +/** A fetch stub that honors the caller's AbortSignal like a real fetch would + * (rejecting when it fires) but otherwise never settles — mirrors a + * silently-dropped connection to /auth/login. */ +function hangingFetchHonoringAbort(): typeof fetch { + return (async (_url: unknown, init?: RequestInit) => { + return new Promise((_resolve, reject) => { + const signal = init?.signal; + if (!signal) return; // never settles + if (signal.aborted) { + reject(signal.reason); + return; + } + signal.addEventListener("abort", () => reject(signal.reason), { once: true }); + }); + }) as typeof fetch; +} test("isApiToken detects an aek_ API token vs a session token", () => { assert.equal(isApiToken("aek_abc123"), true); @@ -14,6 +43,21 @@ test("isApiToken detects an aek_ API token vs a session token", () => { assert.equal(isApiToken(""), false); }); +// LOOP-01 round 2 (LOW): the aek_ prefix check used to be hand-duplicated in +// commands/auth.ts's isApiToken AND transport.ts's refreshSession. Both now +// delegate to this one canonical core/auth.ts export — test it directly (not +// just transitively through isApiToken above) so a regression in the shared +// definition can't hide behind commands/auth.ts's wrapper alone. +test("isApiKeyToken (the canonical core/auth.ts definition shared by commands/auth.ts's isApiToken and transport.ts's refreshSession) detects the aek_ prefix", () => { + assert.equal(isApiKeyToken("aek_abc123"), true); + assert.equal(isApiKeyToken("sess-xyz"), false); + assert.equal(isApiKeyToken(null), false); + assert.equal(isApiKeyToken(undefined), false); + assert.equal(isApiKeyToken(""), false); + // isApiToken is a thin wrapper — the two must never drift apart. + assert.equal(isApiToken("aek_abc123"), isApiKeyToken("aek_abc123")); +}); + test("FileTokenStore writes the token owner-only (0600) and round-trips", async () => { const dir = mkdtempSync(join(tmpdir(), "aether-tok-")); const prev = process.env["AETHER_CONFIG_DIR"]; @@ -37,3 +81,140 @@ test("FileTokenStore writes the token owner-only (0600) and round-trips", async rmSync(dir, { recursive: true, force: true }); } }); + +// ── LOOP-01 round-1 regression: loginWithPassword must not hang forever ── +// on a stalled /auth/login response. This is the headless `--username/ +// --password` flow (login.ts), explicitly meant for CI/scripts, and it's a +// raw fetch() with no ApiClient behind it — so it needs its own bound rather +// than inheriting request()'s. +test("loginWithPassword: a stalled /auth/login response times out instead of hanging the headless login forever", async () => { + const realFetch = globalThis.fetch; + const restoreEnv = setRequestTimeoutMs("5"); + globalThis.fetch = hangingFetchHonoringAbort(); + try { + await assert.rejects(() => + loginWithPassword("https://api.example", new StaticTokenStore(""), { + username: "u", + password: "p", + }), + ); + } finally { + globalThis.fetch = realFetch; + restoreEnv(); + } +}); + +// ── LOOP-06 round 2: a malicious `reason` field must not survive raw ── +// into the thrown Error's message. login.ts's headless `--username/ +// --password` catch writes err.message straight to process.stderr with no +// sanitization of its own, so this field is the last line of defense against +// a compromised/misconfigured backend (or a self-hosted dev server) smuggling +// terminal escape sequences — including OSC 52 clipboard-hijack payloads — +// into the user's terminal. +test("loginWithPassword: a malicious `reason` field is stripped of control chars and length-capped before it reaches the thrown Error's message", async () => { + const realFetch = globalThis.fetch; + // Raw ESC byte + an OSC-style clipboard-hijack-shaped payload + padding well + // past the 200-char cap this mirrors from toHttpError's sanitizeServerText. + const evilReason = "\x1b]52;c;ZXZpbA==\x07" + "A".repeat(300); + globalThis.fetch = (async () => + new Response(JSON.stringify({ authenticated: false, reason: evilReason }), { + status: 401, + headers: { "content-type": "application/json" }, + })) as typeof fetch; + try { + await assert.rejects( + () => loginWithPassword("https://api.example", new StaticTokenStore(""), { username: "u", password: "p" }), + (err: unknown) => { + assert.ok(err instanceof Error); + const msg = err.message; + assert.ok(!msg.includes("\x1b"), "raw ESC byte must never survive into the message"); + assert.ok(!/[\x00-\x08\x0b-\x1f\x7f-\x9f]/.test(msg), "no other C0/C1 control bytes may survive either"); + assert.ok(msg.length < 250, `message must be length-capped, got ${msg.length} chars`); + assert.match(msg, /^login failed: /); + return true; + }, + ); + } finally { + globalThis.fetch = realFetch; + } +}); + +test("loginWithPassword: a non-string `reason` (e.g. a compromised server sending an object/number) falls back to the HTTP status instead of leaking it verbatim", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => + new Response(JSON.stringify({ authenticated: false, reason: { evil: "\x1b[31mpayload" } }), { + status: 403, + headers: { "content-type": "application/json" }, + })) as typeof fetch; + try { + await assert.rejects( + () => loginWithPassword("https://api.example", new StaticTokenStore(""), { username: "u", password: "p" }), + (err: unknown) => { + assert.ok(err instanceof Error); + assert.equal(err.message, "login failed: HTTP 403"); + return true; + }, + ); + } finally { + globalThis.fetch = realFetch; + } +}); + +// ── LOOP-06 round 2 (advisor follow-up): the SUCCESS path carries the same +// hazard — login.ts:74 writes `plan` straight to stdout +// (`✓ Logged in (plan: ${r.plan}).`) with no sanitization of its own. +test("loginWithPassword: a malicious `plan`/`commitment_hash` on a SUCCESSFUL login is sanitized before it reaches the caller", async () => { + const realFetch = globalThis.fetch; + const evilPlan = "\x1b]52;c;ZXZpbA==\x07pro" + "C".repeat(300); + const evilHash = "\x1b[31mhash" + "D".repeat(300); + globalThis.fetch = (async () => + new Response( + JSON.stringify({ + authenticated: true, + session_token: "sess_ok", + plan: evilPlan, + commitment_hash: evilHash, + }), + { status: 200, headers: { "content-type": "application/json" } }, + )) as typeof fetch; + try { + const result = await loginWithPassword("https://api.example", new StaticTokenStore(""), { + username: "u", + password: "p", + }); + assert.ok(result.plan); + assert.ok(!result.plan!.includes("\x1b"), "raw ESC byte must never survive in `plan`"); + assert.ok(result.plan!.length < 210, `plan must be length-capped, got ${result.plan!.length} chars`); + assert.ok(result.commitmentHash); + assert.ok(!result.commitmentHash!.includes("\x1b"), "raw ESC byte must never survive in `commitmentHash`"); + assert.ok( + result.commitmentHash!.length < 210, + `commitmentHash must be length-capped, got ${result.commitmentHash!.length} chars`, + ); + } finally { + globalThis.fetch = realFetch; + } +}); + +test("loginWithPassword: AETHER_REQUEST_TIMEOUT_MS=0 disables the timeout (no AbortSignal attached)", async () => { + const realFetch = globalThis.fetch; + const restoreEnv = setRequestTimeoutMs("0"); + let sawSignal = false; + globalThis.fetch = (async (_url: unknown, init?: RequestInit) => { + sawSignal = init?.signal != null; + return new Response(JSON.stringify({ authenticated: true, session_token: "sess_x" }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }) as typeof fetch; + try { + const store = new StaticTokenStore(""); + const result = await loginWithPassword("https://api.example", store, { username: "u", password: "p" }); + assert.ok(result); + assert.equal(sawSignal, false, "0 means disabled — no AbortSignal.timeout(0), which would abort immediately"); + assert.equal(await store.get(), "sess_x"); + } finally { + globalThis.fetch = realFetch; + restoreEnv(); + } +}); diff --git a/test/auth_401.test.ts b/test/auth_401.test.ts index 0358751..682e2b5 100644 --- a/test/auth_401.test.ts +++ b/test/auth_401.test.ts @@ -1,5 +1,5 @@ // Regression tests for the "login succeeds but model-select throws HTTP 401" -// bug (PR #47). Three layers: +// bug (PR #47). Four layers: // 1. tokenStoreFromEnv: a fresh login must PERSIST even when AETHER_TOKEN is // injected — previously it vanished with the process, so every later run // re-read the stale env token and 401'd despite "✓ Logged in." @@ -7,6 +7,18 @@ // + retry before the 401 surfaces. aek_ API keys never trigger refresh. // 3. errorHint: 401 / 402 / 403 are distinct + the server's own detail // (e.g. a UVT-balance message) is surfaced instead of a bare "HTTP 401". +// 4. renderAuthBox (LOOP-06): `aether auth status` must not print +// "Authenticated" when the server has just rejected the stored token +// with a 401/403 — that used to be folded into the same silent +// "server unreachable, show what we know locally" catch as a genuine +// network outage. +// 5. cmdAuth (LOOP-06 round 2): renderAuthBox's /models call is the FIRST +// network round-trip either the "status" subcommand or bare `aether +// auth` make, and previously nothing was written to stdout until the +// whole thing resolved — up to DEFAULT_REQUEST_TIMEOUT_MS of silence on +// a slow connection ("the REPL just looks hung", the exact defect class +// PR #47 fixed for slash.ts's catalog fetch). cmdAuth must now print a +// loading line BEFORE awaiting renderAuthBox. import { test } from "node:test"; import assert from "node:assert/strict"; import { rmSync } from "node:fs"; @@ -16,6 +28,10 @@ import { tokenStoreFromEnv, FileTokenStore, StaticTokenStore } from "../src/core import { ApiClient } from "../src/core/transport.js"; import { errorHint, HttpError } from "../src/core/errors.js"; import { hintFor } from "../src/core/error_hints.js"; +import { renderAuthBox, cmdAuth } from "../src/commands/auth.js"; +import type { LoginOpts } from "../src/commands/login.js"; +import { stripAnsi } from "../src/ui/theme.js"; +import type { AppContext } from "../src/core/context.js"; function withTempConfigDir(fn: () => Promise): Promise { const dir = join(tmpdir(), `aether-401-${process.pid}-${Math.random().toString(36).slice(2)}`); @@ -247,3 +263,257 @@ test("EnvOverrideTokenStore: update() (auto-refresh) swaps the active token WITH assert.equal(await store.get(), "sess_embedded_rotated", "active token rotated in-process"); assert.equal(await disk.get(), "disk_standalone_login", "standalone on-disk login untouched"); })); + +// ── 5. renderAuthBox (LOOP-06): 401/403 must not read as "Authenticated" ── + +function fakeCtx(api: ApiClient, tokens: StaticTokenStore): AppContext { + return { + cfg: { baseUrl: "https://api.example" }, + api, + tokens, + flags: { cwd: process.cwd(), json: false, audit: false, yes: false }, + confirm: async () => false, + } as unknown as AppContext; +} + +test("renderAuthBox: a 401 from /models renders a distinct 'Session expired' state, not 'Authenticated'", async () => { + const real = globalThis.fetch; + // An aek_ API key never triggers refresh (see the aek_ test above), so the + // 401 from /models surfaces to renderAuthBox's catch untouched. + const tokens = new StaticTokenStore("aek_deadtoken1234"); + stubFetch(() => jsonRes(401, { detail: "token revoked" }), []); + try { + const api = new ApiClient("https://api.example", tokens); + const panel = stripAnsi(await renderAuthBox(fakeCtx(api, tokens))); + assert.match(panel, /Session expired/); + assert.doesNotMatch(panel, /Authenticated/, "must not claim Authenticated for a rejected token"); + assert.match(panel, /aether auth login/); + } finally { + globalThis.fetch = real; + } +}); + +test("renderAuthBox: a 403 from /models also renders 'Session expired' (not silently 'Authenticated')", async () => { + const real = globalThis.fetch; + const tokens = new StaticTokenStore("aek_deadtoken1234"); + stubFetch(() => jsonRes(403, { detail: "forbidden" }), []); + try { + const api = new ApiClient("https://api.example", tokens); + const panel = stripAnsi(await renderAuthBox(fakeCtx(api, tokens))); + assert.match(panel, /Session expired/); + assert.doesNotMatch(panel, /Authenticated/); + } finally { + globalThis.fetch = real; + } +}); + +test("renderAuthBox: a genuine network outage (no HttpError) still falls back to the silent 'Authenticated' panel", async () => { + const real = globalThis.fetch; + const tokens = new StaticTokenStore("aek_deadtoken1234"); + globalThis.fetch = (async () => { + throw Object.assign(new TypeError("fetch failed"), { cause: { code: "ECONNREFUSED" } }); + }) as typeof globalThis.fetch; + try { + const api = new ApiClient("https://api.example", tokens); + const panel = stripAnsi(await renderAuthBox(fakeCtx(api, tokens))); + assert.match(panel, /Authenticated/, "an unreachable server is not a rejected session"); + assert.doesNotMatch(panel, /Session expired/); + } finally { + globalThis.fetch = real; + } +}); + +test("renderAuthBox: a successful /models call renders the normal 'Authenticated' panel with tier/default", async () => { + const real = globalThis.fetch; + const tokens = new StaticTokenStore("aek_deadtoken1234"); + stubFetch(() => jsonRes(200, { tier: "pro", default: "aether-large" }), []); + try { + const api = new ApiClient("https://api.example", tokens); + const panel = stripAnsi(await renderAuthBox(fakeCtx(api, tokens))); + assert.match(panel, /Authenticated/); + assert.doesNotMatch(panel, /Session expired/); + assert.match(panel, /pro/); + assert.match(panel, /aether-large/); + } finally { + globalThis.fetch = real; + } +}); + +// ── 6. cmdAuth (LOOP-06 round 2): loading feedback before the /models call ── +// +// These intercept the process-wide process.stdout.write, which — unlike the +// renderAuthBox tests above — is a genuinely global stream shared with the +// test runner's own reporter. Matching on a broad /Authenticated/ regex over +// that captured stream is a trap: a SIBLING test's own description text +// ("...renders the normal 'Authenticated' panel...") can be flushed by the +// reporter through that same intercepted stream while this test's capture +// window is open, producing a false match unrelated to cmdAuth's own output. +// PANEL_MARKER is the exact, singular header string renderAuthBox emits +// (auth.ts's `theme.bold("Aether Agent — Authenticated")`) — not a string +// that appears anywhere in a test name — so a match can only come from +// cmdAuth's own finished panel actually having been written. +const PANEL_MARKER = "Aether Agent — Authenticated"; + +function captureStdout(): { writes: string[]; restore: () => void } { + const writes: string[] = []; + const orig = process.stdout.write.bind(process.stdout); + process.stdout.write = ((chunk: unknown) => { + writes.push(String(chunk)); + return true; + }) as typeof process.stdout.write; + return { + writes, + restore: () => { + process.stdout.write = orig; + }, + }; +} + +test("cmdAuth 'status': prints a loading line BEFORE the /models call resolves — no silent hang", async () => { + const real = globalThis.fetch; + const tokens = new StaticTokenStore("aek_deadtoken1234"); + let resolveFetch: (r: Response) => void = () => {}; + const pending = new Promise((res) => { + resolveFetch = res; + }); + // Simulate a stalled/slow connection: fetch never resolves until we say so. + globalThis.fetch = (async () => pending) as typeof globalThis.fetch; + const cap = captureStdout(); + try { + const api = new ApiClient("https://api.example", tokens); + const ctx = fakeCtx(api, tokens); + const done = cmdAuth(ctx, ["status"], {} as LoginOpts); + // Let queued microtasks (the write before the await) run while the + // network call is still deliberately left hanging. + await new Promise((r) => setImmediate(r)); + assert.ok( + cap.writes.some((w) => /checking session/i.test(w)), + "a loading line must be written before the network call resolves", + ); + assert.ok( + !cap.writes.some((w) => w.includes(PANEL_MARKER)), + "the finished panel must NOT have printed yet — /models is still pending", + ); + resolveFetch(jsonRes(200, { tier: "pro", default: "aether-large" })); + const code = await done; + assert.equal(code, 0); + assert.ok(cap.writes.some((w) => w.includes(PANEL_MARKER)), "the panel prints once /models resolves"); + } finally { + globalThis.fetch = real; + cap.restore(); + } +}); + +test("cmdAuth bare `aether auth` (no subcommand): same loading line before the /models call resolves", async () => { + const real = globalThis.fetch; + const tokens = new StaticTokenStore("aek_deadtoken1234"); + let resolveFetch: (r: Response) => void = () => {}; + const pending = new Promise((res) => { + resolveFetch = res; + }); + globalThis.fetch = (async () => pending) as typeof globalThis.fetch; + const cap = captureStdout(); + try { + const api = new ApiClient("https://api.example", tokens); + const ctx = fakeCtx(api, tokens); + const done = cmdAuth(ctx, [], {} as LoginOpts); + await new Promise((r) => setImmediate(r)); + assert.ok( + cap.writes.some((w) => /checking session/i.test(w)), + "bare `aether auth` must also show loading feedback before /models resolves", + ); + assert.ok(!cap.writes.some((w) => w.includes(PANEL_MARKER))); + resolveFetch(jsonRes(200, { tier: "pro", default: "aether-large" })); + const code = await done; + assert.equal(code, 0); + assert.ok(cap.writes.some((w) => w.includes(PANEL_MARKER))); + } finally { + globalThis.fetch = real; + cap.restore(); + } +}); + +// ── 7. authRefresh (LOOP-06 round 3): the catch block must go through the +// shared formatErrorLine/errorHint convention — same as chat.ts's printError +// — instead of a hand-built, unstyled `✗ ` template string with no +// hint and no /doctor pointer. ── + +function captureStderr(): { writes: string[]; restore: () => void } { + const writes: string[] = []; + const orig = process.stderr.write.bind(process.stderr); + process.stderr.write = ((chunk: unknown) => { + writes.push(String(chunk)); + return true; + }) as typeof process.stderr.write; + return { + writes, + restore: () => { + process.stderr.write = orig; + }, + }; +} + +test("cmdAuth 'refresh': a network failure prints the shared formatErrorLine glyph + a /doctor hint", async () => { + const real = globalThis.fetch; + // A session token (not aek_-prefixed) so authRefresh actually attempts the + // POST /auth/refresh instead of short-circuiting on "API tokens don't expire". + const tokens = new StaticTokenStore("sess_expiring"); + globalThis.fetch = (async () => { + throw Object.assign(new TypeError("fetch failed"), { cause: { code: "ECONNREFUSED" } }); + }) as typeof globalThis.fetch; + const cap = captureStderr(); + try { + const api = new ApiClient("https://api.example", tokens); + const ctx = fakeCtx(api, tokens); + const code = await cmdAuth(ctx, ["refresh"], {} as LoginOpts); + assert.equal(code, 1); + const out = cap.writes.join(""); + assert.match(out, /✗/, "must use formatErrorLine's glyph, not a bare template string"); + assert.match(out, /\/doctor/, "a network failure must surface errorHint's /doctor pointer"); + assert.match(out, /\n\n$/, "formatErrorLine's trailing blank-line separator must be present"); + } finally { + globalThis.fetch = real; + cap.restore(); + } +}); + +test("cmdAuth 'refresh': a server-rejected refresh (HttpError) still prints the shared glyph + the re-login hint", async () => { + const real = globalThis.fetch; + const tokens = new StaticTokenStore("sess_expiring"); + globalThis.fetch = (async () => jsonRes(401, { detail: "refresh token revoked" })) as typeof globalThis.fetch; + const cap = captureStderr(); + try { + const api = new ApiClient("https://api.example", tokens); + const ctx = fakeCtx(api, tokens); + const code = await cmdAuth(ctx, ["refresh"], {} as LoginOpts); + assert.equal(code, 1); + const out = cap.writes.join(""); + assert.match(out, /✗/, "must use formatErrorLine's glyph, not a bare template string"); + assert.match(out, /refresh token revoked/, "the server's detail must still surface in the message"); + assert.match(out, /aether auth login/, "a 401 must surface errorHint's re-login pointer"); + } finally { + globalThis.fetch = real; + cap.restore(); + } +}); + +test("cmdAuth 'refresh': a 200 with no session_token also prints the shared formatErrorLine glyph (not the one bare ✗ line left in authRefresh)", async () => { + const real = globalThis.fetch; + const tokens = new StaticTokenStore("sess_expiring"); + globalThis.fetch = (async () => jsonRes(200, {})) as typeof globalThis.fetch; + const cap = captureStderr(); + try { + const api = new ApiClient("https://api.example", tokens); + const ctx = fakeCtx(api, tokens); + const code = await cmdAuth(ctx, ["refresh"], {} as LoginOpts); + assert.equal(code, 1); + const out = cap.writes.join(""); + assert.match(out, /✗/, "must use formatErrorLine's glyph, not a bare template string"); + assert.match(out, /Refresh failed/); + assert.match(out, /aether auth login/, "must still point at re-login even with no err object to derive a hint from"); + assert.match(out, /\n\n$/, "formatErrorLine's trailing blank-line separator must be present"); + } finally { + globalThis.fetch = real; + cap.restore(); + } +}); diff --git a/test/brain_cloud.test.ts b/test/brain_cloud.test.ts index 5d797db..ed769e0 100644 --- a/test/brain_cloud.test.ts +++ b/test/brain_cloud.test.ts @@ -58,6 +58,17 @@ test("a clean stream still ends done ok:true", async () => { assert.ok(done && done.type === "done" && done.ok === true); }); +// LOOP-06 round 3: the sibling gap to chat.ts's runCloudTurn — a stream that +// ends after only `delta` frames (no `done`, no `error`) must not be +// fabricated into a successful run either. +test("a stream that ends with only delta frames (no done/error) ends done ok:false, not fabricated success", async () => { + const events = await runCloud([JSON.stringify({ type: "delta", text: "partial" })]); + const done = events.find((e) => e.type === "done"); + assert.ok(done && done.type === "done"); + assert.equal(done.ok, false); + assert.match(done.result, /connection ended|before the server finished/i); +}); + // Finding E's Tier-2/3 metrics (docs/specs/2026-07-10-workflow-viewer-agent-panel-design.md) // must survive the REAL cloud path (SSE -> stream.ts's normalizeFrame -> here), // not just brain_protocol.ts's separate NDJSON decoder — that decoder is never diff --git a/test/chat.test.ts b/test/chat.test.ts index d37af4d..9f2cb19 100644 --- a/test/chat.test.ts +++ b/test/chat.test.ts @@ -3,6 +3,7 @@ import assert from "node:assert/strict"; import { runTurn, ChatTurnError, applyRestart, buildPromptContext, repaintString } from "../src/commands/chat.js"; import { handleSlash } from "../src/commands/slash.js"; import { ApiClient } from "../src/core/transport.js"; +import { StreamIncompleteError } from "../src/core/errors.js"; import type { GlobalFlags, AppContext } from "../src/core/context.js"; import type { TokenStore } from "../src/core/auth.js"; @@ -71,6 +72,61 @@ test("runTurn resolves cleanly on a clean stream", async () => { } }); +// LOOP-06 round 3: a stream that ends after only `delta` frames (no `done`, +// no `error` — e.g. a proxy/load-balancer time-boxing the SSE response and +// closing the socket within the idle window) must NOT be treated as a +// successful turn. decodeSse's for-await loop exits normally on a plain +// end-of-stream, so without an explicit terminal-frame check runTurn used to +// resolve as if the turn had completed cleanly. +test("runTurn throws StreamIncompleteError when the stream ends with only delta frames (no done/error)", async () => { + const real = globalThis.fetch; + globalThis.fetch = sseFetch([JSON.stringify({ type: "delta", text: "partial" })]); + try { + await assert.rejects(() => runTurn(ctxWith(), "hi"), StreamIncompleteError); + } finally { + globalThis.fetch = real; + } +}); + +// LOOP-01/LOOP-06 round-1 regression: request()'s new bounded-by-default +// timeout (AETHER_REQUEST_TIMEOUT_MS, 30s) must NOT apply to the +// non-streaming CHAT_PATH fallback — a full LLM turn can legitimately run +// long, so chat.ts opts that call into stream()'s own generous bound instead. +test("runTurn's non-streaming fallback survives a response slower than the metadata-call timeout default", async () => { + const real = globalThis.fetch; + const ENV_KEY = "AETHER_REQUEST_TIMEOUT_MS"; + const prevEnv = process.env[ENV_KEY]; + process.env[ENV_KEY] = "5"; // far shorter than the fallback response's own delay below + let call = 0; + globalThis.fetch = (async () => { + call++; + if (call === 1) { + // Streaming leg: server signals fail-soft `{"stream": false}`. + return { + ok: true, + status: 200, + headers: new Headers({ "content-type": "application/json" }), + json: async () => ({ stream: false }), + } as unknown as Response; + } + // Fallback leg: deliberately slower than the 5ms metadata-call default — + // this only survives because chat.ts opts into defaultStreamTimeoutMs(). + await new Promise((r) => setTimeout(r, 20)); + return new Response(JSON.stringify({ response: "ok" }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }) as typeof globalThis.fetch; + try { + await runTurn(ctxWith(), "hi"); // must not throw/time out + assert.equal(call, 2, "both the streaming leg and the fallback leg were called"); + } finally { + globalThis.fetch = real; + if (prevEnv === undefined) delete process.env[ENV_KEY]; + else process.env[ENV_KEY] = prevEnv; + } +}); + test("applyRestart sets the new model and clears the agent", () => { const flags = { model: "haiku", agent: "neo", json: false, audit: false, yes: false, cwd: "." } as GlobalFlags; applyRestart(flags, { model: "opus" }); diff --git a/test/client.test.ts b/test/client.test.ts index 78e76d7..99fa324 100644 --- a/test/client.test.ts +++ b/test/client.test.ts @@ -1,6 +1,10 @@ import { test } from "node:test"; import assert from "node:assert/strict"; +import { rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; import { createClient, AetherClient } from "../src/index.js"; +import { FileTokenStore } from "../src/core/auth.js"; test("createClient honors explicit baseUrl + token", () => { const c = createClient({ baseUrl: "https://api.example", token: "tok" }); @@ -23,3 +27,57 @@ test("exposes raw http on the same route", () => { const c = createClient({ baseUrl: "https://x", token: "t" }); assert.ok(c.http); }); + +// ── LOOP-01 round 1: AetherClient's injected-token TokenStore selection must +// share tokenStoreFromEnv's decision (via tokenStoreForInjected) for READS, +// but deliberately keep WRITES in-process — an embedded library client's +// login() must not clobber the standalone CLI's on-disk session. This is the +// mirror of auth_401.test.ts's "tokenStoreFromEnv: set() persists a fresh +// login to disk" test: same shape, opposite (in-process-only) expectation. +function withTempConfigDir(fn: () => Promise): Promise { + const dir = join(tmpdir(), `aether-client-${process.pid}-${Math.random().toString(36).slice(2)}`); + const prev = process.env["AETHER_CONFIG_DIR"]; + process.env["AETHER_CONFIG_DIR"] = dir; + return fn().finally(() => { + if (prev === undefined) delete process.env["AETHER_CONFIG_DIR"]; + else process.env["AETHER_CONFIG_DIR"] = prev; + rmSync(dir, { recursive: true, force: true }); + }); +} + +test("createClient({token}).login() persists the fresh token in-process only — does NOT clobber the CLI's on-disk session", () => + withTempConfigDir(async () => { + const real = globalThis.fetch; + globalThis.fetch = (async (url: unknown, init?: RequestInit) => { + const u = String(url); + if (u.endsWith("/auth/login")) { + return new Response(JSON.stringify({ authenticated: true, session_token: "sess_fresh_embed" }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + if (u.endsWith("/models")) { + const auth = (init?.headers as Record | undefined)?.["Authorization"] ?? ""; + return new Response(JSON.stringify({ authHeader: auth }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + return new Response("{}", { status: 200, headers: { "content-type": "application/json" } }); + }) as typeof globalThis.fetch; + try { + const c = createClient({ baseUrl: "https://api.example", token: "stale_embedded_tok" }); + await c.login("user", "pass"); + // The fresh token IS active for the next authed call on this same client… + const out = await c.http.getJson<{ authHeader: string }>("/models"); + assert.equal(out.authHeader, "Bearer sess_fresh_embed"); + // …but it must never have reached the CLI's on-disk token store. + assert.equal( + await new FileTokenStore().get(), + null, + "an embedded AetherClient login must not clobber the standalone CLI's on-disk session", + ); + } finally { + globalThis.fetch = real; + } + })); diff --git a/test/device.test.ts b/test/device.test.ts index 77de747..5005cf9 100644 --- a/test/device.test.ts +++ b/test/device.test.ts @@ -1,6 +1,20 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { classifyPoll } from "../src/core/device.js"; +import { classifyPoll, pollForToken, type DeviceCode } from "../src/core/device.js"; +import { ApiClient } from "../src/core/transport.js"; +import { StaticTokenStore } from "../src/core/auth.js"; + +const REQUEST_TIMEOUT_ENV_KEY = "AETHER_REQUEST_TIMEOUT_MS"; + +function setRequestTimeoutMs(value: string | undefined): () => void { + const original = process.env[REQUEST_TIMEOUT_ENV_KEY]; + if (value === undefined) delete process.env[REQUEST_TIMEOUT_ENV_KEY]; + else process.env[REQUEST_TIMEOUT_ENV_KEY] = value; + return () => { + if (original === undefined) delete process.env[REQUEST_TIMEOUT_ENV_KEY]; + else process.env[REQUEST_TIMEOUT_ENV_KEY] = original; + }; +} test("classifyPoll maps server responses to poll actions", () => { assert.equal(classifyPoll({ error: "authorization_pending" }), "wait"); @@ -10,3 +24,227 @@ test("classifyPoll maps server responses to poll actions", () => { assert.equal(classifyPoll({ access_token: "aek_x" }), "ready"); assert.equal(classifyPoll({}), "wait"); }); + +// ── pollForToken: LOOP-01 / LOOP-06 regression ─────────────────────────── +// A raw network failure (fetch throwing — server down, DNS failure, timeout) +// must NOT be silently reclassified as `authorization_pending`. Only an +// HttpError carrying a real body from the token endpoint (the documented 400 +// pending/slow_down/expired case) may map to ordinary polling state. + +function jsonRes(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); +} + +/** Fake clock driven entirely through the injected `sleep` — pollForToken's + * loop checks `Date.now() < deadline` against real wall-clock otherwise, + * which would hang the test for the full (realistic, multi-minute) expires_in + * window instead of resolving instantly. */ +function fakeClock(): { sleep: (ms: number) => Promise; restore: () => void } { + const real = Date.now; + let clock = 0; + Date.now = () => clock; + return { + sleep: async (ms: number) => { + clock += ms; + }, + restore: () => { + Date.now = real; + }, + }; +} + +/** Stubs global fetch to play back one Response-or-throw "act" per call, + * repeating the last act once the sequence is exhausted. */ +function stubFetchSequence(acts: Array<() => Response>): { calls: () => number; restore: () => void } { + const real = globalThis.fetch; + let n = 0; + globalThis.fetch = (async () => { + const act = acts[Math.min(n, acts.length - 1)] as () => Response; + n++; + return act(); + }) as typeof globalThis.fetch; + return { + calls: () => n, + restore: () => { + globalThis.fetch = real; + }, + }; +} + +function captureStderr(): { text: () => string; restore: () => void } { + const orig = process.stderr.write.bind(process.stderr); + let out = ""; + process.stderr.write = ((s: string) => ((out += s), true)) as typeof process.stderr.write; + return { + text: () => out, + restore: () => { + process.stderr.write = orig; + }, + }; +} + +const CODE: DeviceCode = { + device_code: "dc_1", + user_code: "ABCD-1234", + verification_uri: "https://aethersystems.net/platform/device", + verification_uri_complete: "https://aethersystems.net/platform/device?u=dc_1", + interval: 1, + expires_in: 10, // with interval=1s -> 10 deterministic poll attempts via the fake clock +}; + +test("pollForToken: a transient network blip does not warn and does not block success", async () => { + const netErr = () => { + throw new TypeError("fetch failed", { cause: { code: "ECONNREFUSED" } }); + }; + const success = () => jsonRes(200, { access_token: "aek_success" }); + const fetchStub = stubFetchSequence([netErr, netErr, success]); + const clock = fakeClock(); + const stderr = captureStderr(); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_key")); + const token = await pollForToken(api, CODE, clock.sleep); + assert.equal(token, "aek_success"); + assert.equal(stderr.text(), "", "two blips (below the 3-in-a-row threshold) must not print a warning"); + } finally { + fetchStub.restore(); + clock.restore(); + stderr.restore(); + } +}); + +test("pollForToken: a sustained network outage fails fast with a distinct message, not silent authorization_pending", async () => { + const netErr = () => { + throw new TypeError("fetch failed", { cause: { code: "ECONNREFUSED" } }); + }; + const fetchStub = stubFetchSequence([netErr]); + const clock = fakeClock(); + const stderr = captureStderr(); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_key")); + await assert.rejects( + () => pollForToken(api, CODE, clock.sleep), + (e: unknown) => { + assert.match((e as Error).message, /couldn't reach the server/); + return true; + }, + ); + assert.match(stderr.text(), /can't reach the server/, "a sustained outage must surface a distinct warning, not silent waiting"); + // All 10 attempts (interval=1, expires_in=10) hit the network branch. + assert.equal(fetchStub.calls(), 10); + } finally { + fetchStub.restore(); + clock.restore(); + stderr.restore(); + } +}); + +test("pollForToken: real authorization_pending (HttpError with body) times out with the generic message and no warning", async () => { + const pending = () => jsonRes(400, { error: "authorization_pending" }); + const fetchStub = stubFetchSequence([pending]); + const clock = fakeClock(); + const stderr = captureStderr(); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_key")); + await assert.rejects( + () => pollForToken(api, CODE, clock.sleep), + (e: unknown) => { + assert.equal((e as Error).message, "login timed out — run `aether auth login` again"); + return true; + }, + ); + assert.equal(stderr.text(), "", "ordinary authorization_pending polling must never print the outage warning"); + } finally { + fetchStub.restore(); + clock.restore(); + stderr.restore(); + } +}); + +test("pollForToken: a repeating 5xx with a parseable JSON object body (e.g. FastAPI's {detail}) is NOT folded into authorization_pending — it counts as a network error and warns/fails distinctly", async () => { + const serverError = () => jsonRes(500, { detail: "internal error" }); + const fetchStub = stubFetchSequence([serverError]); + const clock = fakeClock(); + const stderr = captureStderr(); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_key")); + await assert.rejects( + () => pollForToken(api, CODE, clock.sleep), + (e: unknown) => { + assert.match( + (e as Error).message, + /couldn't reach the server/, + "a sustained 500 outage must surface the distinct network-outage message, not the generic 'run login again'", + ); + return true; + }, + ); + assert.match( + stderr.text(), + /can't reach the server/, + "a repeating 500 must trip the same outage warning as a raw network failure, not read as silent authorization_pending", + ); + // All 10 attempts (interval=1, expires_in=10) hit the network-error branch. + assert.equal(fetchStub.calls(), 10); + } finally { + fetchStub.restore(); + clock.restore(); + stderr.restore(); + } +}); + +test("pollForToken: a 400 with a non-string `error` field (malformed pending shape) is NOT treated as ordinary polling state", async () => { + const malformed = () => jsonRes(400, { error: 123 }); + const fetchStub = stubFetchSequence([malformed]); + const clock = fakeClock(); + const stderr = captureStderr(); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_key")); + await assert.rejects( + () => pollForToken(api, CODE, clock.sleep), + (e: unknown) => { + assert.match((e as Error).message, /couldn't reach the server/); + return true; + }, + ); + assert.match(stderr.text(), /can't reach the server/); + } finally { + fetchStub.restore(); + clock.restore(); + stderr.restore(); + } +}); + +// ── LOOP-01 round-1 regression: a single STALLED (never-settling) poll ── +// request must not sit past its own deadline. Before ApiClient.request() had +// a default timeout, `while (Date.now() < deadline)`'s re-check could never +// re-execute until a hung fetch settled — which, for a silently-dropped +// connection, was never. Now request() itself gives up after +// AETHER_REQUEST_TIMEOUT_MS and pollForToken treats that like any other +// network failure and moves on to its next attempt. +test("pollForToken: a single stalled (hanging) poll request times out instead of blocking the loop forever", async () => { + const restoreEnv = setRequestTimeoutMs("5"); + const real = globalThis.fetch; + let calls = 0; + globalThis.fetch = (async () => { + calls++; + if (calls === 1) return new Promise(() => {}); // the exact "stalled" scenario + return jsonRes(200, { access_token: "aek_after_stall" }); + }) as typeof globalThis.fetch; + const stderr = captureStderr(); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_key")); + // Real deadline (60s out) + a no-op injected sleep: only the ApiClient + // request-level timeout (real setTimeout, 5ms) governs how long this + // test actually takes, decoupled from the fake-clock polling interval. + const token = await pollForToken(api, { ...CODE, expires_in: 60 }, async () => {}); + assert.equal(token, "aek_after_stall"); + assert.equal(calls, 2, "the stalled first attempt timed out and the loop moved on to a second attempt"); + } finally { + globalThis.fetch = real; + restoreEnv(); + stderr.restore(); + } +}); diff --git a/test/embedded-auth.test.ts b/test/embedded-auth.test.ts index 29a9fb5..d03a765 100644 --- a/test/embedded-auth.test.ts +++ b/test/embedded-auth.test.ts @@ -7,7 +7,9 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { tokenStoreFromEnv, + tokenStoreForInjected, EnvOverrideTokenStore, + StaticTokenStore, FileTokenStore, } from "../src/core/auth.js"; @@ -33,3 +35,37 @@ test("the injected token is trimmed before use", async () => { const store = tokenStoreFromEnv({ AETHER_TOKEN: " sess-trim " } as NodeJS.ProcessEnv); assert.equal(await store.get(), "sess-trim"); }); + +// ── tokenStoreForInjected (LOOP-01 round 1): the one shared decision behind +// both tokenStoreFromEnv (CLI entry) and AetherClient's constructor (library +// embed) — pinned directly so the two call sites can't silently diverge again. + +test("tokenStoreForInjected: persistOnLogin=true (CLI-entry semantics) selects EnvOverrideTokenStore", async () => { + const store = tokenStoreForInjected("sess-x", { persistOnLogin: true }); + assert.ok(store instanceof EnvOverrideTokenStore); + assert.equal(await store.get(), "sess-x"); +}); + +test("tokenStoreForInjected: persistOnLogin=false (AetherClient library-embed semantics) selects StaticTokenStore", async () => { + const store = tokenStoreForInjected("sess-x", { persistOnLogin: false }); + assert.ok(store instanceof StaticTokenStore); + assert.equal(await store.get(), "sess-x"); +}); + +test("tokenStoreForInjected: no token (either persistOnLogin setting) falls back to the file store", () => { + assert.ok(tokenStoreForInjected(undefined, { persistOnLogin: true }) instanceof FileTokenStore); + assert.ok(tokenStoreForInjected("", { persistOnLogin: false }) instanceof FileTokenStore); + assert.ok(tokenStoreForInjected(" ", { persistOnLogin: false }) instanceof FileTokenStore); +}); + +test("tokenStoreForInjected: trims the token before use, same as tokenStoreFromEnv", async () => { + const store = tokenStoreForInjected(" sess-trim ", { persistOnLogin: false }); + assert.equal(await store.get(), "sess-trim"); +}); + +test("tokenStoreFromEnv delegates to tokenStoreForInjected with persistOnLogin: true", async () => { + const viaEnv = tokenStoreFromEnv({ AETHER_TOKEN: "sess-parity" } as NodeJS.ProcessEnv); + const viaHelper = tokenStoreForInjected("sess-parity", { persistOnLogin: true }); + assert.equal(viaEnv.constructor, viaHelper.constructor); + assert.equal(await viaEnv.get(), await viaHelper.get()); +}); diff --git a/test/error_hints.test.ts b/test/error_hints.test.ts index 16c12b5..e2ad6a7 100644 --- a/test/error_hints.test.ts +++ b/test/error_hints.test.ts @@ -1,21 +1,43 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { hintFor, isAbortError } from "../src/core/error_hints.js"; -import { HttpError, InsecureTransportError, StreamTimeoutError } from "../src/core/errors.js"; +import { HttpError, InsecureTransportError, StreamIncompleteError, StreamTimeoutError } from "../src/core/errors.js"; test("HTTP statuses map to actionable hints", () => { assert.match(hintFor(new HttpError(401, "HTTP 401"))!, /aether auth login/); assert.match(hintFor(new HttpError(403, "HTTP 403"))!, /\/tier/); assert.match(hintFor(new HttpError(429, "HTTP 429"))!, /rate limited/i); + // 5xx wording is intentionally NOT shared with errors.errorHint's >=500 + // branch (that one names the baseUrl; hintFor has none to name) — only + // 401/402/403/429 are unified via httpStatusHint (LOOP-06 round 1). assert.match(hintFor(new HttpError(503, "HTTP 503"))!, /\/doctor/); assert.equal(hintFor(new HttpError(404, "HTTP 404")), null); }); test("network failures point at connectivity", () => { + // Also intentionally distinct wording from errors.errorHint's network + // branch, same reasoning as the 5xx case above (LOOP-06 round 1). assert.match(hintFor(new TypeError("fetch failed"))!, /aether api/i); assert.match(hintFor(new Error("connect ECONNREFUSED 1.2.3.4:443"))!, /network/); }); +// LOOP-06 round 3: undici puts the failure code on err.cause.code, not in +// the message text, for real fetch failures — errors.errorHint already +// checked this via NETWORK_CODES; hintFor only pattern-matched the message +// and silently returned null for this entire shape. Mirrors +// errors.test.ts's "network failures hint at /doctor with the base url". +test("network failures with the code on err.cause (undici shape) are still detected", () => { + const withCause = new Error("request to host failed"); + (withCause as { cause?: unknown }).cause = { code: "ECONNREFUSED" }; + assert.match(hintFor(withCause)!, /aether api/i); + + // The code undici throws for a body/socket death after headers were + // already received — i.e. a mid-stream drop, not a connect-time failure. + const midStreamDrop = new Error("terminated"); + (midStreamDrop as { cause?: unknown }).cause = { code: "UND_ERR_SOCKET" }; + assert.match(hintFor(midStreamDrop)!, /aether api/i); +}); + test("insecure transport points at the base URL", () => { assert.match(hintFor(new InsecureTransportError("http://evil"))!, /https/); }); @@ -25,6 +47,13 @@ test("stream timeouts point at connectivity", () => { assert.match(hintFor(new StreamTimeoutError(120_000))!, /\/doctor/); }); +// LOOP-06 round 3: a stream that ends with no terminal done/error frame gets +// the same retry/doctor hint, mirroring errors.errorHint. +test("a stream ending without a terminal frame points at connectivity", () => { + assert.match(hintFor(new StreamIncompleteError())!, /retry/); + assert.match(hintFor(new StreamIncompleteError())!, /\/doctor/); +}); + test("aborts are silent (already user-initiated) and detected", () => { const abort = new Error("aborted"); abort.name = "AbortError"; @@ -34,6 +63,19 @@ test("aborts are silent (already user-initiated) and detected", () => { assert.equal(isAbortError("nope"), false); }); +test("isAbortError re-exports errors.isAbortError's fuller shapes (LOOP-06 round 1)", () => { + // Regression for the dead, narrower copy this module used to maintain + // (only checked err.name === "AbortError"): it returned false for both + // shapes below. Now that error_hints.isAbortError re-exports + // errors.isAbortError instead of duplicating the check, it catches them. + const causeWrapped = new TypeError("The operation was aborted"); + (causeWrapped as { cause?: unknown }).cause = { name: "AbortError" }; + assert.equal(isAbortError(causeWrapped), true); + + const messageOnly = new Error("the operation was aborted due to a signal"); + assert.equal(isAbortError(messageOnly), true); +}); + test("unknown errors produce no hint", () => { assert.equal(hintFor(new Error("whatever")), null); assert.equal(hintFor(42), null); diff --git a/test/error_line.test.ts b/test/error_line.test.ts new file mode 100644 index 0000000..46fd052 --- /dev/null +++ b/test/error_line.test.ts @@ -0,0 +1,135 @@ +// test/error_line.test.ts — LOOP-06 regression: chat.ts's printError +// (client-caught errors) and render.ts's Renderer.error (server-streamed +// `error` SSE frames) must render the SAME "✗ " convention — glyph, dim +// " ⤷ hint" line, and trailing blank-line separator — instead of the two +// paths disagreeing on the exact same "session expired" scenario depending +// on which one caught it. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { Writable } from "node:stream"; +import { formatErrorLine } from "../src/ui/error_line.js"; +import { Renderer } from "../src/core/render.js"; +import { cmdChat } from "../src/commands/chat.js"; +import { ApiClient } from "../src/core/transport.js"; +import { stripAnsi } from "../src/ui/theme.js"; +import { errorHint, HttpError } from "../src/core/errors.js"; +import type { AppContext } from "../src/core/context.js"; +import type { TokenStore } from "../src/core/auth.js"; + +function collect(): { w: Writable; text: () => string } { + const chunks: string[] = []; + const w = new Writable({ + write(chunk, _enc, cb) { + chunks.push(String(chunk)); + cb(); + }, + }); + return { w, text: () => chunks.join("") }; +} + +// ── 1. formatErrorLine — the shared primitive ──────────────────────────── + +test("formatErrorLine: bare message gets a leading blank line, the glyph, and a trailing blank-line separator", () => { + const line = stripAnsi(formatErrorLine("boom")); + assert.equal(line, "\n✗ boom\n\n"); +}); + +test("formatErrorLine: a hint renders as its own dim ' ⤷ ' line before the trailing separator", () => { + const line = stripAnsi(formatErrorLine("session expired", { hint: "run `aether auth login`" })); + assert.equal(line, "\n✗ session expired\n ⤷ run `aether auth login`\n\n"); +}); + +test("formatErrorLine: errorCode and refId append onto the SAME head line as chat.ts's printError alone never did", () => { + const line = stripAnsi(formatErrorLine("bad thing", { errorCode: "E42", refId: "r1" })); + assert.equal(line, "\n✗ bad thing [E42] (ref r1)\n\n"); +}); + +test("formatErrorLine: a null hint (errorHint's own return type) is treated as absent, not printed", () => { + const line = stripAnsi(formatErrorLine("boom", { hint: null })); + assert.equal(line, "\n✗ boom\n\n"); +}); + +test("formatErrorLine: msg/hint/errorCode/refId are all sanitized — no escape bytes survive from any field", () => { + const line = formatErrorLine("bad\x1b]52;c;ZXZpbA==\x07thing", { + hint: "hint\x1b[31m", + errorCode: "E\x1b[2J1", + refId: "r\x1b[8m1", + }); + assert.ok(!line.includes("\x1b"), `escape bytes leaked into:\n${JSON.stringify(line)}`); + assert.ok(stripAnsi(line).includes("badthing")); +}); + +// ── 2. Renderer.error (server-streamed frame) now uses the shared format ── + +test("Renderer.error: a streamed error frame now gets the SAME trailing blank-line separator printError has always had", () => { + const out = collect(); + const err = collect(); + const r = new Renderer({ json: false, audit: false, out: out.w, err: err.w }); + r.frame({ type: "error", msg: "rate limited" }); + assert.equal(stripAnsi(err.text()), "\n✗ rate limited\n\n"); +}); + +test("Renderer.error: errorCode/refId still render on the head line under the shared formatter", () => { + const out = collect(); + const err = collect(); + const r = new Renderer({ json: false, audit: false, out: out.w, err: err.w }); + r.frame({ type: "error", msg: "session expired", errorCode: "auth_expired", refId: "req_1" }); + assert.equal(stripAnsi(err.text()), "\n✗ session expired [auth_expired] (ref req_1)\n\n"); +}); + +// ── 3. printError (client-caught error) — same shape, via cmdChat ──────── + +function ctxWithAek(): AppContext { + const tokens = { get: async () => "aek_deadtoken" } as unknown as TokenStore; + return { + cfg: { baseUrl: "https://stub.test", defaultModel: "", backend: "cloud" }, + flags: { json: false, audit: false, yes: false, cwd: "." }, + tokens, + api: new ApiClient("https://stub.test", tokens), + } as unknown as AppContext; +} + +test("cmdChat/printError: a 401 (aek_ key, no refresh) renders the exact shared 'session expired' line", async () => { + const real = globalThis.fetch; + const origStderrWrite = process.stderr.write.bind(process.stderr); + let stderrBytes = ""; + process.stderr.write = ((chunk: unknown): boolean => { + stderrBytes += String(chunk); + return true; + }) as typeof process.stderr.write; + globalThis.fetch = (async () => + new Response(JSON.stringify({ detail: "token revoked" }), { + status: 401, + headers: { "content-type": "application/json" }, + })) as typeof globalThis.fetch; + try { + const ctx = ctxWithAek(); + const code = await cmdChat(ctx, "hi"); + assert.equal(code, 1); + const expectedMsg = "HTTP 401: token revoked"; // HttpError surfaces the server's detail text + const expectedHint = errorHint(new HttpError(401, expectedMsg), ctx.cfg.baseUrl); + assert.equal(stripAnsi(stderrBytes), stripAnsi(formatErrorLine(expectedMsg, { hint: expectedHint }))); + assert.match(stripAnsi(stderrBytes), /aether auth login/, "the same session-expired hint chat.ts has always shown"); + } finally { + globalThis.fetch = real; + process.stderr.write = origStderrWrite; + } +}); + +// ── 4. Cross-path parity — the actual LOOP-06 defect ───────────────────── + +test("LOOP-06 parity: the identical 'session expired' message renders byte-identical head+separator whether printError or Renderer.error catches it", () => { + const out = collect(); + const err = collect(); + const r = new Renderer({ json: false, audit: false, out: out.w, err: err.w }); + r.frame({ type: "error", msg: "session expired or invalid — run `aether auth login` to sign in again" }); + const fromRenderer = stripAnsi(err.text()); + + const fromPrintErrorEquivalent = stripAnsi( + formatErrorLine("HTTP 401", { hint: "session expired or invalid — run `aether auth login` to sign in again" }), + ); + // Both must end in the SAME "\n\n" separator, and neither fuses a hint or + // message onto the line that follows — the exact parity LOOP-06 was about. + assert.ok(fromRenderer.endsWith("\n\n"), "Renderer.error must end with the shared blank-line separator"); + assert.ok(fromPrintErrorEquivalent.endsWith("\n\n"), "printError must end with the shared blank-line separator"); +}); diff --git a/test/errors.test.ts b/test/errors.test.ts index 0628a54..fae7d8c 100644 --- a/test/errors.test.ts +++ b/test/errors.test.ts @@ -1,6 +1,6 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { HttpError, errorHint } from "../src/core/errors.js"; +import { HttpError, StreamIncompleteError, StreamTimeoutError, errorHint } from "../src/core/errors.js"; const BASE = "https://api.aethersystems.net"; @@ -31,6 +31,28 @@ test("network failures hint at /doctor with the base url", () => { assert.match(h, /offline\?/); }); +test("stream timeouts get a retry/doctor hint, matching error_hints.hintFor (LOOP-06 round 2)", () => { + // Regression for LOOP-06 round 2: errorHint used to have no branch for + // StreamTimeoutError, so it fell through to the generic Error branch (no + // cause.code, no "fetch failed" in the message) and returned null — the + // REPL's printError showed a bare "stream timed out..." with zero + // recovery hint, even though the sibling hintFor() handled it correctly. + const h = errorHint(new StreamTimeoutError(120_000), BASE); + assert.notEqual(h, null); + assert.match(h ?? "", /stream went quiet/); + assert.match(h ?? "", /\/doctor/); +}); + +// LOOP-06 round 3: a stream that ends without ever sending a terminal +// done/error frame must get the same retry/doctor hint as any other +// unfinished-connectivity failure, matching error_hints.hintFor. +test("a stream ending without a terminal frame gets a retry/doctor hint", () => { + const h = errorHint(new StreamIncompleteError(), BASE); + assert.notEqual(h, null); + assert.match(h ?? "", /retry/); + assert.match(h ?? "", /\/doctor/); +}); + test("errors with nothing better to say get no hint", () => { assert.equal(errorHint(new HttpError(404, "HTTP 404"), BASE), null); assert.equal(errorHint(new Error("some app error"), BASE), null); diff --git a/test/login.test.ts b/test/login.test.ts new file mode 100644 index 0000000..e3ea487 --- /dev/null +++ b/test/login.test.ts @@ -0,0 +1,194 @@ +// LOOP-06 round 2: end-to-end regression for cmdLogin's headless +// `--username/--password` path. loginWithPassword's thrown Error is written +// straight to process.stderr with NO sanitization of its own (see +// commands/login.ts's `catch (err) { process.stderr.write(\`✗ ${err.message}\n\`) }`) +// — so a malicious/misconfigured server's `reason` field must already be +// sanitized and length-capped by the time it reaches loginWithPassword's +// thrown message (fixed in core/auth.ts via transport.ts's +// sanitizeServerText()). This test exercises the FULL path, not just the +// auth.ts unit, to confirm the raw control bytes never reach stderr. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { cmdLogin } from "../src/commands/login.js"; +import { StaticTokenStore } from "../src/core/auth.js"; +import { ApiClient } from "../src/core/transport.js"; +import type { AppContext } from "../src/core/context.js"; + +function fakeCtx(): AppContext { + return { + cfg: { baseUrl: "https://api.example" }, + tokens: new StaticTokenStore(""), + flags: { cwd: process.cwd(), json: false, audit: false, yes: false }, + confirm: async () => false, + } as unknown as AppContext; +} + +/** Same as fakeCtx() but with a real ApiClient wired in — the device-code + * request/poll paths of cmdLogin go through ctx.api, unlike the headless + * --username/--password path above which calls loginWithPassword directly. */ +function fakeCtxWithApi(): AppContext { + const tokens = new StaticTokenStore(""); + return { + cfg: { baseUrl: "https://api.example" }, + api: new ApiClient("https://api.example", tokens), + tokens, + flags: { cwd: process.cwd(), json: false, audit: false, yes: false }, + confirm: async () => false, + } as unknown as AppContext; +} + +function captureStderr(): { text: () => string; restore: () => void } { + const orig = process.stderr.write.bind(process.stderr); + let out = ""; + process.stderr.write = ((s: string) => ((out += s), true)) as typeof process.stderr.write; + return { + text: () => out, + restore: () => { + process.stderr.write = orig; + }, + }; +} + +test("cmdLogin (--username/--password): a malicious server `plan` on a SUCCESSFUL login cannot survive into the process's stdout output", async () => { + const realFetch = globalThis.fetch; + const realWrite = process.stdout.write.bind(process.stdout); + const evilPlan = "\x1b]52;c;ZXZpbA==\x07pro" + "E".repeat(300); + globalThis.fetch = (async () => + new Response(JSON.stringify({ authenticated: true, session_token: "sess_ok", plan: evilPlan }), { + status: 200, + headers: { "content-type": "application/json" }, + })) as typeof fetch; + let captured = ""; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (process.stdout as any).write = (chunk: unknown): boolean => { + captured += String(chunk); + return true; + }; + try { + const code = await cmdLogin(fakeCtx(), { username: "u", password: "p" }); + assert.equal(code, 0, "a successful login must still report success"); + assert.ok(captured.includes("Logged in"), "the success message must still be reported"); + assert.ok(!captured.includes("\x1b"), "raw ESC byte must never reach stdout"); + assert.ok(!/[\x00-\x08\x0b-\x1f\x7f-\x9f]/.test(captured), "no other C0/C1 control bytes may reach stdout"); + assert.ok(captured.length < 260, `stdout output must be length-capped, got ${captured.length} chars`); + } finally { + globalThis.fetch = realFetch; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (process.stdout as any).write = realWrite; + } +}); + +test("cmdLogin (--username/--password): a malicious server `reason` cannot survive into the process's stderr output", async () => { + const realFetch = globalThis.fetch; + const realWrite = process.stderr.write.bind(process.stderr); + const evilReason = "\x1b]52;c;ZXZpbA==\x07" + "B".repeat(300); + globalThis.fetch = (async () => + new Response(JSON.stringify({ authenticated: false, reason: evilReason }), { + status: 401, + headers: { "content-type": "application/json" }, + })) as typeof fetch; + let captured = ""; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (process.stderr as any).write = (chunk: unknown): boolean => { + captured += String(chunk); + return true; + }; + try { + const code = await cmdLogin(fakeCtx(), { username: "u", password: "p" }); + assert.equal(code, 1, "headless login must fail (exit 1) for a rejected login"); + assert.ok(captured.length > 0, "the failure message must still be reported"); + assert.ok(!captured.includes("\x1b"), "raw ESC byte must never reach stderr"); + assert.ok(!/[\x00-\x08\x0b-\x1f\x7f-\x9f]/.test(captured), "no other C0/C1 control bytes may reach stderr"); + assert.ok(captured.length < 260, `stderr output must be length-capped, got ${captured.length} chars`); + } finally { + globalThis.fetch = realFetch; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (process.stderr as any).write = realWrite; + } +}); + +// ── LOOP-06 round 3: every cmdLogin catch block must go through the shared +// formatErrorLine/errorHint convention (chat.ts's printError, render.ts's +// Renderer.error) instead of a hand-built, unstyled `✗ ` template +// string with no hint and no /doctor pointer. ── + +test("cmdLogin (--username/--password): a network failure prints the shared formatErrorLine glyph + a /doctor hint", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => { + throw Object.assign(new TypeError("fetch failed"), { cause: { code: "ECONNREFUSED" } }); + }) as typeof fetch; + const stderr = captureStderr(); + try { + const code = await cmdLogin(fakeCtx(), { username: "u", password: "p" }); + assert.equal(code, 1); + const out = stderr.text(); + assert.match(out, /✗/, "must use formatErrorLine's glyph, not a bare template string"); + assert.match(out, /\/doctor/, "a network failure must surface errorHint's /doctor pointer"); + assert.match(out, /\n\n$/, "formatErrorLine's trailing blank-line separator must be present"); + } finally { + globalThis.fetch = realFetch; + stderr.restore(); + } +}); + +test("cmdLogin (device-code flow): a network failure requesting the device code prints the shared glyph + hint", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => { + throw Object.assign(new TypeError("fetch failed"), { cause: { code: "ECONNREFUSED" } }); + }) as typeof fetch; + const stderr = captureStderr(); + try { + const code = await cmdLogin(fakeCtxWithApi(), { noBrowser: true }); + assert.equal(code, 1); + const out = stderr.text(); + assert.match(out, /✗/, "must use formatErrorLine's glyph, not a bare template string"); + assert.match(out, /could not start login/); + assert.match(out, /\/doctor/, "a network failure must surface errorHint's /doctor pointer"); + } finally { + globalThis.fetch = realFetch; + stderr.restore(); + } +}); + +test("cmdLogin (device-code flow): a denied authorization still gets the styled ✗ treatment (no hint to show)", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = (async (url: unknown) => { + const u = String(url); + if (u.includes("/auth/device/code")) { + return new Response( + JSON.stringify({ + device_code: "dc1", + user_code: "ABCD", + verification_uri: "https://x.example/device", + verification_uri_complete: "https://x.example/device?c=1", + interval: 0, + expires_in: 30, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + if (u.includes("/auth/device/token")) { + return new Response(JSON.stringify({ error: "access_denied" }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + throw new Error(`unexpected fetch in test: ${u}`); + }) as typeof fetch; + const realStdoutWrite = process.stdout.write.bind(process.stdout); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (process.stdout as any).write = (): boolean => true; // silence the "To sign in, open:..." prompt noise + const stderr = captureStderr(); + try { + const code = await cmdLogin(fakeCtxWithApi(), { noBrowser: true }); + assert.equal(code, 1); + const out = stderr.text(); + assert.match(out, /✗/, "must use formatErrorLine's glyph, not a bare template string"); + assert.match(out, /authorization denied/); + assert.doesNotMatch(out, /⤷/, "a plain 'denied' error has no errorHint-derived hint to show"); + } finally { + globalThis.fetch = realFetch; + process.stdout.write = realStdoutWrite; + stderr.restore(); + } +}); diff --git a/test/long_running_timeout_overrides.test.ts b/test/long_running_timeout_overrides.test.ts new file mode 100644 index 0000000..19c8568 --- /dev/null +++ b/test/long_running_timeout_overrides.test.ts @@ -0,0 +1,100 @@ +// LOOP-01/LOOP-06 round-1 regression: request()'s new bounded-by-default +// timeout (AETHER_REQUEST_TIMEOUT_MS, 30s) must NOT apply to the handful of +// non-streaming calls that block server-side on a completed long-running +// operation (an autonomous test-drive loop, a benchmark pass, or an LLM- +// generated workflow assessment/brainstorm/plan/finalize) rather than +// returning a quick job handle. Each of these opts into stream()'s own +// generous bound (defaultStreamTimeoutMs()) instead — this file proves that +// override actually reaches the fetch call by making the metadata-call +// default too short to survive on its own. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { runBenchmark } from "../src/core/bench.js"; +import { startTestDrive } from "../src/core/test_drive.js"; +import { assessWorkflow, brainstormWorkflow, planWorkflow, finalizeWorkflow, type Workflow } from "../src/core/workflow.js"; +import { ApiClient } from "../src/core/transport.js"; +import { StaticTokenStore } from "../src/core/auth.js"; + +const ENV_KEY = "AETHER_REQUEST_TIMEOUT_MS"; + +function setShortRequestTimeout(): () => void { + const original = process.env[ENV_KEY]; + process.env[ENV_KEY] = "5"; // far shorter than the delayed response below + return () => { + if (original === undefined) delete process.env[ENV_KEY]; + else process.env[ENV_KEY] = original; + }; +} + +/** Resolves successfully, but slower than the shortened metadata-call + * default -- only survives if the caller passed a wider override. */ +function slowJsonFetch(body: unknown, delayMs = 20): typeof fetch { + return (async () => { + await new Promise((r) => setTimeout(r, delayMs)); + return new Response(JSON.stringify(body), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }) as typeof fetch; +} + +function mockFetch(fn: typeof fetch): () => void { + const original = globalThis.fetch; + globalThis.fetch = fn; + return () => { + globalThis.fetch = original; + }; +} + +const WF: Workflow = { + id: "wf1", + name: "test workflow", + createdAt: "", + updatedAt: "", + nodes: [], + edges: [], + subResourceLinks: [], +}; + +test("runBenchmark survives a response slower than the metadata-call timeout default", async () => { + const restoreEnv = setShortRequestTimeout(); + const restoreFetch = mockFetch( + slowJsonFetch({ bottlenecks: [], optimizations: [], patches: [] }), + ); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + const r = await runBenchmark(api, "neo", "src/x.ts:fn"); + assert.deepEqual(r.patches, []); + } finally { + restoreFetch(); + restoreEnv(); + } +}); + +test("startTestDrive survives a response slower than the metadata-call timeout default", async () => { + const restoreEnv = setShortRequestTimeout(); + const restoreFetch = mockFetch(slowJsonFetch({ status: "passed", iterations: 3, patches: [] })); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + const r = await startTestDrive(api, "neo", "src/x.ts:fn", "."); + assert.equal(r.status, "passed"); + } finally { + restoreFetch(); + restoreEnv(); + } +}); + +test("assessWorkflow / brainstormWorkflow / planWorkflow / finalizeWorkflow each survive a response slower than the metadata-call timeout default", async () => { + const restoreEnv = setShortRequestTimeout(); + const restoreFetch = mockFetch(slowJsonFetch({ ok: true })); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + await assert.doesNotReject(() => assessWorkflow(api, WF)); + await assert.doesNotReject(() => brainstormWorkflow(api, WF)); + await assert.doesNotReject(() => planWorkflow(api, WF)); + await assert.doesNotReject(() => finalizeWorkflow(api, WF, "# plan")); + } finally { + restoreFetch(); + restoreEnv(); + } +}); diff --git a/test/model_picker.test.ts b/test/model_picker.test.ts index 87899e9..7584289 100644 --- a/test/model_picker.test.ts +++ b/test/model_picker.test.ts @@ -1,7 +1,11 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { groupItems, flattenGroups, currentIndex, renderPicker } from "../src/ui/model_picker.js"; +import { groupItems, flattenGroups, currentIndex, renderPicker, pickModel } from "../src/ui/model_picker.js"; +import { theme } from "../src/ui/theme.js"; +import { handleSlash } from "../src/commands/slash.js"; import type { CatalogItem } from "../src/types.js"; +import type { AppContext } from "../src/core/context.js"; +import type { Writable } from "node:stream"; function item(overrides: Partial & { id: string }): CatalogItem { return { @@ -142,3 +146,182 @@ test("groupItems only-orchestrators returns just orchestrators group", () => { assert.equal(groups[0]!.label, "Orchestrators"); assert.equal(groups[0]!.items.length, 2); }); + +// ── pickModel (LOOP-06): a throwing key handler must not read as a cancel ── +// +// pickModel takes over raw stdin for arrow-key navigation, so exercising its +// interactive branch means faking process.stdin as a TTY with a captured +// "data" listener. Everything is restored in `finally`. + +type StdinPatch = { + isTTY: PropertyDescriptor | undefined; + rawListeners: unknown; + removeAllListeners: unknown; + on: unknown; + removeListener: unknown; +}; + +function patchStdinAsTTY(onData: (cb: (chunk: Buffer) => void) => void): StdinPatch { + const stdin = process.stdin as unknown as Record; + const saved: StdinPatch = { + isTTY: Object.getOwnPropertyDescriptor(process.stdin, "isTTY"), + rawListeners: stdin["rawListeners"], + removeAllListeners: stdin["removeAllListeners"], + on: stdin["on"], + removeListener: stdin["removeListener"], + }; + Object.defineProperty(process.stdin, "isTTY", { value: true, configurable: true }); + stdin["rawListeners"] = () => []; + stdin["removeAllListeners"] = () => process.stdin; + stdin["on"] = (event: string, cb: (chunk: Buffer) => void) => { + if (event === "data") onData(cb); + return process.stdin; + }; + stdin["removeListener"] = () => process.stdin; + return saved; +} + +function restoreStdin(saved: StdinPatch): void { + const stdin = process.stdin as unknown as Record; + if (saved.isTTY) Object.defineProperty(process.stdin, "isTTY", saved.isTTY); + else delete (process.stdin as unknown as { isTTY?: boolean }).isTTY; + stdin["rawListeners"] = saved.rawListeners; + stdin["removeAllListeners"] = saved.removeAllListeners; + stdin["on"] = saved.on; + stdin["removeListener"] = saved.removeListener; +} + +function fakeOut(throwOn?: string): { out: Writable; writes: string[] } { + const writes: string[] = []; + const out = { + write: (s: string): boolean => { + writes.push(s); + if (throwOn !== undefined && s === throwOn) throw new Error("simulated render fault"); + return true; + }, + } as unknown as Writable; + return { out, writes }; +} + +test("pickModel: a fault inside the key handler prints a distinct diagnostic, not a silent cancel", async () => { + const items = [item({ id: "opus" })]; + const captured: { feed: ((chunk: Buffer) => void) | null } = { feed: null }; + const saved = patchStdinAsTTY((cb) => { + captured.feed = cb; + }); + // rerender() writes the literal string "\x1b[H" (home + redraw) — throwing + // there simulates a real render/write fault reachable only via a key that + // takes the rerender path (up/down), not via the initial render or cleanup. + const { out, writes } = fakeOut("\x1b[H"); + try { + const result = pickModel(items, out); + assert.ok(captured.feed, "pickModel must attach a stdin 'data' listener"); + captured.feed!(Buffer.from("\x1b[A", "utf8")); // up arrow -> rerender() -> throws + const picked = await result; + assert.equal( + picked, + undefined, + "a caught fault resolves undefined (not null) so the caller can tell it apart from a deliberate Escape and skip printing its own redundant 'kept current session.' line", + ); + assert.ok( + writes.some((w) => /picker error/.test(w)), + "a distinct diagnostic must be written so this isn't indistinguishable from Escape", + ); + } finally { + restoreStdin(saved); + } +}); + +// ── pickModel (LOOP-06): empty-state line matches the picker's dim styling ── +// +// theme is disabled (non-TTY) in this test harness, so theme.dim() is a +// no-op passthrough — a plain-text assertion here would pass identically +// against the pre-fix `out.write("no models available.\n")` and tell us +// nothing about the fix. To make this a real regression test, monkeypatch +// the shared theme singleton (model_picker.ts imports the same object +// instance, so the patch is visible inside pickModel) and assert the +// message is actually routed through it. + +test("pickModel: empty items routes the no-models message through theme.dim", async () => { + const origDim = theme.dim; + theme.dim = (s: string): string => `[dim]${s}[/dim]`; + try { + const { out, writes } = fakeOut(); + const picked = await pickModel([], out); + assert.equal(picked, null, "no items means nothing to pick"); + assert.match( + writes.join(""), + /\[dim\]no models available\.\[\/dim\]/, + "the empty-state line must be wrapped in theme.dim like the rest of the picker (footer hints, locked-item marker)", + ); + } finally { + // MUST restore: --test-isolation=none shares this singleton across + // every test in the process, so a leaked patch would corrupt others. + theme.dim = origDim; + } +}); + +test("pickModel: a deliberate Escape resolves null WITHOUT the fault diagnostic", async () => { + const items = [item({ id: "opus" })]; + const captured: { feed: ((chunk: Buffer) => void) | null } = { feed: null }; + const saved = patchStdinAsTTY((cb) => { + captured.feed = cb; + }); + const { out, writes } = fakeOut(); // never throws + try { + const result = pickModel(items, out); + assert.ok(captured.feed, "pickModel must attach a stdin 'data' listener"); + captured.feed!(Buffer.from("\x1b", "utf8")); // bare Escape + const picked = await result; + assert.equal(picked, null, "Escape cancels with null, same as before"); + assert.ok( + !writes.some((w) => /picker error/.test(w)), + "a deliberate cancel must NOT print the internal-fault diagnostic", + ); + } finally { + restoreStdin(saved); + } +}); + +// ── showPicker (slash.ts) composition: no duplicate message on a fault ── +// +// pickModel resolving `undefined` (not `null`) on an internal fault is only +// half the fix — showPicker must actually read that signal, or a real fault +// still shows its own diagnostic AND the generic "kept current session." +// line back to back. This exercises the full handleSlash -> showPicker -> +// pickModel path, not pickModel in isolation. + +function fakeModelCtx(): AppContext { + return { + flags: { yes: false, json: false, audit: false, cwd: "." }, + cfg: { defaultModel: "haiku", baseUrl: "x" }, + api: { getJson: async () => ({ tier: "pro", default: "haiku", models: [item({ id: "opus" })] }) }, + confirm: async () => false, + } as unknown as AppContext; +} + +test("showPicker: a picker fault prints exactly ONE message, not the diagnostic plus a redundant 'kept current session.'", async () => { + const captured: { feed: ((chunk: Buffer) => void) | null } = { feed: null }; + const saved = patchStdinAsTTY((cb) => { + captured.feed = cb; + }); + const { out, writes } = fakeOut("\x1b[H"); // rerender() write throws -> simulated fault + try { + const resultPromise = handleSlash(fakeModelCtx(), "/model", out, undefined); + // Give getCatalog's fetch + pickModel's render a tick to attach the listener. + for (let i = 0; i < 20 && !captured.feed; i++) await Promise.resolve(); + assert.ok(captured.feed, "pickModel must attach a stdin 'data' listener via the /model (bare) path"); + captured.feed!(Buffer.from("\x1b[A", "utf8")); // up arrow -> rerender() -> throws + const res = await resultPromise; + assert.equal(res.restart, undefined, "a faulted picker must not signal a model switch"); + const faultLines = writes.filter((w) => /kept current session/.test(w)); + assert.equal( + faultLines.length, + 1, + `expected exactly one 'kept current session' message, got ${faultLines.length}: ${JSON.stringify(writes)}`, + ); + assert.ok(faultLines[0] && /picker error/.test(faultLines[0]), "the surviving message must be pickModel's own distinct diagnostic"); + } finally { + restoreStdin(saved); + } +}); diff --git a/test/slash.test.ts b/test/slash.test.ts index 893b426..9fe2607 100644 --- a/test/slash.test.ts +++ b/test/slash.test.ts @@ -1,6 +1,6 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { resolveSelection, handleSlash, primeCatalog } from "../src/commands/slash.js"; +import { resolveSelection, handleSlash, primeCatalog, confirmSwitch } from "../src/commands/slash.js"; import type { CatalogItem } from "../src/types.js"; import type { AppContext } from "../src/core/context.js"; @@ -112,3 +112,40 @@ test("/gather rejects when no orchestrator active", async () => { assert.equal(res.exit, false); assert.match(out.join(""), /requires an active orchestrator/i); }); + +// ── confirmSwitch: a locked (tier-gated) item (LOOP-06) ── +// +// Tested directly against confirmSwitch (rather than through handleSlash) +// because the module-level catalog cache in slash.ts is populated once per +// process by the /model tests above and never refetched for a plain id — a +// second, differently-available "opus" wouldn't reach the handler at all. +// confirmSwitch is the exact function this fix touches, so this exercises it +// without fighting that cache. + +test("confirmSwitch: a locked item never signals a restart", async () => { + const out: string[] = []; + const ctx = { flags: { yes: false }, confirm: async () => true } as unknown as AppContext; + const locked = item("gpt5-pro"); + locked.available = false; + const res = await confirmSwitch(ctx, { write: (s: string) => out.push(s) } as never, locked, "model", "free"); + assert.equal(res, null, "a locked item must not produce a restart signal"); +}); + +test("confirmSwitch: locked-item message matches the 403 tier-restriction wording", async () => { + const out: string[] = []; + const ctx = { flags: { yes: false }, confirm: async () => true } as unknown as AppContext; + const locked = item("gpt5-pro"); + locked.available = false; + await confirmSwitch(ctx, { write: (s: string) => out.push(s) } as never, locked, "model", "free"); + const printed = out.join(""); + assert.match(printed, /gpt5-pro is locked on tier free/); + // Same actionable pointer as httpStatusHint(403) in errors.ts, so a + // tier-lock reached via the picker reads the same as one reached over + // the wire, not as a dead end with no next step. + assert.match(printed, /check: \/tier or `aether models`/); + // theme is disabled (non-TTY) in this test harness, so theme.dim() is a + // no-op passthrough here and the plain wording above is the reachable + // proxy for "goes through the same theme.dim(...) call the restart + // warning two lines down already uses" — see the ANSI-wrapping assertions + // in test/theme_factory.test.ts for that half of the behavior. +}); diff --git a/test/slash_abort.test.ts b/test/slash_abort.test.ts index eee4910..f308f9c 100644 --- a/test/slash_abort.test.ts +++ b/test/slash_abort.test.ts @@ -23,14 +23,13 @@ const tokens = { get: async () => "aek_t" } as unknown as TokenStore; test("a network-backed slash command's signal reaches fetch (so SIGINT can cancel it, not the session)", async () => { const captured: { signal?: AbortSignal | null } = {}; const real = globalThis.fetch; + // Hangs (rather than resolving immediately) so there's a genuine in-flight + // window to abort during — request()'s internal `net` controller (see + // transport.ts, LOOP-01/LOOP-06 round-1) is only wired to the caller's + // abort event while the call is still pending. globalThis.fetch = (async (_url: unknown, init?: RequestInit) => { captured.signal = init?.signal; - return { - ok: true, - status: 200, - headers: new Headers({ "content-type": "application/json" }), - json: async () => ({ entries: [], count: 0 }), - } as unknown as Response; + return new Promise(() => {}); }) as typeof globalThis.fetch; try { const ctx = { @@ -45,8 +44,15 @@ test("a network-backed slash command's signal reaches fetch (so SIGINT can cance // catalog cache, so this assertion can't be defeated by an earlier test // (e.g. "/model switch...") having already warmed that cache under // --test-isolation=none. - await handleSlash(ctx, "/audit", new Capture(), ac.signal); - assert.equal(captured.signal, ac.signal); + const pending = handleSlash(ctx, "/audit", new Capture(), ac.signal); + ac.abort(); + await assert.rejects(pending); + // request() now wires the caller's signal to its OWN internal `net` + // AbortController (see abort.test.ts), so we assert the behavioral + // guarantee — a signal reached fetch, and aborting the caller's + // controller aborted it — not reference identity to ac.signal. + assert.ok(captured.signal, "fetch should have received a signal"); + assert.equal(captured.signal?.aborted, true); } finally { globalThis.fetch = real; } diff --git a/test/transport_getbinary_postform.test.ts b/test/transport_getbinary_postform.test.ts new file mode 100644 index 0000000..de74400 --- /dev/null +++ b/test/transport_getbinary_postform.test.ts @@ -0,0 +1,233 @@ +// Regression tests for LOOP-01 round 2 findings on ApiClient.getBinary() / +// postForm() — the two methods commit 1d33357 added to fix the seed +// raw-fetch bypass bug: +// +// HIGH — isCredentialSafeUrl() only checks URL SCHEME (any https host +// passes), so getBinary() attached the live session bearer to ANY absolute +// https URL a caller passed, including a third-party host completely +// different from the configured Aether API baseUrl. Fixed by gating +// attachment on isSameOrigin(target, baseUrl) instead — a cross-origin +// target is now fetched unauthenticated rather than either leaking the +// token or failing the whole call closed. +// +// HIGH — getBinary()/postForm() had NO timeout/AbortSignal.timeout bound at +// all, unlike request()/stream(), reintroducing the exact "stalled +// connection hangs forever" problem a7b3621 fixed for request(). Both now +// accept a timeoutMs override (default AETHER_REQUEST_TIMEOUT_MS) mirroring +// getJson/postJson/deleteJson; getBinary additionally wraps its body with +// an idle/quiet-period timeout (like stream()'s withIdleTimeout) so a +// large-but-healthy download isn't killed by a flat overall cap. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { ApiClient, isSameOrigin } from "../src/core/transport.js"; +import { StaticTokenStore } from "../src/core/auth.js"; +import { RequestTimeoutError, StreamTimeoutError } from "../src/core/errors.js"; + +const enc = new TextEncoder(); + +type Call = { url: string; init: RequestInit }; + +function mockFetch(fn: typeof fetch): () => void { + const original = globalThis.fetch; + globalThis.fetch = fn; + return () => { + globalThis.fetch = original; + }; +} + +function bearer(init: RequestInit): string { + return (init.headers as Record | undefined)?.["Authorization"] ?? ""; +} + +// ── isSameOrigin ──────────────────────────────────────────────────────────── + +test("isSameOrigin: matches only when scheme+host+port all agree", () => { + assert.equal(isSameOrigin("https://api.example/x", "https://api.example"), true); + assert.equal(isSameOrigin("https://api.example:443/x", "https://api.example"), true, "default https port normalizes away"); + assert.equal(isSameOrigin("https://other.example/x", "https://api.example"), false); + assert.equal(isSameOrigin("http://api.example/x", "https://api.example"), false, "scheme differs"); + assert.equal(isSameOrigin("https://api.example:8443/x", "https://api.example"), false, "port differs"); + assert.equal(isSameOrigin("not a url", "https://api.example"), false); +}); + +// ── getBinary: same-origin gating (HIGH #1) ───────────────────────────────── + +test("getBinary: attaches the bearer to a SAME-origin absolute URL", async () => { + const calls: Call[] = []; + const restore = mockFetch((async (url: unknown, init?: RequestInit) => { + calls.push({ url: String(url), init: init ?? {} }); + return new Response("data"); + }) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + await api.getBinary("https://api.example/vault/spaces/download/f.bin"); + assert.equal(bearer(calls[0]!.init), "Bearer sess_abc"); + } finally { + restore(); + } +}); + +test("getBinary: attaches the bearer to a RELATIVE path (always same-origin by construction)", async () => { + const calls: Call[] = []; + const restore = mockFetch((async (url: unknown, init?: RequestInit) => { + calls.push({ url: String(url), init: init ?? {} }); + return new Response("data"); + }) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + await api.getBinary("/vault/spaces/download/f.bin"); + assert.equal(bearer(calls[0]!.init), "Bearer sess_abc"); + } finally { + restore(); + } +}); + +test("getBinary: does NOT attach the bearer to a cross-origin absolute URL, even https", async () => { + const calls: Call[] = []; + const restore = mockFetch((async (url: unknown, init?: RequestInit) => { + calls.push({ url: String(url), init: init ?? {} }); + return new Response("data"); + }) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + await api.getBinary("https://cdn.other-host.example/media/f.png"); + assert.equal(calls.length, 1); + assert.equal(bearer(calls[0]!.init), "", "cross-origin target must not receive the live session token"); + } finally { + restore(); + } +}); + +test("getBinary: a cross-origin 401 is not retried through /auth/refresh (no token was ever sent there)", async () => { + const calls: Call[] = []; + const restore = mockFetch((async (url: unknown, init?: RequestInit) => { + calls.push({ url: String(url), init: init ?? {} }); + return new Response("nope", { status: 401 }); + }) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + await assert.rejects(() => api.getBinary("https://cdn.other-host.example/media/f.png")); + assert.equal(calls.length, 1, "no /auth/refresh call, no retry — the 401 is unrelated to our session"); + } finally { + restore(); + } +}); + +// ── getBinary: timeout (HIGH #2) ───────────────────────────────────────────── + +test("getBinary: times out on a stalled connection instead of hanging forever", async () => { + const restore = mockFetch((() => new Promise(() => {})) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + await assert.rejects( + () => api.getBinary("/vault/spaces/download/f.bin", undefined, 5), + (err: unknown) => err instanceof RequestTimeoutError, + ); + } finally { + restore(); + } +}); + +test("getBinary: times out when the response body goes quiet mid-download (idle timeout, not a flat cap)", async () => { + const restore = mockFetch((async () => { + const body = new ReadableStream({ + start(controller) { + controller.enqueue(enc.encode("first-chunk")); + // never enqueue again, never close -> body goes quiet forever + }, + }); + return new Response(body, { status: 200 }); + }) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + const res = await api.getBinary("/vault/spaces/download/f.bin", undefined, 5); + assert.ok(res.body, "first chunk arrives fine, headers resolved before the quiet period"); + const iterator = (res.body as unknown as AsyncIterable)[Symbol.asyncIterator](); + const first = await iterator.next(); + assert.equal(first.done, false); + await assert.rejects( + () => iterator.next(), + (err: unknown) => err instanceof StreamTimeoutError, + ); + } finally { + restore(); + } +}); + +test("getBinary: a large-but-healthy download (continuous chunks) is NOT killed by a flat cap", async () => { + const restore = mockFetch((async () => { + const body = new ReadableStream({ + async start(controller) { + // Each chunk arrives within the idle window, but the total transfer + // time comfortably exceeds a single flat timeoutMs — proving the + // guard is an idle/quiet-period timeout, not an overall cap. + for (let i = 0; i < 5; i++) { + await new Promise((r) => setTimeout(r, 8)); + controller.enqueue(enc.encode(`chunk-${i}`)); + } + controller.close(); + }, + }); + return new Response(body, { status: 200 }); + }) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + const res = await api.getBinary("/vault/spaces/download/f.bin", undefined, 500); + const chunks: string[] = []; + for await (const chunk of res.body as unknown as AsyncIterable) { + chunks.push(Buffer.from(chunk).toString("utf-8")); + } + assert.deepEqual(chunks, ["chunk-0", "chunk-1", "chunk-2", "chunk-3", "chunk-4"]); + } finally { + restore(); + } +}); + +test("getBinary: an explicit caller AbortSignal still wins as AbortError, not RequestTimeoutError", async () => { + const restore = mockFetch((() => new Promise(() => {})) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + const controller = new AbortController(); + const pending = api.getBinary("/vault/spaces/download/f.bin", controller.signal, 2000); + controller.abort(); + await assert.rejects( + () => pending, + (err: unknown) => err instanceof Error && err.name === "AbortError", + ); + } finally { + restore(); + } +}); + +// ── postForm: timeout (HIGH #2) ─────────────────────────────────────────── + +test("postForm: times out on a stalled connection instead of hanging forever", async () => { + const restore = mockFetch((() => new Promise(() => {})) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + const form = new FormData(); + form.append("file", new Blob(["x"]), "f.txt"); + await assert.rejects( + () => api.postForm("/vault/spaces/upload", form, undefined, 5), + (err: unknown) => err instanceof RequestTimeoutError, + ); + } finally { + restore(); + } +}); + +test("postForm: an explicit timeoutMs override widens the bound past a short default", async () => { + const restore = mockFetch((async () => { + await new Promise((r) => setTimeout(r, 20)); + return new Response(JSON.stringify({ key: "k1" }), { status: 200 }); + }) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_abc")); + const form = new FormData(); + form.append("file", new Blob(["x"]), "f.txt"); + const out = await api.postForm<{ key: string }>("/vault/spaces/upload", form, undefined, 200); + assert.deepEqual(out, { key: "k1" }); + } finally { + restore(); + } +}); diff --git a/test/transport_request.test.ts b/test/transport_request.test.ts new file mode 100644 index 0000000..c23521d --- /dev/null +++ b/test/transport_request.test.ts @@ -0,0 +1,262 @@ +// Regression tests for two round-1 meta-loop findings on ApiClient.request() +// (backing getJson/postJson/deleteJson): +// +// LOOP-01 / LOOP-06 — request() had NO timeout at all, unlike stream()'s +// 120s default. A silently-dropped connection to /models, /auth/refresh, or +// the device-poll endpoint just hung forever. Now bounded by +// AETHER_REQUEST_TIMEOUT_MS (default 30s, mirrors AETHER_STREAM_TIMEOUT_MS). +// +// LOOP-06 — a malformed (non-empty, non-JSON) 2xx body used to silently +// become `undefined as T`, so callers that trust T (slash.ts's getCatalog, +// auth.ts's authRefresh) crashed one layer up with a raw property-access +// TypeError instead of a coherent, hintable error. Now throws +// MalformedResponseError. A genuinely EMPTY 2xx/204 body (e.g. deleteJson's +// "No Content") is unaffected and still resolves to `undefined`. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + ApiClient, + DEFAULT_REQUEST_TIMEOUT_MS, + DEVICE_TOKEN_PATH, + REFRESH_PATH, + defaultRequestTimeoutMs, +} from "../src/core/transport.js"; +import { StaticTokenStore } from "../src/core/auth.js"; +import { MalformedResponseError, RequestTimeoutError, errorHint } from "../src/core/errors.js"; +import { hintFor } from "../src/core/error_hints.js"; + +const ENV_KEY = "AETHER_REQUEST_TIMEOUT_MS"; + +function mockFetch(fn: typeof fetch): () => void { + const original = globalThis.fetch; + globalThis.fetch = fn; + return () => { + globalThis.fetch = original; + }; +} + +function setEnvTimeout(value: string | undefined): () => void { + const original = process.env[ENV_KEY]; + if (value === undefined) delete process.env[ENV_KEY]; + else process.env[ENV_KEY] = value; + return () => { + if (original === undefined) delete process.env[ENV_KEY]; + else process.env[ENV_KEY] = original; + }; +} + +// ── 1. bounded-by-default timeout ──────────────────────────────────────── + +test("ApiClient.getJson times out on a stalled connection instead of hanging forever", async () => { + const restoreFetch = mockFetch((() => new Promise(() => {})) as typeof fetch); + const restoreEnv = setEnvTimeout("5"); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + await assert.rejects( + () => api.getJson("/models"), + (err: unknown) => err instanceof RequestTimeoutError, + ); + } finally { + restoreFetch(); + restoreEnv(); + } +}); + +test("ApiClient.postJson (used by device-poll and vault/workflow calls) times out on a stalled connection", async () => { + // Mirrors device.ts's pollForToken -> api.postJson(DEVICE_TOKEN_PATH, ...) + // call, which passes no signal at all — the exact call site the finding + // named as sitting past its own deadline on a stalled fetch. + const restoreFetch = mockFetch((() => new Promise(() => {})) as typeof fetch); + const restoreEnv = setEnvTimeout("5"); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + await assert.rejects( + () => api.postJson(DEVICE_TOKEN_PATH, { device_code: "dc" }), + (err: unknown) => err instanceof RequestTimeoutError, + ); + } finally { + restoreFetch(); + restoreEnv(); + } +}); + +test("ApiClient.getJson: an explicit caller AbortSignal still wins as AbortError, not RequestTimeoutError", async () => { + const restoreFetch = mockFetch((() => new Promise(() => {})) as typeof fetch); + const restoreEnv = setEnvTimeout("2000"); // long enough that the abort fires first + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + const controller = new AbortController(); + const pending = api.getJson("/models", controller.signal); + controller.abort(); + await assert.rejects( + () => pending, + (err: unknown) => err instanceof Error && err.name === "AbortError", + ); + } finally { + restoreFetch(); + restoreEnv(); + } +}); + +test("ApiClient.postJson: an explicit timeoutMs override widens the bound past a short AETHER_REQUEST_TIMEOUT_MS default (what CHAT_PATH's non-streaming fallback needs to survive a slow-but-healthy LLM turn)", async () => { + const restoreEnv = setEnvTimeout("5"); // the metadata-call default alone would kill this call + const restore = mockFetch((async () => { + await new Promise((r) => setTimeout(r, 20)); + return new Response(JSON.stringify({ response: "ok" }), { status: 200 }); + }) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + const out = await api.postJson<{ response: string }>("/agent/chat", {}, undefined, 200); + assert.deepEqual(out, { response: "ok" }); + } finally { + restore(); + restoreEnv(); + } +}); + +test("ApiClient.postJson: an explicit SHORT timeoutMs override still times out even when AETHER_REQUEST_TIMEOUT_MS default is long", async () => { + // Proves the per-call override REPLACES the default in both directions, + // not just widens it. + const restoreEnv = setEnvTimeout("60000"); + const restore = mockFetch((() => new Promise(() => {})) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + await assert.rejects( + () => api.postJson("/agent/chat", {}, undefined, 5), + (err: unknown) => err instanceof RequestTimeoutError, + ); + } finally { + restore(); + restoreEnv(); + } +}); + +test("defaultRequestTimeoutMs: unset env falls back to DEFAULT_REQUEST_TIMEOUT_MS", () => { + const restore = setEnvTimeout(undefined); + try { + assert.equal(defaultRequestTimeoutMs(), DEFAULT_REQUEST_TIMEOUT_MS); + } finally { + restore(); + } +}); + +test("defaultRequestTimeoutMs: '0' means disabled, not the 30s default", () => { + const restore = setEnvTimeout("0"); + try { + assert.equal(defaultRequestTimeoutMs(), 0); + } finally { + restore(); + } +}); + +test("defaultRequestTimeoutMs: a valid positive override is honored", () => { + const restore = setEnvTimeout("5000"); + try { + assert.equal(defaultRequestTimeoutMs(), 5000); + } finally { + restore(); + } +}); + +test("defaultRequestTimeoutMs: invalid or negative values fall back to the default", () => { + const restoreGarbage = setEnvTimeout("not-a-number"); + try { + assert.equal(defaultRequestTimeoutMs(), DEFAULT_REQUEST_TIMEOUT_MS); + } finally { + restoreGarbage(); + } + const restoreNegative = setEnvTimeout("-5"); + try { + assert.equal(defaultRequestTimeoutMs(), DEFAULT_REQUEST_TIMEOUT_MS); + } finally { + restoreNegative(); + } +}); + +// ── 2. malformed-JSON 2xx body ──────────────────────────────────────────── + +test("ApiClient.getJson: a non-empty, non-JSON 2xx body throws MalformedResponseError instead of silently becoming undefined", async () => { + const restore = mockFetch((async () => new Response("not json", { status: 200 })) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + await assert.rejects( + () => api.getJson("/models"), + (err: unknown) => err instanceof MalformedResponseError && (err as MalformedResponseError).status === 200, + ); + } finally { + restore(); + } +}); + +test("ApiClient.postJson: same malformed-body guard applies to POST (e.g. /auth/refresh)", async () => { + const restore = mockFetch((async () => new Response("not json", { status: 200 })) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + await assert.rejects( + () => api.postJson(REFRESH_PATH, {}), + (err: unknown) => err instanceof MalformedResponseError, + ); + } finally { + restore(); + } +}); + +test("ApiClient.getJson: a body TRUNCATED mid-object (e.g. a dropped connection) throws MalformedResponseError, not silently undefined", async () => { + // JSON.parse's error for truncated-but-nonempty input is the SAME + // "Unexpected end of JSON input" message it gives for a genuinely empty + // body -- classifying by that message text alone would misfile this exact + // scenario (a connection dropped mid-response) as legitimate "no content" + // and swallow it right back into `undefined`, defeating the fix. Reading + // the raw text first and checking it's non-empty BEFORE parsing is what + // tells the two apart. + const restore = mockFetch( + (async () => new Response('{"models":[{"id":"a"', { status: 200 })) as typeof fetch, + ); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + await assert.rejects( + () => api.getJson("/models"), + (err: unknown) => err instanceof MalformedResponseError && (err as MalformedResponseError).status === 200, + ); + } finally { + restore(); + } +}); + +test("ApiClient.deleteJson: a genuinely EMPTY 2xx body (204 No Content) still resolves to undefined, no regression", async () => { + const restore = mockFetch((async () => new Response(null, { status: 204 })) as typeof fetch); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + const out = await api.deleteJson("/vault/spaces/delete"); + assert.equal(out, undefined); + } finally { + restore(); + } +}); + +test("ApiClient.getJson: a well-formed JSON body still parses normally (no regression)", async () => { + const restore = mockFetch( + (async () => new Response(JSON.stringify({ models: [{ id: "a" }] }), { status: 200 })) as typeof fetch, + ); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("")); + const out = await api.getJson<{ models: unknown[] }>("/models"); + assert.deepEqual(out, { models: [{ id: "a" }] }); + } finally { + restore(); + } +}); + +// ── 3. errorHint / hintFor recognize the new error types ───────────────── + +test("errorHint and hintFor give an actionable hint for MalformedResponseError, not silence", () => { + const err = new MalformedResponseError(200); + assert.match(errorHint(err, "https://x") ?? "", /retry|doctor/i); + assert.match(hintFor(err) ?? "", /retry|doctor/i); +}); + +test("errorHint and hintFor give an actionable hint for RequestTimeoutError, not silence", () => { + const err = new RequestTimeoutError(30_000); + assert.match(errorHint(err, "https://x") ?? "", /retry|doctor/i); + assert.match(hintFor(err) ?? "", /retry|doctor/i); +}); diff --git a/test/vault_media_error_hints.test.ts b/test/vault_media_error_hints.test.ts new file mode 100644 index 0000000..21ac081 --- /dev/null +++ b/test/vault_media_error_hints.test.ts @@ -0,0 +1,194 @@ +// Regression tests for LOOP-01 round 2 (media.ts:233-235 / media.ts:208-209 / +// vault.ts:276 / slash_media.ts's 9 catch blocks). +// +// core/vault.ts and core/vision.ts already throw a properly-classified +// HttpError for every non-2xx response (see test/vault_vision_401.test.ts and +// test/auth_401.test.ts) — but the *command layer* never routed that +// classification through errorHint()/hintFor(). vault.ts's fail() and +// media.ts's fail() printed the SAME hardcoded "are you logged in?" hint +// regardless of the actual status; media.ts's mediaGenerate() and every catch +// block in slash_media.ts printed only the raw err.message with no hint at +// all. These tests confirm a 401/402/403/5xx each now render the distinct, +// already-implemented hint from core/error_hints.ts's hintFor(), matching the +// fix already applied to workflow.ts's `workflowNew` (see auth_401.test.ts / +// error_hints.test.ts for the underlying hintFor contract). +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { rmSync } from "node:fs"; +import { join } from "node:path"; +import type { Writable } from "node:stream"; +import { ApiClient, MODELS_PATH } from "../src/core/transport.js"; +import { StaticTokenStore } from "../src/core/auth.js"; +import type { AppContext } from "../src/core/context.js"; +import { cmdVault } from "../src/commands/vault.js"; +import { cmdImage } from "../src/commands/media.js"; +import { photogenSlash } from "../src/commands/slash_media.js"; + +function jsonRes(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { status, headers: { "content-type": "application/json" } }); +} + +function stubFetch(handler: (url: string) => Response): void { + globalThis.fetch = (async (url: unknown) => handler(String(url))) as typeof globalThis.fetch; +} + +function fakeCtx(api: ApiClient): AppContext { + return { + cfg: { baseUrl: "https://api.example" }, + api, + tokens: new StaticTokenStore("aek_test"), + flags: { cwd: process.cwd(), json: false, audit: false, yes: false }, + confirm: async () => true, + } as unknown as AppContext; +} + +function captureWrite(stream: NodeJS.WriteStream): { get: () => string; restore: () => void } { + const orig = stream.write.bind(stream); + let buf = ""; + stream.write = ((chunk: unknown): boolean => { + buf += String(chunk); + return true; + }) as typeof stream.write; + return { get: () => buf, restore: () => { stream.write = orig; } }; +} + +// mediaGenerate/photogenSlash both call ensureOutputDir() unconditionally +// before dispatching, which mkdir's "./aether-output" relative to cwd as a +// side effect of the code under test (not something these tests add) — clean +// it up afterward so the test run doesn't leave it behind in the repo root. +function cleanupOutputDir(): void { + rmSync(join(process.cwd(), "aether-output"), { recursive: true, force: true }); +} + +// ── vault.ts's fail() ─────────────────────────────────────────────────── + +test("cmdVault: a 402 (out of UVT balance) shows the balance hint, not 'are you logged in?'", async () => { + const real = globalThis.fetch; + stubFetch(() => jsonRes(402, { detail: "insufficient balance" })); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdVault(fakeCtx(api), ["list"]); + assert.equal(code, 1); + assert.match(cap.get(), /UVT|balance/i); + assert.doesNotMatch(cap.get(), /are you logged in/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +test("cmdVault: a 403 (plan/tier) shows the plan/tier hint, not 'are you logged in?'", async () => { + const real = globalThis.fetch; + stubFetch(() => jsonRes(403, { detail: "forbidden" })); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdVault(fakeCtx(api), ["list"]); + assert.equal(code, 1); + assert.match(cap.get(), /plan|tier/i); + assert.doesNotMatch(cap.get(), /are you logged in/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +test("cmdVault: a 401 still shows the auth-login hint (unchanged for this status)", async () => { + const real = globalThis.fetch; + stubFetch(() => jsonRes(401, { detail: "expired" })); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + await cmdVault(fakeCtx(api), ["list"]); + assert.match(cap.get(), /aether auth login/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +test("cmdVault: a 5xx shows a retry/connectivity hint, not the auth-login hint", async () => { + const real = globalThis.fetch; + stubFetch(() => jsonRes(500, { detail: "server error" })); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + await cmdVault(fakeCtx(api), ["list"]); + assert.match(cap.get(), /retry|doctor/i); + assert.doesNotMatch(cap.get(), /are you logged in/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +// ── media.ts's fail() (mediaModelsList) ────────────────────────────────── + +test("cmdImage models: a 402 shows the balance hint via fail(), not 'are you logged in?'", async () => { + const real = globalThis.fetch; + stubFetch((url) => (url.endsWith(MODELS_PATH) ? jsonRes(402, { detail: "insufficient balance" }) : jsonRes(404, {}))); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdImage(fakeCtx(api), ["models"]); + assert.equal(code, 1); + assert.match(cap.get(), /UVT|balance/i); + assert.doesNotMatch(cap.get(), /are you logged in/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +// ── media.ts's mediaGenerate inline catch ──────────────────────────────── + +test("cmdImage generate: a 403 from dispatchGeneration prints the plan/tier hint to stdout", async () => { + const real = globalThis.fetch; + stubFetch(() => jsonRes(403, { detail: "forbidden" })); + const cap = captureWrite(process.stdout); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdImage(fakeCtx(api), ["a", "cat", "wearing", "a", "hat"]); + assert.equal(code, 0); // mediaGenerate always returns 0 — per-item failures are printed inline + assert.match(cap.get(), /plan|tier/i); + assert.doesNotMatch(cap.get(), /are you logged in/); + } finally { + cap.restore(); + globalThis.fetch = real; + cleanupOutputDir(); + } +}); + +// ── slash_media.ts's writeErr() ────────────────────────────────────────── + +test("photogenSlash: a 402 from dispatchGeneration writes the balance hint to the slash output stream", async () => { + const real = globalThis.fetch; + stubFetch(() => jsonRes(402, { detail: "insufficient balance" })); + let out = ""; + const writable = { write: (chunk: unknown) => { out += String(chunk); return true; } } as unknown as Writable; + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + await photogenSlash(fakeCtx(api), writable, "a cat wearing a hat", false); + assert.match(out, /UVT|balance/i); + assert.doesNotMatch(out, /are you logged in/); + } finally { + globalThis.fetch = real; + cleanupOutputDir(); + } +}); + +test("photogenSlash: a 5xx writes a retry/connectivity hint, not a bare message", async () => { + const real = globalThis.fetch; + stubFetch(() => jsonRes(503, { detail: "unavailable" })); + let out = ""; + const writable = { write: (chunk: unknown) => { out += String(chunk); return true; } } as unknown as Writable; + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + await photogenSlash(fakeCtx(api), writable, "a cat wearing a hat", false); + assert.match(out, /retry|doctor/i); + } finally { + globalThis.fetch = real; + cleanupOutputDir(); + } +}); diff --git a/test/vault_vision_401.test.ts b/test/vault_vision_401.test.ts new file mode 100644 index 0000000..13f7033 --- /dev/null +++ b/test/vault_vision_401.test.ts @@ -0,0 +1,358 @@ +// Regression tests for LOOP-01: vault.ts's downloadFile/uploadFile/ +// deleteSpacesFile and vision.ts's downloadMediaFile used to bypass ApiClient +// entirely via raw fetch() + private-member casts (`_bearerToken`/`_baseUrl`/ +// `_authHeaders` in vault.ts, a local `authHeaders()` cast in vision.ts). That +// meant none of them got PR #47's refresh-on-401 retry, and because they threw +// plain `Error` (not `HttpError`) instead of ApiClient.request()'s +// toHttpError(), errorHint()/hintFor() could never classify their failures +// into a 401/402/403 hint either. +// +// These four functions now route through ApiClient's public getBinary()/ +// postForm()/deleteJson() — same fetch-stubbing pattern as test/auth_401.test.ts. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { ApiClient, VAULT_SPACES_DOWNLOAD_PATH, VAULT_SPACES_DELETE_PATH, VAULT_SPACES_UPLOAD_PATH } from "../src/core/transport.js"; +import { StaticTokenStore } from "../src/core/auth.js"; +import { HttpError } from "../src/core/errors.js"; +import { hintFor } from "../src/core/error_hints.js"; +import { uploadFile, downloadFile, deleteSpacesFile } from "../src/core/vault.js"; +import { downloadMediaFile } from "../src/core/vision.js"; + +type Call = { url: string; init: RequestInit }; + +function jsonRes(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { status, headers: { "content-type": "application/json" } }); +} + +function stubFetch(handler: (url: string, init: RequestInit) => Response, calls: Call[]): void { + globalThis.fetch = (async (url: unknown, init?: RequestInit) => { + const u = String(url); + calls.push({ url: u, init: init ?? {} }); + return handler(u, init ?? {}); + }) as typeof globalThis.fetch; +} + +function bearer(init: RequestInit): string { + return (init.headers as Record)["Authorization"] ?? ""; +} + +function withTempDir(fn: (dir: string) => Promise): Promise { + const dir = mkdtempSync(join(tmpdir(), "aether-vault-vision-401-")); + return fn(dir).finally(() => rmSync(dir, { recursive: true, force: true })); +} + +// ── uploadFile ──────────────────────────────────────────────────────────── + +test("uploadFile: 401 with a session token triggers one /auth/refresh then a retry that succeeds", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const calls: Call[] = []; + const filePath = join(dir, "hello.txt"); + writeFileSync(filePath, "upload me"); + const store = new StaticTokenStore("sess_expired"); + stubFetch((url, init) => { + if (url.endsWith("/auth/refresh")) return jsonRes(200, { session_token: "sess_fresh" }); + if (bearer(init) === "Bearer sess_fresh") { + assert.ok(init.body instanceof FormData, "upload body must be multipart FormData"); + return jsonRes(200, { key: "k1", filename: "hello.txt", size: 9, content_type: "text/plain" }); + } + return jsonRes(401, { detail: "token expired" }); + }, calls); + try { + const api = new ApiClient("https://api.example", store); + const out = await uploadFile(api, filePath); + assert.deepEqual(out, { key: "k1", filename: "hello.txt", size: 9, content_type: "text/plain" }); + const urls = calls.map((c) => c.url.replace("https://api.example", "")); + assert.deepEqual(urls, [VAULT_SPACES_UPLOAD_PATH, "/auth/refresh", VAULT_SPACES_UPLOAD_PATH]); + } finally { + globalThis.fetch = real; + } + })); + +// LOOP-01 round 2: uploadFile() used to fs.readFileSync() the ENTIRE local +// file into a Buffer and then copy it a second time into an in-memory +// Blob([data]) before any bytes went over the wire — an unbounded-memory gap +// downloadFile()'s stream-to-disk fix (1d33357) never addressed on the upload +// side. It now hands FormData an fs.openAsBlob() Blob backed by the open file +// handle itself, so the file is never buffered whole in JS-land. +// +// That's hard to observe directly (dynamic `import("node:fs")` snapshots its +// named exports, so spying on fs.readFileSync doesn't see calls made through +// a separately-obtained import binding). Instead this pins the OBSERVABLE +// consequence of no-longer-buffering: fs.openAsBlob() re-validates the file +// on disk at read time and throws if it's gone, whereas the old +// readFileSync()+new Blob([data]) copy would happily still contain the bytes +// even after the source file vanished. So: delete the source file out from +// under the (mock) in-flight request and assert the Blob read fails — proof +// the upload body is still tied to the file, not an already-buffered copy. +test("uploadFile: the multipart body is a file-backed Blob (fs.openAsBlob), not an already-buffered copy — reading it after the source file is deleted fails", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const calls: Call[] = []; + const filePath = join(dir, "ephemeral.txt"); + const content = "vault-upload-content"; + writeFileSync(filePath, content); + + let blobReadFailed: unknown; + globalThis.fetch = (async (url: unknown, init?: RequestInit) => { + calls.push({ url: String(url), init: init ?? {} }); + const form = init?.body as FormData; + const file = form.get("file") as Blob; + // Simulate the source file disappearing while the upload is mid-flight + // — a fully-buffered Buffer/Blob copy would be immune to this; a + // genuine file-handle-backed Blob is not. + rmSync(filePath, { force: true }); + try { + await file.text(); + } catch (e) { + blobReadFailed = e; + } + return jsonRes(200, { key: "k1", filename: "ephemeral.txt", size: content.length, content_type: "text/plain" }); + }) as typeof globalThis.fetch; + + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_1")); + const out = await uploadFile(api, filePath); + assert.deepEqual(out, { key: "k1", filename: "ephemeral.txt", size: content.length, content_type: "text/plain" }); + assert.ok( + blobReadFailed, + "the Blob handed to FormData must still be tied to the on-disk file (fs.openAsBlob) — if " + + "uploadFile ever reverts to readFileSync()+new Blob([data]), this read would keep succeeding " + + "even after the source file was deleted, because the whole file would already be buffered in memory", + ); + } finally { + globalThis.fetch = real; + } + })); + +// LOOP-01 round 3: uploadFile() used to rely on postForm()'s bare default +// (AETHER_REQUEST_TIMEOUT_MS, 30s) to bound the WHOLE request — connect + +// send-body + wait-for-response — with no split between a connect-phase +// timeout and an idle/quiet-period timeout on the body the way getBinary()/ +// stream() have. Any upload whose transfer legitimately took longer than 30s +// (a large file, a slow connection) always failed with RequestTimeoutError +// even while data was actively flowing. uploadFile() now explicitly passes +// defaultStreamTimeoutMs() (120s, AETHER_STREAM_TIMEOUT_MS) to postForm() +// instead of letting it fall back to the 30s metadata-call default. +// +// Set AETHER_REQUEST_TIMEOUT_MS very low (what the OLD code would have used, +// since it never overrode postForm's default) and AETHER_STREAM_TIMEOUT_MS +// comfortably high, then have the stubbed fetch take longer than the request +// bound but well within the stream bound. Pre-fix code races the fetch against +// the 30s-default-turned-10ms bound and times out; post-fix code races it +// against the 120s-default-turned-5s bound and succeeds — a fast, deterministic +// discriminator that would fail against the old call site and pass against the +// new one. +test("uploadFile: opts into the stream-class timeout (defaultStreamTimeoutMs), not the 30s metadata-call default", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const prevReqTimeout = process.env["AETHER_REQUEST_TIMEOUT_MS"]; + const prevStreamTimeout = process.env["AETHER_STREAM_TIMEOUT_MS"]; + process.env["AETHER_REQUEST_TIMEOUT_MS"] = "10"; // what the old (un-overridden) default would be + process.env["AETHER_STREAM_TIMEOUT_MS"] = "5000"; // the bound uploadFile() should actually get + const filePath = join(dir, "slow.txt"); + const content = "vault-upload-content"; + writeFileSync(filePath, content); + globalThis.fetch = (async () => { + // Longer than the (old) 10ms request-default, comfortably inside the + // 5000ms stream-default — proves which bound was actually applied. + await new Promise((r) => setTimeout(r, 40)); + return jsonRes(200, { key: "k1", filename: "slow.txt", size: content.length, content_type: "text/plain" }); + }) as typeof globalThis.fetch; + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_1")); + const out = await uploadFile(api, filePath); + assert.deepEqual(out, { key: "k1", filename: "slow.txt", size: content.length, content_type: "text/plain" }); + } finally { + globalThis.fetch = real; + if (prevReqTimeout === undefined) delete process.env["AETHER_REQUEST_TIMEOUT_MS"]; + else process.env["AETHER_REQUEST_TIMEOUT_MS"] = prevReqTimeout; + if (prevStreamTimeout === undefined) delete process.env["AETHER_STREAM_TIMEOUT_MS"]; + else process.env["AETHER_STREAM_TIMEOUT_MS"] = prevStreamTimeout; + } + })); + +// A plain correctness check alongside the above: normal (non-adversarial) +// uploads still carry the exact file bytes end to end. +test("uploadFile: uploads the exact file content via the file-backed Blob", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const calls: Call[] = []; + const filePath = join(dir, "big.txt"); + const content = "vault-upload-content-".repeat(500); // several KB, not just a few bytes + writeFileSync(filePath, content); + stubFetch(() => jsonRes(200, { key: "k1", filename: "big.txt", size: content.length, content_type: "text/plain" }), calls); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("sess_1")); + const out = await uploadFile(api, filePath); + assert.deepEqual(out, { key: "k1", filename: "big.txt", size: content.length, content_type: "text/plain" }); + const body = calls[0]!.init.body; + assert.ok(body instanceof FormData, "upload body must still be multipart FormData"); + const file = body.get("file"); + assert.ok(file instanceof Blob, "the file part must still be a Blob-like object"); + assert.equal(await (file as Blob).text(), content, "the file-backed Blob still carries the exact file content"); + } finally { + globalThis.fetch = real; + } + })); + +// ── downloadFile ────────────────────────────────────────────────────────── + +test("downloadFile: 401 triggers refresh + retry, and streams the body to disk", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const calls: Call[] = []; + const store = new StaticTokenStore("sess_expired"); + const outputPath = join(dir, "out.bin"); + stubFetch((url, init) => { + if (url.endsWith("/auth/refresh")) return jsonRes(200, { session_token: "sess_fresh" }); + if (bearer(init) === "Bearer sess_fresh") return new Response("binary-content-xyz"); + return jsonRes(401, {}); + }, calls); + try { + const api = new ApiClient("https://api.example", store); + const saved = await downloadFile(api, "myfile.bin", outputPath); + assert.equal(saved, outputPath); + assert.equal(readFileSync(outputPath, "utf-8"), "binary-content-xyz"); + const path = VAULT_SPACES_DOWNLOAD_PATH + "/myfile.bin"; + const urls = calls.map((c) => c.url.replace("https://api.example", "")); + assert.deepEqual(urls, [path, "/auth/refresh", path]); + } finally { + globalThis.fetch = real; + } + })); + +// ── deleteSpacesFile ────────────────────────────────────────────────────── + +test("deleteSpacesFile: 401 triggers refresh + retry that succeeds", async () => { + const real = globalThis.fetch; + const calls: Call[] = []; + const store = new StaticTokenStore("sess_expired"); + stubFetch((url, init) => { + if (url.endsWith("/auth/refresh")) return jsonRes(200, { session_token: "sess_fresh" }); + if (bearer(init) === "Bearer sess_fresh") { + assert.equal(init.method, "DELETE"); + return jsonRes(200, { success: true, deleted: "myfile.bin" }); + } + return jsonRes(401, {}); + }, calls); + try { + const api = new ApiClient("https://api.example", store); + const out = await deleteSpacesFile(api, "myfile.bin"); + assert.deepEqual(out, { success: true, deleted: "myfile.bin" }); + const path = VAULT_SPACES_DELETE_PATH + "/myfile.bin"; + const urls = calls.map((c) => c.url.replace("https://api.example", "")); + assert.deepEqual(urls, [path, "/auth/refresh", path]); + } finally { + globalThis.fetch = real; + } +}); + +test("deleteSpacesFile: a non-retryable 401 (aek_ key) surfaces as HttpError, and hintFor gives the auth-login hint", async () => { + const real = globalThis.fetch; + stubFetch(() => jsonRes(401, { detail: "expired" }), []); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_key")); + let caught: unknown; + await assert.rejects( + () => deleteSpacesFile(api, "myfile.bin"), + (e: unknown) => { + caught = e; + return e instanceof HttpError && e.status === 401; + }, + ); + assert.match(hintFor(caught) ?? "", /aether auth login/); + } finally { + globalThis.fetch = real; + } +}); + +// ── downloadMediaFile ───────────────────────────────────────────────────── + +test("downloadMediaFile: 401 triggers refresh + retry, and streams the body to disk (media host is the SAME origin as the API)", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const calls: Call[] = []; + const store = new StaticTokenStore("sess_expired"); + // Same origin as the ApiClient's baseUrl ("https://api.example") so the + // bearer is attached and the refresh-on-401 retry applies — the + // cross-origin case (no bearer attached at all, no refresh) is covered + // separately below and in test/transport_getbinary.test.ts. + const mediaUrl = "https://api.example/output/abc.png"; + stubFetch((url, init) => { + if (url.endsWith("/auth/refresh")) return jsonRes(200, { session_token: "sess_fresh" }); + if (url === mediaUrl && bearer(init) === "Bearer sess_fresh") return new Response("PNGDATA"); + return jsonRes(401, {}); + }, calls); + try { + const api = new ApiClient("https://api.example", store); + const savedPath = await downloadMediaFile(api, mediaUrl, dir, "vision_nano_pro", "image", "myimage.png"); + assert.equal(savedPath, join(dir, "myimage.png")); + assert.equal(readFileSync(savedPath, "utf-8"), "PNGDATA"); + const urls = calls.map((c) => c.url.replace("https://api.example", "")); + assert.deepEqual(urls, [mediaUrl.replace("https://api.example", ""), "/auth/refresh", mediaUrl.replace("https://api.example", "")]); + } finally { + globalThis.fetch = real; + } + })); + +// LOOP-01 round 2 (HIGH): isCredentialSafeUrl() only checked scheme, so a +// cross-origin (or even a same-scheme-but-different-host) media_url used to +// get the live session bearer attached anyway. It's now gated on +// same-origin-as-baseUrl instead, and a cross-origin target is fetched +// UNAUTHENTICATED rather than failing the whole download closed. +test("downloadMediaFile: does NOT attach the bearer to a cross-origin https media host, and still succeeds unauthenticated", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const calls: Call[] = []; + const mediaUrl = "https://media.example/output/abc.png"; // different host than https://api.example + stubFetch((url, init) => { + assert.equal(bearer(init), "", "no Authorization header should be sent to a cross-origin host"); + return url === mediaUrl ? new Response("PNGDATA") : jsonRes(404, {}); + }, calls); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_live_session_token")); + const savedPath = await downloadMediaFile(api, mediaUrl, dir, "vision_nano_pro", "image", "myimage.png"); + assert.equal(readFileSync(savedPath, "utf-8"), "PNGDATA"); + assert.equal(calls.length, 1, "no refresh attempted — no token was ever sent, so a 401 can't occur here"); + } finally { + globalThis.fetch = real; + } + })); + +test("downloadMediaFile: a cross-origin media host is fetched unauthenticated even over PLAIN http (non-loopback) — no InsecureTransportError, no credential sent", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const calls: Call[] = []; + stubFetch((_url, init) => { + assert.equal(bearer(init), "", "no Authorization header should ever reach a foreign http host"); + return new Response("PNGDATA"); + }, calls); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const savedPath = await downloadMediaFile(api, "http://evil.example.com/media.png", dir, "vision_nano_pro", "image", "m.png"); + assert.equal(readFileSync(savedPath, "utf-8"), "PNGDATA"); + assert.equal(calls.length, 1); + } finally { + globalThis.fetch = real; + } + })); + +test("downloadMediaFile: SAME-origin download still fails closed when the API's own baseUrl itself is an insecure (non-loopback http) transport", async () => { + const real = globalThis.fetch; + const calls: Call[] = []; + stubFetch(() => { throw new Error("must not fetch — should fail closed before any network call"); }, calls); + try { + const api = new ApiClient("http://not-localhost.example", new StaticTokenStore("aek_test")); + await assert.rejects( + () => downloadMediaFile(api, "http://not-localhost.example/media.png", "/tmp", "vision_nano_pro", "image"), + /insecure transport/, + ); + assert.equal(calls.length, 0, "no fetch attempted once the guard fires"); + } finally { + globalThis.fetch = real; + } +}); diff --git a/test/workflow_error_hints.test.ts b/test/workflow_error_hints.test.ts new file mode 100644 index 0000000..d293500 --- /dev/null +++ b/test/workflow_error_hints.test.ts @@ -0,0 +1,99 @@ +// Regression tests for LOOP-01 round 3 (workflow.ts:389-391). +// +// workflow.ts's own local fail() helper hardcoded "are you logged in? run: +// aether auth login" as the hint for EVERY error type across all 11 of its +// call sites (workflowList/View/Delete/Save/Export/Import/Assess/Brainstorm/ +// Plan/Finalize/Status) — the exact anti-pattern already eliminated from +// vault.ts's and media.ts's fail() helpers in LOOP-01 round 2 (see +// vault_media_error_hints.test.ts). A 402 (out-of-balance), 403 +// (tier-locked), or 5xx from any of those subcommands showed the misleading +// "are you logged in?" hint even though the user IS logged in fine. +// +// These tests confirm cmdWorkflow's list/view/delete/save subcommands now +// route through hintFor() (matching workflowNew's stream-turn catch, which +// was already correct — see workflow.ts:107) and render the distinct, +// already-implemented hint from core/error_hints.ts for each status. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { ApiClient } from "../src/core/transport.js"; +import { StaticTokenStore } from "../src/core/auth.js"; +import type { AppContext } from "../src/core/context.js"; +import { cmdWorkflow } from "../src/commands/workflow.js"; + +function jsonRes(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { status, headers: { "content-type": "application/json" } }); +} + +function stubFetch(handler: (url: string) => Response): void { + globalThis.fetch = (async (url: unknown) => handler(String(url))) as typeof globalThis.fetch; +} + +function fakeCtx(api: ApiClient): AppContext { + return { + cfg: { baseUrl: "https://api.example" }, + api, + tokens: new StaticTokenStore("aek_test"), + flags: { cwd: process.cwd(), json: false, audit: false, yes: false }, + confirm: async () => true, + } as unknown as AppContext; +} + +function captureWrite(stream: NodeJS.WriteStream): { get: () => string; restore: () => void } { + const orig = stream.write.bind(stream); + let buf = ""; + stream.write = ((chunk: unknown): boolean => { + buf += String(chunk); + return true; + }) as typeof stream.write; + return { get: () => buf, restore: () => { stream.write = orig; } }; +} + +async function runWithStatus(argv: string[], status: number, body: unknown): Promise<{ code: number; err: string }> { + const real = globalThis.fetch; + stubFetch(() => jsonRes(status, body)); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdWorkflow(fakeCtx(api), argv); + return { code, err: cap.get() }; + } finally { + cap.restore(); + globalThis.fetch = real; + } +} + +const SUBCOMMANDS: Array<{ label: string; argv: string[] }> = [ + { label: "list", argv: ["list"] }, + { label: "view", argv: ["view", "my-workflow"] }, + { label: "delete", argv: ["delete", "my-workflow"] }, + { label: "save", argv: ["save", "my-workflow"] }, +]; + +for (const { label, argv } of SUBCOMMANDS) { + test(`cmdWorkflow ${label}: a 402 (out of UVT balance) shows the balance hint, not 'are you logged in?'`, async () => { + const { code, err } = await runWithStatus(argv, 402, { detail: "insufficient balance" }); + assert.equal(code, 1); + assert.match(err, /UVT|balance/i); + assert.doesNotMatch(err, /are you logged in/); + }); + + test(`cmdWorkflow ${label}: a 403 (plan/tier) shows the plan/tier hint, not 'are you logged in?'`, async () => { + const { code, err } = await runWithStatus(argv, 403, { detail: "forbidden" }); + assert.equal(code, 1); + assert.match(err, /plan|tier/i); + assert.doesNotMatch(err, /are you logged in/); + }); + + test(`cmdWorkflow ${label}: a 5xx shows a retry/connectivity hint, not the auth-login hint`, async () => { + const { code, err } = await runWithStatus(argv, 500, { detail: "server error" }); + assert.equal(code, 1); + assert.match(err, /retry|doctor/i); + assert.doesNotMatch(err, /are you logged in/); + }); + + test(`cmdWorkflow ${label}: a 401 still shows the auth-login hint (unchanged for this status)`, async () => { + const { code, err } = await runWithStatus(argv, 401, { detail: "expired" }); + assert.equal(code, 1); + assert.match(err, /aether auth login/); + }); +} diff --git a/test/workflow_vault_error_propagation.test.ts b/test/workflow_vault_error_propagation.test.ts new file mode 100644 index 0000000..3ac8507 --- /dev/null +++ b/test/workflow_vault_error_propagation.test.ts @@ -0,0 +1,168 @@ +// Regression tests for LOOP-01 round 3 (src/core/workflow.ts:322-351). +// +// listWorkflows/getWorkflow/deleteWorkflow used to wrap listSpaces/ +// getSpacesContent/deleteSpacesFile (vault.ts) in a catch-all that turned +// EVERY failure -- an expired session (401), a network outage, or a 5xx -- +// into an empty list / null / false. The command layer (cmdWorkflow) then +// reported that as a legitimate "no data" state: "No workflows found in +// vault.", "workflow not found: ", "delete failed -- workflow may not +// exist" -- with zero indication to run `aether auth login`. +// +// listSpaces/getSpacesContent/deleteSpacesFile never throw for a genuinely +// empty vault or a file with no content (they resolve with `files: []` / +// `content: null`) -- so a thrown error from them is always a REAL failure. +// The fix removes the swallowing catches so it propagates to cmdWorkflow's +// own try/catch (which already calls fail(err)). +// +// These tests assert: +// 1. A 401 from list/view/delete surfaces "aether auth login" via fail(), +// not the old ambiguous "no data" messages. +// 2. A genuinely empty vault (200, `files: []`) still correctly prints +// "No workflows found in vault." -- confirming the fix didn't turn the +// legitimate empty case into a false failure. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + ApiClient, + VAULT_SPACES_LIST_PATH, + VAULT_SPACES_CONTENT_PATH, + VAULT_SPACES_DELETE_PATH, +} from "../src/core/transport.js"; +import { StaticTokenStore } from "../src/core/auth.js"; +import type { AppContext } from "../src/core/context.js"; +import { cmdWorkflow } from "../src/commands/workflow.js"; + +function jsonRes(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { status, headers: { "content-type": "application/json" } }); +} + +function stubFetch(handler: (url: string) => Response): void { + globalThis.fetch = (async (url: unknown) => handler(String(url))) as typeof globalThis.fetch; +} + +function fakeCtx(api: ApiClient): AppContext { + return { + cfg: { baseUrl: "https://api.example" }, + api, + tokens: new StaticTokenStore("aek_test"), + flags: { cwd: process.cwd(), json: false, audit: false, yes: false }, + confirm: async () => true, + } as unknown as AppContext; +} + +function captureWrite(stream: NodeJS.WriteStream): { get: () => string; restore: () => void } { + const orig = stream.write.bind(stream); + let buf = ""; + stream.write = ((chunk: unknown): boolean => { + buf += String(chunk); + return true; + }) as typeof stream.write; + return { get: () => buf, restore: () => { stream.write = orig; } }; +} + +// ── listWorkflows / workflowList ───────────────────────────────────────── + +test("cmdWorkflow list: a 401 from listSpaces surfaces 'aether auth login', not 'No workflows found'", async () => { + const real = globalThis.fetch; + stubFetch((url) => (url.includes(VAULT_SPACES_LIST_PATH) ? jsonRes(401, { detail: "expired" }) : jsonRes(404, {}))); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdWorkflow(fakeCtx(api), ["list"]); + assert.equal(code, 1); + assert.match(cap.get(), /aether auth login/); + assert.doesNotMatch(cap.get(), /No workflows found/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +test("cmdWorkflow list: a genuinely empty vault (200, files: []) still prints 'No workflows found in vault.'", async () => { + const real = globalThis.fetch; + stubFetch((url) => (url.includes(VAULT_SPACES_LIST_PATH) ? jsonRes(200, { success: true, files: [], count: 0 }) : jsonRes(404, {}))); + const cap = captureWrite(process.stdout); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdWorkflow(fakeCtx(api), ["list"]); + assert.equal(code, 0); + assert.match(cap.get(), /No workflows found in vault\./); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +// ── getWorkflow / workflowView ─────────────────────────────────────────── + +test("cmdWorkflow view: a 401 from getSpacesContent surfaces 'aether auth login', not 'workflow not found'", async () => { + const real = globalThis.fetch; + stubFetch((url) => (url.includes(VAULT_SPACES_CONTENT_PATH) ? jsonRes(401, { detail: "expired" }) : jsonRes(404, {}))); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdWorkflow(fakeCtx(api), ["view", "my-flow"]); + assert.equal(code, 1); + assert.match(cap.get(), /aether auth login/); + assert.doesNotMatch(cap.get(), /workflow not found/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +// ── deleteWorkflow / workflowDelete ────────────────────────────────────── + +test("cmdWorkflow delete: a 401 from deleteSpacesFile surfaces 'aether auth login', not 'delete failed'", async () => { + const real = globalThis.fetch; + stubFetch((url) => (url.includes(VAULT_SPACES_DELETE_PATH) ? jsonRes(401, { detail: "expired" }) : jsonRes(404, {}))); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdWorkflow(fakeCtx(api), ["delete", "my-flow"]); + assert.equal(code, 1); + assert.match(cap.get(), /aether auth login/); + assert.doesNotMatch(cap.get(), /delete failed/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +// ── getWorkflow / workflowSave ──────────────────────────────────────────── +// workflowSave's `else if (name)` branch calls getWorkflow BEFORE its own +// try block starts (the try only wraps the later saveWorkflow call) — a real +// failure here must be caught locally or it escapes cmdWorkflow entirely and +// hits main.ts's bare top-level catch (no hint at all), a regression this +// asserts against directly. +test("cmdWorkflow save : a 401 from getSpacesContent surfaces 'aether auth login' via fail(), not a bare unhinted error", async () => { + const real = globalThis.fetch; + stubFetch((url) => (url.includes(VAULT_SPACES_CONTENT_PATH) ? jsonRes(401, { detail: "expired" }) : jsonRes(404, {}))); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdWorkflow(fakeCtx(api), ["save", "my-flow"]); + assert.equal(code, 1); + assert.match(cap.get(), /aether auth login/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +}); + +// ── 5xx / network outage also propagate instead of masquerading as "no data" ── + +test("cmdWorkflow list: a 500 from listSpaces surfaces via fail(), not 'No workflows found'", async () => { + const real = globalThis.fetch; + stubFetch((url) => (url.includes(VAULT_SPACES_LIST_PATH) ? jsonRes(500, { detail: "server error" }) : jsonRes(404, {}))); + const cap = captureWrite(process.stderr); + try { + const api = new ApiClient("https://api.example", new StaticTokenStore("aek_test")); + const code = await cmdWorkflow(fakeCtx(api), ["list"]); + assert.equal(code, 1); + assert.doesNotMatch(cap.get(), /No workflows found/); + } finally { + cap.restore(); + globalThis.fetch = real; + } +});