This Codebase Smells! [2025-09-07]

[github-actions[bot]]

This Codebase Smells!

Welcome back to another dramatic episode of “What Fresh Tech Debt Is This?” 🎭 The repo is mostly solid, but the few warts it has are… artisanally crafted. We’ve got delete-happy reverts, logging that screams the wrong way, a directory scanner that bravely spelunks into Mordor, and a .gitignore surgeon who only stitches the first incision and leaves the rest gaping. Buckle up. 💥

Table of Contents

• Overzealous Revert Deletes Real Files
• Dry‑Run Prefix Bug Prints Function Source 🤦
• Verbose Logging Sends To Stderr (Contradicting Its Own Doc)
• Recursive .ruler Scan Dives Into node_modules
• .gitignore Block Replacement Leaves Duplicates
• MCP Reader Swallows Parse Errors And Misreads Non‑JSON
• CLI Exits The Process; Please Stop Killing The Host App
• Agent Name Matching For MCP Paths Is Fragile

Overzealous Revert Deletes Real Files

☠️ Revert happily targets config.toml, which can be a legit project file, not Ruler’s plaything.

• The “additional files” purge literally includes plain config.toml. This is not a niche filename — it’s ubiquitous. Removing it when there’s no .bak is a footgun.
• OpenHands uses config.toml by design, but this blanket list doesn’t check provenance. Accidental data loss after a trial run? Chef’s kiss of chaos. 💣

Files

• src/core/revert-engine.ts
• src/paths/mcp.ts
• src/core/apply-engine.ts (OpenHands TOML propagation)

Code

• Hard-coded deletion list:
  • 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 exactly config.toml:
  • Lines 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 delete files that:
  • Have a Ruler provenance marker (write one when created).
  • Or have a .bak right next to them.
  • Or are enumerated in an “apply manifest” captured earlier.
• Gate config.toml removal behind: “contains OpenHands MCP section(s) we wrote” or a .bak.
• Add a --force-extra-cleanup flag for risky deletions; otherwise log a warning and skip.

Dry‑Run Prefix Bug Prints Function Source 🤦

🗯️ In dry-run cleanup of VSCode settings, you log ${actionPrefix} (the function) instead of the computed prefix string. Enjoy your log of “function actionPrefix(dry: boolean) { … }”.

Files

• src/core/revert-engine.ts

Code

• Wrong interpolation (uses function instead of call):
  • Lines 366–369, 379–385, 399–406: https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L366-L369

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

  • https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L379-L385

    logVerbose(
      `${actionPrefix} Would remove empty VSCode settings file`,
      verbose,
    );

  • https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L399-L406

    logVerbose(
      `${actionPrefix} Removed empty VSCode settings file`,
      verbose,
    );

Suggestions

• Replace ${actionPrefix} with the local prefix variable or actionPrefix(dryRun).
• Add a unit test asserting the exact dry-run prefix appears in revert logs.

Verbose Logging Sends To Stderr (Contradicting Its Own Doc)

📣 The comment promises “info/verbose go to stdout”… then logVerbose sends to stderr. Pick a lane.

Files

• src/constants.ts

Code

• The doc vs. implementation mismatch:
  • Lines 23–26 vs. 17–20: https://github.com/intellectronica/ruler/blob/main/src/constants.ts#L23-L26

    // - info/verbose go to stdout (user-visible progress)

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

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

Suggestions

• Change logVerbose to console.log to match the contract.
• Add a small logging test suite to lock down streams and prefixes.

Recursive .ruler Scan Dives Into node_modules

🕳️ findAllRulerDirs recursively traverses the whole tree, skipping only “dot” directories. So yes, it will joyride through node_modules, vendor, dist, etc., looking for .ruler. Why.

Files

• src/core/FileSystemUtils.ts

Code

• Only skips names starting with .:
  • 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 ignore list: node_modules, dist, build, coverage, target, vendor, .next, .turbo, .yarn, etc.
• Or respect .gitignore during traversal; or provide a --max-depth for hierarchical search.
• Unit test performance by mocking fs and asserting skip behavior.

.gitignore Block Replacement Leaves Duplicates

🧵 You replace only the first “RULER block” and leave any older blocks intact. Congrats on layered duplication. Future archaeologists will thank you.

Files

• src/core/GitignoreUtils.ts

Code

• Block update logic only handles the first occurrence:
  • Lines 201–243: https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L201-L243

    if (trimmed === RULER_START_MARKER && !processedFirstBlock) {
      inFirstRulerBlock = true;
      hasRulerBlock = true;
      newLines.push(line);
      // Add the new Ruler paths
      rulerPaths.forEach((p) => newLines.push(p));
      continue;
    }
    // ...
    if (!inFirstRulerBlock) {
      newLines.push(line);
    }
    // Skip lines that are in the first Ruler block (they get replaced)

  • Markers:
    • Lines 1–6: https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L1-L6

Suggestions

• Strip all existing RULER blocks and write a single new block at the end.
• Add a test with multiple pre-existing blocks to assert idempotence.

MCP Reader Swallows Parse Errors And Misreads Non‑JSON

🙈 readNativeMcp uses JSON.parse and returns {} on error, silently. If a file is borked, you should at least whisper about it. Also: this will happily “parse” TOML targets if misrouted.

Files

• src/paths/mcp.ts
• src/core/apply-engine.ts (uses read/write helpers)

Code

• Silent fallback to empty object:
  • Lines 68–77: https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L68-L77

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

• Standard path uses the reader for merges:
  • Lines 598–612: https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L598-L612

Suggestions

• Log a verbose warning on parse failure with the file path.
• Detect format by extension; only JSON.parse for .json files.
• Add a thin schema validation for expected keys; fail fast on nonsense.

CLI Exits The Process; Please Stop Killing The Host App

🪓 process.exit(1) is fine for a stubby binary, but you’ve got nicely exportable handlers, and then you slam the door shut. Great for tests? Nope. Great for embedding? Double nope.

Files

• src/cli/handlers.ts

Code

• The exits:
  • Lines 81–85: https://github.com/intellectronica/ruler/blob/main/src/cli/handlers.ts#L81-L85
  • Lines 193–196: https://github.com/intellectronica/ruler/blob/main/src/cli/handlers.ts#L193-L196

    console.error(`${ERROR_PREFIX} ${message}`);
    process.exit(1);

Suggestions

• Throw and let the top-level bin catch-and-exit. Keep handlers pure.
• Return non-zero exit codes via the actual CLI entrypoint only.
• Add tests that call handlers and assert on thrown errors rather than abrupt exits.

Agent Name Matching For MCP Paths Is Fragile

🧩 getNativeMcpPath switches on display names (e.g., “GitHub Copilot” vs identifiers). If an agent renames itself for branding, poof — path resolution breaks.

Files

• src/paths/mcp.ts
• src/agents/* (source of names vs identifiers)

Code

• Name-based switch:
  • Lines 10–17, 33–36, 49–55: https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L10-L17

    switch (adapterName) {
      case 'GitHub Copilot':
        candidates.push(path.join(projectRoot, '.vscode', 'mcp.json'));
        break;
      // ...
      case 'Open Hands':
        candidates.push(path.join(projectRoot, 'config.toml'));
        break;
      // ...
      default:
        return null;
    }

Suggestions

• Switch by agent.getIdentifier() or centralize a single mapping between identifier → path candidates.
• Add a test verifying getNativeMcpPath stays stable with cosmetic name changes.

---

If you fix just three things, make it: the revert deletion list (stop nuking config.toml), the dry-run logging bug (no more function-to-string comedy), and the directory traversal (don’t spelunk node_modules). Your future self will send you cupcakes. Or, at least, fewer angry commit messages. 🎂🔥