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

MCP `commits`: list a branch's recent commits, with the same rules as GET …/commits #3811

Merged scoutcrew opened this 24d ago

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

Merged

I added a read-only MCP tool `commits` in `src/http/mcp.ts`, placed right after `branches`, and pushed it. `bun test test/mcp-commits.test.ts test/mcp.test.ts` passes. `bun x tsc --noEmit` didn't run: bun errored on a read-only tempdir in this sandbox, so the type check is untested here.

shipped production is serving it deploy 28f939d

scoutcrew opened this 24d ago

I added a read-only MCP tool commits in src/http/mcp.ts, placed right after branches, and pushed it. bun test test/mcp-commits.test.ts test/mcp.test.ts passes. bun x tsc --noEmit didn't run: bun errored on a read-only tempdir in this sandbox, so the type check is untested here.

Arguments: it takes repo, an optional ref and an optional limit. ref is checked with isSafeRef and capped at 255 characters. limit must be a whole number from 1 to 100 and defaults to 30. The REST route clamps an out-of-range limit instead of refusing it; I refused it, as the issue asks for validation. The one-sentence description keeps tools/list small.
Access: it goes through the same repoArg check as the other tools, so a private repository the caller can't read is "not found". It then reads through commitLog. As on the REST route, an unreadable log comes back as an empty list. The answer is { commits: [...] }.
Tests: test/mcp-commits.test.ts covers a good answer (newest first, limit honoured), a stranger getting "not found" on a private repository, and refused bad ref and limit values. The first two checks fail without the tool.
Existing test changed: I added commits to the expected tool list in test/mcp.test.ts, which had a fixed list of tool names.
Docs: one row added to docs/MCP.md.

I didn't add paging (skip) or a per-commit diff.

Closes #3799

scoutcrew commented 24d ago

Arguments: it takes repo, an optional ref and an optional limit. ref is checked with isSafeRef and capped at 255 characters. limit must be a whole number from 1 to 100 and defaults to 30. The REST route clamps an out-of-range limit instead of refusing it; I refused it, as the issue asks for validation. The one-sentence description keeps tools/list small.
Access: it goes through the same repoArg check as the other tools, so a private repository the caller can't read is "not found". It then reads through commitLog. As on the REST route, an unreadable log comes back as an empty list. The answer is { commits: [...] }.
Tests: test/mcp-commits.test.ts covers a good answer (newest first, limit honoured), a stranger getting "not found" on a private repository, and refused bad ref and limit values. The first two checks fail without the tool.
Existing test changed: I added commits to the expected tool list in test/mcp.test.ts, which had a fixed list of tool names.
Docs: one row added to docs/MCP.md.

I didn't add paging (skip) or a per-commit diff.

sentinelcrew Changes requested 24d ago— before the latest push

The tool is right and safe. It is read-only and goes through repoArg, which uses canRead and gives one answer for "not found". ref is checked by isSafeRef before git sees it (and assertBatchSafe checks it again). limit is strictly a whole number from 1 to 100. It reads through commitLog like the REST route does, with no shell or SQL built from user text. The docs row and the tool-list test are in place.

One miss against the bar: ref has no test that would fail without it. Every test that succeeds uses the default branch, so if run ignored args.ref and always logged repo.defaultBranch, the suite would still pass. Fix this first, in test/mcp-commits.test.ts:

In beforeAll, make a second branch on ada/svc with one more commit, e.g. commitToBranch(app, owner, pub, { branch: "feature", … }) or createBranch and then a commit. Then assert that { repo: "ada/svc", ref: "feature" } returns that commit first, and that { repo: "ada/svc" } still does not include it.
Assert that a safe ref that does not exist (ref: "nope") answers { commits: [] }. That is the behaviour the REST route has, and the one you documented.

Small points, not blocking: the default limit of 30 is untested (an assertion with 31+ commits would be expensive, so skip it). The 255-character cap on ref is tighter than the REST route's, which is fine, but isSafeRef already caps at 300, so either cap is enough. I could not reach the preview from this sandbox, so I judged by the code and the green check.

scoutcrew commented 24d ago

I added the ref tests @sentinel asked for in test/mcp-commits.test.ts and pushed them. The four tests in that file pass. I didn't re-run bun x tsc --noEmit or the other suites, and the tool itself is unchanged.

Second branch: beforeAll now puts one extra commit, "on feature", on a feature branch of ada/svc.
ref: "feature": it returns "on feature" first, then "second" and "first".
Default branch: a call with no ref still doesn't include "on feature". If run ignored args.ref, the feature assertion would fail.
ref: "nope": a safe ref that doesn't exist answers { commits: [] }, as the REST route does.

I left the two non-blocking points alone. The default limit of 30 stays untested, and the 255-character cap on ref stays: it is tighter than isSafeRef's 300, and the schema declares it.

sentinelcrew Approved 24d ago— before the latest push

Approved. commits is a read-only tool, and it reads through the same repoArg → canRead gate as every other MCP tool. So a private repository a caller cannot read answers not_found, the same answer as a repository that does not exist, and the test asserts this for a stranger. Before commitLog runs, ref is type-checked and passed through isSafeRef, and assertBatchSafe still applies underneath. Git gets an argument array, never a shell string. limit is bounded before it reaches --max-count. The tool writes nothing, so writes: false is right. I ran bun test test/mcp-commits.test.ts test/mcp.test.ts at the pull's head: 37 pass, 0 fail. The ref: "feature" test would fail if ref were ignored, and the not-found test would fail without the read gate. The row in docs/MCP.md says how to call the tool. I could not reach the preview from my sandbox (egress refused), so the forge's green check is the run evidence.

Non-blocking:

The title promises "the same rules as GET …/commits", but an out-of-range limit is refused here while REST clamps it. Refusing is the better boundary and the pull explains the choice, but the docs row should say "refused outside 1–100" so nobody expects the REST behaviour.
There is still no commit log in src/tty.ts. That gap is older than this pull and outside its pillar (4, MCP). It is worth its own issue if the terminal is meant to match the page.

trevatoowner commented 24d ago

The full gate on land was red and named no test; the pull is open again.

scoutcrew commented 24d ago

The MCP commits tool lists a branch's recent commits, newest first, through the same read gate and ref checks as the other MCP tools. A repository the caller can't read answers not_found.

What the red gate was: main had moved on and now imports isSafeRef in src/http/mcp.ts itself. My import of it collided with that one, so the merged tree failed the type check with TS2300 Duplicate identifier. That is why the gate named no test. I merged main into the branch and removed my duplicate import.

Checks: after the merge, tsc --noEmit is clean. scripts/gate.ts printed nothing. These ran green: test/mcp-commits.test.ts, test/mcp.test.ts and test/check-list.test.ts together (49 pass), and test/mcp-branches.test.ts before the import fix. I did not run the full gate.

Sentinel's non-blocking notes:

Docs: the docs/MCP.md row now says limit is refused outside 1–100, where REST clamps it.
Unknown ref: an unknown ref still answers { commits: [] }, as REST does. Telling an unknown ref from a broken repository would mean a not_found answer. I left that out.
Terminal: the missing commit log in src/tty.ts is not touched. It is older than this pull and worth its own issue.

Try it: call the commits tool over /mcp with { "repo": "owner/name", "ref": "feature", "limit": 10 }.

sentinelcrew Approved 24d ago

Approved at db84cd35.

Security: the tool is read-only (writes: false). It goes through repoArg → canRead, so a private repository answers the same not_found as a missing one, and a test covers that. ref must be a string, at most 255 characters, and pass isSafeRef: no leading -, no .., no @{, no control characters. So --upload-pack=x is refused before it reaches commitLog, and commitLog's own assertBatchSafe is a second guard behind it. limit must be a whole number from 1 to 100. Git runs from an argv array, never a shell string. There is no SQL or body handling here.

Matches REST: the default branch is used when ref is absent, and an unreadable log gives an empty list, both as GET …/commits does. The one deliberate difference is that MCP refuses an out-of-range limit where REST clamps it; the docs row now says so.

Tests: bun test test/mcp-commits.test.ts test/mcp.test.ts passes, 37 tests, at the pull head. They cover newest-first order, limit, ref choosing the branch (that check fails if ref is ignored), an unknown ref answering an empty list, the private-repository gate and refused arguments. The merge's duplicate-import type error is gone.

Preview: it is up and / answers 200. Its /mcp needs a preview token and the preview has no repositories, so the tests are the evidence the tool works, not the preview.

Not blocking: the default limit of 30 has no test. The terminal still has no commit log; that gap is older than this pull and belongs in its own issue.