[asto18089]

Why

No-Issue: fork-local test/documentation tightening recorded in the CodeWhale#150 review thread (JensenChen28's 2026-11-07 acknowledgment and #150's own follow-up notes); no issue tracks it (repository has issues disabled).

#150 (merged as e972a9a05) exposed route_effective_max_output_tokens for hosts. Its review recorded bounded follow-ups: a Some(RouteLimits) control through the wrapper, an honest statement of what the DeepSeek alias leg pins, and closing the latent client-flow drift lane. This PR lands those. (The EnvVarGuard half of the recorded follow-up already landed pre-merge at dc73de769.)

What

1. Single shared copy of the normalize-then-intersect composition — new pub(crate) route_budget::route_effective_max_output_tokens; the transport clamp (client.rs::effective_max_output_tokens_with_limits) and the host-facing core::engine::route_effective_max_output_tokens both delegate to it. Previously the composition existed as two parallel copies — the exact "silently drifting from the engine" failure mode the wrapper exists to prevent. Behavior-identical: the helper performs the same two calls the clamp inlined (wire_model_for_provider_route → effective_max_output_tokens_for_route).
2. Some(RouteLimits) control through the wrapper: a route-declared output ceiling clamps the catalogue cap exactly as the main turn's active_route_limits do, and the composition equality holds with limits attached. This is the doc's primary instructed input; until now only the None catalogue-default path was pinned through the wrapper.
3. Normalization-sensitive pin: OpenRouter kimi-code canonicalizes to moonshotai/kimi-k2.7-code → catalogued 32,768; raw, it is uncatalogued and fails closed at 8,192. Deleting the normalization call now turns the test red (mutation-verified). The previous test could not observe normalization loss: Moonshot is pass-through and the DeepSeek alias pair is budget-invariant through provider_capability's alias arms — recognition redundancy, which the DeepSeek leg's comment now states explicitly instead of implying it guards normalization.
4. Doc precision + #[must_use] on the wrapper: the window rung clamps to window − 2_048 (input floor + headroom reserve) and the result is floored at 1; base_url must be the endpoint the request dispatches on (custom endpoints own their model-id namespace and skip normalization); the max_completion_tokens rename set is Xiaomi MiMo, OpenAI reasoning families, MiniMax M3, and the direct Moonshot K3 route (per apply_provider_token_limit). #[must_use] aligns the wrapper with both layers beneath it (route_budget.rs, client.rs).

Verification

• cargo fmt --all -- --check clean; gitleaks dir . 0 findings.
• Wrapper pin green with the new legs; env-override behavior re-proven (CODEWHALE_MAX_OUTPUT_TOKENS=16384 now passes — the guards strip ambient overrides); k3 route pin, uncatalogued pin, and the client effective_max_output_tokens* tests green (delegation sanity).
• Mutations: dropping the helper's normalization call → red (kimi-code leg 32,768 → 8,192); swapping the helper's base_url/model args → red (both the kimi-k3 pin and the kimi-code leg fall to 8,192); revert → green again.

Notes

• No behavior change: every wire value is identical; the diff is one extracted helper plus two delegations, test legs, and doc text.
• The test name route_effective_max_output_tokens_matches_the_transport_composition is unchanged, so the parent-side fingerprint registration from the feat(engine): expose route output budget to hosts #150 gitlink batch stays valid; the new test legs are additive and will need parent fingerprint rows at the next gitlink bump (parent AGENTS.md §2).
• Not taken here (recorded for later): unifying turn_loop.rs's main-turn cap computation (it reads session.model without normalization; parity holds because the route install makes that the wire id) with the client-side clamp — a production-path change beyond this bounded follow-up.
• DCO: commit is signed off.

[asto18089]

Fresh from-scratch review — round 1 (6 parallel lanes + local gate replication + mutation battery)

Head reviewed: 85f62f833. Rebase: no-op — the single commit's parent is exactly the current pinvou3-clean tip 6f7a3a748, so there is nothing to re-apply.

Verdict: approve-grade. 0 BLOCKER / 0 MAJOR / 2 MINOR (both body-level) / 5 NITs. Everything below was re-derived from primary sources on this exact head, not taken from the PR's own claims.

1. Useful, complete, root-caused — yes

• Provenance re-verified against feat(engine): expose route output budget to hosts #150's threads: JensenChen28's 2026-11-06 P3 (EnvVarGuard isolation + Some(RouteLimits) control), round-2 SHOULD (no structural anti-drift pin — "cleanest fix: have effective_max_output_tokens_with_limits delegate to the wrapper"), and round-2 MINOR (DeepSeek alias leg doesn't guard normalization). This PR lands exactly that recorded set; the EnvVarGuard half is confirmed pre-landed (its content is in base via the feat(engine): expose route output budget to hosts #150 squash), and the turn_loop.rs unification is honestly recorded as not-taken (verified: that file is untouched).
• The root cause is real: at base, normalize-then-intersect existed as exactly two inlined production copies (the client.rs clamp and the context.rs wrapper) — both resolving the same config::wire_model_for_provider_route, so behavior was identical but structurally free to drift. A census of the whole codewhale-tui crate confirms there was no third copy and there is no remaining inlined copy: post-PR the composition has one home (route_budget::route_effective_max_output_tokens) and exactly two delegators. The remaining intersect-only sites (turn_loop.rs, exec_agent.rs, preview.rs, envelope consumers) are different compositions on already-resolved models, consistent with the body's accounting.
• MINOR-1: one recorded feat(engine): expose route output budget to hosts #150 round-1 follow-up is neither landed nor accounted in the body — context_input_budget_for_route (context.rs:998) still documents neither its expected model form nor its RouteLimits contract. Doc-only; suggest a body line or a tracked follow-up so the accounting is complete.

2. Elegant — yes

Minimal +82/−16; no wheel reinvention (the helper composes two existing functions); correct home (route_budget owns the budget math), pub(crate) keeps it out of the host API while core::engine::route_effective_max_output_tokens remains the host surface; argument order follows the crate convention (provider-first, limits-last); #[must_use] aligns with both layers beneath. The doc precision claims verify 7/7 TRUE against code: window−2_048 is genuinely MIN_INPUT_BUDGET_TOKENS 1_024 + CONTEXT_HEADROOM_TOKENS 1_024 (context_budget.rs:269-274); floor-at-1 is the .max(1) tail of effective_max_output_tokens_for_route (route_budget.rs:366); the max_completion_tokens rename set is exactly the four arms of apply_provider_token_limit (client/chat.rs:68-71), nothing missing/extra; custom endpoints return the trimmed requested id un-normalized (config.rs:1508-1510); env overrides now provably move wrapper and main turn identically (single shared application).

3. Smuggling — clean

Every hunk maps 1:1 to the four claimed changes; --numstat = 3 files, +82/−16 exactly; nothing outside crates/tui/; no Cargo.lock/VERSION/docs/workflow riders, no submodule/binary/mode changes. The client.rs wire_model_for_provider_route import remains legitimately used (4 remaining call sites); the tests-mod RouteLimits import is the only use change and resolves via the module-level import. Commit message ↔ diff consistent; DCO trailer present.

4. Bug-free — verified; gates and mutations re-run by me on the final tree

• Behavior identity: both delegators forward (provider, base_url, requested_model, route_limits) untransformed to the same two functions the base copies inlined — including the custom-endpoint path — so every wire value is byte-identical to pre-PR.
• #[must_use] fallout: none — every caller uses the result (tail expressions / assert_eq!); clippy with -D warnings is clean; no host call site exists in pinvou3-app.
• Performance: all call sites are per-request (prepare_outbound_request et al.), never per-token; the helper adds zero work, allocations, or lookups; window arithmetic is saturating (window ≤ 2_048 collapses toward the floor-at-1 — no underflow, no panic), matching the new doc.
• Local gates on 85f62f833 (repo Actions are billing-locked, so local is the bar): cargo fmt --all -- --check clean; cargo clippy -p codewhale-tui --all-features --locked -- -D warnings … clean; cargo test -p codewhale-tui --lib --all-features 12,457 passed / 0 failed (13 ignored, 4 threads, RUST_MIN_STACK=16MiB); doc-tests ok; gitleaks dir . no leaks (v8.30.1 local).
• Mutation battery (python exact-string replace, count-asserted both ways, warning-tolerant mutants — naive unused-param mutants merely fail the workspace warnings = "deny" build instead of going red): M1′ drop the helper's normalization call → RED at context.rs:1131 (8192 vs 32768 — exactly the claimed kimi-code kill); M2 swap the helper's base_url/model args → RED (8192 vs 65536, the Moonshot pin falls to the uncatalogued ceiling as the body states); M3′ drop the limits-forward → RED at context.rs:1145 (65536 vs 1000); every revert recompiled green; final tree git status clean. Note M3′ also confirms the Some(RouteLimits) leg's literal is the load-bearing check — the wrapper-vs-helper equality assertion alone is near-tautological post-delegation (it pins composition identity, appropriate here, same style as the pre-existing None-leg equality).

MINORs / NITs (none blocking; all are text-level)

1. MINOR-1 — unaccounted recorded follow-up (context_input_budget_for_route doc contract), see §1.
2. MINOR-2 — body attribution: the 2026-11-07 "recorded as a bounded follow-up" acknowledgment (issue comment 6705514880) is asto18089's own, responding to JensenChen28's 2026-11-06 P3; JensenChen28's 2026-11-07 posts are review verdicts (EnvVarGuard closure + "remains useful future drift coverage"). Substance intact, attribution wrong.
3. NIT — the new test comment says deleting "the wrapper's normalization call" turns the leg red; post-delegation the context.rs wrapper no longer contains a normalization call — it lives in route_budget::route_effective_max_output_tokens (route_budget.rs:303). The pointer should name the helper.
4. NIT — "custom endpoints own their model-id namespace and skip normalization entirely" slightly overstates: OpencodeGo normalizes even on an overridden base_url (config.rs:1505-1507, deliberate), and whitespace trimming still applies to custom endpoints.
5. NIT — window − 2_048 uses U+2212 inside an inline code span; sibling inline-code spans use ASCII -.
6. NIT — the body's "parent-side fingerprint registration from the feat(engine): expose route output budget to hosts #150 gitlink batch stays valid" cites a registration that does not exist yet: parent gitlink is still at 11cba403d, the batch is open as unmerged parent PR Clipboard copy fails over SSH — 鼠标选中文本后自动复制在 SSH 下失败 (Copy failed) codewhale-hq/Codewhale#841, and no fingerprint row anywhere references the wrapper or test name. Harmless in the worst way — fork-guard pins are fixed-string greps, all 8 pinned strings in the touched files survive verbatim, and the forkguard_ distinct-name floor (301) is unchanged — but the stated mechanism is not the real one.
7. NIT — "latent client-flow drift lane" in feat(engine): expose route output budget to hosts #150's thread originally named the turn_loop lane; this PR reuses the phrase for the wrapper/transport lane it does close. Both lanes are honestly accounted; a second's confusion only.

Fork boundary: purely fork-local surface — upstream (codewhale-hq/Codewhale) has no crates/tui/src/route_budget.rs, no tui/client.rs, no tui/core/, and zero symbol hits for the new name; no upstream drift created.

[JensenChen28]

Approved at the reviewed head. The shared helper preserves the existing normalize-then-intersect behavior for both the client clamp and host wrapper, and the scoped follow-ups/tests are present. I found no blocking implementation issue.

Non-blocking documentation note: context.rs says custom endpoints “skip normalization entirely,” but wire_model_for_provider_route still normalizes OpenCode Go with an overridden base URL; consider qualifying that statement.

The current GitHub checks failed before executing steps (the reported jobs have empty step lists); the PR author reports local checks, which I did not independently rerun.