Priority Level

Medium

Task Summary

Add unit-test coverage for the pure helpers in .agents/tools/structural_impact.py and clean up two latent issues that get cheap to fix in the same pass: the module-level _REPO_ROOT global and the asymmetric .resolve() between changed-files paths and source-file paths.

Follow-up to PR #567 review feedback from @nabinchha.

Technical Details & Implementation Plan

1. Unit tests — add .agents/tools/tests/test_structural_impact.py covering:

   • _get_package — engine / config / interface / external paths, including the data-designer vs data-designer-engine substring-precedence ordering
   • _dedup — collisions on shared label prefixes (regression guard for the bug fixed in bfc5db69)
   • _collect_source_files — smoke test against a tmp dir with mock packages/<name>/src/... layout
   • _unknown_package_dirs — synthetic packages outside _KNOWN_PACKAGE_DIRS

2. CI wiring — .agents/tools/ lives outside the three package test suites, so make test won't pick it up. Decide between (a) a new Makefile target, (b) a step in agentic-ci-pr-review.yml / agentic-ci-daily.yml, or (c) folding into an existing CI workflow. Lean toward (b) since the file's own change cadence is tied to the agentic-CI surface.

3. Thread repo_root through helpers — replace the module-level _REPO_ROOT global + global _REPO_ROOT mutation in main() with explicit parameter passing through _collect_source_files, _unknown_package_dirs, _rel, _changed_files_mode, _full_mode. Removes test-time fragility (no need to mutate a global to vary input) and decouples the otherwise-pure helpers from module state.

4. Symmetric path resolution — once helpers take repo_root explicitly, either drop .resolve() on changed_paths (current _changed_files_mode:198) or also .resolve() paths inside _collect_source_files. Latent today because no symlinks live under packages/, but the symmetric form removes a future-divergence trap.

Investigation / Context

PR #567 review thread: #567 (review by @nabinchha and follow-up at #567 (comment))

Relevant prior fixes that justify the test coverage:

• bfc5db69 — _dedup 30-char prefix-collision bug (pre-merge)
• 55f0bdda — changed_node_ids ID-stability bug (pre-merge)

Both bugs were caught during review; tests would have surfaced them earlier and would protect against the most likely future failure mode (graphify API drift at the loose graphifyy==0.4.23 pin).

Dependencies

None