Repository navigation
Conversation
The local manifest check now takes `source` as the third way to define a web or worker service, next to image and build. Exactly one of the three is required, and the owner, repo, branch, rootDir and buildCommand rules and messages are the platform parser's, so an author hears the refusal before the upload. A present but malformed source counts as declared, so it is reported as one problem instead of also reading as missing. The CLI validates and never normalizes. The manifest travels as written, and an unknown key inside source is left to the platform, as it is for every service key.
template deploy reads the platform's coded refusal of a private repo the deployer cannot read, github_not_linked or github_repo_unreachable with its repos. On a terminal or with --agent it links GitHub with the device flow, opens the GitHub App install for a repo still out of reach, waits until every listed repo is readable and deploys again, once. With --json or with no terminal it prints the platform's message and exits non-zero. A missing-variables refusal is still answered once, before or after it. authorizeTerminal and findCallerRepo take any client with request(), so the deploy path reuses them as connect-repo does. The help of template deploy and template edit says how source works.
The template edit help said switching between image and source takes a new
service. The platform refuses that with "delete it and add a new one", and a
service that comes from the project cannot be deleted from a draft, only
marked removed. The help now names both ("delete" for a service the author
added, "removed" for one from the project) and says to add the new one under
another name.
template draft printed nothing for a service built from GitHub, since the
platform's draft view gives it image null plus source. It now prints
"builds from owner/repo@branch (rootDir/)", or "(default branch)" when the
source names no branch.
The source rules comment names the platform's parseTemplateSource. Tests pin
source null as "must be a map" and a second missing-variables refusal as the
error rather than another prompt.
There was a problem hiding this comment.
2 issues found across 8 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="src/template-manifest.ts">
<violation number="1" location="src/template-manifest.ts:75">
P2: This accepts invalid GitHub owners such as `acme-`, so local validation lets an unusable source reach deployment. Require an alphanumeric final character and reject consecutive hyphens.</violation>
</file>
<file name="src/commands/template.ts">
<violation number="1" location="src/commands/template.ts:446">
P3: `--yes` promises a non-interactive run, but it only suppresses the variable prompt (`tty` includes `!opts.yes`); the new GitHub-authorization gate `canAuthorizeHere(opts)` ignores `opts.yes`, so `insta template deploy <code> --yes` on a terminal still starts the device/App-install flow and can block for up to 15 minutes per repo. Gate the GitHub path behind `--yes` as well, so `--yes` means no interactive steps of either kind.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| // The platform's HEALTHCHECK_RE (src/provisioning/templateManifest.ts), verbatim. | ||
| const HEALTHCHECK_RE = /^\/(?!\/)[A-Za-z0-9\-._~!$&'()*+,;=:@%/?]*$/ | ||
| // GitHub's grammars for a source's owner and repo, as the platform parser has them. | ||
| const GITHUB_OWNER_RE = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})$/ |
There was a problem hiding this comment.
P2: This accepts invalid GitHub owners such as acme-, so local validation lets an unusable source reach deployment. Require an alphanumeric final character and reject consecutive hyphens.
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/template-manifest.ts, line 75:
<comment>This accepts invalid GitHub owners such as `acme-`, so local validation lets an unusable source reach deployment. Require an alphanumeric final character and reject consecutive hyphens.</comment>
<file context>
@@ -67,6 +71,9 @@ export const ENV_NAME_RE = /^[A-Z][A-Z0-9_]{0,63}$/
// The platform's HEALTHCHECK_RE (src/provisioning/templateManifest.ts), verbatim.
const HEALTHCHECK_RE = /^\/(?!\/)[A-Za-z0-9\-._~!$&'()*+,;=:@%/?]*$/
+// GitHub's grammars for a source's owner and repo, as the platform parser has them.
+const GITHUB_OWNER_RE = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})$/
+const GITHUB_REPO_RE = /^[A-Za-z0-9._-]{1,100}$/
</file context>
| const GITHUB_OWNER_RE = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})$/ | |
| const GITHUB_OWNER_RE = /^(?!.*--)[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$/ |
| Object.assign(variables, await resolveVariables(missing, {}, { tty, ask })) | ||
| res = await api.rawRequest('POST', `/projects/${p.projectId}/template-deployments`, { ...body, variables }) | ||
| // Each refusal the CLI can answer is answered once, in whichever order the platform raises them. | ||
| const canAuthorize = canAuthorizeHere(opts) |
There was a problem hiding this comment.
P3: --yes promises a non-interactive run, but it only suppresses the variable prompt (tty includes !opts.yes); the new GitHub-authorization gate canAuthorizeHere(opts) ignores opts.yes, so insta template deploy <code> --yes on a terminal still starts the device/App-install flow and can block for up to 15 minutes per repo. Gate the GitHub path behind --yes as well, so --yes means no interactive steps of either kind.
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 446:
<comment>`--yes` promises a non-interactive run, but it only suppresses the variable prompt (`tty` includes `!opts.yes`); the new GitHub-authorization gate `canAuthorizeHere(opts)` ignores `opts.yes`, so `insta template deploy <code> --yes` on a terminal still starts the device/App-install flow and can block for up to 15 minutes per repo. Gate the GitHub path behind `--yes` as well, so `--yes` means no interactive steps of either kind.</comment>
<file context>
@@ -426,16 +442,30 @@ export async function templateDeploy(target: string, opts: TemplateDeployOpts =
- Object.assign(variables, await resolveVariables(missing, {}, { tty, ask }))
- res = await api.rawRequest('POST', `/projects/${p.projectId}/template-deployments`, { ...body, variables })
+ // Each refusal the CLI can answer is answered once, in whichever order the platform raises them.
+ const canAuthorize = canAuthorizeHere(opts)
+ let askedVariables = false
+ let askedGitHub = false
</file context>
| const canAuthorize = canAuthorizeHere(opts) | |
| const canAuthorize = !opts.yes && canAuthorizeHere(opts) |
jwfing
left a comment
There was a problem hiding this comment.
Summary
The GitHub-source deployment flow is well structured overall, but the new draft rendering introduces a terminal-control injection path that should be fixed before merge.
Requirements context
I assessed the change against the detailed PR description, the repository guidance in AGENTS.md, CONTRIBUTING.md, and .claude/skills/developing-insta-cli/SKILL.md, plus the existing GitHub connection and template behavior. The linked design and platform PR were not present in this checkout and were not publicly accessible, so the PR description was the primary feature specification.
Findings
Critical
- Sanitize source metadata before rendering it to a terminal. The source validator permits ESC and other non-whitespace control characters in
branch, and permits control characters generally inrootDir.serviceTextthen interpolates both fields directly into terminal output. A crafted stored draft can therefore emit ANSI/OSC sequences when another user runsinsta template draft, potentially altering displayed output or terminal state. Reject control characters during validation or pass these values through the repository's existingsafeTexthelper, and add a regression test containing ESC/C1 input. (src/template-manifest.ts:128-135,src/commands/template-author.ts:100-115,src/config.ts:300-305)
Suggestion
- The retry loop supports missing variables followed by GitHub authorization, but the tests directly cover only the reverse ordering. Add a test with
missing_variablesfirst andgithub_not_linkedsecond to pin the PR's explicit “whichever order” requirement. (test/template.test.ts:1188-1202)
Information
- Software engineering: The implementation follows the repository's dependency-injection and error-handling conventions. Manifest validation, deduplication, authorization suppression under
--json/non-TTY operation, retry limits, draft rendering, and help text all have focused tests. (test/template.test.ts:95-112,test/template.test.ts:630-648,test/template.test.ts:1112-1202,test/template-author.test.ts:172-182,test/help-surface.test.ts:206-224) - Functionality: Apart from the critical rendering issue, the deploy loop correctly limits each recoverable refusal to one handling attempt and preserves the request body across retries. (
src/commands/template.ts:445-472) - Security: No secrets are newly returned or logged, authorization is not weakened, and no dependencies were added. The terminal-output issue above is the security-relevant blocker.
- Performance: Repository refusals are deduplicated case-insensitively before authorization, and existing polling remains time-bounded; no hot-path synchronous work or unbounded retry loop was introduced. (
src/commands/template.ts:198-207,src/commands/template.ts:445-468,src/commands/github.ts:179-204) - I could not rerun the suite locally because dependencies are absent (
tscwas unavailable), but the public Linux test job and Windows test job both completed successfully for the reviewed head.
Verdict
Request changes. The terminal-control injection is a blocking security issue; after sanitizing the new source fields and adding a regression test, the remaining implementation looks suitable to merge.
A stored draft can carry an escape sequence in a source branch or rootDir, and the draft view printed it to the terminal as is. serviceText now passes owner, repo, branch and rootDir through safeText. The same goes for what a GitHub access refusal puts on screen: the platform message written to stderr before the GitHub flow, and the owner and repo that findCallerRepo prints and throws. validateManifest now follows the platform's order. The required sentence comes first and counts truthy image or build or a present source. Only when that passes is the mutual exclusion counted by key presence, so an empty image or build beside another way is refused as mutually exclusive, while an empty image alone, or an empty image beside an empty build, gets only the required sentence. The message strings are unchanged. Adds a deploy test for the missing_variables then github_not_linked order.
jwfing
left a comment
There was a problem hiding this comment.
Summary
The change cleanly adds GitHub-backed template services and the associated authorization/retry workflow without introducing a blocking correctness, security, or performance issue.
Requirements context
I assessed the change against the PR description, repository guidance, existing GitHub connection behavior, and added tests. The linked platform PR and design document were not present in the checkout and could not be fetched from this environment, so their requirements were taken from the detailed PR description rather than independently inspected.
Findings
Critical
(none)
Suggestion
(none)
Information
- Software engineering / functionality: The validator covers the stated
image/build/sourceordering and field constraints, including null and empty-value cases. The deploy state machine permits each recoverable refusal once and supports either refusal order, with targeted regression coverage. (src/template-manifest.ts:121-139,src/template-manifest.ts:166-175,src/commands/template.ts:448-471,test/template.test.ts:84-132,test/template.test.ts:1137-1258) - Security: Platform-supplied messages and repository identifiers are stripped of terminal control characters, JSON/non-terminal execution does not initiate GitHub authorization, and no new dependency or secret-handling path is introduced. (
src/commands/template.ts:198-210,src/commands/template.ts:465-470,src/commands/template-author.ts:100-116,test/template.test.ts:1172-1185,test/template.test.ts:1252-1257) - Performance: Deploy retries are bounded to one variable recovery and one GitHub recovery. The reused GitHub helpers cap authorization and repository-access polling and back off their cadence, so there is no unbounded hot loop or blocking synchronous work. (
src/commands/template.ts:448-471,src/commands/github.ts:100-139,src/commands/github.ts:179-190) - Verification:
git diff --check main...HEADpassed. The repository’s typecheck and test commands could not be executed because this read-only checkout lacks installed dev dependencies (tscwas unavailable); I did not install packages. (package.json:35-42)
Verdict
Approved under the review rubric: no Critical findings.
What
insta template deploycan now deploy a template whose services build from a GitHub repo. This is the cli part of template GitHub sources. The platform part is platform #654 and the design is Templates: services built from a GitHub repo.A manifest can name a
source. The local check that runs before a directory or GitHub URL deploy takessourceas a third way besideimageandbuildon a web or worker service. It applies the platform's field rules to owner, repo, branch,rootDirandbuildCommand. A service with none of the three, or with two, is refused locally. The check follows the platform's order. When no way is usable it says one of the three is required, and only otherwise does it count the keys present for the exclusion. So an emptyimageorbuildbeside another way is refused as mutually exclusive, and an emptyimageon its own, or beside an emptybuild, gets only the required sentence.A private repo links GitHub from the terminal. When the platform refuses a deploy with
github_not_linkedorgithub_repo_unreachable, and the CLI can authorize here (a terminal or--agent, never--json), it prints the platform's message with control characters stripped, runs the GitHub device flow when no account is linked, opens the App install page for a repo it cannot reach yet, waits for access, and deploys again once. With--json, or outside a terminal without--agent, it makes no GitHub call and rethrows the platform's error unchanged, as before. A refusal with nogithub_*code is thrown as before, which includes a missing branch, because the platform answers that as a plain 400.A missing-variables prompt still works. The CLI answers a
missing_variablesrefusal once and a GitHub refusal once, in whichever order the platform raises them. A second refusal of either kind is the error.insta template draftshows a GitHub service. A service built from a repo printsbuilds from owner/repo@branch, orowner/repo (default branch)when no branch is named, with(rootDir/)when it has one. The owner, repo, branch androotDircome from a stored draft, so they are printed with control characters removed. A branch carrying an escape sequence shows as plain text and never reaches the terminal as a sequence.Help text.
template deploysays what a private repo needs.template editshows theservicesentry that adds a GitHub service, how asourcesetting replaces the whole source, and how to switch between image and source.Merge order. This merges after platform #654 is in production, because an older platform refuses a manifest with
source. A release follows (expected v0.1.24), and this PR does not change the version. The skills PR documents the new surface, as the cli AGENTS.md rule 4 asks, and it stays a draft until that release ships. The oss, mcp, console, skills and e2e changes are separate PRs that link to #654.How
src/template-manifest.tsaddsManifestSource,ManifestService.source,GITHUB_OWNER_RE,GITHUB_REPO_REandsourceProblems.validateManifestfirst checks that a way is usable, a truthyimageorbuildor a presentsource, and says one of the three is required when none is. Only when that passes does it count the mutual exclusion by key presence ofimage,buildandsource. Sosource: nullgetssource must be a mapand is never read as absent,image: ''besidesourceis mutually exclusive, andimage: ''alone or besidebuild: ''gets only the required sentence. The message strings are unchanged. It reports every problem at once, as the check already does. It normalizes nothing, and an unknown key insidesourceis left to the platform, whose strict mode refuses it. Managed types and unknown service keys stay the platform's call.templateDeployinsrc/commands/template.tsposts inside awhile (!res)loop that sends{ ...body, variables }each time.unreachableReposFromreads thecodeandreposof a GitHub access refusal, each repo once and case-insensitive, and returns null for any other body. It passes owner and repo throughsafeTextfromsrc/config.ts, becausefindCallerRepoprints and throws them. On a refusal the CLI writes the platform's message to stderr throughsafeTexttoo, then callsfindCallerRepofor each repo and posts again.canAuthorizeHere(opts)decides whether it may.TemplateApi.requestgains an optionalsignalandTemplateDeployDepsgainsauthorizeandopen, so the flow is injectable in tests.src/commands/github.tsadds theGitHubApitype, aPickofApiClient'srequest.authorizeTerminalandfindCallerRepotake it, sotemplate deploy's injectable client fits. They behave as before.src/commands/template-author.tsaddsTemplateDraftService.source, andserviceTextprints it after the image part. It passes owner, repo, branch androotDirthroughsafeText, so the validator stays as it is and the terminal is protected at render, whatever a draft holds.src/index.tsaddsaddHelpTextontemplate deployandtemplate edit. The kind-switching sentence quotes the platform's two ways out:"delete": truefor a service you added and"removed": truefor one from the project, with the new service under another name.template editalready passes its PATCH body through, so adding a source service needed only the help text.Verify
Run in
cli/on this branch atcc391d0(the review fix on top ofb750732) with Node 22, from insidecli/because the vitest root is..tscexited 0. The repo has no lint or format script, so the gate is typecheck plus test.Test Files 4 passed (4),Tests 295 passed (295).Test Files 102 passed (102),Tests 2137 passed (2137).test/template.test.tsgave 8 failed, and the two new help tests failed on a missingsourceparagraph. The tests that need the new behavior failed first. Two deploy tests (--jsonand no terminal) passed from the start because they pin behavior that was already there. Removing the!canAuthorizecondition and removing theaskedVariablesguard each made its test fail, and restoring them made it pass. Thesource: nullcase is a pin and has no red run.Test Files 2 failed (2)andTests 4 failed | 187 passed (191). The four were the draft with ESC and C1 CSI in its branch androotDir,unreachableReposFromwith control characters in a repo name, the platform message with ESC, OSC and C1 on stderr, andimage: ''besidesourceorbuild. The empty-image-alone test and themissing_variablesthengithub_not_linkedtest passed from the start, because they pin behavior the code already had.What the tests prove:
source: nullequal to['services.web.source must be a map']. An emptyimageorbuildbeside another way gives the mutually exclusive sentence, and an empty one on its own, orimage: ''besidebuild: '', gives only the required sentence.unreachableReposFrom: reads both codes with each repo once and case-insensitive, drops an entry without a string owner and repo, strips control characters from owner and repo before it compares them, and leaves every other error alone, the publish refusal included.--jsonand a run with no terminal stop with the platform's message and make no GitHub call. A second GitHub refusal after linking is the error. A second missing-variables refusal is the error, not another prompt, and a missing-variables refusal is answered once whether it comes before or after the GitHub one. The platform message with ESC, OSC and C1 characters reaches stderr as plain text.rootDircarry ESC and C1 CSI prints none of them.template deployandtemplate edithelp carry the new text.Not verified here.
Known limits.
watchDeploymentkeeps its 15 minute default while a queued build can wait longer. The platform's queue deadline isqueueDeadlineMsinbuilds/config.ts, 4 hours by default. The deployment carries on, and the CLI's timeout message points toinsta agent events.findCallerRepofinds no linked account, not directly from the refusal code. If the platform saysgithub_not_linkedand/me/github/repossays linked, the CLI goes straight to the repo lookup, and the retry then fails with the platform's message.connect-repobehaves the same.code, so it surfaces as before and starts no flow.src/index.tsprints its message as it does for every command. This PR leaves that handler alone.test/github-source.test.tsstill holds the oldimage and build are mutually exclusivetext in a stub error. No assertion depends on it.🤖 Generated with Claude Code