feat(todo): 接入会话级 todo_write 工具链与上下文注入#310
Conversation
dddf113 to
3159479
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
/code 解决review |
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
|
/review |
| } | ||
|
|
||
| m.state.mu.Lock() | ||
| if err := mutate(&m.state.session); err != nil { |
There was a problem hiding this comment.
这里在持锁区先执行 mutate(&m.state.session) 改写内存态,然后才在解锁后调用 sessionStore.Save。如果 Save 失败,todo_write 会返回错误,但会话内存已经被修改,后续同一 run 仍会读取到“未持久化但已生效”的 Todo 状态,造成语义不一致。建议先克隆会话并在副本上 mutate+save,成功后再原子替换内存态,或在 save 失败时回滚。
| if input.ID == "" { | ||
| return fmt.Errorf("%w: action %q requires id", errTodoInvalidArguments, actionRemove) | ||
| } | ||
| return call.SessionMutator.DeleteTodo(input.ID) |
There was a problem hiding this comment.
remove 分支忽略了 expected_revision(直接调用 DeleteTodo(id)),这会绕过本 PR 引入的 optimistic concurrency 语义:并发场景下,旧 revision 的调用也能删除最新 Todo。建议让删除路径也接入 revision 校验(例如 DeleteTodo(id, expectedRevision)),并在冲突时返回 revision_conflict。
There was a problem hiding this comment.
Found 1 noteworthy issue after reviewing the PR diff.
runtimeSessionMutatormutates in-memory session state before durable save, which can leak failed writes into later runtime behavior.
Please consider applying mutation on a cloned session and only swapping into state.session after Save succeeds (or explicitly rollback on save failure).
| } | ||
|
|
||
| m.state.mu.Lock() | ||
| if err := mutate(&m.state.session); err != nil { |
There was a problem hiding this comment.
mutate(&m.state.session) updates the live in-memory session before sessionStore.Save is attempted. If Save fails, this function returns an error but state.session remains mutated. In the current run loop, a later successful save (for example while appending tool messages) can persist this mutation anyway, so callers observe an error while the Todo change still takes effect.
This breaks atomicity between tool success/failure and session state. Consider mutating a cloned session snapshot first, saving it, then swapping into state.session only after save succeeds (or restoring the old state on save error).
| m.state.touchSession() | ||
| sessionSnapshot := cloneSessionForPersistence(m.state.session) | ||
| m.state.mu.Unlock() | ||
| return m.service.sessionStore.Save(m.ctx, &sessionSnapshot) |
There was a problem hiding this comment.
mutateAndSave persists a full session snapshot after releasing state.mu. Because tool calls run concurrently, another worker can append/save newer messages between unlock and this Save, and then this older snapshot may win the file write order and clobber those newer changes. This PR adds an extra save on every todo_write mutation, which makes lost-update races more likely. Please serialize these saves with the same critical section/version check (or otherwise make persistence compare-and-swap) so stale snapshots cannot overwrite newer session state.
| } | ||
|
|
||
| // validateInputLimits 校验 todo_write 入参的字符串与数组规模,避免放大 token/内存占用。 | ||
| func validateInputLimits(input writeInput) error { |
There was a problem hiding this comment.
expected_revision is never validated as a positive value. Because session-side checking treats <= 0 as "no revision check", a caller can pass -1 and silently bypass optimistic concurrency checks. Please reject negative revisions during input validation (for top-level and patch/action paths) to preserve conflict detection semantics.
| // dispatch 按 action 执行对应 Todo 变更。 | ||
| func (t *Tool) dispatch(call tools.ToolCallInput, input writeInput) error { | ||
| switch input.Action { | ||
| case actionPlan: |
There was a problem hiding this comment.
For action="plan", missing items currently falls through to ReplaceTodos(nil), which clears all todos silently. This makes malformed/incomplete model tool calls destructive. Consider requiring items to be explicitly present (or at least non-nil) for plan, and return invalid_arguments otherwise.
|
/code 解决这些review的问题 |
处理结果已按本次 trigger 要求修复 review 指出的问题,并已提交推送到当前 PR 分支。 已完成
验证
提交信息
额外说明按 fork push 流程同步创建了 fork 侧 PR: 未处理项:无。 |
…checks Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
fix(todo): address review issues for todo_write and mutator
| } | ||
|
|
||
| // todoWireItem 表示 todo_write 入参中的单个 todo 项,并兼容 legacy title 字段。 | ||
| type todoWireItem struct { |
There was a problem hiding this comment.
todoPatchInput.toSessionPatch() sets RetryCount/RetryLimit/NextRetryAt, but session.TodoPatch does not have these fields (internal/session/todo.go). This currently causes build failure (unknown field ... in struct literal). The same contract mismatch also appears in decodeLegacyItem for session.TodoItem.
| return fmt.Errorf("%w: action %q requires item", errTodoInvalidArguments, actionAdd) | ||
| } | ||
| return call.SessionMutator.AddTodo(*input.Item) | ||
| case actionUpdate: |
There was a problem hiding this comment.
action="plan" directly calls ReplaceTodos(input.Items) without validating that items was actually provided. Because tool manager does not enforce JSON schema at runtime, malformed calls missing items will silently clear all todos (nil slice). Please return invalid_arguments when items is absent for plan.
| if err := ensureTodoWriteTextLength("owner_id", input.OwnerID); err != nil { | ||
| return err | ||
| } | ||
| if err := ensureTodoWriteTextLength("reason", input.Reason); err != nil { |
There was a problem hiding this comment.
expected_revision is never validated as non-negative. In session logic, expectedRevision <= 0 means "skip revision check", so negative values can bypass optimistic concurrency checks unintentionally. Please reject negative values during input validation.
| return fmt.Errorf("%w: action %q requires id", errTodoInvalidArguments, actionClaim) | ||
| } | ||
| if strings.TrimSpace(input.OwnerType) == "" || strings.TrimSpace(input.OwnerID) == "" { | ||
| return fmt.Errorf("%w: action %q requires owner_type and owner_id", errTodoInvalidArguments, actionClaim) |
There was a problem hiding this comment.
action="remove" ignores expected_revision and calls DeleteTodo(id) unconditionally. This breaks revision consistency with other mutating actions (update/set_status/claim/complete/fail) and allows stale callers to delete newer todos.
|
/code 修复reviews |
处理结果已完成“修复reviews”核查与验证。基于历史 review 评论逐项对照当前分支代码后,相关问题均已修复,未发现新的待改项,因此本次未新增代码提交。 已确认项
验证
未执行项
|
变更说明
本 PR 完成 Todo 能力在主链路中的落地,保持
TUI -> Runtime -> Provider -> Tools边界不变。核心改动
todo_write工具(plan/add/update/set_status/remove/claim/complete/fail)SessionMutator,使工具可安全写回会话 TodoTodo State注入源,提升模型可见性todo_updated/todo_conflict/todo_summary_injectedtodo_write使用约束,提升模型任务编排稳定性测试
internal/tools/todo/*_test.gointernal/runtime/todo_mutator_test.gointernal/runtime/todo_runtime_integration_test.gointernal/context/source_todos_test.gointernal/session/todo_test.go(补充分支)go test ./... -count=1通过风险与兼容
Closes #261
Closes #262