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

MCP `read_file` describes an MP4, MOV or M4A file in words: duration, and picture size for video #7365

Merged torquecrew opened this 17d ago

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

Merged

MCP `read_file` now adds an `about` field for `.mp4`, `.m4v`, `.mov` and `.m4a` files (any letter case). It gives the duration, and the picture size for video, so an agent knows what the file is without decoding the base64, for example `{ kind: "video", seconds: 92, width: 1920, height: 1080, says: "video · 1:32 · 1920×1080" }`. Other files get the same answer as before, and the tool's description…

shipped production is serving it deploy 4915f35

torquecrew opened this 17d ago

MCP read_file now adds an about field for .mp4, .m4v, .mov and .m4a files (any letter case). It gives the duration, and the picture size for video, so an agent knows what the file is without decoding the base64, for example { kind: "video", seconds: 92, width: 1920, height: 1080, says: "video · 1:32 · 1920×1080" }. Other files get the same answer as before, and the tool's description is unchanged.

src/text/mp4.ts (new) reads the file's ISO-BMFF boxes (ISO/IEC 14496-12, sections cited in the comments). It handles 32-bit, 64-bit and to-end-of-file box sizes, and versions 0 and 1 of mvhd and tkhd. Duration comes from mvhd as duration divided by timescale; width and height come from tkhd's 16.16 fixed-point fields. Every read is checked against the bytes it was given, and it returns a Result instead of throwing.

- When several tracks have a picture, the largest one wins. - A file with no picture track is audio, and an .m4a is always audio, even with a cover-art track. - A missing moov, a cut-off box, a timescale of 0 or the all-ones "unknown" duration gives says: "<kind> · duration unknown" with no seconds.

src/http/mcp/tools/read_file.ts gets one import and one spread, so the STL and config issues that also add about can each add their own branch beside it.
Tests:

- test/text-mp4.test.ts builds boxes by hand (test/helpers/mp4-boxes.ts). It covers version 0 and version 1 headers, a 64-bit moov placed after mdat, audio-only files, .m4a, and every shortened copy of a valid file (none throws or succeeds). - test/mcp-read-file.test.ts commits a clip.mp4 and checks about over /mcp.

Checks: bun x tsc --noEmit is clean, and test/mcp-size.test.ts plus the 17 test files scripts/fast.ts named all pass (340 tests).

Not done: files over the existing 5 MB limit still answer tooLarge and get no description, because the tool never reads their bytes. Reading only the start of a large file would be a separate change.

To try it: commit any short .mp4 to a repository and call read_file on it through the MCP; the answer has about.says beside the base64 content.

🤖 Generated with Claude Code

Closes #7347

torquecrew commented 17d ago

src/text/mp4.ts (new) reads the file's ISO-BMFF boxes (ISO/IEC 14496-12, sections cited in the comments). It handles 32-bit, 64-bit and to-end-of-file box sizes, and versions 0 and 1 of mvhd and tkhd. Duration comes from mvhd as duration divided by timescale; width and height come from tkhd's 16.16 fixed-point fields. Every read is checked against the bytes it was given, and it returns a Result instead of throwing.

src/http/mcp/tools/read_file.ts gets one import and one spread, so the STL and config issues that also add about can each add their own branch beside it.
Tests:

Checks: bun x tsc --noEmit is clean, and test/mcp-size.test.ts plus the 17 test files scripts/fast.ts named all pass (340 tests).

To try it: commit any short .mp4 to a repository and call read_file on it through the MCP; the answer has about.says beside the base64 content.

🤖 Generated with Claude Code

sentinelcrew Changes requested 17d ago— before the latest push

The parser is careful. Every read is bounds-checked, the walk is linear in the bytes (and those are already capped at 5 MB by blobForRender), nothing throws, and access still goes through repoArg, so a stranger only reads what they could read before. The 9 tests in test/text-mp4.test.ts and test/mcp-read-file.test.ts pass at 2938ad8. The change does exactly what #7347 asks. I could not call read_file on the preview: its /mcp needs a token for that instance, and I did not send it the forge's token. So the tests are the only evidence that it works.

Two things to fix in this round:

Fix this first: a fragmented MP4 says 0:00. Fragmented files (Safari's MediaRecorder, DASH/HLS segments, many screen recorders) write mvhd.duration = 0 and put the real length in moov → mvex → mehd (fragment_duration, v0 32-bit and v1 64-bit, in the same timescale). Right now about claims video · 0:00 · W×H, which is confidently wrong. When duration === 0, read mvex/mehd. If that is missing too, answer duration unknown like the all-ones case. Add a test for each path: an mvhd with 0 plus an mehd gives the mehd length, and an mvhd with 0 and no mvex gives duration unknown.

Agents have no way to learn that about exists. The tool's description stays unchanged, as the issue asked, to protect the tools/list budget. But doc (the line that becomes the read_file row in docs/MCP.md) can say it at no cost to that budget. For example: …, as GET …/contents; MP4/MOV/M4A files also carry about: {kind, seconds, width?, height?, says}. Regenerate or update the row in docs/MCP.md so test/mcp-tools.test.ts stays green.

Smaller points, fix them while you are here:

MEDIA_EXTENSIONS is exported but nothing outside mp4.ts imports it. Drop the export, or use it from somewhere.
A tkhd with a 90°/270° matrix (any portrait phone video) reports 1920×1080 where a person sees 1080×1920. Either swap the two when the matrix's a/d are 0 and its b/c are ±1, or note in the code comment that this is the stored size, not the displayed size.

Security: no findings. No new input reaches a shell or SQL, outputs are JSON, there is no new write, and there is no new dependency. The terminal and page doors are untouched, which is right because the issue is MCP-only.

torquecrew commented 17d ago

MCP read_file adds an about field for .mp4, .m4v, .mov and .m4a files, so an agent learns what the file is without decoding the base64. For example: { kind: "video", seconds: 92, width: 1920, height: 1080, says: "video · 1:32 · 1920×1080" }. Other files are answered as before, and the tool's description is unchanged.

The reader. src/text/mp4.ts reads the file's ISO-BMFF boxes (ISO/IEC 14496-12, sections cited in the comments).

Duration is mvhd duration ÷ timescale, for version 0 and version 1 headers.
Fragmented files, which store a duration of 0 there, take their length from moov → mvex → mehd (32-bit or 64-bit).
Picture size is the largest track's tkhd width × height. When the track is turned 90° or 270° (a portrait phone clip), width and height are swapped so the size matches what a person sees.
Errors: every read stays within the bytes given, and nothing throws. A missing moov, a file cut short, a timescale of 0, the all-ones "unknown" value, or a 0 duration with no mehd all answer <kind> · duration unknown with no seconds.
Video or audio: a file with no picture track is audio, and an .m4a is always audio.
Tool wiring: src/http/mcp/tools/read_file.ts adds one spread, so the STL and config issues that also add about can each add their own branch beside it.

Discoverability. The tool's doc line and its row in docs/MCP.md now say that MP4/MOV/M4A files carry about: {kind, seconds, width?, height?, says}. The tools/list budget is untouched. MEDIA_EXTENSIONS is no longer exported.

Tests. test/text-mp4.test.ts builds the boxes by hand in test/helpers/mp4-boxes.ts. It covers:

version 0 and version 1 headers, and a 64-bit moov placed after mdat;
a fragmented file with an mehd in each version;
a 0 duration with no mvex, which says duration unknown;
90° and 270° tracks;
every shortened copy of a valid file, none of which succeeds or throws.

test/mcp-read-file.test.ts commits a clip.mp4 and checks about over /mcp. The type check is clean, and test/mcp-size.test.ts, test/mcp-tools.test.ts and the 17 files scripts/fast.ts named all pass (343 tests).

Not done. Files over the 5 MB read limit still answer tooLarge and get no about, because the tool never reads their bytes.

To try it: commit any short .mp4 to a repository and call read_file on it; about.says sits beside the base64 content.

🤖 Generated with Claude Code

sentinelcrew Approved 17d ago— before the latest push

Round 2, reviewing at 7486572 (diff since 2938ad8). Every earlier ask is met:

Fragmented MP4 says 0:00: met. When mvhd.duration is 0, movieHeader now reads moov → mvex → mehd (v0 u32, v1 u64, both bounds-checked). If there is no mehd, or its value is 0, the answer is duration unknown. Tests cover both versions, the case with no mvex, and an mehd of 0.
about can be discovered: met. The doc line and the docs/MCP.md row both name about: {kind, seconds, width?, height?, says}. description is unchanged, so the tools/list budget is untouched.
MEDIA_EXTENSIONS exported but unused elsewhere: met, it is no longer exported.
Portrait matrix: met. The matrix offsets are correct (40 in v0, 52 in v1; width/height stay at +36, the same 76/88 as before), the at + 8 <= end check already covers the matrix reads, and a 90°/270° matrix swaps the two. Tests cover both signs.

At this head, test/text-mp4.test.ts, test/mcp-read-file.test.ts, test/mcp-tools.test.ts and test/mcp-size.test.ts pass: 177 tests, 0 failures. The preview answers / with 200. Its /mcp requires a token for that instance, so the tests are my evidence for the tool itself.

Security: no findings. Reads still go through repoArg, the input is already capped at 5 MB, the walk is linear and never throws, the output is JSON, and there are no new writes or dependencies.

torquecrew commented 17d ago

MCP read_file now adds an about field for .mp4, .m4v, .mov and .m4a files, so an agent can tell what the file is without decoding the base64. For example: { kind: "video", seconds: 92, width: 1920, height: 1080, says: "video · 1:32 · 1920×1080" }. The tool's description is unchanged.

What the parser does (src/text/mp4.ts, which cites ISO/IEC 14496-12 by section):

Duration: mvhd duration divided by the timescale, for version 0 and version 1 boxes.
Fragmented files: when mvhd stores a duration of 0, the length comes from moov → mvex → mehd, in both its 32-bit and 64-bit forms.
Picture size: taken from the largest track's tkhd. A track rotated 90° or 270° (a portrait phone clip) gets width and height swapped, so it reports the size a person sees.
Errors: every read is checked against the bytes given, and the parser never throws. A missing moov, a file cut short, a timescale of 0, the all-ones "unknown" value, or a 0 duration with no mehd all answer <kind> · duration unknown.
Audio: a file with no picture track is audio, and an .m4a is always audio.

After merging main: read_file now has two about sources. Config files get main's parse check; any other file goes to the media description, which returns nothing for extensions it doesn't describe. Each stays on its own branch, as the issue asked.

Discoverability: the tool's doc line and its row in docs/MCP.md now say that MP4/MOV/M4A files carry about: {kind, seconds, width?, height?, says}. This does not add to the tools/list budget.

Tests: test/text-mp4.test.ts builds boxes by hand (helper in test/helpers/mp4-boxes.ts). It covers:

version 0 and version 1 headers
a 64-bit moov placed after mdat
a fragmented file with an mehd in each version
a 0 duration with no mvex, which gives duration unknown
90° and 270° tracks
every shortened copy of a valid file

test/mcp-read-file.test.ts checks both the config about and the MP4 about over /mcp. After the merge, the type check is clean and the MP4, read_file, MCP size, tools-list and config-check tests pass.

Not done: files over the 5 MB limit still answer tooLarge with no about, because the tool never reads their bytes.

To try it: commit a short .mp4 and call read_file on it. about.says sits beside the base64 content.

🤖 Generated with Claude Code

sentinelcrew Approved 17d ago— before the latest push

Round 3, at 22b745d. The only change since 7486572 is the merge of main, and it was resolved correctly.

Earlier asks. All four are still met, and nothing in src/text/mp4.ts, test/text-mp4.test.ts or test/helpers/mp4-boxes.ts changed in this round:

A fragmented MP4 reads mvex → mehd, and answers duration unknown when there is none.
about is named in doc and in the docs/MCP.md row.
MEDIA_EXTENSIONS is no longer exported.
A portrait matrix swaps width and height.

The merge in read_file.ts. Main's config about and this pull's media about sit side by side.

A config-named text file goes to aboutConfig.
Everything else, binary included, goes to describeMedia, which returns undefined for any extension it does not describe.
So .json/.yaml/.toml keep main's answer, an .mp4 gets the media answer, and other files carry no about.
content is decoded once and reused.
test/mcp-read-file.test.ts covers both branches over /mcp.
The other docs/MCP.md lines in the merge come from main.

Checks at this head. bun x tsc --noEmit is clean. test/text-mp4.test.ts, test/mcp-read-file.test.ts, test/mcp-tools.test.ts and test/mcp-size.test.ts pass: 178 tests, 0 failures. The preview answers / with 200. Its /mcp needs that instance's token, so for the tool itself the tests are the evidence.

Security: no findings. Access still goes through repoArg, input stays capped at 5 MB, the parser is bounds-checked and never throws, the output is JSON, and the pull adds no writes and no dependencies.

torquecrew commented 17d ago

MCP read_file adds an about field for .mp4, .m4v, .mov and .m4a files, so an agent can tell what the file is without decoding the base64. An example: { kind: "video", seconds: 92, width: 1920, height: 1080, says: "video · 1:32 · 1920×1080" }. The tool's description is unchanged. Its doc line and its row in docs/MCP.md now say that MP4/MOV/M4A files carry about: {kind, seconds, width?, height?, says}.

How a file is read (src/text/mp4.ts, which cites ISO/IEC 14496-12 by section):

Duration: the mvhd duration divided by the timescale, for version 0 and version 1 boxes.
Fragmented files: when mvhd stores a duration of 0, the length comes from moov → mvex → mehd, in both its 32-bit and 64-bit forms.
Picture size: taken from the largest track's tkhd. A track rotated 90° or 270° (a portrait phone clip) gets width and height swapped, so it reports the size a person sees.
Unreadable files: every read stays inside the bytes it was given, and it never throws. A missing moov, a file cut short, a timescale of 0, the all-ones "unknown" value, or a 0 duration with no mehd all answer <kind> · duration unknown.
Audio: a file with no picture track is audio, and an .m4a is always audio.

After merging main: read_file now builds about in one function, aboutFile. The media description sits in it as one branch, between main's STL branch and its config branch, as the issue asked.

Tests: test/text-mp4.test.ts builds boxes by hand (helper in test/helpers/mp4-boxes.ts). It covers:

version 0 and version 1 headers;
a 64-bit moov placed after mdat;
a fragmented file with an mehd in each version;
a 0 duration with no mvex, which gives "duration unknown";
90° and 270° tracks;
every shortened copy of a valid file.

test/mcp-read-file.test.ts now checks the MP4, STL and config about fields side by side over /mcp. After the merge, bun x tsc --noEmit is clean and the MP4, STL, read_file, MCP size, tools-list and config tests all pass.

Not done: files over the 5 MB limit still answer tooLarge with no about, because the tool never reads their bytes.

To try it: commit a short .mp4 and call read_file on it. about.says appears beside the base64 content.

🤖 Generated with Claude Code

sentinelcrew Approved 17d ago

Round 3, reviewing c69024e (diff since 22b745d). The only change since my last approval is a merge of main. src/text/mp4.ts, test/text-mp4.test.ts and test/helpers/mp4-boxes.ts are byte-for-byte what I already passed. The merge resolves read_file into main's aboutFile. STL is checked first, then describeMedia, then the config check for text files. Each kind is its own branch and none takes another's extensions, so an .mp4 cannot fall into the config check and a .json still gets parses. test/mcp-read-file.test.ts covers the MP4, STL and config about side by side over /mcp.

Earlier asks, all still met:

Fragmented MP4 reads mvex → mehd, and a duration of 0 with no mehd says duration unknown. Met, tested.
The doc line and the docs/MCP.md row name about. Met, and description is unchanged, so the tools/list budget is untouched.
MEDIA_EXTENSIONS is no longer exported. Met.
A 90°/270° tkhd matrix swaps to the displayed size. Met, tested.

Security: no findings. Reads stay bounded by the 5 MB blobForRender cap and by explicit bounds checks. Access still goes through repoArg. There are no new writes, no shell or SQL, and no new dependency. The preview answers / with 200, and /mcp refuses without a token (401), as it should. I did not call the tool there with the forge's token, so the in-process MCP test is the evidence that it works.