Repository navigation
feat(engine): expose route output budget to hosts - #150
Conversation
d0322fd to
c997d2e
Compare
Fresh from-scratch review (6 parallel lanes + full local gate replication)Verdict: approve-grade. The change is correct, minimal, and genuinely useful. Head What was verified (from zero, not from the PR's own claims):
Fixed during review (three force-pushes:
Not this PR's problem, but recorded: the motivating parent-side consumer does not exist yet in Pinvou/pinvou-agent (no Non-blocking follow-ups: Local verification: fmt clean; |
Fresh from-scratch re-review — round 2 (9 lanes + local gate replication + mutation battery)Head reviewed: Verdict: approve-grade; 0 BLOCKER / 0 MAJOR. The four review questions, answered from zero (not from the PR's own claims or the previous round): 1. Useful, complete, root-caused — yes.
2. Elegant — yes. Follows the precedent precisely (same file, same 3. Smuggling — clean. Single commit, single parent == base tip; +72/−0 in exactly 2 files = doc + 4. Bug-free — yes, production code. The whole chain re-audited function by function: total (no panics/unwraps/indexing reachable from host input), saturating arithmetic, fail-closed (uncatalogued → New non-blocking findings (this round)
Local replication (this machine, fresh clone at c997d2e)
None of the above blocks merge; MINORs 1-2 and the two SHOULDs are one-commit follow-ups that can ride the next force-push or a later pass. The #150 → codewhale-hq#841 re-pin is mandatory sequencing, already anticipated by codewhale-hq#841's own body. |
JensenChen28
left a comment
There was a problem hiding this comment.
已审查固定 head c997d2ec0d0f02b56024fa8040d6faf2bd8d6da0,base 11cba403dc6bd6ca5c4dca55042785b03449ac8e,批准本 PR。
Standards
完整两文件 +72 行差异未发现阻断问题。公开包装及 re-export 沿用既有输入预算接口的边界;包装函数与客户端使用同样的 provider/base_url 模型归一化,再调用同一现有路由预算函数。没有新增第二套限额算法或修改现有请求行为。
非阻断 P3:新测试虽持有环境锁,但没有临时移除并恢复 CODEWHALE_MAX_OUTPUT_TOKENS / DEEPSEEK_MAX_OUTPUT_TOKENS;合法继承覆盖会改变 65,536 的固定断言。建议沿用邻近测试的 EnvVarGuard 方式隔离,并补充 Some(RouteLimits) 控制。当前组合断言与别名对照也不足以单独检测未来客户端流程漂移。
Spec
公开签名、effective RouteLimits 输入契约、环境覆盖及传输字段差异说明符合本次“暴露既有预算计算”的范围。已核对 route 的 wire-model 安装、主循环 active limits、客户端最终取较小请求值的路径。接口提供路由上限,仍由宿主传入同一配置及有效限额;未声称已经验收父仓消费者或 gitlink 集成。
实际验证:fetch 最新 base/head、完整差异/调用链/测试内容审查、public re-export 与客户端组合的源码核对、单提交/DCO 检查、git diff --check 通过;只读核对该 head 的 11 项检查均成功。本机未运行 Rust/Cargo、测试、环境覆盖实验或 provider 请求;作者报告的原生运行和变异计数不作为本轮执行结果。发送前重新核对 head/base/讨论,以及 auto-merge/队列均未启用。未执行合并相关操作。
Standards:无阻断问题,保留测试稳健性建议。Spec:本 PR 的 API 暴露范围通过,父仓集成另验。
|
Thank you for the approval and the robustness note. The env-var P3 is acknowledged: the new test holds the environment lock but does not temporarily strip |
c997d2e to
dc73de7
Compare
JensenChen28
left a comment
There was a problem hiding this comment.
Approve dc73de769b75fbc2843a66dad3ce4b1aeae0d6d0, independently checked out against fetched maintenance 11cba403dc6bd6ca5c4dca55042785b03449ac8e.
Standards
The ambient-environment P3 is closed. Both output-token overrides are removed by restoring guards after acquiring the existing test lock; reverse drop order restores their original values before unlocking, including assertion unwinding. The two changelog entries accurately describe the existing public exposure. Production context code and the engine re-export are unchanged from the previously reviewed head; current author/trailer identity matches.
Spec
The wrapper still normalizes the wire model and delegates to the existing route budget calculation. The test-only isolation change preserves that behavior. The nonblocking suggestion for a Some(RouteLimits) control and an independent client-flow comparison remains useful future drift coverage; no current composition defect is established.
Validation: cumulative diff check, exact production/re-export identity and guard-lifetime/source inspection. No local Cargo, native environment experiment, provider execution or author test-count adoption. This approval covers this API exposure, not an unreviewed parent consumer or gitlink update. Required CI remains unwaived; no merge or queue operation.
JensenChen28
left a comment
There was a problem hiding this comment.
补充固定 head dc73de769b75fbc2843a66dad3ce4b1aeae0d6d0 的规范反馈;已有源码批准 5812347923 的功能结论保留。
Standards
环境隔离 P3 已在源码层面闭合:两项 EnvVarGuard 在环境锁之后声明,退出/断言 unwind 时先恢复原值,再释放锁。已核对生产 context 部分及 engine re-export 与此前已审 head 逐字节相同。
[P3] 请移除本 PR 新增的两份 changelog hunk。 CONTRIBUTING.md:191–197 明确要求 changelog 在 main 合并时由发布管理者统一编写,并明确禁止在 PR 分支修改 CHANGELOG.md 和 crates/tui/CHANGELOG.md。本次两处新增内容虽然描述准确,仍与该约定冲突;本轮没有看到适用例外。请仅剥离这两处新增 hunk,保留环境隔离修复,或由维护者明确说明例外。此项是范围有限的贡献规范清理,不新增生产正确性 P2。
Spec
API 暴露行为未变化,本轮需求审查通过。Some(RouteLimits) 控制及独立客户端流程一致性测试仍为有用的非阻断后续覆盖,尚未发现当前组合错误。父仓消费者及 gitlink 集成仍需另验。
实际验证:完整最新增量、守卫捕获/恢复及生命周期源码、生产代码字节一致性、累计 diff check;未运行本机 Rust/Cargo、真实环境变量实验或 provider。作者/其他审核者的原生运行计数未作为本轮执行结果。发送前已重核 head/base/讨论。
Hosts issuing their own one-shot requests (the Pinvou review summon) must put the same max_tokens on the wire as the main turn. Reusing the route-effective cap via a public wrapper mirrors the context_input_budget_for_route precedent: share the math instead of re-deriving constants and silently drifting from the engine. Signed-off-by: asto <asto18089@126.com>
dc73de7 to
af494b7
Compare
|
Accepted — the two changelog hunks are stripped on the rewritten head |
JensenChen28
left a comment
There was a problem hiding this comment.
批准固定 head af494b7a81c86e5a99b769da5559dcf30e9a2c88,base 11cba403dc6bd6ca5c4dca55042785b03449ac8e。
Standards
两份 changelog 均已恢复为 base 的原始字节,关闭此前 CONTRIBUTING.md:191–197 的规范反馈。与已审 dc73 head 相比仅移除这 16 行;生产代码、测试、公开签名及 engine re-export 保持一致,环境变量隔离修复保留。当前 author/Signed-off-by 匹配。
Spec
公开预算接口及其模型归一化、现有限额计算委托符合范围,无新增问题。Some(RouteLimits) 控制和独立客户端流程一致性覆盖仍可作为非阻断后续工作;父仓消费者与 gitlink 集成尚未验收。
实际验证:重新 fetch、完整最新差异及累计范围复核、Git blob/字节一致性、守卫修复保留和 diff check。未本机运行 Cargo/Rust、环境变量实验或 provider;未借用外部测试计数。发送前核对 head/base/讨论,auto-merge 与队列均为空。未执行合并相关操作。
zhuowp
left a comment
There was a problem hiding this comment.
Approved at af494b7a81c86e5a99b769da5559dcf30e9a2c88 against 11cba403dc6bd6ca5c4dca55042785b03449ac8e. I reviewed both files and all three changed fragments. The public helper uses the same model normalization and numeric ceiling as the client when given the same resolved provider, endpoint, model and limits. Request serialization still determines the field for each dialect.
The fixed Linux CI tree passed the new regression and recorded 15,003 tests passed with 15 skipped. All 11 checks succeeded. This approval covers the library API. Downstream host adoption and live provider behavior require separate validation.
Merges pinvou3-clean@6f7a3a748 (#136/#142/#148/#150/#152) into the rate-limit recovery branch, preserving both histories per the established integration pattern. Conflict resolutions (composed, not one-sided): - client.rs: keep the deferred `prepare_outbound_request` fallback from the maintenance route-budget work and wrap the prepared-stream operation in `rate_limit::with_operation` so recovery still owns the logical operation regardless of preparation timing. - turn_loop.rs: adopt the maintenance-typed `TurnFailure` + `split_stream_open_error` plumbing while keeping the rate-limit interruption contract: a RateLimited/QuotaWindow failure parks the turn as `Interrupted` (message surfaced, no terminal error source consumed) and otherwise `finish()` records the typed source as before. Both stream-open sites classify first, then flag `rate_limit_interrupted`. Signed-off-by: asto18089 <asto18089@126.com>
|
The follow-ups recorded in this thread (the |
Why
No-Issue: fork-only host-facing API exposure; no issue tracks it (repository has issues disabled).
Hosts that issue their own one-shot model requests must put the same
max_tokenson the wire as the main turn. The route-effective cap (explicit operator override → documented ceiling ∩ automatic per-request API cap → compatibility/route ceilings → route window clamp, with the wire model normalized exactly as the transport does) lives inroute_budget::effective_max_output_tokens_for_route, which ispub(crate)— so the host cannot reuse it and is pushed toward hard-coded constants that drift.Concrete drift that motivated this (Pinvou review summon, 2026-11-06): the review call hard-coded
max_tokens: 1600; the Kimi K3 reasoning model spent the entire budget on reasoning tokens (empty content,finish_reason=length), and even an interim hard-code of 8192 was 8× under the main-turn wire value (65536 = documented 131072 clamped by the 65536 per-request API cap) for the same route.What
core::engine::route_effective_max_output_tokens(provider, base_url, requested_model, route_limits)next to the existing precedentcontext_input_budget_for_route(published for exactly this reason on the input side).core::enginealongside it. No behavior change; the wrapper delegates to the single existing source.Usage (host side, Pinvou/pinvou-agent)
with
route_limits= the effective limits the engine session resolved for the route (the same host-declared facts carried into the engine — notNone, which falls back to catalogue defaults and diverges wherever the route carries declared facts), andprovider/base_urlfrom the same config the engine's client uses.Notes
route_budgetas apub modof its engine crate (pub fn effective_max_output_tokens_for_route), so integrations there reach the math directly. The fork's single-crate layout keeps that modulepub(crate), and this wrapper restores the same host-facing reachability next tocontext_input_budget_for_route. The parent-side consumer lands via the parent gitlink bump.