feat: add GitHub Marketplace readiness, bug fixes, and expanded test coverage - #1
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub workflows (CI, release, sync/preflight), action input/entrypoint wiring and Docker ENTRYPOINT, emits GitHub Actions outputs from SyncEngine, supplies issue/PR templates, CHANGELOG/CONTRIBUTING docs, and applies formatting/import cleanup across code and tests. ChangesGitHub Actions Marketplace Integration
🎯 3 (Moderate) | ⏱️ ~22 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prepares the HF-MS Sync action for the GitHub Marketplace by introducing a Docker entrypoint script (entrypoint.sh), adding action inputs for state directory and API tokens, and implementing direct writing of GitHub outputs and step summaries from the Python sync engine. It also adds standard repository templates, contribution guidelines, and comprehensive tests. The review feedback highlights two important issues in entrypoint.sh: the potential for word-splitting bugs when constructing CLI arguments as a single unquoted string, and redundant code blocks that duplicate the writing of GitHub outputs and job summaries already handled by the Python engine.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
src/sync_engine.py (1)
376-378: ⚡ Quick winWrite outputs even when no sync items are selected.
Returning early on empty config/target-filter miss skips
_write_github_outputs, so downstream steps may see missing outputs instead ofsync_status=successand zero counters.Proposed fix
items = self._build_sync_items() if not items: logger.warning("No sync items configured") - return [] + results: list[SyncResult] = [] + self._write_results(results) + self._write_github_outputs(results) + return resultsAlso applies to: 394-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sync_engine.py` around lines 376 - 378, The early returns when items is empty skip writing GitHub outputs; instead of returning immediately in the blocks around the shown if-not-items checks, call _write_github_outputs (pass an empty list and zeroed counters / sync_status='success') before returning [] so downstream steps receive explicit outputs; update both occurrences (the if block at the shown snippet and the similar block at lines ~394-395) to invoke _write_github_outputs with appropriate empty/zero values and then return [].tests/test_sync_engine_extended.py (1)
547-638: ⚡ Quick winAdd a GitHub output test for the empty-config path.
TestGitHubOutputsshould includemodels=[]anddatasets=[]withGITHUB_OUTPUTset, asserting output keys are still written (sync_status=success,files_synced=0,bytes_transferred=0).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sync_engine_extended.py` around lines 547 - 638, Add a new test method in the TestGitHubOutputs class (e.g., test_github_output_empty_config) that sets GITHUB_OUTPUT to a temp file, creates hf and ms MockAdapter instances with no files, builds a config using make_config(direction="hf_to_ms", models=[], datasets=[]), constructs a SyncEngine with that config and the adapters, calls engine.sync_all(), then reads the output file and asserts the expected keys are present: "sync_status=success", "files_synced=0", "bytes_transferred=0" and that "models=[]" and "datasets=[]" are written; reference make_config, SyncEngine, and MockAdapter to locate where to add the test..github/workflows/ci.yml (1)
99-103: ⚡ Quick winGate Docker build on action validation as well.
Line 102 should include
action-validationinneedsso image build doesn’t run when action contract checks fail.Suggested fix
- needs: [lint, test] + needs: [lint, test, action-validation]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 99 - 103, The docker-build workflow job named "docker-build" currently depends on [lint, test]; update its needs configuration to also include "action-validation" so the Docker image build is gated by the action-contract checks; locate the docker-build job definition and add "action-validation" to the needs array for the job (the "docker-build" job and its "needs" key).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 4-7: CI is configured to trigger only on branch "main" for both
the push and pull_request workflows, but this PR targets "master"; update the
branches arrays under the push and pull_request keys in .github/workflows/ci.yml
(the branches: [main] entries) to include "master" (e.g., branches: [main,
master]) so CI runs for the master-targeted PR and pushes.
- Around line 17-19: Replace the floating action tags with immutable commit
SHAs: locate every usage of actions/checkout@v4 and actions/setup-python@v5 (and
any other floating tags in the CI workflow) and pin them to the corresponding
commit SHA for the desired release (e.g., actions/checkout@<commit-sha>,
actions/setup-python@<commit-sha>); update all occurrences (including the ones
referenced by actions/checkout and actions/setup-python in the workflow) and
verify the SHAs are the official commit hashes from the actions' repositories to
eliminate supply-chain risk.
- Line 17: The checkout steps use actions/checkout@v4 without disabling
credential persistence; update every actions/checkout@v4 invocation to include a
with: block setting persist-credentials: false (i.e., add "with:" then
"persist-credentials: false" under the actions/checkout@v4 steps) so credentials
are not left available to subsequent steps.
In @.github/workflows/release.yml:
- Around line 95-96: The release action is using raw inputs.version for tag_name
and name while the "Create tag" step normalizes the tag (prefixing with "v"),
causing a mismatch; update the release step to use the normalized tag output
from the tag-creation step (e.g., replace uses of inputs.version in tag_name and
name with the create-tag step's output such as steps.create_tag.outputs.tag_name
or steps.create_tag.outputs.tag) so both tag_name and name reference the exact
normalized tag produced earlier.
- Around line 28-30: Replace mutable action tags with immutable commit SHAs for
each third-party action referenced (actions/checkout@v4,
actions/setup-python@v5, softprops/action-gh-release@v2,
docker/setup-buildx-action@v3, docker/login-action@v3,
docker/metadata-action@v5, docker/build-push-action@v5): for each action, look
up the corresponding repository on GitHub, find the commit that corresponds to
the tagged release you currently reference, and update the workflow to use that
action as @<commit-sha> instead of `@vX`; keep the rest of the step configuration
unchanged and ensure you pin every occurrence of those action names to their
full SHA.
- Line 28: Current checkout steps use actions/checkout@v4 without scoping
credentials; update the checkout steps used by the validate and docker jobs to
set persist-credentials: false to avoid persisting tokens, while leaving the
release job’s checkout unchanged (release still needs the default behavior for
pushing refs). Locate the actions/checkout@v4 steps in the validate and docker
jobs and add persist-credentials: false to their step inputs; do not modify the
release job’s checkout step.
- Around line 62-66: Replace direct interpolation of ${{ inputs.version }} with
a single normalized env var (e.g., VERSION) and perform shell-safe regex
validation before any git or release steps: read inputs.version into an env (not
reusing ${{ inputs.version }} later), validate/normalize it to ensure it matches
SemVer or '^v?\\d+\\.\\d+\\.\\d+$' and prepend 'v' if missing, abort with a
clear error on mismatch; then use that validated env variable for git tag
operations and set the GitHub Release fields (tag_name and name) from this
normalized VERSION rather than the raw input. Reference: the script block that
sets VERSION, the places invoking git tag, and the Release step fields
tag_name/name.
- Around line 19-21: The workflow currently grants workflow-wide write access
via the top-level permissions keys (contents: write, packages: write); change
the global permissions to least-privilege (e.g., contents: read, packages: read)
and add explicit permissions overrides only on jobs that actually need write
(for example add a permissions block with contents: write and/or packages: write
to the release/publish job(s) that perform pushes or package uploads, leaving
jobs like validate with read-only permissions). Locate the top-level permissions
block and each affected job (e.g., the release/publish job names and the
validate job) and move write privileges from global scope into per-job
permissions overrides.
In @.github/workflows/sync.yml:
- Around line 30-33: Add explicit least-privilege permissions to the sync job by
setting a job-level permissions map (e.g., issues: write and any read perms you
actually need) so actions/github-script@v7 can list/create issues with the
GITHUB_TOKEN; pin external actions to specific commit SHAs instead of tags for
both actions/github-script and actions/checkout; and harden both
actions/checkout steps by adding persist-credentials: false to avoid leaking the
token to subsequent steps. Ensure these changes are applied to the sync job
definition (referencing job name "sync"), the actions/github-script usage, and
every actions/checkout invocation.
In `@CONTRIBUTING.md`:
- Line 50: Update the "Target Python version: 3.11" line in CONTRIBUTING.md to
reflect the supported range mentioned in the PR (e.g., "Target Python versions:
3.10–3.12"); locate the exact phrase "Target Python version: 3.11" and replace
it with a pluralized, ranged form so documentation matches the tested matrix.
- Around line 56-69: In CONTRIBUTING.md, the fenced code block that shows the
project structure (the block containing entries like src/, adapters/, config.py,
change_detector.py, tests/, examples/) is missing a language identifier; update
the opening fence for that block to include a language (e.g., add "text" or
"bash" after the triple backticks) so the snippet is
rendered/accessibility-compliant (MD040).
In `@entrypoint.sh`:
- Around line 34-42: Change ARGS from a flat string to a Bash array and append
options with ARGS+=(--config "$CONFIG") etc, then invoke the Python process with
the array expansion python -m src.sync_engine "${ARGS[@]}" to avoid
word-splitting and globbing; additionally, make the job-summary generation
resilient by replacing direct dict indexing r['item_name'] and
r['resource_type'] with safe lookups like r.get('item_name', '') and
r.get('resource_type', '') (or guard with conditional checks) so missing keys
won’t raise KeyError and break the run.
In `@src/sync_engine.py`:
- Around line 426-457: The _write_github_outputs function currently opens and
writes to the file referenced by GITHUB_OUTPUT without handling I/O errors; wrap
the file open/write block in a try/except that catches
OSError/IOError/Exception, log a warning via logger (include the exception
message and the output_file value) and return without re-raising so a file write
failure does not flip a successful sync into a failed run; keep the computation
of total_synced/total_bytes/statuses as-is and ensure logger.info is only called
for the successful write path or still logs a message indicating the write was
skipped on exception.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 99-103: The docker-build workflow job named "docker-build"
currently depends on [lint, test]; update its needs configuration to also
include "action-validation" so the Docker image build is gated by the
action-contract checks; locate the docker-build job definition and add
"action-validation" to the needs array for the job (the "docker-build" job and
its "needs" key).
In `@src/sync_engine.py`:
- Around line 376-378: The early returns when items is empty skip writing GitHub
outputs; instead of returning immediately in the blocks around the shown
if-not-items checks, call _write_github_outputs (pass an empty list and zeroed
counters / sync_status='success') before returning [] so downstream steps
receive explicit outputs; update both occurrences (the if block at the shown
snippet and the similar block at lines ~394-395) to invoke _write_github_outputs
with appropriate empty/zero values and then return [].
In `@tests/test_sync_engine_extended.py`:
- Around line 547-638: Add a new test method in the TestGitHubOutputs class
(e.g., test_github_output_empty_config) that sets GITHUB_OUTPUT to a temp file,
creates hf and ms MockAdapter instances with no files, builds a config using
make_config(direction="hf_to_ms", models=[], datasets=[]), constructs a
SyncEngine with that config and the adapters, calls engine.sync_all(), then
reads the output file and asserts the expected keys are present:
"sync_status=success", "files_synced=0", "bytes_transferred=0" and that
"models=[]" and "datasets=[]" are written; reference make_config, SyncEngine,
and MockAdapter to locate where to add the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02e7d627-afd0-4981-904d-2587e35caa67
📒 Files selected for processing (24)
.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/PULL_REQUEST_TEMPLATE.md.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/sync.ymlCHANGELOG.mdCONTRIBUTING.mdDockerfileaction.ymlentrypoint.shsrc/adapters/huggingface_adapter.pysrc/adapters/modelscope_adapter.pysrc/change_detector.pysrc/report.pysrc/sync_engine.pysrc/utils.pytests/e2e/verify_sync.pytests/test_adapters.pytests/test_change_detector.pytests/test_config.pytests/test_sync_engine.pytests/test_sync_engine_extended.pytests/test_utils.py
💤 Files with no reviewable changes (1)
- tests/test_utils.py
dongjiang1989
added a commit
that referenced
this pull request
May 29, 2026
Addresses PR #1 review from gemini-code-assist: 1. Replace string-based CLI arg building with a bash array to prevent word-splitting and glob expansion when paths contain spaces or special characters. 2. Remove the two large Python heredoc blocks that re-read last_results.json to write $GITHUB_OUTPUT and $GITHUB_STEP_SUMMARY. These are already written directly by the sync engine (_write_github_outputs / _write_results in src/sync_engine.py), so the duplication was unnecessary and risked conflicting writes.
Signed-off-by: dongjiang <dongjiang1989@126.com>
Addresses PR #1 review from gemini-code-assist: 1. Replace string-based CLI arg building with a bash array to prevent word-splitting and glob expansion when paths contain spaces or special characters. 2. Remove the two large Python heredoc blocks that re-read last_results.json to write $GITHUB_OUTPUT and $GITHUB_STEP_SUMMARY. These are already written directly by the sync engine (_write_github_outputs / _write_results in src/sync_engine.py), so the duplication was unnecessary and risked conflicting writes. Signed-off-by: dongjiang <dongjiang1989@126.com>
dongjiang1989
force-pushed
the
change-to-github-marketplace
branch
from
May 29, 2026 05:23
029ff97 to
0c0bdb6
Compare
- ci.yml: add 'master' to branch triggers so CI runs for this PR target - ci.yml: add persist-credentials: false to all checkout steps - ci.yml: gate docker-build on action-validation job - sync_engine.py: write GitHub outputs even when no items configured (prevents downstream steps from seeing missing outputs) - tests: add test_github_output_empty_config for empty config path - CONTRIBUTING.md: update Python version to 3.10-3.12, add code fence lang Signed-off-by: dongjiang <dongjiang1989@126.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/sync.yml (1)
131-145:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHarden sync workflow command construction and permissions
.github/workflows/sync.yml(Run sync step, lines 131-145) buildsARGSusing unquoted${{ inputs.* }}and then runspython -m src.sync_engine $ARGS, which allows argument splitting and potential shell injection viasync_target(since it’s interpolated into the bash script unquoted).- The workflow has no top-level/job-level
permissions:while it usesactions/github-script@v7to list/create issues on failure; with restricted org defaults, the failure-reporting step can fail.Suggested fix
- name: Run sync env: HF_TOKEN: ${{ secrets.HF_TOKEN }} MODELSCOPE_TOKEN: ${{ secrets.MODELSCOPE_TOKEN }} + INPUT_DRY_RUN: ${{ inputs.dry_run }} + INPUT_DIRECTION: ${{ inputs.direction }} + INPUT_SYNC_TARGET: ${{ inputs.sync_target }} run: | - ARGS="--config config/sync_config.yaml --state-dir .sync_state/" + ARGS=(--config "config/sync_config.yaml" --state-dir ".sync_state/") - if [ "${{ inputs.dry_run }}" = "true" ]; then - ARGS="$ARGS --dry-run true" + if [ "$INPUT_DRY_RUN" = "true" ]; then + ARGS+=(--dry-run "true") fi - if [ -n "${{ inputs.direction }}" ] && [ "${{ inputs.direction }}" != "config" ]; then - ARGS="$ARGS --direction ${{ inputs.direction }}" + if [ -n "$INPUT_DIRECTION" ] && [ "$INPUT_DIRECTION" != "config" ]; then + ARGS+=(--direction "$INPUT_DIRECTION") fi - if [ -n "${{ inputs.sync_target }}" ]; then - ARGS="$ARGS --target ${{ inputs.sync_target }}" + if [ -n "$INPUT_SYNC_TARGET" ]; then + ARGS+=(--target "$INPUT_SYNC_TARGET") fi - python -m src.sync_engine $ARGS + python -m src.sync_engine "${ARGS[@]}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sync.yml around lines 131 - 145, The ARGS construction uses unquoted interpolations (ARGS, inputs.sync_target, inputs.direction) and is passed to python via word-splitting, enabling argument-splitting/shell injection; fix by quoting all GitHub input expansions and argument interpolations (use "${{ inputs.sync_target }}", "${{ inputs.direction }}", and quote when appending to ARGS or, better, build a safe arg array and pass it to python -m src.sync_engine without unquoted expansion) so values cannot inject extra tokens, and add explicit workflow- or job-level permissions (e.g., permissions: issues: write) so actions/github-script@v7 can reliably list/create issues on failure.
♻️ Duplicate comments (7)
.github/workflows/ci.yml (2)
5-7:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCI won’t run for the current target branch.
This PR targets
master, but CI is configured formainonly.Suggested fix
on: push: - branches: [main] + branches: [main, master] pull_request: - branches: [main] + branches: [main, master]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 5 - 7, The CI config only triggers for branch name "main" under the YAML keys "branches:" for push and the "pull_request:" block, but the PR targets "master"; update the branch filters in the CI workflow so they include the target branch—either add "master" alongside "main" in both branches arrays (branches: [main, master]) or replace "main" with "master" in the push and pull_request "branches:" entries to match your repo's default branch.
17-19:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPin actions to immutable SHAs and disable persisted checkout credentials.
Using floating tags and default credential persistence weakens workflow security.
#!/bin/bash # Verify floating action refs and checkout credential persistence in CI workflow set -euo pipefail rg -nP '^\s*-\s*uses:\s*[^@\s]+@v[0-9]+' .github/workflows/ci.yml rg -n -A2 'uses:\s*actions/checkout@' .github/workflows/ci.ymlAlso applies to: 40-42, 57-57, 104-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 17 - 19, Replace floating action tags with their immutable SHA digests and disable persisted checkout credentials: update uses: actions/checkout@v4 and uses: actions/setup-python@v5 occurrences to the corresponding commit SHAs (replace the `@vX` refs with full @<sha>) and add persist-credentials: false to the checkout step (the actions/checkout step). Apply the same SHA-pinning to any other uses: actions/checkout@... and actions/setup-python@... occurrences referenced in the workflow..github/workflows/release.yml (2)
60-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize and validate version once, then reuse it everywhere.
Current flow mixes normalized (
v...) and raw values and interpolates raw input directly in shell.Suggested fix
+ - name: Normalize version + id: ver + env: + VERSION_INPUT: ${{ inputs.version }} + run: | + VERSION="$VERSION_INPUT" + [[ "$VERSION" == v* ]] || VERSION="v$VERSION" + [[ "$VERSION" =~ ^v[0-9]+\.[0-9]+\.[0-9]+([.-][0-9A-Za-z.-]+)?$ ]] || exit 1 + echo "version=$VERSION" >> "$GITHUB_OUTPUT" @@ - name: Create tag run: | - VERSION="${{ inputs.version }}" - # Ensure version starts with 'v' - if [[ ! "$VERSION" == v* ]]; then - VERSION="v${VERSION}" - fi + VERSION="${{ steps.ver.outputs.version }}" @@ - name: Update major version tag if: inputs.update_major_tag run: | - VERSION="${{ inputs.version }}" - if [[ ! "$VERSION" == v* ]]; then - VERSION="v${VERSION}" - fi + VERSION="${{ steps.ver.outputs.version }}" @@ - name: Create GitHub Release uses: softprops/action-gh-release@v2 with: - tag_name: ${{ inputs.version }} - name: ${{ inputs.version }} + tag_name: ${{ steps.ver.outputs.version }} + name: ${{ steps.ver.outputs.version }}Also applies to: 76-82, 95-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 60 - 66, Normalize and validate the incoming version once at the start of the workflow (e.g., inside the "Create tag" step) by reading inputs.version into a shell variable, prepend "v" if missing and validate the format (semantic version regex), then export that normalized value to the environment (via GITHUB_ENV) so all subsequent steps use the same canonical $VERSION; update any later steps that currently reference the raw inputs.version to use the exported $VERSION instead to avoid mixing raw and normalized values.
19-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove write permissions to the specific jobs that need them.
Workflow-level write access is broader than necessary.
Suggested fix
permissions: - contents: write - packages: write + contents: read + packages: read jobs: validate: + permissions: + contents: read @@ release: + permissions: + contents: write @@ docker: + permissions: + contents: read + packages: write🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 19 - 21, Remove broad workflow-level write permissions (the top-level permissions block granting contents: write and packages: write) and instead set workflow-level permissions to the least privilege (e.g., contents: read). For each job that actually requires write access (e.g., the job that creates/releases GH Releases or publishes packages—look for jobs named "release", "publish", "publish-package", or similar), add a job-level permissions block with the exact scopes needed (contents: write and/or packages: write). Ensure no other jobs inherit write access and verify any step using actions that require package or contents write has the corresponding job-level permissions.CONTRIBUTING.md (1)
50-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign documented target Python versions with CI support.
This line conflicts with the CI matrix tested in this PR.
Suggested fix
-- Target Python version: 3.11 +- Target Python versions: 3.10–3.12🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` at line 50, The CONTRIBUTING line "Target Python version: 3.11" conflicts with the CI matrix in this PR; update that exact string to match the CI-tested Python versions (e.g., replace the single-version entry with the list or range used in the CI matrix such as "Target Python versions: 3.10, 3.11" or the specific set from the PR) so the documented target versions align with CI.src/sync_engine.py (1)
447-450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
$GITHUB_OUTPUTwrite failures without failing the sync run.A filesystem error here can flip a successful sync into a failed action step.
Suggested fix
- with open(output_file, "a", encoding="utf-8") as f: - f.write(f"sync_status={overall}\n") - f.write(f"files_synced={total_synced}\n") - f.write(f"bytes_transferred={total_bytes}\n") + try: + with open(output_file, "a", encoding="utf-8") as f: + f.write(f"sync_status={overall}\n") + f.write(f"files_synced={total_synced}\n") + f.write(f"bytes_transferred={total_bytes}\n") + except OSError as e: + logger.warning("Failed to write GitHub outputs to %s: %s", output_file, e) + return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sync_engine.py` around lines 447 - 450, Wrap the block that writes to output_file (the lines writing sync_status={overall}, files_synced={total_synced}, bytes_transferred={total_bytes}) in a try/except that catches OSError/IOError and logs a non-fatal warning instead of allowing the exception to propagate and fail the sync run; reference the same variables (output_file, overall, total_synced, total_bytes) and use the module's logger (or logging.warning) to record the failure and the exception details, then continue without re-raising..github/workflows/sync.yml (1)
30-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit least-privilege
permissionsfor the sync job.
actions/github-scriptissue creation can fail under read-only default token permissions.Suggested fix
name: HF-MS Sync +permissions: + contents: read @@ sync: needs: preflight + permissions: + contents: read + issues: writeAlso applies to: 96-100, 189-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sync.yml around lines 30 - 33, Add an explicit least-privilege permissions block to the job(s) that call actions/github-script (e.g., the preflight/sync job) so the workflow token can create issues; inside the job definition (e.g., job name "preflight" / "sync") add a permissions map such as `permissions: contents: read issues: write` (or the minimal scopes your script needs) and mirror the same change for the other jobs referenced in the comment so they also declare explicit minimal permissions instead of relying on defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/sync.yml:
- Around line 131-145: The ARGS construction uses unquoted interpolations (ARGS,
inputs.sync_target, inputs.direction) and is passed to python via
word-splitting, enabling argument-splitting/shell injection; fix by quoting all
GitHub input expansions and argument interpolations (use "${{ inputs.sync_target
}}", "${{ inputs.direction }}", and quote when appending to ARGS or, better,
build a safe arg array and pass it to python -m src.sync_engine without unquoted
expansion) so values cannot inject extra tokens, and add explicit workflow- or
job-level permissions (e.g., permissions: issues: write) so
actions/github-script@v7 can reliably list/create issues on failure.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 5-7: The CI config only triggers for branch name "main" under the
YAML keys "branches:" for push and the "pull_request:" block, but the PR targets
"master"; update the branch filters in the CI workflow so they include the
target branch—either add "master" alongside "main" in both branches arrays
(branches: [main, master]) or replace "main" with "master" in the push and
pull_request "branches:" entries to match your repo's default branch.
- Around line 17-19: Replace floating action tags with their immutable SHA
digests and disable persisted checkout credentials: update uses:
actions/checkout@v4 and uses: actions/setup-python@v5 occurrences to the
corresponding commit SHAs (replace the `@vX` refs with full @<sha>) and add
persist-credentials: false to the checkout step (the actions/checkout step).
Apply the same SHA-pinning to any other uses: actions/checkout@... and
actions/setup-python@... occurrences referenced in the workflow.
In @.github/workflows/release.yml:
- Around line 60-66: Normalize and validate the incoming version once at the
start of the workflow (e.g., inside the "Create tag" step) by reading
inputs.version into a shell variable, prepend "v" if missing and validate the
format (semantic version regex), then export that normalized value to the
environment (via GITHUB_ENV) so all subsequent steps use the same canonical
$VERSION; update any later steps that currently reference the raw inputs.version
to use the exported $VERSION instead to avoid mixing raw and normalized values.
- Around line 19-21: Remove broad workflow-level write permissions (the
top-level permissions block granting contents: write and packages: write) and
instead set workflow-level permissions to the least privilege (e.g., contents:
read). For each job that actually requires write access (e.g., the job that
creates/releases GH Releases or publishes packages—look for jobs named
"release", "publish", "publish-package", or similar), add a job-level
permissions block with the exact scopes needed (contents: write and/or packages:
write). Ensure no other jobs inherit write access and verify any step using
actions that require package or contents write has the corresponding job-level
permissions.
In @.github/workflows/sync.yml:
- Around line 30-33: Add an explicit least-privilege permissions block to the
job(s) that call actions/github-script (e.g., the preflight/sync job) so the
workflow token can create issues; inside the job definition (e.g., job name
"preflight" / "sync") add a permissions map such as `permissions: contents: read
issues: write` (or the minimal scopes your script needs) and mirror the same
change for the other jobs referenced in the comment so they also declare
explicit minimal permissions instead of relying on defaults.
In `@CONTRIBUTING.md`:
- Line 50: The CONTRIBUTING line "Target Python version: 3.11" conflicts with
the CI matrix in this PR; update that exact string to match the CI-tested Python
versions (e.g., replace the single-version entry with the list or range used in
the CI matrix such as "Target Python versions: 3.10, 3.11" or the specific set
from the PR) so the documented target versions align with CI.
In `@src/sync_engine.py`:
- Around line 447-450: Wrap the block that writes to output_file (the lines
writing sync_status={overall}, files_synced={total_synced},
bytes_transferred={total_bytes}) in a try/except that catches OSError/IOError
and logs a non-fatal warning instead of allowing the exception to propagate and
fail the sync run; reference the same variables (output_file, overall,
total_synced, total_bytes) and use the module's logger (or logging.warning) to
record the failure and the exception details, then continue without re-raising.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18a419b0-dc93-4d8d-b0a6-78ce9536724e
📒 Files selected for processing (24)
.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/PULL_REQUEST_TEMPLATE.md.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/sync.ymlCHANGELOG.mdCONTRIBUTING.mdDockerfileaction.ymlentrypoint.shsrc/adapters/huggingface_adapter.pysrc/adapters/modelscope_adapter.pysrc/change_detector.pysrc/report.pysrc/sync_engine.pysrc/utils.pytests/e2e/verify_sync.pytests/test_adapters.pytests/test_change_detector.pytests/test_config.pytests/test_sync_engine.pytests/test_sync_engine_extended.pytests/test_utils.py
💤 Files with no reviewable changes (1)
- tests/test_utils.py
✅ Files skipped from review due to trivial changes (9)
- .github/ISSUE_TEMPLATE/bug_report.yml
- CHANGELOG.md
- src/adapters/huggingface_adapter.py
- src/change_detector.py
- tests/e2e/verify_sync.py
- src/report.py
- tests/test_sync_engine.py
- tests/test_config.py
- src/utils.py
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/ISSUE_TEMPLATE/feature_request.yml
- action.yml
- tests/test_change_detector.py
- Dockerfile
- src/adapters/modelscope_adapter.py
- tests/test_adapters.py
- tests/test_sync_engine_extended.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/sync_engine.py (1)
380-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
$GITHUB_OUTPUTwrites so reporting failures don’t fail the run.Line 380 now executes this path even for empty configs, so an
OSErrorwhile writing outputs can still fail the action step despite no sync failure. Please wrap the file write intry/except OSErrorand log a warning.Suggested fix
- with open(output_file, "a", encoding="utf-8") as f: - f.write(f"sync_status={overall}\n") - f.write(f"files_synced={total_synced}\n") - f.write(f"bytes_transferred={total_bytes}\n") + try: + with open(output_file, "a", encoding="utf-8") as f: + f.write(f"sync_status={overall}\n") + f.write(f"files_synced={total_synced}\n") + f.write(f"bytes_transferred={total_bytes}\n") + except OSError as e: + logger.warning("Failed to write GitHub outputs to %s: %s", output_file, e) + returnAlso applies to: 450-454
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sync_engine.py` at line 380, Wrap the code that writes to the GitHub Actions outputs (the _write_github_outputs(...) implementation and the other output-write site around lines where outputs are written, e.g., the block invoked at lines 450-454) in a try/except OSError block so an OSError does not kill the run; catch OSError and call self.logger.warning (or process logger used in this module) with a clear message including the exception, and ensure normal execution continues when writing $GITHUB_OUTPUT fails. Ensure both the _write_github_outputs function and the other output-write call are guarded the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/sync_engine.py`:
- Line 380: Wrap the code that writes to the GitHub Actions outputs (the
_write_github_outputs(...) implementation and the other output-write site around
lines where outputs are written, e.g., the block invoked at lines 450-454) in a
try/except OSError block so an OSError does not kill the run; catch OSError and
call self.logger.warning (or process logger used in this module) with a clear
message including the exception, and ensure normal execution continues when
writing $GITHUB_OUTPUT fails. Ensure both the _write_github_outputs function and
the other output-write call are guarded the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5881ed48-70cb-4e02-a125-d1c642a3de8f
📒 Files selected for processing (4)
.github/workflows/ci.ymlCONTRIBUTING.mdsrc/sync_engine.pytests/test_sync_engine_extended.py
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_sync_engine_extended.py
The original `docker run ... || true` silently swallowed all errors, masking entrypoint bugs (argument parsing, import failures). Changes: - Add GITHUB_OUTPUT and GITHUB_STEP_SUMMARY env vars to test the output-writing code path inside the container - Capture docker output via tee instead of discarding it - Use set +e / set -e to tolerate non-zero exit (expected without real HF/MS tokens) while still asserting the entrypoint started correctly and all INPUT_* arguments were propagated - Grep for config, dry_run, and direction in output to verify argument bridging from env vars to CLI flags Signed-off-by: dongjiang <dongjiang1989@126.com>
The default sync_config.yaml contains a real model (qwen2.5-7b-instruct) which triggers API calls to HuggingFace and ModelScope, failing without real tokens. New approach: - Add config/ci_smoke_test.yaml with empty models/datasets - Docker smoke test uses this config, exercises the full dry-run flow: entrypoint → CLI parsing → config loading → sync_all() → GitHub outputs - Expects clean exit 0 and asserts 4 checkpoints in output: config summary, dry_run flag, direction flag, "No sync items configured" This means the CI Docker test now fully passes without any tokens, validating the complete pipeline end-to-end. Signed-off-by: dongjiang <dongjiang1989@126.com>
HubApi.list_repo_files() doesn't exist in the ModelScope SDK — it's a HuggingFace method name. This caused a silent fallback to HF endpoint on every MS snapshot, losing file sizes and SHA hashes. Fix: - Use get_model_files(repo_id) for models - Use get_dataset_files(repo_id) for datasets - Parse dict responses with keys 'Name', 'Size', 'Revision' - Keep getattr fallback for potential future object returns Verified: Qwen/Qwen2.5-7B-Instruct now returns 14 files with proper sizes and SHA-256 hashes directly from the MS API. Signed-off-by: dongjiang <dongjiang1989@126.com>
The `docker run hf-ms-sync:ci cat /entrypoint.sh` command failed because Dockerfile ENTRYPOINT is `/entrypoint.sh`, so `cat` was passed as an argument to the sync engine instead of overriding the command. Use `--entrypoint cat` to properly override the entrypoint for the verification step. Signed-off-by: dongjiang <dongjiang1989@126.com>
dongjiang1989
added a commit
that referenced
this pull request
May 29, 2026
sync.yml: - Fix word-splitting bug: replace string ARGS with bash array, same issue as entrypoint.sh (PR #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) Signed-off-by: dongjiang <dongjiang1989@126.com>
dongjiang1989
added a commit
that referenced
this pull request
May 29, 2026
* fix: harden sync workflow and snapshot error handling (#3) sync.yml: - Fix word-splitting bug: replace string ARGS with bash array, same issue as entrypoint.sh (PR #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) Signed-off-by: dongjiang <dongjiang1989@126.com> * fix: bidirectional snapshot guard uses 'or', revert partial exit code Address PR #4 review from gemini-code-assist: 1. BIDIRECTIONAL: change `and` to `or` in snapshot None check. Both snapshots are required for bidirectional sync — if either is None, detect_bidirectional() crashes with AttributeError on the None snapshot. Now raises RuntimeError with specific status per adapter (e.g. "HF=OK, MS=FAILED"). 2. Exit code: revert to exit(1) on any failure (not just all-fail). CI/CD downstream steps already use `if: always()` so they run regardless of exit code. Returning 0 on partial failure would silently mask errors in automated pipelines. +1 test: test_one_snapshot_fails_bidirectional Signed-off-by: dongjiang <dongjiang1989@126.com> --------- Signed-off-by: dongjiang <dongjiang1989@126.com>
dongjiang1989
added a commit
that referenced
this pull request
May 29, 2026
* fix: harden sync workflow and snapshot error handling (#3) sync.yml: - Fix word-splitting bug: replace string ARGS with bash array, same issue as entrypoint.sh (PR #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) Signed-off-by: dongjiang <dongjiang1989@126.com> * fix: bidirectional snapshot guard uses 'or', revert partial exit code Address PR #4 review from gemini-code-assist: 1. BIDIRECTIONAL: change `and` to `or` in snapshot None check. Both snapshots are required for bidirectional sync — if either is None, detect_bidirectional() crashes with AttributeError on the None snapshot. Now raises RuntimeError with specific status per adapter (e.g. "HF=OK, MS=FAILED"). 2. Exit code: revert to exit(1) on any failure (not just all-fail). CI/CD downstream steps already use `if: always()` so they run regardless of exit code. Returning 0 on partial failure would silently mask errors in automated pipelines. +1 test: test_one_snapshot_fails_bidirectional Signed-off-by: dongjiang <dongjiang1989@126.com> * fix: disk space, create_repo, and partial state persistence Problem analysis from failed sync run (151s, exit code 1, empty .sync_state/): 1. Disk space: 4 parallel downloads of Qwen2.5-7B (~14GB total) fills up the GitHub Actions runner (~14GB). Fix: large files (>100MB) transfer sequentially (download→upload→delete→next), small files still use parallel. 2. create_repo_if_needed: was calling get_repo_snapshot (full file listing) just to check if repo exists. Fix: use repo_exists() which is a lightweight API call. Also wrap in try/except to be non-fatal. 3. _update_state: only called on full success, so partial transfers were lost. Fix: call _update_state whenever files_synced is non-empty, so successfully transferred files are recorded and won't be re-synced. Signed-off-by: dongjiang <dongjiang1989@126.com> * fix: add repo_type to create_repo and cross-reference file maps in state update PR #6 review fixes (2 of 3 comments valid): 1. modelscope_adapter.py: Pass repo_type parameter to create_repo() - Without this, dataset repos were incorrectly created as model repos - ModelScope SDK defaults repo_type='model' when not specified 2. sync_engine.py: Cross-reference file maps when updating sync state - When syncing MS→HF, the synced file's SHA is in ms_file_map, not hf_file_map - Without fallback, synced_files wouldn't be recorded in state - Now tries both maps: hf_file_map.get(fp) or ms_file_map.get(fp) - Same logic for MS state: ms_file_map.get(fp) or hf_file_map.get(fp) Comment 1 (repo_exists method) was invalid: HubApi.repo_exists() does exist in the ModelScope SDK (verified: returns bool, accepts repo_type parameter). Signed-off-by: dongjiang <dongjiang1989@126.com> --------- Signed-off-by: dongjiang <dongjiang1989@126.com>
dongjiang1989
added a commit
that referenced
this pull request
May 29, 2026
sync.yml: - Fix word-splitting bug: replace string ARGS with bash array, same issue as entrypoint.sh (PR #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) Signed-off-by: dongjiang <dongjiang1989@126.com>
dongjiang1989
added a commit
that referenced
this pull request
May 29, 2026
* fix: harden sync workflow and snapshot error handling (#3) sync.yml: - Fix word-splitting bug: replace string ARGS with bash array, same issue as entrypoint.sh (PR #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) Signed-off-by: dongjiang <dongjiang1989@126.com> * fix: bidirectional snapshot guard uses 'or', revert partial exit code Address PR #4 review from gemini-code-assist: 1. BIDIRECTIONAL: change `and` to `or` in snapshot None check. Both snapshots are required for bidirectional sync — if either is None, detect_bidirectional() crashes with AttributeError on the None snapshot. Now raises RuntimeError with specific status per adapter (e.g. "HF=OK, MS=FAILED"). 2. Exit code: revert to exit(1) on any failure (not just all-fail). CI/CD downstream steps already use `if: always()` so they run regardless of exit code. Returning 0 on partial failure would silently mask errors in automated pipelines. +1 test: test_one_snapshot_fails_bidirectional Signed-off-by: dongjiang <dongjiang1989@126.com> * fix: disk space, create_repo, and partial state persistence Problem analysis from failed sync run (151s, exit code 1, empty .sync_state/): 1. Disk space: 4 parallel downloads of Qwen2.5-7B (~14GB total) fills up the GitHub Actions runner (~14GB). Fix: large files (>100MB) transfer sequentially (download→upload→delete→next), small files still use parallel. 2. create_repo_if_needed: was calling get_repo_snapshot (full file listing) just to check if repo exists. Fix: use repo_exists() which is a lightweight API call. Also wrap in try/except to be non-fatal. 3. _update_state: only called on full success, so partial transfers were lost. Fix: call _update_state whenever files_synced is non-empty, so successfully transferred files are recorded and won't be re-synced. Signed-off-by: dongjiang <dongjiang1989@126.com> * fix: add repo_type to create_repo and cross-reference file maps in state update PR #6 review fixes (2 of 3 comments valid): 1. modelscope_adapter.py: Pass repo_type parameter to create_repo() - Without this, dataset repos were incorrectly created as model repos - ModelScope SDK defaults repo_type='model' when not specified 2. sync_engine.py: Cross-reference file maps when updating sync state - When syncing MS→HF, the synced file's SHA is in ms_file_map, not hf_file_map - Without fallback, synced_files wouldn't be recorded in state - Now tries both maps: hf_file_map.get(fp) or ms_file_map.get(fp) - Same logic for MS state: ms_file_map.get(fp) or hf_file_map.get(fp) Comment 1 (repo_exists method) was invalid: HubApi.repo_exists() does exist in the ModelScope SDK (verified: returns bool, accepts repo_type parameter). Signed-off-by: dongjiang <dongjiang1989@126.com> * fix(modelscope): improve error message for permission failures When uploading to ModelScope fails due to lack of write access, the API returns a misleading "resource does not exist" error. This commit adds explicit error detection for permission failures (401/403/"does not exist") and raises a clear PermissionError with actionable guidance: - Tells the user they don't have write access - Suggests using their own namespace (e.g., 'username/model-name') - Applies to both SDK upload and HF fallback upload paths This prevents confusing errors when users try to sync to repos they don't own (e.g., 'Qwen/Qwen2.5-7B-Instruct' instead of their own repo). Signed-off-by: dongjiang <dongjiang1989@126.com> * chore(config): use own ModelScope namespace for sync target The previous ms_repo_id 'Qwen/Qwen2.5-7B-Instruct' belongs to the Qwen organization and requires write access that external users do not have. Changed to 'dongjiang1989/Qwen2.5-7B-Instruct' so the sync can create and upload to a repo under the user's own namespace. Signed-off-by: dongjiang <dongjiang@kubeservice.com> Signed-off-by: dongjiang <dongjiang1989@126.com> * fix(modelscope): case-insensitive permission error detection Use str(e).lower() when matching error messages so that variants like "Does Not Exist", "Forbidden", or "FORBIDDEN" are all caught. Also added "forbidden" as an additional keyword to match. Addresses gemini-code-assist review on PR #8. Signed-off-by: dongjiang <dongjiang@kubeservice.com> Signed-off-by: dongjiang <dongjiang1989@126.com> * style(modelscope): fix E501 line-too-long in permission checks Refactor long `or` chain into `any()` with a tuple of keywords to keep lines under 100 chars. Signed-off-by: dongjiang <dongjiang@kubeservice.com> Signed-off-by: dongjiang <dongjiang1989@126.com> --------- Signed-off-by: dongjiang <dongjiang1989@126.com> Signed-off-by: dongjiang <dongjiang@kubeservice.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.
Summary
Transforms the HF-MS Sync project into a production-ready GitHub Marketplace Action. Adds Docker entrypoint with output support, CI/CD pipelines, preflight checks in the sync workflow, 5 critical bug fixes, and expands test coverage from 48 to 90 cases.
What Changed
GitHub Marketplace Readiness
entrypoint.sh— bridges DockerINPUT_*env vars to CLI args, writes$GITHUB_OUTPUT(sync_status,files_synced,bytes_transferred) and$GITHUB_STEP_SUMMARYaction.yml— addedstate_dir,hf_token,modelscope_tokeninputsDockerfile— entrypoint.sh COPY, OCI labelsCI/CD Pipelines
.github/workflows/ci.yml— PR pipeline: ruff lint + format + pytest (Python 3.10/3.11/3.12) + action validation + Docker build.github/workflows/release.yml— auto-tagging, major version management, GHCR Docker image publishing.github/workflows/sync.yml— addedpreflightjob (lint + tests + config validation) before sync; newskip_preflightinput for emergency bypassBug Fixes (5)
src/adapters/huggingface_adapter.pysrc/sync_engine.pysrc/sync_engine.pylast_results.jsonwas never written (broke report step)src/sync_engine.pysynced_filesdict in state was never populated (weakened cross-run change detection)Test Coverage: 48 → 90 cases (+42)
tests/test_report.py(10 cases) — report formatting, JSON output, stdout/summarytests/test_sync_engine_extended.py(19 cases) — MS→HF, bidirectional, partial/full failures, datasets, GITHUB_OUTPUTtests/test_adapters.py(9 cases) — adapter init, SDK fallback, state-based change detectionCommunity Health Files
CHANGELOG.md,CONTRIBUTING.md.github/ISSUE_TEMPLATE/bug_report.yml,feature_request.yml.github/PULL_REQUEST_TEMPLATE.mdCode Quality
ruff formatacross all source and test filesSummary by CodeRabbit
New Features
Documentation
Tests