From 1d333570e7306a095fb0cc817c6556c5aa4fcbdd Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 14:26:24 -0400 Subject: [PATCH 01/27] loop(LOOP-01): route vault/vision auth through ApiClient, not raw fetch() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit downloadSpacesFile/uploadFile/deleteSpacesFile (vault.ts) and downloadMediaFile (vision.ts) issued their own raw fetch() calls via private-member casts into ApiClient's tokens/baseUrl/authHeaders, so they never got PR #47's refresh-on-401 retry or its distinct 401/402/403 hint UX (they threw plain Error, not HttpError, so errorHint()/hintFor() could never classify them). Add two public authed methods to ApiClient — getBinary() (streaming GET, accepts a relative path or an absolute media URL) and postForm() (authed multipart POST) — both going through the same refresh-on-401 retry and toHttpError() classification as request()/stream(). deleteSpacesFile now uses the existing deleteJson(). Delete vault.ts's _bearerToken/_baseUrl/ _authHeaders and vision.ts's authHeaders() cast entirely — no file outside transport.ts needs to know ApiClient's internal field names anymore. Also fold downloadSpacesFile's res.arrayBuffer() into downloadFile() and stream the response body straight to disk instead of buffering the whole file in memory (mirrors vision.ts's existing pipeline() pattern). Add test/vault_vision_401.test.ts mirroring auth_401.test.ts's fetch-stubbing pattern: 401-then-refresh-then-retry for all four functions, HttpError classification + hintFor 401 wording for deleteSpacesFile, and a regression check that downloadMediaFile still fails closed on an insecure (non-loopback http) media host. Co-Authored-By: Claude Sonnet 5 --- src/core/transport.ts | 64 +++++++++++- src/core/vault.ts | 81 ++++----------- src/core/vision.ts | 24 ++--- test/vault_vision_401.test.ts | 185 ++++++++++++++++++++++++++++++++++ 4 files changed, 274 insertions(+), 80 deletions(-) create mode 100644 test/vault_vision_401.test.ts diff --git a/src/core/transport.ts b/src/core/transport.ts index 7203679..9e1903f 100644 --- a/src/core/transport.ts +++ b/src/core/transport.ts @@ -164,7 +164,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> { @@ -269,6 +268,69 @@ export class ApiClient { return this.request("DELETE", path, { signal }); } + /** + * 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. When a + * bearer token would be attached, that target host must ALSO be a + * credential-safe transport — the API's own baseUrl being https doesn't + * vouch for some other (possibly cleartext) host the token would otherwise + * leak to. + */ + async getBinary(pathOrUrl: string, signal?: AbortSignal): Promise { + const target = /^https?:\/\//i.test(pathOrUrl) ? pathOrUrl : this.url(pathOrUrl); + let used: string | null = null; + const send = async (): Promise => { + used = await this.tokens.get(); + const headers = await this.authHeaders(used); + if ("Authorization" in headers && !isCredentialSafeUrl(target)) { + throw new InsecureTransportError(target); + } + return fetch(target, { headers, ...(signal ? { signal } : {}) }); + }; + 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); + return res; + } + + /** + * 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. + */ + async postForm(path: string, form: FormData, signal?: AbortSignal): Promise { + let used: string | null = null; + const send = async (): Promise => { + used = await this.tokens.get(); + return fetch(this.url(path), { + method: "POST", + headers: await this.authHeaders(used), + body: form, + ...(signal ? { signal } : {}), + }); + }; + 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); + try { + return (await res.json()) as T; + } catch { + return undefined as T; + } + } + private async request( method: string, path: string, diff --git a/src/core/vault.ts b/src/core/vault.ts index 40a71ad..5d62832 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 } 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,16 +111,6 @@ 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. */ export async function uploadFile( api: ApiClient, filePath: string, @@ -157,25 +121,24 @@ export async function uploadFile( const filename = path.basename(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. */ + return api.postForm(VAULT_SPACES_UPLOAD_PATH, formData); +} + +/** + * 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 +151,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..ce21208 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 } 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"; @@ -210,12 +209,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 +220,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; diff --git a/test/vault_vision_401.test.ts b/test/vault_vision_401.test.ts new file mode 100644 index 0000000..afb1367 --- /dev/null +++ b/test/vault_vision_401.test.ts @@ -0,0 +1,185 @@ +// 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; + } + })); + +// ── 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", () => + withTempDir(async (dir) => { + const real = globalThis.fetch; + const calls: Call[] = []; + const store = new StaticTokenStore("sess_expired"); + const mediaUrl = "https://media.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, "/auth/refresh", mediaUrl]); + } finally { + globalThis.fetch = real; + } + })); + +test("downloadMediaFile: refuses to attach the bearer over an insecure (non-loopback http) media host", 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("https://api.example", new StaticTokenStore("aek_test")); + await assert.rejects( + () => downloadMediaFile(api, "http://evil.example.com/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; + } +}); From 7cf7192fafa80a37a018833ed860692857102cab Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 14:38:33 -0400 Subject: [PATCH 02/27] loop(LOOP-01,LOOP-06): stop masking device-poll network outages as authorization_pending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pollForToken's catch block folded ANY thrown error into authorization_pending, including raw network failures (ECONNREFUSED, DNS, timeout) with no HTTP body — an outage during `aether auth login` read as silent normal polling for the whole expires_in window. Now only an HttpError carrying a real body (the documented 400 pending/slow_down/expired case) maps to ordinary poll state. Any other error counts as a network failure; after 3 in a row it prints a distinct "can't reach the server" warning, and if the deadline is hit while still failing it throws a distinct timeout message instead of the generic "login timed out — run `aether auth login` again" (which reads as "too slow"). Adds device.test.ts coverage for: a transient blip that still succeeds with no warning, a sustained outage that fails with the distinct message, and genuine authorization_pending polling to deadline with the original message. --- src/core/device.ts | 36 ++++++++++- test/device.test.ts | 142 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 175 insertions(+), 3 deletions(-) diff --git a/src/core/device.ts b/src/core/device.ts index 12db174..b5211f1 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,33 @@ 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" }; + // expired; 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) 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.body && typeof err.body === "object") { + 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 +96,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/test/device.test.ts b/test/device.test.ts index 77de747..f4d18f2 100644 --- a/test/device.test.ts +++ b/test/device.test.ts @@ -1,6 +1,8 @@ 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"; test("classifyPoll maps server responses to poll actions", () => { assert.equal(classifyPoll({ error: "authorization_pending" }), "wait"); @@ -10,3 +12,141 @@ 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(); + } +}); From 1f70f6e942e5836022f798589db48f7909148175 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 14:55:01 -0400 Subject: [PATCH 03/27] loop(LOOP-06): distinguish a rejected session from a network outage in `aether auth status` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit renderAuthBox's /models probe folded a 401/403 (server actively rejected the stored token — expired or revoked session, the stale-AETHER_TOKEN case PR #47 fixed elsewhere) into the same bare catch used for a genuine network outage. Either way the panel printed "Authenticated" with Token/API rows, telling the user they're signed in when the server just said otherwise — `status` had no error state at all for this case. Now an HttpError with status 401 or 403 renders a distinct "Session expired" panel (swapped header/icon, a "sign in again" hint instead of Tier/Default), title included. Any other failure (no HttpError, or a different status, including 5xx) keeps the original silent local-only fallback — an outage or a plan-only 429/etc. is not proof the session itself is dead. Deliberately unchanged: cmdAuth's `status` exit code stays 0 in the expired case (the finding asked for a distinct rendered state, not an exit-code contract — changing that would need renderAuthBox to leak expired-ness out to its caller, which is out of scope here). Extends test/auth_401.test.ts (the repo's fetch-stubbing pattern) with four renderAuthBox cases: 401 -> Session expired, 403 -> Session expired, a real network throw -> still Authenticated (no regression), and a normal 200 -> Authenticated with tier/default. Co-Authored-By: Claude Sonnet 5 --- src/commands/auth.ts | 48 ++++++++++++++++++------ test/auth_401.test.ts | 85 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 121 insertions(+), 12 deletions(-) diff --git a/src/commands/auth.ts b/src/commands/auth.ts index ed2bfe7..d02d8c1 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -9,6 +9,7 @@ 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 } from "../core/errors.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"; @@ -35,37 +36,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 +103,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 ── diff --git a/test/auth_401.test.ts b/test/auth_401.test.ts index 0358751..3b8d99b 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,11 @@ // + 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. import { test } from "node:test"; import assert from "node:assert/strict"; import { rmSync } from "node:fs"; @@ -16,6 +21,9 @@ 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 } from "../src/commands/auth.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 +255,78 @@ 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; + } +}); From a7b362108994ee659bc95911f5dfb486246ff747 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 15:39:11 -0400 Subject: [PATCH 04/27] loop(LOOP-01,LOOP-06): bound ApiClient.request() by default, throw on malformed 2xx bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root fix (transport.ts): ApiClient.request() (backing getJson/postJson/ deleteJson) 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; 0 disables it), using the same raceAgainst/net-controller pattern stream() already uses so a timeout releases the socket without ever being confused with a caller's own abort. Also fixed a latent bug this surfaced: raceAgainst's early-return for an already-aborted signal discarded its `promise` argument without a catch, which could leak an unhandled rejection — now swallowed, closing the same gap in stream()'s pre-existing use of raceAgainst too. Also: a 2xx response whose body fails to parse as JSON used to silently become `undefined as T`, so callers that trust T (slash.ts's getCatalog -> `cat.models`, auth.ts's authRefresh -> `r.session_token`) crashed one layer up with a raw property-access TypeError. request()/postForm() now throw a new MalformedResponseError (extends HttpError) instead, wired into errorHint()/hintFor() for a normal, hinted message. A genuinely EMPTY body (204 No Content, etc.) is unaffected — classified by reading the text first, not by sniffing JSON.parse's error message, so a body truncated mid-object by a dropped connection is correctly treated as malformed, not empty. loginWithPassword() (the headless --username/--password flow, a raw fetch with no ApiClient behind it) gets its own bounded default via the same env var, guarded so an explicit 0 doesn't become an instant AbortSignal.timeout(0). Regression-prevention carve-outs (required by the new default, not scope creep): a flat 30s would kill any non-streaming call that legitimately runs long. postJson/getJson/deleteJson gained an optional per-call timeoutMs override for exactly this. Applied it to: chat.ts's/brain_cloud.ts's/ client.ts's CHAT_PATH non-streaming chat fallback (a full LLM turn), and bench.ts/test_drive.ts/workflow.ts's project-conversion calls (block server-side on completed generation/iteration work, not a job handle) — each now opts into stream()'s own defaultStreamTimeoutMs() instead. Left orchestrator.ts's delegate/broadcast/gather and audit.ts's exportProof at the 30s default — those return a job handle or a fast confirmation, not completed heavy work. Tests: test/transport_request.test.ts (new — default timeout, per-call override in both directions, malformed-vs-empty-vs-truncated body classification, errorHint/hintFor wiring), test/long_running_timeout_overrides.test.ts (new — bench/test_drive/workflow survive a slow-but-healthy response), test/chat.test.ts (fallback survives past the metadata default), test/device.test.ts (a single stalled poll request times out instead of blocking pollForToken's loop), test/auth.test.ts (loginWithPassword times out / 0 disables it), test/abort.test.ts + test/slash_abort.test.ts (updated for request()'s new net-controller wiring — signal identity is no longer preserved end-to-end, same tradeoff stream() already made; behavioral equivalence asserted instead). Note for follow-up: startTestDrive is bounded at the 120s stream default, but it's an iterate-until-green loop that could plausibly run longer in practice; prior behavior was fully unbounded. Revisit if real TDD loops exceed 120s. Co-Authored-By: Claude Sonnet 5 --- src/commands/chat.ts | 10 +- src/core/auth.ts | 11 +- src/core/bench.ts | 17 +- src/core/brain_cloud.ts | 9 +- src/core/client.ts | 7 +- src/core/error_hints.ts | 15 +- src/core/errors.ts | 35 +++ src/core/test_drive.ts | 19 +- src/core/transport.ts | 189 ++++++++++---- src/core/workflow.ts | 15 +- test/abort.test.ts | 22 +- test/auth.test.ts | 76 +++++- test/chat.test.ts | 39 +++ test/device.test.ts | 44 ++++ test/long_running_timeout_overrides.test.ts | 100 ++++++++ test/slash_abort.test.ts | 22 +- test/transport_request.test.ts | 262 ++++++++++++++++++++ 17 files changed, 807 insertions(+), 85 deletions(-) create mode 100644 test/long_running_timeout_overrides.test.ts create mode 100644 test/transport_request.test.ts diff --git a/src/commands/chat.ts b/src/commands/chat.ts index 0cac1e9..d667b1c 100644 --- a/src/commands/chat.ts +++ b/src/commands/chat.ts @@ -7,7 +7,7 @@ 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"; @@ -161,8 +161,12 @@ async function runCloudTurn( } 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) { diff --git a/src/core/auth.ts b/src/core/auth.ts index 4046e36..bf65bf7 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 } from "./transport.js"; export interface TokenStore { get(): Promise; @@ -179,6 +179,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 +195,7 @@ export async function loginWithPassword( password: creds.password, license_key: creds.licenseKey ?? null, }), + ...(timeoutMs > 0 ? { signal: AbortSignal.timeout(timeoutMs) } : {}), }); let body: LoginResponseBody; try { 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..19bc7a5 100644 --- a/src/core/brain_cloud.ts +++ b/src/core/brain_cloud.ts @@ -13,7 +13,7 @@ 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"; @@ -61,9 +61,12 @@ export class CloudBrain implements Brain { } } 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..4033d48 100644 --- a/src/core/client.ts +++ b/src/core/client.ts @@ -8,7 +8,7 @@ // 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"; @@ -76,7 +76,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/error_hints.ts b/src/core/error_hints.ts index 1f19f33..5a8269b 100644 --- a/src/core/error_hints.ts +++ b/src/core/error_hints.ts @@ -2,11 +2,21 @@ // 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, + MalformedResponseError, + RequestTimeoutError, + StreamTimeoutError, + httpStatusHint, +} from "./errors.js"; /** One-line recovery hint for a thrown error, or null when there's nothing * actionable to add. */ export function hintFor(err: unknown): string | null { + // Checked before the generic HttpError branch (MalformedResponseError + // extends it) — same reasoning as errors.errorHint. + if (err instanceof MalformedResponseError) return "retry, or /doctor to check connectivity"; if (err instanceof HttpError) { 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 @@ -16,6 +26,9 @@ export function hintFor(err: unknown): string | null { if (err instanceof StreamTimeoutError) { return "the stream went quiet - retry, or /doctor to check connectivity"; } + if (err instanceof RequestTimeoutError) { + return "the request 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 diff --git a/src/core/errors.ts b/src/core/errors.ts index ab9ed13..77d4f27 100644 --- a/src/core/errors.ts +++ b/src/core/errors.ts @@ -44,6 +44,19 @@ export class StreamTimeoutError extends Error { } } +/** + * 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,6 +84,22 @@ 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([ @@ -117,6 +146,12 @@ export function httpStatusHint(status: number): string | null { * network or a TTY. Consumed under the ✗ line as a dim hint. */ export function errorHint(err: unknown, baseUrl: string): string | null { + // Checked before the generic HttpError branch below (MalformedResponseError + // extends it): 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. + if (err instanceof MalformedResponseError) return "retry, or /doctor to check connectivity"; + if (err instanceof RequestTimeoutError) return "retry, or /doctor to check connectivity"; 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/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 9e1903f..7d766d2 100644 --- a/src/core/transport.ts +++ b/src/core/transport.ts @@ -3,7 +3,14 @@ // // Paths are constants so they change in one place. -import { HttpError, InsecureTransportError, StreamTimeoutError, StreamUnavailableError } from "./errors.js"; +import { + HttpError, + InsecureTransportError, + MalformedResponseError, + RequestTimeoutError, + StreamTimeoutError, + StreamUnavailableError, +} from "./errors.js"; import type { TokenStore } from "./auth.js"; /** @@ -86,6 +93,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; @@ -256,16 +268,22 @@ 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): Promise { - return this.request("DELETE", path, { signal }); + async deleteJson(path: string, signal?: AbortSignal, timeoutMs?: number): Promise { + return this.request("DELETE", path, { signal, timeoutMs }); } /** @@ -324,51 +342,102 @@ export class ApiClient { res = await send(); } if (!res.ok) throw await toHttpError(res); - try { - return (await res.json()) as T; - } catch { - return undefined as T; - } + return parseOkBody(res); } 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); + } +} + async function toHttpError(res: Response): Promise { let body: unknown; try { @@ -422,19 +491,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?.(); }), ); diff --git a/src/core/workflow.ts b/src/core/workflow.ts index 3a50c51..2e0eb34 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,7 +312,7 @@ 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 ──────────────────── 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..d43043b 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 } 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); @@ -37,3 +66,48 @@ 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(); + } +}); + +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/chat.test.ts b/test/chat.test.ts index d37af4d..f93cbd6 100644 --- a/test/chat.test.ts +++ b/test/chat.test.ts @@ -71,6 +71,45 @@ test("runTurn resolves cleanly on a clean stream", async () => { } }); +// 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/device.test.ts b/test/device.test.ts index f4d18f2..945e43b 100644 --- a/test/device.test.ts +++ b/test/device.test.ts @@ -4,6 +4,18 @@ import { classifyPoll, pollForToken, type DeviceCode } from "../src/core/device. 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"); assert.equal(classifyPoll({ error: "slow_down" }), "slow_down"); @@ -150,3 +162,35 @@ test("pollForToken: real authorization_pending (HttpError with body) times out w 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/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/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_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); +}); From 9d8e04ad70169620e2fef1e6f886278124b8dbf3 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 15:50:56 -0400 Subject: [PATCH 05/27] loop(LOOP-01): unify injected-token TokenStore selection between client.ts and auth.ts AetherClient's constructor and tokenStoreFromEnv() each hand-maintained their own copy of the "injected AETHER_TOKEN -> TokenStore" decision, untested against each other and free to silently diverge. Factor the shared skeleton into auth.ts's new tokenStoreForInjected(injected, {persistOnLogin}), with the one axis that legitimately differs (whether an explicit login persists to disk) passed explicitly at each call site: tokenStoreFromEnv keeps persistOnLogin:true (PR #47's CLI-entry fix), AetherClient keeps persistOnLogin:false (a library embed's login() must not clobber the standalone CLI's on-disk session). Add regression tests pinning both directions so the two paths can't drift again. --- src/core/auth.ts | 38 +++++++++++++++++++++---- src/core/client.ts | 12 +++++--- test/client.test.ts | 58 ++++++++++++++++++++++++++++++++++++++ test/embedded-auth.test.ts | 36 +++++++++++++++++++++++ 4 files changed, 135 insertions(+), 9 deletions(-) diff --git a/src/core/auth.ts b/src/core/auth.ts index bf65bf7..4b2d2d9 100644 --- a/src/core/auth.ts +++ b/src/core/auth.ts @@ -91,6 +91,33 @@ export function defaultTokenStore(): TokenStore { return new FileTokenStore(); } +/** + * 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 +126,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 }); } /** diff --git a/src/core/client.ts b/src/core/client.ts index 4033d48..e2be751 100644 --- a/src/core/client.ts +++ b/src/core/client.ts @@ -13,8 +13,7 @@ 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); } 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/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()); +}); From 1721342b6c9caf063324d5442fe11db22a3f516f Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 16:02:29 -0400 Subject: [PATCH 06/27] loop(LOOP-06): unify printError and Renderer.error's terminal formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract a shared formatErrorLine helper (src/ui/error_line.ts) so a client-caught error (chat.ts's printError) and a mid-stream server-emitted error frame (render.ts's Renderer.error) render the identical "✗ msg" glyph, dim " ⤷ hint" line, and trailing blank-line separator — the same "session expired" moment no longer reads differently depending on which path caught it. --- src/commands/chat.ts | 12 ++-- src/core/render.ts | 13 ++-- src/ui/error_line.ts | 40 ++++++++++++ test/error_line.test.ts | 135 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 186 insertions(+), 14 deletions(-) create mode 100644 src/ui/error_line.ts create mode 100644 test/error_line.test.ts diff --git a/src/commands/chat.ts b/src/commands/chat.ts index d667b1c..09937ac 100644 --- a/src/commands/chat.ts +++ b/src/commands/chat.ts @@ -11,6 +11,7 @@ import { CHAT_STREAM_PATH, CHAT_PATH, defaultStreamTimeoutMs } from "../core/tra import { decodeSse } from "../core/stream.js"; import { Renderer } from "../core/render.js"; import { 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"; @@ -961,11 +962,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/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/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/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"); +}); From c292761cea48f8f51515b06107b450275f59299a Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 16:10:32 -0400 Subject: [PATCH 07/27] loop(LOOP-06): document intentional 5xx/network hint divergence, dedupe isAbortError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit error_hints.ts's header comment implied hintFor and errors.errorHint were unified beyond 401/402/403/429 (the only statuses httpStatusHint actually owns). Their 5xx and network-failure wording was always intentionally separate per-surface (errorHint knows the baseUrl; hintFor's public signature doesn't), so make that explicit in comments instead of leaving it looking like an unfixed bug. Also stop maintaining a second, narrower isAbortError in error_hints.ts (only checked err.name, unlike errors.isAbortError's cause-wrapped/regex checks) — it was never imported anywhere in src/, only exercised by its own test. Re-export errors.isAbortError instead so there's one implementation to keep correct. --- src/core/error_hints.ts | 26 +++++++++++++++++++++----- test/error_hints.test.ts | 18 ++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/core/error_hints.ts b/src/core/error_hints.ts index 5a8269b..e39f859 100644 --- a/src/core/error_hints.ts +++ b/src/core/error_hints.ts @@ -9,15 +9,27 @@ import { RequestTimeoutError, StreamTimeoutError, httpStatusHint, + 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. + * + * Only 401/402/403/429 wording is actually shared with errors.errorHint (via + * httpStatusHint — see its own doc comment). The 5xx and network-failure + * branches below are intentionally 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 { // Checked before the generic HttpError branch (MalformedResponseError // extends it) — same reasoning as errors.errorHint. if (err instanceof MalformedResponseError) return "retry, or /doctor to check connectivity"; 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. @@ -33,6 +45,8 @@ export function hintFor(err: unknown): string | null { if (err instanceof Error) { if (err.name === "AbortError") return null; // user-initiated, already explained const m = err.message; + // Deliberately worded differently from errorHint's network branch (no + // baseUrl to name here) — see the module-level note above. if ( m.includes("fetch failed") || m.includes("ECONNREFUSED") || @@ -46,7 +60,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/test/error_hints.test.ts b/test/error_hints.test.ts index 16c12b5..7d9252b 100644 --- a/test/error_hints.test.ts +++ b/test/error_hints.test.ts @@ -7,11 +7,16 @@ 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/); }); @@ -34,6 +39,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); From 774dc1af506fe13a6a792e7860e2ff859f956fb8 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 17:01:10 -0400 Subject: [PATCH 08/27] loop(LOOP-01): gate getBinary's bearer on same-origin, bound getBinary/postForm by timeout, dedupe aek_ prefix check - transport.ts: isCredentialSafeUrl() only checked scheme (any https host passes), so getBinary() attached the live session bearer to ANY absolute https URL a caller passed -- including a third-party media host completely different from the configured Aether API baseUrl. Gate attachment on the new isSameOrigin(target, baseUrl) instead; a cross-origin target is now fetched unauthenticated rather than leaking the token or failing the whole download closed (deliberate, security-relevant behavior change from the prior fail-closed InsecureTransportError on non-loopback http targets -- see the updated tests in vault_vision_401.test.ts for the new contract). - transport.ts: getBinary()/postForm() had no timeout/AbortSignal.timeout bound at all, unlike request()/stream(), reintroducing the "stalled connection hangs forever" bug a7b3621 fixed for request(). Both now accept a timeoutMs override (default AETHER_REQUEST_TIMEOUT_MS), mirroring postJson/getJson/deleteJson. getBinary additionally wraps its body with an idle/quiet-period timeout (stream()'s withIdleTimeout, re-wrapped as a Web ReadableStream) so a large-but-healthy download isn't killed by a flat overall cap -- only a genuinely quiet connection times out. - core/auth.ts: added the canonical isApiKeyToken() export; transport.ts's refreshSession and commands/auth.ts's isApiToken both now delegate to it instead of separately hand-checking the "aek_" prefix. Regression tests: test/transport_getbinary_postform.test.ts (new -- same- origin gating, cross-origin-no-auth-header, connect timeout, body idle timeout, large-healthy-download-not-killed, caller-abort-still-wins, postForm timeout); test/vault_vision_401.test.ts (updated for the new same-origin contract, cross-origin http/https no-credential-sent cases, and same-origin-insecure-baseUrl still fails closed); test/auth.test.ts (direct isApiKeyToken coverage, not just transitively through isApiToken). npm run build && npm test: 753/753 passing. Co-Authored-By: Claude Sonnet 5 --- src/commands/auth.ts | 8 +- src/core/auth.ts | 14 ++ src/core/transport.ts | 181 +++++++++++++---- test/auth.test.ts | 17 +- test/transport_getbinary_postform.test.ts | 233 ++++++++++++++++++++++ test/vault_vision_401.test.ts | 58 +++++- 6 files changed, 463 insertions(+), 48 deletions(-) create mode 100644 test/transport_getbinary_postform.test.ts diff --git a/src/commands/auth.ts b/src/commands/auth.ts index d02d8c1..0bdbfbb 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -10,13 +10,17 @@ 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 } 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"; -/** 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 { diff --git a/src/core/auth.ts b/src/core/auth.ts index 4b2d2d9..f56cf80 100644 --- a/src/core/auth.ts +++ b/src/core/auth.ts @@ -91,6 +91,20 @@ 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 diff --git a/src/core/transport.ts b/src/core/transport.ts index 7d766d2..b7bcde9 100644 --- a/src/core/transport.ts +++ b/src/core/transport.ts @@ -11,7 +11,7 @@ import { StreamTimeoutError, StreamUnavailableError, } from "./errors.js"; -import type { TokenStore } from "./auth.js"; +import { isApiKeyToken, type TokenStore } from "./auth.js"; /** * Is `base` a transport we will attach the session token to? https is allowed to @@ -32,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 @@ -124,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; @@ -293,30 +313,69 @@ export class ApiClient { * 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. When a - * bearer token would be attached, that target host must ALSO be a - * credential-safe transport — the API's own baseUrl being https doesn't - * vouch for some other (possibly cleartext) host the token would otherwise - * leak to. + * 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): Promise { + async getBinary(pathOrUrl: string, signal?: AbortSignal, timeoutMs?: number): Promise { const target = /^https?:\/\//i.test(pathOrUrl) ? pathOrUrl : this.url(pathOrUrl); - let used: string | null = null; - const send = async (): Promise => { - used = await this.tokens.get(); - const headers = await this.authHeaders(used); - if ("Authorization" in headers && !isCredentialSafeUrl(target)) { - throw new InsecureTransportError(target); + 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; } - return fetch(target, { headers, ...(signal ? { signal } : {}) }); - }; - let res = await send(); - if (res.status === 401 && (await this.refreshSession(pathOrUrl, used))) { - void res.body?.cancel().catch(() => {}); - res = await send(); + 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(); + throw err; } - if (!res.ok) throw await toHttpError(res); - return res; } /** @@ -324,25 +383,46 @@ export class ApiClient { * 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): Promise { - let used: string | null = null; - const send = async (): Promise => { - used = await this.tokens.get(); - return fetch(this.url(path), { - method: "POST", - headers: await this.authHeaders(used), - body: form, - ...(signal ? { signal } : {}), - }); - }; - let res = await send(); - if (res.status === 401 && (await this.refreshSession(path, used))) { - void res.body?.cancel().catch(() => {}); - res = await send(); + 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); } - if (!res.ok) throw await toHttpError(res); - return parseOkBody(res); } private async request( @@ -578,3 +658,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/test/auth.test.ts b/test/auth.test.ts index d43043b..f09a607 100644 --- a/test/auth.test.ts +++ b/test/auth.test.ts @@ -4,7 +4,7 @@ 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, StaticTokenStore, loginWithPassword } from "../src/core/auth.js"; +import { FileTokenStore, StaticTokenStore, loginWithPassword, isApiKeyToken } from "../src/core/auth.js"; const REQUEST_TIMEOUT_ENV_KEY = "AETHER_REQUEST_TIMEOUT_MS"; @@ -43,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"]; 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/vault_vision_401.test.ts b/test/vault_vision_401.test.ts index afb1367..6cf7c46 100644 --- a/test/vault_vision_401.test.ts +++ b/test/vault_vision_401.test.ts @@ -145,12 +145,16 @@ test("deleteSpacesFile: a non-retryable 401 (aek_ key) surfaces as HttpError, an // ── downloadMediaFile ───────────────────────────────────────────────────── -test("downloadMediaFile: 401 triggers refresh + retry, and streams the body to disk", () => +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"); - const mediaUrl = "https://media.example/output/abc.png"; + // 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"); @@ -162,20 +166,62 @@ test("downloadMediaFile: 401 triggers refresh + retry, and streams the body to d 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, "/auth/refresh", mediaUrl]); + assert.deepEqual(urls, [mediaUrl.replace("https://api.example", ""), "/auth/refresh", mediaUrl.replace("https://api.example", "")]); } finally { globalThis.fetch = real; } })); -test("downloadMediaFile: refuses to attach the bearer over an insecure (non-loopback http) media host", async () => { +// 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("https://api.example", new StaticTokenStore("aek_test")); + const api = new ApiClient("http://not-localhost.example", new StaticTokenStore("aek_test")); await assert.rejects( - () => downloadMediaFile(api, "http://evil.example.com/media.png", "/tmp", "vision_nano_pro", "image"), + () => 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"); From 8d9498ab1c42e7f82efc9d3597a7d6696a98595e Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 17:04:35 -0400 Subject: [PATCH 09/27] =?UTF-8?q?loop(LOOP-06):=20add=20StreamTimeoutError?= =?UTF-8?q?=20branch=20to=20errorHint=20=E2=80=94=20REPL=20now=20shows=20t?= =?UTF-8?q?he=20retry/doctor=20hint=20for=20mid-turn=20stream=20timeouts,?= =?UTF-8?q?=20matching=20hintFor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/core/errors.ts | 9 +++++++++ test/errors.test.ts | 14 +++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/core/errors.ts b/src/core/errors.ts index 77d4f27..e4d85e6 100644 --- a/src/core/errors.ts +++ b/src/core/errors.ts @@ -152,6 +152,15 @@ export function errorHint(err: unknown, baseUrl: string): string | null { // null and print the message with no actionable next step at all. if (err instanceof MalformedResponseError) return "retry, or /doctor to check connectivity"; if (err instanceof RequestTimeoutError) return "retry, or /doctor to check connectivity"; + // Same wording as error_hints.hintFor's StreamTimeoutError branch — a + // mid-turn stream timeout must read identically whether it surfaces via + // the REPL's printError (this function) or a one-shot/embedder path + // (hintFor). Previously unhandled here, so it fell through to the generic + // Error branch below (no NETWORK_CODES/cause.code, no "fetch failed" in + // the message) and returned null — a bare error with no recovery hint. + if (err instanceof StreamTimeoutError) { + return "the stream went quiet - retry, or /doctor to check connectivity"; + } 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/test/errors.test.ts b/test/errors.test.ts index 0628a54..df536ab 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, StreamTimeoutError, errorHint } from "../src/core/errors.js"; const BASE = "https://api.aethersystems.net"; @@ -31,6 +31,18 @@ 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/); +}); + 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); From eb64f0cae0c56233cdba9948149a92f7504563ea Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 17:17:47 -0400 Subject: [PATCH 10/27] loop(LOOP-06): sanitize+cap server-supplied login text before it hits the terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loginWithPassword built its thrown Error directly from the server's raw `reason` field, bypassing the control-char-strip-and-cap step toHttpError applies to every other server-text path — login.ts's headless catch writes that message straight to stderr with no sanitization of its own, so a compromised/misconfigured backend could inject terminal escape sequences (including OSC 52 clipboard-hijack payloads) via `aether auth login --username/--password`. The success path had the same gap: `plan`/ `commitment_hash` reached login.ts's stdout write unsanitized too. Extract toHttpError's strip-and-cap regex into a shared sanitizeServerText() in transport.ts and apply it to reason/plan/commitment_hash in loginWithPassword, so any server-supplied text loginWithPassword surfaces gets the same treatment as every other HTTP-error path. Adds regression tests in test/auth.test.ts (loginWithPassword-level) and a new test/login.test.ts (full cmdLogin path, capturing process.stderr/stdout) asserting a malicious reason/plan cannot survive into the terminal. --- src/core/auth.ts | 24 +++++++++-- src/core/transport.ts | 23 +++++++++-- test/auth.test.ts | 92 +++++++++++++++++++++++++++++++++++++++++++ test/login.test.ts | 81 +++++++++++++++++++++++++++++++++++++ 4 files changed, 212 insertions(+), 8 deletions(-) create mode 100644 test/login.test.ts diff --git a/src/core/auth.ts b/src/core/auth.ts index f56cf80..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, defaultRequestTimeoutMs, isCredentialSafeUrl } from "./transport.js"; +import { LOGIN_PATH, defaultRequestTimeoutMs, isCredentialSafeUrl, sanitizeServerText } from "./transport.js"; export interface TokenStore { get(): Promise; @@ -246,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/transport.ts b/src/core/transport.ts index b7bcde9..2640630 100644 --- a/src/core/transport.ts +++ b/src/core/transport.ts @@ -518,6 +518,24 @@ async function parseOkBody(res: Response): Promise { } } +/** + * 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 { @@ -533,10 +551,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; } } diff --git a/test/auth.test.ts b/test/auth.test.ts index f09a607..854adaa 100644 --- a/test/auth.test.ts +++ b/test/auth.test.ts @@ -104,6 +104,98 @@ test("loginWithPassword: a stalled /auth/login response times out instead of han } }); +// ── 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"); diff --git a/test/login.test.ts b/test/login.test.ts new file mode 100644 index 0000000..970660a --- /dev/null +++ b/test/login.test.ts @@ -0,0 +1,81 @@ +// 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 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; +} + +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; + } +}); From 5bd112c216172b979a60e5adcac8711235a4717c Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 17:34:53 -0400 Subject: [PATCH 11/27] =?UTF-8?q?loop(LOOP-01):=20route=20vault/media=20co?= =?UTF-8?q?mmand=20failures=20through=20hintFor()=20=E2=80=94=20no=20more?= =?UTF-8?q?=20hardcoded=20"are=20you=20logged=20in=3F"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vault.ts's fail() and media.ts's fail() printed the SAME "are you logged in?" hint for every failure, even though downloadFile/uploadFile/ deleteSpacesFile/dispatchGeneration etc. already throw a properly-classified HttpError (401/402/403/5xx). media.ts's mediaGenerate() and all 9 catch blocks in slash_media.ts printed only the raw err.message with no hint at all. All of these now route through core/error_hints.ts's hintFor(), the same fix already applied to workflow.ts's workflowNew, so a vault or media 402/403/5xx/network-outage shows the correct actionable hint instead of a misleading or missing one. --- src/commands/media.ts | 12 +- src/commands/slash_media.ts | 33 +++-- src/commands/vault.ts | 9 +- test/vault_media_error_hints.test.ts | 194 +++++++++++++++++++++++++++ 4 files changed, 236 insertions(+), 12 deletions(-) create mode 100644 test/vault_media_error_hints.test.ts diff --git a/src/commands/media.ts b/src/commands/media.ts index 7718d90..55fc295 100644 --- a/src/commands/media.ts +++ b/src/commands/media.ts @@ -8,6 +8,7 @@ import type { AppContext } from "../core/context.js"; import type { CatalogResponse } from "../types.js"; import { MODELS_PATH } from "../core/transport.js"; import { fail as coreFail } from "../core/errors.js"; +import { hintFor } from "../core/error_hints.js"; import { filterMediaModels, resolveModelKey, autoRouteModel, buildMediaPrompt, dispatchGeneration, downloadMediaFile, @@ -206,7 +207,13 @@ async function mediaGenerate(ctx: AppContext, prompt: string, kind: MediaKind, f process.stdout.write(theme.dim(` ${text.slice(0, 200)}\n`)); } } catch (err) { - process.stdout.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); + // LOOP-01 round 2: dispatchGeneration()/downloadMediaFile() already + // throw a classified HttpError (401/402/403/5xx) — this used to print + // only the raw message with no actionable next step at all. + const msg = err instanceof Error ? err.message : String(err); + process.stdout.write(`✗ ${msg}\n`); + const hint = hintFor(err); + if (hint) process.stdout.write(theme.dim(` ⤷ ${hint}\n`)); } } process.stdout.write(theme.dim(`\noutput: ${outdir}/\nview: aether output\n`)); @@ -230,6 +237,7 @@ function printMediaHelp(kind: MediaKind): void { ].join("\n")); } +// See vault.ts's identical fix for why hintFor() replaces the hardcoded hint. 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/commands/slash_media.ts b/src/commands/slash_media.ts index eb1803c..e9d4f4b 100644 --- a/src/commands/slash_media.ts +++ b/src/commands/slash_media.ts @@ -8,6 +8,7 @@ import type { Writable } from "node:stream"; import type { AppContext } from "../core/context.js"; import { theme } from "../ui/theme.js"; +import { hintFor } from "../core/error_hints.js"; import { basename } from "node:path"; import { existsSync as fsExistsSync } from "node:fs"; import { @@ -54,6 +55,20 @@ 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 { + const msg = err instanceof Error ? err.message : String(err); + out.write(`${indent}✗ ${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 +101,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 +119,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 +139,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 +163,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 +180,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 +192,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 +254,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 +295,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 +311,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/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(); + } +}); From c0b859d845a5ac5462b4ebc69e8a4e364f3212ea Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 18:17:24 -0400 Subject: [PATCH 12/27] loop(LOOP-01): stream vault uploads via fs.openAsBlob() instead of readFileSync+Blob double-buffering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit uploadFile() read the entire local file into a Buffer (fs.readFileSync) and then copied it again into an in-memory Blob([data]) before any bytes went over the wire — an unaddressed resource-consumption gap on the upload side, even though downloadFile() was fixed to stream straight to disk in 1d33357. Now hands FormData an fs.openAsBlob() Blob backed by the open file handle itself, so a large vault upload no longer holds the whole file in memory twice. Zero-dependency, matches this repo's no-deps convention. Deliberately skipped the finding's "at minimum, add a file-size guard" fallback clause — that was only meant to apply if true streaming wasn't feasible; since openAsBlob() removes the buffering entirely, an arbitrary size cap would just reject legitimate large uploads for no remaining safety reason. Added a regression test that proves the fix bites: since fs.openAsBlob() blobs re-validate the file on disk at read time (and a Buffer-backed Blob copy does not), deleting the source file mid-upload now causes the Blob read to fail — verified this test fails against the pre-fix code and passes against the fix. --- src/core/vault.ts | 17 ++++++-- test/vault_vision_401.test.ts | 80 +++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 3 deletions(-) diff --git a/src/core/vault.ts b/src/core/vault.ts index 5d62832..3e19364 100644 --- a/src/core/vault.ts +++ b/src/core/vault.ts @@ -111,16 +111,27 @@ export async function getSpacesUsage(api: ApiClient): Promise return api.getJson(VAULT_SPACES_USAGE_PATH); } -/** 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). + */ 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); + formData.append("file", blob, filename); return api.postForm(VAULT_SPACES_UPLOAD_PATH, formData); } diff --git a/test/vault_vision_401.test.ts b/test/vault_vision_401.test.ts index 6cf7c46..b1b1aef 100644 --- a/test/vault_vision_401.test.ts +++ b/test/vault_vision_401.test.ts @@ -72,6 +72,86 @@ test("uploadFile: 401 with a session token triggers one /auth/refresh then a ret } })); +// 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; + } + })); + +// 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", () => From cb00f924a0eb097025efa3c99e83f5bc9fd34a2d Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 18:32:23 -0400 Subject: [PATCH 13/27] loop(LOOP-06): print a loading line before aether auth status's /models call renderAuthBox's first move is a network round-trip to /models with no cache to fall back on, and cmdAuth wrote nothing to stdout until the whole call resolved -- up to DEFAULT_REQUEST_TIMEOUT_MS (30s) of silence on a slow connection, "the REPL just looks hung" (the exact class PR #47 fixed for slash.ts's catalog fetch). Print theme.dim("checking session...") before awaiting renderAuthBox in both the "status" subcommand and bare `aether auth` branches, matching the loading-line convention already used by slash.ts/vault.ts/slash_media.ts. Adds cmdAuth-level regression tests proving the loading line is written while /models is still pending (not after), for both call sites. --- src/commands/auth.ts | 12 ++++- test/auth_401.test.ts | 104 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 114 insertions(+), 2 deletions(-) diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 0bdbfbb..aa91b3c 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -170,6 +170,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; @@ -182,8 +189,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; diff --git a/test/auth_401.test.ts b/test/auth_401.test.ts index 3b8d99b..caf2b9b 100644 --- a/test/auth_401.test.ts +++ b/test/auth_401.test.ts @@ -12,6 +12,13 @@ // 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"; @@ -21,7 +28,8 @@ 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 } from "../src/commands/auth.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"; @@ -330,3 +338,97 @@ test("renderAuthBox: a successful /models call renders the normal 'Authenticated 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(); + } +}); From 9ee863edd52ec3d85dcb3406d49836f62ee8fd11 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 18:43:55 -0400 Subject: [PATCH 14/27] loop(LOOP-06): distinguish a picker key-handler fault from a deliberate Escape cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pickModel's onKey catch silently resolved null on ANY thrown error (render/ decode bug), exactly like a user pressing Escape. showPicker then printed the same "kept current session." for both, hiding a real internal fault from the user and from anyone debugging a picker regression. Now the catch writes a distinct dim diagnostic ("picker error — kept current session.") before resolving null, so the two paths are visibly different in the transcript even though both still resolve null and restore the REPL listeners the same way. --- src/ui/model_picker.ts | 7 ++- test/model_picker.test.ts | 106 +++++++++++++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/src/ui/model_picker.ts b/src/ui/model_picker.ts index 3a5590e..59a306b 100644 --- a/src/ui/model_picker.ts +++ b/src/ui/model_picker.ts @@ -240,8 +240,13 @@ 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: without it this resolves null exactly + // like a deliberate Escape, and the caller (slash.ts's showPicker) + // would print the same "kept current session." for both — hiding a + // real internal fault behind the same text as a plain cancel. cleanup(); + out.write(theme.dim(" picker error — kept current session.") + "\n"); resolve(null); } }; diff --git a/test/model_picker.test.ts b/test/model_picker.test.ts index 87899e9..7522895 100644 --- a/test/model_picker.test.ts +++ b/test/model_picker.test.ts @@ -1,7 +1,8 @@ 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 type { CatalogItem } from "../src/types.js"; +import type { Writable } from "node:stream"; function item(overrides: Partial & { id: string }): CatalogItem { return { @@ -142,3 +143,106 @@ 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, null, "a caught fault still resolves null (session isn't bricked)"); + 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); + } +}); + +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); + } +}); From c335d59abaf726bcb910f354ae0321643c3eb78f Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 18:56:47 -0400 Subject: [PATCH 15/27] loop(LOOP-06): style confirmSwitch's tier-lock message, add /tier hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The " is locked on tier " rejection in confirmSwitch was plain unstyled text with no next step, unlike every other auth-adjacent surface where a plan/tier restriction (HTTP 403 via httpStatusHint) gets a dim hint pointing at /tier or `aether models`. It was also the only line in this function not wrapped in theme.dim (the restart warning two lines down already is). Now the locked-item line is dim and appends the same actionable pointer httpStatusHint(403) uses, so a tier lock reached via the picker reads the same as one reached over the wire. confirmSwitch is exported (alongside the existing resolveSelection) so this can be unit-tested directly — the module-level catalog cache the /model tests above populate isn't force-refreshable in-band, so a differently- available item can't reach this branch through handleSlash alone. --- src/commands/slash.ts | 8 ++++++-- test/slash.test.ts | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/commands/slash.ts b/src/commands/slash.ts index 3173f41..1dca733 100644 --- a/src/commands/slash.ts +++ b/src/commands/slash.ts @@ -420,7 +420,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 +428,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/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. +}); From eb7d62ec4029983949468e1ee322ecd3ce109812 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 19:48:30 -0400 Subject: [PATCH 16/27] =?UTF-8?q?loop(LOOP-06):=20reject=20a=20stream=20wi?= =?UTF-8?q?th=20no=20terminal=20frame=20=E2=80=94=20throw=20StreamIncomple?= =?UTF-8?q?teError?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runCloudTurn and CloudBrain's pump both let decodeSse's for-await loop exit cleanly (no throw) when the underlying byte stream just ends without ever sending a done or error frame — e.g. a proxy/load-balancer that time-boxes the SSE response and closes the socket inside the idle window. That rendered whatever partial delta output arrived as a fully successful turn. Both sites now track whether a terminal frame was actually observed and throw/report StreamIncompleteError when it wasn't, routed through errorHint/hintFor like any other failure. --- src/commands/chat.ts | 16 ++++++++++++++-- src/core/brain_cloud.ts | 12 +++++++++--- src/core/error_hints.ts | 7 +++++++ src/core/errors.ts | 23 +++++++++++++++++++++++ test/brain_cloud.test.ts | 11 +++++++++++ test/chat.test.ts | 17 +++++++++++++++++ test/error_hints.test.ts | 9 ++++++++- test/errors.test.ts | 12 +++++++++++- 8 files changed, 100 insertions(+), 7 deletions(-) diff --git a/src/commands/chat.ts b/src/commands/chat.ts index 09937ac..43d2c15 100644 --- a/src/commands/chat.ts +++ b/src/commands/chat.ts @@ -10,7 +10,7 @@ import { buildChatRequest } from "../core/envelope.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"; @@ -89,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 @@ -144,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)) { @@ -155,10 +165,12 @@ 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. diff --git a/src/core/brain_cloud.ts b/src/core/brain_cloud.ts index 19bc7a5..342fac1 100644 --- a/src/core/brain_cloud.ts +++ b/src/core/brain_cloud.ts @@ -16,7 +16,7 @@ import type { ApiClient } 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,6 +60,8 @@ 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: "" }); } diff --git a/src/core/error_hints.ts b/src/core/error_hints.ts index e39f859..3e65c37 100644 --- a/src/core/error_hints.ts +++ b/src/core/error_hints.ts @@ -7,6 +7,7 @@ import { InsecureTransportError, MalformedResponseError, RequestTimeoutError, + StreamIncompleteError, StreamTimeoutError, httpStatusHint, isAbortError as coreIsAbortError, @@ -38,6 +39,12 @@ export function hintFor(err: unknown): string | null { if (err instanceof StreamTimeoutError) { return "the stream went quiet - retry, or /doctor to check connectivity"; } + // Same wording as errors.errorHint's StreamIncompleteError branch — see + // that module's note on why 401/402/403/429 are the only wording actually + // shared; this one has no baseUrl-dependent text so it's identical here too. + 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"; } diff --git a/src/core/errors.ts b/src/core/errors.ts index e4d85e6..83b0977 100644 --- a/src/core/errors.ts +++ b/src/core/errors.ts @@ -44,6 +44,22 @@ 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 @@ -161,6 +177,13 @@ export function errorHint(err: unknown, baseUrl: string): string | null { if (err instanceof StreamTimeoutError) { return "the stream went quiet - retry, or /doctor to check connectivity"; } + // Same wording as error_hints.hintFor's StreamIncompleteError branch — a + // clean-looking premature close must read identically whether it surfaces + // via the REPL's printError (this function) or a one-shot/embedder path + // (hintFor). + if (err instanceof StreamIncompleteError) { + return "retry, or /doctor to check connectivity"; + } 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/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 f93cbd6..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,22 @@ 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 diff --git a/test/error_hints.test.ts b/test/error_hints.test.ts index 7d9252b..1a665dd 100644 --- a/test/error_hints.test.ts +++ b/test/error_hints.test.ts @@ -1,7 +1,7 @@ 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/); @@ -30,6 +30,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"; diff --git a/test/errors.test.ts b/test/errors.test.ts index df536ab..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, StreamTimeoutError, errorHint } from "../src/core/errors.js"; +import { HttpError, StreamIncompleteError, StreamTimeoutError, errorHint } from "../src/core/errors.js"; const BASE = "https://api.aethersystems.net"; @@ -43,6 +43,16 @@ test("stream timeouts get a retry/doctor hint, matching error_hints.hintFor (LOO 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); From f44441fbb596f470f296a7c7d6dd330fa763a8b6 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 19:52:49 -0400 Subject: [PATCH 17/27] =?UTF-8?q?loop(LOOP-01):=20narrow=20pollForToken's?= =?UTF-8?q?=20HttpError=20guard=20to=20the=20RFC=208628=20400=20pending=20?= =?UTF-8?q?shape=20=E2=80=94=20a=205xx=20no=20longer=20masquerades=20as=20?= =?UTF-8?q?authorization=5Fpending?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/core/device.ts | 23 ++++++++++++------- test/device.test.ts | 54 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 8 deletions(-) diff --git a/src/core/device.ts b/src/core/device.ts index b5211f1..d779172 100644 --- a/src/core/device.ts +++ b/src/core/device.ts @@ -70,14 +70,21 @@ export async function pollForToken( 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 — 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) 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.body && typeof err.body === "object") { + // 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 { diff --git a/test/device.test.ts b/test/device.test.ts index 945e43b..5005cf9 100644 --- a/test/device.test.ts +++ b/test/device.test.ts @@ -163,6 +163,60 @@ test("pollForToken: real authorization_pending (HttpError with body) times out w } }); +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 From 960b590042b72696b8f92bdfee665cb4c7d1becd Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 20:07:41 -0400 Subject: [PATCH 18/27] loop(LOOP-01): stop workflow.ts's vault CRUD wrappers from swallowing every failure listWorkflows/getWorkflow/deleteWorkflow caught ANY error from listSpaces/ getSpacesContent/deleteSpacesFile -- an expired-session 401, a network outage, or a 5xx -- and turned it into an empty list / null / false. The command layer then reported that as legitimate "no data": "No workflows found in vault.", "workflow not found: ", "delete failed -- workflow may not exist", with zero indication to run `aether auth login`. vault.ts's own functions never throw for a genuinely empty vault or a content-less file (200 with `files: []` / `content: null`), so a thrown error here is always a real failure -- let it propagate to the existing per-command try/catch that already calls fail(). workflowSave's getWorkflow call sat outside its own try block (the try only wrapped the later saveWorkflow call), so removing the swallow would have let a real failure there escape cmdWorkflow entirely and hit main.ts's bare top-level catch with no hint at all -- gave it its own try/catch too. Adds test/workflow_vault_error_propagation.test.ts covering list/view/ delete/save against a 401 and a 500, plus a control case confirming a genuinely empty vault still prints "No workflows found in vault." --- src/commands/workflow.ts | 11 +- src/core/workflow.ts | 45 ++--- test/workflow_vault_error_propagation.test.ts | 168 ++++++++++++++++++ 3 files changed, 202 insertions(+), 22 deletions(-) create mode 100644 test/workflow_vault_error_propagation.test.ts diff --git a/src/commands/workflow.ts b/src/commands/workflow.ts index 25e81a9..d92d0ac 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); diff --git a/src/core/workflow.ts b/src/core/workflow.ts index 2e0eb34..40d86d2 100644 --- a/src/core/workflow.ts +++ b/src/core/workflow.ts @@ -319,35 +319,40 @@ export async function finalizeWorkflow( 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/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; + } +}); From 7b628379ec7a0b7111a59d30c41812b0355356ff Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 20:15:53 -0400 Subject: [PATCH 19/27] =?UTF-8?q?loop(LOOP-06):=20check=20err.cause.code?= =?UTF-8?q?=20in=20hintFor=20=E2=80=94=20no=20more=20silent=20null=20for?= =?UTF-8?q?=20cause-shaped=20undici=20network=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/core/error_hints.ts | 10 ++++++++++ src/core/errors.ts | 7 +++++-- test/error_hints.test.ts | 17 +++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/core/error_hints.ts b/src/core/error_hints.ts index 3e65c37..147852d 100644 --- a/src/core/error_hints.ts +++ b/src/core/error_hints.ts @@ -6,6 +6,7 @@ import { HttpError, InsecureTransportError, MalformedResponseError, + NETWORK_CODES, RequestTimeoutError, StreamIncompleteError, StreamTimeoutError, @@ -52,9 +53,18 @@ export function hintFor(err: unknown): string | null { 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") || diff --git a/src/core/errors.ts b/src/core/errors.ts index 83b0977..61b94dc 100644 --- a/src/core/errors.ts +++ b/src/core/errors.ts @@ -117,8 +117,11 @@ export class MalformedResponseError extends HttpError { } // 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", diff --git a/test/error_hints.test.ts b/test/error_hints.test.ts index 1a665dd..e2ad6a7 100644 --- a/test/error_hints.test.ts +++ b/test/error_hints.test.ts @@ -21,6 +21,23 @@ test("network failures point at connectivity", () => { 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/); }); From ef73f3e7dfae5198bad8c3d3f577ed2516a18e40 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 20:38:52 -0400 Subject: [PATCH 20/27] loop(LOOP-06): route login.ts + auth.ts's authRefresh through formatErrorLine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every error path in cmdLogin (headless --username/--password, device-code request, and the poll-for-token wait) and authRefresh's failure paths wrote a raw, unstyled `✗ ` straight to stderr — no errTheme color, no errorHint-derived next step, no /doctor pointer, and none of formatErrorLine's trailing blank-line separator. Every other auth-adjacent surface (chat.ts's printError, render.ts's Renderer.error) had already been unified onto that convention; login/refresh, the surfaces a user hits first, were left bare. Route every catch in cmdLogin, plus authRefresh's catch AND its 200-with-no- session_token branch (same bare ✗ line, no err object but the same fix applies), through formatErrorLine(msg, { hint: errorHint(err, baseUrl) }) — same convention chat.ts's printError uses, so a network failure during login or refresh now gets the same /doctor pointer and a rejected session gets the same re-login pointer as everywhere else. Adds regression tests in test/login.test.ts (all three cmdLogin catch paths) and test/auth_401.test.ts (authRefresh's catch + the no-session_token branch) asserting the printed line carries the ✗ glyph, the errorHint-derived hint where one applies, and the separator — and confirms a plain error with no mapped hint (denied-in-browser) still gets styled output with no fabricated hint. Deliberately left login.ts's "No token on stdin" (--with-token, empty stdin) unconverted: it's an exit-2 input-validation message with no underlying err to derive a hint from, matching the unstyled "usage:"-style convention used by other exit-2 paths elsewhere in the CLI (config.ts, vault.ts, workflow.ts), and it wasn't in the finding's cited evidence. --- src/commands/auth.ts | 14 ++++-- src/commands/login.ts | 11 ++-- test/auth_401.test.ts | 85 +++++++++++++++++++++++++++++++ test/login.test.ts | 113 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 217 insertions(+), 6 deletions(-) diff --git a/src/commands/auth.ts b/src/commands/auth.ts index aa91b3c..020b630 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -9,11 +9,12 @@ 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 } from "../core/errors.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. Thin wrapper so callers of THIS module keep importing `isApiToken` @@ -248,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/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` 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/login.test.ts b/test/login.test.ts index 970660a..e3ea487 100644 --- a/test/login.test.ts +++ b/test/login.test.ts @@ -11,6 +11,7 @@ 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 { @@ -22,6 +23,32 @@ function fakeCtx(): AppContext { } 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); @@ -79,3 +106,89 @@ test("cmdLogin (--username/--password): a malicious server `reason` cannot survi (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(); + } +}); From 29297ddc2e5484f943b5f008ca680fe7b1859558 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 20:45:54 -0400 Subject: [PATCH 21/27] =?UTF-8?q?loop(LOOP-01):=20route=20workflow.ts's=20?= =?UTF-8?q?fail()=20through=20hintFor()=20=E2=80=94=20no=20more=20hardcode?= =?UTF-8?q?d=20auth=20hint=20for=20402/403/5xx?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/commands/workflow.ts | 2 +- test/workflow_error_hints.test.ts | 99 +++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 test/workflow_error_hints.test.ts diff --git a/src/commands/workflow.ts b/src/commands/workflow.ts index d92d0ac..ed5fac8 100644 --- a/src/commands/workflow.ts +++ b/src/commands/workflow.ts @@ -387,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/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/); + }); +} From 803647a61b6b789695f70c8858aff98281af479b Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 20:54:29 -0400 Subject: [PATCH 22/27] =?UTF-8?q?loop(LOOP-01):=20uploadFile()=20opts=20in?= =?UTF-8?q?to=20the=20120s=20stream-class=20timeout=20=E2=80=94=20no=20mor?= =?UTF-8?q?e=20flat=2030s=20cap=20on=20large=20vault=20uploads?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit postForm() bounds the ENTIRE request (connect + send-body + wait-for-response) with one flat timeoutMs, unlike getBinary()/stream() which split into a connect-phase timeout plus a separate idle-timeout on the body. fetch() gives no hook to observe outgoing multipart-body progress, so that split isn't achievable for the upload path. Instead uploadFile() now explicitly passes defaultStreamTimeoutMs() (120s) to postForm(), the same pattern client.ts/ workflow.ts already use for their long-running CHAT_PATH fallback, instead of silently inheriting postForm()'s 30s metadata-call default — so a large-but- healthy vault upload is no longer always killed by RequestTimeoutError just for taking longer than 30s. --- src/core/vault.ts | 19 ++++++++++++-- test/vault_vision_401.test.ts | 47 +++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/core/vault.ts b/src/core/vault.ts index 3e19364..ded36ef 100644 --- a/src/core/vault.ts +++ b/src/core/vault.ts @@ -5,7 +5,7 @@ // fetch()) so they get the same refresh-on-401 retry and HttpError // classification as every other call. -import { ApiClient } from "./transport.js"; +import { ApiClient, defaultStreamTimeoutMs } from "./transport.js"; import { VAULT_LIST_PATH, VAULT_BROWSE_PATH, VAULT_SPACES_LIST_PATH, VAULT_SPACES_USAGE_PATH, @@ -122,6 +122,21 @@ export async function getSpacesUsage(api: ApiClient): Promise * 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, @@ -132,7 +147,7 @@ export async function uploadFile( const blob = await fs.openAsBlob(filePath); const formData = new FormData(); formData.append("file", blob, filename); - return api.postForm(VAULT_SPACES_UPLOAD_PATH, formData); + return api.postForm(VAULT_SPACES_UPLOAD_PATH, formData, undefined, defaultStreamTimeoutMs()); } /** diff --git a/test/vault_vision_401.test.ts b/test/vault_vision_401.test.ts index b1b1aef..13f7033 100644 --- a/test/vault_vision_401.test.ts +++ b/test/vault_vision_401.test.ts @@ -128,6 +128,53 @@ test("uploadFile: the multipart body is a file-backed Blob (fs.openAsBlob), not } })); +// 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", () => From 41f7e1edc83a122310385252c10da627ec71fa7c Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 21:15:40 -0400 Subject: [PATCH 23/27] =?UTF-8?q?loop(LOOP-06):=20wrap=20pickModel's=20emp?= =?UTF-8?q?ty-state=20lines=20in=20theme.dim=20=E2=80=94=20matches=20the?= =?UTF-8?q?=20rest=20of=20the=20picker's=20styling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/ui/model_picker.ts | 4 ++-- test/model_picker.test.ts | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/ui/model_picker.ts b/src/ui/model_picker.ts index 59a306b..dc04474 100644 --- a/src/ui/model_picker.ts +++ b/src/ui/model_picker.ts @@ -171,7 +171,7 @@ export async function pickModel( out: Writable, ): Promise { if (items.length === 0) { - out.write("no models available.\n"); + out.write(theme.dim("no models available.") + "\n"); return null; } @@ -184,7 +184,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; } diff --git a/test/model_picker.test.ts b/test/model_picker.test.ts index 7522895..ec78ae1 100644 --- a/test/model_picker.test.ts +++ b/test/model_picker.test.ts @@ -1,6 +1,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { groupItems, flattenGroups, currentIndex, renderPicker, pickModel } from "../src/ui/model_picker.js"; +import { theme } from "../src/ui/theme.js"; import type { CatalogItem } from "../src/types.js"; import type { Writable } from "node:stream"; @@ -225,6 +226,35 @@ test("pickModel: a fault inside the key handler prints a distinct diagnostic, no } }); +// ── 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 }; From b609c4adeaf2610e615b81a5cb835c85273294e5 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 21:30:03 -0400 Subject: [PATCH 24/27] =?UTF-8?q?loop(META):=20simplify=20=E2=80=94=20cons?= =?UTF-8?q?olidate=20duplicated=20baseUrl-independent=20error=20hints?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit errorHint (errors.ts) and hintFor (error_hints.ts) each hand-duplicated four identical branches (MalformedResponseError, StreamTimeoutError, StreamIncompleteError, RequestTimeoutError) that don't depend on baseUrl — different loop rounds added these to both files without reconciling them, and the RequestTimeoutError wording had already drifted between the two copies. Extracted the shared branches into errors.ts's new nonHttpErrorHint(), called first by both errorHint and hintFor; only the genuinely baseUrl-dependent 5xx/network branches remain separate per file. 812 tests still pass; no test pinned the drifted exact wording. --- src/core/error_hints.ts | 39 +++++++++++---------------------- src/core/errors.ts | 48 ++++++++++++++++++++++------------------- 2 files changed, 38 insertions(+), 49 deletions(-) diff --git a/src/core/error_hints.ts b/src/core/error_hints.ts index 147852d..5d143f5 100644 --- a/src/core/error_hints.ts +++ b/src/core/error_hints.ts @@ -5,30 +5,27 @@ import { HttpError, InsecureTransportError, - MalformedResponseError, NETWORK_CODES, - RequestTimeoutError, - StreamIncompleteError, - StreamTimeoutError, 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. * - * Only 401/402/403/429 wording is actually shared with errors.errorHint (via - * httpStatusHint — see its own doc comment). The 5xx and network-failure - * branches below are intentionally 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. */ + * 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 { - // Checked before the generic HttpError branch (MalformedResponseError - // extends it) — same reasoning as errors.errorHint. - if (err instanceof MalformedResponseError) return "retry, or /doctor to check connectivity"; + 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. @@ -37,18 +34,6 @@ export function hintFor(err: unknown): string | null { // 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"; - } - // Same wording as errors.errorHint's StreamIncompleteError branch — see - // that module's note on why 401/402/403/429 are the only wording actually - // shared; this one has no baseUrl-dependent text so it's identical here too. - 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"; - } 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 diff --git a/src/core/errors.ts b/src/core/errors.ts index 61b94dc..eda2984 100644 --- a/src/core/errors.ts +++ b/src/core/errors.ts @@ -159,34 +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 { - // Checked before the generic HttpError branch below (MalformedResponseError - // extends it): 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. - if (err instanceof MalformedResponseError) return "retry, or /doctor to check connectivity"; - if (err instanceof RequestTimeoutError) return "retry, or /doctor to check connectivity"; - // Same wording as error_hints.hintFor's StreamTimeoutError branch — a - // mid-turn stream timeout must read identically whether it surfaces via - // the REPL's printError (this function) or a one-shot/embedder path - // (hintFor). Previously unhandled here, so it fell through to the generic - // Error branch below (no NETWORK_CODES/cause.code, no "fetch failed" in - // the message) and returned null — a bare error with no recovery hint. - if (err instanceof StreamTimeoutError) { - return "the stream went quiet - retry, or /doctor to check connectivity"; - } - // Same wording as error_hints.hintFor's StreamIncompleteError branch — a - // clean-looking premature close must read identically whether it surfaces - // via the REPL's printError (this function) or a one-shot/embedder path - // (hintFor). - if (err instanceof StreamIncompleteError) { - return "retry, or /doctor to check connectivity"; - } + 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); From 54db2687ea32491199ee10affed0961c8924a712 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 22:09:40 -0400 Subject: [PATCH 25/27] loop(META): close terminal-escape-injection gap in getBinary + media error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent security review of the meta-loop diff found that a hostile server-controlled media_url can make fetch()'s own URL parser throw a raw TypeError whose message echoes the target string verbatim — the one path left unsanitized after this same loop closed the equivalent gap everywhere else (sanitizeServerText/sanitizeTerm/formatErrorLine). - transport.ts: getBinary() now sanitizes any non-typed error (a raw fetch()-level throw) before it leaves the function, mirroring sanitizeServerText's existing treatment of every other server string. - media.ts: sanitize resp.media_url, resp.text/response, and the catch block's err.message before writing them to the terminal. - slash_media.ts: sanitize writeErr's err.message (shared by every media slash command's catch block). --- src/commands/media.ts | 15 +++++++++++---- src/commands/slash_media.ts | 7 ++++++- src/core/transport.ts | 19 +++++++++++++++++++ 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/commands/media.ts b/src/commands/media.ts index 55fc295..bbf5616 100644 --- a/src/commands/media.ts +++ b/src/commands/media.ts @@ -17,6 +17,7 @@ import { type MediaKind, type MediaModel, type GenFlags, type GenResult, } from "../core/vision.js"; import { theme } from "../ui/theme.js"; +import { sanitizeTerm } from "../ui/text.js"; import { basename } from "node:path"; import { createInterface, type Interface } from "node:readline"; @@ -197,21 +198,27 @@ async function mediaGenerate(ctx: AppContext, prompt: string, kind: MediaKind, f url: resp.media_url, timestamp: new Date().toISOString(), flags, }; const entry = recordOutput(result); + // resp.media_url and resp.text/response below are server-controlled + // (an LLM generation response) — sanitized before hitting the + // terminal, same as every other server-supplied string in this flow + // (see sanitizeServerText's own doc comment in transport.ts). process.stdout.write( ` ${theme.iceBlue("↓")} #${entry.index} ${entry.filename}\n` + - ` ${theme.dim(`url: ${resp.media_url}`)}\n\n` + ` ${theme.dim(`url: ${sanitizeTerm(resp.media_url)}`)}\n\n` ); if (flags.open) openOutput(entry); } else { process.stdout.write(theme.dim(" no media URL in response\n")); - process.stdout.write(theme.dim(` ${text.slice(0, 200)}\n`)); + process.stdout.write(theme.dim(` ${sanitizeTerm(text.slice(0, 200))}\n`)); } } catch (err) { // LOOP-01 round 2: dispatchGeneration()/downloadMediaFile() already // throw a classified HttpError (401/402/403/5xx) — this used to print - // only the raw message with no actionable next step at all. + // only the raw message with no actionable next step at all. Sanitized: + // a raw fetch()-level failure on a hostile media_url can otherwise + // carry attacker-controlled bytes straight from err.message. const msg = err instanceof Error ? err.message : String(err); - process.stdout.write(`✗ ${msg}\n`); + process.stdout.write(`✗ ${sanitizeTerm(msg)}\n`); const hint = hintFor(err); if (hint) process.stdout.write(theme.dim(` ⤷ ${hint}\n`)); } diff --git a/src/commands/slash_media.ts b/src/commands/slash_media.ts index e9d4f4b..c2883fb 100644 --- a/src/commands/slash_media.ts +++ b/src/commands/slash_media.ts @@ -8,6 +8,7 @@ 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"; @@ -63,8 +64,12 @@ const SF = theme.dim; // `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}✗ ${msg}\n`); + out.write(`${indent}✗ ${sanitizeTerm(msg)}\n`); const hint = hintFor(err); if (hint) out.write(SF(`${indent} ⤷ ${hint}\n`)); } diff --git a/src/core/transport.ts b/src/core/transport.ts index 2640630..26301cd 100644 --- a/src/core/transport.ts +++ b/src/core/transport.ts @@ -374,6 +374,25 @@ export class ApiClient { } 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; } } From d72f699d4e5a042eb4d9968dcd08fbb4f915eaf6 Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 22:09:42 -0400 Subject: [PATCH 26/27] loop(META): bound LLM-generation calls by the 120s stream timeout, not the new 30s default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review found a real regression: commit a7b3621 gave ApiClient.request() a 30s default timeout, and the loop was careful to carve out defaultStreamTimeoutMs() (120s) overrides for every long-running generation call it touched (chat.ts, brain_cloud.ts, client.ts, workflow.ts) — except three sibling LLM-generation calls it missed: vision.ts's dispatchGeneration/parseStoryboard (image/video/storyboard generation — aether image, aether video, /photogen, /videogen, /animate, /reframe, /recut, /storyboard) and the same class in scaffold.ts/port.ts (LLM-backed code generation and cross-language translation). Any of these that legitimately ran past 30s now failed with RequestTimeoutError where they previously succeeded. --- src/core/port.ts | 6 ++++-- src/core/scaffold.ts | 6 ++++-- src/core/vision.ts | 12 +++++++++--- 3 files changed, 17 insertions(+), 7 deletions(-) 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/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/vision.ts b/src/core/vision.ts index ce21208..8408be7 100644 --- a/src/core/vision.ts +++ b/src/core/vision.ts @@ -4,7 +4,7 @@ // download helpers with streaming fetch, output manager with persistent log. // No terminal I/O. Every function wraps a single concept. -import { ApiClient } from "./transport.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"; @@ -189,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()); } // ═════════════════════════════════════════════════════════════════════ @@ -394,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" From 4e4091599cd7f0b30b2a3fe7bfd10f3e5004e0ba Mon Sep 17 00:00:00 2001 From: dbarrante Date: Mon, 20 Jul 2026 22:09:43 -0400 Subject: [PATCH 27/27] loop(META): fix duplicate "kept current session." message on a picker fault MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3's fix (9ee863e) had pickModel() print a distinct diagnostic on an internal key-handler fault, but its caller (slash.ts's showPicker) still unconditionally printed its own generic "kept current session." right after — so a real fault showed two back-to-back, redundant lines instead of one clear message, while a plain Escape correctly showed just one. pickModel() now resolves undefined (not null) on a fault, distinguishing it from a deliberate cancel; showPicker checks for undefined and skips its own message in that case. Added an end-to-end regression test exercising the full handleSlash -> showPicker -> pickModel path (the prior tests only covered pickModel in isolation, which is why this composition bug wasn't caught). --- src/commands/slash.ts | 6 +++++ src/ui/model_picker.ts | 20 +++++++++------ test/model_picker.test.ts | 51 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 69 insertions(+), 8 deletions(-) diff --git a/src/commands/slash.ts b/src/commands/slash.ts index 1dca733..9a02977 100644 --- a/src/commands/slash.ts +++ b/src/commands/slash.ts @@ -374,6 +374,12 @@ async function showPicker( const items = byKind(cat, kind); const picked = await pickModel(items, out); + if (picked === undefined) { + // pickModel hit an internal fault and already printed its own distinct + // diagnostic — printing the generic "kept current session." below too + // would show the same failure as two back-to-back, redundant lines. + return null; + } if (!picked) { // pickModel returned null — either cancelled (Esc) or non-TTY fallback. // If non-TTY, render a flat numbered list so the user can still /model . diff --git a/src/ui/model_picker.ts b/src/ui/model_picker.ts index dc04474..1c583d2 100644 --- a/src/ui/model_picker.ts +++ b/src/ui/model_picker.ts @@ -163,13 +163,18 @@ 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(theme.dim("no models available.") + "\n"); return null; @@ -241,13 +246,14 @@ 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. Write - // a distinct diagnostic first: without it this resolves null exactly - // like a deliberate Escape, and the caller (slash.ts's showPicker) - // would print the same "kept current session." for both — hiding a - // real internal fault behind the same text as a plain cancel. + // 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(); out.write(theme.dim(" picker error — kept current session.") + "\n"); - resolve(null); + resolve(undefined); } }; diff --git a/test/model_picker.test.ts b/test/model_picker.test.ts index ec78ae1..7584289 100644 --- a/test/model_picker.test.ts +++ b/test/model_picker.test.ts @@ -2,7 +2,9 @@ import { test } from "node:test"; import assert from "node:assert/strict"; 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 { @@ -216,7 +218,11 @@ test("pickModel: a fault inside the key handler prints a distinct diagnostic, no 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, null, "a caught fault still resolves null (session isn't bricked)"); + 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", @@ -276,3 +282,46 @@ test("pickModel: a deliberate Escape resolves null WITHOUT the fault diagnostic" 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); + } +});