Fix Codex runtime and goal event handling#500
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe PR integrates computer-use backend status checking into the action allowlist and chat directives, refactors goal state management with visible-state deduplication helpers, preserves runtime ownership on action timeouts, fixes virtualization height caching to use row identities instead of indices, and makes CLI unhandled rejections non-fatal while keeping the process alive. ChangesRuntime service integration and chat improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arul28
changed the title
|
@copilot review but do not make fixes |
arul28
force-pushed
the
ade/quail-codex-goal-loop-682e2913
branch
from
0d5e41e to
9419c86
Compare
|
Post-rebase validation after updating onto origin/main (Linear Lane Launch Fixes #499):\n\n- |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx (1)
1290-1292: ⚡ Quick winAvoid fixed virtual-sizer thresholds in this regression test.
This couples the test to current sizing constants. Compare the rerendered height against the measured “Tall” baseline instead.
Proposed test hardening
- await waitFor(() => { - expect(virtualSizerHeight(rendered.container)).toBeGreaterThan(6_200); - }); + let tallHeight = 0; + await waitFor(() => { + tallHeight = virtualSizerHeight(rendered.container); + expect(tallHeight).toBeGreaterThan(0); + }); @@ - await waitFor(() => { - expect(virtualSizerHeight(rendered.container)).toBeLessThan(6_200); - }); + await waitFor(() => { + expect(virtualSizerHeight(rendered.container)).toBeLessThan(tallHeight); + });Also applies to: 1301-1303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx` around lines 1290 - 1292, The test uses a hard-coded threshold in the waitFor assertion (calling virtualSizerHeight(rendered.container) toBeGreaterThan(6_200)); change it to compare against the measured "Tall" baseline value instead of the fixed 6_200 constant: obtain the baseline height used elsewhere in the test harness (the measured “Tall” baseline variable or function) and assert virtualSizerHeight(rendered.container) is greater than that baseline; apply the same replacement for the similar assertion at lines 1301-1303 so both checks compare to the Tall baseline rather than a magic number.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx`:
- Around line 1290-1292: The test uses a hard-coded threshold in the waitFor
assertion (calling virtualSizerHeight(rendered.container)
toBeGreaterThan(6_200)); change it to compare against the measured "Tall"
baseline value instead of the fixed 6_200 constant: obtain the baseline height
used elsewhere in the test harness (the measured “Tall” baseline variable or
function) and assert virtualSizerHeight(rendered.container) is greater than that
baseline; apply the same replacement for the similar assertion at lines
1301-1303 so both checks compare to the Tall baseline rather than a magic
number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38ae58b3-f00f-4cc7-b6c5-03ffc603fef9
📒 Files selected for processing (10)
apps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
👮 Files not reviewed due to content moderation or server errors (1)
- apps/desktop/src/main/services/chat/agentChatService.test.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
arul28
force-pushed
the
ade/quail-codex-goal-loop-682e2913
branch
from
9419c86 to
7bd249a
Compare
|
Addressed review follow-ups in the latest push:\n\n- Added a rolling rate limit for contained unhandled-rejection logs so a hot async loop does not flood stderr while keeping the runtime alive.\n- Scoped owned-runtime-child preservation to the explicit timeout reconnect path, and added coverage that external runtime connects do not inherit stale child ownership.\n- Made chat row-height stale-key cleanup synchronous and hardened the virtualizer regression test to compare against the measured tall baseline.\n\nAdditional validation:\n- |
1 participant
Summary
Fixes the ADE-side failure modes behind the Codex app-server instability seen in Work chats: a timed-out runtime action could tear down the whole project runtime, repeated goal sync notifications were rendered as visible timeline spam, Computer Use backend status was not exposed through ADE actions, and virtualized chat rows could keep stale heights after row identities changed.
What changed
ade-cli serveruntime on a singleade/actions/calltimeout; ADE now drops the timed-out client socket and reconnects while keeping the runtime/PTY/chat process tree alive.ade-cli servelogged but non-fatal, while leaving uncaught exceptions fatal.computer_use_artifacts.getBackendStatusthrough ADE actions and soften the Codex Computer Use directive so agents do not stall when a helper is not exposed.The upstream Codex app-server README documents goal APIs as state-change notifications:
thread/goal/setemitsthread/goal/updated,thread/goal/getcan returngoal: null, andthread/goal/clearemitsthread/goal/clearedwhen state changes. This patch treats those notifications as state sync, not as unconditional visible transcript events.Docs reference: https://github.com/openai/codex/blob/main/codex-rs/app-server/README.md
Validation
npx vitest run src/main/services/chat/agentChatService.test.tsnpx vitest run src/main/services/localRuntime/localRuntimeConnectionPool.test.ts src/main/services/adeActions/registry.test.ts src/renderer/components/chat/AgentChatMessageList.test.tsxnpm --prefix apps/desktop run typecheck -- --pretty falsenpm --prefix apps/ade-cli run typecheck -- --pretty falsenpm --prefix apps/ade-cli run test -- src/cli.test.tsenv -u ADE_CHAT_SESSION_ID -u ADE_LANE_ID -u ADE_RUN_ID -u ADE_STEP_ID -u ADE_ATTEMPT_ID -u ADE_DEFAULT_ROLE npm --prefix apps/ade-cli run test -- src/adeRpcServer.test.tsenv -u ADE_CHAT_SESSION_ID -u ADE_LANE_ID -u ADE_RUN_ID -u ADE_STEP_ID -u ADE_ATTEMPT_ID -u ADE_DEFAULT_ROLE npm --prefix apps/ade-cli run testnpm run test:desktop:shardednpm --prefix apps/ade-cli run buildnpm --prefix apps/desktop run buildnpm --prefix apps/desktop run lint(0 errors, existing warnings)git diff --checkade doctor --textRisks
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Greptile Summary
This PR hardens the Codex runtime and goal event pipeline across five distinct areas: runtime timeout recovery, rejection resilience in
ade-cli serve, computer-use backend status exposure, Codex goal timeline deduplication, and virtualized chat row height keying.localRuntimeConnectionPool.ts): A timed-outade/actions/callnow drops only the client socket, preserving the spawned runtime/PTY/chat tree;preserveOwnedRuntimeChildOnNextConnectsignals the nexttryConnectto re-attach ownership to the kept-alive child rather than treating it as an external daemon.agentChatService.ts):setCodexGoalAndMaybeEmitUpdateandclearKnownCodexGoalgate all visible timeline chips behind a diff of(objective, status, tokenBudget), so usage-only ticks and repeated sync notifications no longer flood the transcript; four new test cases cover clear-on-unknown, clear-on-known, usage dedup, and silent null-refresh paths.AgentChatMessageList.tsx):measuredHeightsis now keyed by row identity string instead of index, with synchronous GC during render to prevent stale cross-session measurements from creating phantom scroll space.Confidence Score: 5/5
Safe to merge; all five fix areas are well-reasoned and covered by new targeted tests.
Each change is narrowly scoped and validated: the connection-pool ownership flag is tested for both the preserve and discard paths, the goal-deduplication helpers are covered by four new test cases across all meaningful event transitions, and the row-key fix is backed by a JSDOM height measurement test. No data loss, stale ownership, or incorrect event suppression paths were found.
No files require special attention.
Important Files Changed
preserveOwnedRuntimeChildOnNextConnectflag so a timed-out client is dropped without killing the runtime process;tryConnectcorrectly re-attaches ownership only when the flag is set, and falls back to null otherwise.setCodexGoalAndMaybeEmitUpdateandclearKnownCodexGoalhelpers to deduplicate goal timeline chips; visible updates are gated on changes to objective/status/tokenBudget, and clears only fire when a goal was previously stored.measuredHeightsfrom index-keyed to row-identity-keyed and adds synchronous render-phase GC of stale keys, preventing phantom scroll space when session content changes.getBackendStatusto thecomputer_use_artifactsaction allowlist, exposing the broker's backend status through ADE actions.tryConnect, and updates existing timeout test to assert no kill signal on timeout.getBackendStatusin thecomputer_use_artifactsdomain, including return-value assertion against the mock broker.Reviews (2): Last reviewed commit: "Fix Codex runtime and goal event handlin..." | Re-trigger Greptile