#6 - #4445
Conversation
Disable discovered Pi extensions for T3-managed RPC sessions so arbitrary user hooks cannot alter tool execution. Preserve explicit --extension opt-in support, use Pi’s PI_CODING_AGENT_DIR contract, and allow additional cold-start time before the initial state request fails.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| environment. | ||
|
|
||
| An instance may optionally select a Pi configuration directory. When left empty, it uses the user's | ||
| normal Pi configuration; when set, T3 Code passes the directory as `PI_AGENT_DIR`. Session storage |
There was a problem hiding this comment.
🟢 Low providers/pi.md:28
The docs say T3 Code passes the configuration directory as PI_AGENT_DIR, but the provider actually sets PI_CODING_AGENT_DIR. Users who follow this documentation to reproduce or troubleshoot an instance environment will set the wrong variable, causing Pi to fall back to its normal configuration instead of the selected directory. Consider correcting the environment variable name to match the implementation.
🤖 Copy this AI Prompt to have your agent fix this:
In file @docs/providers/pi.md around line 28:
The docs say T3 Code passes the configuration directory as `PI_AGENT_DIR`, but the provider actually sets `PI_CODING_AGENT_DIR`. Users who follow this documentation to reproduce or troubleshoot an instance environment will set the wrong variable, causing Pi to fall back to its normal configuration instead of the selected directory. Consider correcting the environment variable name to match the implementation.
| }), | ||
| ), | ||
| }), | ||
| Effect.forkIn(runtimeScope), |
There was a problem hiding this comment.
🟡 Medium Drivers/PiSessionRuntime.ts:464
When the Pi subprocess exits while its stdout pipe still contains buffered JSONL records, the final lifecycle events are silently dropped. The child.exitCode fiber immediately calls finishUnexpectedTransportFailure, which calls Queue.end(rawEvents), without waiting for outputFiber to finish draining stdout. Subsequent Queue.offer calls from the stdout fiber occur after Queue.end and are discarded, so the final turn/tool completion events never reach consumers and higher-level lifecycle mapping is left incorrect. Consider coordinating the exit-code handler to wait for outputFiber to complete before ending rawEvents.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Drivers/PiSessionRuntime.ts around line 464:
When the Pi subprocess exits while its stdout pipe still contains buffered JSONL records, the final lifecycle events are silently dropped. The `child.exitCode` fiber immediately calls `finishUnexpectedTransportFailure`, which calls `Queue.end(rawEvents)`, without waiting for `outputFiber` to finish draining stdout. Subsequent `Queue.offer` calls from the stdout fiber occur after `Queue.end` and are discarded, so the final turn/tool completion events never reach consumers and higher-level lifecycle mapping is left incorrect. Consider coordinating the exit-code handler to wait for `outputFiber` to complete before ending `rawEvents`.
| } | ||
|
|
||
| if (method === "select") { | ||
| const options = Array.isArray(value.options) |
There was a problem hiding this comment.
🟡 Medium Layers/PiAdapter.ts:395
piExtensionDialog trims every select option via nonEmptyString, so the returned options array stores trimmed values. When the user picks an option, the trimmed label is sent back to the extension instead of the original value — an option like " indented" is returned as "indented". The Set deduplication also collapses options that differ only in surrounding whitespace, silently dropping choices. Consider preserving the original (non-trimmed) non-empty string as the stored and returned value.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/PiAdapter.ts around line 395:
`piExtensionDialog` trims every `select` option via `nonEmptyString`, so the returned `options` array stores trimmed values. When the user picks an option, the trimmed label is sent back to the extension instead of the original value — an option like `" indented"` is returned as `"indented"`. The `Set` deduplication also collapses options that differ only in surrounding whitespace, silently dropping choices. Consider preserving the original (non-trimmed) non-empty string as the stored and returned value.
| return null; | ||
| } | ||
| answers[question.id] = answer; | ||
| const customAnswer = normalizeDraftAnswer(draft?.customAnswer); |
There was a problem hiding this comment.
🟡 Medium src/pendingUserInput.ts:117
buildPendingUserInputAnswers misses cancellation for multi-select questions. When question.multiSelect is true and the user selects only the cancel option, resolvePendingUserInputAnswer returns an array like ["Cancel"], but the check answer === question.cancelOptionLabel compares the array to a string, so it never matches. The selected cancel option is then stored as a normal answer value instead of { cancelled: true }, so the dialog cannot be cancelled. Consider detecting cancel-option selection in a multi-select-aware way before building the result.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/pendingUserInput.ts around line 117:
`buildPendingUserInputAnswers` misses cancellation for multi-select questions. When `question.multiSelect` is true and the user selects only the cancel option, `resolvePendingUserInputAnswer` returns an array like `["Cancel"]`, but the check `answer === question.cancelOptionLabel` compares the array to a string, so it never matches. The selected cancel option is then stored as a normal answer value instead of `{ cancelled: true }`, so the dialog cannot be cancelled. Consider detecting cancel-option selection in a multi-select-aware way before building the result.
| } | ||
| | { readonly _tag: "Failure"; readonly message: string }; | ||
|
|
||
| export function validatePiLaunchArgs(launchArgs: string): string | undefined { |
There was a problem hiding this comment.
🟡 Medium Drivers/PiRuntime.ts:26
validatePiLaunchArgs treats every token as a potential flag, so a value that happens to equal a managed flag causes a false rejection. For example, --system-prompt "--mode" is valid Pi syntax (parseArgs consumes the next token as the prompt value), but the validator sees the value token --mode and rejects the configuration as an attempted managed override. Any value-taking option whose value equals a managed flag (--mode, --session, -c, etc.) triggers the same false rejection. The validator needs to track which tokens are consumed as option values rather than scanning all tokens as standalone flags.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Drivers/PiRuntime.ts around line 26:
`validatePiLaunchArgs` treats every token as a potential flag, so a value that happens to equal a managed flag causes a false rejection. For example, `--system-prompt "--mode"` is valid Pi syntax (`parseArgs` consumes the next token as the prompt value), but the validator sees the value token `--mode` and rejects the configuration as an attempted managed override. Any value-taking option whose value equals a managed flag (`--mode`, `--session`, `-c`, etc.) triggers the same false rejection. The validator needs to track which tokens are consumed as option values rather than scanning all tokens as standalone flags.
| ), | ||
| ); | ||
|
|
||
| const request = Effect.fn("PiSessionRuntime.request")(function* ( |
There was a problem hiding this comment.
🟡 Medium Drivers/PiSessionRuntime.ts:496
request checks closed and registers its deferred in pendingRequests as two separate steps. If close or finishUnexpectedTransportFailure runs after the closed check but before the pendingRequests update, it drains the old (empty) map, then this request inserts a new entry that is never failed. The caller then waits the full 15/30-second timeout and receives a misleading timeout error instead of the immediate close/transport failure. Consider registering the deferred in pendingRequests before checking closed, and removing it if the session is already closed, so the race window cannot strand the request.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Drivers/PiSessionRuntime.ts around line 496:
`request` checks `closed` and registers its `deferred` in `pendingRequests` as two separate steps. If `close` or `finishUnexpectedTransportFailure` runs after the `closed` check but before the `pendingRequests` update, it drains the old (empty) map, then this request inserts a new entry that is never failed. The caller then waits the full 15/30-second timeout and receives a misleading timeout error instead of the immediate close/transport failure. Consider registering the `deferred` in `pendingRequests` before checking `closed`, and removing it if the session is already closed, so the race window cannot strand the request.
|
|
||
| ## Extensions | ||
|
|
||
| Pi loads the user's normal trusted global and project-local extensions. T3 Code does not install, |
There was a problem hiding this comment.
🟡 Medium providers/pi.md:66
The Extensions section states that Pi loads the user's normal trusted global and project-local extensions, but per ADR 0006-preserve-user-pi-extensions.md every Pi runtime is launched with --no-extensions and only explicitly supplied --extension <path> entries are loaded. This tells users their normal extensions are active when they are actually disabled. Consider correcting this section to describe the --no-extensions launch behavior, or document the rationale if the ADR has since been superseded.
🤖 Copy this AI Prompt to have your agent fix this:
In file @docs/providers/pi.md around line 66:
The Extensions section states that Pi loads the user's normal trusted global and project-local extensions, but per ADR `0006-preserve-user-pi-extensions.md` every Pi runtime is launched with `--no-extensions` and only explicitly supplied `--extension <path>` entries are loaded. This tells users their normal extensions are active when they are actually disabled. Consider correcting this section to describe the `--no-extensions` launch behavior, or document the rationale if the ADR has since been superseded.
ApprovabilityVerdict: Needs human review 6 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
1 participant
What Changed
Why
UI Changes
Checklist
Note
High Risk
Large new subprocess/RPC provider path with session continuation rules and broad event-mapping surface; mistakes could affect thread lifecycle, prompts, or cross-instance session isolation.
Overview
Introduces Pi as a first-class provider: managed RPC launch (
--mode rpc, per-instance session dirs, blocked override flags), version gating (≥0.81.1), model/thinking catalog discovery, and aPiAdapterthat maps streaming, tools, compaction, extension UI (confirm/select/input), interrupts, images, and transport failures into existing T3 provider events.User input answers move from plain strings to
ProviderUserInputAnswers, withcancelOptionLabelon questions and mobilebuildPendingUserInputAnswersemitting{ cancelled: true }vs{ value: ... }for Pi-native extension dialogs.Orchestration ingestion now surfaces
context_compactionlifecycle activities and propagates toolstatus(including failed tone). Codex custom models can inherit reasoning pickers with an added Max option. Agent docs addCONTEXT.mdPi terminology andAGENTS.mdissue-tracker/domain pointers.Reviewed by Cursor Bugbot for commit 52bef09. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Pi as a first-class provider with full RPC session runtime integration
piprovider driver with settings schema, model discovery via RPC, session isolation per provider instance, and a status probe that validates Pi CLI version (≥ 0.81.1) and launch argumentsPiAdapterandPiSessionRuntimeto manage Pi RPC lifecycle, including image attachment loading, event streaming, transport failure handling, and continuation identity validationcancelOptionLabelsupport to user input questions so answers encode either{ value }or{ cancelled: true }instead of an ambiguous stringMaxreasoning option viaaddMaxReasoningOptionForCustomModelwhen aneffort/reasoningEffortselector is presentProviderService.startSession📊 Macroscope summarized 52bef09. 45 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.