Skip to content

feat(source): add readPackageVersion to FilesystemBrickSource#144

Merged
samuelds merged 3 commits into
developfrom
feature/filesystem-source-read-package-version
May 24, 2026
Merged

feat(source): add readPackageVersion to FilesystemBrickSource#144
samuelds merged 3 commits into
developfrom
feature/filesystem-source-read-package-version

Conversation

@samuelds
Copy link
Copy Markdown
Contributor

Why this PR

Core's brick-loader (per the user's diagnostic) parses the manifest via `parseManifest()`, which requires a SemVer `version` field. But every `@focus-mcp/brick-*` package on npm publishes an `mcp-brick.json` without a `version` field, and the JS module's exported manifest is just an import of that same JSON. Result: the loader throws `INVALID_VERSION` at every brick load.

Fix in core (handled separately): `brick-loader.ts` now accepts an optional `BrickSource.readPackageVersion()` hook and injects the result into both the disk manifest and the module manifest before `parseManifest()`.

This PR adds the CLI-side implementation of that hook on `FilesystemBrickSource`.

Implementation

Mirrors the resolution logic already used by `readManifest()`:

  • Preferred path: `require.resolve('@focus-mcp/brick-/package.json')`. Node 14+ always allows `./package.json` to be resolved regardless of an `exports` field, so this works for current and future bricks unconditionally.
  • Fallback: if `ERR_PACKAGE_PATH_NOT_EXPORTED` or `MODULE_NOT_FOUND` is thrown (older or weirdly-configured bricks), walk up from the package's main entry until a `package.json` is found.
  • Graceful degradation: returns `undefined` if neither path turns up a string `version`. The core loader keeps its existing INVALID_VERSION behaviour in that case.

Security: same `assertWithinBricksDir()` check as `readManifest()` to prevent symlink/alias escapes.

Tests

4 new unit tests on `FilesystemBrickSource` (22 total, all passing):

  • `readPackageVersion() returns the version from package.json (direct resolve)`
  • `readPackageVersion() falls back to walking up from main when /package.json is not exported`
  • `readPackageVersion() returns undefined when package.json has no version field`
  • `readPackageVersion() returns undefined when neither direct resolve nor walk-up find a package.json`

Existing tests untouched.

Out of scope

The 5 unrelated pre-existing test failures on develop (`NpmInstallerAdapter`, `makeNodeIO`) — verified they fail identically on develop without this change.

🤖 Generated with Claude Code

Implements the optional BrickSource.readPackageVersion() hook so the
core brick-loader can inject a brick's package.json version into its
manifest when mcp-brick.json doesn't carry a "version" field (the
current shape of every published @focus-mcp/brick-* package).

Without this, parseManifest() throws INVALID_VERSION at load time
because the disk manifest and the JS module's exported manifest both
lack a SemVer "version".

Implementation mirrors readManifest():
- Preferred: require.resolve('@focus-mcp/brick-<name>/package.json')
  which works unconditionally since Node 14 (./package.json is always
  exported, regardless of the package's exports field).
- Fallback: walk up from the package's main entry until a
  package.json is found (handles older bricks that restrict exports).
- Returns undefined when no package.json is found or it has no string
  version field — the loader is then free to fall back to its existing
  INVALID_VERSION behaviour.

Adds 4 unit tests covering: direct resolve happy path, walk-up
fallback on ERR_PACKAGE_PATH_NOT_EXPORTED, missing version field, and
total miss.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 24, 2026 16:56
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a CLI-side implementation of the new optional BrickSource.readPackageVersion() hook on FilesystemBrickSource, enabling core to inject a SemVer version into manifests when mcp-brick.json omits it (as with current @focus-mcp/brick-* packages).

Changes:

  • Implement FilesystemBrickSource.readPackageVersion() using require.resolve(.../package.json) with a walk-up fallback from the package main.
  • Apply the same bricks directory containment check pattern used elsewhere in FilesystemBrickSource.
  • Add unit tests covering direct resolve, fallback resolution, and missing/invalid version scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/source/filesystem-source.ts Adds readPackageVersion() to resolve and parse package.json version for a brick package.
src/source/filesystem-source.test.ts Adds unit tests validating the new readPackageVersion() behavior across key resolution paths.
Comments suppressed due to low confidence (2)

src/source/filesystem-source.ts:161

  • pkgPath is typed as string | undefined, but it’s passed to assertWithinBricksDir(pkgPath, ...) and then to readFile(pkgPath, ...), both of which expect a string. With strict: true this won’t type-check, and it also leaves a potential runtime footgun if future edits ever allow pkgPath to remain unset. Consider making pkgPath a non-optional string (e.g., return early on failure) or add an explicit if (!pkgPath) return undefined; guard before using it.
        let pkgPath: string | undefined;
        try {
            // Preferred: package.json is unconditionally resolvable since
            // Node 14 (`./package.json` is always exported, even when an
            // `exports` field restricts other paths).
            pkgPath = require.resolve(`@focus-mcp/brick-${brickName}/package.json`);
        } catch (err: unknown) {
            const code = (err as NodeJS.ErrnoException).code;
            if (code !== 'ERR_PACKAGE_PATH_NOT_EXPORTED' && code !== 'MODULE_NOT_FOUND') {
                throw err;
            }
            // Fallback: walk up from the package's main entry until we find
            // a package.json (handles older bricks that restrict exports).
            const pkgMain = require.resolve(`@focus-mcp/brick-${brickName}`);
            let dir = dirname(pkgMain);
            while (true) {
                const candidate = join(dir, 'package.json');
                try {
                    await readFile(candidate, 'utf-8');
                    pkgPath = candidate;
                    break;
                } catch {
                    const parent = dirname(dir);
                    if (parent === dir) return undefined;
                    dir = parent;
                }
            }
        }

        assertWithinBricksDir(pkgPath, this.#bricksDir);
        const raw = await readFile(pkgPath, 'utf-8');
        const parsed: unknown = JSON.parse(raw);

src/source/filesystem-source.ts:156

  • In the fallback walk-up, the code readFile(candidate, ...) probes for package.json before verifying that dir/candidate is within bricksDir. If require.resolve() ever returns a path outside bricksDir (Node resolution can walk up to ancestor node_modules), this can trigger reads outside the intended containment boundary before assertWithinBricksDir runs. Consider asserting containment on pkgMain before walking, and/or stopping the walk once dir reaches bricksDir (or asserting each candidate before reading it).
            // Fallback: walk up from the package's main entry until we find
            // a package.json (handles older bricks that restrict exports).
            const pkgMain = require.resolve(`@focus-mcp/brick-${brickName}`);
            let dir = dirname(pkgMain);
            while (true) {
                const candidate = join(dir, 'package.json');
                try {
                    await readFile(candidate, 'utf-8');
                    pkgPath = candidate;
                    break;
                } catch {
                    const parent = dirname(dir);
                    if (parent === dir) return undefined;
                    dir = parent;
                }
            }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/source/filesystem-source.test.ts
Comment thread src/source/filesystem-source.ts Outdated
…curity test

Addresses copilot[bot] review on PR #144 plus a biome complexity rule:

- Bug: if the package itself isn't installed, the first
  require.resolve('.../package.json') throws MODULE_NOT_FOUND, the
  fallback then calls require.resolve('@focus-mcp/brick-<name>') which
  also throws MODULE_NOT_FOUND — and that throw wasn't caught, breaking
  the JSDoc contract ("returns undefined if not found"). Now caught.
- Security: added a test for assertWithinBricksDir() on
  readPackageVersion() — symlink/alias escape protection.
- Complexity: extracted three helpers (resolvePackageJsonPath,
  resolvePackageMain, walkUpForPackageJson, extractVersion) so the
  public method's cyclomatic complexity drops back under Biome's
  threshold of 15.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@samuelds samuelds merged commit c5dbe11 into develop May 24, 2026
11 checks passed
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.

2 participants