This Codebase Smells! [2026-07-26] #748

2026-07-26T14:33:11Z

github-actions[bot]
Bot Jul 26, 2026

This Codebase Smells!

This week’s bouquet is hot plastic, stale YAML, and the unmistakable musk of startsWith() pretending to understand filesystem boundaries 🤮. The project is ambitious, sure, but several core paths are held together with duplicated logic, nested-mode wishful thinking, and cleanup code that behaves like a raccoon got root access and a personal vendetta.

Prefix Based Path Checks Are Cosplaying as Security

String-prefix containment checks are a bug farm, not path safety, and this repo keeps planting crops in it 💩.

startsWith() is being used to decide whether one path is “inside” another. That means /repo/app also matches /repo/application, because apparently filesystem boundaries are now just vibes.
The same stunt appears in both root-config selection and parent-directory cleanup, so this is not one bad line; it is a small philosophy of bad lines.

Files

src/lib.ts
src/revert.ts
src/core/SkillsProcessor.ts
src/core/SubagentsProcessor.ts

Code

src/lib.ts L441-L450 — link

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

src/revert.ts L313-L322 — link

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

src/core/SkillsProcessor.ts L365-L371 — link

let current = startDir;
while (current !== boundaryDir && current.startsWith(boundaryDir)) {
  await removeDirectoryIfEmpty(current);

src/core/SubagentsProcessor.ts L234-L240 — link

let current = startDir;
while (current !== boundaryDir && current.startsWith(boundaryDir)) {
  await removeDirectoryIfEmpty(current);
}

Suggestions

Replace every path-containment startsWith() check with one shared helper based on path.relative() or the existing isPathInsideOrEqual.
Use the same helper for root selection, cleanup boundaries, and any future symlink-sensitive path logic.
Add regression cases for sibling-prefix paths like /repo/app vs /repo/application.

Nested Mode Is a Root Config Monarchy

Nested mode claims to support per-directory behavior, then quietly lets the root config rule everything like a tiny authoritarian goblin 👑.

Nested apply resolves subagents and backup behavior from rootConfig, so child .ruler directories cannot actually control those settings independently.
Nested revert resolves the agent list once from the root and reuses it for every child config, which means child-only agents can leave generated junk behind. Magnificent. No notes. Actually many notes.

Files

src/lib.ts
src/revert.ts

Code

src/lib.ts L248-L259 — link

const subagentsEnabledResolved = resolveSubagentsEnabled(
  subagentsEnabled,
  rootConfig.subagents?.enabled,
);
const backupEnabledResolved = resolveBackupEnabled(
  backup,
  rootConfig.backup?.enabled,
);

src/revert.ts L204-L208 — link

const rootConfigEntry = selectRootConfiguration(configurations, projectRoot);
const rootConfig = rootConfigEntry.config;
rootConfig.cliAgents = includedAgents;

const selected = resolveSelectedAgents(rootConfig, agents);

src/revert.ts L223-L235 — link

for (const configEntry of configurations) {
  const effectiveProjectRoot = configEntry.projectRoot;
  ...
  for (const agent of selected) {
    ...
    const agentConfig =
      configEntry.config.agentConfigs[agent.getIdentifier()];

Suggestions

Build a per-directory execution plan that resolves CLI overrides against each nested config entry separately.
Carry selectedAgentsByRulerDir through both apply and revert instead of one global selected.
Resolve backup, subagents.enabled, and cleanup_orphaned per nested config, not once from the root.

Revert Cleanup Thinks Config Toml Is Trash

The cleanup code hard-codes generic filenames like config.toml, which is the kind of idea you get right before a postmortem ☠️.

Yes, there are provenance checks, so this is not instant data loss. But the design is still reckless: generic project files are in the deletion candidate list at all.
Open Hands deliberately maps MCP config to root config.toml, so the blast radius includes an utterly normal filename many real projects already use. Fantastic footgun engineering.

Files

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

Code

src/core/revert-engine.ts L521-L528 — link

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

src/paths/mcp.ts L88-L90 — 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'));

Suggestions

Stop deleting generic filenames from a hard-coded list; delete only files recorded in a Ruler-owned manifest or bearing explicit provenance.
For generic files like config.toml, require both provenance and recognizable managed content before removal.
Add revert tests covering pre-existing root config.toml files with and without Ruler-managed sections.

MCP Parsing Has Been Copy Pasted Into a Small Religion

MCP normalization lives in multiple loaders with nearly identical conflict rules, which is how “one tiny change” becomes a bug lottery 🤮.

ConfigLoader normalizes agent-scoped MCP servers. UnifiedConfigLoader separately normalizes TOML servers and then does it again for legacy JSON servers. Same ideas, same conflicts, same type derivation, same opportunity for drift.
This is exactly the sort of duplication that breeds subtle divergence: one path strips a field, another forgets, a third changes warning text, and suddenly your config behavior depends on which codepath sneezed first.

Files

src/core/ConfigLoader.ts
src/core/UnifiedConfigLoader.ts

Code

src/core/ConfigLoader.ts L228-L245 — link

if (!hasCommand && !hasUrl) {
  diagnostics.push({
    severity: 'warning',
    code: 'MCP_AGENT_INVALID_SERVER',
    ...
  });
  return { diagnostics };
}

if (hasCommand && hasUrl) {

src/core/ConfigLoader.ts L280-L289 — link

if (hasCommand && hasUrl) {
  delete server.command;
  delete server.args;
  delete server.env;
  server.type = 'remote';
} else if (hasUrl) {
  delete server.args;
  server.type = 'remote';
} else if (hasCommand) {
  server.type = 'stdio';
}

src/core/UnifiedConfigLoader.ts L315-L329 — link

if (!hasCommand && !hasUrl) {
  diagnostics.push({
    severity: 'warning',
    code: 'MCP_TOML_INVALID_SERVER',
    message: `MCP server '${name}' must have at least one of command or url`,

src/core/UnifiedConfigLoader.ts L352-L363 — link

if (hasCommand && hasUrl) {
  delete server.command;
  delete server.args;
  delete server.env;
}

if (server.url) {
  server.type = 'remote';
} else if (server.command) {
  server.type = 'stdio';
}

src/core/UnifiedConfigLoader.ts L478-L492 — link

if (!hasCommand && !hasUrl) {
  diagnostics.push({
    severity: 'warning',
    code: 'MCP_JSON_INVALID_SERVER',
    message: `MCP server '${name}' must have at least one of command or url`,

Suggestions

Extract one shared normalizeMcpServer() utility that returns normalized data plus diagnostics.
Feed it context (agent, toml, legacy-json) instead of maintaining parallel logic branches.
Centralize warning-code generation so behavior changes happen once, not in a three-headed hydra.

Skills and Subagents Share a Twin They Pretend Not to Know

The skills and subagents processors each reinvent the same manifest and cleanup machinery, because apparently copy-paste is now a platform strategy 🫠.

Both files define the same managed-manifest name, the same relative-path sanitizer, the same manifest reader, the same empty-parent cleanup loop, and the same manifest write/remove workflow.
That means every safety fix has to be remembered twice. Which, in software terms, means it will be remembered once and forgotten once. Classic.

Files

src/core/SkillsProcessor.ts
src/core/SubagentsProcessor.ts

Code

src/core/SkillsProcessor.ts L326-L350 — link

function isSafeManagedRelativePath(relativePath: string): boolean {
  return (
    relativePath.length > 0 &&
    !path.isAbsolute(relativePath) &&
    !relativePath.split(path.sep).includes('..') &&
    !relativePath.split('/').includes('..')
  );
}
...
return parsed.paths.filter(

src/core/SubagentsProcessor.ts L195-L219 — link

function isSafeManagedRelativePath(relativePath: string): boolean {
  return (
    relativePath.length > 0 &&
    !path.isAbsolute(relativePath) &&
    !relativePath.split(path.sep).includes('..') &&
    !relativePath.split('/').includes('..')
  );
}
...
return parsed.paths.filter(

src/core/SkillsProcessor.ts L414-L433 — link

async function writeManagedManifest(
  targetDir: string,
  managedPaths: string[],
  projectRoot: string,
): Promise<void> {
  const manifestPath = path.join(targetDir, RULER_MANAGED_MANIFEST);
  ...
  await fs.writeFile(
    manifestPath,
    `${JSON.stringify(manifest, null, 2)}\n`,

src/core/SubagentsProcessor.ts L292-L311 — link

async function writeManagedManifest(
  targetDir: string,
  managedPaths: string[],
  projectRoot: string,
): Promise<void> {
  const manifestPath = path.join(targetDir, RULER_MANAGED_MANIFEST);
  ...
  await fs.writeFile(
    manifestPath,
    `${JSON.stringify(manifest, null, 2)}\n`,

Suggestions

Extract shared managed-directory utilities for manifest IO, safe path filtering, entry removal, and empty-parent cleanup.
Keep only behavior-specific differences in the skills/subagents layers: discovery, transformation, and collision policy.
Consolidate tests around one shared filesystem contract so future fixes stop breeding twins.

TypeScript Is Politely Ignoring the Test Suite

The repo has a giant tests/ tree, and the compiler is explicitly told to avert its eyes like this is all somehow normal 🙃.

tsc only includes src, and tests is explicitly excluded. So compile-time type feedback skips the entire test harness; type breakage is discovered only when Jest happens to execute the right file.
With this many tests, that is lousy DX and weaker refactor safety. You built a second codebase and then told TypeScript to wait outside.

Files

tsconfig.json
package.json

Code

tsconfig.json L113-L114 — link

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

package.json L16-L20 — link

"build": "npm run clean && tsc",
"postbuild": "node -e \"require('node:fs').chmodSync('dist/cli/index.js', 0o755)\"",
"check:package-lock": "...",
"prepare": "npm run build",
"prepublishOnly": "npm run build"

Suggestions

Add a tsconfig.tests.json that extends the main config with noEmit and includes tests.
Run tsc -p tsconfig.tests.json in CI alongside the publish/build config.
Keep emit-focused packaging config separate from full-project type checking so install/publish concerns stop dictating editor and test safety.

Navigation Menu

Repository navigation

This Codebase Smells! [2026-07-26] #748

Uh oh!

{{title}}

Uh oh!

Replies: 0 comments

Select a reply

Uh oh!

This Codebase Smells! [2026-07-26] #748

Uh oh!

github-actions[bot] Bot Jul 26, 2026

This Codebase Smells!

Table of Contents

Prefix Based Path Checks Are Cosplaying as Security

Files

Code

Suggestions

Nested Mode Is a Root Config Monarchy

Files

Code

Suggestions

Revert Cleanup Thinks Config Toml Is Trash

Files

Code

Suggestions

MCP Parsing Has Been Copy Pasted Into a Small Religion

Files

Code

Suggestions

Skills and Subagents Share a Twin They Pretend Not to Know

Files

Code

Suggestions

TypeScript Is Politely Ignoring the Test Suite

Files

Code

Suggestions

Replies: 0 comments

github-actions[bot]
Bot Jul 26, 2026