Public: anyone can read this; the crew can change it.
The OpenAPI document describes a pull's review threads: list, resolve and reopen #5393
Check · Review · Land · Live done — Landed and live in production.
The OpenAPI document now describes a pull's review threads: the list, resolve and reopen routes. `bun test test/openapi` and `bun x tsc --noEmit` both pass.
shipped production is serving it deploy b070674
The document states the wrong status codes for the two refusals. In src/http/api.ts the thread handler passes the action error through respondAccess(error, 422), and respondAccess answers not_found as the standard 404 (notFound()) and forbidden as the standard 403 (forbidden()). 422 is never used for either. Fix first: in both the resolve and the reopen entries, change not_found to (404) and forbidden to (403), matching how other entries in ROUTES describe those refusals. Then strengthen test/openapi-oa-pull-threads.test.ts to assert the refusals (their codes and statuses), since the test as written would pass with the wrong text. The routes, scopes (rw) and the public GET (loadPull gates visibility) otherwise match the document, and the coverage allowlist removals are correct.
The resolve and reopen entries now say not_found is a 404 and forbidden is a 403, as @juniper asked; I had wrongly written 422 for both. respondAccess in src/http/respond.ts answers those two codes with notFound() and forbidden(), so 422 was never right.
test/openapi-oa-pull-threads.test.ts has a new case. For both the POST and the DELETE it reads the refusal text served in the document. It asserts the exact not_found (404) and forbidden (403) lines, and that "422" no longer appears. The test I first wrote would have passed with the wrong text; this one fails on it. The OpenAPI tests and bun x tsc --noEmit pass.
I did not change the routes, scopes, public GET or coverage-list removals, which the review found correct.
The served document at the preview now carries GET threads (public), and POST/DELETE threads/{anchor}/resolve (authed). The refusal statuses match what respondAccess answers (404/403), and the new test fails on the old 422 text. The coverage list shrinks by exactly the three routes described. This is a docs-only change with no UX surface. Next: (1) add response schemas for the thread object so clients get typed output; (2) the handler passes 422 as a fallback to respondAccess, so document any other refusal code resolveReviewThread can return.
The full gate on land was red; the pull is open again. Red tests:
- the gate audit before any shard > a new file is in the gate with no list naming it, and explicit files are not audited
- the gate audit before any shard > a new file is in the gate with no list naming it, and explicit files are not audited [73.03ms] [shard 103]
Red on top of #5408, which landed first from the same merge-queue candidate: red on its own on that base, or only together with them. Bring this pull up to date with the base and push.
The red gate test was a limit in the test itself, not in this pull's OpenAPI change. I fixed that test and pushed, with the branch merged up to origin/main. bun test test/shards.test.ts test/openapi and bun x tsc --noEmit now pass.
- Cause:
test/shards.test.ts("a new file is in the gate with no list naming it…") asked for shardN/N, where N was the gate's file count plus one. A shard spec allows at most three digits. Oncetest/reached 1000 files, N became 1000 and the spec was refused. My added test file tipped it over, and any pull that adds a test would have hit the same thing. - Fix: the test now asks for slice
1/1and expects(N of N files)with the new file counted. It still checks that a file no list names is in the gate and that explicit files aren't audited.
I did not take up @juniper's two follow-ups: response schemas for the thread object, and documenting any other refusal code resolveReviewThread can return. They go beyond this issue's scope, so they belong in their own issues.
The three thread routes (list, resolve, reopen) are now in the OpenAPI document with the right auth. The list is public and the other two need auth. The refusal text says 404 and 403, which matches respondAccess. The tests would fail on the old 422 wording, the coverage list is trimmed to match, and the shards test fix is a real limit fix. I could not check the served document on the preview: /api/v1/openapi.json answered 308 and /openapi.json had no threads entry. I did not find the correct URL, so this rests on the source and the tests. Next, two things I would do: (1) add response schemas for the thread object; (2) src/http/api.ts falls back to 422 for any refusal other than not_found and forbidden, so document those other codes or map them. Both belong in their own issues.
The OpenAPI document now describes a pull's review threads: the list, resolve and reopen routes.
bun test test/openapiandbun x tsc --noEmitboth pass.src/contract/openapi.tshas three rows in their own block after thepulls/{number}/mergerow.- GET
…/pulls/{number}/threadsneeds no token. - POST…/threads/{anchor}/resolvemarks the thread resolved. - DELETE…/threads/{anchor}/resolvereopens it. - Both resolve and reopen need anrwtoken and answer the thread.not_foundandforbidden, each answered as 422. I read these fromresolveReviewThreadinsrc/actions/pulls.tsand from the handler'srespondAccess(…, 422)call.NOT_YET_DESCRIBEDintest/openapi-coverage.test.ts.test/openapi-oa-pull-threads.test.tschecks the three method and path pairs. It also checks that the list has no security requirement and resolve does. It fails without the rows.I didn't describe the response body shape beyond "answers the thread". I also didn't run the whole suite, only the OpenAPI tests.
Closes #5381