Conversation
Reviewer's GuideIntroduce a new weaver-cards crate that defines stable, serde-annotated JSON schemas and tests for the observe get-card request/response, wire a new observe get-card handler into weaverd that currently returns a structured refusal, and document the new command and roadmap completion. Sequence diagram for observe get-card request handlingsequenceDiagram
actor User
participant WeaverCLI as weaver_cli
participant Weaverd as weaverd_daemon
participant Router as DomainRouter
participant GetCard as observe_get_card_handler
participant Cards as weaver_cards_crate
participant Writer as ResponseWriter
User->>WeaverCLI: run `weaver observe get-card --uri ... --position ...`
WeaverCLI->>Weaverd: JSONL CommandRequest (domain observe, operation get-card)
Weaverd->>Router: route(request)
Router->>Router: route_observe(operation="get-card")
Router->>GetCard: handle(request, writer)
GetCard->>Cards: GetCardRequest::parse(arguments)
Cards-->>GetCard: Result<GetCardRequest, GetCardError>
alt parse ok
GetCard->>Cards: GetCardResponse::not_yet_implemented(detail)
Cards-->>GetCard: GetCardResponse::Refusal
GetCard->>Writer: write_stdout(serde_json(GetCardResponse))
GetCard-->>Router: DispatchResult(status=1)
else parse error
GetCard-->>Router: Err(DispatchError::invalid_arguments)
end
Router-->>Weaverd: DispatchResult or error
Weaverd-->>WeaverCLI: stdout JSONL stream + exit status
WeaverCLI-->>User: prints refusal JSON response
Updated class diagram for weaver-cards symbol card schemasclassDiagram
class SourcePosition {
u32 line
u32 column
}
class SourceRange {
SourcePosition start
SourcePosition end
}
class CardSymbolKind {
<<enum>>
Function
Method
Class
Interface
Type
Variable
Module
Field
}
class CardLanguage {
<<enum>>
Rust
Python
TypeScript
}
class SymbolRef {
String uri
SourceRange range
CardLanguage language
CardSymbolKind kind
String name
Option~String~ container
}
class SymbolId {
String symbol_id
}
class SymbolIdentity {
String symbol_id
SymbolRef symbol_ref
}
class DetailLevel {
<<enum>>
Minimal
Signature
Structure
Semantic
Full
}
class ParamInfo {
String name
String type_annotation
}
class SignatureInfo {
String display
Vec~ParamInfo~ params
String returns
}
class DocInfo {
String docstring
String summary
String source
}
class LocalInfo {
String name
String kind
u32 decl_line
}
class BranchInfo {
String kind
u32 line
}
class StructureInfo {
Vec~LocalInfo~ locals
Vec~BranchInfo~ branches
}
class LspInfo {
String hover
String type_info
bool deprecated
String source
}
class MetricsInfo {
u32 lines
u32 cyclomatic
Option~u32~ fan_in
Option~u32~ fan_out
}
class DepsInfo {
Vec~String~ calls
Vec~String~ imports
Vec~String~ config
}
class Provenance {
String extracted_at
Vec~String~ sources
}
class SymbolCard {
u32 card_version
SymbolIdentity symbol
Option~SignatureInfo~ signature
Option~DocInfo~ doc
Option~StructureInfo~ structure
Option~LspInfo~ lsp
Option~MetricsInfo~ metrics
Option~DepsInfo~ deps
Provenance provenance
Option~String~ etag
}
class GetCardRequest {
String uri
u32 line
u32 column
DetailLevel detail
+parse(arguments: Vec~String~) Result~GetCardRequest, GetCardError~
}
class RefusalReason {
<<enum>>
NoSymbolAtPosition
UnsupportedLanguage
NotYetImplemented
BackendUnavailable
}
class CardRefusal {
RefusalReason reason
String message
DetailLevel requested_detail
}
class GetCardResponse {
<<enum-like>>
Success(card: SymbolCard)
Refusal(refusal: CardRefusal)
+not_yet_implemented(detail: DetailLevel) GetCardResponse
}
class GetCardError {
<<enum>>
MissingArgument(flag: String)
InvalidValue(flag: String, message: String)
UnknownArgument(argument: String)
}
SourceRange *-- SourcePosition
SymbolRef *-- SourceRange
SymbolIdentity *-- SymbolRef
SymbolCard *-- SymbolIdentity
SymbolCard *-- SignatureInfo
SymbolCard *-- DocInfo
SymbolCard *-- StructureInfo
SymbolCard *-- LspInfo
SymbolCard *-- MetricsInfo
SymbolCard *-- DepsInfo
SymbolCard *-- Provenance
SignatureInfo *-- ParamInfo
StructureInfo *-- LocalInfo
StructureInfo *-- BranchInfo
CardRefusal *-- RefusalReason
GetCardResponse *-- CardRefusal
GetCardResponse *-- SymbolCard
GetCardRequest *-- DetailLevel
CardRefusal *-- DetailLevel
SymbolRef *-- CardLanguage
SymbolRef *-- CardSymbolKind
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughAdd a new workspace crate Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client
participant Router as "weaverd Router"
participant Parser as "GetCardRequest Parser"
participant Handler as "observe::get_card::handle"
participant Writer as "ResponseWriter"
Client->>Router: send CommandRequest(args)
Router->>Handler: route to "get-card"
Handler->>Parser: parse(arguments)
Parser-->>Handler: Ok(GetCardRequest) / Err(GetCardError)
alt parse error
Handler->>Writer: write DispatchError::invalid_arguments envelope
else parsed
Handler->>Handler: build GetCardResponse::not_yet_implemented(detail)
Handler->>Writer: write JSON refusal to stdout
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/weaver-cards/src/request.rs`:
- Around line 157-173: Replace the duplicated string-to-variant mapping by
implementing std::str::FromStr for DetailLevel in detail.rs (match the same
"minimal", "signature", "structure", "semantic", "full" cases and return
Err(String) with a descriptive message for unknown inputs). Then update
parse_detail_level to call value.parse::<DetailLevel>() and map any parse error
into GetCardError::InvalidValue (preserving the "--detail" flag and combining
the FromStr error text with the existing "expected one of: minimal, signature,
structure, semantic, full" hint). This centralises parsing logic into
DetailLevel::from_str while keeping parse_detail_level as a thin adapter
returning GetCardError.
In `@crates/weaver-cards/src/tests/round_trip_tests.rs`:
- Around line 14-43: Convert the plain helper minimal_card() into an rstest
fixture by adding the #[fixture] attribute (importing rstest::fixture) to the
function that returns SymbolCard and keeping its signature as fn minimal_card()
-> SymbolCard; then update any tests that used minimal_card() to accept a
parameter named minimal_card: SymbolCard so rstest injects it; ensure the
fixture constructs the same SymbolCard (symbols: SymbolCard, SymbolIdentity,
SymbolRef, SourceRange, SourcePosition, CardLanguage, CardSymbolKind,
Provenance) and that test modules have rstest in scope.
In `@docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md`:
- Around line 896-908: Update the summary header string "Modified files (7):" to
"Modified files (8):" in the document so the count matches the eight enumerated
entries and the detailed total "15 new + 8 modified"; locate the header text
"Modified files (7):" in the file and change the numeral to 8, leaving the rest
of the paragraph ("Total: ~23 file touches (15 new + 8 modified) ...")
unchanged.
- Line 43: The documentation uses British Oxford spelling but with `-ise`
suffixes; update all `-ise` variants to the en-GB-oxendict `-ize` forms in this
file (e.g., change "recognised" to "recognized", "serialised" to "serialized",
"finalised" to "finalized") and any other instances called out in the comment
(lines referenced include occurrences such as the `observe get-card` sentence
and the other listed locations); perform a case-preserving search-and-replace
for `-ise` → `-ize` across the file to ensure consistency with the repository
documentation standard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad60e19e-786c-4997-901b-75a098e2fc0b
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__snapshot_full_card.snapis excluded by!**/*.snapcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__snapshot_minimal_card.snapis excluded by!**/*.snapcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__snapshot_refusal_no_symbol.snapis excluded by!**/*.snapcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__snapshot_refusal_not_implemented.snapis excluded by!**/*.snapcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__snapshot_structure_card.snapis excluded by!**/*.snapcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__snapshot_success_response.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
Cargo.tomlcrates/weaver-cards/Cargo.tomlcrates/weaver-cards/src/card.rscrates/weaver-cards/src/detail.rscrates/weaver-cards/src/error.rscrates/weaver-cards/src/lib.rscrates/weaver-cards/src/request.rscrates/weaver-cards/src/response.rscrates/weaver-cards/src/symbol.rscrates/weaver-cards/src/tests/behaviour.rscrates/weaver-cards/src/tests/mod.rscrates/weaver-cards/src/tests/round_trip_tests.rscrates/weaver-cards/src/tests/snapshot_tests.rscrates/weaver-cards/tests/features/get_card_schema.featurecrates/weaverd/Cargo.tomlcrates/weaverd/src/dispatch/observe/get_card.rscrates/weaverd/src/dispatch/observe/mod.rscrates/weaverd/src/dispatch/router.rscrates/weaverd/src/dispatch/router/tests.rsdocs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.mddocs/repository-layout.mddocs/roadmap.mddocs/users-guide.md
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaver-cards/src/tests/snapshot_tests.rs Comment on file fn snapshot_success_response() {
let card = SymbolCard {
card_version: 1,
symbol: identity_with_container(),
signature: Some(sample_signature()),
doc: Some(sample_doc()),
structure: Some(sample_structure()),
lsp: None,
metrics: Some(sample_metrics_structure()),
deps: None,
provenance: sample_provenance(),
etag: None,
};
let response = GetCardResponse::Success {
card: Box::new(card),
};
let json = serde_json::to_string_pretty(&response).expect("serialise");
assert_snapshot!(json);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/weaver-cards/src/tests/snapshot_tests.rs (1)
138-226: 🛠️ Refactor suggestion | 🟠 MajorReplace repeated snapshot tests with
rstestparameterized cases.Refactor Lines 138-226 into
#[rstest]cases and#[fixture]setup. Preserve snapshot IDs explicitly withassert_snapshot!(name, json)to keep snapshot filenames stable.Suggested refactor sketch
+use rstest::{fixture, rstest}; + +fn snapshot_json<T: serde::Serialize>(value: &T) -> String { + serde_json::to_string_pretty(value).expect("serialize") +} + +#[fixture] +fn structure_card_fixture() -> SymbolCard { + structure_card() +} + +#[rstest] +#[case("snapshot_minimal_card", minimal_card())] +#[case("snapshot_structure_card", structure_card())] +#[case("snapshot_full_card", full_card())] +fn snapshot_cards(#[case] name: &str, #[case] card: SymbolCard) { + assert_snapshot!(name, snapshot_json(&card)); +}#!/bin/bash set -euo pipefail # Verify repeated harness usage and absence/presence of rstest constructs in this file. rg -n '^\s*#\[test\]|serde_json::to_string_pretty|assert_snapshot!\(json\)' crates/weaver-cards/src/tests/snapshot_tests.rs rg -n '\brstest\b|#\[fixture\]|#\[rstest\]' crates/weaver-cards/src/tests/snapshot_tests.rsAs per coding guidelines: "Use
rstestfixtures for shared setup in Rust tests." and "Replace duplicated tests with#[rstest(...)]parameterised cases in Rust."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cards/src/tests/snapshot_tests.rs` around lines 138 - 226, Replace the repeated #[test] functions in snapshot_tests.rs with rstest parameterized cases: turn structure_card() into a #[fixture] (fixture name e.g., structure_card) and create a #[rstest] test that takes parameters (name, card_or_response) for each snapshot case (minimal card, structure_card, full card, refusal responses, success response), call serde_json::to_string_pretty on the provided value and call assert_snapshot!(name, json) to preserve stable snapshot filenames; update the individual test functions (snapshot_minimal_card, snapshot_structure_card, snapshot_full_card, snapshot_refusal_not_implemented, snapshot_refusal_no_symbol, snapshot_success_response) to entries in the parameter list and add the required rstest imports/attributes. Ensure you reference the existing symbols SymbolCard, structure_card (fixture), GetCardResponse::not_yet_implemented, GetCardResponse::Refusal, and GetCardResponse::Success so the same sample_* helper functions and sample_provenance/etag are reused.
🤖 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/weaver-cards/src/tests/snapshot_tests.rs`:
- Around line 4-5: Update the module-level documentation comment that begins
with "//! Each test serialises..." by changing the British "-ise" spelling
"serialises" to the repository-preferred en-GB-oxendict "-ize" form "serializes"
so the doc line now reads "//! Each test serializes a fixture to pretty-printed
JSON..." (locate the module doc comment at the top of
crates/weaver-cards/src/tests/snapshot_tests.rs).
---
Duplicate comments:
In `@crates/weaver-cards/src/tests/snapshot_tests.rs`:
- Around line 138-226: Replace the repeated #[test] functions in
snapshot_tests.rs with rstest parameterized cases: turn structure_card() into a
#[fixture] (fixture name e.g., structure_card) and create a #[rstest] test that
takes parameters (name, card_or_response) for each snapshot case (minimal card,
structure_card, full card, refusal responses, success response), call
serde_json::to_string_pretty on the provided value and call
assert_snapshot!(name, json) to preserve stable snapshot filenames; update the
individual test functions (snapshot_minimal_card, snapshot_structure_card,
snapshot_full_card, snapshot_refusal_not_implemented,
snapshot_refusal_no_symbol, snapshot_success_response) to entries in the
parameter list and add the required rstest imports/attributes. Ensure you
reference the existing symbols SymbolCard, structure_card (fixture),
GetCardResponse::not_yet_implemented, GetCardResponse::Refusal, and
GetCardResponse::Success so the same sample_* helper functions and
sample_provenance/etag are reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c36df020-541b-40ee-84a2-4abd11d3c87e
📒 Files selected for processing (1)
crates/weaver-cards/src/tests/snapshot_tests.rs
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md (1)
210-210:⚠️ Potential issue | 🟡 MinorNormalize remaining
-isespellings to Oxford-izeforms.Replace the remaining mixed variants to keep the document consistent:
Line 210recognises→recognizes; Line 461serialise/deserialise→serialize/deserialize; Line 561Serialises→Serializes; Line 922serialise→serialize.As per coding guidelines: Use British English en-GB-oxendict spelling, including
-izesuffixes, acrossdocs/**/*.md.Also applies to: 461-461, 561-561, 922-922
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md` at line 210, Change the inconsistent Oxford '-ise' spellings to '-ize' forms across the document: replace "recognises" with "recognizes" (previously at the sentence containing "weaverd router recognises observe"), "serialise/deserialise" with "serialize/deserialize", "Serialises" with "Serializes", and "serialise" with "serialize"; update each exact token occurrence so all instances in this markdown use the en-GB-oxendict '-ize' suffix consistently.
🤖 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/weaver-cards/src/request.rs`:
- Around line 100-110: The function require_arg_value currently just returns the
next token and therefore treats a flag-like token (e.g., starting with '-') as a
value; update require_arg_value to peek/inspect the next item from the iterator
(iter.next()), and if the token begins with '-' (or otherwise looks like a
flag), return Err(GetCardError::InvalidValue { flag: flag.to_string(), message:
"requires a value".to_string() }) instead of accepting it; keep the same
signature and use String::as_str() for valid values so callers of
require_arg_value (the function name) still receive &str when a real value is
present.
In `@crates/weaver-cards/src/tests/snapshot_tests.rs`:
- Around line 206-243: Three near-identical tests (snapshot_refusal_no_symbol,
snapshot_refusal_unsupported_language, snapshot_refusal_backend_unavailable)
should be consolidated into a single parameterised rstest to reduce duplication:
add use rstest::rstest; create one test function (e.g., snapshot_refusal)
annotated with #[rstest] and a table of parameters for the RefusalReason
variants (NoSymbolAtPosition, UnsupportedLanguage, BackendUnavailable),
associated message strings and requested DetailLevel values, and inside the test
construct the GetCardResponse::Refusal with CardRefusal using the supplied
params, serialize with serde_json::to_string_pretty and call assert_snapshot!;
keep the same snapshot assertion and ensure each case maps to its unique input
so snapshots remain stable.
In `@crates/weaverd/src/dispatch/router/tests.rs`:
- Around line 164-167: The test currently asserts raw substrings in the response
string; instead, parse response into JSON and assert specific fields/values to
avoid accidental matches. Replace the three assert!(response.contains(...))
checks with: deserialize response via serde_json::from_str(&response) ->
serde_json::Value, then assert the expected fields under that value (for example
assert_eq!(value["error"]["code"], "not_yet_implemented"),
assert_eq!(value["error"]["type"], "refusal"), and
assert!(value["error"]["message"].as_str().unwrap().contains("structure"))—adjust
the exact keys to match the JSON schema produced by the code under test; use the
response variable from the test to locate the replacement.
---
Duplicate comments:
In `@docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md`:
- Line 210: Change the inconsistent Oxford '-ise' spellings to '-ize' forms
across the document: replace "recognises" with "recognizes" (previously at the
sentence containing "weaverd router recognises observe"),
"serialise/deserialise" with "serialize/deserialize", "Serialises" with
"Serializes", and "serialise" with "serialize"; update each exact token
occurrence so all instances in this markdown use the en-GB-oxendict '-ize'
suffix consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e03de4c-2631-4a40-a10f-b28c72741951
⛔ Files ignored due to path filters (2)
crates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__snapshot_refusal_backend_unavailable.snapis excluded by!**/*.snapcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__snapshot_refusal_unsupported_language.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
crates/weaver-cards/src/card.rscrates/weaver-cards/src/detail.rscrates/weaver-cards/src/lib.rscrates/weaver-cards/src/request.rscrates/weaver-cards/src/tests/round_trip_tests.rscrates/weaver-cards/src/tests/snapshot_tests.rscrates/weaverd/src/dispatch/router/tests.rsdocs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaver-cards/src/tests/snapshot_tests.rs Comment on file fn snapshot_refusal_no_symbol() {
let response = GetCardResponse::Refusal {
refusal: CardRefusal {
reason: RefusalReason::NoSymbolAtPosition,
message: String::from("no symbol found at the requested position"),
requested_detail: DetailLevel::Structure,
},
};
let json = serde_json::to_string_pretty(&response).expect("serialise");
assert_snapshot!(json);
}❌ New issue: Code Duplication crates/weaver-cards/src/tests/snapshot_tests.rs Comment on file fn snapshot_refusal_no_symbol() {
let response = GetCardResponse::Refusal {
refusal: CardRefusal {
reason: RefusalReason::NoSymbolAtPosition,
message: String::from("no symbol found at the requested position"),
requested_detail: DetailLevel::Structure,
},
};
let json = serde_json::to_string_pretty(&response).expect("serialise");
assert_snapshot!(json);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll check the current state of all the flagged files to see what's been addressed. 🏁 Script executed: Length of output: 37170
The two overall comments remain open. These were advisory suggestions, so you may choose to address them in a follow-up. Would you like me to open issues to track them, or generate the required code changes? |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/weaver-cards/src/tests/snapshot_tests.rs (1)
216-247: 🛠️ Refactor suggestion | 🟠 MajorCollapse refusal snapshots into one parameterised
rstest.Remove the three duplicated tests on Line 217, Line 228, and Line 239. Drive them from a single case table and keep snapshot outputs stable per case name.
Refactor patch
+use rstest::rstest; @@ -#[test] -fn snapshot_refusal_no_symbol() { - let response = refusal_response( - RefusalReason::NoSymbolAtPosition, - "no symbol found at the requested position", - DetailLevel::Structure, - ); - let json = serde_json::to_string_pretty(&response).expect("serialise"); - assert_snapshot!(json); -} - -#[test] -fn snapshot_refusal_unsupported_language() { - let response = refusal_response( - RefusalReason::UnsupportedLanguage, - "the requested language is not supported", - DetailLevel::Structure, - ); - let json = serde_json::to_string_pretty(&response).expect("serialise"); - assert_snapshot!(json); -} - -#[test] -fn snapshot_refusal_backend_unavailable() { - let response = refusal_response( - RefusalReason::BackendUnavailable, - "the required backend is not available", - DetailLevel::Semantic, - ); - let json = serde_json::to_string_pretty(&response).expect("serialise"); - assert_snapshot!(json); -} +#[rstest] +#[case( + RefusalReason::NoSymbolAtPosition, + "no symbol found at the requested position", + DetailLevel::Structure +)] +#[case( + RefusalReason::UnsupportedLanguage, + "the requested language is not supported", + DetailLevel::Structure +)] +#[case( + RefusalReason::BackendUnavailable, + "the required backend is not available", + DetailLevel::Semantic +)] +fn snapshot_refusal_variants( + #[case] reason: RefusalReason, + #[case] message: &str, + #[case] detail: DetailLevel, +) { + let response = refusal_response(reason, message, detail); + let json = serde_json::to_string_pretty(&response).expect("serialise"); + assert_snapshot!(json); +}#!/bin/bash # Verify refusal snapshot duplication and rstest adoption in this file. rg -n -C2 'fn snapshot_refusal_' crates/weaver-cards/src/tests/snapshot_tests.rs rg -n -C2 'rstest' crates/weaver-cards/src/tests/snapshot_tests.rsAs per coding guidelines: "Replace duplicated tests with
#[rstest(...)]parameterised cases in Rust."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cards/src/tests/snapshot_tests.rs` around lines 216 - 247, Remove the three duplicate tests snapshot_refusal_no_symbol, snapshot_refusal_unsupported_language, and snapshot_refusal_backend_unavailable and replace them with a single parameterised rstest that iterates cases (e.g. ("no_symbol", RefusalReason::NoSymbolAtPosition, "no symbol found at the requested position", DetailLevel::Structure), ("unsupported_language", RefusalReason::UnsupportedLanguage, "the requested language is not supported", DetailLevel::Structure), ("backend_unavailable", RefusalReason::BackendUnavailable, "the required backend is not available", DetailLevel::Semantic)); in the new test (e.g. fn snapshot_refusal(case_name: &str, reason: RefusalReason, message: &str, detail: DetailLevel)) construct the response via refusal_response(reason, message, detail), serialize to JSON and call assert_snapshot!(json, case_name) so each case has a stable snapshot name; ensure rstest attribute is used and remove the original three functions.
🤖 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/weaver-cards/src/tests/behaviour.rs`:
- Around line 96-100: The tests currently panic on invalid inputs via
build_card's chained .expect() and a hard-coded assert_eq! in
given_refusal_response; replace these with Result-based parsing helpers (e.g.,
add parse_detail_level(raw: &str) -> Result<DetailLevel, String> and
parse_refusal_reason(raw: &str) -> Result<crate::RefusalReason, String>) that
validate strings and return errors, update build_card to return
Result<SymbolCard, String> by calling parse_detail_level and build_card_at_level
(convert None to Err), and change callers (given_card_at_detail,
given_success_response, given_refusal_response) to propagate/unwrap at the step
boundary with expect() messages; in given_refusal_response map parsed
RefusalReason to the appropriate GetCardResponse/crate::CardRefusal variants
instead of asserting a single reason.
In `@docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md`:
- Around line 156-157: The Markdown line that starts with "`cargo-insta`
command-line interface (CLI) was not installed in the environment..." exceeds
the 80-character limit; edit that paragraph in
docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md so the
sentence is wrapped into multiple Markdown lines (<=80 chars per line) while
preserving the inline code backticks and punctuation; ensure the wrapped lines
form the same sentence and paragraph (no extra blank lines) so the Markdown
rendering and meaning remain unchanged.
- Around line 39-40: The Markdown line containing "Behaviour-driven development
(BDD) feature scenarios in `weaver-cards` exercise the request parsing and
response construction via `rstest-bdd` v0.5.0." is longer than 80 characters;
split or wrap this sentence into multiple lines (e.g., break after
"weaver-cards" or "response construction") preserving the inline code formatting
for `weaver-cards` and `rstest-bdd v0.5.0` so each resulting line is <= 80
columns.
---
Duplicate comments:
In `@crates/weaver-cards/src/tests/snapshot_tests.rs`:
- Around line 216-247: Remove the three duplicate tests
snapshot_refusal_no_symbol, snapshot_refusal_unsupported_language, and
snapshot_refusal_backend_unavailable and replace them with a single
parameterised rstest that iterates cases (e.g. ("no_symbol",
RefusalReason::NoSymbolAtPosition, "no symbol found at the requested position",
DetailLevel::Structure), ("unsupported_language",
RefusalReason::UnsupportedLanguage, "the requested language is not supported",
DetailLevel::Structure), ("backend_unavailable",
RefusalReason::BackendUnavailable, "the required backend is not available",
DetailLevel::Semantic)); in the new test (e.g. fn snapshot_refusal(case_name:
&str, reason: RefusalReason, message: &str, detail: DetailLevel)) construct the
response via refusal_response(reason, message, detail), serialize to JSON and
call assert_snapshot!(json, case_name) so each case has a stable snapshot name;
ensure rstest attribute is used and remove the original three functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15b6be56-5efb-4af5-86c2-a18912871724
📒 Files selected for processing (5)
crates/weaver-cards/src/request.rscrates/weaver-cards/src/tests/behaviour.rscrates/weaver-cards/src/tests/snapshot_tests.rscrates/weaverd/src/dispatch/router/tests.rsdocs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md`:
- Line 233: Replace the hardcoded machine-specific path "/home/user/project/"
with a repository-root reference phrase (e.g., "run from repository root" or use
"./" where appropriate) wherever it appears in the document (specifically the
instance in the sentence starting "Weaver is a Rust workspace at
/home/user/project/" and the other occurrence noted). Update the sentence to
remove the absolute path and instruct readers to execute commands from the
repository root so the execution plan is portable across environments.
- Around line 618-619: Update the acceptance criteria under the "Validation:"
block by replacing the optional phrase "`make markdownlint` passes (if
available)" with a mandatory requirement such as "`make markdownlint` passes`"
so markdownlint is treated as a required gate; locate the Validation section
that currently reads "**Validation:** `make fmt` passes. `make markdownlint`
passes (if available)." and make the second sentence unconditional, and if there
are any related checklist or CI notes in the same document that reference the
optional status, update them to reflect markdownlint as required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38ec28c5-fd4b-4503-9c15-7c4a8a720c6d
⛔ Files ignored due to path filters (3)
crates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__refusal_backend_unavailable.snapis excluded by!**/*.snapcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__refusal_no_symbol.snapis excluded by!**/*.snapcrates/weaver-cards/src/tests/snapshots/weaver_cards__tests__snapshot_tests__refusal_unsupported_language.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
crates/weaver-cards/src/tests/behaviour.rscrates/weaver-cards/src/tests/snapshot_tests.rsdocs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md
leynos
changed the title
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 `@docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md`:
- Around line 324-326: Change the crate-level doc comment that currently says
"sub-modules" to use the single-word "submodules" (the line describing declared
sub-modules for `symbol`, `detail`, `card`, `request`, `response`, `error`),
updating the `//!` crate-level comment so it reads "Declares submodules
(`symbol`, `detail`, `card`, `request`, `response`, `error`)."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0b27ca1-f9df-4101-91bc-5f6b1b4dc73c
📒 Files selected for processing (1)
docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md
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 `@docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md`:
- Around line 55-57: Replace the inline parenthetical reference "(lines
788-795)" in the sentence "This satisfies roadmap task 7.1.1 from
`docs/roadmap.md` (lines 788-795) and closes `#75`." with a GitHub-flavoured
numeric footnote marker [^1], and add a footnote definition at the end of the
document in order of first appearance such as "[^1]: `docs/roadmap.md`, lines
788–795" so the reference is numbered and defined per repository documentation
standards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3db3354-52c0-41b7-81ab-8c5510a77c75
📒 Files selected for processing (1)
docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md
leynos
changed the title
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 `@docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md`:
- Around line 55-56: The markdown line "This satisfies roadmap task 7.1.1 from
`docs/roadmap.md`[^1] and closes" currently breaks across a newline before the
token "#75", which is parsed as an ATX heading and triggers MD018; fix by
keeping "#75" on the same line (e.g., "…and closes `#75`.") or escape the hash
(e.g., "\#75") so the token is not treated as a heading; update the sentence
containing `docs/roadmap.md`[^1] and "#75" accordingly to satisfy markdownlint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ad53d6a-40f5-4b36-8dd1-c1e164c6e30e
📒 Files selected for processing (1)
docs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.md
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/weaver-cards/src/tests/round_trip_tests.rs (1)
55-61:⚠️ Potential issue | 🟡 MinorUse en-GB-oxendict
-izespelling in function name and assertion.Change "serialisation" to "serialization" in both the function name and the assertion message.
Proposed fix
#[rstest] -fn symbol_card_re_serialisation_is_stable(minimal_card: SymbolCard) { +fn symbol_card_re_serialization_is_stable(minimal_card: SymbolCard) { let json1 = serde_json::to_string(&minimal_card).expect("serialize 1"); let deserialized: SymbolCard = serde_json::from_str(&json1).expect("deserialize"); let json2 = serde_json::to_string(&deserialized).expect("serialize 2"); - assert_eq!(json1, json2, "re-serialisation must be byte-identical"); + assert_eq!(json1, json2, "re-serialization must be byte-identical"); }As per coding guidelines: "Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cards/src/tests/round_trip_tests.rs` around lines 55 - 61, Rename the test function symbol_card_re_serialisation_is_stable to use "serialization" (e.g. symbol_card_re_serialization_is_stable) and update the assertion message string "re-serialisation must be byte-identical" to "re-serialization must be byte-identical"; ensure any references to the function symbol (e.g. in test attributes or other tests) are updated accordingly so the test still runs.
🤖 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/weaver-cards/src/tests/behaviour.rs`:
- Around line 117-175: build_card_at_level currently only handles
DetailLevel::Minimal and ::Structure and returns None for Signature, Semantic,
and Full; either add fixture branches to return Some(SymbolCard) for
DetailLevel::Signature, DetailLevel::Semantic, and DetailLevel::Full (populating
the relevant fields like signature: Some(...), doc: Some(...),
structure/metrics/deps/etc. as appropriate) or explicitly document/guard the
function so tests that request those levels fail with a clear error; update the
match in build_card_at_level to include arms for Signature, Semantic, and Full
that construct representative SymbolCard fixtures (refer to SignatureInfo,
DocInfo, StructureInfo, MetricsInfo, and sample_provenance()) or change
callers/tests to avoid unsupported levels.
- Around line 239-259: The Gherkin step function names use "serialised" (en-GB
-ise) but the repo convention requires en-GB-oxendict "-ize", so rename the step
functions when_card_serialised and when_response_serialised to
when_card_serialized and when_response_serialized respectively, update their
when(...) attributes' step text to match "serialized" and keep the bodies
unchanged (references to TestWorld fields like json_output, card, response, and
serde_json calls remain the same); also update the corresponding step text in
tests/features/get_card_schema.feature so the feature step matches the new
"serialized" wording.
In `@docs/users-guide.md`:
- Around line 460-461: In the docs entry describing the `--format` option (the
line containing "`--format` (optional) — output format. Currently only `json`
(the default) is supported."), insert a comma after the conjunctive adverb
"Currently" so the sentence reads "Currently, only `json` (the default) is
supported." to improve readability.
---
Duplicate comments:
In `@crates/weaver-cards/src/tests/round_trip_tests.rs`:
- Around line 55-61: Rename the test function
symbol_card_re_serialisation_is_stable to use "serialization" (e.g.
symbol_card_re_serialization_is_stable) and update the assertion message string
"re-serialisation must be byte-identical" to "re-serialization must be
byte-identical"; ensure any references to the function symbol (e.g. in test
attributes or other tests) are updated accordingly so the test still runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31886afa-086d-42b3-bb95-02272ee31e8c
📒 Files selected for processing (11)
crates/weaver-cards/src/card.rscrates/weaver-cards/src/detail.rscrates/weaver-cards/src/lib.rscrates/weaver-cards/src/request.rscrates/weaver-cards/src/tests/behaviour.rscrates/weaver-cards/src/tests/round_trip_tests.rscrates/weaver-cards/src/tests/snapshot_tests.rscrates/weaverd/src/dispatch/observe/mod.rsdocs/execplans/7-1-1-stable-jsonl-schemas-for-observe-get-card.mddocs/roadmap.mddocs/users-guide.md
…ly with detail levels - Changed `build_card` to return `SymbolCard` directly instead of Option - Implemented helper functions for sample data (signature, doc, structure, lsp, metrics, deps) - Simplified `build_card_at_level` to construct SymbolCard with progressively detailed fields - Renamed serialization step functions and feature file steps to use American English spelling "serialized" - Fixed minor typos in function names and docs - Updated round-trip test for consistent American English spelling - Minor doc tweak to users-guide.md for clarity and punctuation Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/weaver-cards/src/tests/behaviour.rs`:
- Around line 334-349: The test helper then_json_field_has_value only checks
top-level keys; convert dotted keys into a JSON Pointer and use
serde_json::Value::pointer to resolve nested fields. In
then_json_field_has_value, build a pointer string like format!("/{}",
key.as_str().replace('.', "/")) and call parsed.pointer(&pointer) (instead of
parsed.get(...)) to find the nested value, keep the existing expected
serde_json::Value::String creation and assertion, and update the
expectancy/error message to include the original key string for clarity.
- Around line 286-299: The step when_card_serialized currently falls back to
serializing world.response when world.card is None; change it to only serialize
world.card by replacing the map/or_else/expect chain with a direct
check/serialize of world.card (e.g., if let Some(c) = world.card.as_ref() {
world.json_output = Some(serde_json::to_string(c).expect("serialize card")); }
else { panic!("card must be set for this step"); }) so the step fails fast when
world.card is missing and no response fallback is used.
In `@docs/users-guide.md`:
- Around line 467-469: Replace the inline roadmap reference "(7.1.2)" after
"Jacquard roadmap" with a GitHub-flavoured numeric footnote marker like [^1] in
the "Tree-sitter card extraction" note, and add a corresponding footnote
definition "[^1]: Jacquard roadmap section 7.1.2 — extraction milestone." as the
next footnote in order of appearance (append to the footnotes section or the end
of the document if none exist).
- Line 471: The sentence "When the operation cannot produce a card the status is
`\"refusal\"`:" is missing a comma after the introductory clause; update it to
"When the operation cannot produce a card, the status is `\"refusal\"`:" and
make the same punctuation change for the analogous sentence referenced at the
other location (the line mentioning the other status at 484) so both
introductory clauses end with a comma for clarity and consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2bf940c4-2e45-48d0-ae0b-3561bbe412b1
📒 Files selected for processing (4)
crates/weaver-cards/src/tests/behaviour.rscrates/weaver-cards/src/tests/round_trip_tests.rscrates/weaver-cards/tests/features/get_card_schema.featuredocs/users-guide.md
…ccess Simplified the JSON serialization logic in `when_card_serialized` by removing fallback serialization of the response and enforcing that a card must be set. Improved the JSON field access in `then_json_field_has_value` by using JSON pointer syntax to support nested keys. Also included minor documentation markdown formatting fixes unrelated to tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/weaver-cards/src/tests/behaviour.rs`:
- Around line 305-325: The two step definitions then_json_contains_field and
then_json_does_not_contain_field currently use parsed.get(field.as_str()) which
only checks top-level keys; change them to construct a JSON Pointer by replacing
dots with slashes (e.g., let pointer = format!("/{}",
field.as_str().replace('.', "/"))); then use parsed.pointer(&pointer) and assert
is_some()/is_none() respectively, preserving the existing assertion messages and
error context.
In `@docs/users-guide.md`:
- Around line 529-533: The prose claims the `structure` detail adds
`attachments` but the canonical `SymbolCard` example omits that field; either
remove `attachments` from the `structure` summary or add `attachments` to the
`SymbolCard` example so the docs match. Locate the narrative mentioning detail
levels (`minimal`, `structure`, `semantic`, `full`) and the `SymbolCard`
example, then update the `structure` line to exclude `attachments` or update the
example payload to include an `attachments` key consistent with the described
schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f1986f7-a289-44a9-a4be-6fcc150013e5
📒 Files selected for processing (2)
crates/weaver-cards/src/tests/behaviour.rsdocs/users-guide.md
…r tests Refactored tests in crates/weaver-cards to use JSON pointer syntax when checking for the presence and absence of nested JSON fields. This improves correctness and robustness of JSON validation by supporting nested keys represented with dot notation. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/users-guide.md (1)
535-539: 🛠️ Refactor suggestion | 🟠 MajorClarify whether attachments are conditionally included.
The description states that
structuredetail "addssignature,doc,attachments,structure, and basicmetrics", implying attachments are always included. If attachments are only populated when present in the source (e.g., when doc comments or decorators exist), rephrase to make the conditional semantics explicit: "optionalattachmentsare included when present."This aligns the prose with the likely schema design where attachments may be absent for symbols without documentation or decorators.
✏️ Proposed fix
Card fields beyond identity are progressively included based on the detail level. `minimal` returns only the `symbol` and `provenance` fields; -`structure` (the default) adds `signature`, `doc`, `attachments`, -`structure`, and basic `metrics`; `semantic` adds LSP hover/type +`structure` (the default) adds `signature`, `doc`, `structure`, and basic +`metrics`; optional `attachments` are included when present. `semantic` adds LSP hover/type information; `full` adds dependency edges and fan-in/out metrics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users-guide.md` around lines 535-539, clarify whether the `attachments` field is always included at the `structure` detail level or only when present. If attachments are conditional (only included when doc comments, decorators, or other metadata exist), update the prose to read: "`structure` (the default) adds `signature`, `doc`, `structure`, and basic `metrics`; optional `attachments` are included when present." This distinguishes required fields from optional enrichments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users-guide.md` around lines 535 - 539, Update the users-guide text describing detail levels so it makes attachments conditional: change the sentence that currently reads "`structure` (the default) adds `signature`, `doc`, `attachments`, `structure`, and basic `metrics`" to indicate that attachments are optional—e.g., "`structure` (the default) adds `signature`, `doc`, `structure`, and basic `metrics`; optional `attachments` are included when present." Reference the `structure` detail level and the `attachments` field in your edit to ensure the prose matches the schema semantics.crates/weaver-cards/src/tests/behaviour.rs (1)
67-222: 🧹 Nitpick | 🔵 TrivialExtract these schema fixtures into shared
rstestsupport.
sample_identity,sample_provenance,sample_signature,sample_doc,sample_structure,sample_lsp,sample_metrics,sample_deps, andbuild_card_at_levelduplicate the sample-card builders already maintained incrates/weaver-cards/src/tests/snapshot_tests.rs. Keep one source of truth so the snapshot suite and the BDD suite cannot drift onto different sample payloads.As per coding guidelines: "Use rstest fixtures for shared setup in Rust tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cards/src/tests/behaviour.rs` around lines 67 - 222, Extract the duplicated test builders (sample_identity, sample_provenance, sample_signature, sample_doc, sample_structure, sample_lsp, sample_metrics, sample_deps, and build_card_at_level) into a shared rstest fixtures module and update the BDD tests to consume them via #[fixture] (or import the shared helpers) rather than redefining them; specifically, create a common fixture provider that exposes those functions/fixtures and replace local definitions in this file (and any duplicates in snapshot_tests.rs) with calls to the shared fixtures (refer to the symbols sample_identity, sample_provenance, sample_signature, sample_doc, sample_structure, sample_lsp, sample_metrics, sample_deps, build_card_at_level and the test functions that call build_card or parse_detail_level) so both suites use the single source of truth.
🤖 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/weaver-cards/src/tests/behaviour.rs`:
- Around line 329-345: The test helper then_json_field_has_value currently
forces the expected value into a String, preventing numeric/boolean/null
comparisons; update then_json_field_has_value to parse the provided value string
into serde_json::Value using serde_json::from_str(value.as_str()) and fall back
to serde_json::Value::String(value) on parse error so numeric/boolean/null
literals compare correctly (refer to function then_json_field_has_value,
variables value, expected, and parsed).
In `@docs/users-guide.md`:
- Line 457: For the `--detail` flag description, insert a comma after the noun
phrase "progressive detail level" so the line reads "`--detail` (optional) —
progressive detail level, controlling how much" (i.e., add a comma after "level"
in the `--detail` description to separate the noun phrase from the participial
clause).
---
Duplicate comments:
In `@crates/weaver-cards/src/tests/behaviour.rs`:
- Around line 67-222: Extract the duplicated test builders (sample_identity,
sample_provenance, sample_signature, sample_doc, sample_structure, sample_lsp,
sample_metrics, sample_deps, and build_card_at_level) into a shared rstest
fixtures module and update the BDD tests to consume them via #[fixture] (or
import the shared helpers) rather than redefining them; specifically, create a
common fixture provider that exposes those functions/fixtures and replace local
definitions in this file (and any duplicates in snapshot_tests.rs) with calls to
the shared fixtures (refer to the symbols sample_identity, sample_provenance,
sample_signature, sample_doc, sample_structure, sample_lsp, sample_metrics,
sample_deps, build_card_at_level and the test functions that call build_card or
parse_detail_level) so both suites use the single source of truth.
In `@docs/users-guide.md`:
- Around line 535-539: Update the users-guide text describing detail levels so
it makes attachments conditional: change the sentence that currently reads
"`structure` (the default) adds `signature`, `doc`, `attachments`, `structure`,
and basic `metrics`" to indicate that attachments are optional—e.g.,
"`structure` (the default) adds `signature`, `doc`, `structure`, and basic
`metrics`; optional `attachments` are included when present." Reference the
`structure` detail level and the `attachments` field in your edit to ensure the
prose matches the schema semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4469757-8517-41dd-b907-3d215c81c0fa
📒 Files selected for processing (2)
crates/weaver-cards/src/tests/behaviour.rsdocs/users-guide.md
- Moved fixture builder functions from behaviour and snapshot_tests into a new shared module `tests::fixtures` for reuse and better organization. - Updated test modules to delegate fixture construction to this shared module, reducing duplication. - Modified `build_card` in behaviour tests to call new shared fixture's `build_card_at_level`. - Adjusted snapshot_tests to wrap shared fixtures with snapshot-specific data. - Added `mod fixtures;` in `tests/mod.rs` to include the new module. - Minor docs fix in users-guide.md for punctuation and formatting. This reorganization enhances maintainability and consistency of test data across weaver-cards crate tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/weaver-cards/src/tests/behaviour.rs`:
- Around line 83-103: The NotYetImplemented branch in build_refusal_response is
duplicating the canonical payload; change the function so that when reason ==
RefusalReason::NotYetImplemented you return
GetCardResponse::not_yet_implemented(detail) instead of constructing a custom
message string—keep the other RefusalReason arms as-is and ensure the function
still returns a GetCardResponse for all branches (use the helper
GetCardResponse::not_yet_implemented(detail) to produce the official refusal
payload).
In `@crates/weaver-cards/src/tests/snapshot_tests.rs`:
- Around line 94-112: The test snapshot_full_card duplicates the base payload
already produced by structure_card(), causing maintenance drift; change the test
to call structure_card() to obtain the base SymbolCard and then mutate/override
only the extra fields needed for the "full" snapshot (e.g., set signature via
fixtures::sample_signature(), doc via fixtures::sample_doc(...), lsp via
fixtures::sample_lsp(), metrics via fixtures::sample_metrics(...), deps via
sample_deps(), provenance via fixtures::sample_provenance(...), etag, etc.)
before serializing and asserting the snapshot, so shared fields like
identity_with_container(), sample_structure(), and common doc/signature are no
longer duplicated.
In `@docs/users-guide.md`:
- Around line 484-533: Clarify the coordinate rules for SymbolCard ranges by
adding a short prose note next to the example that states the indexing and
boundary semantics for card.symbol.ref.range: explicitly say whether line and
column numbers are 0‑based or 1‑based (and whether they follow the same
convention as the request flag --position), and declare if the range is
inclusive (both start and end included) or a half-open/exclusive end (e.g.,
[start, end) with end exclusive). Reference the types/names SymbolCard and
card.symbol.ref.range (and the start/end fields) so readers can find the example
and ensure consistency across request (--position) and response coordinates.
- Around line 535-540: The documentation omits the intermediate "signature"
detail level from the progressive-detail paragraph; update the description to
explicitly include a step for `signature` between `minimal` and `structure`
stating what fields it adds (e.g., returns `symbol`, `provenance`, and the
`signature` field that exposes API/type/signature information), and then
indicate that `structure` (the default) further adds `doc`, `structure`, and
basic `metrics`; keep the existing order: `minimal`, `signature`, `structure`,
`semantic`, `full`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 916029e5-1506-4159-86ff-e9a900100202
📒 Files selected for processing (5)
crates/weaver-cards/src/tests/behaviour.rscrates/weaver-cards/src/tests/fixtures.rscrates/weaver-cards/src/tests/mod.rscrates/weaver-cards/src/tests/snapshot_tests.rsdocs/users-guide.md
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaver-cards/src/tests/behaviour.rs Comment on file fn then_json_contains_field(world: &mut TestWorld, field: QuotedString) {
let json = world.json_output.as_ref().expect("JSON should be set");
let parsed: serde_json::Value = serde_json::from_str(json).expect("valid JSON");
let pointer = format!("/{}", field.as_str().replace('.', "/"));
assert!(
parsed.pointer(&pointer).is_some(),
"expected JSON to contain field '{}', got: {json}",
field.as_str()
);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…est snapshots - Simplified build_refusal_response by extracting refusal_message helper - Returned early for NotYetImplemented case - Cleaned up snapshot_full_card test to use structure_card base Additionally, updated users-guide.md to clarify zero-based ranges in card.symbol.ref.range. This improves code clarity and consistency. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/weaver-cards/src/tests/behaviour.rs (1)
169-191: 🧹 Nitpick | 🔵 TrivialDuplication between field assertion helpers is minor but present.
Both
then_json_contains_fieldandthen_json_does_not_contain_fieldshare JSON parsing and pointer construction. Extract a helper if this pattern grows.♻️ Optional refactor to reduce duplication
+fn parse_and_resolve_pointer(world: &TestWorld, field: &str) -> (serde_json::Value, String) { + let json = world.json_output.as_ref().expect("JSON should be set"); + let parsed: serde_json::Value = serde_json::from_str(json).expect("valid JSON"); + let pointer = format!("/{}", field.replace('.', "/")); + (parsed, pointer) +} + #[then("the JSON contains a {field} field")] fn then_json_contains_field(world: &mut TestWorld, field: QuotedString) { - let json = world.json_output.as_ref().expect("JSON should be set"); - let parsed: serde_json::Value = serde_json::from_str(json).expect("valid JSON"); - let pointer = format!("/{}", field.as_str().replace('.', "/")); + let (parsed, pointer) = parse_and_resolve_pointer(world, field.as_str()); assert!( parsed.pointer(&pointer).is_some(), - "expected JSON to contain field '{}', got: {json}", - field.as_str() + "expected JSON to contain field '{}', got: {parsed}", + field.as_str(), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cards/src/tests/behaviour.rs` around lines 169 - 191, The two step definitions then_json_contains_field and then_json_does_not_contain_field duplicate JSON parsing and pointer construction; extract a small helper (e.g., fn parse_json_and_pointer(world: &TestWorld, field: &QuotedString) -> (serde_json::Value, String) or similar) that: reads world.json_output, parses to serde_json::Value, builds the pointer string (format!("/{}", field.as_str().replace('.', "/"))), and returns the parsed Value and pointer; then update both then_json_contains_field and then_json_does_not_contain_field to call that helper and perform their respective assertions using the returned parsed.pointer(&pointer).
🤖 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/weaver-cards/src/tests/behaviour.rs`:
- Around line 97-108: The match in refusal_message currently uses a wildcard arm
which hides that RefusalReason::NotYetImplemented isn't handled; update the
match in the refusal_message function to explicitly include a NotYetImplemented
arm (e.g. RefusalReason::NotYetImplemented => "not yet implemented") and then
remove the wildcard arm so the match is exhaustive (this lets the compiler catch
any future added variants); reference the RefusalReason enum and the
refusal_message function when making the change.
In `@docs/users-guide.md`:
- Around line 540-546: Replace the semicolon-separated sentence describing
progressive card detail levels with a bulleted list of five items: `minimal` —
returns only the `symbol` and `provenance` fields; `signature` — adds the
`signature` block exposing the callable display string, parameters, and return
type; `structure` — (default) further adds `doc`, `structure`, and basic
`metrics` and note that optional `attachments` are included when present;
`semantic` — adds LSP hover/type information; and `full` — adds dependency edges
and fan-in/out metrics; keep all field names verbatim, start each bullet with
the backtick-quoted level name followed by an em dash, use `-` bullets, wrap
lines at ~80 columns to match repo Markdown style.
---
Duplicate comments:
In `@crates/weaver-cards/src/tests/behaviour.rs`:
- Around line 169-191: The two step definitions then_json_contains_field and
then_json_does_not_contain_field duplicate JSON parsing and pointer
construction; extract a small helper (e.g., fn parse_json_and_pointer(world:
&TestWorld, field: &QuotedString) -> (serde_json::Value, String) or similar)
that: reads world.json_output, parses to serde_json::Value, builds the pointer
string (format!("/{}", field.as_str().replace('.', "/"))), and returns the
parsed Value and pointer; then update both then_json_contains_field and
then_json_does_not_contain_field to call that helper and perform their
respective assertions using the returned parsed.pointer(&pointer).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b79d7c2c-8263-40b4-9cc8-3e642f556433
📒 Files selected for processing (3)
crates/weaver-cards/src/tests/behaviour.rscrates/weaver-cards/src/tests/snapshot_tests.rsdocs/users-guide.md
Refactor the JSON-related test step implementations in behaviour.rs by extracting common code into a helper function that parses JSON and constructs JSON pointers. This reduces duplication and improves readability. Additionally, add handling for the `NotYetImplemented` refusal reason. Also update the users-guide.md formatting for better readability of card detail levels. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/weaver-cards/src/tests/behaviour.rs`:
- Around line 83-107: Remove the duplicate NotYetImplemented message branch so
the canonical payload is only produced by GetCardResponse::not_yet_implemented:
delete the RefusalReason::NotYetImplemented arm from refusal_message and keep
the early-return in build_refusal_response that calls
GetCardResponse::not_yet_implemented(detail); ensure refusal_message only
matches the remaining RefusalReason variants (NoSymbolAtPosition,
UnsupportedLanguage, BackendUnavailable) and returns their strings, or
alternatively move the message mapping into build_refusal_response but do not
keep two different NotYetImplemented representations.
- Around line 135-144: Add a new behaviour scenario alongside
given_request_no_detail that constructs args including an unknown flag (e.g.
"--some-unknown" followed by a value) and calls GetCardRequest::parse(&args),
then assert it returns Ok (set world.request like the other test); specifically
create a new #[given("a get-card request with an unknown flag")] fn (using
TestWorld) that mirrors given_request_no_detail but appends the unknown
flag/value to the args vector and uses GetCardRequest::parse to ensure
forward-compatible unknown-flag handling is locked in.
In `@docs/users-guide.md`:
- Around line 544-545: Update the description of the `signature` block to
clarify it applies only to callable symbols: change the sentence that reads
"adds the `signature` block exposing the callable display string, parameters,
and return type" to include a parenthetical such as "(for callable symbols)"
after "`signature` block" and/or add a short note that non-callable symbols
(classes, variables, constants) may omit or structure `signature` differently so
readers understand the scope; update the `signature` detail-level description
accordingly.
- Around line 547-548: Replace the tautological sentence "Optional `attachments`
are included when present." with a concise phrase such as "May include
`attachments`." in the same paragraph that lists `metrics` and the `semantic`
entry (look for the line containing "`metrics`. Optional `attachments` are
included when present." and the following "`semantic` — adds LSP hover/type
information.") to remove redundancy.
- Around line 463-465: Replace the terse sentence "The response is a JSON object
with a \"status\" field." with a 2–3 sentence explanation that the response is a
discriminated-union envelope: when "status" is "refusal" the envelope carries a
refusal payload indicating why a card could not be produced, and when "status"
is "success" the envelope contains the card payload; note that the overall
envelope shape depends on the "status" value and keep the text wrapped at
roughly 80 columns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7af99c16-4ddb-4600-afac-d05de9a70d1b
📒 Files selected for processing (2)
crates/weaver-cards/src/tests/behaviour.rsdocs/users-guide.md
Add handling of unknown flags in get-card CLI requests so they are silently skipped. Update related behaviour tests and feature scenarios to cover this new behavior. Refactor refusal message construction to handle non-exhaustive reason variants. Enhance user guide documentation to clarify response envelope structure based on status. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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.
Uh oh!
There was an error while loading. Please reload this page.