Skip to content

fix(native-chat): start a Codex chat without waiting on the background model probe - #23831

Merged
brennanb2025 merged 11 commits into
mainfrom
brennanb2025/fu-codex-chat-start-probe
Oct 6, 2026
Merged

brennanb2025 merged 11 commits into
mainfrom
brennanb2025/fu-codex-chat-start-probe

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

In a new Codex chat, the first message could appear and then fail with “Chat could not be started” and Retry after about 15 seconds. Orca had opened the chat’s Codex session, but it made that session wait for a separate background model lookup; when the background lookup hung, chat creation failed with it.

This change lets each chat ask its own Codex for its models. In the local hidden Electron reproduction, the old build failed after 16.1 seconds while the changed build replied. The reviewed catalog commit also replied in under a second in the “Resolve with AI” flow while the background lookup remained hung.

What Changed

A chat now shares a model lookup only with other reads from that same chat. A waiting model picker can use the first successful lookup for the same account, even if an earlier lookup is still running or failed. The shared catalog keeps the newer successful result when lookups finish out of order. A picker wait ends after 30 seconds if no catalog arrives.

The user sees a new Codex chat start and answer even when the background probe stalls. This affects the structured Codex chat on the execution host; terminal-backed chat is a separate path.

Why

The background lookup helps fill model choices before a chat exists. The chat already has its own Codex connection, so tying its start to the background process created the failure. Giving each live session its own lookup removes that dependency while preserving one lookup for simultaneous reads from that session. The bounded picker wait and ordering rule keep concurrent results useful without storing a second durable status flag.

Temporary difference: a separate, pre-existing case still fails if the chat’s own model lookup fails after Codex opens its thread. Fixing that requires changing acquisition-time option restoration, so it is in focused Draft follow-up #25754 from current main rather than expanding this PR. That Draft moves a cold saved-Fast lookup to the first send; its possible 30-second first-send wait remains an open product decision.

Linked Issue

The reported “Resolve with AI” failure in a rebase-conflict worktree and the related new-tab Codex start failure; no separate issue was filed. This also follows the Windows validation of #22971.

Visual Proof

Before, the old build showed a failed chat and Retry after the probe timeout:

Before: chat start failed with Retry

At current head 37cadb17a72, local hidden Electron with a stub Codex showed the unresolved rebase conflict, a successful “Resolve with AI” reply, and a new Codex chat that sent and received “hello picker” while the background probe hung. The current-main merge did not change the PR's six-file catalog diff.

Final head: unresolved rebase conflict with Resolve with AI

Final head: Resolve with AI chat received the stub reply

Final head: new Codex chat sent and received a message

Testing

  • Seven named stub-only test files: 94 tests passed on cdc6810aa1a. They cover a hung probe, concurrent chats, failure TTL, picker waits, out-of-order writes, and cache eviction. After merging current main at 37cadb17a72, eight named files passed 110 tests, including main's corrected Windows startup-line test.

  • tc-gate pnpm tc:node, changed-lines code-quality gate, and git diff --check passed; the lockfile is unchanged.

  • Local hidden Electron before/after on earlier heads: main failed with Retry at +16.1 seconds; PR showed the stub reply at +2.1 seconds. On catalog commit cdc6810aa1a, “Resolve with AI” visibly received its echo reply at +0.961 seconds with no failure across a 60-second observation. A new-tab Codex chat sent and received “hello picker” while a short-lived probe hung. Real Codex was never run in the rig.

  • On exact head 37cadb17a72, local hidden Electron “Resolve with AI” visibly received the stub reply at +7.218 seconds with no failure or Retry during 60 seconds while the probe stayed hung. New tab → Codex opened a chat that sent and received “hello picker”; its stub emitted the reply +2.333 seconds after Send. The new-tab timing sampler did not save samples, so there is no precise visual timestamp for the picker closing or chat tab appearing.

  • Remote M4Air and Windows high spec Electron QA were blocked by the session’s execution constraints, so there is no remote visual verdict. SSH sessions reject this local structured Codex path; no mobile client uses this catalog call. All current-head required CI checks passed; cross-version wire compatibility was skipped.

  • I tested these changes in a local hidden Electron rig on the final head.

  • Automated tests added.

Review

The review loop found and fixed three concurrency problems in this PR: a picker missing a later successful listing, a failed probe hiding another active listing, and an older result replacing a newer catalog. A cache-eviction problem introduced during the review fix was also corrected. A lower-impact error-detail attribution race remains deferred; a picker answer that times out cannot update itself if a catalog arrives later, which is also a limit of the existing one-shot read.

Agent skill upstream boundary

  • Not applicable; this change does not copy or translate skill-installer material.

Notes

The catalog remains capped and its new waiter callbacks and order bookkeeping end when their work settles or the wait deadline expires. No wire, persisted-schema, dependency, or new-process change is in this diff. The branch is mergeable and the current-head review gates are complete.

Checklist

  • This PR is small and focused.
  • The user-facing before and after, mechanism, and choice of approach are described.
  • Earlier before and final-head after screenshots are attached.
  • Self-reviewed for correctness, security, and performance.
  • Cross-platform, SSH, mobile, and remote impact considered.
  • CI and local final-head QA complete; remote visual QA was blocked and is disclosed above.

…d model probe

Opening a Codex chat kicks a session-less model-catalog probe (a throwaway
read-only `codex app-server`) for the account, and the new chat's own
options read then joined that probe's in-flight refresh through the catalog
store's single-flight. When the probe's Codex hung, chat start waited out the
probe's 15s deadline and failed with "codex app-server session exceeded
15000ms", even though the chat's own app-server was up.

Single-flight now joins only a refresh by the same kind of lister. A live
session lists over the connection it already holds; the probe still records
its own failure in the store, and the picker keeps serving whichever listing
succeeded.
…s listing

Single-flight in the model catalog store was split by lister kind, so a new
chat's acquire-time options read no longer joined the session-less probe,
but it still joined any other live chat's in-flight listing for the same
account. When that other chat's Codex was wedged or closing, the new chat
waited out that chat's request timeout and failed to start with its error.

Key in-flight listings by the lister's identity instead: a live session by
its own connection, the probe by the probe itself. A lister still joins its
own in-flight listing, and shouldRefresh still holds back a probe while any
listing for the account is in flight.
…e every listing settles

shouldRefresh reads 'any listing in flight for this account' from whether the
per-account in-flight map exists, so that map must be dropped exactly when its
last listing settles. Nothing covered this: removing the cleanup, or dropping
the map on the first settle, passed every catalog test. A leftover map would
stop every later background refresh and probe for the account.
A live session is keyed by its per-spawn catalog handle (minted in the same
acquire as its connection), the probe by itself.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status: ready for review. Head 40716436f78, CI green (16 pass, 15 skipped).

What this fixes: opening a Codex chat starts a background model-list probe. The chat's own startup joined that probe's in-flight request, so when the probe's Codex hung, the chat failed with "Chat could not be started" after 15 s. This was reproduced 3 of 3 times on main on Windows. Now a chat only waits on its own Codex, and the probe's failure stays in the model catalog.

Review (3 loops):

  • Loop 1 found that splitting the single-flight only by kind (chat vs probe) still let a new chat join another chat's stuck listing on the same account. It fixed this by keying in-flight listings by the lister's identity (efd9da606d2).
  • Loop 2 was clean and added a test that the account is released once its last listing settles (285cb83432c).
  • CI's lint then rejected the bare object key type. The key is now the named union of a chat's per-launch catalog handle (minted with its connection) and the probe (bffe7ad7771).
  • Loop 3 was clean and added a test that one chat's concurrent reads still share one listing (40716436f78).
  • The issues didn't repeat and there were no more than three loops, so no separate architecture challenge was run.
  • Readiness checklist at the start and end of review: no code findings. The end pass asked for the body fixes and screenshots that are now in the body.

Verification:

  • tc:node passes at the head.
  • The catalog, session-options and service suites pass.
  • With in-flight listings merged back into one per account (the old behaviour), the three hang tests time out and fail.
  • Windows 11 box, with every read-only Codex start hung and the chat's Codex real:
    • main fails at the probe's 15 s deadline.
    • This PR at 9d47c5eb59b and at 285cb83432c: the turn runs within 1 s of sending, and the picker lists the live models from the chat's own Codex.
    • Screenshots are in the body. The red "Reconnecting… n/5" rows are the rig's fabricated token getting 401s from OpenAI (the turn is running); main shows the same rows once a turn starts.

Not verified:

  • Windows at bffe7ad7771/40716436f78. Those change only the chat's key, from its connection to the catalog handle created in the same launch, plus tests; unit tests and the ablation cover them.
  • Two live chats hanging on each other on a real box (unit-tested only).

Out of scope, possible follow-up: chat start still depends on the chat's own model/list answering within its request timeout. A network stall there would still fail the start. This is not measured in Orca yet, so it's left for a separate change.

Not merged; marking ready for a human to decide.

@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Changes how the chat startup waits for model listings.

The PR should not merge until older concurrent listings can no longer replace a newer shared catalog.

Findings

  1. P1 Older listing replaces newer models ▶

Summary

This PR lets each Codex chat list models through its own connection instead of joining a background probe or another chat’s listing.

  • In-flight work is deduplicated per lister while background probing remains gated per account.
  • Concurrent successful listings need an ordering safeguard before they update the shared catalog.
Diagram
sequenceDiagram
  participant Probe
  participant Chat
  participant Store
  Probe->>Store: Start model listing
  Chat->>Store: Start independent model listing
  Chat-->>Store: Save newer catalog
  Probe-->>Store: Save older catalog later
  Note over Store: Older result replaces newer shared entry
Loading

Reviews (1) · Last reviewed commit: "test(codex): pin that a chat's own concu..."

(success) => {
this.refreshes.delete(fingerprint)
settle()
return this.recordSuccess(fingerprint, agent, success)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Older listing replaces newer models

When a slow probe and a chat list the same account at once, the chat can save a newer model list before the probe finishes. If the probe then succeeds with an older list, this call replaces the chat’s result and may save the older list to disk. The replacement is marked fresh, so the picker can show outdated models for up to ten minutes and validation can temporarily reject a newly available model. Prevent an older concurrent listing from replacing a newer one.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9bd1d31b-ef96-40a7-ac1e-3f70a2239d9d
📥 Commits

Reviewing files that changed from the base of the PR and between 213ff14 and 37cadb1.

📒 Files selected for processing (4)
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-service.test.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-service.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-store.test.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-store.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The catalog store tracks refreshes by fingerprint and lister identity. Calls with the same fingerprint and lister share an in-flight request. Calls with different listers can proceed independently. The Codex session-options and agent-model catalog service callers now pass the corresponding lister identity. Tests cover concurrent reads and refreshes, hanging listings, and refresh availability after listings settle.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 37cad

No actionable merge-blocking risk remains from the reviewed catalog coordination changes.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 37cad

The change prevents stalled background work from blocking unrelated chat startup while preserving the inspected account-isolation controls. No new credential access or privilege expansion was identified in those paths, but broader security coverage remains incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The directly affected shared state is catalog metadata within one execution-host process and fingerprint. Concurrent sessions using that identity can influence its selected metadata, but stored success fields contain model options and capability information, not provider connections or credentials. The inspected change does not establish new cross-account or cross-service authority.

Trust Boundaries and Controls

  • observed — The caller-facing read supplies a provider, optional session identifier, and optional workspace reference. The host derives catalog identity from a matching-provider session record or its launch account resolver, rather than accepting an arbitrary account-home path from that read. WSL-pinned records do not trigger native probes, and the response exposes catalog metadata rather than the account path.

Resilience and Maintainability Implications

  • observed — The picker deadline bounds only its wait; it does not cancel the underlying listing. Refresh cleanup depends on producer settlement. The inspected live connection has per-request deadlines and rejects pending requests during closure, while independent lister coordination prevents another listing's failure from deciding this session's result.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed and covers the change, reason, user impact, testing, visual proof, review findings, and compatibility considerations. However, the required Linked Issue section has no issu… Add a link to the issue this PR addresses and reference it in the Fixes # field. If the author is a maintainer covered by the template’s exception, state that the exception applies.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the main change: starting a Codex chat without waiting for the background model probe. It is specific and relevant, though somewhat long.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description is detailed and covers the change, reason, user impact, testing, visual proof, review findings, and compatibility considerations. However, the required Linked Issue section has no issue link, and the Fixes # field is blank.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: a128a721-8e39-48a2-8320-d140784031ef

📥 Commits

Reviewing files that changed from the base of the PR and between c49388c and 4071643.

📒 Files selected for processing (5)
  • src/main/codex/codex-structured-session-options-catalog.test.ts
  • src/main/codex/codex-structured-session-options.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-service.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-store.test.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-store.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Comment on lines +193 to +194
const listers: InFlightListings = this.refreshes.get(fingerprint) ?? new Map()
const inFlight = listers.get(lister)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🔎 Supported by static analysis

🏁 Script executed:

sed -n '1,270p' src/main/native-chat/agent-model-catalog/agent-model-catalog-store.ts
sed -n '80,155p' src/main/native-chat/agent-model-catalog/agent-model-catalog-store.test.ts
sed -n '85,135p' src/main/native-chat/agent-model-catalog/agent-model-catalog-service.ts

Repository: stablyai/orca

Length of output: 13936


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- relevant files ---'
git ls-files 'src/main/native-chat/agent-model-catalog/*'
printf '%s\n' '--- store callers and symbols ---'
rg -n -C 4 'recordSuccess|\.refresh\(|refresh\(' src/main/native-chat/agent-model-catalog src/main/native-chat | head -n 260
printf '%s\n' '--- persistence implementation ---'
fd -i 'agent-model-catalog-persistence' src
for f in $(fd -i 'agent-model-catalog-persistence' src); do
  echo "--- $f ---"
  sed -n '1,240p' "$f"
done
printf '%s\n' '--- session listing references ---'
rg -n -C 5 'listModels|AgentModelCatalog|model catalog|catalog' src/main/native-chat | head -n 360
printf '%s\n' '--- PR diff summary and focused diff ---'
git diff --stat 4da485cac1c308243fc22f5ee9c9c15058eee7ea 40716436f78bd079820ca59f8ad028a177abe4e0
git diff --unified=35 4da485cac1c308243fc22f5ee9c9c15058eee7ea 40716436f78bd079820ca59f8ad028a177abe4e0 -- src/main/native-chat/agent-model-catalog/

Repository: stablyai/orca

Length of output: 40398


🤖 get_repo_knowledge executed:

get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions

Length of output: 29698


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- persistence ---'
cat -n src/main/native-chat/agent-model-catalog/agent-model-catalog-persistence.ts
printf '%s\n' '--- exact refresh call sites ---'
rg -n -C 12 'store\.refresh|\.refresh\(fingerprint|recordSuccess\(' src/main --glob '*.ts' --glob '*.tsx'
printf '%s\n' '--- catalog listing APIs and session wiring ---'
rg -n -C 10 'agentModelCatalog|AgentModelCatalog|listModels|list.*model|models.*list|catalog' src/main/native-chat src/main/codex --glob '*.ts' --glob '*.tsx' | head -n 500
printf '%s\n' '--- relevant test definitions after omitted range ---'
sed -n '180,290p' src/main/native-chat/agent-model-catalog/agent-model-catalog-store.test.ts
sed -n '1,190p' src/main/native-chat/agent-model-catalog/agent-model-catalog-service.test.ts

Repository: stablyai/orca

Length of output: 43312


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- Codex catalog/session path ---'
rg -n -C 20 'catalogAccess|store\.refresh|refresh\(|recordSuccess|listModels|list.*model|model.*list' src/main/codex/codex-structured-session-options.ts src/main/codex/codex-structured-session-options-read.ts src/main/codex --glob '*.ts' | head -n 360
printf '%s\n' '--- Claude live write-through path ---'
sed -n '220,330p' src/main/claude/claude-structured-session-options.ts
rg -n -C 12 'writeClaudeCatalogThrough|catalogAccess|recordSuccess|list.*model|model.*list' src/main/claude/claude-structured-session-options.ts src/main/claude --glob '*.ts' | head -n 300
printf '%s\n' '--- probe definition and service refresh ---'
rg -n -C 20 'probes|AgentModelCatalogProbe|create.*Probe|probe.*model|listModels' src/main src/shared --glob '*.ts' | head -n 360

Repository: stablyai/orca

Length of output: 41924


Prevent an older listing from replacing a newer catalog entry.

When two listings for the same fingerprint overlap, the listing that starts first can finish last. recordSuccess then replaces the newer live-session result in memory and persistence. This can leave picker and validation reads with an older model catalog.

Assign a monotonic per-fingerprint start sequence to every async listing, including direct live-session writes. Commit a result only when its sequence is not older than the latest committed success. If an older result settles, return the current entry without persisting the stale result.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes — full PR at 40716436f78 (5 files): the Codex chat-start hang caused by joining the background model-catalog probe's in-flight listing.

  • In-flight listings keyed by lister identity — AgentModelCatalogStore.refresh now takes an AgentModelCatalogLister and keeps a per-fingerprint map of in-flight listings, so a chat's own option read never joins the probe's or another chat's listing and can no longer inherit their deadline or error.
  • New lister union — AgentModelCatalogSessionAccess (the per-spawn handle stored on session.catalogAccess) for a live chat; the probe function for the session-less probe. Call sites in codex-structured-session-options.ts and agent-model-catalog-service.ts pass their own identity; a lister still shares its own concurrent reads (a chat's picker read and its option validation list once).
  • Account-level coordination preserved — shouldRefresh still consults refreshes.has(fingerprint), so a picker read cannot kick a probe while any listing for the account runs.
  • Settle cleanup — settle() removes the lister and drops the fingerprint record only when the map empties, identity-guarded against a recreated map.
  • Tests — new cases pin hang isolation from a probe and from another chat, lister stability across a chat's own concurrent reads, and account release once every lister settles. I confirmed the three changed/adjacent suites pass (vitest 34 tests) and that the hang cases plausibly time out under the pre-change single-flight behavior.

I considered the out-of-order overwrite risk (a listing that starts first can finish last and replace a newer entry): overlapping listers list the same account through the same invocation resolver, so the only observable difference is origin/fetchedAt, and withKnownDefaultEfforts preserves a live child's defaults in either order. No sequence guard is warranted, so I did not flag it.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…-chat-start-probe

# Conflicts:
#	src/main/native-chat/agent-model-catalog/agent-model-catalog-service.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dce7b0b8-65e5-497c-88e1-9943bd46e573
📥 Commits

Reviewing files that changed from the base of the PR and between 4071643 and 213ff14.

📒 Files selected for processing (5)
  • src/main/codex/codex-structured-session-options-catalog.test.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-service.test.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-service.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-store.test.ts
  • src/main/native-chat/agent-model-catalog/agent-model-catalog-store.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Comment on lines +220 to +240
/** For a reader with no Codex of its own: the first listing running for the account that
* succeeds, or null once all of them fail. Null at once when none is running. */
pendingListing(fingerprint: string): Promise<AgentModelCatalogEntry | null> | null {
const runs = [...(this.refreshes.get(fingerprint)?.values() ?? [])]
if (runs.length === 0) {
return null
}
return new Promise((resolve) => {
let unsettled = runs.length
for (const run of runs) {
void run.then((entry) => {
if (entry) {
resolve(entry)
} else if (--unsettled === 0) {
resolve(null)
}
})
}
})
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '175,250p' src/main/native-chat/agent-model-catalog/agent-model-catalog-store.ts
sed -n '100,160p' src/main/native-chat/agent-model-catalog/agent-model-catalog-service.ts
sed -n '120,150p' src/main/codex/codex-structured-session-options-catalog.test.ts

Repository: stablyai/orca

Length of output: 5713


🏁 Script executed:

printf '%s\n' '--- store ---'
nl -ba src/main/native-chat/agent-model-catalog/agent-model-catalog-store.ts | sed -n '1,265p'
printf '%s\n' '--- service ---'
nl -ba src/main/native-chat/agent-model-catalog/agent-model-catalog-service.ts | sed -n '1,220p'
printf '%s\n' '--- session options symbols ---'
rg -n -F -- 'readLiveCodexSessionOptions' src/main
rg -n -F -- 'waitForListing' src/main
printf '%s\n' '--- catalog-related tests ---'
rg -n -F -- 'pendingListing' src/main/native-chat src/main/codex || test "$?" -eq 1
rg -n -F -- 'waitForListing' src/main/native-chat src/main/codex || test "$?" -eq 1

Repository: stablyai/orca

Length of output: 24632


🏁 Script executed:

printf '%s\n' '--- store tests ---'
nl -ba src/main/native-chat/agent-model-catalog/agent-model-catalog-store.test.ts | sed -n '105,155p'
printf '%s\n' '--- service tests ---'
nl -ba src/main/native-chat/agent-model-catalog/agent-model-catalog-service.test.ts | sed -n '185,295p'
printf '%s\n' '--- live session options implementation ---'
nl -ba src/main/codex/codex-structured-session-options.ts | sed -n '165,235p'
printf '%s\n' '--- session adapter method ---'
nl -ba src/main/codex/codex-structured-session-adapter.ts | sed -n '265,295p'
printf '%s\n' '--- RPC method ---'
rg -n -F -- 'waitForListing' src/main/runtime src/renderer src/shared || test "$?" -eq 1
printf '%s\n' '--- new-chat catalog test chronology ---'
nl -ba src/main/codex/codex-structured-session-options-catalog.test.ts | sed -n '130,155p'

Repository: stablyai/orca

Length of output: 16100


🏁 Script executed:

printf '%s\n' '--- picker listing helper ---'
rg -n -F -- 'codexSessionCatalogListingForPicker' src/main/codex/codex-structured-session-options.ts
nl -ba src/main/codex/codex-structured-session-options.ts | sed -n '45,130p'
printf '%s\n' '--- shared result and wait contract ---'
nl -ba src/shared/agent-session-wire.ts | sed -n '425,445p'
nl -ba src/shared/rpc-contract/structured-agent-session-params.ts | sed -n '286,308p'
printf '%s\n' '--- renderer wait/retry flow ---'
nl -ba src/renderer/src/components/native-chat/use-host-model-catalog-upgrade.ts | sed -n '45,125p'
printf '%s\n' '--- hook test asserting wait behavior ---'
nl -ba src/renderer/src/components/native-chat/use-host-model-catalog-upgrade.test.tsx | sed -n '115,165p'

Repository: stablyai/orca

Length of output: 11648


🏁 Script executed:

printf '%s\n' '--- PR diff for catalog store/service/tests ---'
git diff --unified=5 13d94acd4594f775ebcfdbe5a5dc38a0134a8f24 213ff14dd1184dfdfda1d219d6fcf504358c88c8 -- src/main/native-chat/agent-model-catalog/agent-model-catalog-store.ts src/main/native-chat/agent-model-catalog/agent-model-catalog-service.ts src/main/native-chat/agent-model-catalog/agent-model-catalog-store.test.ts src/main/native-chat/agent-model-catalog/agent-model-catalog-service.test.ts src/main/codex/codex-structured-session-options-catalog.test.ts
printf '%s\n' '--- service construction/probe bindings ---'
rg -n -F -- 'createAgentModelCatalogService' src/main
printf '%s\n' '--- probe definitions and timeout handling ---'
rg -n -i -- 'AgentModelCatalogProbe|probes:|modelCatalogProbe|catalogProbe|probe.*catalog' src/main/native-chat src/main/codex

Repository: stablyai/orca

Length of output: 26283


🏁 Script executed:

printf '%s\n' '--- Codex catalog probe ---'
nl -ba src/main/codex/codex-model-catalog-probe.ts | sed -n '1,145p'
printf '%s\n' '--- production catalog service wiring ---'
nl -ba src/main/runtime/structured-agent-model-catalog-wiring.ts | sed -n '1,115p'
printf '%s\n' '--- probe creation and timeout argument bindings ---'
rg -n -F -- 'createCodexModelCatalogProbe' src/main

Repository: stablyai/orca

Length of output: 8826


🏁 Script executed:

rg -n -F -- 'function runCodexAppServerSession' src/main/codex
rg -n -F -- 'runCodexAppServerSession' src/main/codex/codex-app-server-session.ts
nl -ba src/main/codex/codex-app-server-session.ts | sed -n '1,240p'

Repository: stablyai/orca

Length of output: 10628


🏁 Script executed:

nl -ba src/main/codex/codex-app-server-session.ts | sed -n '235,360p'

Repository: stablyai/orca

Length of output: 3464


Resolve pending picker reads from later same-fingerprint successes.

pendingListing snapshots only the runs active when the read starts. If that snapshot contains a hung probe, a later chat refresh can store its successful catalog without resolving the picker’s wait. When the probe reaches its 15-second deadline, the service returns unknown without rereading the store, so the picker remains held until then and misses the available catalog. Resolve outstanding waits when a later listing for the same fingerprint succeeds.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes — incremental re-review of the sync merge into main; the delta since the prior pullfrog review (40716436f78) is commit 213ff14dd1, which merges origin/main (13d94acd45) and reconciles it with the branch's lister-identity fix.

  • Reconciled main's picker-wait feature with lister identity — the conflict in agent-model-catalog-service.ts was resolved by routing a no-entry read through pendingListing(fingerprint) ?? refresh(...), so a session-less read still joins a running listing now that the probe no longer shares a lister key with a live chat.
  • Added AgentModelCatalogStore.pendingListing — resolves with the first in-flight listing for the account that succeeds, or null once all fail; null immediately when none is running.
  • Tests — pin that a waitForListing read answers from a chat's already-running listing instead of starting a probe, and that pendingListing resolves on first success (null once all snapshots fail).

I verified the merge-authored pendingListing mechanics (unsettled accounting cannot resolve null while a snapshotted run is pending; ?? falls through only on a real null) and that main's waitForListing / listingInProgress semantics survive the merge. All 42 tests in the three catalog suites pass.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +124 to +125
? (deps.store.pendingListing(fingerprint) ??
deps.store.refresh(fingerprint, params.agent, probe, () => probe(home)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pendingListing snapshots the in-flight listings at call time, so a waitForListing read that joined only the hung probe does not see the chat's own listing that starts a moment later. If that probe then fails, await listing resolves null and this returns unknown even though the chat's listing has already written an entry, so the picker can keep its fallback list until the next read.

Technical details
# A `waitForListing` read can miss a listing that starts after its snapshot

## Affected sites
- `agent-model-catalog-service.ts:124` — `pendingListing(fingerprint)` snapshots `refreshes.get(fingerprint)`; runs registered after the call are invisible to the returned promise.
- `agent-model-catalog-service.ts:134` — `entry = await listing` with no re-check of the store afterwards.
- `agent-model-catalog-store.ts:222` — `pendingListing` resolves on first success, or `null` once every snapshotted run fails.
- `codex-structured-session-options.ts:55` — the chat's listing runs under a distinct lister key, so it is not part of the probe's snapshot.

## Required outcome
- After `await listing` returns `null`, answer from `store.get(fingerprint)` if a concurrent listing has since landed, instead of returning `origin: 'unknown'`.

## Suggested approach
- `entry = await listing; entry ??= deps.store.get(fingerprint); if (!entry) { return { origin: 'unknown' } }`

@brennanb2025
brennanb2025 marked this pull request as draft October 6, 2026 00:49
@brennanb2025

Copy link
Copy Markdown
Contributor Author

The reported Codex chat could open its session, then show “Chat could not be started” and Retry about 15 seconds later when a separate background model lookup hung. This PR lets a live chat use its own model lookup, so the reported Resolve with AI flow and a new Codex chat can start and answer while that background lookup remains stuck.

The review found and fixed three problems in this PR: the model picker could miss a later successful lookup, a failed background lookup could hide an active chat lookup, and an older result could replace a newer model list. A cache eviction problem introduced during the review was also fixed. A rare error may still show the background lookup's failure detail instead of the chat's own detail. A picker that times out cannot update itself when models arrive later; that behavior predates this PR.

The separate pre-existing case where the chat's own model listing fails during acquisition remains temporary here and is tracked by focused Draft follow-up #25754. Its possible 30-second cold saved-Fast first-send wait is an open product decision, so that follow-up remains Draft.

On reviewed catalog commit cdc6810aa1a, 94 named stub-only tests, the scoped Node typecheck, changed-lines code-quality gate, and diff check passed. After merging current main at 37cadb17a72, eight named files passed 110 tests and the scoped Node typecheck and quality gate passed. A hidden local Mac Electron rig with a stub Codex reproduced the old failure; on exact head 37cadb17a72, Resolve with AI received its reply in 7.218 seconds with no Retry over 60 seconds, and a new-tab Codex chat sent and received “hello picker” while the background probe hung. The new-tab sampler did not save timing samples, so there is no precise visual timestamp for the chat tab appearing; the rendered reply and stub events were captured. Remote M4Air and Windows high spec visual QA were blocked by execution constraints, so those platforms have no visual verdict. All current-head required CI jobs passed; cross-version wire compatibility was skipped.

@brennanb2025
brennanb2025 marked this pull request as ready for review October 6, 2026 03:30
@brennanb2025
brennanb2025 merged commit 4e64fa9 into main Oct 6, 2026
54 checks passed
@brennanb2025
brennanb2025 deleted the brennanb2025/fu-codex-chat-start-probe branch October 12, 2026 05:54
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.

1 participant