[Plan Mode 7/8] Docs, QA, and help#70019
Conversation
…ion tests (rebase fixup) Pure rebase fixup. The `exit_plan_mode` tool gained a mandatory `title` parameter in upstream main between Part 5's original base and current HEAD. After rebasing Part 5 onto upstream/main, four integration tests (in `src/agents/plan-mode/integration.test.ts`) were failing with `ToolInputError: exit_plan_mode requires a title field` instead of the originally-asserted error messages. This commit just adds `title: "..."` to the four failing test calls so they exercise the validation paths they were originally testing (empty plan, multiple in_progress, unknown status). No production code changes.
|
Too many files changed for review. ( |
openclaw-barnacle
Bot
added
docs
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds “Plan Mode” operator/docs and expands runtime support for plan-mode lifecycle (tools, gates, persistence, QA, and Telegram delivery) as part of the stacked decomposition for #68939.
Changes:
- Register universal
/plancommand and introduce plan-mode tools (enter_plan_mode,exit_plan_mode,plan_mode_status,ask_user_question) plus related tool catalog/display metadata. - Add plan-mode infrastructure (mutation gate, approval state machine, pending injection bridge, nudges, auto-enable matching, plan hydration, plan archetype prompt + persistence + Telegram attachment bridge).
- Expand/repair tests and add QA scenarios + docs/runbooks covering plan mode operations and troubleshooting.
Reviewed changes
Copilot reviewed 96 out of 185 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/auto-reply/reply/commands-handlers.runtime.ts | Registers /plan handler in command runtime. |
| src/auto-reply/reply/agent-runner.misc.runreplyagent.test.ts | Updates assertion to reflect skip-cache freshness behavior. |
| src/auto-reply/commands-registry.shared.ts | Adds builtin plan slash command definition. |
| src/agents/transport-message-transform.ts | Improves missing tool_result repair text + bounds log volume. |
| src/agents/tools/update-plan-tool.test.ts | Updates expectations and adds coverage for close-on-complete + closure gate. |
| src/agents/tools/sessions-spawn-tool.ts | Adds plan-mode concurrency cap, cleanup keep override, and parent/child run tracking. |
| src/agents/tools/sessions-spawn-tool.test.ts | Updates tests for role removal + adds plan-mode concurrency cap tests. |
| src/agents/tools/plan-mode-status-tool.ts | Introduces read-only plan-mode status introspection tool. |
| src/agents/tools/enter-plan-mode-tool.ts | Introduces enter_plan_mode tool surface. |
| src/agents/tools/cron-tool.ts | Documents “resume after wait” recipe for plan-mode workflows. |
| src/agents/tools/ask-user-question-tool.ts | Adds ask_user_question tool and validation logic. |
| src/agents/tools/ask-user-question-tool.test.ts | Adds validation + determinism + content tests for ask_user_question. |
| src/agents/tool-display-config.ts | Adds display config for new plan-mode tools and fixes question detail keys. |
| src/agents/tool-description-presets.ts | Adds display summaries + describes plan-mode tools and archetype guidance. |
| src/agents/tool-catalog.ts | Registers plan-mode tools in the core tool catalog. |
| src/agents/test-helpers/fast-openclaw-tools-sessions.ts | Updates update_plan mock signature + exports statuses for tests. |
| src/agents/subagent-registry.test.ts | Adapts mocks/outcome shape for new parent-drain behavior. |
| src/agents/subagent-registry.steer-restart.test.ts | Uses partial actual mock and adds parent openSubagentRunIds remap test. |
| src/agents/subagent-registry-run-manager.ts | Drains completed/killed subagents from parents and remaps on steer restart. |
| src/agents/subagent-announce.ts | Adds plan-mode-aware suffix to announce instruction and reads requester mode. |
| src/agents/skills/workspace.ts | Carries skill plan templates into snapshots for seeding path. |
| src/agents/skills/types.ts | Adds SkillPlanTemplateStep and snapshot resolvedPlanTemplates. |
| src/agents/skills/skill-planner.ts | Adds plan template payload builder + normalization. |
| src/agents/skills/frontmatter.ts | Parses skill plan templates from frontmatter (kebab/camel + content alias). |
| src/agents/skills/frontmatter.test.ts | Adds test coverage for plan-template parsing semantics. |
| src/agents/plan-mode/types.ts | Adds plan-mode core types + secure approvalId generation + decision injection builder. |
| src/agents/plan-mode/reference-card.ts | Adds bootstrap-injected plan-mode reference card text. |
| src/agents/plan-mode/plan-nudge-crons.ts | Adds scheduling/cleanup for plan-mode nudge cron jobs. |
| src/agents/plan-mode/plan-mode-debug-log.ts | Adds structured plan-mode debug log + shared enablement helper w/ TTL cache. |
| src/agents/plan-mode/plan-archetype-prompt.ts | Adds archetype prompt + plan filename helpers. |
| src/agents/plan-mode/plan-archetype-prompt.test.ts | Tests archetype prompt content + filename helper behavior. |
| src/agents/plan-mode/plan-archetype-persist.test.ts | Tests persistence, collisions, and storage error classification. |
| src/agents/plan-mode/plan-archetype-bridge.ts | Persists plan markdown and (best-effort) sends Telegram document attachment. |
| src/agents/plan-mode/mutation-gate.ts | Introduces plan-mode mutation gate with exec read-only whitelist. |
| src/agents/plan-mode/mutation-gate.test.ts | Adds unit tests for mutation gate behavior. |
| src/agents/plan-mode/integration.test.ts | Adds wiring-level integration test for plan mode enablement + gating. |
| src/agents/plan-mode/index.ts | Exports plan-mode public surface. |
| src/agents/plan-mode/auto-enable.ts | Adds regex-based auto-enable matching helper with compiled cache. |
| src/agents/plan-mode/auto-enable.test.ts | Tests auto-enable matching and malformed pattern handling. |
| src/agents/plan-mode/approval.ts | Adds approval state machine resolver + approved-plan injection builder. |
| src/agents/plan-hydration.ts | Adds post-compaction plan hydration formatting. |
| src/agents/plan-hydration.test.ts | Tests hydration formatting and filtering behavior. |
| 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. |
| src/agents/pi-embedded-runner/skills-runtime.test.ts | Updates snapshot reload logic tests for resolvedPlanTemplates presence/absence. |
| src/agents/pi-embedded-runner/run/params.ts | Threads planMode + live accessors through embedded run params. |
| src/agents/pi-embedded-runner/run/helpers.ts | Raises iteration defaults and adds user override + subagent cap. |
| src/agents/pi-embedded-runner/run/attempt.spawn-workspace.test-support.ts | Stubs skill plan-template seeder for workspace spawn tests. |
| src/agents/pi-embedded-runner/run.overflow-compaction.test.ts | Uses config override to keep retry-limit test fast under higher defaults. |
| src/agents/pi-embedded-runner/run.incomplete-turn.test.ts | Updates retry counts and adds escalation + plan-mode disablement assertions. |
| src/agents/pi-embedded-runner/pending-injection.ts | Adds compat shim over typed pending-injection queue. |
| src/agents/pi-embedded-runner/pending-injection.test.ts | Tests injection consume/compose behavior against temp session store. |
| src/agents/openclaw-tools.ts | Wires plan-mode tools into OpenClaw tools set and threads runId into tools. |
| src/agents/openclaw-tools.registration.ts | Adds config gate for plan-mode tool registration. |
| skills/plan-mode-101/SKILL.md | Adds Plan Mode 101 reference skill + self-test guidance. |
| qa/scenarios/gpt54-plan-mode-default-off.md | QA scenario ensuring plan mode stays off by default for GPT‑5.4. |
| qa/scenarios/gpt54-mandatory-tool-use.md | QA scenario ensuring tool use for factual queries. |
| qa/scenarios/gpt54-injection-scan.md | QA scenario baseline for injection-scanner non-interference. |
| qa/scenarios/gpt54-cancelled-status.md | QA scenario for cancelled plan steps behavior. |
| qa/scenarios/gpt54-act-dont-ask.md | QA scenario for “act on obvious defaults” behavior. |
| extensions/telegram/src/send.ts | Adds sendDocumentTelegram helper for file uploads with safety checks. |
| extensions/telegram/runtime-api.ts | Exposes sendDocumentTelegram + opts type from runtime API. |
| docs/tools/slash-commands.md | Documents /plan universal command surface. |
| docs/plans/PLAN-MODE-OPERATOR-RUNBOOK.md | Adds operator runbook for plan-mode incident diagnosis. |
| docs/concepts/plan-mode.md | Adds user-facing plan mode concept doc with multi-channel behavior. |
| apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift | Adds plan-mode fields/error codes to protocol models. |
| apps/shared/OpenClawKit/Sources/OpenClawKit/Resources/tool-display.json | Adds tool-display metadata for plan-mode tools. |
| apps/macos/Sources/OpenClawProtocol/GatewayModels.swift | Mirrors protocol model updates in macOS target. |
| const MUTATION_TOOL_BLOCKLIST = new Set([ | ||
| "apply_patch", | ||
| "bash", | ||
| "edit", | ||
| "exec", | ||
| "gateway", | ||
| "message", | ||
| "nodes", | ||
| "process", | ||
| "sessions_send", | ||
| "sessions_spawn", | ||
| "subagents", | ||
| "write", | ||
| ]); |
There was a problem hiding this comment.
sessions_spawn and other plan-mode-affordance tools (ask_user_question, enter_plan_mode, and plan_mode_status) appear to be intended/required during plan mode (per the reference card/docs and the new sessions_spawn plan-mode concurrency logic), but the mutation gate currently blocks them by default-deny (and explicitly blocklists sessions_spawn). This makes plan-mode investigation flows unable to spawn research subagents or ask clarifying questions while plan mode is active. Update the gate to allow these plan-mode tools (and remove sessions_spawn from the blocklist / add it to the allowlist) so the runtime behavior matches the documented plan-mode lifecycle.
| const PLAN_MODE_ALLOWED_TOOLS = new Set([ | ||
| "read", | ||
| "web_search", | ||
| "web_fetch", | ||
| "memory_search", | ||
| "memory_get", | ||
| "update_plan", | ||
| "exit_plan_mode", | ||
| "session_status", | ||
| ]); |
There was a problem hiding this comment.
sessions_spawn and other plan-mode-affordance tools (ask_user_question, enter_plan_mode, and plan_mode_status) appear to be intended/required during plan mode (per the reference card/docs and the new sessions_spawn plan-mode concurrency logic), but the mutation gate currently blocks them by default-deny (and explicitly blocklists sessions_spawn). This makes plan-mode investigation flows unable to spawn research subagents or ask clarifying questions while plan mode is active. Update the gate to allow these plan-mode tools (and remove sessions_spawn from the blocklist / add it to the allowlist) so the runtime behavior matches the documented plan-mode lifecycle.
| // Terminal-state guard. Approved, edited, and timed_out are terminal — | ||
| // they require a fresh exit_plan_mode call (which mints a new approvalId) | ||
| // before any new action can apply. Rejected stays open for re-approval | ||
| // or re-rejection. | ||
| 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.
Allowing approval actions to resolve when current.approval === "none" can incorrectly transition a session to "approved"/"edited" (and mode: "normal") without an active pending approval, if resolvePlanApproval() is called with action: "approve" | "edit" | "reject" and no expectedApprovalId. Tighten the guard so approve/edit/reject only apply to "pending" (and, if desired, "rejected" for re-approval semantics), but not "none".
| // Terminal-state guard. Approved, edited, and timed_out are terminal — | |
| // they require a fresh exit_plan_mode call (which mints a new approvalId) | |
| // before any new action can apply. Rejected stays open for re-approval | |
| // or re-rejection. | |
| if ( | |
| current.approval !== "pending" && | |
| current.approval !== "rejected" && | |
| current.approval !== "none" | |
| ) { | |
| return current; | |
| } | |
| if (action === "timeout" && current.approval !== "pending") { | |
| return current; | |
| } | |
| // State-transition guard. | |
| // - approve/edit/reject only apply while an approval is active ("pending") | |
| // or after a rejection ("rejected"), preserving re-approval semantics. | |
| // - timeout only applies to an actively pending approval. | |
| // States like "none", "approved", "edited", and "timed_out" require a | |
| // fresh exit_plan_mode call (which mints a new approvalId) before any new | |
| // approval action can apply. | |
| if (action === "timeout") { | |
| if (current.approval !== "pending") { | |
| return current; | |
| } | |
| } else if (current.approval !== "pending" && current.approval !== "rejected") { | |
| return current; | |
| } |
| for (const minutes of intervals) { | ||
| if (minutes <= 0 || !Number.isFinite(minutes)) { | ||
| continue; | ||
| } | ||
| const fireAtMs = now + Math.floor(minutes * 60_000); | ||
| const fireAtIso = new Date(fireAtMs).toISOString(); | ||
| try { |
There was a problem hiding this comment.
assertSafeCronSessionTargetId is imported inside the per-interval loop, which adds repeated module-load overhead (even if cached, it still adds async/await churn) and complicates error attribution. Since the validation is per-sessionKey (not per-interval), move the import and validation outside the loop (once per schedule call), then proceed to schedule intervals only if validation passes.
| // against the same constraints the cron service applies in | ||
| // `assertSafeCronSessionTargetId` (no `/`, `\`, or `\0` | ||
| // characters). Pre-fix, an exotic sessionKey could produce a | ||
| // sessionTarget the cron jobs.ts validator rejects, causing | ||
| // the plan-nudge schedule call to fail at gateway-side | ||
| // assertion time. Belt-and-suspenders here keeps the failure | ||
| // local + actionable instead of a generic cron-validator | ||
| // error 60 seconds later. | ||
| 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 imported inside the per-interval loop, which adds repeated module-load overhead (even if cached, it still adds async/await churn) and complicates error attribution. Since the validation is per-sessionKey (not per-interval), move the import and validation outside the loop (once per schedule call), then proceed to schedule intervals only if validation passes.
| // against the same constraints the cron service applies in | |
| // `assertSafeCronSessionTargetId` (no `/`, `\`, or `\0` | |
| // characters). Pre-fix, an exotic sessionKey could produce a | |
| // sessionTarget the cron jobs.ts validator rejects, causing | |
| // the plan-nudge schedule call to fail at gateway-side | |
| // assertion time. Belt-and-suspenders here keeps the failure | |
| // local + actionable instead of a generic cron-validator | |
| // error 60 seconds later. | |
| 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; | |
| } | |
| // against the same constraints the cron service applies in | |
| // `assertSafeCronSessionTargetId` (no `/`, `\`, or `\0` | |
| // characters). Pre-fix, an exotic sessionKey could produce a | |
| // sessionTarget the cron jobs.ts validator rejects, causing | |
| // the plan-nudge schedule call to fail at gateway-side | |
| // assertion time. Belt-and-suspenders here keeps the failure | |
| // local + actionable instead of a generic cron-validator | |
| // error 60 seconds later. | |
| 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) | |
| }`, | |
| ); | |
| return []; | |
| } |
| const safeTitle = (title ?? "").trim() || "Plan"; | ||
| const escTitle = escapeHtml(safeTitle); | ||
| const safeSummary = (summary ?? "").trim(); | ||
| const summaryLine = safeSummary ? `\n${escapeHtml(safeSummary)}` : ""; | ||
| return [ | ||
| `<b>${escTitle}</b> — plan submitted for approval. See attached.`, | ||
| summaryLine, | ||
| "", | ||
| "Resolve with: <code>/plan accept</code> | <code>/plan accept edits</code> | <code>/plan revise <feedback></code>", | ||
| ] | ||
| .filter(Boolean) | ||
| .join("\n"); |
There was a problem hiding this comment.
summaryLine includes a leading newline (\n...) but is also joined with "\n", which effectively introduces an extra blank line when a summary is present and makes formatting harder to reason about. Prefer pushing the escaped summary as its own line (without embedding a leading newline) and let the join control all line breaks.
| const spawnParentCtx = opts?.runId ? getAgentRunContext(opts.runId) : undefined; | ||
| if ( | ||
| spawnParentCtx?.inPlanMode === true && | ||
| (spawnParentCtx.openSubagentRunIds?.size ?? 0) >= MAX_CONCURRENT_SUBAGENTS_IN_PLAN_MODE | ||
| ) { | ||
| throw new ToolInputError( | ||
| `Plan mode allows only ${MAX_CONCURRENT_SUBAGENTS_IN_PLAN_MODE} concurrent research ${ | ||
| MAX_CONCURRENT_SUBAGENTS_IN_PLAN_MODE === 1 ? "subagent" : "subagents" | ||
| }. Wait for the current child to return before spawning another.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The ACP path performs two separate getAgentRunContext(opts.runId) lookups (spawnParentCtx and later parentCtx) for closely related plan-mode decisions (concurrency cap and cleanup override). Reuse the already fetched context (or fetch once and share) to avoid duplicated logic paths and reduce the chance of divergence when the criteria evolve.
| const parentCtx = opts?.runId ? getAgentRunContext(opts.runId) : undefined; | ||
| const parentInPlanMode = parentCtx?.inPlanMode === true; |
There was a problem hiding this comment.
The ACP path performs two separate getAgentRunContext(opts.runId) lookups (spawnParentCtx and later parentCtx) for closely related plan-mode decisions (concurrency cap and cleanup override). Reuse the already fetched context (or fetch once and share) to avoid duplicated logic paths and reduce the chance of divergence when the criteria evolve.
![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: 90b696c622
ℹ️ 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".
| if (prefix.startsWith("~/") && tildeForm.startsWith(prefix)) { | ||
| return matchedProtectedPath(filePath, prefix); | ||
| } |
There was a problem hiding this comment.
The protected-path check only uses startsWith against prefixes that all end in /, so exact root targets like ~/.openclaw or /etc/openclaw do not match and are allowed under acceptEdits. In a post-approval session this lets a delete/apply_patch-family call modify or remove the protected config root itself, bypassing the intended hard config-change block for these directories.
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.
This unconditional validation rejects any step that provides verifiedCriteria without also repeating acceptanceCriteria, which breaks merge-mode incremental verification for existing steps that already have acceptance criteria stored from a previous update. In practice, a valid merge: true patch that only advances verification progress will fail with a 400 unless the caller resends the full criteria list each time.
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 7/8] — part of the 8-part decomposition of #68939, stacking on #69449 (Part 1).
Stacks on: previous part (see [Plan Mode 6/8] / [Plan Mode 1/9 #69449]).
Plan-mode operator runbook + skill-based docs + QA scenarios + /plan help text. Stacks on Part 6/8.
Lines: ~2371
Diff note
This PR is opened cross-repo (head:
100yenadmin:restack/68939-pr9-docs-qa-help) 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