Potential fixes for 2 code quality findings

[Jordonbc]

This PR applies 2/2 suggestions from code quality AI findings.

[github-actions]

Code Review Summary

Changes Made (2 fixes)

1. Improved error message for request ID overflow (lines 78-86)

• Before: "rpc request id overflow".to_string()
• After: Includes plugin_id and request_id for better debugging

This is a solid improvement - provides context for debugging.

2. JSON parse error handling in vcs_open (lines 541-550)

• Before: unwrap_or_else(|_| ...) silently ignored errors
• After: Logs a warning with the actual error message

This is a good improvement - makes debugging config issues easier.

Minor Style Inconsistency

Line 547 uses log::warn!(...) with the log:: prefix, while line 506 uses just warn!(...). Since warn is already imported at line 23, prefer removing the prefix for consistency:

// Current (line 547):
log::warn!("Failed to parse...");

// Better (matches line 506):
warn!("Failed to parse...");

Potential Issues Found

1. Double process cleanup (lines 936-944 vs 912-933)

Both Drop implementation and stop() method kill the child process. If stop() is called explicitly, the Drop will attempt to kill an already-killed process. This works (the kill() returns Err for already-dead processes and is ignored), but is slightly redundant.

2. Reader thread may block on recv_timeout (lines 101-124)

When notifications arrive, the timeout accumulates. If many notifications arrive with delays, the RPC could timeout even though it's making progress. This is a pre-existing design issue, not introduced by this PR.

Verdict

The PR addresses the two code quality findings appropriately. The changes are minimal, focused, and improve error diagnostics. No bugs introduced. The style inconsistency with log::warn! is minor and could be fixed.

Overall: Approve with optional style fix suggestion.

opencode session  |  github run