Repository navigation
feat(studio): show a skill's files on its page - #2155
Conversation
37bba60 to
f47dbe8
Compare
f47dbe8 to
eacdc3d
Compare
eacdc3d to
4fb5501
Compare
4fb5501 to
846bc07
Compare
01922a6 to
d4961c1
Compare
9f582ba to
5d04990
Compare
98e97d8 to
71559a1
Compare
peppescg
left a comment
There was a problem hiding this comment.
Thanks Daniel. I reviewed this at 71559a1b2, together with the daemon side from #2192.
Before merge
SKILL.mdisn't the real file (apps/server/src/mecatl/knowledge.ts:240). The daemon lists the instruction body without its frontmatter under the nameSKILL.md, and says "clients key off [SkillFileInfo.instructions], not off the name" (internal/adapter/server/skill_files.go:67-70). The BFF dropsinstructions, so the page shows the stripped body asSKILL.md, with the body's size. Someone checkingallowed-toolsordescriptionwon't find them there. Carryinstructionsthrough the contract and label that entry as the instructions, without frontmatter. The description's "the skill's real files" needs the same caveat.- One page view is 1 + N daemon calls on one lock (
knowledge.ts:238-250). When a learned-skill store is configured, everyListSkillFiles/ReadSkillFilestarts withbeginSkillRead, which takes the caller's skill-publication lock and re-publishes learned skills (skill_files.go:119-133, 5s timeout). So up to 33 calls queue behind one lock, each re-publishing, and they compete with learned-skill activate and rollback. Any error that isn't one of the threeskill_file_*codes is rethrown, soPromise.allfails the whole panel: with--rate-limitset (off by default), a skill with more files than the burst gets a 429 instead of its files. The daemon deliberately rejected a single "body plus every asset" read (skill_files.go:33). Load file contents on demand, per file, or at least bound the concurrency, skip reads whose listed size is already over the 25,000-byte cap, and turn per-file failures intounavailableso one failure doesn't hide the rest. - Docs.
user-docs/operating/studio.md:114still says "Select a skill to see its description and slug". Add the limits a reader can't infer from the page: only the first 32 files load, binary or oversize files aren't shown, andSKILL.mdis shown without frontmatter. Then runtask docsandtask site:build.
Small, can go in now
- Gate on the
skill_filesfeature (apps/server/src/routes/knowledge.ts:361), not oncapabilities.skills.internal/adapter/server/features.go:39-43says a client checks it before offering a skill's files, and the BFF already readssnapshot.featuresforserver_info. - Tests. Add
/api/v1/skills/{name}/filesto the "503 runtime_unavailable on every knowledge route" table (apps/server/src/routes/knowledge.test.ts:577). Restore thetoContain("<b>escaped</b> deploys")assertion inconfigured-skill-detail.test.tsx:78; without it thequerySelector("b")check also passes when the description isn't rendered at all. - Background refetch errors (
skill-files.tsx:46). In TanStack Query v5,isErroris also true when a background refetch fails anddatais still there, so files already on screen are replaced by the error panel. Keep showing the data when there is some. formatBytes(skill-files.tsx:94) duplicates the one inlib/formatters.ts, and without an MB unit a 5 MB asset reads "4882.8 KB".- Firefox fade (
styles.css:477). Withoutanimation-timeline, the fallback fades both edges of every block all the time, by up to 40px, so taller blocks that don't scroll lose part of their first and last lines. Fade only when the block overflows, or drop the fade in the fallback. - Long names and scrolling (
skill-files.tsx:54,:60). A truncated file name has notitle, and the scrolling<pre>can't take keyboard focus in Safari; addtabIndex={0}and anaria-label. - Manage placeholder (
configured-skill-detail.tsx:46,:87-119). 33 lines behindSHOW_MANAGE_PLACEHOLDER = false, plus a test that pins the constant. I'd drop it and bring it back from history with #2184 / #2185. - Learned-skill comment (
skill-files.tsx:38-40) says a learned skill's page shows no Files section, but an active learned skill opens this page with a one-file Files section.
Can be follow-ups
- The Files request only starts after the inventory has rendered (
configured-skill-detail.tsx:124). Starting it from the route parameter removes the wait on a cold load. - The route's OpenAPI responses don't declare 429, 400 or 409, which
onErrorcan return.
71559a1 to
3a27f5b
Compare
Replace the Files placeholder on the configured skill page with the skill's real files: SKILL.md first, then the bundled files alphabetically, each with its size and syntax-highlighted text in its own scrolling block that fades at the edges. A file that cannot be shown, because it is too large or not text, stays listed with a one-line reason, and files past the first 32 are summarized as "N more files aren't shown".
Add the BFF route GET /api/v1/skills/{name}/files, which loads every file's text in one response so the browser makes a single request, and hide the Manage section behind a default-off SHOW_MANAGE_PLACEHOLDER flag until a write contract exists.
This needs a published @stacklok-oss/mecatl-sdk that contains skills.listFiles and skills.readFile, so it cannot typecheck against 0.4.0. After that release, bump the pin in server/package.json, the age-gate exclusion in pnpm-workspace.yaml, and the lockfile.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Review of the Files section found it showed a body without frontmatter as if it were the real SKILL.md, and that one slow or rejected file read could take the whole panel down. - Label the instructions entry from the daemon's `instructions` flag, so it reads "Instructions, without frontmatter" and not as the raw file. - Read at most four files at a time, skip a file already listed as over the read cap, and show any other per-file failure on that file only. A rate limited daemon failed the whole page before; a rejected session still reaches sign-in. - Gate the route on the `skill_files` feature, not on the skills capability. - Keep showing files when a background refetch fails, and use the shared byte formatter and a tooltip for truncated file names. - Start the files request when the page opens instead of after the inventory. - Drop the hidden Manage placeholder and the fade fallback for browsers without scroll-driven animations, which faded blocks that do not scroll. - Document the Files section in the Studio user docs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
fb38e2d to
6067e60
Compare
|
Thanks @peppescg, this was a thorough review, and the 429 point was real. Everything below is in Before merge
Small
Follow-ups
Checks: Fully or partially written by an AI agent. |
peppescg
left a comment
There was a problem hiding this comment.
Thanks Daniel. Every before-merge item is fixed, and I checked each one against the daemon side, so this is good to merge from my end.
Checked: the limiter is bounded, keeps order, and leaves no unhandled rejections; the > 25_000 skip matches the daemon's own size > skillfs.MaxOutputBytes check exactly; instructions flows from the daemon through the contract and BFF to the web, which keys off the flag; the gate uses the same skill_files identifier the daemon advertises; rethrowing only unauthenticated/authentication matches the other BFF routes; and the refetch test does fail on the old isError branch.
Nits, none blocking:
apps/web/src/features/knowledge/skill-files.tsx:92: Safari still doesn't make overflow scrollers keyboard-focusable (Chromium only started in 130, WebKit hasn't shipped it), which is what axe'sscrollable-region-focusablerule is about.tabIndex={0}plusaria-label={file.name}with abiome-ignoregiving the reason, or a focusablerole="region"wrapper, would cover it.skill-files.tsx:73-79: the truncated-name tooltip only opens on hover, because the trigger<span>can't take focus. Same keyboard gap; fine to fix together or in a follow-up.apps/server/src/mecatl/knowledge.ts:569: per-file failures are swallowed without a trace. A debug log with the error code would make a rate-limited daemon showing "Couldn't load this file." diagnosable.knowledge.ts:573-588: after an auth rethrow the other workers keep reading. Astoppedflag checked in thewhileloop would avoid the wasted daemon calls.user-docs/operating/studio.md:120-121: "highlighted by file type, each in its own scrolling block" is visible in the UI, so peruser-docs/AGENTS.mdthe paragraph can start at "The first entry…". Line 117 still says selecting a skill shows "its description and slug"; worth adding "and files".
…ard-coding the route count main's Studio job has been failing since the skill files route (#2155) landed next to the knowledge-route convention test (#2313). Each change was fine on its own: #2313 declared 400 and 429 on the 15 knowledge routes and pinned the count at 15, and #2155 added a 16th route without them. - Spread `transportErrors` into `listSkillFilesRoute` like the other knowledge routes. The OpenAPI document and generated types gain the two statuses. - The convention test no longer asserts an exact route count, so the next knowledge route is checked without editing the test. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Summary
SKILL.md, so it is not the raw file and its size is the body's size. Showing the frontmatter fields (allowed-tools,license) is tracked in Skills: expose frontmatter metadata to clients and reconcile the skill page with the latest prototypes #2301.GET /api/v1/skills/{name}/files. It loads the files' text in one response, at most four daemon reads at a time, without requesting a file already listed as over the cap, and only the first 32 files; the page then says "N more files aren't shown." The route answers501 skill_files_unsupportedunless the daemon announces theskill_filesfeature.scroll-fade-yutility from shadcn/ui'stailwind.css(MIT) tostyles.css. Browsers without scroll-driven animations get no fade, unlike shadcn's fallback, which fades both edges of every block even when it does not scroll.This is the last piece of #1853's in-scope work. It replaces the earlier placeholder-only version of this PR.
Depends on
#2192 (merged), which added the daemon RPCs, the HTTP routes, and the SDK methods
skills.listFilesandskills.readFile. Studio's BFF builds against the checked-out SDK (file:../../sdk/typescript; see #2288), so it uses them with no version bump. They are also in the published@stacklok-oss/mecatl-sdk0.5.0.Issue relationship
Closes #1853
Real Manage (writes) is tracked in #2184 and #2185, slash and mention completion in #2160, whether learned skills are usable in #2168, and the skill's frontmatter fields, how large skills load, and the
ListSkillsaccess class in #2301.Type of change
Test plan
Baseline checks
task lint)task test)task test:race)go run ./cmd/mecademo)task docs)task site:build)task api:check)task api:update+engine/CHANGELOG.mdStudio and docs only, no Go code touched. The Go checks above are not applicable.
task studio:check(lint, typecheck, tests, generated-contract check) passes on this branch rebased onmain: 219 server and 1,417 web tests.task docsandtask site:buildpass with the new paragraph inuser-docs/operating/studio.md.Due diligence record
What was checked, how, and what each check can and cannot show. The second commit addresses a review of the first; each fix has a test that I confirmed fails when the fix is removed (a larger concurrency limit, no over-cap skip, per-file failures propagating, an error shown over loaded data, and no early files request).
task studio:check,task docs,task site:build@stacklok-oss/mecatl-sdk0.5.0 from npm in for the checked-out link; typechecked and ran the BFF testsmecated, real BFF, the repo's own skills, plus synthetic skills for a binary file, an oversize file, a long file name, and a 40-file skill404 skill_not_foundbehave as specified--rate-limit 2 --rate-burst 5with a skill that needs 7 daemon calls429and no files. Now it returns200with the files that loaded and a reason on the rest (in the run, three of six files failed, includingSKILL.md, because the burst was spent)animation-timelinesupporttabIndex404for themNot tested: a real browser session with sign-in; skills with hundreds of files; the effect of the publication lock under heavy concurrent learned-skill activity (33 reads took about 29 ms with a small store and about 12 ms without one); a learned skill produced by the real learning pipeline instead of a seeded store; Safari (WebKit would not launch here, so the keyboard check covers Chromium and Firefox only); Firefox for anything but the fade fallback.
Changes
apps/web/src/features/knowledge/skill-files.tsxapps/web/src/features/knowledge/configured-skill-detail.tsxSkillFilesand starts its request on open; no Manage sectionapps/web/src/styles.cssscroll-fade-yutility, without a fallbackapps/server/src/mecatl/knowledge.ts,routes/knowledge.ts,app.tslistSkillFilesservice (bounded reads, per-file failures), the route, and theskillFilescapability from theskill_filesfeatureapps/contracts/**instructions, and the regenerated OpenAPI and clientuser-docs/operating/studio.mdUser-facing change
A configured skill's page lists its files with their text. The first entry is the skill's instructions, without frontmatter.
Special notes for reviewers
Intentional limitations, recorded in the code comments:
501and the panel shows that message. Studio is built from the same commit as the daemon contract it targets.roleortabIndex: Tab already reaches it in Chromium and Firefox, Safari was not tested, and the repo's a11y lint forbidstabIndexon apre.Fully or partially written by an AI agent.
🤖 Generated with Claude Code