feat(#4039): add boost-connector-utils shared package - #4144
feat(#4039): add boost-connector-utils shared package#4144fullsend-ai-coder[bot] wants to merge 8 commits into
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. 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
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4144 +/- ##
==========================================
+ Coverage 58.12% 58.17% +0.04%
==========================================
Files 2422 2425 +3
Lines 96493 96615 +122
Branches 26808 26836 +28
==========================================
+ Hits 56083 56201 +118
- Misses 40219 40223 +4
Partials 191 191
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 2:04 PM UTC · Completed 2:25 PM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR adds a new shared utility package for connector infrastructure in the boost workspace Previous run (2)ReviewFindingsMedium
Low
Labels: PR adds a new package under workspaces/boost/plugins/ |
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 7:39 AM UTC · Completed 7:52 AM UTC |
- Remove unreachable FetchError branch in classifyConnectorError (already covered by generic retryable-code check) - Add rejectUnauthorized: true to createHttpsAgent to guard against NODE_TLS_REJECT_UNAUTHORIZED=0 leaking from dev environments - Switch ca-bundle.ts from namespace imports to named destructured imports to match existing boost workspace conventions - Update connector-shared-infrastructure design doc, tasks, proposal, and spec files to use ai-catalog.providers config root per #4069 - Fix loadCaBundle signature in docs to include logger parameter - Fix createProviderWrapper signature in docs to use ConnectorEntityProvider type and optional ctx parameter - Update exports list in design doc to include classifyConnectorError, validateConnectorStartupConfig, ValidateConnectorStartupConfigOptions, and ConnectorEntityProvider Addresses review feedback on #4144
🔧 Fix agent — iteration 1 (human-triggered)Addressed 11 of 14 review findings: removed dead FetchError branch, added rejectUnauthorized:true TLS hardening, switched to named destructured imports, and updated all connector-shared-infrastructure design docs/specs to use ai-catalog.providers config root with correct function signatures and exports list. Disagreed with 3 findings (pluginPackages cross-ref, file naming convention, out-of-scope openspec updates). Fixed (11):
Disagreed (3):
Tests: passed Updated by fullsend fix agent |
|
/fs-fix following CI error: |
|
🤖 Finished Fix · ✅ Success · Started 8:07 AM UTC · Completed 8:21 AM UTC |
…ompat ConnectorErrorContext lacked a string index signature, making it incompatible with Backstage LoggerService's JsonObject metadata parameter. Add `[key: string]: string | boolean | undefined` so the interface satisfies the `error(message, meta?)` signature without type assertions. Regenerate API reports for boost-connector-utils (new) and boost (re-ordered by api-extractor). Addresses review feedback on #4144
🔧 Fix agent — iteration 2 (human-triggered)Fixed TS2345 compilation error by adding a string index signature to ConnectorErrorContext, making it compatible with Backstage LoggerService's JsonObject metadata type. All 47 tests pass, lint and prettier checks pass, API reports regenerated. Fixed (1):
Tests: passed Updated by fullsend fix agent |
|
/fs-fix Address SonarQube findings (per https://sonarcloud.io/project/issues?id=redhat-developer_rhdh-plugins&pullRequest=4144&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true) and rebase |
|
🤖 Finished Fix · ✅ Success · Started 10:31 AM UTC · Completed 10:47 AM UTC |
- Remove unreachable FetchError branch in classifyConnectorError (already covered by generic retryable-code check) - Add rejectUnauthorized: true to createHttpsAgent to guard against NODE_TLS_REJECT_UNAUTHORIZED=0 leaking from dev environments - Switch ca-bundle.ts from namespace imports to named destructured imports to match existing boost workspace conventions - Update connector-shared-infrastructure design doc, tasks, proposal, and spec files to use ai-catalog.providers config root per #4069 - Fix loadCaBundle signature in docs to include logger parameter - Fix createProviderWrapper signature in docs to use ConnectorEntityProvider type and optional ctx parameter - Update exports list in design doc to include classifyConnectorError, validateConnectorStartupConfig, ValidateConnectorStartupConfigOptions, and ConnectorEntityProvider Addresses review feedback on #4144
…ompat ConnectorErrorContext lacked a string index signature, making it incompatible with Backstage LoggerService's JsonObject metadata parameter. Add `[key: string]: string | boolean | undefined` so the interface satisfies the `error(message, meta?)` signature without type assertions. Regenerate API reports for boost-connector-utils (new) and boost (re-ordered by api-extractor). Addresses review feedback on #4144
🔧 Fix agent — iteration 3 (human-triggered)Addressed all 5 SonarQube findings: node: protocol imports for built-in modules (fs, https), optional chaining for credential validation, replaceAll() for string replacement, and parameterized test for retryable error codes. Also rebased onto main (resolved 2 merge conflicts in openspec docs) and regenerated API reports. Fixed (5):
Tests: passed Decision points
Updated by fullsend fix agent |
c460dad to
6937075
Compare
- Use node: protocol for built-in module imports (node:fs, node:https) - Use optional chaining for credential field empty check - Use String.replaceAll() instead of String.replace() with regex - Parameterize retryable error code tests with it.each - Regenerate API reports after rebase Addresses review feedback on #4144
|
/fs-fix address CI issues (recently for |
|
🤖 Finished Fix · ✅ Success · Started 11:04 AM UTC · Completed 11:18 AM UTC |
Run `yarn backstage-cli repo fix --publish` to add the new boost-connector-utils package to the pluginPackages array in all sibling package.json files sharing pluginId "boost", and fix alphabetical ordering in the connector-utils package itself. Addresses CI failure in `yarn fix --check --publish` on PR #4144
🔧 Fix agent — iteration 4 (human-triggered)Fixed CI failure caused by missing pluginPackages sync. The new boost-connector-utils package needed to be added to all sibling package.json pluginPackages arrays via Fixed (1):
Tests: passed Updated by fullsend fix agent |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 12:23 PM UTC · Completed 12:42 PM UTC |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix Address all fullsend-ai-review comments plus following:
|
|
🤖 Finished Fix · ✅ Success · Started 1:01 PM UTC · Completed 1:17 PM UTC |
- Reorder classifyConnectorError to check retryable/non-retryable codes on err.code and err.cause?.code before NON_RETRYABLE_TYPES so native fetch TypeErrors with network causes (ECONNREFUSED, ETIMEDOUT, etc.) are correctly classified as retryable - Add axios-shaped err.response?.status to HTTP status extraction so 429/5xx/4xx classification works with the documented HTTP client - Introduce FaultIsolationContext interface with optional nextRetryAt; wrappers pass it through to error context when retryable=true, documenting nextRetryAt as connector-owned (not computed by wrappers) - Add changeset for the new public package - Add 7 new unit tests covering native-fetch TypeError+cause, axios response.status, and nextRetryAt passthrough/omission Addresses review feedback on #4144
🔧 Fix agent — iteration 5 (human-triggered)Addressed all 4 human review findings. (1) Reordered classifyConnectorError to check err.code/err.cause?.code before NON_RETRYABLE_TYPES so native-fetch TypeErrors with network causes are retryable. (2) Introduced FaultIsolationContext with connector-owned nextRetryAt passthrough. (3) Added axios err.response?.status to HTTP status extraction. (4) Added changeset for new package. 7 new unit tests, all 54 tests passing. Fixed (4):
Tests: passed Updated by fullsend fix agent |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 1:21 PM UTC · Completed 1:38 PM UTC |
|
/fs-fix both the latest review comments and the failing CI |
|
🤖 Finished Fix · ✅ Success · Started 1:50 PM UTC · Completed 2:06 PM UTC |
- Remove dead code: unreachable value?.trim() === '' check in validateCredentialField (ConfigReader throws on empty strings first) - Fix misleading JSDoc: rewrite validateCredentialField comment to match actual non-empty validation behavior - Fix import convention: use node: prefix for fs/os/path in test file - Move ConnectorEntityProvider interface from fault-isolation.ts to types.ts, consolidating all public interfaces in one location - Align @backstage/config dependency to ^1.3.8 matching monorepo - Regenerate boost/report.api.md to fix CI api-report check Addresses review feedback on #4144
🔧 Fix agent — iteration 6 (human-triggered)Addressed 6 of 7 review findings and fixed the CI api-report failure. Fixed dead code removal, misleading JSDoc, import convention, interface location, dependency version alignment, and regenerated API reports. Disagreed with 1 finding (path traversal) as admin-controlled config consistent with Backstage patterns. Fixed (6):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
Align credential validation docs with non-empty checks, reject whitespace-only secrets, document CA trust replacement, and sync workspace discovery docs, openspec tasks, and classification tests. Signed-off-by: Marek Libra <marek.libra@gmail.com>
Create @red-hat-developer-hub/backstage-plugin-boost-connector-utils providing shared utilities for AI catalog connector entity providers: - loadCaBundle(config, logger): resolve CA bundles from tls.caFile (file path) or tls.caSecret (resolved $env), with graceful failure (WARN for missing file, ERROR for invalid PEM, no expire check) - createHttpsAgent(caBundle?): factory for https.Agent with custom CA - createProviderWrapper(provider, logger, ctx?): wrap EntityProvider connect() in try/catch to prevent unhandled rejections crashing the catalog backend process - createSafeRefresh(fn, connectorId, logger, ctx?): wrap scheduled refresh callbacks with the same fault isolation pattern - classifyConnectorError(error): classify errors as retryable (ECONNREFUSED, ETIMEDOUT, HTTP 5xx/429) or non-retryable (HTTP 4xx, TypeError, TLS cert errors) - isConnectorEnabled(config): check enabled boolean with default true - validateConnectorStartupConfig(config, opts): reject empty credentials and invalid/non-HTTPS endpoint URLs at startup Includes 47 unit tests covering CA loading (file, env, missing, invalid PEM, chains, isolation), fault isolation (connect crash, refresh crash, no rethrow, multi-provider independence, structured error logging), config guards (enabled/disabled/default), and startup validation (HTTPS URLs, credential fields, descriptive errors). Reference app-config YAML at workspaces/boost/examples/ demonstrates MCP Registry, RHOAI, and OCI Skill connector configuration with an air-gapped deployment variant. Config root: ai-catalog.providers.<id> per #4069 clarification. No Helm charts or Operator CRs (deferred per issue clarification). Closes #4039
- Remove unreachable FetchError branch in classifyConnectorError (already covered by generic retryable-code check) - Add rejectUnauthorized: true to createHttpsAgent to guard against NODE_TLS_REJECT_UNAUTHORIZED=0 leaking from dev environments - Switch ca-bundle.ts from namespace imports to named destructured imports to match existing boost workspace conventions - Update connector-shared-infrastructure design doc, tasks, proposal, and spec files to use ai-catalog.providers config root per #4069 - Fix loadCaBundle signature in docs to include logger parameter - Fix createProviderWrapper signature in docs to use ConnectorEntityProvider type and optional ctx parameter - Update exports list in design doc to include classifyConnectorError, validateConnectorStartupConfig, ValidateConnectorStartupConfigOptions, and ConnectorEntityProvider Addresses review feedback on #4144
…ompat ConnectorErrorContext lacked a string index signature, making it incompatible with Backstage LoggerService's JsonObject metadata parameter. Add `[key: string]: string | boolean | undefined` so the interface satisfies the `error(message, meta?)` signature without type assertions. Regenerate API reports for boost-connector-utils (new) and boost (re-ordered by api-extractor). Addresses review feedback on #4144
- Use node: protocol for built-in module imports (node:fs, node:https) - Use optional chaining for credential field empty check - Use String.replaceAll() instead of String.replace() with regex - Parameterize retryable error code tests with it.each - Regenerate API reports after rebase Addresses review feedback on #4144
Run `yarn backstage-cli repo fix --publish` to add the new boost-connector-utils package to the pluginPackages array in all sibling package.json files sharing pluginId "boost", and fix alphabetical ordering in the connector-utils package itself. Addresses CI failure in `yarn fix --check --publish` on PR #4144
- Reorder classifyConnectorError to check retryable/non-retryable codes on err.code and err.cause?.code before NON_RETRYABLE_TYPES so native fetch TypeErrors with network causes (ECONNREFUSED, ETIMEDOUT, etc.) are correctly classified as retryable - Add axios-shaped err.response?.status to HTTP status extraction so 429/5xx/4xx classification works with the documented HTTP client - Introduce FaultIsolationContext interface with optional nextRetryAt; wrappers pass it through to error context when retryable=true, documenting nextRetryAt as connector-owned (not computed by wrappers) - Add changeset for the new public package - Add 7 new unit tests covering native-fetch TypeError+cause, axios response.status, and nextRetryAt passthrough/omission Addresses review feedback on #4144
- Remove dead code: unreachable value?.trim() === '' check in validateCredentialField (ConfigReader throws on empty strings first) - Fix misleading JSDoc: rewrite validateCredentialField comment to match actual non-empty validation behavior - Fix import convention: use node: prefix for fs/os/path in test file - Move ConnectorEntityProvider interface from fault-isolation.ts to types.ts, consolidating all public interfaces in one location - Align @backstage/config dependency to ^1.3.8 matching monorepo - Regenerate boost/report.api.md to fix CI api-report check Addresses review feedback on #4144
Align credential validation docs with non-empty checks, reject whitespace-only secrets, document CA trust replacement, and sync workspace discovery docs, openspec tasks, and classification tests. Signed-off-by: Marek Libra <marek.libra@gmail.com>
157cf58 to
aeaf43b
Compare
|
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
Finding 1 — isValidPem only checks the PEM header, not the footer
isValidPem() checks for -----BEGIN CERTIFICATE----- but not -----END CERTIFICATE-----. A truncated PEM file (e.g., partial write from a failed mount, interrupted download) would pass validation and then fail at TLS handshake time with a cryptic OpenSSL error. Adding a footer check would catch corrupted files early with a clear log message.
Finding 2 — Export a safeGetOptionalString utility and document the ConfigReader edge case in AGENTS.md
Backstage's ConfigReader.getOptionalString() throws TypeError on empty-string values from env var substitution like ${VAR:-} rather than returning undefined. The ai-integrations workspace already hit this and has a safeGetOptionalString wrapper in kserve-kubeflow-connector-backend/src/plugin.ts plus documentation in its AGENTS.md. Since boost-connector-utils is the shared utility package for all AI catalog connectors, this is the natural home for a reusable
export. Additionally, workspaces/boost/AGENTS.md should document this edge case the same way ai-integrations/AGENTS.md does, so future contributors (human and AI) don't rediscover it.
Finding 3 — Remove mcpRegistry from ai-catalog.providers examples; MCP Registry lives under RHDHPLAN-393
MCP Registry has its own config namespace under RHDHPLAN-393 (generic MCP registry entity provider), independent of the ai-catalog.providers hierarchy. The mcpRegistry block under ai-catalog.providers in the example YAML and README code samples should be removed. No TypeScript source changes are needed — the .ts files only use 'mcpRegistry' as arbitrary test fixture names and JSDoc examples, which are fine. The rhoai.mcpCatalog and ociSkill stanzas should
stay for now, as those connectors may be implemented as part of the boost workspace. The openspec files under connector-shared-infrastructure/ and mcp-registry-connector/ also reference ai-catalog.providers.mcpRegistry and need the same correction as a follow-up.
| * @internal | ||
| */ | ||
| function isValidPem(content: Buffer): boolean { | ||
| return content.toString('utf-8').includes(PEM_HEADER); |
There was a problem hiding this comment.
isValidPem checks for the PEM header (-----BEGIN CERTIFICATE-----) but not the footer (-----END CERTIFICATE-----). A truncated PEM file would pass this check and fail later at TLS handshake with a cryptic OpenSSL error. Consider adding a footer check to catch corrupted CA files early.
| // descriptive message pointing the deployer to $env usage. | ||
| let value: string | undefined; | ||
| try { | ||
| value = config.getOptionalString(field); |
There was a problem hiding this comment.
The getOptionalString() call is wrapped in try-catch here for validation, but downstream connectors will need this same pattern for general config reading. ConfigReader throws TypeError on empty-string values from env var substitution (${VAR:-}). Consider exporting a safeGetOptionalString(config, key) utility from this package — the ai-integrations workspace already has this pattern:
/**
* Safely read an optional string from a Backstage Config object.
* ConfigReader throws TypeError for empty-string values from
* env var substitution like ${VAR:-}, so we catch and return undefined.
*/
function safeGetOptionalString(cfg: Config, key: string): string | undefined {
try {
return cfg.getOptionalString(key);
} catch {
// ConfigReader throws TypeError for empty-string values
// from env var substitution like ${VAR:-}
return undefined;
}
}
and documents it as a key convention in its AGENTS.md
| | `boost-backend-module-ogx` | OGX provider module | | ||
| | `boost-backend-module-kagenti` | Kagenti provider module | | ||
| | `ogx-entity-provider` | Independently deployable catalog entity provider | | ||
| | `kagenti-entity-provider` | Independently deployable catalog entity provider | |
There was a problem hiding this comment.
| | `kagenti-entity-provider` | Independently deployable catalog entity provider | | |
| | `kagenti-entity-provider` | Independently deployable catalog entity provider | |
| ### ConfigReader `getOptionalString()` edge case | |
| Backstage's `ConfigReader` throws `TypeError` when the underlying config | |
| value is an empty string (e.g., from env var substitution like | |
| `${UNSET_ENV_VAR:-}`), rather than returning `undefined`. When reading | |
| config values that may come from environment variable substitution, wrap | |
| calls in a try-catch that returns `undefined` (or a default) on | |
| `TypeError`: | |
| ```ts | |
| function safeGetOptionalString( | |
| config: Config, | |
| key: string, | |
| ): string | undefined { | |
| try { | |
| return config.getOptionalString(key); | |
| } catch { | |
| // ConfigReader throws TypeError for empty-string values | |
| // from env var substitution like ${VAR:-} | |
| return undefined; | |
| } | |
| } |
| ai-catalog: | ||
| providers: | ||
| # --- MCP Registry Connector --- | ||
| mcpRegistry: |
There was a problem hiding this comment.
The mcpRegistry block should be removed from ai-catalog.providers — MCP Registry lives under RHDHPLAN-393 with its own independent config namespace. No TypeScript source changes are needed for this — .ts files only use 'mcpRegistry' as test fixture names and JSDoc examples. The rhoai.mcpCatalog and ociSkill stanzas should stay, as those connectors may be implemented as part of boost. The openspec files under connector-shared-infrastructure/ (proposal.md:14, design.md:79/96/201, tasks.md:9, specs/ca-bundle-resolution/spec.md:31) and mcp-registry-connector/ (proposal.md:29, design.md:98/161, tasks.md:13/37/38/43) also reference ai-catalog.providers.mcpRegistry and need the same correction.
| } from '@red-hat-developer-hub/backstage-plugin-boost-connector-utils'; | ||
|
|
||
| // In your connector's init(): | ||
| const connectorConfig = config.getConfig('ai-catalog.providers.mcpRegistry'); |
There was a problem hiding this comment.
This ai-catalog.providers.mcpRegistry config path should be replaced with an RHOAI or OCI example — MCP Registry config is independent of the ai-catalog.providers namespace (RHDHPLAN-393). Also affects lines 42, 131, and 158 in this file.
2 participants
Create @red-hat-developer-hub/backstage-plugin-boost-connector-utils providing shared utilities for AI catalog connector entity providers:
(file path) or tls.caSecret (resolved $env), with graceful failure
(WARN for missing file, ERROR for invalid PEM, no expire check)
connect() in try/catch to prevent unhandled rejections crashing the
catalog backend process
refresh callbacks with the same fault isolation pattern
(ECONNREFUSED, ETIMEDOUT, HTTP 5xx/429) or non-retryable (HTTP
4xx, TypeError, TLS cert errors)
credentials and invalid/non-HTTPS endpoint URLs at startup
Includes 47 unit tests covering CA loading (file, env, missing, invalid PEM, chains, isolation), fault isolation (connect crash, refresh crash, no rethrow, multi-provider independence, structured error logging), config guards (enabled/disabled/default), and startup validation (HTTPS URLs, credential fields, descriptive errors).
Reference app-config YAML at workspaces/boost/examples/ demonstrates MCP Registry, RHOAI, and OCI Skill connector configuration with an air-gapped deployment variant.
Config root: ai-catalog.providers. per #4069 clarification. No Helm charts or Operator CRs (deferred per issue clarification).
Closes #4039
Post-script verification
agent/4039-connector-utils)fe4298b658c7868ca806e4edaf082e0417ba7cb2..HEAD)