fix: push uncommitted changes on every run and ignore .system skills#11
Conversation
- Sync to remote whenever git is configured, not only when new skills are imported. This ensures uncommitted changes are always pushed on every run. - Ignore /.system/ path pattern to filter out OpenAI Codex built-in skills.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR modifies Git synchronization behavior in the start command to unconditionally sync when Git is configured, and extends skill scanning to exclude system-contained skills by filtering paths containing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the showStatus function in src/commands/start.ts to ensure that git synchronization occurs whenever a remote is configured, rather than only when new skills are detected. It also adds /.system/ to the IGNORE_PATH_PATTERNS in src/core/scanner.ts. A review comment correctly identifies that the newSkillsCount variable is now unused and should be removed for better code cleanliness.
| @@ -437,11 +435,11 @@ async function showStatus(config: GlobalConfig) { | |||
|
|
|||
| logger.info(t('start.detectingNew')) | |||
| const newSkillsCount = await autoImportNewSkills(config) | |||
There was a problem hiding this comment.
The variable newSkillsCount is declared and assigned but never used in the rest of the function. Since the conditional logic that previously used this variable has been removed, you can call autoImportNewSkills(config) directly without capturing its return value.
| const newSkillsCount = await autoImportNewSkills(config) | |
| await autoImportNewSkills(config) |
References
- Unused variables should be removed to maintain code cleanliness and avoid confusion about the variable's purpose.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/start.ts (1)
439-446: Behavior change is correct; consider gating the user-facing log to avoid noise.The unconditional sync correctly addresses the goal of pushing any uncommitted local changes on every run, and
autoSyncalready handles the "no changes" case silently via the!result.hashearly-return (line 556-558).However,
logger.info(t('start.syncingToGithub'))at line 444 is now printed on every invocation, even when there is nothing to commit/push. The user will see "Syncing to GitHub…" with no follow-up success message in the common no-op case, which can be slightly confusing. Consider moving the log insideautoSyncafter detecting actual changes, or only logging the success line whenresult.hashis truthy (already the case at line 560), and dropping the pre-log here.Optional refactor — emit the "syncing" message only when something actually syncs
if (config.git?.enabled && config.git?.remote) { - logger.newline() - logger.info(t('start.syncingToGithub')) await autoSync(config) }And inside
autoSync, logstart.syncingToGithubright before the push (or simply rely on the existingstart.syncedToGithubsuccess message).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/start.ts` around lines 439 - 446, The pre-sync user-facing log logger.info(t('start.syncingToGithub')) should not be printed unconditionally; remove or relocate it so users only see a "syncing" message when work will actually be pushed. Modify the start command to drop the unconditional logger.info(t('start.syncingToGithub')) call, and instead emit that message from inside autoSync (or right after autoSync returns) only when autoSync indicates changes (i.e., when result.hash is truthy in autoSync's return path), leaving the existing start.syncedToGithub success log intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commands/start.ts`:
- Around line 439-446: The pre-sync user-facing log
logger.info(t('start.syncingToGithub')) should not be printed unconditionally;
remove or relocate it so users only see a "syncing" message when work will
actually be pushed. Modify the start command to drop the unconditional
logger.info(t('start.syncingToGithub')) call, and instead emit that message from
inside autoSync (or right after autoSync returns) only when autoSync indicates
changes (i.e., when result.hash is truthy in autoSync's return path), leaving
the existing start.syncedToGithub success log intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96b0703d-f562-43b1-9df0-723b431b3fc3
📒 Files selected for processing (2)
src/commands/start.tssrc/core/scanner.ts
Captured return value was unused after removing the git-sync conditional gate. autoImportNewSkills() is called for its side effects.
1 participant
fix: push uncommitted changes on every run and ignore .system skills
Summary by CodeRabbit