Conversation
This is allow the clients to create an identity and save in the OS secure store.
Added following commands, - `account` - shows the account details from toml - `account create <nickname>` - nickname optional - `account set-nickname` - set nickname to the root account The root-user details are stored under config.toml under config dir
📝 WalkthroughWalkthroughAdds identity and account management across two crates. tilekit gains an accounts module that generates Ed25519 keys, derives did:key DIDs, and stores private keys in OS keyring. tiles gains CLI account commands, a utils/accounts module for root-user management persisted in a TOML config, and config utilities (get_or_create_config, save_config) to manage config.toml. New dependencies (ucan, ed25519-dalek, keyring, toml) are added; minor comment removals and a removed directory preflight remain. Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as tiles CLI
participant Cmd as commands/mod.rs
participant Config as utils/config.rs
participant Accounts as utils/accounts.rs
participant TileKit as tilekit/accounts.rs
participant Keyring as OS Keyring
participant File as config.toml
User->>CLI: tiles account create [--nickname]
CLI->>Cmd: run_account_commands(account_args)
Cmd->>Config: get_or_create_config()
Config->>File: read/initialize config.toml
File-->>Config: config Table
Config-->>Cmd: config Table
Cmd->>Accounts: create_root_account(config, nickname)
Accounts->>Accounts: check root-user presence
alt missing
Accounts->>TileKit: create_identity("tiles")
TileKit->>Keyring: generate key & store secret
Keyring-->>TileKit: success
TileKit-->>Accounts: DID string
Accounts->>Accounts: build root-user table
end
Accounts-->>Cmd: updated root-user table
Cmd->>Config: save_config(updated Table)
Config->>File: write config.toml
File-->>Config: success
Cmd->>User: print root account details
sequenceDiagram
actor User
participant CLI as tiles CLI
participant Cmd as commands/mod.rs
participant Config as utils/config.rs
participant Accounts as utils/accounts.rs
participant File as config.toml
User->>CLI: tiles account set-nickname <name>
CLI->>Cmd: run_account_commands(account_args)
Cmd->>Config: get_or_create_config()
Config->>File: read config.toml
File-->>Config: config Table
Config-->>Cmd: config Table
Cmd->>Accounts: set_nickname(config, name)
alt root exists
Accounts->>Accounts: update nickname in table
Accounts-->>Cmd: updated table
Cmd->>Config: save_config(updated Table)
Config->>File: write config.toml
File-->>Config: success
Cmd->>User: print updated details
else missing
Accounts-->>Cmd: error: no root account
Cmd->>User: show guidance message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 11
🧹 Nitpick comments (7)
tiles/src/main.rs (1)
95-110:AccountArgs/AccountCommandsdefined inmain.rsand re-imported intocommands/mod.rsviacrate::— inverts typical dependency direction.Command argument types should live in
commands/mod.rs(or a dedicatedargs.rs), not in the binary root. Currentlycommands/mod.rsreaches back intocrate::for these types, coupling the command handler to the top-level binary entry point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/main.rs` around lines 95 - 110, AccountArgs and AccountCommands are defined in main.rs but imported back into commands/mod.rs, inverting the dependency; move these CLI types into the commands module (or a new args.rs) so command handlers import them rather than main.rs exporting them. Refactor by cutting the structs/enums AccountArgs and AccountCommands out of main.rs and placing them into commands/mod.rs (or commands/args.rs) and update main.rs to use crate::commands::AccountArgs; ensure the #[derive(Debug, Args)] and #[derive(Debug, Subcommand)] attributes remain intact and update any references in commands/mod.rs to use the new local definitions.tiles/src/utils/config.rs (2)
155-155: Remove dead commented-out code.
// pub fn save_config(config: &Table)is a stale declaration fragment that should be deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/config.rs` at line 155, Remove the dead commented-out declaration fragment "// pub fn save_config(config: &Table)" from the source; locate the commented line in the tiles::config utilities (the stray save_config declaration) and delete it so no stale commented code remains.
126-145:get_or_create_confighardcodesDefaultProvider— it cannot be tested in isolation.There is a
ConfigProvidertrait with aMockProviderin the test suite, yetget_or_create_configandsave_configboth bypass it. The unit test at line 163 therefore writes to the real dev filesystem (.tiles_dev/tiles/config.toml). Accept a&dyn ConfigProviderparameter (or make it generic) so tests can inject a temp-dir-backed provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/config.rs` around lines 126 - 145, The function get_or_create_config currently uses DefaultProvider directly; change its signature to accept a ConfigProvider (e.g., fn get_or_create_config(provider: &dyn ConfigProvider) -> Result<Table> or make it generic over P: ConfigProvider) and replace all uses of DefaultProvider.get_config_dir() with provider.get_config_dir(); do the same change for save_config so both functions use the injected provider, update call sites/tests to pass MockProvider or a temp-dir provider, and keep all existing error handling and return types intact.tiles/src/commands/mod.rs (1)
61-61:format!("tiles account create")on a string literal is unnecessary.
format!with no interpolation just allocates aStringfrom a literal. Use"tiles account create".yellow()directly.♻️ Proposed fix
- format!("tiles account create").yellow() + "tiles account create".yellow()Also applies to: 81-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/commands/mod.rs` at line 61, The code uses format!("tiles account create") unnecessarily; replace the format! call with a string literal and call .yellow() directly (e.g., change format!("tiles account create").yellow() to "tiles account create".yellow()); do the same for the other identical occurrence (format!("tiles account create") at the other location) and if any surrounding API expects an owned String, convert with .to_string() after coloring (e.g., "tiles account create".yellow().to_string()).tiles/src/utils/accounts.rs (2)
51-62: Redundant re-insertion ofidon line 58.Line 58 inserts
"id"back into the table with the same value just read from it on line 54. This is a no-op and can be removed.} else { - root_user_table.insert("id".to_owned(), toml::Value::String(did.to_owned())); root_user_table.insert("nickname".to_owned(), toml::Value::String(nickname)); Ok(root_user_table) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/accounts.rs` around lines 51 - 62, In set_nickname remove the redundant re-insertion of "id" (you read did from root_user_table via get("id") and then insert it back) and only insert the new "nickname" into root_user_table before returning; update the function body in set_nickname to stop reinserting toml::Value::String(did.to_owned()) and keep the logic that validates did is non-empty, inserts the nickname, and returns Ok(root_user_table).
1-2: Multiple TODO comments for missing documentation.There are four
TODO: add docs/TODO: Needed docsscattered across the public API (get_root_user_details,create_root_account,save_root_account,set_nickname,create_root_user). Consider tracking these in an issue so they don't stay indefinitely.Would you like me to open an issue to track adding documentation for these public functions?
Also applies to: 12-12, 28-28, 41-41, 50-50, 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/accounts.rs` around lines 1 - 2, Replace the "TODO: add docs" markers in the public API with proper Rust doc comments: add /// docstrings for get_root_user_details, create_root_account, save_root_account, set_nickname, create_root_user (and any other public fns mentioned) that describe purpose, arguments, return values, possible errors, and examples where relevant; update function signatures' docs to mention side effects (e.g., DB writes) and error types, and open a tracking issue to cover any remaining undocumented public functions so they aren't forgotten.tilekit/src/accounts.rs (1)
7-8: Type aliasesDIDandIdentityare currently only used internally.
DIDis not used outside of definingIdentity, andIdentityis the actual return type. Consider either makingIdentitya newtype wrapper (for type safety) or simplifying to justpub type Identity = String;and removing the intermediateDIDalias. As a type alias overString, callers can't distinguish anIdentityfrom any otherStringat compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tilekit/src/accounts.rs` around lines 7 - 8, The DID alias is redundant and unsafe as a plain String alias; either remove DID and make Identity a public type alias (pub type Identity = String) or, preferably, replace Identity with a newtype struct (pub struct Identity(String)) to gain compile-time distinction; update all usages that currently reference DID or Identity to the chosen form, add needed impls (From<String>, AsRef<str>, Debug/Clone/PartialEq/Eq/Serialize/Deserialize as appropriate) and adjust function return types and pattern matches to construct/extract the wrapped String via Identity(...) or .0 if you choose the newtype approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tilekit/Cargo.toml`:
- Line 17: The Cargo.toml currently pins the keyring crate with only the
"apple-native" feature, which causes non-macOS builds to fall back to the
ephemeral mock store; update the keyring dependency so platform-appropriate
native backends are enabled (e.g., add "linux-native" and/or "windows-native" to
the feature list) or replace the single dependency entry with
platform-conditional dependencies using [target.'cfg(target_os =
"linux")'.dependencies] and [target.'cfg(target_os = "windows")'.dependencies]
to enable the proper native features instead of only "apple-native" for the
keyring crate.
- Line 15: The ucan dependency in Cargo.toml uses a floating git branch ("branch
= \"main\"") which is non-reproducible and prevents crates.io publishing; update
the ucan entry to pin a specific git revision by replacing the branch key with a
fixed "rev" (commit SHA) or switch to a crates.io-released version, e.g. change
the ucan dependency line to use rev = "<commit-sha>" (or a published version) so
builds are reproducible and publishing is allowed.
In `@tiles/Cargo.toml`:
- Line 21: The Cargo dependency for the keyring crate currently only enables
"apple-native", which causes the crate to fall back to a non-persistent mock
store on Linux/Windows CI; update the keyring dependency declaration (the
keyring = { ... } entry) to enable platform-specific backends (e.g., add
"linux-native" or "secret-service" and "windows-native") so CI uses real
persistent stores, or alternatively refactor the keyring usage behind an
interface/trait so tests can inject a deterministic mock; adjust Cargo.toml
accordingly and update any initialization code that assumes persistence to use
the chosen backend or the injected trait implementation.
- Line 18: The Cargo.toml dependency declaration for the toml crate is invalid
because it specifies toml = "1.0.3" which doesn't exist; update the toml entry
in Cargo.toml (the toml dependency line) to a valid released version such as
toml = "0.9.10" (or toml = "^0.8.0" if you need wider MSRV compatibility) and
run cargo update/cargo build to verify dependency resolution succeeds.
In `@tiles/src/commands/mod.rs`:
- Line 51: The code currently uses multiple chained unwraps on TOML lookups
(e.g., root_user_config.get("id").unwrap().as_str().unwrap()) which will panic
on missing keys or wrong types; update each lookup (id, name, email, avatar,
etc.) to safely convert with something like
root_user_config.get("<key>").and_then(|v| v.as_str()).ok_or_else(||
anyhow::anyhow!("missing or invalid [root-user].<key>")) so the function returns
an informative anyhow::Error instead of panicking; apply this pattern to the
calls referenced (the get("id"), get("name"), get("email"), get("avatar")
usages) and propagate the Result upward.
- Around line 71-74: The match arm in run_account_commands currently swallows
errors from set_nickname by printing and continuing, causing
run_account_commands to return Ok(()) on failure; change the Err branch that now
does println!("Failed to set nickname due to {}", err) to propagate the error
instead—either return Err(err) (or Err(err.into()) if the function's error type
differs) or use the ? operator where appropriate so run_account_commands fails
when set_nickname fails; update the handling in the Err(err) arm of the match
around set_nickname to return the error rather than printing and continuing.
- Line 54: The println! call prints uncolored text because .green() is applied
to the unit value; update the call so the string itself is colored before
printing. Replace the current println!("Root account has been created with id:
{}", id).green() with a call that formats the message and calls .green() on the
resulting string (e.g., println!("{}", format!("Root account has been created
with id: {}", id).green())), and ensure the Colorize trait from the colored
crate (or whichever coloring crate the project uses) is in scope so the .green()
method resolves.
In `@tiles/src/utils/accounts.rs`:
- Around line 17-25: get_root_account (and similarly create_root_account and
set_nickname) currently panics by calling unwrap() on
config.get(ROOT_USER_CONFIG_KEY) and on toml value coercions like as_table() and
as_str(); change these functions to return Result<..., anyhow::Error> and
replace unwraps with ? while adding anyhow::Context for each fallible step
(e.g., when retrieving ROOT_USER_CONFIG_KEY, converting to a table, and reading
string values) so missing keys or wrong types produce propagated, contextual
errors rather than panics.
- Around line 65-81: In create_root_user, simplify the create_identity match and
nickname handling: replace the match on create_identity("tiles") and the
Err(err) => Err(err) pattern by using the try operator (let did =
create_identity("tiles")?;), and replace the if
nickname.is_some()/nickname.unwrap() block with an if let Some(n) = nickname {
root_user_table.insert("nickname".to_owned(), toml::Value::String(n)); } so you
avoid unwrap and redundant matching while keeping the rest of root_user_table
population unchanged.
In `@tiles/src/utils/config.rs`:
- Around line 147-153: The save_config function currently writes config.toml
in-place causing corruption on crash; change save_config (and use
DefaultProvider.get_config_dir()) to write to a sibling temp file (e.g.,
config.toml.tmp or a uniquely named temp in the same directory), fs::write the
temp file, fs::rename the temp to "config.toml" to atomically replace the
original, and propagate errors as before; ensure the temp file lives on the same
filesystem/dir as config.toml so fs::rename is atomic and remove/cleanup the
temp on error.
In `@tiles/tests/config.rs`:
- Line 66: The file tiles/tests/config.rs contains an extraneous trailing blank
line at the end of the file which causes `cargo fmt --all -- --check` to fail;
open tiles/tests/config.rs and remove the final empty newline so the file ends
with the last line of code (no extra blank line), then re-run `cargo fmt` to
verify formatting passes.
---
Nitpick comments:
In `@tilekit/src/accounts.rs`:
- Around line 7-8: The DID alias is redundant and unsafe as a plain String
alias; either remove DID and make Identity a public type alias (pub type
Identity = String) or, preferably, replace Identity with a newtype struct (pub
struct Identity(String)) to gain compile-time distinction; update all usages
that currently reference DID or Identity to the chosen form, add needed impls
(From<String>, AsRef<str>, Debug/Clone/PartialEq/Eq/Serialize/Deserialize as
appropriate) and adjust function return types and pattern matches to
construct/extract the wrapped String via Identity(...) or .0 if you choose the
newtype approach.
In `@tiles/src/commands/mod.rs`:
- Line 61: The code uses format!("tiles account create") unnecessarily; replace
the format! call with a string literal and call .yellow() directly (e.g., change
format!("tiles account create").yellow() to "tiles account create".yellow()); do
the same for the other identical occurrence (format!("tiles account create") at
the other location) and if any surrounding API expects an owned String, convert
with .to_string() after coloring (e.g., "tiles account
create".yellow().to_string()).
In `@tiles/src/main.rs`:
- Around line 95-110: AccountArgs and AccountCommands are defined in main.rs but
imported back into commands/mod.rs, inverting the dependency; move these CLI
types into the commands module (or a new args.rs) so command handlers import
them rather than main.rs exporting them. Refactor by cutting the structs/enums
AccountArgs and AccountCommands out of main.rs and placing them into
commands/mod.rs (or commands/args.rs) and update main.rs to use
crate::commands::AccountArgs; ensure the #[derive(Debug, Args)] and
#[derive(Debug, Subcommand)] attributes remain intact and update any references
in commands/mod.rs to use the new local definitions.
In `@tiles/src/utils/accounts.rs`:
- Around line 51-62: In set_nickname remove the redundant re-insertion of "id"
(you read did from root_user_table via get("id") and then insert it back) and
only insert the new "nickname" into root_user_table before returning; update the
function body in set_nickname to stop reinserting
toml::Value::String(did.to_owned()) and keep the logic that validates did is
non-empty, inserts the nickname, and returns Ok(root_user_table).
- Around line 1-2: Replace the "TODO: add docs" markers in the public API with
proper Rust doc comments: add /// docstrings for get_root_user_details,
create_root_account, save_root_account, set_nickname, create_root_user (and any
other public fns mentioned) that describe purpose, arguments, return values,
possible errors, and examples where relevant; update function signatures' docs
to mention side effects (e.g., DB writes) and error types, and open a tracking
issue to cover any remaining undocumented public functions so they aren't
forgotten.
In `@tiles/src/utils/config.rs`:
- Line 155: Remove the dead commented-out declaration fragment "// pub fn
save_config(config: &Table)" from the source; locate the commented line in the
tiles::config utilities (the stray save_config declaration) and delete it so no
stale commented code remains.
- Around line 126-145: The function get_or_create_config currently uses
DefaultProvider directly; change its signature to accept a ConfigProvider (e.g.,
fn get_or_create_config(provider: &dyn ConfigProvider) -> Result<Table> or make
it generic over P: ConfigProvider) and replace all uses of
DefaultProvider.get_config_dir() with provider.get_config_dir(); do the same
change for save_config so both functions use the injected provider, update call
sites/tests to pass MockProvider or a temp-dir provider, and keep all existing
error handling and return types intact.
Comment on lines
+147
to
+153
| pub fn save_config(config: &Table) -> Result<()> { | ||
| let tiles_config_dir = DefaultProvider.get_config_dir()?; | ||
| let config_toml_path = tiles_config_dir.join("config.toml"); | ||
|
|
||
| fs::write(config_toml_path, config.to_string())?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
save_config is not atomic — a crash mid-write corrupts config.toml.
fs::write truncates and rewrites the file in-place. A crash between truncation and completion leaves a partial file, silently destroying the user's identity data. Use a sibling temp file + fs::rename for atomicity.
🛡️ Proposed fix
pub fn save_config(config: &Table) -> Result<()> {
let tiles_config_dir = DefaultProvider.get_config_dir()?;
let config_toml_path = tiles_config_dir.join("config.toml");
-
- fs::write(config_toml_path, config.to_string())?;
+ let tmp_path = tiles_config_dir.join("config.toml.tmp");
+ fs::write(&tmp_path, config.to_string())?;
+ fs::rename(&tmp_path, &config_toml_path)?;
Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tiles/src/utils/config.rs` around lines 147 - 153, The save_config function
currently writes config.toml in-place causing corruption on crash; change
save_config (and use DefaultProvider.get_config_dir()) to write to a sibling
temp file (e.g., config.toml.tmp or a uniquely named temp in the same
directory), fs::write the temp file, fs::rename the temp to "config.toml" to
atomically replace the original, and propagate errors as before; ensure the temp
file lives on the same filesystem/dir as config.toml so fs::rename is atomic and
remove/cleanup the temp on error.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tiles/src/commands/mod.rs (1)
55-61:format_args!insideprintln!is unnecessary — use a plain format string.
println!("{}", format_args!(...))works but adds indirection with no benefit. The same result with coloring (consistent with the rest of the command output) is cleaner:♻️ Proposed simplification
- println!( - "{}", - format_args!( - "Root account has been created with id: {}", - root_user_config.id - ) - ) + println!( + "{}", + format!("Root account has been created with id: {}", root_user_config.id).green() + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/commands/mod.rs` around lines 55 - 61, Replace the indirect println!("{}", format_args!(...)) call with a direct format string: change the println invocation that currently wraps format_args! (which prints "Root account has been created with id: {}" using root_user_config.id) to a simple println!("Root account has been created with id: {}", root_user_config.id) (or the project’s existing colored-print helper if other commands use coloring) to remove unnecessary indirection and match the style used elsewhere.tiles/src/utils/config.rs (1)
163-167: Test writes to real filesystem with no cleanup.
get_or_create_config()creates.tiles_dev/tiles/config.tomlin the working directory during test runs. There's no teardown, and running tests concurrently (e.g.,cargo test -- --test-threads=N) can cause races. Consider using aTempDirfixture injected via theConfigProvidertrait (which already exists), or at minimum add anaftercleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/config.rs` around lines 163 - 167, The test test_create_config_file currently calls get_or_create_config() which writes .tiles_dev/tiles/config.toml to the real filesystem with no cleanup and causes races; update the test to use an isolated TempDir (or inject a temporary path via the existing ConfigProvider trait) so the created config is written into a temp directory, and ensure the test removes the temp dir (or lets TempDir drop) after running; specifically, modify test_create_config_file to construct a TempDir, instantiate the ConfigProvider (or a test double) to point to that temp path before calling get_or_create_config(), or call the provider-backed variant of get_or_create_config(), and verify cleanup to avoid concurrent test races.tilekit/src/accounts.rs (1)
33-39:set_default_credential_buildermutates global state — parallel tests can race.Both
tilekit::accounts::testsandtiles::utils::accounts::testscallset_default_credential_builder(mock::default_credential_builder()). Sincecargo testruns test threads concurrently by default, one test can overwrite the global mid-way through another. Consider using theserial_testcrate or structuring tests to avoid global keyring state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tilekit/src/accounts.rs` around lines 33 - 39, The test mutates global state via set_default_credential_builder(mock::default_credential_builder()) which can race with other tests; modify the tests to avoid concurrent mutation by either marking the test function with serial_test::serial (add the serial_test crate and #[serial]) or by saving the previous builder and restoring it after the test (snapshot the current default, call set_default_credential_builder for the test, and restore the snapshot in a finally/teardown to ensure no cross-test leakage). Apply this to the test containing create_identity and any sibling tests that call set_default_credential_builder so they no longer race.tiles/src/utils/accounts.rs (1)
84-88: RedundantOk(…?)wrapping on line 85.
Ok(create_root_user(root_user_table, nickname)?)is equivalent to justcreate_root_user(root_user_table, nickname)(the?already propagates errors and the outerOkre-wraps the success).♻️ Proposed fix
if did.is_empty() { - Ok(create_root_user(root_user_table, nickname)?) + create_root_user(root_user_table, nickname) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/accounts.rs` around lines 84 - 88, The branch that returns a newly created root user wraps a fallible call with both ? and Ok, which is redundant; in the conditional using did, replace the outer Ok(create_root_user(root_user_table, nickname)?) pattern with a direct return of the fallible call (i.e., return the result of create_root_user(root_user_table, nickname) so the ? propagates errors and you don't re-wrap with Ok), keeping the existing else branch that returns root_user_table.clone().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yml:
- Around line 19-23: The YAML rule uses path: "**/*.{rs,toml}" but its
instructions mention Python, so update the .coderabbit.yml rule by either adding
a new rule with path: "**/*.py" containing the Python-specific instructions (or
split into path_instructions entries), or remove the Python reference from the
existing instructions; target the "path" and "instructions" keys in the existing
rule to implement the fix.
In `@tilekit/src/accounts.rs`:
- Line 6: The code currently imports and uses the undocumented symbol
Ed25519Did; replace that with the documented API by switching the import to use
ucan::crypto::did::DidParser (or, if you need raw key material, use the
ucan-key-support crate's Ed25519KeyMaterial) and update all usages of Ed25519Did
to parse or construct DIDs through DidParser::parse / DidParser::from_key (or
construct keys via Ed25519KeyMaterial and convert to a DID) so the module no
longer depends on the private Ed25519Did type; update the use statement and any
code that constructs or expects Ed25519Did to instead call DidParser or
Ed25519KeyMaterial methods (referencing the symbols Ed25519Did, DidParser, and
Ed25519KeyMaterial to locate and change the relevant code).
In `@tiles/src/utils/config.rs`:
- Around line 130-143: The initial-write branch of get_or_create_config writes
directly to config_toml_path which can leave a partial/corrupt file on crash;
change it to write atomically by serializing init_table to a temporary file in
the same directory (e.g., config_toml_path.with_extension("tmp") or a unique
temp name), fs::write the temp file, then fs::rename (or std::fs::rename) the
temp into config_toml_path, mirroring the atomic pattern used by save_config;
keep the same return of init_table and propagate errors as before.
---
Duplicate comments:
In `@tiles/src/commands/mod.rs`:
- Around line 69-74: Replace the unsafe .unwrap() chains on root_user_config
lookups in the Ok(root_user_config) arm with proper error propagation: call
get("id").and_then(|v| v.as_str()).ok_or_else(|| /* create useful error */)? and
similarly for "nickname", so both values return Result<&str, _> and use ? to
propagate errors; then pass the borrowed &root_user_config into
save_root_account(config, &root_user_config)? and use the obtained id and
nickname in the println as before. Ensure the error type matches the function
signature (map to the existing error enum or Box<dyn Error>) so the ? operator
compiles.
- Around line 75-77: The Err branch for the set_nickname call currently prints
to stdout and then continues, which swallows the error; change it to write to
stderr and propagate the error instead. Replace println!("Failed to set nickname
due to {}", err) with eprintln!("Failed to set nickname: {}", err) and then
return the error (e.g., return Err(err.into()) or return Err(err) if the error
type already matches the function's Result), ensuring the function exits with an
Err when set_nickname fails.
In `@tiles/src/utils/accounts.rs`:
- Around line 126-138: In create_root_user, replace the explicit match on
create_identity("tiles") (which currently handles Ok(did) and Err(err) =>
Err(err)) with the try operator so the function returns early on error; call let
did = create_identity("tiles")?; then proceed to insert "id" and optional
"nickname" into root_user_table and return Ok(root_user_table), keeping the
existing root_user_config cloning and nickname handling.
- Around line 113-124: The set_nickname function currently panics due to chained
unwraps when reading ROOT_USER_CONFIG_KEY and its "id"; change those unwraps to
safe error propagation using ok_or_else(...)? and the ? operator: retrieve
root_user via config.get(ROOT_USER_CONFIG_KEY).ok_or_else(||
anyhow::anyhow!("Missing root user config"))?; convert to table with
.as_table().ok_or_else(|| anyhow::anyhow!("Root user not a table"))?; read the
"id" with root_user_table.get("id").and_then(|v| v.as_str()).ok_or_else(||
anyhow::anyhow!("Root user id missing or not a string"))?; then proceed with
cloning, inserting "id" and "nickname" and returning Ok(root_user_table) as
before, preserving the empty-id check and returning Err(...) when appropriate.
In `@tiles/src/utils/config.rs`:
- Around line 147-155: The save_config function uses fs::copy which can corrupt
config.toml on crashes; change the final step to atomically replace config_path
by calling fs::rename(&tmp_path, &config_path) instead of fs::copy(&tmp_path,
&config_path), and keep writing the tmp_path via fs::write as-is; remove the
subsequent fs::remove_file(tmp_path) (or only remove tmp_path on error) so the
successful path relies on the atomic rename; references: save_config,
DefaultProvider.get_config_dir, tmp_path, config_path.
---
Nitpick comments:
In `@tilekit/src/accounts.rs`:
- Around line 33-39: The test mutates global state via
set_default_credential_builder(mock::default_credential_builder()) which can
race with other tests; modify the tests to avoid concurrent mutation by either
marking the test function with serial_test::serial (add the serial_test crate
and #[serial]) or by saving the previous builder and restoring it after the test
(snapshot the current default, call set_default_credential_builder for the test,
and restore the snapshot in a finally/teardown to ensure no cross-test leakage).
Apply this to the test containing create_identity and any sibling tests that
call set_default_credential_builder so they no longer race.
In `@tiles/src/commands/mod.rs`:
- Around line 55-61: Replace the indirect println!("{}", format_args!(...)) call
with a direct format string: change the println invocation that currently wraps
format_args! (which prints "Root account has been created with id: {}" using
root_user_config.id) to a simple println!("Root account has been created with
id: {}", root_user_config.id) (or the project’s existing colored-print helper if
other commands use coloring) to remove unnecessary indirection and match the
style used elsewhere.
In `@tiles/src/utils/accounts.rs`:
- Around line 84-88: The branch that returns a newly created root user wraps a
fallible call with both ? and Ok, which is redundant; in the conditional using
did, replace the outer Ok(create_root_user(root_user_table, nickname)?) pattern
with a direct return of the fallible call (i.e., return the result of
create_root_user(root_user_table, nickname) so the ? propagates errors and you
don't re-wrap with Ok), keeping the existing else branch that returns
root_user_table.clone().
In `@tiles/src/utils/config.rs`:
- Around line 163-167: The test test_create_config_file currently calls
get_or_create_config() which writes .tiles_dev/tiles/config.toml to the real
filesystem with no cleanup and causes races; update the test to use an isolated
TempDir (or inject a temporary path via the existing ConfigProvider trait) so
the created config is written into a temp directory, and ensure the test removes
the temp dir (or lets TempDir drop) after running; specifically, modify
test_create_config_file to construct a TempDir, instantiate the ConfigProvider
(or a test double) to point to that temp path before calling
get_or_create_config(), or call the provider-backed variant of
get_or_create_config(), and verify cleanup to avoid concurrent test races.
Comment on lines
19
to
+23
| - path: "**/*.{rs,toml}" | ||
| instructions: | ||
| "Review the Rust code for conformity with best practices in Rust, | ||
| Systems programming. Highlight any deviations." | ||
| "Review the Rust code and Python code for conformity with best practices in Rust, | ||
| Systems programming and Python. Highlight any deviations. Also highlight if there any | ||
| any security issues in the code" |
There was a problem hiding this comment.
Python review instruction targets the wrong path pattern.
The path is **/*.{rs,toml}, which never matches .py files. The "Python code" mention in instructions has no effect. Either add a separate path_instructions entry for **/*.py, or remove the Python reference from this rule.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yml around lines 19 - 23, The YAML rule uses path:
"**/*.{rs,toml}" but its instructions mention Python, so update the
.coderabbit.yml rule by either adding a new rule with path: "**/*.py" containing
the Python-specific instructions (or split into path_instructions entries), or
remove the Python reference from the existing instructions; target the "path"
and "instructions" keys in the existing rule to implement the fix.
Comment on lines
+130
to
+143
| if config_toml_path.try_exists()? { | ||
| let config_str = fs::read_to_string(config_toml_path)?; | ||
| Ok(config_str.parse::<Table>()?) | ||
| } else { | ||
| let init_table: Table = toml::from_str( | ||
| r#" | ||
| [root-user] | ||
| id = '' | ||
| nickname = '' | ||
| "#, | ||
| )?; | ||
| fs::write(config_toml_path, init_table.to_string())?; | ||
| Ok(init_table) | ||
| } |
There was a problem hiding this comment.
Non-atomic initial write in get_or_create_config can corrupt config.toml.
Line 141 writes directly to config_toml_path. A crash mid-write leaves a partial file; on the next run try_exists() returns true, parsing fails, and the config is unrecoverable. Apply the same temp-file + rename pattern as save_config.
🛡️ Proposed fix
} else {
let init_table: Table = toml::from_str(r#"
[root-user]
id = ''
nickname = ''
"#)?;
- fs::write(config_toml_path, init_table.to_string())?;
+ let tmp_path = tiles_config_dir.join("config.tmp.toml");
+ fs::write(&tmp_path, init_table.to_string())?;
+ fs::rename(&tmp_path, &config_toml_path)?;
Ok(init_table)
}📝 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
| if config_toml_path.try_exists()? { | |
| let config_str = fs::read_to_string(config_toml_path)?; | |
| Ok(config_str.parse::<Table>()?) | |
| } else { | |
| let init_table: Table = toml::from_str( | |
| r#" | |
| [root-user] | |
| id = '' | |
| nickname = '' | |
| "#, | |
| )?; | |
| fs::write(config_toml_path, init_table.to_string())?; | |
| Ok(init_table) | |
| } | |
| if config_toml_path.try_exists()? { | |
| let config_str = fs::read_to_string(config_toml_path)?; | |
| Ok(config_str.parse::<Table>()?) | |
| } else { | |
| let init_table: Table = toml::from_str( | |
| r#" | |
| [root-user] | |
| id = '' | |
| nickname = '' | |
| "#, | |
| )?; | |
| let tmp_path = tiles_config_dir.join("config.tmp.toml"); | |
| fs::write(&tmp_path, init_table.to_string())?; | |
| fs::rename(&tmp_path, &config_toml_path)?; | |
| Ok(init_table) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tiles/src/utils/config.rs` around lines 130 - 143, The initial-write branch
of get_or_create_config writes directly to config_toml_path which can leave a
partial/corrupt file on crash; change it to write atomically by serializing
init_table to a temporary file in the same directory (e.g.,
config_toml_path.with_extension("tmp") or a unique temp name), fs::write the
temp file, then fs::rename (or std::fs::rename) the temp into config_toml_path,
mirroring the atomic pattern used by save_config; keep the same return of
init_table and propagate errors as before.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
under [memory] section
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tiles/src/utils/config.rs (1)
115-122:⚠️ Potential issue | 🟡 Minor
path_buf.to_str().unwrap()panics on non-UTF-8 paths;format!wrapping is redundant.
to_str()returnsNonefor paths with non-UTF-8 characters, causing a panic. Usepath_buf.display()instead. Theanyhow!macro accepts format arguments directly, making the outerformat!call redundant on both error and success paths.🐛 Proposed fix
} else { - return Err(anyhow::anyhow!(format!( - "Not a valid path {}", - path_buf.to_str().unwrap() - ))); + return Err(anyhow!("Not a valid path {}", path_buf.display())); } Ok(format!( "Memory path set successfully at {}", - path_buf.to_str().unwrap() + path_buf.display() ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/config.rs` around lines 115 - 122, The code panics on non-UTF-8 paths because it calls path_buf.to_str().unwrap() and also uses redundant format! wrappers; replace uses of path_buf.to_str().unwrap() with path_buf.display() and pass formatting directly to anyhow! and Ok without the outer format! calls (i.e., use anyhow!("Not a valid path {}", path_buf.display()) and return Ok(format!("Memory path set successfully at {}", path_buf.display())) or equivalent direct formatting), updating the return/error lines that reference path_buf in this block.
🧹 Nitpick comments (1)
tiles/src/utils/config.rs (1)
167-171: Test has no assertions and leaves real filesystem side effects.After the first run
config.tomlexists, so every subsequent run takes thetry_exists() == truebranch, and the initial-creation code path is never exercised again. There are also no assertions on the returnedTablestructure (e.g., that required sectionsroot-userandmemoryare present). Consider injecting a temp directory (viatempfilecrate) per test and asserting key structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/config.rs` around lines 167 - 171, The test test_create_config_file currently lacks assertions and mutates the real filesystem via get_or_create_config; update it to use an isolated temp directory (e.g., tempfile::tempdir) and inject that path into get_or_create_config (or add a helper that accepts a Path) so the test exercises the initial-creation branch; then assert the returned Table contains the required sections/keys such as "root-user" and "memory" (and any expected values), and ensure the temp directory is cleaned up at test end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tiles/src/utils/config.rs`:
- Line 101: set_memory_path_with_provider currently ignores its ConfigProvider
parameter, breaking the abstraction; to fix, thread the provider through instead
of using DefaultProvider inside helpers: change get_or_create_config and
save_config to accept a generic P: ConfigProvider (or &dyn ConfigProvider) and
update their signatures and call sites to take the same provider reference, then
have set_memory_path_with_provider call get_or_create_config(&provider, ...) and
save_config(&provider, ...) so the supplied provider is actually used;
alternatively, if you intend no external provider, remove the generic parameter
from set_memory_path_with_provider and keep DefaultProvider usage consistently.
- Around line 108-110: Replace the panic-causing .as_table().expect(...) call
with proper error handling and propagate a Result/Option instead of aborting;
specifically, in the function that reads the "memory" config (the code that
currently calls .as_table().expect("Failed to parse to table
(memory)").clone()), use .as_table().ok_or_else(|| /* descriptive error */)? or
map_err(...) to convert the invalid type into an error that the caller can
handle (mirroring the error-handling style used in get_memory_path), then clone
the table only after successful conversion so malformed config does not panic
the process.
- Around line 63-79: Replace the panicking `.expect()` calls in this function:
instead of `memory_config.as_table().expect(...)` and
`path.as_str().expect(...)`, convert those `.as_*()` results to `ok_or_else(...)
?` so malformed types return an `Err` rather than aborting; use descriptive
anyhow! messages (e.g. "memory section is not a table" and "memory.path is not a
string") referencing the same `memory_config` and `path` variables/functions
(`get_or_create_config`) to locate the code, and simplify the empty-path error
by using `anyhow!("NOT SET")` (no `format!` wrapper) when returning the Err for
an empty path.
---
Outside diff comments:
In `@tiles/src/utils/config.rs`:
- Around line 115-122: The code panics on non-UTF-8 paths because it calls
path_buf.to_str().unwrap() and also uses redundant format! wrappers; replace
uses of path_buf.to_str().unwrap() with path_buf.display() and pass formatting
directly to anyhow! and Ok without the outer format! calls (i.e., use
anyhow!("Not a valid path {}", path_buf.display()) and return Ok(format!("Memory
path set successfully at {}", path_buf.display())) or equivalent direct
formatting), updating the return/error lines that reference path_buf in this
block.
---
Duplicate comments:
In `@tiles/src/utils/config.rs`:
- Around line 151-159: In save_config, avoid non-atomic fs::copy +
fs::remove_file by replacing the final two calls with a single atomic fs::rename
from tmp_path to config_path; after writing the temp file (tmp_path) call
fs::rename(&tmp_path, &config_path)? to atomically replace the destination and
propagate any error, removing the fs::copy and fs::remove_file calls.
- Line 145: get_or_create_config currently uses fs::write(config_toml_path,
init_table.to_string()) which can leave a partially written file on crash;
change it to the same atomic pattern used in save_config: create a temp path
(e.g., config_toml_path.with_extension("tmp") or similar), write the serialized
init_table to the temp file using File::create + write_all, flush/sync if
save_config does so, then fs::rename the temp file to config_toml_path;
reference get_or_create_config, config_toml_path, and init_table when making the
change and mirror error handling used by save_config.
---
Nitpick comments:
In `@tiles/src/utils/config.rs`:
- Around line 167-171: The test test_create_config_file currently lacks
assertions and mutates the real filesystem via get_or_create_config; update it
to use an isolated temp directory (e.g., tempfile::tempdir) and inject that path
into get_or_create_config (or add a helper that accepts a Path) so the test
exercises the initial-creation branch; then assert the returned Table contains
the required sections/keys such as "root-user" and "memory" (and any expected
values), and ensure the temp directory is cleaned up at test end.
Comment on lines
+63
to
79
| let root_config = get_or_create_config()?; | ||
| let memory_config = root_config | ||
| .get("memory") | ||
| .ok_or_else(|| anyhow!("memory section doesnt exist"))? | ||
| .as_table() | ||
| .expect("Failed to parse to table (memory)"); | ||
|
|
||
| if is_memory_path_found { | ||
| Ok(memory_path) | ||
| } else { | ||
| let path = memory_config | ||
| .get("path") | ||
| .ok_or_else(|| anyhow!("path doesnt exist (memory)"))? | ||
| .as_str() | ||
| .expect("parse failed (memory)"); | ||
| if path.is_empty() { | ||
| Err(anyhow::anyhow!(format!("NOT SET"))) | ||
| } else { | ||
| Ok(path.to_owned()) | ||
| } |
There was a problem hiding this comment.
Panicking .expect() calls will crash the process on a malformed config.
.as_table() (Line 68) and .as_str() (Line 74) both use .expect(), so any manually-edited or corrupted config.toml where those keys hold unexpected types will abort the process rather than surface an error. Replace with ok_or_else + ?. Additionally, anyhow::anyhow!(format!("NOT SET")) (Line 76) wraps a bare literal in format! unnecessarily — anyhow! accepts format args directly.
🐛 Proposed fix
pub fn get_memory_path() -> Result<String> {
let root_config = get_or_create_config()?;
let memory_config = root_config
.get("memory")
.ok_or_else(|| anyhow!("memory section doesnt exist"))?
.as_table()
- .expect("Failed to parse to table (memory)");
+ .ok_or_else(|| anyhow!("memory section is not a table"))?;
let path = memory_config
.get("path")
.ok_or_else(|| anyhow!("path doesnt exist (memory)"))?
.as_str()
- .expect("parse failed (memory)");
+ .ok_or_else(|| anyhow!("memory.path is not a string"))?;
if path.is_empty() {
- Err(anyhow::anyhow!(format!("NOT SET")))
+ Err(anyhow!("NOT SET"))
} else {
Ok(path.to_owned())
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tiles/src/utils/config.rs` around lines 63 - 79, Replace the panicking
`.expect()` calls in this function: instead of
`memory_config.as_table().expect(...)` and `path.as_str().expect(...)`, convert
those `.as_*()` results to `ok_or_else(...) ?` so malformed types return an
`Err` rather than aborting; use descriptive anyhow! messages (e.g. "memory
section is not a table" and "memory.path is not a string") referencing the same
`memory_config` and `path` variables/functions (`get_or_create_config`) to
locate the code, and simplify the empty-path error by using `anyhow!("NOT SET")`
(no `format!` wrapper) when returning the Err for an empty path.
| } | ||
|
|
||
| fn set_memory_path_with_provider<P: ConfigProvider>(provider: &P, path: &str) -> Result<String> { | ||
| fn set_memory_path_with_provider<P: ConfigProvider>(_provider: &P, path: &str) -> Result<String> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
_provider is declared but silently ignored — the abstraction is broken.
get_or_create_config() and save_config() both internally instantiate DefaultProvider, so the generic P: ConfigProvider parameter passed by callers has no effect. This makes the function impossible to test with a mock provider. Either thread the provider through both helpers or remove the dead parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tiles/src/utils/config.rs` at line 101, set_memory_path_with_provider
currently ignores its ConfigProvider parameter, breaking the abstraction; to
fix, thread the provider through instead of using DefaultProvider inside
helpers: change get_or_create_config and save_config to accept a generic P:
ConfigProvider (or &dyn ConfigProvider) and update their signatures and call
sites to take the same provider reference, then have
set_memory_path_with_provider call get_or_create_config(&provider, ...) and
save_config(&provider, ...) so the supplied provider is actually used;
alternatively, if you intend no external provider, remove the generic parameter
from set_memory_path_with_provider and keep DefaultProvider usage consistently.
Comment on lines
+108
to
+110
| .as_table() | ||
| .expect("Failed to parse to table (memory)") | ||
| .clone(); |
There was a problem hiding this comment.
.expect() on Line 109 panics on malformed config.
Same risk as in get_memory_path — if the memory TOML value is not a table, this aborts the process.
🐛 Proposed fix
let mut memory_config = root_config
.get("memory")
.ok_or_else(|| anyhow!("memory section doesnt exist"))?
.as_table()
- .expect("Failed to parse to table (memory)")
+ .ok_or_else(|| anyhow!("memory section is not a table"))?
.clone();📝 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
| .as_table() | |
| .expect("Failed to parse to table (memory)") | |
| .clone(); | |
| let mut memory_config = root_config | |
| .get("memory") | |
| .ok_or_else(|| anyhow!("memory section doesnt exist"))? | |
| .as_table() | |
| .ok_or_else(|| anyhow!("memory section is not a table"))? | |
| .clone(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tiles/src/utils/config.rs` around lines 108 - 110, Replace the panic-causing
.as_table().expect(...) call with proper error handling and propagate a
Result/Option instead of aborting; specifically, in the function that reads the
"memory" config (the code that currently calls .as_table().expect("Failed to
parse to table (memory)").clone()), use .as_table().ok_or_else(|| /* descriptive
error */)? or map_err(...) to convert the invalid type into an error that the
caller can handle (mirroring the error-handling style used in get_memory_path),
then clone the table only after successful conversion so malformed config does
not panic the process.
This was referenced Mar 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.