From 43ab0591152983cab15ef8e0e6933b79752b2198 Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Fri, 3 Apr 2026 00:54:59 +0300 Subject: [PATCH 1/4] impl: remove support for data dir fallback setting This PR is a result of discussion that happened in the https://github.com/coder/coder-jetbrains-toolbox/pull/286. Basically it as an attempt of simplifying the CLI resolution and trying to be more aligned with the VS Code extension. The existing implementation was too cumbersome to understand, brittle and a lot of tedious work needed to solve all of it's usecases. This PR removes the enable fallback to data dir setting, which was used only for access denied exceptions. The CLI resolution uses the binary destination if it was configured by the user, or it automatically falls back to data dir if binary destination was not configured. The implementation respects the user's choice and no longer tries to make smart choices on behalf of the user. For example if the user configured the binary destination but disabled downloads we just prompt him with an error instead of trying to fall on data dir. There were a couple of other small improvements left from the previous PR that are now addressed. - resolves #285 --- CHANGELOG.md | 11 +- .../com/coder/toolbox/cli/CoderCLIManager.kt | 65 +-- .../cli/downloader/CoderDownloadService.kt | 6 +- .../toolbox/settings/ReadOnlyCoderSettings.kt | 10 +- .../coder/toolbox/store/CoderSettingsStore.kt | 35 +- .../com/coder/toolbox/store/StoreKeys.kt | 2 - .../coder/toolbox/views/CoderSettingsPage.kt | 10 - .../com/coder/toolbox/views/ErrorReporter.kt | 18 +- .../resources/localization/defaultMessages.po | 3 - .../coder/toolbox/cli/CoderCLIManagerTest.kt | 14 +- .../com/coder/toolbox/cli/EnsureCLITest.kt | 548 +----------------- .../toolbox/settings/CoderSettingsTest.kt | 9 - .../toolbox/store/CoderSettingsStoreTest.kt | 14 - 13 files changed, 68 insertions(+), 677 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82912ce8..9d700b16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,13 @@ ### Added -- support for using a locally provided CLI binary when downloads are disabled +- `Binary destination` can now point directly to an executable, used as-is; otherwise it is treated as a base directory + as before + +### Removed + +- support for enabling or disabling the fallback to data dir. CLI resolution will always fall back on data dir if binary + destination is not configured. ## 0.8.6 - 2026-03-05 @@ -17,7 +23,8 @@ ### Added - support for configuring the SSH connection timeout, defaults to 10 seconds -- enhanced IDE resolution by supporting latest EAP, latest release, latest installed labels with clear fallback behavior in URI handlers +- enhanced IDE resolution by supporting latest EAP, latest release, latest installed labels with clear fallback behavior + in URI handlers ### Fixed diff --git a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt index 4bbe68af..19dd2676 100644 --- a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt +++ b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt @@ -48,24 +48,17 @@ internal data class Version( /** - * Do as much as possible to get a valid, up-to-date CLI. + * Best effort to get an up-to-date CLI. * * 1. Create a CLI manager for the deployment URL. * 2. If the CLI version matches the build version, return it immediately. - * 3. If downloads are enabled, attempt to download the CLI. + * 3. Otherwise, if downloads are enabled, attempt to download the CLI. * a. On success, return the CLI. - * b. On [java.nio.file.AccessDeniedException]: rethrow if the binary - * path parent equals the data directory or if binary directory - * fallback is disabled. Otherwise, if the fallback data directory - * CLI already matches the build version return it; if not, download - * to the data directory and return the fallback CLI. - * c. Any other exception propagates to the caller. + * b. If there is an exception propagates to the user. * 4. If downloads are disabled: - * a. If the data directory CLI version matches, return it. - * b. If neither the configured binary nor the data directory CLI can + * a. If neither the configured binary nor the data directory CLI can * report a version, throw [IllegalStateException]. - * c. Prefer the configured binary; fall back to the data directory CLI - * only when the configured binary is missing or unexecutable. + * b. Otherwise, warn the user and return the mismatched version. */ suspend fun ensureCLI( context: CoderToolboxContext, @@ -95,44 +88,15 @@ suspend fun ensureCLI( // If downloads are enabled download the new version. if (settings.enableDownloads) { reportProgress("Downloading Coder CLI...") - try { - cli.download(buildVersion, showTextProgress) - return cli - } catch (e: java.nio.file.AccessDeniedException) { - // Might be able to fall back to the data directory. - val binPath = settings.binPath(deploymentURL) - val dataDir = settings.dataDir(deploymentURL) - if (binPath.parent == dataDir || !settings.enableBinaryDirectoryFallback) { - throw e - } - // fall back to the data directory. - val fallbackCLI = CoderCLIManager(context, deploymentURL, true) - val fallbackMatches = fallbackCLI.matchesVersion(buildVersion) - if (fallbackMatches == true) { - reportProgress("Local CLI version from data directory matches server version: $buildVersion") - return fallbackCLI - } - - reportProgress("Downloading Coder CLI to the data directory...") - fallbackCLI.download(buildVersion, showTextProgress) - return fallbackCLI - } - } - - // Try falling back to the data directory. - val dataCLI = CoderCLIManager(context, deploymentURL, true) - val dataCLIMatches = dataCLI.matchesVersion(buildVersion) - if (dataCLIMatches == true) { - reportProgress("Local CLI version from data directory matches server version: $buildVersion") - return dataCLI + cli.download(buildVersion, showTextProgress) + return cli } - // Prefer the binary directory unless the data directory has a - // working binary and the binary directory does not. - if (cliMatches == null && dataCLIMatches == null && !settings.enableDownloads) { + if (cliMatches == null) { throw IllegalStateException("Can't resolve Coder CLI and downloads are disabled") } - return if (cliMatches == null && dataCLIMatches != null) dataCLI else cli + reportProgress("Downloads are disabled, and a cached CLI is used which does not match the server version $buildVersion and could cause compatibility issues") + return cli } /** @@ -151,16 +115,13 @@ data class Features( class CoderCLIManager( private val context: CoderToolboxContext, // The URL of the deployment this CLI is for. - private val deploymentURL: URL, - // If the binary directory is not writable, this can be used to force the - // manager to download to the data directory instead. - private val forceDownloadToData: Boolean = false, + private val deploymentURL: URL ) { private val downloader = createDownloadService() private val gpgVerifier = GPGVerifier(context) val remoteBinaryURL: URL = context.settingsStore.binSource(deploymentURL) - val localBinaryPath: Path = context.settingsStore.binPath(deploymentURL, forceDownloadToData) + val localBinaryPath: Path = context.settingsStore.binPath(deploymentURL) val coderConfigPath: Path = context.settingsStore.dataDir(deploymentURL).resolve("config") private fun createDownloadService(): CoderDownloadService { @@ -180,7 +141,7 @@ class CoderCLIManager( .build() val service = retrofit.create(CoderDownloadApi::class.java) - return CoderDownloadService(context, service, deploymentURL, forceDownloadToData) + return CoderDownloadService(context, service, deploymentURL) } /** diff --git a/src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt b/src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt index 2c2e87c6..da5e2f21 100644 --- a/src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt +++ b/src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt @@ -38,17 +38,17 @@ private val SUPPORTED_BIN_MIME_TYPES = listOf( "application/x-ms-dos-executable", "application/vnd.microsoft.portable-executable" ) + /** * Handles the download steps of Coder CLI */ class CoderDownloadService( private val context: CoderToolboxContext, private val downloadApi: CoderDownloadApi, - private val deploymentUrl: URL, - forceDownloadToData: Boolean, + private val deploymentUrl: URL ) { private val remoteBinaryURL: URL = context.settingsStore.binSource(deploymentUrl) - private val cliFinalDst: Path = context.settingsStore.binPath(deploymentUrl, forceDownloadToData) + private val cliFinalDst: Path = context.settingsStore.binPath(deploymentUrl) private val cliTempDst: Path = cliFinalDst.resolveSibling("${cliFinalDst.name}.tmp") suspend fun downloadCli(buildVersion: String, showTextProgress: (String) -> Unit): DownloadResult { diff --git a/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt b/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt index a925ba1e..750a3074 100644 --- a/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt +++ b/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt @@ -90,12 +90,6 @@ interface ReadOnlyCoderSettings { */ val enableDownloads: Boolean - /** - * Whether to allow the plugin to fall back to the data directory when the - * CLI directory is not writable. - */ - val enableBinaryDirectoryFallback: Boolean - /** * An external command that outputs additional HTTP headers added to all * requests. The command must output each header as `key=value` on its own @@ -179,9 +173,9 @@ interface ReadOnlyCoderSettings { fun binSource(url: URL): URL /** - * To where the specified deployment should download the binary. + * Returns a path to where the CLI is downloaded. */ - fun binPath(url: URL, forceDownloadToData: Boolean = false): Path + fun binPath(url: URL): Path /** * Return the URL and token from the config, if they exist. diff --git a/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt b/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt index 1dcc8959..b6d789bb 100644 --- a/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt +++ b/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt @@ -54,8 +54,6 @@ class CoderSettingsStore( override val globalDataDirectory: String get() = getDefaultGlobalDataDir().normalize().toString() override val globalConfigDir: String get() = getDefaultGlobalConfigDir().normalize().toString() override val enableDownloads: Boolean get() = store[ENABLE_DOWNLOADS]?.toBooleanStrictOrNull() ?: true - override val enableBinaryDirectoryFallback: Boolean - get() = store[ENABLE_BINARY_DIR_FALLBACK]?.toBooleanStrictOrNull() ?: false override val headerCommand: String? get() = store[HEADER_COMMAND] override val tls: ReadOnlyTLSSettings get() = TLSSettings( @@ -127,30 +125,19 @@ class CoderSettingsStore( * * Resolution logic: * 1. If [binaryDestination] is null/blank, return the deployment's data - * directory with the default CLI binary name. [forceDownloadToData] - * is ignored because both paths resolve to the same location. - * 2. If [forceDownloadToData] is true, return a host-specific subdirectory - * under the deployment's data directory with the default CLI binary name. - * 3. If the expanded (~ and $HOME) [binaryDestination] is an existing executable file, + * directory with the default CLI binary name. + * 2. If the expanded (~ and $HOME) [binaryDestination] is an existing executable file, * return it as-is. - * 4. Otherwise, treat [binaryDestination] as a base directory and return a + * 2. Otherwise, treat [binaryDestination] as a base directory and return a * host-specific subdirectory with the default CLI binary name. */ - override fun binPath( - url: URL, - forceDownloadToData: Boolean, - ): Path { - if (binaryDestination.isNullOrBlank()) { - return dataDir(url).resolve(defaultCliBinaryNameByOsAndArch).toAbsolutePath() - } - - val dest = Path.of(expand(binaryDestination!!)) - val isExecutable = Files.isRegularFile(dest) && Files.isExecutable(dest) + override fun binPath(url: URL): Path { + val raw = binaryDestination?.takeIf { it.isNotBlank() } ?: return dataDir(url).resolve( + defaultCliBinaryNameByOsAndArch + ).toAbsolutePath() - if (forceDownloadToData) { - return dataDir(url).resolve(defaultCliBinaryNameByOsAndArch).toAbsolutePath() - } - if (isExecutable) { + val dest = Path.of(expand(raw)) + if (Files.isRegularFile(dest) && Files.isExecutable(dest)) { return dest.toAbsolutePath() } return withHost(dest, url).resolve(defaultCliBinaryNameByOsAndArch).toAbsolutePath() @@ -222,10 +209,6 @@ class CoderSettingsStore( store[HTTP_CLIENT_LOG_LEVEL] = level.toString() } - fun updateBinaryDirectoryFallback(shouldEnableBinDirFallback: Boolean) { - store[ENABLE_BINARY_DIR_FALLBACK] = shouldEnableBinDirFallback.toString() - } - fun updateHeaderCommand(cmd: String) { store[HEADER_COMMAND] = cmd } diff --git a/src/main/kotlin/com/coder/toolbox/store/StoreKeys.kt b/src/main/kotlin/com/coder/toolbox/store/StoreKeys.kt index 3325faef..f73ab9eb 100644 --- a/src/main/kotlin/com/coder/toolbox/store/StoreKeys.kt +++ b/src/main/kotlin/com/coder/toolbox/store/StoreKeys.kt @@ -25,8 +25,6 @@ internal const val DATA_DIRECTORY = "dataDirectory" internal const val ENABLE_DOWNLOADS = "enableDownloads" -internal const val ENABLE_BINARY_DIR_FALLBACK = "enableBinaryDirectoryFallback" - internal const val HEADER_COMMAND = "headerCommand" internal const val TLS_CERT_PATH = "tlsCertPath" diff --git a/src/main/kotlin/com/coder/toolbox/views/CoderSettingsPage.kt b/src/main/kotlin/com/coder/toolbox/views/CoderSettingsPage.kt index 28a40f39..0a7cd393 100644 --- a/src/main/kotlin/com/coder/toolbox/views/CoderSettingsPage.kt +++ b/src/main/kotlin/com/coder/toolbox/views/CoderSettingsPage.kt @@ -68,10 +68,6 @@ class CoderSettingsPage( ) ) - private val enableBinaryDirectoryFallbackField = CheckboxField( - settings.enableBinaryDirectoryFallback, - context.i18n.ptrl("Enable binary directory fallback") - ) private val headerCommandField = TextField( context.i18n.ptrl("Header command"), settings.headerCommand ?: "", @@ -132,7 +128,6 @@ class CoderSettingsPage( enableDownloadsField, useAppNameField, binaryDestinationField, - enableBinaryDirectoryFallbackField, disableSignatureVerificationField, signatureFallbackStrategyField, httpLoggingField, @@ -163,7 +158,6 @@ class CoderSettingsPage( updateDisableSignatureVerification(disableSignatureVerificationField.checkedState.value) updateSignatureFallbackStrategy(signatureFallbackStrategyField.checkedState.value) updateHttpClientLogLevel(httpLoggingField.selectedValueState.value) - updateBinaryDirectoryFallback(enableBinaryDirectoryFallbackField.checkedState.value) updateHeaderCommand(headerCommandField.contentState.value) updateCertPath(tlsCertPathField.contentState.value) updateKeyPath(tlsKeyPathField.contentState.value) @@ -216,10 +210,6 @@ class CoderSettingsPage( settings.fallbackOnCoderForSignatures.isAllowed() } - enableBinaryDirectoryFallbackField.checkedState.update { - settings.enableBinaryDirectoryFallback - } - headerCommandField.contentState.update { settings.headerCommand ?: "" } diff --git a/src/main/kotlin/com/coder/toolbox/views/ErrorReporter.kt b/src/main/kotlin/com/coder/toolbox/views/ErrorReporter.kt index 88ace652..39203752 100644 --- a/src/main/kotlin/com/coder/toolbox/views/ErrorReporter.kt +++ b/src/main/kotlin/com/coder/toolbox/views/ErrorReporter.kt @@ -46,11 +46,8 @@ private class ErrorReporterImpl( } private fun showError(ex: Throwable) { - val textError = if (ex is APIResponseException) { - if (!ex.reason.isNullOrBlank()) { - ex.reason - } else ex.message - } else ex.message ?: ex.toString() + val textError = ex.prettify() + context.cs.launch { context.ui.showSnackbar( UUID.randomUUID().toString(), @@ -61,6 +58,17 @@ private class ErrorReporterImpl( } } + private fun Throwable.prettify(): String? = when (this) { + is APIResponseException -> if (!this.reason.isNullOrBlank()) { + this.reason + } else this.message + + is AccessDeniedException, + is java.nio.file.AccessDeniedException -> "Access denied to ${this.message}" + + is java.nio.file.FileSystemException -> "A file system operation failed when trying to access ${this.message}" + else -> this.toString() + } override fun flush() { if (errorBuffer.isNotEmpty() && visibilityState.value.applicationVisible) { diff --git a/src/main/resources/localization/defaultMessages.po b/src/main/resources/localization/defaultMessages.po index ad6a692f..6be1db2f 100644 --- a/src/main/resources/localization/defaultMessages.po +++ b/src/main/resources/localization/defaultMessages.po @@ -82,9 +82,6 @@ msgstr "" msgid "Verify binary signature using releases.coder.com when CLI signatures are not available from the deployment" msgstr "" -msgid "Enable binary directory fallback" -msgstr "" - msgid "Header command" msgstr "" diff --git a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt index d0caf714..bcd321f8 100644 --- a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt +++ b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt @@ -190,16 +190,10 @@ internal class CoderCLIManagerTest { ) val url = URL("http://localhost") - val ccm1 = CoderCLIManager(context.copy(settingsStore = settings), url) - assertEquals(settings.binSource(url), ccm1.remoteBinaryURL) - assertEquals(settings.dataDir(url), ccm1.coderConfigPath.parent) - assertEquals(settings.binPath(url), ccm1.localBinaryPath) - - // Can force using data directory. - val ccm2 = CoderCLIManager(context.copy(settingsStore = settings), url, true) - assertEquals(settings.binSource(url), ccm2.remoteBinaryURL) - assertEquals(settings.dataDir(url), ccm2.coderConfigPath.parent) - assertEquals(settings.binPath(url, true), ccm2.localBinaryPath) + val ccm = CoderCLIManager(context.copy(settingsStore = settings), url) + assertEquals(settings.binSource(url), ccm.remoteBinaryURL) + assertEquals(settings.dataDir(url), ccm.coderConfigPath.parent) + assertEquals(settings.binPath(url), ccm.localBinaryPath) } @Test diff --git a/src/test/kotlin/com/coder/toolbox/cli/EnsureCLITest.kt b/src/test/kotlin/com/coder/toolbox/cli/EnsureCLITest.kt index a35ab4df..2876fbe0 100644 --- a/src/test/kotlin/com/coder/toolbox/cli/EnsureCLITest.kt +++ b/src/test/kotlin/com/coder/toolbox/cli/EnsureCLITest.kt @@ -8,7 +8,6 @@ import com.coder.toolbox.store.CoderSecretsStore import com.coder.toolbox.store.CoderSettingsStore import com.coder.toolbox.store.DATA_DIRECTORY import com.coder.toolbox.store.DISABLE_SIGNATURE_VALIDATION -import com.coder.toolbox.store.ENABLE_BINARY_DIR_FALLBACK import com.coder.toolbox.store.ENABLE_DOWNLOADS import com.coder.toolbox.util.ConnectionMonitoringService import com.coder.toolbox.util.IgnoreOnWindows @@ -38,7 +37,6 @@ import java.net.InetSocketAddress import java.net.Proxy import java.net.ProxySelector import java.net.URL -import java.nio.file.AccessDeniedException import java.nio.file.Files import java.nio.file.Path import kotlin.test.BeforeTest @@ -89,7 +87,6 @@ internal class EnsureCLITest { coEvery { ui.showYesNoPopup(any(), any(), any(), any()) } returns true } - @Test @IgnoreOnWindows fun `given binaryDestination is a directory and downloads are enabled, returns the CLI when version already matches`() { @@ -99,8 +96,6 @@ internal class EnsureCLITest { DATA_DIRECTORY to testDir("dir-dest-dl-on-match/data").toString(), DISABLE_SIGNATURE_VALIDATION to "true", ) - // Compute expected path before creating files so isExecutable on the - // directory does not interfere with the resolution. val expected = s.binPath(url) createBinary(expected, "1.0.0") @@ -132,7 +127,7 @@ internal class EnsureCLITest { } @Test - fun `given binaryDestination is a directory and downloads are enabled, propagates non-AccessDenied errors during download to the caller`() { + fun `given binaryDestination is a directory and downloads are enabled, propagates errors during download to the caller`() { val (srv, url) = mockServer(errorCode = HttpURLConnection.HTTP_INTERNAL_ERROR) try { val s = settings( @@ -148,129 +143,6 @@ internal class EnsureCLITest { } } - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are enabled, falls back to the data directory CLI when access is denied and data directory version matches`() { - val (srv, url) = mockServer(version = "1.9.0") - val s = settings( - BINARY_DESTINATION to testDir("dir-dest-dl-on-fallback-match/bin").toString(), - DATA_DIRECTORY to testDir("dir-dest-dl-on-fallback-match/data").toString(), - ENABLE_BINARY_DIR_FALLBACK to "true", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val binParent = s.binPath(url).parent - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "2.0.0") - try { - binParent.toFile().mkdirs() - binParent.toFile().setWritable(false) - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(2, 0, 0), ccm.version()) - } finally { - binParent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are enabled, downloads CLI to the data directory when access is denied and data directory CLI is missing`() { - val (srv, url) = mockServer(version = "1.0.0") - val s = settings( - BINARY_DESTINATION to testDir("dir-dest-dl-on-fallback-missing/bin").toString(), - DATA_DIRECTORY to testDir("dir-dest-dl-on-fallback-missing/data").toString(), - ENABLE_BINARY_DIR_FALLBACK to "true", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val binParent = s.binPath(url).parent - val fallbackPath = s.binPath(url, true) - try { - binParent.toFile().mkdirs() - binParent.toFile().setWritable(false) - val ccm = runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertTrue(ccm.localBinaryPath.toFile().exists()) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } finally { - binParent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are enabled, rethrows AccessDeniedException when fallback is enabled but binary path is already inside the data directory`() { - val (srv, url) = mockServer() - // Set BINARY_DESTINATION to the same base as DATA_DIRECTORY so that - // binPath(url).parent == dataDir(url). - val sharedBase = testDir("dir-dest-dl-on-fallback-same-dir/shared") - val s = settings( - BINARY_DESTINATION to sharedBase.toString(), - DATA_DIRECTORY to sharedBase.toString(), - ENABLE_BINARY_DIR_FALLBACK to "true", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val binParent = s.binPath(url).parent - try { - binParent.toFile().mkdirs() - binParent.toFile().setWritable(false) - assertFailsWith(AccessDeniedException::class) { - runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - } - } finally { - binParent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are enabled, rethrows AccessDeniedException when binary directory fallback is disabled`() { - val (srv, url) = mockServer() - val s = settings( - BINARY_DESTINATION to testDir("dir-dest-dl-on-no-fallback/bin").toString(), - DATA_DIRECTORY to testDir("dir-dest-dl-on-no-fallback/data").toString(), - ENABLE_BINARY_DIR_FALLBACK to "false", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val binParent = s.binPath(url).parent - try { - binParent.toFile().mkdirs() - binParent.toFile().setWritable(false) - assertFailsWith(AccessDeniedException::class) { - runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - } - } finally { - binParent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are enabled, rethrows AccessDeniedException when fallback is disabled and binary path is inside the data directory`() { - val (srv, url) = mockServer() - val sharedBase = testDir("dir-dest-dl-on-no-fallback-same-dir/shared") - val s = settings( - BINARY_DESTINATION to sharedBase.toString(), - DATA_DIRECTORY to sharedBase.toString(), - ENABLE_BINARY_DIR_FALLBACK to "false", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val binParent = s.binPath(url).parent - try { - binParent.toFile().mkdirs() - binParent.toFile().setWritable(false) - assertFailsWith(AccessDeniedException::class) { - runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - } - } finally { - binParent.toFile().setWritable(true) - srv.stop(0) - } - } - @Test @IgnoreOnWindows fun `given binaryDestination is an existing executable and downloads are enabled, returns the CLI when version already matches`() { @@ -314,7 +186,7 @@ internal class EnsureCLITest { @Test @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are enabled, propagates non-AccessDenied errors during download to the caller`() { + fun `given binaryDestination is an existing executable and downloads are enabled, propagates errors during download to the caller`() { val (srv, url) = mockServer(errorCode = HttpURLConnection.HTTP_INTERNAL_ERROR) try { val binaryFile = testDir("exec-dest-dl-on-error/bin/my-coder") @@ -334,134 +206,7 @@ internal class EnsureCLITest { @Test @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are enabled, falls back to the data directory CLI when access is denied and data directory version matches`() { - val (srv, url) = mockServer(version = "1.9.9") - val binaryFile = testDir("exec-dest-dl-on-fallback-match/bin/my-coder") - createBinary(binaryFile, "1.0.0") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to testDir("exec-dest-dl-on-fallback-match/data").toString(), - ENABLE_BINARY_DIR_FALLBACK to "true", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "2.0.0") - try { - binaryFile.parent.toFile().setWritable(false) - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(2, 0, 0), ccm.version()) - } finally { - binaryFile.parent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are enabled, downloads CLI to the data directory when access is denied and data directory CLI is missing`() { - val (srv, url) = mockServer(version = "2.0.0") - val binaryFile = testDir("exec-dest-dl-on-fallback-missing/bin/my-coder") - createBinary(binaryFile, "1.0.0") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to testDir("exec-dest-dl-on-fallback-missing/data").toString(), - ENABLE_BINARY_DIR_FALLBACK to "true", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val fallbackPath = s.binPath(url, true) - try { - binaryFile.parent.toFile().setWritable(false) - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertTrue(ccm.localBinaryPath.toFile().exists()) - assertEquals(SemVer(2, 0, 0), ccm.version()) - } finally { - binaryFile.parent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are enabled, rethrows AccessDeniedException when fallback is enabled but binary path is already inside the data directory`() { - val (srv, url) = mockServer() - // Place the executable inside the data dir so binPath.parent == dataDir. - val dataBase = testDir("exec-dest-dl-on-fallback-same-dir/data") - val dataDirForUrl = dataBase.resolve(hostDir(url)) - val binaryFile = dataDirForUrl.resolve("my-coder") - createBinary(binaryFile, "1.0.0") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to dataBase.toString(), - ENABLE_BINARY_DIR_FALLBACK to "true", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - // Verify the precondition: binPath.parent == dataDir - assertEquals(s.dataDir(url), s.binPath(url).parent) - try { - binaryFile.parent.toFile().setWritable(false) - assertFailsWith(AccessDeniedException::class) { - runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - } - } finally { - binaryFile.parent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are enabled, rethrows AccessDeniedException when binary directory fallback is disabled`() { - val (srv, url) = mockServer() - val binaryFile = testDir("exec-dest-dl-on-no-fallback/bin/my-coder") - createBinary(binaryFile, "1.0.0") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to testDir("exec-dest-dl-on-no-fallback/data").toString(), - ENABLE_BINARY_DIR_FALLBACK to "false", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - try { - binaryFile.parent.toFile().setWritable(false) - assertFailsWith(AccessDeniedException::class) { - runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - } - } finally { - binaryFile.parent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are enabled, rethrows AccessDeniedException when fallback is disabled and binary path is inside the data directory`() { - val (srv, url) = mockServer() - val dataBase = testDir("exec-dest-dl-on-no-fallback-same-dir/data") - val dataDirForUrl = dataBase.resolve(hostDir(url)) - val binaryFile = dataDirForUrl.resolve("my-coder") - createBinary(binaryFile, "1.0.0") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to dataBase.toString(), - ENABLE_BINARY_DIR_FALLBACK to "false", - DISABLE_SIGNATURE_VALIDATION to "true", - ) - assertEquals(s.dataDir(url), s.binPath(url).parent) - try { - binaryFile.parent.toFile().setWritable(false) - assertFailsWith(AccessDeniedException::class) { - runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - } - } finally { - binaryFile.parent.toFile().setWritable(true) - srv.stop(0) - } - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are disabled, returns the CLI at destination when version matches`() { + fun `given binaryDestination is an existing executable and downloads are disabled, returns the CLI when version matches`() { val url = URL("http://test.coder.invalid") val binaryFile = testDir("exec-dest-dl-off-match/bin/my-coder") createBinary(binaryFile, "1.0.0") @@ -477,96 +222,22 @@ internal class EnsureCLITest { @Test @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are disabled, returns the data directory CLI when destination version mismatches but data directory version matches`() { + fun `given binaryDestination is an existing executable and downloads are disabled, returns the stale CLI when version mismatches`() { val url = URL("http://test.coder.invalid") - val binaryFile = testDir("exec-dest-dl-off-mismatch-datacli-match/bin/my-coder") + val binaryFile = testDir("exec-dest-dl-off-mismatch/bin/my-coder") createBinary(binaryFile, "1.0.0") val s = settings( BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to testDir("exec-dest-dl-off-mismatch-datacli-match/data").toString(), + DATA_DIRECTORY to testDir("exec-dest-dl-off-mismatch/data").toString(), ENABLE_DOWNLOADS to "false", ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "2.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(2, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are disabled, returns the stale destination CLI when both destination and data directory versions mismatch`() { - val url = URL("http://test.coder.invalid") - val binaryFile = testDir("exec-dest-dl-off-mismatch-datacli-wrong/bin/my-coder") - createBinary(binaryFile, "1.0.0") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to testDir("exec-dest-dl-off-mismatch-datacli-wrong/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "1.5.0") - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } assertEquals(binaryFile.toAbsolutePath(), ccm.localBinaryPath) assertEquals(SemVer(1, 0, 0), ccm.version()) } @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are disabled, returns the stale destination CLI when version mismatches and data directory CLI is missing`() { - val url = URL("http://test.coder.invalid") - val binaryFile = testDir("exec-dest-dl-off-mismatch-datacli-missing/bin/my-coder") - createBinary(binaryFile, "1.0.0") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to testDir("exec-dest-dl-off-mismatch-datacli-missing/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(binaryFile.toAbsolutePath(), ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are disabled, returns the data directory CLI when destination is missing and data directory version matches`() { - val url = URL("http://test.coder.invalid") - val binaryFile = testDir("exec-dest-dl-off-no-cli-datacli-match/bin/my-coder") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to testDir("exec-dest-dl-off-no-cli-datacli-match/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "1.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is an existing executable and downloads are disabled, returns the stale data directory CLI when destination is missing and data directory version mismatches`() { - val url = URL("http://test.coder.invalid") - val binaryFile = testDir("exec-dest-dl-off-no-cli-datacli-wrong/bin/my-coder") - val s = settings( - BINARY_DESTINATION to binaryFile.toString(), - DATA_DIRECTORY to testDir("exec-dest-dl-off-no-cli-datacli-wrong/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "1.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - - @Test - fun `given binaryDestination is an existing executable and downloads are disabled, throws when both destination and data directory CLIs are missing`() { + fun `given binaryDestination points to a non-existent file and downloads are disabled, throws when no CLI binary exists`() { val url = URL("http://test.coder.invalid") val binaryFile = testDir("exec-dest-dl-off-both-missing/bin/my-coder") val s = settings( @@ -606,7 +277,7 @@ internal class EnsureCLITest { @Test @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are disabled, returns the CLI at destination when version matches`() { + fun `given binaryDestination is a directory and downloads are disabled, returns the CLI when version matches`() { val url = URL("http://test.coder.invalid") val s = settings( BINARY_DESTINATION to testDir("dir-dest-dl-off-match/bin").toString(), @@ -623,36 +294,15 @@ internal class EnsureCLITest { @Test @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are disabled, returns the data directory CLI when destination version mismatches but data directory version matches`() { + fun `given binaryDestination is a directory and downloads are disabled, returns the stale CLI when version mismatches`() { val url = URL("http://test.coder.invalid") val s = settings( - BINARY_DESTINATION to testDir("dir-dest-dl-off-mismatch-datacli-match/bin").toString(), - DATA_DIRECTORY to testDir("dir-dest-dl-off-mismatch-datacli-match/data").toString(), + BINARY_DESTINATION to testDir("dir-dest-dl-off-mismatch/bin").toString(), + DATA_DIRECTORY to testDir("dir-dest-dl-off-mismatch/data").toString(), ENABLE_DOWNLOADS to "false", ) val expected = s.binPath(url) createBinary(expected, "1.0.0") - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "2.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(2, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are disabled, returns the stale destination CLI when both destination and data directory versions mismatch`() { - val url = URL("http://test.coder.invalid") - val s = settings( - BINARY_DESTINATION to testDir("dir-dest-dl-off-mismatch-datacli-wrong/bin").toString(), - DATA_DIRECTORY to testDir("dir-dest-dl-off-mismatch-datacli-wrong/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val expected = s.binPath(url) - createBinary(expected, "1.0.0") - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "1.5.0") val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } assertEquals(expected, ccm.localBinaryPath) @@ -661,58 +311,7 @@ internal class EnsureCLITest { @Test @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are disabled, returns the stale destination CLI when version mismatches and data directory CLI is missing`() { - val url = URL("http://test.coder.invalid") - val s = settings( - BINARY_DESTINATION to testDir("dir-dest-dl-off-mismatch-datacli-missing/bin").toString(), - DATA_DIRECTORY to testDir("dir-dest-dl-off-mismatch-datacli-missing/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val expected = s.binPath(url) - createBinary(expected, "1.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(expected, ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are disabled, returns the data directory CLI when destination CLI is missing and data directory version matches`() { - val url = URL("http://test.coder.invalid") - val s = settings( - BINARY_DESTINATION to testDir("dir-dest-dl-off-no-cli-datacli-match/bin").toString(), - DATA_DIRECTORY to testDir("dir-dest-dl-off-no-cli-datacli-match/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "1.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are disabled, returns the stale data directory CLI when destination CLI is missing and data directory version mismatches`() { - val url = URL("http://test.coder.invalid") - val s = settings( - BINARY_DESTINATION to testDir("dir-dest-dl-off-no-cli-datacli-wrong/bin").toString(), - DATA_DIRECTORY to testDir("dir-dest-dl-off-no-cli-datacli-wrong/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "1.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given binaryDestination is a directory and downloads are disabled, throws when both destination and data directory CLIs are missing`() { + fun `given binaryDestination is a directory and downloads are disabled, throws when no CLI binary exists`() { val url = URL("http://test.coder.invalid") val s = settings( BINARY_DESTINATION to testDir("dir-dest-dl-off-both-missing/bin").toString(), @@ -742,52 +341,13 @@ internal class EnsureCLITest { @Test @IgnoreOnWindows - fun `given no binaryDestination configured and downloads are disabled, returns the CLI when the shared path is overwritten with a matching version`() { - val url = URL("http://test.coder.invalid") - val s = settings( - DATA_DIRECTORY to testDir("no-dest-dl-off-mismatch-datacli-match/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val expected = s.binPath(url) - createBinary(expected, "1.0.0") - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "2.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(2, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given no binaryDestination configured and downloads are disabled, returns the stale CLI when version mismatches confirming CLI and data directory paths are identical`() { + fun `given no binaryDestination configured and downloads are disabled, returns the stale CLI when version mismatches`() { val url = URL("http://test.coder.invalid") val s = settings( DATA_DIRECTORY to testDir("no-dest-dl-off-mismatch/data").toString(), ENABLE_DOWNLOADS to "false", ) val expected = s.binPath(url) - // When binaryDestination is not configured, binPath(url) and - // binPath(url, true) resolve to the same path because the - // isNullOrBlank() early return in binPath fires before the - // forceDownloadToData check. So cli and dataCLI share a binary. - assertEquals(expected, s.binPath(url, true)) - createBinary(expected, "1.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(expected, ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given no binaryDestination configured and downloads are disabled, returns the stale CLI when version mismatches and no separate data directory CLI path exists`() { - val url = URL("http://test.coder.invalid") - val s = settings( - DATA_DIRECTORY to testDir("no-dest-dl-off-mismatch-datacli-missing/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val expected = s.binPath(url) createBinary(expected, "1.0.0") val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } @@ -795,38 +355,6 @@ internal class EnsureCLITest { assertEquals(SemVer(1, 0, 0), ccm.version()) } - @Test - @IgnoreOnWindows - fun `given no binaryDestination configured and downloads are disabled, returns the CLI when version matches at the shared data directory path`() { - val url = URL("http://test.coder.invalid") - val s = settings( - DATA_DIRECTORY to testDir("no-dest-dl-off-no-cli-datacli-match/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "1.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - - @Test - @IgnoreOnWindows - fun `given no binaryDestination configured and downloads are disabled, returns the stale CLI when version mismatches at the shared data directory path`() { - val url = URL("http://test.coder.invalid") - val s = settings( - DATA_DIRECTORY to testDir("no-dest-dl-off-no-cli-datacli-wrong/data").toString(), - ENABLE_DOWNLOADS to "false", - ) - val fallbackPath = s.binPath(url, true) - createBinary(fallbackPath, "1.0.0") - - val ccm = runBlocking { ensureCLI(ctx(s), url, "2.0.0", noOpTextProgress) } - assertEquals(fallbackPath, ccm.localBinaryPath) - assertEquals(SemVer(1, 0, 0), ccm.version()) - } - @Test fun `given no binaryDestination configured and downloads are disabled, throws when no CLI binary exists`() { val url = URL("http://test.coder.invalid") @@ -877,7 +405,7 @@ internal class EnsureCLITest { } @Test - fun `given no binaryDestination configured and downloads are enabled, propagates non-AccessDenied errors during download to the caller`() { + fun `given no binaryDestination configured and downloads are enabled, propagates errors during download to the caller`() { val (srv, url) = mockServer(errorCode = HttpURLConnection.HTTP_INTERNAL_ERROR) try { val s = settings( @@ -892,27 +420,6 @@ internal class EnsureCLITest { } } - @Test - @IgnoreOnWindows - fun `given no binaryDestination configured and downloads are enabled, rethrows AccessDeniedException since the binary path is always inside the data directory`() { - val (srv, url) = mockServer() - val s = settings( - DATA_DIRECTORY to testDir("no-dest-dl-on-access-denied/data").toString(), - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val binParent = s.binPath(url).parent - try { - binParent.toFile().mkdirs() - binParent.toFile().setWritable(false) - assertFailsWith(AccessDeniedException::class) { - runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - } - } finally { - binParent.toFile().setWritable(true) - srv.stop(0) - } - } - @Test fun `given no binaryDestination configured and downloads are enabled, downloads and returns a fresh CLI when none exists yet`() { val (srv, url) = mockServer(version = "1.0.0") @@ -935,7 +442,7 @@ internal class EnsureCLITest { } @Test - fun `given no binaryDestination configured and downloads are enabled, propagates non-AccessDenied errors during download when CLI is missing`() { + fun `given no binaryDestination configured and downloads are enabled, propagates errors during download when CLI is missing`() { val (srv, url) = mockServer(errorCode = HttpURLConnection.HTTP_INTERNAL_ERROR) try { val s = settings( @@ -950,27 +457,6 @@ internal class EnsureCLITest { } } - @Test - @IgnoreOnWindows - fun `given no binaryDestination configured and downloads are enabled, rethrows AccessDeniedException when CLI is missing and access to the data directory is denied`() { - val (srv, url) = mockServer() - val s = settings( - DATA_DIRECTORY to testDir("no-dest-dl-on-no-cli-access-denied/data").toString(), - DISABLE_SIGNATURE_VALIDATION to "true", - ) - val binParent = s.binPath(url).parent - try { - binParent.toFile().mkdirs() - binParent.toFile().setWritable(false) - assertFailsWith(AccessDeniedException::class) { - runBlocking { ensureCLI(ctx(s), url, "1.0.0", noOpTextProgress) } - } - } finally { - binParent.toFile().setWritable(true) - srv.stop(0) - } - } - // Utilities private fun testDir(id: String): Path = tmpdir.resolve(id) @@ -1032,10 +518,6 @@ internal class EnsureCLITest { return Pair(srv, URL("http://localhost:" + srv.address.port)) } - /** Build the host directory component used by [CoderSettingsStore.withHost]. */ - private fun hostDir(url: URL): String = - if (url.port > 0) "${url.host}-${url.port}" else url.host - companion object { private val tmpdir: Path = Path.of(System.getProperty("java.io.tmpdir")) .resolve("coder-toolbox-test").resolve("ensure-cli") diff --git a/src/test/kotlin/com/coder/toolbox/settings/CoderSettingsTest.kt b/src/test/kotlin/com/coder/toolbox/settings/CoderSettingsTest.kt index b57354c0..fa4da2df 100644 --- a/src/test/kotlin/com/coder/toolbox/settings/CoderSettingsTest.kt +++ b/src/test/kotlin/com/coder/toolbox/settings/CoderSettingsTest.kt @@ -3,7 +3,6 @@ package com.coder.toolbox.settings import com.coder.toolbox.store.CODER_SSH_CONFIG_OPTIONS import com.coder.toolbox.store.CoderSettingsStore import com.coder.toolbox.store.DISABLE_AUTOSTART -import com.coder.toolbox.store.ENABLE_BINARY_DIR_FALLBACK import com.coder.toolbox.store.ENABLE_DOWNLOADS import com.coder.toolbox.store.HEADER_COMMAND import com.coder.toolbox.store.SSH_CONFIG_OPTIONS @@ -123,11 +122,6 @@ internal class CoderSettingsTest { var expected = "/tmp/coder-toolbox-test/bin-dir/test.coder.com" assertEquals(Path.of(expected).toAbsolutePath(), settings.readOnly().binPath(url).parent) - // Second argument bypasses override. - settings.updateDataDirectory("/tmp/coder-toolbox-test/data-dir") - expected = "/tmp/coder-toolbox-test/data-dir/test.coder.com" - assertEquals(Path.of(expected).toAbsolutePath(), settings.readOnly().binPath(url, true).parent) - // Binary name is always determined by OS and architecture. assertEquals( settings.readOnly().defaultCliBinaryNameByOsAndArch, @@ -280,7 +274,6 @@ internal class CoderSettingsTest { // Test defaults for the remaining settings. val settings = CoderSettingsStore(pluginTestSettingsStore(), Environment(), logger) assertEquals(true, settings.readOnly().enableDownloads) - assertEquals(false, settings.readOnly().enableBinaryDirectoryFallback) assertEquals(null, settings.readOnly().headerCommand) assertEquals(null, settings.readOnly().tls.certPath) assertEquals(null, settings.readOnly().tls.keyPath) @@ -296,7 +289,6 @@ internal class CoderSettingsTest { CoderSettingsStore( pluginTestSettingsStore( ENABLE_DOWNLOADS to false.toString(), - ENABLE_BINARY_DIR_FALLBACK to true.toString(), HEADER_COMMAND to "test header", TLS_CERT_PATH to "tls cert path", TLS_KEY_PATH to "tls key path", @@ -310,7 +302,6 @@ internal class CoderSettingsTest { ) assertEquals(false, settings.readOnly().enableDownloads) - assertEquals(true, settings.readOnly().enableBinaryDirectoryFallback) assertEquals("test header", settings.readOnly().headerCommand) assertEquals("tls cert path", settings.readOnly().tls.certPath) assertEquals("tls key path", settings.readOnly().tls.keyPath) diff --git a/src/test/kotlin/com/coder/toolbox/store/CoderSettingsStoreTest.kt b/src/test/kotlin/com/coder/toolbox/store/CoderSettingsStoreTest.kt index 0158c83e..3a71f169 100644 --- a/src/test/kotlin/com/coder/toolbox/store/CoderSettingsStoreTest.kt +++ b/src/test/kotlin/com/coder/toolbox/store/CoderSettingsStoreTest.kt @@ -5,7 +5,6 @@ import com.coder.toolbox.util.pluginTestSettingsStore import com.jetbrains.toolbox.api.core.diagnostics.Logger import io.mockk.mockk import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertTrue import java.net.URL import java.nio.file.Files @@ -97,19 +96,6 @@ class CoderSettingsStoreTest { assertTrue(result.endsWith(Path.of("test.coder.com", "coder-linux-amd64"))) } - @Test - fun `binPath uses dataDir when forceDownloadToData is true even with binaryDestination set`() { - setOsAndArch("Linux", "x86_64") - val settings = storeWith(BINARY_DESTINATION to "/custom/path") - val url = URL("https://test.coder.com") - val result = settings.binPath(url, forceDownloadToData = true) - - assertTrue(result.isAbsolute) - assertTrue(result.endsWith(Path.of("test.coder.com", "coder-linux-amd64"))) - // The custom path should NOT be part of the result when forceDownloadToData is true. - assertFalse(result.startsWith(Path.of("/custom/path").toAbsolutePath())) - } - @Test fun `binPath returns absolute binaryDestination path when it points to an existing executable`() { val tmpBin = Files.createTempFile("coder-test", null) From 003e15343e8575d89223342f515df2b69a372f45 Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Fri, 3 Apr 2026 22:50:46 +0300 Subject: [PATCH 2/4] chore: reuse exception prettifier We already had a complex exception prettifier that was no longer used (for quite some time now). We repurposed it for the error reporter used by the authentication wizard. --- .../kotlin/com/coder/toolbox/util/Error.kt | 32 +++++++++---------- .../com/coder/toolbox/views/ErrorReporter.kt | 16 ++-------- 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/src/main/kotlin/com/coder/toolbox/util/Error.kt b/src/main/kotlin/com/coder/toolbox/util/Error.kt index 54c4e97c..752aa074 100644 --- a/src/main/kotlin/com/coder/toolbox/util/Error.kt +++ b/src/main/kotlin/com/coder/toolbox/util/Error.kt @@ -4,31 +4,29 @@ import com.coder.toolbox.cli.ex.ResponseException import com.coder.toolbox.sdk.ex.APIResponseException import org.zeroturnaround.exec.InvalidExitValueException import java.net.ConnectException -import java.net.SocketTimeoutException -import java.net.URL import java.net.UnknownHostException -import javax.net.ssl.SSLHandshakeException -fun humanizeConnectionError(deploymentURL: URL, requireTokenAuth: Boolean, e: Exception): String { - val reason = e.message ?: "No reason was provided." - return when (e) { - is java.nio.file.AccessDeniedException -> "Access denied to ${e.file}." - is UnknownHostException -> "Unknown host ${e.message ?: deploymentURL.host}." - is InvalidExitValueException -> "CLI exited unexpectedly with ${e.exitValue}." +private fun accessDenied(file: String) = "Access denied to $file" +private fun fileSystemFailed(file: String) = "A file system operation failed when trying to access $file" + +fun Throwable.prettify(): String { + val reason = this.message ?: "No reason was provided" + return when (this) { + is AccessDeniedException -> accessDenied(this.file.toString()) + is java.nio.file.AccessDeniedException -> accessDenied(this.file) + is FileSystemException -> fileSystemFailed(this.file.toString()) + is java.nio.file.FileSystemException -> fileSystemFailed(this.file) + is UnknownHostException -> "Unknown host $reason" + is InvalidExitValueException -> "CLI exited unexpectedly with ${this.exitValue}." is APIResponseException -> { - if (e.isUnauthorized) { - if (requireTokenAuth) { - "Token was rejected by $deploymentURL; has your token expired?" - } else { - "Authorization failed to $deploymentURL." - } + if (this.isUnauthorized) { + "Authorization failed" } else { reason } } - is SocketTimeoutException -> "Unable to connect to $deploymentURL; is it up?" + is ResponseException, is ConnectException -> "Failed to download Coder CLI: $reason" - is SSLHandshakeException -> "Connection to $deploymentURL failed: $reason. See the documentation for TLS certificates for information on how to make your system trust certificates coming from your deployment." else -> reason } } diff --git a/src/main/kotlin/com/coder/toolbox/views/ErrorReporter.kt b/src/main/kotlin/com/coder/toolbox/views/ErrorReporter.kt index 39203752..e6d6ac53 100644 --- a/src/main/kotlin/com/coder/toolbox/views/ErrorReporter.kt +++ b/src/main/kotlin/com/coder/toolbox/views/ErrorReporter.kt @@ -1,7 +1,7 @@ package com.coder.toolbox.views import com.coder.toolbox.CoderToolboxContext -import com.coder.toolbox.sdk.ex.APIResponseException +import com.coder.toolbox.util.prettify import com.jetbrains.toolbox.api.remoteDev.ProviderVisibilityState import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.launch @@ -52,24 +52,12 @@ private class ErrorReporterImpl( context.ui.showSnackbar( UUID.randomUUID().toString(), context.i18n.ptrl("Error encountered while setting up Coder"), - context.i18n.pnotr(textError ?: ""), + context.i18n.pnotr(textError), context.i18n.ptrl("Dismiss") ) } } - private fun Throwable.prettify(): String? = when (this) { - is APIResponseException -> if (!this.reason.isNullOrBlank()) { - this.reason - } else this.message - - is AccessDeniedException, - is java.nio.file.AccessDeniedException -> "Access denied to ${this.message}" - - is java.nio.file.FileSystemException -> "A file system operation failed when trying to access ${this.message}" - else -> this.toString() - } - override fun flush() { if (errorBuffer.isNotEmpty() && visibilityState.value.applicationVisible) { errorBuffer.forEach { From c3db60871748c7911f553eec6b4e06b1f601819d Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Fri, 3 Apr 2026 22:56:32 +0300 Subject: [PATCH 3/4] chore: fix method doc to be more clear --- src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt | 6 +++--- .../com/coder/toolbox/settings/ReadOnlyCoderSettings.kt | 2 +- .../kotlin/com/coder/toolbox/store/CoderSettingsStore.kt | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt index 19dd2676..6a8617fe 100644 --- a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt +++ b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt @@ -54,10 +54,10 @@ internal data class Version( * 2. If the CLI version matches the build version, return it immediately. * 3. Otherwise, if downloads are enabled, attempt to download the CLI. * a. On success, return the CLI. - * b. If there is an exception propagates to the user. + * b. Any exception propagates to the user. * 4. If downloads are disabled: - * a. If neither the configured binary nor the data directory CLI can - * report a version, throw [IllegalStateException]. + * a. [IllegalStateException] is raised if the CLI does not exist (look into binary destination if it was configured, + * fallback to data dir otherwise) * b. Otherwise, warn the user and return the mismatched version. */ suspend fun ensureCLI( diff --git a/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt b/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt index 750a3074..74013751 100644 --- a/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt +++ b/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt @@ -173,7 +173,7 @@ interface ReadOnlyCoderSettings { fun binSource(url: URL): URL /** - * Returns a path to where the CLI is downloaded. + * Returns a path to where the specified deployment should place the CLI binary. */ fun binPath(url: URL): Path diff --git a/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt b/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt index b6d789bb..08341326 100644 --- a/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt +++ b/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt @@ -128,7 +128,7 @@ class CoderSettingsStore( * directory with the default CLI binary name. * 2. If the expanded (~ and $HOME) [binaryDestination] is an existing executable file, * return it as-is. - * 2. Otherwise, treat [binaryDestination] as a base directory and return a + * 3. Otherwise, treat [binaryDestination] as a base directory and return a * host-specific subdirectory with the default CLI binary name. */ override fun binPath(url: URL): Path { From d3df22d620ad11edac47a09eabce5b17f5927c91 Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Fri, 3 Apr 2026 23:11:24 +0300 Subject: [PATCH 4/4] chore: update README Reflect the new changes in the CLI resolution logic. --- README.md | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 53c3359a..cd598a5b 100644 --- a/README.md +++ b/README.md @@ -447,19 +447,16 @@ storage paths. The options can be configured from the plugin's main Workspaces p - `Binary source` specifies the source URL or relative path from which the Coder CLI should be downloaded. If a relative path is provided, it is resolved against the deployment domain. -- `Enable downloads` allows automatic downloading of the CLI if the current version is missing or outdated. - Defaults to enabled. +- `Enable downloads` allows automatic downloading of the CLI if the current version is missing or outdated. Enabled by + default. - `Binary destination` specifies where the CLI binary is placed. This can be a path to an existing executable (used as-is) or a base directory (the CLI is placed under a host-specific subdirectory). If blank, the data directory is used. Supports `~` and `$HOME` expansion. -- `Enable binary directory fallback` when enabled, if the binary destination is not writable the - plugin falls back to the data directory instead of failing. Only takes effect when downloads are - enabled and the binary destination differs from the data directory. Defaults to disabled. - - `Data directory` directory where deployment-specific data such as session tokens and CLI binaries - are stored. Each deployment gets a host-specific subdirectory (e.g. `coder.example.com`). + are stored. Each deployment gets a host-specific subdirectory (e.g. `coder.example.com`). Supports `~` and `$HOME` + expansion. - `Header command` command that outputs additional HTTP headers. Each line of output must be in the format key=value. The environment variable CODER_URL will be available to the command process. @@ -477,20 +474,21 @@ storage paths. The options can be configured from the plugin's main Workspaces p #### How CLI resolution works When connecting to a deployment the plugin ensures a compatible CLI binary is available. -The settings above interact as follows: - -1. If a CLI already exists at the binary destination and its version matches the deployment, it is - used immediately. -2. If **downloads are enabled**, the plugin downloads the matching version to the binary destination. - - If the download fails with a permission error and **binary directory fallback** is enabled (and - the binary destination is not already in the data directory), the plugin checks whether the data - directory already has a matching CLI. If so it is used; otherwise the plugin downloads to the - data directory instead. - - Any other download error is reported to the user. -3. If **downloads are disabled**, the plugin checks the data directory for a CLI whose version - matches the deployment. If no exact match is found anywhere, whichever CLI is available is - returned — preferring the binary destination unless it is missing, in which case the data - directory CLI is used regardless of its version. If no CLI exists at all, an error is raised. +The binary location is resolved as follows: + +- If **binary destination** points to an existing executable file, it is used as-is. +- If **binary destination** is set but is not an executable file, it is treated as a base + directory and the CLI is placed under a host-specific subdirectory (e.g. + `/coder.example.com/`). +- If **binary destination** is not set, the data directory is used instead. + +Once the binary location is resolved: + +1. If a CLI already exists there and its version matches the deployment, it is used immediately. +2. Otherwise, if **downloads are enabled**, the plugin downloads the matching version to that location. + Any download error is reported to the user. +3. If **downloads are disabled** and the CLI exists but its version does not match, the stale + CLI is used with a warning. If no CLI exists at all, an error is raised. ### TLS settings