-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Change default permissions of CLI reading outside of workspace to false
#4186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 9ef012d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thoughts on this PR @RSO and who would be best to review it? |
RSO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me, but I'd love a second opinion from @catrielmuller to be sure.
|
@lambertjosh , please add a changeset entry with
@RSO the change it's not than big because only affect to the new users, the default values are used to generate the config.json so the existing user will be continue using the existing configuration that they have. (This will be not override the configuration of them) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the docs as well -> https://github.com/Kilo-Org/kilocode/blob/main/apps/kilocode-docs/docs/cli.md
@catrielmuller - thanks, good catch. I actually took the opportunity to try to re-organize the docs a bit to split out the auto-approval feature, and the two operational modes. Please let me know if you think this is clearer or not. Previously interactive mode was nested inside of autonomous mode. |
@catrielmuller - I took a shot at adding a changeset, it's my first one so please take a careful look. 😉
This is great. Just so I understand, upon installation the config.json file is written and on updates it is left alone. Is that an accurate understanding? This way, any changes to the default config.json in updates would not override existing user settings. |
|
@catrielmuller @RSO - I do wonder if by changing this default we will be too negatively impacting the UX for CI/CD or other use cases. They could update their pipelines to modify this line, but I also wonder if some CLI options to allow for overriding auto-approval settings may be worthwhile? Other ideas here? |
|
@pandemicsyn - Can you also do a review to make sure this doesn't break Cloud Agents? |
Should be good to go! Cloud agent actually doesn't rely on auto creation of the config and already used |
Yes, Existing user configurations are respected because the logic effectively merges the defaults with the user's specific config. If a user is missing a specific entry in their config.json, it falls back to the default (And update their config.json), but it won't overwrite their existing settings. Think of the resolution strategy like this: |
reading outside of workspace to false
Co-authored-by: Remon Oldenbeuving <r.s.oldenbeuving@gmail.com>
a10af3d to
9ef012d
Compare
|
I still can't validate the entire CLI works as any model query seems to hang. That said I renamed my existing config.json then re-run |
|
OK thanks to Christiaan's help I have the CLI running locally. I have validated that the CLI works and that the default is changed as expected. |
|
I approve the decision |
Context
Implementation
Changes default configuration file parameter for automatic approval of read operations across the full computer when using the CLI.
The reason is that I don't think most users understand when they install and try the CLI that it arrives with defaults that could cause more data than expected on the users drive to be sent to an LLM and potentially saved or exposed to third parties (like the LLM provider). I personally found this default disconcerting and surprising, and I imagine it could erode user trust in Kilo Code lead to unfortunate accidents. (For both Kilo and the user, as a result of trying to clean up)
Significant behavior change / change management
We need to be careful in rolling out this change, in whether it will impact users who have already been using the CLI for some time and may like the current behavior. It could be construed as a breaking change.
The ideal scenario would be that we change the default moving forward, while leaving current users with the behavior they have likely already gotten used to.
How to Test
Ask Kilo Code CLI to perform an action that would require reading a file outside of it's current directory it was called in. See if it prompts for approval first, if so, this change worked.
Get in Touch