Support configurable Oracle groups of up to five - #782
Conversation
e5ab303 to
d36b9aa
Compare
d36b9aa to
d23a0c5
Compare
d23a0c5 to
105ec1d
Compare
105ec1d to
38e256e
Compare
Await workspace chat restore after switches so dual-Oracle tests no longer race ensureActiveSession, isolate secondary model in strict finalization coverage, fail closed when Primary fails in a pair, and keep dual MCP payloads routeable with top-level partial errors.
SwiftFormat redundantSelf on the dual-Oracle workspace restore listener.
|
Current-main refresh on exact head Please keep it draft for now. App shard 1 has now reported failure while the workflow is still completing, so the current integration is not yet proven. Once that failure is triaged and all required exact-head/current-base checks are green, this should be marked ready for formal review rather than dismissed because it is a draft. |
baron
left a comment
There was a problem hiding this comment.
Request changes
I reviewed the exact current-main integration (e314be39 + c7994839, synthetic merge da7b3dd9). Three issues need resolution before this is safe to approve.
P1 — Fence parent cancellation before returning or persisting a grouped result
OraclePairCoordinator.run drains the task group and constructs a successful result without re-checking the parent task's cancellation state (Sources/RepoPrompt/Infrastructure/MCP/OraclePairCoordinator.swift:148-174). If cancellation arrives after the last child returns but before aggregation, cooperative task-group cancellation does not make a successful child throw. The coordinator can therefore return normally, and tool_groupedChatSend proceeds into group-history persistence and normal result projection (Sources/RepoPrompt/Features/Chat/ViewModels/Oracle/OracleViewModel+MCP.swift:1989-2024).
Please add a cancellation barrier after draining and immediately before the persistence/commit boundary, with a deterministic test that cancels after the final lane succeeds but before result construction. The current cancellation test only cancels lanes while they are suspended in cancellable sleeps, so it does not cover this race.
P1 — The PR's core preparation suite fails reproducibly
Both hosted shard 3 and a clean focused run against the exact current-main synthetic merge fail the same two PR-added tests:
OraclePairPreparationTests.testExplicitContinuationLoadsTheCompletePersistedFiveLaneGroupFromOneColdMember(Tests/RepoPromptTests/Chat/OraclePairPreparationTests.swift:244)OraclePairPreparationTests.testPairedRenamePersistsBothMembersOrLeavesTheVisibleNameUnchanged(Tests/RepoPromptTests/Chat/OraclePairPreparationTests.swift:402)
Both report an unexpected CancellationError at AgentModeViewModel.swift:3123. Every fixture enables deferred initial refresh (OraclePairPreparationTests.swift:942), while fixture cleanup only clears sessions and removes temporary storage (:1042-1045) and does not explicitly settle that background lifecycle. That is the leading test-lifecycle explanation, not proof that the production send/rename operation itself throws; the exact causal path still needs to be established and fixed. Please make the suite own and deterministically settle its background work rather than masking the failure with retries, sleeps, or timeout inflation.
Local validation: ./conductor test --filter OraclePairPreparationTests --async --request-key review-pr-782-oracle-pair — failed, 18 tests / 2 failures, matching hosted CI.
P2 — Collapsed Context Builder cards hide partial Oracle failure
The outer Context Builder result status is derived from discovery termination (MCPContextBuilderToolProvider.swift:653-656), while the nested plan/review retains grouped Oracle partial_failure. ContextBuilderToolCards.status renders only dto.status (Sources/RepoPrompt/Features/AgentMode/Views/ToolCards/ContextBuilderToolCards.swift:193-216). A successful discovery followed by one or more failed Oracle lanes therefore appears as a green completed card until expanded.
Please derive an effective presentation status that promotes nested partial/failure state to warning/failure without changing discovery's ownership of the outer status, and cover the collapsed-card severity.
Maintainer-guidance check
- Authority: cancellation belongs at the group coordinator/caller boundary; nested Oracle status remains owned by the nested result.
- State safety: compatibility/migration and atomic grouped history operations otherwise looked bounded; persistence after parent cancellation remains unsafe.
- Observability: partial Oracle failure must be visible when the card is collapsed.
- Validation: exact-head hosted CI is red and the exact-current-main focused run reproduced it.
- Scope: these are bounded coordinator, fixture-lifecycle, and presentation-status fixes; no architecture rewrite is needed.
Audit disposition — superseded by #796 (2026-08-14)#796 now owns the multi-Oracle design through shared domain-runtime contracts, durable claims/persistence, app and direct-headless adapters, and current exact-head validation. Maintaining this branch in parallel would leave two competing roster, lifecycle, persistence, and compatibility models. Please port any unique cancellation/drain or UI regressions that #796 lacks, preserve contributor credit, and close this PR in favor of #796. I would not revise or rebase #782 wholesale. |
Deep-review assessment — 2026-08-14Disposition: superseded by #796. #796 now owns the current multi-Oracle architecture through shared domain-runtime contracts, ordered lane identity/results, durable persistence and claims, app/direct-headless adapters, Context Builder integration, and current exact-head CI/live evidence. Merging this earlier app-oriented implementation would reintroduce competing roster and persistence models. Please compare the branches only for unique tests or cancellation/drain behavior that #796 does not yet cover, port those narrowly with attribution, and close this PR. The successor should remain the single Oracle-group authority. |
Summary
+control for 0–4 removable additional Oracles (five total)Review focus
Validation
xcruninspection, unavailable on this Linux runnerrepoprompt-mcp --backend headlesswas investigated, but that target is still Darwin-only and does not compile the app-target Oracle implementation; macOS CI is the authoritative build/test result