From 2d2862537fdc8da2a2cbf3320d7f68194013ad09 Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Sun, 19 Jul 2026 09:29:39 -0700 Subject: [PATCH 1/2] fix: honor config-file silent and verbose in logger configuration Config-file silent: true was a documented no-op: wrapCommand configured the logger from CLI flags only and Config.getSilent() had no call sites. - ConfigResolver.resolve now re-configures the supplied logger (and the shared fallbackLogger) from the resolved verbose/silent as soon as the config files are merged, so config-file settings apply to everything logged afterwards. CLI flags still win via the existing pick precedence. - Thread the command logger into ConfigResolver.resolve from the convert, install, and gitignore commands (generate and import already did). - Replace the unconfigured module-level ConsoleLogger fallbacks in the qwencode/augmentcode permission translators with a shared fallbackLogger that wrapCommand and ConfigResolver keep configured, and route warnWithFallback(undefined, ...) through it so those warn paths honor silent end to end. Co-Authored-By: Claude Fable 5 --- src/cli/commands/convert.test.ts | 4 + src/cli/commands/convert.ts | 13 +-- src/cli/commands/gitignore.ts | 7 +- src/cli/commands/install.ts | 26 +++--- src/cli/index.ts | 2 + src/cli/wrap-command.ts | 11 ++- src/config/config-resolver.test.ts | 83 +++++++++++++++++-- src/config/config-resolver.ts | 28 ++++++- src/features/mcp/codexcli-mcp.test.ts | 5 +- .../permissions/augmentcode-permissions.ts | 9 +- .../permissions/qwencode-permissions.ts | 9 +- src/utils/logger.test.ts | 44 +++++++++- src/utils/logger.ts | 22 +++-- 13 files changed, 213 insertions(+), 50 deletions(-) diff --git a/src/cli/commands/convert.test.ts b/src/cli/commands/convert.test.ts index e76fd6117..0f18d182b 100644 --- a/src/cli/commands/convert.test.ts +++ b/src/cli/commands/convert.test.ts @@ -86,6 +86,7 @@ describe("convertCommand", () => { expect(ConfigResolver.resolve).toHaveBeenCalledWith( expect.objectContaining({ targets: ["cursor", "claudecode"] }), + { logger: mockLogger }, ); }); }); @@ -101,6 +102,7 @@ describe("convertCommand", () => { targets: ["cursor", "claudecode"], features: ["*"], }), + { logger: mockLogger }, ); expect(RulesProcessor).toHaveBeenCalledWith( @@ -125,6 +127,7 @@ describe("convertCommand", () => { expect.objectContaining({ features: ["rules", "mcp"], }), + { logger: mockLogger }, ); }); @@ -172,6 +175,7 @@ describe("convertCommand", () => { expect(ConfigResolver.resolve).toHaveBeenCalledWith( expect.objectContaining({ global: true }), + { logger: mockLogger }, ); }); }); diff --git a/src/cli/commands/convert.ts b/src/cli/commands/convert.ts index 78c11d36e..e9d11414e 100644 --- a/src/cli/commands/convert.ts +++ b/src/cli/commands/convert.ts @@ -44,11 +44,14 @@ export async function convertCommand(logger: Logger, options: ConvertOptions): P // Pass both source and destinations as `targets` so per-target feature maps // in `rulesync.jsonc` are honored for every tool involved. Default features // to `*` so every feature that both tools support is attempted. - const config = await ConfigResolver.resolve({ - ...options, - targets: [fromTool, ...toTools], - features: options.features ?? ["*"], - }); + const config = await ConfigResolver.resolve( + { + ...options, + targets: [fromTool, ...toTools], + features: options.features ?? ["*"], + }, + { logger }, + ); const isPreview = config.isPreviewMode(); const modePrefix = isPreview ? "[DRY RUN] " : ""; diff --git a/src/cli/commands/gitignore.ts b/src/cli/commands/gitignore.ts index 1eabac9cf..7585f8530 100644 --- a/src/cli/commands/gitignore.ts +++ b/src/cli/commands/gitignore.ts @@ -166,6 +166,8 @@ const extractRulesyncManagedEntries = (content: string): string[] => { export type GitignoreCommandOptions = { readonly targets?: string[]; readonly features?: RulesyncFeatures; + readonly verbose?: boolean; + readonly silent?: boolean; }; const groupEntriesByDestination = ({ @@ -211,7 +213,10 @@ export const gitignoreCommand = async ( ): Promise => { const gitignorePath = join(process.cwd(), ".gitignore"); const gitattributesPath = join(process.cwd(), ".gitattributes"); - const config = await ConfigResolver.resolve({}); + const config = await ConfigResolver.resolve( + { verbose: options?.verbose, silent: options?.silent }, + { logger }, + ); const resolvedEntries = resolveGitignoreEntries({ targets: options?.targets, diff --git a/src/cli/commands/install.ts b/src/cli/commands/install.ts index 83e5b7d4e..64eaa7a36 100644 --- a/src/cli/commands/install.ts +++ b/src/cli/commands/install.ts @@ -44,11 +44,14 @@ async function runRulesyncInstall(logger: Logger, options: InstallCommandOptions // `--mode apm` is required to opt into the APM layout. const apmExists = await apmManifestExists(projectRoot); - const config = await ConfigResolver.resolve({ - configPath: options.configPath, - verbose: options.verbose, - silent: options.silent, - }); + const config = await ConfigResolver.resolve( + { + configPath: options.configPath, + verbose: options.verbose, + silent: options.silent, + }, + { logger }, + ); const sources = config.getSources(); if (apmExists && sources.length > 0) { @@ -141,11 +144,14 @@ async function runGhInstall(logger: Logger, options: InstallCommandOptions): Pro // gh mode reads sources from `rulesync.jsonc`, never from `apm.yml`. The // disambiguation between rulesync/apm modes lives in `runRulesyncInstall`; // here the user has already opted into gh mode explicitly. - const config = await ConfigResolver.resolve({ - configPath: options.configPath, - verbose: options.verbose, - silent: options.silent, - }); + const config = await ConfigResolver.resolve( + { + configPath: options.configPath, + verbose: options.verbose, + silent: options.silent, + }, + { logger }, + ); const sources = config.getSources(); if (sources.length === 0) { diff --git a/src/cli/index.ts b/src/cli/index.ts index 9d033d653..231fc623d 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -81,6 +81,8 @@ const main = async () => { await gitignoreCommand(logger, { targets: resolvedTargets ? [...resolvedTargets] : undefined, features: cliFeatures, + verbose: (options as { verbose?: boolean }).verbose, + silent: (options as { silent?: boolean }).silent, }); }), ); diff --git a/src/cli/wrap-command.ts b/src/cli/wrap-command.ts index 81417d805..47bb7c1f8 100644 --- a/src/cli/wrap-command.ts +++ b/src/cli/wrap-command.ts @@ -2,7 +2,7 @@ import { Command } from "commander"; import { CLIError } from "../types/json-output.js"; import { formatError } from "../utils/error.js"; -import { ConsoleLogger, JsonLogger, Logger } from "../utils/logger.js"; +import { ConsoleLogger, fallbackLogger, JsonLogger, Logger } from "../utils/logger.js"; export function createLogger({ name, @@ -50,10 +50,15 @@ export function wrapCommand({ const positionalArgs = args.slice(0, -2); const globalOpts = command.parent?.opts() ?? {}; const logger = loggerFactory({ name, globalOpts, getVersion }); - logger.configure({ + // Configure from CLI flags first; commands that resolve a config file + // re-configure via `ConfigResolver.resolve` so config-file + // `verbose`/`silent` also apply (CLI flags still win there). + const cliLoggerOptions = { verbose: Boolean(globalOpts.verbose) || Boolean(options.verbose), silent: Boolean(globalOpts.silent) || Boolean(options.silent), - }); + }; + logger.configure(cliLoggerOptions); + fallbackLogger.configure(cliLoggerOptions); try { await handler(logger, options, globalOpts, positionalArgs); diff --git a/src/config/config-resolver.test.ts b/src/config/config-resolver.test.ts index 05acf6ece..e12b54a17 100644 --- a/src/config/config-resolver.test.ts +++ b/src/config/config-resolver.test.ts @@ -4,7 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { setupTestDirectory } from "../test-utils/test-directories.js"; import { writeFileContent } from "../utils/file.js"; -import type { Logger } from "../utils/logger.js"; +import { fallbackLogger, type Logger } from "../utils/logger.js"; import { ConfigResolver } from "./config-resolver.js"; const { getHomeDirectoryMock } = vi.hoisted(() => { @@ -155,6 +155,73 @@ describe("config-resolver", () => { expect(config.getSilent()).toBe(true); }); + + it("should re-configure the supplied logger from config-file silent/verbose", async () => { + const configContent = JSON.stringify({ + outputRoots: ["./"], + silent: true, + verbose: false, + }); + await writeFileContent(join(testDir, "rulesync.jsonc"), configContent); + const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger; + const fallbackConfigureSpy = vi.spyOn(fallbackLogger, "configure"); + + try { + await ConfigResolver.resolve({ configPath: join(testDir, "rulesync.jsonc") }, { logger }); + + expect(logger.configure).toHaveBeenCalledWith({ verbose: false, silent: true }); + expect(fallbackConfigureSpy).toHaveBeenCalledWith({ verbose: false, silent: true }); + } finally { + fallbackConfigureSpy.mockRestore(); + } + }); + + it("should re-configure the supplied logger with CLI flags winning over the config file", async () => { + const configContent = JSON.stringify({ + outputRoots: ["./"], + silent: false, + verbose: false, + }); + await writeFileContent(join(testDir, "rulesync.jsonc"), configContent); + const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger; + + await ConfigResolver.resolve( + { configPath: join(testDir, "rulesync.jsonc"), silent: true, verbose: true }, + { logger }, + ); + + expect(logger.configure).toHaveBeenCalledWith({ verbose: true, silent: true }); + }); + + it("should enable logger verbose from config-file verbose", async () => { + const configContent = JSON.stringify({ + outputRoots: ["./"], + verbose: true, + }); + await writeFileContent(join(testDir, "rulesync.jsonc"), configContent); + const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger; + + await ConfigResolver.resolve({ configPath: join(testDir, "rulesync.jsonc") }, { logger }); + + expect(logger.configure).toHaveBeenCalledWith({ verbose: true, silent: false }); + }); + + it("should not touch the fallbackLogger when no logger is supplied", async () => { + const configContent = JSON.stringify({ + outputRoots: ["./"], + silent: true, + }); + await writeFileContent(join(testDir, "rulesync.jsonc"), configContent); + const fallbackConfigureSpy = vi.spyOn(fallbackLogger, "configure"); + + try { + await ConfigResolver.resolve({ configPath: join(testDir, "rulesync.jsonc") }); + + expect(fallbackConfigureSpy).not.toHaveBeenCalled(); + } finally { + fallbackConfigureSpy.mockRestore(); + } + }); }); describe("config file targets (getConfigFileTargets)", () => { @@ -559,7 +626,7 @@ describe("config-resolver", () => { join(inputRoot, "rulesync.jsonc"), JSON.stringify({ outputRoots: ["./"], global: true }), ); - const logger = { warn: vi.fn() } as unknown as Logger; + const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger; await ConfigResolver.resolve( { @@ -572,13 +639,13 @@ describe("config-resolver", () => { expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Ignoring "global: true"')); }); - it("should fall back to console.warn when no logger is supplied", async () => { + it("should fall back to the shared fallbackLogger when no logger is supplied", async () => { const inputRoot = join(testDir, "central-rules"); await writeFileContent( join(inputRoot, "rulesync.jsonc"), JSON.stringify({ outputRoots: ["./"], global: true }), ); - const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const fallbackWarnSpy = vi.spyOn(fallbackLogger, "warn").mockImplementation(() => {}); try { await ConfigResolver.resolve({ @@ -586,11 +653,11 @@ describe("config-resolver", () => { inputRoot, }); - expect(consoleWarnSpy).toHaveBeenCalledWith( + expect(fallbackWarnSpy).toHaveBeenCalledWith( expect.stringContaining('Ignoring "global: true"'), ); } finally { - consoleWarnSpy.mockRestore(); + fallbackWarnSpy.mockRestore(); } }); @@ -604,7 +671,7 @@ describe("config-resolver", () => { join(testDir, "rulesync.jsonc"), JSON.stringify({ inputRoot: configuredRoot, global: true }), ); - const logger = { warn: vi.fn() } as unknown as Logger; + const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger; const config = await ConfigResolver.resolve( { @@ -623,7 +690,7 @@ describe("config-resolver", () => { join(inputRoot, "rulesync.jsonc"), JSON.stringify({ outputRoots: ["./"], global: true }), ); - const logger = { warn: vi.fn() } as unknown as Logger; + const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger; await ConfigResolver.resolve( { diff --git a/src/config/config-resolver.ts b/src/config/config-resolver.ts index 3ec122b1d..38f0a3d8c 100644 --- a/src/config/config-resolver.ts +++ b/src/config/config-resolver.ts @@ -19,7 +19,7 @@ import { resolvePath, validateOutputRoot, } from "../utils/file.js"; -import { type Logger, warnWithFallback } from "../utils/logger.js"; +import { fallbackLogger, type Logger, warnWithFallback } from "../utils/logger.js"; import { assertTargetsFeaturesExclusive, Config, @@ -289,6 +289,28 @@ export class ConfigResolver { // so the user knows where to look. assertMergedTargetsFeaturesExclusive({ configByFile, validatedConfigPath, localConfigPath }); + // Wire the resolved `verbose`/`silent` into the logger as soon as they are + // known, so config-file settings are honored by every message emitted from + // here on (including the warnings later in this resolution). CLI flags + // still win via `pick`. Only re-configure when the caller threaded a + // logger through — those call sites pass their CLI flags in the params, so + // precedence stays intact. The shared `fallbackLogger` is kept in sync for + // paths that have no logger threaded through. + const resolvedVerbose = pick({ + cli: verbose, + file: configByFile.verbose, + fallback: getDefaults().verbose, + }); + const resolvedSilent = pick({ + cli: silent, + file: configByFile.silent, + fallback: getDefaults().silent, + }); + if (logger !== undefined) { + logger.configure({ verbose: resolvedVerbose, silent: resolvedSilent }); + fallbackLogger.configure({ verbose: resolvedVerbose, silent: resolvedSilent }); + } + // When `inputRoot` is set (from CLI, programmatic args, or a config file) // the user is decoupling source from output, so "global: true" from the // config file must not apply unless the caller also explicitly passes @@ -317,7 +339,7 @@ export class ConfigResolver { const configParams = { targets: resolvedTargets, features: resolvedFeatures, - verbose: pick({ cli: verbose, file: configByFile.verbose, fallback: getDefaults().verbose }), + verbose: resolvedVerbose, delete: pick({ cli: isDelete, file: configByFile.delete, fallback: getDefaults().delete }), outputRoots: getOutputRootsInLightOfGlobal({ outputRoots: pick({ @@ -328,7 +350,7 @@ export class ConfigResolver { global: resolvedGlobal, }), global: resolvedGlobal, - silent: pick({ cli: silent, file: configByFile.silent, fallback: getDefaults().silent }), + silent: resolvedSilent, simulateCommands: pick({ cli: simulateCommands, file: configByFile.simulateCommands, diff --git a/src/features/mcp/codexcli-mcp.test.ts b/src/features/mcp/codexcli-mcp.test.ts index e9d5c2a61..6e31a4edb 100644 --- a/src/features/mcp/codexcli-mcp.test.ts +++ b/src/features/mcp/codexcli-mcp.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { RULESYNC_RELATIVE_DIR_PATH } from "../../constants/rulesync-paths.js"; import { setupTestDirectory } from "../../test-utils/test-directories.js"; import { ensureDir, writeFileContent } from "../../utils/file.js"; +import { fallbackLogger } from "../../utils/logger.js"; import { CodexcliMcp } from "./codexcli-mcp.js"; import { RulesyncMcp } from "./rulesync-mcp.js"; @@ -341,7 +342,7 @@ args = ["server.js"] }); it("should normalize invalid Codex MCP server names and warn when the last collision wins", async () => { - const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const warnSpy = vi.spyOn(fallbackLogger, "warn").mockImplementation(() => {}); const rulesyncMcp = new RulesyncMcp({ relativeDirPath: RULESYNC_RELATIVE_DIR_PATH, relativeFilePath: ".mcp.json", @@ -371,7 +372,7 @@ args = ["server.js"] }); it("should keep non-representable server names via a stable hash fallback instead of dropping them", async () => { - const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const warnSpy = vi.spyOn(fallbackLogger, "warn").mockImplementation(() => {}); const rulesyncMcp = new RulesyncMcp({ relativeDirPath: RULESYNC_RELATIVE_DIR_PATH, relativeFilePath: ".mcp.json", diff --git a/src/features/permissions/augmentcode-permissions.ts b/src/features/permissions/augmentcode-permissions.ts index 3801f82dc..0178a60ce 100644 --- a/src/features/permissions/augmentcode-permissions.ts +++ b/src/features/permissions/augmentcode-permissions.ts @@ -11,7 +11,7 @@ import type { PermissionAction, PermissionsConfig } from "../../types/permission import { readAugmentcodeSettingsWithLocalOverlay } from "../../utils/augmentcode-settings.js"; import { formatError } from "../../utils/error.js"; import { readFileContentOrNull } from "../../utils/file.js"; -import { ConsoleLogger, type Logger } from "../../utils/logger.js"; +import { fallbackLogger, type Logger } from "../../utils/logger.js"; import { applySharedConfigPatch, sharedConfigFileKey } from "../shared/shared-config-gateway.js"; import { RulesyncPermissions } from "./rulesync-permissions.js"; import { @@ -22,10 +22,11 @@ import { type ToolPermissionsSettablePaths, } from "./tool-permissions.js"; -// Module-level logger used by the importing direction (toRulesyncPermissions), +// Shared fallback logger used by the importing direction (toRulesyncPermissions), // where the instance method has no `logger` parameter. Mirrors the Qwen -// permissions translator's pattern. -const moduleLogger: Logger = new ConsoleLogger(); +// permissions translator's pattern; `fallbackLogger` is configured from CLI +// flags and the resolved config, so `silent` is honored. +const moduleLogger: Logger = fallbackLogger; /** * AugmentCode CLI uses `.augment/settings.json` (project) or `~/.augment/settings.json` (global). diff --git a/src/features/permissions/qwencode-permissions.ts b/src/features/permissions/qwencode-permissions.ts index 1c5196abf..05ce1b13e 100644 --- a/src/features/permissions/qwencode-permissions.ts +++ b/src/features/permissions/qwencode-permissions.ts @@ -8,7 +8,7 @@ import type { AiFileParams, ValidationResult } from "../../types/ai-file.js"; import type { PermissionAction, PermissionsConfig } from "../../types/permissions.js"; import { formatError } from "../../utils/error.js"; import { readFileContentOrNull } from "../../utils/file.js"; -import { ConsoleLogger, type Logger } from "../../utils/logger.js"; +import { fallbackLogger, type Logger } from "../../utils/logger.js"; import { applySharedConfigPatch, sharedConfigFileKey } from "../shared/shared-config-gateway.js"; import { RulesyncPermissions } from "./rulesync-permissions.js"; import { @@ -37,10 +37,11 @@ const QwenSettingsSchema = z.looseObject({ type QwenSettings = z.infer; -// Module-level logger used by the importing direction (toRulesyncPermissions), where the +// Shared fallback logger used by the importing direction (toRulesyncPermissions), where the // instance method has no `logger` parameter. The exporting direction (fromRulesyncPermissions) -// forwards the caller-supplied logger explicitly. -const moduleLogger: Logger = new ConsoleLogger(); +// forwards the caller-supplied logger explicitly. Unlike a private ConsoleLogger instance, +// `fallbackLogger` is configured from CLI flags and the resolved config, so `silent` is honored. +const moduleLogger: Logger = fallbackLogger; /** * Mapping from rulesync canonical tool category names (lowercase) to Qwen Code tool names (PascalCase). diff --git a/src/utils/logger.test.ts b/src/utils/logger.test.ts index 3f1c56a9d..fcd0b5dd1 100644 --- a/src/utils/logger.test.ts +++ b/src/utils/logger.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { Logger } from "./logger.js"; -import { ConsoleLogger, JsonLogger } from "./logger.js"; +import { ConsoleLogger, fallbackLogger, JsonLogger, warnWithFallback } from "./logger.js"; // Mock vitest module vi.mock("./vitest.js", () => ({ @@ -401,3 +401,45 @@ describe("JsonLogger", () => { }); }); }); + +describe("warnWithFallback", () => { + it("routes to the supplied logger when one is given", () => { + const logger = { warn: vi.fn() } as unknown as Logger; + const fallbackWarnSpy = vi.spyOn(fallbackLogger, "warn"); + + try { + warnWithFallback(logger, "message via logger"); + + expect(logger.warn).toHaveBeenCalledWith("message via logger"); + expect(fallbackWarnSpy).not.toHaveBeenCalled(); + } finally { + fallbackWarnSpy.mockRestore(); + } + }); + + it("routes to the shared fallbackLogger when no logger is given", () => { + const fallbackWarnSpy = vi.spyOn(fallbackLogger, "warn").mockImplementation(() => {}); + + try { + warnWithFallback(undefined, "message via fallback"); + + expect(fallbackWarnSpy).toHaveBeenCalledWith("message via fallback"); + } finally { + fallbackWarnSpy.mockRestore(); + } + }); + + it("fallbackLogger honors silent configuration", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + try { + fallbackLogger.configure({ verbose: false, silent: true }); + warnWithFallback(undefined, "suppressed message"); + + expect(warnSpy).not.toHaveBeenCalled(); + } finally { + fallbackLogger.configure({ verbose: false, silent: false }); + warnSpy.mockRestore(); + } + }); +}); diff --git a/src/utils/logger.ts b/src/utils/logger.ts index 40b439b11..0c2914a58 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -226,17 +226,21 @@ export class JsonLogger extends BaseLogger implements Logger { } } +/** + * Shared fallback logger for code paths that have no command logger threaded + * through (module-level translators, `warnWithFallback(undefined, ...)`). + * `wrapCommand` configures it from CLI flags and `ConfigResolver.resolve` + * re-configures it from the resolved config, so `silent`/`verbose` settings + * are honored even on paths where the command logger is not available. + */ +export const fallbackLogger: Logger = new ConsoleLogger(); + /** * Emit a warning through `logger.warn` if a logger is supplied, otherwise - * fall through to `console.warn`. Centralizes the "logger may be optional" - * pattern so call sites stay terse and `oxlint`'s `no-console` exception - * lives in exactly one place. + * fall through to the shared `fallbackLogger`. Centralizes the "logger may + * be optional" pattern so call sites stay terse and the fallback honors the + * configured `silent` mode. */ export function warnWithFallback(logger: Logger | undefined, message: string): void { - if (logger) { - logger.warn(message); - return; - } - // oxlint-disable-next-line no-console - console.warn(message); + (logger ?? fallbackLogger).warn(message); } From 9fa06cb9a0dd6d65a3d80004abefd411e345a444 Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Sun, 19 Jul 2026 09:37:35 -0700 Subject: [PATCH 2/2] refactor: emit conflicting-flags warning once at CLI parsing time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings: BaseLogger.configure no longer warns on verbose+silent — with the logger now re-configured from resolved config values, the warning fired up to four times, leaked raw console.warn into --json mode via the fallbackLogger, and mislabeled a config-file silent as a CLI flag conflict. The warning moved to warnOnConflictingFlags, called once in wrapCommand with JSON-mode suppression. Silent still always wins over verbose regardless of source. Also restore fallbackLogger state in tests and document the process-global caveat. Co-Authored-By: Claude Fable 5 --- src/cli/wrap-command.ts | 9 ++++- src/config/config-resolver.test.ts | 23 ++++++++---- src/config/config-resolver.ts | 5 ++- src/utils/logger.test.ts | 57 +++++++++++++++++++++++++++--- src/utils/logger.ts | 41 +++++++++++++-------- 5 files changed, 106 insertions(+), 29 deletions(-) diff --git a/src/cli/wrap-command.ts b/src/cli/wrap-command.ts index 47bb7c1f8..f3338ef74 100644 --- a/src/cli/wrap-command.ts +++ b/src/cli/wrap-command.ts @@ -2,7 +2,13 @@ import { Command } from "commander"; import { CLIError } from "../types/json-output.js"; import { formatError } from "../utils/error.js"; -import { ConsoleLogger, fallbackLogger, JsonLogger, Logger } from "../utils/logger.js"; +import { + ConsoleLogger, + fallbackLogger, + JsonLogger, + Logger, + warnOnConflictingFlags, +} from "../utils/logger.js"; export function createLogger({ name, @@ -57,6 +63,7 @@ export function wrapCommand({ verbose: Boolean(globalOpts.verbose) || Boolean(options.verbose), silent: Boolean(globalOpts.silent) || Boolean(options.silent), }; + warnOnConflictingFlags({ ...cliLoggerOptions, jsonMode: logger.jsonMode }); logger.configure(cliLoggerOptions); fallbackLogger.configure(cliLoggerOptions); diff --git a/src/config/config-resolver.test.ts b/src/config/config-resolver.test.ts index e12b54a17..c7cbbde0b 100644 --- a/src/config/config-resolver.test.ts +++ b/src/config/config-resolver.test.ts @@ -173,6 +173,7 @@ describe("config-resolver", () => { expect(fallbackConfigureSpy).toHaveBeenCalledWith({ verbose: false, silent: true }); } finally { fallbackConfigureSpy.mockRestore(); + fallbackLogger.configure({ verbose: false, silent: false }); } }); @@ -185,12 +186,16 @@ describe("config-resolver", () => { await writeFileContent(join(testDir, "rulesync.jsonc"), configContent); const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger; - await ConfigResolver.resolve( - { configPath: join(testDir, "rulesync.jsonc"), silent: true, verbose: true }, - { logger }, - ); + try { + await ConfigResolver.resolve( + { configPath: join(testDir, "rulesync.jsonc"), silent: true, verbose: true }, + { logger }, + ); - expect(logger.configure).toHaveBeenCalledWith({ verbose: true, silent: true }); + expect(logger.configure).toHaveBeenCalledWith({ verbose: true, silent: true }); + } finally { + fallbackLogger.configure({ verbose: false, silent: false }); + } }); it("should enable logger verbose from config-file verbose", async () => { @@ -201,9 +206,13 @@ describe("config-resolver", () => { await writeFileContent(join(testDir, "rulesync.jsonc"), configContent); const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger; - await ConfigResolver.resolve({ configPath: join(testDir, "rulesync.jsonc") }, { logger }); + try { + await ConfigResolver.resolve({ configPath: join(testDir, "rulesync.jsonc") }, { logger }); - expect(logger.configure).toHaveBeenCalledWith({ verbose: true, silent: false }); + expect(logger.configure).toHaveBeenCalledWith({ verbose: true, silent: false }); + } finally { + fallbackLogger.configure({ verbose: false, silent: false }); + } }); it("should not touch the fallbackLogger when no logger is supplied", async () => { diff --git a/src/config/config-resolver.ts b/src/config/config-resolver.ts index 38f0a3d8c..40af63389 100644 --- a/src/config/config-resolver.ts +++ b/src/config/config-resolver.ts @@ -295,7 +295,10 @@ export class ConfigResolver { // still win via `pick`. Only re-configure when the caller threaded a // logger through — those call sites pass their CLI flags in the params, so // precedence stays intact. The shared `fallbackLogger` is kept in sync for - // paths that have no logger threaded through. + // paths that have no logger threaded through. Note: `fallbackLogger` is + // process-global state — do not call `resolve` with a logger from a + // long-lived process (e.g. the MCP server) where one repository's config + // would leak into unrelated later operations. const resolvedVerbose = pick({ cli: verbose, file: configByFile.verbose, diff --git a/src/utils/logger.test.ts b/src/utils/logger.test.ts index fcd0b5dd1..42bcbc146 100644 --- a/src/utils/logger.test.ts +++ b/src/utils/logger.test.ts @@ -1,7 +1,13 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { Logger } from "./logger.js"; -import { ConsoleLogger, fallbackLogger, JsonLogger, warnWithFallback } from "./logger.js"; +import { + ConsoleLogger, + fallbackLogger, + JsonLogger, + warnOnConflictingFlags, + warnWithFallback, +} from "./logger.js"; // Mock vitest module vi.mock("./vitest.js", () => ({ @@ -65,14 +71,14 @@ describe("ConsoleLogger", () => { }); describe("configure()", () => { - it("should warn when both verbose and silent are enabled", () => { + it("should not warn when both verbose and silent are enabled (warning lives in warnOnConflictingFlags)", () => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); logger.configure({ verbose: true, silent: true }); - expect(warnSpy).toHaveBeenCalledWith( - "Both --verbose and --silent specified; --silent takes precedence", - ); + expect(warnSpy).not.toHaveBeenCalled(); + expect(logger.silent).toBe(true); + expect(logger.verbose).toBe(false); warnSpy.mockRestore(); }); @@ -443,3 +449,44 @@ describe("warnWithFallback", () => { } }); }); + +describe("warnOnConflictingFlags", () => { + it("warns once when both flags are set outside JSON mode", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + try { + warnOnConflictingFlags({ verbose: true, silent: true, jsonMode: false }); + + expect(warnSpy).toHaveBeenCalledExactlyOnceWith( + "Both --verbose and --silent specified; --silent takes precedence", + ); + } finally { + warnSpy.mockRestore(); + } + }); + + it("does not warn in JSON mode", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + try { + warnOnConflictingFlags({ verbose: true, silent: true, jsonMode: true }); + + expect(warnSpy).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + } + }); + + it("does not warn when only one flag is set", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + try { + warnOnConflictingFlags({ verbose: true, silent: false, jsonMode: false }); + warnOnConflictingFlags({ verbose: false, silent: true, jsonMode: false }); + + expect(warnSpy).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + } + }); +}); diff --git a/src/utils/logger.ts b/src/utils/logger.ts index 0c2914a58..a30d39dbd 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -46,20 +46,15 @@ abstract class BaseLogger { return this._silent; } + // Silent always wins over verbose, regardless of where each value came + // from (CLI flag or config file). The user-facing warning about the + // conflicting CLI flags lives in `warnOnConflictingFlags`, emitted once at + // CLI-flag parsing time — not here, since `configure` may be called again + // with config-file-derived values. configure({ verbose, silent }: { verbose: boolean; silent: boolean }): void { - if (verbose && silent) { - this._silent = false; - if (!isEnvTest()) { - this.onConflictingFlags(); - } - } this._silent = silent; this._verbose = verbose && !silent; } - - protected onConflictingFlags(): void { - console.warn("Both --verbose and --silent specified; --silent takes precedence"); - } } /** @@ -141,11 +136,6 @@ export class JsonLogger extends BaseLogger implements Logger { this._version = version; } - // Suppress raw console.warn in JSON mode to avoid non-JSON text on stderr - protected override onConflictingFlags(): void { - // No-op: conflicting flags warning is silently ignored in JSON mode - } - get jsonMode(): boolean { return true; } @@ -226,6 +216,27 @@ export class JsonLogger extends BaseLogger implements Logger { } } +/** + * Warn once when both `--verbose` and `--silent` were passed on the command + * line. Called at CLI-flag parsing time only (`wrapCommand`), so re-configuring + * a logger from config-file values never re-triggers it. Suppressed in JSON + * mode to keep non-JSON text off stderr, matching the former JsonLogger + * behavior. + */ +export function warnOnConflictingFlags({ + verbose, + silent, + jsonMode, +}: { + verbose: boolean; + silent: boolean; + jsonMode: boolean; +}): void { + if (!verbose || !silent || jsonMode || isEnvTest()) return; + // oxlint-disable-next-line no-console + console.warn("Both --verbose and --silent specified; --silent takes precedence"); +} + /** * Shared fallback logger for code paths that have no command logger threaded * through (module-level translators, `warnWithFallback(undefined, ...)`).