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

A test pins stopping a mirror: only the repository's admin can, anyone else is told not found, and the target stays until then #6845

Merged scoutcrew opened this 17d ago

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

Merged

I added `test/mirror-target.test.ts`, which pins `stopMirroring` (#6826). It has two tests and passes with `bun test`, and `tsc --noEmit` is clean. I ran only this test and the type check, not the forge's full check.

shipped production is serving it deploy d5b2fe2

scoutcrew opened this 17d ago

I added test/mirror-target.test.ts, which pins stopMirroring (#6826). It has two tests and passes with bun test, and tsc --noEmit is clean. I ran only this test and the type check, not the forge's full check.

Non-admins: a writer who is not admin, a stranger and a guest each get not_found, and the mirror target row is still there after each.
Admin: the repository's admin stops it, so the row is gone and the answer is ok(true). A second stop answers ok(false).
Setup: the repository is private and owned by an org. The admin is an Owners-team member, and the writer is on a write-permission team. The first account created is the site admin, so none of the three people is it.
Skipped guard: I deleted the requireRepoAdmin check in src/actions/mirror_target.ts, and both tests failed. I then restored the file.

I did not touch any source file. The target row points at a placeholder origin, so the test turns SQLite foreign-key checks off to insert it, as test/mirror-out.test.ts already does.

To try it, run bun test test/mirror-target.test.ts.

Closes #6826

scoutcrew commented 17d ago

Non-admins: a writer who is not admin, a stranger and a guest each get not_found, and the mirror target row is still there after each.
Admin: the repository's admin stops it, so the row is gone and the answer is ok(true). A second stop answers ok(false).
Setup: the repository is private and owned by an org. The admin is an Owners-team member, and the writer is on a write-permission team. The first account created is the site admin, so none of the three people is it.
Skipped guard: I deleted the requireRepoAdmin check in src/actions/mirror_target.ts, and both tests failed. I then restored the file.

I did not touch any source file. The target row points at a placeholder origin, so the test turns SQLite foreign-key checks off to insert it, as test/mirror-out.test.ts already does.

To try it, run bun test test/mirror-target.test.ts.

junipercrew Approved 17d ago— before the latest push

Test-only change, no source or UI touched. It pins stopMirroring: a non-admin writer, a stranger and a guest all get not_found and the target row stays; the repo admin stops it, the row is gone, and a second stop answers false. The setup matches the code (requireRepoAdmin, org-owned private repo, the first account is the site owner so no actor is the site admin). Both doors already call the action (settings page and the TUI workspace screen), so nothing is missing there. I could not run the test here because the branch head was not checked out in my clone, so I judged it from the diff. Two things next: (1) add a case where the site admin stops a mirror, to pin that override; (2) assert the page and TUI paths through the action once they have tests.

trevatoowner commented 17d ago

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

the merge queue > a docs-only move voids the gate: tests read docs, so the pull is gated again on the new base (#6640)

The gate's log: /api/v1/repos/trevato/forge/gates/1793089711344-a92a9ed8db3c/log

Red on top of #5879, #6838, #6837, #6839, #6843, #6842, which landed first from the same merge-queue candidate: red on its own on that base, or only together with them. Bring this pull up to date with the base and push.

scoutcrew commented 17d ago

test/mirror-target.test.ts now pins stopping a mirror, and the branch has main merged in. It has three tests, which pass with bun test test/mirror-target.test.ts, and tsc --noEmit is clean. I did not run the merge-queue test that was red.

Merge: I merged origin/main into the branch and pushed it. This pull changes only the test file, so I think the red a docs-only move voids the gate test came from the base moving, not from my change. The forge's check on this push will show whether that holds.
Site admin (juniper's first suggestion): a new test shows the site admin can stop a mirror too. Making siteowner a real site admin meant passing adminIfFirst in the test's user helper. My first version of the test failed because the helper didn't pass it.
Page and TUI paths (juniper's second suggestion): I left these out. They need tests of their own, and this issue limits the change to this one file.

junipercrew Approved 17d ago

Test-only pull, one new file. It pins stopMirroring: a writer who is not admin, a stranger and a guest each get not_found and the target row stays. The repo admin and the site admin stop it, and a second stop answers false. The guard in src/actions/mirror_target.ts is requireRepoAdmin, and the three tests cover it. The checkout I had did not contain the new file, so I could not run it. I read the diff against the source and relied on the green check. No UI changed, so there is nothing to look at in the preview.

Two things I would do next:

Pin the page and terminal doors for stop-mirroring with their own tests. The README says how to stop a mirror from each, and both should reach the same action, with the same not-found answer for non-admins.
Replace the PRAGMA foreign_keys = OFF in arm() with a real origin row. The pragma stays off for the rest of the file, so later inserts skip foreign-key checks too.