From f40a7827e6c83666a0ad2d33af7b62c60a2c6659 Mon Sep 17 00:00:00 2001 From: Vladimir Babin Date: Sat, 15 Aug 2026 08:24:42 +0300 Subject: [PATCH 1/2] fix(policies): honour NO_COLOR in hooks manager output; add --no-color MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #688. `failproofai policies` printed ANSI colour even when NO_COLOR was set, because src/hooks/manager.ts hardcoded ANSI escapes at 16 sites instead of gating on the shared predicate the rest of the CLI uses. - Add an ansiHelpers() factory in manager.ts that gates on tui.ts's existing colorsEnabled() (!!out.isTTY && !process.env.NO_COLOR) — no new colour module (per #256). All 16 sites now route through green/yellow/red/dim wrappers that return the string unchanged when colour is off, so plain-text glyphs and column widths are byte-identical minus the escape sequences. - Add a global --no-color flag in bin/failproofai.mjs that sets NO_COLOR=1 and is stripped from args before subcommand parsing; documented in COMMANDS help. - Add a vitest regression test asserting listHooks emits zero ESC bytes under NO_COLOR (isTTY forced true) and still emits them with colour on. --- __tests__/hooks/manager-no-color.test.ts | 83 ++++++++++++++++++++++++ bin/failproofai.mjs | 12 ++++ src/hooks/manager.ts | 53 ++++++++++----- 3 files changed, 132 insertions(+), 16 deletions(-) create mode 100644 __tests__/hooks/manager-no-color.test.ts diff --git a/__tests__/hooks/manager-no-color.test.ts b/__tests__/hooks/manager-no-color.test.ts new file mode 100644 index 00000000..46da518e --- /dev/null +++ b/__tests__/hooks/manager-no-color.test.ts @@ -0,0 +1,83 @@ +// @vitest-environment node +// +// `failproofai policies` (listHooks) hardcoded ANSI escapes in +// src/hooks/manager.ts, so it printed color even when NO_COLOR=1 / --no-color +// was set under a TTY — every other surface routes through tui.ts's +// `colorsEnabled` predicate, and manager.ts was the holdout (issue #688). +// +// These assert the gate both ways: colored when a TTY has color on, and zero +// ESC bytes the moment NO_COLOR is set — with stdout.isTTY forced true so the +// off-TTY short-circuit is not what is being measured. +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { listHooks } from "@/src/hooks/manager"; + +const ESC = "\x1B["; + +function policySource(hookName: string): string { + return ` + import { customPolicies, allow } from "failproofai"; + customPolicies.add({ + name: ${JSON.stringify(hookName)}, + description: "test policy", + match: { events: ["PreToolUse"] }, + fn: async () => allow(), + }); + `; +} + +describe("listHooks — NO_COLOR / --no-color gating", () => { + let tmp: string; + let emptyHome: string; + let lines: string[]; + let logSpy: ReturnType; + let origIsTTY: boolean | undefined; + + beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), "fp-nocolor-")); + emptyHome = mkdtempSync(join(tmpdir(), "fp-nocolor-home-")); + vi.stubEnv("HOME", emptyHome); + vi.stubEnv("USERPROFILE", emptyHome); + // Seed a convention policy so a colored "✓ ON" status row is produced. + const dir = join(tmp, ".failproofai", "policies"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "team-policies.mjs"), policySource("team-rule"), "utf8"); + + lines = []; + logSpy = vi.spyOn(console, "log").mockImplementation((...a: unknown[]) => { + lines.push(a.map(String).join(" ")); + }); + // The escapes short-circuit off a TTY; force one so we measure the + // NO_COLOR gate, not the isTTY gate. + origIsTTY = process.stdout.isTTY; + Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }); + }); + + afterEach(() => { + logSpy.mockRestore(); + vi.unstubAllEnvs(); + Object.defineProperty(process.stdout, "isTTY", { value: origIsTTY, configurable: true }); + rmSync(tmp, { recursive: true, force: true }); + rmSync(emptyHome, { recursive: true, force: true }); + }); + + it("emits ANSI escapes on a color TTY", async () => { + delete process.env.NO_COLOR; + await listHooks(tmp); + expect(lines.join("\n")).toContain(ESC); + }); + + it("emits zero ESC bytes when NO_COLOR is set", async () => { + vi.stubEnv("NO_COLOR", "1"); + await listHooks(tmp); + const out = lines.join("\n"); + expect(out).not.toContain(ESC); + // The plain text still renders — header and the convention section's + // filename column are printed regardless of whether the policy loads. + expect(out).toContain("Failproof AI"); + expect(out).toContain("team-policies.mjs"); + }); +}); diff --git a/bin/failproofai.mjs b/bin/failproofai.mjs index a0b34cc5..9f199f9d 100755 --- a/bin/failproofai.mjs +++ b/bin/failproofai.mjs @@ -34,6 +34,17 @@ if (!process.env.FAILPROOFAI_DIST_PATH) { const args = process.argv.slice(2); +// Global `--no-color`: strip it before subcommand parsing and set NO_COLOR so +// every color surface (tui.ts's `colorsEnabled`, and thus src/hooks/manager.ts) +// falls back to plain text. Removing it from `args` keeps it from being +// mistaken for a policy name or an unknown subcommand. +if (args.includes("--no-color")) { + process.env.NO_COLOR = "1"; + for (let i = args.length - 1; i >= 0; i--) { + if (args[i] === "--no-color") args.splice(i, 1); + } +} + // Normalize 'p' → 'policies' (shorthand alias) if (args[0] === "p") args[0] = "policies"; // Normalize 'configure' / 'setup' → 'config' (aliases), so every later check @@ -365,6 +376,7 @@ COMMANDS --dry-run Show what would be removed, change nothing --yes, -y Skip the confirmation prompt + --no-color Disable ANSI color (also honors NO_COLOR=1) --version, -v Print version and exit --help, -h Show this help message diff --git a/src/hooks/manager.ts b/src/hooks/manager.ts index 2c9dd4ce..f20c54ad 100644 --- a/src/hooks/manager.ts +++ b/src/hooks/manager.ts @@ -26,9 +26,28 @@ import { CliError } from "../cli-error"; import { hookLogWarn } from "./hook-logger"; import { customPoliciesDir, globalPolicyConfigFile } from "./fp-home"; import { readActiveCloudManagedPolicies } from "./cloud-managed-policies"; +import { colorsEnabled } from "./tui"; const VALID_POLICY_NAMES = new Set(BUILTIN_POLICIES.map((p) => p.name)); +/** + * NO_COLOR-aware ANSI wrappers, gated on tui.ts's single `colorsEnabled` + * predicate (`!!out.isTTY && !process.env.NO_COLOR`) — the same source of + * truth `audit/cli.ts` uses. When color is off (piped output, or NO_COLOR / + * `--no-color`) each helper returns the string unchanged, so the plain-text + * width and glyphs stay byte-identical minus the escape sequences. + */ +function ansiHelpers(out: NodeJS.WriteStream = process.stdout) { + const on = colorsEnabled(out); + const wrap = (code: string) => (s: string) => (on ? `\x1B[${code}m${s}\x1B[0m` : s); + return { + green: wrap("32"), // success ✓ / ON + yellow: wrap("33"), // warnings ⚠ / unknown key / MIXED / OBS + red: wrap("31"), // ✗ errors / file not found + dim: wrap("2"), // OFF / beta separator + }; +} + /** Settings path for the Claude Code integration. Kept as a public export for `app/actions/get-hooks-config.ts`. */ export function getSettingsPath(scope: HookScope, cwd?: string): string { return claudeCode.getSettingsPath(scope, cwd); @@ -391,8 +410,9 @@ async function installHooksImpl( const duplicates = otherScopes.filter((s) => hooksInstalledInSettings(s, cwd)); if (duplicates.length > 0) { const scopeList = duplicates.map((s) => `${s} (${scopeLabel(s)})`).join(", "); + const { yellow } = ansiHelpers(); console.log(); - console.log(`\x1B[33mWarning: Failproof AI hooks are also installed at ${scopeList}.\x1B[0m`); + console.log(yellow(`Warning: Failproof AI hooks are also installed at ${scopeList}.`)); console.log(`Having hooks in multiple scopes may cause duplicate policy evaluation.`); console.log(`Use \`failproofai policies --uninstall --scope ${duplicates[0]}\` to remove the other installation,`); console.log(`or \`failproofai policies\` to see all scopes.`); @@ -592,6 +612,7 @@ export async function removeHooks(policyNames?: string[], scope: HookScope | "al * - Custom Hooks section if customPoliciesPath is set */ export async function listHooks(cwd?: string): Promise { + const { green, yellow, red, dim } = ansiHelpers(); const config = readMergedHooksConfig(cwd); const enabledSet = new Set(config.enabledPolicies); const disabledCustomSet = new Set(config.disabledCustomPolicies ?? []); @@ -621,13 +642,13 @@ export async function listHooks(cwd?: string): Promise { const statusCol = 8; const printSimpleRow = (policy: { name: string; description: string }) => { - const mark = enabledSet.has(policy.name) ? `\x1B[32m\u2713\x1B[0m` : " "; + const mark = enabledSet.has(policy.name) ? green(`\u2713`) : " "; console.log(` ${mark}${" ".repeat(statusCol - 1)}${policy.name.padEnd(nameColWidth)}${policy.description}`); printParamsSummary(policy.name, ` ${" ".repeat(statusCol)}`); }; const printBetaSection = (printRow: (p: { name: string; description: string }) => void) => { if (betaPolicies.length > 0) { - console.log(`\n \x1B[2m\u2500\u2500 Beta \u2500\u2500\x1B[0m`); + console.log(`\n ${dim(`\u2500\u2500 Beta \u2500\u2500`)}`); for (const policy of betaPolicies) printRow(policy); } }; @@ -686,7 +707,7 @@ export async function listHooks(cwd?: string): Promise { let row = " "; for (const _scope of installedScopes) { if (enabled) { - row += `\x1B[32m\u2713 ON\x1B[0m` + " ".repeat(COL - 4); + row += green(`\u2713 ON`) + " ".repeat(COL - 4); } else { row += " OFF" + " ".repeat(COL - 5); } @@ -699,7 +720,7 @@ export async function listHooks(cwd?: string): Promise { for (const policy of regularPolicies) printMultiScopeRow(policy); if (betaPolicies.length > 0) { - console.log(`\n \x1B[2m\u2500\u2500 Beta \u2500\u2500\x1B[0m`); + console.log(`\n ${dim(`\u2500\u2500 Beta \u2500\u2500`)}`); for (const policy of betaPolicies) printMultiScopeRow(policy); } @@ -708,7 +729,7 @@ export async function listHooks(cwd?: string): Promise { // Multi-scope warning const scopeNames = installedScopes.join(", "); console.log(); - console.log(`\x1B[33m\u26A0 Hooks in multiple scopes (${scopeNames}).\x1B[0m`); + console.log(yellow(`\u26A0 Hooks in multiple scopes (${scopeNames}).`)); console.log(" Consider keeping one. Remove with: failproofai policies --uninstall --scope \n"); } @@ -717,7 +738,7 @@ export async function listHooks(cwd?: string): Promise { const unknownKeys: string[] = []; for (const key of Object.keys(config.policyParams)) { if (!builtinPolicyNames.has(key)) { - console.log(` \x1B[33mWarning: unknown policyParams key "${key}" — possible typo\x1B[0m`); + console.log(` ${yellow(`Warning: unknown policyParams key "${key}" — possible typo`)}`); unknownKeys.push(key); } } @@ -742,17 +763,17 @@ export async function listHooks(cwd?: string): Promise { const absPath = resolve(findProjectConfigDir(cwd ?? process.cwd()), path); console.log(` ${absPath}`); if (!existsSync(absPath)) { - console.log(` \x1B[31m\u2717 File not found: ${absPath}\x1B[0m`); + console.log(` ${red(`\u2717 File not found: ${absPath}`)}`); continue; } const hooks = await loadCustomHooks(absPath); if (hooks.length === 0) { - console.log(` \x1B[31m\u2717 ERR failed to load (check ~/.failproofai/logs/hooks.log)\x1B[0m`); + console.log(` ${red(`\u2717 ERR failed to load (check ~/.failproofai/logs/hooks.log)`)}`); } else { const descColWidth = nameColWidth; for (const hook of hooks) { const disabled = disabledCustomSet.has(`custom:${absPath}:${hook.name}`); - const status = disabled ? "\x1B[2m OFF\x1B[0m" : "\x1B[32m\u2713 ON\x1B[0m"; + const status = disabled ? dim(` OFF`) : green(`\u2713 ON`); console.log(` ${status} ${hook.name.padEnd(descColWidth)}${hook.description ?? ""}`); } } @@ -814,7 +835,7 @@ export async function listHooks(cwd?: string): Promise { const filename = basename(file); record(filename, hooks.map((h) => h.name)); if (hooks.length === 0) { - console.log(` \x1B[31m\u2717\x1B[0m ${filename.padEnd(colWidth)}\x1B[31mfailed to load\x1B[0m`); + console.log(` ${red(`\u2717`)} ${filename.padEnd(colWidth)}${red(`failed to load`)}`); } else { const hookStates = hooks.map((hook) => ({ hook, @@ -822,10 +843,10 @@ export async function listHooks(cwd?: string): Promise { })); const disabledCount = hookStates.filter((entry) => entry.disabled).length; const status = disabledCount === 0 - ? "\x1B[32m\u2713 ON\x1B[0m" + ? green(`\u2713 ON`) : disabledCount === hooks.length - ? "\x1B[2m OFF\x1B[0m" - : "\x1B[33m\u25D0 MIXED\x1B[0m"; + ? dim(` OFF`) + : yellow(`\u25D0 MIXED`); const hookSummary = hookStates .map(({ hook, disabled }) => `${hook.name}${disabled ? " (OFF)" : ""}`) .join(", "); @@ -834,7 +855,7 @@ export async function listHooks(cwd?: string): Promise { } catch { const filename = basename(file); record(filename, []); - console.log(` \x1B[31m\u2717\x1B[0m ${filename.padEnd(colWidth)}\x1B[31merror\x1B[0m`); + console.log(` ${red(`\u2717`)} ${filename.padEnd(colWidth)}${red(`error`)}`); } } console.log(); @@ -862,7 +883,7 @@ export async function listHooks(cwd?: string): Promise { // that read "ON" would claim enforcement this policy deliberately is // not doing. const status = - artifact.effect === "observe" ? "\x1B[33m\u25D0 OBS\x1B[0m" : "\x1B[32m\u2713 ON\x1B[0m"; + artifact.effect === "observe" ? yellow(`\u25D0 OBS`) : green(`\u2713 ON`); console.log(` ${status} ${artifact.id.padEnd(colWidth)}v${artifact.version}`); } console.log("\n Managed from the dashboard \u2014 not switchable with `failproofai policies`."); From 4838d732d8fcbf9d67900f98b85b6de8e59a1ed7 Mon Sep 17 00:00:00 2001 From: Vladimir Babin Date: Sat, 15 Aug 2026 08:36:39 +0300 Subject: [PATCH 2/2] test(policies): harden NO_COLOR test per review - Assert on the convention-policy status row (team-policies.mjs) so the color-on test measures manager.ts's own gated output rather than incidental color elsewhere. - Use vi.stubEnv("NO_COLOR", "") instead of delete process.env.NO_COLOR so vi.unstubAllEnvs() restores the worker's original value in afterEach. --- __tests__/hooks/manager-no-color.test.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/__tests__/hooks/manager-no-color.test.ts b/__tests__/hooks/manager-no-color.test.ts index 46da518e..239ba028 100644 --- a/__tests__/hooks/manager-no-color.test.ts +++ b/__tests__/hooks/manager-no-color.test.ts @@ -65,9 +65,20 @@ describe("listHooks — NO_COLOR / --no-color gating", () => { }); it("emits ANSI escapes on a color TTY", async () => { - delete process.env.NO_COLOR; + // Register NO_COLOR with vi.stubEnv (empty = unset) so afterEach's + // vi.unstubAllEnvs() restores the worker's original value instead of + // leaving it deleted for later tests. + vi.stubEnv("NO_COLOR", ""); await listHooks(tmp); - expect(lines.join("\n")).toContain(ESC); + const out = lines.join("\n"); + // The convention-policy section renders a colored status for the seeded + // team-policies.mjs (a green "✓ ON" when its `import "failproofai"` resolves, + // or a red "✗ failed to load" when the bare specifier can't resolve in this + // env). Either way manager.ts wraps that span in an ANSI escape — assert on + // the row so the test measures manager.ts's own gated output, not incidental + // color from elsewhere. + expect(out).toContain("team-policies.mjs"); + expect(out).toMatch(/\x1B\[3[0-9]m/); // a foreground-color span from the status row }); it("emits zero ESC bytes when NO_COLOR is set", async () => { @@ -75,8 +86,8 @@ describe("listHooks — NO_COLOR / --no-color gating", () => { await listHooks(tmp); const out = lines.join("\n"); expect(out).not.toContain(ESC); - // The plain text still renders — header and the convention section's - // filename column are printed regardless of whether the policy loads. + // The same rows still render as plain text — the header and the convention + // section's status/filename — proving only the color wrapping was dropped. expect(out).toContain("Failproof AI"); expect(out).toContain("team-policies.mjs"); });