Repository navigation
Set aside a workspace skill that will not load, instead of taking the list down with it - #194
Open
mroops0111 wants to merge 9 commits into
Open
mroops0111 wants to merge 9 commits into
mroops0111 wants to merge 9 commits into
Conversation
…of taking the list down with it A SKILL.md the workspace owner wrote by hand could stop every skill and every run, and say so only in the server log. Rejection is now scoped to the origin that can be fixed by whoever is reading, and travels back with the file and the reason. The structure validator gains one check on the same bar as the two it already had: a companion doc named by a relative path, which the agent's Read tool cannot resolve. House style stays out of it. Closes #188 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ult the way a readiness issue does The three checks took three different arguments and were spread inline at the caller, so a fourth meant editing it. They now share one input whose fenced blocks are already stripped, scanned once, and hang off a list. Issue kinds read as the fault then its subject, matching the readiness issues they sit beside. `extension-target` carried two different faults under one name and is split, which turned up a latent one: a SkillId is any non-empty string, so a directory naming no skill at all parsed fine and was dropped in silence. The grammar is now checked by splitSkillId, which is the one place that defines it. The validator moves beside validateOutput under domain/skill. It owns no fs, git, or subprocess, and infrastructure is for the adapters that do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t be answered Load, availability, and start were treated as two things, and the seam fell in the wrong place. `readinessIssuesFor` took an env parameter and let its caller decide which environment, so the route passed the server's own. Most of what a shipped prompt names in `requiredEnv` is injected by the runner as it assembles a run, so that question answered no every time, and every workspace would have shown `braid:ask` as not ready with six variables it never lacked. Each moment now has its own type, its own shape, and its own way of failing: - load reads the file alone, so `validateSkillFile` returns `SkillLoadIssue[]` and a failure keeps the file out of the list - availability reads it against one workspace, so `SkillManifest.availabilityIssuesIn` returns `SkillAvailabilityIssue[]` and a failure flags a skill that still loads - start reads it against the environment assembled for one run, so `assertSkillCanStart` throws, since a precondition for work about to begin has no reader to report to `requiredEnv` had never been checked anywhere, and two comments claimed otherwise. It is checked now, at the only moment it can be, beside the gateway preflight that already lives there. Names carry the moment. Every issue kind reads as the fault then its subject, the way `missing-mcp-server` already did. `missing-path` was declared and emitted by nobody, and is gone. The prompt section is `## Reference Documents`, matching the mounted directories every row of it names. The framework called these reference dirs from `SkillReferenceDir` through `$BRAID_SHARED_REFERENCE`, and only the heading said companion, so an author read two names for one thing. `SkillStructureValidator.ts` becomes `validateSkillFile.ts`, camelCase like the other function modules beside it, and its constants say what shape they hold rather than pairing COMMON against CATEGORY_SPECIFIC. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…c provider lists An unresolvable default was one of two checks issue #188 named and left for later, since the static case is checkable from the file alone: the form would preselect nothing and could still submit a value the picker never listed. A dynamic provider (graph-node, source, clarify) resolves its options against one workspace, which this file-only moment cannot read, so it stays unchecked here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… preflight fails after they were acquired assertSkillCanStart and the gateway preflight both run after the run token is issued, the output gate opened, and the agent-credential lease taken. A throw from either aborted start() before drain() ever ran, and drain()'s finally was the only place that released those three, so a workspace missing a skill's requiredEnv leaked a live bearer token plus an open gate entry and lease on every rejection. While in there: list() and listUnloadable() each triggered their own full scan(), so a caller wanting both, the skills-list route via Promise.all, walked the same directories twice. scan() now memoises its in-flight result per workspace, and its three independent root scans run together instead of one after another. Two of scan()'s own comments overclaimed what the code guarantees; both now say what actually holds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Several comments across the branch broke mid-clause instead of at a comma or period, two carried a semicolon, one introduced a colon where a comma reads as prose, and a duplicate comment landed on extensionTargetId while restoring an older one during rebase. No code changes, comments only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mroops0111
force-pushed
the
fix/skill-validator-isolation
branch
from
September 23, 2026 05:53
d48949d to
98fc031
Compare
SkillRegistry.ts and routes/skills.ts each had a line breaking with no comma or period at the end, found by writing a small script to check every comment's line endings instead of trusting a manual re-read. No code changes, comments only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s too assertSkillCanStart and the gateway preflight run after buildSessionDir has already staged skill bundles and reference-doc symlinks on disk. The earlier fix released the run token, gate, and credential lease on a preflight failure, but left that directory behind, since only drain()'s finally block, never reached, removed it. resolveSessionDir now says whether it built a fresh directory or reused one from a resumed run, so the failure path removes only the one this attempt owns, never a resumed conversation's own state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
/code-review flagged several tests quietly reinventing fixtures the repo already shares: - validateSkillFile.test.ts built its own frontmatter() from scratch where makeSkillManifestData (used by its sibling assertSkillCanStart .test.ts in the same commit) already does the same job, and repeated a five-field braid.inputs[] object across five tests where only the inputs array itself varied - FsSkillRegistry.test.ts's writeBrokenSkill hardcoded its own copy of the required-section list, the exact drift skillFixtures.ts's own doc comment says its callers are spared from - SubprocessSkillRunner.test.ts inlined a full no-op AgentCredentialStore and a four-method SkillRegistry, both shapes the repo already builds elsewhere (an inert fake, and runnerApp.ts's makeSingleSkillRegistry) - outputContractRetry.test.ts inlined the same SkillRegistry shape skillFixtures.ts gains an `omitSections` option so a test asking for a file missing one required section no longer needs its own section list. test-utils gains `inertAgentCredentialStore`, following the package's existing inert-fake convention (inertSkillRunner, inertRunRepository). Also restores the positive-path case category: generate with Output Files present passes, dropped when SkillStructureValidator.test.ts was replaced by validateSkillFile.test.ts and never carried over. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #188
A SKILL.md the workspace owner wrote by hand could stop every skill and every run in that workspace, and say so only in the server log.
FsSkillRegistry.readSkillFrontmatterthrew andlist()did not catch, so one bad file took outGET /skillsand, through it,buildSessionDirfor every run.What changes
Rejection is scoped to the origin that can be fixed. A builtin or plugin skill that will not load still stops the boot, since nobody but its publisher can fix one. A
workspaceorextensionfile is set aside instead, and the rest of the list survives.The reasons travel back.
GET /workspaces/:id/skillsnow returns{ items, unloadable }, and each item carriesavailability.Studio names the file. The workspace details panel gains a
Skill Filessection listing what did not load with its path and reasons, and what cannot run here yet. It is absent entirely when there is nothing wrong.Two new structure checks. A companion doc named by a relative path, which the agent's Read tool cannot resolve, so the file is never read. A pick input's
defaultnaming no option its static provider offers, which would preselect nothing and let a value the picker never listed reach$ARGUMENTS. Both meet the same bar as the checks already there: the file breaks at run time.Heading case, dash choice, section order, and prompt length were all written and then removed. They are review matters, and a skill withheld over a capital letter costs its author more than the rule saves. Section order in particular was already settled the other way in the code.
A skill is checked at three moments, not two
Load reads the file alone, so its answer holds in every workspace. Availability reads it against one workspace, so the same file passes in one and fails in the next. Start reads it against the environment assembled for one run, the first moment the runner-injected variables exist at all.
readinessIssuesForcollapsed availability and start into one method, took the server's own environment as itsenv, and would have reportedbraid:askas not-ready with six variables it never lacked. Each moment now has its own shape:SkillManifest.availabilityIssuesInfor availability,assertSkillCanStartfor start, thrown rather than reported since a precondition for work about to begin has no reader to report to.Two resource leaks this split turned up
assertSkillCanStartand the gateway preflight run after a run's token is issued, its output gate opened, and its agent credential leased. A failure there abortedstart()beforedrain()'sfinallyever ran, which was the only place any of those three, or the session directorybuildSessionDirhad already staged, got released. Both are now cleaned up on a preflight failure, the session directory only when this attempt built it fresh rather than reused one from a resumed run.Separately,
list()andlistUnloadable()each triggered their own fullscan(), so the skills-list route calling both walked the same directories twice per request.scan()now memoises its in-flight result per workspace, and its three independent root scans run together instead of one after another.Structural commits
SkillLoadIssue.kindreads as the fault then its subject, matchingSkillAvailabilityIssuebeside it.extension-targetcarried two faults under one name and is split.SkillIdis any non-empty string, so an extension directory naming no skill at all parsed fine and was dropped in silence.splitSkillIddefines that grammar and now checks it.validateOutputunderdomain/skill. It owns no fs, git, or subprocess, andinfrastructureis for the adapters that do.Test fixtures
Several tests were quietly reinventing fixtures the repo already shares: a hand-rolled
frontmatter()wheremakeSkillManifestDataalready does the job, a hardcoded copy of the required-section list whereskillFixtures.tsalready promises callers won't need one, and inline no-opAgentCredentialStore/SkillRegistryfakes where an inert fake andmakeSingleSkillRegistryalready exist. All four now go through the shared helper;skillFixtures.tsgained anomitSectionsoption andtest-utilsgainedinertAgentCredentialStoreto make that possible. Also restores thecategory: generate+Output Filespresent passes case, dropped whenSkillStructureValidator.test.tswas replaced and never carried over.Review notes
packages/server/src/infrastructure/skill/FsSkillRegistry.tsis the substance.scan()runs every origin in one pass and decides throw-or-set-aside by origin.packages/server/test/infrastructure/skill/shippedSkills.test.tsis new, and holds this repo's own six prompts to the contract rather than only the fixtures.list()is gone. It claimedbuiltin < plugin < workspace, later wins, but the namespace derives from the origin so a workspace skill is alwaysworkspace:<verb>and that branch never ran. The replacement comment says what actually holds: a plugin's ownskillNamespaceis unconstrained, so a collision there still resolves by insertion order, silently.extensionTargetId's first-hyphen convention (ddd-extracttargetsddd:extract) predates this PR and carries the same limit it always had: a namespace containing a hyphen is not addressable this way. Documented rather than redesigned, since the fix needs a real decision about the directory grammar that is out of scope here.Not fixed here
Route-level
skills.test.tswas not extended to exercise the newunloadable/availabilityresponse shape end to end, and the three new zod schemas (SkillLoadIssue,UnloadableSkill,SkillAvailabilityIssue) have no dedicated schema-level test. Both are coverage gaps, not fixture duplication, and are left for a follow-up.Checks
pnpm lintandpnpm typecheckclean. core 528, server 708, schema 337, studio 302 pass.🤖 Generated with Claude Code