diff --git a/README.md b/README.md index bb24158..3268893 100644 --- a/README.md +++ b/README.md @@ -216,6 +216,8 @@ Roundhouse automatically registers these commands with Telegram on startup: | `/new` | Start a fresh conversation (resets the agent session for this chat) | | `/compact` | Compact session context to free up tokens | | `/verbose` | Toggle tool status messages on/off for this chat | +| `/toggle-review` | Toggle the pi-hard-no auto code review on/off (persists; no restart needed) | +| `/toggle-code-review` | Alias of `/toggle-review` | | `/status` | Show gateway status: version, agent, model, context usage, uptime, etc. | | `/stop` | Stop the current agent run (abort tools, LLM calls, compaction) | | `/restart` | Restart the gateway service (requires `allowedUsers` to be configured) | @@ -246,6 +248,12 @@ Toggles verbose mode for the current chat. When ON, shows tool call status messa Aborts the current agent run for this chat — stops any in-progress tool calls, LLM generation, and compaction. The session is preserved; send another message to continue the conversation. +### `/toggle-review` + +Flips the [pi-hard-no](https://github.com/inceptionstack/pi-hard-no) auto code review on/off and **persists** the state. Takes effect on the next agent turn — no session restart needed (pi-hard-no v1.3.0+ re-reads the setting at each turn). + +Writes to whichever settings file pi-hard-no reads: project-local `/.hardno/settings.json` if present, otherwise global `~/.pi/.hardno/settings.json`. The reply indicates which file was updated. Alias: `/toggle-code-review`. + ### Follow-up notifications When extensions (e.g. code review) queue follow-up work after the agent responds, the gateway shows: diff --git a/src/gateway/code-review-toggle.test.ts b/src/gateway/code-review-toggle.test.ts new file mode 100644 index 0000000..b12998f --- /dev/null +++ b/src/gateway/code-review-toggle.test.ts @@ -0,0 +1,249 @@ +/** + * code-review-toggle.test.ts — Tests for the pi-hard-no enabled flag I/O. + */ + +import { describe, it, expect, afterEach } from "vitest"; +import { mkdtempSync, writeFileSync, mkdirSync, rmSync, readFileSync, existsSync, readdirSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { + toggleEnabled, + readEnabled, + resolveSettingsPath, + resolveGlobalSettingsPath, +} from "./code-review-toggle"; + +let tmpRoots: string[] = []; +function makeFakeHome(): { home: string; settingsPath: string } { + const home = mkdtempSync(join(tmpdir(), "rh-toggle-")); + tmpRoots.push(home); + return { home, settingsPath: join(home, ".pi", ".hardno", "settings.json") }; +} +function makeCwdHome(): { + home: string; + cwd: string; + globalPath: string; + localPath: string; +} { + const root = mkdtempSync(join(tmpdir(), "rh-toggle-cwd-")); + tmpRoots.push(root); + const home = join(root, "home"); + const cwd = join(root, "project"); + mkdirSync(home, { recursive: true }); + mkdirSync(cwd, { recursive: true }); + return { + home, + cwd, + globalPath: join(home, ".pi", ".hardno", "settings.json"), + localPath: join(cwd, ".hardno", "settings.json"), + }; +} +afterEach(() => { + for (const r of tmpRoots) { + try { rmSync(r, { recursive: true, force: true }); } catch { /* ignore */ } + } + tmpRoots = []; +}); + +describe("resolveGlobalSettingsPath", () => { + it("joins home + ~/.pi/.hardno/settings.json", () => { + expect(resolveGlobalSettingsPath("/fake/home")).toBe("/fake/home/.pi/.hardno/settings.json"); + }); +}); + +describe("resolveSettingsPath (F6 fix: local-vs-global routing)", () => { + it("returns global when no cwd given", () => { + const h = makeFakeHome(); + const r = resolveSettingsPath({ home: h.home }); + expect(r.path).toBe(h.settingsPath); + expect(r.isLocal).toBe(false); + }); + + it("returns global when cwd given but no local file exists", () => { + const d = makeCwdHome(); + const r = resolveSettingsPath({ home: d.home, cwd: d.cwd }); + expect(r.path).toBe(d.globalPath); + expect(r.isLocal).toBe(false); + }); + + it("returns local when cwd given AND local .hardno/settings.json exists", () => { + const d = makeCwdHome(); + mkdirSync(join(d.cwd, ".hardno"), { recursive: true }); + writeFileSync(d.localPath, "{}"); + const r = resolveSettingsPath({ home: d.home, cwd: d.cwd }); + expect(r.path).toBe(d.localPath); + expect(r.isLocal).toBe(true); + }); +}); + +describe("readEnabled", () => { + it("returns null when no file exists (global)", () => { + const h = makeFakeHome(); + expect(readEnabled({ home: h.home })).toBeNull(); + }); + + it("reads true from global", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: true })); + expect(readEnabled({ home: h.home })).toBe(true); + }); + + it("reads false from global", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: false, model: "x/y" })); + expect(readEnabled({ home: h.home })).toBe(false); + }); + + it("returns null when enabled key absent", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ model: "x/y" })); + expect(readEnabled({ home: h.home })).toBeNull(); + }); + + it("returns null on malformed JSON", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, "{ not json"); + expect(readEnabled({ home: h.home })).toBeNull(); + }); + + it("reads local when cwd given and local exists (local wins)", () => { + const d = makeCwdHome(); + mkdirSync(join(d.home, ".pi", ".hardno"), { recursive: true }); + mkdirSync(join(d.cwd, ".hardno"), { recursive: true }); + writeFileSync(d.globalPath, JSON.stringify({ enabled: false })); + writeFileSync(d.localPath, JSON.stringify({ enabled: true })); + expect(readEnabled({ home: d.home, cwd: d.cwd })).toBe(true); + }); +}); + +describe("toggleEnabled", () => { + it("creates global settings file when missing, flips default true → false", () => { + const h = makeFakeHome(); + const result = toggleEnabled({ home: h.home }); + expect(result.enabled).toBe(false); + expect(result.fileExisted).toBe(false); + expect(result.wroteLocal).toBe(false); + expect(result.settingsPath).toBe(h.settingsPath); + expect(JSON.parse(readFileSync(h.settingsPath, "utf8")).enabled).toBe(false); + }); + + it("flips false → true (global)", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: false })); + const result = toggleEnabled({ home: h.home }); + expect(result.enabled).toBe(true); + expect(result.fileExisted).toBe(true); + expect(JSON.parse(readFileSync(h.settingsPath, "utf8")).enabled).toBe(true); + }); + + it("flips true → false (global)", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: true })); + const result = toggleEnabled({ home: h.home }); + expect(result.enabled).toBe(false); + expect(JSON.parse(readFileSync(h.settingsPath, "utf8")).enabled).toBe(false); + }); + + it("preserves other fields when flipping", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync( + h.settingsPath, + JSON.stringify({ enabled: true, model: "a/b", reviewTimeoutMs: 99_999, nested: { k: 1 } }) + ); + toggleEnabled({ home: h.home }); + const parsed = JSON.parse(readFileSync(h.settingsPath, "utf8")); + expect(parsed.enabled).toBe(false); + expect(parsed.model).toBe("a/b"); + expect(parsed.reviewTimeoutMs).toBe(99_999); + expect(parsed.nested).toEqual({ k: 1 }); + }); + + it("treats missing enabled key as default true → flips to false", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ model: "x/y" })); + const result = toggleEnabled({ home: h.home }); + expect(result.enabled).toBe(false); + const parsed = JSON.parse(readFileSync(h.settingsPath, "utf8")); + expect(parsed.enabled).toBe(false); + expect(parsed.model).toBe("x/y"); + }); + + it("recovers from malformed existing file by overwriting", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, "{ corrupt"); + const result = toggleEnabled({ home: h.home }); + expect(result.enabled).toBe(false); + expect(JSON.parse(readFileSync(h.settingsPath, "utf8")).enabled).toBe(false); + }); + + it("leaves no tmp file behind after write", () => { + const h = makeFakeHome(); + toggleEnabled({ home: h.home }); + const dir = join(h.home, ".pi", ".hardno"); + const files = readdirSync(dir); + expect(files.some(f => f.startsWith("settings.json.tmp"))).toBe(false); + expect(files).toContain("settings.json"); + expect(existsSync(h.settingsPath)).toBe(true); + }); + + it("two consecutive toggles return to original state", () => { + const h = makeFakeHome(); + const r1 = toggleEnabled({ home: h.home }); + const r2 = toggleEnabled({ home: h.home }); + expect(r1.enabled).toBe(false); + expect(r2.enabled).toBe(true); + }); +}); + +describe("toggleEnabled routing (F6 fix)", () => { + it("writes local when local file exists, leaves global untouched", () => { + const d = makeCwdHome(); + mkdirSync(join(d.cwd, ".hardno"), { recursive: true }); + writeFileSync(d.localPath, JSON.stringify({ model: "m/1" })); + + const result = toggleEnabled({ home: d.home, cwd: d.cwd }); + + expect(result.wroteLocal).toBe(true); + expect(result.settingsPath).toBe(d.localPath); + const local = JSON.parse(readFileSync(d.localPath, "utf8")); + expect(local.enabled).toBe(false); + expect(local.model).toBe("m/1"); + expect(existsSync(d.globalPath)).toBe(false); + }); + + it("writes global when cwd given but no local file", () => { + const d = makeCwdHome(); + const result = toggleEnabled({ home: d.home, cwd: d.cwd }); + expect(result.wroteLocal).toBe(false); + expect(result.settingsPath).toBe(d.globalPath); + expect(existsSync(d.localPath)).toBe(false); + expect(JSON.parse(readFileSync(d.globalPath, "utf8")).enabled).toBe(false); + }); + + it("end-to-end: toggle with cwd writes local, read with cwd sees it (no masking)", () => { + const d = makeCwdHome(); + mkdirSync(join(d.home, ".pi", ".hardno"), { recursive: true }); + mkdirSync(join(d.cwd, ".hardno"), { recursive: true }); + // Pre-existing local without `enabled` + writeFileSync(d.localPath, JSON.stringify({ model: "x/y" })); + // Pre-existing global with enabled=true + writeFileSync(d.globalPath, JSON.stringify({ enabled: true })); + + // Toggle (should flip local default true → false, writing to local) + const result = toggleEnabled({ home: d.home, cwd: d.cwd }); + expect(result.wroteLocal).toBe(true); + expect(result.enabled).toBe(false); + + // Read path (local wins) should see false + expect(readEnabled({ home: d.home, cwd: d.cwd })).toBe(false); + }); +}); diff --git a/src/gateway/code-review-toggle.ts b/src/gateway/code-review-toggle.ts new file mode 100644 index 0000000..9bd1750 --- /dev/null +++ b/src/gateway/code-review-toggle.ts @@ -0,0 +1,176 @@ +/** + * gateway/code-review-toggle.ts — Read/write pi-hard-no's persistent `enabled` flag. + * + * The pi-hard-no extension (v1.3.0+) reads `enabled: boolean` from + * ~/.pi/.hardno/settings.json on each agent_end. Flipping the value here + * takes effect on the next agent turn — no session restart needed. + * + * File routing (matches pi-hard-no's resolveWritePath): + * - If cwd is provided AND /.hardno/settings.json exists, write there. + * - Otherwise, write to ~/.pi/.hardno/settings.json. + * + * Rationale: a project-local .hardno/settings.json takes precedence in + * pi-hard-no's read path. Writing to global when local exists would silently + * fail — the user toggles, but pi-hard-no keeps reading the shadowing local + * file. Matching pi-hard-no's write path eliminates that footgun. + * + * Atomic write: tmp + rename; preserves any other fields already in the file. + * Includes mtime-based retry to handle concurrent writes from pi-hard-no + * itself (e.g. Alt+R toggle happening at the same moment). + */ + +import { + readFileSync, + writeFileSync, + renameSync, + mkdirSync, + existsSync, + statSync, + unlinkSync, +} from "node:fs"; +import { join, dirname } from "node:path"; +import { homedir } from "node:os"; + +/** Default shape when no file exists (pi-hard-no treats missing `enabled` as `true`). */ +const DEFAULT_ENABLED = true; +const MAX_WRITE_ATTEMPTS = 3; + +export interface ToggleOptions { + /** Home dir override (defaults to os.homedir()). */ + home?: string; + /** + * Optional working directory. When provided, a local .hardno/settings.json + * at this path takes precedence (both for reading current state and for + * writing the new one) — matching pi-hard-no's resolution order. + */ + cwd?: string; +} + +export interface ToggleResult { + /** The new state after the toggle. */ + enabled: boolean; + /** Whether the settings file existed before the toggle. */ + fileExisted: boolean; + /** Absolute path we wrote to. */ + settingsPath: string; + /** True if we wrote to a project-local .hardno/settings.json (not global). */ + wroteLocal: boolean; +} + +/** Resolve ~/.pi/.hardno/settings.json for the current user. */ +export function resolveGlobalSettingsPath(home = homedir()): string { + return join(home, ".pi", ".hardno", "settings.json"); +} + +/** + * Resolve which settings file the toggle should act on. + * Local wins when present, matching pi-hard-no's read order. + */ +export function resolveSettingsPath(opts: ToggleOptions = {}): { + path: string; + isLocal: boolean; +} { + const home = opts.home ?? homedir(); + if (opts.cwd) { + const localPath = join(opts.cwd, ".hardno", "settings.json"); + if (existsSync(localPath)) return { path: localPath, isLocal: true }; + } + return { path: resolveGlobalSettingsPath(home), isLocal: false }; +} + +/** Read just the `enabled` field from the effective (local-or-global) file. + * Returns null if unreadable / missing / malformed. + * + * Semantics match pi-hard-no's isEnabledFromDisk: if the more-specific + * (local) file exists but can't be parsed or lacks `enabled`, we do NOT + * fall through to global. + */ +export function readEnabled(opts: ToggleOptions = {}): boolean | null { + const { path } = resolveSettingsPath(opts); + if (!existsSync(path)) { + // If local didn't exist (or no cwd given), we're already at global. + // If cwd was given and we resolved to global, that's fine — fall through + // naturally. existsSync==false means no file to read. + return null; + } + try { + const parsed = JSON.parse(readFileSync(path, "utf8")); + if (typeof parsed === "object" && parsed !== null && !Array.isArray(parsed)) { + if (typeof parsed.enabled === "boolean") return parsed.enabled; + } + return null; + } catch { + return null; + } +} + +/** + * Flip the `enabled` flag. If no file exists, starts from the default (true) + * and flips to false. Preserves all other keys in the file. + * + * Atomic: tmp + rename so a partial-file read is impossible. + * Race-safe: captures mtime at read, checks before rename; on mismatch + * (another writer intervened), re-reads and retries up to 3 times. + * Cleanup: orphan tmp files removed on any write/rename failure. + * + * Routing: if cwd is given and /.hardno/settings.json exists, flips + * that file (local wins). Otherwise flips ~/.pi/.hardno/settings.json. + */ +export function toggleEnabled(opts: ToggleOptions = {}): ToggleResult { + const { path, isLocal } = resolveSettingsPath(opts); + const dir = dirname(path); + + for (let attempt = 1; attempt <= MAX_WRITE_ATTEMPTS; attempt++) { + let existing: Record = {}; + let fileExisted = false; + let current = DEFAULT_ENABLED; + let readMtime: number | null = null; + + if (existsSync(path)) { + fileExisted = true; + try { + const raw = readFileSync(path, "utf8"); + readMtime = statSync(path).mtimeMs; + const parsed = JSON.parse(raw); + if (typeof parsed === "object" && parsed !== null && !Array.isArray(parsed)) { + existing = parsed as Record; + if (typeof existing.enabled === "boolean") current = existing.enabled; + } + } catch { + /* malformed — start fresh, overwrite */ + } + } + + const next = !current; + existing.enabled = next; + + mkdirSync(dir, { recursive: true }); + const tmp = `${path}.tmp-${process.pid}-${Date.now()}-${attempt}`; + try { + writeFileSync(tmp, JSON.stringify(existing, null, 2) + "\n", { encoding: "utf8" }); + + // Race check: if another writer updated the file between our read and + // our rename, discard this tmp and retry so we don't clobber their edits. + if (readMtime !== null && attempt < MAX_WRITE_ATTEMPTS) { + try { + const currentMtime = statSync(path).mtimeMs; + if (currentMtime !== readMtime) { + try { unlinkSync(tmp); } catch { /* ignore */ } + continue; + } + } catch { + /* stat failed — fall through to rename */ + } + } + + renameSync(tmp, path); + return { enabled: next, fileExisted, settingsPath: path, wroteLocal: isLocal }; + } catch (err) { + try { unlinkSync(tmp); } catch { /* ignore */ } + throw err; + } + } + + // Unreachable under normal control flow (loop either returns or throws). + throw new Error("toggleEnabled: exhausted retries without a successful write"); +} diff --git a/src/gateway/commands.ts b/src/gateway/commands.ts index 0becce9..0f9365f 100644 --- a/src/gateway/commands.ts +++ b/src/gateway/commands.ts @@ -12,6 +12,7 @@ import { prepareMemoryForTurn, finalizeMemoryForTurn, flushMemoryThenCompact, de // TODO: move progress into TransportAdapter when multi-transport lands import { createProgressMessage } from "../transports/telegram/progress"; import { getSystemResources } from "./helpers"; +import { toggleEnabled, readEnabled } from "./code-review-toggle"; // ── Types ──────────────────────────────────────────── @@ -418,3 +419,62 @@ export async function handleCrons(ctx: CronsContext): Promise { } console.log(`[roundhouse] /crons for thread=${thread.id}`); } + + +// ── /toggle-review ─────────────────────────────────── + +export interface ToggleReviewContext { + thread: any; + agentThreadId: string; + /** Agent cwd for local-vs-global settings routing. Optional; when absent, + * we write to global ~/.pi/.hardno/settings.json. */ + agentCwd?: string; +} + +/** + * Toggle the pi-hard-no auto code-review on/off persistently. + * + * Writes to whichever settings file pi-hard-no would read (local `.hardno/` + * in the agent's cwd if present, else global `~/.pi/.hardno/`). Takes effect + * on the NEXT agent turn — no session restart needed — because pi-hard-no + * v1.3.0+ re-reads this field at each agent_end. + * + * If pi-hard-no isn't installed, the write still succeeds (just a JSON file + * nobody reads). We can't reliably detect install state, so we don't try. + */ +export async function handleToggleReview(ctx: ToggleReviewContext): Promise { + const { thread, agentThreadId, agentCwd } = ctx; + try { + const result = toggleEnabled({ cwd: agentCwd }); + const state = result.enabled ? "on" : "off"; + const icon = result.enabled ? "✅" : "🔕"; + const scope = result.wroteLocal ? "project-local" : "global"; + const firstTime = result.fileExisted ? "" : "\n_(First time — created settings file.)_"; + await thread.post( + `${icon} Code review: *${state}*\n\n` + + `_Wrote to ${scope} config (\`${result.settingsPath}\`). ` + + `Takes effect on the next agent turn — no restart needed._` + + firstTime + ); + console.log( + `[roundhouse] /toggle-review thread=${thread.id} agentThread=${agentThreadId} → enabled=${result.enabled} path=${result.settingsPath}` + ); + } catch (err) { + const msg = (err as Error).message; + console.error(`[roundhouse] /toggle-review failed:`, err); + try { + await thread.post(`⚠️ Toggle failed: ${msg}`); + } catch { + /* best-effort notify */ + } + } +} + +/** + * Show current code review state without toggling. Read-only helper for UX. + */ +export function currentReviewState(agentCwd?: string): "on" | "off" | "default" { + const v = readEnabled({ cwd: agentCwd }); + if (v === null) return "default"; + return v ? "on" : "off"; +} diff --git a/src/gateway/gateway.ts b/src/gateway/gateway.ts index 231adea..16b0e30 100644 --- a/src/gateway/gateway.ts +++ b/src/gateway/gateway.ts @@ -22,7 +22,7 @@ import { createProgressMessage } from "../transports/telegram/progress"; import { isCommand as _isCmd, isCommandWithArgs as _isCmdArgs, resolveAgentThreadId as _resolveThread, getSystemResources as _getSysRes } from "./helpers"; import { saveAttachments, type AttachmentResult } from "./attachments"; import { handleStreaming as _handleStream } from "./streaming"; -import { handleNew, handleRestart, handleUpdate, handleCompact, handleStatus, handleStop, handleVerbose, handleDoctor, handleCrons, type CommandContext } from "./commands"; +import { handleNew, handleRestart, handleUpdate, handleCompact, handleStatus, handleStop, handleVerbose, handleDoctor, handleCrons, handleToggleReview, type CommandContext } from "./commands"; import { handleModel, handleModelAction, MODEL_ACTION_ID } from "./model-command"; import { handleLater } from "./later-command"; import { handleTopic, applyTopicOverride } from "./topic-command"; @@ -284,6 +284,14 @@ export class Gateway { await handleVerbose({ thread, agentThreadId, verboseThreads }); return; } + // /toggle-review — flip pi-hard-no code-review on/off (persists across sessions) + if (_isCmd(text, "/toggle-review", _botUsername) || _isCmd(text, "/toggle-code-review", _botUsername)) { + if (!isAllowed(message, allowedUsers, allowedUserIds)) return; + const agent = this.router.resolve(agentThreadId); + const agentCwd = (agent.getInfo?.()?.cwd as string | undefined); + await handleToggleReview({ thread, agentThreadId, agentCwd }); + return; + } // /doctor — run health checks immediately if (_isCmd(text, "/doctor", _botUsername)) { if (!isAllowed(message, allowedUsers, allowedUserIds)) return;