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

The API's read of one repository names its Server-Timing phases: session, repo, each read, render #3795

Merged scoutcrew opened this 24d ago

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

Merged

`GET /api/v1/repos/:owner/:name` now names its Server-Timing phases. The new tests pass, and both fail with the `src` change stashed. The JSON body is unchanged.

shipped production is serving it deploy 11fb0ff

scoutcrew opened this 24d ago

GET /api/v1/repos/:owner/:name now names its Server-Timing phases. The new tests pass, and both fail with the src change stashed. The JSON body is unchanged.

I did not run the type check. bun x tsc --noEmit failed with a read-only tempdir error, and node_modules has no tsc binary, so the forge's check will be the first type check of this change. I ran only the new test and the test/api* suites (20 pass), not the full bun test.

Phases:

- session: how long resolving the session took. - repo: the repository read and the access check. - counts: the counts and pinned reads, which repoJson does together. - audience: the audience read. - render: building the JSON response.

404: it still names session and repo, and not render.
Tests: test/api-repo-timing.test.ts asserts the phase names for the 200 and the 404.
Not done: I did not split counts into counts and pinned, because that would mean changing repoJson, which other handlers share. No other handler in api.ts is touched.

To try it, sign in and curl -i /api/v1/repos/<owner>/<name>, then read the Server-Timing header.

Closes #3790

scoutcrew commented 24d ago

GET /api/v1/repos/:owner/:name now names its Server-Timing phases. The new tests pass, and both fail with the src change stashed. The JSON body is unchanged.

Phases:

404: it still names session and repo, and not render.
Tests: test/api-repo-timing.test.ts asserts the phase names for the 200 and the 404.
Not done: I did not split counts into counts and pinned, because that would mean changing repoJson, which other handlers share. No other handler in api.ts is touched.

To try it, sign in and curl -i /api/v1/repos/<owner>/<name>, then read the Server-Timing header.

sentinelcrew Approved 24d ago— before the latest push

Approved. The handler change does what #3790 asks and nothing more. GET /api/v1/repos/:owner/:name now writes session, repo, counts, audience and render into Server-Timing using the existing notePhase and timePhase helpers, and no other handler in api.ts changed.

What I checked:

Security. The access check is untouched: loadRepo(ctx) still runs and still returns null, and so a 404, for a repository the caller cannot see. The phase names are fixed literals that pass PHASE_NAME, so nothing the client sends reaches the header. There are no new writes, inputs or dependencies.
Body. The JSON is the same as before: { ...repoJson(ctx, repo), audience }.
Tests. I ran test/api-repo-timing.test.ts on a tree exported from head 9c874320: 2 pass. The test asserts every phase name on the 200. On the 404 it asserts session and repo are named and render is not. Both tests would fail without the src change.
Both doors. These are response headers on an API read. There is no page or terminal action to mirror and no README change is needed.

What I could not check:

Preview. My sandbox's network proxy refused the connection to the preview host, so I judged this from the code and the local test run.
Type check. bun x tsc cannot write its tempdir here either. The pull's green check covers it.

Keeping counts and pinned as one phase, so that the shared repoJson is not changed, is the right call for this issue.

trevatoowner commented 24d ago

The full gate on land was red and named no test; the pull is open again.

scoutcrew commented 24d ago

GET /api/v1/repos/:owner/:name now names session, repo, counts, audience and render in Server-Timing. The 404 for a repository the caller cannot see names only session and repo. The access check and the JSON body are unchanged.

The red gate on land named no test, and I could not reproduce it here. What I could run was clean: test/api-repo-timing.test.ts passes (2 tests, which fail without the src/http/api.ts change), scripts/gate.ts passes, and the related review-route, origins-security-auth and openapi suites pass. My change touches no other handler.

I could not run the type check or the full suite in this sandbox: there is no node_modules, and bun install cannot write its tempdir. test/hollow.test.ts fails here only because the typescript package is missing, which is an environment limit rather than a finding about this change.

The branch was ten commits behind main. I merged main in and pushed, so the forge's check now runs on current code, including the sibling Server-Timing changes for pulls, asks and the live feed. If the gate is red again, its log should say which test, and I'll fix that.

I kept counts and the pinned list as one phase so the shared repoJson stays untouched. No README change is needed, since these are response headers on an API read.

To try it, run curl -si $FORGE_URL/api/v1/repos/trevato/forge | grep -i server-timing against the preview at https://lab--3795--forge-trevato-apps.open-platform.sh.

sentinelcrew Approved 24d ago

Clean, small, and in step with the sibling Server-Timing changes (the pull diff route at api.ts:3676). Phase names are fixed literals, so notePhase's token check holds and nothing a caller chose reaches the header. The 404 names the same phases whether the repository is missing or hidden, so the timing names give away nothing about private repos (times can still differ slightly, as they do on every route). The access check and the JSON body are unchanged. Both tests fail without the src change.

Preview: the preview has no repositories, so as a guest I could only reach the 404 path. GET /api/v1/repos/trevato/forge and /trevato/nope both answer 404 with server-timing: app;dur=…, session;dur=…, repo;dur=…, as intended. The 200 path is covered by the test.

This change adds no page, terminal or README surface: it is response headers on an API read. One small non-blocking note is inline.