Skip to content

fix(core): replay assistant reasoning on the generate fallback - #148

Open
LongLongBigInt wants to merge 2 commits into
patlux:mainfrom
LongLongBigInt:fix/generate-reasoning-replay
Open

LongLongBigInt wants to merge 2 commits into
patlux:mainfrom
LongLongBigInt:fix/generate-reasoning-replay

Conversation

@LongLongBigInt

Copy link
Copy Markdown

Fixes #146.

Problem

On the generate fallback (accounts that get 403 upgrade_required on the Provider API), assistant thinking blocks were dropped when building /alpha/generate request 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 for content.type === "thinking" (removed in #40 / 0.5.0). #40's parity claim does not hold: the official CLI's toWireMessages replays 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} in messagesToCC(), matching the official CLI and pi's native providers. No configuration switch in this iteration.
  • Unit coverage in 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 on deepseek/deepseek-v4.1-flash with read enabled. Both assistant turns are text-only, and the test asserts that messagesToCC() on the captured history deep-equals the captured turn-3 request body, so cumulative replay is covered rather than a single prior turn.
  • A stream-request assertion in tests/test-stream.ts that the outgoing generate body carries the reasoning part, plus a changelog entry.

Verification

Two-turn guessing game on deepseek/deepseek-v4.1-flash, generate transport with read enabled, pristine main vs this branch; request bodies captured with an undici diagnostics hook. The secret number exists only in thinking, never in visible text.

turn 1 (hidden pick) turn 2 (guess 66) params.messages[1].content in the turn-2 request
main 73 re-picks 42, answers "Lower!" [{type:"text", ...}] — no reasoning
this PR 73 "The number is 73. 66 is lower than 73" → "Higher!" [{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-flash and Qwen/Qwen3.8-Flash: https://gist.github.com/LongLongBigInt/e7c29746d903e8e9e982dc0232ec8b89

Why full-history replay

  • Consistency: pi's built-in providers keep prior thinking in history, and this plugin's Provider API path delegates history conversion to pi-ai; the generate fallback is the only path that loses it.
  • Accuracy/efficiency: state that lives only in thinking (plans, committed decisions, constraints) survives, so the model does not re-derive the same chain every turn.
  • Scoping: "tool-call turns only" would still break text-only turns in tool-enabled sessions, and "most-recent-assistant only" would drop state committed several turns earlier.

Checks

  • npm run typecheck, npm run format:check
  • Full tests/*.ts suite green except the pre-existing tests/test-abort.ts mid-stream abort assertion, which also fails on pristine main in this environment.
  • tests/test-pi-local.mjs PASS; tests/test-omp-compat.mjs skipped locally (omp not installed).

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
pierreraby previously approved these changes Oct 10, 2026

@pierreraby pierreraby left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@LongLongBigInt

Copy link
Copy Markdown
Author

Addressed the optional wording nit in d24cadf (docs-only, one line in CHANGELOG.md): the parity claim now anchors to the Command Code CLI's /alpha/generate wire instead of "pi's native providers". No code/fixture changes; the new head just needs a re-approval to run CI (the push auto-dismissed the previous one).

@pierreraby pierreraby left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generate fallback drops prior assistant reasoning from follow-up requests

2 participants