From bab30952b82a740e25cd7fad2ec561a03aa2f212 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 5 Jun 2026 12:22:35 +0900 Subject: [PATCH] review: approve task domain move --- .../artifacts/review.md | 62 ++++++++++++++++ .../item.md | 2 +- .../thread.md | 70 +++++++++++++++++++ 3 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 work-items/open/20260605-025100-task-domain-in-pod-feature/artifacts/review.md diff --git a/work-items/open/20260605-025100-task-domain-in-pod-feature/artifacts/review.md b/work-items/open/20260605-025100-task-domain-in-pod-feature/artifacts/review.md new file mode 100644 index 00000000..7a865500 --- /dev/null +++ b/work-items/open/20260605-025100-task-domain-in-pod-feature/artifacts/review.md @@ -0,0 +1,62 @@ +# Review: task-domain-in-pod-feature + +## 1. Result + +approve + +## 2. Summary of implementation + +The implementation relocates the Task domain implementation out of `tools` and into `pod::feature::builtin::task`. The new Task feature module now owns the task store/domain types, task tool handlers, feature installation, and the existing reminder/snapshot/restore/rewind/compaction façade. The `tools` crate is reduced to the non-Task built-in tools and no longer exposes `tools::TaskStore`, `tools::task_tools`, or a `builtin_tools(..., task_store, ...)` API. + +TUI task parsing remains local to `tui` and does not add a production dependency on either `pod` or `tools`; its tests use local snapshot JSON fixtures instead of importing the previous TaskStore type. + +## 3. Requirement-by-requirement assessment + +- **Task domain ownership under `pod::feature::builtin::task`: satisfied.** `TaskStore`, `TaskEntry`, `TaskStatus`, `TaskSnapshot`, task tool implementations, reminder hook/state, and snapshot/restore/rewind/compaction methods are cohesive under the Pod built-in Task feature module. +- **No production Task APIs exposed from `tools`: satisfied.** The `tools` crate no longer has the Task module/API surface and `core_builtin_tools()` now installs only non-Task built-ins. +- **Non-Task `tools` behavior remains intact: satisfied.** The remaining tool modules, exports, and tests are conceptually unchanged apart from removing Task-specific test coverage from `tools`. +- **Task tool names/schemas/descriptions/outputs unchanged: satisfied.** The TaskCreate/TaskUpdate/TaskGet/TaskList definitions and handler response strings were moved without semantic changes. +- **TaskStore replay/snapshot/restore and reminder behavior unchanged: satisfied.** The store logic and TaskFeature façade preserve the previous append-history, snapshot, restore, rewind, overview, reminder, and compaction behavior. +- **TUI task compatibility without undesirable dependency: satisfied.** `tui` keeps its own typed compatibility reader and does not depend on `pod` or `tools` in production. +- **Normal ToolRegistry / PreToolCall path unchanged: satisfied.** Task tools are still registered through the built-in feature contribution path and continue through the existing Worker ToolRegistry / PreToolCall policy path. +- **No broad unrelated architecture changes: satisfied.** I did not find new crate-boundary/API extraction, plugin loading, authority-model, WorkItem/MCP, generic UI/event-channel, or broad refactor work in this diff. +- **`package.nix` / `Cargo.lock`: acceptable.** The `Cargo.lock` change follows from removing the `tools` dev-dependency from `tui`; the `cargoHash` update is therefore expected. The conditional `fetchCargoVendor` static-crates patch is not part of the Task-domain move, but it is a narrow and safe packaging guard around an existing static-crates workaround, and `nix build .#yoi --no-link` passed with it. +- **No `.yoi/workflow` or old `.insomnia` workflow changes included: satisfied.** The diff has no changed files under those paths. + +## 4. Blockers + +None. + +## 5. Non-blockers / follow-ups + +- `crates/tui/src/task.rs` still has a stale comment referring to `tools::render_snapshot` / `tools::TaskStore` ownership of the snapshot format. The code is fine, but the comment should be updated in a follow-up or before merge if the author is already revising the branch. +- TUI compatibility tests now rely on local fixture JSON, which is appropriate for avoiding a production dependency, but it leaves some residual drift risk if the Pod-owned snapshot format changes without updating the fixture. + +## 6. Validation assessed or rerun + +Reviewed: + +- Ticket, delegation intent, and decision artifact. +- `git diff develop...HEAD` for the implementation branch. +- Changed Task feature, tools, TUI, packaging, and lockfile files. +- Search results for remaining `tools::TaskStore`, `tools::task_tools`, workflow, and `.insomnia` changes. + +Reran from `/home/hare/Projects/yoi/.worktree/task-domain-in-pod-feature`: + +- `cargo test -p pod task --lib` +- `cargo test -p tools --lib` +- `cargo test -p tools --tests` +- `cargo test -p tui task --lib` +- `cargo test -p pod --lib` +- `cargo test -p llm-worker --lib` +- `cargo check --workspace --all-targets` +- `cargo fmt --check` +- `./tickets.sh doctor` +- `git diff --check` +- `nix build .#yoi --no-link` + +All rerun validation completed successfully. + +## 7. Residual risk + +The main residual risk is fixture drift in TUI task compatibility tests now that `tui` intentionally does not import the Pod-owned TaskStore type. That risk is acceptable for this ticket because the implementation preserves the existing serialized shape and avoids the undesirable production dependency. diff --git a/work-items/open/20260605-025100-task-domain-in-pod-feature/item.md b/work-items/open/20260605-025100-task-domain-in-pod-feature/item.md index cfaba29c..15d07493 100644 --- a/work-items/open/20260605-025100-task-domain-in-pod-feature/item.md +++ b/work-items/open/20260605-025100-task-domain-in-pod-feature/item.md @@ -7,7 +7,7 @@ kind: task priority: P1 labels: [tasks, feature-registry, crate-boundary, tools] created_at: 2026-06-05T02:51:00Z -updated_at: 2026-06-05T02:53:16Z +updated_at: 2026-06-05T03:22:31Z assignee: null legacy_ticket: null --- diff --git a/work-items/open/20260605-025100-task-domain-in-pod-feature/thread.md b/work-items/open/20260605-025100-task-domain-in-pod-feature/thread.md index 132d9797..0d17f1d8 100644 --- a/work-items/open/20260605-025100-task-domain-in-pod-feature/thread.md +++ b/work-items/open/20260605-025100-task-domain-in-pod-feature/thread.md @@ -138,4 +138,74 @@ Report: - whether ready for external review +--- + + + +## Review: approve + +# Review: task-domain-in-pod-feature + +## 1. Result + +approve + +## 2. Summary of implementation + +The implementation relocates the Task domain implementation out of `tools` and into `pod::feature::builtin::task`. The new Task feature module now owns the task store/domain types, task tool handlers, feature installation, and the existing reminder/snapshot/restore/rewind/compaction façade. The `tools` crate is reduced to the non-Task built-in tools and no longer exposes `tools::TaskStore`, `tools::task_tools`, or a `builtin_tools(..., task_store, ...)` API. + +TUI task parsing remains local to `tui` and does not add a production dependency on either `pod` or `tools`; its tests use local snapshot JSON fixtures instead of importing the previous TaskStore type. + +## 3. Requirement-by-requirement assessment + +- **Task domain ownership under `pod::feature::builtin::task`: satisfied.** `TaskStore`, `TaskEntry`, `TaskStatus`, `TaskSnapshot`, task tool implementations, reminder hook/state, and snapshot/restore/rewind/compaction methods are cohesive under the Pod built-in Task feature module. +- **No production Task APIs exposed from `tools`: satisfied.** The `tools` crate no longer has the Task module/API surface and `core_builtin_tools()` now installs only non-Task built-ins. +- **Non-Task `tools` behavior remains intact: satisfied.** The remaining tool modules, exports, and tests are conceptually unchanged apart from removing Task-specific test coverage from `tools`. +- **Task tool names/schemas/descriptions/outputs unchanged: satisfied.** The TaskCreate/TaskUpdate/TaskGet/TaskList definitions and handler response strings were moved without semantic changes. +- **TaskStore replay/snapshot/restore and reminder behavior unchanged: satisfied.** The store logic and TaskFeature façade preserve the previous append-history, snapshot, restore, rewind, overview, reminder, and compaction behavior. +- **TUI task compatibility without undesirable dependency: satisfied.** `tui` keeps its own typed compatibility reader and does not depend on `pod` or `tools` in production. +- **Normal ToolRegistry / PreToolCall path unchanged: satisfied.** Task tools are still registered through the built-in feature contribution path and continue through the existing Worker ToolRegistry / PreToolCall policy path. +- **No broad unrelated architecture changes: satisfied.** I did not find new crate-boundary/API extraction, plugin loading, authority-model, WorkItem/MCP, generic UI/event-channel, or broad refactor work in this diff. +- **`package.nix` / `Cargo.lock`: acceptable.** The `Cargo.lock` change follows from removing the `tools` dev-dependency from `tui`; the `cargoHash` update is therefore expected. The conditional `fetchCargoVendor` static-crates patch is not part of the Task-domain move, but it is a narrow and safe packaging guard around an existing static-crates workaround, and `nix build .#yoi --no-link` passed with it. +- **No `.yoi/workflow` or old `.insomnia` workflow changes included: satisfied.** The diff has no changed files under those paths. + +## 4. Blockers + +None. + +## 5. Non-blockers / follow-ups + +- `crates/tui/src/task.rs` still has a stale comment referring to `tools::render_snapshot` / `tools::TaskStore` ownership of the snapshot format. The code is fine, but the comment should be updated in a follow-up or before merge if the author is already revising the branch. +- TUI compatibility tests now rely on local fixture JSON, which is appropriate for avoiding a production dependency, but it leaves some residual drift risk if the Pod-owned snapshot format changes without updating the fixture. + +## 6. Validation assessed or rerun + +Reviewed: + +- Ticket, delegation intent, and decision artifact. +- `git diff develop...HEAD` for the implementation branch. +- Changed Task feature, tools, TUI, packaging, and lockfile files. +- Search results for remaining `tools::TaskStore`, `tools::task_tools`, workflow, and `.insomnia` changes. + +Reran from `/home/hare/Projects/yoi/.worktree/task-domain-in-pod-feature`: + +- `cargo test -p pod task --lib` +- `cargo test -p tools --lib` +- `cargo test -p tools --tests` +- `cargo test -p tui task --lib` +- `cargo test -p pod --lib` +- `cargo test -p llm-worker --lib` +- `cargo check --workspace --all-targets` +- `cargo fmt --check` +- `./tickets.sh doctor` +- `git diff --check` +- `nix build .#yoi --no-link` + +All rerun validation completed successfully. + +## 7. Residual risk + +The main residual risk is fixture drift in TUI task compatibility tests now that `tui` intentionally does not import the Pod-owned TaskStore type. That risk is acceptable for this ticket because the implementation preserves the existing serialized shape and avoids the undesirable production dependency. + + ---