This Codebase Smells! [2025-09-14]

[github-actions[bot]]

This Codebase Smells!

Welcome to another thrilling episode of “Why Is My Terminal Screaming?” Today’s guest: your repo. It’s got heart, ambition, and a charming habit of deleting real files like it’s cleaning its room by throwing everything out the window. Let’s fix the chaos before it starts converting your config.toml into abstract art. Buckle up. 💥🤡

Table of Contents

• Overzealous Revert Deletes Real Files
• Dry‑Run Logging Bug in Revert
• Verbose Logging Sent To Stderr
• Recursive Scans Dive Into node_modules
• mcp-remote@latest Is A Supply-Chain Footgun
• Concatenation Marker Docs Drift
• Fragile AGENTS.md Heuristics
• .gitignore Block Loses State
• Noisy Global Config Error Logging

Overzealous Revert Deletes Real Files

⚠️ Revert happily unlinks generic config.toml even without provenance.

• The revert cleaner has a hard-coded hitlist including the very generic config.toml. That’s a real file in many projects. You’re one ruler revert away from “fun”.
• The logic “if no .bak → must be generated” is a lovely fantasy. In the actual universe, it’s called data loss. 💣

Files

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

Code

• Deletion list 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',
    ];

• Open Hands targets root 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 written on apply, or
  • Have a .bak, or
  • Are referenced in a generated manifest recorded during apply.
• Gate config.toml deletion: remove only if it contains known OpenHands MCP sections or a .bak is present.
• Add --force-extra-cleanup to explicitly permit nuking generic files.

Dry‑Run Logging Bug in Revert

Dry-run logs show a function, not a prefix. Yes, really. 🤦

• You’re interpolating actionPrefix the function instead of its return value. The log reads like a JS stack trace had a baby with string coercion. Cute, not helpful.

Files

• src/core/revert-engine.ts

Code

• Function used without invocation:
  • Lines 397–405: https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L397-L405

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

Suggestions

• Replace ${actionPrefix} with ${actionPrefix(dryRun)} in both spots.
• Add a unit test for dry-run prefixes to avoid future facepalms.

Verbose Logging Sent To Stderr

“Info/verbose go to stdout” says the doc. “Nah,” says the code.

• logVerbose prints to stderr, contradicting your own comment. Your CI and users will love noisy stderr pretending to be warnings. 🔊

Files

• src/constants.ts

Code

• Mismatch between comment and implementation:
  • Lines 17–27: https://github.com/intellectronica/ruler/blob/main/src/constants.ts#L17-L27

    export function logVerbose(message: string, isVerbose: boolean): void {
      if (isVerbose) {
        console.error(`[ruler:verbose] ${message}`);
      }
    }
    /** ... 
     * - info/verbose go to stdout (user-visible progress)
     * - warn/error go to stderr (problems)
     */

Suggestions

• Send verbose to stdout (console.log) to match the documented contract.
• Add a minimal logger wrapper test that asserts streams and prefixes.

Recursive Scans Dive Into node_modules

Your recursive “find all .ruler dirs” joyride visits the entire tree except hidden dirs. “Entire tree” includes node_modules. 🤮

• This is saboteur-mode in big repos: slow, noisy, and absolutely unnecessary.
• You’re skipping only dot-dirs; everything else is party time.

Files

• src/core/FileSystemUtils.ts

Code

• Only dots are skipped; not node_modules, dist, etc.:
  • Lines 208–221: https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L208-L221

    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

• Introduce an ignore list: node_modules, .git, dist, build, out, .next, .turbo, etc.
• Add an optional --include-dirs / --exclude-dirs CLI flag for explicit control.
• Short-circuit depth when you find a .ruler unless --deep is set.

mcp-remote@latest Is A Supply-Chain Footgun

“latest” is for candy bars and regrets, not production config. 🍭🔓

• Transforming remote → stdio injects npx mcp-remote@latest — perfect for a surprise upgrade mid-run.

Files

• src/mcp/capabilities.ts

Code

• Hard-coded latest:
  • Lines 69–73: https://github.com/intellectronica/ruler/blob/main/src/mcp/capabilities.ts#L69-L73

    const transformedConfig = {
      command: 'npx',
      args: ['-y', 'mcp-remote@latest', config.url as string],
      ...
    };

Suggestions

• Pin versions via config or TOML (e.g., mcp_remote_version = "0.2.x").
• Allow override in ruler.toml; default to a known-good version.
• Add a warning if latest is used.

Concatenation Marker Docs Drift

Docs say the concatenation marker is --- Source: file ---. Code says it’s an HTML comment. Pick a side. 🧻

Files

• README.md
• src/core/RuleProcessor.ts

Code

• README promises triple-dash marker:
  • Line 132: https://github.com/intellectronica/ruler/blob/main/README.md#L132

    - **Concatenation Marker**: Each file's content is prepended with `--- Source: <relative_path_to_md_file> ---` for traceability

• Code actually uses HTML comments:
  • Lines 16–23: https://github.com/intellectronica/ruler/blob/main/src/core/RuleProcessor.ts#L16-L23

    // ... HTML comment with source ...
    return [
      '',
      '',
      `<!-- Source: ${normalizedRel} -->`,
      '',
      content.trim(),
      '',
    ].join('\n');

Suggestions

• Either update the README to reflect HTML comments or switch the code to triple-dash markers consistently.
• Consider a provenance toggle in config for teams that want visible markdown markers.

Fragile AGENTS.md Heuristics

Your “is this generated?” test for root AGENTS.md is guessy and can misclassify legit files. 🔮

• You check for “Generated by Ruler” or presence of “”. If users add that manually or via docs, you’ll incorrectly skip their file.

Files

• src/core/FileSystemUtils.ts

Code

• Heuristic detection:
  • Lines 128–141: https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L128-L141

    const isGenerated = content.startsWith('<!-- Generated by Ruler -->');
    ...
    const containsRulerSources =
      content.includes('<!-- Source: .ruler/') ||
      content.includes('<!-- Source: ruler/');
    const isProbablyGenerated =
      isGenerated || (containsRulerSources && hasRulerFiles);
    
    if (!isProbablyGenerated || !hasRulerFiles) {
      ordered = [{ path: rootAgentsPath, content }, ...ordered];
    }

Suggestions

• Write a signed provenance marker (e.g., <!-- Ruler: {hash-of-sources} -->) that you can verify deterministically.
• Record a manifest on apply so revert/merge logic uses facts, not vibes.

.gitignore Block Loses State

The Ruler block is regenerated with only “new paths” from this run, silently dropping previously managed paths not present this time. Prepare for flip-flopping ignored files. 🧠🪣

• You filter out paths already present outside the block, then replace the first Ruler block entirely with just the new ones.
• If you apply a subset of agents later, older ignored files vanish from the Ruler block, potentially becoming tracked again.

Files

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

Code

• Replacement behavior uses only “new” entries:
  • Lines 62–70: https://github.com/intellectronica/ruler/blob/main/src/core/GitignoreUtils.ts#L62-L70

    const existingPaths = getExistingPathsExcludingRulerBlock(existingContent);
    const newPaths = relativePaths.filter((p) => !existingPaths.includes(p));
    const allRulerPaths = [...new Set(newPaths)].sort();

• Update called from apply:
  • Lines 744–753: https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L744-L753

    if (dryRun) {
      logInfo(`Would update .gitignore ...`, dryRun);
    } else {
      await updateGitignoreUtil(projectRoot, uniquePaths);
      logInfo(`Updated .gitignore ...`, dryRun);
    }

Suggestions

• Maintain a manifest of generated outputs per apply invocation and merge that into the Ruler block; don’t drop previous managed entries unless explicitly removed.
• Or persist the full set of known generated paths under .ruler/.manifest.json and use it to reconcile.

Noisy Global Config Error Logging

An ignorable “can’t stat global config path” becomes a stderr error log. Why are we yelling? 📣

• Checking the global config dir is routine; not finding it isn’t an error, it’s expected.

Files

• src/core/FileSystemUtils.ts

Code

• Loud error on ENOENT:
  • Lines 49–52: https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L49-L52

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

Suggestions

• Only warn on non-ENOENT errors; otherwise silently continue or use logVerbose.
• Add tests for both ENOENT and genuine error paths.

---

If you can only fix a few things this week, prioritize:

• Protect config.toml from accidental deletion.
• Fix the dry-run logging bug.
• Stop scanning node_modules.
• Pin mcp-remote versions.

You’ll save users’ files, their time, your CI, and at least three keyboards. 💾🧯