Repository navigation
github-backup(deploy): reject unrecognized arguments before any side effect (#3287) - #4475
Conversation
…effect (#3287) Strictly account for every deploy.mjs argument before any bws, wrangler, git, subprocess, network, file, lock, or deployment side effect. Only bare, --dry-run, and --deploy-version <id> are accepted; unknown flags, stray positionals, duplicates, trailing args, and missing or flag-like --deploy-version values abort with a usage error naming the offender (exit 2). The --dry-run + --deploy-version combination keeps its #3474 rejection, now also as a usage error (exit 2). Regression coverage: infra/github-backup/tests/test_deploy_unknown_args.py drives the real script with stubbed CLIs.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 34 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bc1fab6fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (arg === "--deploy-version") { | ||
| if (deployVersion !== null) usageError("duplicate argument: --deploy-version"); | ||
| const value = args[i + 1]; | ||
| if (value === undefined) usageError("--deploy-version needs a version id"); |
There was a problem hiding this comment.
When invoked as --deploy-version "$VERSION" with an empty variable, this check accepts the empty string as a version ID. The script then retrieves Bitwarden secrets and invokes wrangler versions view with an empty ID instead of reporting a usage error before external calls. The previous !deployVersion check rejected this input before those operations. Reject empty strings as well as undefined and cover the quoted-empty-value case in the regression tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4d05166 (queue task 5716, lane code-l): --deploy-version "" is now rejected with the same usage error (exit 2) before any bws/wrangler subprocess — the value check is value === undefined || value === "". Regression coverage added in test_deploy_unknown_args.py §3 (empty value: exit 2, error names --deploy-version, no stub call at all). Suites on the laptop at the new head: unknown-args 53/53, dry-run 11/11, rollback 16/16.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Fixes #3287 (phase 1 of #3352).
infra/github-backup/worker/deploy.mjsonly membership-checked--dry-run/--deploy-versionand silently ran the real deploy sequence (versions upload, secret put x2) for any typo'd or unknown argument with exit 0. This change accounts for every argument before any bws/wrangler/git subprocess or deploy side effect: only bare,--dry-run, and--deploy-version <id>are accepted; everything else (unknown flags, positionals, duplicates, trailing args, missing or flag-like--deploy-versionvalues) aborts with a usage error naming the offender (exit 2). The--dry-run+--deploy-versioncombination keeps its #3474 rejection, now also as a usage error (exit 2).Tests (stubbed CLIs only, no real deploy or secret operations):
infra/github-backup/tests/test_deploy_unknown_args.py: 50/50test_deploy_dry_run.py: 11/11test_deploy_rollback_version.py: 16/16Queue task 5716.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.