tasks: add detached runtime plugin registration contract#68915
tasks: add detached runtime plugin registration contract#68915
Conversation
6e5b5d8 to
888f9f4
Compare
Greptile SummaryThis PR adds the explicit plugin registration contract for detached task runtimes: a new state module ( Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/design suggestions with no blocking correctness issues. The implementation is well-structured with clean separation between state, dispatch, and routing layers. Loader rollback/restore coverage is thorough across all four identified paths. Both open observations are P2 and do not affect correctness under the stated design invariants. src/plugins/registry.ts (same-plugin re-registration), src/tasks/task-executor.ts (early-return in cancelDetachedTaskRunById) Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/plugins/registry.ts
Line: 1176-1188
Comment:
**Same-plugin silent re-registration**
The guard `existing.pluginId !== record.id` allows the same plugin to call `registerDetachedTaskRuntime` multiple times, silently replacing the prior runtime without any diagnostic. If a plugin's activation path calls this twice (e.g., due to a misconfigured re-entrant load), the second registration overwrites the first with no warning — the same protection offered to *other* plugins is absent for the registering plugin itself. Consider emitting a warning-level diagnostic or making the second call a no-op when the plugin id already matches.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/tasks/task-executor.ts
Line: 711-724
Comment:
**Registered runtime bypassed for unknown tasks**
When `getTaskById` returns `undefined` the function immediately falls back to `cancelTaskById`, never consulting the registered runtime. If a task were ever created through the registered runtime before it appeared in the core registry (or after it was removed from it), the registered runtime's cancel logic would be skipped entirely. The fallback is correct by current design invariants, but the early-return bypasses the runtime contract the rest of the function enforces.
```typescript
// current
if (!task) {
return cancelTaskById(params);
}
// would be more consistent with the contract:
if (!task) {
const registeredRuntime = getRegisteredDetachedTaskLifecycleRuntime();
if (registeredRuntime) {
const result = await registeredRuntime.cancelDetachedTaskRunById(params);
if (result.found) return result;
}
return cancelTaskById(params);
}
```
This is a design-level call — fine to leave as-is if the invariant "task in registry ⟺ runtime owns it" holds firmly, but worth a comment in the code.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "tasks: harden detached runtime ownership" | Re-trigger Greptile |
| registerDetachedTaskRuntime: (runtime) => { | ||
| const existing = getDetachedTaskLifecycleRuntimeRegistration(); | ||
| if (existing && existing.pluginId !== record.id) { | ||
| pushDiagnostic({ | ||
| level: "error", | ||
| pluginId: record.id, | ||
| source: record.source, | ||
| message: `detached task runtime already registered by ${existing.pluginId}`, | ||
| }); | ||
| return; | ||
| } | ||
| registerDetachedTaskLifecycleRuntime(record.id, runtime); | ||
| }, |
There was a problem hiding this comment.
The guard existing.pluginId !== record.id allows the same plugin to call registerDetachedTaskRuntime multiple times, silently replacing the prior runtime without any diagnostic. If a plugin's activation path calls this twice (e.g., due to a misconfigured re-entrant load), the second registration overwrites the first with no warning — the same protection offered to other plugins is absent for the registering plugin itself. Consider emitting a warning-level diagnostic or making the second call a no-op when the plugin id already matches.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/registry.ts
Line: 1176-1188
Comment:
**Same-plugin silent re-registration**
The guard `existing.pluginId !== record.id` allows the same plugin to call `registerDetachedTaskRuntime` multiple times, silently replacing the prior runtime without any diagnostic. If a plugin's activation path calls this twice (e.g., due to a misconfigured re-entrant load), the second registration overwrites the first with no warning — the same protection offered to *other* plugins is absent for the registering plugin itself. Consider emitting a warning-level diagnostic or making the second call a no-op when the plugin id already matches.
How can I resolve this? If you propose a fix, please make it concise.| export async function cancelDetachedTaskRunById(params: { cfg: OpenClawConfig; taskId: string }) { | ||
| const task = getTaskById(params.taskId); | ||
| if (!task) { | ||
| return cancelTaskById(params); | ||
| } | ||
| const registeredRuntime = getRegisteredDetachedTaskLifecycleRuntime(); | ||
| if (registeredRuntime) { | ||
| const cancelled = await registeredRuntime.cancelDetachedTaskRunById(params); | ||
| if (cancelled.found) { | ||
| return cancelled; | ||
| } | ||
| } | ||
| return cancelTaskById(params); | ||
| } |
There was a problem hiding this comment.
When getTaskById returns undefined the function immediately falls back to cancelTaskById, never consulting the registered runtime. If a task were ever created through the registered runtime before it appeared in the core registry (or after it was removed from it), the registered runtime's cancel logic would be skipped entirely. The fallback is correct by current design invariants, but the early-return bypasses the runtime contract the rest of the function enforces.
// current
if (!task) {
return cancelTaskById(params);
}
// would be more consistent with the contract:
if (!task) {
const registeredRuntime = getRegisteredDetachedTaskLifecycleRuntime();
if (registeredRuntime) {
const result = await registeredRuntime.cancelDetachedTaskRunById(params);
if (result.found) return result;
}
return cancelTaskById(params);
}This is a design-level call — fine to leave as-is if the invariant "task in registry ⟺ runtime owns it" holds firmly, but worth a comment in the code.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tasks/task-executor.ts
Line: 711-724
Comment:
**Registered runtime bypassed for unknown tasks**
When `getTaskById` returns `undefined` the function immediately falls back to `cancelTaskById`, never consulting the registered runtime. If a task were ever created through the registered runtime before it appeared in the core registry (or after it was removed from it), the registered runtime's cancel logic would be skipped entirely. The fallback is correct by current design invariants, but the early-return bypasses the runtime contract the rest of the function enforces.
```typescript
// current
if (!task) {
return cancelTaskById(params);
}
// would be more consistent with the contract:
if (!task) {
const registeredRuntime = getRegisteredDetachedTaskLifecycleRuntime();
if (registeredRuntime) {
const result = await registeredRuntime.cancelDetachedTaskRunById(params);
if (result.found) return result;
}
return cancelTaskById(params);
}
```
This is a design-level call — fine to leave as-is if the invariant "task in registry ⟺ runtime owns it" holds firmly, but worth a comment in the code.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888f9f44a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const cancelled = await registeredRuntime.cancelDetachedTaskRunById(params); | ||
| if (cancelled.found) { |
There was a problem hiding this comment.
Handle runtime cancel exceptions before returning
When a registered detached runtime throws inside cancelDetachedTaskRunById (for example due to a transient RPC failure), this function currently propagates the exception instead of returning the structured cancel result used by the rest of the task APIs. That means callers like task/flow cancellation can fail hard and skip both the legacy fallback and normal reason reporting, leaving cancellation in a partially applied state. Wrap the runtime call in a try/catch and return a non-throwing result (or fallback) to preserve existing cancellation semantics.
Useful? React with 👍 / 👎.
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟠 Detached task lifecycle runtime can be overwritten via internal state registration (plugin isolation bypass)
DescriptionThe new global detached task runtime state allows unguarded overwrites.
Impact (if third-party plugins can access these internal modules at runtime): a malicious or compromised plugin could hijack detached task lifecycle operations (create/start/complete/fail/cancel), enabling task cancellation or manipulation across plugins. Vulnerable code: export function registerDetachedTaskLifecycleRuntime(pluginId: string, runtime: DetachedTaskLifecycleRuntime): void {
detachedTaskLifecycleRuntimeRegistration = { pluginId, runtime };
}RecommendationEnforce exclusivity at the state layer as the single source of truth, so callers cannot bypass checks. Example: export function registerDetachedTaskLifecycleRuntime(
pluginId: string,
runtime: DetachedTaskLifecycleRuntime,
): void {
const existing = detachedTaskLifecycleRuntimeRegistration;
if (existing && existing.pluginId !== pluginId) {
throw new Error(`detached task runtime already registered by ${existing.pluginId}`);
}
detachedTaskLifecycleRuntimeRegistration = { pluginId, runtime };
}Additionally:
2. 🟡 Untrusted detached-task runtime can return arbitrary TaskRecord in cancel response (data leakage)
Description
Because
Vulnerable flow:
Vulnerable code: const cancelled = await registeredRuntime.cancelDetachedTaskRunById(params);
if (cancelled.found) {
return cancelled;
}RecommendationDo not trust Recommended mitigations (apply one or more):
const cancelled = await registeredRuntime.cancelDetachedTaskRunById(params);
if (cancelled.found) {
const task = getTaskById(params.taskId);
return {
...cancelled,
task: task ?? undefined,
};
}
Analyzed PR: #68915 at commit Last updated on: 2026-04-19T11:21:37Z |
3c942b2 to
3bcd35f
Compare
3bcd35f to
68717e7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68717e7317
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (registeredRuntime) { | ||
| const cancelled = await registeredRuntime.cancelDetachedTaskRunById(params); | ||
| if (cancelled.found) { |
There was a problem hiding this comment.
Handle runtime cancel exceptions in detached cancel path
cancelDetachedTaskRunById directly awaits registeredRuntime.cancelDetachedTaskRunById(...) without a guard, so any runtime-side exception (for example transient RPC/network failures) will throw out of this API instead of returning the structured cancel result that callers expect. Since this function now fronts flow and task cancellation paths, one thrown runtime error can abort cancellation mid-operation and skip both legacy fallback and reason reporting.
Useful? React with 👍 / 👎.
* test: stabilize standalone Parallels smoke lanes * fix: strip orphaned OpenAI reasoning blocks before responses API call (openclaw#55787) Merged via squash. Prepared head SHA: 263b952 Co-authored-by: suboss87 <11032439+suboss87@users.noreply.github.com> Co-authored-by: jalehman <550978+jalehman@users.noreply.github.com> Reviewed-by: @jalehman * fix: stabilize release smoke reruns * test: tolerate empty fireworks live responses * test: make OpenWebUI smoke deterministic * tasks: extract detached task lifecycle runtime (openclaw#68886) * tasks: extract detached task lifecycle runtime * tests: relax gateway seam expectation --------- Co-authored-by: Mariano Belinky <mariano@mb-server-643.local> * test: complete workspace setup in update smokes * fix: parse PowerShell cron tools allow-list (openclaw#68858) (thanks @chen-zhang-cs-code) * fix(cron): parse PowerShell tools allow list * fix(cron): clarify tools allow-list help * fix: parse PowerShell cron tools allow-list (openclaw#68858) (thanks @chen-zhang-cs-code) --------- Co-authored-by: Ayaan Zaidi <hi@obviy.us> * fix(browser): discover CDP websocket from bare ws:// URL before attach (openclaw#68715) * fix(browser): discover CDP websocket from bare ws:// URL before attach When browser.cdpUrl is set to a bare ws://host:port (no /devtools/ path), ensureBrowserAvailable would call isChromeReachable -> canOpenWebSocket against the URL verbatim. Chrome only accepts WebSocket upgrades at the specific path returned by /json/version, so the handshake failed immediately with HTTP 400. With attachOnly: true, that surfaced as: Browser attachOnly is enabled and profile "openclaw" is not running. even though the CDP endpoint was reachable and the profile was healthy. Reproduced by the new tests in chrome.test.ts and cdp.test.ts (openclaw#68027). Fix: introduce isDirectCdpWebSocketEndpoint(url) — true only when a ws/wss URL has a /devtools/<kind>/<id> handshake path. Route any other ws/wss cdpUrl (including the bare ws://host:port shape) through HTTP /json/version discovery by normalising the scheme via the existing normalizeCdpHttpBaseForJsonEndpoints helper. Apply this in isChromeReachable, getChromeWebSocketUrl, and createTargetViaCdp. Direct WS endpoints with a /devtools/ path are still opened without an extra discovery round-trip. Fixes openclaw#68027 * test(browser): add seeded fuzz coverage for CDP URL helpers Adds property-based / seeded-fuzz tests for the URL helpers the attachOnly CDP fix depends on (openclaw#68027): - isWebSocketUrl - isDirectCdpWebSocketEndpoint - normalizeCdpHttpBaseForJsonEndpoints - parseBrowserHttpUrl - redactCdpUrl - appendCdpPath - getHeadersWithAuth Follows the existing repo convention (see src/gateway/http-common.fuzz.test.ts): no fast-check dep, small mulberry32 PRNG + hand-rolled generators, deterministic per-describe seeds so failures are reproducible. Lifts cdp.helpers.ts coverage from 77.77% -> 89.54% statements, 67.9% -> 80.24% branches, 78% -> 90% lines. Remaining uncovered lines are inside the WS sender internals (createCdpSender, withCdpSocket, fetchCdpChecked rate-limit branch), which require integration-style mocks and are unrelated to the attachOnly fix. * test(browser): drive cdp.helpers/cdp/chrome to 100% coverage Lifts the three files touched by the openclaw#68027 attachOnly fix to 100% statements/branches/functions/lines across the extensions test suite. Adds cdp.helpers.internal.test.ts, cdp.internal.test.ts, and chrome.internal.test.ts covering error paths, branch matrices, CDP session helpers, Chrome spawn/launch/stop flows, and canRunCdpHealthCommand. Defensively unreachable guards are annotated with c8 ignore + inline justifications. * fix(browser): restore WS fallback for non-/devtools ws:// CDP URLs When /json/version discovery is unavailable (or returns no webSocketDebuggerUrl), fall back to treating the original bare ws/wss URL as a direct WebSocket endpoint. This preserves the openclaw#68027 fix for Chrome's debug port while restoring compatibility with Browserless/ Browserbase-style providers that expose a direct WebSocket root without a /json/version endpoint. Priority order for bare ws/wss cdpUrl inputs: 1. /devtools/<kind>/<id> URL \u2192 direct handshake, no discovery (unchanged) 2. bare ws/wss root \u2192 try HTTP discovery first; if discovery returns a webSocketDebuggerUrl use it; otherwise fall back to the original URL as a direct WS endpoint 3. HTTP/HTTPS URL \u2192 HTTP discovery only, no fallback (unchanged) Affected call sites: isChromeReachable, getChromeWebSocketUrl, createTargetViaCdp. Also renames a misleading test ('still enforces SSRF policy for direct WebSocket URLs') to accurately describe what it tests: SSRF enforcement on the navigation target URL, not on the CDP endpoint. New tests added for all three fallback paths. Coverage remains 100% on all three touched files (238 tests). * fix: browser attachOnly bare ws CDP follow-ups (openclaw#68715) (thanks @visionik) * test(tasks): align detached runtime mock return types * browser: route existing-session user profile through browser nodes (openclaw#68891) * browser: route user profile through browser nodes * browser: align existing-session node docs * browser: preserve host fallback on node discovery errors * browser: preserve configured node pin errors * browser: widen config mock in node pin test * fix: default kimi thinking to off (openclaw#68907) Co-authored-by: termtek <termtek@ubuntu.tail2b72cd.ts.net> * docs(changelog): note kimi thinking default fix * docs(changelog): add thanks for kimi fix * tasks: add detached runtime plugin registration contract (openclaw#68915) * tasks: register detached runtime plugins * tasks: harden detached runtime ownership * tasks: extract detached runtime contract types * changelog: note detached runtime contract * changelog: attribute detached runtime contract * feat(ui): add a2a operator dashboard shell * fix(ui): align a2a baseline with current read contract * feat(ui): add A2A case inspector panels * CI: route small workflows to ubuntu-latest * CI: route fork-blocked workflows to GitHub-hosted runners * CI: fall back to GITHUB_TOKEN for fork automation * CI: skip app-token steps when fork secrets are absent * CI: gate fork app-token steps through env guards * CI: fix stale fallback env guard * fix(ui): sync A2A locale snapshots * fix(ui): sync A2A locale metadata --------- Co-authored-by: Peter Steinberger <steipete@gmail.com> Co-authored-by: Subash Natarajan <suboss87@gmail.com> Co-authored-by: suboss87 <11032439+suboss87@users.noreply.github.com> Co-authored-by: jalehman <550978+jalehman@users.noreply.github.com> Co-authored-by: Mariano <132747814+mbelinky@users.noreply.github.com> Co-authored-by: Mariano Belinky <mariano@mb-server-643.local> Co-authored-by: ZC <chenzhangcode@163.com> Co-authored-by: Ayaan Zaidi <hi@obviy.us> Co-authored-by: Viz <visionik@pobox.com> Co-authored-by: Frank Yang <frank.ekn@gmail.com> Co-authored-by: termtek <termtek@ubuntu.tail2b72cd.ts.net> Co-authored-by: bangtong-ai <bangtong-ai@users.noreply.github.com> Co-authored-by: OpenClaw Agent <openclaw-agent@users.noreply.github.com> Co-authored-by: OpenClaw Bot <bot@openclaw.local>
Summary
PR1added the detached-task lifecycle seam, but there was still no real core-owned way for an external executor to register itself against that seam.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
None.
Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Human Verification (required)
What I personally verified:
What I did not verify:
Authoritative validation on
mb-server, rerun after rebasing onto currentmain:OPENCLAW_TEST_PROFILE=serial OPENCLAW_TEST_SERIAL_GATEWAY=1 pnpm test -- src/tasks/task-executor.test.ts src/plugins/loader.test.ts src/plugins/runtime/runtime-tasks.test.ts src/tasks/detached-task-runtime.test.tspnpm tsgo:corepnpm tsgo:core:testKnown unrelated
mb-serverbaseline noise, not used as the authoritative gate for this narrow PR:pnpm tsgo:allstill fails in existing extension lanes on currentmainpnpm buildstill hits the knownruntime-postbuildbundled dependency staging issue onmb-serverRisks and Mitigations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoReview Conversations