Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]

### Fixed
- **AppPlugin getService crash on missing services** — `AppPlugin.start()` and
`loadTranslations()` now wrap `ctx.getService()` in try/catch, since the kernel's
`getService` throws when a service is not registered (rather than returning `undefined`).
This was the direct cause of `plugin.app.com.example.crm failed to start` — the i18n
service was not registered, so `getService('i18n')` threw an unhandled exception.
- **CLI serve: host config AppPlugin mis-wrap** — `serve.ts` no longer wraps a host/aggregator config
(one that already contains instantiated plugins in its `plugins` array) with an extra `AppPlugin`.
This prevents the `plugin.app.dev-workspace failed to start` error and eliminates duplicate plugin
registration when running `pnpm dev`.
- **plugin-auth CJS→ESM interop** — Added `module` and `exports` fields to
`@objectstack/plugin-auth` package.json so Node.js resolves the ESM build (`.mjs`) when using
dynamic `import()`, eliminating the `ExperimentalWarning: CommonJS module … is loading ES Module`
warning caused by `better-auth` being ESM-only.
- **i18n service registration & state inconsistency** — Discovery API (`getDiscoveryInfo`) now uses
the same async `resolveService()` fallback chain that request handlers (`handleI18n`) use, ensuring
the reported service status is always consistent with actual runtime availability.
Expand Down
6 changes: 5 additions & 1 deletion packages/cli/src/commands/serve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import net from 'net';
import chalk from 'chalk';
import { bundleRequire } from 'bundle-require';
import { loadConfig } from '../utils/config.js';
import { isHostConfig } from '../utils/plugin-detection.js';
import {
printHeader,
printKV,
Expand Down Expand Up @@ -190,7 +191,10 @@ export default class Serve extends Command {
}

// 3. Auto-register AppPlugin if config contains app definitions
if (config.objects || config.manifest || config.apps) {
// Skip if config is a host/aggregator config that already contains
// instantiated plugins — wrapping it would cause duplicate registration
// and startup failures (e.g. plugin.app.dev-workspace).
if (!isHostConfig(config) && (config.objects || config.manifest || config.apps)) {
try {
const { AppPlugin } = await import('@objectstack/runtime');
await kernel.use(new AppPlugin(config));
Expand Down
16 changes: 16 additions & 0 deletions packages/cli/src/utils/plugin-detection.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.

/**
* Detect whether a loaded config is a host/aggregator configuration.
*
* A host config already contains a `plugins` array with instantiated Plugin
* objects (i.e. objects that have an `init` method). Such configs must NOT
* be wrapped again by AppPlugin in the CLI, as that would cause duplicate
* plugin registration and startup failures.
*/
export function isHostConfig(config: any): boolean {
return (
Array.isArray(config.plugins) &&
config.plugins.some((p: any) => typeof p?.init === 'function')
);
}
77 changes: 77 additions & 0 deletions packages/cli/test/serve-host-config.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { describe, it, expect } from 'vitest';
import { isHostConfig } from '../src/utils/plugin-detection';

/**
* Tests for the host config detection logic used in serve.ts
*
* When the root config is a host/aggregator (i.e. its `plugins` array
* contains already-instantiated Plugin objects), the CLI must NOT wrap
* it again with AppPlugin, as that would cause duplicate registration
* and a `plugin.app.<id> failed to start` error.
*/
describe('Host config detection', () => {

it('should detect host config with instantiated plugins', () => {
const config = {
manifest: { id: 'dev-workspace', name: 'dev_workspace' },
plugins: [
{ name: 'objectql', init: async () => {}, start: async () => {} },
{ name: 'driver', init: async () => {}, start: async () => {} },
],
};
expect(isHostConfig(config)).toBe(true);
});

it('should NOT detect pure app bundle config (no plugins)', () => {
const config = {
manifest: { id: 'my-app', name: 'my_app' },
objects: [{ name: 'task', fields: [] }],
};
expect(isHostConfig(config)).toBe(false);
});

it('should NOT detect config with empty plugins array', () => {
const config = {
manifest: { id: 'my-app', name: 'my_app' },
objects: [{ name: 'task', fields: [] }],
plugins: [],
};
expect(isHostConfig(config)).toBe(false);
});

it('should NOT detect config with string plugin references', () => {
const config = {
manifest: { id: 'my-app', name: 'my_app' },
plugins: ['@objectstack/plugin-auth', '@objectstack/objectql'],
};
expect(isHostConfig(config)).toBe(false);
});

it('should NOT detect config with plain object plugins (no init method)', () => {
const config = {
manifest: { id: 'my-app', name: 'my_app' },
plugins: [
{ name: 'some-plugin', version: '1.0.0' },
],
};
expect(isHostConfig(config)).toBe(false);
});

it('should detect if at least one plugin has init method', () => {
const config = {
manifest: { id: 'dev-workspace' },
plugins: [
{ name: 'plain-bundle', version: '1.0.0' },
{ name: 'real-plugin', init: async () => {}, start: async () => {} },
],
};
expect(isHostConfig(config)).toBe(true);
});

it('should handle config without plugins property', () => {
const config = {
manifest: { id: 'my-app', name: 'my_app' },
};
expect(isHostConfig(config)).toBe(false);
});
});
8 changes: 8 additions & 0 deletions packages/plugins/plugin-auth/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,15 @@
"license": "Apache-2.0",
"description": "Authentication & Identity Plugin for ObjectStack",
"main": "dist/index.js",
"module": "dist/index.mjs",
"types": "dist/index.d.ts",
"exports": {
".": {
"import": "./dist/index.mjs",
"require": "./dist/index.js",
"types": "./dist/index.d.ts"
Comment on lines +11 to +13

Copilot AI Mar 11, 2026

Copy link

Choose a reason for hiding this comment

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

The new conditional exports map has types after import/require. In this repo’s packages, types is consistently listed first in exports["."] (e.g. @objectstack/core, @objectstack/runtime), which avoids tooling picking the runtime entry when resolving typings. Reorder the keys to put types first to match existing package conventions.

Suggested change
"import": "./dist/index.mjs",
"require": "./dist/index.js",
"types": "./dist/index.d.ts"
"types": "./dist/index.d.ts",
"import": "./dist/index.mjs",
"require": "./dist/index.js"

Copilot uses AI. Check for mistakes.
}
},
"scripts": {
"build": "tsup --config ../../../tsup.config.ts",
"test": "vitest run",
Expand Down
36 changes: 36 additions & 0 deletions packages/runtime/src/app-plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,22 @@ describe('AppPlugin', () => {
);
});

it('start should handle getService throwing for objectql', async () => {
const bundle = { id: 'com.test.throw' };
const plugin = new AppPlugin(bundle);

vi.mocked(mockContext.getService).mockImplementation(() => {
throw new Error("[Kernel] Service 'objectql' not found");
});

await plugin.start!(mockContext);

expect(mockContext.logger.warn).toHaveBeenCalledWith(
expect.stringContaining('ObjectQL engine service not found'),
expect.any(Object)
);
});

// ═══════════════════════════════════════════════════════════════
// i18n translation auto-loading
// ═══════════════════════════════════════════════════════════════
Expand Down Expand Up @@ -173,6 +189,26 @@ describe('AppPlugin', () => {
);
});

it('should skip translation loading when getService throws for i18n', async () => {
vi.mocked(mockContext.getService).mockImplementation((name: string) => {
if (name === 'objectql') return mockQL;
throw new Error("[Kernel] Service 'i18n' not found");
});

const bundle = {
id: 'com.test.i18nthrow',
translations: [{ en: { messages: { hello: 'Hello' } } }],
};
const plugin = new AppPlugin(bundle);
await plugin.start!(mockContext);

// Should log debug but not throw
expect(mockContext.logger.debug).toHaveBeenCalledWith(
expect.stringContaining('No i18n service registered'),
expect.any(Object)
);
});

it('should handle bundle with no translations gracefully', async () => {
const bundle = { id: 'com.test.notrans' };
const plugin = new AppPlugin(bundle);
Expand Down
22 changes: 18 additions & 4 deletions packages/runtime/src/app-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,15 @@ export class AppPlugin implements Plugin {

// Execute Runtime Step
// Retrieve ObjectQL engine from services
// We cast to any/ObjectQL because ctx.getService returns unknown
const ql = ctx.getService('objectql') as any;

// ctx.getService throws when a service is not registered, so we
// must use try/catch instead of a null-check.
let ql: any;
try {
ql = ctx.getService('objectql');
} catch {
// Service not registered — handled below
Comment on lines +68 to +69

Copilot AI Mar 11, 2026

Copy link

Choose a reason for hiding this comment

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

The new try/catch around ctx.getService('objectql') swallows all getService errors. In ObjectKernel, getService deliberately re-throws non-"not found" errors (e.g. "Service 'x' is async - use await" or factory execution failures). Swallowing those here can mask real startup failures and incorrectly downgrade them to "service not found". Catch the error, only treat the specific "[Kernel] Service 'objectql' not found" case as optional, and rethrow (or log+rethrow) other errors.

Suggested change
} catch {
// Service not registered — handled below
} catch (error: unknown) {
const message = error instanceof Error ? error.message : String(error);
if (message !== "[Kernel] Service 'objectql' not found") {
// Re-throw non-"not found" errors so real startup failures are not masked
throw error;
}
// "[Kernel] Service 'objectql' not found" — handled below via !ql check

Copilot uses AI. Check for mistakes.
}

if (!ql) {
ctx.logger.warn('ObjectQL engine service not found', {
appName: this.name,
Expand Down Expand Up @@ -201,7 +207,15 @@ export class AppPlugin implements Plugin {
* this keeps AppPlugin resilient across server/dev/mock environments.
*/
private loadTranslations(ctx: PluginContext, appId: string): void {
const i18nService = ctx.getService('i18n') as II18nService | undefined;
// ctx.getService throws when a service is not registered, so we
// must use try/catch to gracefully skip when no i18n plugin is loaded.
let i18nService: II18nService | undefined;
try {
i18nService = ctx.getService('i18n') as II18nService;
} catch {
// Service not registered — handled below
Comment on lines +215 to +216

Copilot AI Mar 11, 2026

Copy link

Choose a reason for hiding this comment

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

Same issue as objectql: this try/catch around ctx.getService('i18n') currently swallows all errors, including async-service errors or factory failures that the kernel intentionally rethrows. Narrow the catch to only ignore the explicit "[Kernel] Service 'i18n' not found" case (or equivalent not-found sentinel) and rethrow other errors so real i18n startup failures aren’t silently skipped.

Suggested change
} catch {
// Service not registered — handled below
} catch (err: unknown) {
if (
err instanceof Error &&
err.message &&
err.message.includes("Service 'i18n' not found")
) {
// Service not registered — handled below
} else {
throw err;
}

Copilot uses AI. Check for mistakes.
}

if (!i18nService) {
ctx.logger.debug('[i18n] No i18n service registered; skipping translation loading', { appId });
return;
Expand Down