chore(yun-tianming): collapse secrets apply block, fix misplaced comment, drop review cites

Merged
FE
5d ago
+78-73

Summary

  • src/commands/secrets.ts: secretsSet and secretsUnset carried the same seven-line block (POST /apply, approval gate, verdict, exit code, --json document). Collapsed into one module-private applyEntry helper; the two commands keep their own entry shape, wording and stderr notes.
  • src/commands/cron.ts: assertTargetFlags re-inlined /^https?:\/\//i for --url; it now calls the already-exported, unit-tested isWebUrl from util.ts, which is that exact regex.
  • src/commands/upgrade.ts: the "Called once at CLI start-up. Never blocks…" comment described maybeUpdate but sat above skipsUpdateCheck. git blame: the comment landed with maybeUpdate on 2026-07-04 (294b1ae1); skipsUpdateCheck was inserted between them on 2026-09-13 (9974d9a5). Moved it back onto maybeUpdate; every claim in it re-checked against the body (sync readCache, one console.error line, respawnDetached).
  • src/commands/postgres.ts: the dbStats comment spent four lines on a self-described "historical" Neon contrast ("Neon is no longer used anywhere … retained, not live"). Kept the present-tense facts (read-only snapshot; a suspended instance answers from the provider's control plane, never dialed), dropped the history.
  • src/template-manifest.ts: deleted // Validate runtime fields: command and mountPath, a restatement of the two if statements beneath it.
  • Test comments (test/limits.test.ts, test/compute-domain-region.test.ts, test/compute-domain-flow.test.ts, test/observe-install.test.ts): removed review provenance (cubic P1/P2/P3, r2d2 round N Critical, Review round 2, review of #178, the review example) and "used to"/"no longer" bug narration, keeping the constraint each test pins. Every surviving claim was checked against the code it names (observe/install.ts Codex node -e command and the commandWindows note, observe/hook.ts projectRootFor, commands/observe.ts installRoot, api.ts rawRequest throwing on ≥400). Same precedent as the merged #248 ("drop review cites").

Safety

  • secrets.ts — duplicate collapse, inside one module. The two blocks were byte-identical (confirmed with diff on the spans); the helper is the moved text with return → return null. Exported symbols unchanged. Survivor exercised before and after by test/secrets-collisions.test.ts, which drives secretsSet and secretsUnset through the injected API seam on the plain path, the --json path, and the degraded/not-written verdicts. The 202 approval branch inside the helper is not covered by any test, before or after; it is the same handleApproval line both copies had.
  • cron.ts — duplicate collapse. isWebUrl is /^https?:\/\//i.test(url) (util.ts:130), the identical predicate; cron.ts already imported from util.js. Covered by test/cron.test.ts (assertTargetFlags rejects example.test/hook, accepts https://x.test/h) and test/util.test.ts (isWebUrl, including upper-case scheme).
  • upgrade.ts — documentation drift (comment move; evidence in Summary).
  • postgres.ts, template-manifest.ts, four test files — comment edits; no constraint removed.
  • Mechanical check, as this repo's reviewer does it: tsc transpileModule with removeComments on each touched file at HEAD vs the working tree is byte-identical for postgres.ts, upgrade.ts, template-manifest.ts and all four test files; only cron.ts and secrets.ts differ, and those are the two code changes above.

Verification

npm run typecheck      # tsc -p tsconfig.json --noEmit — clean, ~5 s
npx vitest run         # baseline: 102 files, 1981 passed, 135 skipped, 31 s
                       # final:    102 files, 1981 passed, 135 skipped, 40 s

The 135 skips are the ssh/ssh-keygen-gated suites (ssh-config, ssh-orchestration, ssh-setup-transaction) and a few symlink/uid-gated cases; this machine has no ssh binary, so they skip here and run in CI and on the reviewer's machine. No lint script exists in package.json. Nothing in this change touches ssh code.

Deferred findings

  • src/commands/secrets.ts readStdin (line ~156): concatenates chunks as strings (data += chunk), so a multi-byte UTF-8 character straddling two stdin chunks is mangled — the exact bug template-author.ts readAllStdin documents and test/template-author.test.ts regression-tests — and it .trim()s, altering a secret value with intentional leading/trailing whitespace. Behaviour change; owners' call.
  • cliVersion has three divergent copies: src/version.ts (honours INSTA_CLI_VERSION, which the bun-built binary relies on), src/observe/install.ts:18 (ignores it; falls back to 0.0.0) and src/commands/setup.ts:128 (ignores it; falls back to the string latest). In a compiled binary the latter two cannot find package.json and report the wrong version. Product decision on which fallback each wants.
  • src/resolve-service.ts:47 strips the scheme with /^https?:\/\// without the i flag; github.ts:9 and util.ts:130 are case-insensitive. An upper-case HTTPS:// image ref is not stripped there.
  • src/commands/auth.ts loginOauth/loginDevice: identical five-line login tail (set session → /me → set session with user → persist → info). Not collapsed: both bodies call ApiClient.load() directly and no test exercises them (test/login-dispatch.test.ts injects fakes for the whole flow).
  • src/commands/postgres.ts: the branch/group query-suffix is built inline five times (dbAlwaysOn, dbLimits, dbStats, dbVolume, dbRestart), identical to each other but different from the existing settingsSuffix helper (unconditional group, ? on empty). The command functions have no test coverage, so not refactored.
  • Cross-module duplicates, left as-is because collapsing them moves code across module boundaries: qs() byte-identical in metrics.ts and storage.ts; sleepSeconds ×3 (auth.ts, template.ts, github.ts) and sleep ×2 (deploy-archive.ts, build-logs.ts); readStdin in observe/hook.ts equivalent to readAllStdin.
  • Same-module near-duplicates needing a design choice: compute.ts setDomain/checkDomain share prologue and 409 wrapping around one differing request line; resolve-service.ts promptServiceName/promptPort share an 11-line clack validate wrapper differing in one call; template.ts/template-author.ts field() renderers differ only in pad width (11 vs 9), each pinned by its own tests.
  • Narration comments remain in compute.ts (computeSSH, readAliasStore, the known_hosts rollback), ssh-config.ts (the %C ControlPath block), and util.ts resolveThroughSymlink. PR #286 attempted that sweep and did not reach approval across three rounds; not retried tonight.
  • test/volume.test.ts:141 carries (r2d2 round 2) in the test title, not a comment; renaming a test is not a comment edit, so left.
  • postcss.config.mjs at the repo root (export default { plugins: {} }, added in #16) is referenced by nothing in the repo. Vite (under vitest) auto-loads postcss config and searches parent directories, so this is plausibly a guard against the insta-cloud superproject's config; removing it is a configuration decision.
  • .claude/skills/developing-insta-cli/SKILL.md commands/ table omits github.ts, ssh-config.ts, build-logs.ts, template-author.ts; these are helper/leaf modules rather than command groups, so the sentence "one file per command group" is not contradicted. Noted only.

Security findings

Opened by Yun Tianming: nightly sweep 2026-10-09-1000, model claude-fable-5-1, 84 turns, 9 files, 151 changed lines.
Verification: typecheck (tsc --noEmit) clean; vitest full suite 102 files, 1981 passed, 135 skipped (ssh/ssh-keygen-gated, no ssh on this machine) — identical to baseline; comment-stripped transpile byte-identical for all comment-only files

🤖 Generated with Claude Code

Summary by cubic

Collapses a duplicated seven-line /apply block in secrets.ts into a shared helper, replaces an inline URL regex in cron.ts with the existing isWebUrl util, and cleans up comments that described history or review provenance instead of the code itself. No behavior changes; the full test suite passes with identical results.

  • secrets set and secrets unset now share one module-private applyEntry helper; both commands keep their own output wording and stderr notes.
  • upgrade.ts moves the "Called once at CLI start-up…" comment from skipsUpdateCheck back onto maybeUpdate, which it describes.
  • Removed "used to"/"no longer" bug narration and review round references (cubic P1, r2d2 round N, review of #178) from comments in four test files, keeping the constraint each test pins.
  • Deleted a comment in template-manifest.ts that restated the two if statements beneath it, and trimmed a historical Neon contrast from postgres.ts.
  • Comment-only files produce byte-identical output after comment stripping; secrets.ts and cron.ts are the only files with code changes, both covered by existing tests.