fix: harden sync workflow and snapshot error handling

[dongjiang1989]

Closes: #3

sync.yml:

• Fix word-splitting bug: replace string ARGS with bash array, same issue as entrypoint.sh (PR feat: add GitHub Marketplace readiness, bug fixes, and expanded test coverage  #1 review)

sync_engine.py:

• Wrap HF snapshot fetch in try/except (was unprotected, unlike MS)
• Add explicit guard: HF_TO_MS fails clearly when HF snapshot unavailable
• Add explicit guard: BIDIRECTIONAL fails clearly when both snapshots unavailable
• Fix _update_state: handle None hf_snapshot (was causing AttributeError)
• Change exit code: only exit(1) when ALL items fail, not on partial failure

tests:

• test_both_snapshots_fail_bidirectional: both adapters fail → FAILED
• test_hf_snapshot_fails_ms_to_hf_ok: HF fails but MS_TO_HF only needs MS snapshot → SUCCESS (exposes _update_state bug that was fixed)

Summary by CodeRabbit

• Bug Fixes
  • Improved snapshot error handling: sync continues when individual snapshot fetches fail in one-way sync, while bidirectional sync now fails if required snapshots are missing.
  • Exit code behavior adjusted: process now returns a failure exit when any sync item fails.
• Tests
  • Added tests covering snapshot-fetch error scenarios and related exit behavior.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2144950b-3cb9-446c-9a71-548c9af8a8e8

📥 Commits

Reviewing files that changed from the base of the PR and between 751f9c2 and 67fa193.

📒 Files selected for processing (2)

• src/sync_engine.py
• tests/test_sync_engine_extended.py

---

📝 Walkthrough

Walkthrough

This PR hardens snapshot fetch handling by wrapping HF snapshot retrieval in a try/except and validating required snapshots per sync mode, conditions HF state writes on snapshot presence, refactors CLI exit logic to fail on any FAILED item, updates the GitHub Actions step to pass args via a Bash array, and adds tests for these error paths.

Changes

Snapshot Fetch Resilience

Layer / File(s)	Summary
Snapshot fetch error handling and validation src/sync_engine.py	Initialize hf_snapshot = None, wrap get_repo_snapshot in try/except with warning logging; for BIDIRECTIONAL raise RuntimeError if either snapshot is missing; for HF_TO_MS raise RuntimeError if HF snapshot missing; only update hf_state.last_synced_commit when hf_snapshot exists.
Exit code logic update src/sync_engine.py	Refactor CLI main() to compute any_failed boolean and call sys.exit(1) when any item result status is FAILED.
Workflow CLI argument passing .github/workflows/sync.yml	Change sync invocation to build ARGS as a Bash array (init with --config and --state-dir), append conditional flags, and invoke python -m src.sync_engine "${ARGS[@]}".
Error handling test coverage tests/test_sync_engine_extended.py	Add three tests: bidirectional with both HF/MS fetch failures -> FAILED; bidirectional with HF failure only -> FAILED with HF-specific message; ms_to_hf with HF failure but MS success -> SUCCESS.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A rabbit nibbles through the stack,
hears snapshots stumble, then adapt,
warnings whisper, arrays align,
exit codes check each trailing line,
tests hop in to seal the pact. 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: hardening the sync workflow with improved error handling for snapshot fetches and fixing word-splitting bugs.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-pre-dryrun-fail

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request updates the sync engine to handle HuggingFace snapshot fetch failures gracefully by catching exceptions, logging warnings, and raising explicit errors during bidirectional or unidirectional syncs if required snapshots are missing. It also modifies the CLI exit behavior to only return a non-zero code if all sync items fail, and adds corresponding test cases. The review feedback highlights two important issues: first, a logical bug in the bidirectional check where using 'and' instead of 'or' can lead to an AttributeError if only one snapshot is missing; second, a recommendation to revert the partial-failure exit code change to prevent silent failures in CI/CD pipelines.