pi coding agent in ade -> Primary - #1055
Merged
Merged
Conversation
…idge Makes Pi a first-class chat provider rather than a runtime with rough edges. Sign-in now runs in ADE. A dedicated worker drives Pi's native ModelRuntime.login, and its prompts — auth URL, device code, text, secret, select — render as ADE cards. Pi's own AuthStorage still owns auth.json; ADE never reads, stores, or logs a credential. The terminal /login path stays as a secondary fallback. Pi can now ask the user a question mid-turn via an ask_user tool, and bash, edit, and write are rebuilt from Pi's own definition factories behind an approval card. ADE's default permission mode therefore means "ask before changing things" instead of silently withholding bash. The user's Pi extensions load in chat, bound to a limited UI bridge: select/confirm/input become cards, notify/setStatus become notices, and terminal-only widgets no-op with a one-time warning. The worker pins Pi's projectTrusted to false, so only the user's own profile extensions load and a checked-out repository's .pi/extensions is never executed. Extensions stay off in plan mode, whose read-only promise cannot survive dropping Pi's allowlist. Also fixes two defects shipped by the previous Pi PR: the iOS target did not compile (two Swift type-inference errors), and Pi turns were attributed to the catch-all analytics provider instead of Pi. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR adds Pi provider authentication across desktop, CLI, preload, web runtime, and settings UI. It adds interactive Pi UI bridging, approval-gated tools, extension loading, timeout handling, analytics support, and iOS approval and runtime-mode updates. ChangesPi protocol and interactive runtime
Pi authentication and transport
Analytics and iOS presentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
apps/ade-cli/src/cli.test.ts (1)
2576-2596: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the point where
minTimeoutMsis applied.This test proves
buildCliPlanattaches the floor. It does not proveexecutePlanuses it. The behavior atapps/ade-cli/src/cli.tslines 21263-21266 — raisetimeoutMsto the floor, but keep a larger explicit--timeout-ms— is the part that prevents the premature client timeout, and it is untested.Add a case that runs
executePlanwith a stubbedcreateConnectionand asserts the resolvedtimeoutMs, for both a default--timeout-msand an explicit value above the floor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/cli.test.ts` around lines 2576 - 2596, Extend the test coverage around executePlan to verify it applies minTimeoutMs to the resolved timeoutMs. Stub createConnection and add cases for the default CLI timeout and an explicit --timeout-ms above the floor, asserting the floor is used in the first case while the larger explicit value is preserved in the second; keep the existing buildCliPlan assertions unchanged.apps/desktop/src/main/services/localRuntime/localRuntimeTimeoutPolicy.ts (1)
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the coupling between the transport budget and the service budget.
PI_LOGIN_IPC_TIMEOUT_MSmust stay abovePI_LOGIN_TIMEOUT_MSinapps/desktop/src/main/services/ai/piAuthService.ts. The doc comment states that rule, but nothing enforces it. If someone raises the service timeout past 11 minutes, the transport expires first and the renderer reports a failure for a sign-in the daemon is still running.Add a unit test that asserts
PI_LOGIN_IPC_TIMEOUT_MS > PI_LOGIN_TIMEOUT_MS, or export the service constant and derive this one from it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/localRuntime/localRuntimeTimeoutPolicy.ts` around lines 34 - 42, Guard the timeout relationship used by PI_LOGIN_IPC_TIMEOUT_MS and PI_LOGIN_TIMEOUT_MS so the transport budget always remains greater than the Pi sign-in service budget. Prefer exporting or reusing PI_LOGIN_TIMEOUT_MS from piAuthService, deriving the IPC value with a safety margin; otherwise add a unit test asserting PI_LOGIN_IPC_TIMEOUT_MS > PI_LOGIN_TIMEOUT_MS.apps/ade-cli/src/cli.ts (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the timeout policy shared constants out of the desktop main-process tree.
localRuntimeTimeoutPolicy.tsis shared byapps/ade-cli, the main-process IPC layer, and local-Runtime callers. PlacePI_LOGIN_IPC_TIMEOUT_MS,LONG_RUNNING_LOCAL_RUNTIME_ACTION_TIMEOUTS, andlongRunningLocalRuntimeActionTimeoutMsunderapps/desktop/src/shared/, then import it from there in both the CLI and the desktop main process.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/cli.ts` at line 58, Move PI_LOGIN_IPC_TIMEOUT_MS, LONG_RUNNING_LOCAL_RUNTIME_ACTION_TIMEOUTS, and longRunningLocalRuntimeActionTimeoutMs from the desktop main-process localRuntimeTimeoutPolicy module into apps/desktop/src/shared/. Update the apps/ade-cli import and the desktop main-process IPC/local-Runtime consumers to use the shared module, removing the old main-process definition while preserving the existing exported names and values.apps/desktop/src/main/services/chat/piSdkWorker.ts (1)
770-770: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the
methodparameter so it does not shadow the module helper.This file uses a module-level
method(target, name)helper for dynamic Pi calls, for examplemethod(active, "steer")indispatch. Themethodparameter ofloginProvidershadows it for the whole function body. The current body does not call the helper, so behaviour is correct today. A later edit that adds a helper call insideloginProviderwould silently call a string instead. Rename the parameter tologinMethod.♻️ Proposed rename
-async function loginProvider(providerId: string, method?: string | null): Promise<JsonValue> { +async function loginProvider(providerId: string, loginMethod?: string | null): Promise<JsonValue> {- const requested = nonEmpty(method); + const requested = nonEmpty(loginMethod);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/piSdkWorker.ts` at line 770, Rename the loginProvider parameter method to loginMethod and update every reference to that parameter within the function, leaving the module-level method helper and all other behavior unchanged.apps/desktop/src/main/services/chat/piSdkUiBridge.test.ts (1)
97-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for a signal whose
removeEventListenerthrows.The existing hostile-signal test only throws from
addEventListener, which thetry/catchinrequestalready covers. It does not cover the settle path. Add a case whereaddEventListenersucceeds andremoveEventListenerthrows, then answer the request throughbridge.resolve. The promise must still resolve andpendingCount()must return 0. This pins the fix requested onpiSdkUiBridge.tsLines 73-85.As per coding guidelines: "Record a named regression test or exact alternate verification for every accepted correctness finding."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/piSdkUiBridge.test.ts` around lines 97 - 107, Add a regression test alongside the existing hostile-signal test that uses a signal whose addEventListener succeeds while removeEventListener throws; resolve the request through bridge.resolve, then assert the request promise resolves and bridge.pendingCount() is zero, covering the settle path in request.Source: Coding guidelines
apps/desktop/src/main/services/chat/piSdkUiBridge.ts (1)
66-67: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the
abortedread on a caller-supplied signal.Line 67 reads
options?.signal?.abortedbefore anytryblock.promptOptionsvalidates onlyaddEventListenerandremoveEventListener, so an extension can supply an object whoseabortedgetter throws.requestthen rejects, which breaks the documented never-rejects contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/piSdkUiBridge.ts` around lines 66 - 67, Update the request function’s initial closed/aborted guard to safely handle exceptions from the caller-supplied signal’s aborted getter, preserving the never-rejects contract by returning Promise.resolve(null) when that read throws. Keep the existing behavior for closed and already-aborted requests, and anchor the change in request rather than promptOptions.apps/desktop/src/main/services/chat/agentChatService.ts (1)
11167-11182: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
startPiRuntime's tool-policy restart against a busy runtime.
startPiRuntimecomparestoolPolicyKeyand, on mismatch, callsteardownRuntime(managed, "handle_close")unconditionally. It does not checkmanaged.runtime.busybefore doing this.
runPiTurncallsstartPiRuntime(managed)beforevalidateSessionReadyForTurn(managed)runs. IfstartPiRuntimeis invoked while a Pi turn is in flight (for example, throughresumeSession, which also callsstartPiRuntimeunconditionally) and the permission mode changed since the runtime started, this tears down the live SDK connection mid-turn.The in-flight turn does not lose data silently:
teardownRuntimesetsrt.interrupted = trueon the old runtime object, so the originalrunPiTurncall's catch block reads that flag and emits a gracefulinterruptedstatus. Still, an unrelated action (a permission-mode change from another surface, or a resume call) can abort an active turn that the user did not ask to stop.Compare this to the Cursor runtime:
ensureCursorSdkRuntimeupdates the policy on the live connection when the pool key still matches, instead of tearing it down, andupdateSession's model-switch path defers Cursor's teardown withpendingModelSwitchResetwhile the runtime is busy. Pi has no equivalent guard for this new restart trigger.Add a busy check before tearing down: skip the restart while
managed.runtime.busyis true, and apply the newtoolPolicyKeyon the next turn once the runtime becomes idle (the mismatch check already re-runs on everystartPiRuntimecall).♻️ Proposed guard
if (managed.runtime?.kind === "pi") { + if (managed.runtime.busy) { + // Apply the new tool policy the next time the runtime is idle; + // tearing it down now would abort an in-flight turn. + return managed.runtime; + } if (managed.runtime.poolKey === poolKey && managed.runtime.toolPolicyKey === toolPolicyKey && isPiSdkPooledAlive(managed.runtime.sdk)) { return managed.runtime; } teardownRuntime(managed, "handle_close"); } else if (managed.runtime) { teardownRuntime(managed, "handle_close"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 11167 - 11182, Update startPiRuntime’s existing Pi runtime policy-mismatch branch to check managed.runtime.busy before calling teardownRuntime. When the runtime is busy, preserve the active connection and return it without restarting; allow the existing mismatch teardown to run once the runtime is idle so the next startPiRuntime call applies the new toolPolicyKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/README.md`:
- Around line 656-658: Update the Pi login timeout documentation in this README
section and the corresponding ARCHITECTURE.md description to match
PI_LOGIN_TIMEOUT_MS, which is 10 minutes; only document 11 minutes if the
constant is intentionally raised instead.
In `@apps/desktop/src/main/services/ai/piAuthService.ts`:
- Around line 259-265: Add an onUiCancel handler alongside onUiRequest and
onUiNotice that clears flow.pendingRequestId and emits a pending state without
the stale prompt. Also update the flow’s finish logic to clear pendingRequestId
so completed flows cannot retain a settled request.
In `@apps/desktop/src/main/services/chat/piSdkUiBridge.ts`:
- Around line 73-85: Update the settle function so it removes the request from
pending and calls resolve(value) before cleanup or notification side effects.
Perform removeEventListener and the conditional ui_cancel post afterward, each
defensively so exceptions cannot prevent settlement; preserve the existing
settled guard and side-effect conditions.
In `@apps/desktop/src/main/services/chat/piSdkWorker.ts`:
- Around line 778-799: Update the login-type selection in loginProvider so an
omitted method uses the provider’s authTypes when runtime.getProvider is
unavailable: select the first supported authTypes value, or reject the request
with an error listing the full supported authTypes and requiring an explicit
supported method. Preserve explicit method validation and the existing
provider-specific API-key/OAuth checks.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 4881-4912: Validate renderer-supplied arguments in the IPC
handlers for aiPiLoginStart, aiPiLoginSubmit, and aiPiLoginCancel before calling
startPiLogin, submitPiLoginPrompt, or cancelPiLogin. Reuse requireNonEmptyString
as the action path does for providerId, requestId, and value, and validate
arg.method against the supported PiLoginMethod values when provided; reject
malformed payloads at the boundary rather than forwarding them to the Pi
authentication service.
In `@apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift`:
- Around line 1685-1693: Update the question filtering logic around
rawToolCallQuestionTexts and wrappedQuestionTexts to key normalized signatures
by nonempty turnId, and only remove a raw question when a matching wrapped
question exists in the same turn. Preserve raw cards with no correlation key,
and add a regression test covering identical open-question text across different
turns where the bare question remains visible.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.test.ts`:
- Around line 2576-2596: Extend the test coverage around executePlan to verify
it applies minTimeoutMs to the resolved timeoutMs. Stub createConnection and add
cases for the default CLI timeout and an explicit --timeout-ms above the floor,
asserting the floor is used in the first case while the larger explicit value is
preserved in the second; keep the existing buildCliPlan assertions unchanged.
In `@apps/ade-cli/src/cli.ts`:
- Line 58: Move PI_LOGIN_IPC_TIMEOUT_MS,
LONG_RUNNING_LOCAL_RUNTIME_ACTION_TIMEOUTS, and
longRunningLocalRuntimeActionTimeoutMs from the desktop main-process
localRuntimeTimeoutPolicy module into apps/desktop/src/shared/. Update the
apps/ade-cli import and the desktop main-process IPC/local-Runtime consumers to
use the shared module, removing the old main-process definition while preserving
the existing exported names and values.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 11167-11182: Update startPiRuntime’s existing Pi runtime
policy-mismatch branch to check managed.runtime.busy before calling
teardownRuntime. When the runtime is busy, preserve the active connection and
return it without restarting; allow the existing mismatch teardown to run once
the runtime is idle so the next startPiRuntime call applies the new
toolPolicyKey.
In `@apps/desktop/src/main/services/chat/piSdkUiBridge.test.ts`:
- Around line 97-107: Add a regression test alongside the existing
hostile-signal test that uses a signal whose addEventListener succeeds while
removeEventListener throws; resolve the request through bridge.resolve, then
assert the request promise resolves and bridge.pendingCount() is zero, covering
the settle path in request.
In `@apps/desktop/src/main/services/chat/piSdkUiBridge.ts`:
- Around line 66-67: Update the request function’s initial closed/aborted guard
to safely handle exceptions from the caller-supplied signal’s aborted getter,
preserving the never-rejects contract by returning Promise.resolve(null) when
that read throws. Keep the existing behavior for closed and already-aborted
requests, and anchor the change in request rather than promptOptions.
In `@apps/desktop/src/main/services/chat/piSdkWorker.ts`:
- Line 770: Rename the loginProvider parameter method to loginMethod and update
every reference to that parameter within the function, leaving the module-level
method helper and all other behavior unchanged.
In `@apps/desktop/src/main/services/localRuntime/localRuntimeTimeoutPolicy.ts`:
- Around line 34-42: Guard the timeout relationship used by
PI_LOGIN_IPC_TIMEOUT_MS and PI_LOGIN_TIMEOUT_MS so the transport budget always
remains greater than the Pi sign-in service budget. Prefer exporting or reusing
PI_LOGIN_TIMEOUT_MS from piAuthService, deriving the IPC value with a safety
margin; otherwise add a unit test asserting PI_LOGIN_IPC_TIMEOUT_MS >
PI_LOGIN_TIMEOUT_MS.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c7cf57f-31ff-4456-b45c-fec11dc0b5d9
⛔ Files ignored due to path filters (9)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/agent-routing.mdis excluded by!docs/**docs/features/chat/tool-system.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/configuration-schema.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (42)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/__tests__/piSdk.integration.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/piAuthService.test.tsapps/desktop/src/main/services/ai/piAuthService.tsapps/desktop/src/main/services/analytics/dailyUsageAnalytics.tsapps/desktop/src/main/services/analytics/productAnalyticsPolicy.tsapps/desktop/src/main/services/analytics/productAnalyticsService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/piSdkEventMapper.test.tsapps/desktop/src/main/services/chat/piSdkEventMapper.tsapps/desktop/src/main/services/chat/piSdkPool.tsapps/desktop/src/main/services/chat/piSdkProtocol.test.tsapps/desktop/src/main/services/chat/piSdkProtocol.tsapps/desktop/src/main/services/chat/piSdkUiBridge.test.tsapps/desktop/src/main/services/chat/piSdkUiBridge.tsapps/desktop/src/main/services/chat/piSdkWorker.tsapps/desktop/src/main/services/ipc/ipcTimeouts.test.tsapps/desktop/src/main/services/ipc/ipcTimeouts.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/localRuntime/localRuntimeTimeoutPolicy.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/webclient/adapter/misc.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/config.tsapps/ios/ADE/Views/PersonalChats/PersonalChatsScreen.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkModelCatalog.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkPlanComposerViews.swiftapps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swiftapps/ios/ADETests/ADETests.swift
Comment on lines
+656
to
+658
| `ai.piLoginStart` carries its own transport floor (11 minutes, the same budget | ||
| the desktop client uses) so the CLI does not report a timeout while the daemon | ||
| is still waiting on the user; `--timeout-ms` still applies when it asks for |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Pi login timeout constants and their values.
rg -nP -C3 'PI_LOGIN_[A-Z_]*TIMEOUT_MS|piLoginStart' --type=ts -g '!**/*.test.ts'Repository: arul28/ADE
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -u
echo "## repo files matching names/types"
git ls-files | rg -n '(^|/)(README\.md|.*pi.*|.*login.*|.*(test|spec)\.ts|.*(js|ts)$)' | sed -n '1,200p'
echo
echo "## search all tracked files for PI_LOGIN/piLogin/start timeout constants"
rg -nP -C3 'PI_LOGIN_|piLoginStart|transport floor|minutes|timeout' -S -g '!node_modules' -g '!dist' -g '!build' . | sed -n '1,240p'Repository: arul28/ADE
Length of output: 33173
🏁 Script executed:
#!/bin/bash
set -u
echo "## apps/ade-cli/README.md around lines 640-670"
sed -n '640,670p' apps/ade-cli/README.md
echo
echo "## exact search across tracked files for timeout constant patterns (case-insensitive)"
rg -nP -C3 'login.*timeout|timeout.*login|PI_LOGIN|LOGIN_IPC|START.*TIMEOUT|transport floor|10 minutes|11 minutes|660000|600000|659000' --glob '!apps/ade-cli/README.md' --glob '!node_modules' --glob '!dist' --glob '!build' . | sed -n '1,220p'
echo
echo "## focused constants/timeout declarations in CLI app source files"
rg -nP -C3 'TIMEOUT|timeout_ms|timeoutMs|setTimeout|Timeout|wait.*Login|login.*wait|transport' apps/ade-cli/src --glob '!*.test.ts' | sed -n '1,260p'Repository: arul28/ADE
Length of output: 47672
🏁 Script executed:
#!/bin/bash
set -u
echo "## tracked Python/JS/TS files containing piLoginStart/PiLogin/PI_LOGIN constants"
rg -n -C6 'const P[I_1A-Z0-9_]+[I_1A-Z0-9_]*\s*=|export const P[I_1A-Z0-9_]+[I_1A-Z0-9_]*\s*=|PI_LOGIN|piLoginStart|PI_LOGIN_IPC_TIMEOUT_MS|TimeoutError|IPC_TIMEOUT|login timeout|login-timeline' \
--glob '!apps/ade-cli/README.md' \
--glob '!node_modules' \
--glob '!dist' \
--glob '!build' \
--glob '!*.mjs' \
--glob '!*.cjs' \
. | sed -n '1,260p'
echo
echo "## files in apps/ade-cli/src containing any timeout-related declarations"
rg -n -C4 '^(const|let|var|export const|export let|export var) .*[Tt]imeout|TIMEOUT|timeout_ms|timeoutMs|Seconds\(\d+\)|Timeout' \
apps/ade-cli/src \
--glob '!*.test.ts' \
| sed -n '1,320p'Repository: arul28/ADE
Length of output: 49525
🏁 Script executed:
#!/bin/bash
set -u
echo "## desktop Pi login service header and timeout usage"
sed -n '1,80p' apps/desktop/src/main/services/ai/piAuthService.ts
echo
sed -n '250,278p' apps/desktop/src/main/services/ai/piAuthService.ts
echo
echo "## all tracked definitions of piAuthService or PiLogin timeout symbols"
rg -n -C4 '(PI_LOGIN_TIMEOUT_MS|10 \* 60 \* 1000|6[0-9]{5})|startPiLogin|listPiLoginProviders|piAuthStatus' apps/desktop/src/main/services apps/desktop/src/main/services apps/ade-cli/src docs docs/*.md apps --glob '!node_modules' --glob '!dist' --glob '!build' | sed -n '1,320p'Repository: arul28/ADE
Length of output: 47062
Fix the documented Pi login timeout value.
apps/desktop/src/main/services/ai/piAuthService.ts defines PI_LOGIN_TIMEOUT_MS = 10 * 60 * 1000, but this section and docs/ARCHITECTURE.md describe an 11-minute transport budget. Update this to the actual 10-minute value unless the constant is being raised.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ade-cli/README.md` around lines 656 - 658, Update the Pi login timeout
documentation in this README section and the corresponding ARCHITECTURE.md
description to match PI_LOGIN_TIMEOUT_MS, which is 10 minutes; only document 11
minutes if the constant is intentionally raised instead.
Comment on lines
+73
to
+85
| const settle = (value: string | null, fromDesktop = false): void => { | ||
| if (settled) return; | ||
| settled = true; | ||
| if (timer !== undefined) clearTimeout(timer); | ||
| options?.signal?.removeEventListener("abort", onAbort); | ||
| pending.delete(requestId); | ||
| // A settle the desktop did not ask for — an extension's own timeout or | ||
| // abort signal — would otherwise leave its card on screen forever. | ||
| if (!fromDesktop && !closed) { | ||
| post({ protocolVersion: PI_SDK_PROTOCOL_VERSION, type: "ui_cancel", requestId }); | ||
| } | ||
| resolve(value); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make settle resolve even when removeEventListener or post throws.
settle marks settled = true first, then calls options?.signal?.removeEventListener and post. promptOptions only checks that removeEventListener is a function, so an extension can supply one that throws. post can also throw when the IPC channel is gone. In both cases the code after the throw never runs: the pending entry is not deleted and resolve(value) is never called. The awaiting Pi callback then hangs, which is the exact failure this bridge exists to prevent. The try/catch at Line 89 does not cover the desktop-driven resolve path.
Delete the entry and resolve first, then perform the side effects defensively.
🛡️ Proposed fix
const settle = (value: string | null, fromDesktop = false): void => {
if (settled) return;
settled = true;
if (timer !== undefined) clearTimeout(timer);
- options?.signal?.removeEventListener("abort", onAbort);
pending.delete(requestId);
- // A settle the desktop did not ask for — an extension's own timeout or
- // abort signal — would otherwise leave its card on screen forever.
- if (!fromDesktop && !closed) {
- post({ protocolVersion: PI_SDK_PROTOCOL_VERSION, type: "ui_cancel", requestId });
- }
resolve(value);
+ try {
+ options?.signal?.removeEventListener("abort", onAbort);
+ } catch {
+ // A hostile signal must not break teardown.
+ }
+ // A settle the desktop did not ask for — an extension's own timeout or
+ // abort signal — would otherwise leave its card on screen forever.
+ if (!fromDesktop && !closed) {
+ try {
+ post({ protocolVersion: PI_SDK_PROTOCOL_VERSION, type: "ui_cancel", requestId });
+ } catch {
+ // The channel is gone; the card dies with the worker.
+ }
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const settle = (value: string | null, fromDesktop = false): void => { | |
| if (settled) return; | |
| settled = true; | |
| if (timer !== undefined) clearTimeout(timer); | |
| options?.signal?.removeEventListener("abort", onAbort); | |
| pending.delete(requestId); | |
| // A settle the desktop did not ask for — an extension's own timeout or | |
| // abort signal — would otherwise leave its card on screen forever. | |
| if (!fromDesktop && !closed) { | |
| post({ protocolVersion: PI_SDK_PROTOCOL_VERSION, type: "ui_cancel", requestId }); | |
| } | |
| resolve(value); | |
| }; | |
| const settle = (value: string | null, fromDesktop = false): void => { | |
| if (settled) return; | |
| settled = true; | |
| if (timer !== undefined) clearTimeout(timer); | |
| pending.delete(requestId); | |
| resolve(value); | |
| try { | |
| options?.signal?.removeEventListener("abort", onAbort); | |
| } catch { | |
| // A hostile signal must not break teardown. | |
| } | |
| // A settle the desktop did not ask for — an extension's own timeout or | |
| // abort signal — would otherwise leave its card on screen forever. | |
| if (!fromDesktop && !closed) { | |
| try { | |
| post({ protocolVersion: PI_SDK_PROTOCOL_VERSION, type: "ui_cancel", requestId }); | |
| } catch { | |
| // The channel is gone; the card dies with the worker. | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/chat/piSdkUiBridge.ts` around lines 73 - 85,
Update the settle function so it removes the request from pending and calls
resolve(value) before cleanup or notification side effects. Perform
removeEventListener and the conditional ui_cancel post afterward, each
defensively so exceptions cannot prevent settlement; preserve the existing
settled guard and side-effect conditions.
Comment on lines
+778
to
+799
| const provider = typeof runtime.getProvider === "function" | ||
| ? record((runtime.getProvider as Callable).call(runtime, providerId)) | ||
| : null; | ||
| const auth = record(provider?.auth); | ||
| // Pi only has two login types. An api-key provider without an interactive | ||
| // `login` is ambient-only (env var, cloud profile) and has nothing to run. | ||
| const requested = nonEmpty(method); | ||
| const authType = requested === "api_key" || requested === "oauth" | ||
| ? requested | ||
| : auth?.oauth | ||
| ? "oauth" | ||
| : "api_key"; | ||
| if (authType === "oauth" && auth && !auth.oauth) { | ||
| throw new Error(`Pi provider "${providerId}" does not support OAuth sign-in.`); | ||
| } | ||
| if (authType === "api_key" && auth) { | ||
| const apiKeyAuth = record(auth.apiKey); | ||
| if (!apiKeyAuth) throw new Error(`Pi provider "${providerId}" does not support API key sign-in.`); | ||
| if (typeof apiKeyAuth.login !== "function") { | ||
| throw new Error(`Pi provider "${providerId}" reads its API key from the environment, so there is nothing to sign in to.`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the desktop auth service selects the login method for Pi providers.
fd -t f 'piAuthService.ts' | xargs rg -n -C6 '\blogin\s*\(|authTypes|method'Repository: arul28/ADE
Length of output: 2819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant service/workers without executing repo code.
fd -t f 'piSdkWorker.ts|piAuthService.ts' .
echo '--- piSdkWorker outline ---'
ast-grep outline apps/desktop/src/main/services/chat/piSdkWorker.ts --view expanded || true
echo '--- piAuthService relevant sections ---'
sed -n '120,285p' apps/desktop/src/main/services/chat/piAuthService.ts | cat -n
echo '--- piSdkWorker relevant sections ---'
sed -n '720,820p' apps/desktop/src/main/services/chat/piSdkWorker.ts | cat -n
echo '--- authInventory/authTypes usages ---'
rg -n -C4 'authInventory|authTypes|startPiLogin|api_key|oauth' apps/desktop/src/main/services/chat apps/desktop/src/main apps/desktop/src/frontend -g '*.{ts,tsx}' | head -n 240Repository: arul28/ADE
Length of output: 4396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- piAuthService.ts relevant sections ---'
sed -n '120,285p' apps/desktop/src/main/services/ai/piAuthService.ts | cat -n
echo '--- piSdkWorker.ts relevant login section ---'
sed -n '720,830p' apps/desktop/src/main/services/chat/piSdkWorker.ts | cat -n
echo '--- authInventory/authTypes usages ---'
rg -n -C4 'authInventory|authTypes|startPiLogin|api_key|oauth' apps/desktop/src/main apps/desktop/src/frontend -g '*.{ts,tsx}' | head -n 320Repository: arul28/ADE
Length of output: 40521
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
api_path = Path("apps/desktop/src/main/services/adeActions/registry.ts")
auth_path = Path("apps/desktop/src/main/services/ai/piAuthService.ts")
worker_path = Path("apps/desktop/src/main/services/chat/piSdkWorker.ts")
api_text = api_path.read_text()
auth_text = auth_path.read_text()
worker_text = worker_path.read_text()
print("registry piLoginStart implementation:")
m = re.search(r'piLoginStart: async \(args\?: \{\s*providerId\?: string;\s*method\?: ["\'](?:oauth|api_key)["\']\s*\}\).*?(?:\n\s*\},\n|\n \});', api_text, re.S)
print(m.group(0) if m else "not found")
print("\nauth invoke starts with only supplied args:")
m = re.search(r'pooled\.login\(\{ providerId,\s*\.\.\.\(args\.method \? \{ method: args\.method \} : \{\}\)\s*\}\)', auth_text)
print(m.group(0) if m else "not found")
print("\nworker default logic:")
m = re.search(r'const authType = requested === "api_key" || requested === "oauth"\s*\? requested\s*: auth\?\.oauth\s*\? "oauth"\s*: "api_key";', worker_text)
print(m.group(0) if m else "not found")
# Parse the relevant conditional chain into a truth table from source.
expr = r'const authType = requested === "api_key" || requested === "oauth"\s*\? requested\s*: auth\?\.oauth\s*\? "oauth"\s*: "api_key";'
def auth_type(requested, api_key, oauth):
auth = None
if api_key is not None or oauth is not None:
auth = {"apiKey": {"login": True} if api_key else None, "oauth": oauth}
if requested in ("api_key", "oauth"):
return requested
if auth and auth.get("oauth"):
return "oauth"
return "api_key"
cases = []
for requested in (None, "api_key", "oauth"):
for api_key in (False, True):
for oauth in (False, True):
cases.append((requested, api_key, oauth, auth_type(requested, api_key, oauth)))
print("\nselected cases:")
for r, a, o, at in cases:
print(requested=r, has_api=a, oauth=o, authType=at)
print("oauth-only default to api_key:",
any(r in (None, "") and a is False and o is True and at == "api_key" for r, a, o, at in cases))
PYRepository: arul28/ADE
Length of output: 432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- piLoginStart handler ---'
sed -n '2788,2812p' apps/desktop/src/main/services/adeActions/registry.ts | cat -n
echo '--- startPiLogin invoke with method ---'
sed -n '148,158p' apps/desktop/src/main/services/ai/piAuthService.ts | cat -n
echo '--- loginProvider authType chain ---'
sed -n '758,772p' apps/desktop/src/main/services/chat/piSdkWorker.ts | cat -n
python3 - <<'PY'
from pathlib import Path
import re
for label, path, patterns in [
("registry", Path("apps/desktop/src/main/services/adeActions/registry.ts"), ["piLoginStart"]),
("auth", Path("apps/desktop/src/main/services/ai/piAuthService.ts"), ["pooled.login"]),
("worker", Path("apps/desktop/src/main/services/chat/piSdkWorker.ts"), ['const authType']),
]:
text = path.read_text()
print(f"{label}: contains {patterns!r}: {all(p in text for p in patterns)}")
PYRepository: arul28/ADE
Length of output: 3274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- line count ---'
wc -l apps/desktop/src/main/services/chat/piSdkWorker.ts apps/desktop/src/main/services/ai/piAuthService.ts
echo '--- startPiLogin body from piAuthService.ts ---'
grep -n -A 90 'export async function startPiLogin' apps/desktop/src/main/services/ai/piAuthService.ts
echo '--- pooled.login exact context from piAuthService.ts ---'
grep -n -C 12 'pooled.login' apps/desktop/src/main/services/ai/piAuthService.ts
echo '--- loginProvider body from piSdkWorker.ts ---'
awk '/^async function loginProvider/,/^}/ { print NR ":" $0 }' apps/desktop/src/main/services/chat/piSdkWorker.ts | head -120
echo '--- authInventory implementation ---'
awk '/^async function authInventory/,/^}/ { print NR ":" $0 }' apps/desktop/src/main/services/chat/piSdkWorker.ts | head -90
echo '--- direct source checks ---'
python3 - <<'PY'
from pathlib import Path
for path, needles in [
("apps/desktop/src/main/services/ai/piAuthService.ts", ["pooled.login({ providerId,"]),
("apps/desktop/src/main/services/chat/piSdkWorker.ts", ["const authType = requested ==="]),
]:
text = Path(path).read_text()
for needle in needles:
print(path, "has", repr(needle), needle in text)
PYRepository: arul28/ADE
Length of output: 4177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk '/^async function loginProvider/,/^[[:space:]]*}$/' apps/desktop/src/main/services/chat/piSdkWorker.ts | cat -n
awk '/^async function authInventory/,/^[[:space:]]*}$/' apps/desktop/src/main/services/chat/piSdkWorker.ts | cat -nRepository: arul28/ADE
Length of output: 3236
Derive the default login type from authTypes when getProvider is unavailable.
piLoginStart allows an omitted method, and startPiLogin only passes it when supplied, so loginProvider receives no method. When runtime.getProvider is missing, auth becomes null, making the fallback "api_key" and skipping the provider-specific API-key validation. For omitted method, choose the first supported authTypes value from the provider or report the full authTypes and require the caller to pass a supported type.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/chat/piSdkWorker.ts` around lines 778 - 799,
Update the login-type selection in loginProvider so an omitted method uses the
provider’s authTypes when runtime.getProvider is unavailable: select the first
supported authTypes value, or reject the request with an error listing the full
supported authTypes and requiring an explicit supported method. Preserve
explicit method validation and the existing provider-specific API-key/OAuth
checks.
Comment on lines
+4881
to
+4912
| ipcMain.handle( | ||
| IPC.aiPiLoginStart, | ||
| async (_event, arg: { providerId: string; method?: PiLoginMethod }): Promise<PiLoginResult> => { | ||
| const ctx = getCtx(); | ||
| const result = await startPiLogin(arg); | ||
| if (result.ok) { | ||
| try { | ||
| ctx.aiIntegrationService?.invalidateProviderReadinessCaches(); | ||
| } catch (error) { | ||
| // Matches the action-path twin so both halves of one incident are | ||
| // searchable under a single event name. | ||
| ctx.logger.warn("ai.pi_auth_cache_invalidation_failed", { | ||
| provider: arg.providerId, | ||
| error: getErrorMessage(error), | ||
| }); | ||
| } | ||
| } | ||
| return result; | ||
| }, | ||
| ); | ||
|
|
||
| ipcMain.handle( | ||
| IPC.aiPiLoginSubmit, | ||
| async ( | ||
| _event, | ||
| arg: { providerId: string; requestId: string; value: string }, | ||
| ): Promise<{ ok: boolean; error?: string }> => submitPiLoginPrompt(arg), | ||
| ); | ||
|
|
||
| ipcMain.handle(IPC.aiPiLoginCancel, async (_event, arg: { providerId: string }): Promise<void> => { | ||
| cancelPiLogin(arg); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the Pi login arguments at the IPC boundary.
These three handlers pass renderer-supplied payloads straight through. The TypeScript annotations are erased at runtime, so a malformed payload reaches piAuthService, where args.providerId.trim() throws a TypeError on a missing or non-string value. arg.method is likewise forwarded unchecked and only tested for truthiness inside startPiLogin, so an arbitrary string can reach Pi as a login method.
The action path in apps/desktop/src/main/services/adeActions/registry.ts (lines 2795-2825) already guards the same inputs with requireNonEmptyString. Align the IPC path with it.
🛡️ Proposed fix
+ const requirePiProviderId = (value: unknown): string => {
+ const providerId = typeof value === "string" ? value.trim() : "";
+ if (!providerId) throw new Error("providerId is required.");
+ return providerId;
+ };
+
ipcMain.handle(
IPC.aiPiLoginStart,
- async (_event, arg: { providerId: string; method?: PiLoginMethod }): Promise<PiLoginResult> => {
+ async (_event, arg: { providerId?: unknown; method?: unknown }): Promise<PiLoginResult> => {
const ctx = getCtx();
- const result = await startPiLogin(arg);
+ const providerId = requirePiProviderId(arg?.providerId);
+ const method: PiLoginMethod | undefined =
+ arg?.method === "oauth" || arg?.method === "api_key" ? arg.method : undefined;
+ const result = await startPiLogin({ providerId, ...(method ? { method } : {}) });
if (result.ok) {
try {
ctx.aiIntegrationService?.invalidateProviderReadinessCaches();
} catch (error) {
// Matches the action-path twin so both halves of one incident are
// searchable under a single event name.
ctx.logger.warn("ai.pi_auth_cache_invalidation_failed", {
- provider: arg.providerId,
+ provider: providerId,
error: getErrorMessage(error),
});
}
}
return result;
},
);
ipcMain.handle(
IPC.aiPiLoginSubmit,
async (
_event,
- arg: { providerId: string; requestId: string; value: string },
- ): Promise<{ ok: boolean; error?: string }> => submitPiLoginPrompt(arg),
+ arg: { providerId?: unknown; requestId?: unknown; value?: unknown },
+ ): Promise<{ ok: boolean; error?: string }> => submitPiLoginPrompt({
+ providerId: requirePiProviderId(arg?.providerId),
+ requestId: typeof arg?.requestId === "string" ? arg.requestId : "",
+ value: typeof arg?.value === "string" ? arg.value : "",
+ }),
);
- ipcMain.handle(IPC.aiPiLoginCancel, async (_event, arg: { providerId: string }): Promise<void> => {
- cancelPiLogin(arg);
+ ipcMain.handle(IPC.aiPiLoginCancel, async (_event, arg: { providerId?: unknown }): Promise<void> => {
+ cancelPiLogin({ providerId: requirePiProviderId(arg?.providerId) });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ipcMain.handle( | |
| IPC.aiPiLoginStart, | |
| async (_event, arg: { providerId: string; method?: PiLoginMethod }): Promise<PiLoginResult> => { | |
| const ctx = getCtx(); | |
| const result = await startPiLogin(arg); | |
| if (result.ok) { | |
| try { | |
| ctx.aiIntegrationService?.invalidateProviderReadinessCaches(); | |
| } catch (error) { | |
| // Matches the action-path twin so both halves of one incident are | |
| // searchable under a single event name. | |
| ctx.logger.warn("ai.pi_auth_cache_invalidation_failed", { | |
| provider: arg.providerId, | |
| error: getErrorMessage(error), | |
| }); | |
| } | |
| } | |
| return result; | |
| }, | |
| ); | |
| ipcMain.handle( | |
| IPC.aiPiLoginSubmit, | |
| async ( | |
| _event, | |
| arg: { providerId: string; requestId: string; value: string }, | |
| ): Promise<{ ok: boolean; error?: string }> => submitPiLoginPrompt(arg), | |
| ); | |
| ipcMain.handle(IPC.aiPiLoginCancel, async (_event, arg: { providerId: string }): Promise<void> => { | |
| cancelPiLogin(arg); | |
| }); | |
| const requirePiProviderId = (value: unknown): string => { | |
| const providerId = typeof value === "string" ? value.trim() : ""; | |
| if (!providerId) throw new Error("providerId is required."); | |
| return providerId; | |
| }; | |
| ipcMain.handle( | |
| IPC.aiPiLoginStart, | |
| async (_event, arg: { providerId?: unknown; method?: unknown }): Promise<PiLoginResult> => { | |
| const ctx = getCtx(); | |
| const providerId = requirePiProviderId(arg?.providerId); | |
| const method: PiLoginMethod | undefined = | |
| arg?.method === "oauth" || arg?.method === "api_key" ? arg.method : undefined; | |
| const result = await startPiLogin({ providerId, ...(method ? { method } : {}) }); | |
| if (result.ok) { | |
| try { | |
| ctx.aiIntegrationService?.invalidateProviderReadinessCaches(); | |
| } catch (error) { | |
| // Matches the action-path twin so both halves of one incident are | |
| // searchable under a single event name. | |
| ctx.logger.warn("ai.pi_auth_cache_invalidation_failed", { | |
| provider: providerId, | |
| error: getErrorMessage(error), | |
| }); | |
| } | |
| } | |
| return result; | |
| }, | |
| ); | |
| ipcMain.handle( | |
| IPC.aiPiLoginSubmit, | |
| async ( | |
| _event, | |
| arg: { providerId?: unknown; requestId?: unknown; value?: unknown }, | |
| ): Promise<{ ok: boolean; error?: string }> => submitPiLoginPrompt({ | |
| providerId: requirePiProviderId(arg?.providerId), | |
| requestId: typeof arg?.requestId === "string" ? arg.requestId : "", | |
| value: typeof arg?.value === "string" ? arg.value : "", | |
| }), | |
| ); | |
| ipcMain.handle(IPC.aiPiLoginCancel, async (_event, arg: { providerId?: unknown }): Promise<void> => { | |
| cancelPiLogin({ providerId: requirePiProviderId(arg?.providerId) }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 4881 - 4912,
Validate renderer-supplied arguments in the IPC handlers for aiPiLoginStart,
aiPiLoginSubmit, and aiPiLoginCancel before calling startPiLogin,
submitPiLoginPrompt, or cancelPiLogin. Reuse requireNonEmptyString as the action
path does for providerId, requestId, and value, and validate arg.method against
the supported PiLoginMethod values when provided; reject malformed payloads at
the boundary rather than forwarding them to the Pi authentication service.
Comment on lines
+1685
to
+1693
| guard !rawToolCallQuestionTexts.isEmpty, !wrappedQuestionTexts.isEmpty else { return results } | ||
| return results.filter { item in | ||
| guard case .question(let model) = item, | ||
| let rawTexts = rawToolCallQuestionTexts[model.id] | ||
| else { | ||
| return true | ||
| } | ||
| return rawTexts.isDisjoint(with: wrappedQuestionTexts) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope duplicate question matching to the same turn.
Lines 1685-1693 remove a raw toolCall question when any wrapped question has the same normalized text. The comparison ignores turnId. A valid bare question can disappear if another open wrapped question in a different turn has the same text.
Store wrapped and raw signatures by nonempty turnId. Remove a raw card only when its matching wrapped card is in the same turn. Keep raw cards that have no correlation key.
Add a regression test with two open questions in different turns that use identical text. The bare question must remain visible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift` around lines 1685 -
1693, Update the question filtering logic around rawToolCallQuestionTexts and
wrappedQuestionTexts to key normalized signatures by nonempty turnId, and only
remove a raw question when a matching wrapped question exists in the same turn.
Preserve raw cards with no correlation key, and add a regression test covering
identical open-question text across different turns where the bare question
remains visible.
…ear cancelled login prompts Review found three real gaps. Extensions were loading in ask-first modes, where an extension tool runs outside the approval card that mode promises; they now load only where the mode grants its tools outright. The web client's status drain expired at five minutes while a Pi sign-in may run ten, stranding the UI mid-flow. And a login prompt the worker settled itself stayed on screen because the auth service never handled the cancel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
![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: 4e4a304193
ℹ️ 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".
| * off runs ADE chat with Pi's built-in tools only; the Pi CLI is unaffected | ||
| * either way. | ||
| */ | ||
| piExtensionsEnabled?: boolean; |
There was a problem hiding this comment.
When ai.chat.piExtensionsEnabled: false is set in project configuration, coerceAiChatConfig in projectConfigService.ts never copies this new field, so it is dropped during config loading and the effective value read by piChatExtensionsEnabled remains undefined. In edit and full-auto Pi chats that makes the !== false check enable extensions despite the explicit opt-out; add this boolean to the existing chat-config coercion path.
Useful? React with 👍 / 👎.
Comment on lines
+4317
to
+4318
| const removeLocal = subscribeLocalPiAuthStatusEvents(cb); | ||
| const removeRemote = subscribeRemotePiAuthStatusEvents(cb); |
There was a problem hiding this comment.
When a local project runtime is bound, each Pi status is delivered both through the direct BrowserWindow IPC broadcast in registerIpc.ts and through the same runtime's buffered relay registered in registry.ts, so attaching the callback to both feeds here invokes it twice. The buffered copy can arrive after the direct one; for text or secret prompts, ProvidersSection clears promptValue on every prompt event, so that delayed duplicate can erase an API key the user has already started typing. Select the feed appropriate to the active binding or deduplicate the events.
Useful? React with 👍 / 👎.
…ping a typed key The new ai.chat.piExtensionsEnabled flag was dropped by the chat-config coercer, so setting it to false did nothing. A local runtime also delivers each Pi auth status twice — direct IPC plus the buffered relay — and the second copy could land after the user began typing, clearing the field; only a genuinely new prompt resets it now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb2022d204
ℹ️ 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".
Comment on lines
+684
to
+705
| useEffect(() => { | ||
| const unsubscribe = window.ade.ai.onPiAuthStatus((event) => { | ||
| setFlow((current) => { | ||
| if (!current || current.providerId !== event.providerId) return current; | ||
| if (event.state === "prompt" && event.prompt) return { ...current, prompt: event.prompt }; | ||
| if (event.state !== "pending" || !event.notice) return current; | ||
| // A URL or device code is the step the user has to act on, so it stays | ||
| // on screen; plain progress lines replace each other. | ||
| return event.notice.url || event.notice.userCode | ||
| ? { ...current, link: event.notice, progress: null } | ||
| : { ...current, progress: event.notice.message }; | ||
| }); | ||
| // A local runtime delivers each status twice (direct IPC broadcast plus | ||
| // the buffered relay), and the second copy can land after the user has | ||
| // started typing. Only a genuinely new prompt clears the field, or the | ||
| // duplicate would erase a half-entered API key. | ||
| if (event.state === "prompt" && event.prompt) { | ||
| setPromptValue((current) => (lastPromptRequestIdRef.current === event.prompt!.requestId ? current : "")); | ||
| lastPromptRequestIdRef.current = event.prompt.requestId; | ||
| } | ||
| }); | ||
| return unsubscribe; |
There was a problem hiding this comment.
If the user navigates away while an API-key or manual-code prompt is pending, this cleanup only removes the listener; the main-process flow and worker continue waiting for up to ten minutes. Reopening Settings cannot recover the card because flow is initialized to null and status events ignore providers without a current flow, leaving the user with an invisible sign-in that must be superseded manually. Cancel the flow on teardown or expose enough active-flow state to reconstruct it.
Useful? React with 👍 / 👎.
Comment on lines
+1685
to
+1692
| guard !rawToolCallQuestionTexts.isEmpty, !wrappedQuestionTexts.isEmpty else { return results } | ||
| return results.filter { item in | ||
| guard case .question(let model) = item, | ||
| let rawTexts = rawToolCallQuestionTexts[model.id] | ||
| else { | ||
| return true | ||
| } | ||
| return rawTexts.isDisjoint(with: wrappedQuestionTexts) |
There was a problem hiding this comment.
When a legitimate bare ask_user call and any unrelated wrapped question use the same normalized text—even in different turns or from different providers—this global set comparison removes the bare call's only answerable card. Conversely, after a Pi wrapper resolves but before its raw tool call receives a result, the wrapper drops out of openIds, so the previously suppressed dead card can reappear. Correlate the raw call with its actual wrapper using turn/source/order metadata instead of question text alone.
Useful? React with 👍 / 👎.
…sk_user dedupe Navigating away from Settings mid-sign-in left the worker waiting for its full budget with no way to reach it, since a reopened Settings cannot adopt a flow it has no record of. It now cancels on teardown. On iOS the raw-tool-call dedupe matched any open question with the same text, which could drop an unrelated provider's only answerable card. It now suppresses a raw ask_user call only when the host's own ask_user gate is what wrapped it, and the test fixture carries the providerMetadata a real Pi request sends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b4c41771c
ℹ️ 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".
| // A second sign-in may already own these, so only this attempt's own | ||
| // state is torn down here. | ||
| if (cancelledProviderRef.current === providerId) cancelledProviderRef.current = null; | ||
| setFlow((current) => (current?.providerId === providerId ? null : current)); |
There was a problem hiding this comment.
When submitting an answer fails while the backend login is still running, the error UI exposes “Try again” for the same provider. That starts a replacement flow, but cancellation lets the original start call reach this finally, where matching only providerId clears the replacement flow as well. The new worker then continues without visible prompts or a usable Cancel control; track an attempt ID/generation and only clear state owned by that attempt.
Useful? React with 👍 / 👎.
Comment on lines
+271
to
+272
| editor(title: string, prefill?: string): Promise<string | undefined> { | ||
| return (context.input as (t: string, p?: string) => Promise<string | undefined>)(title, prefill); |
There was a problem hiding this comment.
When a Pi extension calls ui.editor(title, prefill), this forwards prefill as input's placeholder instead of as the field's initial value. The resulting card is blank and cannot submit the unchanged prefill, so editor-based extension flows lose the document they asked the user to edit. Carry the value through PiSdkUiRequestPayload.defaultValue and initialize the renderer's answer state from it.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
… its attempt ui.editor(title, prefill) forwarded the prefill as a placeholder, so the card opened blank and a dismissal returned nothing — losing the text the extension asked to have edited. The prefill is now the card's starting value, and cancel leaves it unchanged, which is what cancel means for an editor. A retry after a failed prompt submit creates a replacement flow; the superseded attempt's teardown matched only the provider and cleared it, leaving a worker running with no visible prompts. Teardown is keyed to its own attempt now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
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)
apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift (1)
1414-1423: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse request/source metadata to distinguish Pi tool-gate approvals.
The no-option guard rejects optionless
approvalrequests beforeworkPendingQuestionEntry()can preserveallowsFreeform: trueandallowsFreeform: falseexplicitly. Pi’s bash/edit/write gate may send a single selectable action request; rely onrequest.kind/sourceor expected tool names instead ofhasSelectableOptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift` around lines 1414 - 1423, Replace the hasSelectableOptions-based early return in workPendingQuestionEntry() with detection of Pi tool-gate approvals using request kind/source metadata or the expected bash, edit, and write tool names. Preserve the explicit allowsFreeform behavior for these requests, and keep Claude approval requests flowing to the question-card path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift`:
- Around line 1414-1423: Replace the hasSelectableOptions-based early return in
workPendingQuestionEntry() with detection of Pi tool-gate approvals using
request kind/source metadata or the expected bash, edit, and write tool names.
Preserve the explicit allowsFreeform behavior for these requests, and keep
Claude approval requests flowing to the question-card path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ea422570-7c2b-415b-a14b-71fb14fe5246
⛔ Files ignored due to path filters (2)
docs/features/chat/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/configuration-schema.mdis excluded by!docs/**
📒 Files selected for processing (13)
apps/desktop/src/main/services/ai/piAuthService.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/piSdkEventMapper.test.tsapps/desktop/src/main/services/chat/piSdkEventMapper.tsapps/desktop/src/main/services/chat/piSdkUiBridge.test.tsapps/desktop/src/main/services/chat/piSdkUiBridge.tsapps/desktop/src/main/services/config/projectConfigService.test.tsapps/desktop/src/main/services/config/projectConfigService.tsapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/webclient/adapter/misc.tsapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/renderer/components/terminals/cliLaunch.test.ts
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/main/services/chat/piSdkUiBridge.test.ts
- apps/desktop/src/renderer/components/settings/ProvidersSection.tsx
- apps/desktop/src/main/services/ai/piAuthService.ts
- apps/desktop/src/renderer/webclient/adapter/misc.ts
- apps/desktop/src/main/services/chat/piSdkUiBridge.ts
- apps/desktop/src/main/services/chat/piSdkEventMapper.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 628aa6753a
ℹ️ 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".
Comment on lines
+745
to
+752
| const result = await window.ade.ai.piLoginStart({ providerId, ...(method ? { method } : {}) }); | ||
| const cancelled = !result.ok && cancelledProviderRef.current === providerId; | ||
| setOutcome({ | ||
| providerId, | ||
| method: method ?? null, | ||
| state: result.ok ? "ok" : cancelled ? "cancelled" : "error", | ||
| ...(result.ok || cancelled || !result.error ? {} : { error: result.error }), | ||
| }); |
There was a problem hiding this comment.
When “Try again” starts a replacement while the previous piLoginStart promise is still pending—for example after submitting a prompt fails—the superseded promise still calls setOutcome, onSignedIn, and loadProviders. Although attemptId now protects flow teardown, it does not protect these updates, so the old failure can display a stale error and focus another retry button throughout the new sign-in, or a sufficiently delayed response can overwrite the replacement’s final outcome. Gate all attempt-owned updates on the current attempt ID.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
attemptId guarded flow teardown but not the updates after the await, so a superseded start could still report its outcome, call onSignedIn, and refresh providers over the replacement the user had just started. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
…i in the matrix Rebased onto origin/main (P0 #1056, Pi #1054/#1055). - registry's `session.settleSessions` had become an `async` function, which silently converted its argument-validation guard from a synchronous throw into a rejected promise — a contract change for any caller that does not await, caught by registry.test.ts. Only the success path is async now, returned as an explicit promise from a sync body. The success assertion is awaited, because that path genuinely did become asynchronous: the session's monitors have to stop before the settle is written. - runtimeBackgroundWork's switch is now exhaustive over ChatRuntime["kind"] with a `never` check, so a newly landed harness fails to compile until someone classifies it. Pi is listed explicitly: its runtime carries only turn-scoped state (activeTurnId, busy, pendingSteers, activeCompactionId, lease) with no subagent, background-task, or remote-run tracking, and it is absent from SUBAGENT_CAPABILITIES so resolveSubagentCapability already degrades it to the no-op descriptor. Zero is a verified fact about Pi, not an unexamined default. - Settle teardown now runs inside the merged candidateSessionsFor loop, so it targets exactly what the corrected targeting settles: nothing at all when the scope is ambiguous, only declared in-lane sessions when linked, and the bounded sweep minus other PRs' claims otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
…i in the matrix Rebased onto origin/main (P0 #1056, Pi #1054/#1055). - registry's `session.settleSessions` had become an `async` function, which silently converted its argument-validation guard from a synchronous throw into a rejected promise — a contract change for any caller that does not await, caught by registry.test.ts. Only the success path is async now, returned as an explicit promise from a sync body. The success assertion is awaited, because that path genuinely did become asynchronous: the session's monitors have to stop before the settle is written. - runtimeBackgroundWork's switch is now exhaustive over ChatRuntime["kind"] with a `never` check, so a newly landed harness fails to compile until someone classifies it. Pi is listed explicitly: its runtime carries only turn-scoped state (activeTurnId, busy, pendingSteers, activeCompactionId, lease) with no subagent, background-task, or remote-run tracking, and it is absent from SUBAGENT_CAPABILITIES so resolveSubagentCapability already degrades it to the no-op descriptor. Zero is a verified fact about Pi, not an unexamined default. - Settle teardown now runs inside the merged candidateSessionsFor loop, so it targets exactly what the corrected targeting settles: nothing at all when the scope is ambiguous, only declared in-lane sessions when linked, and the bounded sweep minus other PRs' claims otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
…i in the matrix Rebased onto origin/main (P0 #1056, Pi #1054/#1055). - registry's `session.settleSessions` had become an `async` function, which silently converted its argument-validation guard from a synchronous throw into a rejected promise — a contract change for any caller that does not await, caught by registry.test.ts. Only the success path is async now, returned as an explicit promise from a sync body. The success assertion is awaited, because that path genuinely did become asynchronous: the session's monitors have to stop before the settle is written. - runtimeBackgroundWork's switch is now exhaustive over ChatRuntime["kind"] with a `never` check, so a newly landed harness fails to compile until someone classifies it. Pi is listed explicitly: its runtime carries only turn-scoped state (activeTurnId, busy, pendingSteers, activeCompactionId, lease) with no subagent, background-task, or remote-run tracking, and it is absent from SUBAGENT_CAPABILITIES so resolveSubagentCapability already degrades it to the no-op descriptor. Zero is a verified fact about Pi, not an unexamined default. - Settle teardown now runs inside the merged candidateSessionsFor loop, so it targets exactly what the corrected targeting settles: nothing at all when the scope is ambiguous, only declared in-lane sessions when linked, and the bounded sweep minus other PRs' claims otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
…i in the matrix Rebased onto origin/main (P0 #1056, Pi #1054/#1055). - registry's `session.settleSessions` had become an `async` function, which silently converted its argument-validation guard from a synchronous throw into a rejected promise — a contract change for any caller that does not await, caught by registry.test.ts. Only the success path is async now, returned as an explicit promise from a sync body. The success assertion is awaited, because that path genuinely did become asynchronous: the session's monitors have to stop before the settle is written. - runtimeBackgroundWork's switch is now exhaustive over ChatRuntime["kind"] with a `never` check, so a newly landed harness fails to compile until someone classifies it. Pi is listed explicitly: its runtime carries only turn-scoped state (activeTurnId, busy, pendingSteers, activeCompactionId, lease) with no subagent, background-task, or remote-run tracking, and it is absent from SUBAGENT_CAPABILITIES so resolveSubagentCapability already degrades it to the no-op descriptor. Zero is a verified fact about Pi, not an unexamined default. - Settle teardown now runs inside the merged candidateSessionsFor loop, so it targets exactly what the corrected targeting settles: nothing at all when the scope is ambiguous, only declared in-lane sessions when linked, and the bounded sweep minus other PRs' claims otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
…ring (#1059) * feat(sessions): promote live background work into canonical phase A session whose foreground turn ended while its background jobs kept going read as idle everywhere a user glances: the Work-tab dot, the TopBar rollup, the dock badge, and the Lanes agent list all showed nothing while agents were mid-run. The "Background work xN" label existed, but only as a label — it never reached the canonical phase those surfaces derive from. canonicalSessionState now promotes a resting session with live background work back to `running`, and reports WHY via a new `liveness` field (turn / background / monitoring). Every existing consumer of the phase inherits the truth without a special case. - Two-state vocabulary: `monitoring` only when watch loops are the SOLE live work, so "still building" and "just watching CI" read differently. - Classification is a denylist (MONITOR_TASK_TYPES / INERT_TASK_TYPES). Unknown task types count as WORKING — an allowlist silently drops a real subagent the first time an SDK renames a type. - Generalized past Claude: codex background subagents and cursor cloud runs now count too. runtimeBackgroundWork() documents what escapes (detached nohup/setsid spawns, user-owned terminals, opencode/droid/pi). - Liveness stays in-memory and empty after restart: orphaned background work is not live work. - A failed, stopped, settled, or hand-raised session still outranks lingering liveness, so a stale "Working" can never mask a failure. - Subagent toolbar badge counts RUNNING subagents, not total tracked — a finished fleet no longer wears a number that only ever grew. - TerminalAttentionSummary.byLaneId removed deliberately: it had no consumer, and laneListSnapshotService already owns the per-lane rollup the Lanes tab and mobile both read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(sessions): settle stops the machinery it claims to conclude Settle was a pure column write. The row went quiet and everything the session had started kept going — background shells held ports, subagent fleets kept spending tokens, and scheduled work woke the thread hours after the user had declared it done. Archive had the mirror problem: it released the lane's port lease and proxy route while the lane's processes were still bound to those ports, and an archived lane is filtered out of every surface that could have shown the user what to stop. Settle now runs a shared teardown (sessionMachineryTeardown.ts) before the lifecycle write, so a settle can never report success while its monitors are still armed: - pauses the session's scheduled work — pauses, not cancels, so an unsettle brings hand-made schedules back rather than having silently deleted them, - calls the new agentChatService.stopBackgroundWork, which stops every live child BEFORE the parent (stopping only the parent leaves the fleet running and untracked, which is how a "stopped" agent keeps spending), - keeps TERMINAL PANES OPEN. An agent's background shell is thread background work; a pane the user opened is theirs, and closing it on settle would destroy scrollback nobody asked to lose, - leaves an ACTIVE foreground turn alone — its subagents are work the user can see happening, and the row un-settles on its own activity anyway, - is best-effort throughout: a provider that cannot be reached delays nothing and blocks nothing. Wired into every settle entry point: the single/bulk ADE actions, the sessions.settle / settleMany IPC handlers, the session.settle* sync commands, and PR-merge auto-settle — which bypasses settlement blockers and is therefore the path most likely to file a session that is still running something. It composes with that service's session targeting rather than replacing it. laneService.archive is now async and stops the lane's chats, PTYs, watchers and auto-rebase through a shared stopLaneRuntimeWork before the status write, so the port lease its callers release immediately afterwards is released after the processes are gone. archiveAndReclaim uses the same helper; delete keeps its runStep version because the delete dialog reports each step. What escapes is documented rather than pretended away: processes an agent detached with nohup/setsid/disown leave ADE's tree entirely, and Codex background subagents are reported but expose no stop control. No new kill logic is introduced — teardown delegates to ptyService/agentChatService disposal, which already route through the Windows-correct tree-kill helpers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): quality pass — honest teardown counts, no zero-record churn Findings from the /quality dual-review on this branch, all verified against the real code paths before applying: - stopBackgroundWork reported the live work it FOUND as the work it stopped, so a Codex session (no per-subagent stop control) or a Cursor session with no cloud agent id claimed a teardown that never happened. It now reports the measured DROP in live work across the call, which is 0 for those cases by construction and can never over-report. - A Claude background task ADE could not stop was closed as "stopped". It now settles as failed with the reason, matching closeOpenClaudeBackgroundTasks — both close the row, only one claims ADE did the stopping. - getSessionSummary emitted backgroundWork: {0,0} on every chat summary. Now omitted when nothing is live, like every other optional field there. - NO_BACKGROUND_WORK was a shared mutable object handed out by reference; frozen. - laneAgents' background hint guarded on a stringly-typed status that chat and CLI summaries spell differently. Callers now pass turnActive explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): keep bulk-settle validation throwing synchronously + Pi in the matrix Rebased onto origin/main (P0 #1056, Pi #1054/#1055). - registry's `session.settleSessions` had become an `async` function, which silently converted its argument-validation guard from a synchronous throw into a rejected promise — a contract change for any caller that does not await, caught by registry.test.ts. Only the success path is async now, returned as an explicit promise from a sync body. The success assertion is awaited, because that path genuinely did become asynchronous: the session's monitors have to stop before the settle is written. - runtimeBackgroundWork's switch is now exhaustive over ChatRuntime["kind"] with a `never` check, so a newly landed harness fails to compile until someone classifies it. Pi is listed explicitly: its runtime carries only turn-scoped state (activeTurnId, busy, pendingSteers, activeCompactionId, lease) with no subagent, background-task, or remote-run tracking, and it is absent from SUBAGENT_CAPABILITIES so resolveSubagentCapability already degrades it to the no-op descriptor. Zero is a verified fact about Pi, not an unexamined default. - Settle teardown now runs inside the merged candidateSessionsFor loop, so it targets exactly what the corrected targeting settles: nothing at all when the scope is ambiguous, only declared in-lane sessions when linked, and the bounded sweep minus other PRs' claims otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(sessions): drop reference to the settlement-blocker helper main removed Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(sessions): consolidate end-of-life teardown coverage, pin the liveness contract Pruned/consolidated: the sessions folder had 5 test files against a 3-file budget after this branch added one. deleteTerminalSession.test.ts and sessionMachineryTeardown.test.ts covered the same contract — what happens to a session's machinery at end of life — split across files for dependency reasons, not behavioral ones. Merged into sessionTeardown.test.ts (12 tests), returning the folder to the 4 files it had before this branch. No tests lost. Added, where the failure mode is actually reachable: - agentChatService.test.ts: drives a real background_tasks_changed level and asserts the summary splits it working/monitoring by denylist (local_bash -> monitoring, local_agent AND an unrecognised type -> working), that a live turn makes stopBackgroundWork decline rather than kill it, and that the record is omitted once the level drains rather than riding along as a zero. - laneAgents.test.ts: a resting agent stays live while its background work is, sorts working ahead of monitoring ahead of idle, reports what is still running instead of the finished turn's stale preview, and counts a split-less (older-peer) summary as working rather than passive. Parity: corrected stale prose in attentionItemBuilder that still described the promotion as a sessionStatusPresentation label override rather than a sessionCanonicalState phase promotion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): give the settle pause an exact undo, and stop calling builds monitors Both P1s from Greptile on #1059, verified against the code before fixing. 1. Unsettle left schedules paused forever. The scheduler's session pause is PERSISTED, and settle took one while every unsettle path only cleared lifecycle columns — so a settled-then-unsettled chat kept its monitors, crons, and scheduled turns disabled indefinitely. A durable pause with no undo is just a slower deletion, and the docs already promised the undo. The scheduler now records which sessions settle paused (`settlePausedSessionIds`, persisted beside the pause it annotates). `setSessionPausedForSettle` claims a pause only when the user had not already taken one; `resumeSessionPausedForSettle` puts back exactly that and nothing else; an explicit user toggle drops settle's claim in either direction so a later unsettle cannot override their choice. Every unsettle entry point — registry single/bulk, both IPC handlers, both sync commands — now runs `resumeSettledSessionMachinery`, mirroring the settle wiring. Background work is deliberately not restarted: ADE cannot re-spawn a shell it stopped, and pretending otherwise is worse than leaving it quiet. 2. Generic backgrounded shells were classified as monitors. `local_bash` / `shell` / `background` / `bash` are how a provider says "the agent backgrounded a command" — a `tail -f` and a 20-minute `npm run build` arrive under the same type. Listing them labelled every background build "Monitoring", telling the user nothing was being produced while it was. Mixed is unknown, and this classifier's own stated rule is that unknown is working; including them contradicted it. MONITOR_TASK_TYPES is now only `monitor` / `monitor_mcp` — types whose whole job is to watch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(prs): update the PR-merge teardown mock for the settle-scoped pause API Missed when setScheduledWorkPaused was split into the settle-scoped setScheduledWorkPausedForSettle; the user-facing toggle keeps its old name and its own callers, which is why only the teardown mocks move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): resume at the settle-clearing write, stop bypasses and false stops Three P1s from the #1059 re-review — Greptile and Codex independently found the first, which is the highest-signal one. 1. Activity-driven unsettle kept schedules paused (Greptile + Codex). Wiring the resume into each unsettle caller missed the most common unsettle of all: a user sending the next message, which clears settled_at through clearTurnStartMarkers. The chat went active while its monitors, crons, and scheduled turns stayed paused across restarts. The resume now hangs off sessionService's new onSettleCleared hook, fired by every route that clears the column — unsettleSession, unsettleSessions, and clearTurnStartMarkers. Per-caller wiring in the registry, both IPC handlers, and both sync commands is deleted as redundant, so a future caller cannot reintroduce the gap. Settle itself stays explicit per entry point because its teardown has to finish before the write. 2. CTO operator settle bypassed teardown entirely (Codex). createCtoOperatorTools called sessionService.settleSession directly, so a CTO-filed chat kept its schedules armed and its background fleet spending. It now runs the shared teardown first, like every other settle entry point. 3. A failed stop still closed the task row (Codex). When stopTask was missing, timed out, or rejected, the emitted terminal row dropped the task from liveBackgroundTaskIds — which is exactly what the caller measures the stop against, so a stop that did not happen was counted as one that did. The task now stays LIVE on failure; the SDK's next authoritative level drains it if it really ended. Under-reporting is the only safe direction here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * revert(sessions): settle no longer pauses scheduled work Cutting a slice of this PR rather than patching it a fourth time. Greptile's latest round found that `settled_at` is cleared from SEVEN places in sessionService, not the three the onSettleCleared hook covered — including `setLastOutputPreview`, the hot PTY-output path. It also found a TOCTOU where a fire-and-forget resume overlapping a later settle releases the newer pause. That is the third consecutive review round to find a defect in the scheduled-work pause specifically, each in a route the previous fix had not traced. The pause is persisted, so it needs a COMPLETE undo or it silently deletes the user's own monitors and crons. Covering the remaining routes means either a pre-read or a split statement on a per-output-chunk path, plus serializing pause/resume per session — real cost and more machinery, for the part of this change that keeps producing bugs. So settle now stops background work only: background shells, subagent fleets, cursor cloud runs. That was the unmanaged, invisible thing the change was actually about, and it has been stable since the second iteration. Scheduled work in ADE is already visible and user-manageable (scheduledWork / nextWakeAt on the summary, a per-session pause toggle), and canonicalSessionState already handles a settled chat woken by a schedule: green while the turn streams, then re-settled. Leaving it running is the pre-existing, deliberate behavior. Removed: settlePausedSessionIds and the two scheduler methods, setScheduledWorkPausedForSettle, sessionService's onSettleCleared hook and its wiring in main/bootstrap, and resumeSettledSessionMachinery. Kept: the CTO operator settle now routing through shared teardown, and unstoppable Claude tasks staying live rather than being reported as stopped. Stopping scheduled work on settle remains a reasonable feature; it needs its own change with the full clear-path inventory up front, not a bolt-on to this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): stop detached work mid-turn, wire RPC CTO teardown, drop the unhonest count Three P1s from Codex on 66e7dba. 1. Settling during an active turn tore down nothing. stopBackgroundWork returned early on a live turn while the caller still wrote settled_at, so PR auto-settlement, the CTO tool, and RPC callers left background shells running under a row that went quiet when the turn ended. The carve-out was too wide: a turn's own SUBAGENTS are work the user can see and are still spared, but its DETACHED background work outlives the turn by construction and an explicit settle is the user saying they are done with it. That now stops mid-turn; only stopActiveClaudeSubagents and cursor cloud-run cancellation are skipped while a turn runs. 2. The ADE RPC operator bridge never received the teardown control. adeRpcServer's createCtoOperatorTools construction had agentChatService in scope but did not pass it, so the CTO settle tool over the desktop socket filed rows without stopping their background work — the in-process path was fixed and the daemon path was not. Same bug class as every other 'wired in-process, missing from the daemon' regression. 3. The stopped count could not be kept honest. stopActiveClaudeSubagents routes through closeOpenClaudeBackgroundTasks, which closes a shell it FAILED to stop, so any before/after measurement silently counted unstoppable work as stopped. This is the third round to land on that number. It had no consumer anywhere, so it is gone rather than approximated; skippedActiveTurn is the remaining, checkable signal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): never close a background task whose stop was not confirmed Two P1s from Codex on cd53e34. 1. An unconfirmed stop still closed the task row. closeOpenClaudeBackgroundTasks emits a terminal 'failed' update when stopTask is absent, times out, or rejects, and that removes the task from liveBackgroundTaskIds — which is exactly what runtimeBackgroundWork derives the row's user-visible liveness from. The session therefore went quiet over a shell that may still be running: the precise lie this whole change exists to remove. A stop we attempted and could not confirm now leaves the task LIVE and logs claude_background_stop_unconfirmed; the SDK's next authoritative level drains it if it really ended. Scoped to failed stop ATTEMPTS, so the turn-end close path ('completed', which attempts nothing) is unchanged. 2. Teardown raced the lifecycle write. Provider stop calls take seconds, and a user starting a turn inside that window runs clearTurnStartMarkers against a settle marker that does not exist yet — after which settleTerminalSession wrote settled_at over the freshly-active session, filing a live turn as settled. The settle now snapshots lastActivityAt before teardown and refuses the write if it moved. Real activity outranks a settle request that predates it. It reports true rather than false: the row exists and the request was handled, it simply woke, and false would surface a spurious 'not found'. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * revert(sessions): settle no longer stops background work Cutting the second and last slice of settle teardown. Six review rounds, every one of them finding a real defect in this specific mechanism: - an unsettle path that skipped the resume (x3, each a route the previous fix had not traced), - settling mid-turn tearing down nothing while the caller still wrote the marker, - the RPC operator bridge never receiving the teardown control, - a stop count that could not be kept honest, - and finally an activity guard that reads lastActivityAt — which is backed by last_output_at, a column clearTurnStartMarkers never writes. The guard I added last round provably cannot fire. The shape is now unambiguous. Teardown is async; settled_at is written and cleared from seven places. A teardown-then-write settle races real activity, and the failure is not one-sided: a user starting a turn during a provider stop call gets their background work stopped AND no settle. Every guard against it either read a column turn-start does not update, or had to be repeated identically at each settle entry point (settleTerminalSession, bulk registry, both IPC handlers, both sync commands, PR auto-settlement, the CTO tool). Doing this correctly needs a synchronous lifecycle revision that teardown can be serialized against — a different change, designed as one, not a wrapper around the existing write. Shipping the half-working version is worse than the status quo, which is the one thing the brief specifically warned about. What ships instead is the half that has been stable since iteration 2 and is what a user actually sees: live background work promoted into the canonical phase across every glanceable surface, the working/monitoring denylist, cross-runtime generalization, running-count subagent badges, and the archive port-lease ordering fix — archive remains the lifecycle path that does stop processes, and its ordering bug is fixed. Removed: sessionMachineryTeardown, stopBackgroundWork, the activity guard, and the teardown calls in every settle entry point. runtimeBackgroundWork stays; it is the surfacing half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): a settled row still reports live background work Greptile and Codex both landed on the same branch, and cutting settle teardown made them right: the settled branch suppressed background-work liveness, and its comment justified that with 'settle now tears the session's machinery down' — which stopped being true when the teardown was removed. A settled session can now legitimately still own a live background shell, subagent, or cloud run. The PHASE stays settled: a declared settle is a human judgment call, and re-lighting the row would let a stubborn monitor out-vote the user's explicit 'this is done'. But liveness now reports the truth, so a surface that wants to show 'settled, but something is still running' can. Hiding it behind the phase is the same lie this module exists to prevent, just at the other end of the lifecycle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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 by CodeRabbit
Greptile Summary
The PR integrates Pi authentication and agent-chat support across desktop, web, CLI, and iOS.
Confidence Score: 3/5
The PR is not yet safe to merge because tightening permissions on an active Pi chat can leave the existing worker’s more permissive tool policy in effect for subsequent steers.
The session update records the stricter permission mode but does not invalidate Pi, and the active-turn steer path calls the existing worker directly without reaching the policy-key restart check.
Files Needing Attention: apps/desktop/src/main/services/chat/agentChatService.ts
Important Files Changed
Sequence Diagram
Reviews (6): Last reviewed commit: "fix(pi): ignore a superseded sign-in att..." | Re-trigger Greptile
Context used (4)
ade codeTUI