test(vscode-e2e): add orchestrator E2E foundation for parallel-mode coverage - #1064
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds typed resource diagnostics, reusable orchestrator replay fixtures, delegation-plan unit tests, and end-to-end cleanup validation. Vitest is integrated into the VS Code E2E package and workflow. ChangesOrchestrator E2E and Resource Diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OrchestratorTest
participant OrchestratorTask
participant LLMock
participant RooCodeAPI
OrchestratorTest->>OrchestratorTask: start orchestration scenario
OrchestratorTask->>LLMock: request child task or resume
LLMock-->>OrchestratorTask: return delegated step or completion
OrchestratorTask-->>OrchestratorTest: emit delegation and completion events
OrchestratorTest->>RooCodeAPI: read final resource diagnostics
RooCodeAPI-->>OrchestratorTest: return task, stack, and listener counters
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/extension/api.ts (1)
356-380: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
registeredTaskIdsentries are only removed on completed/aborted.Tasks that end any other way (evicted via
clearCurrentTask, interrupted and never resumed, provider disposal) stay in the set for the lifetime of the extension host, soregisteredTaskCountnever returns to baseline and the E2E convergence assertions can fail intermittently. Consider also pruning on task disposal/eviction (or reconciling against the provider's task registry insidegetResourceDiagnostics).🤖 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/extension/api.ts` around lines 356 - 380, Ensure registeredTaskIds is pruned when tasks are disposed or evicted, not only on TaskCompleted and TaskAborted. Update the TaskCreated lifecycle wiring around clearCurrentTask/provider disposal or the task’s disposal event to delete task.taskId, while preserving existing completion and abort cleanup; alternatively reconcile the set with the provider’s active task registry in getResourceDiagnostics.
🧹 Nitpick comments (9)
apps/vscode-e2e/src/fixtures/resource-diagnostics.ts (1)
56-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the intentional asymmetry.
Skipping the issue when
final.currentTaskStackLength === 0means a shrinking stack is always considered converged; that's non-obvious next to the strictregisteredTaskCountcheck. A one-line comment would prevent someone "fixing" it later.🤖 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 `@apps/vscode-e2e/src/fixtures/resource-diagnostics.ts` around lines 56 - 62, Add a concise comment immediately above the currentTaskStackLength condition explaining that a final value of zero intentionally treats a shrinking task stack as converged, distinguishing this behavior from the strict registeredTaskCount check.apps/vscode-e2e/src/fixtures/orchestrator-plan.spec.ts (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuite name no longer matches its contents.
This
describenow covers repeated, nested, and cancellation-recovery plans too; consider"orchestrator delegation plans"or splitting into per-scenario blocks.🤖 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 `@apps/vscode-e2e/src/fixtures/orchestrator-plan.spec.ts` at line 37, Update the describe block currently named “orchestrator fan-out delegation plan” to use a broader suite name such as “orchestrator delegation plans,” reflecting its repeated, nested, and cancellation-recovery scenarios.apps/vscode-e2e/src/fixtures/orchestrator.ts (3)
40-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the explicit re-export list with a barrel re-export.
Every symbol is imported and then re-exported verbatim;
export * from "./orchestrator-plan"(plus a plainimportof what this file actually uses) removes ~35 lines that must be kept in sync whenever the plan module grows.🤖 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 `@apps/vscode-e2e/src/fixtures/orchestrator.ts` around lines 40 - 74, Replace the explicit export list in orchestrator.ts with a barrel re-export from "./orchestrator-plan". Add or retain a plain import containing only the symbols this file directly uses, while preserving the existing public exports and removing the duplicated re-export maintenance.
102-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree identical child-completion fixture loops.
The fan-out, repeated, and nested grandchild loops differ only in the step array. Extract a small helper (e.g.
addChildCompletionFixtures(mock, steps)) and reuse it for all three, and likewise for the resume-expectation loops.Also applies to: 169-185, 256-272
🤖 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 `@apps/vscode-e2e/src/fixtures/orchestrator.ts` around lines 102 - 118, Extract the duplicated child-completion fixture loop into a helper such as addChildCompletionFixtures(mock, steps), then reuse it for the fan-out, repeated, and nested grandchild step arrays. Apply the same extraction and reuse pattern to the corresponding resume-expectation loops, preserving each loop’s existing step-specific behavior.
120-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a comment explaining the reversed registration order.
[...].reverse()is load-bearing: it registers the most cumulative resume expectations first so earlier (less specific) predicates can't shadow later rounds. That intent isn't recoverable from the code alone, and the same pattern is repeated four times.🤖 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 `@apps/vscode-e2e/src/fixtures/orchestrator.ts` around lines 120 - 121, Add an explanatory comment immediately before the reversed iteration in the orchestrator resume expectation registration, documenting that reverse order registers the most cumulative expectations first and prevents less-specific predicates from shadowing later rounds. Apply the same comment to all four occurrences of this reversed registration pattern.apps/vscode-e2e/src/suite/orchestrator.test.ts (2)
122-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBaseline diagnostics captured in only two of four tests.
Tests 1 and 3 do the same cleanup but skip the convergence assertion, so a leak introduced by the fan-out or nested flow would only surface (if at all) as a baseline shift in the next test. Consider applying the same baseline/convergence pattern to all four.
Also applies to: 400-400
🤖 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 `@apps/vscode-e2e/src/suite/orchestrator.test.ts` at line 122, Apply the baseline diagnostics and convergence assertion pattern consistently across all four tests in the orchestrator test suite, including tests 1 and 3. Ensure each test captures diagnostics before execution, performs its existing cleanup, and verifies diagnostics converge back to the captured baseline rather than relying on the next test to detect leaks.
108-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated cleanup block into a helper.
The four
finallyblocks duplicate listener removal, the stack-drain loop, and (in two of them) the diagnostics convergence wait. Also notewhile (api.getCurrentTaskStack().length > 0) await api.clearCurrentTask()has no iteration cap — if a task can't be cleared this spins until the mocha timeout instead of failing with a clear message. A shareddrainTaskStack(api, { maxAttempts })+expectDiagnosticsConverged(...)helper would fix both.Also applies to: 226-254, 386-395, 525-553
🤖 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 `@apps/vscode-e2e/src/suite/orchestrator.test.ts` around lines 108 - 117, Extract the duplicated finally-block cleanup into shared drainTaskStack and diagnostics-convergence helpers, including listener removal where appropriate. Update all four cleanup sites around the existing test handlers to use them, and cap clearCurrentTask attempts via maxAttempts so a non-clearable task fails with a clear assertion instead of looping until timeout. Preserve diagnostics waiting only in the cleanup paths that currently require it.packages/types/src/api.ts (1)
12-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the union from
RooCodeEventNameinstead of re-typing literals.These string literals silently duplicate
RooCodeEventNamevalues; a rename in the enum would leave this union stale whileRESOURCE_DIAGNOSTIC_EVENTSinsrc/extension/api.tsstill type-checks via theascast.♻️ Suggested tightening
-export type RooCodeResourceDiagnosticEventName = - | "message" - | "taskCreated" - | "taskStarted" - | "taskCompleted" - | "taskAborted" - | "taskDelegationCompleted" - | "taskDelegationResumed" - | "taskModeSwitched" +export type RooCodeResourceDiagnosticEventName = Extract< + `${RooCodeEventName}`, + | "message" + | "taskCreated" + | "taskStarted" + | "taskCompleted" + | "taskAborted" + | "taskDelegationCompleted" + | "taskDelegationResumed" + | "taskModeSwitched" +>🤖 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 `@packages/types/src/api.ts` around lines 12 - 26, Update RooCodeResourceDiagnosticEventName to derive its allowed values from RooCodeEventName rather than duplicating string literals, and ensure RESOURCE_DIAGNOSTIC_EVENTS remains type-safe without relying on a cast that can hide renamed or invalid events.apps/vscode-e2e/src/fixtures/orchestrator-plan.ts (1)
234-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe cancellation follow-up tool-call id has no single source of truth. One site reconstructs it by string-replacing
"completion_002" → "followup_001", the other hardcodes the literal, so renaming either breaks fixture matching silently.
apps/vscode-e2e/src/fixtures/orchestrator-plan.ts#L234-L247: export aORCHESTRATOR_CANCELLATION_RECOVERY_FOLLOWUP_TOOL_CALL_IDconstant and use it in both matchers instead of.replace(...).apps/vscode-e2e/src/fixtures/orchestrator.ts#L344-L361: import that constant and use it as theask_followup_questiontool-callidinstead of the inline"call_orchestrator_cancellation_child_followup_001"literal.🤖 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 `@apps/vscode-e2e/src/fixtures/orchestrator-plan.ts` around lines 234 - 247, The cancellation follow-up tool-call ID is duplicated across fixtures. In apps/vscode-e2e/src/fixtures/orchestrator-plan.ts#L234-L247, export a single ORCHESTRATOR_CANCELLATION_RECOVERY_FOLLOWUP_TOOL_CALL_ID constant and use it in both matcher functions instead of reconstructing the value with replace; in apps/vscode-e2e/src/fixtures/orchestrator.ts#L344-L361, import and use that constant for the ask_followup_question tool-call ID instead of the inline literal.
🤖 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 `@apps/vscode-e2e/src/fixtures/orchestrator-plan.ts`:
- Line 129: Update ORCHESTRATOR_REPEATED_DELEGATION_FINAL_RESULT to join mapped
rows with a real newline escape, matching the header and the other final-result
constants; preserve the existing row formatting and summary content.
In `@apps/vscode-e2e/src/suite/orchestrator.test.ts`:
- Around line 29-31: Add a suiteTeardown for the “Roo Code Orchestrator” suite
that restores the default OpenRouter configuration after the tests complete. Use
the existing configuration-reset helper or established default-config symbol,
ensuring persisted provider, model, mode, and auto-approval settings do not
affect subsequent suites.
---
Outside diff comments:
In `@src/extension/api.ts`:
- Around line 356-380: Ensure registeredTaskIds is pruned when tasks are
disposed or evicted, not only on TaskCompleted and TaskAborted. Update the
TaskCreated lifecycle wiring around clearCurrentTask/provider disposal or the
task’s disposal event to delete task.taskId, while preserving existing
completion and abort cleanup; alternatively reconcile the set with the
provider’s active task registry in getResourceDiagnostics.
---
Nitpick comments:
In `@apps/vscode-e2e/src/fixtures/orchestrator-plan.spec.ts`:
- Line 37: Update the describe block currently named “orchestrator fan-out
delegation plan” to use a broader suite name such as “orchestrator delegation
plans,” reflecting its repeated, nested, and cancellation-recovery scenarios.
In `@apps/vscode-e2e/src/fixtures/orchestrator-plan.ts`:
- Around line 234-247: The cancellation follow-up tool-call ID is duplicated
across fixtures. In apps/vscode-e2e/src/fixtures/orchestrator-plan.ts#L234-L247,
export a single ORCHESTRATOR_CANCELLATION_RECOVERY_FOLLOWUP_TOOL_CALL_ID
constant and use it in both matcher functions instead of reconstructing the
value with replace; in apps/vscode-e2e/src/fixtures/orchestrator.ts#L344-L361,
import and use that constant for the ask_followup_question tool-call ID instead
of the inline literal.
In `@apps/vscode-e2e/src/fixtures/orchestrator.ts`:
- Around line 40-74: Replace the explicit export list in orchestrator.ts with a
barrel re-export from "./orchestrator-plan". Add or retain a plain import
containing only the symbols this file directly uses, while preserving the
existing public exports and removing the duplicated re-export maintenance.
- Around line 102-118: Extract the duplicated child-completion fixture loop into
a helper such as addChildCompletionFixtures(mock, steps), then reuse it for the
fan-out, repeated, and nested grandchild step arrays. Apply the same extraction
and reuse pattern to the corresponding resume-expectation loops, preserving each
loop’s existing step-specific behavior.
- Around line 120-121: Add an explanatory comment immediately before the
reversed iteration in the orchestrator resume expectation registration,
documenting that reverse order registers the most cumulative expectations first
and prevents less-specific predicates from shadowing later rounds. Apply the
same comment to all four occurrences of this reversed registration pattern.
In `@apps/vscode-e2e/src/fixtures/resource-diagnostics.ts`:
- Around line 56-62: Add a concise comment immediately above the
currentTaskStackLength condition explaining that a final value of zero
intentionally treats a shrinking task stack as converged, distinguishing this
behavior from the strict registeredTaskCount check.
In `@apps/vscode-e2e/src/suite/orchestrator.test.ts`:
- Line 122: Apply the baseline diagnostics and convergence assertion pattern
consistently across all four tests in the orchestrator test suite, including
tests 1 and 3. Ensure each test captures diagnostics before execution, performs
its existing cleanup, and verifies diagnostics converge back to the captured
baseline rather than relying on the next test to detect leaks.
- Around line 108-117: Extract the duplicated finally-block cleanup into shared
drainTaskStack and diagnostics-convergence helpers, including listener removal
where appropriate. Update all four cleanup sites around the existing test
handlers to use them, and cap clearCurrentTask attempts via maxAttempts so a
non-clearable task fails with a clear assertion instead of looping until
timeout. Preserve diagnostics waiting only in the cleanup paths that currently
require it.
In `@packages/types/src/api.ts`:
- Around line 12-26: Update RooCodeResourceDiagnosticEventName to derive its
allowed values from RooCodeEventName rather than duplicating string literals,
and ensure RESOURCE_DIAGNOSTIC_EVENTS remains type-safe without relying on a
cast that can hide renamed or invalid events.
🪄 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: 74f1fbdd-ad35-4be8-92fd-f6758a4553b8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/vscode-e2e/package.jsonapps/vscode-e2e/src/fixtures/orchestrator-plan.spec.tsapps/vscode-e2e/src/fixtures/orchestrator-plan.tsapps/vscode-e2e/src/fixtures/orchestrator.tsapps/vscode-e2e/src/fixtures/resource-diagnostics.spec.tsapps/vscode-e2e/src/fixtures/resource-diagnostics.tsapps/vscode-e2e/src/runTest.tsapps/vscode-e2e/src/suite/orchestrator.test.tsapps/vscode-e2e/vitest.config.tspackages/types/src/api.tssrc/extension/__tests__/api-resource-diagnostics.spec.tssrc/extension/api.ts
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 @.github/workflows/e2e.yml:
- Around line 147-150: Move the “Run vscode-e2e unit tests” step using the
test:unit command to immediately after dependency setup, before the VS Code
binary download steps. Preserve its existing condition and command so the
Vitest-only suite runs independently of binary availability.
🪄 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: a81fae15-df62-468f-99ed-482c88a3e340
📒 Files selected for processing (1)
.github/workflows/e2e.yml
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Addressed the CodeRabbit review feedback in commit
Validation run:
|
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/extension/api.ts (1)
35-51: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReconcile registered tasks across every provider being tracked.
registeredTaskIdsis shared by all calls toregisterListeners(provider), including the separate new-tab provider created at Lines [201]–[202], butgetResourceDiagnostics()compares it only withthis.sidebarProvider.getCurrentTaskStack(). A task created in another tab can therefore be deleted from diagnostics while still active, causing false resource convergence. Track task IDs with their owning provider and reconcile each provider’s stack, or scope registration and diagnostics to the same provider; add a second-provider regression test.Also applies to: 270-287
🤖 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/extension/api.ts` around lines 35 - 51, Update registeredTaskIds and the registerListeners/getResourceDiagnostics flow so task ownership is preserved across every tracked ClineProvider, including the separate new-tab provider. Reconcile registered task IDs against each owning provider’s getCurrentTaskStack rather than only sidebarProvider, ensuring active tasks in other tabs remain in diagnostics. Add a regression test covering registration and reconciliation with two providers.
🧹 Nitpick comments (1)
apps/vscode-e2e/src/suite/orchestrator.test.ts (1)
29-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse a shared OpenRouter default config helper.
getDefaultOpenRouterConfiguration()and the equivalent reset object inapps/vscode-e2e/src/suite/anthropic-opus-4-7.test.tsduplicate the same provider/model/key/baseUrl selection. Put this default in a shared suite util and import it from both teardowns to avoid drift on keys likeopenRouterModelId,mode, and auto-approval flags.🤖 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 `@apps/vscode-e2e/src/suite/orchestrator.test.ts` around lines 29 - 43, Extract getDefaultOpenRouterConfiguration into a shared VS Code E2E suite utility, preserving its provider, model, API-key, base URL, mode, and approval settings. Import and reuse that helper in both orchestrator.test.ts and the reset/teardown configuration in anthropic-opus-4-7.test.ts, removing the duplicated local default object.Source: Path instructions
🤖 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/extension/api.ts`:
- Around line 35-51: Update registeredTaskIds and the
registerListeners/getResourceDiagnostics flow so task ownership is preserved
across every tracked ClineProvider, including the separate new-tab provider.
Reconcile registered task IDs against each owning provider’s getCurrentTaskStack
rather than only sidebarProvider, ensuring active tasks in other tabs remain in
diagnostics. Add a regression test covering registration and reconciliation with
two providers.
---
Nitpick comments:
In `@apps/vscode-e2e/src/suite/orchestrator.test.ts`:
- Around line 29-43: Extract getDefaultOpenRouterConfiguration into a shared VS
Code E2E suite utility, preserving its provider, model, API-key, base URL, mode,
and approval settings. Import and reuse that helper in both orchestrator.test.ts
and the reset/teardown configuration in anthropic-opus-4-7.test.ts, removing the
duplicated local default object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7b70ac8-6b1e-4c8a-b233-6f18aeb31dff
📒 Files selected for processing (9)
.github/workflows/e2e.ymlapps/vscode-e2e/src/fixtures/orchestrator-plan.spec.tsapps/vscode-e2e/src/fixtures/orchestrator-plan.tsapps/vscode-e2e/src/fixtures/orchestrator.tsapps/vscode-e2e/src/fixtures/resource-diagnostics.tsapps/vscode-e2e/src/suite/orchestrator.test.tspackages/types/src/api.tssrc/extension/__tests__/api-resource-diagnostics.spec.tssrc/extension/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/types/src/api.ts
96bc978 to
62d3226
Compare
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/extension/api.ts (1)
270-287: 🗄️ Data Integrity & Integration | 🟠 Major | ⚖️ Poor tradeoffKeep new-tab task IDs from being pruned here when reporting diagnostics.
startNewTask({ newTab: true })registers the newClineProviderand adds itsTaskCreatedevents to the sharedregisteredTaskIds, butgetResourceDiagnostics()still prunes against onlythis.sidebarProvider.getCurrentTaskStack(). That can remove a new-tab task that emittedTaskCreatedbeforeTaskCompleted/TaskAborted, leavingregisteredTaskCountinconsistent with the active/new-tab task flow. Track the provider per registered task ID, or query every registered provider’s stack, before pruning.🤖 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/extension/api.ts` around lines 270 - 287, The getResourceDiagnostics method currently prunes registeredTaskIds using only the sidebar provider’s current task stack. Preserve new-tab task IDs by tracking the owning ClineProvider for each registered task ID or aggregating current task stacks from every registered provider, then prune only IDs absent from all active provider stacks while retaining the existing diagnostics counts.
🧹 Nitpick comments (1)
src/extension/api.ts (1)
35-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo compile-time exhaustiveness check between
RooCodeResourceDiagnosticEventNameandRESOURCE_DIAGNOSTIC_EVENTS. Both files independently enumerate the same 8 diagnostic events; thesatisfiescheck only validates array entries are valid members of the type, not that all members are covered, and theascast at the consumption site (src/extension/api.tsline 280) would silently hide a future mismatch, producingundefinedlistener counts instead of a compile error.
src/extension/api.ts#L35-L44: deriveRESOURCE_DIAGNOSTIC_EVENTSfrom aRecord<RooCodeResourceDiagnosticEventName, true>object literal (e.g., viaObject.keys) so omitting or misnaming an event fails to compile, instead of relying solely on the downstreamascast at line 280.packages/types/src/api.ts#L12-L22: keep as the source of truth for the allowed event names; no change needed here beyond ensuring the implementation site enforces exhaustiveness against it.🤖 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/extension/api.ts` around lines 35 - 44, The RESOURCE_DIAGNOSTIC_EVENTS definition in src/extension/api.ts:35-44 lacks compile-time exhaustiveness; replace the array-only declaration with an exhaustive Record<RooCodeResourceDiagnosticEventName, true> object and derive the event list from its keys, while preserving the existing consumption behavior. packages/types/src/api.ts:12-22 is the source of truth and requires no direct change.
🤖 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/extension/api.ts`:
- Around line 270-287: The getResourceDiagnostics method currently prunes
registeredTaskIds using only the sidebar provider’s current task stack. Preserve
new-tab task IDs by tracking the owning ClineProvider for each registered task
ID or aggregating current task stacks from every registered provider, then prune
only IDs absent from all active provider stacks while retaining the existing
diagnostics counts.
---
Nitpick comments:
In `@src/extension/api.ts`:
- Around line 35-44: The RESOURCE_DIAGNOSTIC_EVENTS definition in
src/extension/api.ts:35-44 lacks compile-time exhaustiveness; replace the
array-only declaration with an exhaustive
Record<RooCodeResourceDiagnosticEventName, true> object and derive the event
list from its keys, while preserving the existing consumption behavior.
packages/types/src/api.ts:12-22 is the source of truth and requires no direct
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ecd47ded-14dd-4193-9f7d-52096c72d161
📒 Files selected for processing (9)
.github/workflows/e2e.ymlapps/vscode-e2e/src/fixtures/orchestrator-plan.spec.tsapps/vscode-e2e/src/fixtures/orchestrator-plan.tsapps/vscode-e2e/src/fixtures/orchestrator.tsapps/vscode-e2e/src/fixtures/resource-diagnostics.tsapps/vscode-e2e/src/suite/orchestrator.test.tspackages/types/src/api.tssrc/extension/__tests__/api-resource-diagnostics.spec.tssrc/extension/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/vscode-e2e/src/fixtures/resource-diagnostics.ts
|
Addressed the
Validation:
|
github-actions
Bot
added
the
awaiting-review
2 participants
Summary
This PR is a follow-up test foundation based on the discussion in #977 (
fix(webview): add durable per-view state base). PR #977 introduces the durable per-view state base needed for parallel-mode work, and edelauna requested more real VS Code E2E coverage around the kinds of cross-provider / cross-panel boundaries that mocked unit tests cannot fully exercise.Instead of putting all complex scenarios into one large commit, this PR adds the E2E foundation in reviewer-friendly slices. The focus here is not to change production orchestration behavior, but to establish durable E2E infrastructure for future parallel-mode work: multi-provider task lifecycles, delegation fan-in/fan-out, nested delegation, cancellation recovery, and resource convergence checks.
Context from #977
#977 fixes the durable per-view state base so sidebar and editor-tab providers can maintain isolated view-local state while still sharing the same extension/global-state boundary. That PR is the root dependency for broader parallel-mode work.
The review feedback on #977 asked for more E2E coverage beyond mocked provider tests, especially around real extension-host behavior. This PR builds on that direction by adding a heavier E2E base that can be reused for future parallel-mode scenarios.
What this PR adds
1. Single-round orchestrator fan-out / fan-in E2E
Adds a baseline orchestrator E2E where a parent orchestrator delegates sequential children to ask, architect, and code modes, then fans their summaries back into the parent completion.
This verifies:
2. Repeated delegation stress E2E
Adds a repeated delegation workflow with three rounds of ask / architect / code children, validating that the orchestrator path remains stable across repeated child creation, completion, resume, and final aggregation.
This verifies:
childIds3. Resource diagnostics API
Adds a small test-focused diagnostics API exposing stable counters:
This is intentionally limited to deterministic resource counters rather than noisy heap/RSS thresholds.
4. Resource convergence sentinel
Wires the diagnostics API into the repeated orchestrator E2E so the test hard-gates that resources converge after cleanup.
This verifies:
5. Nested orchestrator delegation E2E
Adds a nested workflow:
This verifies stable delegation ownership across a task tree, not just a single parent-child edge.
6. Cancellation / recovery E2E
Adds coverage for cancelling/interruption of a delegated child and explicit recovery.
This verifies:
Normal/unit coverage
Each E2E slice includes normal/unit coverage for the helper or API logic it introduces:
Why this is separate from #977
#977 is the production fix for durable per-view state. This PR is the E2E foundation requested from that review context. Keeping it separate makes the review easier:
Validation
The following validation was run after cherry-picking onto latest main and resolving conflicts:
git status --short— cleangit diff --name-only --diff-filter=U— no unresolved conflictsgit diff --check— passedpnpm --dir apps/vscode-e2e exec vitest run src/fixtures/orchestrator-plan.spec.ts src/fixtures/resource-diagnostics.spec.ts— passed, 23 testscd src ; npx vitest run extension/__tests__/api-resource-diagnostics.spec.ts— passed, 3 testspnpm --dir src run check-types— passedpnpm --dir packages/types run check-types— passedA final targeted VS Code E2E run for
orchestrator.testshould be run before merge, since this branch was just replayed onto latest main.Relationship to future work
This PR is intended as the base for more complex parallel-mode E2E testing. It provides reusable orchestration fixtures, resource convergence helpers, and multi-task lifecycle coverage that future PRs can build on when testing real parallel panels/providers.
Summary by CodeRabbit
getResourceDiagnostics()to inspect resource diagnostics (registered task count, current task-stack length, and per-event listener counts).API#getResourceDiagnosticstests for pruning/ejection of previously registered tasks.