mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
81de9f66a6
Fixes #643. ## Before / after ``` $ argent config set ios.additionalDeviceSets "~/Library/Developer/Radon/Devices" --scope project Error: Invalid value for config key "ios.additionalDeviceSets". # ← the whole message ``` ``` $ argent config set ios.additionalDeviceSets "~/Library/Developer/Radon/Devices" --scope project Error: Invalid value for config key "ios.additionalDeviceSets": expected an array of strings. Did you mean: argent config set ios.additionalDeviceSets '["~/Library/Developer/Radon/Devices"]' --scope project Run `argent config list` to see each key's expected value. ``` ``` $ argent config unset ios.additionalDeviceSets --scope project $ cat .argent/config.json { "ios": {} } → {} ``` ## Symptom 1 — the message The issue names three gaps: no expected type, no example, no pointer to `config list`. All three are closed, plus one the issue didn't ask for and I think is the actual fix. **The suggestion carries the user's own value.** A generic `Example: … '["~/DeviceSets/ci"]'` would hand someone a path they have never heard of and expect them to spot the difference. Where the correction is mechanical — a single item offered to a list-valued key — the error offers back what they typed, wrapped, shell-quoted, and **on the scope they asked for**. Pasting it works; verified live that it lands in project scope, not global. Nothing is coerced. The issue floated accepting a bare string and storing it wrapped; I didn't, because `config set` persists, gets committed, and is read by other machines, so a value silently stored in a shape the user didn't type is expensive to debug and `coerceCliValue` already has one silent fallback. Showing the corrected command costs the user one keystroke and keeps the store honest. If a reviewer prefers coercion, this is the smaller change to build on. **The expected shape comes from the validator, not a per-key string** — `asStringArray` *is* "an array of strings", so swapping a key's `parse` swaps its description with it and the two cannot drift. A bespoke validator can override via a new optional `expected`, and degrades to today's exact message if it says nothing. **`config list` states it too.** The issue notes `list` doesn't close the gap either — its description describes the *elements* but never says the value is a list. It now shows `value: an array of strings, e.g. ["~/DeviceSets/ci"]`, and `list --json` carries both fields. `telemetry.enabled` deliberately gets no example: `setConfigValue` refuses it before validating (it's managed by `argent telemetry`), so a "here's how to set it" hint would point at a command you may not run. A schema guard test keeps that honest as keys are added — and asserts every example **actually round-trips through its own validator**, so an example can never ship that reproduces the error it exists to fix. ## Symptom 2 — the empty group Deleting a key now removes any container the delete emptied, stopping at the first ancestor that still holds something. **This is file hygiene, not a behaviour change.** I traced every raw reader of the document — `readConfigObject`, and telemetry's `consent.ts` and `notice.ts`, which each do their own read — and an empty parent is already indistinguishable from an absent one: `getAtPath` returns `undefined` either way, before `parse`, so the merge layer never sees a container at all. The only observable difference is the bytes on disk, which is exactly the complaint. Guards worth noting: - **The root always survives** — an emptied config is `{}`, never a deleted file. `.argent/` is itself a project-root marker, so removing the file wouldn't restore the prior state anyway, and `updateConfig` is a shared publish path with a lock/rename protocol that has no defined meaning for file removal. - **A no-op unset still doesn't touch the file.** That fast path exists so an unset that removes nothing can't materialise a project config and dirty git status; pruning lives inside the mutate callback, which the fast path returns before reaching. So a group someone deliberately left empty by hand is never tidied away — pinned by a test asserting the file stays byte-identical. - Siblings at any level stop the unwind, including an empty array value, which is preserved. **Two existing tests asserted the old behaviour and now assert the new one** — `config-access.test.ts:47` and `config.test.ts:109` (the latter is `clearRememberedAgent`'s sibling-preservation test, so it doubles as proof siblings survive). ## Known same-class case, not fixed here `resetFirstRunNotice` (`packages/telemetry/src/notice.ts`) does its own inline delete and still leaves `{"notices":{}}`. It doesn't go through `deleteAtPath`, so it doesn't inherit this. It's a one-line change to route it through the same function — happy to do it here or as a follow-up, but it pulls the telemetry package into a config PR so I left it out by default. ## Verification Live against the built CLI: the reported case; pasting the suggestion back and confirming it lands in **project** scope; unset leaving `{}`; a sibling key surviving; a non-array key correctly getting the generic example and *no* bogus "Did you mean"; `config list` output; `telemetry.enabled` still delegating unchanged. configuration-core 103 passed, argent-cli 285 passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
118 lines
3.7 KiB
TypeScript
118 lines
3.7 KiB
TypeScript
import { describe, it, expect, beforeEach, afterEach } from "vitest";
|
|
import * as fs from "node:fs";
|
|
import * as path from "node:path";
|
|
import * as os from "node:os";
|
|
import { argentHomeDir, configFilePath } from "../src/paths.js";
|
|
import { readConfigObject, updateConfig } from "../src/config.js";
|
|
import {
|
|
getRememberedAgent,
|
|
setRememberedAgent,
|
|
clearRememberedAgent,
|
|
} from "../src/config-access.js";
|
|
|
|
// Redirect the `~/.argent` home into a tmp dir by mutating process.env.HOME
|
|
// (consumed by os.homedir() via argentHomeDir).
|
|
let tmpHome: string;
|
|
let originalHome: string | undefined;
|
|
let originalUserProfile: string | undefined;
|
|
|
|
beforeEach(() => {
|
|
tmpHome = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "argent-config-home-")));
|
|
originalHome = process.env.HOME;
|
|
originalUserProfile = process.env.USERPROFILE;
|
|
process.env.HOME = tmpHome;
|
|
process.env.USERPROFILE = tmpHome;
|
|
});
|
|
|
|
afterEach(() => {
|
|
if (originalHome === undefined) delete process.env.HOME;
|
|
else process.env.HOME = originalHome;
|
|
if (originalUserProfile === undefined) delete process.env.USERPROFILE;
|
|
else process.env.USERPROFILE = originalUserProfile;
|
|
fs.rmSync(tmpHome, { recursive: true, force: true });
|
|
});
|
|
|
|
function readConfigFile(): unknown {
|
|
return JSON.parse(fs.readFileSync(configFilePath(), "utf8"));
|
|
}
|
|
|
|
describe("paths", () => {
|
|
it("locates config.json under the argent home", () => {
|
|
expect(configFilePath()).toBe(path.join(argentHomeDir(), "config.json"));
|
|
expect(argentHomeDir()).toBe(path.join(tmpHome, ".argent"));
|
|
});
|
|
});
|
|
|
|
describe("readConfigObject / updateConfig", () => {
|
|
it("returns an empty object when the file is missing", () => {
|
|
expect(readConfigObject()).toEqual({});
|
|
});
|
|
|
|
it("returns an empty object when the file is malformed", () => {
|
|
fs.mkdirSync(argentHomeDir(), { recursive: true });
|
|
fs.writeFileSync(configFilePath(), "not json{", "utf8");
|
|
expect(readConfigObject()).toEqual({});
|
|
});
|
|
|
|
it("creates and persists a new key", () => {
|
|
updateConfig((c) => {
|
|
c.telemetry = { enabled: false };
|
|
});
|
|
expect(readConfigFile()).toEqual({ telemetry: { enabled: false } });
|
|
});
|
|
|
|
it("merges rather than clobbering keys it does not touch", () => {
|
|
updateConfig((c) => {
|
|
c.telemetry = { enabled: false };
|
|
});
|
|
updateConfig((c) => {
|
|
c.notices = { firstRun: true };
|
|
});
|
|
expect(readConfigFile()).toEqual({
|
|
telemetry: { enabled: false },
|
|
notices: { firstRun: true },
|
|
});
|
|
});
|
|
});
|
|
|
|
describe("remembered agent (lens config)", () => {
|
|
it("returns null when nothing is stored", () => {
|
|
expect(getRememberedAgent()).toBeNull();
|
|
});
|
|
|
|
it("persists and reads back the chosen agent", () => {
|
|
setRememberedAgent("claude");
|
|
expect(getRememberedAgent()).toBe("claude");
|
|
expect(readConfigFile()).toEqual({ lens: { agent: "claude" } });
|
|
});
|
|
|
|
it("overwrites a previously remembered agent", () => {
|
|
setRememberedAgent("claude");
|
|
setRememberedAgent("codex");
|
|
expect(getRememberedAgent()).toBe("codex");
|
|
});
|
|
|
|
it("treats a blank stored agent as none", () => {
|
|
updateConfig((c) => {
|
|
c.lens = { agent: " " };
|
|
});
|
|
expect(getRememberedAgent()).toBeNull();
|
|
});
|
|
|
|
it("clears the remembered agent without dropping sibling keys", () => {
|
|
updateConfig((c) => {
|
|
c.telemetry = { enabled: true };
|
|
});
|
|
setRememberedAgent("claude");
|
|
clearRememberedAgent();
|
|
expect(getRememberedAgent()).toBeNull();
|
|
// Siblings survive; the emptied `lens` container does not.
|
|
expect(readConfigFile()).toEqual({ telemetry: { enabled: true } });
|
|
});
|
|
|
|
it("clearing when nothing is stored is a no-op that doesn't throw", () => {
|
|
expect(() => clearRememberedAgent()).not.toThrow();
|
|
expect(getRememberedAgent()).toBeNull();
|
|
});
|
|
});
|