PROD-66: two-step agent reply pass (flag-gated experiment)#236
PROD-66: two-step agent reply pass (flag-gated experiment)#236ayush-that wants to merge 22 commits into
Conversation
…ds in latestAssistantText
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional two-step agent mode that streams drafts as thinking, rewrites them through a tools-free reply pass, validates fallback output, preserves turn ordering, persists draft metadata, and records reply-pass tracing. ChangesTwo-step reply flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DraftAgent
participant AgentRunner
participant ReplyAgent
participant MessageStore
participant Langfuse
DraftAgent->>AgentRunner: stream draft as thinking deltas
AgentRunner->>ReplyAgent: rewrite draft without tools
ReplyAgent-->>AgentRunner: return reply or failure
AgentRunner->>MessageStore: persist reply and draft thinking
AgentRunner->>Langfuse: complete reply-pass trace
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ayush-that
changed the base branch from
ayush/prod-65-thinking-in-final-response
to
main
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/agent/src/agent-runner.ts (1)
3405-3483: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize final assistant persistence with prior side effects.
replyPassstarts immediately, and its directappendLogEntryat Line 3449 bypassessession.sideEffects; the laterqueueSideEffect(() => replyPass)only waits for an already-running promise. A fast rewrite can therefore persist the assistant row before queued user, tool, oragent_startentries. Queue the reply-pass work itself before it begins.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/agent/src/agent-runner.ts` around lines 3405 - 3483, The two-step reply pass starts before prior side effects complete, allowing its direct persistence via appendLogEntry in the replyPass closure to overtake queued entries. Refactor the reply-pass creation and scheduling so the entire async body is submitted through queueSideEffect before execution, and assign session.pendingReplyPass to that queued promise, preserving existing ordering and teardown behavior without starting replyPass eagerly.
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-07-10-two-step-agent-reply-pass.md (1)
488-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the checklist to reflect the implemented queue behavior.
The supplied implementation already awaits and clears
session.pendingReplyPassinapps/agent/src/agent-runner.ts:1217-1225, while this plan leaves the corresponding task steps unchecked. Mark completed steps or label this as a historical plan so contributors do not repeat already-landed work.As per coding guidelines, docs must stay aligned with the current implementation rather than old plans.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-10-two-step-agent-reply-pass.md` around lines 488 - 528, Update the Task 8 checklist in the plan to reflect the already-landed implementation: mark the queue-chaining, ordering test, implementation, teardown-safety, and test-validation steps complete where applicable, or clearly label the section as a historical plan. Ensure it does not instruct contributors to repeat the existing `session.pendingReplyPass` await/clear behavior in `run()` and the associated teardown handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/agent/src/agent-runner.ts`:
- Around line 2941-2969: The reply timeout in the two-step rewrite flow is not a
true deadline because agent construction can hang and completed output is
accepted after expiration. Update the logic around createConfiguredAgent,
prompting, and waitForIdle to race the entire pass against timeoutMs, resolve
with null when the deadline wins, and reject any result if timedOut is true;
ensure timer cleanup and agent abortion still occur during timeout handling.
- Around line 3275-3281: Do not include thinkingSignature when assigning
session.latestAssistantMeta for the two-step draft response in the agent runner.
Preserve the original signed reasoning payload separately, or omit the signature
from the draft metadata so buildResumptionMessages() cannot replay draft text as
signed reasoning; apply the same correction to the related assignment near the
second referenced location.
In `@apps/agent/src/agent-runner/message-utils.ts`:
- Around line 69-77: Update hasCodeFenceParity so it requires the rewrite’s
code-fence count to equal the draft’s count, rejecting both dropped and
unmatched added fences. Replace the current one-sided >= comparison with an
exact parity check while retaining the existing fence-counting logic.
- Around line 52-67: Update stripReasoningTags to return the stripped result
directly when paired reasoning tags are removed, including an empty string,
instead of falling back to text.trim(). Preserve the existing handling for
unclosed tags and whitespace normalization, and ensure extractAssistantText does
not publish reasoning-only content.
In `@apps/agent/test/two-step-reply.test.ts`:
- Around line 183-191: Update postAndIdle() to throw an error when the
ten-second deadline expires before the expected agent_end count reaches ordinal,
matching the failure behavior of post(). Keep the existing polling logic, then
add a deadline check after the loop with a diagnostic message identifying the
session and expected completion.
In `@docs/superpowers/plans/2026-07-10-two-step-agent-reply-pass.md`:
- Around line 299-303: Update acceptRewrite to extract URLs from the draft and
reject any rewrite that omits one, while retaining the existing emptiness and
code-fence checks. Add a regression test covering a rewrite that drops a draft
link and verify it is rejected.
- Around line 286-287: Update the provider-key guard in the reply flow to call
ensureProviderApiKey with replyConfig.model.provider, not the full replyConfig.
Since ensureProviderApiKey throws rather than returning a boolean, remove the
conditional return and either let the error propagate or catch it and return
null if this path must remain nullable.
- Around line 289-315: Ensure the timeout covers createConfiguredAgent as well
as prompt and waitForIdle: make agent construction abortable using the existing
AbortController signal, or race the entire reply-pass sequence against the
timeout. Update the sequence around createConfiguredAgent, agent.prompt, and
agent.waitForIdle so setup cannot exceed reply_timeout_ms while preserving
cleanup and fallback behavior.
---
Outside diff comments:
In `@apps/agent/src/agent-runner.ts`:
- Around line 3405-3483: The two-step reply pass starts before prior side
effects complete, allowing its direct persistence via appendLogEntry in the
replyPass closure to overtake queued entries. Refactor the reply-pass creation
and scheduling so the entire async body is submitted through queueSideEffect
before execution, and assign session.pendingReplyPass to that queued promise,
preserving existing ordering and teardown behavior without starting replyPass
eagerly.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-07-10-two-step-agent-reply-pass.md`:
- Around line 488-528: Update the Task 8 checklist in the plan to reflect the
already-landed implementation: mark the queue-chaining, ordering test,
implementation, teardown-safety, and test-validation steps complete where
applicable, or clearly label the section as a historical plan. Ensure it does
not instruct contributors to repeat the existing `session.pendingReplyPass`
await/clear behavior in `run()` and the associated teardown handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 458b8f69-4b0b-420f-b84d-52690069c8fc
📒 Files selected for processing (8)
apps/agent/src/agent-runner.tsapps/agent/src/agent-runner/message-utils.tsapps/agent/src/agent-runner/types.tsapps/agent/src/core/security.tsapps/agent/src/core/types.tsapps/agent/test/message-utils.test.tsapps/agent/test/two-step-reply.test.tsdocs/superpowers/plans/2026-07-10-two-step-agent-reply-pass.md
…ring agent construction
ayush-that
force-pushed
the
shydev/prod-66-experiment-two-step-agent-architecture
branch
from
e904c4f to
da14f28
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/agent/src/agent-runner.ts`:
- Around line 3444-3460: The two-step turn can lose its assistant history entry
when outbound text is empty because the draft row is suppressed and the append
is gated by finalText. Update the two-step persistence logic around
appendLogEntry to append the assistant row whenever a draft exists, using
finalText when available and an appropriate empty content value otherwise, while
preserving the draft thinking and metadata.
- Around line 3405-3409: In the async reply pass, use the snapshotted const
lastUserMessageText captured before queuing instead of allowing
generateStyledReply to read session.lastUserMessageText live. Thread this
snapshot through the reply-pass call and update generateStyledReply’s input or
prompt construction accordingly, preserving the existing two-step rewrite
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cefc9f06-e3ed-4726-a72b-1b9dfcd6aa50
📒 Files selected for processing (4)
apps/agent/src/agent-runner.tsapps/agent/src/agent-runner/message-utils.tsapps/agent/src/agent-runner/types.tsapps/agent/test/two-step-reply.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/agent/src/agent-runner/types.ts
- apps/agent/src/agent-runner/message-utils.ts
- apps/agent/test/two-step-reply.test.ts
…ture, and tighten the fidelity guard
…hen outbound text is stripped
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/agent/src/agent-runner/message-utils.ts`:
- Around line 91-98: Update preservesLinks to extract and normalize URLs from
both draft and rewrite using the same punctuation handling, then require each
complete draft URL to exactly match a candidate URL in the rewrite rather than
using rewrite.includes(url). Preserve the existing case-insensitive HTTP(S)
matching and trailing punctuation normalization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df8df907-7ca3-4ceb-bbfe-45b4be1fbe27
📒 Files selected for processing (3)
apps/agent/src/agent-runner.tsapps/agent/src/agent-runner/message-utils.tsapps/agent/test/two-step-reply.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/agent/test/two-step-reply.test.ts
- apps/agent/src/agent-runner.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/agent/test/two-step-reply.test.ts`:
- Around line 657-661: Extend the assertions in the two-step reply test to
validate each call’s configured model identifier in addition to its provider.
Use the expected base model identifier for draftCall and the configured
reply_model identifier for replyCall, preserving the existing call-count and
provider assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ffd8ceb-f109-4186-b72a-a072822660ed
📒 Files selected for processing (1)
apps/agent/test/two-step-reply.test.ts
|
Reviewed (29 tests pass in a worktree; flag-off is genuinely byte-identical — schema-optional,
Minor: add |
Flag-gated experiment. Adds an optional second "reply" LLM pass that restyles the tool-loop draft in the twin's voice before the final message is published.
Default off (
experiments.two_step) is byte-identical to today; any reply-pass failure (timeout, error, empty, missing key) falls back to the draft, so the answer is never lost.Based on #233 for the reasoning-strip reuse.
Summary by CodeRabbit