base path restriction for FileEditor

[madhur-tandon]

Summary of PR

base path restriction for FileEditor

Change Type

• Bug fix
• New feature
• Breaking change
• Refactor
• Other (dependency update, docs, typo fixes, etc.)

Checklist

• I have read and reviewed the code and I understand what the code is doing.
• I have tested the code to the best of my ability and ensured it works as expected.

Fixes

Resolves #240

Release Notes

FileEditorTool takes a base path parameter and forces all file paths to be relative to that base path. If a path doesn't have the base path as a prefix, it will be restricted from being edited.

• Include this change in the Release Notes.

[enyst]

If I understand correctly, base_path will always be set, is that right? Even though it's defined as an optional

[madhur-tandon]

Yes, you're right. In this specific code path, base_path will always have
a value (either the provided one or workspace.working_dir as fallback).

However, I kept base_path as optional in FileEditorTool's signature because:

1. Other callers might not always want to enforce a base_path restriction
2. The feature is opt-in (base_path=None means no restriction)
3. Backwards compatibility - existing code doesn't need to change

The fallback here ensures a sensible default for this specific use case
while keeping the tool flexible for other scenarios.

[enyst]

Just to clarify, you're right we need to do this, to implement the restriction, but I do think you're right it should be optional too, though. The use case I have in mind is with the OpenHands-CLI, and I think it should be possible for the CLI user to enable or disable this.

This bit of code creates the executor... which I think all callers need? It's not clear to me if we have enough flexibility for the CLI to just say e.g. "--always-approve means yolo, we'll disable path enforcement"? Please correct me if I'm wrong! It's just a tiny question, nothing else, I'd like to understand this.

[enyst]

Just a small question, is there a reason why this isn't workspace_root?

[madhur-tandon]

Yeah, even I had the same hunch i.e. merging both of these parameters together...

but the original issue demanded a separate parameter and since this is my first contribution, decided to follow the proposed idea.

Let me know if this needs to change though...

[enyst]

Got it, I think the issue is written with the help of the agent too, so who knows, we can take it as starting point, but change it freely if it makes sense.

I think maybe workspace root makes sense

[xingyaoww]

I actually have mixed feelings about this:

1. We don't restrict bash, so technically the agent can go over this by using terminal command to modify files outside the cwd
2. Also, i actually run into multiple cases where refering a file outside the cwd to the agent can be helpful (e.g., from another project)

Hence i'm not sure if we actually want to implement this restriction 🤔

[enyst]

Good point about bash @xingyaoww !

@madhur-tandon Frankly, that’s why I was asking about optional: to disable it on my machine. I’ve seen too many times the agent saying “I can’t use file editor, lemme make a python script and run it to replace stuff in file”, which it… does. 😅

I think this, restricting the agent access, is important for many users, but maybe we could think of a more fundamental solution (like sandboxing, which applications based on the SDK can already use). WDYT?