[thebrandonlucas]

Stack 5 of 6. Base: feat/cli-next-step-after-accept (#62), now that #64 and #65 are merged into it. Next: feat/cli-login-replaces-same-user (#67).

Closes #47.

Problem

VOLTAGE_API_KEY takes precedence over a saved login. When that key does not belong to the selected organization, every request failed with a 403 whose hint said only to run voltage context. Nothing said that an environment key was used instead of the saved login.

Behavior

A 403 to VOLTAGE_API_KEY now names the variable and where the organization came from, and gives the next step:

error: Voltage API returned HTTP 403: No Access
hint: The API key came from VOLTAGE_API_KEY and the organization from VOLTAGE_ORGANIZATION_ID. Check that the key can reach the organization, environment, and wallet in use, which `voltage context` shows, or pass --account NAME or unset VOLTAGE_API_KEY to use a saved login.

Credential	Status	Hint	error.detail.credential_sources
VOLTAGE_API_KEY, --org	403	names the key and --org	{"credential": "environment", "organization": "flag"}
VOLTAGE_API_KEY, VOLTAGE_ORGANIZATION_ID	403	names the key and the variable	{"credential": "environment", "organization": "environment"}
VOLTAGE_API_KEY	401	voltage auth status, as before	absent
A saved key or login	403	voltage context, as before	absent
VOLTAGE_API_KEY, missing_credentials body	any	voltage auth status, as for a 401	absent

• Authorization::Account carries the sources, decided once when the session resolves its scope (auth::environment_key_sources, from the same ScopeSources) and carried on Resolver. The wallet picker and the wallet-currency read pass them too. Flags and the environment are not read again.
• A 403 whose body is not JSON (for example, from a proxy) still reports the sources. No other HTTP detail is attached to it, so an HTML 404 is not read as a missing resource.
• A missing_credentials rejection is treated as a 401 for both the hint and the sources.
• A key from the environment is used only without --profile or --account, so the organization comes from --org or VOLTAGE_ORGANIZATION_ID.
• CHANGELOG, troubleshooting.md (HTTP 403, including every organization value: environment, flag and unset) and exit-codes.md are updated.

Checks

On head f49cb19:

• cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked (110 unit, 97 integration), and python3 scripts/check-coverage.py pass in nix develop.
• New tests:
  • a_forbidden_environment_key_names_where_it_and_the_organization_came_from: both organization sources, the hint and the JSON sources.
  • an_unauthorized_environment_key_reports_no_sources: a 401 keeps its hint and has no sources.
  • a_forbidden_environment_key_without_a_json_body_still_names_its_sources: a text/html 403 keeps the hint and the JSON sources.
  • an_unrecognized_environment_key_on_a_403_reports_no_sources: a missing_credentials 403 gets the 401 hint and no sources.
  • rejected_credentials_name_the_command_that_checks_them now uses a saved key, and checks that a saved credential reports no sources.
  • Unit: a_forbidden_environment_key_names_its_sources covers each organization source, including none, and ErrorDetail::Http serialization.
• CRAP: 0 of 527 measured functions are at or above 20. The highest is wait_for_payment at 19.60, as on the base.
• I reviewed the diff with voltage-pr-review.
• The commit is G blu@blu.cx, with no trailers.

[thebrandonlucas]

Review at 7c7034c (Aurelius). 1 low finding. Nothing blocking.

The sources match ambient_api_key's precedence (src/auth.rs:543): a key is sent only without --profile or --account, and environment_key_sources uses the same check. A 401 and a 403 to a saved credential report no sources. Unit and integration tests cover the --org and VOLTAGE_ORGANIZATION_ID sources.

Low: src/api.rs:112 and src/api.rs:141. With VOLTAGE_API_KEY, the 403 hint replaces the old voltage context hint, and it names only the organization: "Check that the key belongs to that organization". A 403 can also come from the environment or the wallet (--env, --wallet, VOLTAGE_ENVIRONMENT_ID). In that case the new hint sends the user to the wrong place, and it no longer names the command that shows the whole scope. Fix: keep the key's source, and add "voltage context shows the organization and environment in use". Or say "the organization and environment" in place of "that organization". Update the pinned strings in a_forbidden_environment_key_names_its_sources, the integration test, and troubleshooting.md.

Note: the new environment_key_sources()? calls in pick.rs and wallet_currency.rs can fail only on a bad VOLTAGE_ORGANIZATION_ID. Scope resolution already rejects that earlier, so I don't ask for a test of that path.

Checks on 7c7034c:

• Gate in nix develop: fmt, clippy, machete, tests (110 unit, 95 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, then check_payload and endpoint at 17.00.
• Signed G blu@blu.cx, with no trailers.
• Not run: a staging check with a key from another organization.

[thebrandonlucas]

First review at 7c7034c, against e3e0c9a. This is not an approval. The main path is right:

• environment_key_sources uses the same ambient_api_key test as select, so sources appear exactly when VOLTAGE_API_KEY is the credential sent.
• The organization source comes from the same sourced_scope that commands use.
• Integration tests cover --org, VOLTAGE_ORGANIZATION_ID, a 401 with no sources, and a saved credential that ignores the key.

I found 1 low problem and 2 nits.

1. Low: a 403 with a non-JSON body gets the new hint but no credential_sources.

• src/api.rs:278: when the body is not JSON, the error gets rejection_hint(status) but no ErrorDetail.
• So a 403 from a proxy or gateway (HTML or empty-but-not-JSON) prints "The API key came from VOLTAGE_API_KEY…", while --json has no error.detail.credential_sources.
• exit-codes.md and troubleshooting.md say that --json reports the sources on a 403. This path is not tested.

Fix: attach ErrorDetail::Http { http_status: status, data: Value::Null, credential_sources: self.authorization.credential_sources(status) } there too, and add an integration test with a text/html 403. Or limit the docs to JSON rejections.

2. Nit: the hint and the sources can disagree for missing_credentials (src/api.rs:292-304).

• A missing_credentials rejection uses rejection_hint(401), but credential_sources(status) uses the real status.
• So a 403 of that type gets the 401 hint ("the API does not accept it") together with credential_sources. Your unit test comment says a rejected key should report no sources.
• Fix: compute the effective status once and pass it to both calls.

3. Nit: the hint has a dangling "that organization" when the source is unknown. environment_key_hint (src/api.rs:140) drops the organization clause for Profile, Default or Unset. The next sentence then says "Check that the key belongs to that organization". Use "the selected organization" when there is no clause.

Checks on 7c7034c:

• Gate in nix develop: fmt, clippy, machete, tests (110 unit, 95 integration) and coverage pass, and the tree is clean afterwards.
• CRAP, with the rustup toolchain: 0 of 527 measured functions are at or above 20. The highest is wait_for_payment at 19.60. send is at 13.04.
• Not run: a staging 403 with a key from another organization, and a live non-JSON 403.

[thebrandonlucas]

Re-review at b7a3e92. This is not an approval. My 3 findings are fixed, and I found nothing new.

• Non-JSON 403: the error carries credential_sources only when sources exist, with data: null. Not attaching a general HTTP detail there is right: http_status() == Some(404) at src/api.rs:845 and :1292 would otherwise treat an HTML 404 as a missing resource. The new text/html test covers it.
• missing_credentials: rejected_as now feeds both the hint and the sources. The new test pins the 401 hint and no sources on a 403 of that type.
• "that organization": the new wording names the organization, environment, and wallet in use and points to voltage context, which shows all three (src/context.rs:171).

Checks on b7a3e92:

• Gate in nix develop: fmt, clippy, machete, tests (110 unit, 97 integration) and coverage pass, and the tree is clean afterwards.
• CRAP, with the rustup toolchain: 0 of 527 measured functions are at or above 20. The highest is wait_for_payment at 19.60. send is at 14.01.
• The commit is signed G blu@blu.cx.
• Not run: a staging 403.

[thebrandonlucas]

Re-review at b7a3e92 (Aurelius). My low finding is fixed: the 403 hint now names the organization, environment and wallet, and points to voltage context. Cicero's three fixes are also correct:

• A non-JSON 403 attaches credential_sources only when there are sources. http_status() is compared only with 404 (src/api.rs:845, :1292), so the new 403 detail changes no retry or read path.
• rejected_as drives both the hint and the sources.
• The no-source hint is pinned.

No findings remain.

Checks on b7a3e92: fmt, clippy, machete, tests (110 unit, 97 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.

[tee8z]

Reviewed at b7a3e92. CRAP on this head: 0 of 527 functions at or above 20, highest wait_for_payment at 19.60. The reported sources match the credential that is actually sent in each precedence case, and no secret reaches the new field. No blocking issues.

• resolver.environment_key_sources()? runs again in pick.rs, wallet_currency.rs, and Authorization::new, re-reading the flags and environment each time. Its error path (a malformed VOLTAGE_ORGANIZATION_ID) can't be reached in practice and is untested. Threading the ScopeSources from scope resolution would remove both problems.
• CredentialSources.credential is always Source::Environment, and organization can serialize as "unset", which troubleshooting.md doesn't list.
• The PR description ends with a "Generated with Claude Code" line; AGENTS.md says to leave tool attribution out of PR descriptions.

The inline suggestion flattens the non-JSON error construction. I applied the suggestions on this head and checked them: cargo clippy --all-targets -- -D warnings is clean and the unit and integration tests pass.

[tee8z]

Approved b7a3e92 against fix/scripts-build-then-run. Credential provenance matches selection precedence. The 401, 403, non-JSON, missing_credentials, and saved-credential cases preserve their intended behavior. Remaining expression-shape suggestions are nonblocking. Lower-stack requested changes remain tracked on #62.

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. CRAP: 527 measured functions, none at or above 20; highest wait_for_payment 19.60. 143 test functions were unmeasured.

[thebrandonlucas]

@tee8z Thanks. All three notes are addressed in f49cb19, rebased onto feat/cli-next-step-after-accept after #64 and #65 merged:

• Repeated environment_key_sources()?: Session::open now resolves the scope once with sourced_scope, and computes the sources with auth::environment_key_sources(flags, &sources). Resolver carries the result as credential_sources, and api.rs, pick.rs and wallet_currency.rs read that field. Flags and the environment are read only once, and the error path that couldn't be reached is gone.
• credential / "unset": troubleshooting.md now says that the credential is always environment, and lists each organization value: environment, flag and unset. I kept the credential field so the JSON shape names the credential's source explicitly.
• Attribution line: the description no longer has one. I also dropped the reviewer-validation section, because it described b7a3e92.
• Inline suggestion: applied.

Checks on f49cb19: fmt, clippy -D warnings, machete, tests (110 unit, 97 integration) and coverage pass in nix develop. CRAP: 0 of 527 measured functions are at or above 20, and the highest is wait_for_payment at 19.60. The commit is signed G blu@blu.cx.

[tee8z]

Approved f49cb19 against feat/cli-next-step-after-accept (#62 c1dc498). My three notes on b7a3e92 are fixed:

• Session::open resolves the scope once with sourced_scope and carries credential_sources on Resolver. api.rs, pick.rs and wallet_currency.rs read that field. Settings::scope is now sourced_scope(..).0, so the scope is unchanged, and the error path that could not be reached is gone.
• troubleshooting.md lists each organization value, and says that credential is always environment.
• The non-JSON 403 is now a single Error { hint, detail, .. } expression, and the description has no attribution line.

Nonblocking: when the wallet picker supplies the organization, credential_sources.organization is unset, and the hint names only the key. The behavior was the same on b7a3e92.

Validation on this head, in nix develop: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked (110 unit, 97 integration) and python3 scripts/check-coverage.py passed. CRAP: 527 measured functions, none at or above 20, highest wait_for_payment at 19.60; 143 functions were unmeasured. When I approved, CI on aarch64 Linux was still pending, and the other jobs had passed. The changes requested on #62 still block this stack.

[tee8z]

Re-approved 0a22d4c. It is a rebase onto #62 0b6e476, with the same patch as f49cb19. Only the CHANGELOG context changed. Validation on this tree, in nix develop: the full gate passed (111 unit, 97 integration). CRAP: 529 measured functions, none at or above 20, highest wait_for_payment at 19.60. CI is green.

The base branch was changed.

[tee8z]

My approval was dismissed by the push that merged #67 as a25b69d. The tree of a25b69d matches 762363e, the head of #67, which I verified and approved, so the content is reviewed. It is not ready to approve yet, for three reasons:

• The branch still holds the three feat(cli): name the next step after an accepted receive or quote #62 commits (59334ed, 6ecd37c, 0b6e476), and master has them as the squash 0e034d2. GitHub reports the PR as conflicting, so it needs a rebase onto master.
• CI has not run on a25b69d.
• The net change against master is 1,360 lines (+1245/−115), with feat(cli): replace a saved login when the same user signs in again #67 included. That is over the 1,000-line limit in AGENTS.md. Each PR was under the limit when reviewed, so the limit may not apply when a reviewed stack is merged down; confirm that this is the intended merge path.

After a rebase that changes only the base, with CI green, I will approve.