test(tasks): cover server-side execute_agent (BUILD) path (#749)

[frankbria]

Summary

Closes #749. Adds the missing API-level test for the task execute / BUILD path.

The existing /tasks/{id}/start tests all omit execute=true, so the agent-dispatch branch (_run_agent → runtime.execute_agent) had zero API-level coverage — a regression in server-side agent dispatch would slip through.

What the test does

TestTasksV2Execution::test_start_task_execute_runs_agent_to_terminal:

• Injects MockProvider via CODEFRAME_LLM_PROVIDER=mock — the router builds the provider through get_provider("mock"); its text-only response makes the ReAct agent complete on the first iteration (react_agent.py:518).
• POST /tasks/{id}/start?execute=true, asserts the immediate executing response.
• Polls GET /tasks/{id}/run until the run leaves RUNNING, asserts it reaches COMPLETED.
• Asserts the run produced output (streaming.run_output_exists) — evidence the BUILD path actually emitted events.

Acceptance criteria

A test injects MockProvider and calls /tasks/{id}/start?execute=true, asserting the run reaches a terminal status and produces expected events.

✅ Met.

Notes

• The /start?execute=true endpoint dispatches execute_agent in a fire-and-forget daemon thread. The test snapshots threads before the POST and joins the new worker before assertions/teardown, so the still-draining thread can't touch the temp workspace DB after the fixture removes it (eliminated an intermittent unable to open database file log). Verified stable across repeated runs.
• Test-only change, single file.

Testing

• uv run pytest tests/ui/test_v2_routers_integration.py → 74 passed
• uv run ruff check tests/ui/test_v2_routers_integration.py → clean
• New test run ~7× individually, no flakes.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 44 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5427aabf-2342-4c2b-a54b-380e1172509b

📥 Commits

Reviewing files that changed from the base of the PR and between a563f4f and 4ca329b.

📒 Files selected for processing (1)

• tests/ui/test_v2_routers_integration.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/749-server-side-execute-agent-path

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review

This PR adds test_start_task_execute_runs_agent_to_terminal, the first API-level test to exercise /tasks/{id}/start?execute=true — closing the gap where the agent-dispatch branch (_run_agent → runtime.execute_agent) had zero coverage. It's a single-file, test-only change with no production code touched, so risk is low. I traced the MockProvider path (get_provider("mock") → text-only "Mock response" → react_agent.py:518 not response.has_tool_calls → classify_error_for_blocker returns None for that text → completes), the /start router's fire-and-forget thread (tasks_v2.py:694-730), and the thread-diff/join pattern used to avoid the teardown race — all of that checks out and the reasoning in the PR description matches the actual code.

One finding worth a look:

tests/ui/test_v2_routers_integration.py:773 — the "produced output" assertion doesn't test what the PR description claims it tests.
RunOutputLogger.__init__ (codeframe/core/streaming.py:87-103) opens the run's output log file in append mode (open(self.log_path, "a", ...)) unconditionally at construction — before any agent iteration runs. run_output_exists() is just path.exists(). Since execute_agent constructs RunOutputLogger near the very start of the call (runtime.py:693), the file exists from that point on regardless of whether any output was actually written. So assert streaming.run_output_exists(workspace, run_id) is True would pass even in a hypothetical where zero events got emitted (e.g., execute_agent errored out immediately after constructing the logger, before writing anything) — it isn't "evidence the BUILD path actually emitted events" as the PR description states, just evidence the run started. Combined with the status == COMPLETED assertion above it, the test still provides reasonable overall signal, but if the intent is specifically to prove events were emitted, consider asserting on the file's content/size (e.g., get_run_output_path(...).stat().st_size > 0, or checking for an expected marker) instead of mere existence.

Nothing else stood out — the thread-snapshot/join approach to avoid the teardown DB race is sound (starlette's TestClient, used here without a with block, opens and fully tears down its own anyio portal thread synchronously per-request, so the diff only ever catches the intended _run_agent daemon thread), the env var injection via monkeypatch.setenv is read inside the joined thread before monkeypatch's teardown reverts it, and the module's existing pytestmark = pytest.mark.v2 already covers the new test per CLAUDE.md convention.

🤖 Generated with Claude Code

[frankbria]

Thanks — good catch on the run_output_exists finding. Confirmed: RunOutputLogger.__init__ opens the output file in append mode at execute_agent start (streaming.py:103, before any iteration), and run_output_exists() is just path.exists() — so the old assertion only proved the run started. Fixed in 4ca329b by asserting get_run_output_path(...).stat().st_size > 0, which proves the BUILD path actually wrote output. Verified the completed mock run produces a non-empty file.

[claude]

Review

Re-reviewed after the fix in 4ca329b. The PR adds test_start_task_execute_runs_agent_to_terminal, the first API-level test to exercise /tasks/{id}/start?execute=true (the _run_agent → runtime.execute_agent BUILD-path branch), which previously had zero coverage. Still a single-file, test-only change — no production code touched.

Previously flagged issue — confirmed fixed. My earlier review noted that streaming.run_output_exists() is just path.exists(), and RunOutputLogger.__init__ (codeframe/core/streaming.py:87-103) opens the log file in append mode unconditionally at execute_agent start (runtime.py:693), so the old assertion only proved the run started, not that it emitted output. The current diff (tests/ui/test_v2_routers_integration.py:773) now asserts output_path.stat().st_size > 0, which does require content to have actually been written and flushed — RunOutputLogger.write() flushes after every write (streaming.py:114), and execute_agent doesn't spawn any further threads (confirmed via grep on runtime.py), so joining the single _run_agent daemon thread before this assertion is sufficient to guarantee the writes landed. This resolves the gap.

Nothing new found. I re-traced the rest of the test against the current code:

• start_single_task (tasks_v2.py:643) is async def, and thread.start() for _run_agent happens synchronously inside the coroutine before the response is returned (tasks_v2.py:729-730), so the threads_before snapshot taken right before the POST reliably captures only threads that exist prior to dispatch — the fire-and-forget worker is guaranteed to show up in the post-request threading.enumerate() diff.
• The test_client/test_workspace fixtures (test_v2_routers_integration.py:25-68) do shutil.rmtree(temp_dir, ignore_errors=True) at teardown with no synchronization otherwise, so the join-before-teardown pattern here is in fact necessary, not defensive-programming.
• monkeypatch.setenv("CODEFRAME_LLM_PROVIDER", "mock") is read by get_provider("mock") inside the joined thread, before the function-scoped monkeypatch fixture reverts it — ordering is safe.

No new correctness, security, or coverage issues stood out in this pass. The 30s poll deadline / 10s join timeout are reasonable for CI and, per the PR description, the test has been run repeatedly without flaking.

🤖 Generated with Claude Code