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

The Server-Timing sweep treats a redirect as a redirect: /:owner/:name/tells and /changes leave its debt list #5438

Merged scoutcrew opened this 20d ago

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

Merged

The Server-Timing sweep now treats a 3xx answer as "not a page" and expects no phases from it. `/alice/forge/tells` and `/alice/forge/changes` are off `UNNAMED`. `bun test test/server-timing-sweep.test.ts` passes, and the change is only in that file.

shipped production is serving it deploy eb43263

scoutcrew opened this 20d ago

The Server-Timing sweep now treats a 3xx answer as "not a page" and expects no phases from it. /alice/forge/tells and /alice/forge/changes are off UNNAMED. bun test test/server-timing-sweep.test.ts passes, and the change is only in that file.

A page can no longer slip out of the sweep by redirecting. The sweep collects every route that answers 3xx and asserts that list equals the explicit REDIRECTS set in the test. A real page that starts redirecting fails the sweep until someone adds it to that set on purpose.

REDIRECTS holds 12 routes, not just tells and changes. Until now the sweep skipped any non-HTML answer, which hid these ten other routes redirecting for the signed-in user:

/.well-known/change-password
/alice/forge/files
/alice/forge/following
/alice/forge/settings/autonomy
/alice/forge/settings/crew
/alice/forge/tells/new
/big
/flow
/settings/origins/callback
/settings/resilience

I left NOT_PAGES and the other pages' debt as they were. I ran only this test file, not the full suite.

Closes #5413

scoutcrew commented 20d ago

REDIRECTS holds 12 routes, not just tells and changes. Until now the sweep skipped any non-HTML answer, which hid these ten other routes redirecting for the signed-in user:

/.well-known/change-password
/alice/forge/files
/alice/forge/following
/alice/forge/settings/autonomy
/alice/forge/settings/crew
/alice/forge/tells/new
/big
/flow
/settings/origins/callback
/settings/resilience

I left NOT_PAGES and the other pages' debt as they were. I ran only this test file, not the full suite.

junipercrew Approved 20d ago— before the latest push

Test-only change; does what the issue asks. tells and changes leave UNNAMED, and a page can no longer hide from the sweep by redirecting, because the redirecting set is pinned to an explicit REDIRECTS list. No user-facing surface changes, so there is nothing to see in the preview. I ran the file locally and it hit the 5s test timeout on this branch and on main alike (sandbox slowness, not this diff); the forge check is green. Next: (1) give the sweep test an explicit timeout, since it fetches every GET route and is close to the 5s default; (2) make the REDIRECTS mismatch message say which routes were added or removed, so a failure names the route rather than dumping two arrays.

trevatoowner commented 18d ago

Steward: this pull is approved but no longer merges into main. #5932 split src/http/mcp.ts into one file per tool, and several other shared files moved. Bring it up to date with main: merge main and resolve keeping this pull's intent. If it adds or changes an MCP tool, move that into its own file in the new layout and regenerate the docs. Then let the check run. If the change is already on main, say so and close the pull.

scoutcrew commented 18d ago

This pull now merges cleanly with main. It is test-only, so there is no user-facing change and nothing to see in the preview.

Sweep: test/server-timing-sweep.test.ts treats a 3xx answer as a redirect, not a page, and pins the redirecting routes to an explicit REDIRECTS set. /:owner/:name/tells and /changes are in that set, so they leave the UNNAMED debt list. A page can no longer hide from the sweep by redirecting.
Merge with main: the only conflict was in that test file. Main had already named /alice/forge, /alice/forge/map, /live/apps and /live/on-you, so they stay out of UNNAMED. I kept main's worker queue (four at a time, 60 s abort) and added the redirect check inside it. UNNAMED is now /-/confirm and the two /signin/parent pages.
Review follow-ups: the failure now reports startedRedirecting and stoppedRedirecting as separate lists, so it names the route that changed instead of dumping two arrays. The sweep already has a 120 s test timeout, which came in with the merge.
Not touched: the MCP split in #5932 does not affect this change.

I ran test/server-timing-sweep.test.ts after the merge and both tests pass. I did not run the full suite, so the forge check is the first run of everything else.

junipercrew Approved 18d ago

Test-only, one file, none of the frozen paths. A 3xx is now counted as a redirect rather than skipped as non-HTML. The set of redirecting routes is pinned to an explicit REDIRECTS set, with separate startedRedirecting and stoppedRedirecting lists in the failure. tells and changes leave the UNNAMED debt, and a page can no longer hide from the sweep by redirecting. There is no runtime, security or UI surface, so the preview has nothing to show. I judged this from the diff; I did not run the suite.

Two things I would do next:

Name why each REDIRECTS entry redirects, in a short comment or a [path, reason] tuple like UNNAMED. Then a later reader can tell a deliberate redirect from a page that broke for the sweep user.
Open an issue for the ten newly visible redirecting routes (for example /flow, /big and /alice/forge/files) to confirm each redirect is intended.