feat(cli): move secrets to OS keychain, update mach token metadata to github#172
Merged
RustMunkey merged 1 commit intomainfrom Mar 28, 2026
Merged
feat(cli): move secrets to OS keychain, update mach token metadata to github#172RustMunkey merged 1 commit intomainfrom
RustMunkey merged 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces keychain-backed secret storage for CLI configuration, updating how sensitive data (API keys, database URLs, signing keys) are persisted. It also updates token metadata image URLs and adds a Solana Token-2022 metadata update script. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Process
participant Config as config.rs
participant OS as OS Keychain
participant FS as TOML File
rect rgba(100, 150, 255, 0.5)
Note over CLI,FS: Configuration Load Flow
CLI->>Config: load(profile)
Config->>FS: read TOML (if exists)
FS-->>Config: TOML data
Config->>OS: kr_get("profile/api_key")
OS-->>Config: secret from keychain
Config->>OS: kr_get("profile/db_url")
OS-->>Config: secret from keychain
Config->>OS: kr_get("profile/node.signing_key")
OS-->>Config: secret from keychain
Config-->>CLI: merged Config with secrets
end
rect rgba(150, 200, 100, 0.5)
Note over CLI,FS: Configuration Save Flow
CLI->>Config: save(config, profile)
Config->>OS: kr_set("profile/api_key", value)
OS-->>Config: ✓
Config->>OS: kr_set("profile/db_url", value)
OS-->>Config: ✓
Config->>OS: kr_set("profile/node.signing_key", value)
OS-->>Config: ✓
Config->>FS: write TOML (secrets stripped)
FS-->>Config: ✓
Config-->>CLI: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🧹 Nitpick comments (4)
scripts/update-token-uri.ts (2)
45-50: Add error handling for keypair loading and documentANCHOR_WALLETin.env.example.Two concerns:
If the keypair file is missing or contains invalid JSON, the error will be cryptic (
ENOENTorSyntaxError). Consider wrapping in try-catch with a user-friendly message.The script uses
ANCHOR_WALLETbut.env.example(line 44) documentsSOLANA_WALLET_PATH. Consider aligning the naming or documenting both.Suggested error handling
function loadKeypair(): Keypair { const walletPath = process.env.ANCHOR_WALLET ?? path.join(os.homedir(), ".config", "solana", "id.json"); - const bytes = JSON.parse(fs.readFileSync(walletPath, "utf-8")) as number[]; - return Keypair.fromSecretKey(Uint8Array.from(bytes)); + try { + const bytes = JSON.parse(fs.readFileSync(walletPath, "utf-8")) as number[]; + return Keypair.fromSecretKey(Uint8Array.from(bytes)); + } catch (err) { + console.error(`Failed to load keypair from ${walletPath}`); + throw err; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/update-token-uri.ts` around lines 45 - 50, The loadKeypair function should catch filesystem and JSON errors and give a clear, actionable message: wrap the fs.readFileSync/JSON.parse and Keypair.fromSecretKey calls in a try-catch inside loadKeypair, validate that the parsed value is an array of numbers of expected length before calling Keypair.fromSecretKey, and throw or log a user-friendly error mentioning the environment variable used (process.env.ANCHOR_WALLET) and the resolved path; also reconcile the env var names by either changing the code to read process.env.SOLANA_WALLET_PATH or updating .env.example to document ANCHOR_WALLET (or include both) so the variable name is consistent and discoverable.
52-89: Consider adding a confirmation prompt for mainnet-beta.This script updates on-chain state. For mainnet operations, a confirmation prompt would help prevent accidental execution.
Optional: Add mainnet confirmation
+import readline from "node:readline"; + +async function confirmMainnet(): Promise<void> { + if (CLUSTER !== "mainnet-beta") return; + const rl = readline.createInterface({ input: process.stdin, output: process.stdout }); + return new Promise((resolve, reject) => { + rl.question("⚠️ You are about to update mainnet. Continue? (yes/no): ", (answer) => { + rl.close(); + if (answer.toLowerCase() === "yes") resolve(); + else reject(new Error("Aborted by user")); + }); + }); +} + async function main() { // ... existing validation ... + await confirmMainnet(); console.log("Sending updateField transaction...");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/update-token-uri.ts` around lines 52 - 89, Add a safety confirmation step in main() that detects when CLUSTER === "mainnet-beta" and prompts the user to explicitly confirm (e.g., type "yes" or pass a force flag) before proceeding with creating/sending the updateField transaction; place the check after printing the summary (Mint/Authority/New URI) and before the createUpdateFieldInstruction/sendAndConfirmTransaction call, and if the user does not confirm, exit the process without sending the transaction. Ensure the prompt references the mint (MACH_MINT_ADDRESS) and NEW_METADATA_URI and allow an environment override (like a FORCE or --yes) for CI use.assets/mach-token-metadata.json (1)
5-5: Consider syncing the URI insetup-mach-token.tsto avoid drift.The metadata now points to GitHub raw URLs, but per the context in
scripts/setup-mach-token.ts:47, theMACH_TOKEN.uriconstant still references the old Arweave URL. Ifsetup-mach-token.tsis re-run (e.g., for a fresh devnet deployment), it will initialize the mint with the Arweave URI, creating inconsistency.Additionally, using
mainbranch in the raw URL means the image is mutable—any push to main could change it. If immutability is desired, consider pinning to a commit SHA or tag (e.g.,https://raw.githubusercontent.com/RustMunkey/maschina/<commit-sha>/assets/mach-token.png).Also applies to: 11-11
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/mach-token-metadata.json` at line 5, The metadata image URI in assets/mach-token-metadata.json has been updated to a GitHub raw URL but the constant MACH_TOKEN.uri in scripts/setup-mach-token.ts still points to the old Arweave URL; update MACH_TOKEN.uri to the same GitHub raw URL used in assets/mach-token-metadata.json (or better, replace both with a commit-SHA–pinned raw.githubusercontent URL) so re-running setup-mach-token.ts mints a token whose on-chain URI matches the metadata file and is immutable if desired.packages/cli/src/config.rs (1)
166-173: Orphaned keychain entries when providers are removed.When a provider is removed from
model_providers, its keychain entry (provider/{name}) is never deleted. These orphaned secrets accumulate in the keychain indefinitely.This is a minor concern since it doesn't affect functionality. Consider documenting this as a known limitation, or tracking previous provider names to clean them up during
save().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/config.rs` around lines 166 - 173, The save() logic currently only sets/deletes keys for providers present in cfg.model_providers, leaving orphaned keychain entries for providers that were removed; update the save() flow (the function that calls kr_set/kr_del) to also remove stale keys: either load the previous provider list and remove any provider/{old_name} not in cfg.model_providers, or query/list existing keychain keys with prefix "provider/" and call kr_del for any name not present in the current cfg.model_providers (use provider.name to compare); ensure this cleanup runs alongside the existing kr_set/kr_del operations so orphaned secrets are removed when providers are deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/config.rs`:
- Around line 156-160: The match for cfg.api_key currently leaves Some("")
untouched; change it so any empty string is treated like None and deletes the
keychain entry: use match &cfg.api_key { Some(v) if !v.is_empty() =>
kr_set(profile, "api_key", v), _ => kr_del(profile, "api_key"), } (i.e., make
the fallback arm call kr_del instead of no-op). Apply the same pattern to the
similar matches for db_url, provider.api_key, and node.signing_key so that
Some("") also triggers kr_del for those keys.
- Around line 77-81: kr_set and kr_del currently swallow all keyring errors
which can hide secret write/delete failures; change both functions (kr_set and
kr_del) to return Result<(), E> (or a suitable keyring::Error) instead of unit,
propagate the error from Entry::new(...) and entry.set_password(...) /
entry.delete_password(...), and update callers to handle or log the Result so
failures are visible; keep the existing KEYRING_SERVICE and kr_key(profile,
field) usage but propagate errors from those calls rather than discarding them.
In `@scripts/update-token-uri.ts`:
- Around line 32-43: The getRpcUrl function currently treats any non-"localnet"
value as devnet, so validate CLUSTER (used in getRpcUrl) against allowed values
("localnet", "devnet", "mainnet-beta"); if the value is unrecognized, either
throw a clear Error or emit a warning (including the actual CLUSTER value)
before falling back, and then proceed to construct the RPC URL using the
existing HELIUS_API_KEY logic; update getRpcUrl to perform this explicit check
and message so unexpected SOLANA_CLUSTER values (e.g., "testnet") are surfaced
instead of silently using devnet.
---
Nitpick comments:
In `@assets/mach-token-metadata.json`:
- Line 5: The metadata image URI in assets/mach-token-metadata.json has been
updated to a GitHub raw URL but the constant MACH_TOKEN.uri in
scripts/setup-mach-token.ts still points to the old Arweave URL; update
MACH_TOKEN.uri to the same GitHub raw URL used in
assets/mach-token-metadata.json (or better, replace both with a
commit-SHA–pinned raw.githubusercontent URL) so re-running setup-mach-token.ts
mints a token whose on-chain URI matches the metadata file and is immutable if
desired.
In `@packages/cli/src/config.rs`:
- Around line 166-173: The save() logic currently only sets/deletes keys for
providers present in cfg.model_providers, leaving orphaned keychain entries for
providers that were removed; update the save() flow (the function that calls
kr_set/kr_del) to also remove stale keys: either load the previous provider list
and remove any provider/{old_name} not in cfg.model_providers, or query/list
existing keychain keys with prefix "provider/" and call kr_del for any name not
present in the current cfg.model_providers (use provider.name to compare);
ensure this cleanup runs alongside the existing kr_set/kr_del operations so
orphaned secrets are removed when providers are deleted.
In `@scripts/update-token-uri.ts`:
- Around line 45-50: The loadKeypair function should catch filesystem and JSON
errors and give a clear, actionable message: wrap the fs.readFileSync/JSON.parse
and Keypair.fromSecretKey calls in a try-catch inside loadKeypair, validate that
the parsed value is an array of numbers of expected length before calling
Keypair.fromSecretKey, and throw or log a user-friendly error mentioning the
environment variable used (process.env.ANCHOR_WALLET) and the resolved path;
also reconcile the env var names by either changing the code to read
process.env.SOLANA_WALLET_PATH or updating .env.example to document
ANCHOR_WALLET (or include both) so the variable name is consistent and
discoverable.
- Around line 52-89: Add a safety confirmation step in main() that detects when
CLUSTER === "mainnet-beta" and prompts the user to explicitly confirm (e.g.,
type "yes" or pass a force flag) before proceeding with creating/sending the
updateField transaction; place the check after printing the summary
(Mint/Authority/New URI) and before the
createUpdateFieldInstruction/sendAndConfirmTransaction call, and if the user
does not confirm, exit the process without sending the transaction. Ensure the
prompt references the mint (MACH_MINT_ADDRESS) and NEW_METADATA_URI and allow an
environment override (like a FORCE or --yes) for CI use.
🪄 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: ecc89ad0-9b9a-491e-a8c2-7bebe1912acc
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockassets/card-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
assets/mach-token-metadata.jsonpackages/cli/Cargo.tomlpackages/cli/src/commands/node/join.rspackages/cli/src/config.rsscripts/update-token-uri.ts
Comment on lines
+77
to
+81
| fn kr_set(profile: &str, field: &str, value: &str) { | ||
| if let Ok(entry) = Entry::new(KEYRING_SERVICE, &kr_key(profile, field)) { | ||
| let _ = entry.set_password(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent failures in keychain writes may cause undetected secret loss.
Both kr_set and kr_del silently discard errors. If the keychain service is unavailable or the operation fails, the user won't know their secrets weren't persisted, potentially causing authentication failures on the next load().
Consider logging a warning or returning a Result so callers can decide how to handle failures.
🛡️ Proposed fix to propagate errors
-fn kr_set(profile: &str, field: &str, value: &str) {
- if let Ok(entry) = Entry::new(KEYRING_SERVICE, &kr_key(profile, field)) {
- let _ = entry.set_password(value);
- }
+fn kr_set(profile: &str, field: &str, value: &str) -> Result<()> {
+ let entry = Entry::new(KEYRING_SERVICE, &kr_key(profile, field))
+ .context("failed to create keychain entry")?;
+ entry.set_password(value)
+ .context("failed to write secret to keychain")?;
+ Ok(())
}
-fn kr_del(profile: &str, field: &str) {
- if let Ok(entry) = Entry::new(KEYRING_SERVICE, &kr_key(profile, field)) {
- let _ = entry.delete_credential();
- }
+fn kr_del(profile: &str, field: &str) -> Result<()> {
+ let entry = Entry::new(KEYRING_SERVICE, &kr_key(profile, field))
+ .context("failed to create keychain entry")?;
+ // Ignore "not found" errors on delete
+ let _ = entry.delete_credential();
+ Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/config.rs` around lines 77 - 81, kr_set and kr_del currently
swallow all keyring errors which can hide secret write/delete failures; change
both functions (kr_set and kr_del) to return Result<(), E> (or a suitable
keyring::Error) instead of unit, propagate the error from Entry::new(...) and
entry.set_password(...) / entry.delete_password(...), and update callers to
handle or log the Result so failures are visible; keep the existing
KEYRING_SERVICE and kr_key(profile, field) usage but propagate errors from those
calls rather than discarding them.
Comment on lines
+156
to
+160
| match &cfg.api_key { | ||
| Some(v) if !v.is_empty() => kr_set(profile, "api_key", v), | ||
| None => kr_del(profile, "api_key"), | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
Empty string values don't delete stale keychain entries.
The _ => {} arm catches Some(""), leaving any existing keychain entry intact. If a user clears a field to an empty string (rather than None), the old secret remains in the keychain and will be loaded on next load().
Consider treating empty strings the same as None:
♻️ Proposed fix
match &cfg.api_key {
Some(v) if !v.is_empty() => kr_set(profile, "api_key", v),
- None => kr_del(profile, "api_key"),
- _ => {}
+ _ => kr_del(profile, "api_key"),
}Apply the same pattern to db_url, provider.api_key, and node.signing_key matches.
📝 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
| match &cfg.api_key { | |
| Some(v) if !v.is_empty() => kr_set(profile, "api_key", v), | |
| None => kr_del(profile, "api_key"), | |
| _ => {} | |
| } | |
| match &cfg.api_key { | |
| Some(v) if !v.is_empty() => kr_set(profile, "api_key", v), | |
| _ => kr_del(profile, "api_key"), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/config.rs` around lines 156 - 160, The match for cfg.api_key
currently leaves Some("") untouched; change it so any empty string is treated
like None and deletes the keychain entry: use match &cfg.api_key { Some(v) if
!v.is_empty() => kr_set(profile, "api_key", v), _ => kr_del(profile, "api_key"),
} (i.e., make the fallback arm call kr_del instead of no-op). Apply the same
pattern to the similar matches for db_url, provider.api_key, and
node.signing_key so that Some("") also triggers kr_del for those keys.
Comment on lines
+32
to
+43
| function getRpcUrl(): string { | ||
| if (CLUSTER === "localnet") return "http://127.0.0.1:8899"; | ||
| const apiKey = process.env.HELIUS_API_KEY; | ||
| if (apiKey) { | ||
| return CLUSTER === "mainnet-beta" | ||
| ? `https://mainnet.helius-rpc.com/?api-key=${apiKey}` | ||
| : `https://devnet.helius-rpc.com/?api-key=${apiKey}`; | ||
| } | ||
| return CLUSTER === "mainnet-beta" | ||
| ? "https://api.mainnet-beta.solana.com" | ||
| : "https://api.devnet.solana.com"; | ||
| } |
There was a problem hiding this comment.
Unrecognized cluster values silently fall back to devnet.
If SOLANA_CLUSTER is set to an unrecognized value (e.g., "testnet"), the function silently falls back to devnet RPC, which could lead to unexpected behavior. Consider validating the cluster value or logging a warning.
Suggested improvement
+const VALID_CLUSTERS = ["localnet", "devnet", "mainnet-beta"] as const;
+type Cluster = (typeof VALID_CLUSTERS)[number];
+
function getRpcUrl(): string {
+ if (!VALID_CLUSTERS.includes(CLUSTER as Cluster)) {
+ console.warn(`Unknown cluster "${CLUSTER}", defaulting to devnet`);
+ }
if (CLUSTER === "localnet") return "http://127.0.0.1:8899";📝 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
| function getRpcUrl(): string { | |
| if (CLUSTER === "localnet") return "http://127.0.0.1:8899"; | |
| const apiKey = process.env.HELIUS_API_KEY; | |
| if (apiKey) { | |
| return CLUSTER === "mainnet-beta" | |
| ? `https://mainnet.helius-rpc.com/?api-key=${apiKey}` | |
| : `https://devnet.helius-rpc.com/?api-key=${apiKey}`; | |
| } | |
| return CLUSTER === "mainnet-beta" | |
| ? "https://api.mainnet-beta.solana.com" | |
| : "https://api.devnet.solana.com"; | |
| } | |
| const VALID_CLUSTERS = ["localnet", "devnet", "mainnet-beta"] as const; | |
| type Cluster = (typeof VALID_CLUSTERS)[number]; | |
| function getRpcUrl(): string { | |
| if (!VALID_CLUSTERS.includes(CLUSTER as Cluster)) { | |
| console.warn(`Unknown cluster "${CLUSTER}", defaulting to devnet`); | |
| } | |
| if (CLUSTER === "localnet") return "http://127.0.0.1:8899"; | |
| const apiKey = process.env.HELIUS_API_KEY; | |
| if (apiKey) { | |
| return CLUSTER === "mainnet-beta" | |
| ? `https://mainnet.helius-rpc.com/?api-key=${apiKey}` | |
| : `https://devnet.helius-rpc.com/?api-key=${apiKey}`; | |
| } | |
| return CLUSTER === "mainnet-beta" | |
| ? "https://api.mainnet-beta.solana.com" | |
| : "https://api.devnet.solana.com"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/update-token-uri.ts` around lines 32 - 43, The getRpcUrl function
currently treats any non-"localnet" value as devnet, so validate CLUSTER (used
in getRpcUrl) against allowed values ("localnet", "devnet", "mainnet-beta"); if
the value is unrecognized, either throw a clear Error or emit a warning
(including the actual CLUSTER value) before falling back, and then proceed to
construct the RPC URL using the existing HELIUS_API_KEY logic; update getRpcUrl
to perform this explicit check and message so unexpected SOLANA_CLUSTER values
(e.g., "testnet") are surfaced instead of silently using devnet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores