Skip to content

Commit 03c8cc6

Browse files
dyoshikawadyoshikawa
authored
Merge pull request #2316 from dyoshikawa/resolve-issue-2312-config-silent
fix: honor config-file silent and verbose in logger configuration
2 parents 31c162a + 9fa06cb commit 03c8cc6

13 files changed

Lines changed: 309 additions & 69 deletions

‎src/cli/commands/convert.test.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ describe("convertCommand", () => {
8686

8787
expect(ConfigResolver.resolve).toHaveBeenCalledWith(
8888
expect.objectContaining({ targets: ["cursor", "claudecode"] }),
89+
{ logger: mockLogger },
8990
);
9091
});
9192
});
@@ -101,6 +102,7 @@ describe("convertCommand", () => {
101102
targets: ["cursor", "claudecode"],
102103
features: ["*"],
103104
}),
105+
{ logger: mockLogger },
104106
);
105107

106108
expect(RulesProcessor).toHaveBeenCalledWith(
@@ -125,6 +127,7 @@ describe("convertCommand", () => {
125127
expect.objectContaining({
126128
features: ["rules", "mcp"],
127129
}),
130+
{ logger: mockLogger },
128131
);
129132
});
130133

@@ -172,6 +175,7 @@ describe("convertCommand", () => {
172175

173176
expect(ConfigResolver.resolve).toHaveBeenCalledWith(
174177
expect.objectContaining({ global: true }),
178+
{ logger: mockLogger },
175179
);
176180
});
177181
});

‎src/cli/commands/convert.ts‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,14 @@ export async function convertCommand(logger: Logger, options: ConvertOptions): P
4444
// Pass both source and destinations as `targets` so per-target feature maps
4545
// in `rulesync.jsonc` are honored for every tool involved. Default features
4646
// to `*` so every feature that both tools support is attempted.
47-
const config = await ConfigResolver.resolve({
48-
...options,
49-
targets: [fromTool, ...toTools],
50-
features: options.features ?? ["*"],
51-
});
47+
const config = await ConfigResolver.resolve(
48+
{
49+
...options,
50+
targets: [fromTool, ...toTools],
51+
features: options.features ?? ["*"],
52+
},
53+
{ logger },
54+
);
5255

5356
const isPreview = config.isPreviewMode();
5457
const modePrefix = isPreview ? "[DRY RUN] " : "";

‎src/cli/commands/gitignore.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,8 @@ const extractRulesyncManagedEntries = (content: string): string[] => {
166166
export type GitignoreCommandOptions = {
167167
readonly targets?: string[];
168168
readonly features?: RulesyncFeatures;
169+
readonly verbose?: boolean;
170+
readonly silent?: boolean;
169171
};
170172

171173
const groupEntriesByDestination = ({
@@ -211,7 +213,10 @@ export const gitignoreCommand = async (
211213
): Promise<void> => {
212214
const gitignorePath = join(process.cwd(), ".gitignore");
213215
const gitattributesPath = join(process.cwd(), ".gitattributes");
214-
const config = await ConfigResolver.resolve({});
216+
const config = await ConfigResolver.resolve(
217+
{ verbose: options?.verbose, silent: options?.silent },
218+
{ logger },
219+
);
215220

216221
const resolvedEntries = resolveGitignoreEntries({
217222
targets: options?.targets,

‎src/cli/commands/install.ts‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,14 @@ async function runRulesyncInstall(logger: Logger, options: InstallCommandOptions
4444
// `--mode apm` is required to opt into the APM layout.
4545
const apmExists = await apmManifestExists(projectRoot);
4646

47-
const config = await ConfigResolver.resolve({
48-
configPath: options.configPath,
49-
verbose: options.verbose,
50-
silent: options.silent,
51-
});
47+
const config = await ConfigResolver.resolve(
48+
{
49+
configPath: options.configPath,
50+
verbose: options.verbose,
51+
silent: options.silent,
52+
},
53+
{ logger },
54+
);
5255
const sources = config.getSources();
5356

5457
if (apmExists && sources.length > 0) {
@@ -141,11 +144,14 @@ async function runGhInstall(logger: Logger, options: InstallCommandOptions): Pro
141144
// gh mode reads sources from `rulesync.jsonc`, never from `apm.yml`. The
142145
// disambiguation between rulesync/apm modes lives in `runRulesyncInstall`;
143146
// here the user has already opted into gh mode explicitly.
144-
const config = await ConfigResolver.resolve({
145-
configPath: options.configPath,
146-
verbose: options.verbose,
147-
silent: options.silent,
148-
});
147+
const config = await ConfigResolver.resolve(
148+
{
149+
configPath: options.configPath,
150+
verbose: options.verbose,
151+
silent: options.silent,
152+
},
153+
{ logger },
154+
);
149155
const sources = config.getSources();
150156

151157
if (sources.length === 0) {

‎src/cli/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ const main = async () => {
8181
await gitignoreCommand(logger, {
8282
targets: resolvedTargets ? [...resolvedTargets] : undefined,
8383
features: cliFeatures,
84+
verbose: (options as { verbose?: boolean }).verbose,
85+
silent: (options as { silent?: boolean }).silent,
8486
});
8587
}),
8688
);

‎src/cli/wrap-command.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,13 @@ import { Command } from "commander";
22

33
import { CLIError } from "../types/json-output.js";
44
import { formatError } from "../utils/error.js";
5-
import { ConsoleLogger, JsonLogger, Logger } from "../utils/logger.js";
5+
import {
6+
ConsoleLogger,
7+
fallbackLogger,
8+
JsonLogger,
9+
Logger,
10+
warnOnConflictingFlags,
11+
} from "../utils/logger.js";
612

713
export function createLogger({
814
name,
@@ -50,10 +56,16 @@ export function wrapCommand({
5056
const positionalArgs = args.slice(0, -2);
5157
const globalOpts = command.parent?.opts() ?? {};
5258
const logger = loggerFactory({ name, globalOpts, getVersion });
53-
logger.configure({
59+
// Configure from CLI flags first; commands that resolve a config file
60+
// re-configure via `ConfigResolver.resolve` so config-file
61+
// `verbose`/`silent` also apply (CLI flags still win there).
62+
const cliLoggerOptions = {
5463
verbose: Boolean(globalOpts.verbose) || Boolean(options.verbose),
5564
silent: Boolean(globalOpts.silent) || Boolean(options.silent),
56-
});
65+
};
66+
warnOnConflictingFlags({ ...cliLoggerOptions, jsonMode: logger.jsonMode });
67+
logger.configure(cliLoggerOptions);
68+
fallbackLogger.configure(cliLoggerOptions);
5769

5870
try {
5971
await handler(logger, options, globalOpts, positionalArgs);

‎src/config/config-resolver.test.ts‎

Lines changed: 84 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
44

55
import { setupTestDirectory } from "../test-utils/test-directories.js";
66
import { writeFileContent } from "../utils/file.js";
7-
import type { Logger } from "../utils/logger.js";
7+
import { fallbackLogger, type Logger } from "../utils/logger.js";
88
import { ConfigResolver } from "./config-resolver.js";
99

1010
const { getHomeDirectoryMock } = vi.hoisted(() => {
@@ -155,6 +155,82 @@ describe("config-resolver", () => {
155155

156156
expect(config.getSilent()).toBe(true);
157157
});
158+
159+
it("should re-configure the supplied logger from config-file silent/verbose", async () => {
160+
const configContent = JSON.stringify({
161+
outputRoots: ["./"],
162+
silent: true,
163+
verbose: false,
164+
});
165+
await writeFileContent(join(testDir, "rulesync.jsonc"), configContent);
166+
const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger;
167+
const fallbackConfigureSpy = vi.spyOn(fallbackLogger, "configure");
168+
169+
try {
170+
await ConfigResolver.resolve({ configPath: join(testDir, "rulesync.jsonc") }, { logger });
171+
172+
expect(logger.configure).toHaveBeenCalledWith({ verbose: false, silent: true });
173+
expect(fallbackConfigureSpy).toHaveBeenCalledWith({ verbose: false, silent: true });
174+
} finally {
175+
fallbackConfigureSpy.mockRestore();
176+
fallbackLogger.configure({ verbose: false, silent: false });
177+
}
178+
});
179+
180+
it("should re-configure the supplied logger with CLI flags winning over the config file", async () => {
181+
const configContent = JSON.stringify({
182+
outputRoots: ["./"],
183+
silent: false,
184+
verbose: false,
185+
});
186+
await writeFileContent(join(testDir, "rulesync.jsonc"), configContent);
187+
const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger;
188+
189+
try {
190+
await ConfigResolver.resolve(
191+
{ configPath: join(testDir, "rulesync.jsonc"), silent: true, verbose: true },
192+
{ logger },
193+
);
194+
195+
expect(logger.configure).toHaveBeenCalledWith({ verbose: true, silent: true });
196+
} finally {
197+
fallbackLogger.configure({ verbose: false, silent: false });
198+
}
199+
});
200+
201+
it("should enable logger verbose from config-file verbose", async () => {
202+
const configContent = JSON.stringify({
203+
outputRoots: ["./"],
204+
verbose: true,
205+
});
206+
await writeFileContent(join(testDir, "rulesync.jsonc"), configContent);
207+
const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger;
208+
209+
try {
210+
await ConfigResolver.resolve({ configPath: join(testDir, "rulesync.jsonc") }, { logger });
211+
212+
expect(logger.configure).toHaveBeenCalledWith({ verbose: true, silent: false });
213+
} finally {
214+
fallbackLogger.configure({ verbose: false, silent: false });
215+
}
216+
});
217+
218+
it("should not touch the fallbackLogger when no logger is supplied", async () => {
219+
const configContent = JSON.stringify({
220+
outputRoots: ["./"],
221+
silent: true,
222+
});
223+
await writeFileContent(join(testDir, "rulesync.jsonc"), configContent);
224+
const fallbackConfigureSpy = vi.spyOn(fallbackLogger, "configure");
225+
226+
try {
227+
await ConfigResolver.resolve({ configPath: join(testDir, "rulesync.jsonc") });
228+
229+
expect(fallbackConfigureSpy).not.toHaveBeenCalled();
230+
} finally {
231+
fallbackConfigureSpy.mockRestore();
232+
}
233+
});
158234
});
159235

160236
describe("config file targets (getConfigFileTargets)", () => {
@@ -559,7 +635,7 @@ describe("config-resolver", () => {
559635
join(inputRoot, "rulesync.jsonc"),
560636
JSON.stringify({ outputRoots: ["./"], global: true }),
561637
);
562-
const logger = { warn: vi.fn() } as unknown as Logger;
638+
const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger;
563639

564640
await ConfigResolver.resolve(
565641
{
@@ -572,25 +648,25 @@ describe("config-resolver", () => {
572648
expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Ignoring "global: true"'));
573649
});
574650

575-
it("should fall back to console.warn when no logger is supplied", async () => {
651+
it("should fall back to the shared fallbackLogger when no logger is supplied", async () => {
576652
const inputRoot = join(testDir, "central-rules");
577653
await writeFileContent(
578654
join(inputRoot, "rulesync.jsonc"),
579655
JSON.stringify({ outputRoots: ["./"], global: true }),
580656
);
581-
const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
657+
const fallbackWarnSpy = vi.spyOn(fallbackLogger, "warn").mockImplementation(() => {});
582658

583659
try {
584660
await ConfigResolver.resolve({
585661
configPath: "rulesync.jsonc",
586662
inputRoot,
587663
});
588664

589-
expect(consoleWarnSpy).toHaveBeenCalledWith(
665+
expect(fallbackWarnSpy).toHaveBeenCalledWith(
590666
expect.stringContaining('Ignoring "global: true"'),
591667
);
592668
} finally {
593-
consoleWarnSpy.mockRestore();
669+
fallbackWarnSpy.mockRestore();
594670
}
595671
});
596672

@@ -604,7 +680,7 @@ describe("config-resolver", () => {
604680
join(testDir, "rulesync.jsonc"),
605681
JSON.stringify({ inputRoot: configuredRoot, global: true }),
606682
);
607-
const logger = { warn: vi.fn() } as unknown as Logger;
683+
const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger;
608684

609685
const config = await ConfigResolver.resolve(
610686
{
@@ -623,7 +699,7 @@ describe("config-resolver", () => {
623699
join(inputRoot, "rulesync.jsonc"),
624700
JSON.stringify({ outputRoots: ["./"], global: true }),
625701
);
626-
const logger = { warn: vi.fn() } as unknown as Logger;
702+
const logger = { warn: vi.fn(), configure: vi.fn() } as unknown as Logger;
627703

628704
await ConfigResolver.resolve(
629705
{

‎src/config/config-resolver.ts‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import {
1919
resolvePath,
2020
validateOutputRoot,
2121
} from "../utils/file.js";
22-
import { type Logger, warnWithFallback } from "../utils/logger.js";
22+
import { fallbackLogger, type Logger, warnWithFallback } from "../utils/logger.js";
2323
import {
2424
assertTargetsFeaturesExclusive,
2525
Config,
@@ -289,6 +289,31 @@ export class ConfigResolver {
289289
// so the user knows where to look.
290290
assertMergedTargetsFeaturesExclusive({ configByFile, validatedConfigPath, localConfigPath });
291291

292+
// Wire the resolved `verbose`/`silent` into the logger as soon as they are
293+
// known, so config-file settings are honored by every message emitted from
294+
// here on (including the warnings later in this resolution). CLI flags
295+
// still win via `pick`. Only re-configure when the caller threaded a
296+
// logger through — those call sites pass their CLI flags in the params, so
297+
// precedence stays intact. The shared `fallbackLogger` is kept in sync for
298+
// paths that have no logger threaded through. Note: `fallbackLogger` is
299+
// process-global state — do not call `resolve` with a logger from a
300+
// long-lived process (e.g. the MCP server) where one repository's config
301+
// would leak into unrelated later operations.
302+
const resolvedVerbose = pick({
303+
cli: verbose,
304+
file: configByFile.verbose,
305+
fallback: getDefaults().verbose,
306+
});
307+
const resolvedSilent = pick({
308+
cli: silent,
309+
file: configByFile.silent,
310+
fallback: getDefaults().silent,
311+
});
312+
if (logger !== undefined) {
313+
logger.configure({ verbose: resolvedVerbose, silent: resolvedSilent });
314+
fallbackLogger.configure({ verbose: resolvedVerbose, silent: resolvedSilent });
315+
}
316+
292317
// When `inputRoot` is set (from CLI, programmatic args, or a config file)
293318
// the user is decoupling source from output, so "global: true" from the
294319
// config file must not apply unless the caller also explicitly passes
@@ -317,7 +342,7 @@ export class ConfigResolver {
317342
const configParams = {
318343
targets: resolvedTargets,
319344
features: resolvedFeatures,
320-
verbose: pick({ cli: verbose, file: configByFile.verbose, fallback: getDefaults().verbose }),
345+
verbose: resolvedVerbose,
321346
delete: pick({ cli: isDelete, file: configByFile.delete, fallback: getDefaults().delete }),
322347
outputRoots: getOutputRootsInLightOfGlobal({
323348
outputRoots: pick({
@@ -328,7 +353,7 @@ export class ConfigResolver {
328353
global: resolvedGlobal,
329354
}),
330355
global: resolvedGlobal,
331-
silent: pick({ cli: silent, file: configByFile.silent, fallback: getDefaults().silent }),
356+
silent: resolvedSilent,
332357
simulateCommands: pick({
333358
cli: simulateCommands,
334359
file: configByFile.simulateCommands,

‎src/features/mcp/codexcli-mcp.test.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
55
import { RULESYNC_RELATIVE_DIR_PATH } from "../../constants/rulesync-paths.js";
66
import { setupTestDirectory } from "../../test-utils/test-directories.js";
77
import { ensureDir, writeFileContent } from "../../utils/file.js";
8+
import { fallbackLogger } from "../../utils/logger.js";
89
import { CodexcliMcp } from "./codexcli-mcp.js";
910
import { RulesyncMcp } from "./rulesync-mcp.js";
1011

@@ -341,7 +342,7 @@ args = ["server.js"]
341342
});
342343

343344
it("should normalize invalid Codex MCP server names and warn when the last collision wins", async () => {
344-
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
345+
const warnSpy = vi.spyOn(fallbackLogger, "warn").mockImplementation(() => {});
345346
const rulesyncMcp = new RulesyncMcp({
346347
relativeDirPath: RULESYNC_RELATIVE_DIR_PATH,
347348
relativeFilePath: ".mcp.json",
@@ -371,7 +372,7 @@ args = ["server.js"]
371372
});
372373

373374
it("should keep non-representable server names via a stable hash fallback instead of dropping them", async () => {
374-
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
375+
const warnSpy = vi.spyOn(fallbackLogger, "warn").mockImplementation(() => {});
375376
const rulesyncMcp = new RulesyncMcp({
376377
relativeDirPath: RULESYNC_RELATIVE_DIR_PATH,
377378
relativeFilePath: ".mcp.json",

0 commit comments

Comments
 (0)