Address second round of PR review feedback

- Replace getConfig<T> with getMetadata/setMetadata on CliAuth, removing
  the unsafe type parameter in favor of returning unknown
- Move updateInstanceConfig from cli-module-auth public API to
  CliAuth.setMetadata, removing the cross-package dependency
- Rename 'config' to 'metadata' in StoredInstance and storage schemas
- Add zod validation at consumer sites (cli-module-actions) for
  type-safe metadata access
- Fix zod imports to use zod/v3 for compatibility with zod v4
- Add proper-lockfile to cli-node for metadata write locking
- Refactor cli-node storage from fs-extra to node:fs
- Remove @backstage/cli-module-auth dependency from cli-module-actions

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Made-with: Cursor

packages/cli-node/src/auth/CliAuth.test.ts

@@ -180,30 +180,43 @@ describe('CliAuth', () => {
     });
   });
 
-  describe('getConfig', () => {
-    it('returns a config value from the instance', async () => {
-      mockStorage.getInstanceConfig.mockResolvedValue([
+  describe('getMetadata / setMetadata', () => {
+    it('returns a metadata value from the instance', async () => {
+      mockStorage.getInstanceMetadata.mockResolvedValue([
         'catalog',
         'scaffolder',
       ]);
 
       const auth = await CliAuth.create();
-      const sources = await auth.getConfig<string[]>('pluginSources');
+      const sources = await auth.getMetadata('pluginSources');
 
       expect(sources).toEqual(['catalog', 'scaffolder']);
-      expect(mockStorage.getInstanceConfig).toHaveBeenCalledWith(
+      expect(mockStorage.getInstanceMetadata).toHaveBeenCalledWith(
         'production',
         'pluginSources',
       );
     });
 
-    it('returns undefined for missing config keys', async () => {
-      mockStorage.getInstanceConfig.mockResolvedValue(undefined);
+    it('returns undefined for missing metadata keys', async () => {
+      mockStorage.getInstanceMetadata.mockResolvedValue(undefined);
 
       const auth = await CliAuth.create();
-      const value = await auth.getConfig('nonexistent');
+      const value = await auth.getMetadata('nonexistent');
 
       expect(value).toBeUndefined();
     });
+
+    it('writes a metadata value to the instance store', async () => {
+      mockStorage.updateInstanceMetadata.mockResolvedValue(undefined);
+
+      const auth = await CliAuth.create();
+      await auth.setMetadata('pluginSources', ['catalog']);
+
+      expect(mockStorage.updateInstanceMetadata).toHaveBeenCalledWith(
+        'production',
+        'pluginSources',
+        ['catalog'],
+      );
+    });
   });
 });

packages/cli-node/src/auth/CliAuth.ts

@@ -17,13 +17,14 @@
 import {
   type StoredInstance,
   getSelectedInstance,
-  getInstanceConfig,
+  getInstanceMetadata,
+  updateInstanceMetadata,
   accessTokenNeedsRefresh,
 } from './storage';
 import { getSecretStore, type SecretStore } from './secretStore';
 import { getAuthInstanceService } from './authIdentifiers';
 import { httpJson } from './httpJson';
-import { z } from 'zod';
+import { z } from 'zod/v3';
 
 const TokenResponseSchema = z.object({
   access_token: z.string().min(1),
@@ -104,11 +105,18 @@ export class CliAuth {
   }
 
   /**
-   * Reads a per-instance configuration value previously stored by the
+   * Reads a per-instance metadata value previously stored by the
    * auth module (e.g. `pluginSources`).
    */
-  async getConfig<T = unknown>(key: string): Promise<T | undefined> {
-    return getInstanceConfig<T>(this.#instance.name, key);
+  async getMetadata(key: string): Promise<unknown> {
+    return getInstanceMetadata(this.#instance.name, key);
+  }
+
+  /**
+   * Writes a per-instance metadata value to the on-disk instance store.
+   */
+  async setMetadata(key: string, value: unknown): Promise<void> {
+    return updateInstanceMetadata(this.#instance.name, key, value);
   }
 
   async #refreshAccessToken(): Promise<void> {

packages/cli-node/src/auth/storage.ts

@@ -15,11 +15,12 @@
  */
 
 import { NotFoundError } from '@backstage/errors';
-import fs from 'fs-extra';
+import { promises as fs } from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
+import lockfile from 'proper-lockfile';
 import YAML from 'yaml';
-import { z } from 'zod';
+import { z } from 'zod/v3';
 
 const METADATA_FILE = 'auth-instances.yaml';
 
@@ -35,10 +36,13 @@ const storedInstanceSchema = z.object({
   issuedAt: z.number().int().nonnegative(),
   accessTokenExpiresAt: z.number().int().nonnegative(),
   selected: z.boolean().optional(),
-  config: z.record(z.string(), z.unknown()).optional(),
+  metadata: z.record(z.string(), z.unknown()).optional(),
+});
+
+const authYamlSchema = z.object({
+  instances: z.array(storedInstanceSchema).default([]),
 });
 
-/** @public */
 export type StoredInstance = {
   name: string;
   baseUrl: string;
@@ -46,12 +50,17 @@ export type StoredInstance = {
   issuedAt: number;
   accessTokenExpiresAt: number;
   selected?: boolean;
-  config?: Record<string, unknown>;
+  metadata?: Record<string, unknown>;
 };
 
-const authYamlSchema = z.object({
-  instances: z.array(storedInstanceSchema).default([]),
-});
+async function pathExists(p: string): Promise<boolean> {
+  try {
+    await fs.stat(p);
+    return true;
+  } catch {
+    return false;
+  }
+}
 
 /** @internal */
 export function getMetadataFilePath(): string {
@@ -67,7 +76,7 @@ export function getMetadataFilePath(): string {
 /** @internal */
 export async function readAll(): Promise<{ instances: StoredInstance[] }> {
   const file = getMetadataFilePath();
-  if (!(await fs.pathExists(file))) {
+  if (!(await pathExists(file))) {
     return { instances: [] };
   }
   const text = await fs.readFile(file, 'utf8');
@@ -86,6 +95,29 @@ export async function readAll(): Promise<{ instances: StoredInstance[] }> {
   }
 }
 
+async function writeAll(data: { instances: StoredInstance[] }): Promise<void> {
+  const file = getMetadataFilePath();
+  await fs.mkdir(path.dirname(file), { recursive: true });
+  const yaml = YAML.stringify(authYamlSchema.parse(data), { indentSeq: false });
+  await fs.writeFile(file, yaml, { encoding: 'utf8', mode: 0o600 });
+}
+
+async function withMetadataLock<T>(fn: () => Promise<T>): Promise<T> {
+  const file = getMetadataFilePath();
+  await fs.mkdir(path.dirname(file), { recursive: true });
+  if (!(await pathExists(file))) {
+    await fs.writeFile(file, '', { encoding: 'utf8', mode: 0o600 });
+  }
+  const release = await lockfile.lock(file, {
+    retries: { retries: 5, factor: 1.5, minTimeout: 100, maxTimeout: 1000 },
+  });
+  try {
+    return await fn();
+  } finally {
+    await release();
+  }
+}
+
 /** @internal */
 export async function getAllInstances(): Promise<{
   instances: StoredInstance[];
@@ -129,12 +161,32 @@ export async function getInstanceByName(name: string): Promise<StoredInstance> {
 }
 
 /** @internal */
-export async function getInstanceConfig<T = unknown>(
+export async function getInstanceMetadata(
   instanceName: string,
   key: string,
-): Promise<T | undefined> {
+): Promise<unknown> {
   const instance = await getInstanceByName(instanceName);
-  return instance.config?.[key] as T | undefined;
+  return instance.metadata?.[key];
+}
+
+/** @internal */
+export async function updateInstanceMetadata(
+  instanceName: string,
+  key: string,
+  value: unknown,
+): Promise<void> {
+  return withMetadataLock(async () => {
+    const data = await readAll();
+    const idx = data.instances.findIndex(i => i.name === instanceName);
+    if (idx === -1) {
+      throw new NotFoundError(`Instance '${instanceName}' not found`);
+    }
+    data.instances[idx] = {
+      ...data.instances[idx],
+      metadata: { ...data.instances[idx].metadata, [key]: value },
+    };
+    await writeAll(data);
+  });
 }
 
 /** @internal */