From 0c82933651ce52a7db20bf39f12ef6fd8ca8da63 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sat, 29 Aug 2026 07:35:56 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20don't=20split=20the=20ledger=20=E2=80=94?= =?UTF-8?q?=20external=20billing=20is=20opt-in;=20v0.72.0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #439 handed /timer and /billing to @profullstack/timer and @profullstack/billing the moment those packages appeared on PATH. That was wrong, and the pit's own test caught it: only half this layer has an outside home. /client, /rate, /payments and /team stay in moshcode — the rails and the permission model are ours, and /client's freeform dotted fields have no shape in the package's typed client model, so moving it would lose data rather than relocate it. Handing over the other half automatically splits one person's records across two stores: /client create "Acme Inc" → ~/.moshcode/business.json /rate set acme-inc $100/hour → ~/.moshcode/business.json /billing acme-inc → reads the package ledger: "no client acme-inc" That is not a missing feature, it is somebody's invoice failing to exist. And it only happens on a machine that installed the CLIs, so CI — which has not — stayed green while every developer box that took the install went red. That asymmetry is why it shipped. So the hand-over is now opt-in: MOSHCODE_EXTERNAL_BILLING=1, after `billing import` has moved the records across. The install tip only appears to somebody who has the package installed and has not switched it on, and it names the import, because switching over without it is the same split by hand. Two guards, both verified to fail when the old behaviour is put back: a unit test that installed is not the same as chosen, and a pit test that puts a fake `timer` on PATH and checks the pit still wrote its own ledger. The second is the one that would have caught this originally — the first pit test did, on a machine with the tools installed, which no CI run has. When the whole layer has an outside home, this becomes the default. Version bumped to 0.72.0 for the release that carries #439 and this fix. --- README.md | 26 ++++++++--- package.json | 2 +- src/business-delegate.mjs | 64 +++++++++++++++++---------- test/business-delegate.test.mjs | 78 +++++++++++++++++++++++++-------- test/business-pit.test.mjs | 36 ++++++++++++++- 5 files changed, 155 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 9124620..a1cd752 100644 --- a/README.md +++ b/README.md @@ -959,15 +959,27 @@ each useful on its own — the timer needs no client, the rate needs no gateway > moshcode install timer billing # or: npm install -g @profullstack/timer @profullstack/billing > ``` > -> With them installed, `/timer` and `/billing` hand the command straight to the -> CLI, the way `/gh` conducts `gh`. Without them, the in-process implementation -> below runs exactly as it always has, so upgrading changes nothing until you -> choose to install. `MOSHCODE_BUILTIN_BILLING=1` pins the built-in either way. +> Installing them changes nothing on its own. Switch the hand-over on when you +> are ready to move: +> +> ```sh +> billing import # look at what would come across +> billing import --apply # move it +> export MOSHCODE_EXTERNAL_BILLING=1 # /timer and /billing now run the CLIs +> ``` +> +> **It is opt-in for a reason.** Only half this layer has an outside home: +> `/client`, `/rate`, `/payments` and `/team` stay here, because the rails and +> the permission model are moshcode's and `/client`'s freeform dotted fields +> have no shape in the package's typed client model. Handing over the other +> half automatically would split your records across two stores — `/client` and +> `/rate` writing `~/.moshcode/business.json` while `/billing` reads the +> package's own ledger, so the invoice for a client you had just created would +> not exist. `billing import` is what closes that gap, which is why it comes +> first. > > The standalone billing carries the same rate model (`$100/hour/agent/upto:4`) -> and bills **agent-hours**, and `billing import` brings across a ledger that -> started in `~/.moshcode/`. `/client`, `/rate`, `/payments` and `/team` stay -> here: the rails and the permission model are moshcode integration. +> and bills **agent-hours**. ```sh moshcode timer on acme --task "batch payments" --agents auto # auto counts the herd diff --git a/package.json b/package.json index a320e20..03a4170 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "moshcode", - "version": "0.71.0", + "version": "0.72.0", "type": "module", "description": "moshcode — a metal wrapper for coding engines and native UGig/CoinPay workflow CLIs, with OpenPRD and moshscript", "repository": { diff --git a/src/business-delegate.mjs b/src/business-delegate.mjs index 13c63b4..7c90ff2 100644 --- a/src/business-delegate.mjs +++ b/src/business-delegate.mjs @@ -5,34 +5,49 @@ // neither is a moshcode idea: tracking time and sending an invoice are things // you want under any agentic CLI, and on Windows, where moshcode does not go. // -// So the rule here is: if the real CLI is installed, moshcode conducts it, the -// same way /gh conducts gh. The built-in implementation stays as the fallback -// for a machine that has not installed it yet, so nothing breaks on upgrade and -// nobody has to install anything to keep working. +// So the rule here is: when asked, moshcode conducts them the way /gh conducts +// gh. The two are NOT kept in sync, and that is the point of preferring the +// external one where it is wanted: a second copy of a billing model is a copy +// that drifts, and the published package is the one that gets the fixes. // -// The two are NOT kept in sync, and that is the point of preferring the -// external one: a second copy of a billing model is a copy that drifts, and the -// published package is the one that gets the fixes. +// --------------------------------------------------------------------------- +// Why this is opt-in rather than "delegate whenever the CLI is on PATH", which +// is what it did when it first landed: +// +// Only half the business layer has somewhere to go. /timer and /billing have +// standalone equivalents; /client, /rate, /payments and /team do not — the +// rails and the permission model are moshcode's, and /client's freeform dotted +// fields have no shape in the package's typed client model, so moving it would +// lose data rather than relocate it. +// +// Delegating that half by default splits one person's records across two +// stores. /client and /rate keep writing ~/.moshcode/business.json while +// /billing reads ~/.profullstack/billing/ledger.json, so: +// +// /client create "Acme Inc" → written to moshcode +// /rate set acme-inc $100/hour → written to moshcode +// /billing acme-inc → "no client acme-inc" +// +// That is not a missing feature, it is somebody's invoice failing to exist. And +// it only happens on a machine that installed the CLIs, so CI — which has not — +// stays green while every developer box that took the install goes red. +// +// Hence: opt in, knowing that `billing import` is how the existing ledger comes +// across. When the whole layer has an outside home, this becomes the default. import { isInstalled } from "./engines.mjs"; import { TOOLS, openTool } from "./tools.mjs"; /** Commands that have an external CLI, and the TOOLS key that owns it. */ export const DELEGATED = { timer: "timer", billing: "billing", invoice: "billing" }; -/** - * Force the in-process implementation. - * - * An escape hatch rather than a setting: somebody debugging a difference - * between the two needs to run the built-in one on a box where the CLI is - * installed, and that is the whole reason this exists. - */ -export function builtinForced() { - return /^(1|true|yes)$/i.test(String(process.env.MOSHCODE_BUILTIN_BILLING || "")); +/** Whether this machine has asked for the standalone CLIs to be used. */ +export function externalEnabled() { + return /^(1|true|yes)$/i.test(String(process.env.MOSHCODE_EXTERNAL_BILLING || "")); } -/** The external CLI for a command, if this machine has it. */ +/** The external CLI for a command, if it is enabled and this machine has it. */ export function externalFor(cmd) { - if (builtinForced()) return null; + if (!externalEnabled()) return null; const key = DELEGATED[String(cmd || "").toLowerCase()]; if (!key) return null; const tool = TOOLS[key]; @@ -75,11 +90,16 @@ export function exitCodeOf(result) { /** * The one-line nudge shown after the built-in runs. * - * Written to stderr, and only when the CLI is missing, so it never lands in the - * middle of `--json` output that something is parsing. + * Written to stderr, and only when the CLI is actually installed and simply not + * switched on, so it never lands in the middle of `--json` output and never + * advertises a tool that is not there. Someone who has installed the package is + * the only person for whom the variable is worth mentioning. */ export function installHint(cmd) { const key = DELEGATED[String(cmd || "").toLowerCase()]; - if (!key || externalFor(cmd) || builtinForced()) return null; - return `tip: moshcode install ${key} — runs @profullstack/${key}, which also works outside moshcode`; + if (!key || externalEnabled()) return null; + const tool = TOOLS[key]; + if (!tool || !isInstalled(tool.bin, tool.binDirs)) return null; + return `tip: @profullstack/${key} is installed — set MOSHCODE_EXTERNAL_BILLING=1 to use it` + + " (move your records first with: billing import)"; } diff --git a/test/business-delegate.test.mjs b/test/business-delegate.test.mjs index 14a54a3..215f90e 100644 --- a/test/business-delegate.test.mjs +++ b/test/business-delegate.test.mjs @@ -9,7 +9,7 @@ import os from "node:os"; import path from "node:path"; import test from "node:test"; -import { DELEGATED, builtinForced, exitCodeOf, externalFor, installHint } from "../src/business-delegate.mjs"; +import { DELEGATED, exitCodeOf, externalEnabled, externalFor, installHint } from "../src/business-delegate.mjs"; import { TOOLS } from "../src/tools.mjs"; /** A directory on PATH holding an executable of the given name. */ @@ -48,46 +48,86 @@ test("with the CLI absent, nothing is delegated and the built-in runs", (t) => { assert.equal(externalFor("billing"), null); }); -test("with the CLI on PATH, the command is handed to it", (t) => { - const saved = process.env.PATH; +test("an installed CLI is NOT used until it is switched on", (t) => { + // The bug this pins. Only half the business layer has an outside home: + // /client and /rate keep writing ~/.moshcode/business.json, so delegating + // /billing the moment the package appears on PATH splits one person's + // records across two stores and their invoice stops existing. It also only + // breaks on a machine that took the install, so CI stays green while every + // developer box that installed the tools goes red. + const savedPath = process.env.PATH; + const savedFlag = process.env.MOSHCODE_EXTERNAL_BILLING; const bin = fakeBin("timer"); process.env.PATH = bin.dir; - t.after(() => { process.env.PATH = saved; bin.cleanup(); }); - const found = externalFor("timer"); - assert.ok(found, "an installed timer should win"); - assert.equal(found.key, "timer"); + delete process.env.MOSHCODE_EXTERNAL_BILLING; + t.after(() => { + process.env.PATH = savedPath; + if (savedFlag === undefined) delete process.env.MOSHCODE_EXTERNAL_BILLING; + else process.env.MOSHCODE_EXTERNAL_BILLING = savedFlag; + bin.cleanup(); + }); + assert.equal(externalEnabled(), false); + assert.equal(externalFor("timer"), null, "installed is not the same as chosen"); }); -test("MOSHCODE_BUILTIN_BILLING pins the in-process implementation", (t) => { +test("MOSHCODE_EXTERNAL_BILLING switches the delegation on", (t) => { const savedPath = process.env.PATH; - const savedFlag = process.env.MOSHCODE_BUILTIN_BILLING; + const savedFlag = process.env.MOSHCODE_EXTERNAL_BILLING; const bin = fakeBin("timer"); process.env.PATH = bin.dir; - process.env.MOSHCODE_BUILTIN_BILLING = "1"; + process.env.MOSHCODE_EXTERNAL_BILLING = "1"; t.after(() => { process.env.PATH = savedPath; - if (savedFlag === undefined) delete process.env.MOSHCODE_BUILTIN_BILLING; - else process.env.MOSHCODE_BUILTIN_BILLING = savedFlag; + if (savedFlag === undefined) delete process.env.MOSHCODE_EXTERNAL_BILLING; + else process.env.MOSHCODE_EXTERNAL_BILLING = savedFlag; bin.cleanup(); }); - assert.equal(builtinForced(), true); - assert.equal(externalFor("timer"), null, "the escape hatch beats an installed CLI"); + assert.equal(externalEnabled(), true); + const found = externalFor("timer"); + assert.ok(found, "an installed timer should win once asked for"); + assert.equal(found.key, "timer"); }); -test("the install tip appears only when there is something to install", (t) => { - const saved = process.env.PATH; +test("switching it on cannot conjure a CLI that is not installed", (t) => { + const savedPath = process.env.PATH; + const savedFlag = process.env.MOSHCODE_EXTERNAL_BILLING; const empty = fs.mkdtempSync(path.join(os.tmpdir(), "moshcode-empty-")); process.env.PATH = empty; + process.env.MOSHCODE_EXTERNAL_BILLING = "1"; t.after(() => { - process.env.PATH = saved; + process.env.PATH = savedPath; + if (savedFlag === undefined) delete process.env.MOSHCODE_EXTERNAL_BILLING; + else process.env.MOSHCODE_EXTERNAL_BILLING = savedFlag; + fs.rmSync(empty, { recursive: true, force: true }); + }); + assert.equal(externalFor("timer"), null, "the built-in still has to run"); +}); + +test("the tip is shown only to somebody who could act on it", (t) => { + const savedPath = process.env.PATH; + const savedFlag = process.env.MOSHCODE_EXTERNAL_BILLING; + const empty = fs.mkdtempSync(path.join(os.tmpdir(), "moshcode-empty-")); + process.env.PATH = empty; + delete process.env.MOSHCODE_EXTERNAL_BILLING; + t.after(() => { + process.env.PATH = savedPath; + if (savedFlag === undefined) delete process.env.MOSHCODE_EXTERNAL_BILLING; + else process.env.MOSHCODE_EXTERNAL_BILLING = savedFlag; fs.rmSync(empty, { recursive: true, force: true }); }); - assert.match(installHint("timer"), /moshcode install timer/); + // Not installed: nothing to say. Advertising a variable that would do + // nothing is worse than silence. + assert.equal(installHint("timer"), null); const bin = fakeBin("timer"); process.env.PATH = bin.dir; t.after(bin.cleanup); - assert.equal(installHint("timer"), null, "no nagging once it is installed"); + const hint = installHint("timer"); + assert.match(hint, /MOSHCODE_EXTERNAL_BILLING/); + assert.match(hint, /billing import/, "it has to name how the records come across"); + + process.env.MOSHCODE_EXTERNAL_BILLING = "1"; + assert.equal(installHint("timer"), null, "no nagging once it is on"); }); test("a passthrough result becomes one exit code", () => { diff --git a/test/business-pit.test.mjs b/test/business-pit.test.mjs index 1e87bd2..afbc0d4 100644 --- a/test/business-pit.test.mjs +++ b/test/business-pit.test.mjs @@ -5,9 +5,9 @@ // been wrong in this codebase before for the same reason: a command that works // when imported is not yet a command anybody can type. import assert from "node:assert/strict"; -import { mkdtempSync, readFileSync } from "node:fs"; +import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { delimiter, join } from "node:path"; import test from "node:test"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; @@ -65,6 +65,38 @@ test("a client, a rate and a timer, typed at the prompt", async () => { assert.match(result.stdout, /timer off/); }); +test("an installed timer CLI does not quietly take the pit's records away", async () => { + // The regression: /timer and /billing gained standalone CLIs, and delegating + // to them the moment they appeared on PATH split one person's records in + // half. /client and /rate have no outside home, so they kept writing + // ~/.moshcode/business.json while /billing read the package's own ledger — + // and the invoice for a client you had just created did not exist. + // + // A fake `timer` on PATH is enough to reproduce it: the check is only + // "is this name executable". Without the opt-in the pit must ignore it. + const dir = mkdtempSync(join(tmpdir(), "moshcode-fake-bin-")); + const isWindows = process.platform === "win32"; + const shim = join(dir, isWindows ? "timer.cmd" : "timer"); + writeFileSync(shim, isWindows ? "@echo off\r\nexit /b 0\r\n" : "#!/bin/sh\nexit 0\n"); + if (!isWindows) chmodSync(shim, 0o755); + + try { + const result = await runTui( + ["/client create Acme", "/timer on acme --task shipping", "/timer off"], + { env: { PATH: `${dir}${delimiter}${process.env.PATH}` } }, + ); + assert.equal(result.status, 0); + // Had it delegated, the shim would have exited 0 having written nothing + // and this file would not exist. + const timers = timersOf(result.home); + assert.equal(timers.entries.length, 1, "the pit kept its own ledger"); + assert.equal(timers.entries[0].task, "shipping"); + assert.ok(businessOf(result.home).clients.acme, "and the client is in the same place"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + test("/business and /merchant are the same door as /client", async () => { const result = await runTui(["/business create Globex", "/merchant list"]); assert.ok(businessOf(result.home).clients.globex, "/business created it");