fix(services): accept --port 0, the worker shape deploy already supports - #195
Open
thegoodengineer wants to merge 1 commit into
Open
fix(services): accept --port 0, the worker shape deploy already supports#195thegoodengineer wants to merge 1 commit into
thegoodengineer wants to merge 1 commit into
Conversation
`insta services add compute <name> --port 0` was rejected by client-side validation, but port 0 is a first-class service shape rather than an invalid port: the worker shape, a long-running process with no HTTP endpoint (queue consumers, background agents), which the compute plane gives no PORT env and no ClusterIP. The platform and `insta deploy --port 0` both accept it, so a worker could only be created by picking some other port at creation and letting the first deploy converge it to 0. The one command whose job is "create a service of this shape" was the one that could not express it. parsePort now accepts 0 and names it in the error, so a genuine typo still reads as one rather than being reported as if 0 were merely out of range. Every other rejection is unchanged, including the non-decimal spellings `Number()` would have honoured: `0x1f90` and `0o17620` start with a 0 the widened range must not swallow. The two option checks in servicesAdd/servicesAddRequestBody move from a truthiness test to a presence test. They happen to work today only because the option arrives as the string "0"; a truthy test on the parsed value would drop the worker port and provision a default-port HTTP service instead of the shape that was asked for, which is the failure mode --no-always-on already hit. The presence test also makes `--port 0` on a non-compute type fail locally, as `--port 3000` does, instead of slipping through as "no port given". parsePort is shared with `insta compute connect-repo --port`, so a repo-backed worker is now expressible the same way; help text and the interactive port prompt name the shape. `insta build --port` is deliberately untouched: it has its own validator and an explicit "a verifier must not bless bad input" test, and widening it means deciding what its port check reports for a worker, which is a separate change. Closes InsForge#163 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the PR, @thegoodengineer! This links #163, but that issue isn't assigned to anyone yet. Our workflow is claim the issue first, then submit the PR. It'll still be reviewed — to keep ownership clear, comment on the issue that you'd like it assigned to you. |
Author
|
Understood, and sorry for getting the order wrong. I have commented on #163 asking for it to be assigned to me: #163 (comment) Say the word if you would prefer I close this and reopen it once the issue is assigned. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #163.
The problem
insta services add compute <name> --port 0was rejected by client-side validation:Port 0 is not an out-of-range port here, it is a service shape: the worker shape, a long-running process with no HTTP endpoint (queue consumers, background agents), which the compute plane gives no
PORTenv and no ClusterIP. The platform accepts it and the deploy path already creates it:So a worker could only be created by picking some other port at creation and letting the first deploy converge it to 0. The one command whose job is "create a service of this shape" was the one that could not express it.
The change
parsePortaccepts 0, and names it in the error. The bound moves fromn < 1ton < 0, and the message becomesport must be 0 (worker: no HTTP endpoint) or an integer between 1 and 65535, got: <raw>. Naming 0 rather than silently widening the range keeps the message useful for a genuine typo, and teaches the shape at the point of error. Every other rejection is unchanged, including the non-decimal spellingsNumber()would have honoured:0x1f90and0o17620start with a 0 that the widened range must not swallow, and the decimal-digits-only regex still refuses them.The two
--portchecks inservicesAdd/servicesAddRequestBodymove from a truthiness test to a presence test. They happen to work today only because the option arrives as the string"0". A truthy test on a parsed value would drop the worker port and provision a default-port HTTP service instead of the shape that was asked for, which is exactly the failure mode--no-always-onalready hit (an explicit value read as "not given"), and the mapping is the place that bug would land. The presence test also fixes a smaller thing in passing:--port 0on a non-compute type now fails locally the way--port 3000does, instead of slipping past the guard as "no port given".Docs.
services add --porthelp now names the worker shape, and so does the interactive port prompt, which is the only place the shape is discoverable without reading the reference.compute connect-repo --portcomes along, because it shares the parser. A repo-backed worker is the same shape as an image-backed one, so--port 0there now overrides the detected HTTP port rather than being refused. Its help text says so, and there is a test.Deliberately not in scope
insta build --portkeeps its own validator, which still rejects 0. Widening it means deciding what the report's port check says for a worker (today a missing port is afailwith "a port mismatch is the #1 deploy mistake"), andbuild.test.tsasserts the current rejection under an explicit rationale: a verifier must not bless bad input. That is a separate change with its own report semantics, not a side effect of this one. Happy to follow up if you want it.The issue also asks whether
--port 0at create should implyendpoint.mode=none. I left that alone: the platform already convergesport: 0tononeon deploy, and I cannot verify a create-time field against the control plane from here. Sending an unverified extra key seemed worse than sending the port the platform already understands.Testing
npm run typecheck && npm test: green (813 passing, 6 skipped), except for one pre-existing unrelated failure on my machine,github-source.test.ts > refuses a manifest that resolves outside the clone through a symlink, which fails identically on unmodifiedmainhere because creating a symlink on Windows needs Developer Mode or elevation (EPERM). CI'swindows-latestrunner is privileged, so it passes there.Six new tests, all negative-controlled (each fails against the current
parsePort, verified by stashing the source change and re-running):parsePortaccepts0and" 0 "parsePortnames 0 and the rejected value in the errorservicesAddRequestBodysendsport: 0rather than dropping it as falsyservicesAddrejects--port 0on a non-compute type, before any config or network accesssourceBody(connect-repo) honours--port 0over the detected portresolveServiceArgslets--port 0through the pre-flight check and does not ask for the port againManually, against a dead API URL so the local validators are what answer:
Follow-up outside this repo
Per CONTRIBUTING,
insta/cli-reference.mdinInsForge/insta-skillsneeds the same edit, since that file is how agents learn the CLI surface. I do not have a change set there; the wording that matches this PR is:and, for
compute connect-repo,0for a worker with no HTTP endpoint.Summary by cubic
Allows
--port 0on service creation to express the worker shape (no HTTP endpoint), which the platform andinsta deployalready supported. Previously,insta services add compute <name> --port 0was rejected by validation, forcing workers to be created at another port and converged later.parsePortnow accepts 0 and names it in the error message.--portchecks inservicesAddandservicesAddRequestBodynow use presence instead of truthiness, so0is not dropped.compute connect-repo --port 0also works because it shares the parser.insta build --portis untouched: it has its own validator and a separate rationale.Written for commit 2a16b05. Summary will update on new commits.