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 handling to the CLI (helpers and Before hooks) and enforces nil-configuration guards in consistency validation methods to fail early if 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)
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
ℹ️ 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
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>
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 {
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.