diff --git a/pdd/commands/maintenance.py b/pdd/commands/maintenance.py index b8ca34378..06f5b2a97 100644 --- a/pdd/commands/maintenance.py +++ b/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 diff --git a/pdd/core/cli.py b/pdd/core/cli.py index b387b684f..e04c9a626 100644 --- a/pdd/core/cli.py +++ b/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 diff --git a/pdd/track_cost.py b/pdd/track_cost.py index 84a564923..f6340ddbc 100644 --- a/pdd/track_cost.py +++ b/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,7 +110,7 @@ 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: @@ -93,7 +118,9 @@ def wrapper(*args, **kwargs): 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: diff --git a/tests/test_issue_1979_sync_exit_code.py b/tests/test_issue_1979_sync_exit_code.py new file mode 100644 index 000000000..cd9aeb4df --- /dev/null +++ b/tests/test_issue_1979_sync_exit_code.py @@ -0,0 +1,215 @@ +"""Issue #1979: single-module `pdd sync ` 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