Skip to content

Fix bind_context active workspace routing - #812

Open
nickdavis wants to merge 4 commits into
repoprompt:mainfrom
nickdavis:codex/fix-bind-context-routing
Open

Fix bind_context active workspace routing#812
nickdavis wants to merge 4 commits into
repoprompt:mainfrom
nickdavis:codex/fix-bind-context-routing

Conversation

@nickdavis

@nickdavis nickdavis commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • restrict context_id bind candidates to contexts in the workspace actively shown by each window
  • fail closed with actionable working_dirs guidance when no open window actively hosts the context
  • preserve intentional stored/inactive lookup for internal routing and Oracle continuation
  • add two-window regressions for stale affinity, lowest-window fallback, active matches, and same-workspace multi-window routing
  • align protected-mutation integration setup so context_id binding uses a genuinely active synthetic workspace

Validation

  • RepoPrompt CE read-only review: no blockers
  • contribution commit and push preflights: passed
  • strict SwiftFormat/SwiftLint: passed
  • RepoPrompt product build: passed
  • BindContextRoutingRecoveryTests: 10 passed
  • TabContextRoutingTests: 62 passed
  • MCPProtectedMutationInvocationIntegrationTests: 4 passed
  • PersistentMCPDistinctConnectionConcurrencyTests: 9 passed
  • ContextBuilderWorktreeInheritanceTests: 8 passed
  • targeted workspace-index timeout case: passed
  • targeted auth-recovery flaky case: passed

The aggregate local pr-ready suite reached its one-hour timeout due suite-order/global-state failures. The routing-related failures found by the first run were corrected; the remaining timeout case and known auth-recovery race both pass independently. Hosted exact-head checks remain the final backstop.

Scope

No locks, serialization, telemetry, retries, or project-specific routing behavior were added. The broad shared-fixture activation attempted during validation was removed in favor of narrow test-local setup.

Comment on lines 6505 to 6511
collectMatchesForContextID: { contextID in
await MainActor.run {
WindowStatesManager.shared.allWindows.compactMap { windowState in
guard let candidate = windowState.workspaceManager.bindingCandidate(forContextID: contextID) else {
guard let candidate = windowState.workspaceManager.storedBindingCandidate(forContextID: contextID) else {
return nil
}
return MCPContextBindingMatch(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The debug scope validation incorrectly fails if a bound context exists in a non-active workspace, as bindingCandidate(forContextID:) now only searches the active one.
Severity: LOW

Suggested Fix

The debug scope validation should be updated to search across all workspaces for a binding candidate, not just the active one. Consider using a method similar to storedBindingCandidate which performs a broader search, ensuring the check doesn't fail when the context is valid but in an inactive workspace.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: Sources/RepoPrompt/Infrastructure/MCP/MCPConnectionManager.swift#L6505-L6511

Potential issue: A debug-only scope validation check in
`MCPConnectionManager+DebugDiagnosticsWorktreeStartup.swift` will incorrectly fail when
a user's MCP connection is bound to a context in a non-active workspace. The refactored
`bindingCandidate(forContextID:)` method now only searches the active workspace.
Consequently, the check `window.workspaceManager.bindingCandidate(forContextID:
boundContextID)?.workspaceID == workspace.id` fails by returning `nil` if the bound
context is in another workspace. This throws a
`DebugWorktreeStartupBenchmarkError.invalidScope` error, creating a false negative
during diagnostic tests, as the connection is legitimately bound.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a valid issue. The changed resolver at this location intentionally uses storedBindingCandidate for generic/internal routing that may target an inactive stored workspace.\n\nThe separate DEBUG benchmark validator has a stricter contract: it first requires an activeWorkspace, then requires boundWorkspaceID == activeWorkspace.id, and finally verifies that boundContextID belongs to that active workspace. A binding to a non-active workspace is intentionally invalid for this visible-workspace benchmark scope. Keeping bindingCandidate active-only makes the final check consistent with those preceding guards; changing it to storedBindingCandidate would weaken the diagnostic invariant rather than fix a false negative.

@baron

baron commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Can you please explain the use case/issue that motivated you to make this PR. What problem are we trying to solve? Where did this fail you?

@nickdavis

nickdavis commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

The motivating failure was a deterministic cross-window routing mismatch in self-hosted MCP workflows.

Concrete reproduction:

  1. Open two RepoPrompt windows, each showing a different workspace (A and B).
  2. Window A also has workspace B in its saved workspace list, but B is not active there.
  3. Bind a long-lived MCP connection with workspace B's context_id.

This is a relatively narrow edge case: it requires multiple windows, overlapping saved-workspace metadata, and a long-lived MCP connection with stale affinity (or deterministic lowest-window fallback). Most single-window workflows are unaffected. However, when triggered, it can route Git, file, or worktree operations to a different repository than the bound context reports, so the fix is primarily about fail-closed routing correctness.

Before this change, bindingCandidate(forContextID:) searched every saved workspace in every window. Both windows could therefore claim B's context even though only one visibly hosted it. WindowRoutingService would then prefer stale connection/window affinity, or fall back to the lowest window ID. The result could be a binding tuple containing B's context/workspace metadata but window A's active presentation.

That becomes user-visible when root-oriented operations run next: manage_worktree, Git, and file tools derive their active window/root authority from window A, while the connection reports workspace B metadata. In my repro, those operations targeted the wrong repository. Rebinding with B's exact working_dirs selected the window actually showing B and repaired the affinity, which isolated the defect to context_id candidate eligibility rather than Git/worktree routing itself.

The intended invariant here is therefore narrow: context_id binding may select a window only when that window is actively showing the candidate workspace and contains the context. If no open window actively shows it, bind_context fails closed and tells the caller to bind by working_dirs; it does not silently switch a visible workspace. Internal operations that intentionally address stored/inactive contexts retain a separate storedBindingCandidate path.

@nickdavis

Copy link
Copy Markdown
Contributor Author

Quick status update: the production routing invariant in this PR still looks correct—the context_id binding path should only select a window actively showing the target workspace, while internal stored/inactive routing remains separate.

The hosted failures exposed an incomplete test-fixture migration: some persistent MCP fixtures still bind synthetic contexts without first making their synthetic workspace active. I’m preparing a narrow test-only follow-up that activates the fixture consistently and removes the duplicated suite-local setup. I do not plan to weaken the production active-workspace requirement.

The unrelated Context Builder timeout is being treated separately.

@nickdavis

Copy link
Copy Markdown
Contributor Author

Updated at exact head 41997a62513086febb4e9b758007f89ca8414ee2.

This follow-up completes only the test-fixture migration required by the active-workspace bind_context invariant. It does not change production routing or include the separate secondary-root Agent worktree work.

Validation:

  • BindContextRoutingRecoveryTests: 10/10 passed
  • PersistentMCPDistinctConnectionConcurrencyTests: 9/9 passed
  • MCPToolExecutionWatchdogIntegrationTests: 23/23 passed
  • MCPProtectedMutationInvocationIntegrationTests: 4/4 passed during focused validation
  • strict Swift lint and git diff --check: passed
  • commit and push contributor preflights: passed, including staged/outgoing secret scans and repository guardrails

The path-selected full root-test lane completed with both directly changed suites passing, but remained red from shared-state/time-out failures in unrelated Context Builder and Git cancellation tests plus a later protected-mutation shared-server timeout. Those failures are recorded rather than hidden; exact-head hosted CI is now queued to validate the isolated PR integration.

morluto commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Audit disposition — best immediate merge candidate (2026-08-14)

The external context_id bind path is correctly restricted to contexts in each window’s active workspace, while stored/inactive lookup remains explicit for Oracle continuation and internal routing. The two-window regressions cover stale affinity, preferred-window routing, deterministic fallback, active same-workspace matches, and inactive-only failure.

Exact-head CI was green in the reviewed state, and I did not find a code-level blocker. Subject to normal branch protection and a non-author approval, this is suitable to merge.

morluto commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Deep-review assessment — 2026-08-14

Disposition: merge candidate. This establishes the right authority split: external bind_context context_id routing only considers contexts in the workspace actively shown by each window, while stored/inactive lookup remains available through explicitly named internal paths for Oracle continuation and other non-binding use cases. Preferred-window and deterministic fallback behavior are clear, and the two-window regressions cover the stale-affinity failure mode.

I did not find a code-level blocker, and the reviewed exact-head hosted CI was green. Obtain the required non-author review and merge without broadening this into general routing or synchronization work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants