Repository navigation
Persist deferred steer delivery across agent instances and restarts - #162
Conversation
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. |
![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: 8d00fc70fd
ℹ️ 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".
| ): Promise<boolean> { | ||
| const filePath = this.sessionPath(sessionKey); | ||
| const entry = redactJsonLine({ type: 'message', message, ts: Date.now() }); | ||
| const entry = redactJsonLine({ type: 'message', message, deliveryId, ts: Date.now() }); |
There was a problem hiding this comment.
When a caller supplies a delivery ID matching a secret-redaction pattern, redactJsonLine changes the persisted ID while duplicate detection still compares against the original value. For example, calling appendMessageOnce twice with ID ghp_abcdefghijklmnopqrst1234567890 returns true twice and stores two messages because the receipt becomes [REDACTED]. The same transformation affects receipts imported through replaceMessages, undermining crash-safe handoff for those IDs. Preserve a stable, collision-resistant receipt identity (for example, a consistently computed digest) independently of message redaction.
Useful? React with 👍 / 👎.
| const deferredEntries = this.inboxFor(sessionKey) | ||
| .promotableSteers() | ||
| .filter((entry) => entry.deferred); |
There was a problem hiding this comment.
After a run leaves a deferred steer, deleting the session and starting a new Agent with the same key resurrects that instruction: this path loads the surviving inbox and appends its contents to the newly empty history. The built-in deleteSession implementations remove history and receipts but not the inbox, and the CLI's sessions delete handler in src/cli/command-dispatcher.ts calls only that method. I reproduced a successful deletion followed by the old instruction appearing before the new session's prompt. Session deletion needs to invalidate the associated pending deliveries, or delivery must distinguish a recreated session from its deleted predecessor.
Useful? React with 👍 / 👎.
| const deliveryId = this.steeringDeliveries.get(msg as InternalMessage); | ||
| if (deliveryId) { | ||
| await this.appendInboxDelivery(store, key, deliveryId, msg as unknown as LLMMessage); |
There was a problem hiding this comment.
For a steer admitted through admit() without a target run, an interruption after the canonical append but before acknowledgement leaves the entry pending and non-deferred. On restart, history already contains its message, but live steering polls it again. Although appendInboxDelivery correctly returns false for the existing receipt, this callback discards that result, and agent-loop.ts unconditionally pushes the message into currentMessages. I reproduced the provider receiving the instruction twice while persisted history contained it once. Reconcile already-adopted entries before returning steering messages so receipt retries cannot replay instructions to the model.
Useful? React with 👍 / 👎.
Unconsumed steering now stays in a durable session inbox until canonical adoption succeeds. Optional SessionStore append-once receipts prevent duplicate delivery across independent Agents and process restarts, including compaction and the append-before-ack failure window; custom stores retain their documented weaker guarantees. The generic loop and host authorization boundaries are unchanged.
JSONL appends preserve existing unterminated tails, duplicate receipts re-establish persistence barriers before acknowledgement, and structural delivery IDs survive message redaction. Doctor probes also release unfinished response bodies so CLI processes can exit after reporting.
Validation at exact topic
27eca3776bc84c54753bea2ee0498b12b8971161: completenpm run verifypassed (414 spec files, CLI smoke), and all three offline examples passed using Node 22.16. Independent Node 22/24 fault-injection and lifecycle reviews passed; no tests, deadlines or required gates were relaxed. Superseded failures remain recorded rather than counted as passing.Normal PR CI run 40130943367 passed all 13 jobs, including both Windows versions and the required Test summary. It checks merge
4eadb4c200442d232d606a3b88d04a31f57d58e5, whose tree includes later main changes absent from this bounded topic; those results cannot qualify the exact topic's complete platform behavior or a Windows release. Studio integration, its complete pin/lock, and final host verification remain separate. Merge must preserve topic ancestry for the normal Studio updater.