From f3dac1847e7ed8c3f4e47081250b52ee9ece7a7a Mon Sep 17 00:00:00 2001 From: Aaron Bockelie Date: Tue, 19 May 2026 00:30:31 -0500 Subject: [PATCH] feat(vault.read): faithful-by-default reads with char-budget pagination (ADR-203, #133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Default vault.read no longer fragments-and-flattens. New contract: - fits READ_PAGE_CHARS (50000) → whole file, byte-exact, one load (common case) - exceeds → verbatim page 1, single contiguous block, with line bookends (lineStart/lineEnd/totalLines + nextPage); page=N to continue. Absolute line numbers preserved so edit.at_line still works on large files. - returnFullFile:true → entire file verbatim (explicit large override; param retained & repurposed, not retired) - query/strategy/maxFragments → semantic fragments (unchanged) - structured envelope no longer double-encodes the body (metadata sans body) Budget is char-based on purpose: line count is an invalid proxy for context cost; only bookends are line-based (for at_line). Hard invariant: a default read must never hand the agent a context-breaking raw dump. Also fixes the latent formatFileRead crash (_Formatter error_) on full-file shapes; formatter now renders verbatim + a Pagination section. src/utils/file-reader.ts rewritten; vault.ts threads `page`; formatter hardened; tool description + CHANGELOG (breaking) updated. make check green (0 errors, baseline 5 warnings, 243/243 incl. 8 new ADR-203 round-trip/pagination/fidelity tests). --- CHANGELOG.md | 3 + src/formatters/vault.ts | 55 +++++- src/semantic/operations/vault.ts | 1 + src/tools/semantic-tools.ts | 4 +- src/utils/file-reader.ts | 268 ++++++++++++++++++++++++------ tests/vault-read-fidelity.test.ts | 123 ++++++++++++++ 6 files changed, 389 insertions(+), 65 deletions(-) create mode 100644 tests/vault-read-fidelity.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index cebdb62..444e811 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- ⚠️ BREAKING: `vault.read` is now **faithful by default** (ADR-203, [#133](https://github.com/aaronsb/obsidian-mcp-plugin/issues/133)). It returns the **complete, byte-exact file source** (no more newline-flattened fragments) when the file fits a ~50k-char budget. Large files return a **verbatim page 1 with absolute line bookends** (`page=N` to continue) instead of a context-breaking raw dump. `returnFullFile: true` is the explicit whole-large-file override; `query`/`strategy`/`maxFragments` still return semantic fragments. The structured envelope no longer double-encodes the body. Clients that relied on the old fragmented default should pass fragment params explicitly. + ### Security - 🔴 CRITICAL: Identified authentication vulnerability - no API key validation ([#9](https://github.com/aaronsb/obsidian-mcp-plugin/issues/9)) - 🔴 CRITICAL: Identified path traversal vulnerability in file operations ([#10](https://github.com/aaronsb/obsidian-mcp-plugin/issues/10)) diff --git a/src/formatters/vault.ts b/src/formatters/vault.ts index fdb3059..02d014c 100644 --- a/src/formatters/vault.ts +++ b/src/formatters/vault.ts @@ -150,13 +150,15 @@ export interface FileReadFragment { } export interface FileReadResponse { - path: string; + path?: string; content: string | FileReadFragment[]; metadata?: { - size: number; - modified: number; + size?: number; + modified?: number; created?: number; - extension: string; + extension?: string; + totalLines?: number; + bytes?: number; }; frontmatter?: Record; tags?: string[]; @@ -166,22 +168,41 @@ export interface FileReadResponse { strategy: string; query?: string; }; + pagination?: { + paginated: boolean; + page: number; + pageLineStart: number; + pageLineEnd: number; + totalLines: number; + bytes: number; + hasMore: boolean; + nextPage: string | null; + oversizedLine?: boolean; + beyondEnd?: boolean; + }; + warning?: string; } export function formatFileRead(response: FileReadResponse): string { - const { path, content, metadata, frontmatter, tags, fragmentMetadata } = response; + const { path, content, metadata, frontmatter, tags, fragmentMetadata, pagination } = response; const lines: string[] = []; - const fileName = path.split('/').pop() || path; + const safePath = path || 'file'; + const fileName = safePath.split('/').pop() || safePath; lines.push(header(1, `File: ${fileName}`)); lines.push(''); // Metadata summary - lines.push(property('Path', path, 0)); - if (metadata) { + lines.push(property('Path', safePath, 0)); + if (metadata && typeof metadata.size === 'number') { lines.push(property('Size', formatFileSize(metadata.size), 0)); + } + if (metadata && typeof metadata.modified === 'number') { lines.push(property('Modified', formatDate(metadata.modified), 0)); } + if (metadata && typeof metadata.totalLines === 'number') { + lines.push(property('Lines', String(metadata.totalLines), 0)); + } // Tags if (tags && tags.length > 0) { @@ -253,6 +274,24 @@ export function formatFileRead(response: FileReadResponse): string { lines.push('```'); } + if (pagination && pagination.paginated) { + lines.push(''); + lines.push(header(2, 'Pagination')); + lines.push(property('Page', `${pagination.page} (lines ${pagination.pageLineStart}-${pagination.pageLineEnd} of ${pagination.totalLines})`, 0)); + if (pagination.hasMore && pagination.nextPage) { + lines.push(property('Next', pagination.nextPage, 0)); + } + if (pagination.beyondEnd) { + lines.push(' (requested page is past end of file)'); + } + lines.push(' returnFullFile=true for the whole file · query/strategy/maxFragments for fragments · line numbers are absolute (edit.at_line works)'); + } + + if (response.warning) { + lines.push(''); + lines.push(`> ${response.warning}`); + } + lines.push(divider()); lines.push(tip('Use `view.file(path)` for full content or `view.window(path, lineNumber)` for a section')); lines.push(summaryFooter()); diff --git a/src/semantic/operations/vault.ts b/src/semantic/operations/vault.ts index 63b98a8..fa0d168 100644 --- a/src/semantic/operations/vault.ts +++ b/src/semantic/operations/vault.ts @@ -46,6 +46,7 @@ export async function executeVaultOperation(ctx: RouterContext, action: string, return await readFileWithFragments(ctx.api, ctx.fragmentRetriever, { path, returnFullFile: paramBool(params, 'returnFullFile'), + page: paramNum(params, 'page'), query: paramStr(params, 'query'), strategy, maxFragments: paramNum(params, 'maxFragments') diff --git a/src/tools/semantic-tools.ts b/src/tools/semantic-tools.ts index 0513f2e..e446b32 100644 --- a/src/tools/semantic-tools.ts +++ b/src/tools/semantic-tools.ts @@ -433,7 +433,7 @@ function getParametersForOperation(operation: string): Record { }, page: { type: 'number', - description: 'Page number for paginated results' + description: 'Page number for paginated results (list/search; also large-file read — see returnFullFile)' }, pageSize: { type: 'number', @@ -450,7 +450,7 @@ function getParametersForOperation(operation: string): Record { }, returnFullFile: { type: 'boolean', - description: 'Return full file instead of fragments (WARNING: large files can consume significant context)' + description: 'read: force the ENTIRE file verbatim regardless of size (explicit large-context override). Default read already returns the whole file verbatim when it fits the size budget; large files return a verbatim page 1 with absolute line bookends (use page=N to continue, or query/strategy/maxFragments for fragments).' }, includeContent: { type: 'boolean', diff --git a/src/utils/file-reader.ts b/src/utils/file-reader.ts index e1660b5..01aa35f 100644 --- a/src/utils/file-reader.ts +++ b/src/utils/file-reader.ts @@ -2,18 +2,47 @@ import { ObsidianAPI } from './obsidian-api'; import { isImageFile } from '../types/obsidian'; import { UniversalFragmentRetriever } from '../indexing/fragment-retriever'; +/** + * Character budget that decides whole-file vs. paginated reads (ADR-203). + * + * Size-based on purpose: a line count is an invalid proxy for context cost + * (1500 short lines vs 1500 long lines differ by orders of magnitude). Only + * the *bookends* are line-based, so `edit.at_line` keeps working on a large + * file. ~50k chars ≈ ~12k tokens — generous enough that the overwhelming + * majority of notes return whole in one load. + */ +export const READ_PAGE_CHARS = 50000; + interface FileReadOptions { path: string; + /** Explicit whole-large-file override (ADR-203): full verbatim regardless of size. */ returnFullFile?: boolean; + /** Sequential page (1-based) for large files that exceed READ_PAGE_CHARS. */ + page?: number; query?: string; strategy?: 'auto' | 'adaptive' | 'proximity' | 'semantic'; maxFragments?: number; } interface FileReadResult { + path?: string; content?: unknown; metadata?: unknown; + frontmatter?: unknown; + tags?: unknown; originalContentLength?: number; + pagination?: { + paginated: boolean; + page: number; + pageLineStart: number; + pageLineEnd: number; + totalLines: number; + bytes: number; + hasMore: boolean; + nextPage: string | null; + oversizedLine?: boolean; + beyondEnd?: boolean; + }; fragmentMetadata?: { totalFragments: number; strategy: string; @@ -28,82 +57,211 @@ interface FileReadResult { } /** - * Shared file reading logic with fragment support - * Used by both classic tools and semantic operations + * Build one page: the longest run of whole lines (starting at `startIdx`, + * 0-based) whose joined size stays within READ_PAGE_CHARS. A single line + * larger than the budget is returned whole as its own page (never split). + */ +function buildPage(lines: string[], startIdx: number): { + text: string; + lineStart: number; + lineEnd: number; + nextIdx: number; + oversizedLine: boolean; +} { + const parts: string[] = []; + let size = 0; + let i = startIdx; + let oversizedLine = false; + while (i < lines.length) { + const ln = lines[i]; + const candidate = parts.length === 0 ? ln.length : size + 1 + ln.length; + if (candidate > READ_PAGE_CHARS && parts.length > 0) break; + if (candidate > READ_PAGE_CHARS && parts.length === 0) oversizedLine = true; + parts.push(ln); + size = candidate; + i++; + if (oversizedLine) break; + } + return { + text: parts.join('\n'), + lineStart: startIdx + 1, + lineEnd: i, // 1-based inclusive end == count of lines consumed + nextIdx: i, + oversizedLine, + }; +} + +/** + * Shared file reading logic (ADR-203). + * + * Faithful by default, never context-breaking: + * - fragment params (query/strategy/maxFragments) → semantic fragments + * - returnFullFile:true → entire file verbatim (explicit large override) + * - fits READ_PAGE_CHARS → entire file verbatim, one load (common case) + * - exceeds budget → bookended page 1 (or `page` N): one contiguous + * verbatim block + line bookends so edit.at_line still works + * + * Content is byte-exact in every branch (never flattened); the structured + * envelope no longer double-encodes the body. */ export async function readFileWithFragments( api: ObsidianAPI, fragmentRetriever: UniversalFragmentRetriever, options: FileReadOptions ): Promise { - const { path, returnFullFile, query, strategy, maxFragments } = options; - - // Get the file + const { path, returnFullFile, page, query, strategy, maxFragments } = options; + const fileResponse = await api.getFile(path); - - // Check if it's an image file + + // Image / binary: passthrough unchanged if (isImageFile(fileResponse)) { return fileResponse as FileReadResult; } - - // Extract content from the response + + // Extract verbatim content + metadata (metadata WITHOUT a copy of the body) let fileContent: string; - let metadata: Record = {}; - + let metaNoBody: Record = {}; + let frontmatter: unknown; + let tags: unknown; + if (typeof fileResponse === 'string') { fileContent = fileResponse; } else if (fileResponse && typeof fileResponse === 'object' && 'content' in fileResponse) { - // Handle structured response from Obsidian API - fileContent = fileResponse.content; - metadata = { ...fileResponse }; - - // If it's still not a string (might be an image or binary file) - if (typeof fileContent !== 'string') { - return fileResponse as FileReadResult; + const fc = (fileResponse as { content: unknown }).content; + if (typeof fc !== 'string') { + return fileResponse as FileReadResult; // image/binary structured } + fileContent = fc; + // Strip the body so it is not embedded twice in the envelope (ADR-203 §3) + const { content: _body, frontmatter: fm, tags: tg, ...rest } = + fileResponse as unknown as Record; + void _body; + metaNoBody = rest; + frontmatter = fm; + tags = tg; } else { - // Handle other non-text files return fileResponse as FileReadResult; } - - // Return full file if requested - if (returnFullFile) { - const wordCount = fileContent.split(/\s+/).length; - + + const totalChars = fileContent.length; + const lines = fileContent.split('\n'); + const totalLines = lines.length; + + // 1. Explicit fragment retrieval (unchanged behaviour) + const wantsFragments = + query !== undefined || strategy !== undefined || maxFragments !== undefined; + if (wantsFragments) { + const docId = `file:${path}`; + fragmentRetriever.indexDocument(docId, path, fileContent); + const fragmentQuery = query || path.split('/').pop()?.replace('.md', '') || ''; + const fragmentResponse = fragmentRetriever.retrieveFragments(fragmentQuery, { + strategy: strategy || 'auto', + maxFragments: maxFragments || 5, + }); return { - content: fileResponse, - metadata: { - ...metadata, - wordCount, - warning: wordCount > 2000 ? - `This file contains ${wordCount} words. Consider using fragment retrieval (remove returnFullFile parameter) to reduce context consumption.` : - null - } + path, + ...metaNoBody, + frontmatter, + tags, + content: fragmentResponse.result, + originalContentLength: totalChars, + fragmentMetadata: { + totalFragments: fragmentResponse.result.length, + strategy: strategy || 'auto', + query: fragmentQuery, + }, + workflow: fragmentResponse.workflow, + efficiency_hints: fragmentResponse.efficiency_hints, }; } - - // Use fragment retrieval - const docId = `file:${path}`; - fragmentRetriever.indexDocument(docId, path, fileContent); - - // Retrieve relevant fragments based on query or path - const fragmentQuery = query || path.split('/').pop()?.replace('.md', '') || ''; - const fragmentResponse = fragmentRetriever.retrieveFragments(fragmentQuery, { - strategy: strategy || 'auto', - maxFragments: maxFragments || 5 - }); - - // Return structured response with fragments + + // 2. Whole file, one load — fits the budget OR explicit override + if (returnFullFile || totalChars <= READ_PAGE_CHARS) { + const overrideOnLarge = !!returnFullFile && totalChars > READ_PAGE_CHARS; + return { + path, + content: fileContent, // verbatim, single contiguous string + frontmatter, + tags, + metadata: { + ...metaNoBody, + totalLines, + bytes: totalChars, + }, + pagination: { + paginated: false, + page: 1, + pageLineStart: 1, + pageLineEnd: totalLines, + totalLines, + bytes: totalChars, + hasMore: false, + nextPage: null, + }, + warning: overrideOnLarge + ? `Returned entire large file verbatim (${totalLines} lines, ${totalChars} bytes) via returnFullFile override.` + : undefined, + }; + } + + // 3. Large file, no override, no fragments → bookended page + const requested = typeof page === 'number' && page >= 1 ? Math.floor(page) : 1; + let idx = 0; + let cur = 1; + let built = buildPage(lines, idx); + while (cur < requested && built.nextIdx < lines.length) { + idx = built.nextIdx; + cur++; + built = buildPage(lines, idx); + } + + // Requested a page past EOF + if (cur < requested) { + return { + path, + content: '', + frontmatter, + tags, + metadata: { ...metaNoBody, totalLines, bytes: totalChars }, + pagination: { + paginated: true, + page: requested, + pageLineStart: totalLines + 1, + pageLineEnd: totalLines, + totalLines, + bytes: totalChars, + hasMore: false, + nextPage: null, + beyondEnd: true, + }, + warning: `Requested page ${requested} is past end of file (file has ${totalLines} lines, last page is ${cur}).`, + }; + } + + const hasMore = built.nextIdx < lines.length; + const nextPageNum = cur + 1; return { - ...metadata, - content: fragmentResponse.result, - originalContentLength: fileContent.length, - fragmentMetadata: { - totalFragments: fragmentResponse.result.length, - strategy: strategy || 'auto', - query: fragmentQuery + path, + content: built.text, // contiguous verbatim block for this line range + frontmatter, + tags, + metadata: { ...metaNoBody, totalLines, bytes: totalChars }, + pagination: { + paginated: true, + page: cur, + pageLineStart: built.lineStart, + pageLineEnd: built.lineEnd, + totalLines, + bytes: totalChars, + hasMore, + nextPage: hasMore ? `vault.read(path='${path}', page=${nextPageNum})` : null, + oversizedLine: built.oversizedLine || undefined, }, - workflow: fragmentResponse.workflow, - efficiency_hints: fragmentResponse.efficiency_hints + warning: + `Large file (${totalLines} lines, ${totalChars} bytes). Returned page ${cur} ` + + `(lines ${built.lineStart}-${built.lineEnd}, verbatim). ` + + (hasMore ? `Use page=${nextPageNum} for more, ` : '') + + `returnFullFile=true for the whole file, or query/strategy/maxFragments for fragments. ` + + `Line numbers are absolute — edit.at_line works on this page.`, }; -} \ No newline at end of file +} diff --git a/tests/vault-read-fidelity.test.ts b/tests/vault-read-fidelity.test.ts new file mode 100644 index 0000000..bc3ceb5 --- /dev/null +++ b/tests/vault-read-fidelity.test.ts @@ -0,0 +1,123 @@ +/** + * ADR-203 — faithful-by-default content reads with char-budget pagination + * and line bookends. Covers #133's intent + the large-raw guard. + */ +import { readFileWithFragments, READ_PAGE_CHARS } from '../src/utils/file-reader'; +import { formatFileRead } from '../src/formatters/vault'; +import { UniversalFragmentRetriever } from '../src/indexing/fragment-retriever'; +import { ObsidianAPI } from '../src/utils/obsidian-api'; +import { App } from 'obsidian'; + +class MockAPI extends ObsidianAPI { + files = new Map(); + constructor() { super({} as App); } + async getFile(path: string): Promise { + const content = this.files.get(path); + if (content === undefined) throw new Error(`not found: ${path}`); + return { path, content, tags: ['#demo'], frontmatter: { title: 'T' } }; + } +} + +const fr = () => new UniversalFragmentRetriever(); + +// A whitespace/structure-sensitive small file — the #133 fidelity case. +const TRICKY = '---\ntitle: T\n---\n\n# H\n\npara **b**\n\n```python\ndef f(x):\n return x*2 # indented\n```\n\ttab-line \n'; + +describe('vault.read fidelity & pagination (ADR-203)', () => { + test('small file: whole verbatim source, byte-exact, not paginated, body not duplicated', async () => { + const api = new MockAPI(); + api.files.set('s.md', TRICKY); + const r: any = await readFileWithFragments(api, fr(), { path: 's.md' }); + + expect(typeof r.content).toBe('string'); + expect(r.content).toBe(TRICKY); // exact bytes, no flatten + expect(r.pagination.paginated).toBe(false); + expect(r.pagination.totalLines).toBe(TRICKY.split('\n').length); + // metadata must NOT carry a second copy of the body (ADR-203 §3) + expect(JSON.stringify(r.metadata)).not.toContain('def f(x)'); + }); + + test('round-trip: a substring taken from the read matches the file for edit.window', async () => { + const api = new MockAPI(); + api.files.set('s.md', TRICKY); + const r: any = await readFileWithFragments(api, fr(), { path: 's.md' }); + // The exact indented code line an editing agent would target: + expect(r.content).toContain(' return x*2 # indented'); + expect(r.content.split('\n')).toContain('\ttab-line '); + }); + + const big = Array.from({ length: 4000 }, (_, i) => `line ${i + 1} ${'x'.repeat(20)}`).join('\n'); + + test('large file: default returns bookended page 1, not the whole dump', async () => { + const api = new MockAPI(); + api.files.set('big.md', big); + const r: any = await readFileWithFragments(api, fr(), { path: 'big.md' }); + + expect(r.pagination.paginated).toBe(true); + expect(r.pagination.page).toBe(1); + expect(r.pagination.pageLineStart).toBe(1); + expect(r.pagination.pageLineEnd).toBeLessThan(r.pagination.totalLines); + expect(r.pagination.hasMore).toBe(true); + expect(r.pagination.nextPage).toContain('page=2'); + // page content is bounded by the char budget (the agent-safety invariant) + expect((r.content as string).length).toBeLessThanOrEqual(READ_PAGE_CHARS); + // ...and is a verbatim contiguous prefix (line 1 present, exact) + expect((r.content as string).split('\n')[0]).toBe('line 1 ' + 'x'.repeat(20)); + }); + + test('large file: page 2 continues contiguously from page 1 (absolute line numbers)', async () => { + const api = new MockAPI(); + api.files.set('big.md', big); + const p1: any = await readFileWithFragments(api, fr(), { path: 'big.md', page: 1 }); + const p2: any = await readFileWithFragments(api, fr(), { path: 'big.md', page: 2 }); + + expect(p2.pagination.pageLineStart).toBe(p1.pagination.pageLineEnd + 1); + const firstLineOfP2 = `line ${p2.pagination.pageLineStart} ${'x'.repeat(20)}`; + expect((p2.content as string).split('\n')[0]).toBe(firstLineOfP2); + }); + + test('large file: returnFullFile=true overrides to the entire verbatim file', async () => { + const api = new MockAPI(); + api.files.set('big.md', big); + const r: any = await readFileWithFragments(api, fr(), { path: 'big.md', returnFullFile: true }); + expect(r.content).toBe(big); + expect(r.pagination.paginated).toBe(false); + expect(r.warning).toMatch(/returnFullFile override/i); + }); + + test('page past EOF is reported, not an error', async () => { + const api = new MockAPI(); + api.files.set('big.md', big); + const r: any = await readFileWithFragments(api, fr(), { path: 'big.md', page: 9999 }); + expect(r.pagination.beyondEnd).toBe(true); + expect(r.content).toBe(''); + expect(r.warning).toMatch(/past end of file/i); + }); + + test('fragment params still route to semantic fragments (unchanged)', async () => { + const api = new MockAPI(); + api.files.set('big.md', big); + const r: any = await readFileWithFragments(api, fr(), { path: 'big.md', maxFragments: 3 }); + expect(Array.isArray(r.content)).toBe(true); + expect(r.fragmentMetadata).toBeDefined(); + }); + + test('formatter renders all shapes without the _Formatter error_ crash', () => { + // whole-file string shape + expect(() => formatFileRead({ + path: 's.md', content: TRICKY, + metadata: { totalLines: 14, bytes: TRICKY.length }, + pagination: { paginated: false, page: 1, pageLineStart: 1, pageLineEnd: 14, totalLines: 14, bytes: TRICKY.length, hasMore: false, nextPage: null }, + } as any)).not.toThrow(); + // paginated shape + const out = formatFileRead({ + path: 'big.md', content: 'line 1 ...\nline 2 ...', + metadata: { totalLines: 4000, bytes: 90000 }, + pagination: { paginated: true, page: 1, pageLineStart: 1, pageLineEnd: 1800, totalLines: 4000, bytes: 90000, hasMore: true, nextPage: "vault.read(path='big.md', page=2)" }, + warning: 'Large file …', + } as any); + expect(out).toContain('Pagination'); + expect(out).toContain('page=2'); + expect(out).not.toContain('Formatter error'); + }); +});