From 0e685591ebe74636bc8c28bdd55119f9f05d6d36 Mon Sep 17 00:00:00 2001
From: Jen Basch <jbasch@apple.com>
Date: Tue, 3 Mar 2026 16:23:42 -0800
Subject: [PATCH] Respect `--omit-project-settings` for all evaluator options
 (#1459)

---
 .idea/inspectionProfiles/Project_Default.xml  |  24 +++-
 .../kotlin/org/pkl/commons/cli/CliCommand.kt  |  35 +++---
 .../org/pkl/commons/cli/CliCommandTest.kt     | 112 ++++++++++++++++--
 3 files changed, 143 insertions(+), 28 deletions(-)

diff --git a/.idea/inspectionProfiles/Project_Default.xml b/.idea/inspectionProfiles/Project_Default.xml
index d723e4a15..9ece71d59 100644
--- a/.idea/inspectionProfiles/Project_Default.xml
+++ b/.idea/inspectionProfiles/Project_Default.xml
@@ -12,6 +12,27 @@
       <option name="REPORT_FIELDS" value="true" />
     </inspection_tool>
     <inspection_tool class="ClassCanBeRecord" enabled="false" level="WEAK WARNING" enabled_by_default="false" />
+    <inspection_tool class="CustomRegExpInspection" enabled="true" level="WARNING" enabled_by_default="true">
+      <option name="myConfigurations">
+        <list>
+          <RegExpInspectionConfiguration>
+            <option name="name" value="PklCliDirectProjectEvaluatorSettingsAccess" />
+            <option name="suppressId" value="PklCliDirectProjectEvaluatorSettingsAccess" />
+            <option name="uuid" value="dd497f47-d38f-3fab-9ed7-eabe699620c8" />
+            <option name="patterns">
+              <list>
+                <InspectionPattern>
+                  <option name="regExp" value="project\?\.evaluatorSettings" />
+                  <option name="_fileType" value="Kotlin" />
+                  <option name="searchContext" value="ANY" />
+                  <option name="replacement" value="evaluatorSettings" />
+                </InspectionPattern>
+              </list>
+            </option>
+          </RegExpInspectionConfiguration>
+        </list>
+      </option>
+    </inspection_tool>
     <inspection_tool class="FieldMayBeFinal" enabled="true" level="INFORMATION" enabled_by_default="true">
       <scope name="AllExceptTruffleAst" level="WARNING" enabled="true" />
     </inspection_tool>
@@ -73,5 +94,6 @@
       <option name="processLiterals" value="true" />
       <option name="processComments" value="true" />
     </inspection_tool>
+    <inspection_tool class="dd497f47-d38f-3fab-9ed7-eabe699620c8" enabled="true" level="ERROR" enabled_by_default="true" editorAttributes="ERRORS_ATTRIBUTES" />
   </profile>
-</component>
+</component>
\ No newline at end of file
diff --git a/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt b/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt
index fb0a4f69e..a59da40a3 100644
--- a/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt
+++ b/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt
@@ -139,6 +139,7 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
   }
 
   private val evaluatorSettings: PklEvaluatorSettings? by lazy {
+    @Suppress("PklCliDirectProjectEvaluatorSettingsAccess")
     if (cliOptions.omitProjectSettings) null else project?.evaluatorSettings
   }
 
@@ -199,34 +200,30 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
     )
   }
 
-  protected val useColor: Boolean by lazy { cliOptions.color?.hasColor() ?: false }
-
-  private val proxyAddress: URI? by lazy {
-    cliOptions.httpProxy
-      ?: project?.evaluatorSettings?.http?.proxy?.address
-      ?: settings.http?.proxy?.address
+  protected val useColor: Boolean by lazy {
+    cliOptions.color?.hasColor() ?: evaluatorSettings?.color?.hasColor() ?: false
   }
 
-  private val noProxy: List<String>? by lazy {
+  protected val proxyAddress: URI? by lazy {
+    cliOptions.httpProxy ?: evaluatorSettings?.http?.proxy?.address ?: settings.http?.proxy?.address
+  }
+
+  protected val noProxy: List<String>? by lazy {
     cliOptions.httpNoProxy
-      ?: project?.evaluatorSettings?.http?.proxy?.noProxy
+      ?: evaluatorSettings?.http?.proxy?.noProxy
       ?: settings.http?.proxy?.noProxy
   }
 
-  private val httpRewrites: Map<URI, URI>? by lazy {
-    cliOptions.httpRewrites
-      ?: project?.evaluatorSettings?.http?.rewrites
-      ?: settings.http?.rewrites()
+  protected val httpRewrites: Map<URI, URI>? by lazy {
+    cliOptions.httpRewrites ?: evaluatorSettings?.http?.rewrites ?: settings.http?.rewrites()
   }
 
-  private val externalModuleReaders: Map<String, PklEvaluatorSettings.ExternalReader> by lazy {
-    (project?.evaluatorSettings?.externalModuleReaders ?: emptyMap()) +
-      cliOptions.externalModuleReaders
+  protected val externalModuleReaders: Map<String, PklEvaluatorSettings.ExternalReader> by lazy {
+    (evaluatorSettings?.externalModuleReaders ?: emptyMap()) + cliOptions.externalModuleReaders
   }
 
-  private val externalResourceReaders: Map<String, PklEvaluatorSettings.ExternalReader> by lazy {
-    (project?.evaluatorSettings?.externalResourceReaders ?: emptyMap()) +
-      cliOptions.externalResourceReaders
+  protected val externalResourceReaders: Map<String, PklEvaluatorSettings.ExternalReader> by lazy {
+    (evaluatorSettings?.externalResourceReaders ?: emptyMap()) + cliOptions.externalResourceReaders
   }
 
   private val externalProcesses:
@@ -240,7 +237,7 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
   }
 
   private val traceMode: TraceMode by lazy {
-    cliOptions.traceMode ?: project?.evaluatorSettings?.traceMode ?: TraceMode.COMPACT
+    cliOptions.traceMode ?: evaluatorSettings?.traceMode ?: TraceMode.COMPACT
   }
 
   private fun HttpClient.Builder.addDefaultCliCertificates() {
diff --git a/pkl-commons-cli/src/test/kotlin/org/pkl/commons/cli/CliCommandTest.kt b/pkl-commons-cli/src/test/kotlin/org/pkl/commons/cli/CliCommandTest.kt
index 61e9f1505..8e16d60d9 100644
--- a/pkl-commons-cli/src/test/kotlin/org/pkl/commons/cli/CliCommandTest.kt
+++ b/pkl-commons-cli/src/test/kotlin/org/pkl/commons/cli/CliCommandTest.kt
@@ -16,6 +16,7 @@
 package org.pkl.commons.cli
 
 import com.github.ajalt.clikt.core.parse
+import com.github.ajalt.clikt.parameters.groups.provideDelegate
 import java.net.URI
 import java.nio.file.Path
 import kotlin.io.path.ExperimentalPathApi
@@ -25,26 +26,44 @@ import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.api.io.TempDir
 import org.pkl.commons.cli.commands.BaseCommand
+import org.pkl.commons.cli.commands.ProjectOptions
+import org.pkl.commons.writeString
 import org.pkl.core.SecurityManagers
+import org.pkl.core.evaluatorSettings.TraceMode
+import org.pkl.core.util.IoUtils
 
 @OptIn(ExperimentalPathApi::class)
 class CliCommandTest {
 
-  class CliTest(options: CliBaseOptions) : CliCommand(options) {
-    override fun doRun() = Unit
-
-    val myAllowedResources = allowedResources
-    val myAllowedModules = allowedModules
-    val myResolvedSourceModules = resolvedSourceModules
-  }
-
   private val cmd =
     object : BaseCommand("test", "") {
+      val projectOptions: ProjectOptions by ProjectOptions()
+
       override fun run() = Unit
 
       override val helpString: String = ""
     }
 
+  class CliTest(options: CliBaseOptions) : CliCommand(options) {
+    override fun doRun() = Unit
+
+    val myResolvedSourceModules = resolvedSourceModules
+    val myAllowedModules = allowedModules
+    val myAllowedResources = allowedResources
+    val myRootDir = rootDir
+    val myModulePath = modulePath
+    val myProxyAddress = proxyAddress
+    val myNoProxy = noProxy
+    val myHttpRewrites = httpRewrites
+    val myExternalModuleReaders = externalModuleReaders
+    val myExternalResourceReaders = externalResourceReaders
+
+    fun myEvaluatorBuilder() = evaluatorBuilder()
+
+    @Suppress("PklCliDirectProjectEvaluatorSettingsAccess")
+    val myProjectEvaluatorSettings = project?.evaluatorSettings
+  }
+
   @Test
   fun `--external-resource-reader and --external-module-reader populate allowed modules and resources`() {
     cmd.parse(
@@ -184,4 +203,81 @@ class CliCommandTest {
     assertThat(cliTest.myResolvedSourceModules)
       .isEqualTo(listOf(tempDir.toUri().resolve("package://example.com/foo@1.2.3#/bar.pkl")))
   }
+
+  val projectWithAllEvaluatorSettings =
+    """
+    amends "pkl:Project"
+    
+    evaluatorSettings {
+      externalProperties { ["foo"] = "bar" }
+      env { ["foo"] = "bar" }
+      allowedModules { "file:" }
+      allowedResources { "file:" }
+      color = "always"
+      noCache = true
+      modulePath { "/tmp/modulepath" }
+      timeout = 30.s
+      moduleCacheDir = "/tmp/cache"
+      rootDir = "/tmp/root"
+      http {
+        proxy {
+          address = "http://example.com:80"
+          noProxy { "example.com" }
+        }
+        rewrites {
+          ["https://example.com/foo/"] = "https://example.com/bar/"
+        }
+      }
+      externalModuleReaders {
+        ["foo"] { executable = "foo" }
+      }
+      externalResourceReaders {
+        ["foo"] { executable = "foo" }
+      }
+      traceMode = "pretty"
+    }
+  """
+      .trimIndent()
+
+  @Test
+  fun `test that --omit-project-settings actually omits project settings`(@TempDir tempDir: Path) {
+    val project = tempDir.resolve("PklProject").writeString(projectWithAllEvaluatorSettings)
+    cmd.parse(arrayOf("--working-dir=$tempDir", "--omit-project-settings"))
+    val opts =
+      cmd.baseOptions.baseOptions(listOf(project.toUri()), cmd.projectOptions, testMode = true)
+    val cliTest = CliTest(opts)
+    val builder = cliTest.myEvaluatorBuilder()
+    assertThat(cliTest.myAllowedModules).isEqualTo(SecurityManagers.defaultAllowedModules)
+    assertThat(cliTest.myAllowedResources).isEqualTo(SecurityManagers.defaultAllowedResources)
+    assertThat(cliTest.myRootDir).isNull()
+    assertThat(builder.environmentVariables).isEqualTo(System.getenv())
+    assertThat(builder.externalProperties).isEmpty()
+    assertThat(builder.moduleCacheDir).isEqualTo(IoUtils.getDefaultModuleCacheDir())
+    assertThat(cliTest.myModulePath).isEmpty()
+    assertThat(builder.color).isFalse
+    assertThat(cliTest.myProxyAddress).isNull()
+    assertThat(cliTest.myNoProxy).isNull()
+    assertThat(cliTest.myHttpRewrites).isNull()
+    assertThat(cliTest.myExternalModuleReaders).isEmpty()
+    assertThat(cliTest.myExternalResourceReaders).isEmpty()
+    assertThat(builder.traceMode).isEqualTo(TraceMode.COMPACT)
+  }
+
+  // hygiene test to ensure new evaluator settings get covered by the above test
+  @Test
+  fun `test project sets all evaluator settings`(@TempDir tempDir: Path) {
+    val project = tempDir.resolve("PklProject").writeString(projectWithAllEvaluatorSettings)
+    cmd.parse(arrayOf("--working-dir=$tempDir"))
+    val opts = cmd.baseOptions.baseOptions(listOf(project.toUri()), testMode = true)
+    val cliTest = CliTest(opts)
+    cliTest.myProjectEvaluatorSettings
+      ?.javaClass
+      ?.declaredMethods
+      ?.filter { it.parameterCount == 0 }
+      ?.forEach {
+        assertThat(it.invoke(cliTest.myProjectEvaluatorSettings))
+          .overridingErrorMessage("project evaluator settings returned null for ${it.name}")
+          .isNotNull
+      }
+  }
 }