Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Scripts/Fixtures/test-suite-contract-ledger.tsv
Original file line number Diff line number Diff line change
Expand Up @@ -2019,9 +2019,12 @@ root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testDefaultProfileResolvesOp
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testFailedProbeDoesNotLeaveSpawnableCacheAndReplacementCanRecover root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testFailedProbeDoesNotLeaveSpawnableCacheAndReplacementCanRecover AgentMode unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.314000 unreviewed retain_pending_review 0 initial census source line 178
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testMakeLaunchConfigurationResolvesExplicitPathWithoutPriorProbe root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testMakeLaunchConfigurationResolvesExplicitPathWithoutPriorProbe AgentMode unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.001000 unreviewed retain_pending_review 0 initial census source line 6
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testOpenCodeHomeBinHintDoesNotLeakIntoNativeDefaultsOrOtherProviders root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testOpenCodeHomeBinHintDoesNotLeakIntoNativeDefaultsOrOtherProviders AgentMode unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.000000 unreviewed retain_pending_review 0 initial census source line 79
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testOpenCodeGlobalMCPConfigDiagnosticDetectsRepoPromptServerInGlobalConfig root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testOpenCodeGlobalMCPConfigDiagnosticDetectsRepoPromptServerInGlobalConfig AgentMode opencode_acp.recursive_mcp_config.detect opencode,mcp_config,recursive_launch,user_guidance provider_error_mapping root_swiftpm routine 1 A global OpenCode config containing a RepoPromptCE MCP entry is detected from XDG_CONFIG_HOME and reports the config path, suspicious key, recursive-launch explanation, and mcp-empty workaround. RepoPrompt CE could keep showing generic OpenCode startup timeout guidance for a self-recursive OpenCode MCP configuration that blocks session/new. test_case retain 0 Follow-up to #117 root cause from @jpmurray: OpenCode global MCP config can recursively launch RepoPrompt CE.
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testOpenCodeGlobalMCPConfigDiagnosticIgnoresEmptyAndUnrelatedMCPConfig root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testOpenCodeGlobalMCPConfigDiagnosticIgnoresEmptyAndUnrelatedMCPConfig AgentMode opencode_acp.recursive_mcp_config.no_false_positive opencode,mcp_config,negative_boundary protocol_negative root_swiftpm routine 1 Empty mcp blocks and unrelated MCP server entries do not produce recursive RepoPrompt MCP diagnostics. Users with normal OpenCode MCP configuration could receive misleading RepoPrompt recursion guidance. test_case retain 0 Boundary coverage for OpenCode recursive MCP config diagnostic.
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testPromptTimeoutDoesNotUseOpenCodeStartupGuidance root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testPromptTimeoutDoesNotUseOpenCodeStartupGuidance AgentMode opencode_acp.timeout.prompt_passthrough timeout,provider_error_boundary,opencode protocol_negative root_swiftpm routine 1 A non-startup ACPRequestTimeoutError for session/prompt remains an apiError carrying the original timeout instead of OpenCode startup-recovery guidance. Mid-session model or prompt stalls could be mislabeled as OpenCode installation/configuration failures, sending users toward irrelevant upgrade guidance. test_case retain 0 PR OpenCode ACP startup timeout guidance boundary coverage
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testRepeatedProbeRefreshesCurrentEnvironmentBeforeSpawn root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testRepeatedProbeRefreshesCurrentEnvironmentBeforeSpawn AgentMode unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.299000 unreviewed retain_pending_review 0 initial census source line 90
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testSessionNewTimeoutNormalizesToOpenCodeRecoveryGuidance root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testSessionNewTimeoutNormalizesToOpenCodeRecoveryGuidance AgentMode opencode_acp.timeout.startup_guidance timeout,startup_guidance,version_identity,opencode provider_error_mapping root_swiftpm routine 1 A session/new ACPRequestTimeoutError normalizes to invalidConfiguration containing OpenCode startup guidance, upgrade command, verification command, agent version identity, and the original timeout detail. OpenCode ACP startup hangs could surface as a generic provider/API failure without actionable upgrade or version context for users and support. test_case retain 0 PR OpenCode ACP startup timeout guidance regression coverage
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testSessionNewTimeoutIncludesRecursiveMCPConfigGuidanceWhenDetected root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testSessionNewTimeoutIncludesRecursiveMCPConfigGuidanceWhenDetected AgentMode opencode_acp.timeout.recursive_mcp_guidance timeout,startup_guidance,opencode,mcp_config provider_error_mapping root_swiftpm routine 1 A session/new timeout uses recursive OpenCode MCP config guidance when detected, preserves original server-unavailable detail, and suppresses generic upgrade-first guidance. Known self-recursive OpenCode MCP failures could continue to advise upgrades instead of pointing users to the actual global config problem. test_case retain 0 Follow-up to #117 root cause from @jpmurray.
root/RepoPromptTests.OpenCodeACPLaunchResolverTests/testWorldWritableExecutableDirectoryIsRejectedWithoutExecution root Tests/RepoPromptTests/AgentMode/OpenCodeACPLaunchResolverTests.swift RepoPromptTests.OpenCodeACPLaunchResolverTests testWorldWritableExecutableDirectoryIsRejectedWithoutExecution AgentMode unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.001000 unreviewed retain_pending_review 0 initial census source line 162
root/RepoPromptTests.OracleMessageFinalisationHubTests/testCancellationBeforeRegistrationResumesOnlyMatchingWaiter root Tests/RepoPromptTests/Chat/OracleMessageFinalisationHubTests.swift RepoPromptTests.OracleMessageFinalisationHubTests testCancellationBeforeRegistrationResumesOnlyMatchingWaiter Chat unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.000000 unreviewed retain_pending_review 0 initial census source line 60
root/RepoPromptTests.OracleMessageFinalisationHubTests/testWaiterCancellationIsLocalAndDoesNotCompleteMessage root Tests/RepoPromptTests/Chat/OracleMessageFinalisationHubTests.swift RepoPromptTests.OracleMessageFinalisationHubTests testWaiterCancellationIsLocalAndDoesNotCompleteMessage Chat unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.000000 unreviewed retain_pending_review 0 initial census source line 6
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ struct OpenCodeACPAgentProvider: ACPAgentProvider {
private let config: OpenCodeAgentConfig
private let repoPromptMCPConfiguration: RepoPromptMCPServerConfiguration
private let launchResolver: OpenCodeACPLaunchResolver
private let recursiveMCPConfigGuidance: @Sendable () -> String?

#if DEBUG
var test_config: OpenCodeAgentConfig {
Expand All @@ -18,11 +19,15 @@ struct OpenCodeACPAgentProvider: ACPAgentProvider {
init(
config: OpenCodeAgentConfig,
repoPromptMCPConfiguration: RepoPromptMCPServerConfiguration = .repoPrompt,
launchResolver: OpenCodeACPLaunchResolver = OpenCodeACPLaunchResolver()
launchResolver: OpenCodeACPLaunchResolver = OpenCodeACPLaunchResolver(),
recursiveMCPConfigGuidance: @escaping @Sendable () -> String? = {
OpenCodeGlobalMCPConfigDiagnostic.detect()?.guidanceMessage
}
) {
self.config = config
self.repoPromptMCPConfiguration = repoPromptMCPConfiguration
self.launchResolver = launchResolver
self.recursiveMCPConfigGuidance = recursiveMCPConfigGuidance
}

var providerID: ACPProviderID {
Expand Down Expand Up @@ -128,7 +133,10 @@ struct OpenCodeACPAgentProvider: ACPAgentProvider {
if error is OpenCodeACPLaunchResolutionError || error is ExecutableFileIdentityError {
return AIProviderError.invalidConfiguration(detail: error.localizedDescription)
}
if let guidance = Self.openCodeACPStartupGuidance(for: error) {
if let guidance = Self.openCodeACPStartupGuidance(
Comment on lines 133 to +136

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The recursiveMCPConfigGuidance() closure is eagerly evaluated for all errors in normalizeError, causing unnecessary filesystem I/O even when the result is discarded for non-startup errors.
Severity: MEDIUM

Suggested Fix

Modify openCodeACPStartupGuidance to accept recursiveMCPConfigGuidance as an escaping closure (@escaping @Sendable () -> String?). Call the closure inside the function, only after the guard check for the error type has passed. In normalizeError, pass the closure itself, not its result.

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/AI/Providers/OpenCode/OpenCodeACPAgentProvider.swift#L133-L136

Potential issue: The `normalizeError` function calls `recursiveMCPConfigGuidance()` as
an argument to `openCodeACPStartupGuidance`. This causes the closure to be evaluated
immediately, triggering synchronous filesystem I/O (checking for config files, reading
them, and parsing JSON) for every error processed. However, `openCodeACPStartupGuidance`
only uses the result for specific startup timeout errors, discarding it for all other
error types. This leads to unnecessary, potentially expensive I/O operations in a hot
path, such as during `session/prompt` timeouts, which degrades performance. This
contradicts the PR's intent to perform this check only for startup timeouts.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch — this was valid. Fixed in c99f229 by passing the diagnostic closure through lazily and invoking it only after the startup-timeout guard passes. I also strengthened the non-startup timeout test so session/prompt normalization asserts the recursive MCP config diagnostic is not called.

for: error,
recursiveMCPConfigGuidance: recursiveMCPConfigGuidance()
) {
return AIProviderError.invalidConfiguration(detail: guidance)
}
if (error as NSError).domain == NSCocoaErrorDomain {
Expand All @@ -137,15 +145,25 @@ struct OpenCodeACPAgentProvider: ACPAgentProvider {
return AIProviderError.apiError(source: error)
}

private static func openCodeACPStartupGuidance(for error: Error) -> String? {
private static func openCodeACPStartupGuidance(
for error: Error,
recursiveMCPConfigGuidance: String?
) -> String? {
guard let timeout = error as? ACPRequestTimeoutError,
["initialize", "authenticate", "session/new", "session/load"].contains(timeout.method)
else {
return nil
}

let message = timeout.localizedDescription.trimmingCharacters(in: .whitespacesAndNewlines)
return "OpenCode ACP did not finish session startup. Update OpenCode with `opencode upgrade`, then verify `opencode --version` and `opencode acp --help` return promptly before retrying. Original error: \(message)"
let recursiveGuidance = recursiveMCPConfigGuidance?
.trimmingCharacters(in: .whitespacesAndNewlines)
let recovery = if let recursiveGuidance, !recursiveGuidance.isEmpty {
" \(recursiveGuidance)"
} else {
" Update OpenCode with `opencode upgrade`, then verify `opencode --version` and `opencode acp --help` return promptly before retrying."
}
return "OpenCode ACP did not finish session startup.\(recovery) Original error: \(message)"
}

private func standardizedWorkingDirectory(from workspacePath: String?) -> String {
Expand All @@ -155,3 +173,95 @@ struct OpenCodeACPAgentProvider: ACPAgentProvider {
?? FileManager.default.temporaryDirectory.path
}
}

struct OpenCodeGlobalMCPConfigDiagnostic: Equatable {
private static let maximumConfigBytes: UInt64 = 2 * 1024 * 1024

let path: String
let suspiciousKeys: [String]

var guidanceMessage: String {
let keys = suspiciousKeys.map { "`\($0)`" }.joined(separator: ", ")
return "OpenCode may be loading RepoPrompt MCP server entries from its global config at `\(path)` (\(keys)). OpenCode loads global MCP servers during `opencode acp` startup; a RepoPrompt/RepoPrompt CE MCP entry can recursively launch RepoPrompt CE and block `session/new`. Back up the config, then remove or disable those OpenCode MCP entries (or temporarily set `\"mcp\": {}`) and retry."
}

static func detect(
environment: [String: String] = ProcessInfo.processInfo.environment,
fileManager: FileManager = .default
) -> OpenCodeGlobalMCPConfigDiagnostic? {
for path in candidateConfigPaths(environment: environment) {
guard isRegularConfigFileWithinLimit(path, fileManager: fileManager),
let data = fileManager.contents(atPath: path),
let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any],
let diagnostic = diagnostic(in: object, path: path)
else {
continue
}
return diagnostic
}
return nil
}

static func diagnostic(in config: [String: Any], path: String) -> OpenCodeGlobalMCPConfigDiagnostic? {
guard let mcp = config["mcp"] as? [String: Any], !mcp.isEmpty else { return nil }
let keys = mcp.compactMap { key, value -> String? in
if isRepoPromptReference(key) || containsRepoPromptReference(value) {
return key
}
return nil
}
guard !keys.isEmpty else { return nil }
return OpenCodeGlobalMCPConfigDiagnostic(path: path, suspiciousKeys: keys.sorted())
}

private static func isRegularConfigFileWithinLimit(_ path: String, fileManager: FileManager) -> Bool {
guard let attributes = try? fileManager.attributesOfItem(atPath: path),
attributes[.type] as? FileAttributeType == .typeRegular
else {
return false
}
let size = (attributes[.size] as? NSNumber)?.uint64Value
?? attributes[.size] as? UInt64
guard let size else { return false }
return size <= maximumConfigBytes
}

private static func candidateConfigPaths(environment: [String: String]) -> [String] {
var paths: [String] = []
if let xdgConfigHome = environment["XDG_CONFIG_HOME"]?.trimmingCharacters(in: .whitespacesAndNewlines),
!xdgConfigHome.isEmpty
{
paths.append(URL(fileURLWithPath: xdgConfigHome).appendingPathComponent("opencode/opencode.json").path)
}
let home = environment["HOME"]?.trimmingCharacters(in: .whitespacesAndNewlines)
?? FileManager.default.homeDirectoryForCurrentUser.path
if !home.isEmpty {
paths.append(URL(fileURLWithPath: home).appendingPathComponent(".config/opencode/opencode.json").path)
}
var seen = Set<String>()
return paths.filter { seen.insert($0).inserted }
}

private static func containsRepoPromptReference(_ value: Any) -> Bool {
if let string = value as? String {
return isRepoPromptReference(string)
}
if let array = value as? [Any] {
return array.contains(where: containsRepoPromptReference)
}
if let dictionary = value as? [String: Any] {
return dictionary.contains { key, value in
isRepoPromptReference(key) || containsRepoPromptReference(value)
}
}
return false
}

private static func isRepoPromptReference(_ string: String) -> Bool {
let normalized = string.lowercased()
return normalized.contains("repoprompt")
|| normalized.contains("repopromptce")
|| normalized.contains("repoprompt_ce_cli")
|| normalized.contains("repoprompt-mcp")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ struct OpenCodeACPControllerModelDiscoveryClient: OpenCodeACPModelDiscoveryClien
let snapshot = AgentACPModelRegistry.shared.currentSnapshot(for: .openCode)
await controller.shutdown()
return snapshot
} catch is CancellationError {
await controller.shutdown()
throw CancellationError()
} catch {
await controller.shutdown()
throw provider.normalizeError(error)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,10 +262,88 @@ final class OpenCodeACPLaunchResolverTests: XCTestCase {
XCTAssertEqual((source as? ACPRequestTimeoutError), timeout)
}

private func makeProviderForNormalization() -> OpenCodeACPAgentProvider {
func testSessionNewTimeoutIncludesRecursiveMCPConfigGuidanceWhenDetected() {
let provider = makeProviderForNormalization(
recursiveMCPConfigGuidance: {
"OpenCode may be loading RepoPrompt MCP server entries from its global config at `/tmp/opencode.json` (`RepoPromptCE`). Remove or disable those OpenCode MCP entries."
}
)
let timeout = ACPRequestTimeoutError(
method: "session/new",
timeoutSeconds: 30,
launchDescription: "opencode acp",
diagnosticHint: "The process did not write ACP stdout before timing out, but it wrote stderr. Last stderr line: `server unavailable key=RepoPromptCE`.",
agentIdentity: "OpenCode 1.17.14"
)

let normalized = provider.normalizeError(timeout)

guard case let AIProviderError.invalidConfiguration(detail) = normalized else {
return XCTFail("Unexpected normalized error: \(normalized)")
}
XCTAssertTrue(detail.contains("global config"))
XCTAssertTrue(detail.contains("RepoPromptCE"))
XCTAssertTrue(detail.contains("Remove or disable"))
XCTAssertTrue(detail.contains("server unavailable key=RepoPromptCE"))
XCTAssertFalse(detail.contains("opencode upgrade"))
}

func testOpenCodeGlobalMCPConfigDiagnosticDetectsRepoPromptServerInGlobalConfig() throws {
let directory = try makeTemporaryDirectory()
let xdgConfigHome = directory.appendingPathComponent("xdg", isDirectory: true)
let openCodeConfigDirectory = xdgConfigHome.appendingPathComponent("opencode", isDirectory: true)
try FileManager.default.createDirectory(at: openCodeConfigDirectory, withIntermediateDirectories: true)
let configPath = openCodeConfigDirectory.appendingPathComponent("opencode.json")
try """
{
"$schema": "https://opencode.ai/config.json",
"mcp": {
"RepoPromptCE": {
"type": "local",
"command": ["/Applications/RepoPrompt.app/Contents/MacOS/repoprompt-mcp"]
},
"unrelated": {
"type": "local",
"command": ["node", "server.js"]
}
}
}
""".write(to: configPath, atomically: true, encoding: .utf8)

let diagnostic = try XCTUnwrap(OpenCodeGlobalMCPConfigDiagnostic.detect(environment: [
"XDG_CONFIG_HOME": xdgConfigHome.path,
"HOME": directory.appendingPathComponent("home", isDirectory: true).path
]))

XCTAssertEqual(diagnostic.path, configPath.path)
XCTAssertEqual(diagnostic.suspiciousKeys, ["RepoPromptCE"])
XCTAssertTrue(diagnostic.guidanceMessage.contains("`RepoPromptCE`"))
XCTAssertTrue(diagnostic.guidanceMessage.contains("recursively launch RepoPrompt CE"))
XCTAssertTrue(diagnostic.guidanceMessage.contains("\"mcp\": {}"))
}

func testOpenCodeGlobalMCPConfigDiagnosticIgnoresEmptyAndUnrelatedMCPConfig() {
XCTAssertNil(OpenCodeGlobalMCPConfigDiagnostic.diagnostic(in: ["mcp": [:]], path: "/tmp/opencode.json"))
XCTAssertNil(OpenCodeGlobalMCPConfigDiagnostic.diagnostic(
in: [
"mcp": [
"filesystem": [
"type": "local",
"command": ["node", "server.js"]
]
]
],
path: "/tmp/opencode.json"
))
}

private func makeProviderForNormalization(
recursiveMCPConfigGuidance: @escaping @Sendable () -> String? = { nil }
) -> OpenCodeACPAgentProvider {
OpenCodeACPAgentProvider(
config: OpenCodeAgentConfig(commandName: "opencode"),
launchResolver: OpenCodeACPLaunchResolver()
launchResolver: OpenCodeACPLaunchResolver(),
recursiveMCPConfigGuidance: recursiveMCPConfigGuidance
)
}

Expand Down
Loading