[Fermionic-Lyu]

Summary

• src/commands/storage.ts and src/commands/metrics.ts each carried a private qs() that was byte-for-byte identical (build a URLSearchParams from a record, skipping undefined and '', and prefix ? when non-empty). The storage copy was added in 8919437 (2026-08) as a second copy of the metrics one from 50fc2e4 (2026-06). The one surviving copy now lives in src/commands/services.ts beside q(), the branch-only form of the same helper that nine command modules already import from there, and both callers import it.

Safety

• Duplicate collapse — the two bodies were identical (confirmed with a 4-line-window scan across src/, which reported this as the only cross-file duplicate). The survivor is exercised by test/storage.test.ts (objectsPath, objectDownloadPath, including the encoding and omitted-param cases) and test/logs-deploy.test.ts (deployEventsPath, including the omitted-param case), all of which ran before and after. No exported symbol of storage.ts or metrics.ts changed name, signature or behaviour; services.ts gains one export (qs) and introduces no import cycle (it imports only ../api.js and ../util.js). No command or flag changed, so skills/insta/cli-reference.md has nothing to mirror.

Verification

npm run typecheck   # tsc -p tsconfig.json --noEmit
npm test            # vitest run

• Baseline (untouched main @ d075ab7): typecheck exit 0; 107 test files passed, 2124 tests passed, 136 skipped, 31.3s.
• Final tree: typecheck exit 0; 107 test files passed, 2124 tests passed, 136 skipped, 33.0s.

There is no lint script in package.json; CI (.github/workflows/ci.yml) runs exactly tsc --noEmit and vitest run on ubuntu and windows, both of which this machine reproduces (ubuntu side). The 136 skipped tests are pre-existing. No suite needs a database, Docker or credentials.

Deferred findings

• src/commands/project.ts:57-58 — resolveProjectName splits cwd and homedir() on / only. On Windows process.cwd() is C:\Users\me\my-app, so a bare insta project create would slugify the whole path to c-users-me-my-app, and the "cwd is the home directory" guard can never match. test/create-name.test.ts only passes POSIX strings, so the Windows CI job does not catch it. A fix (path.basename) changes the project name the command provisions, so it is a product call; not changed.
• src/commands/postgres.ts:26-33, 126-133, 228-233, 258-263, 292-297 — five identical blocks build the ?branch=&group= suffix by hand in dbAlwaysOn, dbLimits, dbStats, dbVolume and dbRestart; the new shared qs({ branch, group: service }) would express each in one line. Only the pure line-builders in that module are tested, not these command bodies, so left as is.
• src/commands/compute.ts (lifecycle, computeStatus, serviceAlwaysOn, computeScale, computeRemoveInstance, serviceVolume, serviceLimits) — the same load-client / require-project / GET services / resolveSoleService prologue is repeated eight times; storage.ts already has a storageTarget helper for exactly this shape. These command bodies have no test coverage, so not refactored.
• src/commands/deploy.ts:140 and :229 — the identical 409 rewrap .catch (repoConnectedHint) appears in prepareSource (tested) and deploy() (not tested). Not extracted because one of the two call sites is uncovered.
• src/commands/auth.ts:71-75 and :87-91 — loginOauth and loginDevice end in the same five lines (set session, GET /me, set session with user, persist, print). Only the dispatch layer is tested (test/login-dispatch.test.ts); left.
• src/commands/mcp.ts:363-371 — installFor parses the existing JSON config once for the "already configured" check and renderJsonConfig parses it again. Harmless; folding them would change the exported renderJsonConfig contract, so left.

---

Opened by Yun Tianming: nightly sweep 2026-10-29-1000, model claude-fable-5-1, 40 turns, 3 files, 24 changed lines.
Verification: npm run typecheck (tsc --noEmit) and npm test (vitest run: 107 files, 2124 passed, 136 skipped) both passed on the final tree, matching baseline; no lint script exists.

🤖 Generated with Claude Code

---

Summary by cubic

Shares the query-string helper between storage.ts and metrics.ts, which each carried an identical private copy, by exporting a single qs from services.ts with no behavior change to any command. Both callers import it alongside the existing q helper.

A comment and new cases in test/services.test.ts directly pin the shared contract: skipping undefined and '' (but keeping '0'), returning '' instead of a bare ?, insertion order, URLSearchParams encoding, and the exact characters where qs diverges from q (space, !, ', (, ), ~).

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

[cubic-dev-ai]

No issues found across 3 files

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

[Fermionic-Lyu]

Addressed

• None required. cubic-dev-ai reported "No issues found" across the 3 files.

Declined

• Nothing to decline. No review points were raised.

Verification

Re-ran the repository gates on the unchanged tree (no lint script exists in package.json; the gates are typecheck and tests):

npm run typecheck   # tsc -p tsconfig.json --noEmit — clean
npm test            # vitest run — 107 files, 2124 passed, 136 skipped

Same result as the baseline reported in the PR description.

---

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

[jwfing]

LGTM - approved.

[jwfing]

Summary

Behaviour-preserving dedup: two byte-identical private qs() copies in metrics.ts and storage.ts collapse into one exported helper in services.ts beside the existing q(); I reproduced typecheck and the full suite green on the head commit and found nothing blocking.

Requirements context

No matching spec/plan found — assessing against the PR description alone. This repo has no /docs/superpowers/ directory; .claude/skills/developing-insta-cli/SKILL.md points at the superproject (insta-cloud) for design notes, and the one it names (docs/superpowers/specs/2026-09-17-cli-command-reorg-design.md) governs the command tree, which this PR does not touch. A pure duplicate-collapse with no surface change has no design note to match, so I assessed against the PR body plus the repo conventions in AGENTS.md and the dev skill.

Convention checks that do apply, and their outcomes:

• AGENTS.md rule 4 (command/flag changes mirrored into skills/insta/cli-reference.md) — N/A, correctly claimed: no command or flag changed.
• Dev-skill rule 6 (a new leaf command owes an MCP tool or a stated reason) — N/A: no new leaf command.
• AGENTS.md rule 3 (pre-commit gate npm run typecheck && npm test) — satisfied, reproduced below.
• Scope discipline — good: 3 files, 24 lines, one concern, and six adjacent cleanups explicitly deferred in the body rather than bundled in. That is the right call on every one of them, particularly project.ts:57-58 (the Windows resolveProjectName bug), which is a product decision and does not belong in a dedup PR.

Findings

Critical

(none)

Suggestion

1. Software engineering — the newly-exported qs has no direct unit test. src/commands/services.ts:13-18

test/services.test.ts is this module's established home for direct tests of its pure exports (assertType, parseCount, parsePort, assertServiceName, parseAccess, resolveServiceId, resolveComputeServiceId, servicesAddRequestBody, parsePgVersion). qs arrives there with only indirect coverage, via objectsPath/objectDownloadPath in test/storage.test.ts:25-52 and deployEventsPath in test/logs-deploy.test.ts.

Promoting a private function to a shared export is the moment to pin its contract, because the two rules that are easy to "clean up" later now have two modules and seven call sites depending on them:

• undefined and '' are both skipped ('' is not the same as "send an empty param");
• an all-empty record yields '', not '?'.

~6 lines in test/services.test.ts. This is a suggestion rather than a blocker because q() has no direct test either, so the PR is consistent with existing precedent rather than regressing from it.

2. Software engineering — util.ts is arguably the more natural home. src/commands/services.ts:13, src/commands/metrics.ts:3

qs is service-agnostic, and metrics.ts — which serves metrics/logs/usage/deploy-events and has nothing to do with the service command group — now imports a command module to obtain a string builder. The dev skill's architecture table assigns "shared helpers" to util.ts.

I'd accept the PR's reasoning (follow q()'s precedent, where eight command modules already import from services.ts; avoid creating a third location). Flagging it only so the long-term target is on record: moving q and qs to util.ts together is the cleaner end state, and is correctly out of scope here.

Worth recording that I checked the cost of the chosen placement and it is zero: services.ts imports only ../api.js and ../util.js — both of which metrics.ts already imported — and it contains no top-level side effects (only const/type/function declarations). So the new edge introduces no cycle and adds no module to the startup graph.

Information

3. q and qs now read as a pair but are not interchangeable. src/commands/services.ts:9-18

They sit adjacent and share a name stem, but encode differently, and storage.ts:9 imports both:

input	q(b)	qs({branch: b})
my branch	?branch=my%20branch	?branch=my+branch
a~b	?branch=a~b	?branch=a%7Eb
o'x	?branch=o'x	?branch=o%27x

Both spellings are spec-valid, every well-behaved server decodes them identically, and no emitted URL changes in this PR — this is pre-existing and purely informational. But a one-line comment on q ("branch-only form; encodes via encodeURIComponent, not URLSearchParams") would stop a future reader from assuming q(b) ≡ qs({ branch: b }) and "simplifying" one into the other.

4. Verification reproduced, with one discrepancy in the reported split.

On head 91b65c5, after npm install --include=dev:

• npm run typecheck → exit 0.
• npm test (vitest run) → 107 files passed, 2258 passed / 2 skipped (2260 total), 37.2s.

The body reports 2124 passed / 136 skipped. Same 2260 total (and the PR touches no test file, so the total cannot move), but the split differs — meaning those ~134 skips are environment-gated on the author's machine, not unconditional pre-existing skips. Nothing here affects the change; noting only that the body's "The 136 skipped tests are pre-existing" does not reproduce, so it is not a reliable baseline marker for the next sweep.

5. The duplicate-collapse claim independently verified. I diffed both base copies out of d075ab7 — src/commands/metrics.ts and src/commands/storage.ts contained byte-identical qs bodies, and the survivor at services.ts:13-18 is byte-identical to both modulo the added export. All seven call sites (storage.ts:26,31; metrics.ts:61,104,113,130,186) are untouched. The "no exported symbol changed name, signature or behaviour" claim holds.

Dimension coverage

• Functionality — behaviour-preserving, verified character-for-character above; no gaps, no API-compatibility or migration concerns, no race conditions (pure function, no shared state).
• Security — no security-relevant changes in this PR. No new user-input path, no new dependency, no auth/authorization touched, nothing newly logged or returned. qs encodes through URLSearchParams, so no injection surface is created or widened; the only delta is one additional export, which is a trivial surface increase within the same package.
• Performance — no impact. Net −1 function definition, no new module in the import graph (see Suggestion 2), no new queries, loops, allocations, or I/O. No DB work, so no index considerations.

Verdict

approved — zero Critical findings. Two Suggestions and three Information notes, none blocking; the author may take or leave all of them. (Informational only — the GitHub approval is a separate human action via the approve flow.)

[jwfing]

Summary

A clean, byte-for-byte-verifiable deduplication: the identical private qs() in metrics.ts and storage.ts is collapsed into one exported helper in services.ts, with no behavioural change to any command.

Requirements context

No matching spec/plan found — assessing against the PR description alone. This repo has no /docs/superpowers/ tree (nor docs/, specs/ or plans/); the only design doc referenced by .claude/skills/developing-insta-cli/SKILL.md lives in the superproject (docs/superpowers/specs/2026-09-17-cli-command-reorg-design.md) and governs the command tree, which this PR does not touch. I reviewed against the PR body, AGENTS.md, and .claude/skills/developing-insta-cli/SKILL.md.

I independently confirmed the central safety claim rather than taking it on trust — extracting the three function bodies from main:src/commands/metrics.ts, main:src/commands/storage.ts and head src/commands/services.ts and diffing them pairwise shows all three identical modulo the export keyword, with no local qs left behind in either caller.

Two further checks the body asserts, both of which hold:

• No import cycle. services.ts imports only ../api.js and ../util.js, and neither imports a command module. metrics.ts is the only new edge.
• No module side effects. services.ts has no top-level executable statements, so metrics.ts importing it pulls in definitions only — no commander registration, no I/O at load.

Also correct that skills/insta/cli-reference.md needs nothing: no command, flag, or exported symbol of storage.ts/metrics.ts changed name, signature or behaviour.

Verification I could not reproduce

npm ci in my sandbox resolved only the cached subset of dependencies (typescript, vitest and tsx never landed), so I could not run npm run typecheck or npm test locally. My assessment rests on static verification plus your reported baseline/final runs and the two CI jobs. Given the change is a 24-line mechanical move with no new logic, I don't consider that a gap worth blocking on — but please confirm both CI jobs (ubuntu + windows) are green before merging, since branch protection won't enforce it for you.

Findings

Critical

(none)

Suggestion

Software engineering — qs is now a shared export with no direct unit test. src/commands/services.ts:13-18, test/services.test.ts:2-5

test/services.test.ts directly unit-tests essentially every other exported pure helper in the module — assertType, assertServiceName, parseCount, parsePort, parseAccess, resolveServiceId, resolveComputeServiceId, servicesAddRequestBody, parsePgVersion, serviceAddedLine, serviceListLine. qs joins that file without one. The coverage you cite is real but indirect: objectsPath/objectDownloadPath (test/storage.test.ts:25-55) and deployEventsPath (test/logs-deploy.test.ts:5-18) exercise it through path builders in other modules, so the helper's own contract — skip undefined, skip '', return '' rather than a bare ? when every param is absent — is pinned only as a side effect of tests that could legitimately be rewritten.

In fairness this is consistency with the file's other exports rather than a regression: the sibling q() has no direct test either. But a single describe('qs') of four assertions is cheap, and it matters more now than it did when the helper was private to one caller — a future edit to the shared qs can silently change the URL two unrelated command groups build.

Project conventions — placement in services.ts vs util.ts. src/commands/services.ts:13-18, src/commands/metrics.ts:3

The architecture table in .claude/skills/developing-insta-cli/SKILL.md assigns commands/ "one file per command group" and util.ts "shared helpers". qs is a fully generic query-string builder, and metrics.ts has no other relationship to the service command group — this import exists purely to reach a utility. util.ts is the documented home for exactly this.

The counter-argument is decent and you make it in the body: q() already lives in services.ts and is imported by eight command modules, and storage.ts already reaches into postgres.ts for fmtBytes. Keeping the two query helpers adjacent has real readability value. I lean toward util.ts because this PR is the moment the helper stops being service-related, but it's a judgement call and squarely the author's — flagging, not blocking.

Information

Functionality — q() and qs() are now adjacent exports that encode differently. src/commands/services.ts:9-18, src/commands/storage.ts:26,31,42

q() uses encodeURIComponent (space → %20); qs() uses URLSearchParams (space → +). storage.ts now calls both for the same branch value — q(branch) on the services route at :42, qs({ branch, … }) on the object routes at :26 and :31. Both forms are legal in a query string and most servers decode them identically, and + itself is safe in both (URLSearchParams emits %2B), so this only diverges for a branch name containing a literal space.

Entirely pre-existing and untouched by this PR — I mention it only because the two helpers now sit on consecutive lines, which invites someone to assume they're interchangeable. If q is ever folded into qs, that's the case to check.

String(v) at src/commands/services.ts:15 is a no-op given the Record<string, string | undefined> parameter type. Preserved verbatim from both originals, which is the right call for a pure move — noting it only in case you'd rather clean it up while the helper is being touched.

The ''-skipping behaviour is now a shared contract. src/commands/services.ts:15 — a caller can never transmit an intentionally-empty parameter. Behaviour is identical to both originals; it's just that the rule is now one module's decision on behalf of two command groups rather than a local convenience.

A heads-up for your own deferred finding on postgres.ts. src/commands/postgres.ts:28,128,230,260,294,481 and src/commands/compute.ts:338 each declare a local const qs = new URLSearchParams(). Adopting the shared qs in either module — as your deferred-findings section proposes for postgres.ts — requires renaming those locals first. It fails loudly at tsc rather than silently, so it's a note for the follow-up PR, not a risk here.

Security

No security-relevant changes in this PR. No new user input path, no change to how values are encoded before they enter a URL (URLSearchParams escaping is preserved exactly), no auth or authorization code touched, nothing newly logged or returned, and no dependency added or changed — package.json is untouched.

Performance

No performance impact. Identical allocation profile per call; the only difference is one fewer function definition parsed per module and one additional resolved import edge at CLI startup, both immeasurable. No DB queries, loops or I/O introduced.

Verdict

approved — zero Critical findings. Both Suggestions (a direct qs test, and util.ts vs services.ts placement) are non-blocking and fine to take, defer, or decline. This is a well-scoped refactor with an unusually honest safety argument, and the deferred-findings section is genuinely useful triage.

(Informational — the GitHub approval is a separate human action via the approve flow.)

[Fermionic-Lyu]

Thanks for the independent diff check and the cycle/side-effect verification.

Addressed

• Direct qs unit test — added describe('qs') in test/services.test.ts with four cases pinning the shared contract: {} and all-absent params return '' (not a bare ?); undefined and '' are skipped while remaining keys keep insertion order; values are encoded the URLSearchParams way ('a b+c&d' → a+b%2Bc%26d); and qs({ branch }) agrees with q(branch) for a branch without spaces, so the two adjacent helpers' only divergence is documented in a test. 4 new tests, all pass.
• q() / qs() encode differently and ''-skipping is now a shared contract — both now stated in a three-line comment above qs in src/commands/services.ts (skips undefined and '', returns '' when nothing is left, space becomes + where q emits %20). Comment only; the function body is still the verbatim original.

Declined

• Move qs to util.ts — kept beside q() in services.ts. The helper is now tested and commented in one place, and moving it alone would split the two query builders across modules while eight importers of q() stay put. If the pair is ever relocated together, it is a one-commit follow-up; not doing it in a PR that merges unread.
• String(v) no-op — left verbatim so the move stays byte-identical to both originals. Trivially droppable in the same follow-up as the placement question.
• postgres.ts / compute.ts local const qs = new URLSearchParams() — noted; the postgres.ts entry in Deferred findings now implies renaming those locals first. tsc would catch a collision, so no risk here.
• CI confirmation — I cannot query GitHub from this environment (gh is not permitted). Both jobs run exactly the two commands reported below, and the ubuntu side is reproduced locally.

Verification

npm run typecheck   # tsc -p tsconfig.json --noEmit → exit 0
npm test            # vitest run → 107 files passed, 2128 passed, 136 skipped, 29.3s

Baseline on main @ d075ab7 was 2124 passed / 136 skipped; the delta is the four new qs cases. No lint script exists in package.json.

---

Yun Tianming, review round 2 of 10, pushed f7f29fb8f2b6.

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[Fermionic-Lyu]

Addressed

• test/services.test.ts:20 — valid. Checked empirically: encodeURIComponent and URLSearchParams diverge on space (%20 vs +) and on !, ', (, ), ~ (left bare vs percent-encoded). Reworded the comment to name that set instead of claiming space is the only difference, and retitled the test to match. The source comment in src/commands/services.ts only gives space as an example, so it was left as is.

Declined

• None.

Verification

• npm run typecheck — clean.
• npm test — 107 files, 2128 passed, 136 skipped (pre-existing skips), 0 failed.
• No lint script exists in package.json, so no lint gate to run.

---

Yun Tianming, review round 3 of 10, pushed fd9f5023c776.

[jwfing]

Summary

A genuine no-op de-duplication: the byte-for-byte identical private qs() in storage.ts and metrics.ts is replaced by a single export in services.ts next to q(), with new direct unit tests pinning its contract — no Critical findings.

Requirements context

No matching spec/plan found in this repo — assessing against the PR description alone. There is no /docs/superpowers/ directory in instacloud-cli; the only spec referenced by the project skill (docs/superpowers/specs/2026-09-17-cli-command-reorg-design.md) lives in the insta-cloud superproject, not here, and it governs the command tree shape — which this PR does not touch. I therefore reviewed against the PR body, AGENTS.md, and .claude/skills/developing-insta-cli/SKILL.md.

Against those conventions the PR is clean:

• Pre-commit gate (npm run typecheck && npm test) — reproduced locally on f7f29fb: tsc --noEmit exit 0; vitest run 107 files / 2262 passed / 2 skipped. (Your run reported 2124 passed / 136 skipped — the delta is environment-driven skips on your machine, not a behavior difference; nothing failed in either run.)
• "Command/flag changes must be mirrored in skills/insta/cli-reference.md" — correctly judged N/A; no command or flag surface changed.
• Rule 3 ("one capability, one path") is the spirit of this change, and it is applied correctly.

Findings

Critical

(none)

Verification performed:

• Confirmed the two deleted bodies were byte-for-byte identical to the survivor (git show d075ab7:src/commands/{storage,metrics}.ts vs services.ts:16-21) — pure move, no semantic drift.
• Confirmed no import cycle: services.ts imports only ../api.js and ../util.js; neither reaches back into metrics.ts or storage.ts (src/api.ts → config/resolve-project/util/version/agent; src/util.ts → node builtins only).
• Confirmed services.ts has no top-level side effects — the new import into metrics.ts cannot change load-time behavior.
• Re-derived every assertion in the new test block in a standalone Node process; all five hold, including qs({ branch: 'feat/x' }) === q('feat/x') (both yield ?branch=feat%2Fx — encodeURIComponent and the form-urlencoded serializer agree on /).
• Confirmed all 7 call sites (metrics.ts:61,104,113,130,186; storage.ts:26,31) are textually unchanged.

Suggestion

Software engineering — the "q and qs only diverge on a space" claim is wrong, and it is the one claim most likely to be acted on.
src/commands/services.ts:13-15 and test/services.test.ts:19-24

Both the doc comment ("a space becomes + where q would emit %20") and the test comment ("the two helpers only diverge on a literal space") understate the difference. encodeURIComponent leaves ~ ' ( ) ! * unescaped; the application/x-www-form-urlencoded serializer behind URLSearchParams preserves only * and escapes the rest:

q ("a~b'c(d)e!f") -> ?branch=a~b'c(d)e!f
qs({branch:"a~b'c(d)e!f"}) -> ?branch=a%7Eb%27c%28d%29e%21f

Nothing in this PR is affected — both helpers keep their existing call sites. But the whole point of putting qs next to q is to invite a future caller to collapse one into the other (your own deferred finding on postgres.ts proposes exactly that), and a branch named release~1 or v1.0(rc) is the case where the swap would silently change the wire format. Suggest widening the comment to "escapes more than encodeURIComponent does — space → +, and ~ ' ( ) ! are percent-encoded" and renaming the test from agrees with q() for a branch name without spaces to something like agrees with q() for an alphanumeric branch path, since feat/x is the only input the current assertion actually covers.

Software engineering — the shared contract silently drops '', and that is now two modules' problem instead of one.
src/commands/services.ts:16-21

if (v !== undefined && v !== '') means no caller can ever send an intentionally-empty parameter (e.g. ?prefix= to mean "root of the bucket" as distinct from "no prefix filter"). This is preserved pre-existing behavior, not a regression, and the comment now documents it — which is an improvement. The suggestion is only that, as a shared export, the next caller who needs ?prefix= will have to either fork the helper or add a flag; worth a line in the comment saying "if you need an explicitly-empty param, don't reach for this."

Information

• Functionality / scope — the de-duplication is partial, which the PR says plainly. src/commands/postgres.ts:26-33, 126-133, 228-233, 258-263, 292-297 still hand-roll ?branch=&group= five times, and postgres.ts:481 / compute.ts:338 / domain.ts:48 / deploy.ts:66 / build-logs.ts:41 each build their own URLSearchParams. So "the one surviving copy" is true for this helper's two call sites, not repo-wide. I'm flagging this only to confirm I agree with the call: the deferred-findings section gives a real reason (those command bodies are untested, so a mechanical swap would be unverifiable), and leaving them is the right scope discipline for a chore PR, not a gap.

• Software engineering — placement is defensible precedent, not ideal layering. qs is a generic URL helper now living in a command module, imported by metrics.ts, which has nothing to do with the service command group. util.ts is the skill's documented home for "shared helpers." That said, q() already lives in services.ts with nine importers, and deploy.ts / manifest.ts / db-query.ts already import pure helpers from there, so putting qs beside q is the lower-surprise choice and keeps the two visually adjacent — which is what makes the divergence comment useful in the first place. No change requested; noting it in case a future PR moves the whole pure-helper cluster out.

• Software engineering — String(v) at src/commands/services.ts:18 is dead given v: string | undefined is already narrowed to string by the guard. Carried over verbatim from both originals, so keeping it is consistent with "pure move."

• Security — no security-relevant changes in this PR. No new user input path (the helper is unchanged and its call sites are identical), no new dependencies, nothing newly logged or returned, no auth/authorization code touched. Worth stating that qs is the parameterization boundary for these URLs and it continues to encode correctly via URLSearchParams; centralizing it marginally reduces the chance of a future hand-rolled, unencoded concatenation.

• Performance — no measurable change. Identical allocations per call (one URLSearchParams, one string). The one new module edge (metrics.ts → services.ts) costs nothing at startup: src/index.ts:20,40 already imports both modules eagerly, so no additional file is loaded on any code path. No DB queries, loops, or I/O involved.

• Test coverage is the best part of this PR. The contract was previously only observable through objectsPath / objectDownloadPath / deployEventsPath; test/services.test.ts:7-25 now pins it directly, including the two behaviors most easily lost in a future rewrite (empty-string return instead of a bare ?, and the skip-'' rule). Matching the existing describe/it style in that file. This is exactly the right amount of test for a shared extraction.

Verdict

approved — zero Critical findings. Two Suggestions (the inaccurate divergence comment is the one worth fixing before merge, since it is the claim a future refactor will rely on) and several Information notes, none blocking. Typecheck and the full suite pass locally on the head commit.

(Informational verdict only — a human still gives the GitHub approval via the approve flow.)

[Fermionic-Lyu]

Addressed

• Divergence comment on qs (Suggestion 1, src/commands/services.ts:13-15): widened the doc comment from "a space becomes +" to the real contract: form-urlencoded escapes more than encodeURIComponent, so space is + not %20, and ~ ' ( ) ! are percent-encoded where q leaves them bare, with a note that swapping one helper for the other changes the wire format. Verified in Node: qs({branch:"a~b'c(d)e!f*g h"}) gives ?branch=a%7Eb%27c%28d%29e%21f*g+h, q gives ?branch=a~b'c(d)e!f*g%20h.
• Test name (Suggestion 1, test/services.test.ts): renamed to agrees with q() for an alphanumeric branch path, since feat/x is the only input it covers. Added one test that pins the divergence above for both helpers, so the comment is now backed by an assertion rather than prose. The test comment itself was already corrected in fd9f502 (cubic thread marked addressed).
• Explicitly-empty params (Suggestion 2): added the line to the qs comment: skipping '' means a caller can never send ?prefix=, and if you need one, don't reach for this helper. Behaviour unchanged.

Declined

• Information notes (partial de-dup in postgres.ts and friends, placement in services.ts vs util.ts, dead String(v)): no change requested by the reviewer and none made. The postgres.ts hand-rolled query strings remain in the PR's Deferred findings; String(v) kept verbatim to keep this a pure move.

Verification

npm run typecheck   -> tsc --noEmit, exit 0
npx vitest run      -> 107 files, 2129 passed | 136 skipped, 0 failed

No lint script exists in this repo.

---

Yun Tianming, review round 4 of 10, pushed 7c886ce1012e.

[jwfing]

Summary

A clean, behaviour-preserving collapse of two byte-identical private qs() copies into one exported helper in src/commands/services.ts, with the shared contract newly pinned by tests — no Critical findings.

Requirements context

No matching spec/plan found — assessing against the PR description alone. This repo has no /docs/superpowers/ tree; AGENTS.md points design docs at the superproject (docs/superpowers/specs/2026-09-17-cli-command-reorg-design.md), which is not part of this checkout. I reviewed against the PR body, AGENTS.md, and .claude/skills/developing-insta-cli/SKILL.md (the repo's own conventions doc).

Conventions checked and satisfied:

• Pre-commit gate (AGENTS.md §3: npm run typecheck && npm test) — the body reports both green against a matching baseline.
• cli-reference mirroring (§4) — correctly identified as not applicable: no command or flag surface changed.
• MCP parity rule (SKILL.md "Command architecture" rule 6) — not applicable: no new leaf command.

A note on my own verification: this sandbox has no npm registry access, so I could not execute vitest locally. Instead I executed the helper's exact body under Node and evaluated all six new assertions in test/services.test.ts directly — all six pass, including the q()/qs() agreement case (feat/x → ?branch=feat%2Fx under both encodeURIComponent and URLSearchParams). CI remains the authority for the full suite.

Findings

Critical

(none)

---

Suggestion

Software engineering — qs is generic infrastructure living in a command module.
src/commands/services.ts:16-21

SKILL.md's architecture table assigns commands/ "one file per command group" and util.ts "shared helpers". A query-string builder is the latter, and src/commands/metrics.ts:3 now imports a command group's module for a reason that has nothing to do with the service command. The PR's rationale — q() already lives there and nine modules import it — is accurate and honestly stated, but it propagates an existing placement wart rather than correcting it, and the body's own deferred list proposes extending the same edge from postgres.ts and compute.ts next.

Counter-argument in the PR's favour, which is why this is non-blocking: the divergence comment at services.ts:13-15 only makes sense sitting beside q, and splitting the pair across files would lose that. Either resolution is defensible; worth a deliberate call rather than inertia. If util.ts is chosen, moving q along with it keeps the pair together.

Software engineering — String(v) is dead, and the duplicated stringification at call sites could go with it.
src/commands/services.ts:18

Under the Record<string, string | undefined> signature with strict: true, v is already narrowed to string by the guard, so String(v) is a no-op. Meanwhile three call sites stringify numbers by hand before handing them over:

• src/commands/storage.ts:26 — limit: limit === undefined ? undefined : String(limit)
• src/commands/metrics.ts:186 — from: from !== undefined ? String(from) : undefined, to: …

Widening the parameter to Record<string, string | number | undefined> would make the internal String(v) load-bearing and let all three call sites drop their ternaries, which is the shape they clearly want. Alternatively just delete the no-op cast. Today's state — cast inside and cast outside — is the one combination that carries the cost of both without the benefit of either.

Software engineering (test coverage) — pin the '0' case, the one value the obvious wrong refactor would break.
test/services.test.ts:6-25

The new tests are a genuine improvement and cover the contract well. One gap: nothing pins that a '0' value survives. The guard is v !== undefined && v !== ''; the tempting simplification to if (v) passes every one of the six new assertions while silently dropping '0'. This is reachable, not hypothetical — resolveLogWindow (src/commands/metrics.ts:158-167) returns from/to as epoch-second numbers, stringified at :186, and from can legitimately be 0 (--from 1970-01-01T00:00:00Z, or --since resolving to now). A one-liner closes it:

expect(qs({ limit: '0' })).toBe('?limit=0')   // falsy but present — not dropped

---

Information

Latent name collision for the deferred follow-ups. src/commands/postgres.ts:28,128,230,260,294,481 and src/commands/compute.ts:338 use qs as a local variable name, and both modules already import from ./services.js (postgres.ts:6, compute.ts:5). The body's deferred item about collapsing the five hand-rolled ?branch=&group= blocks in postgres.ts will therefore need those locals renamed first — flagging so the next sweep doesn't trip on it. src/commands/domain.ts:48 has the same local name but no such import.

null is not handled. qs({ x: null }) would emit x=null rather than skipping. Unreachable from any typed caller under strict: true, so this is a note, not a defect.

---

Dimensions with nothing to report

• Functionality — verified rather than assumed. Both deleted bodies are byte-identical to the survivor; all seven call sites (storage.ts:26,31; metrics.ts:61,104,113,130,186) pass string | undefined only, so the signature binds unchanged; no exported symbol of storage.ts or metrics.ts changed name, signature or behaviour. No import cycle: services.ts:2-3 imports only ../api.js and ../util.js, both of which metrics.ts already imported. services.ts has no top-level side-effecting statements, so the new import cannot reorder anything at load time.
• Security — no security-relevant changes in this PR. No new user input reaches SQL, shell or HTTP by any path that did not already exist; the encoding behaviour for every existing caller is bit-identical, so no injection surface widens or narrows. No new dependencies, no auth or authorization code touched, nothing newly logged or returned.
• Performance — no impact. src/index.ts:20 already imports services.ts statically (that file uses zero dynamic imports), so services.ts was loaded on every CLI invocation before this change; metrics.ts importing it adds no startup cost, no transitive dependency and no new allocation.

Verdict

approved — zero Critical findings. The three Suggestions and two Information notes are all non-blocking; none needs to be resolved before merge.

(Informational only — the GitHub approval is a separate human action via the approve flow.)

[jwfing]

Summary

Pure duplicate-collapse: the byte-identical private qs() in storage.ts and metrics.ts is replaced by one exported qs in services.ts beside q, with new unit tests pinning the contract — I verified the change is behaviour-preserving and found no blocking issues.

Requirements context

No matching spec/plan found — this repo has no /docs/superpowers/ directory (the one referenced by .claude/skills/developing-insta-cli/SKILL.md lives in the superproject, and the design note it cites, 2026-09-17-cli-command-reorg-design.md, covers the command tree, not helper placement). Assessed against the PR description, AGENTS.md, CONTRIBUTING.md, and .claude/skills/developing-insta-cli/SKILL.md. No PostgreSQL schema/query changes, so postgresql-code-review does not apply.

Verified against the stated requirements:

• The two removed bodies are byte-identical to the survivor (git show d075ab7:src/commands/{storage,metrics}.ts, diff of the extracted functions → identical), so "no behaviour change" holds for all seven call sites (storage.ts:26,31; metrics.ts:61,104,113,130,186).
• No import cycle: services.ts:2-3 imports only ../api.js and ../util.js. Both services.ts and metrics.ts are already eagerly imported by src/index.ts:20,40, so the new edge adds no module-load cost.
• No command or flag changed, so the cli-reference.md mirror requirement (AGENTS.md §4) and the MCP-parity rule (SKILL.md "Command architecture" rule 6) are both correctly not triggered.
• I ran the gate on the head commit: npm run typecheck → exit 0; npx vitest run → 107 files, 2263 passed, 2 skipped.

Findings

Critical

(none)

Suggestion

Software engineering — helper placement couples metrics to the service command module (src/commands/services.ts:13-24, src/commands/metrics.ts:3)
AGENTS.md's architecture table names src/commands/ as "one file per command group" and util.ts as "shared helpers". metrics.ts now imports from services.ts solely for a URL helper, with no other relationship to the service group. The existing precedent is squarely on this PR's side — ten modules already import q from services.js — so this is not worth blocking or churning now. But the helper is now cross-cutting by definition ("shared by storage and metrics"), and if a third unrelated group needs it, that is the moment to move q + qs to util.ts (or a small src/commands/query.ts) rather than deepen the dependency on services.ts.

Functionality — the duplicate-collapse goal is only partly realized; the five postgres.ts builders are the likeliest drift site (src/commands/postgres.ts:26-33, 126-133, 228-233, 258-263, 292-297)
The PR's own deferred list is accurate and the reason given (those command bodies are untested, only the pure line-builders are) is a defensible scope call that I agree with for this PR. Worth filing as a tracked follow-up rather than leaving in a PR body: those five hand-rolled ?branch=&group= blocks are now the only remaining copies of a helper whose semantics this PR just documented in detail, and the documented divergence (+ vs %20, ~ ' ( ) !) is exactly the kind of thing that drifts silently when the shared version is edited and the hand-rolled ones are not. Covering those command bodies first, then folding them onto qs, would finish the job.

Information

Software engineering — qs sits above the file's own "pure, unit-tested helpers" banner (src/commands/services.ts:13-26)
As of this PR qs is a pure, unit-tested helper, but it (like q) is declared above the // ---- pure, unit-tested helpers ... ---- divider at line 26. Moving both below it would keep the file's organizing comment honest. Cosmetic; take it or leave it.

Software engineering — name shadowing to watch in the postgres.ts follow-up (src/commands/postgres.ts:28, 128, 230, 260, 294, 481)
Six functions there declare a local const qs = new URLSearchParams(), and postgres.ts already imports from ./services.js. Nothing is broken today — the local declarations simply shadow nothing, since qs is not in that import list — and tsc would reject the collision loudly if someone added it. Just a trap to be aware of when the deferred refactor lands: those locals need renaming or removing in the same edit.

Testing — the new tests are the right ones
test/services.test.ts:6-33 pins exactly the contract that could bite later: '' rather than a bare ?, undefined/'' skipping, insertion order, URLSearchParams encoding, and the precise characters where qs and q disagree. The last case is the valuable one — it makes an accidental q↔qs swap a test failure rather than a wire-format change. The pre-existing path-builder coverage (test/storage.test.ts:25-51, test/logs-deploy.test.ts) still exercises both callers through the shared helper.

Verification — test counts in the PR body differ from a local run, benignly
The body reports 2124 passed / 136 skipped; I measured 2263 passed / 2 skipped on the same head commit. The delta is environment-gated collection, not the change: many suites gate on ssh-keygen availability and symlink permission (test/ssh-orchestration.test.ts:81,1018,1462, test/global-config-mode.test.ts:9, test/ssh-setup-transaction.test.ts:196) or on process.platform. Noting it only so nobody chases the discrepancy later. Both runs are green and match their own baselines.

Convention — branch prefix
CONTRIBUTING.md asks for feat/*, fix/* or docs/*; this is yun-tianming/2026-10-29-1000. Cosmetic given squash-merge, and consistent with the nightly sweep's naming.

Security

No security-relevant changes. No new user input reaches SQL, a shell or an HTTP body; qs is a query-string builder that encodes via URLSearchParams, which is strictly safer than hand concatenation and is unchanged from the code it replaces. No secrets, tokens or PII are newly logged or returned, no auth or authorization path is touched, and no dependency is added (package.json unchanged).

Performance

No impact. Identical function body, same number of calls, same allocations per invocation. The one new import edge (metrics.ts → services.js) costs nothing at startup because src/index.ts:20,40 already loads both modules eagerly to register commands. No DB queries, loops or I/O are involved.

Verdict

approved — zero Critical findings; typecheck and the full vitest suite pass on the head commit, and the change is verifiably behaviour-preserving. The two Suggestions are follow-ups, not merge blockers. (Informational only — the explicit GitHub approval is a separate human action via the approve flow.)

[Fermionic-Lyu]

Thanks for the thorough pass. Two small follow-ups landed this round; the rest is answered below.

Addressed

• Dead String(v) cast (src/commands/services.ts:21): deleted. Under strict: true the v !== undefined && v !== '' guard narrows v to string, so u.set(k, v) typechecks and the emitted string is identical by construction. Took the "just delete the no-op" branch of your suggestion rather than widening the signature (see Declined).
• Pin the '0' case (test/services.test.ts): added keeps a "0" value rather than treating it as absent, asserting qs({ limit: '0' }) → ?limit=0 and qs({ from: '0', to: '0' }) → ?from=0&to=0. One correction to the premise, though: a string '0' is truthy in JS, so an if (v) guard would not drop it under the current string | undefined signature. I checked the if (v) variant directly under Node and it returns ?limit=0. The test is kept as a contract pin (only undefined and '' count as absent) and its comment says plainly that it only becomes a discriminating case once the parameter accepts numbers.

Declined

• Move q/qs to util.ts: agreed it's the better home, but it relocates an export that nine modules import and crosses module boundaries, which is outside what this unread-merge PR should carry. Deferred: move q and qs together with the divergence comment in a dedicated PR.
• Widen to string | number | undefined and drop the call-site ternaries: changes the helper's signature and three call sites in two other modules. Behaviour would be identical, but it's a cross-module API change, so it goes with the same follow-up as the move. The dead cast is gone in the meantime, so the "cast inside and outside" state no longer exists.
• qs local-variable collision in postgres.ts / compute.ts: noted and recorded against the deferred ?branch=&group= collapse; nothing to change here.
• null not handled: agreed it's unreachable under the typed signature; no change.
• cubic inline comment on the test comment: already fixed in fd9f502. The comment now names the actual divergence set (space plus ! ' ( ) ~), which matches the diverges from q() assertion.

Verification

npm run typecheck        # clean
npx vitest run           # 107 files, 2130 passed | 136 skipped (baseline before this round: 2129 passed | 136 skipped)

No lint script exists in package.json.

---

Yun Tianming, review round 5 of 10, pushed 4fbc86458a58.

[jwfing]

Summary

Collapses two byte-identical private qs() helpers (storage.ts, metrics.ts) into one exported qs in services.ts beside the existing q, with new unit tests pinning the shared contract — a clean, behaviour-preserving dedup with no Critical findings.

Requirements context

No matching spec/plan found in this repo — there is no /docs/superpowers/ tree in instacloud-cli (the design docs referenced by .claude/skills/developing-insta-cli/SKILL.md, e.g. docs/superpowers/specs/2026-09-17-cli-command-reorg-design.md, live in the insta-cloud superproject, which is not part of this checkout). Assessed against the PR description, AGENTS.md, and the developing-insta-cli skill (architecture table, pre-commit gate, cli-reference mirroring rule).

Against those: the PR is in scope (3 source files, 24 changed lines, pure refactor), changes no command or flag — so the skills/insta/cli-reference.md mirroring rule in AGENTS.md §4 correctly does not apply — and adds tests rather than only moving code, which matches the repo's "pure, unit-tested helpers" seam convention in services.ts:26.

Findings

Critical

(none)

Functionality — verified, not just read. The refactor's core claim is that the survivor is behaviourally identical to the two deleted copies. I confirmed this directly:

• The two deleted bodies (src/commands/metrics.ts:4-9, src/commands/storage.ts:14-19 at base) and the survivor (src/commands/services.ts:19-24) differ only in u.set(k, String(v)) → u.set(k, v). Given the Record<string, string | undefined> signature and the preceding v !== undefined narrowing, v is already string, so String(v) was a no-op. Zero wire-format change.
• I executed all ten new assertions from test/services.test.ts:7-37 against the shipped implementation under Node 22 — all pass, including the subtle divergence case qs({branch: "a~b'c(d)e!f*g h"}) === '?branch=a%7Eb%27c%28d%29e%21f*g+h' vs q(v) === "?branch=a~b'c(d)e!f*g%20h".
• No import cycle is introduced: services.ts:2-3 imports only ../api.js and ../util.js; the only inbound edge from this family is billing.ts:4 → metrics.ts → services.ts, which stays acyclic.
• All seven qs call sites (storage.ts:26,31; metrics.ts:61,104,113,130,186) pass string | undefined values, so none relied on the removed String() coercion.

Security — no security-relevant changes in this PR. No new user input reaches SQL, shell or HTTP; no auth/authorization path is touched; no dependency added. Worth recording as a positive: qs routes values through URLSearchParams, which escapes &, = and #, so a crafted object key or branch name still cannot inject an extra query parameter — and the storage/metrics call sites keep exactly the encoder they already had.

Performance — no regression. Same allocations per call as before (one URLSearchParams); the only delta is one additional module edge in metrics.ts's graph, already resident via index.ts. No new queries, loops or blocking I/O.

Suggestion

• Software engineering — placement vs. the documented architecture. AGENTS.md's architecture table assigns util.ts the "shared helpers" role, while commands/ is "one file per command group". qs is now a genuinely cross-cutting helper living in a command module (src/commands/services.ts:19-24), which makes metrics.ts:3 a command→command import purely for a utility. Following the q precedent (nine modules already import it from there) is the defensible call and I would not block on it — but if a third non-service consumer appears, moving q + qs together to util.ts is the cleaner endpoint.
• Software engineering — the dedup is only indirectly covered on the metrics side. deployEventsPath (metrics.ts:129-131) is a pure seam and is covered by test/logs-deploy.test.ts:5-17, but the other four metrics call sites (metrics.ts:61, :104, :113, :186) build their paths inline inside async command bodies with no pure seam, so no test observes their query strings. Behaviour is identical by construction here, so this is not a blocker for this PR — but extracting those four the way deployEventsPath already is would make the next change to qs safe to verify.

Information

• src/storage.ts:26 — the String(limit) wart at the call site. objectsPath has to stringify limit (a number in ObjectParams, storage.ts:21) because qs accepts strings only; metrics.ts:186 does the same dance for from/to. Widening the signature to Record<string, string | number | undefined> would delete all three — and the test comment at test/services.test.ts:19-21 already anticipates exactly this ("it pins the contract for whoever widens the signature to numbers later"). Note that widening would make the v !== '' guard asymmetric (0 kept, '' dropped), which is why the current narrow signature is a reasonable place to stop.
• The doc comment at services.ts:13-18 is the most valuable part of this diff. Spelling out that qs and q disagree on space (+ vs %20) and on ~ ' ( ) ! is what stops a future reader from "simplifying" q into qs({branch}) and silently changing the wire format. Good call pinning it with an executable test (test/services.test.ts:33-37) rather than prose alone.
• Deferred findings in the PR body are well-judged. Each one names a concrete reason to defer (product decision for the project.ts Windows path bug, absent test coverage for the postgres.ts / compute.ts / deploy.ts / auth.ts candidates, contract change for mcp.ts). The project.ts:57-58 Windows basename issue in particular looks real and worth its own issue — path.basename would fix it, but it changes a provisioned project name, so keeping it out of a chore PR is correct.
• Verification note on my side: the review sandbox has no npm registry access (typescript/vitest could not be installed), so I could not re-run npm run typecheck && npm test myself. I compensated by executing the helper and every new assertion directly under Node, and by checking the call sites and import graph by hand; the PR's reported baseline-vs-final CI numbers (107 files, 2124 passed, 136 skipped, typecheck exit 0) are consistent with a change of this shape but are taken on trust.

Verdict

approved — zero Critical findings. Scope is tight (4 files, +49/−16), the behaviour-preservation claim holds under direct verification, the new tests genuinely pin the contract rather than restating the implementation, and the two Suggestions are both "next time" items, not merge blockers.

(Informational only — the required GitHub approval is a separate human action via the approve flow.)

[Fermionic-Lyu]

Thanks for the approval. One cosmetic item taken this round; everything else stays as follow-up.

Addressed

• qs above the "pure, unit-tested helpers" banner — moved the divider up so q and qs sit below it (src/commands/services.ts). Pure reorder inside one module: exported names, signatures and bodies are unchanged (git diff --stat: 2 insertions, 2 deletions, one file).
• cubic inline comment on test/services.test.ts:20 — already fixed in fd9f502; the comment now names ! ' ( ) ~ alongside the space, matching the divergence test below it. No further change.

Declined

• Move q + qs to util.ts — agreed it's the right destination once a third unrelated group needs it, and agreed with you that it isn't worth churning now. It moves exports across ten importing modules, which is outside what this unread-merge PR may do. Deferred finding: relocate q/qs together with the divergence comment in a dedicated PR when the next consumer appears.
• Fold the five postgres.ts ?branch=&group= blocks onto qs — those command bodies have no test coverage, so the collapse has no evidence of behaviour preservation. Deferred finding: cover the bodies, rename the six local const qs = new URLSearchParams() declarations (and compute.ts:338), then fold. Noted as a tracked follow-up per your request rather than only in the PR body.
• Branch prefix — the sweep's branch name is set by the wrapper, not by this PR; cosmetic under squash merge.
• Test-count delta — confirmed environment-gated as you described; local run is 2130/136, nothing to chase.

Verification

• npm run typecheck → exit 0
• npx vitest run → 107 files, 2130 passed, 136 skipped (identical before and after the reorder)
• No lint script in package.json.

---

Yun Tianming, review round 6 of 10, pushed 89e072ee6c73.

[Fermionic-Lyu]

Thanks for the direct verification of the ten assertions and the import graph. Nothing changed in the tree this round; every remaining item is either already addressed or a follow-up by your own description.

Addressed

• cubic inline comment on test/services.test.ts:20 (divergence list not exhaustive): already fixed in fd9f502. The comment now names the space case and ! ' ( ) ~, and the executable test on lines 33-37 pins the same list.

Declined

• Move q + qs to src/util.ts: not in this PR, as agreed in rounds 5 and 6. It relocates an export with ten importers, which is beyond a dedup of two identical private copies. Trigger stays the same: a third non-service consumer of qs, then move both helpers together with the divergence comment in a dedicated PR.
• Extract the four inline metrics path builders (metrics.ts:61,104,113,186) into pure seams like deployEventsPath: declined here. Those async command bodies have no test coverage, so the extraction would be a refactor without a before/after check. Noted in Deferred findings as its own PR, tests first.
• Widen the qs signature to accept numbers: declined as a cross-module API change, and you noted the ''/0 asymmetry makes the narrow signature a reasonable stopping point. The '0' test remains as the contract pin for whoever widens it later.

Verification

npm run typecheck   -> tsc --noEmit, exit 0
npm test            -> 107 files, 2130 passed | 136 skipped (2266)
git status          -> clean, tree identical to the previously reviewed commit

Baseline on the PR base is identical (typecheck clean, 2130 / 136). No lint script exists in package.json.

---

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

[jwfing]

LGTM - approved.

[jwfing]

Summary

Pure de-duplication: the byte-identical private qs() in storage.ts and metrics.ts is replaced by a single exported qs in services.ts beside q, with new unit tests pinning the shared contract — no behaviour change at any call site.

Requirements context

No matching spec/plan found — this repo has no /docs/superpowers/ directory (the only reference to one, in .claude/skills/developing-insta-cli/SKILL.md, points at the superproject docs/superpowers/specs/2026-09-17-cli-command-reorg-design.md, which is not vendored here). Assessed against the PR description, AGENTS.md, and the developing-insta-cli skill. The skill's rule 4 ("command/flag changes must be mirrored in skills/insta/cli-reference.md") does not apply — no command or flag surface changed, as the PR body states.

Verification I performed

• Confirmed the two removed bodies were identical to the survivor: git show origin/main:src/commands/{storage,metrics}.ts both contain the same 5-line body, differing from the new one only by a redundant String(v) (see Information below).
• Confirmed no import cycle: services.ts imports only ../api.js and ../util.js (src/commands/services.ts:2-3); nothing in api.ts/util.ts reaches back into a command module.
• Confirmed the new test expectations are correct by evaluating the helper against Node's real URLSearchParams — all six assertions, including the q/qs divergence string "a~b'c(d)e!f*g h" → ?branch=a%7Eb%27c%28d%29e%21f*g+h vs ?branch=a~b'c(d)e!f*g%20h, reproduce exactly.
• Could not re-run npm test / tsc --noEmit in the review sandbox (registry access is restricted; node_modules installs incompletely, no tsc/vitest binary). The PR's reported baseline-vs-final results are taken at face value; CI's test / test-windows jobs are the gate.

Findings

Critical

(none)

Suggestion

Functionality — q and qs encode the same branch value two different ways, and storage.ts now uses both. src/commands/storage.ts:42 builds ?branch=… with q() (encodeURIComponent → space as %20, ~ ' ( ) ! left bare) while src/commands/storage.ts:26 and :31 build the same param with qs() (form-urlencoded → space as +, that punctuation percent-encoded). Service names are constrained by SERVICE_NAME_RE (src/commands/services.ts:7), but branch names are not validated anywhere in this module, so a branch containing any of those characters reaches the platform in two different encodings depending on which storage subcommand ran. This is pre-existing, not introduced here — but the PR is what puts the two helpers side by side at src/commands/services.ts:11-27, and the new doc comment correctly identifies the hazard without resolving it. Unifying (q delegating to qs) is a wire-format change and rightly out of scope for a no-behaviour-change refactor; worth a follow-up issue rather than a change in this PR.

Software engineering — the new describe('qs') block is inserted above describe('assertType') (test/services.test.ts:7-38), ahead of the blocks that mirror the export order of services.ts. Moving it down next to the other pure-helper suites would keep the file's test order tracking the module's. Trivial.

Information

Software engineering — dropping String(v) is runtime-identical, as intended. src/commands/services.ts:23 now passes v straight to u.set(k, v). The parameter type is string | undefined, and URLSearchParams.prototype.set stringifies its argument regardless, so even an untyped JS caller gets the old result. No concern, just recording that the one textual difference from the deleted copies was checked.

Software engineering — module placement. qs is a generic URL helper living in a command module; eleven modules now import URL helpers from src/commands/services.ts. Following q's existing location is the right call for this PR (the alternative creates churn the PR didn't sign up for), but src/util.ts or a small src/url.ts is arguably where both belong long-term.

Security — no security-relevant changes in this PR. No new user input path, no new dependency, no auth/authorization code touched, nothing newly logged or returned. The encoding behaviour is unchanged, and URLSearchParams still escapes & and =, so a user-supplied --prefix/key cannot inject extra query parameters — the new expect(qs({ prefix: 'a b+c&d' })).toBe('?prefix=a+b%2Bc%26d') case (test/services.test.ts:25) now pins that property, which is a small net improvement.

Performance — no impact. Same allocations, same work per call; one additional module edge in a graph src/index.ts already loads eagerly.

Test coverage is adequate for the change. The two callers' path builders (objectsPath, objectDownloadPath in test/storage.test.ts; deployEventsPath in test/logs-deploy.test.ts) already exercised the old private copies including the omitted-param cases, and the six new cases pin the contract directly — including the '0' case, whose comment is honest that it doesn't currently distinguish v !== '' from if (v).

The "Deferred findings" section of the PR body is a useful record and I agree with leaving each one out of this change set; the postgres.ts one (five hand-rolled ?branch=&group= blocks) is the natural next sweep now that qs is shared.

Verdict

approved — no Critical findings. Clean, well-scoped refactor with a correct doc comment and tests that pin the shared contract rather than leaving it implied. (Informational: the human GitHub approval is still a separate step via the approve flow.)

[Fermionic-Lyu]

Yun Tianming stopped here and is leaving this pull request for a human: a reviewer withdrew approval while the checks ran

The Safety section above states the evidence for each change; nothing in it alters behaviour.