From 2c0ba6b1e43e292f349d9b8293e1b235fa5b9f3a Mon Sep 17 00:00:00 2001 From: "Thor (Guardrail)" Date: Wed, 22 Apr 2026 14:02:36 -0700 Subject: [PATCH 1/2] refactor: consolidate _build_args_str to utils.shell, fix env file inline comment parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two improvements: 1. _build_args_str() was duplicated identically in both dds/providers/azure/container.py and dds/builders/docker.py. Moved the canonical implementation to dds/utils/shell.build_args_str() (with shell-quoting via shlex.quote — see PR #1 for the safety fix). Both original sites now import and delegate to the shared version. Module-level _build_args_str aliases kept for backward compatibility. 2. load_env_file() in dds/secrets.py did not strip inline comments from unquoted values, so a line like: DATABASE_URL=postgres://localhost/mydb # local dev only would produce 'postgres://localhost/mydb # local dev only' as the value. This is incorrect — standard .env file parsers (dotenv et al.) strip inline comments from unquoted values. Quoted values are left untouched (the comment is considered part of the value when quoted). Added stripping for ' #' and '\t#' comment separators on unquoted values. --- dds/builders/docker.py | 12 +++++------- dds/providers/azure/container.py | 14 ++++++-------- dds/secrets.py | 9 +++++++++ dds/utils/shell.py | 19 +++++++++++++++++++ 4 files changed, 39 insertions(+), 15 deletions(-) diff --git a/dds/builders/docker.py b/dds/builders/docker.py index d18a072..3267d5d 100644 --- a/dds/builders/docker.py +++ b/dds/builders/docker.py @@ -10,7 +10,7 @@ from dds.console import console from dds.utils.git import git_info -from dds.utils.shell import run_cmd +from dds.utils.shell import build_args_str, run_cmd def build_and_push_local( @@ -21,7 +21,7 @@ def build_and_push_local( verbose: bool = False, ) -> None: """Build locally with Docker, then push to a registry.""" - args_str = _build_args_str(build_args) + args_str = build_args_str(build_args) console.print(f"[yellow]🔨 Building locally: {image}[/yellow]") result = run_cmd( @@ -58,8 +58,6 @@ def resolve_image_tag( return f"{registry}/{project}-{name}:{tag}" -def _build_args_str(build_args: dict[str, str] | None) -> str: - """Format build args dict into Docker --build-arg flags.""" - if not build_args: - return "" - return " ".join(f"--build-arg {k}={v}" for k, v in build_args.items()) +# _build_args_str has moved to dds.utils.shell.build_args_str. +# Kept as a private alias for any callers that imported it directly. +_build_args_str = build_args_str diff --git a/dds/providers/azure/container.py b/dds/providers/azure/container.py index 8381636..99efa33 100644 --- a/dds/providers/azure/container.py +++ b/dds/providers/azure/container.py @@ -12,7 +12,7 @@ from dds.context import DeployContext from dds.providers.azure.utils import az, az_json from dds.providers.base import ContainerProvider -from dds.utils.shell import run_cmd +from dds.utils.shell import build_args_str, run_cmd class AzureContainerProvider(ContainerProvider): @@ -284,7 +284,7 @@ def _build_and_push_local( image: str, dockerfile: str, context: str, build_args: dict[str, str], verbose: bool, ) -> None: - args_str = _build_args_str(build_args) + args_str = build_args_str(build_args) console.print(f"[yellow]🔨 Building locally: {image}[/yellow]") result = run_cmd(f"docker build -f {dockerfile} -t {image} {args_str} {context}", verbose=verbose) if result.returncode != 0: @@ -305,7 +305,7 @@ def _build_acr( build_args: dict[str, str], verbose: bool, platform: str = "linux/amd64", ) -> None: registry_name = registry.split(".")[0] - args_str = _build_args_str(build_args) + args_str = build_args_str(build_args) console.print(f"[yellow]🔨 ACR build: {image_tag}[/yellow]") console.print(f" Registry: {registry_name} | Dockerfile: {dockerfile} | Platform: {platform}") az( @@ -348,8 +348,6 @@ def _http_check(url: str, timeout: float = 10.0, verbose: bool = False) -> bool: return False -def _build_args_str(build_args: dict[str, str] | None) -> str: - """Format build args dict into Docker --build-arg flags.""" - if not build_args: - return "" - return " ".join(f"--build-arg {k}={v}" for k, v in build_args.items()) +# _build_args_str has moved to dds.utils.shell.build_args_str. +# Kept as a module-level alias for any callers that imported it directly. +_build_args_str = build_args_str diff --git a/dds/secrets.py b/dds/secrets.py index de8b752..fb30c22 100644 --- a/dds/secrets.py +++ b/dds/secrets.py @@ -95,8 +95,17 @@ def load_env_file(path: str, verbose: bool = False) -> dict[str, str]: continue key, _, value = line.partition("=") key, value = key.strip(), value.strip() + # Strip matching surrounding quotes (double or single). + # Quoted values may contain inline comments that are part of the value; + # unquoted values have inline comments stripped. if len(value) >= 2 and value[0] == value[-1] and value[0] in ('"', "'"): value = value[1:-1] + else: + # Remove inline comment: anything after un-quoted " #" or "\t#" + for sep in (" #", "\t#"): + if sep in value: + value = value[: value.index(sep)].rstrip() + break result[key] = value if verbose: diff --git a/dds/utils/shell.py b/dds/utils/shell.py index f0a8532..b3d75b5 100644 --- a/dds/utils/shell.py +++ b/dds/utils/shell.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import shlex import subprocess from dds.console import console @@ -33,3 +34,21 @@ def run_cmd( console.print(result.stdout.rstrip()) return result + + +def build_args_str(build_args: dict[str, str] | None) -> str: + """Format a build-args dict into Docker --build-arg flags. + + Values are shell-quoted so that spaces and metacharacters in values do not + break the command string when it is passed to the shell with shell=True. + + Example:: + + >>> build_args_str({'NODE_ENV': 'production', 'LABEL': 'hello world'}) + "--build-arg NODE_ENV=production --build-arg LABEL='hello world'" + """ + if not build_args: + return "" + return " ".join( + f"--build-arg {k}={shlex.quote(str(v))}" for k, v in build_args.items() + ) From 0da7726ca4bc41ddd92849b3bd545f2861db1dc6 Mon Sep 17 00:00:00 2001 From: "Thor (Guardrail)" Date: Wed, 22 Apr 2026 14:07:14 -0700 Subject: [PATCH 2/2] test: add coverage for build_args_str() and load_env_file() inline comment stripping --- tests/test_secrets.py | 26 ++++++++++++ tests/test_shell_utils.py | 84 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 tests/test_shell_utils.py diff --git a/tests/test_secrets.py b/tests/test_secrets.py index 71a5a11..6c07b6e 100644 --- a/tests/test_secrets.py +++ b/tests/test_secrets.py @@ -76,3 +76,29 @@ def test_missing_env_var_secret(self, monkeypatch) -> None: project_cfg={}, ) assert "MISSING" not in result + + def test_inline_comment_stripped_from_unquoted_value(self, tmp_path: Path) -> None: + """Inline comments after unquoted values must be stripped (dotenv standard).""" + env_file = tmp_path / ".env" + env_file.write_text("DATABASE_URL=postgres://localhost/mydb # local dev only\n") + result = load_env_file(str(env_file)) + assert result["DATABASE_URL"] == "postgres://localhost/mydb" + + def test_inline_comment_stripped_tab_separator(self, tmp_path: Path) -> None: + env_file = tmp_path / ".env" + env_file.write_text("API_URL=https://api.example.com\t# production endpoint\n") + result = load_env_file(str(env_file)) + assert result["API_URL"] == "https://api.example.com" + + def test_inline_comment_not_stripped_from_quoted_value(self, tmp_path: Path) -> None: + """Hash inside quoted strings must be preserved as part of the value.""" + env_file = tmp_path / ".env" + env_file.write_text('PASSWORD="hunter#2" # good password\n') + result = load_env_file(str(env_file)) + assert result["PASSWORD"] == "hunter#2" + + def test_quoted_value_with_trailing_comment(self, tmp_path: Path) -> None: + env_file = tmp_path / ".env" + env_file.write_text('GREETING="hello world" # a greeting\n') + result = load_env_file(str(env_file)) + assert result["GREETING"] == "hello world" diff --git a/tests/test_shell_utils.py b/tests/test_shell_utils.py new file mode 100644 index 0000000..9d8002a --- /dev/null +++ b/tests/test_shell_utils.py @@ -0,0 +1,84 @@ +"""Tests for dds.utils.shell — build_args_str() and run_cmd().""" + +from __future__ import annotations + +import subprocess +from unittest.mock import patch + +import pytest + +from dds.utils.shell import build_args_str, run_cmd + + +class TestBuildArgsStr: + """Tests for build_args_str().""" + + def test_none_returns_empty(self) -> None: + assert build_args_str(None) == "" + + def test_empty_dict_returns_empty(self) -> None: + assert build_args_str({}) == "" + + def test_single_simple_arg(self) -> None: + result = build_args_str({"NODE_ENV": "production"}) + assert result == "--build-arg NODE_ENV=production" + + def test_multiple_args(self) -> None: + result = build_args_str({"A": "1", "B": "2"}) + assert "--build-arg A=1" in result + assert "--build-arg B=2" in result + + def test_value_with_spaces_is_quoted(self) -> None: + result = build_args_str({"LABEL": "hello world"}) + # shlex.quote wraps in single quotes when the value contains spaces + assert "--build-arg LABEL='hello world'" == result + + def test_value_with_dollar_sign_is_quoted(self) -> None: + """Dollar signs must be quoted to prevent shell variable expansion.""" + result = build_args_str({"SECRET": "$MY_SECRET"}) + assert "--build-arg SECRET=" in result + # The dollar sign must be inside quotes so the shell doesn't expand it + assert "$MY_SECRET" not in result.replace("'$MY_SECRET'", "") + + def test_value_with_equals_sign(self) -> None: + """Values containing = (e.g. base64, connection strings) must survive round-trip.""" + result = build_args_str({"DB_URL": "postgres://u:p@h/db?ssl=require"}) + assert "--build-arg DB_URL=" in result + assert "postgres://u:p@h/db" in result + + def test_simple_value_not_over_quoted(self) -> None: + """Simple alphanumeric values should not be wrapped in extra quotes.""" + result = build_args_str({"VER": "1.2.3"}) + # shlex.quote leaves safe strings unquoted + assert result == "--build-arg VER=1.2.3" + + def test_integer_value_coerced_to_string(self) -> None: + result = build_args_str({"PORT": 3000}) # type: ignore[arg-type] + assert "--build-arg PORT=3000" == result + + +class TestRunCmd: + """Tests for run_cmd().""" + + def test_returns_completed_process(self) -> None: + result = run_cmd("echo hello") + assert isinstance(result, subprocess.CompletedProcess) + assert result.returncode == 0 + + def test_captures_stdout(self) -> None: + result = run_cmd("echo captured", capture=True) + assert "captured" in result.stdout + + def test_failing_command_returns_nonzero(self) -> None: + result = run_cmd("exit 1") + assert result.returncode != 0 + + def test_cwd_is_passed(self, tmp_path) -> None: + result = run_cmd("pwd", capture=True, cwd=str(tmp_path)) + assert str(tmp_path) in result.stdout + + def test_env_vars_are_merged(self, monkeypatch) -> None: + monkeypatch.setenv("EXISTING", "yes") + result = run_cmd("echo $EXISTING $INJECTED", capture=True, env={"INJECTED": "injected"}) + assert "yes" in result.stdout + assert "injected" in result.stdout