[Plan Mode AUTOMATION] Cron nudges + auto-enable + subagent follow-ups#70089
[Plan Mode AUTOMATION] Cron nudges + auto-enable + subagent follow-ups#70089100yenadmin wants to merge 1 commit into
Conversation
openclaw-barnacle
Bot
added
app: macos
Greptile SummaryThis PR adds the plan-mode automation layer: escalating-retry nudge crons (10/30/60-min intervals), model-specific The new files are well-structured and thoroughly tested. The Confidence Score: 5/5Safe to merge once the foundational Plan Mode PRs (#70031–#70070) land; no correctness or security issues found. All findings are P2 style/cleanup items. The critical auto-close gate logic, cycleId idempotency, and approval metadata persistence are sound and well-covered by unit tests. No files require special attention beyond the three P2 items noted inline. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/plan-mode/plan-nudge-crons.ts
Line: 112-122
Comment:
**Dynamic import re-executed inside the loop body**
`assertSafeCronSessionTargetId` is dynamically imported on every iteration of the `intervals` for-loop (once per nudge, up to 3 times with defaults). The `sessionKey` being validated doesn't change between iterations — if it is invalid the whole schedule call should be aborted before entering the loop at all. As written, the validation check also can't `break` the outer loop; it `continue`s, so the remaining intervals still schedule redundantly. Hoisting both the import and the validation outside the loop would short-circuit immediately on a bad key.
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/gateway/plan-snapshot-persister.ts
Line: 726-730
Comment:
**Stale preflight `allowAutoClose` used in post-write logging branch**
The `else if` checks `!allowAutoClose` (the preflight value) rather than `!appliedAllowAutoClose` (the value actually committed to the store). When the store-lock check flips the decision from allow→deny, neither the line 675 branch nor this branch fires — so no "auto-close suppressed" log appears in the post-write section. The lock-flip warn at line 612-619 inside the callback captures the flip, but consider replacing `!allowAutoClose` with `!appliedAllowAutoClose` to keep the post-write log consistent with the actual applied 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/agents/plan-mode/plan-mode-debug-log.ts
Line: 154-156
Comment:
**`isDebugEnabled` is a trivial one-liner wrapper with no added value**
`isDebugEnabled()` exists solely as `return isPlanModeDebugEnabled()`. Since `isPlanModeDebugEnabled` is now exported directly, the private wrapper adds a call frame without any encapsulation benefit. Calling `isPlanModeDebugEnabled()` directly in `logPlanModeDebug` would eliminate the indirection.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test(plan-mode): add mandatory `title` a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { assertSafeCronSessionTargetId } = await import("../../cron/session-target.js"); | ||
| try { | ||
| assertSafeCronSessionTargetId(params.sessionKey); | ||
| } catch (validationErr) { | ||
| params.log?.warn?.( | ||
| `plan-nudge schedule skipped: sessionKey "${params.sessionKey}" fails cron sessionTarget validation: ${ | ||
| validationErr instanceof Error ? validationErr.message : String(validationErr) | ||
| }`, | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
assertSafeCronSessionTargetId is dynamically imported on every iteration of the intervals for-loop (once per nudge, up to 3 times with defaults). The sessionKey being validated doesn't change between iterations — if it is invalid the whole schedule call should be aborted before entering the loop at all. As written, the validation check also can't break the outer loop; it continues, so the remaining intervals still schedule redundantly. Hoisting both the import and the validation outside the loop would short-circuit immediately on a bad key.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-mode/plan-nudge-crons.ts
Line: 112-122
Comment:
**Dynamic import re-executed inside the loop body**
`assertSafeCronSessionTargetId` is dynamically imported on every iteration of the `intervals` for-loop (once per nudge, up to 3 times with defaults). The `sessionKey` being validated doesn't change between iterations — if it is invalid the whole schedule call should be aborted before entering the loop at all. As written, the validation check also can't `break` the outer loop; it `continue`s, so the remaining intervals still schedule redundantly. Hoisting both the import and the validation outside the loop would short-circuit immediately on a bad key.
How can I resolve this? If you propose a fix, please make it concise.| } else if (params.closeOnComplete && !allowAutoClose) { | ||
| log.info( | ||
| `plan completed but auto-close suppressed (no approved state): sessionKey=${params.sessionKey} — ` + | ||
| "agent must call exit_plan_mode for explicit user approval before mutations unlock", | ||
| ); |
There was a problem hiding this comment.
allowAutoClose used in post-write logging branch
The else if checks !allowAutoClose (the preflight value) rather than !appliedAllowAutoClose (the value actually committed to the store). When the store-lock check flips the decision from allow→deny, neither the line 675 branch nor this branch fires — so no "auto-close suppressed" log appears in the post-write section. The lock-flip warn at line 612-619 inside the callback captures the flip, but consider replacing !allowAutoClose with !appliedAllowAutoClose to keep the post-write log consistent with the actual applied outcome.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/plan-snapshot-persister.ts
Line: 726-730
Comment:
**Stale preflight `allowAutoClose` used in post-write logging branch**
The `else if` checks `!allowAutoClose` (the preflight value) rather than `!appliedAllowAutoClose` (the value actually committed to the store). When the store-lock check flips the decision from allow→deny, neither the line 675 branch nor this branch fires — so no "auto-close suppressed" log appears in the post-write section. The lock-flip warn at line 612-619 inside the callback captures the flip, but consider replacing `!allowAutoClose` with `!appliedAllowAutoClose` to keep the post-write log consistent with the actual applied outcome.
How can I resolve this? If you propose a fix, please make it concise.| function isDebugEnabled(): boolean { | ||
| return isPlanModeDebugEnabled(); | ||
| } |
There was a problem hiding this comment.
isDebugEnabled is a trivial one-liner wrapper with no added value
isDebugEnabled() exists solely as return isPlanModeDebugEnabled(). Since isPlanModeDebugEnabled is now exported directly, the private wrapper adds a call frame without any encapsulation benefit. Calling isPlanModeDebugEnabled() directly in logPlanModeDebug would eliminate the indirection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-mode/plan-mode-debug-log.ts
Line: 154-156
Comment:
**`isDebugEnabled` is a trivial one-liner wrapper with no added value**
`isDebugEnabled()` exists solely as `return isPlanModeDebugEnabled()`. Since `isPlanModeDebugEnabled` is now exported directly, the private wrapper adds a call frame without any encapsulation benefit. Calling `isPlanModeDebugEnabled()` directly in `logPlanModeDebug` would eliminate the indirection.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
Adds the plan-mode automation + follow-up layer across runner/gateway/cron: plan continuation nudges (heartbeat + cron), model-based plan-mode auto-enable, plan-mode debug logging + reference card, subagent lifecycle gating/continuations, and the typed pending-injection queue foundation with schema + protocol updates.
Changes:
- Introduces plan-mode automation primitives (nudge cron scheduling/guards, heartbeat plan nudge prefix, auto-enable matcher, debug log, reference card, plan-mode status tool).
- Wires plan-mode/subagent state through gateway + runner + cron (session row surfaces, event subscriptions, subagent concurrency caps, plan snapshot persistence hooks).
- Expands schemas/protocol for plan-mode execution (new error codes, cron payload
planCycleId, session entry fields, config schema additions, test configs).
Reviewed changes
Copilot reviewed 66 out of 66 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vitest/vitest.plan-mode.config.ts | Adds a dedicated Vitest config/scope for plan-mode test/coverage runs. |
| src/plugins/contracts/plugin-sdk-runtime-api-guardrails.test.ts | Updates runtime export allowlist for Telegram (sendDocumentTelegram, TelegramDocumentOpts). |
| src/infra/heartbeat-runner.ts | Adds buildActivePlanNudge and prepends plan nudge to heartbeat prompt; simplifies plugin resolution. |
| src/infra/heartbeat-runner.plan-nudge.test.ts | Unit tests for the heartbeat plan nudge prefix builder and suppression guards. |
| src/gateway/session-utils.types.ts | Extends gateway session row shape to include exec/plan/pending-interaction fields. |
| src/gateway/session-utils.ts | Refactors session loading/canonicalization and combined store merging; removes deleted-agent helper logic. |
| src/gateway/server.impl.ts | Refactors startup/close wiring; introduces plan snapshot unsubscribe into close handler. |
| src/gateway/server-runtime-subscriptions.ts | Wires plan snapshot persister + subagent gate persistence and broadcasts sessions.changed. |
| src/gateway/server-runtime-handles.ts | Tracks planSnapshotUnsub in mutable runtime state. |
| src/gateway/server-methods/sessions.ts | Extends sessions.changed payload and adjusts send/steer flow. |
| src/gateway/server-close.ts | Ensures plan snapshot subscription is unsubscribed on shutdown. |
| src/gateway/server-close.test.ts | Updates close-handler tests for new constructor shape and planSnapshotUnsub. |
| src/gateway/protocol/schema/error-codes.ts | Adds plan-approval subagent gate error codes with structured details expectations. |
| src/gateway/protocol/schema/cron.ts | Adds optional planCycleId to cron agent-turn payload schema. |
| src/gateway/protocol/index.ts | Re-exports ErrorCode and removes unused channels.start validation exports. |
| src/gateway/plan-snapshot-persister.test.ts | Adds guard-rail tests for approvalRunId defensive behavior. |
| src/cron/types.ts | Adds planCycleId to CronAgentTurnPayloadFields. |
| src/cron/normalize.ts | Consolidates/clarifies sessionTarget:"current" resolution semantics. |
| src/cron/isolated-agent/run.ts | Adds plan-cycle-bound nudge skip guards and wires plan-mode auto-enable at cron turn start. |
| src/cron/isolated-agent/run.plan-mode.test.ts | Tests for plan-cycle nudge guards + autoEnableFor runtime wiring in cron runs. |
| src/config/zod-schema.agent-runtime.ts | Adds per-agent embeddedPi.autoContinue + maxIterations runtime schema. |
| src/config/zod-schema.agent-defaults.ts | Adds defaults schema for embeddedPi autoContinue/maxIterations and planMode (enabled/autoEnableFor/debug). |
| src/config/types.agents.ts | Adds per-agent embeddedPi.autoContinue + maxIterations types. |
| src/config/types.agent-defaults.ts | Adds embeddedPi autoContinue/maxIterations and planMode autoEnableFor/debug types + docs. |
| src/config/sessions/types.ts | Adds plan-mode session state fields, typed pending-injection queue types, and token-total helper rename. |
| src/config/schema.base.generated.ts | Regenerates base config schema for new embeddedPi + planMode fields and updates compaction help text. |
| src/commands/status.summary.ts | Switches to resolveFreshSessionTotalTokens for status cost display. |
| src/commands/sessions.ts | Adjusts token total semantics (fresh vs preserved stale totals) for CLI + JSON output. |
| src/auto-reply/reply/fresh-session-entry.ts | Adds helpers to read latest planMode/acceptEdits/session entry from disk (skip-cache) for freshness. |
| src/auto-reply/reply/agent-runner.misc.runreplyagent.test.ts | Updates test expectations to reflect new skip-cache disk reads. |
| src/agents/tools/update-plan-tool.test.ts | Expands tests for non-empty content, close-on-complete, and closure gate criteria. |
| src/agents/tools/sessions-spawn-tool.ts | Adds plan-mode-aware concurrency cap, cleanup keep behavior, and parent openSubagent tracking. |
| src/agents/tools/sessions-spawn-tool.test.ts | Updates mocks/tests and adds coverage for plan-mode subagent concurrency cap. |
| src/agents/tools/plan-mode-status-tool.ts | Adds plan_mode_status tool for read-only plan-mode introspection (disk + run ctx). |
| src/agents/tools/cron-tool.ts | Adds recipe documentation for sessionTarget:"current" resume-after-wait. |
| src/agents/tool-display-config.ts | Adds display config for plan tools (enter/exit/status) and fixes ask_user_question detail keys. |
| src/agents/subagent-registry.test.ts | Updates agent-events mocks and loosens outcome shape expectations. |
| src/agents/subagent-registry.steer-restart.test.ts | Ensures agent-events mock includes actual exports; adds openSubagentRunIds remap test. |
| src/agents/subagent-registry-run-manager.ts | Drains/replaces parent openSubagentRunIds on child completion/steer restart; outcome update logic tweak. |
| src/agents/subagent-announce.ts | Adds plan-mode-aware announce instruction suffix and reads requester plan mode on announce. |
| src/agents/plan-mode/reference-card.ts | Adds compact plan-mode reference card for system prompt injection. |
| src/agents/plan-mode/plan-nudge-crons.ts | Implements scheduling + cleanup for plan nudge one-shot cron jobs. |
| src/agents/plan-mode/plan-nudge-crons.test.ts | Unit tests for plan nudge cron schedule/cleanup behavior. |
| src/agents/plan-mode/plan-mode-debug-log.ts | Adds opt-in plan-mode debug event logger with env/config gating and TTL caching. |
| src/agents/plan-mode/integration.test.ts | Adds wiring smoke test for plan-mode tool enablement + mutation gate behavior. |
| src/agents/plan-mode/injections.ts | Adds typed pending-injection queue (enqueue/consume/compose) with legacy migration. |
| src/agents/plan-mode/auto-enable.ts | Adds model-id regex matching with compiled-pattern cache for plan-mode auto-enable. |
| src/agents/plan-mode/auto-enable.test.ts | Unit tests for auto-enable matching + malformed-pattern handling. |
| src/agents/pi-tools.ts | Threads planMode + live accessors into before-tool-call hook context. |
| src/agents/pi-tools.before-tool-call.ts | Adds plan-mode mutation gate + post-approval acceptEdits constraint gate with live refresh semantics. |
| src/agents/pi-embedded-runner/skills-runtime.test.ts | Updates snapshot reload logic tests to account for resolvedPlanTemplates presence. |
| src/agents/pi-embedded-runner/run/params.ts | Threads planMode + live accessors through embedded runner params. |
| src/agents/pi-embedded-runner/run/helpers.ts | Raises default retry iteration budget; adds user override and subagent cap constant. |
| src/agents/pi-embedded-runner/run.overflow-compaction.test.ts | Keeps test fast by overriding maxIterations and updating expectations accordingly. |
| src/agents/pi-embedded-runner/pending-injection.ts | Adds backward-compat shim over typed injection queue consumer + composer. |
| src/agents/pi-embedded-runner/pending-injection.test.ts | Adds tests for pending injection consumption/composition with hermetic session store. |
| apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift | Adds new plan-approval gate error codes to Swift client models. |
| apps/macos/Sources/OpenClawProtocol/GatewayModels.swift | Adds new plan-approval gate error codes to macOS Swift client models. |
Comments suppressed due to low confidence (1)
src/gateway/server.impl.ts:232
startupTraceis referenced (e.g.startupTrace.measure(...)) but is no longer declared sincecreateGatewayStartupTrace()andconst startupTrace = ...were removed. This will fail to compile; either restore astartupTraceimplementation (even a no-op that providesmark/measure) or remove allstartupTrace.*call sites in this file.
| resolvedDelivery: { | ||
| channel: prepared.context.resolvedDelivery.channel, | ||
| to: prepared.context.resolvedDelivery.to, | ||
| accountId: prepared.context.resolvedDelivery.accountId, | ||
| threadId: prepared.context.resolvedDelivery.threadId, | ||
| }, |
There was a problem hiding this comment.
When calling executeCronRun, resolvedDelivery is now passed with only { channel, accountId }, dropping to and threadId. executeCronRun (and downstream cron message-tool executor) accepts/uses to/threadId for targeting; omitting them will lose explicit delivery targets for cron agent turns. Pass through to and threadId from prepared.context.resolvedDelivery here as well.
| export function loadSessionEntry(sessionKey: string) { | ||
| const cfg = loadConfig(); | ||
| const key = normalizeOptionalString(sessionKey) ?? ""; | ||
| const target = resolveGatewaySessionStoreTarget({ | ||
| const canonicalKey = resolveSessionStoreKey({ cfg, sessionKey }); | ||
| const agentId = resolveSessionStoreAgentId(cfg, canonicalKey); | ||
| const { storePath, store } = resolveGatewaySessionStoreLookup({ | ||
| cfg, | ||
| key, | ||
| key: normalizeOptionalString(sessionKey) ?? "", | ||
| canonicalKey, | ||
| agentId, | ||
| }); | ||
| const storePath = target.storePath; | ||
| const store = loadSessionStore(storePath); | ||
| const freshestMatch = resolveFreshestSessionStoreMatchFromStoreKeys(store, target.storeKeys); | ||
| const legacyKey = freshestMatch?.key !== target.canonicalKey ? freshestMatch?.key : undefined; | ||
| return { | ||
| const target = resolveGatewaySessionStoreTarget({ | ||
| cfg, | ||
| storePath, | ||
| key: normalizeOptionalString(sessionKey) ?? "", | ||
| store, | ||
| entry: freshestMatch?.entry, | ||
| canonicalKey: target.canonicalKey, | ||
| legacyKey, | ||
| }; | ||
| }); | ||
| const freshestMatch = resolveFreshestSessionStoreMatchFromStoreKeys(store, target.storeKeys); | ||
| const legacyKey = freshestMatch?.key !== canonicalKey ? freshestMatch?.key : undefined; | ||
| return { cfg, storePath, store, entry: freshestMatch?.entry, canonicalKey, legacyKey }; |
There was a problem hiding this comment.
This refactor removes the exported resolveDeletedAgentIdFromSessionKey helper, but other gateway modules still import/use it (e.g. server-methods/chat.ts, sessions-resolve.ts, and related tests). As-is this will cause build failures and/or drop the deleted-agent guard behavior (#65524). Either keep exporting the helper from session-utils.ts or update all call sites to the new deleted-agent validation mechanism (and adjust tests accordingly).
| ); | ||
| return; | ||
| } | ||
| const { entry, canonicalKey, storePath } = loadSessionEntry(key); | ||
| if (!entry?.sessionId) { |
There was a problem hiding this comment.
The deleted-agent guard for sessions.send/sessions.steer appears to have been removed (previously rejected sends targeting sessions whose owning agent was deleted, #65524). This changes behavior and will likely break the existing deleted-agent guard tests in src/gateway/server-methods/sessions.send-deleted-agent.test.ts. Consider reintroducing the resolveDeletedAgentIdFromSessionKey check here (matching chat.send) or moving the guard into loadSessionEntry/session resolution so both send paths consistently fail closed.
| * compatible): on store-write failure inside the underlying queue | ||
| * helper, the queue helper drops the captured entries and returns an | ||
| * empty array — favoring the once-and-only-once guarantee over | ||
| * caller-can-still-inject. Operators see the warn-log line for any | ||
| * disk failure path. |
There was a problem hiding this comment.
The docstring claims that on store-write failure the underlying queue consumer "drops the captured entries and returns an empty array". But consumePendingAgentInjections() is implemented to return any captured entries even if persisting the clear fails (best-effort delivery). Update this comment to match the actual error semantics so callers/operators aren't misled during debugging.
| * compatible): on store-write failure inside the underlying queue | |
| * helper, the queue helper drops the captured entries and returns an | |
| * empty array — favoring the once-and-only-once guarantee over | |
| * caller-can-still-inject. Operators see the warn-log line for any | |
| * disk failure path. | |
| * compatible): if the underlying queue helper captures pending entries | |
| * but then fails to persist the queue clear, it still returns the | |
| * captured entries rather than an empty array. This favors | |
| * caller-can-still-inject best-effort delivery, and operators see the | |
| * warn-log line for any disk failure path. |
![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: 55a76b9012
ℹ️ 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".
| run({ | ||
| reason: params.reason, | ||
| agentId: params.agentId, | ||
| sessionKey: params.sessionKey, | ||
| heartbeat: params.heartbeat, | ||
| }); |
There was a problem hiding this comment.
startHeartbeatRunner no longer forwards params.heartbeat from the wake request, so targeted wakes now always run with targetAgent.heartbeat defaults. This drops requestHeartbeatNow({ heartbeat: { target: "last" } }) overrides used by cron/server wake paths, which can reroute or suppress heartbeat delivery when the agent default target differs from the requested one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real concern, acknowledged. Not fixed in this immediate cycle (we're in cleanup-mode focused on maintainer-takeover; see #70101 for context). Tracked in the umbrella's deferred-items checklist for the bot-triage follow-up cycle. The fix will land on this branch with a Fixed in {SHA} reply here per the standard pr-review-loop pattern. Maintainers can also opt to merge the per-part PR as-is and address in a post-merge polish cycle.
| resolvedDelivery: { | ||
| channel: prepared.context.resolvedDelivery.channel, | ||
| to: prepared.context.resolvedDelivery.to, | ||
| accountId: prepared.context.resolvedDelivery.accountId, | ||
| threadId: prepared.context.resolvedDelivery.threadId, | ||
| }, |
There was a problem hiding this comment.
runCronIsolatedAgentTurn now strips to and threadId when calling executeCronRun, so downstream execution always sees those fields as undefined. The executor uses these values to compute currentChannelId and to pass messageTo/messageThreadId into runEmbeddedPiAgent; dropping them breaks explicit recipient/thread routing for cron turns (especially topic/threaded channels) and can cause tool-side messages to target the wrong place or fail policy checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real concern, acknowledged. Not fixed in this immediate cycle (we're in cleanup-mode focused on maintainer-takeover; see #70101 for context). Tracked in the umbrella's deferred-items checklist for the bot-triage follow-up cycle. The fix will land on this branch with a Fixed in {SHA} reply here per the standard pr-review-loop pattern. Maintainers can also opt to merge the per-part PR as-is and address in a post-merge polish cycle.
|
Acknowledging Greptile / Copilot / Codex review feedback on this PR. Triage status:
For end-to-end integration testing in the meantime, see #70071 [Plan Mode FULL] — the green-CI integrated bundle of all parts. |
55a76b9 to
0b1ed28
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b1ed28085
ℹ️ 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".
| export const validateChannelsStatusParams = ajv.compile<ChannelsStatusParams>( | ||
| ChannelsStatusParamsSchema, | ||
| ); | ||
| export const validateChannelsStartParams = | ||
| ajv.compile<ChannelsStartParams>(ChannelsStartParamsSchema); | ||
| export const validateChannelsLogoutParams = ajv.compile<ChannelsLogoutParams>( | ||
| ChannelsLogoutParamsSchema, |
There was a problem hiding this comment.
validateChannelsStartParams is no longer defined/exported here, but src/gateway/server-methods/channels.ts still imports and calls it for the channels.start RPC. That leaves the handler with a missing symbol (ESM module-link failure at startup, or undefined at runtime depending build target), which breaks channel startup requests even though ChannelsStartParamsSchema still exists in the protocol schema.
Useful? React with 👍 / 👎.
| const subagentPolicy = | ||
| options?.sessionKey && | ||
| isSubagentEnvelopeSession(options.sessionKey, { | ||
| cfg: options.config, | ||
| store: subagentStore, | ||
| }) | ||
| ? resolveSubagentToolPolicyForSession(options.config, options.sessionKey, { | ||
| store: subagentStore, | ||
| }) | ||
| isSubagentSessionKey(options?.sessionKey) && options?.sessionKey | ||
| ? resolveSubagentToolPolicyForSession(options.config, options.sessionKey) |
There was a problem hiding this comment.
This now gates subagent policy only on isSubagentSessionKey(...), which excludes ACP session keys (agent:...:acp:...). ACP children are still treated as subagent envelopes via stored subagentRole/subagentControlScope in subagent-capabilities.ts, so this change skips resolveSubagentToolPolicyForSession for ACP runs and removes core-tool restrictions (e.g. leaf ACP children can access tools that should stay denied such as sessions_spawn/subagents).
Useful? React with 👍 / 👎.
…s Re-cut from merged-upstream state (v4) Cherry-pick / file-copy of 66 files from feat/plan-channel-parity-merged-upstream (merged-head 651a22d) onto current upstream/main. Branch: isolated/pm-automation-followups. v4 absorbs the upstream merge into plan-mode dev (Option B; merge commit b1ead74) plus the post-merge build/test fixes (commit 651a22d).
0b1ed28 to
21292e5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21292e5e59
ℹ️ 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".
| groupChannel: params.groupChannel, | ||
| groupSpace: params.groupSpace, | ||
| memberRoleIds: params.memberRoleIds, | ||
| spawnedBy: params.spawnedBy, |
There was a problem hiding this comment.
This call stopped forwarding memberRoleIds into runEmbeddedAttemptWithBackend, so downstream tool construction now always sees params.memberRoleIds as undefined. In group-channel runs (Discord/Slack/etc.), role-aware policy checks in createOpenClawCodingTools rely on those ids (src/agents/pi-embedded-runner/run/attempt.ts:511), so this regression can incorrectly deny/allow role-gated actions and break channel permission behavior for any session that depends on member roles.
Useful? React with 👍 / 👎.
| @@ -261,10 +268,6 @@ export async function runEmbeddedPiAgent( | |||
| config: params.config, | |||
| }); | |||
| const resolvedWorkspace = workspaceResolution.workspaceDir; | |||
There was a problem hiding this comment.
The canonical-workspace derivation was removed here, and the attempt params no longer carry isCanonicalWorkspace; as a result, bootstrap routing falls back to its default true path (src/agents/pi-embedded-runner/run/attempt-bootstrap-routing.ts:44-46). For non-default workspaces where effectiveWorkspace === resolvedWorkspace, runs are now misclassified as canonical, which changes resolveBootstrapMode to full instead of limited (src/agents/bootstrap-mode.ts:23) and can inject/record canonical bootstrap behavior in the wrong workspace.
Useful? React with 👍 / 👎.
|
Related work from PRtags group Title: Open PR candidate: plan-mode carve-out overlaps integrated full bundle
|
📋 Umbrella tracker: #70101 — master tracker for the 9-PR plan-mode rollout. See it for status of all parts + suggested merge order + carry-forward backlog.
Summary
Adds the plan-mode automation + subagent-follow-up layer: cron-driven escalating-retry nudges (1/3/5-min intervals),
auto-enable(model-specific opt-in for plan mode without explicit/plan on), subagent plan-snapshot persister (so a subagent's plan state is captured + restorable on resume), plan-mode debug log (operator-visible debug trail of plan-mode lifecycle events), reference card prompt (compact plan-mode rules summary the agent sees in its system prompt), and the plan-execution-nudge crons (P2.12a imperative-step nudge text).Also bundles the INJECTIONS foundation (
70a6e4b23a) so this branch compiles standalone (modulo the PlanMode type deps). Without that,pending-injection.tswould have unresolved imports.Carved out of #68939 (closed). Originally planned as
[Plan Mode 4/7]in the numbered sequence; could not be cleanly isolated due to type-dep closure on PR3+PR4 foundational files. Lives here as a thematic PR + in [Plan Mode FULL] for integration testing.Sub-themes for review navigation
This PR bundles substantively different work areas because they were originally split as four sub-PRs in the dev history but have to ship together to compile (each references symbols introduced by the others). External-review convention reads the diff in this order:
Theme A — Plan-mode automation (the headline work)
The cron + nudge + auto-enable / debug-log / reference-card surface that gives plan mode its escalating-retry behavior.
src/agents/plan-mode/plan-nudge-crons.{ts,test.ts}— escalating-retry nudges (1/3/5-min)src/agents/plan-mode/auto-enable.{ts,test.ts}— model-specific opt-insrc/agents/plan-mode/plan-mode-debug-log.{ts,test.ts}— operator debug trailsrc/agents/plan-mode/reference-card.ts— system-prompt reference cardsrc/cron/isolated-agent/run.{ts,plan-mode.test.ts}+src/cron/normalize.ts+src/cron/types.ts— cron executor + plan-mode-aware target resolutionsrc/infra/heartbeat-runner.{ts,plan-nudge.test.ts}— heartbeat-triggered nudge dispatchTheme B — Subagent plan-snapshot persistence
Captures + restores a subagent's plan state so it survives parent restarts / web reconnects.
src/gateway/plan-snapshot-persister.{ts,test.ts}src/gateway/sessions-patch.subagent-gate.test.tsTheme C — Stale-state gate fix (
fresh-session-entry)Hardens the auto-reply pipeline so plan-mode state reads from a fresh session-store entry per turn (instead of a stale in-memory cached entry that would silently drift across long-running sessions). Codex called this out as plausibly tagalong; in practice it's a bug fix uncovered while wiring the cron-nudge dispatcher.
src/auto-reply/reply/fresh-session-entry.{ts,test.ts}Theme D — Subagent gating + spawn-tool refinement
Threads
runId+ plan-mode awareness throughsessions_spawn, with subagent-registry/announce wire-up so anexit_plan_modefrom a parent doesn't approve while research children are still in flight.src/agents/tools/sessions-spawn-tool.{ts,test.ts}src/agents/subagent-{announce,registry-run-manager,registry.test,registry.steer-restart.test}.tsTheme E — Runner / tool-call plumbing (cross-cutting)
Threads automation context (planMode, runId, scheduler-trigger metadata) into the embedded runner + tool-call hook chain so the above themes can fire without per-call session-store reads.
src/agents/pi-embedded-runner/run.{ts,overflow-compaction.test.ts}+run/{helpers,incomplete-turn,params}.tssrc/agents/pi-tools.{ts,before-tool-call.ts}Theme F — Schema additions (foundational)
Config schema for
planMode.autoEnableFor,planMode.approvalTimeoutSeconds, nudge cadence;cronanderror-codesprotocol additions.src/config/types.agent-defaults.ts+zod-schema.agent-defaults.tssrc/config/types.agents.ts+zod-schema.agent-runtime.tssrc/config/schema.base.generated.ts(regenerated)src/gateway/protocol/schema/{cron,error-codes}.ts+protocol/index.tssrc/gateway/server.impl.ts(wires the new schema)Theme G — Apps tooling (passthrough)
apps/macos/Sources/OpenClawProtocol/GatewayModels.swift+apps/shared/.../GatewayModels.swift— generated Swift mirrors of the protocol additionsWhy not split into 4 sub-PRs?
Codex review suggested splitting Themes A / B / C / D. We considered it but the PR-budget on the maintainer side is constrained (we're already at 9 plan-mode PRs); adding 3 more sub-PRs trades one cleanup problem for another. The themed structure above is provided so reviewers can navigate this PR as if it were 4 sub-PRs without the maintenance overhead of actually splitting it.
Overlap with
[Plan Mode INJECTIONS](#70088)This PR includes the typed pending-injection queue foundation (cherry-picked commit
70a6e4b23a) so the branch compiles standalone. The 6 files involved are:src/agents/plan-mode/injections.{ts,test.ts}src/agents/pi-embedded-runner/pending-injection.tssrc/commands/sessions.ts(small change)src/commands/status.summary.ts(small change)src/config/sessions/types.ts(queue-type additions, ~240 lines)For review purposes: read these files in #70088, not here. Once #70088 merges to
main, this PR's diff vsmainautomatically loses these files.What This PR Includes
Plan-mode automation (new files)
src/agents/plan-mode/plan-nudge-crons.ts+ test) — escalating-retry nudges at 1/3/5-min intervals when an agent appears stuck mid-plan. Idempotent against thecycleId.src/agents/plan-mode/auto-enable.ts+ test) — model-specific opt-in to plan mode driven byagents.defaults.planMode.autoEnableForconfig.src/agents/plan-mode/plan-mode-debug-log.ts+ test) — operator-visible debug trail of plan-mode lifecycle events (entered, approved, rejected, cycle restart, etc.).src/agents/plan-mode/reference-card.ts) — compact plan-mode rules summary added to the agent's system prompt.Subagent follow-ups
src/gateway/plan-snapshot-persister.ts+ test) — captures + restores a subagent's plan state across runs.src/gateway/sessions-patch.subagent-gate.test.ts) — gate for subagent-spawned sessions to inherit parent plan-mode context appropriately.src/agents/subagent-registry*) — wire-up changes for plan-mode-aware spawn lifecycle.Heartbeat + runner integration
src/infra/heartbeat-runner.plan-nudge.test.ts+ impl changes insrc/agents/pi-embedded-runner/run.ts+pending-injection.ts) — heartbeat-triggered nudge dispatch.src/agents/pi-tools.before-tool-call.ts,pi-tools.ts,pi-embedded-runner/run/{params,attempt,incomplete-turn}.ts) — threads automation hooks through the runner.Schema additions
src/config/types.agent-defaults.ts,src/config/zod-schema.agent-defaults.ts—planMode.autoEnableFor,planMode.approvalTimeoutSeconds(schema-reserved), nudge cadence config.Foundational (bundled for compile only — same content as [Plan Mode INJECTIONS] #70088)
src/agents/plan-mode/injections.ts+ testsrc/agents/pi-embedded-runner/pending-injection.tssrc/config/sessions/types.tsfor the queueFiles In Scope
Primary review targets (the actual automation work):
src/agents/plan-mode/plan-nudge-crons.ts+ testsrc/agents/plan-mode/auto-enable.ts+ testsrc/agents/plan-mode/plan-mode-debug-log.ts+ testsrc/gateway/plan-snapshot-persister.ts+ testsrc/agents/plan-mode/reference-card.tsSupporting:
src/agents/pi-embedded-runner/src/config/Reviewer Guide
plan-nudge-crons.ts(15 min) — the escalating-retry semantics + cycleId idempotencyauto-enable.ts— model-specific opt-in logicplan-mode-debug-log.ts— operator debug surfaceplan-snapshot-persister.ts— subagent state continuityreference-card.ts— system-prompt additionWhat This PR Does NOT Include
plan-mode/index.ts,types.ts,mutation-gate.ts,approval.ts) → land in[Plan Mode 1/6]+[Plan Mode 2/6]. This PR's code references those types (which is why CI is red until those merge).[Plan Mode FULL]only (separate work)Issue references
Test Status
PlanModetype resolution (resolves toanyuntil [Plan Mode 1/6] + [Plan Mode 2/6] merge to main). Cherry-pick usedgit -c core.hooksPath=/dev/null cherry-pick --continueto bypass; this is the same red-CI-expected pattern as the numbered per-part PRs.Carry-forward / deferred
agents.defaults.planMode.approvalTimeoutSeconds— schema-reserved here; runtime wiring deferred to a follow-up cyclecycleId— initial implementation here; refinement may come in a follow-up