From 05ca01c58c2185477f134cbff281ee357f4157bb Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Fri, 10 Jul 2026 13:09:35 -0700 Subject: [PATCH 1/2] chore(epic): seed epic branch for agentic-sync default-readiness fixes Epic integration branch for the fixes blocking the agentic-sync default GO/NO-GO evaluation: - #1979: pdd sync (single-module) exits 0 despite 'Overall status: Failed' - #1980: agentic sync branch-diff fast path silently under-scopes vs the issue's explicitly requested modules Sub-PRs target this branch. Do not merge this PR into main without maintainer review. Co-Authored-By: Claude Fable 5 From 263b4925f1a53b3f1cfe8a15274d2a18af842c17 Mon Sep 17 00:00:00 2001 From: Sohni Tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Sat, 11 Jul 2026 12:06:24 -0700 Subject: [PATCH 2/2] fix(sync): exit non-zero when single-module sync reports Overall status Failed (#1979) (#1982) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(sync): failing tests for single-module sync exiting 0 on Overall status Failed (#1979) Reproduces issue #1979: pdd sync prints 'Overall status: Failed' but exits 0, so CI, shell && chains, and agentic child runners read a failed sync as success. Red: 4 failure-path tests fail (exit_code == 0), 2 success-path tests pass. Co-Authored-By: Claude Fable 5 * fix(sync): exit non-zero when single-module sync reports Overall status Failed (#1979) The single-module path of the sync command returned the cost-tracking tuple (str(result), total_cost, model_name) unconditionally, so a sync whose aggregated result had overall_success == False printed 'Overall status: Failed' but exited 0. CI, shell && chains, and the agentic child runners (which key off the exit code) read the failed sync as success; the runner already grew a sticky stdout scrape as a workaround. Follow the #1677 / dispatch-helper convention in the same command: raise click.exceptions.Exit(1) on failure, placed after the evidence-manifest write so failure evidence is still recorded. Also widen the re-raise clause to (click.Abort, click.exceptions.Exit) — the existing broad 'except Exception' would otherwise swallow the Exit (it subclasses RuntimeError) and turn it back into exit 0, matching the guard the agentic/global dispatch helpers already carry. The 'Overall status:' stdout contract is unchanged (sync_main untouched); successful runs, including successful --dry-run, still exit 0. Fixes #1979 Co-Authored-By: Claude Fable 5 * test(sync): failed sync must still write the track_cost CSV row while exiting non-zero (#1979) Codex review of PR #1982 (P1): raising click.exceptions.Exit(1) makes track_cost capture the exception and skip the --output-cost / PDD_OUTPUT_COST_PATH row (it only writes when exception_raised is None). The agentic runner sets PDD_OUTPUT_COST_PATH for child syncs and parses the row to accumulate cost and enforce --budget, so failed attempts must keep recording their cost. Red: test_cli_sync_failure_still_writes_cost_csv_row fails (exit is non-zero but no CSV row is written). The companion success-path guard (exactly one row, no double-write) passes. Co-Authored-By: Claude Fable 5 * fix(sync): keep writing the track_cost CSV row when a failed sync exits non-zero (#1979) Codex review of PR #1982 (P1): the Exit(1) raised for overall_success == False was captured by @track_cost as exception_raised, and the wrapper only wrote the --output-cost / PDD_OUTPUT_COST_PATH row when no exception was raised — so exactly the failed child attempts lost their cost row, breaking the agentic runner's cost accumulation and --budget enforcement (pdd/agentic_sync_runner.py sets PDD_OUTPUT_COST_PATH per child and parses the row). Mechanism chosen: stash the (str(result), total_cost, model_name) tuple on ctx.obj under track_cost.EXIT_COST_RESULT_KEY right before raising, and teach the wrapper to write the row from that stash when the captured exception is an intentional click.exceptions.Exit. The alternative — not raising in the command and exiting from a group-level result hook — was rejected because the server's click_executor invokes commands directly via ctx.invoke (bypassing any group hook) and child processes get their exit code from click standalone mode, so only a raise inside the command covers both paths. Behavior preserved: - success path writes exactly ONE row (stash is only set on the failure raise; the wrapper pops it unconditionally so it can never leak into a later tracked command, mirroring the attempted_models handling) - #1677 AmbiguousModuleError and the agentic/global dispatch failure raises carry no stash, so they keep skipping the row as before - crashes / other exceptions keep skipping the row as before Co-Authored-By: Claude Fable 5 * test(sync): failing test for core-dump stream capture leak on failed-sync Exit; make cost-CSV tests hermetic (#1979) Codex round-2 review of PR #1982 (2 P2): P2#1 red test: a failed sync driven through cli.main(standalone_mode=False) with --core-dump must leave sys.stdout/sys.stderr as the original streams. The non-zero branch of PDDCLI.invoke's 'except click.exceptions.Exit' re-raises without _restore_captured_streams(ctx), so embedded top-level invocations (server executor uses ctx.invoke in-process) leak OutputCapture. Red: AssertionError 'failed sync leaked the core-dump OutputCapture onto sys.stdout'. P2#2 (test-only fix): _enable_cost_csv() deletes PYTEST_CURRENT_TEST process-wide, which re-enables onboarding shell detection (subprocess 'ps') — the two cost-CSV tests failed in sandboxes unless PDD_SUPPRESS_SETUP_REMINDER was set externally. Set it via monkeypatch in the helper; verified with env -u PDD_SUPPRESS_SETUP_REMINDER pytest (2 passed). Co-Authored-By: Claude Fable 5 * fix(cli): restore core-dump captured streams before re-raising non-zero Exit (#1979) Codex round-2 review of PR #1982 (P2#1): the 'except click.exceptions.Exit' branch in PDDCLI.invoke re-raised intentional non-zero exits (failed sync, #1677 ambiguous module, checkup --validate-arch-includes) WITHOUT restoring the --core-dump OutputCapture wrappers. One-shot CLI processes die anyway, but embedded top-level invocations — cli.main(standalone_mode=False) and the in-process server executor (ctx.invoke) — were left with sys.stdout/sys.stderr still wrapped for the rest of the process. Call the existing _restore_captured_streams(ctx) helper (written for exactly this, and already used by the ctx.exit(0) early-exit paths and the duplicate-guard re-raises) before the re-raise. The SystemExit branch above already restores inline and is left untouched; exit_code == 0 Exits keep propagating through Click's normal cleanup as before. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- pdd/commands/maintenance.py | 23 ++- pdd/core/cli.py | 7 + pdd/track_cost.py | 31 +++- tests/test_issue_1979_sync_exit_code.py | 215 ++++++++++++++++++++++++ 4 files changed, 272 insertions(+), 4 deletions(-) create mode 100644 tests/test_issue_1979_sync_exit_code.py diff --git a/pdd/commands/maintenance.py b/pdd/commands/maintenance.py index b8ca343784..06f5b2a970 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 b387b684fc..e04c9a6265 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 84a5649232..f6340ddbc1 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 0000000000..cd9aeb4df3 --- /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