Surface explicit task status and attention on background tasks#1178
Merged
chumyin merged 4 commits intoeastreams:devfrom Apr 16, 2026
Merged
Surface explicit task status and attention on background tasks#1178chumyin merged 4 commits intoeastreams:devfrom
chumyin merged 4 commits intoeastreams:devfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a derived Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (loong tasks)
participant Daemon as Daemon / tasks_cli
participant Builder as TaskStatusBuilder
participant Session as Session Runtime
participant Delegate as Delegate
participant Approvals as Approval Service
CLI->>Daemon: request (list/status/create/wait)
Daemon->>Builder: build_task_status_payload(session, delegate, approvals, events, policy)
Builder->>Session: read session state
Builder->>Delegate: read delegate phase
Builder->>Approvals: read approval/attention summary
Builder-->>Daemon: task_status payload (kind,status,display,flags,next_action,signals)
Daemon-->>CLI: JSON + rendered text (includes task_status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/daemon/src/tasks_cli.rs (1)
1212-1220: Nit: Simplify intermediate variable.The function is correct but could be slightly more concise.
♻️ Optional simplification
fn render_task_status_display(kind: &str, recovered: bool) -> String { let base = kind.to_owned(); if !recovered { return base; } - let display = format!("{base} (recovered)"); - display + format!("{base} (recovered)") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/src/tasks_cli.rs` around lines 1212 - 1220, The function render_task_status_display uses an unnecessary intermediate variable and explicit return; simplify by returning kind.to_owned() when not recovered and otherwise returning format!("{kind} (recovered)"), removing the `base` and `display` temporaries; update references inside render_task_status_display to use kind and recovered directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/daemon/src/onboard_web_search.rs`:
- Around line 610-611: The test's hermetic env cleanup is incomplete because
clear_web_search_credential_envs currently only unsets api_key_env_names but not
providers' default_api_key_env, allowing host credentials to leak; update
clear_web_search_credential_envs (or add a helper it calls) to also unset each
provider's default_api_key_env in addition to api_key_env_names, ensuring
ScopedEnv::new() + clear_web_search_credential_envs(&mut env) removes both
api_key_env_names and default_api_key_env entries for every WebSearch provider.
---
Nitpick comments:
In `@crates/daemon/src/tasks_cli.rs`:
- Around line 1212-1220: The function render_task_status_display uses an
unnecessary intermediate variable and explicit return; simplify by returning
kind.to_owned() when not recovered and otherwise returning format!("{kind}
(recovered)"), removing the `base` and `display` temporaries; update references
inside render_task_status_display to use kind and recovered directly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b19ebd29-822a-4b9a-b0b8-239f9fb1e7bc
📒 Files selected for processing (4)
crates/daemon/src/onboard_web_search.rscrates/daemon/src/tasks_cli.rscrates/daemon/tests/integration/tasks_cli.rsdocs/product-specs/background-tasks.md
Collaborator
Author
|
Follow-up pushed in What changed in this pass:
Fresh validation on the updated branch:
Notes:
|
…on fields The task surface already had truthful child-session, approval, and tool-policy facts, but still forced operators to reconstruct effective status by hand. This change derives a stable summary from the existing runtime facts and renders it directly in the CLI surface. While validating the full local gate, an onboarding web-search test proved to be environment-sensitive under real operator credential env vars. The test now clears credential envs explicitly so the configured-provider path stays hermetic during all-features verification. Constraint: Must stay truthful to the existing child-session runtime and avoid inventing a second task state model Rejected: Add a new durable task-state machine | would drift from session truth and broaden the change Rejected: Document raw fields more clearly | would leave the operator inference gap in place Confidence: high Scope-risk: narrow Directive: Keep task_status derived from existing session, approval, and tool-policy facts unless the runtime model itself changes Tested: cargo fmt --all -- --check; cargo clippy --workspace --all-targets --all-features -- -D warnings; cargo test --workspace --locked -q; cargo test --workspace --all-features --locked -q; cargo test -p loongclaw tasks_cli::tests:: --lib -- --test-threads=1; cargo test -p loongclaw --test integration execute_tasks_command_list_returns_visible_background_tasks -- --test-threads=1; cargo test -p loongclaw --test integration execute_tasks_command_status_surfaces_approval_and_tool_policy -- --test-threads=1; cargo test -p loongclaw --test integration execute_tasks_command_create_queues_background_task_and_surfaces_follow_up_recipes -- --test-threads=1; cargo test -p loongclaw --test integration execute_tasks_command_events_and_wait_surface_incremental_payloads -- --test-threads=1; cargo test -p loongclaw resolve_web_search_provider_recommendation_keeps_explicitly_configured_default_provider --lib -- --test-threads=1; cargo test -p loongclaw-app delegate_announce_queue_batches_children_completed_within_debounce_window --lib -- --test-threads=1; cargo test -p loongclaw-app reopening_current_schema_db_skips_metadata_repairs --lib -- --test-threads=1; ./scripts/check-docs.sh; ./scripts/check_architecture_boundaries.sh; ./scripts/check_dep_graph.sh Not-tested: Manual operator walkthrough against a live long-running background task
…ion window The create-path task surface intentionally reports the latest child-session truth. On CI, detached workers can fail quickly enough that the freshly hydrated task is already `failed` instead of still `queued`, which made the integration test assert a scheduler fiction instead of the real runtime state. This narrows the test to the product contract: create must surface a derived status and next action that match the current payload, whether the child is still queued or has already failed. Constraint: The task surface must stay truthful to current child-session state instead of freezing a synthetic queued snapshot for tests Rejected: Force tasks create to always report queued | would regress runtime truth and hide fast child failures Rejected: Add sleeps or polling to wait for a specific state | would keep the test race-prone and slower across CI Confidence: high Scope-risk: narrow Directive: When detached child execution can race inspection, assert the task payload/rendering contract instead of a single transient state Tested: cargo fmt --all -- --check; cargo test -p loongclaw --test integration execute_tasks_command_create_queues_background_task_and_surfaces_follow_up_recipes -- --test-threads=1; CARGO_TARGET_DIR=/Users/chum/loongclaw/.cargo-target-shared cargo test -p loongclaw --test integration execute_tasks_command_create_queues_background_task_and_surfaces_follow_up_recipes -- --test-threads=1; CARGO_TARGET_DIR=/Users/chum/loongclaw/.cargo-target-shared cargo test --workspace --locked -q; CARGO_TARGET_DIR=/Users/chum/loongclaw/.cargo-target-shared cargo test --workspace --all-features --locked -q Not-tested: Manual Windows detached-worker timing reproduction outside GitHub Actions
The task-status branch was already corrected for detached task create races, but CodeRabbit surfaced one remaining test-only gap: the onboarding web-search helper cleared provider alias env names without clearing each provider's canonical default credential env. That meant local host credentials could still leak into the explicit-default-provider test and make CI or maintainer reruns less trustworthy. The fix extends the existing scoped cleanup helper instead of introducing new test-only branching. Constraint: The onboarding recommendation test must stay hermetic even when maintainers have provider credentials in their shell Rejected: Add provider-specific hardcoded removals inside each test | duplicates the descriptor contract and drifts as providers evolve Confidence: high Scope-risk: narrow Directive: Keep test env cleanup descriptor-driven; if web-search provider metadata changes, update the shared cleanup helper rather than individual tests Tested: cargo fmt --all -- --check Tested: cargo clippy --workspace --all-targets --all-features -- -D warnings Tested: cargo test --workspace --locked -q Tested: cargo test --workspace --all-features --locked -q Tested: cargo test -p loongclaw resolve_web_search_provider_recommendation_keeps_explicitly_configured_default_provider --lib -- --test-threads=1 Tested: cargo test -p loongclaw --test integration execute_tasks_command_create_queues_background_task_and_surfaces_follow_up_recipes -- --nocapture Tested: ./scripts/check_architecture_boundaries.sh Tested: ./scripts/check_dep_graph.sh Not-tested: GitHub Actions rerun after push (local CI-parity passed)
chumyin
force-pushed
the
feat/task-status-surface-20260410
branch
from
a16eee1 to
282e975
Compare
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
loong tasksalready exposed truthful child-session, approval, and tool-policy facts, but operators still had to reconstruct effective task state from rawstate,phase, approval counts, and tool policy details.task_statussummary from existing task/session factstask_status.status,task_status.needs_attention,task_status.next_action, andtask_status.signalsin the machine-readable payloadtasks create,tasks list,tasks status, andtasks waitLinked Issues
Change Type
Touched Areas
Risk Track
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --lockedcargo test --workspace --all-features --lockedCommands and evidence:
User-visible / Operator-visible Changes
loong tasks create|list|status|waitnow surface an explicit derived task summary instead of leaving operators to infer the task state from raw session fields.task_status.status,task_status.needs_attention,task_status.next_action, andtask_status.signals.Failure Recovery
state/phasetask_statuson create/list/status/wait surfacesReviewer Focus
crates/daemon/src/tasks_cli.rscompleted/failed/timed_outvs overdue vs approval-pending vs queued/running)needs_attention,next_action, andsignalsstay truthful to existing session/approval/tool-policy factscrates/daemon/tests/integration/tasks_cli.rscrates/daemon/src/onboard_web_search.rsSummary by CodeRabbit
New Features
Tests
Documentation