This Codebase Smells! [2025-10-26]

[github-actions[bot]]

This Codebase Smells!

This week’s roast is sponsored by “works on my machine” vodka and “who needs guardrails” energy drinks. The repo is mostly solid, but the potholes are deep, the guardrails are foam, and the logging is yelling into stderr like an anxious toaster. Strap in. 🔥🧯

Table of Contents

• Overzealous Revert Deletes Real Files
• Dry‑Run Logging Bug in Revert
• Verbose Logging Contradiction
• Recursive Scans Dive Into node_modules
• Inconsistent Rule Loading
• Gitignore Block Duplication Risk
• Ambiguous Agent Config Mapping
• Silent MCP Parse Failures Hide Bugs

Overzealous Revert Deletes Real Files

💣 Revert considers config.toml “extra junk” and may delete it. What could possibly go wrong? 🤡

• The revert cleanup hard-codes config.toml as an “additional file” to remove. That’s a normal file in many repos (and is used by OpenHands here).
• Without a .bak, it unlinks the file outright. That’s a lottery ticket for accidental data loss. 🎰💥

Files

• src/core/revert-engine.ts
• src/paths/mcp.ts

Code

• Hard-coded “cleanup” list includes a generic config.toml:
  • src/core/revert-engine.ts:311-318
    https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L311-L318

    const additionalFiles = [
      '.gemini/settings.json',
      '.mcp.json',
      '.vscode/mcp.json',
      '.cursor/mcp.json',
      '.kilocode/mcp.json',
      'config.toml',
    ];

• OpenHands intentionally targets root config.toml:
  • src/paths/mcp.ts:33-36
    https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L33-L36

    case 'Open Hands':
      // For Open Hands, we target the main config file, not a separate mcp.json
      candidates.push(path.join(projectRoot, 'config.toml'));

Suggestions

• Only remove “additional” files if:
  • They have a Ruler provenance marker, or
  • They have a .bak, or
  • They’re listed in a manifest from apply-time.
• Remove config.toml from the generic list; gate removal behind: “contains known OpenHands MCP sections” AND “backup exists,” or require --force-extra-cleanup.
• Log a WARN and skip if provenance is uncertain; don’t be bold with users’ config files.

Dry‑Run Logging Bug in Revert

🪦 It prints ${actionPrefix} literally in logs instead of using the function. That’s not a prefix; that’s a cry for help.

• The code forgets to call actionPrefix(dryRun) in a few log lines and logs the bare function identifier, producing nonsense.
• This confuses users during dry runs and makes logs less reliable than a fortune cookie.

Files

• src/core/revert-engine.ts

Code

• Literal function name sneaks into logs instead of evaluated prefix:
  • src/core/revert-engine.ts:365-369
    https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L365-L369

    logVerbose(
      `${actionPrefix} Restored VSCode settings from backup`,
      verbose,
    );

  • src/core/revert-engine.ts:378-386
    https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L378-L386

    logVerbose(
      `${actionPrefix} Would remove empty VSCode settings file`,
      verbose,
    );
    ...
    logVerbose(
      `${actionPrefix} Would remove augment.advanced section from ${settingsPath}`,
      verbose,
    );

Suggestions

• Replace ${actionPrefix} with ${actionPrefix(dryRun)} across the file.
• Add a lightweight test that asserts dry-run prefixes appear in log messages.

Verbose Logging Contradiction

🪵 The docs say verbose logs go to stdout. The code screams to stderr. Pick a lane.

• The comment claims info/verbose → stdout, warn/error → stderr.
• logVerbose writes to stderr; logVerboseInfo writes to stdout. It’s a logging mullet: party in the front, chaos in the back. 💇

Files

• src/constants.ts

Code

• Sends verbose to stderr, contradicting the comment:
  • src/constants.ts:23-27,19-21
    https://github.com/intellectronica/ruler/blob/main/src/constants.ts#L23-L27

    // - info/verbose go to stdout (user-visible progress)
    // - warn/error go to stderr (problems)

    https://github.com/intellectronica/ruler/blob/main/src/constants.ts#L17-L21

    export function logVerbose(message: string, isVerbose: boolean): void {
      if (isVerbose) {
        console.error(`[ruler:verbose] ${message}`);
      }
    }

Suggestions

• Standardize: send verbose to stdout consistently (or update the comment to match reality).
• Add a single logging facade test to assert stream destinations for info/verbose/warn/error.

Recursive Scans Dive Into node_modules

🕳️ The “find all .ruler” scan descends into everything that isn’t hidden. Yes, including node_modules. Performance just left the chat. 🐌

• The downward tree walk only avoids directories that start with a dot.
• It happily spelunks node_modules, dist, build, vendor, etc. RIP CPU and developer patience.

Files

• src/core/FileSystemUtils.ts

Code

• Filters only hidden directories:
  • src/core/FileSystemUtils.ts:218-221
    https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L218-L221

    // Recursively search subdirectories (but skip hidden directories like .git)
    if (!entry.name.startsWith('.')) {
      await findRulerDirs(fullPath);
    }

Suggestions

• Add an ignore set: ['node_modules', 'dist', 'build', 'coverage', 'out', 'vendor', '.next', '.turbo'].
• Consider a max-depth and/or a filesystem boundary guard.
• Accept a CLI or config-specified ignore glob for large monorepos.

Inconsistent Rule Loading

🔀 Single-mode loader reads only top-level .md files; other paths use recursive reads. Which timeline am I in? 🌀

• UnifiedConfigLoader only lists top-level .md files.
• Elsewhere, rule files are read recursively (see readMarkdownFiles), leading to divergent behavior across code paths.

Files

• src/core/UnifiedConfigLoader.ts
• src/core/FileSystemUtils.ts

Code

• Non-recursive .md collection in unified loader:
  • src/core/UnifiedConfigLoader.ts:109-114
    https://github.com/intellectronica/ruler/blob/main/src/core/UnifiedConfigLoader.ts#L109-L114

    const dirEntries = await fs.readdir(meta.rulerDir, { withFileTypes: true });
    const mdFiles = dirEntries
      .filter((e) => e.isFile() && e.name.toLowerCase().endsWith('.md'))
      .map((e) => path.join(meta.rulerDir, e.name));

• Recursive reader used elsewhere:
  • src/core/FileSystemUtils.ts:142-153
    https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L142-L153

    } else if (entry.isFile() && entry.name.endsWith('.md')) {
      const content = await fs.readFile(fullPath, 'utf8');
      mdFiles.push({ path: fullPath, content });
    }

Suggestions

• Standardize on one rule loading approach (prefer the recursive helper).
• Add tests that assert nested .md files are included identically in both single and hierarchical modes.

Gitignore Block Duplication Risk

🧱 Only the first Ruler-managed block is replaced; others survive like gremlins after midnight.

• The update only replaces the first block it finds, leaving stale blocks behind if there were multiple (merge conflicts, manual edits, etc.).
• Over time, .gitignore turns into a haunted attic.

Files

• src/core/GitignoreUtils.ts

Code

• Replaces only the first block and leaves others:
  • src/core/GitignoreUtils.ts:113-121,127-137
    https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L113-L121

    let inFirstRulerBlock = false;
    let hasRulerBlock = false;
    let processedFirstBlock = false;
    ...
    if (trimmed === RULER_START_MARKER && !processedFirstBlock) {
      inFirstRulerBlock = true;
      hasRulerBlock = true;

    https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L127-L137

    if (trimmed === RULER_END_MARKER && inFirstRulerBlock) {
      inFirstRulerBlock = false;
      processedFirstBlock = true;
      newLines.push(line);
      continue;
    }
    if (!inFirstRulerBlock) {
      newLines.push(line);
    }

Suggestions

• Strip all existing Ruler blocks first, then write a single canonical block.
• Add a test that starts from a .gitignore containing multiple blocks and verifies only one remains.

Ambiguous Agent Config Mapping

🎭 Substring matching on display names is a bug generator that got a fake mustache and walked into production.

• mapRawAgentConfigs maps per-agent configs using identifier exact match OR display name substring match.
• Ambiguity = last-wins chaos. A key like “code” could map to multiple agents unpredictably.

Files

• src/core/config-utils.ts

Code

• Substring match on user-provided keys:
  • src/core/config-utils.ts:21-31
    https://github.com/intellectronica/ruler/blob/main/src/core/config-utils.ts#L21-L31

    const lowerKey = key.toLowerCase();
    for (const agent of agents) {
      const identifier = agent.getIdentifier();
      if (
        identifier === lowerKey ||
        agent.getName().toLowerCase().includes(lowerKey)
      ) {
        mappedConfigs[identifier] = cfg;
      }
    }

Suggestions

• Deprecate substring matching; accept only:
  • exact identifier matches, or
  • a strict alias map (explicit).
• If multiple agents match a substring, throw a clear error listing the conflicts.
• Add tests for collision scenarios.

Silent MCP Parse Failures Hide Bugs

🫥 readNativeMcp returns {} on any error, swallowing parse failures like a hungry black hole.

• Hiding parse errors makes debugging config issues a scavenger hunt in the dark. With bees.
• At minimum, surface a warning or log the parse error when verbose is enabled.

Files

• src/paths/mcp.ts

Code

• Swallows all errors and returns empty config:
  • src/paths/mcp.ts:75-80
    https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L75-L80

    try {
      const text = await fs.readFile(filePath, 'utf8');
      return JSON.parse(text) as Record<string, unknown>;
    } catch {
      return {};
    }

Suggestions

• Log a warning (or verbose message) with the error context and file path.
• Consider a “null on parse error” vs “empty object on missing file” distinction to make intent explicit.
• Add a unit test that feeds invalid JSON and asserts the warning is emitted.

---

If you address these, you’ll have fewer surprises, faster runs, saner logs, and fewer accidental “I deleted my config.toml, send help” Slack messages. Bless you. 🧼🧹