This Codebase Smells! [2026-04-12]

[github-actions[bot]]

This Codebase Smells!

This week’s bouquet is hot plastic, damp Markdown, and the faint metallic tang of catch {}. The repo clearly does useful work, but it also keeps solving problems by adding a second code path, a magic deletion list, or a 1,423-line helper that desperately wants to be a data table. 💩

I found enough real problems without wasting time on style trivia. Thanks for that, I guess.

Table of Contents

• Recursive Scans Raid Generated Directories
• Revert Cleanup Deletes User Files On Vibes
• Config Loading Has Split Personality Disorder
• MCP Handling Has Two Bosses
• SkillsProcessor Is a Copy-Paste Cathedral
• Revert Ignores Invalid Agent Names

Recursive Scans Raid Generated Directories

Nested discovery crawls every non-hidden directory, which is how you end up treating node_modules like a first-class configuration source. 💩

• findAllRulerDirs() only skips hidden directories and nested git repos. In a real project that still means node_modules, dist, build, coverage, vendor, fixtures, and other generated junk get walked recursively.
• That is wasted IO at best and false-positive .ruler discovery at worst. This repo already ships big tests/ and tmp-fixtures/ trees, so the slowdown is not hypothetical; it is merely waiting for a larger codebase to become embarrassing.

Files

• src/core/FileSystemUtils.ts

Code

• src/core/FileSystemUtils.ts lines 237–255: link

  if (entry.name === '.ruler') {
    rulerDirs.push(fullPath);
  } else {
    // Recursively search subdirectories (but skip hidden directories like .git)
    if (!entry.name.startsWith('.')) {
      // Do not cross git repository boundaries (except the starting root)
      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

• Skip common generated/vendor dirs by default: node_modules, dist, build, coverage, target, vendor, out, etc.
• Add config/CLI excludes for nested discovery instead of hard-wiring “everything that isn’t hidden.”
• Add tests that prove discovery ignores fixtures and build artifacts.

Revert Cleanup Deletes User Files On Vibes

Revert has a filename kill list—including config.toml—because apparently provenance was too mainstream. 🤮

• removeAdditionalAgentFiles() deletes based on hard-coded paths instead of “Ruler created this” evidence. config.toml is a completely ordinary root file name, so this is filesystem roulette with a loaded chamber.
• A .bak check is not provenance. “No backup” means “we don’t know,” not “safe to unlink,” unless the tool has fully committed to being a tiny, overconfident goblin.

Files

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

Code

• src/core/revert-engine.ts lines 311–319: link

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

• src/core/revert-engine.ts lines 333–349: link

  const backupExists = await fileExists(`${fullPath}.bak`);
  if (backupExists) {
    const restored = await restoreFromBackup(fullPath, verbose, dryRun);
    if (restored) {
      filesRemoved++;
    }
  } else {
    if (dryRun) {
      logVerbose(
        `${prefix} Would remove additional file: ${fullPath}`,
        verbose,
      );
    } else {
      await fs.unlink(fullPath);
      logVerbose(`${prefix} Removed additional file: ${fullPath}`, verbose);
    }
    filesRemoved++;
  }

• src/paths/mcp.ts lines 33–36: link

  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;

Suggestions

• Track generated files in a manifest or write a provenance marker into managed configs.
• Only delete ambiguous paths when a marker or backup proves Ruler owns them.
• Make extra cleanup for generic filenames opt-in with a loud --force-style flag.

Config Loading Has Split Personality Disorder

Two different loaders define “the current rules,” and they do not even agree on which Markdown files exist.

• apply-engine uses loadConfig() plus recursive readMarkdownFiles() for actual application, then separately calls loadUnifiedConfig() for MCP. So the repo already has two sources of truth before the first write begins. Lovely.
• Worse, the unified loader only readdir()s top-level *.md files, while the real loader walks recursively and applies precedence logic. Hashes, diagnostics, and future behavior drift are basically built into the architecture.

Files

• src/core/apply-engine.ts
• src/core/FileSystemUtils.ts
• src/core/UnifiedConfigLoader.ts

Code

• src/core/apply-engine.ts lines 283–296: link

  const config = await loadConfig({
    projectRoot,
    configPath,
  });
  
  // Read rule files
  const files = await FileSystemUtils.readMarkdownFiles(rulerDirs[0]);
  
  // Concatenate rules
  const concatenatedRules = concatenateRules(files, path.dirname(primaryDir));
  
  // Load unified config to get merged MCP configuration
  const { loadUnifiedConfig } = await import('./UnifiedConfigLoader');
  const unifiedConfig = await loadUnifiedConfig({ projectRoot, configPath });

• src/core/FileSystemUtils.ts lines 86–98: link

  if (isDir) {
    // Skip .ruler/skills; skills are propagated separately and should not be concatenated
    const relativeFromRoot = path.relative(rulerDir, fullPath);
    const isSkillsDir =
      relativeFromRoot === SKILLS_DIR ||
      relativeFromRoot.startsWith(`${SKILLS_DIR}${path.sep}`);
    if (isSkillsDir) {
      continue;
    }
    await walk(fullPath);
  } else if (isFile && entry.name.endsWith('.md')) {
    const content = await fs.readFile(fullPath, 'utf8');
    mdFiles.push({ path: fullPath, content });
  }

• src/core/UnifiedConfigLoader.ts lines 109–112: link

  const dirEntries = await fs.readdir(meta.rulerDir, { withFileTypes: true });
  const mdFiles = dirEntries
    .filter((e) => e.isFile() && e.name.toLowerCase().endsWith('.md'))
    .map((e) => path.join(meta.rulerDir, e.name));

Suggestions

• Build one canonical resolver for config, rules, and MCP.
• Reuse the same recursive discovery and precedence logic everywhere.
• Make downstream consumers derive from the resolved model instead of reparsing the filesystem differently.

MCP Handling Has Two Bosses

MCP handling is split between adapters and the engine, then defended with a stringly skip list and a parse-error-to-empty-object trap. 🔥

• applyConfigurationsToAgents() hands MCP into each agent and then immediately runs a second handleMcpConfiguration() pass. Several adapters also merge/write MCP themselves, so ownership is duplicated and the abstractions are already fighting in the parking lot.
• The skip list is brittle enough that it already misses Amazon Q (amazon-q-cli vs amazonqcli), and the generic reader returns {} on any JSON failure. So malformed user config can quietly become “empty config, now overwritten.” Amazing. No notes. Actually, many notes.

Files

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

Code

• src/core/apply-engine.ts lines 481–487: link

  await agent.applyRulerConfig(
    concatenatedRules,
    projectRoot,
    agentRulerMcpJson,
    finalAgentConfig,
    backup,
  );

• src/core/apply-engine.ts lines 492–499: link

  await handleMcpConfiguration(
    agent,
    agentConfig,
    config,
    agentRulerMcpJson,
    projectRoot,
    generatedPaths,
    verbose,

• src/agents/AmazonQCliAgent.ts lines 45–53: link

  // Handle MCP configuration if enabled and provided
  const mcpEnabled = agentConfig?.mcp?.enabled ?? true;
  if (mcpEnabled && rulerMcpJson) {
    const mcpPath = path.resolve(
      projectRoot,
      agentConfig?.outputPathConfig ?? outputPaths['mcp'],
    );
    const mcpStrategy = agentConfig?.mcp?.strategy ?? 'merge';

• src/core/apply-engine.ts lines 673–676: link

  agent.getIdentifier() === 'zed' ||
  agent.getIdentifier() === 'gemini-cli' ||
  agent.getIdentifier() === 'amazon-q-cli' ||
  agent.getIdentifier() === 'crush'

• src/agents/AmazonQCliAgent.ts lines 15–17: link

  getIdentifier(): string {
    return 'amazonqcli';
  }

• src/paths/mcp.ts lines 84–89: link

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

Suggestions

• Choose one MCP owner: engine-only with agent-specific transformers, or adapter-only with zero engine fallback.
• Replace string comparisons with explicit agent capability hooks.
• Treat parse errors differently from missing files; fail loudly or require an explicit repair flag.
• Add regression tests for Amazon Q skip behavior and malformed native config preservation.

SkillsProcessor Is a Copy-Paste Cathedral

SkillsProcessor.ts is a 1,423-line copy-paste cathedral where every new agent gets another hand-written filesystem ritual.

• The file manually dispatches target by target and repeats the same mkdir → temp dir → copy → rm → rename → cleanup flow over and over. This is not extensibility; it is clerical labor wearing TypeScript.
• Every repetition is another chance for drift in temp-dir naming, logs, edge-case handling, and bug fixes. When a new skills-capable agent means copy-pasting yet another almost-identical function, the design has already submitted its resignation.

Files

• src/core/SkillsProcessor.ts

Code

• src/core/SkillsProcessor.ts lines 557–573: link

  if (selectedTargets.has('claude')) {
    logVerboseInfo(
      `Copying skills to ${CLAUDE_SKILLS_PATH} for Claude Code, GitHub Copilot, and KiloCode`,
      verbose,
      dryRun,
    );
    await propagateSkillsForClaude(projectRoot, { dryRun });
  }
  
  if (selectedTargets.has('codex')) {
    logVerboseInfo(
      `Copying skills to ${CODEX_SKILLS_PATH} for OpenAI Codex CLI`,
      verbose,
      dryRun,
    );
    await propagateSkillsForCodex(projectRoot, { dryRun });
  }

• src/core/SkillsProcessor.ts lines 703–722: link

  await fs.mkdir(claudeDir, { recursive: true });
  
  // Use atomic replace: copy to temp, then rename
  const tempDir = path.join(claudeDir, `skills.tmp-${Date.now()}`);
  
  try {
    // Copy to temp directory
    await copySkillsDirectory(skillsDir, tempDir);
  
    // Atomically replace the target
    // First, remove existing target if it exists
    try {
      await fs.rm(claudeSkillsPath, { recursive: true, force: true });
    } catch {
      // Target didn't exist, that's fine
    }
  
    // Rename temp to target
    await fs.rename(tempDir, claudeSkillsPath);

• src/core/SkillsProcessor.ts lines 761–780: link

  // Ensure .codex directory exists
  await fs.mkdir(codexDir, { recursive: true });
  
  // Use atomic replace: copy to temp, then rename
  const tempDir = path.join(codexDir, `skills.tmp-${Date.now()}`);
  
  try {
    // Copy to temp directory
    await copySkillsDirectory(skillsDir, tempDir);
  
    // Atomically replace the target
    // First, remove existing target if it exists
    try {
      await fs.rm(codexSkillsPath, { recursive: true, force: true });
    } catch {
      // Target didn't exist, that's fine
    }
  
    // Rename temp to target
    await fs.rename(tempDir, codexSkillsPath);

Suggestions

• Move skills target metadata into a single table: identifiers, labels, target paths, and capability flags.
• Implement generic syncSkillsTarget() and cleanupSkillsTarget() helpers.
• Restrict catches to expected conditions like ENOENT; surface the rest instead of normalizing every error into a shrug.

Revert Ignores Invalid Agent Names

revert treats invalid agent names as a gentle suggestion, which is a ridiculous thing for a destructive command to do.

• resolveSelectedAgents() correctly throws on unknown agents, but revert() catches that case and keeps going “for backward compatibility.” That makes typos easy to miss during the command that is supposed to clean things up safely.
• The integration tests explicitly bless this behavior, so the inconsistency with apply is institutionalized. If the user asked to revert a nonexistent agent, the correct response is failure, not improv comedy with file deletion.

Files

• src/revert.ts
• src/core/agent-selection.ts
• tests/integration/revert-cli.test.ts

Code

• src/revert.ts lines 56–67: link

  try {
    selected = resolveSelectedAgents(config, agents);
  } catch (error) {
    // For backward compatibility, revert continues with available agents if some are invalid
    // This preserves the original behavior where invalid agents were silently ignored
    if (
      error instanceof Error &&
      error.message.includes('Invalid agent specified')
    ) {
      logVerbose(

• src/core/agent-selection.ts lines 37–41: link

  if (invalidAgents.length > 0) {
    throw createRulerError(
      `Invalid agent specified: ${invalidAgents.join(', ')}`,
      `Valid agents are: ${[...validAgentIdentifiers].join(', ')}`,
    );
  }

• tests/integration/revert-cli.test.ts lines 139–146: link

  it('should handle invalid agents list gracefully', () => {
    const output = execSync(`node dist/cli/index.js revert --project-root ${tmpDir} --agents invalid-agent --dry-run`, {
      encoding: 'utf8',
      stdio: 'pipe'
    });
    
    expect(output).toContain('[ruler:dry-run]');
  });

Suggestions

• Make revert use the same strict validation path as apply.
• If compatibility is genuinely required, gate it behind --ignore-invalid-agents.
• Update integration tests to assert failure by default so typos stop being silently “helped.”