[Plan Mode 2/8] Plan-state foundation + core backend MVP#70011
[Plan Mode 2/8] Plan-state foundation + core backend MVP#70011100yenadmin wants to merge 2 commits into
Conversation
openclaw-barnacle
Bot
added
app: macos
There was a problem hiding this comment.
Pull request overview
Lays down the Plan Mode foundation (session state, storage/types) and a core backend MVP (mutation gating + planning nudges), plus skill plan-template seeding and protocol wiring to support plan-mode-related session patches.
Changes:
- Add plan-mode runtime types, approval state machine, and a mutation gate enforced in the before-tool-call hook.
- Extend
update_planwith merge-mode, richer step fields, completion detection/events, and add plan persistence primitives (PlanStore) + plan hydration utilities. - Add skill plan-template parsing/seeding and thread plan-mode state through the embedded runner/tool assembly.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/infra/agent-events.ts | Expands agent event shapes and run context to carry plan/plan-mode state and subagent gating metadata. |
| src/gateway/sessions-patch.ts | Adds sessions.patch handling for planMode toggling with config gating. |
| src/gateway/protocol/schema/sessions.ts | Extends sessions.patch schema with planMode wire field. |
| src/config/zod-schema.ts | Adds skills.limits.maxPlanTemplateSteps config validation. |
| src/config/zod-schema.agent-defaults.ts | Adds agent defaults for auto-continue and plan-mode enablement. |
| src/config/types.skills.ts | Documents/defines maxPlanTemplateSteps skill limit. |
| src/config/types.agent-defaults.ts | Adds config types for auto-continue and plan mode toggle. |
| src/config/sessions/types.ts | Adds SessionEntry.planMode structural state and refactors plugin status/trace line extraction. |
| src/agents/tools/update-plan-tool.ts | Adds merge mode, richer step schema, completion emission, and event payload improvements. |
| src/agents/tools/update-plan-tool.parity.test.ts | Adds parity + merge-mode behavior tests for update_plan. |
| src/agents/tools/enter-plan-mode-tool.ts | Introduces enter_plan_mode tool. |
| src/agents/tools/exit-plan-mode-tool.ts | Introduces exit_plan_mode tool (approval request trigger). |
| src/agents/tool-description-presets.ts | Adds descriptions/summaries for new plan-mode tools. |
| src/agents/tool-catalog.ts | Registers plan-mode tools in catalog for policy/profile filtering. |
| src/agents/test-helpers/fast-openclaw-tools-sessions.ts | Updates tool mocks for new update_plan exports/options. |
| src/agents/skills/workspace.ts | Carries plan templates into skill snapshots for snapshot-backed runs. |
| src/agents/skills/types.ts | Adds plan template types and snapshot resolvedPlanTemplates. |
| src/agents/skills/skill-planner.ts | Builds normalized plan seed payloads from skill templates. |
| src/agents/skills/skill-planner.test.ts | Adds unit tests for plan-template payload building and seeding integration. |
| src/agents/skills/frontmatter.ts | Parses plan-template / planTemplate from SKILL.md frontmatter. |
| src/agents/skills/frontmatter.test.ts | Adds tests for plan-template frontmatter parsing behavior. |
| src/agents/plan-store.ts | Introduces disk-persisted plan store with confinement + locking hardening. |
| src/agents/plan-store.test.ts | Adds tests for PlanStore read/write/lock/confinement. |
| src/agents/plan-mode/* | Adds plan-mode types, approval resolution, mutation gate, and tests. |
| src/agents/plan-hydration.ts | Adds post-compaction plan hydration formatting. |
| src/agents/plan-hydration.test.ts | Adds tests for hydration formatting/filtering. |
| src/agents/pi-tools.ts | Threads planMode into hook context via tool assembly. |
| src/agents/pi-tools.before-tool-call.ts | Enforces mutation gate during plan mode before plugin hookRunner. |
| src/agents/pi-embedded-runner/skills-runtime.ts | Wires skill plan-template seeding + snapshot fallback. |
| src/agents/pi-embedded-runner/run/params.ts | Threads planMode through embedded runner params. |
| src/agents/pi-embedded-runner/run/incomplete-turn.ts | Adjusts retry limits and adds plan-mode-aware retry/ACK behavior. |
| src/agents/pi-embedded-runner/run.ts | Adds auto-continue logic, prompt-addition de-dupe, and threads planMode into attempt setup. |
| src/agents/pi-embedded-runner/run.incomplete-turn.test.ts | Updates tests for retry-limit + auto-continue and plan-mode-aware retry behavior. |
| src/agents/openclaw-tools.ts | Threads runId into update_plan and registers plan-mode tools. |
| src/agents/openclaw-tools.registration.ts | Adds agents.defaults.planMode.enabled gating for plan-mode tools. |
| apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift | Updates SessionsPatchParams fields for protocol changes. |
| apps/macos/Sources/OpenClawProtocol/GatewayModels.swift | Same as shared Swift protocol models. |
Comments suppressed due to low confidence (1)
src/agents/pi-embedded-runner/run.ts:312
resolveHookModelSelectionsupportsattachments(andbuildBeforeModelResolveAttachments()still exists), but this call no longer passes image attachments. That breaks before_model_resolve hooks that select provider/model based on whether images are present. Passattachments: buildBeforeModelResolveAttachments(params.images)to preserve the hook contract.
const hookSelection = await resolveHookModelSelection({
prompt: params.prompt,
provider,
modelId,
hookRunner,
hookContext: hookCtx,
});
| planningOnlyRetryAttempts += 1; | ||
| planningOnlyRetryInstruction = nextPlanningOnlyRetryInstruction; | ||
| planningOnlyRetryInstruction = resolveEscalatingPlanningRetryInstruction( | ||
| planningOnlyRetryAttempts - 1, | ||
| ); |
| public let planmode: AnyCodable? | ||
| public let planapproval: AnyCodable? | ||
| public let lastplansteps: [[String: AnyCodable]]? | ||
|
|
| public let groupactivation: AnyCodable? | ||
| public let planmode: AnyCodable? | ||
| public let planapproval: AnyCodable? | ||
| public let lastplansteps: [[String: AnyCodable]]? |
| createSessionsSpawnTool({ | ||
| agentSessionKey: options?.agentSessionKey, | ||
| agentChannel: options?.agentChannel, | ||
| agentAccountId: options?.agentAccountId, | ||
| agentTo: options?.agentTo, | ||
| agentThreadId: options?.agentThreadId, | ||
| agentGroupId: options?.agentGroupId, | ||
| agentGroupChannel: options?.agentGroupChannel, | ||
| agentGroupSpace: options?.agentGroupSpace, | ||
| agentMemberRoleIds: options?.agentMemberRoleIds, | ||
| sandboxed: options?.sandboxed, | ||
| requesterAgentIdOverride: options?.requesterAgentIdOverride, | ||
| workspaceDir: spawnWorkspaceDir, | ||
| }), |
| export function resolveAckExecutionFastPathInstruction(params: { | ||
| provider?: string; | ||
| modelId?: string; | ||
| prompt: string; | ||
| /** Plan mode disables ack fast-path: a "do it" reply is the approval signal, not a planning skip. */ | ||
| planModeActive?: boolean; | ||
| }): string | null { | ||
| if ( | ||
| !shouldApplyPlanningOnlyRetryGuard({ | ||
| provider: params.provider, | ||
| modelId: params.modelId, | ||
| planModeActive: params.planModeActive, | ||
| }) || |
Greptile SummaryThis PR introduces the plan-mode foundation and core backend MVP: a persistent Confidence Score: 4/5Safe to merge after addressing the resolvePlanApproval none-state bypass; remaining comments are P2 quality improvements. One P1 finding: resolvePlanApproval allows approve/edit on the none state (no pending proposal) when expectedApprovalId is omitted, which could silently unlock mutations. The fix is small and targeted. All other findings are P2 with no current observable misbehavior. src/agents/plan-mode/approval.ts — the terminal-state guard and approve/edit action precondition logic. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/plan-mode/approval.ts
Line: 72-81
Comment:
**`"none"` state allows approve/edit without a pending proposal**
The terminal-state guard explicitly passes `"none"` through alongside `"pending"` and `"rejected"`, and the `"approve"`/`"edit"` actions have no additional check for the current approval state. Unlike `"timeout"` (which is already gated on `current.approval !== "pending"`), an `"approve"` or `"edit"` action against a session in the `"none"` state will succeed and flip `mode` to `"normal"` — unlocking all mutation tools — without any plan ever being proposed.
The `expectedApprovalId` guard provides protection only when the caller provides the field. Since the parameter is optional and `current.approvalId` is `undefined` when approval is `"none"`, a caller that omits `expectedApprovalId` (or a future code path that calls `resolvePlanApproval` directly without it) would bypass the gate entirely.
Adding `"none"` to the terminal-state set, or gating `"approve"` and `"edit"` on `current.approval === "pending"`, closes the gap without changing behavior for the normal flow.
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/mutation-gate.ts
Line: 57-81
Comment:
**`echo` and `printenv` in read-only allowlist can surface env secrets in plan mode**
The prefix check itself is correct (word-boundary via trailing space). However, `"echo"` and `"printenv"` both emit environment variables directly — e.g., `echo $ANTHROPIC_API_KEY` or `printenv MY_SECRET` would be allowed. While the agent already has env access, minimising surface area in plan mode (where the agent is supposed to only read/plan, not act) may be preferable. Consider removing or documenting the explicit decision.
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-store.ts
Line: 261
Comment:
**String prefix vs. path prefix in `confine()` while condition**
`probe.startsWith(this.baseDir)` is a raw string prefix check. If `baseDir` is `/home/user/plans`, a probe of `/home/user/plans2/...` satisfies the check as a string but is not lexically under `baseDir`. Security is preserved in practice because the initial `path.relative` guard (path-segment–aware) already throws for any escaping target before this loop is entered. Using `probe.startsWith(this.baseDir + path.sep) || probe === this.baseDir` makes the invariant explicit and removes the edge-case ambiguity.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(plan-mode): split PR3 core backend ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| current.approval !== "pending" && | ||
| current.approval !== "rejected" && | ||
| current.approval !== "none" | ||
| ) { | ||
| return current; | ||
| } | ||
| if (action === "timeout" && current.approval !== "pending") { | ||
| return current; | ||
| } |
There was a problem hiding this comment.
"none" state allows approve/edit without a pending proposal
The terminal-state guard explicitly passes "none" through alongside "pending" and "rejected", and the "approve"/"edit" actions have no additional check for the current approval state. Unlike "timeout" (which is already gated on current.approval !== "pending"), an "approve" or "edit" action against a session in the "none" state will succeed and flip mode to "normal" — unlocking all mutation tools — without any plan ever being proposed.
The expectedApprovalId guard provides protection only when the caller provides the field. Since the parameter is optional and current.approvalId is undefined when approval is "none", a caller that omits expectedApprovalId (or a future code path that calls resolvePlanApproval directly without it) would bypass the gate entirely.
Adding "none" to the terminal-state set, or gating "approve" and "edit" on current.approval === "pending", closes the gap without changing behavior for the normal flow.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-mode/approval.ts
Line: 72-81
Comment:
**`"none"` state allows approve/edit without a pending proposal**
The terminal-state guard explicitly passes `"none"` through alongside `"pending"` and `"rejected"`, and the `"approve"`/`"edit"` actions have no additional check for the current approval state. Unlike `"timeout"` (which is already gated on `current.approval !== "pending"`), an `"approve"` or `"edit"` action against a session in the `"none"` state will succeed and flip `mode` to `"normal"` — unlocking all mutation tools — without any plan ever being proposed.
The `expectedApprovalId` guard provides protection only when the caller provides the field. Since the parameter is optional and `current.approvalId` is `undefined` when approval is `"none"`, a caller that omits `expectedApprovalId` (or a future code path that calls `resolvePlanApproval` directly without it) would bypass the gate entirely.
Adding `"none"` to the terminal-state set, or gating `"approve"` and `"edit"` on `current.approval === "pending"`, closes the gap without changing behavior for the normal flow.
How can I resolve this? If you propose a fix, please make it concise.| const READ_ONLY_EXEC_PREFIXES = [ | ||
| "ls", | ||
| "cat", | ||
| "pwd", | ||
| "git status", | ||
| "git log", | ||
| "git diff", | ||
| "git show", | ||
| "which", | ||
| "find", | ||
| "grep", | ||
| "rg", | ||
| "head", | ||
| "tail", | ||
| "wc", | ||
| "file", | ||
| "stat", | ||
| "du", | ||
| "df", | ||
| "echo", | ||
| "printenv", | ||
| "whoami", | ||
| "hostname", | ||
| "uname", | ||
| ]; |
There was a problem hiding this comment.
echo and printenv in read-only allowlist can surface env secrets in plan mode
The prefix check itself is correct (word-boundary via trailing space). However, "echo" and "printenv" both emit environment variables directly — e.g., echo $ANTHROPIC_API_KEY or printenv MY_SECRET would be allowed. While the agent already has env access, minimising surface area in plan mode (where the agent is supposed to only read/plan, not act) may be preferable. Consider removing or documenting the explicit decision.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-mode/mutation-gate.ts
Line: 57-81
Comment:
**`echo` and `printenv` in read-only allowlist can surface env secrets in plan mode**
The prefix check itself is correct (word-boundary via trailing space). However, `"echo"` and `"printenv"` both emit environment variables directly — e.g., `echo $ANTHROPIC_API_KEY` or `printenv MY_SECRET` would be allowed. While the agent already has env access, minimising surface area in plan mode (where the agent is supposed to only read/plan, not act) may be preferable. Consider removing or documenting the explicit decision.
How can I resolve this? If you propose a fix, please make it concise.| // walk up). If it resolves outside baseDir, reject — a parent | ||
| // symlink would redirect us elsewhere. | ||
| let probe = path.dirname(target); | ||
| while (probe.startsWith(this.baseDir)) { |
There was a problem hiding this comment.
confine() while condition
probe.startsWith(this.baseDir) is a raw string prefix check. If baseDir is /home/user/plans, a probe of /home/user/plans2/... satisfies the check as a string but is not lexically under baseDir. Security is preserved in practice because the initial path.relative guard (path-segment–aware) already throws for any escaping target before this loop is entered. Using probe.startsWith(this.baseDir + path.sep) || probe === this.baseDir makes the invariant explicit and removes the edge-case ambiguity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-store.ts
Line: 261
Comment:
**String prefix vs. path prefix in `confine()` while condition**
`probe.startsWith(this.baseDir)` is a raw string prefix check. If `baseDir` is `/home/user/plans`, a probe of `/home/user/plans2/...` satisfies the check as a string but is not lexically under `baseDir`. Security is preserved in practice because the initial `path.relative` guard (path-segment–aware) already throws for any escaping target before this loop is entered. Using `probe.startsWith(this.baseDir + path.sep) || probe === this.baseDir` makes the invariant explicit and removes the edge-case ambiguity.
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: 76d9b14550
ℹ️ 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 hookSelection = await resolveHookModelSelection({ | ||
| prompt: params.prompt, | ||
| attachments: buildBeforeModelResolveAttachments(params.images), | ||
| provider, |
There was a problem hiding this comment.
The resolveHookModelSelection call now omits attachments, so before_model_resolve hooks lose image metadata even though the hook event type still supports it. In runs with image inputs, hooks that select a vision-capable provider/model based on attachments will now receive only prompt text and can choose the wrong model, causing avoidable multimodal failures; this regression is introduced by dropping the buildBeforeModelResolveAttachments(params.images) argument at this callsite.
Useful? React with 👍 / 👎.
| createSessionsSpawnTool({ | ||
| agentSessionKey: options?.agentSessionKey, | ||
| agentChannel: options?.agentChannel, | ||
| agentAccountId: options?.agentAccountId, |
There was a problem hiding this comment.
This adds a second createSessionsSpawnTool(...) outside the existing embedded ? [] : [...] guard, so non-embedded runs now expose duplicate sessions_spawn tools and embedded runs unexpectedly expose sessions_spawn at all. Because core tools are forwarded without deduplication, this changes runtime tool surface and can enable spawn behavior in contexts that were previously intentionally blocked.
Useful? React with 👍 / 👎.
| content: [], | ||
| details: { | ||
| status: "entered" as const, | ||
| mode: "plan" as const, |
There was a problem hiding this comment.
The tool returns status: "entered"/mode: "plan" but performs no state transition, and in this commit there is no runtime handler that applies a sessions.patch for enter_plan_mode. As a result, the agent is told plan mode was entered while SessionEntry.planMode remains unchanged, so the mutation gate never arms and plan-mode safety can be bypassed in normal execution.
Useful? React with 👍 / 👎.
|
Closed for sequential rollout — please disregard cumulative diff. This PR was opened with The fix: sequential rollout. Each Currently open:
Next: when 1/9 merges, This PR's branch is preserved on the fork and will be reopened with a clean diff in its turn. |
Summary
[Plan Mode 2/8] — part of the 8-part decomposition of #68939, stacking on #69449 (Part 1).
Stacks on: previous part (see [Plan Mode 1/8] / [Plan Mode 1/9 #69449]).
Stacks logically on Part 1 (#69449); GitHub diff is shown vs main. Combines Parts 2 (plan-state foundation: storage, types) + Part 3 (core backend MVP: gates, nudges).
Lines: ~5615
Diff note
This PR is opened cross-repo (head:
100yenadmin:restack/68939-pr2-3-foundation-backend) againstopenclaw:main. Because GitHub doesn't support stacked-PR bases that point at fork refs, the displayed diff cumulatively includes all previous parts in the stack. Reviewers should focus on the changes specific to this part — see commit list for the new content beyond what previous parts established.Test status
After post-v2026.4.21 rebase: all targeted plan-mode tests pass (53/53 in unit-fast for approval + integration suites). The full suite has pre-existing vitest workspace project-name conflicts unrelated to this work.
Related