From 2a4267c44fa1758b78d376bae97dd1ad32dc526f Mon Sep 17 00:00:00 2001 From: Ethan Dutton <46871249+ejdutton@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:13:56 -0400 Subject: [PATCH] fix(validation): directory links are valid; LINK_TARGETS_DIRECTORY narrows to typed slots (#126) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A directory link is a legitimate navigational target. `[docs](docs/)` renders as a navigable tree on GitHub; making it a hard error pushed authors into awkward workarounds (repointing to `docs/README.md` to dodge the rule). The rule was mis-scoped, not wrong in spirit. A directory is only a problem for a typed single-file slot — a reference contractually one file (e.g. a packaging `files:` source entry). It leaked into the general link path. Implements D1–D7 from the issue: - D1+D2: Delete the directory-error branch in the resources link-validator. A resolved directory now passes existence-checking like any file. The misnamed LINK_BROKEN_FILE "Link target is a directory" is gone. - D3: The bundling link walk (`walk-link-graph`) silently skips directory targets — no exclusion record, no surfaced issue. `LINK_TARGETS_DIRECTORY` now fires only when a typed single-file slot resolves to a directory; net-new emission added in `packaging-validator.validateFilesConfig`. - D4: New `local_directory` value in the `LinkType` enum; `classifyLink` returns it for local hrefs ending in `/`. Diagnostic hint only — validation treats it identically to `local_file`. - D5: HTML inherits the corrected rule through the shared `classifyLink` → `validateLocalFileLink` path (PR #116 had shipped the strict behavior; this corrects it). - D6: Explicit test confirms external trailing-slash URLs never trigger directory determination — filesystem-only is locked in. - D7: `docs/validation-codes.md` records that GitHub-style index resolution (`docs/` → `docs/README.md`) is deliberately out of scope. Migration: `validation.allow` entries for `LINK_TARGETS_DIRECTORY` against navigational links no longer match any emitted issue and will surface as `ALLOW_UNUSED`. `allow` entries against `files:` source paths still apply. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 1 + docs/validation-codes.md | 8 +-- packages/agent-schema/src/validation-codes.ts | 4 +- packages/agent-skills/src/skill-packager.ts | 4 +- .../src/validators/packaging-validator.ts | 31 ++++++++--- .../src/validators/walker-to-issues.ts | 1 - packages/agent-skills/src/walk-link-graph.ts | 9 ++-- .../agent-skills/test/skill-packager.test.ts | 11 ++-- .../validators/packaging-validator.test.ts | 52 +++++++++---------- .../test/validators/walker-to-issues.test.ts | 5 +- packages/resources/src/link-parser.ts | 7 +++ packages/resources/src/link-validator.ts | 36 ++++--------- .../src/schemas/resource-metadata.ts | 4 ++ .../link-validator.integration.test.ts | 46 +++++++++++++--- .../test/link-parser-classify.test.ts | 23 ++++++++ .../test/link-validator-helpers.test.ts | 18 +++++++ 16 files changed, 174 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b8c3753..6666719b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`ResourceMetadataSchema` is now `strict()`.** Shipped with first-class HTML support (#112): the resource-metadata schema rejects unknown top-level fields instead of silently accepting them, so a typo or stale field in code that constructs `ResourceMetadata` now fails at parse time rather than passing through. Move any extra data into a recognized field or drop it. - **Resource ids now carry a file-extension suffix.** `generateIdFromPath` appends `-` to every resource id (e.g. `guide.md` → `guide-md`, `guide.html` → `guide-html`, `README.md` → `readme-md`). This makes a markdown file and a same-stem HTML file distinct resources instead of colliding — the prerequisite for first-class HTML resources sharing a directory with their markdown source. Resource ids are internal, path-derived identifiers (never hand-authored in config or frontmatter), but anything that referenced an id by its old bare form must use the suffixed form — most visibly `vat rag query --resource-id` filters and re-indexed chunk ids (re-index to regenerate). +- **Navigational directory links are now valid; `LINK_TARGETS_DIRECTORY` narrows to typed single-file slots ([#126](https://github.com/jdutton/vibe-agent-toolkit/issues/126)).** A markdown or HTML link whose target resolves to an existing directory (`[docs](docs/)`, ``) is now a valid navigational reference — existence-checked like any other local link, and a resolved directory passes. The previous behavior in `vat resources validate` (a misnamed `LINK_BROKEN_FILE` with message *"Link target is a directory"*) and in the skill-bundling link walk (an `error`-severity `LINK_TARGETS_DIRECTORY` for any directory-shaped prose link) is gone. `LINK_TARGETS_DIRECTORY` now fires only when a **typed single-file slot** — currently a packaging `files:` source entry — resolves to a directory; the contract demanded a file. A new `local_directory` value joins the `LinkType` enum and `classifyLink` returns it for local hrefs ending in `/`; this is a diagnostic hint, never load-bearing — `docs/` (slash, classified as `local_directory`) and `docs` (no slash, shape-ambiguous `local_file`) are validated identically. The directory rule is explicitly filesystem-only: external trailing-slash URLs (`https://example.com/docs/`) are never judged by it. GitHub-style index resolution (`docs/` → `docs/README.md`) is documented as deliberately out of scope — it is unnecessary once a directory is a valid target. **Migration:** `validation.allow` entries targeting `LINK_TARGETS_DIRECTORY` for navigational links (e.g. `[ToC](docs/sub/)`) no longer match any emitted issue and will surface as `ALLOW_UNUSED` warnings; remove them. `allow` entries against `files:` source paths still apply. - **`vat resources validate` gains per-code severity configuration, and external-URL findings no longer fail the build by default.** Resource findings now use the same configurable severity framework as `vat skills`: each is a documented code (e.g. `LINK_BROKEN_FILE`, `EXTERNAL_URL_DEAD`) with a default severity, overridable per project under `resources.validation.severity` / `resources.validation.allow`. External-URL findings now default to `warning` and no longer flip the exit code (fixing a bug where they always failed the command); set their severity to `error` to restore failing. Severity now also accepts an `info` level. The never-implemented `resources.validation.checkLinks`/`checkAnchors`/`allowExternal` keys are removed. - **`validation.severity` / `validation.allow` keys are validated against real codes.** A mistyped code key (e.g. `LNIK_OUTSIDE_PROJECT`) is now a config-load error instead of a silent no-op. - **Corpus seed entries now require `bucket`, `confidence`, and `maturity` metadata fields.** `PluginEntrySchema` in `vat corpus scan`'s seed loader gains three required enum fields: `bucket: 'official' | 'community'`, `confidence: 'first-party' | 'curated' | 'listed'`, and `maturity: 'production' | 'experimental' | 'example'`. The bundled `corpus/seed.yaml` is updated; downstream callers running custom seeds must add the fields to every entry. `bucket` is the load-bearing discriminator (`official` entries report named findings; `community` entries are aggregate-only in follow-up work). The other two are descriptive metadata used by triage tooling. diff --git a/docs/validation-codes.md b/docs/validation-codes.md index 15b84806..b8976fb8 100644 --- a/docs/validation-codes.md +++ b/docs/validation-codes.md @@ -64,9 +64,11 @@ Static-analysis codes that fire anywhere markdown is analyzed — `vat resources ### `LINK_TARGETS_DIRECTORY` - **Default:** `error` -- **What:** Markdown link resolves to a directory rather than a file. -- **Why it matters:** Directory links are ambiguous — agents and renderers cannot load a directory as content. The author almost certainly intended a specific file inside it. -- **Fix:** Point the link at a specific file (e.g. `README.md` inside the directory) instead of the directory itself. +- **What:** A **typed single-file slot** (e.g. a packaging `files:` source entry) resolves to a directory. +- **Why it matters:** A typed single-file slot is contractually one file. A directory cannot satisfy that contract — packagers cannot copy a directory into a `dest` path declared as a file. +- **Fix:** Point the slot at a specific file inside the directory, or remove the entry. +- **Not in scope:** Navigational markdown/HTML links (`[docs](docs/)`, ``). A directory is a valid navigational target: it is existence-checked like any other local link, and a resolved directory passes. The trailing slash is a diagnostic hint only — never a correctness gate. This rule is also filesystem-only: an external URL whose path component looks like a directory (`https://example.com/docs/`) is never judged by this code. +- **Not implemented (decision record, #126):** GitHub-style index resolution (`docs/` → `docs/README.md`) is intentionally not implemented. It is unnecessary once a directory is a valid navigational target. ### `LINK_TO_NAVIGATION_FILE` diff --git a/packages/agent-schema/src/validation-codes.ts b/packages/agent-schema/src/validation-codes.ts index fc78fed6..748ebcd5 100644 --- a/packages/agent-schema/src/validation-codes.ts +++ b/packages/agent-schema/src/validation-codes.ts @@ -40,8 +40,8 @@ export const CODE_REGISTRY = { ), LINK_TARGETS_DIRECTORY: entry( 'error', - 'Markdown link resolves to a directory rather than a file.', - 'Point the link at a specific file (e.g. README.md inside the directory) instead of the directory itself.', + 'A typed single-file slot (e.g. a packaging files: source entry) resolves to a directory.', + 'Point the slot at a specific file inside the directory, or remove the entry. Navigational links to directories are valid and do not emit this code.', 'link_targets_directory', ), LINK_TO_NAVIGATION_FILE: entry( diff --git a/packages/agent-skills/src/skill-packager.ts b/packages/agent-skills/src/skill-packager.ts index 3044c424..93a958f6 100644 --- a/packages/agent-skills/src/skill-packager.ts +++ b/packages/agent-skills/src/skill-packager.ts @@ -439,7 +439,7 @@ export async function packageSkill( addBundledAssetsToOutputRegistry(outputResources, bundledAssets, pathMap, registry, collidedAssets); // Include excluded resources (with source paths) for pattern-based rule matching for (const excl of excludedReferences) { - if (excl.excludeReason === 'directory-target' || excl.excludeReason === 'outside-project') { + if (excl.excludeReason === 'outside-project') { continue; } const exclResource = (registry as WalkableRegistry).getResource(safePath.resolve(excl.path)); @@ -457,7 +457,7 @@ export async function packageSkill( const bundledResourceIds = new Set(bundledResources.map(r => r.id)); const excludedIds = [...new Set( excludedReferences - .filter(r => r.excludeReason !== 'directory-target' && r.excludeReason !== 'outside-project') + .filter(r => r.excludeReason !== 'outside-project') .map(r => { const res = (registry as WalkableRegistry).getResource(safePath.resolve(r.path)); return res?.id; diff --git a/packages/agent-skills/src/validators/packaging-validator.ts b/packages/agent-skills/src/validators/packaging-validator.ts index 57d8d66b..a7e8d892 100644 --- a/packages/agent-skills/src/validators/packaging-validator.ts +++ b/packages/agent-skills/src/validators/packaging-validator.ts @@ -13,7 +13,7 @@ * - vat skills audit --user (report issues, exit 0 always) */ -import { existsSync } from 'node:fs'; +import { existsSync, statSync } from 'node:fs'; import { readFile } from 'node:fs/promises'; import { basename, dirname } from 'node:path'; @@ -42,7 +42,6 @@ import { import { walkerExclusionsToIssues } from './walker-to-issues.js'; /** Exclude reason constants to avoid duplicate string literals */ -const EXCLUDE_REASON_DIRECTORY = 'directory-target' as const; const EXCLUDE_REASON_OUTSIDE_PROJECT = 'outside-project' as const; const DETAIL_REASON_DEPTH: ExcludedReferenceDetail['reason'] = 'depth-exceeded'; @@ -128,10 +127,15 @@ export interface PackagingValidationResult { } /** - * Validate files config entries for duplicate dest values. + * Validate files config entries: + * - duplicate dest values (DUPLICATE_FILES_DEST) + * - source paths that resolve to a directory (LINK_TARGETS_DIRECTORY) — a + * `files:` entry is a typed single-file slot; a directory cannot satisfy + * the contract. See #126. */ function validateFilesConfig( files: Array<{ source: string; dest: string }> | undefined, + skillDir: string, ): ValidationIssue[] { if (!files?.length) return []; @@ -148,6 +152,20 @@ function validateFilesConfig( }); } destSet.add(normalized); + + const sourceAbsolute = safePath.resolve(skillDir, entry.source); + try { + // eslint-disable-next-line security/detect-non-literal-fs-filename -- entry.source from validated config + if (existsSync(sourceAbsolute) && statSync(sourceAbsolute).isDirectory()) { + issues.push(createRegistryIssue( + 'LINK_TARGETS_DIRECTORY', + `files: source '${entry.source}' resolves to a directory; a files: entry is a single-file slot.`, + toForwardSlash(entry.source), + )); + } + } catch { + // stat failure: leave it to other validation paths to report. + } } return issues; @@ -269,7 +287,7 @@ export async function validateSkillForPackaging( } // Validate files config - rawIssues.push(...validateFilesConfig(packagingConfig?.files)); + rawIssues.push(...validateFilesConfig(packagingConfig?.files, dirname(skillPath))); // Compat capability detection: collect observations from SKILL.md and // surface each as a CAPABILITY_* issue. Observations are also returned @@ -649,11 +667,11 @@ function getResolvedMarkdownLinks( * Reasons that are reported as validation errors instead of excluded references. * These are filtered out of the excluded references list. */ -const VALIDATION_ERROR_REASONS: ReadonlySet = new Set([EXCLUDE_REASON_DIRECTORY, EXCLUDE_REASON_OUTSIDE_PROJECT]); +const VALIDATION_ERROR_REASONS: ReadonlySet = new Set([EXCLUDE_REASON_OUTSIDE_PROJECT]); /** * Deduplicate excluded references by path, preserving detail from first occurrence. - * Filters out entries reported as validation errors (directory-target, outside-project). + * Filters out entries reported as validation errors (outside-project). */ function deduplicateExcludedReferences( excludedReferences: LinkResolution[], @@ -692,7 +710,6 @@ function mapExcludeReason( case 'skill-definition': return 'skill-definition'; case 'gitignored': return 'gitignored'; case 'depth-exceeded': - case EXCLUDE_REASON_DIRECTORY: case EXCLUDE_REASON_OUTSIDE_PROJECT: case 'missing-target': case undefined: diff --git a/packages/agent-skills/src/validators/walker-to-issues.ts b/packages/agent-skills/src/validators/walker-to-issues.ts index a8d4e2f0..89c1b5fe 100644 --- a/packages/agent-skills/src/validators/walker-to-issues.ts +++ b/packages/agent-skills/src/validators/walker-to-issues.ts @@ -9,7 +9,6 @@ const REASON_TO_CODE: Record, Issue 'outside-project': 'LINK_OUTSIDE_PROJECT', gitignored: 'LINK_TO_GITIGNORED_FILE', 'skill-definition': 'LINK_TO_SKILL_DEFINITION', - 'directory-target': 'LINK_TARGETS_DIRECTORY', 'navigation-file': 'LINK_TO_NAVIGATION_FILE', 'missing-target': 'LINK_MISSING_TARGET', 'pattern-matched': null, diff --git a/packages/agent-skills/src/walk-link-graph.ts b/packages/agent-skills/src/walk-link-graph.ts index 451613ae..170545a7 100644 --- a/packages/agent-skills/src/walk-link-graph.ts +++ b/packages/agent-skills/src/walk-link-graph.ts @@ -28,7 +28,7 @@ export interface LinkResolution { /** Whether the file will be bundled */ bundled: boolean; /** Reason it was excluded (only set when bundled is false) */ - excludeReason?: 'depth-exceeded' | 'pattern-matched' | 'directory-target' | 'outside-project' | 'navigation-file' | 'skill-definition' | 'gitignored' | 'missing-target' | undefined; + excludeReason?: 'depth-exceeded' | 'pattern-matched' | 'outside-project' | 'navigation-file' | 'skill-definition' | 'gitignored' | 'missing-target' | undefined; /** The rule that matched (only set for pattern-matched exclusions) */ matchedRule?: ExcludeRule | undefined; /** Link text from the source markdown */ @@ -183,11 +183,14 @@ function checkExclusions( excludeMatchers: ExcludeMatcher[], excludedReferences: LinkResolution[], ): boolean { - // Check if target is a directory + // Directory targets are valid navigational link targets (#126). A directory + // cannot be bundled as a file, so the walker silently skips it — no + // exclusion record, no surfaced issue. LINK_TARGETS_DIRECTORY is reserved + // for typed single-file slots (e.g. packaging `files:` source entries) + // where the contract demands a file. try { // eslint-disable-next-line security/detect-non-literal-fs-filename -- Path from parsed markdown if (existsSync(targetPath) && statSync(targetPath).isDirectory()) { - excludedReferences.push(makeExclusion(targetPath, 'directory-target', link)); return true; } } catch { diff --git a/packages/agent-skills/test/skill-packager.test.ts b/packages/agent-skills/test/skill-packager.test.ts index 70de5c83..b2a14f9f 100644 --- a/packages/agent-skills/test/skill-packager.test.ts +++ b/packages/agent-skills/test/skill-packager.test.ts @@ -652,17 +652,18 @@ describe('packageSkill - stripPrefix edge cases', () => { }); // ============================================================================ -// Excluded resource filtering (directory-target, outside-project) +// Directory link handling (#126 — directories are valid navigational targets) // ============================================================================ -describe('packageSkill - excluded resource filtering', () => { - it('should skip directory-target exclusions from output registry', async () => { +describe('packageSkill - directory link handling', () => { + it('packages a skill with a navigational directory link; the directory is not bundled', async () => { const tmp = getTempDir(); const sub = safePath.join(tmp, 'docs'); await mkdir(sub, { recursive: true }); await writeFile(safePath.join(sub, 'page.md'), '# Page'); - // Link to a directory — should be excluded with 'directory-target' reason + // `[docs](./docs/)` is a navigational link; the walker silently skips + // the directory itself but still follows `./docs/page.md`. const sp = await writeSkillMd( tmp, UNIT_SKILL_NAME, @@ -671,9 +672,7 @@ describe('packageSkill - excluded resource filtering', () => { const result = await packWithOutput(sp); - // page.md should be bundled, but directory link should be excluded expect(existsSync(safePath.join(result.outputPath, 'resources', 'page.md'))).toBe(true); - // The skill should still package without errors expect(result.files.root).toBe('SKILL.md'); }); }); diff --git a/packages/agent-skills/test/validators/packaging-validator.test.ts b/packages/agent-skills/test/validators/packaging-validator.test.ts index 58031b30..4224c4fd 100644 --- a/packages/agent-skills/test/validators/packaging-validator.test.ts +++ b/packages/agent-skills/test/validators/packaging-validator.test.ts @@ -661,29 +661,6 @@ describe('validateSkillForPackaging - Severity / allow config (framework)', () = expect(result.activeErrors.map(e => e.code)).not.toContain('OUTSIDE_PROJECT_BOUNDARY'); }); - it('allows LINK_TARGETS_DIRECTORY per-path', async () => { - const tempDir = getTempDir(); - const conceptsDir = safePath.join(tempDir, 'docs/sub'); - fs.mkdirSync(conceptsDir, { recursive: true }); - fs.writeFileSync(safePath.join(conceptsDir, 'README.md'), '# Sub'); - - const skillContent = createSkillContent( - { name: TEST_SKILL_NAME, description: VALID_DESCRIPTION }, - '\n# Test\n\nSee [Sub](./docs/sub/) for details.', - ); - const { skillPath } = createTransitiveSkillStructure(tempDir, {}, skillContent); - - // location is 'docs/sub' (relative to project root, no trailing slash) - const result = await validateSkillForPackaging(skillPath, { - validation: { - allow: { - LINK_TARGETS_DIRECTORY: [{ paths: ['docs/sub'], reason: 'ToC target' }], - }, - }, - }); - - expect(result.activeErrors).toHaveLength(0); - }); }); describe('validateSkillForPackaging - Metadata reporting', () => { @@ -877,7 +854,11 @@ describe('validateSkillForPackaging - Link collection integration', () => { expect(result.metadata.excludedReferences).toEqual([]); }); - it('should error when skill links to a directory', async () => { + it('accepts a navigational directory link without emitting LINK_TARGETS_DIRECTORY (#126)', async () => { + // `[Concepts](./concepts/)` is a navigational link to an existing + // directory. Per #126, this is valid — LINK_TARGETS_DIRECTORY is + // reserved for typed single-file slots (e.g. packaging `files:` + // source entries), not navigational markdown links. const tempDir = getTempDir(); const conceptsDir = safePath.join(tempDir, 'concepts'); fs.mkdirSync(conceptsDir, { recursive: true }); @@ -891,10 +872,29 @@ describe('validateSkillForPackaging - Link collection integration', () => { const result = await validateSkillForPackaging(skillPath); - expect(result.status).toBe('error'); + expect(result.activeErrors.map(e => e.code)).not.toContain('LINK_TARGETS_DIRECTORY'); + }); + + it('emits LINK_TARGETS_DIRECTORY when a files: source entry resolves to a directory (#126)', async () => { + // A `files:` source is a typed single-file slot — it cannot be + // satisfied by a directory, so LINK_TARGETS_DIRECTORY fires here. + const tempDir = getTempDir(); + const scriptsDir = safePath.join(tempDir, 'scripts'); + fs.mkdirSync(scriptsDir, { recursive: true }); + + const skillContent = createSkillContent( + { name: TEST_SKILL_NAME, description: VALID_DESCRIPTION }, + '\n# Test\n', + ); + const { skillPath } = createTransitiveSkillStructure(tempDir, {}, skillContent); + + const result = await validateSkillForPackaging(skillPath, { + files: [{ source: 'scripts', dest: 'scripts/tool.mjs' }], + }); + const dirError = result.activeErrors.find(e => e.code === 'LINK_TARGETS_DIRECTORY'); expect(dirError).toBeDefined(); - expect(dirError?.message).toContain('concepts'); + expect(dirError?.message).toContain('scripts'); }); it('should report directFileCount <= fileCount when links are excluded by depth', async () => { diff --git a/packages/agent-skills/test/validators/walker-to-issues.test.ts b/packages/agent-skills/test/validators/walker-to-issues.test.ts index 2b29a8cd..72259a53 100644 --- a/packages/agent-skills/test/validators/walker-to-issues.test.ts +++ b/packages/agent-skills/test/validators/walker-to-issues.test.ts @@ -12,12 +12,14 @@ const resolution = (reason: LinkResolution['excludeReason'], path: string): Link describe('walkerExclusionsToIssues', () => { it('maps each reason to the expected issue code', () => { + // Directory targets are silently skipped by the walker (#126) and so do + // not appear in excludedReferences — there is no `directory-target` + // reason to map. const input: LinkResolution[] = [ resolution('depth-exceeded', '/root/a.md'), resolution('outside-project', '/other/b.md'), resolution('gitignored', '/root/dist/c.md'), resolution('skill-definition', '/root/other/SKILL.md'), - resolution('directory-target', '/root/dir'), resolution('navigation-file', '/root/README.md'), resolution('missing-target', '/root/nope.md'), resolution('pattern-matched', '/root/docs/x.md'), @@ -29,7 +31,6 @@ describe('walkerExclusionsToIssues', () => { 'LINK_OUTSIDE_PROJECT', 'LINK_TO_GITIGNORED_FILE', 'LINK_TO_SKILL_DEFINITION', - 'LINK_TARGETS_DIRECTORY', 'LINK_TO_NAVIGATION_FILE', 'LINK_MISSING_TARGET', // pattern-matched emits no issue diff --git a/packages/resources/src/link-parser.ts b/packages/resources/src/link-parser.ts index 18065436..b8d2485a 100644 --- a/packages/resources/src/link-parser.ts +++ b/packages/resources/src/link-parser.ts @@ -194,6 +194,7 @@ function extractLinkText(node: Link | LinkReference): string { * classifyLink('#heading') // 'anchor' * classifyLink('./file.md') // 'local_file' * classifyLink('./file.md#anchor') // 'local_file' + * classifyLink('docs/') // 'local_directory' * ``` */ export function classifyLink(href: string): LinkType { @@ -211,6 +212,12 @@ export function classifyLink(href: string): LinkType { if (href.includes(':')) { return 'unknown'; } + // Trailing slash signals directory intent. Diagnostic hint only — validation + // treats local_directory and local_file identically. Anchored hrefs are + // handled below (this branch only matches when there is no '#'). + if (href.endsWith('/')) { + return 'local_directory'; + } // Links with anchors are still local file links if (href.includes('#')) { return 'local_file'; diff --git a/packages/resources/src/link-validator.ts b/packages/resources/src/link-validator.ts index 58caa262..8aaa2616 100644 --- a/packages/resources/src/link-validator.ts +++ b/packages/resources/src/link-validator.ts @@ -15,7 +15,6 @@ * - External resources (outside project) skip git-ignore checks */ -import fs from 'node:fs/promises'; import path from 'node:path'; import { createRegistryIssue, type ValidationIssue } from '@vibe-agent-toolkit/agent-schema'; @@ -89,6 +88,9 @@ export async function validateLink( ): Promise { switch (link.type) { case 'local_file': + case 'local_directory': + // Slash is a diagnostic hint, not a behavior gate: existence-checked the + // same as local_file. A resolved directory is a valid target. return await validateLocalFileLink(link, sourceFilePath, fragmentsByFile, options); case 'anchor': @@ -246,18 +248,10 @@ async function validateLocalFileLink( const notFound = fileExistenceIssue(fileResult, link, sourceFilePath, options?.projectRoot); if (notFound) return notFound; - if (fileResult.isDirectory) { - return createRegistryIssue( - 'LINK_BROKEN_FILE', - `Link target is a directory: ${fileResult.resolvedPath}`, - linkExtras( - link, - sourceFilePath, - options?.projectRoot, - 'Link to a file inside the directory (e.g., README.md or index.md), or fix the link to point at the intended file.', - ), - ); - } + // A resolved target — file or directory — is a valid link target. The + // directory rule (LINK_TARGETS_DIRECTORY) is reserved for typed single-file + // slots in packaging config (see packages/agent-skills), not navigational + // markdown/HTML links. See issue #126. const gitIgnoreIssue = gitIgnoreSafetyIssue(link, sourceFilePath, fileResult.resolvedPath, options); if (gitIgnoreIssue) return gitIgnoreIssue; @@ -323,24 +317,12 @@ async function validateAnchorLink( */ async function validateResolvedFile( resolvedPath: string, -): Promise<{ exists: boolean; resolvedPath: string; actualName?: string; isDirectory: boolean }> { +): Promise<{ exists: boolean; resolvedPath: string; actualName?: string }> { const verification = await verifyCaseSensitiveFilename(resolvedPath); - let isDirectory = false; - if (verification.exists) { - try { - // eslint-disable-next-line security/detect-non-literal-fs-filename -- resolvedPath validated by verifyCaseSensitiveFilename - const stats = await fs.stat(resolvedPath); - isDirectory = stats.isDirectory(); - } catch { - // Stat failed after verifyCaseSensitiveFilename said exists — treat as file. - } - } - - const result: { exists: boolean; resolvedPath: string; actualName?: string; isDirectory: boolean } = { + const result: { exists: boolean; resolvedPath: string; actualName?: string } = { exists: verification.exists, resolvedPath, - isDirectory, }; if (verification.actualName) { diff --git a/packages/resources/src/schemas/resource-metadata.ts b/packages/resources/src/schemas/resource-metadata.ts index 23253b86..be362cb3 100644 --- a/packages/resources/src/schemas/resource-metadata.ts +++ b/packages/resources/src/schemas/resource-metadata.ts @@ -6,6 +6,9 @@ import { SHA256Schema } from './checksum.js'; * Type of link found in markdown resources. * * - `local_file`: Link to a local file (relative or absolute path) + * - `local_directory`: Link whose href ends with `/` (e.g., `docs/`). + * Diagnostic hint only — validation treats it identically to `local_file` + * (existence-checked; directory targets are valid). * - `anchor`: Link to a heading anchor (e.g., #heading-slug) * - `external`: HTTP/HTTPS URL to external resource * - `email`: Mailto link @@ -13,6 +16,7 @@ import { SHA256Schema } from './checksum.js'; */ export const LinkTypeSchema = z.enum([ 'local_file', + 'local_directory', 'anchor', 'external', 'email', diff --git a/packages/resources/test/integration/link-validator.integration.test.ts b/packages/resources/test/integration/link-validator.integration.test.ts index f3920340..5af69452 100644 --- a/packages/resources/test/integration/link-validator.integration.test.ts +++ b/packages/resources/test/integration/link-validator.integration.test.ts @@ -754,28 +754,60 @@ describe('validateLink', () => { expect(result).toBeNull(); }); - it('emits broken_file when leading-/ target is a directory', async () => { - // /docs/ resolves to projectRoot/docs (an existing directory). + it('treats existing directory as valid target (leading-/ href)', async () => { + // /docs/ resolves to projectRoot/docs (an existing directory). Per #126, + // a directory is a valid navigational link target. await assertValidation( { sourceFile, - link: createLink('local_file', '/docs/', 'Directory target', 1), + link: createLink('local_directory', '/docs/', 'Directory target', 1), headingsMap: fragmentIndex(), - expected: { code: 'LINK_BROKEN_FILE', messageContains: 'Link target is a directory' }, + expected: null, validationOptions: { projectRoot, skipGitIgnoreCheck: true }, }, expect, ); }); - it('emits broken_file when relative link target is a directory', async () => { + it('treats existing directory as valid target (relative href)', async () => { // sourceFile is in projectRoot/docs/sub; ../ resolves to projectRoot/docs. await assertValidation( { sourceFile, - link: createLink('local_file', '../', 'Relative directory target', 1), + link: createLink('local_directory', '../', 'Relative directory target', 1), + headingsMap: fragmentIndex(), + expected: null, + validationOptions: { projectRoot, skipGitIgnoreCheck: true }, + }, + expect, + ); + }); + + it('treats slashless directory-shaped href as valid when target exists', async () => { + // `/docs` (no trailing slash) is shape-ambiguous (local_file by + // classification); once resolved to a directory it is treated identically + // to `/docs/`. + await assertValidation( + { + sourceFile, + link: createLink('local_file', '/docs', 'Slashless directory target', 1), + headingsMap: fragmentIndex(), + expected: null, + validationOptions: { projectRoot, skipGitIgnoreCheck: true }, + }, + expect, + ); + }); + + it('emits broken_file when a directory-shaped href targets a missing path', async () => { + // Regression guard: existence checking still fires; missing directories + // are LINK_BROKEN_FILE (not a directory error). + await assertValidation( + { + sourceFile, + link: createLink('local_directory', '/missing-dir/', 'Missing directory', 1), headingsMap: fragmentIndex(), - expected: { code: 'LINK_BROKEN_FILE', messageContains: 'Link target is a directory' }, + expected: { code: 'LINK_BROKEN_FILE', messageContains: 'File not found' }, validationOptions: { projectRoot, skipGitIgnoreCheck: true }, }, expect, diff --git a/packages/resources/test/link-parser-classify.test.ts b/packages/resources/test/link-parser-classify.test.ts index 35ceffd0..cc78625f 100644 --- a/packages/resources/test/link-parser-classify.test.ts +++ b/packages/resources/test/link-parser-classify.test.ts @@ -25,4 +25,27 @@ describe('classifyLink (exported)', () => { expect(classifyLink('javascript:void(0)')).toBe('unknown'); expect(classifyLink('tel:+1234567890')).toBe('unknown'); }); + it('classifies trailing-slash hrefs as local_directory', () => { + expect(classifyLink('docs/')).toBe('local_directory'); + expect(classifyLink('./docs/')).toBe('local_directory'); + expect(classifyLink('../docs/')).toBe('local_directory'); + expect(classifyLink('/docs/')).toBe('local_directory'); + expect(classifyLink('/')).toBe('local_directory'); + }); + it('keeps external trailing-slash URLs external (directory rule is local-only)', () => { + expect(classifyLink('https://example.com/docs/')).toBe('external'); + expect(classifyLink('http://example.com/')).toBe('external'); + }); + it('keeps slashless directory-shaped hrefs as local_file (resolved by stat)', () => { + // `docs` (no slash, no extension) stays shape-ambiguous — validator + // resolves it via the filesystem; once resolved to a directory it is + // treated identically to `docs/`. + expect(classifyLink('docs')).toBe('local_file'); + expect(classifyLink('./docs')).toBe('local_file'); + }); + it('classifies trailing-slash + anchor as local_file (anchored ref takes precedence)', () => { + // `docs/#anchor` does not end with `/`, so the directory branch does not + // fire; the existing anchor-handling branch keeps it as local_file. + expect(classifyLink('docs/#anchor')).toBe('local_file'); + }); }); diff --git a/packages/resources/test/link-validator-helpers.test.ts b/packages/resources/test/link-validator-helpers.test.ts index 603d2686..cf6c7f99 100644 --- a/packages/resources/test/link-validator-helpers.test.ts +++ b/packages/resources/test/link-validator-helpers.test.ts @@ -14,6 +14,7 @@ import { fragmentIndex, gitIgnoreSafetyIssue, resolutionFailureIssue, + validateLink, type ValidateLinkOptions, } from '../src/link-validator.js'; import type { ResourceLink } from '../src/types.js'; @@ -231,3 +232,20 @@ describe('gitIgnoreSafetyIssue', () => { expect(issue?.message).toContain(TARGET_SECRET); }); }); + +describe('validateLink — external boundary (#126)', () => { + // Lock-in: the directory rule is filesystem-only. External URLs — including + // folder-shaped ones — must never trigger a directory determination. A + // server-resolved trailing slash has no client-determinable meaning, so we + // judge external links by network response alone. + it('returns null for external trailing-slash URL without filesystem checks', async () => { + const link: ResourceLink = { + type: 'external', + href: 'https://example.com/docs/', + text: 'docs', + line: 1, + }; + const result = await validateLink(link, SOURCE, fragmentIndex(), makeOptions()); + expect(result).toBeNull(); + }); +});