chore(yun-tianming): drop review cites and change narration, dedupe api request path

Merged
FE
1mo ago
+107-151

Summary

  • Comment cleanup across 25 src/ files. Removed review-round citations (cubic P1/P2, r2d2 round N, spec 4.1 / FAQ 7.x, console PR #290, insta-platform #385/#357, #138), dated incident notes (seen on prod 2026-09-10, insta-e2e run 31284364163, staging 2026-08-25 … insta-warm-00178a-16, verified … on 2026-09-11), provenance (Ported from firth ×3, Design doc §5, Spec 4.4, (user report)), and change narration (used to, previously, no longer, at first, the first version, an earlier round declined, which is exactly how this was missed, the three retellings of the retired Neon path in db.ts and one in metrics.ts, the pro|enterprise history in billing.ts, the beta-api retirement in env.ts/config.ts, the v0.0.46 anecdote in upgrade.ts). Where a story carried a constraint, the constraint survives in one line (e.g. the lock-takeover inode rule in compute.ts, the CA-key shape checks in ssh-config.ts, the archive-digest rule in deploy-archive.ts).
  • Duplicated comment blocks collapsed. The 8-line -- argv-splitting paragraph attached to EXEC_OPTIONS in compute.ts repeated the one on splitExecArgs and was removed; index.ts's __ssh-ensure-cert note shrinks to its constraint (the long form stays in compute.ts); the two consecutive "whole blob, not first field" paragraphs in ssh-config.ts become one; the duplicated "URL never appears in PowerShell source" paragraph in util.ts keeps only the smart-quote fact the outer doc lacks; a redundant first line above EXPLICIT_ADDRESS in github-source.ts is dropped.
  • Documentation drift fixed. feedback.ts's submit doc said "10s timeout" while FEEDBACK_TIMEOUT_MS = 15_000; it now names the constant. In db.ts the doc comment describing dbVolumeLines ("reads the CANONICAL volume* names only …") sat above fmtBytes; it is moved onto dbVolumeLines, which had no comment.
  • Dead code. src/flyctl-build.ts imported info from ./util.js and never used it (orphaned by commit f94dfe8). src/commands/setup.ts re-exported whichOnPath from ../spawn.js; nothing imports it through setup.ts.
  • Redundant work removed in src/api.ts. fetch() serialized the request body twice (once for the agent-signature payload, once for the fetch body); it now serializes once and uses the same string for both. request() duplicated rawRequest()'s >= 400 → ApiError line; it now calls rawRequest() and adds only the agent-mode 202 check.

Safety

  • Comment edits (all files except the two below): no code tokens changed; every deletion was a citation, date, provenance tag, or narration of a past state; every shrink keeps the "must/must not" sentence. git diff shows only // and /** */ lines outside src/api.ts, src/flyctl-build.ts:7, and src/commands/setup.ts:25.
  • Dead code, src/flyctl-build.ts unused import: grep -n '\binfo\b' src/flyctl-build.ts returns only the import line; tsc --noUnusedLocals reported it as TS6133 and reports nothing else in src/.
  • Dead code, whichOnPath re-export in src/commands/setup.ts: grep -rn whichOnPath src test scripts *.md finds the definition in spawn.ts:29, its one internal call at spawn.ts:73, and the re-export; no importer. The package is a CLI (files: dist/**/*.js, bin: insta), so no external consumer. resolveSpawnable on the same line is live (ensure-skills.ts, test/self-install.test.ts) and is kept.
  • Redundant work, src/api.ts single JSON.stringify: same object, no awaits between the two original calls that could mutate it, so the two strings were identical by construction; agentHeaders still receives '' when body is undefined. Exercised by test/agent.test.ts (signed POST/PUT bodies, evidence:false) and test/api-signal.test.ts.
  • Pure refactor inside one module, request() via rawRequest(): the only reordering is the 202 check now running after the >= 400 throw; a status cannot be both 202 and ≥400, so every input takes the same path. Exported names and signatures unchanged. Exercised by test/agent.test.ts (202 → AgentApprovalRequired, 2xx bodies), test/api-signal.test.ts (401 → ApiError with body, abort propagation), test/user-agent.test.ts.
  • Documentation drift (feedback.ts, db.ts): prose corrected to match FEEDBACK_TIMEOUT_MS at feedback.ts:48 and the volumeGib/volumeSize reads at db.ts:187; no new claims.

Verification

Baseline on the untouched tree:

npx tsc --noEmit                 # exit 0
npx vitest run                   # 77 files passed, 1467 passed | 120 skipped, exit 0

Final tree:

npx tsc --noEmit                 # exit 0
npx vitest run                   # 77 files passed, 1467 passed | 120 skipped, exit 0

Not run here: the 103 test/ssh-orchestration.test.ts cases and the 16 ssh -G cases in test/ssh-config.test.ts skip because this machine has no ssh-keygen/ssh binary (same 120 skips as baseline; the rest are win32-only). The test-windows CI job was not reproducible here. No test flaked between runs. There is no lint script in package.json or CI.

Deferred findings

Defects (reported, not changed):

  • src/api.ts:111 refresh() reads res.body.accessToken after a < 400 check; an empty 2xx body parses to null (line 103), the TypeError is caught at 115 and turned into return false, so the user sees the original 401 instead of the fault.
  • src/agent.ts:59-66 loadAgentSession converts every failure (EACCES, malformed JSON) into the "run insta setup agent" guidance, which cannot fix a permissions problem.
  • src/commands/project.ts:25-33 tryInstallObserve swallows real write failures under a comment that names only "assets missing".
  • src/index.ts:481 program.parseAsync(computeArgv) is unawaited; a commander-level rejection (e.g. the preAction hook) becomes an unhandled rejection rather than fail().
  • src/commands/domain.ts:24,53 non-null assertions on optional priceCents/checkoutUrl print $NaN / undefined; recordLines at :142 yields -Infinity widths on an empty array (exported, untested).
  • src/commands/build.ts:93 the WALK_CAP check returns from the current directory only; the walk continues into siblings, so the cap does not bound work (only the truncated flag is tested).
  • src/commands/metrics.ts:49-61 humanBytes labels base-1024 memory/storage values KB/MB/GB; it is a third bytes formatter despite storage.ts:10's "don't grow a third copy" note, and it is untested directly.
  • cliVersion exists three times: src/version.ts honours the baked INSTA_CLI_VERSION; src/commands/setup.ts:127 and src/observe/install.ts:18 do not, so in the compiled binary (no package.json) setup agent installs insta@latest and the observe VERSION file says 0.0.0.
  • src/commands/storage.ts:23 parseObjectLimit uses bare Number() (accepts 0x10, 1e2) while services.ts:38-55 anchors on /^\s*(\d+)\s*$/ for exactly that reason; test/storage.test.ts never covers those inputs.
  • src/commands/govern.ts:9 interpolates limit without encodeURIComponent while branch beside it is encoded.
  • src/commands/compute.ts:741 the limits write path prints res.body.service?.name ?? id, dropping the serviceName ?? rung every sibling path has; :361 lifecycle passes id as fallbackName.
  • src/deploy-archive.ts:180 await wait(POLL_MS) runs even when remaining < POLL_MS, sleeping past the deadline before overdue() fires.
  • src/commands/metrics.ts:105 usage() calls requireProject() even when --proj <id> is explicit, triggering auto-link in an unlinked directory.
  • src/config.ts:327-330 persistAutoLink walks ancestors via isHomeLinkTarget, then writeProject walks again; resolveProjectLink re-reads the project.json that findProjectRoot just read.
  • src/commands/deploy.ts:23,256 re-derive the port with bare Number() bypassing the validation at :167-172.

Refactors not done (coverage or boundary):

  • qs() is byte-identical in src/commands/metrics.ts:4 and src/commands/storage.ts:14; collapsing means one module importing from another (or a new home in util.ts), left for the owners.
  • The 409 → repoConnectedHint re-wrap at src/commands/deploy.ts:115 and :194 is identical, but no test exercises the .catch path, only the pure hint function.
  • fmtMib/fmtMb (db.ts:47, compute.ts:622), mib/mb (pack.ts:206, build.ts:114), two table renderers (template.ts:36, db-query.ts:42), and targetService/storageTarget (github.ts:198, storage.ts:47, inlined six more times in compute.ts/db.ts) are near-duplicates with drifted labels or types.
  • Cyclic imports: commands/setup.ts ↔ commands/mcp.ts; setup.ts → project.ts → ensure-skills.ts → setup.ts.
  • src/commands/upgrade.ts:203-212 autoEnabled() reads ~/.insta/config.json directly instead of through config.ts, ignoring its overrides.
  • src/commands/compute.ts (2026 lines) holds domains, lifecycle, exec, limits/volumes and the whole SSH store, with a second import block mid-file at ~:750.

Comments left alone on purpose: the "Railway-style" comparisons in setup.ts and run.ts read as design intent; the isSafeCAHostPattern docblock at ssh-config.ts:579-596 sits 40 lines from its function and overlaps the allowlist doc, but relocating it was not verified line by line tonight; setup.ts:340-346 has an orphaned paragraph between shouldOfferLogin and makePromptSource.

Test environment: with ssh-keygen absent, 103 of the 116 ssh-orchestration cases and 16 ssh-config cases are silently green; nothing in the run output distinguishes "verified" from "not run".

🤖 Generated with Claude Code

Security findings

Opened by Yun Tianming: nightly sweep 2026-09-17-1000, model claude-fable-5-1, 106 turns, 27 files, 258 changed lines.
Verification: npx tsc --noEmit (exit 0) and npx vitest run (77 files, 1467 passed, 120 skipped) both passed on the final tree, identical to baseline; the 119 ssh-dependent skips could not run here (no ssh-keygen/ssh binary) and the Windows CI job was not reproducible.

🤖 Generated with Claude Code

Summary by cubic

Cleans up comments across 25 src files and removes redundant work in the API client, with no behavior changes. Comments now state current constraints without review citations, dates, provenance tags, or change narration; where a story carried a constraint, the constraint survives in one line.

Refactors

  • request() now delegates to rawRequest() and adds only the agent-mode 202 check.
  • fetch() serializes the body once, reusing the string for both the agent-signature payload and the fetch body.
  • Removed the unused info import in flyctl-build.ts and the unused whichOnPath re-export in setup.ts.
  • Fixed doc drift in feedback.ts (outdated 10s timeout) and db.ts (misplaced dbVolumeLines comment).