fix(mcp): persist handoff results durably to survive transport timeouts

[anilkmr-a2z]

Fixes #447

Summary

handoff was fully synchronous with no persistence between result extraction and the HTTP response, so a client-side transport timeout could silently drop the result with no way to retrieve it. This PR makes handoff results durable:

• Persists handoff results to a durable store before terminal teardown, so results survive even if the response never reaches the client.
• On a transport timeout, returns a structured pending response with a job_id instead of dropping the result.
• Adds a GET /handoff-results/{job_id} endpoint to retrieve the persisted result once it's ready.

Test plan

• test/clients/ — passing
• test/api/ — passing
• test/mcp_server/ — passing
• test/services/ — passing
• Combined scoped run: 1179 passed. 4 pre-existing failures remain, confirmed unrelated to this change by stashing the diff on main and reproducing the same 4 failures.

[fanhongy]

Sweet. @anilkmr-a2z thanks for picking this issue. Looks a clean apprach to me. Feel free to assign to me for review

[codecov-commenter]

Codecov Report

❌ Patch coverage is 86.44068% with 16 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@41c8ce7). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/cli_agent_orchestrator/api/main.py	73.17%	11 Missing ⚠️
src/cli_agent_orchestrator/clients/database.py	95.65%	2 Missing ⚠️
...cli_agent_orchestrator/services/cleanup_service.py	66.66%	2 Missing ⚠️
src/cli_agent_orchestrator/mcp_server/server.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #453   +/-   ##
=======================================
  Coverage        ?   89.40%           
=======================================
  Files           ?      157           
  Lines           ?    18755           
  Branches        ?        0           
=======================================
  Hits            ?    16768           
  Misses          ?     1987           
  Partials        ?        0           

Flag	Coverage Δ
unittests	89.40% <86.44%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[anilkmr-a2z]

Thanks for the offer! Just rebased onto main and fixed a formatting nit - CI's green now. Still marked draft while I do final validation; will flip to ready for review and add you shortly.

[call-me-ram]

The persistence substrate here is solid — schema on the existing DB (inheriting the 0700/0600 posture from the #372 follow-up), correct retention wiring into cleanup_service, honest handler-branch coverage (running→completed/error incl. ValueError and generic Exception), job_id format validation. But as it stands it doesn't fix the failure mode #447 actually reports, so requesting changes on four points:

1. [must-fix] The job_id never reaches the supervisor in the primary scenario. pending=True/job_id are returned only from the except requests.Timeout branch (server.py:756–766), which fires after the internal HTTP deadline (timeout + 180s). #447's scenario is the opposite ordering: the provider's tools/call deadline (e.g. Codex's 600s) expires while _handoff_impl is still blocked in requests.post (e.g. 1080s for timeout=900) — the tool's return value, the only carrier of job_id, is serialized to a dead transport. The id is minted per-call (uuid4().hex), never logged, and there's no list/lookup-by-terminal endpoint, so the durably persisted row is undiscoverable in exactly the case the PR targets. Fix directions (any one): emit the job_id before the blocking wait (MCP progress/log notification), accept a caller-supplied idempotency key, add a recent-results listing, or soft-return a structured running result before the provider transport deadline (the issue's own "desired behavior").

2. [must-fix] Persistence happens AFTER teardown; the PR body claims the opposite. "Persists handoff results … before terminal teardown" — in fact run_agent_step extracts at agent_step.py:221 and tears down at :229–252 before returning; the upsert_handoff_result(job_id, "completed", ...) runs only after that in the endpoint's success branch. A crash in between loses the result — violating #447's "persist result, then tear down." Either plumb job_id into run_agent_step and persist between extract and teardown, or correct the claim.

3. [should-fix] No usable retrieval path for the supervisor. No MCP tool wraps the new endpoint, and the pending message says GET /handoff-results/{job_id} with no base URL — the supervisor LLM must guess host/port, be allowed to curl, and (if auth is on) hold a bearer token it has no way to obtain. A get_handoff_result MCP tool closes this and gives finding 1's polling half a home.

4. [should-fix] The new GET is unauthenticated while serving full worker output. get_handoff_result_endpoint (main.py:2183) has no require_any_scope dependency; auth here is per-endpoint, so when auth is enabled last_message (potentially prompts/secrets) is anonymous-readable. The 128-bit random job_id is a real capability mitigant, but newer sensitive GETs follow the gated posture — please add SCOPE_READ.

Smaller items, fine as follow-ups: the author-acknowledged same-job_id double-execute race (a re-POST with state="running" should attach/reject, not launch a second worker — and note the client can never reuse the key anyway today, which is finding 1 again); _migrate_add_handoff_results is redundant (create_all already creates missing tables — the raw-SQL duplicate can drift); blocking sqlite calls from async handlers (consistent with existing debt, but to_thread precedent exists); no startup reconciliation for stuck running rows (bounded by the 14-day purge, so minor). Test gaps worth closing with the rework: a true persist-then-fetch integration test (the GET tests mock get_handoff_result), and an auth test on the new endpoint.

Happy to re-review quickly — the substrate is right, it just needs the discovery/ordering half of the contract.

[anilkmr-a2z]

Pushed 430a210 addressing findings 2-4:

Finding 2 (persist before teardown): moved the completed-state persist inside run_agent_step, between the output extraction and the teardown call, and threaded job_id through as a new optional parameter. The HTTP handler no longer persists on the success path at all - only run_agent_step does, and it does so before the terminal is destroyed. Updated the docstring's claim to match. Added unit tests asserting the ordering directly (not just that both calls happened) and that a persistence failure doesn't block teardown or fail the step.

Finding 3 (no MCP-callable retrieval path): added a get_handoff_result tool wrapping the GET endpoint, following the same pattern as delete_terminal. Updated the pending-timeout message to point at the tool name instead of a raw HTTP path. Added tests for the completed/running/not-found/error branches.

Finding 4 (unauthenticated GET): added require_any_scope(SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN) to the endpoint, matching /events and /memory/export. No-op when auth is disabled.

Finding 1 (job_id never reaching the caller in the actual #447 scenario): confirmed this is real - traced through _handoff_impl and the timeout math holds: the internal HTTP timeout (timeout + 180) is always longer than the caller-supplied timeout, so _handoff_impl's own except requests.Timeout can only fire after an external client with a shorter deadline (e.g. Codex's 600s) has already given up. The job_id genuinely never reaches the caller in that ordering.

I don't want to pick one of your four suggested directions unilaterally since they're real architecture forks, not a bugfix:

• emitting job_id via an MCP progress/log notification before blocking
• accepting a caller-supplied idempotency key
• adding a recent-results listing endpoint
• soft-returning a structured running result before the provider's transport deadline (this one matches issue Long-running handoff results can be lost after MCP timeout; make handoff durable and async #447's own suggested near-term fix most closely, but changes handoff's external contract - callers would need to handle a running result on the currently-synchronous path)

Which direction do you think fits best, or would you rather this land as a separate follow-up PR scoped explicitly to that ordering problem, given the durability substrate here is now correctly independent of it?

[gutosantos82]

@anilkmr-a2z thanks for the thorough turnaround — findings 2–4 look good: persisting inside run_agent_step between extraction and teardown (with the ordering-asserting test), the get_handoff_result MCP tool, and scope-gating the GET all address @call-me-ram's points cleanly.

On finding #1 (the job_id not reaching the caller when the provider's tools/call deadline expires before the internal timeout + 180s) — you're right that it's an architecture fork, not a bugfix. My recommendation is option 2, the caller-supplied idempotency key, landed as a separate follow-up PR with this durability substrate merging now.

Why option 2 over the others:

• It's the only option where the identifier travels in the request direction, so it survives even when the response transport is already dead — the supervisor can call get_handoff_result with the key it chose, which is exactly Long-running handoff results can be lost after MCP timeout; make handoff durable and async #447's scenario. Options 1/3/4 all lean on the response path or out-of-band delivery.
• It also retires the same-job_id double-execute race you already flagged — one primitive fixes both.
• Option 1 (progress/log notification) isn't reliable end-to-end: nothing uses Context today, and there's no guarantee the notification reaches the model's context (the consumer of the id is the LLM, not a human watching logs). Fine as belt-and-braces, not the fix.
• Option 3 (recent-results listing) needs a caller_id column and has recency ambiguity for concurrent handoffs, and does nothing for the race.
• Option 4 (soft-return running) is the cleanest long-term model but changes handoff's external contract — the consumers are supervisor agents steered by the skills/README/examples and the e2e expectations, so it ripples across all of those. Worth designing on its own.

Two things to fold into the follow-up when you do option 2:

• The dedupe needs to be an atomic insert-or-detect — the current upsert_handoff_result is get-then-write, which still races. Since job_id is the primary key, an INSERT-or-IntegrityError (or INSERT OR IGNORE + re-read) gives first-writer-wins under SQLite's serialized writes.
• The job_id validator currently requires 32-char lowercase hex, which a caller-supplied key won't match — either relax it or hash the caller key to hex in the MCP tool.
• Optional complement: a provider-aware cap on client_timeout (shorten it below the known provider deadline only when it would otherwise exceed it) would also help the calls that today fail with no job_id at all.

So: happy for this to merge once the substrate is finalized, with two housekeeping items —

• please change Fixes #447 → Refs #447 in the description so merging doesn't auto-close the issue (the discovery/ordering half lands in the follow-up), and
• it needs a rebase (currently conflicting with main).

Thanks again — nice work here, and thanks @call-me-ram for the sharp review.

[call-me-ram]

Re-reviewed at 8b85f58. Three of my four findings are properly fixed — thanks for the thorough round.

Finding 2 (persist-before-teardown) — fixed. run_agent_step now takes job_id and persists between extraction and teardown (agent_step.py:449-464), with the ordering documented at the call site and in the docstring (steps 6/7). This is what #447 asked for, and the claim in the PR body is now true.

Finding 3 (retrieval path) — fixed. The get_handoff_result MCP tool (server.py:1335+) gives the supervisor a callable path instead of a bare URL it would have had to guess a host, port and token for.

Finding 4 (auth on the GET) — fixed. get_handoff_result_endpoint now carries require_any_scope(SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN), matching the posture of the other sensitive GETs.

Finding 1 is still open, and it is the one that decides whether this PR fixes #447.

Minting job_id client-side before the POST (server.py:725) and sending it in the payload is a real improvement — the server can now record the row under a key the client already knows. But the id still only ever reaches the supervisor through one carrier: the HandoffResult returned from the except requests.Timeout branch (server.py:766-776).

#447's reported ordering is the other one:

• MCP client timeout: client_timeout = timeout + _CLIENT_TIMEOUT_HEADROOM — e.g. 900 + 180 = 1080s
• Provider tools/call deadline: e.g. Codex's 600s

The provider transport dies at 600s while _handoff_impl is still parked in requests.post. The requests.Timeout branch never runs, so nothing is returned, so the job_id is never observed by anyone. I re-checked the two ways it might still be recoverable, and neither exists at this head:

• no listing/lookup endpoint (GET /handoff-results/{job_id} is by-id only; no by-terminal, by-session or recent-results route)
• job_id is logged only on failure paths (main.py:1906, 1959, 1971, 1982, 1990 — all logger.warning on persist failure), never on the success path

So the durable row exists and is correct, and remains undiscoverable in precisely the scenario the PR targets. Everything downstream of discovery is now built and working, which is why I would like to see this last piece rather than land it half-connected.

Any one of these closes it:

• emit the job_id before the blocking wait (MCP progress/log notification), or
• accept a caller-supplied idempotency key so the supervisor already holds it, or
• log the job_id unconditionally at INFO on the success path and add a by-terminal/recent listing, or
• soft-return a structured running result before the provider transport deadline (issue #447's own stated "desired behavior")

The last is the most faithful to the issue and also removes the double-execute race you documented at server.py:722-724, since a caller who holds the key up front can be made idempotent.

Also needs a rebase — the PR is currently DIRTY (conflicts with main); ac07018 merged upstream/main on 07-19 but main has moved since.

Re-review will be quick once finding 1 has a carrier — the substrate, ordering, retrieval tool and auth are all in good shape now.

[gutosantos82]

PR Review: #453 — fix(mcp): persist handoff results durably to survive transport timeouts (re-review)

Context

Head 8b85f586, MERGEABLE, all 11 CI checks green (unit 3.10/3.11/3.12, Code
Quality, Security Scan, Trivy, Dependency Review, MCP Apps, Web UI). 10 files,
+984/−5. reviewDecision=CHANGES_REQUESTED — external reviewer call-me-ram
requested changes at the previous head (2026-07-17) on four findings. The
author (anilkmr-a2z) pushed 430a2104 ("address PR #453 review") + a merge of
upstream/main + a test fix (8b85f586), and replied point-by-point on the PR.
fanhongy has volunteered as a reviewer. Codecov: 86.4% patch coverage.

Delta since our previous report (at 3630081f): +224 net lines, all directly
responsive to the external review. Our previous verdict was also "Request
changes" on essentially the same finding-1 grounds plus the "Fixes #447"
linkage.

What the new commits fix (verified in the diff)

Finding 2 — persist-before-teardown: FIXED, correctly

The previous placement persisted state="completed" in the HTTP handler
after run_agent_step returned — i.e. after the terminal (the only other
copy of the result) was already torn down. Now:

• run_agent_step takes an optional job_id and performs the
  upsert_handoff_result(job_id, "completed", ...) via asyncio.to_thread
  between extraction and teardown (agent_step.py), best-effort (logged,
  never raised).
• The handler's success branch no longer persists at all; a NOTE comment
  explains why. Error-path persistence (all four exception branches) stays in
  the handler, which is the right split since only the handler knows the
  exception kind.
• Tests assert the ordering (["upsert", "delete"] observed sequence),
  not merely that both calls happened; plus no-job_id no-op and
  persistence-failure-doesn't-block-teardown cases. This is exactly what the
  reviewer asked for.
• Bonus: the sqlite write moved off the event loop (to_thread), addressing
  one of the reviewer's smaller items on this path.

Finding 3 — usable retrieval path: FIXED

New get_handoff_result MCP tool wrapping GET /handoff-results/{job_id}
(same pattern as delete_terminal), returning
success/state/terminal_id/last_message/error_message, with 404 and generic
failures mapped to structured success=False. The pending-timeout message now
says "Retrieve the result with the get_handoff_result tool, job_id=..."
instead of a bare HTTP path with no base URL/auth. Tests cover
completed/running/not-found/error branches and assert the message names the
tool and contains the job_id.

Finding 4 — unauthenticated GET: FIXED

get_handoff_result_endpoint now depends on
require_any_scope(SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN), matching the
posture of /events and /memory/export; no-op when auth is disabled (the
default). Docstring documents the rationale.

What remains open

Finding 1 (must-fix) — job_id still never reaches the caller in the primary #447 scenario

Unchanged at this head: client_timeout = timeout + _CLIENT_TIMEOUT_HEADROOM
(180s), and pending=True/job_id are returned only from the
except requests.Timeout branch. When the provider's MCP tool-call deadline
(Codex's injected 600s) expires while _handoff_impl is still blocked in
requests.post (e.g. 1080s for timeout=900), the tool's return value — the
only carrier of the job_id — is serialized to a dead transport. The id is
minted per-call client-side, never logged, and there is no
list/lookup-by-terminal surface, so the durably persisted row is
undiscoverable in exactly the targeted case. The new get_handoff_result tool
gives the polling half a home but does not solve discovery: the caller still
has nothing to poll with.

The author's handling of this is exemplary rather than evasive: they traced
the timeout math, confirmed the finding is real, enumerated the four candidate
directions from the review (progress/log notification before blocking;
caller-supplied idempotency key; recent-results listing; soft-return a
structured running result before the provider deadline — noting the last
matches issue #447's own near-term suggestion but changes handoff's external
contract), and explicitly asked the reviewer to pick a direction or bless a
scoped follow-up PR. That question has been sitting unanswered since
2026-07-17 — the ball is in the reviewer's court, not the author's.

"Fixes #447" still overstates scope

The PR body still opens with "Fixes #447"; merging as-is auto-closes an issue
whose desired async lifecycle (dedup/idempotency, immediate id return,
completion hint, ack) remains substantially unimplemented. Should become
"Related to #447" / "Partially addresses #447" unless finding 1 is resolved
in-PR.

Minor residuals (non-blocking, mostly reviewer-acknowledged follow-ups)

• No dedicated auth test on the new GET and no true persist-then-fetch
  integration test (the GET tests mock get_handoff_result) — the reviewer
  listed these as gaps "worth closing with the rework"; still open.
• _migrate_add_handoff_results remains a raw-SQL duplicate of what
  create_all already does (drift risk, reviewer-flagged as follow-up).
• upsert_handoff_result is still query-then-write (benign today: fresh uuid
  per call); same-job_id double-execute race still documented-not-fixed
  (inherent to finding 1's resolution).
• Retention sweep still filters on created_at (a >14-day-running job would
  be purged mid-flight; theoretical).

Security assessment

Improved since last head: the new endpoint is now scope-gated when auth is
enabled, closing the anonymous-read exposure of last_message that the
external review flagged. Otherwise unchanged: localhost-only/Host-header
posture inherited, 128-bit strictly-validated job_id, no permission-surface
changes. Sensitive paths touched (mcp_server/server.py, api/main.py,
agent_step.py) — publish gating applies.

Verdict

Request changes — but note the standing CHANGES_REQUESTED from
call-me-ram already blocks merge, and the author has done everything asked
except the one item they were explicitly told had multiple valid directions
and have asked for guidance on. Posting a second formal request-changes adds
little; the highest-value human action is to answer the author's direction
question on finding 1 (pick one of the four fixes, or agree to a scoped
follow-up PR with the issue linkage softened to "Partially addresses #447").
If a comment is published from this report, it should say that. Not
merge-ready as-is solely because of the finding-1/issue-linkage pair; the
durability substrate itself is now correctly built, correctly ordered, gated,
and well-tested.