trace-needs-input-marker -> main - #951
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds attention and settlement source metadata across desktop, iOS, CLI, and persistence layers. It narrows “needs you” detection to explicit or structured signals, removes clean-exit auto-settlement, updates lifecycle actions and bucketing, and revises related tests and sync contracts. ChangesSession lifecycle semantics
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
c7d910f to
e41e672
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e41e6726c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
e41e672 to
7cd0b1c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cd0b1cefc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/ade-cli/src/services/push/pushPublisherService.ts (1)
1551-1565: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not let CLI runtime heartbeats erase explicit attention.
handleSessionAttentionRequestedsets tracked CLI runs towaiting_for_input, but the nextonCliRuntimeSignalmapswaiting-inputoridletostaleand overwrites that explicit state. Runtime signals must not be authoritative over an existing explicit request; track the phase source or preserve the waiting phase untilpending_input_resolvedor explicit dismissal.🤖 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/ade-cli/src/services/push/pushPublisherService.ts` around lines 1551 - 1565, The onCliRuntimeSignal handler currently overwrites an explicit waiting_for_input state created by handleSessionAttentionRequested. Preserve that explicit attention state when an existing run is waiting for input, allowing runtime signals to update it only after pending_input_resolved or explicit dismissal; use the existing run-tracking state or phase source to distinguish explicit attention from inferred runtime phases.apps/ade-cli/src/services/sync/syncRemoteCommandService.ts (2)
1660-1667: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear provider-projected lifecycle fields when leaving awaiting input.
These projections spread prior session state and only overwrite lifecycle fields in the awaiting branch. When the chat resumes, provider-derived source and pending-input metadata can remain stale; the new bucket logic then continues to expose false actionable input.
apps/ade-cli/src/services/sync/syncRemoteCommandService.ts#L1660-L1667: clear provider-ownedpendingInputItemIdandattentionSourcein active/idle branches.apps/desktop/src/main/services/sessions/chatSessionProjection.ts#L89-L96: clear staleprovider_structuredattention provenance after awaiting input ends.apps/ade-cli/src/services/sync/syncRemoteCommandService.ts#L2229-L2236: clear provider-owned pending-input and source fields in list projections.🤖 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/ade-cli/src/services/sync/syncRemoteCommandService.ts` around lines 1660 - 1667, Clear stale provider-owned pending-input metadata and attention provenance when projections leave awaiting-input: update the active/idle branches around apps/ade-cli/src/services/sync/syncRemoteCommandService.ts lines 1660-1667 to reset pendingInputItemId and attentionSource; update apps/desktop/src/main/services/sessions/chatSessionProjection.ts lines 89-96 to remove stale provider_structured provenance; and update the list projection around apps/ade-cli/src/services/sync/syncRemoteCommandService.ts lines 2229-2236 to reset both provider-owned fields. Preserve the awaiting-input branch’s existing populated values.
3497-3518: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
runtimeState: "waiting-input"coupled with a real pending input ID.The projection can emit
runtimeState: "waiting-input"whilependingInputItemIdis omitted/null, butsessionStatusBucket()only considersattentionRequestedAtor a non-nullpendingInputItemId. Provider summaries that await input must carry an ID or an explicit wait signal so they are bucketed as awaiting input instead of running.🤖 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/ade-cli/src/services/sync/syncRemoteCommandService.ts` around lines 3497 - 3518, The sessionStatusBucket function currently treats waiting-input runtime states without pendingInputItemId as running. Update its awaiting-input condition to recognize runtimeState "waiting-input" only when coupled with a real pending input ID, and ensure provider summary projections supply that ID or an equivalent explicit wait signal before calling sessionStatusBucket.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts`:
- Around line 589-595: Update the description of tools.setSessionSettleOverride
to state that the "clear" override removes the pin and returns the row to its
declared lifecycle state, matching the inputSchema enum; do not refer to null.
In `@apps/ios/ADE/Services/Database.swift`:
- Around line 2007-2010: Update updateSessionLifecycle to accept a two-level
optional settleSource value, allowing the field to be explicitly set or cleared.
Pass "user" when applying a settle and nil when unsetting it, ensuring
optimistic lifecycle updates do not retain stale provenance.
---
Outside diff comments:
In `@apps/ade-cli/src/services/push/pushPublisherService.ts`:
- Around line 1551-1565: The onCliRuntimeSignal handler currently overwrites an
explicit waiting_for_input state created by handleSessionAttentionRequested.
Preserve that explicit attention state when an existing run is waiting for
input, allowing runtime signals to update it only after pending_input_resolved
or explicit dismissal; use the existing run-tracking state or phase source to
distinguish explicit attention from inferred runtime phases.
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 1660-1667: Clear stale provider-owned pending-input metadata and
attention provenance when projections leave awaiting-input: update the
active/idle branches around
apps/ade-cli/src/services/sync/syncRemoteCommandService.ts lines 1660-1667 to
reset pendingInputItemId and attentionSource; update
apps/desktop/src/main/services/sessions/chatSessionProjection.ts lines 89-96 to
remove stale provider_structured provenance; and update the list projection
around apps/ade-cli/src/services/sync/syncRemoteCommandService.ts lines
2229-2236 to reset both provider-owned fields. Preserve the awaiting-input
branch’s existing populated values.
- Around line 3497-3518: The sessionStatusBucket function currently treats
waiting-input runtime states without pendingInputItemId as running. Update its
awaiting-input condition to recognize runtimeState "waiting-input" only when
coupled with a real pending input ID, and ensure provider summary projections
supply that ID or an equivalent explicit wait signal before calling
sessionStatusBucket.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6db1c455-220f-47cd-b438-e8c1eec3f07e
⛔ Files ignored due to path filters (5)
docs/features/agents/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-sessions.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (47)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/services/push/pushPublisherService.test.tsapps/ade-cli/src/services/push/pushPublisherService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/sessionLifecycle.test.tsxapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/sessionLifecycle.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.test.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prMergeAutoSettlementService.tsapps/desktop/src/main/services/sessions/chatSessionProjection.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sessions/settleTerminalSession.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/lanes/laneAgents.test.tsapps/desktop/src/renderer/components/lanes/laneAgents.tsapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsxapps/desktop/src/renderer/components/terminals/sessionLifecycleActions.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/components/work/SessionLifecycleChips.test.tsxapps/desktop/src/renderer/lib/terminalAttention.test.tsapps/desktop/src/renderer/lib/terminalAttention.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/sessionCanonicalState.test.tsapps/desktop/src/shared/sessionCanonicalState.tsapps/desktop/src/shared/types/sessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkSessionCanonicalState.swift
💤 Files with no reviewable changes (3)
- apps/desktop/src/main/utils/terminalSessionSignals.ts
- apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
- apps/desktop/src/renderer/lib/terminalAttention.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c50a1c9f73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
c50a1c9 to
2a02d84
Compare
There was a problem hiding this comment.
💡 Codex Review
Degrade lane detail when chat projection fails
If agentChatService.listSessions rejects—for example while one chat-state read is malformed—the newly added promise rejects the entire Promise.all, so the socket/mobile lanes.getDetail command stops returning an otherwise valid lane. The corresponding lane-list builder already catches this optional projection and falls back to []; apply the same fallback here so chat enrichment cannot take down lane detail.
AGENTS.md reference: AGENTS.md:L35-L35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return workCanonicalSessionState( | ||
| status: session.status, | ||
| runtimeState: effectiveRuntime, | ||
| runtimeState: session.runtimeState, |
There was a problem hiding this comment.
When iOS receives an updated AgentChatSessionSummary with awaitingInput == true or a pendingInputItemId before the separately hydrated terminal row is refreshed, this bridge now ignores both summary fields and derives from the stale session.pendingInputItemId alone. The row can therefore remain Running/Ready without a Needs you badge—and an “Until I’m asked” snooze can keep it hidden—despite a live provider approval or question. Feed summary.pendingInputItemId (and the structured awaiting flag where needed for restored requests) into the canonical input rather than discarding the summary’s attention state.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64324ca86e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| update terminal_sessions | ||
| set settle_override = ?, | ||
| settle_source = case | ||
| when ? = 'settled' then 'user' |
There was a problem hiding this comment.
When the CTO operator calls setSessionSettleOverride with "settled" (ctoOperatorTools.ts), this branch always records the source as user, so the new Session Info field incorrectly reports “User action” instead of “ADE operator.” Accept a source in the underlying override service and have the operator tool pass operator, while UI/bulk user callers retain the user default.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
|
@codex review |
64324ca to
7bf5369
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf536912f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Explicit/provider-structured attention owns this phase until the | ||
| // lifecycle event that resolves it. PTY heartbeats are observational and | ||
| // must not erase a real request. | ||
| if (existing?.phase === "waiting_for_input" || existing?.phase === "waiting_for_approval") return; |
There was a problem hiding this comment.
When an explicit CLI ask is cleared through session.settleSession with dismissPendingInput (the desktop/socket settle path), settleTerminalSession resolves it by emitting an idle runtime state, but this guard discards that authoritative update because the push run is still waiting; the only new call to handleSessionAttentionResolved is wired to PTY user input. Consequently, Dismiss & settle clears the persisted attention while the Live Activity and aggregate push state remain waiting_for_input until the process exits or somebody later types into the PTY. The fresh evidence beyond the prior PTY-response issue is this separate dismiss-and-settle resolution path, which should also invoke the resolution handler.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/pty/ptyService.ts (1)
4102-4108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNotify the publisher only after the PTY write succeeds.
markPtyUserInputis called beforeentry.pty.write(...)inwrite,writeTerminal, andwriteBySessionId(Lines 5153-5154, 5442-5443, and 5543-5544). A failed write therefore marks the Live Activity asrunningeven though the CLI remains blocked on input. Keep local bookkeeping before the write if needed, but invokeonSessionUserInputonly after a successful write in all paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/pty/ptyService.ts` around lines 4102 - 4108, The publisher notification in markPtyUserInput must occur only after entry.pty.write succeeds. Separate local bookkeeping from onSessionUserInput, then update write, writeTerminal, and writeBySessionId to invoke the publisher callback after their successful writes while preserving bookkeeping before the write.apps/ade-cli/src/services/sync/syncRemoteCommandService.ts (1)
1660-1685: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not discard a structured pending-input item when
awaitingInputis false.A chat with
pendingInputItemIdbutawaitingInput: falsetakes theactivebranch and gets projected as running withpendingInputItemId: null. That contradicts the canonical contract and hides a provider request from remote clients. Treat a non-empty pending item as the same explicit attention condition in both projection implementations.Proposed fix
+ const providerPendingInputItemId = chat.pendingInputItemId?.trim() || null; - if (chat.awaitingInput) { + if (chat.awaitingInput || providerPendingInputItemId) { return { ...base, runtimeState: "waiting-input" as const, chatIdleSinceAt: null, - pendingInputItemId: chat.pendingInputItemId ?? session.pendingInputItemId ?? null, + pendingInputItemId: providerPendingInputItemId ?? session.pendingInputItemId ?? null, attentionSource: "provider_structured" as const, }; }Apply the equivalent change in
listRemoteWorkSessions.Also applies to: 2241-2266
🤖 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/ade-cli/src/services/sync/syncRemoteCommandService.ts` around lines 1660 - 1685, Update both projection implementations, including listRemoteWorkSessions, so a non-empty chat.pendingInputItemId is treated as an explicit waiting-input condition even when chat.awaitingInput is false. Ensure the waiting branch preserves the pending item ID and provider_structured attention, while active and idle/ended branches are only used when no pending item exists.
🧹 Nitpick comments (1)
apps/ade-cli/src/services/sync/syncRemoteCommandService.ts (1)
3601-3620: 📐 Maintainability & Code Quality | 🔵 TrivialValidate remote lane snapshot RPC behavior.
These projections are served through desktop RPC, but the ADE CLI test here only covers direct
work.getSessionchat pending-input merging. Exerciselanes.refreshSnapshotsandlanes.getDetailthrough both headless runtime and the socket-backedade rpc --stdiopath, then runade typecheck,ade test, andade build.🤖 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/ade-cli/src/services/sync/syncRemoteCommandService.ts` around lines 3601 - 3620, Add ADE CLI integration coverage for remote lane snapshot projections by exercising lanes.refreshSnapshots and lanes.getDetail through both the headless runtime and socket-backed ade rpc --stdio path. Verify snapshot results and detail responses match expected behavior, then run ade typecheck, ade test, and ade build. Use the existing sync service test setup around laneService.listStateSnapshots and related RPC handlers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/push/pushPublisherService.ts`:
- Around line 1890-1901: Update handleSessionAttentionResolved to remove the
session’s queued question and approval alerts from pendingAlerts when
transitioning the run back to running, in addition to clearing their dedupe
state. Perform this cleanup before scheduleFlush(true), and add a regression
test covering resolution before prompt flush or when Attention is unavailable.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 8334-8337: Update the unsettlement metadata construction around
settledAt and settleSource to set settleOverride to .some(nil) instead of nil,
ensuring any existing override is cleared while preserving the other settlement
fields.
In `@apps/ios/ADETests/WorkSessionCanonicalStateTests.swift`:
- Around line 249-260: Update the affected XCTest expectations in the existing
iOS lifecycle tests to match workCanonicalSessionState: runtime waiting-input
alone should expect running/stale as applicable, prompt-preview cases should no
longer expect needsYou, and clean exit code 0 should expect ended rather than
settled. Preserve the new testRowWrapperMapsChatSummaryPendingItemToNeedsYou
expectation and change only the specified existing assertions.
---
Outside diff comments:
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 1660-1685: Update both projection implementations, including
listRemoteWorkSessions, so a non-empty chat.pendingInputItemId is treated as an
explicit waiting-input condition even when chat.awaitingInput is false. Ensure
the waiting branch preserves the pending item ID and provider_structured
attention, while active and idle/ended branches are only used when no pending
item exists.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 4102-4108: The publisher notification in markPtyUserInput must
occur only after entry.pty.write succeeds. Separate local bookkeeping from
onSessionUserInput, then update write, writeTerminal, and writeBySessionId to
invoke the publisher callback after their successful writes while preserving
bookkeeping before the write.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 3601-3620: Add ADE CLI integration coverage for remote lane
snapshot projections by exercising lanes.refreshSnapshots and lanes.getDetail
through both the headless runtime and socket-backed ade rpc --stdio path. Verify
snapshot results and detail responses match expected behavior, then run ade
typecheck, ade test, and ade build. Use the existing sync service test setup
around laneService.listStateSnapshots and related RPC handlers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 323d5183-971c-48c4-b26a-267bbda52e42
⛔ Files ignored due to path filters (5)
docs/features/agents/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-sessions.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (50)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/services/push/pushPublisherService.test.tsapps/ade-cli/src/services/push/pushPublisherService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/sessionLifecycle.test.tsxapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/sessionLifecycle.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.test.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prMergeAutoSettlementService.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sessions/chatSessionProjection.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sessions/settleTerminalSession.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/lanes/laneAgents.test.tsapps/desktop/src/renderer/components/lanes/laneAgents.tsapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsxapps/desktop/src/renderer/components/terminals/sessionLifecycleActions.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/components/work/SessionLifecycleChips.test.tsxapps/desktop/src/renderer/lib/terminalAttention.test.tsapps/desktop/src/renderer/lib/terminalAttention.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/sessionCanonicalState.test.tsapps/desktop/src/shared/sessionCanonicalState.tsapps/desktop/src/shared/types/sessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkSessionCanonicalState.swiftapps/ios/ADETests/WorkSessionCanonicalStateTests.swift
💤 Files with no reviewable changes (3)
- apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
- apps/desktop/src/main/utils/terminalSessionSignals.ts
- apps/desktop/src/renderer/lib/terminalAttention.ts
🚧 Files skipped from review as they are similar to previous changes (33)
- apps/desktop/src/main/utils/terminalSessionSignals.test.ts
- apps/desktop/src/shared/types/sync.ts
- apps/desktop/src/renderer/components/lanes/laneAgents.ts
- apps/ios/ADE/Views/Work/WorkRootComponents.swift
- apps/desktop/src/main/services/sessions/settleTerminalSession.ts
- apps/desktop/src/shared/adeCliGuidance.ts
- apps/desktop/src/main/services/adeActions/registry.test.ts
- apps/ade-cli/src/adeRpcServer.test.ts
- apps/desktop/src/renderer/components/lanes/laneAgents.test.ts
- apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts
- apps/ios/ADE/Models/RemoteModels.swift
- apps/desktop/src/main/services/prs/prAsync.test.ts
- apps/desktop/src/renderer/components/terminals/SessionCard.test.tsx
- apps/desktop/src/renderer/lib/terminalAttention.test.ts
- apps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsx
- apps/desktop/src/main/services/state/kvDb.test.ts
- apps/desktop/src/main/services/adeActions/registry.ts
- apps/desktop/src/main/services/state/kvDb.ts
- apps/desktop/src/main/services/sessions/sessionService.test.ts
- apps/desktop/src/renderer/components/work/SessionLifecycleChips.test.tsx
- apps/ade-cli/src/tuiClient/sessionLifecycle.ts
- apps/ade-cli/src/tuiClient/adeApi.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
- apps/ade-cli/src/tuiClient/tests/sessionLifecycle.test.tsx
- apps/desktop/src/renderer/components/terminals/SessionContextMenu.test.tsx
- apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts
- apps/desktop/src/main/services/lanes/laneListSnapshotService.test.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts
- apps/desktop/src/shared/sessionCanonicalState.ts
- apps/desktop/src/renderer/components/terminals/sessionLifecycleActions.ts
- apps/ios/ADE/Views/Work/WorkSessionCanonicalState.swift
- apps/ios/ADE/Services/Database.swift
- apps/desktop/src/main/services/sessions/sessionService.ts
| settledAt: .some(nil), | ||
| settleOverride: nil | ||
| settleOverride: nil, | ||
| settleSource: .some(nil) | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear the settle override when unsetting.
nil for String?? means “leave untouched,” so a prior "active" override remains locally after unsettle. Use .some(nil) to clear it with the other settlement metadata and keep the iOS optimistic state aligned with the desktop mirror.
Proposed fix
- settleOverride: nil,
+ settleOverride: .some(nil),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| settledAt: .some(nil), | |
| settleOverride: nil | |
| settleOverride: nil, | |
| settleSource: .some(nil) | |
| ) | |
| settledAt: .some(nil), | |
| settleOverride: .some(nil), | |
| settleSource: .some(nil) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Services/SyncService.swift` around lines 8334 - 8337, Update the
unsettlement metadata construction around settledAt and settleSource to set
settleOverride to .some(nil) instead of nil, ensuring any existing override is
cleared while preserving the other settlement fields.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bde1573b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const run = runs.get(sessionId); | ||
| if (!run || (scopeKey != null && run.scopeKey !== scopeKey)) return; | ||
| if (run.phase !== "waiting_for_input" && run.phase !== "waiting_for_approval") return; | ||
| run.phase = "running"; |
There was a problem hiding this comment.
In the socket session.settleSession path for a tracked CLI with an explicit ask, settleTerminalSession first emits idle, but onCliRuntimeSignal suppresses that signal while the run is awaiting attention; this callback then hard-codes the run back to running even though the session is immediately settled. The Live Activity therefore reports running until another heartbeat changes it to stale, after which it can linger until the stale-run TTL. The fresh evidence after the earlier resolution-callback fix is that the invoked handler uses the same running transition intended for ordinary PTY user responses; dismiss-and-settle needs a terminal/completed transition or removal instead.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ef0f9e9be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 1. Deterministic attention beats everything — including the failure and | ||
| // stale checks below (an agent explicitly asking is actionable regardless). | ||
| if (args.pendingInputItemId || args.runtimeState === "waiting-input" || args.attentionRequestedAt) { | ||
| if (args.pendingInputItemId || args.attentionRequestedAt) { |
There was a problem hiding this comment.
When a restored provider request has awaitingInput: true but its pending item ID is temporarily unavailable, this condition now classifies the desktop session as running/ready instead of needs_you, so the badge and tab highlight disappear and an “Until I’m asked” snooze can keep the row hidden. Fresh evidence beyond the iOS report is that desktop projectChatOntoSession still maps every chat.awaitingInput to runtimeState: "waiting-input" while explicitly allowing pendingInputItemId to remain null (chatSessionProjection.ts:89-95); carry provider-structured provenance into this canonical input rather than dropping that signal with generic runtime inference.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1d850aec0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onAttentionResolved: (resolvedSessionId) => { | ||
| args.pushPublisherService?.handleSessionSettled(null, resolvedSessionId); | ||
| }, |
There was a problem hiding this comment.
When a running tracked CLI is settled without dismissPendingInput, this callback is never invoked, so the publisher never receives handleSessionSettled; the bulk session.settleSessions route bypasses it as well. The session becomes settled while its Live Activity remains running until a later heartbeat makes it stale or expires it. Invoke the publisher after every successful settlement, not only from the attention-dismissal branch. Fresh evidence beyond the earlier dismiss-and-settle thread is that ordinary and bulk settlements still have no publisher transition.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| // "active" is an explicit keep-active pin, "settled" acts like a declared | ||
| // settle. Mirrors canonicalSessionState in shared/sessionCanonicalState.ts. | ||
| if (args.attentionRequestedAt) return "awaiting-input"; | ||
| if (args.attentionRequestedAt || args.pendingInputItemId) return "awaiting-input"; |
There was a problem hiding this comment.
When a restored chat reports awaitingInput: true without a pending item ID, the projector sets attentionSource: "provider_structured" (and this service also sets pendingInputWaiting), but this predicate ignores both and counts the session as running. Lane badges and counters therefore disagree with the canonical session state; the duplicate remote lane predicate has the same omission. Fresh evidence beyond the earlier remote-projection finding is that the canonical state now explicitly treats source-only provider attention as needs_you, while these rollups still do not.
AGENTS.md reference: AGENTS.md:L33-L34
Useful? React with 👍 / 👎.
| session.attentionRequestedAt || session.pendingInputItemId | ||
| ? ["Attention source", lifecycleSourceLabel(session.attentionSource)] | ||
| : null, |
There was a problem hiding this comment.
For a restored structured request whose pending item ID is temporarily unavailable, the session now carries attentionSource: "provider_structured" and is canonically needs_you, but this condition is false and omits the Attention source row entirely. Include provider provenance in the visibility condition so the new audit surface still reports “Provider request.” Fresh evidence beyond the previous provenance comment is that the source is now propagated correctly; the renderer gate itself drops it.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/lib/terminalAttention.ts (1)
177-186: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHighlight provider-structured chat attention.
A provider-structured session without an item id is now canonical
needs_you, but this predicate returns false because it checks onlypendingInputItemIdandattentionRequestedAt. IncludeattentionSource === "provider_structured"and pass it from callers so the chat tab matches the session state.Suggested fix
export function sessionNeedsChatTabHighlight(args: { runtimeState?: TerminalRuntimeState; toolType?: TerminalToolType | null; pendingInputItemId?: string | null; + attentionSource?: TerminalSessionSummary["attentionSource"]; attentionRequestedAt?: string | null; }): boolean { if (!isChatToolType(args.toolType)) return false; if (args.pendingInputItemId) return true; + if (args.attentionSource === "provider_structured") return true; if (args.attentionRequestedAt) return true; return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/lib/terminalAttention.ts` around lines 177 - 186, Update sessionNeedsChatTabHighlight to accept an attentionSource value and return true when it is "provider_structured", even without pendingInputItemId or attentionRequestedAt. Update every caller to pass the session’s attentionSource so provider-structured needs_you state highlights the chat tab consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 3541-3545: Update summarizeLaneRuntime so the final bucket
selection gives "awaiting-input" precedence over "running" when sessions include
both states, including provider-structured attention. Preserve existing
classification logic and add a regression test covering a mixed running session
and provider-blocked chat, asserting the remote snapshot reports
"awaiting-input".
---
Outside diff comments:
In `@apps/desktop/src/renderer/lib/terminalAttention.ts`:
- Around line 177-186: Update sessionNeedsChatTabHighlight to accept an
attentionSource value and return true when it is "provider_structured", even
without pendingInputItemId or attentionRequestedAt. Update every caller to pass
the session’s attentionSource so provider-structured needs_you state highlights
the chat tab consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2af2b1f2-8aa3-4bff-a0d3-6dbfae532553
📒 Files selected for processing (11)
apps/ade-cli/src/services/push/pushPublisherService.test.tsapps/ade-cli/src/services/push/pushPublisherService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.test.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.tsapps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsxapps/desktop/src/renderer/lib/terminalAttention.tsapps/desktop/src/shared/sessionCanonicalState.test.tsapps/desktop/src/shared/sessionCanonicalState.tsapps/ios/ADETests/WorkSessionCanonicalStateTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ade-cli/src/services/push/pushPublisherService.test.ts
- apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts
|
@codex review |
b946015 to
c8de87f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8de87ffeb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const session = sessionService.get(event.sessionId); | ||
| if ( | ||
| session | ||
| && (session.status !== "running" || session.runtimeState === "idle") |
There was a problem hiding this comment.
When a tracked CLI is already idle and the user settles it, sessionService.get() derives runtimeState solely from the persisted status, so a live session is returned as status: "running", runtimeState: "running" and this condition skips handleSessionSettled. The earlier idle signal has already fired, and the supposed heartbeat is only evaluated while processing later output chunks, so a genuinely quiet CLI can leave its stale Live Activity open until the two-hour TTL. Fresh evidence beyond the prior settlement threads is that the centralized listener reads the non-projected SessionService row rather than ptyService's live idle state; resolve the live/projected runtime state here before deciding whether settlement is at rest.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/renderer/lib/terminalAttention.ts (1)
177-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHighlight structured provider attention too.
A session with
attentionSource: "provider_structured"but no pending-item ID isneeds_youincanonicalSessionState, yet this helper returnsfalse. ThreadattentionSourceinto this input and treat"provider_structured"as highlighted.Proposed fix
export function sessionNeedsChatTabHighlight(args: { runtimeState?: TerminalRuntimeState; toolType?: TerminalToolType | null; pendingInputItemId?: string | null; + attentionSource?: TerminalSessionSummary["attentionSource"]; attentionRequestedAt?: string | null; }): boolean { if (!isChatToolType(args.toolType)) return false; if (args.pendingInputItemId) return true; + if (args.attentionSource === "provider_structured") return true; if (args.attentionRequestedAt) return true; return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/lib/terminalAttention.ts` around lines 177 - 186, Update sessionNeedsChatTabHighlight to accept an attentionSource argument, then return true when it equals "provider_structured" in addition to the existing pendingInputItemId and attentionRequestedAt conditions. Ensure callers pass the session’s attentionSource through to preserve highlighting for structured provider attention.apps/ade-cli/src/services/sync/syncRemoteCommandService.ts (1)
2237-2269: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
listRemoteWorkSessionsre-implementsprojectChatOntoSessionwith missing cases.This inline block duplicates
projectChatOntoSession(defined earlier in this same file) but diverges from it in three ways:
- it never merges
chat.orchestrationRunId/orchestrationRole/orchestrationTag/orchestrationParentSessionId/spawnKindinto the result,pendingInputItemIddoesn't fall back tosession.pendingInputItemIdwhen the chat has none,- it only branches on
chat.status === "idle", not"idle" || "ended"— a chat that ended while the underlying PTY session is stillstatus: "running"falls through toreturn session;unchanged, leaving a staleruntimeState/attentionSource/pendingInputItemId.
getRemoteWorkSession(single-session read, a few hundred lines above) already delegates toprojectChatOntoSessioncorrectly, sowork.listSessions-style reads andwork.getSessionreads can now disagree for the same session.🐛 Proposed fix: reuse `projectChatOntoSession`
- if (chat.awaitingInput) { - return { - ...session, - runtimeState: "waiting-input" as const, - chatIdleSinceAt: null, - pendingInputItemId: chat.pendingInputItemId ?? null, - attentionSource: "provider_structured" as const, - }; - } - if (chat.status === "active") { - return { - ...session, - runtimeState: "running" as const, - chatIdleSinceAt: null, - pendingInputItemId: null, - attentionSource: session.attentionSource === "provider_structured" ? null : session.attentionSource, - }; - } - if (chat.status === "idle") { - return { - ...session, - runtimeState: "idle" as const, - chatIdleSinceAt: chat.idleSinceAt ?? null, - pendingInputItemId: null, - attentionSource: session.attentionSource === "provider_structured" ? null : session.attentionSource, - }; - } - return session; + return projectChatOntoSession(session, chat);🤖 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/ade-cli/src/services/sync/syncRemoteCommandService.ts` around lines 2237 - 2269, Replace the duplicated inline chat projection in listRemoteWorkSessions with the existing projectChatOntoSession helper. Preserve the surrounding visibility and running-session checks, and pass each eligible session and its matching chat through that helper so orchestration fields, pending-input fallback, and ended-chat handling remain consistent with getRemoteWorkSession.apps/desktop/src/shared/types/sync.ts (1)
1856-1856: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce deflate negotiation before decoding.
decodeEnvelopeTextwill inflate anycompression: "deflate"envelope that has valid base64 payload/uncompressed metadata, whilehandleMessagecalls it before checkinglatestHello?.compression?.codec === "deflate". Reject unnegotiated deflate envelopes here, or enforce the negotiated codec state in the decoder itself.🤖 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/shared/types/sync.ts` at line 1856, Ensure deflate envelopes are decoded only after deflate has been negotiated: update decodeEnvelopeText or the handleMessage call path to reject compression: "deflate" unless latestHello?.compression?.codec is "deflate". Preserve decoding for gzip and other valid negotiated envelopes.apps/ios/ADE/Services/SyncService.swift (1)
1803-1804: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHonor small negotiated frame budgets.
max(16 * 1024, ...)makes every chunk exceed a peer-advertised budget such as 1,025 bytes, even though lines 1796-1798 accept that budget.syncEncodeEnvelopeFramesthen throws andsendEnvelopesilently returnsfalse, dropping every oversized outbound message. Remove the fixed 16 KiB floor and derive a positive payload size frommaxFrameBytes.Proposed fix
- let partBytes = max(16 * 1024, ((maxFrameBytes - 1_024) * 3) / 4) + let partBytes = max(1, ((maxFrameBytes - 1_024) * 3) / 4)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Services/SyncService.swift` around lines 1803 - 1804, Update the chunk-size calculation in syncEncodeEnvelopeFrames to remove the fixed 16 KiB minimum and derive a positive partBytes value directly from the negotiated maxFrameBytes, while preserving the existing frame-overhead adjustment and total calculation so small budgets such as 1,025 bytes are honored.
🧹 Nitpick comments (5)
apps/ade-cli/src/services/sync/syncRemoteCommandService.ts (1)
3525-3599: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated bucket/rollup logic vs.
laneListSnapshotService.ts.Shares one root cause with the desktop copy — see consolidated comment.
🤖 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/ade-cli/src/services/sync/syncRemoteCommandService.ts` around lines 3525 - 3599, The sessionStatusBucket and summarizeLaneRuntime implementations duplicate the canonical bucket and lane-rollup logic from laneListSnapshotService.ts. Replace this local logic with the shared implementation, reusing its established symbols and preserving the existing LaneListSnapshot runtime shape.apps/desktop/src/main/services/lanes/laneListSnapshotService.ts (1)
104-183: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated bucket/rollup logic vs.
syncRemoteCommandService.ts.
sessionStatusBucket/summarizeLaneRuntimehere are near-duplicates of the same-named functions inapps/ade-cli/src/services/sync/syncRemoteCommandService.ts. See consolidated comment for the cross-file recommendation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/lanes/laneListSnapshotService.ts` around lines 104 - 183, Remove the duplicated sessionStatusBucket and summarizeLaneRuntime implementations from laneListSnapshotService.ts and reuse the canonical same-named helpers from syncRemoteCommandService.ts, consolidating shared bucket and lane rollup logic while preserving the existing outputs and precedence behavior.apps/ade-cli/src/services/push/pushPublisherService.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame "at-rest + explicitly settled" predicate is implemented three times across two files. Each site independently re-derives whether a CLI session is at rest (
status/runtimeState) and explicitly settled (settleOverride/settledAt), risking silent drift if the settlement rule changes in only one place.
apps/ade-cli/src/services/push/pushPublisherService.ts#L1567-1633: extract theatRest && (settleOverride === "settled" || (settleOverride !== "active" && settledAt))predicate fromonCliRuntimeSignalinto a shared, exported helper (e.g.isAtRestAndSettled) taking{status, runtimeState, settleOverride, settledAt}.apps/ade-cli/src/services/push/pushPublisherService.ts#L925-956: reuse the same helper inresolveMissingMeta's settle fast path instead of re-derivingatRest/settled inline.apps/ade-cli/src/bootstrap.ts#L691-708: import and reuse the same helper in thesessionService.onChangedlistener instead of duplicating the condition a third time.🤖 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/ade-cli/src/services/push/pushPublisherService.ts` at line 1, Extract the shared at-rest-and-settled predicate from onCliRuntimeSignal into an exported isAtRestAndSettled helper accepting status, runtimeState, settleOverride, and settledAt. Replace the inline checks in resolveMissingMeta and the bootstrap sessionService.onChanged listener with this helper, preserving the existing settlement semantics.
925-956: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated atRest/settled predicate (see consolidated comment).
Same
atRest && settledpredicate asonCliRuntimeSignalbelow andbootstrap.ts's session-changed listener. Worth consolidating into a shared helper — see the cross-file consolidated comment.🤖 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/ade-cli/src/services/push/pushPublisherService.ts` around lines 925 - 956, Extract the duplicated at-rest and settled-session predicate into a shared helper, preserving the existing settleOverride and settledAt semantics. Update resolveMissingMeta, onCliRuntimeSignal, and bootstrap.ts’s session-changed listener to use the helper instead of repeating the inline condition.apps/ade-cli/src/bootstrap.ts (1)
691-708: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSettlement/
atRestpredicate duplicated across files.The
(status/runtimeState) atRest && (settleOverride/settledAt) settledcheck here re-implements the same predicate found inpushPublisherService.ts'sresolveMissingMetaandonCliRuntimeSignal. Three near-identical copies of business-critical attention/settlement logic across two files is a drift risk — a future tweak to one copy (e.g. adding a new settle source) could silently miss the others.Consider extracting a shared predicate (e.g.
isAtRestAndSettled(status, runtimeState, settleOverride, settledAt)) into a shared module consumed by all three sites.🤖 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/ade-cli/src/bootstrap.ts` around lines 691 - 708, Extract the shared at-rest-and-settled predicate used by the sessionService.onChanged callback and pushPublisherService’s resolveMissingMeta and onCliRuntimeSignal into a common module. Replace all three inline checks with that helper, preserving the current status, runtimeState, settleOverride, and settledAt semantics and updating all call sites to use the single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/components/lanes/laneAgents.ts`:
- Around line 41-44: Update the lane activity projection around the summary
attention checks to treat attentionSource === "provider_structured" as awaiting
input alongside pendingInputItemId and attentionRequestedAt. Apply the same
condition to the input-hint projection referenced by the additional location,
preserving existing runtime-state handling for other sessions.
---
Outside diff comments:
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 2237-2269: Replace the duplicated inline chat projection in
listRemoteWorkSessions with the existing projectChatOntoSession helper. Preserve
the surrounding visibility and running-session checks, and pass each eligible
session and its matching chat through that helper so orchestration fields,
pending-input fallback, and ended-chat handling remain consistent with
getRemoteWorkSession.
In `@apps/desktop/src/renderer/lib/terminalAttention.ts`:
- Around line 177-186: Update sessionNeedsChatTabHighlight to accept an
attentionSource argument, then return true when it equals "provider_structured"
in addition to the existing pendingInputItemId and attentionRequestedAt
conditions. Ensure callers pass the session’s attentionSource through to
preserve highlighting for structured provider attention.
In `@apps/desktop/src/shared/types/sync.ts`:
- Line 1856: Ensure deflate envelopes are decoded only after deflate has been
negotiated: update decodeEnvelopeText or the handleMessage call path to reject
compression: "deflate" unless latestHello?.compression?.codec is "deflate".
Preserve decoding for gzip and other valid negotiated envelopes.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 1803-1804: Update the chunk-size calculation in
syncEncodeEnvelopeFrames to remove the fixed 16 KiB minimum and derive a
positive partBytes value directly from the negotiated maxFrameBytes, while
preserving the existing frame-overhead adjustment and total calculation so small
budgets such as 1,025 bytes are honored.
---
Nitpick comments:
In `@apps/ade-cli/src/bootstrap.ts`:
- Around line 691-708: Extract the shared at-rest-and-settled predicate used by
the sessionService.onChanged callback and pushPublisherService’s
resolveMissingMeta and onCliRuntimeSignal into a common module. Replace all
three inline checks with that helper, preserving the current status,
runtimeState, settleOverride, and settledAt semantics and updating all call
sites to use the single implementation.
In `@apps/ade-cli/src/services/push/pushPublisherService.ts`:
- Line 1: Extract the shared at-rest-and-settled predicate from
onCliRuntimeSignal into an exported isAtRestAndSettled helper accepting status,
runtimeState, settleOverride, and settledAt. Replace the inline checks in
resolveMissingMeta and the bootstrap sessionService.onChanged listener with this
helper, preserving the existing settlement semantics.
- Around line 925-956: Extract the duplicated at-rest and settled-session
predicate into a shared helper, preserving the existing settleOverride and
settledAt semantics. Update resolveMissingMeta, onCliRuntimeSignal, and
bootstrap.ts’s session-changed listener to use the helper instead of repeating
the inline condition.
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 3525-3599: The sessionStatusBucket and summarizeLaneRuntime
implementations duplicate the canonical bucket and lane-rollup logic from
laneListSnapshotService.ts. Replace this local logic with the shared
implementation, reusing its established symbols and preserving the existing
LaneListSnapshot runtime shape.
In `@apps/desktop/src/main/services/lanes/laneListSnapshotService.ts`:
- Around line 104-183: Remove the duplicated sessionStatusBucket and
summarizeLaneRuntime implementations from laneListSnapshotService.ts and reuse
the canonical same-named helpers from syncRemoteCommandService.ts, consolidating
shared bucket and lane rollup logic while preserving the existing outputs and
precedence behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 80fbb6cc-4938-4787-9980-45eaae8bb911
⛔ Files ignored due to path filters (5)
docs/features/agents/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-sessions.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (50)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/services/push/pushPublisherService.test.tsapps/ade-cli/src/services/push/pushPublisherService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/sessionLifecycle.test.tsxapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/sessionLifecycle.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.test.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prMergeAutoSettlementService.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sessions/chatSessionProjection.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sessions/settleTerminalSession.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/lanes/laneAgents.test.tsapps/desktop/src/renderer/components/lanes/laneAgents.tsapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.test.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsxapps/desktop/src/renderer/components/terminals/sessionLifecycleActions.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/components/work/SessionLifecycleChips.test.tsxapps/desktop/src/renderer/lib/terminalAttention.test.tsapps/desktop/src/renderer/lib/terminalAttention.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/sessionCanonicalState.test.tsapps/desktop/src/shared/sessionCanonicalState.tsapps/desktop/src/shared/types/sessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkSessionCanonicalState.swiftapps/ios/ADETests/WorkSessionCanonicalStateTests.swift
💤 Files with no reviewable changes (2)
- apps/desktop/src/main/utils/terminalSessionSignals.ts
- apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/renderer/components/terminals/SessionCard.test.tsx
- apps/desktop/src/renderer/components/lanes/laneAgents.test.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
- apps/desktop/src/shared/adeCliGuidance.ts
- apps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.ts
- apps/ios/ADE/Views/Work/WorkRootComponents.swift
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
* ci: build and test the iOS app on every PR The iOS app was the only ADE surface with zero CI coverage; three consecutive merges (#1117, #1120, #1121) each landed a Swift compile break, and 6 ADETests failures accumulated invisibly. New test-ios job on macos-26 (Xcode 26) builds the ADE scheme for testing and runs ADETests. It always runs — ci-pass deliberately counts "skipped" as failure — but exits success immediately on PRs that don't touch apps/ios/** or ci.yml, so non-iOS PRs pay only runner spin-up. SPM packages cached on Package.resolved. Make the suite it gates green (1494 tests, 0 failures locally): - Three PR-list tests still built 'queue' group fixtures; queue workflows were removed in 1b3d33b and the joins narrowed to integration groups. Fixtures now use 'integration'; the scoping/filter subjects and every other assertion are unchanged. - testFilterPullRequestListItemsMatchesStateAndSearch asserted a search for "review" returns one row, but both fixtures contain "review" in title/branch — wrong since the day it landed (4f18960); state narrowing is covered by the following assertions. - testRosterCleanExitAndLegacyPayloadRemainCompatible expected clean exit to settle; 31bac9b (#951) made settle declared-only. Expect .ended and additionally pin exitCode == 0. - testRelayCandidateRuntimeIgnoresReadyBeforeAccepted raced a real 350 ms negotiation deadline against the host scheduler. The budget is now a SyncConnectionRaceBudget field (production defaults byte-identical) and the test hook widens only that window; assertions untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: let the iOS test host sign ad-hoc; surface full failure detail CODE_SIGNING_ALLOWED=NO left the test host unsigned, so simulator keychain access failed with missing-entitlement errors in the account sign-out and DPoP proof tests (they pass locally, where the host signs ad-hoc). Also replace output truncation with -quiet and upload the .xcresult bundle on failure so CI failures are diagnosable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: address review — persist-credentials off, timeout test keeps a real window - test-ios checkout no longer persists the GITHUB_TOKEN into .git/config; xcodebuild runs PR-controlled build phases and needs no authenticated git. - awaitRelayCandidateReadyForTesting takes an acceptedWindowNanoseconds override; the negotiation-timeout test passes 50ms so it exercises the timeout path without sitting out the wide scheduling-safe window (SyncRecoveryPolicyTests back to ~9s). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
New Features
listLanescan include status, with new lane summary helpers.Bug Fixes
Documentation