fix(#2046): config-set <key> null unsets (removes) the key instead of writing "null"

`gsd-tools config-set <key> null` — the "Clear" action documented in
settings-integrations.md / settings-advanced.md — previously fell through the
value parser as the literal STRING "null" and persisted it. Consequences:
"cleared" keys stayed set (config-get returned truthy "null"), and for secret
keys (brave_search/firecrawl/exa_search) a masked success line hid a truthy
4-char value on disk that integrations could pass along as a real credential.
There was also no unset/delete verb at all.

Fix: parse a bare `null` to JS null and short-circuit to a real UNSET that
DELETES the key from config.json — the semantic the docs already describe
("Remove the stored key" / "remove the key by setting it to null"). Deleting
(not persisting JSON null) is the correct clear: a persisted null is still a
present value consumers must special-case.

- src/config.cts:
  - parse block: `else if (val === 'null') parsedValue = null;`
  - cmdConfigSet: when parsedValue === null, short-circuit BEFORE the typed
    per-key validator gauntlet (so clearing an enum/boolean/number key removes
    it rather than being rejected) and before the project_code special-case;
    mask the previous value for secret keys in the output.
  - new `unsetConfigValue()` + `_unsetNestedValue()` mirroring setConfigValue/
    _setNestedValue: same prototype-pollution guard, but never creates missing
    intermediates and never prunes empty parents; returns { previousValue,
    existed }. Unsetting a never-set key is an idempotent no-op success.
- tests/config.test.cjs: new suite covering non-secret routing key, secret key,
  typed-enum-key bypass (context), idempotent unset, literal-"null"-on-disk
  guard, the unset-path prototype-pollution guard (alert #26 parity), and a
  4-segment deep-nested unset.
- tests/review-model-config.test.cjs: update the stale round-trip test that
  codified the bug (asserted config-set null → config-get returns "null") to the
  fixed contract — the model key is removed; the review workflow's
  `[ -n "$VAR" ] && [ "$VAR" != "null" ]` guard handles the empty read as
  "no override → reviewer default", same as the old "null" sentinel.

The 4 documented "Clear" flows (settings-integrations.md, settings-advanced.md)
were verified — their prose already describes removal, so the fix makes them
accurate rather than aspirational; no doc wording change required.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

.changeset/2046-config-set-null-unset.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 2058
+---
+**`gsd-tools config-set <key> null` now clears (removes) the key instead of persisting the literal string `"null"`.** The documented "Clear" action previously fell through the value parser and stored `"null"` — a truthy value — so "cleared" keys stayed set and `config-get` returned `"null"`; for secret keys (`brave_search`/`firecrawl`/`exa_search`) a masked success line hid a truthy value on disk that integrations could pass along as a real credential. `config-set <key> null` now deletes the key (short-circuiting the typed per-key validators so clearing an enum/boolean/number key removes it rather than being rejected), making the "Clear" flows in `settings-integrations.md` / `settings-advanced.md` actually clear.

src/config.cts

@@ -38,6 +38,14 @@ interface SetConfigValueResult {
   previousValue: unknown;
 }
 
+interface UnsetConfigValueResult {
+  updated: boolean;
+  unset: true;
+  key: string;
+  value: null;
+  previousValue: unknown;
+}
+
 interface WorkstreamContext {
   configPath?: string;
   [key: string]: unknown;
@@ -452,6 +460,87 @@ function _setNestedValue(
   return previousValue;
 }
 
+/**
+ * Deletes a value from the config object, allowing nested values via dot
+ * notation (e.g., "review.models.gemini"). Mirrors `_setNestedValue`'s
+ * prototype-pollution guard on every path segment (including intermediates).
+ *
+ * Unlike `_setNestedValue`, this NEVER creates missing intermediate objects —
+ * if any segment along the path is missing (or not a plain, non-array
+ * object), the key doesn't exist and we return early without mutating
+ * `config` at all.
+ *
+ * Does not prune now-empty parent objects after deletion (matches the
+ * conservative, structure-preserving behaviour callers expect from a bare
+ * unset).
+ *
+ * Returns { previousValue, existed } — existed is false when the leaf key
+ * (or an intermediate segment) was never present.
+ * Calls error() (process.exit(1)) on prototype-pollution attempts.
+ */
+function _unsetNestedValue(
+  config: Record<string, unknown>,
+  keyPath: string,
+): { previousValue: unknown; existed: boolean } {
+  const keys = keyPath.split('.');
+  let current: Record<string, unknown> = config;
+  for (let i = 0; i < keys.length - 1; i++) {
+    const key = keys[i];
+    if (key === '__proto__' || key === 'prototype' || key === 'constructor') {
+      error('Invalid config key (prototype pollution guard): ' + keyPath, ERROR_REASON.CONFIG_PARSE_FAILED);
+    }
+    const existingChild = current[key];
+    if (existingChild === undefined || existingChild === null || typeof existingChild !== 'object' || Array.isArray(existingChild)) {
+      // Path doesn't exist — nothing to unset, and we must not create it.
+      return { previousValue: undefined, existed: false };
+    }
+    current = existingChild as Record<string, unknown>;
+  }
+  const lastKey = keys[keys.length - 1];
+  if (lastKey === '__proto__' || lastKey === 'prototype' || lastKey === 'constructor') {
+    error('Invalid config key (prototype pollution guard): ' + keyPath, ERROR_REASON.CONFIG_PARSE_FAILED);
+  }
+  const existed = Object.prototype.hasOwnProperty.call(current, lastKey);
+  const previousValue = current[lastKey];
+  if (existed) {
+    delete current[lastKey];
+  }
+  return { previousValue, existed };
+}
+
+/**
+ * Deletes a key from the config file, allowing nested values via dot
+ * notation. Mirrors `setConfigValue`'s load/lock/write cycle.
+ *
+ * Does not call `output()`, so can be used as one step in a command without triggering `exit(0)` in
+ * the happy path. But note that `error()` will still `exit(1)` out of the process.
+ */
+function unsetConfigValue(cwd: string, keyPath: string): UnsetConfigValueResult {
+  const configPath = path.join(planningDir(cwd), 'config.json');
+
+  return withPlanningLock(cwd, () => {
+    // Load existing config or start with empty object
+    let config: Record<string, unknown> = {};
+    try {
+      if (fs.existsSync(configPath)) {
+        config = JSON.parse(fs.readFileSync(configPath, 'utf-8')) as Record<string, unknown>;
+      }
+    } catch (err) {
+      error('Failed to read config.json: ' + (err as Error).message, ERROR_REASON.CONFIG_PARSE_FAILED);
+    }
+
+    const { previousValue, existed } = _unsetNestedValue(config, keyPath);
+
+    // Write back
+    try {
+      platformWriteSync(configPath, JSON.stringify(config, null, 2));
+      return { updated: existed, unset: true, key: keyPath, value: null, previousValue };
+    } catch (err) {
+      error('Failed to write config.json: ' + (err as Error).message);
+    }
+  }) as UnsetConfigValueResult;
+}
+
 /**
  * Sets a value in the config file, allowing nested values via dot notation (e.g.,
  * "workflow.research").
@@ -586,6 +675,7 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string |
   let parsedValue: unknown = val;
   if (val === 'true') parsedValue = true;
   else if (val === 'false') parsedValue = false;
+  else if (val === 'null') parsedValue = null;
   // #1581: Number.isFinite (not !isNaN) so 'Infinity'/'-Infinity' are NOT
   // coerced to non-finite numbers that JSON.stringify later renders as `null`
   // (disk=null while the CLI echoed 'Infinity'). They fall through to the
@@ -596,6 +686,25 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string |
     try { parsedValue = JSON.parse(val); } catch { /* keep as string */ }
   }
 
+  // #2046: a bare `null` unsets (deletes) the key — the documented "Clear" action.
+  // Short-circuits before every typed per-key validator so clearing a typed key
+  // (enum/boolean/number) removes it rather than being rejected. Deleting (not
+  // persisting JSON null) is the correct "clear": a persisted null is still a
+  // present, truthy-adjacent value that consumers must special-case — worst for
+  // secret keys where a leftover value can be passed as a real credential.
+  if (parsedValue === null) {
+    const unsetResult = unsetConfigValue(cwd, kp);
+    if (isSecretKey(kp)) {
+      const maskedPrev = unsetResult.previousValue === undefined
+        ? undefined
+        : maskSecret(unsetResult.previousValue as Parameters<typeof maskSecret>[0]);
+      output({ ...unsetResult, value: null, previousValue: maskedPrev, masked: true }, raw, `${kp} unset`);
+      return;
+    }
+    output(unsetResult, raw, `${kp} unset`);
+    return;
+  }
+
   // #1581: project_code is an identifier string — never number-coerce it. A
   // leading-zero code like '007' must persist verbatim (not collapse to 7).
   if (kp === 'project_code') {

tests/config.test.cjs

@@ -747,6 +747,131 @@ describe('config-set — no silent coercion (#1581)', () => {
   });
 });
 
+// ─── config-set <key> null — unset/clear (#2046) ─────────────────────────────
+
+describe('config-set <key> null — unset/clear (#2046)', () => {
+  let tmpDir;
+
+  beforeEach(() => { tmpDir = createTempProject(); });
+  afterEach(() => { cleanup(tmpDir); });
+
+  test('non-secret routing key: config-set <key> null removes the key (not the string "null")', () => {
+    const setResult = runGsdTools('config-set review.models.gemini foo', tmpDir);
+    assert.ok(setResult.success, `Command failed: ${setResult.error}`);
+    assert.strictEqual(readConfig(tmpDir).review.models.gemini, 'foo');
+
+    const unsetResult = runGsdTools('config-set review.models.gemini null', tmpDir);
+    assert.ok(unsetResult.success, `Command failed: ${unsetResult.error}`);
+
+    const config = readConfig(tmpDir);
+    assert.ok(
+      !config.review || !config.review.models || !Object.prototype.hasOwnProperty.call(config.review.models, 'gemini'),
+      'review.models.gemini must be absent on disk after unset, not the string "null"'
+    );
+
+    const getResult = runGsdTools('config-get review.models.gemini', tmpDir);
+    assert.ok(
+      !getResult.output || !getResult.output.trim() || getResult.output.trim() === 'undefined',
+      `config-get should return empty/undefined after unset, got: ${getResult.output}`
+    );
+    assert.notStrictEqual(getResult.output && getResult.output.trim(), 'null');
+  });
+
+  test('secret key: config-set brave_search null removes the key (never persists "null")', () => {
+    const setResult = runGsdTools('config-set brave_search sk-test-1234', tmpDir);
+    assert.ok(setResult.success, `Command failed: ${setResult.error}`);
+    assert.strictEqual(readConfig(tmpDir).brave_search, 'sk-test-1234');
+
+    const unsetResult = runGsdTools('config-set brave_search null', tmpDir);
+    assert.ok(unsetResult.success, `Command failed: ${unsetResult.error}`);
+
+    const configPath = path.join(tmpDir, '.planning', 'config.json');
+    const rawText = fs.readFileSync(configPath, 'utf-8');
+    const config = JSON.parse(rawText);
+    assert.ok(
+      !Object.prototype.hasOwnProperty.call(config, 'brave_search'),
+      'brave_search must be absent on disk after unset'
+    );
+    assert.doesNotMatch(rawText, /"brave_search":\s*"null"/,
+      'raw config.json must never contain brave_search mapped to the literal string "null"');
+  });
+
+  test('typed-key bypass: config-set context null clears an enum-typed key without validator rejection', () => {
+    const setResult = runGsdTools('config-set context dev', tmpDir);
+    assert.ok(setResult.success, `Command failed: ${setResult.error}`);
+    assert.strictEqual(readConfig(tmpDir).context, 'dev');
+
+    const unsetResult = runGsdTools('config-set context null', tmpDir);
+    assert.ok(unsetResult.success, `config-set context null must succeed (bypass enum validator), got error: ${unsetResult.error}`);
+
+    const config = readConfig(tmpDir);
+    assert.ok(!Object.prototype.hasOwnProperty.call(config, 'context'),
+      'context key must be removed after unset');
+  });
+
+  test('idempotent: config-set <never-set key> null succeeds with no crash and no key written', () => {
+    const result = runGsdTools('config-set git.base_branch null', tmpDir);
+    assert.ok(result.success, `Command failed: ${result.error}`);
+
+    const config = readConfig(tmpDir);
+    assert.ok(
+      !config.git || !Object.prototype.hasOwnProperty.call(config.git, 'base_branch'),
+      'git.base_branch must not be present after unsetting a key that was never set'
+    );
+  });
+
+  test('literal-"null" guard: no config-set <key> null ever persists the literal string "null" on disk', () => {
+    runGsdTools('config-set review.models.gemini foo', tmpDir);
+    runGsdTools('config-set review.models.gemini null', tmpDir);
+    runGsdTools('config-set brave_search sk-test-1234', tmpDir);
+    runGsdTools('config-set brave_search null', tmpDir);
+    runGsdTools('config-set context dev', tmpDir);
+    runGsdTools('config-set context null', tmpDir);
+
+    const configPath = path.join(tmpDir, '.planning', 'config.json');
+    const rawText = fs.readFileSync(configPath, 'utf-8');
+    assert.doesNotMatch(rawText, /:\s*"null"/,
+      `config.json must never contain a key mapped to the literal string "null": ${rawText}`);
+  });
+
+  test('prototype-pollution guard: unsetting a __proto__ leaf is rejected, no pollution (alert #26 parity)', () => {
+    // Create the intermediate so the unset walk reaches the leaf guard (a bare
+    // dynamic-prefix key passes the schema gate, so this exercises the
+    // _unsetNestedValue guard, not the schema gate — mirrors the set-path test).
+    const seed = runGsdTools('config-set agent_skills.sonnet-coder true', tmpDir);
+    assert.ok(seed.success, `seed failed: ${seed.error}`);
+
+    const result = runGsdTools('config-set agent_skills.__proto__ null', tmpDir);
+    assert.strictEqual(result.success, false, `Expected rejection, got: ${result.output}`);
+    assert.ok(
+      result.error.includes('prototype pollution guard'),
+      `Expected pollution-guard error, got: ${result.error}`,
+    );
+    // No prototype pollution occurred via the unset path.
+    assert.strictEqual(({}).polluted, undefined);
+    assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'polluted'), false);
+  });
+
+  test('deep (4-segment) nested key: config-set <deep> null removes only the leaf', () => {
+    const set = runGsdTools('config-set review.reviewer_instances.myinst.model some-model', tmpDir);
+    assert.ok(set.success, `deep set failed: ${set.error}`);
+    assert.strictEqual(readConfig(tmpDir).review.reviewer_instances.myinst.model, 'some-model');
+
+    const unset = runGsdTools('config-set review.reviewer_instances.myinst.model null', tmpDir);
+    assert.ok(unset.success, `deep unset failed: ${unset.error}`);
+
+    const config = readConfig(tmpDir);
+    assert.ok(
+      !config.review.reviewer_instances.myinst ||
+        !Object.prototype.hasOwnProperty.call(config.review.reviewer_instances.myinst, 'model'),
+      'the leaf model key must be removed',
+    );
+    // Parent objects along the path are preserved (unset removes only the leaf).
+    assert.ok(config.review && config.review.reviewer_instances,
+      'parent objects on the path must remain after leaf unset');
+  });
+});
+
 // ─── config-set (research_before_questions and discuss_mode) ──────────────────
 
 describe('config-set research_before_questions and discuss_mode', () => {

tests/review-model-config.test.cjs

@@ -4,11 +4,14 @@
  * Verifies the review.models.<cli> dynamic config key pattern:
  *   - isValidConfigKey accepts review.models.<cli-name>
  *   - validateKnownConfigKeyPath suggests review.models.<cli-name> for review.model
- *   - End-to-end round-trip via config-set / config-get for both model IDs and null
+ *   - End-to-end round-trip via config-set / config-get for model IDs and the
+ *     null "Clear" action (#2046 — config-set <key> null unsets the key)
  */
 
 const { test, describe, beforeEach, afterEach } = require('node:test');
 const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
 const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
 
 describe('review.models.<cli> config key', () => {
@@ -110,28 +113,49 @@ describe('review.models.<cli> config key', () => {
     );
   });
 
-  test('round-trip: config-set null then config-get returns "null"', () => {
-    // The issue spec documents null as the "fall back to CLI default" sentinel.
-    // cmdConfigSet does not parse 'null' as JSON null — it stores the literal
-    // string 'null'. config-get --raw returns the string 'null', and the
-    // workflow's `[ "$VAR" != "null" ]` guard handles this.
+  test('round-trip: config-set null UNSETS the model key (#2046 — the "Clear" action)', () => {
+    // #2046: `config-set <key> null` now DELETES the key (the documented "Clear"
+    // action) instead of persisting the literal string "null". A previously-set
+    // model override is removed cleanly; config-get then reports key-not-found.
+    // The review workflow's guard (`[ -n "$VAR" ] && [ "$VAR" != "null" ]`,
+    // review.md:259) treats the resulting empty read as "no override → use the
+    // reviewer's default", exactly as it treated the old "null" sentinel.
     const setResult = runGsdTools(
+      ['config-set', 'review.models.gemini', 'gemini-3.1-pro-preview'],
+      tmpDir,
+      { HOME: tmpDir, USERPROFILE: tmpDir }
+    );
+    assert.ok(setResult.success, `config-set failed: ${setResult.error}`);
+
+    const clearResult = runGsdTools(
       ['config-set', 'review.models.gemini', 'null'],
       tmpDir,
       { HOME: tmpDir, USERPROFILE: tmpDir }
     );
-    assert.ok(setResult.success, `config-set null failed: ${setResult.error}`);
+    assert.ok(clearResult.success, `config-set null failed: ${clearResult.error}`);
 
+    // The key is gone from disk — not persisted as the string "null".
+    const configPath = path.join(tmpDir, '.planning', 'config.json');
+    const rawText = fs.readFileSync(configPath, 'utf-8');
+    const config = JSON.parse(rawText);
+    assert.ok(
+      !config.review || !config.review.models ||
+        !Object.prototype.hasOwnProperty.call(config.review.models, 'gemini'),
+      `review.models.gemini must be absent after clear, got: ${rawText}`
+    );
+    assert.doesNotMatch(rawText, /"gemini":\s*"null"/,
+      'must never persist review.models.gemini as the literal string "null"');
+
+    // config-get on the removed key reports not-found (the review workflow reads
+    // it as `... 2>/dev/null || echo ""` → empty → the `[ -n "$VAR" ]` guard falls
+    // back to the reviewer default).
     const getResult = runGsdTools(
       ['config-get', 'review.models.gemini', '--raw'],
       tmpDir,
       { HOME: tmpDir, USERPROFILE: tmpDir }
     );
-    assert.ok(getResult.success, `config-get failed: ${getResult.error}`);
-    assert.strictEqual(
-      getResult.output,
-      'null',
-      'config-get should return the literal string "null" so the workflow guard can match it'
-    );
+    assert.ok(!getResult.success, 'config-get on a cleared key should report not-found');
+    assert.notStrictEqual(getResult.output && getResult.output.trim(), 'null',
+      'config-get must not emit the literal string "null" for a cleared key');
   });
 });