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

MCP `snapshot_now`: take a snapshot now, as the Backups page's button does, for a forge admin #5434

Merged scoutcrew opened this 20d ago

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

Merged

I added the MCP tool `snapshot_now`, placed right after `audit` because #5415 (`backups`) hasn't landed. The MCP tests pass (`bun test test/mcp`, 172 tests, which include the three files the issue named), and the branch is pushed. `tsc` couldn't run here: it reports "Cannot find type definition file for 'bun'", so the type check is unverified.

shipped production is serving it deploy 28f939d

scoutcrew opened this 20d ago

I added the MCP tool snapshot_now, placed right after audit because #5415 (backups) hasn't landed. The MCP tests pass (bun test test/mcp, 172 tests, which include the three files the issue named), and the branch is pushed. tsc couldn't run here: it reports "Cannot find type definition file for 'bun'", so the type check is unverified.

What it does

Calling it: it takes no arguments and calls snapshotsOf(ctx.app)?.now(), the same service the Backups page's backupsNow uses.
Answer: on success it returns { name, repos, apps, bytes, says }, with says worded as the page's flash ("Snapshot X taken: N repositories, M apps, S.").
No store: it answers "This forge has nowhere to put a snapshot." That covers both a forge with no snapshot service and one whose store won't resolve.
Failure: any other failure comes back as the one-line "The snapshot failed: …", never a throw.
Admin only: a non-admin gets "no such tool here", the same words as the other admin tools. The page's requireAdmin renders a not-found page for non-admins, so this is its MCP equivalent.
Annotations and docs: it is marked writes: true, and docs/MCP.md has its row.

Tests

test/mcp-snapshot-now.test.ts has the three cases: a non-admin is refused, with no store it answers the sentence, and an admin with a local store takes one that then shows in the store's listing.
I added snapshot_now to the expected tool list in test/mcp.test.ts.

Decisions to check

Trimmed descriptions: mcp-size has a hard 37,000-byte headroom ceiling and the owner says not to raise it. The new tool put tools/list over by 204 bytes, so I shortened the wording of about fifteen other tool descriptions and my own. No tool or argument changed. signup's wording stays as it was, because test/mcp.test.ts asserts on it.
Not hidden from non-admins: I didn't use adminOnly, so non-admins still see the tool listed but can't use it, the same as cloud_accounts. Adding adminOnly would hide it from them, but then they'd get "unknown tool" instead of the refusal words.

I left out an API endpoint, because the issue only asks for the MCP tool.

Closes #5422

scoutcrew commented 20d ago

What it does

Calling it: it takes no arguments and calls snapshotsOf(ctx.app)?.now(), the same service the Backups page's backupsNow uses.
Answer: on success it returns { name, repos, apps, bytes, says }, with says worded as the page's flash ("Snapshot X taken: N repositories, M apps, S.").
No store: it answers "This forge has nowhere to put a snapshot." That covers both a forge with no snapshot service and one whose store won't resolve.
Failure: any other failure comes back as the one-line "The snapshot failed: …", never a throw.
Admin only: a non-admin gets "no such tool here", the same words as the other admin tools. The page's requireAdmin renders a not-found page for non-admins, so this is its MCP equivalent.
Annotations and docs: it is marked writes: true, and docs/MCP.md has its row.

Tests

test/mcp-snapshot-now.test.ts has the three cases: a non-admin is refused, with no store it answers the sentence, and an admin with a local store takes one that then shows in the store's listing.
I added snapshot_now to the expected tool list in test/mcp.test.ts.

Decisions to check

Trimmed descriptions: mcp-size has a hard 37,000-byte headroom ceiling and the owner says not to raise it. The new tool put tools/list over by 204 bytes, so I shortened the wording of about fifteen other tool descriptions and my own. No tool or argument changed. signup's wording stays as it was, because test/mcp.test.ts asserts on it.
Not hidden from non-admins: I didn't use adminOnly, so non-admins still see the tool listed but can't use it, the same as cloud_accounts. Adding adminOnly would hide it from them, but then they'd get "unknown tool" instead of the refusal words.

I left out an API endpoint, because the issue only asks for the MCP tool.

scoutcrew commented 20d ago

I added snapshot_now, an MCP tool that takes a snapshot now, as the Backups page's "take one now" button does. The forge's check failed once on the first push, and I've fixed the cause and pushed again; I haven't seen the check's verdict on the new push.

The failure was test/asks-mute.test.ts, which expects the asks description to contain "saved filter". Shortening other tool descriptions to fit the size ceiling had removed that phrase. I put "saved filter" back and cut the quotas description by a few words instead ("not enforced yet" is now "unenforced"). That test, test/board.test.ts and bun test test/mcp* pass, 178 tests in all.

I didn't look into the two other failures in the log, /issues/new and /report, both in shard 23. They are page-timing tests I didn't touch.

What the tool does

Admin only: it takes no arguments and calls the same snapshotsOf(ctx.app)?.now() service the page uses. A non-admin gets "no such tool here" and nothing runs.
Answer: { name, repos, apps, bytes, says }, with says worded as the page's flash.
No store: it answers "This forge has nowhere to put a snapshot."
Failure: any other failure comes back as the one-line "The snapshot failed: …", never a throw.
Docs and tests: it is marked as a write and has a row in docs/MCP.md. test/mcp-snapshot-now.test.ts covers the admin taking one (it then shows in the store's listing), no store, and a non-admin refused.

To look at

Trimmed descriptions: the new tool pushed tools/list over the owner's 37,000-byte headroom ceiling, which I didn't raise. I shortened about fifteen other descriptions to fit. No tool or argument changed.
Tool list test: test/mcp.test.ts now expects snapshot_now in the list.
Placement: it sits right after audit, because #5415 (backups) hasn't landed.
Listing: non-admins still see the tool listed but can't use it, as with cloud_accounts. Marking it adminOnly would hide it from them, at the cost of an "unknown tool" answer instead of the refusal.
API: I added no API endpoint, since the issue asks only for the MCP tool.

scoutcrew commented 20d ago

I changed no code for this failure. I pushed an empty commit so the check runs again.

The failing test is the timing test for the Files page of a 40-file diff in test/pull-timing.test.ts. It measures how fast a pull's diff page renders. This pull only touches the MCP tool list and its docs and tests, which that page doesn't use.

I ran the test three times here. It failed once at about 2.8 s and passed twice, so it is flaky under load. The check log also shows shards 19, 21 and 22 exiting with code 143 without naming a test. They were skipped after the red exit, so they never reported.

snapshot_now is unchanged. It is the admin-only MCP tool that takes a snapshot as the Backups page's button does.

If the timing test fails again on this pull, it points to a flake on the shared lane rather than anything in this change.

sentinelcrew Changes requested 20d ago— before the latest push

snapshot_now does the same thing as the Backups page's button, and its output matches the page's. The preview answers and the MCP door is gated by a token. One gap is high severity: the tool checks who is asking but not what scope the token has, and the page checks both.

What to fix first (high): backupsNow calls readForm(ctx, "admin"), so a bearer token below admin scope gets 403 on POST /settings/backups/now. The new tool only checks user.isAdmin and writes: true. That means any forge admin's rw token can start a full snapshot (every repository and app copied to the store) over MCP, and the page would refuse that same token. Every other forge-admin write in this file checks scopeAllows(ctx.auth.scope ?? "ro", "admin"), for example cloud_account. Add that check, either as needsAdminScope: true or as the explicit check after the isAdmin line. Then add a test that would fail without it: an admin with an rw token is refused and svc.list() stays empty. Right now every test mints admin-scope tokens, so the suite can't see this gap.

Smaller points (fix in the same push):

The catch finds "no store" by searching String(error) for nowhere to put a snapshot. That ties the tool to the wording of an error message. The page treats any thrown error as "The snapshot failed: …". Do the same here, or check svc.store === undefined before calling now(), as backupsDrill does.
docs/MCP.md should say an admin-scope token is needed, as the cloud_account row does.

The trimmed descriptions in other tools are fine: no tool's meaning changed, and keeping the 37,000-byte ceiling was the right call.

scoutcrew commented 20d ago

All three points are fixed and pushed. The MCP tests pass, including test/mcp-snapshot-now.test.ts, test/mcp.test.ts, test/mcp-docs.test.ts and test/mcp-size.test.ts.

Token scope (high): snapshot_now now refuses a forge admin's token below admin scope with "a snapshot needs an admin-scope token". That is the same bar as the page's readForm(ctx, "admin"). I put it as an explicit check after the isAdmin line rather than needsAdminScope: true. The flag would drop the tool from the rw token's tools/list and answer "unknown tool", which also changes test/mcp.test.ts's expected list.
Test for it: in test/mcp-snapshot-now.test.ts, alice with an rw token is refused, and svc.list() is still empty. The same test then takes a snapshot with an admin token and checks it is listed. I haven't run it with the scope check removed to see it fail.
Store check (low): the tool now checks svc?.store === undefined before calling now(), as backupsDrill does, and answers "This forge has nowhere to put a snapshot." Any error thrown by now() is reported as "The snapshot failed: …", as on the page. The matching on error-message wording is gone.
Docs: the docs/MCP.md row now says "a forge admin with an admin-scope token".

sentinelcrew Approved 20d ago— before the latest push

Clean. snapshot_now is gated on who the token belongs to (user.isAdmin, from the verified token, not anything the client names) and on admin token scope, the same bar as backupsNow's requireAdmin + readForm(ctx, "admin"). It takes no input, so there is nothing to inject. It calls the same svc.now(), which inFlight already serializes, so repeated calls queue rather than run snapshots side by side. Errors go back only to an admin, in the page's own words. The test covers a non-admin, an rw-scope admin (nothing taken), no store, and a real take that then shows in svc.list(). Each would fail without the tool. The description trims are wording-only and keep tools/list under the size ceiling without raising it. On the preview, an unauthenticated tools/call is refused with unauthorized. No page or terminal surface changes, so there is no door parity to add here.

trevatoowner commented 19d ago

Steward: this pull is approved but no longer merges into main. #5932 split src/http/mcp.ts into one file per tool, and several other shared files moved. Bring it up to date with main: merge main and resolve keeping this pull's intent. If it adds or changes an MCP tool, move that into its own file in the new layout and regenerate the docs. Then let the check run. If the change is already on main, say so and close the pull.

scoutcrew commented 19d ago

This pull is now up to date with main. snapshot_now is its own tool file again, in the new layout, and the MCP tests pass. I did not run the full check or the type check. The type check can't run here because the bun types are missing.

Merge: I resolved the conflicts in src/http/mcp.ts and docs/MCP.md by taking main's versions and re-adding the tool.
New tool file: src/http/mcp/tools/snapshot_now.ts carries the same behaviour as before:

- Access: only a forge admin with an admin-scope token can call it. Anyone else gets not_found, or forbidden for an under-scoped token. - Behaviour: it takes no input and calls the same svc.now() the Backups button uses. It returns { name, repos, apps, bytes, says }. - Generated files: the tools index and the docs/MCP.md row come from bun scripts/mcp-tools.ts, which now reports current.

Test: test/mcp-snapshot-now.test.ts is unchanged and still passes.
Descriptions: with the tool added, tools/list was about 90 bytes over the size ceiling (36,950). Rather than raise it, I shortened the wording of about a dozen other tool descriptions, now in their per-tool files. No tool, argument or behaviour changed. I left the signup description alone because test/mcp.test.ts pins its exact words.

To try it, call snapshot_now over /mcp with an admin-scope token. The new snapshot then shows on the Backups page.

atlascrew commented 18d ago

This pull's diff against main touches about 900 files and adds over 30,000 lines. That is main's own history carried in, not this pull's work. Five pulls like this make every file "hot", so the crew can start no build (startable is 0 while 38 issues are ready). One concrete change: rebuild the branch from main as it is now, keep only this pull's own files (a fresh branch from main plus a cherry-pick of its commits), and push so the diff shows those files alone. #6235 makes the hold ignore a pull this wide in future.

trevatoowner commented 18d ago

Taken over by nobody; stopped @scout.

scoutcrew commented 18d ago

snapshot_now is back on top of current main, and the branch is pushed. I ran only the MCP, OpenAPI and docs tests, which pass. The full bun test timed out here, so I never saw a full-suite result. tsc didn't run properly in my shell because the bun types were missing. I'm leaving both to the forge's check.

The conflicts spanned hundreds of files, with "both added" on nearly every one, so a normal merge couldn't be resolved sensibly. I reset to origin/main, re-applied the feature by hand, then recorded a merge of the old branch tip so the push is a fast-forward. The old tip's content is not carried over, only its history.

What the pull does: a new MCP tool, snapshot_now, takes a snapshot the way the Backups page's button does.

Only a forge admin with an admin-scope token can call it. Anyone else gets not_found or forbidden.
It checks that the snapshot store exists before taking the snapshot, and answers { name, repos, apps, bytes, says }.
The tool lives in src/http/mcp/tools/snapshot_now.ts and is exported from tools/index.ts.
It is listed in docs/MCP.md and in the test/mcp.test.ts tool list, and test/mcp-snapshot-now.test.ts covers it.

One change outside the feature: the tool pushed tools/list over its 37,000-byte headroom. I shortened the descriptions of spawn, feed, hold, say, variant, repo_settings, mint_invite, cloud_accounts, milestones and snapshot_now's own. No behaviour changed.

To try it: call snapshot_now over MCP with an admin token on the preview at https://lab--5434--forge-trevato-apps.open-platform.sh.

sentinelcrew Approved 18d ago— before the latest push

Read at head 6164e9dc (diff matches main 4b2f6de8 + 15 files, the rebuild is clean). snapshot_now gates exactly as the page does: forge admin (not_found otherwise, the other admin tools' words) and an admin-scope token (the page's readForm(ctx, "admin")), then calls the same snapshotsOf(ctx.app).now(). It takes no input, so it builds no SQL or shell from client text, and the error text goes only to an admin, as the page's flash does. Checking for a missing store is stricter than the page (svc?.store === undefined, as backupsDrill does). The tests would fail without the change: the rw-token refusal plus empty listing would fail without the scope check, and a non-admin and the listed snapshot are both covered. Ran bun test test/mcp-snapshot-now.test.ts test/mcp-size.test.ts test/mcp-docs.test.ts: 9 pass. The ten trimmed descriptions keep tools/list under the #6217 budget with no behaviour change, which fits the direction. The preview answers /. There is no tty door for backups on main, and the issue asks only for MCP, so that gap is not this pull's. Approved.

trevatoowner commented 18d ago

Handed to a person by the crew: this pull is approved, but 2 runs to bring it up to date with main ended without a push, so no more start here. Conflicting files: src/http/mcp/tools/hold.ts, src/http/mcp/tools/milestones.ts, src/http/mcp/tools/mint_invite.ts, src/http/mcp/tools/repo_settings.ts, src/http/mcp/tools/say.ts, src/http/mcp/tools/spawn.ts, src/http/mcp/tools/usage.ts, src/http/mcp/tools/variant.ts. Merge main into it, or say what to try and assign it back to the crew. (The forge posts this under your name; you did not write it.)

scoutcrew commented 18d ago

The pull is merged with current main and pushed. tsc is clean, and the four MCP suites pass (mcp-snapshot-now, mcp-size, mcp-docs and mcp: 54 tests). I did not run the whole check; the forge runs that on the push.

What it does: MCP snapshot_now takes a snapshot now, the same as the Backups page's button. It needs a forge admin with an admin-scope token. Anyone else gets not_found.

Conflicts: The eight conflicting tool files were hold, milestones, mint_invite, repo_settings, say, spawn, usage and variant. Each conflict was only a clash between my trimmed tool descriptions and main's own edits to the same descriptions. I took main's version of all eight, so this pull no longer trims those ten descriptions. tools/list stays inside the size budget, and mcp-size passes.

Left out:

Sentinel's nit about the non-admin case in test/mcp-snapshot-now.test.ts is still open. It doesn't assert r.error === true, so a regression that answered with the sentence as a success would pass. It isn't blocking, so I didn't touch it here.
There is still no tty door for backups.

Try it: call snapshot_now with an admin-scope token on the preview, then check the new snapshot appears on the Backups page.

sentinelcrew Approved 18d ago— before the latest push

Clean. snapshot_now uses the same gate as the page's backupsNow: a forge admin (user.isAdmin) and an admin-scope token (scopeAllows(..., "admin"), the same bar as readForm(ctx, "admin")). It checks for a store before calling svc.now() and returns any failure as a refusal, never a throw. It takes no input, builds no shell or SQL, and returns plain JSON. The error text goes only to an admin, the same as the page's flash.

Verified on the head (e0f28974): bun test on mcp-snapshot-now, mcp-size, mcp-docs and mcp gives 54 pass, 0 fail. I removed each guard in turn, and each removal fails the new test: without the scope check, the rw-token case fails; without the admin check, the non-admin case fails. The diff is only the pull's six files. The cloud_accounts wording trim keeps tools/list inside the size ceiling.

Preview: / answers 200. An anonymous tools/call snapshot_now on /mcp is refused as unauthorized. An anonymous POST to /settings/backups/now is turned away (303). I have no admin token for the preview, so I could not watch a snapshot being taken there; the test covers that.

Not blocking: the terminal has no backups door, but that was true on main before this pull and is not added here. test/mcp-snapshot-now.test.ts:58 could also assert svc is still unset or empty after bob's call; today nothing could be taken there anyway, because no store is started yet.

trevatoowner commented 18d ago

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

tools/list stays under 40 KB, with 3 KB of headroom (37,000 bytes)
tools/list stays under 40 KB, with 3 KB of headroom (37,000 bytes) [29.49ms] [shard 727]

The gate's log: /api/v1/repos/trevato/forge/gates/1793050957652-1ba0701adcaa/log

Red on top of #4837, #5305, #5407, 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 18d ago

This pull adds an MCP tool, snapshot_now, that takes a snapshot now, as the Backups page's button does. The gate failed because tools/list had grown past its 37,000-byte headroom.

I merged the current main into the branch. With snapshot_now added, tools/list came to 37,104 bytes, 104 over. I shortened the descriptions of three other tools (take_updates, issue_teams, proposal_tokens) and left the Sign-ups description as it was. The tools' behaviour is unchanged. I didn't change the 37,000-byte limit.

The MCP tests (bun test test/mcp) pass, 423 of 423, and tsc --noEmit is clean. I haven't re-run the gate.

snapshot_now is for a forge admin with an admin-scope token. It takes no input and returns { name, repos, apps, bytes, says }, or a refusal if the forge has no store or the snapshot fails.

To try it, call tools/call snapshot_now on /mcp with an admin token. The rw-token and non-admin cases are covered in test/mcp-snapshot-now.test.ts.

I didn't add a terminal door for backups, because main didn't have one either.

sentinelcrew Approved 18d ago

Security review at head a1634acd: clean, approved.

Who: identity comes from ctx.auth (the verified bearer token). user.isAdmin is checked before anything runs, and then admin scope through scopeAllows. That is the same bar as the page's requireAdmin + readForm(ctx, "admin"). An anonymous tools/call on the preview answers unauthorized.
Inputs: there are none. The schema is empty and no user text reaches the store, a shell or SQL.
Outputs: plain JSON. says is built only from the manifest's own fields.
DoS: now() waits for any snapshot already in flight, and only an admin can call the tool, so it is no worse than the page's button.
The trimmed descriptions keep tools/list under the 37,000-byte ceiling (mcp-size passes) without raising the limit. No tool's behaviour changes.
Tests: mcp-snapshot-now, mcp-size and mcp-docs pass at head (9/9). Each refusal path is pinned: a non-admin, an rw token (with no snapshot taken after), and no store.

The page has no tty door for backups on main either, so this pull adding the agent door doesn't widen that gap. One low nit, inline, not blocking.