fix(service): validate --upload before spending anything - #482
Merged
Conversation
Found while validating #409 against real EC2 — I made the mistake myself. `--upload` takes a value, so `--upload --region us-east-1` binds "--region" as the filename and leaves "us-east-1" to be parsed as a positional argument. The service is handed a stray argv element, the region goes silently unset, and the dry run prints "Instance: t4g.micro in -" with no cost line at all. Three changes, in order of what they cost: - The path is validated during argument parsing, before the AWS client exists. `uploadToInstance` already stat'd the file, but by then an instance is running, so a typo cost a launch. Now it costs $0. - A value starting with "-" is named as a flag rather than reported as "no such file or directory: --region", which is technically true and buries the lede. A directory is also rejected. - A dry run with no resolved region says why it can't quote a cost bound instead of omitting the line. Silence there reads as "this is free", and an unset region is the symptom of this exact mistake, so naming it points at the cause. Also commits docs-gen/service.md, which #409 generated but never committed. The drift gate diffs tracked files only, so an untracked new fragment slips through CI and would first fail at tag time, where `make check-docs` runs as a release guard. The new test neutralises AWS credentials rather than assuming none are present: its claim is "this never reaches AWS", and a developer's shell usually has AWS_PROFILE set. Verified by mutation — with the guard removed it now fails in 0.00s having contacted nothing, where before it spent 30s reaching a real account.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Found while validating #409 against real EC2 — I made this mistake myself, and the dry run rendered a plan that couldn't work.
The bug
--uploadtakes a value, so this:binds
--regionas the filename and leavesus-east-1to be parsed as a positional argument. The service gets a stray argv element, the region silently goes unset, and the dry run happily prints:No cost line, because there's no region to price — but nothing says so.
Three changes, ordered by what they cost
Validation moves before the launch.
uploadToInstancealready stat'd the file, but only after an instance was running — so a typo cost a launch. It's now checked during argument parsing, before the AWS client is even built. This is the substance of the fix; the message improvements below are the cosmetics.A flag-shaped value is named as such:
rather than
--upload --region: stat --region: no such file or directory, which is technically true and buries the lede. Directories are rejected too.The dry run explains a missing cost bound instead of dropping the line. Silence reads as "this is free", and an unset region is the symptom of this exact mistake, so naming it points at the cause:
Validation runs before the
--dry-runbranch, not just before the launch: a preview that renders an unusable plan is worse than no preview, since catching this for free is the whole point of--dry-run.Also: a missed reference fragment
This commits
docs-gen/service.md, which #409 generated but never committed. The drift gate isgit diff --exit-code docs-gen/, which sees tracked files only — so a brand-new untracked fragment passes CI and would first fail at tag time, wheremake check-docsruns as a release guard. Worth knowing the gate has that blind spot.Verification
All three guards mutation-tested:
if strings.HasPrefix(path, "-")→if falseTestValidateServiceUploadfailsRunETestServiceRejectsABadUploadBeforeSpendingAnythingfailsTestRenderServiceDryRunSaysWhyTheCostBoundIsMissingfailsThe second mutation exposed a flaw in my own test worth calling out: it initially reached a real AWS account (30s, a live
DescribeInstances) because this shell hasAWS_PROFILEset. It never launched anything — the instance ID was nonexistent, and I leak-checked three regions to confirm zero instances — but a unit test whose claim is "this never reaches AWS" must not depend on the ambient environment being empty. It now neutralises the credential env vars and points the config/credential files at an empty temp dir, so the same mutation fails in 0.00s having contacted nothing.Also added
spawnRegiontoresetServiceFlags, since it's the root persistent flag shared with every other command and these tests now set it.make check-docspasses. Fullcmdsuite green exceptTestCatalogValid, which fails only on my machine —~/.spawn/catalog.yamlis a personal overlay pointing chimerax/paraview at private ECR, exactly as #392 prescribes, and the validator checks the merged view. CI has no overlay.