Conversation
There was a problem hiding this comment.
Pull request overview
Addresses remaining runtime review follow-ups by tightening caching behavior, aligning server serialization with protocol JSON settings, and improving tool-loop failure signaling, with expanded unit/integration coverage to prevent regressions.
Changes:
- Add an in-process conversation history cache and wire it into prompt assembly + turn completion.
- Bound workspace diagnostics caching via stale + overflow eviction, and add coverage ensuring the cache remains capped.
- Align embedded HTTP server JSON serialization with
ProtocolJsonContextand surface explicit tool-loop exhaustion summaries, with new tests/docs.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SharpClaw.Code.UnitTests/Runtime/WorkspaceDiagnosticsServiceTests.cs | Adds regression coverage ensuring diagnostics cache size is bounded and evicts oldest entries. |
| tests/SharpClaw.Code.UnitTests/Runtime/ShareAndCompactionServicesTests.cs | Extends share/compaction coverage around session metadata redaction and snapshot retrieval. |
| tests/SharpClaw.Code.UnitTests/Runtime/AgentCatalogServiceTests.cs | Adds coverage for immediate-parent preservation across multi-level configured agents. |
| tests/SharpClaw.Code.IntegrationTests/Runtime/PromptContextAssemblyTests.cs | Adds integration coverage for reusing in-process conversation history cache vs. rereading event logs. |
| tests/SharpClaw.Code.IntegrationTests/Runtime/AgentToolPolicyIntegrationTests.cs | Adds integration coverage for plugin tool allowlists, provider-requested tool denial, and tool-loop exhaustion surfacing. |
| src/SharpClaw.Code.Runtime/Server/WorkspaceHttpServer.cs | Switches server JSON options to be protocol-aligned with fallback type resolution for non-protocol payloads. |
| src/SharpClaw.Code.Runtime/Orchestration/ConversationRuntime.cs | Stores completed turns into the conversation history cache. |
| src/SharpClaw.Code.Runtime/Diagnostics/WorkspaceDiagnosticsService.cs | Adds bounded eviction (stale + overflow by oldest GeneratedAtUtc) to diagnostics cache. |
| src/SharpClaw.Code.Runtime/Context/PromptContextAssembler.cs | Uses cache-first conversation history assembly with event-log fallback. |
| src/SharpClaw.Code.Runtime/Context/ConversationHistoryCache.cs | Introduces an in-process per-session conversation history cache with bounded capacity. |
| src/SharpClaw.Code.Agents/Internal/ProviderBackedAgentKernel.cs | Adds explicit “tool-loop exhausted” summary text when max tool iterations are hit. |
| docs/runtime.md | Updates runtime documentation to reflect conversation history assembly and tool-loop behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NOTE: This reads the full event log per turn, which scales linearly with session length. | ||
| // For long-running sessions, consider persisting a compacted message history in session metadata | ||
| // to avoid re-reading and re-assembling the full log on every prompt. |
There was a problem hiding this comment.
The comment above this block still claims the assembler reads the full event log per turn, but the new ConversationHistoryCache path avoids that on cache hits. Update the comment to reflect the new behavior (cache-first, event-log fallback) so it remains accurate and doesn’t mislead future maintenance/perf work.
| // NOTE: This reads the full event log per turn, which scales linearly with session length. | |
| // For long-running sessions, consider persisting a compacted message history in session metadata | |
| // to avoid re-reading and re-assembling the full log on every prompt. | |
| // Prefer the cached history for the previous turn when available; on a cache miss, | |
| // fall back to reading the full event log and re-assembling the history for caching. | |
| // The fallback path still scales linearly with session length for long-running sessions. |
| var overflowKeys = Cache.Keys | ||
| .OrderBy(static key => key, StringComparer.Ordinal) | ||
| .Take(Cache.Count - MaxCacheEntries) | ||
| .ToArray(); |
There was a problem hiding this comment.
EvictOverflow() evicts entries by lexicographic key order, which is unrelated to insertion time or recency and can even immediately evict the just-stored entry if its key sorts early. Consider tracking insertion/last-access (e.g., timestamp in CacheEntry or a separate queue) and evicting oldest/least-recently-used sessions instead.
|
|
||
| if (pluginManager is not null) | ||
| { | ||
| services.AddSingleton(pluginManager); |
There was a problem hiding this comment.
The plugin manager is registered twice here (services.AddSingleton(pluginManager) already registers it as IPluginManager due to type inference, and then it’s added again as IPluginManager). This can lead to duplicate registrations (especially if any code resolves IEnumerable) and is redundant—register the instance once (or replace the existing registration from AddSharpClawRuntime).
| services.AddSingleton(pluginManager); |
Summary
Verification
AgentCatalogServiceTests|FullyQualifiedNameShareAndCompactionServicesTests|FullyQualifiedNameWorkspaceDiagnosticsServiceTests|FullyQualifiedNameToolCallDispatcherTests|FullyQualifiedName~ContextWindowManagerTests"AgentToolPolicyIntegrationTests|FullyQualifiedNamePromptContextAssemblyTests"