From 6b76618f0132f3a494da56275883e981e453f78f Mon Sep 17 00:00:00 2001 From: Loki FastStart Date: Mon, 11 May 2026 14:20:49 +0000 Subject: [PATCH 1/3] feat: /toggle-review command for pi-hard-no runtime on/off New slash command (alias: /toggle-code-review) that flips ~/.pi/.hardno/settings.json `enabled` boolean. Takes effect on the NEXT agent turn \u2014 no session restart needed \u2014 because pi-hard-no v1.3.0+ re-reads that field at each agent_end. Implementation: - src/gateway/code-review-toggle.ts * resolveSettingsPath() / readEnabled() / toggleEnabled() * Atomic write (tmp + rename), preserves other keys in the file * Treats missing file / missing enabled key as default `true` * Recovers from malformed JSON by overwriting - src/gateway/commands.ts: handleToggleReview() posts confirmation - src/gateway/gateway.ts: wired into handleOrAbort (snappy path, not thread-queue-gated, matches /verbose) - src/gateway/code-review-toggle.test.ts: 15 unit tests Command depends on pi-hard-no \u2265 1.3.0 being installed for runtime effect; the write itself always succeeds (plain JSON), so the command is safe even if the extension isn't loaded \u2014 just a no-op at the extension end. Tests: 15 new / 471 total green. Typecheck clean. --- src/gateway/code-review-toggle.test.ts | 153 +++++++++++++++++++++++++ src/gateway/code-review-toggle.ts | 83 ++++++++++++++ src/gateway/commands.ts | 57 +++++++++ src/gateway/gateway.ts | 8 +- 4 files changed, 300 insertions(+), 1 deletion(-) create mode 100644 src/gateway/code-review-toggle.test.ts create mode 100644 src/gateway/code-review-toggle.ts diff --git a/src/gateway/code-review-toggle.test.ts b/src/gateway/code-review-toggle.test.ts new file mode 100644 index 0000000..76b8159 --- /dev/null +++ b/src/gateway/code-review-toggle.test.ts @@ -0,0 +1,153 @@ +/** + * 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 } 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") }; +} +afterEach(() => { + for (const r of tmpRoots) { + try { rmSync(r, { recursive: true, force: true }); } catch { /* ignore */ } + } + tmpRoots = []; +}); + +describe("resolveSettingsPath", () => { + it("joins home + ~/.pi/.hardno/settings.json", () => { + expect(resolveSettingsPath("/fake/home")).toBe("/fake/home/.pi/.hardno/settings.json"); + }); +}); + +describe("readEnabled", () => { + it("returns null when file does not exist", () => { + const h = makeFakeHome(); + expect(readEnabled(h.home)).toBeNull(); + }); + + it("returns true when enabled=true", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: true })); + expect(readEnabled(h.home)).toBe(true); + }); + + it("returns false when enabled=false", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: false, model: "x/y" })); + expect(readEnabled(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(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(h.home)).toBeNull(); + }); + + it("returns null on non-boolean enabled (string)", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: "yes" })); + expect(readEnabled(h.home)).toBeNull(); + }); +}); + +describe("toggleEnabled", () => { + it("creates settings file when missing, flips default true → false", () => { + const h = makeFakeHome(); + const result = toggleEnabled(h.home); + expect(result.enabled).toBe(false); + expect(result.fileExisted).toBe(false); + expect(result.settingsPath).toBe(h.settingsPath); + expect(JSON.parse(readFileSync(h.settingsPath, "utf8")).enabled).toBe(false); + }); + + it("flips false → true", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: false })); + const result = toggleEnabled(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", () => { + const h = makeFakeHome(); + mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ enabled: true })); + const result = toggleEnabled(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(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(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(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(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(h.home); + const r2 = toggleEnabled(h.home); + expect(r1.enabled).toBe(false); + expect(r2.enabled).toBe(true); + }); +}); diff --git a/src/gateway/code-review-toggle.ts b/src/gateway/code-review-toggle.ts new file mode 100644 index 0000000..6c6da41 --- /dev/null +++ b/src/gateway/code-review-toggle.ts @@ -0,0 +1,83 @@ +/** + * 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. + * + * Atomic write: tmp + rename; preserves any other fields already in the file. + */ + +import { readFileSync, writeFileSync, renameSync, mkdirSync, existsSync } from "node:fs"; +import { join } 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; + +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; +} + +/** Resolve ~/.pi/.hardno/settings.json for the current user. */ +export function resolveSettingsPath(home = homedir()): string { + return join(home, ".pi", ".hardno", "settings.json"); +} + +/** Read just the `enabled` field. Returns null if unreadable / missing / malformed. */ +export function readEnabled(home = homedir()): boolean | null { + const path = resolveSettingsPath(home); + if (!existsSync(path)) 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 crash mid-write never leaves a partial file. + */ +export function toggleEnabled(home = homedir()): ToggleResult { + const path = resolveSettingsPath(home); + const dir = join(home, ".pi", ".hardno"); + + let existing: Record = {}; + let fileExisted = false; + let current = DEFAULT_ENABLED; + + if (existsSync(path)) { + fileExisted = true; + try { + const parsed = JSON.parse(readFileSync(path, "utf8")); + 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()}`; + writeFileSync(tmp, JSON.stringify(existing, null, 2) + "\n", { encoding: "utf8" }); + renameSync(tmp, path); + + return { enabled: next, fileExisted, settingsPath: path }; +} diff --git a/src/gateway/commands.ts b/src/gateway/commands.ts index 0becce9..f106e1d 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,59 @@ export async function handleCrons(ctx: CronsContext): Promise { } console.log(`[roundhouse] /crons for thread=${thread.id}`); } + +// ── /toggle-review ─────────────────────────────────── + +export interface ToggleReviewContext { + thread: any; + agentThreadId: string; +} + +/** + * Toggle the pi-hard-no auto code-review on/off persistently. + * + * Writes to ~/.pi/.hardno/settings.json — pi-hard-no v1.3.0+ re-reads this + * field at each agent_end, so the change takes effect on the NEXT agent turn + * without a session restart. + * + * If pi-hard-no isn't installed in the user's pi setup, the write still + * succeeds (just a JSON file nobody reads). We can't reliably detect install + * state from here, so we don't try — the command stays simple. + */ +export async function handleToggleReview(ctx: ToggleReviewContext): Promise { + const { thread, agentThreadId } = ctx; + try { + const result = toggleEnabled(); + const state = result.enabled ? "on" : "off"; + const icon = result.enabled ? "✅" : "🔕"; + const note = result.fileExisted + ? "" + : "\n_(First time — created `~/.pi/.hardno/settings.json`.)_"; + await thread.post( + `${icon} Code review: *${state}*\n\n` + + `_Takes effect on the next agent turn — no restart needed._` + + note + ); + console.log( + `[roundhouse] /toggle-review thread=${thread.id} agentThread=${agentThreadId} → enabled=${result.enabled}` + ); + } 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 + * (e.g. `/toggle-review status` — not wired yet but exposed for reuse). + */ +export function currentReviewState(): "on" | "off" | "default" { + const v = readEnabled(); + if (v === null) return "default"; + return v ? "on" : "off"; +} diff --git a/src/gateway/gateway.ts b/src/gateway/gateway.ts index 231adea..4f5bb1c 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,12 @@ 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; + await handleToggleReview({ thread, agentThreadId }); + return; + } // /doctor — run health checks immediately if (_isCmd(text, "/doctor", _botUsername)) { if (!isAllowed(message, allowedUsers, allowedUserIds)) return; From ebdd24cf0e23b9ac7da898dd762db59ec104a3b1 Mon Sep 17 00:00:00 2001 From: Loki FastStart Date: Mon, 11 May 2026 14:43:00 +0000 Subject: [PATCH 2/3] fix: address review findings F6, F1 (lost-update) on /toggle-review F6 (Medium): toggle now respects pi-hard-no's local-vs-global file precedence. New ToggleOptions { cwd?, home? } \u2014 when cwd is provided and /.hardno/settings.json exists, writes there instead of global. Gateway passes agent.getInfo().cwd so the toggle hits the same file pi-hard-no will read on the next agent_end. Previously: roundhouse wrote global, pi-hard-no read local (enabled still true), toggle silently ineffective. F1 Codex (High) / F2 reviewer (Medium): lost-update race on concurrent writers. toggleEnabled() now captures mtime at read and retries (up to 3x) if another writer (e.g. pi-hard-no's own Alt+R handler) updates the file between our read and rename. Also cleans up orphan tmp files on any write/rename failure. API surface changes: - resolveSettingsPath(opts) now returns { path, isLocal } instead of a bare string - resolveGlobalSettingsPath() kept for the case where cwd isn't known - toggleEnabled(opts) signature: ToggleOptions object (was home: string) - readEnabled(opts) signature: ToggleOptions object (was home: string) - ToggleResult.wroteLocal: boolean \u2014 surfaced to handler for UX clarity ("wrote to project-local config" vs "global") User-facing: handleToggleReview now reports scope + path: "Wrote to project-local config (~/repos/x/.hardno/settings.json). Takes effect on the next agent turn \u2014 no restart needed." Tests: 21 new (was 15) / 477 total green. New coverage: - resolveSettingsPath with/without cwd, with/without local file - toggleEnabled routing: local present/absent, global fallback - end-to-end: toggle writes local, read picks it up (F6 regression) --- src/gateway/code-review-toggle.test.ts | 152 +++++++++++++++++++----- src/gateway/code-review-toggle.ts | 155 ++++++++++++++++++++----- src/gateway/commands.ts | 39 ++++--- src/gateway/gateway.ts | 4 +- 4 files changed, 272 insertions(+), 78 deletions(-) diff --git a/src/gateway/code-review-toggle.test.ts b/src/gateway/code-review-toggle.test.ts index 76b8159..b12998f 100644 --- a/src/gateway/code-review-toggle.test.ts +++ b/src/gateway/code-review-toggle.test.ts @@ -6,7 +6,12 @@ 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 } from "./code-review-toggle"; +import { + toggleEnabled, + readEnabled, + resolveSettingsPath, + resolveGlobalSettingsPath, +} from "./code-review-toggle"; let tmpRoots: string[] = []; function makeFakeHome(): { home: string; settingsPath: string } { @@ -14,6 +19,25 @@ function makeFakeHome(): { home: string; settingsPath: string } { 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 */ } @@ -21,79 +45,107 @@ afterEach(() => { tmpRoots = []; }); -describe("resolveSettingsPath", () => { +describe("resolveGlobalSettingsPath", () => { it("joins home + ~/.pi/.hardno/settings.json", () => { - expect(resolveSettingsPath("/fake/home")).toBe("/fake/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 file does not exist", () => { + it("returns null when no file exists (global)", () => { const h = makeFakeHome(); - expect(readEnabled(h.home)).toBeNull(); + expect(readEnabled({ home: h.home })).toBeNull(); }); - it("returns true when enabled=true", () => { + 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(h.home)).toBe(true); + expect(readEnabled({ home: h.home })).toBe(true); }); - it("returns false when enabled=false", () => { + 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(h.home)).toBe(false); + 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(h.home)).toBeNull(); + 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(h.home)).toBeNull(); + expect(readEnabled({ home: h.home })).toBeNull(); }); - it("returns null on non-boolean enabled (string)", () => { - const h = makeFakeHome(); - mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); - writeFileSync(h.settingsPath, JSON.stringify({ enabled: "yes" })); - expect(readEnabled(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 settings file when missing, flips default true → false", () => { + it("creates global settings file when missing, flips default true → false", () => { const h = makeFakeHome(); - const result = toggleEnabled(h.home); + 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", () => { + 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(h.home); + 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", () => { + 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(h.home); + const result = toggleEnabled({ home: h.home }); expect(result.enabled).toBe(false); expect(JSON.parse(readFileSync(h.settingsPath, "utf8")).enabled).toBe(false); }); @@ -105,7 +157,7 @@ describe("toggleEnabled", () => { h.settingsPath, JSON.stringify({ enabled: true, model: "a/b", reviewTimeoutMs: 99_999, nested: { k: 1 } }) ); - toggleEnabled(h.home); + toggleEnabled({ home: h.home }); const parsed = JSON.parse(readFileSync(h.settingsPath, "utf8")); expect(parsed.enabled).toBe(false); expect(parsed.model).toBe("a/b"); @@ -117,7 +169,7 @@ describe("toggleEnabled", () => { const h = makeFakeHome(); mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); writeFileSync(h.settingsPath, JSON.stringify({ model: "x/y" })); - const result = toggleEnabled(h.home); + const result = toggleEnabled({ home: h.home }); expect(result.enabled).toBe(false); const parsed = JSON.parse(readFileSync(h.settingsPath, "utf8")); expect(parsed.enabled).toBe(false); @@ -128,14 +180,14 @@ describe("toggleEnabled", () => { const h = makeFakeHome(); mkdirSync(join(h.home, ".pi", ".hardno"), { recursive: true }); writeFileSync(h.settingsPath, "{ corrupt"); - const result = toggleEnabled(h.home); + 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(h.home); + 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); @@ -145,9 +197,53 @@ describe("toggleEnabled", () => { it("two consecutive toggles return to original state", () => { const h = makeFakeHome(); - const r1 = toggleEnabled(h.home); - const r2 = toggleEnabled(h.home); + 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 index 6c6da41..9bd1750 100644 --- a/src/gateway/code-review-toggle.ts +++ b/src/gateway/code-review-toggle.ts @@ -5,15 +5,46 @@ * ~/.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 } from "node:fs"; -import { join } from "node:path"; +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. */ @@ -22,17 +53,46 @@ export interface ToggleResult { 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 resolveSettingsPath(home = homedir()): string { +export function resolveGlobalSettingsPath(home = homedir()): string { return join(home, ".pi", ".hardno", "settings.json"); } -/** Read just the `enabled` field. Returns null if unreadable / missing / malformed. */ -export function readEnabled(home = homedir()): boolean | null { - const path = resolveSettingsPath(home); - if (!existsSync(path)) return null; +/** + * 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)) { @@ -48,36 +108,69 @@ export function readEnabled(home = homedir()): boolean | 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 crash mid-write never leaves a partial 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(home = homedir()): ToggleResult { - const path = resolveSettingsPath(home); - const dir = join(home, ".pi", ".hardno"); +export function toggleEnabled(opts: ToggleOptions = {}): ToggleResult { + const { path, isLocal } = resolveSettingsPath(opts); + const dir = dirname(path); - let existing: Record = {}; - let fileExisted = false; - let current = DEFAULT_ENABLED; + 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 parsed = JSON.parse(readFileSync(path, "utf8")); - if (typeof parsed === "object" && parsed !== null && !Array.isArray(parsed)) { - existing = parsed as Record; - if (typeof existing.enabled === "boolean") current = existing.enabled; + 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 */ } - } catch { - /* malformed — start fresh, overwrite */ } - } - const next = !current; - existing.enabled = next; + const next = !current; + existing.enabled = next; - mkdirSync(dir, { recursive: true }); - const tmp = `${path}.tmp-${process.pid}-${Date.now()}`; - writeFileSync(tmp, JSON.stringify(existing, null, 2) + "\n", { encoding: "utf8" }); - renameSync(tmp, path); + 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; + } + } - return { enabled: next, fileExisted, settingsPath: path }; + // 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 f106e1d..0f9365f 100644 --- a/src/gateway/commands.ts +++ b/src/gateway/commands.ts @@ -420,40 +420,44 @@ 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 ~/.pi/.hardno/settings.json — pi-hard-no v1.3.0+ re-reads this - * field at each agent_end, so the change takes effect on the NEXT agent turn - * without a session restart. + * 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 in the user's pi setup, the write still - * succeeds (just a JSON file nobody reads). We can't reliably detect install - * state from here, so we don't try — the command stays simple. + * 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 } = ctx; + const { thread, agentThreadId, agentCwd } = ctx; try { - const result = toggleEnabled(); + const result = toggleEnabled({ cwd: agentCwd }); const state = result.enabled ? "on" : "off"; const icon = result.enabled ? "✅" : "🔕"; - const note = result.fileExisted - ? "" - : "\n_(First time — created `~/.pi/.hardno/settings.json`.)_"; + 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` + - `_Takes effect on the next agent turn — no restart needed._` + - note + `_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}` + `[roundhouse] /toggle-review thread=${thread.id} agentThread=${agentThreadId} → enabled=${result.enabled} path=${result.settingsPath}` ); } catch (err) { const msg = (err as Error).message; @@ -467,11 +471,10 @@ export async function handleToggleReview(ctx: ToggleReviewContext): Promise Date: Mon, 11 May 2026 14:46:11 +0000 Subject: [PATCH 3/3] docs: document /toggle-review + /toggle-code-review commands Architect review on pi-hard-no flagged missing docs for the new toggle; same rationale applies here \u2014 it's a user-facing slash command that users won't discover without README mention. - Added row to the Telegram bot commands table (+ alias) - Added details section explaining persistence + routing semantics --- README.md | 8 ++++++++ 1 file changed, 8 insertions(+) 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: