Remove plan mode from daemon#140
Merged
glittercowboy merged 1 commit intomainfrom May 2, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
💤 Files with no reviewable changes (13)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR removes plan-capability support and plan-related tooling/tests/UI, shifts warm-worker identity to browser-grant fields, removes plan-evidence/outbox logic, and threads a Pi provider string through actor/control/executor wiring. go.mod updates ChangesPlan-capability removal & provider/browser-grant migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md (3)
19-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDoc is out-of-sync with “remove plan mode” objective (plan capability references).
The plan currently says
internal/pi/worker_key_test.goshould verify “plan capability identity” and theWorkerKeystruct includesPlanAPIBaseURL,PlanTokenHash, andPlanExpiresAt. This conflicts with the PR goal of removing plan mode wiring/capabilities. Please remove/replace those plan-capability references (tests + key fields + any “Plan*” behavior) so the implementation plan matches the new protocol/runtime surface.Also applies to: 249-270, 298-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md` around lines 19 - 23, The docs and tests still reference plan-mode fields and behavior that should be removed; update the WorkerKey-related code and tests to drop plan capability wiring: remove the PlanAPIBaseURL, PlanTokenHash, and PlanExpiresAt fields from the WorkerKey struct (and any usage in NewWorkerKey/WorkerSnapshot), and update internal/pi/worker_key_test.go to stop asserting "plan capability identity" — replace those assertions with checks relevant to the new protocol surface (e.g., default provider behavior or other existing identity fields), and update any doc entries in the plan file that list "plan capability" to match the new fields/behavior.
298-343:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmbedded
WorkerKeyincludes Plan fields butNewWorkerKeydoesn’t populate them (internal inconsistency).In the snippet,
WorkerKeydefinesPlanAPIBaseURL/PlanTokenHash/PlanExpiresAt, butNewWorkerKey(opts Options)sets only the non-plan fields shown—leaving those plan fields at their zero values. If you keep plan-related identity (which likely shouldn’t, given the PR objective),NewWorkerKeymust populate them fromOptions; otherwise, remove the fields and any associated test expectations so the hash/key identity logic is consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md` around lines 298 - 343, WorkerKey declares PlanAPIBaseURL, PlanTokenHash and PlanExpiresAt but NewWorkerKey(opts Options) does not set them, creating an internal inconsistency; either populate those fields from the provided Options (e.g. set WorkerKey.PlanAPIBaseURL = opts.PlanAPIBaseURL, PlanTokenHash = opts.PlanTokenHash, PlanExpiresAt = opts.PlanExpiresAt) so key identity includes plan data, or remove the three Plan* fields from the WorkerKey type (and update any tests that expect them) so NewWorkerKey and the key/hash logic (e.g. anywhere KeyHash is computed) remain consistent.
1748-1813:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
WarmClaudeSdkWorker.stop()can leave an in-flightturn()Promise unresolved.In the embedded TS implementation,
stop()just callsthis.prompt.close(); ifthis.activeis set (aturn()is currently waiting for aresult), the pump loop may end without callingresolveorreject, leaving the pending Promise hanging. Ifstop()can run during warm-worker reset/teardown, this can break callers.Suggested fix in the plan snippet: in
stop(), ifthis.activeexists, reject it (or resolve with a well-defined cancellation result) and clearthis.active/this.pumpError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md` around lines 1748 - 1813, stop() currently only calls this.prompt.close() and can leave a pending turn Promise unresolved; update WarmClaudeSdkWorker.stop() so that after calling this.prompt.close() it checks if this.active exists, creates a cancellation Error (e.g. new Error("WarmClaudeSdkWorker stopped")), assigns it to this.pumpError, calls this.active.reject(err), and then clears this.active (and any transient state) so the in-flight Promise is deterministically rejected and no dangling resolver remains.internal/pi/worker_key_test.go (1)
29-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
BrowserSessionIDin the negative case too.Right now this still passes if
NewWorkerKeyonly keys onBrowserSessionID, so it no longer proves thatBrowserGrantIDandBrowserIDparticipate in the key.Suggested fix
withoutBrowser := base withoutBrowser.BrowserGrantID = "" withoutBrowser.BrowserID = "" + withoutBrowser.BrowserSessionID = "" if a == NewWorkerKey(withoutBrowser) { t.Fatal("worker key ignored browser grant") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pi/worker_key_test.go` around lines 29 - 49, The test TestWorkerKeyIncludesBrowserGrant currently leaves BrowserSessionID set and thus could pass if NewWorkerKey only uses BrowserSessionID; update the negative case by also clearing withoutBrowser.BrowserSessionID = "" so that NewWorkerKey(base) != NewWorkerKey(withoutBrowser) only if BrowserGrantID and BrowserID are considered; locate the failing test and modify the withoutBrowser struct assignments (in TestWorkerKeyIncludesBrowserGrant) to set BrowserGrantID, BrowserID, and BrowserSessionID to empty strings before comparing.internal/pi/extension/index.ts (1)
60-71:⚠️ Potential issue | 🟠 MajorAdd plan-mode built-ins to the disallowed list.
With
permissionMode: "bypassPermissions", the Agent SDK does not enforceallowedToolsas a restriction; onlydisallowedToolscan block specific built-ins. EnterPlanMode and ExitPlanMode are documented built-in tools in the Claude Agent SDK, and dropping them fromCLAUDE_BUILTINSre-enables the exact plan-mode capabilities this PR aims to remove.Suggested fix
const CLAUDE_BUILTINS = [ "Bash", "BashOutput", "KillShell", "Read", "Write", "Edit", "MultiEdit", "NotebookEdit", "Glob", "Grep", "WebFetch", "WebSearch", - "Task", "Agent", "TodoWrite", + "Task", "Agent", "TodoWrite", "EnterPlanMode", "ExitPlanMode", "ListMcpResourcesTool", "ReadMcpResourceTool", "AskUserQuestion", "PushNotification", "ScheduleWakeup", "Monitor", "Skill", "ToolSearch", "TeamCreate", "TeamDelete", "SendMessage",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pi/extension/index.ts` around lines 60 - 71, The CLAUDE_BUILTINS array is missing the documented plan-mode tools so they remain allowed; update the CLAUDE_BUILTINS constant in internal/pi/extension/index.ts to include "EnterPlanMode" and "ExitPlanMode" so those plan-mode built-ins are treated as disallowed by your permission logic (look for the CLAUDE_BUILTINS array definition and add the two symbols to that list).
🧹 Nitpick comments (1)
internal/pi/extension/schema-to-zod.test.mjs (1)
72-72: ⚡ Quick winAdd a direct assertion for the new
create_taskdiscriminator branch.Right now the test only asserts the
start_next_itempath; a typo in the renamed discriminator could slip through without failing this test.Proposed test addition
const operationResult = operationSchema.safeParse({ type: "start_next_item", leaseTtlSeconds: 1800, }); assert.equal(operationResult.success, false); @@ ["leaseTtlSeconds"], ); + +const createTaskResult = operationSchema.safeParse({ + type: "create_task", + title: "Wire task flow", + items: [{}], +}); +assert.equal(createTaskResult.success, true); + +assert.equal(operationSchema.safeParse({ + type: "create_plan", + title: "legacy value", + items: [{}], +}).success, false); console.log("schema-to-zod tests passed");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pi/extension/schema-to-zod.test.mjs` at line 72, Add an explicit assertion that the schema-to-zod conversion produces the expected branch for the new discriminator value "create_task": locate the test in schema-to-zod.test.mjs that currently asserts the "start_next_item" path and add a direct expect/assert for the discriminator branch where type: { const: "create_task" } (verify the generated Zod union/branch or matcher for "create_task" exists and matches the expected shape). Ensure the assertion references the same helper/subject used in the test (e.g., the conversion function or generated Zod schema variable) so a typo in "create_task" will cause the test to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/pi/control.go`:
- Line 139: Replace the unconditional inheritance of the parent environment in
control mode (the cmd.Env = os.Environ() assignment) with the same
allowlist-based environment builder used by internal/pi/executor.go; thread any
required provider creds explicitly through ControlOptions (add fields if needed)
and use those to populate the builder so control-mode subprocesses only receive
the sanitized, allowlisted variables rather than the full parent env. Ensure you
locate and reuse the executor’s env-builder function (or extract it to a shared
helper) and replace references to os.Environ() in control.go with a call that
builds cmd.Env from ControlOptions via that allowlist.
---
Outside diff comments:
In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md`:
- Around line 19-23: The docs and tests still reference plan-mode fields and
behavior that should be removed; update the WorkerKey-related code and tests to
drop plan capability wiring: remove the PlanAPIBaseURL, PlanTokenHash, and
PlanExpiresAt fields from the WorkerKey struct (and any usage in
NewWorkerKey/WorkerSnapshot), and update internal/pi/worker_key_test.go to stop
asserting "plan capability identity" — replace those assertions with checks
relevant to the new protocol surface (e.g., default provider behavior or other
existing identity fields), and update any doc entries in the plan file that list
"plan capability" to match the new fields/behavior.
- Around line 298-343: WorkerKey declares PlanAPIBaseURL, PlanTokenHash and
PlanExpiresAt but NewWorkerKey(opts Options) does not set them, creating an
internal inconsistency; either populate those fields from the provided Options
(e.g. set WorkerKey.PlanAPIBaseURL = opts.PlanAPIBaseURL, PlanTokenHash =
opts.PlanTokenHash, PlanExpiresAt = opts.PlanExpiresAt) so key identity includes
plan data, or remove the three Plan* fields from the WorkerKey type (and update
any tests that expect them) so NewWorkerKey and the key/hash logic (e.g.
anywhere KeyHash is computed) remain consistent.
- Around line 1748-1813: stop() currently only calls this.prompt.close() and can
leave a pending turn Promise unresolved; update WarmClaudeSdkWorker.stop() so
that after calling this.prompt.close() it checks if this.active exists, creates
a cancellation Error (e.g. new Error("WarmClaudeSdkWorker stopped")), assigns it
to this.pumpError, calls this.active.reject(err), and then clears this.active
(and any transient state) so the in-flight Promise is deterministically rejected
and no dangling resolver remains.
In `@internal/pi/extension/index.ts`:
- Around line 60-71: The CLAUDE_BUILTINS array is missing the documented
plan-mode tools so they remain allowed; update the CLAUDE_BUILTINS constant in
internal/pi/extension/index.ts to include "EnterPlanMode" and "ExitPlanMode" so
those plan-mode built-ins are treated as disallowed by your permission logic
(look for the CLAUDE_BUILTINS array definition and add the two symbols to that
list).
In `@internal/pi/worker_key_test.go`:
- Around line 29-49: The test TestWorkerKeyIncludesBrowserGrant currently leaves
BrowserSessionID set and thus could pass if NewWorkerKey only uses
BrowserSessionID; update the negative case by also clearing
withoutBrowser.BrowserSessionID = "" so that NewWorkerKey(base) !=
NewWorkerKey(withoutBrowser) only if BrowserGrantID and BrowserID are
considered; locate the failing test and modify the withoutBrowser struct
assignments (in TestWorkerKeyIncludesBrowserGrant) to set BrowserGrantID,
BrowserID, and BrowserSessionID to empty strings before comparing.
---
Nitpick comments:
In `@internal/pi/extension/schema-to-zod.test.mjs`:
- Line 72: Add an explicit assertion that the schema-to-zod conversion produces
the expected branch for the new discriminator value "create_task": locate the
test in schema-to-zod.test.mjs that currently asserts the "start_next_item" path
and add a direct expect/assert for the discriminator branch where type: { const:
"create_task" } (verify the generated Zod union/branch or matcher for
"create_task" exists and matches the expected shape). Ensure the assertion
references the same helper/subject used in the test (e.g., the conversion
function or generated Zod schema variable) so a typo in "create_task" will cause
the test to fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9d0c421-8322-47ee-b3e8-30d0bfa8daaa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
cmd/pi_tui.godocs/superpowers/plans/2026-04-29-warm-pi-provider-processes.mdgo.modinternal/lab/scenarios.gointernal/lab/server.gointernal/lab/static/index.htmlinternal/pi/control.gointernal/pi/control_test.gointernal/pi/executor.gointernal/pi/executor_test.gointernal/pi/extension/claude-prompt.test.mjsinternal/pi/extension/index.tsinternal/pi/extension/plan-tools.jsinternal/pi/extension/plan-tools.test.mjsinternal/pi/extension/schema-to-zod.test.mjsinternal/pi/extension/tool-policy.jsinternal/pi/extension_package_test.gointernal/pi/worker_key.gointernal/pi/worker_key_test.gointernal/session/actor.gointernal/session/actor_test.gointernal/session/plan_evidence.gointernal/session/plan_evidence_outbox.gointernal/session/plan_evidence_test.go
💤 Files with no reviewable changes (14)
- internal/pi/extension_package_test.go
- internal/lab/server.go
- internal/pi/extension/tool-policy.js
- cmd/pi_tui.go
- internal/lab/static/index.html
- internal/lab/scenarios.go
- internal/session/plan_evidence.go
- internal/pi/extension/plan-tools.test.mjs
- internal/pi/extension/plan-tools.js
- internal/pi/executor_test.go
- internal/session/plan_evidence_outbox.go
- internal/session/plan_evidence_test.go
- internal/pi/control_test.go
- internal/pi/worker_key.go
glittercowboy
force-pushed
the
codex/remove-plan-mode
branch
from
8d7b64d to
9f99077
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/session/actor.go (1)
221-227:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSeed the control provider before the first task runs.
currentPiProvider()falls back toclaude-cliuntilrunPiExecutor()has calledsetPiProvider(). That means a compact/context-stats request on a resumed or freshly started session will build the control subprocess env for the wrong provider and drop non-Anthropic credentials beforeinternal/pi/control.golaunchespi. For sessions usingopenrouter/zai/kimi-coding, control commands can fail after a daemon restart or before the first task executes.Please thread a default provider into the actor at construction time, or restore it from persisted session state, so
runPiControlhas the right provider even when no task has executed yet.Also applies to: 1310-1317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/actor.go` around lines 221 - 227, The actor is seeding the control provider too late (currentPiProvider() defaults to claude-cli until setPiProvider() runs), causing control subprocesses to be built with wrong credentials; fix by initializing the actor's provider at construction or restoring it from persisted session state so runPiControl/runPiExecutor use the correct provider before any task runs—specifically, populate the actor's provider field used by currentPiProvider() during NewActor/constructor or session restore paths (or load it from the persisted session record) so pi.RunControl called in runPiControl (the call site invoking pi.RunControl with BinaryPath/CWD/SessionFile/Model/Provider/Command) gets the right Provider even on resumed sessions or before setPiProvider() is ever invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/session/actor.go`:
- Around line 221-227: The actor is seeding the control provider too late
(currentPiProvider() defaults to claude-cli until setPiProvider() runs), causing
control subprocesses to be built with wrong credentials; fix by initializing the
actor's provider at construction or restoring it from persisted session state so
runPiControl/runPiExecutor use the correct provider before any task
runs—specifically, populate the actor's provider field used by
currentPiProvider() during NewActor/constructor or session restore paths (or
load it from the persisted session record) so pi.RunControl called in
runPiControl (the call site invoking pi.RunControl with
BinaryPath/CWD/SessionFile/Model/Provider/Command) gets the right Provider even
on resumed sessions or before setPiProvider() is ever invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 725f5a1b-66a2-4668-80bd-7def5c511ba4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
cmd/pi_tui.godocs/superpowers/plans/2026-04-29-warm-pi-provider-processes.mdgo.modinternal/lab/scenarios.gointernal/lab/server.gointernal/lab/static/index.htmlinternal/pi/control.gointernal/pi/control_test.gointernal/pi/executor.gointernal/pi/executor_test.gointernal/pi/extension/claude-prompt.test.mjsinternal/pi/extension/index.tsinternal/pi/extension/plan-tools.jsinternal/pi/extension/plan-tools.test.mjsinternal/pi/extension/schema-to-zod.test.mjsinternal/pi/extension/tool-policy.jsinternal/pi/extension_package_test.gointernal/pi/worker_key.gointernal/pi/worker_key_test.gointernal/session/actor.gointernal/session/actor_test.gointernal/session/plan_evidence.gointernal/session/plan_evidence_outbox.gointernal/session/plan_evidence_test.go
💤 Files with no reviewable changes (13)
- cmd/pi_tui.go
- internal/pi/extension/plan-tools.js
- internal/lab/server.go
- internal/pi/extension_package_test.go
- internal/pi/extension/tool-policy.js
- internal/session/plan_evidence.go
- internal/pi/worker_key.go
- internal/pi/extension/plan-tools.test.mjs
- internal/pi/executor_test.go
- internal/lab/scenarios.go
- internal/lab/static/index.html
- internal/session/plan_evidence_test.go
- internal/session/plan_evidence_outbox.go
✅ Files skipped from review due to trivial changes (4)
- go.mod
- internal/pi/extension/claude-prompt.test.mjs
- internal/pi/executor.go
- docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/pi/extension/schema-to-zod.test.mjs
- internal/pi/worker_key_test.go
- internal/session/actor_test.go
glittercowboy
force-pushed
the
codex/remove-plan-mode
branch
from
9f99077 to
67013fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/session/actor.go (1)
903-910:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove provider seeding earlier than
runPiExecutor().
piProvideris only updated after the task leaves the queue.runPiControl()reads that same session-scoped field forContextStatsRequest/CompactRequest, so a control request that lands between task acceptance and this point can still run with the previous provider's env/credentials.Suggested fix
+func (a *Actor) SetProvider(provider string) { + a.taskMu.Lock() + a.piProvider = pi.ProviderOrDefault(provider) + a.taskMu.Unlock() +}And update the task-acceptance path in
internal/loop/daemon.gobefore the actor becomes observable to concurrent control requests:if actor == nil { actor, err = d.manager.Spawn(ctx, session.Options{ SessionID: msg.SessionID, CWD: msg.CWD, Model: msg.Model, Provider: msg.Provider, // ... }) // ... } else { + actor.SetProvider(msg.Provider) actor.SetBrowserContext(browserGrantID, browserID) actor.SetBrowserGrant(browserGrant, session.BrowserRuntimeSnapshot{ // ... }, d.browserRPCSocket) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/actor.go` around lines 903 - 910, The provider must be seeded before any control or executor runs: move the call to a.setPiProvider(pi.ProviderOrDefault(tc.Provider)) to occur immediately after determining/normalizing the model (i.e., right after model = normalizePiModel(model) and a.setPiModel(model)), so that session-scoped piProvider is updated prior to runPiExecutor() and runPiControl() reading it; also ensure the task-acceptance path in internal/loop/daemon.go updates the actor's provider (via setPiProvider) before the actor is published/made observable to concurrent control requests so control requests cannot observe the previous provider/credentials.
🧹 Nitpick comments (2)
docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md (2)
1782-1791: ⚡ Quick winAlign
WarmClaudeSdkWorker.stop()ordering with the stated “reject before closing” intent.The doc snippet closes the prompt queue first (
this.prompt.close()), then rejects the active turn and setspumpError. If the goal is to reliably reject an in-progress turn before the prompt pump is closed/stopped, the ordering should be swapped: setpumpError, reject the active turn, then close the prompt queue.🛠️ Suggested doc change
async stop() { - this.prompt.close(); if (this.active) { const err = new Error("WarmClaudeSdkWorker stopped"); this.pumpError = err; const active = this.active; this.active = null; active.reject(err); } + this.prompt.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md` around lines 1782 - 1791, The stop() method in WarmClaudeSdkWorker should reject the in-progress turn before closing the prompt pump; change the ordering in WarmClaudeSdkWorker.stop(): if this.active is set, create and assign the Error to this.pumpError, capture the active reference, set this.active = null, call active.reject(err), and only after handling rejection call this.prompt.close() so the active turn is reliably rejected before the prompt queue is shut down.
249-270: ⚡ Quick winMinor clarity: tighten what the worker-key browser test is proving.
TestWorkerKeyIncludesBrowserGrantcorrectly makes the key differ by blanking browser identity fields, but it would be clearer if the snippet emphasized which field(s) are expected to drive inequality (e.g., change onlyBrowserSessionIDvs changing all browser fields). This is a small doc quality improvement to prevent future “false confidence” in what’s actually covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md` around lines 249 - 270, The test TestWorkerKeyIncludesBrowserGrant is unclear because it clears all three browser fields at once; change it to make minimal, focused assertions: keep base as-is and create one variant that changes only the BrowserGrantID (or only BrowserSessionID / BrowserID) and assert NewWorkerKey(base) != NewWorkerKey(variant), and optionally add separate tiny subtests that change each browser field individually to prove each one affects NewWorkerKey; update references to NewWorkerKey, TestWorkerKeyIncludesBrowserGrant, and the fields BrowserGrantID, BrowserID, BrowserSessionID so the test explicitly documents which field caused the inequality.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md`:
- Around line 514-515: The doc snippet calls processEnv which invokes browserEnv
using positional fields (browserEnv(base, opts.BrowserGrantID, opts.BrowserID,
opts.BrowserSessionID)) but the actual function signature is browserEnv(base
[]string, opts Options) []string; update the documentation so processEnv calls
browserEnv with the Options object (e.g., pass opts as the second argument) or
otherwise mirror the real signature, and mention the correct symbols browserEnv
and processEnv so readers use the matching call form.
---
Outside diff comments:
In `@internal/session/actor.go`:
- Around line 903-910: The provider must be seeded before any control or
executor runs: move the call to
a.setPiProvider(pi.ProviderOrDefault(tc.Provider)) to occur immediately after
determining/normalizing the model (i.e., right after model =
normalizePiModel(model) and a.setPiModel(model)), so that session-scoped
piProvider is updated prior to runPiExecutor() and runPiControl() reading it;
also ensure the task-acceptance path in internal/loop/daemon.go updates the
actor's provider (via setPiProvider) before the actor is published/made
observable to concurrent control requests so control requests cannot observe the
previous provider/credentials.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md`:
- Around line 1782-1791: The stop() method in WarmClaudeSdkWorker should reject
the in-progress turn before closing the prompt pump; change the ordering in
WarmClaudeSdkWorker.stop(): if this.active is set, create and assign the Error
to this.pumpError, capture the active reference, set this.active = null, call
active.reject(err), and only after handling rejection call this.prompt.close()
so the active turn is reliably rejected before the prompt queue is shut down.
- Around line 249-270: The test TestWorkerKeyIncludesBrowserGrant is unclear
because it clears all three browser fields at once; change it to make minimal,
focused assertions: keep base as-is and create one variant that changes only the
BrowserGrantID (or only BrowserSessionID / BrowserID) and assert
NewWorkerKey(base) != NewWorkerKey(variant), and optionally add separate tiny
subtests that change each browser field individually to prove each one affects
NewWorkerKey; update references to NewWorkerKey,
TestWorkerKeyIncludesBrowserGrant, and the fields BrowserGrantID, BrowserID,
BrowserSessionID so the test explicitly documents which field caused the
inequality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b1f09e9-7db1-4251-ad91-82a1bac50ae5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
cmd/pi_tui.godocs/superpowers/plans/2026-04-29-warm-pi-provider-processes.mdgo.modinternal/lab/scenarios.gointernal/lab/server.gointernal/lab/static/index.htmlinternal/loop/daemon.gointernal/pi/control.gointernal/pi/control_test.gointernal/pi/executor.gointernal/pi/executor_test.gointernal/pi/extension/claude-prompt.test.mjsinternal/pi/extension/index.tsinternal/pi/extension/plan-tools.jsinternal/pi/extension/plan-tools.test.mjsinternal/pi/extension/schema-to-zod.test.mjsinternal/pi/extension/tool-policy.jsinternal/pi/extension_package_test.gointernal/pi/worker_key.gointernal/pi/worker_key_test.gointernal/session/actor.gointernal/session/actor_test.gointernal/session/plan_evidence.gointernal/session/plan_evidence_outbox.gointernal/session/plan_evidence_test.go
💤 Files with no reviewable changes (13)
- internal/pi/extension/tool-policy.js
- cmd/pi_tui.go
- internal/pi/extension_package_test.go
- internal/pi/extension/plan-tools.js
- internal/pi/executor_test.go
- internal/lab/server.go
- internal/lab/scenarios.go
- internal/session/plan_evidence_test.go
- internal/session/plan_evidence.go
- internal/lab/static/index.html
- internal/pi/extension/plan-tools.test.mjs
- internal/pi/worker_key.go
- internal/session/plan_evidence_outbox.go
✅ Files skipped from review due to trivial changes (2)
- go.mod
- internal/pi/worker_key_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/pi/extension/claude-prompt.test.mjs
- internal/pi/executor.go
- internal/pi/control_test.go
- internal/pi/control.go
- internal/session/actor_test.go
glittercowboy
force-pushed
the
codex/remove-plan-mode
branch
from
67013fe to
c7b57ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/loop/daemon.go (1)
1041-1048:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't update the actor provider before the task is accepted.
actor.SetProvider(msg.Provider)runs even ifSendTasklater fails because the session is busy. That mutates session-wide provider state for a task that never started, and it can also flipcurrentPiProvider()for the task that is still running.
runPiExecutoralready seeds the provider when execution actually begins, so this eager update is redundant and risky.Suggested fix
} else { - actor.SetProvider(msg.Provider) actor.SetBrowserContext(browserGrantID, browserID) actor.SetBrowserGrant(browserGrant, session.BrowserRuntimeSnapshot{ ErrorCode: runtime.ErrorCode,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/loop/daemon.go` around lines 1041 - 1048, The code currently calls actor.SetProvider(msg.Provider) before attempting to SendTask, which can mutate session-wide provider state even if SendTask fails; remove the eager provider update — delete the actor.SetProvider(msg.Provider) call from this block (leaving actor.SetBrowserContext and actor.SetBrowserGrant intact) and rely on runPiExecutor to seed the provider when execution actually begins; ensure no other code path reintroduces a pre-accept provider mutation and that currentPiProvider() remains driven by the provider set during runPiExecutor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-29-warm-pi-provider-processes.md`:
- Around line 1792-1801: In stop(), always set this.pumpError to a new Error and
mark this.active = null even when there is no current turn so subsequent turn()
calls fail fast; if an active promise exists still call active.reject(err) as
before, then call this.prompt.close() — set pumpError before closing the prompt
so prompt.push() (in turn()) will see the error; refer to stop(), turn(),
pumpError, active, prompt.push(), and prompt.close() when making the change.
In `@internal/session/actor.go`:
- Around line 380-382: SetProvider currently forwards provider strings to
setPiProvider without normalizing empties, causing empty Provider in a new
message to leave the previous provider intact; update SetProvider (and the other
occurrences around the setPiProvider usage) to normalize empty values before
persisting—i.e., treat "" as a clear/default value and call setPiProvider with
that normalized value (or explicitly clear the provider state) so previous task
provider state cannot leak into subsequent sessions; reference the Actor type
and the SetProvider and setPiProvider methods when making the change.
---
Outside diff comments:
In `@internal/loop/daemon.go`:
- Around line 1041-1048: The code currently calls
actor.SetProvider(msg.Provider) before attempting to SendTask, which can mutate
session-wide provider state even if SendTask fails; remove the eager provider
update — delete the actor.SetProvider(msg.Provider) call from this block
(leaving actor.SetBrowserContext and actor.SetBrowserGrant intact) and rely on
runPiExecutor to seed the provider when execution actually begins; ensure no
other code path reintroduces a pre-accept provider mutation and that
currentPiProvider() remains driven by the provider set during runPiExecutor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 501810b8-d41a-47a1-8926-889e782a06ea
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
cmd/pi_tui.godocs/superpowers/plans/2026-04-29-warm-pi-provider-processes.mdgo.modinternal/lab/scenarios.gointernal/lab/server.gointernal/lab/static/index.htmlinternal/loop/daemon.gointernal/pi/control.gointernal/pi/control_test.gointernal/pi/executor.gointernal/pi/executor_test.gointernal/pi/extension/claude-prompt.test.mjsinternal/pi/extension/index.tsinternal/pi/extension/plan-tools.jsinternal/pi/extension/plan-tools.test.mjsinternal/pi/extension/schema-to-zod.test.mjsinternal/pi/extension/tool-policy.jsinternal/pi/extension_package_test.gointernal/pi/worker_key.gointernal/pi/worker_key_test.gointernal/session/actor.gointernal/session/actor_test.gointernal/session/plan_evidence.gointernal/session/plan_evidence_outbox.gointernal/session/plan_evidence_test.go
💤 Files with no reviewable changes (13)
- internal/lab/static/index.html
- internal/session/plan_evidence_test.go
- internal/pi/extension/plan-tools.test.mjs
- internal/pi/extension/tool-policy.js
- internal/pi/worker_key.go
- internal/pi/executor_test.go
- internal/lab/server.go
- internal/lab/scenarios.go
- cmd/pi_tui.go
- internal/session/plan_evidence_outbox.go
- internal/pi/extension_package_test.go
- internal/session/plan_evidence.go
- internal/pi/extension/plan-tools.js
✅ Files skipped from review due to trivial changes (3)
- go.mod
- internal/pi/extension/schema-to-zod.test.mjs
- internal/pi/control.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/pi/extension/claude-prompt.test.mjs
- internal/session/actor_test.go
- internal/pi/control_test.go
- internal/pi/executor.go
glittercowboy
force-pushed
the
codex/remove-plan-mode
branch
from
c7b57ef to
5887d16
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dependency
Verification
Post-merge
Summary by CodeRabbit
Removed Features
Behavior Changes
Tests