fix: codex permission and plan#10
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR overhauls MCP configuration ownership and threads Codex permission/plan modes through the app server. RxCode now stores its own MCP config and generates per-provider configs at launch (replacing the prior "shell out to claude mcp …" model), Codex turn/start carries explicit approvalPolicy/sandbox plus a synthesized ExitPlanMode for plan mode, and ClaudeService gains a session-id descendant tracker to reap subagents that escape the original pgid.
Changes:
- Replace CLI-shelling MCPService with an RxCode-owned
mcp.jsonplus generated Claude/Codex configs and global/project override semantics. - Wire Codex permission/plan modes (approval policy, sandbox, plan-mode developer instructions, synthesized
ExitPlanMode, MCP-coverrides) end-to-end and filter pending-permission UI to the active session. - Track Claude descendants via
POSIX_SPAWN_SETSID+ ppid/sid polling so reparented subagents are reliably killed on cancel/finalize.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| RxCode/Services/MCPService.swift | Rewrites MCP service to own config, adds Claude/Codex generators, importers, and stdio probe changes |
| RxCode/Services/CodexAppServer.swift | Adds plan/permission params, MCP overrides, synthesized ExitPlanMode, auto-accept short-circuit |
| RxCode/Services/ClaudeService.swift | Adds descendant tracker + SETSID-based cleanup; threads mcpConfigPath |
| RxCode/App/AppState.swift | Wires new MCP API, simplifies SummarizationProvider, refreshes MCP on project switch |
| RxCode/Views/Settings/MCPSettingsTab.swift | Replaces scope sections with single per-server view + override controls |
| RxCode/Views/SettingsView.swift | Simplifies summarization provider copy/options |
| RxCode/Views/Permission/PermissionQueueBanner.swift | Filters banner to current session |
| RxCode/Views/MainView.swift | Filters presented permission to current session |
| RxCode/Views/Toolbar/TodoProgressToolbarItem.swift | Doc tweak for Codex plan source |
| Packages/Sources/RxCodeCore/Models/MCPServer.swift | Adds MCPProjectOverride, MCPServerRecord, MCPConfiguration |
| Packages/Sources/RxCodeCore/Models/StreamEvent.swift | Adds todoSnapshot event + TodoSnapshotEvent |
| Packages/Sources/RxCodeCore/Models/TodoItem.swift | Adds parseCodexPlanUpdate |
| Packages/Sources/RxCodeCore/Models/TodoSnapshot.swift | Doc tweak |
| Packages/Tests/RxCodeCoreTests/TodoExtractorTests.swift | New tests for Codex plan parsing |
| CLAUDE.md / AGENTS.md | Doc/table reformat + writing-rule update |
Comments suppressed due to low confidence (8)
RxCode/Services/MCPService.swift:760
- The new
readJSONLineis a regression vs. the previous implementation. It now performs a singleavailableDataread and discards everything after the first newline:
- If the MCP server emits a non-JSON banner line (warnings, version notices, etc.) before the JSON-RPC reply, the function returns nil — the previous code skipped non-JSON lines and kept reading.
- If the JSON reply is split across multiple kernel reads (large
tools/listresponses are commonly >4KB and arrive in chunks), only the first chunk is examined; the partial JSON fails to parse and the probe reports "No response to initialize." or "Server did not return a tools list." - If both the initialize response and the tools/list response happen to be flushed together into one chunk, the second line is silently dropped.
The previous implementation maintained a buffer across multiple availableData reads and looped until a complete JSON object was decoded. The new version cannot reproduce that behavior and will break stdio MCP probing for many real servers.
private nonisolated static func readJSONLine(from handle: FileHandle) async -> [String: Any]? {
await Task.detached {
let data = handle.availableData
guard !data.isEmpty,
let text = String(data: data, encoding: .utf8),
let line = text.split(whereSeparator: \.isNewline).first else {
return nil
}
let lineData = Data(String(line).utf8)
return (try? JSONSerialization.jsonObject(with: lineData)) as? [String: Any]
}.value
}
RxCode/Services/MCPService.swift:476
- When emitting Codex MCP overrides for HTTP/SSE transports, only
urlandbearer_token_env_varare written. Anyheadersconfigured for the server (which the Claude path does emit and whichMCPServerRecord.headerscarries) are silently dropped, so authenticated HTTP/SSE MCP servers will fail to connect when launched under Codex even though they work under Claude.
case .http, .sse:
parts.append("url=\(tomlString(record.url ?? ""))")
if let bearer = record.bearerTokenEnvVar, !bearer.isEmpty {
parts.append("bearer_token_env_var=\(tomlString(bearer))")
}
RxCode/Services/MCPService.swift:279
makeInfoandmakeDetailhard-codescope: .userfor every server returned from the new RxCode-owned config, regardless of whether the record actually has any project overrides set. Callers and UI code that still consultMCPServerInfo.scope(e.g., the removal-confirmation dialog message previously, or any future filtering) will see misleading values. Either removescopefromMCPServerInfo/MCPServerDetailentirely (since RxCode is now the source of truth and the old scope distinction no longer applies) or compute it fromprojectOverride/isGloballyEnabledso it isn't a lie.
private func makeInfo(record: MCPServerRecord, projectPath: String?) -> MCPServerInfo {
MCPServerInfo(
name: record.name,
transport: record.transport,
endpoint: endpoint(from: record),
status: .unknown,
scope: .user,
projectPath: projectPath,
isGloballyEnabled: record.isGloballyEnabled,
projectOverride: record.projectOverride(for: projectPath),
effectiveEnabled: record.isEnabled(for: projectPath)
)
}
private func makeDetail(record: MCPServerRecord, projectPath: String?) -> MCPServerDetail {
MCPServerDetail(
name: record.name,
scope: .user,
transport: record.transport,
url: record.transport == .stdio ? nil : record.url,
command: record.transport == .stdio ? record.command : nil,
args: record.args,
env: record.env,
headers: record.headers,
projectPath: projectPath
)
}
RxCode/Services/MCPService.swift:107
remove(name:scope:)ignores thescopeargument entirely and always deletes the record from the global config. The previous behavior allowed scope-targeted removal (project-scope removal would not affect the user-scope row of the same name). After this PR, "Remove" from the project view will purge the server for every project and globally — there is no way to remove only the project override. If that's the intent, thescopeparameter should be dropped from the API; otherwise the implementation needs to remove only the relevant projectOverride or only the global record when scope ≠.user.
func remove(name: String, scope: MCPScope) async throws {
var config = try loadConfig()
config.servers.removeAll { $0.name == name }
try saveConfig(config)
}
RxCode/Services/MCPService.swift:403
importCodexConfigis a hand-rolled TOML parser that doesn't recognize[[mcp_servers.foo.args]]-style nesting, multi-line arrays, comments after values, or escaped/multi-line strings. More importantlytomlArrayValuessplits theargs = [...]body on raw,which will mis-tokenize any arg containing a comma (e.g.--filter=a,b). Since the import runs once and overwrites the on-disk config, a partial parse permanently loses or corrupts existing Codex MCP entries. Consider using a real TOML parser, or bailing out (returning[]) when any line in the[mcp_servers.*]section can't be parsed unambiguously.
private func importCodexConfig() -> [MCPServerRecord] {
let path = (NSHomeDirectory() as NSString).appendingPathComponent(".codex/config.toml")
guard let text = try? String(contentsOfFile: path, encoding: .utf8) else { return [] }
var records: [MCPServerRecord] = []
var currentName: String?
var fields: [String: String] = [:]
func flush() {
guard let name = currentName else { return }
let enabled = fields["enabled"].map { $0.trimmingCharacters(in: .whitespaces) != "false" } ?? true
if let url = fields["url"]?.tomlUnquoted {
records.append(MCPServerRecord(name: name, transport: .http, url: url, isGloballyEnabled: enabled))
} else if let command = fields["command"]?.tomlUnquoted {
let args = fields["args"]?.tomlArrayValues ?? []
let cwd = fields["cwd"]?.tomlUnquoted
records.append(MCPServerRecord(name: name, transport: .stdio, command: command, args: args, cwd: cwd, isGloballyEnabled: enabled))
}
}
for rawLine in text.split(whereSeparator: \.isNewline).map(String.init) {
let line = rawLine.trimmingCharacters(in: .whitespaces)
guard !line.isEmpty, !line.hasPrefix("#") else { continue }
if line.hasPrefix("[mcp_servers."), line.hasSuffix("]") {
flush()
let name = String(line.dropFirst("[mcp_servers.".count).dropLast())
currentName = name.trimmingCharacters(in: CharacterSet(charactersIn: "\""))
fields = [:]
} else if currentName != nil, let equal = line.firstIndex(of: "=") {
let key = String(line[..<equal]).trimmingCharacters(in: .whitespaces)
let value = String(line[line.index(after: equal)...]).trimmingCharacters(in: .whitespaces)
fields[key] = value
}
}
flush()
return records
}
RxCode/Services/MCPService.swift:234
- When
loadConfigfinds no on-disk file, it builds a fresh import viaimportExistingConfigs()and then immediately callstry saveConfig(imported). If the disk write fails (permissions, disk full, sandbox), every subsequent operation that reads the config —list,get,add,setGlobalEnabled,setProjectOverride,codexConfigOverrides,writeClaudeConfig— will throw too because each callstry loadConfig(), which re-runs the failing import + save path. Returning the imported config without persisting (or persisting best-effort and ignoring the failure) would make the service resilient to a transient write failure.
private func loadConfig() throws -> MCPConfiguration {
let url = configURL()
if FileManager.default.fileExists(atPath: url.path) {
let data = try Data(contentsOf: url)
return try JSONDecoder().decode(MCPConfiguration.self, from: data)
}
let imported = importExistingConfigs()
try saveConfig(imported)
return imported
}
RxCode/Services/MCPService.swift:164
codexConfigOverridesalways emitsenabled=falsefor disabled servers via-c "mcp_servers.<name>={enabled=false,...}". The TOML inline table for stdio always includescommand, but for stdio entries that originated from Claude config (no Codexcommandfield is required to be valid Codex schema) the Codex CLI may still reject the override if any field shape is incompatible. More concerning: emitting overrides for every server on every spawn (including disabled ones) inflates the command line and starts a process for servers the user explicitly disabled — Codex still loads the config, parses, and discovers them. If the goal is "global default plus per-project override," consider only passing entries for servers that are enabled for this project, mirroringenabledRecordsused for the Claude path.
func codexConfigOverrides(projectPath: String?) async -> [String] {
do {
let config = try loadConfig()
var pairs: [String] = []
for record in config.servers.sorted(by: { $0.name < $1.name }) {
// Always emit the full inline table. A bare `enabled=false`
// override fails codex's validation ("invalid transport") when
// the server isn't already defined in ~/.codex/config.toml.
let key = "mcp_servers.\(tomlKey(record.name))"
let enabled = record.isEnabled(for: projectPath)
pairs += ["-c", "\(key)=\(tomlInlineTable(for: record, enabled: enabled))"]
}
return pairs
} catch {
logger.warning("Failed to build Codex MCP overrides: \(error.localizedDescription)")
return []
}
}
RxCode/Services/CodexAppServer.swift:607
- The auto-accept short-circuit for
.auto/.bypassPermissionsreturns a decision payload of{"decision": "accept"}. Other approval handlers in this file (and the user-prompted path below) include richer fields likeselectedOption,optionId, etc., depending on the approval method. Foritem/commandExecution/requestApprovalCodex commonly expects adecisionof"approved"(not"accept") and may also need anoptionId. Verify against the Codex app-server protocol — an unrecognized decision string will likely cause Codex to treat it as a rejection or stall the turn.
// Belt-and-suspenders: even though we set approvalPolicy on thread/start,
// older codex versions may still escalate. Auto-accept when the user picked
// .auto or .bypassPermissions and we're not in plan mode.
if !planMode, permissionMode == .auto || permissionMode == .bypassPermissions {
try Self.writeJSONLine(Self.response(id: requestId, result: [
"decision": .string("accept")
]), to: stdin)
return
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+84
to
98
| if scope != .user, let projectPath, !projectPath.isEmpty { | ||
| record.isGloballyEnabled = false | ||
| record.projectOverrides[projectPath] = .enabled | ||
| } | ||
|
|
||
| for path in candidatePaths { | ||
| if let entry = root?.projects?[path]?.mcpServers?[name] { | ||
| return makeDetail(name: name, entry: entry, scope: .local, projectPath: path) | ||
| } | ||
| let disabled = Set(root?.projects?[path]?.disabledMcpjsonServers ?? []) | ||
| if !disabled.contains(name), | ||
| let entry = readProjectMCPFile(projectRoot: path)?.mcpServers?[name] { | ||
| return makeDetail(name: name, entry: entry, scope: .project, projectPath: path) | ||
| if let index = config.servers.firstIndex(where: { $0.name == name }) { | ||
| let existing = config.servers[index] | ||
| record.projectOverrides = existing.projectOverrides.merging(record.projectOverrides) { _, new in new } | ||
| if scope == .user { | ||
| record.isGloballyEnabled = existing.isGloballyEnabled | ||
| } | ||
| config.servers[index] = record | ||
| } else { | ||
| config.servers.append(record) | ||
| } |
Comment on lines
+364
to
+366
| let params = method == "thread/start" | ||
| ? threadStartParams(threadId: activeThreadId, cwd: cwd, permissionMode: permissionMode, planMode: planMode) | ||
| : threadParams(threadId: activeThreadId, cwd: cwd) |
Comment on lines
+466
to
+483
| private func startDescendantTracker(streamId: UUID, root: pid_t) { | ||
| // Capture sid once at startup — getsid() on a live root returns the session id, | ||
| // which equals `root` itself since we spawned with POSIX_SPAWN_SETSID. | ||
| let sid = getsid(root) | ||
| let task = Task.detached { [weak self] in | ||
| while !Task.isCancelled { | ||
| let pids = Self.descendantPids(of: root, sid: sid) | ||
| if !pids.isEmpty { | ||
| await self?.mergeTrackedDescendants(streamId: streamId, pids: pids) | ||
| } | ||
| // 500ms is a balance: short enough to catch transient ppid links before | ||
| // an intermediate parent dies and reparenting hides the child, while not | ||
| // burning measurable CPU on the `ps` invocation (~5ms per snapshot). | ||
| try? await Task.sleep(nanoseconds: 500_000_000) | ||
| } | ||
| } | ||
| descendantTrackers[streamId] = task | ||
| } |
Comment on lines
893
to
+912
| private func removeProcess(streamId: UUID) { | ||
| streamPGIDs.removeValue(forKey: streamId) | ||
| descendantTrackers.removeValue(forKey: streamId)?.cancel() | ||
| // Retain `trackedDescendants[streamId]` long enough for the SIGKILL re-snapshot | ||
| // in cancel/finalize (those run in detached tasks after this method). Clear it | ||
| // after a short delay so the actor doesn't accumulate stale entries. | ||
| let clearKey = streamId | ||
| Task { [weak self] in | ||
| try? await Task.sleep(nanoseconds: 6_000_000_000) | ||
| await self?.clearTrackedDescendants(streamId: clearKey) | ||
| } | ||
| // If stdin is still open (e.g. abnormal exit before `result`), release the handle. | ||
| if let handle = stdinHandles.removeValue(forKey: streamId) { | ||
| try? handle.close() | ||
| } | ||
| } | ||
|
|
||
| private func clearTrackedDescendants(streamId: UUID) { | ||
| trackedDescendants.removeValue(forKey: streamId) | ||
| } |
Comment on lines
79
to
92
|
|
||
| enum SummarizationProvider: String, CaseIterable, Identifiable { | ||
| case selectedClient | ||
| case claudeCode | ||
| case codex | ||
| case openAI | ||
|
|
||
| var id: String { rawValue } | ||
|
|
||
| var displayName: String { | ||
| switch self { | ||
| case .selectedClient: return "Selected Client" | ||
| case .claudeCode: return "Claude Code" | ||
| case .codex: return "Codex" | ||
| case .openAI: return "OpenAI" | ||
| case .selectedClient: return "Thread Model" | ||
| case .openAI: return "OpenAI-Compatible Endpoint" | ||
| } | ||
| } | ||
| } |
Comment on lines
736
to
755
| @@ -752,7 +748,10 @@ final class AppState { | |||
| endpoint: info.endpoint, | |||
| status: existing.status, | |||
| scope: info.scope, | |||
| projectPath: info.projectPath | |||
| projectPath: info.projectPath, | |||
| isGloballyEnabled: info.isGloballyEnabled, | |||
| projectOverride: info.projectOverride, | |||
| effectiveEnabled: info.effectiveEnabled | |||
| )) | |||
Comment on lines
+319
to
+330
| private var effectiveBinding: Binding<Bool> { | ||
| Binding( | ||
| get: { server.effectiveEnabled }, | ||
| set: { newValue in | ||
| if inProject { | ||
| onProjectOverrideChange(newValue ? .enabled : .disabled) | ||
| } else { | ||
| onGlobalEnabledChange(newValue) | ||
| } | ||
| } | ||
| ) | ||
| } |
Contributor
Author
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.