Resolve password-store's store_dir relative to file declaring#359
Resolve password-store's store_dir relative to file declaring#359
store_dir relative to file declaring#359Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical path resolution bug affecting Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
1fexd
force-pushed
the
relative-store-dir
branch
from
33ba3b3 to
2bbe2ea
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for resolving relative store_dir paths in the password-store provider. The approach of intercepting the resolved configuration and adjusting the path is a good proof-of-concept. My review includes a few suggestions to improve the implementation: a fix for a potential bug in path joining logic, some readability improvements, and suggestions for more comprehensive test coverage. These changes will make the new logic more robust and maintainable.
| pub fn join_path(store_dir: Option<String>, provider_source: Option<PathBuf>) -> Option<String> { | ||
| if let Some(store_path) = store_dir.as_deref() { | ||
| let path = PathBuf::from(store_path); | ||
| if path.is_relative() { | ||
| if let Some(provider_source_path) = provider_source { | ||
| if provider_source_path.is_file() { | ||
| if let Some(mut parent) = provider_source_path.parent().map(|x| x.to_path_buf()) { | ||
| parent.extend(&path); | ||
| return parent.into_os_string().to_str().map(|x| String::from(x)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| store_dir | ||
| } |
There was a problem hiding this comment.
This function has a potential bug and can be made more readable.
- Bug: If
parent.into_os_string().to_str()returnsNone(which can happen with non-UTF-8 paths), the function will returnNone, effectively losing thestore_dirconfiguration. It should fall back to the originalstore_dir. - Readability: The nested
ifstatements can be flattened to improve readability. - Idiomatic Rust: Using
parent.push(path)is more idiomatic for appending a path thanparent.extend(&path).
Here is a suggested refactoring that addresses these points:
pub fn join_path(store_dir: Option<String>, provider_source: Option<PathBuf>) -> Option<String> {
if let (Some(store_path_str), Some(provider_source_path)) = (store_dir.as_deref(), provider_source) {
let path = PathBuf::from(store_path_str);
if path.is_relative() && provider_source_path.is_file() {
if let Some(parent) = provider_source_path.parent() {
let mut new_path = parent.to_path_buf();
new_path.push(path);
if let Some(s) = new_path.to_str() {
return Some(s.to_string());
}
}
}
}
store_dir
}| let x = ResolvedProviderConfig::PasswordStore { prefix, store_dir: joined_path, gpg_opts }; | ||
| return get_provider_from_resolved(provider_name, &x); |
There was a problem hiding this comment.
For better readability and maintainability, consider using a more descriptive variable name instead of x. For example, updated_resolved_config would make the code's intent clearer.
| let x = ResolvedProviderConfig::PasswordStore { prefix, store_dir: joined_path, gpg_opts }; | |
| return get_provider_from_resolved(provider_name, &x); | |
| let updated_resolved_config = ResolvedProviderConfig::PasswordStore { prefix, store_dir: joined_path, gpg_opts }; | |
| return get_provider_from_resolved(provider_name, &updated_resolved_config); |
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test() { | ||
| let result = join_path(Some(String::from("./password-store")), Some(PathBuf::from("/home/replace/me/with/a/real/path/fnox.toml"))); | ||
| assert!(result.is_some()); | ||
| assert_eq!(result.unwrap(), "/home/replace/me/with/a/real/path/./password-store") | ||
| } | ||
| } |
There was a problem hiding this comment.
The test for join_path is a good start, but it could be improved:
- Test Name: The name
testis not descriptive. A better name would betest_join_path_with_relative_store_dir. - Test Coverage: The test only covers the happy path for a relative
store_dir. Consider adding more test cases to cover other scenarios, such as:- An absolute
store_dir(should be returned unchanged). store_dirisNone.provider_sourceisNone.
- An absolute
Here's an example of how you could expand the tests:
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_join_path_with_relative_store_dir() {
let result = join_path(Some(String::from("./password-store")), Some(PathBuf::from("/home/user/project/fnox.toml")));
assert_eq!(result, Some("/home/user/project/./password-store".to_string()));
}
#[test]
fn test_join_path_with_absolute_store_dir() {
let store_dir = Some("/etc/passwords".to_string());
let result = join_path(store_dir.clone(), Some(PathBuf::from("/home/user/project/fnox.toml")));
assert_eq!(result, store_dir);
}
#[test]
fn test_join_path_no_provider_source() {
let store_dir = Some("./password-store".to_string());
let result = join_path(store_dir.clone(), None);
assert_eq!(result, store_dir);
}
#[test]
fn test_join_path_no_store_dir() {
let result = join_path(None, Some(PathBuf::from("/home/user/project/fnox.toml")));
assert_eq!(result, None);
}
}
Greptile SummaryThis PR fixes a real bug where a relative The fix is applied both at the top-level Confidence Score: 4/5Core logic is correct but the unit test will fail in CI due to open issues from the previous review rounds, making the PR not yet mergeable. The approach correctly addresses the described bug and the recursive resolution code path in resolver.rs is now covered. However, issues identified in prior review rounds — the src/providers/mod.rs — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[get_provider_resolved] --> B[resolve_provider_config]
B --> C[get_provider_from_resolved2]
C --> D{Is PasswordStore?}
D -- Yes --> E[config.provider_sources.get provider_name]
E --> F[join_path store_dir, provider_source]
F --> G{store_dir relative AND\nprovider_source is_file?}
G -- Yes --> H[parent_dir joined with store_dir]
G -- No --> I[original store_dir unchanged]
H --> J[get_provider_from_resolved]
I --> J
D -- No --> K[get_provider_from_resolved directly]
L[resolve_secret_ref in resolver.rs] --> M[resolve_provider_config_with_context]
M --> C
Reviews (4): Last reviewed commit: "Merge branch 'main' into relative-store-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Does this change make sense, @jdx? Should I check out the LLM reviews? |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
2 participants
Hello,
I've run into an issue where declaring a
password-storeprovider in a base config file with a relativestore_dirpath results in an error when the provider is used in a child directory, as the path is not resolved relative to the base config file, but apparently relative to the current directory?Consider the following example, taken from the documentation, and adapted:
project/fnox.toml:project/services/worker/fnox.toml:Running
fnox get API_KEYinproject/services/workerresults in:This PR implements a PoC (I'm not familiar with Rust, so you'd probably have to rework the code) which works around this issue by resolving the
store_dirrelative to theprojectdirectory.