Summary of Changes

Hello @ishaanxgupta, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the internal logic for managing the enabled/disabled states of agents and skills. By introducing a new featureToggleUtils.ts module and a FeatureToggleStrategy pattern, it abstracts the common operations for feature toggling. This change centralizes the core functionality, making the code more modular, reducing duplication in agentSettings.ts and skillSettings.ts, and improving overall maintainability for future feature management.

Highlights

• Code Refactoring for Feature Toggling: Introduced a new utility file, featureToggleUtils.ts, to centralize and abstract the common logic for enabling and disabling various features, such as agents and skills.
• Strategy Pattern Implementation: Implemented a FeatureToggleStrategy interface, allowing for feature-specific logic (e.g., how agents are enabled/disabled versus how skills are) to be defined and passed to generic toggle functions.
• Reduced Code Duplication: Refactored agentSettings.ts and skillSettings.ts to leverage the new generic enableFeature and disableFeature functions, significantly reducing redundant code for managing feature states across different settings scopes.
• Improved Maintainability: The changes enhance the modularity and maintainability of the settings management system by separating generic toggling mechanisms from feature-specific implementation details.

@sehoon38 please have a look, there are no test files for these 2 files, should I implement them in another PR or should I do them in this itself?

Hi there! Thank you for your contribution to Gemini CLI. We really appreciate the time and effort you've put into this pull request.

To keep our backlog manageable and ensure we're focusing on current priorities, we are closing pull requests that haven't seen maintainer activity for 30 days. Currently, the team is prioritizing work associated with 🔒 maintainer only or help wanted issues.

If you believe this change is still critical, please feel free to comment with updated details. Otherwise, we encourage contributors to focus on open issues labeled as help wanted. Thank you for your understanding!

@sehoon38 @scidomino could you please reopen this one, it previously got closed due to lack of help wanted label

This PR is a solid refactoring that improves code structure by creating a reusable utility for toggling features. This is a definite improvement for maintainability.

The most critical issue is the lack of tests for the new, shared logic. The existing tests for the agents and skills commands mock the functions that were refactored, so the new logic in featureToggleUtils.ts and the strategies in agentSettings.ts and skillSettings.ts are not covered by any tests. This is a significant risk for a change that touches settings.

Before this PR is merged, I recommend adding unit tests for the new and refactored files.

Here are my specific recommendations:

packages/cli/src/utils/featureToggleUtils.test.ts

• Test enableFeature when the feature is already enabled in all scopes.
• Test enableFeature when the feature is disabled in one scope.
• Test enableFeature when the feature is disabled in both scopes.
• Test disableFeature when the feature is already disabled.
• Test disableFeature when the feature is enabled.
• Test disableFeature with an invalid scope.

packages/cli/src/utils/agentSettings.test.ts

• Test the agentStrategy with various settings configurations (e.g., enabled, disabled, not present).

packages/cli/src/utils/skillSettings.test.ts

• Test the skillStrategy with various settings configurations (e.g., in the disabled list, not in the list).

Adding these tests will ensure that this refactoring is safe and that the logic is correct.

@scidomino added test and ran them on local all tests are passing and preflight also checked

Three test are failing in e2e one timedout one is flaky ig and one failed legit but i dont think that has anything to do with the refactor which this PR does🤔

tests look good!