feat(http-api): add GET /agent-task/runs/:id as a pure read - #11894
Merged
Conversation
An out-of-process orchestrator could already list, inspect, watch and cancel a detached cook through the generic controller-job surface — cook and fanout are both `ControllerJobDriver`s (#11857, #11871) — but had no way to read the durable agent-task *run* the job supervises. A run is not a job. Resolve it through the activity agent-task provider's `probe_by_id`, which is already documented as an indexed, non-mutating lookup (#10308) and already understands Cook-id aliasing. Deliberately NOT `agent_task_lifecycle::status()`, the reconciling read behind the CLI: - it rewrites the durable record on the way out, and a GET that mutates is a decision, not something to replicate silently - for a non-controller-local record it performs a live runner round trip. The daemon accept loop handles one connection inline before accepting the next, so an unbounded read stalls every other client of a shared process - `require_run` in this module already refuses the same reconciling facade for the same reason (#6768) The response says `reconciles: false` and names the command that does, rather than letting a caller assume freshness. Bounding: exactly one indexed probe, no fallback. `activity::show_activity` would fall back to a full-corpus scan of up to 1000 records across three stores; that is unbounded work on a serial loop, and wrong on an agent-task route anyway. The id is length-capped before it reaches the record store. Redaction: the `ActivityItem` projection is a typed field-by-field allowlist — ids, timestamps, state, and evidence *references*. `command`/`cwd` are `None` for an agent-task record, so no prompt or provider output is reachable. A failing probe is reported as a flag, never as its message, because error text from this subsystem can quote record contents and the daemon copies `message`/`details` straight into the response body. Auth posture is identical to its neighbours: the read-only API takes no bearer token and is protected by the daemon's hard loopback-only bind validation. No submit or retry route: submission has its own safety review.
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
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.
What already works — because cook and fanout are controller jobs now
POST /controller/jobs→enqueue_controller_job→driver("agent-task.cook", v). Idempotency-keyed;validate_secret_referencesfails closed on any inline prompt/env/token viadeny_unknown_fields.POST /controller/jobs/:id/cancelworks; the genericPOST /jobs/:id/canceldeliberately refuses controller jobs (store/mod.rs:1564) with a hint pointing at the right route, so the driver'scancel()can stop the work it owns.GET /jobs/:id/eventscarries driver-projectedpublic_progress/public_result; evidence URIs hydrate throughGET /runs/:id/artifacts/:id/content.GET /activity/:idalready resolved agent-task run ids, including Cook aliases.#11857 and #11871 closed most of this as a side effect.
Existing redaction is already sound and needed no change:
ControllerJobState(holding the privaterequest) hangs offStoredJob, notJob, andjob_store.get()returnsstored.job.clone()— soGET /jobs/:idcannot leak it.CookJobDriver::public_erroralready collapses to a typed code plus"controller-owned cook supervision failed", precisely because cook error text quotes provider output.The one real gap, and why it is a pure read
GET /agent-task/runs/:id— an agent-task-scoped read that fails closed on a non-agent-task id.It does not call
agent_task_service's status function. It could not, and should not.homeboy-agents/Cargo.tomlstates it — "Depends on homeboy-core; core does not depend on it."http_api.rsis core. That call is a dependency cycle.status_with_optionsdoes ~8store::write_recordcalls and, for a non-controller-local record,reconcile_runner_job_state— a live network round trip to the runner. On a serial accept loop that stalls the entire daemon. This file already refused exactly this once:require_run's doc comment (Adopt runs_service/evidence_report facade across observation-store consumers (single activity-aware surface) #6768) declines the reconciling facade for the same reason.So it resolves via
activity::agent_task_provider::probe_by_id— an existing, registered, indexed, explicitly non-mutating lookup (#10308) that already handles Cook-id aliasing, returningActivityItem, the same projectionGET /activity/:idserves. Not an invented shape.Deliberately not
activity::show_activity, which falls back to a full-corpus scan of up to 1000 records across three stores when probes miss.The response carries
reconciles: falseandreconcile_with: "homeboy agent-task status"rather than letting a caller assume freshness.SSE — not feasible, and not attempted
daemon/mod.rs:1155:Inline, single-threaded, no per-connection spawn. One SSE client would hold the accept loop for the life of the stream, blocking
/health,daemon status, and the reverse-broker/runner/jobs/*routes remote runners depend on. That is a total daemon outage, not degradation. Zero hits forevent-stream/chunked/Transfer-Encodinganywhere.Making it safe needs: per-connection concurrency at 1155; a streaming writer (
write_http_responseemitsContent-Length+Connection: closeand returns after onewrite!); bounds that do not exist (max streams, max lifetime, keepalive frames); and socket timeouts.Pre-existing hazard found while reading:
read_http_requestblocks onstream.read()with no timeout, so today a client that connects and sends nothing wedges the entire daemon. Filed separately — it deserves attention independent of this work.Cheaper alternative, proposed not built:
JobEventalready carriessequence: u64, butjob_store.events()returns the wholeVecunder a mutex every call. AGET /jobs/:id/events?after=Ncursor is a pure request/response change needing zero restructuring, bounds the payload, and turns O(n²) polling into O(n).Auth and redaction
Auth: identical to neighbours by inheritance.
broker_authapplies only to/files/*and/runner/*; the read-only API is reached via_ => route_read_only_api(...)without it. Protection is the daemon's hard loopback-only bind validation. This route sits in the same fallthrough — not less protected, and deliberately not more, since a lone authenticated route in an unauthenticated block is confusing rather than safer.Redaction, three layers:
ActivityItemis a typed allowlist and hardcodescommand/cwdtoNonefor agent-task records, so no prompt or provider output is reachable; a failing probe reportsprobe_failed: trueand never its message, because the daemon copieserr.messagestraight into the body; and the id is length-capped at 256 bytes before the store lookup.No write path added — tests assert
POST /agent-task/runs/:id,/agent-task/runs,.../cancel,.../retryall fail to route.Verification
cargo check --workspace --testsclean.http_apitests: 36 passed, 1 failed —artifact_content_serves_encoded_artifact_store_locator, which I confirmed fails identically onorigin/main. Pre-existing, filed separately, unrelated to this change.Flagging
activity.rs:572), and registration is process-global and irreversible, so a fake would poison the shared lib-test binary. It needs ahomeboy-agents-level test.http_api/types.rsto add theHttpEndpointvariant — impossible otherwise.probe_failedconflates a store outage with a miss (both 404). Deliberate — it matchesresolve_activity_item, and the alternative leaks error text.requested_idandrun_idmay differ when a Cook alias resolves to an attempt record; both are exposed.daemon/mod.rsneeded zero changes — the_ => route_read_only_api(...)fallthrough picks the route up, and no daemon arm shadows/agent-task.