fix(coding-agent): scope continueQueuedMessages to explicit queue-consuming continuations - #4127
fix(coding-agent): scope continueQueuedMessages to explicit queue-consuming continuations#4127Yeachan-Heo wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1831007df8
ℹ️ 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 continueQueued = options?.continueQueuedOnly | ||
| ? this.agent.continueQueuedMessages.bind(this.agent) | ||
| : this.agent.continue.bind(this.agent); |
There was a problem hiding this comment.
Preserve queue-only overflow recovery
When overflow maintenance finds nothing eligible to compact while the history has a resumable non-assistant tail, #runAutoCompaction deliberately schedules the queued_continue path at lines 14922-14932 only when a follow-up or steer is waiting. With this new default, that path calls agent.continue() and resubmits the same oversized tail instead of consuming the queued message, recreating the replay that overflowNoopWouldReplay is meant to prevent and potentially leaving the user's prompt undelivered. Mark these explicit compaction queue drains as continueQueuedOnly as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on exact PR head a3a3abd0f7160eebf072f04c8207b864a8a44d52.
The overflow no-op queued_continue branches now opt into continueQueuedOnly, and focused follow-up + steer regressions construct the persisted and in-memory resumable tail explicitly. Evidence after rebasing onto current dev (515ef1aa63ca32fc833ee0f3a6a991d6b8e37a1d):
- exact two regressions:
2 pass / 0 fail - full focused file:
21 pass / 1 skip / 0 fail bun --cwd=packages/coding-agent run check:types: pass- Biome check on the edited source/test: pass
- PR head contains exact current base and the worktree is clean
The same PR branch was force-updated with lease; no merge was performed.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
1831007 to
a3a3abd
Compare
MERGE_READY / LGTM — exact head
|
|
BLOCKER — verified regression. This PR breaks a test file it does not touch. MethodRebased the PR head onto current Result
One of those failures is pre-existing ( Cause, isolated to one expressionI reverted only the routing selector, keeping everything else in the PR including the new // PR (agent-session.ts:4980-4982)
const continueQueued = options?.continueQueuedOnly
? this.agent.continueQueuedMessages.bind(this.agent)
: this.agent.continue.bind(this.agent);Reverting just that back to the tail-role selector → 10 pass / 1 fail, i.e. back to the WhyThe threshold-compaction resume path reaches The PR's premise is right: routing on "the last message is a non-assistant execution tail" is the wrong signal, and an explicit flag is the correct fix. The flag is just not yet applied at every site that legitimately drains a queue. The threshold-compaction resume is a genuine queue-consuming continuation and needs To unblock
Also needs a rebase for exact-head CI regardless. Not questioning the design — the flag is the right call. This is a missed call site, and it is the kind of miss the existing broad-suite evidence in the PR description could not surface, because that run predates current |
probepark
left a comment
There was a problem hiding this comment.
Requesting changes — see the detailed analysis in my earlier comment.
Summary: rebased onto current dev and this PR regresses packages/coding-agent/test/agent-session-auto-compaction-queue.test.ts, a file it does not modify. Clean dev gives 10 pass / 1 fail; with this PR, 9 pass / 2 fail.
Isolated to one expression — reverting only the continueQueued routing selector at agent-session.ts:4980-4982 restores the baseline, so startsQueuedSuccessor is not implicated.
Cause: the threshold-compaction resume reaches #scheduleAgentContinue without continueQueuedOnly, so it now takes agent.continue and never drains the queued message. Only 6 of 21 call sites pass the flag.
The design is right — an explicit flag beats routing on "the last message is a non-assistant execution tail". The flag is just not applied at every site that legitimately drains a queue. Add it to the threshold-compaction resume and audit the other 15 flagless sites against the code rather than the CHANGELOG's intent.
Also needs a rebase for exact-head CI.
a3a3abd to
e13e945
Compare
…suming continuations continueQueuedMessages() whenever the last message was a non-assistant execution tail. Retry, fallback, auto-compaction, deep-interview, active-goal, TTSR, and reminder continuations then either drained the queue without continuing the current context or failed with "No queued messages to continue" when nothing was queued, breaking AgentSession continuation across all eight dev CI coding-agent shards (99 local failures across 15 agent-session files). The queue-consuming continuation is now opt-in via continueQueuedOnly and used only by the explicit follow-up/steer drain scheduled after foreground bash/eval/python settles, which must avoid stale-tail replay. All other scheduled continuations keep continue() semantics. Focused regression proof: 238 pass / 1 skip / 0 fail across 16 agent-session files covering every shard failure class (fallback attempt accounting, queued successor delivery, auto-compaction, manual retry, deep-interview, active-goal reminders, resilient retry, retry-delay, TTSR, todo reminders, retry busy recovery, mid-run compaction) plus the #4120 fencing cohort (93 pass: queued-prompts, issue-825, input-controller keybindings/skill-queue). Broad 70-file AgentSession run: 769 pass / 20 skip / 0 fail (re-run; first run had one unrelated flaky agent-session-eager-todo fsync test that passes in isolation and at HEAD). Lore-id: 4124-postmerge-continuation-r2 Constraint: preserve ordered follow-up delivery without concurrent model turns Constraint: keep continueQueuedMessages only for explicit queue-consuming scheduled continuations Rejected: exact revert of #4120 | drops the bash/eval/python queue-fencing intent Rejected: role-based selection with assistant-only continueQueuedMessages | still breaks non-assistant retry/compaction tails Confidence: high Scope-risk: medium Reversibility: revert-commit Tested: 238 + 93 focused plus 769 broad agent-session tests Not-tested: full 777-file coding-agent suite (remaining shards covered by CI)
Review follow-up: the explicit queue-consuming continuation must also be used by the auto-compaction queued_continue recovery paths (overflow no-op with a resumable non-assistant tail, pre/post-compaction queued-message delivery) and the user-interrupt steering drain. With the default continue() these drains replay the oversized non-assistant tail and strand the queued follow-up/steer, recreating the stale-tail replay the #4120 fencing was meant to prevent. Lore-id: 4124-postmerge-continuation-r2 Constraint: keep continueQueuedMessages only for explicit queue-consuming scheduled continuations Constraint: preserve ordered follow-up delivery without concurrent model turns Confidence: high Scope-risk: narrow Reversibility: revert-commit Tested: focused agent-session regression cohorts plus fencing cohort Not-tested: full 777-file coding-agent suite (CI covers remaining shards)
Auto-compaction overflow no-op recovery must consume queued follow-up and steering messages without replaying the resumable oversized tail. The regressions also keep queued continuation busy handling bound to continueQueuedMessages(). Lore-id: 4124-postmerge-continuation-r2-overflow Constraint: preserve queue-only overflow recovery from discussion_r3745892872 Constraint: keep retry continuations on continue() semantics Confidence: high Scope-risk: narrow Reversibility: revert-commit Tested: bun test packages/coding-agent/test/agent-session-auto-compaction-continue.test.ts (21 pass, 1 skip)
Auto-compaction queue resumes are explicit queue-consuming continuations. Assert continueQueuedMessages after threshold and pre-continue maintenance so the regression contract matches the follow-up/steer ingress fix rather than accepting ordinary tail replay. Lore-id: 4124-current-dev-queue-resume Constraint: ordinary retry and maintenance continuations retain continue() semantics Constraint: explicit queued drains use continueQueuedMessages() Confidence: high Scope-risk: narrow Reversibility: revert-commit Tested: 54 pass, 1 skip across auto-compaction queue, deep-interview continuation, and auto-compaction continuation files
An idle genuine or synthetic steer is an explicit queue drain, but it still used the ordinary continuation path. That replayed continuation ownership, skewed deep-interview attempt accounting, and could consume or supersede ingress through the wrong path. Route the scheduled idle steer through continueQueuedMessages and pin stale-stop behavior to the queue-specific call. Lore-id: 4124-idle-steer-queue-ingress Constraint: genuine ingress claims deep-interview ownership exactly once Constraint: synthetic and agent-attributed ingress cannot supersede genuine ownership Constraint: ordinary retry/fallback/maintenance continuations retain continue() Confidence: high Scope-risk: narrow Reversibility: revert-commit Tested: 54 pass, 1 skip across three continuation regression files Tested: coding-agent package check Tested: four managed fallback/overflow interaction cases
e13e945 to
dc09d83
Compare
MERGE_READY / LGTM — refreshed exact head
|
Yeachan-Heo
left a comment
There was a problem hiding this comment.
MERGE_READY — owner hold 해제. Exact-head CI가 terminal green이며 현재 확인된 unresolved blocker가 없습니다. Merge/release는 owner-controlled 단계로 남깁니다.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Regression
Fixes #4124. Dev merge
e8a600c557for #4120 broke AgentSession continuation across all eight coding-agent CI shards: its continuation selection routed every scheduled continuation throughcontinueQueuedMessages()whenever the last message was a non-assistant execution tail. Retry, fallback, auto-compaction, deep-interview, active-goal, TTSR, and todo-reminder continuations then either drained the queue without continuing the current context or failed withNo queued messages to continuewhen nothing was queued.Local reproduction on exact current dev (
6608c201, base of this branch): 99 failing tests across 15 agent-session files, mapping one-to-one onto the issue's shard failure classes:agent-session-fallback-upstream-count.e2eagent-session-auto-compaction-continue,agent-session-manual-retryagent-session-deep-interview-continuation,agent-session-goal-reminderagent-session-retry-fallback,agent-session-fallback-attempt-transactionagent-session-deep-interview-continuation,agent-session-resilient-retry,agent-session-todo-reminderagent-session-retry-busy-recovery,agent-session-invalid-prompt-retry,agent-session-retry-cap,agent-session-ssh-refresh,agent-session-midrun-compaction,agent-session-auto-compaction-queueFix
Smallest safe change to the shared continuation admission boundary in
packages/coding-agent/src/session/agent-session.ts:continueQueuedMessages()is now opt-in via a newcontinueQueuedOnlyoption on#scheduleAgentContinueand is used only by the explicit queue-consuming scheduled continuations: the follow-up/steer drain scheduled after foreground bash/eval/python settles (#scheduleQueuedFollowUpContinuationand the python-result continuation), which must avoid stale-tail replay.continue()semantics, restoring the pre-fix(coding-agent): gate queued prompts during busy operations #4120 contract for retry, fallback, compaction, deep-interview, goal-reminder, TTSR, and reminder continuations.The #4120 queue-fencing intent is preserved: queued prompts still wait for compaction/foreground bash/eval to settle and are delivered through the dedicated queue continuation afterwards.
Verification
238 pass / 1 skip / 0 failacross 16 agent-session files, includingagent-session-queued-prompts.test.ts+issue-825-repro.test.ts+input-controller-keybindings.test.ts+input-controller-skill-queue.test.ts(fencing cohort:93 pass / 0 fail).769 pass / 20 skip / 0 fail(re-run; first run had one unrelated flakyagent-session-eager-todofsync test that passes in isolation and passes at HEAD — classified unrelated to this change, matching the prior lane's finding).670 pass / 20 skip / 99 fail.bunx @biomejs/biome@2.5.2 checkon the edited file passes;bun --cwd=packages/coding-agent run check:typespasses after generating the ignored docs index.This lane supersedes the crashed prior continuation lane for #4124; the uncommitted work was independently reconstructed against exact dev and this is the only PR for the issue.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]