Include review prompt context for helpers and file windows

[Iron-Ham]

Summary

• Include selected dependency/helper and dependent/caller source bodies in local inspect review prompts
• Add explicit prompt semantics for before/after entity snippets and line-numbered file context windows
• Build commit/range graphs from the reviewed tree so helper/caller bodies match the target revision
• Include before-side dependency context for removed calls and renamed/moved entities

Closes #15

Test plan

• cargo test --workspace
• Local inspect review command against a temporary git fixture with a mock OpenAI-compatible server, asserting the outbound prompt contains context semantics, helper source, caller source, and file context

[vercel]

@Iron-Ham is attempting to deploy a commit to the rs545837's projects Team on Vercel.

A member of the Team first needs to authorize it.

[inspect-review]

inspect review

Triage: 46 entities analyzed | 0 critical, 0 high, 22 medium, 24 low
Verdict: standard_review

Findings (7)

1. [low] In collect_related_entity_code, the function filters entities by max_lines but uses the wrong variable name. It checks line_count > max_lines but should be checking against max_dependent_lines or max_dependency_lines depending on the caller. The parameter is named max_lines but callers pass options.max_dependent_lines or options.max_dependency_lines, so this works by accident. However, the real bug is that line_count calculation is dep.end_line.saturating_sub(dep.start_line) + 1 which will be 0 when end_line < start_line due to saturation, allowing entities with invalid spans to pass the filter.
2. [low] In context_span, when start_line is 0 and fallback_line is 0, it defaults to 1. But when end_line is 0 or less than start, it calculates end = start + content_lines - 1. If entity_content is None, content_lines defaults to 1, making end = 1 + 1 - 1 = 1. However, if entity_content is Some but empty (0 lines), content_lines would be max(1) = 1, which is correct. The bug is that entity_content.map(|c| c.lines().count().max(1)) will return 1 for empty content, but the actual line count should be 0 for empty strings, leading to incorrect span calculation.
3. [low] In extract_source_context, the function uses 1-based line numbers but accesses the lines vector with 0-based indexing. When building the content string, it does lines[line_no - 1] which is correct. However, the range (window_start..=window_end) is inclusive and uses 1-based numbers, but if window_end equals lines.len(), then accessing lines[window_end - 1] when window_end = lines.len() would access lines[lines.len() - 1] which is valid. But the issue is that window_end is clamped to lines.len(), and the loop goes ..=window_end, so when window_end = lines.len(), the loop tries to access lines[lines.len() - 1] which is the last element (correct), but the format string shows line_no which would be lines.len() for the last iteration, which is correct for display. Actually, this appears correct on closer inspection.
4. [low] In build_graph_for_source for the GitRef case, after building the graph from the temporary snapshot directory, it attempts to remove the directory with let _ = std::fs::remove_dir_all(snapshot). This silently ignores errors. More critically, the graph is built with paths relative to the snapshot directory, but the function returns the graph which will have file paths pointing to the now-deleted temporary directory. Any subsequent operations that try to read files using these paths will fail.
5. [low] In merge_related_entities, the function checks if entities are duplicates by comparing entity_name, entity_type, file_path, and content. However, it doesn't compare start_line and end_line. This means if the same entity appears at different line positions (e.g., due to code movement), they would be incorrectly merged as duplicates even though they represent different versions of the entity at different locations.
6. [low] In resolve_before_entity_id, the function searches for an entity in before_graph by matching name, file_path, and entity_type. However, it uses change.old_entity_name and change.old_file_path with .unwrap_or() fallbacks. If an entity was renamed, old_entity_name would be Some, but if it was only moved (file changed), old_file_path would be Some. The issue is that for a pure rename (same file), old_file_path would be None, so it falls back to change.file_path which is the NEW file path, not the old one. This would fail to find renamed entities that stayed in the same file.
7. [low] In collect_dependent_code, the function calls collect_related_entity_code with graph.get_dependents(entity_id) for the after-side, but then uses before_entity_id.unwrap_or(entity_id) for the before-side. If before_entity_id is None, it falls back to entity_id, which is the after-side entity ID. This will look up dependents using the wrong entity ID in the before_graph, potentially returning incorrect or missing dependents.

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes

[inspect-review]

inspect review

Triage: 47 entities analyzed | 0 critical, 0 high, 24 medium, 23 low
Verdict: standard_review

Findings (7)

1. [low] In analyze_with_options, the loop at line ~324 iterates over dependencies.iter().chain(before_dependencies.iter()) to populate dependency_edges, but this creates duplicate edges when the same dependency exists in both graphs. This causes incorrect dependency edge counts and graph structure.
2. [low] In analyze_with_options, the loop at line ~330 iterates over dependents.iter().chain(before_dependents.iter()) to populate dependency_edges, but this creates duplicate edges when the same dependent exists in both graphs. This causes incorrect dependency edge counts and graph structure.
3. [low] In context_span, when start_line is 0 and fallback_line is 0, the function returns start = 1, but then calculates end = start + content_lines - 1. If content_lines is 1, this gives end = 1, which is correct. However, if entity_content is None, content_lines defaults to 1, which may not reflect the actual entity span, leading to incorrect context windows.
4. [low] In extract_source_context, the function uses 1-based line numbers for anchor_start and anchor_end but the lines vector is 0-indexed. When accessing lines[line_no - 1], if line_no equals lines.len() + 1, this will panic with an index out of bounds error.
5. [low] In merge_related_entities, entities are considered duplicates only if all fields match including content. However, the relation field is then updated to 'before_after' if relations differ. This means if the same entity appears in both before and after graphs with identical content but the content is actually different between versions, they won't be merged, leading to duplicate entries in the output.
6. [low] In list_source_files_at_ref, the function filters files using is_source_file but does not filter out noise files using is_noise_file like list_source_files does. This inconsistency means the before_graph may include noise files that the after_graph excludes, causing graph comparison issues.
7. [low] In build_graph_for_source for the GitRef case, if materialize_source_tree succeeds but EntityGraph::build fails or takes a long time, the temporary snapshot directory is only cleaned up after the build completes. If the process is interrupted or panics during graph building, the temp directory will leak.

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes

[inspect-review]

inspect review

Triage: 63 entities analyzed | 0 critical, 0 high, 33 medium, 30 low
Verdict: standard_review

Findings (7)

1. [low] In analyze_with_options, the code collects dependencies/dependents from both after and before graphs and adds them all to dependency_edges, but then uses dependent_names.len() and dependency_names.len() for counts instead of the original dependents.len() and dependencies.len(). The related_names function deduplicates by (name, file_path), which means if an entity moves files or is renamed, it will be counted as 2 separate entities in the names list, but the blast_radius calculation uses the original entity IDs which won't double-count. This creates an inconsistency where dependent_count/dependency_count may not match the actual number of unique entities.
2. [low] In collect_dependent_code and collect_dependency_code, when merging entities from before and after graphs, the code passes before_entity_id.unwrap_or(entity_id) to get dependencies/dependents from the before graph. However, if before_entity_id is None (entity was added in this change), it will use the after entity_id to query the before graph, which won't find anything. This is correct behavior for added entities, but for renamed entities where resolve_before_entity_id returns None (fails to find the entity), it will incorrectly use the new entity_id on the old graph.
3. [low] In context_span, when start_line is 0 and fallback_line is 0, it defaults to 1. Then if end_line is also 0 (less than start), it calculates end = start + content_lines - 1 = 1 + content_lines - 1. But if entity_content is None, content_lines defaults to 1, making end = 1. This means for a deleted entity with no content and no line info, the span will be (1, 1), which may not be meaningful. More critically, if entity_content is Some but empty string, lines().count() returns 1 (not 0), so an empty entity gets a 1-line span.
4. [low] In extract_source_context, the function uses 1-based line numbers for the window (window_start, window_end) but then indexes into the 0-based lines vector with lines[line_no - 1]. If entity_start_line is 0, it gets clamped to 1 via .max(1), but then window_start could be 1, and the loop does lines[1 - 1] = lines[0] which is correct. However, if entity_end_line is 0 and gets clamped to anchor_start, and then window_end is calculated, there's a potential off-by-one: the range is window_start..=window_end (inclusive), so if window_end equals lines.len(), the access lines[window_end - 1] would be lines[lines.len() - 1] which is valid. But the code does .min(lines.len()) on window_end, so if lines.len() is 10, window_end could be 10, and then lines[10 - 1] = lines[9] is valid. This appears correct on closer inspection.
5. [low] In build_context, the function calls build_graph_for_source for the before_graph only if include_before_graph is true. However, in analyze_with_options, this parameter is set based on options.include_dependency_code, not options.include_dependent_code. This means if a user sets include_dependent_code=true but include_dependency_code=false, the before_graph won't be built, and the code that tries to collect before-side dependents in collect_dependent_code will get None for before_graph and skip that collection. This seems like a logic error - the before_graph should be built if either dependent or dependency code collection is enabled.
6. [low] In resolve_before_entity_id, the function tries to find an entity in before_graph by matching (name, file_path, entity_type). However, it uses change.old_entity_name and change.old_file_path with .unwrap_or() fallbacks to current values. If an entity is renamed but the file stays the same, old_file_path will be None, so it falls back to change.file_path. But if the entity was also moved to a different file, this fallback is incorrect - it should use the old file path from the change. The code assumes old_file_path is always set for renames, but the FileChange struct has old_file_path: Option<String> which may be None even for renamed entities if only the entity name changed within the same file.
7. [low] In merge_related_entities, the deduplication logic checks if existing.relation != entity.relation and then sets it to "before_after". However, if the same entity appears 3+ times with different relations (e.g., "before", "after", "before" again from multiple calls), the first match will set it to "before_after" and subsequent matches will see existing.relation == Some("before_after") and entity.relation == Some("before"), which are not equal, so it will set it to "before_after" again (no-op). But if we have "before", "after", then "before_after" from a third source, the logic would incorrectly overwrite. This is a minor edge case but shows the merge logic is not robust.

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes