feat: implement Apollo CLI OpenAPI v0#1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Rust apollo-cli v0 scaffold: clap CLI surface, command execution (init/auth/profile/app/env/namespace/config/release/api), platform-aware config and credential backends (env/file/native), OpenAPI HTTP client, structured output/errors, secret redaction, CI workflow, and extensive tests. ChangesApollo CLI Scaffold v0
Sequence Diagram (high-level request flow) sequenceDiagram
participant User
participant Parser
participant Execute
participant Config
participant Credential
participant OpenApiClient
participant OutputWriter
User->>Parser: run `apollo ...`
Parser->>Execute: execute(Cli)
Execute->>Config: load_config / resolve_context
Execute->>Credential: resolve_token or token_from_env_or_stdin
Execute->>OpenApiClient: request(method, path, body)
OpenApiClient->>Execute: OpenApiResponse / CliError
Execute->>OutputWriter: render_success / render_error
OutputWriter->>User: write stdout/stderr (with redaction)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67df365ccd
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR scaffolds a standalone Rust-based apollo CLI (v0 foundation), including initial command routing, config/context resolution, credential storage abstraction, redaction utilities, structured output/errors, and CI/testing setup.
Changes:
- Adds clap-based CLI surface area with top-level command groups (
auth,profile, etc.) plus global flags and basic routing. - Implements config loading/saving + runtime context resolution, along with initial
profileandauthcommand behavior. - Introduces structured output/error rendering with redaction, and adds integration/unit tests plus a basic Rust CI workflow.
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/auth.rs | Integration tests for auth flows (env token, file fallback, logout). |
| tests/cli.rs | Integration tests for help output, global flags, and placeholder command errors. |
| tests/profile.rs | Integration tests for profile commands and context precedence. |
| tests/redaction.rs | Tests for redaction of text, JSON fields, and Sensitive debug output. |
| src/cli.rs | clap CLI definitions (commands + global flags + output enum). |
| src/command.rs | Command dispatch + implementations for auth/profile scaffolding. |
| src/config.rs | Config path resolution, config load/save, and runtime context/output resolution. |
| src/credential.rs | Credential store abstraction (native/file/env) + token acquisition helpers. |
| src/error.rs | Structured error model and rendering via OutputWriter. |
| src/lib.rs | Library entrypoint + CLI parse tests. |
| src/main.rs | Binary entrypoint delegating to apollo_cli::main_entry. |
| src/output.rs | Output writer for JSON/table + redaction hook. |
| src/redaction.rs | Redaction utilities and Sensitive wrapper type. |
| README.md | Developer docs and CLI behavior overview for the scaffold. |
| LICENSE | Apache 2.0 license text. |
| Cargo.toml | Crate manifest with dependencies and dev-dependencies. |
| Cargo.lock | Dependency lockfile for reproducible builds. |
| .gitignore | Ignores Rust build artifacts (/target). |
| .github/workflows/ci.yml | CI workflow running fmt, clippy (warnings as errors), and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
Cargo.toml (1)
15-22: ⚡ Quick winAlign
serde_jsonversion between runtime and dev dependencies.Using different
serde_jsonversions across[dependencies]and[dev-dependencies]can cause unnecessary duplicate crate resolution and harder debugging. Prefer a single version unless a test-only constraint requires divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 15 - 22, The crate lists serde_json twice with mismatched versions; update the serde_json entry under [dev-dependencies] to match the runtime version used in [dependencies] (change serde_json = "1.0.145" to serde_json = "1.0.150" or vice‑versa so both use the same version string) to avoid duplicate crate resolution; edit Cargo.toml to make the two serde_json entries identical and then run cargo update if needed to refresh the lockfile.src/output.rs (1)
63-78: ⚡ Quick winDistinguish serialization error messages.
Lines 68 and 71-72 use identical
.expect("structured success json serialization")messages for different serialization operations (to_valuevsto_string_pretty). If either fails, the message won't indicate which step caused the panic.♻️ Proposed fix to clarify error messages
OutputFormat::Json => { let value = - serde_json::to_value(value).expect("structured success json serialization"); + serde_json::to_value(value).expect("serialize success to json value"); let value = redactor.redact_json(value); RenderedOutput::stdout( serde_json::to_string_pretty(&value) - .expect("structured success json serialization"), + .expect("pretty-print json value"), ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/output.rs` around lines 63 - 78, The two identical expect messages in render_success's OutputFormat::Json arm obscure which serde step failed; update the expect on serde_json::to_value(value) to a message like "failed to convert success value to serde_json::Value in render_success" and change the expect on serde_json::to_string_pretty(&value) to something like "failed to serialize redacted success JSON to string in render_success" (refer to render_success, OutputFormat::Json, serde_json::to_value, serde_json::to_string_pretty, and redactor.redact_json to locate the lines).src/error.rs (1)
216-221: ⚡ Quick winExtract duplicated
ensure_trailing_newlinehelper.This function is identical to the one in
src/output.rslines 95-100. Duplication makes maintenance harder if the logic ever needs to change.♻️ Proposed fix to eliminate duplication
Move the helper to
output.rsand make itpub(crate), then reuse it here:In
src/output.rs:-fn ensure_trailing_newline(mut body: String) -> String { +pub(crate) fn ensure_trailing_newline(mut body: String) -> String { if !body.ends_with('\n') { body.push('\n'); } body }In
src/error.rs:+use crate::output::ensure_trailing_newline; + use crate::cli::OutputFormat; use crate::output::{OutputStream, OutputWriter, RenderedOutput, StructuredError}; // ... rest of file ... -fn ensure_trailing_newline(mut body: String) -> String { - if !body.ends_with('\n') { - body.push('\n'); - } - body -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/error.rs` around lines 216 - 221, The function ensure_trailing_newline is duplicated; remove the copy from the error module and expose the single implementation in the output module as pub(crate) fn ensure_trailing_newline(...), then call that shared helper from the error code (importing or referencing output::ensure_trailing_newline as needed) so both modules reuse the one implementation and no duplicate helper remains.src/cli.rs (1)
56-61: ⚡ Quick winConsider adding
value_nameto the Use command's name argument.The
namefield will work as a required positional argument, but adding avalue_nameattribute would improve the help text clarity for users.📝 Suggested improvement
#[derive(Clone, Debug, Eq, PartialEq, Subcommand)] pub enum ProfileCommand { List, Show, - Use { name: String }, + Use { + #[arg(value_name = "PROFILE_NAME")] + name: String, + }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.rs` around lines 56 - 61, Update the ProfileCommand::Use variant to annotate its name field with a clap value_name so the help text displays a clear placeholder; specifically, add a #[clap(value_name = "NAME")] (or similar casing you prefer) attribute on the Use variant's name field (the "name: String" field) so clap shows NAME instead of a generic placeholder in the help output.src/config.rs (1)
56-64: 💤 Low valueConsider documenting the Linux fallback assumption.
The current implementation treats any non-macOS, non-Windows platform as Linux. This is reasonable for a v0 scaffold, but a comment would help future maintainers understand this design choice.
📝 Suggested documentation
fn current() -> Self { if cfg!(target_os = "macos") { Self::MacOs } else if cfg!(target_os = "windows") { Self::Windows } else { + // Treat all other platforms (Linux, BSD, etc.) as Linux Self::Linux } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.rs` around lines 56 - 64, Add a short inline comment above the current() function explaining that the implementation intentionally treats any non-macOS and non-Windows target as Linux (a pragmatic fallback for the v0 scaffold) so future maintainers understand the assumption and can extend the match to other OSes; reference the current() function and its returned variants Self::MacOs, Self::Windows, and Self::Linux and keep the comment concise and placed adjacent to the function definition.tests/profile.rs (1)
231-240: ⚡ Quick winInconsistent environment cleanup vs auth tests.
The
base_commandhelper intests/auth.rsremovesAPOLLO_TOKEN(line 187) and setsAPOLLO_CLI_TEST_DISABLE_NATIVE(line 190), but this version does neither. IfAPOLLO_TOKENis set in the CI environment, it could interfere with profile tests. For consistency and isolation, consider aligning both test files.🔧 Align with auth.rs env handling
fn base_command(home: &TempDir) -> Command { let mut command = Command::cargo_bin("apollo").expect("apollo binary"); command.env_remove("APOLLO_PROFILE"); command.env_remove("APOLLO_SERVER"); command.env_remove("APOLLO_OUTPUT"); + command.env_remove("APOLLO_TOKEN"); command.env_remove("XDG_CONFIG_HOME"); command.env_remove("APPDATA"); + command.env("APOLLO_CLI_TEST_DISABLE_NATIVE", "1"); command.env("HOME", home.path()); command }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/profile.rs` around lines 231 - 240, The base_command helper in tests/profile.rs currently doesn't remove APOLLO_TOKEN or set APOLLO_CLI_TEST_DISABLE_NATIVE, causing CI environment tokens to leak into profile tests; update the base_command function to call command.env_remove("APOLLO_TOKEN") and command.env("APOLLO_CLI_TEST_DISABLE_NATIVE", "1") (matching the handling in tests/auth.rs) so tests run in isolation and native auth is disabled.tests/auth.rs (1)
182-230: ⚡ Quick winConsider extracting common test helpers to a shared module.
The helpers
base_command,temp_home,config_path,write_config, andnormalize_tomlare duplicated in bothtests/auth.rsandtests/profile.rswith only minor differences (e.g.,APOLLO_CLI_TEST_DISABLE_NATIVEenv var). Extracting them to a common test support module (e.g.,tests/common/mod.rs) would reduce duplication and ensure consistent test setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/auth.rs` around lines 182 - 230, Extract the duplicated test helpers into a shared test support module and update tests to import them: create a tests/common/mod.rs (or tests/common.rs) and move functions base_command, temp_home, config_path, credential_file_path, write_config, and normalize_toml into it; change base_command to accept optional extra env overrides (e.g., a slice or map) so the minor difference APOLLO_CLI_TEST_DISABLE_NATIVE can be passed from tests/auth.rs vs tests/profile.rs; update both tests to use common::base_command(...) and the other helpers, remove the duplicated implementations, and ensure module visibility (pub) for the moved functions.src/command.rs (2)
93-96: ⚡ Quick winDuplicated default credential logic.
This constructs a default
CredentialRefidentical to the logic incredential::status()(lines 154-157). As noted in the credential.rs review, extract this to a shared helper likecredential::default_credential_ref(profile).♻️ Use shared helper
After adding
default_credential_reftocredential.rs:- let credential_ref = context.credential.clone().unwrap_or_else(|| CredentialRef { - backend: "native".to_owned(), - key: profile.clone(), - }); + let credential_ref = context + .credential + .clone() + .unwrap_or_else(|| credential::default_credential_ref(&profile));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command.rs` around lines 93 - 96, This code duplicates the default CredentialRef construction used in credential::status(); replace the inline construction in command.rs (the credential_ref initialization that uses context.credential.clone().unwrap_or_else(...) and the CredentialRef { backend: "native", key: profile.clone() }) by calling a shared helper credential::default_credential_ref(profile) (add that helper to credential.rs if not present) and use context.credential.clone().unwrap_or_else(|| credential::default_credential_ref(profile.clone())) so the default logic is centralized.
82-88: ⚡ Quick winRedundant clone of profile string.
profileon line 84 is cloned even though it's already aStringthat was cloned from the context on line 55. This creates an unnecessary allocation.♻️ Remove redundant clone
let response = AuthLoginResponse { stored: true, - profile: profile.clone(), + profile, backend: credential_ref.backend, key: credential_ref.key, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command.rs` around lines 82 - 88, The AuthLoginResponse construction needlessly clones profile; replace profile.clone() with profile to avoid the extra allocation (use the moved String for AuthLoginResponse.profile), and confirm there are no subsequent uses of the profile variable after constructing AuthLoginResponse; relevant symbols: profile, AuthLoginResponse, credential_ref, writer.render_success.src/credential.rs (4)
159-163: 💤 Low valueInconsistent handling of unknown credential backends.
Unknown credential backends are silently ignored but with different semantics:
status()returnsOk(None)(line 162), whiledelete()returnsOk(())(line 200). While this might enable forward compatibility, it could also hide configuration mistakes. Consider whether unknown backends should produce a warning or error.Also applies to: 194-201
204-228: 💤 Low valueFunction name order doesn't match execution order.
The function is named
token_from_env_or_stdin, suggesting env is checked first, but the implementation checks stdin first (based on thetoken_stdinflag) and only falls back to env when the flag is false. Consider renaming totoken_from_stdin_or_envto match the actual precedence, or reorder the logic to match the name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/credential.rs` around lines 204 - 228, The function token_from_env_or_stdin currently checks stdin first (via the token_stdin flag) then falls back to env::var("APOLLO_TOKEN"), which contradicts its name; either rename the function to token_from_stdin_or_env to reflect stdin-first precedence or flip the logic so it queries env::var("APOLLO_TOKEN") first and only reads stdin when env is empty or token_stdin is true. Update the function signature/name (token_from_env_or_stdin) and all call sites accordingly if renaming, or reorder the branches around token_stdin, Sensitive::new, and env::var to match the chosen precedence.
92-99: 💤 Low valueConsider documenting the fallback behavior when config_path has no parent.
The
parent().unwrap_or_else(|| Path::new("."))fallback assumes that if the config file has no parent directory, the credentials directory should be./credentials. While this is unlikely given typical config paths, it could be surprising if config_path is just a bare filename.📝 Optional clarifying comment
pub fn new(config_path: &Path) -> Self { Self { + // Credentials are stored in a sibling "credentials" directory. + // If config_path has no parent, fall back to "./credentials". base_dir: config_path .parent() .unwrap_or_else(|| Path::new(".")) .join("credentials"), } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/credential.rs` around lines 92 - 99, The constructor Credential::new currently computes base_dir from config_path.parent().unwrap_or_else(|| Path::new(".")).join("credentials") which silently falls back to "./credentials" when config_path has no parent; add a brief doc comment on pub fn new (or an inline comment above the base_dir initialization) documenting this fallback behavior — that when config_path is a bare filename the credentials directory becomes ./credentials — so callers are not surprised, and mention the rationale (fallback to current directory) and any alternative callers should use if they need a different resolution.
154-157: ⚡ Quick winConsolidate the default credential reference construction.
The default
CredentialRef { backend: "native", key: profile }pattern appears in both thestatusfunction (lines 154-157) and is duplicated incommand.rs(logout handler, lines 93-96). Extracting this to a shared helper improves maintainability and ensures consistency.♻️ Proposed helper function
+pub fn default_credential_ref(profile: &str) -> CredentialRef { + CredentialRef { + backend: "native".to_owned(), + key: profile.to_owned(), + } +} + pub fn status( config_path: &Path, profile: &str, credential: Option<&CredentialRef>, ) -> CredentialStatus { // ... - let credential = credential.cloned().unwrap_or_else(|| CredentialRef { - backend: "native".to_owned(), - key: profile.to_owned(), - }); + let credential = credential.cloned().unwrap_or_else(|| default_credential_ref(profile)); // ... }Then update
command.rsline 93-96 to use this helper.Also applies to: 193-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/credential.rs` around lines 154 - 157, The code duplicates the default CredentialRef { backend: "native", key: profile } construction in src/credential.rs (status) and in command.rs (logout handler) and elsewhere (lines ~193-202); refactor by adding a single helper function (e.g., fn default_credential_ref(profile: &str) -> CredentialRef) inside src/credential.rs or a shared module, returning CredentialRef with backend = "native" and key = profile.to_owned(), then replace the direct inline constructions in the status function (the credential.cloned().unwrap_or_else(...) call), the logout handler in command.rs, and the other duplicated site(s) to call this helper to avoid repetition and ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 15-17: Replace the mutable tag refs for GitHub Actions in the
workflow with their immutable commit SHAs: update the three "uses" entries shown
(actions/checkout@v4, dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2) to
pin each to the corresponding commit SHA at the desired version; fetch the exact
commit SHAs from each action's repository (or the release commit that matches
the tags) and substitute the tag refs with the form uses:
owner/repo@<commit-sha> so the workflow uses immutable SHAs instead of
`@v4/`@stable/@v2.
- Line 15: Update the GitHub Actions checkout step to disable persisted
credentials by adding the persist-credentials: false input to the
actions/checkout@v4 step; locate the checkout step (uses: actions/checkout@v4)
in the workflow and add the persist-credentials: false key so downstream steps
do not receive the checkout token.
In `@src/command.rs`:
- Around line 210-220: The server field currently uses unwrap_or_default() in
ProfileSummaryRow::from_profile which yields an empty string for None and causes
inconsistent table output compared to the "<none>" placeholder used elsewhere;
update ProfileSummaryRow::from_profile so its server value maps missing servers
to the same "<none>" placeholder (e.g., replace unwrap_or_default() with logic
that returns "<none>" when profile.server is None) so ProfileSummaryRow::server
rendering matches other missing-value behavior.
In `@src/credential.rs`:
- Around line 116-126: The set method in credential.rs writes the token file
(function set, using path_for_key and Sensitive) but only applies 0o600
permissions on Unix; on Windows you must either apply appropriate ACL
restrictions or explicitly document the limitation. Fix by adding a
Windows-specific branch after fs::write(&path, ...) that calls a helper (e.g.,
set_file_permissions_windows or set_windows_acl) to restrict the file to the
current user using Win32 ACL APIs (or the crate windows-rs/winapi) and return
map_err on failure; alternatively, if you cannot implement ACLs now, add a clear
comment and runtime warning/error in set noting that Windows permissions are not
enforced and recommend secure storage/backends.
In `@src/output.rs`:
- Around line 37-61: In render_error, JSON-formatted errors are currently
emitted via RenderedOutput::stdout; change the JSON branch (match arm for
OutputFormat::Json in render_error) to return a RenderedOutput with
OutputStream::Stderr (the same stream used by the Table branch) instead of using
RenderedOutput::stdout so errors consistently go to stderr; keep the existing
serde_json::to_string_pretty call and redactor.redact_json usage but construct
the RenderedOutput with stream = OutputStream::Stderr and the serialized body.
In `@src/redaction.rs`:
- Around line 11-22: The regexes in Redactor::redact_text (the local variables
authorization and consumer_token) are being compiled on every call; move their
Regex::new(...) initialization into static, lazily-initialized globals (e.g.,
using std::sync::OnceLock or lazy_static) such as AUTHORIZATION and
CONSUMER_TOKEN, then change redact_text to reference these statics (calling
get_or_init() or deref) and reuse the precompiled Regex instances when
performing replace_all; keep the same capture groups and replacement logic
(format!("${{1}}{}", REDACTED)) but remove the per-call compilation.
In `@tests/auth.rs`:
- Around line 207-214: The credential_file_path function currently hardcodes a
macOS path; make it platform-aware by using a standard data directory helper
(e.g., dirs, dirs-next or directories crate) instead of "Library/Application
Support" so tests build the path from the system data directory; update
credential_file_path to call the appropriate API (e.g.,
dirs_next::data_local_dir or directories::ProjectDirs::data_dir) and then join
"apollo/credentials/<key>.token" (keeping TempDir and PathBuf usage) so Linux
uses ~/.local/share or XDG dirs and macOS keeps Library/Application Support.
- Around line 199-205: The helper config_path in tests/auth.rs currently
hardcodes the macOS "Library/Application Support/apollo/config.toml" location
which breaks Linux CI; update config_path(TempDir) to detect the platform at
runtime and return the appropriate path: on macOS keep the existing
Library/Application Support/apollo/config.toml, otherwise use XDG_CONFIG_HOME if
set or fallback to home/.config/apollo/config.toml (constructing with PathBuf
joins). Ensure the function references TempDir.path() and builds the path
accordingly so tests on Linux will find the config file.
In `@tests/profile.rs`:
- Around line 246-252: The config_path helper hardcodes macOS paths; update the
config_path function in tests/profile.rs to be platform-aware like
tests/auth.rs: detect macOS (cfg!(target_os = "macos")) and return
home.path().join("Library").join("Application
Support").join("apollo").join("config.toml") for macOS, otherwise return the
Linux/Unix equivalent (e.g.,
home.path().join(".config").join("apollo").join("config.toml")); keep the
function signature config_path(home: &TempDir) -> PathBuf and mirror the same
branching logic used in tests/auth.rs so CI on Linux uses the correct path.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 15-22: The crate lists serde_json twice with mismatched versions;
update the serde_json entry under [dev-dependencies] to match the runtime
version used in [dependencies] (change serde_json = "1.0.145" to serde_json =
"1.0.150" or vice‑versa so both use the same version string) to avoid duplicate
crate resolution; edit Cargo.toml to make the two serde_json entries identical
and then run cargo update if needed to refresh the lockfile.
In `@src/cli.rs`:
- Around line 56-61: Update the ProfileCommand::Use variant to annotate its name
field with a clap value_name so the help text displays a clear placeholder;
specifically, add a #[clap(value_name = "NAME")] (or similar casing you prefer)
attribute on the Use variant's name field (the "name: String" field) so clap
shows NAME instead of a generic placeholder in the help output.
In `@src/command.rs`:
- Around line 93-96: This code duplicates the default CredentialRef construction
used in credential::status(); replace the inline construction in command.rs (the
credential_ref initialization that uses
context.credential.clone().unwrap_or_else(...) and the CredentialRef { backend:
"native", key: profile.clone() }) by calling a shared helper
credential::default_credential_ref(profile) (add that helper to credential.rs if
not present) and use context.credential.clone().unwrap_or_else(||
credential::default_credential_ref(profile.clone())) so the default logic is
centralized.
- Around line 82-88: The AuthLoginResponse construction needlessly clones
profile; replace profile.clone() with profile to avoid the extra allocation (use
the moved String for AuthLoginResponse.profile), and confirm there are no
subsequent uses of the profile variable after constructing AuthLoginResponse;
relevant symbols: profile, AuthLoginResponse, credential_ref,
writer.render_success.
In `@src/config.rs`:
- Around line 56-64: Add a short inline comment above the current() function
explaining that the implementation intentionally treats any non-macOS and
non-Windows target as Linux (a pragmatic fallback for the v0 scaffold) so future
maintainers understand the assumption and can extend the match to other OSes;
reference the current() function and its returned variants Self::MacOs,
Self::Windows, and Self::Linux and keep the comment concise and placed adjacent
to the function definition.
In `@src/credential.rs`:
- Around line 204-228: The function token_from_env_or_stdin currently checks
stdin first (via the token_stdin flag) then falls back to
env::var("APOLLO_TOKEN"), which contradicts its name; either rename the function
to token_from_stdin_or_env to reflect stdin-first precedence or flip the logic
so it queries env::var("APOLLO_TOKEN") first and only reads stdin when env is
empty or token_stdin is true. Update the function signature/name
(token_from_env_or_stdin) and all call sites accordingly if renaming, or reorder
the branches around token_stdin, Sensitive::new, and env::var to match the
chosen precedence.
- Around line 92-99: The constructor Credential::new currently computes base_dir
from config_path.parent().unwrap_or_else(|| Path::new(".")).join("credentials")
which silently falls back to "./credentials" when config_path has no parent; add
a brief doc comment on pub fn new (or an inline comment above the base_dir
initialization) documenting this fallback behavior — that when config_path is a
bare filename the credentials directory becomes ./credentials — so callers are
not surprised, and mention the rationale (fallback to current directory) and any
alternative callers should use if they need a different resolution.
- Around line 154-157: The code duplicates the default CredentialRef { backend:
"native", key: profile } construction in src/credential.rs (status) and in
command.rs (logout handler) and elsewhere (lines ~193-202); refactor by adding a
single helper function (e.g., fn default_credential_ref(profile: &str) ->
CredentialRef) inside src/credential.rs or a shared module, returning
CredentialRef with backend = "native" and key = profile.to_owned(), then replace
the direct inline constructions in the status function (the
credential.cloned().unwrap_or_else(...) call), the logout handler in command.rs,
and the other duplicated site(s) to call this helper to avoid repetition and
ensure consistency.
In `@src/error.rs`:
- Around line 216-221: The function ensure_trailing_newline is duplicated;
remove the copy from the error module and expose the single implementation in
the output module as pub(crate) fn ensure_trailing_newline(...), then call that
shared helper from the error code (importing or referencing
output::ensure_trailing_newline as needed) so both modules reuse the one
implementation and no duplicate helper remains.
In `@src/output.rs`:
- Around line 63-78: The two identical expect messages in render_success's
OutputFormat::Json arm obscure which serde step failed; update the expect on
serde_json::to_value(value) to a message like "failed to convert success value
to serde_json::Value in render_success" and change the expect on
serde_json::to_string_pretty(&value) to something like "failed to serialize
redacted success JSON to string in render_success" (refer to render_success,
OutputFormat::Json, serde_json::to_value, serde_json::to_string_pretty, and
redactor.redact_json to locate the lines).
In `@tests/auth.rs`:
- Around line 182-230: Extract the duplicated test helpers into a shared test
support module and update tests to import them: create a tests/common/mod.rs (or
tests/common.rs) and move functions base_command, temp_home, config_path,
credential_file_path, write_config, and normalize_toml into it; change
base_command to accept optional extra env overrides (e.g., a slice or map) so
the minor difference APOLLO_CLI_TEST_DISABLE_NATIVE can be passed from
tests/auth.rs vs tests/profile.rs; update both tests to use
common::base_command(...) and the other helpers, remove the duplicated
implementations, and ensure module visibility (pub) for the moved functions.
In `@tests/profile.rs`:
- Around line 231-240: The base_command helper in tests/profile.rs currently
doesn't remove APOLLO_TOKEN or set APOLLO_CLI_TEST_DISABLE_NATIVE, causing CI
environment tokens to leak into profile tests; update the base_command function
to call command.env_remove("APOLLO_TOKEN") and
command.env("APOLLO_CLI_TEST_DISABLE_NATIVE", "1") (matching the handling in
tests/auth.rs) so tests run in isolation and native auth is disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5465d79d-b6b5-43e9-826e-591047041a0d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/ci.yml.gitignoreCargo.tomlLICENSEREADME.mdsrc/cli.rssrc/command.rssrc/config.rssrc/credential.rssrc/error.rssrc/lib.rssrc/main.rssrc/output.rssrc/redaction.rstests/auth.rstests/cli.rstests/profile.rstests/redaction.rs
nobodyiam
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 266455ece1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 084f6a9741
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/openapi.rs (1)
353-360: ⚡ Quick winFail fast instead of hanging when a request is never captured.
These helpers block forever on
recv(). If a future regression skips the network call, the suite stalls instead of producing a clear failure.Suggested patch
use std::fs; use std::io::{Read, Write}; use std::net::{SocketAddr, TcpListener, TcpStream}; use std::path::PathBuf; use std::sync::mpsc::{self, Receiver}; use std::thread; +use std::time::Duration; @@ fn request(self) -> CapturedRequest { - self.request_rx.recv().expect("captured request") + self.request_rx + .recv_timeout(Duration::from_secs(5)) + .expect("captured request") } fn requests(self, count: usize) -> Vec<CapturedRequest> { (0..count) - .map(|_| self.request_rx.recv().expect("captured request")) + .map(|_| { + self.request_rx + .recv_timeout(Duration::from_secs(5)) + .expect("captured request") + }) .collect() } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/openapi.rs` around lines 353 - 360, The helpers request and requests currently block forever on self.request_rx.recv(), causing test suites to hang; update them to fail fast by using recv_timeout with a reasonable Duration (e.g. Duration::from_secs(1)) instead of recv(), and change the expect message to indicate a timed-out wait for a captured request; apply the same change inside requests' loop (mapping over request_rx.recv_timeout(...).expect(...)) and add the necessary use of std::time::Duration so a missing network call produces a clear panic instead of hanging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/command.rs`:
- Around line 460-483: The helpers required_server and required_operator
currently accept Some("") as valid; change them to treat empty or
whitespace-only strings as missing by filtering out blank values (e.g., use
.filter(|s| !s.trim().is_empty()) on context.server and on the command_operator
/ context.operator results) so they return the existing CliError::invalid_input
when the value is empty; update required_operator to apply this blank-check both
to the mapped command_operator and to context.operator before calling
ok_or_else.
In `@src/http.rs`:
- Around line 64-69: The code currently passes the raw response body into
CliError::http_status (in the status.is_success() failure branch), which can
leak sensitive echoed data; change the call to first redact or bound the body
(e.g., replace with a redacted/trimmed string or call a shared redact helper)
before passing it to CliError::http_status, ensuring you reference the same
parameters (path, format) but provide a safe version of body (for example
truncated to a safe max length and with sensitive patterns masked) so only
redacted/limited content is rendered.
- Around line 47-58: The blocking HTTP flow uses self.client.request(...).send()
without a timeout and embeds raw response.text() into CliError::http_status; add
a per-request timeout (e.g., request = request.timeout(Duration::from_secs(30)))
on the reqwest::blocking::RequestBuilder before .send() to prevent hangs, and
when handling non-2xx responses, parse the response body as JSON
(serde_json::Value) and either redact sensitive keys or truncate/serialize a
sanitized subset before passing it to CliError::http_status (instead of the raw
text and instead of relying only on Redactor::redact_text for
OutputFormat::Table); update references around request, .send(),
CliError::network and CliError::http_status to use the sanitized payload.
---
Nitpick comments:
In `@tests/openapi.rs`:
- Around line 353-360: The helpers request and requests currently block forever
on self.request_rx.recv(), causing test suites to hang; update them to fail fast
by using recv_timeout with a reasonable Duration (e.g. Duration::from_secs(1))
instead of recv(), and change the expect message to indicate a timed-out wait
for a captured request; apply the same change inside requests' loop (mapping
over request_rx.recv_timeout(...).expect(...)) and add the necessary use of
std::time::Duration so a missing network call produces a clear panic instead of
hanging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c90a78ec-f1ea-4712-b9e7-37a193a99aae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlREADME.mdsrc/cli.rssrc/command.rssrc/credential.rssrc/error.rssrc/http.rssrc/lib.rstests/auth.rstests/cli.rstests/openapi.rstests/profile.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cli.rs
- tests/auth.rs
- src/credential.rs
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/cli.rs (1)
80-107: ⚖️ Poor tradeoffConsider extracting shared profile setup fields.
InitArgsandProfileAddArgsduplicate five fields:operator,token_stdin,store_token_in_file,overwrite, and (with different positioning)name. While duplication in CLI arg structs is common, if the fields continue to diverge or additional profile commands are added, consider extracting a shared base struct or using#[command(flatten)]for common options.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.rs` around lines 80 - 107, InitArgs and ProfileAddArgs duplicate common CLI fields (name, operator, token_stdin, store_token_in_file, overwrite); extract those into a shared struct (e.g., ProfileOpts) and flatten it into both InitArgs and ProfileAddArgs using #[command(flatten)] so the common options are declared once. Update the definitions for InitArgs and ProfileAddArgs to remove the duplicated fields and reference the new shared struct (ensure symbol names like ProfileOpts, InitArgs, ProfileAddArgs and the field names operator/token_stdin/store_token_in_file/overwrite/name are used consistently), and run cargo build to confirm no arg parsing regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/command.rs`:
- Around line 235-240: When overwriting a profile in src/command.rs you rebuild
ProfileConfig but drop any existing credential; update the overwrite path so
ProfileConfig preserves the current credential instead of setting credential:
None — fetch the existing profile (if any) and set profile_config.credential =
existing_profile.credential.clone() or take() when constructing profile_config
(same change for the similar block at 242-257), and only replace credential when
the user provides a new token.
- Around line 815-820: The prompt helpers currently ignore a 0-byte read (EOF)
and treat it like empty input; in prompt_yes_no / prompt_required / prompt_line
replace the single-step read_line call with one that captures the returned usize
(bytes read) and map_err as before, then check if bytes == 0 and return an
aborted prompt error (e.g. return Err(CliError::aborted(...) or similar
project-specific variant) instead of treating it as empty/default); preserve
existing trimming/validation of `line` and `value` for non-EOF cases and apply
the same change to the other occurrence referenced in the comment.
- Around line 1002-1014: ProfileSetupResponse is emitting snake_case fields
(active_profile, config_path, next_steps) which breaks JSON consistency; add a
serde attribute to the ProfileSetupResponse struct (e.g., #[serde(rename_all =
"camelCase")]) so all serialized field names become camelCase to match other
responses, leaving existing field names and skip_serializing_if annotations
intact; update any tests or serialization consumers if they relied on snake_case
names.
- Around line 232-237: The setup currently assigns OutputFormat::Json into new
ProfileConfig when cli.global.output is None, causing implicit JSON to be
persisted; change the logic so that ProfileConfig.output is set only when
cli.global.output is Some(value) (i.e., persist explicit overrides), and keep a
separate local display variable (profile_output) computed as
cli.global.output.unwrap_or(OutputFormat::Json) for responses shown during
setup; update the code around resolve_setup_operator, profile_output, and
ProfileConfig construction so ProfileConfig.output is None unless an explicit
--output was provided.
In `@src/error.rs`:
- Around line 87-94: The ProfileAlreadyExists error should carry the command
context instead of hardcoding "profile add": change the
CliErrorKind::ProfileAlreadyExists variant to include a command: Option<String>,
update the constructor function profile_already_exists(profile: &str, format:
OutputFormat) -> Self to accept a command parameter (e.g.,
profile_already_exists(profile: &str, command: Option<String>, format:
OutputFormat) -> Self) and set the variant's command field accordingly, then
update all call sites that construct this error (both from the init flow and
profile add flow) to pass the appropriate command string (e.g.,
Some("init".to_owned()) or Some("profile add".to_owned())) or None as needed,
and finally adjust the error render logic that currently uses command:
Some("profile add".to_owned()) to read the command from the variant's command
field.
---
Nitpick comments:
In `@src/cli.rs`:
- Around line 80-107: InitArgs and ProfileAddArgs duplicate common CLI fields
(name, operator, token_stdin, store_token_in_file, overwrite); extract those
into a shared struct (e.g., ProfileOpts) and flatten it into both InitArgs and
ProfileAddArgs using #[command(flatten)] so the common options are declared
once. Update the definitions for InitArgs and ProfileAddArgs to remove the
duplicated fields and reference the new shared struct (ensure symbol names like
ProfileOpts, InitArgs, ProfileAddArgs and the field names
operator/token_stdin/store_token_in_file/overwrite/name are used consistently),
and run cargo build to confirm no arg parsing regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f315852-57bd-4f0a-acd4-f4fe3b00f1ad
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlREADME.mdsrc/cli.rssrc/command.rssrc/error.rstests/cli.rstests/profile.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/cli.rs
- Cargo.toml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec9370fc82
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03f993b672
ℹ️ 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".
| profile_config.credential = credential.clone(); | ||
|
|
||
| let mut config = loaded.config.clone(); | ||
| config.profiles.insert(profile_name.clone(), profile_config); |
There was a problem hiding this comment.
When an existing profile with a native credential is overwritten without entering a new token, the replacement config has credential: None, but auth resolution treats that as the default native key equal to the profile name. The old token therefore remains active for the overwritten profile (and may be sent to the new server), even though the profile was replaced without a credential; delete the previous credential or explicitly preserve only when requested.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not resolving this one yet. Current profile overwrite behavior intentionally preserves the existing credential unless a replacement token is provided; changing that would alter credential lifecycle semantics and likely needs an explicit --clear-credential/--preserve-credential product decision.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f19ecbaee5
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/auth.rs`:
- Line 184: The assertion currently checks config_root(&home).join("dev.token")
which doesn't match how credential_file_path(&home, "../dev") normalizes; update
the test to assert the normalized credential path or that the credentials
directory is absent: either replace the line with
assert!(!credential_file_path(&home, "../dev").exists()); or
assert!(!config_root(&home).join("apollo").join("credentials").exists()); so the
check aligns with credential_file_path and the expected location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 979127c9-292e-4474-9e3c-2ceb63d89b16
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.ymlCargo.tomlREADME.mdsrc/command.rssrc/config.rssrc/credential.rssrc/error.rssrc/http.rssrc/output.rssrc/redaction.rstests/auth.rstests/cli.rstests/openapi.rstests/profile.rstests/redaction.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/ci.yml
- tests/redaction.rs
- Cargo.toml
- src/redaction.rs
- src/output.rs
- tests/openapi.rs
- src/config.rs
- src/command.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40d13ddbae
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/command.rs`:
- Around line 46-52: The helper non_blank currently returns the original String
when non-empty, preserving leading/trailing whitespace; update non_blank to trim
the input and return Some(trimmed_string) (or None if trimmed is empty) so
resolve_setup_profile_name, resolve_setup_server, and resolve_setup_operator
(which reuse non_blank) will persist trimmed values (e.g., let t =
value.trim().to_string(); if t.is_empty() { None } else { Some(t) }).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d6391f8-aaca-4a01-9a3f-52c717a694ae
📒 Files selected for processing (5)
src/command.rssrc/config.rssrc/credential.rstests/auth.rstests/profile.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/auth.rs
- src/credential.rs
- tests/profile.rs
- src/config.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c847b1889c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8e43a6f61
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cc8f74787
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 497c017ead
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5bf9aaf42
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc5d56094a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 252be79bd7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd7cb17359
ℹ️ 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".
| if let Some(existing_name) = find_app_namespace(openapi, app_id, namespace_name)? { | ||
| return Ok(existing_name); |
There was a problem hiding this comment.
When namespace create --public is run for an AppNamespace that already exists as private, such as the default application namespace, this early return skips the only request that would carry isPublic: true and then creates the env/cluster namespace using the existing private AppNamespace name. The command can report success while no public namespace was created for other apps to associate with; inspect the existing AppNamespace's isPublic value and reject the mismatch instead of blindly reusing it.
Useful? React with 👍 / 👎.
| let token = fs::read_to_string(path).map_err(|error| error.to_string())?; | ||
| Ok(Some(Sensitive::new(token.trim().to_owned()))) |
There was a problem hiding this comment.
If a file-backed credential file is empty or whitespace-only, for example after a manual edit or a truncated overwrite, this still returns Some(Sensitive("")). auth status then reports the profile as authenticated and OpenAPI commands send a blank Authorization header instead of falling back to the normal authentication-required path, so the trimmed token should be filtered the same way environment and stdin tokens are.
Useful? React with 👍 / 👎.
Summary
Refs apolloconfig/apollo#5624.
Refs apolloconfig/apollo#5628.
Refs apolloconfig/apollo#5629.
Refs apolloconfig/apollo#5630.
Refs apolloconfig/apollo#5631.
Refs apolloconfig/apollo#5625.
Refs apolloconfig/apollo#5626.
This PR initializes the standalone Rust
apolloCLI repository and implements the first v0 OpenAPI-oriented slice:init,auth,profile,app,env,namespace,config,release, andapiapollo initapollo profile add [name], with--use,--overwrite,--token-stdin, and explicit file credential fallback support--profile,--server,--output json|table, and--yesprofile list/show/useAuthorizationheader model/openapi/v1/*commands for app/env/namespace/config/release workflowsapollo api <method> <path> [--body ...]config listandrelease listconfig setfallback from update to create when Apollo reports a missing itemScope Notes
The CLI is intentionally standalone and does not add code to
apolloconfig/apollo.This PR uses existing Apollo Portal OpenAPI endpoints only. It does not use deprecated Portal WebAPI endpoints for CLI runtime behavior and does not implement server-side schema changes, generated OpenAPI SDK bindings, MCP, or agent sessions. Local integration setup used Portal session endpoints only to create a temporary test app and Consumer token for smoke validation.
Validation
cargo fmt --checkcargo testcargo test --test openapicargo clippy --all-targets --all-features -- -D warningsgit diff --checkcargo run -- --helpcargo run -- namespace create --helpcargo run -- config apply --helpapollo profile add prod --server ... --operator ... --use --output json./mvnw -B -pl apollo-assembly -am -DskipTests packagein a local Apollo checkouthttp://127.0.0.1:8070usinggithub,database-discovery,authprofiles and H2, coveringenv list,app list,namespace list,config list,config set,config get,config diff,release create,release list, andapi get /openapi/v1/envsThe local pre-push hook also completed and reported no data security issues. The latest push emitted a non-blocking FaaS rule fetch warning before completing the scan.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores