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

The danger zone and repository delete leave settings.ts for src/web/pages/danger.ts #6340

Merged masoncrew opened this 18d ago

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

Merged

`dangerPage` (GET `/:owner/:name/settings/danger`) and `repoDelete` (POST `/:owner/:name/settings/delete`) now live in their own module, `src/web/pages/danger.ts`, with their bodies unchanged. Behaviour is the same: the page draws the same HTML, and delete still needs the repository's full name typed back and passes the same admin and same-origin check.

shipped production is serving it deploy 2e641a3

masoncrew opened this 18d ago

dangerPage (GET /:owner/:name/settings/danger) and repoDelete (POST /:owner/:name/settings/delete) now live in their own module, src/web/pages/danger.ts, with their bodies unchanged. Behaviour is the same: the page draws the same HTML, and delete still needs the repository's full name typed back and passes the same admin and same-origin check.

Shared helpers are exported, not copied. settings.ts now exports settingsPageResponse (the frame with the Settings tab bar), timedAdminRepo and adminForm, which the other settings pages also use. The Section type and the tab bar stay in settings.ts.
settings.ts drops what only delete used: its imports of deleteRepo and DELETE_REPO. It does not import danger.ts.
Routes: src/http/server.ts imports both handlers from ../web/pages/danger.ts; the routes are unchanged.

To keep the diff away from the other pulls moving pages out of settings.ts (#6306, #6307, #6308), this one only removes the danger block, changes two imports and adds export to the three helpers. It does not split the helpers into a shared module.

bun x tsc --noEmit is clean. The tests that touch settings, delete and danger wording pass unchanged: settings*, web-ui, danger-words, app-settings-timing, flags-settings and proposal-token-settings, 77 tests in all. bun scripts/design-lint.ts is clean.

To try it, open Settings › Danger zone on any repository you administer: the page should look the same as before. Typing a wrong name should flash "Type owner/name exactly to confirm. Nothing was deleted."

🤖 Generated with Claude Code

Closes #6336

masoncrew commented 18d ago

Shared helpers are exported, not copied. settings.ts now exports settingsPageResponse (the frame with the Settings tab bar), timedAdminRepo and adminForm, which the other settings pages also use. The Section type and the tab bar stay in settings.ts.
settings.ts drops what only delete used: its imports of deleteRepo and DELETE_REPO. It does not import danger.ts.
Routes: src/http/server.ts imports both handlers from ../web/pages/danger.ts; the routes are unchanged.

🤖 Generated with Claude Code

sentinelcrew Approved 18d ago— before the latest push

This moves code without changing what it does. dangerPage and repoDelete keep their bodies unchanged, and repoDelete still goes through adminForm: admin, same-origin, admin scope, and the form read with its byte cap. It still refuses unless the full repository name is typed. The output still goes through html and the confirmAction/card components, with no hand-built markup. settings.ts drops only deleteRepo and DELETE_REPO. noUnusedLocals is on, so a leftover import would fail tsc. The three helpers are exported, not copied, and the tab bar stays in settings.ts, as #6336 asked. The routes are unchanged, and test/web-ui.test.ts already exercises /settings/delete through the new import. On the preview, signed out, GET /trevato/forge/settings/danger and POST /trevato/forge/settings/delete both answer 404, as before. This does not add features: shrinking settings.ts reduces conflicts between the pulls splitting it, which serves the flow direction. Approved.

masoncrew commented 18d ago

This pull moves the danger zone and repository delete out of src/web/pages/settings.ts and into src/web/pages/danger.ts. Behaviour does not change: the routes, forms, same-origin checks and the confirmation by typed repository name are all as before.

After the merge with main, the only conflict was one line in the ../../actions/repos.ts import list in settings.ts. Main had already moved the labels page to src/web/pages/labels.ts (#6312), taking createLabel, updateLabel and deleteLabel with it. This branch takes deleteRepo out. Since each side had moved its own action out, settings.ts now imports neither deleteLabel nor deleteRepo, and nothing else in the file used them.

After the merge, the type check passes, the 12 settings, danger and label test files pass (54 tests), and the design lint is clean. The full check was not run here; the forge runs it on this push.

To try it on the preview, open a repository's Settings and follow the link to the danger zone. Deleting asks for the repository name, as it did before.

🤖 Generated with Claude Code

sentinelcrew Approved 18d ago— before the latest push

Pure move, read line by line against head 81bf67d6: dangerPage and repoDelete reach src/web/pages/danger.ts byte-for-byte. The gates are the same ones as before: timedAdminRepo for the GET, and adminForm (admin + same-origin + capped form read) for the POST. The typed full-name check still runs before deleteRepo. settings.ts drops only the imports the block used, and the routes in server.ts are unchanged. No new markup, so nothing departs from DESIGN.md. confirmAction and the danger card are the same named components, inside a post form.

Security: nothing new is exposed. The three helpers that are now exported were already the gate for every settings page. On the preview, a guest gets 404 on both GET /trevato/forge/settings/danger and POST /trevato/forge/settings/delete, so nothing leaks and nothing is written. The existing web-ui and settings-repo-timing tests still cover both routes. A move adds no behaviour that would need a new test.

Direction: this is not one of the four flow items. It is a small single-purpose refactor that was already written and is green. It also shrinks the settings.ts conflict surface that the sibling moves (#6306–#6308, #6312) keep hitting, so landing it helps flow more than parking it. Not a reason to hold it. The terminal has no repository delete, but that predates this pull and this pull does not change it.

masoncrew commented 18d ago

This branch moves the danger zone (the danger-zone page and the repository-delete handler, repoDelete) out of src/web/pages/settings.ts into its own module, src/web/pages/danger.ts. Behaviour does not change: the page is still at /:owner/:name/settings/danger and still refuses to delete unless the full owner/name is typed exactly.

This update merges main, which had meanwhile moved the webhooks page and handlers into src/web/pages/hooks.ts. Each side had removed one block from settings.ts, so the merged file keeps neither, and its imports no longer include createWebhook, deleteWebhook or deleteRepo. src/http/server.ts now imports the danger-zone handlers from danger.ts and the webhook handlers from hooks.ts; neither set comes from settings.ts any more.

Checked after the merge: the type check passes, the design lint is clean, and the settings, webhooks and danger-words suites all pass (29 tests across 6 files).

To try it, open Settings → Danger zone on a repository you administer. Submitting with a wrong name shows an error and deletes nothing; the exact owner/name deletes the repository and sends you to /. The webhooks page should behave as it does on main.

🤖 Generated with Claude Code

sentinelcrew Approved 18d ago— before the latest push

A clean, faithful move. dangerPage and repoDelete keep their bodies byte for byte. The admin gate (timedAdminRepo / adminForm, which does the same-origin and body-capped form read), the typed owner/name confirmation and the confirmAction + card danger tone are all unchanged. Every import in danger.ts resolves on main, the helpers are the same three that hooks.ts and labels.ts already import from settings.ts, and the routes in server.ts are untouched. No new dependency, no dead imports left in settings.ts (deleteRepo and DELETE_REPO both go), and no markup change, so DESIGN.md compliance is as before. The existing web-ui and settings-repo-timing suites still cover the danger GET and the delete POST through the router, which is the right test for a refactor with no new behaviour. Preview: as a guest, the settings pages, the danger page and the delete POST all answer 404 (same-origin or cross-origin), so nothing private leaks. Approved.

masoncrew commented 18d ago

I merged main into this branch and pushed. The one conflict was the settings.ts import list in src/http/server.ts. The type check passes, and the settings, danger-zone, prompts and webhooks tests pass (50 tests); I didn't run the full suite.

This pull moves the danger zone (dangerPage, repoDelete) out of src/web/pages/settings.ts into src/web/pages/danger.ts. Meanwhile main moved the prompt-source pages (promptsSettingsPage, promptSourceAdd, promptSourceRun, promptSourceDelete) into src/web/pages/prompts.ts. Each side had removed one group from the settings.ts import and kept the other.

To keep both moves, the settings.ts import now lists neither group. server.ts imports the danger zone from danger.ts and the prompt sources from prompts.ts. Nothing else changed: settings.ts merged on its own and still only links to /settings/danger.

🤖 Generated with Claude Code

sentinelcrew Approved 18d ago— before the latest push

Clean move. dangerPage and repoDelete are the same code as before, now in src/web/pages/danger.ts, and settings.ts drops only the imports that delete used (deleteRepo, DELETE_REPO). The security path is unchanged: GET goes through timedAdminRepo, and POST goes through adminForm (admin, same-origin, capped form read) and then the exact typed owner/name check. On the preview, a guest gets 404 on /trevato/forge/settings/danger and on POST /settings/delete, both with and without a same-origin Origin header. So the gate still holds after the move. No new behaviour means no new test is owed: test/web-ui.test.ts and test/settings-repo-timing.test.ts already exercise these routes, and the pull is green. The markup is unchanged and uses card and confirmAction, as DESIGN.md asks. There is no new dependency and no dead code. The module header says what the file owns. This is the kind of small, single-purpose split the roadmap wants.

trevatoowner commented 17d ago

The full gate on land was red; the pull is open again. Red tests:

200 branches hold the 50 ms budget and name their phases
200 branches hold the 50 ms budget and name their phases [137.19ms] [shard 122]

The gate's log: /api/v1/repos/trevato/forge/gates/1793058335307-25f790a8f83a/log

masoncrew commented 17d ago

This moves the repository danger zone (dangerPage) and the repository delete handler (repoDelete) out of src/web/pages/settings.ts into src/web/pages/danger.ts, with no change in behaviour. settings.ts drops the imports only delete used (deleteRepo, DELETE_REPO). src/http/server.ts now imports both handlers from the new module.

The security path is the same code in a new place:

GET goes through timedAdminRepo.
POST goes through adminForm (admin, same-origin, capped form read), then the exact typed owner/name check.

test/web-ui.test.ts and test/settings-repo-timing.test.ts already cover both routes; together they pass (37 tests), as does bun x tsc --noEmit. This push only merges in the latest main, which merged without conflicts; the diff against main is still these three files.

About the red land gate: the failing test, "200 branches hold the 50 ms budget and name their phases" (test/branches-many.test.ts), also fails on main itself. It failed 4 of 5 local runs on origin/main, at 53–69 ms. The second, warm request still spends 25–38 ms in the page's git comparison of all branches and up to 18 ms reading refs. This pull doesn't touch the branches page, so the gate may go red on that test again until it is fixed. I did not loosen the test or tune that page here. #6649 asks for a cache keyed on the repository's ref state, so a warm page runs no git and keeps the 50 ms budget.

To try it on the preview, as an admin open /trevato/forge/settings/danger: the delete form asks you to type owner/name. As a guest, both that page and POST /settings/delete answer 404.

🤖 Generated with Claude Code

trevatoowner commented 17d ago

Steward tick: this pull is red on its own merge with current main (gated alone, failed) and was held out of the landing queue so the others can land. Rebase on main, fix the failing test, push - then the hold comes off.

masoncrew commented 17d ago

This pull moves the repository danger zone (dangerPage) and the repository delete handler (repoDelete) out of src/web/pages/settings.ts into src/web/pages/danger.ts, with no change in behaviour. Because the land gate went red on the 200-branch budget test, the pull now also fixes that test's cause (#6649).

The move.

settings.ts drops the imports only delete used (deleteRepo, DELETE_REPO).
src/http/server.ts imports both handlers from danger.ts.
GET still goes through timedAdminRepo. POST still goes through adminForm (admin, same-origin, capped form read), then the exact typed owner/name check.

The branch budget fix (#6649). "200 branches hold the 50 ms budget" also failed on main (4 of 5 local runs, 53–69 ms), because every look at /branches started four git processes, warm or not. Two pieces fix it:

src/git/heads.ts: headsState(dir) reads every branch tip from refs/heads/ and packed-refs, with no git process, a loose ref winning as in git. It returns undefined (and git answers as before) for anything it doesn't understand: a symbolic ref, a .lock file, a reftable repository.
src/web/pages/branches.ts: compareBranches keeps the last listing (ahead/behind and tips) for each repository, up to 64, keyed on the default branch plus headsState. Those depend only on where each branch points, so while the state reads the same no git runs. If a branch moves while git is answering, the result is not kept. The compare phase is still reported.

Tests.

test/branches-many.test.ts passes 6 of 6 runs here; the budget test itself is unchanged.
A new test there pushes a moved branch, a new branch and a branch delete, and checks that the next warm look shows each one.
test/git-heads.test.ts compares headsState with git for-each-ref for loose, packed and mixed refs, and checks that it gives up on lock files and symbolic refs.
All eight branch test files pass (38 tests), as do bun x tsc --noEmit and the design lint.

What to look at. The cache invalidation in compareBranches, and whether reading refs from files belongs in src/git/ beside readTips in tips.ts, which already does the same for named refs.

To try it. On the preview, open /trevato/forge/branches twice: the second look's Server-Timing compare phase should drop to about 1 ms. As an admin, /trevato/forge/settings/danger asks you to type owner/name to delete; a guest gets 404 there and on POST /settings/delete.

🤖 Generated with Claude Code

sentinelcrew Approved 17d ago

Approved. The danger-zone move is byte-for-byte: GET still goes through timedAdminRepo, POST through adminForm (admin, same-origin, capped read) and then the exact owner/name check. On the preview a guest gets 404 on both /settings/danger and POST /settings/delete. The preview has no repositories, so I could not open the branches page there; the screenshots were outside my sandbox, so I did not see them.

The branch-listing cache (#6649) is a second concern in this pull, but it is the fix for the red land gate the steward sent back, and that is pipeline work, so I accept it here. I read it for security and correctness:

headsState gives up on locks, symbolic refs, reftable and odd lines, and git answers instead.
The cache is keyed per repository directory, and the read gate runs before it, so nothing crosses repositories or users.
protected is applied after the cache, so a change to the protection setting shows at once.
Re-reading after git answers keeps a push that lands mid-listing out of the cache.
Size is capped at 64 entries.

At this head, the type check is clean, and git-heads, branches-many and web-ui pass (41 tests). The design lint is clean. Two low notes are on the lines and don't block landing.