Skip to content

Enable easy strict flags in test tsconfig#1559

Draft
JamieMagee wants to merge 4 commits into
masterfrom
strict/easy-flags
Draft

Enable easy strict flags in test tsconfig#1559
JamieMagee wants to merge 4 commits into
masterfrom
strict/easy-flags

Conversation

@JamieMagee
Copy link
Copy Markdown
Contributor

Enables strictFunctionTypes, noImplicitThis, alwaysStrict, strictBindCallApply, and useUnknownInCatchVariables in test/tsconfig.json. Fixes the 19 resulting errors.

Enable strictFunctionTypes, noImplicitThis, alwaysStrict,
strictBindCallApply, and useUnknownInCatchVariables. Fix 19 errors.
Add non-null assertions to test files for:
- EntityCoordinates.fromString() return values in setup functions
- summary.described, summary.licensed, summary.files property access
- Nested properties: sourceLocation, urls, attributions, natures
Fix ~240 null-safety issues across providers, business, routes,
middleware, lib, and bin.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Enables several additional TypeScript strictness flags for the test build and updates tests and supporting runtime code to compile cleanly under the stricter settings.

Changes:

  • Enabled strictFunctionTypes, noImplicitThis, alwaysStrict, strictBindCallApply, and useUnknownInCatchVariables in test/tsconfig.json.
  • Updated test suites to satisfy stricter typing (notably catch error typing and nullable fields).
  • Tightened typings across providers/business code (type guards, return type adjustments, and non-null assertions) to resolve the resulting compilation errors.
Show a summary per file
File Description
test/tsconfig.json Enables additional strict compiler flags for tests.
test/summary/scancode.ts Adds non-null assertions for nullable summary fields in tests.
test/summary/reuse.ts Casts catch error to Error for useUnknownInCatchVariables.
test/summary/licensee.ts Casts catch error to Error for useUnknownInCatchVariables.
test/summary/fossology.ts Adds non-null assertion to EntityCoordinates.fromString() result in tests.
test/summary/clearlyDefined.ts Adds non-null assertions for nullable summary fields and coordinates in tests.
test/providers/summary/scancode/new-summarizer.mts Casts catch error to Error.
test/providers/summary/scancode/legacy-summarizer.mts Casts catch error to Error.
test/providers/store/mongoDefinition.ts Casts catch error to Error.
test/providers/store/fileHarvest.ts Casts catch error to Error.
test/providers/store/fileDefintion.ts Casts catch error to Error.
test/providers/store/fileAttachment.ts Casts catch error to Error.
test/providers/store/azblobDefinition.ts Casts catch error to Error.
test/providers/store/azblobAttachment.ts Casts catch error to Error.
test/providers/store/abstractFileStore.ts Casts catch error to Error.
test/providers/caching/redis.ts Casts catch error to Error.
test/lib/util.ts Adjusts types/non-null assertions to satisfy stricter checks.
test/lib/spdx.ts Adds casts for null test cases under stricter typing.
test/lib/licenseMatcher.ts Adds non-null assertions for coordinates parsing in tests.
test/lib/curation.ts Casts YAML-loaded value to match constructor input type.
test/lib/coordinatesMapper.ts Adds non-null assertions for mapper results/coordinates parsing in tests.
routes/auth.ts Adds type guards/non-null assertions for stricter nullability typing.
providers/upgrade/recomputeHandler.ts Adds explicit cast on validate() return to satisfy stricter typing.
providers/upgrade/defVersionCheck.ts Adds non-null assertions and adjusts getCoordinates signature to allow null.
providers/upgrade/azureQueueConfig.ts Adds non-null assertions around config-derived connection strings.
providers/summary/scancode/new-summarizer.ts Tightens null handling and normalizer return values under strict typing.
providers/summary/scancode/legacy-summarizer.ts Tightens null handling and normalizer return values under strict typing.
providers/summary/scancode.ts Adds non-null assertion when constructing summarizer via factory.
providers/summary/reuse.ts Adjusts metadata/license normalization typing for strict mode.
providers/summary/licensee.ts Replaces lodash get() usage with optional chaining and normalizes license typing.
providers/summary/fossology.ts Non-null assertion for options assignment under strict mode.
providers/summary/clearlydefined.ts Adds null/undefined-aware normalization and stricter provider indexing.
providers/stores/mongoConfig.ts Adds explicit option typing/casts for strict mode.
providers/stores/mongo.ts Aligns return types with DefinitionStore interface; adds non-null assertions around files.
providers/stores/fileHarvestStore.ts Adds non-null assertions for URN parsing and tool/version indexing.
providers/stores/dispatchDefinitionStore.ts Adjusts list/find typings with casts to satisfy strict checks.
providers/stores/azblobHarvestStore.ts Adds non-null assertion for URN parsing.
providers/stores/azblobDefinitionStore.ts Adds non-null assertion for coordinate parsing and adds find override to satisfy interface.
providers/stores/azblobConfig.ts Adds explicit option typing/casts for strict mode.
providers/stores/abstractMongoDefinitionStore.ts Adds non-null assertion for fromObject() result.
providers/stores/abstractFileStore.ts Adds type-predicate filtering and non-null assertion for result coordinates parsing.
providers/search/memory.ts Adds non-null assertion for EntityCoordinates.fromObject() result.
providers/search/azureConfig.ts Adds non-null assertions around required config values.
providers/search/abstractSearch.ts Ensures reductions handle missing arrays with `
providers/queueing/memoryQueue.ts Tightens queue message typing and non-null assertion on message text.
providers/queueing/azureStorageQueue.ts Tightens queue message typing and fixes dequeueMultiple() return shaping.
providers/logging/winstonConfig.ts Adds non-null assertions for AppInsights client usage under stricter typing.
providers/harvest/throttling/listBasedFilter.ts Adds type predicates and non-null assertions for strict typing.
providers/harvest/crawlerQueueConfig.ts Adds non-null assertions for config values and casts options for strict mode.
providers/harvest/crawlerConfig.ts Adds non-null assertions for required config values and casts options for strict mode.
providers/harvest/azureQueueConfig.ts Adds non-null assertions for required connection string.
providers/curation/mongoCurationStore.ts Adds non-null assertions for curation data fields and coordinates conversions.
providers/curation/mongoConfig.ts Adds non-null assertion for required connection string.
providers/curation/memoryStore.ts Adds non-null assertions for curation data/coordinates.
providers/curation/githubConfig.ts Tightens options typing and uses non-null assertion for required token.
providers/curation/github.ts Adds numerous non-null assertions and typed filters/initializers under strict mode.
providers/curation/azureQueueConfig.ts Adds non-null assertion for required connection string.
providers/caching/redisConfig.ts Adds non-null assertions for required Redis config values.
middleware/githubConfig.ts Adds non-null assertions for required GitHub token/team config entries.
middleware/github.ts Adds non-null assertions for options/cache usage and annotates return types.
lib/licenseMatcher.ts Handles potentially-null file lists and adds non-null assertions for coordinates/type/tool version.
business/suggestionService.ts Uses a cast to force namespace = null for strict typing.
business/definitionService.ts Adds non-null assertions and tighter filtering/coordinate parsing under strict mode.
bin/config.ts Adds non-null assertions for rate-limit config parsing.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

providers/stores/dispatchDefinitionStore.ts:76

  • find() is declared to return Promise<DefinitionFindResult>, but _performInSequence() can return null (e.g., if all stores fail/throw or a store returns null). The as Promise<DefinitionFindResult> cast masks this and can cause runtime failures when callers expect { data: ... }. Consider ensuring find() always returns a valid DefinitionFindResult (e.g., { data: [], continuationToken: undefined }) or throws when no underlying store can satisfy the operation.
  find(query: DefinitionFindQuery, continuationToken = ''): Promise<DefinitionFindResult> {
    return this._performInSequence(store => store.find(query, continuationToken)) as Promise<DefinitionFindResult>
  }
  • Files reviewed: 63/64 changed files
  • Comments generated: 5

Comment on lines +78 to 79
if (!harvested?.reuse?.metadata!.CreatorTool) {
throw new Error('Invalid REUSE data')
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

harvested?.reuse?.metadata!.CreatorTool can throw a TypeError when reuse.metadata is missing, preventing the intended Invalid REUSE data error from being thrown. Use optional chaining for metadata (or explicitly check harvested.reuse?.metadata before reading CreatorTool) so invalid inputs fail gracefully with the expected error.

Copilot uses AI. Check for mistakes.
Comment on lines +53 to 55
list(coordinates: EntityCoordinates): Promise<string[]> {
return this._performInSequence(store => store.list(coordinates)) as Promise<string[]>
}
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list() is typed to always return Promise<string[]>, but _performInSequence() can return null if all stores throw or return falsy results; the as Promise<string[]> cast hides that. This can lead to runtime errors when callers assume an array. Consider making _performInSequence accept a required default value (e.g., [] for list) or have list() explicitly fall back to an empty array (or throw) instead of returning null.

This issue also appears on line 74 of the same file.

Copilot uses AI. Check for mistakes.
Comment on lines +68 to +70
// @ts-expect-error - AzBlob store does not support find; satisfies DefinitionStore interface
override async find(_query: DefinitionFindQuery, _continuationToken?: string): Promise<DefinitionFindResult> {
return super.find() as unknown as DefinitionFindResult
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This find() override returns super.find() which is implemented to return null in AbstractAzBlobStore, but the method is typed as returning DefinitionFindResult. Returning null here violates the DefinitionStore contract and can break code paths if this store is used without a dispatch fallback. Prefer explicitly throwing an "unsupported" error or returning an empty DefinitionFindResult ({ data: [] }) to satisfy the interface without relying on casts.

Suggested change
// @ts-expect-error - AzBlob store does not support find; satisfies DefinitionStore interface
override async find(_query: DefinitionFindQuery, _continuationToken?: string): Promise<DefinitionFindResult> {
return super.find() as unknown as DefinitionFindResult
/**
* AzBlob store does not support find; return an empty result to satisfy
* the DefinitionStore contract without returning null.
*/
override async find(_query: DefinitionFindQuery, _continuationToken?: string): Promise<DefinitionFindResult> {
return { data: [] }

Copilot uses AI. Check for mistakes.
Comment on lines 62 to 64
async validate(definition: Definition | null): Promise<Definition | null> {
return this._upgradePolicy.validate(definition)
return this._upgradePolicy.validate(definition) as Promise<Definition | null>
}
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate() is declared as Promise<Definition | null> but the underlying UpgradeHandler.validate contract (and implementations like DefinitionVersionChecker.validate) can return undefined to indicate "needs recompute". Casting away undefined (as Promise<Definition | null>) can mislead callers/types and still allows undefined to flow at runtime. Consider keeping the return type as Promise<Definition | null | undefined> and returning the policy result without a cast.

Copilot uses AI. Check for mistakes.
Comment on lines 88 to 91
async _getRelatedDefinitions(coordinates: EntityCoordinates): Promise<RelatedDefinitions | null> {
const query = coordinates.asRevisionless()
query.namespace = query.namespace ? query.namespace : null // explicitly exclude namespace
query.namespace = query.namespace ? query.namespace : (null as unknown as string) // explicitly exclude namespace
const results = await this.definitionService.find(query)
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

query.namespace is being assigned null via (null as unknown as string), which defeats type checking and makes it hard to reason about the query shape. Since downstream query logic appears to intentionally treat namespace: null as "explicitly match no-namespace definitions", consider updating the query type to allow namespace?: string | null (e.g., DefinitionFindQuery.namespace) or introducing a dedicated query type for this use case, rather than casting null to string.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants