fix: make dependency graph independent of walk order - #37
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d6b4cfd15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed all four review findings in 6ff0dad by tracking relative import bindings separately from source/package nodes and rebinding them to the deterministic best source as files are discovered. Added regressions for multiple index aliases, extension-vs-index walk order, bare-package collisions, and unsupported index files. Verified with cargo fmt, the full Rust test suite (66 unit tests + integration tests), clippy with warnings denied, focused graph tests, and the npm installer test. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ff0dad3a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the follow-up findings in 168c7a4. Relative imports are now indexed by candidate path, so adding a source revisits only affected bindings; edge reference counts make each rebind constant-time instead of rescanning all imports. External specifiers also use a separate node map, including exact-string collisions with source paths. Added a regression for the exact bare-specifier collision; graph tests and clippy with warnings denied pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 168c7a413c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfe5593d39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if let Some(target_idx) = self.find_best_source(&source_candidates) { | ||
| target_idx | ||
| } else { | ||
| self.get_or_create_external_node(&resolved) |
There was a problem hiding this comment.
When a Python file imports a module whose sibling basename belongs to another language—for example, src/main.py contains import utils and the project has src/utils.ts—find_best_source uses the generic extension order and rebinds the import to the TypeScript file (and it prefers that file even if src/utils.py also exists). This makes find_related report a false cross-language dependency; Python candidates should only match Python modules/packages before falling back to an external node.
Useful? React with 👍 / 👎.
| self.decrement_edge(source, old_target); | ||
| self.increment_edge(source, new_target); | ||
| self.import_edges[edge_index].target = new_target; |
There was a problem hiding this comment.
When the importer is walked before a local Python module—for example, src/main.py imports utils before src/utils.py is added—the old target here is the provisional external node, but moving the edge leaves its external_nodes entry behind. As a result, all_nodes() and stats() include an isolated, nonexistent external utils node, while adding the same files in the opposite order never creates it, so the graph summaries remain walk-order dependent; retire the external entry once no import edge targets it.
Useful? React with 👍 / 👎.
1 participant
Summary
Problem
The graph builder resolves imports such as
./routertosrc/router. When the importer is walked beforesrc/router.ts, it creates a placeholder node. Adding the real file later previously created a second node, so reverse and transitive dependency queries missed the edge depending on filesystem traversal order.Verification
cargo fmt --checkcargo test(all passing; one existing ignored semantic integration test)cargo clippy --all-targets --all-features -- -D warningsnpm test --prefix npm