Skip to content

fix(scripts): build, then run the command after a leading --build - #65

Merged
thebrandonlucas merged 2 commits into
feat/cli-next-step-after-acceptfrom
fix/scripts-build-then-run
Oct 21, 2026
Merged

thebrandonlucas merged 2 commits into
feat/cli-next-step-after-acceptfrom
fix/scripts-build-then-run

Conversation

@thebrandonlucas

@thebrandonlucas thebrandonlucas commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Stack 4 of 6. Base: feat/cli-picker-currency (#64). Next: feat/cli-403-credential-source.

Closes #37.

Problem

scripts/voltage-staging and scripts/voltage-local accepted --build only as the sole argument. scripts/voltage-staging --build login --account me passed --build to the binary, and clap rejected it with a usage line that showed the wrapper's injected flags. docs/development.md shows voltage --build in an alias block, so it looks like a flag you can combine with a command.

Behavior

Arguments Result
--build Builds, then exits with cargo's status (as before).
--build CMD ... Builds, then runs CMD ... through the wrapper.
--build with --config-dir, --api-url or --auth-url Refused before building, exit 1.
voltage-staging --build CMD ... with a missing or invalid .env Refused before building, exit 1. --build alone needs no .env.
--build CMD ... and the build fails Exits with cargo's status. The command does not run.
--build anywhere but first Passed to the binary, as before.
  • The build still runs without VOLTAGE_* variables, so it does not see staging credentials.
  • docs/development.md describes the leading --build in the scripts list and in the staging walkthrough.
  • No CHANGELOG line, because the wrappers are development tools, not part of voltage.

Checks

On head e3e0c9a:

  • cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked (109 unit, 93 integration), and python3 scripts/check-coverage.py pass in nix develop. These ran on 8dd04b0, the previous head of this PR. e3e0c9a changes only scripts/voltage-staging and docs/development.md from it, and this PR changes no Rust.
  • The repository has no tests for the wrapper scripts, so I ran each path by hand in nix develop:
    • voltage-local --build builds, exit 0.
    • voltage-local --build --version prints the version.
    • voltage-local --build --config-dir x is refused, exit 1.
    • RUSTFLAGS=--bogus voltage-local --build --version exits 101 and prints no version.
    • voltage-staging --build --version with a test .env prints the version. Without .env, it reports the missing .env and does not build. With a .env of mode 644, it reports the mode and does not build.
    • voltage-staging --build with no .env builds, exit 0. The build sees no VOLTAGE_* variables, checked with a cargo stub.
    • voltage-staging --build --api-url=x is refused before building.
    • RUSTFLAGS=--bogus voltage-staging --build --version exits 101. A cargo stub that exits 3 makes it exit 3 and run nothing.
  • CRAP: not run, because no src/ or tests/ changed. The base scores 0 functions at or above 20, highest wait_for_payment at 19.60.
  • I reviewed the diff with voltage-pr-review.

Current-head reviewer validation

Reviewed e3e0c9a against feat/cli-picker-currency. The head is unchanged from the completed local review.

  • Verification gate passed on this head: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked, and python3 scripts/check-coverage.py. These reviewer checks used Rust 1.98.1 on Linux.
  • CRAP is not required: this layer changes no src/ or tests/ files. Wrapper success, rejection, missing/private .env, build failure, and environment-isolation checks passed.

The wallet picker listed only a name and an ID, so a USD wallet looked
like a BTC wallet. Each wallet now shows its currency between the two
when all of its balances report the same one, and typing the currency
filters on it. A wallet with no balance currency, or with balances
that disagree, shows only its name and ID.

Closes #50
@thebrandonlucas
thebrandonlucas force-pushed the feat/cli-picker-currency branch from d8fdb1a to 386c61c Compare October 20, 2026 18:24
@thebrandonlucas
thebrandonlucas force-pushed the fix/scripts-build-then-run branch from a4e0621 to 8dd04b0 Compare October 20, 2026 18:24

@thebrandonlucas thebrandonlucas left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 8dd04b0 (Aurelius). No findings.

I ran both wrappers on this head:

  1. voltage-local --build --config-dir x refuses before building, with exit 1.
  2. A stub cargo that exits 3: voltage-local --build --version and voltage-staging --build --version both exit 3 without running the binary.
  3. voltage-local --build --version builds, then prints voltage 0.1.0 (8dd04b0).
  4. voltage-local --build alone builds, then exits 0.
  5. voltage-staging --build --version with no .env builds, then stops at "Missing .env".

The build environment is now a copy, so the cargo PATH and RUSTUP_HOME no longer leak into the binary's environment in voltage-local. That is an improvement.

Note: the repository has no test harness for scripts/, so these paths have no automated test. I don't count that as a finding for this PR.

Checks on 8dd04b0: fmt, clippy, machete, tests (109 unit, 93 integration) and coverage pass, and the tree is clean. CRAP: 0 functions at or above 20. The highest is wait_for_payment at 19.60. Signed G blu@blu.cx, with no trailers.

@thebrandonlucas thebrandonlucas left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review at 8dd04b0, against 386c61c. This is not an approval. The change does what #37 asks: a leading --build builds, then runs the rest of the arguments. The refused flags are still checked before the build.

1. Low: voltage-staging --build CMD builds before it checks .env (scripts/voltage-staging:37-46). With a missing or bad .env, the person waits for a full cargo build and then gets "Missing .env". I reproduced this: with no .env, --build --version builds, then exits 1. Fix: when args is not empty after --build, check .env first. --build alone should still not read .env, and its build environment should still have no VOLTAGE_* variables. One way is to move the .env checks into a function, and call it before the build when args is not empty.

2. Nit: the PR description's Checks are for head a4e0621, not 8dd04b0. Update the head hash.

Untested paths: the repository has no tests for the wrapper scripts, so the new paths (build failure, --build alone, --build with a command) are covered only by the manual runs in the description. That is acceptable for development scripts. If you add tests later, a test that runs --build --config-dir x and expects the refusal would be cheap.

Checks on 8dd04b0:

  • Gate in nix develop: fmt, clippy, machete, tests (109 unit, 93 integration) and coverage pass, and the tree is clean afterwards.
  • By hand:
    • voltage-local --build --version prints the version, exit 0.
    • voltage-local --build --config-dir x is refused, exit 1.
    • voltage-staging --build --api-url=x is refused before any build, exit 1.
    • voltage-staging --build --version with no .env builds, then exits 1 with "Missing .env".
  • CRAP: not run, because there is no change to src/ or tests/. The base scores 0 functions at or above 20.

@thebrandonlucas thebrandonlucas left a comment •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate of #65 (review) (Cicero). A GitHub error made me post it twice.

`scripts/voltage-staging --build login ...` passed `--build` to the
binary, which rejected it. A leading `--build` in either wrapper now
builds and then runs the remaining arguments, if any. The wrapper still
refuses `--config-dir`, `--api-url` and `--auth-url` before it builds,
and a failed build exits with cargo's status without running the command.

Closes #37.
@thebrandonlucas
thebrandonlucas force-pushed the fix/scripts-build-then-run branch from 8dd04b0 to e3e0c9a Compare October 20, 2026 18:29
@thebrandonlucas

Copy link
Copy Markdown
Contributor Author

Fixed Cicero's findings in e3e0c9a: voltage-staging --build CMD now checks .env before it builds, and --build alone still needs no .env. The build environment still has no VOLTAGE_* variables. The description's Checks now name the current head.

@thebrandonlucas thebrandonlucas left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at e3e0c9a. This is not an approval. Both of my findings are fixed, and I found nothing new.

  • With a command after --build, .env is now checked before cargo_build() runs.
  • --build alone still exits before it reads .env.
  • The build environment is still built from os.environ with VOLTAGE_* removed, and the .env values are never merged into it.
  • The PR description now names e3e0c9a.

Checks, run by hand with a cargo stub and VOLTAGE_API_KEY exported:

  • --build --version with no .env: "Missing .env", exit 1, and no build.
  • --build alone: builds, and the build sees 0 VOLTAGE_* variables. Exit 0.
  • --build alone with a build that exits 3: the wrapper exits 3.
  • The commit is signed G blu@blu.cx. I did not run the gate or CRAP, because only scripts/ and docs/ changed since 8dd04b0, where both passed.

@tee8z tee8z left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at e3e0c9a. I ran each row of the description's behavior table against both wrappers with a stubbed cargo, and every row holds. No src/ or tests/ changes, so no CRAP run is needed. No blocking issues.

  • docs/development.md now documents voltage --build login …, and step 4 then suggests folding --profile staging into the alias. After that, --build is no longer first: it is forwarded to the binary and clap rejects it, the same symptom as #37. Either strip --build wherever it appears, or add a sentence saying it must come first.
  • Nit: scripts/voltage-staging has a cargo_build() helper while scripts/voltage-local inlines the same logic. One shape would keep them from drifting.
  • The PR description ends with a "Generated with Claude Code" line; AGENTS.md says to leave tool attribution out of PR descriptions.

@tee8z tee8z left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved e3e0c9a against feat/cli-picker-currency. The leading --build behavior, failed-build exit status, pre-build rejection paths, and credential isolation passed wrapper checks. The profile alias limitation is a nonblocking documentation follow-up: --build must be the first wrapper argument.

Validation on this unchanged head: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked, and python3 scripts/check-coverage.py passed. Wrapper behavior checks also passed. CRAP is not required because this layer changes no Rust source or tests.

Base automatically changed from feat/cli-picker-currency to feat/cli-next-step-after-accept October 21, 2026 13:42
@thebrandonlucas
thebrandonlucas merged commit c1dc498 into feat/cli-next-step-after-accept Oct 21, 2026
7 checks passed
@thebrandonlucas
thebrandonlucas deleted the fix/scripts-build-then-run branch October 21, 2026 13:42
thebrandonlucas added a commit that referenced this pull request Oct 21, 2026
* feat(cli): show each wallet's currency in the picker

The wallet picker listed only a name and an ID, so a USD wallet looked
like a BTC wallet. Each wallet now shows its currency between the two
when all of its balances report the same one, and typing the currency
filters on it. A wallet with no balance currency, or with balances
that disagree, shows only its name and ID.

Closes #50

* fix(scripts): build, then run the command after a leading --build

`scripts/voltage-staging --build login ...` passed `--build` to the
binary, which rejected it. A leading `--build` in either wrapper now
builds and then runs the remaining arguments, if any. The wrapper still
refuses `--config-dir`, `--api-url` and `--auth-url` before it builds,
and a failed build exits with cargo's status without running the command.

Closes #37.
thebrandonlucas added a commit that referenced this pull request Oct 21, 2026
* feat(cli): name the next step after an accepted receive or quote

An accepted payments receive or quotes create returns no body, so
human output now names the command that shows the invoice, address,
or quote, with the --org, --env, and --account that the command used.
--quiet and machine-readable output leave the hint out, and so does a
request body without an ID.

A command that takes a resource ID also takes it as --id, as
quotes create and payments send do, and rejects both at once.

Closes #52
Closes #36

* feat(cli): show each wallet's currency in the picker (#64)

The wallet picker listed only a name and an ID, so a USD wallet looked
like a BTC wallet. Each wallet now shows its currency between the two
when all of its balances report the same one, and typing the currency
filters on it. A wallet with no balance currency, or with balances
that disagree, shows only its name and ID.

Closes #50

* fix(scripts): build, then run the command after a leading --build (#65)

* feat(cli): show each wallet's currency in the picker

The wallet picker listed only a name and an ID, so a USD wallet looked
like a BTC wallet. Each wallet now shows its currency between the two
when all of its balances report the same one, and typing the currency
filters on it. A wallet with no balance currency, or with balances
that disagree, shows only its name and ID.

Closes #50

* fix(scripts): build, then run the command after a leading --build

`scripts/voltage-staging --build login ...` passed `--build` to the
binary, which rejected it. A leading `--build` in either wrapper now
builds and then runs the remaining arguments, if any. The wrapper still
refuses `--config-dir`, `--api-url` and `--auth-url` before it builds,
and a failed build exits with cargo's status without running the command.

Closes #37.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants