Repository navigation
feat(template): upgrade a deployed template instance, and roll it back one step - #367
Conversation
The upgrade applies every field the manifest declares for a service that already exists, always-on, public access and the volume mount path among them. The `--help` text still carried the superseded sentence saying those three were left alone, which is the opposite of what runs, and on `--yes --json` no plan is printed, so that text is all an agent ever reads. Rollback goes through the same planner, so it writes those fields too whenever the OLDER manifest declares them, and it prints no plan at all. Its two warnings were the whole disclosure and named only image, start command, port and variables. They now name every setting, with the qualifier that a field the older version is silent about keeps its current value. Also: one lookup serves both verbs, so a service that came from no template no longer tells someone who ran `rollback` there is nothing to upgrade.
… a skipped service The platform's final-review pass changed four things in the contract this command speaks. Three of them are reachable from here. Two new coded 409s arrive where only a bare `conflict (HTTP 409)` would have been printed. `template_superseded` (the instance has already moved off this deployment) and `template_version_content_changed` (the recorded manifest for that version is not the content this instance ran) both name a deployment or a version in the platform's own sentence, and that name is the whole of what makes them actionable, so they are printed as given with only the next step appended, rather than replaced by wording of ours. `template_superseded` is raised by the preview too, which is a GET and so never went through postWithAdvice — the preview now gets the same reading as the POST. `template rollback` gains `--set NAME=value`, matching `template upgrade`'s flag. Before this, a version that dropped a required variable could not be rolled back at all: the upgrade's env write is an authoritative replace, so it deleted that value, the older manifest still requires it, and the platform's 400 naming it was a dead end. Rollback prints no plan, so those names exist only in that 400 — the command prompts from it on a terminal and, with no TTY or `--yes`, fails with the `--set NAME=value` list, having rolled nothing back. A deployment's service entry can now carry `skipped: true`: a service the user deleted, which the run stepped over. Its `state` stays `pending`, which under the existing marks read as "not reached yet", so it gets its own mark and names the reason. It is also out of `partialMessage`'s healthy count — a run that did everything it could should not report a shortfall for a service that was not there — while staying in the list, because the count is what raises the question. Descriptions corrected for what the upgrade now actually does: it creates a volume a new version declares on an existing service rather than only rewriting a mount path, and a public-access change on a storage bucket reaches that bucket on every branch that has a copy.
`template_no_step_back` has two causes now: the instance was never upgraded, or it was already rolled back and the way forward is an upgrade. Only the platform can tell them apart, and the second names the version to return to. The CLI replaced that sentence with one of its own, so it printed "never upgraded" for both, which is the wrong one in the case a user reaches by rolling back twice. Moved the code into KEEP_PLATFORM_SENTENCE, which exists for exactly this: codes whose platform sentence names the thing to act on. The test that pinned the replaced wording now asserts both causes survive.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/template-upgrade.test.ts">
<violation number="1" location="test/template-upgrade.test.ts:274">
P2: This assertion depends on the test runner not having a TTY: with both streams attached to a terminal, the mocked confirmation returns true and rollback proceeds instead of rejecting. Set both `isTTY` flags to false around the assertion, then restore them.</violation>
</file>
<file name="src/commands/template.ts">
<violation number="1" location="src/commands/template.ts:279">
P3: When every service in a partial run is marked `skipped`, `ran.length` is 0 and the header prints `0/0 services healthy`. Since this change intentionally excludes skipped services from the run, special-case the all-skipped case (for example "no services were deployed — all were deleted before the run") so the count never renders as 0/0.</violation>
<violation number="2" location="src/commands/template.ts:488">
P2: An added service skips its `missing_variables` lines here, so the plan says only that the service is added while execution still asks for its required values. Print those variables before continuing.</violation>
<violation number="3" location="src/commands/template.ts:491">
P2: A null target value appears as literal `null` in the change plan, unlike a null live value. Render `f.next ?? '(none)'` so the reviewed target is clear.</violation>
<violation number="4" location="src/commands/template.ts:593">
P1: A piped upgrade skips this confirmation and still posts unless `--yes` is checked. Reject non-TTY runs without `--yes`, as rollback does, rather than applying changes unattended.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| const tty = !opts.yes && !!process.stdin.isTTY && !!process.stdout.isTTY | ||
| const variables = missing.length ? await resolveVariables(missing, given, { tty, ask }) : given | ||
|
|
||
| if (!opts.yes && tty) { |
There was a problem hiding this comment.
P1: A piped upgrade skips this confirmation and still posts unless --yes is checked. Reject non-TTY runs without --yes, as rollback does, rather than applying changes unattended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/template.ts, line 593:
<comment>A piped upgrade skips this confirmation and still posts unless `--yes` is checked. Reject non-TTY runs without `--yes`, as rollback does, rather than applying changes unattended.</comment>
<file context>
@@ -458,3 +466,202 @@ export async function templateDeploy(target: string, opts: TemplateDeployOpts =
+ const tty = !opts.yes && !!process.stdin.isTTY && !!process.stdout.isTTY
+ const variables = missing.length ? await resolveVariables(missing, given, { tty, ask }) : given
+
+ if (!opts.yes && tty) {
+ const go = await clack.confirm({ message: `Upgrade ${service} from ${plan.from_version} to ${plan.to_version}?` })
+ if (clack.isCancel(go)) throw new CliCancel()
</file context>
| if (!opts.yes && tty) { | |
| if (!opts.yes) { | |
| if (!tty) throw new Error('upgrade needs --yes without a terminal to confirm on') |
|
|
||
| it('rollback without a terminal or --yes rolls back nothing and fails', async () => { | ||
| const { api, posts } = fakeApi() | ||
| await expect(templateRollback('n8n', {}, deps(api))).rejects.toThrow(/--yes/) |
There was a problem hiding this comment.
P2: This assertion depends on the test runner not having a TTY: with both streams attached to a terminal, the mocked confirmation returns true and rollback proceeds instead of rejecting. Set both isTTY flags to false around the assertion, then restore them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/template-upgrade.test.ts, line 274:
<comment>This assertion depends on the test runner not having a TTY: with both streams attached to a terminal, the mocked confirmation returns true and rollback proceeds instead of rejecting. Set both `isTTY` flags to false around the assertion, then restore them.</comment>
<file context>
@@ -0,0 +1,277 @@
+
+ it('rollback without a terminal or --yes rolls back nothing and fails', async () => {
+ const { api, posts } = fakeApi()
+ await expect(templateRollback('n8n', {}, deps(api))).rejects.toThrow(/--yes/)
+ expect(posts).toEqual([])
+ })
</file context>
| if (s.added) { lines.push(` ${who} added by ${plan.to_version}`); continue } | ||
| for (const f of s.fields) { | ||
| const drift = f.drifted ? ' (you changed this, the upgrade will overwrite it)' : '' | ||
| lines.push(` ${who} ${f.label} ${f.live ?? '(none)'} → ${f.next}${drift}`) |
There was a problem hiding this comment.
P2: A null target value appears as literal null in the change plan, unlike a null live value. Render f.next ?? '(none)' so the reviewed target is clear.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/template.ts, line 491:
<comment>A null target value appears as literal `null` in the change plan, unlike a null live value. Render `f.next ?? '(none)'` so the reviewed target is clear.</comment>
<file context>
@@ -458,3 +466,202 @@ export async function templateDeploy(target: string, opts: TemplateDeployOpts =
+ if (s.added) { lines.push(` ${who} added by ${plan.to_version}`); continue }
+ for (const f of s.fields) {
+ const drift = f.drifted ? ' (you changed this, the upgrade will overwrite it)' : ''
+ lines.push(` ${who} ${f.label} ${f.live ?? '(none)'} → ${f.next}${drift}`)
+ }
+ for (const name of s.missing_variables) {
</file context>
| lines.push(` ${who} ${f.label} ${f.live ?? '(none)'} → ${f.next}${drift}`) | |
| lines.push(` ${who} ${f.label} ${f.live ?? '(none)'} → ${f.next ?? '(none)'}${drift}`) |
| for (const s of plan.services) { | ||
| const who = s.service_name ?? s.key | ||
| if (s.gone) { lines.push(` ${who} no longer exists, skipped`); continue } | ||
| if (s.added) { lines.push(` ${who} added by ${plan.to_version}`); continue } |
There was a problem hiding this comment.
P2: An added service skips its missing_variables lines here, so the plan says only that the service is added while execution still asks for its required values. Print those variables before continuing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/template.ts, line 488:
<comment>An added service skips its `missing_variables` lines here, so the plan says only that the service is added while execution still asks for its required values. Print those variables before continuing.</comment>
<file context>
@@ -458,3 +466,202 @@ export async function templateDeploy(target: string, opts: TemplateDeployOpts =
+ for (const s of plan.services) {
+ const who = s.service_name ?? s.key
+ if (s.gone) { lines.push(` ${who} no longer exists, skipped`); continue }
+ if (s.added) { lines.push(` ${who} added by ${plan.to_version}`); continue }
+ for (const f of s.fields) {
+ const drift = f.drifted ? ' (you changed this, the upgrade will overwrite it)' : ''
</file context>
| if (s.added) { lines.push(` ${who} added by ${plan.to_version}`); continue } | |
| if (s.added) { | |
| lines.push(` ${who} added by ${plan.to_version}`) | |
| for (const name of s.missing_variables) { | |
| lines.push(` ${who} ${name} is required by ${plan.to_version} and has no value, pass --set ${name}=...`) | |
| } | |
| continue | |
| } |
| const healthy = ran.filter((s) => s?.state === 'healthy').length | ||
| return [ | ||
| `template deployment finished partial: ${healthy}/${services.length} services healthy`, | ||
| `template deployment finished partial: ${healthy}/${ran.length} services healthy`, |
There was a problem hiding this comment.
P3: When every service in a partial run is marked skipped, ran.length is 0 and the header prints 0/0 services healthy. Since this change intentionally excludes skipped services from the run, special-case the all-skipped case (for example "no services were deployed — all were deleted before the run") so the count never renders as 0/0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/template.ts, line 279:
<comment>When every service in a partial run is marked `skipped`, `ran.length` is 0 and the header prints `0/0 services healthy`. Since this change intentionally excludes skipped services from the run, special-case the all-skipped case (for example "no services were deployed — all were deleted before the run") so the count never renders as 0/0.</comment>
<file context>
@@ -266,9 +270,13 @@ export function serviceStateLines(dep: any): string[] {
+ const healthy = ran.filter((s) => s?.state === 'healthy').length
return [
- `template deployment finished partial: ${healthy}/${services.length} services healthy`,
+ `template deployment finished partial: ${healthy}/${ran.length} services healthy`,
...serviceStateLines(dep),
...(dep?.error ? [` ${dep.error}`] : []),
</file context>
| `template deployment finished partial: ${healthy}/${ran.length} services healthy`, | |
| `template deployment finished partial: ${ran.length ? `${healthy}/${ran.length} services healthy` : 'no services were deployed — all were deleted before the run'}`, | |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The implementation is generally sound, but the required agent-facing command reference has not been updated.
Requirements context
I assessed the change against the PR description, the existing template command conventions, AGENTS.md, the repository development guide, and the README command surface. The linked platform #645 and console #601 content was unavailable from this checkout, so their contracts could only be evaluated through the PR description and client implementation.
Findings
Critical
- The new
template upgradeandtemplate rollbackcommands and their flags are absent from the requiredskills/insta/cli-reference.mdupdate. The repository explicitly classifies this mirror as non-negotiable and says a command is only half-complete without it; this PR changes the command surface but contains only CLI source/tests. Please update the superproject reference or identify the companion change that does so. (AGENTS.md:8-17,.claude/skills/developing-insta-cli/SKILL.md:36-38,src/index.ts:576-587)
Suggestion
-
Update the local command summary as well: it still says
insta templateonly browses and deploys vialist,info, anddeploy, so users reading the repository documentation will not discover either new operation. (README.md:240) -
Add direct upgrade-variable coverage. The tests verify plan pinning and JSON behavior, and extensively exercise rollback
--set, but do not demonstrate that upgrade forwards a supplied value, prompts for a newly required variable on a TTY, or refuses before POST when that value is unavailable non-interactively. (test/template-upgrade.test.ts:88-110,test/template-upgrade.test.ts:224-263) -
The development guide asks every new leaf command to state its MCP reachability decision in the PR description. This is explicitly non-blocking, but neither new command has that decision recorded. (
.claude/skills/developing-insta-cli/SKILL.md:65-79,src/index.ts:576-587)
Information
-
Software engineering/functionality: service-name resolution, pinned version/digest submission, refusal handling, confirmation rules, approval gates, coded conflicts, rollback retry behavior, and deployment polling follow existing patterns. (
src/commands/template.ts:527-666) -
Security: no new dependency, shell/SQL execution, authorization bypass, or explicit secret logging was found; supplied variables remain confined to API request bodies. (
src/commands/template.ts:587-603,src/commands/template.ts:642-658) -
Performance: no material concern found. Each operation adds one bounded service-list lookup, one preview for upgrades, one mutation, and the existing bounded deployment poll; there is no new N+1 or blocking synchronous work. (
src/commands/template.ts:548-571,src/commands/template.ts:601-608,src/commands/template.ts:647-663) -
git diff --checkpasses. I could not independently run the typecheck or tests becausenode_modulesis absent and the read-only review prohibited installing dependencies;npm run typecheckstopped attsc: not found. (package.json:35-40)
Verdict
Request changes because an explicit, non-negotiable command-documentation requirement is unmet.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The commands largely follow existing CLI conventions, but the upgrade flow does not yet guarantee that the displayed plan is the plan applied.
Requirements context
I assessed this against the PR description, command help, AGENTS.md, and .claude/skills/developing-insta-cli/SKILL.md. The linked platform/console PR contents were not available in this checkout, so no additional requirements were inferred from them; the separate command-reference update was accepted as described but could not be verified here.
Findings
Critical
-
The preview is not pinned to the live service state it describes. The GET obtains live values for the plan, but the POST sends only the template version and digest. If a service setting changes while the confirmation is open, that change does not affect either pin, and the upgrade can overwrite a value that was never displayed or marked as drifted. This violates the stated guarantee that “the plan the user saw is the plan that runs.” Have the platform return a plan token or live-state revision/hash and submit it with the POST, rejecting stale plans.
src/commands/template.ts:569-573,src/commands/template.ts:599-603 -
Added services omit the settings and required variables that will be applied. The
continueafter printing “added by …” bypasses both the field loop andmissing_variablesloop. Consequently, an added service’s image, port, access, volume settings, and required variables are not listed even though the command help promises every declared setting is shown before application. Print the added-service heading without skipping its details, and add a regression test for an added service with fields and a missing variable.src/commands/template.ts:485-495,src/index.ts:576-580
Suggestion
-
Update the repository’s command summary, which still lists only
template list,info, anddeploy.README.md:240 -
The development guide asks every new leaf command to state its MCP reachability decision in the PR description. Neither new command has that decision documented; this is explicitly non-blocking.
.claude/skills/developing-insta-cli/SKILL.md:65-79
Information
- Software engineering: the tests cover the main request paths, JSON modes, coded conflicts, missing variables, and skipped services.
test/template-upgrade.test.ts:1-277,test/template.test.ts:1117-1131 - Security: no security-relevant issue found;
--setnames are validated and values are sent in JSON without being printed.src/commands/template.ts:141-149,src/commands/template.ts:587-603 - Performance: no material concern found; the new flow adds bounded lookup/preview requests and reuses the existing deployment poller.
src/commands/template.ts:548-571,src/commands/template.ts:606-608 git diff --checkpassed. The required typecheck/tests could not be rerun because dependencies are absent in the review checkout (tsc: not found); installing them would violate the read-only constraint.AGENTS.md:15
Verdict
Request changes because both Critical findings affect the advertised safety and completeness of the upgrade plan.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The new commands largely follow existing CLI patterns, but the implementation does not yet provide the promised plan for added services and can disclose mistyped --set secrets.
Requirements context
I assessed the change against the PR description, command help, the repository’s command conventions, and the stated paired platform/console/skills PR contracts. No additional upgrade/rollback design document exists in this checkout; the linked repositories were not accessible from this environment. The repository requires command-reference mirroring, which the description identifies as InsForge/instacloud-skills#170.
Findings
Critical
-
Added services skip the per-field plan the command promises.
upgradePlanLinesemits onlyadded by <version>and immediately continues, so it omits that service’s image, port, public access, volume configuration, variables, and evenmissing_variables. This conflicts with the command help’s guarantee that every declared setting is listed before application and prevents the user from reviewing material resource/access changes before confirmation. Render the added-service fields and non-secret variable requirements before continuing, and add coverage usingadded: true; the current variable-plan test covers only an existing service. (src/commands/template.ts:485-495,src/index.ts:576-580,test/template-upgrade.test.ts:42-46) -
A malformed
--setcan disclose its value. Both new commands accept secret-like values through--set, but the shared parser includes the complete rejected pair in its error. A common typo such as a lowercase variable name therefore writes the supplied credential to stderr and potentially CI/agent logs. Report only the variable name or redact everything after the first=; extend the existing malformed-input test to assert that the value is absent. (src/commands/template.ts:141-146,src/commands/template.ts:557-558,src/commands/template.ts:615-616,test/template-upgrade.test.ts:265-269)
Suggestion
(none)
Information
- Software engineering: The main success, refusal, JSON, missing-variable, rollback-confirmation, skipped-service, and coded-409 paths have focused tests and follow the repository’s dependency-injection conventions. I could not execute the suite or typecheck because this read-only checkout has no installed dependencies (
tsc: not found). - Functionality: Beyond the added-service plan omission above, I found no additional correctness issue in service resolution, version/digest pinning, confirmation behavior, or deployment polling.
- Security: No new dependency, authorization bypass, shell execution, or unsafe URL construction was introduced; the secret-bearing error described above is the security blocker.
- Performance: No performance concerns found. Each operation performs a bounded number of setup requests followed by the existing bounded deployment poll, with no new N+1 or synchronous hot-path work.
Verdict
Request changes because the PR has two Critical findings: incomplete disclosure before an upgrade and possible secret exposure.
… value The upgrade plan printed `added by <version>` for a service the new version introduces and then skipped the rest of that service: its field rows and its `missing_variables` were never rendered, so the command's own promise that every declared setting is listed before anything runs was false exactly where it mattered most, a service that does not exist yet and will be created and billed. The added line now says the service is created by this upgrade and the loop falls through to whatever rows the plan carries for it, which today is its required variables and, the day the planner sends them, its fields. The help text said "the same services are reused" and promised a per-setting plan for all of them. It now separates the three cases the platform actually reports: an existing service is reused and every declared setting is applied and listed first, an added service is created and named with the variables it requires rather than a per-setting comparison (there is no live service to compare against), and a service the new version no longer declares is left running and named as dropped. parseSetFlags put the whole rejected pair in its error. `--set` is how these commands take secrets, and a lowercase name is a common typo, so one typo wrote a live credential to stderr and from there into CI and agent logs. The parser now shows the name and hides everything after the first `=`; a pair with no `=` is not split at all, so none of it is shown. Redacting here rather than at the three call sites means a fourth caller cannot reintroduce the leak.
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/template.ts">
<violation number="1" location="src/commands/template.ts:155">
P2: Malformed names are echoed without validation, so `--set hunter2-live-key=x` leaks `hunter2-live-key` to stderr despite being rejected. Show the prefix only when it matches the allowed variable-name pattern; otherwise redact the whole token.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
| function redactSetPair(pair: string): string { | ||
| const eq = pair.indexOf('=') | ||
| // Nothing of a pair with no `=`: it was never split, so the whole token may be the value itself. | ||
| return eq >= 0 ? `${pair.slice(0, eq)}=<value hidden>` : '<value hidden: no NAME= in front of it>' |
There was a problem hiding this comment.
P2: Malformed names are echoed without validation, so --set hunter2-live-key=x leaks hunter2-live-key to stderr despite being rejected. Show the prefix only when it matches the allowed variable-name pattern; otherwise redact the whole token.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/template.ts, line 155:
<comment>Malformed names are echoed without validation, so `--set hunter2-live-key=x` leaks `hunter2-live-key` to stderr despite being rejected. Show the prefix only when it matches the allowed variable-name pattern; otherwise redact the whole token.</comment>
<file context>
@@ -142,12 +142,19 @@ export function parseSetFlags(pairs: string[]): Record<string, string> {
+function redactSetPair(pair: string): string {
+ const eq = pair.indexOf('=')
+ // Nothing of a pair with no `=`: it was never split, so the whole token may be the value itself.
+ return eq >= 0 ? `${pair.slice(0, eq)}=<value hidden>` : '<value hidden: no NAME= in front of it>'
+}
+
</file context>
| return eq >= 0 ? `${pair.slice(0, eq)}=<value hidden>` : '<value hidden: no NAME= in front of it>' | |
| const name = pair.slice(0, eq) | |
| return eq >= 0 && /^[A-Z][A-Z0-9_]{0,63}$/.test(name) ? `${name}=<value hidden>` : '<value hidden: no valid NAME= in front of it>' |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The new template upgrade and one-step rollback commands match the stated contract and fit the existing CLI architecture, with no blocking findings.
Requirements context
I reviewed against the PR description, the repository guidance in AGENTS.md and .claude/skills/developing-insta-cli/SKILL.md, existing template command conventions, and the paired agent-facing documentation in instacloud-skills#170. The linked platform and console PRs were not publicly accessible, so their API implementation details could not be independently inspected.
Findings
Critical
(none)
Suggestion
test/template-upgrade.test.ts:104-125— Add direct upgrade-flow coverage for forwarding--set, rejecting an unresolved required variable under--yes/without a TTY, and declining the confirmation. The implementation atsrc/commands/template.ts:595-605appears correct and reuses tested helpers, but the current upgrade tests only assert an emptyvariablesobject, leaving core advertised behavior without an integration-style regression test.
Information
src/commands/template.ts:556-673— Functionality follows the documented flow: service-to-deployment resolution, pinned version/digest application, refusal handling, confirmation rules, approval gates, rollback variable recovery, and bounded deployment polling.src/commands/template.ts:141-155— Security review found no newly exposed secrets or unsafe shell/SQL execution. Invalid--setvalues are now redacted, request paths use trusted identifiers or encode the branch, and no authorization behavior is bypassed.src/commands/template.ts:572-616— Performance review found no material concern: upgrade adds one service-list lookup and one preview request before using the existing bounded polling loop; there are no N+1 operations, blocking hot-path work, or new dependencies.src/index.ts:576-587— Command registration and flag shapes follow repository conventions, and the required agent-facing mirror exists in the paired documentation PR.test/template-upgrade.test.ts:1-298—git diff --checkpassed. I could not execute typecheck or tests locally because this read-only checkout has no installedtypescriptorvitest; the reported CI verification therefore remains the executable evidence.
Verdict
Approved: zero Critical findings. The testing suggestion is non-blocking.
…tually prints The command help said an added service is named with the variables it requires and not a per-setting comparison. upgradePlanLines prints the added-by line and then every field as `(none) -> <new value>`, which is what insta-platform #645 made the plan carry. Same sentence as the one corrected in insta-skills #170, which this one was copied from.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index.ts">
<violation number="1" location="src/index.ts:576">
P3: The plan prints `→`, not `->`; use the rendered arrow so the help matches the actual added-service rows.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| .option('-y, --yes', 'non-interactive: missing required variables fail with a --set list instead of prompting') | ||
| .option('--json') | ||
| .action(guard((target, o) => template.templateDeploy(target, o))) | ||
| tpl.command('upgrade <service>').description('Move a service deployed from a template to the version the registry serves now. Shows what will change and asks before it runs. A service that already exists is reused, and every setting the new version declares is applied: image, start command, port, always-on, public access, the volume and its mount path, and the variables, each of them listed in the plan first. A service the new version ADDS is created by the upgrade: the plan names it as added and then lists every setting it will be created with, each written `(none) -> <new value>` because there is no live service to compare against, followed by the variables it requires. A service the new version no longer declares is left running, named in the plan as dropped. A volume the new version adds is attached at your plan\'s size and mounted where the manifest says; an existing one is never resized. For a storage bucket, a public-access change reaches that bucket on every branch that has a copy. Refused with a 409 when the template was republished since the plan, or when this instance has already moved off this deployment') |
There was a problem hiding this comment.
P3: The plan prints →, not ->; use the rendered arrow so the help matches the actual added-service rows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/index.ts, line 576:
<comment>The plan prints `→`, not `->`; use the rendered arrow so the help matches the actual added-service rows.</comment>
<file context>
@@ -573,7 +573,7 @@ tpl.command('deploy <code-or-dir-or-url>').description('Deploy a template onto a
.option('--json')
.action(guard((target, o) => template.templateDeploy(target, o)))
-tpl.command('upgrade <service>').description('Move a service deployed from a template to the version the registry serves now. Shows what will change and asks before it runs. A service that already exists is reused, and every setting the new version declares is applied: image, start command, port, always-on, public access, the volume and its mount path, and the variables, each of them listed in the plan first. A service the new version ADDS is created by the upgrade: the plan names it as added, with the variables it requires, and not a per-setting comparison, because there is no live service to compare it against. A service the new version no longer declares is left running, named in the plan as dropped. A volume the new version adds is attached at your plan\'s size and mounted where the manifest says; an existing one is never resized. For a storage bucket, a public-access change reaches that bucket on every branch that has a copy. Refused with a 409 when the template was republished since the plan, or when this instance has already moved off this deployment')
+tpl.command('upgrade <service>').description('Move a service deployed from a template to the version the registry serves now. Shows what will change and asks before it runs. A service that already exists is reused, and every setting the new version declares is applied: image, start command, port, always-on, public access, the volume and its mount path, and the variables, each of them listed in the plan first. A service the new version ADDS is created by the upgrade: the plan names it as added and then lists every setting it will be created with, each written `(none) -> <new value>` because there is no live service to compare against, followed by the variables it requires. A service the new version no longer declares is left running, named in the plan as dropped. A volume the new version adds is attached at your plan\'s size and mounted where the manifest says; an existing one is never resized. For a storage bucket, a public-access change reaches that bucket on every branch that has a copy. Refused with a 409 when the template was republished since the plan, or when this instance has already moved off this deployment')
.option('--branch <b>', 'branch the service is on (default: current)')
.option('--set <NAME=value>', 'set a template variable the new version requires (repeatable)', (v: string, prev: string[]) => [...prev, v], [] as string[])
</file context>
| tpl.command('upgrade <service>').description('Move a service deployed from a template to the version the registry serves now. Shows what will change and asks before it runs. A service that already exists is reused, and every setting the new version declares is applied: image, start command, port, always-on, public access, the volume and its mount path, and the variables, each of them listed in the plan first. A service the new version ADDS is created by the upgrade: the plan names it as added and then lists every setting it will be created with, each written `(none) -> <new value>` because there is no live service to compare against, followed by the variables it requires. A service the new version no longer declares is left running, named in the plan as dropped. A volume the new version adds is attached at your plan\'s size and mounted where the manifest says; an existing one is never resized. For a storage bucket, a public-access change reaches that bucket on every branch that has a copy. Refused with a 409 when the template was republished since the plan, or when this instance has already moved off this deployment') | |
| tpl.command('upgrade <service>').description('Move a service deployed from a template to the version the registry serves now. Shows what will change and asks before it runs. A service that already exists is reused, and every setting the new version declares is applied: image, start command, port, always-on, public access, the volume and its mount path, and the variables, each of them listed in the plan first. A service the new version ADDS is created by the upgrade: the plan names it as added and then lists every setting it will be created with, each written `(none) → <new value>` because there is no live service to compare against, followed by the variables it requires. A service the new version no longer declares is left running, named in the plan as dropped. A volume the new version adds is attached at your plan\'s size and mounted where the manifest says; an existing one is never resized. For a storage bucket, a public-access change reaches that bucket on every branch that has a copy. Refused with a 409 when the template was republished since the plan, or when this instance has already moved off this deployment') |
2 participants
What
insta template upgrade <service>moves a service deployed from a template to the version theregistry serves now, and
insta template rollback <service>returns it one step.Both address a service name rather than a deployment id, because that is what a person and an
agent already have in hand. The CLI resolves the deployment from the service.
Paired with insta-platform #645 and insta-console #601.
The agent-facing command reference this repository requires as a mirror is updated in
InsForge/instacloud-skills#170, which documents both commands and every flag added here.
It lives in the superproject's
skillssubmodule, so it cannot be a file in this PR.How
upgradeprints the plan before it does anything: the version move, and per field the live valueagainst the new one. A field changed by hand after deploying is marked so, because the upgrade will
overwrite it. Then it asks, and
--yesor no TTY skips the question.The plan the user saw is the plan that runs. The CLI sends back the version and digest from it, so
a republish in between answers 409 and nothing happens, with a sentence telling the user to look
again.
--set NAME=valuesupplies a required variable the new version adds. Variables the instancealready holds are never asked for again, and generated secrets are recovered by the platform rather
than transiting the CLI.
rollbackprints its warnings before the confirmation: what the older version restores, that asetting it does not declare keeps its current value, that a public access change on a storage
service reaches that bucket on every branch, and that data the app migrated under the newer version
is not restored. It takes
--settoo, because an upgrade can drop a variable the older versionstill requires.
Coded 409s reach the user as sentences rather than a bare conflict. Two of them name the deployment
or the version to act on, and those keep the platform's own wording instead of being replaced with
ours, which is what
KEEP_PLATFORM_SENTENCEis for.Verify
npm run typecheckandnpm test, 103 files and 2141 tests--setpaths, the destroy-without-asking rule off a TTYand each coded 409 all have cases
asserts the platform sentence survives for each
🤖 Generated with Claude Code
Summary by cubic
Adds
insta template upgrade <service>andinsta template rollback <service>so a deployed template service can move to the version the registry serves now, or one step back. Both commands take a service name rather than a deployment id, show what will change before running, and confirm before acting.upgradeprints the plan — the version move and, per field, the live value versus the new one, with hand-changed fields marked as being overwritten — then sends back the version and digest it showed, so a republish in between answers 409 and nothing runs.(none) → <new value>plus its required variables; a dropped one is left running. A skipped service now reads[skipped: …]instead of[pending]in deployment output and no longer counts against the healthy total.--set NAME=valuesupplies a required variable the new version adds; variables the instance already holds are never re-asked, and generated secrets are recovered by the platform. A rejected--setnever echoes the value, which is where a credential sits.rollbackprints its warnings before confirmation: what the older version restores, that undeclared settings keep their current value, that a public-access change on a storage service reaches that bucket on every branch, and that app-migrated data is not restored. It takes--settoo, and off a TTY it fails without--yes.Written for commit 549f58d. Summary will update on new commits.