Conversation
…Validator Add 10 new tests to the ImportsValidator test module covering: - Three-file and four-file circular dependency chain detection - Depth boundary verification (chain of 5 files stays under limit) - Concurrent cycle detection with shared cache (8 and 10 threads) - Concurrent cycle near MAX_IMPORT_DEPTH boundary - Diamond dependency pattern with missing file at leaf - Filesystem error mid-chain recovery (missing intermediate file) - Concurrent validation with different root file types sharing cache - Non-CLAUDE root file type exclusion from cycle/depth rules All tests are purely additive - no production code changes. Closes #456
Remove redundant comments that restate code context, and normalize the assertion pattern in test_filesystem_error_mid_chain_recovery to use .any() consistently with the rest of the test suite.
- Rename test_depth_at_boundary_no_trigger to test_depth_below_boundary_no_trigger (was testing depth 4, not the true MAX_IMPORT_DEPTH=5 boundary) - Add test_depth_at_boundary_no_trigger for the actual boundary (6 files, depth 5) - Fix test_cycle_detection_four_file_chain assertion message: CC-MEM-003 is absent because cycle detection short-circuits traversal, not due to depth being <=5 - Add message content assertion to test_cycle_detection_three_file_chain to verify format_cycle produces a path containing the cycle files - Fix test_concurrent_diamond_dependency_no_duplicate_diagnostics to assert count=1 (the name promised deduplication but only checked presence) - Interleave CLAUDE.md and SKILL.md thread spawning in test_concurrent_different_roots_shared_cache so both groups race concurrently - Rename test_filesystem_error_mid_chain_recovery to test_missing_transitive_import_stops_traversal (it tests a missing file, not an I/O error)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 test suite for the Highlights
Changelog
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
The pull request introduces 11 new tests for the ImportsValidator, addressing gaps in multi-file circular dependency detection, depth boundary conditions, and concurrent validation safety. The tests are comprehensive, covering complex scenarios like diamond dependencies, interleaved root types (CLAUDE.md vs. SKILL.md), and shared cache contention. The implementation follows existing patterns and correctly verifies the validator's logic, including the suppression of specific rules for non-CLAUDE.md roots and the prevention of duplicate diagnostics. No issues were identified in the added test code.
There was a problem hiding this comment.
Pull request overview
Adds comprehensive inline test coverage for ImportsValidator to address gaps around multi-file cycle detection, depth boundary behavior, and concurrent access to the shared import cache (issue #456).
Changes:
- Added 11 new inline unit tests in
rules::importscovering 3-/4-node cycles, depth boundaries, missing-transitive-import traversal behavior, and several concurrency scenarios using a sharedImportCache. - Updated
CHANGELOG.mdto note the expandedImportsValidatortest coverage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/agnix-core/src/rules/imports.rs | Adds new cycle/depth/concurrency-focused tests for ImportsValidator, including shared-cache multi-thread scenarios. |
| CHANGELOG.md | Documents the addition of the new ImportsValidator tests under “Added”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix misleading comment in test_depth_at_boundary_no_trigger that referenced a nonexistent file 'e'; rewrite to match actual chain (CLAUDE -> a -> b -> c -> d -> leaf) and describe depth check precisely - Add channel-based recv_timeout to test_concurrent_cycle_detection_no_deadlock so a deadlock regression fails CI within 10s instead of hanging indefinitely - Fix CHANGELOG wording: above-boundary depth condition is covered by an existing test, not newly added; update to say 'at and below' with a note about complementing existing coverage
Summary
ImportsValidatorinline test module covering gaps identified in issue Add concurrency tests for ImportsValidator circular dependency detection #456Tests Added
Multi-file circular chains
test_cycle_detection_three_file_chain- A→B→C→A (3-node cycle), verifies CC-MEM-002 fires and message contains the full cycle pathtest_cycle_detection_four_file_chain- A→B→C→D→A (4-node cycle), verifies CC-MEM-002 and cycle detection short-circuits before depth checktest_cycle_detection_three_file_chain_with_non_claude_root- same 3-file cycle with SKILL.md root, verifies CC-MEM-002/003 are suppressedDepth boundary conditions
test_depth_below_boundary_no_trigger- 5-file chain (depth 4), well below MAX_IMPORT_DEPTH=5test_depth_at_boundary_no_trigger- 6-file chain (depth 5), exactly at MAX_IMPORT_DEPTH, verifies CC-MEM-003 does NOT fireConcurrent cache access
test_concurrent_cycle_detection_no_deadlock- 8 threads sharingArc<RwLock<HashMap>>on a 2-file cycletest_concurrent_three_file_cycle_shared_cache- 10 threads on 3-file cycle, verifies cache populated for all 3 filestest_concurrent_cycle_near_depth_limit- 5 threads on 5-file cycle at depth 4, verifies CC-MEM-002 fires and CC-MEM-003 does nottest_concurrent_diamond_dependency_no_duplicate_diagnostics- diamond dependency (CLAUDE→{b,c}→shared→missing), asserts exactly 1 missing-import diagnostic per threadtest_concurrent_different_roots_shared_cache- interleaved CLAUDE.md and SKILL.md threads sharing one cache; CLAUDE threads get CC-MEM-002, SKILL threads do notMid-chain traversal
test_missing_transitive_import_stops_traversal- missing b.md in chain a.md→b.md stops recursion, c.md never referencedTest Plan
cargo test --lib -p agnix-core -- rules::imports::tests --quiet→ 65/65 passcargo test -p agnix-core→ full suite passes (2916 tests)cargo build -p agnix-core→ zero warningsRelated Issues
Closes #456