feat(provider): add OpenClaw, Hermes, and PiAgent harnesses with reasoning controls - #5505
feat(provider): add OpenClaw, Hermes, and PiAgent harnesses with reasoning controls#5505Hioness wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect service conventions review — findings in the new OpenClawRuntime service module (apps/server/src/provider/openclawRuntime.ts) and its consumer OpenClawProvider.ts. The Hermes/Pi adapters, drivers, and AcpReasoningConfig look consistent with the conventions (namespace subpath imports, Context.Service-free helper modules, scoped resource ownership, no hidden runtimes).
Posted via Macroscope — Effect Service Conventions
| import { | ||
| OpenClawRuntime, | ||
| openClawRuntimeErrorDetail, | ||
| type OpenClawGatewayConnection, | ||
| type OpenClawRuntimeShape, | ||
| } from "../openclawRuntime.ts"; |
There was a problem hiding this comment.
This import sits mid-file, after the reasoning-option constants and openClawModelCapabilities. Suggest hoisting it into the top import block so the module keeps the canonical order (imports first).
Posted via Macroscope — Effect Service Conventions
| export class OpenClawRuntime extends Context.Service<OpenClawRuntime, OpenClawRuntimeShape>()( | ||
| "t3/provider/openclawRuntime", | ||
| ) {} | ||
|
|
||
| export const OpenClawRuntimeLive = Layer.effect(OpenClawRuntime, makeOpenClawRuntime).pipe( | ||
| Layer.provideMerge(NetService.layer), | ||
| ); |
There was a problem hiding this comment.
Canonical single-file order for a service module is imports, errors/schemas, the Context.Service tag (with its inline interface), then make, then layer — here the tag lands after makeOpenClawRuntime. Consider also exporting a real make and naming the canonical layer layer (export const layer = Layer.effect(OpenClawRuntime, make)), so namespace consumers read OpenClawRuntime.layer in server.ts and the tests.
Posted via Macroscope — Effect Service Conventions
| export interface OpenClawRuntimeShape { | ||
| /** | ||
| * Resolve the gateway for this instance: connect to `gatewayUrl` when set, | ||
| * otherwise spawn `binaryPath gateway --port <free>` and wait for protocol | ||
| * readiness. The child process lifetime is bound to the caller's scope. | ||
| */ | ||
| readonly connectToOpenClawGateway: (input: { | ||
| readonly binaryPath: string; | ||
| readonly gatewayUrl?: string; | ||
| readonly gatewayToken?: string; | ||
| readonly environment?: NodeJS.ProcessEnv; | ||
| readonly stateDir?: string; | ||
| readonly launchArgs?: ReadonlyArray<string>; | ||
| readonly port?: number; | ||
| readonly timeoutMs?: number; | ||
| }) => Effect.Effect<OpenClawGatewayConnection, OpenClawRuntimeError, Scope.Scope>; | ||
| readonly runOpenClawCommand: (input: { | ||
| readonly binaryPath: string; | ||
| readonly args: ReadonlyArray<string>; | ||
| readonly environment?: NodeJS.ProcessEnv; | ||
| }) => Effect.Effect<OpenClawCommandResult, OpenClawRuntimeError>; | ||
| } |
There was a problem hiding this comment.
Consider defining this interface inline in the Context.Service declaration rather than keeping a standalone OpenClawRuntimeShape, and referring to the inferred interface as OpenClawRuntime["Service"] at the implementation and at consumers (OpenClawProvider.ts imports the shape type today).
Posted via Macroscope — Effect Service Conventions
| export class OpenClawRuntimeError extends Data.TaggedError(OPENCLAW_RUNTIME_ERROR_TAG)<{ | ||
| readonly operation: string; | ||
| readonly cause?: unknown; | ||
| readonly detail: string; | ||
| }> { | ||
| static readonly is = (u: unknown): u is OpenClawRuntimeError => | ||
| P.isTagged(u, OPENCLAW_RUNTIME_ERROR_TAG); | ||
| } |
There was a problem hiding this comment.
Service failures should be declared with Schema.TaggedErrorClass and structured attributes (like apps/server/src/provider/Errors.ts), with message derived from those attributes and cause: Schema.optional(Schema.Defect()) instead of cause?: unknown. The hand-rolled static is predicate would then become an exported schema predicate (export const isOpenClawRuntimeError = Schema.is(OpenClawRuntimeError)), which the OpenClawRuntimeError.is(...) call sites here and in OpenClawAdapter.ts can use.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Effect service conventions review — findings in the new OpenClawRuntime service module (apps/server/src/provider/openclawRuntime.ts) and its consumer OpenClawProvider.ts. The Hermes/Pi adapters, drivers, and AcpReasoningConfig look consistent with the conventions (namespace subpath imports, Context.Service-free helper modules, scoped resource ownership, no hidden runtimes).
Posted via Macroscope — Effect Service Conventions
| export class OpenClawRuntime extends Context.Service<OpenClawRuntime, OpenClawRuntimeShape>()( | ||
| "t3/provider/openclawRuntime", | ||
| ) {} | ||
|
|
||
| export const OpenClawRuntimeLive = Layer.effect(OpenClawRuntime, makeOpenClawRuntime).pipe( | ||
| Layer.provideMerge(NetService.layer), | ||
| ); |
There was a problem hiding this comment.
Canonical single-file order for a service module is imports, errors/schemas, the Context.Service tag (with its inline interface), then make, then layer — here the tag lands after makeOpenClawRuntime. Consider also exporting a real make and naming the canonical layer layer (export const layer = Layer.effect(OpenClawRuntime, make)), so namespace consumers read OpenClawRuntime.layer in server.ts and the tests.
Posted via Macroscope — Effect Service Conventions
| export class OpenClawRuntimeError extends Data.TaggedError(OPENCLAW_RUNTIME_ERROR_TAG)<{ | ||
| readonly operation: string; | ||
| readonly cause?: unknown; | ||
| readonly detail: string; | ||
| }> { | ||
| static readonly is = (u: unknown): u is OpenClawRuntimeError => | ||
| P.isTagged(u, OPENCLAW_RUNTIME_ERROR_TAG); | ||
| } |
There was a problem hiding this comment.
Service failures should be declared with Schema.TaggedErrorClass and structured attributes (like apps/server/src/provider/Errors.ts), with message derived from those attributes and cause: Schema.optional(Schema.Defect()) instead of cause?: unknown. The hand-rolled static is predicate would then become an exported schema predicate (export const isOpenClawRuntimeError = Schema.is(OpenClawRuntimeError)), which the OpenClawRuntimeError.is(...) call sites here and in OpenClawAdapter.ts can use.
Posted via Macroscope — Effect Service Conventions
| export interface OpenClawRuntimeShape { | ||
| /** | ||
| * Resolve the gateway for this instance: connect to `gatewayUrl` when set, | ||
| * otherwise spawn `binaryPath gateway --port <free>` and wait for protocol | ||
| * readiness. The child process lifetime is bound to the caller's scope. | ||
| */ | ||
| readonly connectToOpenClawGateway: (input: { | ||
| readonly binaryPath: string; | ||
| readonly gatewayUrl?: string; | ||
| readonly gatewayToken?: string; | ||
| readonly environment?: NodeJS.ProcessEnv; | ||
| readonly stateDir?: string; | ||
| readonly launchArgs?: ReadonlyArray<string>; | ||
| readonly port?: number; | ||
| readonly timeoutMs?: number; | ||
| }) => Effect.Effect<OpenClawGatewayConnection, OpenClawRuntimeError, Scope.Scope>; | ||
| readonly runOpenClawCommand: (input: { | ||
| readonly binaryPath: string; | ||
| readonly args: ReadonlyArray<string>; | ||
| readonly environment?: NodeJS.ProcessEnv; | ||
| }) => Effect.Effect<OpenClawCommandResult, OpenClawRuntimeError>; | ||
| } |
There was a problem hiding this comment.
Consider defining this interface inline in the Context.Service declaration rather than keeping a standalone OpenClawRuntimeShape, and referring to the inferred interface as OpenClawRuntime["Service"] at the implementation and at consumers (OpenClawProvider.ts imports the shape type today).
Posted via Macroscope — Effect Service Conventions
| import { | ||
| OpenClawRuntime, | ||
| openClawRuntimeErrorDetail, | ||
| type OpenClawGatewayConnection, | ||
| type OpenClawRuntimeShape, | ||
| } from "../openclawRuntime.ts"; |
There was a problem hiding this comment.
This import sits mid-file, after the reasoning-option constants and openClawModelCapabilities. Suggest hoisting it into the top import block so the module keeps the canonical order (imports first).
Posted via Macroscope — Effect Service Conventions
| relativePath: input.relativePath, | ||
| }); | ||
|
|
||
| yield* fileSystem.makeDirectory(target.absolutePath, { recursive: true }).pipe( |
There was a problem hiding this comment.
🟠 High workspace/WorkspaceFileSystem.ts:348
makeDirectory creates directories outside the workspace root when a symlinked parent is traversed. If workspace/link is a symlink to /outside, calling makeDirectory with relativePath: "link/new-directory" resolves the path lexically inside the root, then calls fileSystem.makeDirectory(target.absolutePath, { recursive: true }), which creates /outside/new-directory outside the workspace. Unlike readFile, which calls NodeFSP.realpath on both the workspace root and target and checks that the real target path stays inside the real workspace root, makeDirectory only relies on resolveRelativePathWithinRoot — a lexical check that does not follow symlinks. Consider adding the same real-path containment check before mutating the target.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/workspace/WorkspaceFileSystem.ts around line 348:
`makeDirectory` creates directories outside the workspace root when a symlinked parent is traversed. If `workspace/link` is a symlink to `/outside`, calling `makeDirectory` with `relativePath: "link/new-directory"` resolves the path lexically inside the root, then calls `fileSystem.makeDirectory(target.absolutePath, { recursive: true })`, which creates `/outside/new-directory` outside the workspace. Unlike `readFile`, which calls `NodeFSP.realpath` on both the workspace root and target and checks that the real target path stays inside the real workspace root, `makeDirectory` only relies on `resolveRelativePathWithinRoot` — a lexical check that does not follow symlinks. Consider adding the same real-path containment check before mutating the target.
| handledDirectoryRevealRef.current = request; | ||
|
|
||
| const segments = request.path.split("/"); | ||
| let ancestorPath = ""; |
There was a problem hiding this comment.
🟡 Medium files/FileBrowserPanel.tsx:390
The directory-reveal effect calls selectedItem.select() and model.scrollToPath() without first closing the active tree search. When a folder is created while a search query is active, the hide-non-matches search mode keeps the new directory row hidden, so the reveal request has no visible effect. The file-reveal effect above calls model.closeSearch() before selecting and scrolling — the directory-reveal effect should do the same.
| handledDirectoryRevealRef.current = request; | |
| const segments = request.path.split("/"); | |
| let ancestorPath = ""; | |
| handledDirectoryRevealRef.current = request; | |
| model.closeSearch(); | |
| const segments = request.path.split("/"); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/files/FileBrowserPanel.tsx around lines 390-393:
The directory-reveal effect calls `selectedItem.select()` and `model.scrollToPath()` without first closing the active tree search. When a folder is created while a search query is active, the `hide-non-matches` search mode keeps the new directory row hidden, so the reveal request has no visible effect. The file-reveal effect above calls `model.closeSearch()` before selecting and scrolling — the directory-reveal effect should do the same.
| * single trailing `\r`; `StringDecoder` handles UTF-8 multi-byte sequences | ||
| * that span chunk boundaries. | ||
| */ | ||
| export function makePiRecordSplitter(): { |
There was a problem hiding this comment.
🟠 High Layers/PiAgentSessionRuntime.ts:168
flush() silently drops the last JSON record if it has no trailing \n. When pi writes a final response object and closes stdout immediately after it, flushRecords() returns [] because remainder contains no newline, so the response is discarded and its pending RPC waits until timeout. The remaining content should still be emitted as a record (even without a trailing newline). Additionally, the stdout consumer never calls flush() at EOF, so any partial final record is lost regardless. Consider emitting remainder (if non-empty) from flushRecords() when called by flush(), and invoking recordSplitter.flush() after the stdout stream completes.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/PiAgentSessionRuntime.ts around line 168:
`flush()` silently drops the last JSON record if it has no trailing `\n`. When pi writes a final response object and closes stdout immediately after it, `flushRecords()` returns `[]` because `remainder` contains no newline, so the response is discarded and its pending RPC waits until timeout. The remaining content should still be emitted as a record (even without a trailing newline). Additionally, the stdout consumer never calls `flush()` at EOF, so any partial final record is lost regardless. Consider emitting `remainder` (if non-empty) from `flushRecords()` when called by `flush()`, and invoking `recordSplitter.flush()` after the stdout stream completes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 219abf4. Configure here.
| }), | ||
| ], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Pi ignores reasoning capability flag
Medium Severity
The reasoning flag in piModelCapabilities is not used to conditionally include the reasoningEffort option. This results in all models, even those without reasoning capabilities, incorrectly displaying the reasoningEffort control. The issue is compounded during RPC model discovery, where the Pi RPC reasoning field is ignored, and piModelCapabilities(true) is always applied.
Reviewed by Cursor Bugbot for commit 219abf4. Configure here.
| ? { model: requestedStartModelId } | ||
| : boundModelId | ||
| ? { model: boundModelId } | ||
| : {}), |
There was a problem hiding this comment.
Hermes stores ACP model encoding
Medium Severity
When no model selection is provided, startSession falls back to boundModelId from ACP, which is provider:model, and stores that on session.model. T3 catalogs and defaults use provider/model slugs (hermesModelSlugFromAcpModelId already exists for this). Grok normalizes before storing; Hermes does not, so session model state can fail to match catalog slugs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 219abf4. Configure here.
| providerInstanceId: boundInstanceId, | ||
| threadId: input.threadId, | ||
| payload: { resume: resumeKey !== undefined }, | ||
| }); |
There was a problem hiding this comment.
OpenClaw resume flag wrong on recreate
Medium Severity
session.started sets resume from whether a resume cursor was supplied, not whether the existing gateway session was actually reused. When sessions.describe returns not-found and a fresh session is created, the event still reports resume: true.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 219abf4. Configure here.
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
| response("prompt", id, { ok: true }); | ||
| return; | ||
| } | ||
| emitMockToolCalls(); |
There was a problem hiding this comment.
🟡 Medium scripts/pi-mock-agent.ts:211
handlePrompt calls emitMockToolCalls() unconditionally, so every prompt — including the documented minimal happy-path with no T3_PI_* flags set — emits a full bash tool execution lifecycle (tool_execution_start, tool_execution_update, tool_execution_end). The emitToolCalls flag parsed from T3_PI_EMIT_TOOL_CALLS is never consulted, so tests that omit that flag still receive tool events and cannot use the mock to simulate a tool-free turn. Gate the call behind if (emitToolCalls) so tool events are only emitted when explicitly requested.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/scripts/pi-mock-agent.ts around line 211:
`handlePrompt` calls `emitMockToolCalls()` unconditionally, so every prompt — including the documented minimal happy-path with no `T3_PI_*` flags set — emits a full `bash` tool execution lifecycle (`tool_execution_start`, `tool_execution_update`, `tool_execution_end`). The `emitToolCalls` flag parsed from `T3_PI_EMIT_TOOL_CALLS` is never consulted, so tests that omit that flag still receive tool events and cannot use the mock to simulate a tool-free turn. Gate the call behind `if (emitToolCalls)` so tool events are only emitted when explicitly requested.
| if (activeTurnId && exitKind === "error") { | ||
| yield* offerRuntimeEvent({ | ||
| ...base, | ||
| type: "turn.completed", | ||
| payload: { state: "failed", errorMessage: message }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟠 High Layers/PiAgentAdapter.ts:664
When the Pi process exits with code 0 during an active turn, the turn is never completed — consumers receive session.exited but no turn.completed. The handler clears activeTurnId and emits turn.completed only when exitKind === "error", so a graceful exit leaves the active turn permanently unfinished. Consider emitting a turn.completed event for the active turn on every exit, not just error exits.
if (activeTurnId) {
+ const interrupted = ctx.interruptedTurnIds.has(activeTurnId);
+ ctx.interruptedTurnIds.delete(activeTurnId);
yield* offerRuntimeEvent({
...base,
type: "turn.completed",
- payload: { state: "failed", errorMessage: message },
+ payload: interrupted
+ ? { state: "interrupted" }
+ : exitKind === "error"
+ ? { state: "failed", errorMessage: message }
+ : { state: "cancelled", errorMessage: message },
});
}🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/PiAgentAdapter.ts around lines 664-670:
When the Pi process exits with code 0 during an active turn, the turn is never completed — consumers receive `session.exited` but no `turn.completed`. The handler clears `activeTurnId` and emits `turn.completed` only when `exitKind === "error"`, so a graceful exit leaves the active turn permanently unfinished. Consider emitting a `turn.completed` event for the active turn on every exit, not just error exits.
| ...(input.launchArgs !== undefined ? { launchArgs: input.launchArgs } : {}), | ||
| }) | ||
| .pipe(Effect.provideService(Scope.Scope, gatewayScope)); | ||
| yield* Ref.set(cached, Option.some(connection)); |
There was a problem hiding this comment.
🟠 High Layers/OpenClawAdapter.ts:153
After the gateway WebSocket closes or the spawned process exits, acquire keeps returning the cached OpenClawGatewayConnection forever. Subsequent startSession, sendTurn, and other calls reuse the dead connection instead of reconnecting, so the provider stays unusable until the entire driver scope is recreated. The holder caches the connection in cached but never clears it when markSessionsClosed fires on the closed event or the process exits. Consider resetting cached to Option.none() when the connection's events stream emits closed, so acquire reconnects on the next call.
- yield* Ref.set(cached, Option.some(connection));
+ yield* Ref.set(cached, Option.some(connection));
+ yield* connection.events.pipe(
+ Stream.runForEach((event) =>
+ event.kind === "closed"
+ ? Ref.set(cached, Option.none())
+ : Effect.void
+ ),
+ Effect.forkScoped,
+ );🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OpenClawAdapter.ts around line 153:
After the gateway WebSocket closes or the spawned process exits, `acquire` keeps returning the cached `OpenClawGatewayConnection` forever. Subsequent `startSession`, `sendTurn`, and other calls reuse the dead connection instead of reconnecting, so the provider stays unusable until the entire driver scope is recreated. The holder caches the connection in `cached` but never clears it when `markSessionsClosed` fires on the `closed` event or the process exits. Consider resetting `cached` to `Option.none()` when the connection's `events` stream emits `closed`, so `acquire` reconnects on the next call.
| case "assistant": { | ||
| const delta = trimText(data.delta) ?? trimText(data.text); | ||
| if (!delta) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Medium Layers/OpenClawAdapter.ts:474
handleAgentEvent trims every assistant and thinking delta with trimText, stripping leading/trailing whitespace from each chunk. When a streaming delta arrives with boundary whitespace (e.g. " hello " then "world"), consumers concatenating content.delta events receive "helloworld" instead of " hello world". Use the raw data.delta/data.text value and only skip chunks when the field is absent or not a string, rather than discarding whitespace-only chunks.
case "assistant": {
- const delta = trimText(data.delta) ?? trimText(data.text);
- if (!delta) {
+ const delta = asString(data.delta) ?? asString(data.text);
+ if (delta === undefined) {
return;
}🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OpenClawAdapter.ts around lines 474-478:
`handleAgentEvent` trims every `assistant` and `thinking` delta with `trimText`, stripping leading/trailing whitespace from each chunk. When a streaming delta arrives with boundary whitespace (e.g. `" hello "` then `"world"`), consumers concatenating `content.delta` events receive `"helloworld"` instead of `" hello world"`. Use the raw `data.delta`/`data.text` value and only skip chunks when the field is absent or not a string, rather than discarding whitespace-only chunks.
| Effect.gen(function* () { | ||
| if (yield* Ref.get(promptSettled)) { | ||
| return; | ||
| } | ||
|
|
||
| const promptResult = yield* Ref.get(promptResultRef); | ||
| if (promptResult !== undefined) { | ||
| const liveCtx = sessions.get(input.threadId); | ||
| if (liveCtx && !liveCtx.stopped && liveCtx.acpSessionId === prepared.acpSessionId) { | ||
| appendPromptResultToTurn( | ||
| liveCtx, | ||
| prepared.turnId, | ||
| prepared.promptParts, | ||
| promptResult, | ||
| ); | ||
| } | ||
| yield* withThreadLock( |
There was a problem hiding this comment.
🟡 Medium Layers/HermesAdapter.ts:1166
readThread returns corrupted turn history with duplicate prompt entries. When the main settlement path appends a prompt result at line 1104 but the fiber is interrupted before promptSettled is set, the Effect.ensuring cleanup at line 1175 appends the same result again because it only checks promptResultRef, not whether the append already happened. Consider tracking the append state in the same Ref that guards settlement, or making appendPromptResultToTurn idempotent.
Effect.gen(function* () {
if (yield* Ref.get(promptSettled)) {
return;
}
const promptResult = yield* Ref.get(promptResultRef);
if (promptResult !== undefined) {
+ yield* Ref.set(promptSettled, true);
const liveCtx = sessions.get(input.threadId);
if (liveCtx && !liveCtx.stopped && liveCtx.acpSessionId === prepared.acpSessionId) {
appendPromptResultToTurn(🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/HermesAdapter.ts around lines 1166-1182:
`readThread` returns corrupted turn history with duplicate prompt entries. When the main settlement path appends a prompt result at line 1104 but the fiber is interrupted before `promptSettled` is set, the `Effect.ensuring` cleanup at line 1175 appends the same result again because it only checks `promptResultRef`, not whether the append already happened. Consider tracking the append state in the same `Ref` that guards settlement, or making `appendPromptResultToTurn` idempotent.
| setTimeout(() => { | ||
| emitAgentEnd(); | ||
| emitAgentSettled(); | ||
| }, settleDelayMs); |
There was a problem hiding this comment.
🟡 Medium scripts/pi-mock-agent.ts:218
The setTimeout at line 218 is never stored or cancelled, so aborting a delayed run produces duplicate agent_end/agent_settled events when the orphaned timer fires. Worse, if a new prompt starts before that timer expires, its callback sets isStreaming=false and emits terminal events for the new run, corrupting its lifecycle. Save the timer handle and clearTimeout it in handleAbort before emitting the terminal events.
setTimeout(() => {
+ settleTimer = undefined;
emitAgentEnd();
emitAgentSettled();
}, settleDelayMs);🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/scripts/pi-mock-agent.ts around lines 218-221:
The `setTimeout` at line 218 is never stored or cancelled, so aborting a delayed run produces duplicate `agent_end`/`agent_settled` events when the orphaned timer fires. Worse, if a new prompt starts before that timer expires, its callback sets `isStreaming=false` and emits terminal events for the new run, corrupting its lifecycle. Save the timer handle and `clearTimeout` it in `handleAbort` before emitting the terminal events.
| return byKind?.optionId.trim() || acpPermissionOutcome(decision); | ||
| } | ||
|
|
||
| function selectAutoApprovedHermesPermissionOption( |
There was a problem hiding this comment.
🟠 High Layers/HermesAdapter.ts:223
selectAutoApprovedHermesPermissionOption never reaches its "accept" fallback. When a permission request offers only allow_once, full-access mode sends the synthetic allow-always outcome string as the option ID. Hermes does not recognize that ID and maps it to denial, so a permission that should be auto-approved is denied. The issue is that selectHermesPermissionOptionId falls back to acpPermissionOutcome(decision) (which returns "allow-always" for "acceptForSession") when no matching option is present, producing a non-undefined return that short-circuits the ?? chain. Consider checking whether the selected value is actually one of request.options before returning it, or excluding the acpPermissionOutcome fallback in the auto-approve path so undefined propagates and the "accept" alternative is tried.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/HermesAdapter.ts around line 223:
`selectAutoApprovedHermesPermissionOption` never reaches its `"accept"` fallback. When a permission request offers only `allow_once`, full-access mode sends the synthetic `allow-always` outcome string as the option ID. Hermes does not recognize that ID and maps it to denial, so a permission that should be auto-approved is denied. The issue is that `selectHermesPermissionOptionId` falls back to `acpPermissionOutcome(decision)` (which returns `"allow-always"` for `"acceptForSession"`) when no matching option is present, producing a non-`undefined` return that short-circuits the `??` chain. Consider checking whether the selected value is actually one of `request.options` before returning it, or excluding the `acpPermissionOutcome` fallback in the auto-approve path so `undefined` propagates and the `"accept"` alternative is tried.
| } | ||
| const cwd = path.resolve(input.cwd.trim()); | ||
| const existing = sessions.get(input.threadId); | ||
| if (existing && !existing.stopped) { |
There was a problem hiding this comment.
🟠 High Layers/OpenClawAdapter.ts:716
stopAll calls stopSessionInternal directly, so it never sends chat.abort for sessions with an active run — OpenClaw keeps generating remotely after the local context is deleted, with no way for this adapter to interrupt the orphaned run. The same problem occurs in startSession: replacing an existing session calls stopSessionInternal(existing) instead of the aborting stopSession path, so an active run on the old context continues in the background after the new context is installed. Both paths should go through stopSession (or otherwise send chat.abort before deleting the context) so in-flight runs are cancelled.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OpenClawAdapter.ts around line 716:
`stopAll` calls `stopSessionInternal` directly, so it never sends `chat.abort` for sessions with an active run — OpenClaw keeps generating remotely after the local context is deleted, with no way for this adapter to interrupt the orphaned run. The same problem occurs in `startSession`: replacing an existing session calls `stopSessionInternal(existing)` instead of the aborting `stopSession` path, so an active run on the old context continues in the background after the new context is installed. Both paths should go through `stopSession` (or otherwise send `chat.abort` before deleting the context) so in-flight runs are cancelled.
|
|
||
| const rawOptions = Array.isArray(request.options) ? request.options : []; | ||
| const options = rawOptions | ||
| .map((entry): UserInputQuestion["options"][number] | undefined => { |
There was a problem hiding this comment.
🟠 High Layers/PiAgentAdapter.ts:481
select requests discard each option's protocol value and expose only its display label. When the user picks an option, respondToUserInput sends that label back to Pi as value. For any request where option values differ from labels (e.g. { value: "model-id", label: "Model Name" }), Pi receives "Model Name" instead of "model-id", so the user's selection is misinterpreted or rejected. Consider preserving a label-to-value mapping when the pending request is created so the answer can be translated back to the original value before responding.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/PiAgentAdapter.ts around line 481:
`select` requests discard each option's protocol `value` and expose only its display `label`. When the user picks an option, `respondToUserInput` sends that label back to Pi as `value`. For any request where option values differ from labels (e.g. `{ value: "model-id", label: "Model Name" }`), Pi receives `"Model Name"` instead of `"model-id"`, so the user's selection is misinterpreted or rejected. Consider preserving a label-to-value mapping when the pending request is created so the answer can be translated back to the original value before responding.
| Effect.logError("Failed to process Pi runtime notification.", { cause }), | ||
| ), | ||
| ), | ||
| ).pipe(Effect.forkChild); |
There was a problem hiding this comment.
🟠 High Layers/PiAgentAdapter.ts:973
The Pi event consumer fiber is forked with Effect.forkChild inside the Effect.scoped wrapping startSession, so the scope closes and interrupts the fiber when startSession returns — even though sessionScope was transferred separately. This stops all Pi event processing after startup: assistant output, tool events, agent_settled, extension requests, and process exit are never consumed, so turns hang and clients receive no runtime updates. Fork the consumer into sessionScope instead so it survives the startSession scope.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/PiAgentAdapter.ts around line 973:
The Pi event consumer fiber is forked with `Effect.forkChild` inside the `Effect.scoped` wrapping `startSession`, so the scope closes and interrupts the fiber when `startSession` returns — even though `sessionScope` was transferred separately. This stops all Pi event processing after startup: assistant output, tool events, `agent_settled`, extension requests, and process exit are never consumed, so turns hang and clients receive no runtime updates. Fork the consumer into `sessionScope` instead so it survives the `startSession` scope.
1 participant
Problem
OpenClaw, Hermes, and Grok did not show a reasoning control in the composer. Every other harness has one.
Fix
This PR adds the OpenClaw, Hermes, and PiAgent harnesses. It also adds the reasoning controls that the new harnesses did not have.
OpenClaw now advertises a
reasoningEffortdescriptor. The adapter already sent the selection to the gateway. The control appears now that the catalog declares it.Grok and Hermes read the effort or reasoning session config option during ACP discovery. They advertise it as a
reasoningdescriptor and apply the selection on each turn. The control stays hidden when the CLI does not expose such an option.The UI is descriptor-driven. Web, desktop, and mobile show the control without client changes.
Tests
Server typecheck passes. All 130 targeted tests pass. New tests cover the ACP reasoning bridge, the provider catalogs, and the adapter apply path.
Model
Model: deepseek-v4-flash. Harness: opencode.
Note
High Risk
Large new surface area: spawns external CLIs/gateways, WebSocket sessions, and turn/approval lifecycle; misconfiguration or process leaks could affect orchestration reliability.
Overview
Adds Pi, Hermes, and OpenClaw as first-class provider drivers (adapters, snapshots, text generation, and driver registration), with CI-friendly mock
piand OpenClaw gateway scripts driving the same protocols as production.Reasoning in the composer is wired through shared ACP reasoning helpers: Grok and Hermes discover effort/reasoning session config options and expose them as model
reasoningdescriptors, then apply selections viasession/set_config_optionon session start and each turn (includingeffortonturn.started). Hermes also gets a full ACP adapter (permissions, steering, MCP skip when bound). OpenClaw uses a shared gateway holder and WebSocket runtime with broad adapter tests.Mobile picks up provider icons and display labels for the new drivers. RPC auth gains operate scopes for
projectsMakeDirectoryandprojectsDeleteFile.Reviewed by Cursor Bugbot for commit 219abf4. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add OpenClaw, Hermes, and PiAgent provider harnesses with reasoning controls
piAgent,hermes,openclaw) end-to-end: contracts, server drivers, adapters, text generation services, UI icons, settings schemas, picker entries, and user docs.HermesAdapter,PiAgentAdapter,OpenClawAdapter) that maps native protocol events to the canonicalProviderRuntimeEventstream, including session lifecycle, tool calls, approvals, and token usage.AcpReasoningConfigbridge that reads ACPSessionConfigOptionentries and constructs areasoningselect descriptor; Grok discovery also gains this capability.makeDirectoryanddeleteFileworkspace RPC endpoints (projects.makeDirectory,projects.deleteFile) with path-safety checks, recursive-deletion guards, cache invalidation, and authorization scope enforcement.navigator.clipboard.writeTextcalls with a centralizedwriteTextToClipboardhelper that falls back todocument.execCommand('copy')on insecure origins.piAgentrollback is explicitly unsupported and returns aProviderAdapterValidationError; OpenClaw also lacks rollback and free-form user input support.📊 Macroscope summarized 219abf4. 25 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.