🐛 fixes for docs/makefile and async work

[shawn-hurley]

This will make the full analysis mode on my Mac run in about 2 minutes, which is much more in line with everything else. That is the biggest fix. I added OTEL to trace what is happening so we can debug this a little more easily next time.

I also had help from Claude, who taught me a little about Tokio and how to actually use it, and it turns out I was doing it all wrong. It also helped simplify the types and other such things.

There was also a bug in the makefile that was causing a local run with containers to fail. It was never updated to work with any UUID containers, so that was causing an issue as well.

This is a big change, but with the docs and fix-ups, working with the project again should be easier.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR adds an opt-in telemetry provider (OpenTelemetry OTLP tracing and a Prometheus metrics endpoint) as a new public provider::telemetry module with global metrics, tracer init/shutdown, context extraction, and a minimal metrics HTTP server. It instruments many gRPC handlers and graph/project operations with tracing/metrics, moves blocking I/O and heavy work into spawn_blocking/blocking JoinSet tasks, changes many symbol/query APIs from owned String to &str, replaces several Arc<Mutex<_>> patterns with RwLock or plain Mutex, converts subprocess calls to tokio::process::Command, tightens error handling, updates container/Docker/Make/Cargo files, and updates documentation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 I hopped through spans and counters bright,
Traces chased the morning light,
Borrowed strings kept memory slight,
Blocking tasks tucked in at night,
Metrics hum — the garden's right.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: bug fixes for documentation/Makefile and async/Tokio work improvements.
Description check	✅ Passed	The description is directly related to the changeset, explaining the performance improvements, OTEL tracing addition, Tokio async corrections, and Makefile container UID fixes.
Docstring Coverage	✅ Passed	Docstring coverage is 83.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

src/provider/csharp.rs (2)

101-106: ⚠️ Potential issue | 🟠 Major

Keep unsupported full configs source-only for compatibility.

This turns a previously ignored config value into an init-time UNIMPLEMENTED error. Existing callers that still send full will now fail initialization even though the provider has historically coerced everything to source-only.

Based on learnings In the c-sharp-analyzer-provider codebase (src/provider/csharp.rs), the analysis_mode in the init function is intentionally hard-coded to AnalysisMode::SourceOnly, ignoring any value from config. Full analysis is not supported. The maintainers prefer explicit code documentation over runtime warnings or errors to communicate this constraint.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/csharp.rs` around lines 101 - 106, Replace the runtime
UNIMPLEMENTED error and use the historical compatibility behavior by forcing
analysis_mode to AnalysisMode::SourceOnly in init instead of reading
saved_config; specifically, in the init function replace the block that
constructs analysis_mode from saved_config and returns
Err(Status::unimplemented(...)) with a direct assignment analysis_mode =
AnalysisMode::SourceOnly and add a short comment explaining that full analysis
is unsupported and intentionally coerced to source-only for backward
compatibility (remove the Status::unimplemented usage and any early return that
rejects "Full" configs).

---

163-179: ⚠️ Potential issue | 🟠 Major

Don't overlap SDK XML indexing with the main graph build.

Both spawned branches call project.load_sdk_from_path(), which writes to self.db_path, while init() keeps going into project.get_project_graph() before the handle is awaited. On a cold start that creates concurrent SQLite work against the same file and makes init nondeterministic.

Also applies to: 195-229

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/csharp.rs` around lines 163 - 179, Concurrent calls to
project.load_sdk_from_path() (spawned via tokio::spawn in the SdkSource match
arms) race with init()'s subsequent project.get_project_graph() and cause
simultaneous SQLite writes; remove the overlapping concurrency by not spawning
those load tasks or by collecting their JoinHandles and awaiting them (or
awaiting the load future inline) before allowing init() to proceed to
project.get_project_graph(); update the SdkSource::Found/Downloaded branches
that create the tokio::spawn (and the clone/guard/read logic around project_arc
and project.load_sdk_from_path) so the SDK loading completes (and returns its
Result) prior to continuing with graph construction.

src/provider/dependency_resolution.rs (1)

202-219: ⚠️ Potential issue | 🟠 Major

Check external command exit status before continuing.

ilspycmd and paket add are both treated as success as long as the process starts. A non-zero exit here currently returns a decompile directory that was never produced or falls through into a later "reference assembly" lookup error, which hides the real tool failure.

Also applies to: 675-681

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/dependency_resolution.rs` around lines 202 - 219, The code
currently ignores the exit status of external commands (e.g.,
Command::new(ilspycmd) producing decompile_output), so a non-zero exit can lead
to returning a decompile directory that doesn't exist; update the call sites
(the ilspy decompile block and the other paket add block around lines 675-681)
to check decompile_output.status.success() (or equivalent for the paket
command), and if false log or include the process stderr/stdout in the error
message and return an Err with context instead of continuing—use the existing
decompile_output variable, the ilspycmd invocation, and the paket add invocation
names to locate and modify the code paths.

src/main.rs (1)

141-155: ⚠️ Potential issue | 🟠 Major

Propagate listener failures out of main.

These branches only log bind/serve errors and then fall through to Ok(()). If the port/socket cannot be bound, the process still exits with status 0, so containers and supervisors can treat a failed start as healthy.

Also applies to: 166-175, 177-191, 199-213

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main.rs` around lines 141 - 155, The gRPC serve blocks (inside
rt.block_on awaiting
Server::builder()...serve(...).with_current_subscriber().await) currently only
log errors and return Ok(()), which hides startup failures; change them to
propagate the error from the await (e.g. convert the Err(e) branch to return
Err(e.into()) or use the ? operator so main returns a non-zero exit on
bind/serve failures). Update the same pattern for the other serve blocks
mentioned (the blocks around the Server::builder().serve(...).await occurrences)
so any listener bind/serve error bubbles out of main instead of silently logging
and succeeding.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/provider/code_snip.rs`:
- Line 83: The displayed line numbers are currently 0-based because the code
builds them as `skip_lines + index`; update the formatting expression where the
snippet lines are produced (the `format!("{} {}\n", skip_lines + index, line)`
call that uses `skip_lines` and `index`) to add 1 for user-facing display so it
becomes `skip_lines + index + 1`, ensuring line numbers are 1-based for users.

In `@src/provider/csharp.rs`:
- Around line 358-363: The current info-level log prints the entire request
(info!("request: {:?}", r)) including user-supplied condition payload and
transport metadata; replace that with logging only the specific, non-sensitive
fields from evaluate_request (e.g., identifiers or small summary fields from
evaluate_request.condition_info) or lower it to debug level with redaction.
Locate the block that gets evaluate_request (variable evaluate_request =
r.get_ref()) and change the info! invocation to log only selected fields from
evaluate_request (or remove it and keep the existing debug!("evaluate request:
{:?}", evaluate_request.condition_info) if that suffices), ensuring you do NOT
interpolate the entire r or full payload at info level.
- Around line 297-307: project.load_to_database() now returns Result but its
error is only logged while the function continues and records an "ok" metric;
change this to handle errors by matching the Result from
project.load_to_database().await: on Ok(...) proceed as before; on Err(e) log
the error with context, increment the init failure metric (e.g.,
METRICS.grpc_requests_total.with_label_values(&["init","err"]).inc()), observe
duration, and fail the RPC (either return an Err(Status::internal(format!(...)))
or return an InitResponse with successful: false and an explanatory message)
instead of recording the "ok" metric and returning success—update the code
around project.load_to_database(), the METRICS.grpc_requests_total increment,
and the creation/return of InitResponse accordingly.

In `@src/provider/dependency_resolution.rs`:
- Around line 796-800: The error handler for adding SDK XML files currently
resets current_graph (StackGraph::new()) and reloads symbols via
SourceType::load_symbols_into_graph, which discards earlier successful work and
breaks combined_file_to_tag/success_count consistency; instead, remove the reset
and reload calls so the handler only logs the error (keep error!("Failed to add
SDK XML file {:?} to graph: {}", file, e)) and then continue to the next file,
ensuring combined_file_to_tag and success_count are only mutated on successful
additions to current_graph (i.e., update combined_file_to_tag and increment
success_count inside the success branch where the graph was actually modified).

In `@src/provider/telemetry.rs`:
- Around line 301-313: The spawned task for each accepted connection currently
blocks forever on tokio::io::AsyncReadExt::read(&mut stream, &mut buf).await;
add a timeout around that read (use tokio::time::timeout with a reasonable
Duration, e.g., a few seconds) in the closure created after listener.accept(),
and if the timeout elapses treat it as a failed/ignored request: log/debug the
timeout, drop/close the stream, and return from the async task; keep normal read
success behavior unchanged so metrics are still served when bytes arrive.

---

Outside diff comments:
In `@src/main.rs`:
- Around line 141-155: The gRPC serve blocks (inside rt.block_on awaiting
Server::builder()...serve(...).with_current_subscriber().await) currently only
log errors and return Ok(()), which hides startup failures; change them to
propagate the error from the await (e.g. convert the Err(e) branch to return
Err(e.into()) or use the ? operator so main returns a non-zero exit on
bind/serve failures). Update the same pattern for the other serve blocks
mentioned (the blocks around the Server::builder().serve(...).await occurrences)
so any listener bind/serve error bubbles out of main instead of silently logging
and succeeding.

In `@src/provider/csharp.rs`:
- Around line 101-106: Replace the runtime UNIMPLEMENTED error and use the
historical compatibility behavior by forcing analysis_mode to
AnalysisMode::SourceOnly in init instead of reading saved_config; specifically,
in the init function replace the block that constructs analysis_mode from
saved_config and returns Err(Status::unimplemented(...)) with a direct
assignment analysis_mode = AnalysisMode::SourceOnly and add a short comment
explaining that full analysis is unsupported and intentionally coerced to
source-only for backward compatibility (remove the Status::unimplemented usage
and any early return that rejects "Full" configs).
- Around line 163-179: Concurrent calls to project.load_sdk_from_path() (spawned
via tokio::spawn in the SdkSource match arms) race with init()'s subsequent
project.get_project_graph() and cause simultaneous SQLite writes; remove the
overlapping concurrency by not spawning those load tasks or by collecting their
JoinHandles and awaiting them (or awaiting the load future inline) before
allowing init() to proceed to project.get_project_graph(); update the
SdkSource::Found/Downloaded branches that create the tokio::spawn (and the
clone/guard/read logic around project_arc and project.load_sdk_from_path) so the
SDK loading completes (and returns its Result) prior to continuing with graph
construction.

In `@src/provider/dependency_resolution.rs`:
- Around line 202-219: The code currently ignores the exit status of external
commands (e.g., Command::new(ilspycmd) producing decompile_output), so a
non-zero exit can lead to returning a decompile directory that doesn't exist;
update the call sites (the ilspy decompile block and the other paket add block
around lines 675-681) to check decompile_output.status.success() (or equivalent
for the paket command), and if false log or include the process stderr/stdout in
the error message and return an Err with context instead of continuing—use the
existing decompile_output variable, the ilspycmd invocation, and the paket add
invocation names to locate and modify the code paths.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 87d999df-94ef-4c35-a865-5af4f2c21b1b

📥 Commits

Reviewing files that changed from the base of the PR and between 57af5b3 and 09466d6.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (25)

• Cargo.toml
• Dockerfile
• Makefile
• README.md
• docs/architecture.md
• docs/development.md
• docs/testing.md
• src/c_sharp_graph/class_query.rs
• src/c_sharp_graph/dependency_xml_analyzer.rs
• src/c_sharp_graph/field_query.rs
• src/c_sharp_graph/language_config.rs
• src/c_sharp_graph/loader.rs
• src/c_sharp_graph/method_query.rs
• src/c_sharp_graph/namespace_query.rs
• src/c_sharp_graph/query.rs
• src/c_sharp_graph/results.rs
• src/main.rs
• src/pipe_stream/server.rs
• src/provider/code_snip.rs
• src/provider/csharp.rs
• src/provider/dependency_resolution.rs
• src/provider/mod.rs
• src/provider/project.rs
• src/provider/target_framework.rs
• src/provider/telemetry.rs

💤 Files with no reviewable changes (1)

• src/pipe_stream/server.rs

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how start_position.line is defined in the proto or related code
rg -n "start_position" --type=rust -C3 | head -60

# Check proto definition for Position
fd -e proto | xargs rg -n "Position" -A5

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 4250

---

🏁 Script executed:

# Get the context around line 83 in code_snip.rs
cat -n src/provider/code_snip.rs | head -100

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 4419

---

🏁 Script executed:

# Find tests for code_snip
rg -n "code_snip" --type=rust -l

# Check for any tests related to GetCodeSnipRequest
rg -n "GetCodeSnipRequest" --type=rust -C5

# Look for how start_position.line values are set in the codebase
rg -n "start_position\.line.*=" --type=rust -C2

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 5888

---

🏁 Script executed:

# Look at how start_position.line is set in csharp.rs (context from earlier results showed line 737-739)
sed -n '730,750p' src/provider/csharp.rs

# Search for where line numbers come from in the analyzer
rg -n "line_number" --type=rust -C2 | head -80

# Look for any test setup that might show expected indexing
rg -n "#\[test\]" --type=rust -A10 | grep -A10 "code_location\|Position\|line"

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 8631

---

🏁 Script executed:

# Look for where lines are extracted from tree-sitter or C# analyzer
rg -n "tree.sitter\|tree_sitter" --type=rust -C3

# Search for any indexing hints or comments
rg -n "0.index\|1.index\|zero.index\|one.index" --type=rust

# Look at the proto file definition to understand Position semantics
fd -e proto -exec cat {} \;

# Search for any documentation about line numbering
rg -n "line.*index\|1.indexed\|0.indexed" --type=rust --type=markdown

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 215

---

🏁 Script executed:

# Find proto files
fd -e proto

# Look for tree-sitter usage
rg -n "tree_sitter\|tree-sitter" --type=rust -C3

# Look for Documentation or comments about line indexing
rg -n "0.*index|1.*index" -i --type=rust --type=markdown | head -30

# Check if there's any usage of tree_sitter that shows indexing
rg -n "start\|line" --type=rust | grep -i "position\|index" | head -20

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 108

---

🏁 Script executed:

# Read the proto file to understand Position semantics
cat -n src/build/proto/provider.proto

# Also check if there's any documentation or comments in the repo about line numbering
rg -n "line.*number\|Position.*line" --type=rust --type=markdown -C2 | head -60

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 6564

---

🏁 Script executed:

# Search for where line numbers are extracted from C# analysis or tree-sitter
rg -n "start_line\|start_position" --type=rust -B3 -A1 | head -100

# Look for any LSP-related imports or references
rg -n "lsp\|Language.*Server" --type=rust -i

# Search for how positions are populated from external sources
rg -n "from\|convert\|parse" --type=rust | grep -i "position\|line" | head -20

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 60

---

🏁 Script executed:

# Look for Position struct construction in the code
rg -n "Position\s*{" --type=rust -A3

# Look for all locations where line values are assigned
rg -n "\.line\s*=" --type=rust -B2 -A1

# Check if there's any external dependency or integration
rg -n "extern\|use.*analyzer\|use.*external" --type=rust | head -30

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 165

---

🏁 Script executed:

# Find Position construction with simpler pattern
rg -n "Position {" --type=rust -A3

# Look for line assignments
rg -n "line:" --type=rust | head -30

# Search for where positions are created from external data
rg -n "ResultNode\|Result {" --type=rust -A5 | head -60

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 863

---

🏁 Script executed:

# Look at query.rs to see where source_info comes from
sed -n '410,430p' src/c_sharp_graph/query.rs

# Get more context around line 416 and 420
sed -n '400,440p' src/c_sharp_graph/query.rs

# Search for source_info definition
rg -n "source_info" --type=rust -B2 -A2 | head -50

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 6072

---

🏁 Script executed:

# Find where span is defined
rg -n "span" --type=rust -B2 -A2 | grep -i "struct\|pub\|line" | head -40

# Look for tree-sitter usage which might populate span
rg -n "tree.sitter\|tree_sitter" --type=rust -i

# Look for imports related to graph or span
rg -n "^use\|^import" --type=rust src/c_sharp_graph/query.rs

# Check results.rs for span definition
head -200 src/c_sharp_graph/results.rs

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 9777

---

🏁 Script executed:

# Look for where source_info.span comes from - check graph implementation
rg -n "source_info\|\.span" --type=rust src/c_sharp_graph/ | grep -i "fn\|struct\|pub" | head -30

# Look for any documentation or comments about indexing in results.rs
head -50 src/c_sharp_graph/results.rs

# Search for tree-sitter integration
find . -name "*.rs" -type f -exec grep -l "tree.sitter\|tree_sitter" {} \;

# Look for Cargo.toml to see dependencies
cat Cargo.toml | grep -A5 -B5 "tree\|sitter"

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 2734

---

🏁 Script executed:

# Search for tree-sitter usage and how it provides line numbers
rg -n "tree.sitter\|stack.graph" --type=rust -i -B3 -A3 | head -80

# Look for any conversion between tree-sitter indexing and displayed indexing
rg -n "offset\|index\|line.*0\|line.*1" --type=rust | grep -i "tree\|sitter\|convert" | head -20

# Check if there's any adjustment when creating Position objects from tree-sitter
rg -n "span\.start\.line\|span\.end\.line" --type=rust -B2 -A2

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 60

---

🏁 Script executed:

# Try to find tree-sitter imports
rg -n "use.*tree" --type=rust

# Look for any documentation comments about line indexing
rg -n "0.*index|1.*index|0-based|1-based" --type=rust -i

# Check the actual query.rs file to understand the span structure better
sed -n '1,50p' src/c_sharp_graph/query.rs

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 2576

---

🌐 Web query:

tree-sitter line number indexing 0-based 1-based convention

💡 Result:

Tree-sitter uses 0-based indexing for line numbers (rows) and column numbers in its Point and Range structures. The first line of a file is row 0, and the first column on a line is column 0. This applies to node positions (ts_node_start_point, ts_node_end_point), input edits (TSInputEdit), and ranges (TSRange). Byte offsets are also 0-based. This is the core library convention, maintained for API consistency despite user-facing tools like editors using 1-based line numbers.

Citations:

• 1: Count lines starting from 1, not 0 tree-sitter/tree-sitter#4653
• 2: 0-indexed line numbers in CLI output tree-sitter/tree-sitter#287
• 3: https://tree-sitter.github.io/py-tree-sitter/classes/tree_sitter.Point.html
• 4: https://deepwiki.com/tree-sitter/py-tree-sitter/3.6-range-and-point
• 5: https://tree-sitter.github.io/tree-sitter/using-parsers/2-basic-parsing.html

---

Fix line number output to use 1-based indexing for user display.

The output format on line 83 uses skip_lines + index, which produces 0-based line numbers because start_position.line comes from tree-sitter (which uses 0-based indexing). Users expect 1-based line numbers, so the output should be skip_lines + index + 1 to match editor conventions.

Example: For the first line of a file, tree-sitter provides line 0, but users expect to see line 1 in the code snippet.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/code_snip.rs` at line 83, The displayed line numbers are
currently 0-based because the code builds them as `skip_lines + index`; update
the formatting expression where the snippet lines are produced (the `format!("{}
{}\n", skip_lines + index, line)` call that uses `skip_lines` and `index`) to
add 1 for user-facing display so it becomes `skip_lines + index + 1`, ensuring
line numbers are 1-based for users.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Fail init when dependency/SDK graph loading fails.

project.load_to_database() now returns a real Result, but this path only logs it and still returns successful: true. That leaves the provider running with a partial graph while the init metrics are recorded as "ok".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/csharp.rs` around lines 297 - 307, project.load_to_database()
now returns Result but its error is only logged while the function continues and
records an "ok" metric; change this to handle errors by matching the Result from
project.load_to_database().await: on Ok(...) proceed as before; on Err(e) log
the error with context, increment the init failure metric (e.g.,
METRICS.grpc_requests_total.with_label_values(&["init","err"]).inc()), observe
duration, and fail the RPC (either return an Err(Status::internal(format!(...)))
or return an InitResponse with successful: false and an explanatory message)
instead of recording the "ok" metric and returning success—update the code
around project.load_to_database(), the METRICS.grpc_requests_total increment,
and the creation/return of InitResponse accordingly.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Stop logging the full evaluate request at info level.

This emits the entire user-supplied condition payload into normal logs on every evaluation, and can also drag along transport metadata. Log the specific fields you need instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/csharp.rs` around lines 358 - 363, The current info-level log
prints the entire request (info!("request: {:?}", r)) including user-supplied
condition payload and transport metadata; replace that with logging only the
specific, non-sensitive fields from evaluate_request (e.g., identifiers or small
summary fields from evaluate_request.condition_info) or lower it to debug level
with redaction. Locate the block that gets evaluate_request (variable
evaluate_request = r.get_ref()) and change the info! invocation to log only
selected fields from evaluate_request (or remove it and keep the existing
debug!("evaluate request: {:?}", evaluate_request.condition_info) if that
suffices), ensuring you do NOT interpolate the entire r or full payload at info
level.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't discard prior SDK XML work after one bad file.

This error branch resets current_graph, but it leaves combined_file_to_tag and success_count intact. After a single failure, later DB writes can no longer resolve earlier successful files from the reset graph, so the aggregate load becomes internally inconsistent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/dependency_resolution.rs` around lines 796 - 800, The error
handler for adding SDK XML files currently resets current_graph
(StackGraph::new()) and reloads symbols via SourceType::load_symbols_into_graph,
which discards earlier successful work and breaks
combined_file_to_tag/success_count consistency; instead, remove the reset and
reload calls so the handler only logs the error (keep error!("Failed to add SDK
XML file {:?} to graph: {}", file, e)) and then continue to the next file,
ensuring combined_file_to_tag and success_count are only mutated on successful
additions to current_graph (i.e., update combined_file_to_tag and increment
success_count inside the success branch where the graph was actually modified).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Add a timeout before waiting for metrics request bytes.

Each accepted connection spawns a task that blocks on read() before responding, even though the request contents are ignored. A client that connects and stays idle can hold a task and socket indefinitely and exhaust the metrics endpoint.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/telemetry.rs` around lines 301 - 313, The spawned task for each
accepted connection currently blocks forever on
tokio::io::AsyncReadExt::read(&mut stream, &mut buf).await; add a timeout around
that read (use tokio::time::timeout with a reasonable Duration, e.g., a few
seconds) in the closure created after listener.accept(), and if the timeout
elapses treat it as a failed/ignored request: log/debug the timeout, drop/close
the stream, and return from the async task; keep normal read success behavior
unchanged so metrics are still served when bytes arrive.

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

README.md (1)

194-201: ⚠️ Potential issue | 🟡 Minor

Add language specifier to fenced code block.

The code block starting at line 196 is missing a language specifier, which triggers MD040 linting warnings.

📝 Proposed fix

 # Full test suite with server management
-make run-tests
+```bash
+make run-tests
+```

Note: The issue is that line 196 shows make run-tests but the code block should have a language specifier. Looking at the context, this appears to be inside a code block that needs bash specified.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 194 - 201, The fenced code block containing the
commands "make run-tests" and the two "cargo ..." lines is missing a language
specifier causing MD040; update the opening fence to include the language (e.g.,
change ``` to ```bash) so the block becomes a bash-highlighted code block and
the linter warning is resolved.

src/provider/dependency_resolution.rs (1)

202-219: ⚠️ Potential issue | 🟠 Major

Check ILSpy and Paket exit codes before continuing.

The code captures decompile_output (line 202-219) and paket_reference_output (line 675-681) but never checks their exit status. output().await? only reports spawn failures; a non-zero exit from either tool returns Ok(Output), allowing execution to continue with missing or invalid artifacts. This pattern is already correctly handled elsewhere in the same file at line 282 with status.success(), but is missing at both call sites.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/dependency_resolution.rs` around lines 202 - 219, The spawned
external commands (Command::new(ilspycmd) producing decompile_output and the
paket invocation producing paket_reference_output) must have their exit statuses
checked before proceeding: after awaiting .output() inspect
decompile_output.status.success() and paket_reference_output.status.success()
and return an Err or bail with a clear message that includes stderr/stdout and
the relevant command (include decompile_out_name or reference_assemblies for
context) when success() is false; update the code around the ilspy Command and
the paket invocation to mirror the existing pattern used at the other call site
(use Output.status.success() and include Output.stderr in the error message).

♻️ Duplicate comments (5)

src/provider/code_snip.rs (1)

82-84: ⚠️ Potential issue | 🟡 Minor

Return 1-based line numbers in the snippet text.

The rendered prefix is still skip_lines + index, so the first displayed line in a file is 0. This is user-facing output and should be shifted to 1-based numbering.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/code_snip.rs` around lines 82 - 84, The snippet prefixes are
0-based because the closure in the iterator mapping uses format!("{} {}",
skip_lines + index, line); update the mapping to output 1-based line numbers by
adding 1 to the computed number (i.e., use skip_lines + index + 1) in the Ok
branch of the closure that handles (index, s) so displayed lines start at 1;
keep the Err branch unchanged.

src/provider/csharp.rs (2)

297-307: ⚠️ Potential issue | 🟠 Major

Fail init when load_to_database() fails.

This still logs the Result and then increments the "init","ok" metric and returns successful: true. A failed dependency/SDK DB load leaves the provider partially initialized but reported as healthy.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/csharp.rs` around lines 297 - 307, The code currently ignores
errors from project.load_to_database() and always increments
METRICS.grpc_requests_total with ["init","ok"] and returns a successful
InitResponse; change the logic in the init handler to inspect the result of
project.load_to_database(): if res is Err, log the error (include the error
details), call
METRICS.grpc_requests_total.with_label_values(&["init","err"]).inc(), observe
duration with init_timer.observe_duration(), and return a failed gRPC response
(e.g., Err(Status::internal(...)) or an InitResponse indicating failure) instead
of proceeding to the successful branch; keep the existing successful path
(incrementing ["init","ok"], observe duration, and returning InitResponse) only
for the Ok(res) case.

---

358-363: ⚠️ Potential issue | 🟠 Major

Stop logging the full evaluate request at info level.

This still prints the entire condition payload plus request metadata on every call. Log only a small redacted summary at info, and keep the full payload at debug/trace if needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/csharp.rs` around lines 358 - 363, Replace the info-level dump
of the full request (the info!("request: {:?}", r) call) with a small redacted
summary that does not include the full condition payload or full request
metadata; for example log only a short identifier and sanitized metadata (use
evaluate_request to extract an id/name or log condition_info length instead) and
keep the existing debug!("evaluate request: {:?}",
evaluate_request.condition_info) for the full payload; update the
tracing::Span/metrics code to remain unchanged but remove sensitive details from
the info! call so only non-sensitive summary data is emitted.

src/provider/dependency_resolution.rs (1)

796-800: ⚠️ Potential issue | 🟠 Major

Don't discard previously indexed SDK XML state after one bad file.

Resetting current_graph here throws away the successfully merged files, but combined_file_to_tag and success_count are preserved. The later DB write then runs against mismatched graph/tag state.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/dependency_resolution.rs` around lines 796 - 800, The error
branch currently discards the merged state by resetting current_graph via
StackGraph::new() and reloading symbols with
SourceType::load_symbols_into_graph, causing combined_file_to_tag and
success_count to become inconsistent; instead, in the Err(e) arm for the SDK XML
add (the block that logs "Failed to add SDK XML file {:?} to graph: {}"), remove
the current_graph = StackGraph::new() and the subsequent load_symbols_into_graph
call so the existing current_graph and tag mappings remain intact, log the error
and skip that file (do not modify combined_file_to_tag or success_count for the
failed file) so processing can continue safely with the previously merged state.

src/provider/telemetry.rs (1)

301-313: ⚠️ Potential issue | 🟠 Major

Don't wait forever for bytes you never use.

Each accepted metrics connection still blocks on read() before serving metrics. A client that connects and stays idle can keep the spawned task and socket open indefinitely and starve the endpoint.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/telemetry.rs` around lines 301 - 313, The spawned connection
handler currently waits indefinitely on tokio::io::AsyncReadExt::read after
listener.accept, allowing a slow/idle client to hold the task and socket; wrap
that read in a tokio::time::timeout (e.g. Duration::from_secs(1)) so the read
returns quickly on idle and you can proceed to serve metrics or close the
socket; specifically update the code inside the tokio::spawn handler that calls
AsyncReadExt::read on stream to use tokio::time::timeout(...) around the read,
handle timeout by treating it as zero bytes (or logging and continuing) and then
continue with the metric response/flush and socket shutdown.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Dockerfile`:
- Line 16: The Dockerfile currently installs dotnet-sdk-9.0 but only
dotnet-runtime-8.0 (see the RUN microdnf install line), which will break
execution of projects targeting net9.0 (the repo accepts net9.0 via
is_valid_base_tfm() and maps it to SDK 9.0 in to_channel()); update the
Dockerfile to also install dotnet-runtime-9.0 alongside dotnet-runtime-8.0 (or
explicitly restrict/document that net9.0 container execution is unsupported) so
runtime and SDK versions match for net9.0 projects.

In `@docs/testing.md`:
- Around line 284-292: The docs example uses the Podman-specific hostname
"host.containers.internal" in the OTEL_EXPORTER_OTLP_ENDPOINT env value which
will not work for Docker users; update the docs/testing.md example (the
container run snippet that sets OTEL_EXPORTER_OTLP_ENDPOINT) to include a brief
note or alternate example showing Docker's hostname "host.docker.internal"
(e.g., an added sentence or comment directly after the code block) so readers
know to use host.docker.internal when running the same command with Docker.

In `@src/provider/code_snip.rs`:
- Around line 63-65: The snippet width calculation currently omits the requested
end line and can underflow for reversed ranges; update the calculation so take
is computed safely as the inclusive line count plus trailing context: set take =
end_position.line.saturating_sub(start_position.line).saturating_add(1).saturating_add(context_lines)
(casting to usize as needed) and keep skip_lines =
start_position.line.saturating_sub(context_lines) to avoid wrapping for reversed
ranges and ensure single-line requests return one line.

In `@src/provider/csharp.rs`:
- Around line 117-126: The code currently stores the new Project into
self.project before completing language-config validation, graph loading,
dependency resolution, and DB loading, which can leave a half-initialized
project visible to RPCs; instead, keep the newly created Project in a local
variable and perform all validation/load steps (language-config checks, graph
loading, dependency resolution, DB loading) first, and only after all succeed
acquire the write lock and assign *self.project = Some(project) (update the
block that currently writes to self.project and the subsequent read_owned usage)
so that self.project is updated atomically after successful initialization.

In `@src/provider/project.rs`:
- Around line 296-304: The current code calls SerializableStackGraph::from_graph
and then attempts serializable_graph.load_into(&mut graph) but only logs errors;
because load_into can partially populate graph, a failing load can still leave
graph.iter_symbols().count() > 0 and cause a corrupted cache to be treated as
valid. Update the load error handling in the block handling
serializable_graph.load_into to treat deserialization failures as cache misses
by returning Ok(None) (or alternatively propagate the error) instead of just
logging; specifically modify the Err(e) branch that currently calls
debug!("unable to load graph: {}", e) so it returns Ok(None) (or returns the
error) to force rebuilding from source.

---

Outside diff comments:
In `@README.md`:
- Around line 194-201: The fenced code block containing the commands "make
run-tests" and the two "cargo ..." lines is missing a language specifier causing
MD040; update the opening fence to include the language (e.g., change ``` to
```bash) so the block becomes a bash-highlighted code block and the linter
warning is resolved.

In `@src/provider/dependency_resolution.rs`:
- Around line 202-219: The spawned external commands (Command::new(ilspycmd)
producing decompile_output and the paket invocation producing
paket_reference_output) must have their exit statuses checked before proceeding:
after awaiting .output() inspect decompile_output.status.success() and
paket_reference_output.status.success() and return an Err or bail with a clear
message that includes stderr/stdout and the relevant command (include
decompile_out_name or reference_assemblies for context) when success() is false;
update the code around the ilspy Command and the paket invocation to mirror the
existing pattern used at the other call site (use Output.status.success() and
include Output.stderr in the error message).

---

Duplicate comments:
In `@src/provider/code_snip.rs`:
- Around line 82-84: The snippet prefixes are 0-based because the closure in the
iterator mapping uses format!("{} {}", skip_lines + index, line); update the
mapping to output 1-based line numbers by adding 1 to the computed number (i.e.,
use skip_lines + index + 1) in the Ok branch of the closure that handles (index,
s) so displayed lines start at 1; keep the Err branch unchanged.

In `@src/provider/csharp.rs`:
- Around line 297-307: The code currently ignores errors from
project.load_to_database() and always increments METRICS.grpc_requests_total
with ["init","ok"] and returns a successful InitResponse; change the logic in
the init handler to inspect the result of project.load_to_database(): if res is
Err, log the error (include the error details), call
METRICS.grpc_requests_total.with_label_values(&["init","err"]).inc(), observe
duration with init_timer.observe_duration(), and return a failed gRPC response
(e.g., Err(Status::internal(...)) or an InitResponse indicating failure) instead
of proceeding to the successful branch; keep the existing successful path
(incrementing ["init","ok"], observe duration, and returning InitResponse) only
for the Ok(res) case.
- Around line 358-363: Replace the info-level dump of the full request (the
info!("request: {:?}", r) call) with a small redacted summary that does not
include the full condition payload or full request metadata; for example log
only a short identifier and sanitized metadata (use evaluate_request to extract
an id/name or log condition_info length instead) and keep the existing
debug!("evaluate request: {:?}", evaluate_request.condition_info) for the full
payload; update the tracing::Span/metrics code to remain unchanged but remove
sensitive details from the info! call so only non-sensitive summary data is
emitted.

In `@src/provider/dependency_resolution.rs`:
- Around line 796-800: The error branch currently discards the merged state by
resetting current_graph via StackGraph::new() and reloading symbols with
SourceType::load_symbols_into_graph, causing combined_file_to_tag and
success_count to become inconsistent; instead, in the Err(e) arm for the SDK XML
add (the block that logs "Failed to add SDK XML file {:?} to graph: {}"), remove
the current_graph = StackGraph::new() and the subsequent load_symbols_into_graph
call so the existing current_graph and tag mappings remain intact, log the error
and skip that file (do not modify combined_file_to_tag or success_count for the
failed file) so processing can continue safely with the previously merged state.

In `@src/provider/telemetry.rs`:
- Around line 301-313: The spawned connection handler currently waits
indefinitely on tokio::io::AsyncReadExt::read after listener.accept, allowing a
slow/idle client to hold the task and socket; wrap that read in a
tokio::time::timeout (e.g. Duration::from_secs(1)) so the read returns quickly
on idle and you can proceed to serve metrics or close the socket; specifically
update the code inside the tokio::spawn handler that calls AsyncReadExt::read on
stream to use tokio::time::timeout(...) around the read, handle timeout by
treating it as zero bytes (or logging and continuing) and then continue with the
metric response/flush and socket shutdown.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d6f523e5-ff79-4e76-bf8e-3616d8e53070

📥 Commits

Reviewing files that changed from the base of the PR and between 09466d6 and ae8ffff.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (25)

• Cargo.toml
• Dockerfile
• Makefile
• README.md
• docs/architecture.md
• docs/development.md
• docs/testing.md
• src/c_sharp_graph/class_query.rs
• src/c_sharp_graph/dependency_xml_analyzer.rs
• src/c_sharp_graph/field_query.rs
• src/c_sharp_graph/language_config.rs
• src/c_sharp_graph/loader.rs
• src/c_sharp_graph/method_query.rs
• src/c_sharp_graph/namespace_query.rs
• src/c_sharp_graph/query.rs
• src/c_sharp_graph/results.rs
• src/main.rs
• src/pipe_stream/server.rs
• src/provider/code_snip.rs
• src/provider/csharp.rs
• src/provider/dependency_resolution.rs
• src/provider/mod.rs
• src/provider/project.rs
• src/provider/target_framework.rs
• src/provider/telemetry.rs

💤 Files with no reviewable changes (1)

• src/pipe_stream/server.rs

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider noting Docker vs Podman hostname differences.

The example uses host.containers.internal which is Podman-specific. Docker users would need host.docker.internal. Consider adding a note or alternative.

Suggested documentation addition

 podman run --pod analyzer-c-sharp --name c-sharp -d \
   -e OTEL_EXPORTER_OTLP_ENDPOINT=http://host.containers.internal:4317 \
   -e OTEL_SERVICE_NAME=c-sharp-provider \
   -e METRICS_PORT=9090 \
   -v test-data:/analyzer-lsp/examples:U,z \
   c-sharp-provider:latest --port 14651

+> Note: For Docker, use host.docker.internal instead of host.containers.internal.

</details>

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

```suggestion
For container-based tests, pass OTEL env vars to the provider container:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/testing.md` around lines 284 - 292, The docs example uses the
Podman-specific hostname "host.containers.internal" in the
OTEL_EXPORTER_OTLP_ENDPOINT env value which will not work for Docker users;
update the docs/testing.md example (the container run snippet that sets
OTEL_EXPORTER_OTLP_ENDPOINT) to include a brief note or alternate example
showing Docker's hostname "host.docker.internal" (e.g., an added sentence or
comment directly after the code block) so readers know to use
host.docker.internal when running the same command with Docker.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Include the requested line in the snippet span.

take is computed as end - start + context_lines, so a valid single-line request returns zero requested lines. With context_lines == 0 that produces an empty snippet, and reversed ranges can also wrap on subtraction instead of failing fast.

Suggested fix

         let context_lines = self.context_lines;
+        if end_position.line < start_position.line
+            || (end_position.line == start_position.line
+                && end_position.character < start_position.character)
+        {
+            return Err(Status::invalid_argument("end position precedes start position"));
+        }
         let skip_lines = (start_position.line as usize).saturating_sub(context_lines);
-        let take = (end_position.line - start_position.line) as usize + context_lines;
+        let requested_lines = (end_position.line - start_position.line) as usize + 1;
+        let take = requested_lines + context_lines;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/provider/code_snip.rs` around lines 63 - 65, The snippet width
calculation currently omits the requested end line and can underflow for
reversed ranges; update the calculation so take is computed safely as the
inclusive line count plus trailing context: set take =
end_position.line.saturating_sub(start_position.line).saturating_add(1).saturating_add(context_lines)
(casting to usize as needed) and keep skip_lines =
start_position.line.saturating_sub(context_lines) to avoid wrapping for reversed
ranges and ensure single-line requests return one line.

[coderabbitai]

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 @.github/workflows/demo-testing.yml:
- Line 122: The MOUNT_OPT override currently sets MOUNT_OPT=":z" which conflicts
with other paths that expect ":U,z" (and the Makefile default), causing
permission/ownership drift; update the workflow to set MOUNT_OPT to ":U,z" (or
remove the override so the Makefile default applies) so the podman path and
make-driven test path use the same mount options and avoid rootless
write/ownership issues — update the MOUNT_OPT value referenced in the workflow
(symbol MOUNT_OPT) accordingly.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6d9e798f-b5b8-406e-9b87-2ece26de2252

📥 Commits

Reviewing files that changed from the base of the PR and between 97b59ee and 25df6b6.

📒 Files selected for processing (1)

• .github/workflows/demo-testing.yml

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "1) Workflow override in run-analyzer-tests:"
rg -n -C2 'MOUNT_OPT' .github/workflows/demo-testing.yml

echo
echo "2) Direct podman mounts in provider test step:"
rg -n -C1 ':U,z' .github/workflows/demo-testing.yml

echo
echo "3) Makefile default + demo run target usage:"
rg -n -C2 'MOUNT_OPT \?=|run-demo-c-sharp-pod:|\$\(MOUNT_OPT\)' Makefile

Repository: konveyor/c-sharp-analyzer-provider

Length of output: 2576

---

Use :U,z (or default) for MOUNT_OPT to avoid permission drift.

Line 122 overrides MOUNT_OPT to :z, but this same workflow's direct podman path (lines 70-77) uses :U,z, and the Makefile default is also :U,z. This mismatch can reintroduce rootless write/ownership issues in the make-driven test path when make run-external-providers-pod executes with the environment variable override.

Proposed fix

-          MOUNT_OPT: ":z"
+          MOUNT_OPT: ":U,z"

(Alternative: remove the override entirely and rely on Makefile default.)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	MOUNT_OPT: ":z"
	MOUNT_OPT: ":U,z"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/demo-testing.yml at line 122, The MOUNT_OPT override
currently sets MOUNT_OPT=":z" which conflicts with other paths that expect
":U,z" (and the Makefile default), causing permission/ownership drift; update
the workflow to set MOUNT_OPT to ":U,z" (or remove the override so the Makefile
default applies) so the podman path and make-driven test path use the same mount
options and avoid rootless write/ownership issues — update the MOUNT_OPT value
referenced in the workflow (symbol MOUNT_OPT) accordingly.

[mguetta1]

Can you add a comment/doc line for that the ilspycmd requires .NET 8.0 runtime?

[mguetta1]

@shawn-hurley Hi,
Is the goal here to enable full mode analysis? because 1) the init request is still skipped for full mode (here)
And 2) I removed the condition and tested it in Konveyor, nerd-dinner full analysis failed with error: "timed out starting providers after 8m0s"