[cwiklik]

The six inline review notes on #713, which merged on the approval before they were walked. None of them changes behavior on the claim path; four are doc corrections and one is a guard clause. Kept separate from #389 so the drain loop does not bury them.

The doc claims were wrong, in two places

openapi.yaml's 404 description and the comment at errors.ts both said the claim refusals are "deliberately indistinguishable" and give "no existence oracle" (@clawgenti, @ingpaolodettori-dev). That does not hold as written. turn_not_found, session_not_found and 409 turn_not_claimable are three different codes, and the last one's message names the state — only the status matches on two of the three. A worker legitimately needs them apart to record why it dropped a queue entry.

Both now say what is actually true, which is narrower and sufficient: every caller on this route authenticated with the deployment-wide SH_EXCHANGE_TOKEN, and a successful call hands back the credential itself, so no refusal reveals anything the same caller could not already obtain. Both also note this is not spec §8.1's 404-rather-than-403 rule, which governs the owner-scoped /v1 routes a session token reaches. openapi.yaml:310's existence-oracle claim is about that rule and is correct — left alone.

This also resolves @clawgenti's note that the refusal paths pass turn.sessionId into the error body. That behavior stays: a refused worker wants the session id in its log line, and the caller already holds the exchange token. The fix was the doc.

The turn id is now bounded before Redis

POST /internal/turn-credentials took any string as its turnId and built a key from it — HGETALL sh:cp:turn:<input>, with TurnStore.tag free to append :${keyTag} besides (@ingpaolodettori-dev on handlers.ts:1109). submitTurn is the only minter and its ids are always t-${randomUUID()}, so anything else is refused invalid_request before Redis is touched.

This closes no hole on its own — get only reads, and the handler re-checks the record's owner against its session. It bounds the key space a shared-bearer route can be made to address. 400 rather than 404 because this caller gets the credential on success, so a distinguishable "you sent nonsense" tells it nothing new and names the actual bug.

isMintedTurnId lives in turn-record.ts beside turnKey rather than reusing sandbox-handlers.ts's UUID_RE, which is unexported and does not carry the t- prefix that is this module's own minting contract. The owner-scoped /v1 turn routes deliberately do not use it: there a malformed id must stay a 404, indistinguishable from another subject's turn, or the error code becomes the oracle assertOwner exists to deny.

Verified load-bearing by mutation — with the gate removed, exactly one test fails and it is the new one. server.test.ts's unknown-turn case now uses a well-formed id; left as t-nobody the gate would answer first and it would stop covering the 404 at all.

Two smaller doc items

• http-api.md's code list gains 429 async_quota_exhausted and a 501 group (async_tenancy_unsupported, workload_runs_unavailable) — codes the same 1.3.0 changelog introduced.
• http-api.md's "a turn already running keeps its model" gains the queued-async-turn caveat. A queued turn is a next turn: it picks up a PATCHed model whenever a worker claims it, because the exchange reads model from the live session. @ingpaolodettori-dev also asked for an explicit checkbox to pin model/sandboxTier on the record — that goes on Coding-agent subagents B3: grants minted at dequeue from the turn record (no bearer in the queue) #388's M2 list, since pinning means adding both to the turn record.

Verification

• packages/control-plane suite: 1157 passed, 3 skipped, against real Redis on 6379. No Errors line.
• make typecheck: clean. make check-links: 198 files, 0 broken links.
• Prettier clean on all seven changed files.
• make lint fails two hooks for environmental reasons only, neither a finding against this diff: hadolint needs the Docker daemon, and the prettier hook's own pnpm env writes an allowBuilds stub into pnpm-workspace.yaml under pnpm 11 (reverted, not shipped).

Refs #713, #388

Assisted-By: Claude Code

[clawgenti]

Summary

Follow-ups to #713. The OpenAPI 404 description and the errors.ts comment now correctly say that the claim refusals are distinguishable. They also make clear that this is not the §8.1 404-not-403 rule. POST /internal/turn-credentials now refuses any turnId outside the minted t-<uuid> shape with 400 invalid_request before Redis is read. MINTED_TURN_ID_RE matches what submitTurn mints with the production newId (randomUUID, main.ts:523), and a contract test pins it against openapi.yaml. The coupling test uses the production newId, and the 404 case in server.test.ts was moved to a well-formed id, so the 404 path is still covered. Editing the 1.3.0 changelog entry is fine: the latest release (v0.10.0) still ships API 1.2.0. No blocking issues.

One nit outside the diff (no inline anchor): the comment at turn-exchange.ts:58 still says the 404 is "Shaped like every other not-found answer". That is the same framing this PR corrects in errors.ts. Also, exchangeTurnCredential is re-exported from index.ts without the shape gate, so a direct library caller skips it. That is fine as long as the gate is meant to be HTTP-only. All checks pass. Ready for human review.

Author: cwiklik (CONTRIBUTOR — returning external; every changed file was read in full)
Areas reviewed: TypeScript (control-plane src + tests), Docs/OpenAPI, Security
Agent/IDE config (.claude/.vscode): none
Commits: 3 commits, all signed-off: yes
CI status: passing (17/17)

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[cwiklik]

Both nits addressed in ae71734 (comments only, no behavior change).

turn-exchange.ts:58 — correct, and it was the last copy of the framing this PR
set out to fix. It now says the 404 is deliberately distinguishable from the other
claim refusals and is not the §8.1 404-not-403, with a pointer to openapi.yaml's
404 description rather than a fourth restatement of the argument.

exchangeTurnCredential re-exported without the gate — intended, and the doc
comment now says so instead of leaving a reader to infer it. The gate bounds caller
input reaching a Redis key on a route authenticated by one deployment-wide bearer;
that is a property of the route, not of the function. A library caller has already
passed whatever gate it answers to, and repeating the check there would only convert
its own 404 into a 400. Grepped packages and harness: nothing outside the package
imports it today — the only callers are handlers.ts and the package's own tests.
If a second route ever reaches it, the gate goes on that route too.

Control-plane suite: 1158 passed, 3 skipped against real Redis, no Errors.
tsc and prettier clean.

Assisted-By: Claude Code

[clawgenti]

Clawgenti Code Review

Summary

No must-fix issues. The shape gate goes in at the right point: the handler checks it after the empty check and before exchangeTurnCredential touches Redis. MINTED_TURN_ID_RE matches what submitTurn mints (t-${randomUUID()}, main.ts:523), and the contract test pins the OpenAPI pattern to it. The corrected refusal docs now match the codes in errors.ts, and the new 429/501 entries in http-api.md are all real codes in CP_ERROR_CODES. One suggestion inline: the coupling test restates the production newId instead of using it.

Author: cwiklik (CONTRIBUTOR — returning external; I read every changed file in full)
Areas reviewed: TypeScript (control-plane src + tests), OpenAPI YAML, Docs
Agent/IDE config (.claude/.vscode): none
Commits: 4 commits, all signed off: yes
CI status: passing (17/17)

All checks pass. Ready for human review.

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[clawgenti]

suggestion — This test passes newId: () => randomUUID() itself, so it restates the production wiring instead of exercising it. If main.ts:523 ever switches newId to a different generator (nanoid, ulid, a prefixed id), this test still passes while every real claim 400s. That is the exact dead-letter failure the comment describes, and it contradicts "the only test that fails when it breaks". To make the coupling real, put the production id factory in one exported place (e.g. export const newTurnUuid = randomUUID beside MINTED_TURN_ID_RE), have main.ts import it, and use it here.

[cwiklik]

Fixed in 13fb679. The finding is correct, and it contradicted the test's own comment —
() => randomUUID() restated the wiring instead of exercising it, so a switch to nanoid
or a prefixed id would have kept it green while every claim 400s.

One change to the proposed shape: the binding is defaultNewId in handler-util.ts,
not newTurnUuid beside MINTED_TURN_ID_RE. newId has three shape-dependent
consumers and only one is turns — session ids are their own leafSessionId sanitisation
(gap #10), and sandboxId needs 26 random hex and refuses a shorter source. Putting
it in turn-record.ts would move the session and sandbox contract into the turn module.
handler-util.ts is also the existing home for this shape: defaultAuditLog and
defaultRunKubectl are the same thing for their CpDeps fields.

depsFromEnv now wires it — the only production CpDeps producer; I grepped packages
and harness for another newId: and there is none. Three tests inject the same
binding rather than rebuilding it: the coupling test, turn-e2e (which claims every
turn it submits, and was the suite silently carrying this invariant), and
turn-record's accepts exactly what submitTurn mints.

Verified by mutation — defaultNewId → nid-${randomUUID()} fails 8 tests: the
coupling test, all six turn-e2e tests, and the turn-record unit test. Before this
commit the same mutation left every one of them green, which is precisely the gap.

Control-plane suite: 1158 passed, 3 skipped against real Redis, no Errors. tsc and
prettier clean.

[clawgenti]

Clawgenti Code Review

Summary

No must-fix issues. The new head commit (13fb679) takes the earlier suggestion. turn-handlers.test.ts, turn-record.test.ts and turn-e2e.test.ts now inject defaultNewId, which is the same binding main.ts wires. Before, they restated () => randomUUID(). So if the production minter drifts from MINTED_TURN_ID_RE, the coupling tests now fail instead of staying green. Moving randomUUID into handler-util.defaultNewId doesn't change behavior. submitTurn still mints t-${deps.newId()}, and the shape gate in handlers.exchangeTurnCredential still runs after the empty check and before Redis. The 429/501 codes added to http-api.md all exist in CP_ERROR_CODES with those statuses. The earlier nit on the turn-exchange.ts not-found comment was fixed in ae71734.

Author: cwiklik (CONTRIBUTOR — returning external; I read every changed file in full)
Areas reviewed: TypeScript (control-plane src + tests), OpenAPI YAML, Docs, Security
Agent/IDE config (.claude/.vscode): none
Commits: 5 commits, all signed off: yes
CI status: pending (14 passing; check, deploy-scripts, k8s-kind-e2e still running)

All checks pass. Ready for human review.

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[ingpaolodettori-dev]

Summary

These are follow-ups to #713. The new head 13fb679 adds one test/refactor commit on top of ae71734, and it fixes the coupling-test nit from the previous round. main.ts now wires an exported defaultNewId (handler-util.ts) as the production CpDeps.newId, following the existing defaultAuditLog/defaultRunKubectl pattern. The three shape-dependent tests (turn-handlers coupling, turn-e2e, turn-record) inject that same binding, so a production switch to nanoid or a prefixed id now fails them. Runtime behaviour is unchanged (defaultNewId is still randomUUID()). Imports are clean: randomUUID was dropped from main.ts and turn-handlers.test.ts, and the files that still use it keep it. The f206e8b/ae71734 substance still holds. One nit is left from the previous round, and nothing blocks merge.

Author: cwiklik (MEMBER — maintainer)
Areas reviewed: TypeScript (control-plane src + tests), Docs/OpenAPI
Agent/IDE config (.claude/.vscode): none
Commits: 5 commits, all signed-off: yes (DCO passing)
CI status: passing so far; check/deploy-scripts/deploy-scripts-bash32/k8s-kind-e2e still in progress at draft time

Assisted-By: Claude Code

[ingpaolodettori-dev]

nit (carried over from the previous round, not yet addressed): The 1.3.0 "New error codes" bullet still leaves out workload_runs_unavailable (501). It is already in openapi.yaml's code enum and in http-api.md's 501 group, which the PR body describes as codes "the same 1.3.0 changelog introduced". Add it here.

[cwiklik]

Fixed in 330b07a. The bullet now reads:

- New error codes `turn_not_cancellable` (409, cancelling a turn that is not queued),
  `turn_not_claimable` (409, claiming a cancelled or terminal turn), `turn_not_found` (404),
  `async_quota_exhausted` (429, `MOCA_ASYNC_QUEUED_PER_OWNER` non-terminal turns already held),
  `async_tenancy_unsupported` (501, async under `MOCA_TENANCY=multi`) and
  `workload_runs_unavailable` (501, a submission naming a `workloadId`). Either 501 is answered
  before the quota is touched, so a request that can never be served is never reported as a 429 and
  leaves no record, no queue entry and no quota slot behind (§4.2).

Verified it belongs to 1.3.0 rather than to an earlier version before adding it: in the control
plane the only route that emits the code is submitTurn (turn-handlers.ts:104), which this release
added. knative-server/src/server.ts:972 answers it too, but that is the harness's own data-plane
document, not this one.

The trailing sentence is deliberate. Three of 1.3.0's new codes describe the same route — two 501s
and a 429 — and the order they are answered in is specified rather than incidental (spec §4.2, and
submitTurn's own doc block says so): both 501s precede reserveOpenSlot, so a request that can
never be served is never reported as a quota problem. That is the fact which makes three codes on one
route unambiguous, and the changelog is where an API consumer would look for it.

One thing I did not do, and want to flag rather than quietly skip: nothing pins this list against
CP_ERROR_CODES, which is why the omission survived two rounds. I considered a contract test in the
spirit of the openapi-contract.test.ts ones, and decided against it here — codes accumulate across
releases, so the test would need to know which version introduced each code, and that attribution
exists nowhere in the tree. Inventing it in a test would be a third source of truth rather than a
check on the two. Better as its own issue; say the word and I will open one.