refactor: Route async task mode through TaskExecutionService (#95)

[AndriiPasternak31]

Summary

Test-only follow-up to #320, which superseded the original chat.py refactor.

After #320 landed the unified async path with a different implementation (_run_async_task_with_persistence + slot_already_held=True), the production code changes originally on this branch were dropped during the second rebase. What remains on top of main is a small set of parity tests plus one shared helper that cover behaviors not exercised by TestAsyncModeUnifiedExecutor from #320.

Scope

Production code: none. (grep "_execute_task_background" src/ returns zero hits as of #320.)

Tests added (tests/test_parallel_task.py):

• TestAsyncSessionPersistence (3 tests) — async save_to_session=True creates user+assistant messages, explicit chat_session_id is honored, and chat_response_ready is broadcast (verified via session existence).
• TestAsyncCollaborationActivity (1 test) — async task with X-Source-Agent header creates and completes the collaboration activity end-to-end. Polls /api/activities/timeline with a bounded retry instead of a fixed sleep.
• TestAsyncSafetyNet (1 test) — execution record is created before the background task is spawned (no silent launch failure). Marked @pytest.mark.requires_agent.

Helper added (tests/testing_utils/assertions.py):

• poll_execution_until_done(...) — shared polling helper for execution terminal status, used by the five new tests above. Documented as @pytest.mark.slow-only.

Review feedback addressed

• time.sleep(1) magic number in TestAsyncCollaborationActivity → replaced with a bounded retry loop on the timeline endpoint (10s deadline, 0.5s interval).
• poll_execution_until_done busy-poll concern → docstring now flags it as @pytest.mark.slow-only and warns against use from non-slow tests.
• TestAsyncSafetyNet missing @pytest.mark.requires_agent → added.
• execution_time_ms regression and context_used guard from the earlier review are no longer applicable — those lived in the dropped chat.py changes; refactor(exec): route async /task through TaskExecutionService (#95) #320 handles both in TaskExecutionService.

Test plan

• pytest tests/test_parallel_task.py::TestAsyncSessionPersistence -v
• pytest tests/test_parallel_task.py::TestAsyncCollaborationActivity -v
• pytest tests/test_parallel_task.py::TestAsyncSafetyNet -v
• pytest tests/test_parallel_task.py::TestAsyncMode -v (regression — should still pass)

Follow-up to #95 (closed by #320).

[vybe]

Thanks for the clean refactor! The delegation pattern is solid and the 5 new tests are a great addition. A few items before merge:

Required

• execution_time_ms regression in _save_to_chat_session() — The old _execute_task_background() passed execution_time_ms to db.add_chat_message(). The new helper omits it because TaskExecutionResult has no such field. Async chat sessions will now always store NULL for execution time. Either add execution_time_ms: Optional[int] to TaskExecutionResult and populate it in the service, or explicitly accept this loss and document it.

• context_used guard dropped — Old: context_used if context_used > 0 else None. New: stores result.context_used directly (could persist 0 instead of NULL). Small behavior change worth preserving for consistency.

• Please rebase onto main before merge — the branch has a merge commit (merge: resolve changelog conflict with upstream/main) that should be cleaned up.

Minor

• Issue #95 is missing the type-refactor label.
• Confirm public.py's _execute_public_chat_background() (line 775) is considered resolved — it appears to already delegate to TaskExecutionService, but the issue's "Files to Change" section lists it. Either close the loop in the PR description or follow up in a separate issue.
• Add ## Security Considerations section to task-execution-service.md (even a brief "N/A — service layer, no direct auth or user input" is sufficient to satisfy the flow format spec).

Please address the required items and request re-review.

[AndriiPasternak31]

Addressed review feedback

Required items

1. execution_time_ms regression — Fixed. Added execution_time_ms: Optional[int] = None field to TaskExecutionResult dataclass, populated it in execute_task(), and passed it through _save_to_chat_session() to db.add_chat_message(). Async chat sessions will now correctly store execution time.

2. context_used guard — Verified. The service already applies the context_used if context_used > 0 else None guard (lines 287, 313 in task_execution_service.py) when building TaskExecutionResult, so result.context_used is already None when the raw value is 0. No code change needed.

3. Rebase onto main — Done. Rebased cleanly onto upstream/main, removed the merge commit. History is now linear with 5 clean commits.

Minor items

4. Issue Route async task mode through TaskExecutionService #95 type-refactor label — Unable to add (insufficient permissions on upstream). Please add manually.

5. public.py delegation status — Confirmed. _execute_public_chat_background() (line 699) already delegates to TaskExecutionService via task_execution_service.execute_task(). No follow-up needed.

6. Security Considerations section — Added to task-execution-service.md: "N/A — service layer, no direct auth or user input. Auth enforced by calling routers."

Additional improvements merged from main

• Preserved the _on_task_done callback from bug: Chat-triggered executions fail with 'Silent launch failure' despite agent being healthy #279 (surfaces unhandled asyncio exceptions)
• Passed subscription_id through to TaskExecutionService for SUB-004 usage tracking

[vybe]

Please rebase this branch onto main to resolve any conflicts and incorporate recent changes.

Once rebased, request re-review.

[AndriiPasternak31]

Rebased onto main. Since #320 landed the refactor with a different implementation (_run_async_task_with_persistence + slot_already_held=True), the original code changes in this PR are no longer applicable and were dropped during rebase. What remains on top of main:

b4de8d4 — test: add async task parity tests for issue #95

• TestAsyncSessionPersistence (3 tests): save_to_session creates user+assistant messages, explicit session_id is respected, WebSocket chat_response_ready fires.
• TestAsyncCollaborationActivity (1 test): agent-to-agent async task completes the collaboration activity end-to-end.
• TestAsyncSafetyNet (1 test): execution record is created before the background task is spawned (no silent launch failure).

These cover behaviors not exercised by the upstream TestAsyncModeUnifiedExecutor added in #320.

1f48311 — test: extract poll_execution_until_done helper

• Shared helper in tests/testing_utils/assertions.py removes duplicated polling loops across the 5 new tests above.

No production code changes. Dropped:

• Original _execute_task_background refactor — superseded by refactor(exec): route async /task through TaskExecutionService (#95) #320.
• docs/memory/feature-flows/task-execution-service.md edits — upstream already documents refactor(exec): route async /task through TaskExecutionService (#95) #320's approach.
• docs/memory/changelog.md entry — file was deprecated on main (git history is now the source of truth).

Let me know if you'd prefer this closed as superseded instead of merged as a test-only follow-up.

[dolho]

🤖 PR Reviewer (Trinity)

PR Review — refactor: Route async task mode through TaskExecutionService (#95)

Overall: Straightforward refactor with good intent. The diff is limited to test_parallel_task.py and testing_utils/assertions.py — notably the chat.py changes described in the PR summary are not present in the diff. This is the main concern.

Scope mismatch

The PR description says:

src/backend/routers/chat.py — Rewrote _execute_task_background() as thin wrapper, simplified async branch, extracted shared helper

But chat.py does not appear in the diff at all. The diff adds 228 lines of tests and a poll_execution_until_done helper but contains zero changes to chat.py. Either:

1. The chat.py changes were not committed/pushed, or
2. The PR description is inaccurate and the actual refactor is elsewhere.

This should be resolved before merge. If the core refactor of chat.py is missing, the tests are testing behavior that hasn't changed yet; if it was intentionally omitted, the PR title and description need updating.

Test-only concerns

poll_execution_until_done busy-polls with time.sleep(2.0) (assertions.py:264-273): This is a blocking sleep in what may be a synchronous test context. For slow tests marked @pytest.mark.slow this is acceptable, but the utility function is now in testing_utils/assertions.py making it reusable in non-slow tests. Consider max_wait defaulting to something tighter or add a note that it is for @pytest.mark.slow contexts only.

TestAsyncCollaborationActivity.test_async_collaboration_activity_completed (test_parallel_task.py:191): Uses time.sleep(1) unconditionally after poll_execution_until_done. This is fragile — if the activity write is slow the assertion will still fail. The comment says "Brief wait for activity completion" but this is a magic number with no retry. Consider polling the activities endpoint with a timeout instead.

Missing import time (test_parallel_task.py:191): time.sleep(1) is called but time is not imported at the top of the class/method. There is a time import inside poll_execution_until_done (assertions.py:262) but not in the test file itself. This will raise NameError at runtime.

No functional/security concerns

The added tests are read-only API interactions (GET sessions, GET activities, GET executions). No new write paths, no injection risks.

[AndriiPasternak31]

Pushed d111205 and 91bddd7 after running the suite live against a real backend (http://localhost:8000). Two test bugs that static review couldn't catch:

d111205 — /api/agents/{name}/chat/sessions response shape misuse

The endpoint returns {agent_name, session_count, sessions: [...]}, not a bare list. Two of the three TestAsyncSessionPersistence tests treated it as a bare list:

• test_async_save_to_session_creates_chat_messages was a false positive — iterating the dict yielded 3 keys (the dict's own field names), so len(sessions) > 0 was always 3 > 0. The assertion was structurally meaningless and would have passed against a backend returning zero sessions. This is the worst kind of test bug — green CI with no signal.
• test_async_save_to_session_with_explicit_session_id raised AttributeError calling .get('id') on a string (a dict key) instead of a session dict.

Both now extract sessions_resp.json()['sessions'] before iterating. (Audited the rest of the suite — only test_agent_chat.py:188-203 has a similar latent false-positive pattern, but it's pre-existing and out of scope here.)

91bddd7 — X-Source-Agent must be a real, distinct agent

TestAsyncCollaborationActivity.test_async_collaboration_activity_completed originally used the literal 'test-source-agent' as the source. Two compounding production behaviors made the test unable to ever observe the activity it created:

1. /api/activities/timeline filters activities by agent_name ∈ accessible_agents (routers/activities.py:47-50) — non-existent sources are silently hidden.
2. Even if the source name resolved, using the same name as the target would trip the SELF-EXEC-001 short-circuit in chat.py:790 and write activity_type=self_task instead of agent_collaboration — different bucket, the filter wouldn't match.

The test now picks the first real agent from /api/agents that isn't the target, with pytest.skip when only one agent is available. Both production constraints are documented inline so this doesn't get re-broken.

Live result: 5 passed in 32.40s against the unified-async path that #320 shipped.

[vybe]

⚠️ Wrong base branch — this PR targets main directly but all feature work must land in dev first per our branching model (feature/* → dev → main).

Please either:

• Rebase this branch onto dev and update the base branch, or
• Close this PR and open a new one targeting dev

See docs/DEVELOPMENT_WORKFLOW.md for the full SDLC.

[vybe]

Hey @AndriiPasternak31 — this PR is targeting main directly, and it looks like it's also been open since March 25. Per our dev workflow, all branches should target dev (feature/* → dev → main). Could you rebase onto dev and check for conflicts with what's landed in the last month? Happy to help review once it's retargeted.

[AndriiPasternak31]

Closing as superseded by #320.

After the second rebase, this branch carried only test-only follow-up against the unified async path that #320 shipped. On reflection:

• refactor(exec): route async /task through TaskExecutionService (#95) #320's TestAsyncModeUnifiedExecutor already covers the 429-at-capacity contract and the parallel_mode activity flag — the production-correctness bar.
• The remaining tests here (TestAsyncSessionPersistence, TestAsyncCollaborationActivity, TestAsyncSafetyNet) and the poll_execution_until_done helper are useful but not blocking. If a future regression demands them, they can be re-filed as a fresh test-coverage PR against dev rather than carrying a long-stale branch forward.
• PR currently targets main, but the project has since moved to dev as default integration (a799abce "docs(workflow): adopt dev as default base"). Reopening would require both a base-branch flip and a rebase onto a now-week-old upstream/dev.

Closing without merge.