feat(tools): stack-env ergonomics — summary/hint, remove_stack_env_vars, collision check

src/tools/stacks.ts

@@ -8,6 +8,8 @@ import type { DockhandClient } from '../client/dockhand-client.js';
 import type { StackEnv, EnvVariable } from '../types/dockhand.js';
 import { registerTool, jsonResponse, textResponse } from '../utils/tool-helper.js';
 import { encodePath } from '../utils/encode-path.js';
+import { diffEnvVars, parseDotEnvKeys, removeKeysFromDotEnv } from '../utils/env-helpers.js';
+import type { EnvDiff } from '../utils/env-helpers.js';
 
 export function registerStackTools(server: McpServer, client: DockhandClient): void {
 
@@ -150,8 +152,10 @@ export function registerStackTools(server: McpServer, client: DockhandClient): v
     },
     async ({ environmentId, name, variables, mode = 'merge' }) => {
       let finalVariables: EnvVariable[];
+      let diff: EnvDiff | undefined;
 
       if (mode === 'merge') {
+        // GET is load-bearing here: a failure must NOT issue a PUT (no data loss).
         const existing = await client.get<StackEnv>(
           `/api/stacks/${encodePath(name)}/env`,
           { env: environmentId },
@@ -178,11 +182,30 @@ export function registerStackTools(server: McpServer, client: DockhandClient): v
           });
         }
         finalVariables = Array.from(mergedByKey.values());
+        diff = diffEnvVars(Array.isArray(existingVars) ? existingVars : [], variables, 'merge');
       } else {
+        // replace: PUT the payload directly. No GET, no summary — the existing
+        // contract (replace does not GET) stays intact.
         finalVariables = variables;
       }
 
-      return jsonResponse(await client.put(`/api/stacks/${encodePath(name)}/env`, { variables: finalVariables }, { env: environmentId }));
+      const result = (await client.put(
+        `/api/stacks/${encodePath(name)}/env`, { variables: finalVariables },
+        { env: environmentId })) as Record<string, unknown>;
+
+      const hint =
+        diff && diff.added.length === 0 && diff.preserved.length > 0
+          ? `merge mode preserved ${diff.preserved.length} variable(s) you did not include and removed nothing. To remove variables use remove_stack_env_vars, or update_stack_env with mode="replace".`
+          : undefined;
+
+      return jsonResponse({
+        ...result,
+        ...(diff ? { summary: {
+          added: diff.added.length, updated: diff.updated.length,
+          preserved: diff.preserved.length, removed: diff.removed.length,
+        } } : {}),
+        ...(hint ? { hint } : {}),
+      });
     }
   );
 
@@ -208,6 +231,93 @@ export function registerStackTools(server: McpServer, client: DockhandClient): v
     }
   );
 
+  registerTool(server, 'remove_stack_env_vars',
+    'Remove environment variables from a stack across BOTH stores. Database-backed keys (secrets, and non-secrets that live in the database for git stacks) are dropped by rebuilding the full remaining database set — remaining secrets stay masked as "***"; .env-backed non-secret keys are removed by rewriting the .env file. Result reports `removed` (all keys actually removed, from either store) and `not_found` (keys present in neither). This is the safe way to delete variables — `update_stack_env` in the default merge mode cannot remove keys.',
+    {
+      environmentId: z.number().describe('Environment ID'),
+      name: z.string().describe('Stack name'),
+      keys: z.array(z.string()).describe('Variable names to remove'),
+    },
+    async ({ environmentId, name, keys }) => {
+      const uniqueKeys = [...new Set(keys)];
+      const keySet = new Set(uniqueKeys);
+
+      const structured = await client.get<StackEnv>(
+        `/api/stacks/${encodePath(name)}/env`, { env: environmentId });
+      const vars = Array.isArray(structured?.variables) ? structured.variables : [];
+      const structuredKeys = new Set(vars.map((v) => v.key));
+      const secretKeys = new Set(vars.filter((v) => v.isSecret).map((v) => v.key));
+
+      const raw = await client.get<string>(
+        `/api/stacks/${encodePath(name)}/env/raw`, { env: environmentId });
+      const rawStr = typeof raw === 'string' ? raw : '';
+      const envKeys = new Set(parseDotEnvKeys(rawStr));
+
+      const removed = uniqueKeys.filter((k) => structuredKeys.has(k) || envKeys.has(k));
+      const notFound = uniqueKeys.filter((k) => !structuredKeys.has(k) && !envKeys.has(k));
+
+      // A target changes the DB store if it is a secret, or a non-secret that lives
+      // in the DB (git stacks: present in the structured view but NOT in .env).
+      const dbManagedTargets = uniqueKeys.filter(
+        (k) => secretKeys.has(k) || (structuredKeys.has(k) && !envKeys.has(k)));
+
+      if (dbManagedTargets.length > 0) {
+        // Rebuild the FULL remaining DB-backed set minus targets — never drop
+        // untouched vars. Keep secrets (masked '***'; the backend preserves the real
+        // value) and DB-managed non-secrets (not in .env). .env-backed non-secrets are
+        // handled by the raw rewrite below, so they are excluded here to avoid creating
+        // a DB/.env duplicate.
+        const remaining = vars
+          .filter((v) => !keySet.has(v.key))
+          .filter((v) => v.isSecret || !envKeys.has(v.key))
+          .map((v) => ({ key: v.key, value: v.isSecret ? '***' : v.value, isSecret: v.isSecret ?? false }));
+        await client.put(`/api/stacks/${encodePath(name)}/env`,
+          { variables: remaining }, { env: environmentId });
+      }
+
+      const envTargets = uniqueKeys.filter((k) => envKeys.has(k));
+      if (envTargets.length > 0) {
+        try {
+          const newContent = removeKeysFromDotEnv(rawStr, envTargets);
+          await client.put(`/api/stacks/${encodePath(name)}/env/raw`,
+            { content: newContent }, { env: environmentId });
+        } catch (e) {
+          return jsonResponse({
+            removed: removed.filter((k) => !envTargets.includes(k)),
+            not_found: notFound,
+            error: `DB entries removed, but .env rewrite failed: ${e instanceof Error ? e.message : String(e)}`,
+          });
+        }
+      }
+
+      return jsonResponse({ removed, not_found: notFound });
+    }
+  );
+
+  registerTool(server, 'check_stack_env_collisions',
+    'Read-only check reporting variable keys defined BOTH as a database-backed secret and in the plain .env file. Such duplicates are ambiguous: at deploy the secret (shell environment) wins over the .env value. Remove the duplicate copy with `remove_stack_env_vars`.',
+    {
+      environmentId: z.number().describe('Environment ID'),
+      name: z.string().describe('Stack name'),
+    },
+    async ({ environmentId, name }) => {
+      const structured = await client.get<StackEnv>(
+        `/api/stacks/${encodePath(name)}/env`, { env: environmentId });
+      const secretKeys = new Set(
+        (Array.isArray(structured?.variables) ? structured.variables : [])
+          .filter((v) => v.isSecret).map((v) => v.key));
+      const raw = await client.get<string>(
+        `/api/stacks/${encodePath(name)}/env/raw`, { env: environmentId });
+      const envKeys = parseDotEnvKeys(typeof raw === 'string' ? raw : '');
+      const collisions = envKeys.filter((k) => secretKeys.has(k));
+      return jsonResponse(
+        collisions.length > 0
+          ? { collisions, note: 'These keys exist BOTH as a DB secret and in .env. The DB secret (shell-env) wins at deploy; remove the .env copy with remove_stack_env_vars.' }
+          : { collisions: [] },
+      );
+    }
+  );
+
   registerTool(server, 'validate_stack_env', 'Validate the environment variables of a stack for completeness and correctness without mutating; use `update_stack_env` or `update_stack_env_raw` to fix any reported issues.',
     {
       environmentId: z.number().describe('Environment ID'),

src/utils/env-helpers.ts

@@ -0,0 +1,70 @@
+import type { EnvVariable } from '../types/dockhand.js';
+
+export interface EnvDiff {
+  added: string[];
+  updated: string[];
+  preserved: string[];
+  removed: string[];
+}
+
+/**
+ * Diff a payload against the existing variables for update_stack_env.
+ * A payload value of '***' means "keep existing value" and never counts as updated.
+ */
+export function diffEnvVars(
+  existing: EnvVariable[],
+  payload: EnvVariable[],
+  mode: 'merge' | 'replace',
+): EnvDiff {
+  const existingByKey = new Map(existing.map((v) => [v.key, v]));
+  const payloadKeys = new Set(payload.map((v) => v.key));
+
+  const added: string[] = [];
+  const updated: string[] = [];
+  for (const v of payload) {
+    const prev = existingByKey.get(v.key);
+    if (!prev) {
+      added.push(v.key);
+      continue;
+    }
+    const valueChanged = v.value !== '***' && v.value !== prev.value;
+    const secretChanged = v.isSecret !== undefined && v.isSecret !== prev.isSecret;
+    if (valueChanged || secretChanged) updated.push(v.key);
+  }
+
+  const untouched = existing.filter((v) => !payloadKeys.has(v.key)).map((v) => v.key);
+  return {
+    added,
+    updated,
+    preserved: mode === 'merge' ? untouched : [],
+    removed: mode === 'replace' ? untouched : [],
+  };
+}
+
+/** Extract variable keys from raw .env content, skipping blank lines and comments. */
+export function parseDotEnvKeys(content: string): string[] {
+  const keys: string[] = [];
+  for (const rawLine of content.split(/\r?\n/)) {
+    let line = rawLine.trim();
+    if (!line || line.startsWith('#')) continue;
+    if (line.startsWith('export ')) line = line.slice(7).trim();
+    const eq = line.indexOf('=');
+    if (eq <= 0) continue;
+    keys.push(line.slice(0, eq).trim());
+  }
+  return keys;
+}
+
+/** Remove the given keys from raw .env content, preserving order, comments and blank lines. */
+export function removeKeysFromDotEnv(content: string, keys: string[]): string {
+  const drop = new Set(keys);
+  const kept = content.split(/\r?\n/).filter((rawLine) => {
+    let line = rawLine.trim();
+    if (!line || line.startsWith('#')) return true;
+    if (line.startsWith('export ')) line = line.slice(7).trim();
+    const eq = line.indexOf('=');
+    if (eq <= 0) return true;
+    return !drop.has(line.slice(0, eq).trim());
+  });
+  return kept.join('\n');
+}

tests/env-helpers.test.ts

@@ -0,0 +1,59 @@
+import { describe, it, expect } from 'vitest';
+import { diffEnvVars, parseDotEnvKeys, removeKeysFromDotEnv } from '../src/utils/env-helpers.js';
+
+describe('diffEnvVars', () => {
+  const existing = [
+    { key: 'A', value: 'a', isSecret: false },
+    { key: 'B', value: 'b', isSecret: true },
+    { key: 'C', value: 'c', isSecret: false },
+  ];
+
+  it('merge: added/updated/preserved (omitted keys only); masked re-send is not surfaced', () => {
+    const d = diffEnvVars(existing, [
+      { key: 'A', value: 'a2' },          // updated (value changed)
+      { key: 'D', value: 'd' },           // added
+      { key: 'B', value: '***' },         // re-sent masked -> unchanged, in NO bucket
+    ], 'merge');
+    expect(d.added).toEqual(['D']);
+    expect(d.updated).toEqual(['A']);
+    expect(d.preserved).toEqual(['C']);   // only the omitted key
+    expect(d.removed).toEqual([]);
+  });
+
+  it('replace: untouched existing keys count as removed, preserved empty', () => {
+    const d = diffEnvVars(existing, [{ key: 'A', value: 'a' }], 'replace');
+    expect(d.removed.sort()).toEqual(['B', 'C']);
+    expect(d.preserved).toEqual([]);
+    expect(d.updated).toEqual([]); // same value -> not updated
+  });
+
+  it('isSecret flip counts as updated', () => {
+    const d = diffEnvVars(existing, [{ key: 'A', value: 'a', isSecret: true }], 'merge');
+    expect(d.updated).toEqual(['A']);
+  });
+});
+
+describe('parseDotEnvKeys', () => {
+  it('returns keys, skipping comments and blank lines', () => {
+    const content = '# comment\nA=1\n\nB=two=with=eq\n  C = 3 \n';
+    expect(parseDotEnvKeys(content)).toEqual(['A', 'B', 'C']);
+  });
+
+  it('strips a leading "export " prefix before extracting the key', () => {
+    expect(parseDotEnvKeys('export A=1\nB=2')).toEqual(['A', 'B']);
+  });
+});
+
+describe('removeKeysFromDotEnv', () => {
+  it('drops matching KEY= lines, preserves comments/blanks/order', () => {
+    const content = '# keep\nA=1\nB=2\n\nC=3';
+    expect(removeKeysFromDotEnv(content, ['B'])).toBe('# keep\nA=1\n\nC=3');
+  });
+  it('is a no-op for keys not present', () => {
+    const content = 'A=1\nB=2';
+    expect(removeKeysFromDotEnv(content, ['Z'])).toBe('A=1\nB=2');
+  });
+  it('drops an "export KEY=" line when KEY is targeted', () => {
+    expect(removeKeysFromDotEnv('export A=1\nB=2', ['A'])).toBe('B=2');
+  });
+});

tests/stack-env-collisions.test.ts

@@ -0,0 +1,44 @@
+import { describe, it, expect, vi } from 'vitest';
+import { registerStackTools } from '../src/tools/stacks.js';
+
+type ToolHandler = (args: Record<string, unknown>) => Promise<unknown>;
+function jsonOut(res: unknown): Record<string, unknown> {
+  return JSON.parse((res as { content: { text: string }[] }).content[0].text);
+}
+function setup() {
+  const handlers = new Map<string, ToolHandler>();
+  const server = { tool: (n: string, _d: string, _s: unknown, cb: ToolHandler) => handlers.set(n, cb) };
+  const client = { get: vi.fn(), put: vi.fn() };
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  registerStackTools(server as any, client as any);
+  const handler = handlers.get('check_stack_env_collisions');
+  if (!handler) throw new Error('check_stack_env_collisions not registered');
+  return { handler, client };
+}
+
+describe('check_stack_env_collisions', () => {
+  it('reports keys that are both DB secret and in .env', async () => {
+    const { handler, client } = setup();
+    client.get.mockImplementation((path: string) =>
+      path.endsWith('/env/raw')
+        ? Promise.resolve('TZ=x\nDB_PASSWORD=leak\n')
+        : Promise.resolve({ variables: [
+            { key: 'DB_PASSWORD', value: '***', isSecret: true },
+            { key: 'TZ', value: 'x', isSecret: false },
+          ] }));
+    const out = jsonOut(await handler({ environmentId: 10, name: 'x' }));
+    expect(out.collisions).toEqual(['DB_PASSWORD']);
+    expect(typeof out.note).toBe('string');
+  });
+
+  it('no collisions → empty list, no note', async () => {
+    const { handler, client } = setup();
+    client.get.mockImplementation((path: string) =>
+      path.endsWith('/env/raw')
+        ? Promise.resolve('TZ=x\n')
+        : Promise.resolve({ variables: [{ key: 'DB_PASSWORD', value: '***', isSecret: true }] }));
+    const out = jsonOut(await handler({ environmentId: 10, name: 'x' }));
+    expect(out.collisions).toEqual([]);
+    expect(out.note).toBeUndefined();
+  });
+});

tests/stack-env-merge-behavior.test.ts

@@ -156,3 +156,36 @@ describe('update_stack_env — merge behaviour (mocked client)', () => {
     expect(putBody(client).variables).toEqual([{ key: 'X', value: 'y' }]);
   });
 });
+
+// Response-Parser: jsonResponse liefert { content: [{ type:'text', text: JSON.stringify(x) }] }
+function jsonOut(res: unknown): Record<string, unknown> {
+  const text = (res as { content: { text: string }[] }).content[0].text;
+  return JSON.parse(text) as Record<string, unknown>;
+}
+
+describe('update_stack_env — summary/hint (merge only)', () => {
+  it('merge subset (no new keys) → summary + remove hint', async () => {
+    const { handler, client } = setup();
+    client.get.mockResolvedValue({ variables: [
+      { key: 'DB_PASSWORD', value: 's', isSecret: true },
+      { key: 'TZ', value: 'Europe/Berlin' },
+      { key: 'HOST', value: 'h' },
+    ] });
+    const res = await handler({ environmentId: 10, name: 'x',
+      variables: [{ key: 'DB_PASSWORD', value: '***', isSecret: true }] });
+    const out = jsonOut(res);
+    expect(out.summary).toEqual({ added: 0, updated: 0, preserved: 2, removed: 0 });
+    expect(typeof out.hint).toBe('string');
+    expect(String(out.hint)).toContain('remove_stack_env_vars');
+  });
+
+  it('replace mode: no summary/hint and no extra GET (existing contract preserved)', async () => {
+    const { handler, client } = setup();
+    const res = await handler({ environmentId: 10, name: 'x',
+      variables: [{ key: 'A', value: 'a' }], mode: 'replace' });
+    const out = jsonOut(res);
+    expect(client.get).not.toHaveBeenCalled();
+    expect(out.summary).toBeUndefined();
+    expect(out.hint).toBeUndefined();
+  });
+});