fix: make CLI help parsing command-wide#477
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughCentralizes help-argument detection and usage text across CLI commands by adding IsHelpArgs variants, per-command usage helpers, and early-return help handling; replaces many hardcoded usage literals and extends tests for help/escape behaviors. ChangesCLI Help Handling Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e2eaf0d80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/drive9/cli/cat.go (1)
22-26: ⚡ Quick winHelp output bypasses the
outwriter.
catWithWritertakesout io.Writerso callers/tests can capture output, but the help path writes toos.Stdoutdirectly. This is inconsistent and makes the help branch untestable via the injected writer. Prefer writing toout.♻️ Proposed fix
func catWithWriter(c *client.Client, args []string, out io.Writer) error { if IsHelpArgs(args) { - _, _ = fmt.Fprintln(os.Stdout, fsCatUsage()) + _, _ = fmt.Fprintln(out, fsCatUsage()) return nil }🤖 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 `@cmd/drive9/cli/cat.go` around lines 22 - 26, The help branch in catWithWriter currently writes to os.Stdout, bypassing the injected out writer; change the call that prints the usage (where IsHelpArgs(args) is true) to write to the provided out writer (use fmt.Fprintln(out, fsCatUsage()) or equivalent) so fsCatUsage() output is testable via catWithWriter's out parameter and behavior remains the same otherwise.
🤖 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 `@cmd/drive9/cli/help.go`:
- Around line 16-26: IsHelpArgs currently treats a bare "help" token anywhere
before "--" as help intent and thereby swallows legitimate positional data;
update IsHelpArgs so that bare unprefixed "help" is only treated as help when it
appears as the first argument, while keeping dash-prefixed forms (IsHelpArg
matches like "-h", "-help", "--help") valid in any position; modify the loop in
IsHelpArgs to check the argument index and only return true for the bare "help"
case when index == 0, otherwise continue scanning for prefixed help flags and
respect the existing "--" escape logic.
In `@cmd/drive9/main_test.go`:
- Around line 177-182: The vault subcommand help strings are missing the colon
after "usage", causing inconsistency; update the help/usage text returned by the
vault helper functions in cmd/drive9/cli/secret.go and
cmd/drive9/cli/secret_put.go to use the same "usage: drive9 ..." prefix (i.e.,
add the colon) so the output matches other commands; update the string literals
used in the vault command's usage/help generators (the functions that
build/return the usage string for the vault and vault put commands) to include
the colon.
---
Nitpick comments:
In `@cmd/drive9/cli/cat.go`:
- Around line 22-26: The help branch in catWithWriter currently writes to
os.Stdout, bypassing the injected out writer; change the call that prints the
usage (where IsHelpArgs(args) is true) to write to the provided out writer (use
fmt.Fprintln(out, fsCatUsage()) or equivalent) so fsCatUsage() output is
testable via catWithWriter's out parameter and behavior remains the same
otherwise.
🪄 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
Run ID: 96d7d546-61a4-4bf5-a557-0f06c3ddc6a8
📒 Files selected for processing (26)
cmd/drive9/cli/cat.gocmd/drive9/cli/chmod.gocmd/drive9/cli/cli_consolidation_test.gocmd/drive9/cli/cp.gocmd/drive9/cli/ctx.gocmd/drive9/cli/db.gocmd/drive9/cli/doctor.gocmd/drive9/cli/find.gocmd/drive9/cli/grep.gocmd/drive9/cli/help.gocmd/drive9/cli/journal.gocmd/drive9/cli/ls.gocmd/drive9/cli/mkdir.gocmd/drive9/cli/mount.gocmd/drive9/cli/mount_vault.gocmd/drive9/cli/mv.gocmd/drive9/cli/rm.gocmd/drive9/cli/secret.gocmd/drive9/cli/secret_put.gocmd/drive9/cli/sh.gocmd/drive9/cli/sql.gocmd/drive9/cli/stat.gocmd/drive9/cli/symlink.gocmd/drive9/cli/token.gocmd/drive9/main.gocmd/drive9/main_test.go
|
@codex review Review PR #477 as an incremental review after responder fixes. Current head SHA is Focus on concrete bugs only. Report findings only when there is a plausible failure path in the current head. Do not report speculative, style-only, preference-only, or duplicate findings. Do not suppress risky changed paths merely because a prior responder concluded they are safe. Review DisciplineRound classification: Use the previous Codex/CodeRabbit review/comment baseline as already reviewed context, then focus on the diff since that baseline plus any touched prior findings. Still scan in-scope changes for high-severity correctness, security, data consistency, performance, compatibility, observability, and testing risks. If no issues are found, state Stable Invariants
Severity Rubric
Finding ContractOrder findings by severity, then confidence. Each finding must include:
Do not duplicate the same finding across multiple comments. Changes Since Previous Review BaselineCommit
GitHub review thread refresh at current head showed zero unresolved review threads. Accepted Findings Fixed: Do Not Repeat Unless New Failure Path Exists
Do not re-report those exact accepted findings unless the current head introduces a new, distinct failure path. Risky Changed Paths To Verify
Please review the current head and return only concrete bugs with plausible failure paths under the finding contract above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 493af8a831
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/drive9/cli/cp_test.go (1)
463-476: 💤 Low valueOptional: simplify cwd handling with
t.Chdir(tmp)This test can replace the
os.Getwd/os.Chdir+ cleanup restore witht.Chdir(tmp)(available in Go 1.24+), avoiding global-state footguns around working directory changes.♻️ Proposed simplification
tmp := t.TempDir() - oldWD, err := os.Getwd() - if err != nil { - t.Fatalf("Getwd: %v", err) - } - if err := os.Chdir(tmp); err != nil { - t.Fatalf("Chdir(%q): %v", tmp, err) - } - t.Cleanup(func() { - if err := os.Chdir(oldWD); err != nil { - t.Fatalf("restore cwd: %v", err) - } - }) + t.Chdir(tmp)🤖 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 `@cmd/drive9/cli/cp_test.go` around lines 463 - 476, Replace the manual working-directory save/restore in TestCpTreatsHelpLikeSourcesAsData with t.Chdir(tmp): remove the os.Getwd/Chdir/cleanup block and call t.Chdir(tmp) right after creating tmp so the testing framework handles restoring the original cwd; update references in the TestCpTreatsHelpLikeSourcesAsData function accordingly.
🤖 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.
Nitpick comments:
In `@cmd/drive9/cli/cp_test.go`:
- Around line 463-476: Replace the manual working-directory save/restore in
TestCpTreatsHelpLikeSourcesAsData with t.Chdir(tmp): remove the
os.Getwd/Chdir/cleanup block and call t.Chdir(tmp) right after creating tmp so
the testing framework handles restoring the original cwd; update references in
the TestCpTreatsHelpLikeSourcesAsData function accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4bbf996-ec62-49be-8bda-6f1fe5937f92
📒 Files selected for processing (12)
cmd/drive9/cli/cat.gocmd/drive9/cli/cat_test.gocmd/drive9/cli/cli_consolidation_test.gocmd/drive9/cli/cp.gocmd/drive9/cli/cp_test.gocmd/drive9/cli/ctx.gocmd/drive9/cli/help.gocmd/drive9/cli/secret.gocmd/drive9/cli/secret_commands_test.gocmd/drive9/cli/secret_put.gocmd/drive9/main.gocmd/drive9/main_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/drive9/cli/cp.go
- cmd/drive9/cli/cat.go
- cmd/drive9/cli/secret_put.go
- cmd/drive9/cli/help.go
- cmd/drive9/main.go
- cmd/drive9/cli/ctx.go
- cmd/drive9/cli/secret.go
- cmd/drive9/cli/cli_consolidation_test.go
|
@codex review Review PR #477 as an incremental review after round 2 responder fixes. Current head SHA is Focus on concrete bugs only. Report findings only when there is a plausible failure path in the current head. Do not report speculative, style-only, preference-only, or duplicate findings. Do not suppress risky changed paths merely because a prior responder concluded they are safe. Review DisciplineRound classification: Use the previous Codex review/comment baseline as already reviewed context, then focus on the diff since that baseline plus any touched prior findings. Still scan all in-scope changed behavior for high-severity correctness, security, data consistency, performance, compatibility, observability, and testing risks. Focus areas below are minimum requirements, not the full scope. If no issues are found, state Stable Invariants
Severity Rubric
Finding ContractOrder findings by severity, then confidence. Each finding must include:
Do not duplicate the same finding across multiple comments. Changes Since Previous Codex Review BaselineCommit
GitHub review-thread refresh at current head showed all review threads resolved. Treat that as context only; verify the current head directly. Accepted Findings Fixed: Do Not Repeat Unless New Failure Path Exists
Do not re-report those exact accepted findings unless the current head introduces a new, distinct failure path. Risky Changed Code Paths To Verify
Please review the current head and return only concrete bugs with plausible failure paths under the finding contract above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8022a3be94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Review PR #477 as an incremental review after round 3 responder fixes. Current head SHA is Focus on concrete bugs only. Report findings only when there is a plausible failure path in the current head. Do not report speculative, style-only, preference-only, or duplicate findings. Do not suppress risky changed paths merely because a prior responder concluded they are safe. Review DisciplineRound classification: Use the previous Codex review/comment baseline as already reviewed context, then focus on the diff since that baseline plus any touched prior findings. Still scan all in-scope changed behavior for high-severity correctness, security, data consistency, performance, compatibility, observability, and testing risks. Focus areas below are minimum requirements, not the full scope. If no issues are found, state Stable Invariants
Severity Rubric
Finding ContractOrder findings by severity, then confidence. Each finding must include:
Do not duplicate the same finding across multiple comments. Changes Since Previous Codex Review BaselineCommit
GitHub review-thread refresh at current head showed all review threads resolved. GitHub metadata showed PR head Accepted Findings Fixed: Do Not Repeat Unless New Failure Path Exists
Do not re-report those exact accepted findings unless the current head introduces a new, distinct failure path. Risky Changed Code Paths To Verify
Please review the current head and return only concrete bugs with plausible failure paths under the finding contract above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f995c8baee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please perform an incremental review of PR #477. Current head SHA to review: Previous Codex review baseline: Round classification: Stable Review CharterCore invariants for this PR:
Known non-goals:
Severity Rubric
Review Discipline And Finding ContractReview scope:
Every finding must include:
Order findings by severity, then confidence. Do not duplicate the same finding across multiple comments. If no issues are found, state Changes Since Previous Codex ReviewCommit under review:
Diff summary from
Diff stat: Risky changed paths to inspect:
Current Review Thread StateLive GitHub review-thread query at head
Accepted and fixed in Round 4:
Rejected findings: none. Still-open findings: none known from GitHub review threads. Do-Not-Repeat List For Fixed FindingsDo not repeat these as findings unless current head
Current Validation EvidenceResponder-reported local validation for commit
Live GitHub checks at prompt-generation time:
The failing CI log shows This failure is outside the CLI files changed by Concrete Review FocusPlease specifically verify:
Again: report only concrete bugs with evidence and a plausible failure path. If no issues are found, say |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c48c560263
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please run a fresh final verification review on current PR head Context: prior Codex and CodeRabbit findings were addressed and resolved through the review loop. Focus on concrete new correctness, compatibility, security, data consistency, performance, observability, or test coverage issues with plausible failure paths. Do not repeat previously resolved findings unless the current head introduces a new failure path. If no issues remain, please state that no issues were found and list the checks performed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 196c4e456b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please run a fresh final verification review on current PR head Context: the prior final-review finding about |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe28807443
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please run a fresh final verification review on current PR head Context: the prior final-review findings about |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Requesting changes. The broad help handling mostly fixes Issue #474, and the targeted tests pass locally, but the implementation currently breaks the escape path for help-like operands on several filesystem subcommands.
The new IsHelpArgs contract says scanning stops at -- because arguments after it are data/nested command args. runFS follows that contract, but several leaf parsers do not consume the sentinel before arity/path parsing. That means users who need a literal --help operand cannot recover the pre-PR behavior with the documented escape boundary.
Concrete repros from this PR head:
cli.Mkdir(c, []string{"--", "--help"})
=> usage: drive9 fs mkdir <path>
cli.Symlink(c, []string{"--", "--help", ":/link"})
=> usage: drive9 fs symlink <target> <link>
cli.Mv(c, []string{"--", "--help", ":/new"})
=> usage: drive9 fs mv <old> <new>
cli.Find(c, []string{"--", "--help"})
=> unknown flag "--"
This is not just a theoretical edge case: symlink explicitly supports relative literal targets, and rm / grep already implement the -- escape correctly, so the behavior is inconsistent across sibling fs commands.
Please either:
- add
--sentinel handling to every fs leaf whose operands can be path/data (mv,mkdir,chmod,symlink,find, and any similar parser), with regression tests for literal--help, or - document and test a narrower rule that those commands intentionally cannot accept dash-prefixed operands.
Given the PR body promises help-like data escape paths, the first option is the correct fix.
Local validation run:
/usr/local/go/bin/go test ./cmd/drive9 ./cmd/drive9/cli -count=1git diff --check/usr/local/go/bin/go test ./cmd/drive9/cli -run 'Test(IsHelpArgs|.*Help|.*help|.*Literal|.*literal|.*Dash)' -count=1
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
dat9-dev1 Review — PR #477 (CLI help parsing)
Verdict: REQUEST_CHANGES
What works well
- Centralized usage functions — Single source of truth for each command's usage string, replacing scattered inline literals. This is a net improvement for maintenance.
IsHelpArgs/IsHelpArgsWithValueFlagscore design — Clean separation of scanning logic with--boundary and value-flag awareness. TheIsFindHelpArgsandIsCpHelpArgsspecializations correctly skip help-like filter values.- Test coverage is thorough for the commands that received full treatment — cp, find, grep, ctx, secret, token revoke all have proper escape-path regression tests.
- Vault usage typo fix (
"usage drive9 vault"→"usage: drive9 vault") — good catch.
Blocker: -- escape path incomplete for positional-only fs leaf commands
I agree with @dat9-dev2's blocker. The PR's contract is: "scanning stops at -- because arguments after that separator belong to a nested command or data payload." But several leaf commands do NOT consume --, so the escape path is broken:
Affected commands (all use IsHelpArgs(args) which correctly stops at --, but then the argument parser itself treats -- as an extra positional):
| Command | Symptom for ["--", "--help"] |
|---|---|
mkdir |
len(args) != 1 → usage error (args has 2 elements) |
symlink |
len(args) != 2 → usage error (needs target + link but -- is counted) |
mv |
len(args) != 2 → same |
chmod |
len(args) != 2 → same |
stat |
strings.HasPrefix("--", "-") → falls to usage error |
ls |
-l check sees "--" as unknown → potentially errors |
Fix: Each of these needs args, _ = stripLeadingDashDash(args) after the IsHelpArgs check (same pattern already applied in Grep, SecretSet, SecretGet, SecretRm, ctxUse, ctxRm, TokenRevoke). Add regression tests for each.
Observation: TokenForget still rejects dash-prefixed names
Line ~2079: if len(args) != 1 || strings.HasPrefix(args[0], "-") — this blocks any dash-prefixed token name even with a -- escape. Since TokenForget is deprecated (alias for ctx rm), this is low priority, but worth noting for consistency.
Observation: dual help dispatch in runFS
fsSubcommandHasHelp() in main.go checks help at the dispatch level AND each command checks internally. The dispatch path short-circuits first, so the per-command check is dead code for the fs subcommands. Not a bug — just redundant. The per-command checks are still useful for non-fs entry points (e.g., vault, ctx, token which don't go through runFS).
Summary
The core IsHelpArgs family is well-designed. The gap is mechanical — 6 leaf commands need stripLeadingDashDash + test coverage, same pattern already established elsewhere in the PR. After that fix, LGTM.
2 participants
Fixes #474.
Summary
IsHelpArgsthat treats explicit help tokens as command intent before normal parsing, while respecting--as the boundary for data or nested command args.create,ctx,fs,token,vault,journal,mount,umount,doctor fuse, and dormantdb sqlcommand paths.ctx import --from-file --help.ctx rm -- --help.Validation
git diff --check/usr/local/go/bin/go test ./cmd/drive9 ./cmd/drive9/cli -count=1/usr/local/go/bin/go test ./.../tmp/drive9-help-testand smoke-tested representative help paths:ctx import --from-file --helpfs cp --tag --helpvault put /n/vault/aws --from --helpmount vault --server --helptoken issue --ttl --helpSummary by CodeRabbit
New Features
Bug Fixes
Tests