fix: name the host, the reason, and the setting when a request fails - #200
Open
tonychang04 wants to merge 2 commits into
Open
fix: name the host, the reason, and the setting when a request fails#200tonychang04 wants to merge 2 commits into
tonychang04 wants to merge 2 commits into
Conversation
`insta login --device` against a terminated insta-oss box printed exactly
`error: fetch failed`. The CLI knew the host it dialled and which setting
pointed it there, and said neither, so the only way to find out was to read
~/.insta/config.json.
Three changes, all on the failure path:
- A transport failure now throws NetworkError instead of leaking undici's
`TypeError: fetch failed`: it names the host and translates the cause code
into a reason ("connect timeout", "DNS lookup failed", ...). The original
error stays as `cause`, so telemetry still lifts `cause.code`.
- Any error against a target that is not the cloud default carries two lines
under it: the URL with the setting that chose it (INSTA_API_URL, INSTA_ENV,
`login --api-url`, `env use`), and the one command back to InstaCloud.
On prod nothing is added, so the common error is unchanged.
- `env show` and `status` grow a `source:` line, and `status` stops reporting
an unreachable host as "(not logged in)". `env show` also marks the mcp and
skills rows as the cloud fallback they are when apiUrl is custom.
Provenance is derived, never stored and never probed. `env use` only ever
writes a host from the env table, so a persisted apiUrl outside it can only
have come from `login --api-url`. Nothing asks the host what it is: the case
this exists for is a host that does not answer.
Before:
error: fetch failed
After:
error: cannot reach api.98-87-8-168.sslip.io (connect timeout)
target: https://api.98-87-8-168.sslip.io (saved by `insta login --api-url`)
for InstaCloud: insta env use prod
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/api.ts">
<violation number="1" location="src/api.ts:17">
P2: When deploy remaps a 409 error, it drops the new `ApiError.context`, so non-default target failures omit the URL and provenance. Preserve `e.context` when constructing the replacement error.</violation>
<violation number="2" location="src/api.ts:95">
P2: When `login --env <name>` fails before persistence, `targetContext()` reports the target as `--api-url flag` because it derives provenance without the command's `--env` option. Pass the explicit target provenance into `ApiClient` so the error identifies `--env=<name>` correctly.</violation>
<violation number="3" location="src/api.ts:112">
P2: When the peer closes the connection while `res.text()` reads the response body, the new catch has already finished, so the CLI still leaks `TypeError: fetch failed` without the host or reason. Include body consumption in the `NetworkError` try/catch path.</violation>
</file>
<file name="src/target.ts">
<violation number="1" location="src/target.ts:45">
P2: When `login --api-url` is given a known environment URL, this labels the persisted target as `env use` even though login writes it too. Record provenance when persisting or use an ambiguous label for stored named URLs.</violation>
<violation number="2" location="src/target.ts:52">
P2: When `INSTA_API_URL` overrides an already-custom stored URL, unsetting it leaves the CLI on that same custom host, so the displayed `for InstaCloud` recovery is false. Build recovery from both override and stored provenance, or print the steps needed to clear the override and select prod.</violation>
<violation number="3" location="src/target.ts:53">
P3: The `unset INSTA_ENV` recovery branch is unreachable. `source === 'INSTA_ENV=...'` only happens when apiUrl resolves to a named environment's host, so `t.env` is always truthy; every consumer of `recovery` (targetLines, auth.ts, env.ts) prints it only when `env` is null/custom. Remove the branch and let INSTA_ENV fall through to the default recovery, or drop the env-gating and show it for named environments.</violation>
<violation number="4" location="src/target.ts:74">
P2: When the resolved target is staging, `t.env` is non-null, so this suppresses the recovery command even though staging is not the cloud default. Add the recovery line for every `t.env !== DEFAULT_ENV` target.</violation>
</file>
<file name="src/commands/env.ts">
<violation number="1" location="src/commands/env.ts:25">
P2: When a custom `apiUrl` is combined with `INSTA_MCP_URL` or `INSTA_SKILLS_REPO`, `env show` still calls the selected values cloud fallbacks. Check each override before adding its fallback label.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+112
to
126
| let res: Response | ||
| try { | ||
| res = await this.fetchImpl(this.apiUrl + path, { | ||
| method, | ||
| headers, | ||
| body: body === undefined ? undefined : JSON.stringify(body), | ||
| }) | ||
| } catch (e) { | ||
| // A transport failure, not an HTTP status: there is no response to parse and no 401 to | ||
| // refresh past, so it goes straight out as a NetworkError naming the host and the setting. | ||
| throw new NetworkError(this.apiUrl, (e as { cause?: unknown })?.cause ?? e, await this.targetContext()) | ||
| } | ||
| const text = await res.text() | ||
| let parsed: any = null | ||
| try { parsed = text ? JSON.parse(text) : null } catch { parsed = { raw: text } } |
There was a problem hiding this comment.
P2: When the peer closes the connection while res.text() reads the response body, the new catch has already finished, so the CLI still leaks TypeError: fetch failed without the host or reason. Include body consumption in the NetworkError try/catch path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/api.ts, line 112:
<comment>When the peer closes the connection while `res.text()` reads the response body, the new catch has already finished, so the CLI still leaks `TypeError: fetch failed` without the host or reason. Include body consumption in the `NetworkError` try/catch path.</comment>
<file context>
@@ -80,11 +109,18 @@ export class ApiClient {
- headers,
- body: body === undefined ? undefined : JSON.stringify(body),
- })
+ let res: Response
+ try {
+ res = await this.fetchImpl(this.apiUrl + path, {
</file context>
| // body carries the parsed error payload for callers that branch on machine-readable errors | ||
| // (e.g. template deploy's missing_variables); the message stays the human line. | ||
| constructor(public status: number, msg: string, public body?: any) { super(msg); this.name = 'ApiError' } | ||
| constructor(public status: number, msg: string, public body?: any, public context?: ErrorContext) { super(msg); this.name = 'ApiError' } |
There was a problem hiding this comment.
P2: When deploy remaps a 409 error, it drops the new ApiError.context, so non-default target failures omit the URL and provenance. Preserve e.context when constructing the replacement error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/api.ts, line 17:
<comment>When deploy remaps a 409 error, it drops the new `ApiError.context`, so non-default target failures omit the URL and provenance. Preserve `e.context` when constructing the replacement error.</comment>
<file context>
@@ -5,11 +5,30 @@ import { autoResolveProject, promptChoice, type ProjectItem } from './resolve-pr
// body carries the parsed error payload for callers that branch on machine-readable errors
// (e.g. template deploy's missing_variables); the message stays the human line.
- constructor(public status: number, msg: string, public body?: any) { super(msg); this.name = 'ApiError' }
+ constructor(public status: number, msg: string, public body?: any, public context?: ErrorContext) { super(msg); this.name = 'ApiError' }
+}
+
</file context>
| private noteCache?: ErrorContext | ||
| private async targetContext(): Promise<ErrorContext> { | ||
| if (!this.noteCache) { | ||
| try { this.noteCache = targetLines(await describeTarget(this.apiUrl)) } catch { this.noteCache = [] } |
There was a problem hiding this comment.
P2: When login --env <name> fails before persistence, targetContext() reports the target as --api-url flag because it derives provenance without the command's --env option. Pass the explicit target provenance into ApiClient so the error identifies --env=<name> correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/api.ts, line 95:
<comment>When `login --env <name>` fails before persistence, `targetContext()` reports the target as `--api-url flag` because it derives provenance without the command's `--env` option. Pass the explicit target provenance into `ApiClient` so the error identifies `--env=<name>` correctly.</comment>
<file context>
@@ -57,17 +76,27 @@ export class ApiClient {
+ private noteCache?: ErrorContext
+ private async targetContext(): Promise<ErrorContext> {
+ if (!this.noteCache) {
+ try { this.noteCache = targetLines(await describeTarget(this.apiUrl)) } catch { this.noteCache = [] }
+ }
+ return this.noteCache
</file context>
| else source = '--api-url flag' | ||
|
|
||
| const recovery = | ||
| source === 'INSTA_API_URL' ? 'unset INSTA_API_URL' |
There was a problem hiding this comment.
P2: When INSTA_API_URL overrides an already-custom stored URL, unsetting it leaves the CLI on that same custom host, so the displayed for InstaCloud recovery is false. Build recovery from both override and stored provenance, or print the steps needed to clear the override and select prod.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/target.ts, line 52:
<comment>When `INSTA_API_URL` overrides an already-custom stored URL, unsetting it leaves the CLI on that same custom host, so the displayed `for InstaCloud` recovery is false. Build recovery from both override and stored provenance, or print the steps needed to clear the override and select prod.</comment>
<file context>
@@ -0,0 +1,104 @@
+ else source = '--api-url flag'
+
+ const recovery =
+ source === 'INSTA_API_URL' ? 'unset INSTA_API_URL'
+ : source.startsWith('INSTA_ENV=') ? 'unset INSTA_ENV'
+ : 'insta env use prod'
</file context>
| export function targetLines(t: Target): string[] { | ||
| if (t.env === DEFAULT_ENV) return [] | ||
| const lines = [` target: ${t.apiUrl} (${t.source})`] | ||
| if (!t.env) lines.push(` for InstaCloud: ${t.recovery}`) |
There was a problem hiding this comment.
P2: When the resolved target is staging, t.env is non-null, so this suppresses the recovery command even though staging is not the cloud default. Add the recovery line for every t.env !== DEFAULT_ENV target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/target.ts, line 74:
<comment>When the resolved target is staging, `t.env` is non-null, so this suppresses the recovery command even though staging is not the cloud default. Add the recovery line for every `t.env !== DEFAULT_ENV` target.</comment>
<file context>
@@ -0,0 +1,104 @@
+export function targetLines(t: Target): string[] {
+ if (t.env === DEFAULT_ENV) return []
+ const lines = [` target: ${t.apiUrl} (${t.source})`]
+ if (!t.env) lines.push(` for InstaCloud: ${t.recovery}`)
+ return lines
+}
</file context>
Suggested change
| if (!t.env) lines.push(` for InstaCloud: ${t.recovery}`) | |
| if (t.env !== DEFAULT_ENV) lines.push(` for InstaCloud: ${t.recovery}`) |
| else if (i.stored && normalizeUrl(i.stored) === want) { | ||
| // A stored host the env table knows was written by `env use` (or by `login --env`, which | ||
| // writes the same value); anything else was a literal URL the user typed at `login --api-url`. | ||
| source = envForApiUrl(i.stored) ? 'saved by `insta env use`' : 'saved by `insta login --api-url`' |
There was a problem hiding this comment.
P2: When login --api-url is given a known environment URL, this labels the persisted target as env use even though login writes it too. Record provenance when persisting or use an ambiguous label for stored named URLs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/target.ts, line 45:
<comment>When `login --api-url` is given a known environment URL, this labels the persisted target as `env use` even though login writes it too. Record provenance when persisting or use an ambiguous label for stored named URLs.</comment>
<file context>
@@ -0,0 +1,104 @@
+ else if (i.stored && normalizeUrl(i.stored) === want) {
+ // A stored host the env table knows was written by `env use` (or by `login --env`, which
+ // writes the same value); anything else was a literal URL the user typed at `login --api-url`.
+ source = envForApiUrl(i.stored) ? 'saved by `insta env use`' : 'saved by `insta login --api-url`'
+ } else if (i.stored === null && env === DEFAULT_ENV) source = 'built-in default'
+ // Nothing in the environment or on disk accounts for this URL, so it came from the flag the
</file context>
Comment on lines
+25
to
+26
| info(`mcp: ${mcpUrl} (${mcpServer}${env ? '' : ', cloud fallback'})`) | ||
| info(`skills: ${skills}${env ? '' : ' (cloud fallback)'}`) |
There was a problem hiding this comment.
P2: When a custom apiUrl is combined with INSTA_MCP_URL or INSTA_SKILLS_REPO, env show still calls the selected values cloud fallbacks. Check each override before adding its fallback label.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/env.ts, line 25:
<comment>When a custom `apiUrl` is combined with `INSTA_MCP_URL` or `INSTA_SKILLS_REPO`, `env show` still calls the selected values cloud fallbacks. Check each override before adding its fallback label.</comment>
<file context>
@@ -7,17 +7,24 @@
+ info(`source: ${target.source}`)
+ // A custom apiUrl has no matching mcp/skills entry, so resolveEnv falls back to the cloud's.
+ // Say so: an unlabelled cloud mcp host under an insta-oss api host reads as a matched pair.
+ info(`mcp: ${mcpUrl} (${mcpServer}${env ? '' : ', cloud fallback'})`)
+ info(`skills: ${skills}${env ? '' : ' (cloud fallback)'}`)
+ if (!env) info(`switch: ${target.recovery}`)
</file context>
Suggested change
| info(`mcp: ${mcpUrl} (${mcpServer}${env ? '' : ', cloud fallback'})`) | |
| info(`skills: ${skills}${env ? '' : ' (cloud fallback)'}`) | |
| info(`mcp: ${mcpUrl} (${mcpServer}${env || process.env.INSTA_MCP_URL ? '' : ', cloud fallback'})`) | |
| info(`skills: ${skills}${env || process.env.INSTA_SKILLS_REPO ? '' : ' (cloud fallback)'}`) |
|
|
||
| const recovery = | ||
| source === 'INSTA_API_URL' ? 'unset INSTA_API_URL' | ||
| : source.startsWith('INSTA_ENV=') ? 'unset INSTA_ENV' |
There was a problem hiding this comment.
P3: The unset INSTA_ENV recovery branch is unreachable. source === 'INSTA_ENV=...' only happens when apiUrl resolves to a named environment's host, so t.env is always truthy; every consumer of recovery (targetLines, auth.ts, env.ts) prints it only when env is null/custom. Remove the branch and let INSTA_ENV fall through to the default recovery, or drop the env-gating and show it for named environments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/target.ts, line 53:
<comment>The `unset INSTA_ENV` recovery branch is unreachable. `source === 'INSTA_ENV=...'` only happens when apiUrl resolves to a named environment's host, so `t.env` is always truthy; every consumer of `recovery` (targetLines, auth.ts, env.ts) prints it only when `env` is null/custom. Remove the branch and let INSTA_ENV fall through to the default recovery, or drop the env-gating and show it for named environments.</comment>
<file context>
@@ -0,0 +1,104 @@
+
+ const recovery =
+ source === 'INSTA_API_URL' ? 'unset INSTA_API_URL'
+ : source.startsWith('INSTA_ENV=') ? 'unset INSTA_ENV'
+ : 'insta env use prod'
+
</file context>
None of these reproduce under `npm test`, because the suite runs on Node and never drives the shipped artifact. - Bun, not just Node. The compiled binaries install.sh serves put the failure code on the error ITSELF, with no `cause`, spelled in Bun's own CamelCase, so every reason fell through to a raw `ConnectionRefused`. Bun also reports a connect TIMEOUT as `ConnectionRefused`, so it cannot tell a terminated box from a refused port; it now renders as the honest "could not connect" rather than a confident lie about the one host this feature exists for. Node's ECONNREFUSED really does mean refused and keeps the precise wording. - `--api-url` at a dead host, with prod already persisted, advised "insta env use prod" — a no-op that prints "already on prod". The advice now undoes whatever actually chose the host: "drop --api-url". - `--json` published `source` as prose containing backticks. It now publishes a stable token (`saved-api-url`, `env-api-url`, ...) and keeps the prose for the terminal, so an agent is not branching on a sentence we will reword. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The incident
A real session today, verbatim:
That is the entire output. The cause:
~/.insta/config.jsonstill heldapiUrl: https://api.98-87-8-168.sslip.io, an insta-oss test box saved earlierby
insta login --api-url. The box had since been terminated, so the fetchfailed at the network layer. The cloud API was fine the whole time. There was
no way to see any of this without opening the config file by hand.
Earlier in the same session, against a live insta-oss daemon:
Also true, also unhelpful: it never named the host that answered, so it was not
obvious the CLI was talking to a daemon rather than to InstaCloud.
The CLI knew the host it dialled and which setting pointed it there. It said
neither.
Before / after
Unreachable host, the exact incident config:
Same host via the env var, the second line changes and nothing else:
Cloud-only flow against a daemon:
insta env showon a custom target:On the cloud default, errors gain nothing:
targetLines()returns[]forprod, so the normal path is byte-identical to today.What changed
NetworkErrorreplaces undici's leakedTypeError: fetch failed. It namesthe host and translates the cause code into a reason (
connect timeout,DNS lookup failed,connection refused, TLS cases). The original error iskept as
cause, sotelemetry.tsstill liftscause.code.under it: the URL with the setting that chose it, and the one command back to
InstaCloud.
env showandstatusgain asource:line.statusstops reporting anunreachable host as
(not logged in), which was a flat misdiagnosis and sentthe user hunting for a login problem that did not exist.
env showlabels themcpandskillsrows as the cloud fallback they arewhen
apiUrlis custom. Previously a cloud MCP host printed unlabelleddirectly beneath an insta-oss API host, which reads as a matched pair and is
exactly the "CLI on one deployment, agents on another" failure
env.ts's ownheader says must never happen.
--jsonon both commands gainssource(andstatusgainsunreachable).Additive only; no field was removed or renamed. No command or flag changed.
What I deliberately did not build
The obvious design is: add
GET /inforeturning{product, version}, call iton
loginandenv use, store the answer beside the URL, and label the targetfrom the stored kind. Do not do this. Three reasons, in order of weight:
unreachable host. A design whose identification step is a request to that
host degrades to "unknown" in precisely the case that motivated it, having
spent a round trip to learn nothing.
env useonly ever writes a host from the env table, so a persistedapiUrlthat is not one of those can only have come fromlogin --api-urlor a hand edit.
INSTA_API_URLandINSTA_ENVare readable from theenvironment. That is the entire decision tree, it is pure, it costs nothing,
and it works with the network down. See
src/target.ts.reused. A label that says
insta-ossabout a host that is now somethingelse is worse than no label, because it is confidently wrong.
Two corollaries worth writing down:
probe, and inherits its staleness. It is also unnecessary: once the 501 names
the host, it is a good message. The remaining gap is one line of copy on the
daemon, not machinery here.
"which box am I on", and the host is that answer.
api.98-87-8-168.sslip.ioidentifies the box;insta-ossdoes not.Nothing here is stored and nothing is probed, so there is no new state to
migrate, invalidate, or get wrong.
Note for whoever does the deferred part
If we do eventually want product identity, it is two fields on an existing
route, not a new one.
GET /healthzis already unauthenticated on both sides(
insta-platform/src/server.ts,insta-oss/src/server.ts) and both returnbyte-identical
{"ok":true}.Gotcha: the platform's
/healthzdeclares a TypeBox response schema(
Type.Object({ ok: Type.Optional(Type.Boolean()) })). Fastify serializesstrictly, so an added
productfield is silently stripped unless the schemais extended too. It will look like the server is ignoring you.
If that lands, probe it only from
env show/status(which already donetwork I/O and are the "where am I" commands), never on every command, never
blockingly on
login, and never store the answer.Related, not fixed here
insta-oss/src/auth.tsis the onenotCloudmessage in the daemon thatomits the product name; every other one ends "insta-oss is a single-tenant
local runtime". One-word fix, daemon side.
.insta/project.jsonrecords no target, so a cloud project id stays "linked"while the CLI points at a daemon. Arguably a bigger "which box" hazard than
the product label.
COMPATIBILITY.mdalready notes that insta-mcp maps every status >= 500 toplatform_error: "upstream error, retry", so the daemon's 501 guidance neverreaches an agent at all.
Testing
npm run typecheckclean. 864 tests pass, newtest/target.test.tscoversprovenance derivation, the line builder, the cause-code translation tables, and
the client throwing
NetworkErroron a dead transport.test/github-source.integration.test.tsfails on this machine and also failsidentically on unmodified
mainbecause the localgitpredates--initial-branch. Not related to this change.QA against the shipped binary and a live daemon
The suite was not enough. The compiled binary was driven against a genuinely
terminated box (
api.98-87-8-168.sslip.io), a live insta-oss daemon from mergedmain (
api.3-208-30-50.sslip.io, projectdemo, branches main/feat/rest),staging, and the cloud. Three defects came out of that, all in the second
commit, none of them reproducible under
npm test:failure code on the error itself, with no
cause, in Bun's own CamelCase, soevery reason fell through to a raw
ConnectionRefused. Bun also reports aconnect timeout as
ConnectionRefused, so it cannot distinguish aterminated box from a refused port. It now renders as the honest "could not
connect" instead of a confident lie about the exact host this feature exists
for. Node's own
ECONNREFUSEDkeeps the precise wording.--api-urlat a dead host advised a no-op. With prod already persisted,for InstaCloud: insta env use prodwould print "already on prod". Theadvice now undoes whatever actually chose the host:
drop --api-url.--jsonpublished prose with backticks in it.sourceis now a stabletoken (
saved-api-url,env-api-url,saved-env,env-name,default,flag) with the prose kept for the terminal.Verified good, real output:
Staging gets one line and no recovery line, since staging is InstaCloud:
The cloud is byte-identical to the released binary:
error: unauthorized (HTTP 401)and nothing more.env showmakes no network call, so it stillanswers "where am I" with the host down.
Incidentally confirmed while probing: the live daemon's
GET /healthzreturnsexactly
{"ok":true}, the same bytes as the cloud's. Today there is no way atall to tell the two apart from a response.
Honest remaining gaps
Two states where a user still cannot tell where they are, neither introduced
here and neither fixed here:
die()carries no target. In agent mode,agent session missing, expired, or for another project/environmentnames neither the project nor theenvironment, though its own wording says both matter. It is thrown from
agent.tsas a plain Error, so it never passes through the client that knowsthe target. Fixing it properly means restructuring that catch to tell "no
session file" apart from "session for another host": that is the agent auth
path and does not belong in this PR.
project on a daemon, switch back to prod, and you get a bare
project not found (HTTP 404): prod deliberately suppresses the target lines,and they would not help anyway, because the useful fact is where the project
is, not where you are. The real fix is
.insta/project.jsonrecording thetarget that minted it. Deliberately not bolted on here as a half-measure.
One divergence worth recording: the daemon answers
branch listfor an unknownproject with
{branches: []}and exit 0, whereservices listandsecrets liston the same id return 404. A missing project reads as an emptyone on that route.
🤖 Generated with Claude Code