Skip to content

feat(schema): resolve symlinked schema directories#1299

Open
benjymoses wants to merge 2 commits into
Fission-AI:mainfrom
benjymoses:feat/symlinked-schema-dirs
Open

feat(schema): resolve symlinked schema directories#1299
benjymoses wants to merge 2 commits into
Fission-AI:mainfrom
benjymoses:feat/symlinked-schema-dirs

Conversation

@benjymoses

@benjymoses benjymoses commented Jul 3, 2026

Copy link
Copy Markdown

What it does

Adds a shared isSchemaDir() helper in resolver.ts and uses it everywhere schema directories are enumerated — the six discovery sites in listSchemas() / listSchemasWithInfo(), plus schema validate in schema.ts. The helper accepts real directories and symlinks whose target is a directory, while still rejecting symlinks-to-files and broken/dangling symlinks.

What it fixes

Schema discovery filtered directory entries with fs.Dirent.isDirectory(). That method reports the raw entry type from readdir, so a symlink returns false even when it points at a real directory. As a result, a symlinked schema directory placed in the user (~/.local/share/openspec/schemas), project, or package schema location was silently skipped — schemas could only be discovered as regular directories, never via symlink.

Before → After

~/.local/share/openspec/schemas/
└── my-schema -> /shared/team-schemas/my-schema   (symlink to a dir)
Before:  openspec schema list   →  (my-schema missing)
After:   openspec schema list   →  my-schema

Symlink-to-file and broken symlinks remain excluded in both cases.

Why it's safe to merge

  • Purely widening. Real directories still take the fast path (entry.isDirectory() short-circuits); only symlink entries — previously dropped entirely — now get a statSync follow-up. No existing input changes behaviour.
  • Defence in depth unchanged. The existing inner schema.yaml existence check still runs, so a symlinked dir without a valid schema.yaml is rejected exactly as a real dir would be.
  • Broken symlinks handled. statSync throws ENOENT on dangling links; the helper catches it and returns false rather than propagating.
  • Full suite passes (pnpm test); the resolver file is 47/47 including 9 new cases.

Notes for review

  • The one remaining raw entry.isDirectory() is inside isSchemaDir itself — intentional, it's the helper's own directory check.
  • isSchemaDir is exported so schema.ts reuses it rather than duplicating the statSync try/catch.
  • Other readdirSync call sites (view.ts, spec.ts, and copyDirRecursive in schema.ts) were left untouched — they enumerate changes/specs or perform recursive copies, not schema discovery.
  • Generated with Claude Code using claude-opus-4-8.

Summary by CodeRabbit

  • Bug Fixes
    • Schema validation and discovery now correctly recognize schema directories even when they are symlinks.
    • Project, user, and built-in schemas are now included more reliably, while symlinks to files or broken links are ignored.
    • Schema listings and schema details now reflect symlinked directories consistently during validation and lookup.

Schema discovery filtered directory entries with `Dirent.isDirectory()`,
which reports the raw entry type and returns false for symlinks — even
those pointing at a real directory. As a result, symlinked schema dirs in
the user/project/package schema locations were silently skipped.

Add a shared `isSchemaDir()` helper that accepts real directories and
dereferences symlinks (via statSync) to admit symlinked directories while
still rejecting symlinks-to-files and broken links. Use it at all six
discovery sites in resolver.ts and in `schema validate` in schema.ts.
Add unit tests for isSchemaDir (real dir, symlink-to-dir, symlink-to-file,
broken symlink, regular file) plus integration tests confirming listSchemas
and listSchemasWithInfo pick up a symlinked user schema dir while ignoring
symlinks whose target is a file.
@benjymoses benjymoses requested a review from TabishB as a code owner July 3, 2026 16:53
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c58c063c-e0ac-4a77-a52e-feaa27b6c520

📥 Commits

Reviewing files that changed from the base of the PR and between 65a7233 and 20458d2.

📒 Files selected for processing (3)
  • src/commands/schema.ts
  • src/core/artifact-graph/resolver.ts
  • test/core/artifact-graph/resolver.test.ts

📝 Walkthrough

Walkthrough

Adds an exported isSchemaDir helper in the artifact-graph resolver that treats symlinks as valid schema directories when their target resolves to a directory via fs.statSync, returning false for broken symlinks. listSchemas, listSchemasWithInfo, and the schema validate command are updated to use this helper instead of entry.isDirectory(). Tests cover symlink scenarios.

Changes

Symlink-aware schema directory detection

Layer / File(s) Summary
isSchemaDir helper
src/core/artifact-graph/resolver.ts
New isSchemaDir(parentDir, entry) function resolves symlinks via fs.statSync, returning true for directories/valid symlinks-to-directories and false for broken symlinks.
Schema listing wiring
src/core/artifact-graph/resolver.ts
listSchemas and listSchemasWithInfo replace entry.isDirectory() checks with isSchemaDir(...) across package, user, and project-local schema roots.
Schema validate command
src/commands/schema.ts
Imports isSchemaDir and uses it in the schema validate loop to determine schema-directory eligibility.
Symlink test coverage
test/core/artifact-graph/resolver.test.ts
New tests verify isSchemaDir behavior across real directories, symlinked directories, symlinks to files, broken symlinks, and regular files, plus symlink handling in listSchemas/listSchemasWithInfo.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

Suggested reviewers: TabishB, alfred-openspec

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: resolving symlinked schema directories.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant