feat: API protocol implementation plan with kernel service extensions#548
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…apping Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
… protocol, extend API routes Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds spec-level infrastructure to map ObjectStack’s API protocol namespaces to kernel services/plugins, including a dispatcher routing contract and an implementation plan document.
Changes:
- Extends
CoreServiceName(+ requirement defs) withai,i18n,ui,workflow. - Expands Discovery schemas with additional capabilities flags and route fields.
- Introduces
api/dispatcher.zod.ts(+ tests) and addscontributes.routesto plugin manifests for dispatcher routing.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/spec/src/system/core-services.zod.ts | Adds new core service identifiers and marks them optional in ServiceRequirementDef. |
| packages/spec/src/kernel/manifest.zod.ts | Adds contributes.routes for plugin-declared API prefix → service ownership. |
| packages/spec/src/api/index.ts | Exports the new dispatcher schema module. |
| packages/spec/src/api/dispatcher.zod.ts | Defines dispatcher route/config schemas and a default /api/v1/* routing table. |
| packages/spec/src/api/dispatcher.test.ts | Adds Vitest coverage for dispatcher schemas/defaults. |
| packages/spec/src/api/discovery.zod.ts | Adds new capability flags and optional routes for additional namespaces. |
| packages/spec/API_IMPLEMENTATION_PLAN.md | Provides a detailed protocol→service→REST mapping and phased implementation/ADR plan. |
| fallback: z.enum(['404', 'proxy', 'custom']).default('404') | ||
| .describe('Behavior when no route matches'), | ||
|
|
There was a problem hiding this comment.
DispatcherConfigSchema supports fallback: 'proxy', but there’s no validation that proxyTarget is present in that case (and absent otherwise). Add a .superRefine/discriminated union so invalid configs (proxy fallback without target) don’t validate.
| criticality: ServiceCriticalitySchema.default('optional') | ||
| .describe('Service criticality level for unavailability handling'), |
There was a problem hiding this comment.
ServiceCriticalitySchema is described in core-services.zod.ts as a startup-time requirement level, but here it’s reused to drive request-time HTTP behavior when a service is unavailable. Either align the semantics/docstrings, or introduce a dedicated route-level enum to avoid conflating startup criticality with runtime HTTP behavior.
| * The dispatcher is the central routing component that: | ||
| * 1. Matches incoming request URLs against registered route prefixes | ||
| * 2. Delegates to the corresponding CoreService implementation | ||
| * 3. Returns 503 Service Unavailable when a service is not registered |
There was a problem hiding this comment.
Dispatcher doc says it "Returns 503 Service Unavailable when a service is not registered", but the criticality docs indicate some cases return 500. Please reconcile this documentation so it reflects the intended behavior consistently.
| * 3. Returns 503 Service Unavailable when a service is not registered | |
| * 3. Handles service unavailability (including unregistered services) | |
| * according to the route's criticality policy |
| describe('DEFAULT_DISPATCHER_ROUTES', () => { | ||
| it('should have routes for all protocol namespaces', () => { | ||
| expect(DEFAULT_DISPATCHER_ROUTES.length).toBeGreaterThanOrEqual(14); | ||
| }); |
There was a problem hiding this comment.
Test name says it verifies routes exist for all protocol namespaces, but the assertion only checks DEFAULT_DISPATCHER_ROUTES.length >= 14, which wouldn’t catch missing namespaces (or duplicates) as long as the array is long enough. Consider asserting the exact expected set of prefixes/namespaces to make this test meaningful.
| it('should accept proxy fallback with target', () => { | ||
| const config = DispatcherConfigSchema.parse({ | ||
| routes: [], | ||
| fallback: 'proxy', | ||
| proxyTarget: 'https://api.example.com', |
There was a problem hiding this comment.
Add a negative test asserting DispatcherConfigSchema rejects fallback: 'proxy' when proxyTarget is missing (this will validate the conditional requirement logic for proxy mode).
| hub: z.boolean().default(false).describe('Is Hub management enabled?'), | ||
| ai: z.boolean().default(false).describe('Is the AI engine enabled?'), | ||
| workflow: z.boolean().default(false).describe('Is the Workflow engine enabled?'), | ||
| notifications: z.boolean().default(false).describe('Is the Notification service enabled?'), | ||
| i18n: z.boolean().default(false).describe('Is the i18n service enabled?'), |
There was a problem hiding this comment.
ApiCapabilitiesSchema adds new feature flags (ai/workflow/notifications/i18n), but current discovery tests don’t exercise these fields (defaults or parsing). Add/extend tests to cover at least one new flag’s default and one enabled case to prevent silent regressions.
| /** Base URL for Package Management */ | ||
| packages: z.string().optional().describe('e.g. /api/v1/packages'), | ||
|
|
||
| /** Base URL for Workflow Engine */ | ||
| workflow: z.string().optional().describe('e.g. /api/v1/workflow'), |
There was a problem hiding this comment.
ApiRoutesSchema adds new optional routes (packages/workflow/realtime/notifications/ai/i18n), but current discovery tests don’t cover parsing of these new keys. Add a test case that includes a couple of these new route fields so the expansion doesn’t go untested.
| /** Service name this plugin provides */ | ||
| service: z.string().describe('Service name this plugin provides'), | ||
| /** Protocol method names implemented */ |
There was a problem hiding this comment.
In contributes.routes, service is currently z.string(), which allows arbitrary values that won’t match CoreServiceName and can break dispatcher routing. Consider importing CoreServiceName from ../system/core-services.zod and using it here so plugin manifests are validated against the known service registry.
Evaluates how the 61-method ObjectStackProtocolSchema maps to kernel services and plugins, defines REST endpoints, and extends the spec with the missing infrastructure.
Development Plan (
API_IMPLEMENTATION_PLAN.md)Spec Changes
core-services.zod.ts— Addai,i18n,ui,workflowtoCoreServiceNameenum andServiceRequirementDefdiscovery.zod.ts— ExtendApiRoutesSchemawith 6 new optional route fields (packages,workflow,realtime,notifications,ai,i18n); extendApiCapabilitiesSchemawith 4 capability flagsdispatcher.zod.ts(new) —DispatcherRouteSchema/DispatcherConfigSchema/DEFAULT_DISPATCHER_ROUTESdefining how HttpDispatcher routes URL prefixes to kernel servicesmanifest.zod.ts— Addcontributes.routesso plugins can declare API route ownership:dispatcher.test.ts(new) — 18 tests covering route validation, defaults, criticality levels, and DEFAULT_DISPATCHER_ROUTES completenessAll 3099 tests pass across 98 test files.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.