fix: handle flags placed after positional args in all commands#88
fix: handle flags placed after positional args in all commands#88
Conversation
📝 WalkthroughWalkthroughAdds interspersed-flag parsing and application to the CLI (helpers and Before hooks), updates many commands to use filtered positional args, and adds early nil-configuration guards in consistency Validate() methods to fail when global config is not loaded. Changes
Poem
🚥 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 unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `golangci-lint` configuration to improve the quality of Go code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f55064e-05a3-44d3-977f-f5148ea6881e
📒 Files selected for processing (3)
internal/cli/cli.gointernal/consistency/diff/table_diff.gointernal/consistency/mtree/merkle.go
rasifr
force-pushed
the
fix/ACE-171/flags-after-positional-args
branch
from
2f40218 to
1e14e38
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/consistency/mtree/merkle.go (1)
50-52: Consider adding nil guard toaceSchema()for defense-in-depth.This function directly accesses
config.Cfg.MTree.Schemaand is called from multiple methods (MtreeInit,BuildMtree, etc.) that don't go throughValidate(). While the CLI ensures config is loaded before these paths execute, a nil guard here would provide consistent protection:func aceSchema() string { + if config.Cfg == nil { + return "ace" // fallback default + } return config.Cfg.MTree.Schema }Alternatively, the callers could check
config.Cfgbefore invoking operations that depend on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/consistency/mtree/merkle.go` around lines 50 - 52, aceSchema() dereferences config.Cfg.MTree.Schema without guards; add nil checks inside aceSchema() to defend against missing config: ensure config.Cfg != nil and config.Cfg.MTree != nil before accessing Schema and return a safe default (e.g., empty string) if either is nil; keep callers (MtreeInit, BuildMtree, Validate) unchanged so they benefit from the centralized guard and optionally add a debug log inside aceSchema() when falling back to the default.
🤖 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/consistency/mtree/merkle.go`:
- Around line 50-52: aceSchema() dereferences config.Cfg.MTree.Schema without
guards; add nil checks inside aceSchema() to defend against missing config:
ensure config.Cfg != nil and config.Cfg.MTree != nil before accessing Schema and
return a safe default (e.g., empty string) if either is nil; keep callers
(MtreeInit, BuildMtree, Validate) unchanged so they benefit from the centralized
guard and optionally add a debug log inside aceSchema() when falling back to the
default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83cdfd26-e3a4-48fa-8e96-6ef873bca506
📒 Files selected for processing (3)
internal/cli/cli.gointernal/consistency/diff/table_diff.gointernal/consistency/mtree/merkle.go
|
|
||
| func TableDiffCLI(ctx *cli.Context) error { | ||
| // findFlag returns the flag definition matching name (including aliases). | ||
| func findFlag(flags []cli.Flag, name string) cli.Flag { |
There was a problem hiding this comment.
Most of this seems unnecessary and complicates things. Standard handling for go executables I believe is to process all of the flags first before the usual positional arguments. For example, ace table-diff -q public.test is ok but table-diff public.test -q isn't.
I think the only thing I think that this PR should have is this nil checks:
--- a/internal/consistency/diff/table_diff.go
+++ b/internal/consistency/diff/table_diff.go
@@ -727,6 +727,10 @@ func (t *TableDiffTask) Validate() error {
return fmt.Errorf("cluster_name and table_name are required arguments")
}
+ if config.Cfg == nil {
+ return fmt.Errorf("configuration not loaded")
+ }
+
if t.BlockSize > config.Cfg.TableDiff.MaxBlockSize && !t.OverrideBlockSize {
return fmt.Errorf("block row size should be <= %d", config.Cfg.TableDiff.MaxBlockSize)
}
diff --git a/internal/consistency/mtree/merkle.go b/internal/consistency/mtree/merkle.go
index ac9e84c..b8c6ce1 100644
--- a/internal/consistency/mtree/merkle.go
+++ b/internal/consistency/mtree/merkle.go
@@ -1095,6 +1095,11 @@ func (m *MerkleTreeTask) Validate() error {
if m.Mode == "listen" {
return nil
}
+
+ if config.Cfg == nil {
+ return fmt.Errorf("configuration not loaded")
+ }
+
cfg := config.Cfg.MTree.Diff
if m.BlockSize != 0 && !m.OverrideBlockSize {
There was a problem hiding this comment.
The original bug report was a panic (nil pointer dereference) when running ace table-diff public.test -h. Keeping only the nil guard fixes the crash, but ace table-diff public.test -h would then silently return nothing — which is still wrong. A user who types a valid subcommand, a valid table name, and a valid flag deserves either help output or a clear error, not silence.
Go's stdlib flag package stopping at the first non-flag argument is a limitation of the stdlib, not a CLI convention. Many utilities users interact with daily support flags after positional arguments:
grep 'ace' README.md -n # works as expected
psql mydb -c "SELECT 1" # works as expected
git show HEAD --stat # works as expected
psql postgres -p 5431 --help # works as expectedAt minimum, the command should produce helpful output or a clear error message. Silent return creates confusing UX and makes troubleshooting harder. The nil guard fixes the crash but does not fully address the usability issue that triggered the report.
Go's flag.FlagSet stops parsing at the first non-flag argument, so flags appearing after a positional arg (e.g. "table-diff public.test -q") were silently misinterpreted as cluster or table names, and -h/--help caused a nil pointer panic when config was not loaded. Add applyInterspersedFlags() to detect and apply stranded flags via ctx.Set() before the action runs, and filteredPositionalArgs() to strip flag-like tokens from the positional arg list. All command Before hooks now call applyInterspersedFlags(), and all action closures and CLI functions use filteredPositionalArgs() for arg resolution. Help flags (-h/--help) show command help via ctx.Lineage()[1] and return early. Also add nil guards on config.Cfg in TableDiffTask.Validate() and MerkleTreeTask.Validate() to prevent panics when config is not loaded. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…onal arg urfave/cli validates Required flags before the Before hook runs, which means applyInterspersedFlags never gets a chance to apply --diff-file when it is placed after the positional argument. Both table-repair and table-rerun were affected. Removing Required:true is safe — both commands already validate an empty DiffFilePath at the application level (table_repair.go Validate() and table_rerun.go CheckDiffFileFormat()) and return clear error messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers all QA scenarios: flags before/after positional args, --key=value form, short aliases, unknown/missing flag errors, help flags, -- separator, and positional arg count correctness. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rasifr
force-pushed
the
fix/ACE-171/flags-after-positional-args
branch
from
1e14e38 to
1d2dfc3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cli/cli.go (1)
445-455: Consider extracting a sharedBeforehook helper.The repeated
applyInterspersedFlags+ debug-level setup block is duplicated across many commands; a single helper would reduce drift risk.Also applies to: 468-478, 509-519, 532-542, 560-569, 583-592, 610-619, 637-646, 664-673, 687-696, 710-719, 737-746, 764-773, 791-800, 818-827
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli.go` around lines 445 - 455, Extract the duplicated Before hook into a single helper function (e.g. sharedBefore(ctx *cli.Context) error) that calls applyInterspersedFlags(ctx) and sets logger level based on ctx.Bool("debug") (use logger.SetLevel(log.DebugLevel) or logger.SetLevel(log.InfoLevel)), return any error from applyInterspersedFlags; then replace each inline Before anonymous func in the command definitions with a reference to this helper (e.g. Before: sharedBefore) so all commands (the instances now using the anonymous func around applyInterspersedFlags and logger.SetLevel) share the same implementation.internal/cli/cli_test.go (1)
329-338: Add a no---diff-filenegative test case.Given the flag is no longer CLI-required, adding an explicit failure test for missing
diff-filewould lock in expected enforcement behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 329 - 338, Add a negative test case to the existing table-driven tests (the same slice containing the case with tc "Finding-1 diff-file flag after positional arg"): create a new entry (e.g., tc: "Finding-1 no diff-file negative") with args set to []string{"public.orders"} and flags containing the diff-file flag (&cli.StringFlag{Name: "diff-file", Aliases: []string{"f"}}), then assert that running the CLI/parser or calling the command's Validate() for that test returns an error (expect a validation failure) rather than succeeding; reference the existing fields wantPositional/wantStrings in the table and add a flag such as wantError or expectValidateError to signal the test should fail validation.
🤖 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/cli_test.go`:
- Around line 329-338: Add a negative test case to the existing table-driven
tests (the same slice containing the case with tc "Finding-1 diff-file flag
after positional arg"): create a new entry (e.g., tc: "Finding-1 no diff-file
negative") with args set to []string{"public.orders"} and flags containing the
diff-file flag (&cli.StringFlag{Name: "diff-file", Aliases: []string{"f"}}),
then assert that running the CLI/parser or calling the command's Validate() for
that test returns an error (expect a validation failure) rather than succeeding;
reference the existing fields wantPositional/wantStrings in the table and add a
flag such as wantError or expectValidateError to signal the test should fail
validation.
In `@internal/cli/cli.go`:
- Around line 445-455: Extract the duplicated Before hook into a single helper
function (e.g. sharedBefore(ctx *cli.Context) error) that calls
applyInterspersedFlags(ctx) and sets logger level based on ctx.Bool("debug")
(use logger.SetLevel(log.DebugLevel) or logger.SetLevel(log.InfoLevel)), return
any error from applyInterspersedFlags; then replace each inline Before anonymous
func in the command definitions with a reference to this helper (e.g. Before:
sharedBefore) so all commands (the instances now using the anonymous func around
applyInterspersedFlags and logger.SetLevel) share the same implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d8370b0-4c6c-4548-950c-257bc01b57c7
📒 Files selected for processing (4)
internal/cli/cli.gointernal/cli/cli_test.gointernal/consistency/diff/table_diff.gointernal/consistency/mtree/merkle.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/consistency/diff/table_diff.go
2 participants
Go's flag.FlagSet stops parsing at the first non-flag argument, so flags appearing after a positional arg (e.g. "table-diff public.test -q") were silently misinterpreted as cluster or table names, and -h/--help caused a nil pointer panic when config was not loaded.
Add applyInterspersedFlags() to detect and apply stranded flags via ctx.Set() before the action runs, and filteredPositionalArgs() to strip flag-like tokens from the positional arg list. All command Before hooks now call applyInterspersedFlags(), and all action closures and CLI functions use filteredPositionalArgs() for arg resolution. Help flags (-h/--help) show command help via ctx.Lineage()[1] and return early. Also add nil guards on config.Cfg in TableDiffTask.Validate() and MerkleTreeTask.Validate() to prevent panics when config is not loaded.