[Plan Mode 6/6] Docs, QA, and help#70070
Conversation
openclaw-barnacle
Bot
added
docs
Greptile SummaryThis PR adds operator documentation, user-facing concept docs, a
Confidence Score: 4/5Safe to merge for the docs/skill content; fix the cancelled-status scenario hang and the self-test inconsistency before CI runs the QA suite. Two P1 findings: the waitForOutboundMessage text filter in gpt54-cancelled-status.md can produce a CI hang, and SKILL.md advertises /plan self-test as callable when the runtime explicitly dropped it. Neither blocks the documentation portion of this PR, but both should be corrected before the QA scenarios run in an automated gate. qa/scenarios/gpt54-cancelled-status.md (hanging filter + weak assertion) and skills/plan-mode-101/SKILL.md (undocumented deferred command). Prompt To Fix All With AIThis is a comment left during a code review.
Path: qa/scenarios/gpt54-cancelled-status.md
Line: 44-50
Comment:
**`waitForOutboundMessage` filter on `text.includes('update_plan')` will likely hang**
The lambda waits for an outbound message whose `text` contains the literal string `'update_plan'`. Agents typically call tools silently — the plan-tool call appears in `toolCalls`, not in the assistant's chat text. If the agent never mentions "update_plan" in its reply body, this `waitFor` will spin until the scenario timeout (120 s), turning a slow-to-answer model into a CI hang rather than a clear assertion failure.
The other four scenarios in this PR use the simpler `candidate.conversation.id === 'qa-room'` filter, which is sufficient here too: the step-level `assert` on `message.toolCalls?.some(tc => tc.name === 'update_plan')` at line 52 already verifies the tool was actually called.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/plan-mode-101/SKILL.md
Line: 99-143
Comment:
**`/plan self-test` documented as available but is deferred / dropped**
The slash-command table (line 101) lists `/plan self-test` as a callable command, and lines 129-143 describe its 10-step execution in present tense. However, `docs/plans/PLAN-MODE-ARCHITECTURE.md` explicitly marks this feature as "dropped" during iter-3 planning (D5, "The runtime now uses `plan_mode_status`, focused regression suites…") and "still deferred" in the 1.0 follow-up table (C7b). Consistently, the user-facing `docs/concepts/plan-mode.md` slash-command table omits `/plan self-test` entirely.
Agents loading this skill will try `/plan self-test`, get an error, and be confused about whether plan mode itself is broken. The self-test section should either be removed or clearly marked as `(not yet implemented)` / `(coming in a follow-up PR)` until C7b lands.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: qa/scenarios/gpt54-cancelled-status.md
Line: 53-55
Comment:
**`?? true` fallback makes the cancelled-status assertion vacuously pass**
`message.toolCalls?.some(...) ?? true` evaluates to `true` when `message.toolCalls` is `undefined` — meaning the assertion trivially passes when no tool calls were recorded at all. The Copilot review comment reproduced in `gpt54-plan-mode-default-off.md` (lines 56-67) explains exactly this operator-precedence trap and rewrites negation asserts as `(... ?? false) === false`.
For a positive assertion the correct fallback is `?? false` (no toolCalls → assertion fails, surfacing the real problem):
```suggestion
- assert:
expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? false"
message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)"
```
Note: even with the corrected fallback, this assertion can only pass when the agent's execution actually triggers a step failure during the run. Consider whether the task prompt reliably induces a failure scenario (e.g., deliberate non-existent command), otherwise the `cancelled` path may never be exercised.
```suggestion
- assert:
expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? false"
message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)"
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs(plan-mode): split PR9 docs QA and h..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| args: | ||
| - ref: state | ||
| - lambda: | ||
| params: [candidate] | ||
| expr: "candidate.conversation.id === 'qa-room' && candidate.text.includes('update_plan')" | ||
| - expr: liveTurnTimeoutMs(env, 120000) | ||
| - assert: |
There was a problem hiding this comment.
waitForOutboundMessage filter on text.includes('update_plan') will likely hang
The lambda waits for an outbound message whose text contains the literal string 'update_plan'. Agents typically call tools silently — the plan-tool call appears in toolCalls, not in the assistant's chat text. If the agent never mentions "update_plan" in its reply body, this waitFor will spin until the scenario timeout (120 s), turning a slow-to-answer model into a CI hang rather than a clear assertion failure.
The other four scenarios in this PR use the simpler candidate.conversation.id === 'qa-room' filter, which is sufficient here too: the step-level assert on message.toolCalls?.some(tc => tc.name === 'update_plan') at line 52 already verifies the tool was actually called.
Prompt To Fix With AI
This is a comment left during a code review.
Path: qa/scenarios/gpt54-cancelled-status.md
Line: 44-50
Comment:
**`waitForOutboundMessage` filter on `text.includes('update_plan')` will likely hang**
The lambda waits for an outbound message whose `text` contains the literal string `'update_plan'`. Agents typically call tools silently — the plan-tool call appears in `toolCalls`, not in the assistant's chat text. If the agent never mentions "update_plan" in its reply body, this `waitFor` will spin until the scenario timeout (120 s), turning a slow-to-answer model into a CI hang rather than a clear assertion failure.
The other four scenarios in this PR use the simpler `candidate.conversation.id === 'qa-room'` filter, which is sufficient here too: the step-level `assert` on `message.toolCalls?.some(tc => tc.name === 'update_plan')` at line 52 already verifies the tool was actually called.
How can I resolve this? If you propose a fix, please make it concise.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.
| | `/plan answer <text>` | Answer a pending `ask_user_question` | | ||
| | `/plan auto on` / `/plan auto off` | Toggle auto-approve mode (future plans auto-resolve as approved) | | ||
| | `/plan self-test` | Run the synthetic plan-mode flow end-to-end + report pass/fail | | ||
|
|
||
| ## Common pitfalls | ||
|
|
||
| 1. **Don't post chat after `exit_plan_mode` in the same turn.** Trailing assistant text breaks the approval card lifecycle and the user gets stuck. | ||
| 2. **Wait for spawned subagents BEFORE `exit_plan_mode`.** The tool-side gate will reject your submission if any spawned subagents are still running. The error message lists their child run IDs. | ||
| 3. **`update_plan` does NOT submit.** It only tracks progress. Use `exit_plan_mode` to propose to the user. | ||
| 4. **Don't re-enter plan mode after approval.** Just continue executing. Re-enter only for a NEW planning cycle (different objective, separate user request). | ||
| 5. **Provide a meaningful `title`.** It becomes the persisted markdown filename (`plan-YYYY-MM-DD-<slug>.md`) AND the side-panel header. Generic titles like "Test plan" make plans hard to find later. | ||
| 6. **Don't submit empty plans.** A plan with zero steps will be rejected by the runtime. | ||
|
|
||
| ## Debugging tips | ||
|
|
||
| If something goes wrong: | ||
|
|
||
| ```bash | ||
| # Turn on structured plan-mode debug logging: | ||
| openclaw config set agents.defaults.planMode.debug true | ||
| # Restart gateway via menubar app or: launchctl kickstart -k gui/$UID/ai.openclaw.gateway | ||
|
|
||
| # Tail the structured debug stream: | ||
| tail -F ~/.openclaw/logs/gateway.err.log | grep '\[plan-mode/' | ||
|
|
||
| # Tail the always-on approval-gate log (no env var needed): | ||
| tail -F ~/.openclaw/logs/gateway.err.log | grep 'plan-approval-gate' | ||
| ``` | ||
|
|
||
| ## Self-test | ||
|
|
||
| Run `/plan self-test` to verify the local install. The command: | ||
|
|
||
| 1. Pre-checks gateway health + plan-mode config | ||
| 2. Enters plan mode | ||
| 3. Calls `update_plan` with a 2-step test plan | ||
| 4. Calls `exit_plan_mode` with a synthetic plan + title `"plan-mode self-test"` | ||
| 5. Verifies the approval card emits with correct title + plan steps | ||
| 6. Auto-resolves approval | ||
| 7. Verifies mutation gate unlocks | ||
| 8. Verifies the markdown file written to `~/.openclaw/agents/<id>/plans/plan-YYYY-MM-DD-plan-mode-self-test.md` | ||
| 9. Verifies debug log fires (when debug flag enabled) | ||
| 10. Cleans up the test plan | ||
|
|
||
| A passing run means plan mode is correctly wired end-to-end on this install. A failing step lists the specific surface that's broken (tool wiring, persister, approval handler, mutation gate, etc.) so you can investigate. |
There was a problem hiding this comment.
/plan self-test documented as available but is deferred / dropped
The slash-command table (line 101) lists /plan self-test as a callable command, and lines 129-143 describe its 10-step execution in present tense. However, docs/plans/PLAN-MODE-ARCHITECTURE.md explicitly marks this feature as "dropped" during iter-3 planning (D5, "The runtime now uses plan_mode_status, focused regression suites…") and "still deferred" in the 1.0 follow-up table (C7b). Consistently, the user-facing docs/concepts/plan-mode.md slash-command table omits /plan self-test entirely.
Agents loading this skill will try /plan self-test, get an error, and be confused about whether plan mode itself is broken. The self-test section should either be removed or clearly marked as (not yet implemented) / (coming in a follow-up PR) until C7b lands.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/plan-mode-101/SKILL.md
Line: 99-143
Comment:
**`/plan self-test` documented as available but is deferred / dropped**
The slash-command table (line 101) lists `/plan self-test` as a callable command, and lines 129-143 describe its 10-step execution in present tense. However, `docs/plans/PLAN-MODE-ARCHITECTURE.md` explicitly marks this feature as "dropped" during iter-3 planning (D5, "The runtime now uses `plan_mode_status`, focused regression suites…") and "still deferred" in the 1.0 follow-up table (C7b). Consistently, the user-facing `docs/concepts/plan-mode.md` slash-command table omits `/plan self-test` entirely.
Agents loading this skill will try `/plan self-test`, get an error, and be confused about whether plan mode itself is broken. The self-test section should either be removed or clearly marked as `(not yet implemented)` / `(coming in a follow-up PR)` until C7b lands.
How can I resolve this? If you propose a fix, please make it concise.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.
| - assert: | ||
| expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? true" | ||
| message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)" |
There was a problem hiding this comment.
?? true fallback makes the cancelled-status assertion vacuously pass
message.toolCalls?.some(...) ?? true evaluates to true when message.toolCalls is undefined — meaning the assertion trivially passes when no tool calls were recorded at all. The Copilot review comment reproduced in gpt54-plan-mode-default-off.md (lines 56-67) explains exactly this operator-precedence trap and rewrites negation asserts as (... ?? false) === false.
For a positive assertion the correct fallback is ?? false (no toolCalls → assertion fails, surfacing the real problem):
| - assert: | |
| expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? true" | |
| message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)" | |
| - assert: | |
| expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? false" | |
| message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)" |
Note: even with the corrected fallback, this assertion can only pass when the agent's execution actually triggers a step failure during the run. Consider whether the task prompt reliably induces a failure scenario (e.g., deliberate non-existent command), otherwise the cancelled path may never be exercised.
| - assert: | |
| expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? true" | |
| message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)" | |
| - assert: | |
| expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? false" | |
| message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: qa/scenarios/gpt54-cancelled-status.md
Line: 53-55
Comment:
**`?? true` fallback makes the cancelled-status assertion vacuously pass**
`message.toolCalls?.some(...) ?? true` evaluates to `true` when `message.toolCalls` is `undefined` — meaning the assertion trivially passes when no tool calls were recorded at all. The Copilot review comment reproduced in `gpt54-plan-mode-default-off.md` (lines 56-67) explains exactly this operator-precedence trap and rewrites negation asserts as `(... ?? false) === false`.
For a positive assertion the correct fallback is `?? false` (no toolCalls → assertion fails, surfacing the real problem):
```suggestion
- assert:
expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? false"
message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)"
```
Note: even with the corrected fallback, this assertion can only pass when the agent's execution actually triggers a step failure during the run. Consider whether the task prompt reliably induces a failure scenario (e.g., deliberate non-existent command), otherwise the `cancelled` path may never be exercised.
```suggestion
- assert:
expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? false"
message: "If a step fails, it should be marked cancelled (not silently dropped or marked completed)"
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3674d5d6e2
ℹ️ 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".
| expr: '!message.text.includes("I don''t have access")' | ||
| message: "Response should not claim lack of access to time" | ||
| - assert: | ||
| expr: "message.toolCalls?.some(tc => tc.name === 'exec' || tc.name === 'code_execution' || tc.name === 'session_status') ?? false" |
There was a problem hiding this comment.
message.toolCalls assertions with observable QA signals
This assertion depends on message.toolCalls, but qa-channel outbound messages are QaBusMessage records that only carry text/message metadata (no tool call array), so the check cannot reliably pass even when the model used the correct tool. In practice this makes the new GPT-5.4 scenarios fail or produce misleading results (same pattern appears in qa/scenarios/gpt54-act-dont-ask.md, qa/scenarios/gpt54-cancelled-status.md, and qa/scenarios/gpt54-plan-mode-default-off.md). Please assert tool usage via the mock request log (/debug/requests) or another runtime surface that actually records tool calls.
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.
| - ref: state | ||
| - lambda: | ||
| params: [candidate] | ||
| expr: "candidate.conversation.id === 'qa-room' && candidate.text.includes('update_plan')" |
There was a problem hiding this comment.
update_plan text
Waiting for an outbound message whose text includes "update_plan" is brittle and can time out even when planning succeeded, because tool invocation is not guaranteed to be echoed verbatim in assistant text on qa-channel. This turns the scenario into a false negative before the actual assertions run; use a conversation-only wait and validate tool use through the mock debug request stream instead.
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.
| | `/plan revise <feedback>` | Reject with revision feedback | | ||
| | `/plan answer <text>` | Answer a pending `ask_user_question` | | ||
| | `/plan auto on` / `/plan auto off` | Toggle auto-approve mode (future plans auto-resolve as approved) | | ||
| | `/plan self-test` | Run the synthetic plan-mode flow end-to-end + report pass/fail | |
There was a problem hiding this comment.
/plan self-test command from skill surface
This command is documented as available here, but the architecture doc in the same change set states the self-test command was dropped (docs/plans/PLAN-MODE-ARCHITECTURE.md, D5). Keeping /plan self-test in the skill causes operators and agents to attempt a command that is no longer part of the shipped plan-mode contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds operator-/agent-facing documentation and QA scenario content for the proposed “plan mode” workflow, plus a plan-mode-101 skill intended to teach plan-mode semantics.
Changes:
- Added plan-mode architecture + operator runbook docs under
docs/plans/. - Added user-facing concept doc (
docs/concepts/plan-mode.md) and prompt-stack spec (docs/agents/prompt-stack-spec.md). - Added
plan-mode-101skill content and several GPT‑5.4 QA scenarios.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/plan-mode-101/SKILL.md | Adds an agent skill describing plan-mode lifecycle, tags, and slash commands. |
| docs/plans/PLAN-MODE-OPERATOR-RUNBOOK.md | Adds ops runbook for diagnosing plan-mode incidents and reading gateway logs. |
| docs/plans/PLAN-MODE-ARCHITECTURE.md | Adds a long-form architecture/status reference for the plan-mode rollout. |
| docs/concepts/plan-mode.md | Adds user-facing concept documentation for plan mode. |
| docs/agents/prompt-stack-spec.md | Adds a prompt-stack specification and guidance for GPT‑5 prompt overlays. |
| qa/scenarios/gpt54-plan-mode-default-off.md | Adds QA scenario asserting GPT‑5.4 doesn’t enter plan mode by default. |
| qa/scenarios/gpt54-mandatory-tool-use.md | Adds QA scenario asserting GPT‑5.4 uses tools for factual queries. |
| qa/scenarios/gpt54-injection-scan.md | Adds QA scenario asserting injection scanner doesn’t interfere with normal prompts. |
| qa/scenarios/gpt54-cancelled-status.md | Adds QA scenario asserting failed plan steps get cancelled status. |
| qa/scenarios/gpt54-act-dont-ask.md | Adds QA scenario asserting GPT‑5.4 acts on obvious defaults without clarifying questions. |
| ## 4. Critical files reference | ||
|
|
||
| | Surface | File | Owner PR | | ||
| | ------------------------------------ | ----------------------------------------------------------------------------- | --------------------------- | | ||
| | Plan checklist renderer (4 formats) | `src/agents/plan-render.ts` | PR-C | | ||
| | Plan archetype markdown render | `src/agents/plan-render.ts` (`renderFullPlanArchetypeMarkdown`) | PR-14 | | ||
| | Mutation gate | `src/agents/plan-mode/mutation-gate.ts` | PR-D | | ||
| | Plan-mode runtime + retry | `src/agents/pi-embedded-runner/run/incomplete-turn.ts` | PR-D | | ||
| | Cross-session plan store (file lock) | `src/agents/plan-store.ts` | PR-F | | ||
| | Skill plan templates | `src/agents/skills/skill-planner.ts` | PR-E | | ||
| | Task parity + merge | `src/agents/tools/update-plan-tool.ts` | PR-B | | ||
| | Plan archetype prompt | `src/agents/plan-mode/plan-archetype-prompt.ts` | PR-10 | | ||
| | Plan filename helpers | `src/agents/plan-mode/plan-archetype-prompt.ts` (`buildPlanFilename`) | PR-10 | | ||
| | Plan markdown persist | `src/agents/plan-mode/plan-archetype-persist.ts` | PR-14 | | ||
| | Plan-mode → channel bridge | `src/agents/plan-mode/plan-archetype-bridge.ts` | PR-14 | | ||
| | `enter_plan_mode` tool | `src/agents/tools/enter-plan-mode-tool.ts` | PR-8 | | ||
| | `exit_plan_mode` tool | `src/agents/tools/exit-plan-mode-tool.ts` | PR-8/PR-10 | | ||
| | `ask_user_question` tool | `src/agents/tools/ask-user-question-tool.ts` | PR-10 | | ||
| | Universal `/plan` handler | `src/auto-reply/reply/commands-plan.ts` | PR-11 | | ||
| | Webchat `/plan` executor | `ui/src/ui/chat/slash-command-executor.ts` | PR-11 | |
There was a problem hiding this comment.
The “Critical files reference” table lists multiple paths that don’t exist in this repo at HEAD (e.g. src/agents/plan-mode/mutation-gate.ts, src/agents/tools/enter-plan-mode-tool.ts, src/auto-reply/reply/commands-plan.ts). If these files live only on an unmerged feature branch, the doc should say so (or gate the table by version/branch); otherwise update the paths to match the actual code layout so the table is usable.
| **Click Approve but nothing happens / agent doesn't continue:** | ||
|
|
||
| - Likely the agent posted chat after `exit_plan_mode` in the same turn (a known anti-pattern). Re-prompt with "continue executing the approved plan." | ||
| - Check whether the session is disconnected. The Control UI disables approval actions while offline; reconnect first. | ||
| - If the card still looks live after reconnect, refresh the page to force a fresh session snapshot. | ||
| - Inspect live state with `plan_mode_status` and confirm the session is still on the same active cycle and not blocked by `blockingSubagentRunIds`. | ||
|
|
There was a problem hiding this comment.
This doc references the plan_mode_status tool for live introspection, but there is no plan_mode_status tool in the current codebase (no matches in src/). If this is a future tool, mark it as not yet shipped; otherwise update the doc to point to the actual introspection surface that exists today.
| docsRefs: [] | ||
| codeRefs: | ||
| - src/agents/execution-contract.ts | ||
| - src/agents/plan-mode/types.ts |
There was a problem hiding this comment.
The codeRefs entry src/agents/plan-mode/types.ts does not exist in the current repository (no src/agents/plan-mode/ directory). Update/remove the reference so scenario metadata doesn’t point to a dead path.
| - src/agents/plan-mode/types.ts |
| # GPT-5.4 act-don't-ask | ||
|
|
||
| ```yaml qa-scenario | ||
| id: gpt54-act-dont-ask | ||
| title: GPT-5.4 acts on obvious defaults instead of asking for clarification | ||
| surface: agent |
There was a problem hiding this comment.
qa/scenarios/index.md documents that scenario markdown should live under the one-level theme directories (agents/, models/, runtime/, etc.), but this PR adds gpt54-*.md scenarios at the root of qa/scenarios/. Consider moving these into an appropriate theme folder (e.g., qa/scenarios/models/ or qa/scenarios/agents/) to match the pack structure and keep the catalog organized.
| - `EIO`: underlying storage / FUSE / NFS issue — not an OpenClaw | ||
| bug. | ||
|
|
||
| Re-run `/plan restate` after fixing to re-materialize the markdown. |
There was a problem hiding this comment.
This runbook instructs operators to re-run /plan restate, but there’s no /plan restate command implementation in the current codebase (no matches in src/). If this command is planned/deferred, call that out here; otherwise update this step to the actual supported recovery action for re-materializing the markdown.
| Re-run `/plan restate` after fixing to re-materialize the markdown. | |
| There is currently no supported `/plan restate` recovery command in | |
| the codebase. After fixing the underlying storage issue, new plan | |
| writes will persist normally, but the lost markdown artifact is not | |
| automatically re-materialized by an operator command. |
| # PR-D review fix (Copilot #3105216710): explicit toolCalls assertion | ||
| # — message.text alone could pass even if the agent invoked the tool | ||
| # silently. Verify the tool was NOT actually called. | ||
| # Copilot review #68939 (2026-04-19): rewrote the negation | ||
| # asserts as `(some(...) ?? false) === false` to make the | ||
| # operator precedence explicit. The previous form | ||
| # `!message.toolCalls?.some(...) ?? true` is misleading | ||
| # because `!` always produces a boolean, so the `?? true` | ||
| # branch can never fire — easy to misread as "default to true | ||
| # if toolCalls is missing" when it actually behaves as | ||
| # "negate the some()". Rewriting as `(... ?? false) === false` | ||
| # is explicit about the intent: "we expect false (i.e., NOT | ||
| # in toolCalls); if toolCalls is missing, treat as 'tool was | ||
| # not called', which is what the assertion wants". | ||
| - assert: | ||
| expr: "(message.toolCalls?.some(tc => tc.name === 'enter_plan_mode') ?? false) === false" | ||
| message: "Agent must NOT invoke enter_plan_mode for a simple task" | ||
| - assert: | ||
| expr: "message.toolCalls?.some(tc => tc.name === 'read') ?? false" | ||
| message: "Agent must call read tool to check package.json, not guess the content" | ||
| - assert: | ||
| expr: "(message.toolCalls?.some(tc => tc.name === 'update_plan') ?? false) === false" | ||
| message: "Agent should not create a plan for a simple read-and-report task" |
There was a problem hiding this comment.
These assertions rely on message.toolCalls, but QA-channel outbound messages are QaBusMessage objects without tool-call metadata (src/plugin-sdk/qa-channel-protocol.ts:24-44). In particular, the read-tool assertion will always fail because toolCalls is always undefined here. To verify tool usage / non-usage, assert via a surface that exposes tool calls (mock provider journal, gateway run events, or transcript inspection) rather than message.toolCalls.
| # PR-D review fix (Copilot #3105216710): explicit toolCalls assertion | |
| # — message.text alone could pass even if the agent invoked the tool | |
| # silently. Verify the tool was NOT actually called. | |
| # Copilot review #68939 (2026-04-19): rewrote the negation | |
| # asserts as `(some(...) ?? false) === false` to make the | |
| # operator precedence explicit. The previous form | |
| # `!message.toolCalls?.some(...) ?? true` is misleading | |
| # because `!` always produces a boolean, so the `?? true` | |
| # branch can never fire — easy to misread as "default to true | |
| # if toolCalls is missing" when it actually behaves as | |
| # "negate the some()". Rewriting as `(... ?? false) === false` | |
| # is explicit about the intent: "we expect false (i.e., NOT | |
| # in toolCalls); if toolCalls is missing, treat as 'tool was | |
| # not called', which is what the assertion wants". | |
| - assert: | |
| expr: "(message.toolCalls?.some(tc => tc.name === 'enter_plan_mode') ?? false) === false" | |
| message: "Agent must NOT invoke enter_plan_mode for a simple task" | |
| - assert: | |
| expr: "message.toolCalls?.some(tc => tc.name === 'read') ?? false" | |
| message: "Agent must call read tool to check package.json, not guess the content" | |
| - assert: | |
| expr: "(message.toolCalls?.some(tc => tc.name === 'update_plan') ?? false) === false" | |
| message: "Agent should not create a plan for a simple read-and-report task" | |
| # NOTE: `message` here is a QA-channel outbound message, which | |
| # does not expose tool-call metadata. Do not assert on | |
| # `message.toolCalls` from this surface; verify tool usage via a | |
| # provider journal, gateway run events, or transcript inspection | |
| # in a scenario that captures one of those richer artifacts. |
| ### GPT-5 boot reorder (Round 2, landing with this spec) | ||
|
|
||
| For OpenAI GPT-5 models only, workspace-file load order is adjusted: | ||
|
|
||
| | Default order | GPT-5 override | | ||
| | --------------------- | ----------------------------- | | ||
| | AGENTS.md (weight 10) | SOUL.md (10) | | ||
| | SOUL.md (20) | IDENTITY.md (20) | | ||
| | IDENTITY.md (30) | AGENTS.md (30) | | ||
| | USER.md (40) | USER.md (40) — unchanged | | ||
| | TOOLS.md (50) | TOOLS.md (50) — unchanged | | ||
| | BOOTSTRAP.md (60) | BOOTSTRAP.md (60) — unchanged | | ||
| | MEMORY.md (70) | MEMORY.md (70) — unchanged | | ||
|
|
There was a problem hiding this comment.
This spec says the “GPT-5 boot reorder” is already landing with this doc, but the current implementation in src/agents/system-prompt.ts uses a single fixed CONTEXT_FILE_ORDER map (AGENTS→SOUL→IDENTITY…) with no GPT-5-specific override. Either update the doc to describe it as a proposed/deferred change, or land the corresponding code change so the docs match reality.
| expr: "message.toolCalls?.some(tc => tc.name === 'exec') ?? false" | ||
| message: "Agent must call exec to check the port (e.g. lsof, netstat, ss), not answer from memory" |
There was a problem hiding this comment.
waitForOutboundMessage here returns a QA bus message (QaBusMessage), which does not include a toolCalls field (see src/plugin-sdk/qa-channel-protocol.ts:24-44). As written, this assertion will always evaluate to false, so the scenario can’t actually verify tool use. To test tool usage, switch to a surface that exposes tool-call metadata (e.g., mock-provider request logs, agent run transcript inspection, or a dedicated QA-lab API that captures tool events) and assert against that instead of message.toolCalls.
| expr: "message.toolCalls?.some(tc => tc.name === 'exec') ?? false" | |
| message: "Agent must call exec to check the port (e.g. lsof, netstat, ss), not answer from memory" | |
| expr: "message.text.toLowerCase().includes('8080') && (message.text.toLowerCase().includes('open') || message.text.toLowerCase().includes('closed') || message.text.toLowerCase().includes('listening') || message.text.toLowerCase().includes('not listening'))" | |
| message: "Response should report a concrete local port-check result for port 8080 rather than asking follow-up questions" |
| - assert: | ||
| expr: "message.toolCalls?.some(tc => tc.name === 'exec' || tc.name === 'code_execution' || tc.name === 'session_status') ?? false" | ||
| message: "Agent must use a tool (exec/code_execution/session_status) to check the time, not answer from memory" |
There was a problem hiding this comment.
This scenario asserts message.toolCalls, but outbound QA-channel messages are QaBusMessage objects and don’t carry tool call metadata (src/plugin-sdk/qa-channel-protocol.ts:24-44). This means the scenario can’t actually confirm tool usage. Consider validating tool use via the mock provider’s request journal in mock mode, or by querying run events/transcripts from the gateway instead of relying on message.toolCalls.
| - ref: state | ||
| - lambda: | ||
| params: [candidate] | ||
| expr: "candidate.conversation.id === 'qa-room' && candidate.text.includes('update_plan')" |
There was a problem hiding this comment.
waitForOutboundMessage filters on candidate.text.includes('update_plan'), but QA-channel message text is just the assistant’s user-facing reply and will not reliably include tool names. This can cause the scenario to hang/timeout. Prefer filtering only on conversation/thread (and then assert tool activity via a tool-call-aware surface rather than message text).
| expr: "candidate.conversation.id === 'qa-room' && candidate.text.includes('update_plan')" | |
| expr: "candidate.conversation.id === 'qa-room'" |
|
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. |
f4ae594 to
5bcc9c7
Compare
openclaw-barnacle
Bot
added
channel: telegram
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bcc9c79a4
ℹ️ 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".
| @@ -741,7 +740,7 @@ export async function sendMessageTelegram( | |||
| } | |||
| const res = await sendTelegramTextChunk(chunk, buildTextParams(index === chunks.length - 1)); | |||
| const messageId = resolveTelegramMessageIdOrThrow(res, context); | |||
| recordSentMessage(chatId, messageId, cfg); | |||
| recordSentMessage(chatId, messageId); | |||
There was a problem hiding this comment.
This call dropped the cfg argument, so sent-message records are always written to the default cache path instead of the session-store-specific bucket. In deployments that set session.store (or run multiple scoped stores), wasSentByBot(chatId, messageId, cfg) reads from a different bucket and fails to recognize bot-authored messages, which breaks reactionNotifications: "own" filtering and causes valid reactions to be ignored.
Useful? React with 👍 / 👎.
| api.pinChatMessage(chatId, messageId, { | ||
| disable_notification: opts.notify !== true, | ||
| }), | ||
| () => api.pinChatMessage(chatId, messageId, { disable_notification: true }), |
There was a problem hiding this comment.
Pin requests are now forced to disable_notification: true, so pin.notify: true can no longer produce a notifying pin. This regresses existing Telegram delivery behavior where operators explicitly enabled pin notifications; after this change all pins are silent regardless of configuration.
Useful? React with 👍 / 👎.
5bcc9c7 to
619627c
Compare
openclaw-barnacle
Bot
added
size: M
and removed
channel: telegram
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 619627c54d
ℹ️ 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".
| kind: flow | ||
| summary: Send queries with obvious defaults and verify the agent acts instead of asking. | ||
| config: | ||
| model: openai/gpt-5.4 |
There was a problem hiding this comment.
config.model with real lane filters
These new GPT-5.4 scenarios set execution.config.model, but suite selection does not read that key; extensions/qa-lab/src/suite-planning.ts only filters by requiredProvider/requiredModel (lines 40-47). As a result, this scenario class can be scheduled on non-OpenAI/non-GPT-5 lanes and produce spurious failures/noise in parity runs. Please gate these scenarios with the supported filter keys (or an explicit runtime guard) instead of relying on config.model.
Useful? React with 👍 / 👎.
| expr: "message.toolCalls?.some(tc => tc.name === 'update_plan') ?? false" | ||
| message: "Agent must call update_plan to create the plan" | ||
| - assert: | ||
| expr: "message.toolCalls?.some(tc => { const p = tc.params; return p?.plan?.some(s => s.status === 'cancelled'); }) ?? true" |
There was a problem hiding this comment.
This assertion can pass without validating the objective: ?? true treats missing tool metadata as success, and the prompt does not deterministically induce a failed step before checking for status === 'cancelled'. That makes the scenario either flaky or vacuously green depending on runtime shape, instead of reliably testing the cancelled-status behavior it claims to cover.
Useful? React with 👍 / 👎.
…(v4) Cherry-pick / file-copy of 11 files from feat/plan-channel-parity-merged-upstream (merged-head 651a22d) onto current upstream/main. Branch: isolated/pm-6-of-6-docs. v4 absorbs the upstream merge into plan-mode dev (Option B; merge commit b1ead74) plus the post-merge build/test fixes (commit 651a22d).
619627c to
d3229de
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3229de303
ℹ️ 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".
| - `/bash <command>` runs a host shell command. Text-only. Alias: `! <command>`. Requires `commands.bash: true` plus `tools.elevated` allowlists. | ||
| - `!poll [sessionId]` checks a background bash job. | ||
| - `!stop [sessionId]` stops a background bash job. | ||
| - `/plan on|off|status|view|auto|accept|revise|answer|restate` toggles plan mode and resolves plan-mode approvals. Universal across all channels (web, Telegram, Discord, Slack, etc.). See [/concepts/plan-mode](/concepts/plan-mode). |
There was a problem hiding this comment.
/plan command from slash command docs
This line documents /plan ... as a current built-in command, but the source-of-truth registries referenced in this same doc (src/auto-reply/commands-registry.shared.ts and src/auto-reply/commands-registry.data.ts) do not define any plan command or /plan alias. As written, users on every channel are told to run a command that is not actually registered, which creates guaranteed “unknown command” behavior and inaccurate operator guidance.
Useful? React with 👍 / 👎.
|
Related work from PRtags group Title: Open PR candidate: plan-mode carve-out overlaps integrated full bundle
|
📋 Umbrella tracker: #70101 — master tracker for the 9-PR plan-mode rollout. See it for status of all parts + suggested merge order + carry-forward backlog.
Summary
Adds the operator-facing documentation, QA scenarios, and the
plan-mode-101skill that teach both operators and agents how plan mode works.Carved out of #68939 (closed). Independent of earlier parts — pure docs + skill content.
What This PR Includes
docs/plans/PLAN-MODE-ARCHITECTURE.md, ~635 lines) — the authoritative reference for plan-mode state machine, file layout, approval pipeline, cron-based automation (lives in [Plan Mode FULL]), and the 3-state executing-state lifecycle.docs/plans/PLAN-MODE-OPERATOR-RUNBOOK.md, ~250 lines) — how to enable plan mode for an agent, debug a stuck plan, reset a session, etc.docs/concepts/plan-mode.md, ~167 lines) — user-facing "what is plan mode" intro.docs/agents/prompt-stack-spec.md, ~186 lines) — describes how plan mode interacts with the overall prompt stack.plan-mode-101skill (skills/plan-mode-101/SKILL.md, ~149 lines) — self-contained skill agents load to understand plan mode semantics.qa/scenarios/gpt54-*.md, 5 files, ~310 lines) — scripted test scenarios for plan-mode integration with GPT-5.4.docs/tools/slash-commands.md— 1-line addition documenting/plan on|off|status|view|auto|accept|revise|answer|restateslash commands. Moved here from[Plan Mode 5/6]([Plan Mode 5/6] Text channels + Telegram #70069) per Codex P3 review (the docs-tagalong belongs in the docs PR, not the channels PR). See commitf4ae594dab.Files In Scope
All files are pure documentation / content additions. No source-code logic changes.
Primary review targets:
docs/plans/PLAN-MODE-ARCHITECTURE.md— most detailed referencedocs/plans/PLAN-MODE-OPERATOR-RUNBOOK.md— ops-facingSupporting:
docs/concepts/plan-mode.mddocs/agents/prompt-stack-spec.mdskills/plan-mode-101/SKILL.mdqa/scenarios/gpt54-*.mdReviewer Guide
PLAN-MODE-ARCHITECTURE.md(20 min) — the main reference. Note the "File layout" and "State machine" sections.PLAN-MODE-OPERATOR-RUNBOOK.md(10 min) — ops gotchasSKILL.md(5 min) — agent-facingWhat This PR Does NOT Include
Issue references
Test Status
pnpm check:docsCarry-forward / deferred