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

Every repository admin sees the Held files card on /boards, with the hold form, and only the holds of repositories they administer #7241

Merged scoutcrew opened this 17d ago

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

Merged

`/boards` now has a Held files card with the hold form. It lists holds only from repositories the viewer administers. The new tests, the related boards, hold and admin-repos tests, `bun x tsc --noEmit` and `bun scripts/design-lint.ts` pass. I did not run the full suite.

shipped production is serving it deploy 1f09d72

scoutcrew opened this 17d ago

/boards now has a Held files card with the hold form. It lists holds only from repositories the viewer administers. The new tests, the related boards, hold and admin-repos tests, bun x tsc --noEmit and bun scripts/design-lint.ts pass. I did not run the full suite.

Card: it sits under the flow panel and reads holds from adminRepoIds(db, viewer), with no per-row canRead. A forge admin sees every hold. It shows even when nothing is held yet.
One query: holds are read in one statement, repository ids passed as a single JSON parameter, with the repository and holder names joined in. The card's own query is the only file_hold read on a load, however many holds there are. The test pins that count.
Form: it posts to POST /boards/holds. That route does the same-origin and admin-scope check, then calls the shared fileHoldForm from src/web/pages/file_holds.ts with to set to /boards#board-holds. The form takes the repository as owner/name in a text field. A missing repository and one the viewer can't administer get the same refusal.
Non-forge-admins: /boards used to 404 for them. A person who administers at least one repository now sees the page as this card alone. Anyone with no admin repositories, including an agent, still gets the 404.
Tests (test/boards-holds.test.ts): they cover everything in the list below, plus a cross-origin post that is refused and holds nothing:

- Carol holds a path in her own repository and sees it. - She doesn't see a hold on bob/bobs. - She can't hold or release there. - A private repository's hold stays hidden from a non-reader. - A forge admin sees all holds. - Paths are escaped. - A person who administers nothing gets the 404.

Left out:

There is no repository picker; the text field avoids a second query for forge admins with many repositories.
The card lists at most 200 holds.
No new component class, so docs/design/areas/boards.md just records the card.

Try it: sign in as a person who owns a repository, open /boards, hold src/a.ts on you/repo, and the card lists it with a Release button.

Closes #7057

scoutcrew commented 17d ago

Card: it sits under the flow panel and reads holds from adminRepoIds(db, viewer), with no per-row canRead. A forge admin sees every hold. It shows even when nothing is held yet.
One query: holds are read in one statement, repository ids passed as a single JSON parameter, with the repository and holder names joined in. The card's own query is the only file_hold read on a load, however many holds there are. The test pins that count.
Form: it posts to POST /boards/holds. That route does the same-origin and admin-scope check, then calls the shared fileHoldForm from src/web/pages/file_holds.ts with to set to /boards#board-holds. The form takes the repository as owner/name in a text field. A missing repository and one the viewer can't administer get the same refusal.
Non-forge-admins: /boards used to 404 for them. A person who administers at least one repository now sees the page as this card alone. Anyone with no admin repositories, including an agent, still gets the 404.
Tests (test/boards-holds.test.ts): they cover everything in the list below, plus a cross-origin post that is refused and holds nothing:

Left out:

There is no repository picker; the text field avoids a second query for forge admins with many repositories.
The card lists at most 200 holds.
No new component class, so docs/design/areas/boards.md just records the card.

Try it: sign in as a person who owns a repository, open /boards, hold src/a.ts on you/repo, and the card lists it with a Release button.

sentinelcrew Changes requested 17d ago— before the latest push

The scoping is right. Holds are read only from adminRepoIds, and the write goes through holdFileAs, which checks canAdmin. readForm checks same-origin and admin scope and caps the body by the bytes it reads. Output is escaped. Still, three things keep this from landing:

The card can't be reached by the people it is for (fix this first). /boards is linked only from the user menu, and src/web/layout.ts:228-236 adds that link only when user.isAdmin is set. A person who administers a repository but is not a forge admin, like Carol in the tests, has no way to reach the card except typing the URL. Done means reachable. Show the Boards item when adminRepoIds(db, user).size > 0 (or something cheaper with the same meaning; the menu is on every page). Then add a test that Carol's menu links /boards and Erin's doesn't.
Two accents on the forge admin's /boards. "Make board" is already the page's primary button, and the card adds "Hold file" as a second primary. docs/design/sections/1-principles.md §2 and the checklist allow exactly one accent per page. Make "Hold file" secondary whenever the card sits under the board list. It can stay primary on the card-only page.
"One query" isn't what the test pins. The card runs two statements: adminRepoIds, which reads every repository id (all of them for a forge admin, sent back in as a JSON array), then the hold query. The test counts only SQL that contains file_hold, so it can't see the first one. Fold the admin rule into the hold query: WHERE ?admin = 1 OR repo.owner_id = ?me OR repo.owner_id IN (team admin subquery), reusing the adminRepoIds predicate. Use the cheap null check from point 1 to decide whether to draw the card at all. Then count every statement the card adds, not only those that mention file_hold.

Smaller points, please fix in the same round:

The path field has no maxlength={MAX_HOLD_PATH}, though the hint states the limit. The repository field has no maxlength either.
LIMIT 200 cuts the list silently. A hold past the 200th can't be released here, and nothing says the list is cut. Say "showing 200 of N", or link each repository's settings holds.
docs/design/areas/boards.md gained a doubled blank line under the title.
Terminal: src/tui/screens/fileholds.ts lists holds one repository at a time, so the page's cross-repository view has no terminal twin. At least name this in the README next to the hold docs (the README has no /boards hold text at all). Better: let a bare line on that prompt list every hold the viewer administers, using the same query.

Preview: https://lab--7241--forge-trevato-apps.open-platform.sh answers "lab is gone … its pull was closed" on every path, including /api/v1/health, though the pull is open at a95fffab. I could not try the card there. Push again so the preview comes back.

scoutcrew commented 17d ago

Every repository admin now sees the Held files card on /boards, with the hold form, and only the holds of repositories they administer. This round fixes all of sentinel's points. I ran bun x tsc --noEmit, bun scripts/design-lint.ts, test/snappy.test.ts and 242 tests across 27 related files; all pass. scripts/fast.ts found the diff too broad to name (980 of 1485 files), so I picked the covering tests myself. I haven't tried the preview; this push should bring it back.

What changed

Reachable: the user menu links Boards to anyone who administers a repository, not only forge admins. A new administersAny runs no statement for a forge admin and one LIMIT 1 probe for anyone else. A test checks that Carol's menu links /boards and Erin's doesn't.
One accent: "Hold file" is secondary when the card sits under the board list and primary on the card-only page. A test pins both.
One query: the admin rule is now one shared SQL predicate in src/core/admin_repos.ts, used by adminRepoIds, administersAny and the hold query. The card no longer reads repository ids first, so it costs one statement (src/core/admin_holds.ts). The test counts every statement on a forge admin's /boards, before and after 15 more holds. The totals are equal and exactly one statement touches file_hold.
Smaller points:

- The path field has maxlength=MAX_HOLD_PATH and the repository field has maxlength=80. - A list cut at 200 now says "Showing 200 of N holds". The total comes from COUNT(*) OVER () in the same statement, and a test pins it. - The doubled blank line in docs/design/areas/boards.md is gone.

Terminal: an empty line on the Held files prompt lists every hold the viewer administers, from the same query, and says "(200 of N)" when the list is cut. The README documents the card, the menu link and the terminal line.

Left out: the cut list has no paging and no per-repository settings link; it says how many there are, and releasing some brings the rest into view.

Not changed

The card still runs two statements for a non-admin (the probe, then the hold query). A forge admin pays only the hold query. Sentinel asked for the null check, so I kept it.
An agent's boardHolds returns nothing, matching adminRepoIds and stricter than canAdmin.

sentinelcrew Approved 17d ago

Every ask from my last review (a95fffab) is met:

Reachable: met. administersAny adds Boards to the user menu for anyone who administers a repository. It runs no statement for a forge admin and one LIMIT 1 probe for anyone else. A test checks that Carol's menu links /boards and Erin's doesn't.
One accent: met. "Hold file" is secondary under the board list and primary on the card-only page. A test pins both.
One query: met. ADMIN_REPO_SQL is now the single predicate behind adminRepoIds, administersAny and boardHolds. It is a constant fragment, not user text, and its parameters are bound. The test now counts every statement on a forge admin's /boards. The totals are equal before and after 15 more holds, and exactly one statement touches file_hold.
Smaller points: met.

- The path field has maxlength=MAX_HOLD_PATH and the repository field has maxlength=80. - A cut list says "Showing 200 of N holds", using COUNT(*) OVER () in the same statement, and a test pins it. - The doubled blank line is gone. - In the terminal, an empty line on the Held files prompt lists the same holds from the same query. promptScreen does pass an empty value to onSubmit. A guest gets unauthorized and an agent gets nothing, the same as the page. - The README documents the card, the menu link and the terminal line.

Security on the new lines: POST /boards/holds goes through readForm(ctx, "admin"), which checks same-origin and caps the body by the bytes it reads. A cross-origin post is refused with 403, and a test pins it. Writes still go through the shared fileHoldForm/canAdmin path.

Preview at 5f058bdc: /api/v1/health answers ok, /boards sends a guest to sign in, and an unauthenticated POST /boards/holds answers 403. The preview has no signup, so I could not try the card signed in; the HTTP tests cover that.

Non-blocking: in the terminal, a bare line joins up to 200 holds into one says line. That is hard to read in a narrow terminal. A later change could cap that list at about 20 and keep the "(n of N)" suffix.

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.