Fix TypeScript compilation errors in security module#491
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Rename PluginConfigValidator interface to IPluginConfigValidator to avoid conflict - Fix import paths in test files (security/ subfolder) - Fix logger.error calls to use correct signature (message, error?, meta?) - Fix globalThis.window checks using type assertions - Add missing 'certified: false' field to all PluginCapability test objects - Add type annotation for ZodIssue in formatZodErrors - Cast configSchema to any for partial() method call Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
… silent to error log level - Fix ZodError.errors to ZodError.issues for Zod 4.x compatibility - Change test log level from 'silent' to 'error' (silent not supported) - Fix getService mock type signature with generic parameter Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
6448fd0
into
copilot/scan-kernel-code-against-spec
There was a problem hiding this comment.
Pull request overview
This pull request fixes 26 TypeScript compilation errors in the packages/core/src/security/ module by addressing four main categories of issues: interface/class name collisions, Zod 4.x API changes, logger signature updates, and environment type guards.
Changes:
- Renamed
PluginConfigValidatorinterface toIPluginConfigValidatorto resolve naming conflict with the class of the same name - Updated Zod error handling from deprecated
.errorsproperty to.issuesfor Zod 4.x compatibility - Modified
logger.error()calls to use the 3-parameter signature(message, error?, meta?)instead of the 2-parameter signature - Changed environment detection from
typeof windowtotypeof (globalThis as any).windowto fix Node.js TypeScript compilation errors - Fixed test imports, log levels, and test fixture data to match current API requirements
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/security/plugin-signature-verifier.ts | Updated environment checks to use globalThis and logger calls to 3-parameter signature |
| packages/core/src/security/plugin-permission-enforcer.test.ts | Fixed relative imports, changed log level from 'silent' to 'error', added required certified field, fixed getService mock typing |
| packages/core/src/security/plugin-config-validator.ts | Updated Zod error handling from .errors to .issues, added type parameter to ZodError, fixed logger signature, added casting for partial() method |
| packages/core/src/security/plugin-config-validator.test.ts | Fixed relative imports and changed log level from 'silent' to 'error' |
| packages/core/src/plugin-loader.ts | Renamed interface from PluginConfigValidator to IPluginConfigValidator to avoid name collision, added deprecation notice |
| if (!isValid) { | ||
| const error = `Signature verification failed for plugin: ${plugin.name}`; | ||
| this.logger.error(error, { plugin: plugin.name, publisherId }); | ||
| this.logger.error(error, undefined, { plugin: plugin.name, publisherId }); |
There was a problem hiding this comment.
Inconsistent logger.error usage within this file. This call uses the 3-parameter signature with undefined as the second parameter, but other calls in the same file (lines 152, 304, 330) use the 2-parameter signature when there's no Error object. For consistency, when logging without an Error object, use the 2-parameter signature: this.logger.error(error, { plugin: plugin.name, publisherId }) instead of this.logger.error(error, undefined, { plugin: plugin.name, publisherId }).
| this.logger.error(error, undefined, { plugin: plugin.name, publisherId }); | |
| this.logger.error(error, { plugin: plugin.name, publisherId }); |
| if (this.config.strictMode) { | ||
| const error = `Plugin missing signature (strict mode): ${plugin.name}`; | ||
| this.logger.error(error, { plugin: plugin.name }); | ||
| this.logger.error(error, undefined, { plugin: plugin.name }); |
There was a problem hiding this comment.
Inconsistent logger.error usage within this file. This call uses the 3-parameter signature with undefined as the second parameter, but other calls in the same file (lines 152, 304, 330) use the 2-parameter signature when there's no Error object. For consistency, when logging without an Error object, use the 2-parameter signature: this.logger.error(error, { plugin: plugin.name }) instead of this.logger.error(error, undefined, { plugin: plugin.name }).
| this.logger.error(error, undefined, { plugin: plugin.name }); | |
| this.logger.error(error, { plugin: plugin.name }); |
| ].join('\n'); | ||
|
|
||
| this.logger.error(errorMessage, { | ||
| this.logger.error(errorMessage, undefined, { |
There was a problem hiding this comment.
Inconsistent logger.error usage within this file. This call uses the 3-parameter signature with undefined as the second parameter, but other calls in the same file (lines 152, 304, 330) use the 2-parameter signature when there's no Error object. For consistency, when logging without an Error object, use the 2-parameter signature: this.logger.error(errorMessage, { plugin: plugin.name, errors: formattedErrors }) instead of the 3-parameter version with undefined.
| this.logger.error(errorMessage, undefined, { | |
| this.logger.error(errorMessage, { |
| // Conditionally import crypto for Node.js environments | ||
| let cryptoModule: typeof import('crypto') | null = null; | ||
| if (typeof window === 'undefined') { | ||
| if (typeof (globalThis as any).window === 'undefined') { |
There was a problem hiding this comment.
The approach using typeof (globalThis as any).window is a workaround for TypeScript compilation in Node.js environments. A more idiomatic approach would be to check for Node.js using typeof process !== 'undefined' && process.versions?.node !== undefined (as used in logger.ts:27) rather than checking for the absence of window. This is more explicit about detecting the Node.js environment and avoids the type assertion.
| if (typeof (globalThis as any).window === 'undefined') { | |
| if (typeof process !== 'undefined' && process.versions?.node !== undefined) { |
| private computePluginHash(plugin: PluginMetadata): string { | ||
| // In browser environment, use SubtleCrypto | ||
| if (typeof window !== 'undefined') { | ||
| if (typeof (globalThis as any).window !== 'undefined') { |
There was a problem hiding this comment.
The approach using typeof (globalThis as any).window is inconsistent with the codebase pattern for environment detection. A more idiomatic approach would be to check for Node.js using typeof process !== 'undefined' && process.versions?.node !== undefined (as used in logger.ts:27) rather than checking for the absence of window. This is more explicit about detecting the Node.js environment and avoids the type assertion.
| ): Promise<boolean> { | ||
| // In browser environment, use SubtleCrypto | ||
| if (typeof window !== 'undefined') { | ||
| if (typeof (globalThis as any).window !== 'undefined') { |
There was a problem hiding this comment.
The approach using typeof (globalThis as any).window is inconsistent with the codebase pattern for environment detection. A more idiomatic approach would be to check for Node.js using typeof process !== 'undefined' && process.versions?.node !== undefined (as used in logger.ts:27) rather than checking for the absence of window. This is more explicit about detecting the Node.js environment and avoids the type assertion.
CI build failing with 26 TypeScript errors in
packages/core/src/security/. Root causes: interface/class name collision, Zod 4.x API changes, incorrect logger signatures, and Node.js environment type checks.Changes
Duplicate export resolution: Renamed
PluginConfigValidatorinterface toIPluginConfigValidatorinplugin-loader.ts(class with same name exported fromsecurity/)Zod 4.x compatibility: Changed
ZodError.errors→ZodError.issues, added type parameterZodError<any>for property accessLogger signature fix: Updated
logger.error(message, metadata)→logger.error(message, undefined, metadata)to match 3-parameter signature(message, error?, meta?)Environment type guards: Changed
typeof window→typeof (globalThis as any).windowto avoid Node.js compilation errorsTest fixtures:
security/subdirectory tests'silent'→'error'certified: falsefield toPluginCapabilitytest objectsgetServicemock to use generic type parameterOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.