This Codebase Smells! [2026-07-12]

[github-actions[bot]]

This Codebase Smells!

This week’s aroma is “security check used as a coaster” with top notes of copy-paste architecture and a lingering finish of CI sadness. 🤮 There’s good intent buried in here, but too much of the implementation still behaves like a developer muttered “eh, probably fine” and sprinted away from the keyboard. Let’s disinfect the worst bits.

Table of Contents

• Root Path Guard Gives Up at Filesystem Root
• Apply and Unified Config Run Two Rule Pipelines
• Broken Config Files Get Repaired With a Flamethrower
• Agent Output Path Rules Are Copy Pasted Everywhere
• The Test Tooling Is Slow Blind and Manual

Root Path Guard Gives Up at Filesystem Root

🧨 The helper that is supposed to keep writes inside the project can fail open when it reaches /, which is a truly inspired interpretation of “security boundary.”

The containment check walks up the directory tree until it finds something real. If it walks all the way to filesystem root without finding an existing ancestor, it just returns success. Amazing. The guard dog saw the fence disappear and decided that was a management problem.

Because both assertManagedPathInsideRoot() and writeGeneratedFile() rely on this logic, a non-existent out-of-tree target can be blessed without ever proving it lives under the project root. It’s not the easiest bug to hit, but fail-open path validation is exactly the kind of “rare edge case” that becomes a postmortem later.

Files

• src/core/FileSystemUtils.ts

Code

• src/core/FileSystemUtils.ts lines 93-95 — https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L93-L95

  const parent = path.dirname(current);
  if (parent === current) {
    return;
  }

• src/core/FileSystemUtils.ts lines 107-118 — https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L107-L118

  await assertContainingDirectoryInsideRoot(managedPath, rootPath, action);
  
  try {
    const realManagedPath = await fs.realpath(managedPath);
    if (!isPathInsideOrEqual(realRoot, realManagedPath)) {
      throw new Error(`${action}: ${managedPath}`);
    }
  } catch (error) {
    if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
      throw error;
    }
  }

Concrete suggestions

• Fail closed when the upward walk hits filesystem root without proving containment.
• Validate the resolved lexical path against rootPath before any mkdir/write, then re-check the real path after creation.
• Add tests for non-existent paths outside the repo, not just symlink/hardlink cases.

Apply and Unified Config Run Two Rule Pipelines

🤹 The repo reads and concatenates rules in two separate subsystems, because apparently one source of truth was too emotionally stable.

apply-engine.ts reads markdown files and builds the actual rule payload itself, then separately loads UnifiedConfigLoader and uses it only for MCP data. Meanwhile UnifiedConfigLoader has its own rule discovery, metadata, diagnostics, and hashing path. So yes, this codebase has two different machines doing the same job and politely pretending that won’t drift.

That split is already begging for inconsistency: rule ordering fixes, diagnostics, inclusion rules, and future metadata changes can land in one path and never reach the other. The main apply flow is effectively saying, “Thanks for the unified loader, I’ll just use the side dish.” 🍽️

Files

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

Code

• src/core/apply-engine.ts lines 302-315 — https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L302-L315

  const files = await FileSystemUtils.readMarkdownFiles(rulerDirs[0], {
    includeAgents: shouldIncludeAgentsInRules(config),
  });
  
  const concatenatedRules = concatenateRules(files, path.dirname(primaryDir));
  
  const { loadUnifiedConfig } = await import('./UnifiedConfigLoader');
  const unifiedConfig = await loadUnifiedConfig({
    projectRoot: effectiveProjectRoot,
    configPath,
    checkGlobal: !localOnly,
  });

• src/core/apply-engine.ts lines 73-82 — https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L73-L82

  const config = await loadConfigForRulerDir(
    rulerDir,
    configPath,
    resolvedNested,
    localOnly,
  );
  const files = await FileSystemUtils.readMarkdownFiles(rulerDir, {
    includeAgents: shouldIncludeAgentsInRules(config),
  });

• src/core/UnifiedConfigLoader.ts lines 199-239 — https://github.com/intellectronica/ruler/blob/main/src/core/UnifiedConfigLoader.ts#L199-L239

  const mdFiles = await FileSystemUtils.readMarkdownFiles(meta.rulerDir, {
    includeAgents: includeAgentsInRules,
  });
  ...
  const concatenated = concatenateRules(
    ruleFiles.map((f) => ({ path: f.path, content: f.content })),
    path.dirname(meta.rulerDir),
  );

Concrete suggestions

• Make apply-engine consume unifiedConfig.rules.concatenated instead of rebuilding rules itself.
• Extract one shared rule-loading pipeline for single and nested modes.
• Surface unified-loader diagnostics in the apply path so warnings actually protect the code that gets written.

Broken Config Files Get Repaired With a Flamethrower

🔥 Several agents treat “config missing” and “config invalid” as the same thing, then overwrite the user’s file like a raccoon with admin access.

OpenCode, RooCode, and CodexCli all swallow parse failures and continue with a fresh config. That means malformed user config can get silently bulldozed into a brand-new managed file instead of surfacing a useful error. Congrats: the tool is now “self-healing” in the same way deleting production is a kind of cleanup.

The really spicy part is that MistralVibeAgent already shows the sane pattern: tolerate ENOENT, throw on invalid config, preserve the original file. So the repo already knows how not to be reckless; it just hasn’t bothered to be consistent about it.

Files

• src/agents/OpenCodeAgent.ts
• src/agents/RooCodeAgent.ts
• src/agents/CodexCliAgent.ts
• src/agents/MistralVibeAgent.ts
• tests/unit/agents/RooCodeAgent.test.ts

Code

• src/agents/OpenCodeAgent.ts lines 65-86 — https://github.com/intellectronica/ruler/blob/main/src/agents/OpenCodeAgent.ts#L65-L86

  try {
    const existingMcpConfig = JSON.parse(await fs.readFile(mcpPath, 'utf-8'));
    ...
  } catch {
    finalMcpConfig = {
      $schema: 'https://opencode.ai/config.json',
      mcp: (rulerMcpJson.mcpServers ?? {}) as Record<string, unknown>,
    };
  }

• src/agents/RooCodeAgent.ts lines 78-86 — https://github.com/intellectronica/ruler/blob/main/src/agents/RooCodeAgent.ts#L78-L86

  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 = {};
  }

• src/agents/CodexCliAgent.ts lines 102-109 — https://github.com/intellectronica/ruler/blob/main/src/agents/CodexCliAgent.ts#L102-L109

  let existingConfig: CodexCliConfig = {};
  try {
    const existingContent = await fs.readFile(configPath, 'utf8');
    existingConfig = parseTOML(existingContent);
  } catch {
    // File doesn't exist or can't be parsed, use empty config
  }

• src/agents/MistralVibeAgent.ts lines 156-168 — https://github.com/intellectronica/ruler/blob/main/src/agents/MistralVibeAgent.ts#L156-L168

  try {
    const existingContent = await fs.readFile(configPath, 'utf8');
    configExists = true;
    existingConfig = parseTOML(existingContent) as VibeConfig;
  } catch (error) {
    if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
      existingConfig = {};
    } else {
      throw new Error(`Invalid Mistral config at ${configPath}: ...`);
    }
  }

Concrete suggestions

• Only swallow ENOENT; treat parse errors as real errors and stop.
• Preserve malformed files untouched and avoid creating backups/writes when parsing fails.
• Extract a shared “read existing managed config” helper with strict error semantics.
• Replace the Roo invalid-JSON behavior with tests that assert failure and preservation.

Agent Output Path Rules Are Copy Pasted Everywhere

🧬 The repo already has a shared output-path resolver, and then a bunch of agents re-implement the same precedence logic anyway because duplication is apparently a spiritual practice.

getAgentOutputPaths() already centralizes outputPath / outputPathInstructions / outputPathConfig precedence, and core apply/revert logic uses it. But agents like Aider, CodexCli, and MistralVibe still hand-roll the same fallback chains inline. This is how tiny logic differences metastasize into “why does revert disagree with apply?” bugs six months from now.

Worse, the shared helper drives .gitignore and revert accounting while agent implementations do the actual writes. If those two worlds drift, the repo gets the delightful experience of tracking one path and mutating another. Chef’s kiss. 💩

Files

• src/agents/agent-utils.ts
• src/core/apply-engine.ts
• src/agents/AiderAgent.ts
• src/agents/CodexCliAgent.ts
• src/agents/MistralVibeAgent.ts

Code

• src/agents/agent-utils.ts lines 22-36 — https://github.com/intellectronica/ruler/blob/main/src/agents/agent-utils.ts#L22-L36

  if ('instructions' in defaultPaths) {
    const instructionsPath =
      agentConfig?.outputPath ??
      agentConfig?.outputPathInstructions ??
      defaultPaths.instructions;
    paths.push(instructionsPath);
  }
  ...
  const configPath =
    agentConfig?.outputPathConfig ?? defaultPaths[configKey];

• src/core/apply-engine.ts lines 452-457 — https://github.com/intellectronica/ruler/blob/main/src/core/apply-engine.ts#L452-L457

  const outputPaths = getAgentOutputPaths(agent, projectRoot, agentConfig);
  logVerbose(
    `Agent ${agent.getName()} output paths: ${outputPaths.join(', ')}`,
    verbose,
  );
  generatedPaths.push(...outputPaths);

• src/agents/AiderAgent.ts lines 33-39 — https://github.com/intellectronica/ruler/blob/main/src/agents/AiderAgent.ts#L33-L39

  outputPath:
    agentConfig?.outputPath ||
    agentConfig?.outputPathInstructions ||
    undefined,

• src/agents/CodexCliAgent.ts lines 60-65 — https://github.com/intellectronica/ruler/blob/main/src/agents/CodexCliAgent.ts#L60-L65

  outputPath:
    agentConfig?.outputPath ||
    agentConfig?.outputPathInstructions ||
    undefined,

• src/agents/MistralVibeAgent.ts lines 76-80 — https://github.com/intellectronica/ruler/blob/main/src/agents/MistralVibeAgent.ts#L76-L80

  outputPath:
    agentConfig?.outputPath ||
    agentConfig?.outputPathInstructions ||
    undefined,

Concrete suggestions

• Move all output-path resolution into one shared helper that returns typed, absolute paths.
• Make agent adapters consume that helper instead of re-encoding precedence by hand.
• Add table-driven tests that assert apply, revert, and agent write paths all agree for the same config inputs.

The Test Tooling Is Slow Blind and Manual

🐢 The repo’s tooling manages to be simultaneously expensive, under-scoped, and hand-maintained — a nasty little hat trick.

Every Jest invocation runs a full build first, even though plenty of unit tests import src directly while only CLI-style helpers actually depend on dist. So fast feedback gets dragged through a production build because the suite is one giant bucket of unrelated concerns.

Then lint ignores tests/**, and formatting is managed through a lovingly hand-assembled pile of path globs in package.json. So contributors get slower test runs, weaker static checks, and more script-maintenance chores. Truly premium developer experience. ✨🤡

Files

• jest.config.js
• jest.setup.js
• tests/unit/cli/commands.test.ts
• tests/harness.ts
• package.json

Code

• jest.config.js lines 27-28 — https://github.com/intellectronica/ruler/blob/main/jest.config.js#L27-L28

  // Build the project once before all tests run to prevent race conditions
  globalSetup: './jest.setup.js',

• jest.setup.js lines 3-8 — https://github.com/intellectronica/ruler/blob/main/jest.setup.js#L3-L8

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

• tests/unit/cli/commands.test.ts lines 1-3 — https://github.com/intellectronica/ruler/blob/main/tests/unit/cli/commands.test.ts#L1-L3

  import yargs from 'yargs';
  import { hideBin } from 'yargs/helpers';
  import { createCli, run } from '../../../src/cli/commands';

• tests/harness.ts lines 52-57 — https://github.com/intellectronica/ruler/blob/main/tests/harness.ts#L52-L57

  export function runRuler(command: string, projectRoot: string): string {
    const fullCommand = `node dist/cli/index.js ${command} --project-root ${projectRoot}`;
    return execSync(fullCommand, {
      stdio: 'pipe',
      encoding: 'utf8',
    });
  }

• package.json lines 8-12 — https://github.com/intellectronica/ruler/blob/main/package.json#L8-L12

  "lint": "eslint \"src/**/*.{ts,tsx}\"",
  "format": "prettier --write package.json package-lock.json tsconfig.json README.md .prettierrc.js eslint.config.mjs jest.config.js jest.setup.js \".github/**/*.yml\" \"src/**/*.{ts,tsx,json,md}\" \"tests/**/*.{ts,tsx,json,md}\"",
  "format:check": "prettier --check package.json package-lock.json tsconfig.json README.md .prettierrc.js eslint.config.mjs jest.config.js jest.setup.js \".github/**/*.yml\" \"src/**/*.{ts,tsx,json,md}\" \"tests/**/*.{ts,tsx,json,md}\"",
  "test": "jest --coverage",
  "test:watch": "jest --watch",

Concrete suggestions

• Split Jest into projects: fast unit tests on src, build-dependent CLI/integration tests separately.
• Lint tests/**/*.{ts,tsx} with test-specific ESLint overrides instead of pretending test code is decorative.
• Replace the giant Prettier allowlist with prettier --write . / --check . plus .prettierignore.