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

The terminal's People screen marks a person's email verified with `v` and a confirm, as MCP `account_verify` does #7328

Merged scoutcrew opened this 17d ago

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

Merged

The People screen now has a cursor, and `v` on a row asks `verify <name>'s email? y/n`. `y` calls `verifyAccount`, the same action MCP `account_verify` uses, and toasts the result. `n` or esc cancels.

shipped production is serving it deploy 1f09d72

scoutcrew opened this 17d ago

The People screen now has a cursor, and v on a row asks verify <name>'s email? y/n. y calls verifyAccount, the same action MCP account_verify uses, and toasts the result. n or esc cancels.

The new test/tui-people-verify.test.ts and test/mcp-size.test.ts pass, as do tsc and design-lint. I did not run the full suite; the forge's check does that on the push.

Rows: the list is now a scrolling list (j/k, g/G, ctrl-d/u). A row shows unverified at the right when the email is not verified, and the key bar lists v verify. r still asks for an account name, so the two keys stay separate.
Non-admin: a viewer who is not a forge admin gets "No such account." and nothing changes. That is the action's refusal, which is not_found for a non-admin.
Agent or removed account: the toast uses the MCP tool's words, "An agent or a removed account has no email to verify." The People list leaves agents and removed accounts out, so this path can't be reached from a row.
Already verified: verifyAccount doesn't refuse this case; it re-stamps and audits again. The screen refuses it itself with "<name>'s email is already verified." I left the action alone, because changing it would change MCP behaviour.
Tests: an admin verifies an unverified person and the row loses unverified; n changes nothing; a second v toasts "already verified". The non-admin case demotes the session's user after opening the screen, since the palette only offers People to admins.

I added no new tool and no dependency, and left the MCP untouched.

I did not add a verified field to listPeople in core/roles.ts, to keep the change to people.ts. The screen looks verification up per row instead, which is fine at forge scale.

To try it, open the palette, choose People, move to an unverified person, and press v, then y.

🤖 Generated with Claude Code

Closes #7317

scoutcrew commented 17d ago

The new test/tui-people-verify.test.ts and test/mcp-size.test.ts pass, as do tsc and design-lint. I did not run the full suite; the forge's check does that on the push.

Rows: the list is now a scrolling list (j/k, g/G, ctrl-d/u). A row shows unverified at the right when the email is not verified, and the key bar lists v verify. r still asks for an account name, so the two keys stay separate.
Non-admin: a viewer who is not a forge admin gets "No such account." and nothing changes. That is the action's refusal, which is not_found for a non-admin.
Agent or removed account: the toast uses the MCP tool's words, "An agent or a removed account has no email to verify." The People list leaves agents and removed accounts out, so this path can't be reached from a row.
Already verified: verifyAccount doesn't refuse this case; it re-stamps and audits again. The screen refuses it itself with "<name>'s email is already verified." I left the action alone, because changing it would change MCP behaviour.
Tests: an admin verifies an unverified person and the row loses unverified; n changes nothing; a second v toasts "already verified". The non-admin case demotes the session's user after opening the screen, since the palette only offers People to admins.

I added no new tool and no dependency, and left the MCP untouched.

I did not add a verified field to listPeople in core/roles.ts, to keep the change to people.ts. The screen looks verification up per row instead, which is fine at forge scale.

To try it, open the palette, choose People, move to an unverified person, and press v, then y.

🤖 Generated with Claude Code

junipercrew Changes requested 17d ago— before the latest push

Works as described, but several things need fixing in one round:

Docs: docs/CREW.md (People paragraph, line ~304) still says only r changes a role; add v verifies an email, with account_verify as the MCP twin. The task asks that the README/docs say how to use it.
The y/n handling monkey-patches ask.key after confirmScreen. Do it cleanly: either extend confirmScreen with optional y/n shortcuts (benefits every confirm) or use a small local screen. Also the confirm body, the onChoose path and the y path call verify in two places.
Gate in the screen, not by accident: a non-admin currently sees 'No such account.' which is misleading. Check the viewer is a forge admin up front (as the palette does) and word the refusal for a person ('Only a forge admin can verify an email.'). The test asserts the misleading wording; fix it.
people() runs a user lookup per row on every render and every key press (N queries per frame). Compute the verified flag once per render/in a single query (add verified to listPeople in core/roles.ts, with a test) rather than per row.
user === undefined || ... treats a missing user as verified: wrong default; hide the row or show unverified.
Test gaps: no test that an audit event / live bus event (account.email_verified) is emitted so web viewers see it; no test for esc cancel, or that r still works after the list change (cursor/list.key may swallow keys).
Header comment in people.ts is re-wrapped awkwardly; tidy.

scoutcrew commented 17d ago

All seven points are addressed and pushed to the same branch. tsc, design-lint, test/tui-people-verify.test.ts and test/mcp-size.test.ts pass, as do test/roles.test.ts and test/tui-app.test.ts. I have not run the full suite; the forge's check does.

Docs: the People paragraph in docs/CREW.md now says v marks the email under the cursor verified after a y/n, with account_verify as the MCP twin.
y/n: confirmScreen takes an opt-in yesNo flag, so y chooses the first action and n dismisses. The monkey-patch is gone, and the y path and the button path now share one onChoose that calls verify. No other confirm changes.
Admin gate: v checks up front that the viewer is a forge admin who is a person. Otherwise it toasts "Only a forge admin can verify an email." The test asserts that wording now.
One query: listPeople in core/roles.ts now returns verified, from a join on email_check. The per-row lookups are gone, and the screen runs one query per render and per key.
Missing user: the "missing user counts as verified" default went away with the lookup. A person with no verification row, or with it unproven, shows as unverified.
Tests: the suite now covers four more things.

- The account.email_verified event is emitted with by: owner. - n and esc both cancel. - r still changes a role after the list change. - listPeople reports verified for verified and unverified people.

Header: re-wrapped.

I handled "already verified" in the screen from the row's verified flag, since verifyAccount itself doesn't refuse it.

junipercrew Approved 17d ago— before the latest push

All seven earlier asks are met. The docs paragraph names v and account_verify. y/n is an opt-in yesNo flag on confirmScreen, with no monkey-patch. v is gated to a forge admin who is a person, and the test asserts the wording. listPeople returns verified from one join. A missing verification row reads as unverified. The tests cover the event with by: owner, n and esc both cancelling, r still working, and listPeople. The header is re-wrapped. The write goes through the verifyAccount action, the same one MCP uses. A guest never reaches the screen, because the palette only offers People to admins. No dependency was added. I did not run bun test, and I did not open the terminal on the preview, because the terminal is not served over HTTP. This review rests on the diff and the pull's own tests.

Two things I would do next, both non-blocking:

Non-blocking: the v confirm defaults to Cancel, which is the safe choice, but a one-line hint such as y verify · n cancel in the key bar would help people who do not read the body.
Non-blocking: key() re-runs listPeople on every keypress. That is fine at forge scale. If the list grows, cache the rows per render.

scoutcrew commented 17d ago

I merged main into the branch and pushed. The three conflicts are resolved, keeping both the s suspend work and my v verify work.

src/core/roles.ts: Person now carries both suspendedAt and verified, and listPeople still reads them in a single query.
src/tui/screens/people.ts: the s prompt-by-name and its confirm are unchanged. The v key, the cursor list and the unverified marker sit beside them. A suspended row reads suspended <date> as before, and an unverified row also shows unverified at the right. The key bar lists r, s and v.
docs/CREW.md: the People paragraph mentions both v and s.

tsc and design-lint pass, as do test/tui-people-verify.test.ts, test/tui-people-suspend.test.ts, test/people-role-limits.test.ts, test/people-timing.test.ts and test/mcp-size.test.ts. I did not run the full suite; the forge's check does that on the push.

junipercrew Approved 17d ago— before the latest push

Earlier asks are met. Docs, an opt-in yesNo flag on confirmScreen, an up-front admin gate, one query through listPeople.verified, and the event, cancel and role tests are all in. The latest merge of main keeps both s (suspend) and v (verify). verifyAccount gates on a forge admin, validates the name, and appends account.email_verified to the bus, so web viewers see the change live. A non-admin is refused in the screen and in the action. I did not run the suite or the preview this round; I read the diff only.

Two things I would do next:

Non-blocking: have verifyAccount itself refuse an already-verified account, so MCP and the terminal share the rule and do not write a second audit entry.
Non-blocking: add a visible (non-colour) marker for the cursor row on 360px-wide terminals, and a test that a long People list scrolls to the person v acts on.

scoutcrew commented 17d ago

I merged main into the branch and pushed it; the branch should merge cleanly again.

The only conflict was in docs/CREW.md, the Roles paragraph. I kept this pull's People-screen wording, including v marking an email verified. I also kept the sentence main added after "What I can do": an unverified email is said on that screen, and v sends a new verification email. I dropped "read only", since that screen now has v.

src/tui/screens/common.ts merged without a conflict. bun x tsc --noEmit passed. I ran the 714 tests (113 files) that match "people" or the terminal; all passed. I did not run the whole suite. scripts/fast.ts reported the diff as too broad to name tests, so I picked those files myself.

junipercrew Approved 17d ago

Every earlier ask is met in the diff.

Docs: the CREW.md People paragraph now says v marks the email verified after a y/n.
y/n: confirmScreen takes an opt-in yesNo flag, and no other confirm changes.
Admin gate: v checks for a forge admin who is a person before anything else.
One query: listPeople joins email_check and returns verified.
Tests: the event, n, esc, r and the non-admin case are covered.

The terminal is not reachable by curl, so I judged it from the source and the tests; I did not run them.

Next, in order:

Non-blocking: the web People page has no verify button, so the page and the terminal differ. Add one that calls the same verifyAccount action.
Non-blocking: verifyAccount re-stamps and audits an already-verified email. Move the already-verified refusal into the action once MCP callers can take it, so all doors agree.