Add durable local GitHub waitpoints for Codex#4267
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| }); | ||
| return; | ||
| } | ||
| if (threadStatus.value.latestTurnId !== waitpoint.originatingTurnId) { |
There was a problem hiding this comment.
🟠 High github/GitHubWaitpointWorker.ts:204
The pre-claim latestTurnId check on line 204 expires a waitpoint that was already successfully delivered. If a crash occurs after threads.resume dispatches the continuation but before repository.markDelivered records the delivery, the thread has already advanced past originatingTurnId. On recovery, this check treats that expected advancement as unrelated activity and expires the row instead of allowing the deterministic command ID to be deduplicated by the orchestration layer. The crash-recovery path is thereby defeated — a delivered continuation is recorded as expired. Consider deferring or relaxing this check until after the delivery lease is claimed, or removing it from the pre-claim path so that crash-recovery relies on command-receipt deduplication to complete bookkeeping.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/github/GitHubWaitpointWorker.ts around line 204:
The pre-claim `latestTurnId` check on line 204 expires a waitpoint that was already successfully delivered. If a crash occurs after `threads.resume` dispatches the continuation but before `repository.markDelivered` records the delivery, the thread has already advanced past `originatingTurnId`. On recovery, this check treats that expected advancement as unrelated activity and expires the row instead of allowing the deterministic command ID to be deduplicated by the orchestration layer. The crash-recovery path is thereby defeated — a delivered continuation is recorded as expired. Consider deferring or relaxing this check until after the delivery lease is claimed, or removing it from the pre-claim path so that crash-recovery relies on command-receipt deduplication to complete bookkeeping.
| }), | ||
| ); | ||
|
|
||
| yield* client.handleServerRequest("item/tool/call", dynamicToolWaitRegistry.handle); |
There was a problem hiding this comment.
🟠 High Layers/CodexSessionRuntime.ts:1163
When cancelTurn or cancelAll fires before a t3.await_github tool call is handled, the registry still invokes handleT3CodexDynamicToolCall, which ignores the cancellation signal and calls registerGitHubWaitpoint unconditionally. This persists a durable waitpoint for a turn that was already interrupted or for a session that was closed, causing the thread to resume later even though the turn was explicitly canceled.
The await_github branch in handleT3CodexDynamicToolCall never checks the cancelled effect — it proceeds to register the waitpoint regardless of whether the turn or session was canceled. Consider checking the cancellation state before registering the GitHub waitpoint, so late or concurrent calls after an interrupt or close do not create durable waitpoints.
Also found in 1 other location(s)
apps/server/src/provider/Layers/CodexDynamicTools.ts:155
The
await_githubbranch never observes thecancelledeffect. Consequently, whenmakeT3CodexDynamicToolWaitRegistry.handlerejects registration because the turn was already interrupted or the session is closed, it passes an already-completed cancellation effect but this branch still callsregisterGitHubWaitpointand reports success. A late tool request can therefore create a durable waitpoint for a cancelled/closed turn, unlike the ordinary wait path.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CodexSessionRuntime.ts around line 1163:
When `cancelTurn` or `cancelAll` fires before a `t3.await_github` tool call is handled, the registry still invokes `handleT3CodexDynamicToolCall`, which ignores the cancellation signal and calls `registerGitHubWaitpoint` unconditionally. This persists a durable waitpoint for a turn that was already interrupted or for a session that was closed, causing the thread to resume later even though the turn was explicitly canceled.
The `await_github` branch in `handleT3CodexDynamicToolCall` never checks the `cancelled` effect — it proceeds to register the waitpoint regardless of whether the turn or session was canceled. Consider checking the cancellation state before registering the GitHub waitpoint, so late or concurrent calls after an interrupt or close do not create durable waitpoints.
Also found in 1 other location(s):
- apps/server/src/provider/Layers/CodexDynamicTools.ts:155 -- The `await_github` branch never observes the `cancelled` effect. Consequently, when `makeT3CodexDynamicToolWaitRegistry.handle` rejects registration because the turn was already interrupted or the session is closed, it passes an already-completed cancellation effect but this branch still calls `registerGitHubWaitpoint` and reports success. A late tool request can therefore create a durable waitpoint for a cancelled/closed turn, unlike the ordinary wait path.
| }); | ||
| } | ||
|
|
||
| export const make = Effect.gen(function* () { |
There was a problem hiding this comment.
🟡 Medium persistence/GitHubWaitpoints.ts:171
markWaitpointDelivered, rescheduleWaitpoint, and expireWaitpoint match rows by id and state alone, without checking the current lease. When worker A's lease expires and worker B reclaims the row, A can still overwrite B's claim — e.g., A reschedules the waitpoint after B has dispatched, then B's markDelivered matches zero rows (since the state is no longer delivering) but the call still returns success, leaving a dispatched waitpoint stuck in pending for further retries. The WHERE clauses in these updates need to include the caller's lease token/value and the calls need to detect zero-row updates so stale workers cannot mutate the current owner's claim.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/persistence/GitHubWaitpoints.ts around line 171:
`markWaitpointDelivered`, `rescheduleWaitpoint`, and `expireWaitpoint` match rows by `id` and state alone, without checking the current lease. When worker A's lease expires and worker B reclaims the row, A can still overwrite B's claim — e.g., A reschedules the waitpoint after B has dispatched, then B's `markDelivered` matches zero rows (since the state is no longer `delivering`) but the call still returns success, leaving a dispatched waitpoint stuck in `pending` for further retries. The `WHERE` clauses in these updates need to include the caller's lease token/value and the calls need to detect zero-row updates so stale workers cannot mutate the current owner's claim.
There was a problem hiding this comment.
Effect service conventions: two new modules import a local service module in a way that erases its module namespace. Both diverge from the repo-wide convention (and the sibling imports in the same files) of importing a service module as a namespace and accessing Module.Tag / Module.layer through it.
Posted via Macroscope — Effect Service Conventions
| import * as Layer from "effect/Layer"; | ||
| import * as Schema from "effect/Schema"; | ||
|
|
||
| import { GitHubCli, type GitHubCliError } from "../sourceControl/GitHubCli.ts"; |
There was a problem hiding this comment.
GitHubCli is consumed here as a service tag (yield* GitHubCli, line 201) and its error type is referenced (GitHubCliError, line 78). At a service boundary the module should be imported as a namespace — every other consumer in the repo uses import * as GitHubCli and GitHubCli.GitHubCli. Suggested change:
- line 13:
import * as GitHubCli from "../sourceControl/GitHubCli.ts"; - line 78:
export type GitHubPullRequestProbeError = GitHubCli.GitHubCliError | GitHubPullRequestProbeDecodeError; - line 201:
const gitHubCli = yield* GitHubCli.GitHubCli;
Posted via Macroscope — Effect Service Conventions
| import { | ||
| GitHubWaitpointWorker, | ||
| layer as workerLayer, |
There was a problem hiding this comment.
layer as workerLayer aliases the export and erases the module namespace. The three sibling imports above use import * as ...; import the worker module the same way and reference its members through the namespace:
-import {
- GitHubWaitpointWorker,
- layer as workerLayer,
- threadGatewayLayer,
-} from "./GitHubWaitpointWorker.ts";
+import * as GitHubWaitpointWorker from "./GitHubWaitpointWorker.ts";Then update the call sites below to GitHubWaitpointWorker.layer (line 25), GitHubWaitpointWorker.threadGatewayLayer (line 28), and GitHubWaitpointWorker.GitHubWaitpointWorker (line 33).
Posted via Macroscope — Effect Service Conventions
1 participant
Summary
t3.await_githubto Codex for checks-settled, new-review-activity, and PR-closed waitsghCLI, without T3 Connect or a public endpointCloses #4266.
Delivery semantics
The tool registers the waitpoint and returns immediately, telling the agent to finish the current turn. The worker polls due rows every 30 seconds while T3 is running. Pending rows and expired delivery leases are recovered after restart. If T3 crashes after dispatch but before marking delivery, the existing orchestration command-receipt store deduplicates the retry.
The UI needs no new transition for this milestone: delivery is a normal
thread.turn.start, so connected clients show the thread running again through existing projection events.Verification
orchestration/decider.ts)gh pr viewadapter smoke against Add a host-managed wait tool for Codex sessions #4262Intentional limitations
waitingstate yetStack note
This is a stacked draft based on #4262 (
t3.wait). Until #4262 lands, GitHub shows its two foundation commits in this PR as well. The durable GitHub implementation begins at35dba04f.Note
Add durable GitHub waitpoints and T3 dynamic tools for Codex sessions
t3.waitandt3.await_github) to Codex session runtimes, allowing threads to pause for a timed delay or until a GitHub PR condition is met.GitHubWaitpointRepositorybacked by a new SQLite table (github_waitpoints, migration 034) with idempotent registration, exclusive claim leases, rescheduling, and expiration.GitHubWaitpointWorkerthat polls due waitpoints every 5 seconds, probes current PR state via the GitHub CLI, evaluates conditions (checks_settled,new_review_activity,pull_request_closed), and resumes threads viaGitHubWaitpointThreadGatewaywhen conditions are satisfied.registerGitHubWaitpointthroughCodexAdapterandCodexDriver.deliveringstate until lease expiry if the worker crashes mid-claim.📊 Macroscope summarized 35dba04. 15 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.