diff --git a/.npmrc b/.npmrc new file mode 100644 index 0000000..fcb4c98 --- /dev/null +++ b/.npmrc @@ -0,0 +1,4 @@ +# Supply-chain quarantine: refuse to install any npm package published < 7 days ago. +# Organization-wide policy; critical after 2026-05-12 Mini Shai-Hulud wave +# (@mistralai/mistralai 2.2.2-2.2.4, 169 packages total). +min-release-age=7 diff --git a/CHANGELOG.md b/CHANGELOG.md index 5803b3a..bc22ba9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # Changelog +## 1.3.0 (2026-05-11) + +### Features + +- **Persistent + runtime-toggleable enabled state.** New `enabled: boolean` setting in `.hardno/settings.json` (default `true`). Persisted across sessions — previously the Alt+R / `/review` toggle was in-memory only and reset on restart. Now the state sticks. + +- **External runtime toggle.** The extension re-reads just the `enabled` field from disk at each `agent_end`, so external tools (e.g. `roundhouse`'s `/toggle-review`) can flip it by writing to `~/.pi/.hardno/settings.json` without restarting the session. New exports: `isEnabledFromDisk()`, `writeEnabledToDisk()`. + +- **Atomic writes.** Toggle persistence uses tmp+rename so a crash mid-write never leaves a partial settings file. + +### Notes + +- Default behavior unchanged: no `enabled` key → treated as `true`. +- Local `.hardno/settings.json` (cwd) takes precedence over global (`~/.pi/.hardno/`). When local exists but has no `enabled` key, it does NOT fall through to global — the more-specific file "wins" on silence too. + ## 1.2.0 (2026-05-10) ### Features diff --git a/README.md b/README.md index 75455f0..127de52 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,7 @@ Use `/scaffold-review-files` to generate config templates. ```json { + "enabled": true, "maxReviewLoops": 100, "model": "amazon-bedrock/us.meta.llama4-maverick-17b-instruct-v1:0", "thinkingLevel": "off", @@ -84,21 +85,24 @@ Use `/scaffold-review-files` to generate config templates. } ``` -| Setting | Type | Default | Description | -| ------------------ | ----------- | -------------------------------------------------------------- | ------------------------------------------------------------------------------------------ | -| `maxReviewLoops` | integer > 0 | `100` | Max review→fix→review cycles before giving up | -| `model` | string | `"amazon-bedrock/us.meta.llama4-maverick-17b-instruct-v1:0"` | Reviewer model (`"provider/model-id"`) | -| `thinkingLevel` | string | `"off"` | `off\|minimal\|low\|medium\|high\|xhigh` | -| `architectEnabled` | boolean | `true` | Enable architect review (triggers when >1 file reviewed from git) | -| `reviewTimeoutMs` | integer > 0 | `120000` | Max wall-clock per review in ms | -| `toggleShortcut` | string | `"alt+r"` | Key id for toggling review on/off | -| `judgeEnabled` | boolean | `false` | Opt-in LLM gate that suppresses redundant reviews on read-only turns (see [Judge](#judge)) | -| `judgeModel` | string | `"amazon-bedrock/us.anthropic.claude-haiku-4-5-20251001-v1:0"` | Model used by the judge (`"provider/model-id"`) | -| `judgeTimeoutMs` | integer > 0 | `10000` | Max wall-clock per judge classification call in ms | -| `cancelShortcut` | string | `""` (none) | Key id for cancelling review (opt-in, see below) | +| Setting | Type | Default | Description | +| ------------------ | ----------- | -------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- | +| `enabled` | boolean | `true` | Master on/off for auto-review. Persisted; re-read each turn so external tools (e.g. roundhouse `/toggle-review`) take effect without session restart. | +| `maxReviewLoops` | integer > 0 | `100` | Max review→fix→review cycles before giving up | +| `model` | string | `"amazon-bedrock/us.meta.llama4-maverick-17b-instruct-v1:0"` | Reviewer model (`"provider/model-id"`) | +| `thinkingLevel` | string | `"off"` | `off\|minimal\|low\|medium\|high\|xhigh` | +| `architectEnabled` | boolean | `true` | Enable architect review (triggers when >1 file reviewed from git) | +| `reviewTimeoutMs` | integer > 0 | `120000` | Max wall-clock per review in ms | +| `toggleShortcut` | string | `"alt+r"` | Key id for toggling review on/off | +| `judgeEnabled` | boolean | `false` | Opt-in LLM gate that suppresses redundant reviews on read-only turns (see [Judge](#judge)) | +| `judgeModel` | string | `"amazon-bedrock/us.anthropic.claude-haiku-4-5-20251001-v1:0"` | Model used by the judge (`"provider/model-id"`) | +| `judgeTimeoutMs` | integer > 0 | `10000` | Max wall-clock per judge classification call in ms | +| `cancelShortcut` | string | `""` (none) | Key id for cancelling review (opt-in, see below) | > **Note:** `roundupEnabled` is accepted as a legacy alias for `architectEnabled`. +> **`enabled` toggle persistence & routing:** The in-TUI toggle (Alt+R / `/review`) and external tools write to whichever file `loadSettings` would read — local `cwd/.hardno/settings.json` if present, else global `~/.pi/.hardno/settings.json`. This matches the read precedence so a toggle never silently no-ops. The extension re-reads just the `enabled` field at each `agent_end`, so flipping it externally (e.g. via roundhouse `/toggle-review`) takes effect on the NEXT agent turn — no session restart needed. + ### `.hardno/review-rules.md` Custom review rules appended to the reviewer prompt. Only include review criteria — the surrounding prompt (tools, budget, workflow, response format) is handled automatically. diff --git a/dismiss.test.ts b/dismiss.test.ts index 2c5a563..ea9922a 100644 --- a/dismiss.test.ts +++ b/dismiss.test.ts @@ -1,15 +1,21 @@ import { describe, it, expect } from "vitest"; -import { findingKey, numberFindings, parseDismissals, filterSuppressed, DismissTracker } from "./dismiss"; +import { + findingKey, + numberFindings, + parseDismissals, + filterSuppressed, + DismissTracker, +} from "./dismiss"; describe("findingKey", () => { it("extracts severity + location from finding line", () => { - const line = '- **Medium:** src/gateway/model.ts:97 — chatId extraction uses wrong index'; + const line = "- **Medium:** src/gateway/model.ts:97 — chatId extraction uses wrong index"; const key = findingKey(line); expect(key).toContain("medium:src/gateway/model.ts:97"); }); it("handles numbered finding format (F# prefix)", () => { - const line = '- **F1 Medium:** src/foo.ts:10 — something bad'; + const line = "- **F1 Medium:** src/foo.ts:10 — something bad"; const key = findingKey(line); expect(key).toBe("medium:src/foo.ts:10 — something bad"); }); @@ -22,7 +28,7 @@ describe("findingKey", () => { describe("numberFindings", () => { it("numbers finding bullets sequentially", () => { - const text = '- **High:** foo.ts:1 — bug\n- **Low:** bar.ts:2 — nit\nSome other text'; + const text = "- **High:** foo.ts:1 — bug\n- **Low:** bar.ts:2 — nit\nSome other text"; const { numbered, findings } = numberFindings(text); expect(numbered).toContain("**F1 High:**"); expect(numbered).toContain("**F2 Low:**"); @@ -31,7 +37,7 @@ describe("numberFindings", () => { }); it("preserves non-finding lines unchanged", () => { - const text = 'Header\n\n- **Medium:** x.ts:5 — issue\n\nFooter'; + const text = "Header\n\n- **Medium:** x.ts:5 — issue\n\nFooter"; const { numbered } = numberFindings(text); expect(numbered).toContain("Header"); expect(numbered).toContain("Footer"); @@ -41,7 +47,8 @@ describe("numberFindings", () => { describe("parseDismissals", () => { it("parses DISMISS F# with colon separator", () => { - const text = "The chatId extraction is intentional.\nDISMISS F1: intentional design for telegram thread format"; + const text = + "The chatId extraction is intentional.\nDISMISS F1: intentional design for telegram thread format"; const dismissals = parseDismissals(text); expect(dismissals.size).toBe(1); expect(dismissals.get(1)).toBe("intentional design for telegram thread format"); @@ -69,21 +76,21 @@ describe("parseDismissals", () => { describe("filterSuppressed", () => { it("removes suppressed findings", () => { - const text = '- **High:** foo.ts:1 — bug one\n- **Low:** bar.ts:2 — nit two'; - const suppressed = new Set([findingKey('- **High:** foo.ts:1 — bug one')]); + const text = "- **High:** foo.ts:1 — bug one\n- **Low:** bar.ts:2 — nit two"; + const suppressed = new Set([findingKey("- **High:** foo.ts:1 — bug one")]); const result = filterSuppressed(text, suppressed); expect(result).not.toContain("bug one"); expect(result).toContain("nit two"); }); it("returns null when all findings suppressed", () => { - const text = '- **High:** foo.ts:1 — bug one'; - const suppressed = new Set([findingKey('- **High:** foo.ts:1 — bug one')]); + const text = "- **High:** foo.ts:1 — bug one"; + const suppressed = new Set([findingKey("- **High:** foo.ts:1 — bug one")]); expect(filterSuppressed(text, suppressed)).toBeNull(); }); it("returns original when no suppressions", () => { - const text = '- **Low:** x.ts:5 — something'; + const text = "- **Low:** x.ts:5 — something"; expect(filterSuppressed(text, new Set())).toBe(text); }); }); @@ -91,7 +98,7 @@ describe("filterSuppressed", () => { describe("DismissTracker", () => { it("tracks dismissals and suppresses after threshold", () => { const tracker = new DismissTracker(); - const findings = ['- **Medium:** src/foo.ts:10 — bad pattern', '- **Low:** src/bar.ts:5 — nit']; + const findings = ["- **Medium:** src/foo.ts:10 — bad pattern", "- **Low:** src/bar.ts:5 — nit"]; tracker.setLastFindings(findings); // First dismiss @@ -106,7 +113,7 @@ describe("DismissTracker", () => { it("reset clears all state", () => { const tracker = new DismissTracker(); - tracker.setLastFindings(['- **High:** x.ts:1 — bug']); + tracker.setLastFindings(["- **High:** x.ts:1 — bug"]); tracker.processDismissals("DISMISS F1: nope"); tracker.processDismissals("DISMISS F1: nope again"); expect(tracker.getSuppressed().size).toBe(1); diff --git a/dismiss.ts b/dismiss.ts index 2b298e9..c0a1a9c 100644 --- a/dismiss.ts +++ b/dismiss.ts @@ -35,16 +35,18 @@ export function numberFindings(text: string): { numbered: string; findings: stri const findings: string[] = []; let counter = 0; - const numbered = lines.map(line => { - // Match finding bullets: - **Severity:** ... - const match = line.match(/^(\s*-\s*)\*\*(\w+):\*\*(.*)$/); - if (match) { - counter++; - findings.push(line); - return `${match[1]}**F${counter} ${match[2]}:**${match[3]}`; - } - return line; - }).join("\n"); + const numbered = lines + .map((line) => { + // Match finding bullets: - **Severity:** ... + const match = line.match(/^(\s*-\s*)\*\*(\w+):\*\*(.*)$/); + if (match) { + counter++; + findings.push(line); + return `${match[1]}**F${counter} ${match[2]}:**${match[3]}`; + } + return line; + }) + .join("\n"); return { numbered, findings }; } @@ -53,7 +55,7 @@ export function numberFindings(text: string): { numbered: string; findings: stri export function parseDismissals(text: string): Map { const dismissals = new Map(); // Match: DISMISS F1: reason or DISMISS F1 - reason or DISMISS F1 reason - const pattern = /DISMISS\s+F(\d+)\s*[:–\-]\s*(.+)/gi; + const pattern = /DISMISS\s+F(\d+)\s*[:–-]\s*(.+)/gi; let match; while ((match = pattern.exec(text)) !== null) { dismissals.set(parseInt(match[1], 10), match[2].trim()); @@ -66,7 +68,7 @@ export function filterSuppressed(text: string, suppressed: Set): string if (suppressed.size === 0) return text; const lines = text.split("\n"); - const filtered = lines.filter(line => { + const filtered = lines.filter((line) => { const match = line.match(/^\s*-\s*\*\*\w+:\*\*/); if (!match) return true; // keep non-finding lines const key = findingKey(line); @@ -74,7 +76,7 @@ export function filterSuppressed(text: string, suppressed: Set): string }); // If all findings were suppressed, return null (should be LGTM) - const remaining = filtered.filter(l => l.match(/^\s*-\s*\*\*/)); + const remaining = filtered.filter((l) => l.match(/^\s*-\s*\*\*/)); if (remaining.length === 0) return null; return filtered.join("\n"); @@ -112,7 +114,9 @@ export class DismissTracker { this.dismissed.set(key, { key, reason, count: 1 }); } count++; - log(`dismiss: F${fNum} dismissed (${key}) — "${reason}" [count=${this.dismissed.get(key)!.count}]`); + log( + `dismiss: F${fNum} dismissed (${key}) — "${reason}" [count=${this.dismissed.get(key)!.count}]`, + ); } return count; } diff --git a/index.ts b/index.ts index d92d980..c0d2a18 100644 --- a/index.ts +++ b/index.ts @@ -30,6 +30,8 @@ import { loadReviewRules, loadAutoReviewRules, loadShortcutSettingsSync, + isEnabledFromDisk, + writeEnabledToDisk, } from "./settings"; import { runReviewSession } from "./reviewer"; import { classifyBashCommand, defaultJudgeRunner } from "./judge"; @@ -272,6 +274,15 @@ export default function (pi: ExtensionAPI) { try { orchestrator.setEnabled(!orchestrator.isEnabled); + // Persist so the toggle survives restart + is visible to external tools. + // Write to the same file the read path would load (local wins if present) + // so an in-TUI toggle isn't masked by a local settings file on next read. + try { + writeEnabledToDisk(orchestrator.isEnabled, { cwd: ctx.cwd }); + if (settings) settings.enabled = orchestrator.isEnabled; + } catch (err: any) { + log(`warning: could not persist toggle: ${err?.message ?? err}`); + } if (orchestrator.isEnabled) { if (ctx.hasUI) ctx.ui.notify(`Review: on`, "info"); // Only prompt to review if agent is idle and there are pending files. @@ -706,13 +717,28 @@ export default function (pi: ExtensionAPI) { // Process DISMISS markers from agent's response (before running review) if (lastAssistant) { - const textParts = (lastAssistant.content ?? []).filter((b: any) => b.type === "text").map((b: any) => b.text); + const textParts = (lastAssistant.content ?? []) + .filter((b: any) => b.type === "text") + .map((b: any) => b.text); const agentText = textParts.join("\n"); if (agentText) { orchestrator.processDismissals(agentText); } } + // Runtime re-read: pick up toggles made by external tools (e.g. + // roundhouse's /toggle-review) since the last turn. Cheap — synchronous + // stat+parse of a tiny JSON file once per agent_end. + const diskEnabled = isEnabledFromDisk(ctx.cwd); + if (diskEnabled !== null && diskEnabled !== orchestrator.isEnabled) { + log(`runtime toggle: disk says enabled=${diskEnabled}, updating orchestrator`); + orchestrator.setEnabled(diskEnabled); + // F4: guard like the toggleReview handler for consistency. `settings` + // is set on session_start before agent_end can fire, but defensive + // code is cheap and matches the other call site. + if (settings) settings.enabled = diskEnabled; + } + if (!orchestrator.isEnabled) { // Keep tracking state (modifiedFiles, agentToolCalls) so we can // offer to review when the user toggles review back on. @@ -724,7 +750,7 @@ export default function (pi: ExtensionAPI) { await runAutoReview(ctx, "auto"); }); - // ── Shortcuts ────────────────────────────────────── + // ── Shortcuts ────────────────────────────────── // Cancel handler — shared by shortcut + command function cancelReview(ctx: { ui: any; hasUI?: boolean }, source: string) { @@ -874,6 +900,11 @@ export default function (pi: ExtensionAPI) { architectRules = rRules; settings = settingsResult.settings; + // Seed orchestrator enabled-state from persisted settings. This makes the + // toggle survive restarts (previously it was reset to `true` each session). + orchestrator.setEnabled(settings.enabled); + if (!settings.enabled) log(`review starts disabled (persisted enabled=false)`); + if (autoReviewRules) log("Loaded auto-review rules from .hardno/auto-review.md"); if (customRules) log("Loaded custom rules from .hardno/review-rules.md"); if (architectRules) log("Loaded architect rules from .hardno/architect.md"); diff --git a/orchestrator.ts b/orchestrator.ts index 9a19e5b..5eee10b 100644 --- a/orchestrator.ts +++ b/orchestrator.ts @@ -166,7 +166,11 @@ export class ReviewOrchestrator { // All findings suppressed — treat as LGTM if (filtered === null) { log("dismiss: all findings suppressed — treating as LGTM"); - return { ...result, isLgtm: true, text: "No issues found (previously dismissed findings suppressed)." }; + return { + ...result, + isLgtm: true, + text: "No issues found (previously dismissed findings suppressed).", + }; } // Number remaining findings and track them diff --git a/package.json b/package.json index 4d97005..eb843a9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@inceptionstack/pi-hard-no", - "version": "1.2.1", + "version": "1.3.0", "type": "module", "description": "Pi extension — automatic code review after every agent turn", "license": "MIT", diff --git a/settings.ts b/settings.ts index 0fac954..8d08cc6 100644 --- a/settings.ts +++ b/settings.ts @@ -9,8 +9,16 @@ */ import { readFile } from "node:fs/promises"; -import { readFileSync } 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"; /** @@ -58,6 +66,10 @@ function readConfigFileSync(cwd: string, filename: string): string | null { // ── Types ──────────────────────────────────────────── export interface AutoReviewSettings { + /** Master on/off switch for auto-review. Persisted across sessions. + * Re-read from disk at each agent_end so external tools (e.g. roundhouse's + * /toggle-review) can flip it without restarting the session. */ + enabled: boolean; maxReviewLoops: number; model: string; // "provider/model-id" e.g. "amazon-bedrock/us.meta.llama4-maverick-17b-instruct-v1:0" thinkingLevel: string; // "off" | "minimal" | "low" | "medium" | "high" | "xhigh" @@ -85,6 +97,7 @@ export const DEFAULT_TOGGLE_SHORTCUT = "alt+r"; export const DEFAULT_CANCEL_SHORTCUT = ""; // no default shortcut — use /cancel-review command export const DEFAULT_SETTINGS: AutoReviewSettings = { + enabled: true, maxReviewLoops: 100, model: "amazon-bedrock/us.meta.llama4-maverick-17b-instruct-v1:0", thinkingLevel: "off", @@ -112,6 +125,16 @@ export function parseSettings(parsed: Record): { const errors: string[] = []; const settings = { ...DEFAULT_SETTINGS }; + if ("enabled" in parsed) { + if (typeof parsed.enabled === "boolean") { + settings.enabled = parsed.enabled; + } else { + errors.push( + `[hard-no] "enabled" must be a boolean (got ${JSON.stringify(parsed.enabled)}). Using default: ${DEFAULT_SETTINGS.enabled}.`, + ); + } + } + if ("maxReviewLoops" in parsed) { if ( typeof parsed.maxReviewLoops === "number" && @@ -330,3 +353,161 @@ export async function loadAutoReviewRules(cwd: string): Promise { const content = await readConfigFile(cwd, "auto-review.md"); return content?.trim() || null; } + +// ── Runtime on/off toggle ────────────────────────────── + +/** + * Fast synchronous read of just the `enabled` field from settings.json. + * Called on each agent_end to pick up toggles made by external tools (e.g. + * `roundhouse` `/toggle-review`) without restarting the session. + * + * Local (.hardno/settings.json in cwd) takes precedence over global + * (~/.pi/.hardno/settings.json). + * + * Semantics: local "wins on silence" too — if the local file exists but is + * missing/malformed/unreadable (anything except ENOENT), we do NOT fall + * through to global. This prevents a surprise where corrupting local silently + * exposes a different global value. + */ +export function isEnabledFromDisk(cwd: string, home?: string): boolean | null { + for (const dir of configDirs(cwd, home)) { + const path = join(dir, "settings.json"); + let raw: string; + try { + raw = readFileSync(path, "utf8"); + } catch (err: any) { + // Only fall through on ENOENT. Permission errors, IO errors, etc. should + // halt resolution — if the more-specific file exists but we can't read + // it, we shouldn't silently fall through to the less-specific file. + if (err?.code === "ENOENT") continue; + return null; + } + // File exists: parse once; any failure/shape-mismatch = return null WITHOUT + // falling through (local wins on silence / corruption). + try { + const parsed = JSON.parse(raw); + if (typeof parsed === "object" && parsed !== null && !Array.isArray(parsed)) { + if (typeof parsed.enabled === "boolean") return parsed.enabled; + } + return null; + } catch { + return null; + } + } + return null; +} + +/** + * Resolve which settings.json path to *write* to for persisting the `enabled` + * flag. Matches the read-resolution path used by loadSettings / isEnabledFromDisk: + * - If local (cwd/.hardno/settings.json) exists, write there (local wins). + * - Otherwise, write to global (~/.pi/.hardno/settings.json). + * + * This prevents the "toggle appears to revert" bug where writing global + * gets masked by a local file that the read path loads first. + */ +export function resolveWritePath(cwd: string, home?: string): string { + const [localDir, globalDir] = configDirs(cwd, home); + const localPath = join(localDir, "settings.json"); + if (existsSync(localPath)) return localPath; + return join(globalDir, "settings.json"); +} + +/** + * Persist `enabled: boolean` to the settings file selected by resolveWritePath. + * Atomic tmp+rename; preserves other fields. Creates the file/dir if missing. + * + * Race handling: does a read-modify-write with a small retry window. If the + * file mtime changes between our read and our intended rename, we re-read, + * re-apply `enabled`, and retry (up to 3 attempts). Eliminates the common + * case where an external writer (e.g. roundhouse) flips a different field + * concurrently. + * + * Before overwriting a non-plain-object or malformed file (rare; usually + * means another tool wrote garbage), we back up the original bytes to + * .corrupt-.bak so user data isn't silently discarded. + */ +export function writeEnabledToDisk( + enabled: boolean, + opts: { cwd?: string; home?: string; targetPath?: string } = {}, +): void { + const home = opts.home ?? homedir(); + // Explicit targetPath wins (useful for tests); otherwise resolve via cwd. + const target = + opts.targetPath ?? + (opts.cwd ? resolveWritePath(opts.cwd, home) : join(home, ".pi", ".hardno", "settings.json")); + const dir = dirname(target); + + const MAX_ATTEMPTS = 3; + + for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { + // Read existing (preserve other fields) + let existing: Record = {}; + let wasMalformed = false; + let rawBytes: string | null = null; + let readMtime: number | null = null; + try { + rawBytes = readFileSync(target, "utf8"); + readMtime = statSync(target).mtimeMs; + const parsed = JSON.parse(rawBytes); + if (typeof parsed === "object" && parsed !== null && !Array.isArray(parsed)) { + existing = parsed as Record; + } else { + wasMalformed = true; // valid JSON but wrong shape (array / scalar) + } + } catch (err: any) { + if (err?.code !== "ENOENT") wasMalformed = true; + // else: file missing, start fresh + } + + // Safety: if the existing file had bytes we can't safely merge into, + // back them up before overwrite so user data isn't silently lost. + if (wasMalformed && rawBytes !== null) { + try { + const backup = `${target}.corrupt-${Date.now()}.bak`; + writeFileSync(backup, rawBytes, { encoding: "utf8" }); + } catch { + /* best-effort only */ + } + } + + existing.enabled = enabled; + + // Atomic write: tmp + rename in the same directory. + mkdirSync(dir, { recursive: true }); + const tmp = `${target}.tmp-${process.pid}-${Date.now()}-${attempt}`; + try { + writeFileSync(tmp, JSON.stringify(existing, null, 2) + "\n", { encoding: "utf8" }); + + // Race check: if the file's mtime changed after our read, someone else + // wrote between our read and our rename. Discard our tmp and retry so + // we don't clobber their edits to other fields. + if (readMtime !== null && attempt < MAX_ATTEMPTS) { + try { + const currentMtime = statSync(target).mtimeMs; + if (currentMtime !== readMtime) { + try { + unlinkSync(tmp); + } catch { + /* ignore */ + } + continue; + } + } catch { + /* stat failed — fall through to rename */ + } + } + + renameSync(tmp, target); + return; + } catch (err) { + // Clean up orphan tmp on any failure so we don't leave garbage behind. + try { + unlinkSync(tmp); + } catch { + /* ignore */ + } + throw err; + } + } +} diff --git a/test/settings.test.ts b/test/settings.test.ts index a1d3bd6..6e7cd7c 100644 --- a/test/settings.test.ts +++ b/test/settings.test.ts @@ -8,8 +8,20 @@ import { loadShortcutSettingsSync, configDirs, readConfigFile, + isEnabledFromDisk, + writeEnabledToDisk, + resolveWritePath, + loadSettings, } from "../settings"; -import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from "node:fs"; +import { + mkdtempSync, + writeFileSync, + mkdirSync, + rmSync, + readFileSync, + existsSync, + readdirSync, +} from "node:fs"; import { join } from "node:path"; import { tmpdir, homedir } from "node:os"; @@ -570,3 +582,443 @@ describe("readConfigFile", () => { } }); }); + +describe("enabled setting", () => { + it("parseSettings defaults enabled to true", () => { + const { settings } = parseSettings({}); + expect(settings.enabled).toBe(true); + }); + + it("parseSettings accepts explicit enabled=false", () => { + const { settings, errors } = parseSettings({ enabled: false }); + expect(settings.enabled).toBe(false); + expect(errors).toEqual([]); + }); + + it("parseSettings accepts explicit enabled=true", () => { + const { settings, errors } = parseSettings({ enabled: true }); + expect(settings.enabled).toBe(true); + expect(errors).toEqual([]); + }); + + it("parseSettings rejects non-boolean enabled with error + default", () => { + const { settings, errors } = parseSettings({ enabled: "yes" }); + expect(settings.enabled).toBe(DEFAULT_SETTINGS.enabled); + expect(errors.length).toBe(1); + expect(errors[0]).toMatch(/"enabled".*boolean/); + }); +}); + +describe("isEnabledFromDisk", () => { + function makeDirs() { + const root = mkdtempSync(join(tmpdir(), "hardno-toggle-")); + const localDir = join(root, "project"); + const fakeHome = join(root, "home"); + const localCfg = join(localDir, ".hardno"); + const globalCfg = join(fakeHome, ".pi", ".hardno"); + mkdirSync(localCfg, { recursive: true }); + mkdirSync(globalCfg, { recursive: true }); + return { + root, + localDir, + fakeHome, + localCfg, + globalCfg, + cleanup() { + rmSync(root, { recursive: true, force: true }); + }, + }; + } + + it("returns null when no settings file exists", () => { + const d = makeDirs(); + try { + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBeNull(); + } finally { + d.cleanup(); + } + }); + + it("reads enabled=false from global settings", () => { + const d = makeDirs(); + try { + writeFileSync(join(d.globalCfg, "settings.json"), JSON.stringify({ enabled: false })); + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBe(false); + } finally { + d.cleanup(); + } + }); + + it("reads enabled=true from local settings (local wins)", () => { + const d = makeDirs(); + try { + writeFileSync(join(d.globalCfg, "settings.json"), JSON.stringify({ enabled: false })); + writeFileSync(join(d.localCfg, "settings.json"), JSON.stringify({ enabled: true })); + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBe(true); + } finally { + d.cleanup(); + } + }); + + it("returns null when local file exists but has no enabled key (doesn't fall through)", () => { + // Rationale: the more-specific file wins even when silent. This prevents + // a surprise where removing `enabled` from local silently exposes global. + const d = makeDirs(); + try { + writeFileSync(join(d.localCfg, "settings.json"), JSON.stringify({ model: "x/y" })); + writeFileSync(join(d.globalCfg, "settings.json"), JSON.stringify({ enabled: false })); + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBeNull(); + } finally { + d.cleanup(); + } + }); + + it("returns null on malformed JSON (caller falls back to cached)", () => { + const d = makeDirs(); + try { + writeFileSync(join(d.globalCfg, "settings.json"), "{ not json"); + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBeNull(); + } finally { + d.cleanup(); + } + }); +}); + +describe("writeEnabledToDisk", () => { + function makeFakeHome() { + const root = mkdtempSync(join(tmpdir(), "hardno-write-")); + return { + fakeHome: root, + settingsPath: join(root, ".pi", ".hardno", "settings.json"), + cleanup() { + rmSync(root, { recursive: true, force: true }); + }, + }; + } + + it("creates settings file when missing", () => { + const h = makeFakeHome(); + try { + writeEnabledToDisk(false, { home: h.fakeHome }); + const raw = readFileSync(h.settingsPath, "utf8"); + expect(JSON.parse(raw).enabled).toBe(false); + } finally { + h.cleanup(); + } + }); + + it("preserves other fields when flipping enabled", () => { + const h = makeFakeHome(); + try { + mkdirSync(join(h.fakeHome, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify({ model: "a/b", reviewTimeoutMs: 99_999 })); + writeEnabledToDisk(true, { home: h.fakeHome }); + const parsed = JSON.parse(readFileSync(h.settingsPath, "utf8")); + expect(parsed.enabled).toBe(true); + expect(parsed.model).toBe("a/b"); + expect(parsed.reviewTimeoutMs).toBe(99_999); + } finally { + h.cleanup(); + } + }); + + it("overwrites existing enabled value", () => { + const h = makeFakeHome(); + try { + writeEnabledToDisk(false, { home: h.fakeHome }); + writeEnabledToDisk(true, { home: h.fakeHome }); + expect(JSON.parse(readFileSync(h.settingsPath, "utf8")).enabled).toBe(true); + } finally { + h.cleanup(); + } + }); + + it("recovers from malformed existing file (overwrites with fresh content)", () => { + const h = makeFakeHome(); + try { + mkdirSync(join(h.fakeHome, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, "{ corrupt"); + writeEnabledToDisk(false, { home: h.fakeHome }); + const parsed = JSON.parse(readFileSync(h.settingsPath, "utf8")); + expect(parsed.enabled).toBe(false); + } finally { + h.cleanup(); + } + }); + + it("leaves no tmp file behind", () => { + const h = makeFakeHome(); + try { + writeEnabledToDisk(false, { home: h.fakeHome }); + const dir = join(h.fakeHome, ".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); + } finally { + h.cleanup(); + } + }); +}); + +describe("resolveWritePath + precedence (F1 fix)", () => { + function makeDirs() { + const root = mkdtempSync(join(tmpdir(), "hardno-write-prec-")); + const localDir = join(root, "project"); + const fakeHome = join(root, "home"); + const localCfg = join(localDir, ".hardno"); + const globalCfg = join(fakeHome, ".pi", ".hardno"); + mkdirSync(localCfg, { recursive: true }); + mkdirSync(globalCfg, { recursive: true }); + return { + root, + localDir, + fakeHome, + localCfg, + globalCfg, + cleanup() { + rmSync(root, { recursive: true, force: true }); + }, + }; + } + + it("resolveWritePath returns global when no local file exists", () => { + const d = makeDirs(); + try { + const p = resolveWritePath(d.localDir, d.fakeHome); + expect(p).toBe(join(d.globalCfg, "settings.json")); + } finally { + d.cleanup(); + } + }); + + it("resolveWritePath returns local when local file exists", () => { + const d = makeDirs(); + try { + writeFileSync(join(d.localCfg, "settings.json"), "{}"); + const p = resolveWritePath(d.localDir, d.fakeHome); + expect(p).toBe(join(d.localCfg, "settings.json")); + } finally { + d.cleanup(); + } + }); + + it("writeEnabledToDisk with cwd writes to local when local exists (F1)", () => { + const d = makeDirs(); + try { + writeFileSync(join(d.localCfg, "settings.json"), JSON.stringify({ model: "m/1" })); + writeEnabledToDisk(false, { cwd: d.localDir, home: d.fakeHome }); + // Local file was updated + const local = JSON.parse(readFileSync(join(d.localCfg, "settings.json"), "utf8")); + expect(local.enabled).toBe(false); + expect(local.model).toBe("m/1"); + // Global file untouched (doesn't exist) + expect(existsSync(join(d.globalCfg, "settings.json"))).toBe(false); + } finally { + d.cleanup(); + } + }); + + it("writeEnabledToDisk with cwd writes to global when no local (F1)", () => { + const d = makeDirs(); + try { + writeEnabledToDisk(false, { cwd: d.localDir, home: d.fakeHome }); + const global = JSON.parse(readFileSync(join(d.globalCfg, "settings.json"), "utf8")); + expect(global.enabled).toBe(false); + expect(existsSync(join(d.localCfg, "settings.json"))).toBe(false); + } finally { + d.cleanup(); + } + }); + + it("end-to-end: toggle writes to local, read picks it up (no masking)", () => { + const d = makeDirs(); + try { + // Pre-existing local file without `enabled` + writeFileSync(join(d.localCfg, "settings.json"), JSON.stringify({ model: "x/y" })); + // Pre-existing global file with enabled=true + writeFileSync(join(d.globalCfg, "settings.json"), JSON.stringify({ enabled: true })); + + // Write to the effective path (local wins) + writeEnabledToDisk(false, { cwd: d.localDir, home: d.fakeHome }); + + // Read path should now see local.enabled=false, NOT fall through to global + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBe(false); + } finally { + d.cleanup(); + } + }); +}); + +describe("isEnabledFromDisk (F2 fix: local silence does not fall through)", () => { + function makeDirs() { + const root = mkdtempSync(join(tmpdir(), "hardno-silence-")); + const localDir = join(root, "project"); + const fakeHome = join(root, "home"); + const localCfg = join(localDir, ".hardno"); + const globalCfg = join(fakeHome, ".pi", ".hardno"); + mkdirSync(localCfg, { recursive: true }); + mkdirSync(globalCfg, { recursive: true }); + return { + root, + localDir, + fakeHome, + localCfg, + globalCfg, + cleanup() { + rmSync(root, { recursive: true, force: true }); + }, + }; + } + + it("malformed local does NOT fall through to global", () => { + const d = makeDirs(); + try { + writeFileSync(join(d.localCfg, "settings.json"), "{ not json"); + writeFileSync(join(d.globalCfg, "settings.json"), JSON.stringify({ enabled: false })); + // Bug before fix: would return false. After fix: null (local wins on silence). + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBeNull(); + } finally { + d.cleanup(); + } + }); + + it("array-at-root local does NOT fall through to global", () => { + const d = makeDirs(); + try { + writeFileSync(join(d.localCfg, "settings.json"), JSON.stringify([1, 2, 3])); + writeFileSync(join(d.globalCfg, "settings.json"), JSON.stringify({ enabled: false })); + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBeNull(); + } finally { + d.cleanup(); + } + }); + + it("missing local DOES fall through to global (ENOENT is the only pass-through)", () => { + const d = makeDirs(); + try { + writeFileSync(join(d.globalCfg, "settings.json"), JSON.stringify({ enabled: false })); + expect(isEnabledFromDisk(d.localDir, d.fakeHome)).toBe(false); + } finally { + d.cleanup(); + } + }); +}); + +describe("writeEnabledToDisk safety (F2 race retry, F3 corrupt backup)", () => { + function makeFakeHome() { + const root = mkdtempSync(join(tmpdir(), "hardno-safety-")); + return { + fakeHome: root, + settingsPath: join(root, ".pi", ".hardno", "settings.json"), + cleanup() { + rmSync(root, { recursive: true, force: true }); + }, + }; + } + + it("backs up corrupt bytes before overwrite (F3)", () => { + const h = makeFakeHome(); + try { + mkdirSync(join(h.fakeHome, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, "{ corrupt-content"); + writeEnabledToDisk(false, { home: h.fakeHome }); + const dir = join(h.fakeHome, ".pi", ".hardno"); + const files = readdirSync(dir); + const backup = files.find((f) => f.startsWith("settings.json.corrupt-")); + expect(backup).toBeDefined(); + // Original bytes preserved in backup + expect(readFileSync(join(dir, backup!), "utf8")).toBe("{ corrupt-content"); + // New file is valid JSON with enabled=false + expect(JSON.parse(readFileSync(h.settingsPath, "utf8")).enabled).toBe(false); + } finally { + h.cleanup(); + } + }); + + it("backs up non-plain-object root (array) before overwrite (F3)", () => { + const h = makeFakeHome(); + try { + mkdirSync(join(h.fakeHome, ".pi", ".hardno"), { recursive: true }); + writeFileSync(h.settingsPath, JSON.stringify([1, 2, 3])); + writeEnabledToDisk(false, { home: h.fakeHome }); + const dir = join(h.fakeHome, ".pi", ".hardno"); + const files = readdirSync(dir); + expect(files.some((f) => f.startsWith("settings.json.corrupt-"))).toBe(true); + } finally { + h.cleanup(); + } + }); + + it("cleans up tmp file on write failure", () => { + const h = makeFakeHome(); + try { + // Write to a readonly dir to force failure. Use a path that can't be mkdir'd. + // Simulating a rename failure is hard without a custom fs mock — at minimum, + // check no leftover *.tmp-* files on success (covered by earlier test). + writeEnabledToDisk(false, { home: h.fakeHome }); + const dir = join(h.fakeHome, ".pi", ".hardno"); + const files = readdirSync(dir); + expect(files.every((f) => !f.includes(".tmp-"))).toBe(true); + } finally { + h.cleanup(); + } + }); +}); + +describe("loadSettings integration (F4 coverage)", () => { + function makeDirs() { + const root = mkdtempSync(join(tmpdir(), "hardno-load-int-")); + const localDir = join(root, "project"); + const fakeHome = join(root, "home"); + const localCfg = join(localDir, ".hardno"); + const globalCfg = join(fakeHome, ".pi", ".hardno"); + mkdirSync(localCfg, { recursive: true }); + mkdirSync(globalCfg, { recursive: true }); + return { + root, + localDir, + fakeHome, + localCfg, + globalCfg, + cleanup() { + rmSync(root, { recursive: true, force: true }); + }, + }; + } + + it("persisted enabled=false survives load cycle", async () => { + const d = makeDirs(); + try { + writeEnabledToDisk(false, { cwd: d.localDir, home: d.fakeHome }); + // Simulate subsequent session_start + // loadSettings uses cwd-only path via readConfigFile which does its own + // resolution. The toggle's file is in ~/.pi/.hardno/ (no local). We need + // to make HOME point at fakeHome for loadSettings to find it. + const origHome = process.env.HOME; + try { + process.env.HOME = d.fakeHome; + const { settings } = await loadSettings(d.localDir); + expect(settings.enabled).toBe(false); + } finally { + if (origHome === undefined) delete process.env.HOME; + else process.env.HOME = origHome; + } + } finally { + d.cleanup(); + } + }); + + it("persisted to local file survives load cycle", async () => { + const d = makeDirs(); + try { + // Pre-seed local so resolveWritePath picks it + writeFileSync(join(d.localCfg, "settings.json"), "{}"); + writeEnabledToDisk(false, { cwd: d.localDir, home: d.fakeHome }); + const { settings } = await loadSettings(d.localDir); + expect(settings.enabled).toBe(false); + } finally { + d.cleanup(); + } + }); +});