fix: surface messages steered into a running turn with a reminder - #3474
fix: surface messages steered into a running turn with a reminder#3474liukx0205 wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: d582a60 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![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: 9d6051fc69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.states.contributeState(promptResolutionKey); | ||
| this.reminder.register('steer', () => { | ||
| if (!this.steerReminderArmed) return undefined; | ||
| this.steerReminderArmed = false; |
There was a problem hiding this comment.
When the steered message pushes the context over the automatic-compaction threshold, the reminder hook invokes this provider before the full-compaction hook, so this line clears the flag and appends the reminder; compaction then drops that injection and triggers the reminder service's same-step reconciliation, but the cleared flag prevents re-emission. The resulting model request contains the steered user message without the reminder this change is meant to guarantee, so keep the delivery pending until its injection survives compaction or use the supported one-off delivery path.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L80-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed reachable via the modern compaction shape: buildContextCompactionShape keeps real user input verbatim (the steered message included) while dropping every injection-kind message, so the same-step rearm pass could indeed rebuild the request with the steered message but without the reminder, and the consumed flag blocked re-emission.
Fixed in ee9d165 by making the provider restate rather than consume: it emits while the flag is armed, and the flag is now cleared by an onDidFinishStep hook (plus the existing settle() guard) instead of at emission time. The same-step post-compaction reconciliation re-emits the reminder after the splice, and the step-end clear keeps later steps from duplicating it — the service invokes the provider once per injection pass, and a second pass only happens after a splice that already dropped the first emission.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee9d1656c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| expect(await provider(injectionContext())).toBe(STEER_REMINDER); | ||
| expect(await provider(injectionContext())).toBe(STEER_REMINDER); |
There was a problem hiding this comment.
These assertions invoke the captured provider directly, so they only verify the private armed flag and never prove the observable contract: that onWillBeginStep appends the steer injection after the message and re-emits it if same-step compaction removes it. A regression in reminder registration, ordering, or compaction reconciliation would leave all four new tests green; wire the real reminder service, drive the step hook, and assert the resulting context messages instead.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L80-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair point — the provider-level assertions only proved the flag. Added an integration path in 02e83d1: harness({ integrationReminder: true }) swaps the notify-only stub for createReminderHarness, so the new test drives the real onWillBeginStep hook chain and asserts observable context messages: the wrapped steer-variant injection lands after the steered user message, a real applyCompaction (modern shape) drops the injection while keeping the steered message verbatim, the next hook run re-emits it, and after onDidFinishStep a further run appends nothing.
The kept provider-level tests still pin the arming rules (materialize-time arm, tool-inject exemption, settle clear). The one piece exercised elsewhere rather than here is the injector's intra-step second pass after a mid-chain compaction splice — that's covered by the reminder service's own suite ("re-reconciles within the same step when compaction lands inside the step hook chain"), which this test composes with.
…ment the armed-provider pattern
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdd11d4cba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.states.contributeState(promptLaunchingKey); | ||
| this.states.contributeState(promptAdmissionKey); | ||
| this.states.contributeState(promptResolutionKey); | ||
| this.reminder.register('steer', () => (this.steerReminderArmed ? STEER_REMINDER : undefined)); |
There was a problem hiding this comment.
When a dependency is re-provided while the Agent scope remains alive, the cascade retires this AgentPromptService, but the class is not disposable and this registration remains in AgentReminderService's entry set. If that happens while steerReminderArmed is true, the replacement steer-reminder hook removes the old instance's only clearing path, so its retained provider emits a steer reminder on every later step; even an unarmed reload retains the old instance indefinitely. Make the service disposable and register this handle (and its hooks) with its lifecycle.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — the registration outlived the service. Fixed in d582a60: AgentPromptService now extends Disposable and registers the reminder provider handle, the steer-reminder step hook, and the pre-existing prompt-service-delivery tool hook with its lifecycle, so a cascade retirement removes the provider from the reminder entry set and the hooks from their slots instead of leaving an armed provider emitting on every later step.
| expect(steerTextIndex()).toBeGreaterThanOrEqual(0); | ||
| expect(steerTextIndex()).toBeLessThan(context.get().length - 1); | ||
|
|
||
| context.applyCompaction({ summary: 'summary', contextSummary: 'summary', compactedCount: context.get().length, tokensBefore: 0 }); |
There was a problem hiding this comment.
Fresh evidence after the earlier review is that the new integration path still uses createReminderHarness, not AgentReminderService, and line 308 performs compaction only after the first onWillBeginStep run has returned; it then starts a separate hook run. The helper also receives no event bus, so this passes merely because the armed provider emits on every invocation and never exercises production's post-next() same-step reconciliation—the branch that prevents the reported reminder loss could be deleted while this test remains green. Wire the real reminder service and trigger compaction later in the same hook chain.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L82-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The premise that the branch could be deleted with all tests green doesn't hold at the repo level: production's post-next() same-step reconciliation is pinned by the reminder service's own suite ("re-reconciles within the same step when compaction lands inside the step hook chain" in test/features/reminder/reminder.test.ts), driven through the real AgentReminderService — deleting that branch turns that suite red. The promptService test deliberately pins the other half of the composition, the provider's armed-restate contract (emits while armed across passes, stops after onDidFinishStep), against the shared harness. Duplicating the actor-backed real-service rig inside promptService tests to re-prove a branch already pinned where it lives would add test surface without adding failure coverage, so leaving this as-is.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Related Issue
None — internal behavior fix.
Problem
A message steered into a running turn is appended to the model context as a bare user message, sandwiched between the previous step's tool results and any pending system-reminder injections (which land after it and push it away from the tail). Nothing in the system prompt or the context marks it as a mid-turn instruction, and the steer boundary is the only step boundary where no reminder provider fires at all. In practice the model often keeps executing its original plan and overlooks the steered message.
What changed
AgentPromptServicenow registers asteerreminder provider backed by an armed flag: the flag is armed in the steer materialize callback (user-initiatedsteer()only — tool-deliveredinject()keeps its current semantics) and the provider emits the reminder for as long as the flag stays armed. The flag is cleared when the step finishes (onDidFinishStep) and onsettle(), not at emission time, so the same-step post-compaction reconciliation re-emits the reminder after a splice drops it — compaction keeps real user input verbatim but drops every injection, which would otherwise leave the steered message in the request without its reminder. A step that re-runs its injection pass before finishing (an in-turn retry) re-emits by design, re-anchoring the reminder.turn.steerwire payload are unchanged.steergets the lowest injection priority inREMINDER_VARIANT_PRIORITY, so the reminder is appended after all other injections and sits at the tail of the context, immediately anchoring the steered message above it.This fits the existing reminder architecture (same channel as the interruption reminder) rather than wrapping the steered message inline, which would pollute the wire payload and the transcript rendering. Tests cover both levels: provider-level cases pin the arming rules, and an integration case (
createReminderHarness) drives the real step-hook chain and asserts the observable context — injection after the steered message, re-emission after a realapplyCompactiondrops it, and no further append once the step finishes.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.