feat(todos): add CRUD tool + RPC for the agent task board#1983
Merged
Conversation
Add a "Never write code on `main`" rule to the Git workflow sections of both AGENTS.md and CLAUDE.md so agents fork a feature branch off the latest `main` before any code edits. Keeps `main` clean and advancing only via merged PRs.
Replace the wholesale `todowrite` tool with a single `todo` tool that dispatches on an `op` field (`add` / `edit` / `update_status` / `remove` / `replace` / `clear` / `list`), and expose the same operations over JSON-RPC as `openhuman.todos_*` so the desktop UI can drive the list directly. Both surfaces share `openhuman::todos::ops` and persist into the existing per-thread `TaskBoardStore`, so agent and user edits stay in lock-step. Tool results and RPC responses include a markdown rendering of the list (`- [ ] …` / `- [~] …` / `- [!] …` / `- [x] …`) with card ids and indented notes/blockers so transcripts and the task board UI can render without re-formatting. Surfaced to the orchestrator via the existing global tool registry (orchestrator has `allowed_tools: None`); the `Writer` profile fixture and `all_tools` registry test updated from `todowrite` to `todo`.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR consolidates todo/task board management by removing the single-operation ChangesGit workflow documentation
Todos subsystem refactor and tool unification
Sequence DiagramsequenceDiagram
participant Agent
participant TodoTool
participant ops as openhuman::todos::ops
participant TaskBoardStore
participant ScratchStore
Agent->>TodoTool: execute(op: "add", content, ...)
TodoTool->>TodoTool: current_location()
alt Thread context available
TodoTool->>ops: add(BoardLocation::Thread, ...)
else No thread context
TodoTool->>ops: add(BoardLocation::Scratch, ...)
end
ops->>TaskBoardStore: load_cards() or ScratchStore.snapshot()
ops->>ops: validate & apply mutations
ops->>ops: enforce single in-progress
ops->>TaskBoardStore: save_cards() or ScratchStore.replace()
ops->>ops: render_markdown(cards)
ops->>TodoTool: TodosSnapshot
TodoTool->>Agent: ToolResult {cards, markdown, threadId}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/openhuman/todos/store.rs (1)
34-39: ⚡ Quick winAdd structured logs on scratch-store init/access paths.
This new shared-state path currently has no observability. Please add debug logs with stable prefixes and context (e.g., init vs reuse).
As per coding guidelines:
src/**/*.rs: Add substantial, development-oriented logs on new/changed flows at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error handling paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/todos/store.rs` around lines 34 - 39, Add structured debug logs to the global_scratch_store path: when the function is called, log an entry message with a stable prefix (e.g., "scratch_store:enter"), then log whether you are initializing a new store or reusing an existing one (detect in the closure passed to OnceCell::get_or_init and the get path) with context like "scratch_store:init" vs "scratch_store:reuse". Include any relevant identifiers or state (e.g., pointer/Arc strong_count or a simple store id) and log an exit/return message ("scratch_store:exit") so callers can trace entry->branch->exit; target the function global_scratch_store, the static STORE OnceCell, and the ScratchTodoStore::new call when adding these debug statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/todos/store.rs`:
- Around line 23-29: The current snapshot() and replace() split the
read-modify-write across separate locks causing races; add an atomic mutation
method (e.g., pub fn mutate<F>(&self, f: F) where F: FnOnce(&mut
Vec<TaskBoardCard>)) that takes the lock once, applies the closure, and returns
any needed result, then refactor all scratch CRUD paths to call this mutate(...)
instead of snapshot()/replace() so load, modify and save happen inside a single
critical section (update uses of snapshot, replace, and direct .lock() mutations
to use the new mutate API and adjust callers to accept any return values the
closure produces).
In `@src/openhuman/tools/impl/agent/todo.rs`:
- Around line 157-162: The helper required_string should trim whitespace and
reject values that are empty after trimming to avoid false "not found" errors
for fields like "id" used by edit/remove/update_status; modify required_string
to extract the string as now, call .trim() on it, return
Ok(trimmed_string.to_string()) only if trimmed is non-empty, otherwise return an
anyhow::anyhow!("missing required field `{key}`") (or similar) so
leading/trailing whitespace is ignored and empty-after-trim values are treated
as missing.
---
Nitpick comments:
In `@src/openhuman/todos/store.rs`:
- Around line 34-39: Add structured debug logs to the global_scratch_store path:
when the function is called, log an entry message with a stable prefix (e.g.,
"scratch_store:enter"), then log whether you are initializing a new store or
reusing an existing one (detect in the closure passed to OnceCell::get_or_init
and the get path) with context like "scratch_store:init" vs
"scratch_store:reuse". Include any relevant identifiers or state (e.g.,
pointer/Arc strong_count or a simple store id) and log an exit/return message
("scratch_store:exit") so callers can trace entry->branch->exit; target the
function global_scratch_store, the static STORE OnceCell, and the
ScratchTodoStore::new call when adding these debug statements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 351d7240-9e24-4953-a359-f4f7f66548ce
📒 Files selected for processing (17)
AGENTS.mdCLAUDE.mdsrc/core/all.rssrc/openhuman/agent/profiles.rssrc/openhuman/agent/progress.rssrc/openhuman/agent/schemas.rssrc/openhuman/agent/task_board.rssrc/openhuman/mod.rssrc/openhuman/todos/mod.rssrc/openhuman/todos/ops.rssrc/openhuman/todos/schemas.rssrc/openhuman/todos/store.rssrc/openhuman/tools/impl/agent/mod.rssrc/openhuman/tools/impl/agent/todo.rssrc/openhuman/tools/impl/agent/todo_write.rssrc/openhuman/tools/ops.rssrc/openhuman/tools/ops_tests.rs
💤 Files with no reviewable changes (1)
- src/openhuman/tools/impl/agent/todo_write.rs
Address CodeRabbit review on tinyhumansai#1983: - `ops.rs`: take a process-global parking_lot mutex at the entry of every public CRUD op when the target is the scratch store, so the load → mutate → save sequence is atomic. Without this two concurrent scratch `add` / `edit` / `remove` calls could lose updates because `snapshot()` and `replace()` each take the inner lock independently. Per-thread ops are already atomic at the file-rename level via `TaskBoardStore::put`, so they don't take the new lock. - `tools/impl/agent/todo.rs`: trim required string args and reject empty-after-trim, so a tool call passing `" task-123 "` for `id` no longer triggers a false "not found" in `edit` / `remove` / `update_status`.
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
todoagent tool dispatching onop(add/edit/update_status/remove/replace/clear/list), replacing the old wholesaletodowritetool.openhuman.todos_*JSON-RPC surface so the desktop UI (or any other RPC caller) can drive the same per-thread task board.- [ ]/- [~]/- [!]/- [x]) bundled in every tool result + RPC response so transcripts and the kanban UI can render without re-formatting.CLAUDE.mdandAGENTS.mdrequiring all code edits to land on a branch forked frommain.Problem
The previous
todowritetool only supported wholesale list replacement, so agents had to resend the entire list to change one card and there was no way for users to nudge the list from the UI without rewriting it themselves. Output was also a plain-text checklist, which the chat UI had to re-parse to render. Two surfaces (agent tool + user RPC) needed parity so a user-driven status flip and an agent-driven status flip touch the same state.Solution
src/openhuman/todos/domain withops.rs(CRUD + markdown renderer),store.rs(process-global scratch fallback when no thread context exists), andschemas.rs(controller registry). All CRUD goes through one set of pure ops that load → mutate → persist via the existing per-threadTaskBoardStore, so agent and user edits share identical persistence and rendering.openhuman.todos_*controllers:list,add,edit,update_status,remove,replace,clear. Wired intosrc/core/all.rsfor both schemas and registered controllers.src/openhuman/tools/impl/agent/todo.rsexposes a singletodotool that branches onop. Returns{threadId, cards, markdown}JSON so the orchestrator gets a ready-to-render markdown payload alongside the structured cards. Falls back to the process-global scratch store when invoked outside a chat thread.allowed_tools: None, so the newtodotool surfaces automatically. TheWriterprofile fixture and theall_toolsregistry test were updated from"todowrite"to"todo".scratch_test_lock()so cargo's parallel runner doesn't race on the process-global scratch store.Submission Checklist
pnpm test:coverage/pnpm test:rustnot run locally in this environment; tests added cover happy path + failure cases for every CRUD op and tool dispatch branch.Impact
openhuman.todos_*RPC without rewriting the whole list. No behavior change for already-completed boards (data on disk is unchanged — sameTaskBoardCardshape).todowritetool is removed in favor oftodo. Any external profile referencingtodowriteby name needs to be updated totodo.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check(auto-fixes applied by pre-push hook and committed aschore: apply auto-fixes)pnpm typecheck(no TS changes in this PR)cargo test --lib -- openhuman::todos openhuman::tools::implementations::agent::todo openhuman::agent::profiles— 53 + 16 passedcargo fmt+GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml --lib— cleanValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
todowritewith granulartodoCRUD tool + matching JSON-RPC.Parity Contract
op: replacereproduces the oldtodowritewholesale-replace semantics, persisted via the sameTaskBoardStore.allowed_tools: None); profile fixture andall_toolsregistry test updated fromtodowritetotodo.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests