Lobster: harden embedded runtime integration by mbelinky · Pull Request #61566 · openclaw/openclaw

mbelinky · 2026-04-06T00:02:47Z

Summary

Problem: the new in-process Lobster integration still had a few follow-up hardening gaps from Greptile review.
Why it matters: managed TaskFlow resume mode could accept incomplete resume params, the embedded runtime loader did unnecessary work on every tool call, and the duplicated TaskFlow test mock could drift.
What changed: managed resume mode now requires token and approve, the embedded runtime is memoized per runner and resolves the installed Lobster package root without fixed path arithmetic, and the shared TaskFlow test mock now lives in one helper.
What did NOT change (scope boundary): no changes in src/tasks/**, src/plugins/runtime/**, or Lobster core itself.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related Lobster: run workflows in process #61523
Related Lobster: add managed TaskFlow mode #61555
This PR fixes a bug or regression

Root Cause / Regression History (if applicable)

Root cause: the first two Lobster PRs landed the main transport and TaskFlow changes, but left a few correctness and maintainability gaps in the plugin seam.
Missing detection / guardrail: managed resume mode did not have coverage for missing token / approve, and the embedded runner had no regression test asserting runtime memoization.
Prior context (git blame, prior PR, issue, or refactor if known): this follows up on Greptile review feedback from Lobster: run workflows in process #61523 and Lobster: add managed TaskFlow mode #61555.
Why this regressed now: the earlier PRs were intentionally kept tight to land the transport and TaskFlow slices first.
If unknown, what was ruled out: N/A

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file:
- extensions/lobster/src/lobster-runner.test.ts
- extensions/lobster/src/lobster-tool.test.ts
- extensions/lobster/src/lobster-taskflow.test.ts
Scenario the test should lock in: managed resume mode rejects incomplete resume params, embedded runtime loading is memoized per runner, and TaskFlow tests share one mock helper.
Why this is the smallest reliable guardrail: the changed behavior is fully inside the Lobster plugin seam.
Existing test that already covers this (if any): earlier Lobster tests covered the happy paths, but not these follow-up edge cases.
If no new test is added, why not: N/A

User-visible / Behavior Changes

Managed Lobster resume calls now fail fast if token or approve is missing.
Repeated Lobster tool invocations on the same runner no longer reload the embedded runtime.

Diagram (if applicable)

Before:
[lobster tool] -> [validate some flow params] -> [load embedded runtime every call]

After:
[lobster tool] -> [validate complete managed resume params] -> [reuse embedded runtime per runner]

Security Impact (required)

New permissions/capabilities? (Yes/No) No
Secrets/tokens handling changed? (Yes/No) No
New/changed network calls? (Yes/No) No
Command/tool execution surface changed? (Yes/No) No
Data access scope changed? (Yes/No) No
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: macOS
Runtime/container: local Node/pnpm workspace in a clean worktree
Model/provider: N/A
Integration/channel (if any): Lobster bundled plugin
Relevant config (redacted): default local test config

Steps

Run targeted Lobster Vitest coverage for runner/tool/taskflow.
Run Oxlint on the touched Lobster files.
Run pnpm build.

Expected

Lobster hardening changes pass targeted plugin tests and lint.
Build should stay green unless current main already has an unrelated failure.

Actual

Targeted Lobster tests passed.
Oxlint passed.
pnpm build failed in untouched Pi runner code on current main:
- src/agents/pi-embedded-runner/model.ts(516,11): error TS2304: Cannot find name 'DEFAULT_CONTEXT_TOKENS'.
- src/agents/pi-embedded-runner/model.ts(521,11): error TS2304: Cannot find name 'DEFAULT_CONTEXT_TOKENS'.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

Verified scenarios:
- managed resume rejects missing token
- managed resume rejects missing approve
- embedded runtime loads once per runner
- shared TaskFlow mock still supports tool/taskflow tests
Edge cases checked:
- runtime loader failure still remains retryable because the memoized promise resets on rejection
What you did not verify:
- no full green build because current main fails in untouched Pi runner code

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? (Yes/No) Yes
Config/env changes? (Yes/No) No
Migration needed? (Yes/No) No
If yes, exact upgrade steps:

Risks and Mitigations

Risk:
- Memoizing the embedded runtime could retain a failed load forever.
- Mitigation:
  - the cached promise is cleared on rejection so later calls can retry.
Risk:
- Tightening managed resume validation could reject callers that relied on deferred runner errors.
- Mitigation:
  - this only affects invalid calls and now fails earlier with clearer error messages.

AI Assistance

AI-assisted: yes
Testing: targeted vitest, oxlint, build attempt

greptile-apps · 2026-04-06T00:09:19Z

Greptile Summary

This PR hardens the embedded Lobster runtime integration across three areas: managed TaskFlow resume mode now validates that token and approve are both present before entering the managed path (failing fast with actionable errors instead of propagating missing fields to the runner); the embedded runtime is memoized per runner instance via runtimePromise ??= ... with promise-reset on failure to allow retry; and the createFakeTaskFlow mock is extracted into a shared taskflow-test-helpers.ts to prevent drift between the tool and taskflow test suites.

Key changes:

lobster-runner.ts: runtime loading is memoized, token/approve are validated for the resume action, and the package-root resolver uses upward package.json traversal instead of fixed path offsets
lobster-tool.ts: parseResumeFlowParams now throws early if token or approve is missing in managed TaskFlow resume mode
taskflow-test-helpers.ts: new shared file consolidating the createFakeTaskFlow Vitest mock
Minor issues: the createLimitedSink error message uses "maxStdoutBytes" even for the stderr sink, flowExpectedRevision is typed as Type.Number() in the schema but validated as integer at runtime, and the timeout test relies on real wall-clock timing

Confidence Score: 4/5

PR is safe to merge; all new validation and memoization logic is correct and well-tested.

The core correctness changes — managed resume validation, runtime memoization, and the shared test helper extraction — are properly implemented and covered by new tests. The identified issues are cosmetic or schema-alignment concerns (misleading error message, Type.Number vs Type.Integer, real-clock timeout test, package.json name validation) that do not affect the guarded behavior introduced by this PR.

extensions/lobster/src/lobster-runner.ts (resolveInstalledLobsterRoot package name validation, createLimitedSink error message), extensions/lobster/src/lobster-tool.ts (flowExpectedRevision schema type)

Comments Outside Diff (4)

extensions/lobster/src/lobster-runner.ts, line 189 (link)

Misleading error message for stderr sink

The error always says maxStdoutBytes even when the overflowing sink is stderr (label === "stderr"), making it harder for callers to diagnose which stream was exhausted.

Prompt To Fix With AI

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-runner.ts
Line: 189

Comment:
**Misleading error message for stderr sink**

The error always says `maxStdoutBytes` even when the overflowing sink is stderr (`label === "stderr"`), making it harder for callers to diagnose which stream was exhausted.



How can I resolve this? If you propose a fix, please make it concise.

extensions/lobster/src/lobster-tool.ts, line 266-270 (link)

as cast is safe today but couples parseResumeFlowParams validation to runnerParams building implicitly

The cast works correctly because parseResumeFlowParams validates that params.token and params.approve are present before returning a non-null result, and runnerParams is built from the same params source at lines 218–227. However, TypeScript cannot verify this invariant structurally — the correctness relies on the ordering dependency between these two blocks.

If the runnerParams-building logic is ever decoupled from parseResumeFlowParams (e.g., moved into a factory or reordered), the cast would silently pass even with missing fields. Consider returning token and approve from ManagedFlowResumeParams and using those values to build the narrowed runnerParams, so TypeScript enforces the constraint structurally rather than through a cast.

Prompt To Fix With AI

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-tool.ts
Line: 266-270

Comment:
**`as` cast is safe today but couples `parseResumeFlowParams` validation to `runnerParams` building implicitly**

The cast works correctly because `parseResumeFlowParams` validates that `params.token` and `params.approve` are present before returning a non-null result, and `runnerParams` is built from the same `params` source at lines 218–227. However, TypeScript cannot verify this invariant structurally — the correctness relies on the ordering dependency between these two blocks.

If the `runnerParams`-building logic is ever decoupled from `parseResumeFlowParams` (e.g., moved into a factory or reordered), the cast would silently pass even with missing fields. Consider returning `token` and `approve` from `ManagedFlowResumeParams` and using those values to build the narrowed `runnerParams`, so TypeScript enforces the constraint structurally rather than through a cast.

How can I resolve this? If you propose a fix, please make it concise.

extensions/lobster/src/lobster-tool.ts, line 196 (link)

Schema type for flowExpectedRevision does not match the runtime integer check

The schema declares Type.Number() (which permits floats like 1.5), but readOptionalNumber enforces Number.isInteger at runtime. A provider that validates the schema before sending could pass a float, receiving an opaque runtime error rather than a schema-level rejection. Aligning the schema to Type.Integer() (available in @sinclair/typebox) surfaces the constraint to providers at the schema boundary.

Prompt To Fix With AI

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-tool.ts
Line: 196

Comment:
**Schema type for `flowExpectedRevision` does not match the runtime integer check**

The schema declares `Type.Number()` (which permits floats like `1.5`), but `readOptionalNumber` enforces `Number.isInteger` at runtime. A provider that validates the schema before sending could pass a float, receiving an opaque runtime error rather than a schema-level rejection. Aligning the schema to `Type.Integer()` (available in `@sinclair/typebox`) surfaces the constraint to providers at the schema boundary.



How can I resolve this? If you propose a fix, please make it concise.

extensions/lobster/src/lobster-runner.test.ts, line 295-325 (link)

Timeout test depends on real wall-clock timing and may flake under CI load

The test uses a 200 ms timeoutMs against a fake runtime that deliberately delays 500 ms. In heavily loaded CI environments the event loop can lag enough that the timing boundaries blur, causing intermittent failures. Using vi.useFakeTimers() with vi.advanceTimersByTimeAsync() would make this test deterministic regardless of host load and align with the repo's test performance guardrails.

Prompt To Fix With AI

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-runner.test.ts
Line: 295-325

Comment:
**Timeout test depends on real wall-clock timing and may flake under CI load**

The test uses a 200 ms `timeoutMs` against a fake runtime that deliberately delays 500 ms. In heavily loaded CI environments the event loop can lag enough that the timing boundaries blur, causing intermittent failures. Using `vi.useFakeTimers()` with `vi.advanceTimersByTimeAsync()` would make this test deterministic regardless of host load and align with the repo's test performance guardrails.

How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-runner.ts
Line: 189

Comment:
**Misleading error message for stderr sink**

The error always says `maxStdoutBytes` even when the overflowing sink is stderr (`label === "stderr"`), making it harder for callers to diagnose which stream was exhausted.

```suggestion
      callback(new Error(`lobster ${label} output limit exceeded`));
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-runner.ts
Line: 598-614

Comment:
**Package root resolver does not validate the found `package.json` belongs to `@clawdbot/lobster`**

`resolveInstalledLobsterRoot` walks upward from the resolved Lobster entry file and stops at the first `package.json` it encounters. In unusual layouts (symlinked `node_modules`, custom workspace hoisting, or monorepo overrides), the first `package.json` found could belong to a parent scope, causing incorrect module paths to be used downstream in `loadEmbeddedToolRuntimeFromPackage`.

Adding a name check after reading the file would make this robust against edge-case misresolves:

```ts
const { readFileSync } = await import("node:fs");
// inside the loop, after existsSync:
const pkg = JSON.parse(readFileSync(packageJsonPath, "utf8")) as { name?: string };
if (pkg.name === "@clawdbot/lobster") {
  return currentDir;
}
// otherwise continue walking upward
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-tool.ts
Line: 266-270

Comment:
**`as` cast is safe today but couples `parseResumeFlowParams` validation to `runnerParams` building implicitly**

The cast works correctly because `parseResumeFlowParams` validates that `params.token` and `params.approve` are present before returning a non-null result, and `runnerParams` is built from the same `params` source at lines 218–227. However, TypeScript cannot verify this invariant structurally — the correctness relies on the ordering dependency between these two blocks.

If the `runnerParams`-building logic is ever decoupled from `parseResumeFlowParams` (e.g., moved into a factory or reordered), the cast would silently pass even with missing fields. Consider returning `token` and `approve` from `ManagedFlowResumeParams` and using those values to build the narrowed `runnerParams`, so TypeScript enforces the constraint structurally rather than through a cast.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-tool.ts
Line: 196

Comment:
**Schema type for `flowExpectedRevision` does not match the runtime integer check**

The schema declares `Type.Number()` (which permits floats like `1.5`), but `readOptionalNumber` enforces `Number.isInteger` at runtime. A provider that validates the schema before sending could pass a float, receiving an opaque runtime error rather than a schema-level rejection. Aligning the schema to `Type.Integer()` (available in `@sinclair/typebox`) surfaces the constraint to providers at the schema boundary.

```suggestion
      flowExpectedRevision: Type.Optional(Type.Integer()),
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/lobster/src/lobster-runner.test.ts
Line: 295-325

Comment:
**Timeout test depends on real wall-clock timing and may flake under CI load**

The test uses a 200 ms `timeoutMs` against a fake runtime that deliberately delays 500 ms. In heavily loaded CI environments the event loop can lag enough that the timing boundaries blur, causing intermittent failures. Using `vi.useFakeTimers()` with `vi.advanceTimersByTimeAsync()` would make this test deterministic regardless of host load and align with the repo's test performance guardrails.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Lobster: harden embedded runtime integra..." | Re-trigger Greptile}

greptile-apps · 2026-04-06T00:09:26Z

 function resolveInstalledLobsterRoot() {
  const require = createRequire(import.meta.url);
  const sdkEntry = require.resolve("@clawdbot/lobster");
-  return path.resolve(path.dirname(sdkEntry), "../../..");
+  let currentDir = path.dirname(sdkEntry);
+
+  while (true) {
+    const packageJsonPath = path.join(currentDir, "package.json");
+    if (existsSync(packageJsonPath)) {
+      return currentDir;
+    }
+    const parentDir = path.dirname(currentDir);
+    if (parentDir === currentDir) {
+      throw new Error("Unable to resolve the installed @clawdbot/lobster package root");
+    }
+    currentDir = parentDir;
+  }
 }


Package root resolver does not validate the found package.json belongs to @clawdbot/lobster

resolveInstalledLobsterRoot walks upward from the resolved Lobster entry file and stops at the first package.json it encounters. In unusual layouts (symlinked node_modules, custom workspace hoisting, or monorepo overrides), the first package.json found could belong to a parent scope, causing incorrect module paths to be used downstream in loadEmbeddedToolRuntimeFromPackage.

Adding a name check after reading the file would make this robust against edge-case misresolves:

const { readFileSync } = await import("node:fs"); // inside the loop, after existsSync: const pkg = JSON.parse(readFileSync(packageJsonPath, "utf8")) as { name?: string }; if (pkg.name === "@clawdbot/lobster") { return currentDir; } // otherwise continue walking upward

Prompt To Fix With AI

This is a comment left during a code review. Path: extensions/lobster/src/lobster-runner.ts Line: 598-614 Comment: **Package root resolver does not validate the found `package.json` belongs to `@clawdbot/lobster`** `resolveInstalledLobsterRoot` walks upward from the resolved Lobster entry file and stops at the first `package.json` it encounters. In unusual layouts (symlinked `node_modules`, custom workspace hoisting, or monorepo overrides), the first `package.json` found could belong to a parent scope, causing incorrect module paths to be used downstream in `loadEmbeddedToolRuntimeFromPackage`. Adding a name check after reading the file would make this robust against edge-case misresolves: ```ts const { readFileSync } = await import("node:fs"); // inside the loop, after existsSync: const pkg = JSON.parse(readFileSync(packageJsonPath, "utf8")) as { name?: string }; if (pkg.name === "@clawdbot/lobster") { return currentDir; } // otherwise continue walking upward ``` How can I resolve this? If you propose a fix, please make it concise.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b9f58db86

ℹ️ 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".

chatgpt-codex-connector · 2026-04-06T01:42:01Z

 - Providers/CLI: remove bundled CLI text-provider backends and the `agents.defaults.cliBackends` surface, while keeping ACP harness sessions and Gemini media understanding on the native bundled providers.
 - Matrix/exec approvals: clarify unavailable-approval replies so Matrix no longer claims chat approvals are unsupported when native exec approvals are merely unconfigured. (#61424) Thanks @gumadeiras.
 - Docs/IRC: replace public IRC hostname examples with `irc.example.com` and recommend private servers for bot coordination while listing common public networks for intentional use.
+<<<<<<< HEAD


Remove unresolved merge marker from changelog

The new <<<<<<< HEAD line is a merge-conflict artifact, not content, and it will leak into rendered release notes/changelog output as broken text. This can also break any tooling that expects clean Markdown changelog entries, so the marker should be removed before landing.

Useful? React with 👍 / 👎.

mbelinky · 2026-04-06T01:52:49Z

Merged via squash.

Prepared head SHA: a6f48309fd8278af993bcac80c212736dab065c7
Merge commit: b167df78aa17ad52c0deed80a62dd70906d09fb7

Thanks @mbelinky!

@mbelinky