From f86dedcbe729b2f0ae23bc5bf1a8ed87d28eecb6 Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:41:53 +0300 Subject: [PATCH 1/6] refactor(helm): share raw scalar reading between readers Reading a scalar's exact source text, rather than the value YAML parsed it into, is about to have a second reader: chart metadata needs `appVersion` read under the same rules a tag is read under. Move the primitive into its own module so both readers agree on what "usable" means instead of one importing an internal of the other. Pure move. The extract-image-references test suite is untouched. Co-Authored-By: Claude Opus 5 (1M context) --- packages/helm/src/extract-image-references.ts | 82 +----------------- packages/helm/src/index.ts | 3 +- packages/helm/src/raw-scalar.ts | 83 +++++++++++++++++++ 3 files changed, 87 insertions(+), 81 deletions(-) create mode 100644 packages/helm/src/raw-scalar.ts diff --git a/packages/helm/src/extract-image-references.ts b/packages/helm/src/extract-image-references.ts index f2e9498..f7a6076 100644 --- a/packages/helm/src/extract-image-references.ts +++ b/packages/helm/src/extract-image-references.ts @@ -1,16 +1,5 @@ import { isPair, isScalar, parseDocument, visit, type Document, type Pair } from 'yaml'; - -/** A half-open character range into the original source text. */ -interface SourceRange { - readonly start: number; - readonly end: number; -} - -/** A scalar's exact source text, never the value YAML parsed it into. */ -interface RawScalar { - readonly text: string; - readonly range: SourceRange; -} +import { readUsableScalar, type RawScalar } from './raw-scalar'; /** * A candidate image reference found in a Helm values file. `tag` is @@ -142,17 +131,6 @@ function readSiblingScalar(source: string, items: readonly Pair[], name: string) return pair === undefined ? undefined : readUsableScalar(source, pair.value); } -/** - * Reads a scalar a consumer can act on. Helm resolves template syntax at - * render time, and this package never renders, so a templated value is no - * more usable here than a missing one and reads as `undefined` too. - */ -function readUsableScalar(source: string, node: unknown): RawScalar | undefined { - const scalar = readRawScalar(source, node); - - return scalar === undefined || containsHelmTemplateSyntax(scalar.text) ? undefined : scalar; -} - /** Whether a mapping's parent key is `image` or ends in `Image`. */ function hasCorroboratingParentKey(path: readonly unknown[]): boolean { const parent = path[path.length - 1]; @@ -166,61 +144,5 @@ function keyNameOf(pair: Pair): string | undefined { return isScalar(pair.key) ? String(pair.key.value) : undefined; } -/** Whether a scalar's raw text contains unresolved Helm template syntax. */ -function containsHelmTemplateSyntax(text: string): boolean { - return text.includes('{{') && text.includes('}}'); -} - -/** - * Slices a scalar node's exact source text out of the document, stripping a - * single layer of matching quotes when the scalar was written quoted. Only - * plain and single/double-quoted scalars carry a usable range here; a - * missing value, or a non-scalar value (a nested mapping or sequence), - * yields `undefined` so the caller skips the candidate entirely. - */ -function readRawScalar(source: string, node: unknown): RawScalar | undefined { - if (!isScalar(node)) { - return undefined; - } - - const range = node.range; - - if (!range) { - return undefined; - } - - const [start, end] = range; - - // Nothing after the colon (`repository:`) parses as a zero-length scalar, - // not `null` — treat it as absent rather than an empty-string value. - if (start === end) { - return undefined; - } - - const raw = source.slice(start, end); - const { text, offset } = stripQuotes(raw); - - return { - text, - range: { start: start + offset, end: start + offset + text.length }, - }; -} - -// A quote pair is the shortest string that can carry one: two characters, -// one at each end. -const MIN_QUOTED_LENGTH = 2; - -/** Strips one layer of matching single or double quotes, if present. */ -function stripQuotes(raw: string): { text: string; offset: number } { - const first = raw[0]; - const last = raw[raw.length - 1]; - - if (raw.length >= MIN_QUOTED_LENGTH && first === last && (first === '"' || first === "'")) { - return { text: raw.slice(1, raw.length - 1), offset: 1 }; - } - - return { text: raw, offset: 0 }; -} - export { extractImageReferences }; -export type { ImageReference, RawScalar, SourceRange }; +export type { ImageReference }; diff --git a/packages/helm/src/index.ts b/packages/helm/src/index.ts index dc28aa5..a958b65 100644 --- a/packages/helm/src/index.ts +++ b/packages/helm/src/index.ts @@ -1,2 +1,3 @@ export { extractImageReferences } from './extract-image-references'; -export type { ImageReference, RawScalar, SourceRange } from './extract-image-references'; +export type { ImageReference } from './extract-image-references'; +export type { RawScalar, SourceRange } from './raw-scalar'; diff --git a/packages/helm/src/raw-scalar.ts b/packages/helm/src/raw-scalar.ts new file mode 100644 index 0000000..f97bced --- /dev/null +++ b/packages/helm/src/raw-scalar.ts @@ -0,0 +1,83 @@ +import { isScalar } from 'yaml'; + +/** A half-open character range into the original source text. */ +interface SourceRange { + readonly start: number; + readonly end: number; +} + +/** A scalar's exact source text, never the value YAML parsed it into. */ +interface RawScalar { + readonly text: string; + readonly range: SourceRange; +} + +/** + * Reads a scalar a consumer can act on. Helm resolves template syntax at + * render time, and this package never renders, so a templated value is no + * more usable here than a missing one and reads as `undefined` too. + */ +function readUsableScalar(source: string, node: unknown): RawScalar | undefined { + const scalar = readRawScalar(source, node); + + return scalar === undefined || containsHelmTemplateSyntax(scalar.text) ? undefined : scalar; +} + +/** Whether a scalar's raw text contains unresolved Helm template syntax. */ +function containsHelmTemplateSyntax(text: string): boolean { + return text.includes('{{') && text.includes('}}'); +} + +/** + * Slices a scalar node's exact source text out of the document, stripping a + * single layer of matching quotes when the scalar was written quoted. Only + * plain and single/double-quoted scalars carry a usable range here; a + * missing value, or a non-scalar value (a nested mapping or sequence), + * yields `undefined` so the caller skips the candidate entirely. + */ +function readRawScalar(source: string, node: unknown): RawScalar | undefined { + if (!isScalar(node)) { + return undefined; + } + + const range = node.range; + + if (!range) { + return undefined; + } + + const [start, end] = range; + + // Nothing after the colon (`repository:`) parses as a zero-length scalar, + // not `null` — treat it as absent rather than an empty-string value. + if (start === end) { + return undefined; + } + + const raw = source.slice(start, end); + const { text, offset } = stripQuotes(raw); + + return { + text, + range: { start: start + offset, end: start + offset + text.length }, + }; +} + +// A quote pair is the shortest string that can carry one: two characters, +// one at each end. +const MIN_QUOTED_LENGTH = 2; + +/** Strips one layer of matching single or double quotes, if present. */ +function stripQuotes(raw: string): { text: string; offset: number } { + const first = raw[0]; + const last = raw[raw.length - 1]; + + if (raw.length >= MIN_QUOTED_LENGTH && first === last && (first === '"' || first === "'")) { + return { text: raw.slice(1, raw.length - 1), offset: 1 }; + } + + return { text: raw, offset: 0 }; +} + +export { readUsableScalar }; +export type { RawScalar, SourceRange }; From 616125bf98dcacbbf40840edaddf39756d69630f Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:52:06 +0300 Subject: [PATCH 2/6] feat(helm): resolve the chart governing a values file A values file means little on its own. Its tags may be absent, in which case Helm renders `.Chart.AppVersion` in their place, and whether the file is worth reading at all depends on the chart it sits under. The nearest ancestor holding `Chart.yaml` wins, so a subchart's values file resolves against the subchart's own metadata rather than its parent's, which is what Helm would actually deploy. Everything under the chart's `templates/` directory is out of scope, Go template source being nothing this package can check. With no chart above it, a file is in scope only when its own name says it is a values file. Paths are split on `/` rather than handed to `node:path`. Callers pass a URI path, forward-slash separated on every platform, and this package stays free of platform path semantics. `ResolvedTag` distinguishes the two provenances because they differ in what they can offer a message: a tag written in the file has a range to underline, a tag taken from chart metadata has a file to point at. Co-Authored-By: Claude Opus 5 (1M context) --- packages/helm/src/chart-context.test.ts | 171 ++++++++++++++++++++++++ packages/helm/src/chart-context.ts | 142 ++++++++++++++++++++ packages/helm/src/index.ts | 2 + 3 files changed, 315 insertions(+) create mode 100644 packages/helm/src/chart-context.test.ts create mode 100644 packages/helm/src/chart-context.ts diff --git a/packages/helm/src/chart-context.test.ts b/packages/helm/src/chart-context.test.ts new file mode 100644 index 0000000..27ce667 --- /dev/null +++ b/packages/helm/src/chart-context.test.ts @@ -0,0 +1,171 @@ +import { describe, expect, it } from 'vitest'; +import { resolveTag, resolveValuesFileContext, type ChartMetadata, type ReadTextFile } from './chart-context'; +import type { ImageReference } from './extract-image-references'; + +/** + * A file system described as a path-to-contents record, so a test states the + * directory shape it is about instead of writing temporary files. + */ +function createFakeFileSystem(files: Record): ReadTextFile { + return async (path) => Promise.resolve(files[path]); +} + +/** A reference whose only field `resolveTag` reads is the tag. */ +function createReference(tag: string | undefined): ImageReference { + return { + repository: { text: 'my-service', range: { start: 0, end: 10 } }, + tag: tag === undefined ? undefined : { text: tag, range: { start: 20, end: 20 + tag.length } }, + registry: undefined, + }; +} + +const CHART_METADATA = ['name: my-chart', 'appVersion: 1.2.3', ''].join('\n'); + +describe('resolveValuesFileContext', () => { + it('should resolve a values file against the chart in its own directory', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': CHART_METADATA }); + + const context = await resolveValuesFileContext('/repo/chart/values.yaml', readTextFile); + + expect(context?.chart?.path).toBe('/repo/chart/Chart.yaml'); + expect(context?.chart?.appVersion).toBe('1.2.3'); + }); + + it('should resolve a values file several directories deep against the chart above it', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': CHART_METADATA }); + + const context = await resolveValuesFileContext('/repo/chart/env/production/values.yaml', readTextFile); + + expect(context?.chart?.path).toBe('/repo/chart/Chart.yaml'); + }); + + it('should resolve a subchart values file against the subchart own metadata', async () => { + const readTextFile = createFakeFileSystem({ + '/repo/chart/Chart.yaml': ['name: parent', 'appVersion: 1.0.0', ''].join('\n'), + '/repo/chart/charts/sub/Chart.yaml': ['name: sub', 'appVersion: 2.0.0', ''].join('\n'), + }); + + const context = await resolveValuesFileContext('/repo/chart/charts/sub/values.yaml', readTextFile); + + expect(context?.chart?.path).toBe('/repo/chart/charts/sub/Chart.yaml'); + expect(context?.chart?.appVersion).toBe('2.0.0'); + }); + + it('should find chart metadata written as Chart.yml', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yml': CHART_METADATA }); + + const context = await resolveValuesFileContext('/repo/chart/values.yaml', readTextFile); + + expect(context?.chart?.path).toBe('/repo/chart/Chart.yml'); + expect(context?.chart?.appVersion).toBe('1.2.3'); + }); + + it('should treat any yaml file beneath a chart directory as in scope', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': CHART_METADATA }); + + const context = await resolveValuesFileContext('/repo/chart/production.yaml', readTextFile); + + expect(context?.chart?.path).toBe('/repo/chart/Chart.yaml'); + }); + + it('should find chart metadata sitting at the filesystem root', async () => { + const readTextFile = createFakeFileSystem({ '/Chart.yaml': CHART_METADATA }); + + const context = await resolveValuesFileContext('/values.yaml', readTextFile); + + expect(context?.chart?.path).toBe('/Chart.yaml'); + }); + + it('should exclude a file under the chart templates directory', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': CHART_METADATA }); + + await expect(resolveValuesFileContext('/repo/chart/templates/deployment.yaml', readTextFile)).resolves.toBeUndefined(); + }); + + it('should exclude a file named values.yaml under the chart templates directory', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': CHART_METADATA }); + + await expect(resolveValuesFileContext('/repo/chart/templates/values.yaml', readTextFile)).resolves.toBeUndefined(); + }); + + it('should exclude a subchart templates file against the subchart rather than against its parent', async () => { + const readTextFile = createFakeFileSystem({ + '/repo/chart/Chart.yaml': ['name: parent', 'appVersion: 1.0.0', ''].join('\n'), + '/repo/chart/charts/sub/Chart.yaml': ['name: sub', 'appVersion: 2.0.0', ''].join('\n'), + }); + + // Relative to the parent chart this path reads `charts/sub/templates/...`, + // which no `templates/` rule would exclude. + await expect(resolveValuesFileContext('/repo/chart/charts/sub/templates/values.yaml', readTextFile)).resolves.toBeUndefined(); + }); + + it('should treat a standalone values.yaml with no chart above it as in scope with no chart', async () => { + const readTextFile = createFakeFileSystem({}); + + const context = await resolveValuesFileContext('/repo/loose/values.yaml', readTextFile); + + expect(context).toEqual({ chart: undefined }); + }); + + it('should ignore an unrelated yaml file with no chart above it', async () => { + const readTextFile = createFakeFileSystem({}); + + await expect(resolveValuesFileContext('/repo/loose/config.yaml', readTextFile)).resolves.toBeUndefined(); + }); + + describe('appVersion', () => { + it('should report no appVersion when the chart declares none', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': ['name: my-chart', 'version: 0.1.0', ''].join('\n') }); + + const context = await resolveValuesFileContext('/repo/chart/values.yaml', readTextFile); + + expect(context?.chart?.path).toBe('/repo/chart/Chart.yaml'); + expect(context?.chart?.appVersion).toBeUndefined(); + }); + + it('should read appVersion from raw source text, not the value YAML parsed it into', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': ['name: my-chart', 'appVersion: 1.10', ''].join('\n') }); + + const context = await resolveValuesFileContext('/repo/chart/values.yaml', readTextFile); + + // YAML's core schema coerces the plain scalar 1.10 to the float 1.1. + expect(context?.chart?.appVersion).toBe('1.10'); + }); + + it('should report no appVersion when the declared one contains template syntax', async () => { + const readTextFile = createFakeFileSystem({ + '/repo/chart/Chart.yaml': ['name: my-chart', 'appVersion: "{{ .Chart.Version }}"', ''].join('\n'), + }); + + const context = await resolveValuesFileContext('/repo/chart/values.yaml', readTextFile); + + expect(context?.chart?.appVersion).toBeUndefined(); + }); + }); +}); + +describe('resolveTag', () => { + const chart: ChartMetadata = { path: '/repo/chart/Chart.yaml', appVersion: '1.2.3' }; + + it('should prefer the tag written in the file over the chart appVersion', () => { + const reference = createReference('9.9.9'); + + expect(resolveTag(reference, chart)).toEqual({ source: 'file', text: '9.9.9', range: reference.tag?.range }); + }); + + it('should fall back to the chart appVersion for a tagless reference', () => { + expect(resolveTag(createReference(undefined), chart)).toEqual({ + source: 'chart-metadata', + text: '1.2.3', + metadataPath: '/repo/chart/Chart.yaml', + }); + }); + + it('should resolve nothing for a tagless reference with no chart', () => { + expect(resolveTag(createReference(undefined), undefined)).toBeUndefined(); + }); + + it('should resolve nothing for a tagless reference under a chart that declares no appVersion', () => { + expect(resolveTag(createReference(undefined), { path: '/repo/chart/Chart.yaml', appVersion: undefined })).toBeUndefined(); + }); +}); diff --git a/packages/helm/src/chart-context.ts b/packages/helm/src/chart-context.ts new file mode 100644 index 0000000..91db861 --- /dev/null +++ b/packages/helm/src/chart-context.ts @@ -0,0 +1,142 @@ +import { parseDocument } from 'yaml'; +import type { ImageReference } from './extract-image-references'; +import { readUsableScalar, type SourceRange } from './raw-scalar'; + +/** Reads a file's text, or `undefined` when nothing readable sits at that path. */ +type ReadTextFile = (path: string) => Promise; + +/** The chart metadata governing a values file. */ +interface ChartMetadata { + /** Path of the metadata file itself, so a message can point the reader at it. */ + readonly path: string; + /** `appVersion` exactly as written, or `undefined` when the chart declares none. */ + readonly appVersion: string | undefined; +} + +/** A YAML file this package has something to say about, and the chart that governs it. */ +interface ValuesFileContext { + readonly chart: ChartMetadata | undefined; +} + +/** + * The tag a reference is checked against, and where it came from. The two + * arms differ in what they can offer a message: a tag written in the file + * has a range to underline, a tag taken from chart metadata has a file to + * point the reader at instead. + */ +type ResolvedTag = + | { readonly source: 'file'; readonly text: string; readonly range: SourceRange } + | { readonly source: 'chart-metadata'; readonly text: string; readonly metadataPath: string }; + +/** + * The names chart metadata goes by, in the order they are tried. Helm's own + * convention is `Chart.yaml`, and `Chart.yml` turns up often enough that the + * second spelling is a new row here rather than a new branch below. + */ +const CHART_METADATA_FILE_NAMES: readonly string[] = ['Chart.yaml', 'Chart.yml']; + +/** The chart subdirectory holding Go templates, whose syntax yields nothing checkable. */ +const TEMPLATES_DIRECTORY_NAME = 'templates'; + +/** + * The names a values file goes by when no chart vouches for it — outside a + * chart directory the name is the only evidence there is. + */ +const STANDALONE_VALUES_FILE_NAME = /^values\.ya?ml$/i; + +/** + * Resolves the chart governing a YAML file, and with it whether this package + * has anything to say about that file at all. + * + * The nearest ancestor directory holding chart metadata wins, which is what + * makes a subchart's values file resolve against the subchart's own + * `appVersion` rather than its parent's — the subchart is what Helm would + * actually deploy. Every YAML file beneath a chart directory is in scope, + * not just the conventionally named ones, because environment overlays + * rarely carry the name `values.yaml`. + * + * A file under the chart's `templates/` directory is out of scope outright, + * even one named `values.yaml`. It is Go template source, and template + * syntax resolves at render time, which this package never does. + * + * With no chart anywhere above it, a file is in scope only when its own name + * says it is a values file — that keeps a standalone values file working + * while leaving unrelated workspace YAML alone. + * + * Paths are split on `/` here rather than handed to `node:path`. Callers pass + * a URI path, which is forward-slash separated on every platform, and + * keeping platform path semantics out of this package keeps it a pure + * source-text library. + */ +async function resolveValuesFileContext(path: string, readTextFile: ReadTextFile): Promise { + const segments = path.split('/'); + + for (let depth = segments.length - 1; depth > 0; depth -= 1) { + const chart = await readChartMetadata(segments.slice(0, depth), readTextFile); + + if (chart !== undefined) { + const firstSegmentBelowChart = segments[depth]; + + return firstSegmentBelowChart === TEMPLATES_DIRECTORY_NAME ? undefined : { chart }; + } + } + + const fileName = segments[segments.length - 1]; + + return fileName !== undefined && STANDALONE_VALUES_FILE_NAME.test(fileName) ? { chart: undefined } : undefined; +} + +/** + * Reads the chart metadata sitting directly in one directory, if any. + * + * The candidate path is assembled by joining segments rather than by + * concatenating a directory and a name, so the filesystem root — whose + * segments are a lone empty string — yields `/Chart.yaml` instead of + * `//Chart.yaml`. + */ +async function readChartMetadata(directorySegments: readonly string[], readTextFile: ReadTextFile): Promise { + for (const fileName of CHART_METADATA_FILE_NAMES) { + const metadataPath = [...directorySegments, fileName].join('/'); + const text = await readTextFile(metadataPath); + + if (text !== undefined) { + return { path: metadataPath, appVersion: readAppVersion(text) }; + } + } + + return undefined; +} + +/** + * Reads `appVersion` exactly as the chart wrote it. YAML coerces + * `appVersion: 1.10` to the float `1.1`, so a parsed value would report a + * version the chart never declared. An `appVersion` carrying Helm template + * syntax is no more usable than an absent one, and reads as `undefined` too. + */ +function readAppVersion(source: string): string | undefined { + // `getIn` hands back parsed values by default, and a parsed value is the + // one thing this package never reads a field from. + return readUsableScalar(source, parseDocument(source).getIn(['appVersion'], true))?.text; +} + +/** + * Resolves the tag a reference is checked against. A tag written in the file + * wins; a tagless reference falls back to the governing chart's + * `appVersion`, which is what Helm would render in its place. `undefined` + * means the reference is checked against nothing at all and must produce no + * marker. + */ +function resolveTag(reference: ImageReference, chart: ChartMetadata | undefined): ResolvedTag | undefined { + if (reference.tag !== undefined) { + return { source: 'file', text: reference.tag.text, range: reference.tag.range }; + } + + if (chart?.appVersion !== undefined) { + return { source: 'chart-metadata', text: chart.appVersion, metadataPath: chart.path }; + } + + return undefined; +} + +export { resolveTag, resolveValuesFileContext }; +export type { ChartMetadata, ReadTextFile, ResolvedTag, ValuesFileContext }; diff --git a/packages/helm/src/index.ts b/packages/helm/src/index.ts index a958b65..d413d19 100644 --- a/packages/helm/src/index.ts +++ b/packages/helm/src/index.ts @@ -1,3 +1,5 @@ +export { resolveTag, resolveValuesFileContext } from './chart-context'; +export type { ChartMetadata, ReadTextFile, ResolvedTag, ValuesFileContext } from './chart-context'; export { extractImageReferences } from './extract-image-references'; export type { ImageReference } from './extract-image-references'; export type { RawScalar, SourceRange } from './raw-scalar'; From 002080e4769e94cbc4977abe513ce579cb0cd23a Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:53:49 +0300 Subject: [PATCH 3/6] feat(vscode): scope checks to charts and resolve tagless images via appVersion Which YAML files this extension marks up is now the Helm package's answer, not a filename pattern here: anything beneath a chart directory is in scope, that chart's `templates/` directory never is, and a tagless reference is checked against the governing chart's `appVersion`, which is what Helm would render in its place. A tagless reference nothing can resolve is dropped before the registry is asked, so it renders no mark, no hover, and no diagnostic. That makes the `'no-tag'` reason dead, and with it the `UncheckedReason` and `ReferenceVerdict` wrappers: every surface now takes `ImageVerdict` and `UnverifiableReason` from the registry package directly. Callers migrated and the old names deleted in one wave rather than aliased. A tag the file never wrote needs saying so. Its `tag-not-found` diagnostic attaches to the repository value, there being nothing else to underline, and both the diagnostic and the hover name the chart it came from through one shared sentence, so the two surfaces cannot word it differently. A `Chart.{yaml,yml}` watcher re-checks the open documents each chart governs. A version bump otherwise leaves a stale checkmark standing at exactly the moment correctness matters most. Co-Authored-By: Claude Opus 5 (1M context) --- apps/vscode/src/diagnostics.test.ts | 45 ++++++-- apps/vscode/src/diagnostics.ts | 23 +++-- apps/vscode/src/extension.test.ts | 69 +++++++++++-- apps/vscode/src/extension.ts | 101 +++++++++++++----- apps/vscode/src/hover.test.ts | 59 ++++++----- apps/vscode/src/hover.ts | 25 +++-- apps/vscode/src/marks.test.ts | 23 +++-- apps/vscode/src/marks.ts | 8 +- apps/vscode/src/reference-check.test.ts | 130 +++++++++++++++++++++--- apps/vscode/src/reference-check.ts | 105 ++++++++++--------- apps/vscode/test/fake-file-system.ts | 16 +++ apps/vscode/test/vscode-stub.ts | 73 ++++++++++++- 12 files changed, 515 insertions(+), 162 deletions(-) create mode 100644 apps/vscode/test/fake-file-system.ts diff --git a/apps/vscode/src/diagnostics.test.ts b/apps/vscode/src/diagnostics.test.ts index 7dccb1d..1d2f4ea 100644 --- a/apps/vscode/src/diagnostics.test.ts +++ b/apps/vscode/src/diagnostics.test.ts @@ -1,18 +1,19 @@ import * as vscode from 'vscode'; import { describe, expect, it } from 'vitest'; -import type { SourceRange } from 'helm'; +import type { ResolvedTag, SourceRange } from 'helm'; +import type { ImageVerdict, UnverifiableReason } from 'oci-registry'; import { createFakeDocument } from '../test/fake-document'; import { diagnosticsFor } from './diagnostics'; -import type { ReferenceCheck, ReferenceVerdict, UncheckedReason } from './reference-check'; +import type { ReferenceCheck } from './reference-check'; const REPOSITORY = 'registry.example.com/svc'; const TAG = '1.0'; const VALUES_YAML = ['image:', ` repository: ${REPOSITORY}`, ` tag: "${TAG}"`, ''].join('\n'); +const CHART_METADATA_PATH = '/repo/chart/Chart.yaml'; // A record rather than a list, so a new reason fails the build here instead // of going untested. -const UNCHECKED_VERDICTS: Record = { - 'no-tag': { kind: 'unverifiable', reason: 'no-tag' }, +const UNCHECKED_VERDICTS: Record = { 'guessed-registry': { kind: 'unverifiable', reason: 'guessed-registry' }, 'needs-login': { kind: 'unverifiable', reason: 'needs-login', registry: REPOSITORY }, 'authentication-failure': { kind: 'unverifiable', reason: 'authentication-failure' }, @@ -21,6 +22,11 @@ const UNCHECKED_VERDICTS: Record = { 'malformed-reference': { kind: 'unverifiable', reason: 'malformed-reference' }, }; +/** Stands in for `vscode.workspace.asRelativePath`, shortening enough that a message can be seen to have used it. */ +function describeChartPath(path: string): string { + return path.replace('/repo/', ''); +} + /** The source range of `text`'s first occurrence in {@link VALUES_YAML}. */ function rangeOfText(text: string): SourceRange { const start = VALUES_YAML.indexOf(text); @@ -36,21 +42,27 @@ function documentRangeOfText(document: vscode.TextDocument, text: string): vscod } /** A check over the single reference in {@link VALUES_YAML}, carrying that file's real offsets. */ -function createCheck(verdict: ReferenceVerdict): ReferenceCheck { +function createCheck(verdict: ImageVerdict, tag: ResolvedTag = { source: 'file', text: TAG, range: rangeOfText(TAG) }): ReferenceCheck { return { reference: { repository: { text: REPOSITORY, range: rangeOfText(REPOSITORY) }, - tag: { text: TAG, range: rangeOfText(TAG) }, + tag: tag.source === 'file' ? { text: tag.text, range: tag.range } : undefined, registry: undefined, }, + tag, verdict, }; } +/** The same reference written without a tag, checked against the chart's `appVersion` instead. */ +function createChartMetadataCheck(verdict: ImageVerdict): ReferenceCheck { + return createCheck(verdict, { source: 'chart-metadata', text: TAG, metadataPath: CHART_METADATA_PATH }); +} + describe('diagnostics', () => { it('should report an error naming the missing repository, positioned on the repository value', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); - const fileDiagnostics = diagnosticsFor(document, [createCheck({ kind: 'repository-not-found', repository: REPOSITORY })]); + const fileDiagnostics = diagnosticsFor(document, [createCheck({ kind: 'repository-not-found', repository: REPOSITORY })], describeChartPath); expect(fileDiagnostics).toHaveLength(1); expect(fileDiagnostics[0]?.severity).toBe(vscode.DiagnosticSeverity.Error); @@ -60,25 +72,36 @@ describe('diagnostics', () => { it('should report an error naming the missing tag, positioned on the tag value', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); - const fileDiagnostics = diagnosticsFor(document, [createCheck({ kind: 'tag-not-found', repository: REPOSITORY, tag: TAG })]); + const fileDiagnostics = diagnosticsFor(document, [createCheck({ kind: 'tag-not-found', repository: REPOSITORY, tag: TAG })], describeChartPath); expect(fileDiagnostics).toHaveLength(1); expect(fileDiagnostics[0]?.severity).toBe(vscode.DiagnosticSeverity.Error); - expect(fileDiagnostics[0]?.message).toContain(TAG); + expect(fileDiagnostics[0]?.message).toBe(`Tag '${TAG}' not found in '${REPOSITORY}'.`); expect(fileDiagnostics[0]?.range).toEqual(documentRangeOfText(document, TAG)); }); + it('should attach a tag taken from chart metadata to the repository value, and name the chart it came from', () => { + const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); + const check = createChartMetadataCheck({ kind: 'tag-not-found', repository: REPOSITORY, tag: TAG }); + const fileDiagnostics = diagnosticsFor(document, [check], describeChartPath); + + // Nothing in this file spells the tag out, so there is no text to + // underline and no way for the reader to find it without being told. + expect(fileDiagnostics[0]?.range).toEqual(documentRangeOfText(document, REPOSITORY)); + expect(fileDiagnostics[0]?.message).toBe(`Tag '${TAG}' not found in '${REPOSITORY}'. Tag taken from appVersion in chart/Chart.yaml.`); + }); + it('should report nothing for a reference that exists', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); - expect(diagnosticsFor(document, [createCheck({ kind: 'exists', registry: 'registry.example.com' })])).toEqual([]); + expect(diagnosticsFor(document, [createCheck({ kind: 'exists', registry: 'registry.example.com' })], describeChartPath)).toEqual([]); }); it('should report nothing for any unverifiable reason, since an unreachable registry is not a missing image', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); for (const verdict of Object.values(UNCHECKED_VERDICTS)) { - expect(diagnosticsFor(document, [createCheck(verdict)])).toEqual([]); + expect(diagnosticsFor(document, [createCheck(verdict)], describeChartPath)).toEqual([]); } }); }); diff --git a/apps/vscode/src/diagnostics.ts b/apps/vscode/src/diagnostics.ts index 43c18fd..1db384e 100644 --- a/apps/vscode/src/diagnostics.ts +++ b/apps/vscode/src/diagnostics.ts @@ -1,5 +1,5 @@ import * as vscode from 'vscode'; -import type { ReferenceCheck } from './reference-check'; +import { chartProvenanceSentence, type ReferenceCheck } from './reference-check'; import { rangeOf } from './source-range'; /** @@ -7,11 +7,18 @@ import { rangeOf } from './source-range'; * verdicts qualify: that an unverifiable verdict never renders as an error * is the one invariant this feature must not break, because an expired token * or an unreachable registry must never look like a missing image. + * + * `describeChartPath` shortens a chart metadata path for display — an + * absolute path in the Problems panel is noise the reader has to scan past. */ -function diagnosticsFor(document: vscode.TextDocument, checks: readonly ReferenceCheck[]): vscode.Diagnostic[] { +function diagnosticsFor( + document: vscode.TextDocument, + checks: readonly ReferenceCheck[], + describeChartPath: (path: string) => string +): vscode.Diagnostic[] { const fileDiagnostics: vscode.Diagnostic[] = []; - for (const { reference, verdict } of checks) { + for (const { reference, tag, verdict } of checks) { if (verdict.kind === 'repository-not-found') { fileDiagnostics.push( new vscode.Diagnostic( @@ -23,10 +30,12 @@ function diagnosticsFor(document: vscode.TextDocument, checks: readonly Referenc } else if (verdict.kind === 'tag-not-found') { fileDiagnostics.push( new vscode.Diagnostic( - // Unreachable fallback: only a reference that named a tag can come - // back tag-not-found. It keeps the type honest without an assertion. - rangeOf(document, reference.tag?.range ?? reference.repository.range), - `Tag '${verdict.tag}' not found in '${verdict.repository}'.`, + // A tag taken from chart metadata has no text in this file to + // underline, so the squiggle goes on the repository it applies to + // and the message points at the chart instead of leaving the + // reader hunting for a tag that was never written here. + rangeOf(document, tag.source === 'file' ? tag.range : reference.repository.range), + `Tag '${verdict.tag}' not found in '${verdict.repository}'.${chartProvenanceSentence(tag, describeChartPath)}`, vscode.DiagnosticSeverity.Error ) ); diff --git a/apps/vscode/src/extension.test.ts b/apps/vscode/src/extension.test.ts index 78cdd56..64ca237 100644 --- a/apps/vscode/src/extension.test.ts +++ b/apps/vscode/src/extension.test.ts @@ -3,13 +3,17 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { createFakeContext } from '../test/fake-context'; import { noDockerCredentials } from '../test/fake-credentials'; import { bumpVersion, createFakeDocument } from '../test/fake-document'; +import { createFakeFileSystem } from '../test/fake-file-system'; import { fakeFetchResponse } from '../test/fake-fetch'; import { createTextEditorStub, + emitDidChangeChartMetadata, emitDidChangeVisibleTextEditors, emitDidOpenTextDocument, getLastDiagnosticCollection, + getLastFileSystemWatcher, getRegisteredHoverProvider, + setOpenTextDocuments, setVisibleTextEditors, setWarningMessageAnswer, window, @@ -20,6 +24,12 @@ import { activate, deactivate } from './extension'; const REPOSITORY = 'docker.io/library/nginx'; const TAG = '1.19'; const VALUES_YAML = ['image:', ` repository: ${REPOSITORY}`, ` tag: ${TAG}`, ''].join('\n'); +const TAGLESS_VALUES_YAML = ['image:', ` repository: ${REPOSITORY}`, ' pullPolicy: IfNotPresent', ''].join('\n'); + +// No chart metadata anywhere, so the fixtures named `values.yaml` resolve +// through the filename fallback and the older tests keep describing exactly +// what they did before chart context existed. +const NO_FILES = createFakeFileSystem({}); /** Stages `document` as the only visible editor, then fires the open event. */ async function openInVisibleEditor(document: vscode.TextDocument): Promise { @@ -79,12 +89,13 @@ describe('extension', () => { } setVisibleTextEditors([]); + setOpenTextDocuments([]); setWarningMessageAnswer(undefined); window.showWarningMessage.mockClear(); }); it('should create an output channel and register it for disposal on activate', () => { - activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials, readTextFile: NO_FILES }); expect(vscode.window.createOutputChannel).toHaveBeenCalledWith('Infra Tools'); expect(context.subscriptions.length).toBeGreaterThanOrEqual(1); @@ -95,20 +106,20 @@ describe('extension', () => { }); it('should register a yaml hover provider on activate', () => { - activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials, readTextFile: NO_FILES }); expect(vscode.languages.registerHoverProvider).toHaveBeenCalledWith({ language: 'yaml' }, expect.anything()); }); it('should create one mark decoration type on activate', () => { - activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials, readTextFile: NO_FILES }); expect(vscode.window.createTextEditorDecorationType).toHaveBeenCalledWith({ after: { margin: '0 0 0 0.5em' } }); }); it('should set both diagnostics and marks when a values file opens', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(404, { errors: [{ code: 'MANIFEST_UNKNOWN' }] })); - activate(context, { fetch, credentials: noDockerCredentials }); + activate(context, { fetch, credentials: noDockerCredentials, readTextFile: NO_FILES }); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); const editor = await openInVisibleEditor(document); @@ -121,7 +132,7 @@ describe('extension', () => { it('should hover a checked reference, and stop once the document has been edited past the check', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); - activate(context, { fetch, credentials: noDockerCredentials }); + activate(context, { fetch, credentials: noDockerCredentials, readTextFile: NO_FILES }); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); await openInVisibleEditor(document); @@ -135,7 +146,7 @@ describe('extension', () => { it('should re-apply marks to an editor that becomes visible after the document was checked', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); - activate(context, { fetch, credentials: noDockerCredentials }); + activate(context, { fetch, credentials: noDockerCredentials, readTextFile: NO_FILES }); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); @@ -150,7 +161,7 @@ describe('extension', () => { it('should survive an editor disposed mid-check, since an unhandled rejection kills the extension host', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); - activate(context, { fetch, credentials: noDockerCredentials }); + activate(context, { fetch, credentials: noDockerCredentials, readTextFile: NO_FILES }); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); const disposedEditor: TextEditorStub = { @@ -167,14 +178,14 @@ describe('extension', () => { }); it('should create a status bar item on activate', () => { - activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials, readTextFile: NO_FILES }); expect(vscode.window.createStatusBarItem).toHaveBeenCalled(); }); it('should notify, and raise no diagnostic, for a registry with no local credential', async () => { const fetch = vi.fn(); - activate(context, { fetch, credentials: noDockerCredentials }); + activate(context, { fetch, credentials: noDockerCredentials, readTextFile: NO_FILES }); const document = createFakeDocument( '/repo/chart/values.yaml', @@ -194,7 +205,7 @@ describe('extension', () => { }); it('should notify once per registry across files, not once per file', async () => { - activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials, readTextFile: NO_FILES }); const values = ['image:', ' repository: private.example.com/svc', ' tag: 1.0.0', ''].join('\n'); @@ -206,10 +217,46 @@ describe('extension', () => { it('should issue no request for a document that is not a values file', async () => { const fetch = vi.fn(); - activate(context, { fetch, credentials: noDockerCredentials }); + activate(context, { fetch, credentials: noDockerCredentials, readTextFile: NO_FILES }); await emitDidOpenTextDocument(createFakeDocument('/repo/chart/deployment.yaml', VALUES_YAML)); expect(fetch).not.toHaveBeenCalled(); }); + + it('should watch chart metadata under both spellings on activate', () => { + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials, readTextFile: NO_FILES }); + + expect(getLastFileSystemWatcher()?.globPattern).toBe('**/Chart.{yaml,yml}'); + }); + + it('should re-check the documents a chart governs when its metadata changes, and leave the others alone', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); + activate(context, { + fetch, + credentials: noDockerCredentials, + readTextFile: createFakeFileSystem({ + '/repo/chart/Chart.yaml': ['apiVersion: v2', 'name: my-service', `appVersion: ${TAG}`, ''].join('\n'), + '/repo/other/Chart.yaml': ['apiVersion: v2', 'name: other', 'appVersion: 2.0.0', ''].join('\n'), + }), + }); + + const governed = createFakeDocument('/repo/chart/values.yaml', TAGLESS_VALUES_YAML); + const unrelated = createFakeDocument( + '/repo/other/values.yaml', + ['image:', ' repository: docker.io/library/redis', ' pullPolicy: Always', ''].join('\n') + ); + + await emitDidOpenTextDocument(governed); + await emitDidOpenTextDocument(unrelated); + setOpenTextDocuments([governed, unrelated]); + fetch.mockClear(); + + await emitDidChangeChartMetadata({ path: '/repo/chart/Chart.yaml' }); + + // A bumped `appVersion` is exactly when a stale checkmark costs the most, + // and the chart nobody touched has nothing new to be asked about. + expect(fetch).toHaveBeenCalledTimes(1); + expect(fetch).toHaveBeenCalledWith(`https://registry-1.docker.io/v2/library/nginx/manifests/${TAG}`, expect.anything()); + }); }); diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index caeceee..2dea90f 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -1,4 +1,5 @@ import * as vscode from 'vscode'; +import type { ReadTextFile } from 'helm'; import { localDockerCredentials, type CredentialEnvironment, type FetchLike } from 'oci-registry'; import { diagnosticsFor } from './diagnostics'; import { hoverFor } from './hover'; @@ -8,16 +9,42 @@ import { checkImageReferencesInDocument, checksAsOf, registriesNeedingLogin, typ const DIAGNOSTIC_COLLECTION_NAME = 'infra-tools-images'; +// Both spellings, because chart resolution accepts both and a watcher that +// covered only one would leave half the charts in a workspace unwatched. +const CHART_METADATA_GLOB = '**/Chart.{yaml,yml}'; + interface ActivateDependencies { /** Only tests override this; production activation uses the platform's `fetch`. */ readonly fetch?: FetchLike; /** Only tests override this; production activation reads the developer's real Docker config. */ readonly credentials?: CredentialEnvironment; + /** Only tests override this; production activation reads through the workspace file system. */ + readonly readTextFile?: ReadTextFile; +} + +/** + * Reads a workspace file as text, through VS Code's file system rather than + * Node's, so a remote workspace resolves its charts on the machine the + * extension host actually runs on. `Uri.file` pins the scheme to `file`, so + * a virtual workspace — a repository browsed without being cloned — resolves + * no charts at all and falls back to the values-file naming rule. + * + * A read that throws is reported as nothing readable rather than as a + * failure: chart resolution asks about `Chart.yaml` in every ancestor + * directory of a file, so absent is the ordinary answer, not an error. + */ +async function readWorkspaceTextFile(path: string): Promise { + try { + return new TextDecoder().decode(await vscode.workspace.fs.readFile(vscode.Uri.file(path))); + } catch { + return undefined; + } } /** * Called by the extension host on activation. Wires the three surfaces a - * check is shown on and re-checks a Helm values file whenever one opens. + * check is shown on, re-checks a Helm values file whenever one opens, and + * re-checks the files a chart governs whenever its metadata changes. */ function activate(context: vscode.ExtensionContext, dependencies: ActivateDependencies = {}): void { const channel = vscode.window.createOutputChannel('Infra Tools'); @@ -27,6 +54,7 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend const checkDependencies = { fetch: dependencies.fetch ?? (globalThis as unknown as { fetch: FetchLike }).fetch, credentials: dependencies.credentials ?? localDockerCredentials, + readTextFile: dependencies.readTextFile ?? readWorkspaceTextFile, }; const diagnostics = vscode.languages.createDiagnosticCollection(DIAGNOSTIC_COLLECTION_NAME); context.subscriptions.push(diagnostics); @@ -38,30 +66,53 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend const loginPrompts = createLoginPrompts(context.globalState); context.subscriptions.push(loginPrompts); - context.subscriptions.push( - vscode.workspace.onDidOpenTextDocument(async (document) => { - // VS Code never awaits a listener, so anything escaping this callback - // is an unhandled rejection, and that takes the extension host down. - try { - const checked = await checkImageReferencesInDocument(document, checkDependencies); - - if (checked === undefined) { - return; - } - - checksByDocument.set(document.uri.toString(), checked); - diagnostics.set(document.uri, diagnosticsFor(document, checksAsOf(checked, document))); - applyMarks(vscode.window.visibleTextEditors, checksByDocument, markDecorationType); - - // Not awaited: a notification stays up until the developer answers - // it, and holding an open-document listener for that long would tie - // this file's check to a dialog about a registry. - void loginPrompts.report(registriesNeedingLogin(checked.checks)); - } catch (error) { - channel.appendLine(`Checking image references in ${document.uri.toString()} failed: ${String(error)}`); + /** + * Checks one document and republishes everything shown for it. The open + * listener and the chart watcher both go through here, so the two can + * never drift into publishing different things about the same document. + */ + async function checkDocument(document: vscode.TextDocument): Promise { + // VS Code never awaits a listener, so anything escaping this callback + // is an unhandled rejection, and that takes the extension host down. + try { + const checked = await checkImageReferencesInDocument(document, checkDependencies); + + if (checked === undefined) { + return; } - }) - ); + + checksByDocument.set(document.uri.toString(), checked); + diagnostics.set(document.uri, diagnosticsFor(document, checksAsOf(checked, document), vscode.workspace.asRelativePath)); + applyMarks(vscode.window.visibleTextEditors, checksByDocument, markDecorationType); + + // Not awaited: a notification stays up until the developer answers + // it, and holding an open-document listener for that long would tie + // this file's check to a dialog about a registry. + void loginPrompts.report(registriesNeedingLogin(checked.checks)); + } catch (error) { + channel.appendLine(`Checking image references in ${document.uri.toString()} failed: ${String(error)}`); + } + } + + /** + * Re-checks every open document whose last check read this metadata file. + * A bumped `appVersion` otherwise leaves a stale checkmark standing at + * exactly the moment the developer is relying on it. + */ + async function recheckDocumentsGovernedBy(uri: vscode.Uri): Promise { + for (const document of vscode.workspace.textDocuments) { + if (checksByDocument.get(document.uri.toString())?.chartMetadataPath === uri.path) { + await checkDocument(document); + } + } + } + + context.subscriptions.push(vscode.workspace.onDidOpenTextDocument(checkDocument)); + + const chartMetadataWatcher = vscode.workspace.createFileSystemWatcher(CHART_METADATA_GLOB); + context.subscriptions.push(chartMetadataWatcher); + context.subscriptions.push(chartMetadataWatcher.onDidChange(recheckDocumentsGovernedBy)); + context.subscriptions.push(chartMetadataWatcher.onDidCreate(recheckDocumentsGovernedBy)); context.subscriptions.push( vscode.languages.registerHoverProvider( @@ -70,7 +121,7 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend provideHover(document: vscode.TextDocument, position: vscode.Position): vscode.Hover | undefined { const checked = checksByDocument.get(document.uri.toString()); - return checked === undefined ? undefined : hoverFor(document, position, checksAsOf(checked, document)); + return checked === undefined ? undefined : hoverFor(document, position, checksAsOf(checked, document), vscode.workspace.asRelativePath); }, } ) diff --git a/apps/vscode/src/hover.test.ts b/apps/vscode/src/hover.test.ts index adbd2f0..2a52c8a 100644 --- a/apps/vscode/src/hover.test.ts +++ b/apps/vscode/src/hover.test.ts @@ -1,20 +1,22 @@ import type * as vscode from 'vscode'; import { describe, expect, it } from 'vitest'; -import type { SourceRange } from 'helm'; +import type { ResolvedTag, SourceRange } from 'helm'; +import type { ImageVerdict, UnverifiableReason } from 'oci-registry'; import { createFakeDocument } from '../test/fake-document'; import { hoverFor } from './hover'; -import type { ReferenceCheck, ReferenceVerdict, UncheckedReason } from './reference-check'; +import type { ReferenceCheck } from './reference-check'; const REPOSITORY = 'registry.example.com/svc'; const TAG = '1.0'; const VALUES_YAML = ['image:', ` repository: ${REPOSITORY}`, ` tag: "${TAG}"`, ''].join('\n'); +const CHART_METADATA_PATH = '/repo/chart/Chart.yaml'; +const PROVENANCE_SENTENCE = ' Tag taken from appVersion in chart/Chart.yaml.'; -const VERIFIED_VERDICT: ReferenceVerdict = { kind: 'exists', registry: 'mirror.example.com' }; +const VERIFIED_VERDICT: ImageVerdict = { kind: 'exists', registry: 'mirror.example.com' }; // The sentence each reason owes the reader, as a record so a new reason fails // the build here instead of hovering with someone else's explanation. -const UNCHECKED_REASON_SENTENCES: Record = { - 'no-tag': 'the reference names no tag to check.', +const UNCHECKED_REASON_SENTENCES: Record = { 'guessed-registry': 'nothing here names a registry, and Docker Hub — the only one left to try — does not have it.', 'needs-login': 'no local Docker credential for that registry. Run `docker login` against it.', 'authentication-failure': 'the registry refused the local Docker credential for it.', @@ -23,8 +25,13 @@ const UNCHECKED_REASON_SENTENCES: Record = { 'malformed-reference': 'the reference is not a valid image reference.', }; +/** Stands in for `vscode.workspace.asRelativePath`, shortening enough that a hover can be seen to have used it. */ +function describeChartPath(path: string): string { + return path.replace('/repo/', ''); +} + /** The unverifiable verdict a reason produces. Only `needs-login` carries a registry, so the shape cannot be built generically. */ -function unverifiableVerdict(reason: UncheckedReason): ReferenceVerdict { +function unverifiableVerdict(reason: UnverifiableReason): ImageVerdict { return reason === 'needs-login' ? { kind: 'unverifiable', reason, registry: 'registry.example.com' } : { kind: 'unverifiable', reason }; } @@ -36,32 +43,26 @@ function rangeOfText(text: string): SourceRange { } /** A check over the single reference in {@link VALUES_YAML}, carrying that file's real offsets. */ -function createCheck(verdict: ReferenceVerdict): ReferenceCheck { +function createCheck(verdict: ImageVerdict, tag: ResolvedTag = { source: 'file', text: TAG, range: rangeOfText(TAG) }): ReferenceCheck { return { reference: { repository: { text: REPOSITORY, range: rangeOfText(REPOSITORY) }, - tag: { text: TAG, range: rangeOfText(TAG) }, + tag: tag.source === 'file' ? { text: tag.text, range: tag.range } : undefined, registry: undefined, }, + tag, verdict, }; } -/** A check over the same reference written without a tag, which is what a `no-tag` verdict comes from. */ -function createTaglessCheck(verdict: ReferenceVerdict): ReferenceCheck { - return { - reference: { - repository: { text: REPOSITORY, range: rangeOfText(REPOSITORY) }, - tag: undefined, - registry: undefined, - }, - verdict, - }; +/** The same reference written without a tag, checked against the chart's `appVersion` instead. */ +function createChartMetadataCheck(verdict: ImageVerdict): ReferenceCheck { + return createCheck(verdict, { source: 'chart-metadata', text: TAG, metadataPath: CHART_METADATA_PATH }); } /** The hover at the first character of `text`'s occurrence in {@link VALUES_YAML}. */ function hoverAtText(document: vscode.TextDocument, checks: readonly ReferenceCheck[], text: string): vscode.Hover | undefined { - return hoverFor(document, document.positionAt(VALUES_YAML.indexOf(text)), checks); + return hoverFor(document, document.positionAt(VALUES_YAML.indexOf(text)), checks, describeChartPath); } /** The plain text of a hover's single content entry. */ @@ -87,18 +88,30 @@ describe('hover', () => { it('should render its own sentence for every reason a reference went unverified', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); - for (const [reason, sentence] of Object.entries(UNCHECKED_REASON_SENTENCES) as [UncheckedReason, string][]) { + for (const [reason, sentence] of Object.entries(UNCHECKED_REASON_SENTENCES) as [UnverifiableReason, string][]) { const hoverText = getHoverText(hoverAtText(document, [createCheck(unverifiableVerdict(reason))], REPOSITORY)); expect(hoverText).toBe(`Not verified: ${sentence}`); } }); - it('should explain a tagless reference, which carries no tag to hover in the first place', () => { + it('should name the chart a tag was taken from, on a verified reference and an unverified one alike', () => { + const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); + const verified = createChartMetadataCheck(VERIFIED_VERDICT); + const unverified = createChartMetadataCheck(unverifiableVerdict('network-error')); + + // A checkmark earned by a tag the file never wrote is worth attributing: + // the hover is the only surface that can say where it came from. + expect(getHoverText(hoverAtText(document, [verified], REPOSITORY))).toBe(`Verified on \`mirror.example.com\`.${PROVENANCE_SENTENCE}`); + expect(getHoverText(hoverAtText(document, [unverified], REPOSITORY))).toBe( + `Not verified: ${UNCHECKED_REASON_SENTENCES['network-error']}${PROVENANCE_SENTENCE}` + ); + }); + + it('should say nothing about a chart for a tag the file writes itself', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); - const check = createTaglessCheck({ kind: 'unverifiable', reason: 'no-tag' }); - expect(getHoverText(hoverAtText(document, [check], REPOSITORY))).toContain('no tag'); + expect(getHoverText(hoverAtText(document, [createCheck(VERIFIED_VERDICT)], REPOSITORY))).not.toContain('appVersion'); }); it('should provide no hover for a reference that does not exist, which already speaks through its diagnostic', () => { diff --git a/apps/vscode/src/hover.ts b/apps/vscode/src/hover.ts index 5094767..ac8c836 100644 --- a/apps/vscode/src/hover.ts +++ b/apps/vscode/src/hover.ts @@ -1,11 +1,11 @@ import * as vscode from 'vscode'; -import type { ReferenceCheck, UncheckedReason } from './reference-check'; +import type { UnverifiableReason } from 'oci-registry'; +import { chartProvenanceSentence, type ReferenceCheck } from './reference-check'; import { containsOffset } from './source-range'; // A table rather than a switch, so adding a reason fails the build here // instead of hovering with no explanation. -const UNCHECKED_REASON_TEXT: Record = { - 'no-tag': 'the reference names no tag to check.', +const UNCHECKED_REASON_TEXT: Record = { 'guessed-registry': 'nothing here names a registry, and Docker Hub — the only one left to try — does not have it.', 'needs-login': 'no local Docker credential for that registry. Run `docker login` against it.', 'authentication-failure': 'the registry refused the local Docker credential for it.', @@ -17,8 +17,18 @@ const UNCHECKED_REASON_TEXT: Record = { /** * What hovering a checked reference reports. A not-found verdict gets no * hover: it already speaks through its diagnostic, and it names no registry. + * + * A reference checked against a tag it never wrote is told so here as well + * as in the diagnostic, because a hover is the only surface a verified + * reference has, and a checkmark earned by someone else's tag is worth + * knowing about. */ -function hoverFor(document: vscode.TextDocument, position: vscode.Position, checks: readonly ReferenceCheck[]): vscode.Hover | undefined { +function hoverFor( + document: vscode.TextDocument, + position: vscode.Position, + checks: readonly ReferenceCheck[], + describeChartPath: (path: string) => string +): vscode.Hover | undefined { const offset = document.offsetAt(position); const check = checks.find( ({ reference }) => @@ -29,14 +39,15 @@ function hoverFor(document: vscode.TextDocument, position: vscode.Position, chec return undefined; } - const { verdict } = check; + const { tag, verdict } = check; + const provenance = chartProvenanceSentence(tag, describeChartPath); if (verdict.kind === 'exists') { - return new vscode.Hover(new vscode.MarkdownString(`Verified on \`${verdict.registry}\`.`)); + return new vscode.Hover(new vscode.MarkdownString(`Verified on \`${verdict.registry}\`.${provenance}`)); } if (verdict.kind === 'unverifiable') { - return new vscode.Hover(new vscode.MarkdownString(`Not verified: ${UNCHECKED_REASON_TEXT[verdict.reason]}`)); + return new vscode.Hover(new vscode.MarkdownString(`Not verified: ${UNCHECKED_REASON_TEXT[verdict.reason]}${provenance}`)); } return undefined; diff --git a/apps/vscode/src/marks.test.ts b/apps/vscode/src/marks.test.ts index 3e3377e..03adae9 100644 --- a/apps/vscode/src/marks.test.ts +++ b/apps/vscode/src/marks.test.ts @@ -1,18 +1,22 @@ import * as vscode from 'vscode'; import { describe, expect, it, vi } from 'vitest'; -import type { SourceRange } from 'helm'; +import type { ResolvedTag, SourceRange } from 'helm'; +import type { ImageVerdict } from 'oci-registry'; import { bumpVersion, createFakeDocument } from '../test/fake-document'; import { createTextEditorStub, type TextEditorStub } from '../test/vscode-stub'; import { applyMarks, createMarkDecorationType, marksFor } from './marks'; -import type { DocumentChecks, ReferenceCheck, ReferenceVerdict } from './reference-check'; +import type { DocumentChecks, ReferenceCheck } from './reference-check'; const REPOSITORY = 'registry.example.com/svc'; const TAG = '1.0'; const VALUES_YAML = ['image:', ` repository: ${REPOSITORY}`, ` tag: "${TAG}"`, ''].join('\n'); -const VERIFIED_VERDICT: ReferenceVerdict = { kind: 'exists', registry: 'registry.example.com' }; -const MISSING_VERDICT: ReferenceVerdict = { kind: 'tag-not-found', repository: REPOSITORY, tag: TAG }; -const UNCHECKED_VERDICT: ReferenceVerdict = { kind: 'unverifiable', reason: 'network-error' }; +const VERIFIED_VERDICT: ImageVerdict = { kind: 'exists', registry: 'registry.example.com' }; +const MISSING_VERDICT: ImageVerdict = { kind: 'tag-not-found', repository: REPOSITORY, tag: TAG }; +const UNCHECKED_VERDICT: ImageVerdict = { kind: 'unverifiable', reason: 'network-error' }; + +/** A tagless reference's tag comes from the governing chart, which is the only way one is checked at all. */ +const CHART_METADATA_TAG: ResolvedTag = { source: 'chart-metadata', text: TAG, metadataPath: '/repo/chart/Chart.yaml' }; /** The source range of `text`'s first occurrence in {@link VALUES_YAML}. */ function rangeOfText(text: string): SourceRange { @@ -22,13 +26,14 @@ function rangeOfText(text: string): SourceRange { } /** A check over the single reference in {@link VALUES_YAML}, carrying that file's real offsets. */ -function createCheck(verdict: ReferenceVerdict): ReferenceCheck { +function createCheck(verdict: ImageVerdict): ReferenceCheck { return { reference: { repository: { text: REPOSITORY, range: rangeOfText(REPOSITORY) }, tag: { text: TAG, range: rangeOfText(TAG) }, registry: undefined, }, + tag: { source: 'file', text: TAG, range: rangeOfText(TAG) }, verdict, }; } @@ -40,7 +45,7 @@ function asEditors(editors: readonly TextEditorStub[]): readonly vscode.TextEdit /** One document's stored checks, tagged with the version it currently has. */ function createChecksByDocument(document: vscode.TextDocument, checks: readonly ReferenceCheck[]): Map { - return new Map([[document.uri.toString(), { version: document.version, checks }]]); + return new Map([[document.uri.toString(), { version: document.version, checks, chartMetadataPath: undefined }]]); } /** An editor that throws the way a disposed one does. */ @@ -92,7 +97,7 @@ describe('marks', () => { registry: 'ghcr.io', }; - const [mark] = marksFor(document, [{ reference, verdict: { kind: 'exists', registry: 'ghcr.io' } }]); + const [mark] = marksFor(document, [{ reference, tag: CHART_METADATA_TAG, verdict: { kind: 'exists', registry: 'ghcr.io' } }]); // The file says `ghcr.io` two lines up. Appending it restates the file. expect(mark?.renderOptions?.after?.contentText).toBe(' ✓'); @@ -107,7 +112,7 @@ describe('marks', () => { registry: undefined, }; - const [mark] = marksFor(document, [{ reference, verdict: { kind: 'exists', registry: 'docker.io' } }]); + const [mark] = marksFor(document, [{ reference, tag: CHART_METADATA_TAG, verdict: { kind: 'exists', registry: 'docker.io' } }]); // A file that spells out no registry cannot have the answer restated to // it, so naming Docker Hub is the only way the mark says where it looked. diff --git a/apps/vscode/src/marks.ts b/apps/vscode/src/marks.ts index 42a2aaf..47222c3 100644 --- a/apps/vscode/src/marks.ts +++ b/apps/vscode/src/marks.ts @@ -1,7 +1,7 @@ import * as vscode from 'vscode'; import type { ImageReference } from 'helm'; -import { resolveExplicitHost } from 'oci-registry'; -import { checksAsOf, type DocumentChecks, type ReferenceCheck, type ReferenceVerdict } from './reference-check'; +import { resolveExplicitHost, type ImageVerdict } from 'oci-registry'; +import { checksAsOf, type DocumentChecks, type ReferenceCheck } from './reference-check'; import { rangeOf } from './source-range'; type MarkKind = 'verified' | 'missing' | 'unchecked'; @@ -16,7 +16,7 @@ const MARKS: Record { - const checked = await checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials }); +async function checkValuesFile(document: vscode.TextDocument, fetch: FetchLike, readTextFile: ReadTextFile = NO_FILES): Promise { + const checked = await checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials, readTextFile }); if (checked === undefined) { throw new Error('expected the document to be checked'); @@ -30,12 +42,13 @@ async function checkValuesFile(document: vscode.TextDocument, fetch: FetchLike): } describe('reference-check', () => { - it('should ignore a document that is not the conventional values file name', async () => { + it('should ignore a document that is neither named as a values file nor sits under a chart', async () => { const fetch = vi.fn(); const document = createFakeDocument('/repo/chart/deployment.yaml', VALUES_YAML); - expect(isHelmValuesFile(document)).toBe(false); - await expect(checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials })).resolves.toBeUndefined(); + await expect( + checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials, readTextFile: NO_FILES }) + ).resolves.toBeUndefined(); expect(fetch).not.toHaveBeenCalled(); }); @@ -43,8 +56,29 @@ describe('reference-check', () => { const fetch = vi.fn(); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML, 'plaintext'); - expect(isHelmValuesFile(document)).toBe(false); - await expect(checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials })).resolves.toBeUndefined(); + await expect( + checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials, readTextFile: NO_FILES }) + ).resolves.toBeUndefined(); + expect(fetch).not.toHaveBeenCalled(); + }); + + it('should check any yaml file beneath a chart directory, whatever it is named', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); + const document = createFakeDocument('/repo/chart/production.yaml', VALUES_YAML); + + const { checks } = await checkValuesFile(document, fetch, ONE_CHART); + + expect(checks).toHaveLength(1); + expect(fetch).toHaveBeenCalledTimes(1); + }); + + it('should ignore a file under the chart templates directory, where nothing renders to a real reference', async () => { + const fetch = vi.fn(); + const document = createFakeDocument('/repo/chart/templates/values.yaml', VALUES_YAML); + + await expect( + checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials, readTextFile: ONE_CHART }) + ).resolves.toBeUndefined(); expect(fetch).not.toHaveBeenCalled(); }); @@ -55,21 +89,75 @@ describe('reference-check', () => { // The extractor collects YAML errors instead of throwing, so a malformed // file yields zero references rather than the `undefined` that means this // feature has nothing to say about the document. - await expect(checkValuesFile(document, fetch)).resolves.toEqual({ version: document.version, checks: [] }); + await expect(checkValuesFile(document, fetch)).resolves.toEqual({ version: document.version, checks: [], chartMetadataPath: undefined }); expect(fetch).not.toHaveBeenCalled(); }); - it('should ask no registry about a tagless reference, and report it as unverifiable', async () => { + it('should check a tagless reference against the governing chart appVersion', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); + const document = createFakeDocument('/repo/chart/values.yaml', TAGLESS_VALUES_YAML); + + await checkValuesFile(document, fetch, ONE_CHART); + + expect(fetch).toHaveBeenCalledWith('https://ghcr.io/v2/my-org/my-service/manifests/1.2.3', expect.anything()); + }); + + it('should record where a tag taken from chart metadata came from', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); + const document = createFakeDocument('/repo/chart/values.yaml', TAGLESS_VALUES_YAML); + + const { checks } = await checkValuesFile(document, fetch, ONE_CHART); + + expect(checks[0]?.tag).toEqual({ source: 'chart-metadata', text: '1.2.3', metadataPath: '/repo/chart/Chart.yaml' }); + }); + + it('should resolve a subchart values file against the subchart own appVersion, not the parent chart', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); + const document = createFakeDocument('/repo/chart/charts/sub/values.yaml', TAGLESS_VALUES_YAML); + const readTextFile = createFakeFileSystem({ + '/repo/chart/Chart.yaml': CHART_YAML, + '/repo/chart/charts/sub/Chart.yaml': SUBCHART_YAML, + }); + + await checkValuesFile(document, fetch, readTextFile); + + // The parent's 1.2.3 is what Helm would ignore here, so asking about it + // would report on a release nobody is deploying. + expect(fetch).toHaveBeenCalledWith('https://ghcr.io/v2/my-org/my-service/manifests/4.5.6', expect.anything()); + }); + + it('should produce no check at all for a tagless reference no chart metadata can resolve', async () => { const fetch = vi.fn(); const document = createFakeDocument('/repo/chart/values.yaml', TAGLESS_VALUES_YAML); const { checks } = await checkValuesFile(document, fetch); - expect(checks).toHaveLength(1); - expect(checks[0]?.verdict).toEqual({ kind: 'unverifiable', reason: 'no-tag' }); + // Silence, not an unchecked mark: there is nothing to check it against + // and nothing the developer could do about a mark saying so. + expect(checks).toEqual([]); + expect(fetch).not.toHaveBeenCalled(); + }); + + it('should produce no check for a tagless reference whose governing chart declares no appVersion', async () => { + const fetch = vi.fn(); + const document = createFakeDocument('/repo/chart/values.yaml', TAGLESS_VALUES_YAML); + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': APPVERSIONLESS_CHART_YAML }); + + const { checks } = await checkValuesFile(document, fetch, readTextFile); + + expect(checks).toEqual([]); expect(fetch).not.toHaveBeenCalled(); }); + it('should report the chart metadata file the checks depend on', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); + const governed = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); + const standalone = createFakeDocument('/elsewhere/values.yaml', VALUES_YAML); + + await expect(checkValuesFile(governed, fetch, ONE_CHART)).resolves.toMatchObject({ chartMetadataPath: '/repo/chart/Chart.yaml' }); + await expect(checkValuesFile(standalone, fetch)).resolves.toMatchObject({ chartMetadataPath: undefined }); + }); + it('should return the verdict the registry answer implies for a tagged reference', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); @@ -78,10 +166,24 @@ describe('reference-check', () => { expect(checks).toHaveLength(1); expect(checks[0]?.reference.repository.text).toBe(REPOSITORY); + expect(checks[0]?.tag).toEqual({ + source: 'file', + text: TAG, + range: { start: VALUES_YAML.indexOf(TAG), end: VALUES_YAML.indexOf(TAG) + TAG.length }, + }); expect(checks[0]?.verdict).toEqual({ kind: 'exists', registry: 'docker.io' }); expect(fetch).toHaveBeenCalledTimes(1); }); + it('should prefer the tag the file writes over the governing chart appVersion', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); + const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); + + await checkValuesFile(document, fetch, ONE_CHART); + + expect(fetch).toHaveBeenCalledWith(`https://registry-1.docker.io/v2/library/nginx/manifests/${TAG}`, expect.anything()); + }); + it('should check a bare repository against the registry its document declares', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); const document = createFakeDocument('/repo/chart/values.yaml', DECLARED_REGISTRY_VALUES_YAML); diff --git a/apps/vscode/src/reference-check.ts b/apps/vscode/src/reference-check.ts index c657eff..041201e 100644 --- a/apps/vscode/src/reference-check.ts +++ b/apps/vscode/src/reference-check.ts @@ -1,53 +1,33 @@ import type * as vscode from 'vscode'; -import { extractImageReferences, type ImageReference } from 'helm'; -import { checkImageExistence, type CredentialEnvironment, type FetchLike, type ImageVerdict, type UnverifiableReason } from 'oci-registry'; - -// Matching any YAML file beneath a chart directory, and excluding that -// chart's templates directory, is Helm chart-context knowledge this ticket -// doesn't implement yet. -const VALUES_FILE_NAME_PATTERN = /^values\.ya?ml$/i; - -/** Every reason the registry package reports, plus the ones settled before asking it. */ -type UncheckedReason = UnverifiableReason | 'no-tag'; - -/** - * A registry verdict, or the one outcome the extension settles itself. - * - * `ImageVerdict` is taken whole rather than picked apart, so the registry - * package's own split — only `'needs-login'` carries a host — survives the - * trip to the UI instead of being flattened into an optional field the - * notification code would then have to re-check. - */ -type ReferenceVerdict = ImageVerdict | { readonly kind: 'unverifiable'; readonly reason: 'no-tag' }; +import { extractImageReferences, resolveTag, resolveValuesFileContext, type ImageReference, type ReadTextFile, type ResolvedTag } from 'helm'; +import { checkImageExistence, type CredentialEnvironment, type FetchLike, type ImageVerdict } from 'oci-registry'; /** * One checked reference, projected onto all three surfaces, so diagnostics, * marks, and hovers can never disagree about a reference. + * + * The resolved tag rides along because it is not always in the file: a tag + * taken from `appVersion` has a chart to name and no range to underline, and + * both of those are decisions a surface has to make per reference. */ interface ReferenceCheck { readonly reference: ImageReference; - readonly verdict: ReferenceVerdict; + readonly tag: ResolvedTag; + readonly verdict: ImageVerdict; } /** A document's checks, tagged with the document version they describe. */ interface DocumentChecks { readonly version: number; readonly checks: readonly ReferenceCheck[]; -} - -function isHelmValuesFile(document: vscode.TextDocument): boolean { - if (document.languageId !== 'yaml') { - return false; - } - - const fileName = document.uri.path.split('/').pop() ?? ''; - - return VALUES_FILE_NAME_PATTERN.test(fileName); + /** The metadata file whose `appVersion` these checks may depend on. */ + readonly chartMetadataPath: string | undefined; } interface CheckDependencies { readonly fetch: FetchLike; readonly credentials: CredentialEnvironment; + readonly readTextFile: ReadTextFile; } /** @@ -55,13 +35,23 @@ interface CheckDependencies { * nothing to say about the document at all, which is not the same as a * checked document that produced no findings: the caller replaces a * document's diagnostics and marks only when it gets checks back. + * + * Which documents those are is the Helm package's call, not this file's — a + * values file is anything beneath a chart directory, and that is chart + * knowledge a future CLI would otherwise have to reimplement. */ async function checkImageReferencesInDocument(document: vscode.TextDocument, dependencies: CheckDependencies): Promise { - if (!isHelmValuesFile(document)) { + if (document.languageId !== 'yaml') { + return undefined; + } + + const { fetch, credentials, readTextFile } = dependencies; + const context = await resolveValuesFileContext(document.uri.path, readTextFile); + + if (context === undefined) { return undefined; } - const { fetch, credentials } = dependencies; const version = document.version; // No guard around this: the extractor collects YAML syntax errors rather @@ -69,26 +59,41 @@ async function checkImageReferencesInDocument(document: vscode.TextDocument, dep // open listener's own catch covers a genuine fault. const references = extractImageReferences(document.getText()); - // A tagless reference has nothing to ask a registry until `appVersion` - // resolution lands, but it still comes through as a check. Dropping it is - // what made real references render nothing, which reads as a broken tool. + // A reference that resolves to no tag at all is dropped here rather than + // carried as an outcome: there is nothing to ask a registry and nothing + // truthful to render, and a mark that says only "unchecked" on a file the + // developer cannot act on is noise they would switch the feature off over. + const resolved = references.flatMap((reference) => { + const tag = resolveTag(reference, context.chart); + + return tag === undefined ? [] : [{ reference, tag }]; + }); + const checks = await Promise.all( - references.map(async (reference) => ({ + resolved.map(async ({ reference, tag }) => ({ reference, - verdict: - reference.tag === undefined - ? ({ kind: 'unverifiable', reason: 'no-tag' } as const) - : await checkImageExistence({ - repository: reference.repository.text, - tag: reference.tag.text, - declaredRegistry: reference.registry, - fetch, - credentials, - }), + tag, + verdict: await checkImageExistence({ + repository: reference.repository.text, + tag: tag.text, + declaredRegistry: reference.registry, + fetch, + credentials, + }), })) ); - return { version, checks }; + return { version, checks, chartMetadataPath: context.chart?.path }; +} + +/** + * The sentence a message owes the reader when the tag it reports was never + * written in the file. Shared by the diagnostic and the hover so the two + * cannot word the same provenance differently, and empty for a tag the file + * wrote, which needs no explaining. + */ +function chartProvenanceSentence(tag: ResolvedTag, describeChartPath: (path: string) => string): string { + return tag.source === 'file' ? '' : ` Tag taken from appVersion in ${describeChartPath(tag.metadataPath)}.`; } /** @@ -120,5 +125,5 @@ function registriesNeedingLogin(checks: readonly ReferenceCheck[]): string[] { return [...registries]; } -export { checkImageReferencesInDocument, checksAsOf, isHelmValuesFile, registriesNeedingLogin }; -export type { DocumentChecks, ReferenceCheck, ReferenceVerdict, UncheckedReason }; +export { chartProvenanceSentence, checkImageReferencesInDocument, checksAsOf, registriesNeedingLogin }; +export type { DocumentChecks, ReferenceCheck }; diff --git a/apps/vscode/test/fake-file-system.ts b/apps/vscode/test/fake-file-system.ts new file mode 100644 index 0000000..b50637b --- /dev/null +++ b/apps/vscode/test/fake-file-system.ts @@ -0,0 +1,16 @@ +import type { ReadTextFile } from 'helm'; + +/** + * A filesystem described as a path-to-text map. + * + * Chart resolution walks ancestor directories looking for metadata, so a + * test's subject is the shape of a directory tree. Writing that shape as a + * literal keeps it visible in the test that depends on it, where temporary + * files would put it somewhere the reader has to go and find. + */ +function createFakeFileSystem(files: Record): ReadTextFile { + // eslint-disable-next-line @typescript-eslint/promise-function-async -- a canned lookup, nothing to await + return (path: string) => Promise.resolve(files[path]); +} + +export { createFakeFileSystem }; diff --git a/apps/vscode/test/vscode-stub.ts b/apps/vscode/test/vscode-stub.ts index 6195326..53d6ca1 100644 --- a/apps/vscode/test/vscode-stub.ts +++ b/apps/vscode/test/vscode-stub.ts @@ -283,8 +283,53 @@ function getLastTerminal(): TerminalStub | undefined { return terminals[terminals.length - 1]; } +/** + * Test-only stand-in for `vscode.FileSystemWatcher`. The glob it was created + * with is recorded, because which files a watcher covers is as much of the + * extension's behaviour as what it does when one changes. Each of the three + * events is a separate emitter, so a test firing a change never drives a + * create listener that would have re-run the same work. + */ +interface FileSystemWatcherStub { + readonly globPattern: string; + readonly onDidChange: (listener: (uri: unknown) => unknown) => { dispose: () => void }; + readonly onDidCreate: (listener: (uri: unknown) => unknown) => { dispose: () => void }; + readonly onDidDelete: (listener: (uri: unknown) => unknown) => { dispose: () => void }; + readonly dispose: ReturnType; + /** Test-only: drives the change listeners and awaits them. Not part of the real `vscode` API. */ + readonly fireDidChange: (uri: unknown) => Promise; + /** Test-only: drives the create listeners and awaits them. Not part of the real `vscode` API. */ + readonly fireDidCreate: (uri: unknown) => Promise; +} + +const fileSystemWatchers: FileSystemWatcherStub[] = []; + +function createFileSystemWatcherStub(globPattern: string): FileSystemWatcherStub { + const didChange = createEventEmitterStub(); + const didCreate = createEventEmitterStub(); + const didDelete = createEventEmitterStub(); + const watcher: FileSystemWatcherStub = { + globPattern, + onDidChange: didChange.event, + onDidCreate: didCreate.event, + onDidDelete: didDelete.event, + dispose: vi.fn(), + fireDidChange: didChange.fire, + fireDidCreate: didCreate.fire, + }; + + fileSystemWatchers.push(watcher); + + return watcher; +} + const workspace = { onDidOpenTextDocument: onDidOpenTextDocumentEmitter.event, + textDocuments: [] as readonly unknown[], + // The real one shortens a path against the open workspace folders. There + // are none here, and a test asserting on a message wants the path it wrote. + asRelativePath: vi.fn((path: string) => path), + createFileSystemWatcher: vi.fn((globPattern: string) => createFileSystemWatcherStub(globPattern)), }; /** @@ -296,6 +341,29 @@ async function emitDidOpenTextDocument(document: unknown): Promise { await onDidOpenTextDocumentEmitter.fire(document); } +/** + * Test-only helper that replaces `workspace.textDocuments`. Not part of the + * real `vscode` API — tests set it to stage which documents the editor holds + * open, and reset it so one test's documents never leak into another. + */ +function setOpenTextDocuments(documents: readonly unknown[]): void { + workspace.textDocuments = documents; +} + +/** Test-only helper: the most recently created file system watcher. */ +function getLastFileSystemWatcher(): FileSystemWatcherStub | undefined { + return fileSystemWatchers[fileSystemWatchers.length - 1]; +} + +/** + * Test-only helper that fires the most recent watcher's `onDidChange` and + * awaits every registered listener, standing in for a developer saving a + * chart's metadata file. + */ +async function emitDidChangeChartMetadata(uri: unknown): Promise { + await getLastFileSystemWatcher()?.fireDidChange(uri); +} + /** * Test-only helper that replaces `window.visibleTextEditors`. Not part of the * real `vscode` API — tests set it to stage which editors the extension can @@ -314,14 +382,16 @@ async function emitDidChangeVisibleTextEditors(editors: readonly TextEditorStub[ await onDidChangeVisibleTextEditorsEmitter.fire(editors); } -export type { StatusBarItemStub, TerminalStub, TextEditorStub }; +export type { FileSystemWatcherStub, StatusBarItemStub, TerminalStub, TextEditorStub }; export { createTextEditorStub, Diagnostic, DiagnosticSeverity, + emitDidChangeChartMetadata, emitDidChangeVisibleTextEditors, emitDidOpenTextDocument, getLastDiagnosticCollection, + getLastFileSystemWatcher, getLastStatusBarItem, getLastTerminal, getRegisteredHoverProvider, @@ -330,6 +400,7 @@ export { MarkdownString, Position, Range, + setOpenTextDocuments, setVisibleTextEditors, setWarningMessageAnswer, StatusBarAlignment, From d642bb0294950abbdd92ab3d01dfe663e5cb964d Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:57:15 +0300 Subject: [PATCH 4/6] refactor(helm): narrow chart metadata to the one name Helm loads Accepting `Chart.yml` alongside `Chart.yaml` resolved an `appVersion` out of a file Helm's own loader ignores, contradicting the reason chart context exists: to answer with what Helm would actually deploy. Dropping it also halves the filesystem probes per ancestor directory, and leaves a single exported constant a watcher can be built from rather than a list another workspace has to spell out again. A chart's own `Chart.yaml` also came back in scope as a values file, since the exclusion test only looked for a `templates` segment. It declares a chart rather than supplying values to one. Co-Authored-By: Claude Opus 5 (1M context) --- packages/helm/src/chart-context.test.ts | 13 ++++++--- packages/helm/src/chart-context.ts | 35 +++++++++++-------------- packages/helm/src/index.ts | 2 +- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/packages/helm/src/chart-context.test.ts b/packages/helm/src/chart-context.test.ts index 27ce667..650e005 100644 --- a/packages/helm/src/chart-context.test.ts +++ b/packages/helm/src/chart-context.test.ts @@ -51,13 +51,20 @@ describe('resolveValuesFileContext', () => { expect(context?.chart?.appVersion).toBe('2.0.0'); }); - it('should find chart metadata written as Chart.yml', async () => { + it('should ignore metadata written as Chart.yml, which Helm itself would not load', async () => { const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yml': CHART_METADATA }); const context = await resolveValuesFileContext('/repo/chart/values.yaml', readTextFile); - expect(context?.chart?.path).toBe('/repo/chart/Chart.yml'); - expect(context?.chart?.appVersion).toBe('1.2.3'); + // In scope through the filename fallback, but governed by no chart: an + // `appVersion` Helm would never read is worse than none at all. + expect(context).toEqual({ chart: undefined }); + }); + + it('should exclude a chart own metadata file, which declares a chart rather than supplying values', async () => { + const readTextFile = createFakeFileSystem({ '/repo/chart/Chart.yaml': CHART_METADATA }); + + await expect(resolveValuesFileContext('/repo/chart/Chart.yaml', readTextFile)).resolves.toBeUndefined(); }); it('should treat any yaml file beneath a chart directory as in scope', async () => { diff --git a/packages/helm/src/chart-context.ts b/packages/helm/src/chart-context.ts index 91db861..3754ae8 100644 --- a/packages/helm/src/chart-context.ts +++ b/packages/helm/src/chart-context.ts @@ -29,11 +29,12 @@ type ResolvedTag = | { readonly source: 'chart-metadata'; readonly text: string; readonly metadataPath: string }; /** - * The names chart metadata goes by, in the order they are tried. Helm's own - * convention is `Chart.yaml`, and `Chart.yml` turns up often enough that the - * second spelling is a new row here rather than a new branch below. + * The one name chart metadata goes by. Helm's own loader recognises + * `Chart.yaml` and nothing else, so accepting a second spelling would + * resolve an `appVersion` out of a file Helm would ignore — the opposite of + * what this module exists to do. */ -const CHART_METADATA_FILE_NAMES: readonly string[] = ['Chart.yaml', 'Chart.yml']; +const CHART_METADATA_FILE_NAME = 'Chart.yaml'; /** The chart subdirectory holding Go templates, whose syntax yields nothing checkable. */ const TEMPLATES_DIRECTORY_NAME = 'templates'; @@ -42,7 +43,7 @@ const TEMPLATES_DIRECTORY_NAME = 'templates'; * The names a values file goes by when no chart vouches for it — outside a * chart directory the name is the only evidence there is. */ -const STANDALONE_VALUES_FILE_NAME = /^values\.ya?ml$/i; +const STANDALONE_VALUES_FILE_NAME_PATTERN = /^values\.ya?ml$/i; /** * Resolves the chart governing a YAML file, and with it whether this package @@ -57,7 +58,9 @@ const STANDALONE_VALUES_FILE_NAME = /^values\.ya?ml$/i; * * A file under the chart's `templates/` directory is out of scope outright, * even one named `values.yaml`. It is Go template source, and template - * syntax resolves at render time, which this package never does. + * syntax resolves at render time, which this package never does. So is the + * chart's own metadata file, which declares a chart rather than supplying + * values to one. * * With no chart anywhere above it, a file is in scope only when its own name * says it is a values file — that keeps a standalone values file working @@ -75,15 +78,15 @@ async function resolveValuesFileContext(path: string, readTextFile: ReadTextFile const chart = await readChartMetadata(segments.slice(0, depth), readTextFile); if (chart !== undefined) { - const firstSegmentBelowChart = segments[depth]; + const isUnderTemplates = segments[depth] === TEMPLATES_DIRECTORY_NAME; - return firstSegmentBelowChart === TEMPLATES_DIRECTORY_NAME ? undefined : { chart }; + return isUnderTemplates || chart.path === path ? undefined : { chart }; } } const fileName = segments[segments.length - 1]; - return fileName !== undefined && STANDALONE_VALUES_FILE_NAME.test(fileName) ? { chart: undefined } : undefined; + return fileName !== undefined && STANDALONE_VALUES_FILE_NAME_PATTERN.test(fileName) ? { chart: undefined } : undefined; } /** @@ -95,16 +98,10 @@ async function resolveValuesFileContext(path: string, readTextFile: ReadTextFile * `//Chart.yaml`. */ async function readChartMetadata(directorySegments: readonly string[], readTextFile: ReadTextFile): Promise { - for (const fileName of CHART_METADATA_FILE_NAMES) { - const metadataPath = [...directorySegments, fileName].join('/'); - const text = await readTextFile(metadataPath); + const metadataPath = [...directorySegments, CHART_METADATA_FILE_NAME].join('/'); + const text = await readTextFile(metadataPath); - if (text !== undefined) { - return { path: metadataPath, appVersion: readAppVersion(text) }; - } - } - - return undefined; + return text === undefined ? undefined : { path: metadataPath, appVersion: readAppVersion(text) }; } /** @@ -138,5 +135,5 @@ function resolveTag(reference: ImageReference, chart: ChartMetadata | undefined) return undefined; } -export { resolveTag, resolveValuesFileContext }; +export { CHART_METADATA_FILE_NAME, resolveTag, resolveValuesFileContext }; export type { ChartMetadata, ReadTextFile, ResolvedTag, ValuesFileContext }; diff --git a/packages/helm/src/index.ts b/packages/helm/src/index.ts index d413d19..fbc0f1a 100644 --- a/packages/helm/src/index.ts +++ b/packages/helm/src/index.ts @@ -1,4 +1,4 @@ -export { resolveTag, resolveValuesFileContext } from './chart-context'; +export { CHART_METADATA_FILE_NAME, resolveTag, resolveValuesFileContext } from './chart-context'; export type { ChartMetadata, ReadTextFile, ResolvedTag, ValuesFileContext } from './chart-context'; export { extractImageReferences } from './extract-image-references'; export type { ImageReference } from './extract-image-references'; From ad63a94fd27376079cedaf76acc8250768a027c0 Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:57:25 +0300 Subject: [PATCH 5/6] refactor(vscode): build the chart watcher from the name resolution reads The watcher glob spelled out the chart metadata names a second time, in a different workspace from the resolver that reads them. A future third spelling would have left the watcher silently under-covering, which shows up as nothing happening. It is derived from the resolver's constant now. The `onDidCreate` registration could match nothing. A document checked without a chart records no metadata path, so a chart appearing above it never re-checks it. Catching that means re-resolving every open document on every write, which no ticket has asked for, so the dead registration goes and the limitation is written down on the handler instead. `chartProvenanceSentence` moves out of the module that produces checks and into its own, beside `source-range`, the established home for a helper the diagnostic and hover surfaces share. Co-Authored-By: Claude Opus 5 (1M context) --- apps/vscode/src/diagnostics.ts | 3 ++- apps/vscode/src/extension.test.ts | 4 ++-- apps/vscode/src/extension.ts | 15 ++++++++++----- apps/vscode/src/hover.ts | 3 ++- apps/vscode/src/reference-check.ts | 12 +----------- apps/vscode/src/tag-provenance.ts | 16 ++++++++++++++++ apps/vscode/test/vscode-stub.ts | 3 --- 7 files changed, 33 insertions(+), 23 deletions(-) create mode 100644 apps/vscode/src/tag-provenance.ts diff --git a/apps/vscode/src/diagnostics.ts b/apps/vscode/src/diagnostics.ts index 1db384e..6644d9d 100644 --- a/apps/vscode/src/diagnostics.ts +++ b/apps/vscode/src/diagnostics.ts @@ -1,6 +1,7 @@ import * as vscode from 'vscode'; -import { chartProvenanceSentence, type ReferenceCheck } from './reference-check'; +import type { ReferenceCheck } from './reference-check'; import { rangeOf } from './source-range'; +import { chartProvenanceSentence } from './tag-provenance'; /** * The diagnostics a document's checks call for. Only the two not-found diff --git a/apps/vscode/src/extension.test.ts b/apps/vscode/src/extension.test.ts index 64ca237..29ce58c 100644 --- a/apps/vscode/src/extension.test.ts +++ b/apps/vscode/src/extension.test.ts @@ -224,10 +224,10 @@ describe('extension', () => { expect(fetch).not.toHaveBeenCalled(); }); - it('should watch chart metadata under both spellings on activate', () => { + it('should watch the chart metadata name chart resolution actually reads', () => { activate(context, { fetch: vi.fn(), credentials: noDockerCredentials, readTextFile: NO_FILES }); - expect(getLastFileSystemWatcher()?.globPattern).toBe('**/Chart.{yaml,yml}'); + expect(getLastFileSystemWatcher()?.globPattern).toBe('**/Chart.yaml'); }); it('should re-check the documents a chart governs when its metadata changes, and leave the others alone', async () => { diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index 2dea90f..34b23a6 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -1,5 +1,5 @@ import * as vscode from 'vscode'; -import type { ReadTextFile } from 'helm'; +import { CHART_METADATA_FILE_NAME, type ReadTextFile } from 'helm'; import { localDockerCredentials, type CredentialEnvironment, type FetchLike } from 'oci-registry'; import { diagnosticsFor } from './diagnostics'; import { hoverFor } from './hover'; @@ -9,9 +9,10 @@ import { checkImageReferencesInDocument, checksAsOf, registriesNeedingLogin, typ const DIAGNOSTIC_COLLECTION_NAME = 'infra-tools-images'; -// Both spellings, because chart resolution accepts both and a watcher that -// covered only one would leave half the charts in a workspace unwatched. -const CHART_METADATA_GLOB = '**/Chart.{yaml,yml}'; +// Built from the name chart resolution looks for, rather than spelled out +// again here: a watcher covering a different set of files than the resolver +// reads is a staleness bug that shows up as nothing happening. +const CHART_METADATA_GLOB = `**/${CHART_METADATA_FILE_NAME}`; interface ActivateDependencies { /** Only tests override this; production activation uses the platform's `fetch`. */ @@ -98,6 +99,11 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend * Re-checks every open document whose last check read this metadata file. * A bumped `appVersion` otherwise leaves a stale checkmark standing at * exactly the moment the developer is relying on it. + * + * Only a document that already resolved through this chart qualifies. A + * chart appearing above a file checked without one would also change that + * file's answer, but catching it means re-resolving every open document on + * every write, and no ticket has asked for it yet. */ async function recheckDocumentsGovernedBy(uri: vscode.Uri): Promise { for (const document of vscode.workspace.textDocuments) { @@ -112,7 +118,6 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend const chartMetadataWatcher = vscode.workspace.createFileSystemWatcher(CHART_METADATA_GLOB); context.subscriptions.push(chartMetadataWatcher); context.subscriptions.push(chartMetadataWatcher.onDidChange(recheckDocumentsGovernedBy)); - context.subscriptions.push(chartMetadataWatcher.onDidCreate(recheckDocumentsGovernedBy)); context.subscriptions.push( vscode.languages.registerHoverProvider( diff --git a/apps/vscode/src/hover.ts b/apps/vscode/src/hover.ts index ac8c836..422cb26 100644 --- a/apps/vscode/src/hover.ts +++ b/apps/vscode/src/hover.ts @@ -1,7 +1,8 @@ import * as vscode from 'vscode'; import type { UnverifiableReason } from 'oci-registry'; -import { chartProvenanceSentence, type ReferenceCheck } from './reference-check'; +import type { ReferenceCheck } from './reference-check'; import { containsOffset } from './source-range'; +import { chartProvenanceSentence } from './tag-provenance'; // A table rather than a switch, so adding a reason fails the build here // instead of hovering with no explanation. diff --git a/apps/vscode/src/reference-check.ts b/apps/vscode/src/reference-check.ts index 041201e..b8f1af3 100644 --- a/apps/vscode/src/reference-check.ts +++ b/apps/vscode/src/reference-check.ts @@ -86,16 +86,6 @@ async function checkImageReferencesInDocument(document: vscode.TextDocument, dep return { version, checks, chartMetadataPath: context.chart?.path }; } -/** - * The sentence a message owes the reader when the tag it reports was never - * written in the file. Shared by the diagnostic and the hover so the two - * cannot word the same provenance differently, and empty for a tag the file - * wrote, which needs no explaining. - */ -function chartProvenanceSentence(tag: ResolvedTag, describeChartPath: (path: string) => string): string { - return tag.source === 'file' ? '' : ` Tag taken from appVersion in ${describeChartPath(tag.metadataPath)}.`; -} - /** * A document's checks, or none once the text has moved on. Recorded offsets * belong to the version that was checked, so projecting them onto edited @@ -125,5 +115,5 @@ function registriesNeedingLogin(checks: readonly ReferenceCheck[]): string[] { return [...registries]; } -export { chartProvenanceSentence, checkImageReferencesInDocument, checksAsOf, registriesNeedingLogin }; +export { checkImageReferencesInDocument, checksAsOf, registriesNeedingLogin }; export type { DocumentChecks, ReferenceCheck }; diff --git a/apps/vscode/src/tag-provenance.ts b/apps/vscode/src/tag-provenance.ts new file mode 100644 index 0000000..74dfc2d --- /dev/null +++ b/apps/vscode/src/tag-provenance.ts @@ -0,0 +1,16 @@ +import type { ResolvedTag } from 'helm'; + +/** + * The sentence a message owes the reader when the tag it reports was never + * written in the file. + * + * Shared by the diagnostic and the hover so the two cannot word the same + * provenance differently, and empty for a tag the file wrote, which needs no + * explaining. `describeChartPath` shortens the path for display: an absolute + * path in the Problems panel is noise the reader has to scan past. + */ +function chartProvenanceSentence(tag: ResolvedTag, describeChartPath: (path: string) => string): string { + return tag.source === 'file' ? '' : ` Tag taken from appVersion in ${describeChartPath(tag.metadataPath)}.`; +} + +export { chartProvenanceSentence }; diff --git a/apps/vscode/test/vscode-stub.ts b/apps/vscode/test/vscode-stub.ts index 53d6ca1..2f4decb 100644 --- a/apps/vscode/test/vscode-stub.ts +++ b/apps/vscode/test/vscode-stub.ts @@ -298,8 +298,6 @@ interface FileSystemWatcherStub { readonly dispose: ReturnType; /** Test-only: drives the change listeners and awaits them. Not part of the real `vscode` API. */ readonly fireDidChange: (uri: unknown) => Promise; - /** Test-only: drives the create listeners and awaits them. Not part of the real `vscode` API. */ - readonly fireDidCreate: (uri: unknown) => Promise; } const fileSystemWatchers: FileSystemWatcherStub[] = []; @@ -315,7 +313,6 @@ function createFileSystemWatcherStub(globPattern: string): FileSystemWatcherStub onDidDelete: didDelete.event, dispose: vi.fn(), fireDidChange: didChange.fire, - fireDidCreate: didCreate.fire, }; fileSystemWatchers.push(watcher); From 6123c1c1573eda7066cac82720589a4d390d560f Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Thu, 24 Sep 2026 15:03:46 +0300 Subject: [PATCH 6/6] fix(vscode): drop a chart re-check superseded by a later one Two Chart.yaml saves in quick succession could let the slower registry answer publish last, leaving a result for an appVersion already replaced. Only the latest check started for a document now publishes. The watcher test now bumps appVersion before the change event, so it proves the re-check uses the new version, and the ImageReference docstring no longer defers appVersion resolution to a later ticket. Co-Authored-By: Claude Opus 5.5 (1M context) --- apps/vscode/src/extension.test.ts | 55 ++++++++++++++++--- apps/vscode/src/extension.ts | 16 +++++- packages/helm/src/extract-image-references.ts | 4 +- 3 files changed, 62 insertions(+), 13 deletions(-) diff --git a/apps/vscode/src/extension.test.ts b/apps/vscode/src/extension.test.ts index 29ce58c..04f91f9 100644 --- a/apps/vscode/src/extension.test.ts +++ b/apps/vscode/src/extension.test.ts @@ -31,6 +31,11 @@ const TAGLESS_VALUES_YAML = ['image:', ` repository: ${REPOSITORY}`, ' pullPol // what they did before chart context existed. const NO_FILES = createFakeFileSystem({}); +/** Chart metadata declaring `appVersion`, for tests about what a bump re-checks. */ +function chartMetadataWithAppVersion(appVersion: string): string { + return ['apiVersion: v2', 'name: my-service', `appVersion: ${appVersion}`, ''].join('\n'); +} + /** Stages `document` as the only visible editor, then fires the open event. */ async function openInVisibleEditor(document: vscode.TextDocument): Promise { const editor = createTextEditorStub(document); @@ -230,16 +235,13 @@ describe('extension', () => { expect(getLastFileSystemWatcher()?.globPattern).toBe('**/Chart.yaml'); }); - it('should re-check the documents a chart governs when its metadata changes, and leave the others alone', async () => { + it('should re-check the documents a chart governs against its new appVersion, and leave the others alone', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); - activate(context, { - fetch, - credentials: noDockerCredentials, - readTextFile: createFakeFileSystem({ - '/repo/chart/Chart.yaml': ['apiVersion: v2', 'name: my-service', `appVersion: ${TAG}`, ''].join('\n'), - '/repo/other/Chart.yaml': ['apiVersion: v2', 'name: other', 'appVersion: 2.0.0', ''].join('\n'), - }), - }); + const files: Record = { + '/repo/chart/Chart.yaml': chartMetadataWithAppVersion('1.18'), + '/repo/other/Chart.yaml': ['apiVersion: v2', 'name: other', 'appVersion: 2.0.0', ''].join('\n'), + }; + activate(context, { fetch, credentials: noDockerCredentials, readTextFile: createFakeFileSystem(files) }); const governed = createFakeDocument('/repo/chart/values.yaml', TAGLESS_VALUES_YAML); const unrelated = createFakeDocument( @@ -252,6 +254,7 @@ describe('extension', () => { setOpenTextDocuments([governed, unrelated]); fetch.mockClear(); + files['/repo/chart/Chart.yaml'] = chartMetadataWithAppVersion(TAG); await emitDidChangeChartMetadata({ path: '/repo/chart/Chart.yaml' }); // A bumped `appVersion` is exactly when a stale checkmark costs the most, @@ -259,4 +262,38 @@ describe('extension', () => { expect(fetch).toHaveBeenCalledTimes(1); expect(fetch).toHaveBeenCalledWith(`https://registry-1.docker.io/v2/library/nginx/manifests/${TAG}`, expect.anything()); }); + + it('should keep the latest re-check when an earlier one for the same document answers after it', async () => { + type FakeResponse = ReturnType; + let answerSlowRequest: (response: FakeResponse) => void = () => undefined; + const slowAnswer = new Promise((resolve) => { + answerSlowRequest = resolve; + }); + const fetch = vi.fn(async (url: string) => (url.endsWith('/manifests/1.18') ? slowAnswer : Promise.resolve(fakeFetchResponse(200)))); + const files: Record = { '/repo/chart/Chart.yaml': chartMetadataWithAppVersion('1.17') }; + activate(context, { fetch, credentials: noDockerCredentials, readTextFile: createFakeFileSystem(files) }); + + const document = createFakeDocument('/repo/chart/values.yaml', TAGLESS_VALUES_YAML); + await emitDidOpenTextDocument(document); + setOpenTextDocuments([document]); + + // Two saves in quick succession, as autosave produces while typing a + // version: the first one's registry answer is still in flight when the + // second one lands. + files['/repo/chart/Chart.yaml'] = chartMetadataWithAppVersion('1.18'); + const firstRecheck = emitDidChangeChartMetadata({ path: '/repo/chart/Chart.yaml' }); + await vi.waitFor(() => { + expect(fetch).toHaveBeenCalledWith(expect.stringMatching(/\/manifests\/1\.18$/), expect.anything()); + }); + + files['/repo/chart/Chart.yaml'] = chartMetadataWithAppVersion(TAG); + await emitDidChangeChartMetadata({ path: '/repo/chart/Chart.yaml' }); + + answerSlowRequest(fakeFetchResponse(404, { errors: [{ code: 'MANIFEST_UNKNOWN' }] })); + await firstRecheck; + + const setCalls = getLastDiagnosticCollection()?.set.mock.calls ?? []; + + expect(setCalls[setCalls.length - 1]?.[1]).toEqual([]); + }); }); diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index 34b23a6..4bbcbb1 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -61,6 +61,10 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend context.subscriptions.push(diagnostics); const checksByDocument = new Map(); + // The most recent check started per document. A registry answer can land + // after a later check's, so only the latest check may publish. + const latestCheckByDocument = new Map(); + let checksStarted = 0; const markDecorationType = createMarkDecorationType(); context.subscriptions.push(markDecorationType); @@ -71,18 +75,26 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend * Checks one document and republishes everything shown for it. The open * listener and the chart watcher both go through here, so the two can * never drift into publishing different things about the same document. + * + * A check superseded while it waited on a registry publishes nothing. Two + * `Chart.yaml` saves in quick succession would otherwise let the slower + * answer win, and that answer is about the `appVersion` already replaced. */ async function checkDocument(document: vscode.TextDocument): Promise { + const key = document.uri.toString(); + const checkNumber = ++checksStarted; + latestCheckByDocument.set(key, checkNumber); + // VS Code never awaits a listener, so anything escaping this callback // is an unhandled rejection, and that takes the extension host down. try { const checked = await checkImageReferencesInDocument(document, checkDependencies); - if (checked === undefined) { + if (checked === undefined || latestCheckByDocument.get(key) !== checkNumber) { return; } - checksByDocument.set(document.uri.toString(), checked); + checksByDocument.set(key, checked); diagnostics.set(document.uri, diagnosticsFor(document, checksAsOf(checked, document), vscode.workspace.asRelativePath)); applyMarks(vscode.window.visibleTextEditors, checksByDocument, markDecorationType); diff --git a/packages/helm/src/extract-image-references.ts b/packages/helm/src/extract-image-references.ts index f7a6076..d32c1f7 100644 --- a/packages/helm/src/extract-image-references.ts +++ b/packages/helm/src/extract-image-references.ts @@ -3,8 +3,8 @@ import { readUsableScalar, type RawScalar } from './raw-scalar'; /** * A candidate image reference found in a Helm values file. `tag` is - * `undefined` for a tagless reference — resolving it via `appVersion` is a - * later ticket's job. + * `undefined` for a tagless reference, which `resolveTag` resolves through + * the governing chart's `appVersion`. */ interface ImageReference { readonly repository: RawScalar;