Skip to content

feat(loader): inject package.json version into manifest via optional BrickSource hook#109

Merged
samuelds merged 2 commits into
developfrom
feature/inject-package-version
May 24, 2026
Merged

feat(loader): inject package.json version into manifest via optional BrickSource hook#109
samuelds merged 2 commits into
developfrom
feature/inject-package-version

Conversation

@samuelds
Copy link
Copy Markdown
Contributor

Problem

Every brick load via the CLI threw `INVALID_VERSION` at `parseManifest()`. Cause: published `@focus-mcp/brick-*` packages don't carry a `version` field in `mcp-brick.json` — the version lives in their `package.json`. The manifest validator was correctly rejecting them, but the validation was over-strict given the npm publishing reality.

Fix

Add an optional hook `readPackageVersion(name)` to the `BrickSource` interface. When a source implements it, the loader injects the returned value into both raw manifests (disk + module export) before they reach `parseManifest()`.

```ts
export interface BrickSource {
list(): Promise<readonly string[]>;
readManifest(name: string): Promise;
loadModule(name: string): Promise;
// NEW (optional):
readPackageVersion?(name: string): Promise<string | undefined>;
}
```

Helper:

```ts
function injectVersion(raw: unknown, pkgVersion: string | undefined): unknown {
if (pkgVersion === undefined) return raw;
if (raw === null || typeof raw !== 'object' || Array.isArray(raw)) return raw;
const obj = raw as Record<string, unknown>;
if (obj['version'] !== undefined) return raw; // preserve existing
return { ...obj, version: pkgVersion };
}
```

Loader change: just two call sites in `loadBricks()` now pass through `injectVersion()`.

Backwards compatibility

The hook is optional. Sources that don't implement it behave exactly as before (the INVALID_VERSION error still surfaces when the manifest legitimately has no version and no fallback is available). Existing manifests with an explicit `version` are never overwritten.

Tests (387/387 pass)

4 new unit tests:

  • ✅ injection on missing version when hook returns a string
  • ✅ existing `version` is preserved (hook value ignored)
  • ✅ INVALID_VERSION when hook is absent AND manifest has no version (existing behaviour)
  • ✅ INVALID_VERSION when hook returns `undefined` AND manifest has no version

Pair with CLI

Already shipped in `@focus-mcp/cli@2.4.1` (cli#144, cli#145): `FilesystemBrickSource.readPackageVersion()` resolves `@focus-mcp/brick-/package.json` (with walk-up fallback + symlink containment check).

Once this core PR is published, the bug is fully resolved end-to-end: `focus add ` will no longer throw INVALID_VERSION for any of the `@focus-mcp/brick-*` packages.

🤖 Generated with Claude Code

…BrickSource hook

Published @focus-mcp/brick-* packages don't carry a `version` field in
`mcp-brick.json` — the version lives in their `package.json`. So every
brick load via the CLI was throwing INVALID_VERSION at parseManifest().

This commit:

- Adds an optional `readPackageVersion(name): Promise<string|undefined>`
  to the `BrickSource` interface. Backwards-compatible — sources that
  don't implement it behave exactly as before.
- In `loadBricks()`, calls the hook (when available) and injects the
  returned value into both the raw disk manifest and the raw module-
  exported manifest before they reach `parseManifest()`.
- Adds `injectVersion()` helper: non-mutating, preserves existing
  `version` values, passes non-object raws through untouched.
- 4 new unit tests cover: injection on missing version, preservation of
  existing version, INVALID_VERSION when hook is absent, INVALID_VERSION
  when hook returns undefined. 387/387 pass.

Pair with @focus-mcp/cli@2.4.1 which implements the hook on
FilesystemBrickSource.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 24, 2026 19:54
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

This PR updates the brick loader to optionally source a missing manifest version from the brick package’s package.json, via a new optional BrickSource.readPackageVersion(name) hook. This aligns core’s manifest validation with the publishing shape of @focus-mcp/brick-* packages and prevents spurious INVALID_VERSION failures when loading bricks via the CLI.

Changes:

  • Extend BrickSource with optional readPackageVersion(name): Promise<string | undefined>.
  • Inject the package version into disk and module manifests (when missing) prior to calling parseManifest().
  • Add unit tests covering version injection and fallback behavior, plus a changeset for a minor bump.

Reviewed changes

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

File Description
packages/core/src/loader/brick-loader.ts Adds optional version fallback hook and injects version prior to manifest validation.
packages/core/src/loader/brick-loader.test.ts Adds tests for version injection and existing fallback behavior.
.changeset/feat-brick-loader-inject-package-version.md Documents the new loader behavior and publishes a minor version bump.

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

Comment thread packages/core/src/loader/brick-loader.ts
Comment thread packages/core/src/loader/brick-loader.ts Outdated
Comment thread packages/core/src/loader/brick-loader.test.ts
Comment thread packages/core/src/loader/brick-loader.test.ts
… hook + test assertions

Addresses copilot[bot] review on PR #109 (4 findings):

1. Bug: injectVersion() ran for validation but the brick pushed into
   result.bricks was the original (no-version) one — consumers reading
   `brick.manifest.version` would still see undefined. Now we push the
   brick with the parsed (version-injected) manifest.
2. Bug: readPackageVersion() was called unconditionally and any throw
   propagated as a load failure — even when the manifest already had a
   valid version. Now wrapped in try/catch: on throw, pkgVersion falls
   back to undefined and the manifest's own version (if any) is used.
3. Test gap: the existing "injects version" test didn't assert that the
   version reached the returned brick. Added the missing assertion.
4. Test comment: the "does not override" test had a comment referring
   to version '9.9.9' that didn't match the actual values. Fixed.

Plus a new test for the hook-throws-but-manifest-has-version case
(scenario #2 above). 388/388 pass.

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