This Codebase Smells! [2025-11-02]

[github-actions[bot]]

This Codebase Smells!

Welcome to another week of “what fresh chaos is this?” The codebase is functional, ambitious, and sprinkled with “who hurt you?” energy. I laughed, I cried, I wrote this review. Buckle up, buttercups. 💥🧯

Table of Contents

• Revert Nukes Real config.toml (OpenHands)
• Dry‑Run Prefix Bug: Using a Function Like a String
• Codex Config: JSON vs TOML Schizophrenia
• Verbose Logs Go To Stderr (Your Own Comment Says Don’t)
• Gitignore Adds Leading Slashes → Duplicate, Noisy Entries
• Unified Config “version” Is Hardcoded Make‑Believe
• Global Config Check Spams console.error For No Reason
• Revert Can Delete .gitignore Entirely
• Type Mismatch: yargs v18 With @types/yargs v17

Revert Nukes Real config.toml (OpenHands)

☢️ Revert happily deletes a legit project’s config.toml without evidence it created it.

• The revert cleanup hard-codes config.toml for deletion. If you tried OpenHands or just… have a config.toml (like a normal project), boom—gone if no .bak. This is not “clean up,” this is “oops, production.” 💣

Files

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

Code

• Hard-coded deletion list:
  • 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:
  • 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'));

Recommendations

• Never delete generic files unless: file has a Ruler provenance marker, a corresponding .bak exists, or it’s listed in a recorded manifest.
• Gate config.toml removal behind “contains OpenHands MCP sections” or only restore when .bak exists; otherwise warn and skip.
• Add a --force-extra-cleanup flag for “generic” deletions and log a red, screaming warning in dry-run.

Dry‑Run Prefix Bug: Using a Function Like a String

🤦 The log uses actionPrefix as if it were a string, not a function call. Spoiler: it prints “[object Function]”.

• This breaks consistency and looks ridiculous in output, especially during dry runs. You wrote the helper. Call it. Please. 🙃

Files

• src/core/revert-engine.ts

Code

• Wrong usage of actionPrefix:
  • https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L361-L369

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

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

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

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

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

Recommendations

• Replace ${actionPrefix} with ${actionPrefix(dryRun)} everywhere.
• Add a tiny test to catch literal “[object Function]” in logs. You’ll thank me later. Or not. I don’t care.

Codex Config: JSON vs TOML Schizophrenia

🧠 One path writes TOML, another reads JSON. Pick a personality.

• Codex agent defaults to .codex/config.toml, while MCP path resolver points to .codex/config.json. That’s a guaranteed “why is nothing updating?” spiral.

Files

• src/paths/mcp.ts
• src/agents/CodexCliAgent.ts

Code

• MCP thinks Codex uses JSON:
  • https://github.com/intellectronica/ruler/blob/main/src/paths/mcp.ts#L27-L30

    case 'OpenAI Codex CLI':
      candidates.push(path.join(projectRoot, '.codex', 'config.json'));
      break;

• Agent writes TOML:
  • https://github.com/intellectronica/ruler/blob/main/src/agents/CodexCliAgent.ts#L147-L152

    return {
      instructions: path.join(projectRoot, DEFAULT_RULES_FILENAME),
      config: path.join(projectRoot, '.codex', 'config.toml'),
    };

Recommendations

• Standardize on TOML (as the agent does) and update getNativeMcpPath to return .codex/config.toml.
• Add a migration shim: if .json exists, warn and prefer .toml; optionally convert once.

Verbose Logs Go To Stderr (Your Own Comment Says Don’t)

📣 The comments say verbose logs go to stdout; you print them to stderr. Bold move.

• This breaks tooling that pipes stderr as “warnings/errors only,” flooding CI logs with “verbose” noise. My eyes. They burn. 🔥

Files

• src/constants.ts

Code

• You said verbose to stdout; then used console.error:
  • https://github.com/intellectronica/ruler/blob/main/src/constants.ts#L23-L37

    // - 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}`);
      }
    }

Recommendations

• Swap console.error -> console.log in logVerbose to honor your own contract.
• Add a tiny logger test to assert streams so this never backslides.

Gitignore Adds Leading Slashes → Duplicate, Noisy Entries

🧻 You normalize entries with a leading “/”, then try to dedupe against raw lines that may not have it. Guess what? Duplicates.

• This yields churn like “AGENTS.md” vs “/AGENTS.md” across blocks. Your diffs will look like spaghetti.

Files

• src/core/GitignoreUtils.ts

Code

• Forced leading slash:
  • https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L58-L60

    return p.startsWith('/') ? p : `/${p}`;

• Dedupe compares against raw existing lines (no normalization):
  • https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L66-L66

    const newPaths = relativePaths.filter((p) => !existingPaths.includes(p));

Recommendations

• Normalize both sides consistently (strip leading slash and backslashes before comparison).
• Only write .gitignore when content actually changes; avoid churn in CI diffs.

Unified Config “version” Is Hardcoded Make‑Believe

🧪 The unified config metadata pretends the version is “0.0.0-dev”. Why? Just… why.

• This sabotages diagnostics and support, and makes reproducibility a joke.

Files

• src/core/UnifiedConfigLoader.ts

Code

• Fake version:
  • https://github.com/intellectronica/ruler/blob/main/src/core/UnifiedConfigLoader.ts#L38-L39

    version: '0.0.0-dev',

Recommendations

• Read version from package.json at load time (cache it).
• Include head commit hash if available; your future self debugging CI will clap.

Global Config Check Spams console.error For No Reason

📣 Looking for a global .ruler dir? You log an error on any exception. For a “not found” path. Really?

• This creates noisy logs any time the global directory doesn’t exist (which is… normal). Users will assume doom. It’s just missing.

Files

• src/core/FileSystemUtils.ts

Code

• Loud ENOENT:
  • https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L49-L53

    console.error(
      `[ruler] Error checking global config directory ${globalConfigDir}:`,
      err,
    );

Recommendations

• Only log non-ENOENT errors. Missing directories should be silent or verbose-only.

Revert Can Delete .gitignore Entirely

🧨 If Ruler owned the entire .gitignore block and nothing else remains, you just delete the file. That’s… bold.

• This risks confusing pipelines that expect .gitignore to exist, and it’s surprising behavior for a “revert.”

Files

• src/revert.ts

Code

• Unlinking the whole file:
  • https://github.com/intellectronica/ruler/blob/main/src/revert.ts#L214-L221

    if (newContent.trim() === '') {
      await fs.unlink(gitignorePath);
      logVerbose(`${prefix} Removed empty .gitignore file`, verbose);
    } else {
      await fs.writeFile(gitignorePath, newContent);
    }

Recommendations

• Prefer leaving an empty .gitignore with a comment “Reverted by Ruler” to avoid surprise removals.
• Only unlink if user explicitly asks (e.g., --prune-empty-gitignore).

Type Mismatch: yargs v18 With @types/yargs v17

🧬 Your types and runtime versions are playing twister.

• Using @types/yargs@^17 with yargs@^18.0.0 is asking for subtle CLI type bugs. Also: yargs v18 isn’t widely used; v17 is common.

Files

• package.json

Code

• Mismatch:
  • https://github.com/intellectronica/ruler/blob/main/package.json#L46-L68

    "@types/yargs": "^17.0.34",
    ...
    "yargs": "^18.0.0",

Recommendations

• Align versions: either upgrade both to a known compatible pair (prefer stable v17.x) or drop @types if using yargs’ own types.
• Add a simple CLI compile check in CI to prevent regressions.

---

If you can’t find enough issues, it’s because you’ve already fixed most of the obvious landmines (good job), but the ones above are real footguns. Clean them up, and this project will go from “why is it yelling at me” to “hey, that’s surprisingly nice.” 🎯

Happy ship… reverting. Or applying. Whatever. Just stop deleting people’s config.toml. 🙏