Fix permission prompt to show actual file directory#177
Fix permission prompt to show actual file directory#177Zebastjan wants to merge 2 commits intosynthetic-lab:mainfrom
Conversation
Previously, permission prompts always showed the directory where octofriend was started (process.cwd() at startup), not the directory of the file being operated on. This was misleading when working on files in different directories from where octofriend was launched. Now permission prompts correctly show the directory of the file being edited/created/read by extracting it from the tool arguments. Changes: - Import path module for directory operations - Update WhitelistAllowDescription to use actual file paths - Extract directory from filePath for read/edit/create operations - Use dirPath argument for list operations (with cwd fallback) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Question: how are you navigating to a different directory while octofriend is running (like in your example scenario)? Or do you just mean that this is for when octofriend is attempting to change a file outside its current directory? |
| } | ||
| case "list": | ||
| case "list": { | ||
| const dirPath = fn.arguments?.dirPath || cwd; |
There was a problem hiding this comment.
This isn't enough to actually change whitelist behavior!
You'll need to also change the addToWhitelist function. You should check if the file is within cwd, and if it's not, then you use the whitelist key with either edits:${cwd} or edits:${directory} (when directory is outside of cwd).
The expected behavior for any permission ask to edit/read within the current working directory of where octo started is to blanket-apply to the entire directory!
There was a problem hiding this comment.
Awesome, thanks for getting back to me on this. I will fix it up and send in an update as soon as I get a chance.
I realized that I didn't answer this question. In the example scenario I simply explicitly asked Octo to change to a different directory, write out a test file into that directory, and then later remove the test file. So it was very explicit, but in other scenarios I think the same problem would come up where you're touching on a project that's not in the directory where you started. And you say, there's this other project in this other directory. Let's have a look at that. |
This addresses the code review feedback that the previous
implementation only changed the display text but didn't
actually change what directory gets whitelisted.
Now the whitelist key includes the directory:
- Files within cwd: uses edits:${cwd} or read:${cwd}
(whitelists the entire working directory)
- Files outside cwd: uses edits:${directory} or read:${directory}
(whitelists only that specific directory)
This ensures the whitelist behavior matches what the
permission prompt displays to the user.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Thanks for the review! You're absolutely right - I only changed the display but not the actual whitelist behavior. I've now updated the New behavior:
The whitelist key is now determined by checking if the file's directory is within the startup cwd, and uses the appropriate directory for the whitelist key. This ensures the whitelist behavior matches what the permission prompt displays. Example scenarios:
Let me know if this is the behavior you were expecting! |
sdjidjev
left a comment
There was a problem hiding this comment.
The functionality looks good but have a suggestion!
| return `read:${directory}`; | ||
| } | ||
| case "list": { | ||
| const dirPath = fn.arguments?.dirPath ? path.resolve(fn.arguments.dirPath) : cwd; |
There was a problem hiding this comment.
This could be fragile as-is; you could instead declare a top-level function used by this component as well as WhitelistAllowDescription:
function getDirectoryForWhitelist(fn: ToolCall, cwd: string): string {
const filePath = "filePath" in fn.arguments ? fn.arguments.filePath : undefined;
// returns unmatchable placeholder to require future explicit permission when no filePath defined
if (!filePath) return INVALID_DIRECTORY;
const fileDirectory = path.dirname(path.resolve(filePath));
const relativePath = path.relative(cwd, fileDirectory);
const isExternalPath = relativePath.startsWith("..") || path.isAbsolute(relativePath);
return isExternalPath ? fileDirectory : cwd;
}
and then declare
const whitelistDirectory = getDirectoryForWhitelist(fn, cwd);
at the top of each usecase.
PS path.isAbsolute(relativePath) is for the edge-case in Windows when the directories are in different drives (C: vs D:)
Summary
This PR fixes permission prompts to show the directory of the file being operated on, rather than the directory where octofriend was initially started.
Problem
When octofriend prompts for permission to edit files, it offers a "Yes, and always allow file changes in {directory}" option. Previously, this directory was determined by where the octofriend process was initially launched from (
process.cwd()at startup), not by the directory of the file being edited.Example scenario:
/tmpfor testing~/dev/octofriend/home/zebastjan/dev/octofriend/source/app.tsx/tmp"/tmp, it's in~/dev/octofriendSolution
Changed the permission prompt logic to derive the directory from the actual file path being operated on, rather than using the process's startup working directory.
Changes Made
pathmodule import for directory operationsWhitelistAllowDescriptionfunction to extract directory from file paths:filePathargument usingpath.dirname(path.resolve(filePath))dirPathargument if provided, fallback tocwdif notTesting
✅ Tested by starting octofriend in
/tmpand editing files in~/dev/Omnibus✅ Permission prompt now correctly shows
/home/zebastjan/dev/Omnibusinstead of/tmp✅ Build passes with no TypeScript errors
Impact
This is more precise and safer - users won't accidentally whitelist an entire unrelated directory. The permission prompt now accurately reflects which directory will be whitelisted.
🤖 Generated with Claude Code