feat(plugins): report setup descriptor drift#71194
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🔵 Potential CPU/memory exhaustion from quadratic drift-diagnostics checks in setup registry resolution
Description
Vulnerable code (quadratic checks for providers): for (const declaredId of declaredProviderIds) {
if (!params.providers.some((provider) => matchesProvider(provider, declaredId))) {
params.diagnostics.push({ ... });
}
}
for (const provider of params.providers) {
if (!declaredProviderIds.some((declaredId) => matchesProvider(provider, declaredId))) {
params.diagnostics.push({ ... });
}
}RecommendationAvoid quadratic checks and cap diagnostic growth.
Example (providers) using sets: const declared = new Set(declaredProviderIds.map((id) => normalizeProviderId(id)).filter(Boolean));
const runtime = new Set(params.providers.map((p) => normalizeProviderId(p.id)).filter(Boolean));
for (const id of declared) {
if (!runtime.has(id)) {
// push diagnostic
}
}
for (const id of runtime) {
if (!declared.has(id)) {
// push diagnostic
}
}This reduces CPU usage and makes registry building resilient against pathological plugin inputs. Analyzed PR: #71194 at commit Last updated on: 2026-04-24T18:16:46Z |
Greptile SummaryThis PR adds additive drift diagnostics to the setup registry, emitting structured Confidence Score: 5/5Safe to merge — all findings are P2 (no enforcement change, no breaking contract). The implementation is correct: provider drift detection properly uses matchesProvider (honours aliases), CLI backend detection uses normalized-ID maps, and diagnostics are scoped to plugins with explicit descriptors. The two P2 findings are a partial-throw edge case in the diagnostic surface (low risk for a non-enforcing feature) and a missing no-drift test case for setup.providers. Neither blocks merge. src/plugins/setup-registry.test.ts — the no-drift path for setup.providers is not covered. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/plugins/setup-registry.ts
Line: 563-572
Comment:
**Partial registration silently skips drift diagnostics**
If `setupRegistration.register(api)` throws mid-execution, any providers or CLI backends already pushed into `recordProviders`/`recordCliBackends` (and the global `providers`/`cliBackends` lists via the handler closures) are retained in the registry, but the `catch { continue }` branch skips `pushSetupDescriptorDriftDiagnostics`. A plugin that throws half-way will have partial registrations visible to callers with zero drift diagnostics reported, making the diagnostic surface incomplete for that plugin. Given that this is a diagnostic-only feature (no enforcement), the risk is low — but callers who rely on diagnostics to audit descriptor drift may get a false-clean signal for a misbehaving plugin.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/plugins/setup-registry.test.ts
Line: 457-461
Comment:
**No-drift path for `setup.providers` is untested**
The "does not report drift" test uses `mockOpenAiCliBackendRegistration`, which never sets `setup.providers` in the manifest — so `declaredProviderIds` is always `undefined` and the provider-matching branch in `pushSetupDescriptorDriftDiagnostics` is never exercised on the no-drift path. A regression in `matchesProvider` that produces false-positive drift for correctly-matching providers would pass this test suite undetected. A second no-drift fixture that declares `setup.providers: [{ id: "openai" }]` and registers a provider with `id: "openai"` would close this gap.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(plugins): report setup descriptor d..." | Re-trigger Greptile |
| } catch { | ||
| continue; | ||
| } | ||
| pushSetupDescriptorDriftDiagnostics({ | ||
| record, | ||
| providers: recordProviders, | ||
| cliBackends: recordCliBackends, | ||
| diagnostics, | ||
| }); | ||
| } |
There was a problem hiding this comment.
If setupRegistration.register(api) throws mid-execution, any providers or CLI backends already pushed into recordProviders/recordCliBackends (and the global providers/cliBackends lists via the handler closures) are retained in the registry, but the catch { continue } branch skips pushSetupDescriptorDriftDiagnostics. A plugin that throws half-way will have partial registrations visible to callers with zero drift diagnostics reported, making the diagnostic surface incomplete for that plugin. Given that this is a diagnostic-only feature (no enforcement), the risk is low — but callers who rely on diagnostics to audit descriptor drift may get a false-clean signal for a misbehaving plugin.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/setup-registry.ts
Line: 563-572
Comment:
**Partial registration silently skips drift diagnostics**
If `setupRegistration.register(api)` throws mid-execution, any providers or CLI backends already pushed into `recordProviders`/`recordCliBackends` (and the global `providers`/`cliBackends` lists via the handler closures) are retained in the registry, but the `catch { continue }` branch skips `pushSetupDescriptorDriftDiagnostics`. A plugin that throws half-way will have partial registrations visible to callers with zero drift diagnostics reported, making the diagnostic surface incomplete for that plugin. Given that this is a diagnostic-only feature (no enforcement), the risk is low — but callers who rely on diagnostics to audit descriptor drift may get a false-clean signal for a misbehaving plugin.
How can I resolve this? If you propose a fix, please make it concise.| expect(resolvePluginSetupRegistry({ env: {} }).diagnostics).toEqual([]); | ||
| }); | ||
|
|
||
| it("does not load setup-api modules from the current working directory", () => { | ||
| const pluginRoot = makeTempDir(); |
There was a problem hiding this comment.
setup.providers is untested
The "does not report drift" test uses mockOpenAiCliBackendRegistration, which never sets setup.providers in the manifest — so declaredProviderIds is always undefined and the provider-matching branch in pushSetupDescriptorDriftDiagnostics is never exercised on the no-drift path. A regression in matchesProvider that produces false-positive drift for correctly-matching providers would pass this test suite undetected. A second no-drift fixture that declares setup.providers: [{ id: "openai" }] and registers a provider with id: "openai" would close this gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/setup-registry.test.ts
Line: 457-461
Comment:
**No-drift path for `setup.providers` is untested**
The "does not report drift" test uses `mockOpenAiCliBackendRegistration`, which never sets `setup.providers` in the manifest — so `declaredProviderIds` is always `undefined` and the provider-matching branch in `pushSetupDescriptorDriftDiagnostics` is never exercised on the no-drift path. A regression in `matchesProvider` that produces false-positive drift for correctly-matching providers would pass this test suite undetected. A second no-drift fixture that declares `setup.providers: [{ id: "openai" }]` and registers a provider with `id: "openai"` would close this gap.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
Regression Test Plan (if applicable)
src/plugins/setup-registry.test.tsUser-visible / Behavior Changes
Plugin setup registry results now include diagnostics when explicit
setup.providersorsetup.cliBackendsdescriptors disagree with setup-api runtime registrations.Diagram (if applicable)
N/A
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation: N/ARepro + Verification
Environment
Steps
pnpm test src/plugins/setup-registry.test.ts.pnpm check:changed.Expected
Actual
pnpm check:changedpassed: 101 files, 1190 tests.Evidence
Human Verification (required)
Review Conversations
Compatibility / Migration
Yes)No)No)Risks and Mitigations