feat(cli): publish agent skills from a declared docs.yml path - #17751
matlegault wants to merge 3 commits into
Conversation
…ctions.options.skills.path) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Adds a declared skills path to docs.yml with discovery/validation/manifest generation and upload wiring. The core design looks reasonable, but the uploadFiles default change silently turns publish paths that previously relied on the default into non-upload mode for skills, and the generated index.json temp file is never cleaned up. A couple of smaller robustness/readability items too.
- 🟡 1 warning(s)
- 🔵 2 suggestion(s)
AI Review
🟡 warning — packages/cli/docs-resolver/src/DocsDefinitionResolver.ts (line 253)
Changing uploadFiles from a defaulted parameter to an optional one makes uploadsEnabled a proxy for "caller explicitly passed uploadFiles". Any existing caller that relied on defaultUploadFiles (i.e. omitted the arg but does publish) now silently loses agent-skills uploads, and the filesToUploadSet files still get uploaded via the default — so the failure is partial and silent.
Safer to key off an explicit intent flag (e.g. a skipAgentSkills/validationOnly option set by the validation rules) rather than inferring it from argument presence.
🔵 suggestion — packages/cli/docs-resolver/src/DocsDefinitionResolver.ts (line 917)
existsSync(join(fern, WELL_KNOWN_SKILLS_DIRECTORY)) is a sync FS call in an otherwise async method; doesPathExist (already used by collectWellKnownSkillsFiles) keeps it consistent and non-blocking.
This review is complete for the current scope. Addressing all critical and warning findings above satisfies the review; suggestions are optional. Request another review only if subsequent changes introduce new behavior or materially expand the pull request's scope.
| const temporaryDirectory = await mkdtemp(path.join(tmpdir(), "fern-agent-skills-")); | ||
| const indexJsonAbsoluteFilePath = AbsoluteFilePath.of(path.join(temporaryDirectory, "index.json")); | ||
| await writeFile(indexJsonAbsoluteFilePath, generateSkillsIndexManifest(skills)); |
There was a problem hiding this comment.
🔵 suggestion
The mkdtemp directory holding the generated index.json is never removed. Each fern generate --docs run leaks a temp dir into the OS tmp dir (small, but unbounded across CI runs). Consider registering cleanup after the upload completes, or writing the manifest into an existing run-scoped temp dir that's already cleaned up.
There was a problem hiding this comment.
Deliberately left alone: in ledger mode publishDocs doesn't read file contents inside uploadFiles — it records ledgerFilePaths (hash → absolute path) and re-reads the files later in uploadMissingBlobs, after resolve() has returned. Removing the temp dir inside the resolver would therefore delete index.json before it's actually uploaded. The dir holds a single small JSON file under the OS tmp dir (which CI runners discard per job), so I'd rather leave the lifetime to the OS than add a process-exit hook here. Happy to revisit if publishDocs grows a run-scoped temp dir we can reuse.
Switched the existsSync call to doesPathExist in ca4a84f.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Re the |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 5 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| const filesToUpload: FilePathPair[] = [ | ||
| ...agentSkillsUploads, | ||
| ...Array.from(filesToUploadSet).map( |
There was a problem hiding this comment.
🟡 Shared skill images disappear
When docs content references a skill image, filesToUpload submits one source for two destinations. Image measurement retains only the docs destination. The published skill omits the image, breaking its installed reference.
Learn more
The resolver now emits two upload records when the same absolute file is both a declared skill asset and docs content. The publish uploader builds filesMap by absolute path, then image measurement deduplicates those paths in publishDocs. Only the last destination reaches the image manifest, while both records are excluded from the non-image path. The generated well-known destination therefore receives no upload.
Example: A skill contains assets/logo.png, and a docs page embeds that same repository file. The docs image path uploads, but .well-known/skills/my-skill/assets/logo.png does not. Installing the skill leaves its logo reference broken.
Recommended fix: Preserve uploads by destination rather than deduplicating images solely by absoluteFilePath. Each FilePathPair.relativeFilePath needs its own image manifest entry, while image dimensions and hashes can still be cached by source path.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed the behaviour: publishDocs keys filesMap/measuredImages by absoluteFilePath, so when the same image is both a declared-skill asset and embedded in docs content, only the docs destination survives and the .well-known/skills/<name>/… copy is never uploaded. It only triggers when a docs page reaches into the skills directory (e.g. ../.agents/skills/plant-care/assets/logo.png), which is unusual, and the fix belongs in the uploader (dedupe measurement/hashing by source path, emit one manifest entry per destination) rather than in the resolver. Since that touches the publish path for every site, I'm flagging it for the maintainers to decide whether it goes here or in a follow-up rather than folding it into this PR silently.
| async function collectSkillFiles(skillDirectory: AbsoluteFilePath): Promise<DeclaredSkillFile[]> { | ||
| const files = (await getAllFilesInDirectory(skillDirectory)).map( | ||
| (absoluteFilePath): DeclaredSkillFile => ({ | ||
| absoluteFilePath: AbsoluteFilePath.of(absoluteFilePath), | ||
| relativeFilePathInSkill: relative(skillDirectory, AbsoluteFilePath.of(absoluteFilePath)) | ||
| }) |
There was a problem hiding this comment.
🟡 Declared skill dotfiles are omitted
When a skill contains a dotfile, collectSkillFiles silently excludes it. The shared directory walker filters dot-prefixed files, producing incomplete installed skills.
Learn more
Declared skills use a general-purpose directory walker that intentionally drops any file whose basename starts with a dot. The generated manifest derives directly from this returned list, and the upload loop uses the same list. Hidden directories are traversed, but hidden files themselves disappear without validation or a warning.
Example: A skill contains SKILL.md and .tool-config. Discovery succeeds, but index.json lists only SKILL.md, and installation never receives .tool-config.
Recommended fix: Add a skill-specific recursive collector that includes dotfiles while retaining the intended symbolic-link policy. Use its complete result for both manifest generation and uploads.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional, and consistent with the existing .well-known/skills passthrough, which uses the same getAllFilesInDirectory walker and has always dropped dotfiles (and symlinks). Skills are consumed by npx skills add, which fetches exactly the files listed in index.json, and the dotfiles that show up in practice in a skills directory are .DS_Store/.gitkeep, which shouldn't be published. If a real need for hidden files in a bundle surfaces, the fix is a skill-specific collector as suggested — I'd rather not diverge from the passthrough behaviour speculatively in this PR.
| ), | ||
| skills: convertSkillsPageAction(pageActions.options?.skills) | ||
| skills: convertSkillsPageAction(pageActions.options?.skills), | ||
| skillsDirectory: resolveFilepath(pageActions.options?.skills?.path, absoluteFilepathToDocsConfig) |
There was a problem hiding this comment.
🟥 Skills path can expose host files
An absolute or escaping skills.path makes discoverDeclaredSkills read outside the repository. A matching SKILL.md lets publishing serve that directory's files publicly.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This isn't a trust boundary the CLI can enforce: docs.yml is authored by the same person who owns the repo and runs fern generate, and they can already publish any host file by copying it into fern/ (or referencing it as an image). ../ is intentionally allowed so a fern/docs.yml can point at a repo-root .agents/skills/ shared with local coding agents, and clamping to the git root would break the monorepo case where fern/ and the skills live in different sub-packages. Publishing also requires a SKILL.md with valid frontmatter in every served directory, so accidentally pointing at / or ~ fails validation rather than uploading. Leaving as-is; happy to add a warning for absolute paths if reviewers want a nudge.
… dir - Replace the regex link scanner with an mdast (GFM) parse so angle-bracket destinations, balanced parentheses, percent-encoding, and reference definitions are all checked for escaping the skill directory. - Write the generated index.json into one content-addressed temp directory per process instead of a fresh mkdtemp per rebuild. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Docs Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on
Docs generation runs |
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
1 participant
Description
Description
Linear ticket: Refs — none
Rebase of #16510 (closed, stale since July) onto current
main, with the review gaps from that PR addressed.Adds
page-actions.options.skills.pathto docs.yml. It points at a directory of agent skills in the docs repo (resolved relative to the folder containing docs.yml;../allowed, e.g. a repo-root.agents/skills/). Atfern generate --docsthe CLI:<path>/<name>/SKILL.md, validates the bundle (kebab-case name ≤64 chars matching its directory, non-empty description ≤1024 chars, unique names, no markdown references escaping the skill dir),.well-known/skills/index.jsondiscovery manifest ({ skills: [{ name, description, files }] }, the v0.1.0 layoutnpx skills add <docs-url>consumes),.well-known/skills/<name>/…alongside the docs bundle.Nothing is written back to the repo. The existing raw passthrough of
fern/.well-known/skills/still applies when nopathis declared; when one is declared, a hand-populated.well-known/skills/folder is ignored with a warning.pathis CLI-only:parseDocsConfigurationresolves it topageActions.options.skillsDirectory(anAbsoluteFilePaththat never reaches FDR), andskillson the wire keeps the same shape as before.Compatibility with fern-platform today
well-known-skills-transport.ts,docs-router/skills.ts) serves both.well-known/agent-skills(v0.2.0) and.well-known/skills(v0.1.0) from the uploaded files; theskillsCLI probesagent-skillsfirst and falls back toskills. Emitting the v0.1.0 layout is therefore still consumable end to end; moving the generated output to v0.2.0 (url+digestper skill) is a possible follow-up, not a blocker..well-known/agent-skillsthrough verbatim even whenpathis declared,valid-well-known-skillsnow validates that directory too (this was the open finding on feat(cli): generate .well-known agent skills bundle from docs.yml skills path #16510).repositoryfrom the skills page-action config. That is reverted here:SkillsModal.tsxonfern-platform/appstill rendersconfig.repositoryas the "View source" link, so removing it from the CLI would silently break existing sites.repositorystays accepted, validated (URL), and forwarded.Changes Made
fern/apis/docs-yml/definition/docs.yml+ generated schemas/types: new optionalskills.pathconfiguration-loader/parseDocsConfiguration.ts: resolvespath→skillsDirectorydocs-resolver/utils/declaredSkills.ts(new): discovery, validation, manifest generation. Markdown references are extracted from the GFM AST (mdast-util-from-markdown+mdast-util-gfm), not a regex, so<…>targets, balanced parentheses, percent-encoding and[ref]: …definitions are all checked. The generatedindex.jsonis written once per content hash into a single memoized temp dir per process.docs-resolver/utils/collectWellKnownSkillsFiles.ts: named constants for both well-known dirs; collector accepts a directory listdocs-resolver/DocsDefinitionResolver.ts: uploads generated skills +agent-skillspassthrough (only when real uploads are enabled, so validation-only reads don't touch the FS)docs-validatorrulesvalid-well-known-skills(declared-path validation,agent-skillsvalidation, conflict warning) andvalid-skills-page-action(emptypatherror)packages/cli/cli/changes/unreleased/feat-skills-declared-path.ymlTesting
pnpm turbo run test --filter @fern-api/docs-resolver --filter @fern-api/docs-validator --filter @fern-api/configuration-loader: 21/21 files each, 338 + 148 + 234 tests passingpnpm turbo run compilefor the affected packages;biome check/biome lint --error-on-warnings/biome formaton the changed files; prettier on changed ymlfern check --warningson Document page-actions.options.skills.path and publish the fern-docs skill from this site docs#6961 with the CLI built from this branch (pnpm fern:build):Found 0 errors(warnings pre-existing)fern-dev-tests/tests/agent-skills-declared-path.spec.tsspec in fern-platform; can only go green oncefern-devships this changeCompanion docs change: fern-api/docs#6961 (draft; blocked on the CLI release that ships this, since
fern.config.jsonthere pins 5.100.0).Link to Devin session: https://app.devin.ai/sessions/d920802193244dc58438f544c0cfb233
Open in Devin Desktop: https://app.devin.ai/desktop/session/d920802193244dc58438f544c0cfb233?variant=devin
Requested by: @matlegault