Epic: agentic-sync default-readiness fixes (#1979 exit codes, #1980 scope reconciliation)#1981
Draft
sohni-tagirisa wants to merge 2 commits into
Draft
Epic: agentic-sync default-readiness fixes (#1979 exit codes, #1980 scope reconciliation)#1981sohni-tagirisa wants to merge 2 commits into
sohni-tagirisa wants to merge 2 commits into
Conversation
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 <noreply@anthropic.com>
…us Failed (#1979) (#1982) * test(sync): failing tests for single-module sync exiting 0 on Overall status Failed (#1979) Reproduces issue #1979: pdd sync <basename> 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Epic: agentic-sync default-readiness fixes
Integration branch for the two remaining correctness blockers identified by the agentic-sync E2E validation (epic #1868) before the agentic-vs-regular default sync evaluation can be trusted:
pdd sync <basename>exits non-zero whenOverall status: FailedWhy these two
Both bugs make failure look like success, which poisons any exit-code- or status-based comparison of regular vs agentic sync:
agentic_sync_runner.pysticky capture); direct invocations have nothing.Process
Each fix ships as a TDD sub-PR (failing test committed and pushed before the fix) into this branch, with an adversarial Codex review loop per
docs/runbooks/pr-loop-process.md.Do not merge this epic into
mainwithout maintainer review.🤖 Generated with Claude Code