fix(task): isolate task configuration from focused provider state - #1085
fix(task): isolate task configuration from focused provider state#1085edelauna wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTask operations now use task-owned mode and API configuration for prompts, context handling, request setup, rate limiting, and retries. Provider profile-change listeners were removed. Submitted mode and provider profile changes update task state immediately. Tests cover configuration isolation. ChangesTask configuration flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/task/Task.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/task/__tests__/Task.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/Task.ts`:
- Line 3997: Update setupProviderProfileChangeListener and the
ProviderProfileChanged handling so only the task that explicitly selected the
changed profile updates its task-owned apiConfiguration; prevent focused-task
changes from overwriting unrelated tasks’ API handlers or rate limits. Add a
package-local unit test covering a ProviderProfileChanged event emitted while
this task is not focused and verify its API configuration remains unchanged.
- Around line 4036-4037: Update submitUserMessage to synchronize the task’s
_taskMode immediately after provider.setMode(mode) succeeds, so subsequent
getTaskMode() consumers use the selected mode. Add a package-local unit test
covering a mode switch through submitUserMessage followed by a request,
verifying the updated mode is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7d3c65b-e2c6-4cca-9569-2fa63e26d78d
📒 Files selected for processing (2)
src/core/task/Task.tssrc/core/task/__tests__/Task.spec.ts
8023403 to
59414bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/__tests__/Task.spec.ts`:
- Around line 530-571: Update the “only applies profile changes to the focused
task” test to dispose both Task instances after assertions, using their
dispose() methods before the test completes. Ensure the shared mockProvider no
longer retains the parent and child profile-change listeners for subsequent
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5779997-ce83-47d3-bd7e-71b8bbacbd0c
📒 Files selected for processing (2)
src/core/task/Task.tssrc/core/task/__tests__/Task.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/task/Task.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/task/Task.ts (1)
1510-1518: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the selected provider profile on the task.
After
provider.setProviderProfile(providerProfile)succeeds, this code rebuildsapibut does not update_taskApiConfigName.saveClineMessages()then persists the prior profile in task metadata. A resumed task can restore the wrong profile.Set the task API config name from
newState.currentApiConfigName, withproviderProfileas a fallback. Add a regression test that switches profiles and verifies persisted task metadata.Proposed fix
const newState = await provider.getState() +this.setTaskApiConfigName(newState?.currentApiConfigName ?? providerProfile) if (newState?.apiConfiguration) { this.updateApiConfiguration(newState.apiConfiguration) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/Task.ts` around lines 1510 - 1518, Update the provider-profile handling after setProviderProfile in the task flow to assign _taskApiConfigName from newState.currentApiConfigName, falling back to providerProfile when unavailable, before persisting task metadata. Add a regression test that switches profiles and verifies saveClineMessages() persists the selected profile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/task/Task.ts`:
- Around line 1510-1518: Update the provider-profile handling after
setProviderProfile in the task flow to assign _taskApiConfigName from
newState.currentApiConfigName, falling back to providerProfile when unavailable,
before persisting task metadata. Add a regression test that switches profiles
and verifies saveClineMessages() persists the selected profile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7808404d-5caa-4a5b-8337-45277dac09dc
📒 Files selected for processing (2)
src/core/task/Task.tssrc/core/task/__tests__/Task.spec.ts
github-actions
Bot
added
the
awaiting-review
aa07009 to
968b489
Compare
github-actions
Bot
removed
the
awaiting-review
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
968b489 to
625a490
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
added
the
has-conflicts
2 participants
Related GitHub Issue
References: #369 (PR 1 of 3)
Description
This is PR 1 of 3 for #369. It prevents a task's mode and API/profile configuration from leaking through shared focused-provider state, while preserving current single-concurrency behavior.
Tasknow readsmodefromgetTaskMode()and API configuration from its ownapiConfigurationin system-prompt generation, manual and forced context condensing, request construction, and rate-limit/retry-backoff handling. Provider state remains the source for genuinely shared settings such as custom modes, experiments, and profile thresholds.Profile-change notifications now update only the focused task, preventing a profile change from replacing another live task's API handler/configuration. This is a prerequisite for safe concurrent delegation work, without introducing fan-out scheduling, queues, e2e changes, persistence changes, or extension API changes.
Test Procedure
pnpm --dir src exec vitest core/task/__tests__/Task.spec.tspnpm --dir src exec eslint --prune-suppressions --max-warnings=0 core/task/Task.ts core/task/__tests__/Task.spec.tspnpm --dir src check-typespnpm --dir src lintThe focused unit coverage verifies task-local mode/API configuration in prompt and request paths, task-local rate limits during retry backoff, and that profile changes update only the focused task.
Pre-Submission Checklist
Visual Snapshots
Not applicable; this PR has no UI changes.
Videos (interaction / animation only)
Not applicable; this PR has no UI changes.
Documentation Updates
Additional Notes
No changeset included; changesets are managed separately.
Get in Touch
Summary by CodeRabbit
Bug Fixes
Tests