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");