-
Couldn't load subscription status.
- Fork 2.2k
Add granular tool permissions #3345
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: dev
Are you sure you want to change the base?
Conversation
|
Hey thanks for PR, one note tho: |
|
@rekram1-node I made updates based on the feedback over in that issue as well as catching some of the files that weren't modified in my initial commit. Not sure if you want to wait on that issue or just iterate something in. How would you like to proceed? |
packages/opencode/src/agent/agent.ts
Outdated
| // Ensure required fields exist and have correct types | ||
| return 'edit' in data && | ||
| 'bash' in data && | ||
| (typeof data.bash === 'string' || typeof data.bash === 'object'); |
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.
You shouldn't need to do this, you should be able to leave it as it was but add a catchall for the undefined permissions
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.
example:
opencode/packages/opencode/src/config/config.ts
Lines 464 to 470 in f81e28c
| agent: z | |
| .object({ | |
| plan: Agent.optional(), | |
| build: Agent.optional(), | |
| general: Agent.optional(), | |
| }) | |
| .catchall(Agent) |
And after cleaning this up it should allow you to not have to have the type assertions in some of the other places
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.
I'll follow up on this over the weekend. Typescript isn't really a language I'm strong in, but I'll make the changes for this.
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.
I had Claude make the updates, they passed the typecheck and commit hooks. I wont be able to actually test till the weekend. Figured I'd just get the changes in so it's at least available for now.
My suspicion is that it's change for the bash permissions fallback was erroneous as well.
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.
@rekram1-node I did testing last night and this morning. The changes work. I only changed the part where it had a fallback permission for bash, I think defaults should already be handled in agents.ts
Let me know if anything else needs some work.
I had Claude make these changes to support permissions for MCP tool calls. There are some MCP servers I use that I just can't trust an AI to call freely.