Fix secrets endpoints: use /api/v1 and shared response parser#106
Merged
Fix secrets endpoints: use /api/v1 and shared response parser#106
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
WalkthroughAdds a top-level Secrets command group to the CLI with subcommands to set, list, and delete environment secrets; introduces CLI arg types and command handler; adds GatewayClient secrets REST methods; registers the secrets command module and centralizes GatewayClient construction in main. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant SecretsCmd
participant GatewayClient
participant API
User->>CLI: run "statespace secrets set|list|delete" args
CLI->>SecretsCmd: dispatch run(cmd, gateway)
SecretsCmd->>GatewayClient: resolve environment id
SecretsCmd->>GatewayClient: set/list/delete secret(s)
GatewayClient->>API: HTTP PUT/GET/DELETE /api/v1/environments/{env_id}/secrets...
API-->>GatewayClient: HTTP response
GatewayClient-->>SecretsCmd: parsed result or error
SecretsCmd-->>CLI: print results / errors
CLI-->>User: stdout/stderr
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@binaries/statespace-cli/src/commands/secrets.rs`:
- Around line 1-57: This new CLI commands module is missing unit tests; add
tests that exercise run_set (valid KEY=VALUE and invalid formats), run_list
(empty keys and non-empty keys output), run_delete (successful deletion
message), and error propagation from resolve_env_id; create test functions that
construct a mocked or fake GatewayClient implementing the same async methods
used (get_environment, set_secret, list_secret_keys, delete_secret), call
run_set/run_list/run_delete/resolve_env_id with appropriate
SecretsSetArgs/SecretsListArgs/SecretsDeleteArgs inputs, assert returned Result
variants and captured stdout/stderr strings for the "✓" messages and "No secrets
set." output, and include a test where the mock get_environment returns an error
to verify resolve_env_id propagates it.
- Around line 23-28: The loop over args.secrets currently uses
secret.split_once('=') but doesn't reject empty or whitespace-only keys (e.g.,
"=VALUE" or " KEY =VALUE"); update the parsing in the loop that handles secret
(where you destructure into key and value) to trim whitespace from both parts,
then validate that key.trim() is not empty and return Err(Error::cli(...)) with
a clear message like "Invalid secret format '{secret}': missing key" if it's
empty; keep the rest of the flow (using key and value) unchanged so the API only
receives validated, trimmed keys.
In `@binaries/statespace-cli/src/gateway/client.rs`:
- Around line 366-369: The URL builds the secret endpoint by interpolating raw
key into the path (let url = format!( "{}/api/v1/environments/{}/secrets/{}",
self.base_url, env_id, key )), which breaks for keys with URL-special
characters; fix by URL-encoding the key (use urlencoding::encode or equivalent)
before interpolating—e.g., compute let encoded = urlencoding::encode(&key) and
use encoded in the format call (mirror the approach used in remove_ssh_key) so
routing works for keys containing /, ?, #, etc.
- Around line 387-390: The constructed URL uses the raw secret key and must be
URL-encoded like in set_secret to safely handle special characters; update the
code that builds url (the format! with self.base_url, env_id, key) to
percent-encode key (and any other path components if needed) before
interpolation (use the same encoding helper used by set_secret or a consistent
percent-encoding utility), ensuring the final string uses the encoded key.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
binaries/statespace-cli/src/args.rsbinaries/statespace-cli/src/commands/mod.rsbinaries/statespace-cli/src/commands/secrets.rsbinaries/statespace-cli/src/gateway/client.rsbinaries/statespace-cli/src/main.rs
Comment on lines
+1
to
+57
| use crate::args::{SecretsCommands, SecretsDeleteArgs, SecretsListArgs, SecretsSetArgs}; | ||
| use crate::error::{Error, Result}; | ||
| use crate::gateway::GatewayClient; | ||
| use crate::identifiers::normalize_environment_reference; | ||
|
|
||
| pub(crate) async fn run(cmd: SecretsCommands, gateway: GatewayClient) -> Result<()> { | ||
| match cmd { | ||
| SecretsCommands::Set(args) => run_set(args, gateway).await, | ||
| SecretsCommands::List(args) => run_list(args, gateway).await, | ||
| SecretsCommands::Delete(args) => run_delete(args, gateway).await, | ||
| } | ||
| } | ||
|
|
||
| async fn resolve_env_id(gateway: &GatewayClient, app: &str) -> Result<String> { | ||
| let reference = normalize_environment_reference(app).map_err(Error::cli)?; | ||
| let env = gateway.get_environment(&reference).await?; | ||
| Ok(env.id) | ||
| } | ||
|
|
||
| async fn run_set(args: SecretsSetArgs, gateway: GatewayClient) -> Result<()> { | ||
| let env_id = resolve_env_id(&gateway, &args.app).await?; | ||
|
|
||
| for secret in &args.secrets { | ||
| let Some((key, value)) = secret.split_once('=') else { | ||
| return Err(Error::cli(format!( | ||
| "Invalid secret format '{secret}': expected KEY=VALUE" | ||
| ))); | ||
| }; | ||
| gateway.set_secret(&env_id, key, value).await?; | ||
| eprintln!("✓ Set {key}"); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn run_list(args: SecretsListArgs, gateway: GatewayClient) -> Result<()> { | ||
| let env_id = resolve_env_id(&gateway, &args.app).await?; | ||
| let keys = gateway.list_secret_keys(&env_id).await?; | ||
|
|
||
| if keys.is_empty() { | ||
| println!("No secrets set."); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| for key in &keys { | ||
| println!("{key}"); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn run_delete(args: SecretsDeleteArgs, gateway: GatewayClient) -> Result<()> { | ||
| let env_id = resolve_env_id(&gateway, &args.app).await?; | ||
| gateway.delete_secret(&env_id, &args.key).await?; | ||
| eprintln!("✓ Deleted {}", args.key); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
New command module lacks test coverage.
Per coding guidelines for binaries/statespace-cli/src/commands/**: "Flag PRs that add new commands without corresponding unit tests." This new module introduces set, list, and delete subcommands without accompanying tests.
Consider adding unit tests for:
run_setparsing ofKEY=VALUEformat (valid and invalid inputs)run_listoutput formatting (empty and non-empty cases)- Error propagation from
resolve_env_id
Would you like me to help generate test scaffolding for this module, or open an issue to track adding test coverage?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@binaries/statespace-cli/src/commands/secrets.rs` around lines 1 - 57, This
new CLI commands module is missing unit tests; add tests that exercise run_set
(valid KEY=VALUE and invalid formats), run_list (empty keys and non-empty keys
output), run_delete (successful deletion message), and error propagation from
resolve_env_id; create test functions that construct a mocked or fake
GatewayClient implementing the same async methods used (get_environment,
set_secret, list_secret_keys, delete_secret), call
run_set/run_list/run_delete/resolve_env_id with appropriate
SecretsSetArgs/SecretsListArgs/SecretsDeleteArgs inputs, assert returned Result
variants and captured stdout/stderr strings for the "✓" messages and "No secrets
set." output, and include a test where the mock get_environment returns an error
to verify resolve_env_id propagates it.
Comment on lines
+23
to
+28
| for secret in &args.secrets { | ||
| let Some((key, value)) = secret.split_once('=') else { | ||
| return Err(Error::cli(format!( | ||
| "Invalid secret format '{secret}': expected KEY=VALUE" | ||
| ))); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating non-empty key before API call.
split_once('=') allows edge cases like =VALUE (empty key) or leading/trailing whitespace. Client-side validation would provide faster feedback than waiting for an API error.
💡 Optional enhancement
let Some((key, value)) = secret.split_once('=') else {
return Err(Error::cli(format!(
"Invalid secret format '{secret}': expected KEY=VALUE"
)));
};
+ let key = key.trim();
+ if key.is_empty() {
+ return Err(Error::cli(format!(
+ "Invalid secret format '{secret}': key cannot be empty"
+ )));
+ }
gateway.set_secret(&env_id, key, value).await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@binaries/statespace-cli/src/commands/secrets.rs` around lines 23 - 28, The
loop over args.secrets currently uses secret.split_once('=') but doesn't reject
empty or whitespace-only keys (e.g., "=VALUE" or " KEY =VALUE"); update the
parsing in the loop that handles secret (where you destructure into key and
value) to trim whitespace from both parts, then validate that key.trim() is not
empty and return Err(Error::cli(...)) with a clear message like "Invalid secret
format '{secret}': missing key" if it's empty; keep the rest of the flow (using
key and value) unchanged so the API only receives validated, trimmed keys.
antidmg
force-pushed
the
fix/secrets-api-endpoints
branch
from
166ed53 to
e16d386
Compare
Implements set, list, and delete subcommands for managing per-environment secrets via the gateway API. The CLI resolves environment names/URLs to UUIDs, then calls the dashboard secrets endpoints. Usage: statespace secrets set --app my-dashboard DB=postgresql://... API_KEY=sk_... statespace secrets list --app my-dashboard statespace secrets delete --app my-dashboard DB Amp-Thread-ID: https://ampcode.com/threads/T-019c98d6-a71d-747d-b6ad-bb34b5aa92c6
antidmg
force-pushed
the
fix/secrets-api-endpoints
branch
2 times, most recently
from
de80a8d to
819b119
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
binaries/statespace-cli/src/commands/secrets.rs (2)
23-31:⚠️ Potential issue | 🟠 MajorPre-validate all secrets before any write to avoid partial updates.
At Line 23, validation and API mutation are interleaved. If one later entry is invalid, earlier secrets are already written. This creates partial, non-atomic updates and harder rollback behavior. Also reject empty keys before calling the API.
Proposed fix
async fn run_set(args: SecretsSetArgs, gateway: GatewayClient) -> Result<()> { let env_id = resolve_env_id(&gateway, &args.app).await?; - for secret in &args.secrets { - let Some((key, value)) = secret.split_once('=') else { + let mut parsed = Vec::with_capacity(args.secrets.len()); + for secret in &args.secrets { + let Some((raw_key, raw_value)) = secret.split_once('=') else { return Err(Error::cli(format!( "Invalid secret format '{secret}': expected KEY=VALUE" ))); }; - gateway.set_secret(&env_id, key, value).await?; + let key = raw_key.trim(); + if key.is_empty() { + return Err(Error::cli(format!( + "Invalid secret format '{secret}': missing key" + ))); + } + parsed.push((key.to_string(), raw_value.to_string())); + } + + for (key, value) in parsed { + gateway.set_secret(&env_id, &key, &value).await?; eprintln!("✓ Set {key}"); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/commands/secrets.rs` around lines 23 - 31, Pre-validate and collect all secret key/value pairs before calling the API to avoid partial updates: first iterate over &args.secrets, use secret.split_once('=') to parse each entry, reject any missing '=' or empty key (return Err(Error::cli(...)) on validation failure), and accumulate validated (key, value) pairs; only after all inputs are validated, iterate the collected pairs and call gateway.set_secret(&env_id, key, value).await? and eprintln!("✓ Set {key}") for each to ensure atomic-like behavior and no partial writes.
1-57:⚠️ Potential issue | 🟠 MajorAdd unit tests for the new secrets command handlers.
This file introduces new command logic and error paths, but there are no corresponding tests in the PR for
set/list/deleteflows and invalid input handling.Would you like me to draft a focused unit test scaffold for
run_set,run_list,run_delete, andresolve_env_iderror propagation?As per coding guidelines:
binaries/statespace-cli/src/commands/**: “Flag PRs that add new commands without corresponding unit tests” and “modify command logic without updating tests.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/commands/secrets.rs` around lines 1 - 57, Add unit tests covering the new command handlers: write tests for run_set, run_list, run_delete and for resolve_env_id error propagation; mock GatewayClient methods (get_environment, set_secret, list_secret_keys, delete_secret) to return expected values or errors, assert resolve_env_id propagates gateway errors, assert run_set handles valid KEY=VALUE and returns a CLI error for invalid formats (secret.split_once failure), assert run_list prints "No secrets set." when keys empty and prints keys when present, and assert run_delete calls delete_secret and prints the deleted key; locate the handlers by function names run_set, run_list, run_delete and resolve_env_id and use the SecretsSetArgs/SecretsListArgs/SecretsDeleteArgs structs to construct test inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@binaries/statespace-cli/src/gateway/client.rs`:
- Around line 360-398: Move the three methods set_secret, list_secret_keys,
delete_secret and the local Payload<'a> struct out of client.rs into a new
gateway domain module named secrets (e.g., gateway::secrets) so domain-specific
API calls live in their own file; in the new module implement the same async
methods (same signatures) as impls on the existing Client type (reuse
self.base_url, self.http, with_headers, and call check_api_response /
parse_api_response), bring in serde::Serialize and urlencoding::encode where
needed, remove the originals from client.rs, and add a pub mod secrets;
declaration in the gateway module so callers continue to use Client::set_secret
/ list_secret_keys / delete_secret unchanged.
---
Duplicate comments:
In `@binaries/statespace-cli/src/commands/secrets.rs`:
- Around line 23-31: Pre-validate and collect all secret key/value pairs before
calling the API to avoid partial updates: first iterate over &args.secrets, use
secret.split_once('=') to parse each entry, reject any missing '=' or empty key
(return Err(Error::cli(...)) on validation failure), and accumulate validated
(key, value) pairs; only after all inputs are validated, iterate the collected
pairs and call gateway.set_secret(&env_id, key, value).await? and eprintln!("✓
Set {key}") for each to ensure atomic-like behavior and no partial writes.
- Around line 1-57: Add unit tests covering the new command handlers: write
tests for run_set, run_list, run_delete and for resolve_env_id error
propagation; mock GatewayClient methods (get_environment, set_secret,
list_secret_keys, delete_secret) to return expected values or errors, assert
resolve_env_id propagates gateway errors, assert run_set handles valid KEY=VALUE
and returns a CLI error for invalid formats (secret.split_once failure), assert
run_list prints "No secrets set." when keys empty and prints keys when present,
and assert run_delete calls delete_secret and prints the deleted key; locate the
handlers by function names run_set, run_list, run_delete and resolve_env_id and
use the SecretsSetArgs/SecretsListArgs/SecretsDeleteArgs structs to construct
test inputs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
binaries/statespace-cli/src/args.rsbinaries/statespace-cli/src/commands/mod.rsbinaries/statespace-cli/src/commands/secrets.rsbinaries/statespace-cli/src/gateway/client.rsbinaries/statespace-cli/src/main.rs
Comment on lines
+360
to
+398
| pub(crate) async fn set_secret(&self, env_id: &str, key: &str, value: &str) -> Result<()> { | ||
| #[derive(Serialize)] | ||
| struct Payload<'a> { | ||
| value: &'a str, | ||
| } | ||
|
|
||
| let url = format!( | ||
| "{}/api/v1/environments/{}/secrets/{}", | ||
| self.base_url, | ||
| env_id, | ||
| urlencoding::encode(key) | ||
| ); | ||
| let resp = self | ||
| .with_headers(self.http.put(&url)) | ||
| .json(&Payload { value }) | ||
| .send() | ||
| .await?; | ||
|
|
||
| check_api_response(resp).await | ||
| } | ||
|
|
||
| pub(crate) async fn list_secret_keys(&self, env_id: &str) -> Result<Vec<String>> { | ||
| let url = format!("{}/api/v1/environments/{}/secrets", self.base_url, env_id); | ||
| let resp = self.with_headers(self.http.get(&url)).send().await?; | ||
|
|
||
| parse_api_response(resp).await | ||
| } | ||
|
|
||
| pub(crate) async fn delete_secret(&self, env_id: &str, key: &str) -> Result<()> { | ||
| let url = format!( | ||
| "{}/api/v1/environments/{}/secrets/{}", | ||
| self.base_url, | ||
| env_id, | ||
| urlencoding::encode(key) | ||
| ); | ||
| let resp = self.with_headers(self.http.delete(&url)).send().await?; | ||
|
|
||
| check_api_response(resp).await | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move secrets gateway calls into a dedicated domain file.
Lines 360-398 add a new secrets domain directly in gateway/client.rs. Please move these methods into a dedicated gateway domain module (for example binaries/statespace-cli/src/gateway/secrets.rs) and keep client.rs as shared client primitives/composition.
As per coding guidelines: binaries/statespace-cli/src/gateway/**/*.rs: “Use one file per domain (e.g., environments.rs, auth.rs, tokens.rs) where each file owns the types and API calls for that domain.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@binaries/statespace-cli/src/gateway/client.rs` around lines 360 - 398, Move
the three methods set_secret, list_secret_keys, delete_secret and the local
Payload<'a> struct out of client.rs into a new gateway domain module named
secrets (e.g., gateway::secrets) so domain-specific API calls live in their own
file; in the new module implement the same async methods (same signatures) as
impls on the existing Client type (reuse self.base_url, self.http, with_headers,
and call check_api_response / parse_api_response), bring in serde::Serialize and
urlencoding::encode where needed, remove the originals from client.rs, and add a
pub mod secrets; declaration in the gateway module so callers continue to use
Client::set_secret / list_secret_keys / delete_secret unchanged.
819b119 to
d8e8823
Compare
antidmg
force-pushed
the
fix/secrets-api-endpoints
branch
from
d8e8823 to
3253f4d
Compare
4 tasks
This was referenced Mar 9, 2026
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
1 participant
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.
Switches secrets CLI endpoints from
/dashboard/apps/to/api/v1/environments/and replaces manual response parsing inlist_secret_keyswith the sharedparse_api_responsehelper.Summary by CodeRabbit
New Features
Refactor