fix(steer): stop a steered Slack turn from posting "no response" and losing its answer - #655
Conversation
…losing its answer
Steering a running turn in Slack posted the literal "응답 없음" placeholder into
the thread, and the follow-up run's real answer never arrived. Observed on suji
(cli: cursor, transport: print, midRunPolicy: steer), where every steer takes the
kill path because a print-transport run exposes neither steerTurnInBand nor
replaceTurn:
[slack:in] 아니 15일이 다음주 화요일인데 월요일로 대답했잖아
[slack:in] ㅎㅇ <- steers the running turn
[jaw:kill] reason=steer cli=cursor
[jaw:main] exited code=143, text=0 chars <- killed turn
[slack:out] 응답 없음 <- posted to the thread
[jaw:main] exited code=0, text=12 chars <- real answer, never sent
Two independent defects produced that pair.
The killed turn resolved to the placeholder. Its collector was still waiting when
the steer killed the process, so the terminal carried no text and collect.ts fell
through to its t('tg.noResponse') default. That default is meant for a turn that
genuinely produced nothing, not for one deliberately retired mid-flight. The
collector now watches steer_started for its own scope and, when a different
request retires the turn, resolves empty and marks the payload superseded; the
Slack dispatch path stays silent on that marker instead of posting an empty body.
An ordinary empty terminal keeps the existing diagnostic.
The follow-up answer had no route back. The kill path built its orchestrate meta
from origin/scope/chatSessionId/requestId only, dropping target, chatId, remoteKey
and replyViaTarget, so the follow-up terminal was unaddressed. Nothing else could
recover it either: ingress returns early unless the disposition is 'new_run', so a
steered submission leaves no dispatch waiter, and the standing target-reply
forwarder delivered fromQueue turns exclusively. The kill path now carries the
owning conversation through to the follow-up run and marks it fromSteer, and the
forwarder treats that as the same kind of orphan it already handles for queued
turns.
Verified with npm run typecheck, the five existing steer suites (48 passing), and
a new regression that drives the real collector through supersession, an ordinary
empty terminal, and a steer belonging to another scope.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![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: 254b700e35
ℹ️ 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".
| broadcast('steer_started', stripUndefined({ prompt: newPrompt, origin: source || 'web', scope: scopeKey, | ||
| sessionId: chatSessionId, target: meta?.target, chatId: meta?.chatId, requestId: meta?.requestId, | ||
| remoteKey: meta?.remoteKey, replyViaTarget: meta?.replyViaTarget, mode: 'kill-steer' })); |
There was a problem hiding this comment.
This event is emitted only after waitForExitSettled, but the standard CLI exit handler resolves the old spawn promise before its settleExit callback runs; that lets the old pipeline broadcast orchestrate_done and the Slack collector post tg.noResponse before superseded can be set. Consequently ordinary kill-steer requests still exhibit the reported placeholder regression. Emit a separate retirement signal before the exit wait while retaining the exit-settle/salvage barrier for starting the follow-up.
AGENTS.md reference: AGENTS.md:L285-L289
Useful? React with 👍 / 👎.
| const steerMeta = stripUndefined({ origin, scope: scopeKey, chatSessionId, requestId: meta?.requestId, | ||
| target: meta?.target, chatId: meta?.chatId, remoteKey: meta?.remoteKey, | ||
| replyViaTarget: meta?.replyViaTarget, _fromSteer: true, | ||
| _skipInsert: true, _steerContext: steerContext || undefined }); |
There was a problem hiding this comment.
The new _fromSteer marker is consumed only by orchestrate(), although the kill-steer branch routes reset prompts to orchestrateReset() and idle continue prompts to the direct terminal in orchestrateContinue(). Those terminals propagate only fromQueue, so Slack's new forwarder rejects their orchestrate_done; because ingress returned without a live waiter for the steered submission, users never receive Reset complete. or No pending work to continue.. Propagate fromSteer from both direct terminal paths as well.
AGENTS.md reference: AGENTS.md:L287-L289
Useful? React with 👍 / 👎.
| if (type === 'steer_started' | ||
| && data['scope'] === binding.scope | ||
| && (data['sessionId'] === undefined || data['sessionId'] === binding.chatSessionId) | ||
| && (!requestId || data['requestId'] !== requestId)) { | ||
| superseded = true; |
There was a problem hiding this comment.
This condition also matches native-input and cancel-reprompt events, although those modes keep the existing collector as the owner of the replacement answer rather than creating an orphaned _fromSteer run. If such a native turn ends absent/empty or with no renderable diagnostic, the collector now returns empty with superseded: true, and Slack treats that as success without sending the prior no-response diagnostic; no standing forwarder can replace it. Require data.mode === 'kill-steer' when setting this flag.
AGENTS.md reference: AGENTS.md:L287-L288
Useful? React with 👍 / 👎.
| requestId, | ||
| replyViaTarget, | ||
| ...(fromQueue ? { fromQueue: true } : {}), | ||
| ...(fromSteer ? { fromSteer: true } : {}), |
There was a problem hiding this comment.
Running the repository-required bash structure/verify-counts.sh on this commit fails because structure/str_func.md still records the pre-change sizes for src/agent/spawn.ts, src/orchestrator/pipeline.ts, src/orchestrator/collect.ts, and src/slack/bot.ts. Regenerate those derived counts with the documented --fix command and commit the synchronized architecture file.
AGENTS.md reference: AGENTS.md:L351-L354
Useful? React with 👍 / 👎.
Resolves the #660 review before landing: - Slack tool turns keep the default steer policy; only synthetic thread addresses force followup, so a second Slack message during a tool turn still steers instead of queueing (regression against #655). - The A1 tool anchor now states that a grant exists only for a fresh print Cursor/Claude/Codex/Grok main turn, so the prompt stops advertising tools that 401 on Codex App, Pi, pooled native adapters and workers. - A live DM whose conversations.info omits sharing fields no longer fails closed; the two-person membership and users.info workspace checks are what prove the DM. - The operator credential is minted on first operator use instead of every boot, so an install that never uses operator mode holds no operator secret. docs/slack-tools.md records the accepted deployment model for the residual #646 limitations, the in-conversation scope of pin/reaction/bookmark creation, and the requester-visible workspace roster.
1 participant
Problem
Steering a running turn in Slack posted the literal
응답 없음("no response") placeholder into the thread, and the follow-up run's real answer never arrived. The thread then looked dead even though the agent had finished the work.Observed live on suji (
cli: cursor,perCli.cursor.transport: print,midRunPolicy: steer). A print-transport run exposes neithersteerTurnInBandnorreplaceTurn, so every steer there takes the kill path:Two independent defects produce that pair.
The killed turn resolved to the placeholder. Its collector was still waiting when the steer killed the process, so the terminal carried no text and
collect.tsfell through to itst('tg.noResponse')default. That default exists for a turn that genuinely produced nothing, not for one deliberately retired mid-flight.The follow-up answer had no route back. The kill path built its orchestrate meta from
origin/scope/chatSessionId/requestIdonly, droppingtarget,chatId,remoteKeyandreplyViaTarget— so the follow-up terminal was unaddressed. Nothing else could recover it:ingress.tsreturns early unless the disposition isnew_run, so a steered submission leaves no dispatch waiter, and the standing target-reply forwarder deliveredfromQueueturns exclusively.Change
The collector watches
steer_startedfor its own scope. When a different request retires the turn it resolves empty and marks the payloadsuperseded; the Slack dispatch path stays silent on that marker rather than posting an empty body. Identity is checked on both scope and session, and a steer carrying the turn's own request id is not a supersession, so an unrelated conversation cannot retire this one. An ordinary empty terminal keeps its existing diagnostic.The kill path now carries the owning conversation through to the follow-up run and tags it
fromSteer. The Slack target-reply forwarder treats that as the same kind of orphan it already handles for queued turns, so the answer lands in the thread it came from.Behaviour
응답 없음응답 없음Validation
npm run typecheck— clean.cursor-acp-steer,native-steer-handler,codex-app-steer,queue-steer-main-wait,retired-busy-steer) — 48/48 passing.tests/unit/steer-superseded-delivery.test.tsdrives the real collector through supersession, an ordinary empty terminal, and a steer belonging to another scope.npm testcompared against anorigin/devbaseline worktree: no new failures. Head shows 23 failures, baseline 22; the delta is two load-sensitive timing tests that pass in isolation, plus one file that fails identically on the untouched baseline (browser-web-ai-headed-policy, ENOENT onskills_ref/web-ai/SKILL.md— the submodule is not initialised in either worktree). All are environmental and in files this PR does not touch.