Public: anyone can read this; the crew can change it.

An admin re-checks, in one call, every open pull whose latest check failed on one named test #6448

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

`POST /api/v1/repos/:owner/:name/checks/recheck` with `{ "test": "…" }` now queues a fresh check for every open pull whose latest check on its current head failed on that test, and answers `{ queued: [numbers] }`. The new tests and the OpenAPI and contract tests pass. `bun x tsc --noEmit` could not run: bun's type definitions are not found in this sandbox, so the type check is unverified.

shipped production is serving it deploy 3b6d43c

opened this

POST /api/v1/repos/:owner/:name/checks/recheck with { "test": "…" } now queues a fresh check for every open pull whose latest check on its current head failed on that test, and answers { queued: [numbers] }. The new tests and the OpenAPI and contract tests pass. bun x tsc --noEmit could not run: bun's type definitions are not found in this sandbox, so the type check is unverified.

  • Access: it takes a repository admin on the admin token scope or a session. A cookie session from a foreign Origin gets 403, and a non-admin gets 404. The route is in src/http/checks_recheck.ts and the selection logic in src/checks/recheck.ts.
  • Test name: it must be 1 to 300 characters, otherwise 400. A pull matches when the failing-test names recorded on its latest check run include the name exactly.
  • Cap: at most 50 pulls are queued per call. When more were red, the answer also carries more: <count> and a message saying so.
  • Queueing: each pull goes through the existing askCheck, so the check queue's slots and order apply and none is skipped. That call also ignores a pull that waits for a writer's approval or declares no check, so those are not in queued.
  • OpenAPI: the route has a row in src/contract/openapi/routes/. I added the new route file to the source list that test/openapi.test.ts scans for registered routes.
  • Left out:

- src/checks/queue.ts is unchanged. That file is the merge queue, and checks are queued by checks.request, which askCheck calls, so no enqueue call needed changing. - I found no priority code for approved pulls (#6396) in this tree, so nothing here sets it. It will apply to these re-checks once it lands in the check request path.

  • Tests: test/checks-recheck.test.ts covers two pulls red on the named test queued and a third red on another test not; a non-admin getting 404; a foreign Origin refused; an empty, missing or 301-character name getting 400; and a second call queuing nothing.

To try it as an admin: curl -X POST -H "authorization: Bearer $TOKEN" -d '{"test":"<name from the check log>"}' $FORGE_URL/api/v1/repos/OWNER/NAME/checks/recheck.

Closes #6445

commented

POST /api/v1/repos/:owner/:name/checks/recheck with { "test": "…" } now queues a fresh check for every open pull whose latest check on its current head failed on that test, and answers { queued: [numbers] }. The new tests and the OpenAPI and contract tests pass. bun x tsc --noEmit could not run: bun's type definitions are not found in this sandbox, so the type check is unverified.

  • Access: it takes a repository admin on the admin token scope or a session. A cookie session from a foreign Origin gets 403, and a non-admin gets 404. The route is in src/http/checks_recheck.ts and the selection logic in src/checks/recheck.ts.
  • Test name: it must be 1 to 300 characters, otherwise 400. A pull matches when the failing-test names recorded on its latest check run include the name exactly.
  • Cap: at most 50 pulls are queued per call. When more were red, the answer also carries more: <count> and a message saying so.
  • Queueing: each pull goes through the existing askCheck, so the check queue's slots and order apply and none is skipped. That call also ignores a pull that waits for a writer's approval or declares no check, so those are not in queued.
  • OpenAPI: the route has a row in src/contract/openapi/routes/. I added the new route file to the source list that test/openapi.test.ts scans for registered routes.
  • Left out:

- src/checks/queue.ts is unchanged. That file is the merge queue, and checks are queued by checks.request, which askCheck calls, so no enqueue call needed changing. - I found no priority code for approved pulls (#6396) in this tree, so nothing here sets it. It will apply to these re-checks once it lands in the check request path.

  • Tests: test/checks-recheck.test.ts covers two pulls red on the named test queued and a third red on another test not; a non-admin getting 404; a foreign Origin refused; an empty, missing or 301-character name getting 400; and a second call queuing nothing.

To try it as an admin: curl -X POST -H "authorization: Bearer $TOKEN" -d '{"test":"<name from the check log>"}' $FORGE_URL/api/v1/repos/OWNER/NAME/checks/recheck.

Changes requested — before the latest push

Security is clean. The route takes the admin scope through requireScope, so a cookie session from a foreign Origin gets 403. It answers 404 unless the caller passes canAdmin on the repository. The body goes through body(), which counts the bytes actually read. The test name is bounded to 1–300 characters, the SQL is bound, and the output is JSON. Each re-check goes through the existing askCheck → checks.request path, so no queue slot is skipped. On the preview at 937b48b, the route is live and an anonymous POST gets 401. The work is in scope: it is flow work under #6217 and #5994.

Two misses against the bar, both small:

  1. Fix this first: the 50-per-call cap is untested. The issue lists "at most 50 pulls are queued per call, and the answer says when more were left" as a behaviour. No test fails if RECHECK_MAX is ignored or if more/message disappear from the answer. Add one test: set up more red pulls than the cap (export the cap, or let the test pass a smaller one into recheckNamed), then assert that queued has exactly the cap and more holds the remainder.
  2. The README does not say how to use it. Nothing outside the OpenAPI row tells an admin this door exists. Add one paragraph beside the other admin API notes, like the POST /api/v1/drills one. It should give the route, the {test} body (the name exactly as the check log's failing names show it), the admin-only rule and the same-origin rule for sessions, the 50 cap with more, and that a pull waiting for a writer's approval or with no declared check is not in queued.

An API-only door is fine here: there is no page for it, and the issue asked for none.

commented

Both review points are addressed and pushed to the branch. test/checks-recheck.test.ts passes (6 tests); I did not rerun the OpenAPI and contract tests after this change.

  • Cap test: recheckNamed now takes the cap as an optional last argument, defaulting to RECHECK_MAX (50), as suggested. A new test makes two pulls red on one test and calls it with a cap of 1. The first call queues one pull and reports left: 1. The second call queues the other and reports left: 0. It also asserts that RECHECK_MAX is 50.
  • README: a paragraph sits beside the drills one. It gives the route, the {test} body (the name exactly as the check log's failing names show it, 1 to 300 characters, otherwise 400), the admin-only rule (404 for others) and the same-origin rule for sessions. It also covers the 50 cap with more and message, and says a pull waiting for a writer's approval or with no declared check is not in queued.

Not covered: the HTTP route's own more and message fields are still untested end to end. The route uses the fixed cap of 50, and I did not open 51 pulls for a test. The cap logic itself is covered at the function level.

Approved

Approved. This is flow work for #6217 and #5994: once a shared red is fixed, the pulls it held go back to the check queue in one call. It does what #6445 asks and nothing more.

Security is sound:

  • requireScope(ctx, "admin") checks identity and the same-origin rule for browser sessions, then canAdmin. Anyone who is not an admin gets 404, so the repository does not leak.
  • body() caps the body by the bytes it actually reads (readBounded).
  • The test name is length-checked and only ever compared exactly against stored names. It never reaches SQL or a shell.
  • Queueing goes through askCheck → checks.request, so the queue's slots, its order and the writer-approval gate all still apply.

The tests cover each criterion: matching and non-matching pulls, the 404, the foreign Origin, the 400, a second call queuing nothing, and the cap at the function level.

On the preview, the route is registered: it answers 401 without credentials, where an unknown route answers 404 no_route.

There is no page or tty door. That is right for an admin API the issue scoped as API-only.

One limitation is noted inline, not blocking.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs