fix: review pass — security, correctness, performance, and aesthetic fixes#22
Open
Clawsharp-Admin wants to merge 14 commits intomainfrom
Open
fix: review pass — security, correctness, performance, and aesthetic fixes#22Clawsharp-Admin wants to merge 14 commits intomainfrom
Clawsharp-Admin wants to merge 14 commits intomainfrom
Conversation
30 independent code review agents examined 499 commits, 761 source files, and 303 test files across 6 versions. Overall score: 8.1/10. 10 blocking findings, 52 should-fix, 62 suggestions. Critical: plugin verification bypassed, MCP schemas not forwarded, SQLite FTS broken for ACL users, SQL injection in vector search. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6-agent parallel review applying C# Aesthetic Architecture checklist to all 499 commits across v2.0-v2.5. Overall 8.3/10. Fixes (24 findings): - v2.0: OidcService JWT dedup, PolicySimulator decomposition, PolicyEvaluator ContainsOrdinal helper, "user" magic string - v2.1: Remove no-op RegexOptions.Compiled, dead gauge method - v2.2: McpExecutionContext immutable, dead MCP DTOs removed, IsOriginDenied dead branch removed - v2.3: JsonDocument leak cached, BuildDataSummary dedup, WebhookFormatterRegistry shared, 27 magic status strings extracted to DeliveryStatuses/DeliveryOutcomes constants, dead NotifyCircuitOpenedAsync removed, channel AttemptCount fix - v2.4: JSON contexts sealed, chunking helpers consolidated, ToAsyncEnumerable dedup, "auto" default → "recursive", dead LoadPlugins sync wrapper removed, private key scrubbed from WellKnownKeys comment, .key files gitignored - v2.5: DelegateAsync → (Text, IsError) tuple return fixing broken outcome classification, metadata key constants, unrecognized auth type warning Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Performance scan (3 agents, 518 files, 0 critical): - Redis IBatch pipelining: 60→1 round-trips per search, 500→1 per ingestion, FT.SEARCH for delete/count ops - Utf8JsonContent helper: eliminates double UTF-8 encoding across 23 HTTP call sites in 16 channel/tool files - ToolRegistry.GetDefinitions() cached (invalidate on register) - TagStripFilter zero-allocation tag matching via StringBuilder indexer + Equals(ReadOnlySpan<char>) instead of ToString() Re-applied fixes lost during git-filter-repo history rewrite: - MCP tool schema: AIFunction subclass forwards real ParametersSchemaJson - SQL injection: LINQ post-filtering replaces string-interpolated department IDs in SQLite FTS/vector and MsSql vector search - MsSqlMemory.ClearAsync: TRUNCATE → DELETE for FTS consistency - SqliteMemory.ClearAsync: wrapped in transaction - Canonical manifest: ManifestData properties reordered to match verifier - SQLite UpsertChunks: batched FTS delete/insert/update (3N → 3 ops) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review artifacts and AI agent files are local development outputs that should not be committed to the repository. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace per-chunk UPDATE loop (N round-trips) with a single UPDATE...CASE statement for all embeddings in one SQL call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Standardize search result count parameter to `top_k` across MemorySearchTool, WebSearchTool, KnowledgeSearchTool - Replace `[shell]` error prefix with `Error:` in ShellTool (4 guard/approval paths now match timeout/empty-command paths) - Fix DocumentReadTool leaking absolute path in not-found error (use caller-provided inputPath instead of resolved path) - Add explicit error for unknown InteractionsTool query instead of silently returning summary - Type bulk replay response with BulkReplayResponse DTO registered in WebhookJsonContext (removes AOT-unsafe anonymous object) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Plugin fault-tolerance: use PluginLoader.RegisterPluginServices()
with try-catch-log-continue instead of bare ConfigureServices loop.
Replace NullLogger with real console logger for plugin discovery.
- Knowledge config init→set: ChunkingConfig, EmbeddingBatchConfig,
RetrievalConfig properties changed to { get; set; } to preserve
defaults when STJ deserializes empty objects (prevents ChunkSize=0)
- A2aConfig record→class: all 7 A2A config types changed from
sealed record to sealed class for consistency with every other
config POCO in the project
- AuthorizationBehavior: remove unused ILogger parameter (CS9113)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace 18 raw string channel names in GatewayHost with ChannelName.X.Value (16 in RegisterChannels + 2 TryGetValue calls) - Replace 5 raw channel name strings in Discord, Slack, and KnowledgeIngestionWorker with ChannelName.X.Value - Remove 6 redundant null-forgiving operators where flow analysis proves non-null (_cts, _http, rule.ExpiresAt) - Replace Endpoints! null-forgiving with Endpoints?.TryGetValue == true Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add .ConfigureAwait(false) to all ~916 await expressions that were missing it across 127 source files. Coverage goes from ~35% (497/1406) to ~100% (1413/1413). Subsystems covered: Core pipeline, providers, channels (18), webhooks, A2A, MCP server, knowledge pipeline, memory backends, tools, CLI, cost tracking, features, organization, security, auth, analytics, cron. Migration files (EF Core scaffolding) excluded. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move 25 test files from flat tests/ root into structured Unit/ subdirectories (Channels/, Config/, Core/, Pipeline/, Providers/, Security/) matching the source project layout. Add TestLoggers.cs helper for test infrastructure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address findings from 30-agent review pass: hardened A2A task store with file locking and corruption recovery, added LazyAsyncInit for thread-safe memory backend initialization, strengthened security guards (SSRF, shell, path, web pairing), improved cost tracker thread safety, fixed channel edge cases (Lark, WeChat, Matrix, IRC), tightened config validation, added FilePermissions utility for safe file creation, removed unused handlers (CompactSession, ExecuteToolCall), and updated 60+ test files for new signatures and expanded coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ClawsharpSignTests shells out to clawsharp-sign via `dotnet run --no-build`, but the test project had no reference to it, so it was never built on CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The project-wide ConfigureAwait(false) enforcement pass incorrectly inserted .ConfigureAwait(false) inside raw string literals — SQLite RAISE() trigger bodies and Playwright JavaScript evaluation strings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dotnet run --project still evaluates the project file on CI, hitting the .NET 10 glob expansion bug on GitHub Actions runners. Switch to dotnet exec <dll> which invokes the pre-built assembly directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.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.
Summary
Comprehensive review and fix pass across v2.0–v2.5, combining findings from 30-agent code review, 6-agent aesthetic architecture review, and 3-agent performance scan.
Security Fixes
requireSigned: false)Secretfield +MaskKey()for legacy mode, bearer tokens never appear in logs/spansCorrectness Fixes
(Text, IsError)tuple returnPerformance Fixes
Aesthetic Fixes (24 findings across v2.0–v2.5)
Test plan
🤖 Generated with Claude Code