feat(tools): stack-env ergonomics — summary/hint, remove_stack_env_vars, collision check#105
Conversation
…-secret loss)
The /env PUT payload previously kept only isSecret vars from the
structured GET, dropping any non-secret DB-managed variables (git
stacks store non-secrets in the DB too, not just in .env). Rebuild
the full remaining DB-backed set (secrets masked '***', DB-managed
non-secrets kept as-is) minus the removal targets instead.
Response shape changes from {removed_secrets, removed_env, not_found}
to {removed, not_found} (plus optional error on partial .env-rewrite
failure) since a removal target can now live in either store.
parseDotEnvKeys and removeKeysFromDotEnv treated a leading "export " token as part of the key, so "export KEY=value" lines were neither recognized as an existing key nor removable. Strip the optional "export " prefix after trimming, before extracting the key.
…or path - remove_stack_env_vars: regression test asserting that removing one secret does not drop unrelated DB-managed non-secrets (git-stack vars not present in .env) from the rebuilt /env PUT payload. - remove_stack_env_vars: error path for the first (/env) PUT failing — reported via `error`, and the /env/raw PUT must not be attempted. - env-helpers: parseDotEnvKeys/removeKeysFromDotEnv "export KEY=" cases.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two new MCP tools, remove_stack_env_vars and check_stack_env_collisions, along with helper utilities for parsing, diffing, and removing environment variables across database-backed and .env stores. It also updates update_stack_env to return a diff summary and hint in merge mode, and adds comprehensive test coverage. The review feedback focuses on improving robustness by adding defensive checks when processing variables returned from external API responses in diffEnvVars, remove_stack_env_vars, and check_stack_env_collisions to prevent potential runtime TypeError crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 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); |
There was a problem hiding this comment.
In diffEnvVars wird das existing-Array direkt über v.key gemappt, ohne zu prüfen, ob v definiert ist oder ob v.key ein String ist. Da existing direkt aus der externen API-Antwort stammt (existingVars in update_stack_env), kann es null-, undefined- oder fehlerhafte Elemente enthalten. Dies führt zu einem Laufzeit-TypeError und bringt das Tool zum Absturz.
Wir sollten das existing-Array vor der Verarbeitung defensiv filtern, um sicherzustellen, dass alle Elemente valide Objekte mit einer key-Eigenschaft sind.
const validExisting = existing.filter((v) => v && typeof v.key === 'string');
const existingByKey = new Map(validExisting.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 = validExisting.filter((v) => !payloadKeys.has(v.key)).map((v) => v.key);| 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)); |
There was a problem hiding this comment.
In remove_stack_env_vars wird das vars-Array direkt über v.key und v.isSecret gemappt und gefiltert, ohne zu prüfen, ob v definiert ist oder ob v.key ein String ist. Wenn die API-Antwort null-, undefined- oder fehlerhafte Elemente enthält, führt dies zu einem Laufzeit-TypeError und bringt das Tool zum Absturz.
Wir sollten das vars-Array defensiv filtern, um sicherzustellen, dass alle Elemente valide Objekte mit einer key-Eigenschaft sind, bevor wir sie verarbeiten. Dies entspricht dem defensiven Muster, das auch in update_stack_env verwendet wird.
| 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 vars = (Array.isArray(structured?.variables) ? structured.variables : []) | |
| .filter((v) => v && typeof v.key === 'string'); | |
| const structuredKeys = new Set(vars.map((v) => v.key)); | |
| const secretKeys = new Set(vars.filter((v) => v.isSecret).map((v) => v.key)); |
| const secretKeys = new Set( | ||
| (Array.isArray(structured?.variables) ? structured.variables : []) | ||
| .filter((v) => v.isSecret).map((v) => v.key)); |
There was a problem hiding this comment.
In check_stack_env_collisions wird das structured?.variables-Array direkt über v.isSecret und v.key gefiltert und gemappt, ohne zu prüfen, ob v definiert ist oder ob v.key ein String ist. Wenn die API-Antwort null-, undefined- oder fehlerhafte Elemente enthält, führt dies zu einem Laufzeit-TypeError und bringt das Tool zum Absturz.
Wir sollten das Array defensiv filtern, um sicherzustellen, dass alle Elemente valide Objekte mit einer key-Eigenschaft sind, bevor wir isSecret prüfen.
const secretKeys = new Set(
(Array.isArray(structured?.variables) ? structured.variables : [])
.filter((v) => v && v.isSecret && typeof v.key === 'string')
.map((v) => v.key));There was a problem hiding this comment.
Pull request overview
Additive improvements to the stack environment-variable tooling to better support Dockhand’s two-store model (DB secrets vs .env), including safer deletion ergonomics and better merge-mode feedback.
Changes:
- Extend
update_stack_env(merge mode only) to return asummaryof changes plus a user-facinghintwhen nothing was removed. - Add
remove_stack_env_varsto remove keys across DB-backed variables and.env(with masking/preservation rules). - Add
check_stack_env_collisionsto detect keys that exist both as DB secrets and in.env, plus helper utilities/tests for.envparsing and key removal.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/tools/stacks.ts |
Adds merge-mode summary/hint to update_stack_env; introduces remove_stack_env_vars and check_stack_env_collisions. |
src/utils/env-helpers.ts |
New helper utilities for env diffing and .env parsing/rewriting. |
tests/stack-env-merge-behavior.test.ts |
Tests merge-mode summary + hint, and asserts replace mode contract remains unchanged. |
tests/stack-env-remove.test.ts |
Tests deletion behavior across stores, including a regression for DB-held non-secrets. |
tests/stack-env-collisions.test.ts |
Tests collision reporting for DB secret vs .env duplicates. |
tests/env-helpers.test.ts |
Unit tests for diffEnvVars, parseDotEnvKeys, and removeKeysFromDotEnv. |
tests/tool-descriptions.test.ts |
Ensures the new tools are included in the expected tooling cluster. |
| 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 } : {}), | ||
| }); |
| } 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)}`, | ||
| }); | ||
| } |
Gemini review: diffEnvVars, remove_stack_env_vars and check_stack_env_collisions
mapped over externally-sourced variable arrays without guarding against
null/undefined elements or non-string keys — a malformed API response could
crash the tool with a TypeError. Filter for valid { key: string } entries first,
consistent with the existing guard in update_stack_env. Adds a defensive test.
|
Danke @gemini-code-assist — alle drei Findings adressiert (Commit |
|
Danke für das Update. Da die defensiven Filter implementiert wurden und die Testabdeckung sowie der Typecheck erfolgreich sind, habe ich keine weiteren Anmerkungen. Die Änderungen sehen aus technischer Sicht konsistent aus. |
|
🎉 This PR is included in version 1.9.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
…) (#110) * feat(tools): add upsertDotEnv helper Pure helper for env-var auto-routing: replaces an existing KEY= line (preserving an optional "export " prefix, comments, blank lines and order) or appends a new KEY=value line when the key is absent. Prepares update_stack_env to write isSecret:false vars into .env. * feat(tools): route isSecret:false vars to .env in update_stack_env update_stack_env used to send every variable (secret or not) through PUT /env, the DB-only store. Non-secrets landed in an orphaned DB row that get_stack_env never surfaces, since it always reads non-secrets from the .env file — the value was silently never applied (#109). update_stack_env now auto-routes: isSecret:true -> DB (PUT /env, as before), isSecret:false -> the .env file (PUT /env/raw), upserted via upsertDotEnv in merge mode or fully rebuilt in replace mode. The merge-mode structured GET stays load-bearing (GET failure -> no write at all, per #105). Response gains db.secretsWritten/env.nonSecretsWritten so callers can see where a value actually landed; a failure in the .env step after a successful DB write is reported as a partial success (error set, db.secretsWritten still reflects what was persisted). Replace mode now also does a best-effort GET purely to compute the summary (a GET failure never blocks the write, since replace is a deliberate full wipe) — updates the #105 "replace never GETs" contract accordingly. * test(tools): repro tests for update_stack_env review findings Locks in the intended behaviour for the review findings on update_stack_env's DB/.env auto-routing (see #109 follow-up): - Critical 1: degrading the only existing secret to isSecret:false must still flush the stale encrypted DB row (secrets.length alone is not enough to decide whether the DB PUT should fire). - Critical 2: promoting a plain .env key to isSecret:true must scrub it out of .env, not just add it to the DB. - Critical 3: an orphaned non-secret DB row untouched by a call must be migrated into .env before the DB PUT (DELETE-all+INSERT) drops it, and must never be reported as "preserved" in the summary. - Critical 4: replace mode reverts to the original #105 contract - no GET, no summary/hint (removes the #109 best-effort summary GET). - Important 5: the merge summary baseline must combine DB secrets AND .env keys, so a key already tracked in .env is "updated" not "added", and an omitted .env key is "preserved". - Important 6: extra fields on the underlying PUT responses should be passed through additively. - Minor 7: duplicate keys in the payload must be de-duplicated before building the DB/.env write sets. Also updates the "pure secret payload" test: promoting/creating a secret now always checks .env for a same-named plaintext copy to scrub (Critical 2), so a raw GET+PUT happens even for a brand-new key. These tests are red against the current handler. * fix(tools): correct update_stack_env DB/.env routing edge cases Fixes 7 review findings on the merge/replace auto-routing added in the previous commits: - Critical 1: the DB PUT now also fires when secrets existed before the call but the payload demotes the last one to isSecret:false (secrets.length would otherwise be 0 and the stale encrypted row would never be purged). - Critical 2: keys explicitly promoted to isSecret:true this call (promotedKeys) are now scrubbed from .env via removeKeysFromDotEnv, so the plaintext copy cannot linger next to the new DB secret. - Critical 3: orphaned non-secret DB rows (isSecret:false/undefined entries in the structured store - not supposed to exist post-fix, but possible leftovers) are migrated into .env before the DB PUT would otherwise silently delete them via DELETE-all+INSERT. - Critical 4: replace mode drops the #109 best-effort summary GET entirely - back to the original #105 contract (no GET, no summary, no hint) since replace is a deliberate full wipe, not a preview. - Important 5: the merge summary's diff baseline now combines DB secrets (isSecret:true) with .env keys (parsed from the raw GET, when one already happened) instead of the raw structured GET, which used to include orphaned DB rows and misreport them as "preserved" right before they were deleted. - Important 6: the JSON response now spreads through any extra fields on the underlying PUT responses before applying success/db/env/ summary/hint/error, so passthrough API metadata is not dropped. - Minor 7: the incoming variables array is de-duplicated by key (last one wins) before it feeds into any of the above. All 340 tests pass, including the new repro suite from the previous commit. * fix(tools): protect live .env value from stale DB-orphan migration (N1) Re-review found the Critical-3 migration could overwrite a live .env value with the stale value of an orphaned DB non-secret row sharing the same key. Only migrate orphaned DB rows whose key is NOT already in .env — the live .env is authoritative for non-secrets; the stale DB row is dropped by the DB PUT regardless. * fix(tools): upsertDotEnv preserves trailing newline when appending Gemini review: appending to .env content that ends with a trailing newline inserted a spurious blank line (A=1\n\nB=2). Drop the empty trailing element before appending and restore the newline afterwards. Adds a test.
## [1.9.1](v1.9.0...v1.9.1) (2026-07-19) ### Bug Fixes * **tools:** route isSecret:false vars to .env in update_stack_env ([#109](#109)) ([#110](#110)) ([b637efb](b637efb)), closes [#105](#105) [#105](#105) [#105](#105) [#105](#105) ### Documentation * **release:** backfill v1.9.0 changelog entry ([#108](#108)) ([c4d49af](c4d49af)) ### CI/CD * **release:** restore @semantic-release/git and changelog plugins ([#107](#107)) ([e02ae91](e02ae91))
Was
Drei additive, nicht-breaking Verbesserungen rund um Stack-Env, hervorgegangen aus einer Investigation zum Dockhand-Zwei-Speicher-Modell (DB-Secrets vs.
.env):update_stack_envliefert immerge-Modus zusätzlichsummary {added, updated, preserved, removed}+ einenhint, wenn nichts entfernt wurde (das „ich-wollte-eigentlich-löschen"-Muster).replacebleibt unverändert.success/countbleiben.remove_stack_env_vars(env, name, keys[])— entfernt Keys stack-typ-bewusst über beide Speicher: DB-Rebuild der vollen verbleibenden Menge minus Ziel-Keys (Secrets via***-Bewahrung; DB-gehaltene Nicht-Secrets bleiben erhalten),.env-Rewrite für.env-gehaltene Keys. Kollisionen aus beiden. Return{ removed, not_found }.check_stack_env_collisions(env, name)— read-only, meldet Keys die gleichzeitig DB-Secret und in.envsind.diffEnvVars,parseDotEnvKeys,removeKeysFromDotEnv(inkl.export KEY=-Support).Qualität
docker buildsauber.update_stack_env(GET-Fehler → kein PUT) unverändert.Motivation/Details: Investigation zum Zwei-Speicher-Modell (AFFiNE-Stack-Env-Duplikat auf HHDOCKER03).