Enrich agent task board workflow#2891
Conversation
|
Warning Review limit reached
More reviews will be available in 12 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (40)
📝 WalkthroughWalkthroughAdds rich task-card metadata and per-card approval mode, implements persistence/normalization and RPC/schema changes, provides a TaskBrief editor and single-card update flow, and introduces a require-task-plan-approval autonomy setting with UI, i18n, and tests. ChangesTask Board Brief Metadata and Approval Management
Sequence Diagram(s)sequenceDiagram
participant BoardUI as TaskKanbanBoard / TaskBriefDialog
participant Conversations as Conversations.tsx
participant RPC as JSON-RPC Server (todos/threads)
participant TodoOps as todos ops
participant Store as TaskBoard store
BoardUI->>Conversations: onUpdateCard(card, nextCard)
Conversations->>RPC: threads.task_board_put (updated cards)
RPC->>TodoOps: handle edit/add (CardPatch)
TodoOps->>Store: store.put / normalise_board (trim/filter fields)
Store->>RPC: persisted board
RPC->>Conversations: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/openhuman/config/ops.rs (1)
870-872: ⚡ Quick winAdd a debug log for the new autonomy state transition.
This new persisted toggle updates a security-relevant setting but emits no diagnostic line.
As per coding guidelines "Rust code: use `log` / `tracing` crate for logging at `debug` or `trace` level; add substantial, development-oriented logs on new/changed flows at ... state transitions ...".Suggested patch
if let Some(require_task_plan_approval) = update.require_task_plan_approval { config.autonomy.require_task_plan_approval = require_task_plan_approval; + tracing::debug!( + require_task_plan_approval, + "[config][autonomy] require_task_plan_approval updated" + ); }🤖 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/config/ops.rs` around lines 870 - 872, When update.require_task_plan_approval is applied to config.autonomy.require_task_plan_approval, add a debug/tracing log that records the state transition and includes both the previous value and the new value; locate the block handling update.require_task_plan_approval (the if let Some(require_task_plan_approval) = update.require_task_plan_approval { ... } that assigns config.autonomy.require_task_plan_approval) and emit a debug message (using the project’s logging crate, e.g. log::debug! or tracing::debug!) before or after the assignment that prints the old value, the new require_task_plan_approval, and a short context string like "autonomy.require_task_plan_approval updated".
🤖 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 `@app/src/components/settings/panels/AgentAccessPanel.tsx`:
- Around line 266-281: The checkbox label and description in AgentAccessPanel
(the JSX block using requireTaskPlanApproval and toggleTaskPlanApproval) are
hard-coded; switch them to use the i18n hook useT() from
app/src/lib/i18n/I18nContext (call useT() at the top of AgentAccessPanel) and
replace the two literal strings ("Require task plan approval" and "Pause before
an assigned agent executes an agent-authored task brief.") with t('...') keys
(e.g. agentAccess.requireTaskPlanApproval and
agentAccess.requireTaskPlanApprovalDescription or similar). Also add those two
keys and translations to the locale resource files so the strings are available
for all locales.
In `@app/src/pages/Conversations.tsx`:
- Line 1145: Replace the hard-coded advisory passed to setSendAdvisory with a
localized string via the i18n hook: import and call useT() from
app/src/lib/i18n/I18nContext (or use the existing t() in this component) and
change setSendAdvisory('Could not update task; changes were not saved.') to
setSendAdvisory(t('Could not update task; changes were not saved.')) (or
preferably use a proper i18n key like t('conversations.updateTaskFailure') and
add that key to the locale files); ensure useT() is imported and t is available
in the Conversations component scope before updating the call.
In `@app/src/pages/conversations/components/TaskKanbanBoard.tsx`:
- Around line 139-140: The TaskKanbanBoard component contains many hard-coded,
user-visible strings (badges like the approval text, dialog headings, labels and
button text) that must be localized: import and call useT() from
app/src/lib/i18n/I18nContext inside the TaskKanbanBoard function, replace
literals such as the approval badge (e.g. the expression using
card.approvalMode), the dialog headings, labels and button captions referenced
around the flagged lines with t('namespace.key') calls, and add corresponding
keys to the locale resource files; ensure you use descriptive keys (e.g.
taskKanban.approval.required, taskKanban.approval.none, taskKanban.dialog.title,
taskKanban.button.create, etc.) so all user-facing strings listed in
TaskKanbanBoard are replaced with t(...) usages.
In `@src/openhuman/todos/ops.rs`:
- Around line 66-68: The patch currently uses approval_mode:
Option<TaskApprovalMode> so edit code that checks patch.approval_mode (e.g., the
update block around lines 310-312) can only set a value and cannot clear it to
None; change the patch type to a tri-state Option<Option<TaskApprovalMode>>
(e.g., approval_mode: Option<Option<TaskApprovalMode>>) and update the update
logic to handle three cases: None => no change, Some(Some(value)) => set to
value, Some(None) => clear the field to None; adjust any uses of
TaskApprovalMode and patch.approval_mode checks accordingly to implement these
semantics.
---
Nitpick comments:
In `@src/openhuman/config/ops.rs`:
- Around line 870-872: When update.require_task_plan_approval is applied to
config.autonomy.require_task_plan_approval, add a debug/tracing log that records
the state transition and includes both the previous value and the new value;
locate the block handling update.require_task_plan_approval (the if let
Some(require_task_plan_approval) = update.require_task_plan_approval { ... }
that assigns config.autonomy.require_task_plan_approval) and emit a debug
message (using the project’s logging crate, e.g. log::debug! or tracing::debug!)
before or after the assignment that prints the old value, the new
require_task_plan_approval, and a short context string like
"autonomy.require_task_plan_approval updated".
🪄 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: c34e746c-0a09-445e-ae31-c50b99afc892
📒 Files selected for processing (15)
app/src/components/settings/panels/AgentAccessPanel.tsxapp/src/pages/Conversations.tsxapp/src/pages/conversations/components/TaskKanbanBoard.tsxapp/src/pages/conversations/components/__tests__/TaskKanbanBoard.test.tsxapp/src/types/turnState.tsapp/src/utils/tauriCommands/config.tssrc/openhuman/agent/task_board.rssrc/openhuman/config/ops.rssrc/openhuman/config/schema/autonomy.rssrc/openhuman/config/schemas.rssrc/openhuman/threads/turn_state/mirror_tests.rssrc/openhuman/todos/ops.rssrc/openhuman/todos/schemas.rssrc/openhuman/tools/impl/agent/todo.rstests/json_rpc_e2e.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel hey — the code looks solid to me, but CI is failing (Frontend Unit Tests + Frontend Coverage) so I'll hold off on approving until those are green. Fix those and I'll come back.
A few things I noticed while reading:
Approach is clean overall. The tri-state Option<Option<TaskApprovalMode>> for edit vs. the Some(parse_approval_mode(...)?).flatten() dance for add is a bit unconventional but correct — None.flatten() → None, Some(None).flatten() → None, Some(Some(x)).flatten() → Some(x) all give the right outcomes. The normalise_board coverage for the new string fields (trimming, retaining non-empty) is thorough. i18n coverage across all 13 locales is clean.
CodeRabbit dedup note: the CHANGES_REQUESTED from CR was on an earlier commit. The current code already uses t(...) for all user-visible strings in AgentAccessPanel, Conversations.tsx, and TaskKanbanBoard.tsx, and the approval_mode tri-state is implemented in approval_mode_patch_from_params. Those findings are addressed.
[major] CI failures need investigation before merge.
test / Frontend Unit Tests and Frontend Coverage (Vitest) are both failing. The new TaskKanbanBoard.test.tsx tests use getByText('Task brief') and getByLabelText('Title'), which depend on the i18n test setup returning actual translation values rather than keys. If the CI environment mocks useT() to return keys, these calls will fail. The PR description mentions a AgentMessageBubble import that breaks compile in the local checkout — if that also breaks in CI test runs, it could cascade. Check the CI logs to confirm whether the failure is pre-existing or introduced by this PR.
[minor] TaskBriefDialog is missing ARIA dialog semantics.
The overlay renders a <section> without role="dialog", aria-modal="true", or aria-labelledby. Screen readers will not identify it as a modal dialog. Standard fix:
<section
role="dialog"
aria-modal="true"
aria-labelledby="task-brief-title"
className="..."
>and id="task-brief-title" on the <h3>. Also missing: Escape key handler to close and focus trap on open.
[minor] AgentAccessPanel toggle not covered by an updated test.
The PR description references AutonomyPanel.test.tsx as a focused test but it's not in the diff. The persist call signature now always sends requireTaskPlanApproval alongside every other setting change (workspace_only, trusted_roots, auto_approve removals). Any existing test that asserts the exact shape of the persist payload will start failing silently. Worth adding a targeted test that toggles the new checkbox and verifies the value is included in the persisted update.
AI risk assessment:
- What it does: Extends task board cards with agent execution metadata (objective, plan, assigned agent, allowed tools, approval mode, acceptance criteria, evidence) and adds a require-plan-approval autonomy setting. Wires these through Rust core, todo RPC, thread task-board RPCs, and surfaces them in a conversation UI task brief dialog.
- Breaking risk: Low. All new fields use
#[serde(default)]so existing persisted boards deserialize cleanly. Frontend reads use optional chaining. The autonomy config defaults torequire_task_plan_approval = trueon upgrade, which is behavior-changing but intentional. - Security risk: Zero. No auth surface changes. Task brief fields are plain text stored via existing channels; no eval or deserialization of untrusted code.
- Bottom line: Safe to merge — once CI is green.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel merge from main looks clean — no conflicts or regressions from the upstream pull. The code is still in good shape from the previous review.
Still waiting on CI to go fully green before approving: Frontend Unit Tests, Frontend Coverage (Vitest), Build Tauri App, E2E, and coverage jobs are pending. Once those come back clean, I'll approve.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel the code still looks good — the new commit covering the UI update tests is a nice addition and addresses the coverage gap I flagged. All four CodeRabbit threads are resolved. CI is still running though (Frontend Unit Tests, Rust Core Tests, Build Tauri, E2E, and coverage jobs all pending). Once those finish green, I'll come back and approve this.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
CodeRabbit-style Review — PR #2891 (Kanban Enrichment)
Overall: Clean, backward-compatible implementation. All new fields properly optional with #[serde(default)]. 3 major, 2 minor, 4 nitpicks.
Verified / Looks Good
TaskApprovalModeserializes assnake_case, matching TS types exactlyTaskBoardCarduses#[serde(rename_all = "camelCase")]— wire names match TS interface- All 7 new fields have
skip_serializing_if— fully backward-compatible normalise_boardcorrectly trims objective, assigned_agent, and all vec fieldsrequire_task_plan_approvalcorrectly wired: config → patch → TOML save → live policy reload → event bus- All 26 locale chunk files have new keys
cargo checkpasses,cargo fmt --checkpasses- Both E2E tests (brief roundtrip + autonomy settings) exercise the full RPC path
AgentAccessPanel.test.tsxcorrectly disambiguates checkboxes
Questions
approval_modeauto-population only fires onadd, notedit. Is "inherit at creation time" the intended semantic? A comment would prevent future confusion.render_markdownfor new sub-fields lacks a focused unit test — consider adding one to pin the format.
There was a problem hiding this comment.
💡 Minor + Nitpicks
Minor 4: TaskBriefDialog rendered inline without a portal. fixed inset-0 can be clipped by a transform-bearing ancestor. Consider createPortal(dialog, document.body) for safety.
Minor 5: render_markdown doesn't escape field values — a plan step with **bold** or backticks corrupts the markdown output. Low-urgency for internal tooling but worth noting.
Nitpicks:
schemas.rs—string_array_inputdescriptions forplan/evidenceare slightly inconsistent withallowed_tools/assigned_agententries.TaskKanbanBoard.tsx—TaskBriefDialogandTaskKanbanBoardlack JSDoc comments for their non-trivial prop surfaces.task_board.rs—TaskApprovalMode::as_str()is unused outside tests. ConsiderDisplayderive.TaskKanbanBoard.test.tsx— Labels lackhtmlFor/idpairs; screen readers relying on explicit association won't announce them.
…, i18n
- Use load_config_with_timeout() instead of Config::load_or_init() in
default_task_approval_mode() to respect the 30s disk-I/O stall guard
that every other hot-path config read uses.
- Change BriefList's <li key={value}> to <li key={index}> so duplicate
plan steps (e.g. two "Write tests" entries) don't produce React
duplicate-key warnings and incorrect reconciliation.
- Replace hardcoded 'Could not move task; changes were not saved.' in
handleMoveTaskCard with t('conversations.taskKanban.updateFailed'),
matching the already-correct handleUpdateTaskCard below it.
…ring
The test was asserting the old hardcoded literal 'Could not move task;
changes were not saved.' After fixing handleMoveTaskCard to use
t('conversations.taskKanban.updateFailed'), the rendered text is
'Could not update task; changes were not saved.' — update the assertion
to match.
…tion Wrap the long getByText() call to match Prettier's 100-char print width, consistent with the adjacent edit-persistence test.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel all three major findings from my earlier review are addressed — nice clean fixes in 0a9d3bd:
default_task_approval_modenow usesload_config_with_timeout()instead of the directConfig::load_or_init()callBriefListkeys switched from content strings to index- Both
handleMoveTaskCardandhandleUpdateTaskCarduse the i18n string; test assertion updated to match
The one open minor (TaskBriefDialog rendered inline rather than via a portal, which can be clipped by transform ancestors) is non-blocking and can be addressed as a follow-up.
Code is clean. Waiting on CI — Build, TypeCheck, Rust Quality, and E2E are still pending. Once those are green I'll approve.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
pr-manager: all 3 major findings addressed and CI is fully green.
Fixed:
default_task_approval_mode()insrc/openhuman/tools/impl/agent/todo.rs: switched fromConfig::load_or_init().awaittocrate::openhuman::config::ops::load_config_with_timeout().awaitto respect the 30s disk-I/O stall guard used on every other hot-path config read.BriefListinapp/src/pages/conversations/components/TaskKanbanBoard.tsx: changed<li key={value}>to<li key={index}>to avoid duplicate-key warnings on repeated plan steps.handleMoveTaskCardinapp/src/pages/Conversations.tsx: replaced the hardcoded literal witht('conversations.taskKanban.updateFailed'), matchinghandleUpdateTaskCard.- Updated the corresponding test assertion in
Conversations.render.test.tsxto match the i18n-resolved string and fixed Prettier line wrapping.
All CI checks pass.
2 participants
Summary
Problem
The current task board behaves like a basic Kanban board, which leaves agents without enough structured context when picking up work. Tasks need to carry execution-oriented metadata so an agent can understand the plan, ownership, approved tools, acceptance criteria, and evidence expectations without reverse-engineering intent from a short title.
Solution
The implementation expands the shared task card model in Rust and TypeScript, wires the new fields through todo operations, controller schemas, agent todo tooling, task-board mirroring, and thread task-board RPCs, then surfaces editable controls in the conversation task board UI. It also adds an autonomy config flag so users can opt into requiring plan approval before agent execution.
Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. CI coverage jobs are running; local full coverage was not run due checkout dependency blockers noted below.## RelatedCloses #NNNin the## Relatedsection — no issue was provided.Impact
Desktop app and core JSON-RPC behavior change. Existing simple Kanban fields remain compatible; new fields are optional and default to empty/automatic values. No new external services or network dependencies are introduced.
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
agent-task-brief-workflow54769f0e6d390785d05f8cff3d43c4c5dffb18faValidation Run
pnpm --filter openhuman-app format:checkvia pre-push hookpnpm typecheckblocked locally; see Validation Blocked. CI Type Check TypeScript is running on the GitHub runner.pnpm debug unit src/pages/conversations/components/__tests__/TaskKanbanBoard.test.tsxpnpm debug unit src/pages/conversations/components/__tests__/TaskKanbanBoard.test.tsx src/services/api/threadApi.test.ts src/components/settings/panels/__tests__/AutonomyPanel.test.tsxGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml task_board -- --nocaptureGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml todo -- --nocaptureGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml apply_autonomy_settings -- --nocaptureGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --test json_rpc_e2e json_rpc_task_board_brief_roundtrips_across_todos_and_threads_rpc -- --nocaptureGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --test json_rpc_e2e json_rpc_config_autonomy_settings_roundtrip -- --nocapturecargo fmt --manifest-path ../Cargo.toml --all --checkvia pre-push hook;cargo check --manifest-path src-tauri/Cargo.tomlvia pre-push hook completed with existing warnings.cargo fmt --manifest-path src-tauri/Cargo.toml --all --checkandcargo check --manifest-path src-tauri/Cargo.tomlvia pre-push hook.Validation Blocked
command: pnpm typecheckerror: local compile fails because workspace dependencies/types are unavailable: recharts, qrcode.react, @rive-app/react-webgl2, @noble/ciphers/chacha, @noble/ciphers/webcrypto, rehype-katex, remark-math, @tauri-apps/plugin-barcode-scanner.impact: TypeScript full-project compile and one conversations render test that imports AgentMessageBubble cannot run in this checkout until those dependencies are present. Focused task board unit tests and Rust E2E coverage passed.Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit