-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add File Attachment Capability to new_task Tool #3086
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
Conversation
|
|
🚀 |
|
this will be a brilliant time saver thank you |
|
We're focused on orchestrator improvements this week - @cte will be in touch about this |
* base * button callback * prompt * smol * full truncate * base 2 * changeset * dup new task resp * Update src/core/prompts/commands.ts Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * comments --------- Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
b762aca to
ac06efa
Compare
|
Rebased, now it's up to date with the recent Cline.ts refactor |
jr
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.
I like this idea a lot.
It seems like it overlaps a lot with what readFileTool does. Is it practical to share most of that code, or are there reasons I'm not thinking of why that wouldn't work?
|
@jr |
ac06efa to
37ccca4
Compare
38a0c3b to
6b9865f
Compare
|
@jr I have also split the tests, now we test the file reading logic separately from the |
|
@daniel-lxs can you move over to experimental and fix conflicts? |
|
@samhvw8 please review #3086 with respect to #2886: It looks like this #3086 simply factors out Both PRs touch |
|
Hey @KJ7LNW, the refactor to the The |
…tached by the parent
KJ7LNW
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.
Overall, I like the implementation and I think this will be very useful.
Most of my comments are about best practices and ways to minimize differences across the refactor.
Thank you for putting this together!
src/shared/fileReadUtils.ts
Outdated
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.
processFileForReading is only used for tools, so it would be better placed in something like src/core/tools/toolHelpers.ts because fileReadUtils.ts is intended for file operations they have nothing to do with the user interface.
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.
as below, move the test to src/core/tools/__tests__
src/shared/fileReadUtils.ts
Outdated
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 am pretty sure this function already exists somewhere else? or maybe it was just reordered?
src/shared/fileReadUtils.ts
Outdated
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 think this function exists somewhere else, or maybe it was already here, but I do not understand why we see a bunch of + lines
src/shared/fileReadUtils.ts
Outdated
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.
does the stereotype need to be accessible by the user interface? if so put it in shared/ , otherwise it goes in toolHelpers.ts or maybe a file dedicated to tool structures if we do not have one yet
src/shared/fileReadUtils.ts
Outdated
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.
This should probably stay in readFileTool.ts and export it if you need it somewhere else.
someday functions like this will be in a tool base class to build xml without string concatenation with a proper xml builder library .
src/core/prompts/tools/new-task.ts
Outdated
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 see #2886 and work with @samhvw8 so that are line range formats do not diverge; multiple range formats would probably confuse the model across tools.
Gemini and Claude have both been shown empirically tested that the following format used in #2886 works very well for the model. this is a real life example:
<read_file>
<args>
<file>
<path>src/core/config/__tests__/ContextProxy.test.ts</path>
<line_range>130-140</line_range> <!-- Around line 134 -->
<line_range>160-170</line_range> <!-- Around line 166 -->
</file>
<file>
<path>src/exports/api.ts</path>
<line_range>60-75</line_range> <!-- Around line 67 -->
</file>
<file>
<path>src/exports/types.ts</path> <!-- To see RooCodeSettings definition -->
</file>
<file>
<path>src/schemas/index.ts</path>
<line_range>150-160</line_range> <!-- To see historyItemSchema definition -->
</file>
</args>
</read_file>standardizing XML representation will simplify it tool class refactors in the future
src/core/tools/readFileTool.ts
Outdated
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.
where did access allowed go?
if it moved somewhere centralized, then great but I want to make sure we are not creating regressions
src/core/tools/readFileTool.ts
Outdated
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.
These look like unnecessary variable rename that cause diff review bloat.
Unless there is a really good reason to rename variables let's not do that if we can avoid it, so we can tell what actually change for the pull request, and what is original .
src/core/tools/readFileTool.ts
Outdated
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.
This is a huge function hoist, and we need to make sure there are no hallucinations: please show the diff to a smart model (o4-high, etc) and ask it to validate that there is exactly no behavior change.
Especially, please validate around output formatting and file line endings, leading and trailing blank lines, etc, which have been validated in detail (and hopefully the tests will still enforce). If you were to look up the pull request related to XML output formatting, this was done meticulously for consistent output and help gemini with understanding things.
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.
Hey @KJ7LNW, the refactor to the read_file logic was made to unify the logic so it can be used in both places that is needed without duplicating the code.
The read_file tool unit tests were separated from the file reading logic, but I made sure to avoid modifying the tests themselves on both cases, if you see a case where a test no longer serves its original purpose please let me know.
If the test still pass, then hopefully it is fine. I only made this comment because I have seen models hallucinate during large code moves, so I always ask the model in a separate question showing only the diff and have it validate that the difference in behavior is identical.
It probably is identical but, I operate on the side of caution with these kinds of things.
6b9865f to
533d9e9
Compare
|
This can potentially break power-steering and it's not something we want at the moment. |
Add File Attachment Capability to new_task Tool
Closes: #2931
This PR introduces a new parameter to the new_task tool, enabling the parent agent to attach files by mimicking how users reference files in tasks.
Context
This enhancement adds a
<files>parameter to the new_task tool. Files specified in this parameter will be processed using the same logic as theread_filefunction, including:path:startLine:endLinePurpose:
This allows tasks to be created with immediately available file context, which:
Implementation Details
File Specification Format:
<file>path/to/file.ts</file><file>path/to/file.ts:10:50</file>(lines 10-50)Backend Changes:
AttachedFileSpecinterface inshared/tools.tsnewTaskTool.tsto parse file paths and extract line rangesClineProvider.tsto pass file specifications to the Cline constructorCline.tsto process attached files with line ranges and format them appropriately in the initial promptUI Enhancements:
ChatRow.tsxto display attached files as badges within the task content block@filename:10-50)Documentation:
new-task.tsto explain the new syntax and provide examplesVisual Comparison
Before: New tasks required the child agent to explicitly request files, leading to redundant operations and token usage.
After: Files are immediately available to the child agent, with badges showing attached files:
@filenamefor full files@filename:10-50for files with line rangesTesting Protocol
Basic Functionality:
Line Range Testing:
src/core/Cline.ts:580:600)UI Verification:
Error Handling:
Important
This PR adds file attachment capability with line range specifications to the
new_tasktool, updating backend processing and UI display.<files>parameter tonew_tasktool for attaching files with optional line ranges.read_filelogic, including line limits and line numbers.AttachedFileSpecinterface inshared/tools.ts.newTaskTool.tsto parse and handle file paths and line ranges.ClineProvider.tsto pass file specifications toCline.Cline.tsto format attached files for task prompts.ChatRow.tsxto display attached files as badges with line ranges.new-task.tsto include new syntax and examples for file attachments.This description was created by
for d5a1d4af904060bdb8e5bce3075e3acdf9740847. You can customize this summary. It will automatically update as commits are pushed.