Repository navigation
fix: support a host-owned idle steering fallback - #593
EugeneTheDev merged 6 commits into
Conversation
(cherry picked from commit 84bfbe8)
Resolve the conflict in performSteeringRequest by keeping the host-owned idle fallback branch and adopting the new startNewTurnFromSteering signature from main (ACP v2 rework): the idle steer now forwards its clientUserMessageId like the injection path does.
|
Merged latest main ( Verification on Windows: typecheck passes; the 29 targeted steering/plan-approval tests pass. The full suite fails 70 tests in 10 files, and a run of the same 10 files on the unmodified @EugeneTheDev could you take a look at the conflict resolution, since it touches the steering path your v2 work changed? @fontanierh heads-up that #441's patch in this PR has been rebased over the v2 rework. |
EugeneTheDev
left a comment
EugeneTheDev
left a comment
There was a problem hiding this comment.
A couple of small notes
| throw RequestError.invalidParams(); | ||
| } | ||
| const meta = params["_meta"]; | ||
| if (meta !== undefined && !isRecord(meta)) { |
There was a problem hiding this comment.
Problem: _meta: null is now rejected with invalidParams, so a client that does not use the new option can no longer steer.
How it happens:
- ACP allows
_metato benull(_meta?: {[key: string]: unknown} | nullin the SDK types). - A client sends
_session/steeringwith{"sessionId": "...", "prompt": [...], "_meta": null}. - On
mainthe parser ignores_meta, so this works. HereisRecord(null)is false and the call throws "steering _meta must be an object". The new testrejects malformed steering metadataasserts this fornull.
Impact: this breaks "Calls without the option behave as before" for clients that serialize an empty _meta as null. The steer fails instead of injecting. It only affects clients that send null, but the PR introduces it.
Fix: treat null like a missing _meta: if (meta !== undefined && meta !== null && !isRecord(meta)). The meta?.["steering"] line already handles null. Move null from the rejected list to the accepted one in steer-events.test.ts, e.g. add it to the reports injected ... (metadata: %j) list. I applied this temporarily and steer-events.test.ts passed (19 tests), with typecheck clean.
There was a problem hiding this comment.
Fixed: _meta: null is now treated like an absent _meta, and the accepted path is covered by adding null to the reports injected metadata list.
| })).rejects.toThrow(RequestError); | ||
| }); | ||
|
|
||
| it('rejects an unsupported idle behavior', async () => { |
There was a problem hiding this comment.
rejects an unsupported idle behavior only differs from the rejects malformed steering metadata ... test below by its input. Add {steering: {idleBehavior: "startDetachedTurn"}} to that it.each list and delete this test. That saves about 9 lines, and the unsupported value then also checks that no turn is started or steered.
There was a problem hiding this comment.
Done: folded {steering: {idleBehavior: "startDetachedTurn"}} into the malformed-metadata it.each list (which also asserts no turn starts) and dropped the standalone test.
The ACP type allows `_meta: null`, so treat it like an absent `_meta` instead of rejecting the steer. Fold the unsupported-idleBehavior case into the malformed-metadata it.each list, which also asserts that no turn is started, and drop its standalone test.
leehom0123
left a comment
leehom0123
left a comment
There was a problem hiding this comment.
Both notes addressed in b2a3c59 — null _meta is now accepted like an absent one, and the standalone idleBehavior test folded into the malformed-metadata list. Typecheck and the steering suites pass (29 tests). Ready for re-review.
EugeneTheDev
left a comment
EugeneTheDev
left a comment
There was a problem hiding this comment.
Looks good, please check docs comment
| - API key via `CODEX_API_KEY` or `OPENAI_API_KEY`. | ||
| - A custom OpenAI-compatible gateway, when the client opts in to the gateway auth capability. | ||
|
|
||
| ## Host-owned steering fallback |
There was a problem hiding this comment.
This section is too detailed for the README. Please move it to a new docs/steering.md and drop it from the README.
- Narrow features already have their own files in
docs/(subagent-sessions.md,session-compaction.md), so this follows the existing layout. _session/steeringhas no doc of its own yet. The new file can describe the whole method, not just this option: the_meta.steering.supportedcapability, the request, every outcome (injected,startedNewTurn,failed,promptRequired), andidleBehavior. The note thatsupporteddoes not show whether this option is available fits there too.- In
docs/air-extensions.md, link thesteeringrow of the "JetBrains shared keys" table tosteering.md. Don't put the contract itself in that file.
There was a problem hiding this comment.
Thanks for the pointer — moving it out makes the README consistent with how the other narrow features are laid out.
Done: the section is out of the README and into a new docs/steering.md. The file covers the whole method, not just this option — the _meta.steering.supported capability, the request shape, every outcome (injected, startedNewTurn, failed, promptRequired), and the idleBehavior opt-in. The note that supported alone doesn’t tell clients whether the option is available went there too. The README now carries a single Features bullet linking to the doc; no other section was left behind. The steering row in the "JetBrains shared keys" table of docs/air-extensions.md links to steering.md, and the contract itself stays out of that file.
Docs-only change, so no test or typecheck impact. Ready for another look when you have a moment.
README held a 28-line section for one opt-in. The whole method now has its own doc covering the capability, the request, every outcome, and idleBehavior; the README keeps a single Features bullet and the AIR shared-keys table links to the doc.
Upstream v2.4.0 (agentclientprotocol#593) adds a host-owned idle steering fallback: parseSessionSteerParams validates `_meta` and `_meta.steering`, keeps only `{steering: {idleBehavior: "promptRequired"}}`, and SessionSteerRequest types `_meta` as that shape. No upstream PR absorbs this patch. The downstream patch forwards the steering request's `_meta`, so a turn the steer starts carries `_meta.codexHooks.reviewToken` into prompt() and continues the hook review. Forward the whole `_meta` after upstream's validation instead of upstream's reduced copy, and type it as a record that also has upstream's `steering` shape. `idleBehavior: "promptRequired"` therefore still answers `promptRequired` without starting a turn, and any other steer still starts a reviewed turn. The hook review docs mention the fallback. Every other line of the patch is unchanged. The existing reviewed-steering test fails with upstream's reduced `_meta`; a new test (idle fallback next to reviewed metadata, then a reviewed steer that starts a turn) fails with upstream's reduced `_meta` and when only `codexHooks` is forwarded. Adapted from c0e41b9 (original 960f779).
If a Codex turn finishes just before
_session/steeringreaches it, the adapter starts another turn and returnsstartedNewTurn. The client never receives that new prompt's final response. If the original prompt is still waiting for plan approval, steering waits too, which can block clients that process steering and permission replies serially.This adds the opt-in requested in #440:
{"_meta":{"steering":{"idleBehavior":"promptRequired"}}}Live turns still receive the input through native steering. When no turn can accept it, the adapter returns
promptRequiredwithreason: "noRunningTurn", leaving the input unconsumed and starting no new turn. The client can answer any pending approval, wait for the original prompt to finish, then send the input through a normalsession/prompt. Calls without the option behave as before.This builds on @fontanierh's patch in #441, cherry-picked with the original author and source reference preserved. I added regression tests for pending plan approval, malformed metadata tests, and usage documentation.
Fixes #440.
Testing
4583cb3, in history, attachment, and path assertions.The real Codex tests used temporary local runners, adapted for the removed
authRequired()call in the repository's test script. Cancellation dispatch has unit coverage; the real test used the reject-plan path.For clients:
promptRequiredis a steering result, not completion of the original prompt or its approval request. The existing_meta.steering.supportedflag also does not distinguish support for this option, so clients need to check the adapter version.