Repository navigation
Add .NET intelligence, verification, MCP hosting, and native binaries - #37
Conversation
|
Important Review skippedToo many files! This PR contains 131 files, which is 31 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configuration
📒 Files selected for processing: 131
You can disable this status message by setting the
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. Comment |
Telli
left a comment
Telli
left a comment
There was a problem hiding this comment.
Reviewed by reading the code; nothing was built or run. CI is red on build-and-test (ubuntu-latest): one existing MCP stdio fixture test fails, and the other OS jobs were cancelled. There's a likely cause inline on SharpClaw.Code.Mcp.Server.csproj.
Highest priority:
- HTTP MCP bind address can be overridden by a workspace
appsettings.json/Kestrel__*env vars, bypassingValidateHttpOptions. IsTestProject=falseis ignored. Verification of this repo's own solution would fail onSharpClaw.Testing.Xunit.- All analyzer references are stripped, including SDK-shipped source generators. This gives spurious
csharp_diagnosticserrors on STJ contexts and[GeneratedRegex], and the rename safety gate can pass renames that break the real build. - The semantic cache holds a global gate across an interactive approval prompt.
Everything else inline is medium/low: lock timeout, the 120 s automatic-verification timeout, TFM duplicates, log classification, diagnostics cache overwrite, restore detection, the release package gate, and HTTP host/TLS handling.
Things that looked solid: the MCP tool allowlist is enforced on tools/call, calls route through the permission engine non-interactively, token comparison is constant-time, stdio stdout stays clean, process execution uses ArgumentList and kills the process tree, rename boundary checks and rollback hold up, and all new contracts are registered in ProtocolJsonContext.
Generated by Claude Code
|
Addressed and resolved all 15 review threads in f597334. Local validation: warning-free Release solution/example build; 414 passing tests (280 unit, 114 integration, 20 parity); 41.28% coverage against the 38% floor; all agent scenarios/gates; DocFX with warnings as errors; exact prepacked package installability through an isolated clean SDK consumer and CLI tool. The original failing stdio fixture test now passes. Required GitHub checks on this exact head are being watched before merge; no policy bypass is being used. |
|
The first review-fix CI run passed Linux/macOS and all package/docs/extension jobs, but Windows exposed pooled SQLite handles retained after host disposal. Fixed in 08b0cda: knowledge and usage stores clear only their owned pools when the host shuts down; failed initialization disposes its connection; a new regression checks exclusive database access and workspace deletion after disposal. The cancellation parity test now surfaces startup failures, cancels/drains its task, and uses the existing longer Windows startup allowance. Local focused lifecycle/HTTP/verification tests (23) and the full parity harness (20) pass. Fresh required checks are running on 08b0cda. |
Summary
Add compiler-backed .NET intelligence and permission-gated verification to the existing coding-agent runtime:
verify,/verify, andverify_workspace. Restore is explicit; automatic verification and bounded repair are opt-in. Failed verification produces truthful completion and exit status, while prompt assembly consumes approved cached diagnostics without running builds.Validation
dotnet build SharpClawCode.sln --configuration Release --warnaserror— zero warnings/errors, including examples.dotnet test SharpClawCode.sln --configuration Release— 415 passed: 280 unit, 115 integration, 20 parity.Risk and compatibility
SDK-dependent semantic/build/test features still require a compatible installed .NET SDK and restored project assets. Query compilations exclude project-supplied analyzers/source generators. Automatic verification initially covers recorded supported file mutations.
Only macOS ARM64 native execution was completed locally. Windows x64/ARM64, Linux x64/ARM64, and macOS x64 acceptance remain gated on their matching release CI runners; cross-publishing is not counted as execution. This PR does not publish a release.
Review follow-up
All 15 review threads were addressed in f597334 with regression coverage: fixture shared-runtime startup, HTTP listener isolation and TLS proxy hosts, evaluated test/restore properties, SDK generator support and rename safety, approval coordination and load cleanup, verification lock/timeouts and TFM deduplication, diagnostic classification/cache retention, and install tests against the exact release package source.
The framework-dependent CLI and aggregate SDK package require the ASP.NET Core 10 shared runtime, included with the .NET 10 SDK. Self-contained binaries include it. Remote HTTP access now uses a local TLS reverse proxy; non-loopback plaintext listeners are rejected. Rename is disabled with an explicit unverified result when custom generators are excluded.
Windows CI follow-up: 08b0cda releases host-owned SQLite pools during disposal and adds a database lifecycle regression; 3e4d107 synchronizes cancellation parity through typed turn-start events without racing session file writes. All 415 local tests pass, and the final observer regression passes separately.