refactor: address review findings from Rovodev PR #1377 by dyoshikawa · Pull Request #1396 · dyoshikawa/rulesync

dyoshikawa · 2026-03-30T05:51:07Z

Summary

Can generate/import to gemini-cli #7 Case-insensitive blocklist: DISALLOWED_ROVODEV_MODULAR_RULE_BASENAMES now compares lowercased segments, preventing bypass on case-insensitive filesystems (macOS/Windows)
Add support for opencode-ai/opencode #5 Gitignore scope: moved **/AGENTS.local.md entry from common to rovodev target since it is Rovodev-specific
fix: add claudecode to valid targets in parser validation #2 DRY refactor: extracted buildDeletionRulesFromPaths helper in rules-processor.ts to deduplicate 4 instances of the file-discovery-to-deletion pattern
Add support for Openhands CLI #4 Readability: split RovodevRule.fromFile into fromModularFile / fromRootFile private helpers
Add support for gemini-cli #6 Clarifying comment: added note explaining nonRootPathsForImport is intentionally unused on the forDeletion path

Additional fixes (from PR self-review)

Add test cases for case-insensitive basename rejection (mixed-case variants)
Add JSDoc documenting two modes of buildDeletionRulesFromPaths (root vs non-root)
Move forDeletion check before nonRootPathsForImport filter to avoid unnecessary computation
Sync .gitignore via pnpm dev gitignore

Closes #1392

Test plan

pnpm cicheck passes (lint, typecheck, 4608 tests, cspell, secretlint)
Verify gitignore generation with pnpm dev gitignore produces correct output
Verify Rovodev rules generation works correctly in project and global modes

🤖 Generated with Claude Code

- Make DISALLOWED_ROVODEV_MODULAR_RULE_BASENAMES case-insensitive to prevent bypass on macOS/Windows filesystems (#7) - Scope AGENTS.local.md gitignore entry to rovodev target instead of common, since it is Rovodev-specific (#5) - Extract buildDeletionRulesFromPaths helper in rules-processor.ts to deduplicate the file-discovery-to-deletion pattern (#2) - Split RovodevRule.fromFile into fromModularFile/fromRootFile private helpers for improved readability (#4) - Add clarifying comment for nonRootPathsForImport unused on forDeletion path (#6) Closes #1392 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Refactors and hardens Rovodev-related rules handling in Rulesync, addressing follow-up review findings from Rovodev PR #1377 (including filesystem case-insensitivity and some internal cleanup).

Changes:

Added a buildDeletionRulesFromPaths helper in rules-processor.ts to deduplicate repeated “discover → validate → forDeletion → filter” logic.
Hardened Rovodev modular-rule reserved basename filtering to be case-insensitive (macOS/Windows) and refactored RovodevRule.fromFile into clearer private helpers.
Scoped **/AGENTS.local.md gitignore entry to the rovodev target (instead of common).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/features/rules/rules-processor.ts	Extracts a helper to reduce duplication in deletion-rule creation and clarifies import vs deletion filtering behavior.
src/features/rules/rovodev-rule.ts	Makes reserved-basename checks case-insensitive and splits `fromFile` into modular vs root helpers for readability.
src/cli/commands/gitignore-entries.ts	Moves `AGENTS.local.md` ignore entry from `common` to `rovodev` to limit scope.

Copilot · 2026-03-30T05:54:37Z

+            const relativeDirPath =
+              opts?.relativeDirPathOverride ?? resolveRelativeDirPath(filePath);
+            const relativeFilePath = opts?.baseDirOverride
+              ? relative(effectiveBaseDir, filePath)
+              : basename(filePath);


buildDeletionRulesFromPaths allows baseDirOverride without requiring relativeDirPathOverride, but in that case relativeDirPath still comes from resolveRelativeDirPath(filePath) (which is based on this.baseDir). That can produce an incorrect deletion path (duplicated subdirs) if a future call site passes only baseDirOverride. Consider either (a) computing relativeDirPath relative to effectiveBaseDir when baseDirOverride is set, or (b) asserting/enforcing that relativeDirPathOverride must be provided whenever baseDirOverride is used.

Copilot · 2026-03-30T05:54:38Z

    for (const segment of relativePath.split(/[/\\]/)) {
      if (segment === "" || segment === "." || segment === "..") {
        continue;
      }
-      if (DISALLOWED_ROVODEV_MODULAR_RULE_BASENAMES.has(segment)) {
+      if (DISALLOWED_ROVODEV_MODULAR_RULE_BASENAMES.has(segment.toLowerCase())) {
        return false;


The new case-insensitive reserved-name check isn’t currently covered by tests: existing specs only assert rejection for "AGENTS.md" / "AGENTS.local.md" exact casing. Add test cases for mixed/lowercase variants (e.g., "agents.md", "Agents.Local.MD", and nested segments) to prevent regressions on case-insensitive filesystems.

…d gitignore sync - Add test cases for case-insensitive basename rejection in isAllowedModularRulesRelativePath - Add JSDoc documenting two modes of buildDeletionRulesFromPaths - Move forDeletion check before nonRootPathsForImport filter to avoid unnecessary computation - Sync .gitignore via pnpm dev gitignore Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…d gitignore sync - Enforce both-or-neither opts in buildDeletionRulesFromPaths (#1) - Rename destructured param in fromRootFile for clarity (#2) - Remove redundant optional chaining after nonRoot guard (#3) - Remove duplicate **/.rovodev/ gitignore entry (#4) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Copilot · 2026-03-31T03:33:33Z

    feature: "general",
    entry: "**/.rovodev/AGENTS.md",
  },
+  { target: "rovodev", feature: "rules", entry: "**/AGENTS.local.md" },


The Rovodev target’s gitignore entries no longer cover the modular rules output directory (.rovodev/.rulesync/modular-rules/). Since RovodevRule.getSettablePaths() uses this location for non-root rules, generated modular rule files may now show up as unignored artifacts (previously masked by **/.rovodev/). Add an explicit ignore entry for .rovodev/.rulesync/modular-rules/ (likely under feature: "rules") and re-sync the generated .gitignore.

Suggested change

{ target: "rovodev", feature: "rules", entry: "**/AGENTS.local.md" },

{ target: "rovodev", feature: "rules", entry: "**/AGENTS.local.md" },

{

target: "rovodev",

feature: "rules",

entry: "**/.rovodev/.rulesync/modular-rules/",

},

Copilot · 2026-03-31T03:33:34Z

  // Rovodev
  {
    target: "rovodev",
    feature: "general",


**/.rovodev/AGENTS.md is tagged as feature: "general", but it’s the primary Rovodev rules output file (analogous to **/CLAUDE.md, **/GEMINI.md, etc. which are tagged as feature: "rules"). Because general entries are always included by filterGitignoreEntries, this makes it impossible to exclude Rovodev rules outputs when filtering by features. Consider tagging this entry as feature: "rules" for consistency and correct feature filtering behavior.

Suggested change

feature: "general",

feature: "rules",

…ev/.rulesync/ entry - Move **/AGENTS.local.md back to common target to ensure it is always gitignored regardless of whether rovodev target is enabled - Add **/.rovodev/.rulesync/ entry to cover generated modular-rules directory that was not covered by individual file entries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…nale comment for common AGENTS.local.md - Replace inconsistent opts?.baseDirOverride/relativeDirPathOverride branching with a single isNonRoot boolean guard for clarity - Add comment explaining why AGENTS.local.md is in common gitignore scope Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Copilot · 2026-03-31T04:16:49Z

+  // AGENTS.local.md is placed in common scope (not rovodev-only) so that
+  // local rule files are always gitignored regardless of which targets are enabled.
+  // This prevents accidental commits when a user disables the rovodev target.
  { target: "common", feature: "general", entry: "**/AGENTS.local.md" },


PR description says the **/AGENTS.local.md gitignore entry was moved from common to rovodev, but this change keeps it in common (and adds a comment justifying that choice). Please either update the PR description to match the implemented behavior, or move the entry to the rovodev target if that was the intended outcome.

dyoshikawa · 2026-03-31T04:26:21Z

@dyoshikawa Thank you!

Copilot AI review requested due to automatic review settings March 30, 2026 05:51

Copilot started reviewing on behalf of dyoshikawa March 30, 2026 05:51 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

dyoshikawa and others added 2 commits March 29, 2026 23:32

Copilot AI review requested due to automatic review settings March 31, 2026 03:29

Copilot started reviewing on behalf of dyoshikawa March 31, 2026 03:30 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

dyoshikawa and others added 2 commits March 30, 2026 21:02

Copilot AI review requested due to automatic review settings March 31, 2026 04:12

Copilot started reviewing on behalf of dyoshikawa March 31, 2026 04:13 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

dyoshikawa mentioned this pull request Mar 31, 2026

Rovodev rule: type guard extraction, closure testability, and minor cleanups #1402

Closed

dyoshikawa merged commit 8dc77fb into main Mar 31, 2026
14 checks passed

dyoshikawa deleted the issue-1392 branch March 31, 2026 04:26

Navigation Menu

Uh oh!

refactor: address review findings from Rovodev PR #1377 - #1396

refactor: address review findings from Rovodev PR #1377#1396
dyoshikawa merged 5 commits into
mainfrom
issue-1392

dyoshikawa commented Mar 30, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 30, 2026

Uh oh!

Copilot AI Mar 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Uh oh!

dyoshikawa commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dyoshikawa commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Additional fixes (from PR self-review)

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dyoshikawa commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dyoshikawa commented Mar 30, 2026 •

edited

Loading