Graph-slice: stable JSONL complete with deterministic same-file slice (7.2.1)#106
Open
Graph-slice: stable JSONL complete with deterministic same-file slice (7.2.1)#106
Conversation
…ice response - Replace not_yet_implemented refusal with real graph-slice handler - Return entry symbol card plus same-file sibling cards up to max_cards budget - Populate spillover metadata for truncated slices - Defer multi-file edge extraction and graph traversal to roadmap items 7.2.2-7.2.5 - Add end-to-end snapshot tests with 40 fixtures covering python and rust - Add new shared fixtures module for graph-slice tests in weaver-e2e - Update design docs, user guide, and roadmap to reflect stable contract This change completes roadmap item 7.2.1 by providing a stable JSON request and response schema with real runtime coverage while deferring full graph traversal to later roadmap phases. It improves user experience by providing successful graph-slice responses bounded by budget constraints and exposes spillover data for client handling of truncation. Closes #7.2.1 Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (43)
📒 Files selected for processing (15)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideImplements the runtime for observe graph-slice to return a deterministic same-file slice under the stable JSONL schema, wires it through the router with backend support, adds end-to-end fixtures and snapshots (Python/Rust), and updates docs/roadmap and user guide to reflect the shipped behavior and refusal shapes. Sequence diagram for observe graph-slice deterministic same-file slicesequenceDiagram
actor User
participant WeaverCLI as Weaver_CLI
participant DaemonRouter as Daemon_Router
participant GraphSliceHandler as Graph_Slice_Handler
participant FusionBackends as Fusion_Backends
participant CardExtractor as Card_Extractor
participant Enrichment as Enrichment_Backend
participant FileSystem as File_System
User->>WeaverCLI: run observe graph-slice (uri, line, column, budget, detail)
WeaverCLI->>DaemonRouter: JSONL CommandRequest
DaemonRouter->>GraphSliceHandler: handle(request, writer, backends)
GraphSliceHandler->>GraphSliceHandler: GraphSliceRequest.parse(arguments)
GraphSliceHandler-->>DaemonRouter: DispatchError (invalid_arguments) if parse fails
GraphSliceHandler->>GraphSliceHandler: Url.parse(request.uri)
GraphSliceHandler->>GraphSliceHandler: resolve_file_path(file_uri)
GraphSliceHandler->>FileSystem: read_to_string(path)
FileSystem-->>GraphSliceHandler: source
GraphSliceHandler->>FusionBackends: provider()
FusionBackends-->>GraphSliceHandler: SemanticBackendProvider
GraphSliceHandler->>FusionBackends: card_extractor()
FusionBackends-->>GraphSliceHandler: Card_Extractor_clone
GraphSliceHandler->>CardExtractor: extract(path, source, entry_line, entry_column, entry_detail)
alt unsupported language
CardExtractor-->>GraphSliceHandler: CardExtractionError_UnsupportedLanguage
GraphSliceHandler->>GraphSliceHandler: map_extraction_error(error)
GraphSliceHandler-->>DaemonRouter: GraphSliceResponse.Refusal(UnsupportedLanguage)
else no symbol at position
CardExtractor-->>GraphSliceHandler: CardExtractionError_NoSymbolAtPosition
GraphSliceHandler->>GraphSliceHandler: map_extraction_error(error)
GraphSliceHandler-->>DaemonRouter: GraphSliceResponse.Refusal(NoSymbolAtPosition)
else success
CardExtractor-->>GraphSliceHandler: entry_card
GraphSliceHandler->>FusionBackends: enrich_card_if_requested(entry_card, entry_detail)
GraphSliceHandler->>GraphSliceHandler: discover_same_file_cards(SameFileDiscovery)
loop each_candidate_position
GraphSliceHandler->>CardExtractor: extract(path, source, line, column, node_detail)
alt no_symbol_or_unsupported_language
CardExtractor-->>GraphSliceHandler: CardExtractionError_NoSymbolAtPosition_or_UnsupportedLanguage
GraphSliceHandler-->>GraphSliceHandler: skip_position
else success
CardExtractor-->>GraphSliceHandler: sibling_card
GraphSliceHandler->>FusionBackends: enrich_card_if_requested(sibling_card, node_detail)
GraphSliceHandler-->>GraphSliceHandler: collect_in_BTreeMap_by_symbol_id
end
end
GraphSliceHandler->>GraphSliceHandler: stable_card_order sort
GraphSliceHandler->>GraphSliceHandler: apply_card_budget(entry_card, sibling_cards, max_cards)
GraphSliceHandler-->>DaemonRouter: GraphSliceResponse.Success(cards, edges = [], spillover)
end
DaemonRouter->>WeaverCLI: JSONL response (status from exit_status)
WeaverCLI-->>User: prints deterministic same-file slice
Updated class diagram for observe graph-slice handler and related typesclassDiagram
class GraphSliceHandler {
+handle(request: CommandRequest, writer: ResponseWriter, backends: FusionBackends) Result~DispatchResult~
+build_response(request: GraphSliceRequest, path: Path, source: str, backends: FusionBackends) Result~GraphSliceResponse~
+discover_same_file_cards(discovery: SameFileDiscovery) Result~Vec_SymbolCard_~
+candidate_positions(source: str) Vec_Tuple_u32_u32_
+first_non_whitespace_column(line: str) Option_u32_
+stable_card_order(left: SymbolCard, right: SymbolCard) Ordering
+apply_card_budget(entry_card: SymbolCard, sibling_cards: Vec_SymbolCard_, max_cards: u32) Tuple_Vec_SymbolCard__SliceSpillover_
+enrich_card_if_requested(card: SymbolCard, detail: DetailLevel, source: str, backends: FusionBackends) void
+resolve_file_path(uri: Url) Result~PathBuf~
+map_extraction_error(error: CardExtractionError) Result~GraphSliceResponse~
}
class GraphSliceRequest {
+parse(args: Vec_str_) Result~GraphSliceRequest~
+uri() str
+line() u32
+column() u32
+depth() u32
+direction() SliceDirection
+edge_types() Vec_SliceEdgeType_
+min_confidence() f32
+budget() SliceBudget
+entry_detail() DetailLevel
+node_detail() DetailLevel
}
class GraphSliceResponse {
+Success
+Refusal
+not_yet_implemented() GraphSliceResponse
}
class SliceEntry {
+symbol_id: str
}
class SliceConstraints {
+depth: u32
+direction: SliceDirection
+edge_types: Vec_SliceEdgeType_
+min_confidence: f32
+budget: SliceBudget
+entry_detail: DetailLevel
+node_detail: DetailLevel
}
class SliceBudget {
+max_cards: u32
+max_edges: u32
+max_estimated_tokens: u32
}
class SliceSpillover {
+truncated: bool
+frontier: Vec_SpilloverCandidate_
+empty() SliceSpillover
}
class SpilloverCandidate {
+symbol_id: str
+depth: u32
}
class SliceRefusal {
+reason: SliceRefusalReason
+message: str
}
class SliceRefusalReason {
<<enumeration>>
UnsupportedLanguage
NoSymbolAtPosition
PositionOutOfRange
NotYetImplemented
}
class SymbolCard {
+symbol: SymbolDescriptor
+provenance: SymbolProvenance
}
class SymbolDescriptor {
+symbol_id: str
+symbol_ref: SymbolRef
}
class SymbolRef {
+container: Option_str_
+name: str
+kind: SymbolKind
+range: SourceRange
}
class SourceRange {
+start: SourcePosition
+end: SourcePosition
}
class SourcePosition {
+line: u32
+column: u32
}
class SymbolProvenance {
+sources: Vec_str_
}
class DetailLevel {
<<enumeration>>
Minimal
Structure
Semantic
}
class FusionBackends {
+provider() SemanticBackendProvider
}
class SemanticBackendProvider {
+card_extractor() CardExtractor
+lsp_client() LspClient
}
class CardExtractor {
+extract(input: CardExtractionInput) Result~SymbolCard~
}
class CardExtractionInput {
+path: Path
+source: str
+line: u32
+column: u32
+detail: DetailLevel
}
class CardExtractionError {
<<enumeration>>
UnsupportedLanguage
InvalidPath
NoSymbolAtPosition
PositionOutOfRange
Parse
}
class Enrichment {
+try_lsp_enrichment(card: SymbolCard, source: str, backends: FusionBackends) EnrichmentOutcome
}
class EnrichmentOutcome {
<<enumeration>>
Enriched
Skipped
Failed
}
class SameFileDiscovery {
+request: GraphSliceRequest
+path: Path
+source: str
+entry_symbol_id: str
+backends: FusionBackends
}
GraphSliceHandler --> GraphSliceRequest
GraphSliceHandler --> GraphSliceResponse
GraphSliceHandler --> SameFileDiscovery
GraphSliceHandler --> SliceSpillover
GraphSliceHandler --> SpilloverCandidate
GraphSliceHandler --> FusionBackends
GraphSliceHandler --> CardExtractor
GraphSliceHandler --> Enrichment
GraphSliceResponse --> SliceEntry
GraphSliceResponse --> SliceConstraints
GraphSliceResponse --> SymbolCard
GraphSliceResponse --> SliceSpillover
GraphSliceResponse --> SliceRefusal
SliceRefusal --> SliceRefusalReason
SliceConstraints --> SliceBudget
SliceConstraints --> DetailLevel
SymbolCard --> SymbolDescriptor
SymbolCard --> SymbolProvenance
SymbolDescriptor --> SymbolRef
SymbolRef --> SourceRange
SourceRange --> SourcePosition
FusionBackends --> SemanticBackendProvider
SemanticBackendProvider --> CardExtractor
SemanticBackendProvider --> Enrichment
CardExtractor --> CardExtractionInput
CardExtractor --> CardExtractionError
Enrichment --> EnrichmentOutcome
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
handle/build_response,fs::read_to_stringerrors currently bubble up as genericDispatchErrors; consider mapping IO failures (missing file, permission issues) into a clearer invalid-arguments or user-facing refusal so clients can distinguish local file problems from internal daemon faults. - The
discover_same_file_cardsimplementation calls the extractor once per non-blank line viacandidate_positions; for large files this could be quite expensive, so it may be worth adding a heuristic cap or a more symbol-aware scan to avoid O(lines) extraction calls. - In
detail_value, usingunreachable!for additionalDetailLevelvariants will cause panics if the enum grows; replacing this with a non-panicking_arm or a dedicated conversion method onDetailLevelwould make the helper more robust to future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handle`/`build_response`, `fs::read_to_string` errors currently bubble up as generic `DispatchError`s; consider mapping IO failures (missing file, permission issues) into a clearer invalid-arguments or user-facing refusal so clients can distinguish local file problems from internal daemon faults.
- The `discover_same_file_cards` implementation calls the extractor once per non-blank line via `candidate_positions`; for large files this could be quite expensive, so it may be worth adding a heuristic cap or a more symbol-aware scan to avoid O(lines) extraction calls.
- In `detail_value`, using `unreachable!` for additional `DetailLevel` variants will cause panics if the enum grows; replacing this with a non-panicking `_` arm or a dedicated conversion method on `DetailLevel` would make the helper more robust to future changes.
## Individual Comments
### Comment 1
<location path="crates/weaverd/src/dispatch/observe/graph_slice.rs" line_range="237-246" />
<code_context>
+ ))
+}
+
+fn apply_card_budget(
+ entry_card: SymbolCard,
+ sibling_cards: Vec<SymbolCard>,
+ max_cards: u32,
+) -> (Vec<SymbolCard>, SliceSpillover) {
+ let remaining_capacity = max_cards.saturating_sub(1) as usize;
+ let included_siblings = sibling_cards
+ .iter()
+ .take(remaining_capacity)
+ .cloned()
+ .collect::<Vec<_>>();
+ let frontier = sibling_cards
+ .iter()
+ .skip(remaining_capacity)
+ .map(|card| SpilloverCandidate {
+ symbol_id: card.symbol.symbol_id.clone(),
+ depth: 1,
+ })
+ .collect::<Vec<_>>();
+
+ let mut cards = Vec::with_capacity(1 + included_siblings.len());
+ cards.push(entry_card);
+ cards.extend(included_siblings);
+
+ let spillover = if frontier.is_empty() {
+ SliceSpillover::empty()
+ } else {
+ SliceSpillover {
+ truncated: true,
+ frontier,
+ }
+ };
+
+ (cards, spillover)
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Card budget logic ignores a `max_cards` value of 0 and still returns the entry card, which may violate the intended budget constraints.
In `apply_card_budget`, `remaining_capacity` uses `max_cards.saturating_sub(1)`, but the entry card is always added to `cards`. When `max_cards == 0`, the function still returns a single-card vector (the entry) and marks the slice as truncated. If the budget is intended as a strict upper bound, this violates the contract. Please handle `max_cards == 0` explicitly (e.g., return no cards and put the entry plus siblings in the frontier, or normalize `max_cards` to ≥ 1 at validation) so the behavior matches the budget semantics.
</issue_to_address>
### Comment 2
<location path="crates/weaverd/src/dispatch/observe/graph_slice.rs" line_range="139-149" />
<code_context>
+ discovery: SameFileDiscovery<'_>,
+) -> Result<Vec<SymbolCard>, DispatchError> {
+ let extractor = discovery.backends.provider().card_extractor().clone();
+ let mut cards = BTreeMap::new();
+ let mut visited_positions = BTreeSet::new();
+
+ for (line, column) in candidate_positions(discovery.source) {
</code_context>
<issue_to_address>
**suggestion:** The `visited_positions` set in `discover_same_file_cards` appears redundant given how `candidate_positions` is computed.
Here `visited_positions` deduplicates `(line, column)` pairs, but `candidate_positions` yields each line at most once and `first_non_whitespace_column` produces only one column per line. That makes the set unnecessary overhead unless you expect future changes to introduce duplicates. I’d either remove `visited_positions` or add a brief comment explaining under what conditions duplicates could appear to justify keeping it.
```suggestion
let extractor = discovery.backends.provider().card_extractor().clone();
let mut cards = BTreeMap::new();
for (line, column) in candidate_positions(discovery.source) {
if line == discovery.request.line() && column == discovery.request.column() {
continue;
}
```
</issue_to_address>
### Comment 3
<location path="crates/weaverd/src/dispatch/observe/graph_slice_tests.rs" line_range="177-186" />
<code_context>
+}
+
+#[rstest]
+fn unsupported_language_returns_structured_refusal(
+ backends: (FusionBackends<SemanticBackendProvider>, TempDir),
+) {
+ let (mut backends, temp_dir) = backends;
+ let path = write_source(&temp_dir, "notes.txt", "plain text\n");
+ let uri = Url::from_file_path(&path).expect("file uri").to_string();
+ let request = make_request(&["--uri", &uri, "--position", "1:1"]);
+
+ let (status, payload) = dispatch_payload(&request, &mut backends);
+
+ assert_eq!(status, 1);
+ assert_eq!(payload["status"], "refusal");
+ assert_eq!(payload["refusal"]["reason"], "unsupported_language");
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `NoSymbolAtPosition` and `PositionOutOfRange` refusals to cover all mapped extraction errors.
The handler maps multiple `CardExtractionError` variants to `NoSymbolAtPosition` and `PositionOutOfRange` via `map_extraction_error`, but tests currently only cover `UnsupportedLanguage`. Please add tests that:
* Trigger `NoSymbolAtPosition` (valid position with no symbol) and assert `status == 1`, `refusal.reason == "no_symbol_at_position"`, and the expected message.
* Trigger `PositionOutOfRange` (position beyond file bounds) and assert `refusal.reason == "position_out_of_range"`.
You can mirror the pattern in `unsupported_language_returns_structured_refusal`, using `write_source` and carefully chosen cursor positions, to lock down the error-to-refusal mapping and message shape.
Suggested implementation:
```rust
#[rstest]
fn unsupported_language_returns_structured_refusal(
backends: (FusionBackends<SemanticBackendProvider>, TempDir),
) {
let (mut backends, temp_dir) = backends;
let path = write_source(&temp_dir, "notes.txt", "plain text\n");
let uri = Url::from_file_path(&path).expect("file uri").to_string();
let request = make_request(&["--uri", &uri, "--position", "1:1"]);
let (status, payload) = dispatch_payload(&request, &mut backends);
assert_eq!(status, 1);
assert_eq!(payload["status"], "refusal");
assert_eq!(payload["refusal"]["reason"], "unsupported_language");
}
#[rstest]
fn no_symbol_at_position_returns_structured_refusal(
backends: (FusionBackends<SemanticBackendProvider>, TempDir),
) {
let (mut backends, temp_dir) = backends;
// Use a supported language extension so we exercise symbol extraction
let path = write_source(&temp_dir, "main.rs", "fn main() {}\n");
let uri = Url::from_file_path(&path).expect("file uri").to_string();
// Choose a position that is within the file but on whitespace (no symbol)
// Line/column are 1-based in the CLI interface.
let request = make_request(&["--uri", &uri, "--position", "1:4"]);
let (status, payload) = dispatch_payload(&request, &mut backends);
assert_eq!(status, 1);
assert_eq!(payload["status"], "refusal");
assert_eq!(payload["refusal"]["reason"], "no_symbol_at_position");
// The handler should surface a helpful message; assert the shape here.
let message = payload["refusal"]["message"]
.as_str()
.expect("refusal message");
assert!(
message.to_lowercase().contains("no symbol") || message.to_lowercase().contains("no node"),
"unexpected refusal message for no_symbol_at_position: {message}"
);
}
#[rstest]
fn position_out_of_range_returns_structured_refusal(
backends: (FusionBackends<SemanticBackendProvider>, TempDir),
) {
let (mut backends, temp_dir) = backends;
let path = write_source(&temp_dir, "main.rs", "fn main() {}\n");
let uri = Url::from_file_path(&path).expect("file uri").to_string();
// Position beyond file bounds: line 10, column 1 for a single‑line file
let request = make_request(&["--uri", &uri, "--position", "10:1"]);
let (status, payload) = dispatch_payload(&request, &mut backends);
assert_eq!(status, 1);
assert_eq!(payload["status"], "refusal");
assert_eq!(payload["refusal"]["reason"], "position_out_of_range");
let message = payload["refusal"]["message"]
.as_str()
.expect("refusal message");
assert!(
message.to_lowercase().contains("out of range")
|| message.to_lowercase().contains("outside file bounds"),
"unexpected refusal message for position_out_of_range: {message}"
);
}
```
These tests assume:
1. `main.rs` is treated as a supported language by the graph slice handler. If your handler uses a different extension (e.g. `.py`, `.ts`), update the filename accordingly.
2. Positions are 1-based `line:column` as in the existing tests. If they are 0-based, adjust the `--position` arguments.
3. The exact refusal messages may differ slightly. If `map_extraction_error` uses a specific phrasing, tighten the `assert!` conditions on `message` to match the exact expected string instead of `contains(...)` checks.
</issue_to_address>
### Comment 4
<location path="crates/weaverd/src/dispatch/observe/graph_slice_tests.rs" line_range="86-95" />
<code_context>
+ serde_json::to_string_pretty(transcript).expect("serialize transcript")
+}
+
+#[rstest]
+#[case::python_01(PYTHON_CASES[0])]
+#[case::python_02(PYTHON_CASES[1])]
</code_context>
<issue_to_address>
**suggestion (testing):** Extend invalid-argument tests to cover non-`file://` URIs and malformed URIs.
The new `handle` logic now validates URIs with `Url::parse` and `resolve_file_path`, returning `DispatchError::invalid_arguments` for unsupported schemes and malformed URIs, but the current `invalid_arguments_return_dispatch_error` tests only cover missing `--uri` and bad `--position`.
Please add rstest cases like:
* `&[
</issue_to_address>
### Comment 5
<location path="crates/weaverd/src/dispatch/observe/graph_slice.rs" line_range="128" />
<code_context>
+ })
+}
+
+struct SameFileDiscovery<'a> {
+ request: &'a GraphSliceRequest,
+ path: &'a Path,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `discover_same_file_cards` by removing the `SameFileDiscovery` struct, extracting card-extraction/error-handling into a helper, and separating enrichment from provenance normalization to make the control flow flatter and easier to read.
- `SameFileDiscovery` currently adds indirection without much benefit and is immediately destructured in `discover_same_file_cards`. You can simplify the call site and function signature by passing explicit parameters, which makes both sides easier to read and removes the need to mentally jump to the struct definition:
```rust
fn discover_same_file_cards(
request: &GraphSliceRequest,
path: &Path,
source: &str,
entry_symbol_id: &str,
backends: &mut FusionBackends<SemanticBackendProvider>,
) -> Result<Vec<SymbolCard>, DispatchError> {
let extractor = backends.provider().card_extractor().clone();
// ... same body, replacing discovery.* with the params ...
}
// call site
let sibling_cards = discover_same_file_cards(
request,
path,
source,
&entry_symbol_id,
backends,
)?;
```
- The main loop in `discover_same_file_cards` is doing a lot: candidate generation, extraction, error handling, entry-skip, enrichment, and deduplication. Extracting the extraction/error-handling piece into a helper that returns `Option<SymbolCard>` flattens the loop and gives you one place to manage the `CardExtractionError` logic for siblings:
```rust
fn extract_card_at_position(
extractor: &impl CardExtractorTrait, // adjust to actual trait
path: &Path,
source: &str,
line: u32,
column: u32,
detail: DetailLevel,
) -> Result<Option<SymbolCard>, DispatchError> {
match extractor.extract(CardExtractionInput {
path,
source,
line,
column,
detail,
}) {
Ok(card) => Ok(Some(card)),
Err(CardExtractionError::NoSymbolAtPosition { .. }) => Ok(None),
Err(CardExtractionError::UnsupportedLanguage { .. }) => Ok(None),
Err(CardExtractionError::PositionOutOfRange { .. }) => {
let message = format!(
"computed position {line}:{column} should be valid during same-file slice discovery: {error}"
);
Err(DispatchError::internal(message))
}
Err(CardExtractionError::InvalidPath { path }) => Err(DispatchError::internal(
format!(
"Tree-sitter extractor requires an absolute path: {}",
path.display()
),
)),
Err(CardExtractionError::Parse { language, message }) => Err(
DispatchError::internal(format!(
"Tree-sitter parse failed for {language}: {message}"
)),
),
}
}
```
Then the loop becomes:
```rust
for (line, column) in candidate_positions(source) {
if (line, column) == (request.line(), request.column()) {
continue;
}
if !visited_positions.insert((line, column)) {
continue;
}
let Some(mut card) = extract_card_at_position(
&extractor,
path,
source,
line,
column,
request.node_detail(),
)? else {
continue;
};
if card.symbol.symbol_id == entry_symbol_id {
continue;
}
enrich_card_if_requested(&mut card, request.node_detail(), source, backends);
cards.entry(card.symbol.symbol_id.clone()).or_insert(card);
}
```
This keeps all current behavior, but centralizes error handling and removes a large `match` from the loop.
- `enrich_card_if_requested` is mixing the decision of “should we enrich?” with provenance normalization. Pulling the provenance mutation into a small helper makes the control flow easier to scan:
```rust
fn normalize_lsp_provenance(provenance: &mut ProvenanceType) {
provenance
.sources
.retain(|source_name| source_name != "tree_sitter_degraded_semantic");
if !provenance
.sources
.iter()
.any(|source_name| source_name == "lsp_hover")
{
provenance.sources.push(String::from("lsp_hover"));
}
}
fn enrich_card_if_requested(
card: &mut SymbolCard,
detail: DetailLevel,
source: &str,
backends: &mut FusionBackends<SemanticBackendProvider>,
) {
if detail < DetailLevel::Semantic {
return;
}
if enrich::try_lsp_enrichment(card, source, backends) == EnrichmentOutcome::Enriched {
normalize_lsp_provenance(&mut card.provenance);
}
}
```
This keeps semantics intact while making enrichment behavior more declarative and easier to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+177
to
+186
| fn unsupported_language_returns_structured_refusal( | ||
| backends: (FusionBackends<SemanticBackendProvider>, TempDir), | ||
| ) { | ||
| let (mut backends, temp_dir) = backends; | ||
| let path = write_source(&temp_dir, "notes.txt", "plain text\n"); | ||
| let uri = Url::from_file_path(&path).expect("file uri").to_string(); | ||
| let request = make_request(&["--uri", &uri, "--position", "1:1"]); | ||
|
|
||
| let (status, payload) = dispatch_payload(&request, &mut backends); | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Add tests for NoSymbolAtPosition and PositionOutOfRange refusals to cover all mapped extraction errors.
The handler maps multiple CardExtractionError variants to NoSymbolAtPosition and PositionOutOfRange via map_extraction_error, but tests currently only cover UnsupportedLanguage. Please add tests that:
- Trigger
NoSymbolAtPosition(valid position with no symbol) and assertstatus == 1,refusal.reason == "no_symbol_at_position", and the expected message. - Trigger
PositionOutOfRange(position beyond file bounds) and assertrefusal.reason == "position_out_of_range".
You can mirror the pattern in unsupported_language_returns_structured_refusal, using write_source and carefully chosen cursor positions, to lock down the error-to-refusal mapping and message shape.
Suggested implementation:
#[rstest]
fn unsupported_language_returns_structured_refusal(
backends: (FusionBackends<SemanticBackendProvider>, TempDir),
) {
let (mut backends, temp_dir) = backends;
let path = write_source(&temp_dir, "notes.txt", "plain text\n");
let uri = Url::from_file_path(&path).expect("file uri").to_string();
let request = make_request(&["--uri", &uri, "--position", "1:1"]);
let (status, payload) = dispatch_payload(&request, &mut backends);
assert_eq!(status, 1);
assert_eq!(payload["status"], "refusal");
assert_eq!(payload["refusal"]["reason"], "unsupported_language");
}
#[rstest]
fn no_symbol_at_position_returns_structured_refusal(
backends: (FusionBackends<SemanticBackendProvider>, TempDir),
) {
let (mut backends, temp_dir) = backends;
// Use a supported language extension so we exercise symbol extraction
let path = write_source(&temp_dir, "main.rs", "fn main() {}\n");
let uri = Url::from_file_path(&path).expect("file uri").to_string();
// Choose a position that is within the file but on whitespace (no symbol)
// Line/column are 1-based in the CLI interface.
let request = make_request(&["--uri", &uri, "--position", "1:4"]);
let (status, payload) = dispatch_payload(&request, &mut backends);
assert_eq!(status, 1);
assert_eq!(payload["status"], "refusal");
assert_eq!(payload["refusal"]["reason"], "no_symbol_at_position");
// The handler should surface a helpful message; assert the shape here.
let message = payload["refusal"]["message"]
.as_str()
.expect("refusal message");
assert!(
message.to_lowercase().contains("no symbol") || message.to_lowercase().contains("no node"),
"unexpected refusal message for no_symbol_at_position: {message}"
);
}
#[rstest]
fn position_out_of_range_returns_structured_refusal(
backends: (FusionBackends<SemanticBackendProvider>, TempDir),
) {
let (mut backends, temp_dir) = backends;
let path = write_source(&temp_dir, "main.rs", "fn main() {}\n");
let uri = Url::from_file_path(&path).expect("file uri").to_string();
// Position beyond file bounds: line 10, column 1 for a single‑line file
let request = make_request(&["--uri", &uri, "--position", "10:1"]);
let (status, payload) = dispatch_payload(&request, &mut backends);
assert_eq!(status, 1);
assert_eq!(payload["status"], "refusal");
assert_eq!(payload["refusal"]["reason"], "position_out_of_range");
let message = payload["refusal"]["message"]
.as_str()
.expect("refusal message");
assert!(
message.to_lowercase().contains("out of range")
|| message.to_lowercase().contains("outside file bounds"),
"unexpected refusal message for position_out_of_range: {message}"
);
}These tests assume:
main.rsis treated as a supported language by the graph slice handler. If your handler uses a different extension (e.g..py,.ts), update the filename accordingly.- Positions are 1-based
line:columnas in the existing tests. If they are 0-based, adjust the--positionarguments. - The exact refusal messages may differ slightly. If
map_extraction_erroruses a specific phrasing, tighten theassert!conditions onmessageto match the exact expected string instead ofcontains(...)checks.
Comment on lines
+86
to
+95
| #[rstest] | ||
| fn valid_request_returns_success_and_echoed_constraints( | ||
| backends: (FusionBackends<SemanticBackendProvider>, TempDir), | ||
| ) { | ||
| let (mut backends, temp_dir) = backends; | ||
| let path = write_source( | ||
| &temp_dir, | ||
| "slice.rs", | ||
| concat!( | ||
| "struct Counter(u32);\n\n", |
There was a problem hiding this comment.
suggestion (testing): Extend invalid-argument tests to cover non-file:// URIs and malformed URIs.
The new handle logic now validates URIs with Url::parse and resolve_file_path, returning DispatchError::invalid_arguments for unsupported schemes and malformed URIs, but the current invalid_arguments_return_dispatch_error tests only cover missing --uri and bad --position.
Please add rstest cases like:
- `&[
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e431fb22e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+257
to
+259
| let mut cards = Vec::with_capacity(1 + included_siblings.len()); | ||
| cards.push(entry_card); | ||
| cards.extend(included_siblings); |
There was a problem hiding this comment.
When callers pass --max-cards 0 (currently allowed by parsing), apply_card_budget still unconditionally appends entry_card, so the response can return 1 card while echoing constraints.budget.max_cards = 0. This breaks the documented budget contract that cards must fit within the applied budget and can cause downstream consumers that trust the echoed budget to mis-handle responses.
Useful? React with 👍 / 👎.
Comment on lines
+210
to
+211
| line.char_indices() | ||
| .find_map(|(index, ch)| (!ch.is_whitespace()).then_some((index as u32) + 1)) |
There was a problem hiding this comment.
first_non_whitespace_column derives the column from char_indices() byte offsets, but extraction APIs treat columns as character positions. On lines with multibyte leading whitespace/text, this computes a larger-than-actual column, which can select the wrong symbol or raise PositionOutOfRange during same-file discovery and abort an otherwise valid graph-slice request.
Useful? React with 👍 / 👎.
…discovery - Enforce `max_cards >= 1` at parse time and add defensive zero-budget guard at runtime. - Cap same-file discovery to 256 positions to avoid unbounded extraction cost. - Correct column computation in candidate positions to use character offsets instead of UTF-8 byte offsets. - Map local source file read failures to invalid argument dispatch errors for clearer client feedback. - Add comprehensive tests for refusals, invalid arguments, and edge cases related to position parsing. - Normalize LSP enrichment provenance to exclude degraded semantic source and include "lsp_hover" when enriched. These changes harden the 7.2.1 observe graph-slice implementation based on review findings and improve robustness and user experience. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication, Large Assertion Blocks)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| graph_slice_tests.rs | 2 advisory rules | 10.00 → 8.82 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Comment on lines
+214
to
+226
| fn position_out_of_range_returns_structured_refusal( | ||
| backends: (FusionBackends<SemanticBackendProvider>, TempDir), | ||
| ) { | ||
| let (mut backends, temp_dir) = backends; | ||
| let path = write_source(&temp_dir, "main.rs", "fn main() {}\n"); | ||
| let uri = Url::from_file_path(&path).expect("file uri").to_string(); | ||
| let request = make_request(&["--uri", &uri, "--position", "10:1"]); | ||
|
|
||
| let (status, payload) = dispatch_payload(&request, &mut backends); | ||
|
|
||
| assert_eq!(status, 1); | ||
| assert_eq!(payload["status"], "refusal"); | ||
| assert_eq!(payload["refusal"]["reason"], "position_out_of_range"); |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 3 functions with similar structure: no_symbol_at_position_returns_structured_refusal,position_out_of_range_returns_structured_refusal,unsupported_language_returns_structured_refusal
Comment on lines
+214
to
+226
| fn position_out_of_range_returns_structured_refusal( | ||
| backends: (FusionBackends<SemanticBackendProvider>, TempDir), | ||
| ) { | ||
| let (mut backends, temp_dir) = backends; | ||
| let path = write_source(&temp_dir, "main.rs", "fn main() {}\n"); | ||
| let uri = Url::from_file_path(&path).expect("file uri").to_string(); | ||
| let request = make_request(&["--uri", &uri, "--position", "10:1"]); | ||
|
|
||
| let (status, payload) = dispatch_payload(&request, &mut backends); | ||
|
|
||
| assert_eq!(status, 1); | ||
| assert_eq!(payload["status"], "refusal"); | ||
| assert_eq!(payload["refusal"]["reason"], "position_out_of_range"); |
There was a problem hiding this comment.
❌ New issue: Large Assertion Blocks
The test suite contains 4 assertion blocks with at least 4 assertions, threshold = 4
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/graph_slice.rs`:
- Around line 220-241: The comparator stable_card_order is allocating a String
each comparison via format!("{:?}", left_ref.kind) / right_ref.kind); change the
tuple key to compare the SymbolKind values directly instead of formatting them,
and update the tuple elements to use left_ref.kind and right_ref.kind (or
otherwise compare SymbolKind) so no heap allocation occurs in stable_card_order;
additionally add Ord (and PartialOrd if needed) to the derives for the
SymbolKind enum in node.rs (the SymbolKind definition currently derives Debug,
Clone, Copy, PartialEq, Eq) so SymbolKind can be compared directly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fea81a2b-fe2c-4c19-a97b-895e2c3a8d26
⛔ Files ignored due to path filters (43)
crates/weaver-e2e/tests/snapshots/graph_slice_python_abstract_base_class.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_async_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_class_init_methods.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_classmethod_staticmethod.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_complex_types.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_control_flow.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_dataclass.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_decorator_stack.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_default_params.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_generator_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_google_docstring.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_import_block.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_lambda_assignment.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_module_doc_and_imports.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_module_variable.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_nested_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_numpy_docstring.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_property_decorator.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_simple_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_python_varargs_kwargs.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_refusal_unsupported_language.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_async_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_attribute_macro.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_closure_assignment.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_const_static.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_control_flow.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_derive_macro.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_doc_comments.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_enum_definition.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_generics_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_impl_methods.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_lifetime_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_module_doc_and_uses.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_result_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_simple_function.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_struct_definition.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_trait_definition.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_trait_impl.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_tuple_struct.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_type_alias.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_rust_use_block.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_truncated_python_classmethod_staticmethod.snapis excluded by!**/*.snapcrates/weaver-e2e/tests/snapshots/graph_slice_truncated_rust_trait_impl.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
crates/weaver-cards/src/graph_slice/parse.rscrates/weaver-cards/src/graph_slice/request_tests.rscrates/weaver-e2e/src/graph_slice_fixtures/mod.rscrates/weaver-e2e/src/graph_slice_fixtures/python.rscrates/weaver-e2e/src/graph_slice_fixtures/rust.rscrates/weaver-e2e/src/lib.rscrates/weaver-e2e/tests/graph_slice_snapshots.rscrates/weaver-e2e/tests/test_support/mod.rscrates/weaverd/src/dispatch/observe/graph_slice.rscrates/weaverd/src/dispatch/observe/graph_slice_tests.rscrates/weaverd/src/dispatch/router.rsdocs/execplans/7-2-1-define-stable-jsonl-request-and-response-schemas-for-observe-graph-slice.mddocs/jacquard-card-first-symbol-graph-design.mddocs/roadmap.mddocs/users-guide.md
Comment on lines
+220
to
+241
| fn stable_card_order(left: &SymbolCard, right: &SymbolCard) -> std::cmp::Ordering { | ||
| let left_ref = &left.symbol.symbol_ref; | ||
| let right_ref = &right.symbol.symbol_ref; | ||
| ( | ||
| left_ref.container.as_deref().unwrap_or_default(), | ||
| left_ref.name.as_str(), | ||
| format!("{:?}", left_ref.kind), | ||
| left_ref.range.start.line, | ||
| left_ref.range.start.column, | ||
| left_ref.range.end.line, | ||
| left_ref.range.end.column, | ||
| ) | ||
| .cmp(&( | ||
| right_ref.container.as_deref().unwrap_or_default(), | ||
| right_ref.name.as_str(), | ||
| format!("{:?}", right_ref.kind), | ||
| right_ref.range.start.line, | ||
| right_ref.range.start.column, | ||
| right_ref.range.end.line, | ||
| right_ref.range.end.column, | ||
| )) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if SymbolKind implements Ord
ast-grep --pattern $'#[derive($$$Ord$$$)]
$$$
enum SymbolKind {
$$$
}'Repository: leynos/weaver
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Find SymbolKind definition
rg "enum SymbolKind" -A 10 --type rsRepository: leynos/weaver
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Find SymbolKind definition
rg "enum SymbolKind" -A 15Repository: leynos/weaver
Length of output: 950
🏁 Script executed:
#!/bin/bash
# Get the full SymbolKind definition including derives
rg "#\[derive" -B 5 -A 15 crates/weaver-graph/src/node.rs | grep -A 15 "SymbolKind"Repository: leynos/weaver
Length of output: 1138
🏁 Script executed:
#!/bin/bash
# Check for manual Ord implementation for SymbolKind
rg "impl.*Ord.*SymbolKind|impl.*SymbolKind.*Ord" -A 5Repository: leynos/weaver
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Verify SymbolKind implements Copy (confirming Ord can be safely derived)
rg "Copy" crates/weaver-graph/src/node.rs | grep -i symbolkindRepository: leynos/weaver
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check the actual code location mentioned in the review
cat -n crates/weaverd/src/dispatch/observe/graph_slice.rs | sed -n '215,245p'Repository: leynos/weaver
Length of output: 1257
🏁 Script executed:
#!/bin/bash
# Get the exact line with the derive for SymbolKind to confirm Copy
sed -n '1,50p' crates/weaver-graph/src/node.rs | rg "#\[derive" -A 2 | head -20Repository: leynos/weaver
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Get exact line number and derive for SymbolKind
rg -n "enum SymbolKind" -B 1 crates/weaver-graph/src/node.rsRepository: leynos/weaver
Length of output: 129
Remove unnecessary heap allocations in sort comparator.
format!("{:?}", kind) allocates a new String on each comparison call. Replace with direct SymbolKind comparison:
♻️ Suggested refactor
fn stable_card_order(left: &SymbolCard, right: &SymbolCard) -> std::cmp::Ordering {
let left_ref = &left.symbol.symbol_ref;
let right_ref = &right.symbol.symbol_ref;
(
left_ref.container.as_deref().unwrap_or_default(),
left_ref.name.as_str(),
- format!("{:?}", left_ref.kind),
+ left_ref.kind,
left_ref.range.start.line,
left_ref.range.start.column,
left_ref.range.end.line,
left_ref.range.end.column,
)
.cmp(&(
right_ref.container.as_deref().unwrap_or_default(),
right_ref.name.as_str(),
- format!("{:?}", right_ref.kind),
+ right_ref.kind,
right_ref.range.start.line,
right_ref.range.start.column,
right_ref.range.end.line,
right_ref.range.end.column,
))
}Add Ord to the derives for SymbolKind in crates/weaver-graph/src/node.rs (currently only derives Debug, Clone, Copy, PartialEq, Eq).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaverd/src/dispatch/observe/graph_slice.rs` around lines 220 - 241,
The comparator stable_card_order is allocating a String each comparison via
format!("{:?}", left_ref.kind) / right_ref.kind); change the tuple key to
compare the SymbolKind values directly instead of formatting them, and update
the tuple elements to use left_ref.kind and right_ref.kind (or otherwise compare
SymbolKind) so no heap allocation occurs in stable_card_order; additionally add
Ord (and PartialOrd if needed) to the derives for the SymbolKind enum in node.rs
(the SymbolKind definition currently derives Debug, Clone, Copy, PartialEq, Eq)
so SymbolKind can be compared directly.
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.
Summary
Changes
Why
This change locks the stable JSONL contract for observe graph-slice, delivering a deterministic, same-file slice now, while preserving the plan to implement full graph traversal and multi-file edge extraction in later milestones (7.2.2 and beyond). It provides a robust testing surface via 40 fixture-based end-to-end snapshots and ensures downstream tooling can rely on a stable, predictable schema.
Testing Plan
Migration / Compatibility
Notes for Reviewers
References
📎 Task: https://www.devboxer.com/task/ccab0145-07d2-4f5a-862d-c1678eba521f