fix(pi): make Pi chat and the Pi CLI usable again - #1072
Conversation
@codex was never in the ship loop's expected-review-bot set — Phase 1 waits on Greptile and CodeRabbit — so the ping only added a bot whose comments the loop then had to reason about. Retire it the same way @copilot was: as a written prohibition in both the skill and the playbook, so future agents inherit it instead of rediscovering the preference. An ordinary push now posts no review comment at all. The >250-file @greptile/@CodeRabbit pings are unchanged.
Pi chat and tracked Pi CLI terminals were both unusable. Two separate causes,
one shared root: ADE and Pi disagreed about where Pi's sessions live.
Session store. ADE resolved a store root Pi never writes to, then validated a
file Pi had not written yet (Pi does not create the JSONL until the first
assistant message). Both are fixed, and the resolution now mirrors Pi's own
precedence: --session-dir > PI_CODING_AGENT_SESSION_DIR > profile settings.json
> <agentDir>/sessions. A checkout's .pi/settings.json is deliberately ignored:
that file belongs to a repository ADE has not vouched for, and honouring it
would let any clone redirect where ADE authorizes and leases sessions.
CLI resume. `defaultResumeCommandForTool("pi")` produced `pi --continue`, which
means "the most recent session for this directory" — and since ADE chat and the
tracked CLI now share one native store, that could be another terminal's session
or a chat's. A fresh terminal reopened a four-day-old transcript this way. Pi is
now resumed only by a session id ADE captured for that terminal.
Ownership. One store shared by two writers needs both a live lock and a durable
claim: .ade-lease (removed on release) and .ade-owner (never removed).
Also: sign-in no longer cancels on unmount or discards its success event, the
Providers panel is rebuilt on the OpenCode pattern with local model servers that
are never offered an API key, and per-provider chat accents land for opencode,
cursor, droid and pi. Claude and Codex keep their exact shipped bubbles.
Windows parity is covered on the native runner, and iOS mirrors the accents.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesPi runtime and session coordination
Pi provider authentication and settings
Provider experience and platform styling
CLI, IPC, and operations
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 |
windows-foundation caught this on the first run, which is the point of having put piSession.test.ts on that job. The store canonicalizes with realpathSync.native so an 8.3 short name and its long spelling compare equal; the tests asserted against plain realpathSync, which on Windows preserves the short form. Expectations now use the same call the code does: expected C:\Users\RUNNER~1\... (plain realpathSync) received C:\Users\runneradmin\... (realpathSync.native) The production path was already correct — only the test expectation was wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/settings/ChatAppearancePreview.test.tsx (1)
77-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert provider-specific preview behavior.
The count assertion does not prove that the sixth shell is Pi. The
toContainassertion also allows46%for every provider, so an incorrect accent assignment can pass. Add a named regression test such asit("uses the widened border mix for Cursor and Pi")and assert each provider identity with its expected mix.🤖 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/renderer/components/settings/ChatAppearancePreview.test.tsx` around lines 77 - 97, Strengthen ChatAppearancePreview tests by adding a named regression test that identifies each rendered provider shell and verifies its expected --chat-user-border-accent-mix value, specifically ensuring Cursor and Pi use the widened mix. Do not rely on aggregate shell counts or toContain; use the provider identity attributes exposed by each section and assert the exact mix per provider.Source: Coding guidelines
🧹 Nitpick comments (11)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
1633-1657: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
piPermissionModeToPickerValueinto the sharednativeLaunchControls.tsmodule.The mapping logic is correct: the four pass-through values match
AgentChatOpenCodePermissionMode, anddefault/autofall back toedit. This round-trips cleanly with the new Pi branch innativeLaunchControls.ts, which writespermissionMode: controls.opencodePermissionModedirectly.This diff's stated goal is to centralize provider-specific native-control conversions in
nativeLaunchControls.ts(seesummarizeNativeControls,applyUnifiedPermissionToNativeControls, each with dedicated unit tests innativeLaunchControls.test.ts).piPermissionModeToPickerValueis the same kind of conversion but stays local to this component, with no isolated test coverage. Moving it alongside its sibling conversions keeps the pattern consistent and makes it directly testable, the same way the Pi branch fix innativeLaunchControls.tswas.♻️ Suggested move (illustrative)
-// apps/desktop/src/renderer/components/chat/AgentChatPane.tsx -function piPermissionModeToPickerValue( - mode: AgentChatPermissionMode | undefined, -): AgentChatOpenCodePermissionMode | undefined { - if (mode === "plan" || mode === "edit" || mode === "full-auto" || mode === "config-toml") return mode; - if (mode === "default" || mode === "auto") return "edit"; - return undefined; -} +// apps/desktop/src/renderer/lib/nativeLaunchControls.ts +export function piPermissionModeToPickerValue( + mode: AgentChatPermissionMode | undefined, +): AgentChatOpenCodePermissionMode | undefined { + if (mode === "plan" || mode === "edit" || mode === "full-auto" || mode === "config-toml") return mode; + if (mode === "default" || mode === "auto") return "edit"; + return undefined; +}🤖 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/renderer/components/chat/AgentChatPane.tsx` around lines 1633 - 1657, Move piPermissionModeToPickerValue from AgentChatPane into the shared nativeLaunchControls module, preserving its existing mapping for plan, edit, full-auto, config-toml, default, and auto. Update the component to import and use the shared helper, and add or extend nativeLaunchControls tests to cover the conversion.apps/desktop/src/renderer/components/settings/PiProvidersPanel.tsx (1)
800-831: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the derived provider rows and the model-id index.
This block runs on every render. The panel re-renders on each keystroke in the search field and on each Pi status notice during a sign-in. The loop at Lines 824-831 calls
decodePiRegistryIdfor every entry ininstallation.availableModelIdseach time, andmodelIdsByProvideris consumed only byPiProviderDetailModalat Line 903.Wrap
allRowsandmodelIdsByProviderinuseMemo.ProvidersSection.tsxalready usesuseMemofor the equivalent OpenCode catalog derivations, so this keeps the existing pattern.♻️ Proposed refactor
- const allRows = buildPiProviderRows( - installation.providers.filter((provider) => provider.configured), - signableProviders, - ); + const allRows = React.useMemo( + () => buildPiProviderRows( + installation.providers.filter((provider) => provider.configured), + signableProviders, + ), + [installation.providers, signableProviders], + );- const modelIdsByProvider = new Map<string, string[]>(); - for (const registryId of installation.availableModelIds) { - const decoded = decodePiRegistryId(registryId); - if (!decoded) continue; - const existing = modelIdsByProvider.get(decoded.providerId); - if (existing) existing.push(decoded.modelId); - else modelIdsByProvider.set(decoded.providerId, [decoded.modelId]); - } + const modelIdsByProvider = React.useMemo(() => { + const map = new Map<string, string[]>(); + for (const registryId of installation.availableModelIds) { + const decoded = decodePiRegistryId(registryId); + if (!decoded) continue; + const existing = map.get(decoded.providerId); + if (existing) existing.push(decoded.modelId); + else map.set(decoded.providerId, [decoded.modelId]); + } + return map; + }, [installation.availableModelIds]);Note that
signableProvidersisproviders ?? [], which creates a new array whenprovidersisnull. Derive it withuseMemotoo, or key the memo onproviders.As per coding guidelines: "Preserve existing application patterns before introducing new abstractions."
🤖 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/renderer/components/settings/PiProvidersPanel.tsx` around lines 800 - 831, Memoize the derived provider data in the panel: use useMemo for signableProviders, allRows, and modelIdsByProvider, with dependencies covering their source values and preserving correct recalculation when providers, installation data, or signableProviders change. Keep the existing filtering and decode logic unchanged, and follow the established ProvidersSection.tsx memoization pattern.Source: Coding guidelines
apps/desktop/src/renderer/components/settings/ProvidersSection.tsx (1)
213-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the orphaned doc comment.
The local
panelhelper moved toproviderSectionPrimitives.tsx. Its doc comment stayed behind and now sits directly aboveprettifyProviderId, which it does not describe.♻️ Proposed fix
-/** Squared bordered surface — the shared "ledger" panel used across this section. */ function prettifyProviderId(id: string): string {🤖 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/renderer/components/settings/ProvidersSection.tsx` around lines 213 - 214, Remove the orphaned doc comment immediately before prettifyProviderId; the comment describes the relocated panel helper and should not remain attached to the formatting function.apps/desktop/src/main/services/ai/piInstallation.ts (1)
412-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe added
localfallback at Line 415 is unreachable.
runtimeAuthTypealready returnsfallback?.authType ?? nullfor any fallback type other than"unknown". WhenfallbackProvider.authType === "local",runtimeAuthTypereturns"local", so the??branch never runs. WhenruntimeAuthTypereturnsnull, the fallback type is"unknown",null, or absent, so the=== "local"test is always false.Either drop the redundant branch, or move the
localpreference insideruntimeAuthTypeif the intent is forlocalto survive an"unknown"runtime type.♻️ Proposed simplification
- // A loopback provider stays local only when Pi's runtime has no auth - // type of its own to report. - const authType = runtimeAuthType(runtime, fallbackProvider) - ?? (fallbackProvider?.authType === "local" ? "local" as const : null); + const authType = runtimeAuthType(runtime, fallbackProvider);🤖 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/ai/piInstallation.ts` around lines 412 - 417, Remove the redundant local fallback from the authType assignment near runtimeAuthType, since runtimeAuthType already preserves fallbackProvider.authType when it is local. Keep the existing authMethods derivation and runtime behavior unchanged.apps/desktop/src/renderer/components/settings/piProviderRow.ts (1)
39-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe doc comment describes a mapping the implementation does not perform.
The comment states that a named lookup keeps the id-namespace assumption in one place, instead of a bare index that degrades silently. The body is a bare index into
connections. Any Pi provider id that happens to match an ADE runtime key resolves, and every other id returnsnullsilently — the exact behavior the comment says it prevents. Either encode the two agreed ids explicitly, or correct the comment.♻️ Proposed fix (explicit map)
-/** - * ADE's local-server probe for a Pi provider, or `null` when ADE has none. - * - * The probe map is keyed by ADE's own provider ids while the row carries Pi's, - * and the two namespaces only happen to agree on `ollama` and `lmstudio`. A - * named lookup keeps that assumption in one place instead of leaving a bare - * index to degrade silently into "no detection" for anything else. - */ +/** + * ADE's local-server probe for a Pi provider, or `null` when ADE has none. + * + * The probe map is keyed by ADE's own provider ids while the row carries Pi's. + * The two namespaces agree only on `ollama` and `lmstudio`, so the mapping is + * listed here rather than assumed for every id. + */ +const ADE_RUNTIME_ID_BY_PI_PROVIDER_ID: Record<string, string> = { + ollama: "ollama", + lmstudio: "lmstudio", +}; + export function runtimeConnectionForPiProvider( connections: AiRuntimeConnections, piProviderId: string, ): AiRuntimeConnectionStatus | null { - return connections[piProviderId.trim().toLowerCase()] ?? null; + const runtimeId = ADE_RUNTIME_ID_BY_PI_PROVIDER_ID[piProviderId.trim().toLowerCase()]; + if (!runtimeId) return null; + return connections[runtimeId] ?? null; }🤖 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/renderer/components/settings/piProviderRow.ts` around lines 39 - 52, Update runtimeConnectionForPiProvider so it explicitly maps Pi provider ids to the corresponding ADE runtime keys, limiting lookups to the agreed ollama and lmstudio mappings; return null for all other ids. Keep the function’s existing null fallback and align the doc comment with this explicit mapping behavior.apps/desktop/src/main/services/ai/piAuthService.test.ts (1)
289-327: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for a cancel that lands while the worker is still being acquired.
claimProviderGenerationexists to stop a start that has not registered a flow yet. The new tests all cancel afterawait flush(), so a flow always exists and the generation-mismatch branch never runs. A regression that drops the generation claim would leak an acquired worker and would still pass this suite. Add a test that holdsacquireWorkerpending, callscancelPiLogin, then resolves the acquisition, and assertsreleaseis called and the start resolves as superseded.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/ai/piAuthService.test.ts` around lines 289 - 327, Add a named regression test covering cancellation during worker acquisition: keep acquireWorker pending, start Pi login, call cancelPiLogin before acquisition resolves, then resolve the worker and verify its release method is called and the start resolves with the superseded error without announcing an outcome. Use the existing test helpers such as createFakeWorker, installWorker, startPiLogin, and cancelPiLogin, and ensure this exercises the claimProviderGeneration mismatch path rather than an already-registered flow.Source: Coding guidelines
apps/desktop/src/renderer/components/settings/providerSectionPrimitives.tsx (1)
286-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
offsetParentin the focus-trap filterWhen
ProviderDetailDialogis tested with jsdom,offsetParentisnullfor its descendants because jsdom has no layout engine. The filter then removes every descendant, so tests exercise only the dialog fallback instead of Tab cycling. Use layout-independent checks such as[hidden],[aria-hidden="true"],disabled, and negativetabIndex, or stub the layout API in the test. Add coverage for first/last focus cycling.🤖 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/renderer/components/settings/providerSectionPrimitives.tsx` around lines 286 - 291, Update the focusable-element filter in the focus-trap logic around ProviderDetailDialog to avoid offsetParent, using layout-independent visibility and focusability checks such as hidden/aria-hidden, disabled, and negative tabIndex. Preserve first/last fallback behavior, and add coverage verifying Tab cycling from the last element to the first and reverse cycling from the first to the last.apps/desktop/src/main/services/chat/piSdkWorker.ts (1)
305-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the containment predicate instead of duplicating it.
Lines 313-314 reimplement
pathWithinDirectoryfromapps/desktop/src/main/services/chat/piSessionStore.tsLines 106-110, clause for clause. This file already imports from that module at Line 19. Two copies of a security-relevant predicate can diverge: a correction applied to one copy leaves the other wrong, and the wrong one is the authorization boundary for the worker.Export
pathWithinDirectoryfrompiSessionStore.tsand use it here.The comment at Line 308 also names
piSessionLease. The.nativerealpath rationale now lives inpiSessionStore.tsLines 148-155. Update the reference.♻️ Proposed shared helper
In
apps/desktop/src/main/services/chat/piSessionStore.ts:-function pathWithinDirectory(filePath: string, directoryPath: string): boolean { +export function pathWithinDirectory(filePath: string, directoryPath: string): boolean {In this file:
-import { piSessionHeaderMatchesCwd, readPiSessionHeader } from "./piSessionStore"; +import { pathWithinDirectory, piSessionHeaderMatchesCwd, readPiSessionHeader } from "./piSessionStore";function sessionFileIsAuthorized(filePath: string, sessionRoot: string | null): boolean { if (!sessionRoot) return true; try { - // `.native` on both sides, matching piSessionLease: on Windows the JS + // `.native` on both sides, matching piSessionStore: on Windows the JS // realpath keeps 8.3 short names and junction casing, so one spelling of a // directory fails containment against another spelling of itself. const resolvedFile = fs.realpathSync.native(filePath); const resolvedDir = fs.realpathSync.native(sessionRoot); - const relative = path.relative(resolvedDir, resolvedFile); - return relative === "" || (!relative.startsWith(`..${path.sep}`) && relative !== ".." && !path.isAbsolute(relative)); + return pathWithinDirectory(resolvedFile, resolvedDir); } catch { return false; } }🤖 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 305 - 318, Export the existing pathWithinDirectory helper from piSessionStore.ts and replace the duplicated containment logic in sessionFileIsAuthorized with that shared helper. Preserve the current authorization behavior, and update the nearby comment to reference piSessionStore.ts as the source of the .native realpath rationale rather than piSessionLease.apps/desktop/src/main/services/chat/piSessionStore.ts (1)
228-246: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRequire
sessionRootinresolvePiSessionFile.When
sessionFileis supplied withoutsessionRoot, the resolver skips store-containment checks. All production callers passsessionRoot, so make it required and update the test calls atpiSession.test.ts:75andpiSession.test.ts:290.🤖 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/piSessionStore.ts` around lines 228 - 246, Require a non-empty sessionRoot in resolvePiSessionFile before resolving an explicit sessionFile, returning null when it is absent; retain the canonical path and containment validation using that root. Update the test calls in piSession.test.ts at the referenced cases to provide sessionRoot.apps/desktop/src/main/services/pty/ptyService.ts (1)
1145-1158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
exportafter the JSDoc block.
exportis placed before the doc comment, so the comment no longer attaches toPI_CONTINUATION_FLAG_REfor editors and API docs.♻️ Proposed fix
-export /** +/** * Pi's continuation flags. `-r` is Pi's interactive session *picker* and `-c` * is "continue the most recent session" — neither is a resume ADE can target, * so both are stripped when ADE rebuilds a launch. */ -const PI_CONTINUATION_FLAG_RE = /(?:^|\s)(?:--continue|-c|-r)(?:\s|$)/iu; +export const PI_CONTINUATION_FLAG_RE = /(?:^|\s)(?:--continue|-c|-r)(?:\s|$)/iu;🤖 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/pty/ptyService.ts` around lines 1145 - 1158, Move the export modifier from before the JSDoc block to the PI_CONTINUATION_FLAG_RE declaration, placing the comment immediately above the exported constant so editors and API documentation associate it correctly. Leave stripPiContinuationArgs unchanged.apps/desktop/src/main/services/pty/ptyService.test.ts (1)
185-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep
fileStatsconsistent inunlinkSyncandwriteFileSync.
unlinkSyncdeletesfileContentsbut leavesfileStats, andwriteFileSyncsetsfileContentswithout updatingfileStats.promises.statandpromises.opentreat a stalefileStatsentry as an existing file, so a deleted path can still reportEEXISTforwx.♻️ Proposed fix
unlinkSync: vi.fn((p: string) => { fileContents.delete(p); + fileStats.delete(p); existsSyncResults.set(p, false); }), writeFileSync: vi.fn((p: string, data: unknown) => { fileContents.set(p, String(data)); + fileStats.set(p, { ...fileStats.get(p), size: Buffer.byteLength(String(data), "utf8") }); existsSyncResults.set(p, true); }),🤖 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/pty/ptyService.test.ts` around lines 185 - 204, Update the mock filesystem handlers in the test setup: make unlinkSync remove the path from fileStats, and make writeFileSync create or refresh its fileStats entry alongside fileContents. Keep promises.stat and promises.open behavior consistent so deleted paths no longer appear to exist and newly written paths are tracked.
🤖 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/desktop/src/main/services/__tests__/piSdk.integration.test.ts`:
- Around line 369-394: Update the installedPiArgs test helper to accept an
optional sessionRoot override, defaulting to fixture.sessionRoot, and pass
configured for this test’s connection setup. Ensure the worker receives
configured as sessionRoot and keep classifyPiSessionFile using the same
configured value so both paths reflect the production configuration.
In `@apps/desktop/src/main/services/ai/piInstallation.ts`:
- Around line 249-262: Update the comments around authType in the provider
classification block so they consistently state that authInfo.type takes
precedence over loopback detection. Change the final authType fallback to
classify as "local" only when localBaseUrl is present, removing the non-loopback
provider.baseUrl condition while preserving the existing api-key and null
fallbacks.
In `@apps/desktop/src/main/services/chat/piSession.test.ts`:
- Around line 459-467: Use actual newline characters in both child.stdin.write
calls within the test cleanup and release flow, replacing the escaped
“release\n” literals while leaving the childScript template literal escaping
unchanged.
In `@apps/desktop/src/main/services/chat/piSessionOwnership.ts`:
- Around line 20-22: Update ownerPathFor to canonicalize existing session files
with fs.realpathSync.native, falling back to path.resolve when the file does not
yet exist. Ensure readPiSessionOwner and piSessionIsAdoptableByTerminal use the
canonical sidecar path, and add a regression test covering ownership written
through one spelling and read through an aliased spelling.
In `@apps/desktop/src/main/services/chat/piSessionStore.ts`:
- Around line 126-138: Update readPiSessionHeader to read only a bounded prefix
of each session file instead of using fs.readFileSync on the complete file.
Extract the first line from that prefix, and add a fallback that handles headers
whose newline is not present within the initial bounded read while avoiding
loading the full transcript; preserve the existing JSON parsing and validation
behavior.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 5548-5549: Update the isContinueLaunch detection near
PI_CONTINUATION_FLAG_RE so it evaluates actual CLI argv tokens rather than the
rendered startupCommand containing the user prompt. Reuse the parsed launch
arguments or exclude the appended prompt before testing, while preserving
detection for genuine standalone continuation flags in initialResumeCommand and
startupCommand.
- Around line 3899-3904: Move the initialInputCancel cleanup outside the
initialInputTimer guard in closeEntry at
apps/desktop/src/main/services/pty/ptyService.ts#L3899-L3904, then apply the
same change in dispose at
apps/desktop/src/main/services/pty/ptyService.ts#L7593-L7598, correcting
indentation there. Clear the cancel hook unconditionally before conditionally
clearing the timer.
- Around line 6289-6325: Update closeEntry and dispose to invoke
entry.initialInputCancel?.() independently of entry.initialInputTimer, so the
waitForPtyQuiet listener is unsubscribed immediately when the PTY closes.
Preserve existing timer cancellation behavior for paths that still use
initialInputTimer.
In `@apps/desktop/src/renderer/components/settings/PiProviderDetailModal.tsx`:
- Around line 60-73: Update the sign-in transition triggered by onStartSignIn in
PiProviderDetailModal so focus moves into the newly rendered flow card when flow
becomes non-null, targeting its heading or Cancel button. Follow the existing
focus-management pattern used for prompts and retry handling, ensuring focus is
set after the flow card mounts instead of returning to the unmounted provider
tile.
In `@apps/desktop/src/renderer/components/settings/piProviderRow.ts`:
- Around line 20-24: Update piProviderModelCount to preserve an explicit
availableModelCount of 0 instead of falling back to modelCount, using nullish
fallback semantics if unresolved availability is represented by null or
undefined. If the fallback intentionally covers unresolved availability,
document that behavior in the function’s doc comment while ensuring zero remains
a valid result.
In `@apps/desktop/src/renderer/components/settings/PiProvidersPanel.tsx`:
- Around line 668-675: In the prompt-event handler, compute whether the request
is a duplicate before calling setPromptValue, then have the functional updater
use that captured result instead of reading lastPromptRequestIdRef.current.
Update lastPromptRequestIdRef.current only after capturing the comparison,
preserving the behavior that duplicate prompts retain input while genuinely new
prompts clear it.
In `@apps/desktop/src/renderer/components/settings/providerSectionPrimitives.tsx`:
- Around line 305-314: Update the focus effect around the dialogRef so its
cleanup restores previous focus only when the dialog component unmounts, not
when suspended changes. Keep the effect dependent on suspended so entry focus
still occurs when suspension is lifted, while preventing nested dialogs from
focusing the page behind them during transitions.
- Around line 316-334: Update the dialog overlay in the component containing
dialogRef to track where the pointer press began and invoke onClose only when
the press originated on the backdrop itself; remove the panel’s stopPropagation
click handler since the target check replaces it, while preserving normal
backdrop dismissal and preventing drags or text selection from closing the
dialog.
In `@apps/desktop/src/renderer/components/settings/ProvidersSection.test.tsx`:
- Around line 812-832: Add a named regression test for the Pi flow re-adoption
path: render ProvidersSection, unmount and re-render it, then emit a prompt
event for the same provider without an active flow. Assert that the provider
prompt input and Cancel button appear, covering the branch that rebuilds a flow
from prompt or pending events rather than the existing success-event settle
path.
In `@apps/ios/ADE/Views/Work/WorkChatHeaderAndMessageViews.swift`:
- Around line 344-360: The iOS user bubble border must use raised contrast for
deep accents, matching desktop behavior. Update userBubbleBorder near
userBubbleFill to detect ADEColor.isDeepChatAccent(accent) and apply the
corresponding white-mix border treatment, while preserving existing behavior for
other accents. Add the regression test
testDeepProviderUserBubbleUsesRaisedBorderContrast using a Pi or Cursor user
bubble.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/components/settings/ChatAppearancePreview.test.tsx`:
- Around line 77-97: Strengthen ChatAppearancePreview tests by adding a named
regression test that identifies each rendered provider shell and verifies its
expected --chat-user-border-accent-mix value, specifically ensuring Cursor and
Pi use the widened mix. Do not rely on aggregate shell counts or toContain; use
the provider identity attributes exposed by each section and assert the exact
mix per provider.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ai/piAuthService.test.ts`:
- Around line 289-327: Add a named regression test covering cancellation during
worker acquisition: keep acquireWorker pending, start Pi login, call
cancelPiLogin before acquisition resolves, then resolve the worker and verify
its release method is called and the start resolves with the superseded error
without announcing an outcome. Use the existing test helpers such as
createFakeWorker, installWorker, startPiLogin, and cancelPiLogin, and ensure
this exercises the claimProviderGeneration mismatch path rather than an
already-registered flow.
In `@apps/desktop/src/main/services/ai/piInstallation.ts`:
- Around line 412-417: Remove the redundant local fallback from the authType
assignment near runtimeAuthType, since runtimeAuthType already preserves
fallbackProvider.authType when it is local. Keep the existing authMethods
derivation and runtime behavior unchanged.
In `@apps/desktop/src/main/services/chat/piSdkWorker.ts`:
- Around line 305-318: Export the existing pathWithinDirectory helper from
piSessionStore.ts and replace the duplicated containment logic in
sessionFileIsAuthorized with that shared helper. Preserve the current
authorization behavior, and update the nearby comment to reference
piSessionStore.ts as the source of the .native realpath rationale rather than
piSessionLease.
In `@apps/desktop/src/main/services/chat/piSessionStore.ts`:
- Around line 228-246: Require a non-empty sessionRoot in resolvePiSessionFile
before resolving an explicit sessionFile, returning null when it is absent;
retain the canonical path and containment validation using that root. Update the
test calls in piSession.test.ts at the referenced cases to provide sessionRoot.
In `@apps/desktop/src/main/services/pty/ptyService.test.ts`:
- Around line 185-204: Update the mock filesystem handlers in the test setup:
make unlinkSync remove the path from fileStats, and make writeFileSync create or
refresh its fileStats entry alongside fileContents. Keep promises.stat and
promises.open behavior consistent so deleted paths no longer appear to exist and
newly written paths are tracked.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 1145-1158: Move the export modifier from before the JSDoc block to
the PI_CONTINUATION_FLAG_RE declaration, placing the comment immediately above
the exported constant so editors and API documentation associate it correctly.
Leave stripPiContinuationArgs unchanged.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 1633-1657: Move piPermissionModeToPickerValue from AgentChatPane
into the shared nativeLaunchControls module, preserving its existing mapping for
plan, edit, full-auto, config-toml, default, and auto. Update the component to
import and use the shared helper, and add or extend nativeLaunchControls tests
to cover the conversion.
In `@apps/desktop/src/renderer/components/settings/piProviderRow.ts`:
- Around line 39-52: Update runtimeConnectionForPiProvider so it explicitly maps
Pi provider ids to the corresponding ADE runtime keys, limiting lookups to the
agreed ollama and lmstudio mappings; return null for all other ids. Keep the
function’s existing null fallback and align the doc comment with this explicit
mapping behavior.
In `@apps/desktop/src/renderer/components/settings/PiProvidersPanel.tsx`:
- Around line 800-831: Memoize the derived provider data in the panel: use
useMemo for signableProviders, allRows, and modelIdsByProvider, with
dependencies covering their source values and preserving correct recalculation
when providers, installation data, or signableProviders change. Keep the
existing filtering and decode logic unchanged, and follow the established
ProvidersSection.tsx memoization pattern.
In `@apps/desktop/src/renderer/components/settings/providerSectionPrimitives.tsx`:
- Around line 286-291: Update the focusable-element filter in the focus-trap
logic around ProviderDetailDialog to avoid offsetParent, using
layout-independent visibility and focusability checks such as
hidden/aria-hidden, disabled, and negative tabIndex. Preserve first/last
fallback behavior, and add coverage verifying Tab cycling from the last element
to the first and reverse cycling from the first to the last.
In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx`:
- Around line 213-214: Remove the orphaned doc comment immediately before
prettifyProviderId; the comment describes the relocated panel helper and should
not remain attached to the formatting function.
🪄 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: 32513b19-0f19-4e0e-aeaa-163539cb9318
⛔ Files ignored due to path filters (8)
docs/ARCHITECTURE.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/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-sessions.mdis excluded by!docs/**docs/playbooks/ship-lane.mdis excluded by!docs/**
📒 Files selected for processing (54)
.agents/skills/ship/SKILL.md.github/workflows/ci.ymlapps/ade-cli/README.mdapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/__tests__/piSdk.integration.test.tsapps/desktop/src/main/services/ai/piAuthService.test.tsapps/desktop/src/main/services/ai/piAuthService.tsapps/desktop/src/main/services/ai/piInstallation.test.tsapps/desktop/src/main/services/ai/piInstallation.tsapps/desktop/src/main/services/chat/agentChatService.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/piSdkWorker.tsapps/desktop/src/main/services/chat/piSession.test.tsapps/desktop/src/main/services/chat/piSessionLease.test.tsapps/desktop/src/main/services/chat/piSessionLease.tsapps/desktop/src/main/services/chat/piSessionOwnership.tsapps/desktop/src/main/services/chat/piSessionStore.tsapps/desktop/src/main/services/externalSessions/discoverPi.tsapps/desktop/src/main/services/ipc/ipcChannelRedaction.test.tsapps/desktop/src/main/services/ipc/ipcChannelRedaction.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/chatSurfaceTheme.test.tsapps/desktop/src/renderer/components/chat/chatSurfaceTheme.tsapps/desktop/src/renderer/components/lanes/laneDesignTokens.tsapps/desktop/src/renderer/components/settings/ChatAppearancePreview.test.tsxapps/desktop/src/renderer/components/settings/ChatAppearancePreview.tsxapps/desktop/src/renderer/components/settings/OpenCodeProviderDetailModal.tsxapps/desktop/src/renderer/components/settings/PiProviderDetailModal.tsxapps/desktop/src/renderer/components/settings/PiProvidersPanel.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/renderer/components/settings/piProviderRow.tsapps/desktop/src/renderer/components/settings/providerSectionPrimitives.tsxapps/desktop/src/renderer/components/shared/useOpenProviderSignIn.tsapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/components/work/PiLoginPromptButton.tsxapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/lib/lobeProviderIconSrc.tsapps/desktop/src/renderer/lib/nativeLaunchControls.test.tsapps/desktop/src/renderer/lib/nativeLaunchControls.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/config.tsapps/ios/ADE/Views/Components/ADEDesignSystem.swiftapps/ios/ADE/Views/Work/WorkChatHeaderAndMessageViews.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (2)
- apps/desktop/src/main/services/chat/piSessionLease.test.ts
- apps/desktop/src/renderer/components/work/PiLoginPromptButton.tsx
| it("writes into a user-configured session directory when one is set", async () => { | ||
| const fixture = createFixture(); | ||
| const configured = path.join(fixture.root, "configured-sessions"); | ||
| fs.mkdirSync(configured, { recursive: true }); | ||
| const connection = await acquireTracked(fixture, `configured:${Date.now()}`, { | ||
| sessionStorageDir: configured, | ||
| }); | ||
| const planned = connection.pooled.sessionFile!; | ||
|
|
||
| expect(path.dirname(planned)).toBe(configured); | ||
| await connection.pooled.sendPrompt({ prompt: "write the session header" }); | ||
| await nextRequest(); | ||
| await waitFor(() => fs.existsSync(planned)); | ||
|
|
||
| expect(classifyPiSessionFile({ | ||
| filePath: planned, | ||
| cwd: fixture.cwd, | ||
| sessionId: connection.pooled.sessionId, | ||
| sessionRoot: configured, | ||
| })).toEqual({ state: "authorized", filePath: fs.realpathSync(planned) }); | ||
| expect(sessionFiles(fixture)).toEqual([]); | ||
| expect(piSessionRootForEnvironment({ | ||
| PI_CODING_AGENT_DIR: fixture.agentDir, | ||
| PI_CODING_AGENT_SESSION_DIR: configured, | ||
| })).toBe(configured); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pass a sessionRoot that matches the configured storage directory.
installedPiArgs always sets sessionRoot: fixture.sessionRoot (Line 198). This test sets sessionStorageDir to configured, which sits outside fixture.sessionRoot. In production piSessionStoreForEnvironment returns root === storageDir for the configured case (apps/desktop/src/main/services/chat/piSessionStore.ts Line 93), so this pair never occurs.
The test passes today because the create path does not run validatedSessionFile; the worker authorizes a session file only on resume. Line 383 then classifies against configured, not against the sessionRoot the worker actually received. The two values disagree.
Let the helper accept an overriding sessionRoot, and pass configured for both. A later resume-from-configured-storage test can then reuse the same fixture without failing for an unrelated reason.
💚 Proposed fixture alignment
const connection = await acquireTracked(fixture, `configured:${Date.now()}`, {
+ sessionRoot: configured,
sessionStorageDir: configured,
});In installedPiArgs, add the override:
extensions?: boolean;
+ sessionRoot?: string;
sessionStorageDir?: string;
}): {- sessionRoot: fixture.sessionRoot,
+ sessionRoot: options?.sessionRoot ?? fixture.sessionRoot,📝 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.
| it("writes into a user-configured session directory when one is set", async () => { | |
| const fixture = createFixture(); | |
| const configured = path.join(fixture.root, "configured-sessions"); | |
| fs.mkdirSync(configured, { recursive: true }); | |
| const connection = await acquireTracked(fixture, `configured:${Date.now()}`, { | |
| sessionStorageDir: configured, | |
| }); | |
| const planned = connection.pooled.sessionFile!; | |
| expect(path.dirname(planned)).toBe(configured); | |
| await connection.pooled.sendPrompt({ prompt: "write the session header" }); | |
| await nextRequest(); | |
| await waitFor(() => fs.existsSync(planned)); | |
| expect(classifyPiSessionFile({ | |
| filePath: planned, | |
| cwd: fixture.cwd, | |
| sessionId: connection.pooled.sessionId, | |
| sessionRoot: configured, | |
| })).toEqual({ state: "authorized", filePath: fs.realpathSync(planned) }); | |
| expect(sessionFiles(fixture)).toEqual([]); | |
| expect(piSessionRootForEnvironment({ | |
| PI_CODING_AGENT_DIR: fixture.agentDir, | |
| PI_CODING_AGENT_SESSION_DIR: configured, | |
| })).toBe(configured); | |
| }); | |
| it("writes into a user-configured session directory when one is set", async () => { | |
| const fixture = createFixture(); | |
| const configured = path.join(fixture.root, "configured-sessions"); | |
| fs.mkdirSync(configured, { recursive: true }); | |
| const connection = await acquireTracked(fixture, `configured:${Date.now()}`, { | |
| sessionRoot: configured, | |
| sessionStorageDir: configured, | |
| }); | |
| const planned = connection.pooled.sessionFile!; | |
| expect(path.dirname(planned)).toBe(configured); | |
| await connection.pooled.sendPrompt({ prompt: "write the session header" }); | |
| await nextRequest(); | |
| await waitFor(() => fs.existsSync(planned)); | |
| expect(classifyPiSessionFile({ | |
| filePath: planned, | |
| cwd: fixture.cwd, | |
| sessionId: connection.pooled.sessionId, | |
| sessionRoot: configured, | |
| })).toEqual({ state: "authorized", filePath: fs.realpathSync(planned) }); | |
| expect(sessionFiles(fixture)).toEqual([]); | |
| expect(piSessionRootForEnvironment({ | |
| PI_CODING_AGENT_DIR: fixture.agentDir, | |
| PI_CODING_AGENT_SESSION_DIR: configured, | |
| })).toBe(configured); | |
| }); |
🤖 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/__tests__/piSdk.integration.test.ts` around
lines 369 - 394, Update the installedPiArgs test helper to accept an optional
sessionRoot override, defaulting to fixture.sessionRoot, and pass configured for
this test’s connection setup. Ensure the worker receives configured as
sessionRoot and keep classifyPiSessionFile using the same configured value so
both paths reflect the production configuration.
| // A loopback base URL wins over everything, including a stored auth entry. | ||
| // LM Studio ships `apiKey: "lmstudio"` in models.json — a placeholder its | ||
| // OpenAI-compatible endpoint requires and ignores — so keying off the | ||
| // presence of a key classified a server the user runs as an API provider, | ||
| // offered to "sign in" to it, and reported it connected on the strength of | ||
| // a config file rather than a reachable server. | ||
| const localBaseUrl = loopbackBaseUrl(provider.baseUrl); | ||
| // A stored auth entry still wins: it is the one piece of evidence that the | ||
| // provider really has an interactive credential. Only the *placeholder* | ||
| // key case — a loopback server with no auth-store entry, which is what LM | ||
| // Studio ships — is reclassified, so a provider behind a local gateway | ||
| // does not lose its sign-in. | ||
| const authType = authInfo.type | ||
| ?? (provider.apiKey ? "api-key" as const : provider.baseUrl ? "local" as const : null); | ||
| ?? (localBaseUrl ? "local" as const : provider.apiKey ? "api-key" as const : provider.baseUrl ? "local" as const : null); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reconcile the two precedence comments, and re-check the non-loopback baseUrl fallback.
Two problems in this block:
-
The comment at Lines 249-254 states that a loopback base URL "wins over everything, including a stored auth entry". The comment at Lines 256-260 states the opposite. The code implements the second version:
authInfo.typetakes precedence through??. Remove or rewrite the first block so the stated precedence matches the code. -
The final fallback
provider.baseUrl ? "local" : nullclassifies any provider with a custom non-loopbackbaseUrlaslocal. That contradicts theloopbackBaseUrldocstring at Lines 203-205, which states that a remote provider reached through a custombaseUrlproxy is still remote. Such a provider then getsauthType: "local"andauthMethods: ["local"], but nobaseUrlfield, because Line 273 only spreadslocalBaseUrl. InPiProvidersPanel.tsxLine 808, rows withauthType === "local"move into the "Local model servers" group and lose every sign-in affordance, andPiLocalServerCardthen renders no endpoint. Restrict thelocalfallback tolocalBaseUrl.
🐛 Proposed fix
- // A loopback base URL wins over everything, including a stored auth entry.
- // LM Studio ships `apiKey: "lmstudio"` in models.json — a placeholder its
- // OpenAI-compatible endpoint requires and ignores — so keying off the
- // presence of a key classified a server the user runs as an API provider,
- // offered to "sign in" to it, and reported it connected on the strength of
- // a config file rather than a reachable server.
const localBaseUrl = loopbackBaseUrl(provider.baseUrl);
// A stored auth entry still wins: it is the one piece of evidence that the
// provider really has an interactive credential. Only the *placeholder*
// key case — a loopback server with no auth-store entry, which is what LM
// Studio ships — is reclassified, so a provider behind a local gateway
// does not lose its sign-in.
const authType = authInfo.type
- ?? (localBaseUrl ? "local" as const : provider.apiKey ? "api-key" as const : provider.baseUrl ? "local" as const : null);
+ ?? (localBaseUrl ? "local" as const : provider.apiKey ? "api-key" as const : null);📝 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.
| // A loopback base URL wins over everything, including a stored auth entry. | |
| // LM Studio ships `apiKey: "lmstudio"` in models.json — a placeholder its | |
| // OpenAI-compatible endpoint requires and ignores — so keying off the | |
| // presence of a key classified a server the user runs as an API provider, | |
| // offered to "sign in" to it, and reported it connected on the strength of | |
| // a config file rather than a reachable server. | |
| const localBaseUrl = loopbackBaseUrl(provider.baseUrl); | |
| // A stored auth entry still wins: it is the one piece of evidence that the | |
| // provider really has an interactive credential. Only the *placeholder* | |
| // key case — a loopback server with no auth-store entry, which is what LM | |
| // Studio ships — is reclassified, so a provider behind a local gateway | |
| // does not lose its sign-in. | |
| const authType = authInfo.type | |
| ?? (provider.apiKey ? "api-key" as const : provider.baseUrl ? "local" as const : null); | |
| ?? (localBaseUrl ? "local" as const : provider.apiKey ? "api-key" as const : provider.baseUrl ? "local" as const : null); | |
| const localBaseUrl = loopbackBaseUrl(provider.baseUrl); | |
| // A stored auth entry still wins: it is the one piece of evidence that the | |
| // provider really has an interactive credential. Only the *placeholder* | |
| // key case — a loopback server with no auth-store entry, which is what LM | |
| // Studio ships — is reclassified, so a provider behind a local gateway | |
| // does not lose its sign-in. | |
| const authType = authInfo.type | |
| ?? (localBaseUrl ? "local" as const : provider.apiKey ? "api-key" as const : null); |
🤖 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/ai/piInstallation.ts` around lines 249 - 262,
Update the comments around authType in the provider classification block so they
consistently state that authInfo.type takes precedence over loopback detection.
Change the final authType fallback to classify as "local" only when localBaseUrl
is present, removing the non-loopback provider.baseUrl condition while
preserving the existing api-key and null fallbacks.
| child.stdin?.write("release\\n"); | ||
| await waitForChildExit(child); | ||
| const parentLease = acquirePiSessionLease({ sessionFile: session.file, owner: "cli", ownerId: "parent" }); | ||
| expect(JSON.parse(fs.readFileSync(lockPath, "utf8")).pid).toBe(process.pid); | ||
| parentLease.release(); | ||
| } finally { | ||
| if (child.exitCode == null && child.signalCode == null) { | ||
| child.stdin?.write("release\\n"); | ||
| await waitForChildExit(child); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a real newline when writing to the child's stdin.
Line 421 sits inside the childScript template literal, so "ready\\n" correctly emits ready\n into the child source. Lines 459 and 466 are plain test source, not inside that template. There "release\\n" is the two characters backslash and n, not a newline.
The test still passes today, because the child matches with chunk.includes("release"). The escaping is a latent trap: if the child is later changed to line-based parsing, the write stops matching and the test hangs until the timeout and kill path.
🐛 Proposed fix
- child.stdin?.write("release\\n");
+ child.stdin?.write("release\n");
await waitForChildExit(child);
const parentLease = acquirePiSessionLease({ sessionFile: session.file, owner: "cli", ownerId: "parent" });
expect(JSON.parse(fs.readFileSync(lockPath, "utf8")).pid).toBe(process.pid);
parentLease.release();
} finally {
if (child.exitCode == null && child.signalCode == null) {
- child.stdin?.write("release\\n");
+ child.stdin?.write("release\n");📝 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.
| child.stdin?.write("release\\n"); | |
| await waitForChildExit(child); | |
| const parentLease = acquirePiSessionLease({ sessionFile: session.file, owner: "cli", ownerId: "parent" }); | |
| expect(JSON.parse(fs.readFileSync(lockPath, "utf8")).pid).toBe(process.pid); | |
| parentLease.release(); | |
| } finally { | |
| if (child.exitCode == null && child.signalCode == null) { | |
| child.stdin?.write("release\\n"); | |
| await waitForChildExit(child); | |
| child.stdin?.write("release\n"); | |
| await waitForChildExit(child); | |
| const parentLease = acquirePiSessionLease({ sessionFile: session.file, owner: "cli", ownerId: "parent" }); | |
| expect(JSON.parse(fs.readFileSync(lockPath, "utf8")).pid).toBe(process.pid); | |
| parentLease.release(); | |
| } finally { | |
| if (child.exitCode == null && child.signalCode == null) { | |
| child.stdin?.write("release\n"); | |
| await waitForChildExit(child); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 461-461: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(lockPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/piSession.test.ts` around lines 459 -
467, Use actual newline characters in both child.stdin.write calls within the
test cleanup and release flow, replacing the escaped “release\n” literals while
leaving the childScript template literal escaping unchanged.
| function ownerPathFor(sessionFile: string): string { | ||
| return `${path.resolve(sessionFile)}.ade-owner`; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Canonicalize the session file before deriving the owner sidecar path.
ownerPathFor applies path.resolve only. It does not resolve symlinks or platform aliases. apps/desktop/src/main/services/chat/piSessionStore.ts Line 229 documents the same hazard and canonicalizes with fs.realpathSync.native so that macOS aliases such as /var -> /private/var cannot produce two sidecars for one JSONL file.
Ownership is durable and crosses surfaces: ADE chat writes the sidecar, and a tracked terminal reads it on a later launch. If the two surfaces reach the same session file through different spellings, readPiSessionOwner returns null, piSessionIsAdoptableByTerminal returns true, and the terminal adopts the chat's session. That is the exact failure this module exists to prevent, and the failure the regression test at apps/desktop/src/main/services/chat/piSession.test.ts Line 157 documents.
Resolve the real path when the file exists, and fall back to path.resolve for a path Pi has not written yet.
🐛 Proposed canonicalization
function ownerPathFor(sessionFile: string): string {
- return `${path.resolve(sessionFile)}.ade-owner`;
+ const resolved = path.resolve(sessionFile);
+ try {
+ // Same canonical spelling the store uses, so one JSONL file can never
+ // carry two ownership sidecars.
+ return `${fs.realpathSync.native(resolved)}.ade-owner`;
+ } catch {
+ // Pi has not written the file yet; the lexical path is all there is.
+ return `${resolved}.ade-owner`;
+ }
}Add a regression test that records ownership through one spelling and reads it back through an aliased spelling.
📝 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.
| function ownerPathFor(sessionFile: string): string { | |
| return `${path.resolve(sessionFile)}.ade-owner`; | |
| } | |
| function ownerPathFor(sessionFile: string): string { | |
| const resolved = path.resolve(sessionFile); | |
| try { | |
| // Same canonical spelling the store uses, so one JSONL file can never | |
| // carry two ownership sidecars. | |
| return `${fs.realpathSync.native(resolved)}.ade-owner`; | |
| } catch { | |
| // Pi has not written the file yet; the lexical path is all there is. | |
| return `${resolved}.ade-owner`; | |
| } | |
| } |
🤖 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/piSessionOwnership.ts` around lines 20 -
22, Update ownerPathFor to canonicalize existing session files with
fs.realpathSync.native, falling back to path.resolve when the file does not yet
exist. Ensure readPiSessionOwner and piSessionIsAdoptableByTerminal use the
canonical sidecar path, and add a regression test covering ownership written
through one spelling and read through an aliased spelling.
Source: Coding guidelines
| export function readPiSessionHeader(filePath: string): PiSessionHeader | null { | ||
| try { | ||
| const line = fs.readFileSync(filePath, "utf8").split(/\r?\n/u, 1)[0] ?? ""; | ||
| const parsed = JSON.parse(line) as Record<string, unknown>; | ||
| const id = nonEmpty(parsed.id); | ||
| const cwd = normalizePiSessionCwd(parsed.cwd); | ||
| const parsedAt = Date.parse(nonEmpty(parsed.timestamp) ?? ""); | ||
| const createdAt = Number.isFinite(parsedAt) ? parsedAt : null; | ||
| return parsed.type === "session" && id && cwd ? { id, cwd, createdAt } : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Read only the header prefix instead of the whole session file.
readPiSessionHeader loads the complete JSONL file into memory and then keeps only the first line. Pi session files hold the full transcript, so they grow to megabytes. listPiSessionFilesForCwd calls this for every .jsonl under the store root, synchronously, on the Electron main process. The cost of one discovery pass is proportional to the total size of the store.
Read a bounded prefix instead. The header is the first line, so a fixed-size read is sufficient in practice, with a fallback when no newline appears in the prefix.
⚡ Proposed bounded header read
+const PI_SESSION_HEADER_READ_BYTES = 64 * 1024;
+
+function firstLineOf(filePath: string): string {
+ const handle = fs.openSync(filePath, "r");
+ try {
+ const buffer = Buffer.allocUnsafe(PI_SESSION_HEADER_READ_BYTES);
+ const read = fs.readSync(handle, buffer, 0, buffer.length, 0);
+ const text = buffer.subarray(0, read).toString("utf8");
+ const newline = text.search(/\r?\n/u);
+ // No newline in the prefix: the first record is larger than the budget.
+ return newline === -1 ? (read < PI_SESSION_HEADER_READ_BYTES ? text : "") : text.slice(0, newline);
+ } finally {
+ fs.closeSync(handle);
+ }
+}
+
/** Read a native Pi header. A session without a cwd is invalid for ADE use. */
export function readPiSessionHeader(filePath: string): PiSessionHeader | null {
try {
- const line = fs.readFileSync(filePath, "utf8").split(/\r?\n/u, 1)[0] ?? "";
+ const line = firstLineOf(filePath);
const parsed = JSON.parse(line) as Record<string, unknown>;📝 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.
| export function readPiSessionHeader(filePath: string): PiSessionHeader | null { | |
| try { | |
| const line = fs.readFileSync(filePath, "utf8").split(/\r?\n/u, 1)[0] ?? ""; | |
| const parsed = JSON.parse(line) as Record<string, unknown>; | |
| const id = nonEmpty(parsed.id); | |
| const cwd = normalizePiSessionCwd(parsed.cwd); | |
| const parsedAt = Date.parse(nonEmpty(parsed.timestamp) ?? ""); | |
| const createdAt = Number.isFinite(parsedAt) ? parsedAt : null; | |
| return parsed.type === "session" && id && cwd ? { id, cwd, createdAt } : null; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| const PI_SESSION_HEADER_READ_BYTES = 64 * 1024; | |
| function firstLineOf(filePath: string): string { | |
| const handle = fs.openSync(filePath, "r"); | |
| try { | |
| const buffer = Buffer.allocUnsafe(PI_SESSION_HEADER_READ_BYTES); | |
| const read = fs.readSync(handle, buffer, 0, buffer.length, 0); | |
| const text = buffer.subarray(0, read).toString("utf8"); | |
| const newline = text.search(/\r?\n/u); | |
| // No newline in the prefix: the first record is larger than the budget. | |
| return newline === -1 ? (read < PI_SESSION_HEADER_READ_BYTES ? text : "") : text.slice(0, newline); | |
| } finally { | |
| fs.closeSync(handle); | |
| } | |
| } | |
| /** Read a native Pi header. A session without a cwd is invalid for ADE use. */ | |
| export function readPiSessionHeader(filePath: string): PiSessionHeader | null { | |
| try { | |
| const line = firstLineOf(filePath); | |
| const parsed = JSON.parse(line) as Record<string, unknown>; | |
| const id = nonEmpty(parsed.id); | |
| const cwd = normalizePiSessionCwd(parsed.cwd); | |
| const parsedAt = Date.parse(nonEmpty(parsed.timestamp) ?? ""); | |
| const createdAt = Number.isFinite(parsedAt) ? parsedAt : null; | |
| return parsed.type === "session" && id && cwd ? { id, cwd, createdAt } : null; | |
| } catch { | |
| return null; | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 127-127: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(filePath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/piSessionStore.ts` around lines 126 -
138, Update readPiSessionHeader to read only a bounded prefix of each session
file instead of using fs.readFileSync on the complete file. Extract the first
line from that prefix, and add a fallback that handles headers whose newline is
not present within the initial bounded read while avoiding loading the full
transcript; preserve the existing JSON parsing and validation behavior.
| // 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; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Read the previous request id before you update the ref.
setPromptValue receives a functional updater. React does not guarantee that it runs synchronously inside this handler; it runs during the render phase. Line 674 assigns lastPromptRequestIdRef.current = event.prompt.requestId immediately after the call. If the updater runs after that assignment, the comparison at Line 673 sees the new request id and returns current, so a genuinely new prompt never clears the field. The stale answer from the previous prompt then stays in the input.
Capture the comparison result before the state update.
🐛 Proposed fix
if (event.state === "prompt" && event.prompt) {
- setPromptValue((current) => (lastPromptRequestIdRef.current === event.prompt!.requestId ? current : ""));
+ const isDuplicate = lastPromptRequestIdRef.current === event.prompt.requestId;
lastPromptRequestIdRef.current = event.prompt.requestId;
+ if (!isDuplicate) setPromptValue("");
}📝 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.
| // 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; | |
| } | |
| // 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) { | |
| const isDuplicate = lastPromptRequestIdRef.current === event.prompt.requestId; | |
| lastPromptRequestIdRef.current = event.prompt.requestId; | |
| if (!isDuplicate) setPromptValue(""); | |
| } |
🤖 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/renderer/components/settings/PiProvidersPanel.tsx` around
lines 668 - 675, In the prompt-event handler, compute whether the request is a
duplicate before calling setPromptValue, then have the functional updater use
that captured result instead of reading lastPromptRequestIdRef.current. Update
lastPromptRequestIdRef.current only after capturing the comparison, preserving
the behavior that duplicate prompts retain input while genuinely new prompts
clear it.
| useEffect(() => { | ||
| if (suspended) return; | ||
| const node = dialogRef.current; | ||
| if (!node) return; | ||
| const previous = document.activeElement as HTMLElement | null; | ||
| node.focus(); | ||
| return () => { | ||
| previous?.focus?.(); | ||
| }; | ||
| }, [suspended]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Focus returns to the page behind the dialog when a nested dialog opens.
The effect depends on suspended. When suspended changes from false to true, React runs the cleanup and calls previous?.focus?.(). previous is the element that held focus before this dialog opened, so focus moves behind both dialogs at the moment the nested dialog appears. The nested dialog usually claims focus afterwards, but that depends on effect ordering between two separate components.
Restore focus only on unmount. Keep the entry focus keyed on suspended.
♻️ Proposed fix
+ const restoreFocusRef = useRef<HTMLElement | null>(null);
+
+ // Capture once, restore once: a nested dialog suspends this one, and
+ // returning focus then would drop the user behind both dialogs.
+ useEffect(() => {
+ restoreFocusRef.current = document.activeElement as HTMLElement | null;
+ return () => {
+ restoreFocusRef.current?.focus?.();
+ };
+ }, []);
+
// Move focus into the dialog so keyboard users aren't acting under the overlay.
useEffect(() => {
if (suspended) return;
- const node = dialogRef.current;
- if (!node) return;
- const previous = document.activeElement as HTMLElement | null;
- node.focus();
- return () => {
- previous?.focus?.();
- };
+ dialogRef.current?.focus();
}, [suspended]);As per path instructions for apps/desktop/src/**: "check for IPC security, proper main/renderer process separation, and React best practices."
📝 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.
| useEffect(() => { | |
| if (suspended) return; | |
| const node = dialogRef.current; | |
| if (!node) return; | |
| const previous = document.activeElement as HTMLElement | null; | |
| node.focus(); | |
| return () => { | |
| previous?.focus?.(); | |
| }; | |
| }, [suspended]); | |
| const restoreFocusRef = useRef<HTMLElement | null>(null); | |
| // Capture once, restore once: a nested dialog suspends this one, and | |
| // returning focus then would drop the user behind both dialogs. | |
| useEffect(() => { | |
| restoreFocusRef.current = document.activeElement as HTMLElement | null; | |
| return () => { | |
| restoreFocusRef.current?.focus?.(); | |
| }; | |
| }, []); | |
| useEffect(() => { | |
| if (suspended) return; | |
| dialogRef.current?.focus(); | |
| }, [suspended]); |
🤖 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/renderer/components/settings/providerSectionPrimitives.tsx`
around lines 305 - 314, Update the focus effect around the dialogRef so its
cleanup restores previous focus only when the dialog component unmounts, not
when suspended changes. Keep the effect dependent on suspended so entry focus
still occurs when suspension is lifted, while preventing nested dialogs from
focusing the page behind them during transitions.
Source: Path instructions
| return createPortal( | ||
| <div | ||
| className="fixed inset-0 z-50 flex items-center justify-center" | ||
| style={{ background: "rgba(0,0,0,0.70)" }} | ||
| onClick={onClose} | ||
| > | ||
| <div | ||
| ref={dialogRef} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label={`${title} provider`} | ||
| tabIndex={-1} | ||
| className="w-full max-w-md max-h-[85vh] overflow-y-auto outline-none" | ||
| style={{ | ||
| background: COLORS.cardBgSolid, | ||
| border: `1px solid ${COLORS.outlineBorder}`, | ||
| boxShadow: "0 28px 80px -36px rgba(0,0,0,0.82)", | ||
| }} | ||
| onClick={(event) => event.stopPropagation()} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The backdrop closes the dialog when a drag that starts inside ends outside.
The overlay closes on click. A click fires on the overlay when the user presses inside the dialog, drags over the backdrop, and releases. Selecting text in the dialog body therefore closes it and discards the view. Track the press target instead.
🐛 Proposed fix
const dialogRef = useRef<HTMLDivElement | null>(null);
+ const pressedBackdrop = useRef(false); return createPortal(
<div
className="fixed inset-0 z-50 flex items-center justify-center"
style={{ background: "rgba(0,0,0,0.70)" }}
- onClick={onClose}
+ onMouseDown={(event) => {
+ pressedBackdrop.current = event.target === event.currentTarget;
+ }}
+ onClick={(event) => {
+ if (event.target !== event.currentTarget) return;
+ if (!pressedBackdrop.current) return;
+ pressedBackdrop.current = false;
+ onClose();
+ }}
>
<div
ref={dialogRef}
role="dialog"
aria-modal="true"
aria-label={`${title} provider`}
tabIndex={-1}
className="w-full max-w-md max-h-[85vh] overflow-y-auto outline-none"
style={{
background: COLORS.cardBgSolid,
border: `1px solid ${COLORS.outlineBorder}`,
boxShadow: "0 28px 80px -36px rgba(0,0,0,0.82)",
}}
- onClick={(event) => event.stopPropagation()}
>With the target check on the overlay, the stopPropagation handler on the panel is no longer required.
📝 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.
| return createPortal( | |
| <div | |
| className="fixed inset-0 z-50 flex items-center justify-center" | |
| style={{ background: "rgba(0,0,0,0.70)" }} | |
| onClick={onClose} | |
| > | |
| <div | |
| ref={dialogRef} | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={`${title} provider`} | |
| tabIndex={-1} | |
| className="w-full max-w-md max-h-[85vh] overflow-y-auto outline-none" | |
| style={{ | |
| background: COLORS.cardBgSolid, | |
| border: `1px solid ${COLORS.outlineBorder}`, | |
| boxShadow: "0 28px 80px -36px rgba(0,0,0,0.82)", | |
| }} | |
| onClick={(event) => event.stopPropagation()} | |
| return createPortal( | |
| <div | |
| className="fixed inset-0 z-50 flex items-center justify-center" | |
| style={{ background: "rgba(0,0,0,0.70)" }} | |
| onMouseDown={(event) => { | |
| pressedBackdrop.current = event.target === event.currentTarget; | |
| }} | |
| onClick={(event) => { | |
| if (event.target !== event.currentTarget) return; | |
| if (!pressedBackdrop.current) return; | |
| pressedBackdrop.current = false; | |
| onClose(); | |
| }} | |
| > | |
| <div | |
| ref={dialogRef} | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={`${title} provider`} | |
| tabIndex={-1} | |
| className="w-full max-w-md max-h-[85vh] overflow-y-auto outline-none" | |
| style={{ | |
| background: COLORS.cardBgSolid, | |
| border: `1px solid ${COLORS.outlineBorder}`, | |
| boxShadow: "0 28px 80px -36px rgba(0,0,0,0.82)", | |
| }} | |
| > |
🤖 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/renderer/components/settings/providerSectionPrimitives.tsx`
around lines 316 - 334, Update the dialog overlay in the component containing
dialogRef to track where the pointer press began and invoke onClose only when
the press originated on the backdrop itself; remove the panel’s stopPropagation
click handler since the target check replaces it, while preserving normal
backdrop dismissal and preventing drags or text selection from closing the
dialog.
| // The outcome is reported by the status event, not only by whoever is still | ||
| // awaiting the start call, so a card that remounted mid-flow still learns. | ||
| it("reports a Pi sign-in that completed while no start call was pending", async () => { | ||
| const getStatusMock = window.ade.ai.getStatus as ReturnType<typeof vi.fn>; | ||
| getStatusMock.mockReset(); | ||
| getStatusMock.mockResolvedValue(buildStatus(true, [], { piInstallation: buildPiInstallation() })); | ||
| (window.ade.ai.piLoginProviders as ReturnType<typeof vi.fn>).mockResolvedValue([ | ||
| { id: "xai", name: "xAI", authTypes: ["oauth"], configured: false }, | ||
| ]); | ||
|
|
||
| renderProvidersSection(); | ||
| await screen.findByRole("button", { name: "Connect xAI in Pi" }); | ||
| getStatusMock.mockClear(); | ||
|
|
||
| await act(async () => { | ||
| emitPiAuthStatus?.({ providerId: "xai", state: "success" }); | ||
| }); | ||
|
|
||
| expect(screen.getByText(/Signed in to xAI\./)).toBeTruthy(); | ||
| expect(getStatusMock).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The flow re-adoption branch is untested.
This test emits state: "success", which takes the early-return settle path at PiProvidersPanel.tsx Line 642 and never reaches the re-adoption branch at Lines 649-658. That branch rebuilds a flow when a prompt or pending event arrives with no current flow. It is the code that makes a login survive a Settings unmount and remount, which the PR objectives list as a change. No test covers it.
The test also does not remount, although the comment describes a card that "remounted mid-flow".
Add a case that unmounts the section, re-renders it, then emits a prompt event for the same provider, and asserts the prompt input and the Cancel button appear. Do you want me to write that test?
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/renderer/components/settings/ProvidersSection.test.tsx`
around lines 812 - 832, Add a named regression test for the Pi flow re-adoption
path: render ProvidersSection, unmount and re-render it, then emit a prompt
event for the same provider without an active flow. Assert that the provider
prompt input and Cancel button appear, covering the branch that rebuilds a flow
from prompt or pending events rather than the existing success-event settle
path.
Source: Coding guidelines
| private var userBubbleFill: Color { | ||
| isCodexChat | ||
| ? workMixColors(accent, workViolet, 0.44) | ||
| : workMixColors(accent, workViolet, 0.36) | ||
| // Claude and Codex shipped looking right, so they keep the original stops, | ||
| // which mix toward a fixed violet. | ||
| if ADEColor.chatAccentKeepsOriginalBubble(accent) { | ||
| return isCodexChat | ||
| ? workMixColors(accent, workViolet, 0.44) | ||
| : workMixColors(accent, workViolet, 0.36) | ||
| } | ||
| // Near-black accents (Cursor, Pi) lift toward white instead — mixing them | ||
| // toward violet turned two different runtimes into the same purple, and | ||
| // deepening them would sink the bubble into the transcript background. | ||
| if ADEColor.isDeepChatAccent(accent) { | ||
| return workMixColors(accent, Color.white, 0.14) | ||
| } | ||
| // Everything else shades from its own accent, so a per-provider colour is | ||
| // actually visible as that colour. | ||
| return accent |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the deep-accent border treatment on iOS.
userBubbleFill detects deep accents, but userBubbleBorder at Lines 363-367 keeps the normal border treatment. Desktop increases the deep-accent border mix so Cursor and Pi keep a visible bubble edge. Add the same deep-accent branch for the iOS border.
Add a regression test named testDeepProviderUserBubbleUsesRaisedBorderContrast for a Pi or Cursor user bubble.
🤖 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/WorkChatHeaderAndMessageViews.swift` around lines 344
- 360, The iOS user bubble border must use raised contrast for deep accents,
matching desktop behavior. Update userBubbleBorder near userBubbleFill to detect
ADEColor.isDeepChatAccent(accent) and apply the corresponding white-mix border
treatment, while preserving existing behavior for other accents. Add the
regression test testDeepProviderUserBubbleUsesRaisedBorderContrast using a Pi or
Cursor user bubble.
1 participant
Pi chat and tracked Pi CLI terminals were both unusable. Two separate causes, one shared root: ADE and Pi disagreed about where Pi's sessions live.
Session store
ADE resolved a store root Pi never writes to, then validated a file Pi had not written yet — Pi doesn't create the JSONL until the first assistant message. Both are fixed, and resolution now mirrors Pi's own precedence:
--session-dir>PI_CODING_AGENT_SESSION_DIR> profilesettings.jsonsessionDir><agentDir>/sessionsA checkout's
.pi/settings.jsonis deliberately ignored. Pi's ownSettingsManagermerges it, but that file belongs to a repository ADE has not vouched for — honouring it would let any clone redirect where ADE authorizes and leases sessions.CLI resume
defaultResumeCommandForTool("pi")producedpi --continue, which means "the most recent session for this directory." Since ADE chat and the tracked CLI now share one native store, that could be another terminal's session or a chat's — a fresh terminal reopened a four-day-old transcript this way. Pi is now resumed only by a session id ADE captured for that terminal.Note
pi -ris Pi's browse/select picker, not "continue" — they are not interchangeable.Ownership
One store with two writers needs both a live lock and a durable claim:
.ade-lease(removed on release) and.ade-owner(never removed).Also in this PR
piSessionLease.ts(699 lines) split intopiSessionStore/piSessionLease/piSessionOwnership.registerIpc's closure so the credential contract is reachable from a test —aiPiLoginSubmit's value can be a raw API key.Verification
piSession.test.tsadded to the nativewindows-foundationjob — path containment,realpathSync.native, and hard-link lease publication were Linux-only. That surfaced a real gotcha: the cross-process lease child spawned Node with a stripped env, and Windows needsSystemRoot.piis inLAUNCH_PROFILESand works, but five help/error strings denied it existed. Fixed.piAuthService.tshas no logger calls at all.Summary by CodeRabbit
New Features
Bug Fixes
Documentation