diff --git a/.changeset/mcp-api-optimizations.md b/.changeset/mcp-api-optimizations.md new file mode 100644 index 0000000..c35284d --- /dev/null +++ b/.changeset/mcp-api-optimizations.md @@ -0,0 +1,9 @@ +--- +"@contentrain/mcp": minor +--- + +Three additive GitHub-provider optimizations that cut API calls per write (no breaking changes) + +- **applyPlan blob inlining** — `applyPlanToGitHub` now inlines file content directly in the Git tree entries instead of POSTing a blob per file. A write drops from `N+3` GitHub mutations to a fixed `3` (tree + commit + ref) regardless of file count, easing the mutation-rate secondary limit. This brings the GitHub provider to parity with the GitLab provider, which already inlines content in its commit actions. Deletions are unchanged (null-sha entries); content is passed through byte-for-byte. +- **GitHubReader opt-in read memoization** — `new GitHubReader(client, repo, { memoize: true })` deduplicates repeated `(path, ref)` reads within one operation across `readFile` / `listDirectory` / `fileExists`. Opt-in and off by default; failed reads are evicted so transient errors are retried, not cached. The provider's own long-lived reader does not enable it (a reader that outlives a write must not serve stale results). +- **buildContextChange stats injection** — `buildContextChange(reader, operation, source, { stats })` lets a caller that already knows the model/entry counts skip the O(models·locales) reader walk; only `readConfig` remains. The emitted context.json is byte-identical to the scanned variant. Parameterless callers are unaffected. diff --git a/packages/mcp/src/core/context.ts b/packages/mcp/src/core/context.ts index 0993883..8133799 100644 --- a/packages/mcp/src/core/context.ts +++ b/packages/mcp/src/core/context.ts @@ -8,6 +8,17 @@ import { readConfig } from './config.js' const CONTEXT_PATH = '.contentrain/context.json' +/** + * Pre-computed stats a caller can pass to {@link buildContextChange} to skip + * the model/entry scan. `entries` is nullable — a `null` is dropped from the + * emitted context.json by {@link canonicalStringify}, exactly as a failed + * scan is today. + */ +export interface ContextStats { + models: number + entries: number | null +} + /** * Read the committed `.contentrain/context.json` payload written by the * most recent content or model operation. Returns `null` when the file @@ -94,27 +105,41 @@ export async function writeContext( * read surface. GitHubProvider pays an extra round trip per model; the * payoff is a context.json that matches what the local write path emits, * so cross-provider merges stay deterministic. + * + * A caller that already knows the model/entry counts (e.g. Studio deriving + * them from its own index) can pass `opts.stats` to skip that O(models· + * locales) scan entirely — only `readConfig` remains, for the locale list + * and the `lastOperation.locale` default. The emitted context.json is + * byte-identical to the scanned variant for the same logical state. */ export async function buildContextChange( reader: RepoReader, operation: { tool: string, model: string, locale?: string, entries?: string[] }, source?: ContextSource, + opts?: { stats?: ContextStats }, ): Promise { const config = await readConfig(reader) - const models = await listModels(reader) const locales = config?.locales.supported ?? ['en'] - let totalEntries: number | null = null - try { - const fullModels = await Promise.all(models.map(m => readModel(reader, m.id))) - const counts = await Promise.all( - fullModels - .filter((m): m is NonNullable => m !== null) - .map(m => countEntries(reader, m)), - ) - totalEntries = counts.reduce((acc, c) => acc + c.total, 0) - } catch { - totalEntries = null + let modelCount: number + let totalEntries: number | null + if (opts?.stats) { + modelCount = opts.stats.models + totalEntries = opts.stats.entries + } else { + const models = await listModels(reader) + modelCount = models.length + try { + const fullModels = await Promise.all(models.map(m => readModel(reader, m.id))) + const counts = await Promise.all( + fullModels + .filter((m): m is NonNullable => m !== null) + .map(m => countEntries(reader, m)), + ) + totalEntries = counts.reduce((acc, c) => acc + c.total, 0) + } catch { + totalEntries = null + } } const context: ContextJson = { @@ -128,7 +153,7 @@ export async function buildContextChange( source: resolveSource(source), }, stats: { - models: models.length, + models: modelCount, entries: totalEntries as number, locales, lastSync: new Date().toISOString(), diff --git a/packages/mcp/src/providers/github/apply-plan.ts b/packages/mcp/src/providers/github/apply-plan.ts index 5460527..513605e 100644 --- a/packages/mcp/src/providers/github/apply-plan.ts +++ b/packages/mcp/src/providers/github/apply-plan.ts @@ -4,12 +4,12 @@ import { isNotFoundError, resolveRepoPath } from '../shared/index.js' import type { GitHubClient } from './client.js' import type { RepoRef } from './types.js' -interface TreeEntry { - path: string - mode: '100644' | '100755' | '040000' | '160000' | '120000' - type: 'blob' | 'tree' | 'commit' - sha: string | null -} +// A GitHub Git-tree entry. Writes carry `content` inline (GitHub creates the +// blob as part of createTree); deletions carry `sha: null`. The two are +// mutually exclusive — an entry may set `content` OR `sha`, never both. +type TreeEntry = + | { path: string, mode: '100644', type: 'blob', content: string } + | { path: string, mode: '100644', type: 'blob', sha: null } /** * Apply a plan to a GitHub repository as a single atomic commit via the @@ -19,8 +19,12 @@ interface TreeEntry { * branch, or the HEAD of `input.base` (or the repo's default branch) * when the target branch does not yet exist. * 2. Read the base tree SHA from that commit. - * 3. Walk `input.changes` in parallel — create a blob per non-null - * content, emit a tree entry with `sha: null` for each deletion. + * 3. Map `input.changes` to tree entries — `content` inline for each write, + * `sha: null` for each deletion. No per-file blob round trip: GitHub + * creates the blobs as part of `createTree`, which keeps the write to a + * fixed 3 mutations (tree + commit + ref) regardless of file count and + * stays under the mutation-rate secondary limit. Mirrors the GitLab + * provider, which already inlines content in its commit actions. * 4. Create a new tree layered on top of the base tree with the collected * entries. * 5. Create the commit (tree, parents, author). @@ -44,9 +48,7 @@ export async function applyPlanToGitHub( }) const baseTreeSha = baseCommit.data.tree.sha - const treeEntries = await Promise.all( - input.changes.map(change => buildTreeEntry(client, repo, change)), - ) + const treeEntries = input.changes.map(change => buildTreeEntry(repo, change)) const tree = await client.rest.git.createTree({ owner: repo.owner, @@ -96,22 +98,15 @@ export async function applyPlanToGitHub( } } -async function buildTreeEntry( - client: GitHubClient, - repo: RepoRef, - change: FileChange, -): Promise { +function buildTreeEntry(repo: RepoRef, change: FileChange): TreeEntry { const path = resolveRepoPath(repo.contentRoot, change.path) if (change.content === null) { return { path, mode: '100644', type: 'blob', sha: null } } - const blob = await client.rest.git.createBlob({ - owner: repo.owner, - repo: repo.name, - content: change.content, - encoding: 'utf-8', - }) - return { path, mode: '100644', type: 'blob', sha: blob.data.sha } + // Inline UTF-8 content — the write path never produces binary/base64, so + // there is no blob-encoding branch to preserve. GitHub creates the blob + // when the tree is created. + return { path, mode: '100644', type: 'blob', content: change.content } } async function resolveBaseSha( diff --git a/packages/mcp/src/providers/github/reader.ts b/packages/mcp/src/providers/github/reader.ts index de20ea1..345f6bd 100644 --- a/packages/mcp/src/providers/github/reader.ts +++ b/packages/mcp/src/providers/github/reader.ts @@ -15,15 +15,46 @@ import type { RepoRef } from './types.js' * commit SHA. When omitted, GitHub resolves to the repository's default * branch — which is usually wrong for Contentrain flows, so callers * should always pass the explicit `contentrain` tracking branch. + * + * Pass `{ memoize: true }` to dedupe reads within one operation: a repeated + * `(path, ref)` returns the in-flight/cached promise instead of a fresh + * `getContent`. This is OPT-IN and only safe for a SHORT-LIVED, read-only + * reader — a long-lived reader that outlives a write would serve stale + * results, so the provider's own reader never enables it. Failed reads are + * evicted so a transient error is retried, not cached forever. */ export class GitHubReader implements RepoReader { + private readonly fileMemo = new Map>() + private readonly listMemo = new Map>() + private readonly existsMemo = new Map>() + constructor( private readonly client: GitHubClient, private readonly repo: RepoRef, + private readonly opts?: { memoize?: boolean }, ) {} + /** + * Run `fetch` through the given memo when memoization is enabled, keyed by + * `(ref, repoPath)`. A rejected promise is evicted so the next call retries. + */ + private memoized(memo: Map>, repoPath: string, ref: string | undefined, fetch: () => Promise): Promise { + if (!this.opts?.memoize) return fetch() + const key = `${ref ?? ''}:${repoPath}` + const cached = memo.get(key) + if (cached) return cached + const promise = fetch() + memo.set(key, promise) + promise.catch(() => memo.delete(key)) + return promise + } + async readFile(path: string, ref?: string): Promise { const repoPath = resolveRepoPath(this.repo.contentRoot, path) + return this.memoized(this.fileMemo, repoPath, ref, () => this.readFileUncached(path, repoPath, ref)) + } + + private async readFileUncached(path: string, repoPath: string, ref?: string): Promise { const response = await this.client.rest.repos.getContent({ owner: this.repo.owner, repo: this.repo.name, @@ -59,35 +90,39 @@ export class GitHubReader implements RepoReader { async listDirectory(path: string, ref?: string): Promise { const repoPath = resolveRepoPath(this.repo.contentRoot, path) - try { - const response = await this.client.rest.repos.getContent({ - owner: this.repo.owner, - repo: this.repo.name, - path: repoPath, - ref, - }) - const data = response.data - if (!Array.isArray(data)) return [] - return data.map(entry => entry.name) - } catch (error) { - if (isNotFoundError(error)) return [] - throw error - } + return this.memoized(this.listMemo, repoPath, ref, async () => { + try { + const response = await this.client.rest.repos.getContent({ + owner: this.repo.owner, + repo: this.repo.name, + path: repoPath, + ref, + }) + const data = response.data + if (!Array.isArray(data)) return [] + return data.map(entry => entry.name) + } catch (error) { + if (isNotFoundError(error)) return [] + throw error + } + }) } async fileExists(path: string, ref?: string): Promise { const repoPath = resolveRepoPath(this.repo.contentRoot, path) - try { - await this.client.rest.repos.getContent({ - owner: this.repo.owner, - repo: this.repo.name, - path: repoPath, - ref, - }) - return true - } catch (error) { - if (isNotFoundError(error)) return false - throw error - } + return this.memoized(this.existsMemo, repoPath, ref, async () => { + try { + await this.client.rest.repos.getContent({ + owner: this.repo.owner, + repo: this.repo.name, + path: repoPath, + ref, + }) + return true + } catch (error) { + if (isNotFoundError(error)) return false + throw error + } + }) } } diff --git a/packages/mcp/tests/core/context.test.ts b/packages/mcp/tests/core/context.test.ts new file mode 100644 index 0000000..2341d76 --- /dev/null +++ b/packages/mcp/tests/core/context.test.ts @@ -0,0 +1,111 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { RepoReader } from '../../src/core/contracts/index.js' +import { buildContextChange, type ContextStats } from '../../src/core/context.js' + +/** + * buildContextChange stats-injection: passing pre-computed `opts.stats` + * must produce a byte-identical context.json to the scanned variant while + * skipping the O(models·locales) reader walk. Uses an in-memory reader so + * there are no git subprocesses. + */ + +class MemoryReader implements RepoReader { + constructor(private readonly files: Map) {} + + async readFile(path: string): Promise { + const content = this.files.get(path) + if (content === undefined) throw new Error(`not found: ${path}`) + return content + } + + async listDirectory(path: string): Promise { + const prefix = `${path}/` + const names = new Set() + for (const key of this.files.keys()) { + if (!key.startsWith(prefix)) continue + const rest = key.slice(prefix.length) + names.add(rest.split('/')[0]!) + } + return [...names] + } + + async fileExists(path: string): Promise { + if (this.files.has(path)) return true + const prefix = `${path}/` + for (const key of this.files.keys()) if (key.startsWith(prefix)) return true + return false + } +} + +function fixtureReader(): MemoryReader { + const files = new Map() + files.set('.contentrain/config.json', JSON.stringify({ + version: 1, + stack: 'other', + workflow: 'auto-merge', + locales: { default: 'en', supported: ['en', 'tr'] }, + domains: ['blog'], + })) + files.set('.contentrain/models/hero.json', JSON.stringify({ + id: 'hero', name: 'Hero', kind: 'singleton', domain: 'blog', i18n: true, + fields: { title: { type: 'string', required: true } }, + })) + return new MemoryReader(files) +} + +const OP = { tool: 'contentrain_content_save', model: 'hero', locale: 'en' } + +beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(new Date('2026-07-09T12:00:00.000Z')) +}) + +afterEach(() => { + vi.useRealTimers() +}) + +describe('buildContextChange stats injection', () => { + it('produces byte-identical output to the scanned variant', async () => { + const reader = fixtureReader() + + const scanned = await buildContextChange(reader, OP) + // Read back what the scan computed, then inject exactly that. + const scannedStats = JSON.parse(scanned.content).stats as { models: number, entries?: number } + const stats: ContextStats = { models: scannedStats.models, entries: scannedStats.entries ?? null } + + const injected = await buildContextChange(reader, OP, undefined, { stats }) + + expect(injected.path).toBe(scanned.path) + expect(injected.content).toBe(scanned.content) + }) + + it('skips the model/entry scan when stats are injected', async () => { + const reader = fixtureReader() + const listSpy = vi.spyOn(reader, 'listDirectory') + + await buildContextChange(reader, OP, undefined, { stats: { models: 1, entries: 0 } }) + + // listModels / countEntries both go through listDirectory; the injected + // path must not touch it (only readConfig's readFile runs). + expect(listSpy).not.toHaveBeenCalled() + }) + + it('still walks the reader when stats are NOT injected', async () => { + const reader = fixtureReader() + const listSpy = vi.spyOn(reader, 'listDirectory') + + await buildContextChange(reader, OP) + + expect(listSpy).toHaveBeenCalled() // listModels reads .contentrain/models + }) + + it('drops a null entries count from the emitted stats', async () => { + const reader = fixtureReader() + + const change = await buildContextChange(reader, OP, undefined, { stats: { models: 2, entries: null } }) + const stats = JSON.parse(change.content).stats as Record + + expect(stats['models']).toBe(2) + expect('entries' in stats).toBe(false) + }) +}) diff --git a/packages/mcp/tests/providers/github/apply-plan.test.ts b/packages/mcp/tests/providers/github/apply-plan.test.ts index 29cbad1..182c31b 100644 --- a/packages/mcp/tests/providers/github/apply-plan.test.ts +++ b/packages/mcp/tests/providers/github/apply-plan.test.ts @@ -4,9 +4,10 @@ import type { GitHubClient } from '../../../src/providers/github/client.js' /** * applyPlanToGitHub unit tests — exercise the Git Data API write flow - * against a mocked Octokit. Validates that blobs are created for writes, - * deletions emit null-sha tree entries, branches are created vs. updated - * depending on existence, and the default branch is the fallback base. + * against a mocked Octokit. Validates that writes carry their content + * inline in the tree (no per-file blob round trip), deletions emit + * null-sha tree entries, branches are created vs. updated depending on + * existence, and the default branch is the fallback base. */ interface StubShape { @@ -69,12 +70,12 @@ describe('applyPlanToGitHub', () => { expect(commit.sha).toBe('new-commit') expect(updateRef).toHaveBeenCalledWith({ owner: 'o', repo: 'r', ref: 'heads/cr/test', sha: 'new-commit' }) - expect(createBlob).toHaveBeenCalledWith({ - owner: 'o', - repo: 'r', - content: '{}', - encoding: 'utf-8', - }) + // Content is inlined into the tree entry — no per-file blob POST. + expect(createBlob).not.toHaveBeenCalled() + const treeArg = createTree.mock.calls[0]![0].tree + expect(treeArg).toEqual([ + { path: '.contentrain/config.json', mode: '100644', type: 'blob', content: '{}' }, + ]) }) it('creates a branch from input.base when the target does not exist', async () => { @@ -166,18 +167,46 @@ describe('applyPlanToGitHub', () => { author: AUTHOR, }) - // Exactly one blob was created (the keep), the deletion should not trigger createBlob. - expect(createBlob).toHaveBeenCalledTimes(1) - expect(createBlob).toHaveBeenCalledWith({ owner: 'o', repo: 'r', content: '{}', encoding: 'utf-8' }) + // No blob POSTs at all — writes go inline, deletions carry null sha. + expect(createBlob).not.toHaveBeenCalled() - // The tree passed to createTree contains a null-sha entry for the deleted path. + // The tree passed to createTree: inline content for the keep, null-sha + // entry for the delete. const treeArg = createTree.mock.calls[0]![0].tree expect(treeArg).toEqual(expect.arrayContaining([ - { path: 'keep.json', mode: '100644', type: 'blob', sha: 'b' }, + { path: 'keep.json', mode: '100644', type: 'blob', content: '{}' }, { path: 'gone.json', mode: '100644', type: 'blob', sha: null }, ])) }) + it('passes write content through byte-for-byte (unicode, emoji, newlines)', async () => { + const getRef = vi.fn().mockResolvedValue({ data: { object: { sha: 'base' } } }) + const getCommit = vi.fn().mockResolvedValue({ data: { tree: { sha: 'bt' } } }) + const createBlob = vi.fn() + const createTree = vi.fn().mockResolvedValue({ data: { sha: 't' } }) + const createCommit = vi.fn().mockResolvedValue({ + data: { + sha: 'c', + message: 'm', + author: { name: 'MCP', email: 'ai@contentrain.io', date: '2026-01-01T00:00:00Z' }, + }, + }) + const updateRef = vi.fn().mockResolvedValue({}) + + const payload = '{\n "title": "Merhaba 🚀",\n "emoji": "café — naïve"\n}\n' + const client = mockClient({ getRef, getCommit, createBlob, createTree, createCommit, updateRef }) + await applyPlanToGitHub(client, REPO, { + branch: 'cr/utf8', + changes: [{ path: 'body.json', content: payload }], + message: 'utf8', + author: AUTHOR, + }) + + expect(createBlob).not.toHaveBeenCalled() + const treeArg = createTree.mock.calls[0]![0].tree + expect(treeArg[0].content).toBe(payload) + }) + it('prefixes change paths with repo.contentRoot', async () => { const getRef = vi.fn().mockResolvedValue({ data: { object: { sha: 'base' } } }) const getCommit = vi.fn().mockResolvedValue({ data: { tree: { sha: 'bt' } } }) diff --git a/packages/mcp/tests/providers/github/reader.test.ts b/packages/mcp/tests/providers/github/reader.test.ts index 0df370f..32f5f67 100644 --- a/packages/mcp/tests/providers/github/reader.test.ts +++ b/packages/mcp/tests/providers/github/reader.test.ts @@ -160,3 +160,69 @@ describe('GitHubReader.fileExists', () => { await expect(reader.fileExists('a.json')).rejects.toThrow(/Rate limited/) }) }) + +describe('GitHubReader memoization (opt-in)', () => { + function fileResponse(body: string) { + return { data: { type: 'file', encoding: 'base64', content: b64(body), size: body.length, sha: 'a' } } + } + + it('is OFF by default — repeated reads hit getContent every time', async () => { + const getContent = vi.fn().mockResolvedValue(fileResponse('{}')) + const reader = new GitHubReader(mockClient({ getContent }), { owner: 'o', name: 'r' }) + + await reader.readFile('a.json', 'contentrain') + await reader.readFile('a.json', 'contentrain') + + expect(getContent).toHaveBeenCalledTimes(2) + }) + + it('when ON, dedupes repeated readFile for the same (path, ref) to one getContent', async () => { + const getContent = vi.fn().mockResolvedValue(fileResponse('{}')) + const reader = new GitHubReader(mockClient({ getContent }), { owner: 'o', name: 'r' }, { memoize: true }) + + const [a, b] = await Promise.all([ + reader.readFile('a.json', 'contentrain'), + reader.readFile('a.json', 'contentrain'), + ]) + await reader.readFile('a.json', 'contentrain') + + expect(a).toBe('{}') + expect(b).toBe('{}') + expect(getContent).toHaveBeenCalledTimes(1) + }) + + it('when ON, keys by ref — the same path on a different ref is a separate read', async () => { + const getContent = vi.fn().mockResolvedValue(fileResponse('{}')) + const reader = new GitHubReader(mockClient({ getContent }), { owner: 'o', name: 'r' }, { memoize: true }) + + await reader.readFile('a.json', 'contentrain') + await reader.readFile('a.json', 'main') + + expect(getContent).toHaveBeenCalledTimes(2) + }) + + it('when ON, dedupes listDirectory and fileExists independently', async () => { + const getContent = vi.fn().mockResolvedValue({ data: [{ name: 'en.json' }] }) + const reader = new GitHubReader(mockClient({ getContent }), { owner: 'o', name: 'r' }, { memoize: true }) + + await reader.listDirectory('dir', 'contentrain') + await reader.listDirectory('dir', 'contentrain') + await reader.fileExists('dir', 'contentrain') + await reader.fileExists('dir', 'contentrain') + + // 1 for listDirectory, 1 for fileExists — each method has its own memo. + expect(getContent).toHaveBeenCalledTimes(2) + }) + + it('when ON, does NOT cache a rejected read — the next call retries', async () => { + const getContent = vi.fn() + .mockRejectedValueOnce(Object.assign(new Error('Server Error'), { status: 500 })) + .mockResolvedValueOnce(fileResponse('{"ok":true}')) + const reader = new GitHubReader(mockClient({ getContent }), { owner: 'o', name: 'r' }, { memoize: true }) + + await expect(reader.readFile('a.json', 'contentrain')).rejects.toThrow(/Server Error/) + // Second call must issue a fresh request rather than replay the failure. + expect(await reader.readFile('a.json', 'contentrain')).toBe('{"ok":true}') + expect(getContent).toHaveBeenCalledTimes(2) + }) +})