-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix repeated steering and reliable stop #3903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Quicksaver
wants to merge
26
commits into
pingdotgg:main
Choose a base branch
from
Quicksaver:fix/repeated-steering-and-stop
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
fc384cd
Fix steering acknowledgement and turn interruption
Quicksaver 298f4b5
Preserve root turn interruption projection
Quicksaver cf6b0c8
Harden steering acknowledgement and turn interruption
Quicksaver 3414977
Fix active Codex turn selection without timestamps
Quicksaver e2e05c8
Merge upstream Android support and preserve steering fixes
Quicksaver 06e38dc
Align Expo updates with fork project
Quicksaver ef1f35e
Merge upstream/main into fix/repeated-steering-and-stop
Quicksaver dd45496
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver 6c08e81
Remove unrelated Expo fork customization
Quicksaver 726dd73
Document repeated steering and reliable stop behavior
Quicksaver 0ac7499
Correct repeated steering branch documentation
Quicksaver 195fbca
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver 425371a
Document upstream merge impact on repeated steering
Quicksaver 8a889a6
Isolate Codex interrupt resolution coverage
Quicksaver d1dca69
Keep steering acknowledgements and fallback turns current
Quicksaver 0bf687f
Harden steering and interrupt review coverage
Quicksaver cb6190e
Refresh branch assessment details
Quicksaver 4277c28
Refresh branch guidance for steering and stop fixes
Quicksaver e13518a
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver 7f9ab76
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver e47c771
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver c1a0db3
Fix pnpm virtual store and Effect Vitest resolution
Quicksaver 5384094
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver d193aba
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver 4d21b97
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver 31b31bc
Merge remote-tracking branch 'upstream/main' into fix/repeated-steeri…
Quicksaver File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # Repeated Steering And Reliable Stop | ||
|
|
||
| Running conversations allow users to send any number of steering prompts and stop the active agent at any time, including after one or more steers. | ||
|
|
||
| Expected behavior: | ||
|
|
||
| - A steering send remains locally busy only until the server projects that exact user-message id. That acknowledgement stays valid when another client later projects a newer user message, while an unrelated projected user message alone must not acknowledge the dispatch. Steering the existing running turn must not wait for a new turn or session transition before re-enabling the composer. | ||
| - Root interruption commands retain the projected active turn id in orchestration events, but the provider command reactor intentionally lets the root Codex adapter resolve the authoritative active provider turn. Subagent interruption continues to target the selected child turn explicitly and must not fall back to a root turn. | ||
| - Codex root interruption reads the live provider thread with `includeTurns: true`, selects the most recently started `inProgress` turn, and bounds that lookup with a timeout. When either candidate lacks `startedAt`, provider response order is authoritative and the later entry wins. A failed lookup is logged and may fall back to the session turn read after that lookup finishes; a successful lookup with no active turn returns without reviving a stale cached id. | ||
|
|
||
| Conflict guidance: | ||
|
|
||
| - In `apps/web/src/components/ChatView.logic.ts`, `apps/web/src/components/ChatView.logic.test.ts`, and `apps/web/src/components/ChatView.tsx`, preserve exact-id acknowledgement and consecutive-steer coverage. Keep the draft-hero dock transition and early in-flight guard, and call `beginLocalDispatch` only after `newMessageId()` provides the exact expected id. | ||
| - Keep live-turn lookup, ordering, timeout, and fallback coverage in `apps/server/src/provider/Layers/CodexInterruptResolution.test.ts`, separate from the runtime model/effort instruction coverage in `apps/server/src/provider/Layers/CodexSessionRuntime.test.ts`. `apps/server/src/provider/Layers/CodexSessionRuntime.ts` must retain both developer-instruction behavior and branch-owned interrupt resolution. | ||
|
|
||
| Primary files: | ||
|
|
||
| - `apps/web/src/components/ChatView.tsx` | ||
| - `apps/web/src/components/ChatView.logic.ts` | ||
| - `apps/server/src/provider/Layers/CodexSessionRuntime.ts` | ||
|
|
||
| Regression coverage lives in `apps/web/src/components/ChatView.logic.test.ts` and `apps/server/src/provider/Layers/CodexInterruptResolution.test.ts`. Keep coverage for consecutive in-turn steers, exact-message acknowledgement, timestamp-based live-turn selection, lookup timeout/failure fallback, and successful empty reads that suppress stale interrupts. | ||
|
|
||
| Use the existing `hasServerAcknowledgedLocalDispatch` helper for client dispatch correlation. Defer an explicit server receipt keyed by message id unless projected ids stop being authoritative. | ||
|
|
||
| ## Development Ports | ||
|
|
||
| - Web: `5738` | ||
| - Server/WebSocket: `13778` |
207 changes: 207 additions & 0 deletions
207
apps/server/src/provider/Layers/CodexInterruptResolution.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,207 @@ | ||
| import * as NodeAssert from "node:assert/strict"; | ||
|
|
||
| import * as Deferred from "effect/Deferred"; | ||
| import * as Effect from "effect/Effect"; | ||
| import * as Fiber from "effect/Fiber"; | ||
| import * as TestClock from "effect/testing/TestClock"; | ||
| import { describe, it } from "@effect/vitest"; | ||
| import { TurnId } from "@t3tools/contracts"; | ||
| import type * as CodexRpc from "effect-codex-app-server/rpc"; | ||
| import type * as EffectCodexSchema from "effect-codex-app-server/schema"; | ||
|
|
||
| import { | ||
| findActiveCodexTurnId, | ||
| resolveCodexInterruptTurnId, | ||
| shouldReplaceActiveCodexTurnCandidate, | ||
| } from "./CodexSessionRuntime.ts"; | ||
|
|
||
| function makeThreadReadResponse( | ||
| turns: EffectCodexSchema.V2ThreadReadResponse["thread"]["turns"], | ||
| ): EffectCodexSchema.V2ThreadReadResponse { | ||
| return { | ||
| thread: { | ||
| cliVersion: "0.0.0-test", | ||
| createdAt: 1, | ||
| cwd: "/tmp/project", | ||
| ephemeral: false, | ||
| id: "provider-thread-1", | ||
| modelProvider: "openai", | ||
| preview: "test thread", | ||
| sessionId: "session-1", | ||
| source: "appServer", | ||
| status: { type: "active", activeFlags: [] }, | ||
| turns, | ||
| updatedAt: 2, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| describe("findActiveCodexTurnId", () => { | ||
| it("selects the most recently started in-progress turn", () => { | ||
| const snapshot = makeThreadReadResponse([ | ||
| { id: "turn-active-new", status: "inProgress", startedAt: 30, items: [] }, | ||
| { id: "turn-completed", status: "completed", startedAt: 20, items: [] }, | ||
| { id: "turn-active-old", status: "inProgress", startedAt: 10, items: [] }, | ||
| ]); | ||
|
|
||
| NodeAssert.equal(findActiveCodexTurnId(snapshot), "turn-active-new"); | ||
| }); | ||
|
|
||
| it("selects a later in-progress turn without a start timestamp", () => { | ||
| const snapshot = makeThreadReadResponse([ | ||
| { id: "turn-active-old", status: "inProgress", startedAt: 10, items: [] }, | ||
| { id: "turn-active-new", status: "inProgress", items: [] }, | ||
| ]); | ||
|
|
||
| NodeAssert.equal(findActiveCodexTurnId(snapshot), "turn-active-new"); | ||
| }); | ||
|
|
||
| it("selects a later timestamped turn after one without a timestamp", () => { | ||
| const snapshot = makeThreadReadResponse([ | ||
| { id: "turn-active-old", status: "inProgress", items: [] }, | ||
| { id: "turn-active-new", status: "inProgress", startedAt: 10, items: [] }, | ||
| ]); | ||
|
|
||
| NodeAssert.equal(findActiveCodexTurnId(snapshot), "turn-active-new"); | ||
| }); | ||
|
|
||
| it("returns undefined when no turn is active", () => { | ||
| const response = makeThreadReadResponse([]); | ||
| NodeAssert.equal(findActiveCodexTurnId(response), undefined); | ||
| }); | ||
| }); | ||
|
|
||
| describe("resolveCodexInterruptTurnId", () => { | ||
| it.effect("requests turns when resolving an interrupt without a projected turn id", () => { | ||
| let requestedParams: CodexRpc.ClientRequestParamsByMethod["thread/read"] | undefined; | ||
| let readThreadCallCount = 0; | ||
|
|
||
| return Effect.gen(function* () { | ||
| const turnId = yield* resolveCodexInterruptTurnId({ | ||
| providerThreadId: "provider-thread-1", | ||
| requestedTurnId: undefined, | ||
| readSessionActiveTurnId: Effect.succeed(undefined), | ||
| readThread: (params) => { | ||
| readThreadCallCount += 1; | ||
| requestedParams = params; | ||
| return Effect.succeed( | ||
| makeThreadReadResponse([ | ||
| { id: "turn-active", status: "inProgress", startedAt: 10, items: [] }, | ||
| ]), | ||
| ); | ||
| }, | ||
| }); | ||
|
|
||
| NodeAssert.deepStrictEqual(requestedParams, { | ||
| threadId: "provider-thread-1", | ||
| includeTurns: true, | ||
| }); | ||
| NodeAssert.equal(readThreadCallCount, 1); | ||
| NodeAssert.equal(turnId, "turn-active"); | ||
| }); | ||
| }); | ||
|
|
||
| it.effect("does not revive a stale projected turn after a successful empty read", () => | ||
| Effect.gen(function* () { | ||
| const turnId = yield* resolveCodexInterruptTurnId({ | ||
| providerThreadId: "provider-thread-1", | ||
| requestedTurnId: undefined, | ||
| readSessionActiveTurnId: Effect.succeed(TurnId.make("turn-stale")), | ||
| readThread: () => Effect.succeed(makeThreadReadResponse([])), | ||
| }); | ||
|
|
||
| NodeAssert.equal(turnId, undefined); | ||
| }), | ||
| ); | ||
|
|
||
| it.effect("falls back to the projected turn when the live lookup fails", () => | ||
| Effect.gen(function* () { | ||
| const projectedTurnId = TurnId.make("turn-projected"); | ||
| const turnId = yield* resolveCodexInterruptTurnId({ | ||
| providerThreadId: "provider-thread-1", | ||
| requestedTurnId: undefined, | ||
| readSessionActiveTurnId: Effect.succeed(projectedTurnId), | ||
| readThread: () => Effect.fail("lookup failed"), | ||
| }); | ||
|
|
||
| NodeAssert.equal(turnId, projectedTurnId); | ||
| }), | ||
| ); | ||
|
|
||
| it.effect("bounds the live lookup and falls back to the projected turn on timeout", () => | ||
| Effect.gen(function* () { | ||
| const projectedTurnId = TurnId.make("turn-projected"); | ||
| const lookupStarted = yield* Deferred.make<void>(); | ||
| const resolution = yield* resolveCodexInterruptTurnId({ | ||
| providerThreadId: "provider-thread-1", | ||
| requestedTurnId: undefined, | ||
| readSessionActiveTurnId: Effect.succeed(projectedTurnId), | ||
| readThread: () => | ||
| Effect.gen(function* () { | ||
| yield* Deferred.succeed(lookupStarted, undefined); | ||
| return yield* Effect.never; | ||
| }), | ||
| }).pipe(Effect.forkScoped); | ||
|
|
||
| yield* Deferred.await(lookupStarted); | ||
| yield* TestClock.adjust("2 seconds"); | ||
| NodeAssert.equal(yield* Fiber.join(resolution), projectedTurnId); | ||
| }), | ||
| ); | ||
|
|
||
| it.effect("reads the projected fallback after a live lookup times out", () => | ||
| Effect.gen(function* () { | ||
| let projectedTurnId = TurnId.make("turn-old"); | ||
| const lookupStarted = yield* Deferred.make<void>(); | ||
| const resolution = yield* resolveCodexInterruptTurnId({ | ||
| providerThreadId: "provider-thread-1", | ||
| requestedTurnId: undefined, | ||
| readSessionActiveTurnId: Effect.sync(() => projectedTurnId), | ||
| readThread: () => | ||
| Effect.gen(function* () { | ||
| yield* Deferred.succeed(lookupStarted, undefined); | ||
| return yield* Effect.never; | ||
| }), | ||
| }).pipe(Effect.forkScoped); | ||
|
|
||
| yield* Deferred.await(lookupStarted); | ||
| // Mutate after the live lookup starts to verify that the fallback is | ||
| // evaluated lazily after the timeout instead of captured up front. | ||
| projectedTurnId = TurnId.make("turn-current"); | ||
| yield* TestClock.adjust("2 seconds"); | ||
| NodeAssert.equal(yield* Fiber.join(resolution), projectedTurnId); | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| describe("shouldReplaceActiveCodexTurnCandidate", () => { | ||
| it("selects the first candidate", () => { | ||
| NodeAssert.equal(shouldReplaceActiveCodexTurnCandidate({ startedAt: 10 }, undefined), true); | ||
| }); | ||
|
|
||
| it("orders timestamped turns by start time and lets a later equal entry win", () => { | ||
| NodeAssert.equal( | ||
| shouldReplaceActiveCodexTurnCandidate({ startedAt: 20 }, { startedAt: 10 }), | ||
| true, | ||
| ); | ||
| NodeAssert.equal( | ||
| shouldReplaceActiveCodexTurnCandidate({ startedAt: 10 }, { startedAt: 20 }), | ||
| false, | ||
| ); | ||
| NodeAssert.equal( | ||
| shouldReplaceActiveCodexTurnCandidate({ startedAt: 10 }, { startedAt: 10 }), | ||
| true, | ||
| ); | ||
| }); | ||
|
|
||
| it("lets the later provider entry win when either timestamp is absent", () => { | ||
| for (const [candidate, selected] of [ | ||
| [{}, { startedAt: 10 }], | ||
| [{ startedAt: null }, { startedAt: 10 }], | ||
| [{ startedAt: 10 }, {}], | ||
| [{ startedAt: 10 }, { startedAt: null }], | ||
| ] as const) { | ||
| NodeAssert.equal(shouldReplaceActiveCodexTurnCandidate(candidate, selected), true); | ||
| } | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.