-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Plan (Agent) Mode #6445
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?
Plan (Agent) Mode #6445
Conversation
✅ Deploy Preview for continuedev canceled.
|
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.
cubic found 5 issues across 9 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai
to give specific feedback.
data-testid={`tool-policy-item-${props.tool.function.name}`} | ||
onClick={(e) => { | ||
dispatch(toggleToolSetting(props.tool.function.name)); | ||
e.stopPropagation(); | ||
e.preventDefault(); | ||
}} | ||
> | ||
{props.excluded || policy === "disabled" ? ( | ||
{disabled || policy === "disabled" ? ( |
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.
The check disables the UI for both disabled and policy === 'disabled', but the onClick handler above still dispatches toggleToolSetting regardless of disabled state. This could allow toggling a disabled tool via keyboard or programmatic click, which is a UX bug.
<div | ||
className={`flex w-8 flex-row items-center justify-end gap-2 px-2 py-0.5 sm:w-16 ${props.excluded ? "cursor-not-allowed" : "hover:bg-list-active hover:text-list-active-foreground cursor-pointer"}`} | ||
className={`flex w-8 flex-row items-center justify-end gap-2 px-2 py-0.5 sm:w-16 ${disabled ? "cursor-not-allowed" : "hover:bg-list-active hover:text-list-active-foreground cursor-pointer"}`} |
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.
ClassName string is getting long and complex, which can make it hard to maintain. Consider extracting conditional logic to a variable for readability.
@@ -16,7 +17,7 @@ | |||
interface ToolDropdownItemProps { | |||
tool: Tool; | |||
duplicatesDetected: boolean; | |||
excluded: boolean; | |||
isGroupEnabled: boolean; |
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.
Prop name changed from 'excluded' to 'isGroupEnabled', but the rest of the codebase may still reference the old prop name. Ensure all usages are updated to prevent runtime errors.
@@ -3,6 +3,7 @@ | |||
InformationCircleIcon, | |||
} from "@heroicons/react/24/outline"; | |||
import { Tool } from "core"; | |||
import { BUILT_IN_GROUP_NAME } from "core/tools/builtIn"; |
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.
Importing BUILT_IN_GROUP_NAME directly from 'core/tools/builtIn' may break if the module path or export changes. Consider centralizing such constants or using an alias if this is a common pattern.
@@ -27,7 +27,19 @@ test("should be able to toggle modes", async () => { | |||
); | |||
}); | |||
|
|||
// Check that it switched to Agent mode | |||
// Check that it switched to Plan mode |
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.
The comment says 'Check that it switched to Plan mode', but the following assertion checks for 'Agent' mode. The comment should match the actual check being performed to avoid confusion.
// Check that it switched to Plan mode | |
// Check that it switched to Agent mode |
Can't wait for this to merge. Thanks @RomneyDa for working on this PR. |
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.
All looks good (and +1 I'm hyped for this) but seems like at least one or two of the cubic comments might be worth addressing
Description
Adds a "Plan" mode which disables any tools that are not read only
Is available as "Plan" in mode dropdown, and included in cmd + . toggling
Under the hood, just adds a readOnlyMode boolean rather than adding a third "mode"
Tests
Updates toggle test to account for plan mode
Summary by cubic
Added a "Plan" mode that lets users run the agent with only read-only tools enabled. This mode is available in the mode dropdown and keyboard toggle, and updates the UI to reflect tool restrictions.