feat(core)!: drop unused module-lifecycle public types; consolidate wire + session-file utilities#930
Conversation
The worker's session loop invokes `initModuleWorkspace`, `onSessionStart`,
and `collectModuleData` from `agent-worker/src/modules/lifecycle.ts`. Each
call resolves `moduleRegistry.getWorkerModules()`, which by registration
intent surfaces every module that has `onBeforeResponse` in its prototype.
In practice, the only modules ever registered are `ModelProviderModule`s
(Claude OAuth, ChatGPT OAuth, Bedrock, Gemini CLI, the API-key catalog
module) — all inheriting from `BaseModule`, whose lifecycle hooks are
no-ops. Three `for` loops over no-op methods run on every session start,
plus three `await import("../modules/lifecycle")` dynamic imports that
violate the static-import rule documented in CLAUDE.md.
Delete the call sites in `openclaw/worker.ts` and the whole
`agent-worker/src/modules/` directory. The `WorkerModule` /
`BaseModule` interface surface in `@lobu/core` and
`gateway/modules/module-system.ts` stays — it's public-API, and removing
it is a separate, larger decision (see REPORT.md "Do not do"). Re-adding
the call sites later, when an actual module needs them, is ~20 LOC.
Validation: `make typecheck` clean from worktree root (server + owletto).
`bunx tsc --noEmit` clean inside `packages/agent-worker`.
`MessagePayload` is the gateway↔worker wire contract: produced by `MessageConsumer` and `EmbeddedDeploymentManager.dispatch*`, consumed by `GatewayClient.handleThreadMessage` / `handleExecJob` over SSE. Both sides need to agree on the shape, but the type was declared twice and had already drifted: the worker's copy in `agent-worker/src/gateway/ types.ts` was missing `organizationId`, `networkConfig`, `egressConfig`, `mcpConfig`, `nixConfig`, and `preApprovedTools` (all present on the gateway side). The worker's zod schema was patched with `.passthrough()` to keep the extra fields from being stripped at parse time (PR #871 regression), but the static type silently lied — TS would have happily accepted reads of e.g. `payload.preApprovedTools` as `undefined` even when the gateway always populates it. Move both `MessagePayload` and `JobType` (plus the worker-side `QueuedMessage` envelope) into `packages/core/src/worker/wire.ts` and re-export them from `@lobu/core`. The worker and gateway both import from there; `queue-producer.ts` and `gateway/types.ts` keep a re-export so the existing `from "./types"` / `from "../infrastructure/queue/ queue-producer"` paths continue to resolve without churn. Type unification: - `agentOptions: AgentOptions` (was `Record<string, any>` on the gateway, `AgentOptions` on the worker). The worker reads `.model`, `.allowedTools`, `.disallowedTools`, `.timeoutMinutes` directly, so `AgentOptions` is the honest shape. `Record<string, any>` is assignable into it via the `[key: string]: unknown` index signature. - `platformMetadata: Record<string, unknown>` (was `Record<string, any>` on the gateway, a richer named interface on the worker). The unknown flavour forces three new `typeof === "string"` guards inside `base-deployment-manager.ts` where the env-var builder reads `originalMessageTs`, `botResponseTs`, and `teamId` off the bag. - `teamId?: string` (was `string` on the gateway, `string | undefined` on the worker). The worker SSE zod schema marks it `.optional()`, and the worker has explicit fallback logic (`payload.teamId ?? platformMetadata.teamId`). The gateway-side `buildMessagePayload` always sets a non-empty string, so concrete callers are unaffected. Validation: `make build-packages` clean. `make typecheck` clean (server + owletto). `bunx tsc --noEmit` clean inside `packages/agent-worker`.
Two HTTP surfaces parse the worker's `.openclaw/session.jsonl`:
- the worker's own `/session/messages` and `/session/stats` (rooted at
`WORKSPACE_DIR`), and
- the gateway's `/session/messages` and `/session/stats` proxy fallback
(rooted at `workspaces/<agentId>`, used when the worker is offline).
The gateway proxies to the worker when the worker is online and falls
back to its own copy otherwise — so the two parsers MUST agree on the
JSONL line shape. They had drifted. The worker used `safeJsonParse` (a
core util that debug-logs malformed lines); the gateway inlined a bare
`try { JSON.parse } catch {}`. The worker carried two extra
`SessionEntry` fields (`tokensBefore`, `firstKeptEntryId`) that no
parser actually read. The display-projection logic in `entryToMessage`
was duplicated character-for-character.
Move the shared parts (`SessionEntry`, `ParsedMessage`,
`parseSessionEntries`, `entryToMessage`) into
`packages/core/src/utils/session-file.ts` and have both call sites
import from `@lobu/core`. `safeJsonParse` wins as the canonical line
parser — it's already in core, and the debug-log on malformed lines is
strictly more useful than silently swallowing them.
What does NOT move: `findSessionFile`. The two call sites have
intentionally different path policies (worker scans its own
`WORKSPACE_DIR` one level deep; gateway scans `workspaces/<agentId>`
three levels deep with a `SAFE_AGENT_ID` regex guard against path
traversal). Collapsing them would force one side to inherit the other's
policy — not a behaviour change to make silently.
Dropped from the canonical `SessionEntry`: `tokensBefore` and
`firstKeptEntryId`. They were declared on the worker's local interface
but nothing reads them — only `agent-worker/src/__tests__/
memory-flush-runtime.test.ts` mentions them in a fixture. Reintroduce
when an actual consumer needs them.
Validation: `make build-packages` clean. `make typecheck` clean
(server + owletto). `bunx tsc --noEmit` clean inside `packages/agent-worker`.
…directly Spike 8a5f686 hoisted `MessagePayload`/`JobType`/`QueuedMessage` into `@lobu/core` but left three re-export shims so existing import paths kept resolving. The user's global CLAUDE.md is explicit ("Do NOT add backwards- compatibility shims, deprecated annotations, or fallback aliases"), and codex flagged the leftover aliases on review. Delete them: - `packages/server/src/gateway/infrastructure/queue/queue-producer.ts` drop `export type { JobType, MessagePayload } from "@lobu/core"`. - `packages/server/src/gateway/orchestration/base-deployment-manager.ts` drop `import type { MessagePayload } from "../infrastructure/queue/ queue-producer.js"` + the trailing `export type { MessagePayload }` alias; pull the type from `@lobu/core` instead. - `packages/agent-worker/src/gateway/types.ts` shrinks to its only remaining concern: the worker-only `ResponseData = ThreadResponsePayload & { originalMessageId: string }`. The wire-contract types are gone. Every importer migrated to `import type { MessagePayload, ... } from "@lobu/core"`: - worker: `sse-client.ts`, `message-batcher.ts`, `__tests__/message- batcher.test.ts`. - gateway: `orchestration/{impl/embedded-deployment,message-consumer}.ts`, `services/platform-helpers.ts`, three orchestration tests (`embedded-deployment.test.ts`, `orchestration-harden.test.ts`, `base-deployment-grants.test.ts`). Validation: `make build-packages` clean. `make typecheck` clean (server + owletto). `bunx tsc --noEmit` clean inside `packages/agent-worker`.
Spike 83a2cf4 deleted the worker call sites for `initModuleWorkspace`, `onSessionStart`, and `collectModuleData` but left the interfaces and no-op base implementations they targeted. Codex flagged the dead surfaces on review; the rule from CLAUDE.md is "remove the old code entirely instead of keeping it alongside the new code." Verified zero callers anywhere in the monorepo before deleting: for sym in onSessionStart onSessionEnd initWorkspace \ onBeforeResponse generateActionButtons handleAction \ ActionButton ModuleSessionContext DispatcherModule \ DispatcherContext getContainerAddress getWorkerModules; do grep -rn "\\b$sym\\b" packages/ scripts/ examples/ config/ done After this commit the only matches are `initWorkspaceProvider` in `packages/server/src/workspace/` (different concept, unrelated). The sibling `WorkerContext` symbol in `packages/server/src/gateway/routes/ internal/*.ts` is a Hono variable-context type; the deleted one was the worker-module hook context and is removed only from `packages/core/src/ modules.ts`. `BaseProviderModule` (the only `BaseModule` subclass — Claude / ChatGPT / Bedrock / Gemini / API-key catalog) was verified to NOT override any of the deleted methods (it overrides `isEnabled`, `buildEnvVars`, `getModelOptions`, and the provider-specific surface only), so trimming `BaseModule` is type-safe. Deletions: - `packages/core/src/modules.ts`: `WorkerContext`, `WorkerModule`, `ActionButton`, `ModuleSessionContext` interfaces; `IModuleRegistry.getWorkerModules` + `ModuleRegistry.getWorkerModules` method. - `packages/core/src/index.ts`: `ActionButton` and `ModuleSessionContext` type re-exports. - `packages/server/src/gateway/modules/module-system.ts`: `DispatcherContext`, `DispatcherModule` interfaces; the `WorkerModule<TModuleData>` and `DispatcherModule<TModuleData>` clauses on `BaseModule implements`; the `initWorkspace`, `onSessionStart`, `onSessionEnd`, `onBeforeResponse`, `generateActionButtons`, `handleAction`, `getContainerAddress` methods on `BaseModule`; and the `getContainerAddress(): string` member on the `OrchestratorModule` interface. Validation: `make typecheck` clean (server + owletto). `bunx tsc --noEmit` clean inside `packages/agent-worker`. `make build-packages` clean.
`ModuleRegistry.registerAvailableModules(modulePackages?: string[])` exists to dynamically `await import(packageName)` plugin packages at startup. Both call sites pass no args — the loop body is unreachable: packages/agent-worker/src/index.ts:42 → `registerAvailableModules()` packages/server/src/gateway/services/core-services.ts:844 → same Verified monorepo-wide: grep -rn "registerAvailableModules" packages/ scripts/ examples/ config/ Two hits, both no-arg. The method is also the only dynamic `import()` in `@lobu/core`, which conflicts with the project memory rule "No dynamic imports — always static `import`" (`feedback_no_dynamic_imports`). Deletions: - `packages/core/src/modules.ts`: `IModuleRegistry.registerAvailableModules` signature and `ModuleRegistry.registerAvailableModules` method (~40 lines including the `@example` block). - `packages/agent-worker/src/index.ts:42`: drop the no-arg call. - `packages/server/src/gateway/services/core-services.ts:844`: drop the no-arg call; the comment above the remaining `initAll()` is updated to read "Initialize all registered modules" (registration now happens at the explicit `moduleRegistry.register(...)` sites earlier in `core-services.ts`). Behaviour: zero change. Both code paths previously entered the function, iterated an empty array, and returned. Removing them removes the only dynamic-import in `@lobu/core` and drops one no-op step out of every gateway/worker boot. Validation: `make typecheck` clean (server + owletto). `bunx tsc --noEmit` clean in `packages/agent-worker`. `make build-packages` clean.
…urce
Two type duplicates that codex flagged on review:
1. `ModelOption` was declared in BOTH `packages/core/src/api-types.ts:38`
AND `packages/server/src/gateway/modules/module-system.ts:5`. Same
shape (`{label: string; value: string}`), declared twice. Every
provider module imported the server-local copy; the core export was
carried only by the public SDK surface.
2. `PrefillSkill` was declared in BOTH `packages/core/src/api-types.ts:94`
AND `packages/server/src/gateway/auth/settings/token-service.ts:4`.
Same shape. The token-service file also carried a local
`PrefillMcpServer` interface that was structurally identical to core's
`PrefillMcp` except for a narrowed `type: "sse" | "streamable-http" |
"stdio"` vs core's `type?: string`. The narrowed type is declaration-
only and never narrowed at runtime, so widening to core's shape is
behaviour-neutral; the token route emits whatever string the operator
passed in either way.
`@lobu/core` wins (it's the public API surface; deleting it would be a
breaking change for SDK consumers). The server-local duplicates go.
Migrated imports — every consumer now pulls `ModelOption` / `PrefillMcp`
/ `PrefillSkill` from `@lobu/core` directly:
- `packages/server/src/gateway/auth/gemini/cli-module.ts`
- `packages/server/src/gateway/auth/api-key-provider-module.ts`
- `packages/server/src/gateway/auth/claude/oauth-module.ts`
- `packages/server/src/gateway/auth/bedrock/provider-module.ts`
- `packages/server/src/gateway/auth/chatgpt/chatgpt-oauth-module.ts`
- `packages/server/src/gateway/auth/provider-model-options.ts`
- `packages/server/src/gateway/auth/settings/token-service.ts`
(`prefillMcpServers?: PrefillMcp[]` — field name stays, element type
pulled from core).
- `packages/server/src/gateway/routes/public/agent-config.ts`
- `packages/server/src/gateway/modules/module-system.ts`
(`ModelOption` now re-imported from `@lobu/core` for the
`OrchestratorModule` / `ModelProviderModule` interfaces in this file).
Verification:
grep -rn "^export interface ModelOption\\b\\|^interface ModelOption\\b" \
"^export interface PrefillSkill\\b\\|^interface PrefillSkill\\b" \
"^export interface PrefillMcp\\b\\|^interface PrefillMcpServer\\b" \
packages/ 2>/dev/null | grep -v "/dist/"
returns one match per symbol — all under `packages/core/src/api-types.ts`.
Validation: `make typecheck` clean (server + owletto). `bunx tsc --noEmit`
clean inside `packages/agent-worker`. `make build-packages` clean.
Two trivial leftovers from rounds 1–3: 1. The `<TModuleData>` type parameter on `ModuleInterface` (core), `OrchestratorModule` (server module-system), and `BaseModule` (server module-system) was a placeholder for the deleted `WorkerModule` surface (`onBeforeResponse(): Promise<TModuleData | null>`). With that surface gone in spike 9de939d the parameter has no semantic use — the core declaration already had it underscored (`_TModuleData`), and every instantiation site (`ModelProviderModule extends OrchestratorModule`, `OrchestratorModule[]`, `(m): m is OrchestratorModule` guard) was passing no arg. Removed the type parameter from all three declarations. 2. `packages/agent-worker/src/gateway/types.ts:5` carried a stale doc comment pointing imports at `@lobu/core/worker/wire`. That subpath isn't declared in `packages/core/package.json`'s `exports` field — only `.` is exported — so the path doesn't resolve. Every actual import in the tree already uses `@lobu/core` (the package root). Comment updated to match. No code import paths changed. Verification (`grep -rn`, excluding `/dist/`): - `BaseModule<` / `OrchestratorModule<` / `ModuleInterface<` → zero matches. - `TModuleData` → zero matches. - `@lobu/core/worker/wire` → zero matches. `make typecheck` clean (server + owletto). `bunx tsc --noEmit` clean in `packages/agent-worker`. `make build-packages` clean.
…omment Two trivial leftovers from spike 9de939d (the module-lifecycle deletion) and spike 9a401f5 (the session-file hoist): 1. `moduleData` was wire-plumbing for the deleted `collectModuleData()` path. The collector was removed in 9de939d, so nothing calls `setModuleData` anymore — `this.moduleData` is always `undefined`, and the two `sendResponse` sites that read it serialize `undefined` into every response. Verified monorepo-wide: grep -rn "\\bmoduleData\\b\\|setModuleData" packages/ scripts/ \ examples/ config/ Five hits, all on the write side (declaration / setter / two reads of the always-undefined field / the wire-type slot). Zero readers on the gateway end. Deleted: - `packages/core/src/types.ts`: `moduleData?` field on `ThreadResponsePayload`. - `packages/core/src/worker/transport.ts`: `setModuleData` method on the `WorkerTransport` interface. - `packages/agent-worker/src/gateway/gateway-integration.ts`: the `private moduleData?` field, the `setModuleData` method, and the two `moduleData: this.moduleData` lines in `sendStreamDelta` / `signalCompletion`. 2. `packages/server/src/gateway/routes/public/agent-history.ts:75` pointed at `@lobu/core/utils/session-file`. That subpath isn't declared in `packages/core/package.json`'s `exports` — only `.` is exported. Every actual import in the tree uses `@lobu/core`. Comment updated to match reality. `make typecheck` clean (server + owletto). `bunx tsc --noEmit` clean in `packages/agent-worker`. `make build-packages` clean. Monorepo grep for `moduleData` / `setModuleData` / `@lobu/core/utils/session-file` returns zero matches.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR refactors the module system architecture by centralizing worker↔gateway wire contract types into ChangesModule System and Wire Contract Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/core/src/utils/session-file.ts`:
- Around line 120-128: The model_change branch currently builds content and
model strings directly from entry.provider and entry.modelId, which can produce
"undefined/..." values; update the logic in the model_change handler (the block
checking if (entry.type === "model_change") that returns id, type, content,
model, timestamp, isVerbose) to defensively handle missing fields: derive safe
values for provider and modelId (e.g., const providerSafe = entry.provider ?? ''
and const modelIdSafe = entry.modelId ?? ''), then build content and model using
only the defined parts (omit the slash if one side is empty or return a sensible
fallback like 'unknown' or an empty string) so content and model never contain
the literal "undefined". Ensure timestamp and isVerbose behavior is unchanged.
🪄 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: 43147c05-a060-400c-9831-5021a4d79e08
📒 Files selected for processing (34)
packages/agent-worker/src/__tests__/message-batcher.test.tspackages/agent-worker/src/gateway/gateway-integration.tspackages/agent-worker/src/gateway/message-batcher.tspackages/agent-worker/src/gateway/sse-client.tspackages/agent-worker/src/gateway/types.tspackages/agent-worker/src/index.tspackages/agent-worker/src/modules/lifecycle.tspackages/agent-worker/src/openclaw/worker.tspackages/agent-worker/src/server.tspackages/core/src/index.tspackages/core/src/modules.tspackages/core/src/types.tspackages/core/src/utils/session-file.tspackages/core/src/worker/transport.tspackages/core/src/worker/wire.tspackages/server/src/gateway/__tests__/base-deployment-grants.test.tspackages/server/src/gateway/__tests__/embedded-deployment.test.tspackages/server/src/gateway/__tests__/orchestration-harden.test.tspackages/server/src/gateway/auth/api-key-provider-module.tspackages/server/src/gateway/auth/bedrock/provider-module.tspackages/server/src/gateway/auth/chatgpt/chatgpt-oauth-module.tspackages/server/src/gateway/auth/claude/oauth-module.tspackages/server/src/gateway/auth/gemini/cli-module.tspackages/server/src/gateway/auth/provider-model-options.tspackages/server/src/gateway/auth/settings/token-service.tspackages/server/src/gateway/infrastructure/queue/queue-producer.tspackages/server/src/gateway/modules/module-system.tspackages/server/src/gateway/orchestration/base-deployment-manager.tspackages/server/src/gateway/orchestration/impl/embedded-deployment.tspackages/server/src/gateway/orchestration/message-consumer.tspackages/server/src/gateway/routes/public/agent-config.tspackages/server/src/gateway/routes/public/agent-history.tspackages/server/src/gateway/services/core-services.tspackages/server/src/gateway/services/platform-helpers.ts
💤 Files with no reviewable changes (6)
- packages/agent-worker/src/index.ts
- packages/agent-worker/src/modules/lifecycle.ts
- packages/agent-worker/src/gateway/gateway-integration.ts
- packages/agent-worker/src/openclaw/worker.ts
- packages/core/src/worker/transport.ts
- packages/core/src/types.ts
| if (entry.type === "model_change") { | ||
| return { | ||
| id: entry.id, | ||
| type: "model_change", | ||
| content: `${entry.provider}/${entry.modelId}`, | ||
| model: `${entry.provider}/${entry.modelId}`, | ||
| timestamp: entry.timestamp, | ||
| isVerbose: true, | ||
| }; |
There was a problem hiding this comment.
Handle potentially undefined provider or modelId in model_change entries.
If entry.provider or entry.modelId is undefined, the content/model strings become "undefined/modelId" or "provider/undefined", which would surface to API consumers.
🛡️ Proposed defensive handling
if (entry.type === "model_change") {
+ const model = entry.provider && entry.modelId
+ ? `${entry.provider}/${entry.modelId}`
+ : entry.provider || entry.modelId || "unknown";
return {
id: entry.id,
type: "model_change",
- content: `${entry.provider}/${entry.modelId}`,
- model: `${entry.provider}/${entry.modelId}`,
+ content: model,
+ model,
timestamp: entry.timestamp,
isVerbose: true,
};
}🤖 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 `@packages/core/src/utils/session-file.ts` around lines 120 - 128, The
model_change branch currently builds content and model strings directly from
entry.provider and entry.modelId, which can produce "undefined/..." values;
update the logic in the model_change handler (the block checking if (entry.type
=== "model_change") that returns id, type, content, model, timestamp, isVerbose)
to defensively handle missing fields: derive safe values for provider and
modelId (e.g., const providerSafe = entry.provider ?? '' and const modelIdSafe =
entry.modelId ?? ''), then build content and model using only the defined parts
(omit the slash if one side is empty or return a sensible fallback like
'unknown' or an empty string) so content and model never contain the literal
"undefined". Ensure timestamp and isVerbose behavior is unchanged.
Summary
Multi-round simplification on
packages/core+packages/server+packages/agent-worker. Cumulative ~385 LOC net deletion across 34 files. Includes a breaking change for@lobu/core: removes public types that were a speculative future-extensibility seam with zero internal consumers.Breaking change
Drops the following types from
@lobu/core's publicindex.ts:ActionButton(was previously exported)ModuleSessionContext(was previously exported)WorkerModule,WorkerContext,DispatcherModule,DispatcherContext, the no-opBaseModulelifecycle methods (initWorkspace/onSessionStart/onSessionEnd/onBeforeResponse),generateActionButtons+handleActiondispatcher surface,getContainerAddress,ModuleRegistry.getWorkerModules.Internal consumers: zero. Agent verified across all monorepo packages including
@lobu/openclaw-plugin. The dispatcher had no real handlers wired up; the lifecycle hooks had no real subclasses overriding them (only no-opBaseModuledefaults).External consumers (npm): speculative. Last touched only by prior cleanup PR #820. Worth flagging in release notes; release-please should bump
@lobu/coremajor (v7.x → v8.0).Other simplifications
MessagePayload+JobType+QueuedMessageinto@lobu/core/worker/wire; delete 3 re-export shim files; migrate 10 importers to pull from@lobu/coredirectly.session.jsonlparser into@lobu/core/utils/session-file; collapse worker + gateway parsers into one canonical impl (debug-log on malformed JSONL on gateway side — flagged in commit body).findSessionFiledeliberately kept split — different path policies (1-level vs 3-level depth + traversal guard).registerAvailableModules()(both call sites passed no args).ModelOption+PrefillSkill/PrefillMcpServerto@lobu/core; update 9 importers.moduleData/setModuleDatawire plumbing (5 write-only sites, zero readers after the lifecycle deletion).TModuleDatageneric fromModuleInterface/OrchestratorModule/BaseModule.Process
Built across 5 rounds of spike commits driven by
codex exec --config model_reasoning_effort=highreview at each round. Codex initial verdicts: NOT ENOUGH (4 rounds). Final verdict: ENOUGH, ship as breaking PR. Allwip(spike):commit history preserved on branch — squash on merge.Test plan
make typecheckcleanmake build-packagescleanfeat!:title and proposes@lobu/corev8.0.0 in the next release PRSummary by CodeRabbit
Refactor
Chores