Conversation
Use shellexpand to resolve tilde and environment variables in log_dir and SASL external PEM paths during config parsing. Add shellexpand as a new dependency for tilde (~) and environment variable ($VAR) expansion in file paths. Add tests for path expansion, log_dir and sasl pem integration. Fixes osa1#192, fixes osa1#429
osa1
left a comment
osa1
left a comment
There was a problem hiding this comment.
Am I right that this is vibe-coded?
I know we don't have a policy in this repo for vibe-coded patches yet, but for now you should (1) review every line yourself and make sure you understand every line and design decision (2) declare that the PR is vibe-coded so that I know how detailed I should give feedback. (I'm not keen on talking to an LLM in a GitHub comments section and micro-managing it in GitHub comments)
| Ok(expanded) => PathBuf::from(expanded.as_ref()), | ||
| Err(err) => { | ||
| println!("Failed to expand path {path:?}: {err}"); | ||
| path.to_owned() |
There was a problem hiding this comment.
When expand_path fails to expand a variable we can't continue, we should show the error to the user and stop.
parse_config should return all errors, including IO errors (which we don't return currently, but we should) + path expansion errors. The call site (main) already deals with errors (prints them and exits).
| PathBuf::from(format!("{home}/test_logs")) | ||
| ); | ||
|
|
||
| let _ = fs::remove_dir_all(&dir); |
There was a problem hiding this comment.
You don't have to create a file to test config file parsing and shell expansion. Refactor the code to separate the IO parts (the let contents = { ... } part in parse_config) from parsing parts, then test the parsing function.
|
Sorry for the additional review round. Yes, I used Claude Code to speed up the implementation, but I reviewed the code myself before submitting it. That said, I didn’t examine the AI-generated output carefully enough, particularly in terms of error handling and test design. The silent fallback on expansion failure and the unnecessary file I/O in the tests are issues I should have caught earlier. I will fix both: Make expand_path return an error instead of silently falling back, and ensure parse_config propagates all relevant errors, including I/O and path expansion errors. Separate I/O from parsing so tests can call the parsing function directly. Going forward, I will clearly declare AI assistance upfront and apply a more thorough review before submitting changes. |
|
Feel free to reopen when you have an update. |
2 participants
Summary
~) and environment variable ($VAR) expansion forlog_dirand SASL externalpempaths in configshellexpandcrate to resolve paths like~/tiny_logsor$HOME/logsduring config parsingparse_configafter deserialization, before the config is used by the rest of the applicationshellexpandas a new dependency for path expansionexpand_pathfunction, 2 integration tests verifyingparse_configexpandslog_dirand
sasl pempaths end-to-endFixes #192, fixes #429
Test plan
cargo test -p tiny— 15 tests pass (10 existing + 5 new)cargo build— builds successfully