Develop - #1097
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- shell: append user-level bin dirs (~/.local/bin etc.) to PATH in spawned
pty so CLIs like claude are reachable when npm strips shell-rc PATH entries
- auth: handle structured error object {code, message} in
resolveApiErrorMessage to prevent React crash (white screen) on login failure
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Pi as a full provider alongside the existing four, wired through the same IProvider contract: live chat runtime (RPC mode, agent_settled as the sole success terminal), model catalog, install/auth status, session history and disk sync, skills discovery, and permission modes. Unsupported capabilities (MCP) surface an explicit ERR rather than empty success. - Pin @earendil-works/pi-coding-agent@0.83.0 (exact version) - New list/pi facets: paths, rpc-client, session-store, runtime, models, auth, sessions, session-synchronizer, skills, token-usage, mcp - Central wiring: registry, capabilities, token-usage, watcher, agent routes - Frontend: pi provider type, brand/logo, per-provider model state, permission picker, MCP exclusion, /skill:<name> display - Fix runtime close-before-settle detection via real process exit hook - Skip MCP-unsupported providers in global add/remove instead of reporting a spurious failure Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughPi is added as a complete provider with RPC runtime, session parsing, models, authentication, skills, capabilities, backend routing, WebSocket support, and frontend integration. Realtime chat reconciliation now handles streaming messages, thinking metadata, tool results, and duplicate snapshots. ChangesPi provider integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant AgentRouter
participant PiRuntime
participant PiRpcClient
participant SessionStore
Client->>AgentRouter: send provider pi request
AgentRouter->>PiRuntime: run message and session options
PiRuntime->>PiRpcClient: start RPC and bind native session
PiRpcClient-->>PiRuntime: stream text, thinking, and tool events
PiRuntime-->>Client: normalized realtime messages
PiRpcClient-->>PiRuntime: agent_settled
PiRuntime->>SessionStore: load persisted history and usage
SessionStore-->>Client: reconciled session history
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
docs/pi-provider-integration-plan.md-3-7 (1)
3-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the Pi implementation status documents.
The PR implements Pi support, but the plan says “待实施,” the TODO still lists Pi integration, and all implementation tasks remain unchecked. This makes the delivered backend and frontend behavior unclear.
docs/pi-provider-integration-plan.md#L3-L7: Mark the plan as implemented, or mark it as a historical design document.TODO.md#L3-L4: Remove the completed Pi integration task or replace it with remaining work.openspec/changes/add-pi-provider/tasks.md#L21-L61: Mark completed tasks, or state that the checklist is historical.As per path instructions, “document the backend/frontend behavior clearly.”
🤖 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 `@docs/pi-provider-integration-plan.md` around lines 3 - 7, Update the Pi implementation status documentation: in docs/pi-provider-integration-plan.md lines 3-7, mark the plan implemented or historical; in TODO.md lines 3-4, remove the completed Pi integration task or replace it with remaining work; and in openspec/changes/add-pi-provider/tasks.md lines 21-61, mark completed tasks or identify the checklist as historical, clearly reflecting the delivered backend and frontend behavior.Source: Path instructions
openspec/changes/refactor-provider-seams/test-definition.md-20-20 (1)
20-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win使正常路径样本数与描述一致。
该行写明样本数为
6，但描述列出了 5 个能力契约场景和 4 个 provider characterization 冒烟场景，共 9 个命名样本组。请修正数量，或重写描述以明确样本的分组方式。🤖 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 `@openspec/changes/refactor-provider-seams/test-definition.md` at line 20, Update the normal-path entry in the test-definition table so the stated sample count matches the described five capability-contract scenarios plus four provider characterization smoke scenarios, or revise the description to explicitly define the grouping represented by the count.src/components/onboarding/view/subcomponents/AgentConnectionsStep.tsx-49-55 (1)
49-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep one reachable Pi login action visible.
The onboarding
Picard hashideLogin: true, but it passesonLogin={() => onOpenProviderLogin(providerCard.provider)}toAgentConnectionCard. The login button is hidden regardless of auth status, except whenstatus.authenticated, so an unauthenticated Pi card has no modal entry point. Expose a custom Pi action or remove the onboarding card.🤖 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 `@src/components/onboarding/view/subcomponents/AgentConnectionsStep.tsx` around lines 49 - 55, Keep an unauthenticated Pi login entry point reachable: update the Pi configuration in AgentConnectionsStep.tsx (lines 49-55) by removing hideLogin or adding a visible custom action that invokes onOpenProviderLogin. Apply the equivalent reachability fix to the Pi entry in AccountContent.tsx (line 123), if it uses the same hidden-login configuration.server/modules/providers/list/pi/pi-session-store.provider.ts-133-139 (1)
133-139: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake snapshot contents immutable.
Object.freeze()freezes only the outer object or array.snapshot.entries[0],snapshot.messages[0].message, andsnapshot.lastUsage.costremain writable. This violates the documented immutable snapshot contract.Deep-freeze parsed values or expose immutable copies. Add a test that attempts nested mutation.
🤖 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 `@server/modules/providers/list/pi/pi-session-store.provider.ts` around lines 133 - 139, Update the snapshot construction in the Pi session store provider so header, entries, messages, currentModel, and lastUsage are deeply immutable, including nested objects and arrays rather than only their outer containers. Reuse or add a deep-freeze/immutable-copy helper as appropriate, and add coverage that attempts nested mutations such as entries, message fields, and lastUsage.cost and verifies the snapshot remains unchanged.server/modules/providers/list/pi/pi-session-store.provider.ts-153-165 (1)
153-165: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate fields before casting JSON records to snapshot types.
parseHeader()accepts a version-3 header withoutid,timestamp, orcwd.isValidEntry()accepts an entry withouttimestamp.isCompleteUsage()accepts acostobject with onlytotal.These values are then returned as types that declare those fields required. Validate every required field, or loosen the exported types to match the accepted schema.
Also applies to: 168-175, 276-283
🤖 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 `@server/modules/providers/list/pi/pi-session-store.provider.ts` around lines 153 - 165, Update parseHeader, isValidEntry, and isCompleteUsage to validate every field required by PiSessionHeader, entry, and usage types before casting parsed JSON records. Require id, timestamp, and cwd for version-3 headers, require timestamp on entries, and ensure usage cost contains all fields declared required rather than accepting only total; alternatively loosen the exported types to match the accepted schema.server/modules/providers/list/pi/pi-session-store.provider.ts-116-124 (1)
116-124: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject a parsed but invalid final record.
The EOF exception applies to incomplete JSON. A complete JSON record can still fail
isValidEntry(). The current branch silently drops that corruption when the file has no final newline.Throw
PI_SESSION_CORRUPTfor every parsed record that fails validation. Add a regression test for a valid JSON EOF record with an invalid entry shape.Proposed correction
if (!isValidEntry(parsed)) { - if (isLastLine && !endsWithNewline) { - continue; - } + // Parsed JSON is a complete record. Reject schema failures at EOF. throw new AppError(`Pi session 文件损坏（行号 ${lineNumber}）`, {🤖 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 `@server/modules/providers/list/pi/pi-session-store.provider.ts` around lines 116 - 124, Update the invalid-entry handling in the Pi session parsing flow to throw PI_SESSION_CORRUPT for every parsed record that fails isValidEntry(), including a complete JSON record at EOF without a trailing newline. Restrict the isLastLine && !endsWithNewline continuation to incomplete JSON handling before validation, and add a regression test covering a valid JSON EOF record with an invalid entry shape.server/modules/providers/list/pi/pi-auth.provider.ts-98-100 (1)
98-100: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap
getAvailableModels()with a probe deadline.
RpcClient.start()has an internal timeout, butgetAvailableModels()does not. If that RPC request stalls,probeAuthenticated()never settles or closes the client, so apply an application-level timeout around both calls and reject if it expires.🤖 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 `@server/modules/providers/list/pi/pi-auth.provider.ts` around lines 98 - 100, Update probeAuthenticated around client.start() and client.getAvailableModels() to enforce an application-level deadline covering both operations, rejecting when the deadline expires. Ensure timeout or RPC failures propagate through the existing error path so the client is still closed and the probe settles.server/modules/providers/services/provider-token-usage.service.ts-315-334 (1)
315-334: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck that the persisted Pi transcript still exists.
A nonempty stale
jsonl_pathpasses the guard at Line 316. The branch then callsPiTokenUsageProviderwithout validating the file. This does not guarantee theSESSION_FILE_NOT_FOUNDresponse for a deleted Pi transcript.Reject a missing path in the guard. Add a test for a nonempty path whose file is absent.
Proposed fix
- if (!session.jsonl_path) { + if (!session.jsonl_path || !dependencies.fileExists(session.jsonl_path)) {🤖 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 `@server/modules/providers/services/provider-token-usage.service.ts` around lines 315 - 334, Update the Pi-session guard in the token-usage flow to verify that the persisted file referenced by session.jsonl_path still exists, rejecting both empty and stale paths with the existing SESSION_FILE_NOT_FOUND AppError before calling PiTokenUsageProvider.getTokenUsage. Add a test covering a nonempty jsonl_path whose file is absent.server/modules/providers/list/pi/pi-models.provider.ts-88-90 (1)
88-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the default model against an empty
stateModel.
stateModel && options.find(...)?.valuereturns''whengetState()reportsmodel: ''.??does not replace'', soDEFAULTbecomes an empty string that matches no option. Use a nullish-safe lookup instead.🛠️ Proposed fix
- const defaultValue = - (stateModel && options.find((option) => option.value === stateModel)?.value) ?? - options[0].value; + const matchedDefault = stateModel + ? options.find((option) => option.value === stateModel)?.value + : undefined; + const defaultValue = matchedDefault ?? options[0].value;🤖 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 `@server/modules/providers/list/pi/pi-models.provider.ts` around lines 88 - 90, Update the default model calculation in the provider’s state-model lookup so an empty stateModel cannot become the selected default. Normalize or otherwise treat empty stateModel as absent before applying the matching option lookup and fallback to options[0].value, while preserving valid non-empty model selections.server/modules/providers/list/pi/pi-models.provider.ts-79-85 (1)
79-85: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse an English error message.
Both
AppErrormessages use Chinese text ('Pi 未认证'). The rest of this module and the codebase use English strings. This message can reach clients and logs, so keep it consistent and translatable through the frontend locale files.🛠️ Proposed fix
} catch { - throw new AppError('Pi 未认证', { code: 'PI_NOT_AUTHENTICATED' }); + throw new AppError('Pi is not authenticated.', { code: 'PI_NOT_AUTHENTICATED' }); } if (rows.length === 0) { - throw new AppError('Pi 未认证', { code: 'PI_NOT_AUTHENTICATED' }); + throw new AppError('Pi is not authenticated.', { code: 'PI_NOT_AUTHENTICATED' }); }🤖 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 `@server/modules/providers/list/pi/pi-models.provider.ts` around lines 79 - 85, Update both AppError constructions in the Pi provider authentication flow to use the established English message convention instead of the Chinese text, while preserving the existing PI_NOT_AUTHENTICATED error code and control flow.
🧹 Nitpick comments (7)
server/modules/providers/list/claude/claude-runtime.provider.js (1)
212-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
ANTHROPIC_MODELnow overrides the explicit UI model selection.
process.env.ANTHROPIC_MODELtakes the highest precedence. When an operator sets that variable, every request runs with that model, and the model the user picked in the web UI is discarded without any feedback.resolveClaudeEffortat Line 220 then resolves effort against the environment model, not the selected model.If the goal is only to change the default when the user makes no explicit selection, place
options.modelfirst. If the override is intentional, disable or annotate the model picker in the UI so the effective model stays visible to the user.♻️ Proposed precedence change
- // Prefer the locally configured model (ANTHROPIC_MODEL from Claude Code - // settings) so the app always matches the host CLI. When it is unset, fall - // back to the model picked in the web UI, then the provider catalog default. - sdkOptions.model = - process.env.ANTHROPIC_MODEL?.trim() || - options.model || - CLAUDE_FALLBACK_MODELS.DEFAULT; + // Prefer the model picked in the web UI. When the user makes no explicit + // selection, fall back to the locally configured model (ANTHROPIC_MODEL from + // Claude Code settings), then the provider catalog default. + sdkOptions.model = + options.model || + process.env.ANTHROPIC_MODEL?.trim() || + CLAUDE_FALLBACK_MODELS.DEFAULT;🤖 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 `@server/modules/providers/list/claude/claude-runtime.provider.js` around lines 212 - 218, Update the model precedence in the SDK options assignment near resolveClaudeEffort so the explicit options.model selection takes priority over process.env.ANTHROPIC_MODEL, using the environment value only when no UI model is selected and retaining CLAUDE_FALLBACK_MODELS.DEFAULT as the final fallback.server/modules/providers/list/pi/pi-rpc-client.provider.test.ts (1)
223-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate the real-spawn test so the unit suite stays hermetic.
This test spawns the actual Pi CLI. It depends on the installed
@earendil-works/pi-coding-agentpackage exposing a resolvabledist/cli.js, on a usablenodebinary, and on the RPC handshake finishing inside the default test timeout. Any of those conditions can fail in CI for reasons unrelated toPiRpcClient, and the failure looks like a regression in this wrapper.Gate the test behind an explicit environment flag, or move it to a separate integration suite. Also set an explicit timeout so a stalled handshake fails fast with a clear message.
♻️ Proposed gating
-test('real spawn: default PiRpcClient resolves cli.js and receives an RPC response', async () => { +test('real spawn: default PiRpcClient resolves cli.js and receives an RPC response', { + skip: process.env.PI_INTEGRATION_TESTS !== '1' + ? 'set PI_INTEGRATION_TESTS=1 to run the real-spawn test' + : false, + timeout: 30_000, +}, async () => { const client = new PiRpcClient({});🤖 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 `@server/modules/providers/list/pi/pi-rpc-client.provider.test.ts` around lines 223 - 235, Gate the real-spawn test around PiRpcClient.start and getAvailableModels behind an explicit environment flag so the default unit suite remains hermetic, while preserving its cleanup through client.close. Configure an explicit, clearly bounded test timeout for the enabled integration check so stalled RPC handshakes fail promptly.server/modules/providers/list/pi/pi-runtime.provider.ts (1)
498-507: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemove the abort listener when the run settles.
signal.addEventListener('abort', beginAbort, { once: true })is never removed.{ once: true }removes the listener only after the signal fires. When a run settles normally, the listener stays attached to the signal and retains the whole run closure, includingwriter,rpc, andactiveThinkingBlocks.If a caller reuses one
AbortSignalacross several turns of a session, each turn adds another listener, and aborting later invokesbeginAbortfor every completed run. Each stale call is a no-op becausesettledis true, but the retained closures accumulate for the lifetime of the signal.Remove the listener inside
finish.♻️ Proposed fix
const finish = (outcome: PiRunOutcome, closeRpc = true): void => { if (settled) return; finalizeAllThinkingBlocks(); settled = true; state = 'SETTLED'; if (abortTimer) clearTimeout(abortTimer); + signal?.removeEventListener('abort', beginAbort); activeRuns.delete(runId);
signalandbeginAbortare declared afterfinishin the executor. Move thesignalresolution and thebeginAbortdefinition abovefinish, or capture the removal in a mutable cleanup callback thatfinishinvokes.🤖 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 `@server/modules/providers/list/pi/pi-runtime.provider.ts` around lines 498 - 507, Update the run executor’s finish path to remove the abort listener when the run settles, including normal completion, errors, and aborts. Make signal and beginAbort available to finish by moving their declarations earlier or using a cleanup callback, then call signal.removeEventListener('abort', beginAbort) before completing settlement; preserve the existing immediate-abort behavior.server/modules/providers/list/pi/pi-runtime.provider.test.ts (1)
641-642: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAwait
client.start()in T28.The assertion depends on
createClientrunning synchronously insidestart()before the firstawait. That holds today, but any futureawaitplaced ahead ofcreateClientinPiRpcClient.start()makes this assertion read an emptycapturedarray.voidalso discards a rejection. Await the promise so the test stays valid and reports failures.♻️ Proposed change
-test('T28: default RPC client spawns with --no-extensions', () => { +test('T28: default RPC client spawns with --no-extensions', async () => {- void client.start(); + await client.start(); assert.ok(captured.includes('--no-extensions'), 'runtime spawn flags include --no-extensions');🤖 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 `@server/modules/providers/list/pi/pi-runtime.provider.test.ts` around lines 641 - 642, Update the T28 test to await the promise returned by client.start() before asserting captured runtime flags. Replace the void invocation so start failures propagate through the test and the assertion remains valid if PiRpcClient.start() becomes asynchronous before createClient.server/modules/providers/tests/provider-token-usage.service.test.ts (1)
174-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the latest-snapshot behavior.
Lines 179-205 write one usage snapshot. An implementation that returns the first snapshot would pass this test. Add a distinct earlier usage snapshot and keep the assertion on the later values.
🤖 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 `@server/modules/providers/tests/provider-token-usage.service.test.ts` around lines 174 - 223, Update the T22 test in “Pi token usage returns the last valid usage snapshot” to write two valid assistant usage messages with distinct token values, placing the asserted values in the later snapshot. Keep the existing expected assertion unchanged so the test specifically verifies that getSessionTokenUsage returns the latest snapshot rather than the first.server/modules/websocket/tests/shell-websocket.service.test.ts (1)
121-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the Pi resume branch.
This test covers only the fresh-session path, where
resolveProviderSessionIdreturnsnull. Thepi --session "<id>"branch atshell-websocket.service.tslines 220-222 stays untested. That branch builds the resume id into the spawned command, so it is the branch most worth asserting.🧪 Suggested additional test
test('Pi agent terminals resume with the resolved session id', () => { const pty = createFakePty(); const socket = createFakeSocket(); let spawnedArguments: string[] = []; const dependencies = { resolveProviderSessionId: () => 'pi-resume-123', spawnPty: (_shell: string, args: string | string[]) => { spawnedArguments = typeof args === 'string' ? [args] : args; return pty as never; }, }; handleShellConnection(socket as never, dependencies); socket.emit( 'message', JSON.stringify({ type: 'init', projectPath: process.cwd(), sessionId: 'pi-resume-123', hasSession: true, provider: 'pi', }), ); assert.match(spawnedArguments.join(' '), /pi --session "pi-resume-123"/); assert.ok(socket.frames.some((frame) => frame.includes('Resuming Pi session'))); pty.emitExit(); });🤖 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 `@server/modules/websocket/tests/shell-websocket.service.test.ts` around lines 121 - 150, Add a test alongside the existing Pi fresh-session test that configures resolveProviderSessionId to return a session ID and initializes a Pi connection with hasSession true. Assert the spawned command includes the resolved ID in the pi --session branch and that the socket reports resuming the Pi session, then emit the PTY exit to preserve cleanup.server/modules/websocket/services/shell-websocket.service.ts (1)
220-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a fallback when the Pi resume fails.
The
codexandclaudebranches fall back to a fresh session when the resume command exits non-zero (codex resume "<id>" || codex). Thepibranch returns onlypi --session "<id>". If the session id no longer exists in Pi, the terminal shows an error and stops.cursorandopencodehave the same gap, so treat this as optional consistency work.🤖 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 `@server/modules/websocket/services/shell-websocket.service.ts` around lines 220 - 222, Update the resume-command construction in the shell service so the pi branch falls back to a fresh pi session when resuming the specified session exits non-zero, matching the existing codex and claude behavior. Optionally apply the same fallback pattern to the cursor and opencode branches, while preserving normal resume behavior when the session exists.
🤖 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 `@openspec/changes/add-pi-provider/design.md`:
- Around line 57-59: Update openspec/changes/add-pi-provider/design.md lines
57-59 so every Pi MCP operation, including read/list and write operations,
returns ERR-PROVIDER-CAPABILITY-UNSUPPORTED instead of an empty successful read
result. Update openspec/changes/add-pi-provider/test-definition.md line 62 to
add a read/list MCP test that requires the same unsupported error, while
retaining write coverage.
In `@openspec/changes/add-pi-provider/test-definition.md`:
- Around line 23-29: Update the normal-path coverage table in the test
definition to include nine samples, adding named entries for the
installed-and-authenticated status scenario and Pi session synchronizer
discovery with failure isolation. Add corresponding acceptance-gate entries that
verify both behaviors, while preserving the existing sample counts and coverage
categories.
In `@openspec/changes/refactor-provider-seams/design.md`:
- Around line 54-55: 明确 provider-qualified 唯一性与迁移规则：在
openspec/changes/refactor-provider-seams/design.md（54-55、124-125）规定迁移前同 provider
重复行的确定性 survivor、关联记录外键重映射、history 保留及冲突处理，并要求单事务完成；在
openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md（49-51）规定未来同
provider 重复写入的拒绝或显式 upsert行为，且不同 provider 的相同 native ID 必须保留为两行；在
openspec/changes/refactor-provider-seams/tasks.md（38-39）加入 provider_session_id
IS NOT NULL 过滤、事务边界及 provider-qualified merge 断言；在
openspec/changes/refactor-provider-seams/test-definition.md（35-43）补充 R6 同
provider 规则和 R14 跨 provider 保留两行的验证。
In `@openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md`:
- Around line 21-37: 统一错误码契约：在
openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md:21-37
明确区分需求 ID 与实际返回字段和值；在 openspec/changes/refactor-provider-seams/design.md:91-98
统一 ERR 标识、AppError.code 与 HTTP 状态码的映射；在
openspec/changes/refactor-provider-seams/test-definition.md:30-33 断言实际返回字段及其精确
code 值，而非仅断言 ERR-* 别名。
In `@openspec/changes/refactor-provider-seams/test-definition.md`:
- Around line 57-59: 将“边界”覆盖率要求从“≥ 90%”提高为“100%”，确保规范性场景 R7 和 R9 必须全部通过；保留该行对
R7、R9 等场景的引用，不降低其他类别的现有门槛。
In `@server/modules/providers/list/pi/pi-auth.provider.ts`:
- Around line 77-80: Replace the synchronous spawnSync version probe in
checkInstalled(), used by getStatus(), with an asynchronous child-process call
that preserves the existing pi --version arguments and applies
VERSION_TIMEOUT_MS. Update the surrounding call chain as needed so the async
result is awaited without blocking the event loop.
In `@server/modules/providers/list/pi/pi-rpc-client.provider.ts`:
- Around line 78-91: Update the onClose handler around RpcClient.process to log
a warning when the child process field is unavailable, and add a startup
assertion or bounded watchdog in the surrounding runtime so missing exit
notifications cannot leave the request unresolved. Preserve the existing exit
listener cleanup and ERR-PI-RUN-FAILED behavior when the process is available.
In `@server/modules/providers/list/pi/pi-session-store.provider.ts`:
- Around line 82-85: Update PiSessionStoreProvider.load and the callers
PiSessionsProvider.fetchHistory and PiTokenUsageProvider.getTokenUsage to parse
session files off the Node.js event loop using an asynchronous bounded/streaming
parser or worker-based implementation. Avoid full-file synchronous reads,
splitting, or JSON.parse on the main thread, and preserve the existing
PiSessionSnapshot behavior.
In `@server/modules/providers/list/pi/pi-session-synchronizer.provider.ts`:
- Around line 33-43: The Pi session synchronization path currently performs
synchronous file reads and JSONL parsing during scanning. Update
PiSessionStore.load and the loop in the session synchronizer to use an
asynchronous, non-blocking load/parse path, await the result before incrementing
processed, and preserve per-file error handling; alternatively remove this
synchronizer from HTTP/WS execution if that is the established design.
In `@server/modules/providers/list/pi/pi.provider.ts`:
- Around line 42-54: Update withProbe so client.start() executes inside the
try/finally lifecycle, ensuring client.close(MODELS_PROBE_GRACE_MS) runs when
startup rejects. Pass client directly to fn without the unknown-to-PiModelsProbe
cast, preserving type checking through PiRpcClient’s exposed probe methods.
In `@src/components/settings/view/tabs/agents-settings/AgentsSettingsTab.tsx`:
- Around line 25-26: The Pi tab list exposes permissions without corresponding
content, resulting in an empty tab. Update the selectedAgent handling in
AgentsSettingsTab so Pi permissions is only exposed once
AgentCategoryContentSection supports a Pi renderer; preferably add the Pi
permissions content path there, otherwise remove permissions from Pi’s returned
categories until implemented.
---
Minor comments:
In `@docs/pi-provider-integration-plan.md`:
- Around line 3-7: Update the Pi implementation status documentation: in
docs/pi-provider-integration-plan.md lines 3-7, mark the plan implemented or
historical; in TODO.md lines 3-4, remove the completed Pi integration task or
replace it with remaining work; and in openspec/changes/add-pi-provider/tasks.md
lines 21-61, mark completed tasks or identify the checklist as historical,
clearly reflecting the delivered backend and frontend behavior.
In `@openspec/changes/refactor-provider-seams/test-definition.md`:
- Line 20: Update the normal-path entry in the test-definition table so the
stated sample count matches the described five capability-contract scenarios
plus four provider characterization smoke scenarios, or revise the description
to explicitly define the grouping represented by the count.
In `@server/modules/providers/list/pi/pi-auth.provider.ts`:
- Around line 98-100: Update probeAuthenticated around client.start() and
client.getAvailableModels() to enforce an application-level deadline covering
both operations, rejecting when the deadline expires. Ensure timeout or RPC
failures propagate through the existing error path so the client is still closed
and the probe settles.
In `@server/modules/providers/list/pi/pi-models.provider.ts`:
- Around line 88-90: Update the default model calculation in the provider’s
state-model lookup so an empty stateModel cannot become the selected default.
Normalize or otherwise treat empty stateModel as absent before applying the
matching option lookup and fallback to options[0].value, while preserving valid
non-empty model selections.
- Around line 79-85: Update both AppError constructions in the Pi provider
authentication flow to use the established English message convention instead of
the Chinese text, while preserving the existing PI_NOT_AUTHENTICATED error code
and control flow.
In `@server/modules/providers/list/pi/pi-session-store.provider.ts`:
- Around line 133-139: Update the snapshot construction in the Pi session store
provider so header, entries, messages, currentModel, and lastUsage are deeply
immutable, including nested objects and arrays rather than only their outer
containers. Reuse or add a deep-freeze/immutable-copy helper as appropriate, and
add coverage that attempts nested mutations such as entries, message fields, and
lastUsage.cost and verifies the snapshot remains unchanged.
- Around line 153-165: Update parseHeader, isValidEntry, and isCompleteUsage to
validate every field required by PiSessionHeader, entry, and usage types before
casting parsed JSON records. Require id, timestamp, and cwd for version-3
headers, require timestamp on entries, and ensure usage cost contains all fields
declared required rather than accepting only total; alternatively loosen the
exported types to match the accepted schema.
- Around line 116-124: Update the invalid-entry handling in the Pi session
parsing flow to throw PI_SESSION_CORRUPT for every parsed record that fails
isValidEntry(), including a complete JSON record at EOF without a trailing
newline. Restrict the isLastLine && !endsWithNewline continuation to incomplete
JSON handling before validation, and add a regression test covering a valid JSON
EOF record with an invalid entry shape.
In `@server/modules/providers/services/provider-token-usage.service.ts`:
- Around line 315-334: Update the Pi-session guard in the token-usage flow to
verify that the persisted file referenced by session.jsonl_path still exists,
rejecting both empty and stale paths with the existing SESSION_FILE_NOT_FOUND
AppError before calling PiTokenUsageProvider.getTokenUsage. Add a test covering
a nonempty jsonl_path whose file is absent.
In `@src/components/onboarding/view/subcomponents/AgentConnectionsStep.tsx`:
- Around line 49-55: Keep an unauthenticated Pi login entry point reachable:
update the Pi configuration in AgentConnectionsStep.tsx (lines 49-55) by
removing hideLogin or adding a visible custom action that invokes
onOpenProviderLogin. Apply the equivalent reachability fix to the Pi entry in
AccountContent.tsx (line 123), if it uses the same hidden-login configuration.
---
Nitpick comments:
In `@server/modules/providers/list/claude/claude-runtime.provider.js`:
- Around line 212-218: Update the model precedence in the SDK options assignment
near resolveClaudeEffort so the explicit options.model selection takes priority
over process.env.ANTHROPIC_MODEL, using the environment value only when no UI
model is selected and retaining CLAUDE_FALLBACK_MODELS.DEFAULT as the final
fallback.
In `@server/modules/providers/list/pi/pi-rpc-client.provider.test.ts`:
- Around line 223-235: Gate the real-spawn test around PiRpcClient.start and
getAvailableModels behind an explicit environment flag so the default unit suite
remains hermetic, while preserving its cleanup through client.close. Configure
an explicit, clearly bounded test timeout for the enabled integration check so
stalled RPC handshakes fail promptly.
In `@server/modules/providers/list/pi/pi-runtime.provider.test.ts`:
- Around line 641-642: Update the T28 test to await the promise returned by
client.start() before asserting captured runtime flags. Replace the void
invocation so start failures propagate through the test and the assertion
remains valid if PiRpcClient.start() becomes asynchronous before createClient.
In `@server/modules/providers/list/pi/pi-runtime.provider.ts`:
- Around line 498-507: Update the run executor’s finish path to remove the abort
listener when the run settles, including normal completion, errors, and aborts.
Make signal and beginAbort available to finish by moving their declarations
earlier or using a cleanup callback, then call
signal.removeEventListener('abort', beginAbort) before completing settlement;
preserve the existing immediate-abort behavior.
In `@server/modules/providers/tests/provider-token-usage.service.test.ts`:
- Around line 174-223: Update the T22 test in “Pi token usage returns the last
valid usage snapshot” to write two valid assistant usage messages with distinct
token values, placing the asserted values in the later snapshot. Keep the
existing expected assertion unchanged so the test specifically verifies that
getSessionTokenUsage returns the latest snapshot rather than the first.
In `@server/modules/websocket/services/shell-websocket.service.ts`:
- Around line 220-222: Update the resume-command construction in the shell
service so the pi branch falls back to a fresh pi session when resuming the
specified session exits non-zero, matching the existing codex and claude
behavior. Optionally apply the same fallback pattern to the cursor and opencode
branches, while preserving normal resume behavior when the session exists.
In `@server/modules/websocket/tests/shell-websocket.service.test.ts`:
- Around line 121-150: Add a test alongside the existing Pi fresh-session test
that configures resolveProviderSessionId to return a session ID and initializes
a Pi connection with hasSession true. Assert the spawned command includes the
resolved ID in the pi --session branch and that the socket reports resuming the
Pi session, then emit the PTY exit to preserve cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e894c365-4981-42e9-b23d-d49f1fb20e4a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (106)
.gitignoreCONTEXT.mdTODO.mddocs/paseo-agent-switching-analysis.mddocs/paseo-provider-switch-design.mddocs/pi-provider-integration-plan.mdopenspec/changes/add-pi-provider/.openspec.yamlopenspec/changes/add-pi-provider/design.mdopenspec/changes/add-pi-provider/proposal.mdopenspec/changes/add-pi-provider/specs/pi-provider/spec.mdopenspec/changes/add-pi-provider/tasks.mdopenspec/changes/add-pi-provider/test-definition.mdopenspec/changes/refactor-provider-seams/.openspec.yamlopenspec/changes/refactor-provider-seams/design.mdopenspec/changes/refactor-provider-seams/proposal.mdopenspec/changes/refactor-provider-seams/specs/provider-seams/spec.mdopenspec/changes/refactor-provider-seams/tasks.mdopenspec/changes/refactor-provider-seams/test-definition.mdopenspec/config.yamlpackage.jsonserver/index.tsserver/modules/agent/agent.module.tsserver/modules/agent/agent.routes.tsserver/modules/agent/tests/agent.routes.test.tsserver/modules/commands/commands.routes.tsserver/modules/commands/tests/commands.test.tsserver/modules/providers/list/claude/claude-runtime.provider.jsserver/modules/providers/list/pi/index.tsserver/modules/providers/list/pi/pi-auth.provider.test.tsserver/modules/providers/list/pi/pi-auth.provider.tsserver/modules/providers/list/pi/pi-mcp.provider.test.tsserver/modules/providers/list/pi/pi-mcp.provider.tsserver/modules/providers/list/pi/pi-models.provider.test.tsserver/modules/providers/list/pi/pi-models.provider.tsserver/modules/providers/list/pi/pi-paths.provider.test.tsserver/modules/providers/list/pi/pi-paths.provider.tsserver/modules/providers/list/pi/pi-rpc-client.provider.test.tsserver/modules/providers/list/pi/pi-rpc-client.provider.tsserver/modules/providers/list/pi/pi-runtime.provider.test.tsserver/modules/providers/list/pi/pi-runtime.provider.tsserver/modules/providers/list/pi/pi-session-store.provider.test.tsserver/modules/providers/list/pi/pi-session-store.provider.tsserver/modules/providers/list/pi/pi-session-synchronizer.provider.test.tsserver/modules/providers/list/pi/pi-session-synchronizer.provider.tsserver/modules/providers/list/pi/pi-sessions.provider.test.tsserver/modules/providers/list/pi/pi-sessions.provider.tsserver/modules/providers/list/pi/pi-skills.provider.test.tsserver/modules/providers/list/pi/pi-skills.provider.tsserver/modules/providers/list/pi/pi-token-usage.provider.test.tsserver/modules/providers/list/pi/pi-token-usage.provider.tsserver/modules/providers/list/pi/pi.provider.test.tsserver/modules/providers/list/pi/pi.provider.tsserver/modules/providers/provider.registry.tsserver/modules/providers/provider.routes.tsserver/modules/providers/services/mcp.service.tsserver/modules/providers/services/provider-capabilities.service.tsserver/modules/providers/services/provider-token-usage.service.tsserver/modules/providers/services/session-synchronizer.service.tsserver/modules/providers/services/sessions-watcher.service.tsserver/modules/providers/services/sessions.service.tsserver/modules/providers/tests/mcp.test.tsserver/modules/providers/tests/provider-capabilities.service.test.tsserver/modules/providers/tests/provider-registry.test.tsserver/modules/providers/tests/provider-runtime.service.test.tsserver/modules/providers/tests/provider-token-usage.service.test.tsserver/modules/providers/tests/provider.routes.test.tsserver/modules/providers/tests/sessions-watcher-paths.test.tsserver/modules/websocket/services/shell-websocket.service.tsserver/modules/websocket/tests/shell-websocket.service.test.tsserver/shared/types.tssrc/components/auth/types.tssrc/components/auth/utils.tssrc/components/chat/constants/providerEffort.tssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatMessages.test.tssrc/components/chat/hooks/useChatMessages.tssrc/components/chat/hooks/useChatProviderState.tssrc/components/chat/types/types.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/components/chat/view/subcomponents/CommandResultModal.tsxsrc/components/chat/view/subcomponents/MessageComponent.tsxsrc/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsxsrc/components/llm-logo-provider/PiLogo.tsxsrc/components/llm-logo-provider/SessionProviderLogo.tsxsrc/components/mcp/types.tssrc/components/onboarding/view/subcomponents/AgentConnectionCard.tsxsrc/components/onboarding/view/subcomponents/AgentConnectionsStep.tsxsrc/components/provider-auth/hooks/useProviderAuthStatus.tssrc/components/provider-auth/types.tssrc/components/provider-auth/view/ProviderLoginModal.tsxsrc/components/settings/constants/constants.tssrc/components/settings/view/tabs/agents-settings/AgentListItem.tsxsrc/components/settings/view/tabs/agents-settings/AgentsSettingsTab.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentCategoryContentSection.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsxsrc/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsxsrc/components/skills/view/ProviderSkills.tsxsrc/i18n/locales/en/chat.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/fr/chat.jsonsrc/i18n/locales/fr/settings.jsonsrc/stores/sessionMessageReconciliation.test.tssrc/stores/sessionMessageReconciliation.tssrc/stores/useSessionStore.tssrc/types/app.ts
| **决策 3：Pi 提供「不支持」语义的 mcp facet，而非把 mcp 改成 optional。** | ||
| 理由：`IProvider` 当前强制 mcp（E2），改成 optional 属于被切掉的重构。Pi 的 mcp facet 读操作返回完整的分组空结构，写操作抛 `ERR-PROVIDER-CAPABILITY-UNSUPPORTED`。capabilities 矩阵里 Pi 手写 `supportsMcp:false`。 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return the unsupported error for every Pi MCP operation.
The design returns an empty successful result for Pi MCP reads. The normative spec requires ERR-PROVIDER-CAPABILITY-UNSUPPORTED for unsupported Pi MCP requests. The current test only verifies writes, so it permits the invalid read behavior.
openspec/changes/add-pi-provider/design.md#L57-L59: Make read and write MCP operations returnERR-PROVIDER-CAPABILITY-UNSUPPORTED.openspec/changes/add-pi-provider/test-definition.md#L62-L62: Add a read/list MCP test that requires the same error instead of an empty result.
📍 Affects 2 files
openspec/changes/add-pi-provider/design.md#L57-L59(this comment)openspec/changes/add-pi-provider/test-definition.md#L62-L62
🤖 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 `@openspec/changes/add-pi-provider/design.md` around lines 57 - 59, Update
openspec/changes/add-pi-provider/design.md lines 57-59 so every Pi MCP
operation, including read/list and write operations, returns
ERR-PROVIDER-CAPABILITY-UNSUPPORTED instead of an empty successful read result.
Update openspec/changes/add-pi-provider/test-definition.md line 62 to add a
read/list MCP test that requires the same unsupported error, while retaining
write coverage.
| | 维度 | 是否覆盖 | 样本数 | 说明 | | ||
| |---|---|---|---| | ||
| | 正常路径 | 是 | 9 | 每条 spec 需求的正常场景各一 | | ||
| | 异常 | 是 | 8 | 进程早关、协议错误、未认证、未安装、中间损坏、不支持版本、优雅超时、无 usage | | ||
| | 边界 | 是 | 5 | 单 chunk 多行、一行跨多 chunk、尾部半行、active leaf 回溯、最后 model_change | | ||
| | 对抗 | 是 | 4 | 畸形 JSON、stderr 污染 stdout parser、未知事件、已知事件非法 payload | | ||
| | 高风险 | 是 | 4 | abort 与 late native event 竞争仅一个终态、绑定先于首事件、进程 unexpected close reject pending、`--no-extensions` probe/runtime 一致 | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the missing required-path tests.
The table claims nine normal-path samples, but it enumerates eight. It also omits tests for the installed-and-authenticated status scenario and Pi session synchronizer discovery and failure isolation. Add named samples and acceptance-gate entries for these required behaviors.
🤖 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 `@openspec/changes/add-pi-provider/test-definition.md` around lines 23 - 29,
Update the normal-path coverage table in the test definition to include nine
samples, adding named entries for the installed-and-authenticated status
scenario and Pi session synchronizer discovery with failure isolation. Add
corresponding acceptance-gate entries that verify both behaviors, while
preserving the existing sample counts and coverage categories.
| **决策 4：native 身份加 `(provider, provider_session_id)` 唯一约束 + provider-qualified lookup/merge。** | ||
| 替代方案：仅靠 UUID 唯一性（即 add-pi-provider 现状）——被否，那是临时兜底；本 change 的职责就是根治（E7）。迁移前必须先合并真实库中的重复行（E12）。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
明确同 provider 重复 ID 的迁移规则和未来写入规则。
当前文档允许“拒绝或确定性合并”，但没有定义 survivor、外键重映射、history 保留和冲突处理。R14 还把跨 provider 的相同 native ID 描述为“provider-qualified 合并”。这不能形成可执行的数据安全测试。请明确：
-
迁移前同 provider 重复行如何确定性合并。
-
迁移如何重映射关联记录，并在单事务内完成。
-
未来同 provider 重复写入是拒绝还是显式 upsert。
-
不同 provider 的相同 native ID 必须保留为两行，不能合并。
-
重复查询必须使用
WHERE provider_session_id IS NOT NULL，否则所有未绑定 native ID 的行可能被当成同一组。 -
openspec/changes/refactor-provider-seams/design.md#L54-L55: 定义唯一约束、迁移 survivor 和关联记录处理。 -
openspec/changes/refactor-provider-seams/design.md#L124-L125: 将迁移风险转化为具体的合并和验证步骤。 -
openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md#L49-L51: 将迁移重复、未来写入和拒绝规则拆成独立规范。 -
openspec/changes/refactor-provider-seams/tasks.md#L38-L39: 添加非空过滤、事务边界和 provider-qualified merge 断言。 -
openspec/changes/refactor-provider-seams/test-definition.md#L35-L43: R6 验证同 provider 规则，R14 验证跨 provider 仍有两行。
📍 Affects 4 files
openspec/changes/refactor-provider-seams/design.md#L54-L55(this comment)openspec/changes/refactor-provider-seams/design.md#L124-L125openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md#L49-L51openspec/changes/refactor-provider-seams/tasks.md#L38-L39openspec/changes/refactor-provider-seams/test-definition.md#L35-L43
🤖 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 `@openspec/changes/refactor-provider-seams/design.md` around lines 54 - 55, 明确
provider-qualified 唯一性与迁移规则：在
openspec/changes/refactor-provider-seams/design.md（54-55、124-125）规定迁移前同 provider
重复行的确定性 survivor、关联记录外键重映射、history 保留及冲突处理，并要求单事务完成；在
openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md（49-51）规定未来同
provider 重复写入的拒绝或显式 upsert行为，且不同 provider 的相同 native ID 必须保留为两行；在
openspec/changes/refactor-provider-seams/tasks.md（38-39）加入 provider_session_id
IS NOT NULL 过滤、事务边界及 provider-qualified merge 断言；在
openspec/changes/refactor-provider-seams/test-definition.md（35-43）补充 R6 同
provider 规则和 R14 跨 provider 保留两行的验证。
| ### Requirement: 未注册 provider 与不支持能力的错误码区分 | ||
|
|
||
| The system SHALL 对"未注册的 provider"返回 `ERR-UNSUPPORTED-PROVIDER`，对"已注册但缺少某 facet"返回 `ERR-PROVIDER-CAPABILITY-UNSUPPORTED`，二者为不同的稳定错误。 | ||
|
|
||
| The system SHALL NOT 用空成功结果把"不支持"伪装成"支持但无数据"。 | ||
|
|
||
| #### Scenario: 未注册 provider | ||
| - **WHEN** 调用方以一个未注册的 provider id 请求任意 facet | ||
| - **THEN** 系统以 `ERR-UNSUPPORTED-PROVIDER` 拒绝 | ||
|
|
||
| #### Scenario: 已注册但 facet 不支持 | ||
| - **WHEN** 调用方对一个已注册 provider 请求其未提供的 facet | ||
| - **THEN** 系统以 `ERR-PROVIDER-CAPABILITY-UNSUPPORTED` 拒绝，而非返回空成功 | ||
|
|
||
| #### Scenario: 注册时 descriptor 非法 | ||
| - **WHEN** 注册一个默认权限模式不在其权限模式列表中的 provider | ||
| - **THEN** 系统在注册阶段以 `ERR-PROVIDER-DESCRIPTOR-INVALID` 拒绝，不进入可用集合 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
统一稳定的 wire error code。
spec.md 和 test-definition.md 使用 ERR-* 标识符作为返回的错误码，但 design.md 将相同标识符映射为不带 ERR- 的 AppError.code 值。请明确 ERR-* 是需求 ID 还是实际返回字段，并在 spec 和测试中使用精确的字段和值。
openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md#L21-L37: 区分需求 ID 与返回的code。openspec/changes/refactor-provider-seams/design.md#L91-L98: 统一 ERR ID、AppError.code和 HTTP 状态码的命名。openspec/changes/refactor-provider-seams/test-definition.md#L30-L33: 断言实际返回字段和值，不要只断言ERR-*别名。
📍 Affects 3 files
openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md#L21-L37(this comment)openspec/changes/refactor-provider-seams/design.md#L91-L98openspec/changes/refactor-provider-seams/test-definition.md#L30-L33
🤖 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 `@openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md` around
lines 21 - 37, 统一错误码契约：在
openspec/changes/refactor-provider-seams/specs/provider-seams/spec.md:21-37
明确区分需求 ID 与实际返回字段和值；在 openspec/changes/refactor-provider-seams/design.md:91-98
统一 ERR 标识、AppError.code 与 HTTP 状态码的映射；在
openspec/changes/refactor-provider-seams/test-definition.md:30-33 断言实际返回字段及其精确
code 值，而非仅断言 ERR-* 别名。
| | 契约异常全通过 | 100% | R2、R3、R4、R6、R12（错误码精确匹配） | | ||
| | 单一终态 | 100% | R10、R11、R12、R13 | | ||
| | 边界 | ≥ 90% | R7、R9 等 | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
对规范性边界场景要求 100% 通过。
spec.md 将 R7 和 R9 定义为 SHALL 行为。当前 ≥ 90% 门槛允许一个规范性场景失败后仍然放行。请将 R7、R9 的门槛设为 100%。较低门槛只适用于非规范性的诊断测试。
建议修改
-| 边界 | ≥ 90% | R7、R9 等 |
+| 边界 | 100% | R7、R9 |📝 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.
| | 契约异常全通过 | 100% | R2、R3、R4、R6、R12（错误码精确匹配） | | |
| | 单一终态 | 100% | R10、R11、R12、R13 | | |
| | 边界 | ≥ 90% | R7、R9 等 | | |
| | 契约异常全通过 | 100% | R2、R3、R4、R6、R12（错误码精确匹配） | | |
| | 单一终态 | 100% | R10、R11、R12、R13 | | |
| | 边界 | 100% | R7、R9 | |
🤖 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 `@openspec/changes/refactor-provider-seams/test-definition.md` around lines 57
- 59, 将“边界”覆盖率要求从“≥ 90%”提高为“100%”，确保规范性场景 R7 和 R9 必须全部通过；保留该行对 R7、R9
等场景的引用，不降低其他类别的现有门槛。
| onClose: (listener) => { | ||
| // The official client exposes its spawned ChildProcess as a public | ||
| // field once start() has run. Forward its `exit` to the runtime so a | ||
| // process death before agent_settled surfaces as a failure. | ||
| // `process` is a runtime-public field; the shipped .d.ts marks it | ||
| // private, so reach it through a narrow typed view. | ||
| const child = (client as unknown as { process: ChildProcess | null }).process; | ||
| if (!child) return () => {}; | ||
| const onExit = (): void => listener(); | ||
| child.once('exit', onExit); | ||
| return () => { | ||
| child.removeListener('exit', onExit); | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pkg=$(fd -t d -H 'pi-coding-agent' node_modules 2>/dev/null | head -n 1 || true)
if [ -z "$pkg" ]; then
echo "package not installed in sandbox; verify in an environment with dependencies installed"
exit 0
fi
fd -e d.ts -e ts . "$pkg" --exec rg -n -C3 '\bprocess\b.*ChildProcess|class RpcClient' {} \;Repository: siteboon/claudecodeui
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching target =="
fd -a 'pi-rpc-client\.provider\.ts|pi-runtime\.provider\.ts$' . || true
echo "== backend skill =="
if [ -f ".agents/skills/backend-module-standards/SKILL.md" ]; then
wc -l ".agents/skills/backend-module-standards/SKILL.md"
sed -n '1,220p' ".agents/skills/backend-module-standards/SKILL.md"
else
echo "backend skill missing"
fi
echo "== rpc client outline and relevant lines =="
file=$(fd 'pi-rpc-client\.provider\.ts' . | head -n 1 || true)
if [ -n "$file" ]; then
wc -l "$file"
ast-grep outline "$file" --match PiRpcClientProvider --view expanded || true
sed -n '1,140p' "$file"
fi
echo "== runtime relevant lines =="
runtime=$(fd 'pi-runtime\.provider\.ts' . | head -n 1 || true)
if [ -n "$runtime" ]; then
wc -l "$runtime"
sed -n '540,625p' "$runtime"
fi
echo "== package references to pi-coding-agent and local lock files =="
rg -n '"`@pi-coding-agent/client`"|"pi-coding-agent|rpc\.onClose|agent_settled|ERR-PI-RUN-FAILED' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' .Repository: siteboon/claudecodeui
Length of output: 22244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RpcClient imports and full PiRpcClient class =="
rpc_file=$(fd 'pi-rpc-client\.provider\.ts' . | head -n 1 || true)
if [ -n "$rpc_file" ]; then
sed -n '1,120p' "$rpc_file"
sed -n '120,200p' "$rpc_file"
fi
echo "== lockfile/package exact `@earendil-works/pi-coding-agent` versions =="
rg -n '"`@earendil-works/pi-coding-agent`"|(`@earendil-works/pi-coding-agent`@)' package.json package-lock.json bun.lockb npm-shrinkwrap.json pnpm-lock.yaml yarn.lock 2>/dev/null || true
echo "== runtime file size and state/finish helpers =="
runtime=$(fd 'pi-runtime\.provider\.ts' . | head -n 1 || true)
if [ -n "$runtime" ]; then
wc -l "$runtime"
sed -n '1,120p' "$runtime"
sed -n '440,510p' "$runtime"
sed -n '560,635p' "$runtime"
fi
echo "== runtime tests around process close =="
t=$(fd 'pi-runtime\.provider\.test\.ts' . | head -n 1 || true)
if [ -n "$t" ]; then
sed -n '420,460p' "$t"
fiRepository: siteboon/claudecodeui
Length of output: 17722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime main entry and resolve loop =="
runtime=$(fd 'pi-runtime\.provider\.ts' . | head -n 1 || true)
sed -n '360,460p' "$runtime"
sed -n '480,505p' "$runtime"
echo "== check installed package source if present =="
pkg=$(fd -t d -H 'pi-coding-agent' node_modules 2>/dev/null | head -n 1 || true)
if [ -n "$pkg" ]; then
echo "found package: $pkg"
rg -n -C3 'class RpcClient|process:|onClose|process' "$pkg" --glob '!node_modules/**'
fi
echo "== installed declarations for RpcClient process if present =="
if [ -n "$pkg" ]; then
fd -e d.ts . "$pkg" --exec sed -n
fiRepository: siteboon/claudecodeui
Length of output: 4457
Add a watchdog when RpcClient.process is unavailable.
If the shipped package changes and client.process is absent, onClose returns a no-op subscription. The runtime only returns ERR-PI-RUN-FAILED from rpc.onClose, so a Pi process death before agent_settled can leave the run unresolved. Log a warning when the field is missing, and add a startup assertion or bounded watchdog timeout in the runtime so a missing exit signal cannot hang the request.
🤖 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 `@server/modules/providers/list/pi/pi-rpc-client.provider.ts` around lines 78 -
91, Update the onClose handler around RpcClient.process to log a warning when
the child process field is unavailable, and add a startup assertion or bounded
watchdog in the surrounding runtime so missing exit notifications cannot leave
the request unresolved. Preserve the existing exit listener cleanup and
ERR-PI-RUN-FAILED behavior when the process is available.
| load(filePath: string): PiSessionSnapshot { | ||
| const raw = readFileSync(filePath, 'utf8'); | ||
| const endsWithNewline = raw.endsWith('\n'); | ||
| const lines = raw.split('\n'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Move session parsing off the Node.js event loop.
PiSessionsProvider.fetchHistory() and PiTokenUsageProvider.getTokenUsage() call this loader. readFileSync(), split(), and JSON.parse() process the complete JSONL file synchronously. A large session or slow disk stalls unrelated server work.
Use an asynchronous bounded parser or a worker-based parser. Do not only replace readFileSync(), because complete-file splitting and parsing still block the event loop.
🤖 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 `@server/modules/providers/list/pi/pi-session-store.provider.ts` around lines
82 - 85, Update PiSessionStoreProvider.load and the callers
PiSessionsProvider.fetchHistory and PiTokenUsageProvider.getTokenUsage to parse
session files off the Node.js event loop using an asynchronous bounded/streaming
parser or worker-based implementation. Avoid full-file synchronous reads,
splitting, or JSON.parse on the main thread, and preserve the existing
PiSessionSnapshot behavior.
| for (const filePath of files) { | ||
| try { | ||
| const sessionId = this.indexFile(filePath); | ||
| if (sessionId) { | ||
| processed += 1; | ||
| } | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| console.warn('[PiProvider] Failed to synchronize session file:', filePath, message); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether PiSessionStore.load uses synchronous filesystem IO.
fd -t f 'pi-session-store.provider.ts' server | xargs -r ast-grep outline --items all
fd -t f 'pi-session-store.provider.ts' server | xargs -r rg -n -C3 'readFileSync|readFile|statSync|createReadStream'Repository: siteboon/claudecodeui
Length of output: 1737
Make Pi session scanning asynchronous before using the result.
PiSessionStore.load(filePath) reads the file synchronously and parses JSONL synchronously. The session synchronizer calls it inside a file-scanned loop, so large or numerous transcript files block event-loop work while scanning. If keeping this synchronizer, remove it from the HTTP/WS flow or convert PiSessionStore.load / this loop to an async path that does not call synchronous file read and parse inline.
🤖 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 `@server/modules/providers/list/pi/pi-session-synchronizer.provider.ts` around
lines 33 - 43, The Pi session synchronization path currently performs
synchronous file reads and JSONL parsing during scanning. Update
PiSessionStore.load and the loop in the session synchronizer to use an
asynchronous, non-blocking load/parse path, await the result before incrementing
processed, and preserve per-file error handling; alternatively remove this
synchronizer from HTTP/WS execution if that is the established design.
| async withProbe(fn) { | ||
| const client = new PiRpcClient(); | ||
| await client.start(); | ||
| try { | ||
| return await fn(client as unknown as PiModelsProbe); | ||
| } finally { | ||
| try { | ||
| await client.close(MODELS_PROBE_GRACE_MS); | ||
| } catch { | ||
| // ignore close failures during probe teardown | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify PiRpcClient's public surface against PiModelsProbe (getAvailableModels/getState/start/close).
fd -t f 'pi-rpc-client.provider.ts' server | xargs -r ast-grep outline --items all
fd -t f 'pi-rpc-client.provider.ts' server | xargs -r rg -n -C3 'async (start|close|getAvailableModels|getState)\s*\('Repository: siteboon/claudecodeui
Length of output: 1499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -t f 'pi\.provider\.ts$|pi-rpc-client\.provider\.ts$|pi-.*probe|models-rpc|ModelsRpc' server
echo
echo "pi.provider.ts relevant section:"
sed -n '1,120p' server/modules/providers/list/pi/pi.provider.ts
echo
echo "pi-rpc-client.provider.ts relevant section:"
sed -n '1,230p' server/modules/providers/list/pi/pi-rpc-client.provider.ts
echo
echo "Search for PiModelsProbe/interface/adapter:"
rg -n "PiModelsProbe|PiModelsRpc|getAvailableModels|getState|RpcClientDeps|withProbe" serverRepository: siteboon/claudecodeui
Length of output: 15499
Close the PiRpcClient when start() rejects, and stop suppressing its probe surface.
Move client.start() inside the probe lifecycle or initialize it in the same try; otherwise a rejected start never reaches finally, leaving the spawned Pi subprocess alive. Also pass client directly to fn; client already exposes getAvailableModels() and getState() via PiRpcClient, so as unknown as PiModelsProbe hides the type check.
🤖 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 `@server/modules/providers/list/pi/pi.provider.ts` around lines 42 - 54, Update
withProbe so client.start() executes inside the try/finally lifecycle, ensuring
client.close(MODELS_PROBE_GRACE_MS) runs when startup rejects. Pass client
directly to fn without the unknown-to-PiModelsProbe cast, preserving type
checking through PiRpcClient’s exposed probe methods.
| if (selectedAgent === 'pi') { | ||
| return ['account', 'permissions', 'skills']; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not expose an empty Pi Permissions tab.
When selectedAgent === 'pi', this list includes permissions. However, AgentCategoryContentSection.tsx renders permission content only for claude, cursor, and codex; it has no Pi branch. Selecting Pi and Permissions therefore renders an empty content area. Add the Pi permissions renderer before exposing this tab, or remove permissions until the feature exists. The PR objective includes Pi permission modes.
🤖 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 `@src/components/settings/view/tabs/agents-settings/AgentsSettingsTab.tsx`
around lines 25 - 26, The Pi tab list exposes permissions without corresponding
content, resulting in an empty tab. Update the selectedAgent handling in
AgentsSettingsTab so Pi permissions is only exposed once
AgentCategoryContentSection supports a Pi renderer; preferably add the Pi
permissions content path there, otherwise remove permissions from Pi’s returned
categories until implemented.
|
hey @keenJoe, make sure to do the following:
After doing this, resubmit the PR |
2 participants
add pi agent
Summary by CodeRabbit