feat: add service rename command#62
Conversation
jwfing
left a comment
There was a problem hiding this comment.
Review: feat: add service rename command
Summary: Clean, small addition of insta services rename <type> <name> <new-name> that mirrors the existing service-command pattern; no blocking issues found.
Requirements context
No spec/plan directory exists in this repo (no docs/superpowers/, no docs/specs/, no docs/ at all) — assessed against the PR description and the repo conventions in AGENTS.md / .claude/skills/developing-insta-cli/SKILL.md.
Findings
Critical
(none)
Suggestion
- Functionality — name-validation asymmetry (
src/commands/services.ts:53-65vs92-104).servicesRenamenow callsassertServiceName(newName), butservicesAddstill accepts any name (no client-side validation). This means a user can create a service with a name (e.g.Primary) that the platform/rename rule would later reject, and the two commands disagree on what a valid name is. Consider applyingassertServiceNameinservicesAddas well so creation and rename share one rule, or intentionally document that rename is the stricter gate. - Software engineering / functionality — regex is looser than "lower-kebab" (
src/commands/services.ts:7,20-22)./^[a-z0-9][a-z0-9-]{0,38}$/accepts a trailing hyphen (foo-) and consecutive hyphens (foo--bar), which aren't typical "lower-kebab". Since this is a client-side pre-check for an authoritative platform endpoint (the platform lives in the sibling../platformsubmodule and isn't visible here), an over-permissive check just defers to the server error — not a correctness bug — but it's worth confirming the pattern (including the 39-char max from{0,38}) matches the platform's actual service-name rule so the local error is accurate. Adding a test for a trailing-hyphen / max-length case would lock that intent in.
Information
- Docs mirroring (
AGENTS.mdnon-negotiable #4 / dev skill). New/renamed commands must be mirrored inskills/insta/cli-reference.mdin the insta-cloud superprojectskills/submodule "in the same change set." That doc isn't part of this repo, so it can't appear in this PR's diff — just a reminder to land thecli-reference.mdupdate alongside this so the agent-facing surface doc stays current. - Test coverage matches convention. The added
assertServiceNameunit tests (acceptsprimary-db; rejectsPrimaryand-db) follow the file's "pure, unit-tested helpers" pattern. TheservicesRenameflow itself is untested, but that's consistent with the sibling commands (servicesRemove,servicesScale,servicesUpgrade) which also unit-test only their pure helpers — no regression here.
Dimension notes
- Security: No new risk.
newNameis regex-validated then sent as a JSON body field; the serviceidin the URL comes from the platform's ownserviceslist (resolved viaresolveServiceId),branchisencodeURIComponent-escaped viaq(). No secrets logged, no auth/authorization change. - Performance: One
GET /services+ onePOST /.../rename, identical to the other service commands. No N+1, no hot-path/blocking concerns. - Conventions:
servicesRenamematches the established structure (assertType→ApiClient.load→requireProject→ resolve id →rawRequest→handleApproval→--json/info), and the commander registration +guardarity insrc/index.ts:114-116are correct.
Verdict
approved — no Critical findings. The two Suggestions (validation asymmetry, regex strictness) are non-blocking; the human approval on GitHub remains a separate action. (Note: this bot cannot cast the GitHub approval; posting as a comment.)
Fermionic-Lyu
left a comment
There was a problem hiding this comment.
LGTM, Approved. (Relaying John-bot's approved verdict — approved with the maintainer account, since John-bot can't approve its own PR.)
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/services.ts">
<violation number="1" location="src/commands/services.ts:7">
P2: The `SERVICE_NAME_RE` regex allows service names to end with a hyphen (e.g., `my-db-`) or contain consecutive hyphens (`test--double`). The error message describes the convention as "lower-kebab" but kebab-case typically doesn't allow trailing or consecutive hyphens. Consider tightening the regex to reject trailing hyphens so the CLI catches these before the platform API does, keeping the error message consistent and actionable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| export const SERVICE_TYPES = ['postgres', 'storage', 'compute'] as const | ||
| export type ServiceType = (typeof SERVICE_TYPES)[number] | ||
| const SERVICE_NAME_RE = /^[a-z0-9][a-z0-9-]{0,38}$/ |
There was a problem hiding this comment.
P2: The SERVICE_NAME_RE regex allows service names to end with a hyphen (e.g., my-db-) or contain consecutive hyphens (test--double). The error message describes the convention as "lower-kebab" but kebab-case typically doesn't allow trailing or consecutive hyphens. Consider tightening the regex to reject trailing hyphens so the CLI catches these before the platform API does, keeping the error message consistent and actionable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/services.ts, line 7:
<comment>The `SERVICE_NAME_RE` regex allows service names to end with a hyphen (e.g., `my-db-`) or contain consecutive hyphens (`test--double`). The error message describes the convention as "lower-kebab" but kebab-case typically doesn't allow trailing or consecutive hyphens. Consider tightening the regex to reject trailing hyphens so the CLI catches these before the platform API does, keeping the error message consistent and actionable.</comment>
<file context>
@@ -4,6 +4,7 @@ import { info, printJson, handleApproval, renderNextActions } from '../util.js'
export const SERVICE_TYPES = ['postgres', 'storage', 'compute'] as const
export type ServiceType = (typeof SERVICE_TYPES)[number]
+const SERVICE_NAME_RE = /^[a-z0-9][a-z0-9-]{0,38}$/
export function q(branch?: string): string {
</file context>
Summary
Test Plan
Summary by cubic
Adds a services rename CLI command to rename a service and re-key its managed secret names. Validates the new name locally with
assertServiceNameto prevent invalid requests.Written for commit 95d960e. Summary will update on new commits.