Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
WalkthroughTerminology and types were migrated from "environment(s)" to "application(s)" across CLI and gateway layers; added application data models and an Upserter abstraction for sync with checksum-based change detection and early-exit. No public API removals; run_sync now depends on an Upserter trait. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
binaries/statespace-cli/src/gateway/applications.rs (1)
33-34:⚠️ Potential issue | 🟡 Minor
#[allow(dead_code)]onauth_tokenrequires an explanatory comment.The annotation exists on an individual item (which is allowed), but there is no inline comment explaining why
auth_tokenis kept despite being unused. Per the coding guidelines, the rationale must be stated.♻️ Proposed fix
- #[allow(dead_code)] + // Retained for forward-compatibility: the API returns auth_token on reads + // but the CLI does not currently consume it for Application listings. + #[allow(dead_code)] pub auth_token: Option<String>,As per coding guidelines: "No file-level
#![allow(dead_code)]suppression - delete dead code or annotate individual items with#[allow(dead_code)]and explain why."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/gateway/applications.rs` around lines 33 - 34, The field auth_token currently has #[allow(dead_code)] with no explanation; add a concise inline comment above the auth_token field explaining why the field is intentionally kept despite being unused (for example: reserved for future auth feature, maintained for API compatibility, or used conditionally in other build targets), so the suppression follows the guideline requiring rationale; keep the attribute on the auth_token field and include the explanatory sentence referencing the auth_token symbol.
🤖 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/app.rs`:
- Around line 83-91: Replace the string-based status handling with a typed enum:
add an ApplicationStatus enum in gateway/applications.rs (variants Running,
Pending, Creating, and an Other fallback annotated with #[serde(other)]), change
the application model's status field to ApplicationStatus, update
deserialization accordingly, and then in
binaries/statespace-cli/src/commands/app.rs match on app.status
(ApplicationStatus::Running, ::Pending, ::Creating, ::Other) to produce the same
formatted symbols and messages; ensure any uses of app.status.as_str() are
updated to use the enum and implement Display or pattern matches where needed.
In `@binaries/statespace-cli/src/commands/sync.rs`:
- Around line 57-60: run_sync now calls gateway.upsert_application(&name, files)
and prints either "Created" or "Updated" based on result.created, but no tests
were added; add unit tests that mock the gateway to assert upsert_application is
invoked with the expected name and files and verify the printed output uses the
correct action and result.name. Specifically, add async tests for run_sync (or
the command handler function) that: 1) mock the gateway.upsert_application to
return a Result with created = true and name = "foo" and assert stderr contains
"Created application 'foo'"; 2) mock it to return created = false and name =
"foo" and assert stderr contains "Updated application 'foo'"; use the existing
Gateway trait/test double pattern in the repo (or a mocking crate) and the async
test harness (tokio::test) to run the command and capture stderr output. Ensure
tests target the run_sync invocation path and fail if upsert_application isn't
called with the provided name and files.
In `@binaries/statespace-cli/src/gateway/applications.rs`:
- Around line 27-30: Replace the raw String status on Application with a typed
enum: add a pub(crate) enum ApplicationStatus (derive Debug, Clone, Deserialize,
PartialEq, Eq) annotated #[serde(rename_all = "snake_case")] with variants
Active, Pending, Stopped and an #[serde(other)] Unknown; change
Application::status from String to ApplicationStatus and update any
creation/deserialization sites and comparisons to use ApplicationStatus; also
ensure serde::Deserialize is imported where needed.
In `@binaries/statespace-cli/src/gateway/client.rs`:
- Line 237: The scope string format!("environments:{scope}") is a legacy
wire-format required by the server and can be overlooked during future API
renames; update the two occurrences of format!("environments:{scope}") (used
where the request scope is built) to add a short inline comment/TODO like "//
TODO: rename to \"applications:{scope}\" when server API is updated" right
beside each occurrence so future maintainers know this is intentional and must
be changed when the server changes.
---
Outside diff comments:
In `@binaries/statespace-cli/src/gateway/applications.rs`:
- Around line 33-34: The field auth_token currently has #[allow(dead_code)] with
no explanation; add a concise inline comment above the auth_token field
explaining why the field is intentionally kept despite being unused (for
example: reserved for future auth feature, maintained for API compatibility, or
used conditionally in other build targets), so the suppression follows the
guideline requiring rationale; keep the attribute on the auth_token field and
include the explanatory sentence referencing the auth_token symbol.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
binaries/statespace-cli/src/args.rsbinaries/statespace-cli/src/commands/app.rsbinaries/statespace-cli/src/commands/ssh.rsbinaries/statespace-cli/src/commands/ssh_config.rsbinaries/statespace-cli/src/commands/sync.rsbinaries/statespace-cli/src/commands/tokens.rsbinaries/statespace-cli/src/gateway/applications.rsbinaries/statespace-cli/src/gateway/client.rsbinaries/statespace-cli/src/gateway/mod.rsbinaries/statespace-cli/src/gateway/tokens.rsbinaries/statespace-cli/src/identifiers.rsbinaries/statespace-cli/src/names.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
binaries/statespace-cli/src/commands/sync.rs (1)
45-68: 🛠️ Refactor suggestion | 🟠 MajorExtract checksum comparison into a pure helper to keep
run_syncthin.
This block is non-trivial for a command handler; a helper keeps orchestration concise.♻️ Suggested extraction
+fn has_changes(prev: &SyncState, name: &str, checksums: &[(String, String)]) -> bool { + if prev.name != name { + return true; + } + let prev_map: std::collections::HashMap<&str, &str> = prev + .checksums + .iter() + .map(|(k, v)| (k.as_str(), v.as_str())) + .collect(); + checksums.len() != prev.checksums.len() + || checksums + .iter() + .any(|(p, c)| prev_map.get(p.as_str()) != Some(&c.as_str())) +} @@ - if let Some(ref prev) = cached { - let same_target = prev.name == name; - if same_target { - let prev_map: std::collections::HashMap<&str, &str> = prev - .checksums - .iter() - .map(|(k, v)| (k.as_str(), v.as_str())) - .collect(); - let changed = checksums.len() != prev.checksums.len() - || checksums - .iter() - .any(|(p, c)| prev_map.get(p.as_str()) != Some(&c.as_str())); - - if !changed { - eprintln!("No changes detected, skipping sync."); - return Ok(()); - } - } - } + if let Some(ref prev) = cached { + if !has_changes(prev, &name, &checksums) { + eprintln!("No changes detected, skipping sync."); + return Ok(()); + } + }As per coding guidelines: "Command handlers should be thin orchestration - parse args, call gateway/services, and format output; extract complex validation and parsing to pure functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/commands/sync.rs` around lines 45 - 68, Extract the checksum comparison logic into a pure helper function (e.g., fn has_checksum_changes(name: &str, checksums: &[(String,String)], cached: &Option<CachedType>) -> bool) so run_sync remains thin: move creation of prev_map, the same_target check, length comparison and the any(...) lookup into that helper, return true if changed; in run_sync replace the inline block that builds checksums and checks cached/prev with a call to the helper and keep the eprintln!("No changes detected, skipping sync.")/return Ok(()) behavior when it returns false. Ensure the helper only depends on inputs (name, checksums, cached) and references the same symbols (checksums, cached, prev.name, prev.checksums) so callers are unchanged.
🤖 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/app.rs`:
- Around line 4-6: The app command handlers (run_create, run_list, run_get,
run_delete) lack unit tests; add a test module mirroring the pattern in sync.rs
that exercises path validation, API calls, output formatting, and user
confirmation by mocking the gateway/application client and any filesystem or
stdin interactions; create tests for success and error paths for each function,
assert correct calls to normalize_application_reference/generate_name where
applicable, and verify formatted output and exit behavior using the same mock
helpers and test harness used by other CLI tests.
- Around line 84-90: The match is trying to move app.status out of a borrowed
reference (for app in &apps). Change the match to use a reference: match
&app.status { ... } and update each arm to match &ApplicationStatus::Running,
&ApplicationStatus::Pending | &ApplicationStatus::Creating,
&ApplicationStatus::Unknown, etc., so you don't attempt to move the enum out of
&app while keeping the format! calls intact (symbols: apps, app,
ApplicationStatus, status).
---
Outside diff comments:
In `@binaries/statespace-cli/src/commands/sync.rs`:
- Around line 45-68: Extract the checksum comparison logic into a pure helper
function (e.g., fn has_checksum_changes(name: &str, checksums:
&[(String,String)], cached: &Option<CachedType>) -> bool) so run_sync remains
thin: move creation of prev_map, the same_target check, length comparison and
the any(...) lookup into that helper, return true if changed; in run_sync
replace the inline block that builds checksums and checks cached/prev with a
call to the helper and keep the eprintln!("No changes detected, skipping
sync.")/return Ok(()) behavior when it returns false. Ensure the helper only
depends on inputs (name, checksums, cached) and references the same symbols
(checksums, cached, prev.name, prev.checksums) so callers are unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
binaries/statespace-cli/src/commands/app.rsbinaries/statespace-cli/src/commands/sync.rsbinaries/statespace-cli/src/gateway/applications.rsbinaries/statespace-cli/src/gateway/client.rsbinaries/statespace-cli/src/gateway/mod.rs
Comment on lines
+4
to
6
| use crate::gateway::applications::ApplicationStatus; | ||
| use crate::identifiers::normalize_application_reference; | ||
| use crate::names::generate_name; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the file structure for tests related to CLI commands
find . -path '*/binaries/statespace-cli*' -name '*test*' -o -path '*/binaries/statespace-cli*' -name 'tests' -type d | head -20Repository: statespace-tech/statespace
Length of output: 52
🏁 Script executed:
# Check the structure of the CLI directory
ls -la binaries/statespace-cli/src/Repository: statespace-tech/statespace
Length of output: 707
🏁 Script executed:
# Look for test files in the entire CLI crate
find binaries/statespace-cli -type f -name '*.rs' | grep -E '(test|tests)' | head -20Repository: statespace-tech/statespace
Length of output: 52
🏁 Script executed:
# Examine the app.rs file to understand the handlers and their scope
wc -l binaries/statespace-cli/src/commands/app.rs
head -150 binaries/statespace-cli/src/commands/app.rsRepository: statespace-tech/statespace
Length of output: 4448
🏁 Script executed:
# Search for test files or test modules in the entire CLI crate
find binaries/statespace-cli -type f -name '*.rs' | xargs grep -l "mod test\|#\[cfg(test)\]" 2>/dev/nullRepository: statespace-tech/statespace
Length of output: 269
🏁 Script executed:
# Check if there's a tests directory at the crate root
ls -la binaries/statespace-cli/tests/ 2>/dev/null || echo "No tests directory found"Repository: statespace-tech/statespace
Length of output: 94
🏁 Script executed:
# Search for any tests that reference app commands
rg "run_create|run_list|run_get|run_delete|AppCommands" binaries/statespace-cli --type rsRepository: statespace-tech/statespace
Length of output: 96
🏁 Script executed:
# Check for test coverage in the entire statespace codebase for CLI commands
rg "run_create|run_list|run_get|run_delete" --type rs --type toml | head -30Repository: statespace-tech/statespace
Length of output: 96
🏁 Script executed:
# Search for test references to the app command handlers
rg "run_create|run_list|run_get|run_delete" binaries/statespace-cli --glob='*.rs'Repository: statespace-tech/statespace
Length of output: 2221
🏁 Script executed:
# Check what files have test modules in the CLI
rg "#\[cfg\(test\)\]" binaries/statespace-cli --glob='*.rs' -A 2Repository: statespace-tech/statespace
Length of output: 951
🏁 Script executed:
# List all files in commands directory
ls -la binaries/statespace-cli/src/commands/Repository: statespace-tech/statespace
Length of output: 771
🏁 Script executed:
# Check if app.rs has any test module at the end
tail -50 binaries/statespace-cli/src/commands/app.rsRepository: statespace-tech/statespace
Length of output: 1545
🏁 Script executed:
# Verify that sync.rs (which has tests) and app.rs can be compared for test structure
echo "=== sync.rs has tests ===" && grep -A 20 "#\[cfg(test)\]" binaries/statespace-cli/src/commands/sync.rs | head -30
echo ""
echo "=== Confirming app.rs has NO test module ===" && grep -c "#\[cfg(test)\]" binaries/statespace-cli/src/commands/app.rs || echo "No test modules found"Repository: statespace-tech/statespace
Length of output: 733
🏁 Script executed:
# Check the complete app.rs to ensure we haven't missed anything
cat binaries/statespace-cli/src/commands/app.rsRepository: statespace-tech/statespace
Length of output: 4400
Add unit tests for app command handlers. The run_create, run_list, run_get, and run_delete functions contain business logic (path validation, API calls, output formatting, user confirmation) but lack test coverage. Similar CLI commands like sync.rs have test modules with mocks; apply the same pattern here.
Per coding guidelines: "CLI commands currently lack test coverage. Flag PRs that 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/app.rs` around lines 4 - 6, The app
command handlers (run_create, run_list, run_get, run_delete) lack unit tests;
add a test module mirroring the pattern in sync.rs that exercises path
validation, API calls, output formatting, and user confirmation by mocking the
gateway/application client and any filesystem or stdin interactions; create
tests for success and error paths for each function, assert correct calls to
normalize_application_reference/generate_name where applicable, and verify
formatted output and exit behavior using the same mock helpers and test harness
used by other CLI tests.
Comment on lines
+84
to
+90
| for app in &apps { | ||
| let status = match app.status { | ||
| ApplicationStatus::Running => format!("✓ {}", app.status), | ||
| ApplicationStatus::Pending | ApplicationStatus::Creating => { | ||
| format!("⏳ {}", app.status) | ||
| } | ||
| ApplicationStatus::Unknown => format!("✗ {}", app.status), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ApplicationStatus definition to check Copy/Clone implementation
rg -n "enum ApplicationStatus" --type rsRepository: statespace-tech/statespace
Length of output: 96
🏁 Script executed:
# Read the target file at lines 84-90
head -n 95 binaries/statespace-cli/src/commands/app.rs | tail -n 15Repository: statespace-tech/statespace
Length of output: 632
🏁 Script executed:
# Check the full ApplicationStatus enum definition and its derives
rg -A 5 "pub enum ApplicationStatus" --type rsRepository: statespace-tech/statespace
Length of output: 96
🏁 Script executed:
# Verify the for loop context - get more surrounding lines
sed -n '80,95p' binaries/statespace-cli/src/commands/app.rsRepository: statespace-tech/statespace
Length of output: 633
🏁 Script executed:
# Find ApplicationStatus definition without type filter
rg -n "enum ApplicationStatus" --glob "*.rs"Repository: statespace-tech/statespace
Length of output: 160
🏁 Script executed:
# Search in common locations
find . -name "*.rs" -type f | xargs grep -l "enum ApplicationStatus"Repository: statespace-tech/statespace
Length of output: 123
🏁 Script executed:
# Look in gateway or types files
find . -path "*/src/*" -name "*.rs" | xargs grep -n "enum ApplicationStatus"Repository: statespace-tech/statespace
Length of output: 162
🏁 Script executed:
# Read the ApplicationStatus enum definition with its derives
sed -n '28,40p' binaries/statespace-cli/src/gateway/applications.rsRepository: statespace-tech/statespace
Length of output: 428
🏁 Script executed:
# Get more context around the enum definition
sed -n '20,50p' binaries/statespace-cli/src/gateway/applications.rsRepository: statespace-tech/statespace
Length of output: 864
Fix move out of borrowed app in the status match—code will not compile.
for app in &apps yields &Application; matching on app.status attempts to move a non-Copy enum out of a borrowed reference. Match on &app.status instead to defer the dereference to the pattern matching logic.
🐛 Suggested fix
- let status = match app.status {
+ let status = match &app.status {
ApplicationStatus::Running => format!("✓ {}", app.status),
ApplicationStatus::Pending | ApplicationStatus::Creating => {
format!("⏳ {}", app.status)
}
ApplicationStatus::Unknown => format!("✗ {}", app.status),
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@binaries/statespace-cli/src/commands/app.rs` around lines 84 - 90, The match
is trying to move app.status out of a borrowed reference (for app in &apps).
Change the match to use a reference: match &app.status { ... } and update each
arm to match &ApplicationStatus::Running, &ApplicationStatus::Pending |
&ApplicationStatus::Creating, &ApplicationStatus::Unknown, etc., so you don't
attempt to move the enum out of &app while keeping the format! calls intact
(symbols: apps, app, ApplicationStatus, status).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
binaries/statespace-cli/src/commands/ssh_config.rs (1)
12-22:⚠️ Potential issue | 🟡 MinorUpdate documentation and clarify the SSH user change impact.
The hardcoded SSH config template now uses
User app(line 14), but the CLI documentation indocs/pages/reference/cli.mdstill referencesSSH user (default: env). This inconsistency needs resolution:
- If
--userflag behavior changed, updatecli.mdto reflect the new default- If the hardcoded template in
statespace_confignow usesappinstead ofenv, clarify whether this breaks existing user setups- Add migration guidance for users upgrading from older versions who may have
User envconfigurations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/commands/ssh_config.rs` around lines 12 - 22, The SSH template constant STATESPACE_SSH_CONFIG now hardcodes "User app" but docs still state "SSH user (default: env)"; update docs/pages/reference/cli.md to reflect that the CLI default user is now "app" (or, if the intended behavior is unchanged, revert STATESPACE_SSH_CONFIG to use the previous env-based default), and add a short migration note explaining how to change existing configs (e.g., replace "User env" with "User app" or re-run the CLI with the --user flag) plus whether this change can break existing setups; mention the --user flag behavior explicitly so readers know whether it overrides the template.binaries/statespace-cli/src/commands/tokens.rs (1)
63-63: 🧹 Nitpick | 🔵 TrivialTrack planned API migration for scope prefix change.
The code correctly handles the current API (which still uses
"environments:"prefix), but explicit TODO comments inbinaries/statespace-cli/src/gateway/client.rs(lines 241, 311) indicate the server API is planned to be updated to use"applications:"instead. When that migration happens, the hardcoded replacement at line 63 will need to be updated together with the gateway client changes.Consider adding a comment linking line 63 to those TODOs or refactoring to make the scope prefix configurable if the migration happens gradually.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/commands/tokens.rs` at line 63, The current hardcoded replacement let scope = token.scope.replace("environments:", ""); must be linked to the planned server API change (the TODOs referenced in the gateway client) or made configurable: either add a TODO comment next to this line referencing the gateway-client TODOs so future changes stay in sync, or refactor by introducing a single shared configurable prefix (e.g., SCOPE_PREFIX constant or config/env var) and replace token.scope.replace(...) with token.scope.replace(SCOPE_PREFIX, "") so both this code and the gateway client can read the same symbol and be updated together.binaries/statespace-cli/src/identifiers.rs (1)
29-35:⚠️ Potential issue | 🟠 MajorEnforce a
.label boundary when matching allowed host suffixes.Line 33 currently accepts hosts like
fooapp.statespace.com, becausestrip_suffix("app.statespace.com")does not require a dot boundary before the suffix.Suggested fix
- if let Some(stripped) = host.strip_suffix(suffix) { - let name = stripped.strip_suffix('.').unwrap_or(stripped); - if !name.is_empty() { - return Some(name.to_string()); - } - } + if let Some(name) = host.strip_suffix(suffix).and_then(|s| s.strip_suffix('.')) { + if !name.is_empty() { + return Some(name.to_string()); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/identifiers.rs` around lines 29 - 35, The current loop over ALLOWED_APP_HOST_SUFFIXES uses host.strip_suffix(suffix) which permits matches like "fooapp.statespace.com" because it doesn't require a dot boundary; update the check to enforce a dot before the suffix by matching either exact equality or stripping a dot-prefixed suffix. Concretely, in the loop handling ALLOWED_APP_HOST_SUFFIXES (the host variable and strip_suffix usage), replace the strip_suffix(suffix) logic with either host == suffix or host.strip_suffix(&format!(".{}", suffix)) (and then trim the leading dot with strip_suffix('.') as before), ensuring you only accept matches where the suffix is preceded by a '.'.
♻️ Duplicate comments (2)
binaries/statespace-cli/src/commands/app.rs (2)
84-90:⚠️ Potential issue | 🔴 CriticalFix move-out-of-borrow in status match (compile blocker).
Line 85 matches on
app.statuswhile iteratingfor app in &apps; this moves a non-Copyfield out of a borrowed value.Suggested fix
- let status = match app.status { + let status = match &app.status { ApplicationStatus::Running => format!("✓ {}", app.status), ApplicationStatus::Pending | ApplicationStatus::Creating => { format!("⏳ {}", app.status) } ApplicationStatus::Unknown => format!("✗ {}", app.status), };#!/bin/bash set -euo pipefail # Verify borrowed iteration + move-prone status match rg -n -C2 "for app in &apps|match app.status" binaries/statespace-cli/src/commands/app.rs # Verify ApplicationStatus does not derive Copy rg -n -C3 "derive\\(Debug, Clone, Deserialize, PartialEq, Eq\\)|pub\\(crate\\) enum ApplicationStatus" binaries/statespace-cli/src/gateway/applications.rsExpected verification result:
match app.statusappears inapp.rs, andApplicationStatusderivesClonebut notCopy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/commands/app.rs` around lines 84 - 90, The match on app.status while iterating with for app in &apps moves a non-Copy field out of a borrowed value; change the match to borrow the status instead (e.g., match &app.status) or match on app.status.clone() if you prefer cloning (ApplicationStatus derives Clone), and update the formatted arms to dereference or reference accordingly (the loop is in app.rs where for app in &apps and the enum is ApplicationStatus).
39-40:⚠️ Potential issue | 🟠 MajorAdd unit tests for the modified app command logic.
run_create,run_list,run_get, andrun_deletewere changed but this module still has no command-level tests validating updated gateway calls and output/confirmation behavior.As per coding guidelines: "CLI commands currently lack test coverage. Flag PRs that: Modify command logic without updating tests."
Also applies to: 68-71, 100-101, 115-116, 130-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binaries/statespace-cli/src/commands/app.rs` around lines 39 - 40, Add unit tests covering the CLI app command functions run_create, run_list, run_get, and run_delete to validate the updated gateway interactions and user-facing behavior: mock the gateway client used by create_application and other backend calls, assert that run_create calls .create_application(&name, files, args.visibility) with the right args and handles success/error output, test run_list/run_get produce the expected formatted output given mocked responses, and test run_delete confirmation/abort flows and resulting gateway calls; place tests alongside the module and ensure they assert printed output, prompts, and that the updated branches in those functions are exercised.
🤖 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 14-18: Rename the function resolve_env_id to resolve_app_id and
the local variable env to app inside that function; update its signature and all
call sites that invoke resolve_env_id to use resolve_app_id instead; inside the
function keep the logic (call normalize_application_reference,
gateway.get_application, map_err(Error::cli)) but return app.id and update any
references to the variable env to app (and update any tests/usages accordingly).
In `@binaries/statespace-cli/src/identifiers.rs`:
- Around line 14-15: The error message "Invalid application URL: {input}.
Expected https://{{name}}.app.statespace.com" and the parser (currently
accepting http on lines ~24-25) are inconsistent; update the URL parsing logic
in identifiers.rs (the code that validates application URLs) to require the
scheme "https" only (reject "http") so it matches the error text, and ensure the
validator returns the same error message when the scheme is not "https"; update
or add tests that exercise both "http" and "https" inputs to verify the stricter
validation.
---
Outside diff comments:
In `@binaries/statespace-cli/src/commands/ssh_config.rs`:
- Around line 12-22: The SSH template constant STATESPACE_SSH_CONFIG now
hardcodes "User app" but docs still state "SSH user (default: env)"; update
docs/pages/reference/cli.md to reflect that the CLI default user is now "app"
(or, if the intended behavior is unchanged, revert STATESPACE_SSH_CONFIG to use
the previous env-based default), and add a short migration note explaining how
to change existing configs (e.g., replace "User env" with "User app" or re-run
the CLI with the --user flag) plus whether this change can break existing
setups; mention the --user flag behavior explicitly so readers know whether it
overrides the template.
In `@binaries/statespace-cli/src/commands/tokens.rs`:
- Line 63: The current hardcoded replacement let scope =
token.scope.replace("environments:", ""); must be linked to the planned server
API change (the TODOs referenced in the gateway client) or made configurable:
either add a TODO comment next to this line referencing the gateway-client TODOs
so future changes stay in sync, or refactor by introducing a single shared
configurable prefix (e.g., SCOPE_PREFIX constant or config/env var) and replace
token.scope.replace(...) with token.scope.replace(SCOPE_PREFIX, "") so both this
code and the gateway client can read the same symbol and be updated together.
In `@binaries/statespace-cli/src/identifiers.rs`:
- Around line 29-35: The current loop over ALLOWED_APP_HOST_SUFFIXES uses
host.strip_suffix(suffix) which permits matches like "fooapp.statespace.com"
because it doesn't require a dot boundary; update the check to enforce a dot
before the suffix by matching either exact equality or stripping a dot-prefixed
suffix. Concretely, in the loop handling ALLOWED_APP_HOST_SUFFIXES (the host
variable and strip_suffix usage), replace the strip_suffix(suffix) logic with
either host == suffix or host.strip_suffix(&format!(".{}", suffix)) (and then
trim the leading dot with strip_suffix('.') as before), ensuring you only accept
matches where the suffix is preceded by a '.'.
---
Duplicate comments:
In `@binaries/statespace-cli/src/commands/app.rs`:
- Around line 84-90: The match on app.status while iterating with for app in
&apps moves a non-Copy field out of a borrowed value; change the match to borrow
the status instead (e.g., match &app.status) or match on app.status.clone() if
you prefer cloning (ApplicationStatus derives Clone), and update the formatted
arms to dereference or reference accordingly (the loop is in app.rs where for
app in &apps and the enum is ApplicationStatus).
- Around line 39-40: Add unit tests covering the CLI app command functions
run_create, run_list, run_get, and run_delete to validate the updated gateway
interactions and user-facing behavior: mock the gateway client used by
create_application and other backend calls, assert that run_create calls
.create_application(&name, files, args.visibility) with the right args and
handles success/error output, test run_list/run_get produce the expected
formatted output given mocked responses, and test run_delete confirmation/abort
flows and resulting gateway calls; place tests alongside the module and ensure
they assert printed output, prompts, and that the updated branches in those
functions are exercised.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
binaries/statespace-cli/src/args.rsbinaries/statespace-cli/src/commands/app.rsbinaries/statespace-cli/src/commands/secrets.rsbinaries/statespace-cli/src/commands/ssh.rsbinaries/statespace-cli/src/commands/ssh_config.rsbinaries/statespace-cli/src/commands/sync.rsbinaries/statespace-cli/src/commands/tokens.rsbinaries/statespace-cli/src/gateway/applications.rsbinaries/statespace-cli/src/gateway/client.rsbinaries/statespace-cli/src/gateway/environments.rsbinaries/statespace-cli/src/gateway/mod.rsbinaries/statespace-cli/src/gateway/tokens.rsbinaries/statespace-cli/src/identifiers.rsbinaries/statespace-cli/src/names.rs
💤 Files with no reviewable changes (1)
- binaries/statespace-cli/src/gateway/environments.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
binaries/statespace-cli/src/commands/secrets.rs (1)
20-57:⚠️ Potential issue | 🟠 MajorAdd unit tests for the updated secrets command resolution flow.
run_set,run_list, andrun_deletenow rely onresolve_application_id/get_application, but this command change ships without matching tests in this file. Please add tests that cover successful resolution and invalid reference failures.As per coding guidelines: "CLI commands currently lack test coverage. Flag PRs that: ... 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 20 - 57, Add unit tests for run_set, run_list, and run_delete that exercise the new application-resolution flow: create tests that mock/stub GatewayClient so resolve_application_id/get_application returns a valid app id and assert the command calls (gateway.set_secret, gateway.list_secret_keys, gateway.delete_secret) are invoked and expected stdout/stderr contains the success messages; and add tests where resolve_application_id/get_application fails (invalid reference) and assert the command returns the appropriate Err and does not call the gateway methods. Name tests to reference run_set, run_list, run_delete and use async test harness (tokio or equivalent) so they exercise the async functions. Ensure both success and invalid-reference failure paths are covered for each command.
🤖 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 14-17: The function resolve_application_id is shadowing the
parameter app by reusing the name for the fetched entity; rename the fetched
variable returned from gateway.get_application(...) to a clearer name like
application (or resolved_application) and update the subsequent Ok(...) to use
application.id so the parameter and the fetched entity are distinct (refer to
resolve_application_id and gateway.get_application).
---
Outside diff comments:
In `@binaries/statespace-cli/src/commands/secrets.rs`:
- Around line 20-57: Add unit tests for run_set, run_list, and run_delete that
exercise the new application-resolution flow: create tests that mock/stub
GatewayClient so resolve_application_id/get_application returns a valid app id
and assert the command calls (gateway.set_secret, gateway.list_secret_keys,
gateway.delete_secret) are invoked and expected stdout/stderr contains the
success messages; and add tests where resolve_application_id/get_application
fails (invalid reference) and assert the command returns the appropriate Err and
does not call the gateway methods. Name tests to reference run_set, run_list,
run_delete and use async test harness (tokio or equivalent) so they exercise the
async functions. Ensure both success and invalid-reference failure paths are
covered for each command.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
binaries/statespace-cli/src/commands/secrets.rsbinaries/statespace-cli/src/commands/sync.rsbinaries/statespace-cli/src/gateway/client.rs
Comment on lines
+14
to
+17
| async fn resolve_application_id(gateway: &GatewayClient, app: &str) -> Result<String> { | ||
| let reference = normalize_application_reference(app).map_err(Error::cli)?; | ||
| let app = gateway.get_application(&reference).await?; | ||
| Ok(app.id) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid parameter shadowing in resolve_application_id.
Line 16 reuses app as a different type, which hurts readability. Rename the resolved entity variable to application (or similar).
♻️ Proposed cleanup
async fn resolve_application_id(gateway: &GatewayClient, app: &str) -> Result<String> {
let reference = normalize_application_reference(app).map_err(Error::cli)?;
- let app = gateway.get_application(&reference).await?;
- Ok(app.id)
+ let application = gateway.get_application(&reference).await?;
+ Ok(application.id)
}As per coding guidelines: "Write self-documenting code - prefer clear names over comments."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn resolve_application_id(gateway: &GatewayClient, app: &str) -> Result<String> { | |
| let reference = normalize_application_reference(app).map_err(Error::cli)?; | |
| let app = gateway.get_application(&reference).await?; | |
| Ok(app.id) | |
| async fn resolve_application_id(gateway: &GatewayClient, app: &str) -> Result<String> { | |
| let reference = normalize_application_reference(app).map_err(Error::cli)?; | |
| let application = gateway.get_application(&reference).await?; | |
| Ok(application.id) | |
| } |
🤖 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 14 - 17, The
function resolve_application_id is shadowing the parameter app by reusing the
name for the fetched entity; rename the fetched variable returned from
gateway.get_application(...) to a clearer name like application (or
resolved_application) and update the subsequent Ok(...) to use application.id so
the parameter and the fetched entity are distinct (refer to
resolve_application_id and gateway.get_application).
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
2 participants
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.
Summary
Changes
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspaceChecklist
Summary by CodeRabbit