Skip to content

feat(session): 增加 Todo 状态持久化与校验#280

Merged
phantom5099 merged 3 commits into
1024XEngineer:mainfrom
wynxing:refix_br
Apr 14, 2026
Merged

feat(session): 增加 Todo 状态持久化与校验#280
phantom5099 merged 3 commits into
1024XEngineer:mainfrom
wynxing:refix_br

Conversation

@wynxing

@wynxing wynxing commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

背景

Session 增加结构化 Todo 状态,沿用现有 internal/session JSON 持久化链路,不引入新的存储子系统。

本次修改

  • internal/session 新增 TodoItemTodoStatus
  • Session 增加 Todos []TodoItem 字段
  • Save/Load 路径中增加 Todo 的规范化和校验
  • 支持旧 session 文件缺失 todos 字段时按空列表兼容加载
  • 增加 Todo 基础 helper:
    • FindTodo
    • AddTodo
    • UpdateTodoStatus
    • DeleteTodo
  • 删除 Todo 前增加反向依赖检查,返回更明确的错误信息
  • 同步补充 session 持久化文档和 Todo 设计说明

校验规则

  • Todo 状态固定为:
    • pending
    • in_progress
    • completed
  • 拒绝空 id
  • 拒绝空 content
  • 拒绝重复 ID
  • 拒绝自依赖
  • 拒绝引用不存在的依赖项

测试

已通过:

go test ./internal/session/...
go test ./...

link #261

@codecov

codecov Bot commented Apr 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.58170% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/session/todo.go 86.61% 12 Missing and 7 partials ⚠️

📢 Thoughts on this report? Let us know!

fennoai[bot]

This comment was marked as outdated.

fennoai[bot]

This comment was marked as outdated.

fennoai[bot]

This comment was marked as outdated.

fennoai[bot]

This comment was marked as outdated.

@wynxing

wynxing commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator Author

/simplify

@fennoai

fennoai Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Completed

  • Checked historical review comments and identified unresolved items.
  • Fixed all worth-fixing unresolved comments: repaired garbled UTF-8 Chinese text in docs/session-persistence-design.md (todos section).
  • Ran a code-simplifier subagent pass and applied a behavior-preserving simplification in internal/session/todo.go:
    • extracted shared ensureTodoID helper
    • removed duplicated Todo ID trim/empty checks in FindTodo / UpdateTodoStatus / DeleteTodo
  • Added regression test TestSessionTodoHelpersRejectEmptyID in internal/session/todo_test.go.
  • Ran tests successfully:
    • go test ./internal/session/...
    • go test ./...
  • Committed and pushed to the existing PR branch (Yumiue:refix_br).

Commit

  • c3af8c4 fix(session): resolve todo doc garbling and simplify todo id validation

Not Done

  • No additional non-requested refactors were made.

View job run

- repair garbled UTF-8 text in session persistence todo section
- extract ensureTodoID to reduce duplicated id normalization
- add regression test for empty todo id behavior

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Yumiue <188874804+Yumiue@users.noreply.github.com>
@wynxing

wynxing commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator Author

/review

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已按代码质量、性能、安全性、文档一致性完成复核,提交 1 条建议修正项(非阻塞)。

Comment thread internal/session/todo.go
break
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normalizeAndValidateTodoslen(items)==0 时返回 nil,会让 Save/LoadSession.Todosnil[] 之间切换。当前 NewWithWorkdir 初始化为 []TodoItem{},且文档将 todos 描述为固定结构字段;该行为会带来语义不一致与序列化不稳定(omitempty 下字段可能被省略)。建议这里返回空切片([]TodoItem{})或在 Save/Load 层统一规范为空切片。

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

基于当前 PR 头部提交完成审查。发现 1 个建议修复项(测试覆盖缺口),其余代码质量/性能/安全/文档一致性未见阻塞问题。

}
if err := session.UpdateTodoStatus("todo-1", TodoStatus("paused")); err == nil || !strings.Contains(err.Error(), `invalid todo status`) {
t.Fatalf("expected invalid status error, got %v", err)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议补一组 nil receiver 用例((*Session)(nil).AddTodo/UpdateTodoStatus/DeleteTodo)。当前生产代码在对应分支返回错误,但测试未锁定这些保护分支,后续重构时容易回归为 panic 或错误信息漂移。

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed a focused review of the latest PR diff (quality, performance, security, and documentation accuracy).

No blocking issues found in the current revision.

@@ -0,0 +1,47 @@
# Session Todo 设计说明

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed end-to-end against the current implementation in internal/session; no inconsistencies found here.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the latest PR diff at commit c3af8c451c1b1928f9750361ce80bb680762870f across code quality, performance, security, and documentation consistency.

No noteworthy issues found in the current patch set.

@@ -51,6 +52,22 @@ NeoCode 当前使用本地 JSON 文件持久化会话,以保持实现简单、
- `user_constraints`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked this section in the latest commit: the UTF-8 text is readable and aligned with the current Todo persistence behavior.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已基于最新提交 c3af8c451c1b1928f9750361ce80bb680762870f 完成复审(代码质量、性能、安全、文档一致性)。

结论:未发现阻塞性问题;以下为 1 条可选一致性优化建议。

Comment thread internal/session/todo.go
break
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normalizeAndValidateTodos 在空输入时返回 nil,会让 Session.Todosnil/[] 之间切换。当前 NewWithWorkdir 初始化为 []TodoItem{},如果希望持久化前后语义更稳定,可考虑统一规范为空切片。

@phantom5099 phantom5099 merged commit d4e108a into 1024XEngineer:main Apr 14, 2026
2 checks passed
@Cai-Tang-www

Copy link
Copy Markdown
Collaborator

@xgopilot 这个Pr完美的实现了issues/261吗

@fennoai

fennoai Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

结论

不算“完美实现” issue #261;当前 PR 只覆盖了其中一部分基础能力。

未覆盖或不满足的关键点

  1. 状态集合不完整:当前仅支持 pending/in_progress/completedinternal/session/todo.go:13-20),缺少 issue feat(todo): 在 Session 中引入结构化 Todo 状态并完成持久化 #261 要求的 blocked/failed/canceled
  2. 缺少状态机迁移约束UpdateTodoStatus 仅校验目标状态是否合法(internal/session/todo.go:88-119),没有实现 issue 要求的“按来源状态限制迁移、终态不可逆”。
  3. 数据模型不完整TodoItem 仅有 ID/Content/Status/Dependencies/Priority/CreatedAt/UpdatedAtinternal/session/todo.go:23-31),缺少 issue 提到的 OwnerType/OwnerID/Acceptance/Artifacts/Revision 等字段,也没有 TodoVersioninternal/session/store.go:22-38)。
  4. 并发保护未实现:没有 expected_revision 校验与 revision 递增逻辑(代码与测试中均未出现该机制)。
  5. 依赖校验未做环检测:目前只做“自依赖 + 依赖存在性”校验(internal/session/todo.go:198-207),未实现 issue 要求的循环依赖(DAG)检测。
  6. Helper 集不一致:已有 FindTodo/AddTodo/UpdateTodoStatus/DeleteTodointernal/session/todo.go:49-155),但 issue 要求的 UpdateTodo/GetTodoByID/ListTodos/SetTodoStatus 组合未完整对齐。
  7. 测试矩阵未覆盖上述缺口:现有测试覆盖 round-trip、缺失字段兼容、基础校验(如 internal/session/store_test.go:772-937, internal/session/todo_test.go:9-187),但没有状态迁移矩阵、环检测、revision 冲突相关测试。

已覆盖的部分(仅限本 PR 范围)

  • Session 已持久化 Todos,并在 Save/Load 做规范化与校验(internal/session/store.go:85-90internal/session/todo.go:178-231)。
  • 缺失 todos 字段可兼容加载为空列表(internal/session/store_test.go:834-870)。
  • 实现了基础增删改查与删除前依赖阻断(internal/session/todo.go:49-155)。

如果要达到 issue #261 的“完整验收标准”,还需要补齐上面 1-7 项。

View job run

@fennoai fennoai Bot mentioned this pull request Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants