feat(plugins): add session followup turn API and gateway-restart extension#60951
feat(plugins): add session followup turn API and gateway-restart extension#60951corbin-breton wants to merge 4 commits into
Conversation
Greptile SummaryThis PR adds a The core infrastructure changes (
Additionally, Confidence Score: 2/5Not safe to merge — two logic bugs in the gateway-restart extension can cause spurious session resumes and unrecoverable failures. The core API additions are solid. The bugs are in the gateway-restart extension: (1) the restart marker is written before pre-commands execute, so a failed command leaves a stale marker that triggers a spurious followup on the next real restart; (2) the marker is deleted before confirming the followup was enqueued, making failures non-retryable. extensions/gateway-restart/index.ts — both the execute tool handler (marker-before-commands ordering) and the watcher service start() (delete-before-confirm ordering) need fixes before merging. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/gateway-restart/index.ts
Line: 138-149
Comment:
**Stale restart marker left on disk when pre-command fails**
The restart marker is written to disk at line 138, and only then are the pre-commands executed via `execSync` (lines 142–147). `execSync` throws (synchronously) if a command exits with a non-zero status, propagating the error out of `execute`. Because `setTimeout(() => process.exit(0), 500)` is placed after the loop (line 149), the gateway never exits when a command fails — but the marker file is already on disk.
On the next gateway restart (for any reason), `gateway-restart-watcher` will find the stale marker, consume it, and inject a spurious followup turn into the original session.
The marker file write should be deferred until after all pre-commands have succeeded:
```ts
// Execute pre-commands first; abort with no marker if any fail
const commandOutputs: string[] = [];
for (const command of commands) {
const output = execSync(command, {
encoding: "utf8",
stdio: ["ignore", "pipe", "pipe"],
});
commandOutputs.push(output);
}
// Only write the marker once we know we're committed to restarting
const marker: RestartMarker = { ... };
fs.mkdirSync(path.dirname(markerPath), { recursive: true });
fs.writeFileSync(markerPath, `${JSON.stringify(marker, null, 2)}\n`, "utf8");
setTimeout(() => process.exit(0), 500);
```
Alternatively, wrap `execSync` in a try/catch that removes the marker file on failure.
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/gateway-restart/index.ts
Line: 200-213
Comment:
**Marker deleted before followup is confirmed enqueued**
The marker is unconditionally deleted (`fs.rmSync`) before `enqueueFollowupTurn` is awaited. While `enqueueFollowupTurn` wraps its internals in try/catch and always resolves to a boolean, if the function returns `false` (session not found, session store unavailable after restart, etc.), the marker is already gone and there is no way to retry. The warning logged at line 220 is the only signal — the session resume opportunity is permanently lost.
Consider only deleting the marker after a successful enqueue:
```ts
const enqueued = await followupRuntime.enqueueFollowupTurn({ ... });
if (enqueued) {
fs.rmSync(markerPath, { force: true });
ctx.logger.info(`[gateway-restart] Enqueued followup turn for session ${marker.sessionKey}.`);
} else {
ctx.logger.warn(`[gateway-restart] Failed to enqueue followup turn for session ${marker.sessionKey}. Marker preserved for manual inspection.`);
}
```
If idempotent retries are intentionally not supported, at minimum document that the marker is consumed regardless of enqueue outcome.
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/plugins/runtime/runtime-followup.ts
Line: 43-44
Comment:
**`rememberFollowupDrainCallback` unconditionally overwrites the active runner on hot sessions**
`rememberFollowupDrainCallback(params.sessionKey, runner)` always overwrites the stored drain callback with the freshly constructed noop-typing runner. When this is called for a session that is **currently being drained** by a channel-specific runner, the active `scheduleFollowupDrain` loop uses a local `effectiveRunFollowup` variable — but when the drain's `finally` block fires and calls `scheduleFollowupDrain(key, effectiveRunFollowup)` to restart, that function re-reads `FOLLOWUP_RUN_CALLBACKS.get(key)` (drain.ts line 86) and **picks up the noop runner** instead of the original.
The result is that subsequent turns in an active hot session would be executed with the noop typing controller, bypassing any channel-level typing/delivery logic.
This path is unlikely in the gateway-restart use case (the session is cold after a restart), but the API is general-purpose and will be called by other plugins. Consider guarding against overwriting an existing active callback:
```ts
if (!FOLLOWUP_RUN_CALLBACKS.has(params.sessionKey)) {
rememberFollowupDrainCallback(params.sessionKey, runner);
}
kickFollowupDrainIfIdle(params.sessionKey);
```
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/gateway-restart/index.ts
Line: 62-64
Comment:
**Module-level mutable state creates re-registration hazard**
`followupRuntime`, `registeredStateDir`, and `registeredMarkerFileName` are module-level `let` variables that are overwritten every time `register()` is called. If the plugin is ever unloaded and re-registered (e.g., after a config reload), a second `register()` call silently replaces them.
Prefer closing over them inside `register` and passing them directly into the service `start()` callback, which already receives `ctx.stateDir`:
```ts
register(api) {
const config = resolveConfig(api.pluginConfig);
const localFollowupRuntime = api.runtime.followup;
api.registerService({
id: "gateway-restart-watcher",
async start(ctx) {
const markerPath = getMarkerPath(ctx.stateDir, config.markerFileName);
// use localFollowupRuntime here
},
});
}
```
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/gateway-restart/index.ts
Line: 44-60
Comment:
**`FollowupRuntime` type is locally re-declared rather than derived from the SDK**
The `FollowupRuntime` type duplicates the shape of `PluginRuntimeCore["followup"]` verbatim. If the SDK type evolves (e.g., new optional parameters are added to `enqueueFollowupTurn`), this local copy will silently drift. Since `api.runtime.followup` is already typed on the `api` object provided by the framework, the module-level variable can carry the inferred type directly, avoiding the duplication.
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/gateway-restart/package.json
Line: 7-11
Comment:
**Missing `openclaw.install.npmSpec` in package metadata**
Per the repo's CLAUDE.md bundled plugin naming rules, `openclaw.install.npmSpec` should equal the package name. The `openclaw` block here only has `extensions` — `install.npmSpec` is absent.
```suggestion
"openclaw": {
"extensions": [
"./index.ts"
],
"install": {
"npmSpec": "@openclaw/gateway-restart-plugin"
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(plugins): add session followup turn..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fs.writeFileSync(markerPath, `${JSON.stringify(marker, null, 2)}\n`, "utf8"); | ||
|
|
||
| const commandOutputs: string[] = []; | ||
| for (const command of commands) { | ||
| const output = execSync(command, { | ||
| encoding: "utf8", | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); | ||
| commandOutputs.push(output); | ||
| } | ||
|
|
||
| setTimeout(() => process.exit(0), 500); |
There was a problem hiding this comment.
The restart marker is written to disk at line 138, and only then are the pre-commands executed via execSync (lines 142–147). execSync throws (synchronously) if a command exits with a non-zero status, propagating the error out of execute. Because setTimeout(() => process.exit(0), 500) is placed after the loop (line 149), the gateway never exits when a command fails — but the marker file is already on disk.
On the next gateway restart (for any reason), gateway-restart-watcher will find the stale marker, consume it, and inject a spurious followup turn into the original session.
The marker file write should be deferred until after all pre-commands have succeeded:
// Execute pre-commands first; abort with no marker if any fail
const commandOutputs: string[] = [];
for (const command of commands) {
const output = execSync(command, {
encoding: "utf8",
stdio: ["ignore", "pipe", "pipe"],
});
commandOutputs.push(output);
}
// Only write the marker once we know we're committed to restarting
const marker: RestartMarker = { ... };
fs.mkdirSync(path.dirname(markerPath), { recursive: true });
fs.writeFileSync(markerPath, `${JSON.stringify(marker, null, 2)}\n`, "utf8");
setTimeout(() => process.exit(0), 500);Alternatively, wrap execSync in a try/catch that removes the marker file on failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/gateway-restart/index.ts
Line: 138-149
Comment:
**Stale restart marker left on disk when pre-command fails**
The restart marker is written to disk at line 138, and only then are the pre-commands executed via `execSync` (lines 142–147). `execSync` throws (synchronously) if a command exits with a non-zero status, propagating the error out of `execute`. Because `setTimeout(() => process.exit(0), 500)` is placed after the loop (line 149), the gateway never exits when a command fails — but the marker file is already on disk.
On the next gateway restart (for any reason), `gateway-restart-watcher` will find the stale marker, consume it, and inject a spurious followup turn into the original session.
The marker file write should be deferred until after all pre-commands have succeeded:
```ts
// Execute pre-commands first; abort with no marker if any fail
const commandOutputs: string[] = [];
for (const command of commands) {
const output = execSync(command, {
encoding: "utf8",
stdio: ["ignore", "pipe", "pipe"],
});
commandOutputs.push(output);
}
// Only write the marker once we know we're committed to restarting
const marker: RestartMarker = { ... };
fs.mkdirSync(path.dirname(markerPath), { recursive: true });
fs.writeFileSync(markerPath, `${JSON.stringify(marker, null, 2)}\n`, "utf8");
setTimeout(() => process.exit(0), 500);
```
Alternatively, wrap `execSync` in a try/catch that removes the marker file on failure.
How can I resolve this? If you propose a fix, please make it concise.| fs.rmSync(markerPath, { force: true }); | ||
|
|
||
| const requestedAtMs = Date.parse(marker.requestedAt); | ||
| const durationSeconds = Number.isFinite(requestedAtMs) | ||
| ? ((Date.now() - requestedAtMs) / 1000).toFixed(1) | ||
| : "unknown"; | ||
| const completionText = | ||
| `[gateway-restart] Gateway restart complete (took ${durationSeconds}s). ${marker.message ?? ""}`.trim(); | ||
|
|
||
| const enqueued = await followupRuntime.enqueueFollowupTurn({ | ||
| sessionKey: marker.sessionKey, | ||
| prompt: `${completionText} Continue with any remaining work.`, | ||
| source: "gateway-restart", | ||
| }); |
There was a problem hiding this comment.
The marker is unconditionally deleted (fs.rmSync) before enqueueFollowupTurn is awaited. While enqueueFollowupTurn wraps its internals in try/catch and always resolves to a boolean, if the function returns false (session not found, session store unavailable after restart, etc.), the marker is already gone and there is no way to retry. The warning logged at line 220 is the only signal — the session resume opportunity is permanently lost.
Consider only deleting the marker after a successful enqueue:
const enqueued = await followupRuntime.enqueueFollowupTurn({ ... });
if (enqueued) {
fs.rmSync(markerPath, { force: true });
ctx.logger.info(`[gateway-restart] Enqueued followup turn for session ${marker.sessionKey}.`);
} else {
ctx.logger.warn(`[gateway-restart] Failed to enqueue followup turn for session ${marker.sessionKey}. Marker preserved for manual inspection.`);
}If idempotent retries are intentionally not supported, at minimum document that the marker is consumed regardless of enqueue outcome.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/gateway-restart/index.ts
Line: 200-213
Comment:
**Marker deleted before followup is confirmed enqueued**
The marker is unconditionally deleted (`fs.rmSync`) before `enqueueFollowupTurn` is awaited. While `enqueueFollowupTurn` wraps its internals in try/catch and always resolves to a boolean, if the function returns `false` (session not found, session store unavailable after restart, etc.), the marker is already gone and there is no way to retry. The warning logged at line 220 is the only signal — the session resume opportunity is permanently lost.
Consider only deleting the marker after a successful enqueue:
```ts
const enqueued = await followupRuntime.enqueueFollowupTurn({ ... });
if (enqueued) {
fs.rmSync(markerPath, { force: true });
ctx.logger.info(`[gateway-restart] Enqueued followup turn for session ${marker.sessionKey}.`);
} else {
ctx.logger.warn(`[gateway-restart] Failed to enqueue followup turn for session ${marker.sessionKey}. Marker preserved for manual inspection.`);
}
```
If idempotent retries are intentionally not supported, at minimum document that the marker is consumed regardless of enqueue outcome.
How can I resolve this? If you propose a fix, please make it concise.| rememberFollowupDrainCallback(params.sessionKey, runner); | ||
| kickFollowupDrainIfIdle(params.sessionKey); |
There was a problem hiding this comment.
rememberFollowupDrainCallback unconditionally overwrites the active runner on hot sessions
rememberFollowupDrainCallback(params.sessionKey, runner) always overwrites the stored drain callback with the freshly constructed noop-typing runner. When this is called for a session that is currently being drained by a channel-specific runner, the active scheduleFollowupDrain loop uses a local effectiveRunFollowup variable — but when the drain's finally block fires and calls scheduleFollowupDrain(key, effectiveRunFollowup) to restart, that function re-reads FOLLOWUP_RUN_CALLBACKS.get(key) (drain.ts line 86) and picks up the noop runner instead of the original.
The result is that subsequent turns in an active hot session would be executed with the noop typing controller, bypassing any channel-level typing/delivery logic.
This path is unlikely in the gateway-restart use case (the session is cold after a restart), but the API is general-purpose and will be called by other plugins. Consider guarding against overwriting an existing active callback:
if (!FOLLOWUP_RUN_CALLBACKS.has(params.sessionKey)) {
rememberFollowupDrainCallback(params.sessionKey, runner);
}
kickFollowupDrainIfIdle(params.sessionKey);Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/runtime/runtime-followup.ts
Line: 43-44
Comment:
**`rememberFollowupDrainCallback` unconditionally overwrites the active runner on hot sessions**
`rememberFollowupDrainCallback(params.sessionKey, runner)` always overwrites the stored drain callback with the freshly constructed noop-typing runner. When this is called for a session that is **currently being drained** by a channel-specific runner, the active `scheduleFollowupDrain` loop uses a local `effectiveRunFollowup` variable — but when the drain's `finally` block fires and calls `scheduleFollowupDrain(key, effectiveRunFollowup)` to restart, that function re-reads `FOLLOWUP_RUN_CALLBACKS.get(key)` (drain.ts line 86) and **picks up the noop runner** instead of the original.
The result is that subsequent turns in an active hot session would be executed with the noop typing controller, bypassing any channel-level typing/delivery logic.
This path is unlikely in the gateway-restart use case (the session is cold after a restart), but the API is general-purpose and will be called by other plugins. Consider guarding against overwriting an existing active callback:
```ts
if (!FOLLOWUP_RUN_CALLBACKS.has(params.sessionKey)) {
rememberFollowupDrainCallback(params.sessionKey, runner);
}
kickFollowupDrainIfIdle(params.sessionKey);
```
How can I resolve this? If you propose a fix, please make it concise.| let followupRuntime: FollowupRuntime | null = null; | ||
| let registeredStateDir: string | null = null; | ||
| let registeredMarkerFileName = "restart-pending.json"; |
There was a problem hiding this comment.
followupRuntime, registeredStateDir, and registeredMarkerFileName are module-level let variables that are overwritten every time register() is called. If the plugin is ever unloaded and re-registered (e.g., after a config reload), a second register() call silently replaces them.
Prefer closing over them inside register and passing them directly into the service start() callback, which already receives ctx.stateDir:
register(api) {
const config = resolveConfig(api.pluginConfig);
const localFollowupRuntime = api.runtime.followup;
api.registerService({
id: "gateway-restart-watcher",
async start(ctx) {
const markerPath = getMarkerPath(ctx.stateDir, config.markerFileName);
// use localFollowupRuntime here
},
});
}Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/gateway-restart/index.ts
Line: 62-64
Comment:
**Module-level mutable state creates re-registration hazard**
`followupRuntime`, `registeredStateDir`, and `registeredMarkerFileName` are module-level `let` variables that are overwritten every time `register()` is called. If the plugin is ever unloaded and re-registered (e.g., after a config reload), a second `register()` call silently replaces them.
Prefer closing over them inside `register` and passing them directly into the service `start()` callback, which already receives `ctx.stateDir`:
```ts
register(api) {
const config = resolveConfig(api.pluginConfig);
const localFollowupRuntime = api.runtime.followup;
api.registerService({
id: "gateway-restart-watcher",
async start(ctx) {
const markerPath = getMarkerPath(ctx.stateDir, config.markerFileName);
// use localFollowupRuntime here
},
});
}
```
How can I resolve this? If you propose a fix, please make it concise.| type FollowupRuntime = { | ||
| enqueueFollowupTurn: (params: { | ||
| sessionKey: string; | ||
| prompt: string; | ||
| agentId?: string; | ||
| originatingChannel?: string; | ||
| originatingTo?: string; | ||
| originatingAccountId?: string; | ||
| originatingThreadId?: string | number; | ||
| originatingChatType?: string; | ||
| model?: string; | ||
| provider?: string; | ||
| extraSystemPrompt?: string; | ||
| timeoutMs?: number; | ||
| source?: string; | ||
| }) => Promise<boolean>; | ||
| }; |
There was a problem hiding this comment.
FollowupRuntime type is locally re-declared rather than derived from the SDK
The FollowupRuntime type duplicates the shape of PluginRuntimeCore["followup"] verbatim. If the SDK type evolves (e.g., new optional parameters are added to enqueueFollowupTurn), this local copy will silently drift. Since api.runtime.followup is already typed on the api object provided by the framework, the module-level variable can carry the inferred type directly, avoiding the duplication.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/gateway-restart/index.ts
Line: 44-60
Comment:
**`FollowupRuntime` type is locally re-declared rather than derived from the SDK**
The `FollowupRuntime` type duplicates the shape of `PluginRuntimeCore["followup"]` verbatim. If the SDK type evolves (e.g., new optional parameters are added to `enqueueFollowupTurn`), this local copy will silently drift. Since `api.runtime.followup` is already typed on the `api` object provided by the framework, the module-level variable can carry the inferred type directly, avoiding the duplication.
How can I resolve this? If you propose a fix, please make it concise.| "openclaw": { | ||
| "extensions": [ | ||
| "./index.ts" | ||
| ] | ||
| } |
There was a problem hiding this comment.
openclaw.install.npmSpec in package metadata
Per the repo's CLAUDE.md bundled plugin naming rules, openclaw.install.npmSpec should equal the package name. The openclaw block here only has extensions — install.npmSpec is absent.
| "openclaw": { | |
| "extensions": [ | |
| "./index.ts" | |
| ] | |
| } | |
| "openclaw": { | |
| "extensions": [ | |
| "./index.ts" | |
| ], | |
| "install": { | |
| "npmSpec": "@openclaw/gateway-restart-plugin" | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/gateway-restart/package.json
Line: 7-11
Comment:
**Missing `openclaw.install.npmSpec` in package metadata**
Per the repo's CLAUDE.md bundled plugin naming rules, `openclaw.install.npmSpec` should equal the package name. The `openclaw` block here only has `extensions` — `install.npmSpec` is absent.
```suggestion
"openclaw": {
"extensions": [
"./index.ts"
],
"install": {
"npmSpec": "@openclaw/gateway-restart-plugin"
}
}
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d268aa213
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const sessionFile = | ||
| sessionEntry.sessionFile?.trim() || | ||
| resolveSessionFilePath(sessionEntry.sessionId, sessionEntry, { agentId }); |
There was a problem hiding this comment.
buildFollowupRunForSession falls back to resolveSessionFilePath(..., { agentId }), which ignores the configured session.store path used to load the session entry. When users keep sessions in a custom store location and an entry lacks sessionFile, the follow-up run will target the default agent sessions directory instead of the existing transcript, so the resumed turn can lose prior context and write to the wrong file.
Useful? React with 👍 / 👎.
| fs.writeFileSync(markerPath, `${JSON.stringify(marker, null, 2)}\n`, "utf8"); | ||
|
|
||
| const commandOutputs: string[] = []; | ||
| for (const command of commands) { | ||
| const output = execSync(command, { |
There was a problem hiding this comment.
The marker file is written before executing pre-restart commands, so any execSync failure leaves a stale marker behind while the gateway stays up. On a later unrelated restart, the watcher will treat that stale marker as a completed restart request and enqueue an incorrect follow-up for the old session. This should be written only after commands succeed (or explicitly cleaned up on command failure).
Useful? React with 👍 / 👎.
3d268aa to
cbabb08
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbabb08e27
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| provider, | ||
| model, |
There was a problem hiding this comment.
buildFollowupRunForSession reconstructs provider/model from the stored session entry, but it never sets run.authProfileId/authProfileIdSource. For sessions that were pinned to a non-default auth profile (for example via model/profile switching), plugin-initiated followups will run with default credentials instead, which can hit the wrong account or fail entirely when only the overridden profile has valid keys. This regresses behavior compared with normal queued turns that preserve session auth-profile selection.
Useful? React with 👍 / 👎.
cbabb08 to
1171cf1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1171cf1520
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| provider, | ||
| model, | ||
| timeoutMs, | ||
| blockReplyBreak: "text_end", | ||
| authProfileId: sessionEntry.authProfileOverride?.trim() || undefined, |
There was a problem hiding this comment.
buildFollowupRunForSession rebuilds run from the stored session but does not carry over sessionEntry.thinkingLevel, so plugin-enqueued followups run with thinkLevel unset; downstream, embedded runs default missing think level to "off" (src/agents/pi-embedded-runner/run.ts), which changes behavior for sessions that were explicitly using /think high (or other non-default levels). This causes restart/proactive turns to resume with different model-thinking settings than the active session and can materially change answer quality and latency.
Useful? React with 👍 / 👎.
1171cf1 to
ab4846b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab4846b4e4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const enqueued = enqueueFollowupRun( | ||
| params.sessionKey, | ||
| followupRun, | ||
| { mode: "followup" }, |
There was a problem hiding this comment.
enqueueFollowupTurn hardcodes { mode: "followup" } for enqueueFollowupRun, and getFollowupQueue reapplies incoming settings to any existing queue. That means if a busy session is currently using another queue mode (for example collect/interrupt from session config), this plugin followup call silently flips the queue mode mid-drain and changes debounce/drop behavior for already queued user turns. Preserve the existing queue mode (or resolve from the session entry) instead of forcing followup here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc31b24293
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const runner = createFollowupRunner({ | ||
| typing: noopTyping, | ||
| typingMode: "never", | ||
| defaultModel: followupRun.run.model ?? DEFAULT_MODEL, | ||
| }); |
There was a problem hiding this comment.
createRuntimeFollowup builds a new runner without sessionKey/storePath/sessionStore, so createFollowupRunner skips session persistence and queue session-refresh paths (see persistRunSessionUsage and refreshQueuedFollowupSession guards in src/auto-reply/reply/followup-runner.ts). In cold-session plugin followups, if the first queued turn triggers compaction/session rotation, later queued turns can continue with stale sessionId/sessionFile and stale runtime accounting instead of following the rotated session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c55696ce0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { execSync } from "node:child_process"; | ||
| import fs from "node:fs"; | ||
| import path from "node:path"; | ||
| import { Type } from "@sinclair/typebox"; |
There was a problem hiding this comment.
gateway-restart imports @sinclair/typebox here, but extensions/gateway-restart/package.json does not declare any dependencies. In production plugin installs (npm install --omit=dev inside the plugin directory), this package will not be present unless it is explicitly listed, so loading the plugin can fail with a module resolution error before registration.
Useful? React with 👍 / 👎.
| const enqueued = enqueueFollowupRun( | ||
| params.sessionKey, | ||
| followupRun, | ||
| { mode: existingMode ?? "followup" }, |
There was a problem hiding this comment.
This enqueue path only passes { mode: ... }, so when no queue exists yet it creates a new queue with default debounce/cap/drop policy instead of the session-configured queue settings. As a result, burst plugin followups on idle sessions can be debounced/dropped/summarized differently from normal turns that use resolveQueueSettings, which changes behavior for sessions that rely on custom queueDebounceMs, queueCap, or queueDrop.
Useful? React with 👍 / 👎.
…nsion Add a new `runtime.followup.enqueueFollowupTurn()` method to the plugin runtime, enabling plugins to schedule proactive agent turns for any session — including cold sessions with no active user interaction. ## Problem Plugins have no way to inject a user-role message into an existing session and trigger an agent turn. The available primitives (`enqueueSystemEvent`, `subagent.run`) either don't initiate turns or create new sessions instead of continuing the original conversation. ## Solution ### Core API: `runtime.followup.enqueueFollowupTurn()` A new namespace on `PluginRuntimeCore` that: 1. Reconstructs a full `FollowupRun` from the session store via a new `buildFollowupRunForSession()` helper — resolving agent identity, workspace, provider/model, session file, and delivery context from the persisted `SessionEntry`. 2. Enqueues the run into the existing followup queue infrastructure. 3. Handles the cold-start drain problem by creating a fresh followup runner with a no-op typing controller and registering it as the drain callback, so the turn executes immediately even when no prior drain loop exists for that session. ### Bundled Extension: `gateway-restart` A reference implementation and practical tool that demonstrates the followup turn API: - **Tool** (`gateway_restart`): Agent calls this to write a restart marker, run optional allowlisted pre-commands, then exit the process. Systemd (or equivalent) auto-restarts the gateway. - **Service** (`gateway-restart-watcher`): On startup, checks for the restart marker. If found, calls `enqueueFollowupTurn()` to resume the original session with a completion message. The agent wakes up with full session context and continues working. ## Files - `src/auto-reply/reply/queue/build-followup-run.ts` — Cold-session FollowupRun builder - `src/plugins/runtime/runtime-followup.ts` — Plugin runtime namespace - `src/plugins/runtime/types-core.ts` — Type surface addition - `src/plugins/runtime/index.ts` — Wiring - `extensions/gateway-restart/` — Bundled extension (tool + service) - `test/helpers/plugins/plugin-runtime-mock.ts` — Test mock update ## Testing End-to-end verified on an isolated sandbox gateway: 1. Agent reads config, makes a change, calls `gateway_restart` 2. Gateway exits, restarts, watcher consumes marker 3. `enqueueFollowupTurn()` fires, agent resumes with full context 4. Agent validates the config change persisted and reports success
…rve queue mode buildFollowupRunForSession now passes the resolved storePath through resolveSessionFilePathOptions so custom session.store locations produce the correct transcript path on the fallback branch. enqueueFollowupTurn reads the existing queue mode before enqueueing instead of unconditionally forcing mode=followup, which could clobber a hot session's collect/interrupt/steer mode mid-drain. Targeted tests added for both fixes.
8c55696 to
f7f8943
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7f89435fc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } as FollowupRun["run"]["execOverrides"]) | ||
| : undefined, | ||
| // Message provider for reply routing | ||
| messageProvider: sessionEntry.lastChannel ?? undefined, |
There was a problem hiding this comment.
buildFollowupRunForSession rebuilds routing state but never sets run.agentAccountId, so cold/plugin followups lose the account binding that normal turns carry. Downstream, createFollowupRunner forwards queued.run.agentAccountId into runEmbeddedPiAgent, and message-tool execution uses that value as the fallback account selector; in multi-account sessions this can route tool actions through the wrong account (or fail auth) even though the session was originally tied to a specific account.
Useful? React with 👍 / 👎.
|
Superseded by #63330 — rebased onto latest main with all bug fixes included and CI passing. |
1 participant
Summary
Add a new
runtime.followup.enqueueFollowupTurn()method to the plugin runtime, enabling plugins to schedule proactive agent turns for any session — including cold sessions with no active user interaction.Problem
Plugins have no way to inject a user-role message into an existing session and trigger an agent turn. The available primitives (
enqueueSystemEvent,subagent.run) either don't initiate turns or create new sessions instead of continuing the original conversation. This blocks use cases like:Solution
Core API:
runtime.followup.enqueueFollowupTurn()A new
followupnamespace onPluginRuntimeCorethat:Reconstructs a full
FollowupRunfrom the session store viabuildFollowupRunForSession()— resolving agent identity, workspace, provider/model, session file, and delivery context from the persistedSessionEntry.Enqueues the run into the existing followup queue infrastructure with
mode: "followup".Handles cold-start drain by creating a fresh followup runner with a no-op typing controller and registering it as the drain callback, so the turn executes immediately even when no prior drain loop exists for that session key.
Bundled Extension:
gateway-restartA reference implementation that demonstrates the followup turn API:
Tool (
gateway_restart): Agent calls this to write a restart marker, run optional allowlisted pre-commands, then exit the process. Systemd (or equivalent) auto-restarts the gateway.Service (
gateway-restart-watcher): On startup, checks for the restart marker. If found, callsenqueueFollowupTurn()to resume the original session with a completion message. The agent wakes up with full session context and continues working.Disabled by default (
enabledByDefault: falsein manifest). Opt-in viaplugins.entries.gateway-restart.enabled: true.Files Changed
src/auto-reply/reply/queue/build-followup-run.tssrc/plugins/runtime/runtime-followup.tssrc/plugins/runtime/types-core.tsfollowuptype toPluginRuntimeCoresrc/plugins/runtime/index.tscreateRuntimeFollowup()extensions/gateway-restart/index.tsextensions/gateway-restart/openclaw.plugin.jsonextensions/gateway-restart/package.jsontest/helpers/plugins/plugin-runtime-mock.tsfollowupto mockTesting
End-to-end verified on an isolated sandbox gateway (separate user, separate port, no channel conflicts):
gateway_restarttoolgateway_restartenqueueFollowupTurn()enqueues and drains a cold-session followup turntsgo,oxlint, commit hooks)Design Decisions
runtime-followup.ts— all heavy dependencies (buildFollowupRunForSession,createFollowupRunner,createTypingController) are dynamically imported to keep module load lightweight.originatingChannel/originatingToaren't provided, the builder falls back to the session entry'slastChannel/lastTo, so replies route to wherever the session was last active.senderIsOwner: truefor plugin-initiated turns — these are trusted system actions, not external user messages.extensions/rather thansrc/plugins/bundled/— follows the repo's plugin discovery convention.