-
Notifications
You must be signed in to change notification settings - Fork 14
cli/sync: Detect and report errored git syncs via SyncResult #512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #512 +/- ##
==========================================
+ Coverage 80.52% 80.60% +0.08%
==========================================
Files 16 16
Lines 2192 2233 +41
Branches 454 465 +11
==========================================
+ Hits 1765 1800 +35
- Misses 277 279 +2
- Partials 150 154 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
1 similar comment
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
update_repo() across Git, Hg, and SVN sync classes now returns a SyncResult instead of None. Callers can distinguish successful syncs from failures and inspect structured error details, instead of errors being silently swallowed or raising uncaught exceptions. New dataclasses in sync/base.py: - SyncResult: tracks ok status and a list of SyncError entries - SyncError: records step name, message, and original exception GitSync.update_repo() changes: - All 13 except-CommandError-return paths now record errors in SyncResult with labeled steps (obtain, set-remotes, fetch, rebase, checkout, stash-save, stash-pop, status, etc.) - Original early-return vs continue-on-error control flow is preserved per step - Best-effort cleanup uses contextlib.suppress(CommandError) for rebase abort, stash pop, and reset operations HgSync and SvnSync updated for API consistency, wrapping obtain and pull/update failures in SyncResult. Additional fixes: - cmd/git: Fix rev_list() _all parameter shadowed by builtin - sync/git: Disambiguate rev-list with refs/heads/ for local branch names that collide with filesystem paths - sync/git: Return early on invalid-upstream rebase instead of falling through to stash-pop - sync/git: Add __str__() to GitRemoteRefNotFound to avoid AttributeError from parent CommandError.__str__() - sync/git: Add explicit _message type annotation on GitRemoteRefNotFound - libvcs/__init__: Export SyncResult and SyncError from top-level package - tests/sync: Fix hg test isolation by using factory fixture instead of deleting session-scoped remote - ruff: Move A002 suppression to per-file-ignores in pyproject.toml Test coverage: - All 13 GitSync.update_repo() error steps now have dedicated tests covering obtain, set-remotes, symbolic-ref, rev-list, remote-name, remote-ref-not-found, fetch, status, stash-save, checkout, rebase (invalid upstream + conflict), stash-pop, and submodule-update - SyncResult multi-error accumulation test - Hg and SVN obtain/pull/update failure tests - Git rev_list _all parameter test See also: vcs-python/vcspull#512
why: When libvcs GitSync.update_repo() silently swallowed errors (e.g. fetch failures), vcspull reported "✓ Synced" for repos that actually failed. With libvcs now returning SyncResult, vcspull can detect these failures. what: - Add SyncFailedError exception class - Check SyncResult from update_repo() and raise on failure - Add parametrized tests for fetch-failure, exit-on-error, and mixed good/errored repo scenarios
why: CI installs libvcs from PyPI which doesn't have the SyncResult changes yet. Tests must be skipped until the libvcs release includes it. what: - Add skipif marker checking for libvcs.sync.base.SyncResult presence
why: CI needs libvcs with SyncResult to run errored sync tests. what: - Add [tool.uv.sources] pointing libvcs at errored-sync-behavior branch - Remove HAS_SYNC_RESULT skipif guard from test_sync_errored_repo - Update uv.lock to resolve libvcs from git
…d errored sync tests why: set_remotes=True was passed unconditionally to all VCS types, crashing SVN on second sync (TypeError). Also needed errored sync test coverage for SVN, HG, and git branch mismatch scenarios. what: - Gate set_remotes=True on vcs == "git" in update_repo() - Add test_sync_rev_branch_mismatch (xfail: awaits libvcs checkout fix) - Add test_sync_errored_svn_repo (deleted remote → failure detected) - Add test_sync_errored_hg_repo (deleted remote → failure detected)
… lock why: libvcs e082804 fixed GitSync.update_repo() to pass check_returncode=True to cmd.checkout(), so branch-mismatch checkout failures now properly raise CommandError and produce SyncResult(ok=False). The xfail is no longer needed. what: - Remove @pytest.mark.xfail decorator from test_sync_rev_branch_mismatch - Update uv.lock to pin libvcs at e082804 (was 373770a)
…failure tests why: Fill test coverage gaps for errored sync behavior across scenarios. what: - Add summary-line count verification fixtures to SYNC_ERRORED_REPO_FIXTURES - Add test_sync_all_repos_fail for multi-repo all-fail git scenario - Add test_sync_cross_vcs_mixed_failure for git-ok + svn-fails scenario
…biguous branch/dir why: Two real-world bugs need regression tests before fixes: 1. Path-like patterns (~/...) produce 'No repo found for "None"' instead of showing the actual search term 2. Repos with a local branch name matching a directory name cause a fatal git rev-list ambiguity error but report "Synced" (false success) what: - Add test_sync_none_message_for_path_pattern: verifies the search term appears in output and "None" does not - Add test_sync_ambiguous_branch_dir_name: creates a repo with local branch and directory of the same name, verifies fatal error is not paired with a success message - Both tests marked xfail(strict=True) pending fixes
…h patterns why: When syncing with a path-like pattern (e.g. ~/nonexistent/) that matches no configured repo, the error message showed "None" instead of the actual search term, because only the path variable was set while name stayed None. what: - Use fallback chain (name or path or vcs_url or repo_pattern) when formatting NO_REPOS_FOR_TERM_MSG - Remove xfail from test_sync_none_message_for_path_pattern
…_name why: The ambiguous branch/dir rev-list bug is fixed in libvcs via refs/heads/ disambiguation, so the test now passes. what: - Remove xfail marker from test_sync_ambiguous_branch_dir_name
…ummary why: When `vcspull sync my_repo not_in_config` runs, the "not_in_config" pattern produces a "no repo found" log message but the summary says "0 failed". Unmatched patterns should count as failures in the summary. what: - Add test_sync_unmatched_pattern_counts_in_summary with xfail(strict=True) - Asserts summary contains "1 failed" and "1 synced"
why: When `vcspull sync my_repo not_in_config` runs, the "not_in_config" pattern produces a "no repo found" message but the summary says "0 failed". This is confusing because the user can't tell at a glance that a pattern didn't match any configured repository. what: - Track unmatched_count before the repo_patterns loop - Emit error-styled "✗ No repo found…" message for unmatched patterns - Seed summary["failed"] and summary["total"] with unmatched_count - Remove OutputMode.HUMAN gate on the log.info call - Remove xfail from test_sync_unmatched_pattern_counts_in_summary
why: With the refs/heads/ disambiguation fix in libvcs, the ambiguous branch/directory name scenario (e.g. branch "notes" + directory "notes/") is resolved. The test should now assert success instead of checking for the contradiction. what: - Update docstring to describe this as a regression test - Assert no fatal error appears in output - Assert sync succeeds with "1 synced, 0 failed" - Remove unused first_result variable assignment - Clean up debug instrumentation
why: Codify the disciplined TDD loop used for bug reproduction and fixing into a reusable Claude Code slash command invocable via /tdd-fix. what: - Add .claude/commands/tdd-fix.md with 6-phase workflow: understand bug, write xfail test, find root cause, fix, remove xfail, recovery loop - Include cross-repo (vcspull+libvcs) workflow and stale install warnings - Reference project conventions from AGENTS.md throughout
…log message why: When an unmatched pattern is synced, the user sees two lines: one from log.info() and one from formatter.emit_text(). Only the styled message should appear. what: - Add test_sync_unmatched_pattern_no_duplicate_log with xfail - Assert the "No repo found" message appears exactly once in stdout
…ebug
why: The unmatched-pattern and failed-sync code paths both call
log.info() AND formatter.emit_text(), producing duplicate messages
visible to the user. The log.info() output is a plain-text duplicate
of the styled formatter output.
what:
- Change log.info(NO_REPOS_FOR_TERM_MSG) to log.debug() at line 662
- Change log.info("Failed syncing") to log.debug() at line 764
- formatter.emit_text() remains the sole user-facing output path
why: Fix verified — log.info() downgraded to log.debug() eliminates the duplicate unmatched-pattern message. what: - Remove @pytest.mark.xfail(strict=True) from test - Update docstring to describe as regression test - Move import re to module level
…mbiguation why: uv.lock pinned libvcs to commit e082804 (before the refs/heads/ fix at b9b14a4), so uv run silently overwrote any manual editable install with the old version on every invocation. what: - Run uv lock --upgrade-package libvcs to pick up remote HEAD (34a9ea5)
why: create_git_remote_repo() without post_init creates empty repos (no commits). The new libvcs error handling correctly reports rev-list HEAD failure on empty repos, causing 8 test failures. what: - Import git_remote_repo_single_commit_post_init from libvcs - Pass it as remote_repo_post_init to all create_git_remote_repo() calls - Update uv.lock to include libvcs _all parameter fix
why: When every pattern is unmatched (total_repos == 0, unmatched_count > 0), the early return skipped summary emission. JSON/NDJSON consumers got no machine-readable output; humans saw no summary line. what: - Emit summary event and human-readable summary before early return - Add test for all-patterns-unmatched scenario
…summary emission why: The summary emit logic (structured event + human-readable text) was duplicated across three call sites, risking format drift. what: - Extract _emit_summary(formatter, colors, summary) helper - Replace all three call sites with the helper - Exit-on-error path now also emits human-readable summary
f4483ac to
999bc42
Compare
…neration why: Manually writing CHANGES entries from branch commits is tedious and error-prone; this automates the categorization and formatting. what: - 5-phase workflow: gather context, categorize, generate, review, insert - Sandboxed tool permissions (Edit-only, no Write/commit/push) - Cross-project portable via pyproject.toml name detection - Mandatory user confirmation gate before modifying CHANGES
…rnal code review processing why: Provide a repeatable workflow for validating and applying external code review feedback as separate atomic commits with quality gate enforcement. what: - Add review-feedback.md slash command accepting pasted feedback as argument - Validate each feedback point before applying (user confirmation gate) - Apply one commit per valid point with full quality gates between each
… sync summary why: Address code review feedback — summary["total"] started as unmatched_count and was labeled "repos", making the count inaccurate for JSON/NDJSON consumers and misleading in human output. what: - Initialize total to 0 (incremented per actual repo), not unmatched_count - Add separate "unmatched" key to summary dict for pattern misses - Display "N unmatched" in human-readable summary only when non-zero - Update tests to assert on "unmatched" instead of inflated "failed" count
why: Address code review feedback — test_sync accepted expected_in_err, expected_not_in_err, and expected_exit_code parameters but never asserted them, allowing regressions to slip through. what: - Replace contextlib.suppress(SystemExit) with explicit try/except to capture exit code - Assert exit_code matches expected_exit_code - Check stderr (result.err) against expected_in_err and expected_not_in_err - Check stdout (result.out) against expected_in_out and expected_not_in_out separately
…ssages why: Address code review feedback — SyncFailedError produced bare error strings without repo name/URL context, making debugging difficult when the exception propagates beyond the immediate caller. what: - Add repo_name and errors attributes to SyncFailedError - Format message as "Sync failed for <name>: <errors>" - Handle empty errors gracefully (omit colon + detail) - Pass repo name from repo_dict at the raise site
…mary-only mode why: Address code review feedback — unmatched-pattern "No repo found..." messages were emitted before rendering decisions, defeating the purpose of --summary-only which should produce minimal output. what: - Gate unmatched-pattern emit_text behind "not summary_only" check - Keep debug log unconditional for troubleshooting
why: Address code review feedback — previewed count is always 0 in actual sync since dry_run returns early, adding noise to the summary. what: - Show previewed count only when non-zero in human-readable summary - Keep previewed key in JSON/NDJSON output for schema stability - Reorder summary parts: repos, synced, failed, then optional previewed/unmatched
…ation why: Address code review feedback — tests wrote config via raw yaml.dump/write_text instead of the project's write_config helper from tests.helpers, inconsistent with project conventions. what: - Import write_config from tests.helpers - Replace all yaml_config.write_text(yaml.dump(...)) with write_config() - Covers all 18 config-writing sites in test_cli.py
…h --exit-on-error why: When --exit-on-error is set and patterns don't match any repo, the process exits 0. Automation scripts expect non-zero on any problem. what: - Add exit-on-error check in early return path (all patterns unmatched) - Add exit-on-error check after sync loop (mixed matched/unmatched) - Add parametrized test for all-unmatched and mixed-unmatched cases
…hens why: Non-portable Unicode em-dash characters (U+2014) in a developer-facing command file can cause issues in limited-Unicode environments. what: - Replace all 10 em-dash occurrences with double hyphens
…tput why: summary['total'] was the only uncolored count in the human-readable summary line, making it visually inconsistent with synced/failed/etc. what: - Wrap summary['total'] with colors.info() in _emit_summary
…red tests
why: test_sync_errored_svn_repo and test_sync_errored_hg_repo used raw
subprocess.run calls instead of libvcs factory fixtures, contrary to
AGENTS.md conventions.
what:
- Replace subprocess SVN setup with create_svn_remote_repo factory fixture
- Replace subprocess HG setup with create_hg_remote_repo factory fixture
- Remove manual @pytest.mark.skipif decorators from test functions
(skipping is handled by pytest.skip() in the empty_svn_repo / empty_hg_repo
fixture bodies, which propagates through the fixture dependency chain)
- Remove manual monkeypatch.setenv("HGUSER") (hgconfig fixture handles it)
- Import svn/hg post-init functions and CreateRepoPytestFixtureFn type
afe3e71 to
382213c
Compare
why: The breaking changes entry for libvcs v0.39.0 was already present, but the user-facing features, bug fixes, and test coverage notes were missing from the unreleased changelog section. what: - Add Features section: errored sync detection, exit-on-error for unmatched patterns, improved summary output - Add Bug fixes section: phantom "None" message, duplicate log lines, summary emission for all-unmatched runs - Add Tests section: errored sync, SVN/HG, cross-VCS, rev/branch mismatch, unmatched patterns, write_config refactor
Summary
SyncResult--exit-on-erroris setcolored total count)
"None"instead of theactual search term
set_remotesbeing passed to non-git VCS (SVN, HG)branch/directory test coverage
Motivation
When
vcspull syncruns, a git fetch can fail (remote unreachable,branch mismatch) but the CLI still reports
✓ Synced <project>.This happens because libvcs
GitSync.update_repo()catchesCommandErrorsilently and returnsNone. vcspull sees a normalreturn and reports success.
With libvcs v0.39.0 returning
SyncResult, vcspull can now detectthese failures and report them correctly.
Additionally, unmatched patterns (typos, stale config entries) were
silently ignored — the summary showed
0 failedeven when patternsmatched nothing. Automation scripts using
--exit-on-errorhad noway to detect this.
Design decisions
SyncResultfailuresinto a
SyncFailedErrorat theupdate_repo()wrapper level letsthe existing exception-based error handling work without changes
update_repo()returnsNone(oldlibvcs), the
result is not Noneguard prevents any behavior changeunmatchedis trackedindependently from
failed(which counts sync errors), givingusers a clear signal about config/pattern problems vs VCS failures
_emit_summaryhelper: Deduplicates three inline summary-emitsites (early return, exception path, end of loop)
Changes
src/vcspull/cli/sync.pySyncFailedErrorexception; checkSyncResult.okinupdate_repo()and raise on failureset_remotes=Truefor git (not SVN/HG)unmatched_count; add"unmatched"key to summary dict_emit_summary()helper with conditionalpreviewed/unmatched display and colored total count
"None"bug: usename or path or vcs_url or repo_patternas search termlog.infotolog.debug--summary-onlymode--exit-on-errorandunmatched_count > 0tests/test_cli.pytest_sync_errored_repo(5 parametrized cases: fetch failure,exit-on-error, mixed good/errored, summary lines)
test_sync_errored_svn_repoandtest_sync_errored_hg_repousing libvcs factory fixtures
test_sync_cross_vcs_mixed_failure(git OK + SVN fails)test_sync_all_repos_fail(multiple git repos all fail)test_sync_rev_branch_mismatch(main-only remote, rev: master)test_sync_ambiguous_branch_dir_nameregression testtest_sync_none_message_for_path_patterntest_sync_unmatched_pattern_counts_in_summarytest_sync_all_patterns_unmatched_emits_summarytest_sync_unmatched_pattern_no_duplicate_logtest_sync_unmatched_exit_on_error(2 parametrized cases)test_sync_human_output_redacts_repo_pathstest_syncto assert exit code and check stderr separatelywrite_confighelper throughoutOther files
pyproject.toml/uv.lock: libvcs 0.38.6 → 0.39.0CHANGES: Note breaking libvcs bumptests/test_sync.py: Passpost_inittocreate_git_remote_repoTest plan
fetch-failure-shows-failed— deleted remote → "Failed syncing"fetch-failure-exit-on-error—--exit-on-errorsurfaces itmixed-good-and-fetch-failure— good repo synced, bad failed-xDependencies