Repository navigation
fix: preserve Codex provider for workspace auto-naming #15635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -163,6 +163,82 @@ struct AutoNamingEnvironmentPolicy: Sendable { | |||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /// Builds the isolated Codex invocation used for workspace naming. | ||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||
| /// `--ignore-user-config` keeps tools, MCP servers, and rules out of the | ||||||||||||||||||||||||||||||||||||||
| /// summarizer, but it also removes the user's model provider. Re-apply only | ||||||||||||||||||||||||||||||||||||||
| /// the provider selection, its provider table, and the selected model. | ||||||||||||||||||||||||||||||||||||||
| struct CodexAutoNamingArguments: Sendable { | ||||||||||||||||||||||||||||||||||||||
| static func build(configToml: String?) -> [String] { | ||||||||||||||||||||||||||||||||||||||
| var arguments = [ | ||||||||||||||||||||||||||||||||||||||
| "exec", | ||||||||||||||||||||||||||||||||||||||
| "-c", "default_tools_enabled=false", | ||||||||||||||||||||||||||||||||||||||
| "-c", "tools={}", | ||||||||||||||||||||||||||||||||||||||
| "-c", "mcp_servers={}", | ||||||||||||||||||||||||||||||||||||||
| "-c", "web_search=\"disabled\"", | ||||||||||||||||||||||||||||||||||||||
| "-c", "approval_policy=never", | ||||||||||||||||||||||||||||||||||||||
| "-c", "shell_environment_policy.inherit=none", | ||||||||||||||||||||||||||||||||||||||
| "--skip-git-repo-check", | ||||||||||||||||||||||||||||||||||||||
| "--ephemeral", | ||||||||||||||||||||||||||||||||||||||
| "--ignore-user-config", | ||||||||||||||||||||||||||||||||||||||
| "--ignore-rules", | ||||||||||||||||||||||||||||||||||||||
| "--sandbox", "read-only" | ||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||
| guard let configToml else { return arguments } | ||||||||||||||||||||||||||||||||||||||
| let overrides = providerOverrides(from: configToml) | ||||||||||||||||||||||||||||||||||||||
| for override in overrides.reversed() { | ||||||||||||||||||||||||||||||||||||||
| arguments.insert(contentsOf: ["-c", override], at: 1) | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| return arguments | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| private static func providerOverrides(from toml: String) -> [String] { | ||||||||||||||||||||||||||||||||||||||
| var model: String? | ||||||||||||||||||||||||||||||||||||||
| var modelProvider: String? | ||||||||||||||||||||||||||||||||||||||
| var providerEntries: [(section: String, key: String, value: String)] = [] | ||||||||||||||||||||||||||||||||||||||
| var section = "" | ||||||||||||||||||||||||||||||||||||||
| for rawLine in toml.split(whereSeparator: \.isNewline) { | ||||||||||||||||||||||||||||||||||||||
| let line = rawLine.trimmingCharacters(in: .whitespacesAndNewlines) | ||||||||||||||||||||||||||||||||||||||
| guard !line.isEmpty, !line.hasPrefix("#") else { continue } | ||||||||||||||||||||||||||||||||||||||
| if line.first == "[", line.last == "]" { | ||||||||||||||||||||||||||||||||||||||
| section = String(line.dropFirst().dropLast()) | ||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| guard let equals = line.firstIndex(of: "=") else { continue } | ||||||||||||||||||||||||||||||||||||||
| let key = line[..<equals].trimmingCharacters(in: .whitespacesAndNewlines) | ||||||||||||||||||||||||||||||||||||||
| let value = line[line.index(after: equals)...].trimmingCharacters(in: .whitespacesAndNewlines) | ||||||||||||||||||||||||||||||||||||||
| if section.isEmpty { | ||||||||||||||||||||||||||||||||||||||
| if key == "model" { model = String(value) } | ||||||||||||||||||||||||||||||||||||||
| if key == "model_provider" { modelProvider = String(value) } | ||||||||||||||||||||||||||||||||||||||
| } else if section.hasPrefix("model_providers.") { | ||||||||||||||||||||||||||||||||||||||
| providerEntries.append((section, String(key), String(value))) | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| guard let modelProvider, | ||||||||||||||||||||||||||||||||||||||
| let providerName = providerNameFromValue(modelProvider), | ||||||||||||||||||||||||||||||||||||||
| providerName.allSatisfy({ $0.isLetter || $0.isNumber || $0 == "_" || $0 == "-" }) else { | ||||||||||||||||||||||||||||||||||||||
| return model.map { ["model=\($0)"] } ?? [] | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| var result = ["model_provider=\(modelProvider)"] | ||||||||||||||||||||||||||||||||||||||
| if let model { result.append("model=\(model)") } | ||||||||||||||||||||||||||||||||||||||
| result.append(contentsOf: providerEntries | ||||||||||||||||||||||||||||||||||||||
| .filter { $0.section.hasPrefix("model_providers.\(providerName)") } | ||||||||||||||||||||||||||||||||||||||
| .map { | ||||||||||||||||||||||||||||||||||||||
| let prefix = "model_providers.\(providerName)" | ||||||||||||||||||||||||||||||||||||||
| let nestedPath = String($0.section.dropFirst(prefix.count)) | ||||||||||||||||||||||||||||||||||||||
| .trimmingCharacters(in: CharacterSet(charactersIn: ".")) | ||||||||||||||||||||||||||||||||||||||
| let keyPath = nestedPath.isEmpty ? $0.key : "\(nestedPath).\($0.key)" | ||||||||||||||||||||||||||||||||||||||
| return "model_providers.\(providerName).\(keyPath)=\($0.value)" | ||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+224
to
+232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '160,247p' CLI/CMUXCLI+AutoNaming.swift
sed -n '1,75p' cmuxCLITests/CodexAutoNamingArgumentsTests.swiftRepository: manaflow-ai/cmux Length of output: 6089 🏁 Script executed: printf '%s\n' '--- relevant symbols and provider-table references ---'
rg -n --glob '!*.xcodeproj/**' --glob '!DerivedData/**' 'model_providers|CodexAutoNamingArguments|providerOverrides|model_provider' .
printf '%s\n' '--- changed-file context ---'
cat -n CLI/CMUXCLI+AutoNaming.swift | sed -n '185,240p'
printf '%s\n' '--- PR diff summary and focused diff ---'
git diff --stat 860619f53fac450a05ebe3541cab5b4f22fc933c 59948fac376b8c482da938575c4ada3b8853a7e5 -- CLI/CMUXCLI+AutoNaming.swift cmuxCLITests/CodexAutoNamingArgumentsTests.swift
git diff --unified=30 860619f53fac450a05ebe3541cab5b4f22fc933c 59948fac376b8c482da938575c4ada3b8853a7e5 -- CLI/CMUXCLI+AutoNaming.swift cmuxCLITests/CodexAutoNamingArgumentsTests.swiftRepository: manaflow-ai/cmux Length of output: 41727 🌐 Web query:
💡 Result: Match the provider section boundary exactly.
🐛 Suggested fix- result.append(contentsOf: providerEntries
- .filter { $0.section.hasPrefix("model_providers.\(providerName)") }
- .map {
- let prefix = "model_providers.\(providerName)"
+ let prefix = "model_providers.\(providerName)"
+ result.append(contentsOf: providerEntries
+ .filter { $0.section == prefix || $0.section.hasPrefix(prefix + ".") }
+ .map {
let nestedPath = String($0.section.dropFirst(prefix.count))
.trimmingCharacters(in: CharacterSet(charactersIn: "."))Add regression coverage with both 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| private static func providerNameFromValue(_ value: String) -> String? { | ||||||||||||||||||||||||||||||||||||||
| guard value.count >= 2, value.first == "\"", value.last == "\"" else { return nil } | ||||||||||||||||||||||||||||||||||||||
| return String(value.dropFirst().dropLast()) | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /// Pure auto-naming logic: throttle decisions, transcript extraction, | ||||||||||||||||||||||||||||||||||||||
| /// prompt construction, and response sanitization. | ||||||||||||||||||||||||||||||||||||||
| struct AutoNamingEngine: Sendable { | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -148,25 +148,17 @@ extension CMUXCLI { | |||||||||||||||||||||||||
| try? FileManager.default.removeItem(at: outputFile) | ||||||||||||||||||||||||||
| try? FileManager.default.removeItem(at: workingDirectory) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| var arguments = CodexAutoNamingArguments.build( | ||||||||||||||||||||||||||
| configToml: codexConfigToml(from: summarizerEnv) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| arguments += [ | ||||||||||||||||||||||||||
| "--cd", workingDirectory.path, | ||||||||||||||||||||||||||
| "--output-last-message", outputFile.path, | ||||||||||||||||||||||||||
| "-" | ||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||
| guard runAutoNamingSummarizer( | ||||||||||||||||||||||||||
| executable: executable, | ||||||||||||||||||||||||||
| arguments: [ | ||||||||||||||||||||||||||
| "exec", | ||||||||||||||||||||||||||
| "-c", "default_tools_enabled=false", | ||||||||||||||||||||||||||
| "-c", "tools={}", | ||||||||||||||||||||||||||
| "-c", "mcp_servers={}", | ||||||||||||||||||||||||||
| "-c", "web_search=false", | ||||||||||||||||||||||||||
| "-c", "approval_policy=never", | ||||||||||||||||||||||||||
| "-c", "shell_environment_policy.inherit=none", | ||||||||||||||||||||||||||
| "--skip-git-repo-check", | ||||||||||||||||||||||||||
| "--ephemeral", | ||||||||||||||||||||||||||
| "--ignore-user-config", | ||||||||||||||||||||||||||
| "--ignore-rules", | ||||||||||||||||||||||||||
| "--sandbox", "read-only", | ||||||||||||||||||||||||||
| "--cd", workingDirectory.path, | ||||||||||||||||||||||||||
| "--output-last-message", outputFile.path, | ||||||||||||||||||||||||||
| "-" | ||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||
| arguments: arguments, | ||||||||||||||||||||||||||
| prompt: prompt, | ||||||||||||||||||||||||||
| environment: summarizerEnv, | ||||||||||||||||||||||||||
| timeout: timeout | ||||||||||||||||||||||||||
|
|
@@ -175,4 +167,11 @@ extension CMUXCLI { | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return (try? String(contentsOf: outputFile, encoding: .utf8)) ?? "" | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private func codexConfigToml(from env: [String: String]) -> String? { | ||||||||||||||||||||||||||
| let home = env["CODEX_HOME"] ?? | ||||||||||||||||||||||||||
| ((env["HOME"].map { $0 + "/.codex" }) ?? "") | ||||||||||||||||||||||||||
| guard !home.isEmpty else { return nil } | ||||||||||||||||||||||||||
| return try? String(contentsOfFile: home + "/config.toml", encoding: .utf8) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+171
to
+176
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '130,190p' CLI/CMUXCLI+AutoNamingDispatch.swift
rg -n 'CODEX_HOME|expandingTildeInPath|codexConfigToml' CLI cmuxCLITestsRepository: manaflow-ai/cmux Length of output: 11412 🏁 Script executed: set -eu
printf '%s\n' '--- AutoNaming arguments and environment ---'
rg -n -A45 -B15 'struct CodexAutoNamingArguments|enum CodexAutoNamingArguments|CodexAutoNamingArguments\.build|codexSummarizerEnvironment|runAutoNamingSummarizer' CLI cmuxCLITests
printf '%s\n' '--- Relevant test sections ---'
sed -n '1,130p' cmuxCLITests/CLICodexQueuedHookContractTests.swift
sed -n '640,710p' cmuxCLITests/CLICodexHookTimeoutRegressionTests.swift
printf '%s\n' '--- Path normalization helpers and Codex environment handling ---'
sed -n '1,155p' CLI/CMUXCLI+AutoNamingSummarizers.swift
sed -n '90,135p' CLI/CMUXCLI+AutoNaming.swift
rg -n -A25 -B10 'codexSummarizerEnvironment|normalizedHookValue|CODEX_HOME.*empty|CODEX_HOME.*unset' CLIRepository: manaflow-ai/cmux Length of output: 42967 🌐 Web query:
💡 Result: 🏁 Script executed: set -eu
printf '%s\n' '--- Summarizer process runner and working directory ---'
rg -n -A70 -B20 'func runAutoNamingSummarizer|runAutoNamingSummarizer\(' CLI
printf '%s\n' '--- Exact reviewed helper and related path helpers ---'
sed -n '133,185p' CLI/CMUXCLI+AutoNamingDispatch.swift
rg -n -A18 -B8 'URL\(fileURLWithPath: home\)|standardizedFileURL|absoluteURL|expandingTildeInPath' CLI/CMUXCLI+AutoNamingDispatch.swift CLI/CMUXCLI+AutoNamingSummarizers.swift CLI/CMUXCLI+AutoNaming.swiftRepository: manaflow-ai/cmux Length of output: 28651 Treat an empty Codex ignores an empty Codex does not expand a leading Suggested fix private func codexConfigToml(from env: [String: String]) -> String? {
- let home = env["CODEX_HOME"] ??
+ let home = env["CODEX_HOME"].flatMap { $0.isEmpty ? nil : $0 } ??
((env["HOME"].map { $0 + "/.codex" }) ?? "")
guard !home.isEmpty else { return nil }
return try? String(contentsOfFile: home + "/config.toml", encoding: .utf8)📝 Committable suggestion
Suggested change
🧰 Tools🪛 ast-grep (0.45.3)[error] 174-174: A file is read from a path built from runtime/request input via FileManager.contents(atPath:), Data(contentsOf:), or String(contentsOfFile:). An attacker can supply '../' sequences or absolute paths to read files outside the intended directory (path traversal). Validate and canonicalize the path, reject '..' components, and confine reads to an allow-listed base directory (e.g. resolve with URL(fileURLWithPath:relativeTo:) and verify the resolved path is still inside the base) before reading. (path-traversal-file-read-request-input-swift) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import Testing | ||
|
|
||
| struct CodexAutoNamingArgumentsTests { | ||
| @Test func disablesFeaturesAndForwardsSelectedProviderAndModel() { | ||
| let args = CodexAutoNamingArguments.build(configToml: """ | ||
| model = "gpt-5-codex" | ||
| model_provider = "subrouter" | ||
| [model_providers.subrouter] | ||
| name = "Subrouter" | ||
| base_url = "http://127.0.0.1:31415/v1" | ||
| experimental_bearer_token = "secret" | ||
| [model_providers.subrouter.http_headers] | ||
| X-Subrouter-Agent = "sr" | ||
| [profiles.default] | ||
| model = "ignored" | ||
| """) | ||
| let overrides = configOverrides(args) | ||
| #expect(overrides.contains("web_search=\"disabled\"")) | ||
| #expect(!overrides.contains("web_search=false")) | ||
| #expect(overrides.contains("model_provider=\"subrouter\"")) | ||
| #expect(overrides.contains("model=\"gpt-5-codex\"")) | ||
| #expect(overrides.contains("model_providers.subrouter.base_url=\"http://127.0.0.1:31415/v1\"")) | ||
| #expect(overrides.contains("model_providers.subrouter.experimental_bearer_token=\"secret\"")) | ||
| #expect(overrides.contains("model_providers.subrouter.http_headers.X-Subrouter-Agent=\"sr\"")) | ||
| #expect(!overrides.contains(where: { $0.contains("profiles") })) | ||
| #expect(args.contains("--ignore-user-config")) | ||
| #expect(args.contains("--ignore-rules")) | ||
| } | ||
|
|
||
| @Test func keepsIsolationWhenUserConfigIsMissing() { | ||
| let args = CodexAutoNamingArguments.build(configToml: nil) | ||
| let overrides = configOverrides(args) | ||
| #expect(overrides.contains("default_tools_enabled=false")) | ||
| #expect(overrides.contains("tools={}")) | ||
| #expect(overrides.contains("mcp_servers={}")) | ||
| #expect(overrides.contains("web_search=\"disabled\"")) | ||
| #expect(!overrides.contains(where: { $0.hasPrefix("model_provider=") })) | ||
| } | ||
|
|
||
| private func configOverrides(_ args: [String]) -> [String] { | ||
| zip(args, args.dropFirst()).compactMap { pair in | ||
| pair.0 == "-c" ? pair.1 : nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: manaflow-ai/cmux
Length of output: 5900
🏁 Script executed:
Repository: manaflow-ai/cmux
Length of output: 24066
🏁 Script executed:
Repository: manaflow-ai/cmux
Length of output: 9941
🌐 Web query:
official OpenAI Codex CLI config.toml model_providers provider table model field configuration reference💡 Result:
Reset the section when a TOML header has a trailing comment.
For this valid input:
the parser emits:
The Codex config contract defines
modelas a top-level setting, not amodel_providers.<id>field. This can make the auto-naming invocation reject or mis-handle the provider configuration. A provider-prefix filter fix does not remove this exact-section entry.Suggested fix
🤖 Prompt for AI Agents