feat(rsync): expose --exclude/--include/--exclude-from/--include-from#938
Open
ethany-nv wants to merge 1 commit into
Open
feat(rsync): expose --exclude/--include/--exclude-from/--include-from#938ethany-nv wants to merge 1 commit into
ethany-nv wants to merge 1 commit into
Conversation
Bumps the bundled rsync_bin to the f-luo/rsync fork (commit c84164e), which adds wildmatch-based filter support that upstream gokrazy/rsync v0.3.1 lacks (file-based filters were stubbed errNotYetImplemented and the wildcard matcher panicked). Plumbs an ordered RsyncFilterSpec through RsyncRequest so rules apply to both foreground transfers and the watch-based upload daemon. CLI uses a single argparse Action across all four flags so first-match-wins ordering between --exclude and --include is preserved. --exclude-from / --include-from paths are validated and resolved to absolute paths at parse time so the daemon can re-read them on every reconnect. Note: src/runtime/cmd/rsync/ resolves the same Go module, so runtime container images must roll out in lockstep with the client — old-server binaries panic on filter rules they don't understand. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe changes extend the rsync library and CLI with filter rule support across multiple layers. New types ( Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Parser
participant Workflow as Workflow
participant Library as Rsync Library
participant RsyncClient as Rsync Client
User->>CLI: Invoke rsync upload/download<br/>with --include/--exclude flags
CLI->>Workflow: Parse arguments
Workflow->>Workflow: Collect filter rules<br/>in order
Workflow->>Workflow: Build RsyncFilterSpec<br/>(resolve paths, validate files)
Workflow->>Library: Create RsyncRequest<br/>with filter_spec
Library->>RsyncClient: Pass filter_spec to<br/>upload/download
RsyncClient->>RsyncClient: Build filter args<br/>via _build_filter_args()
RsyncClient->>RsyncClient: Apply args to rsync<br/>command execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #938 +/- ##
==========================================
+ Coverage 43.88% 44.44% +0.56%
==========================================
Files 211 211
Lines 26982 27033 +51
Branches 4226 4231 +5
==========================================
+ Hits 11841 12015 +174
+ Misses 14505 14381 -124
- Partials 636 637 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/rsync/rsync.py (1)
1834-1887: ⚡ Quick winMake
filter_specnon-optional on the new API surface.
Nonenever carries distinct behavior here; every entry point immediately coerces it toRsyncFilterSpec(). Tightening these signatures to accept an empty spec directly keeps the types simpler and removes the normalization branch in three places. Please verify no external caller relies onNoneas a sentinel before changing the signatures. As per coding guidelines "Avoid unnecessary Optional types in Python — only useOptional[T]orT | NonewhenNonehas a distinct meaning from an empty value (e.g., 'not provided' vs 'provided but empty')."Also applies to: 1924-2034
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rsync/rsync.py` around lines 1834 - 1887, The public API should not accept None for filter_spec because callers always coerce it to RsyncFilterSpec(); change the function signature (the parser that returns RsyncRequest) to make filter_spec: RsyncFilterSpec (remove Optional/None), give it a default of RsyncFilterSpec() if you want a default, and remove the runtime normalization branch (the "filter_spec or RsyncFilterSpec()" in the RsyncRequest constructor). Update the other two similar APIs mentioned (the code regions around the other occurrences of filter_spec at ~1924-2034) to the same non-optional signature and remove any code paths that check for/filter None; also search for external callers and adjust any call sites that pass None to instead omit the argument or pass RsyncFilterSpec().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/workflow.py`:
- Around line 145-147: The check currently uses os.path.isfile(resolved) which
allows files that exist but are not readable; change the validation in the same
helper where the OSMOUserError is raised (the block referencing resolved and
kind.value) to also verify the file is readable (e.g., os.access(resolved,
os.R_OK) or attempt to open the file) and raise the same OSMOUserError with a
clear message (e.g., '--{kind.value} file is not readable: {value}') when the
check fails so unreadable `*-from` files are reported up front.
---
Nitpick comments:
In `@src/lib/rsync/rsync.py`:
- Around line 1834-1887: The public API should not accept None for filter_spec
because callers always coerce it to RsyncFilterSpec(); change the function
signature (the parser that returns RsyncRequest) to make filter_spec:
RsyncFilterSpec (remove Optional/None), give it a default of RsyncFilterSpec()
if you want a default, and remove the runtime normalization branch (the
"filter_spec or RsyncFilterSpec()" in the RsyncRequest constructor). Update the
other two similar APIs mentioned (the code regions around the other occurrences
of filter_spec at ~1924-2034) to the same non-optional signature and remove any
code paths that check for/filter None; also search for external callers and
adjust any call sites that pass None to instead omit the argument or pass
RsyncFilterSpec().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 784db0d0-f38a-4f7f-b016-20e2c0f47b66
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
src/cli/tests/test_cli.pysrc/cli/workflow.pysrc/go.modsrc/lib/rsync/rsync.py
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.
Description
Bumps the bundled
rsync_binto the f-luo/rsync fork via ago.modreplacedirective. The fork ports rsync's wildmatch algorithm and uncomments--exclude-from/--include-from, both of which were stubbederrNotYetImplementedin upstreamgokrazy/rsync v0.3.1(the wildcard matcher also panicked on common patterns like*.log). The Bazel repo name@com_github_gokrazy_rsyncis preserved because the fork keeps the original Go module path, so no BUILD changes are needed.Adds an ordered
RsyncFilterSpeccarried onRsyncRequestand threaded throughparse_rsync_request/rsync_upload/rsync_download. Filter rules apply to both foreground transfers and the watch-based upload daemon — they survive themultiprocessing.spawnboundary as a frozen dataclass with tuple fields, andRsyncDaemonMetadata.from_dictis updated with backward-compat handling so existing PID files (with or without legacysrc/dst_modulekeys) still load. Subprocess builders inject filter args between-avand the source/dest paths.Exposes four new flags on both
osmo workflow rsync uploadandosmo workflow rsync download:--exclude PATTERN(repeatable)--include PATTERN(repeatable)--exclude-from FILE--include-from FILEA single
argparse.Actiondispatches byoption_stringso all four flags interleave into one orderedfilter_ruleslist — this preserves rsync's first-match-wins semantics across--excludeand--include(e.g.--include '*.py' --exclude '*'to whitelist Python files). For*-fromflags, file existence is validated and paths are resolved to absolute at parse time so daemon mode fails loudly upfront rather than at every reconnect.Cross-cutting risk:
src/runtime/cmd/rsync/resolves the same Go module, so runtime container images must roll out in lockstep with the client. Old-server binaries panic when a new client sends wildcard filter rules.Tests: Added 10 unit tests covering CLI parse-order preservation,
*-fromfile validation, abs-path resolution,_build_filter_argsordering, JSON roundtrip of the spec, and threeRsyncDaemonMetadata.from_dictbackward-compat scenarios (no filter_spec, legacy schema, full roundtrip). Manually smoke-testedrsync_bin -av --exclude='*.log' src/ dst/end-to-end and confirmed*.logwas filtered.Issue #None
Checklist
Summary by CodeRabbit
Release Notes
New Features
Chores