fix(devin): recover oversized prompt history after rejection#6014
Conversation
roboomp
left a comment
There was a problem hiding this comment.
review:p2 — focused implementation; the six targeted Devin provider tests pass, but the recovery heuristic needs explicit maintainer sign-off.
Blocking: total protobuf size is not equivalent to compactable history, so the opaque intermittent trailer can misroute large system-prompt/tool-schema requests into ineffective context maintenance.
Coverage: no partial-output case protects the new zero-output guard. Maintainer call: accept this heuristic tradeoff or require size-causal evidence/a shrinkable-history measurement.
Thanks for the tightly scoped patch and regression coverage, @riverpilot.
314a919 to
73b9760
Compare
|
Addressed both review findings in
Re-verified: 8 focused Devin tests, 3,009 AI tests (337 skipped), and |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ce706662
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 450856e95f
ℹ️ 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".
… context overflow sizing
8acfae8 to
cc3b201
Compare
|
Rebased onto latest
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc3b2011b5
ℹ️ 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".
Rebase onto main tightened AssistantMessage content to block arrays; update the large tool-execution recovery fixture accordingly.
|
Follow-up after rebase: CI
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cc82b1f03
ℹ️ 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".
| // A trailing developer message can accompany the current user | ||
| // prompt. Earlier user-role records may instead be flushed | ||
| // execution history and must remain eligible for compaction. | ||
| if (lastRole === "developer") { |
There was a problem hiding this comment.
Exclude all trailing user active-turn messages
The fresh remaining active-tail case is when the same prompt ends with another user-role companion: this backward scan is skipped unless the final role is developer, but #promptWithMessage can append same-turn messages after the prompt (e.g. file mentions after messages.push(message) in packages/coding-agent/src/session/agent-session.ts:8876-8892), and image file mentions convert to role: "user" in packages/coding-agent/src/session/messages.ts:845-872. For a huge current user prompt followed by a small user companion, the huge live prompt is counted in shrinkablePrompts, so the opaque Devin rejection is mislabeled as ContextOverflow and sent through compaction even though that active tail cannot be shrunk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Investigated against the existing regression suite. The history contains indistinguishable consecutive user-role records: flushed prior execution history must remain compactable, while same-turn user companions must be excluded. Without an active-turn boundary in the Message/Context contract, excluding every trailing user record breaks the prior-history regression and misclassifies large flushed execution output. The current implementation preserves that safety invariant; this finding needs an explicit active-turn boundary before it can be implemented without regressing the earlier P1/P2 fixes.
Summary
invalid_argument: an internal error occurredtrailers as context overflow when the serialized, compactablechatMessagePromptshistory is already at least 512 KiB.Related #4218
Screenshots
No visual change.
Testing
bun test packages/ai/test/devin-large-request-recovery.test.ts— 5 passedbun test packages/ai/test/devin-frame-cap.test.ts packages/ai/test/devin-streaming-args.test.ts packages/ai/test/devin-large-request-recovery.test.ts— 8 passedbun test packages/coding-agent/test/agent-session-auto-compaction-progress-guard.test.ts— 28 passedbun --cwd=packages/ai test— 3,009 passed, 337 skipped, 0 failedbun run check:tsRisk
Notes
AI Review Report
Focused review verified that
AIError.finalizepreserves the structuralContextOverflowflag alongside text-derivedTransient, AgentSession prioritizes overflow maintenance over retry, the revised threshold measures only compactable history, and partial-output/fixed-envelope cases remain non-overflow. No substantive findings remain.Security Audit