feat: show gitignore prompt only when changes would be made #22
Conversation
WalkthroughAdds a pre-check to detect whether exported paths introduce new .gitignore patterns, makes updateGitignoreWithPaths return a boolean indicating if .gitignore changed, and updates CLI control flow and logging to prompt or write only when new patterns exist; includes tests for these scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Export
participant Checker as checkForNewGitignorePatterns()
participant Updater as updateGitignoreWithPaths()
participant Gitignore as .gitignore
User->>CLI: export (paths + flags)
CLI->>Checker: checkForNewGitignorePatterns(repoPath, paths)
alt New patterns found
Checker-->>CLI: true
CLI->>User: prompt to add (unless --no-gitignore)
alt Confirmed or --gitignore
CLI->>Updater: updateGitignoreWithPaths(repoPath, paths)
Updater->>Gitignore: write new patterns
Updater-->>CLI: true (changed)
CLI-->>User: "gitignore updated"
else Declined or --no-gitignore
CLI-->>User: skip update
end
else No new patterns
Checker-->>CLI: false
CLI-->>User: skip prompt/update (no changes)
end
CLI-->>User: export completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/integration/roundtrip.test.ts (1)
25-27: Avoid require() in ESM/TS testThis test file uses ESM imports; require may be undefined under Vitest ESM. Import mkdirSync from node:fs at top instead.
- const { mkdirSync } = require('fs'); - mkdirSync(agentDir, { recursive: true }); + import { mkdirSync } from 'node:fs' + mkdirSync(agentDir, { recursive: true })test/export-functionality.test.ts (1)
139-141: Use path.dirname; remove custom helper (cross‑platform bug)String‑split dirname breaks on Windows paths. Prefer Node’s path.dirname.
- const exportedFiles = [ + const exportedFiles = [ @@ - const fullPath = join(tempDir, file) - expect(existsSync(fullPath) || existsSync(dirname(fullPath))).toBe(true) + const fullPath = join(tempDir, file) + expect(existsSync(fullPath) || existsSync(require('path').dirname(fullPath))).toBe(true) }) }) }) -// Helper to get dirname -function dirname(path: string): string { - const parts = path.split('/') - return parts.slice(0, -1).join('/') -}Or better:
-import { join } from 'path' +import { join, dirname } from 'path' @@ - expect(existsSync(fullPath) || existsSync(dirname(fullPath))).toBe(true) + expect(existsSync(fullPath) || existsSync(dirname(fullPath))).toBe(true) @@ -// Helper to get dirname -function dirname(path: string): string { - const parts = path.split('/') - return parts.slice(0, -1).join('/') -} +// (helper removed)Also applies to: 166-170
src/cli.ts (1)
384-384: Auto-detection always maps AGENTS.md to 'codex', never 'opencode'.Line 384 hardcodes
AGENTS.mddetection as 'codex' format. Since both codex and opencode useAGENTS.md(per the export cases at lines 282-286 and 302-306), users cannot convertAGENTS.mdfiles to opencode format using auto-detection.Consider adding logic to differentiate between the two formats or defaulting to the newer format (opencode).
🧹 Nitpick comments (4)
test/export-functionality.test.ts (1)
153-154: Mapping both 'codex' and 'opencode' to AGENTS.md can be ambiguousBoth formats point to the same file; ensure exporter doesn’t write it twice (see exporters.ts). If keeping both, document precedence.
src/importers.ts (2)
671-696: Near‑duplicate of importCodex — extract a helperimportOpenCode mirrors importCodex. Extract a small importSingleFile(format, id, description) to reduce drift.
+function importSingleFile(format: ImportResult['format'], filePath: string, id: string, description: string): ImportResult { + const content = readFileSync(filePath, 'utf-8') + const isPrivateFile = isPrivateRule(filePath) + const metadata: any = { id, alwaysApply: true, description } + if (isPrivateFile) metadata.private = true + return { format, filePath, rules: [{ metadata, content: content.trim() }], raw: content } +} @@ -export function importOpenCode(filePath: string): ImportResult { - const content = readFileSync(filePath, 'utf-8') - const isPrivateFile = isPrivateRule(filePath) - const metadata: any = { - id: 'opencode-agents', - alwaysApply: true, - description: 'OpenCode agents and instructions' - } - if (isPrivateFile) { - metadata.private = true - } - const rules: RuleBlock[] = [{ metadata, content: content.trim() }] - return { format: 'opencode', filePath, rules, raw: content } -} +export function importOpenCode(filePath: string): ImportResult { + return importSingleFile('opencode', filePath, 'opencode-agents', 'OpenCode agents and instructions') +}
896-900: Unnecessary any-cast on formattypes include 'junie'; drop the as any.
- format: 'junie' as any, + format: 'junie',src/exporters.ts (1)
46-96: Link paths may not match private-rule filenamesConditional section links to .agent/${id}.md, but private rules are written under .agent/private/NNN-id.md. If includePrivate is true, links will 404. Consider mapping IDs to actual emitted paths and use those.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(5 hunks)src/cli.ts(10 hunks)src/exporters.ts(4 hunks)src/importers.ts(2 hunks)src/index.ts(2 hunks)src/types.ts(2 hunks)test/export-functionality.test.ts(2 hunks)test/gitignore-prompt-conditional.test.ts(1 hunks)test/integration/roundtrip.test.ts(1 hunks)test/opencode.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/export-functionality.test.ts (2)
src/exporters.ts (1)
exportToOpenCode(392-394)src/index.ts (1)
exportToOpenCode(37-37)
test/opencode.test.ts (3)
src/importers.ts (1)
importOpenCode(671-696)src/types.ts (1)
RuleBlock(14-21)src/exporters.ts (1)
exportToOpenCode(392-394)
src/exporters.ts (1)
src/types.ts (2)
RuleBlock(14-21)ExportOptions(35-41)
src/importers.ts (2)
src/index.ts (3)
importOpenCode(18-18)ImportResult(49-49)RuleBlock(48-48)src/types.ts (2)
ImportResult(23-28)RuleBlock(14-21)
src/cli.ts (5)
src/utils/colors.ts (1)
color(37-60)src/exporters.ts (1)
exportToOpenCode(392-394)src/index.ts (2)
exportToOpenCode(37-37)importOpenCode(18-18)src/utils/prompt.ts (1)
confirm(3-13)src/importers.ts (1)
importOpenCode(671-696)
🔇 Additional comments (25)
test/integration/roundtrip.test.ts (1)
131-133: Add format check looks fineIncluding 'opencode' in the round‑trip assertion aligns with new format support.
test/export-functionality.test.ts (1)
5-5: Public API import extended to OpenCode — OKImporting exportToOpenCode for mapping coverage looks good.
src/index.ts (1)
18-18: Re‑exports for OpenCode look goodSurface area updated cleanly; no further concerns.
Also applies to: 37-38
src/exporters.ts (3)
340-363: Good consolidationexportSingleFileWithHeaders removes duplication across single‑file exporters.
392-394: OpenCode exporter OKDelegating to shared helper keeps outputs consistent.
552-555: AGENTS.md is written twice — last writer wins (data loss risk)exportAll calls both exportToCodex and exportToOpenCode with the same path, clobbering content. Choose one or gate by option/format selection.
- exportToCodex(rules, join(repoPath, 'AGENTS.md'), options) + // Choose one; default to Codex (or gate via option) + exportToCodex(rules, join(repoPath, 'AGENTS.md'), options) @@ - exportToOpenCode(rules, join(repoPath, 'AGENTS.md'), options) + // exportToOpenCode(rules, join(repoPath, 'AGENTS.md'), options) // avoid overwriteAlternative: add a preferOpenCode boolean and branch.
Likely an incorrect or invalid review comment.
src/types.ts (2)
24-24: LGTM!The type extension correctly adds 'opencode' to the ImportResult.format union type, maintaining consistency with the new format support.
36-36: LGTM!The type extension correctly adds 'opencode' to the ExportOptions.format union type, maintaining consistency with the new format support.
test/opencode.test.ts (6)
8-50: LGTM!The import test properly validates:
- Format detection
- File path preservation
- Rule extraction
- Metadata parsing (id, alwaysApply)
- Content preservation
The test includes proper cleanup using try-finally.
52-103: LGTM!The export test validates:
- Header generation from descriptions
- Content preservation
- Multiple rule handling
- Proper formatting with double newlines
105-130: LGTM!This test ensures that rules without descriptions don't generate unnecessary headers. The assertion on line 125 correctly validates that no header is added when description is missing.
132-179: LGTM!This roundtrip test validates that:
- Content formatting is preserved (code blocks, lists, blockquotes)
- Import/export cycle maintains fidelity
- Headers are properly added during export
181-246: LGTM!These tests comprehensively cover private rule handling:
- Detection of private files by naming convention (.local.md)
- Private flag propagation during import
- Default filtering of private rules during export
- includePrivate option support
248-283: LGTM!This test validates conditional rules handling:
- Global rules (alwaysApply: true)
- Scoped rules with file patterns
- Proper section headers for context-specific rules
test/gitignore-prompt-conditional.test.ts (7)
11-49: LGTM!The test setup is well-structured:
- Proper temp directory management with beforeEach/afterEach
- Helper function runDotAgentExport correctly sets NODE_ENV='test' to prevent interactive prompts
- Error handling captures both stdout and exit codes
51-76: LGTM!This test correctly validates that when all patterns already exist in .gitignore, no update occurs and no "Updated .gitignore" message is shown. This aligns perfectly with the PR objective.
78-100: LGTM!This test validates that when patterns are missing, .gitignore is updated and the success message is shown.
102-119: LGTM!This test confirms that a new .gitignore file is created when it doesn't exist, which is expected behavior.
121-135: LGTM!This test validates that --no-gitignore flag prevents .gitignore creation/updates as expected.
137-162: LGTM!This test validates --gitignore flag behavior: it attempts auto-update but correctly doesn't update when patterns already exist. The assertion on line 161 confirms no update message when patterns are already present.
164-225: LGTM!These tests validate multi-format export scenarios:
- No update when all format patterns exist
- Update when some patterns are new
src/cli.ts (4)
6-6: LGTM!The imports correctly include the new importOpenCode and exportToOpenCode functions.
338-357: LGTM! Conditional gitignore prompting implemented correctly.The logic properly:
- Checks for new patterns using
checkForNewGitignorePatterns(line 339)- Respects --gitignore flag for auto-update (lines 341-344)
- Only prompts when there are new patterns and --no-gitignore is not set (lines 345-349)
- Only shows success message when actual update occurred (lines 354-356)
This aligns perfectly with the PR objective: "show gitignore prompt only when changes would be made."
475-492: LGTM!The
checkForNewGitignorePatternsfunction correctly:
- Returns true if .gitignore doesn't exist and there are paths to add
- Reads existing .gitignore and checks if patterns are already present
- Handles directory patterns by appending
**- Returns true only if there are new patterns to add
494-522: LGTM!The updated
updateGitignoreWithPathsfunction correctly:
- Returns boolean to indicate whether an update occurred
- Only appends patterns that don't already exist
- Creates .gitignore if it doesn't exist
- Returns true only when actual changes are made
JosXa
force-pushed
the
feat/conditional-gitignore-prompt
branch
from
7c5b386 to
840b118
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cli.ts (1)
466-483: Suggest extracting duplicated pattern-checking logic.The pattern-checking logic (lines 477-480) is duplicated in
updateGitignoreWithPaths(lines 499-502). Consider extracting this into a shared helper function to adhere to DRY principles.Example refactor:
+function filterNewPatterns(content: string, paths: string[]): string[] { + return paths.filter(p => { + const pattern = p.endsWith('/') ? p + '**' : p + return !content.includes(pattern) + }) +} + function checkForNewGitignorePatterns(repoPath: string, paths: string[]): boolean { const gitignorePath = join(repoPath, '.gitignore') if (!existsSync(gitignorePath)) { return paths.length > 0 } const content = readFileSync(gitignorePath, 'utf-8') - - const newPatterns = paths.filter(p => { - const pattern = p.endsWith('/') ? p + '**' : p - return !content.includes(pattern) - }) - + const newPatterns = filterNewPatterns(content, paths) return newPatterns.length > 0 }Then update
updateGitignoreWithPathsat lines 499-502 to use the same helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cli.ts(3 hunks)test/gitignore-prompt-conditional.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cli.ts (2)
src/utils/colors.ts (1)
color(37-60)src/utils/prompt.ts (1)
confirm(3-13)
🔇 Additional comments (6)
src/cli.ts (3)
332-344: LGTM! Clean conditional logic for gitignore prompting.The pre-check for new patterns and the updated conditional logic correctly implement the feature to prompt only when changes would be made. The flow properly handles all three cases: explicit
--gitignore, default behavior with new patterns, and--no-gitignore.
346-351: LGTM! Conditional logging aligns with PR objectives.The boolean return value from
updateGitignoreWithPathsis correctly used to log success messages only when the gitignore file was actually modified.
485-513: LGTM! Boolean return correctly signals actual modifications.The function now properly returns
trueonly when the gitignore file is created or modified, andfalsewhen no changes are needed. This enables the conditional logging implemented in the export command.test/gitignore-prompt-conditional.test.ts (3)
1-49: LGTM! Well-structured test setup.The test infrastructure is clean with proper setup/teardown, and the
runDotAgentExporthelper correctly setsNODE_ENVto 'test' to bypass interactive prompts, which aligns with the prompt utility implementation.
51-136: LGTM! Comprehensive coverage of core conditional behavior.These test cases thoroughly validate the main feature: gitignore updates occur only when new patterns would be added. The assertions correctly verify both file state changes and CLI output messages.
137-226: LGTM! Excellent coverage of edge cases and multi-format scenarios.The test suite comprehensively validates:
- Flag behavior (--gitignore with existing patterns correctly shows intent but doesn't update)
- Multi-format exports with varying pattern overlap
- Correct identification of truly new patterns across multiple exports
This provides strong confidence in the conditional gitignore feature.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.ts (1)
487-512: Bug confirmed: function appends all patterns instead of only new ones, causing duplicates on repeated calls.The implementation at line 504 appends the full
patternsvariable (which contains all input paths plus the header), even thoughfilterNewPatternsat line 501 has already identified only the truly new patterns. On repeated calls with overlapping paths, this causes:
- The header comment repeated multiple times
- Previously-added patterns duplicated in .gitignore
The existing tests (gitignore-flag.test.ts, export-functionality.test.ts) only verify single export operations, not the scenario of multiple calls with overlapping paths that would expose this duplication bug.
The suggested fix in the review comment is correct: append only
newPatternswith a conditional header check instead of re-appending the full block.
🧹 Nitpick comments (3)
src/cli.ts (3)
332-351: Gate auto-update and logging on hasNewPatterns to avoid misleading outputWhen --gitignore is set, you log “Updating .gitignore …” even if nothing will change. Gate both the decision and message on hasNewPatterns.
- if (values['gitignore']) { - // --gitignore flag: automatically update gitignore (answer yes) - shouldUpdateGitignore = true - console.log(color.info('Updating .gitignore (auto-enabled by --gitignore flag)')) - } else if (!values['no-gitignore'] && hasNewPatterns) { + if (values['gitignore']) { + // --gitignore flag: auto-update only if there are new patterns + if (hasNewPatterns) { + shouldUpdateGitignore = true + console.log(color.info('Updating .gitignore (auto-enabled by --gitignore flag)')) + } else { + console.log(color.dim('No new .gitignore entries needed')) + } + } else if (!values['no-gitignore'] && hasNewPatterns) { // Default behavior: ask user only if there are new patterns console.log() shouldUpdateGitignore = await confirm('Add exported files to .gitignore?', true) } - // If --no-gitignore is specified or no new patterns, shouldUpdateGitignore remains false + // If --no-gitignore is specified or no new patterns, shouldUpdateGitignore remains false
473-485: LGTM with a tiny early-exit micro-optimizationSolid pre-check. Optionally short‑circuit when paths.length === 0 to skip I/O.
function checkForNewGitignorePatterns(repoPath: string, paths: string[]): boolean { const gitignorePath = join(repoPath, '.gitignore') + if (paths.length === 0) return false + // If gitignore doesn't exist, all patterns are new if (!existsSync(gitignorePath)) { return paths.length > 0 }
536-545: Header sentinel collision may suppress private patterns blockupdateGitignore checks content.includes('# Added by dotagent:'). Because exported-files header also starts with this prefix, the private block may never be added. Match the exact private header or use distinct sentinels.
- // Check if patterns are already present - if (!content.includes('# Added by dotagent:')) { + // Check if private patterns are already present (exact header) + if (!content.includes('# Added by dotagent: ignore private AI rule files')) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cli.ts (2)
src/utils/colors.ts (1)
color(37-60)src/utils/prompt.ts (1)
confirm(3-13)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cli.ts (3)
395-396: convert('roo') will crash: importRoo is used but never imported from importers.jsDynamic import omits importRoo while the switch uses it, causing a ReferenceError at runtime.
- const { importCopilot, importCursor, importCline, importWindsurf, importZed, importCodex, importAider, importClaudeCode, importGemini, importQodo } = await import('./importers.js') + const { importCopilot, importCursor, importCline, importWindsurf, importZed, importCodex, importAider, importClaudeCode, importGemini, importQodo, importRoo } = await import('./importers.js')Also applies to: 429-431
501-526: Appends duplicates: only new patterns should be writtenWhen any new pattern exists, the function appends all paths (including already-present ones), duplicating lines. Append only the newPatterns.
function updateGitignoreWithPaths(repoPath: string, paths: string[]): boolean { const gitignorePath = join(repoPath, '.gitignore') - - const patterns = [ - '', - '# Added by dotagent: ignore exported AI rule files', - ...paths.map(p => p.endsWith('/') ? p + '**' : p), - '' - ].join('\n') + const buildBlock = (list: string[]) => + [ + '', + '# Added by dotagent: ignore exported AI rule files', + ...list.map(p => (p.endsWith('/') ? p + '**' : p)), + '' + ].join('\n') @@ - // Check if any of the patterns already exist - const newPatterns = filterNewPatterns(content, paths) - - if (newPatterns.length > 0) { - appendFileSync(gitignorePath, patterns) + // Append only patterns that are actually new + const newPatterns = filterNewPatterns(content, Array.from(new Set(paths))) + if (newPatterns.length > 0) { + appendFileSync(gitignorePath, buildBlock(newPatterns)) return true } return false } else { - writeFileSync(gitignorePath, patterns.trim() + '\n') + writeFileSync(gitignorePath, buildBlock(Array.from(new Set(paths))).trim() + '\n') return true } }
551-554: Header collision prevents adding private patternsupdateGitignore checks for the generic marker '# Added by dotagent:' which will be present after exported-patterns writes, so the private block never appends. Match the private header specifically.
- if (!content.includes('# Added by dotagent:')) { + if (!content.includes('# Added by dotagent: ignore private AI rule files')) { console.log(color.info('Updating .gitignore with private file patterns')) appendFileSync(gitignorePath, '\n\n' + privatePatterns + '\n', 'utf-8') }
🧹 Nitpick comments (3)
src/cli.ts (3)
332-345: UX win: prompt only when new patterns; consider dedup + clearer no-op messagingLooks good. Two small improvements:
- Deduplicate exportedPaths before checking/updating to avoid repeated entries.
- With --gitignore but no new patterns, log a no-op message and skip the update call.
- if (!isDryRun && exportedPaths.length > 0) { + if (!isDryRun && exportedPaths.length > 0) { let shouldUpdateGitignore = false - - // Check if there are actually new patterns to add - const hasNewPatterns = checkForNewGitignorePatterns(outputDir, exportedPaths) + const uniqueExportedPaths = Array.from(new Set(exportedPaths)) + // Check if there are actually new patterns to add + const hasNewPatterns = checkForNewGitignorePatterns(outputDir, uniqueExportedPaths) @@ - if (values['gitignore']) { + if (values['gitignore']) { // --gitignore flag: automatically update gitignore (answer yes) - shouldUpdateGitignore = true - console.log(color.info('Updating .gitignore (auto-enabled by --gitignore flag)')) + if (hasNewPatterns) { + shouldUpdateGitignore = true + console.log(color.info('Updating .gitignore (auto-enabled by --gitignore flag)')) + } else { + console.log(color.dim('No .gitignore changes needed')) + } @@ - if (shouldUpdateGitignore) { - const wasUpdated = updateGitignoreWithPaths(outputDir, exportedPaths) + if (shouldUpdateGitignore) { + const wasUpdated = updateGitignoreWithPaths(outputDir, uniqueExportedPaths) if (wasUpdated) { console.log(color.success('Updated .gitignore')) } }Also applies to: 346-351
466-485: Solid normalization; consider stripping inline comments to avoid false positivesGood move to compare by normalized lines and variants. To handle lines like "foo/ # comment", strip inline comments before trimming.
- const lines = content - .split(/\r?\n/) - .map(l => l.trim()) - .filter(l => l && !l.startsWith('#')) + const lines = content + .split(/\r?\n/) + // strip inline comments while allowing escaped \# + .map(l => l.replace(/(?<!\\)#.*$/, '').trim()) + .filter(l => l.length > 0)
487-499: Pre-check is correct and efficientThe hasNewPatterns gate is correct; returns true on missing .gitignore and otherwise defers to filterNewPatterns. Consider passing a deduped list (as in the earlier suggestion) to avoid redundant work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cli.ts (2)
src/utils/colors.ts (1)
color(37-60)src/utils/prompt.ts (1)
confirm(3-13)
|
🎉 This PR is included in version 2.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Summary
Summary by CodeRabbit
Bug Fixes
--gitignoreand--no-gitignoreflags to avoid unnecessary prompts or file creation.Tests