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

[sohni-tagirisa]

Summary

Single-module pdd sync <basename> printed Overall status: Failed in the final summary panel but exited 0, so CI gates, shell && chains, and the agentic child runners (which key off the exit code) read a failed sync as success. Fixes #1979.

Root cause

pdd/sync_main.py aggregates per-language results, prints the Overall status: panel, and returns overall_success inside the aggregated dict. The sync command in pdd/commands/maintenance.py then did return str(result), total_cost, model_name unconditionally — the return value of a click command in standalone mode does not set the exit code, so the process exited 0. Downstream tooling had already grown a workaround (the runner's sticky stdout scrape of Overall status: ... Failed, pdd/agentic_sync_runner.py:2806).

Fix

Follow the established convention in the same command (#1677 AmbiguousModuleError handler; the agentic/global dispatch helpers _run_agentic_sync_dispatch / _run_global_sync_dispatch already do exactly this on failure):

• After the evidence-manifest write, check the aggregated result and raise click.exceptions.Exit(1) when overall_success == False.
• Widen the single-module path's re-raise clause from except click.Abort to except (click.Abort, click.exceptions.Exit) — Exit subclasses RuntimeError, so the existing broad except Exception handler would otherwise swallow the new raise and turn it back into exit 0 (the dispatch helpers already carry this exact guard).

Deliberately unchanged:

• The Overall status: stdout contract (pdd/sync_main.py untouched) — the runner's sticky capture and pdd/server/jobs.py:684 scrape keep working.
• The issue-URL agentic dispatch path, which already exits non-zero. The child runner computes success = exit_code == 0 and then downgrades on failure markers, so a child that now exits 1 and prints the marker still yields a single failure result — no double-fire.
• pdd/server/click_executor.py maps a raised Exit to exit_code=1 via its generic handler, so the server path reports failure correctly too.
• Cost-tracking interaction: track_cost skips the CSV row when the wrapped command raises — this is the pre-existing, accepted behavior for the pdd sync should resolve modules by canonical path, not ambiguous short basenames #1677 handler and both dispatch helpers' Exit(1) failure paths; the raise here is placed after the evidence-manifest write so --evidence failure runs still record their manifest.

Sibling findings (noted, not changed here)

• _run_agentic_sync_dispatch and _run_global_sync_dispatch already raise click.exceptions.Exit(1) on failure — the single-module path was the only sync path missing it.
• The generic except Exception handler in sync (and the same shape in many other commands) calls handle_error(...) and then return None, i.e. an unexpected crash also exits 0. handle_error only prints/records. That is a repo-wide convention across command modules, not the one-line mistake fixed here, so it is out of scope for this PR — worth its own issue.
• grep -rn overall_success pdd/commands/ shows no other command consuming a success flag without exiting on it.

Test plan

New tests: tests/test_issue_1979_sync_exit_code.py (CliRunner against the real CLI group, sync_main stubbed to return the real aggregated-dict shape):

• test_cli_sync_failed_overall_status_exits_nonzero[default|quiet] — failed sync exits non-zero, incl. --quiet
• test_cli_sync_successful_result_exits_zero
• test_cli_sync_dry_run_success_exits_zero
• test_cli_sync_dry_run_failure_exits_nonzero
• test_cli_sync_failure_still_writes_evidence_manifest — the raise happens after the evidence write

All 4 failure-path tests were committed red first (see the test(sync): commit and the red-output PR comment), then went green with the fix. Also ran tests/test_issue_1677_canonical_module_resolution.py, tests/test_commands_maintenance.py, tests/test_gate_failed_sync.py, tests/test_jobs_sync_failure_detection.py, tests/test_issue_1714_sync_stall.py, tests/test_force_local_issue_sync.py — all pass. python3 -m pylint pdd/commands/maintenance.py introduces no new warnings (remaining W0718s are pre-existing handlers).

E2E (real console entrypoint)

Minimal temp project (one prompts/hello_python.prompt + .pddrc), network blackholed via an unreachable proxy so the generate step deterministically fails:

env -u OPENAI_API_KEY -u ANTHROPIC_API_KEY \
  HTTPS_PROXY=http://127.0.0.1:9 HTTP_PROXY=http://127.0.0.1:9 \
  PDD_AUTO_UPDATE=false PYTHONPATH=<worktree> \
  python3 -m pdd.cli --local --no-core-dump sync hello --max-attempts 1 --budget 0.01; echo $?

• Failure run: prints Overall status: Failed, exit code 1 (was 0 before the fix).
• Success run: python3 -m pdd.cli --local --no-core-dump sync hello --dry-run; echo $? on the same project reports and exits 0.

Full transcript in the PR comments.

Part of epic PR #1981.

🤖 Generated with Claude Code

[sohni-tagirisa]

Pre-fix investigation summary

Convention followed (all in pdd/commands/maintenance.py):

• pdd sync should resolve modules by canonical path, not ambiguous short basenames #1677 AmbiguousModuleError handler in the same sync command: print the error, then raise click.exceptions.Exit(1) so quiet runs and agentic child runners never read failure as success (commit 0f9b4b653, tests in tests/test_issue_1677_canonical_module_resolution.py::test_cli_sync_ambiguous_exits_nonzero — the CliRunner-against-real-CLI pattern the new tests copy).
• _run_agentic_sync_dispatch and _run_global_sync_dispatch already do if not success: raise click.exceptions.Exit(1) and carry an except (click.Abort, click.exceptions.Exit): raise guard above their broad except Exception. The single-module path had neither — and its try only re-raised click.Abort, so a naive Exit(1) would have been swallowed by except Exception and turned back into exit 0. The fix adds both pieces, matching the dispatch helpers exactly.

Sibling hunt results:

• grep -rn overall_success pdd/commands/ — only the sync command consumes it; no other command family has the same flag-returned-but-exit-0 shape.
• The two dispatch paths (issue-URL agentic, global no-basename) already exit non-zero on failure; single-module was the only gap, matching the issue report.
• Out-of-scope observation: the generic except Exception handler in sync (and the same handle_error(...); return None shape across many command modules) means an unexpected crash also exits 0 — handle_error only prints/records. That is a repo-wide convention, not the one-line mistake in this command family, so it is noted here for a follow-up issue rather than changed in this PR.

Interaction checks:

• track_cost (pdd/track_cost.py): catches the raised exception, skips the cost-CSV row, re-raises. Identical to the pre-existing behavior of the pdd sync should resolve modules by canonical path, not ambiguous short basenames #1677 handler and both dispatch helpers' failure raises. The new raise is placed after _write_sync_evidence_manifest, so --evidence failure runs still write their manifest (covered by test_cli_sync_failure_still_writes_evidence_manifest).
• pdd/agentic_sync_runner.py child runner: success = exit_code == 0, then downgraded by the sticky Overall status: ... Failed marker. A child that now exits 1 and prints the marker produces a single failure result — no double-fire. Stdout contract untouched (pdd/sync_main.py not modified), so the sticky capture and pdd/server/jobs.py:684 scrape keep working.
• pdd/server/click_executor.py maps a raised Exit to exit_code=1 in its generic handler — server jobs report failure correctly.

[sohni-tagirisa]

Red test output (TDD step 1 — tests committed and pushed in 3db1db6 before the fix)

Command: python3 -m pytest tests/test_issue_1979_sync_exit_code.py -vv with pdd/commands/maintenance.py at the red commit (pre-fix).

============================= test session starts ==============================
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_failed_overall_status_exits_nonzero[default] FAILED [ 16%]
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_failed_overall_status_exits_nonzero[quiet] FAILED [ 33%]
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_successful_result_exits_zero PASSED [ 50%]
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_dry_run_success_exits_zero PASSED [ 66%]
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_dry_run_failure_exits_nonzero FAILED [ 83%]
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_failure_still_writes_evidence_manifest FAILED [100%]
=================================== FAILURES ===================================
>       assert result.exit_code != 0, (
E       AssertionError: failed sync must exit non-zero (args=[]); output:
E       assert 0 != 0
E        +  where 0 = <Result okay>.exit_code
/Users/sohnitagirisa/.local/state/agent-worktrees/promptdriven__pdd/fix-1979/tests/test_issue_1979_sync_exit_code.py:55: AssertionError
>       assert result.exit_code != 0, (
E       AssertionError: failed sync must exit non-zero (args=['--quiet']); output:
E       assert 0 != 0
E        +  where 0 = <Result okay>.exit_code
/Users/sohnitagirisa/.local/state/agent-worktrees/promptdriven__pdd/fix-1979/tests/test_issue_1979_sync_exit_code.py:55: AssertionError
>       assert result.exit_code != 0, result.output
E       AssertionError: Complete onboarding with `pdd setup` to install tab completion and configure API
E       assert 0 != 0
E        +  where 0 = <Result okay>.exit_code
/Users/sohnitagirisa/.local/state/agent-worktrees/promptdriven__pdd/fix-1979/tests/test_issue_1979_sync_exit_code.py:94: AssertionError
>       assert result.exit_code != 0, result.output
E       AssertionError: Complete onboarding with `pdd setup` to install tab completion and configure API
E       assert 0 != 0
E        +  where 0 = <Result okay>.exit_code
/Users/sohnitagirisa/.local/state/agent-worktrees/promptdriven__pdd/fix-1979/tests/test_issue_1979_sync_exit_code.py:113: AssertionError
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_failed_overall_status_exits_nonzero[default]
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_failed_overall_status_exits_nonzero[quiet]

All 4 failure-path tests red — exit_code was 0 despite the failed aggregated result — and the 2 success-path tests green, confirming the bug at the CLI level before any product change. With the fix commit (1437061) applied, all 6 pass.

[sohni-tagirisa]

E2E evidence (real console entrypoint, post-fix)

Minimal temp project: .pddrc (prompts_dir prompts, generate_output_path src/) + prompts/hello_python.prompt. Entrypoint per pyproject.toml (pdd = "pdd.cli:cli"), driven as python3 -m pdd.cli with PYTHONPATH pinned to the fix branch checkout.

Failure run (generate step deterministically fails: no API keys, network blackholed via unreachable proxy)

$ env -u OPENAI_API_KEY -u ANTHROPIC_API_KEY \
    HTTP_PROXY=http://127.0.0.1:9 HTTPS_PROXY=http://127.0.0.1:9 \
    PDD_AUTO_UPDATE=false PYTHONPATH=<fix-branch-checkout> \
    python3 -m pdd.cli --local --no-core-dump sync hello --max-attempts 1 --budget 0.01; echo "exit=$?"
2026-07-10 13:47:30,621 - pdd.llm_invoke - ERROR - [FATAL] All candidate models failed. Last error (InternalServerError): litellm.InternalServerError: InternalServerError: Github_copilotException - Connection error.
ERROR: [FATAL] All candidate models failed. Last error (InternalServerError): litellm.InternalServerError: InternalServerError: Github_copilotException - Connection error.
│ python   │ Failed │    $0.0000 │ Sync failed: Exception during 'generate':   │
│ Total time: 18.84s | Total cost: $0.0000 | Overall status: Failed │
exit=1

The Overall status: Failed summary panel still prints (stdout contract unchanged) and the process now exits 1 (was 0 before the fix).

Success run (--dry-run reporting-only, same project)

$ python3 -m pdd.cli --local --no-core-dump sync hello --dry-run; echo "exit=$?"
╭───────── PDD Sync Dry Run ─────────╮
Total Estimated Cost: $0.000000
exit=0

[sohni-tagirisa]

Codex adversarial review — PR #1982 @ 1437061

Verdict: DON'T MERGE
Findings: 1 total (1 P1, 0 P2)

P1 (blocking)

• [P1] Failed sync exits before the cost tuple reaches @track_cost — pdd/commands/maintenance.py:513-526
  The new click.exceptions.Exit(1) is raised after sync_main() has already returned (result, total_cost, model_name), but before the command returns the normal (str(result), total_cost, model_name) tuple. @track_cost only writes --output-cost / PDD_OUTPUT_COST_PATH rows when the wrapped command returns normally, so failed single-module syncs now produce no cost CSV row. This breaks the cost-tracking return contract and directly affects agentic/global parent runners, which launch child pdd sync with PDD_OUTPUT_COST_PATH and parse that CSV to accumulate costs and enforce budgets. The PR fixes the exit code but silently corrupts cost accounting for exactly the failed child attempts that issue-sync needs to observe.
  Suggested fix: Preserve the normal cost tuple return path and signal nonzero exit after tracking, for example by setting a failure flag on ctx.obj and letting the result callback exit 1, or by teaching track_cost to record the known tuple before re-raising an intentional Exit.

P2 (non-blocking)

Notes

• The GitHub issue URL and no-basename dispatch branches remain before this new gate, so their routing behavior appears unchanged.
• The new focused tests pass, but they do not cover --output-cost / PDD_OUTPUT_COST_PATH, which is the missed contract here.

(Posted by the orchestrator on behalf of codex exec — sandboxed review had no network access.)

[sohni-tagirisa]

Implementer response to Codex review

Finding: raising click.exceptions.Exit(1) skips the track_cost cost-CSV row for failed syncs, breaking the agentic runner's per-child cost accumulation and --budget enforcement (pdd/agentic_sync_runner.py:2398 sets PDD_OUTPUT_COST_PATH and parses the row) — Status: FIXING.

Plan: keep the non-zero exit (the raise must stay in the command — the server's click_executor invokes commands directly via ctx.invoke, bypassing any group-level result hook, and child processes get their exit code from click standalone mode, so option (a) would regress the server path back to exit 0); instead, stash the (str(result), total_cost, model_name) tuple on ctx.obj under a key owned by track_cost right before raising, and teach track_cost to write the CSV row from that stashed tuple when the captured exception is an intentional click.exceptions.Exit — failing test first (failed sync with PDD_OUTPUT_COST_PATH must produce exactly one CSV row AND exit non-zero), success path still writes exactly one row, #1677 and the dispatch paths (no stash) unchanged.

[sohni-tagirisa]

Implementer response — P1 fixed

Finding: Exit(1) skipped the track_cost CSV row for failed syncs, breaking child-sync cost accumulation / --budget enforcement — Status: FIXED (test commit 181283082, fix commit f640b57f5).

Mechanism chosen (option b from the plan above): the sync command stashes (str(result), total_cost, model_name) on ctx.obj under track_cost.EXIT_COST_RESULT_KEY immediately before raising click.exceptions.Exit(1); the @track_cost wrapper pops the stash unconditionally in its finally (mirroring the attempted_models anti-leak handling) and writes the CSV row from it only when the captured exception is an intentional click.exceptions.Exit. Option (a) — returning normally and exiting from a group-level result hook — was rejected because the server's click_executor invokes commands directly via ctx.invoke (bypassing group hooks), so it would have regressed the server path back to exit 0.

Preserved: success path writes exactly one row (guard test); #1677 AmbiguousModuleError and the agentic/global dispatch failure raises carry no stash, so their row-skipping semantics are byte-for-byte unchanged; crashes/other exceptions skip the row as before.

Red (test pushed first, 181283082)

tests/test_issue_1979_sync_exit_code.py::test_cli_sync_failure_still_writes_cost_csv_row FAILED
E       AssertionError: failed sync must still write the cost CSV row
tests/test_issue_1979_sync_exit_code.py::test_cli_sync_success_writes_exactly_one_cost_csv_row PASSED

(The new tests monkeypatch.delenv("PYTEST_CURRENT_TEST") so the row-writer's pytest guard doesn't mask the behavior, and disable the duplicate-run guard / auto-update that activate without it.)

Green (after f640b57f5)

python3 -m pytest tests/test_issue_1979_sync_exit_code.py tests/test_track_cost.py -vv → 37 passed (8 issue tests incl. both new ones + full track_cost suite). Regression: test_issue_1677_canonical_module_resolution.py, test_commands_maintenance.py, test_gate_failed_sync.py, test_jobs_sync_failure_detection.py → 90 passed. Pylint on both touched modules: no new findings (W0612 end_time in track_cost.py is pre-existing, verified against the pre-change tree).

E2E (real entrypoint, same blackholed-network failure project as the earlier transcript, now with PDD_OUTPUT_COST_PATH set)

$ env ... PDD_OUTPUT_COST_PATH=.../cost_e2e.csv python3 -m pdd.cli --local --no-core-dump sync hello --max-attempts 1 --budget 0.01; echo "EXIT=$?"
│ Total time: 0.71s | Total cost: $0.0000 | Overall status: Failed │
EXIT=1
$ cat cost_e2e.csv
timestamp,model,command,cost,input_files,output_files,attempted_models,...
2026-07-10T15:56:54.901,unknown,sync,0.0,,,github_copilot/claude-opus-4.6-fast;github_copilot/claude-opus-4.5;gemini/gemini-3.1-pro-preview;...

Failed sync now exits 1 and writes the cost row (cost 0.0 here because every model attempt failed at the connection layer before billing — the row itself, including attempted_models, is what the runner's parser consumes).

[sohni-tagirisa]

Codex adversarial review — PR #1982 @ f640b57

Verdict: MERGE
Findings: 2 total (0 P1, 2 P2)

P1 (blocking)

• None.

P2 (non-blocking)

• [P2] Failed sync leaks core-dump stream capture in embedded top-level invocations — pdd/commands/maintenance.py:530-531
  The new single-module failure now raises click.exceptions.Exit(1) while core dump is enabled by default. That exception path bypasses the root process_commands() callback, and PDDCLI.invoke's except click.exceptions.Exit branch re-raises without restoring _stdout_capture / _stderr_capture. In a normal one-shot CLI process this dies with the process, but cli.main(..., standalone_mode=False) / other embedded top-level invocations are left with sys.stdout as OutputCapture, which pollutes later in-process commands/tests.
  Suggested fix: Restore captured streams in the root except click.exceptions.Exit branch before re-raising, or route intentional non-zero exits through the same cleanup helper used by the other exit/error paths.

• [P2] Cost-CSV tests remove the global pytest marker and re-enable unrelated runtime behavior — tests/test_issue_1979_sync_exit_code.py:120-128
_enable_cost_csv() deletes PYTEST_CURRENT_TEST process-wide to make track_cost write CSV rows; that also turns on code intentionally suppressed under pytest, including onboarding shell detection. In this sandbox, the new tests fail unless PDD_SUPPRESS_SETUP_REMINDER=1 is externally set because shell detection attempts ps before the fake sync reaches the cost path. Existing track_cost tests avoid this by patching os.environ.get narrowly rather than removing the env var globally.
  Suggested fix: Patch only the track_cost pytest guard or add a test-only bypass for cost CSV writing, and suppress the setup reminder explicitly if these tests keep invoking the full CLI.

Notes

• The new stash mechanism itself looks sound for leak-across-commands and double-write cases: it pops the key in finally, only consumes it for click.exceptions.Exit, and leaves normal returns using the original result tuple.
• Issue URL and global dispatch paths are unchanged. Failed child syncs should still be treated as failed attempts while cost is parsed from the temp CSV.

(Round-2 re-review; posted by the orchestrator on behalf of codex exec.)

[sohni-tagirisa]

Implementer response to Codex review (round 2, 2 P2)

P2#1: failed sync leaks core-dump OutputCapture onto sys.stdout/sys.stderr in embedded top-level invocations (the except click.exceptions.Exit branch in PDDCLI.invoke re-raises non-zero Exits without restoring captured streams) — Status: FIXING.
Plan: call the existing _restore_captured_streams(ctx) (pdd/core/cli.py:93) before the non-zero re-raise in that branch — failing test first (drive a failed sync through cli.main(..., standalone_mode=False) with --core-dump and assert sys.stdout/sys.stderr are the original streams afterwards); the SystemExit branch above already restores inline and stays untouched; this also heals the pre-existing #1677 / checkup Exit paths that shared the leak.

P2#2: _enable_cost_csv() deletes PYTEST_CURRENT_TEST process-wide, re-enabling onboarding shell detection (ps) so the two cost-CSV tests are not hermetic in sandboxes without PDD_SUPPRESS_SETUP_REMINDER set externally — Status: FIXING.
Plan: make the helper hermetic by additionally setting PDD_SUPPRESS_SETUP_REMINDER=1 via monkeypatch (short-circuits _should_show_onboarding_reminder before any shell detection; the duplicate-run guard and auto-update were already disabled there), and verify with env -u PDD_SUPPRESS_SETUP_REMINDER python3 -m pytest .... Test-only change, single test(sync): commit alongside the P2#1 red test.

[sohni-tagirisa]

Implementer response — round-2 P2s fixed

P2#1 (core-dump stream capture leak on non-zero Exit) — Status: FIXED (test commit 676ff13b1 pushed before fix commit 90d655c97).
PDDCLI.invoke's except click.exceptions.Exit branch now calls the existing _restore_captured_streams(ctx) before re-raising a non-zero Exit — the same helper the ctx.exit(0) early-exit paths and duplicate-guard re-raises already use. The SystemExit branch (which restores inline) and the exit_code == 0 propagation are untouched. This also heals the pre-existing #1677 / checkup --validate-arch-includes Exit paths that shared the leak.

Red (pre-fix, commit 676ff13b1):

tests/test_issue_1979_sync_exit_code.py::test_failed_sync_restores_captured_streams_in_process FAILED
E       AssertionError: failed sync leaked the core-dump OutputCapture onto sys.stdout

The test drives a failed sync through cli.main(["--core-dump", "sync", "my_module"], standalone_mode=False) — the embedded top-level invocation shape Codex flagged (click returns the exit code for Exit there) — and asserts sys.stdout/sys.stderr are the original stream objects afterwards (with a finally restore so a red run can never poison other tests).

Green (after 90d655c97): the test passes; tests/test_core_dump.py (33 passed) and tests/core/test_cli.py + tests/test_track_cost.py + the full issue file (116 passed) all green. Pylint on pdd/core/cli.py: identical warning count (15) before and after the change — no new findings.

P2#2 (cost-CSV tests not hermetic after deleting PYTEST_CURRENT_TEST) — Status: FIXED (same test commit 676ff13b1, test-only).
_enable_cost_csv() now also sets PDD_SUPPRESS_SETUP_REMINDER=1 via monkeypatch, short-circuiting _should_show_onboarding_reminder before the shell detection (ps) that broke sandboxes. Verified hermetic:

$ env -u PDD_SUPPRESS_SETUP_REMINDER python3 -m pytest \
    tests/test_issue_1979_sync_exit_code.py::test_cli_sync_failure_still_writes_cost_csv_row \
    tests/test_issue_1979_sync_exit_code.py::test_cli_sync_success_writes_exactly_one_cost_csv_row -q
2 passed

Nothing consciously left unfixed from the round-2 review.

Note for reviewers: 5 pre-existing failures in tests/test_core_dump.py reproduce on the unmodified branch tip when the developer shell exports PDD_AUTO_UPDATE=false (they assert auto_update is called); they pass in a clean env (env -u PDD_AUTO_UPDATE) both before and after this change — unrelated to this PR.

[sohni-tagirisa]

Codex adversarial review — round 3 (final) @ 90d655c

Verdict: MERGE — no findings.

Loop summary: round 1 found 1 P1 (cost-CSV row lost on failure exit) → fixed via ctx-stash + track_cost Exit handling; round 2 verdict MERGE with 2 P2s (stream-capture leak on in-process Exit, non-hermetic cost tests) → both fixed; round 3 clean. Merging into epic/agentic-sync-default-readiness per the PR-loop runbook.

(Posted by the orchestrator on behalf of codex exec.)