feat(agents): plan mode runtime + escalating retry + auto-continue [Phase 3.C]#67538
feat(agents): plan mode runtime + escalating retry + auto-continue [Phase 3.C]#67538100yenadmin wants to merge 20 commits into
Conversation
…ne, types Phase 3.C of the GPT 5.4 parity sprint. Implements plan mode as an opt-in feature where mutation tools are blocked during Phase A (plan) until the user approves the plan, then Phase B (execute) runs normally with full tool access. ## Plan mode mutation gate (mutation-gate.ts) - Blocks 11 mutation tools by exact name: apply_patch, edit, exec, gateway, message, nodes, process, sessions_send, sessions_spawn, subagents, write - Blocks suffix patterns: *.write, *.edit, *.delete - Allows: read, web_search, web_fetch, memory_search, memory_get, update_plan, exit_plan_mode, session_status - Special exec whitelist: read-only commands (ls, cat, git status, grep, find, etc.) are allowed; mutating commands are blocked - 30 unit tests covering all paths ## Approval state machine (approval.ts) - States: none → pending → approved | edited | rejected | timed_out - approve: transitions to normal mode, records confirmedAt - edit: re-enters plan mode with edited plan - reject/timeout: transitions to normal mode, aborts turn - buildApprovedPlanInjection(): generates system message for Phase B - 7 unit tests ## Types (types.ts) - PlanMode, PlanApprovalState, PlanModeSessionState - DEFAULT_PLAN_MODE_STATE Built independently of PR #61845. Plan mode is NEVER auto-enabled — it requires explicit user opt-in via config, slash command, or UI toggle (entry points follow in a separate commit). Tracking: #66345, #67520.
Greptile SummaryThis PR implements three complementary features for GPT-5.4 agentic parity: escalating retry instructions (3 urgency levels), opt-in auto-continue cycling (ACK injection after retry exhaustion), and plan-mode primitives (mutation gate, approval state machine, crypto-secure approval IDs). Previous critical findings from earlier review rounds—shell compound-operator bypass in the exec whitelist and stale Two P2 gaps remain:
Confidence Score: 4/5Safe to merge with awareness of two P2 gaps in Both prior P1 issues (exec whitelist shell bypass, stale approvalId fail-open) are resolved. The src/agents/pi-embedded-runner/run.ts — Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run.ts
Line: 1816-1831
Comment:
**`stopOnMutation` is effectively a no-op**
`autoContinueAccumulatedMutation` can never be set to `true` in the current implementation because the conditions for entering this block are mutually exclusive with having produced side effects.
The outer guard requires `nextPlanningOnlyRetryInstruction` to be non-null, but `resolvePlanningOnlyRetryInstruction` returns `null` whenever `attempt.replayMetadata.hadPotentialSideEffects` is `true` (see `incomplete-turn.ts` line 594). So by the time execution reaches line 1820, `hadPotentialSideEffects` is always `false` for the current attempt, and the flag is never flipped.
For mutations from earlier loop iterations: a previous attempt with mutations would have had `nextPlanningOnlyRetryInstruction == null`, so it bypassed this block entirely and `autoContinueAccumulatedMutation` was never updated.
The result is that `stopOnMutation: true` (the default) provides no actual guard. The config option is accepted and stored but has no runtime effect.
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/pi-embedded-runner/run.ts
Line: 1708-1730
Comment:
**`planModeActive` flag not forwarded to retry resolvers**
`incomplete-turn.ts` added a `planModeActive?: boolean` parameter to `resolvePlanningOnlyRetryInstruction`, `resolveReasoningOnlyRetryInstruction`, `resolveEmptyResponseRetryInstruction`, and `resolveAckExecutionFastPathInstruction`. The parameter comment explicitly states that when plan mode is active, planning-only IS the desired state and retry pressure should be suppressed.
None of the four call sites here pass `planModeActive`. When plan mode is eventually wired into `run.ts` (the gate, state machine, and session tracking from `src/agents/plan-mode/` are not connected yet), forgetting to update these calls means the escalating retry instructions will fire against the agent even during plan phases where it's supposed to produce a plan and call `exit_plan_mode`. The missing link is easy to overlook in a follow-up PR.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix(agents): #67538 — per-field merge fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 4f123cdd10
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Adds a standalone plan-mode runtime module under src/agents/plan-mode/ to support a two-phase workflow (planning vs execution) by (a) gating mutation-capable tools during planning and (b) tracking user approval outcomes.
Changes:
- Introduces plan-mode session/approval types and defaults.
- Adds a mutation-gate helper that blocks selected tools (with an
execread-only prefix whitelist) while in plan mode. - Adds an approval state transition helper plus approved-plan system-message injection, with Vitest coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/agents/plan-mode/types.ts | New plan-mode state/approval type definitions and defaults |
| src/agents/plan-mode/mutation-gate.ts | Mutation-gating logic for plan mode (blocklist, suffix patterns, exec whitelist) |
| src/agents/plan-mode/mutation-gate.test.ts | Unit tests for mutation gate behavior |
| src/agents/plan-mode/approval.ts | Approval state machine transitions + approved-plan injection builder |
| src/agents/plan-mode/approval.test.ts | Unit tests for approval transitions and injection formatting |
| src/agents/plan-mode/index.ts | Public exports for plan-mode module |
…efault deny, confirmedAt - Block shell compound operators (&&, ||, |, ;, >, >>, backticks, $()) in exec/bash commands to prevent chaining past the prefix allowlist - Add "bash" to the mutation tool blocklist (alias for exec) - Default to blocked for unknown tools (fail-closed, not fail-open) - Clear confirmedAt on "edit" transition to prevent stale approval leaking - Remove unused PlanApprovalState import
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a44b85cda
ℹ️ 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".
…itelist Two Codex P1 issues at 95%+ confidence: 1. Newlines as shell separators: "ls\nrm -rf tmp" bypassed the compound operator check because \n wasn't in the regex. Added \n and \r to the operator detection pattern. 2. Dangerous flags on allowed prefixes: "find . -delete" passed because "find" is whitelisted. Added a dangerous-flag check for -delete, -exec, -execdir, --delete, -rf, -i that runs after the operator check but before the prefix allowlist. Added 4 new test cases covering both vectors + bash alias.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9ee0671b9
ℹ️ 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".
`env` as a prefix allows arbitrary command execution (e.g. `env npm install`). Remove it — users who need environment variables can use `printenv` which is already allowlisted.
- Convert args from mappings to arrays - Replace sendAgentMessage with state.addInboundMessage - Replace waitForToolCall with waitForOutboundMessage + lambda predicates - Replace assertions: blocks with assert: actions - Add waitForGatewayHealthy + waitForQaChannelReady setup - Use ref: state / ref: env for runtime objects - Fix gpt54-plan-mode-default-off scenario ID to match filename - Simplify injection-scan to baseline check (scanner unit-tested in #67512)
- Add explicit tests for bash tool blocked in plan mode, shell compound operators (redirect, pipe, semicolon), newlines in commands, and dangerous flags (find -delete, find -exec) - Add early-return guard in resolvePlanApproval for stale timeouts that fire after the user has already acted on the approval - Add test verifying stale timeout is ignored after approval is resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e5c22981b
ℹ️ 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".
…ear confirmedAt Three fixes at 95%+ confidence from Copilot review: 1. Remove -i from DANGEROUS_FLAGS — blocks grep -i (case-insensitive search), the most common grep usage in plan mode. The remaining flags (-delete, -exec, -execdir, --delete, -rf) are genuinely dangerous. 2. Add read-only suffix patterns (.read, .search, .list, .get, .view) so MCP read tools like custom_mcp.read pass the fail-closed default. Mutation suffixes (.write, .edit, .delete) are still blocked. 3. Clear confirmedAt on reject and timeout transitions in the approval state machine. Previously only edit cleared it; reject/timeout spread current which could leak a stale confirmedAt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 285dc1e9a3
ℹ️ 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".
Plan-mode rollout series — status updateThis PR is part of a 10-PR plan-mode rollout. The cumulative state ships locally as Series overview + landing order: see #68441 (latest comment). This PR's role in the series: see the original PR description above. Live test status: covered by the cumulative install — all behaviors from this PR are verified in production-like usage on Telegram + webchat. No regressions detected since the live install (2026-04-18). |
📋 Architecture & Status — single source of truth (commit
|
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/agents/pi-embedded-runner/run/incomplete-turn.ts:360
shouldApplyPlanningOnlyRetryGuard()is now used to gate reasoning-only and empty-response retries, and it returnsfalsewhenplanModeActiveis true. That would disable recovery for reasoning-only/empty-response failures during plan mode (even though those failure modes aren’t “planning-only” and still need remediation to produce a visible plan). Suggest splitting this into (a) a provider/model guard for retry features and (b) a separate “suppress act-now pressure in plan mode” check that applies only to the planning-only retry + ACK fast-path logic.
if (
!shouldApplyPlanningOnlyRetryGuard({
provider: params.provider,
modelId: params.modelId,
planModeActive: params.planModeActive,
})
) {
return null;
}
const assistant = params.attempt.currentAttemptAssistant ?? params.attempt.lastAssistant;
if (params.attempt.assistantTexts.join("\n\n").trim().length > 0) {
return null;
}
if (assistant?.stopReason === "error") {
return null;
}
if (!isReasoningOnlyAssistantTurn(assistant)) {
return null;
}
return REASONING_ONLY_RETRY_INSTRUCTION;
}
export function resolveEmptyResponseRetryInstruction(params: {
provider?: string;
modelId?: string;
payloadCount: number;
aborted: boolean;
timedOut: boolean;
attempt: IncompleteTurnAttempt;
/** When true, planning-only is the desired state — skip retry pressure. */
planModeActive?: boolean;
}): string | null {
if (
params.aborted ||
params.timedOut ||
params.attempt.clientToolCall ||
params.attempt.yieldDetected ||
params.attempt.didSendDeterministicApprovalPrompt ||
params.attempt.lastToolError ||
params.attempt.replayMetadata.hadPotentialSideEffects
) {
return null;
}
if (
!shouldApplyPlanningOnlyRetryGuard({
provider: params.provider,
modelId: params.modelId,
planModeActive: params.planModeActive,
})
) {
return null;
}
| /** | ||
| * Auto-continuation for planning-only turns. When enabled, the runner | ||
| * automatically injects an ACK fast-path instruction instead of surfacing | ||
| * the plan to the user, up to `maxCycles` consecutive auto-continue cycles. | ||
| * Each cycle = 1 ACK injection + up to 3 planning retries = ~4 API calls. | ||
| */ | ||
| autoContinue?: { | ||
| /** Enable auto-continuation. Default: false. */ | ||
| enabled?: boolean; | ||
| /** | ||
| * Max auto-continue cycles before pausing for user review. Default: 3. | ||
| * Total worst-case API calls = 1 + (maxCycles x 4). Default 3 = ~13 calls max. | ||
| */ | ||
| maxCycles?: number; | ||
| /** Pause when any attempt in the run produces mutating tool calls. Default: true. */ | ||
| stopOnMutation?: boolean; |
There was a problem hiding this comment.
The comment’s worst-case call formula (1 + (maxCycles x 4)) is inconsistent with the runner’s behavior: strict-agentic can already do the initial attempt + 3 retries before any auto-continue cycles, and each cycle can add 1 auto-continue attempt + up to 3 retries. Update the formula (and/or the meaning of maxCycles) to match the real budgeting so operators don’t underestimate API usage.
| export const PLANNING_ONLY_RETRY_INSTRUCTION_FIRM = | ||
| "CRITICAL: You have described the plan multiple times without acting. You MUST call a tool in this turn. No more planning or narration. If a real blocker prevents action, state the exact blocker in one sentence. Otherwise, call the first tool NOW."; | ||
| export const PLANNING_ONLY_RETRY_INSTRUCTION_FINAL = | ||
| "Final reminder: this is the third planning-only turn. Please call a tool now to make progress. If a real blocker prevents action, state the exact blocker in one sentence so the user can unblock you."; |
There was a problem hiding this comment.
PLANNING_ONLY_RETRY_INSTRUCTION_FINAL says “this is the third planning-only turn”, but it’s selected on the 3rd retry (attemptIndex >= 2), which typically means the model is about to produce a 4th planning-only turn including the original. Consider adjusting the wording to match what’s actually happening (e.g. “third retry” / “final retry”) so the instruction is accurate.
| "Final reminder: this is the third planning-only turn. Please call a tool now to make progress. If a real blocker prevents action, state the exact blocker in one sentence so the user can unblock you."; | |
| "Final reminder: this is the final planning-only retry. Please call a tool now to make progress. If a real blocker prevents action, state the exact blocker in one sentence so the user can unblock you."; |
|
Closing in favor of consolidated PR #68939. Rationale: this branch was 734 commits behind upstream/main and the The full iteration history + decision log lives in |
TL;DR
Three complementary features to fix GPT-5.4's planning-only behavior: (1) opt-in plan mode that gates mutating tools until user approval, (2) escalating retry instructions with 3 urgency levels, and (3) opt-in auto-continue that keeps the agent working without human intervention. Together these close the gap between GPT-5.4's cautious defaults and agentic execution.
Tracking
What this PR does
Plan mode runtime (opt-in)
Two-phase execution: plan (mutation gate ON, read-only tools allowed) → execute (full tool access after approval).
none → pending → approved | edited | rejected | timed_outEscalating retry (automatic, GPT-5 only)
When planning-only turn detected (text with zero tool calls), up to 3 retries with escalating urgency:
Retry limit raised from 2 → 3 for strict-agentic contract.
Auto-continue (opt-in config)
After retries exhausted, instead of blocking for user input, injects ACK fast-path automatically:
{ "agents": { "defaults": { "embeddedPi": { "autoContinue": { "enabled": true, // default: false "maxCycles": 3, // each cycle ≈ 4 API calls "stopOnMutation": true } } } } }Flow diagrams
Escalating retry + auto-continue
Plan mode
API call math
Files changed
src/agents/plan-mode/*.ts(6 files)qa/scenarios/gpt54-*.md(5 files)src/agents/pi-embedded-runner/run.tssrc/agents/pi-embedded-runner/run/incomplete-turn.tssrc/agents/pi-embedded-runner/run.incomplete-turn.test.tssrc/agents/agent-scope.tsresolveAgentAutoContinue()config resolversrc/config/types.agent-defaults.tsautoContinuetypesrc/config/zod-schema.agent-defaults.tsautoContinueZod schemaTests
Rollback
autoContinue.enabled: false(default)All features are additive. No existing behavior changes unless opted in.
Dependencies
What follows
/planslash command