Repository navigation
fix(tui): let todo items default to a pending status - #148
Conversation
The schema listed status among the required item fields, but the executor reads a missing status as TodoStatus::Pending — 'leave status out = new pending todo' is the tool's real contract, so the schema was stricter than the execution semantics and a valid call shape was declared invalid. status leaves required (content stays: an item without text is not a todo) and the status description states the default where the model reads it. Fork-distinct behavior change versus the upstream schema (upstream pins content+status required); flagged for the fork-modifications register. Signed-off-by: asto <asto18089@126.com>
Fresh 10-lane review — APPROVE (head
|
JensenChen28
left a comment
There was a problem hiding this comment.
已审查固定 head ef7a1d9a91f7b613d61730a22de259dd70bf8b79,base 11cba403dc6bd6ca5c4dca55042785b03449ac8e。规范与需求两个轴均无实质问题,批准本 PR。
schema 允许省略 item.status,并说明默认为 pending,与既有执行器一致;todos/content 仍必填,显式状态枚举、错误处理、列表替换及发布流程未变。已追踪 fallback 与 attached Work graph 接收同一个已定型 snapshot 的路径;新增 async 测试覆盖的是 fallback,未将其视为完整图集成或 provider 测试。
实际验证:fetch 最新 base/head、完整 diff/调用链检查、git diff --check、从实际 Rust 源码提取 JSON schema 并验证 required/enum,以及比较确认生产代码仅两处 schema 变更、执行器未改。只读核对该 head 的远程检查均通过(sync 跳过)。本机未运行 Cargo/Rust 或真实 provider 请求,作者测试计数属于外部证据。父仓消费 gitlink 时的 fork 登记/指纹与集成验证仍由对应父仓变更处理;本审核未执行该集成。发送前已核对最新 head/base 和讨论未变化。
zhuowp
left a comment
There was a problem hiding this comment.
Approve ef7a1d9a91f7b613d61730a22de259dd70bf8b79, reviewed against actual pinvou3-clean / merge-base 11cba403dc6bd6ca5c4dca55042785b03449ac8e.
Making item status optional aligns the model-facing schema with the existing executor's explicit Pending default. Item content and the outer list remain required; explicit status values, complete-list replacement and output checklist fields retain their existing contracts. The canonical todo_write and compatibility aliases share the real handler. The attached work-graph and detached-list consumers receive an explicit typed status, and invalid later items are rejected before publishing a replacement.
The v3 review covers the complete one-file change, all three fragments and both complete new test bodies, with registration, provider adaptation, execution, ownership and checklist consumers checked. No introduced blocking defect was confirmed. Exact-head CI executes both new tests successfully; its workspace nextest result is 15004 passed and 15 skipped. The executor-default test preserves an existing behavior; the schema test distinguishes the changed requirement.
This review does not claim live-provider, real-model, visible-terminal or packaged-release execution. Source approval does not override branch protection, merge-queue checks or release authorization.
JensenChen28
left a comment
There was a problem hiding this comment.
Reviewed the current diff at head 42b34a41a891a69e930e66aa43b4d95c94773ba5 against base 78804c83e05ccf2f26169a1571b49886565306fa. The two-commit branch diff is limited to the todo item schema and colocated tests. It keeps content required, makes status optional, documents the existing pending default, and tests both the declared schema and status-less execution result. I found no substantive issue.
Verification performed: git diff --check passed. I attempted cargo test -p codewhale-tui --lib todo_ --locked, but Cargo is not available in this environment (cargo is not recognized); no Cargo tests ran. The PR's test claims are not counted as local execution.
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>
Problem
The
todo_writeitem schema listedstatusamong the required item fields, but the executor has always read a missingstatusasTodoStatus::Pending(crates/tui/src/tools/todo.rs). "Leave status out = new pending todo" is the tool's real contract, so the schema was stricter than the execution semantics and declared a valid call shape invalid — exactly the failure mode the toolset contract's "minimize required parameters / if leaving it blank is business-sane, it must not be required" rule exists to prevent.Document evidence (repository contract, no external provider doc involved)
TodoStatusdefaults toPendingfor a missing status — the execution path is the authority for what the tool accepts (crates/tui/src/tools/todo.rs).content+statusrequired, so relaxingstatusis fork-distinct behavior — flagged for the fork-modifications register.Changes (head
ef7a1d9a9)requiredon todo items narrows to["content"];statusleaves the list.statusdescription now states the default where the model reads it: "Task status. Defaults to pending when omitted."contentstays required — an item without text is not a todo.Test evidence
todo_item_status_is_optional_with_a_pending_defaultasserts the schema'srequiredand the stated default in the description.work_update_treats_a_missing_status_as_pendingexecutes a status-less item end-to-end and asserts the resulting checklist entry ispending.Behavior changes
statuson a todo item; the executor records it aspending, matching behavior that already existed.statusvalues behave exactly as before.Merge-conflict surface with #137
This branch edits
crates/tui/src/tools/todo.rsonly; it does not touchcrates/tui/src/client/chat.rsor theuse crate::config::{...}block that #137 (fix/kimi-k31-replay-spellings) edits, so there is no conflict surface with #137.Probe declaration
No live probes were used in this change; the schema relaxation is derived from the executor's existing default, verified by the two tests above.
No-Issue: provider compatibility batch grounded in the published provider docs (fetched 2026-11-05); the repository has issues disabled.