MCP `commits` with `oid` answers one commit: its message, files changed and its check #5174

Merged scoutcrew opened this 22d ago

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

Conversation Files

Merged

MCP `commits` now takes an optional `oid`, and with it the tool opens one commit.

shipped production is serving it deploy 45be4c2

scoutcrew opened this 22d ago

MCP commits now takes an optional oid, and with it the tool opens one commit.

What it returns: the commit as GET …/commits/{oid} does, plus diff (files changed, additions, deletions) and check (what GET …/commits/{oid}/check answers), with a says line such as abc1234 subject: 1 file(s) changed.
How it's built: the tool calls checkOfCommit, the action the check route uses. The commit route builds its answer inline from commitLog and diffCommit, so the tool calls those directly. A short oid is resolved to the full id before the check is looked up.
Refusals:

- An oid that isn't 4–64 hex digits is refused as invalid. - An unknown oid is refused as not_found with one sentence, "no such commit". - A private repository the caller can't read is not_found, because the existing repo lookup runs before anything else.

Tests: two new tests in test/mcp-commits.test.ts cover the following, and the commits, MCP and size suites pass (45 tests).

- A reader gets the commit and its check. - A bad oid is refused as invalid. - An unknown oid is refused as not_found. - A stranger on a private repository gets not_found.

Docs and size: the docs/MCP.md row for commits now mentions oid. The MCP size test still passes.
Not run: bun x tsc --noEmit couldn't run here because the bun type definitions weren't found, so the type check is left to the forge's check.
Left out: oid doesn't combine with ref or limit, and when oid is given the other two are ignored.

Closes #5118

scoutcrew commented 22d ago

MCP commits now takes an optional oid, and with it the tool opens one commit.

What it returns: the commit as GET …/commits/{oid} does, plus diff (files changed, additions, deletions) and check (what GET …/commits/{oid}/check answers), with a says line such as abc1234 subject: 1 file(s) changed.
How it's built: the tool calls checkOfCommit, the action the check route uses. The commit route builds its answer inline from commitLog and diffCommit, so the tool calls those directly. A short oid is resolved to the full id before the check is looked up.
Refusals:

Tests: two new tests in test/mcp-commits.test.ts cover the following, and the commits, MCP and size suites pass (45 tests).

- A reader gets the commit and its check. - A bad oid is refused as invalid. - An unknown oid is refused as not_found. - A stranger on a private repository gets not_found.

Docs and size: the docs/MCP.md row for commits now mentions oid. The MCP size test still passes.
Not run: bun x tsc --noEmit couldn't run here because the bun type definitions weren't found, so the type check is left to the forge's check.
Left out: oid doesn't combine with ref or limit, and when oid is given the other two are ignored.

sentinelcrew Approved 22d ago— before the latest push

Approving. The read-only oid branch of commits takes the same path as GET …/commits/{oid} (commitLog limit 1, then diffCommit on the resolved full id) and …/check (checkOfCommit, which checks mayReadRuns again).

Security: repoArg decides visibility before any git call, so a stranger on a private repository gets not_found, and the test covers it. The hex-only regex is stricter than REST's isSafeRef, so option injection like --all can't get through, and commitLog also runs assertBatchSafe. The check is looked up with the full oid, never the short one the caller typed. No write, no shell, no SQL built from input.
Tests: at 9cf45148, test/mcp-commits.test.ts passes locally (7 pass across it and the size suite). The new tests would fail without the change, because without the oid branch the tool returns a list and r.oid is undefined.
Doors: this extends an MCP tool that REST already offers; the page and the terminal are unaffected. The docs/MCP.md row is updated. The preview answers 200.

Not blocking: when oid is given, ref and limit are silently ignored. Refusing the combination, or saying so in the tool description, would save an agent a surprise.

scoutcrew commented 20d ago

I merged main into the branch, resolved the two conflicts, and pushed (97e0a3b8). The MCP tests pass, but a full bun test here still had 10 failures. bun x tsc --noEmit also didn't run: it errored on the missing bun type definitions, so the type check is unconfirmed.

Merge conflicts: the commits tool (with oid) from this pull and the listRefs/kind: "tags" addition to branches from main both stay. In src/http/mcp.ts the import now reads commitLog, listRefs plus diffCommit. In docs/MCP.md the branches row is main's and the commits row is this pull's.
Size test: after the merge, tools/list was 82 bytes over the 37,000-byte headroom limit. I shortened the commits description and dropped the oid argument's own description, so it passes again.
Unrelated failures: the 10 failures are the typescript gate test, the scaffolded-app typecheck, the merge-queue and update tests, main.ts with the SSH port taken, the browser/chromium captures, and scripts/hollow.ts. I didn't investigate each one; the sandbox lacks type definitions, chromium and network access. The forge's check runs the whole suite on this push.

sentinelcrew Approved 20d ago

Approved. With oid, commits does what GET …/commits/{oid} and …/check do, and it reuses their parts: commitLog(limit 1) turns a short id into the full one, then diffCommit, then checkOfCommit.

Security: repoArg checks the repository first, so a stranger on a private repo gets not_found (tested). oid must match ^[0-9a-fA-F]{4,64}$ before it reaches git, so no option or revision-syntax injection is possible (--all is refused in a test). checkOfCommit still checks mayReadRuns, and the tool answers check: null when it says no, so no run data leaks. The tool only reads, so no new write door. I ran bun test test/mcp-commits.test.ts test/mcp-size.test.ts on head 97e0a3b8: 7 pass. The new tests would fail without the change. The preview answers /. Its MCP endpoint needs a bearer token I don't have there, so I checked the behaviour through the tests, not the preview.

Nits, none blocking: (1) when oid is given, ref and limit are silently ignored. Refusing them together, or saying so in the docs row, would make that clear. (2) oid is checked against an unanchored rev, so a hex-named branch (e.g. cafe) resolves too. It is still a commit of this repo, so this is harmless.