Skip to content

feat: add Pi Coding Agent support#862

Open
zhushanwen321 wants to merge 94 commits into
tiann:mainfrom
zhushanwen321:feat-pi-support
Open

feat: add Pi Coding Agent support#862
zhushanwen321 wants to merge 94 commits into
tiann:mainfrom
zhushanwen321:feat-pi-support

Conversation

@zhushanwen321

Copy link
Copy Markdown

Summary

Closes #840

Add Pi Coding Agent as a first-class agent flavor. Pi has a built-in --mode rpc that speaks JSONL over stdin/stdout. The integration follows the same subprocess pattern as Codex — spawn agent process, communicate via stdio pipes, receive real-time event stream. No ACP dependency.

Feature Coverage

Core features

Feature Status Key files
Register as agent flavor shared/modes.ts flavors.ts schemas.ts
CLI command (hapi pi) cli/commands/pi.ts registry.ts
Session create / resume / kill cli/pi/session.ts loop.ts runPi.ts
Text messaging + streaming output piTransport.ts piMessageAccumulator.ts piEventConverter.ts
Abort generation loop.ts
Model switching runPi.ts — provider learned from get_state
Tool call display piEventConverter.ts
Dynamic model discovery CLI auto-discovery → hub callPiRpc → web usePiModels.ts → model dropdown grouped by provider
Thinking Level (6 levels) PiThinkingLevelPanel.tsx — auto-filters levels per model's capability
Permission mode (default / yolo) PiPermissionPanel.tsx
Dedicated Pi composer toolbar Two Pi-specific pill buttons in composer toolbar (model selector + thinking level) with dropdown panels (PiModelPanel.tsx + PiThinkingLevelPanel.tsx). PiPermissionPanel.tsx in settings popover
Web UI agent icon AgentFlavorIcon.tsx
Context budget bar modelConfig.ts — 200K window

Extended features

Feature Status Notes
Skills / slash commands ✅ Full stack CLI: get_commands → generic listSlashCommands → web autocomplete
Steer (mid-stream steering) ✅ Transparent User messages auto-routed as steer when Pi is streaming

Not implemented (out of scope)

Follow-up, Queue modes, History replay, Compact, Fork, Clone, Switch session, Session stats, HTML export. Wireable via callPiRpc / callPiEndpoint generics when needed.

New vs Modified Files

21 new files:

  • cli/src/pi/ — 8 source files + 5 test files
  • web/ — 6 Pi UI components + usePiModels.ts hook

28 modified existing files across all 4 packages. All changes follow one of three zero-impact patterns:

  1. Enum / Record extension — add 'pi' key, no existing entries changed
  2. If-else branch addition=== 'pi' appended at chain tail
  3. New method additioncallPiRpc<T> / callPiEndpoint<T>, no existing methods modified

Notable hub-side changes (affect all agents)

  • sessions.ts effort endpoint uses supportsEffort() from shared flavors instead of hardcoded flavor !== 'claude'
  • syncEngine.applySessionConfig surfaces actual CLI error messages instead of generic "Missing applied session config"
  • sessionCache normalizes { provider, modelId } model object to plain string for DB storage (prevents [object Object])

Notable shared changes

  • SessionModelRequestSchema accepts { provider, modelId } object form in addition to plain string
  • MetadataSchema adds optional piAvailableModels for offline model list fallback
  • PiModelSummary includes reasoning and thinkingLevelMap fields for per-model thinking level filtering

Known Limitation

Pi's RPC mode has no tool permission approval mechanism — tools execute automatically. HAPI's "approve tool calls from phone" won't work for Pi. The permission selector offers default and yolo only.

Test Coverage

  • Unit tests: 29 cases in cli/src/pi/, all pass
  • E2E protocol tests: 15 cases against live Pi RPC, 12 pass
  • Automated: bun run typecheck all 3 packages pass, bun run build:web succeeds

zhushanwen321 and others added 30 commits June 6, 2026 01:53
- PiTransport: spawn pi --mode rpc, JSONL stdio, ENOENT/EPIPE handling
- PiEventConverter: Pi AgentEvent → HAPI AgentMessage conversion
- runPi: session lifecycle, dual-track event routing, model switching
- pi command: CLI registration with PI_PERMISSION_MODES
- Shared: add 'pi' to AGENT_FLAVORS, FLAVOR_CAPS, FLAVOR_LABELS

30 tests passing (15 transport + 15 converter)
…safety net

- Add cli/src/pi/types.ts with PiAgentEvent/PiResponseEvent discriminated unions
- PiTransport: constructor uses options object, double-start guard, drop log
- PiEventConverter: typed events via type assertions, top-level try/catch
- runPi: safeCleanup guard prevents double-cleanup race, sendAgentMessage
  for converted events, keepAlive() for session pings
- 33 tests passing
- Business logic review: pass (0 must_fix)
- Standards review: pass (0 must_fix)
- Taste review: P0 types issue fixed in code
- Robustness review v2: pass (v1 3 MUST_FIX all fixed)
- Integration review: pass (0 must_fix)
- Test results: 33 passing, all type errors resolved
- PiTransport: buffer cross-chunk reassembly test
- PiEventConverter: tool_execution_end with missing result/toolCallId
- handleResponse: 10 tests covering all branches (error, get_state,
  set_model, new_session, abort, prompt, unknown command)
- Extract handleResponse to accept onUpdate callback for testability
- Total: 46 tests passing (was 33)
…i RPC capture

- TC-4-01 to TC-4-15: manual tests covering tool execution, thinking
  lifecycle, multi-turn, abort, error scenarios, model switch, cleanup
- Priority: P0 (tool fields, failure, thinking, multi-turn, abort)
  > P1 (basic conversation, write tool, model switch, usage) > P2 (edge cases)
- Includes actual Pi RPC event sequence from live capture as reference
- e2e-test-plan.md updated with test environment setup instructions
- Total test cases: 35 (6 unit + 14 integration + 15 manual)
P0/P1 automated tests (8/8 pass):
- TC-4-01: Basic text conversation ✓
- TC-4-02: Tool read (field names verified) ✓
- TC-4-03: Tool write (file created) ✓
- TC-4-04: Tool failure (isError=true) ✓
- TC-4-05: Thinking lifecycle + usage ✓
- TC-4-06: Multi-turn context retention ✓
- TC-4-07: Abort generation ✓
- TC-4-14: Token count ✓
- TC-4-15: Extension UI events ignored ✓

P2 results:
- TC-4-10: Invalid token → 401 ✓
- TC-4-12: Ctrl+C cleanup, no orphans ✓
- TC-4-08: ENOENT (harness issue, exit code correct)
- TC-4-11: set_model not supported by Pi (success=false)
- TC-4-13: Pi crash (harness output capture issue)
…odelId

Previous test used invalid provider='' + modelId='deepseek-chat'.
Re-tested with provider='deepseek' + modelId='deepseek-v4-flash':
- set_model success=true
- model switched glm-5.1 → deepseek-v4-flash
- subsequent prompt confirmed working

Final E2E results: 12/15 PASS, 2 FAIL (test harness), 1 SKIP
Local harness workflow artifacts should not be tracked in the repo.
Five bugs fixed for end-to-end pi session via hapi web UI:

1. runner buildCliArgs: add 'pi' branch to spawn correct command
   (was falling back to 'claude', launching wrong agent)
2. runPi: implement real keep-alive (2s interval) to prevent hub
   30s timeout marking session inactive
3. runPi: bump keep-alive to active state during agent/turn_start
4. sessionResume: add 'pi' to flavor switch and resume condition
   (was returning undefined, causing 'cannotResume' on inactive session)
5. PiEventConverter: emit codex-compatible {type:'message',message:...}
   /{type:'reasoning',message:...} with streamId; dedup by skipping
   text_start/text_end (only send deltas) to avoid triple-rendered text
6. PiTransport: fallback to stdout 'end' event when child process
   close event doesn't fire (bun spawn quirk)

Verified end-to-end: web UI shows pi reasoning + reply correctly,
session stays online, no duplicate text.
Three of four follow-up bugs reported after the initial fix (6c28949):

1. Stuck in 'queued' status — fix
   Pi's runner doesn't use MessageQueue2, so the base session's
   onBatchConsumed hook never fires. Add a FIFO of pending localIds
   in runPi and emit messages-consumed on agent_start. turn_start
   is intentionally skipped (it can fire multiple times per agent
   run after tool calls). A prompt rejection from Pi also consumes
   the localId so the next prompt isn't poisoned.

2. AI thinking only displays ':' — fix
   Pi emits pure incremental deltas (text_delta / thinking_delta)
   per token. The web reducer dedupes reasoning by streamId WITHIN
   one message's content array only — separate wire messages
   produce separate renders. Without accumulation, 50 deltas = 50
   reasoning renders, of which the reducer keeps only the last
   delta (a single character like ':').

3. Output text on separate lines — fix
   Same root cause as tiann#2 but for text: the reducer appends each
   text AgentMessage as a new agent-text block (no dedup), so 50
   deltas become a 50-row character-by-character column.

4. Tool call execution status (in_progress -> completed)
   The tool result wire CodexMessage type is 'tool-call-result'
   (with callId + is_error?); the internal AgentMessage 'tool_result'
   is converted to that. Status mapping is preserved.

Implementation: extract a PiMessageAccumulator class (testable in
isolation) that mirrors codex's ReasoningProcessor pattern:
- message_start resets state and streamId
- text_delta / thinking_delta append to internal text / reasoning
- text_start/thinking_start/text_end/thinking_end ignored (they
  carry full partial state — would duplicate)
- message_end flushes (max 1 reasoning + 1 text message, in order)
- turn_end safety net flushes if active
- flushIfActive() exposed for transport close / crash

The converter now routes AgentMessage through convertAgentMessage
so the wire format is codex-shaped (matches opencode/gemini/kimi
path). AgentMessage 'text' and CodexMessage 'message' both gain
optional id; convertAgentMessage preserves caller-provided id for
streamId-based dedup on the web side.

Tests: 16 new PiMessageAccumulator tests + 5 updated
PiEventConverter tests + 4 messageConverter tests, all passing.
Full suite: 909/910 (1 unrelated macOS path normalization). tsc
clean.
The web session-resume helper referenced metadata.piSessionId, but the
shared MetadataSchema does not define the field, and the back-end has no
path to populate it (Pi session resume is out of scope per spec.md).
This caused web typecheck to fail and would also have produced a
runtime 'resume_unavailable' from the hub if a user tried to resume a Pi
session that had any user messages (the stale 'flavor === pi' branch in
inactiveSessionCanResume claimed resume was supported).

Revert the two early Pi branches from the web resume helper. Add a
comment pointing at the spec and noting what to undo when back-end
resume ships (re-add 'case pi' + 'piSessionId' on MetadataSchema +
extend hub resolveAgentResumeId).
1. cli/src/runner/run.ts buildCliArgs: stop forwarding --resume to the pi
   binary. Pi session resume is out of scope (no piSessionId on
   Metadata), so forwarding would create an orphan session the hub can't
   track. Hub already returns null from resolveAgentResumeId for
   flavor='pi' and falls through to fresh spawn; this just hardens the
   runner layer to match.

2. cli/src/pi/runPi.ts: cache currentProvider from get_state and use it
   for subsequent set_model RPCs. Pi's set_model requires both provider
   and modelId, but the bootstrap-time code emitted provider: '' which
   Pi rejects. The bootstrap-time model is still applied by Pi at
   startup, so suppressing set_model until get_state arrives is a no-op
   for same-model configs rather than a wrong-model emit.

3. web/src/components/AssistantChat/modelOptions.ts: add explicit pi
   branches to getModelOptionsForFlavor and getNextModelForFlavor.
   Without them, Pi sessions fell through to the Claude preset cycler,
   which would push sonnet/opus ids into a Pi session via
   set-session-config. Mirrors the opencode handling introduced earlier.

Tests added/updated: buildCliArgs covers pi + claude resume; handleResponse
mirror test covers provider caching; modelOptions tests cover pi
no-fallback behavior for both option list and cycler.
- Add piSessionId to MetadataSchema (shared/src/schemas.ts)
- Persist piSessionId from get_state response to metadata (cli/src/pi/runPi.ts)
- Pass --session-id to Pi spawn on resume (cli/src/pi/runPi.ts)
- Add pi branch to resolveAgentResumeId (hub/src/sync/syncEngine.ts)
- Add case 'pi' to resolveAgentSessionIdFromMetadata (web/src/lib/sessionResume.ts)
- Replace pi resume skip guard with --session-id forwarding (cli/src/runner/run.ts)
- Preserve piSessionId in pickExistingSessionMetadata (cli/src/agent/sessionFactory.ts)
- Add pi badge to AgentFlavorIcon (web/src/components/AgentFlavorIcon.tsx)
- Fix transport.onClose crash-marking on normal shutdown (cli/src/pi/runPi.ts)
- resume.ts: add pi branch to dispatchLocalResume() so hapi resume
  dispatches to runPi instead of falling through to cursor
- runPi.ts: accept existingSessionId and use bootstrapExistingSession
  when resuming, matching other agents' pattern
- agentCommandOptions.ts: parse --session-id in addition to --resume
  so runner-spawned pi resume actually forwards the session ID
- types.ts: export PiPermissionMode alongside other agent permission
  mode types for consistent import convention
- Switch from structured output to file-based JSON output for reliability
- Replace per-round file limit (20→30) with clear wording (remove misleading split-commits instruction)
- Return { data, error } from readResultFile() to surface parse/validation failures in abortReason
- Fix lastMustFix sentinel: initialize to null, use ?? for explicit N/A reporting
- Add getAgentDirs() to dynamically discover agent dirs from cli/src/
- Document rollbackTo() atomic-round design intent
- Add isValidIssue() validation, runFinalCleanup() helper, git repo pre-check
The metadata whitelist rebuild kept piAvailableModels but omitted
piSelectedModel, so the first resume/local-handoff update dropped the
provider identity — after which web fell back to modelId-only matching
and could select the wrong provider for duplicate modelIds.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Wait for Pi to confirm config changes before persisting them — why it matters: the active-session model/effort endpoints trust the CLI's SetSessionConfig response and immediately persist whatever is returned, but the Pi handler returns after only writing JSONL to stdin. If Pi rejects an invalid provider/model or thinking level, or the subprocess is already gone, the hub still stores the new model/effort and the UI reports success even though Pi kept the old runtime state. Evidence cli/src/pi/runPi.ts:186.
    Suggested fix:
    if (config.model !== undefined && piSession.currentModel && piSession.currentProvider) {
        await sendPiRpcAndWait(transport, { type: 'set_model', provider: piSession.currentProvider, modelId: piSession.currentModel })
    }
    if (config.effort !== undefined) {
        await sendPiRpcAndWait(transport, { type: 'set_thinking_level', level: piSession.currentThinkingLevel ?? 'off' })
    }

Questions

  • None.

Summary

  • Review mode: follow-up after new commits
  • One Major issue remains: active Pi model/effort changes are acknowledged to the hub before the Pi subprocess confirms them, so persisted session state can diverge from the actual agent runtime.

Testing

  • Not run (automation). Suggested coverage: a Pi SetSessionConfig test where the Pi set_model/set_thinking_level response is success: false should reject the web request and leave session model/effort unchanged.

HAPI Bot

Comment thread cli/src/pi/runPi.ts Outdated

// Forward changes to Pi process
if (piSession.currentModel && piSession.currentProvider) {
transport.send({ type: 'set_model', provider: piSession.currentProvider, modelId: piSession.currentModel });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Wait for Pi to confirm config changes before persisting them

The active-session /model and /effort paths trust this SetSessionConfig return value and immediately persist it, but this handler only writes set_model/set_thinking_level to Pi's stdin and returns before the corresponding Pi response says whether the command succeeded. If Pi rejects an invalid provider/model or thinking level, or the subprocess is already gone, HAPI still stores the new model/effort and the UI reports success while Pi continues with the old runtime state.

Suggested fix:

if (config.model !== undefined && piSession.currentModel && piSession.currentProvider) {
    await sendPiRpcAndWait(transport, { type: 'set_model', provider: piSession.currentProvider, modelId: piSession.currentModel })
}
if (config.effort !== undefined) {
    await sendPiRpcAndWait(transport, { type: 'set_thinking_level', level: piSession.currentThinkingLevel ?? 'off' })
}

SetSessionConfig was fire-and-forget — transport.send wrote JSONL to
stdin and returned immediately. If Pi rejected an invalid provider/model
or thinking level, the hub still persisted the new value and the UI
reported success while Pi kept the old runtime state.

Use sendPiRpcAndWait so a failed set_model/set_thinking_level rejects
the web request and leaves the session config unchanged.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Resolve successful set_model responses for awaited Pi config changes — why it matters: the follow-up change now awaits sendPiRpcAndWait() before the hub persists an active Pi model change, but the set_model response branch updates currentModel/currentProvider and never resolves the pending RPC. A successful model switch therefore waits for the 10s timeout and the /sessions/:id/model request returns 409 even though Pi accepted the change. Evidence cli/src/pi/loop.ts:136.
    Suggested fix:
    case 'set_model': {
        const parsed = PiSetModelDataSchema.safeParse(response.data);
        if (parsed.success) {
            const data = parsed.data;
            const modelId = data.id ?? data.modelId;
            if (modelId) session.currentModel = modelId;
            if (data.provider && data.provider.length > 0) session.currentProvider = data.provider;
        }
        resolvePendingRpc(resolver, response);
        break;
    }

Questions

  • None.

Summary

  • Review mode: follow-up after new commits
  • One Major issue remains: successful active Pi model changes now time out because the new awaited RPC path has no resolver completion for set_model responses.

Testing

  • Not run (automation). Suggested coverage: a sendPiRpcAndWait(... set_model ...) test that emits a successful set_model response and expects the promise to resolve before timeout.

HAPI Bot

Comment thread cli/src/pi/loop.ts
handleGetState(response.data, session);
break;
}
case 'set_model': {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Resolve successful set_model responses for awaited Pi config changes

The follow-up change now awaits sendPiRpcAndWait() before the hub persists an active Pi model change, but this set_model response branch updates currentModel/currentProvider and never calls resolvePendingRpc. A successful model switch therefore waits for the 10s timeout and the /sessions/:id/model request returns 409 even though Pi accepted the change.

Suggested fix:

case 'set_model': {
    const parsed = PiSetModelDataSchema.safeParse(response.data);
    if (parsed.success) {
        const data = parsed.data;
        const modelId = data.id ?? data.modelId;
        if (modelId) session.currentModel = modelId;
        if (data.provider && data.provider.length > 0) session.currentProvider = data.provider;
    }
    resolvePendingRpc(resolver, response);
    break;
}

SetSessionConfig awaits sendPiRpcAndWait(set_model) before reporting the
model applied, but handleResponse's set_model branch updated state and
fell through without calling resolvePendingRpc. The pending RPC promise
then waited the full 10s timeout and rejected, making /sessions/:id/model
return 409 even though Pi accepted the change. Mirror every other branch
by resolving the pending RPC after updating currentModel/currentProvider.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Do not route Pi keyboard cycling through bare model IDs — why it matters: Pi model changes need { provider, modelId }, but the new Pi options list collapses every model to value: piModel.modelId; the existing Ctrl/Cmd+M shortcut then calls onModelChange(getNextModelForFlavor(...)) with only that string. In runPi, a string model only reuses the first cached provider matching the modelId, so duplicate Pi model IDs across providers switch to the wrong provider or can suppress set_model when the provider is unknown. Evidence web/src/components/SessionChat.tsx:490.
    Suggested fix:
    availableModelOptions={
        agentFlavor === "codex"
            ? codexModelOptions
            : agentFlavor === "cursor"
                ? (cursorCatalogPending || !cursorPicker || cursorPicker.modelOptions.length === 0 ? undefined : cursorPicker.modelOptions)
                : agentFlavor === "opencode"
                    ? opencodeModelOptions
                    : undefined
    }

Questions

  • None.

Summary

  • Review mode: follow-up after new commits
  • One Major issue found: the dedicated Pi picker sends provider-qualified models, but the generic keyboard cycling path still receives bare Pi model IDs and can apply the wrong provider.

Testing

  • Not run (automation): bun is not installed in this runner image (bun typecheck failed with command not found). Suggested coverage: a composer/model-options test that verifies Pi with dynamic models does not call onModelChange with a bare string from Ctrl/Cmd+M.

HAPI Bot

Comment thread web/src/components/SessionChat.tsx Outdated
? piModelsState.availableModels
: piCachedModels
return models.map((piModel) => ({
value: piModel.modelId,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Do not route Pi keyboard cycling through bare model IDs

Pi model changes need { provider, modelId }, but this new options list collapses every model to value: piModel.modelId. The existing Ctrl/Cmd+M shortcut in HappyComposer uses availableModelOptions with getNextModelForFlavor(...) and then calls onModelChange(...), so Pi can post a bare string instead of the provider-qualified object. In runPi, a string model only reuses the first cached provider matching that modelId; when two providers expose the same model ID, the shortcut switches the wrong provider, and before cached models load it can suppress set_model entirely.

Suggested fix:

availableModelOptions={
    agentFlavor === "codex"
        ? codexModelOptions
        : agentFlavor === "cursor"
            ? (cursorCatalogPending || !cursorPicker || cursorPicker.modelOptions.length === 0 ? undefined : cursorPicker.modelOptions)
            : agentFlavor === "opencode"
                ? opencodeModelOptions
                : undefined
}

…el suppressed

- loop.ts: split agent_start/turn_start branches. Pi emits both per prompt;
  draining on both popped the FIFO twice and shipped an undefined localId to
  the hub. agent_start now only sets thinking state; turn_start drains.
- runPi.ts: when set_model is suppressed (provider unknown), throw instead of
  silently returning applied, so the hub returns 409 rather than persisting a
  piSelectedModel Pi never received.
- loop.test.ts: assert agent_start does not drain; add regression test that a
  single turn drains exactly one real localId.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Keep Pi out of the generic model cycler — why it matters: the Pi picker sends { provider, modelId }, but SessionChat also passes piModelOptions into HappyComposer.availableModelOptions. The global Ctrl/Cmd+M shortcut uses that generic list and calls onModelChange(getNextModelForFlavor(...)), so it posts only a bare modelId. Pi needs the provider to disambiguate duplicate model IDs; runPi falls back to the first cached provider match, or throws when provider is not known. Evidence web/src/components/SessionChat.tsx:1121.
    Suggested fix:
    availableModelOptions={
        agentFlavor === 'codex'
            ? codexModelOptions
            : agentFlavor === 'cursor'
                ? (
                    cursorCatalogPending
                    || !cursorPicker
                    || cursorPicker.modelOptions.length === 0
                        ? undefined
                        : cursorPicker.modelOptions
                )
                : agentFlavor === 'opencode'
                    ? opencodeModelOptions
                    : undefined
    }

Questions

  • None.

Summary

  • Review mode: follow-up after new commits
  • One Major issue found: dynamic Pi model options still enter the generic keyboard model-cycling path and lose provider identity.

Testing

  • Not run (automation). Suggested coverage: a HappyComposer or model-options test where Pi has dynamic models with duplicate modelIds across providers and Ctrl/Cmd+M does not call onModelChange with a bare string.

HAPI Bot

Comment thread web/src/components/SessionChat.tsx Outdated
? opencodeModelOptions
: undefined
: agentFlavor === 'pi'
? piModelOptions

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Keep Pi out of the generic model cycler

The dedicated Pi picker sends { provider, modelId }, but this branch feeds piModelOptions into HappyComposer.availableModelOptions. The global Ctrl/Cmd+M shortcut uses that generic list and calls onModelChange(getNextModelForFlavor(...)), which returns only a bare modelId. Pi model changes need the provider to disambiguate duplicate model IDs; runPi falls back to the first cached provider match, or throws when the provider is not known.

Suggested fix:

availableModelOptions={
    agentFlavor === 'codex'
        ? codexModelOptions
        : agentFlavor === 'cursor'
            ? (
                cursorCatalogPending
                || !cursorPicker
                || cursorPicker.modelOptions.length === 0
                    ? undefined
                    : cursorPicker.modelOptions
            )
            : agentFlavor === 'opencode'
                ? opencodeModelOptions
                : undefined
}

SessionChat fed piModelOptions into HappyComposer.availableModelOptions,
so the global Ctrl/Cmd+M shortcut ran getNextModelForFlavor over the Pi
list and called onModelChange with a bare modelId string. Pi needs
{ provider, modelId } to disambiguate duplicate model IDs across
providers; a bare string made runPi fall back to the first cached
provider match (wrong provider) or throw when the provider was unknown.

Drop the piModelOptions useMemo and pass undefined for Pi, mirroring
modelOptions.ts where the Pi branch already returns the current model
unchanged (no-op) when no custom options are supplied. Pi model changes
now go only through the dedicated provider-qualified picker (piModels).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Do not persist Pi config before subprocess confirmation — runPi assigns currentModel, currentProvider, and currentThinkingLevel before sendPiRpcAndWait confirms set_model / set_thinking_level. When Pi rejects the model/provider or the RPC times out, the handler throws and the route returns 409, but the mutated PiSession keeps running; its 2s keepalive sends those unconfirmed values back to the hub, where handleSessionAlive persists them. That leaves the UI/DB showing a model or effort Pi never accepted. Evidence cli/src/pi/runPi.ts:167.
    Suggested fix:
    const previous = {
        model: piSession.currentModel,
        provider: piSession.currentProvider,
        thinkingLevel: piSession.currentThinkingLevel,
    }
    try {
        let requestedModel: { provider: string; modelId: string } | null | undefined
        if (config.model !== undefined) {
            requestedModel = resolveRequestedPiModel(config.model, piSession.cachedPiModels)
            if (requestedModel) {
                await sendPiRpcAndWait(piSession, transport, {
                    type: 'set_model',
                    provider: requestedModel.provider,
                    modelId: requestedModel.modelId,
                })
                piSession.currentModel = requestedModel.modelId
                piSession.currentProvider = requestedModel.provider
            }
        }
    
        if (config.effort !== undefined) {
            const nextLevel = config.effort === null ? null : PiThinkingLevelSchema.parse(config.effort.trim().toLowerCase())
            await sendPiRpcAndWait(piSession, transport, { type: 'set_thinking_level', level: nextLevel ?? 'off' })
            piSession.currentThinkingLevel = nextLevel
        }
    } catch (error) {
        piSession.currentModel = previous.model
        piSession.currentProvider = previous.provider
        piSession.currentThinkingLevel = previous.thinkingLevel
        throw error
    }

Questions

  • None.

Summary

  • Review mode: follow-up after new commits
  • One Major issue found: failed Pi model/effort changes can still leak into persisted hub state through keepalive after the RPC rejects.

Testing

  • Not run (automation). Suggested coverage: a runPi/SetSessionConfig test where set_model or set_thinking_level returns an error and subsequent keepalive does not report the rejected value.

HAPI Bot

Comment thread cli/src/pi/runPi.ts Outdated
if (cached) piSession.currentProvider = cached.provider;
} else {
// { provider, modelId } form
piSession.currentModel = modelValue.modelId;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] currentModel / currentProvider are mutated before sendPiRpcAndWait confirms set_model; currentThinkingLevel has the same pattern below. If Pi rejects the RPC or it times out, the handler throws and hub returns 409, but the long-lived PiSession keeps those unconfirmed values and its periodic keepalive can persist them anyway. Keep requested values local until the awaited Pi RPC succeeds, or restore the previous state in catch.

Suggested fix:

const previous = {
    model: piSession.currentModel,
    provider: piSession.currentProvider,
    thinkingLevel: piSession.currentThinkingLevel,
}
try {
    const requestedModel = resolveRequestedPiModel(config.model, piSession.cachedPiModels)
    if (requestedModel) {
        await sendPiRpcAndWait(piSession, transport, {
            type: 'set_model',
            provider: requestedModel.provider,
            modelId: requestedModel.modelId,
        })
        piSession.currentModel = requestedModel.modelId
        piSession.currentProvider = requestedModel.provider
    }
    // Apply currentThinkingLevel only after set_thinking_level succeeds.
} catch (error) {
    piSession.currentModel = previous.model
    piSession.currentProvider = previous.provider
    piSession.currentThinkingLevel = previous.thinkingLevel
    throw error
}

SetSessionConfig previously mutated piSession.currentModel /
currentProvider / currentThinkingLevel BEFORE awaiting
sendPiRpcAndWait(set_model / set_thinking_level). When Pi rejected the
value or the RPC timed out, the handler threw and the route returned
409, but PiSession kept the unconfirmed values; the 2s keepalive then
reported them back to the hub, where handleSessionAlive persisted a
model/effort Pi never accepted.

Resolve the requested model/effort into locals first, send the RPCs,
and only commit to PiSession after each await resolves. The null
(clear-model) path needs no RPC so it still commits immediately; the
unknown-provider path still throws without committing.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Confirm Pi startup config before persisting it — the new Pi path passes opts.model into bootstrapSession, initializes PiSession.currentModel from the same unconfirmed value, and later applies startup model/effort with fire-and-forget transport.send. If Pi rejects set_model / set_thinking_level, or the requested model is not found in get_available_models, the hub can still persist and show a model/effort Pi never accepted. Evidence cli/src/pi/runPi.ts:44, cli/src/pi/session.ts:62, cli/src/pi/runPi.ts:123.
    Suggested fix:
    const startupModel = opts.model?.trim() || null;
    const bootstrap = opts.existingSessionId
        ? await bootstrapExistingSession({ ... })
        : await bootstrapSession({
            flavor: 'pi',
            startedBy,
            workingDirectory,
            model: undefined,
        });
    
    const piSession = new PiSession({
        ...,
        model: null,
        initialModel: startupModel,
    });
    
    // After transport.start() and wireTransportEvents():
    const models = parsePiModels(await sendPiRpcAndWait(piSession, transport, { type: 'get_available_models' }));
    if (startupModel) {
        const match = models.find((m) => m.modelId === startupModel);
        if (!match) throw new Error(`Pi model not found: ${startupModel}`);
        await sendPiRpcAndWait(piSession, transport, {
            type: 'set_model',
            provider: match.provider,
            modelId: match.modelId,
        });
        piSession.currentModel = match.modelId;
        piSession.currentProvider = match.provider;
        piSession.pushKeepAlive();
    }

Questions

  • None.

Summary

  • Review mode: follow-up after new commits
  • One Major issue found: startup Pi model/effort can still be persisted before Pi confirms it, despite the mid-session RPC path now waiting for confirmation.

Testing

  • Not run (automation/static review; no PR code executed). Suggested coverage: start a Pi session with an invalid/unavailable --model and with a rejected --effort, then assert the hub session does not persist the rejected values.

HAPI Bot

Comment thread cli/src/pi/runPi.ts Outdated
flavor: 'pi',
startedBy,
workingDirectory,
model: opts.model

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] opts.model is persisted at session creation before Pi confirms it, and the later startup set_model path is fire-and-forget. PiSession.currentModel is also initialized from this same requested value, so keepalive can continue reporting it even when get_available_models has no matching provider or Pi rejects set_model. The mid-session SetSessionConfig path now waits for confirmation; startup needs the same treatment for model and effort.

Suggested fix:

const startupModel = opts.model?.trim() || null;

// Do not pass startupModel to bootstrapSession/PiSession.currentModel yet.
const piSession = new PiSession({
    ...,
    model: null,
    initialModel: startupModel,
});

const models = parsePiModels(await sendPiRpcAndWait(piSession, transport, { type: 'get_available_models' }));
if (startupModel) {
    const match = models.find((m) => m.modelId === startupModel);
    if (!match) throw new Error(`Pi model not found: ${startupModel}`);
    await sendPiRpcAndWait(piSession, transport, {
        type: 'set_model',
        provider: match.provider,
        modelId: match.modelId,
    });
    piSession.currentModel = match.modelId;
    piSession.currentProvider = match.provider;
}

Two startup paths persisted the requested --model before Pi confirmed it:

1. handleGetState set session.currentModel = session.initialModel as soon
   as get_state returned, using the unconfirmed startup model instead of
   Pi's actual default. If the model was unavailable or rejected, the 2s
   keepAlive reported it to the hub, which persisted/showed a model Pi
   never accepted.

2. get_available_models then sent set_model fire-and-forget, so a Pi
   rejection was never observed and currentModel stayed on the bad value.

Fix: handleGetState now reports Pi's real current model (newModel) while
a startup model is merely requested. get_available_models resolves the
provider from the cached list, awaits set_model, and commits
currentModel/currentProvider only on success — on rejection it logs and
keeps Pi's default. The await is fired detached so the
get_available_models RPC itself still resolves for ListPiModels.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Do not persist Pi startup model before Pi confirms it — runPi still passes opts.model into bootstrapSession and into PiSession, then starts keepalive before get_available_models / set_model confirm the requested model. SessionCache.handleSessionAlive persists every non-undefined keepalive model, so an unavailable or rejected startup model can still be stored and shown even though the new async set_model path later keeps Pi on its default. Evidence cli/src/pi/runPi.ts:44, cli/src/pi/runPi.ts:57, cli/src/pi/session.ts:62, hub/src/sync/sessionCache.ts:207.
    Suggested fix:
    const startupModel = opts.model?.trim() || null;
    
    const bootstrap = opts.existingSessionId
        ? await bootstrapExistingSession({
            sessionId: opts.existingSessionId,
            flavor: 'pi',
            startedBy,
            workingDirectory,
        })
        : await bootstrapSession({
            flavor: 'pi',
            startedBy,
            workingDirectory,
            model: undefined,
        });
    
    const piSession = new PiSession({
        api: bootstrap.api,
        client: apiSession,
        path: workingDirectory,
        logPath: logger.getLogPath(),
        startedBy,
        startingMode,
        model: null,
        initialModel: startupModel,
    });

Questions

  • None.

Summary
Review mode: follow-up after new commits
One Major issue remains from the previous pass: the follow-up change now waits for startup set_model, but the requested model is still persisted before Pi confirms it.

Testing

  • Not run (automation/static review). Suggested coverage: start Pi with an invalid or unavailable --model and assert the stored hub session model remains Pi's actual default or null, not the rejected requested value.

HAPI Bot

Comment thread cli/src/pi/runPi.ts Outdated
flavor: 'pi',
startedBy,
workingDirectory,
model: opts.model

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] opts.model is still written before Pi confirms it. This line stores the requested model during session creation, and line 57 seeds PiSession.currentModel with the same value before piSession.startKeepAlive() begins reporting it. Because the hub persists every non-undefined keepalive model, an unavailable or rejected startup model can still be shown/stored even though the later async set_model handler keeps Pi on its default.

Suggested fix:

const startupModel = opts.model?.trim() || null;

const bootstrap = opts.existingSessionId
    ? await bootstrapExistingSession({
        sessionId: opts.existingSessionId,
        flavor: 'pi',
        startedBy,
        workingDirectory,
    })
    : await bootstrapSession({
        flavor: 'pi',
        startedBy,
        workingDirectory,
        model: undefined,
    });

const piSession = new PiSession({
    api: bootstrap.api,
    client: apiSession,
    path: workingDirectory,
    logPath: logger.getLogPath(),
    startedBy,
    startingMode,
    model: null,
    initialModel: startupModel,
});

The startup --model still reached the hub unconfirmed via two paths the
previous Fix tiann#13 left open:

1. bootstrapSession({ model: opts.model }) seeded the hub session model
   at creation time, and SessionCache.handleSessionAlive persists every
   non-undefined keepAlive model — so an unavailable/rejected model was
   stored and shown before get_available_models/set_model ran.
2. PiSession constructor set this.currentModel = opts.model, so the very
   first keepAlive (sent by startKeepAlive before any RPC confirms the
   model) reported the unconfirmed value.

Pass model: undefined to bootstrapSession and start PiSession.currentModel
at null; opts.model is still captured as initialModel and applied/committed
only after get_available_models confirms it exists and set_model succeeds
(Fix tiann#13). The hub now sees Pi's real current model from the first
get_state keepAlive and switches to the requested model only once accepted.

Also add sendPiRpcAndWait contract tests pinning the await<->resolve
symmetry (Fix tiann#10): set_model/set_thinking_level/get_available_models must
resolve before timeout on a success response, and reject on a Pi error.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Do not persist Pi startup effort before Pi confirms it — runPi restores opts.effort directly into piSession.currentThinkingLevel before the subprocess has accepted set_thinking_level. startKeepAlive() is already running, and PiSession.pushKeepAlive() persists effort, so a resumed session can show/store a thinking level Pi rejected or ignored. This is the same confirmation contract already fixed for startup model selection. Evidence cli/src/pi/runPi.ts:131, cli/src/pi/runPi.ts:347, cli/src/pi/session.ts:87.
    Suggested fix:
    let startupThinkingLevel: PiThinkingLevel | null = null;
    if (opts.effort) {
        const result = PiThinkingLevelSchema.safeParse(opts.effort.trim().toLowerCase());
        if (result.success) startupThinkingLevel = result.data;
    }
    
    // after transport.start() and wireTransportEvents(...)
    if (startupThinkingLevel) {
        void (async () => {
            try {
                await sendPiRpcAndWait(piSession, transport, {
                    type: "set_thinking_level",
                    level: startupThinkingLevel,
                });
                piSession.currentThinkingLevel = startupThinkingLevel;
                piSession.pushKeepAlive();
            } catch (error) {
                logger.debug("[pi] Startup thinking level rejected, keeping Pi default:", error);
            }
        })();
    }

Questions

  • None.

Summary
Review mode: follow-up after new commits
The previous startup-model finding appears resolved in the follow-up commits. One analogous Major issue remains for startup/resume effort: HAPI can persist an unconfirmed Pi thinking level before Pi accepts it.

Testing

  • Not run (automation/static review). Suggested coverage: resume/start Pi with a saved unsupported effort and assert the hub does not persist that value unless Pi returns a successful set_thinking_level response.

HAPI Bot

Comment thread cli/src/pi/runPi.ts Outdated
if (opts.effort) {
const result = PiThinkingLevelSchema.safeParse(opts.effort.trim().toLowerCase());
if (result.success) {
piSession.currentThinkingLevel = result.data;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] This assigns the restored startup effort before Pi has confirmed set_thinking_level. Because keepalive is already running and includes effort, the hub can persist a thinking level that Pi later rejects or ignores, leaving the UI out of sync with the subprocess.

Suggested fix:

let startupThinkingLevel: PiThinkingLevel | null = null;
if (opts.effort) {
    const result = PiThinkingLevelSchema.safeParse(opts.effort.trim().toLowerCase());
    if (result.success) startupThinkingLevel = result.data;
}

if (startupThinkingLevel) {
    void (async () => {
        try {
            await sendPiRpcAndWait(piSession, transport, {
                type: "set_thinking_level",
                level: startupThinkingLevel,
            });
            piSession.currentThinkingLevel = startupThinkingLevel;
            piSession.pushKeepAlive();
        } catch (error) {
            logger.debug("[pi] Startup thinking level rejected, keeping Pi default:", error);
        }
    })();
}

runPi restored opts.effort straight into piSession.currentThinkingLevel
before startKeepAlive ran, and pushKeepAlive persists effort — so a
resumed/runner-spawned session could store/show a thinking level Pi
rejected or ignored. This is the effort analog of the startup-model
confirmation contract (Fix tiann#13/tiann#14).

Capture the requested effort into a local startupThinkingLevel instead of
mutating currentThinkingLevel up front. After transport.start() and the
get_state/get_available_models/get_commands sends, await set_thinking_level
and commit currentThinkingLevel + push a keepAlive only on success; on
rejection keep Pi's default (already reported by get_state). The await is
detached so the run loop is not blocked, and get_state is sent before the
set so its authoritative baseline lands first and cannot clobber the
confirmed value.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Omit unknown Pi runtime config from keepalive — PiSession starts with currentModel/currentThinkingLevel set to null, then startKeepAlive() immediately sends those values before get_state, set_model, or set_thinking_level has confirmed anything. The hub treats null as an explicit clear (hub/src/sync/sessionCache.ts:207, hub/src/sync/sessionCache.ts:223), so resuming a Pi session can erase the persisted model/effort before Pi reports the actual state. Evidence cli/src/pi/session.ts:85.
    Suggested fix:
    type KeepAliveRuntime = Parameters<ApiSessionClient['keepAlive']>[2]
    
    currentModel: string | null | undefined
    currentThinkingLevel: PiThinkingLevel | null | undefined
    
    private getKeepAliveRuntime(): KeepAliveRuntime {
        const runtime: NonNullable<KeepAliveRuntime> = {}
        if (this.currentModel !== undefined) runtime.model = this.currentModel
        if (this.currentThinkingLevel !== undefined) runtime.effort = this.currentThinkingLevel
        return Object.keys(runtime).length > 0 ? runtime : undefined
    }
    
    pushKeepAlive(): void {
        this.client.keepAlive(this.piIsStreaming, this.mode, this.getKeepAliveRuntime())
    }

Questions

  • None.

Summary
Review mode: follow-up after new commits
The prior startup-effort finding is addressed in the new commits. One Major issue remains: the first Pi heartbeat still sends null runtime config, which the hub persists as an explicit clear before Pi confirms or reports its real model/effort.

Testing

  • Not run (automation/static review). Suggested coverage: resume an inactive Pi session with stored model/effort, emit the first keepalive before Pi state responses, and assert the hub does not clear those fields until Pi sends a confirmed value.

HAPI Bot

Comment thread cli/src/pi/session.ts Outdated
}

pushKeepAlive(): void {
this.client.keepAlive(this.piIsStreaming, this.mode, {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] This first keepalive sends model: null and effort: null because both fields are initialized to null before Pi reports get_state or confirms startup set_model / set_thinking_level. The hub treats null as an explicit clear, so resuming a Pi session can erase the stored model/effort before Pi has confirmed the actual runtime config.

Suggested fix:

type KeepAliveRuntime = Parameters<ApiSessionClient['keepAlive']>[2]

currentModel: string | null | undefined
currentThinkingLevel: PiThinkingLevel | null | undefined

private getKeepAliveRuntime(): KeepAliveRuntime {
    const runtime: NonNullable<KeepAliveRuntime> = {}
    if (this.currentModel !== undefined) runtime.model = this.currentModel
    if (this.currentThinkingLevel !== undefined) runtime.effort = this.currentThinkingLevel
    return Object.keys(runtime).length > 0 ? runtime : undefined
}

pushKeepAlive(): void {
    this.client.keepAlive(this.piIsStreaming, this.mode, this.getKeepAliveRuntime())
}

…isted state

Fix tiann#14 changed PiSession.currentModel to start at null so the startup
--model was not leaked before confirmation. But the hub treats keepAlive
model:null as an explicit clear (sessionCache.ts only skips when the
field is undefined), so the first heartbeat (startKeepAlive runs before
get_state) now erased a resumed Pi session's persisted model/effort
before Pi reported its real state.

Distinguish "unknown" from "clear": currentModel/currentThinkingLevel
start undefined and keepAlive omits undefined fields (via
getKeepAliveRuntime), so the hub leaves persisted values alone until Pi
confirms. null remains an explicit clear and is still forwarded. Once
get_state/set_model/set_thinking_level confirm a value it is set and
reported normally.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Disable the generic model shortcut for Pi — Pi model changes need { provider, modelId }, but the newly enabled Pi onModelChange path lets HappyComposer's existing Ctrl/Cmd+M handler call onModelChange(getNextModelForFlavor('pi', model)), which returns only the current bare modelId or null. That bypasses the dedicated provider-qualified picker and can either clear the Pi model when session.model is empty, fail with provider is not yet known, or choose the wrong provider when multiple providers share the same modelId. Evidence web/src/components/SessionChat.tsx:1170.
    Suggested fix:
    const handleGlobalKeyDown = (e: globalThis.KeyboardEvent) => {
        if (agentFlavor === 'pi') {
            return
        }
        if (e.key === 'm' && (e.metaKey || e.ctrlKey) && onModelChange && supportsModelChange(agentFlavor)) {
            e.preventDefault()
            onModelChange(getNextModelForFlavor(agentFlavor, model, availableModelOptions))
            haptic('light')
        }
    }

Questions

  • None.

Summary
Review mode: follow-up after new commits
The previous keepalive/null-runtime issue is addressed. One Major issue remains: the Pi session now wires onModelChange, but the generic model-cycle shortcut still emits string/null values instead of Pi's provider-qualified model object.

Testing

  • Not run (automation/static review). Suggested coverage: render a Pi session, press Ctrl/Cmd+M, and assert no /model request is sent; model changes should only come from selecting an entry in PiModelPanel.

HAPI Bot

: undefined)
: handleModelChange
: agentFlavor === 'pi'
? (props.session.active && !piModelsState.error ? handleModelChange : undefined)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] This makes onModelChange available to HappyComposer for Pi, which also enables the existing global Ctrl/Cmd+M model cycler. That cycler calls getNextModelForFlavor('pi', model) and sends a bare string or null, while Pi's RPC needs { provider, modelId }. The result can clear the model when session.model is empty, fail with provider is not yet known, or select the wrong provider for duplicate modelIds.

Suggested fix:

const handleGlobalKeyDown = (e: globalThis.KeyboardEvent) => {
    if (agentFlavor === 'pi') {
        return
    }
    if (e.key === 'm' && (e.metaKey || e.ctrlKey) && onModelChange && supportsModelChange(agentFlavor)) {
        e.preventDefault()
        onModelChange(getNextModelForFlavor(agentFlavor, model, availableModelOptions))
        haptic('light')
    }
}

Fix tiann#11 removed piModelOptions from availableModelOptions, assuming
getNextModelForFlavor('pi', model, undefined) was a no-op. It is not:
the Pi branch returns normalizeCurrentModel(model), i.e. the current
modelId as a bare string, so the shortcut still called onModelChange with
a bare modelId. That loses the provider and can pick the wrong cached
match, clear the model when session.model is empty, or hit 'provider is
not yet known'. Short-circuit the handler for Pi so model changes go only
through the dedicated provider-qualified PiModelPanel.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Persist Pi's provider-qualified model outside explicit web model changes — Pi now stores session.model as the bare modelId and relies on metadata.piSelectedModel to disambiguate duplicate model IDs in the web picker/thinking-level filtering. That metadata is only written when /sessions/:id/model returns an applied object; the new startup/get_state paths set currentModel/currentProvider and then only keepAlive the bare string, so a Pi session opened with Pi's default model, resumed from CLI, or started with --model can render/select/filter against the wrong provider whenever two providers share a modelId. Evidence cli/src/pi/loop.ts:191.
    Suggested fix:
    function persistSelectedPiModel(session: PiSession): void {
        if (!session.currentModel || !session.currentProvider) return
        session.updateMetadata((meta) => ({
            ...meta,
            piSelectedModel: {
                provider: session.currentProvider!,
                modelId: session.currentModel!
            }
        }))
    }
    
    // Call after get_state and every successful set_model path updates both fields.
    session.currentModel = match.modelId
    session.currentProvider = match.provider
    persistSelectedPiModel(session)

Questions

  • None.

Summary
Review mode: follow-up after new commits
One Major issue remains. The previous generic Ctrl/Cmd+M finding is addressed, but Pi still loses provider identity unless the model was changed through the web picker.

Testing

  • Not run (automation/static review). Suggested coverage: wireTransportEvents should assert updateMetadata writes piSelectedModel after a get_state response with { modelId, provider }, and after startup set_model succeeds.

HAPI Bot

Comment thread cli/src/pi/loop.ts
provider: match.provider,
modelId: match.modelId,
});
session.currentModel = match.modelId;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] This startup set_model success updates currentModel/currentProvider, but the only value that later reaches the hub is the keepAlive model string from PiSession.getKeepAliveRuntime(). The provider-qualified metadata is only persisted through the /sessions/:id/model applied response path, so sessions that start/resume with Pi's current model or a CLI --model leave metadata.piSelectedModel unset/stale. The web Pi picker and thinking-level filter then fall back to a bare modelId match, which can select the wrong provider when providers share IDs.

Suggested fix:

function persistSelectedPiModel(session: PiSession): void {
    if (!session.currentModel || !session.currentProvider) return
    session.updateMetadata((meta) => ({
        ...meta,
        piSelectedModel: {
            provider: session.currentProvider!,
            modelId: session.currentModel!
        }
    }))
}

session.currentModel = match.modelId
session.currentProvider = match.provider
persistSelectedPiModel(session)

… paths

Pi stores session.model as the bare modelId and relies on
metadata.piSelectedModel ({ provider, modelId }) to disambiguate
duplicate modelId values across providers in the web picker and
thinking-level filtering. But piSelectedModel was only written by the web
/sessions/:id/model path (hub persistPiSelectedModel). The runtime paths
that set currentModel/currentProvider — get_state, the startup
get_available_models set_model, and the set_model response — only
keepAlive'd the bare modelId, so a Pi session on Pi's default model,
resumed from CLI, or started with --model had no provider identity in
metadata and could render/filter against the wrong provider.

Add persistSelectedPiModel(session) (no-op unless both fields are known)
and call it after get_state, after a successful startup set_model, and
after the set_model response updates the fields. This mirrors what the
web picker already does.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Terminal-started Pi sessions are marked local but have no local input path — hapi pi reaches runPi() without setting startingMode, so line 28 defaults terminal launches to local and line 53 marks controlledByUser: true. Unlike the other local-capable agents, this new Pi integration always spawns pi --mode rpc with piped stdio and only forwards prompts from apiSession.onUserMessage, so the terminal user gets no interactive prompt while the web UI treats the session as local-controlled. The first terminal hapi pi experience is therefore a live session that cannot be driven from either side until the user manually switches it from the web. Evidence cli/src/pi/runPi.ts:28.
    Suggested fix:
    const startingMode: 'local' | 'remote' = opts.startingMode ?? 'remote'

Questions

  • None.

Summary
Review mode: follow-up after new commits
One Major issue remains. The prior provider-qualified model metadata finding is addressed, but the default Pi launch mode still advertises local control without implementing a local stdin/TUI path.

Testing

  • Not run (automation/static review). Suggested coverage: a runPi() unit test asserting a terminal launch without --hapi-starting-mode calls setControlledByUser(..., 'remote')/keeps the session remotely drivable, or alternatively a local Pi launcher test if local terminal control is implemented.

HAPI Bot

Comment thread cli/src/pi/runPi.ts Outdated
} = {}): Promise<void> {
const workingDirectory = opts.workingDirectory ?? getInvokedCwd();
const startedBy = opts.startedBy ?? 'terminal';
const startingMode: 'local' | 'remote' = opts.startingMode

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] hapi pi reaches this default with startedBy === 'terminal', so the session is marked local and setControlledByUser() reports controlledByUser: true. The Pi process is still spawned only as pi --mode rpc with piped stdio, and prompts are only forwarded from apiSession.onUserMessage, so there is no terminal input path while the web UI sees the session as locally controlled. A plain terminal launch can leave users with a live Pi session that cannot be driven from either side until they manually switch it from the web.

Suggested fix:

const startingMode: 'local' | 'remote' = opts.startingMode ?? 'remote'

A terminal `hapi pi` launch defaulted to startingMode 'local' and marked
the session controlledByUser, but Pi only runs as `pi --mode rpc` with
piped stdio — there is no local terminal/TUI input path like Claude/Codex
have. The terminal user could not drive the session and the web treated
it as local-controlled, so the first terminal Pi session was stuck until
manually switched from the web.

Default to 'remote' so the session is immediately drivable from the web.
An explicit opts.startingMode (runner path) still takes precedence.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Pi local resume still marks the RPC-only process as local-controlled — hapi resume now routes Pi sessions through runPi(), but this branch forces startingMode: 'local'. runPi() documents and implements Pi as pi --mode rpc with piped stdio and no terminal/TUI input path, and setControlledByUser(apiSession, startingMode) will publish controlledByUser: true. That leaves a resumed Pi process advertised as local-controlled even though local input is not implemented; remote-only controls such as model/thinking changes are hidden or rejected until the user switches back to remote. Evidence cli/src/commands/resume.ts:155.
    Suggested fix:
    await runPi({
        existingSessionId: base.existingSessionId,
        workingDirectory: base.workingDirectory,
        resumeSessionId: base.resumeSessionId,
        startedBy: base.startedBy,
        startingMode: 'remote',
        model: target.model ?? undefined,
        effort: target.effort ?? undefined,
    })

Questions

  • None.

Summary
Review mode: follow-up after new commits
One Major issue remains. The prior plain hapi pi default-mode finding is fixed at the latest head, but the new Pi path in hapi resume still opts into the same unsupported local-control state.

Testing

  • Not run (automation/static review; bun is unavailable in this runner environment). Suggested coverage: update the Pi resume test to assert startingMode: 'remote' for dispatchLocalResume unless a real Pi local terminal input path is added.

HAPI Bot

Comment thread cli/src/commands/resume.ts Outdated
workingDirectory: base.workingDirectory,
resumeSessionId: base.resumeSessionId,
startedBy: base.startedBy,
startingMode: 'local',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] This Pi resume path forces startingMode: 'local', but runPi() only launches pi --mode rpc with piped stdio and no terminal/TUI input path. Because runPi() then calls setControlledByUser(apiSession, startingMode), the resumed session is advertised as local-controlled even though local input is not implemented; the web UI hides/rejects remote-only controls such as model/thinking changes until the user switches back to remote.

Suggested fix:

await runPi({
    existingSessionId: base.existingSessionId,
    workingDirectory: base.workingDirectory,
    resumeSessionId: base.resumeSessionId,
    startedBy: base.startedBy,
    startingMode: 'remote',
    model: target.model ?? undefined,
    effort: target.effort ?? undefined,
})

The previous Fix tiann#19 changed the `hapi pi` default to remote, but
`hapi resume` still passed startingMode: 'local' into runPi for Pi
sessions, re-introducing the same unsupported local-control state on the
resume path: setControlledByUser publishes controlledByUser while Pi has
no terminal/TUI input, hiding/rejecting remote-only controls until a web
switch. Pass 'remote' here too and update the resume test accordingly.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] bun run test:e2e now has no tests to run — this PR deletes e2e/scratchlist.spec.ts, which was the only file under the Playwright testDir (./e2e). The root script still runs playwright test, and playwright.config.ts still points at that directory, so the documented e2e command now fails/does nothing useful instead of exercising the scratchlist fixture. Evidence e2e/scratchlist.spec.ts:1 (deleted), related config playwright.config.ts:7.
    Suggested fix:
    // e2e/scratchlist.spec.ts
    import { test, expect } from '@playwright/test'
    
    test('scratchlist fixture loads', async ({ page }) => {
        await page.goto('/e2e-fixtures/scratchlist-fixture.html?session=smoke')
        await expect(page.getByTestId('scratchlist-panel')).toBeVisible()
    })

Questions

  • None.

Summary
Review mode: follow-up after new commits
One Major issue found. The previous Pi local-resume finding is fixed at the latest head, but this patch removes the only Playwright spec while keeping the e2e command/config in place.

Testing

  • Not run (automation/static review). Suggested: run bun run test:e2e after restoring or replacing the deleted spec.

HAPI Bot

Comment thread e2e/scratchlist.spec.ts
@@ -1,318 +0,0 @@
/*

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Deleting this file removes the only spec under the configured Playwright testDir (./e2e), while package.json still exposes bun run test:e2e as playwright test. That leaves the documented e2e command with no tests to execute for the scratchlist fixture.

Suggested fix:

// e2e/scratchlist.spec.ts
import { test, expect } from '@playwright/test'

test('scratchlist fixture loads', async ({ page }) => {
    await page.goto('/e2e-fixtures/scratchlist-fixture.html?session=smoke')
    await expect(page.getByTestId('scratchlist-panel')).toBeVisible()
})

The earlier "remove unrelated E2E harness" commit (d1e5b4c) deleted the
whole e2e/ directory this branch had added, but scratchlist.spec.ts is a
main-branch Playwright spec (the only file under playwright testDir
./e2e). Its removal left `bun run test:e2e` with no tests to run while
the script and playwright.config.ts still point at that directory.

Restore scratchlist.spec.ts from main; the unrelated harness files
(HARNESS.md, harness.*, integration/*.mts) that were genuinely
branch-only additions stay removed.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • No issues found.

Questions

  • None.

Summary
Review mode: follow-up after new commits
No high-confidence findings in the latest full PR diff. The prior bot finding about the deleted Playwright scratchlist spec is addressed by the follow-up commit restoring e2e/scratchlist.spec.ts. Residual risk: Pi runtime behavior depends on the external pi --mode rpc protocol and could not be exercised here.

Testing

  • Not run (automation/static review). Attempted focused subset with bun test cli/src/pi/*.test.ts hub/src/sync/sessionModel.test.ts web/src/components/AssistantChat/modelOptions.test.ts web/src/lib/sessionResume.test.ts shared/src/flavors.test.ts shared/src/modes.test.ts, but this environment does not have bun on PATH (bun: command not found).

HAPI Bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pi Coding Agent Support

1 participant