Conversation
🤖 Pull request artifacts
|
WalkthroughReplaces flag-based bootstrap with a urfave/cli v3 app, removes the legacy internal/config package, injects structured cli-logger into client/watcher/syncer, adds a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as "CLI App (urfave/cli)"
participant Logger as "Logger (cli-logger)"
participant ClientFactory as "Client Factory"
participant Client as "FTP Client"
participant Watcher
participant Syncer
User->>CLI: invoke `sftp-sync sync [source] --dest ... --exclude ...`
CLI->>Logger: initialize logger (load .env, set level)
CLI->>ClientFactory: New(dest, logger.WithContext("client",""))
ClientFactory-->>Client: return ftp client
CLI->>Watcher: New(source, excludes, logger.WithContext("watcher",""))
CLI->>Syncer: New(source, client, logger.WithContext("syncer",""))
CLI->>Watcher: Watch(ctx, &wg)
Watcher->>CLI: emit fs events
CLI->>Syncer: Sync(ctx, event.AbsPath)
Syncer->>Client: perform upload/remove
Client-->>Syncer: operation result
Syncer->>Logger: structured info/error logs
Watcher->>CLI: close events channel when done
CLI->>CLI: wait for goroutines (wg.Wait)
CLI->>Logger: shutdown / log.Close()
CLI-->>User: exit with status code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
third_party/urfave-cli-v3/cli.go (2)
56-58: Help and version handling not implemented.The stub discards output and doesn't implement automatic
--helpor--versionflag handling that the realurfave/cli/v3provides. Users expecting these flags to work will see no output. This is acceptable for a minimal stub, but worth noting for Phase 3 testing mentioned in the migration plan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/urfave-cli-v3/cli.go` around lines 56 - 58, The current Command.Run stub silences flag output (fs.SetOutput(io.Discard)) and doesn't handle automatic --help/--version behavior; update Command.Run to preserve output (remove or change io.Discard to os.Stdout or a provided writer) and implement basic handling: register standard help and version flags on the local flag set created by flag.NewFlagSet, detect when --help or --version is present after parsing, and print the command's help/version (use Command's Usage/UsageText or Version fields) then return without executing the command; reference Command.Run, the fs variable from flag.NewFlagSet, and the existing c.Name/c.Usage/Version symbols to locate where to add this logic.
25-30:Requiredfield is defined but not enforced.The
StringFlag.Requiredfield exists butRun()never validates it. If the realurfave/cli/v3enforces required flags during parsing, this stub will behave differently. Consider whether the stub should error when a required flag is missing, or document this as an intentional limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/urfave-cli-v3/cli.go` around lines 25 - 30, The StringFlag.Required field is defined but never enforced; update the CLI flag parsing path (where flags are processed—look for the Run() or parse/resolve functions that handle StringFlag) to validate that any StringFlag with Required == true has a non-empty value (consider checking Destination, provided context map, or resolved flag values) and return or surface an error when a required flag is missing; ensure you reference StringFlag.Required and the parsing entrypoint (e.g., Run / parseFlags / resolveFlags) so callers get a clear error instead of silently continuing, or add documentation comment on StringFlag stating that Required is intentionally not enforced if you opt not to implement validation.internal/config/cli_app.go (1)
34-40: Redundant assignments:Destinationbindings already populatecfg.The
Destinationpointers on each flag (lines 21, 26, 31) already write parsed values directly tocfgfields duringRun(). The Action then re-reads the same values viacmd.String()/cmd.StringSlice()/cmd.Bool()and assigns them again.Either approach works, but using both is confusing and could mask bugs. Consider removing the redundant assignments in Action and keeping only the
WatchPathassignment (which has noDestinationbinding):♻️ Simplified Action
Action: func(_ context.Context, cmd *cli.Command) error { - cfg.Dest = cmd.String("dest") - cfg.ExcludePaths = append([]string{}, cmd.StringSlice("exclude")...) - cfg.Debug = cmd.Bool("debug") cfg.WatchPath = cmd.Args().Get(0) return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/cli_app.go` around lines 34 - 40, The Action function in the CLI currently re-reads flags (using cmd.String, cmd.StringSlice, cmd.Bool) and assigns them back to cfg.Dest, cfg.ExcludePaths and cfg.Debug even though those flags already use Destination to populate cfg; remove the redundant assignments for cfg.Dest, cfg.ExcludePaths and cfg.Debug from the Action body and keep only the WatchPath assignment (cfg.WatchPath = cmd.Args().Get(0)) and the existing return nil so the Destination-bound flag values are the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/cli_app.go`:
- Around line 17-32: The exhaustruct linter flags the StringFlag in the Flags
slice as missing an explicit Required field; update the StringFlag (the entry
that sets Name "dest" and Destination &cfg.Dest) to include Required: false
(explicitly) so the flag struct is fully specified; apply the same explicit
Required: false to any other cli flag structs in this block if they are also
missing it (e.g., the StringSliceFlag for "exclude" and BoolFlag "debug") to
silence the linter.
---
Nitpick comments:
In `@internal/config/cli_app.go`:
- Around line 34-40: The Action function in the CLI currently re-reads flags
(using cmd.String, cmd.StringSlice, cmd.Bool) and assigns them back to cfg.Dest,
cfg.ExcludePaths and cfg.Debug even though those flags already use Destination
to populate cfg; remove the redundant assignments for cfg.Dest, cfg.ExcludePaths
and cfg.Debug from the Action body and keep only the WatchPath assignment
(cfg.WatchPath = cmd.Args().Get(0)) and the existing return nil so the
Destination-bound flag values are the single source of truth.
In `@third_party/urfave-cli-v3/cli.go`:
- Around line 56-58: The current Command.Run stub silences flag output
(fs.SetOutput(io.Discard)) and doesn't handle automatic --help/--version
behavior; update Command.Run to preserve output (remove or change io.Discard to
os.Stdout or a provided writer) and implement basic handling: register standard
help and version flags on the local flag set created by flag.NewFlagSet, detect
when --help or --version is present after parsing, and print the command's
help/version (use Command's Usage/UsageText or Version fields) then return
without executing the command; reference Command.Run, the fs variable from
flag.NewFlagSet, and the existing c.Name/c.Usage/Version symbols to locate where
to add this logic.
- Around line 25-30: The StringFlag.Required field is defined but never
enforced; update the CLI flag parsing path (where flags are processed—look for
the Run() or parse/resolve functions that handle StringFlag) to validate that
any StringFlag with Required == true has a non-empty value (consider checking
Destination, provided context map, or resolved flag values) and return or
surface an error when a required flag is missing; ensure you reference
StringFlag.Required and the parsing entrypoint (e.g., Run / parseFlags /
resolveFlags) so callers get a clear error instead of silently continuing, or
add documentation comment on StringFlag stating that Required is intentionally
not enforced if you opt not to implement validation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44cddcdf-193d-4535-a7f8-97bc33ed466a
📒 Files selected for processing (6)
docs/cli-urfave-v3-migration-plan.mdgo.modinternal/config/cli_app.gointernal/config/config.gothird_party/urfave-cli-v3/cli.gothird_party/urfave-cli-v3/go.mod
capcom6
force-pushed
the
codex/plan-migration-to-urfave-v3
branch
from
dd9e578 to
5286363
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/commands/sync/config.go`:
- Around line 27-38: parseConfig currently reads only cmd.Args().Get(0) and
ignores extra positional args; update parseConfig to enforce exactly one
positional source by checking cmd.Args().Len() (or equivalent) and returning an
error if len != 1, then set cfg.Source from cmd.Args().Get(0); keep other fields
(cfg.Dest, cfg.Excludes) and call cfg.validate() as before so a second
positional argument is rejected rather than silently ignored.
In `@main.go`:
- Around line 28-29: The root context created as ctx :=
logger.WithLogger(context.Background(), log) is not signal-aware, so replace the
background context with a context that cancels on SIGINT/SIGTERM and then attach
the logger to it; e.g. create a signal-aware rootContext (using
signal.NotifyContext or context.WithCancel + signal.Notify for
os.Interrupt/syscall.SIGTERM), pass that into logger.WithLogger to produce ctx,
and ensure the signal-aware context is used by the watcher/syncer goroutines so
ctx.Done() is triggered on termination.
- Around line 87-90: The current error handling after app.Run(ctx, os.Args)
always sets exitCode = codes.InternalError, which discards specific exit codes
returned by cli.Exit() from syncAction; change the handler to detect whether the
returned error implements urfave/cli/v3's cli.ExitCoder (or has an ExitCode
method) and, if so, set exitCode to that ExitCode(), otherwise fall back to
codes.InternalError; update the block around app.Run and the exitCode assignment
so it preserves and uses the ExitCode from the error when available.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cf44c3d-0aa1-4e10-b09c-9fe4738f407c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.modinternal/cli/codes/codes.gointernal/cli/commands/sync/config.gointernal/cli/commands/sync/sync.gointernal/client/ftp.gointernal/config/config.gointernal/config/config_test.gointernal/config/errors.gointernal/config/params.gointernal/config/version.gomain.go
💤 Files with no reviewable changes (5)
- internal/config/params.go
- internal/config/config_test.go
- internal/config/errors.go
- internal/config/version.go
- internal/config/config.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
There was a problem hiding this comment.
🧹 Nitpick comments (2)
main.go (1)
93-98: Consider deferringstop()earlier for consistent cleanup.Currently
stop()is called afterlog.Close(). While functionally correct, deferringstop()immediately after creating the context (at line 30) would ensure signal cleanup even if a panic occurs between context creation and the explicitstop()call.♻️ Proposed fix
rootCtx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() ctx := logger.WithLogger(rootCtx, log) ... if err := log.Close(); err != nil { fmt.Fprintf(os.Stderr, "failed to close logger: %v\n", err) } - stop() os.Exit(exitCode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.go` around lines 93 - 98, Defer stop() immediately after you create the cancellable context (i.e., right after the call that returns stop/cancel in main) so cleanup runs on panic or early returns; then remove (or make redundant) the later explicit stop() just before os.Exit to avoid double-calling—reference the stop() function returned from the context creation and the main function where log.Close() and os.Exit are invoked.internal/cli/commands/sync/config.go (1)
20-24: Remove redundant conditional block.The
if len(c.Excludes) == 0block returnsnil, but the code also returnsnilimmediately after. This conditional serves no purpose.♻️ Proposed fix
if c.Dest == "" { return cli.Exit("destination server is required", 1) } - if len(c.Excludes) == 0 { - return nil - } - return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/sync/config.go` around lines 20 - 24, Remove the redundant conditional that checks len(c.Excludes) == 0 and returns nil; simply return nil unconditionally. Locate the block referencing c.Excludes (the if len(c.Excludes) == 0 { return nil } followed by return nil) and delete the if-statement so the function returns nil once without the unnecessary conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cli/commands/sync/config.go`:
- Around line 20-24: Remove the redundant conditional that checks
len(c.Excludes) == 0 and returns nil; simply return nil unconditionally. Locate
the block referencing c.Excludes (the if len(c.Excludes) == 0 { return nil }
followed by return nil) and delete the if-statement so the function returns nil
once without the unnecessary conditional.
In `@main.go`:
- Around line 93-98: Defer stop() immediately after you create the cancellable
context (i.e., right after the call that returns stop/cancel in main) so cleanup
runs on panic or early returns; then remove (or make redundant) the later
explicit stop() just before os.Exit to avoid double-calling—reference the stop()
function returned from the context creation and the main function where
log.Close() and os.Exit are invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6b0ec64-e15e-46f5-9255-1d2b186461e9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.goreleaser.yamlgo.modinternal/cli/commands/sync/config.gointernal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- internal/cli/commands/sync/sync.go
capcom6
force-pushed
the
codex/plan-migration-to-urfave-v3
branch
from
1edd346 to
3b9e7e6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/commands/sync/sync.go (1)
70-70: Remove the stale placeholder comment in the action path.Line 70 (
Add your sync logic here) no longer reflects the actual implementation and may confuse future edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/sync/sync.go` at line 70, Remove the stale placeholder comment "Add your sync logic here" from the sync command's action handler (the function that implements the sync command) so it no longer misleads readers; update or delete the comment in the action path (the function where sync logic is executed) and, if helpful, replace it with a brief, accurate comment or leave the implementation code without the placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cli/commands/sync/sync.go`:
- Line 70: Remove the stale placeholder comment "Add your sync logic here" from
the sync command's action handler (the function that implements the sync
command) so it no longer misleads readers; update or delete the comment in the
action path (the function where sync logic is executed) and, if helpful, replace
it with a brief, accurate comment or leave the implementation code without the
placeholder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a276f748-44b3-4cd1-b86e-bdbcce75d8d6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.goreleaser.yamlgo.modinternal/cli/codes/codes.gointernal/cli/commands/sync/config.gointernal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp.gointernal/config/config.gointernal/config/config_test.gointernal/config/errors.gointernal/config/params.gointernal/config/version.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
💤 Files with no reviewable changes (5)
- internal/config/errors.go
- internal/config/params.go
- internal/config/config_test.go
- internal/config/version.go
- internal/config/config.go
✅ Files skipped from review due to trivial changes (2)
- internal/cli/codes/codes.go
- internal/syncer/syncer.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/client/client.go
- go.mod
- internal/cli/commands/sync/config.go
capcom6
force-pushed
the
codex/plan-migration-to-urfave-v3
branch
from
3c8f011 to
c192e92
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
README.md (2)
136-139: Tighten tone/wording in Contributing section.Line 136 (“open source”) should be hyphenated as “open-source,” and Lines 136–139 use a very informal/exclamatory tone. Consider a slightly more neutral style for consistency with the rest of the README.
Suggested wording
-Contributions are what make the open source community such an amazing place to learn, inspire, and create. Any contributions you make are **greatly appreciated**. +Contributions are what make the open-source community a great place to learn, inspire, and create. Any contributions you make are **greatly appreciated**. -If you have a suggestion that would make this better, please fork the repo and create a pull request. You can also simply open an issue with the tag "enhancement". -Don't forget to give the project a star! Thanks again! +If you have a suggestion to improve this project, please fork the repository and open a pull request. You can also open an issue with the `enhancement` label. +If this project is useful to you, consider starring it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 136 - 139, Update the "Contributing" section to use a neutral, professional tone and hyphenate "open-source"; specifically, in the Contributing paragraph replace the current informal/exclamatory sentences with a concise version such as: express appreciation succinctly, invite contributions by forking and opening pull requests or filing issues with the "enhancement" label, and remove exclamation points (i.e., change "open source" to "open-source", drop emotive phrasing like "greatly appreciated" and "Don't forget to give the project a star! Thanks again!").
190-190: Remove unused reference definitionproduct-screenshot.Line 190 defines
[product-screenshot], but there’s no active usage (the only occurrence is commented out at Line 70). This triggers markdownlint MD053 and adds dead reference clutter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 190, Remove the unused markdown reference definition "[product-screenshot]: images/screenshot.png" (the "product-screenshot" reference) from README.md so the dead reference is eliminated; if you intended to use it instead, restore or uncomment the corresponding image usage where "product-screenshot" is referenced, otherwise delete the reference line to satisfy markdownlint MD053.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 54-65: Add an "Installation" section to the README and include it
in the TABLE OF CONTENTS between "About The Project" and "Usage"; the
Installation section should provide at least one canonical install path (for
example a "go install github.com/owner/repo/cmd/sftp-sync@latest" command and/or
instructions for using the release binary), mention any prerequisites, and show
the basic first-time setup steps required before running "sftp-sync" so readers
can follow the TOC entry and then find the install instructions prior to the
existing "Usage" content.
- Line 152: Update the README.md text string that currently reads "Distributed
under the Apache License 2.0. See `LICENSE.txt` for more information." to
reference the correct license filename; replace `LICENSE.txt` with `LICENSE` so
the line reads "Distributed under the Apache License 2.0. See `LICENSE` for more
information." This change targets the exact sentence in README.md (the line
containing the phrase "See `LICENSE.txt`") and should be a simple text
substitution with no other content changes.
---
Nitpick comments:
In `@README.md`:
- Around line 136-139: Update the "Contributing" section to use a neutral,
professional tone and hyphenate "open-source"; specifically, in the Contributing
paragraph replace the current informal/exclamatory sentences with a concise
version such as: express appreciation succinctly, invite contributions by
forking and opening pull requests or filing issues with the "enhancement" label,
and remove exclamation points (i.e., change "open source" to "open-source", drop
emotive phrasing like "greatly appreciated" and "Don't forget to give the
project a star! Thanks again!").
- Line 190: Remove the unused markdown reference definition
"[product-screenshot]: images/screenshot.png" (the "product-screenshot"
reference) from README.md so the dead reference is eliminated; if you intended
to use it instead, restore or uncomment the corresponding image usage where
"product-screenshot" is referenced, otherwise delete the reference line to
satisfy markdownlint MD053.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba3c3bcc-0ce3-4992-8479-fe53841110e0
⛔ Files ignored due to path filters (1)
images/logo.pngis excluded by!**/*.png
📒 Files selected for processing (1)
README.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
283-283: Remove unused markdown reference definition.The
product-screenshotreference is defined but never used (the only reference at line 80 is commented out). Consider removing this definition to keep the markdown references clean.🧹 Proposed cleanup
-[product-screenshot]: images/screenshot.png🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 283, Remove the unused markdown reference definition "[product-screenshot]: images/screenshot.png" from README.md (symbol: product-screenshot); if there is a commented-out usage of that reference elsewhere, either remove or restore that commented reference to avoid leaving orphaned metadata—ensure no remaining references to product-screenshot remain after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Line 283: Remove the unused markdown reference definition
"[product-screenshot]: images/screenshot.png" from README.md (symbol:
product-screenshot); if there is a commented-out usage of that reference
elsewhere, either remove or restore that commented reference to avoid leaving
orphaned metadata—ensure no remaining references to product-screenshot remain
after the change.
7a07a36 to
b6338a9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
README.md (2)
283-283: Remove unused markdown reference definition.The
[product-screenshot]reference definition is unused (the actual screenshot link at line 80 is commented out as a template placeholder).🧹 Proposed cleanup
-[product-screenshot]: images/screenshot.png🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 283, Remove the unused markdown reference definition "[product-screenshot]: images/screenshot.png" from README.md; locate the reference label "[product-screenshot]" in the file and delete that reference line (and optionally remove or update any commented template screenshot link that references it) so no orphaned markdown reference remains.
43-43: Clarify FTP vs. SFTP to avoid user confusion.The project is named "sftp-sync" but the description states it syncs with "a remote FTP server." Line 210 confirms SFTP support is a future roadmap item. Consider adding a brief note early in the README clarifying that despite the name, the current version supports FTP (not SFTP), with SFTP planned for the future.
📝 Suggested clarification
After line 82, consider adding:
> **Note:** Despite the project name, the current version supports FTP protocol. SFTP support is planned for a future release (see [Roadmap](`#roadmap`)).Also applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 43, Add a short clarifying note in README.md (e.g., immediately after the project description or after the "sftp-sync" heading) stating that despite the project name "sftp-sync" the current release supports FTP, not SFTP, and that SFTP support is planned for a future release; reference the "Roadmap" heading so users can find planned SFTP work (suggested wording: Note: Despite the project name, the current version supports the FTP protocol; SFTP support is planned for a future release — see Roadmap).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Line 283: Remove the unused markdown reference definition
"[product-screenshot]: images/screenshot.png" from README.md; locate the
reference label "[product-screenshot]" in the file and delete that reference
line (and optionally remove or update any commented template screenshot link
that references it) so no orphaned markdown reference remains.
- Line 43: Add a short clarifying note in README.md (e.g., immediately after the
project description or after the "sftp-sync" heading) stating that despite the
project name "sftp-sync" the current release supports FTP, not SFTP, and that
SFTP support is planned for a future release; reference the "Roadmap" heading so
users can find planned SFTP work (suggested wording: Note: Despite the project
name, the current version supports the FTP protocol; SFTP support is planned for
a future release — see Roadmap).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2138937-9075-4d15-abc9-9da5f41be8fb
⛔ Files ignored due to path filters (1)
images/logo.pngis excluded by!**/*.png
📒 Files selected for processing (1)
README.md
capcom6
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
main.go (2)
87-89: Remove commented-out command registration to reduce ambiguity.Keeping
// sync.Command(),in theCommandslist is confusing now that rootActionis the execution path.Suggested cleanup
Commands: []*cli.Command{ - // sync.Command(), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.go` around lines 87 - 89, Remove the commented-out command registration to eliminate ambiguity: delete the commented line "// sync.Command()," from the Commands slice so the Commands: []*cli.Command{ ... } list contains only active entries (or is left empty) now that the root Action is the execution path; locate the Commands slice declaration (Commands: []*cli.Command{ ... }) and remove the commented sync.Command() reference.
59-60: Drop redundant emptyDescriptionfield.
Description: ""adds noise without behavior change.Suggested cleanup
- Description: "",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.go` around lines 59 - 60, Remove the redundant empty Description field from the command struct literal (the line `Description: ""`) so the code is cleaner; locate the struct where `ArgsUsage: "[source]"` is set (the command definition) and delete the `Description: ""` entry leaving only meaningful fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@main.go`:
- Around line 87-89: Remove the commented-out command registration to eliminate
ambiguity: delete the commented line "// sync.Command()," from the Commands
slice so the Commands: []*cli.Command{ ... } list contains only active entries
(or is left empty) now that the root Action is the execution path; locate the
Commands slice declaration (Commands: []*cli.Command{ ... }) and remove the
commented sync.Command() reference.
- Around line 59-60: Remove the redundant empty Description field from the
command struct literal (the line `Description: ""`) so the code is cleaner;
locate the struct where `ArgsUsage: "[source]"` is set (the command definition)
and delete the `Description: ""` entry leaving only meaningful fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7aee00cc-d68d-4378-beb1-ed46da1e4c3b
📒 Files selected for processing (2)
internal/cli/commands/sync/sync.gomain.go
✅ Files skipped from review due to trivial changes (1)
- internal/cli/commands/sync/sync.go
capcom6
force-pushed
the
codex/plan-migration-to-urfave-v3
branch
from
9b39067 to
c4c9773
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
README.md (1)
283-283: Remove unusedproduct-screenshotreference or use it.
[product-screenshot]is currently defined but not referenced anywhere, which trips markdownlint (MD053).Proposed cleanup
-[product-screenshot]: images/screenshot.png🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 283, The README defines an unused markdown reference label "[product-screenshot]" which triggers markdownlint MD053; either remove the reference definition line "[product-screenshot]: images/screenshot.png" or add a matching inline reference where the image should appear (e.g., use "![Alt text][product-screenshot]" near the project overview or screenshot section). Update the README so the product-screenshot label is either referenced by an image tag (to use the asset) or deleted to eliminate the unused reference.internal/syncer/syncer.go (1)
99-103: Usefilepath.Joinfor the local filesystem path (absPath).Line 99 and Line 103 join
absPathwithpath.Join, which is for slash-separated paths, not OS filesystem paths. SinceabsPathis used withos.ReadDir()and represents a local filesystem path, usefilepath.Joinfor consistency with Go's filesystem path handling conventions. ForrelPath,path.Joinis acceptable as it's passed throughpathNormalize()which converts to forward slashes anyway.Proposed patch
if file.IsDir() { - if dErr := s.syncDir(ctx, path.Join(absPath, file.Name()), path.Join(relPath, file.Name())); dErr != nil { + if dErr := s.syncDir(ctx, filepath.Join(absPath, file.Name()), path.Join(relPath, file.Name())); dErr != nil { return dErr } } else { - if fErr := s.syncFile(ctx, path.Join(absPath, file.Name()), path.Join(relPath, file.Name())); fErr != nil { + if fErr := s.syncFile(ctx, filepath.Join(absPath, file.Name()), path.Join(relPath, file.Name())); fErr != nil { return fErr } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/syncer/syncer.go` around lines 99 - 103, The code uses path.Join when combining absPath with file.Name() inside calls to s.syncDir and s.syncFile, but absPath is a local filesystem path (used with os.ReadDir) and should be joined using filepath.Join to respect OS-specific separators; update the two call sites that construct the local path (the first arg passed into s.syncDir and s.syncFile) to use filepath.Join(absPath, file.Name()) while leaving the relPath construction with path.Join as-is (relPath is normalized by pathNormalize). Also add the necessary import for "path/filepath" if not already present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/syncer/syncer.go`:
- Around line 99-103: The code uses path.Join when combining absPath with
file.Name() inside calls to s.syncDir and s.syncFile, but absPath is a local
filesystem path (used with os.ReadDir) and should be joined using filepath.Join
to respect OS-specific separators; update the two call sites that construct the
local path (the first arg passed into s.syncDir and s.syncFile) to use
filepath.Join(absPath, file.Name()) while leaving the relPath construction with
path.Join as-is (relPath is normalized by pathNormalize). Also add the necessary
import for "path/filepath" if not already present.
In `@README.md`:
- Line 283: The README defines an unused markdown reference label
"[product-screenshot]" which triggers markdownlint MD053; either remove the
reference definition line "[product-screenshot]: images/screenshot.png" or add a
matching inline reference where the image should appear (e.g., use "![Alt
text][product-screenshot]" near the project overview or screenshot section).
Update the README so the product-screenshot label is either referenced by an
image tag (to use the asset) or deleted to eliminate the unused reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70342fd1-60e0-4442-93da-b5e94f46dbcd
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumimages/logo.pngis excluded by!**/*.png
📒 Files selected for processing (16)
.goreleaser.yamlREADME.mdgo.modinternal/cli/codes/codes.gointernal/cli/commands/sync/config.gointernal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp.gointernal/config/config.gointernal/config/config_test.gointernal/config/errors.gointernal/config/params.gointernal/config/version.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
💤 Files with no reviewable changes (5)
- internal/config/params.go
- internal/config/errors.go
- internal/config/config_test.go
- internal/config/version.go
- internal/config/config.go
✅ Files skipped from review due to trivial changes (2)
- go.mod
- internal/cli/codes/codes.go
🚧 Files skipped from review as they are similar to previous changes (7)
- .goreleaser.yaml
- internal/client/client.go
- internal/cli/commands/sync/config.go
- internal/client/ftp.go
- internal/watcher/watcher.go
- internal/cli/commands/sync/sync.go
- main.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
272-272: Remove unused markdown reference.The
product-screenshotreference is defined but never used in the document (the reference at line 79 is commented out). Consider removing this unused definition to keep the reference section clean.🧹 Proposed cleanup
-[product-screenshot]: images/screenshot.png🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 272, Remove the unused markdown reference "[product-screenshot]: images/screenshot.png" from README by deleting the definition for the symbol product-screenshot (it's defined but never referenced elsewhere—only a commented example exists); ensure no other references to product-screenshot remain and tidy the reference section accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Line 272: Remove the unused markdown reference "[product-screenshot]:
images/screenshot.png" from README by deleting the definition for the symbol
product-screenshot (it's defined but never referenced elsewhere—only a commented
example exists); ensure no other references to product-screenshot remain and
tidy the reference section accordingly.
capcom6
force-pushed
the
codex/plan-migration-to-urfave-v3
branch
from
7911c0e to
7d7880c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
272-272: Consider removing unused screenshot reference.Line 272 defines
[product-screenshot]but it's only used in a commented-out section (line 79). Consider removing this reference definition for cleaner markdown, or uncomment and populate the screenshot section if you plan to add product screenshots soon.🧹 Proposed cleanup
-[product-screenshot]: images/screenshot.png🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 272, Remove the unused markdown reference "[product-screenshot]" from the README (the definition shown in the diff) or alternatively restore and populate the commented-out screenshot section that references it; locate the reference label "[product-screenshot]" and either delete that definition line or uncomment and fill the screenshot block where it's referenced so the label is actually used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 43: Add a short clarification under the "About The Project" heading to
state that, despite the repository name "sftp-sync", the tool currently supports
FTP only and SFTP is planned for a future release; update any mentions of "FTP
server" or ftp:// examples if desired to remain consistent, and ensure the
roadmap entry referencing SFTP (the planned feature) stays labeled as future
work so readers aren’t confused.
- Around line 156-159: The README usage example is missing the required "sync"
subcommand; verify the CLI's commands (look for the sync command in your CLI
setup, e.g., the "sync" entry in the cli.App Commands slice) and if the CLI
requires a subcommand, update the example invocation to "sftp-sync sync
--dest=ftp://username:password@hostname:port/path/to/remote/folder
--exclude=.git /path/to/local/folder" so it matches the "Sync Command Options"
section; if instead the CLI exposes sync as the default behavior (no
subcommand), remove or rename the "Sync Command Options" header to avoid
confusion and make the README consistent with the actual CLI (referencing the
"sync" command name and "Sync Command Options" section to locate the docs to
change).
---
Nitpick comments:
In `@README.md`:
- Line 272: Remove the unused markdown reference "[product-screenshot]" from the
README (the definition shown in the diff) or alternatively restore and populate
the commented-out screenshot section that references it; locate the reference
label "[product-screenshot]" and either delete that definition line or uncomment
and fill the screenshot block where it's referenced so the label is actually
used.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65268ede-4194-4ca2-b043-ead6f9480728
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumimages/logo.pngis excluded by!**/*.png
📒 Files selected for processing (16)
.goreleaser.yamlREADME.mdgo.modinternal/cli/codes/codes.gointernal/cli/commands/sync/config.gointernal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp.gointernal/config/config.gointernal/config/config_test.gointernal/config/errors.gointernal/config/params.gointernal/config/version.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
💤 Files with no reviewable changes (5)
- internal/config/params.go
- internal/config/version.go
- internal/config/errors.go
- internal/config/config_test.go
- internal/config/config.go
✅ Files skipped from review due to trivial changes (4)
- internal/cli/codes/codes.go
- go.mod
- internal/cli/commands/sync/config.go
- internal/watcher/watcher.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/client/client.go
- internal/cli/commands/sync/sync.go
- main.go
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.
Motivation
flag-based parsing withurfave/cli/v3to modernize CLI handling while preserving existing flag semantics and validation.Configstays the single runtime contract.Description
docs/cli-urfave-v3-migration-plan.mdthat outlines phases, compatibility checklist, and risks.newCLIAppininternal/config/cli_app.gowhich wires--dest, repeatable--exclude,--debug, program usage, and the positionalwatch-pathintoConfig.Parse(args []string)ininternal/config/config.goto run the new CLI app (app.Run(...)) and preserve existing validation viavalidate()and the wrapped error messagefailed to parse flags: ....github.com/urfave/cli/v3togo.modand point it to a local stub implementation withreplaceunderthird_party/urfave-cli-v3that implements the minimal API used here.Testing
go test ./...to exercise unit tests includinginternal/configparsing tests, and the test suite completed successfully.Codex Task
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation