[Plan Mode 3/6] Advanced plan interactions#70067
Conversation
openclaw-barnacle
Bot
added
app: web-ui
Greptile SummaryThis PR adds the advanced plan-mode interaction layer: the Confidence Score: 4/5Safe to merge with minor issues worth addressing before or shortly after landing. All three findings are P2. The legacy option-validation gap is a real behavioral hole but only affects sessions persisted before the pendingQuestionOptions field was introduced, and the new pendingInteraction path handles it correctly. The quote-concatenation escape pattern is intentionally conservative but lacks a false-positive test. The missing i flag on DESTRUCTIVE_FIND_FLAGS is a minor inconsistency. None block the primary plan-mode paths. src/gateway/sessions-patch.ts (legacy answer-validation gap) and src/agents/plan-mode/accept-edits-gate.ts (escape pattern breadth + missing /i flag). Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/sessions-patch.ts
Line: 705-716
Comment:
**Option validation skipped for legacy sessions with no stored options**
When `resolvePendingQuestionState` resolves via the `"legacy"` path, `pendingQuestion.options` is `entry.pendingQuestionOptions ?? []`. For sessions persisted before `pendingQuestionOptions` was introduced, this is an empty array, so `offeredOptions.length > 0` is `false` and the inner rejection never fires — any freetext answer passes through even when `allowFreetext` is `false`. A text-channel user on such a legacy session could submit arbitrary answer text that bypasses the contract the agent specified.
The guard should either (a) treat an empty stored options list as "no options known, skip validation" (i.e., current behaviour, documented explicitly), or (b) reject the answer entirely when `allowFreetext === false && offeredOptions.length === 0`, since accepting unvalidated text when the question was configured as fixed-choice is semantically wrong.
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/accept-edits-gate.ts
Line: 161-175
Comment:
**Quote-concatenation escape pattern blocks legitimate commands**
The pattern `/["'][a-z]["']["'][a-z]["']/i` (line 167) matches any adjacent single-character quoted tokens, not only those that reconstruct a destructive verb. For example, `echo "a""b"` or `python -c "x""=1"` would be blocked under `acceptEdits`, with no path to allow them short of asking the user for explicit confirmation.
The "false-positive discipline" test section in the companion test file doesn't cover adjacent-quoted non-destructive commands, so this over-blocking is undetected by the test suite. Given that `acceptEdits` is an elevated permission the user explicitly granted, surprising blocks on innocuous commands could be jarring. Consider narrowing the pattern to only flag adjacent quoted fragments that spell known destructive verbs (e.g., cross-reference with `DESTRUCTIVE_VERBS_FOR_ESCAPE_DETECTION`), or add a test + comment explicitly acknowledging this trade-off.
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/accept-edits-gate.ts
Line: 106-115
Comment:
**`DESTRUCTIVE_FIND_FLAGS` patterns lack the `/i` flag, inconsistent with peer patterns**
`DESTRUCTIVE_SQL_PATTERNS` and `DESTRUCTIVE_ESCAPE_PATTERNS` all use the `/i` (case-insensitive) flag. `DESTRUCTIVE_FIND_FLAGS` does not. The `-delete` flag and `-exec` are always lowercase in practice, but the inconsistency creates an undocumented assumption. More concretely, `find /tmp -type f -DELETE` (hypothetical) would slip through. Adding `/i` to these regexes aligns them with the rest of the pattern suite and removes the silent assumption.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(plan-mode): split PR4 advanced plan..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const allowFreetext = pendingQuestion.allowFreetext; | ||
| if (!allowFreetext) { | ||
| const offeredOptions = pendingQuestion.options; | ||
| if (offeredOptions.length > 0 && !offeredOptions.includes(answer)) { | ||
| return invalid( | ||
| `planApproval action="answer" rejected: answer "${answer}" not in offered options [${offeredOptions | ||
| .map((o) => `"${o}"`) | ||
| .join( | ||
| ", ", | ||
| )}] (the agent disabled free-text for this question — pick one of the offered options exactly)`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
When resolvePendingQuestionState resolves via the "legacy" path, pendingQuestion.options is entry.pendingQuestionOptions ?? []. For sessions persisted before pendingQuestionOptions was introduced, this is an empty array, so offeredOptions.length > 0 is false and the inner rejection never fires — any freetext answer passes through even when allowFreetext is false. A text-channel user on such a legacy session could submit arbitrary answer text that bypasses the contract the agent specified.
The guard should either (a) treat an empty stored options list as "no options known, skip validation" (i.e., current behaviour, documented explicitly), or (b) reject the answer entirely when allowFreetext === false && offeredOptions.length === 0, since accepting unvalidated text when the question was configured as fixed-choice is semantically wrong.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/sessions-patch.ts
Line: 705-716
Comment:
**Option validation skipped for legacy sessions with no stored options**
When `resolvePendingQuestionState` resolves via the `"legacy"` path, `pendingQuestion.options` is `entry.pendingQuestionOptions ?? []`. For sessions persisted before `pendingQuestionOptions` was introduced, this is an empty array, so `offeredOptions.length > 0` is `false` and the inner rejection never fires — any freetext answer passes through even when `allowFreetext` is `false`. A text-channel user on such a legacy session could submit arbitrary answer text that bypasses the contract the agent specified.
The guard should either (a) treat an empty stored options list as "no options known, skip validation" (i.e., current behaviour, documented explicitly), or (b) reject the answer entirely when `allowFreetext === false && offeredOptions.length === 0`, since accepting unvalidated text when the question was configured as fixed-choice is semantically wrong.
How can I resolve this? If you propose a fix, please make it concise.| // Quote concatenation: `"r""m" file`, `'r''m' file`. The | ||
| // concatenation of adjacent quoted fragments that together | ||
| // spell a destructive verb — catches the common "r""m" / | ||
| // "rm"+"" / "r"m patterns. Intentionally conservative — | ||
| // matches when adjacent quoted tokens start with the first | ||
| // letter of a destructive verb and can reconstruct into it. | ||
| /["'][a-z]["']["'][a-z]["']/i, | ||
| // Hex-encoded destructive verbs: `\x72m`, `\x72\x6d`. A | ||
| // destructive verb's first letter is `\xNN` followed by the | ||
| // remainder. Conservative — also flags any `\xNN` byte escape | ||
| // inside an exec command, which is itself highly suspicious | ||
| // under acceptEdits. | ||
| /\\x[0-9a-f]{2}/i, | ||
| // Octal-encoded bytes (e.g., `\162m`). | ||
| /\\[0-7]{3}/, |
There was a problem hiding this comment.
The pattern /["'][a-z]["']["'][a-z]["']/i (line 167) matches any adjacent single-character quoted tokens, not only those that reconstruct a destructive verb. For example, echo "a""b" or python -c "x""=1" would be blocked under acceptEdits, with no path to allow them short of asking the user for explicit confirmation.
The "false-positive discipline" test section in the companion test file doesn't cover adjacent-quoted non-destructive commands, so this over-blocking is undetected by the test suite. Given that acceptEdits is an elevated permission the user explicitly granted, surprising blocks on innocuous commands could be jarring. Consider narrowing the pattern to only flag adjacent quoted fragments that spell known destructive verbs (e.g., cross-reference with DESTRUCTIVE_VERBS_FOR_ESCAPE_DETECTION), or add a test + comment explicitly acknowledging this trade-off.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-mode/accept-edits-gate.ts
Line: 161-175
Comment:
**Quote-concatenation escape pattern blocks legitimate commands**
The pattern `/["'][a-z]["']["'][a-z]["']/i` (line 167) matches any adjacent single-character quoted tokens, not only those that reconstruct a destructive verb. For example, `echo "a""b"` or `python -c "x""=1"` would be blocked under `acceptEdits`, with no path to allow them short of asking the user for explicit confirmation.
The "false-positive discipline" test section in the companion test file doesn't cover adjacent-quoted non-destructive commands, so this over-blocking is undetected by the test suite. Given that `acceptEdits` is an elevated permission the user explicitly granted, surprising blocks on innocuous commands could be jarring. Consider narrowing the pattern to only flag adjacent quoted fragments that spell known destructive verbs (e.g., cross-reference with `DESTRUCTIVE_VERBS_FOR_ESCAPE_DETECTION`), or add a test + comment explicitly acknowledging this trade-off.
How can I resolve this? If you propose a fix, please make it concise.| const DESTRUCTIVE_SQL_PATTERNS: readonly RegExp[] = [ | ||
| /\bDROP\s+TABLE\b/i, | ||
| /\bDROP\s+DATABASE\b/i, | ||
| /\bDROP\s+SCHEMA\b/i, | ||
| /\bDELETE\s+FROM\b/i, | ||
| /\bTRUNCATE\s+(TABLE\s+)?/i, | ||
| // Redis | ||
| /\bFLUSHALL\b/i, | ||
| /\bFLUSHDB\b/i, | ||
| ]; |
There was a problem hiding this comment.
DESTRUCTIVE_FIND_FLAGS patterns lack the /i flag, inconsistent with peer patterns
DESTRUCTIVE_SQL_PATTERNS and DESTRUCTIVE_ESCAPE_PATTERNS all use the /i (case-insensitive) flag. DESTRUCTIVE_FIND_FLAGS does not. The -delete flag and -exec are always lowercase in practice, but the inconsistency creates an undocumented assumption. More concretely, find /tmp -type f -DELETE (hypothetical) would slip through. Adding /i to these regexes aligns them with the rest of the pattern suite and removes the silent assumption.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/plan-mode/accept-edits-gate.ts
Line: 106-115
Comment:
**`DESTRUCTIVE_FIND_FLAGS` patterns lack the `/i` flag, inconsistent with peer patterns**
`DESTRUCTIVE_SQL_PATTERNS` and `DESTRUCTIVE_ESCAPE_PATTERNS` all use the `/i` (case-insensitive) flag. `DESTRUCTIVE_FIND_FLAGS` does not. The `-delete` flag and `-exec` are always lowercase in practice, but the inconsistency creates an undocumented assumption. More concretely, `find /tmp -type f -DELETE` (hypothetical) would slip through. Adding `/i` to these regexes aligns them with the rest of the pattern suite and removes the silent assumption.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
Adds “advanced plan-mode interactions” to the plan-mode stack: clarifying-question flow via the approval pipeline, plan-mode self-introspection, plan archetype prompting + persistence, and post-approval “accept edits” constraints.
Changes:
- Extend
sessions.patchhandling to support plan-mode state transitions, plan approvals, question-answer routing, and queued next-turn injections. - Add new plan-mode tools (
ask_user_question,plan_mode_status,exit_plan_mode) and wire them into tool catalog + runtime interception. - Introduce plan archetype prompt + markdown persistence, and enforce accept-edits constraints during post-approval execution.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gateway/sessions-patch.ts | Implements plan-mode toggle/approval/question resolution and injection-queue writes. |
| src/gateway/sessions-patch.test.ts | Adds unit coverage for plan auto-mode, approval dedup, and question answer validation. |
| src/gateway/protocol/schema/sessions.ts | Extends sessions.patch schema with planMode/planApproval/lastPlanSteps fields. |
| src/config/sessions/types.ts | Adds session persistence types for pending interactions, injection queue, and post-approval permissions; adjusts token helpers. |
| src/auto-reply/reply/agent-runner-execution.ts | Consumes pending agent injections at turn start; wires fresh plan-mode/acceptEdits reads; adjusts compaction notice handling. |
| src/agents/tools/exit-plan-mode-tool.ts | Adds exit_plan_mode tool with subagent gate + archetype field parsing. |
| src/agents/tools/exit-plan-mode-tool.test.ts | Tests exit-plan-mode subagent gate and archetype-field parsing behaviors. |
| src/agents/tools/ask-user-question-tool.ts | Adds ask_user_question tool with options validation and deterministic questionId. |
| src/agents/tools/ask-user-question-tool.test.ts | Tests schema/validation, duplicate option rejection, and deterministic IDs. |
| src/agents/tool-description-presets.ts | Adds/updates plan-mode tool display summaries and detailed descriptions. |
| src/agents/tool-catalog.ts | Registers plan-mode tools (including ask_user_question) in the tool catalog. |
| src/agents/plan-mode/plan-archetype-prompt.ts | Adds plan archetype system prompt fragment + filename/slug helpers. |
| src/agents/plan-mode/plan-archetype-prompt.test.ts | Tests prompt fragment content and filename/slug helpers. |
| src/agents/plan-mode/plan-archetype-persist.ts | Persists plan archetype markdown to disk with collision handling + path hardening. |
| src/agents/plan-mode/plan-archetype-persist.test.ts | Tests persistence, collisions, traversal rejection, and recoverable storage errors. |
| src/agents/plan-mode/accept-edits-gate.ts | Implements accept-edits constraint gate + apply_patch target-path extraction. |
| src/agents/plan-mode/accept-edits-gate.test.ts | Adversarial tests for destructive/self-restart/config-change detection and move-path parsing. |
| src/agents/pi-embedded-subscribe.handlers.tools.ts | Intercepts plan-mode tools, persists plan-mode state, emits approval/question events, auto-approve, and nudges. |
| src/agents/pi-embedded-runner/run/attempt.ts | Prepends plan-mode rules/archetype/reference-card content into system prompt; threads planMode/getLatest* to hooks. |
| src/agents/openclaw-tools.ts | Registers plan-mode tools when feature-enabled and threads run/session context. |
Comments suppressed due to low confidence (1)
src/config/sessions/types.ts:742
resolveSessionTotalTokensappears to have been removed/renamed, but it’s still imported/used elsewhere (e.g.src/commands/sessions.tsandsrc/commands/status.summary.ts). This will break builds unless all call sites are updated in the same stack step. Consider reintroducingresolveSessionTotalTokens()as a thin alias (non-fresh semantics) or update all call sites to the new name in this PR/stack layer.
export function resolveFreshSessionTotalTokens(
entry?: Pick<SessionEntry, "totalTokens" | "totalTokensFresh"> | null,
): number | undefined {
const total = entry?.totalTokens;
if (typeof total !== "number" || !Number.isFinite(total) || total < 0) {
return undefined;
}
if (entry?.totalTokensFresh === false) {
return undefined;
}
return total;
| if (evt.stream === "compaction") { | ||
| const phase = readStringValue(evt.data.phase) ?? ""; | ||
| if (phase === "start") { | ||
| // Keep custom compaction callbacks active, but gate the | ||
| // fallback user-facing notice behind explicit opt-in. | ||
| const notifyUser = | ||
| runtimeConfig?.agents?.defaults?.compaction?.notifyUser === true; | ||
| if (params.opts?.onCompactionStart) { | ||
| await params.opts.onCompactionStart(); | ||
| } else if (shouldNotifyUserAboutCompaction) { | ||
| } else if (notifyUser && params.opts?.onBlockReply) { | ||
| // Send directly via opts.onBlockReply (bypassing the | ||
| // pipeline) so the notice does not cause final payloads | ||
| // to be discarded on non-streaming model paths. | ||
| await sendCompactionNotice("start"); | ||
| } | ||
| } | ||
| if (phase === "end") { | ||
| const completed = evt.data?.completed === true; | ||
| if (completed) { | ||
| attemptCompactionCount += 1; | ||
| if (params.opts?.onCompactionEnd) { | ||
| await params.opts.onCompactionEnd(); | ||
| } else if (shouldNotifyUserAboutCompaction) { | ||
| await sendCompactionNotice("end"); | ||
| const currentMessageId = | ||
| params.sessionCtx.MessageSidFull ?? params.sessionCtx.MessageSid; | ||
| const noticePayload = params.applyReplyToMode({ | ||
| text: "🧹 Compacting context...", | ||
| replyToId: currentMessageId, | ||
| replyToCurrent: true, | ||
| isCompactionNotice: true, | ||
| }); | ||
| try { | ||
| await params.opts.onBlockReply(noticePayload); | ||
| } catch (err) { | ||
| // Non-critical notice delivery failure should not | ||
| // bubble out of the fire-and-forget event handler. | ||
| logVerbose( | ||
| `compaction start notice delivery failed (non-fatal): ${String(err)}`, | ||
| ); | ||
| } | ||
| } else if (shouldNotifyUserAboutCompaction) { | ||
| await sendCompactionNotice("incomplete"); | ||
| } | ||
| } | ||
| const completed = evt.data?.completed === true; | ||
| if (phase === "end" && completed) { | ||
| attemptCompactionCount += 1; | ||
| await params.opts?.onCompactionEnd?.(); | ||
| } |
There was a problem hiding this comment.
Compaction user-notification behavior regressed: when agents.defaults.compaction.notifyUser is enabled and no onCompactionEnd callback is provided, the code sends the start notice but no longer sends the completion/incomplete notice. This contradicts existing tests and documented behavior (expects both “Compacting…” and “Compaction complete”). Restore end/incomplete notices (or update tests + schema help consistently).
| * Per-variant requirements: | ||
| * - `approve` / `edit`: only `approvalId` (optional but | ||
| * recommended for staleness protection). | ||
| * - `reject`: optional `feedback` (capped to 8 KiB to bound |
There was a problem hiding this comment.
The inline documentation contradicts the schema: it says reject has optional feedback, but the discriminated-union schema makes feedback required for action: "reject". Please align the comment with the actual contract so API consumers don’t implement the wrong behavior.
| * - `reject`: optional `feedback` (capped to 8 KiB to bound | |
| * - `reject`: REQUIRES `feedback` (capped to 8 KiB to bound |
| @@ -19,6 +34,7 @@ import { | |||
| } from "../auto-reply/thinking.js"; | |||
There was a problem hiding this comment.
This file declares planApprovalGateLog between import blocks and then continues with more static import statements. While valid JS, it violates the repo’s established import ordering convention (all imports first, then constants) and can trip import-sorting lint rules. Move the logger initialization below the full import section.
| // Live-test iter-3 R6a: always-on logger for the tool-side subagent | ||
| // gate at exit_plan_mode. Mirrors the iter-2 `gateway/plan-approval-gate` | ||
| // diagnostic so operators can see EVERY gate decision (including | ||
| // silent-bypass cases like missing runId or unregistered ctx) without | ||
| // flipping the env-gated plan-mode debug log. | ||
| const exitPlanGateLog = createSubsystemLogger("agents/exit-plan-gate"); | ||
| import { stringEnum } from "../schema/typebox.js"; | ||
| import { | ||
| describeExitPlanModeTool, | ||
| EXIT_PLAN_MODE_TOOL_DISPLAY_SUMMARY, | ||
| } from "../tool-description-presets.js"; | ||
| import { type AnyAgentTool, ToolInputError, readStringParam } from "./common.js"; |
There was a problem hiding this comment.
Same import-ordering issue as in sessions-patch.ts: exitPlanGateLog is initialized before later static imports. To match repo conventions (and avoid import-order lint failures), keep all imports together at the top and move exitPlanGateLog initialization below them.
| /["'][a-z]["']["'][a-z]["']/i, | ||
| // Hex-encoded destructive verbs: `\x72m`, `\x72\x6d`. A | ||
| // destructive verb's first letter is `\xNN` followed by the | ||
| // remainder. Conservative — also flags any `\xNN` byte escape | ||
| // inside an exec command, which is itself highly suspicious | ||
| // under acceptEdits. | ||
| /\\x[0-9a-f]{2}/i, | ||
| // Octal-encoded bytes (e.g., `\162m`). | ||
| /\\[0-7]{3}/, | ||
| ]; |
There was a problem hiding this comment.
The “byte-escape” detection is currently overly broad: /\\x[0-9a-f]{2}/i (and the octal escape regex) will block any command containing an escape sequence (e.g. printf "\x1b[31m"), even when it isn’t related to destructive verbs. This doesn’t match the surrounding comment that says the escape checks are “near a destructive verb” and risks false positives during normal execution. Consider tightening these patterns to only match escapes that could encode destructive verbs (or only apply them when other destructive indicators are present).
![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: 2cca8bfb0e
ℹ️ 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".
| return true; | ||
| } | ||
| const needle = `${prefix} `; | ||
| return cmd.startsWith(needle); |
There was a problem hiding this comment.
The destructive-action gate only matches when a destructive verb is the first token (cmd.startsWith("rm "), etc.), so commands like sudo rm -rf ... (or other wrappers such as env ... rm) bypass the hard constraint even when acceptEdits is enabled. In those sessions this allows destructive filesystem actions that are supposed to require explicit user confirmation; the matcher should normalize/unwrap common command prefixes or scan command tokens beyond position 0.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real concern, acknowledged. Not fixed in this immediate cycle (we're in cleanup-mode focused on maintainer-takeover; see #70101 for context). Tracked in the umbrella's deferred-items checklist for the bot-triage follow-up cycle. The fix will land on this branch with a Fixed in {SHA} reply here per the standard pr-review-loop pattern. Maintainers can also opt to merge the per-part PR as-is and address in a post-merge polish cycle.
| if (!prefix.startsWith("~/") && absoluteForm.startsWith(prefix)) { | ||
| return matchedProtectedPath(filePath, prefix); |
There was a problem hiding this comment.
Protected-path checks only use startsWith(prefix) where prefixes are declared with trailing slashes, so exact root targets like ~/.openclaw or /etc/openclaw do not match and are allowed. Under acceptEdits, a write/delete/edit against those exact directory roots should still be blocked as a config-change action; add an equality check for the slash-trimmed root in addition to descendant-prefix matching.
Useful? React with 👍 / 👎.
|
Acknowledging Greptile / Copilot / Codex review feedback on this PR. Triage status:
For end-to-end integration testing in the meantime, see #70071 [Plan Mode FULL] — the green-CI integrated bundle of all parts. |
2cca8bf to
567f96c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 567f96cc0e
ℹ️ 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".
| emitAgentApprovalEvent({ | ||
| runId: ctx.params.runId, | ||
| ...(ctx.params.sessionKey ? { sessionKey: ctx.params.sessionKey } : {}), | ||
| data: questionApprovalData, | ||
| }); |
There was a problem hiding this comment.
This branch emits the ask_user_question approval event immediately but never persists a matching pending-question record first, unlike the exit_plan_mode path just above. The new answer handler now hard-rejects planApproval.action="answer" when no pending question state exists, and there is no writer for pendingInteraction/pendingQuestionApprovalId in this commit, so user clicks can be rejected as “no pending ask_user_question” and the question flow becomes unusable.
Useful? React with 👍 / 👎.
| if (seg === "..") { | ||
| if (stack.length > 1 || (stack.length === 1 && stack[0] !== "")) { | ||
| stack.pop(); | ||
| } |
There was a problem hiding this comment.
collapse() drops leading .. segments when there is no prior stack, so paths like ../.openclaw/config.toml become .openclaw/config.toml before prefix checks. In a normal repo working directory under $HOME, that relative path resolves into the protected config directory, but checkProtectedPath will miss it and allow the write under acceptEdits, bypassing a hard config-change constraint.
Useful? React with 👍 / 👎.
…m state (v4) Cherry-pick / file-copy of 23 files from feat/plan-channel-parity-merged-upstream (merged-head 651a22d) onto current upstream/main. Branch: isolated/pm-3-of-6-advanced. v4 absorbs the upstream merge into plan-mode dev (Option B; merge commit b1ead74) plus the post-merge build/test fixes (commit 651a22d). v4 scope additions: src/agents/context-file-injection-scan.{ts,test.ts}, src/auto-reply/reply/commands-system-prompt.ts
567f96c to
09d9612
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09d961276b
ℹ️ 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 configChange = checkConfigChange(cmd); | ||
| if (configChange) { | ||
| return configChange; |
There was a problem hiding this comment.
The config-change gate only inspects exec/bash via checkConfigChange regexes (openclaw config ..., doctor --fix) and never applies protected-path checks to shell commands, so writes like echo x > ~/.openclaw/config.toml or sed -i ... /etc/openclaw/... pass under acceptEdits even though they are config mutations the policy says to block. This creates a direct bypass of the config constraint for any mutation done through shell redirection or tools like tee/sed.
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
This is the advanced plan-mode interactions layer. The 2/6 PR shipped the core: enter / update / exit, the mutation gate, the approval state machine, the subagent gate, plan persistence as markdown. That's enough to plan-then-execute, but it leaves the agent two-state — "planning" or "executing" — with no way to bring the user into the loop mid-plan, no way to self-introspect, and no permission tier between "user must approve every mutation" and "agent has free reign". This PR fills those gaps.
Concretely it adds:
ask_user_question(clarifying questions routed through the same approval-card pipeline asexit_plan_mode, plan-mode-safe — does not exit),plan_mode_status(read-only introspection so the agent can self-diagnose without inferring state from tool errors), plan archetypes (the persisted-markdown structure plus the system-prompt fragment that teaches Opus-quality decision-complete plans), and the accept-edits gate — Claude-Code-style auto-edit permission granted by the "Accept, allow edits" approval button, runtime-enforced against three hard constraints (no destructive actions, no self-restart, no config changes). Theexit_plan_modetool itself is extended in this PR to add the archetype fields (analysis/assumptions/risks/verification/references) and to maketitlemandatory at the schema layer.TL;DR
exit-plan-mode-tool.ts(archetype fields + mandatory title),sessions-patch.ts(planApprovaldiscriminated union + answer routing +acceptEditspermission grant),protocol/schema/sessions.ts(planApprovalwire schema),sessions/types.ts(PendingInteraction+PostApprovalPermissions),tool-catalog.ts+tool-description-presets.ts+openclaw-tools.ts(registration + presets),pi-embedded-runner/run/attempt.ts(live-readgetLatestAcceptEditsaccessor threading),agent-runner-execution.ts(acceptEdits accessor wiring),pi-embedded-subscribe.handlers.tools.ts(theask_user_questionruntime intercept that emits the approval event).ask_user_questiondoes NOT introduce a new approval kind — it piggybacks onkind:"plugin"(same payload shape asexit_plan_mode), with the consumer-side render switching on the presence of aquestionfield. Single approval persister ([Plan Mode 2/6] Core backend MVP #70066), single state machine, single answer routing path. The user clicks an option button →sessions.patch { planApproval: { action: "answer", answer, approvalId } }→ gateway validatesapprovalIdagainstpendingQuestionApprovalId→ enqueues a[QUESTION_ANSWER]:injection on next agent turn.rm,rmdir,unlink,shred,trash,truncate,find -delete,find -exec rm, SQLDROP TABLE/DELETE FROM/TRUNCATE TABLE, RedisFLUSHALL/FLUSHDB,diskutil erase{disk,all}. (2) self-restart —openclaw gateway {stop,restart,kill},launchctl {kickstart,unload,stop} ai.openclaw.*,systemctl {restart,stop,kill} openclaw*,pkill openclaw,kill <pid>co-located withopenclaw/gateway,kill $(pgrep openclaw),scripts/restart-mac.sh. (3) config changes —openclaw config {set,delete,unset},openclaw doctor --fix, write/edit/apply_patch into~/.openclaw/,~/.claude/,~/.config/openclaw/,/etc/openclaw/,/usr/local/etc/openclaw/. Plus a layered-defense escape-pattern detector: env-var indirection ($RM), backtick /$(...)subshell, quote concatenation ("r""m"), hex (\x72) and octal (\162) byte escapes near a destructive verb all block.Why this PR is split out
The plan-mode work in 2/6 ends at "agent submits plan, user approves verbatim, agent executes." That's the MVP. The advanced interactions are a coherent next slice — they share the approval-card pipeline, they share the discriminated
planApprovalschema, and they layer on top of the persisted-plan-cycle state from 2/6 — but they're additive enough to review independently. Splitting them out keeps the 2/6 review surface focused on "is the state machine right" without dragging in the question-routing UX, the archetype prompt-engineering, or the accept-edits enforcement matrix.Critical flows
Flow 1 —
ask_user_questionlifecycleThe clarifying-question loop. Agent calls
ask_user_questionmid-planning, the runtime intercepts the tool result and emits akind:"plugin"approval event with aquestionfield, the user picks an option, the answer arrives in the agent's next turn as a synthetic user message. No transition out of plan mode — the session stays armed for the agent to continue investigating or to callexit_plan_modeonce the answer lands.sequenceDiagram participant Agent participant Runtime as pi-embedded subscribe participant Gateway as sessions-patch participant UI as Control UI / Telegram / CLI participant User Agent->>Runtime: ask_user_question({ question, options[2..6], allowFreetext? }) Note over Agent,Runtime: schema enforces 2-6 options,<br/>rejects duplicate option text Runtime->>Runtime: detects status:"question_submitted"<br/>derives approvalId = `question-${toolCallId}`<br/>(deterministic — prompt-cache stable) Runtime->>Gateway: emit AgentApprovalEvent(kind:"plugin", question:{prompt, options, allowFreetext}) Gateway->>Gateway: persist PendingInteraction{kind:"question", approvalId, prompt, options} Gateway->>UI: agent_approval_event broadcast UI->>User: render N option buttons (web inline / Telegram inline / "/plan answer <choice>") User->>UI: clicks "1 PR" UI->>Gateway: sessions.patch { planApproval: { action:"answer", answer:"1 PR", approvalId } } Gateway->>Gateway: validate approvalId == pendingQuestionApprovalId<br/>reject if mismatched (stale-click guard) Gateway->>Gateway: enqueue PendingAgentInjection{kind:"question_answer", text:"[QUESTION_ANSWER]: 1 PR"} Gateway-->>Agent: next turn: synthetic user message<br/>"[QUESTION_ANSWER]: 1 PR" Agent->>Agent: continues plan; eventually calls exit_plan_mode<br/>(session was always still in plan mode)Flow 2 — Accept-edits gate decision tree
Granted when the user clicks "Accept, allow edits" (vs plain "Approve"). Layer 1 is the prompt —
buildAcceptEditsPlanInjectioninapproval.tsteaches the agent the three constraints. Layer 2 is this gate, called from the before-tool-call hook on EVERY tool call whenpostApprovalPermissions.acceptEdits === true. Fail-OPEN by design: only blocks on explicit matches; everything else passes.flowchart TD Tool[Tool call about to fire] --> Gate{getLatestAcceptEdits()<br/>fresh-from-disk read} Gate -- false --> AllowNoGate[allow — gate not invoked] Gate -- true --> Dispatch{toolName?} Dispatch -- exec / bash --> Cmd[exec command string] Dispatch -- write / edit / apply_patch --> Path[filePath + extracted additionalPaths] Dispatch -- other --> AllowOther[allow — outside gate scope] Cmd --> D1{matches DESTRUCTIVE_EXEC_PREFIXES<br/>rm / rmdir / shred / trash / truncate /<br/>diskutil erase…?} D1 -- yes --> BlockD[block — constraint:'destructive'] D1 -- no --> D2{matches DESTRUCTIVE_SQL_PATTERNS<br/>DROP TABLE / DELETE FROM /<br/>TRUNCATE / FLUSHALL?} D2 -- yes --> BlockD D2 -- no --> D3{matches DESTRUCTIVE_FIND_FLAGS<br/>find -delete / find -exec rm?} D3 -- yes --> BlockD D3 -- no --> D4{matches DESTRUCTIVE_ESCAPE_PATTERNS<br/>$RM / `…rm…` / $(…rm…) /<br/>quote-concat / hex / octal?} D4 -- yes --> BlockDE[block — constraint:'destructive'<br/>'shell-escape construct near destructive verb'] D4 -- no --> R1{matches SELF_RESTART_PATTERNS<br/>openclaw gateway stop / launchctl /<br/>pkill openclaw / kill $(pgrep openclaw)?} R1 -- yes --> BlockR[block — constraint:'self_restart'] R1 -- no --> C1{matches CONFIG_CHANGE_PATTERNS<br/>openclaw config set / delete / unset /<br/>openclaw doctor --fix?} C1 -- yes --> BlockC[block — constraint:'config_change'] C1 -- no --> AllowExec[allow] Path --> P1[normalize: expand ~,<br/>collapse .. and . segments,<br/>generate tildeForm + absoluteForm] P1 --> P2{any candidate path<br/>(filePath + apply_patch headers)<br/>starts with PROTECTED_CONFIG_PATH_PREFIXES?<br/>~/.openclaw/, ~/.claude/, /etc/openclaw/…} P2 -- yes --> BlockP[block — constraint:'config_change'<br/>'write to protected config path'] P2 -- no --> AllowPath[allow] BlockD --> Reason[return reason → 'ask the user for explicit confirmation'] BlockDE --> Reason BlockR --> Reason BlockC --> Reason BlockP --> ReasonFlow 3 — Plan archetype lifecycle
The archetype is a system-prompt fragment + a tool-schema extension + a disk artifact. It's appended to the system prompt when the session is in plan mode (PR-10 prompt fragment in
plan-archetype-prompt.ts); the agent fills in the archetype fields when it callsexit_plan_mode; the runtime persists the rendered markdown under~/.openclaw/agents/<agentId>/plans/plan-YYYY-MM-DD-<slug>.md; and on a future plan cycle the operator (or the agent reading the plans dir) can reference the prior plans for continuity.sequenceDiagram participant Skill as Skill / system prompt participant Agent participant ExitTool as exit_plan_mode participant Persister as plan-archetype-persist participant FS as ~/.openclaw/agents/<id>/plans/ Note over Skill: PLAN_ARCHETYPE_PROMPT appended to<br/>system prompt while planMode === "plan" Skill->>Agent: "produce a decision-complete plan with<br/>title, summary, analysis, plan[], assumptions,<br/>risks, verification, references" Agent->>Agent: investigates, reads files, web_search,<br/>maybe ask_user_question for tradeoffs Agent->>ExitTool: exit_plan_mode({ title (REQUIRED), plan[], analysis,<br/>assumptions, risks, verification, references }) ExitTool->>ExitTool: title schema-required (rejects with actionable<br/>error if missing — no silent "Active Plan" fallback) ExitTool->>ExitTool: subagent gate — block if openSubagentRunIds.size > 0 ExitTool->>Persister: persistPlanArchetypeMarkdown({ agentId, title, markdown }) Persister->>Persister: validate agentId (no /, \, control chars,<br/>no "." / ".." / dot-only) Persister->>Persister: mkdir baseDir, reject symlinks at agent/plans dirs,<br/>realpath() containment check Persister->>FS: writeFile(plan-2026-04-22-fix-foo.md, flag:"wx")<br/>(O_CREAT | O_EXCL — atomic, TOCTOU-safe) alt EEXIST (collision) Persister->>FS: retry with -2 / -3 / … suffix up to MAX_COLLISION_SUFFIX (99) else ENOSPC / EACCES / EIO Persister-->>ExitTool: throw PlanPersistStorageError(code)<br/>(operator-actionable; agent turn not retried) end Persister-->>ExitTool: { absPath, filename } ExitTool-->>Agent: tool result + approval card emitted Note over Agent,FS: Future cycles: operator / agent can grep<br/>~/.openclaw/agents/<id>/plans/ for prior plans;<br/>filenames sort chronologically by date prefixPer-file deep dive
src/agents/tools/ask-user-question-tool.ts(130 lines + 174-line test)What it does. Schema-validated tool that emits a
question_submittedtool result; the runtime intercept (seepi-embedded-subscribe.handlers.tools.ts:1815-1862) detects this result shape and fires anagent_approval_eventthrough the existingkind:"plugin"pipeline. The session stays in plan mode the entire time.Schema (
ask-user-question-tool.ts:32-60):The
additionalProperties: falsewas added in response to Copilot review #68939 to align with the same hardening applied toplan_mode_statusandenter_plan_mode— keeps the agent from smuggling extra fields through the tool surface that the runtime would silently drop (a class of bug we hit onupdate_planearly on).Runtime validation beyond schema (
ask-user-question-tool.ts:78-104):questionnon-empty after trim — rejects whitespace-only.optionslength 2-6 after filtering blanks — UI cap.["1 PR", "1 PR"]would be unrecoverable).Why
runIdis inCreateAskUserQuestionToolOptions. Same pattern asexit_plan_mode— the runtime threads itsrunIdso the tool can scope future approval/answer correlation if needed. Currently unused on the question side (the approvalId is derived fromtoolCallIdwhich is already run-scoped), but kept symmetric so a future per-run question dedup or rate-limit can drop in without a constructor signature change.Prompt-cache stability (
ask-user-question-tool.ts:107-112).questionId = q-${toolCallId}is deterministic. Earlier drafts usedcrypto.randomUUID()per call — that invalidated the prompt-cache prefix on every transcript replay (transcript repair, retry-after-error). The toolCallId is already stable for a given call, so byte-stable derivation gives free cache hits on replay.Tool-result content is non-empty (
ask-user-question-tool.ts:117). Earlier drafts returnedcontent: []; that tripped third-party transcript-pairing extensions (lossless-claw) which inject[lossless-claw] missing tool resultplaceholders into the agent's context on re-read. Now returns a one-line"Question submitted to user: ..."string so pairing-pass sees content.src/agents/plan-mode/accept-edits-gate.ts(564 lines + 629-line test)Posture: fail-OPEN. Unknown tools and commands ALLOW. The mutation-gate in plan mode is fail-CLOSED; this gate is post-approval execution, where the user opted into auto-edit, so the policy is "block the explicit three categories, allow everything else." Documented at the top of the file (
accept-edits-gate.ts:27-35).Layered defense. Layer 1 is
buildAcceptEditsPlanInjectioninapproval.ts(the prompt that teaches the agent the three constraints and tells it to ask before destructive/restart/config). Layer 2 is this file — runtime enforcement that fires even if the prompt is ignored. Together they're complementary; neither is sufficient alone (prompt can be ignored / instruction-tuned around; runtime can be bypassed via shell escapes the gate doesn't recognize). Documented ataccept-edits-gate.ts:36-46.The three constraints.
Destructive (
accept-edits-gate.ts:88-176, 272-315). Three sub-checks: prefix match against a curated list (rm,rmdir,unlink,shred,trash,truncate,diskutil erasedisk,diskutil eraseall); SQL pattern match (DROP TABLE,DROP DATABASE,DROP SCHEMA,DELETE FROM,TRUNCATE TABLE, RedisFLUSHALL/FLUSHDB); find-family flag match (find ... -delete,find ... -exec rm,-execdir rm). Plus the C4 escape-vector layer (see below). Prefix matching uses an exact-or-trailing-space boundary sormtool --helpandrmate config.tomlare NOT false-positives — there's a baseline test ataccept-edits-gate.test.ts:99-107covering this.Self-restart (
accept-edits-gate.ts:198-218, 317-330). Patterns target the gateway specifically:openclaw gateway {stop|restart|kill},launchctl {kickstart|unload|stop} ai.openclaw.*,systemctl {restart|stop|kill} openclaw*,pkill openclaw,killall openclaw,kill <n>? ...openclaw|gateway, plus the indirect formspgrep openclaw | xargs kill(matched on the source side) andkill $(pgrep openclaw)/kill `pgrep openclaw`(matched on the subshell side). The bundled operator helperscripts/restart-mac.shis also listed.Config changes (
accept-edits-gate.ts:223-248, 332-345, 404-438). Two-pronged: command-pattern match (openclaw config {set|delete|unset},openclaw doctor --fix) AND path-prefix match for write/edit/apply_patch tools targeting~/.openclaw/,~/.claude/,~/.config/openclaw/,/etc/openclaw/,/usr/local/etc/openclaw/. Path normalization (accept-edits-gate.ts:357-402) expands~, collapses..and.segments, and produces BOTH a tilde form and an absolute form so a write to~/.openclaw/../.openclaw/config.tomlor/Users/x/.openclaw/config.tomlboth resolve to the same protected target.The C4 escape-vector layer (
accept-edits-gate.ts:130-192). This is the live-test iter-1/2/3-equivalent hardening for this gate — the prefix/SQL/find checks catch the 99% case, but a sophisticated agent could shell-escape: env-var indirection ($RM file,${SHRED}), backtick subshell (`echo rm` file),$(…)subshell ($(echo rm) file), quote concatenation ("r""m" file), hex byte escapes (\x72m), octal byte escapes (\162m). All of these match an explicit pattern and block. Rationale spelled out ataccept-edits-gate.ts:140-149: underacceptEditsthe user opted in for trusted-plan execution, not for cleverness budget; legitimate post-approval execs rarely need env-var indirection for destructive verbs; primary defense remains the prompt layer; this is defense-in-depth so a prompt-ignoring agent can't shell-escape silently.apply_patchmulti-path coverage (accept-edits-gate.ts:60-69, 480-553).apply_patchcarries paths in the patch envelope (*** Update File: <path>,*** Add File: <path>,*** Delete File: <path>,*** Move to: <dst>), not in aparams.pathfield. Without parsing them out, a malicious patch could write to~/.openclaw/config.tomland bypass the protected-path check (the singularfilePathwould be absent).extractApplyPatchTargetPathsparses all four envelope verbs, dedupes, and the caller threads them asadditionalPathsfor the gate to check. The*** Move to:regex was a Codex review #68939 fix — the actualapply_patchgrammar uses Move-to as a SUB-marker nested inside an*** Update File:hunk, NOT the older*** Move File: src -> dstsingle-line form; pre-fix the regex matched the non-existent form and missed every real Move destination.What "≥95% confidence" means in practice. It's the prompt-side bar (Layer 1), not a numerical threshold the gate reads. The injection text in
approval.tstells the agent: "you may self-modify the plan during execution AT HIGH CONFIDENCE (≥95%); for anything you're uncertain about, ask the user." There's no probability variable in the gate code — the agent's self-assessment is what gates Layer 1, and Layer 2 hard-blocks the three categories regardless of confidence. The two layers compose: agent self-restraint on uncertain edits, runtime hard-block on the three categories.The fail-OPEN posture is intentional and asymmetric to the plan-mode mutation gate (which is fail-CLOSED). The reason: in plan mode the user has not seen or approved any plan yet, so the safest default is "block unknown until the user explicitly opts in." Under acceptEdits the user has already approved a plan AND opted into auto-edit; the safest default flips to "allow unknown, hard-block the explicit dangerous categories." Inverting this would mean adding a per-tool allowlist for normal post-approval mutations and a per-command allowlist for execs — high churn cost for no real safety win, since the prompt + gate already cover the realistic threat model (an agent ignoring the constraint guidance and dispatching a destructive call).
How the gate is wired into the runtime.
getLatestAcceptEdits(live-read accessor, threaded throughattempt.ts:642-644) is consulted by the before-tool-call hook on every tool call. When it returns true, the hook callscheckAcceptEditsConstraint(params)with the toolName, exec command (if applicable), filePath (if applicable), andextractApplyPatchTargetPaths(params.input)forapply_patchcalls. Ifresult.blocked === true, the tool call is rejected with theresult.reasonstring surfaced as the error — actionable text the agent can read and re-route throughask_user_questionfor explicit user confirmation.src/agents/plan-mode/plan-archetype-persist.ts(217 lines + 249-line test)What it does. Atomically persists the rendered plan markdown under
~/.openclaw/agents/<agentId>/plans/plan-YYYY-MM-DD-<slug>.md. Always written, regardless of session origin (web/CLI/Telegram/etc.) — operators get a durable audit trail of everyexit_plan_modecycle. Telegram document delivery is layered on top byplan-archetype-bridge.ts(lands in 5/6).Idempotence + collision handling (
plan-archetype-persist.ts:152-179). Atomic create withwxflag (O_CREAT | O_EXCL) — the OS rejects the open withEEXISTif the file already exists. Caught and retried with-2,-3, … up toMAX_COLLISION_SUFFIX = 99. This was a Copilot review #68939 fix from a priorexistsSync+writeFilepattern that had a TOCTOU window (parallel agent calls writing the same date+slug could race the existence check). With per-day filenames and 99-cap, production-unreachable but defensive.Path-traversal defense (
plan-archetype-persist.ts:74-150). Three layers::85-92) — rejects/,\, control characters (\p{Cc}to satisfy the no-control-regex lint rule), and./../ dot-only.:111-117) —path.resolve(target).startsWith(path.resolve(baseDir)).:118-150) — Copilot review feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939 fix: a pre-existing symlink like~/.openclaw/agents/<id> -> /etcwould bypass the syntactic + lexical checks (the path component is fine; the symlink target is the escape vector). Nowlstat()s each component, refuses if it's a symlink, thenrealpath()s base + target and re-checks containment.Recoverable storage errors (
plan-archetype-persist.ts:181-217).ENOSPC/EACCES/EIOare wrapped inPlanPersistStorageErrorwith a distinctive prefix so the bridge / caller can surface an actionable operator message rather than confuse it with a bug. Plan-mode treats these as non-fatal — the plan approval still proceeds; only the durable audit artifact is lost.src/agents/plan-mode/plan-archetype-prompt.ts(168 lines + 100-line test)The system-prompt fragment (
plan-archetype-prompt.ts:14-134). Adapted from a hand-tuned "Plan Mode" prompt and tightened for OpenClaw's tool surface. Sits ON TOP of the existing plan-mode prompt rules — those cover the action contract ("don't write the plan in chat, use exit_plan_mode") while this fragment covers the QUALITY of the plan submitted: required fields onexit_plan_mode, decision-completeness bar, anti-patterns, when to useask_user_question, the "Questions DO NOT exit plan mode" clarification, and the self-check before submission.Filename helpers (
plan-archetype-prompt.ts:142-168).buildPlanFilenameSluglowercases, normalizes NFKD, strips diacritics, collapses non-alphanumeric to single hyphens, trims, slices to maxLen, re-trims. Falls back to"untitled"(NOT"plan"— Copilot review #68939 caught a doc bug claiming the latter; the helper has always returned"untitled").buildPlanFilenameprefixes with ISO date so plans sort chronologically:plan-2026-04-22-fix-websocket-reconnect.md.What the prompt explicitly forbids (anti-pattern list at
plan-archetype-prompt.ts:89-98). The fragment was tuned against six observed agent failure modes from live testing: (1) "bare file list with no analysis" — the kind of plan that looks complete but skips the why; (2) "three vague paragraphs followed by 'and we add tests as needed'" — handwave on verification; (3) "title that's actually the agent's chat narration" —'I checked all five VMs...'is analysis text, not a title (this directly seeded the mandatory-title schema check); (4) "defers key behavior decisions to 'implementation will decide'" — pushes hidden decisions into execution; (5) "invents repo facts (paths, exports, types) without having read them" — the rule thatConcrete: name real files, modules, symbols, APIs, schemas, configsis a direct response to this; (6) "mixes must-have changes with optional nice-to-haves" — bloats the approval surface. Each anti-pattern is a real instance the team saw in early plan-mode rollout and is now explicitly called out so the agent self-rejects before submission.src/agents/tools/exit-plan-mode-tool.ts(modified — +418 net incl. test churn)The 2/6 PR shipped the basic
exit_plan_modetool. This PR extends it with:Mandatory
title(exit-plan-mode-tool.ts:51-60, 219-230). PR-9 / Bug 2/6:titleis now REQUIRED and rejected with an actionableToolInputErrorif missing. Pre-fix, the approval card defaulted to"Active Plan"/"Plan approval requested"(uninformative for the user) and the persisted markdown filename slug fell back tountitled(uninformative for the operator browsing~/.openclaw/agents/<id>/plans/). Schema-level rejection beats a silent fallback — the agent retries on the next attempt with a real title.Archetype fields (
exit-plan-mode-tool.ts:90-143, 357-418).analysis,assumptions,risks({risk, mitigation}[]),verification,references— all optional and backwards-compatible. The plan-archetype prompt fragment tells the agent which are required for which kind of plan (e.g.analysisrequired for non-trivial multi-file changes;verificationrequired for any plan that ships code).readPlanArchetypeFieldsparses each defensively (trim + drop blank entries) so a malformed agent payload doesn't poison the approval card.Tool-side subagent gate (
exit-plan-mode-tool.ts:254-310). Iter-3 R6a always-on diagnostic + iter-1 R3 hard-block. When the parent run has open subagent runs (research spawned during plan-mode investigation),exit_plan_moderejects the submission with aToolInputErrorlisting the pending children (truncated to 5 with "and N more"). Plus theSUBAGENT_SETTLE_GRACE_MSwindow: if the last subagent completed less than the grace ms ago, block to let completion events propagate before the approval-resume turn fires (prevents the announce-turn-races-approval RW1 race window).Always-on diagnostic line (
exit-plan-mode-tool.ts:267-269). Everyexit_plan_modecall emits ONE structured line to gateway.err.log via theagents/exit-plan-gatesubsystem logger:This was added in iter-3 R6a after a class of bug where the gate silently bypassed (no runId, ctx not registered, openSubagentRunIds undefined) without leaving a trace — operators couldn't tell from logs whether the gate fired or not. Now operators can grep
agents/exit-plan-gatefor every submission attempt and see the decision plus the reason for any bypass.Supporting changes
src/agents/openclaw-tools.ts(+28 / -1) — registerscreateAskUserQuestionToolandcreatePlanModeStatusToolbehind the same plan-mode-enabled gate asenter_plan_mode/exit_plan_mode. Theplan_mode_statustool itself is referenced by registration here but its implementation file is owned by Plan Mode 2/6 ([Plan Mode 2/6] Core backend MVP #70066) so the dependency is honored.src/agents/tool-catalog.ts(+31) —ask_user_questioncatalog entry,codingprofile,includeInOpenClawGroup: true. Plan-mode enabled gate inherited from the registration site.src/agents/tool-description-presets.ts(+87) —ASK_USER_QUESTION_TOOL_DISPLAY_SUMMARY,PLAN_MODE_STATUS_TOOL_DISPLAY_SUMMARY,describePlanModeStatusTool,describeAskUserQuestionTool. Plus pointer text on every plan-mode tool description: "To inspect live plan-mode state at runtime, callplan_mode_status(read-only diagnostic)" — gives the agent a single source of truth for self-debugging.src/config/sessions/types.ts(+327 / -11) —PostApprovalPermissions(acceptEdits,grantedAt,approvalId),PendingInteraction(discriminated union overkind:"plan" | "question"),PendingInteractionStatus,PendingAgentInjectionKind(typed kinds for the priority-ordered injection queue that supersedes the legacypendingAgentInjection: stringfield).src/gateway/protocol/schema/sessions.ts(+183) — refactorsplanApprovalfrom a flat optional-fields object to a discriminated union overaction, with per-variant required fields (rejectrequiresfeedback1-8192 chars;answerrequiresanswertext +approvalId;autorequiresautoEnabled). Pre-fix all per-action fields were Optional and the runtime validated post-hoc; the runtime checks remain as defense-in-depth but are now unreachable on the happy path. Adds thelastPlanStepspatch field with closed status enum (pending | in_progress | completed | cancelled) and Wave B1 closure-gate fields (acceptanceCriteria,verifiedCriteria).src/gateway/sessions-patch.ts(+767 / -2) — answer routing forplanApproval.action === "answer"(:641-680), validatesapprovalIdagainstpendingQuestionApprovalId(server-side answer-guard), enqueues aPendingAgentInjectionEntryofkind:"question_answer".acceptEditspermission grant onaction === "edit"(:947-969), explicit clear onaction === "approve"so a prior cycle's grant doesn't carry forward. Plan-mode cycle entry clears any stalepostApprovalPermissions(:610).src/gateway/sessions-patch.test.ts(+603) — coverage for the new discriminated-union validation, answer-routing happy path, answer-routing stale-approvalId rejection,autoaction gate-OFF rejection, etc. (Note: 50 tests in the file total; the question/answer/acceptEdits subset is the new surface area.)src/agents/pi-embedded-runner/run/attempt.ts(+132 / -1) — threadsgetLatestAcceptEdits(live-read accessor; pattern mirrorsgetLatestPlanMode) into the embedded runner so the before-tool-call hook can re-check after mid-turn approval transitions without a stale snapshot. Unrelated WIP in the originating commit was stripped during the cherry-pick (attempt.ts:635-644).src/auto-reply/reply/agent-runner-execution.ts(+205 / -43) — wiresresolveLatestAcceptEditsFromDisk(fromfresh-session-entry.ts) as the live-read accessor passed to the runner. Same disk-fresh pattern asresolveLatestPlanModeFromDisk.src/agents/pi-embedded-subscribe.handlers.tools.ts(+760) — the runtime intercept forask_user_question. Detectsstatus === "question_submitted"in the tool-resultdetails, derives a deterministicapprovalId = question-${toolCallId}(prompt-cache stability — deep-dive review fix; was previouslyquestion-<timestamp>-<random>which surfaced as duplicate stale cards), emits anagent_approval_eventwithkind:"plugin"+ aquestionfield. The plan-card UI switches to a question-render branch when the field is present.Runtime data flow
ask_user_questiontool body (ask-user-question-tool.ts:76-128)pi-embedded-subscribe.handlers.tools.ts:1815-1862)AgentApprovalEventstream/plan answersessions-patch.tsanswer branch (:641-680)sessions.patch { planApproval: action:"answer" }sessions-patch.ts:641-680≠ pendingQuestionApprovalIdsessions-patch.tsanswer branchpendingAgentInjections[]queueSessionEntrywriteagent-runner-execution.ts(composePromptWithPendingInjection)[QUESTION_ANSWER]: ...exit_plan_mode(still in plan mode)/plan accept editssessions-patch.tsapprove branch (:947-969)sessions.patch { planApproval: action:"edit" }acceptEditspermission setsessions-patch.ts:958-963SessionEntry.postApprovalPermissionscheckAcceptEditsConstraint(accept-edits-gate.ts:455-506)getLatestAcceptEditsask_user_question)Security properties (with file:line evidence)
additionalProperties: falseonask_user_questionschemaask-user-question-tool.ts:59additionalProperties: falseonexit_plan_modeplan-step schemaexit-plan-mode-tool.ts:74additionalProperties: falseonexit_plan_moderisks-entry schemaexit-plan-mode-tool.ts:117additionalProperties: falseonplanApprovaldiscriminated union (every variant)protocol/schema/sessions.ts(eachType.Object(...)in the union)accept-edits-gate.ts:455-506(dispatch),:88-176(destructive),:198-218(self-restart),:223-248(config-change cmd),:242-248(config-change paths)accept-edits-gate.ts:130-192(patterns +checkDestructiveEscape)apply_patchmulti-path extraction (single-path verbs + Move-to)accept-edits-gate.ts:521-553(extractApplyPatchTargetPaths); caller threads viaadditionalPaths~,..,., double-slashaccept-edits-gate.ts:357-402(normalizeCandidatePath)exit_plan_modesubagent block when research children in flightexit-plan-mode-tool.ts:281-292(hard reject with child IDs);:297-309(settle-grace window)exit_plan_modemandatory title at schema layer (no silent fallback)exit-plan-mode-tool.ts:219-230plan-archetype-persist.ts:85-92(syntactic),:111-117(lexical),:118-150(symlink + realpath)O_CREAT | O_EXCLviawxflag)plan-archetype-persist.ts:170acceptEditspermission scoped byapprovalId(no cycle-A → cycle-B leak)sessions/types.ts:94-98, cleared on plan-mode entry atsessions-patch.ts:610acceptEditsgranted only onaction === "edit", explicitly cleared onaction === "approve"sessions-patch.ts:947-969approvalIdagainstpendingQuestionApprovalIdsessions-patch.ts:641-680(answer guard); schema-level requirement atprotocol/schema/sessions.ts(answer variantapprovalId: NonEmptyString)approvalId/questionIdderivation (prompt-cache stable)ask-user-question-tool.ts:107-112(questionId),pi-embedded-subscribe.handlers.tools.ts:1827-1833(approvalId)Review-cycle history (carried forward from #68939)
Each new file carries inline
Copilot review #68939andCodex P1/P2 review #68939markers pointing to the specific original-umbrella comment that motivated the fix. Notable carries on this PR's surface:additionalProperties: falseonask_user_questionschema (Copilot feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939, 2026-04-19) —ask-user-question-tool.ts:57-59. Aligns with the same hardening onplan_mode_statusandenter_plan_mode.exit_plan_modediscriminated-union refactor ofplanApproval(Copilot feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939, 2026-04-19) —protocol/schema/sessions.ts. Per-variant required fields (rejectrequiresfeedback,answerrequiresanswer+approvalId,autorequiresautoEnabled).rejectrequiresfeedbackat schema (Copilot feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939, 2026-04-19) —protocol/schema/sessions.ts. Closes the loophole where a malformed client could submit "reject with no guidance" and leave the agent stuck.lastPlanSteps[].statusclosed enum (Copilot feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939, 2026-04-19) —protocol/schema/sessions.ts. WasNonEmptyString, now matchesPlanStepStatusruntime type so an arbitrary status can't drift through into UI rendering.wx-flag plan persist (Copilot feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939, 2026-04-19) —plan-archetype-persist.ts:170. Replaced the priorexistsSync+writeFilepattern that had a TOCTOU window.realpath()-based containment + symlink rejection (Copilot feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939, 2026-04-19) —plan-archetype-persist.ts:118-150. Catches the symlink-as-escape-vector class.*** Move to:SUB-marker recognition inapply_patch(Codex review feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939, 2026-04-20) —accept-edits-gate.ts:537. Pre-fix the regex matched a non-existent*** Move File: src -> dstform and missed every real Move destination.question-answer routing requiresapprovalId(Codex P1 feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939) —protocol/schema/sessions.ts(answer variant) +sessions-patch.tsanswer guard. Without this a stale or accidental/plan answercould overwritependingAgentInjectionwith garbage.accept-edits-gate.ts:130-192. Layered defense for env-var / subshell / quote-concat / hex / octal byte escapes near a destructive verb.questionId/approvalIdderivation (PR-10 review H5) —ask-user-question-tool.ts:107-112,pi-embedded-subscribe.handlers.tools.ts:1827-1833. Replaced random suffixes that invalidated prompt-cache prefixes on transcript replay.titleonexit_plan_mode(PR-9 Tier 1 + Bug 2/6 fix) —exit-plan-mode-tool.ts:219-230. Schema rejection beats silent"Active Plan"fallback.exit-plan-mode-tool.ts:297-309. Prevents announce-turn-races-approval window where the parent's announce turn collides with the approval-resume turn.agents/exit-plan-gatediagnostic (iter-3 R6a) —exit-plan-mode-tool.ts:267-269. Everyexit_plan_modecall emits one structured line; operators can grep silent-bypass cases.Backward compatibility
ask_user_question,plan_mode_status) are registered behindagents.defaults.planMode.enabled(the same gate asenter_plan_mode/exit_plan_modein 2/6). Sessions where plan mode is OFF see no behavioral change.analysis/assumptions/risks/verification/referencesonexit_plan_modeare all optional; agents that don't fill them in submit a plain step-list plan as before. The system-prompt fragment tells the agent when each is required for QUALITY, but the schema accepts the bare-minimum (title+plan[]) form.acceptEditsdefaults to absent.postApprovalPermissionsisundefinedby default. Granted only on the explicitaction: "edit"approval (the "Accept, allow edits" button), explicitly cleared onaction: "approve"(verbatim execution) and on entry into a new plan-mode cycle. The gate is not invoked at all whenacceptEditsis false — the runtime only calls it whengetLatestAcceptEdits()returns true.exit_plan_modemandatory title is the one breaking change at the tool surface. Mitigation: the rejection error is actionable ("Re-call exit_plan_mode with the title field included. Example: title: 'Refactor websocket reconnect race'."), and plan mode is opt-in anyway, so existing on-disk sessions running normal mode never see it. Agents that were callingexit_plan_modewithout a title in 2/6 received a"Active Plan"fallback header; they now get a clear retry signal instead.planApprovaldiscriminated-union schema is wire-additive — pre-existing fields (approve/edit/reject/auto) keep their semantics;answeris new. Older clients that don't know aboutanswersimply don't send it. Older servers that don't know about it would have rejected the field asadditionalProperties: false, but those servers also lack theask_user_questionruntime intercept, so the question wouldn't have been emitted in the first place.PendingInteractionis server-side only. Persisted onSessionEntry, not on the wire. Legacy session-on-disk shapes lacking the field are accepted (it's optional); writes always populate the new shape.Test coverage matrix
accept-edits-gate.test.tsrm/rm -rf/rmdir/shred/trash/unlink/truncate, prefix non-collision (rmtool,rmate), SQLDROP TABLE/DELETE FROM, find-delete/-exec rm. Self-restart: `openclaw gateway stopask-user-question-tool.test.tsstatus: "question_submitted",questionIdderivation, non-emptycontent).plan-archetype-persist.test.ts/,\, control chars,./.., dot-only). Path-traversal containment. Symlink rejection at agent + plans dirs.realpath()-based containment.EEXISTretry loop,MAX_COLLISION_SUFFIXcap.ENOSPC/EACCES/EIO→PlanPersistStorageErrorwith code preserved.plan-archetype-prompt.test.ts"untitled"fallback for empty/whitespace/pure-punctuation. Filename helper: ISO date prefix, slug,.mdsuffix, chronological sort.exit-plan-mode-tool.test.tsToolInputErrorwith retry guidance. Archetype-fields parsing: blank-entry filtering, malformed-payload tolerance.sessions-patch.test.tsplanApproval.action === "answer"happy path, stale-approvalId rejection, missing-approvalIdrejection.action === "auto"feature-gate.acceptEditsgrant onedit, explicit clear onapprove, clear on plan-mode-cycle entry.PendingInteractionshape on the SessionEntry side.Total new test lines this PR: 2,022 across 6 test files.
Parity benchmark callout
User ran a benchmark testing pass where the same prompts hit OpenClaw + Codex + Claude Code on the same Anthropic + OpenAI models. Headline numbers:
For the advanced-interactions surface specifically:
ask_user_questionpattern matches Claude Code's clarifying-question pattern. Both surface the question through the same approval channel as the destructive-action approval (Claude Code's permission dialog; OpenClaw's plan-approval card pipeline). Both use a constrained N-option choice with optional freetext fallback. Both wait synchronously for the answer (no background polling). Both inject the answer back as a synthetic user message tagged with a stable marker ([QUESTION_ANSWER]:here; equivalent in Claude Code).approvalId, cleared on cycle entry) and hard-blocks the same three classes plus the layered escape-vector detector. The behavior delta is scope (workspace vs cycle) — OpenClaw is tighter; the constraint set is convergent.~/.openclaw/agents/<id>/plans/) rather than declarative templates, but the plan-shape is the same: required title + step-list + assumptions/risks/verification.Mergeability scorecard
agents.defaults.planMode.enabled; new tools registered only when on.planApprovaldiscriminated union extends prior optional-fields shape;lastPlanStepsis a new optional patch field.SessionEntryshape changependingInteraction,postApprovalPermissions,pendingAgentInjections[]all optional; legacy on-disk shapes load fine.exit_plan_modemandatory titleToolInputErrorwith retry guidance; plan mode opt-in.sessions-patch.test.ts.planMode.enabled: falseadditionalProperties: falseon every new schema; runtime gate on every new permission tier; always-on diagnostic on every gate decision.exit_plan_mode(off the LLM hot path).What a reviewer can verify in <30 min
accept-edits-gate.ts+accept-edits-gate.test.ts— read top-of-file rationale (47 lines), skim the constants tables (DESTRUCTIVE_EXEC_PREFIXES,SQL_PATTERNS,FIND_FLAGS,ESCAPE_PATTERNS,SELF_RESTART_PATTERNS,CONFIG_CHANGE_PATTERNS,PROTECTED_CONFIG_PATH_PREFIXES), then runpnpm test src/agents/plan-mode/accept-edits-gate.test.ts(629 lines, ~80 cases) — see all three constraint classes plus the escape vectors green. (10 min)ask-user-question-tool.ts+ test — read schema (lines 32-60), the duplicate-rejection rule (:96-104), the deterministicquestionIdderivation (:107-112). Run the test file. (5 min)exit-plan-mode-tool.ts— read the mandatory-title block (:219-230), the archetype-fields schema (:90-143), the subagent gate (:254-310). Runpnpm test src/agents/tools/exit-plan-mode-tool.test.ts. (5 min)sessions-patch.tsanswer routing + acceptEdits grant —:641-680(answer routing + stale-approvalId guard),:947-969(grant + clear semantics). Cross-referenceprotocol/schema/sessions.tsdiscriminated union for the wire schema. (5 min)plan-archetype-persist.tssecurity review — read:74-150(the three layers of path-traversal defense). Runpnpm test src/agents/plan-mode/plan-archetype-persist.test.ts. (5 min)Total: ~30 min for a confident green-light on the security-critical surface.
What this PR does NOT include
plan_mode_statustool source. Referenced fromopenclaw-tools.ts(registration) andtool-description-presets.ts(preset) here, but the implementation file lives in[Plan Mode 2/6] Core backend MVP([Plan Mode 2/6] Core backend MVP #70066) which this PR depends on. CI red on this PR will resolve once 2/6 lands.[Plan Mode 4/6] Web UI + i18n./plan accept,/plan reject,/plan answer, Telegram inline buttons). →[Plan Mode 5/6] Text channels + Telegram.[Plan Mode AUTOMATION]([Plan Mode AUTOMATION] Cron nudges + auto-enable + subagent follow-ups #70089) + bundled in[Plan Mode FULL]([Plan Mode FULL] Integrated bundle for testing (Parts 1\u20136 + automation + executing-state lifecycle) #70071).archetype: "bug-fix"in frontmatter) is a follow-up.planMode.autoEnableForruntime wiring. Schema-reserved onagents.defaults.planMode.autoEnableFor; cron-time scanner deferred to[Plan Mode FULL].approvalTimeoutSecondscron watchdog. Schema-reserved; auto-dismiss of stale approval cards is a known follow-up.Failure-mode walk
A few realistic ways the new surface area can fail in production, and what happens:
ask_user_questionwith malformed payload (e.g. 1 option, 7 options, duplicate options, blank options): rejected with aToolInputErrorlisting the specific failure ("options must contain at least 2 non-empty strings","options contain duplicate text: 'foo'", etc.). The agent re-attempts with a corrected payload. No approval event fires, no UI render, no race condition. Covered byask-user-question-tool.test.ts:75-103.approvalIdguard atsessions-patch.ts:641-680validates the incomingapprovalIdagainstpendingQuestionApprovalId; mismatch → reject the patch. Stale clicks don't pollute the injection queue.exit_plan_modewhile subagents are in flight: tool throwsToolInputErrorwith the open run IDs (truncated to 5 with "and N more"), the gateway.err.log gets a structuredagents/exit-plan-gateline for the operator, the agent's next turn surfaces the error and the agent waits for completion before re-attempting.exit-plan-mode-tool.test.ts:50-86covers the listing + truncation + guidance cases.PlanPersistStorageError(ENOSPC)thrown with operator-facing prefix; the bridge surfaces an actionable warn-level log line; plan-mode treats this as non-fatal, the approval still proceeds, only the durable markdown audit is lost. The operator sees the exact code in logs and can free space and retry.~/.openclaw/agents/<id>pointing at/etc:lstat()detects the symlink at the agent-dir level and refuses withagent directory must not be a symlink: .... No write happens. The operator sees the rejection in logs, recreates the directory as a real dir.rm -rf build/: gate matchesDESTRUCTIVE_EXEC_PREFIXESrm, returns{blocked: true, constraint: 'destructive', reason: 'Command "rm" is a destructive action and is blocked under acceptEdits. Ask the user for explicit confirmation before proceeding.'}. Tool call rejected; agent reads the reason, callsask_user_questionto get explicit confirmation, then dispatchesrmonly after the user answers.kill $(pgrep openclaw): matchesSELF_RESTART_PATTERNSsubshell pattern, blocked withconstraint: 'self_restart'. Even if the agent trieskill `pgrep openclaw`(backtick variant), the alternate regex catches it.apply_patchwith a hunk that moves into~/.openclaw/config.toml:extractApplyPatchTargetPathsparses the*** Move to: ~/.openclaw/config.tomlenvelope (Codex review feat(plan-mode): full rollout + iter-1/2/3 hardening (consolidates PRs A/B/C/D/E/F/7/8/10/11) #68939 fix),additionalPathscarries it to the gate,checkProtectedPathmatches the prefix, blocked withconstraint: 'config_change'.Issue references
Files in scope
Primary review targets (security-sensitive surface):
src/agents/plan-mode/accept-edits-gate.ts+ test — three-constraint gate, escape-vector layer, apply_patch multi-pathsrc/agents/tools/ask-user-question-tool.ts+ test — schema, duplicate rejection, deterministic ID derivationsrc/agents/plan-mode/plan-archetype-persist.ts+ test — three-layer path-traversal defense, atomic createsrc/agents/tools/exit-plan-mode-tool.ts+ test — mandatory title, archetype fields, subagent gateWire / state changes:
src/gateway/protocol/schema/sessions.ts—planApprovaldiscriminated union,lastPlanStepspatchsrc/gateway/sessions-patch.ts+ test — answer routing,acceptEditsgrant + clear semanticssrc/config/sessions/types.ts—PendingInteraction,PostApprovalPermissions, typed injection queueSupporting:
src/agents/plan-mode/plan-archetype-prompt.ts+ test — system-prompt fragment, slug helperssrc/agents/openclaw-tools.ts,src/agents/tool-catalog.ts,src/agents/tool-description-presets.ts— registration + presetssrc/agents/pi-embedded-runner/run/attempt.ts—getLatestAcceptEditsaccessor threadingsrc/auto-reply/reply/agent-runner-execution.ts— accessor wiringsrc/agents/pi-embedded-subscribe.handlers.tools.ts—ask_user_questionruntime interceptCarry-forward / deferred
planMode.autoEnableForruntime wiring →[Plan Mode FULL]approvalTimeoutSecondscron watchdog →[Plan Mode FULL]PR-8 review fix Codex P1 #3098235203 — Decision C option (b))[Plan Mode 5/6](gated on upstream Telegram SDK surface re-add)