feat(agent): task plan-approval gate + kanban approval UI (G8)#2969
Closed
sanil-23 wants to merge 4 commits into
Closed
feat(agent): task plan-approval gate + kanban approval UI (G8)#2969sanil-23 wants to merge 4 commits into
sanil-23 wants to merge 4 commits into
Conversation
…metadata Task sources previously created "dumb" cards that set only `notes`, leaving tinyhumansai#2891's enriched brief fields empty even though `enrich.rs` already computes a summary, urgency, and an actionable prompt. - Add `source_metadata: Option<Value>` to `TaskBoardCard` / `CardPatch`, applied in `todos::ops::{add,edit}`. - Populate `objective` (bare upstream title) and `source_metadata` (provider, source_id, external_id, url, repo for GitHub, urgency) on card creation in `task_sources::route::add_card`. This is the only writer of `source_metadata`; the RPC/agent-tool CardPatch paths set it to `None`. - Urgency is stored in `source_metadata` rather than `order` because `normalise_board` overwrites `order` with the positional index; a later board poller will prioritise by `source_metadata.urgency`. - TS `TaskBoardCard` gains an optional `sourceMetadata` field for parity. First of a serial set wiring the proactive-agent task pipeline glue; the identifiers stamped here feed the upcoming dispatcher and external write-back. Co-Authored-By: Claude <noreply@anthropic.com>
… write-back
Wires the proactive task pipeline so enriched cards actually run and the
board reflects the outcome. One executor, two feeders, deduped by a claim.
- New `agent/task_dispatcher.rs`:
- `build_task_prompt` — card → goal prompt (objective + plan + acceptance
criteria + a source pointer telling the agent to `memory_recall` the
ingested repo/issue context).
- `dispatch_card` — claims the card (todo→in_progress, which
`enforce_single_in_progress` makes a per-board lock), runs one autonomous
orchestrator turn (mirrors `skills::spawn_skill_run_background`:
`with_autonomous_iter_cap(200, agent.run_single(..))`), then writes back
`done` + evidence / `blocked` + reason. Detached; returns a run id.
- Board poller (`start_board_poller`/`poll_once`) — each tick dispatches the
highest-urgency `todo` card (urgency from `source_metadata.urgency`), gated
by `scheduler_gate` capacity. Catch-all for cards without a proactive
trigger.
- Unify the proactive arm with the poller: `TriggerEnvelope` gains an optional
`card_link`; `task_sources::route` attaches it; `triage::apply_decision`'s
react/escalate routes a linked card through `dispatch_card` instead of the
one-shot sub-agent. The claim deduplicates against the poller, so both
feeders are safe.
- Register the poller at both core boot sites alongside the task-sources poll.
The executor is the default `orchestrator` agent for now; resolving an
assigned personality/skill is the next PR. Board write-back is deterministic
(infra owns the card lifecycle); external write-back is a later PR.
Co-Authored-By: Claude <noreply@anthropic.com>
Wire autonomy.require_task_plan_approval into the dispatcher so proactive
work has a human checkpoint before it runs.
- Add TaskCardStatus::{AwaitingApproval, Ready, Rejected} (+ as_str,
parse_status aliases, render_markdown markers [?]/[ ]/[-]).
- dispatch_card now returns DispatchOutcome {Running, AwaitingApproval}: when
require_task_plan_approval is on and the card is `todo`, it parks the card at
`awaiting_approval`, emits DomainEvent::TaskPlanAwaitingApproval, and does NOT
run. `ready` (approved) cards bypass the gate; the poller picks todo|ready.
- New openhuman.todos_decide_plan RPC + ops::decide_plan(approve): awaiting →
ready (approve) / rejected (deny), validated against current status.
- Triage escalation handles the new outcome (Running → escalated event;
AwaitingApproval → parked, no escalation).
- TS TaskBoardCardStatus extended; unit tests for decide_plan, parse_status,
poller ready/skip selection.
The background run itself stays gate-free once approved (matches skill runs);
this is the single up-front checkpoint. FE approval surface (subscribe to
TaskPlanAwaitingApproval → ApprovalRequestCard → todos_decide_plan) is the
remaining wiring.
Co-Authored-By: Claude <noreply@anthropic.com>
Completes the plan-approval UX. Cards parked at `awaiting_approval` now surface inline on the task board with Approve / Reject buttons: - TaskKanbanBoard buckets the approval-flow statuses into existing columns (awaiting_approval/ready → To do, rejected → Blocked) so they're visible without widening the grid; an `awaiting_approval` card renders Approve/Reject (reusing chat.approval.* labels) instead of the move arrows. - threadApi.decidePlan calls openhuman.todos_decide_plan and rebuilds the board from the returned snapshot. - Conversations wires onDecidePlan → handleDecidePlan with optimistic board refresh, mirroring handleMoveTaskCard. Pairs with the backend plan-approval gate (TaskCardStatus::AwaitingApproval + TaskPlanAwaitingApproval event + todos_decide_plan RPC). Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR implements a human-in-the-loop task plan approval workflow. Cards flow from ingestion with source metadata through optional approval gating to autonomous execution, with fallback polling dispatch for high-urgency unclaimed cards and UI controls for human approval/rejection decisions. ChangesTask Plan Approval Workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
7 tasks
coderabbitai
Bot
added
feature
![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)
app/src/services/api/threadApi.ts (1)
170-188: ⚡ Quick winAdd namespaced debug diagnostics to
decidePlanrequest lifecycle.Please add dev-only logs (entry/success/failure) with stable context fields (
threadId,cardId,approve) to match the app logging standard for new flows.As per coding guidelines: “Use namespaced
debugfor diagnostics in app/src/ with dev-only detail; verbose logging required on new/changed flows.”🤖 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 `@app/src/services/api/threadApi.ts` around lines 170 - 188, Add namespaced dev-only debug logs around the decidePlan flow: at entry, on success, and on error. In the decidePlan async function, before calling callCoreRpc, emit a debug entry log (use the module namespace, e.g., debug('services:api:threadApi:decidePlan:entry')) with context {threadId, cardId, approve}; after unwrapEnvelope and before returning the TaskBoard emit a success debug log with the same stable fields plus resulting cards length or threadId; wrap the callCoreRpc/unwrapEnvelope in try/catch and on catch emit a failure debug log (same stable fields and the caught error) and rethrow or return null consistent with current behavior. Use decidePlan, callCoreRpc, and unwrapEnvelope identifiers to locate the spots to add these logs.
🤖 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/ops.rs`:
- Around line 364-386: The decide_plan function lacks debug telemetry; add
tracing::debug! logs with a grep-friendly prefix (e.g., "decide_plan:") and
correlation fields thread_id, id, approve, and the outcome or error.
Specifically, instrument decide_plan (around calls to load_cards, after finding
current, and before returning) to log entry with thread_id, id and approve, then
capture the Result from update_status and log success/failure (include the error
message when Err) before returning; reference the decide_plan, load_cards,
current (TaskCardStatus check) and update_status symbols when adding the logs.
- Around line 364-386: decide_plan currently reads the card (using load_cards
and checking current.status) and then calls update_status which reloads/saves
separately, allowing a race to change status between read and write; change the
flow so the read-check-and-write happen in one atomic mutation path: either
extend update_status (or add update_status_if_current) to accept an expected
current status (TaskCardStatus::AwaitingApproval) and the desired new_status,
perform a single load->compare->persist inside that function, and call that from
decide_plan (passing approve => Ready or Rejected and expected AwaitingApproval)
so the transition fails if the card changed concurrently, returning an error
Result<TodosSnapshot, String>.
---
Nitpick comments:
In `@app/src/services/api/threadApi.ts`:
- Around line 170-188: Add namespaced dev-only debug logs around the decidePlan
flow: at entry, on success, and on error. In the decidePlan async function,
before calling callCoreRpc, emit a debug entry log (use the module namespace,
e.g., debug('services:api:threadApi:decidePlan:entry')) with context {threadId,
cardId, approve}; after unwrapEnvelope and before returning the TaskBoard emit a
success debug log with the same stable fields plus resulting cards length or
threadId; wrap the callCoreRpc/unwrapEnvelope in try/catch and on catch emit a
failure debug log (same stable fields and the caught error) and rethrow or
return null consistent with current behavior. Use decidePlan, callCoreRpc, and
unwrapEnvelope identifiers to locate the spots to add these logs.
🪄 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: b40ba256-f1e9-429a-8bb5-e248aec17761
📒 Files selected for processing (18)
app/src/pages/Conversations.tsxapp/src/pages/conversations/components/TaskKanbanBoard.tsxapp/src/services/api/threadApi.tsapp/src/types/turnState.tssrc/core/event_bus/events.rssrc/core/jsonrpc.rssrc/openhuman/agent/mod.rssrc/openhuman/agent/task_board.rssrc/openhuman/agent/task_dispatcher.rssrc/openhuman/agent/triage/envelope.rssrc/openhuman/agent/triage/escalation.rssrc/openhuman/channels/runtime/startup.rssrc/openhuman/notifications/rpc.rssrc/openhuman/task_sources/route.rssrc/openhuman/threads/turn_state/mirror_tests.rssrc/openhuman/todos/ops.rssrc/openhuman/todos/schemas.rssrc/openhuman/tools/impl/agent/todo.rs
Comment on lines
+364
to
+386
| pub fn decide_plan( | ||
| location: &BoardLocation, | ||
| id: &str, | ||
| approve: bool, | ||
| ) -> Result<TodosSnapshot, String> { | ||
| let cards = load_cards(location)?; | ||
| let current = cards | ||
| .iter() | ||
| .find(|c| c.id == id) | ||
| .ok_or_else(|| format!("todo id '{id}' not found"))?; | ||
| if current.status != TaskCardStatus::AwaitingApproval { | ||
| return Err(format!( | ||
| "card '{id}' is not awaiting approval (status: {})", | ||
| current.status.as_str() | ||
| )); | ||
| } | ||
| let new_status = if approve { | ||
| TaskCardStatus::Ready | ||
| } else { | ||
| TaskCardStatus::Rejected | ||
| }; | ||
| update_status(location, id, new_status) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add debug logs for the new decide_plan flow.
This new path currently has no entry/decision diagnostics. Add tracing::debug! with stable prefix and correlation fields (thread_id, id, approve, outcome/error).
As per coding guidelines: “Use log / tracing at debug / trace levels for verbose diagnostics on new/changed flows in Rust” and include grep-friendly prefixes plus correlation fields.
🤖 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/ops.rs` around lines 364 - 386, The decide_plan function
lacks debug telemetry; add tracing::debug! logs with a grep-friendly prefix
(e.g., "decide_plan:") and correlation fields thread_id, id, approve, and the
outcome or error. Specifically, instrument decide_plan (around calls to
load_cards, after finding current, and before returning) to log entry with
thread_id, id and approve, then capture the Result from update_status and log
success/failure (include the error message when Err) before returning; reference
the decide_plan, load_cards, current (TaskCardStatus check) and update_status
symbols when adding the logs.
Make decide_plan status-check and transition atomic in one mutation path.
Line 369 reads the card status, then Line 385 calls update_status (which re-loads/re-saves). A concurrent edit between those steps can bypass the stale-decision guard and still transition a card that is no longer awaiting_approval.
💡 Suggested direction
pub fn decide_plan(
location: &BoardLocation,
id: &str,
approve: bool,
) -> Result<TodosSnapshot, String> {
- let cards = load_cards(location)?;
- let current = cards
- .iter()
- .find(|c| c.id == id)
- .ok_or_else(|| format!("todo id '{id}' not found"))?;
- if current.status != TaskCardStatus::AwaitingApproval {
- return Err(format!(
- "card '{id}' is not awaiting approval (status: {})",
- current.status.as_str()
- ));
- }
- let new_status = if approve {
- TaskCardStatus::Ready
- } else {
- TaskCardStatus::Rejected
- };
- update_status(location, id, new_status)
+ let _scratch_guard = maybe_scratch_lock(location);
+ let mut cards = load_cards(location)?;
+ let card = cards
+ .iter_mut()
+ .find(|c| c.id == id)
+ .ok_or_else(|| format!("todo id '{id}' not found"))?;
+ if card.status != TaskCardStatus::AwaitingApproval {
+ return Err(format!(
+ "card '{id}' is not awaiting approval (status: {})",
+ card.status.as_str()
+ ));
+ }
+ card.status = if approve {
+ TaskCardStatus::Ready
+ } else {
+ TaskCardStatus::Rejected
+ };
+ card.updated_at = Utc::now().to_rfc3339();
+ enforce_single_in_progress(&cards)?;
+ let cards = save_cards(location, cards)?;
+ emit_progress(location, &cards);
+ Ok(into_snapshot(location, cards))
}🤖 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/ops.rs` around lines 364 - 386, decide_plan currently
reads the card (using load_cards and checking current.status) and then calls
update_status which reloads/saves separately, allowing a race to change status
between read and write; change the flow so the read-check-and-write happen in
one atomic mutation path: either extend update_status (or add
update_status_if_current) to accept an expected current status
(TaskCardStatus::AwaitingApproval) and the desired new_status, perform a single
load->compare->persist inside that function, and call that from decide_plan
(passing approve => Ready or Rejected and expected AwaitingApproval) so the
transition fails if the card changed concurrently, returning an error
Result<TodosSnapshot, String>.
7 tasks
Contributor
Author
|
Superseded by #2974, which consolidates all six proactive-pipeline gap PRs into a single branch (7 ordered commits). Closing in favour of that. |
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
autonomy.require_task_plan_approval: atodocard is parked at a newawaiting_approvalstatus (emittingTaskPlanAwaitingApproval) instead of running;ready/approved cards bypass.openhuman.todos_decide_planRPC (ops::decide_plan) approves (→ready) or rejects (→rejected).awaiting_approvalcards with inline Approve/Reject (buckets the new statuses into existing columns; reuseschat.approval.*labels).Problem
The dispatcher (#2965) ran proactive work with no up-front human checkpoint, even though
require_task_plan_approvalexisted (read only at card creation, never at dispatch).Solution
dispatch_cardreturnsDispatchOutcome{Running, AwaitingApproval}; the gate parkstodocards when approval is required. The autonomous run itself stays gate-free once approved (matches skill runs) — this is the single checkpoint. AddsTaskCardStatus::{AwaitingApproval, Ready, Rejected}, the event, the RPC, and the board UI ()Submission Checklist
cargo testgreen locally.Related
Summary by CodeRabbit