diff --git a/README.md b/README.md index 4d10ec7..2ca2ae0 100644 --- a/README.md +++ b/README.md @@ -131,7 +131,7 @@ Add to your MCP settings: | `get_stack_compose` | Read compose file | | `update_stack_compose` | Update compose file | | `get_stack_env` | Read environment variables | -| `update_stack_env` | Update environment variables | +| `update_stack_env` | Update environment variables (**merge** by default — safe for partial updates; use `mode="replace"` to overwrite all) | | `get_stack_env_raw` | Read raw .env file | | `validate_stack_env` | Validate env variables | | `scan_stacks` | Scan filesystem for stacks | @@ -365,6 +365,26 @@ Add to your MCP settings: ## Important Notes +### update_stack_env — Merge vs Replace Semantics + +The Dockhand REST endpoint `PUT /api/stacks/{name}/env` has **replace-semantics**: submitting a partial list of variables silently deletes all other variables from the stack. A single-variable update would wipe everything else. + +To prevent accidental data loss, this MCP tool defaults to **merge mode**: + +1. It fetches the current variable list via `GET /api/stacks/{name}/env`. +2. It merges the incoming variables by key (new values overwrite existing ones on key collision). +3. It writes the full combined list back via `PUT`. + +``` +# Safe partial update — only PRINTER_HUB_SSO_TRUST_TOKEN changes, all others preserved +update_stack_env(environmentId=1, name="my-stack", variables=[{key: "MY_VAR", value: "new"}]) + +# Explicit full replacement — all other variables are deleted +update_stack_env(environmentId=1, name="my-stack", variables=[...], mode="replace") +``` + +Use `mode="replace"` only when you intentionally want to replace the entire variable set. + ### Environment ID is Required Most Docker resource endpoints (containers, stacks, images, networks, volumes) require an `environmentId` parameter. This maps to the `?env=` query parameter in the Dockhand API. Without it, endpoints return empty arrays. diff --git a/src/tools/stacks.ts b/src/tools/stacks.ts index 29ba065..b812e3d 100644 --- a/src/tools/stacks.ts +++ b/src/tools/stacks.ts @@ -5,6 +5,7 @@ import { z } from 'zod'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; 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'; @@ -136,7 +137,7 @@ export function registerStackTools(server: McpServer, client: DockhandClient): v ); registerTool(server, 'update_stack_env', - 'Update secret environment variables (database-backed, encrypted at rest). Variables flagged isSecret:true are stored in the Dockhand database and injected into containers via shell-env at deploy time — they are NEVER written to the .env file. For non-secret variables that Docker Compose reads from the .env file at container start, use `update_stack_env_raw`.', + 'Update secret environment variables (database-backed, encrypted at rest). Variables flagged isSecret:true are stored in the Dockhand database and injected into containers via shell-env at deploy time — they are NEVER written to the .env file. For non-secret variables that Docker Compose reads from the .env file at container start, use `update_stack_env_raw`.\n\n**IMPORTANT — merge vs replace semantics:** The underlying Dockhand REST endpoint (`PUT /api/stacks/{name}/env`) has replace-semantics: sending a partial list silently deletes all other variables. This tool therefore defaults to `mode="merge"`: it fetches the current variables first, merges your payload by key (new values win on collision), and then writes the full combined list. Use `mode="replace"` only when you intentionally want to wipe all existing variables and set exactly the provided list.', { environmentId: z.number().describe('Environment ID'), name: z.string().describe('Stack name'), @@ -145,9 +146,35 @@ export function registerStackTools(server: McpServer, client: DockhandClient): v value: z.string().describe('Variable value as string'), isSecret: z.boolean().optional().describe('When true, store value in the Dockhand database (encrypted at rest) and inject via shell-env at deploy. When false/omitted, value is written to the .env file as plain text — DO NOT use for credentials.'), })).describe('Environment variables — flag secrets with isSecret:true'), + mode: z.enum(['merge', 'replace']).optional().describe('How to handle existing variables. "merge" (default): fetch existing vars, update/add the provided ones, preserve all others. "replace": overwrite the entire variable list with exactly the provided variables — all others are deleted.'), }, - async ({ environmentId, name, variables }) => { - return jsonResponse(await client.put(`/api/stacks/${encodePath(name)}/env`, { variables }, { env: environmentId })); + async ({ environmentId, name, variables, mode = 'merge' }) => { + let finalVariables: EnvVariable[]; + + if (mode === 'merge') { + const existing = await client.get( + `/api/stacks/${encodePath(name)}/env`, + { env: environmentId }, + ); + const existingVars: EnvVariable[] = existing?.variables ?? []; + const mergedByKey = new Map( + existingVars.map((v) => [v.key, v]), + ); + for (const v of variables) { + mergedByKey.set(v.key, v); + } + finalVariables = Array.from(mergedByKey.values()); + } else { + finalVariables = variables; + } + + return jsonResponse( + await client.put( + `/api/stacks/${encodePath(name)}/env`, + { variables: finalVariables }, + { env: environmentId }, + ), + ); } ); diff --git a/tests/stack-env-merge.test.ts b/tests/stack-env-merge.test.ts new file mode 100644 index 0000000..b1faae4 --- /dev/null +++ b/tests/stack-env-merge.test.ts @@ -0,0 +1,170 @@ +/** + * Static analysis tests for the update_stack_env merge-semantic implementation. + * + * Background: The Dockhand REST endpoint PUT /api/stacks/{name}/env has + * replace-semantics — sending a partial variable list silently deletes all + * other variables. This test suite verifies that the MCP tool wrapper adds a + * merge layer so that partial updates do not cause data loss. + * + * See: https://github.com/strausmann/mcp-dockhand/issues/ + * Incident: hangar-print-hub stack (2026-06-01), 7 of 8 variables deleted by + * a single-key update_stack_env call. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'fs'; +import { join, dirname } from 'path'; +import { fileURLToPath } from 'url'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const stacksSource = readFileSync( + join(__dirname, '..', 'src', 'tools', 'stacks.ts'), + 'utf-8', +); + +/** + * Extract the registerTool(...) source block for a named tool. The block + * spans from the registerTool(server, '', ...) line to the next + * registerTool( call (or end of file if last). + */ +function extractToolBlock(source: string, toolName: string): string { + const startPattern = new RegExp( + `registerTool\\s*\\(\\s*server\\s*,\\s*'${toolName}'`, + ); + const startMatch = startPattern.exec(source); + if (!startMatch) { + throw new Error(`Tool '${toolName}' not found in source`); + } + const startIdx = startMatch.index; + const afterStart = source.slice(startIdx + 1); + const nextToolMatch = /registerTool\s*\(/.exec(afterStart); + const endIdx = nextToolMatch + ? startIdx + 1 + nextToolMatch.index + : source.length; + return source.slice(startIdx, endIdx); +} + +describe('update_stack_env — merge-semantic implementation', () => { + const block = extractToolBlock(stacksSource, 'update_stack_env'); + + describe('mode parameter', () => { + it('declares a mode parameter with z.enum', () => { + expect(block).toMatch(/mode:\s*z\.enum\s*\(\s*\[/); + }); + + it('mode enum includes "merge" and "replace"', () => { + // Both values must appear in the enum declaration + expect(block).toMatch(/'merge'/); + expect(block).toMatch(/'replace'/); + }); + + it('mode is optional (defaults to "merge")', () => { + // .optional() is chained on the enum + expect(block).toMatch(/z\.enum\s*\(\s*\[[\s\S]*?\]\s*\)\s*\.optional\(\)/); + }); + + it('default value is "merge" in the handler destructuring', () => { + // Handler must default mode to 'merge': mode = 'merge' + expect(block).toMatch(/mode\s*=\s*['"]merge['"]/); + }); + }); + + describe('merge path — GET then PUT', () => { + it('calls client.get to fetch existing variables when merging', () => { + // Must call client.get for the /env endpoint + expect(block).toMatch(/client\.get\s* { + // Key-based deduplication via Map + expect(block).toMatch(/new\s+Map\s* { + // mergedByKey.set(v.key, v) pattern + expect(block).toMatch(/mergedByKey\.set\s*\(\s*v\.key\s*,\s*v\s*\)/); + }); + + it('converts the Map back to an array for the PUT body', () => { + // Array.from(mergedByKey.values()) + expect(block).toMatch(/Array\.from\s*\(\s*mergedByKey\.values\s*\(\s*\)\s*\)/); + }); + }); + + describe('replace path — PUT without GET', () => { + it('assigns variables directly in replace mode without a GET call', () => { + // replace branch: finalVariables = variables + expect(block).toMatch(/finalVariables\s*=\s*variables/); + }); + }); + + describe('PUT call — always uses finalVariables', () => { + it('PUT body references finalVariables, not the raw input variables', () => { + // The PUT must use finalVariables as the payload + expect(block).toMatch(/variables:\s*finalVariables/); + }); + + it('PUT targets the correct /env endpoint with encodePath', () => { + expect(block).toMatch(/client\.put\s*\(/); + expect(block).toMatch(/\/api\/stacks\/\$\{encodePath\(name\)\}\/env['"`,]/); + }); + + it('passes environmentId as env query parameter on PUT', () => { + expect(block).toMatch(/env:\s*environmentId/); + }); + }); + + describe('description — documents the merge-default behaviour', () => { + it('tool description explains the Dockhand replace-semantics risk', () => { + expect(block).toMatch(/replace.semantics|replace-semantics/i); + }); + + it('tool description mentions the merge default', () => { + expect(block).toMatch(/mode="merge"|mode=.merge./i); + }); + + it('tool description mentions the replace opt-in', () => { + expect(block).toMatch(/mode="replace"|mode=.replace./i); + }); + }); + + describe('type safety', () => { + it('imports StackEnv type from dockhand types', () => { + expect(stacksSource).toMatch(/import\s+type\s+\{[^}]*StackEnv[^}]*\}/); + }); + + it('imports EnvVariable type from dockhand types', () => { + expect(stacksSource).toMatch(/import\s+type\s+\{[^}]*EnvVariable[^}]*\}/); + }); + + it('uses typed GET for the existing variables fetch', () => { + // client.get(...) + expect(block).toMatch(/client\.get\s*<\s*StackEnv\s*>/); + }); + + it('declares finalVariables with EnvVariable[] type', () => { + expect(block).toMatch(/finalVariables:\s*EnvVariable\[\]/); + }); + }); +}); + +describe('update_stack_env — existing contract not broken', () => { + const block = extractToolBlock(stacksSource, 'update_stack_env'); + + it('still declares variables as a required array parameter', () => { + expect(block).toMatch(/variables:\s*z\.array\(/); + // Must NOT be optional + expect(block).not.toMatch(/variables:\s*z\.array\([\s\S]*?\}\s*\)\s*\)\s*\.optional/); + }); + + it('still references update_stack_env_raw for non-secret writes', () => { + expect(block).toMatch(/update_stack_env_raw/); + }); + + it('variable schema still includes key, value, and optional isSecret', () => { + expect(block).toMatch(/key:\s*z\.string\(\)/); + expect(block).toMatch(/value:\s*z\.string\(\)/); + expect(block).toMatch(/isSecret:\s*z\.boolean\(\)\.optional\(\)/); + }); +}); diff --git a/tests/stack-env-tools.test.ts b/tests/stack-env-tools.test.ts index 5ababb1..2edf5db 100644 --- a/tests/stack-env-tools.test.ts +++ b/tests/stack-env-tools.test.ts @@ -71,9 +71,11 @@ describe('update_stack_env (rawContent cleanup)', () => { expect(block).not.toMatch(/body\.rawContent/); }); - it('sends {variables} directly as the request body', () => { + it('sends variables as the "variables" key in the PUT request body', () => { const block = extractToolBlock(stacksSource, 'update_stack_env'); - expect(block).toMatch(/\{\s*variables\s*\}/); + // After the merge-semantic refactor the PUT body uses finalVariables, + // not the raw input — but the JSON key sent to the API is still "variables". + expect(block).toMatch(/variables:\s*finalVariables/); }); it('declares variables as a required parameter', () => {