[wip] puzzlectl: add credential provision subcommand#10
[wip] puzzlectl: add credential provision subcommand#10sallyom wants to merge 1 commit intoLobsterTrap:mainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new CLI subcommand variant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
sallyom
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/puzzlectl/src/main.rs (1)
1361-1363: Add client-sidebranch_idvalidation before provisioning call.For consistency with other branch-oriented commands and clearer CLI errors, validate
branch_idbefore invoking D-Bus.♻️ Proposed patch
CredentialAction::Provision { branch_id } => { + validate_branch_id(&branch_id)?; let result = client.provision_credentials(&branch_id).await?; match cli.output {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 1361 - 1363, Before calling client.provision_credentials in the CredentialAction::Provision arm, validate the provided branch_id on the client side (same validation used by other branch commands: e.g., parse/validate as BranchId::from_str or the existing validate_branch_id/parse_branch_id helper) and return a friendly CLI error if malformed or empty instead of invoking client.provision_credentials(&branch_id). Move the validation above the await call, and only call client.provision_credentials(&branch_id).await? when validation succeeds; ensure error handling uses the same cli.output/error formatting as the other branch-oriented commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/puzzlectl/src/main.rs`:
- Around line 1361-1363: Before calling client.provision_credentials in the
CredentialAction::Provision arm, validate the provided branch_id on the client
side (same validation used by other branch commands: e.g., parse/validate as
BranchId::from_str or the existing validate_branch_id/parse_branch_id helper)
and return a friendly CLI error if malformed or empty instead of invoking
client.provision_credentials(&branch_id). Move the validation above the await
call, and only call client.provision_credentials(&branch_id).await? when
validation succeeds; ensure error handling uses the same cli.output/error
formatting as the other branch-oriented commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7b1ae55-7b73-49be-b9df-dc76947e4e46
📒 Files selected for processing (1)
crates/puzzlectl/src/main.rs
There was a problem hiding this comment.
Pull request overview
Adds a new puzzlectl credential provision subcommand to trigger per-branch phantom-token credential provisioning via the existing D-Bus client API (verified in a podman machine session-bus setup).
Changes:
- Adds
CredentialAction::Provision { branch_id }to the CLI subcommands. - Wires the new action into the
credentialcommand handler and prints the D-Bus result as JSON or text.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| } | ||
| CredentialAction::Provision { branch_id } => { |
There was a problem hiding this comment.
CredentialAction::Provision passes branch_id straight to D-Bus without the validate_branch_id(...) check that other branch-id-taking commands use (see BranchAction/AgentAction). Please validate the branch id before calling client.provision_credentials(...) to fail fast and preserve the CLI’s path-traversal/format hardening pattern (M-ctl2).
| CredentialAction::Provision { branch_id } => { | |
| CredentialAction::Provision { branch_id } => { | |
| let branch_id = validate_branch_id(&branch_id)?; |
| /// Provision phantom-token credentials for a branch | ||
| Provision { | ||
| /// Branch ID to provision credentials for | ||
| branch_id: String, |
There was a problem hiding this comment.
The new Provision subcommand uses the argument name branch_id, while other subcommands that take a branch id consistently call the positional field id (e.g., BranchAction::{Inspect,Approve,...} and AgentAction::{Info,Kill}). Consider renaming this field to id (and updating the match arm) to keep the CLI surface consistent and reduce cognitive overhead.
| branch_id: String, | |
| id: String, |
sallyom
force-pushed
the
creds-provision
branch
2 times, most recently
from
c0e69f5 to
35343d7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| done < <( | ||
| python3 - "$PROVISION_RESULT" <<'PY' | ||
| import json | ||
| import sys |
There was a problem hiding this comment.
The new JSON parsing relies on python3, but this script runs with set -euo pipefail and currently does not verify that python3 exists. If python3 is missing (or fails), this can unexpectedly abort puzzle-podman even though credential provisioning is intended to be non-fatal. Consider guarding this block with a command -v python3 check and falling back to an empty env-var list (or using an already-available parser) when Python is unavailable.
| for env_var, phantom in payload.get("phantom_env_vars", {}).items(): | ||
| if env_var and phantom: | ||
| print(f"{env_var}={phantom}") |
There was a problem hiding this comment.
The embedded Python only catches JSON parse errors; if phantom_env_vars is present but not an object (or any other runtime error occurs), it will raise and emit a traceback to stderr. Since provisioning is best-effort here, it would be safer to wrap the iteration in a broader try/except (or validate the type before calling .items()) and exit 0 on unexpected shapes.
| for env_var, phantom in payload.get("phantom_env_vars", {}).items(): | |
| if env_var and phantom: | |
| print(f"{env_var}={phantom}") | |
| try: | |
| env_map = payload.get("phantom_env_vars", {}) | |
| # Ensure we only iterate if phantom_env_vars is a mapping; otherwise, exit quietly. | |
| if not isinstance(env_map, dict): | |
| sys.exit(0) | |
| for env_var, phantom in env_map.items(): | |
| if env_var and phantom: | |
| print(f"{env_var}={phantom}") | |
| except Exception: | |
| # Best-effort provisioning: on any unexpected error, exit successfully. | |
| sys.exit(0) |
| let domains = c | ||
| .get("target_domains") | ||
| .and_then(|v| v.as_array()) | ||
| .map(|domains| { | ||
| domains.iter().filter_map(|v| v.as_str()).collect::<Vec<_>>() | ||
| }) | ||
| .unwrap_or_default(); | ||
| (allowed_profiles.is_empty() | ||
| || allowed_profiles.iter().any(|p| *p == "*" || *p == profile)) | ||
| && domains.iter().any(|d| *d == domain) |
There was a problem hiding this comment.
The credential test filtering checks target_domains using exact string equality, but the proxy/credential logic supports glob patterns like *.openai.com (see domain_matches in puzzle-proxy). This can produce false negatives (e.g., api.openai.com won't match *.openai.com). Consider reusing the same domain-matching logic (domain_matches / StoredCredential::matches_domain) when deciding whether a credential matches the requested domain.
| (allowed_profiles.is_empty() | ||
| || allowed_profiles.iter().any(|p| *p == "*" || *p == profile)) |
There was a problem hiding this comment.
The profile filter treats an empty allowed_profiles list as matching all profiles (allowed_profiles.is_empty() || ...), but the runtime credential logic (StoredCredential::matches_profile) only matches when the list contains "*" or the exact profile. This can cause puzzlectl credential test to report matches that won't actually inject. Consider aligning the CLI semantics with the backend (either remove the is_empty() fast-path here, or ensure empty lists are normalized to "*" at creation time).
| (allowed_profiles.is_empty() | |
| || allowed_profiles.iter().any(|p| *p == "*" || *p == profile)) | |
| allowed_profiles | |
| .iter() | |
| .any(|p| *p == "*" || *p == profile) |
Signed-off-by: sallyom <somalley@redhat.com>
sallyom
force-pushed
the
creds-provision
branch
from
35343d7 to
6f12c3e
Compare
verified in a podman machine session-bus setup
Summary by CodeRabbit
Release Notes
provisionsubcommand to the credential management CLI tool, enabling users to provision credentials for a specified branch with support for JSON or plain text output formatting.