Repository navigation
service add / deploy: --endpoint http|tcp, and show a tcp service's tcp:// address - #368
Conversation
…/ address Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 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="src/commands/services.ts">
<violation number="1" location="src/commands/services.ts:164">
P2: `--endpoint tcp` without `--port` is accepted and sent with no port, so the platform's default (8080 per the `--port` help) applies. The `--endpoint` help says tcp needs `--port`, so refuse it locally before the network call.</violation>
</file>
<file name="src/commands/deploy.ts">
<violation number="1" location="src/commands/deploy.ts:34">
P2: A successful TCP deploy without an effective `--port` prints `tcp://<host>:?`, so the reported address is incomplete. Resolve the service/response port or require `--port` for TCP deploys.</violation>
<violation number="2" location="src/commands/deploy.ts:200">
P3: The archive-lane `--json` output for a tcp service carries `endpointHost` but no port, so a JSON consumer cannot reconstruct the `tcp://host:port` address the human line prints, while an http deploy's port lives in the URL itself. Include the port (e.g. `port: effOpts.port ? Number(effOpts.port) : undefined`) alongside `endpointHost` so the dial address is recoverable from JSON.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| } | ||
| if (opts.endpoint !== undefined) { | ||
| if (type !== 'compute') throw new Error('--endpoint is only valid for compute services') | ||
| parseEndpoint(opts.endpoint) |
There was a problem hiding this comment.
P2: --endpoint tcp without --port is accepted and sent with no port, so the platform's default (8080 per the --port help) applies. The --endpoint help says tcp needs --port, so refuse it locally before the network call.
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/services.ts, line 164:
<comment>`--endpoint tcp` without `--port` is accepted and sent with no port, so the platform's default (8080 per the `--port` help) applies. The `--endpoint` help says tcp needs `--port`, so refuse it locally before the network call.</comment>
<file context>
@@ -143,6 +159,10 @@ export async function servicesAdd(type: string, name: string, opts: ServicesAddO
}
+ if (opts.endpoint !== undefined) {
+ if (type !== 'compute') throw new Error('--endpoint is only valid for compute services')
+ parseEndpoint(opts.endpoint)
+ }
// Presence, not truthiness: `--no-always-on` is an explicit false and is just as compute-only.
</file context>
| parseEndpoint(opts.endpoint) | |
| if (parseEndpoint(opts.endpoint) === 'tcp' && opts.port === undefined) throw new Error('--endpoint tcp needs --port') |
| // Where the deploy answers: the HTTPS URL, or for an endpoint tcp service its tcp address (the | ||
| // platform reports no URL for one). Pure, so it's unit-tested. | ||
| export function deployedTarget(r: { url?: string; endpointHost?: string | null }, port: string | undefined): string { | ||
| if (r.endpointHost) return `tcp://${r.endpointHost}:${port ?? '?'}` |
There was a problem hiding this comment.
P2: A successful TCP deploy without an effective --port prints tcp://<host>:?, so the reported address is incomplete. Resolve the service/response port or require --port for TCP deploys.
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/deploy.ts, line 34:
<comment>A successful TCP deploy without an effective `--port` prints `tcp://<host>:?`, so the reported address is incomplete. Resolve the service/response port or require `--port` for TCP deploys.</comment>
<file context>
@@ -23,10 +23,18 @@ export function deployRequestBody(source: { image: string }, branch: string, opt
+// Where the deploy answers: the HTTPS URL, or for an endpoint tcp service its tcp address (the
+// platform reports no URL for one). Pure, so it's unit-tested.
+export function deployedTarget(r: { url?: string; endpointHost?: string | null }, port: string | undefined): string {
+ if (r.endpointHost) return `tcp://${r.endpointHost}:${port ?? '?'}`
+ return r.url || '(no URL: a worker)'
+}
</file context>
| const d = source.deployed | ||
| if (opts.json) return printJson({ image: d.image, url: d.url, branch: d.branch, group: d.group, machineId: d.machineId }) | ||
| info(`deployed ${d.image} -> ${d.url} (branch ${d.branch}, group ${d.group})`) | ||
| if (opts.json) return printJson({ image: d.image, url: d.url, ...(d.endpointHost ? { endpointHost: d.endpointHost } : {}), branch: d.branch, group: d.group, machineId: d.machineId }) |
There was a problem hiding this comment.
P3: The archive-lane --json output for a tcp service carries endpointHost but no port, so a JSON consumer cannot reconstruct the tcp://host:port address the human line prints, while an http deploy's port lives in the URL itself. Include the port (e.g. port: effOpts.port ? Number(effOpts.port) : undefined) alongside endpointHost so the dial address is recoverable from JSON.
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/deploy.ts, line 200:
<comment>The archive-lane `--json` output for a tcp service carries `endpointHost` but no port, so a JSON consumer cannot reconstruct the `tcp://host:port` address the human line prints, while an http deploy's port lives in the URL itself. Include the port (e.g. `port: effOpts.port ? Number(effOpts.port) : undefined`) alongside `endpointHost` so the dial address is recoverable from JSON.</comment>
<file context>
@@ -189,16 +197,16 @@ export async function deploy(dir: string | undefined, opts: DeployOpts): Promise
const d = source.deployed
- if (opts.json) return printJson({ image: d.image, url: d.url, branch: d.branch, group: d.group, machineId: d.machineId })
- info(`deployed ${d.image} -> ${d.url} (branch ${d.branch}, group ${d.group})`)
+ if (opts.json) return printJson({ image: d.image, url: d.url, ...(d.endpointHost ? { endpointHost: d.endpointHost } : {}), branch: d.branch, group: d.group, machineId: d.machineId })
+ info(`deployed ${d.image} -> ${deployedTarget(d, effOpts.port)} (branch ${d.branch}, group ${d.group})`)
return
</file context>
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The TCP request fields and address rendering largely match the intended platform contract. One blocking defect remains: source deploys bypass endpoint validation, allowing an invalid flag to trigger a deployment with the endpoint silently omitted.
Requirements context
Reviewed against the supplied PR description, the linked platform PR #658 contract, AGENTS.md, README.md, and the repository's developing-insta-cli skill. The README provides general thin-client guidance; no TCP-specific requirements were found there. The explicit requirement that endpoint typos fail before network calls is not met by source deploys.
Findings
Critical
- src/deploy-archive.ts:122: Validate and normalize endpoint before preparing a source deploy - deploy() passes the raw endpoint into prepareSource() without calling parseEndpoint(). The archive body then silently converts anything except exact 'http' or 'tcp' to undefined. Consequently,
insta deploy . --port 7000 --endpoint tpccan upload, build, and deploy using the existing endpoint instead of failing locally;' TCP ', explicitly accepted by the parser tests, is also silently ignored. Other source lanes perform discovery/build work before deployRequestBody() finally validates the flag. Parse and normalize endpoint at command entry before config/network/build work, pass the normalized value to both lanes, and add tests proving invalid values make no network calls and normalized values reach the archive body.
Suggestion
(none)
Information
- Testing: Focused tests follow repository conventions but miss command orchestration - The pure rendering/body tests and injected archive API follow the repository's testing conventions. Coverage includes omitted flags, service-add refusals, HTTP output compatibility, TCP output, and the empty-URL negative control. Testing deployRequestBody() alone does not exercise validation ordering or the archive bypass.
- Functionality: Response handling matches the companion platform contract - Service rendering uses endpoint_host, while deploy responses use endpointHost. Archive completion accepts an empty URL with a TCP host and retains the missing-host failure. Release remains dependent on platform #658 deploying first; the description also identifies the required command-reference update as a companion PR.
- Security: No additional security findings in the reviewed changes - The new fields travel in existing JSON request bodies and display public service addresses. No dependencies, shell execution, credential output, or authorization changes were introduced; existing approval handling remains in place.
- Performance: No performance regression identified - The change adds constant-time validation and formatting without additional requests or changes to polling behavior. Invalid source inputs can cause unnecessary build work, covered by the blocking validation finding.
Tests
Tests and typecheck could not run: the shell failed at startup because the sandbox could not create a namespace. Read commit 71c05ba through GitHub and statically inspected the changed code, surrounding deploy flow, and tests. Local checkout cross-checking was unavailable. The reported 2137 passing tests were not independently verified.
Verdict
request_changes: One Critical correctness finding requires request_changes under the specified verdict rule.
deploy() handed the raw --endpoint flag to prepareSource() unvalidated; the archive lane's request body silently turned anything other than an exact 'http'/'tcp' into undefined, so a typo like --endpoint tpc built and deployed using the service's existing endpoint instead of failing locally, and accepted-but-unnormalized values like ' TCP ' were lost the same way. Parse and normalize the flag once, before prepareSource's discovery/pack/upload calls, and pass the normalized value to both lanes.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The TCP request bodies, service rendering, and archive completion handling largely match the intended contract. One blocking requirement gap remains: deploy validates --endpoint after project resolution, which can already make network calls and persist a project link. Local execution was unavailable; this review used the supplied diff and read-only GitHub source access.
Requirements context
Reviewed against the PR description, supplied AGENTS.md, repository README, developing-insta-cli guide, and the API contract in InsForge/instacloud-platform#658. The companion command-reference change exists in InsForge/instacloud-skills#171. No separate CLI TCP requirements document was found in the documentation inspected.
Findings
Critical
- src/commands/deploy.ts:182-191: Validate endpoint before resolving the project - deploy() calls requireProject() before this validation. In an unlinked directory, requireProject() calls the platform to resolve projects and can persist an automatic link. Consequently,
insta deploy --image app:2 --endpoint tpccan perform network calls and change local project configuration before rejecting the typo, violating the explicit local-refusal requirement. Move endpoint validation before ApiClient.load()/requireProject(). Extend the regression test to assert that requireProject is never called for an invalid endpoint; the current resolved mock hides this behavior.
Suggestion
(none)
Information
- Software engineering: Focused tests cover the main TCP paths - The added tests cover normalization, compute-only validation, request-body omission, TCP and HTTP rendering, and archive acceptance with a no-host negative control. Helpers follow the surrounding pure-function pattern. The validation-order coverage gap is described in the blocking finding.
- Functionality: Wire fields and archive handling match the companion contract - Endpoint is omitted from serialized requests when unspecified. Service output uses endpoint_host, deploy output uses endpointHost, and archive completion accepts an empty URL only with a TCP host or the existing worker exception. JSON output retains the new host fields. Deployment must follow platform #658 as documented.
- Security: No new injection or authorization issue identified - Endpoint values are restricted to an enum and sent through JSON serialization. The changes retain existing API authentication and approval handling, introduce no dependencies, and add no secret-bearing output.
- Performance: New work is bounded - The change adds small validation and formatting operations without additional API calls, polling loops, blocking I/O, or large allocations.
Tests
No tests or typecheck ran: shell startup failed with 'bwrap: No permissions to create a new namespace'. Inspected GitHub source at head e1b0de3, including surrounding API/project resolution and existing deployment tests. Could not cross-check the local checkout. The reported 2137 passing tests and clean typecheck remain author-provided evidence.
Verdict
request_changes: One Critical finding violates the explicit requirement to reject invalid endpoints before network calls, so the verdict is request_changes.
requireProject() can resolve a project for an unlinked directory and persist an automatic link as a side effect. Validating --endpoint after it meant an invalid value like --endpoint tpc could still touch the network and change local project config before being rejected. Move the check ahead of ApiClient.load()/requireProject(), and assert in the regression test that neither is called for an invalid endpoint.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The implementation matches the TCP endpoint contract, including validation before project resolution, request-body compatibility, and address rendering. No blocking correctness, security, or performance issues were identified. Local verification was unavailable; review used the supplied diff and read-only GitHub source access.
Requirements context
Reviewed against the PR description, supplied AGENTS.md, repository README and developing-insta-cli guide, and the platform #658 API contract. No separate CLI TCP requirements document was found in the documentation inspected. Companion instacloud-skills#171 documents both flags and TCP limitations.
Findings
Critical
(none)
Suggestion
- test/endpoint-tcp.test.ts: Assert command-level JSON output - Add coverage that captures and parses deploy --json output for both image and archive lanes, asserting endpointHost survives and stdout contains exactly one JSON document. Current tests verify helpers and archive request plumbing but do not assert this promised output contract.
Information
- Software engineering: Focused tests cover validation and compatibility - Coverage includes normalization, compute-only refusals, omitted endpoint fields, HTTP rendering controls, archive acceptance and rejection, and explicit assertions that invalid endpoints never reach ApiClient.load or requireProject. Helpers follow the surrounding pure-function pattern.
- Functionality: Wire fields match the platform contract - Service rendering uses endpoint_host; deployment responses use endpointHost. Archive completion accepts an empty URL with a TCP host while retaining the existing worker exception and missing-host failure. The companion platform rejects TCP deploys without a valid explicit port, so the formatter's unknown-port fallback is not a demonstrated failure.
- Security: No new security issue identified - Endpoint inputs are enum-validated and serialized through existing JSON requests. Authentication and approval handling remain intact. The change introduces no dependencies, shell execution, or secret-bearing output.
- Performance: Additional work is small and bounded - The change adds string validation and formatting without additional API requests, polling loops, blocking I/O, or significant allocations.
- Rollout: Platform deployment remains a prerequisite - As documented, release must follow platform #658 because older platforms ignore endpoint. The required command-reference update exists in companion PR instacloud-skills#171.
Tests
No tests or typecheck ran: shell startup failed with 'bwrap: No permissions to create a new namespace'. Inspected surrounding source through GitHub at head 8377360 and reviewed the supplied tests. Could not cross-check the local checkout. The reported 2137 passing tests and clean typecheck remain author-provided evidence.
Verdict
approved: No Critical findings were identified, so the strict verdict rule yields approved.
2 participants
The CLI half of InsForge/instacloud-platform#658, which adds compute endpoint tcp: a compute service reached as raw TCP on its own port, at its own IPv6 address.
Changes
service add/deploy: new--endpoint <http|tcp>.service add, fails before any network call.endpointin the one gated body;urlwhen the platform names the tcp host (endpointHost, added in #658);urlwith no tcp host is still the "no URL" failure it always was, and that case has a control test.service listand theservice addline showtcp://<endpoint_host>:<port>for a tcp service instead of a domain, ortcp/<port> (address after the first deploy)before a deploy has confirmed the host;deployprints-> tcp://host:port;--jsonpassesendpoint/endpoint_host/endpointHostthrough.Tests
test/endpoint-tcp.test.ts:typecheckclean.Order and companions
endpointand creates an http service.cli-reference.md(AGENTS rule 4), in its own PR.service add,deploy), not a new leaf, so rule 6 does not bind. The matchingendpointargument on the MCPdeploy/servicestools will follow in InsForge/instacloud-mcp.🤖 Generated with Claude Code
Summary by cubic
Adds an
--endpoint <http|tcp>option toservice addanddeploy, letting a compute service be reached as raw TCP on its own port at its own IPv6 address instead of an HTTPS domain. TCP services now showtcp://<host>:<port>(ortcp/<port> (address after the first deploy)before the host is confirmed) inservice add,service list, anddeployoutput, and--jsonpasses the endpoint fields through.Details
deploynormalizes the flag beforeApiClient.load()orrequireProject(): either can touch the network, andrequireProject()can persist an automatic link for an unlinked directory, so a typo like--endpoint tpcdies before any of that runs.endpointHost; the no-URL-without-host failure is unchanged.endpointand create an HTTP service.Written for commit 8377360. Summary will update on new commits.