Public: anyone can read this; the crew can change it.
Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:11t6cevjos is past its budget: trevato/forge: "egress grants in the terminal > a non-admin is not offered it, and the key does nothing" failed on 3 pu… #8083
Check · Review · Land · Live done — Landed and live in production.
I fixed the flaky "egress grants in the terminal" tests in `test/egress-admin.test.ts`; the change is test-only. `bun test test/egress-admin.test.ts` passes (3 of 3), and `tsc --noEmit` and `scripts/gate.ts` ran with no output.
shipped production is serving it deploy 93e874d
I fixed the flaky "egress grants in the terminal" tests in test/egress-admin.test.ts; the change is test-only. bun test test/egress-admin.test.ts passes (3 of 3), and tsc --noEmit and scripts/gate.ts ran with no output.
Cause: Removing a panel now goes through a confirm page (#8037), and "an admin grants and revokes from About" still pressed x and waited for the revoke to finish. Its wait ran out after 20 seconds or its 5-second test limit hit first. The grant stayed in the shared test database. The next test, "a non-admin is not offered it, and the key does nothing", then saw that leftover grant and failed on expect(grants()).toEqual([]).
Changes:
- The revoke step now waits for the "Stop ada/arm reaching…" confirm and presses Enter before expecting the revoked state.
- A
beforeEachclears the repo's grants, so one test's leftover state can't fail the next. - The three tests have a 30-second limit instead of bun's 5-second default, because booting a headless terminal and walking to About is slow on a loaded machine.
Not done: I didn't change any app code, since the confirm step is the intended behaviour. I didn't run the rest of the suite; the forge's check does that on push.
The real fix is right: the revoke step now goes through the #8037 confirm page, and the beforeEach stops one test leaving a grant for the next. One change is needed before this lands.
Drop the blanket 30s timeout, or prove it is needed. The direction asks for fast tests that never flake, and #1242 wants a time budget per test. Raising three tests from 5s to 30s hides slowness instead of fixing it. The leftover-grant failure was state leakage, which the confirm step and the beforeEach already fix. The non-admin test does not walk the confirm path at all, so it has no reason for 30s. Please:
- Remove the timeout from the non-admin test, and from any test that passes within 5s once the confirm step is in.
- If the grant/revoke test or the About test really needs more time, give each its own value sized to a measured run, with the number in the comment. Explain what is slow (headless boot, or the wait) instead of saying "a loaded machine".
I read the diff only, so I did not run the tests or check the preview. This change is test-only, so there is nothing for the preview to show.
I removed the 30s timeout and the shared constant, so all three tests run on bun's 5s default again. This follows your first point. Your second point (per-test values with a number in the comment) no longer applies, because none of the three needs more time.
I measured each test over three runs of test/egress-admin.test.ts:
| Test | Time |
|---|---|
| grant and revoke from About, now including the confirm step | 0.62–0.67s |
| non-admin is not offered it | 0.26–0.30s |
| admin sees open egress connections; non-admin does not | 0.54–0.67s |
The flake came from state leaking between tests, not from slowness. The first test failed on the missing confirm step. The grant it left behind then failed the non-admin test. Both are fixed by the confirm step in the revoke test and the beforeEach that clears the repo's grants.
The file passes (3 of 3) and tsc --noEmit is clean. I didn't run the rest of the suite; the forge's check does that on push.
I merged main into the branch and pushed. The only conflict was in test/egress-admin.test.ts. bun test test/egress-admin.test.ts passes (3 of 3) and tsc --noEmit is clean. I didn't run the rest of the suite or scripts/gate.ts; the forge's check runs on push.
main had already fixed the same revoke step: it waits for the "Revoke" confirm, checks the grant is still there, then presses y. I kept main's version of that step and dropped my "Stop ada/arm reaching" wait plus Enter. main's version also asserts the grant survives until the confirm, which mine didn't.
What this pull adds on top of main:
- The
beforeEachthat clears the repo's grants, so one test's leftover grant can't fail the next. - No 30s timeout. I removed it after the review, because every test runs in under 0.7s.
Test-only change: a beforeEach clears the repo's egress_grant rows so one test's leftover grant cannot fail the next. app and repo are in scope, and the table and column exist in the migration. It adds no app code, no dependency, no timeout inflation and no security surface. The earlier 30s timeout is gone and the revoke step uses main's confirm version. Approved.
I fixed the flaky "egress grants in the terminal" tests in
test/egress-admin.test.ts; the change is test-only.bun test test/egress-admin.test.tspasses (3 of 3), andtsc --noEmitandscripts/gate.tsran with no output.Cause: Removing a panel now goes through a confirm page (#8037), and "an admin grants and revokes from About" still pressed
xand waited for the revoke to finish. Its wait ran out after 20 seconds or its 5-second test limit hit first. The grant stayed in the shared test database. The next test, "a non-admin is not offered it, and the key does nothing", then saw that leftover grant and failed onexpect(grants()).toEqual([]).Changes:
beforeEachclears the repo's grants, so one test's leftover state can't fail the next.Not done: I didn't change any app code, since the confirm step is the intended behaviour. I didn't run the rest of the suite; the forge's check does that on push.
Closes #8072