Repository navigation
fix(core): replay assistant reasoning on the generate fallback - #148
LongLongBigInt wants to merge 2 commits into
Conversation
The generate transport dropped assistant thinking blocks when building
request history, so the model could not see its own earlier reasoning and
re-decided hidden state on follow-up turns. The official Command Code CLI
replays thinking as `{type:"reasoning",text}` in `toWireMessages`, and
pi's native providers keep prior reasoning in history too.
Add the thinking branch to `messagesToCC()`, cover it with unit and
stream-request tests, and keep a live-capture fixture pair (session
history + generate request body) for cumulative replay. List the behavior
change in the changelog.
pierreraby
left a comment
There was a problem hiding this comment.
Thanks for the focused fix and the regression coverage. Reviewed at 86ae3f0.
I independently checked command-code@1.79.2 from the npm tarball (including its published integrity): toWireMessages does replay assistant thinking as { type: "reasoning", text }, so the converter change matches that wire behavior. The cumulative fixtures cover both previous text-only assistant turns, and the updated mixed-content test preserves reasoning/text/tool-call order.
Local verification is green: full npm test (375 tests, no failures/skips, plus real Pi and OMP against mock APIs), typecheck, formatting and diff-check. The mid-stream abort test also passed here. As a regression control, the four updated/new converter assertions and the new request-serialization assertion fail against pristine main and pass at this head. CI, security checks and the memory benchmark are green on the same head; a fresh second review found no blocking issue.
One optional wording nit: in the changelog, I'd keep the parity claim specific to the official Command Code CLI rather than “pi's native providers” in general, since history handling differs by provider. This does not block approval.
Our execution was local/mock-only; I reviewed your sanitized live captures but did not independently rerun the paid model experiments. Keeping this first iteration flag-free is consistent with the agreed scope; we can revisit an opt-out or model-specific behavior if a concrete regression appears.
|
Addressed the optional wording nit in |
pierreraby
left a comment
There was a problem hiding this comment.
Thanks for addressing the wording nit. Re-approved at d24cadf.
I verified the delta from the previously reviewed 86ae3f0: one documentation-only commit, replacing one line in CHANGELOG.md, with no code or fixture changes. The wording now scopes parity to the Command Code CLI's /alpha/generate wire, consistent with the toWireMessages implementation independently checked in command-code@1.79.2.
Both workflows pass on this exact head:
The current code/security checks are also green. The only skipped check is the memory workflow's PR-comment update.
This follow-up review covers the bounded docs-only delta on top of the previously reviewed implementation. I did not rerun the local suite or paid model probes for this documentation change.
Fixes #146.
Problem
On the generate fallback (accounts that get
403 upgrade_requiredon the Provider API), assistant thinking blocks were dropped when building/alpha/generaterequest history. The model cannot see its own earlier reasoning and silently re-decides hidden state on follow-up turns. Visible text and tool calls are preserved, so the bug presents as random forgetfulness rather than an obvious error.Cause
messagesToCC()had no branch forcontent.type === "thinking"(removed in #40 / 0.5.0). #40's parity claim does not hold: the official CLI'stoWireMessagesreplays thinking as{type:"reasoning",text}on 1.32.2, 1.72.4 and the current 1.79.2, and pi's native providers keep prior reasoning in history too.Changes
fix(core): replay assistant thinking as{type:"reasoning",text}inmessagesToCC(), matching the official CLI and pi's native providers. No configuration switch in this iteration.tests/test-pure-functions.ts, including a live-capture fixture pair (tests/fixtures/issue-146-{session,generate-turn3}.json) from a 3-turn generate session ondeepseek/deepseek-v4.1-flashwithreadenabled. Both assistant turns are text-only, and the test asserts thatmessagesToCC()on the captured history deep-equals the captured turn-3 request body, so cumulative replay is covered rather than a single prior turn.tests/test-stream.tsthat the outgoing generate body carries thereasoningpart, plus a changelog entry.Verification
Two-turn guessing game on
deepseek/deepseek-v4.1-flash, generate transport withreadenabled, pristinemainvs this branch; request bodies captured with an undici diagnostics hook. The secret number exists only in thinking, never in visible text.params.messages[1].contentin the turn-2 requestmain[{type:"text", ...}]— no reasoning[{type:"reasoning", text: …563 chars…}, {type:"text", ...}]Raw sessions and sanitized wire captures, plus the same protocol on
inclusionai/ling-3.1-flash:free,z-ai/glm-5.3-flashandQwen/Qwen3.8-Flash: https://gist.github.com/LongLongBigInt/e7c29746d903e8e9e982dc0232ec8b89Why full-history replay
Checks
npm run typecheck,npm run format:checktests/*.tssuite green except the pre-existingtests/test-abort.tsmid-stream abort assertion, which also fails on pristinemainin this environment.tests/test-pi-local.mjsPASS;tests/test-omp-compat.mjsskipped locally (omp not installed).