feat(executor): carry prior reasoning into system instructions

[warelik]

Summary

• Adds translator.carry-over-thinking-in-system config flag (default false).
• When enabled and translating to OpenAI Chat Completions on a non-compat model, prior assistant reasoning/thinking blocks are extracted and rewritten as a labeled system instruction (Prior assistant reasoning (unverified context)).
• Canonical targets (isCompat / reasoning_content) remain untouched.
• Boundaries: keeps the most recent 3 blocks, caps each at 4000 runes, and marks older/truncated blocks.

Test plan

• go test ./... green (8563 passed, Plus).
• New unit tests in internal/runtime/executor/helps/carry_over_test.go cover carry-over, merge, drop, bounds, compat guard, and default-off behavior.
• Stock branch pushed for patch reference; stock issue to follow.

Files changed

• internal/config/config_types.go
• internal/config/config.go
• internal/runtime/executor/helps/carry_over.go (new)
• internal/runtime/executor/helps/carry_over_test.go (new)
• internal/runtime/executor/helps/codex_multi_agent_v2.go

[warelik]

Review of the thinking carry-over feature at head c75a7d6. This is a well-scoped, well-tested change: it builds cleanly, gofmt and go vet pass, the new unit tests pass, and the executor, config, and openai-claude translator suites are all green. The flag defaults to off and the canonical compat path is correctly left untouched, which matches the stated goal.

Requesting changes, mainly for one behavioral inconsistency plus a few edge cases:

1. codex_multi_agent_v2.go - the new Claude-to-OpenAI branch calls ConvertClaudeRequestToOpenAIWithCompat directly and bypasses sdktranslator.TranslateRequest. That registry wrapper is what runs plugin NormalizeRequest hooks and the summary-config round trip. Summary intent is re-derived downstream in ApplyRequestThinking, but plugin request normalizers are not re-applied, so with the flag on and a plugin installed, Claude-to-OpenAI requests silently skip plugin normalization. The sibling else branch still routes through TranslateRequestWithCodexMultiAgentV2 (and thus the registry), so the two branches are inconsistent. See inline.

2. Because that branch reuses the compat converter, signed thinking blocks - which the stock non-compat path keeps as canonical reasoning_content - are also moved into the system text. If the intent is only to rescue unsigned thinking, consider whether signed blocks should remain as reasoning_content.

3. carry_over.go - merging into an existing system message drops non-object content-array parts (inline).

4. carry_over.go - the drop pass also removes assistant messages that were already empty and carried no reasoning (inline).

5. The new translator.carry-over-thinking-in-system flag is not documented in config.example.yaml; please add an entry.

Verdict: changes requested. The core mechanism is sound and safe behind the default-off flag; addressing item 1 (or documenting the bypass as intentional) plus the smaller items would get this to a mergeable state.

[warelik]

All findings addressed in the latest commits on ao/airouters-18-thinking-carry-over:

1. Registry bypass fixed. Claude→OpenAI now extracts unsigned thinking into the source system field before calling TranslateRequestWithCodexMultiAgentV2, so sdktranslator.TranslateRequest (and plugin NormalizeRequest hooks) still run.
2. Signed thinking preserved. Unsigned Claude thinking is carried into the system message; signed thinking with a compatible GPT signature is left in place and maps to reasoning_content as before.
3. String content preserved. mergeCarryOverIntoSystemMessage and injectClaudeCarryOverSystem now wrap plain string array elements as {"type":"text","text":"..."} parts instead of dropping them.
4. Doc comment corrected. CarryOverThinkingToSystem now documents that empty assistant messages are dropped, including pre-existing empty ones, which is intentional because OpenAI rejects empty assistant turns.

go test ./... green on both Plus and stock branches.

[warelik]

Re-review of updated head 583f604. The redesign addresses all four findings from the previous round (review 4997951757):

1. Plugin-hook bypass - resolved. The Claude branch now extracts unsigned thinking into Claude's top-level system field via carryOverClaudeSource, then routes through TranslateRequestWithCodexMultiAgentV2 / the registry, so plugin NormalizeRequest hooks and summary-config logic run exactly as on every other path.

2. Signed thinking moved - resolved. carryOverClaudeSource extracts only unsigned (non-GPT-compatible) thinking and leaves GPT-signed blocks in place, so the stock registry converter maps them to canonical reasoning_content. The new CarryOverKeepsSignedReasoningContent test pins this well.

3. String-array data loss - resolved. mergeCarryOverIntoSystemMessage (and the new injectClaudeCarryOverSystem) now wrap bare string content parts into text parts instead of dropping them. Covered by PreservesStringArrayElements.

4. Doc comment - the CarryOverThinkingToSystem comment now matches the drop behavior.

Verification at 583f604: go build ./cmd/server clean; gofmt and go vet clean; go test across runtime/executor/..., config, translator/openai/claude, signature, and thinking all green, including the four new carry-over tests.

Approving. One non-blocking doc nit inline at carry_over.go:184 (the comment says hooks still see a Claude-shaped payload, but the registry runs NormalizeRequest after native translation, so the hook sees the translated payload; the functional claim that hooks now run is correct). Optional to tidy in a follow-up.

[warelik]

AO review: approved (posted as COMMENT; GitHub rejects APPROVE on ones own PR).

Independent verification at head 583f604, in addition to the converged review cycle already on this PR (round 1 findings all resolved in the redesign):

• The gate is correctly scoped: !isCompat && cfg != nil && cfg.Translator.CarryOverThinkingInSystem && to == FormatOpenAI. Default-off is a pure no-op (short-circuits before any processing), and the nil-cfg caller in codex_executor_request.go is guarded.
• Flag-off behavior is unchanged, canonical compat targets are untouched, and GPT-compatible signed thinking stays in place to map to reasoning_content via the registry — only unsigned/incompatible thinking is rescued into the labeled system block. No thinking/signature degradation, consistent with the Plus-first mission.
• Registry and plugin NormalizeRequest hooks run as before (carry-over operates on the Claude source before TranslateRequestWithCodexMultiAgentV2).
• Dropping empty assistant messages cannot break tool-call pairing: any assistant with a non-empty tool_calls array is kept; only genuinely empty turns (which OpenAI rejects anyway) are removed.
• Bounds are correct: most recent 3 blocks kept, 4000-rune per-block cap, omission and truncation markers present. Helpers (thinking.GetThinkingText, sigcompat.CompatibleSignatureForProvider, translatorcommon.SetRawArrayItems/JoinRawArray) all exist and match usage. CI build passes.

Two non-blocking notes inline (config example doc still missing from round 1; one unreachable branch). Neither blocks merge.

[warelik]

AO review: approved (posted as COMMENT; GitHub rejects APPROVE on ones own PR).

Re-review of head a2d8af7 (delta from previously approved 583f604 is comment-only):

• The only changes are the two doc comments in carry_over.go and codex_multi_agent_v2.go, fixing the NormalizeRequest-ordering nit from the earlier cycle. Verified the new wording against sdk/translator/registry.go: when a native route exists (Claude→OpenAI does), the registry translates first and then runs plugin NormalizeRequest hooks on the final provider payload — the new comments state exactly that.
• No code changes, so the full verification from the previous review carries over: gate scope, default-off no-op, signed-thinking preservation, empty-assistant drop safety, bounds, and helper usage all unchanged.
• CI green on this head (build passes).

The two earlier non-blocking notes remain open and are fine to address later or leave: (1) translator.carry-over-thinking-in-system still absent from config.example.yaml; (2) the unreachable content: [] branch at carry_over.go:254. Neither blocks merge.

[warelik]

Fixed both 23:23 self-review nits:

• carry-over-thinking-in-system is now documented in config.example.yaml with the default OFF, what it does, and a P2 trade-off note.
• carryOverClaudeSource drop guard simplified: the unreachable len(keptParts) == 0 branch is removed and JoinRawArray is used directly.

[warelik]

AO review: approved (posted as COMMENT; GitHub rejects APPROVE on ones own PR).

Re-review of head c968ddf. This single follow-up commit cleanly addresses both non-blocking notes from the previous approval:

• config.example.yaml now documents translator.carry-over-thinking-in-system, including default-off behavior, the labeled unverified-context system instruction, 3-block / 4000-rune bounds, and the canonical-target exclusion. The wording matches the implementation.
• The unreachable len(keptParts) == 0 branch in carryOverClaudeSource is removed. The prior guard drops an extracted assistant turn when keptParts is empty and there is no tool use; any tool_use is an object appended to keptParts, so all surviving paths can safely call JoinRawArray(keptParts) directly. This is behavior-preserving simplification.

GitHub reports the PR cleanly mergeable, and CI runs go build ./cmd/server plus go test ./...; build check is green at this head. No findings.