This Codebase Smells! [2025-08-31]

[github-actions[bot]]

This Codebase Smells! A Weekly Report

Welcome back to another episode of “You shipped this? On purpose?” This repo is the software equivalent of an IKEA bookshelf held together by vibes, TODOs, and a strong belief in try/catch. It’s almost good — like a sandwich you dropped, picked up, and convinced yourself the five‑second rule applies. Let’s fix the spicy bits before they fix you. 💣🧯

Table of Contents

• Overzealous Revert Deletes Real Files
• Dry‑Run Logging Bug in Revert
• Recursive Scans Dive Into node_modules
• Fuzzy Agent Filters Invite Surprise
• Strict JSON Parser Can Clobber Existing MCP Config
• Two Config Loaders, Two Sources of Truth
• Home‑Directory MCP Paths Are Footguns (Mitigated)

Overzealous Revert Deletes Real Files

☠️ Your revert “cleanup” casually targets config.toml, which real projects often own. Bold strategy, Cotton.

• If there’s no .bak, you unlink it. That’s not “cleanup,” that’s “oops we deleted prod configs” energy. 🤦
• This is especially spicy because OpenHands uses a root config.toml. If Ruler ran in a mixed repo, kiss that file goodbye.

Files

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

Code

• Hard‑coded deletion list including generic config.toml:
  • Lines 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 MCP path is exactly config.toml:
  • Lines 38–41: https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L38-L41

    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 delete “additional” files when:
  • They have a Ruler provenance marker (write one when creating).
  • Or a .bak exists.
  • Or they are listed in a recorded manifest produced at apply-time.
• Gate config.toml removal behind OpenHands-specific TOML shape checks (known [mcp.*] keys) and/or backup presence.
• Add --force-extra-cleanup before deleting any generic filenames; default to warn-only.

Dry‑Run Logging Bug in Revert

🪓 Dry‑run logging tries to use actionPrefix but forgets it’s a function, not a string. Your logs say “undefined theater,” and I can’t even hate‑watch it.

• You correctly compute const prefix = actionPrefix(dryRun) earlier, then log with ${actionPrefix} in a few places. That prints the function reference instead of the prefix.

Files

• src/core/revert-engine.ts

Code

• Misuse of actionPrefix in string templates:
  • Lines 365–369, 378–386, 399–406: https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L365-L406

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

Suggestions

• Replace ${actionPrefix} with ${prefix} where the local variable exists, or call actionPrefix(dryRun) inline consistently.
• Add a unit test that asserts the exact log string in dry‑run mode to prevent regressions.

Recursive Scans Dive Into node_modules

🤿 Your .ruler search spelunks the entire tree, skipping only dotdirs. Congratulations, you just invented a CPU miner.

• findAllRulerDirs recurses into everything not starting with .. That includes node_modules, dist, build, vendor, and other performance graveyards.

Files

• src/core/FileSystemUtils.ts

Code

• Only skips directories that start with a dot:
  • Lines 205–223: https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L205-L223

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

Suggestions

• Add an explicit denylist: node_modules, dist, build, coverage, target, vendor, .venv, .cache, etc.
• Consider a max depth or early cutoffs for known package/distribution directories.
• Add a CLI flag --scan-all for the two people who actually want the current behavior.

Fuzzy Agent Filters Invite Surprise

🎯 “Substring includes” to pick agents? Fantastic way to accidentally run the wrong ones and then blame user error. Been there, done that, rolled back prod.

• CLI filters and defaults use includes() on agent names, which is ambiguous and brittle. One mistyped “co” runs Copilot, Codex, and probably your coffee machine.

Files

• src/core/apply-engine.ts

Code

• Fuzzy matching by substring:
  • Lines 257–262: https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L257-L262

    selected = allAgents.filter((agent) =>
      filters.some(
        (f) =>
          agent.getIdentifier() === f ||
          agent.getName().toLowerCase().includes(f),
      ),
    );

Suggestions

• Prefer exact matches against stable getIdentifier(); allow explicit wildcards as an opt-in (e.g. '*code*').
• If you keep fuzzy matching, present a disambiguation prompt or a dry‑run list before applying.
• Emit a warning whenever multiple agents match a single filter token.

Strict JSON Parser Can Clobber Existing MCP Config

🧨 readNativeMcp uses strict JSON.parse and silently returns {} on error — goodbye user config, hello “why did my MCP servers vanish?”

• You already implemented a lenient parser (strip comments/trailing commas) in UnifiedConfigLoader but didn’t reuse it here. Inconsistent and dangerous.

Files

• src/paths/mcp.ts
• src/core/UnifiedConfigLoader.ts

Code

• Strict parse with silent fallback to {}:
  • Lines 75–83: https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L75-L83

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

• Lenient parse logic exists elsewhere:
  • Lines 290–316 (excerpt): https://github.com/intellectronica/ruler/blob/main/src/core/UnifiedConfigLoader.ts#L290-L316

    const stripped = raw
      .replace(/\/\*[\\s\\S]*?\*\//g, '')
      .replace(/(^|\\s+)\/\/.*$/gm, '$1')
      .replace(/,\\s*([}\\]])/g, '$1');
    parsed = JSON.parse(stripped) as Record<string, unknown>;

Suggestions

• Share a single JSON parse helper that tolerates comments/trailing commas and use it in both places.
• When parsing fails, log a warning and back up the original file before writing a normalized version.
• Preserve unknown keys during merge: carry through base keys you don’t recognize to avoid data loss.

Two Config Loaders, Two Sources of Truth

🪞 You have ConfigLoader and UnifiedConfigLoader parsing TOML separately, with different validation and semantics. Pick a favorite child already.

• ConfigLoader uses zod validation and a specific structure; UnifiedConfigLoader parses raw TOML and assembles another view. Double the code paths; triple the ways to be inconsistent.

Files

• src/core/ConfigLoader.ts
• src/core/UnifiedConfigLoader.ts

Code

• Zod schema and strict loader:
  • Lines 30–45: https://github.com/intellectronica/ruler/blob/main/src/core/ConfigLoader.ts#L30-L45

    const rulerConfigSchema = z.object({
      default_agents: z.array(z.string()).optional(),
      agents: z.record(z.string(), agentConfigSchema).optional(),
      mcp: z.object({ ... }).optional(),
      gitignore: z.object({ enabled: z.boolean().optional() }).optional(),
      nested: z.boolean().optional(),
    });

• Independent TOML parsing and projection:
  • Lines 64–91: https://github.com/intellectronica/ruler/blob/main/src/core/UnifiedConfigLoader.ts#L64-L91

    if (tomlRaw && typeof tomlRaw === 'object' ... ) {
      defaultAgents = ( ... ).map((a) => String(a));
    }
    // ...
    const toml: TomlConfig = { raw: tomlRaw, schemaVersion: 1, agents: {}, defaultAgents, nested };

Suggestions

• Consolidate on a single loader; make the other consume its results instead of re‑parsing.
• Share schemas/types; generate types from zod where you need them to avoid drift.
• Add integration tests that compare both outputs for the same config and fail on divergence until removed.

Home‑Directory MCP Paths Are Footguns (Mitigated)

🥾 getNativeMcpPath still includes HOME‑based configs (e.g. Cursor, Windsurf). You correctly avoid writing outside the project root, but why keep tempting fate?

• The apply path guards against writing to HOME — good. But the candidate resolution still prefers or returns HOME paths for some adapters; future callers might misuse this.

Files

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

Code

• Home paths are still considered:
  • Lines 20–27, 33–33, 51–53: https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L20-L27

    case 'Cursor':
      candidates.push(path.join(projectRoot, '.cursor', 'mcp.json'));
      candidates.push(path.join(home, '.cursor', 'mcp.json'));
    case 'Windsurf':
      candidates.push(path.join(home, '.codeium', 'windsurf', 'mcp_config.json'));

• Guard prevents writing outside of project root:
  • Lines 591–598: https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L591-L598

    if (!dest.startsWith(projectRoot)) {
      logVerbose(`Skipping MCP config ... outside project: ${dest}`, verbose);
      return;
    }

Suggestions

• Remove HOME candidates entirely, or return them only when explicitly asked (e.g. --allow-home-mcp).
• If keeping them, surface a warning when a HOME path is chosen and always dry‑run unless --force.
• Add tests that assert no write attempts outside projectRoot for all adapters.

---

If you were hoping I’d run out of complaints: alas, I’m fueled by bad patterns and lukewarm coffee. But seriously — fix the deletions, tighten matching, stop spelunking node_modules, and unify config parsing. Do that, and this will go from “please don’t” to “yeah okay, ship it” real quick. 🚀