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

src/commands/cron.ts

@@ -16,7 +16,7 @@
 //      forward because it can (see buildRequest).
 import { randomUUID } from 'node:crypto'
 import { ApiClient, ApiError, requireProject } from '../api.js'
-import { die, handleApproval, info, printJson, refuse } from '../util.js'
+import { die, handleApproval, info, isWebUrl, printJson, refuse } from '../util.js'
 import { q, resolveSoleService } from './services.js'
 
 // ---- wire shapes (the fields this module reads; the platform sends more) ----
@@ -321,7 +321,7 @@ export function assertTargetFlags(o: TargetOpts, partial = false): void {
   if (o.url && o.service) throw new Error('--url and --service name two different targets — pass one')
   if (o.path !== undefined && !o.service) throw new Error('--path applies to --service (an external target carries its path in the --url)')
   if (!partial && !o.url && !o.service) throw new Error('name a target: --url <https://…> or --service <name> [--path /api/cron]')
-  if (o.url !== undefined && !/^https?:\/\//i.test(o.url)) throw new Error(`--url must be an absolute http(s) URL, got: ${o.url}`)
+  if (o.url !== undefined && !isWebUrl(o.url)) throw new Error(`--url must be an absolute http(s) URL, got: ${o.url}`)
   if (o.path !== undefined && !o.path.startsWith('/')) throw new Error(`--path must start with /, got: ${o.path}`)
 }
 

src/commands/postgres.ts

@@ -205,12 +205,8 @@ export function dbStatsLines(group: string, body: any): string[] {
 }
 
 // Point-in-time stats snapshot for a postgres service: connections vs the server's ceiling, cache
-// hit rate, database size. Read-only. insta-db-backed: a suspended instance answers from the
-// provider's control plane (shown as "(suspended)" with structural zeros), never dialed. That is
-// every environment today — the Neon-backed contrast below is historical: Neon is no longer used
-// anywhere, and the code that handled it is retained, not live. Neon-backed: the platform read
-// over a direct SQL connection, so a one-shot call could wake a suspended endpoint — acceptable
-// for an explicit command, which is why nothing here polls.
+// hit rate, database size. Read-only. A suspended instance answers from the provider's control
+// plane (shown as "(suspended)" with structural zeros), never dialed.
 export async function dbStats(service: string | undefined, opts: Opts): Promise<void> {
   const api = await ApiClient.load()
   const p = await requireProject()

src/commands/secrets.ts

@@ -195,6 +195,29 @@ export function applyServiceLines(services: ApplyServiceOutcome[]): string[] {
   })
 }
 
+// POST one entry to /apply and settle what `secrets set` and `secrets unset` share: the approval
+// gate, the exit code, and the --json document. Returns null when the caller has nothing left to
+// print — the platform gated the write, or --json already carried the outcome.
+async function applyEntry(
+  d: SecretsDeps,
+  name: string,
+  branch: string | undefined,
+  entry: { kind: 'set' | 'delete' },
+  opts: { service?: string; json?: boolean },
+): Promise<{ verdict: ApplyVerdict; services: ApplyServiceOutcome[] } | null> {
+  const res = await d.api.rawRequest('POST', `/projects/${d.projectId}/apply`, { branch: branch ?? d.linkedBranch, entries: [entry] })
+  if (handleApproval(res, opts.json)) return null
+  const entries: ApplyEntryOutcome[] = res.body.entries ?? []
+  const services: ApplyServiceOutcome[] = res.body.services ?? []
+  const verdict = applyVerdict(entries, services)
+  process.exitCode = applyExitCode(verdict)
+  if (opts.json) {
+    printJson({ ok: verdict === 'ok', verdict, name, branch: branch ?? null, service: opts.service ?? null, entries, services })
+    return null
+  }
+  return { verdict, services }
+}
+
 // Set a user secret. Project-wide by default; --branch scopes it to one branch. --service binds
 // it to a branch service instead, which implies the current branch (binding requires one). Value
 // comes from the argument, or stdin when omitted (keeps secret values out of shell history).
@@ -214,13 +237,9 @@ export async function secretsSet(
   const branch = opts.service ? (opts.branch ?? d.linkedBranch) : opts.branch
   // The batch always deploys on ONE branch, so the top level falls back to the linked one even when the entry itself is project-wide.
   const entry = { kind: 'set' as const, name, value: v, ...(branch ? { branch } : {}), ...(opts.service ? { service: opts.service } : {}) }
-  const res = await d.api.rawRequest('POST', `/projects/${d.projectId}/apply`, { branch: branch ?? d.linkedBranch, entries: [entry] })
-  if (handleApproval(res, opts.json)) return
-  const entries: ApplyEntryOutcome[] = res.body.entries ?? []
-  const services: ApplyServiceOutcome[] = res.body.services ?? []
-  const verdict = applyVerdict(entries, services)
-  process.exitCode = applyExitCode(verdict)
-  if (opts.json) return printJson({ ok: verdict === 'ok', verdict, name, branch: branch ?? null, service: opts.service ?? null, entries, services })
+  const outcome = await applyEntry(d, name, branch, entry, opts)
+  if (!outcome) return
+  const { verdict, services } = outcome
   info(`set ${name}${opts.service ? ` → ${opts.service}` : ''} (${branch ? `branch ${branch}` : 'project-wide'})`)
   for (const line of applyServiceLines(services)) info(line)
   // Verdict note on stderr, after the per-service lines: not success, and not the same failure either.
@@ -241,15 +260,11 @@ export async function secretsUnset(
   // pair without one) — so --service defaults to the linked branch, exactly as `secrets set` does.
   const branch = opts.service ? (opts.branch ?? d.linkedBranch) : opts.branch
   const entry = { kind: 'delete' as const, name, ...(branch ? { branch } : {}), ...(opts.service ? { service: opts.service } : {}) }
-  const res = await d.api.rawRequest('POST', `/projects/${d.projectId}/apply`, { branch: branch ?? d.linkedBranch, entries: [entry] })
-  if (handleApproval(res, opts.json)) return
-  const entries: ApplyEntryOutcome[] = res.body.entries ?? []
-  const services: ApplyServiceOutcome[] = res.body.services ?? []
-  const verdict = applyVerdict(entries, services)
-  process.exitCode = applyExitCode(verdict)
   // The EFFECTIVE branch, not the flag: with --service and no --branch the scope that was deleted
   // is the linked branch's, and the output has to say which scope it actually touched.
-  if (opts.json) return printJson({ ok: verdict === 'ok', verdict, name, branch: branch ?? null, service: opts.service ?? null, entries, services })
+  const outcome = await applyEntry(d, name, branch, entry, opts)
+  if (!outcome) return
+  const { verdict, services } = outcome
   const scope = opts.service ? `${opts.service}, branch ${branch}` : branch ? `branch ${branch}` : 'project-wide'
   info(`unset ${name} (${scope})`)
   for (const line of applyServiceLines(services)) info(line)

src/commands/upgrade.ts

@@ -384,8 +384,6 @@ export async function autoupdate(mode?: string): Promise<void> {
   info(`autoupdate: ${enabled ? 'on' : 'off'} (default on while the CLI is pre-1.0 — \`insta config autoupdate off\` to disable)`)
 }
 
-// Called once at CLI start-up. Never blocks: reads the cache synchronously, prints at most one
-// stderr line, and (when due) spawns detached children for the registry check / quiet upgrade.
 /** Commands that must never nudge about an update or spawn a background one.
  *
  *  `upgrade`/`autoupdate` are the update machinery itself. Every `__` command
@@ -406,6 +404,8 @@ export function skipsUpdateCheck(cmd: string | undefined, sub?: string | undefin
   return cmd === 'upgrade' || cmd === 'autoupdate' || !!cmd?.startsWith('__')
 }
 
+// Called once at CLI start-up. Never blocks: reads the cache synchronously, prints at most one
+// stderr line, and (when due) spawns detached children for the registry check / quiet upgrade.
 export function maybeUpdate(current: string, argv: string[]): void {
   if (skipsUpdateCheck(argv[2], argv[3])) return
   const channel = detectChannel()

src/template-manifest.ts

@@ -174,7 +174,6 @@ export function validateManifest(m: TemplateManifest): string[] {
     if (authored.spec !== undefined) {
       problems.push(`${where}: compute size is the platform's to choose — remove spec`)
     }
-    // Validate runtime fields: command and mountPath
     if (svc.command !== undefined && (typeof svc.command !== 'string' || !svc.command.trim())) {
       problems.push(`${where}.command must be a non-empty string`)
     }

test/compute-domain-flow.test.ts

@@ -1,6 +1,6 @@
-// The COMMAND-level flow of the three domain verbs, through the injected API seam (r2d2 round 1
-// Suggestion — the pure renderers are covered in compute-domain-region.test.ts, this covers what
-// the commands actually send and print): the preflight service lookup, the explicit `group` on
+// The COMMAND-level flow of the three domain verbs, through the injected API seam (the pure
+// renderers are covered in compute-domain-region.test.ts; this covers what the commands actually
+// send and print): the preflight service lookup, the explicit `group` on
 // every call so the platform's `default` fallback is never relied on, --json passing the platform
 // body through untouched, and 409 → the release-then-rebind message.
 import { describe, it, expect, vi, afterEach, afterAll } from 'vitest'

test/compute-domain-region.test.ts

@@ -85,7 +85,7 @@ describe('domainGuidanceLines (after attach: what to do next, from the platform
 
   // The printed follow-up must reach the same service on the same branch: without --group it dies
   // on the very ambiguity error this feature raises, and without --branch it checks the linked
-  // branch instead (cubic P2).
+  // branch instead.
   it('the follow-up check command carries the resolved group and the invoked branch', () => {
     expect(domainGuidanceLines(bound, { group: 'api' }).at(-1)).toBe('then: insta domain check app.customer.com --group api')
     expect(domainGuidanceLines(bound, { group: 'api', branch: 'preview' }).at(-1))
@@ -179,7 +179,7 @@ describe('domainStatusLines (check: every stage + where it routes)', () => {
 
   // A PLANE answer always carries `ssl`, so a plane answer with no `origin` is a daemon too old to
   // say where the hostname lands: routing is UNCONFIRMED and must not be reported as serving —
-  // "the plane said active" is not evidence that traffic reaches this region (r2d2 round 1 Critical).
+  // "the plane said active" is not evidence that traffic reaches this region.
   it('failure shape 3 — a plane daemon reports no routing target: UNCONFIRMED, never serving', () => {
     const { origin: _o, edgeOrigin: _e, originOk: _k, ...noReport } = active
     const lines = domainStatusLines(noReport as DomainView)
@@ -215,23 +215,23 @@ describe('domainStatusLines (check: every stage + where it routes)', () => {
   })
 
   // An error STATE with no reason attached is still an error state — a row that says `error` has
-  // not been observed serving, and printing a URL beside it is the blackhole lie (cubic P2).
+  // not been observed serving, and printing a URL beside it is the blackhole lie.
   it('error state with NO reason: still blocked, and the missing reason is said out loud', () => {
     const lines = domainStatusLines({ ...active, status: 'error' })
     expect(lines).toContain('  error       error       (the plane reported an error state with no reason)')
     expect(lines.at(-1)).toBe('  serving     not yet     (the plane reports an error state)')
   })
 
   // configured: true alongside an outstanding record blocker means the verdict and the record set
-  // disagree — report the disagreement rather than papering over it with a URL (cubic P1).
+  // disagree — report the disagreement rather than papering over it with a URL.
   it('configured but a record is still missing: the disagreement wins, not the verdict', () => {
     const lines = domainStatusLines({ ...active, dns: active.dns.map((d) => (d.type === 'TXT' ? { ...d, status: 'missing' } : d)) })
     expect(lines.at(-1)).toBe('  serving     not yet     (add the ownership TXT)')
     expect(lines.join('\n')).not.toContain('  serving     https://app.customer.com')
   })
 
   // An unchecked TXT (no per-record status on a not-yet-configured answer) blocks too: "we have not
-  // looked" is not "it is there" (cubic P3 — this branch was previously untested).
+  // looked" is not "it is there".
   it('ownership unchecked: rendered as unchecked AND counted as a blocker', () => {
     const noStatus = { ...bound, dns: bound.dns.map(({ status: _s, ...d }) => d) }
     const lines = domainStatusLines(noStatus)
@@ -240,18 +240,18 @@ describe('domainStatusLines (check: every stage + where it routes)', () => {
   })
 
   // No records at all: the stages are still drawn. An omitted stage reads as "not required", when
-  // the truth is the platform told us nothing to publish (cubic P2).
+  // the truth is the platform told us nothing to publish.
   it('no records from the platform: ownership and cname stages are still drawn, as unknown blockers', () => {
     const lines = domainStatusLines({ ...bound, dns: [] })
     expect(lines[1]).toBe('  ownership   unknown     the platform returned no ownership TXT for this domain — nothing to publish yet; ask an operator')
     expect(lines[2]).toBe('  cname       unknown     the platform returned no routing record for this domain — nothing to publish yet; ask an operator')
     expect(lines.at(-1)).toContain('no ownership TXT from the platform')
   })
 
-  // The PARTIAL case (r2d2 round 2 Critical): records came back, but not the routing one. Keying
-  // the "no routing record" stage on an entirely empty set let a payload carrying only the
-  // ownership TXT skip the stage and add no blocker — so a configured answer with a live cert and
-  // a confirmed origin printed a URL for a hostname with nothing pointing at us.
+  // The PARTIAL case: records came back, but not the routing one. Keying the "no routing record"
+  // stage on an entirely empty set would let a payload carrying only the ownership TXT skip the
+  // stage and add no blocker — a configured answer with a live cert and a confirmed origin would
+  // print a URL for a hostname with nothing pointing at us.
   it('records present but NO routing record: the stage is still drawn, blocks, and never says serving', () => {
     const txtOnly: DomainView = {
       ...bound, configured: true, status: 'active', ssl: 'active',
@@ -279,9 +279,9 @@ describe('domainStatusLines (check: every stage + where it routes)', () => {
     expect(lines.join('\n')).not.toContain('add CNAME customer.com')
   })
 
-  // r2d2 round 3 Critical: the blocker rule reached only the ownership TXT and the FIRST routing
-  // record. Every other record rendered from `configured` alone and blocked nothing, so a payload
-  // whose SECOND apex record was bad still ended in `serving https://…`.
+  // The blocker rule must reach every routing record, not just the ownership TXT and the FIRST
+  // one: a record rendered from `configured` alone blocks nothing, so a payload whose SECOND apex
+  // record is bad would still end in `serving https://…`.
   it('apex A/AAAA pair with the AAAA missing: both routing records are judged, and it does not serve', () => {
     const apex: DomainView = {
       // Fly-shaped (an apex is Fly's A/AAAA path): no ownership TXT, no plane ssl/origin fields.

test/limits.test.ts

@@ -36,8 +36,8 @@ describe('parseMemoryMb', () => {
   })
 })
 
-// Review round 2: both jwfing and cubic independently flagged the bare Number() on --cpu (NaN
-// serializes to null on the wire) and the unvalidated db strings. These pin the new seams.
+// A bare Number() on --cpu lets NaN serialize to null on the wire, and the db strings need the
+// same local validation. These pin the parsing seams.
 import { parseCpu, fmtMb } from '../src/commands/compute.js'
 import { fetchDbInstance } from '../src/commands/postgres.js'
 import { ApiError } from '../src/api.js'
@@ -71,20 +71,18 @@ describe('parseDbCpu / parseDbMemory (provider quantity strings)', () => {
 describe('fmtMib (display must not claim a ceiling the API did not set)', () => {
   it('collapses whole and half GiB', () => {
     expect(fmtMib(2048)).toBe('2 GiB')
-    expect(fmtMib(1536)).toBe('1.5 GiB') // the review example: was shown as "2 GiB"
+    expect(fmtMib(1536)).toBe('1.5 GiB') // never rounded up to "2 GiB"
   })
   it('keeps everything else exact in MiB', () => {
     expect(fmtMib(1300)).toBe('1300 MiB')
     expect(fmtMib(512)).toBe('512 MiB')
   })
 })
 
-// The round-3 Critical: rawRequest THROWS on >=400, so the no-instance soft-path must live in a
-// catch — status-branching on its return value was unreachable dead code and the read crashed with
-// a raw ApiError. These drive the seam with a stub client, which is exactly the test that would
-// have caught it (the 502 branch was never taken by any test). That soft path is the legacy Neon
-// path: Neon is no longer used by any environment, so these stay as retained coverage for code
-// that is not live.
+// rawRequest THROWS on >=400 (api.ts), so the no-instance soft path must live in a catch, not in
+// status branching on its return value. These drive the seam with a stub client so the 502 branch
+// is actually taken. That soft path is the legacy Neon path: Neon is no longer used by any
+// environment, so these stay as retained coverage for code that is not live.
 describe('fetchDbInstance (the read seam)', () => {
   const stub = (fn: () => Promise<any>) => ({ rawRequest: fn }) as any
 
@@ -122,8 +120,7 @@ describe('fmtMb (compute display)', () => {
   })
 })
 
-// Case-exactness (round-3 suggestion): k8s quantities are case-sensitive, so local validation
-// must reject what the server would.
+// k8s quantities are case-sensitive, so local validation must reject what the server would.
 describe('parseDbMemory case-exactness', () => {
   it('rejects lowercase unit variants the backend refuses', () => {
     for (const raw of ['4gi', '4GI', '8m', '2048mi']) {