[pdettori]

What

OpenAPI 3.1 spec for the client-facing surface of the data plane (docs/api/harness-openapi.yaml), the twin of the control plane's docs/api/openapi.yaml:

• GET /health — liveness
• POST /v1/turn — one agent turn, sync JSON or SSE (full TurnStreamFrame vocabulary)
• POST /v1/runs — leaf/solve/prompt jobs, inline or queued (async 202/501 rules)
• GET /runs/status — async run polling (all seven status shapes)

The document covers the session-token auth model (MI1 R7: the token names the session; an authenticated caller may not name a tenant) and every error code the handlers emit.

How it stays honest

Mirrors the control-plane pattern:

• packages/knative-server/src/routes.ts — new declaration-of-record route table (all 11 served routes, client/internal surface flags, deprecated aliases)
• packages/knative-server/test/openapi-contract.test.ts — asserts doc ↔ table agreement on routes, operationIds, and security, plus a handler guard that drives the real server and asserts every table entry routes (non-404). The knative-server dispatches from if-chains rather than a table, so that guard is what keeps the declaration from drifting from the handler.

The control-plane openapi.yaml now points at the harness document instead of saying the data-plane paths are deliberately undescribed.

Review round

The review asked for the document to match the handler at this head. Where the code was the side
that was off, the code moved: every unclassified failure now answers a stable internal_error
(the turn capacity 503 answers sandbox_unavailable), with the exception text kept to the server
log — the control-plane document's rule; the auth-path credential_unavailable 503 now carries
Retry-After from the same saturation knob; and /health sets text/plain. The rest moved the
document: the exchange's PASSTHROUGH codes on every authenticated route, the /v1/runs 503 body
as the LeafResult failed/saturated branch, the 500 catch-alls, the aborted RunStatus
branch, and the smaller accuracy items. The branch also merges main (post-#469): a run naming
a workloadId is now 501 workloads_unavailable, which the 501 entry documents.

Testing

• packages/knative-server: 454 passed (448 baseline + 6 new); 429 after merging main, whose fix: stop asking Context Service to create sandbox pools #469 removed the workload-route tests
• packages/control-plane: 602 passed (description edit is contract-test-safe)
• tsc --noEmit and Prettier clean
• pre-commit run --all-files clean

Assisted-By: Claude Code

[ingpaolodettori-dev]

Thanks for this. Having the route table + handler guard pattern mirror the control plane is a good shape, and the route/operationId/security half checks out: routes.ts matches everything the handler serves, and the guard isn't vacuous.

I'm requesting changes because the document is meant to be the contract (the PR body says it covers "every error code the handlers emit"), and several response bodies and codes don't match the handler at this head. The contract test only pins routes, operationIds and security, so nothing would catch these:

• must-fix: the /v1/runs 503 body is a LeafResult ({status:'failed',reason:'saturated'}), not Error.
• must-fix: authenticated routes can return the control-plane PASSTHROUGH codes, and none of them are documented: token_expired, credential_*, endpoint_unresolved, and an authenticated session_not_found. They can also return credential_unavailable as a 503 without Retry-After.
• must-fix: internal_error is never emitted. The 500 (and turn-503) error field carries the raw exception text.

Smaller accuracy items (subject_conflict on /runs/status, tenant on /v1/turn, optional sessionId, aborted in RunStatus, /health Content-Type, workspaceReset.tier, an unreachable 501 case) are inline. For each, either the doc or the code can move. Where the code is the one that's off (e.g. emitting internal_error, setting Retry-After on credential_unavailable), fixing the code may be the better outcome.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: Docs (OpenAPI), TypeScript (knative-server), tests
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing

[ingpaolodettori-dev]

must-fix: This 503 isn't an Error. After the bounded wait, handleLeaf answers 503 + Retry-After with {"status":"failed","reason":"saturated"} (server.ts, the isSaturated(result) branch), which has no error field. A client generated from this spec would mis-parse it. Reference the failed LeafResult branch, or a dedicated {status, reason: saturated} schema.

[pdettori]

Fixed in f7196a4: the 503 now documents both bodies — a oneOf with the saturated LeafResult branch ({status: failed, reason: saturated}) as the first arm and Error as the second, and the description names the two causes so the two shapes aren't a surprise.

[ingpaolodettori-dev]

must-fix: When a token is presented, resolveTurnAuth runs the control-plane exchange, and its PASSTHROUGH set (turn-auth.ts:135-143) forwards token_expired (401), credential_required / credential_ambiguous / endpoint_unresolved (400), plus credential_not_found and session_not_found (404) via writeAuthError. None of these are listed. The 404 also contradicts "Authenticated turns create-or-resume instead and do not take this path" (line 108). Every other exchange failure, including control plane unreachable, becomes credential_unavailable 503 with no Retry-After (writeAuthError uses plain JSON_HEADERS), so the 503 section's Retry-After promise doesn't hold for it either. The same applies to /v1/runs, and token_expired to /runs/status.

[pdettori]

Fixed in ddc02d1 + f7196a4: every authenticated route's response lists now name the exchange's refusals — token_expired on 401, credential_required/credential_ambiguous/endpoint_unresolved on 400, credential_not_found and an authenticated session_not_found on 404, with the line-108 claim corrected (the exchange refuses before the harness's own create-or-resume runs). For credential_unavailable I took the code-side option: writeAuthError now advertises Retry-After from the same saturation knob, so the 503's promise holds uniformly. /runs/status gains token_expired on 401, plus a 503 entry for the one auth failure it can hit (turnAuthDeps() sits inside the try, so an unusable keyset under a serving pod reaches writeAuthError there).

[ingpaolodettori-dev]

must-fix: internal_error is never emitted. turnErrorCode returns the raw message for every status other than 404/410, and the route catch-all writes {error: String(err)}, so the 500 error is the exception text. The same goes for the 503 above: its error is the capacity exception's message, not a code. Either emit stable codes (internal_error, a capacity code) or say these bodies are unclassified prose. /v1/runs and /runs/status also have undocumented 500 catch-alls, and /v1/runs can return 404 workload_not_found.

[pdettori]

Fixed in ddc02d1, taking the code-side option: turnErrorCode now returns internal_error for 500s and sandbox_unavailable for the capacity 503, and the six route catch-alls share one internalError() helper. The raw text never reaches the body — it goes to the server log — mirroring the control-plane document's rule for its internal_error (an arbitrary error's text can carry a Redis connection string or a presented token). /v1/runs and /runs/status now document the 500 catch-alls, and /v1/runs the 404 workload_not_found (the workloadId it comes from resolves through the /workloads lifecycle, which the document's scope note already puts out of scope).

[pdettori]

Follow-up: main has since merged #469 (stop-context-service-sandbox-pools), so this branch now merges main and the workload picture changed under us — resolveRunWorkload is gone. The 404 workload_not_found no longer exists: a run naming a workloadId answers 501 workloads_unavailable instead (Context Service no longer allocates sandbox pools for Moca, #455). The spec's /v1/runs 404 now carries only the exchange's session_not_found/credential_not_found, and the 501 lists workloads_unavailable alongside async_runs_unavailable. The stable-code and catch-all fixes above are unchanged by the merge; the suite re-ran green against merged main (429 — main's #469 also removed the workload-route tests).

[ingpaolodettori-dev]

suggestion: subject_conflict can't happen here: authorizeRunRead (turn-auth.ts:282-300) verifies the token and the sid but never reads X-SH-Subject. verifyToken can throw token_expired (401), which isn't listed.

[pdettori]

Fixed in f7196a4: subject_conflict dropped from /runs/status's 400 list — authorizeRunRead never reads X-SH-Subject — and token_expired added to its 401.

[ingpaolodettori-dev]

suggestion: "on every authenticated route" is too broad. /v1/turn parses only {sessionId, prompt}, so a tenant field is silently ignored there, not refused with tenant_not_allowed (that check lives only on /v1/runs and /runs/status). Either narrow the wording or add the check. Also missing from the /v1/turn 400 list: read_error.

[pdettori]

Fixed in f7196a4: the tenant rule is now scoped to the two routes that accept one (/v1/runs body, /runs/status query), with /v1/turn called out as reading only prompt and sessionId, so a tenant field there is inert rather than refused. read_error added to /v1/turn's 400 list.

[ingpaolodettori-dev]

suggestion: aborted is a real persisted status that comes back as {status:'aborted'}, but none of the oneOf branches accept it, so a validator built from this schema rejects a legitimate response. The prose below covers it, but add the branch (and drop "all six" from the PR description: there are seven).

[pdettori]

Fixed in f7196a4: RunStatus gains the aborted branch. "All six" is dropped from the PR description — it now reads all seven.

[ingpaolodettori-dev]

nit: The handler does res.writeHead(200).end('ok'), with no Content-Type header, so text/plain isn't what's on the wire. Set the header in server.ts (one line), or loosen the doc.

[pdettori]

Fixed in ddc02d1, the one-line option: the handler sets Content-Type: text/plain, pinned by an assertion in the /health test. The doc stands as written.

[ingpaolodettori-dev]

nit: WorkspaceReset is {from, reason} (select-sandbox.ts:407-410), so there's no tier in TurnResult.sandbox.workspaceReset. The tier is on sandbox itself and on the SSE frame.

[pdettori]

Fixed in f7196a4: tier removed from TurnResult.sandbox.workspaceReset, with a description noting the tier lives on sandbox itself and on the SSE frame.

[ingpaolodettori-dev]

nit: "or on a deployment that requires auth" can't be reached. With no token under SH_REQUIRE_AUTH, token_required 401 fires first, so 501 only ever goes to token-bearing callers.

[pdettori]

Fixed in f7196a4: the parenthetical is gone — the 501 now names a token-bearing caller, and notes that a token-less caller on a require-auth deployment gets 401 token_required before this.

[ingpaolodettori-dev]

nit: server.close() isn't awaited, and each startServer() also registers a SIGTERM listener and runs prepareServerProcess. That's harmless at two instances, but await new Promise((r) => server.close(() => r())) keeps the file from leaking a listening socket into the next test.

[pdettori]

Fixed in ddc02d1: afterEach awaits the close. Left the SIGTERM listener and prepareServerProcess alone — harmless at two instances.

[ingpaolodettori-dev]

Thanks for the quick turnaround. I re-checked the incremental diff 831d7c2 → 66fdfef (ddc02d1, f7196a4, and the merge of main) against the handler.

All 11 items from the previous review are resolved. I verified each reply against the code, not just the thread:

• /v1/runs 503 is now documented as the failed/saturated LeafResult (oneOf with Error).
• PASSTHROUGH codes are listed on every authenticated route. credential_unavailable now sends Retry-After.
• turnErrorCode emits stable internal_error / sandbox_unavailable codes. All route catch-alls go through internalError(), so exception text only reaches the log.
• The smaller items are also fixed: tenant scope, read_error, optional sessionId, aborted, /health Content-Type, workspaceReset.tier, the 501 wording, and the awaited server.close().

The merge with main is clean. git show --remerge-diff 66fdfef shows that the single server.ts conflict was resolved fully in favour of #469's rejectWorkloads, and there is no other hand edit. workload_not_found is gone from the doc, and /v1/runs 501 now lists workloads_unavailable, which matches handleRunLeafParsed.

The remaining comments are non-blocking (two suggestions, two nits).

Author: pdettori (MEMBER — maintainer)
Areas reviewed: Docs (OpenAPI), TypeScript (knative-server), tests, merge resolution
Agent/IDE config (.claude/.vscode): none
Commits: 4 commits (3 new since the last review), all signed-off: yes
CI status: passing on 66fdfef (check, lint, codeql, deploy-scripts, microvm-gates, proto, scans); k8s-kind-e2e still running at review time

[ingpaolodettori-dev]

suggestion: Adding Retry-After to credential_unavailable changes what mocactl shows. classify() (packages/mocactl/src/api/errors.ts:83) treats any harness 503 that has a Retry-After as retry-after, and that renders as "the harness has no capacity — retry in Ns". A control-plane-unreachable or bad-keyset failure now gets reported as a capacity problem, where it used to say "the harness is unavailable". A one-line fix on the client side: check err.code === 'sandbox_unavailable' (or test UNAVAILABLE.has(err.code) before the retry-after branch).

[ingpaolodettori-dev]

suggestion: The pre-first-frame path now sends a stable code, but the post-first-frame event: error frame a few lines below still puts the raw err.message in errorMessage. That's the leak the new turnErrorCode doc comment rules out (Redis URLs, tokens). Either scrub it the same way (log it, then send internal_error, or the stable code), or state in the doc that TurnStreamFrame.errorMessage is unfiltered.

[ingpaolodettori-dev]

nit: This says "an operator fix, not a retry, resolves it", but the response declares (and writeAuthError now sends) Retry-After. Soften it to something like "usually needs an operator fix; Retry-After is advisory", or skip the header on this route.

[ingpaolodettori-dev]

nit: After the merge this header still calls it "the context-service workload lifecycle". The entries below (and the yaml header) now say it's 501-refused (#455), so it's worth aligning this line too.