[codex] Warp-inspired runtime extensions#11
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds external-agent execution, skill-pack management, work-item import/export, and a workbench status command. It also introduces new protocol models/events, runtime and CLI wiring, project references, docs, and tests. ChangesFeature Surface Expansion
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run 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 |
There was a problem hiding this comment.
Pull request overview
Adds Warp-inspired runtime extensions: a new ExternalAgents module (Claude/OpenCode/Gemini/Codex CLI adapters), a session-aware skill-pack ecosystem with built-in packs and install/enable/disable/run flows, work-item import/export (GitHub + generic JSON) with session metadata persistence and Markdown/JSON summaries, and a static workbench/status surface. Protocol models, events, AOT serialization context, runtime DI, CLI/REPL commands, and docs are updated to expose these surfaces.
Changes:
- New
SharpClaw.Code.ExternalAgentsandSharpClaw.Code.WorkItemsprojects with adapters, registries, services, permission-aware execution, and runtime events. - Skill packs (
ISkillPackRegistry, manifest, install/enable/disable/run) layered alongside the legacy skill registry;SkillsCommandHandlerextended with new subcommands. WorkbenchStatusServiceplusworkbench/work/external/agent-status/checkpointsCLI+slash handlers, runtime DI, diagnostics check, config schema, and docs.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SharpClaw.Code.ExternalAgents/** | New module: adapter base, four built-in adapters, registry, permission-gated service, executable resolver, default config provider, DI |
| src/SharpClaw.Code.WorkItems/** | New module: GitHub + generic providers, registry, session-aware import/export service, DI |
| src/SharpClaw.Code.Skills/Services/SkillPackRegistry.cs + Abstractions + DI | Built-in packs, file-backed install/enable/disable, prompt template expansion |
| src/SharpClaw.Code.Protocol/** | New event types, models (ExternalAgent/SkillPack/WorkItem/WorkbenchStatus), AOT serializer entries, polymorphic event mapping |
| src/SharpClaw.Code.Runtime/** | Workbench status service + abstraction, external-agent config provider, health diagnostic check, config merge of new sections, DI wiring |
| src/SharpClaw.Code.Commands/Handlers/** | New External, Work, Workbench, CheckpointsSlash, AgentStatusSlash handlers; Skills extended with pack commands; Agents extended with external subcommand |
| src/SharpClaw.Code.Cli/Composition/CliServiceCollectionExtensions.cs | Registers new handlers and shares AgentsCommandHandler instance across both CLI and slash command surfaces |
| SharpClawCode.sln + new .csproj files | New project references and solution entries |
| tests/SharpClaw.Code.UnitTests/** | Adds tests for GitHubWorkItemProvider URL parsing, SkillPackRegistry listing/installing, and the Codex external-agent adapter command shaping |
| docs/{external-agents,skills,work-items,workbench}.md + README.md | New documentation for the user-facing features |
Comments suppressed due to low confidence (2)
src/SharpClaw.Code.Skills/Services/SkillPackRegistry.cs:122
EnableAsync/DisableAsynclook up the pack directory usingskillIdas a literal directory name, butResolveAsync/BuildPromptAsyncallow the user to refer to a pack by eitherManifest.IdorManifest.Name(case-insensitive). As a result, a user canskills runa pack by name but thenskills enable --id <Name>will return "not found" and a no-op. Consider resolving the pack viaResolveAsyncfirst and acting on itsSourcedirectory.
src/SharpClaw.Code.Commands/Handlers/WorkCommandHandler.cs:94ExportSummaryAsyncrequiresrequest.Provider(it is later persisted intoWorkItemSummaryExportedEvent), butWorkCommandHandler.ExecuteExportSummaryAsyncalways hardcodes"github"for the provider regardless of which provider actually imported the work item. The exported event will therefore mis-attribute generic/Jira summaries as GitHub. Pull the provider from the session-storedWorkItem.Provider(already available viaReadWorkItem) instead of from the request.
private async Task<int> ExecuteExportSummaryAsync(string format, CommandExecutionContext context, CancellationToken cancellationToken)
{
var result = await workItemService
.ExportSummaryAsync(new WorkItemExportRequest("github", context.SessionId, null, format), context.WorkingDirectory, cancellationToken)
.ConfigureAwait(false);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+88
to
+100
| private async Task<(ConversationSession Session, bool Created)> ResolveSessionAsync(string workspace, string? sessionId, WorkItem workItem, CancellationToken cancellationToken) | ||
| { | ||
| ConversationSession? session = null; | ||
| if (!string.IsNullOrWhiteSpace(sessionId)) | ||
| { | ||
| session = await sessionStore.GetByIdAsync(workspace, sessionId, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| session ??= await sessionStore.GetLatestAsync(workspace, cancellationToken).ConfigureAwait(false); | ||
| if (session is not null) | ||
| { | ||
| return (session, false); | ||
| } |
Comment on lines
+29
to
+35
| using var client = new HttpClient(); | ||
| client.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue("SharpClawCode", "1.0")); | ||
| var token = Environment.GetEnvironmentVariable("GITHUB_TOKEN"); | ||
| if (!string.IsNullOrWhiteSpace(token)) | ||
| { | ||
| client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", token); | ||
| } |
| ResolveMetadata(session, SharpClawWorkflowMetadataKeys.ActiveAgentId), | ||
| status.RuntimeState, | ||
| status.ApprovalSettings, | ||
| 0, |
Comment on lines
+78
to
+95
| var targetDirectory = pathService.Combine(GetWorkspaceRoot(workspaceRoot, ensureExists: true), manifest.Id); | ||
| fileSystem.CreateDirectory(targetDirectory); | ||
| await fileSystem.WriteAllTextAsync( | ||
| pathService.Combine(targetDirectory, ManifestFileName), | ||
| JsonSerializer.Serialize(manifest, ProtocolJsonContext.Default.SkillPackManifest), | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
||
| if (request.EnableAfterInstall) | ||
| { | ||
| fileSystem.TryDeleteFile(pathService.Combine(targetDirectory, DisabledFileName)); | ||
| } | ||
| else | ||
| { | ||
| await fileSystem.WriteAllTextAsync(pathService.Combine(targetDirectory, DisabledFileName), systemClock.UtcNow.ToString("O"), cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| return new SkillPack(manifest, targetDirectory, false, request.EnableAfterInstall, systemClock.UtcNow); | ||
| } |
Comment on lines
+115
to
+149
| private async Task<ConversationSession> ResolveSessionAsync(string workspace, ExternalAgentRunRequest request, CancellationToken cancellationToken) | ||
| { | ||
| ConversationSession? session = null; | ||
| if (!string.IsNullOrWhiteSpace(request.SessionId)) | ||
| { | ||
| session = await sessionStore.GetByIdAsync(workspace, request.SessionId, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| session ??= await sessionStore.GetLatestAsync(workspace, cancellationToken).ConfigureAwait(false); | ||
| if (session is not null) | ||
| { | ||
| return session; | ||
| } | ||
|
|
||
| var created = new ConversationSession( | ||
| CreateIdentifier("session"), | ||
| "External agent session", | ||
| SessionLifecycleState.Active, | ||
| request.PermissionMode, | ||
| OutputFormat.Text, | ||
| workspace, | ||
| workspace, | ||
| systemClock.UtcNow, | ||
| systemClock.UtcNow, | ||
| null, | ||
| null, | ||
| new Dictionary<string, string>()); | ||
| await sessionStore.SaveAsync(workspace, created, cancellationToken).ConfigureAwait(false); | ||
| await PublishAsync( | ||
| workspace, | ||
| created.Id, | ||
| new SessionCreatedEvent(CreateIdentifier("event"), created.Id, null, systemClock.UtcNow, created), | ||
| cancellationToken).ConfigureAwait(false); | ||
| return created; | ||
| } |
| public string Provider => "generic"; | ||
|
|
||
| /// <inheritdoc /> | ||
| public bool CanImport(string idOrUrl) => idOrUrl.EndsWith(".json", StringComparison.OrdinalIgnoreCase) || idOrUrl.TrimStart().StartsWith('{'); |
Comment on lines
+56
to
+70
| if (command.Arguments.Length >= 2 && string.Equals(command.Arguments[0], "import", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return ExecuteImportAsync("github", command.Arguments[1], context, cancellationToken); | ||
| } | ||
|
|
||
| if (command.Arguments.Length == 0 || string.Equals(command.Arguments[0], "show", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return ExecuteExportSummaryAsync("markdown", context, cancellationToken); | ||
| } | ||
|
|
||
| if (string.Equals(command.Arguments[0], "export-summary", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return ExecuteExportSummaryAsync("markdown", context, cancellationToken); | ||
| } | ||
|
|
|
|
||
| /// <inheritdoc /> | ||
| public Task<int> ExecuteAsync(SlashCommandParseResult command, CommandExecutionContext context, CancellationToken cancellationToken) | ||
| => agentsCommandHandler.ExecuteAsync(new SlashCommandParseResult(true, "agents", ["external", "status"]), context, cancellationToken); |
Comment on lines
+29
to
+43
| var manifest = new SkillPackManifest( | ||
| "custom-review", | ||
| "Custom Review", | ||
| "1.0.0", | ||
| "Custom review pack", | ||
| "tests", | ||
| ["review"], | ||
| null, | ||
| null, | ||
| null, | ||
| null, | ||
| null, | ||
| null, | ||
| "Review {{arguments}} in {{workspace}}."); | ||
| await File.WriteAllTextAsync(manifestPath, JsonSerializer.Serialize(manifest, ProtocolJsonContext.Default.SkillPackManifest)); |
Comment on lines
+179
to
+186
| private static void Validate(SkillPackManifest manifest) | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(manifest.Id); | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(manifest.Name); | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(manifest.Version); | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(manifest.Description); | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(manifest.EntryPointPrompt); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
src/SharpClaw.Code.WorkItems/Providers/GitHubWorkItemProvider.cs (1)
29-35: 🏗️ Heavy liftUse
IHttpClientFactoryfor HttpClient management
new HttpClient()on each call increases connection churn and can hurt reliability under repeated imports. The codebase already demonstrates the proper pattern usingIHttpClientFactoryin TelemetryServiceCollectionExtensions.cs—configure headers and timeouts centrally via dependency injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SharpClaw.Code.WorkItems/Providers/GitHubWorkItemProvider.cs` around lines 29 - 35, Replace the direct new HttpClient usage in GitHubWorkItemProvider (where the code creates a client, sets UserAgent and Authorization from GITHUB_TOKEN) with an injected IHttpClientFactory: add an IHttpClientFactory dependency to the GitHubWorkItemProvider constructor and call _httpClientFactory.CreateClient("GitHub") (or a suitable named/typed client) instead of new HttpClient(); move default header and timeout configuration into your DI setup (following the pattern in TelemetryServiceCollectionExtensions.cs) so UserAgent, Authorization (if token present) and timeouts are applied centrally, and remove per-call client creation to avoid connection churn.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/work-items.md`:
- Line 20: Replace the internal reference GenericWorkItemFixture with a concrete
JSON example in the docs: update the sentence that currently reads "The JSON
shape matches `GenericWorkItemFixture`..." to include the provided JSON payload
example (provider, id, title, description, url, status, labels, assignee,
metadata) so CLI users can copy/import directly; ensure the example is fenced as
JSON and matches the suggested keys/values exactly.
In `@src/SharpClaw.Code.Commands/Handlers/AgentsCommandHandler.cs`:
- Around line 208-211: ParseMode currently coerces any unknown string to
ExternalAgentMode.WorkspaceWrite; change it to validate explicitly and fail
fast: map "readOnly" and "read" (case-insensitive) to
ExternalAgentMode.ReadOnly, map "write" and "workspaceWrite" (case-insensitive)
to ExternalAgentMode.WorkspaceWrite, and for any other null/unknown value throw
an ArgumentException (or ArgumentOutOfRangeException) with a clear message;
update the ParseMode method to perform these checks and throw instead of
returning a default.
In `@src/SharpClaw.Code.Commands/Handlers/ExternalCommandHandler.cs`:
- Around line 107-110: The ParseMode method currently treats any unrecognized
string as ExternalAgentMode.WorkspaceWrite, which risks accidental mutating
behavior; update ParseMode to explicitly accept only known values ("readOnly" /
"read" -> ExternalAgentMode.ReadOnly, "writeOnly" / "write" / "workspaceWrite"
-> ExternalAgentMode.WorkspaceWrite) and throw an informative exception (e.g.,
ArgumentException) or return a Result/nullable to surface invalid '--mode'
inputs so callers fail fast instead of defaulting to write mode; reference the
ParseMode function and ExternalAgentMode enum when implementing this validation
and ensure the error message mentions the invalid value and expected modes.
In `@src/SharpClaw.Code.Commands/Handlers/WorkCommandHandler.cs`:
- Around line 56-59: The code in WorkCommandHandler calls ExecuteImportAsync and
ExecuteExportAsync with a hardcoded "github" provider, breaking non-GitHub
sessions; change those calls to use the current session/provider instead of the
literal string. Replace the "github" argument with the runtime provider (e.g.
context.Session?.Provider or the provider parsed from the command) when calling
ExecuteImportAsync and ExecuteExportAsync so import/export use the active
provider (keep a sensible fallback only if absolutely needed). Ensure you update
all occurrences (the ExecuteImportAsync and ExecuteExportAsync calls referenced
in this diff) to reference that provider variable.
In `@src/SharpClaw.Code.ExternalAgents/Adapters/ExternalAgentAdapterBase.cs`:
- Around line 60-64: The error message uses Descriptor.ExecutableName which can
be incorrect when ExternalAgentAdapterConfig.ExecutablePath overrides the
default; modify the executable resolution block in ExternalAgentAdapterBase
(where executableResolver.Resolve(ResolveExecutable(adapterConfig)) is called)
to capture the resolved executable identifier (e.g., store
ResolveExecutable(adapterConfig) into a local like resolvedExecutable or
resolvedPath) and use that value in the Failure(...) message instead of
Descriptor.ExecutableName so the missing-executable error reports the actual
configured/resolved executable.
In `@src/SharpClaw.Code.ExternalAgents/Services/ExternalAgentRegistry.cs`:
- Around line 27-30: The loop over orderedAdapters calling
adapter.GetStatusAsync(...) can let one adapter exception fail the whole report;
change the foreach in ExternalAgentRegistry so each adapter call is protected
with try/catch: for each adapter in orderedAdapters, call await
adapter.GetStatusAsync(workspaceRoot, cancellationToken).ConfigureAwait(false)
inside a try block and on success Add the result to statuses, and in the catch
create and add a fallback/failed status object (including adapter identifier and
exception message/stack) so one adapter failure doesn't break the entire catalog
report; ensure you still observe cancellationToken and do not swallow exceptions
globally.
In `@src/SharpClaw.Code.ExternalAgents/Services/ExternalAgentService.cs`:
- Around line 89-113: Wrap the call to adapter.RunAsync inside a try/catch that
preserves cancellation (catch only Exception and rethrow if cancellation
requested), and on any non-cancellation exception construct an
ExternalAgentRunResult representing a failure (set FailureKind to a
runtime/exception kind, include the exception message/stack in OutputText and
ExternalSessionId as appropriate, and set an exit code) then call
PublishFailedAsync (or publish an ExternalAgentRunFailedEvent analogous to the
successful path) with the workspace, session.Id, request.AdapterId and the
constructed result, await SaveSessionMetadataAsync only on success, and finally
return the failure result; update ExternalAgentService where adapter.RunAsync is
invoked to ensure all exceptions produce a consistent ExternalAgentRunResult and
emitted failed event instead of letting the exception escape.
- Around line 118-124: The code currently falls back to GetLatestAsync when a
requested SessionId is provided but not found; change the logic in
ExternalAgentService so that if request.SessionId is non-empty you call
sessionStore.GetByIdAsync(workspace, request.SessionId, cancellationToken) and
if that returns null immediately throw a clear exception (e.g.
InvalidOperationException or a NotFoundException) indicating the specific
SessionId was not found, and only call sessionStore.GetLatestAsync when
request.SessionId is null/empty; update the code around the session variable and
the calls to GetByIdAsync/GetLatestAsync accordingly.
In
`@src/SharpClaw.Code.ExternalAgents/Services/PathExternalAgentExecutableResolver.cs`:
- Around line 31-34: The resolver in PathExternalAgentExecutableResolver appends
every extension from the extensions list to executableNameOrPath
unconditionally, which produces invalid candidates when the input already
contains an extension (e.g., "foo.exe" -> "foo.exe.EXE"); update the resolution
logic to first check if Path.HasExtension(executableNameOrPath) or
Path.GetExtension(executableNameOrPath) matches a Windows extension and, if so,
only test the given name as-is (construct candidate = Path.Combine(directory,
executableNameOrPath) and call File.Exists), otherwise iterate extensions and
append them as currently done; ensure you still use the existing variables
(extensions, executableNameOrPath, candidate, directory, File.Exists) and
preserve current directory iteration behavior.
In `@src/SharpClaw.Code.Runtime/Workflow/WorkbenchStatusService.cs`:
- Around line 38-40: The code in WorkbenchStatusService.cs currently reads the
entire session event stream via eventStore.ReadAllAsync and then calls
TakeLast(8) into the variable events, which loads full history; change this to
fetch only the last 8 events from the store instead of reading all. Update the
callsite using a store API that returns the tail (e.g.,
eventStore.ReadLastAsync(workspace, session.Id, 8, cancellationToken)) or, if
such an API is not available, replace the ReadAllAsync + TakeLast logic with a
streaming/iterator read that iterates the stream in reverse or reads pages and
stops once 8 events are collected (so the variable events still ends up as the
last 8 events but without materializing the entire stream). Ensure you replace
usages of eventStore.ReadAllAsync(...).TakeLast(8) and keep cancellationToken
handling intact.
In `@src/SharpClaw.Code.Skills/Services/SkillPackRegistry.cs`:
- Around line 146-153: ResolveFromDirectoryAsync currently calls
JsonSerializer.Deserialize which can throw JsonException and it returns a
SkillPack without validating the manifest, causing ListAsync enumeration to fail
on a single malformed manifest; update ResolveFromDirectoryAsync to catch
JsonException (and other deserialization exceptions), log or swallow the error
and return null so the directory scan can continue, and ensure you call
Validate(manifest) (same as ResolveAsync) before constructing/returning the
SkillPack instance to reject invalid manifests early; reference
ResolveFromDirectoryAsync, JsonSerializer.Deserialize, Validate(manifest),
ResolveAsync and SkillPack when making the changes.
- Around line 78-79: The code in SkillPackRegistry.cs uses manifest.Id and
skillId directly with IPathService.Combine/GetWorkspaceRoot and
fileSystem.CreateDirectory, allowing path traversal or absolute paths; modify
the code to validate and sanitize these IDs before any path composition:
add/extend Validate() to reject empty or whitespace IDs, any path separator
characters (Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), rooted
paths (Path.IsPathRooted), parent-segment sequences (".."), and any
Path.GetInvalidFileNameChars; alternatively normalize IDs by taking
Path.GetFileName(id) or using a dedicated SanitizeSkillPackId(string) helper and
call it everywhere manifest.Id or skillId are used (e.g., in GetWorkspaceRoot,
where paths are combined and in directory creation like
fileSystem.CreateDirectory(targetDirectory)) so that only safe, single-segment
directory names are allowed.
In `@src/SharpClaw.Code.WorkItems/Providers/GenericWorkItemProvider.cs`:
- Around line 27-28: The JSON deserialization in GenericWorkItemProvider
currently lets a JsonException bubble up instead of normalizing to the
provider's InvalidOperationException; wrap the call to
JsonSerializer.Deserialize(...) that assigns 'fixture' in
GenericWorkItemProvider in a try/catch that catches JsonException and rethrows a
new InvalidOperationException("Generic work-item fixture could not be parsed.",
innerException) so malformed JSON is reported consistently (preserve the
original exception as the inner exception).
In `@src/SharpClaw.Code.WorkItems/Services/WorkItemRegistry.cs`:
- Around line 13-15: Resolve currently falls back to CanImport even when a
non-empty provider argument was explicitly given, which can return an unintended
provider; update Resolve (method Resolve and use of orderedProviders) to first
attempt the case-insensitive provider match, and if provider is not null/empty
(use string.IsNullOrWhiteSpace or IsNullOrEmpty) and no match is found return
null immediately, otherwise proceed to the existing fallback that picks the
first orderedProviders item where CanImport(idOrUrl) is true.
In `@src/SharpClaw.Code.WorkItems/Services/WorkItemService.cs`:
- Around line 91-99: The current logic falls back to GetLatestAsync when a
requested sessionId is not found; change it so that when sessionId is provided
(non-empty) you call sessionStore.GetByIdAsync(workspace, sessionId, ...) and if
that returns null you fail immediately (throw an informative exception or return
an explicit error) instead of calling sessionStore.GetLatestAsync; only call
GetLatestAsync when sessionId is null/whitespace. Update the code around the
session variable and the calls to sessionStore.GetByIdAsync and GetLatestAsync
in the method (the block handling sessionId/session) to implement this behavior
and surface a clear error when the explicit session is missing.
---
Nitpick comments:
In `@src/SharpClaw.Code.WorkItems/Providers/GitHubWorkItemProvider.cs`:
- Around line 29-35: Replace the direct new HttpClient usage in
GitHubWorkItemProvider (where the code creates a client, sets UserAgent and
Authorization from GITHUB_TOKEN) with an injected IHttpClientFactory: add an
IHttpClientFactory dependency to the GitHubWorkItemProvider constructor and call
_httpClientFactory.CreateClient("GitHub") (or a suitable named/typed client)
instead of new HttpClient(); move default header and timeout configuration into
your DI setup (following the pattern in TelemetryServiceCollectionExtensions.cs)
so UserAgent, Authorization (if token present) and timeouts are applied
centrally, and remove per-call client creation to avoid connection churn.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c3a67092-172f-4c6a-8e67-40f657fe811b
📒 Files selected for processing (65)
README.mdSharpClawCode.slndocs/external-agents.mddocs/skills.mddocs/work-items.mddocs/workbench.mdsrc/SharpClaw.Code.Cli/Composition/CliServiceCollectionExtensions.cssrc/SharpClaw.Code.Commands/Handlers/AgentStatusSlashCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/AgentsCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/CheckpointsSlashCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/ExternalCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/SkillsCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/WorkCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/WorkbenchCommandHandler.cssrc/SharpClaw.Code.Commands/SharpClaw.Code.Commands.csprojsrc/SharpClaw.Code.ExternalAgents/Abstractions/IExternalAgentAdapter.cssrc/SharpClaw.Code.ExternalAgents/Abstractions/IExternalAgentConfigProvider.cssrc/SharpClaw.Code.ExternalAgents/Abstractions/IExternalAgentExecutableResolver.cssrc/SharpClaw.Code.ExternalAgents/Abstractions/IExternalAgentRegistry.cssrc/SharpClaw.Code.ExternalAgents/Abstractions/IExternalAgentService.cssrc/SharpClaw.Code.ExternalAgents/Adapters/BuiltInExternalAgentAdapters.cssrc/SharpClaw.Code.ExternalAgents/Adapters/ExternalAgentAdapterBase.cssrc/SharpClaw.Code.ExternalAgents/AssemblyMarker.cssrc/SharpClaw.Code.ExternalAgents/ExternalAgentsServiceCollectionExtensions.cssrc/SharpClaw.Code.ExternalAgents/Services/DefaultExternalAgentConfigProvider.cssrc/SharpClaw.Code.ExternalAgents/Services/ExternalAgentRegistry.cssrc/SharpClaw.Code.ExternalAgents/Services/ExternalAgentService.cssrc/SharpClaw.Code.ExternalAgents/Services/PathExternalAgentExecutableResolver.cssrc/SharpClaw.Code.ExternalAgents/SharpClaw.Code.ExternalAgents.csprojsrc/SharpClaw.Code.Protocol/Events/ExternalAgentEvents.cssrc/SharpClaw.Code.Protocol/Events/RuntimeEvent.cssrc/SharpClaw.Code.Protocol/Events/SkillAndWorkItemEvents.cssrc/SharpClaw.Code.Protocol/Models/ExternalAgentModels.cssrc/SharpClaw.Code.Protocol/Models/OpenCodeParityModels.cssrc/SharpClaw.Code.Protocol/Models/SharpClawWorkflowMetadataKeys.cssrc/SharpClaw.Code.Protocol/Models/SkillPackModels.cssrc/SharpClaw.Code.Protocol/Models/WorkItemModels.cssrc/SharpClaw.Code.Protocol/Operational/WorkbenchStatusReport.cssrc/SharpClaw.Code.Protocol/Serialization/ProtocolJsonContext.cssrc/SharpClaw.Code.Runtime/Abstractions/IWorkbenchStatusService.cssrc/SharpClaw.Code.Runtime/Composition/RuntimeServiceCollectionExtensions.cssrc/SharpClaw.Code.Runtime/Configuration/SharpClawConfigService.cssrc/SharpClaw.Code.Runtime/Diagnostics/Checks/ExternalAgentHealthCheck.cssrc/SharpClaw.Code.Runtime/Diagnostics/OperationalDiagnosticsCoordinator.cssrc/SharpClaw.Code.Runtime/Export/SessionExportService.cssrc/SharpClaw.Code.Runtime/ExternalAgents/RuntimeExternalAgentConfigProvider.cssrc/SharpClaw.Code.Runtime/SharpClaw.Code.Runtime.csprojsrc/SharpClaw.Code.Runtime/Workflow/WorkbenchStatusService.cssrc/SharpClaw.Code.Skills/Abstractions/ISkillPackRegistry.cssrc/SharpClaw.Code.Skills/Services/SkillPackRegistry.cssrc/SharpClaw.Code.Skills/SkillsServiceCollectionExtensions.cssrc/SharpClaw.Code.WorkItems/Abstractions/IWorkItemProvider.cssrc/SharpClaw.Code.WorkItems/Abstractions/IWorkItemRegistry.cssrc/SharpClaw.Code.WorkItems/Abstractions/IWorkItemService.cssrc/SharpClaw.Code.WorkItems/AssemblyMarker.cssrc/SharpClaw.Code.WorkItems/Providers/GenericWorkItemProvider.cssrc/SharpClaw.Code.WorkItems/Providers/GitHubWorkItemProvider.cssrc/SharpClaw.Code.WorkItems/Services/WorkItemRegistry.cssrc/SharpClaw.Code.WorkItems/Services/WorkItemService.cssrc/SharpClaw.Code.WorkItems/SharpClaw.Code.WorkItems.csprojsrc/SharpClaw.Code.WorkItems/WorkItemsServiceCollectionExtensions.cstests/SharpClaw.Code.UnitTests/ExternalAgents/ExternalAgentAdapterTests.cstests/SharpClaw.Code.UnitTests/SharpClaw.Code.UnitTests.csprojtests/SharpClaw.Code.UnitTests/Skills/SkillPackRegistryTests.cstests/SharpClaw.Code.UnitTests/WorkItems/GitHubWorkItemProviderTests.cs
Comment on lines
+146
to
+153
| var manifestText = await fileSystem.ReadAllTextIfExistsAsync(pathService.Combine(directory, ManifestFileName), cancellationToken).ConfigureAwait(false); | ||
| if (string.IsNullOrWhiteSpace(manifestText)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var manifest = JsonSerializer.Deserialize(manifestText, ProtocolJsonContext.Default.SkillPackManifest); | ||
| if (manifest is null) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all SkillPackManifest deserialization paths and current exception handling coverage.
rg -n -C3 "JsonSerializer\\.Deserialize\\(.*SkillPackManifest" src
rg -n -C3 "catch \\(JsonException\\)|Validate\\(manifest\\)" src/SharpClaw.Code.SkillsRepository: clawdotnet/SharpClawCode
Length of output: 2607
🏁 Script executed:
sed -n '145,175p' src/SharpClaw.Code.Skills/Services/SkillPackRegistry.csRepository: clawdotnet/SharpClawCode
Length of output: 1266
🏁 Script executed:
rg -n "ResolveFromDirectoryAsync" src/SharpClaw.Code.Skills/Services/SkillPackRegistry.cs | head -20Repository: clawdotnet/SharpClawCode
Length of output: 303
🏁 Script executed:
sed -n '40,60p' src/SharpClaw.Code.Skills/Services/SkillPackRegistry.csRepository: clawdotnet/SharpClawCode
Length of output: 874
🏁 Script executed:
sed -n '30,50p' src/SharpClaw.Code.Skills/Services/SkillPackRegistry.csRepository: clawdotnet/SharpClawCode
Length of output: 871
Add exception handling and validation to ResolveFromDirectoryAsync to prevent malformed manifests from breaking directory scans.
At line 152, JsonSerializer.Deserialize can throw JsonException uncaught. In ListAsync, this breaks enumeration of all remaining packs when a single manifest is malformed. Additionally, the method returns a SkillPack without calling Validate(manifest), unlike the ResolveAsync method which validates at line 76.
🛡️ Suggested fix
private async Task<SkillPack?> ResolveFromDirectoryAsync(string directory, CancellationToken cancellationToken)
{
var manifestText = await fileSystem.ReadAllTextIfExistsAsync(pathService.Combine(directory, ManifestFileName), cancellationToken).ConfigureAwait(false);
if (string.IsNullOrWhiteSpace(manifestText))
{
return null;
}
- var manifest = JsonSerializer.Deserialize(manifestText, ProtocolJsonContext.Default.SkillPackManifest);
- if (manifest is null)
+ SkillPackManifest? manifest;
+ try
{
- return null;
+ manifest = JsonSerializer.Deserialize(manifestText, ProtocolJsonContext.Default.SkillPackManifest);
}
+ catch (JsonException)
+ {
+ return null;
+ }
+ if (manifest is null)
+ {
+ return null;
+ }
+ try
+ {
+ Validate(manifest);
+ }
+ catch (ArgumentException)
+ {
+ return null;
+ }
+ catch (InvalidOperationException)
+ {
+ return null;
+ }
return new SkillPack(manifest, directory, false, !fileSystem.FileExists(pathService.Combine(directory, DisabledFileName)));
}📝 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
| var manifestText = await fileSystem.ReadAllTextIfExistsAsync(pathService.Combine(directory, ManifestFileName), cancellationToken).ConfigureAwait(false); | |
| if (string.IsNullOrWhiteSpace(manifestText)) | |
| { | |
| return null; | |
| } | |
| var manifest = JsonSerializer.Deserialize(manifestText, ProtocolJsonContext.Default.SkillPackManifest); | |
| if (manifest is null) | |
| var manifestText = await fileSystem.ReadAllTextIfExistsAsync(pathService.Combine(directory, ManifestFileName), cancellationToken).ConfigureAwait(false); | |
| if (string.IsNullOrWhiteSpace(manifestText)) | |
| { | |
| return null; | |
| } | |
| SkillPackManifest? manifest; | |
| try | |
| { | |
| manifest = JsonSerializer.Deserialize(manifestText, ProtocolJsonContext.Default.SkillPackManifest); | |
| } | |
| catch (JsonException) | |
| { | |
| return null; | |
| } | |
| if (manifest is null) | |
| { | |
| return null; | |
| } | |
| try | |
| { | |
| Validate(manifest); | |
| } | |
| catch (ArgumentException) | |
| { | |
| return null; | |
| } | |
| catch (InvalidOperationException) | |
| { | |
| return null; | |
| } | |
| return new SkillPack(manifest, directory, false, !fileSystem.FileExists(pathService.Combine(directory, DisabledFileName))); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/SharpClaw.Code.Skills/Services/SkillPackRegistry.cs` around lines 146 -
153, ResolveFromDirectoryAsync currently calls JsonSerializer.Deserialize which
can throw JsonException and it returns a SkillPack without validating the
manifest, causing ListAsync enumeration to fail on a single malformed manifest;
update ResolveFromDirectoryAsync to catch JsonException (and other
deserialization exceptions), log or swallow the error and return null so the
directory scan can continue, and ensure you call Validate(manifest) (same as
ResolveAsync) before constructing/returning the SkillPack instance to reject
invalid manifests early; reference ResolveFromDirectoryAsync,
JsonSerializer.Deserialize, Validate(manifest), ResolveAsync and SkillPack when
making the changes.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/SharpClaw.Code.Sessions/Storage/SqliteEventStore.cs (1)
70-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared deserialization logic to reduce duplication.
ReadLatestAsyncandReadAllAsyncshare nearly identical reader-to-events loops (connection open, command execute, read/deserialize/skip-null/accumulate). Extracting a private helper for the reader loop would eliminate the duplication and ensure both paths stay consistent if the serialization format changes.♻️ Suggested refactor: extract a shared reader helper
+ private static async Task<List<RuntimeEvent>> ReadEventsAsync(DbCommand command, CancellationToken cancellationToken) + { + var events = new List<RuntimeEvent>(); + await using var reader = await command.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false); + while (await reader.ReadAsync(cancellationToken).ConfigureAwait(false)) + { + if (reader.IsDBNull(0)) continue; + var runtimeEvent = JsonSerializer.Deserialize(reader.GetString(0), ProtocolJsonContext.Default.RuntimeEvent); + if (runtimeEvent is not null) events.Add(runtimeEvent); + } + return events; + } + public async Task<IReadOnlyList<RuntimeEvent>> ReadLatestAsync(string workspacePath, string sessionId, int count, CancellationToken cancellationToken) { ArgumentOutOfRangeException.ThrowIfNegativeOrZero(count); await using var connection = await SqliteSessionStoreDatabase .OpenConnectionAsync(fileSystem, storagePathResolver, workspacePath, cancellationToken) .ConfigureAwait(false); await using var command = connection.CreateCommand(); command.CommandText = """ SELECT payload_json FROM runtime_events WHERE session_id = $sessionId ORDER BY sequence DESC LIMIT $count; """; command.Parameters.AddWithValue("$sessionId", sessionId); command.Parameters.AddWithValue("$count", count); - var events = new List<RuntimeEvent>(); - await using var reader = await command.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false); - while (await reader.ReadAsync(cancellationToken).ConfigureAwait(false)) - { - if (reader.IsDBNull(0)) continue; - var runtimeEvent = JsonSerializer.Deserialize(reader.GetString(0), ProtocolJsonContext.Default.RuntimeEvent); - if (runtimeEvent is not null) events.Add(runtimeEvent); - } + var events = await ReadEventsAsync(command, cancellationToken); events.Reverse(); return events; }The same helper can then be used in
ReadAllAsyncas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SharpClaw.Code.Sessions/Storage/SqliteEventStore.cs` around lines 70 - 107, `ReadLatestAsync` duplicates the same reader-to-`RuntimeEvent` deserialization loop used by `ReadAllAsync`, so extract that logic into a shared private helper in `SqliteEventStore` and have both methods call it. Keep the helper responsible for reading rows, skipping null payloads, deserializing with `ProtocolJsonContext.Default.RuntimeEvent`, and collecting results, while `ReadLatestAsync` still applies its reverse-order handling after reusing the shared loop.src/SharpClaw.Code.Sessions/Abstractions/IEventStore.cs (1)
36-40: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOverride
ReadLatestAsyncin concrete event stores for efficiency.The default implementation loads all events into memory via
ReadAllAsyncthen takes the lastcount. For sessions with large event logs, this is O(n) memory and I/O. Concrete stores (e.g.,SqliteEventStore) should override this with a bounded query (e.g.,ORDER BY sequence DESC LIMIT count) to avoid loading the entire event history.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SharpClaw.Code.Sessions/Abstractions/IEventStore.cs` around lines 36 - 40, The default IEventStore.ReadLatestAsync implementation is inefficient because it reads the full event history via ReadAllAsync and then trims it; update concrete event stores such as SqliteEventStore to override ReadLatestAsync with a bounded query that fetches only the latest count events (for example using descending sequence order with a limit) and returns them in the correct chronological order. Keep the existing argument validation and cancellation handling, and use the same method name/signature so the optimized override is picked up transparently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/SharpClaw.Code.Sessions/Abstractions/IEventStore.cs`:
- Around line 36-40: The default IEventStore.ReadLatestAsync implementation is
inefficient because it reads the full event history via ReadAllAsync and then
trims it; update concrete event stores such as SqliteEventStore to override
ReadLatestAsync with a bounded query that fetches only the latest count events
(for example using descending sequence order with a limit) and returns them in
the correct chronological order. Keep the existing argument validation and
cancellation handling, and use the same method name/signature so the optimized
override is picked up transparently.
In `@src/SharpClaw.Code.Sessions/Storage/SqliteEventStore.cs`:
- Around line 70-107: `ReadLatestAsync` duplicates the same
reader-to-`RuntimeEvent` deserialization loop used by `ReadAllAsync`, so extract
that logic into a shared private helper in `SqliteEventStore` and have both
methods call it. Keep the helper responsible for reading rows, skipping null
payloads, deserializing with `ProtocolJsonContext.Default.RuntimeEvent`, and
collecting results, while `ReadLatestAsync` still applies its reverse-order
handling after reusing the shared loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b658f6a9-fed5-4c0c-9904-7c10f7b84bc8
📒 Files selected for processing (30)
docs/external-agents.mddocs/work-items.mdsrc/SharpClaw.Code.Commands/Handlers/AgentStatusSlashCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/AgentsCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/ExternalCommandHandler.cssrc/SharpClaw.Code.Commands/Handlers/WorkCommandHandler.cssrc/SharpClaw.Code.ExternalAgents/Adapters/ExternalAgentAdapterBase.cssrc/SharpClaw.Code.ExternalAgents/Services/DefaultExternalAgentConfigProvider.cssrc/SharpClaw.Code.ExternalAgents/Services/ExternalAgentRegistry.cssrc/SharpClaw.Code.ExternalAgents/Services/ExternalAgentService.cssrc/SharpClaw.Code.ExternalAgents/Services/PathExternalAgentExecutableResolver.cssrc/SharpClaw.Code.Protocol/Operational/WorkbenchStatusReport.cssrc/SharpClaw.Code.Runtime/Composition/RuntimeServiceCollectionExtensions.cssrc/SharpClaw.Code.Runtime/WorkItems/RuntimeWorkItemConfigProvider.cssrc/SharpClaw.Code.Runtime/Workflow/WorkbenchStatusService.cssrc/SharpClaw.Code.Sessions/Abstractions/IEventStore.cssrc/SharpClaw.Code.Sessions/Storage/HostAwareEventStore.cssrc/SharpClaw.Code.Sessions/Storage/SqliteEventStore.cssrc/SharpClaw.Code.Skills/Services/SkillPackRegistry.cssrc/SharpClaw.Code.WorkItems/Abstractions/IWorkItemConfigProvider.cssrc/SharpClaw.Code.WorkItems/Providers/GenericWorkItemProvider.cssrc/SharpClaw.Code.WorkItems/Providers/GitHubWorkItemProvider.cssrc/SharpClaw.Code.WorkItems/Services/DefaultWorkItemConfigProvider.cssrc/SharpClaw.Code.WorkItems/Services/WorkItemRegistry.cssrc/SharpClaw.Code.WorkItems/Services/WorkItemService.cssrc/SharpClaw.Code.WorkItems/SharpClaw.Code.WorkItems.csprojsrc/SharpClaw.Code.WorkItems/WorkItemsServiceCollectionExtensions.cstests/SharpClaw.Code.UnitTests/Skills/SkillPackRegistryTests.cstests/SharpClaw.Code.UnitTests/WorkItems/GenericWorkItemProviderTests.cstests/SharpClaw.Code.UnitTests/WorkItems/WorkItemRegistryTests.cs
💤 Files with no reviewable changes (1)
- src/SharpClaw.Code.Protocol/Operational/WorkbenchStatusReport.cs
✅ Files skipped from review due to trivial changes (3)
- docs/work-items.md
- src/SharpClaw.Code.WorkItems/Abstractions/IWorkItemConfigProvider.cs
- docs/external-agents.md
🚧 Files skipped from review as they are similar to previous changes (14)
- src/SharpClaw.Code.WorkItems/SharpClaw.Code.WorkItems.csproj
- src/SharpClaw.Code.Commands/Handlers/AgentStatusSlashCommandHandler.cs
- src/SharpClaw.Code.WorkItems/WorkItemsServiceCollectionExtensions.cs
- src/SharpClaw.Code.ExternalAgents/Services/PathExternalAgentExecutableResolver.cs
- src/SharpClaw.Code.WorkItems/Services/WorkItemRegistry.cs
- src/SharpClaw.Code.Runtime/Composition/RuntimeServiceCollectionExtensions.cs
- src/SharpClaw.Code.ExternalAgents/Services/ExternalAgentRegistry.cs
- src/SharpClaw.Code.WorkItems/Providers/GenericWorkItemProvider.cs
- src/SharpClaw.Code.WorkItems/Services/WorkItemService.cs
- src/SharpClaw.Code.Skills/Services/SkillPackRegistry.cs
- src/SharpClaw.Code.ExternalAgents/Services/ExternalAgentService.cs
- src/SharpClaw.Code.Runtime/Workflow/WorkbenchStatusService.cs
- src/SharpClaw.Code.ExternalAgents/Adapters/ExternalAgentAdapterBase.cs
- src/SharpClaw.Code.Commands/Handlers/AgentsCommandHandler.cs
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
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
Validation
Intentionally Deferred
Summary by CodeRabbit
Release Notes
New Features
--output-format json.Documentation