feat(vault.read): faithful-by-default reads with char-budget pagination (ADR-203, #133)

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).
This commit is contained in:
Aaron Bockelie 2026-05-19 00:30:31 -05:00
parent 150cf5c071
commit f3dac1847e
6 changed files with 389 additions and 65 deletions

View file

@ -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))

View file

@ -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<string, unknown>;
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());

View file

@ -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')

View file

@ -433,7 +433,7 @@ function getParametersForOperation(operation: string): Record<string, unknown> {
},
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<string, unknown> {
},
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',

View file

@ -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<FileReadResult> {
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<string, unknown> = {};
let metaNoBody: Record<string, unknown> = {};
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<string, unknown>;
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.`,
};
}
}

View file

@ -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<string, string>();
constructor() { super({} as App); }
async getFile(path: string): Promise<any> {
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');
});
});