refactor(common): extract resolve_data_dir helper from get_db_path

[geoffjay]

refactor(common): extract resolve_data_dir helper from get_db_path

refactor(common): extract resolve_data_dir helper from get_db_path (closes #722)

[geoffjay]

This change is part of the following stack:

• refactor(common): extract resolve_data_dir helper from get_db_path #729 ◀

_{Change managed by git-spice.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.77%. Comparing base (a5867f0) to head (d222912).
⚠️ Report is 621 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #729      +/-   ##
==========================================
+ Coverage   55.66%   63.77%   +8.11%     
==========================================
  Files         126      173      +47     
  Lines       13759     7733    -6026     
  Branches        0     2566    +2566     
==========================================
- Hits         7659     4932    -2727     
+ Misses       6100     2780    -3320     
- Partials        0       21      +21     

Flag	Coverage Δ
frontend	63.77% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[geoffjay]

👀 Conductor: Awaiting Reviewer

PR #729 (refactor(common): extract resolve_data_dir helper) has CI passing and is in the review-agent queue awaiting a reviewer pickup.

Nudge posted by conductor pipeline sync.

[geoffjay]

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

1. Fix the four separate env-var tests → single sequential test (blocking)
2. Rebase onto main:

git-spice branch restack
git-spice branch submit

[geoffjay]

Review: refactor(common): extract resolve_data_dir helper from get_db_path

Stack Position

Branch issue-722 is based directly on main (not in a stack). No parent PR to review first.

---

Scope: PR Is Too Large to Review as a Single Unit

The PR description says this is a refactor(common) that extracts resolve_data_dir from get_db_path and closes #722. The actual diff is 1,281 additions and 5,456 deletions across 60+ files containing at least five distinct, unrelated changes:

Change Area	Files	Lines
crates/common/src/storage.rs refactor (stated)	1	~80
Removal of entire BAML crate and baml_src/	30+	~5,000 deletions
Agent/room/workflow YAML updates (Code Index Protocol, schema headers, sandbox_bypass)	25+	~800
UI themes.ts deletion	1	142 deletions
Config/audit updates (.cargo/)	2	~30

Per project conventions: "Keep changes focused on the issue scope — don't refactor unrelated code." This PR should be split into focused pieces before it can be reviewed effectively.

---

Blocking Issue: .cargo/config.toml — sccache wrapper is active, not commented

# Uncomment after: brew install sccache
[build]
rustc-wrapper = "sccache"

The comment says "Uncomment after: brew install sccache" but the rustc-wrapper line is not commented out — it is active. Any developer (or CI) without sccache installed will get a build failure:

error: could not exec the linker `sccache`: No such file or directory

Either comment it out, add a CI step that installs sccache before building, or move this to a developer-local config (e.g. .cargo/config.local.toml in .gitignore). As committed, this is a build-breaking change for anyone without sccache.

---

The storage.rs Refactoring Is Correct

For reference when the focused PR is submitted:

• resolve_data_dir() is cleanly extracted as a private function — the inline match block is gone from get_db_path, which reads better
• The improved error message now includes the project name: "for '{project_name}'" vs the old generic "Failed to determine project directories" — strictly better
• The doc comment on get_db_path accurately describes the three-way env selection
• The test runs all four AGENTD_ENV variants sequentially inside a single #[test] to avoid races between parallel test threads sharing the process environment — this is the correct pattern

---

Other Unrelated Changes — Evaluation Deferred

The BAML removal (crates/baml, baml_src/), the agent YAML updates, and the UI deletion all appear intentional but cannot be properly reviewed bundled with this refactor. Each deserves its own PR and description explaining the rationale (especially the BAML removal, which is a significant feature deprecation).

---

Suggested Split

1. This issue (#722): storage.rs only — the refactor is correct and can merge immediately once isolated
2. BAML removal: baml_src/, crates/baml/, Cargo.toml workspace removal, hook/monitor Cargo.toml cleanup — needs its own issue/description
3. Agent YAML updates: Code Index Protocol additions, sandbox_bypass entries, schema headers — agent YAML changes affect all future runs and should be reviewed separately
4. Misc: .cargo/config.toml sccache, ui/themes.ts, .cargo/audit.toml can accompany whichever PR is appropriate, but the sccache config must be fixed before merging

Requesting changes. Adding needs-rework.