This Codebase Smells! [2026-08-02]

[github-actions[bot]]

This Codebase Smells!

This week’s aroma profile is burnt path math, copy-paste marmalade, and a crisp finish of “ship it, probably” 💩. The tests are plentiful, the intentions are noble, and the implementation still manages to trip over its own shoelaces in impressively creative ways.

I found enough real problems that I never had to whine about formatting. That is, somehow, the meanest compliment I can give this repo 🤮.

Table of Contents

• String Prefix Path Checks Are Fake Containment
• Nested Discovery Ignores Git Worktrees
• Configuration Loading Is Duplicated and Expensive
• MCP Merge Is Not Actually a Merge
• Remote MCP Support Is Guesswork and Secret Spillage
• Agent Metadata Is Smeared Across Manual Registries
• CI and Release Automation Are Brittle and Wasteful

String Prefix Path Checks Are Fake Containment

🤡 startsWith() is not path containment, and duplicating the bug in both apply and revert does not make it “battle tested.”

selectRootConfiguration() in both flows treats path strings as if filesystems were just long tweets. A sibling like /repo/app2/.ruler can be mistaken for living under /repo/app, which means hierarchical root selection can drift into the wrong config scope.

The extra insult is that the repo already contains a correct helper for this exact problem. So this is not an unavoidable edge case; it is a self-inflicted paper cut with a staple gun.

Files

• src/lib.ts
• src/revert.ts
• src/core/path-utils.ts

Code

• src/lib.ts lines 445–454 (link)

  const normalizedProjectRoot = path.resolve(projectRoot);
  if (!normalizedDir.startsWith(normalizedProjectRoot)) {
    continue;
  }

• src/revert.ts lines 325–334 (link)

  const normalizedProjectRoot = path.resolve(projectRoot);
  if (!normalizedDir.startsWith(normalizedProjectRoot)) {
    continue;
  }

• src/core/path-utils.ts lines 3–15 (link)

  const relative = path.relative(path.resolve(parentPath), path.resolve(targetPath));
  return relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative));

Suggestions

• Replace both startsWith() checks with isPathInsideOrEqual().
• Extract selectRootConfiguration() into one shared helper instead of copy-pasting the same mistake twice.
• Add regression tests for sibling-prefix cases like /repo/app vs /repo/app2.

Nested Discovery Ignores Git Worktrees

☠️ Nested scanning only respects .git directories, which is adorable if you’ve never met a worktree, submodule, or modern Git setup.

findAllRulerDirs() stops traversal at nested repos only when .git is a directory. Linked worktrees commonly use a .git file with a gitdir: pointer, so nested discovery can wander across repository boundaries and start “helping” where it absolutely should not.

What makes this smell especially ripe is that GitignoreUtils already handles .git files correctly. The codebase knows the rule in one place and forgets it in another, like a goldfish with commit access.

Files

• src/core/FileSystemUtils.ts
• src/core/GitignoreUtils.ts

Code

• src/core/FileSystemUtils.ts lines 590–603 (link)

  const gitDir = path.join(fullPath, '.git');
  const gitStat = await fs.stat(gitDir);
  if (gitStat.isDirectory() && path.resolve(fullPath) !== rootPath) {
    continue;
  }

• src/core/GitignoreUtils.ts lines 117–119 (link)

  * Resolves ignore files Ruler manages. Linked worktrees store `.git` as a
  * file containing a `gitdir:` pointer, so `.git/info/exclude` must be resolved
  * through that pointer.

• src/core/GitignoreUtils.ts lines 152–160 (link)

  if (dotGitStat.isFile()) {
    const dotGitContent = await fs.readFile(dotGitPath, 'utf8');
    const gitDirMatch = dotGitContent.match(/^gitdir:\s*(.+)\s*$/m);
  }

Suggestions

• Switch nested-boundary detection to lstat() and handle .git files with gitdir: pointers.
• Extract a shared “resolve Git boundary” helper and reuse it in both discovery and ignore-file logic.
• Add tests for linked worktrees and submodules, not just vanilla .git/ repos from 2009.

Configuration Loading Is Duplicated and Expensive

💸 The apply path loads config twice, re-reads rules twice, and maintains two overlapping config models because apparently I/O grows on trees.

loadSingleConfiguration() calls loadConfig(), reads markdown files, then imports and calls loadUnifiedConfig(), which goes right back to disk for TOML and markdown again. In nested mode, this little circus repeats per .ruler directory.

You also have LoadedConfig and RulerUnifiedConfig living side-by-side, which is a fantastic way to guarantee drift over time. Two sources of truth are not “flexibility”; they are an argument waiting to happen.

Files

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

Code

• src/core/apply-engine.ts lines 337–359 (link)

  const config = await loadConfig({ ... });
  
  const files = await FileSystemUtils.readMarkdownFiles(rulerDirs[0], { ... });
  
  const { loadUnifiedConfig } = await import('./UnifiedConfigLoader');
  const unifiedConfig = await loadUnifiedConfig({ ... });

• src/core/ConfigLoader.ts lines 298–306 (link)

  export interface LoadedConfig {
    defaultAgents?: string[];
    agentConfigs: Record<string, IAgentConfig>;
    cliAgents?: string[];
    mcp?: GlobalMcpConfig;
  }

• src/core/UnifiedConfigTypes.ts lines 9–16 (link)

  export interface RulerUnifiedConfig {
    meta: ConfigMeta;
    toml: TomlConfig;
    rules: RulesBundle;
    mcp: McpBundle | null;
  }

• src/core/UnifiedConfigLoader.ts lines 243–279 (link)

  const mdFiles = await FileSystemUtils.readMarkdownFiles(meta.rulerDir, {
    includeAgents: includeAgentsInRules,
  });
  
  const concatenated = concatenateRules(...)

Suggestions

• Make one loader canonical; derive legacy projections from it instead of parsing the world twice.
• Pass pre-read rule files and config state through the apply pipeline instead of rediscovering them downstream.
• Add a nested-mode perf test or trace assertion so this kind of duplicate I/O gets caught instead of quietly fermenting.

MCP Merge Is Not Actually a Merge

🧨 mergeMcp() does a shallow server replacement, so changing one field can silently nuke the rest of that server’s config.

Right now, merging is just { ...baseServers, ...incomingServers }. If a base server has env, headers, or timeout, and the incoming config only changes one piece, the whole original server object gets replaced. Congratulations: your “merge” is a polite overwrite wearing a fake moustache.

This is the kind of bug that looks fine in happy-path tests, then eats somebody’s auth headers in production and shrugs.

Files

• src/mcp/merge.ts

Code

• src/mcp/merge.ts lines 68–76 (link)

  const baseServers = collectMcpServers(base, serverKey);
  const incomingServers = collectMcpServers(incoming, serverKey);
  
  const mergedServers = { ...baseServers, ...incomingServers };

Suggestions

• Deep-merge per-server objects instead of replacing them wholesale.
• If full replacement is sometimes intended, make it explicit with a per-server overwrite flag.
• Add tests for partial overrides of env, headers, args, and timeout.

Remote MCP Support Is Guesswork and Secret Spillage

🔐 Remote MCP handling currently combines protocol roulette, runtime @latest dependencies, and credential leakage into argv — a truly cursed little trifecta.

When an agent lacks remote support, the code rewrites remote servers to npx -y mcp-remote@latest ... --header .... That means secrets can land in process arguments, and behavior depends on whatever the npm registry thinks “latest” means today. Spectacularly bad idea.

Elsewhere, OpenHands decides whether a server is sse or shttp by regexing the URL for /sse/, rejects non-Bearer/custom headers outright, and Mistral claims streamable-http support while always emitting http. That is not protocol support; that is improv comedy with auth tokens.

Files

• src/mcp/capabilities.ts
• src/mcp/propagateOpenHandsMcp.ts
• src/agents/MistralVibeAgent.ts

Code

• src/mcp/capabilities.ts lines 29–36 (link)

  return Object.entries(headers).flatMap(([key, value]) =>
    typeof value === 'string' ? ['--header', `${key}: ${value}`] : [],
  );

• src/mcp/capabilities.ts lines 87–90 (link)

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

• src/mcp/propagateOpenHandsMcp.ts lines 40–42 (link)

  function classifyRemoteServer(url: string): 'sse' | 'shttp' {
    return /\/sse(\/|$)/i.test(url) ? 'sse' : 'shttp';
  }

• src/mcp/propagateOpenHandsMcp.ts lines 84–86 (link)

  throw new Error(
    `OpenHands MCP remote server "${name}" has unsupported headers...`,
  );

• src/agents/MistralVibeAgent.ts lines 209–218 (link)

  private determineTransport(...): 'stdio' | 'http' | 'streamable-http' {
    if (server.url) {
      return 'http';
    }
  }

• src/agents/MistralVibeAgent.ts lines 234–235 (link)

  return true; // Mistral Vibe supports http and streamable-http transports

Suggestions

• Introduce one shared remote-server type with explicit transport metadata instead of guessing from URL shapes.
• Stop putting secrets in argv; use env vars, temp config files, or native header fields.
• Pin or vendor the bridge tool instead of invoking mcp-remote@latest.
• Add end-to-end tests for sse, streamable-http, and non-Bearer/custom-header cases.

Agent Metadata Is Smeared Across Manual Registries

🫠 Adding an agent currently means editing a scavenger hunt of arrays, switches, maps, and constants, because one source of truth was apparently too mainstream.

The same metadata is repeated across allAgents, MCP path resolution, skill-target mapping, and path constants. That means every new adapter is a four-file memory test, and every missed edit becomes another weird “why does this one agent behave differently?” bug report.

This is classic drift bait. The code is manually synchronizing facts that should be declarative. It works right up until it very much does not, which is a bold engineering lifestyle choice.

Files

• src/agents/index.ts
• src/paths/mcp.ts
• src/core/SkillsProcessor.ts
• src/constants.ts

Code

• src/agents/index.ts lines 38–71 (link)

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

• src/paths/mcp.ts lines 64–90 (link)

  const candidates: string[] = [];
  switch (adapterName) {
    case 'GitHub Copilot':
      candidates.push(path.join(projectRoot, '.mcp.json'));
      break;

• src/core/SkillsProcessor.ts lines 159–173 (link)

  const SKILL_TARGET_TO_IDENTIFIERS = new Map<SkillTarget, readonly string[]>([
    ['claude', ['claude', 'copilot', 'kilocode']],
    ['goose', ['goose', 'amp', 'zed']],
    ['vibe', ['mistral']],
  ]);

• src/constants.ts lines 57–70 (link)

  export const CLAUDE_SKILLS_PATH = '.claude/skills';
  export const CODEX_SKILLS_PATH = '.agents/skills';
  export const GOOSE_SKILLS_PATH = '.agents/skills';
  export const VIBE_SKILLS_PATH = '.vibe/skills';

Suggestions

• Create a single AgentDescriptor registry with identifier, display name, factory, MCP-path resolver, skills target, and capability flags.
• Derive allAgents, CLI help, path resolution, and skills propagation from that registry.
• Add an exhaustiveness test so a new agent cannot be added half-wired.

CI and Release Automation Are Brittle and Wasteful

🚨 The workflows are a glorious mash-up of happy-path assumptions, hardcoded repo identity, and a completely unnecessary coverage tax.

release.yml only blocks prereleases when EVENT_NAME=release, so a manual workflow_dispatch recovery run can still publish a prerelease version to npm latest if somebody supplies the right tag. That safety rail is decorative. Meanwhile the smell-report workflow hardcodes intellectronica/ruler twice, because apparently forks and repo renames are for peasants.

On top of that, npm test always means jest --coverage, so every normal test run in CI drags instrumentation around like a dead refrigerator. You have separate jobs and matrices; you do not need to pay the coverage bill in every single lane.

Files

• .github/workflows/release.yml
• .github/workflows/this-codebase-smells.yml
• package.json
• .github/workflows/ci.yml

Code

• .github/workflows/release.yml lines 4–15 (link)

  release:
    types: [published]
  workflow_dispatch:
    inputs:
      release_tag:
      release_sha:

• .github/workflows/release.yml lines 48–56 (link)

  if [ "${EVENT_NAME}" = "release" ] && [ "${RELEASE_PRERELEASE}" = "true" ]; then
    echo "::error::GitHub prereleases must not be published to npm latest."
    exit 1
  fi

• .github/workflows/this-codebase-smells.yml lines 99–100 (link)

  owner="intellectronica"
  repo="ruler"

• .github/workflows/this-codebase-smells.yml lines 118–120 (link)

  owner="intellectronica"
  repo="ruler"
  repo_id=$(gh api "repos/$owner/$repo" --jq .node_id)

• package.json lines 11–14 (link)

  "test": "jest --coverage",
  "test:watch": "jest --watch",
  "test:coverage": "jest --coverage",
  "test:integration": "jest tests/e2e tests/integration --verbose"

• .github/workflows/ci.yml lines 28–33 (link)

  - run: npm ci
  - run: npm run check:package-lock
  - run: npm run format:check
  - run: npm run lint
  - run: npm test
  - run: npm run build

Suggestions

• Reject prerelease versions in workflow_dispatch too, or require an explicit npm dist-tag and publish with --tag.
• Replace hardcoded owner/repo values with ${{ github.repository }} or derived shell variables from that context.
• Split fast test from test:coverage, then run coverage in exactly one CI lane instead of all of them.
• Add workflow tests for manual-release prerelease paths and repo rename/fork behavior, because these workflows currently trust vibes more than logic.