This Codebase Smells! [2026-03-29]

[github-actions[bot]]

This Codebase Smells!

This week’s perfume is a rich, artisanal blend of fake abstractions, accidental config vandalism, and “surely this won’t scale” optimism 🤮. The repo clearly wants to be a tidy adapter-driven CLI, but parts of it behave like a raccoon with fs.unlink permissions. I stopped at seven high-ROI fixes, mostly because my blood pressure filed a complaint.

Table of Contents

• Substring Agent Filters Pick the Wrong Agents
• Open Hands Treats Root config.toml Like Disposable Junk
• Disabling Skills Leaves Stale Generated Directories Behind
• Nested Discovery Crawls the Whole Repo Like a Goblin
• The Adapter Pattern Is Mostly Decorative
• Invalid MCP Files Get “Recovered” by Being Overwritten
• The Test Suite Pays a Full Build Tax and Commits Dead Artifacts

Substring Agent Filters Pick the Wrong Agents

🤡 --agents pi can legitimately select GitHub Copilot, which is some world-class clown engineering.

• Agent selection accepts a filter when it matches an identifier or merely appears anywhere inside an agent display name. That means pi matches the real pi agent and also "GitHub Copilot" because, congratulations, "copilot" contains "pi".
• The same logic is reused for default_agents, so this is not just a CLI paper cut. It can silently generate extra agent files from perfectly valid config. Very user-friendly, if the user is your worst enemy.

Files

• src/core/agent-selection.ts
• src/agents/CopilotAgent.ts
• src/agents/PiAgent.ts

Code

• CLI matching uses substring checks: src/core/agent-selection.ts#L44-L49

  selected = allAgents.filter((agent) =>
    filters.some(
      (f) =>
        agent.getIdentifier() === f ||
        agent.getName().toLowerCase().includes(f),
    ),
  );

• default_agents repeats the same trick: src/core/agent-selection.ts#L81-L83

  return defaults.some(
    (d) => identifier === d || agent.getName().toLowerCase().includes(d),
  );

• The names really do collide this stupidly: src/agents/CopilotAgent.ts#L11-L17, src/agents/PiAgent.ts#L7-L13

  getIdentifier(): string {
    return 'copilot';
  }
  
  getName(): string {
    return 'GitHub Copilot';
  }

  getIdentifier(): string {
    return 'pi';
  }
  
  getName(): string {
    return 'Pi Coding Agent';
  }

Suggestions

• Match agent identifiers exactly by default.
• If fuzzy matching stays, only allow it when the result is unambiguous.
• Add collision tests for short filters like pi, code, q, and other chaos goblins.

Open Hands Treats Root config.toml Like Disposable Junk

☠️ A generic repo-root config.toml is being treated as Ruler-owned, which is how tools become urban legends.

• Open Hands MCP targets plain config.toml in the project root. That filename is absurdly generic. Then revert hard-codes the same file into its cleanup list and will unlink it if no backup exists. Incredible stuff: the most collision-prone path in the repo, now with delete privileges.
• Apply is no saint either. If parsing the existing TOML fails, Ruler shrugs, assumes empty config, and writes a fresh file anyway. That turns “user has an unrelated root TOML” into “user had an unrelated root TOML.”

Files

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

Code

• Open Hands points at 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'));
    break;

• Revert hard-codes config.toml into extra cleanup: src/core/revert-engine.ts#L311-L319

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

• If there is no backup, it just deletes the file: src/core/revert-engine.ts#L333-L347

  const backupExists = await fileExists(`${fullPath}.bak`);
  if (backupExists) {
    const restored = await restoreFromBackup(fullPath, verbose, dryRun);
    if (restored) {
      filesRemoved++;
    }
  } else {
    await fs.unlink(fullPath);
  }

• Parse failure becomes “guess we’ll recreate it”: src/mcp/propagateOpenHandsMcp.ts#L123-L128, src/mcp/propagateOpenHandsMcp.ts#L192-L197

  try {
    const tomlContent = await fs.readFile(openHandsConfigPath, 'utf8');
    config = parseTOML(tomlContent);
  } catch {
    // File doesn't exist, we'll create it.
  }

  if (backup) {
    const { backupFile } = await import('../core/FileSystemUtils');
    await backupFile(openHandsConfigPath);
  }
  await fs.writeFile(openHandsConfigPath, stringify(config));

Suggestions

• Stop using bare root config.toml unless ownership is provable.
• Only overwrite or delete files with a Ruler marker, manifest entry, or known backup.
• On invalid existing TOML, fail loudly with path + parse error instead of rewriting from {}.

Disabling Skills Leaves Stale Generated Directories Behind

🧟 There is cleanup logic for disabled skills, and the main flow politely never calls it. Absolute genius.

• propagateSkills() explicitly handles !skillsEnabled by cleaning generated skill directories. Great. Unfortunately, lib.ts only calls propagateSkills() when the resolved flag is truthy, so the cleanup branch is dead code in normal runs.
• Result: once skills were generated, turning them off doesn’t reliably remove .claude/skills, .codex/skills, and the rest of the undead swarm. The flag says “disabled”; the filesystem says “lol no.”

Files

• src/lib.ts
• src/core/SkillsProcessor.ts

Code

• Nested mode only calls skills propagation when enabled: src/lib.ts#L118-L124

  const skillsEnabledResolved = resolveSkillsEnabled(
    skillsEnabled,
    rootConfig.skills?.enabled,
  );
  if (skillsEnabledResolved) {
    const { propagateSkills } = await import('./core/SkillsProcessor');

• Single-directory mode has the same bug: src/lib.ts#L178-L183

  const skillsEnabledResolved = resolveSkillsEnabled(
    skillsEnabled,
    singleConfig.config.skills?.enabled,
  );
  if (skillsEnabledResolved) {

• The cleanup path exists, but it’s stranded: src/core/SkillsProcessor.ts#L453-L460

  if (!skillsEnabled) {
    logVerboseInfo(
      'Skills support disabled, cleaning up skills directories',
      verbose,
      dryRun,
    );
    await cleanupSkillsDirectories(projectRoot, dryRun, verbose);
  }

Suggestions

• Always call propagateSkills(projectRoot, agents, resolvedFlag, ...).
• Better yet, rename it to something honest like syncSkillsState.
• Add regression tests for enabled -> disabled transitions in both single and nested modes.

Nested Discovery Crawls the Whole Repo Like a Goblin

🐀 Nested mode recursively walks every non-hidden directory, which is adorable until it meets a monorepo and starts eating drywall.

• The README sells nested mode for complex hierarchies and monorepos. The implementation responds by recursively traversing basically everything except hidden directories. So node_modules, dist, coverage, vendored code, build output, and other landfill all get a warm personal visit.
• That is a needless performance tax and a reliability hazard. The bigger the repo, the more this feature turns into a filesystem punishment ritual.

Files

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

Code

• The docs explicitly market nested discovery: README.md#L169-L176

  - Discover all `.ruler/` directories in the project hierarchy
  - Load and concatenate rules from each directory in order
  - Decide whether nested mode is enabled using the following precedence:
    1. `ruler apply --nested` (or `--no-nested`) takes top priority
    2. `nested = true` in `ruler.toml`

• The implementation skips only hidden directories: src/core/FileSystemUtils.ts#L240-L255

  // Recursively search subdirectories (but skip hidden directories like .git)
  if (!entry.name.startsWith('.')) {
    const gitDir = path.join(fullPath, '.git');
    try {
      const gitStat = await fs.stat(gitDir);
      if (gitStat.isDirectory() && path.resolve(fullPath) !== rootPath) {
        continue;
      }
    } catch {
      // no .git boundary, continue traversal
    }
    await findRulerDirs(fullPath);
  }

Suggestions

• Hard-skip obvious junk dirs: node_modules, dist, build, coverage, vendor, .next, etc.
• Respect .gitignore or a Ruler-specific exclude list.
• Replace manual recursion with a fast glob/search that supports ignore patterns.

The Adapter Pattern Is Mostly Decorative

🎭 You built agent adapters and then stuffed the real behavior back into central special-case code like a magician hiding pigeons in his sleeves.

• AbstractAgent advertises a nice adapter interface. Then apply-engine.ts immediately branches on hard-coded agent identifiers for routing, skipping, and transforming MCP behavior. So the core still knows everybody’s weird little secrets. Splendid abstraction. No notes. Actually, many notes.
• The same smell shows up in SkillsProcessor.ts, where per-agent skills propagation is mostly the same access -> mkdir -> temp dir -> copy -> rm -> rename ritual duplicated over and over. Adding one agent now means editing core orchestration and cloning more boilerplate. Maintenance by photocopier 💩.

Files

• src/agents/AbstractAgent.ts
• src/core/apply-engine.ts
• src/core/SkillsProcessor.ts

Code

• The abstraction says agents should own behavior: src/agents/AbstractAgent.ts#L13-L27

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

• The core still hard-codes per-agent branching: src/core/apply-engine.ts#L651-L683

  if (agent.getIdentifier() === 'openhands') {
    return await applyOpenHandsMcpConfiguration(...);
  }
  
  if (agent.getIdentifier() === 'opencode') {
    return await applyOpenCodeMcpConfiguration(...);
  }
  
  if (
    agent.getIdentifier() === 'zed' ||
    agent.getIdentifier() === 'gemini-cli' ||
    agent.getIdentifier() === 'amazon-q-cli' ||
    agent.getIdentifier() === 'crush'
  ) {
    return;
  }

• And more special-case transforms, because why stop digging: src/core/apply-engine.ts#L893-L900

  let mcpToMerge = filteredMcpJson;
  if (agent.getIdentifier() === 'claude') {
    mcpToMerge = transformMcpForClaude(filteredMcpJson);
  } else if (agent.getIdentifier() === 'kilocode') {
    mcpToMerge = transformMcpForKiloCode(filteredMcpJson);
  } else if (agent.getIdentifier() === 'factory') {

• Skills propagation is basically copy-paste with costume changes: src/core/SkillsProcessor.ts#L647-L665, src/core/SkillsProcessor.ts#L705-L723

  export async function propagateSkillsForClaude(...) {
    const skillsDir = path.join(projectRoot, RULER_SKILLS_PATH);
    const claudeSkillsPath = path.join(projectRoot, CLAUDE_SKILLS_PATH);
    ...
    if (options.dryRun) {
      return [`Copy skills from ${RULER_SKILLS_PATH} to ${CLAUDE_SKILLS_PATH}`];
    }
  }

  export async function propagateSkillsForCodex(...) {
    const skillsDir = path.join(projectRoot, RULER_SKILLS_PATH);
    const codexSkillsPath = path.join(projectRoot, CODEX_SKILLS_PATH);
    ...
    if (options.dryRun) {
      return [`Copy skills from ${RULER_SKILLS_PATH} to ${CODEX_SKILLS_PATH}`];
    }
  }

Suggestions

• Move MCP application/transform hooks onto the agent interface.
• Replace per-agent skills functions with a registry like { agentId, targetPath, sharedTargetGroup? }.
• Keep the core engine dumb; make adapters smart. That is literally the whole point of the pattern.

Invalid MCP Files Get “Recovered” by Being Overwritten

🤕 Several code paths treat parse failures as {} and then happily write new config, which is less recovery and more assisted amnesia.

• readNativeMcp() swallows every error and returns an empty object. So malformed JSON, unsupported vendor changes, bad edits, or corrupted files all become “looks empty to me, boss.” That is a spectacular way to lose diagnostics and context.
• The same pattern shows up in agent-specific writers like OpenCode and RooCode. If parsing fails, the code assumes a blank config and keeps going. Users don’t get a clear error; they get their file quietly normalized into whatever Ruler currently feels like writing. Wonderful.

Files

• src/paths/mcp.ts
• src/core/apply-engine.ts
• src/mcp/propagateOpenCodeMcp.ts
• src/agents/RooCodeAgent.ts

Code

• Generic MCP reads swallow everything: src/paths/mcp.ts#L84-L88

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

• Codex fallback ignores invalid TOML and keeps going: src/core/apply-engine.ts#L906-L917

  let existing = await readNativeMcp(dest);
  if (isCodexToml) {
    try {
      const tomlContent = await fs.readFile(dest, 'utf8');
      existing = parseTOML(tomlContent) as Record<string, unknown>;
    } catch (error) {
      // ignore missing or invalid TOML, fall back to previously read value
    }
  }

• OpenCode does the same “empty config if parse fails” routine: src/mcp/propagateOpenCodeMcp.ts#L108-L113

  try {
    const existingContent = await fs.readFile(openCodeConfigPath, 'utf8');
    existingConfig = JSON.parse(existingContent);
  } catch {
    // File doesn't exist, we'll create it
  }

• RooCode too, because consistency in bad ideas is still consistency: src/agents/RooCodeAgent.ts#L71-L79

  try {
    const existingContent = await fs.readFile(mcpPath, 'utf-8');
    const parsed = JSON.parse(existingContent);
    if (parsed && typeof parsed === 'object') {
      existingConfig = parsed as Record<string, unknown>;
    }
  } catch {
    existingConfig = {};
  }

Suggestions

• Treat ENOENT as normal, but treat parse errors as real errors.
• Abort on invalid existing config unless the user explicitly asks for overwrite.
• Back up the raw file before parsing and include path + parse error in the failure message.

The Test Suite Pays a Full Build Tax and Commits Dead Artifacts

🧱 Every npm test starts with a full project build, while compiled .js test files sit in the repo like stale roadkill.

• globalSetup unconditionally runs npm run build, so even plain unit tests pay the cost of compiling the whole project before doing anything useful. That might make sense for a narrow e2e lane, but for the default test path it’s just a big slow tax receipt.
• Meanwhile Jest only matches .ts tests, but compiled .js copies of tests are committed under tests/unit/core/. So the repo stores dead generated artifacts that do not even run. This is clutter, confusion, and future drift bundled into one convenient little trash bag.

Files

• jest.setup.js
• jest.config.js
• tests/unit/core/RuleProcessor.test.ts
• tests/unit/core/RuleProcessor.test.js
• tests/unit/core/FileSystemUtils.test.js

Code

• The suite always builds first: jest.setup.js#L3-L8

  module.exports = async () => {
    console.log('Building project for tests...');
    execSync('npm run build', { stdio: 'inherit' });
    console.log('Build completed.');
  };

• Jest only matches TypeScript tests anyway: jest.config.js#L3-L9

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

• Source test exists in TS: tests/unit/core/RuleProcessor.test.ts#L4-L10

  describe('RuleProcessor', () => {
    it('concatenates and formats rule content with source markers', () => {
      const files = [
        { path: '/project/.ruler/a.md', content: 'A rule' },
        { path: '/project/.ruler/b.md', content: 'B rule' },
      ];

• And a compiled JS corpse is committed beside it: tests/unit/core/RuleProcessor.test.js#L4-L11

  describe('RuleProcessor', () => {
      it('concatenates and formats rule content with source markers', () => {
          const files = [
              { path: '/project/.ruler/a.md', content: 'A rule' },
              { path: '/project/.ruler/b.md', content: 'B rule' },
          ];
          jest.spyOn(process, 'cwd').mockReturnValue('/project');

Suggestions

• Build only for e2e/integration suites that require emitted dist artifacts.
• Remove committed generated .js tests and ignore them so they stop fossilizing in git.
• Split fast unit tests from heavyweight CLI/e2e runs so local feedback stops taking the scenic route.