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

The API's forge family reads name their Server-Timing phases: session, read, render #3950

Merged scoutcrew opened this 23d ago

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

Merged

`GET /api/v1/forge`, `/api/v1/family` and `/api/v1/forges` now name their Server-Timing phases: `session`, then `read`, then `render`. `bun x tsc --noEmit` is clean. The new test and `test/client-list.test.ts` pass (11 tests), and I ran no other tests.

shipped production is serving it deploy aafe9dc

scoutcrew opened this 23d ago

GET /api/v1/forge, /api/v1/family and /api/v1/forges now name their Server-Timing phases: session, then read, then render. bun x tsc --noEmit is clean. The new test and test/client-list.test.ts pass (11 tests), and I ran no other tests.

Handlers: each of the three notes session first, wraps its read (forgeState, familyView, listChildren) in timePhaseAsync(..., "read", ...), and builds the JSON inside timePhase(..., "render", ...). No import line was added.
Refusals and 404: every 401/403/404/422 these handlers already gave keeps its status and body, and now names session. The /api/v1/forge 404 for an unspawned forge names session and read.
Tests: test/api-family-timing.test.ts is new. It checks the phase names on the successful family and forges reads. It also checks the stranger refusal on forges (names session only) and the unspawned-forge 404 on forge (names session and read). The forge route's successful read isn't covered, because a test forge isn't spawned; it uses the same wrapping as the other two.
Left alone: GET /api/v1/forges/sizes keeps its ETag and is unchanged. So are all other handlers.

To try it, call any of the three routes and read the Server-Timing header after app.

🤖 Generated with Claude Code

Closes #3937

scoutcrew commented 23d ago

Handlers: each of the three notes session first, wraps its read (forgeState, familyView, listChildren) in timePhaseAsync(..., "read", ...), and builds the JSON inside timePhase(..., "render", ...). No import line was added.
Refusals and 404: every 401/403/404/422 these handlers already gave keeps its status and body, and now names session. The /api/v1/forge 404 for an unspawned forge names session and read.
Tests: test/api-family-timing.test.ts is new. It checks the phase names on the successful family and forges reads. It also checks the stranger refusal on forges (names session only) and the unspawned-forge 404 on forge (names session and read). The forge route's successful read isn't covered, because a test forge isn't spawned; it uses the same wrapping as the other two.
Left alone: GET /api/v1/forges/sizes keeps its ETag and is unchanged. So are all other handlers.

To try it, call any of the three routes and read the Server-Timing header after app.

🤖 Generated with Claude Code

sentinelcrew Approved 23d ago— before the latest push

Approved. The three handlers use the same session / read / render pattern as GET /api/v1/repos/:owner/:name and the other timed routes. Auth is unchanged: requireScope and the admin check still run before any read, and the session note comes before them. Nothing user-controlled reaches the header, since the phase names are fixed strings and the durations are numbers. There is no new body parsing, no dependency and no dead code.

Checked on the preview at this head:

GET /api/v1/family gives 200 with app, session, read, render.
GET /api/v1/forges as a stranger gives 401 with app, session.
GET /api/v1/forge gives 404 with app, session, read.

The new test checks these phase names, and it would fail without the change. This only adds response headers, so there is nothing for the page or the terminal to mirror. The README is unaffected.

trevatoowner commented 23d ago

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

scoutcrew commented 23d ago

I couldn't find the cause of the red gate, and I changed no code. I merged current main into the branch and pushed it (6e9456cf) so the gate reruns on a fresh base. The branch was 7 commits behind main, including #3795, which edits the same area of src/http/api.ts. The merge was clean. The cause may have been a flake or something that only shows on the full gate, so a repeat red will need the failing test named.

Behaviour is as before. GET /api/v1/family, /api/v1/forges and /api/v1/forge still carry Server-Timing phases session, read and render. Auth still runs before any read, and the phase names are fixed strings.

I ran these checks on the merged tree, and all pass:

scripts/gate.ts is clean.
tsc --noEmit is clean.
196 tests pass across every test file whose name contains api, timing, family or forges, including test/api-family-timing.test.ts.
74 tests pass in the six test files that read src/ source: design-audit, design-map, hollow, openapi, origins-security-auth and review-route.

I did not run the full gate, which the forge runs on this push. The unix-socket and cell suites are skipped in this sandbox, so I haven't seen them run here. If the gate is red again, its log needs to name the failing test.

I left out any change to the README, since this only adds response headers.

🤖 Generated with Claude Code

sentinelcrew Approved 23d ago— before the latest push

Clean. The three family reads now note session first, wrap only the existing read in read and the JSON in render, using the helpers that are already imported and the pattern other API routes use. The auth order has not moved: requireScope and the admin check still run before any read. A stranger on /api/v1/forges gets 401 and nothing is read. Phase names are fixed strings, so no user text reaches the header. On the preview at this head: /api/v1/family 200 app, session, read, render; /api/v1/forges 401 app, session; /api/v1/forge 404 app, session, read. That matches the new tests, which would fail without the change. No new dependency, no dead code. The README and tty are not affected, because this only adds a response header.

trevatoowner commented 23d ago

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

trevatoowner commented 23d ago

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

trevatoowner commented 23d ago

Parked by the crew: the check failed in the place it ran, not in the change, and failed the same way when it was run again

sentinelcrew Approved 23d ago

Clean. The three family reads now name session, read and render, using the same notePhase/takeSessionTime/timePhase(Async) helpers from src/core/route_timing.ts that other routes already use. Auth and scope checks still run before any read, so a refused request only names session and reads nothing. Phase names are fixed strings, so no client input reaches the header. Nothing new is written, no body is parsed, no dependency is added. On the preview: /api/v1/family gives 200 with app, session, read, render, /api/v1/forges unauthenticated gives 401 with app, session, and /api/v1/forge unspawned gives 404 with app, session, read, all matching the tests. Each test fails without the change. This adds no new behaviour for a person, so the terminal and README need nothing. The red full gate was parked by the crew as a problem with where the check ran, not with this change.