Meta-loop: further harden the auth flow + UX beyond PR #47#53
Merged
Conversation
…ch() 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 <noreply@anthropic.com>
…thorization_pending 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.
…n `aether auth status` 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 <noreply@anthropic.com>
… malformed 2xx bodies 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 <noreply@anthropic.com>
…nt.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.
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.
…pe isAbortError 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.
…y/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 <noreply@anthropic.com>
…shows the retry/doctor hint for mid-turn stream timeouts, matching hintFor
… the terminal 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.
… no more hardcoded "are you logged in?" 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.
…adFileSync+Blob double-buffering 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.
…ls 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.
…te Escape cancel
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.
The "<id> is locked on tier <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.
…ncompleteError 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.
…400 pending shape — a 5xx no longer masquerades as authorization_pending
… 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: <name>", "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."
…for cause-shaped undici network errors
…rrorLine
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 `✗ <message>` 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.
… hardcoded auth hint for 402/403/5xx
… no more flat 30s cap on large vault uploads 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.
…hes the rest of the picker's styling
…or hints 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.
…error paths 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).
…t the new 30s default 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.
… fault 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).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e4091599c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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)); |
There was a problem hiding this comment.
When a vault download body errors after some chunks have been received, such as an idle timeout, abort, or mid-transfer network reset, pipeline rejects only after createWriteStream(outputPath) has already written a truncated file at the requested destination. The previous buffered path only called writeFileSync after the full arrayBuffer() succeeded, so failed downloads did not leave a corrupt final file behind. Stream to a temporary file and rename on success, or unlink the destination on failure.
Useful? React with 👍 / 👎.
| res = await send(); | ||
| } | ||
| if (!res.ok) throw await toHttpError(res); | ||
| return await parseOkBody<T>(res); |
There was a problem hiding this comment.
If a non-streaming endpoint sends response headers within the timeout and then stalls or trickles an incomplete JSON body, the raceAgainst(fetch(...)) timer has already been cleared and parseOkBody(res) waits on res.text() with no timeout. In that scenario calls like /models or /auth/refresh can still hang indefinitely despite the new request timeout; race the body read as well, or keep aborting net on the same deadline until parsing completes.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Planned follow-up from #47 ("run a meta-loop over this branch to further optimize the auth flow + UX, not just patch the reported bug"). Ran an automated multi-round audit-fix-test loop (LOOP-01 backend/API lens + LOOP-06 UX lens, adapted for a terminal CLI — no browser/Playwright applies here) against
origin/main(which already includes #47 and the TypeScript 7 migration), then a full independent review pass on top of the loop's own results.Process: Map → parallel Discover (2 lenses) → adversarial Verify (3 independent skeptics per finding, majority-refute kills it, empty evidence auto-voids a vote) → sequential Implement (one file-group per commit, regression test required) → Test → repeat, capped at 3 rounds. Then two independent cold reviewers (no context from the loop) audited the full resulting diff — one security-focused, one general-quality — and I fixed everything they confirmed before opening this PR.
Results across 3 rounds: 37 raw findings → 34 survived adversarial verification → 23 file-groups implemented. Test count grew 687 → 812 as the loop ran, plus 1 more from my own review-driven fixes: 813/813 passing, clean
tscbuild, verified via a from-scratch rebuild.Headline fixes
vault.ts's upload/download/delete andvision.ts's media download made their own rawfetch()calls via private-member casts intoApiClient, bypassing Fix: terminal auth 401 on model-select + UX/UI cleanup #47's refresh-on-401 entirely — a token expiring mid-session hard-failed vault/media ops with no recovery. Now routed through two newApiClientmethods (getBinary,postForm) that share the same refresh-on-401, timeout, and same-origin bearer-gating logic as everything else.aether auth login's polling could previously read as an ordinary "still waiting for approval" instead of a real problem; a 5xx could false-positive the same way. Now correctly distinguished.login.tsandauthRefreshnever adopted the sharedformatErrorLine/hint convention Fix: terminal auth 401 on model-select + UX/UI cleanup #47 established elsewhere — a failed login or failed refresh (arguably the two most consequential auth failures) rendered as a raw, hint-less, uncolored line. Now consistent. Terminal-injection sanitization (sanitizeServerText/sanitizeTerm) extended to close the remaining gaps a hostile server response could exploit.ApiClient.request()now timeout-bounded by default (30s), with the long-running generation/streaming call sites explicitly opted into the existing 120s stream-class bound instead — plusStreamIncompleteError/MalformedResponseErrorso a clean-but-premature stream end or a genuinely malformed 2xx body fails loudly instead of reading as silent success.Independent review findings (fixed before opening this PR)
Two fresh reviewers with zero prior context audited the full diff cold. Both confirmed the core auth/token/transport hardening is sound and tried hard to break it. Real findings, fixed in 3 follow-up commits:
getBinary()'s ownfetch()URL-parse failure on a hostilemedia_urlcould echo raw attacker bytes (OSC/CSI escape sequences) straight to the terminal — the one gap left after this same loop sanitized every other server-string path. Closed at the source (getBinary) plus the two consumer call sites (media.ts,slash_media.ts).vision.ts's image/video/storyboard generation,scaffold.ts,port.ts) — any of these that legitimately ran past 30s would now fail where they used to succeed. Fixed.Two review comments were left as documented, non-blocking follow-up (both explicitly not blocking): the "Simplify" phase's error-line consolidation didn't reach every surface (
media.ts/slash_media.ts/run.tsstill hand-roll a similar-but-not-identical error format — pre-existing scope gap, not a new break), and a pre-existing (unchanged by this branch) edge case where a truly empty 2xx/auth/refreshbody still throws a slightly confusing message inside the safe, formatted error path.Test plan
npm run buildclean from a from-scratchdist/rebuildnpm test— 813/813 passingmain🤖 Generated with automated LOOP-01/LOOP-06 audit loop + independent review, orchestrated via Claude Code.