This Codebase Smells! [2026-05-24]

[github-actions[bot]]

This Codebase Smells!

This week’s bouquet is burnt config files, fermented boilerplate, and a top note of “it passed on my machine” 🤮.
I went spelunking through the repo so you don’t have to, and wow — this thing isn’t broken so much as aggressively committed to making future-you miserable 💩.

Table of Contents

• Revert Plays Russian Roulette with config.toml
• Nested Discovery Crawls the Repo Like a Feral Raccoon
• Broken Configs Get Silently “Handled”
• Your Quality Gates Are Wearing a Blindfold
• The Agent Zoo Is All Boilerplate, No Plan
• SkillsProcessor Is a Copy-Paste Landfill

Revert Plays Russian Roulette with config.toml

revert treats a generic root-level config.toml like disposable trash, which is the sort of confidence usually reserved for malware.

• Open Hands explicitly writes MCP config into the project’s root config.toml, and revert later unlinks config.toml if there’s no backup. That is wildly optimistic for a file name this generic.
• Translation: one unlucky repo with a legitimate config.toml gets “cleaned up” by your tool, and congratulations, you’ve invented config-based vandalism ☠️.

Files

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

Code

• Hard-coded cleanup list: src/core/revert-engine.ts#L311-L318

  const additionalFiles = [
    '.gemini/settings.json',
    '.mcp.json',
    '.vscode/mcp.json',
    '.cursor/mcp.json',
    '.junie/mcp/mcp.json',
    '.kilocode/mcp.json',
    'config.toml',
  ];

• Unconditional delete path: src/core/revert-engine.ts#L333-L347

  if (backupExists) {
    const restored = await restoreFromBackup(fullPath, verbose, dryRun);
  } else {
    await fs.unlink(fullPath);
  }

• Open Hands targets root config.toml: 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'));

Concrete suggestions

• Only delete config.toml when:
  1. a .bak exists, or
  2. the file contains a Ruler provenance marker / known Open Hands MCP section.
• Record created files during apply and use that manifest during revert.
• Treat generic filenames as restore-only, not delete-by-default.

Nested Discovery Crawls the Repo Like a Feral Raccoon

Nested mode recursively walks basically the whole tree, which is how you end up indexing fixtures, junk, and whatever other cursed debris lives under the repo.

• findAllRulerDirs() skips hidden directories and nested git repos, but not tests, tmp-fixtures, coverage, out, or any other landfill. That’s a performance problem and a correctness problem.
• In this repo, there are real fixture .ruler directories under test data, so nested discovery can absolutely hoover up things that are test scaffolding, not user config. Gorgeous. No notes. 🦝🔥

Files

• src/core/FileSystemUtils.ts
• tests/integration/fixtures/unified/.ruler/AGENTS.md
• tmp-fixtures/unified-basic/.ruler/AGENTS.md

Code

• Recursive walk with almost no ignore policy: src/core/FileSystemUtils.ts#L264-L300

  if (entry.isDirectory()) {
    if (entry.name === '.ruler') {
      rulerDirs.push(fullPath);
    } else {
      if (!entry.name.startsWith('.')) {
        await findRulerDirs(fullPath);
      }
    }
  }

• Test fixture contains real .ruler content: tests/integration/fixtures/unified/.ruler/AGENTS.md#L1-L2

  # Primary Rules
  Line A

• Temp fixture too, because of course it does: tmp-fixtures/unified-basic/.ruler/AGENTS.md#L1-L2

  # AGENTS
  Main

Concrete suggestions

• Add an explicit directory ignore set for common non-source trees: tests, tmp-fixtures, coverage, dist, build, out, etc.
• Respect .gitignore or allow a config-level ignore list for nested discovery.
• Prefer a bounded/project-aware traversal instead of “walk everything that isn’t dot-prefixed”.

Broken Configs Get Silently “Handled”

When config loading goes wrong, the code shrugs, mutters “defaults are fine,” and drives off a cliff with remarkable composure.

• loadConfig() warns and falls back to {} for non-ENOENT read/parse failures, and applyHandler() separately swallows load failures when resolving nested. So a busted config can quietly degrade behavior instead of failing fast.
• That is not resilience; that is error laundering. Users get mystery behavior, and maintainers get bug reports written in blood. 🤡

Files

• src/core/ConfigLoader.ts
• src/cli/handlers.ts

Code

• Parse/read failure becomes empty config: src/core/ConfigLoader.ts#L202-L225

  } catch (err) {
    if (err instanceof Error && (err as ErrnoException).code !== 'ENOENT') {
      if (err.message.includes('[ruler]')) {
        throw err;
      }
      console.warn(
        `[ruler] Warning: could not read config file at ${configFile}: ${err.message}`,
      );
    }
    raw = {};
  }

• Handler silently defaults nested = false: src/cli/handlers.ts#L95-L105

  try {
    const config = await loadConfig({ projectRoot, configPath });
    nested = config.nested ?? false;
  } catch {
    nested = false;
  }

Concrete suggestions

• Fail hard on invalid or unreadable explicitly requested config files.
• Reserve default fallback for true “file missing” cases only.
• Return typed diagnostics from config loading and let the CLI decide whether to warn or abort.

Your Quality Gates Are Wearing a Blindfold

The repo’s test/lint setup leaves chunks of code completely unverified, which is a bold strategy if your goal is surprise explosions.

• eslint only covers src, Jest only matches *.ts tests, and TypeScript excludes tests entirely. Meanwhile, there are committed *.test.js files and random root-level JS experiment scripts just vibing outside the safety rails.
• So yes, there are tests here; no, that does not mean they run. Spectacular bit of performance art 🎭.

Files

• package.json
• jest.config.js
• tsconfig.json
• tests/unit/core/FileSystemUtils.test.js
• tests/unit/core/RuleProcessor.test.js
• test_manual_toml.js

Code

• Lint only hits src: package.json#L6-L14

  "lint": "eslint \"src/**/*.{ts,tsx}\"",
  "test": "jest",
  "build": "tsc"

• Jest only matches TS tests: jest.config.js#L3-L6

  preset: 'ts-jest',
  testEnvironment: 'node',
  testMatch: ['**/tests/**/*.{test,spec}.ts', '**/*.test.ts'],

• TypeScript excludes tests: tsconfig.json#L113-L114

  "include": ["src"],
  "exclude": ["node_modules", "dist", "tests"]

• Committed JS “tests” that Jest ignores: tests/unit/core/FileSystemUtils.test.js#L1-L5, tests/unit/core/RuleProcessor.test.js#L1-L5

  "use strict";
  describe('RuleProcessor', () => {

• Root-level scratchpad script in the repo like an abandoned sandwich: test_manual_toml.js#L1-L4

  // Test if we can manually construct TOML strings to get desired format
  function buildInlineArray(items) {

Concrete suggestions

• Expand lint/typecheck/test coverage to include tests/**/* and root JS scripts, or delete/archive the junk.
• Convert stray JS tests to TS or explicitly include JS test patterns in Jest.
• Move ad hoc experiment scripts into a non-committed scratch area or scripts/experimental/ with ownership.

The Agent Zoo Is All Boilerplate, No Plan

You have a small safari park of near-identical agent classes plus a hand-maintained registry, because apparently data tables were too mainstream.

• Many adapters are tiny subclasses whose entire personality is “return identifier, return name, return path.” Then src/agents/index.ts manually instantiates the whole herd.
• This is pure maintenance tax: every new agent means new file, new class, new imports, new registry entry, new opportunity for someone to typo themselves into oblivion. Delightful 🐒💥

Files

• src/agents/AbstractAgent.ts
• src/agents/ClaudeAgent.ts
• src/agents/KiroAgent.ts
• src/agents/TraeAgent.ts
• src/agents/index.ts

Code

• Base abstraction is just metadata slots: src/agents/AbstractAgent.ts#L13-L27

  export abstract class AbstractAgent implements IAgent {
    abstract getIdentifier(): string;
    abstract getName(): string;
    abstract getDefaultOutputPath(projectRoot: string): string;
  }

• Representative boilerplate class Create first version #1: src/agents/ClaudeAgent.ts#L7-L18

  export class ClaudeAgent extends AbstractAgent {
    getIdentifier(): string { return 'claude'; }
    getName(): string { return 'Claude Code'; }
    getDefaultOutputPath(projectRoot: string): string {
      return path.join(projectRoot, 'CLAUDE.md');
    }
  }

• Representative boilerplate class feat: initial implementation of Ruler v0.1.0 #2: src/agents/TraeAgent.ts#L8-L19

  export class TraeAgent extends AbstractAgent {
    getIdentifier(): string { return 'trae'; }
    getName(): string { return 'Trae AI'; }
    getDefaultOutputPath(projectRoot: string): string {
      return path.join(projectRoot, '.trae', 'rules', 'project_rules.md');
    }
  }

• Manual registry of doom: src/agents/index.ts#L38-L71

  export const allAgents: IAgent[] = [
    new CopilotAgent(),
    new ClaudeAgent(),
    new CodexCliAgent(),
    // ...
    new JetBrainsAiAssistantAgent(),
  ];

Concrete suggestions

• Replace metadata-only subclasses with a declarative agent registry object.
• Keep custom classes only for agents with truly unique apply/MCP behavior.
• Generate CLI help and capability tables from the same registry source.

SkillsProcessor Is a Copy-Paste Landfill

SkillsProcessor.ts is 38KB of duplicated directory shuffling, which is not a subsystem so much as a cry for help.

• Cleanup repeats the same access / dry-run / rm / catch pattern for every target, and propagation repeats the same temp-dir-copy-rename sequence again and again. The file is huge because it keeps retyping the same idea with different folder names like a very tired intern.
• Bonus points for temp dirs named with Date.now(), because nothing says “robust file operations” like millisecond collision roulette 🎰.

Files

• src/core/SkillsProcessor.ts

Code

• Repeated cleanup blocks: src/core/SkillsProcessor.ts#L177-L236

  // Clean up .claude/skills
  try {
    await fs.access(claudeSkillsPath);
    if (dryRun) {
      logVerboseInfo(`DRY RUN: Would remove ${CLAUDE_SKILLS_PATH}`, verbose, dryRun);
    } else {
      await fs.rm(claudeSkillsPath, { recursive: true, force: true });
    }
  } catch {}

• Repeated per-target orchestration: src/core/SkillsProcessor.ts#L557-L619

  if (selectedTargets.has('claude')) {
    await propagateSkillsForClaude(projectRoot, { dryRun });
  }
  if (selectedTargets.has('codex')) {
    await propagateSkillsForCodex(projectRoot, { dryRun });
  }

• One of many near-identical propagation functions: src/core/SkillsProcessor.ts#L683-L731, src/core/SkillsProcessor.ts#L741-L789

  const tempDir = path.join(claudeDir, `skills.tmp-${Date.now()}`);
  await copySkillsDirectory(skillsDir, tempDir);
  await fs.rename(tempDir, claudeSkillsPath);

• Hard-coded target map that should be driving a generic implementation: src/core/SkillsProcessor.ts#L140-L154

  const SKILL_TARGET_TO_IDENTIFIERS = new Map<SkillTarget, readonly string[]>([
    ['claude', ['claude', 'copilot', 'kilocode']],
    ['codex', ['codex']],
    // ...
  ]);

Concrete suggestions

• Introduce a single target descriptor table: { target, identifiers, relPath, logLabel }.
• Replace the N cleanup blocks with one cleanupTargetDirectory() loop.
• Replace the N propagation functions with one generic propagateSkillsToTarget() using fs.mkdtemp() or crypto.randomUUID() for temp dirs.
• Split discovery, orchestration, and filesystem mutation into separate modules so this file can stop eating the rest of the codebase alive.