This Codebase Smells! [2025-11-02] #286

2025-11-02T15:05:38Z

github-actions[bot]
bot Nov 2, 2025

This Codebase Smells!

It’s that time again: I opened this repo, took a deep breath, and promptly regretted every life choice that led me here. The code is 60% good intentions, 30% duct tape, and 10% “surely no one will ever do that” optimism. I’m here to be your brutally honest rubber duck with a flamethrower. Buckle up, buttercup. 💣🔥

Overzealous Revert Deletes Real Files

☠️ Revert eagerly deletes config.toml as “extra cleanup,” even when it’s a legit project file.

The “clean up extra agent files” list includes a generic config.toml — like, universally common config name; what could go wrong? Everything. 💥 If you’ve ever used TOML for anything else (you have), Revert can nuke it with no mercy when there’s no .bak.
The “Open Hands” adapter points directly to project-root config.toml. If that path exists without a Ruler backup, it gets unlinked. Hope you didn’t need your configuration! 🤡

Files

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

Code

Hard-coded deletion list includes config.toml:
- src/core/revert-engine.ts:311-318
  - https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L311-L318
```
const additionalFiles = [
  '.gemini/settings.json',
  '.mcp.json',
  '.vscode/mcp.json',
  '.cursor/mcp.json',
  '.kilocode/mcp.json',
  'config.toml',
];
```

Open Hands targets project-root config.toml:

src/paths/mcp.ts:33-36

https://github.com/intellectronica/ruler/blob/main/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'));

Suggestions

Only delete files that:
- Have a .bak, or
- Are listed in a manifest produced by apply, or
- Contain a clear Ruler provenance marker comment.
Gate config.toml removal behind “contains OpenHands MCP markers” or backup presence only; otherwise skip with a warning.
Add a --force-extra-cleanup flag to enable aggressive deletions; default to safe behavior.
Log loudly when skipping deletion of generic files; this is not something to be “quiet” about.

Dry‑Run Logging Bug in Revert

🤦 Template literals log the function actionPrefix itself instead of the computed prefix — the logs are nonsense, like your linter gave up.

Several places interpolate ${actionPrefix} instead of the local prefix value. In dry-run mode, logs read like a stack trace had a stroke. This makes debugging revert behavior super fun. Not.
The correct pattern is used elsewhere in the file. Copy/paste claimed another victim.

Files

src/core/revert-engine.ts

Code

Misused ${actionPrefix} instead of ${prefix}:
- src/core/revert-engine.ts:366-369
  - https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L366-L369
```
logVerbose(
  `${actionPrefix} Restored VSCode settings from backup`,
  verbose,
);
```
- src/core/revert-engine.ts:379-386
  - https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L379-L386
```
logVerbose(
  `${actionPrefix} Would remove empty VSCode settings file`,
  verbose,
);
...
`${actionPrefix} Would remove augment.advanced section from ${settingsPath}`
```
- src/core/revert-engine.ts:399-406
  - https://github.com/intellectronica/ruler/blob/main/src/core/revert-engine.ts#L399-L406
```
logVerbose(
  `${actionPrefix} Removed empty VSCode settings file`,
  verbose,
);
...
`${actionPrefix} Removed augment.advanced section from VSCode settings`
```

Suggestions

Replace all ${actionPrefix} with a local prefix = actionPrefix(dryRun) and interpolate ${prefix}.
Add a unit test that captures logged output for dry-run vs non-dry-run to prevent this regression.
Consider a tiny logger wrapper that returns a bound logger object with mode baked-in to kill this class of bugs.

Recursive Scans Dive Into node_modules

🐌 Your recursive .ruler discovery happily spelunks through node_modules and friends. Performance bellyflop incoming.

The directory walk skips “hidden” dirs only, but not the common megafolders like node_modules, dist, build, etc. Enjoy your accidental 150k-directory traversal every time. 😵‍💫
This impacts commands that rely on hierarchical discovery and any read of markdown under .ruler.

Files

src/core/FileSystemUtils.ts

Code

Only hidden dirs are skipped:

src/core/FileSystemUtils.ts:215-221

https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L215-L221

} else {
  // Recursively search subdirectories (but skip hidden directories like .git)
  if (!entry.name.startsWith('.')) {
    await findRulerDirs(fullPath);
  }
}

Suggestions

Exclude known heavy dirs by default: node_modules, dist, build, out, .next, .turbo, target, vendor, coverage, etc.
Add a glob-based “ignore” option to the scan and pipe CLI flags through to it.
Cap recursion depth or file count in pathological repos; warn when caps are hit.

Ambiguous Agent Config Mapping Clobbers Settings

🎯 Substring matching between config keys and agent display names is a landmine of accidental matches.

mapRawAgentConfigs maps by exact identifier OR substring of display name. If multiple display names include the substring (shockingly common), last write wins. Silent override party! 🎉
This behavior is unpredictable and almost certainly not what users intend. The function doesn’t even stop at the first match — it keeps bulldozing.

Files

src/core/config-utils.ts

Code

Substring match + no early return:
- src/core/config-utils.ts:25-31
  - https://github.com/intellectronica/ruler/blob/main/src/core/config-utils.ts#L25-L31
```
if (
  identifier === lowerKey ||
  agent.getName().toLowerCase().includes(lowerKey)
) {
  mappedConfigs[identifier] = cfg;
}
```

Suggestions

Match only by exact identifier (e.g., copilot, claude, etc.). Drop name-based substring heuristics — they’re chaos.
If you keep display-name matching, require full-word matching and error on ambiguity. Log a warning if multiple candidates exist.
Add tests for ambiguous cases to lock behavior down.

Global Config Logging Is Noisy Footgun

📣 On failure to stat the global config dir, you console.error the OS path every time. It’s not an error; it’s expected.

When local .ruler isn’t found and global config doesn’t exist, findRulerDir logs a scary error even though it’s a totally normal situation. Hello, noise. 🥁
This pollutes stderr and makes real problems harder to spot.

Files

src/core/FileSystemUtils.ts

Code

Noisy error logging on normal absence:
- src/core/FileSystemUtils.ts:48-53
  - https://github.com/intellectronica/ruler/blob/main/src/core/FileSystemUtils.ts#L48-L53
```
} catch (err) {
  console.error(
    `[ruler] Error checking global config directory ${globalConfigDir}:`,
    err,
  );
}
```

Suggestions

Log this under verbose only, or downgrade to a quiet warning that doesn’t include the error object.
Return null silently; normal flow should not shout.

Node Compatibility: structuredClone Usage

🪵 Using structuredClone can explode on older Node runtimes. Enjoy the runtime faceplant.

structuredClone is used to copy settings before merge. That’s cute, but Node 16 users won’t find it funny when it’s undefined at runtime. 🤡
There’s no polyfill or fallback. If you didn’t set “engines” to enforce Node ≥18, this is a compatibility booby trap.

Files

src/vscode/settings.ts
package.json (no engines field)

Code

Use of structuredClone:

src/vscode/settings.ts:101-107

https://github.com/intellectronica/ruler/blob/main/src/vscode/settings.ts#L101-L107

export function mergeAugmentMcpServers(
  existingSettings: VSCodeSettings,
  newServers: AugmentMcpServer[],
  strategy: McpStrategy,
): VSCodeSettings {
  const result = structuredClone(existingSettings);

No Node engine requirement:
- package.json
  - https://github.com/intellectronica/ruler/blob/main/package.json
```
{
  "name": "@intellectronica/ruler",
  ...
  "dependencies": {
    ...
  }
}
```

Suggestions

Either:
- Add engines with "node": ">=18" and enforce via CI, or
- Replace with a safe deep clone: JSON.parse(JSON.stringify(existingSettings)) for this config shape.
Add a tiny helper clone<T>(obj: T): T to centralize the approach and future-proof it.

If you were hoping this would be a love letter, sorry. But good news: these issues are fixable, and the fixes are small and independent. Do them, and your repo will smell less like a burning datacenter and more like a mildly stressed developer’s desk plant. 🌿✨

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

This Codebase Smells! [2025-11-02] #286

Uh oh!

{{title}}

Uh oh!

Uh oh!

{{editor}}'s edit

{{editor}}'s edit

Uh oh!

Replies: 0 comments

Select a reply

Uh oh!

This Codebase Smells! [2025-11-02] #286

Uh oh!

Uh oh!

github-actions[bot] bot Nov 2, 2025

This Codebase Smells!

Table of Contents

Overzealous Revert Deletes Real Files

Files

Code

Suggestions

Dry‑Run Logging Bug in Revert

Files

Code

Suggestions

Recursive Scans Dive Into node_modules

Files

Code

Suggestions

Ambiguous Agent Config Mapping Clobbers Settings

Files

Code

Suggestions

Global Config Logging Is Noisy Footgun

Files

Code

Suggestions

Node Compatibility: structuredClone Usage

Files

Code

Suggestions

Replies: 0 comments

github-actions[bot]
bot Nov 2, 2025