feat(ai-chat): agent workspace phases 1 to 3, assistant mode, approval floor, session isolation - #2384
feat(ai-chat): agent workspace phases 1 to 3, assistant mode, approval floor, session isolation#2384J2TeamNNL wants to merge 11 commits into
Conversation
…pprovals per session
…der lease and tool scope
…s, results and schema changes
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79f7c788b0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| else { return } | ||
| self?.providerWaitReason = ProviderStreamLease.waitMessage(providerName: leaseProviderName) | ||
| } | ||
| await ProviderStreamLease.shared.acquire(configId: leaseConfigId, sessionId: leaseSessionId) |
There was a problem hiding this comment.
When two agent sessions use the same Copilot provider configuration, this lease only serializes their turns; it does not switch the cached CopilotChatProvider's conversationId. After session A releases the lease, session B therefore appends its prompt to A's server-side conversation, and subsequent turns from both sessions share context despite having separate local transcripts. Store Copilot conversation state per agent session or explicitly swap/reset it when ownership changes.
Useful? React with 👍 / 👎.
| "version": .string(Bundle.main.appVersion) | ||
| ]) | ||
| ]) | ||
| ) |
There was a problem hiding this comment.
For an outside MCP server that enforces the protocol lifecycle, completing initialize is not sufficient: the client must send notifications/initialized before issuing tools/list or tools/call. This method returns immediately after the initialize response, so such servers reject the following tools/list request and none of their tools are registered.
Useful? React with 👍 / 👎.
| return MCPClientSession( | ||
| configuration: configuration, | ||
| transport: MCPStreamableHttpClientTransport(credentialsProvider: provider), |
There was a problem hiding this comment.
When an outside Streamable HTTP MCP server returns an Mcp-Session-Id during initialization, this transport cannot preserve it: MCPStreamableHttpClientTransport exposes only response bodies and its later requests never include the negotiated header. Consequently the initialize request can succeed while the immediately following tools/list is rejected by any sessionful server. Use a client transport that captures the initialization header and sends it on subsequent requests.
Useful? React with 👍 / 👎.
| /// Sessions are listed again before any window asks for one, so a session whose window was | ||
| /// closed last run is in the rail from the start rather than appearing once its connection | ||
| /// happens to be opened. | ||
| Task { await AgentSessionRegistry.shared.restore() } |
There was a problem hiding this comment.
On launch, this unstructured task does not actually ensure restoration finishes before a window or welcome action calls session(for:). If that happens while store.load() is suspended, the registry creates a new default session and later appends the stored session as well, leaving duplicate sessions for the same conversation in the rail and persisting both. Gate session creation on restoration completion or perform restoration before exposing the registry to UI actions.
Useful? React with 👍 / 👎.
| updatedAt: record.updatedAt, | ||
| approvals: approvals | ||
| ) | ||
| restored.append(session) |
There was a problem hiding this comment.
After relaunch, restored sessions never authorize or register their outside MCP tools. MCPRemoteToolCoordinator.attach is invoked only from makeSession, while this restore path constructs and appends sessions directly; reopening one through session(for:) returns the existing session without attaching it. Thus an allowlisted server's tools disappear from every restored session until the user creates a brand-new session.
Useful? React with 👍 / 👎.
… alert instead of the window
1 participant
All seven phases of the agent workspace plan. The assistant gets the whole window, every write it proposes waits for a human, sessions are isolated and run in parallel, the pane beside the conversation shows what the session actually did, the welcome window is a second way in, and a session can call an MCP server that is not TablePro.
What is here
f67878136rootViewon the three panesWorkspacePanesalready owns, rather than nesting a split view965fc20d6ApprovalRequestID(sessionId:toolUseId:), and a call is evaluated against the connection it targetsa9a35ad7e3001d2693AgentSessionRegistry, not in a windowb2435e4cc183c9093fbe5b966c6Phase 4, sessions are not owned by a window
The four process-global singletons phase 3 split up were still reached through a field on the window's right panel, so a session's lifetime was the window's.
AgentSessionRegistryholds them instead.RightPanelState.aiViewModelwas a creating getter read from inside SwiftUI bodies. It is nowsession, a read, plusstartSession(), which only a user action or an explicit.taskcalls. Opening a connection creates no session.teardown()used to callclearSessionData(), which emptiedmessages. Window close, disconnect and session loss all reach that path, so a transcript the user never asked to lose was gone from three ordinary places. It now stops the session: cancel, persist the partial turn, markstopped, and release only the derived context a reopened session rebuilds.ToolApprovalCenter,ProviderStreamLease) are outside the observation graph, so a rail row that asked them a question would render once and never update.applicationWillTerminatepersists every session and marks a working onefailed. Nothing persisted AI state at quit before, so a session killed mid-stream came back with its last turn missing. The transcript write needed a synchronous path (AIChatStorage.saveSync) because an actor hop at terminate may never be scheduled.Phase 5, the result pane
AgentArtifactProjectionis a pure function over the session's ownChatTurnhistory rather than a second observable store. That is what makes a restored session's pane correct with no replay: the transcript is what was restored, and there is one record of "waiting" instead of two that can disagree.Two things had to change underneath it:
resolveAndAwaitApprovalsappended every pending block at once but awaited them one at a time, so only the first had a continuation registered: a click on the third card hitresolve's missing-continuation guard and did nothing while the stream stayed parked on the first. Every waiting call now registers up front, andToolApprovalCenteralso buffers a decision that arrives before its turn..keyboardShortcut(.defaultAction), soReturnfired whichever button AppKit reached first. Only the first row still waiting takes it, resolved from the transcript through\.chatPrimaryPendingToolUseId.ExplainQueryChatToolwraps the server-side explain tool per decision 5, deliberately without itsanalyzeparameter: a.readOnlychat tool is auto-approved, andanalyzeruns the statement for real.DDLChangeReaderis certainty-or-raw-SQL, and it does not reuseQueryClassifier.strippingStringLiteralsbecause that treats a backticked identifier as a literal and removes it, which would lose the only objectDROP TABLE `order items`names.Phase 7, outside MCP servers
Answered the two blocking questions as the plan recommended: per connection and HTTP-only.
ext__<serverUUID>__, keyed on the id rather than the name, andtablepro,table-pro,table_proare reserved slugs. A server the user called "TablePro" would otherwise land insideClaudeAgentProvider's pre-approvedmcp__tablepro__*wildcard.computeInitialApprovalStateforces every remote call to.pending, checked ahead of the.readOnlyshortcut, in every chat mode, whateveraiAlwaysAllowedToolsholds. "Read-only" is the server's claim about itself.https.Bugs found and fixed along the way
Each of these was pre-existing, not introduced here:
aiViewModelwas a weak snapshot taken inonAppear, before a session existed.Returnacted on whichever approval button AppKit reached first.accessibilityDescription, which was nil.docs/scripts/check-writing-style.shfailed under a C locale, because its bracket expressions over non-ASCII glyphs match individual bytes there and every glyph it checks starts0xE2. It reported every ellipsis in the corpus as a modifier glyph.ImportFromAppSourcePickerhad alegacy_swiftui_aspect_ratioviolation onmain.Verification
Merged
upstream/main(8 commits, the Compare & Sync and routines work) into the branch; the only conflict wasCHANGELOG.md, resolved into one[Unreleased]block in canonical section order.** BUILD SUCCEEDED **after the merge.swiftlint lint --strictclean overTablePro TableProTests TableProUITests(5,187 files).docs/scripts/check-writing-style.shanddocs/scripts/check-docs-against-source.pyboth pass.New suites, all green:
AgentSessionRegistryTests(13),AgentSessionStatusTests(13),AgentSessionPendingPromptTests(6),AIChatPersistenceTests(5),AgentArtifactProjectionTests(15),DDLChangeReaderTests(14),ToolApprovalCenterOrderingTests(6),AgentLaunchRoutingTests(4),MCPServerConfigurationTests(8),MCPRemoteToolPolicyTests(12),MCPRemoteToolApprovalTests(5),MCPAuditChainVersioningTests(6).ConnectionWindowPaneResolverTestsextended with the mode matrix.CI
Unit tests,Package Tests,Validate docs,Lint workflows and scriptsandBuild for testingall pass.UI testsfails on all three shards, and the same tests fail onupstream/mainitself (run 32631046978,3848a21a1), so this is not a regression from this branch:testCompareSyncOpensFromFileMenutestCompareIsDisabledUntilBothEndpointsAreChosentestTargetPickerStartsWithNoConnectionChosentestSwapIsDisabledWhenNoEndpointIsChosentestBannerStatesNothingHasBeenWrittenBeforeAnyRuntestRunInNewTabOpensATabAndActuallyRunsTheQuerytestCommandDeleteDeletesTheEditorLineAfterSelectingAResultRowtestCommandReturnOpensTheResultInANewTabtestAFailedQueryShowsTheDatabaseErrortestHelpMenuOpensTheSampleDatabaseThe first five are
CompareSyncUITests, which arrived with the Compare & Sync window in3848a21a1and have never been green. The rest are the "sample database never finished opening" family, whose membership drifts run to run.Nothing in either list touches assistant mode, sessions, the result pane, the welcome panel or the MCP client.
Confirmed locally as well: the eight suites that touch a surface this PR changes all pass (18 cases across
SingleWindowMenuContractUITests,AuxiliaryWindowCloseUITests,TableProLaunchUITests,NewConnectionCommandUITests,DataSettingsUITests,SettingsWindowTitleUITests,ConnectionCloseUITests), and the only local failures are the same fiveCompareSyncUITests.CompareSyncUITestsis now quarantined (33525726f), with the root cause written into the entry:CompareSyncLauncher.opengates onLicenseManager.isFeatureAvailable(.compareSync)and callsNSAlert.runModal()when the licence is absent, which it always is underUITestCase.launchApp()'s throwaway container. The suite's ownguard item.isEnabled else { throw XCTSkip(...) }cannot fire, because the gate is in the launcher rather than in menu validation. The modal then holds the main thread, which is why cases after it in the same shard fail on unrelated assertions ("The sample database never finished opening", "Not hittable") — one licence gate takes several unrelated tests with it. That is worth fixing on main; it is not this PR's to fix.Not done
TableProUITestscoverage for assistant mode. Written and then withdrawn rather than landed: the mode control sits in the toolbar's overflow menu at the test window's width, and the suite has no AI provider, so an approval card cannot be reached at all. A suite that self-skips reads as coverage without being any. The deterministic parts it would have asserted (the pane's four views and their empty states, the approval ordering, the remote-tool gate) are covered by the unit suites above.The local-only unit failures
Worth recording, because the previous version of this description called the suite "red before this branch" with 33 failing entries, and CI says otherwise:
Unit testspasses on this branch in CI. The failures are specific to the machine I ran on, not to the branch.29 tests failed locally; one was a real test bug and is fixed. The other 28 are environment-sensitive and reproduce on that machine deterministically, in isolation, on
upstream/mainas well:79f7c788b):ValidateDriverDescriptorTests(2) asserted"MySQL"was already claimed "by the built-in MySQL plugin". Nothing claims it under XCTest, becauseapplicationDidFinishLaunchingreturns early whenXCTestConfigurationFilePathis set, so no plugin ever loads anddriverPluginsis empty. The duplicate check the tests exist to prove had nothing to collide with. The tests now seed the occupant themselves.StructureChangeManagerUndoTests(3).StructureChangeManager'sUndoManagerleavesgroupsByEventat its defaulttrue, so undo granularity is decided by run-loop boundaries: the same two column edits are one undo step or two depending on when the loop turns.multipleUndospasses as a single test, fails with its suite, and.serializeddoes not help. CI's timing happens to fall the right way. The fix is to make each mutation an explicit undo group instead of depending on the run loop, which changes Structure-tab undo behaviour and wants its own PR.AWSSSOFetchTests(7),SSEEventStreamTests(3),SaveCompletionTests(3),DataChangeManagerExtendedTests(2),MCPHttpServerTransportTests/MCPHttpKeepAliveTests/MCPHttpServerTransportPairingTests(3, ports),SequelAceImporterTests/TablePlusImporterTests(2, these read for other apps' files on disk),SchemaColumnStoreCancellationTests/ScopedDriverCancellationTests(2),SQLCompletionProviderTests(1),SSHMatchExecutorTests(1).