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

GET /api/v1/repos/:owner/:name/issues/:number carries an ETag and answers a repeat with 304 until the issue changes #9150

Merged opened this

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

Merged

`GET /api/v1/repos/:owner/:name/issues/:number` now carries a weak `etag` from the exact JSON sent, and a repeat with a matching `If-None-Match` gets an empty 304.

shipped production is serving it deploy 4183511

opened this

GET /api/v1/repos/:owner/:name/issues/:number now carries a weak etag from the exact JSON sent, and a repeat with a matching If-None-Match gets an empty 304.

  • Headers: the 200 and the 304 both carry etag, cache-control: private, no-cache and vary: authorization, cookie. The route reuses the tagged() helper that the comments route already uses in src/http/api/issues.ts.
  • Access first: the issue is loaded and access-checked before any tag is compared. A stranger asking for a sealed issue is refused whatever tag they hold, including *.
  • Changes: the tag is computed over the body that embeds the comments, so any change to the fields or comments in that body changes it. The test covers a new comment (200 with a different tag and commentsTotal 1). Closing, retitling, assigning, labelling and comment edits are not tested.
  • OpenAPI: the route's description now names the etag header and the 304, and lists 304 as an extra status (getApiV1ReposOwnerNameIssuesNumber.ts).
  • Index regeneration: I regenerated src/contract/openapi/routes/index.ts because test/openapi-routes.test.ts flagged it as stale. The regeneration also moves the Iterations export, which was out of order from #9145. That is a one-line reorder, not a behaviour change.

I added a "single issue ETag" block to test/issues-etag.test.ts. It covers 200 then 304, a comment giving 200 with a new tag, and the stranger's refusal.

bun x tsc --noEmit is clean. The new tests, the neighbouring issue and comment tests, and test/openapi-routes.test.ts pass; I ran those 38 files by hand and the one failure was the stale index above. bun scripts/gate.ts printed nothing; I did not check its exit code. scripts/fast.ts said the diff reaches 1105 test files, too many to run here, so I left the full suite to the forge's check.

I did not touch the pulls route or the MCP issue tool. A pull's number answers through this same route, so it gets the tag too.

Closes #9143

commented

GET /api/v1/repos/:owner/:name/issues/:number now carries a weak etag from the exact JSON sent, and a repeat with a matching If-None-Match gets an empty 304.

  • Headers: the 200 and the 304 both carry etag, cache-control: private, no-cache and vary: authorization, cookie. The route reuses the tagged() helper that the comments route already uses in src/http/api/issues.ts.
  • Access first: the issue is loaded and access-checked before any tag is compared. A stranger asking for a sealed issue is refused whatever tag they hold, including *.
  • Changes: the tag is computed over the body that embeds the comments, so any change to the fields or comments in that body changes it. The test covers a new comment (200 with a different tag and commentsTotal 1). Closing, retitling, assigning, labelling and comment edits are not tested.
  • OpenAPI: the route's description now names the etag header and the 304, and lists 304 as an extra status (getApiV1ReposOwnerNameIssuesNumber.ts).
  • Index regeneration: I regenerated src/contract/openapi/routes/index.ts because test/openapi-routes.test.ts flagged it as stale. The regeneration also moves the Iterations export, which was out of order from #9145. That is a one-line reorder, not a behaviour change.

I added a "single issue ETag" block to test/issues-etag.test.ts. It covers 200 then 304, a comment giving 200 with a new tag, and the stranger's refusal.

bun x tsc --noEmit is clean. The new tests, the neighbouring issue and comment tests, and test/openapi-routes.test.ts pass; I ran those 38 files by hand and the one failure was the stale index above. bun scripts/gate.ts printed nothing; I did not check its exit code. scripts/fast.ts said the diff reaches 1105 test files, too many to run here, so I left the full suite to the forge's check.

I did not touch the pulls route or the MCP issue tool. A pull's number answers through this same route, so it gets the tag too.

commented

GET /api/v1/repos/:owner/:name/issues/:number now returns a weak etag and answers a repeat If-None-Match with an empty 304. The failing check was a flaky test in test/api-pulls-check-preview.test.ts that this change doesn't touch. I've fixed the test and pushed it.

The failed check: "the list carries an ETag: a repeat is 304 until a pull's verdict changes" gave ids to its test runs with crypto.randomUUID(). The pulls list reads the greatest run id per pull as the latest run. About half the time, the "ok" run inserted mid-test sorted before the earlier "running" run, so the list never changed and the test got a 304 where it expected a 200. It failed the same way on main (a4606c6f). With the new ids it passed four runs in a row. Only the test changed; the pulls code didn't.

The earlier gate failure: the gate wanted a Try: line in a commit message, and the empty commit I added for it carries one. The message says to call the issue route for the etag, then repeat with If-None-Match for a 304.

bun x tsc --noEmit is clean. I didn't re-run the full suite here, so the forge's check on this push is the full verdict.

What the issue route does:

  • The 200 carries etag, cache-control: private, no-cache and vary: authorization, cookie.
  • Access is checked before any tag is compared, so a stranger asking for a sealed issue is refused whatever tag they hold, including *.
  • The tag covers the embedded comments, so a new comment gives a 200 with a new tag. I tested only the comment case; closing, retitling, assigning and labelling each change the body but have no test of their own.
  • The OpenAPI description names the etag header and the 304, and I regenerated the routes index.

Left out: the pulls route and the MCP issue tool. A pull's number answers through this same issues route, so it gets the tag too.

To try: curl -si -H "authorization: Bearer $TOKEN" <preview>/api/v1/repos/OWNER/NAME/issues/1, then repeat with -H 'if-none-match: <etag>' for a 304.

Changes requested — before the latest push

The route is wired correctly: access is checked in loadConversation before tags are compared, tagged() is reused, headers match, and the OpenAPI summary names the etag and 304. One round of fixes:

  1. Box 2 is only partly tested. The issue says closing, retitling, assigning, labelling and commenting each change the tag. Only commenting is tested. Add a case for each of the other four: PATCH or label or assign, then a GET with the old tag must give 200 with a different etag.
  2. The stranger test asserts not 304 and not 200, which a 500 would pass. Assert the exact refusal status (the one the sealed issue gives a stranger without a tag) and that the response carries no etag.
  3. The flaky-test fix in test/api-pulls-check-preview.test.ts and the index.ts reorder of Iterations are unrelated to this issue. Keep the reorder only if the openapi-routes test demands it, and put the flake fix in its own pull.
commented

I fixed points 1 and 2 and pushed. I kept the two items from point 3 in this pull; the reasons are below.

Done:

  • All five changes tested: test/issues-etag.test.ts now has a case for retitle, assign, label and close, alongside the existing comment case. Each one reads the tag, makes the change, then sends a GET with the old tag. It expects a 200 with a different etag.

- Label seeding: the label is created straight in the database, because creating one over the API needs admin scope and the test token is rw. - Closing a bug: closing needs a note, so the test sends one.

  • Stranger's refusal: the test now asserts the exact status. A stranger asking for the sealed issue with no tag gets 404, and with their own tag or * they get the same 404, with no etag header.

bun x tsc --noEmit is clean and the eight tests in test/issues-etag.test.ts pass. I did not re-run the full suite.

Where I disagree on point 3:

  • Flake fix: I'd rather keep it here. The forge's check failed this pull on that test, and it also fails on main (a4606c6f). The check decides whether this lands, so moving the fix to another pull would leave this one red until that one merges. Say so if you still want it split out, and I'll revert it from this pull.
  • index.ts reorder: I kept it because test/openapi-routes.test.ts fails without the regenerated index. Regenerating also moved the Iterations export, which was already out of order from #9145, so the reorder isn't mine to choose.
Approved

Earlier asks are met. All five changes (retitle, assign, label, close, comment) now each have a test showing a new tag and a 200. The stranger test asserts the exact 404 and no etag header, for no tag, their own tag and *. Access is checked before tags are compared. Non-blocking: the flake fix in api-pulls-check-preview and the index reorder are unrelated to the issue, but they are small and keep the check green.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs