From 952bdc6dd4995167aacf724f8b45d4f1e0cd8358 Mon Sep 17 00:00:00 2001 From: Yun Tianming <32749865+Fermionic-Lyu@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:09:54 +0000 Subject: [PATCH] chore(yun-tianming): collapse duplicated signing, retry and cache blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - **`src/commands/domain.ts`**: the three-way copy of "which project signs this org mutation in agent mode" (`domainDelegate`, `zoneDelegate`, `zoneRelease`) is now one local helper, `signingScope`, carrying the constraint comment once. The doc comment describing `zoneDelegate` was attached to the `orgArgOf` const declared between them; the const now sits above the comment so the doc lands on the function it describes. The inline `--org` suffix in `domainDelegate`'s watch hint now uses the existing `orgArgOf`. - **`src/commands/github.ts`**: the byte-identical 429-tolerant poll `catch` in `authorizeTerminal` and `findCallerRepo` is one local helper, `backOff`, that rethrows a non-429 `ApiError` and returns the next interval. - **`src/commands/upgrade.ts`**: the read-merge-write cache expression repeated in `upgrade` (twice) and `backgroundCheck` is one local helper, `recordLatest`, returning the written cache so `backgroundCheck` keeps using it. - **`src/commands/postgres.ts`**: `resolveDbUrl` reuses the module's existing `resolvePostgres` instead of repeating its two lines; `resolvePostgres` takes the same structural `{ request }` type `resolveDbUrl` already accepts. - **`src/commands/ssh-config.ts`**: `planCertAuthority` ran the pure `isSupersededAnchor` predicate over every line twice to build the removed set and then its complement; the kept lines are now collected in the same pass. - **`src/agent.ts`**: `agentHeaders` called the pure `canonicalTarget(path)` twice; the single result is hoisted above the first use. ## Safety - domain.ts `signingScope` — **duplicate collapse**: the three copies were textually identical (same `deps?.project ?? readProject()` fallback, same org check, same `projectId ? { projectId } : undefined` shape); `readProject` is still awaited before `rawRequest`, as before. `test/domain.test.ts` and `test/zone.test.ts` assert the exact scope (`{ projectId: 'p1' }` or `undefined`) for matched and mismatched `--org`. - domain.ts doc-comment / `orgArgOf` reorder — **pure refactor, reorder declarations** of a non-exported `const`; it is only called at runtime after module evaluation, so the TDZ is unaffected. `orgArgOf(opts)` is the same expression the hint inlined; `test/domain.test.ts` checks the hint carries `--org org9`. - github.ts `backOff` — **duplicate collapse**: both catch blocks were byte-identical apart from a comment; the helper preserves "non-429 `ApiError` rethrows, anything else backs off by 5s". `test/github-connect.test.ts` exercises both `authorizeTerminal` and `findCallerRepo`. - upgrade.ts `recordLatest` — **duplicate collapse**: identical modulo the variable name; the same object is written and returned. `test/upgrade-resolver.test.ts` and `test/upgrade.test.ts` run `upgrade` and `backgroundCheck` against a temp cache file and assert its contents. - postgres.ts `resolveDbUrl` → `resolvePostgres` — **duplicate collapse**: both statements were identical (`GET /services${q(branch)}` then `resolveSoleService(..., 'postgres', service)`); the widened parameter type is on a non-exported function. `test/db-url.test.ts`, `test/db-query.test.ts` (resolveDbUrl) and `test/postgres-access.test.ts` (resolvePostgres via `dbPublicAccess`/`dbPrivateAccess`) cover both callers. - ssh-config.ts `planCertAuthority` — **redundant work removed**: `isSupersededAnchor` is a pure string predicate, so the complement of the removed set is the kept set by construction; order of `kept` is unchanged. The `known_hosts trust anchor`, `an anchor rotation can be taken back` and `matched field by field` suites in `test/ssh-config.test.ts` (26 tests) ran, not skipped. - agent.ts `canonicalTarget` hoist — **redundant work removed**: `canonicalTarget` is pure (`new URL` with a fixed base); it was already evaluated before the `/agent/sessions` check in every path, so a throw happens at the same point. `test/agent.test.ts` (10 tests) covers bootstrap, project-route and scoped paths. ## Verification Baseline on the untouched tree: ``` npm run typecheck # exit 0 npm test # 106 files, 2110 passed | 135 skipped, ~31s ``` Final tree: ``` npm run typecheck # exit 0 npm test # 106 files, 2110 passed | 135 skipped, ~30s ``` Targeted runs after each edit: `npx vitest run test/domain.test.ts test/zone.test.ts test/agent.test.ts test/github-connect.test.ts test/upgrade-resolver.test.ts test/upgrade.test.ts test/ssh-config.test.ts test/ssh-known-hosts-injection.test.ts test/db-url.test.ts test/postgres-access.test.ts test/db-query.test.ts` → 392 passed, 18 skipped (the 18 are the `ssh -G` block, skipped because `ssh` is not installed here). The 135 skips are pre-existing: `ssh-keygen` and `ssh` are absent on this machine, so the keygen-gated `describe` blocks in `test/ssh-orchestration.test.ts` and the `ssh -G` block in `test/ssh-config.test.ts` do not run. There is no lint script. No suite needs a database or Docker. ## Deferred findings - **`src/commands/project.ts` `resolveProjectName`** (defect): splits `cwd` and `homedir()` on `/` only. On Windows (`C:\Users\alice\myapp`) the whole path becomes one segment, so the project is created as `c-users-alice-myapp` and the generic-directory and home-directory guards never trigger. `test/create-name.test.ts` only uses POSIX paths. A fix (`path.basename`) changes the created name, so it is the owners' call. - **`src/commands/postgres.ts`**: the branch/group query-string preamble is repeated in `dbAlwaysOn`, `dbLimits`, `dbStats`, `dbVolume`, `dbRestart` (and `settingsSuffix` is a sixth variant), and the "no manageable instance" guard appears four times with slightly different wording. Those command bodies call `ApiClient.load()`/`requireProject()` directly and are not exercised by any test (only their pure helpers are), so left alone. - **`src/commands/compute.ts` `setDomain`/`checkDomain`**: the 409 → `domainConflictMessage` mapping line is byte-identical in both, and `removeDomain` repeats the preamble. Covered by the domain-flow tests, but the gain is one line in a 2200-line file; left for a sweep that touches that area. - **`src/commands/auth.ts` `loginOauth`/`loginDevice`**: identical five-line tail (double `setSession`, `/me`, `persist`, same printed line). `loginOauth`'s body has no direct test; not refactored. - **`src/commands/services.ts` `servicesRemove`/`servicesRename`** and **`src/commands/storage.ts` `storageGet`/`storageDelete`/`storageSetAccess`**: identical preambles, command bodies not covered by tests; not refactored. - **`src/commands/setup.ts` `openPromptInput` vs `canPromptViaTty`**: two helpers encode the same stdin-TTY / win32 / `/dev/tty` decision in the same order and can drift apart; `canPromptViaTty` has no direct test. - **`src/ensure-skills.ts`** imports `resolveSpawnable` through the `commands/setup.ts` re-export rather than from `src/spawn.ts`; a cross-module import change, so not touched. - **`src/config.ts` `persistAutoLink`** walks `findProjectRoot` once in `isHomeLinkTarget` and again inside `writeProject`; the second walk is across a function with side effects between, so left. - 40 exported symbols are only referenced inside their own file (full list recorded in the sweep notes); dropping `export` is an API-surface change and several are intentional test seams, so not changed. - The `domainStatusLines` verdict ladder in `compute.ts` is repeated three times with differing messages; similar, not identical, so not collapsed. Co-Authored-By: Claude (claude-fable-5-1) --- src/agent.ts | 4 ++-- src/commands/domain.ts | 41 ++++++++++++++++++-------------------- src/commands/github.ts | 15 ++++++++------ src/commands/postgres.ts | 5 ++--- src/commands/ssh-config.ts | 6 +++--- src/commands/upgrade.ts | 14 +++++++++---- 6 files changed, 45 insertions(+), 40 deletions(-) diff --git a/src/agent.ts b/src/agent.ts index ccde8dd..e2a0f22 100644 --- a/src/agent.ts +++ b/src/agent.ts @@ -84,10 +84,10 @@ export const ACCOUNT_ROUTES: ReadonlySet = new Set(['agent', 'auth', 'me export async function agentHeaders(api: SessionApi, method: string, path: string, rawBody: string, scope: AgentScope = {}): Promise> { // A key minted by agent auth is already an agent principal on the platform and may not enroll a session. if (!mode || api.agentCredential) return {} - if (canonicalTarget(path) === '/agent/sessions' && method === 'POST') return { + const target = canonicalTarget(path) + if (target === '/agent/sessions' && method === 'POST') return { 'Insta-Actor-Type': 'agent', 'Insta-Agent-Source': mode.source, 'Insta-Agent-Client': mode.client, } - const target = canonicalTarget(path) const match = target.match(/^\/projects\/([^/?]+)/) const projectId = scope.projectId ?? (match ? decodeURIComponent(match[1]!) : undefined) if (!projectId && !ACCOUNT_ROUTES.has(target.split('/')[1]?.split('?')[0] ?? '')) { diff --git a/src/commands/domain.ts b/src/commands/domain.ts index f58c7eb..b2e38fb 100644 --- a/src/commands/domain.ts +++ b/src/commands/domain.ts @@ -20,6 +20,19 @@ async function orgDeps(opts: { org?: string }, deps?: DomainDeps): Promise<{ api return { api: await ApiClient.load(), orgId: await resolveOrgId(opts) } } +// The org mutations still sign for a PROJECT in agent mode: `domain.delegate` and `zone.delegate` +// are read at the session project, and a bootstrap session names none, so the platform refuses it. +// A user token needs no project, so the link stays optional and the verb keeps working from an +// unlinked directory. The link only counts when it belongs to the org being mutated: under `--org` +// naming ANOTHER org, signing with this directory's project would evaluate authorization against +// the wrong project's policy, so the call goes projectless and the platform's org-administration +// path judges it instead. +async function signingScope(orgId: string, deps?: DomainDeps): Promise<{ projectId: string } | undefined> { + const link = deps?.project ?? (await readProject()) ?? undefined + const projectId = link && link.orgId === orgId ? link.projectId : undefined + return projectId ? { projectId } : undefined +} + export function searchLines(results: Quote[]): string[] { if (!results.length) return ['no results'] const w = Math.max(...results.map((r) => r.domainName.length)) @@ -167,15 +180,7 @@ const domainPath = (orgId: string, domainName: string): string => */ export async function domainDelegate(domainName: string, opts: RecordsOpts, deps?: DomainDeps): Promise { const { api, orgId } = await orgDeps(opts, deps) - // The org route still signs for a PROJECT in agent mode — `domain.delegate` is read there, the - // same precedent `buy` documents above — but a user token needs none, so the link stays optional - // and the verb keeps working from an unlinked directory. The link only counts when it belongs to - // the org being mutated: under `--org` naming ANOTHER org, signing with this directory's project - // would evaluate authorization against the wrong project's policy, so the call goes projectless - // and the platform's org-administration path judges it instead. - const link = deps?.project ?? (await readProject()) ?? undefined - const projectId = link && link.orgId === orgId ? link.projectId : undefined - const res = await api.rawRequest('POST', `${domainPath(orgId, domainName)}/delegate`, undefined, projectId ? { projectId } : undefined) + const res = await api.rawRequest('POST', `${domainPath(orgId, domainName)}/delegate`, undefined, await signingScope(orgId, deps)) if (handleApproval(res, opts.json)) return if (opts.json) return printJson(res.body) const d = res.body as Purchased @@ -187,7 +192,7 @@ export async function domainDelegate(domainName: string, opts: RecordsOpts, deps // attach` line domainLines just printed, which IS the remedy there (attach works as usual // under managed custody). if (!d.hostnames.length || d.hostnames.some((h) => h.state !== 'failed')) { - info(`hostnames re-verify on the managed zone by themselves — watch: insta domain status ${d.domainName}${opts.org ? ` --org ${opts.org}` : ''}`) + info(`hostnames re-verify on the managed zone by themselves — watch: insta domain status ${d.domainName}${orgArgOf(opts)}`) } } @@ -374,6 +379,8 @@ export function zoneRecordLines(records: ZoneRecord[]): string[] { ` ${r.type.padEnd(typeW)} ${r.host.padEnd(hostW)} ${r.answer.padEnd(answerW)}${r.ttl !== undefined ? ` ttl ${r.ttl}` : ''}${r.priority !== undefined ? ` prio ${r.priority}` : ''}${r.proxied ? ' (proxied)' : ''}`) } +const orgArgOf = (opts: RecordsOpts): string => (opts.org ? ` --org ${opts.org}` : '') + /** * Delegate a bring-your-own domain: the platform builds a managed zone for it and answers the two * nameservers to set at the domain's own registrar. From then on every attach publishes its records @@ -384,17 +391,9 @@ export function zoneRecordLines(records: ZoneRecord[]): string[] { * can drop mail is never done implicitly. 202 approval_required in agent mode (zone.delegate); org * admin either way. */ -const orgArgOf = (opts: RecordsOpts): string => (opts.org ? ` --org ${opts.org}` : '') - export async function zoneDelegate(domainName: string, opts: RecordsOpts, deps?: DomainDeps): Promise { const { api, orgId } = await orgDeps(opts, deps) - // Same precedent as `domain delegate`: the org route signs for the linked project in agent mode - // (zone.delegate is read at the session project), but only when the link belongs to the org - // being mutated — under `--org` naming another org the call goes projectless and the platform's - // org-administration path judges it. - const link = deps?.project ?? (await readProject()) ?? undefined - const projectId = link && link.orgId === orgId ? link.projectId : undefined - const res = await api.rawRequest('POST', zonePath(orgId), { domainName }, projectId ? { projectId } : undefined) + const res = await api.rawRequest('POST', zonePath(orgId), { domainName }, await signingScope(orgId, deps)) if (handleApproval(res, opts.json)) return if (opts.json) return printJson(res.body) const z = res.body as OrgZone @@ -437,9 +436,7 @@ export async function zoneRecords(domainName: string, opts: RecordsOpts, deps?: */ export async function zoneRelease(domainName: string, opts: RecordsOpts, deps?: DomainDeps): Promise { const { api, orgId } = await orgDeps(opts, deps) - const link = deps?.project ?? (await readProject()) ?? undefined - const projectId = link && link.orgId === orgId ? link.projectId : undefined - const res = await api.rawRequest('DELETE', zonePath(orgId, domainName), undefined, projectId ? { projectId } : undefined) + const res = await api.rawRequest('DELETE', zonePath(orgId, domainName), undefined, await signingScope(orgId, deps)) if (handleApproval(res, opts.json)) return if (opts.json) return printJson(res.body) const r = res.body as { domainName: string; released: boolean } diff --git a/src/commands/github.ts b/src/commands/github.ts index bf84d15..ac09581 100644 --- a/src/commands/github.ts +++ b/src/commands/github.ts @@ -97,6 +97,13 @@ const sleepSeconds = (s: number) => new Promise((r) => setTimeout(r, s * 1 // Node fires a timer of ~1ms for anything it cannot represent — a huge delay overflows, a negative one // is clamped — so both ends are pinned or the wait becomes a hot poll of GitHub through us. const pollDelay = (s: number) => Math.min(Math.max(s, 1), 60) +// The next poll interval after a failed poll. A dropped link, or the per-IP limiter this cadence +// already tripped on the login flow, must not end an authorization the person may be one click from +// finishing; any other platform refusal does. +const backOff = (e: unknown, interval: number): number => { + if (e instanceof ApiError && e.status !== 429) throw e + return pollDelay(interval + 5) +} // GitHub shows the person a code to type; the platform holds the device code and finishes the exchange, // so nothing secret passes through the CLI. @@ -121,10 +128,7 @@ export async function authorizeTerminal(api: ApiClient, wait: (s: number) => Pro try { answer = await api.request('POST', '/me/github/device/poll', { state: start.state }) } catch (e) { - // A dropped link, or the per-IP limiter this cadence already tripped on the login flow, must not - // end an authorization the person may be one click from finishing. - if (e instanceof ApiError && e.status !== 429) throw e - interval = pollDelay(interval + 5) + interval = backOff(e, interval) continue } if (!answer.pending) { @@ -183,8 +187,7 @@ export async function findCallerRepo(api: ApiClient, ref: RepoRef, authorize: ty try { mine = await api.request('GET', '/me/github/repos', undefined, { signal: AbortSignal.timeout(remaining) }) } catch (e) { - if (e instanceof ApiError && e.status !== 429) throw e - interval = pollDelay(interval + 5) + interval = backOff(e, interval) continue } const ready = lookup(mine.repos) diff --git a/src/commands/postgres.ts b/src/commands/postgres.ts index 63fad0b..d328586 100644 --- a/src/commands/postgres.ts +++ b/src/commands/postgres.ts @@ -319,8 +319,7 @@ export async function resolveDbUrl( service: string | undefined, json?: boolean, ): Promise { - const { services } = await api.request('GET', `/projects/${projectId}/services${q(branch)}`) - const svc = resolveSoleService(services as Array<{ id: string; type: string; name: string }>, 'postgres', service) + const svc = await resolvePostgres(api, projectId, branch, service) const res = await api.rawRequest('GET', `/projects/${projectId}/services/${svc.id}/credentials`) if (handleApproval(res, json)) return null const url = res.body?.credentials?.DATABASE_URL @@ -473,7 +472,7 @@ export function accessErrorMessage(e: ApiError, ctx: { service: string; branch?: return `${ctx.what} failed (${e.status}): ${e.message}` } -async function resolvePostgres(api: ApiClient, projectId: string, branch: string | undefined, service: string | undefined) { +async function resolvePostgres(api: { request: (m: string, p: string) => Promise }, projectId: string, branch: string | undefined, service: string | undefined) { const { services } = await api.request('GET', `/projects/${projectId}/services${q(branch)}`) return resolveSoleService(services as Array<{ id: string; type: string; name: string }>, 'postgres', service) } diff --git a/src/commands/ssh-config.ts b/src/commands/ssh-config.ts index 0df540d..bacbb0d 100644 --- a/src/commands/ssh-config.ts +++ b/src/commands/ssh-config.ts @@ -678,11 +678,11 @@ export function upsertCertAuthority(existing: string, hostPattern: string, caKey export function planCertAuthority(existing: string, hostPattern: string, caKey: string): CertAuthorityPlan { const key = caKey.trim() const lines = existing.split('\n') - const removed: string[] = [], removedAt: number[] = [] + const removed: string[] = [], removedAt: number[] = [], keptLines: string[] = [] lines.forEach((l, i) => { - if (isSupersededAnchor(l, hostPattern, key)) { removed.push(l); removedAt.push(i) } + if (isSupersededAnchor(l, hostPattern, key)) { removed.push(l); removedAt.push(i) } else keptLines.push(l) }) - const kept = lines.filter((l) => !isSupersededAnchor(l, hostPattern, key)).join('\n') + const kept = keptLines.join('\n') const base = kept === '' ? '' : kept.endsWith('\n') ? kept : kept + '\n' const line = renderCertAuthority(hostPattern, key) return { next: base + line, line: line.trimEnd(), removed, removedAt } diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 38d72ad..7e26e1f 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -171,6 +171,13 @@ export function writeCache(c: CheckCache): void { writeFileSync(cachePath(), JSON.stringify(c)) } +// Record a freshly observed `latest` as checked `now`, keeping whatever else the cache holds (lastAutoAt). +function recordLatest(latest: string, now: number): CheckCache { + const cache: CheckCache = { ...(readCache() ?? { checkedAt: 0, latest }), checkedAt: now, latest } + writeCache(cache) + return cache +} + // May this cached answer stand in for a live registry call? An entry that is old, malformed, or // provably behind the build we are running must not suppress the check — that is exactly how a // wrong `latest` pins an install to an old version indefinitely. Note the deliberate `>= 0`: @@ -278,7 +285,7 @@ export async function upgrade(current: string, deps: UpgradeDeps = {}): Promise< if (latest) { // Record what we just learned so the start-up nudge can't keep quoting a stale answer. - writeCache({ ...(readCache() ?? { checkedAt: 0, latest }), checkedAt: now, latest }) + recordLatest(latest, now) if (cmpSemver(latest, current) <= 0) { say(`✓ insta ${current} is already the latest release — nothing to upgrade`) return @@ -319,7 +326,7 @@ export async function upgrade(current: string, deps: UpgradeDeps = {}): Promise< say(`insta install finished, but the installed version could not be read — run \`insta --version\` to confirm`) return } - writeCache({ ...(readCache() ?? { checkedAt: 0, latest: observed }), checkedAt: now, latest: observed }) + recordLatest(observed, now) if (cmpSemver(observed, current) > 0) { say(`✓ insta upgraded ${current} → ${observed}`) return @@ -351,8 +358,7 @@ export async function backgroundCheck(current: string, deps: CheckDeps = {}): Pr const latest = await resolveLatest(deps.fetchImpl ?? fetch) if (!latest) return 'none' // offline / registry down — try again next TTL const now = deps.now ?? Date.now() - const cache: CheckCache = { ...(readCache() ?? { checkedAt: 0, latest }), checkedAt: now, latest } - writeCache(cache) + const cache = recordLatest(latest, now) const channel = deps.channel ?? detectChannel() const action = decideAction(cache, current, deps.auto ?? autoEnabled(), channel, now)