Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 31fe07a9c0daf770dabf97aabf60117b09883cc6 against base/current main 9162ea6db1fe0f57d6fc4de5120fac5c5a1938be. I inspected the Runs lifecycle changes, clarify_gateway binding primitive, stop/orphan cleanup, run/session status updates, exact-head tests/CI state, the source PR #68105, the now-merged batch-clarify contract in #89467, and the adjacent open Runs work in #89748 and #89754.
The core direction is sound. Using run_id as the clarify queue session key mirrors the approval queue's per-run isolation, the response endpoint validates stable server-issued choice IDs rather than trusting labels, free text is bounded and never echoed in clarify.responded, secret redaction happens before HTTP egress, and stop/cleanup release the blocking Event.wait rather than leaving an unanswerable agent thread parked. Preserving #68105 / @hazeion as the substantive source is also the right provenance model; this PR is a salvage/superseder, not a clean-room replacement.
I found two merge blockers at the ownership/state boundaries.
1. The new clarification mutation is authenticated to the URL-selected profile, but not authorized to the run's owning profile
The API listener is multiplexed: /p/<profile>/... sets _api_request_profile, enters that profile's runtime scope, and _expected_api_key() explicitly returns the API key authorized for that URL-selected profile. That gives the request a concrete profile principal.
But Runs are stored in adapter-global maps keyed only by run_id, and _handle_run_clarification() does not compare that principal with the profile that created the run. In fact the queued run status currently records created_at, session_id, and model, but not request_profile. The new handler does:
run_id = request.match_info["run_id"]
if run_id not in self._run_statuses: ...
...
clarify_session_key = self._run_clarify_sessions.get(run_id)
pending = clarify_gateway.get_pending_by_id(request_id, session_key=clarify_session_key)
...
clarify_gateway.resolve_gateway_clarify(...)So a request authenticated as profile B can address a run created under profile A if it knows A's run_id + clarification request_id; the queue binding proves which run, but never which profile principal owns that run. The mutation then resumes A's blocked agent from B's authenticated URL scope.
This is an inherited gap from #68105's handler, not new authorship by @meiqinsi, but this salvage is the point where the endpoint would enter current main. Please bind every run to its creation profile and fail closed before reading or mutating clarification state when the current request profile differs. A regression witness should create a run through /p/alpha/v1/runs with alpha's API key, capture its pending clarification, then POST that exact run/request pair through /p/beta/... using beta's valid key and prove it cannot resolve or alter alpha's wait. The positive alpha→alpha path should still resume normally.
This also exposes the broader defect-class question for the existing run-control family (GET, events, approval, steer, stop): they use the same global run_id namespace. I am not asking this PR to silently claim class closure unless the whole family is audited, but the new mutating clarification endpoint should not deepen a known profile-ownership hole. #89754's idempotency work is adjacent evidence of the same ownership axis: run admission bookkeeping also needs profile scope.
2. _session_awaiting_user: Dict[str, bool] loses legitimate concurrent waiters
Current main already states the load-bearing invariant in _handle_runs: client-provided session_id is a conversation scope, not a run/authorization namespace, and multiple concurrent Runs can intentionally share it. That is exactly why approvals key their blocking state by run_id.
This PR gets the clarify request queue right (clarify_session_key = run_id) but projects the new session-level state into a single bit:
self._session_awaiting_user: Dict[str, bool] = {}
...
self._session_awaiting_user[session_id] = True
...
self._session_awaiting_user.pop(session_id, None)If runs A and B share session s and both are waiting for clarification, A answering, timing out, stopping, or merely finishing cleanup removes s from the map while B is still waiting_for_clarification. The per-run status for B remains truthful, but the session-level contract this PR is explicitly adding becomes false. The same textual session_id can also exist under separate multiplexed profile homes/state DBs, so the raw-string key collapses profile ownership as well as run multiplicity.
Please make session waiting state ownership-preserving: e.g. (profile, session_id) -> set[run_id/request_id] / refcount, or derive it from the owned per-run state rather than maintaining a lossy boolean. Add a witness with two concurrent runs sharing one session: put both into clarify wait, resolve/stop/timeout one, and assert the session still reports awaiting-user until the second waiter exits. Add the cross-profile same-session-id twin if the session map remains adapter-global.
Composition / merge order
- #68105 / @hazeion — substantive source. This PR correctly credits and supersedes it; the profile-authorization blocker above is inherited from that implementation and should be repaired without losing attribution.
- #89467 / @ethernet8023 — merged current-main clarify contract. It added
questionsbatches. This Runs callback does not acceptquestions, so the core deliberately treats it as a legacy callback and decomposes a batch into sequential prompts; that is compatible, not a blocker. Please add one composed batch witness, though, because this is the exact kind of current-main semantic that a salvage can miss. - #89748 / @RoySRose — complementary event-ordering fix on the same Runs/SSE seam. If it lands first, this branch should rebase and ensure
clarify.request/clarify.respondedobey the same ordering primitive rather than recreating a second cross-thread enqueue rule. - #89754 / @RoySRose — complementary run/approval idempotency work on the same file/tests. It needs composition with clarification admission/control ownership rather than a textual conflict-only rebase.
- #2971 — remains the umbrella request. This PR closes ask/answer for Runs, but correctly does not claim defer/park/wake or the full resumable-interaction class.
Verification / CI
The branch is exactly on current main (no base drift at review time). The PR reports its touched Runs/API/toolset/clarify suites green on macOS and the new tests cover exact request binding, single-use resolution, multi-select shape, auth, redaction/bounds, and stop release.
Repository-hosted exact-head evidence is currently incomplete: the CI workflow for 31fe07a... ended in failure before creating any jobs, while Nix/Docker and label-rerun workflows are action_required; fetching the CI run returns an empty job list. So there is no executed exact-head matrix to independently corroborate the local suite yet.
Re-review gate: bind run control to the creating profile; replace the session-level boolean with multiplicity/profile-safe ownership and add the concurrent-waiter witness; add a current-main batch-clarify composition test; then obtain an executed exact-head CI matrix after the repository workflow issue clears.
|
@andrexibiza Thanks for the review. Addressed the two blockers plus the #89467 composition test.
Will rebase if #89748 / #89754 land first. Exact-head CI still depends on the repo workflow issue you noted. |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact head dd38b401ced229ea5a5c024446aaa20b6cca32f9 and GitHub's current-main test merge 2feff65e2ede5f75c65ec5d5503ef873cbf1468c (f43eabee5f36e11448086ee8ee17c499958e81bf + this head).
The two code blockers and the requested #89467 composition witness are closed. I found no remaining code blocker from my prior review.
- Profile ownership: run admission records
request_profile; clarification checks the URL-selected principal before any pending-state lookup or mutation and returns the indistinguishable404 run_not_foundon mismatch. The alpha/beta regression proves beta's valid key cannot resolve alpha's exact run/request pair, leaves the pending entry intact, and preserves the alpha→alpha positive path. - Concurrent wait ownership: the lossy adapter-global
_session_awaiting_userboolean is gone. Waiting truth is per run, and the same-session_idconcurrency witness proves resolving one run does not clear or consume its sibling's wait. - Merged batch contract: the real
clarify_tool(..., questions=[...])path is exercised as sequential prompts on one run with distinct request IDs; replay of the spent first ID fails with 409 before the second prompt is answered.
#89748 and #89754 are still open, so their prospective composition condition has not triggered. The synthesized test merge is clean against current main, and I found no new semantic conflict in the composed Runs lifecycle.
The only remaining gate is repository-hosted verification. The exact-head CI/Docker/Nix workflows are action_required; the CI run references test-merge SHA 2feff65e... but created no jobs. Do not represent the matrix as green or merge until a maintainer approves/runs the fork workflows and the exact test-merge checks execute successfully.
Non-blocking housekeeping: the PR description still says awaiting_user is stored in a session map. That sentence is stale; the implementation correctly removed that map in favor of per-run truth.
|
Re-reviewed current head The requested source repairs are closed:
I also checked the request-id/choice validation, exact run binding in Disposition: my two source blockers are cleared on |
dd38b40 to
7cb5629
Compare
7cb5629 to
ed17637
Compare
(cherry picked from commit 6c7fdfc) Co-authored-by: Cursor <cursoragent@cursor.com>
Port multi_select, awaiting_user isolation, profile binding, and sequential batch coverage onto api_server_runs (implementation already in prior commit) plus matching tests/docs. Original PR commits consolidated during rebase onto main's extracted runs module. Co-authored-by: Cursor <cursoragent@cursor.com>
Split the RequestKey import so older aiohttp releases do not null out `web` and break /v1/runs handlers. Co-authored-by: Cursor <cursoragent@cursor.com>
The clarification route delegated to a missing _handle_run_clarification during the main rebase, which 500'd POSTs and left clarify waiters hung until the suite timeout. Wire the handler back and adapt unit tests to main's run-ownership stamps. Co-authored-by: Cursor <cursoragent@cursor.com>
ed17637 to
472f440
Compare
What does this PR do?
Makes
/v1/runssupport the existingclarifytool as a real hard gate: the agent thread blocks onwait_for_response, clients get a bounded SSEclarify.request, and they answer viaPOST /v1/runs/{run_id}/clarification.This salvages #68105 onto current
main(keeps/steer, session-model lock, and stop semantics), then adds:multi_selectthrough the Runs callback → register → SSE prompt, and acceptresponse.type: "choices".awaiting_useron run status while clarification is pending; clear on answer, timeout,/stop, or cleanup.Clarify stays a Runs-only overlay (
enable_clarify=True+ callback) — it is not added to the defaulthermes-api-servertoolset.Scope is ask/answer on
/v1/runsonly; defer/park/wake is out of scope.Related Issue
Related to #2971 and #68105 (credit: @hazeion).
Fixes #
Type of Change
Changes Made
gateway/platforms/api_server.py— Runs clarify callback, SSEclarify.request/clarify.responded,POST /v1/runs/{run_id}/clarification,waiting_for_clarificationas in-flight,multi_select+awaiting_usertools/clarify_gateway.py— session-boundget_pending_by_id/resolve_gateway_clarifytests/gateway/test_api_server_runs.py— ask/answer, binding, multi-select, stop-while-waiting,awaiting_userenter/exittests/gateway/test_api_server.py/test_api_server_toolset.py/tests/tools/test_clarify_gateway.py— capabilities + overlay + gateway binding coveragewebsite/docs/user-guide/features/api-server.md(+ programmatic integration endpoint list) — document clarification APIHow to Test
scripts/run_tests.sh tests/gateway/test_api_server_runs.pyscripts/run_tests.sh tests/gateway/test_api_server_toolset.py tests/tools/test_clarify_gateway.py tests/gateway/test_api_server.pyclarify→ SSEclarify.request→GET /v1/runs/{id}showswaiting_for_clarification+awaiting_user: true→POST .../clarificationresumes the same turnmulti_select=true→response.type=choiceswithchoice_ids→ callback receives a JSON array string/stopwhile waiting clears pending clarify and does not hang the agent foreverChecklist
Code
fix(scope):,feat(scope):, etc.)scripts/run_tests.shon the touched suites and all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
N/A — covered by gateway unit/integration tests around Runs clarification SSE + POST.