diff --git a/CHANGELOG.md b/CHANGELOG.md index 32456dc..d12cd3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,23 @@ All notable changes to `@inceptionstack/roundhouse` are documented here. -## [0.5.40] — 2026-05-16 +## [0.5.40] — 2026-05-19 + +### Added +- **Optional quality extensions.** `pi-hard-no` (code quality inspector) and + `pi-branch-enforcer` are now **opt-in for new setups**. Existing setups that + already have them enabled are unaffected. +- **Toggle commands:** `/toggle-quality-inspector [on|off]` and + `/toggle-branch-enforcer [on|off]` let users enable/disable these extensions + from chat. No argument shows current state with inline keyboard buttons. + Idempotent setter semantics (safe for delivery dups). +- **Shared `pi-settings` module** (`src/pi-settings.ts`): centralises all + `~/.pi/agent/settings.json` read/write/update logic with atomic writes, + per-process queue + on-disk lockfile (`proper-lockfile`), and + `MalformedPiSettingsError` on parse failure. All 4 existing writers migrated. +- **Doctor check improvement:** `pi-settings` check now distinguishes + ENOENT (warn, “not found”) from SyntaxError (fail, “malformed JSON” + + parse error message). ### Changed - **Refactor:** Renamed `/stop` command to `/cancel` for semantic clarity. diff --git a/README.md b/README.md index e53060f..2286eef 100644 --- a/README.md +++ b/README.md @@ -222,6 +222,8 @@ Roundhouse automatically registers these commands with Telegram on startup: | `/doctor` | Run health checks and show system status | | `/crons` | Manage scheduled jobs (list, trigger, pause, resume) | | `/jobs` | List scheduled jobs (alias for /crons) | +| `/toggle-quality-inspector` | Toggle pi-hard-no code review on/off | +| `/toggle-branch-enforcer` | Toggle pi-branch-enforcer guard on/off | These appear in Telegram's `/` command menu automatically. @@ -254,6 +256,24 @@ When extensions (e.g. code review) queue follow-up work after the agent responds Fast operations that complete within 2 seconds show no extra messages. +### Optional extensions + +Roundhouse ships with two optional pi extensions that are **not enabled by default** for new setups: + +- **pi-hard-no** — Code quality inspector that reviews agent output +- **pi-branch-enforcer** — Guards against committing to protected branches + +These are installed globally by `/update` but must be explicitly enabled: + +| Command | Description | +|---------|-------------| +| `/toggle-quality-inspector [on\|off]` | Enable/disable pi-hard-no. No arg shows current state + buttons. | +| `/toggle-branch-enforcer [on\|off]` | Enable/disable pi-branch-enforcer. No arg shows current state + buttons. | + +After toggling, run `/restart` for the change to take effect (pi loads extensions at startup). + +**Existing setups** that already have these extensions enabled are unaffected — toggling off is opt-in. + ## File attachments Roundhouse handles voice messages, images, documents, and other file attachments from Telegram: diff --git a/package-lock.json b/package-lock.json index 20350a1..a3eff65 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@inceptionstack/roundhouse", - "version": "0.5.33", + "version": "0.5.38", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@inceptionstack/roundhouse", - "version": "0.5.33", + "version": "0.5.38", "license": "MIT", "dependencies": { "@chat-adapter/state-memory": "^4.26.0", @@ -15,6 +15,7 @@ "chat": "^4.26.0", "croner": "^10.0.1", "p-queue": "^9.2.0", + "proper-lockfile": "^4.1.2", "qrcode-terminal": "^0.12.0", "tsx": "^4.0.0" }, diff --git a/package.json b/package.json index cac4896..6ae6742 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@inceptionstack/roundhouse", - "version": "0.5.38", + "version": "0.5.40", "type": "module", "description": "Multi-platform chat gateway that routes messages through a configured AI agent", "license": "MIT", @@ -45,6 +45,7 @@ "chat": "^4.26.0", "croner": "^10.0.1", "p-queue": "^9.2.0", + "proper-lockfile": "^4.1.2", "qrcode-terminal": "^0.12.0", "tsx": "^4.0.0" }, diff --git a/src/cli/doctor/checks/agent.ts b/src/cli/doctor/checks/agent.ts index f285d53..a7f06aa 100644 --- a/src/cli/doctor/checks/agent.ts +++ b/src/cli/doctor/checks/agent.ts @@ -53,26 +53,43 @@ export const agentChecks: DoctorCheck[] = [ id: "pi-settings", category: "agent", name: "Pi settings", async run() { const settingsPath = join(homedir(), ".pi", "agent", "settings.json"); + let raw: string; try { - const raw = await readFile(settingsPath, "utf8"); - const settings = JSON.parse(raw); - const model = settings.defaultModel ? `${settings.defaultProvider}/${settings.defaultModel}` : "not configured"; - const issues: string[] = []; - if (!settings.defaultProvider) issues.push("No defaultProvider set"); - if (!settings.defaultModel) issues.push("No defaultModel set"); + raw = await readFile(settingsPath, "utf8"); + } catch (err: any) { + if (err.code === "ENOENT") { + return { + id: "pi-settings", category: "agent", name: "Pi settings", + status: "warn" as const, summary: "not found", + details: [`${settingsPath} does not exist`], + }; + } return { id: "pi-settings", category: "agent", name: "Pi settings", - status: issues.length ? "warn" : "pass", - summary: issues.length ? `${issues.length} issue(s)` : `model: ${model}`, - details: issues.length ? issues : undefined, + status: "fail" as const, summary: `read error: ${err.message}`, + details: [`Failed to read ${settingsPath}: ${err.message}`], }; - } catch { + } + let settings: any; + try { + settings = JSON.parse(raw); + } catch (parseErr: any) { return { id: "pi-settings", category: "agent", name: "Pi settings", - status: "warn", summary: "not found", - details: [`${settingsPath} does not exist`], + status: "fail" as const, summary: "malformed JSON", + details: [`${settingsPath}: ${parseErr.message}`], }; } + const model = settings.defaultModel ? `${settings.defaultProvider}/${settings.defaultModel}` : "not configured"; + const issues: string[] = []; + if (!settings.defaultProvider) issues.push("No defaultProvider set"); + if (!settings.defaultModel) issues.push("No defaultModel set"); + return { + id: "pi-settings", category: "agent", name: "Pi settings", + status: issues.length ? "warn" : "pass", + summary: issues.length ? `${issues.length} issue(s)` : `model: ${model}`, + details: issues.length ? issues : undefined, + }; }, }, ]; diff --git a/src/cli/setup/runtime.ts b/src/cli/setup/runtime.ts index 8ee2714..1091316 100644 --- a/src/cli/setup/runtime.ts +++ b/src/cli/setup/runtime.ts @@ -1,7 +1,7 @@ -import { dirname } from "node:path"; -import { readFile, mkdir } from "node:fs/promises"; -import { atomicWriteJson, execOrFail } from "./helpers"; +import { readFile } from "node:fs/promises"; +import { execOrFail } from "./helpers"; import { type SetupOptions, type StepLog, PI_SETTINGS_PATH } from "./types"; +import { updatePiSettings } from "../../pi-settings"; import { getAgentDefinition, type AgentDefinition, @@ -19,47 +19,49 @@ export function resolveAgentForSetup(opts: SetupOptions, logger: StepLog): Agent existing = JSON.parse(await readFile(PI_SETTINGS_PATH, "utf8")); } catch {} - const settings: Record = { ...existing }; + await updatePiSettings((settings) => { + // Merge with what we read above (preserves any concurrent writes + // since our read — the lock serialises the final RMW). + const merged = { ...settings }; - if (ctx.force) { - settings.defaultProvider = ctx.provider; - settings.defaultModel = ctx.model; - } else { - if (existing.defaultProvider && existing.defaultProvider !== ctx.provider) { - logger.warn(`Pi provider already set to '${existing.defaultProvider}' (keeping, use --force to override)`); + if (ctx.force) { + merged.defaultProvider = ctx.provider; + merged.defaultModel = ctx.model; } else { - settings.defaultProvider = ctx.provider; + if (existing.defaultProvider && existing.defaultProvider !== ctx.provider) { + logger.warn(`Pi provider already set to '${existing.defaultProvider}' (keeping, use --force to override)`); + } else { + merged.defaultProvider = ctx.provider; + } + if (existing.defaultModel && existing.defaultModel !== ctx.model) { + logger.warn(`Pi model already set to '${existing.defaultModel}' (keeping, use --force to override)`); + } else { + merged.defaultModel = ctx.model; + } } - if (existing.defaultModel && existing.defaultModel !== ctx.model) { - logger.warn(`Pi model already set to '${existing.defaultModel}' (keeping, use --force to override)`); - } else { - settings.defaultModel = ctx.model; - } - } - if (!Array.isArray(settings.packages)) settings.packages = []; + if (!Array.isArray(merged.packages)) merged.packages = []; - const pkgs = settings.packages as string[]; - const selfPkg = "npm:@inceptionstack/roundhouse"; - const selfIdx = pkgs.indexOf(selfPkg); - if (selfIdx !== -1) pkgs.splice(selfIdx, 1); + const pkgs = merged.packages as string[]; + const selfPkg = "npm:@inceptionstack/roundhouse"; + const selfIdx = pkgs.indexOf(selfPkg); + if (selfIdx !== -1) pkgs.splice(selfIdx, 1); - const coreExtensions = [ - "npm:@inceptionstack/pi-hard-no", - "npm:@inceptionstack/pi-branch-enforcer", - ]; - for (const ext of coreExtensions) { - if (!pkgs.includes(ext)) pkgs.push(ext); - } + // coreExtensions is empty — pi-hard-no and pi-branch-enforcer are now opt-in + const coreExtensions: string[] = []; + for (const ext of coreExtensions) { + if (!pkgs.includes(ext)) pkgs.push(ext); + } + + if (ctx.psst) { + const psstPkg = "npm:@miclivs/pi-psst"; + if (!pkgs.includes(psstPkg)) pkgs.push(psstPkg); + } - if (ctx.psst) { - const psstPkg = "npm:@miclivs/pi-psst"; - if (!pkgs.includes(psstPkg)) pkgs.push(psstPkg); - } + return merged; + }); - await mkdir(dirname(PI_SETTINGS_PATH), { recursive: true }); - await atomicWriteJson(PI_SETTINGS_PATH, settings); - logger.ok(`~/.pi/agent/settings.json (${settings.defaultProvider}, ${settings.defaultModel})`); + logger.ok(`~/.pi/agent/settings.json (${existing.defaultProvider ?? ctx.provider}, ${existing.defaultModel ?? ctx.model})`); }; agent.installExtension = async (ext: string) => { diff --git a/src/cli/update.ts b/src/cli/update.ts index 610df29..ac4fa1b 100644 --- a/src/cli/update.ts +++ b/src/cli/update.ts @@ -7,8 +7,8 @@ import { homedir } from "node:os"; import { execSync } from "node:child_process"; -import { readFileSync, writeFileSync } from "node:fs"; import { provisionBundle } from "../provisioning/bundle"; +import { updatePiSettings } from "../pi-settings"; const GLOBAL_PI_EXTENSION_PACKAGES = [ "@inceptionstack/pi-hard-no", @@ -117,16 +117,18 @@ export async function updateSelf( } } -export function patchPiSettings(): void { +export async function patchPiSettings(): Promise { try { - const settingsPath = `${homedir()}/.pi/agent/settings.json`; - const settings = JSON.parse(readFileSync(settingsPath, "utf8")); - const selfPkg = "npm:@inceptionstack/roundhouse"; - if (!Array.isArray(settings.packages)) settings.packages = []; - if (!settings.packages.includes(selfPkg)) { - settings.packages.push(selfPkg); - writeFileSync(settingsPath, JSON.stringify(settings, null, 2) + "\n"); - } + await updatePiSettings((settings) => { + const selfPkg = "npm:@inceptionstack/roundhouse"; + if (!Array.isArray(settings.packages)) settings = { ...settings, packages: [] }; + const pkgs = [...(settings.packages as string[])]; + if (!pkgs.includes(selfPkg)) { + pkgs.push(selfPkg); + return { ...settings, packages: pkgs }; + } + return settings; + }); } catch { /* settings.json may not exist yet — fine, setup will create it */ } } @@ -173,7 +175,7 @@ export async function performUpdate(progress: UpdateProgress): Promise { + const desired = parseDesiredState(ctx.text); + + try { + // No arg → show current state + ON/OFF inline keyboard + if (desired === null) { + const enabled = await isPiPackageEnabled(pkg); + return buildSelectableMenu({ + current: enabled ? "on" : "off", + options: [ + { key: "on", label: "ON" }, + { key: "off", label: "OFF" }, + ], + actionId, + textHeader: `${enabled ? "✅" : "🚫"} *${label}:* ${enabled ? "ON" : "OFF"}`, + textHint: `_Usage:_ \`/toggle-${actionId === EXT_TOGGLE_QI_ACTION_ID ? "quality-inspector" : "branch-enforcer"} on|off\``, + columns: 2, + }); + } + + // Explicit setter — idempotent + const { changed } = desired === "on" + ? await enablePiPackage(pkg) + : await disablePiPackage(pkg); + + const icon = desired === "on" ? "✅" : "🚫"; + const state = desired === "on" ? "ON" : "OFF"; + const note = changed ? "\n\nRestart pi to apply: /restart" : "\n\n(no change)"; + return { text: `${icon} ${label}: ${state}${note}` }; + } catch (err) { + if (err instanceof MalformedPiSettingsError) { + return { + text: `⚠️ ~/.pi/agent/settings.json is malformed. Refusing to write.\nRun /doctor to inspect.\n\n\`${err.message}\``, + }; + } + throw err; + } +} + +export async function handleToggleQualityInspector(ctx: ExtToggleContext): Promise { + return handleExtensionToggle(QI_PACKAGE, "Quality inspector (pi-hard-no)", EXT_TOGGLE_QI_ACTION_ID, ctx); +} + +export async function handleToggleBranchEnforcer(ctx: ExtToggleContext): Promise { + return handleExtensionToggle(BE_PACKAGE, "Branch enforcer (pi-branch-enforcer)", EXT_TOGGLE_BE_ACTION_ID, ctx); +} + +/** + * Inline-keyboard click handler for quality inspector. + * Button value is just "on" or "off" (≤3 bytes, well under Telegram's 64-byte limit). + */ +export async function handleExtToggleQiAction(event: { value?: string }): Promise { + const state = event.value; + if (state !== "on" && state !== "off") return; + try { + const { changed } = state === "on" + ? await enablePiPackage(QI_PACKAGE) + : await disablePiPackage(QI_PACKAGE); + const icon = state === "on" ? "✅" : "🚫"; + const label = "Quality inspector (pi-hard-no)"; + const note = changed ? "\n\nRestart pi to apply: /restart" : "\n\n(no change)"; + return { text: `${icon} ${label}: ${state.toUpperCase()}${note}` }; + } catch (err) { + if (err instanceof MalformedPiSettingsError) { + return { text: `⚠️ settings.json malformed. Run /doctor.` }; + } + throw err; + } +} + +/** + * Inline-keyboard click handler for branch enforcer. + */ +export async function handleExtToggleBeAction(event: { value?: string }): Promise { + const state = event.value; + if (state !== "on" && state !== "off") return; + try { + const { changed } = state === "on" + ? await enablePiPackage(BE_PACKAGE) + : await disablePiPackage(BE_PACKAGE); + const icon = state === "on" ? "✅" : "🚫"; + const label = "Branch enforcer (pi-branch-enforcer)"; + const note = changed ? "\n\nRestart pi to apply: /restart" : "\n\n(no change)"; + return { text: `${icon} ${label}: ${state.toUpperCase()}${note}` }; + } catch (err) { + if (err instanceof MalformedPiSettingsError) { + return { text: `⚠️ settings.json malformed. Run /doctor.` }; + } + throw err; + } +} diff --git a/src/gateway/gateway.ts b/src/gateway/gateway.ts index 587d405..9975bf7 100644 --- a/src/gateway/gateway.ts +++ b/src/gateway/gateway.ts @@ -26,6 +26,14 @@ import { handleNew, handleRestart, handleUpdate, handleCompact, handleStatus, ha import { handleModel, handleModelAction, MODEL_ACTION_ID } from "./model-command"; import { handleLater } from "./later-command"; import { handleTopic, handleTopicAction, TOPIC_ACTION_ID, applyTopicOverride } from "./topic-command"; +import { + handleToggleQualityInspector, + handleToggleBranchEnforcer, + handleExtToggleQiAction, + handleExtToggleBeAction, + EXT_TOGGLE_QI_ACTION_ID, + EXT_TOGGLE_BE_ACTION_ID, +} from "./extension-toggle-command"; import { type CommandDescriptor, type CommandInvocation, @@ -846,6 +854,24 @@ export class Gateway { progress: (t, initialText) => this.transport.progress(t, initialText), }), }, + { + triggers: ["/toggle-quality-inspector"], + stage: "pre-turn", + acceptsArgs: true, + invoke: ({ text }) => handleToggleQualityInspector({ text }), + actions: { + [EXT_TOGGLE_QI_ACTION_ID]: (ev) => handleExtToggleQiAction({ value: ev.value }), + }, + }, + { + triggers: ["/toggle-branch-enforcer"], + stage: "pre-turn", + acceptsArgs: true, + invoke: ({ text }) => handleToggleBranchEnforcer({ text }), + actions: { + [EXT_TOGGLE_BE_ACTION_ID]: (ev) => handleExtToggleBeAction({ value: ev.value }), + }, + }, ]; } diff --git a/src/gateway/model-command.ts b/src/gateway/model-command.ts index 409dff5..c8c11cc 100644 --- a/src/gateway/model-command.ts +++ b/src/gateway/model-command.ts @@ -16,9 +16,10 @@ import { homedir } from "node:os"; import { join } from "node:path"; -import { readFileSync, writeFileSync, mkdirSync } from "node:fs"; +import { readFileSync } from "node:fs"; import type { RichResponse } from "../transports"; import { buildSelectableMenu } from "../transports"; +import { updatePiSettings } from "../pi-settings"; /** Known model aliases \u2192 Bedrock model IDs */ export const MODEL_ALIASES: Record = { @@ -60,11 +61,6 @@ function readSettings(): Record { } } -function writeSettings(settings: Record): void { - mkdirSync(join(homedir(), ".pi", "agent"), { recursive: true }); - writeFileSync(SETTINGS_PATH, JSON.stringify(settings, null, 2) + "\n"); -} - function getCurrentModelLabel(settings: Record): string { const provider = settings.defaultProvider ?? "unknown"; const model = settings.defaultModel ?? "unknown"; @@ -112,36 +108,38 @@ export async function handleModel(ctx: ModelCommandContext): Promise` and the menu callback). * Returns a RichResponse with confirmation text. */ -export function applyModelSelection( +export async function applyModelSelection( target: string, - settings: Record | null, -): RichResponse { - if (!settings) settings = readSettings(); - +): Promise { const resolved = MODEL_ALIASES[target]; if (!resolved) { if (target.includes(".") || target.includes("/")) { // Treat as a raw provider/model id passthrough. + const settings = readSettings(); const provider = settings.defaultProvider ?? "amazon-bedrock"; - settings.defaultModel = target; - settings.defaultProvider = provider; - writeSettings(settings); + await updatePiSettings((s) => ({ + ...s, + defaultProvider: provider, + defaultModel: target, + })); return { text: `\u2705 Model set to: \`${provider}/${target}\`` }; } const aliases = Object.keys(MODEL_ALIASES).join(", "); return { text: `\u274c Unknown model: \`${target}\`\n\nAvailable: ${aliases}` }; } - settings.defaultProvider = resolved.provider; - settings.defaultModel = resolved.model; - writeSettings(settings); + await updatePiSettings((s) => ({ + ...s, + defaultProvider: resolved.provider, + defaultModel: resolved.model, + })); console.log(`[roundhouse] /model: switched to ${resolved.provider}/${resolved.model}`); return { text: `\u2705 Switched to *${resolved.label}*` }; @@ -151,8 +149,8 @@ export function applyModelSelection( * Handle inline-keyboard callback for model selection. * Wired from the descriptor's `actions[MODEL_ACTION_ID]`. */ -export function handleModelAction(event: { value?: string }): RichResponse | void { +export async function handleModelAction(event: { value?: string }): Promise { const alias = event.value; if (!alias || !MODEL_ALIASES[alias]) return; - return applyModelSelection(alias, null); + return applyModelSelection(alias); } diff --git a/src/pi-settings.ts b/src/pi-settings.ts new file mode 100644 index 0000000..20a532a --- /dev/null +++ b/src/pi-settings.ts @@ -0,0 +1,212 @@ +/** + * pi-settings.ts — Shared read/write/update primitives for ~/.pi/agent/settings.json + * + * Single source of truth for all settings.json mutations in roundhouse. + * Provides: + * - Atomic writes (tmp + rename) + * - Serialised read-modify-write via per-process queue + on-disk lockfile + * - Idempotent enable/disable helpers for the `packages[]` array + * + * All functions throw MalformedPiSettingsError on parse failure — callers + * decide how to surface it (never silent rebuild). + */ + +import { homedir } from "node:os"; +import { resolve, dirname } from "node:path"; +import { readFile, writeFile, rename, unlink, mkdir } from "node:fs/promises"; +import { randomBytes } from "node:crypto"; +import lockfile from "proper-lockfile"; + +export const PI_SETTINGS_PATH = resolve(homedir(), ".pi", "agent", "settings.json"); +const LOCK_PATH = resolve(homedir(), ".pi", "agent", ".settings.lock"); + +export interface PiSettings { + defaultProvider?: string; + defaultModel?: string; + packages?: string[]; + [k: string]: unknown; +} + +/** + * Typed error for malformed settings.json (bad JSON, non-string packages, etc.). + * Callers catch this at the boundary and decide how to respond. + */ +export class MalformedPiSettingsError extends Error { + public readonly path: string; + constructor(message: string, path: string = PI_SETTINGS_PATH) { + super(message); + this.name = "MalformedPiSettingsError"; + this.path = path; + } +} + +/** + * Read ~/.pi/agent/settings.json. + * Returns `{}` if the file doesn't exist (ENOENT). + * Throws MalformedPiSettingsError on invalid JSON. + */ +export async function readPiSettings(): Promise { + let raw: string; + try { + raw = await readFile(PI_SETTINGS_PATH, "utf8"); + } catch (err: any) { + if (err.code === "ENOENT") return {}; + throw err; + } + try { + const parsed = JSON.parse(raw); + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + throw new MalformedPiSettingsError( + `settings.json is not a JSON object (got ${Array.isArray(parsed) ? "array" : typeof parsed})`, + ); + } + // Validate packages[] if present + if (parsed.packages !== undefined) { + if (!Array.isArray(parsed.packages)) { + throw new MalformedPiSettingsError( + `settings.json "packages" field is not an array (got ${typeof parsed.packages})`, + ); + } + for (const entry of parsed.packages) { + if (typeof entry !== "string") { + throw new MalformedPiSettingsError( + `settings.json "packages" contains non-string entry: ${JSON.stringify(entry)}`, + ); + } + } + } + return parsed as PiSettings; + } catch (err) { + if (err instanceof MalformedPiSettingsError) throw err; + throw new MalformedPiSettingsError( + `Failed to parse settings.json: ${err instanceof Error ? err.message : String(err)}`, + ); + } +} + +/** + * Atomic write of settings.json (tmp + rename). + * Deduplicates packages[] on every write. + * Creates parent directory if needed. + */ +export async function writePiSettings(settings: PiSettings): Promise { + // Deduplicate packages + if (Array.isArray(settings.packages)) { + settings = { ...settings, packages: [...new Set(settings.packages)] }; + } + + const dir = dirname(PI_SETTINGS_PATH); + await mkdir(dir, { recursive: true }); + + const tmp = `${PI_SETTINGS_PATH}.tmp.${randomBytes(4).toString("hex")}`; + try { + await writeFile(tmp, JSON.stringify(settings, null, 2) + "\n", { mode: 0o600 }); + await rename(tmp, PI_SETTINGS_PATH); + } catch (err) { + try { await unlink(tmp); } catch {} + throw err; + } +} + +// ── Per-process serialisation queue ── +// Ensures only one RMW cycle is in-flight at a time within this process. +let _queue: Promise = Promise.resolve(); + +function enqueue(fn: () => Promise): Promise { + const next = _queue.then(fn, fn); + _queue = next.catch(() => {}); // swallow so chain doesn't break + return next; +} + +/** + * Read-modify-write under per-process queue + on-disk lockfile. + * Serialises across all writers (toggle, /model, /update, provisioning). + * + * Throws MalformedPiSettingsError if the file can't be parsed — + * the mutator is never called in that case. + */ +export async function updatePiSettings( + mutator: (s: PiSettings) => PiSettings | Promise, +): Promise { + return enqueue(async () => { + const dir = dirname(PI_SETTINGS_PATH); + await mkdir(dir, { recursive: true }); + + // Ensure lock directory exists (proper-lockfile needs the target to exist) + // We lock a separate sentinel file so we don't conflict with the main file's tmp/rename + const lockDir = dirname(LOCK_PATH); + await mkdir(lockDir, { recursive: true }); + // Ensure the lock target file exists + try { + await writeFile(LOCK_PATH, "", { flag: "wx" }); + } catch (err: any) { + if (err.code !== "EEXIST") throw err; + } + + let release: (() => Promise) | undefined; + try { + release = await lockfile.lock(LOCK_PATH, { + retries: { retries: 5, minTimeout: 50, maxTimeout: 500 }, + stale: 10_000, + }); + + const current = await readPiSettings(); + const updated = await mutator(current); + await writePiSettings(updated); + return updated; + } finally { + if (release) { + try { await release(); } catch {} + } + } + }); +} + +/** + * Idempotent: ensure `pkg` is present in packages[]. + * Creates the file if missing. Throws MalformedPiSettingsError on bad JSON. + */ +export async function enablePiPackage(pkg: string): Promise<{ changed: boolean }> { + let changed = false; + await updatePiSettings((s) => { + const pkgs = Array.isArray(s.packages) ? [...s.packages] : []; + if (pkgs.includes(pkg)) { + changed = false; + return s; + } + changed = true; + return { ...s, packages: [...pkgs, pkg] }; + }); + return { changed }; +} + +/** + * Idempotent: ensure `pkg` is absent from packages[]. + * If the file doesn't exist, returns { changed: false } (nothing to remove). + * Throws MalformedPiSettingsError on bad JSON. + */ +export async function disablePiPackage(pkg: string): Promise<{ changed: boolean }> { + let changed = false; + await updatePiSettings((s) => { + const pkgs = Array.isArray(s.packages) ? [...s.packages] : []; + const idx = pkgs.indexOf(pkg); + if (idx === -1) { + changed = false; + return s; + } + changed = true; + const next = [...pkgs]; + next.splice(idx, 1); + return { ...s, packages: next }; + }); + return { changed }; +} + +/** + * Check if a package is currently enabled in settings.json. + * Returns false if file missing. Throws MalformedPiSettingsError on bad JSON. + */ +export async function isPiPackageEnabled(pkg: string): Promise { + const settings = await readPiSettings(); + return Array.isArray(settings.packages) && settings.packages.includes(pkg); +} diff --git a/src/provisioning/bundle.ts b/src/provisioning/bundle.ts index 411c7d5..f92d73d 100644 --- a/src/provisioning/bundle.ts +++ b/src/provisioning/bundle.ts @@ -270,15 +270,16 @@ export function provisionExtensionFiles(opts: ProvisionOpts = {}): void { /** * Ensure core extensions are listed in ~/.pi/agent/settings.json packages array. + * NOTE: pi-hard-no and pi-branch-enforcer are now opt-in — not auto-added. + * This function preserves any existing packages but no longer force-adds the + * quality extensions on new setups. */ export function provisionExtensions(opts: ProvisionOpts = {}): void { const { log = consoleLog } = opts; const settingsPath = resolve(homedir(), ".pi", "agent", "settings.json"); - const coreExtensions = [ - "npm:@inceptionstack/pi-hard-no", - "npm:@inceptionstack/pi-branch-enforcer", - ]; + // coreExtensions is now empty — pi-hard-no and pi-branch-enforcer are opt-in + const coreExtensions: string[] = []; try { let settings: Record = {}; diff --git a/src/transports/telegram/bot-commands.ts b/src/transports/telegram/bot-commands.ts index 9cdb69c..db8f397 100644 --- a/src/transports/telegram/bot-commands.ts +++ b/src/transports/telegram/bot-commands.ts @@ -24,4 +24,6 @@ export const BOT_COMMANDS: BotCommand[] = [ { command: "doctor", description: "Run diagnostics" }, { command: "crons", description: "List scheduled cron jobs" }, { command: "jobs", description: "Show running jobs" }, + { command: "toggle-quality-inspector", description: "Toggle pi-hard-no code review" }, + { command: "toggle-branch-enforcer", description: "Toggle pi-branch-enforcer guard" }, ]; diff --git a/test/bundle.test.ts b/test/bundle.test.ts index 4e1cb7b..16e20e3 100644 --- a/test/bundle.test.ts +++ b/test/bundle.test.ts @@ -9,6 +9,7 @@ import { provisionUvx, provisionMcporterConfig, provisionBundle, + provisionExtensions, SKILLS_DIR, SKILLS_REPO, type ProvisionLog, @@ -122,4 +123,61 @@ describe("bundle", () => { expect(alreadyInstalled.length).toBeGreaterThanOrEqual(3); }); }); + + describe("provisionExtensions", () => { + const settingsDir = resolve(homedir(), ".pi", "agent"); + const settingsPath = resolve(settingsDir, "settings.json"); + let originalContent: string | null = null; + + beforeEach(() => { + try { + const { readFileSync } = require("node:fs"); + originalContent = readFileSync(settingsPath, "utf8"); + } catch { + originalContent = null; + } + }); + + afterEach(() => { + if (originalContent !== null) { + writeFileSync(settingsPath, originalContent); + } else { + try { rmSync(settingsPath); } catch {} + } + }); + + it("does NOT auto-add pi-hard-no or pi-branch-enforcer to fresh settings", () => { + // Write a minimal settings.json with no packages + mkdirSync(settingsDir, { recursive: true }); + writeFileSync(settingsPath, JSON.stringify({ defaultProvider: "test" }, null, 2)); + + const log = createMockLog(); + provisionExtensions({ log }); + + const { readFileSync } = require("node:fs"); + const result = JSON.parse(readFileSync(settingsPath, "utf8")); + const pkgs = result.packages ?? []; + expect(pkgs).not.toContain("npm:@inceptionstack/pi-hard-no"); + expect(pkgs).not.toContain("npm:@inceptionstack/pi-branch-enforcer"); + }); + + it("does NOT strip pi-hard-no or pi-branch-enforcer from existing settings", () => { + // Write settings.json WITH both extensions already present + mkdirSync(settingsDir, { recursive: true }); + const existing = { + defaultProvider: "test", + packages: ["npm:@inceptionstack/pi-hard-no", "npm:@inceptionstack/pi-branch-enforcer", "npm:other"], + }; + writeFileSync(settingsPath, JSON.stringify(existing, null, 2)); + + const log = createMockLog(); + provisionExtensions({ log }); + + const { readFileSync } = require("node:fs"); + const result = JSON.parse(readFileSync(settingsPath, "utf8")); + expect(result.packages).toContain("npm:@inceptionstack/pi-hard-no"); + expect(result.packages).toContain("npm:@inceptionstack/pi-branch-enforcer"); + expect(result.packages).toContain("npm:other"); + }); + }); }); diff --git a/test/extension-toggle.test.ts b/test/extension-toggle.test.ts new file mode 100644 index 0000000..7f7cba6 --- /dev/null +++ b/test/extension-toggle.test.ts @@ -0,0 +1,152 @@ +/** + * test/extension-toggle.test.ts — Tests for /toggle-quality-inspector and /toggle-branch-enforcer + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { resolve } from "node:path"; +import { mkdirSync, writeFileSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { randomBytes } from "node:crypto"; + +const TEST_DIR = resolve(tmpdir(), `ext-toggle-test-${randomBytes(4).toString("hex")}`); + +// Mock homedir before any imports that use it +vi.mock("node:os", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + homedir: () => resolve(TEST_DIR, "fakehome"), + }; +}); + +function getSettingsPath(): string { + return resolve(TEST_DIR, "fakehome", ".pi", "agent", "settings.json"); +} + +function ensureSettingsDir(): void { + mkdirSync(resolve(TEST_DIR, "fakehome", ".pi", "agent"), { recursive: true }); +} + +describe("extension-toggle-command", () => { + beforeEach(() => { + ensureSettingsDir(); + }); + + afterEach(() => { + try { rmSync(TEST_DIR, { recursive: true, force: true }); } catch {} + }); + + describe("handleToggleQualityInspector", () => { + it("'on' enables pi-hard-no and returns confirmation", async () => { + const { handleToggleQualityInspector } = await import("../src/gateway/extension-toggle-command"); + writeFileSync(getSettingsPath(), JSON.stringify({ packages: [] })); + const result = await handleToggleQualityInspector({ text: "/toggle-quality-inspector on" }); + expect(result.text).toContain("ON"); + expect(result.text).toContain("✅"); + expect(result.text).toContain("/restart"); + const content = JSON.parse(readFileSync(getSettingsPath(), "utf8")); + expect(content.packages).toContain("npm:@inceptionstack/pi-hard-no"); + }); + + it("'off' disables pi-hard-no and returns confirmation", async () => { + const { handleToggleQualityInspector } = await import("../src/gateway/extension-toggle-command"); + writeFileSync(getSettingsPath(), JSON.stringify({ packages: ["npm:@inceptionstack/pi-hard-no"] })); + const result = await handleToggleQualityInspector({ text: "/toggle-quality-inspector off" }); + expect(result.text).toContain("OFF"); + expect(result.text).toContain("🚫"); + expect(result.text).toContain("/restart"); + const content = JSON.parse(readFileSync(getSettingsPath(), "utf8")); + expect(content.packages).not.toContain("npm:@inceptionstack/pi-hard-no"); + }); + + it("no arg returns menu with current state (RichResponse with menu)", async () => { + const { handleToggleQualityInspector } = await import("../src/gateway/extension-toggle-command"); + writeFileSync(getSettingsPath(), JSON.stringify({ packages: ["npm:@inceptionstack/pi-hard-no"] })); + const result = await handleToggleQualityInspector({ text: "/toggle-quality-inspector" }); + expect(result.menu).toBeDefined(); + expect(result.text).toContain("ON"); + // The ON button should be selected + const onBtn = result.menu!.sections[0].buttons.find((b: any) => b.value === "on"); + expect(onBtn?.selected).toBe(true); + }); + + it("idempotent: second 'on' returns (no change)", async () => { + const { handleToggleQualityInspector } = await import("../src/gateway/extension-toggle-command"); + writeFileSync(getSettingsPath(), JSON.stringify({ packages: ["npm:@inceptionstack/pi-hard-no"] })); + const result = await handleToggleQualityInspector({ text: "/toggle-quality-inspector on" }); + expect(result.text).toContain("(no change)"); + expect(result.text).not.toContain("/restart"); + }); + }); + + describe("handleExtToggleQiAction", () => { + it("action handler with value='on' enables package", async () => { + const { handleExtToggleQiAction } = await import("../src/gateway/extension-toggle-command"); + writeFileSync(getSettingsPath(), JSON.stringify({ packages: [] })); + const result = await handleExtToggleQiAction({ value: "on" }); + expect(result).toBeDefined(); + expect(result!.text).toContain("ON"); + const content = JSON.parse(readFileSync(getSettingsPath(), "utf8")); + expect(content.packages).toContain("npm:@inceptionstack/pi-hard-no"); + }); + + it("action handler with value='off' disables package", async () => { + const { handleExtToggleQiAction } = await import("../src/gateway/extension-toggle-command"); + writeFileSync(getSettingsPath(), JSON.stringify({ packages: ["npm:@inceptionstack/pi-hard-no"] })); + const result = await handleExtToggleQiAction({ value: "off" }); + expect(result).toBeDefined(); + expect(result!.text).toContain("OFF"); + }); + }); + + describe("live dispatch (regression)", () => { + it("/toggle-quality-inspector reaches transport.postRich with the original transport thread", async () => { + const { Gateway } = await import("../src/gateway/gateway"); + const { isCommand, isCommandWithArgs } = await import("../src/gateway/helpers"); + + const router = { + resolve: () => ({ name: "noop" } as any), + dispose: async () => {}, + }; + const config = { + agent: { type: "noop" }, + chat: { botUsername: "test", adapters: {} }, + } as any; + const postRich = vi.fn(async () => {}); + const transport = { postRich, progress: vi.fn() }; + const gw = new Gateway(router, config); + (gw as any).transport = transport; + + // Write settings so the command handler can read them + writeFileSync(getSettingsPath(), JSON.stringify({ packages: [] })); + + const internals = gw as any; + const all = internals.buildCommandDescriptors({ + allowedUsers: [], allowedUserIds: [], verboseThreads: new Set(), + threadLocks: new Map(), abortControllers: new Map(), + }); + + // Pre-turn commands — dispatchInTurnCommand works for any descriptor list + const preTurn = all.filter((d: any) => d.stage === "pre-turn"); + const matchers = { + isCommand: (t: string, c: string) => isCommand(t, c, "test"), + isCommandWithArgs: (t: string, c: string) => isCommandWithArgs(t, c, "test"), + }; + + const transportThread = { id: "telegram:42", post: vi.fn() }; + + // dispatchInTurnCommand works for any descriptor list (it's just match+invoke+post) + const handled = await internals.dispatchInTurnCommand( + preTurn, matchers, + transportThread, { text: "/toggle-quality-inspector on" }, + "/toggle-quality-inspector on", "main", + ); + expect(handled).toBe(true); + expect(postRich).toHaveBeenCalledTimes(1); + + const [passedThread, passedResponse] = postRich.mock.calls[0]; + expect(passedThread).toBe(transportThread); + expect(passedResponse.text).toContain("ON"); + }); + }); +}); diff --git a/test/pi-settings.test.ts b/test/pi-settings.test.ts new file mode 100644 index 0000000..2111759 --- /dev/null +++ b/test/pi-settings.test.ts @@ -0,0 +1,171 @@ +/** + * test/pi-settings.test.ts — Unit tests for src/pi-settings.ts + */ + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { resolve, dirname } from "node:path"; +import { mkdirSync, writeFileSync, readFileSync, rmSync, existsSync, statSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { randomBytes } from "node:crypto"; + +// We test via the real module but with a patched PI_SETTINGS_PATH +// to avoid touching the real ~/.pi/agent/settings.json. +// Vitest runs in the same process, so we mock the constant via vi.mock. + +const TEST_DIR = resolve(tmpdir(), `pi-settings-test-${randomBytes(4).toString("hex")}`); +const TEST_SETTINGS_PATH = resolve(TEST_DIR, "settings.json"); +const TEST_LOCK_PATH = resolve(TEST_DIR, ".settings.lock"); + +import { vi } from "vitest"; + +// Mock the paths used by pi-settings +vi.mock("node:os", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + homedir: () => resolve(TEST_DIR, "fakehome"), + }; +}); + +// The module uses resolve(homedir(), ".pi", "agent", "settings.json") +// With our mocked homedir, it becomes TEST_DIR/fakehome/.pi/agent/settings.json +function getSettingsPath(): string { + return resolve(TEST_DIR, "fakehome", ".pi", "agent", "settings.json"); +} + +describe("pi-settings", () => { + beforeEach(() => { + mkdirSync(resolve(TEST_DIR, "fakehome", ".pi", "agent"), { recursive: true }); + }); + + afterEach(() => { + try { rmSync(TEST_DIR, { recursive: true, force: true }); } catch {} + }); + + // Dynamic import to pick up the mocked homedir + async function getModule() { + // Clear module cache to get fresh imports with mocked homedir + const mod = await import("../src/pi-settings"); + return mod; + } + + describe("readPiSettings", () => { + it("returns {} when file does not exist", async () => { + const { readPiSettings } = await getModule(); + const result = await readPiSettings(); + expect(result).toEqual({}); + }); + + it("returns parsed settings from valid JSON", async () => { + const { readPiSettings } = await getModule(); + const settingsPath = getSettingsPath(); + writeFileSync(settingsPath, JSON.stringify({ defaultProvider: "test", packages: ["a"] })); + const result = await readPiSettings(); + expect(result.defaultProvider).toBe("test"); + expect(result.packages).toEqual(["a"]); + }); + + it("throws MalformedPiSettingsError on invalid JSON", async () => { + const { readPiSettings, MalformedPiSettingsError } = await getModule(); + const settingsPath = getSettingsPath(); + writeFileSync(settingsPath, "not json {{{"); + await expect(readPiSettings()).rejects.toThrow(MalformedPiSettingsError); + }); + }); + + describe("writePiSettings", () => { + it("writes atomic JSON and deduplicates packages", async () => { + const { writePiSettings } = await getModule(); + const settingsPath = getSettingsPath(); + await writePiSettings({ defaultProvider: "p", packages: ["a", "b", "a"] }); + const content = JSON.parse(readFileSync(settingsPath, "utf8")); + expect(content.packages).toEqual(["a", "b"]); + expect(content.defaultProvider).toBe("p"); + }); + + it("cleans up tmp file on success (no stale .tmp files)", async () => { + const { writePiSettings } = await getModule(); + await writePiSettings({ packages: [] }); + const dir = dirname(getSettingsPath()); + const files = require("node:fs").readdirSync(dir); + const tmpFiles = files.filter((f: string) => f.includes(".tmp.")); + expect(tmpFiles).toHaveLength(0); + }); + }); + + describe("updatePiSettings", () => { + it("serialises concurrent calls (all mutations observed)", async () => { + const { updatePiSettings } = await getModule(); + const settingsPath = getSettingsPath(); + writeFileSync(settingsPath, JSON.stringify({ count: 0 })); + + // Run 10 concurrent increments + const promises = Array.from({ length: 10 }, () => + updatePiSettings((s) => ({ ...s, count: ((s as any).count ?? 0) + 1 })), + ); + await Promise.all(promises); + + const result = JSON.parse(readFileSync(settingsPath, "utf8")); + expect(result.count).toBe(10); + }); + }); + + describe("enablePiPackage", () => { + it("adds package to empty settings", async () => { + const { enablePiPackage } = await getModule(); + const { changed } = await enablePiPackage("npm:test-pkg"); + expect(changed).toBe(true); + const content = JSON.parse(readFileSync(getSettingsPath(), "utf8")); + expect(content.packages).toContain("npm:test-pkg"); + }); + + it("is idempotent — second call returns changed=false", async () => { + const { enablePiPackage } = await getModule(); + await enablePiPackage("npm:test-pkg"); + const { changed } = await enablePiPackage("npm:test-pkg"); + expect(changed).toBe(false); + }); + }); + + describe("disablePiPackage", () => { + it("removes package from existing packages", async () => { + const { enablePiPackage, disablePiPackage } = await getModule(); + await enablePiPackage("npm:test-pkg"); + const { changed } = await disablePiPackage("npm:test-pkg"); + expect(changed).toBe(true); + const content = JSON.parse(readFileSync(getSettingsPath(), "utf8")); + expect(content.packages).not.toContain("npm:test-pkg"); + }); + + it("is idempotent — removing non-existent pkg returns changed=false", async () => { + const { disablePiPackage } = await getModule(); + const settingsPath = getSettingsPath(); + writeFileSync(settingsPath, JSON.stringify({ packages: ["other"] })); + const { changed } = await disablePiPackage("npm:not-there"); + expect(changed).toBe(false); + }); + }); + + describe("deduplication", () => { + it("deduplicates packages on write", async () => { + const { updatePiSettings } = await getModule(); + const settingsPath = getSettingsPath(); + writeFileSync(settingsPath, JSON.stringify({ packages: ["a", "b", "a", "c", "b"] })); + await updatePiSettings((s) => s); // identity mutation triggers write + const content = JSON.parse(readFileSync(settingsPath, "utf8")); + expect(content.packages).toEqual(["a", "b", "c"]); + }); + }); + + describe("malformed JSON preservation", () => { + it("does not modify file on malformed JSON error", async () => { + const { enablePiPackage, MalformedPiSettingsError } = await getModule(); + const settingsPath = getSettingsPath(); + const badContent = "{invalid json here"; + writeFileSync(settingsPath, badContent); + await expect(enablePiPackage("npm:test")).rejects.toThrow(MalformedPiSettingsError); + // File unchanged + expect(readFileSync(settingsPath, "utf8")).toBe(badContent); + }); + }); +});