Conversation
github-actions
Bot
added
scope: cli
scope: shared
scope: components
scope: agent-runtime
status:needs-pr-attention
|
@tommy0103, this pull request needs updates before review. It is marked If the PR remains invalid for 7 days, it will be closed and marked Policy findings |
0253ff4 to
d27b19e
Compare
tommy0103
changed the title
![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: d27b19e2a2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
| // A dying ACP process ends any engine-opened turn it hosted: drop the | ||
| // activity marker so busy status and the idle-GC guard release the session. | ||
| this.onSessionTerminated((sessionId) => this.handler?.clearEngineTurnActivity(sessionId)); |
There was a problem hiding this comment.
When an autonomous turn's ACP process exits without emitting turnEnded, this callback never runs because onSessionTerminated is fed only by the terminated listener, while SessionManager also reports agent death through exit. The stale marker then makes live status remain running and causes hasActiveTurn() to exempt the session from idle GC indefinitely; clear it on the exit lifecycle path as well.
AGENTS.md reference: apps/cli/AGENTS.md:L59-L63
Useful? React with 👍 / 👎.
|
Verified and fixed: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea4b53ae12
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| state.turn.phase !== 'idle' || | ||
| state.engineTurn !== undefined || |
There was a problem hiding this comment.
When a cron/task turn is active, this method now considers the session busy, but the isSourceBusy callback passed to SessionForkService still independently calls resolveSessionLiveStatus() without consulting engineTurn. Because autonomous turns have neither normal execution ownership nor presence, a fork during such a turn is treated as an idle-source fork and bypasses the supportsActiveTurnFork() and resident ACP identity checks in forkInner, allowing session/fork to run against an active source without the required advertised capability. Route this activity state into the fork predicate as well.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L97-L102
Useful? React with 👍 / 👎.
|
Valid — the fork busy predicate ( |
420280b to
80330df
Compare
|
Confirmed against the cancel path: the unfinished autonomous entry flows through |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 519310d1b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| !( | ||
| row.item.message.acpTurnId && isAutonomousTurnId(row.item.message.acpTurnId) | ||
| ) && |
There was a problem hiding this comment.
When a scheduled or background turn finishes, buildChatStreamItems still publishes that autonomous entry as lastCompletedAssistantMessageId. This guard hides only the per-row fork action; SessionChatInterface.handleForkFromMenu and SessionDetail's getLastAssistantTurnId() path still submit the autonomous ID, which cloneHistoryThroughTurn rejects, so header and side-panel forks fail until another normal assistant turn completes. Track the last forkable assistant separately.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L97-L103
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| this.deps.logger.warn( | ||
| `[${sessionId}] Failed to cancel engine-opened turn: ${formatErrorMessage(error)}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
When agentClient.cancel() rejects for an active engine-opened turn, this catch only logs the error; the marker was already cleared and the function subsequently returns { success: true }. The agent can therefore continue background work while live status reports idle and the UI reports that Stop succeeded. Return the failure and retain or restore the activity marker until cancellation or termination is confirmed.
Useful? React with 👍 / 👎.
| // turn (a cron fire, a task wake) — the only live work on the agent. | ||
| // Cancel it through the plain ACP session cancel instead of reporting | ||
| // a stale no-op while the work continues. | ||
| if (this.deps.isEngineTurnActive(sessionId)) { |
There was a problem hiding this comment.
When a delayed stop request for an older client turn arrives after that turn ends but while an engine-opened turn is active, this boolean check treats the stale request as targeting the engine turn and cancels the newer work. This bypasses the exact-match guarantee documented immediately above the branch; expose the current engine turn ID and require the request to match its synthesized assistant entry before canceling.
Useful? React with 👍 / 👎.
|
Both fixed in b6471c6:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6471c6675
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const divider = engineTurnDividerFor(entry, sessionId); | ||
| if (divider) items.push(divider); |
There was a problem hiding this comment.
Whenever an engine turn precedes a search or pinned-message target, inserting this extra item shifts the chat-stream index away from the raw history index: SessionChatInterface still passes activeSearchResult.messageIndex and history.findIndex(...) directly to scrollToIndex, while that method indexes this expanded items array. Pinned-message navigation therefore lands one message early per preceding engine turn, and search can fail to reveal distant targets once enough dividers put the intended row outside Virtua's mounted buffer; translate history indices to stream indices or scroll by message ID instead.
AGENTS.md reference: packages/components/src/components/ai-gui/AGENTS.md:L80-L83
Useful? React with 👍 / 👎.
tommy0103
changed the title
b6471c6 to
05282b8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05282b858d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return { success: false, error: 'The agent is no longer connected.' }; | ||
| } | ||
| try { | ||
| await session.agentClient.cancel(session.acpSessionId); |
There was a problem hiding this comment.
When a user dispatch starts after the initial ownership snapshot but before this awaited session-wide cancel completes, the cancel can hit the new client prompt rather than the engine turn; MessageProcessor.extractQueueKey deliberately puts session/cancel on an unkeyed lane, so dispatch is not serialized behind it. Afterward, this branch also clears the newly claimed turn's presence. Hold the session conflict lease across the cancel or recheck ownership before clearing state so Stop cannot interrupt a turn that started during the await.
Useful? React with 👍 / 👎.
|
Fenced in 19e7176: the check-cancel-clear sequence now runs inside the session rewrite barrier (the same primitive dispatch/steer respects), and the activity marker is rechecked inside it. A client turn can no longer start between the ownership snapshot and the awaited |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19e71766f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| presence: this.sessionActivePresence.getStatus(sessionId), | ||
| execution: this.executionService.getExecutionSnapshot(sessionId), | ||
| hasPendingDispatch: this.sessionDispatchWatcher.hasPendingDispatch(sessionId), | ||
| engineTurnActive: this.store.isEngineTurnActive(sessionId), |
There was a problem hiding this comment.
When an autonomous turn is producing updates and the user submits another message, this status makes the renderer treat the session as busy and enqueue the message. However, SessionDispatchWatcher builds both dispatch snapshots solely from executionService.getExecutionSnapshot() and durable session metadata, neither of which includes engineTurn; the queue mutation therefore wakes the watcher, which can immediately promote and dispatch the queued prompt against the same ACP agent while the cron/task turn is still running. Thread this activity predicate into daemon dispatch admission so queued work remains queued until the autonomous turn ends.
Useful? React with 👍 / 👎.
|
Fixed in 35a8bc7: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35a8bc7056
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const hasActiveTurn = | ||
| (typeof activeTurnId === 'string' && activeTurnId.length > 0) || engineTurnActive; |
There was a problem hiding this comment.
When an engine-opened turn is active, this reports hasActiveTurn: true while leaving activeTurnId undefined. queueGoalTurn() only waits when both fields are present (session-execution-service.ts:1303-1306), so a Set/Resume action falls through to startGoalTurn() and opens a second ACP prompt against the same agent while the autonomous turn is still running, violating the goal contract and risking interleaved output. Add a waitable engine-turn owner or make the goal queue wait directly for engine activity to clear.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L50-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95c24c8575
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| !('title' in update.update) && | ||
| !('updatedAt' in update.update) | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
When a Kimi cron/task turn has emitted only its metadata-only turn.started marker, this return records the engine as busy but creates no unfinished assistant entry. The frontend derives activeAssistantTurnId solely from history, and canStopAgentEnabled requires that ID, so Stop is unavailable throughout the potentially long or hung pre-first-output period. Persist an owner entry that rendering may hide, or expose the autonomous cancel ID through live status so the turn can be stopped immediately.
Useful? React with 👍 / 👎.
| const isForkableAssistantMessage = (message: SessionHistoryParsed): boolean => | ||
| message.role === 'assistant' && | ||
| message.finished === true && | ||
| (!message.acpTurnId || !isAutonomousTurnId(message.acpTurnId)); |
There was a problem hiding this comment.
When a completed legacy assistant without acpTurnId is followed by a completed autonomous entry, this predicate selects the legacy entry as lastForkableAssistantMessageId, so the header and side-panel actions submit it. However, cloneHistoryThroughTurn rejects any ID-less source that has a later finished assistant, returning SOURCE_TURN_NOT_FORKABLE; the newly added selector therefore exposes an action guaranteed to fail in the exact legacy-before-autonomous case. Fresh evidence beyond the earlier fork-target comment is that this new selector explicitly classifies that incompatible case as forkable; either keep legacy entries eligible only while they satisfy the CLI rule or align the CLI rule with this selection.
Useful? React with 👍 / 👎.
…y entries An agent engine can open turns of its own behind the client's back (a kimi cron fire, a task wake). Their ACP updates carried no turn identity, so the history layer appended them to the last client turn's assistant entry: the real reply was folded away and the interrupting turn's text became the visible answer, and after a daemon restart such updates were dropped outright. - history-apply: route turn-stamped updates by ownership — scoped to kimi-stamped turns only (auto: id or turnOrigin marker): adopt only a delta-accepting entry or create assistant:autonomous-<turnId>. Every other id keeps the exact legacy last-wins restamping (claude per-message uuids, codex collab child turns are boundaries inside one client turn). turnEnded finalizes the owning entry once; acpTurnOrigin persists. - session-transient-store: route marked updates to a synthesized autonomous target even while a client turn owns the session; maintain an engine-turn activity marker (set on first marked update, cleared on end marker or ACP process termination) feeding hasActiveTurn (idle-GC guard) and live status unknown -> running. - Bump the kimi submodule for the matching stamping (turnId auto:<n> + turnOrigin + turnEnded on engine-opened turns). Model: kimi-code/kimi-for-coding
Model: kimi-code/kimi-for-coding
…-resend paths A finished engine-opened entry carries a deliberately non-fork-position auto: id, but both selection paths treated any finished entry with an acpTurnId as a fork boundary: Fork on such an entry forwarded the id to session/fork, and edit-and-resend on the next user turn picked it as the nearest preceding boundary. Skip autonomous entries while walking to the real provider boundary in both paths, and reject them as fork sources in cloneHistoryThroughTurn. Model: kimi-code/kimi-for-coding
SessionManager reports unexpected agent death only through exit (the terminated event fires solely on the explicit terminate path), so an engine turn whose process crashed without a turnEnded marker kept the session permanently busy and exempt from the idle GC. Clear the marker on the manager's exit event as well. Model: kimi-code/kimi-for-coding
The unfinished autonomous entry made resolveActiveAssistantTurnId hand its id to the Stop control, but cancelSession only interrupts ids owned by the client-turn registries, so the request fell through to the stale path, reported success, and the engine turn kept running. In the stale branch, an active engine turn now gets cancelled through the plain ACP session cancel (the only live work on the agent) and the marker clears. Model: kimi-code/kimi-for-coding
getSessionMonitorState read only presence and client-turn phase, so the machine monitor showed an engine-turn session as idle mid-turn; and resetMachineDisconnectedSessionsToIdle would reset a non-idle meta status on a session whose only live work is an engine-opened turn. Read the activity marker in both. Model: kimi-code/kimi-for-coding
…ession for engine turns Two review findings: - The engine-turn cancel path cleared the activity marker before the ACP cancel, logged failures, and still returned success: a failed stop left the cron/task turn running while status went idle and the GC guard released the session. Now the marker clears only after a delivered cancel; failures (or an unavailable ACP session) keep it and return success: false, distinguishing an undeliverable cancel from a cancelled turn. - enqueueACPUpdate ran recordSuppressedAcpReplay unconditionally, so a live engine turn firing during the loadSession replay-suppression window was dropped as replay (and its end marker with it, losing the turn permanently). Autonomous-identity updates resolve first and bypass suppression — replay carries no autonomous marker. Model: kimi-code/kimi-for-coding
The ACP session cancel is session-wide and dispatch is not serialized behind a stop request: a client turn starting between the ownership snapshot and the awaited cancel would take the bullet meant for the engine turn, and the branch would then clear the new turn's presence. Hold the rewrite barrier across the check-cancel-clear sequence and recheck the marker inside it, so no turn can start mid-flight and an already-ended engine turn is not cancelled twice. Model: kimi-code/kimi-for-coding
The dispatch snapshot read only client-turn ownership, so a user message arriving during an engine-opened turn was promoted and dispatched against the busy agent immediately (engine-side reject or a hidden engine queue). Include the activity marker in getExecutionSnapshot hasActiveTurn, so dispatch admission holds queued work until the engine turn's end marker clears it. Model: kimi-code/kimi-for-coding
Model: DimAgent
Model: DimAgent
95c24c8 to
5f3797d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f3797d0eb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const engineTurnActive = this.deps.isEngineTurnActive(sessionId); | ||
| const hasActiveTurn = | ||
| (typeof activeTurnId === 'string' && activeTurnId.length > 0) || engineTurnActive; |
There was a problem hiding this comment.
When a cron/task turn is active, this snapshot deliberately sets hasActiveTurn while omitting activeTurnId, but SessionEditAndResendService checks only freshExecution.activeTurnId after calling prepareReplacementSession. It therefore skips cancellation/waiting and can adopt the replacement ACP session and close the old one while the autonomous prompt is still executing, causing output loss or concurrent session mutation. Reject or wait on ownerless engine activity before preparing the replacement.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L50-L52
Useful? React with 👍 / 👎.
Separate engine prompt occupancy from client turn ownership and fail closed across provider preparation and persistence. Document the admission limitation and future architecture follow-ups. Model: DimAgent
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b104908e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Engine updates can replace the marker while ACP cancel is | ||
| // awaited. Never clear a replacement owner's activity. | ||
| if (this.deps.getEngineTurnOwnerForCancel(sessionId, turnId) === liveOwner) { | ||
| this.deps.clearEngineTurnActivity(sessionId, liveOwner); |
There was a problem hiding this comment.
When ACP acknowledges cancel() before the autonomous prompt has actually completed, clearing the marker here immediately makes getExecutionSnapshot() report the prompt slot as free and wakes engine-turn waiters, so a queued goal or new dispatch can start a second prompt while the cancelled work is still unwinding. The ordinary-turn path in this same method explicitly treats cancel acknowledgement as distinct from prompt completion; retain engine activity until turnEnded, raw prompt completion, or process termination confirms release.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L50-L51
Useful? React with 👍 / 👎.
Keep upstream completed-reply fork gating and context copying alongside the branch forkable-target logic that skips autonomous turns; union the session-history-writes evidence lists; rebase the kimi submodule branch onto upstream kimi main (usage accounting deltas) and re-point the gitlink; re-compress the session AGENTS rules to fit the 8 KiB gate while keeping both engine-turn boundaries; give the upstream steer-provenance test the engine-activity dependency. Model: DimAgent
2 participants
Related issue
Refs #640
Problem / pressure
#653 separates engine-opened turns into their own history entries. That new entry type then flowed through existing lifecycle predicates that were written for client-dispatched turns only: fork and edit-and-resend treated a deliberately non-fork-position
auto:id as a fork boundary; the engine-turn activity marker leaked when the ACP process died viaexit(neverterminated); Stop matched no client turn and reported success while the work continued, and a failed stop would have cleared state anyway; a live engine turn firing during theloadSessionreplay-suppression window was dropped as replay; the machine monitor and the disconnected-session idle reset could not see the turn.Summary
auto:) entries are skipped while walking to the real provider boundary in both the CLI and renderer selection paths,cloneHistoryThroughTurnrejects an engine-opened fork source, and the renderer no longer offers Fork on such entries.exitevent; machine monitor andresetMachineDisconnectedSessionsToIdleread the marker.session/cancel; the marker clears only after a delivered cancel, and a failed or undeliverable cancel keeps it and returnssuccess: false.enqueueACPUpdateresolves the autonomous identity before replay suppression, so a live engine turn firing during theloadSessionwindow is never dropped as replay (replay carries no autonomous marker).(The origin-divider UI that briefly lived here was reverted: a separate stream item breaks the
history index == chat items indexcoordinate contract thathandleScrollToMessageand search navigation rely on — see #660 for the properly constrained design.)Visual explanation
Simple change: predicate-level guards and one cancel-path branch, each mirrored by unit tests; no control-flow restructuring.
Before / after
auto:id; fails or forks wrongexitas well asterminatedsuccess: falseloadSessionsuppression dropped permanentlyTest plan
apps/cli: 148 targeted tests passing (session-fork, edit-and-resend, execution-service incl. new cancel-path cases, machine-runtime engine-turn, live-status predicate, transient store).packages/components: 15 stream-item tests passing; tsc clean everywhere; prettier clean.Context handoff
Instructions for reviewing agents
session-edit-and-resend-service.tsandsession-chat-interface.tsxbackward-walk skip conditions,session-fork-service.tscloneHistoryThroughTurnrejection,session-execution-service.tsengine-turn cancel branch (success ordering),message-handler.tsenqueueACPUpdatesuppression ordering,machine-runtime.tsexit wiring.Authoring context
session/cancel(cancels the agent's current turn, whatever it is); replay bypass trusts the autonomous marker as proof of liveness.Original user prompt
Show original prompt