From 74319b58b916fec5ec49fd84773ce25ef88e7249 Mon Sep 17 00:00:00 2001 From: Jaixii Date: Mon, 10 Aug 2026 02:41:07 -0400 Subject: [PATCH 1/4] fix(ci): SHA-pin actions/checkout in cowork-auto-pr workflow The cowork-auto-pr.yml workflow used unpinned actions/checkout@v4 while all other workflows (ci.yml, publish.yml, release-audit.yml) use SHA-pinned versions. This creates supply-chain risk and inconsistency. Pinned to d23441a48e516b6c34aea4fa41551a30e30af803 (v6) to match the fleet standard. --- .github/workflows/cowork-auto-pr.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/cowork-auto-pr.yml b/.github/workflows/cowork-auto-pr.yml index b27f04e..3a455a8 100644 --- a/.github/workflows/cowork-auto-pr.yml +++ b/.github/workflows/cowork-auto-pr.yml @@ -16,7 +16,7 @@ jobs: # without this step every run failed with "not a git repository" and no # PR was ever opened (fleet-wide defect: 11/11 seeded copies lacked it). - name: Check out the pushed branch - uses: actions/checkout@v4 + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: ref: ${{ github.ref_name }} fetch-depth: 0 From b501fc426412e8702a137c22a56554fa8f9530b5 Mon Sep 17 00:00:00 2001 From: Jaixii Date: Fri, 14 Aug 2026 23:59:54 -0400 Subject: [PATCH 2/4] test(ci): add SHA-pin and silent-failure regression tests - test_all_actions_sha_pinned: enforces 40-char SHA refs for all remote actions - test_no_silent_failure_on_validation_steps: catches '|| true' suppression on lint/test/audit steps (validation theater trap) Regression guard so future mutable-tag PRs are caught in CI. --- tests/test_ci_hygiene.py | 85 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 tests/test_ci_hygiene.py diff --git a/tests/test_ci_hygiene.py b/tests/test_ci_hygiene.py new file mode 100644 index 0000000..707bd6b --- /dev/null +++ b/tests/test_ci_hygiene.py @@ -0,0 +1,85 @@ +"""CI hygiene regression tests. + +Ensures workflow files follow security best practices: +- All GitHub Actions are SHA-pinned (no mutable tags like @v4) +- No silent-failure traps (|| true on validation steps) +""" + +from __future__ import annotations + +import pytest +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +WORKFLOWS_DIR = REPO_ROOT / ".github" / "workflows" + +# Pattern: uses: OWNER/ACTION@REF +# SHA-pinned refs are exactly 40 hex chars. +# Mutable tags look like @v4, @v4.2.2, @main, @release/v1, etc. +USES_PATTERN = re.compile(r"uses:\s*([^@\s]+)@(\S+)") +SHA_PATTERN = re.compile(r"^[0-9a-f]{40}$") + +# Local/composite actions (e.g. ./.github/actions/foo) don't need SHA pins. +LOCAL_ACTION_PREFIX = "./" + + +class TestWorkflowHygiene: + """Regression guards for CI workflow security and correctness.""" + + @pytest.fixture + def workflow_files(self) -> list[Path]: + files = list(WORKFLOWS_DIR.glob("*.yml")) + list(WORKFLOWS_DIR.glob("*.yaml")) + if not files: + pytest.skip("No workflow files found") + return files + + def test_all_actions_sha_pinned(self, workflow_files: list[Path]) -> None: + """Every remote action reference must use a 40-char SHA, not a mutable tag. + + Mutable tags like @v4 can be silently moved to point at different commits, + creating a supply-chain attack vector. SHA pins lock the exact commit. + """ + violations: list[str] = [] + for wf in workflow_files: + for lineno, line in enumerate(wf.read_text(encoding="utf-8").splitlines(), 1): + match = USES_PATTERN.search(line) + if not match: + continue + action, ref = match.group(1), match.group(2) + # Strip inline comments (e.g. "# v4.2.2") + ref = ref.split("#")[0].strip() + if action.startswith(LOCAL_ACTION_PREFIX): + continue + if not SHA_PATTERN.match(ref): + violations.append(f"{wf.name}:{lineno} {action}@{ref}") + + assert not violations, ( + f"Found {len(violations)} mutable action reference(s). " + "Pin to a 40-char SHA instead:\n" + "\n".join(violations) + ) + + def test_no_silent_failure_on_validation_steps(self, workflow_files: list[Path]) -> None: + """Validation/lint/test steps must not suppress failures with '|| true'. + + A step whose purpose is to fail the build on defects (linters, type + checkers, security scanners) must not hide failures. This catches the + 'validation theater' trap where a real check is neutered. + """ + validation_keywords = ("lint", "check", "test", "audit", "scan", "format", "typecheck") + violations: list[str] = [] + for wf in workflow_files: + lines = wf.read_text(encoding="utf-8").splitlines() + for lineno, line in enumerate(lines, 1): + stripped = line.strip() + if "|| true" not in stripped: + continue + # Check if this line or the step name above contains a validation keyword + context = " ".join(lines[max(0, lineno - 5) : lineno]).lower() + if any(kw in context for kw in validation_keywords): + violations.append(f"{wf.name}:{lineno} {stripped[:80]}") + + assert not violations, ( + f"Found {len(violations)} validation step(s) with '|| true' suppression. " + "Remove the suppression so failures are visible:\n" + "\n".join(violations) + ) From 243b18a5c7786eb4a5c396d965430de1da11b3ce Mon Sep 17 00:00:00 2001 From: Jaixii Date: Sun, 16 Aug 2026 20:32:13 -0400 Subject: [PATCH 3/4] cowork-bot: stream subprocess output in real time via Popen Switch dispatch from subprocess.run(capture_output=True) to subprocess.Popen with inherited file descriptors. The previous implementation buffered all child stdout/stderr in memory before printing, causing UX lag on long-running tools (deploydiff, schemaforge, configdrift) and potential OOM on large outputs. Popen streams output directly to the parent terminal. Added test_dispatch_streaming.py with regression guards against capture_output=True and stdout=PIPE. Updated existing dispatch tests to mock Popen instead of run. --- src/devforge/cli.py | 13 ++-- tests/test_cli.py | 24 +++++--- tests/test_dispatch_streaming.py | 102 +++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 18 deletions(-) create mode 100644 tests/test_dispatch_streaming.py diff --git a/src/devforge/cli.py b/src/devforge/cli.py index 5abac27..3517eaa 100644 --- a/src/devforge/cli.py +++ b/src/devforge/cli.py @@ -172,16 +172,13 @@ def dispatch(ctx: typer.Context): # `--config file.yaml`) reach the underlying CLI instead of being # rejected by typer as "No such option". forwarded = list(ctx.args) - result = subprocess.run( + # Stream output in real time via Popen with inherited file descriptors. + # The previous subprocess.run(capture_output=True) buffered all output + # in memory, causing UX lag and potential OOM on large tool output. + proc = subprocess.Popen( [sys.executable, "-m", module_name] + forwarded, - capture_output=True, - text=True, ) - if result.stdout: - sys.stdout.write(result.stdout) - if result.stderr: - sys.stderr.write(result.stderr) - sys.exit(result.returncode) + sys.exit(proc.wait()) dispatch.__name__ = tool_name dispatch.__doc__ = f"Run `{pkg}` commands via the {tool_name} subcommand." diff --git a/tests/test_cli.py b/tests/test_cli.py index 022df80..0140348 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -125,19 +125,21 @@ def test_dispatch_not_installed_shows_install_hint(self, _mock): assert 'pip install "git+https://github.com/Coding-Dev-Tools/devforge-cli.git[guard]"' in result.stdout @mock.patch("devforge.cli._is_tool_installed", return_value=True) - @mock.patch("devforge.cli.subprocess.run") - def test_dispatch_installed_tool_runs(self, mock_run, _mock_installed): + @mock.patch("devforge.cli.subprocess.Popen") + def test_dispatch_installed_tool_runs(self, mock_popen, _mock_installed): """When a tool is installed, dispatch calls the subprocess.""" - mock_run.return_value = mock.MagicMock(returncode=0) + mock_proc = mock.MagicMock() + mock_proc.wait.return_value = 0 + mock_popen.return_value = mock_proc with mock.patch("devforge.cli.sys.exit"): runner.invoke(app, ["guard"]) - mock_run.assert_called_once() - cmd = mock_run.call_args[0][0] + mock_popen.assert_called_once() + cmd = mock_popen.call_args[0][0] assert "api_contract_guardian" in cmd @mock.patch("devforge.cli._is_tool_installed", return_value=True) - @mock.patch("devforge.cli.subprocess.run") - def test_dispatch_forwards_tool_flags(self, mock_run, _mock_installed): + @mock.patch("devforge.cli.subprocess.Popen") + def test_dispatch_forwards_tool_flags(self, mock_popen, _mock_installed): """Tool flags (e.g. `--config file.yaml`) must reach the underlying CLI. Regression guard for the silent-failure trap where typer rejected any @@ -145,11 +147,13 @@ def test_dispatch_forwards_tool_flags(self, mock_run, _mock_installed): With ignore_unknown_options/allow_extra_args, such flags are forwarded via ctx.args. """ - mock_run.return_value = mock.MagicMock(returncode=0) + mock_proc = mock.MagicMock() + mock_proc.wait.return_value = 0 + mock_popen.return_value = mock_proc with mock.patch("devforge.cli.sys.exit"): runner.invoke(app, ["guard", "--config", "x.yaml", "--verbose"]) - mock_run.assert_called_once() - cmd = mock_run.call_args[0][0] + mock_popen.assert_called_once() + cmd = mock_popen.call_args[0][0] # Underlying module is launched... assert "api_contract_guardian" in cmd # ...and the tool flags are forwarded, not swallowed by typer. diff --git a/tests/test_dispatch_streaming.py b/tests/test_dispatch_streaming.py new file mode 100644 index 0000000..6015cb5 --- /dev/null +++ b/tests/test_dispatch_streaming.py @@ -0,0 +1,102 @@ +"""Regression tests for subprocess output streaming in dispatch. + +The dispatch command must stream stdout/stderr in real time rather than +buffering the entire output via capture_output=True. Long-running tools +(deploydiff, schemaforge, configdrift on large datasets) can produce +megabytes of output that should reach the user's terminal immediately. +""" + +from __future__ import annotations + +import subprocess +import sys +from unittest import mock + +from typer.testing import CliRunner + +from devforge.cli import app + +runner = CliRunner() + + +class TestDispatchStreaming: + """dispatch must NOT use subprocess.run with capture_output=True. + + Real-time streaming requires subprocess.Popen (or subprocess.run with + stdout=None, stderr=None) so the child process inherits the parent's + file descriptors directly. + """ + + @mock.patch("devforge.cli._is_tool_installed", return_value=True) + def test_dispatch_does_not_buffer_output(self, _mock_installed): + """subprocess.run must NOT be called with capture_output=True. + + capture_output=True buffers the entire child output in memory before + the parent can write anything. For tools that produce large or + long-running output, this is a UX regression: the user sees nothing + until the tool finishes, and memory usage grows unbounded. + """ + with mock.patch("devforge.cli.subprocess.run") as mock_run: + mock_run.return_value = mock.MagicMock(returncode=0, stdout="", stderr="") + with mock.patch("devforge.cli.sys.exit"): + runner.invoke(app, ["guard", "--help"]) + + if mock_run.called: + # If subprocess.run is used, it must NOT capture output + call_kwargs = mock_run.call_args[1] if mock_run.call_args[1] else {} + assert call_kwargs.get("capture_output") is not True, ( + "dispatch uses subprocess.run(capture_output=True) which buffers " + "all output. Use subprocess.Popen or stdout=None to stream." + ) + assert call_kwargs.get("stdout") is not subprocess.PIPE, ( + "dispatch uses stdout=PIPE which buffers output. " + "Use stdout=None to inherit the parent's stdout." + ) + + @mock.patch("devforge.cli._is_tool_installed", return_value=True) + def test_dispatch_uses_popen_or_inherited_fds(self, _mock_installed): + """dispatch should use subprocess.Popen for real-time streaming, + or subprocess.run without capture (stdout=None, stderr=None). + """ + with mock.patch("devforge.cli.subprocess.Popen") as mock_popen, \ + mock.patch("devforge.cli.subprocess.run") as mock_run: + + # Set up Popen mock to simulate a successful run + mock_proc = mock.MagicMock() + mock_proc.wait.return_value = 0 + mock_popen.return_value = mock_proc + + with mock.patch("devforge.cli.sys.exit"): + runner.invoke(app, ["guard"]) + + # Either Popen was used (preferred for streaming) + # or subprocess.run was used WITHOUT capture_output + if mock_popen.called: + # Good: Popen streams by default + assert True + elif mock_run.called: + kwargs = mock_run.call_args[1] if mock_run.call_args[1] else {} + assert kwargs.get("capture_output") is not True + assert kwargs.get("stdout") is not subprocess.PIPE + else: + raise AssertionError( + "Neither subprocess.Popen nor subprocess.run was called" + ) + + @mock.patch("devforge.cli._is_tool_installed", return_value=True) + def test_dispatch_exit_code_propagates(self, _mock_installed): + """The child process exit code must propagate to the parent.""" + with mock.patch("devforge.cli.subprocess.Popen") as mock_popen: + mock_proc = mock.MagicMock() + mock_proc.wait.return_value = 42 + mock_popen.return_value = mock_proc + + with mock.patch("devforge.cli.sys.exit") as mock_exit: + runner.invoke(app, ["guard"]) + + if mock_popen.called: + # sys.exit(42) raises SystemExit; CliRunner catches it and + # may call sys.exit(0) afterward. Check that 42 was among + # the calls rather than asserting exactly one call. + exit_codes = [c.args[0] for c in mock_exit.call_args_list] + assert 42 in exit_codes, f"Expected exit code 42 in {exit_codes}" From cd9d002da4088f0ce6cc173d05ba337c8d2fd7e1 Mon Sep 17 00:00:00 2001 From: Jaixii Date: Sun, 16 Aug 2026 20:35:03 -0400 Subject: [PATCH 4/4] cowork-bot: fix ruff lint and format in test_dispatch_streaming --- tests/test_dispatch_streaming.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/tests/test_dispatch_streaming.py b/tests/test_dispatch_streaming.py index 6015cb5..a6aa45d 100644 --- a/tests/test_dispatch_streaming.py +++ b/tests/test_dispatch_streaming.py @@ -9,12 +9,9 @@ from __future__ import annotations import subprocess -import sys -from unittest import mock - -from typer.testing import CliRunner - from devforge.cli import app +from typer.testing import CliRunner +from unittest import mock runner = CliRunner() @@ -49,8 +46,7 @@ def test_dispatch_does_not_buffer_output(self, _mock_installed): "all output. Use subprocess.Popen or stdout=None to stream." ) assert call_kwargs.get("stdout") is not subprocess.PIPE, ( - "dispatch uses stdout=PIPE which buffers output. " - "Use stdout=None to inherit the parent's stdout." + "dispatch uses stdout=PIPE which buffers output. Use stdout=None to inherit the parent's stdout." ) @mock.patch("devforge.cli._is_tool_installed", return_value=True) @@ -58,9 +54,10 @@ def test_dispatch_uses_popen_or_inherited_fds(self, _mock_installed): """dispatch should use subprocess.Popen for real-time streaming, or subprocess.run without capture (stdout=None, stderr=None). """ - with mock.patch("devforge.cli.subprocess.Popen") as mock_popen, \ - mock.patch("devforge.cli.subprocess.run") as mock_run: - + with ( + mock.patch("devforge.cli.subprocess.Popen") as mock_popen, + mock.patch("devforge.cli.subprocess.run") as mock_run, + ): # Set up Popen mock to simulate a successful run mock_proc = mock.MagicMock() mock_proc.wait.return_value = 0 @@ -79,9 +76,7 @@ def test_dispatch_uses_popen_or_inherited_fds(self, _mock_installed): assert kwargs.get("capture_output") is not True assert kwargs.get("stdout") is not subprocess.PIPE else: - raise AssertionError( - "Neither subprocess.Popen nor subprocess.run was called" - ) + raise AssertionError("Neither subprocess.Popen nor subprocess.run was called") @mock.patch("devforge.cli._is_tool_installed", return_value=True) def test_dispatch_exit_code_propagates(self, _mock_installed):