feat(#3298): scaffold boost-backend with ProviderManager, extension point, and security middleware#3466
feat(#3298): scaffold boost-backend with ProviderManager, extension point, and security middleware#3466fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
fullsend-ai-coder
Bot
requested review from
a team,
durandom and
gabemontero
as code owners
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
|
|
🤖 Finished Review · ✅ Success · Started 5:14 AM UTC · Completed 5:31 AM UTC |
ReviewFindingsHigh
Medium
Low
Info
Previous runReviewFindingsCritical
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| * Install this alongside the plugin in the backend: | ||
| * | ||
| * ```ts | ||
| * backend.add(import('@red-hat-developer-hub/backstage-plugin-boost-backend')); |
There was a problem hiding this comment.
[critical] logic-error
The service factory's factory() method calls providerManager.getActiveProvider() eagerly at factory creation time and returns its result directly. It will throw 'No AI provider is registered' if no provider module has registered yet. Moreover, the factory returns the AgenticProvider object directly (not a lazy proxy), so switching providers at runtime via switchProvider() will not be reflected — consumers hold a stale reference.
Suggested fix: Return a lazy proxy that delegates to providerManager.getActiveProvider() on each call, or return the ProviderManager itself (updating the service ref type accordingly).
| * Returns the resource metadata needed for conditional permission checks, | ||
| * or `undefined` if the resource is not found. | ||
| * | ||
| * @public |
There was a problem hiding this comment.
[medium] api-contract
authorizeLifecycleAction accepts BasicPermission but several boost-common permissions are ResourcePermission types. The TypeScript type constraint prevents passing resource-scoped permissions. The JSDoc notes this is intentional scaffolding, but when ResourcePermission support is added, widening the parameter type will be a public API change.
Suggested fix: Widen the parameter type to accept Permission now to avoid a future breaking change, or add a runtime check that rejects resource-scoped permissions with a clear error.
| } | ||
|
|
||
| if (!mode) { | ||
| return 'development-only-no-auth'; |
There was a problem hiding this comment.
[medium] fail-open
validateSecurityMode() defaults to 'development-only-no-auth' when no config is provided — a fail-open default. While the mode is currently not wired to any auth gate (authorizeLifecycleAction always runs permission checks regardless), the default creates risk for when the mode is eventually consumed. The production warning is advisory-only and does not block startup.
Suggested fix: Change the default to 'full' (fail-closed). If dev-no-auth must be supported, require explicit config and refuse to start in production unless mode is 'plugin-only' or 'full'.
| permissions: coreServices.permissions, | ||
| permissionsRegistry: coreServices.permissionsRegistry, | ||
| }, | ||
| async init({ |
There was a problem hiding this comment.
[medium] scope-alignment
securityMode is computed from config and logged but never passed to middleware or used to gate authorization. The value is dead code in the current implementation. When a future developer wires this up, the fail-open default becomes a real concern.
Suggested fix: Either remove 'development-only-no-auth' mode or enforce it: throw error (not just warn) when NODE_ENV=production. Document that securityMode wiring is deferred to a future PR.
| permissions: _permissions, | ||
| permissionsRegistry, | ||
| }) { | ||
| logger.info('Initializing boost backend plugin'); |
There was a problem hiding this comment.
[medium] permission-expansion
Plugin registers 23 permissions and passes them to createPermissionIntegrationRouter but does not register any permission rules or resource types. Without rule registration, conditional policies for resource permissions will fail to evaluate. The framework likely fails closed (DENY) but this should be confirmed.
Suggested fix: Add a TODO comment noting that rules and resourcePermissions must be registered when conditional permission evaluation is implemented. Confirm the framework fails closed when rules are missing.
| "type": "git", | ||
| "url": "git+https://github.com/redhat-developer/rhdh-plugins.git", | ||
| "directory": "workspaces/boost/plugins/boost-backend" | ||
| }, |
There was a problem hiding this comment.
[low] pattern-inconsistency
test script adds --passWithNoTests --coverage flags while sibling packages use bare 'backstage-cli package test'.
Suggested fix: Align with sibling convention.
| "dist" | ||
| ], | ||
| "repository": { | ||
| "type": "git", |
There was a problem hiding this comment.
[low] pattern-inconsistency
lint:check, lint:fix, tsc, and start scripts not present in sibling packages. Backend plugins may legitimately need start, but lint:check/lint:fix are redundant with lint.
Suggested fix: Remove redundant scripts or add to all siblings for consistency.
| "dependencies": { | ||
| "@backstage/backend-plugin-api": "^1.9.1", | ||
| "@backstage/errors": "^1.2.7", | ||
| "@backstage/plugin-permission-common": "^0.9.7", |
There was a problem hiding this comment.
[low] pattern-inconsistency
exports and typesVersions fields present but absent in sibling packages.
Suggested fix: Remove or add to siblings for consistency.
| private activeProvider: AgenticProvider | undefined; | ||
| private readonly providers = new Map<string, AgenticProvider>(); | ||
|
|
||
| /** |
There was a problem hiding this comment.
[low] architectural-conflict
ProviderManager uses raw Map while AGENTS.md says not to create raw Map caches. However, this Map holds live provider instances (non-serializable), not cached data — the rule does not apply.
Suggested fix: Add comment explaining why this Map is not subject to the cacheService rule.
| * | ||
| * @public | ||
| */ | ||
| export type ResourceLoader = (req: Request) => Promise< |
There was a problem hiding this comment.
[low] api-design
ResourceLoader returns loosely typed { createdBy?: string; lifecycleStage?: string } not backed by any shared type in boost-common. Acceptable for placeholder scaffolding.
Suggested fix: Define shared interface in boost-common when resource loaders are implemented.
…oint, and security middleware Implements the core boost-backend plugin package: - ProviderManager: manages registered AI providers with hot-swap support (registerProvider, getActiveProvider, switchProvider) - boostProviderExtensionPoint: extension point in boost-node for provider modules to register their implementations - boostAiProviderServiceFactory: default service factory resolving to the active provider via ProviderManager - authorizeLifecycleAction: Express middleware implementing fine-grained permission check -> boost.admin fallback -> 403 pattern - validateSecurityMode: rejects 'none' with clear error, warns when development-only-no-auth is used in production (NODE_ENV=production) - Permission registration: all 23 boost permissions registered via permissionsRegistry.addPermissions() - Resource loader placeholders for agents and tools (store integration deferred to later issues) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gabemontero
force-pushed
the
feat/3298-boost-backend-scaffold
branch
from
5eaeee2 to
be2f52a
Compare
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
|
🤖 Finished Review · ✅ Success · Started 1:40 PM UTC · Completed 1:55 PM UTC |
| * Resolves to the active provider from the `ProviderManager`. | ||
| * Install this alongside the plugin in the backend: | ||
| * | ||
| * ```ts |
There was a problem hiding this comment.
[high] logic-error
The boostAiProviderServiceFactory calls providerManager.getActiveProvider() eagerly inside factory(), which executes once at service-resolution time. If no provider module has registered yet when the factory resolves, getActiveProvider() throws. The factory returns a fixed reference — subsequent switchProvider() calls won't affect consumers that already resolved the service.
Suggested fix: Change the factory to return a lazy wrapper/proxy that delegates chat, chatStream, and descriptor to providerManager.getActiveProvider() at invocation time rather than resolution time.
| */ | ||
| export function validateSecurityMode( | ||
| mode: string | undefined, | ||
| logger: LoggerService, |
There was a problem hiding this comment.
[medium] fail-open
When boost.security.mode is absent from configuration, validateSecurityMode defaults to 'development-only-no-auth'. The security mode is currently validated and logged but not consumed by downstream logic, making this a latent design concern rather than an active vulnerability. However, defaulting to the least restrictive mode is a fail-open pattern.
Suggested fix: Default to the most restrictive mode ('full') when no mode is configured, or require explicit opt-in for development-only-no-auth.
| boostProviderExtensionPoint, | ||
| } from '@red-hat-developer-hub/backstage-plugin-boost-node'; | ||
| import { Router } from 'express'; | ||
| import { ProviderManager } from './provider/ProviderManager'; |
There was a problem hiding this comment.
[medium] api-contract
Module-level singleton ProviderManager couples the factory to the plugin module's import graph. If imported from different module instances, they'd operate on different ProviderManager instances. Deviates from Backstage DI patterns where state is created inside register().
Suggested fix: Move new ProviderManager() inside the register() function body and wire through Backstage's DI.
| } | ||
|
|
||
| // Set up HTTP routes | ||
| const router = Router(); |
There was a problem hiding this comment.
[low] security-mode-unused
The validated securityMode value is computed and logged but never consumed by any downstream logic. Expected for a scaffold PR.
| return async (req: Request, _res: Response, next: NextFunction) => { | ||
| try { | ||
| const credentials = await httpAuth.credentials(req); | ||
|
|
There was a problem hiding this comment.
[low] auth-bypass
The authorizeLifecycleAction middleware falls back to boost.admin when a fine-grained permission is denied. While documented as intentional, admin holders implicitly bypass all fine-grained permissions including future separation-of-duties controls.
| register(env) { | ||
| // Register the extension point so provider modules can register | ||
| env.registerExtensionPoint(boostProviderExtensionPoint, { | ||
| registerProvider(provider) { |
There was a problem hiding this comment.
[low] unused-dependency
httpAuth declared in deps but not destructured; permissions destructured as _permissions but unused.
| * the active provider automatically. | ||
| */ | ||
| registerProvider(provider: AgenticProvider): void { | ||
| const id = provider.descriptor.id; |
There was a problem hiding this comment.
[low] race-condition
No synchronization in ProviderManager, though safe in Node.js single-threaded synchronous context.
| getActiveProvider(): AgenticProvider { | ||
| if (!this.activeProvider) { | ||
| throw new Error( | ||
| 'No AI provider is registered. Install a provider module ' + |
There was a problem hiding this comment.
[low] error-handling-pattern
Uses plain Error instead of @backstage/errors classes (NotFoundError, ConflictError), unlike security.ts which uses NotAllowedError.
| * @packageDocumentation | ||
| */ | ||
|
|
||
| export { |
There was a problem hiding this comment.
[low] export-style
Exports more items than typical for backend plugins in this repo. Exporting ProviderManager as a public class creates a larger contract surface.
| * limitations under the License. | ||
| */ | ||
|
|
||
| import type { Request, Response, NextFunction, RequestHandler } from 'express'; |
There was a problem hiding this comment.
[low] type-safety
authorizeLifecycleAction accepts BasicPermission but some boost permissions are ResourcePermission. TypeScript prevents misuse at compile time.
Implements the core boost-backend plugin package:
(registerProvider, getActiveProvider, switchProvider)
modules to register their implementations
active provider via ProviderManager
permission check -> boost.admin fallback -> 403 pattern
development-only-no-auth is used in production (NODE_ENV=production)
permissionsRegistry.addPermissions()
deferred to later issues)
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Closes #3298
Post-script verification
feat/3298-boost-backend-scaffold)3036a95068caf62a3ac06568b0d21dbc25322ac9..HEAD)