feat: resolve tagless images through the governing chart's appVersion

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<UncheckedReason, ReferenceVerdict> = {
-  'no-tag': { kind: 'unverifiable', reason: 'no-tag' },
+const UNCHECKED_VERDICTS: Record<UnverifiableReason, ImageVerdict> = {
   '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<UncheckedReason, ReferenceVerdict> = {
   '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([]);
     }
   });
 });

apps/vscode/src/diagnostics.ts

@@ -1,17 +1,25 @@
 import * as vscode from 'vscode';
 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
  * 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 +31,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
         )
       );

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,17 @@ 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({});
+
+/** 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<TextEditorStub> {
@@ -79,12 +94,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 +111,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 +137,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 +151,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 +166,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 +183,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 +210,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 +222,78 @@ 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 the chart metadata name chart resolution actually reads', () => {
+    activate(context, { fetch: vi.fn(), credentials: noDockerCredentials, readTextFile: NO_FILES });
+
+    expect(getLastFileSystemWatcher()?.globPattern).toBe('**/Chart.yaml');
+  });
+
+  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));
+    const files: Record<string, string> = {
+      '/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(
+      '/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();
+
+    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,
+    // 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());
+  });
+
+  it('should keep the latest re-check when an earlier one for the same document answers after it', async () => {
+    type FakeResponse = ReturnType<typeof fakeFetchResponse>;
+    let answerSlowRequest: (response: FakeResponse) => void = () => undefined;
+    const slowAnswer = new Promise<FakeResponse>((resolve) => {
+      answerSlowRequest = resolve;
+    });
+    const fetch = vi.fn(async (url: string) => (url.endsWith('/manifests/1.18') ? slowAnswer : Promise.resolve(fakeFetchResponse(200))));
+    const files: Record<string, string> = { '/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([]);
+  });
 });