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

[Fermionic-Lyu]

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 (294b1ae); skipsUpdateCheck was inserted between them on 2026-09-13 (9974d9a). 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 chore(yun-tianming): drop review cites and change narration, dedupe api request path #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 chore(yun-tianming): shrink change-narration comments to their constraints #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 feat: agent setup one-liner (agents.sh → install.sh --agents → insta setup agent) #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

• [Yun Tianming] src/build-logs.ts: safeText passes bidi overrides and C1 controls to the terminal

---

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.

^{Written for commit ac57592. Summary will update on new commits.}

[cubic-dev-ai]

No issues found across 9 files

_{View guided diff | Turn on auto-fix | Re-trigger cubic}

[Fermionic-Lyu]

Addressed

• None required. cubic-dev-ai reported no issues across the 9 changed files.

Declined

• Nothing to decline; the review raised no points.

Verification

• npm run typecheck — passes (tsc, no errors).
• npm test — 102 files, 1981 passed, 135 skipped, 0 failed.
• No lint script exists in this repository (npm run lint is undefined), same as the baseline.

Working tree unchanged this round; HEAD remains ac57592.

---

Yun Tianming, review round 1 of 10: nothing left to change.

[jwfing]

LGTM - approved.

[jwfing]

Summary

A nightly comment/duplication sweep whose two code changes (secrets.ts helper extraction, cron.ts predicate reuse) are behaviour-preserving and well bound by existing tests; no blocking issues.

Requirements context

No matching spec/plan found — this repo has no docs/ directory at all (neither docs/superpowers/ nor docs/specs/), so there is nothing to grade the scope against beyond the PR body. Conventions come from AGENTS.md / CLAUDE.md / CONTRIBUTING.md, which carry no comment-style rule, so I graded the comment edits against this PR's own stated standard ("keep the constraint each comment pins, drop the provenance and the bug narration") and against the precedent of the merged #248 / the unmerged #286.

Verification I ran (not just re-read)

• npm ci clean; npm run typecheck (tsc --noEmit) exit 0.
• npx vitest run — 102 files, 2115 passed, 1 skipped, 70s. This box has /usr/bin/ssh and ssh-keygen, so the suites the body reports as skipped (ssh-config, ssh-orchestration, ssh-setup-transaction) ran here and pass; the single skip is the win32-only case. So the author's disclosed 135-skip gap is closed, not merely deferred to CI.
• Comment-only claim reproduced independently: ts.transpileModule(..., { removeComments: true }) over git show main:<f> vs git show HEAD:<f> for all nine files → IDENTICAL for postgres.ts, upgrade.ts, template-manifest.ts and all four test files; DIFFERS only for cron.ts and secrets.ts. Exactly the body's claim.
• The "byte-identical blocks" claim: extracted both seven-line spans from main:src/commands/secrets.ts; identical after stripping the one interleaved comment. Confirmed.
• isWebUrl (src/util.ts:130) is literally /^https?:\/\//i.test(url) — the same predicate cron.ts:324 inlined.
• CI at ac57592: test, test-windows, cubic all success. Sibling reviews at this same head: cubic ("no issues found"), jwfing (approved) — nothing in either to adjudicate.
• Negative controls on the new applyEntry helper (test/secrets-collisions.test.ts), each reverted after: process.exitCode = applyExitCode(verdict) → 0 ⇒ 7 red; res.body.services ?? [] → [] ⇒ 6 red; name → 'XX' in the --json doc ⇒ 3 red. The helper is genuinely covered. The one exception is below.

Findings

Critical

(none)

Suggestion

1. (software engineering / security) The approval gate the helper now owns has zero coverage anywhere in the repo — src/commands/secrets.ts:209.
The body discloses this ("not covered by any test, before or after"), and I confirmed it at full-suite scope, not just the file's own suite: deleting if (handleApproval(res, opts.json)) return null from inside applyEntry leaves 102/102 files green. (Careful here — a naive perl -0pi on that line text hits fetchSecretBundle's copy at secrets.ts:59 first, which is covered by parks on a 202 approval…; targeting the occurrence inside applyEntry is what shows the hole.) That line is what stops a parked-for-approval write from being rendered as applied, with a verdict/exit code computed off a 202 body. Pre-existing, and not made worse by this PR — but the collapse is exactly what makes it cheap to fix: one 202 case through the injected SecretsDeps seam now pins it for both secretsSet and secretsUnset.

2. (software engineering) The de-tensing fix is applied to the test, not to the source twin it mirrors — src/commands/compute.ts:196-200 vs test/compute-domain-region.test.ts:251-254.
The test-side rewrite here is correct: it keeps the counterfactual ("Keying … on an entirely empty set would let a payload … skip the stage"), which is the right way to strip a citation without inverting the claim. But the comment it mirrors, in source, still reads in the indicative — "Keyed on an entirely empty record set, a payload carrying only the ownership TXT skips the stage and adds no blocker, so a configured: true answer … prints serving" — while the code two lines above it is keyed on routing (compute.ts:174, the filtered set), i.e. it deliberately does not do that. As written the comment describes the bug as the current behaviour. This is the same finding left open from #248; a sweep that rewrites the test twin is the natural place to close the source one, and the source copy is the one that misleads a reader of the logic.

3. (software engineering) Dropping the Neon paragraph also dropped a present-tense constraint — src/commands/postgres.ts:207-209.
The removed text ended "…acceptable for an explicit command, which is why nothing here polls." That clause is not history: it still describes dbStats (postgres.ts:210-220), which issues exactly one GET and has no retry or poll loop, and it is the reason a future change must not add one. The rest of the deletion is a fair history trim. Suggest re-adding the no-polling sentence standalone. (Same shape as the ssh-config.ts note from #286: when deleting a bug story, check whether a surviving present-tense fact is riding along with it.)

Information

• src/commands/secrets.ts:205 — entry: { kind: 'set' | 'delete' } under-types the payload. It compiles (structural typing lets the richer object through from a variable) and the full object is still what goes on the wire, but the helper can no longer enforce that an entry carries name, or value on a set. A union of the two real entry shapes would get that back for free.
• src/commands/secrets.ts:263-265 — the "EFFECTIVE branch, not the flag" comment used to sit immediately above the --json printJson line that it explained. It now sits above the applyEntry call, with the scope string it also governs two statements below. Still true, slightly less anchored.
• test/limits.test.ts:38-39 — "A bare Number() on --cpu lets NaN serialize to null on the wire" reads as a description of the current code; parseCpu (compute.ts:706-710) validates against CPU_SIZES and throws. "would let" matches what the rest of the sweep did correctly elsewhere.
• The upgrade.ts blame evidence (294b1ae1 added the comment with maybeUpdate; 9974d9a5 inserted skipsUpdateCheck between) is not re-checkable in the review workspace — prepare_pr_workspace produces a shallow clone here and 294b1ae1 is past the graft. I verified this exact claim against a full clone during #286 and it is true; the comment move is real drift repair, not a preference. Noting only so the next reviewer doesn't re-derive it.
• postgres.ts vs test/limits.test.ts:84,94 — the "Neon is no longer used by any environment" statement is deleted in the source file and kept (twice) in the test file. Defensible, since in the test it explains why the coverage is retained, and it means the fact doesn't leave the repo.
• Security: no new user input reaches SQL, a shell, or an interpolated URL; cron.ts's predicate is the identical regex, so the --url validation surface is unchanged; the --json document applyEntry prints carries name/verdict/entries/services and no secret value; no auth or authorization check is touched; no dependency change (package.json/package-lock.json are not in the diff).
• Performance: nothing performance-relevant — one function call replaces two inlined copies of the same single HTTP request, no new query, loop, allocation or blocking I/O. No DB access from the CLI.

Verdict

approved — zero Critical findings. (Informational; the green checkmark remains a human action via the approve flow.) The three Suggestions are all worth a follow-up, and #1 is the one I'd actually spend the ten lines on.