From 14573a271406f24bbdbc75aec384f26da4da78f1 Mon Sep 17 00:00:00 2001 From: voiys Date: Tue, 10 Nov 2026 00:13:29 +0100 Subject: [PATCH] Warn when a forEnv() arg is not a value of the @currentEnv enum A static forEnv() arg that is not a member of the @currentEnv item's enum can never match, so @required=forEnv(prodution) silently never requires anything. It is now a warning with the closest enum value as a hint. Only checked when the item is a plain enum with literal members. --- .bumpy/forenv-validate-env-names.md | 5 + .../src/content/docs/reference/functions.mdx | 2 + .../varlock/src/cli/helpers/arg-errors.ts | 35 +-- .../src/cli/helpers/test/arg-errors.test.ts | 2 +- .../src/cli/helpers/test/error-checks.test.ts | 27 ++ .../varlock/src/env-graph/lib/env-graph.ts | 20 +- .../varlock/src/env-graph/lib/resolver.ts | 40 +++ .../env-graph/test/forenv-env-names.test.ts | 237 ++++++++++++++++++ packages/varlock/src/lib/suggest-closest.ts | 50 ++++ .../src/lib/test/suggest-closest.test.ts | 19 ++ 10 files changed, 403 insertions(+), 34 deletions(-) create mode 100644 .bumpy/forenv-validate-env-names.md create mode 100644 packages/varlock/src/env-graph/test/forenv-env-names.test.ts create mode 100644 packages/varlock/src/lib/suggest-closest.ts create mode 100644 packages/varlock/src/lib/test/suggest-closest.test.ts diff --git a/.bumpy/forenv-validate-env-names.md b/.bumpy/forenv-validate-env-names.md new file mode 100644 index 000000000..95b032db1 --- /dev/null +++ b/.bumpy/forenv-validate-env-names.md @@ -0,0 +1,5 @@ +--- +varlock: patch +--- + +Warn when a `forEnv()` argument is not one of the `@currentEnv` enum values, with a "did you mean" hint, so a typo like `forEnv(prodution)` no longer silently never matches diff --git a/packages/varlock-website/src/content/docs/reference/functions.mdx b/packages/varlock-website/src/content/docs/reference/functions.mdx index 2272aa010..2e666f720 100644 --- a/packages/varlock-website/src/content/docs/reference/functions.mdx +++ b/packages/varlock-website/src/content/docs/reference/functions.mdx @@ -209,6 +209,8 @@ DEV_API_KEY= # @required=forEnv(staging, production) PROD_API_KEY= ``` + +If the `@currentEnv` item is typed as an `enum`, varlock warns about any argument written out literally that is not one of the enum values, because it can never match. For example, `forEnv(prodution)` is reported instead of silently never being true, and the warning suggests the closest enum value when there is one (within two edits). Arguments that are not literal, such as `forEnv($OTHER_ITEM)`, are not checked, and neither is an enum whose values are computed. diff --git a/packages/varlock/src/cli/helpers/arg-errors.ts b/packages/varlock/src/cli/helpers/arg-errors.ts index 1fbab4f8b..e93ed3d73 100644 --- a/packages/varlock/src/cli/helpers/arg-errors.ts +++ b/packages/varlock/src/cli/helpers/arg-errors.ts @@ -1,5 +1,6 @@ import { CliExitError } from './exit-error'; import { fmt } from './pretty-format'; +import { suggestClosest as suggestClosestName } from '../../lib/suggest-closest'; // Errors about the CLI arguments themselves - unrelated to the env-graph's ValidationError, // which is about config item values failing their schema. @@ -29,46 +30,16 @@ type CommandNotFoundError = { /** gunshi's placeholder name for the unnamed entry command; never something to suggest */ const ANONYMOUS_COMMAND_NAME = '(anonymous)'; -/** - * Levenshtein distance, kept dependency-free for the "did you mean" suggestion. - * - * @internal exported for unit tests - */ -export function levenshtein(a: string, b: string): number { - const rows = a.length + 1; - const cols = b.length + 1; - const dist: Array> = Array.from({ length: rows }, () => new Array(cols).fill(0)); - for (let i = 0; i < rows; i++) dist[i][0] = i; - for (let j = 0; j < cols; j++) dist[0][j] = j; - for (let i = 1; i < rows; i++) { - for (let j = 1; j < cols; j++) { - const cost = a[i - 1] === b[j - 1] ? 0 : 1; - dist[i][j] = Math.min( - dist[i - 1][j] + 1, - dist[i][j - 1] + 1, - dist[i - 1][j - 1] + cost, - ); - } - } - return dist[a.length][b.length]; -} - const stripDashes = (value: string) => value.replace(/^--?/, ''); /** * Closest candidate within edit distance 2, or undefined when nothing is close enough. + * Leading dashes are ignored so `--alow` matches `--allow`. * * @internal exported for unit tests */ export function suggestClosest(typed: string, candidates: Array, maxDistance = 2) { - let best: { name: string, distance: number } | undefined; - for (const candidate of candidates) { - const distance = levenshtein(stripDashes(typed), stripDashes(candidate)); - if (distance <= maxDistance && (!best || distance < best.distance)) { - best = { name: candidate, distance }; - } - } - return best?.name; + return suggestClosestName(typed, candidates, maxDistance, stripDashes); } // Errors arrive from gunshi's own bundle, so `instanceof` against the classes re-exported diff --git a/packages/varlock/src/cli/helpers/test/arg-errors.test.ts b/packages/varlock/src/cli/helpers/test/arg-errors.test.ts index 6d41b619a..310e47fe1 100644 --- a/packages/varlock/src/cli/helpers/test/arg-errors.test.ts +++ b/packages/varlock/src/cli/helpers/test/arg-errors.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest'; +import { levenshtein } from '../../../lib/suggest-closest'; import { - levenshtein, suggestClosest, commandPathFromArgs, isArgError, diff --git a/packages/varlock/src/cli/helpers/test/error-checks.test.ts b/packages/varlock/src/cli/helpers/test/error-checks.test.ts index 8f8cd8592..6001d3bee 100644 --- a/packages/varlock/src/cli/helpers/test/error-checks.test.ts +++ b/packages/varlock/src/cli/helpers/test/error-checks.test.ts @@ -131,4 +131,31 @@ describe('checkForSchemaErrors prints each error once', () => { expect(countOccurrences(output, err.message), err.message).toBe(1); } }); + + it('shows the did-you-mean for a forEnv() typo in a root decorator', async () => { + const g = await loadGraph(outdent` + # @currentEnv=$APP_ENV + # @import(./.env.extra, enabled=forEnv(prodution), allowMissing=true) + # --- + # @type=enum(development, production) + # @public + APP_ENV=production + `); + const output = captureOutput(() => checkForSchemaErrors(g, { noThrow: true })); + expect(output).toContain('"prodution" is not a possible value of APP_ENV'); + expect(output).toContain('Did you mean "production"?'); + }); + + it('lists the possible values for a forEnv() root decorator with no close match', async () => { + const g = await loadGraph(outdent` + # @currentEnv=$APP_ENV + # @import(./.env.extra, enabled=forEnv(staging), allowMissing=true) + # --- + # @type=enum(development, production) + # @public + APP_ENV=production + `); + const output = captureOutput(() => checkForSchemaErrors(g, { noThrow: true })); + expect(output).toContain('"staging" is not a possible value of APP_ENV (the @currentEnv item: development, production)'); + }); }); diff --git a/packages/varlock/src/env-graph/lib/env-graph.ts b/packages/varlock/src/env-graph/lib/env-graph.ts index c6a89a42d..256be9ead 100644 --- a/packages/varlock/src/env-graph/lib/env-graph.ts +++ b/packages/varlock/src/env-graph/lib/env-graph.ts @@ -6,7 +6,9 @@ import { ConfigItem, type TypeGenItemInfo } from './config-item'; import { EnvGraphDataSource, FileBasedDataSource, ImportAliasSource } from './data-source'; import { computeFilteredKeys, type ParsedItemFilter } from './item-filter'; -import { BaseResolvers, createResolver, type ResolverChildClass } from './resolver'; +import { + BaseResolvers, checkForEnvArgs, createResolver, type Resolver, type ResolverChildClass, +} from './resolver'; import { BaseDataTypes, type EnvGraphDataTypeFactory } from './data-types'; import { findGraphCycles, getTransitiveDeps, type GraphAdjacencyList } from './graph-utils'; import { ResolutionError, SchemaError, ValidationError } from './errors'; @@ -258,6 +260,12 @@ export class EnvGraph { configSchema: Record = {}; + /** + * every forEnv() call seen while processing, with the key of the @currentEnv item it + * evaluates against, checked once all item types are known + */ + forEnvCalls = new Map(); + /** * Tracks directory/file paths that have already been loaded as imports. @@ -780,6 +788,16 @@ export class EnvGraph { } } this.checkBootDynamicItems(); + // grouped by @currentEnv item, so each item's env names are read once + const forEnvCallsByEnvKey = new Map>(); + this.forEnvCalls.forEach((envKey, call) => { + if (!envKey) return; + const calls = forEnvCallsByEnvKey.get(envKey); + if (calls) calls.push(call); + else forEnvCallsByEnvKey.set(envKey, [call]); + }); + forEnvCallsByEnvKey.forEach((calls, envKey) => checkForEnvArgs(calls, this.configSchema[envKey])); + this.forEnvCalls.clear(); // now execute all root decorators for (const source of this.sortedDataSources) { diff --git a/packages/varlock/src/env-graph/lib/resolver.ts b/packages/varlock/src/env-graph/lib/resolver.ts index d01a4504c..4d75cb2e7 100644 --- a/packages/varlock/src/env-graph/lib/resolver.ts +++ b/packages/varlock/src/env-graph/lib/resolver.ts @@ -25,6 +25,7 @@ import { DecoratorInstance } from './decorators'; import { getErrorLocation } from './error-location'; import { isBuiltinVar } from './builtin-vars'; import { REGEX_LIKE_STRING, parseRegexLikeString } from './regex-like-string'; +import { suggestClosest } from '../../lib/suggest-closest'; const execAsync = promisify(exec); @@ -692,6 +693,11 @@ export const ForEnvResolver: typeof Resolver = createResolver({ arrayMinLength: 1, }, process() { + // The args are checked against the @currentEnv item's enum once all items are typed (see + // checkForEnvArgs). Which item that is has to be read now: @disable is processed before + // the file's own @currentEnv takes effect, so it uses the parent's. + const forEnvCalls = this.envGraph?.forEnvCalls; + if (forEnvCalls && !forEnvCalls.has(this)) forEnvCalls.set(this, this.dataSource?.envFlagKey); return this.arrArgs!; }, async resolve(matchEnvArgs) { @@ -712,6 +718,40 @@ export const ForEnvResolver: typeof Resolver = createResolver({ }, }); +/** + * A static `forEnv()` arg that is not a member of the @currentEnv item's enum can never match, + * so the call is dead (`@required=forEnv(prodution)` silently never requires anything). + * Reported as a warning, not an error, since a schema with a stale arg (an env dropped from + * the enum) loads fine today. + * + * Only checked when the item is a plain `enum(...)` whose members are written literally. + * Dynamic args (`forEnv($X)`) and any other type of env item are left alone. + * + * Everything goes in the message rather than the tip, because warnings on a data source + * (a root decorator's) are printed without their tip. + */ +export function checkForEnvArgs(forEnvCalls: Array, envItem?: ConfigItem) { + if (!envItem || envItem.hasComputedType) return; + const members = envItem.dataType?.coercedType; + if (typeof members !== 'object' || !('enum' in members)) return; + const envNames = members.enum.map(String); + const envNameSet = new Set(envNames); + + for (const resolver of forEnvCalls) { + for (const arg of resolver.arrArgs ?? []) { + if (!(arg instanceof StaticValueResolver)) continue; + const argName = String(arg.staticValue); + if (envNameSet.has(argName)) continue; + const suggestion = suggestClosest(argName, envNames); + resolver._errors.push(new SchemaError( + `${resolver.fnName}(): "${argName}" is not a possible value of ${envItem.key} (the @currentEnv item: ${envNames.join(', ')}), so it can never match` + + `${suggestion ? `. Did you mean "${suggestion}"?` : ''}`, + { isWarning: true }, + )); + } + } +} + export const EqResolver: typeof Resolver = createResolver({ name: 'eq', icon: 'material-symbols:equal', diff --git a/packages/varlock/src/env-graph/test/forenv-env-names.test.ts b/packages/varlock/src/env-graph/test/forenv-env-names.test.ts new file mode 100644 index 000000000..7aa2fbe64 --- /dev/null +++ b/packages/varlock/src/env-graph/test/forenv-env-names.test.ts @@ -0,0 +1,237 @@ +import { describe, test, expect } from 'vitest'; +import outdent from 'outdent'; +import type { EnvGraph } from '../index'; +import { loadGraph } from './type-generation/helpers'; + +async function load(files: Record, opts?: { fallbackEnv?: string }) { + const g = await loadGraph({ files, ...opts }); + await g.resolveEnvValues(); + return g; +} + +/** the "can never match" findings, from every item and every data source */ +function forEnvFindings(g: EnvGraph) { + return [ + ...Object.values(g.configSchema).flatMap((item) => item.errors), + ...g.sortedDataSources.flatMap((source) => source.errors), + ].filter((e) => e.message.includes('can never match')); +} + +const ENV_ITEM = outdent` + # @currentEnv=$APP_ENV + # --- + # @type=enum(development, production) + # @public + APP_ENV=production +`; + +describe('forEnv() args that are not a value of the @currentEnv enum', () => { + test('a typo in @required=forEnv() warns with a did-you-mean hint', async () => { + const g = await load({ + '.env.schema': `${ENV_ITEM}\n# @required=forEnv(prodution) @type=url\nAPI_URL=\n`, + }); + const [finding, ...rest] = forEnvFindings(g); + expect(rest).toEqual([]); + expect(finding.isWarning).toBe(true); + expect(finding.message).toBe( + 'forEnv(): "prodution" is not a possible value of APP_ENV (the @currentEnv item: development, production), so it can never match. Did you mean "production"?', + ); + expect(g.configSchema.API_URL.validationState).toBe('warn'); + }); + + test('it stays a warning: the schema still loads and resolves', async () => { + const g = await load({ + '.env.schema': `${ENV_ITEM}\n# @required=forEnv(prodution)\nAPI_KEY=abc\n`, + }); + expect(forEnvFindings(g)).toHaveLength(1); + expect(g.configSchema.API_KEY.isValid).toBe(true); + expect(g.configSchema.API_KEY.resolvedValue).toBe('abc'); + }); + + test('correct spelling does not warn', async () => { + const g = await load({ + '.env.schema': `${ENV_ITEM}\n# @required=forEnv(production) @type=url\nAPI_URL=https://example.com\n`, + }); + expect(forEnvFindings(g)).toEqual([]); + expect(g.configSchema.API_URL.validationState).toBe('valid'); + }); + + test('only the bad arg of several is reported', async () => { + const g = await load({ + '.env.schema': `${ENV_ITEM}\n# @required=forEnv(development, prodution, production)\nAPI_KEY=abc\n`, + }); + const findings = forEnvFindings(g); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('"prodution"'); + }); + + test('a case difference gets a hint', async () => { + const g = await load({ + '.env.schema': `${ENV_ITEM}\n# @required=forEnv(Production)\nAPI_KEY=abc\n`, + }); + expect(forEnvFindings(g)[0].message).toContain('Did you mean "production"?'); + }); + + test('with nothing close, it only lists the possible values', async () => { + const g = await load({ + '.env.schema': `${ENV_ITEM}\n# @required=forEnv(staging)\nAPI_KEY=abc\n`, + }); + const [finding] = forEnvFindings(g); + expect(finding.message).toContain('(the @currentEnv item: development, production)'); + expect(finding.message).not.toContain('Did you mean'); + }); + + test('checks forEnv() used as an item value', async () => { + const g = await load({ + '.env.schema': `${ENV_ITEM}\nLOG_LEVEL=if(forEnv(prodution), warn, debug)\n`, + }); + expect(forEnvFindings(g)).toHaveLength(1); + }); + + test('checks forEnv() in a root decorator', async () => { + const g = await load({ + '.env.schema': outdent` + # @currentEnv=$APP_ENV + # @import(./.env.extra, enabled=forEnv(prodution)) + # --- + # @type=enum(development, production) + # @public + APP_ENV=production + `, + '.env.extra': 'EXTRA=1\n', + }); + const findings = forEnvFindings(g); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('Did you mean "production"?'); + }); + + describe('an imported file with its own @currentEnv', () => { + // @disable runs before the file's own @currentEnv applies, so it evaluates against the + // parent's env item; everything else in the file uses the file's own + const parent = outdent` + # @currentEnv=$APP_ENV + # @import(./.env.child) + # --- + # @type=enum(development, production) + # @public + APP_ENV=production + `; + const childHeader = (disable: string) => outdent` + # @disable=${disable} + # @currentEnv=$CHILD_ENV + # --- + # @type=enum(staging, release) + # @public + CHILD_ENV=staging + `; + + test('@disable is checked against the parent env, not the file\'s own', async () => { + const g = await load({ + '.env.schema': parent, + '.env.child': `${childHeader('forEnv(development)')}\n`, + }); + expect(forEnvFindings(g)).toEqual([]); + + const typo = await load({ + '.env.schema': parent, + '.env.child': `${childHeader('forEnv(prodution)')}\n`, + }); + const findings = forEnvFindings(typo); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('possible value of APP_ENV'); + }); + + test('@import(enabled=) in the file is checked against the file\'s own env item', async () => { + const g = await load({ + '.env.schema': parent, + '.env.child': outdent` + # @currentEnv=$CHILD_ENV + # @import(./.env.grandchild, enabled=forEnv(production)) + # --- + # @type=enum(staging, release) + # @public + CHILD_ENV=staging + `, + '.env.grandchild': 'GRANDCHILD=1\n', + }); + const findings = forEnvFindings(g); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('"production" is not a possible value of CHILD_ENV'); + }); + + test('items in the file are checked against the file\'s own env item', async () => { + const g = await load({ + '.env.schema': parent, + '.env.child': `${childHeader('false')}\n# @required=forEnv(staging, development)\nKEY=abc\n`, + }); + const findings = forEnvFindings(g); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('"development" is not a possible value of CHILD_ENV'); + }); + }); + + test('numeric enum members compare by their string form', async () => { + const g = await load({ + '.env.schema': outdent` + # @currentEnv=$STAGE + # --- + # @type=enum(1, 2) + # @public + STAGE=1 + # @required=forEnv(2, 3) + API_KEY=abc + `, + }); + const findings = forEnvFindings(g); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('"3"'); + }); + + describe('left alone', () => { + test('dynamic args', async () => { + const g = await load({ + '.env.schema': `${ENV_ITEM}\n# @required=forEnv($APP_ENV)\nAPI_KEY=abc\n`, + }); + expect(forEnvFindings(g)).toEqual([]); + }); + + test('a @currentEnv item that is not an enum', async () => { + const g = await load({ + '.env.schema': outdent` + # @currentEnv=$APP_ENV + # --- + # @public + APP_ENV=production + # @required=forEnv(prodution) + API_KEY=abc + `, + }); + expect(forEnvFindings(g)).toEqual([]); + }); + + test('an enum whose members are computed', async () => { + const g = await load({ + '.env.schema': outdent` + # @currentEnv=$APP_ENV + # --- + # @type=enum(development, $EXTRA_ENV) + # @public + APP_ENV=production + EXTRA_ENV=production + # @required=forEnv(prodution) + API_KEY=abc + `, + }); + // control: the type really is computed, so the skip is the computed-type guard + expect(g.configSchema.APP_ENV.hasComputedType).toBe(true); + expect(forEnvFindings(g)).toEqual([]); + }); + + test('no @currentEnv item (a graph-level fallback env is not checked)', async () => { + const g = await load({ + '.env.schema': '# @required=forEnv(prodution)\nAPI_KEY=abc\n', + }, { fallbackEnv: 'production' }); + expect(forEnvFindings(g)).toEqual([]); + }); + }); +}); diff --git a/packages/varlock/src/lib/suggest-closest.ts b/packages/varlock/src/lib/suggest-closest.ts new file mode 100644 index 000000000..eed178085 --- /dev/null +++ b/packages/varlock/src/lib/suggest-closest.ts @@ -0,0 +1,50 @@ +/** + * Levenshtein distance, kept dependency-free for "did you mean" suggestions. + */ +export function levenshtein(a: string, b: string): number { + const rows = a.length + 1; + const cols = b.length + 1; + const dist: Array> = Array.from({ length: rows }, () => new Array(cols).fill(0)); + for (let i = 0; i < rows; i++) dist[i][0] = i; + for (let j = 0; j < cols; j++) dist[0][j] = j; + for (let i = 1; i < rows; i++) { + for (let j = 1; j < cols; j++) { + const cost = a[i - 1] === b[j - 1] ? 0 : 1; + dist[i][j] = Math.min( + dist[i - 1][j] + 1, + dist[i][j - 1] + 1, + dist[i - 1][j - 1] + cost, + ); + } + } + return dist[a.length][b.length]; +} + +/** Names this long are not typos of anything; skipping them also bounds the matrix below. */ +const MAX_SUGGEST_LENGTH = 64; + +/** + * Closest candidate within edit distance 2 (by default), or undefined when nothing is close + * enough. `normalize` is applied to both sides before comparing. + */ +export function suggestClosest( + typed: string, + candidates: Array, + maxDistance = 2, + normalize: (value: string) => string = (value) => value, +) { + const typedNormalized = normalize(typed); + if (typedNormalized.length > MAX_SUGGEST_LENGTH) return undefined; + let best: { name: string, distance: number } | undefined; + for (const candidate of candidates) { + const candidateNormalized = normalize(candidate); + if (candidateNormalized.length > MAX_SUGGEST_LENGTH) continue; + // the length gap alone is a lower bound on the distance + if (Math.abs(typedNormalized.length - candidateNormalized.length) > maxDistance) continue; + const distance = levenshtein(typedNormalized, candidateNormalized); + if (distance <= maxDistance && (!best || distance < best.distance)) { + best = { name: candidate, distance }; + } + } + return best?.name; +} diff --git a/packages/varlock/src/lib/test/suggest-closest.test.ts b/packages/varlock/src/lib/test/suggest-closest.test.ts new file mode 100644 index 000000000..c89d77497 --- /dev/null +++ b/packages/varlock/src/lib/test/suggest-closest.test.ts @@ -0,0 +1,19 @@ +import { describe, it, expect } from 'vitest'; +import { suggestClosest } from '../suggest-closest'; + +describe('suggestClosest', () => { + it('finds the nearest candidate within the distance', () => { + expect(suggestClosest('prodution', ['development', 'production'])).toBe('production'); + expect(suggestClosest('Production', ['development', 'production'])).toBe('production'); + }); + + it('returns undefined when nothing is close', () => { + expect(suggestClosest('staging', ['development', 'production'])).toBeUndefined(); + }); + + it('gives up on very long input instead of computing a huge matrix', () => { + const long = 'a'.repeat(10_000); + expect(suggestClosest(long, [long, 'a'])).toBeUndefined(); + expect(suggestClosest('a', [long])).toBeUndefined(); + }); +});