fix(config): unpin installs frozen on the retired beta-api host - #65
fix(config): unpin installs frozen on the retired beta-api host#65tonychang04 wants to merge 3 commits into
Conversation
readGlobal materialises DEFAULT_API into the object persist() writes back, so whatever the default was at a user's first login is frozen into config.json permanently. beta-api.insta.insforge.dev was the default in v0.0.3..v0.0.16 and went NXDOMAIN on 2026-07-27, leaving those installs hard-broken with no escape except INSTA_API_URL or hand-editing the file — upgrading the binary cannot fix them, and every fresh install looks fine. Treat a retired host as absent on read so it falls through to DEFAULT_API. Resolving on read fixes every invocation immediately; the file heals itself the next time any command persists. A persisted localhost or self-hosted URL is a deliberate choice and still wins, as does INSTA_API_URL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
jwfing
left a comment
There was a problem hiding this comment.
Review: fix(config): unpin installs frozen on the retired beta-api host
Summary: A tight, well-tested read-path fix that treats the retired beta-api.insta.insforge.dev host as absent so frozen configs fall through to DEFAULT_API and self-heal on next persist — clean to merge.
Requirements context
No matching spec/plan found — this repo has no docs/superpowers/ or docs/specs/ directory (only README.md, AGENTS.md, CLAUDE.md, and the developing-insta-cli skill). Assessed against the PR description, AGENTS.md conventions, and the surrounding code. Repo gate per AGENTS.md is npm run typecheck && npm test; I ran both — see Verdict.
Findings
Critical
(none)
Suggestion
- Functionality — completeness of the retirement set (
src/config.ts:32). The fix is only as correct asRETIRED_API_URLSbeing the complete list of hosts the CLI ever shipped as a default and later retired. The PR establishes the history (localhost in v0.0.0–v0.0.2 — deliberately excluded;beta-api…in v0.0.3–v0.0.16;api.instacloud.comfrom v0.0.17). That reasoning is sound and matches the currentDEFAULT_API, so no change is required now — just flagging that this Set is the load-bearing assumption, and any future default-host rotation must add the outgoing host here (with a regression test) or a new cohort silently re-freezes.
Information
- Software engineering — incidental hardening (
src/config.ts:46). The new guardtypeof parsed.apiUrl === 'string' && parsed.apiUrlalso rejects an empty-stringapiUrl, which the previousparsed.apiUrl ?? DEFAULT_APIwould have kept (since??only catches null/undefined). A persisted""would have madefetch(this.apiUrl + path)hit a relative URL — so this is a small correctness improvement beyond the stated scope. Worth a one-line note in the PR body; no action needed. - Security — no security-relevant changes. No new user input reaches SQL/shell/HTTP; the
apiUrlstill comes only from the config file /INSTA_API_URL/--api-url(unchanged trust boundary). The.replace(/\/+$/, '')normalization regex is anchored and linear — no ReDoS. No secrets logged or returned. - Performance — no concerns. One
Set.haslookup on a normalized string perreadGlobal; no new I/O, loops, or allocations in any hot path.
Notes on what I verified
readGlobalreally is the single choke point. Grepped every.apiUrlconsumer: all flow throughApiClient(api.ts:18/62), which loads viareadGlobal(api.ts:16);auth.tsonly setsapiUrlexplicitly on--api-url. Nothing reads the persisted value bypassing the resolver, so the fix reaches every invocation as claimed.- Spread order is correct (
src/config.ts:47):{ ...parsed, apiUrl: … }keepsaccessToken/refreshToken/autoUpdateintact while overriding the stale host — covered by the "keeps the rest of the config intact" test. - Self-heal path is real:
writeGlobal(await readGlobal())(whatApiClient.persist()andupgrade.ts:104-106both do) rewrites the healed value — covered by the "healed value reaches disk" test. - Test suite is meaningful and green: ran
npx vitest run→ 20 files / 121 tests pass;npx tsc --noEmitclean. The 7 new cases cover the retirement path plus the trailing-slash bypass and the three must-not-change guardrails (localhost,INSTA_API_URL, no config file), matching the PR's mutation-check claim (revert → 4 fail / 3 pass).
Verdict
approved (informational; the human still gives the explicit GitHub approval via the approve flow). Zero Critical findings — the two items above are non-blocking. Posted as a COMMENT per the verdict rule.
… backend comment The retired host was a different deployment, not a rebrand: beta-api resolved to insta-beta-api-lb in us-west-1, while DEFAULT_API is insta-platform-prod-alb in us-east-2. So a session minted there is not valid at the new host, and repointing the URL alone would have traded 'fetch failed' for an opaque 401 that reads as a bug in this migration. Drop accessToken/refreshToken/user alongside the host and print one stderr line pointing at 'insta login'. stderr, not stdout, so --json output stays parseable. The 'same backend, branded domain' comment that implied portability was wrong; corrected in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Follow-up commit The retired host was a different deployment, not a rebrand.
Separate deployments in separate regions. So an Now drops The false comment is corrected in place rather than left to mislead the next reader. Tests: 3 added (session dropped, notice on stderr and not stdout, healthy config keeps its session). 124 pass, Open question that is not this PR's to answer: whether accounts and projects created against the beta deployment were carried over to prod. If they weren't, re-logging in gets these users a working CLI pointed at an account that doesn't hold their old projects. That's a data question for a human, and it doesn't block this — the current behaviour for them is a CLI that cannot reach any host at all. |
Not the error message — requireProject already dies with an actionable 401 hint and the stderr note covers the rest. The reason is that api.ts's 401 path POSTs the stored refresh token to whatever apiUrl now resolves to, which would send a credential minted by the retired deployment to a different one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@qa-bot reviewed Conceded: my justification for clearing the session was weak. I wrote that keeping the tokens would swap Kept, for the reason that does hold: it would send one deployment's credential to another. Adopted: the post-release triage trap. Resolve-on-read leaves the dead host in the file until something persists, so Added to the release note section below. Heal triggers confirmed as Boundary noted, not changed: uppercase, |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
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/config.ts">
<violation number="1" location="src/config.ts:55">
P2: With `INSTA_API_URL` set, retired configurations skip session cleanup and the recovery notice. An authenticated request that gets a 401 can then POST the retired deployment’s refresh token to the env-selected endpoint; detect/clear the retired persisted URL before applying the env API override.</violation>
</file>
<file name="test/config-api-url.test.ts">
<violation number="1" location="test/config-api-url.test.ts:58">
P3: A failed assertion in the stderr/stdout test leaves both process stream spies installed because cleanup is performed only after the assertions. Restoring the spies in a `finally` block or from `afterEach` would keep one test failure from contaminating later tests.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| try { | ||
| const parsed = JSON.parse(await readFile(GLOBAL_FILE, 'utf8')) as GlobalConfig | ||
| return { ...parsed, apiUrl: envApi ?? parsed.apiUrl ?? DEFAULT_API } | ||
| if (envApi) return { ...parsed, apiUrl: envApi } |
There was a problem hiding this comment.
P2: With INSTA_API_URL set, retired configurations skip session cleanup and the recovery notice. An authenticated request that gets a 401 can then POST the retired deployment’s refresh token to the env-selected endpoint; detect/clear the retired persisted URL before applying the env API override.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/config.ts, line 55:
<comment>With `INSTA_API_URL` set, retired configurations skip session cleanup and the recovery notice. An authenticated request that gets a 401 can then POST the retired deployment’s refresh token to the env-selected endpoint; detect/clear the retired persisted URL before applying the env API override.</comment>
<file context>
@@ -19,32 +19,55 @@ export type GlobalConfig = {
- const persisted =
- typeof parsed.apiUrl === 'string' && parsed.apiUrl && !isRetired(parsed.apiUrl) ? parsed.apiUrl : undefined
- return { ...parsed, apiUrl: envApi ?? persisted ?? DEFAULT_API }
+ if (envApi) return { ...parsed, apiUrl: envApi }
+ if (typeof parsed.apiUrl === 'string' && isRetired(parsed.apiUrl)) {
+ // Drop the stored session with the host: it was minted by a different deployment (see
</file context>
| await (await load()).readGlobal() | ||
| expect(err).toHaveBeenCalledWith(expect.stringContaining(RETIRED)) | ||
| expect(out).not.toHaveBeenCalled() | ||
| err.mockRestore(); out.mockRestore() |
There was a problem hiding this comment.
P3: A failed assertion in the stderr/stdout test leaves both process stream spies installed because cleanup is performed only after the assertions. Restoring the spies in a finally block or from afterEach would keep one test failure from contaminating later tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/config-api-url.test.ts, line 58:
<comment>A failed assertion in the stderr/stdout test leaves both process stream spies installed because cleanup is performed only after the assertions. Restoring the spies in a `finally` block or from `afterEach` would keep one test failure from contaminating later tests.</comment>
<file context>
@@ -35,11 +35,32 @@ test('a persisted retired host is replaced by the current default', async () =>
+ await (await load()).readGlobal()
+ expect(err).toHaveBeenCalledWith(expect.stringContaining(RETIRED))
+ expect(out).not.toHaveBeenCalled()
+ err.mockRestore(); out.mockRestore()
+})
+
</file context>
The bug
readGlobalmaterialisesDEFAULT_APIinto the object it returns, andpersist()writes thatwhole object back after login. So the default in force at a user's first login becomes a
literal in
~/.insta/config.jsonand stays there forever:There is no migration anywhere in the tree, and
login/loginOauthonly callsetApiUrlwhen--api-urlis passed — so re-logging in does not repoint an existing install either.DEFAULT_APIwashttps://beta-api.insta.insforge.devin v0.0.3 (2026-07-03) through v0.0.16(2026-07-15 14:13Z); v0.0.17 moved it to
https://api.instacloud.com. That host wentNXDOMAIN on 2026-07-27 when the
insta.insforge.devzone was emptied, so everyone who firstlogged in during that window is hard-broken right now, with no escape but
INSTA_API_URLorhand-editing the file.
Two things make it stay invisible:
autoUpdatehas been on by default since 2026-07-04, so thoseinstalls keep upgrading the binary while staying pinned; and fresh installs have been correct all
along, so anyone checking on a clean machine sees it working.
The fix
Treat a retired host as absent on read, so it falls through to
DEFAULT_API.Resolving on read (rather than rewriting the file inside a read function) fixes every invocation
immediately, and the file heals itself the next time any command persists —
ApiClient.persist()or
insta upgrade --auto.readGlobalis the single choke point:ApiClient.load()is the onlyconsumer of
apiUrl, so nothing bypasses it.Deliberately not touched: a persisted
localhostor self-hosted URL still wins — those arereal choices, and the very first release (v0.0.0–v0.0.2) defaulted to
http://localhost:8080,which we must not rewrite.
INSTA_API_URLstill beats everything, including the retired host;explicit beats ambient.
Scoped to the exact string the CLI ever wrote (
https://…, trailing slashes tolerated). Anhttp://variant hand-typed via--api-urlis left alone as a deliberate value.Tests
test/config-api-url.test.ts, 7 cases — the retirement path, and the guardrails that must notchange. Verified they can actually fail: with
src/config.tsreverted, 4 fail and 3 pass —the 3 that pass are exactly the must-not-change cases (localhost,
INSTA_API_URL, no config file).npx tsc --noEmitclean; full suite 20 files / 121 tests green.Blast radius
One function, read path only. Worst case for a user not in the affected cohort is nil — their
value is untouched. No command or flag surface changes, so no
skills/insta/cli-reference.mdupdate needed.
Summary by cubic
Treat the retired beta API host as absent when reading global config so pinned installs fall back to https://api.instacloud.com, and drop the old session to prevent cross-deployment token refresh. Prints a one-line stderr notice suggesting re-login; stdout stays clean for
--json.readGlobal, ignorehttps://beta-api.insta.insforge.dev(trailing slashes ok) and use the default.accessToken/refreshToken/userand print one stderr notice to runinsta login(stdout untouched). This avoids sending a retired deployment’s refresh token to a different one via the 401 refresh path.INSTA_API_URLstill wins;localhost/self-hosted URLs remain unchanged.test/config-api-url.test.tsto cover host retirement, session drop, stderr notice, and guardrails.Written for commit 5b2b6b1. Summary will update on new commits.
Release note
After this ships,
jq .apiUrl ~/.insta/config.jsonis no longer a valid way to check whethera user is still pinned. Resolve-on-read means the file keeps the retired host until something
persists (
login,logout,upgrade --auto on|off, or a successful token refresh — which astale token never reaches). A fully-fixed user who only runs read commands still has the old
value on disk.
The check that reflects reality:
Credit: @qa-bot found this in review.