fix: run cargo test/build --workspace in CI, isolate ponytail's flaky config test - #120
Conversation
…ld --workspace in CI crates/ponytail/src/config.rs: config_dir() ignored env-var overrides (dirs::config_dir() reads the real OS profile dir directly), so defaults_to_full and roundtrip_default_mode raced each other over the real ~/.config/agentflare/ponytail/config.json under cargo's parallel test runner. Add PONYTAIL_CONFIG_DIR_OVERRIDE (same pattern as paths::home()'s AGENTFLARE_HOME_OVERRIDE) and isolate the three tests that read/write persisted config state. .github/workflows/ci.yml: cargo build/test had no --workspace flag, so with a root package present and no default-members set, only the root package was ever built/tested — none of crates/ponytail, crates/caveman, crates/agent-registry, crates/skill-registry, or crates/gateway-registry's own unit tests ran in CI despite PRs showing all-green. Verified locally: --workspace correctly picks up all workspace member tests. This depends on the config.rs fix above — landing --workspace alone would have turned CI red on the pre-existing ponytail flake. Fixes #119 Fixes #118
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCI now runs build and test across the full Rust workspace. Ponytail config resolution accepts a test-only config directory override, and the config tests use a temporary isolated directory for env and persisted-state cases. ChangesCI workspace coverage and ponytail config test isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/ponytail/src/config.rs (1)
91-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale temp dir not cleaned up after test run.
The temp directory created at line 100-102 is only removed pre-emptively at the start of the next run, never after the current one finishes. Not critical since it's OS temp space and gets wiped/reused, but worth a
Drop-based or trailing cleanup for hygiene.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ponytail/src/config.rs` around lines 91 - 107, The test-only isolation helper with_temp_config_dir currently removes the temp directory only before the next run, leaving stale state behind after the current test finishes. Update with_temp_config_dir in config.rs to perform cleanup after the closure returns by ensuring the temp dir is deleted on exit, preferably via a Drop-based guard or equivalent trailing cleanup, while preserving the PONYTAIL_CONFIG_DIR_OVERRIDE setup/teardown behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ponytail/src/config.rs`:
- Around line 91-107: The test-only override in with_temp_config_dir leaks
PONYTAIL_CONFIG_DIR_OVERRIDE if the closure panics or returns early because
cleanup after f() is not panic-safe. Update with_temp_config_dir to use an RAII
guard around the set_var/remove_var lifecycle so the environment variable is
always restored, even when defaults_to_full, reads_env_var, or
roundtrip_default_mode fail. Keep the fix localized to with_temp_config_dir and
the temp config dir handling in config.rs.
---
Nitpick comments:
In `@crates/ponytail/src/config.rs`:
- Around line 91-107: The test-only isolation helper with_temp_config_dir
currently removes the temp directory only before the next run, leaving stale
state behind after the current test finishes. Update with_temp_config_dir in
config.rs to perform cleanup after the closure returns by ensuring the temp dir
is deleted on exit, preferably via a Drop-based guard or equivalent trailing
cleanup, while preserving the PONYTAIL_CONFIG_DIR_OVERRIDE setup/teardown
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1ef6f84-83bf-4dea-9b09-225ae5751fa1
📒 Files selected for processing (2)
.github/workflows/ci.ymlcrates/ponytail/src/config.rs
If the test closure panicked (e.g. a failing assert_eq! in defaults_to_full/reads_env_var/roundtrip_default_mode), the bare remove_var() after f() was skipped, leaking PONYTAIL_CONFIG_DIR_OVERRIDE process-wide for the rest of the test binary. A Drop guard restores it unconditionally, dropped while ENV_TEST_LOCK is still held. (CodeRabbit review on #120)
Summary
PONYTAIL_CONFIG_DIR_OVERRIDEtocrates/ponytail/src/config.rs::config_dir(), mirroring the root crate'spaths::home()/AGENTFLARE_HOME_OVERRIDEpattern. Without it,config_dir()reads the real OS profile directory directly (dirs::config_dir()ignores env-var overrides), sodefaults_to_fullandroundtrip_default_moderaced each other over the real~/.config/agentflare/ponytail/config.jsonunder cargo's parallel test runner. All three tests that touch persisted config state are now isolated via a temp dir.--workspaceto bothcargo build/cargo teststeps in.github/workflows/ci.yml. This repo's rootCargo.tomldefines both a[workspace]and a root[package]with nodefault-membersset — per Cargo's default-member resolution, plaincargo test/cargo buildonly built/tested the root package. None ofcrates/ponytail,crates/caveman,crates/agent-registry,crates/skill-registry, orcrates/gateway-registry's own unit tests ever ran in CI, despite PRs (including Caveman L1 integration: port markdown compressor to Rust #117) showing all-green.These two changes are bundled together because landing
--workspacealone, without the ponytail test-isolation fix, would have turned CI red for an unrelated reason (the pre-existing flake) on every future PR.Test plan
cargo test -p ponytail— 36 tests, ran cleancargo test --workspacex4 in a row locally — no flakes (previously reproduced the race within a handful of runs)cargo test --verbose(no--workspace, matching the old CI command) that it only runs 3 test binaries (root package + its 2 integration tests) — none of the workspace member crates' own unit testsFixes #119
Fixes #118
Summary by CodeRabbit
cargo buildandcargo testacross the full Rust workspace with more verbose output.