-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: add modes to walkaway #7119
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
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.
Comprehensive Review of PR #7119: feat: add modes to walkaway
After thoroughly reviewing the code changes, I'm approving this PR with some suggestions for improvement.
Summary of Changes Reviewed
This PR introduces significant improvements to task lifecycle management, mode handling, and tooling:
- Task Lifecycle Enhancements: Added async mode initialization with proper state management
- Mode Handling: Support for
mode_slug
parameter from cloud bridge and API - Critical Bug Fix: Fixed workspace path initialization order to prevent crashes
- New Tooling: Added vector-backed code search tool with Qdrant integration
- Memory Management: Improved resource cleanup using WeakMap for event listeners
- Test Coverage: Added comprehensive tests for mode_slug functionality
Review Comments
🟢 Critical Bug Fix - Workspace Path Initialization
File: src/core/task/Task.ts
(Lines 306-318)
Excellent fix for the workspace path initialization issue! Moving the workspace path initialization before creating controllers prevents the undefined workspace path crashes that could occur when controllers tried to access this.cwd
before it was properly initialized. This is a critical fix that improves stability.
🟢 Mode Handling Implementation
File: src/core/webview/ClineProvider.ts
(Lines 710-734)
Good implementation of bridge mode_slug handling. The code properly:
- Checks for mode_slug from the cloud extension bridge
- Validates the mode exists before applying it
- Logs the mode switch for debugging
- Falls back gracefully if the mode doesn't exist
🟡 Suggestion - Enhanced Error Handling
File: src/core/tools/codebaseSearchTool.ts
While the implementation is solid, consider adding more specific error handling for network failures when connecting to Qdrant. Currently, errors are caught generically. Consider using specific error types for better debugging and user feedback.
🟢 Memory Management Improvements
File: src/core/webview/ClineProvider.ts
(WeakMap usage)
Great use of WeakMap for tracking task event listeners! This prevents memory leaks by allowing garbage collection of tasks when they're no longer referenced elsewhere. The automatic cleanup when tasks are disposed is a significant improvement.
🟢 Comprehensive Test Coverage
File: src/core/webview/__tests__/ClineProvider.spec.ts
(Lines 2211-2288)
Excellent test coverage for the new bridge mode_slug functionality! The tests properly verify mode switching, validation, logging, and integration with handleModeSwitch.
🟡 Minor - Documentation
File: packages/types/src/task.ts
Consider adding JSDoc comments to the new InitTaskOptions
interface to document the purpose of each parameter, especially mode_slug
.
⚠️ Action Required
The PR has merge conflicts that need to be resolved before merging. Please rebase or merge the latest changes from the main branch.
Overall Assessment
This is a well-implemented feature that adds valuable functionality for task lifecycle management and mode handling. The critical workspace path fix alone makes this PR important to merge. The code quality is good, with proper error handling, memory management, and comprehensive test coverage.
Recommendation: APPROVE ✅ - Once the merge conflicts are resolved and the minor suggestions are considered, this PR is ready to merge.
Not a blocker, but could/should the new task tool use this for setting the mode? |
Also, are the integration test failures real? |
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.
It seems there are a couple of unrelated changes mixed in. If those fixes are necessary, then it would help to know what issues they are targeting to be able to test them properly.
The integration tests seem to be genuinely failing, can you take a look?
@mrubens @daniel-lxs -- sorry there was some code left over from some merge conflicts the integration tests are passing now-- not sure what changed |
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.
LGTM
@@ -97,11 +97,13 @@ export class API extends EventEmitter<RooCodeEvents> implements RooCodeAPI { | |||
text, | |||
images, | |||
newTab, | |||
modeSlug, | |||
}: { | |||
configuration: RooCodeSettings |
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.
Doesn't RooCodeSettings allow you to set the mode? Or am I misremembering?
…ombining mode plumbing with approve/deny functionality
This PR for cloud walkaway introduces task lifecycle, mode-handling, and tooling improvements across the extension: core types in packages/types/src/task.ts were extended and hardened, the main task implementation in src/core/task/Task.ts adds robust async mode initialization (taskMode/taskModeReady/getTaskMode/waitForModeInitialization), safer workspace-path initialization, stronger dispose/abort logic that removes listeners and releases resources, background collection of API usage tokens and persistence of GPT‑5 per-turn metadata, and submitUserMessage now accepts/apply a provided mode_slug before sending. A new vector-backed code search tool was added at src/core/tools/codebaseSearchTool.ts which validates parameters, queries the CodeIndexManager, sends structured results to the UI, and returns a readable output to the agent. The webview provider in src/core/webview/ClineProvider.ts improves lifecycle and memory management (tracking/removing task listeners with a WeakMap, clearer dispose flow), applies bridge-provided mode_slug when creating tasks, and wires code-index status subscriptions and state merging helpers. Finally, the extension API in src/extension/api.ts now tracks running tasks in a taskMap, broadcasts task events over IPC and file logs, supports starting tasks with a mode_slug, and uses the taskMap when cancelling tasks.
Important
Enhance task lifecycle and mode handling in the cloud walkaway extension, adding modeSlug support, a new code search tool, and updating the extension API for improved task management.
CreateTaskOptions
intask.ts
to includemodeSlug
and other options.modeSlug
handling increateTask()
andsubmitUserMessage()
inTask.ts
.ClineProvider.ts
usinghandleModeSwitch()
.createTask()
inClineProvider.ts
to applymodeSlug
from bridge options.codebaseSearchTool.ts
for vector-backed code search.ClineProvider.ts
for better lifecycle and memory management.api.ts
to track tasks intaskMap
and supportmodeSlug
instartNewTask()
.This description was created by
for 0dc85ee. You can customize this summary. It will automatically update as commits are pushed.