This Codebase Smells! [2025-08-31]

[github-actions[bot]]

This Codebase Smells!

Welcome back to another episode of “What’s That Odor?” where your code makes my IDE cry. Today’s special: a potpourri of risky deletes, duplicated logic, mixed TOML dialects, and logs that sound like a garage band. Strap in, buttercup — we’re refactoring your despair into delight. 💅🔥

Table of Contents

• Overzealous Revert Deletes Real Files
• Home-Directory MCP Paths Are Footguns
• Duplicated Agent Selection Logic
• Mixed TOML Libraries + Hand-Rolled Serializer
• Recursive Scans Dive Into node_modules
• Logging Is Inconsistent And Loud
• .gitignore Block Handling Is Brittle
• Directory Removal Race Conditions

Overzealous Revert Deletes Real Files

☠️ Revert includes a generic config.toml in “extra cleanup” and will nuke it without a backup.

• If config.toml exists and wasn’t created by Ruler (no .bak), it’s removed anyway — welcome to Data Loss Land™.
• This is particularly spicy given OpenHands targets exactly config.toml, but the revert’s “bonus delete” is agent‑agnostic. Brave. 🤦

Files

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

Code

• Hard-coded “extra files” includes 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 targets the project 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 “extra” files if:
  • They have a Ruler provenance marker in content, or
  • A sibling .bak exists, or
  • They’re listed in a recorded manifest from apply.
• Remove config.toml from the generic list; if you must, gate it by “contains known OpenHands MCP markers”.
• Add a --force-extra-cleanup flag and warn loudly before touching generic files.

Home-Directory MCP Paths Are Footguns

🚫 MCP targets sometimes point to $HOME, but writes are blocked if the result isn’t under the project root — net effect: silent “do nothing.”

• For Windsurf, getNativeMcpPath only proposes a homedir path; apply refuses to write outside the project, so… you wrote nothing. Clap clap.
• Cursor/OpenCode do similar juggling but at least include a project path candidate; Windsurf just vibes in ~/.codeium/... and gets ghosted.

Files

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

Code

• Windsurf only uses $HOME candidate:
  • Lines 24–27: https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L24-L27

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

• Apply refuses to write outside project:
  • Lines 591–598: https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L591-L598

    // Prevent writing MCP configs outside the project root
    if (!dest.startsWith(projectRoot)) {
      logVerbose(`Skipping MCP config ... outside project: ${dest}`, verbose);
      return;
    }

Suggestions

• Always include a project-local candidate for any agent (e.g., .windsurf/mcp.json) and prefer it when absent.
• If you intend to allow $HOME writes, gate behind --allow-home-mcp or TOML flag with big, neon warnings.
• Emit a user-facing warning when skipping an MCP write due to path being outside project.

Duplicated Agent Selection Logic

🪓 You implemented agent selection twice, in two places, with the same code. Why.

• selectAgentsToRun and resolveSelectedAgents duplicate logic, validation, and error wording — a lovely breeding ground for drift bugs.
• One change = two PRs = infinite facepalms.

Files

• src/core/apply-engine.ts
• src/core/agent-selection.ts

Code

• In apply-engine.ts:
  • Lines 226–305: https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L226-L305

    export function selectAgentsToRun(allAgents: IAgent[], config: LoadedConfig): IAgent[] {
      // CLI --agents > config.default_agents > per-agent.enabled flags > default all
      ...
    }

• In agent-selection.ts:
  • Lines 13–92: https://github.com/intellectronica/ruler/blob/main/src/core/agent-selection.ts#L13-L92

    export function resolveSelectedAgents(config: LoadedConfig, allAgents: IAgent[]): IAgent[] {
      // CLI --agents > default_agents > per-agent.enabled flags > default all
      ...
    }

Suggestions

• Keep only one selection function (prefer the dedicated core/agent-selection.ts).
• Replace internal use in apply-engine.ts with the exported helper and delete the duplicate.
• Add unit tests for edge-cases: invalid CLI names, overlapping substrings, per-agent overrides.

Mixed TOML Libraries + Hand-Rolled Serializer

🤹 You parse with toml and stringify with @iarna/toml, then manually glue strings in places. Choose one flavor. Please.

• Parsing: toml in multiple modules; stringifying: @iarna/toml.
• CodexCliAgent hand-writes TOML strings (arrays, inline tables, headers). That’s a vibe until someone adds a value with quotes and your TOML turns into modern art.

Files

• src/core/ConfigLoader.ts
• src/core/UnifiedConfigLoader.ts
• src/mcp/propagateOpenHandsMcp.ts
• src/agents/CodexCliAgent.ts

Code

• Mixed TOML toolchain and manual stringbuilding:
  • Parse and stringify: https://github.com/intellectronica/ruler/blob/main/src/mcp/propagateOpenHandsMcp.ts#L2-L3

    import * as TOML from 'toml';
    import { stringify } from '@iarna/toml';

  • Manual TOML generation in Codex:
    • Lines 140–148: https://github.com/intellectronica/ruler/blob/main/src/agents/CodexCliAgent.ts#L140-L148

      tomlContent += `\n[mcp_servers.${serverName}]\n`
      if (serverConfig.command) {
        tomlContent += `command = "${serverConfig.command}"\n`
      }

    • Lines 160–169: https://github.com/intellectronica/ruler/blob/main/src/agents/CodexCliAgent.ts#L160-L169

      if (serverConfig.args && Array.isArray(serverConfig.args)) {
        const argsStr = JSON.stringify(serverConfig.args).replace(/\"/g, '\"').replace(/,/g, ', ')
        tomlContent += `args = ${argsStr}\n`
      }

Suggestions

• Standardize on @iarna/toml for both parse and stringify across the repo.
• Build structured objects and call stringify — do not manually format TOML.
• Add tests with edge-case values (quotes, commas, unicode, empty arrays) to prevent regressions.

Recursive Scans Dive Into node_modules

🕳️ Your “find all .ruler directories” recurses into every folder not starting with a dot. That includes node_modules. For fun. For hours. For heat. 🔥

• You skip dot-folders (good), but not the usual landmines like node_modules, dist, build, etc.
• Results: I/O storm, wasted CPU, and one very tired fan.

Files

• src/core/FileSystemUtils.ts

Code

• Recursive search, only skipping dot-directories:
  • Lines 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

• Maintain an ignore set: node_modules, dist, build, coverage, .next, out, target, etc.
• Accept an optional CLI or TOML ignore list for bizarre enterprise monorepos.
• Short-circuit on symlinks or mount points if you’re feeling spicy.

Logging Is Inconsistent And Loud

📣 Some messages use console.log, others console.warn, others a bespoke logVerbose to stderr. It’s a symphony of noise.

• Hierarchical mode spams stdout even when --verbose is off.
• “Verbose” sometimes goes to stderr, sometimes it doesn’t; dry-run prefixing is inconsistent across the codebase.

Files

• src/core/apply-engine.ts
• src/constants.ts

Code

• Unconditional logging of internal progress:
  • Lines 330–333: https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L330-L333

    console.log(`[ruler] Processing .ruler directory: ${config.rulerDir}`);

• Verbose uses stderr via console.error:
  • Lines 13–20: https://github.com/intellectronica/ruler/blob/main/src/constants.ts#L13-L20

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

Suggestions

• Centralize a logger with levels (info/warn/error/verbose) and consistent sink (stdout vs stderr) policy.
• Gate non-essential logs behind --verbose; reserve stdout for user‑visible outputs and summaries.
• Ensure all dry-run messages use actionPrefix and align on one style.

.gitignore Block Handling Is Brittle

🧱 Updating the Ruler block replaces only the first Ruler block and keeps any others. Because multiplying special blocks sounds like a great idea.

• Path normalization tries to strip “project basename” prefixes from relative paths. That’s… creative, and fragile across monorepos.
• Writes only .gitignore; doesn’t consider .git/info/exclude, which some teams rely on.

Files

• src/core/GitignoreUtils.ts

Code

• Ruler block markers:
  • Lines 4–5: https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L4-L5

    const RULER_START_MARKER = '# START Ruler Generated Files';
    const RULER_END_MARKER = '# END Ruler Generated Files';

• Replace only the first Ruler block; leave others untouched:
  • Lines 117–125, 127–136: https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L117-L136

    if (trimmed === RULER_START_MARKER && !processedFirstBlock) {
      inFirstRulerBlock = true;
      ...
      rulerPaths.forEach((p) => newLines.push(p));
    }
    ...
    if (!inFirstRulerBlock) {
      newLines.push(line);
    }

• “Remove project basename” trick:
  • Lines 41–47: https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L41-L47

    const projectBasename = path.basename(normalizedProjectRoot);
    if (normalizedPath.StartsWith(projectBasename + path.sep)) {
      relative = normalizedPath.substring(projectBasename.length + 1);
    }

Suggestions

• Collapse all existing Ruler blocks into one canonical block.
• Base path normalization strictly on path.relative(projectRoot, p) for both absolute and relative inputs — no basename hacks.
• Optionally support writing to .git/info/exclude via flag to avoid polluting repo files.

Directory Removal Race Conditions

🧨 You check a directory tree is empty, then call fs.rm(..., { recursive: true }). If files appear between checks, you can delete content you didn’t intend to.

• The log says “Removed empty directory tree” but the OS does not care about your log messages.
• It’s a tiny window, but that’s how “whoops” moments are born.

Files

• src/core/revert-engine.ts

Code

• “Empty” check followed by recursive removal:
  • Lines 591–599 (see behavior in executeDirectoryAction via callers): https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L220-L265 and https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L164-L175

    await fs.rm(dirPath, { recursive: true });
    logVerbose(`${prefix} Removed empty directory...`, verbose);

Suggestions

• Use fs.rmdir without recursive for the final remove; if it fails, directory isn’t empty, so bail.
• Alternatively, remove children bottom-up (non-recursive final call) to avoid TOCTOU footguns.
• Unit test the “file appears mid-cleanup” scenario to prove safety.

---

If you made it this far without crying: congratulations, you’re ready to ship fewer regrets. Make these fixes, and your codebase will smell less like a burning data center and more like a freshly linted monorepo. 💐