fix(sync): exit non-zero when single-module sync reports Overall status Failed (#1979)

pdd/commands/maintenance.py

@@ -11,7 +11,7 @@
 from ..auto_deps_main import auto_deps_main
 from ..agentic_sync import _is_github_issue_url, run_agentic_sync, run_global_sync
 from ..construct_paths import _find_pddrc_file, _load_pddrc_config
-from ..track_cost import track_cost
+from ..track_cost import EXIT_COST_RESULT_KEY, track_cost
 from ..core.errors import handle_error
 from ..sync_determine_operation import AmbiguousModuleError
 from ..core.utils import _run_setup_utility, echo_model_line
@@ -510,8 +510,27 @@ def _restore_model_default(_prev=_prev_model_default):
                 agentic_fallback=_agentic_fallback_from_sync_result(result),
                 **grounding_kwargs_from_ctx(ctx.obj),
             )
+        overall_success = (
+            bool(result.get("overall_success", True))
+            if isinstance(result, Mapping)
+            else True
+        )
+        if not overall_success:
+            # Issue #1979: the summary panel says "Overall status: Failed" but the
+            # command used to return the cost-tracking tuple unconditionally and
+            # exit 0. Follow the #1677 / dispatch-helper convention and exit
+            # non-zero so CI, shell `&&` chains, and the agentic child runners
+            # never read a failed sync as success. Raised AFTER the evidence
+            # manifest write so failure evidence is still recorded.
+            #
+            # Stash the cost tuple for @track_cost first: the wrapper writes the
+            # --output-cost / PDD_OUTPUT_COST_PATH CSV row from this stash on an
+            # intentional Exit, so failed attempts keep their cost row (the
+            # agentic runner parses it to accumulate cost and enforce --budget).
+            ctx.obj[EXIT_COST_RESULT_KEY] = (str(result), total_cost, model_name)
+            raise click.exceptions.Exit(1)
         return str(result), total_cost, model_name
-    except click.Abort:
+    except (click.Abort, click.exceptions.Exit):
         raise
     except AmbiguousModuleError as exc:
         # Issue #1677: an ambiguous module name is a hard, actionable error. Always

pdd/core/cli.py

@@ -432,6 +432,13 @@ def invoke(self, ctx):
             # Intentional failure (e.g. checkup --validate-arch-includes, failed sync): do not
             # route through handle_error — that misreports "unexpected error" after
             # the command already printed diagnostics.
+            #
+            # Restore any core-dump OutputCapture before re-raising: this path
+            # bypasses process_commands(), and embedded top-level invocations
+            # (cli.main(standalone_mode=False), the in-process server executor)
+            # would otherwise be left with sys.stdout/sys.stderr still wrapped
+            # for the rest of the process (issue #1979 review follow-up).
+            _restore_captured_streams(ctx)
             raise
         except Exception as e:
             # Handle all other exceptions

pdd/track_cost.py

@@ -24,6 +24,16 @@
 ]
 _FULL_FIELDNAMES = _ATTEMPT_FIELDNAMES + _PROVENANCE_FIELDNAMES
 
+# ctx.obj key under which a tracked command may stash its
+# `(str(result), total_cost, model_name)` tuple right before raising an
+# intentional `click.exceptions.Exit` (e.g. `pdd sync` exiting 1 when the
+# aggregated result has overall_success == False, issue #1979). The wrapper
+# below writes the cost-CSV row from this stash so a failed-but-completed run
+# keeps its row — the agentic runner parses PDD_OUTPUT_COST_PATH from child
+# syncs to accumulate cost and enforce --budget. Crashes and other exceptions
+# still skip the row as before.
+EXIT_COST_RESULT_KEY = 'track_cost_exit_result'
+
 
 def looks_like_file(path_str) -> bool:
     """Check if string looks like a file path."""
@@ -73,6 +83,21 @@ def wrapper(*args, **kwargs):
         finally:
             end_time = datetime.now()
 
+            # Pop any stashed exit-result unconditionally so it can never leak
+            # into a later tracked command sharing the same ctx.obj. It only
+            # feeds the row when the command raised an intentional click Exit.
+            stashed_exit_result = None
+            try:
+                if ctx.obj is not None and isinstance(ctx.obj, dict):
+                    stashed_exit_result = ctx.obj.pop(EXIT_COST_RESULT_KEY, None)
+            except Exception:
+                stashed_exit_result = None
+            exit_result = (
+                stashed_exit_result
+                if isinstance(exception_raised, click.exceptions.Exit)
+                else None
+            )
+
             try:
                 input_files, output_files = collect_files(args, kwargs)
 
@@ -85,15 +110,17 @@ def wrapper(*args, **kwargs):
                                 files_set.add(abs_path)
                     ctx.obj['core_dump_files'] = files_set
 
-                if exception_raised is None:
+                if exception_raised is None or exit_result is not None:
                     if ctx.obj and hasattr(ctx.obj, 'get'):
                         output_cost_path = ctx.obj.get('output_cost') or os.getenv('PDD_OUTPUT_COST_PATH')
                     else:
                         output_cost_path = os.getenv('PDD_OUTPUT_COST_PATH')
 
                     if output_cost_path and os.environ.get('PYTEST_CURRENT_TEST') is None:
                         command_name = ctx.command.name
-                        cost, model_name = extract_cost_and_model(result)
+                        cost, model_name = extract_cost_and_model(
+                            result if exception_raised is None else exit_result
+                        )
 
                         attempted_models_list = ctx.obj.get('attempted_models') if ctx.obj and isinstance(ctx.obj, dict) else None
                         if not attempted_models_list:

tests/test_issue_1979_sync_exit_code.py

@@ -0,0 +1,215 @@
+"""Issue #1979: single-module `pdd sync <basename>` must exit non-zero when the
+aggregated result reports `overall_success == False`.
+
+Before this fix, the sync command printed `Overall status: Failed` in the final
+summary panel but returned the `(str(result), total_cost, model_name)` cost-tracking
+tuple unconditionally, so CI, shell `&&` chains, and the agentic child runners
+(which key off the exit code) read a failed sync as success.
+
+Convention: the #1677 AmbiguousModuleError handler and the agentic/global dispatch
+helpers in the same command raise `click.exceptions.Exit(1)` on failure.
+"""
+
+import csv
+import sys
+
+import pytest
+from click.testing import CliRunner
+
+import pdd.cli  # noqa: F401 - registers commands (incl. sync) on the core CLI
+from pdd.core.cli import cli as real_cli
+
+
+def _fake_sync_main(*, overall_success: bool):
+    """Build a sync_main stand-in returning an aggregated result dict shaped like
+    the real `pdd/sync_main.py` return value (see sync_main.py ~line 1391)."""
+
+    def fake(ctx, basename, **kwargs):  # noqa: ARG001 - mirrors sync_main call shape
+        aggregated = {
+            "overall_success": overall_success,
+            "results_by_language": {
+                "python": {
+                    "success": overall_success,
+                    "error": None if overall_success else "generate step failed",
+                    "total_cost": 0.0123,
+                    "model_name": "test-model",
+                }
+            },
+            "total_cost": 0.0123,
+            "primary_model": "test-model",
+        }
+        return aggregated, 0.0123, "test-model"
+
+    return fake
+
+
+@pytest.mark.parametrize("extra", [[], ["--quiet"]], ids=["default", "quiet"])
+def test_cli_sync_failed_overall_status_exits_nonzero(tmp_path, monkeypatch, extra):
+    """A single-module sync whose aggregated result has overall_success == False
+    must exit non-zero (issue #1979), including under --quiet."""
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        "pdd.commands.maintenance.sync_main", _fake_sync_main(overall_success=False)
+    )
+
+    result = CliRunner().invoke(
+        real_cli, ["--no-core-dump", *extra, "sync", "my_module"]
+    )
+    assert result.exit_code != 0, (
+        f"failed sync must exit non-zero (args={extra}); output:\n{result.output}"
+    )
+
+
+def test_cli_sync_successful_result_exits_zero(tmp_path, monkeypatch):
+    """A successful single-module sync must keep exiting 0."""
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        "pdd.commands.maintenance.sync_main", _fake_sync_main(overall_success=True)
+    )
+
+    result = CliRunner().invoke(real_cli, ["--no-core-dump", "sync", "my_module"])
+    assert result.exit_code == 0, result.output
+
+
+def test_cli_sync_dry_run_success_exits_zero(tmp_path, monkeypatch):
+    """A successful --dry-run reporting-only run must keep exiting 0."""
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        "pdd.commands.maintenance.sync_main", _fake_sync_main(overall_success=True)
+    )
+
+    result = CliRunner().invoke(
+        real_cli, ["--no-core-dump", "sync", "my_module", "--dry-run"]
+    )
+    assert result.exit_code == 0, result.output
+
+
+def test_cli_sync_dry_run_failure_exits_nonzero(tmp_path, monkeypatch):
+    """A --dry-run whose aggregated result reports failure must also exit non-zero."""
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        "pdd.commands.maintenance.sync_main", _fake_sync_main(overall_success=False)
+    )
+
+    result = CliRunner().invoke(
+        real_cli, ["--no-core-dump", "sync", "my_module", "--dry-run"]
+    )
+    assert result.exit_code != 0, result.output
+
+
+def test_cli_sync_failure_still_writes_evidence_manifest(tmp_path, monkeypatch):
+    """The failure exit must be raised AFTER the evidence manifest write so
+    `--evidence` runs still record failure evidence (cost/manifest path)."""
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        "pdd.commands.maintenance.sync_main", _fake_sync_main(overall_success=False)
+    )
+    calls = []
+    monkeypatch.setattr(
+        "pdd.commands.maintenance._write_sync_evidence_manifest",
+        lambda **kwargs: calls.append(kwargs),
+    )
+
+    result = CliRunner().invoke(
+        real_cli, ["--no-core-dump", "sync", "my_module", "--evidence"]
+    )
+    assert result.exit_code != 0, result.output
+    assert len(calls) == 1, "evidence manifest must still be written on failure"
+    assert calls[0]["result"]["overall_success"] is False
+
+
+def _enable_cost_csv(tmp_path, monkeypatch):
+    """Route the track_cost CSV row to a temp file and neutralise the runtime
+    behaviors that key off PYTEST_CURRENT_TEST (track_cost skips row-writing
+    under pytest; the duplicate-run guard, auto-update, and onboarding shell
+    detection — which shells out to `ps` and breaks in sandboxes — activate
+    without it). Codex round-2 review: PDD_SUPPRESS_SETUP_REMINDER must be set
+    here, not inherited from the outer environment, so these tests stay
+    hermetic."""
+    csv_path = tmp_path / "cost.csv"
+    monkeypatch.delenv("PYTEST_CURRENT_TEST", raising=False)
+    monkeypatch.setenv("PDD_OUTPUT_COST_PATH", str(csv_path))
+    monkeypatch.setenv("PDD_DISABLE_DUPLICATE_GUARD", "1")
+    monkeypatch.setenv("PDD_AUTO_UPDATE", "false")
+    monkeypatch.setenv("PDD_SUPPRESS_SETUP_REMINDER", "1")
+    return csv_path
+
+
+def _read_cost_rows(csv_path):
+    with csv_path.open(newline="", encoding="utf-8") as handle:
+        return list(csv.DictReader(handle))
+
+
+def test_cli_sync_failure_still_writes_cost_csv_row(tmp_path, monkeypatch):
+    """Codex review of PR #1982: the failure exit must NOT skip the track_cost
+    CSV row. The agentic runner sets PDD_OUTPUT_COST_PATH for child syncs
+    (pdd/agentic_sync_runner.py ~2398) and parses the row to accumulate cost and
+    enforce --budget, so failed attempts must still record their cost."""
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        "pdd.commands.maintenance.sync_main", _fake_sync_main(overall_success=False)
+    )
+    csv_path = _enable_cost_csv(tmp_path, monkeypatch)
+
+    result = CliRunner().invoke(real_cli, ["--no-core-dump", "sync", "my_module"])
+
+    assert result.exit_code != 0, result.output
+    assert csv_path.is_file(), "failed sync must still write the cost CSV row"
+    rows = _read_cost_rows(csv_path)
+    assert len(rows) == 1, rows
+    assert rows[0]["command"] == "sync"
+    assert rows[0]["model"] == "test-model"
+    assert float(rows[0]["cost"]) == pytest.approx(0.0123)
+
+
+def test_cli_sync_success_writes_exactly_one_cost_csv_row(tmp_path, monkeypatch):
+    """Successful syncs must keep writing exactly ONE cost row (no double-write
+    from the failure-path plumbing)."""
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        "pdd.commands.maintenance.sync_main", _fake_sync_main(overall_success=True)
+    )
+    csv_path = _enable_cost_csv(tmp_path, monkeypatch)
+
+    result = CliRunner().invoke(real_cli, ["--no-core-dump", "sync", "my_module"])
+
+    assert result.exit_code == 0, result.output
+    rows = _read_cost_rows(csv_path)
+    assert len(rows) == 1, rows
+    assert rows[0]["command"] == "sync"
+    assert float(rows[0]["cost"]) == pytest.approx(0.0123)
+
+
+def test_failed_sync_restores_captured_streams_in_process(tmp_path, monkeypatch):
+    """Codex round-2 review of PR #1982: the non-zero `click.exceptions.Exit`
+    branch in PDDCLI.invoke must restore the core-dump `OutputCapture` streams
+    before re-raising. The server executor invokes commands in-process
+    (pdd/server/executor.py, ctx.invoke), so a failed sync with --core-dump
+    would otherwise leak OutputCapture onto sys.stdout/sys.stderr for the rest
+    of the process. Driven via cli.main(standalone_mode=False), the embedded
+    top-level invocation shape (click returns the exit code for Exit there)."""
+    from pdd.core.cli import OutputCapture  # noqa: PLC0415 - test-local import
+
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        "pdd.commands.maintenance.sync_main", _fake_sync_main(overall_success=False)
+    )
+    monkeypatch.setenv("PDD_AUTO_UPDATE", "false")
+    monkeypatch.setenv("PDD_SUPPRESS_SETUP_REMINDER", "1")
+
+    original_stdout, original_stderr = sys.stdout, sys.stderr
+    try:
+        exit_code = real_cli.main(
+            ["--core-dump", "sync", "my_module"], standalone_mode=False
+        )
+        leaked_stdout, leaked_stderr = sys.stdout, sys.stderr
+    finally:
+        # Never let a red run poison sys.stdout/sys.stderr for later tests.
+        sys.stdout, sys.stderr = original_stdout, original_stderr
+
+    assert exit_code == 1
+    assert not isinstance(leaked_stdout, OutputCapture), (
+        "failed sync leaked the core-dump OutputCapture onto sys.stdout"
+    )
+    assert leaked_stdout is original_stdout
+    assert leaked_stderr is original_stderr