fix: make parity test resilient with workspace_root() helper - #153
Conversation
Summary of ChangesHello @avifenesh, 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 significantly enhances the robustness and reliability of path resolution within the project's test suite. By replacing a brittle, hardcoded ancestor lookup with a dynamic and cached 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
|
![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 workspace_root() helper function to dynamically find the workspace root, replacing a brittle hardcoded path traversal. This is a solid improvement for making the tests more resilient to directory structure changes. The use of OnceLock for caching is also a good choice for performance. I have one suggestion to make the workspace detection logic within the new helper even more robust.
| if content.contains("[workspace]") || content.contains("[workspace.") { | ||
| return ancestor.to_path_buf(); | ||
| } |
There was a problem hiding this comment.
The current check using content.contains("[workspace]") is a bit brittle. It could lead to false positives if the string [workspace] appears in a comment or a string literal within Cargo.toml, or incorrectly match unrelated table names like [workspace-foo]. A more robust approach is to check for this pattern at the beginning of a trimmed line and ensure it's a valid table definition, which is how TOML tables are defined.
if content.lines().any(|line| {
let trimmed = line.trim();
if trimmed.starts_with("[workspace]") {
// Check what comes after `[workspace]` to distinguish from e.g. `[workspace-foo]`
let rest = &trimmed["[workspace]".len()..];
return rest.is_empty() || rest.starts_with(|c: char| c.is_whitespace() || c == '#' || c == '.');
}
false
}) {
return ancestor.to_path_buf();
}
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Pull request overview
This PR makes the CLAUDE/AGENTS parity and fixtures-based tests robust to different repository layouts by introducing a reusable workspace-root discovery helper. It replaces brittle path assumptions with dynamic detection based on ancestor Cargo.toml files containing a workspace section.
Changes:
- Added a
workspace_root()test helper incrates/agnix-core/src/lib.rsthat walks ancestor directories fromCARGO_MANIFEST_DIR, looking for aCargo.tomlcontaining[workspace]or[workspace.], caching the result withOnceLock. - Updated
test_repo_agents_md_matches_claude_md()to useworkspace_root()instead of hard-coded.ancestors().nth(2)-based repo root inference. - Updated
get_fixtures_dir()in the same test module to derivetests/fixturesfromworkspace_root(), aligning fixture path resolution with the new helper. - Documented the behavioral fix in
CHANGELOG.mdunder the “Fixed” section.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/agnix-core/src/lib.rs |
Adds a cached workspace_root() helper in the test module and wires it into the CLAUDE/AGENTS parity test and fixtures-directory helper to remove brittle ancestor-based path logic. |
CHANGELOG.md |
Notes that the CLAUDE/AGENTS parity test now uses dynamic workspace root detection and explains the new helper’s behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn workspace_root() -> &'static Path { | ||
| use std::sync::OnceLock; | ||
|
|
||
| static ROOT: OnceLock<PathBuf> = OnceLock::new(); | ||
| ROOT.get_or_init(|| { | ||
| let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
| for ancestor in manifest_dir.ancestors() { | ||
| let cargo_toml = ancestor.join("Cargo.toml"); | ||
| if let Ok(content) = std::fs::read_to_string(&cargo_toml) { | ||
| if content.contains("[workspace]") || content.contains("[workspace.") { | ||
| return ancestor.to_path_buf(); | ||
| } | ||
| } | ||
| } | ||
| panic!( | ||
| "Failed to locate workspace root from CARGO_MANIFEST_DIR={}", | ||
| manifest_dir.display() | ||
| ); | ||
| }) | ||
| .as_path() | ||
| } |
There was a problem hiding this comment.
We now have three nearly identical workspace_root implementations across the test suite (this one plus the helpers in crates/agnix-cli/tests/cli_integration.rs and crates/agnix-cli/tests/fixture_family_cli.rs), which risks these definitions drifting if the workspace-detection logic ever changes. Consider centralizing this helper in a shared test utility (or reusing the existing one) so that workspace-root resolution logic is defined in a single place.
| let claude = std::fs::read_to_string(repo_root.join("CLAUDE.md")).unwrap(); | ||
| let agents = std::fs::read_to_string(repo_root.join("AGENTS.md")).unwrap(); |
There was a problem hiding this comment.
This test still uses bare unwrap() when reading CLAUDE.md and AGENTS.md, which will panic with a generic message and loses the opportunity to show which path failed and why; the repo already uses the unwrap_or_else(|e| panic!(...)) pattern for file reads in tests (for example crates/agnix-cli/tests/rule_parity.rs:53-59, 61-64) to provide clearer diagnostics. To keep failure output consistent and more actionable when these files are missing or unreadable (one of the goals in issue #90), consider switching these unwrap() calls to expect or unwrap_or_else with messages that include the resolved path and underlying error.
| let claude = std::fs::read_to_string(repo_root.join("CLAUDE.md")).unwrap(); | |
| let agents = std::fs::read_to_string(repo_root.join("AGENTS.md")).unwrap(); | |
| let claude_path = repo_root.join("CLAUDE.md"); | |
| let claude = std::fs::read_to_string(&claude_path).unwrap_or_else(|e| { | |
| panic!("Failed to read CLAUDE.md at {}: {e}", claude_path.display()); | |
| }); | |
| let agents_path = repo_root.join("AGENTS.md"); | |
| let agents = std::fs::read_to_string(&agents_path).unwrap_or_else(|e| { | |
| panic!("Failed to read AGENTS.md at {}: {e}", agents_path.display()); | |
| }); |
Addresses Copilot review feedback.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Summary
.ancestors().nth(2)pattern with dynamicworkspace_root()helperOnceLockfor caching workspace root detectionChanges
workspace_root()helper function that searches for[workspace]in ancestor Cargo.toml filestest_repo_agents_md_matches_claude_md()to use the new helperget_fixtures_dir()to use the new helperTest plan
Closes #90