Skip to content

feat: add execution features with criticality scoring#142

Merged
ArtemisMucaj merged 7 commits into
mainfrom
claude/execution-features-criticality-bdL1S
Apr 12, 2026
Merged

feat: add execution features with criticality scoring#142
ArtemisMucaj merged 7 commits into
mainfrom
claude/execution-features-criticality-bdL1S

Conversation

@ArtemisMucaj
Copy link
Copy Markdown
Owner

@ArtemisMucaj ArtemisMucaj commented Apr 11, 2026

Introduces forward call-chain analysis rooted at entry-point symbols,
complementing the existing upward ImpactAnalysisUseCase.

New domain models (src/domain/models/execution_feature.rs):

  • FeatureNode: single node in a forward BFS call chain (symbol, file,
    line, depth, repository_id)
  • ExecutionFeature: full feature record with id, name, entry_point,
    path, depth, file_count, and a 0.0–1.0 criticality score

New use case (src/application/use_cases/execution_features.rs):

  • Entry-point detection: symbols with zero callers, or whose short name
    matches well-known patterns (main, run, start, init, handle, execute,
    process, test_, it_, handle_, on_)
  • Forward BFS with cycle detection via visited HashSet (mirrors the
    pattern used by ImpactAnalysisUseCase and SymbolContextUseCase)
  • Criticality scoring — five weighted signals summed and clamped to 1.0:
    file spread (0.30), security sensitivity (0.25), external calls
    (0.20), test coverage gap (0.15), BFS depth (0.10)
  • Public API: list_features, get_feature, get_affected_features

CLI additions (src/cli/mod.rs):

  • FeaturesSubcommand enum with list, get, and affected subcommands
  • Features { subcommand } variant added to Commands

Wiring:

  • Container::execution_features_use_case() factory method
  • ExecutionFeaturesController with text/json/vimgrep formatters
  • Router matches Commands::Features and delegates to controller
  • Features added to read_only set in main.rs (no writes needed)

https://claude.ai/code/session_01CZpnvUWc3brSbRpDJfByrB

Summary by CodeRabbit

  • New Features

    • Added execution‑features analysis: list top entry‑point features for a repository, fetch feature details by symbol (includes call‑chain path with file/line, depth, file count, stable id), and find features impacted by changed symbols. Features are ranked by a normalized criticality score in [0.0,1.0].
  • CLI / UX

    • New "features" subcommands (list/get/impacted) with JSON, vimgrep, and human‑readable text outputs; treated as read‑only.
  • Tests

    • Added comprehensive async test suite covering entry‑point detection, traversal, deduplication, ordering, scoring, and limits.

claude added 3 commits April 11, 2026 21:36
Introduces forward call-chain analysis rooted at entry-point symbols,
complementing the existing upward ImpactAnalysisUseCase.

New domain models (src/domain/models/execution_feature.rs):
- FeatureNode: single node in a forward BFS call chain (symbol, file,
  line, depth, repository_id)
- ExecutionFeature: full feature record with id, name, entry_point,
  path, depth, file_count, and a 0.0–1.0 criticality score

New use case (src/application/use_cases/execution_features.rs):
- Entry-point detection: symbols with zero callers, or whose short name
  matches well-known patterns (main, run, start, init, handle, execute,
  process, test_*, it_*, handle_*, on_*)
- Forward BFS with cycle detection via visited HashSet (mirrors the
  pattern used by ImpactAnalysisUseCase and SymbolContextUseCase)
- Criticality scoring — five weighted signals summed and clamped to 1.0:
    file spread (0.30), security sensitivity (0.25), external calls
    (0.20), test coverage gap (0.15), BFS depth (0.10)
- Public API: list_features, get_feature, get_affected_features

CLI additions (src/cli/mod.rs):
- FeaturesSubcommand enum with list, get, and affected subcommands
- Features { subcommand } variant added to Commands

Wiring:
- Container::execution_features_use_case() factory method
- ExecutionFeaturesController with text/json/vimgrep formatters
- Router matches Commands::Features and delegates to controller
- Features added to read_only set in main.rs (no writes needed)

https://claude.ai/code/session_01CZpnvUWc3brSbRpDJfByrB
Covers all major behaviours of the execution features use case:

Entry-point detection:
- zero-caller symbol is detected as entry point
- well-known name pattern (handle_*) is detected even when symbol has callers
- pure-callee symbol is not treated as entry point

Forward BFS / path construction:
- path contains all reachable symbols with correct depths
- file_count is reported correctly

Cycle/graph safety:
- mutual recursion (A ↔ B) terminates and deduplicates
- diamond dependency (A→B, A→C, B→D, C→D) deduplicates D

Criticality scoring:
- security-sensitive symbol raises criticality vs plain path
- criticality is always in the [0.0, 1.0] range

list_features:
- returns empty slice for unknown/empty repository
- results are sorted by descending criticality
- limit parameter caps the result count

get_feature:
- returns None for unknown symbol
- returns correct entry_point and repository_id when found
- feature id is stable across repeated calls

get_affected_features:
- includes features whose path contains a changed symbol
- excludes features whose path does not contain any changed symbol
- returns empty slice for empty changed_symbols input
- results sorted by descending criticality

https://claude.ai/code/session_01CZpnvUWc3brSbRpDJfByrB
Aligns the naming with the existing impact analysis vocabulary used
throughout the codebase (ImpactAnalysisUseCase, impact_use_case,
total_affected, etc.).

Also renames the corresponding CLI subcommand (Affected → Impacted),
the controller method (affected → impacted), and updates all call sites
and test references.

https://claude.ai/code/session_01CZpnvUWc3brSbRpDJfByrB
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 11, 2026

Warning

Rate limit exceeded

@ArtemisMucaj has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 25 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 25 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4d85a13-4adb-493b-b6ce-81769f89b0c5

📥 Commits

Reviewing files that changed from the base of the PR and between babc857 and 0f59a73.

📒 Files selected for processing (1)
  • src/application/use_cases/execution_features.rs
📝 Walkthrough

Walkthrough

Adds ExecutionFeaturesUseCase and domain models to detect repository entry points, perform forward BFS call-chain traversal, compute a normalized criticality score, and expose list/get/impacted APIs; wires CLI, controller, router, container, lib exports, and end-to-end integration tests.

Changes

Cohort / File(s) Summary
Core use-case & domain
src/application/use_cases/execution_features.rs, src/domain/models/execution_feature.rs
New ExecutionFeaturesUseCase, plus FeatureNode and ExecutionFeature types: entry-point detection, forward BFS path construction, resolved symbol handling, and weighted criticality calculation.
Use-case & model exports
src/application/use_cases/mod.rs, src/domain/models/mod.rs
Added module declarations and public re-exports for execution features.
CLI
src/cli/mod.rs
New FeaturesSubcommand (List/Get/Impacted) and added Commands::Features routing.
API controller
src/connector/api/controller/execution_features_controller.rs, src/connector/api/controller/mod.rs
New ExecutionFeaturesController with async list, get, and impacted endpoints and multi-format output helpers; controller re-export added.
Routing & DI
src/connector/api/router.rs, src/connector/api/container.rs
Router gains execution_features_controller field and dispatch arms; container adds execution_features_use_case() factory.
Library & runtime wiring
src/lib.rs, src/main.rs
Public re-exports for use case, CLI subcommand, and domain types; features included in read-only command set.
Tests
tests/execution_features_tests.rs
Comprehensive async integration tests covering entry-point heuristics, BFS traversal (cycles/diamond dedup), criticality bounds, list/get/impacted semantics, ordering, and limit behavior using an in-memory DuckDB call graph.

Sequence Diagram(s)

sequenceDiagram
    actor CLI as CLI
    participant Router as Router
    participant Controller as ExecutionFeaturesController
    participant UseCase as ExecutionFeaturesUseCase
    participant CallGraph as CallGraphUseCase

    CLI->>Router: route(Commands::Features)
    Router->>Controller: dispatch List/Get/Impacted
    Controller->>UseCase: list_features / get_feature / get_impacted_features
    UseCase->>CallGraph: query callers/callees & resolve symbols
    CallGraph-->>UseCase: call-graph edges + symbol metadata
    UseCase->>UseCase: detect entry points -> BFS traversal -> compute criticality
    UseCase-->>Controller: ExecutionFeature(s)
    Controller->>Controller: format output (JSON / vimgrep / text)
    Controller-->>Router: Result<String>
    Router-->>CLI: formatted response
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 I hop through symbols, one paw, then two,
BFS leaves a trail of paths in morning dew.
I count the files, I weigh the depth and light,
Entry points gleam — I guard them through the night. 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat: add execution features with criticality scoring' directly and clearly describes the main change: introducing execution features functionality with criticality scoring.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/execution-features-criticality-bdL1S

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/application/use_cases/execution_features.rs`:
- Around line 233-246: The root node is seeded with an empty file_path and line
0 (queue.push_back((entry_point.to_string(), 0, String::new(), 0))) which
creates a phantom file and skews file_count and criticality; update the seeding
to populate the file_path and line with the real source location for the entry
symbol (use whatever source-location lookup is available for entry_point or
symbol table accessor) and ensure FeatureNode entries created in the BFS use
those real values (affecting the initial push_back and the FeatureNode
construction in the while loop).
- Around line 104-118: get_feature currently takes the first resolved symbol and
always builds a feature for it using repo = repository_id.unwrap_or(""), which
causes two bugs: it treats non-entry-point symbols as entry points and defaults
to an empty repo string instead of the symbol's repo. Fix by after resolving
(resolved = self.call_graph.resolve_symbols(...)) selecting the candidate (fqn =
&resolved[0]) then verify it's an entry point (e.g., via a call_graph method or
symbol metadata - call_graph.is_entry_point(fqn) or fetching symbol metadata)
and if not return Ok(None); for the repository, when repository_id is None use
the resolved symbol's repository (extract repo from the resolved symbol metadata
or call_graph.get_symbol_repo(fqn)) instead of "" and pass that repo into
self.build_feature(fqn, repo). Ensure you reference get_feature,
call_graph.resolve_symbols, call_graph.is_entry_point/get_symbol_repo (or
equivalent), and build_feature when making the change.
- Around line 308-313: The call-graph error is being swallowed by
unwrap_or_default on the Result from self.call_graph.find_callers (called with
callers_query created via CallGraphQuery::new().with_repository), causing
erroneous empty-call semantics; replace the unwrap_or_default with proper error
propagation or handling—either propagate the Result up from the surrounding
function (return Err) when find_callers fails, or explicitly log the error with
tracing::warn! or tracing::error! and then handle the failure case—so update the
call to self.call_graph.find_callers(entry_point, &callers_query).await to
return/forward the error or log it before using an empty list (and adjust the
surrounding function signature to Result if propagating).
- Around line 272-280: The leaf detection currently uses a global depth check
which undercounts leaves; instead of testing path.iter().any(|n| n.depth ==
node.depth + 1) and comparing to the global max, change the logic in the
unresolved_callees calculation to look for child nodes of the current node
(e.g., any entry in path with parent/preceding reference pointing to node.id or
otherwise reachable from this node) — i.e., detect whether there exists any node
with depth == node.depth + 1 that is actually a child of this node (or whose
ancestor chain includes this node) and only count node as unresolved if no such
child exists; update the code around unresolved_callees, path, node.depth and
the node identifier/parent field (e.g., node.id / node.parent_id or predecessor)
accordingly.

In `@src/connector/api/controller/execution_features_controller.rs`:
- Around line 68-79: The current empty-result branch always returns a
human-readable string, breaking the format contract for commands like `impacted
--format json`; update the empty check to return format-appropriate empty
outputs based on the `format: OutputFormat` value: for `OutputFormat::Json`
return an empty JSON array
(`serde_json::to_string_pretty(&Vec::<Feature>::new())` or equivalent), for
`OutputFormat::Vimgrep` return an empty string, and for `OutputFormat::Text`
keep the existing `"No features impacted..."` message; adjust the code around
`features.is_empty()` and the final `match format { ... }` (referencing
`OutputFormat`, `format_list_vimgrep`, `format_list_text`, and
`serde_json::to_string_pretty`) so empty results follow the same formatting
rules as non-empty ones.
- Around line 46-53: The current None arm always returns a human-readable
sentence and ignores OutputFormat; change the None branch in the match over
result so it mirrors the Some branch's format handling: match on OutputFormat
and return machine-friendly empty outputs (e.g. OutputFormat::Json -> "null" via
serde_json, OutputFormat::Vimgrep -> empty string, OutputFormat::Text -> the
existing message or empty string). Update the None arm in the function in
execution_features_controller.rs where result is matched, referencing
OutputFormat::Json, OutputFormat::Vimgrep, OutputFormat::Text and reuse
Self::format_feature_vimgrep / Self::format_feature_text or serde_json for the
Json case.
🪄 Autofix (Beta)

❌ Autofix failed (check again to retry)

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 39f46e88-248e-4db2-8370-cbcc68897021

📥 Commits

Reviewing files that changed from the base of the PR and between 6f7642c and 8785952.

📒 Files selected for processing (12)
  • src/application/use_cases/execution_features.rs
  • src/application/use_cases/mod.rs
  • src/cli/mod.rs
  • src/connector/api/container.rs
  • src/connector/api/controller/execution_features_controller.rs
  • src/connector/api/controller/mod.rs
  • src/connector/api/router.rs
  • src/domain/models/execution_feature.rs
  • src/domain/models/mod.rs
  • src/lib.rs
  • src/main.rs
  • tests/execution_features_tests.rs

Comment thread src/application/use_cases/execution_features.rs
Comment thread src/application/use_cases/execution_features.rs
Comment thread src/application/use_cases/execution_features.rs Outdated
Comment thread src/application/use_cases/execution_features.rs Outdated
Comment thread src/connector/api/controller/execution_features_controller.rs
Comment thread src/connector/api/controller/execution_features_controller.rs Outdated
…er output

- BFS root: pre-fetch entry-point callees to resolve its own file_path
  (caller_file_path) instead of seeding the root node with an empty string,
  preventing the empty path from inflating file_count and file_spread_score.

- Leaf detection: replace the broken post-BFS loop (which used global max-depth
  and checked for sibling nodes rather than per-node children) with a
  symbols_with_callees HashSet tracked during BFS. A node is unresolved when
  find_callees returns nothing for it in the repository, not merely when BFS
  added no new nodes from it (handles diamond dedup correctly).

- Error propagation: propagate find_callers errors in the test-coverage gap
  signal with ? instead of silently swallowing them via unwrap_or_default.

- get_feature – repo discovery: when repository_id is None, discover the
  effective repository from the resolved symbol's call-graph edges instead of
  passing an empty string to build_feature (which would yield an empty feature).

- get_feature – entry-point check: verify the resolved symbol has no callers in
  the effective repository (or matches a known name pattern) before building a
  feature; return None for non-entry-point symbols.

- Controller format-awareness: get returns "null" / "" / text message per
  OutputFormat when the feature is not found; impacted returns "[]" / "" / text
  message per OutputFormat when the list is empty (removes the format-blind early
  return).

Adds test_get_feature_non_entry_point_returns_none to cover the entry-point
verification fix. All 20 integration tests pass.

https://claude.ai/code/session_01CZpnvUWc3brSbRpDJfByrB
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
src/connector/api/controller/execution_features_controller.rs (1)

117-129: Consider deduplicating vimgrep rendering.

format_list_vimgrep and format_feature_vimgrep duplicate the same line format (file:line:1:symbol). A shared helper would reduce drift risk.

Also applies to: 165-177

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/connector/api/controller/execution_features_controller.rs` around lines
117 - 129, format_list_vimgrep and format_feature_vimgrep duplicate the same
"file:line:1:symbol" rendering; extract a single helper (e.g.,
format_vimgrep_entry) that accepts the node or (file_path, line, symbol) and
returns the formatted string, then call that helper from both
format_list_vimgrep and format_feature_vimgrep to replace the inline format!
invocation so the formatting logic is centralized and deduplicated.
tests/execution_features_tests.rs (2)

143-175: BFS-order test currently doesn’t verify order.

The test description promises BFS ordering, but Lines 168-171 only assert symbol presence. A DFS/unstable traversal could still pass. Assert the expected ordered sequence for this graph.

Proposed tightening
-    // All symbols in the chain must appear.
-    let syms: Vec<&str> = feature.path.iter().map(|n| n.symbol.as_str()).collect();
-    assert!(syms.contains(&"a"), "path missing 'a'");
-    assert!(syms.contains(&"b"), "path missing 'b'");
-    assert!(syms.contains(&"c"), "path missing 'c'");
+    // Verify BFS order for this linear chain.
+    let syms: Vec<&str> = feature.path.iter().map(|n| n.symbol.as_str()).collect();
+    assert_eq!(syms, vec!["main", "a", "b", "c"]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/execution_features_tests.rs` around lines 143 - 175, The test
test_feature_path_depth_and_order currently only checks presence of symbols but
not their order; update the assertions on feature.path (and/or the syms Vec
derived from it) to assert the exact BFS sequence for this linear graph (expect
["main","a","b","c"] with "main" at index 0 and increasing depths 0..3) instead
of using contains checks so the test fails if traversal is not BFS-ordered;
reference feature.path, feature.path[i].symbol, and feature.depth when making
the stronger assertions.

177-199: file_count assertion is too weak to catch regressions.

Line 198 (>= 1) will pass for many incorrect implementations, including partial path/file attribution regressions. Prefer asserting an exact count from a deterministic fixture (or adjust fixture inputs so expected distinct files are unambiguous).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/execution_features_tests.rs` around lines 177 - 199, The test
test_feature_file_count currently asserts only that feature.file_count >= 1
which is too weak; update the assertion to check the exact expected distinct
file count derived from the fixture (three distinct files from
call_ref("main","a","src/main.rs",...), call_ref("a","b","src/a.rs",...),
call_ref("b","c","src/b.rs",...)), e.g. assert that feature.file_count equals 3
so regressions in file attribution are caught; if the fixture is ambiguous
adjust the call_ref entries so the expected distinct files are deterministic
before asserting equality via
ExecutionFeaturesUseCase::new(...).get_feature(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/connector/api/controller/execution_features_controller.rs`:
- Around line 117-129: format_list_vimgrep and format_feature_vimgrep duplicate
the same "file:line:1:symbol" rendering; extract a single helper (e.g.,
format_vimgrep_entry) that accepts the node or (file_path, line, symbol) and
returns the formatted string, then call that helper from both
format_list_vimgrep and format_feature_vimgrep to replace the inline format!
invocation so the formatting logic is centralized and deduplicated.

In `@tests/execution_features_tests.rs`:
- Around line 143-175: The test test_feature_path_depth_and_order currently only
checks presence of symbols but not their order; update the assertions on
feature.path (and/or the syms Vec derived from it) to assert the exact BFS
sequence for this linear graph (expect ["main","a","b","c"] with "main" at index
0 and increasing depths 0..3) instead of using contains checks so the test fails
if traversal is not BFS-ordered; reference feature.path, feature.path[i].symbol,
and feature.depth when making the stronger assertions.
- Around line 177-199: The test test_feature_file_count currently asserts only
that feature.file_count >= 1 which is too weak; update the assertion to check
the exact expected distinct file count derived from the fixture (three distinct
files from call_ref("main","a","src/main.rs",...),
call_ref("a","b","src/a.rs",...), call_ref("b","c","src/b.rs",...)), e.g. assert
that feature.file_count equals 3 so regressions in file attribution are caught;
if the fixture is ambiguous adjust the call_ref entries so the expected distinct
files are deterministic before asserting equality via
ExecutionFeaturesUseCase::new(...).get_feature(...).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1cc1e143-52fb-429c-81f3-7cedbe7513e8

📥 Commits

Reviewing files that changed from the base of the PR and between 8785952 and bec1102.

📒 Files selected for processing (3)
  • src/application/use_cases/execution_features.rs
  • src/connector/api/controller/execution_features_controller.rs
  • tests/execution_features_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/application/use_cases/execution_features.rs

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 12, 2026

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found.

…structural signals

Entry-point detection:
- Remove ENTRY_POINT_EXACT and ENTRY_POINT_PREFIXES name-pattern matching.
  Entry points are now defined purely structurally: a symbol that calls at
  least one other symbol but is never called within the same repository.
  Name-based matching produced false positives for any function prefixed with
  handle_ or on_ regardless of its position in the call graph.
- Simplify find_entry_points: drop the dead caller_file HashMap (was collected
  but never used) and remove the HashMap import.

Criticality scoring:
- Remove WEIGHT_SECURITY and SECURITY_KEYWORDS. A static keyword list cannot
  generalize across languages and frameworks without false positives; the
  remaining structural signals already capture meaningful risk dimensions.
- Redistribute the 0.25 security weight across the four remaining signals:
    file_spread      0.30 → 0.35
    external_calls   0.20 → 0.25
    test_coverage    0.15 → 0.25  (largest bump: most actionable signal)
    depth            0.10 → 0.15
- Remove is_security_sensitive and short_name_is_entry_point helpers.

Tests:
- Remove test_well_known_name_is_entry_point_even_with_callers (tested the
  deleted name-pattern behavior).
- Remove test_security_sensitive_raises_criticality (tested the deleted
  security keyword signal).

https://claude.ai/code/session_01CZpnvUWc3brSbRpDJfByrB
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/application/use_cases/execution_features.rs (1)

234-289: ⚠️ Potential issue | 🟠 Major

Feature nodes still carry incorrect source locations.

The root node now gets a real file_path, but it still hardcodes line: 0, and queued children inherit the parent edge’s reference_file_path. In a chain like main -> a -> b, node a is recorded from src/main.rs instead of src/a.rs. That skews file_count/file_spread_score and makes the exported locations point at caller sites rather than the current symbol. Resolve each symbol’s own location before pushing its FeatureNode, and stop hardcoding the root line to 0.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/use_cases/execution_features.rs` around lines 234 - 289, The
code pushes FeatureNode entries using caller-site locations (and root uses line:
0); fix by resolving each symbol's actual location before creating its
FeatureNode: for the root, derive both file_path and line from a reference whose
callee_symbol() equals entry_point (instead of hardcoding line: 0), and inside
the BFS loop, look up the current symbol's own location (either via an existing
call_graph location/definition API or by finding a reference where
reference.callee_symbol() == current) and use that
callee_file_path()/callee_line() for FeatureNode; stop using
reference_file_path()/reference_line() for the node itself (keep them only for
queuing children if needed). Ensure you use the existing symbols
initial_callees, queue, visited, FeatureNode, and
self.call_graph.find_callees/find-definition-like API to locate the correct
places to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/application/use_cases/execution_features.rs`:
- Around line 320-336: The test-coverage check in build_feature is only looking
at direct callers (call_graph.find_callers on entry_point), but earlier
filtering ensures entry symbols have no in-repo direct callers so
has_test_caller is always false; change the check to look for test callers
transitively or via reachable symbols: e.g., compute the set of symbols
reachable from entry_point (or the reverse reachable callers transitively) using
the call graph API (a transitive find_callers/find_callers_transitive or BFS
over call_graph), then check that set with is_test_symbol to set test_gap_score;
update the logic around callers_of_entry and test_gap_score to use this
transitive caller/test-symbol scan so the coverage weight can contribute to
ranking.

In `@tests/execution_features_tests.rs`:
- Around line 137-158: The test test_feature_file_count currently only asserts
feature.file_count >= 1 which won't catch regressions; update the assertion to
assert the exact expected number of distinct files (3) for the seeded refs
(call_ref entries for "main" -> "src/main.rs", "a" -> "src/a.rs", "b" ->
"src/b.rs") or alternatively assert the set of file paths equals
{"src/main.rs","src/a.rs","src/b.rs"} after calling
ExecutionFeaturesUseCase::get_feature (use make_call_graph_use_case and
cg.save_references as in the test to seed data) so any change in file path
recording fails loudly.

---

Duplicate comments:
In `@src/application/use_cases/execution_features.rs`:
- Around line 234-289: The code pushes FeatureNode entries using caller-site
locations (and root uses line: 0); fix by resolving each symbol's actual
location before creating its FeatureNode: for the root, derive both file_path
and line from a reference whose callee_symbol() equals entry_point (instead of
hardcoding line: 0), and inside the BFS loop, look up the current symbol's own
location (either via an existing call_graph location/definition API or by
finding a reference where reference.callee_symbol() == current) and use that
callee_file_path()/callee_line() for FeatureNode; stop using
reference_file_path()/reference_line() for the node itself (keep them only for
queuing children if needed). Ensure you use the existing symbols
initial_callees, queue, visited, FeatureNode, and
self.call_graph.find_callees/find-definition-like API to locate the correct
places to change.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9d644c69-83b6-44e1-bad9-150e170774bc

📥 Commits

Reviewing files that changed from the base of the PR and between bec1102 and f5e7c66.

📒 Files selected for processing (2)
  • src/application/use_cases/execution_features.rs
  • tests/execution_features_tests.rs

Comment thread src/application/use_cases/execution_features.rs Outdated
Comment on lines +137 to +158
/// `file_count` must equal the number of distinct files touched by the path.
#[tokio::test(flavor = "multi_thread")]
async fn test_feature_file_count() {
let cg = make_call_graph_use_case().await;
// Three separate files.
let refs = vec![
call_ref("main", "a", "src/main.rs", 1, "repo1"),
call_ref("a", "b", "src/a.rs", 5, "repo1"),
call_ref("b", "c", "src/b.rs", 10, "repo1"),
];
cg.save_references(&refs).await.expect("seed failed");

let uc = ExecutionFeaturesUseCase::new(cg);
let feature = uc
.get_feature("main", Some("repo1"))
.await
.expect("get_feature failed")
.expect("feature should exist");

// Nodes: main (src/main.rs), a (src/a.rs via ref file), b (src/b.rs), c (leaf)
// Exact count depends on how file_path is recorded, but must be ≥ 1.
assert!(feature.file_count >= 1, "file_count must be at least 1");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

This test won’t catch a broken file_count.

assert!(feature.file_count >= 1) still passes when path nodes are tagged with caller files or :0, so it doesn't protect the file-spread signal at all. Please pin the expected count (or exact file set) for this seeded graph so regressions in path location handling fail loudly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/execution_features_tests.rs` around lines 137 - 158, The test
test_feature_file_count currently only asserts feature.file_count >= 1 which
won't catch regressions; update the assertion to assert the exact expected
number of distinct files (3) for the seeded refs (call_ref entries for "main" ->
"src/main.rs", "a" -> "src/a.rs", "b" -> "src/b.rs") or alternatively assert the
set of file paths equals {"src/main.rs","src/a.rs","src/b.rs"} after calling
ExecutionFeaturesUseCase::get_feature (use make_call_graph_use_case and
cg.save_references as in the test to seed data) so any change in file path
recording fails loudly.

claude added 2 commits April 12, 2026 20:29
… from scoring

- Replace partial_cmp().unwrap_or(Equal) with f32::total_cmp() in both
  sort_by calls — more idiomatic and avoids the unwrap.

- Remove the find_callers call from build_feature. Entry points are
  structurally defined as having no callers in the repository, so the DB
  round-trip always returned empty and has_test_caller was always false.
  Replacing it with the constant TEST_COVERAGE_GAP_SCORE applied unconditionally
  eliminates one DB round-trip per entry point per list_features call without
  changing any output.

- Drop TEST_COVERAGE_PRESENT_SCORE constant and is_test_symbol helper —
  both are now dead code.

https://claude.ai/code/session_01CZpnvUWc3brSbRpDJfByrB
Replace the two-statement guard (if total_callees_seen == 0 { ... }) with
.max(1) directly in the external_score calculation, removing the mutable
reassignment.

https://claude.ai/code/session_01CZpnvUWc3brSbRpDJfByrB
@ArtemisMucaj ArtemisMucaj merged commit 117cefb into main Apr 12, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants