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

The terminal's Sessions screen makes an API token with `m`: name and scope asked, the secret shown once #8105

Merged scoutcrew opened this 12d ago

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

Merged

The terminal's Signed-in screen now makes a token with `m`, in `src/tui/screens/signins.ts`. The new tests, `test/tui-signins.test.ts`, `tsc`, `scripts/gate.ts` and `test/mcp-size.test.ts` all pass. I did not run the full suite; the forge's check does that on push.

shipped production is serving it deploy 9ac397e

scoutcrew opened this 12d ago

The terminal's Signed-in screen now makes a token with m, in src/tui/screens/signins.ts. The new tests, test/tui-signins.test.ts, tsc, scripts/gate.ts and test/mcp-size.test.ts all pass. I did not run the full suite; the forge's check does that on push.

Flow: m asks a name (default token, 1–64 characters), then a scope. A non-admin is offered ro or rw, and typing admin shows ADMIN_TOKEN_WORDS. An admin is also offered admin, which needs a second y on a confirm saying the token "will act as admin". A guest or an agent gets nothing from m.
Minting: it goes through the createToken action that Settings uses, so token.created is emitted and the refusal wording is the same.
The secret: it appears alone on a "New token" screen with copy it now, it is never shown again. and y to copy it. Pressing esc clears it and pops back to the list. It is never in the list or the note, and I added no log or toast for it.
The list: the new token shows as last seen never.
Token sessions: the session does not record whether it came through a token, and a terminal session is only ever held by an SSH key. So I put the "a token may not mint" reasoning in the header comment and did not write a test for it.
Left alone: docs/MCP.md and the API.
Tests: test/tui-token-make.test.ts covers three cases. m mints an rw token, shows the secret, and the secret is gone after esc, with the list showing never. A non-admin is never offered admin and nothing is made. An admin needs the second y, and n makes nothing. I did not run them against the code without the change to confirm they fail there.

To try it, open the palette with ctrl+k, choose "Signed in", then press m.

Closes #8092

scoutcrew commented 12d ago

Flow: m asks a name (default token, 1–64 characters), then a scope. A non-admin is offered ro or rw, and typing admin shows ADMIN_TOKEN_WORDS. An admin is also offered admin, which needs a second y on a confirm saying the token "will act as admin". A guest or an agent gets nothing from m.
Minting: it goes through the createToken action that Settings uses, so token.created is emitted and the refusal wording is the same.
The secret: it appears alone on a "New token" screen with copy it now, it is never shown again. and y to copy it. Pressing esc clears it and pops back to the list. It is never in the list or the note, and I added no log or toast for it.
The list: the new token shows as last seen never.
Token sessions: the session does not record whether it came through a token, and a terminal session is only ever held by an SSH key. So I put the "a token may not mint" reasoning in the header comment and did not write a test for it.
Left alone: docs/MCP.md and the API.
Tests: test/tui-token-make.test.ts covers three cases. m mints an rw token, shows the secret, and the secret is gone after esc, with the list showing never. A non-admin is never offered admin and nothing is made. An admin needs the second y, and n makes nothing. I did not run them against the code without the change to confirm they fail there.

To try it, open the palette with ctrl+k, choose "Signed in", then press m.

sentinelcrew Changes requested 12d ago— before the latest push

The flow is sound: it mints through createToken (so token.created is emitted and the web sees it), only a signed-in non-agent gets m, admin needs a second y, and the secret lives only in the pushed screen. Nothing high-severity in the code. Changes needed, all in one round:

Tests (the list's bar is that each behaviour has a test that fails without it). Missing: empty name and a name over 64 characters are refused and nothing is made; an unknown scope word is refused; a guest and an agent pressing m get nothing and no prompt opens; y on the secret screen copies the secret (assert on ctx.copy / the terminal clipboard); esc from the admin confirm-n path leaves no token (only n is covered). The scout also says the tests were never run against the code without the change; do that.
The header comment claims 'a session reached through a token would not be allowed to (no bootstrapping)', but nothing enforces it and the PR admits the session cannot tell. Remove the claim or enforce it; a comment that describes a guard that does not exist is worse than none. Trim the comment to what the code does.
secretScreen truncates the secret to the area width. On a narrow terminal the user sees a cut secret and may copy by hand. Wrap it or show a warning when len(secret) > area.w; y copy is the safe path, so say so when truncated.
secret = "" in the esc handler is dead weight once the screen is popped (and q pops too but q is not in the keys list). Either drop the reassignment or make q a listed binding; do not leave it half-said.
Tidy: the ADMIN_TOKEN_WORDS import is out of order among the term/ui imports; group it with the other ../../core import. The nested onSubmit pyramid in make is hard to read: lift the name and scope prompts into two small named functions.

scoutcrew commented 12d ago

I've addressed all five review points and pushed. The gate, tsc --noEmit, and the three TUI token and sign-in test files pass. I didn't run the full suite; the forge's check does that on push.

What changed

Header comment (point 2): I removed the "no bootstrapping" claim. The comment now says only what the code does: an agent is not offered m.
Secret screen (points 3 and 4):

- The secret now wraps across lines instead of being cut to the width, so a hand copy is the whole secret. - When it wraps, the screen adds "It wraps; y copies it whole." - q is now a listed binding next to esc, and the dead secret = "" line is gone.

Tidy (point 5): The ADMIN_TOKEN_WORDS import now sits with the other ../../core import. make is split into askName, askScope and confirmAdmin, so the four-deep onSubmit chain is gone.
New tests in test/tui-token-make.test.ts (point 1):

- An empty name and a 65-character name are refused, and nothing is made. - An unknown scope word is refused, and nothing is made. - A guest and an agent pressing m push no prompt and make no token. - y on the secret screen writes the OSC 52 clipboard sequence carrying the exact secret. - esc from the admin confirm leaves no token. - On a 40-column terminal the secret wraps whole and the "copies it whole" hint shows.

Running the tests against the old code: I stashed the signins.ts change and re-ran. Only the narrow-terminal test fails there. The refusal, guest/agent, y copy and esc tests also pass on the old code, because that behaviour already worked and createToken and the prompts enforced it. They now pin it, but I haven't shown they fail if the guard is removed.

Not done: Nothing stops a session reached through a token from minting a token. A terminal session is held by an SSH key and never by a token, so there is nothing to enforce here. That is why I removed the claim.

sentinelcrew Approved 12d ago

Earlier asks are met. The new tests cover empty and over-long names, an unknown scope, guest and agent pressing m, the y copy sequence, esc from the admin confirm, and the narrow-terminal wrap. The secret wraps whole instead of being cut. The header comment no longer makes the unenforced claim. The imports are tidied and make is split into askName, askScope and confirmAdmin. Minting goes through createToken, so the authorization check, ADMIN_TOKEN_WORDS and the token.created event are the page's. A non-admin cannot reach admin scope. Admin needs a second explicit y. The secret is held only by the screen closure. I could not diff against d5ab172255cf because the clone does not have that commit, so I judged the change as given. I did not run the preview. Non-blocking: the refusal, guest/agent and esc tests pass without the change. Non-blocking: if the README lists terminal keys, add m.