[kantord]

Summary

• Studio's skill page can only show a skill's name and description, because the client API stops at ListSkills, although the daemon already reads a skill's body and bundled assets for the Skill tool.
• Adds two read-only RPCs, ListSkillFiles and ReadSkillFile, with HTTP routes GET /v1/skills/files?name= and GET /v1/skills/files/read?name=&file=, TypeScript SDK methods skills.listFiles and skills.readFile, four stable error codes, a skill_files compatibility feature, and the docs.
• A listing is SKILL.md (the instruction body, without its frontmatter, marked instructions: true) first, then the bundled assets sorted by logical name. Reads go through the caller's own skill view, the same one ListSkills and the Skill tool use, so a caller's learned skills resolve as body-only.
• Reads return text only. A file over 25,000 bytes (the Skill tool's output cap), or one that is not text (invalid UTF-8, or containing a NUL byte, as the Skill tool also refuses), is refused with a stable error code (HTTP 422), never truncated or repaired.
• The Studio page that uses this is feat(studio): show a skill's files on its page #2155. Studio can only use the published SDK, so feat(studio): show a skill's files on its page #2155 cannot merge until an SDK release includes this change. That is why this is a separate PR.

Issue relationship

Relates to #1853

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

Baseline checks

• Linting (task lint)
• Fast offline test suite (task test): not run separately; task test:race runs the same suites
• Pre-review race suite (task test:race), see the note below
• Offline demo (go run ./cmd/mecademo): tool call, permission ask and approval, result
• Markdown changed: docs generation/link checks (task docs)
• User docs/user-facing behavior changed: site build (task site:build): not run
• Guarded engine API affected: compatibility check (task api:check)
• Intentional engine API change: task api:update + engine/CHANGELOG.md: not needed; engine/adapter/skillfs is a reference adapter excluded from the guarded engine API (engine/COMPATIBILITY.md)

task test:race passed on the final commit except four mcp/oauthlogin tests (TestExactRedirect*). They need the fixed port 18473, which a local Studio dev server was holding; that package is untouched by this PR, and it passes when the port is free (rerun in an isolated network namespace). Everything else passed: 190 packages ok, no failures, no data races.

What was tested, and what that does and does not show

Because this PR must be released before the Studio PR can consume it, the evidence below is aimed at one question: will the Studio PR work against the SDK that this PR releases?

Unit and integration tests (in this PR)

• gRPC and HTTP tests with a fake skill source: listing order, sizes and the instructions marker, unknown skill, reading SKILL.md and an asset, every refusal (invalid name, missing file, oversize, non-text including a NUL byte), the exact 25,000-byte boundary, body limits (oversize, NUL, empty), the HTTP status and the stable error code for each refusal, and that every read goes through the publication preface.
• Real catalog (engine/adapter/skillfs): SourceForPartitions serves the caller's own view, shows a learned skill as body-only, and returns not-found for another caller's learned skill.
• Real daemon wiring (internal/app): a built service lists and reads only the caller's own learned skill (another caller gets ErrSkillNotFound), and with no learned-skill store the deployment's skills still list and read, with an explicit-store positive control.
• I checked that the new tests fail against the bugs they guard: ignoring the caller's partition, returning no source when there is no learned-skill store, > changed to >= at the cap, the NUL check removed, and the old 413 mapping.
• The full TypeScript SDK suite (378 tests), lint, and the API snapshot check.

Review findings, reproduced before they were fixed. A review of the first version found two bugs. Both were reproduced end to end with a real mecated, the real Studio server, and the Studio page in a browser, then fixed and re-checked the same way.

• With no learned-skill store (no HOME or XDG directory), a skill appeared in the inventory but its page's Files section showed skill "plain-demo": tool: skill not found. It now lists and reads.
• An asset containing a NUL byte but valid UTF-8 was served as text. It is now refused, and the page shows "This is a binary file, so it can't be shown here."

Against the SDK as it will be published (done locally)

• Built the package with pnpm pack, the same step the release workflow's prepack runs, and installed that tarball into Studio in place of the published 0.4.0.
• Studio's lint, typecheck, generated-contract check, and all tests passed against that tarball (201 server and 779 web tests).
• Ran the real Studio server (BFF) against a real mecated built from this branch, with the tarball SDK, using real public skills plus synthetic skills with a binary file, an oversize file, a NUL file, and a 40-file skill. Observed: six files in order with sizes for a real skill; refused files shown with a reason while the readable files in the same skill still loaded; 32 files shown with 8 omitted for the 40-file skill; an unknown skill returned 404 skill_not_found; GetCompatibilityInfo advertises skill_files.
• Called both SDK methods from the tarball over the SDK's HTTP transport against the real daemon (an earlier build of this branch), including the refusal codes and the renamed /v1/skills/files/read route. The gRPC path was exercised with an in-repo SDK build, not the tarball.

What this does not show

• The tarball is a local build of this branch, not the artifact from the release workflow. It is the same source and the same build step, so I rate the remaining risk low, but only the real release proves it. The new version will also need an entry in Studio's minimumReleaseAgeExclude before it installs; that belongs to feat(studio): show a skill's files on its page #2155.
• The learned-skill path is tested in-process against the real catalog and wiring, not through a live daemon with a real learning pipeline.
• Large skills (hundreds of files) and concurrent publication of learned skills while files are read are not tested. The filesystem source rejects a skill with more than 1,024 assets, and reads share ListSkills' publication lock, but there is no load test. The gRPC-driver source's own bound was not checked.

Changes

File	Change
contracts/proto/mecatl/v1/harness.proto, contracts/gen/**	ListSkillFiles, ReadSkillFile, their messages, and SkillFileInfo.instructions
internal/adapter/server/skill_files.go	The service methods, size cap, error sentinels, and the design comments
internal/adapter/server/{grpc,http}.go	gRPC handlers and the two HTTP routes
internal/adapter/server/{errorcodes,classification,features}.go	Four stable error codes, the access classification (KindCallerOwned), and the skill_files feature
internal/adapter/server/service.go, internal/app/build.go	A SkillSourceFor seam wired to the caller's live skill view (the static source when there is no learned-skill store); ListSkills shares its publication preface
engine/adapter/skillfs/{atomic,fssource}.go	SourceForPartitions, a read-only tool.SkillSource over one caller's view; asset read errors no longer carry an absolute path
sdk/typescript/**	skills.listFiles and skills.readFile, the RPC catalog rows, error codes, API snapshots, and tests
user-docs/reference/**	gRPC and HTTP API references, regenerated gRPC schema and SDK reference

User-facing change

None directly. Clients of the daemon API and the TypeScript SDK gain two read-only methods; Studio uses them in #2155.

Special notes for reviewers

These choices are permanent once released, so they are worth a look. Each is recorded as a DECISION: in skill_files.go with the rejected alternative.

• Shape. A skill's body is modelled as a file named SKILL.md next to its assets, so a client lists and shows one uniform set of files. The engine's own vocabulary is "body" and "assets" with no file concept (ADR 0108); the alternative is a GetSkill that returns the body plus the asset list and a ReadSkillAsset, which makes every client special-case the body.
• Access class. KindCallerOwned, like ListLearnedSkills and GetLearnedSkill: the boundary resolves the skill through the caller's own view and treats a skill outside it as absent.
• Feature identifier. skill_files announces the pair, as features.go requires for every additive RPC. Against a daemon without it the routes answer a generic 404 that looks like skill_not_found. The SDK only announces it.
• Marker, not name. SkillFileInfo.instructions marks the body; clients should not key off the name SKILL.md.
• HTTP shape. GET /v1/skills/{name}/files would be natural but panics at registration, since ServeMux finds it ambiguous with GET /v1/skills/learned/{id}. The routes use a literal files segment and query parameters, like /v1/mcp/resources/read.
• SKILL.md is the body only (the text after the frontmatter, not the raw file). tool.SkillSource has no accessor for the raw file, so the frontmatter is not included and the listed size is the body's size.
• Oversize is an error, not a truncated success, so a client cannot mistake a partial file for a whole one. It and non-text both answer 422 / FailedPrecondition with different stable codes; 413 is not used because it already means an oversized request body here.
• ListSkills' classification text now notes that the caller's learned skills are added to its view. Its kind (KindSharedInfrastructure) is left to a separate change.

Fully or partially written by an AI agent.

🤖 Generated with Claude Code

[peppescg]

I ran a panel review on 962ee593 covering spec, standards, test adequacy, security, architecture, reuse and duplication. Path resolution, caller isolation, read limits and dependency direction are all solid, and I found no security blockers. But a few points should be settled before the SDK release. Once #2155 consumes this contract, it can't change.

Please fix before merge

1. Skills are listed but cannot be opened when there is no learned-skill store. In internal/app/build.go:2566, SkillSourceFor returns nil when assets.liveSkills == nil. LiveSkills falls back to the static snapshot in the same case.
   • liveSkills is only built when LearningStoreURL is set or UserModelDir resolves. A container with no HOME/XDG directory has neither; mecak8s with the default empty learning-store endpoint is one example.
   • There, ListSkills shows the deployment skills, while ListSkillFiles/ReadSkillFile return skill_not_found for every one of them.
   • Fix: return assets.skillSource, which mirrors registerSkillFamily and LiveSkills. Please add a composition test with no learned-skill store; the current one always sets UserModelDir.
2. NUL bytes are not refused. The Skill tool's readAsset (engine/adapter/skillfs/tool.go) rejects data containing NUL as "not textual". ReadSkillFile only checks utf8.Valid, so a binary that happens to be valid UTF-8 is served as text. That contradicts the SPEC in skill_files.go ("a non-text file is refused"). Fix: || bytes.IndexByte(data, 0) >= 0 on the ErrSkillFileNotText check, plus a fixture for it.
3. HTTP 413 for skill_file_too_large. 413 means the request was too large, and the status fallback in errorcodes.go already maps 413 to request_too_large/InvalidArgument. A client or proxy that branches on status would misread it. Suggest 422, with FailedPrecondition on gRPC. Once released, the status is permanent.
4. The SKILL.md wire name. It permanently names content that is the body without frontmatter. A client offering "download/copy file" would hand out a SKILL.md that discovery rejects, and if the raw file is ever exposed, that name is already taken. Cheap and additive before release: a role field on SkillFileInfo (for example bool instructions, or a small enum) so clients key off it rather than the string, and state the body-only meaning in the proto comment, not only in Go.
5. No feature identifier. features.go says every additive change, including a new RPC, is announced through features, and that clients never gate on version. The published SDK runs against arbitrary daemons. Against an older one, GET /v1/skills/files returns a generic 404 that is easy to confuse with skill_not_found. Either add skill_files, or record the exception explicitly. I know ListSkills and the learned-skill RPCs set no precedent here either.
6. The 25,000 cap is guarded by the wrong comparison. TestSkillFileCapMatchesSkillToolCap compares skillfs.MaxOutputBytes to the literal 25_000, not to maxSkillFileBytes. Lowering the server constant goes unnoticed. The stated reason for the copy also doesn't hold: internal/adapter/server already imports engine/adapter/{memledger,nofs,sessnap,cronparse}, and no depguard rule forbids skillfs. Suggest maxSkillFileBytes = skillfs.MaxOutputBytes and dropping the test.

Tests

• No boundary test on the cap. Changing both > maxSkillFileBytes checks to >= still passes the suite. Please add a 25,000-byte asset that must read successfully next to the 25,001-byte one.
• The four error-code strings are never asserted. The tests check HTTP and gRPC statuses only. skill_not_found and skill_file_not_found share NotFound/404, so the missing-file case would pass with the wrong code.
• SKILL.md refusals are untested. Nothing covers an oversize or non-UTF-8 body, or an empty or frontmatter-only body.
• Bob's isolation checks only assert err != nil. In skill_files_composition_test.go they should assert errors.Is(err, tool.ErrSkillNotFound), so an unrelated failure can't pass as isolation.
• Nothing asserts the new RPCs go through the publication preface (beginSkillRead).

Nits

• The 1,024 truncation silently drops one asset. FSSource and the grpcdriver source already enforce the bound and reject an over-limit inventory outright. The server's maxSkillFileEntries-1 cut then drops the last asset of a skill with exactly 1,024, with no signal. Suggest deleting the constant and the truncation.
• A physical path can leak (CWE-209). If a skill directory disappears after discovery, the os.OpenRoot error in fssource.go includes the absolute path, and it reaches the client through err.Error(). Map it to a fixed message.
• The // DECISION: block in skillfs/atomic.go is misplaced. It sits above NewLiveToolForPartitions, so it becomes that function's godoc, but it describes SourceForPartitions.
• ListSkills keeps a stale rationale. It stays KindSharedInfrastructure with "identical for every caller", which this PR's own DECISION says no longer holds.
• Correction to the PR description: the engine API exemption applies because skillfs is an excluded reference adapter in COMPATIBILITY.md, not because only a method was added.

[kantord]

Thanks @peppescg, this was a useful review. Everything below is in 18aeda376; I checked each claim before acting on it.

Please fix before merge

1. No learned-skill store. Confirmed, and reproduced end to end: with no HOME/XDG directory, ListSkills showed the skill and the page's Files section said skill "plain-demo": tool: skill not found. SkillSourceFor now falls back to assets.skillSource. The new composition test runs with no store and with an explicit store as a positive control; it fails against the old behavior.
2. NUL bytes. Confirmed, also reproduced in the browser: a valid-UTF-8 file with NULs was shown as text. Now refused as skill_file_not_text, with a fixture.
3. 413. Agreed. skill_file_too_large is now 422 / FailedPrecondition (same status as not-text, different stable code). Tests assert status and code for every refusal.
4. SKILL.md name. Added SkillFileInfo.instructions and documented in the proto that the body is the text after the frontmatter. I did not verify your point that discovery rejects a body-only SKILL.md; I took the marker because it is cheap and additive before release.
5. Feature identifier. Added skill_files (features.go, ServerFeature.SkillFiles in the SDK, API snapshots). The SDK only announces it; it does not gate calls on it.
6. Cap. maxSkillFileBytes = skillfs.MaxOutputBytes, and the comparison test is gone. You were right that nothing forbids the import.

Tests. Added the exact 25,000-byte boundary (changing > to >= now fails), the four error-code strings over HTTP, SKILL.md refusals (oversize, NUL, empty body), errors.Is(err, tool.ErrSkillNotFound) for Bob, and a test that reads go through the publication preface. I mutation-checked each new test.

Nits. Removed the 1,023-asset truncation (I confirmed FSSource rejects over 1,024; I did not check the gRPC-driver source). Asset read errors no longer carry absolute paths. The DECISION: comment is back above SourceForPartitions. The PR description now says why the engine API check does not apply.

One thing left as is. ListSkills stays KindSharedInfrastructure; I only corrected its rationale text to note that the caller's learned skills are added. Changing its kind seems like a separate change.

Evidence. Full task test:race is clean on this commit, and Studio (#2155) passes lint, typecheck, and all tests against a freshly packed SDK tarball. The PR description lists what that does and does not show.

Fully or partially written by an AI agent.