Add thread controls: model, effort, modes, interrupt, approvals, and answers - #8
Conversation
- threads set changes an existing thread's model, provider instance, reasoning effort, fast mode, other model options, permission, and plan mode; threads send takes the same flags and applies them before its turn. Values are checked against T3's provider catalog, which T3 serves over WebSocket RPC, and mapped to each model's option ids: Codex fast mode is serviceTier=priority, Claude's is fastMode. Changes follow T3 Code's composer order and are verified in the projection. The CLI refuses a permission change during a running turn, which T3 would kill, and a provider switch on a started thread, which T3 rejects. - Every sent turn carries the thread's model selection, which is how T3 applies a model or effort change to a live session. - threads interrupt, approve, decline, and answer act on a running turn and on pending approvals and questions, verify the provider's response, and can wait for the turn that follows. - models list prints providers, models, and their options. - Pending requests include questions that outlive their turn, the decisions an approval offers, and whether a request blocks a turn. - Turn ownership no longer guesses from the provider: a live Codex thread folded a mid-turn message into the running turn. A message stays with the turn it was sent into unless a turn without its own prompt starts within five seconds after that turn ends, and waits allow that window. - Proposed plans appear at every read detail level. - README and skill updates.
Mira PR WalkthroughThis PR adds CLI controls for thread models, reasoning effort, provider options, permissions, execution modes, interruption, approvals, and question answers. It validates options against T3's provider catalog, sends model selection on every turn, improves turn matching, and exposes proposed plans at every read detail level. It also expands tests and usage documentation; prerequisite PR #7 must merge first. graph LR
cli["src/cli.ts"] --> controls["src/threadControls.ts"]
cli --> service["src/service.ts"]
controls --> catalog["src/catalog.ts"]
controls --> threadapi["src/threadApi.ts"]
service --> threadapi
threadapi --> transcript["src/transcript.ts"]
Confidence: 4/5 ◉◉◉◉○ Likely safe after prerequisite merge
Key files to review:
19 files reviewed · 2 comments (
|
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. |
![clark-review[bot]](https://avatars.githubusercontent.com/u/8057288?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mira Review Summary
src/transcript.ts:429 leaves historical ordinary questions selectable after their provider callbacks expire. At src/transcript.ts:205, multiple queued mid-turn messages can be assigned inconsistently, causing waits to return the wrong reply.
Key Issues
| Issue | Location | |
|---|---|---|
| 🔴 | Historical ordinary questions remain selectable after their provider callbacks expire. | src/transcript.ts:429 |
| 🔴 | Multiple queued mid-turn messages are assigned inconsistently, causing waits to return the wrong reply. | src/transcript.ts:205 |
| detail: text(payload.detail) ?? text(payload.requestKind) ?? text(activity.summary), | ||
| questions, | ||
| turnId, | ||
| responseMode, |
There was a problem hiding this comment.
Bug
Exclude ordinary questions whose turn has ended
Only approvals are filtered by inRunningTurn. An unresolved ordinary user-input.requested activity from a completed or interrupted turn now remains in pendingRequests, even though its provider callback is no longer available. answerThread consumes this list in selectRequest, so these historical questions can be selected for an invalid response or make a current question ambiguous. Apply the running-turn check to ordinary questions as well, while preserving responseMode === "message" questions across turns.
| responseMode, | |
| if (responseMode !== "message" && !inRunningTurn && flag !== true) return []; |
Prompt for AI Agents
In src/transcript.ts around line 429, update pendingRequests so unresolved ordinary user-input requests are excluded after their originating turn ends, using the same running-turn and explicit pending-flag logic as approvals. Preserve message-mode questions across turns. Add regression tests in src/transcript.test.ts for completed and interrupted turns with unresolved ordinary questions and for selecting a current question alongside historical activities.
Apply this code change:
if (responseMode !== "message" && !inRunningTurn && flag !== true) return [];
Not useful? Reply
@clark-review rejectto dismiss this suggestion.
There was a problem hiding this comment.
Fixed in e2bfd3a. Ordinary questions now count as pending only while their turn runs, unless T3's pending flag says otherwise; message-mode questions stay open across turns. Test: "drops ordinary questions whose turn ended and keeps the request kind as detail".
| Date.parse(next.startedAt) - Date.parse(running.endedAt) <= QUEUED_TURN_GRACE_MS && | ||
| !sorted.some( | ||
| (other) => | ||
| other !== message && other.role === "user" && other.createdAt > message.createdAt && other.createdAt <= next.startedAt, |
There was a problem hiding this comment.
Bug
Do not mistake another mid-turn message for a new turn's prompt
When two user messages arrive during the same running turn and are queued for the following turn, the later message satisfies this predicate for the earlier one. The earlier message is consequently assigned to the old turn, while the later message is assigned to the queued turn. waitForTurn uses that assignment to choose the reply, so waiting for the earlier message returns the wrong turn. Distinguish a prompt submitted after the running turn ended from additional messages submitted during it.
Prompt for AI Agents
In src/transcript.ts lines 201-206, revise groupTurns' queuedTurn prompt detection so later user messages submitted during running's lifetime do not count as next's own prompt. Use running.endedAt to distinguish independently submitted prompts after completion. Add a regression test with two mid-turn messages followed by a queued turn, and verify waitForTurn for the first message follows the queued turn.
Not useful? Reply
@clark-review rejectto dismiss this suggestion.
There was a problem hiding this comment.
Fixed in e2bfd3a. Only a user message sent after the running turn ended counts as the next turn's own prompt, so every message queued during the turn goes to the queued turn. Test: "gives every message queued during a turn to the turn that starts right after it".
![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: eeb041f002
ℹ️ 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".
| detail: text(payload.detail), | ||
| requestKind: text(payload.requestKind), |
There was a problem hiding this comment.
When an approval or question has no payload.detail, this changes the existing detail field from the request kind or activity summary to null. JSON consumers of threads inspect and threads wait that display or classify pendingRequests[].detail therefore lose data unless they are updated for the new requestKind field; retain the previous fallback while adding the new fields.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2bfd3a. detail falls back to the request kind and then the activity summary again, next to the new requestKind field.
| const responseMode = kind === "user-input" && payload.responseMode === "message" ? "message" : null; | ||
| const inRunningTurn = runningTurn !== null && turnId === runningTurn; | ||
| if (kind === "approval" && !inRunningTurn && flag !== true) return []; |
There was a problem hiding this comment.
When a non-message user-input request's turn completes without a user-input.resolved activity, T3 has already auto-closed the question, but this predicate filters historical approvals only and continues returning that question indefinitely. It then appears in inspection, can block threads settle, and may be selected by threads answer only for the provider to reject it as stale; non-message questions should require membership in the running turn unless the projection explicitly marks them pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2bfd3a, together with the matching Mira finding: ordinary questions from an ended turn are no longer pending, so inspect, settle, and answer ignore them.
| !sorted.some( | ||
| (other) => | ||
| other !== message && other.role === "user" && other.createdAt > message.createdAt && other.createdAt <= next.startedAt, |
There was a problem hiding this comment.
When two user messages arrive during the same running turn and the provider queues them, the later message makes this some check false for the earlier one, so the earlier message is assigned to the already-running turn rather than the follow-up turn. As a result, threads send --wait for that first message can return the previous turn's answer even though the provider handles it later; multiple queued messages must not be interpreted as evidence that only the last message belongs to the queued turn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2bfd3a, together with the matching Mira finding: messages queued during the same turn no longer count as each other's prompts.
| if (thinkingEffort !== undefined) { | ||
| // T3 provider drivers use different descriptor ids for the same user-facing control. | ||
| setProviderOption(selections, "reasoningEffort", thinkingEffort); | ||
| setProviderOption(selections, "effort", thinkingEffort); | ||
| setProviderOption(selections, "reasoning", thinkingEffort); |
There was a problem hiding this comment.
When server.getConfig is unavailable and the target thread uses OpenCode, threads set --thinking-effort falls back to this function, but it writes only reasoningEffort, effort, and reasoning. OpenCode reads the variant option, so the command reports a successful change while the next turn does not receive the requested effort; add variant to the unchecked alias set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2bfd3a. Without the catalog, effort changes on existing threads also set variant. Handovers keep their current three aliases.
| if (next.instanceId !== current.instanceId && thread.session != null) { | ||
| // T3 rejects moving a started conversation to another driver or to incompatible resume state. | ||
| const from = catalog ? findProvider(catalog, current.instanceId) : null; | ||
| const to = catalog ? findProvider(catalog, next.instanceId) : null; | ||
| const compatible = from && to && from.driver === to.driver && from.continuationKey === to.continuationKey; |
There was a problem hiding this comment.
When two instances use the same driver but both omit continuation.groupKey, both parsed keys are null, so this equality marks them compatible even though the catalog provides no evidence that they share resume state. Switching a started thread between such instances can leave the next turn unable to resume the conversation; compatibility should require a non-null matching continuation key rather than treating two missing keys as a match.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2bfd3a. A provider switch on a started thread now needs a matching, non-empty continuation key. Test: "refuses a provider switch when neither instance names its resume state".
Combine the wait loop's bounded polling and single full read from #7 with the queue grace window and blocking-request rules of this branch.
- Ordinary questions count as pending only while their turn runs; T3 closes them when the turn ends. Message-mode questions stay open. - Pending request detail falls back to the request kind or activity summary, as before, next to the new requestKind field. - Every message queued during a turn goes to the turn that starts right after it; only a prompt sent after the earlier turn ended counts as the next turn's own prompt. - Without the catalog, effort changes on existing threads also set OpenCode's variant option. - A provider switch needs a matching, non-empty continuation key.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2bfd3a077
ℹ️ 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".
| descriptor.values.find((value) => value.id === "priority" || value.id === "fast")?.id ?? | ||
| descriptor.values.find((value) => !value.isDefault && value.id !== "default")?.id ?? |
There was a problem hiding this comment.
When a model exposes serviceTier but offers neither priority nor fast, --speed fast silently selects the first non-default tier. Catalogs can contain unrelated tiers such as flex, so this can make a supposedly fast request slower instead of reporting MODEL_OPTION_UNSUPPORTED; only explicitly recognized fast-tier values should be selected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d44049a. --speed fast now picks only a tier named priority or fast and otherwise reports MODEL_OPTION_UNSUPPORTED. Test: "turns fast mode on only with a tier named for speed".
- Messages queued during a turn claim the turns that start back to back after it, one each, instead of all landing in the first queued turn. A message whose turn has not started yet stays pending. - --speed fast only picks a service tier named priority or fast; other tiers, such as flex, can be slower.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d44049aa6b
ℹ️ 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".
| if (next.instanceId !== current.instanceId && thread.session != null) { | ||
| // T3 rejects moving a started conversation to another driver or to incompatible resume state. |
There was a problem hiding this comment.
When a persisted thread has prior turns or messages but its provider session is absent, this condition skips the compatibility check and permits switching to another driver or incompatible continuation group. The code's own invariant says T3 cannot move a started conversation, so the subsequent turn can fail to resume even though threads set reported success; determine whether the conversation started from turn/history data rather than only thread.session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e5580af. A thread counts as started once it has a session, a turn, or any messages, so a provider switch is refused for history without a live session too.
| const tier = serviceTier?.type === "select" ? serviceTierValue(serviceTier, fast) : null; | ||
| if (fastMode?.type === "boolean") setOption(options, fastMode.id, fast); | ||
| else if (serviceTier && tier) setOption(options, serviceTier.id, tier); | ||
| else if (fast) { | ||
| throw invalid("MODEL_OPTION_UNSUPPORTED", `${model.slug} has no fast mode.`, { model: model.slug }); | ||
| } |
There was a problem hiding this comment.
When the catalog contains the current model but omits optionDescriptors, descriptors is null, so same-model options are retained. If the saved selection has fastMode: true or a fast serviceTier, requesting --speed standard reaches this branch without changing or rejecting anything because only the fast case throws; the command can therefore report that the settings are already applied while the thread remains in fast mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e5580af. When the catalog has no option descriptors for the model, speed and effort are set unchecked, the way handovers do, so --speed standard writes fastMode=false and serviceTier=default. Test: "sets speed and effort unchecked when T3 does not describe the model's options".
| const descriptors = model.options; | ||
| const carried = (current.options ?? []).filter((option) => { | ||
| if (descriptors === null) return sameModel; |
There was a problem hiding this comment.
When an older or alternate projection returns modelSelection.options as an object map, a representation the existing normalizeProviderOptions helper explicitly supports, this direct .filter call throws because the value is not an array. Any threads set model change made while the catalog is available can therefore fail with an internal error instead of preserving and updating those options; normalize the runtime value before filtering and comparing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e5580af. Saved options are normalized with the existing normalizeProviderOptions before they are filtered or compared. Test: "reads saved options stored as an object map".
| const restarted = await adapter.poll( | ||
| thread.id, | ||
| (candidate) => (!liveSession(candidate) || candidate.session?.runtimeMode === requested ? candidate : null), | ||
| adapter.controlTimeoutMs, |
There was a problem hiding this comment.
When T3 stops the old live session before starting its replacement, this predicate treats the transient stopped state as successful because liveSession is false. It also treats a failed restart that leaves a stopped session with lastError as success, so threads set can return before the requested permission is active—or report success after the restart failed—instead of waiting for a live session with the requested mode or raising THREAD_PERMISSION_NOT_APPLIED.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e5580af. Success now needs a live session that reports the new mode, and a transient stopped state keeps the wait going. A stopped or errored session with a new lastError fails at once with THREAD_PERMISSION_NOT_APPLIED. A session that stays stopped without a new error is reported as applied at the next start, with sessionRestart: false. Tests cover all three paths.
| if (interactionMode === "plan" && catalog) { | ||
| const provider = findProvider(catalog, (modelSelection ?? current)?.instanceId ?? ""); | ||
| if (provider && !provider.supportsPlanMode) { |
There was a problem hiding this comment.
When a thread is already in plan mode and its provider is changed without an explicit --mode, interactionMode is null, so this validation is skipped. An unstarted thread can therefore be moved to a provider such as OpenCode that does not support T3's plan mode while retaining thread.interactionMode === "plan"; the model update succeeds, but the next turn carries an unsupported mode and can be rejected. Validate the effective mode (interactionMode ?? thread.interactionMode) against the selected provider.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e5580af. When a thread in plan mode changes provider, the new provider must support plan mode. Test: "checks that a new provider supports the plan mode the thread keeps".
- A thread with history counts as started even without a session, so a provider switch is refused there too. - A provider change checks plan mode when the thread keeps it. - A model without option descriptors gets speed and effort set unchecked, so --speed standard turns fast mode off. - Saved options stored as an object map are read correctly. - A permission change succeeds only when a live session reports the new mode; a restart that stops the session with a new error fails at once, and a session that stays stopped counts as applied without a restart.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5580af1db
ℹ️ 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 responseMode = kind === "user-input" && payload.responseMode === "message" ? "message" : null; | ||
| const inRunningTurn = runningTurn !== null && turnId === runningTurn; | ||
| // T3 closes ordinary questions when their turn ends; message-mode questions stay open. | ||
| if (responseMode === null && !inRunningTurn && flag !== true) return []; |
There was a problem hiding this comment.
Fresh evidence after the earlier fix is the flag !== true exception: when a later request of the same kind sets the thread-wide pending flag, every unresolved historical request is admitted again, including ordinary questions and approvals whose turns have ended. This can make threads answer ambiguous, block settlement, or send a response to a stale request; non-message requests from ended turns should remain excluded because the flag does not identify which request is pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1011dd0. Approvals and ordinary questions count only while their turn runs, even when the thread-wide flag is set. A false flag still closes them all, and message-mode questions stay open. Test: "ignores the thread-wide pending flag for requests from an ended turn".
| ); | ||
| } | ||
|
|
||
| const sameModel = instanceId === current.instanceId && model.slug === current.model; |
There was a problem hiding this comment.
When the saved selection uses one of the catalog model's aliases and that model has no option descriptors, findModel resolves it successfully but this comparison treats it as a model switch. The fallback at line 221 then omits all saved options before applying the requested change, so a command such as threads set --thinking-effort high silently discards unrelated provider settings; recognize aliases as the same model before deciding whether to retain those options.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1011dd0. A selection that names the model by a catalog alias counts as the same model, so its saved options are kept. Test: "keeps saved options when the selection names the model by an alias".
| const settings = hasSettingsChange(options.settings) | ||
| ? await changeSettingsWithApi(api, adapter, thread, options.settings) | ||
| : null; |
There was a problem hiding this comment.
When threads send includes model, effort, speed, or interaction-mode settings while the target already has a running turn, this applies the metadata immediately and then submits the message without ensuring it gets a new turn. Providers may fold a mid-turn message into the existing turn rather than queueing it, so that prompt is handled by the already-running provider with the old model and modes even though the command reports the requested settings were applied before the message; reject this combination while a turn is running or wait until the message can start its own turn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1011dd0. threads send with new settings now refuses with THREAD_BUSY while a turn runs and dispatches nothing, because the message could join that turn. threads set alone still works mid-turn, since its changes apply at the next turn. Test: "refuses to send with new settings while a turn runs".
- Approvals and ordinary questions count as pending only while their turn runs, even when T3's thread-wide flag is set; the flag cannot say which request it means. - A saved selection that names the model by an alias counts as the same model, so its options are kept. - threads send with new settings refuses while a turn runs, because the message could join that turn with the old model and modes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1011dd0b0c
ℹ️ 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".
|
|
||
| function describeSelection(selection: ModelSelection | null | undefined): string { | ||
| if (!selection) return "unknown"; | ||
| const options = (selection.options ?? []).map((option) => `${option.id}=${String(option.value)}`).join(", "); |
There was a problem hiding this comment.
When an older or alternate projection returns modelSelection.options as an object map—a representation the new normalizeProviderOptions helper explicitly supports—this .map call throws. Because the description is constructed before writeSuccess, even --json threads inspect returns an error instead of the inspection envelope; normalize the runtime value before formatting it.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4553268. The model description normalizes saved options first. A built-CLI test runs threads inspect against a mock server whose thread stores options as an object map.
threads inspect normalizes saved options before formatting the model, so older projections no longer make it fail.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4553268118
ℹ️ 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".
| thread.id, | ||
| (candidate) => { | ||
| const session = candidate.session; | ||
| if (liveSession(candidate) && session?.runtimeMode === requested) return "restarted" as const; |
There was a problem hiding this comment.
When a permission restart fails after the session has adopted the requested runtimeMode, T3 can project session.status === "error" with a new lastError; however, liveSession() treats every status except stopped as live, so this branch returns "restarted" before the failure check below can run. Fresh evidence after the earlier restart fix is this ordering: an errored session with the requested mode is still accepted, causing threads set or threads send to report success even though no usable provider session restarted; exclude error (and other non-ready transitional states) from the success condition or evaluate the new error first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 126e5d3. A new error on a stopped or errored session is checked first, and success needs a ready, idle, or running session that reports the new mode. Test: "does not count an errored session with the new mode as restarted".
| const waited = await adapter.waitForTurn(threadId, { | ||
| timeoutMs: wait.timeoutMs, | ||
| ...(messageId === undefined ? {} : { messageId }), | ||
| }); |
There was a problem hiding this comment.
When threads approve --wait or threads answer --wait successfully resolves the request but the resumed turn exceeds the requested timeout, this uncaught THREAD_WAIT_TIMEOUT replaces the already-confirmed response result and contains no marker that the write succeeded. An automated caller can therefore interpret the whole command as failed and retry a response that T3 has already applied; handle the timeout like sendThreadMessage does by preserving an explicit responded: true/request identifier in the error details, or return the confirmed response with a timed-out wait status.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 126e5d3. If the wait after a confirmed approve or answer times out, the error is THREAD_WAIT_TIMEOUT with responded: true and the request id, and its message says not to respond again. The README and skill say the same. Test: "reports a response that T3 accepted when the wait after it times out".
| const settled = await adapter.poll( | ||
| threadId, | ||
| (candidate) => (turnRunning(candidate) ? null : { thread: candidate, failure: failureOf(candidate) }), | ||
| adapter.controlTimeoutMs, |
There was a problem hiding this comment.
When a message is already queued behind the running turn, interrupting that turn can let the queued turn start before the next poll. This predicate then continues waiting because the thread is still globally running, even though the requested turnId was successfully interrupted; a long replacement turn produces THREAD_INTERRUPT_NOT_VERIFIED, while a short one makes the command return details for the unrelated turn. Verify that the captured turn is no longer active or running rather than requiring the entire thread to become idle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 126e5d3. With a captured turn id, verification checks that this turn is no longer the running latest turn or the session's active turn, so a queued turn that starts next does not hold up the command. Test: "verifies the interrupted turn even when a queued turn starts next".
- A permission restart counts only when a ready, idle, or running session reports the new mode; a failed restart is checked first. - When the wait after approve or answer times out, the error carries responded: true and the request id, so callers do not respond twice. - Interrupt verification checks the interrupted turn itself, so a queued turn that starts right after it does not hold up the command.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 126e5d3593
ℹ️ 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 waited = await adapter | ||
| .waitForTurn(threadId, { timeoutMs: wait.timeoutMs, ...(messageId === undefined ? {} : { messageId }) }) |
There was a problem hiding this comment.
For ordinary approvals and questions, messageId is undefined, so waitForTurn follows whichever turn is latest. If another message was already queued behind the blocked turn, that queued turn can start while awaitResolution is polling; --wait then returns the unrelated queued reply or times out on it even though the turn containing the answered request has finished. Preserve the request's turnId and wait for that specific turn to end.
Useful? React with 👍 / 👎.
| // A message sent during a running turn may join that turn, which keeps its current model and modes. | ||
| if (turnRunning(thread)) { | ||
| throw new CliError( |
There was a problem hiding this comment.
When a caller explicitly repeats settings the thread already has—for example, threads send --model <current-model>—this rejects the send solely because a turn is running, even though planThreadSettings would produce no setting commands and an otherwise identical send without the flag is supported mid-turn. This breaks declarative callers that always pass their desired settings; compute the plan first and raise THREAD_BUSY only when it contains an actual change.
Useful? React with 👍 / 👎.
| /** The catalog is only needed to check model settings. Older T3 servers do not serve it. */ | ||
| async function catalogFor(api: T3Api, change: ThreadSettingsChange): Promise<ProviderCatalog | null> { | ||
| if (!hasModelChange(change) && change.interactionMode !== "plan") return null; | ||
| return await fetchCatalog(api).catch(() => null); |
There was a problem hiding this comment.
Any catalog failure—not only an older server lacking server.getConfig—is converted to null. If the WebSocket ticket, connection, or RPC fails transiently, a request such as threads set --provider typo --model typo therefore takes the unchecked fallback, persists the invalid selection, and can report success even though the next turn will fail. Only the explicit unsupported-RPC response should enable the compatibility fallback; operational and authentication failures should remain errors.
Useful? React with 👍 / 👎.
| const deadline = Date.now() + timeoutMs; | ||
| let last: T3Thread | null = null; | ||
| do { | ||
| const read = await this.read(threadId).catch(() => null); |
There was a problem hiding this comment.
When the detail endpoint stalls, this awaited read can consume its 30-second HTTP timeout and then another timeout in the snapshot fallback before the loop rechecks deadline. Consequently the 5-second settings verification and 15-second response verification can hang for roughly a minute, and even the 30-second interrupt/restart deadline is not enforced. Race each read against the remaining deadline, as the turn and dispatch wait loops already do.
Useful? React with 👍 / 👎.
| const current = thread.modelSelection ?? null; | ||
| let modelSelection: ModelSelection | null = null; | ||
| if (hasModelChange(change)) { | ||
| if (!current) { | ||
| throw new CliError("T3_INVALID_THREAD", `Thread ${thread.id} has no saved model selection.`, { | ||
| details: { threadId: thread.id }, | ||
| }); |
There was a problem hiding this comment.
A thread without a saved modelSelection is rejected even when the caller supplies both --provider and --model, which are sufficient to construct and catalog-validate a complete selection. Such threads are otherwise supported—the projection type makes the field optional and turn dispatch already tolerates its absence—so older threads cannot be repaired with threads set and must remain on an unknown model selection. Require the existing selection only when an omitted provider or model must be inherited.
Useful? React with 👍 / 👎.
1 participant
Stacked on #7; merge that first. This PR's diff contains only the thread controls.
Changes
threads set: changes an existing thread's model, reasoning effort, fast mode, other model options (--option id=value), permission, and plan or build mode.--dry-runchecks a change without applying it.threads sendtakes the same flags and applies them before its turn.server.getConfig, and mapped to each model's option ids. Codex fast mode isserviceTier=priority; Claude's isfastMode, on Opus models only.threads interrupt,threads approve|decline,threads answer: each verifies the provider's response and can--waitfor the turn that follows.answer --dismisscloses a question that outlived its turn.models list: prints providers and models with their options.Testing
pnpm checkpasses with 135 tests. Every command was exercised live against T3 0.0.45-nightly on a throwaway thread: model switch, approve, decline, interrupt, answering a question that outlived its turn, and permission changes on a live session.