Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #729 +/- ##
=======================================
Coverage 55.66% 55.66%
=======================================
Files 126 126
Lines 13759 13762 +3
=======================================
+ Hits 7659 7661 +2
- Misses 6100 6101 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
👀 Conductor: Awaiting ReviewerPR #729 ( Nudge posted by conductor pipeline sync. |
geoffjay
left a comment
There was a problem hiding this comment.
Review: refactor(common): extract resolve_data_dir helper from get_db_path
Stack position: issue-722 is directly on main with no parent PR
dependency, but git-spice reports (needs restack) — the branch is behind
main and must be rebased before the conductor can merge it.
The extraction of resolve_data_dir is the right refactor — sharing the
AGENTD_ENV-based path resolution between get_db_path and future callers
is clearly valuable. Two issues need fixing before this can merge.
Blocking — test race condition
The PR adds four separate #[test] functions that each mutate
std::env::var("AGENTD_ENV") via set_var / remove_var:
#[test]
fn test_resolve_data_dir_development() { ... }
#[test]
fn test_resolve_data_dir_dev_shorthand() { ... }
#[test]
fn test_resolve_data_dir_test() { ... }
#[test]
fn test_resolve_data_dir_production_contains_project_name() { ... }These will race against each other and against the existing
test_get_db_path_respects_agentd_env when cargo test runs them in
parallel (the default). The result is non-deterministic test failures.
The comment directly above these tests says:
// All AGENTD_ENV variants are exercised in a single test to prevent
// data races between parallel test threads that share the process
// environment.The implementation contradicts that comment. The existing
test_get_db_path_respects_agentd_env test in this file demonstrates the
correct pattern: exercise all AGENTD_ENV variants sequentially inside a
single #[test] function so only one test holds the env-var mutation at a
time.
Fix: collapse the four new tests into a single
test_resolve_data_dir_respects_agentd_env function that sequences all
variants, following the same structure as test_get_db_path_respects_agentd_env.
Non-blocking — doc comment displacement
The original get_db_path doc comment (lines 23–40 in the current file)
describes db_filename, mentions create_dir_all, and gives macOS/Linux
path examples. After this PR the extracted comment will be attached to
the private resolve_data_dir function, leaving the public get_db_path
with no doc comment at all.
Suggestion: keep a condensed doc comment on get_db_path describing its
public contract (what it returns, when it errors), and let resolve_data_dir
have a brief internal note. The full path examples are more useful on the
public function.
Action required before merge
- Fix the four separate env-var tests → single sequential test (blocking)
- Rebase onto
main:
git-spice branch restack
git-spice branch submit
refactor(common): extract resolve_data_dir helper from get_db_path
refactor(common): extract resolve_data_dir helper from get_db_path (closes #722)