[Plan Mode 1/6] Plan-state foundation#70031
Conversation
Greptile SummaryThis PR adds the plan-mode data layer:
Confidence Score: 2/5Not safe to merge — two P0 import errors will break the TypeScript build on every file that transitively depends on openclaw-tools or attempt.ts. The plan-store and hydration logic is well-implemented, but the PR adds import statements in both openclaw-tools.ts and attempt.ts that reference files absent from the repository. These are not deferred work items — they are direct imports that the TypeScript compiler will fail on immediately, blocking the entire build. src/agents/openclaw-tools.ts and src/agents/pi-embedded-runner/run/attempt.ts reference modules that do not exist in the head commit. The entire src/agents/plan-mode/ directory is missing.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { applyNodesToolWorkspaceGuard } from "./openclaw-tools.nodes-workspace-guard.js"; | ||
| import { | ||
| collectPresentOpenClawTools, | ||
| isPlanModeToolsEnabledForOpenClawTools, | ||
| isUpdatePlanToolEnabledForOpenClawTools, | ||
| } from "./openclaw-tools.registration.js"; | ||
| import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; | ||
| import type { SpawnedToolContext } from "./spawned-context.js"; | ||
| import type { ToolFsPolicy } from "./tool-fs-policy.js"; | ||
| import { createAgentsListTool } from "./tools/agents-list-tool.js"; | ||
| import { createAskUserQuestionTool } from "./tools/ask-user-question-tool.js"; | ||
| import { createCanvasTool } from "./tools/canvas-tool.js"; | ||
| import type { AnyAgentTool } from "./tools/common.js"; | ||
| import { createCronTool } from "./tools/cron-tool.js"; | ||
| import { createEmbeddedCallGateway } from "./tools/embedded-gateway-stub.js"; | ||
| import { createEnterPlanModeTool } from "./tools/enter-plan-mode-tool.js"; | ||
| import { createExitPlanModeTool } from "./tools/exit-plan-mode-tool.js"; | ||
| import { createGatewayTool } from "./tools/gateway-tool.js"; | ||
| import { createImageGenerateTool } from "./tools/image-generate-tool.js"; | ||
| import { createImageTool } from "./tools/image-tool.js"; | ||
| import { createMessageTool } from "./tools/message-tool.js"; | ||
| import { createMusicGenerateTool } from "./tools/music-generate-tool.js"; | ||
| import { createNodesTool } from "./tools/nodes-tool.js"; | ||
| import { createPdfTool } from "./tools/pdf-tool.js"; | ||
| import { createPlanModeStatusTool } from "./tools/plan-mode-status-tool.js"; | ||
| import { createSessionStatusTool } from "./tools/session-status-tool.js"; |
There was a problem hiding this comment.
openclaw-tools.ts imports five symbols that do not exist anywhere in the head commit (9cc645655):
isPlanModeToolsEnabledForOpenClawToolsfrom./openclaw-tools.registration.js— not exported byopenclaw-tools.registration.tscreateAskUserQuestionToolfrom./tools/ask-user-question-tool.jscreateEnterPlanModeToolfrom./tools/enter-plan-mode-tool.jscreateExitPlanModeToolfrom./tools/exit-plan-mode-tool.jscreatePlanModeStatusToolfrom./tools/plan-mode-status-tool.js
None of those four tool files appear anywhere in the repository tree at the PR's head SHA. tsc and bundlers will error on these unresolved imports, breaking the build for every downstream consumer of openclaw-tools.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/openclaw-tools.ts
Line: 9-34
Comment:
**Missing modules — compilation will fail**
`openclaw-tools.ts` imports five symbols that do not exist anywhere in the head commit (`9cc645655`):
- `isPlanModeToolsEnabledForOpenClawTools` from `./openclaw-tools.registration.js` — not exported by `openclaw-tools.registration.ts`
- `createAskUserQuestionTool` from `./tools/ask-user-question-tool.js`
- `createEnterPlanModeTool` from `./tools/enter-plan-mode-tool.js`
- `createExitPlanModeTool` from `./tools/exit-plan-mode-tool.js`
- `createPlanModeStatusTool` from `./tools/plan-mode-status-tool.js`
None of those four tool files appear anywhere in the repository tree at the PR's head SHA. `tsc` and bundlers will error on these unresolved imports, breaking the build for every downstream consumer of `openclaw-tools`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in bf19766b5a. Removed the four forward-reference imports + the registration block from openclaw-tools.ts. The plan-mode tool implementations and the isPlanModeToolsEnabledForOpenClawTools helper land in [Plan Mode 2/6] (#70066) where they belong; this foundation PR ships the data layer (schema, plan-store, plan-hydration, types) only. Sequential merge of 1/6 → 2/6 produces the wired-together state on main.
| } | ||
|
|
||
| export class PlanStore { | ||
| /** Realpath-resolved base directory — used for confinement checks. */ | ||
| private readonly baseDir: string; | ||
|
|
||
| constructor(baseDir: string) { | ||
| // Resolve symlinks at construction. If baseDir doesn't exist yet, fall | ||
| // back to the literal path — confinement check at use time will still | ||
| // reject targets that escape this resolved root. | ||
| let resolved: string; | ||
| try { | ||
| resolved = realpathSync(baseDir); | ||
| } catch { | ||
| resolved = path.resolve(baseDir); | ||
| } | ||
| this.baseDir = resolved; | ||
| } | ||
|
|
There was a problem hiding this comment.
startsWith is a string prefix check, not a path prefix check
while (probe.startsWith(this.baseDir)) {If baseDir is /home/user/.openclaw/plans, a probe that walked up to a sibling like /home/user/.openclaw/plans-backup would also satisfy startsWith. The correct guard is:
while (probe === this.baseDir || probe.startsWith(this.baseDir + path.sep)) {The lexical rel.startsWith("..") check at the top of confine() prevents a path that escapes baseDir from reaching the while loop, so this is not currently exploitable — but the guard is semantically wrong.
| } | |
| export class PlanStore { | |
| /** Realpath-resolved base directory — used for confinement checks. */ | |
| private readonly baseDir: string; | |
| constructor(baseDir: string) { | |
| // Resolve symlinks at construction. If baseDir doesn't exist yet, fall | |
| // back to the literal path — confinement check at use time will still | |
| // reject targets that escape this resolved root. | |
| let resolved: string; | |
| try { | |
| resolved = realpathSync(baseDir); | |
| } catch { | |
| resolved = path.resolve(baseDir); | |
| } | |
| this.baseDir = resolved; | |
| } | |
| while (probe === this.baseDir || probe.startsWith(this.baseDir + path.sep)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-store.ts
Line: 218-236
Comment:
**`startsWith` is a string prefix check, not a path prefix check**
```ts
while (probe.startsWith(this.baseDir)) {
```
If `baseDir` is `/home/user/.openclaw/plans`, a probe that walked up to a sibling like `/home/user/.openclaw/plans-backup` would also satisfy `startsWith`. The correct guard is:
```ts
while (probe === this.baseDir || probe.startsWith(this.baseDir + path.sep)) {
```
The lexical `rel.startsWith("..")` check at the top of `confine()` prevents a path that escapes `baseDir` from reaching the while loop, so this is not currently exploitable — but the guard is semantically wrong.
```suggestion
while (probe === this.baseDir || probe.startsWith(this.baseDir + path.sep)) {
```
How can I resolve this? If you propose a fix, please make it concise.| async lock(namespace: string): Promise<() => Promise<void>> { | ||
| const lockFile = this.lockPath(namespace); | ||
| const dir = path.dirname(lockFile); | ||
| await fs.mkdir(dir, { recursive: true, mode: 0o700 }); | ||
|
|
||
| // Generate a unique lock token so release can verify ownership. |
There was a problem hiding this comment.
Mutating i mid-loop to "grant one extra iteration" is hard to reason about. The for-loop increment runs after continue, so i -= 1 followed by continue effectively replays the last iteration once — but only when stale eviction happens on the final retry, which is an edge case. A clearer approach is to break out of the loop into a separate tail-acquisition attempt after the unlink, or increase maxRetries by 1 and document the extra slot.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-store.ts
Line: 390-395
Comment:
**Loop-counter mutation is fragile**
Mutating `i` mid-loop to "grant one extra iteration" is hard to reason about. The for-loop increment runs after `continue`, so `i -= 1` followed by `continue` effectively replays the last iteration once — but only when stale eviction happens on the final retry, which is an edge case. A clearer approach is to break out of the loop into a separate tail-acquisition attempt after the `unlink`, or increase `maxRetries` by 1 and document the extra slot.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
Establishes foundational “plan mode” plumbing across the agent runtime: enriches plan/approval event contracts, adds durable plan persistence primitives, and wires plan-aware behaviors into tools and the embedded runner.
Changes:
- Expanded agent event/run-context contracts to carry structured plan/approval state and subagent-gate tracking.
- Added cross-session plan persistence utilities (
PlanStore) and post-compaction plan hydration formatting. - Enhanced
update_plan(merge mode, richer step fields, completion signaling) and introduced skill plan-template seeding + related config/schema additions.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/infra/agent-events.ts | Extends plan/approval event payloads and run-context state used by plan mode + subagent gating. |
| src/config/zod-schema.ts | Adds skills.limits.maxPlanTemplateSteps schema. |
| src/config/types.skills.ts | Adds maxPlanTemplateSteps to skills limits typing/docs. |
| src/agents/tools/update-plan-tool.ts | Adds merge mode, richer step schema, event emission enhancements, and completion detection. |
| src/agents/tools/update-plan-tool.parity.test.ts | New parity/merge-mode coverage for update_plan. |
| src/agents/test-helpers/fast-openclaw-tools-sessions.ts | Updates mocks for new update_plan exports/signature. |
| src/agents/skills/workspace.ts | Includes per-skill plan templates in snapshots so seeding works in snapshot-backed runs. |
| src/agents/skills/types.ts | Adds planTemplate types and snapshot plumbing. |
| src/agents/skills/skill-planner.ts | New plan-template payload builder + normalization helpers. |
| src/agents/skills/skill-planner.test.ts | Tests for plan-template payload building and seeding behavior. |
| src/agents/skills/frontmatter.ts | Parses plan-template / planTemplate frontmatter into structured templates. |
| src/agents/skills/frontmatter.test.ts | Tests for plan-template frontmatter parsing behaviors. |
| src/agents/plan-store.ts | New persistent plan store with namespace validation + file locking. |
| src/agents/plan-store.test.ts | Tests for PlanStore read/write/lock and confinement/validation cases. |
| src/agents/plan-hydration.ts | Formats active plan steps for post-compaction injection. |
| src/agents/plan-hydration.test.ts | Tests hydration formatting rules. |
| src/agents/pi-tools.ts | Threads plan-mode state/accessors into before-tool-call hook context. |
| src/agents/pi-embedded-runner/skills-runtime.ts | Implements skill plan-template resolution + emits seed plan events. |
| src/agents/pi-embedded-runner/run/attempt.ts | Seeds plan templates before first turn; prepends plan-mode prompt block; threads plan-mode context into tools. |
| src/agents/pi-embedded-runner/run/attempt.spawn-workspace.test-support.ts | Stubs plan-template seeding for spawn-workspace tests. |
| src/agents/openclaw-tools.ts | Registers plan-mode tools and threads runId into update_plan and sessions_spawn. |
| 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, | ||
| // PR-8 follow-up: thread runId so the spawn tool can read | ||
| // AgentRunContext.inPlanMode (for cleanup:keep override) and | ||
| // add the child runId to AgentRunContext.openSubagentRunIds | ||
| // (so exit_plan_mode can block on pending children). | ||
| runId: options?.runId, | ||
| }), |
There was a problem hiding this comment.
createSessionsSpawnTool is being registered twice (once inside the embedded ? [] : [...] block and again unconditionally below). This will produce duplicate tool names (likely sessions_spawn) and can break tool registration / conflict detection at runtime. Consolidate to a single registration and thread runId into the existing non-embedded createSessionsSpawnTool call instead of adding a second instance.
There was a problem hiding this comment.
Fixed in bf19766b5a. Same fix as Codex's P1 on :353 — removed the unconditional duplicate createSessionsSpawnTool block. The conditional registration inside the embedded ? [] : [...] branch is the single source.
| // PR-8 follow-up: return non-empty content. Empty content arrays | ||
| // trip third-party transcript-pairing extensions (lossless-claw) | ||
| // which inject `[lossless-claw] missing tool result` placeholders | ||
| // into the agent's read-time context, polluting it with synthetic | ||
| // errors. Non-empty content satisfies the pairing check and keeps | ||
| // the agent's view of past turns clean. | ||
| const stepCount = plan.length; | ||
| const summaryLine = allTerminal | ||
| ? `Plan complete (${stepCount} ${stepCount === 1 ? "step" : "steps"}).` | ||
| : `Plan updated (${stepCount} ${stepCount === 1 ? "step" : "steps"}).`; | ||
| return { | ||
| content: [], | ||
| content: [{ type: "text" as const, text: summaryLine }], | ||
| details: { |
There was a problem hiding this comment.
update_plan now returns a non-empty content array. There are existing unit tests (e.g. src/agents/tools/update-plan-tool.test.ts) and potentially other callers that assert content: [] for this tool; those will fail and/or treat the output as a behavior change. Update the affected tests/consumers (or keep content empty and solve the transcript-pairing issue elsewhere) so the tool contract is consistent across the codebase.
| // `src/infra/fs-safe.ts:72-84`). On Windows the symlink rejection | ||
| // degrades to none — Windows symlinks to outside baseDir would still be | ||
| // caught by the realpath-based `confine()` walk. |
There was a problem hiding this comment.
The comment claims that on Windows (where O_NOFOLLOW is unavailable) symlink rejection “would still be caught by the realpath-based confine() walk.” confine() only realpaths existing ancestors (directories), so a leaf symlink like <namespace>/plan.json -> C:\... would not be detected on Windows and read() would follow it. Either tighten the comment to reflect the limitation, or add an explicit realpath confinement check for the full file path on non-O_NOFOLLOW platforms (accepting the remaining TOCTOU risk) to reduce exposure.
| // `src/infra/fs-safe.ts:72-84`). On Windows the symlink rejection | |
| // degrades to none — Windows symlinks to outside baseDir would still be | |
| // caught by the realpath-based `confine()` walk. | |
| // `src/infra/fs-safe.ts:72-84`). On Windows this provides no leaf-symlink | |
| // rejection for the target file; any realpath-based confinement elsewhere | |
| // only covers existing ancestors and does not prevent a symlinked | |
| // `<namespace>/plan.json` from being followed. |
| // by later commits. | ||
| const planModeFeatureEnabled = params.config?.agents?.defaults?.planMode?.enabled === true; | ||
| const planModeAppendPrompt = | ||
| params.planMode === "plan" |
There was a problem hiding this comment.
planModeAppendPrompt injects the full “PLAN MODE ACTIVE” rules whenever params.planMode === "plan", even if agents.defaults.planMode.enabled is false. If a session ends up with planMode: "plan" while the feature flag is off (stale session state, config flip, etc.), this would still change agent behavior despite the flag. Consider gating the active-branch injection on planModeFeatureEnabled (or assert/normalize params.planMode earlier) so enabled: false reliably implies no prompt-level behavior change.
| params.planMode === "plan" | |
| planModeFeatureEnabled && params.planMode === "plan" |
![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: 9cc6456552
ℹ️ 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 type { SpawnedToolContext } from "./spawned-context.js"; | ||
| import type { ToolFsPolicy } from "./tool-fs-policy.js"; | ||
| import { createAgentsListTool } from "./tools/agents-list-tool.js"; | ||
| import { createAskUserQuestionTool } from "./tools/ask-user-question-tool.js"; |
There was a problem hiding this comment.
This commit adds imports that are not resolvable in this tree, so src/agents/openclaw-tools.ts cannot be loaded. In this commit, src/agents/openclaw-tools.registration.ts does not export isPlanModeToolsEnabledForOpenClawTools, and the imported modules ./tools/ask-user-question-tool.js, ./tools/enter-plan-mode-tool.js, ./tools/exit-plan-mode-tool.js, and ./tools/plan-mode-status-tool.js are not present. Because these are static ESM imports, any runtime path importing createOpenClawTools will fail before execution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf19766b5a. Same root cause as Greptile's P0 on :29 — the foundation PR was importing tool factories that don't exist on this branch. Removed the imports + the registration block. Plan-mode tools land in [Plan Mode 2/6] (#70066). See the commit message for details.
| createSessionsSpawnTool({ | ||
| agentSessionKey: options?.agentSessionKey, | ||
| agentChannel: options?.agentChannel, | ||
| agentAccountId: options?.agentAccountId, |
There was a problem hiding this comment.
The tool list now includes createSessionsSpawnTool(...) twice for non-embedded runs (once inside the !embedded block and again unconditionally here). That produces duplicate sessions_spawn definitions, which makes tool selection ambiguous and can bypass the new run-scoped behavior because the earlier registration does not pass runId. Keep only one sessions_spawn registration (the runId-aware one).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf19766b5a. Removed the unconditional createSessionsSpawnTool block at line ~345; the conditional registration inside embedded ? [] : [...] (line ~309) stays as the single registration.
100yenadmin
changed the title
100yenadmin
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf19766b5a
ℹ️ 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".
| @@ -731,6 +749,7 @@ export function buildWorkspaceSkillSnapshot( | |||
| })), | |||
| ...(skillFilter === undefined ? {} : { skillFilter }), | |||
| resolvedSkills, | |||
| ...(resolvedPlanTemplates.length > 0 ? { resolvedPlanTemplates } : {}), | |||
There was a problem hiding this comment.
buildWorkspaceSkillSnapshot omits resolvedPlanTemplates when no templates are present, but resolveEmbeddedRunSkillEntries treats resolvedPlanTemplates === undefined as an old snapshot and forces shouldLoadSkillEntries=true (src/agents/pi-embedded-runner/skills-runtime.ts:33-37). That means every newly built snapshot with zero plan templates is misclassified and reloads skills from disk on each attempt, defeating the snapshot-backed fast path in the common case and adding avoidable hot-path I/O.
Useful? React with 👍 / 👎.
| if (verifiedCriteria && !acceptanceCriteria) { | ||
| throw new ToolInputError( | ||
| `plan[${index}].verifiedCriteria requires plan[${index}].acceptanceCriteria to be set`, | ||
| ); |
There was a problem hiding this comment.
The parser currently throws if verifiedCriteria is provided without acceptanceCriteria, but in merge mode those acceptance criteria can already exist on the previous step and are preserved during merge. This blocks incremental verification updates (e.g. merge:true patching only verifiedCriteria) even though the tool description says verification should be updated incrementally, causing avoidable ToolInputError failures for normal progress updates.
Useful? React with 👍 / 👎.
bf19766 to
a87df8d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a87df8d7a2
ℹ️ 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".
| modelRegistry: params.modelRegistry, | ||
| model: params.model, | ||
| thinkingLevel: mapThinkingLevel(params.thinkLevel), | ||
| tools: builtInTools, |
There was a problem hiding this comment.
createAgentSession is now called with tools: builtInTools, but splitSdkTools provides an empty built-in list in this path, so the session receives an empty tool allowlist. In this runtime, tools is the name allowlist used to authorize execution, so custom tools registered in customTools become non-executable even though their definitions are present. This breaks normal runs that depend on OpenClaw tools (e.g., update_plan, read, exec).
Useful? React with 👍 / 👎.
| const shouldUseWebSocketTransport = shouldUseOpenAIWebSocketTransport({ | ||
| provider: params.provider, | ||
| modelApi: params.model.api, | ||
| modelBaseUrl: params.model.baseUrl, | ||
| }); |
There was a problem hiding this comment.
The websocket gate is invoked without modelBaseUrl, so shouldUseOpenAIWebSocketTransport can no longer distinguish custom OpenAI-compatible endpoints from the public default. For provider=openai and modelApi=openai-responses, this can incorrectly force websocket transport even when a custom base URL/proxy is configured, causing requests to bypass the configured endpoint and fail.
Useful? React with 👍 / 👎.
| }) | ||
| .strict() | ||
| .optional(), | ||
| surfaces: z | ||
| .record( | ||
| z.string(), | ||
| z | ||
| .object({ | ||
| silentReply: SilentReplyPolicyConfigSchema.optional(), | ||
| silentReplyRewrite: SilentReplyRewriteConfigSchema.optional(), | ||
| }) | ||
| .strict(), | ||
| ) | ||
| .optional(), | ||
| }) |
There was a problem hiding this comment.
surfaces support in root config schema
The root OpenClawSchema object now closes immediately after plugins while remaining .strict(), which removes the surfaces key from accepted config. Existing configs that set surfaces.* (used by silent-reply surface policy resolution) will now fail validation as unknown keys, turning a previously valid runtime configuration into a startup error.
Useful? React with 👍 / 👎.
Cherry-pick / file-copy of 30 files from feat/plan-channel-parity-merged-upstream (merged-head 651a22d) onto current upstream/main. This re-cut absorbs the upstream merge into plan-mode dev (Option β; merge commit b1ead74) plus the post-merge build/test fixes (commit 651a22d). v4 scope additions vs v3: - src/agents/agent-scope.{ts,test.ts} - src/agents/system-prompt.ts - src/agents/system-prompt-contribution.ts - src/agents/pi-embedded-runner/system-prompt.ts - src/agents/system-prompt-gpt5-boot-reorder.test.ts - src/agents/skills.buildworkspaceskillsnapshot.test.ts - extensions/openai/prompt-overlay.ts - extensions/openai/index.test.ts These were the GPT-5 prompt foundation files originally deferred to closed #69449. They turned out to be load-bearing for plan-mode (resolveAgentAutoContinue, resolveAgentMaxIterations referenced from pi-embedded-runner/run.ts) and have been folded into this rollout per the v4 plan.
a87df8d to
6e98957
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e98957648
ℹ️ 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 { PLAN_ARCHETYPE_PROMPT } from "../../plan-mode/plan-archetype-prompt.js"; | ||
| import { PLAN_MODE_REFERENCE_CARD } from "../../plan-mode/reference-card.js"; |
There was a problem hiding this comment.
attempt.ts now statically imports ../../plan-mode/plan-archetype-prompt.js and ../../plan-mode/reference-card.js, but this commit does not add those modules (there is no src/agents/plan-mode/ tree at this revision). Because these are top-level ESM imports, any runtime path that loads runEmbeddedAttempt will fail during module resolution before execution starts.
Useful? React with 👍 / 👎.
| stablePrefix: OPENAI_GPT5_BEHAVIOR_CONTRACT, | ||
| sectionOverrides: | ||
| params.mode === "friendly" ? { interaction_style: OPENAI_FRIENDLY_PROMPT_OVERLAY } : {}, | ||
| stablePrefix: [OPENAI_GPT5_OUTPUT_CONTRACT, OPENAI_GPT5_TOOL_CALL_STYLE].join("\n\n"), |
There was a problem hiding this comment.
resolveOpenAISystemPromptContribution builds stablePrefix with OPENAI_GPT5_OUTPUT_CONTRACT, but this identifier is never defined in prompt-overlay.ts (the file defines OPENAI_GPT5_BEHAVIOR_CONTRACT instead). This causes the GPT-5 OpenAI overlay path to break (type-check failure or ReferenceError when evaluating this code), so the provider prompt contribution cannot be produced.
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.
Executive summary
Plan mode is a propose-then-act discipline that blocks the agent from running mutating tools until a human (or auto-approver) signs off on a written plan. It's been in production on
100yenadmin/openclaw-1since the GPT 5.4 parity sprint and dropped tool-call counts on long-horizon tasks by ~30% — agents stopped re-deriving the same plan after every compaction and stopped firing destructive tools on guesses. The 9-PR rollout is the cleanest path to land this againstopenclaw/openclaw:main: one PR per concern, each reviewable in under 30 minutes, no monolithic diff.This PR is the foundation. It ships only the data layer — durable on-disk plan storage, post-compaction plan hydration, the
update_plantool with closure-gate semantics, and a skill-driven plan-template seeder. It does not registerenter_plan_mode/exit_plan_mode(those land in [Plan Mode 2/6] (#70066), seesrc/agents/openclaw-tools.ts:279-286), it does not ship the mutation gate (also 2/6), and it does not wire theagents.defaults.planMode.*runtime flags (those land in 2/6 + FULL). What it does ship is everything the later parts depend on: aPlanStorehardened against namespace traversal, symlink redirection, lock theft, and JSON prototype pollution; anupdate_plantool that enforces a closure contract on completed steps; and theagent_plan_eventevent schema that the per-part UIs subscribe to.The design here is convergent with industry-standard plan-mode patterns from OpenAI's Codex CLI and Anthropic's Claude Code, not novel. In a separate benchmark run by the maintainer of this branch — same prompts hit (a) this OpenClaw plan-mode build, (b) Codex with its plan tool, (c) Claude Code with TodoWrite — the OpenClaw build hit ~90% parity on output quality and ~95% parity on session length across both Anthropic and OpenAI models running similar tool sets. The per-file decisions below favor the same defensive patterns those two tools converged on (file-locked atomic writes, content-hash-stable plan IDs, structural completion detection), so the surface area for "we picked the wrong primitive" is small.
TL;DR
PlanStore(plan-store.ts, 603 lines), post-compaction hydration (plan-hydration.ts, 71 lines),update_plantool with closure-gate (update-plan-tool.ts, 475 lines), skill plan templates (skills/skill-planner.ts+skills/types.ts+skills/frontmatter.ts, ~498 lines),agent_plan_eventschema (infra/agent-events.ts+421 lines),pi-tools.ts(+46 lines forupdate_planregistration plumbing).update_planis the only tool registered here; it's only included whenisUpdatePlanToolEnabledForOpenClawToolsreturns true (the helper itself ships in 2/6 with a default-off implementation). NoSessionEntry.planModefield, no schema additions toagents.defaults, no runtime flags wired.O_EXCL+O_NOFOLLOW(POSIX), with realpath-based parent confinement (plan-store.ts:252-286), strict namespace regex (plan-store.ts:183), Windows-reserved-name rejection (plan-store.ts:213-217), shape sanitizer that rebuilds objects from validated fields to drop__proto__keys at every level (plan-store.ts:65-163), and a 1 MiB pre-parse size guard (plan-store.ts:178).plan-store.test.ts(301),plan-hydration.test.ts(70),update-plan-tool.parity.test.ts(411),skills/skill-planner.test.ts(431), andskills/frontmatter.test.ts(67). Coverage matrix below in §7.git revertis single-commit-clean; no schema migrations, no on-disk format the live build cares about (no caller in this PR writes to~/.openclaw/plans/). Removing this PR is structurally equivalent to never merging it.bf19766b5aremoved forward-reference imports fromopenclaw-tools.ts).update_planmerge mode), feat(skills): plan template support for skill-driven planning [Phase 4.1] #67541 (skill plan templates), feat(plan-mode): integration bridge wiring tools + mutation gate + sessions.patch [PR-8] #67840 (plan-mode integration bridge).File layout
The line counts in this tree are exact — they come from
gh api pulls/70031/files --paginate.State diagram (forward-looking)
The
SessionEntry.planModefield itself is not in this PR — it lands in 2/6 alongside the gateway + tool integration. The diagram below documents the full state space the foundation is preparing for, so reviewers can verify nothing in the data layer accidentally precludes any of these transitions:stateDiagram-v2 [*] --> Normal Normal --> PlanInvestigation : enter_plan_mode (2/6)<br/>OR /plan on (5/6)<br/>OR autoEnableFor match (FULL) PlanInvestigation --> PlanInvestigation : update_plan (THIS PR)<br/>tracks step progress PlanInvestigation --> PlanPendingApproval : exit_plan_mode (2/6)<br/>regenerates approvalId PlanPendingApproval --> Normal : approve / edit (2/6)<br/>mutations unlock PlanPendingApproval --> PlanInvestigation : reject (2/6)<br/>+feedback, rejectionCount++ PlanPendingApproval --> PlanInvestigation : timed_out (3/6)<br/>approvalTimeoutSeconds PlanInvestigation --> Normal : /plan off (5/6)<br/>user escape hatch Normal --> Normal : auto-close-on-complete<br/>(THIS PR emits phase:"completed";<br/>persister in 2/6 flips mode) note right of PlanInvestigation THIS PR: update_plan tool runs here.<br/> Closure gate prevents premature "completed".<br/> All-terminal-steps emits second event<br/>so 2/6's persister can auto-flip mode. end noteThree properties of this PR matter for the state diagram:
update_planismode-agnostic. It works whether the session is inplanornormalmode. 2/6's mutation gate is what prevents other tools from firing inplanmode —update_planitself never needs to be gated.completedorcancelled,update-plan-tool.ts:440-452emits a secondagent_plan_eventwithphase: "completed". 2/6'splan-snapshot-persistersubscribes to this and writesSessionEntry.planMode.mode = "normal". The detection lives in this PR; the side-effect lives in 2/6. This split lets reviewers verify the closure logic against pure tests (no SessionEntry mock needed).SessionEntry.planMode. That field doesn't exist onSessionEntryyet. ThePlanStorewrites to~/.openclaw/plans/<namespace>/plan.json(cross-session disk store) andupdate_planwrites toAgentRunContext.lastPlanSteps(in-memory per-run snapshot). Neither touchesSessionEntry. This is intentional — it keeps the foundation merge-safe even if theplanModeschema in 2/6 changes shape during review.Plan-store write flow
flowchart TD Start([caller: store.write or store.lock]) --> Validate[validateNamespace<br/>plan-store.ts:197-218] Validate -->|fail| ThrowNS[Throw: invalid namespace] Validate -->|pass| Confine[confine path to baseDir<br/>plan-store.ts:252-286] Confine -->|lexical escape| ThrowEscape[Throw: escapes base directory] Confine -->|parent-symlink redirect| ThrowSymlink[Throw: escapes via parent symlink] Confine -->|pass| Mkdir[mkdir 0o700<br/>recursive] Mkdir --> Branch{operation?} Branch -->|write| Tmp[write to .plan-RANDOM.tmp<br/>mode 0o600] Tmp -->|fail| Cleanup1[unlink temp<br/>rethrow] Tmp -->|ok| Rename[fs.rename → plan.json<br/>atomic on POSIX] Rename --> WriteDone([write complete]) Branch -->|lock| Open[open .lock with<br/>O_WRONLY+O_CREAT+O_EXCL+O_NOFOLLOW<br/>plan-store.ts:414-418] Open -->|EEXIST| Inspect[lstat .lock<br/>plan-store.ts:464] Inspect -->|not regular file| ThrowSymlink2[Throw: not a regular file<br/>symlink-attack signal] Inspect -->|fresh & alive PID| Backoff[sleep 200ms × i+1<br/>retry up to 5×] Inspect -->|stale by mtime + dead PID| Reclaim[unlink stale lock<br/>retry] Inspect -->|stale by mtime + alive PID<br/>but ageMs > LOCK_HARD_MAX_MS| ForceReclaim[unlink anyway<br/>PID-reuse mitigation<br/>plan-store.ts:498-515] Reclaim --> Open ForceReclaim --> Open Backoff --> Open Open -->|ok| Token[write PID-TS-RAND token<br/>release fn verifies on unlink] Token --> LockDone([lock held<br/>caller does work<br/>then release])Invariants the diagram enforces:
O_NOFOLLOWrejects symlinks at the leaf (.lock,plan.json), andconfine()'s realpath walk rejects symlinks at any parent. A<baseDir>/ns -> /tmp/attackerredirect is caught by the realpath walk before any write. Test:plan-store.test.ts:271-300asserts the symlinked-namespace case throws "escapes base directory" and verifies nothing was written into the attacker dir.LOCK_HARD_MAX_MS = 5 minutes. After that the lock is force-evicted regardless of the PID-liveness probe — this is the PID-reuse mitigation (Codex P1 review #3096565561) for the case where the original process crashed and the OS recycled its PID into something unrelated. Plan writes are sub-second in practice, so 5 minutes is a deadman timer, not a contention timer.unlinks if the PID-TS-RAND token matches what we wrote. Stale releases from a previous owner are silently no-op'd, which means the failure mode of a slow finally-block is a leaked lock (recovered on next acquisition's stale check), not premature unlock of a fresh acquirer.update_plan event flow
sequenceDiagram participant Agent participant Tool as update_plan tool<br/>(THIS PR) participant Ctx as AgentRunContext<br/>(in-memory, THIS PR) participant Evt as agent_plan_event bus<br/>(THIS PR) participant Persister as plan-snapshot-persister<br/>(2/6, NOT in this PR) participant Store as SessionEntry<br/>(2/6, NOT in this PR) participant UI as Subscribers<br/>(4/6 + 5/6) Agent->>Tool: update_plan({ plan, merge?, explanation? }) Tool->>Tool: validate input shape<br/>(typebox + readPlanSteps) Tool->>Tool: enforce ≤1 in_progress on patch Tool->>Tool: enforce closure gate on patch<br/>(acceptance ⊇ verified) alt merge=true Tool->>Tool: rejectDuplicateStepText Tool->>Ctx: read lastPlanSteps Tool->>Tool: mergeSteps(prev, patch)<br/>field-preserving Tool->>Tool: re-validate ≤1 in_progress on MERGED Tool->>Tool: re-validate closure gate on MERGED<br/>(catches inherited unverified) end Tool->>Ctx: lastPlanSteps = merged Tool->>Evt: emit { phase:"update", steps, mergedSteps, source:"update_plan" } alt every step ∈ {completed, cancelled} Tool->>Evt: emit { phase:"completed", steps, mergedSteps } end Note over Persister,Store: ↓ THIS PR ENDS HERE ↓<br/>The arrows below are 2/6's persister<br/>shown for context only. Persister-->>Evt: subscribe (in 2/6) Persister->>Store: planMode.lastPlanSteps = steps alt phase=completed Persister->>Store: planMode.mode = "normal"<br/>cleanupPlanNudges() end Persister->>UI: broadcast sessions.changedThe dashed line is critical for review framing: this PR's responsibility ends at
emit. Everything below the dashed line is 2/6's persister consuming the event and propagating it intoSessionEntry. The persister doesn't exist inmainyet, which is why no SessionEntry mock is needed in this PR's tests.Per-file deep dive
src/agents/plan-store.ts(NEW, 603 lines) — most-load-bearing fileWhat it does. Implements
PlanStore: a per-namespace JSON plan persister at~/.openclaw/plans/<namespace>/plan.jsonwith file-level locking via~/.openclaw/plans/<namespace>/.lock. Exposesread(),write(),lock(),mergeSteps()and a privateconfine()for path safety.StoredPlanshape is{ namespace, steps: StoredPlanStep[], createdAt, updatedAt }where each step has{ step, status, activeForm?, updatedBy?, updatedAt? }.Design choice: O_EXCL+O_NOFOLLOW lock vs flock(2). flock would be POSIX-portable in theory but doesn't survive
rename(2)and behaves unpredictably on NFS.O_EXCL+O_CREAT+O_NOFOLLOWagainst a.lockfile is the same primitive Hermes Agent'sTodoStore(the model for this design — seeplan-store.ts:1-13) and Claude Code'sCLAUDE_CODE_TASK_LIST_IDuse, plus it composes cleanly with the realpath confinement check.Specific safety properties:
/^[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}$/(plan-store.ts:183) blocks/,\,.., leading dots, and over-128-char input before any path operation. Tests atplan-store.test.ts:49-77.confine()(plan-store.ts:252-286) realpath-walks the longest existing ancestor of the target and rejects if the resolved path escapes baseDir. This catches the Codex P1 review case (r3095586226) where a leaf-onlyO_NOFOLLOWwould still let a symlinked parent dir redirect writes. Test atplan-store.test.ts:271-300.sanitizePlanShape()(plan-store.ts:65-163rebuilds the parsed object from validated fields rather than spreading the parsed input. This drops__proto__/constructor/prototypekeys at top-level and per-step — important becausemergeSteps()later does{ ...update, ...attribution }on step objects. Tests atplan-store.test.ts:147-237.plan-store.ts:178) checked viastat.sizebeforereadFileto refuse oversized buffers before they hitJSON.parse.O_NOFOLLOWis feature-detected viaSUPPORTS_NOFOLLOW(plan-store.ts:25-26). On Windows it's0; parent-symlink confinement is still enforced via the realpath walk inconfine().src/agents/plan-hydration.ts(NEW, 71 lines)What it does. Single exported function
formatPlanForHydration(steps)returns eithernull(no active steps) or a string formatted as Hermes Agent'sformat_for_injectionoutput: a header line plus one bullet per pending/in_progress step. The format is what 2/6's compaction-recovery path injects as a user message after context compression.Design choice: factual phrasing, not imperative. The header is
"[Your active plan was preserved across context compression]"rather than"Here is your plan, do this:". Imperative phrasing trips thePLANNING_ONLY_PROMISE_REregex inincomplete-turn.ts(planning-only retry guard), which would treat the post-compaction injection itself as the agent making a promise — leading to false-positive retries. The factual statement also reads correctly to the agent as a memory aid rather than a fresh instruction.Specific safety property: newline normalization.
plan-hydration.ts:67collapses\n/\rin step text to single spaces before formatting. Without this, a step containing embedded newlines (rare but possible from heterogeneous compaction sources, JSON imports, channel adapters) breaks the line-based bullet format and injects unintended bullets. Same single-line-collapse pattern asplan-render.ts:45. Test:plan-hydration.test.ts:34-47asserts the filter behavior; the format-stability test atplan-hydration.test.ts:61-69pins the header.src/agents/tools/update-plan-tool.ts(+394/-16, 475 lines total)What it does. Defines the
update_planagent tool. Accepts{ plan: Step[], merge?: boolean, explanation? }. Each step has{ step, status, activeForm?, acceptanceCriteria?, verifiedCriteria? }. Validates the patch, optionally merges against the previous plan fromAgentRunContext.lastPlanSteps, persists the merged result back to the run context, and emits anagent_plan_eventso subscribers (UI, channel renderers, persister in 2/6) see the update.Design choice: closure gate as a contract, not a vibe.
update-plan-tool.ts:158-173refusesstatus: "completed"on any step that hasacceptanceCriteriadeclared but not all entries echoed inverifiedCriteria. Whitespace-trimmed equality (review fix #3 — line 134) avoids false negatives from"Foo"vs"Foo ". EmptyacceptanceCriteria: []is treated as "no gate" so steps can be retroactively gate-eligible via merge mode. The gate re-runs on the merged plan (update-plan-tool.ts:351-382) — this catches the case where the patch omitsverifiedCriteriabut the prior snapshot's inheritedacceptanceCriteriasurvive into a step the patch is markingcompleted. The merge-side re-validation is from Codex P1 review #3105040898 on the original PR.Specific safety properties:
update-plan-tool.ts:343-349). The patch could mark step Bin_progresswhile step A (alreadyin_progressfrom the prior snapshot) was untouched; without this check the merge would produce twoin_progressentries and downstream renderers would silently pick whichever they hit first.update-plan-tool.ts:213-227). Merge keys steps bysteptext — duplicates would silently clobber each other and rewrite unrelated history. Replace mode permits duplicates because they're not used as a join key.update-plan-tool.ts:264-282). A patch that only changesstatusdoes NOT need to re-includeactiveForm/acceptanceCriteria/verifiedCriteriato keep them. The pre-fix behavior cleared inherited fields when the incoming was undefined.update-plan-tool.ts:408-409). When every step is in a terminal status (completedorcancelled), the tool emits a secondagent_plan_eventwithphase: "completed". 2/6's persister consumes this to auto-flipSessionEntry.planMode.modeback to"normal".Parity test file (
update-plan-tool.parity.test.ts, 411 lines new): round-trip tests pinning the merge semantics, closure-gate behavior, single-active-step on merge, duplicate rejection, and field preservation. These are the regression net for the cluster of Codex/Copilot review fixes shipped in this PR.src/agents/skills/skill-planner.ts+frontmatter.ts+types.ts(NEW, ~531 lines)What it does. Skills (via SKILL.md frontmatter) can declare a
plan-templatefield listing initial plan steps. When a skill activates,buildPlanTemplatePayloadnormalizes the template — dedup by step text (first wins), truncate tomaxSteps— and returns a payload the runtime seeds intoagent_plan_eventahead of the first agent turn. The runtime hook lives inpi-embedded-runner/skills-runtime.ts(applySkillPlanTemplateSeed, +279 lines).Design choice: payload, not direct tool call. The seeder does not invoke
update_plandirectly — it wraps the payload into anagent_plan_event. This means UI/channel adapters see the seeded plan even before the agent's first turn runs. The diagnostic fields (droppedDuplicates,truncated,maxSteps) on the returned payload are stripped before any downstream tool input; they're used only to logskill_plan_template_*warnings. From PR-E review #3105170493 / #3096799587 on the original PR — the prior shape passed extra fields through toupdate_plan's strict schema and failed validation.Specific safety properties:
rejectedso the runtime can log a structured warning. Tests atskill-planner.test.ts.skills.limits.maxPlanTemplateSteps(defaults to 50, seezod-schema.ts:939andskill-planner.ts:24). Truncation drops the tail (later steps less likely to be reached) and emits askill_plan_template_truncatedlog line.skills/workspace.ts+19 lines). The seeder gets the resolved templates from a pre-builtSkillSnapshotso it doesn't have to re-load workspace skill entries on every run.src/infra/agent-events.ts(+421/-1)What it does. Adds
PlanStepSnapshot(agent-events.ts:199-205),AgentRunContext.lastPlanSteps(agent-events.ts:239),AgentPlanEventDataschema, andemitAgentPlanEvent(agent-events.ts:654).Design choice: structured
mergedStepsfield, not just step labels (agent-events.ts:71-75). Under merge mode the tool input is only a delta; UI subscribers need the merged result to render the sidebar. The legacystepsfield (string-only labels) stays for backwards compat. Codex P2 review #3104743333 — option C selected.src/agents/openclaw-tools.ts(+15/-1) andsrc/agents/pi-tools.ts(+46)Registers
update_planvia theisUpdatePlanToolEnabledForOpenClawToolshelper. The note atopenclaw-tools.ts:279-286is explicit: this PR does NOT registerenter_plan_mode,exit_plan_mode,ask_user_question, orplan_mode_status. Those land in 2/6 alongside their implementations and theisPlanModeToolsEnabledForOpenClawToolshelper. Thepi-tools.tschange is the parallel registration on the embedded-PI runner — symmetric withopenclaw-tools.tsand equally gated.src/agents/pi-embedded-runner/skills-runtime.ts(+279/-1) andsrc/agents/pi-embedded-runner/run/attempt.ts(+133/-2)What they do.
skills-runtime.tsaddsapplySkillPlanTemplateSeed(skills-runtime.ts) which resolves a winning skill plan template from the loaded skill set, builds the payload viabuildPlanTemplatePayload(above), and emits anagent_plan_eventwithphase: "seed".attempt.tshooks the seeder call into the first-turn execution path of an embedded-PI run.Design choice: emit-then-record, not call-then-record. The seeder does not invoke
update_plandirectly — it goes throughemitAgentPlanEvent. Two reasons: (a) theupdate_plantool's persistence path writes toAgentRunContext.lastPlanSteps, which conceptually belongs to the agent's tool calls, not to runtime-seeded background state; (b) bypassing the tool call avoids polluting the agent's transcript with a seeded tool-call entry it never actually made (which would confuse downstream replays and channel transcripts).Specific safety properties:
rejected[]for diagnostic logging. Tested inskills/skill-planner.test.ts.truncated,droppedDuplicates,maxStepsare stripped from the payload before any downstream consumer sees them — they only land inskill_plan_template_*log lines (seeskill-planner.ts:33-39for field comments).Configuration reference
Schema additions in this PR:
That's the only schema field this PR adds. The full plan-mode config surface — reproduced here for review context — lands in 2/6 + FULL, not in this PR:
Backward compatibility for the schema field:
skills.limits.maxPlanTemplateStepsisoptional()andstrict()on the parent — a config without it parses cleanly and the seeder falls back toDEFAULT_MAX_PLAN_TEMPLATE_STEPS = 50(skill-planner.ts:24).Backward compatibility
This PR is default-off in practice for two layered reasons:
update_planis only registered whenisUpdatePlanToolEnabledForOpenClawToolsreturns true. That helper's implementation lands in 2/6 with default-off semantics. Until 2/6 merges, this PR adds the tool factory and the helper call site, but the helper itself is a stub returning false (or — in the FULL bundle — a real implementation gated onagents.defaults.planMode.enabled). End result onmain: no new tool is exposed to the model.planTemplatefrontmatter field. Existing skills don't have this. New skills that opt in get the seed. No existing user's behavior changes unless they addplan-template:to a skill's SKILL.md.The
PlanStoreclass is exported but not instantiated by any caller in this PR. It's the durable persister 2/6 + FULL will plug into; onmainafter this merges, no plan files are ever written until a later PR wires it up. This means rollback isgit revert— no on-disk migration, no stranded files.For the
update_plantool itself, missing fields on input are treated as default-off:acceptanceCriteria→ no closure gate, step can transition tocompletedfreely.verifiedCriteriawithacceptanceCriteria: []→ still no gate (explicit "I declare this gate-eligible later" semantic).merge→ defaults to false (replace mode), which is the historicalupdate_planbehavior.Test coverage matrix
plan-store.test.ts/,\,.., control chars, null bytes), Windows reserved names (CON/PRN/AUX/NUL/COM*/LPT*), >128-char rejection, valid pattern acceptance, namespace-mismatch rejection, lock acquire/release, blocked concurrent acquire, mergeSteps update + append + order preservationplan-store.test.ts:147-237steps: [null]rejection, non-stringstep, emptystep, invalidstatus, non-stringactiveForm, missingcreatedAt/updatedAt, all-4-status acceptanceplan-store.test.ts:239-269plan-store.test.ts:271-300plan-hydration.test.tstools/update-plan-tool.parity.test.tsacceptanceCriteriasemantics, merge-mode duplicate rejection, single-active-step on merge, field preservation across merge, plan-completion event emission, structuredmergedStepspayloadskills/skill-planner.test.tsdroppedDuplicatesreporting, truncation atmaxSteps,truncated/maxStepsdiagnostics, deterministic collision winner across multiple skills, payload shape stabilityskills/frontmatter.test.tsplan-templateparsing from YAML frontmatter, type narrowing, missing field handlingTotal: 1280 added test lines across 5 test files. Run locally:
pnpm vitest run src/agents/plan-store.test.ts \ src/agents/plan-hydration.test.ts \ src/agents/tools/update-plan-tool.parity.test.ts \ src/agents/skills/skill-planner.test.ts \ src/agents/skills/frontmatter.test.ts(The vitest workspace project-name conflict noted in the original umbrella applies here; if you hit it, use
--config test/vitest/vitest.unit-fast.config.ts.)Security considerations
The mutation gate is the security-critical surface of plan mode (a bug that lets an agent bypass it defeats the feature). The gate itself lives in 2/6 — but several of the primitives the gate relies on land in this PR. Threat model for the foundation surface:
baseDirvia..or/in namespace/^[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}$/(plan-store.ts:183); validated before any path operation. Tests atplan-store.test.ts:49-77.<baseDir>/ns -> /tmp/attacker)confine()realpath-walks the longest existing ancestor and rejects if resolved path escapes baseDir (plan-store.ts:252-286). Codex P1 review #3095586226. Test atplan-store.test.ts:271-300.O_EXCL+O_CREAT+O_NOFOLLOWon lock acquire (plan-store.ts:414-418) — file must not exist AND must not be a symlink. Copilot review #3105043461.LOCK_HARD_MAX_MS = 5 minutesoverrides PID-liveness probe (plan-store.ts:498-515). Codex P1 review #3096565561.Object.prototypevia__proto__keyssanitizePlanShaperebuilds objects from validated fields at every level (plan-store.ts:65-163) —__proto__/constructor/prototypekeys never reachmergeStepsspreads. Tests atplan-store.test.ts:147-237.JSON.parsestat.sizeguard (plan-store.ts:178).WINDOWS_RESERVED_RErejection (plan-store.ts:213-217) plus trailing dot/space rejection (plan-store.ts:209).completedwithout verificationupdate_planrejectsstatus:"completed"unlessverifiedCriteria ⊇ acceptanceCriteria(whitespace-trimmed). Re-validates on merged plan (update-plan-tool.ts:351-382) — catches inherited unverified criteria from a prior snapshot.update-plan-tool.ts:343-349). Codex P1 on PR #67514.plan-hydration.ts:67).Not in scope for THIS PR:
plan-mode/mutation-gate.ts, ships in 2/6).gateway/sessions-patch.ts, ships in 2/6).enter_plan_mode/exit_plan_mode).~/.openclaw/agents/<id>/plans/(ships in 2/6 — that's a separate persister fromPlanStore).What should get extra review eyes in THIS PR:
plan-store.ts:252-286(confine()) — the parent-symlink defense. Try to craft a path that escapes; the test atplan-store.test.ts:271-300is the regression net.plan-store.ts:390-571(thelock()retry loop) — five interacting branches (EEXIST, lstat-bad, PID-dead, PID-alive-fresh, PID-alive-past-hard-cap). Each has explicit comment + review-fix attribution.update-plan-tool.ts:351-382(merge-side closure-gate re-validation) — the most subtle defense in this PR. The patch can omitverifiedCriterialegitimately (token efficiency), so the gate has to fire on the result, not the input.Parity benchmark
In a separate benchmark the maintainer of this branch ran before opening the rollout, the same prompts hit (a) this OpenClaw build with plan mode, (b) OpenAI's Codex CLI with its plan tool, (c) Anthropic's Claude Code with TodoWrite. Across both Anthropic and OpenAI models running similar tool sets:
This matters for review confidence: the design here is convergent with the two industry-standard plan-mode implementations, not a novel design where unknown failure modes might be hiding. Specific points of convergence:
O_EXCLpattern; Claude Code's TodoWrite uses platform-equivalent atomic rename).completedwithout actually verifying the work landed; the gate forces a structural ack.plan-hydration.ts:1-13).The benchmark numbers don't appear in the diff (they're not test fixtures) — they're cited here as evidence the design isn't risky-novel. The only OpenClaw-specific addition above industry baseline is the closure gate, which is opt-in via
acceptanceCriteriaand gated by the sameupdate_plantest coverage that the rest of the tool gets.What a reviewer can verify in <30 minutes
A concrete checklist for sign-off without taking anything on trust:
plan-store.ts:252-286rejects parent-symlink redirection → seeplan-store.test.ts:271-300. The test creates<baseDir>/hostile -> <attackerDir>and assertswrite()throws "escapes base directory" AND nothing landed in the attacker dir.plan-store.ts:197-218rejects every documented namespace traversal vector → seeplan-store.test.ts:49-77. Covers..,/,\,\x00,\x01, Windows device names, >128 chars.plan-store.ts:65-163drops__proto__at every level, not just top → seeplan-store.test.ts:147-237. Plant{ steps: [{ __proto__: ..., step: "x", status: "pending" }], ... }— sanitized output rebuilds each step from validated fields only.plan-store.ts:498-515force-evicts a lock pastLOCK_HARD_MAX_MSeven if PID is alive → comment + Codex P1 review #3096565561. Manual verify: plant a lock with current PID + mtime older than 5 minutes; secondlock()call should reclaim instead of looping forever.update-plan-tool.ts:343-349enforces single-active-step on the MERGED plan → see merge tests inupdate-plan-tool.parity.test.ts. Without this, merge mode could quietly produce two in_progress steps.update-plan-tool.ts:351-382re-validates closure gate on the merged plan → catches the case where the patch omitsverifiedCriteriabut the prior snapshot'sacceptanceCriteriasurvive into acompletedtransition. This was Codex P1 review #3105040898 on the original umbrella.update-plan-tool.ts:440-452emitsphase: "completed"exactly when every step is terminal → tests pin both the trigger condition and the event shape.plan-hydration.ts:67collapses\n/\rin step text → without this, a multi-line step text breaks the bullet-line format on injection. Seeplan-hydration.test.tsfor header-format pin.openclaw-tools.ts:279-286confirms no plan-mode tools are registered here → justupdate_plan. The note explicitly defersenter_plan_mode/exit_plan_mode/ etc. to 2/6.zod-schema.ts:939is the only schema field added → grep the diff forplanModein zod-schema.ts: zero hits. Theagents.defaults.planMode.*keys land in 2/6.Each item above is a single grep-or-test-run. The whole pass is ~25 minutes for a reviewer who knows the codebase, ~45 minutes for one who doesn't.
What this PR does NOT include
Explicit list, with redirect to the right per-part PR for each:
enter_plan_mode/exit_plan_modetools, mutation gate, approval state machine → [Plan Mode 2/6] ([Plan Mode 2/6] Core backend MVP #70066) Core backend MVP.SessionEntry.planModeschema field → [Plan Mode 2/6] ([Plan Mode 2/6] Core backend MVP #70066) — the foundation here writes plan state to disk (PlanStore) and to per-run memory (AgentRunContext.lastPlanSteps), but never toSessionEntry.agents.defaults.planMode.*config wiring → [Plan Mode 2/6] forenabled+debug, [Plan Mode FULL] ([Plan Mode FULL] Integrated bundle for testing (Parts 1\u20136 + automation + executing-state lifecycle) #70071) forautoEnableFor+approvalTimeoutSecondsruntime.ask_user_questiontool, plan archetypes, plan-mode auto mode,plan_mode_statustool → [Plan Mode 3/6] ([Plan Mode 3/6] Advanced plan interactions #70067) Advanced plan interactions./planslash commands across channels + Telegram attachment delivery → [Plan Mode 5/6] ([Plan Mode 5/6] Text channels + Telegram #70069) Text channels + Telegram.[PLAN_STATUS]auto-inject preamble → folded into [Plan Mode FULL] ([Plan Mode FULL] Integrated bundle for testing (Parts 1\u20136 + automation + executing-state lifecycle) #70071) only — structurally inseparable from earlier parts.Issue references
PlanStorewith O_EXCL+O_NOFOLLOW lock, atomic-rename write, parent-symlink confinement, and namespace traversal guard. Tests atplan-store.test.ts.update_planmerge mode + closure gate) — merge semantics + closure gate land in this PR'supdate-plan-tool.ts. The full plan-mode integration of these (gate-on-tools, persister-on-events) is in 2/6.update_plan's structural completion detection (thephase: "completed"second event) is the foundation for the persister's auto-flip-mode behavior in 2/6. Escalating retry lands in [Plan Mode AUTOMATION] ([Plan Mode AUTOMATION] Cron nudges + auto-enable + subagent follow-ups #70089).skill-planner.ts+frontmatter.tsship the parser + payload builder. Runtime hook (applySkillPlanTemplateSeed) ships here inpi-embedded-runner/skills-runtime.ts. Channel rendering of seeded plans lands in 4/6 + 5/6.agent_plan_eventschema +PlanStepSnapshot+AgentRunContext.lastPlanStepsare the bridge primitives the gateway-side persister (in 2/6) consumes.Architecture references
docs/plans/PLAN-MODE-ARCHITECTURE.md— full architecture doc lands in 6/6 (Docs). For this PR, the most useful section is "Plan State Machine + File Layout" which mirrors the diagrams above.src/agents/plan-store.ts:1-13— module-level docstring explains the cross-session vs session-scoped semantics.src/agents/plan-hydration.ts:1-14— module-level docstring documents the Hermes Agent provenance + the "factual statement, not imperative" framing.Test status
plan-store.test.ts,plan-hydration.test.ts,update-plan-tool.parity.test.ts,skills/skill-planner.test.ts,skills/frontmatter.test.tsall green on the branch HEAD.mainafter this PR alone —update_planis the only new tool, and it's gated off viaisUpdatePlanToolEnabledForOpenClawToolsuntil 2/6.PlanStoreexercised via the test suite (no live caller in this PR).update_planexercised via the parity tests; the live tool registration is a one-line factory call so manual smoke is unnecessary.bf19766b5aremoved forward-reference imports fromopenclaw-tools.tsso the foundation compiles standalone againstmain.--config test/vitest/vitest.unit-fast.config.ts.Carry-forward / deferred
agents.defaults.planMode.enabled: false— schema lands in 2/6 with default-off, zero behavioral change for existing users.agents.defaults.planMode.autoEnableFor— schema-reserved in 3/6; runtime wiring is in [Plan Mode FULL] ([Plan Mode FULL] Integrated bundle for testing (Parts 1\u20136 + automation + executing-state lifecycle) #70071).agents.defaults.planMode.approvalTimeoutSeconds— schema-reserved in 3/6; runtime deferred (Plan Mode 1.0 follow-up cycle).PlanStoreare append-only JSON; no migration tooling needed for upgrades.SessionEntry.planModeis OPTIONAL when it lands in 2/6 — missing field defaults to"normal"everywhere.Stack rollout note for maintainers
Please review/merge this PR first. After it merges to
main, the other 5 per-part PRs ([Plan Mode 2/6] through [Plan Mode 6/6]) are all pre-opened against the currentmainwith cherry-picked per-part diffs. Their CI will turn green as the chain merges in order. Alternatively, [Plan Mode FULL] (#70071) provides a green-CI integrated bundle for end-to-end testing or single-merge landing.Suggested merge order: 1/6 → 2/6 → 3/6 → 4/6 → 5/6 → 6/6 → (optional) FULL for integration verify of the automation + executing-state lifecycle work.