Resync with graphify-py @ 92e682f (v0.9.0)#31
Open
rblaine95 wants to merge 4 commits into
Open
Conversation
Port the applicable feature, security, and bug-fix changes from
`graphify-py` v0.8.49 -> v0.9.0 (38 upstream commits) and bump the
workspace to `0.9.0`.
Ports:
- Node IDs are now the full repo-relative path with the extension
dropped and separators collapsed to `_`, so same-named files in
different directories stay distinct (#1504, #1509). Legacy graphs
read as-is with a one-line rebuild nudge.
- Cross-file type-reference stubs are sourceless and carry an
`origin_file` so same-label stubs from different files do not merge
(#1462, #1500).
- New C# cross-file type resolver plus `enum`/`struct`/`record` node
types (#1466).
- Java `enum`/annotation node types, generic-argument and field-type
references (#1512, #1510, #1485, #1487).
- Objective-C header routing, generic property types, class-method
selectors, and `@import` handling (#1475).
- New WPF/XAML extractor: `x:Class`, named controls, `{Binding}`
paths, and ViewModel / code-behind resolution (#1460, #1473).
- Vue single-file-component extraction and `.metal` routing through
the C++ extractor (#1468, #1480).
- Obsidian export only overwrites notes it owns via a manifest, the
Canvas uses a `ceil(sqrt(n))` grid, and note/canvas filenames are
de-duplicated case-insensitively (#1506, #1452, #1453).
- Wiki articles link with portable relative Markdown links instead of
`[[wikilinks]]`, with case-folded slug dedup (#1444, #1453).
- LLM requests send `stream: false`, and `kimi`/`gemini`/`deepseek`
honour a bare `*_BASE_URL` (#1223, #1458).
- CLI `--timing` on `extract`/`cluster-only`/`label`, and
`--missing-only` on `cluster-only`/`label` (#1490, #1481).
- `reflect --if-stale` also weighs the graph sidecars, the Read|Glob
hook matches a file's real trailing extension, and `affected` /
`explain` resolve a source-file path to its file-level node
(#1470, #1463, #1503).
Glory to the Omnissiah
Resolve the 24 findings from the first CodeRabbit pass on the resync.
Genuine divergences from graphify-py are fixed; findings that match
graphify-py are kept and the rationale recorded in an in-code comment
(CodeRabbit reads code, not commit messages).
Fixes:
- `extract()` now clears the thread-local XAML `ViewModel` class cache
per run (generation-gated across the rayon pool), matching graphify-py
`_XAML_CSHARP_CLASS_CACHE.clear()`; a repeated in-process run re-scans
`.cs` instead of serving stale members. Adds a regression test.
- Obsidian manifest write propagates IO errors instead of swallowing
them, like the sibling note writes and graphify-py `.write_text`.
- `ingest` C-family map includes `.cxx` (extracted as C++ and present in
`analyze`'s family map; graphify-py's build map omits it).
- Legacy-id alias registration is skipped unless the node id shares the
new stem, so a mismatched prefix no longer maps unrelated edges.
- `collect_cs_files` uses `entry.file_type()` so a symlinked directory
cannot loop forever.
- `is_objc_header` reads only the 256 KiB it inspects.
- Vue dynamic-import recovery scans the masked source, so an `import()`
in `<template>`/`<style>` is not edged.
Kept as graphify-py parity (documented in code):
- Sourceless stub `source_location` stays `Some("")` (graphify-py emits
`""`); wiki resolver keys by label and reserves `index` only in the
resolver; C# merges a file's namespaces into one scope; the direct
`<Binding Path>` element emits no generated-member edge; validation
runs once before the semantic re-key.
Also strengthens the affected test assertions (exact set/count checks,
`origin_file` and canonical-id checks, real absolute/relative paths, raw
event-edge count, loud directory-walk errors).
By the will of the Machine God
📝 WalkthroughWalkthroughThis release bumps the workspace to 0.9.0, changes node IDs to full repo-relative stems, adds ChangesGraphify 0.9.0 release
Sequence Diagram(s)sequenceDiagram
participant extract_xaml
participant code_behind_resolver
participant viewmodel_resolver
participant cs_class_lookup
participant toolkit_extractor
participant XamlBuilder
extract_xaml->>code_behind_resolver: resolve x:Class and event handlers
extract_xaml->>viewmodel_resolver: detect DataContext / Prism / naming
viewmodel_resolver->>cs_class_lookup: scan *ViewModel classes
extract_xaml->>toolkit_extractor: emit ObservableProperty / RelayCommand members
extract_xaml->>XamlBuilder: deduplicate nodes and edges
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/graphify-extract/src/extractors/powershell/mod.rs (1)
243-258: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep PowerShell type-reference placeholders sourceless.
PsRefCtx::ensure_named_nodematerializes unresolved type references, but this still records them as if they were real declarations inself.str_pathand leavesorigin_fileempty. Align it with the new stub contract so cross-file resolution can distinguish provenance without treating the reference site as a definition.Proposed fix
- fn ensure_named_node(&mut self, name: &str, line: usize) -> String { + fn ensure_named_node(&mut self, name: &str, _line: usize) -> String { let nid1 = make_id(&[self.stem, name]); if self.seen_ids.contains(&nid1) { return nid1; } let nid2 = make_id1(name); if self.seen_ids.insert(nid2.clone()) { self.nodes.push(Node { id: nid2.clone(), label: name.to_string(), file_type: "code".to_string(), - source_file: self.str_path.to_string(), - source_location: Some(format!("L{line}")), + source_file: String::new(), + source_location: None, metadata: None, - origin_file: None, + origin_file: Some(self.str_path.to_string()), }); } nid2 }🤖 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/graphify-extract/src/extractors/powershell/mod.rs` around lines 243 - 258, Keep unresolved PowerShell type-reference placeholders from being recorded as real declarations in PsRefCtx::ensure_named_node. Update the placeholder node creation path so these sourceless references do not use self.str_path as their source_file and instead carry the new stub provenance contract, with origin_file populated appropriately to distinguish them from actual definitions. Make the change in the ensure_named_node flow where Node is pushed for the fallback id.
🧹 Nitpick comments (10)
crates/graphify-build/src/migrate.rs (1)
34-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[must_use]to the pure ID helpers.make_id,make_id1,file_stem,old_file_stems, andsemantic_id_remapall return owned values with no side effects, so dropped results stay silent here.graph_has_legacy_idsalready follows this pattern.🤖 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/graphify-build/src/migrate.rs` around lines 34 - 53, Add #[must_use] to the pure ID helper functions so dropped return values are flagged: update make_id, make_id1, file_stem, old_file_stems, and semantic_id_remap to use the same pattern already used by graph_has_legacy_ids. Keep the functions’ behavior unchanged; only add the attribute on these named helpers so callers are warned when the returned owned ID values are ignored.Source: Coding guidelines
crates/graphify-extract/tests/parity_languages.rs (1)
185-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
Result-returning tests over repeatedexpect(...).These new filesystem tests are correctly isolated with
tempfile::tempdir(), but the repeatedexpect("tempdir")/expect("write")/expect("mkdir")pattern is drifting from the repo rule fortests/**/*.rs. Please switch these tests to returnResultand use?for setup so they stay aligned with the workspace’s clippy/test conventions. As per coding guidelines,tests/**/*.rs: “In test files, prefer?operator returningResult.”Also applies to: 202-211, 235-241, 250-256, 282-290, 718-729, 763-774, 791-797, 814-823, 992-996
🤖 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/graphify-extract/tests/parity_languages.rs` around lines 185 - 191, The filesystem tests in `parity_languages.rs` are still using repeated `expect(...)` for setup, which violates the repo convention for `tests/**/*.rs`. Update the affected test functions to return `Result` and replace `tempdir`, `write`, `mkdir`, and similar setup `expect` calls with `?`, keeping the test logic in the existing helper/test names so the setup failures propagate cleanly.Source: Coding guidelines
crates/graphify-extract/tests/csharp_type_resolution.rs (1)
3-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Result-returning tests instead of blanketunwrap/expectallowances.This suite opts out of clippy at file scope and then uses
unwrap/expectthroughout. The test guideline here is to prefer?and keep any unavoidable allowance narrow, so this pattern will spread the wrong default if it lands as-is.Proposed pattern
-#![allow(clippy::expect_used, clippy::unwrap_used)] +use std::error::Error; -fn write_file(root: &Path, rel: &str, text: &str) -> PathBuf { +fn write_file(root: &Path, rel: &str, text: &str) -> std::io::Result<PathBuf> { let p = root.join(rel); - std::fs::create_dir_all(p.parent().expect("parent")).expect("create_dir_all"); - std::fs::write(&p, text).expect("write"); - p + if let Some(parent) = p.parent() { + std::fs::create_dir_all(parent)?; + } + std::fs::write(&p, text)?; + Ok(p) } #[test] -fn csharp_cross_file_inherits_resolves_to_real_def() { - let tmp = tempfile::tempdir().unwrap(); +fn csharp_cross_file_inherits_resolves_to_real_def() -> Result<(), Box<dyn Error>> { + let tmp = tempfile::tempdir()?; // ... - let core = write_file(...); + let core = write_file(...)?; // ... + Ok(()) }As per coding guidelines,
tests/**/*.rs: "In test files, prefer?operator returningResult."Also applies to: 49-52, 76-77, 119-120, 138-139, 179-180, 218-219, 252-253
🤖 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/graphify-extract/tests/csharp_type_resolution.rs` around lines 3 - 17, Replace the file-wide clippy allowance and the `unwrap`/`expect`-based test style in `csharp_type_resolution.rs` with `Result`-returning tests that use `?` for fallible setup and assertions. Update `write_file` and the affected test cases to propagate errors instead of calling `expect`, and keep any clippy allowance narrowly scoped only if truly needed. Use the test functions and helper `write_file` as the main places to apply this pattern.Source: Coding guidelines
crates/graphify-export/tests/vault_safety.rs (1)
7-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the file-wide
unwrap/expectsuppression here.This test file can return
Resultfrom each test and use?; the blanket allow weakens the lint signal more broadly than necessary. As per coding guidelines, in test files prefer?operator returningResult, and the file-top#![allow(clippy::expect_used, clippy::unwrap_used)]pattern is reserved fortests/parity.rs.🤖 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/graphify-export/tests/vault_safety.rs` around lines 7 - 12, Remove the file-wide Clippy suppression for unwrap/expect in this test module and update the tests in vault_safety.rs to return Result so they can use the ? operator instead of unwrap/expect. Keep only the narrow lint allowances that are still needed for this file, and follow the existing test style used elsewhere rather than the blanket allow pattern reserved for parity tests.Source: Coding guidelines
crates/graphify-build/src/ingest.rs (1)
308-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark
snapshot_source_filesas#[must_use].This helper is pure and returns an owned map, so it should be annotated
#[must_use]to match the workspace Rust rules.As per coding guidelines, "Use
#[must_use]attribute on pure functions returning owned values."♻️ Proposed fix
+#[must_use] fn snapshot_source_files(graph: &Graph) -> IndexMap<String, String> {🤖 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/graphify-build/src/ingest.rs` around lines 308 - 325, Mark the pure helper `snapshot_source_files` with `#[must_use]` so callers are warned when the returned `IndexMap<String, String>` is ignored. Add the attribute directly on the function definition in `ingest.rs`, keeping the existing behavior unchanged and matching the workspace rule for pure functions returning owned values.Source: Coding guidelines
crates/graphify-extract/src/extractors/svelte.rs (1)
394-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark
vue_mask_non_scriptas#[must_use].This helper is pure and returns owned data, so it should carry
#[must_use]per the Rust workspace rules.As per coding guidelines, "Use
#[must_use]attribute on pure functions returning owned values."♻️ Proposed fix
+#[must_use] fn vue_mask_non_script(src: &str) -> (String, Option<String>) {🤖 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/graphify-extract/src/extractors/svelte.rs` around lines 394 - 421, Mark the pure helper `vue_mask_non_script` with `#[must_use]` so callers are warned when its returned masked source and language value are ignored. Add the attribute directly on the function declaration in the Svelte extractor module, keeping the existing behavior unchanged. This should align with the workspace rule for pure functions returning owned values.Source: Coding guidelines
tests/cli_commands.rs (1)
881-899: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Result+?in these new integration tests.The new test paths are all fallible filesystem operations, so
fn ...() -> Result<(), _>reads better than stackingunwrap()s and aligns with the repo’s test guidance. As per coding guidelines,tests/**/*.rs: “In test files, prefer?operator returningResult.” Based on learnings, this workspace avoids.unwrap()usage.Also applies to: 901-930
🤖 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 `@tests/cli_commands.rs` around lines 881 - 899, The new integration test uses fallible filesystem setup with unwrap chains instead of the repo’s preferred Result-based style. Update `cluster_only_timing_emits_stage_lines` to return `Result<(), _>` and propagate the `tempdir`, path creation, and any other fallible calls with `?` rather than `unwrap()`. Apply the same pattern to the related new tests in the surrounding block so they follow the existing test guidance and avoid panics.Sources: Coding guidelines, Learnings
crates/graphify-reflect/tests/parity.rs (1)
587-596: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
Result-returning tests over newunwrap()calls.These changed tests are doing fallible tempdir/fs setup already, so
?is the better fit here and matches the repo’s test-file guidance. As per coding guidelines,tests/**/*.rs: “In test files, prefer?operator returningResult.” Based on learnings, this workspace avoids.unwrap()usage.Also applies to: 641-682
🤖 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/graphify-reflect/tests/parity.rs` around lines 587 - 596, The new test setup in lessons_fresh_missing_output_is_not_fresh and the other affected tests still uses unwrap() for fallible tempdir/filesystem operations, which should be replaced with Result-returning tests. Update the test functions in parity.rs to return a Result and propagate errors with ? instead of unwrap(), following the existing test helper pattern and keeping the assertions intact in functions like lessons_fresh_missing_output_is_not_fresh and the other changed test cases.Sources: Coding guidelines, Learnings
tests/cli.rs (1)
604-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
Result-returning tests here instead ofunwrap().These integration tests already do fallible filesystem setup, so returning
Result<()>and using?keeps failures cleaner and matches the repo’s Rust test style. As per coding guidelines,tests/**/*.rs: “In test files, prefer?operator returningResult.” Based on learnings, “do not use.unwrap()anywhere” in this workspace.Also applies to: 643-674
🤖 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 `@tests/cli.rs` around lines 604 - 639, The test `explain_source_file_path_prefers_file_level_node` still uses `unwrap()` for fallible setup and assertions, which conflicts with the repo’s test style. Change this test to return `Result<()>` and replace the `tempfile::tempdir`, `fs::write`, and any other fallible calls with `?`, following the same pattern used in nearby tests in `tests/cli.rs`; keep the existing assertions and add the final `Ok(())` return. Also apply the same `Result`-returning, no-`unwrap()` cleanup to the related tests referenced in this block.Sources: Coding guidelines, Learnings
crates/graphify-extract/src/postprocess.rs (1)
39-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the helper as
#[must_use].
node_disambiguation_source_keyis a pure helper returning an ownedString, so it should be annotated to prevent accidental dropped results. As per coding guidelines, "Use#[must_use]attribute on pure functions returning owned values".🤖 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/graphify-extract/src/postprocess.rs` around lines 39 - 50, Add `#[must_use]` to `node_disambiguation_source_key` in `postprocess.rs`. This helper is a pure function returning an owned `String`, so annotate it to prevent callers from accidentally ignoring the result. Keep the function behavior unchanged and apply the attribute directly to the `node_disambiguation_source_key` definition alongside the existing helper documentation.Source: Coding guidelines
🤖 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/graphify-affected/tests/parity.rs`:
- Around line 381-384: The assertion in the parity test is still checking the
pre-migration file-node ID instead of the ID produced after
`load_graph()`/`build_from_json()`. Update the `resolve_seed` expectation for
`app/api/example/route.ts/` to use the migrated node ID documented by the
sibling test, and keep the test aligned with the post-load graph keying
behavior.
In `@crates/graphify-export/src/obsidian.rs`:
- Around line 653-669: Reject symlinked write targets in owned_write before
calling std::fs::write: the current owned-path check only protects against
overwriting unowned files but still allows a symlinked target to escape
output_dir. Update owned_write (and any helper it uses) to inspect the target
with symlink_metadata, reject if the target or any relevant parent component is
a symlink, or otherwise canonicalize and verify the resolved path stays under
the output_dir root before writing. Use the owned_write function and its
target/parent path handling as the place to enforce this.
In `@crates/graphify-extract/src/extractors/dotnet/xaml.rs`:
- Around line 76-80: `with_xaml_extract_root` only restores
`XAML_ACTIVE_EXTRACT_ROOT` after `f()` returns normally, so a panic can leak the
temporary root into later work on the same thread. Update this helper to restore
the previous thread-local value even when `f` unwinds, using an unwind-safe
guard/RAII pattern around the `f()` call and ensuring the original value is
always put back before exit.
In `@crates/graphify-extract/src/extractors/multi/cache.rs`:
- Around line 121-125: The AST cache in multi/cache.rs is returning stale .xaml
results before dependent C# inputs are reconsidered, so the cache key or bypass
rules need to account for ViewModel/member resolution dependencies. Update the
JS_CACHE_BYPASS_SUFFIXES handling or the cache lookup path in the relevant
extractor logic so .xaml either bypasses caching like other dynamic files or
includes sibling C# code-behind inputs in the key, and make sure
clear_xaml_csharp_class_cache() is applied in the path that can still hit cached
AST results.
In `@crates/graphify-extract/src/extractors/multi/csharp.rs`:
- Around line 39-50: Update collect_csharp_scope and the C# extraction flow so
nested namespace_declaration/file_scoped_namespace_declaration blocks are
combined into a single fully qualified namespace instead of separate entries.
Then adjust the fqn_to_id registration logic and any lookup/repointing paths
that consume own_ns so types inside nested namespaces are emitted and resolved
as A.B.T rather than A.T or B.T. Use the existing collect_csharp_scope,
fqn_to_id, and related C# extractor paths to ensure the definition map includes
nested-namespace types consistently.
In `@crates/graphify-extract/src/extractors/objc.rs`:
- Around line 575-603: The Objective-C selector reconstruction in objc.rs is
dropping separator characters, so distinct selectors can collapse into the same
identifier; update the method/declaration parsing around the selector assembly
logic in the code that builds method names from identifier children so colon
structure is preserved before make_id. Apply the same fix in both the method
declaration path and the call-site reconstruction path referenced by the
existing selector concatenation code, using the selector-building logic in the
ObjC extractor rather than raw parts.concat().
In `@crates/graphify-extract/src/types.rs`:
- Around line 22-27: Normalize the new `origin_file` field in `types::...`
before graph serialization, since it can currently emit absolute machine-local
paths and break determinism. Update the same root-based path handling used for
`source_file` in the extraction/JSON output path so `origin_file` is relativized
too, or exclude it from emitted graph data if it should remain internal-only.
Use the `origin_file` field in `types.rs` and the existing normalization logic
around graph emission to keep both fields consistent.
In `@crates/graphify-reflect/tests/parity.rs`:
- Around line 651-682: The `lessons_fresh_false_when_analysis_or_labels_newer`
test only covers the `.graphify_analysis.json` sidecar case, so it does not
verify behavior when `.graphify_labels.json` is newer. Update the
`lessons_fresh` test coverage by either adding a second assertion that sets
`labels` newer than `LESSONS.md`, or split the current test into two focused
tests so both sidecars are explicitly exercised.
In `@crates/graphify-serve/src/graph.rs`:
- Around line 781-818: The new source path matching in graph search is too lossy
because search_tokens(...).join(" ") collapses separators and can treat
different file paths as identical. Update the matching logic in the graph search
routine that builds source_exact, exact, prefix, and substring so that
source_file is compared using a normalized full path string before any
token-based matching. Keep tokenized matching only as a fallback for node label
and ID comparisons, and preserve the preferred/source_exact ordering behavior in
the same flow.
In `@crates/graphify-wiki/src/generate.rs`:
- Around line 75-80: The wiki link resolver in generate.rs is colliding because
`resolver` uses only display text, so duplicate labels and the special `"index"`
entry can overwrite or misroute real articles. Update the resolution logic in
the code around `resolver.insert(...)` and the article-generation flow to use a
stable per-article identifier like `cid`/`nid`, or split special-page entries
into a separate namespace, so community/god-node articles and `index` each
resolve deterministically without clobbering each other.
- Around line 96-127: Keep the index generation aligned with the actual written
god-node articles. In `generate.rs`, the `god_articles` collection is filtered
to only nodes present in `graph`, but `index_md(...)` still uses the original
`god_nodes_data` slice, so update the `index_md` call (or its input) to use the
filtered god-node set derived from the same loop that builds `god_articles`. Use
the existing `god_articles`/`write_god_node_articles` flow and the `index_md`
function to ensure `index.md` only advertises nodes that actually got articles
written.
In `@src/cli/args.rs`:
- Around line 172-178: Update the `label` command help text in `Args` so it no
longer claims every community name is always refreshed, since `missing_only`
changes that behavior. Adjust the variant description associated with `graphify
label` to mention the `--missing-only` option and reflect that only unnamed or
placeholder communities are renamed when it is set.
In `@src/cli/cluster_only.rs`:
- Around line 190-191: The `cluster_only` flow is treating a failed
`read_existing_labels` call as an empty label set when `opts.missing_only` is
enabled, which can cause `existing` to be empty and later overwrite a broken
`.graphify_labels.json` with relabeled data. Update the `existing`
initialization in `cluster_only` to handle `read_existing_labels` errors
explicitly instead of using `unwrap_or_default`, and make the `--missing-only`
path preserve the current labels file on read failure the same way the
non-`--missing-only` path does. Verify the relabel/write logic around the later
community output block only runs when the labels file was successfully read or
intentionally absent.
In `@src/cli/extract.rs`:
- Around line 69-71: The orchestration comment in the CLI entry point is out of
sync with the actual flow because graph export still happens before
run_analysis_phase(). Update the comment near the main orchestration in extract
to match the real execution order, or adjust the code order if the intended flow
is detect → AST → semantic → build → cluster → analyze → export, and keep the
wording aligned with the functions that perform export and analysis.
---
Outside diff comments:
In `@crates/graphify-extract/src/extractors/powershell/mod.rs`:
- Around line 243-258: Keep unresolved PowerShell type-reference placeholders
from being recorded as real declarations in PsRefCtx::ensure_named_node. Update
the placeholder node creation path so these sourceless references do not use
self.str_path as their source_file and instead carry the new stub provenance
contract, with origin_file populated appropriately to distinguish them from
actual definitions. Make the change in the ensure_named_node flow where Node is
pushed for the fallback id.
---
Nitpick comments:
In `@crates/graphify-build/src/ingest.rs`:
- Around line 308-325: Mark the pure helper `snapshot_source_files` with
`#[must_use]` so callers are warned when the returned `IndexMap<String, String>`
is ignored. Add the attribute directly on the function definition in
`ingest.rs`, keeping the existing behavior unchanged and matching the workspace
rule for pure functions returning owned values.
In `@crates/graphify-build/src/migrate.rs`:
- Around line 34-53: Add #[must_use] to the pure ID helper functions so dropped
return values are flagged: update make_id, make_id1, file_stem, old_file_stems,
and semantic_id_remap to use the same pattern already used by
graph_has_legacy_ids. Keep the functions’ behavior unchanged; only add the
attribute on these named helpers so callers are warned when the returned owned
ID values are ignored.
In `@crates/graphify-export/tests/vault_safety.rs`:
- Around line 7-12: Remove the file-wide Clippy suppression for unwrap/expect in
this test module and update the tests in vault_safety.rs to return Result so
they can use the ? operator instead of unwrap/expect. Keep only the narrow lint
allowances that are still needed for this file, and follow the existing test
style used elsewhere rather than the blanket allow pattern reserved for parity
tests.
In `@crates/graphify-extract/src/extractors/svelte.rs`:
- Around line 394-421: Mark the pure helper `vue_mask_non_script` with
`#[must_use]` so callers are warned when its returned masked source and language
value are ignored. Add the attribute directly on the function declaration in the
Svelte extractor module, keeping the existing behavior unchanged. This should
align with the workspace rule for pure functions returning owned values.
In `@crates/graphify-extract/src/postprocess.rs`:
- Around line 39-50: Add `#[must_use]` to `node_disambiguation_source_key` in
`postprocess.rs`. This helper is a pure function returning an owned `String`, so
annotate it to prevent callers from accidentally ignoring the result. Keep the
function behavior unchanged and apply the attribute directly to the
`node_disambiguation_source_key` definition alongside the existing helper
documentation.
In `@crates/graphify-extract/tests/csharp_type_resolution.rs`:
- Around line 3-17: Replace the file-wide clippy allowance and the
`unwrap`/`expect`-based test style in `csharp_type_resolution.rs` with
`Result`-returning tests that use `?` for fallible setup and assertions. Update
`write_file` and the affected test cases to propagate errors instead of calling
`expect`, and keep any clippy allowance narrowly scoped only if truly needed.
Use the test functions and helper `write_file` as the main places to apply this
pattern.
In `@crates/graphify-extract/tests/parity_languages.rs`:
- Around line 185-191: The filesystem tests in `parity_languages.rs` are still
using repeated `expect(...)` for setup, which violates the repo convention for
`tests/**/*.rs`. Update the affected test functions to return `Result` and
replace `tempdir`, `write`, `mkdir`, and similar setup `expect` calls with `?`,
keeping the test logic in the existing helper/test names so the setup failures
propagate cleanly.
In `@crates/graphify-reflect/tests/parity.rs`:
- Around line 587-596: The new test setup in
lessons_fresh_missing_output_is_not_fresh and the other affected tests still
uses unwrap() for fallible tempdir/filesystem operations, which should be
replaced with Result-returning tests. Update the test functions in parity.rs to
return a Result and propagate errors with ? instead of unwrap(), following the
existing test helper pattern and keeping the assertions intact in functions like
lessons_fresh_missing_output_is_not_fresh and the other changed test cases.
In `@tests/cli_commands.rs`:
- Around line 881-899: The new integration test uses fallible filesystem setup
with unwrap chains instead of the repo’s preferred Result-based style. Update
`cluster_only_timing_emits_stage_lines` to return `Result<(), _>` and propagate
the `tempdir`, path creation, and any other fallible calls with `?` rather than
`unwrap()`. Apply the same pattern to the related new tests in the surrounding
block so they follow the existing test guidance and avoid panics.
In `@tests/cli.rs`:
- Around line 604-639: The test
`explain_source_file_path_prefers_file_level_node` still uses `unwrap()` for
fallible setup and assertions, which conflicts with the repo’s test style.
Change this test to return `Result<()>` and replace the `tempfile::tempdir`,
`fs::write`, and any other fallible calls with `?`, following the same pattern
used in nearby tests in `tests/cli.rs`; keep the existing assertions and add the
final `Ok(())` return. Also apply the same `Result`-returning, no-`unwrap()`
cleanup to the related tests referenced in this block.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce4b49b0-9ac9-4c71-92d5-d429c0e5afc2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (128)
Cargo.tomlREADME.mdUSAGE.mdcrates/graphify-affected/src/lib.rscrates/graphify-affected/tests/parity.rscrates/graphify-analyze/src/classify.rscrates/graphify-build/src/build_fn.rscrates/graphify-build/src/ingest.rscrates/graphify-build/src/lib.rscrates/graphify-build/src/migrate.rscrates/graphify-build/tests/parity.rscrates/graphify-detect/src/extensions.rscrates/graphify-detect/tests/parity_classify.rscrates/graphify-export/src/canvas.rscrates/graphify-export/src/obsidian.rscrates/graphify-export/tests/vault_safety.rscrates/graphify-extract/src/extractors/apex.rscrates/graphify-extract/src/extractors/bash.rscrates/graphify-extract/src/extractors/blade.rscrates/graphify-extract/src/extractors/dart.rscrates/graphify-extract/src/extractors/dm/dmf.rscrates/graphify-extract/src/extractors/dm/dmi.rscrates/graphify-extract/src/extractors/dm/dmm.rscrates/graphify-extract/src/extractors/dm/source.rscrates/graphify-extract/src/extractors/dotnet/csproj.rscrates/graphify-extract/src/extractors/dotnet/mod.rscrates/graphify-extract/src/extractors/dotnet/razor.rscrates/graphify-extract/src/extractors/dotnet/sln.rscrates/graphify-extract/src/extractors/dotnet/slnx.rscrates/graphify-extract/src/extractors/dotnet/xaml.rscrates/graphify-extract/src/extractors/elixir.rscrates/graphify-extract/src/extractors/fortran.rscrates/graphify-extract/src/extractors/go/mod.rscrates/graphify-extract/src/extractors/go/refs.rscrates/graphify-extract/src/extractors/go/walk.rscrates/graphify-extract/src/extractors/groovy.rscrates/graphify-extract/src/extractors/json_lang.rscrates/graphify-extract/src/extractors/julia/mod.rscrates/graphify-extract/src/extractors/julia/walk.rscrates/graphify-extract/src/extractors/manifest_ingest.rscrates/graphify-extract/src/extractors/markdown.rscrates/graphify-extract/src/extractors/mcp.rscrates/graphify-extract/src/extractors/mod.rscrates/graphify-extract/src/extractors/multi/cache.rscrates/graphify-extract/src/extractors/multi/csharp.rscrates/graphify-extract/src/extractors/multi/mod.rscrates/graphify-extract/src/extractors/objc.rscrates/graphify-extract/src/extractors/pascal/forms.rscrates/graphify-extract/src/extractors/pascal/mod.rscrates/graphify-extract/src/extractors/pascal/package.rscrates/graphify-extract/src/extractors/powershell/manifest.rscrates/graphify-extract/src/extractors/powershell/mod.rscrates/graphify-extract/src/extractors/python_rationale.rscrates/graphify-extract/src/extractors/rust_lang/mod.rscrates/graphify-extract/src/extractors/rust_lang/walk.rscrates/graphify-extract/src/extractors/sql/mod.rscrates/graphify-extract/src/extractors/sql/walk.rscrates/graphify-extract/src/extractors/svelte.rscrates/graphify-extract/src/extractors/terraform.rscrates/graphify-extract/src/extractors/verilog.rscrates/graphify-extract/src/extractors/zig.rscrates/graphify-extract/src/generic/graph.rscrates/graphify-extract/src/generic/inherit/java.rscrates/graphify-extract/src/generic/inherit/kotlin.rscrates/graphify-extract/src/generic/inherit/mod.rscrates/graphify-extract/src/generic/inherit/scala.rscrates/graphify-extract/src/generic/inherit/swift.rscrates/graphify-extract/src/generic/mod.rscrates/graphify-extract/src/generic/references/java.rscrates/graphify-extract/src/generic/walk.rscrates/graphify-extract/src/ids.rscrates/graphify-extract/src/ids_tests.rscrates/graphify-extract/src/lang_configs.rscrates/graphify-extract/src/lib.rscrates/graphify-extract/src/postprocess.rscrates/graphify-extract/src/postprocess_tests.rscrates/graphify-extract/src/symbol_resolution_tests.rscrates/graphify-extract/src/types.rscrates/graphify-extract/tests/cross_file_multi.rscrates/graphify-extract/tests/csharp_type_resolution.rscrates/graphify-extract/tests/dotnet_xaml.rscrates/graphify-extract/tests/file_node_id_spec.rscrates/graphify-extract/tests/fixtures/bindings.xamlcrates/graphify-extract/tests/fixtures/sample.metalcrates/graphify-extract/tests/fixtures/sample.xamlcrates/graphify-extract/tests/fixtures/sample.xaml.cscrates/graphify-extract/tests/fixtures/xaml_viewmodel/App.csprojcrates/graphify-extract/tests/fixtures/xaml_viewmodel/ViewModels/DesignViewModel.cscrates/graphify-extract/tests/fixtures/xaml_viewmodel/ViewModels/MainViewModel.cscrates/graphify-extract/tests/fixtures/xaml_viewmodel/ViewModels/PrismOrderViewModel.cscrates/graphify-extract/tests/fixtures/xaml_viewmodel/ViewModels/SettingsViewModel.cscrates/graphify-extract/tests/fixtures/xaml_viewmodel/ViewModels/ToolkitViewModel.cscrates/graphify-extract/tests/fixtures/xaml_viewmodel/Views/DesignView.xamlcrates/graphify-extract/tests/fixtures/xaml_viewmodel/Views/ExplicitMainWindow.xamlcrates/graphify-extract/tests/fixtures/xaml_viewmodel/Views/PrismOrderView.xamlcrates/graphify-extract/tests/fixtures/xaml_viewmodel/Views/SettingsView.xamlcrates/graphify-extract/tests/fixtures/xaml_viewmodel/Views/ToolkitView.xamlcrates/graphify-extract/tests/parity.rscrates/graphify-extract/tests/parity_languages.rscrates/graphify-extract/tests/parity_postprocess.rscrates/graphify-extract/tests/vue_extraction.rscrates/graphify-hooks/src/platform/common/hooks_json.rscrates/graphify-hooks/tests/read_hook.rscrates/graphify-llm/src/constants.rscrates/graphify-llm/src/deepseek.rscrates/graphify-llm/src/gemini.rscrates/graphify-llm/src/kimi.rscrates/graphify-llm/src/openai_compat.rscrates/graphify-llm/tests/custom_endpoint.rscrates/graphify-llm/tests/openai_compat_http.rscrates/graphify-reflect/src/lib.rscrates/graphify-reflect/tests/parity.rscrates/graphify-serve/src/graph.rscrates/graphify-serve/tests/parity.rscrates/graphify-wiki/src/generate.rscrates/graphify-wiki/src/render.rscrates/graphify-wiki/src/util.rscrates/graphify-wiki/tests/parity.rsgraphify-pysrc/cli/args.rssrc/cli/cluster_only.rssrc/cli/dispatch.rssrc/cli/extract.rssrc/cli/mod.rssrc/cli/reflect.rssrc/cli/timer.rstests/cli.rstests/cli_commands.rs
Comment on lines
+39
to
+50
| fn collect_csharp_scope( | ||
| node: tree_sitter::Node<'_>, | ||
| source: &[u8], | ||
| own_ns: &mut Vec<String>, | ||
| usings: &mut Vec<String>, | ||
| aliases: &mut HashMap<String, String>, | ||
| ) { | ||
| match node.kind() { | ||
| "namespace_declaration" | "file_scoped_namespace_declaration" => { | ||
| if let Some(nm) = node.child_by_field_name("name") { | ||
| own_ns.push(nm.utf8_text(source).unwrap_or("").trim().to_string()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Nested namespace syntax never resolves correctly.
collect_csharp_scope records namespace A { namespace B { ... } } as two separate entries ("A", "B"), but fqn_to_id only registers definitions when a file has exactly one namespace string. That means any type declared under nested namespace blocks is omitted from the definition map, and later lookups try A.T / B.T instead of A.B.T. Cross-file repointing will therefore fail for valid C# nested-namespace files.
Also applies to: 118-124, 232-259
🤖 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/graphify-extract/src/extractors/multi/csharp.rs` around lines 39 - 50,
Update collect_csharp_scope and the C# extraction flow so nested
namespace_declaration/file_scoped_namespace_declaration blocks are combined into
a single fully qualified namespace instead of separate entries. Then adjust the
fqn_to_id registration logic and any lookup/repointing paths that consume own_ns
so types inside nested namespaces are emitted and resolved as A.B.T rather than
A.T or B.T. Use the existing collect_csharp_scope, fqn_to_id, and related C#
extractor paths to ensure the definition map includes nested-namespace types
consistently.
Member
Author
There was a problem hiding this comment.
Is this resolved/addressed?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Outline =="
ast-grep outline crates/graphify-extract/src/extractors/multi/csharp.rs --view expanded || true
echo
echo "== csharp.rs (top) =="
sed -n '1,260p' crates/graphify-extract/src/extractors/multi/csharp.rs
echo
echo "== csharp.rs (rest) =="
sed -n '261,420p' crates/graphify-extract/src/extractors/multi/csharp.rs
echo
echo "== Search for nested namespace coverage =="
rg -n -C3 'nested namespace|namespace A|namespace B|A\.B|file_scoped_namespace_declaration|namespace_declaration' crates/graphify-extract/tests crates/graphify-extract/src/extractors/multi/csharp.rsLength of output: 16346
@rblaine95 Not yet — I checked the current PR branch, and this is still not addressed.
Why:
collect_csharp_scopestill records nested blocks separately instead of composingA.B.fqn_to_idregistration still skips files whenown_ns.len() > 1.- The code now explicitly documents this as deferred for graphify-py parity in
crates/graphify-extract/src/extractors/multi/csharp.rsaround Line 121.
So the comment should remain open unless you want to intentionally accept that parity tradeoff and treat it as out of scope for this PR.
🧠 Learnings used
Learnt from: rblaine95
Repo: bunkerlab-net/graphify PR: 8
File: crates/graphify-build/src/dedup_label.rs:15-16
Timestamp: 2026-05-24T18:19:10.174Z
Learning: In the bunkerlab-net/graphify Rust workspace, do not use `.unwrap()` anywhere. If you must fail fast (e.g., during static `LazyLock<...>` initialization such as compiling a `Regex`), use `.expect(<descriptive_reason>)` with a clear, human-readable message explaining what assumption is being made and why it should never be false in this code path. When reviewing, do not suggest changing existing `.expect(...)` calls to `.unwrap()` in this repository.
Learnt from: rblaine95
Repo: bunkerlab-net/graphify PR: 8
File: crates/graphify-dedup/src/score.rs:29-31
Timestamp: 2026-05-24T18:19:46.125Z
Learning: In the bunkerlab-net/graphify Rust workspace, do not use `.unwrap()` in non-test code. Use `.expect("...")` only when the failure indicates a logically guaranteed invariant, and the message must clearly explain why it holds. For `LazyLock<regex::Regex>` values created from literal patterns, use `.expect("static <name> regex")` (replace `<name>` with the specific regex label) and add `#[allow(clippy::expect_used)]`; do not use `.unwrap()` even if you add `#[allow(clippy::unwrap_used)]`.
Comment on lines
+96
to
127
| let mut god_articles: Vec<(String, String)> = Vec::new(); // (node_id, slug) | ||
| for node_data in god_nodes_data { | ||
| if graph.contains_node(&node_data.id) { | ||
| let slug = make_unique_slug(&safe_filename(&node_data.label), &mut used_slugs); | ||
| resolver | ||
| .entry(node_data.label.clone()) | ||
| .or_insert(slug.clone()); | ||
| god_articles.push((node_data.id.clone(), slug)); | ||
| } | ||
| } | ||
|
|
||
| // Second pass: render and write each article with the full resolver in hand. | ||
| let mut count = 0usize; | ||
| let wiki_ctx = WikiCtx { | ||
| graph, | ||
| labels, | ||
| node_community: &node_community, | ||
| deg_map: °_map, | ||
| resolver: &resolver, | ||
| output_dir, | ||
| }; | ||
| count += write_community_articles(&wiki_ctx, &filtered, cohesion, &mut used_slugs)?; | ||
| count += write_god_node_articles(&wiki_ctx, god_nodes_data, &mut used_slugs)?; | ||
| count += write_community_articles(&wiki_ctx, &filtered, cohesion, &community_slugs)?; | ||
| count += write_god_node_articles(&wiki_ctx, &god_articles)?; | ||
|
|
||
| let index = index_md( | ||
| &filtered, | ||
| labels, | ||
| god_nodes_data, | ||
| graph.node_count(), | ||
| graph.edge_count(), | ||
| &resolver, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep index.md aligned with the filtered god-node set.
This loop drops missing god_nodes_data entries from god_articles, but index_md(...) still receives the original slice. The index can therefore advertise god nodes that never get an article written.
🤖 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/graphify-wiki/src/generate.rs` around lines 96 - 127, Keep the index
generation aligned with the actual written god-node articles. In `generate.rs`,
the `god_articles` collection is filtered to only nodes present in `graph`, but
`index_md(...)` still uses the original `god_nodes_data` slice, so update the
`index_md` call (or its input) to use the filtered god-node set derived from the
same loop that builds `god_articles`. Use the existing
`god_articles`/`write_god_node_articles` flow and the `index_md` function to
ensure `index.md` only advertises nodes that actually got articles written.
Member
Author
There was a problem hiding this comment.
Is this resolved/addressed?
There was a problem hiding this comment.
@rblaine95 Not yet — this still looks valid.
write_god_node_articles(...) uses the filtered god_articles, but index_md(...) is still called with the original god_nodes_data, so index.md can still list god-node articles that were skipped.
Relevant spot:
crates/graphify-wiki/src/generate.rs: theindex_md(...)call still passesgod_nodes_datarather than a filtered set derived fromgod_articles.
🧠 Learnings used
Learnt from: rblaine95
Repo: bunkerlab-net/graphify PR: 8
File: crates/graphify-build/src/dedup_label.rs:15-16
Timestamp: 2026-05-24T18:19:10.174Z
Learning: In the bunkerlab-net/graphify Rust workspace, do not use `.unwrap()` anywhere. If you must fail fast (e.g., during static `LazyLock<...>` initialization such as compiling a `Regex`), use `.expect(<descriptive_reason>)` with a clear, human-readable message explaining what assumption is being made and why it should never be false in this code path. When reviewing, do not suggest changing existing `.expect(...)` calls to `.unwrap()` in this repository.
Learnt from: rblaine95
Repo: bunkerlab-net/graphify PR: 8
File: crates/graphify-dedup/src/score.rs:29-31
Timestamp: 2026-05-24T18:19:46.125Z
Learning: In the bunkerlab-net/graphify Rust workspace, do not use `.unwrap()` in non-test code. Use `.expect("...")` only when the failure indicates a logically guaranteed invariant, and the message must clearly explain why it holds. For `LazyLock<regex::Regex>` values created from literal patterns, use `.expect("static <name> regex")` (replace `<name>` with the specific regex label) and add `#[allow(clippy::expect_used)]`; do not use `.unwrap()` even if you add `#[allow(clippy::unwrap_used)]`.
Resolve the findings from the second CodeRabbit pass. Genuine bugs and determinism gaps are fixed (several are bugs `graphify-py` shares, fixed per AGENTS.md); findings that faithfully mirror `graphify-py` are kept, with the parity reasoning recorded in code. Fixes: - `with_xaml_extract_root` restores the thread-local extract root through an RAII guard, so a panic can't leak it to a reused rayon worker (parity with graphify-py's `try`/`finally`). - `.xaml` joins the AST disk-cache bypass list: XAML resolution scans sibling `.cs` code-behind, so a content-keyed cache served stale `ViewModel` members across runs. - `origin_file` is relativized alongside `source_file` (extraction and build) so the #1462 stub field no longer leaks absolute machine paths into graph JSON. - objc and PowerShell `ensure_named_node` emit sourceless stubs (empty `source_file`, `origin_file` set) so cross-file references rewire onto real definitions instead of becoming phantom duplicates (#1402/#1462). - Obsidian `owned_write` refuses to follow a symlink at the target or a parent component, so a symlinked note can't escape the vault (#1506). - `find_node` matches a source path by its normalized full path rather than tokenized form, so distinct paths no longer collide. - `cluster-only --missing-only` preserves a malformed labels file instead of relabeling over it, matching the non-`--missing-only` path. - `#[must_use]` on the pure ID/stem helpers; corrected `extract` and `label` doc-comments. Kept as parity (documented in code): the objc compound-selector node-ID form, C# nested-namespace registration deferral, and the wiki/`index_md` god-node list all mirror `graphify-py` exactly. Test strengthening: `lessons_fresh` now covers both graph sidecars (matching the Python parametrization), the trailing-slash seed test exercises the re-keyed file node, and the affected test suites return `Result` with `?` per the repo convention. By the will of the Machine God
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/graphify-serve/src/graph.rs (1)
762-807: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize backslashes before deriving the basename.
query_pathaccepts Windows-style separators, butquery_basenamedoes not. A query likesrc\foo.rscan matchsource_filewhile failing the L1 file-node preference because the basename remainssrc\foo.rs.Proposed fix
+ let normalized_label_path = label.replace('\\', "/"); let query_basename = strip_diacritics( - std::path::Path::new(label) + std::path::Path::new(&normalized_label_path) .file_name() .and_then(|n| n.to_str()) - .unwrap_or(label), + .unwrap_or(label), ) .to_lowercase();🤖 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/graphify-serve/src/graph.rs` around lines 762 - 807, Normalize the query string for basename extraction in the search logic so Windows-style backslashes are treated as path separators before computing `query_basename`. In the matching block that builds `query_basename` and `query_path`, make `query_basename` use the same slash-normalized input as `query_path` so `src\foo.rs` can still satisfy the L1 file-node preference in the `source_exact` branch. Keep the existing `strip_diacritics`, `Path::file_name`, and `get_norm_label` matching flow intact, only ensure the basename is derived from normalized separators.
🧹 Nitpick comments (1)
crates/graphify-serve/src/graph.rs (1)
757-833: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[must_use]tofind_node.This changed pure function returns an owned
Vec<String>, so it should be annotated.Proposed fix
+#[must_use] pub fn find_node(graph: &Graph, label: &str) -> Vec<String> {As per coding guidelines, “Use
#[must_use]attribute on pure functions returning owned values”.🤖 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/graphify-serve/src/graph.rs` around lines 757 - 833, Add #[must_use] to find_node so callers don’t accidentally ignore its Vec<String> result; this is a pure lookup function, so annotate the find_node definition in graph.rs and keep the existing behavior unchanged. Use the find_node symbol as the target for the attribute addition.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/graphify-serve/src/graph.rs`:
- Around line 762-807: Normalize the query string for basename extraction in the
search logic so Windows-style backslashes are treated as path separators before
computing `query_basename`. In the matching block that builds `query_basename`
and `query_path`, make `query_basename` use the same slash-normalized input as
`query_path` so `src\foo.rs` can still satisfy the L1 file-node preference in
the `source_exact` branch. Keep the existing `strip_diacritics`,
`Path::file_name`, and `get_norm_label` matching flow intact, only ensure the
basename is derived from normalized separators.
---
Nitpick comments:
In `@crates/graphify-serve/src/graph.rs`:
- Around line 757-833: Add #[must_use] to find_node so callers don’t
accidentally ignore its Vec<String> result; this is a pure lookup function, so
annotate the find_node definition in graph.rs and keep the existing behavior
unchanged. Use the find_node symbol as the target for the attribute addition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 90d343bb-c734-4f74-896f-a8c54e9ac79e
📒 Files selected for processing (22)
crates/graphify-affected/tests/parity.rscrates/graphify-build/src/ingest.rscrates/graphify-build/src/migrate.rscrates/graphify-export/src/obsidian.rscrates/graphify-export/tests/vault_safety.rscrates/graphify-extract/src/extractors/dotnet/xaml.rscrates/graphify-extract/src/extractors/multi/cache.rscrates/graphify-extract/src/extractors/multi/csharp.rscrates/graphify-extract/src/extractors/multi/mod.rscrates/graphify-extract/src/extractors/objc.rscrates/graphify-extract/src/extractors/powershell/mod.rscrates/graphify-extract/src/extractors/svelte.rscrates/graphify-extract/src/postprocess.rscrates/graphify-extract/tests/csharp_type_resolution.rscrates/graphify-extract/tests/parity_languages.rscrates/graphify-reflect/tests/parity.rscrates/graphify-serve/src/graph.rssrc/cli/args.rssrc/cli/cluster_only.rssrc/cli/extract.rstests/cli.rstests/cli_commands.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/cli.rs
- crates/graphify-extract/src/postprocess.rs
- crates/graphify-affected/tests/parity.rs
- src/cli/args.rs
- crates/graphify-reflect/tests/parity.rs
- crates/graphify-build/src/ingest.rs
- crates/graphify-extract/src/extractors/multi/csharp.rs
- crates/graphify-extract/tests/csharp_type_resolution.rs
- crates/graphify-extract/src/extractors/multi/mod.rs
- crates/graphify-export/src/obsidian.rs
- src/cli/extract.rs
- crates/graphify-export/tests/vault_safety.rs
- crates/graphify-extract/src/extractors/svelte.rs
- crates/graphify-extract/src/extractors/dotnet/xaml.rs
- crates/graphify-extract/tests/parity_languages.rs
- src/cli/cluster_only.rs
- crates/graphify-build/src/migrate.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports the applicable changes from
graphify-pyv0.8.49 → v0.9.0 (38 upstream commits) and bumps the workspace to0.9.0. Submodule pointer advanced to92e682f.Breaking change
_), e.g.src/auth/session.py→src_auth_session,validate()inside →src_auth_session_validate(#1504/#1509). Same-named files in different directories no longer collide. Abuild_from_jsonre-key migrates pre-#1504 graphs; read-only consumers (query/serve) print a one-line rebuild nudge instead of silently re-keying.Ported
origin_fileso same-label stubs from different files stay distinct (#1462/#1500).enum/struct/recordnode types (#1466).@import(#1475).x:Class, named controls,{Binding}paths, ViewModel / code-behind resolution (#1460/#1473)..metalrouted through the C++ extractor (#1468/#1480).ceil(sqrt(n))grid, case-fold filename dedup (#1506/#1452/#1453); wiki portable relative-Markdown links (#1444).stream: false;kimi/gemini/deepseekhonour a bare*_BASE_URL(#1223/#1458).--timingonextract/cluster-only/label;--missing-onlyoncluster-only/label(#1490/#1481).--if-staleweighs graph sidecars; the Read|Glob hook matches the real trailing extension;affected/explainresolve a source-file path to its file-level node (#1470/#1463/#1503).Docs (
README.md,USAGE.md) reconciled: new flags, formats, env vars, node-ID scheme, and export behaviours. Parity tests ported for every new upstream test case.Verification
cargo clippy --all-targets --all-features --workspace— cleancargo nextest run --workspace— 2283 passed, 2 skippedhk check— pass--type committed—findings: 0(24 first-pass findings resolved: genuine divergences fixed, graphify-py-matching ones kept with in-code// Parity dispute (CodeRabbit): …comments)Summary by CodeRabbit
--timingfor extract and label/cluster-only flows, plus--missing-onlyto rename only placeholder communities.