1.10.2#287
Conversation
📝 WalkthroughWalkthroughThis PR performs a comprehensive refactoring of CLI command handling to pass arguments explicitly instead of reading from global ChangesCLI Arguments Refactoring with HTTP Client Centralization
Documentation and Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/utils/validateargs.go (1)
77-86:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard empty argument before indexing
arg[0]to prevent panic.With iteration now starting at
args[2:], an empty positional argument can reach this branch and panic onarg[0].Proposed fix
if len(args) > 2 { for _, arg := range args[2:] { + if arg == "" { + return fmt.Errorf("%s", FormatError("Empty argument provided.\nOnly flags are allowed after declaring a Pokémon's name")) + } // Check for an empty flag after Pokémon's name if arg == "-" || arg == "--" { return fmt.Errorf("%s", FormatError(fmt.Sprintf("Empty flag '%s'.\nPlease specify valid flag(s).", arg))) } // Check if the argument after Pokémon's name is an attempted flag if arg[0] != '-' { return fmt.Errorf("%s", FormatError(fmt.Sprintf("Invalid argument '%s'.\nOnly flags are allowed after declaring a Pokémon's name", arg))) } } }🤖 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/utils/validateargs.go` around lines 77 - 86, The loop over args[2:] accesses arg[0] without checking for an empty string, which can panic; modify the loop in validateargs.go to first guard empty arguments (e.g., if arg == "" or len(arg) == 0) and return a formatted error via FormatError (similar style to the existing empty-flag check) before any access to arg[0]; keep the existing checks for "-" / "--" and the flag-character check afterwards so invalid or empty positional values are rejected safely.connections/connection.go (1)
157-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply bounded read pattern to CallTCGData to match the hardening pattern used elsewhere in the file.
Line 167 reads the response body without limits, risking memory exhaustion on oversized responses. The codebase already implements a bounded read pattern with
io.LimitReaderand validation (lines 107-113) that should be replicated here.Proposed fix
- body, err := io.ReadAll(resp.Body) + body, err := io.ReadAll(io.LimitReader(resp.Body, maxAPIResponseBytes+1)) if err != nil { return nil, fmt.Errorf("error reading response body: %w", err) } + if len(body) > maxAPIResponseBytes { + return nil, fmt.Errorf("response body exceeds %d bytes", maxAPIResponseBytes) + }🤖 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 `@connections/connection.go` around lines 157 - 168, The CallTCGData code reads resp.Body with io.ReadAll unbounded; mirror the bounded-read hardening used earlier (the pattern around the previous io.LimitReader usage) by wrapping resp.Body with io.LimitReader using the same max size constant (e.g., maxResponseBodySize or maxPayloadSize), read from that limited reader instead of resp.Body directly, and validate the returned byte length (error if it equals the limit implying truncation or exceeds allowed size). Update the logic in CallTCGData (around the httpClient.Do(req), resp.Body and subsequent io.ReadAll call) to use io.LimitReader and the same validation check used in the other function.
🤖 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/ability/ability_test.go`:
- Line 64: The test currently ignores the returned error from AbilityCommand, so
negative-path expectations (wantError) aren't asserted; update the table-driven
test to capture both outputs by changing the call to retrieve (output, err) :=
AbilityCommand(tt.args) and then assert err presence/absence matches
tt.wantError (and optionally assert error contents when wantError is true),
referring to the AbilityCommand call and the tt.wantError field in the test
table to implement the check.
In `@cmd/speed/speed_test.go`:
- Line 32: The test currently ignores the error returned from SpeedCommand (call
site: SpeedCommand in cmd/speed/speed_test.go) which can hide failures; update
the test to capture the error (e.g., output, err := SpeedCommand(tt.args)) and
assert or fail on it — for example use t.Fatalf/t.Errorf or the test helper
being used (require.NoError/assert.NoError) to fail the test when err != nil
before asserting on output so command failures don't produce false positives.
In `@cmd/types/types_test.go`:
- Line 38: The test currently discards the error from TypesCommand (output, _ :=
TypesCommand(tt.args)); change this to capture the error (output, err :=
TypesCommand(tt.args)) and assert it against the test case expectation
(tt.wantErr) instead of ignoring it: if tt.wantErr { assert.Error(t, err) /
require.Error(t, err) } else { assert.NoError(t, err) / require.NoError(t, err)
} and then continue asserting on output; update references to TypesCommand,
output, err and tt.wantErr in the test to perform these checks.
In `@cmd/types/types.go`:
- Line 41: The current derivation endpoint := strings.ToLower(args[0])[0:4] is
brittle and can panic on short args; replace this with a named constant (e.g.,
const endpoint = "xxxx") for the fixed endpoint and stop slicing args[0];
additionally add a short input validation that checks len(args) and len(args[0])
(or ensure args[0] exists) before using it to avoid panics, updating any
references to the local variable endpoint and the args usage in the surrounding
function to use the constant and the new guard.
In `@docs/Infrastructure_Guide/cloudflare-tunnel.md`:
- Around line 96-97: The docs hardcode /home/ubuntu for copying cloudflared
credentials; update the commands to use a generic home reference (e.g., $HOME or
~ or a variable like CLOUD_USER_HOME or use $(whoami) to build the path) and
reference the cloudflared source as ~/.cloudflared/cert.pem and
~/.cloudflared/*.json so the copy to /etc/cloudflared/ works for non‑Ubuntu or
differently named user accounts; ensure the example shows the variable usage
consistently for both cert.pem and the JSON files.
- Around line 51-54: The doc hardcodes the ARM64 binary URL
(cloudflared-linux-arm64) in the curl command which will fail on amd64 hosts;
update the install snippet to be architecture-neutral by detecting architecture
(e.g., use curl .../cloudflared-linux-$(uname -m) -o /usr/local/bin/cloudflared
or map uname -m to the correct release name) and ensure the target path
(/usr/local/bin/cloudflared) and curl -L usage remain the same so the same
command works for both x86_64 and aarch64 hosts.
In `@docs/Infrastructure_Guide/n8n.md`:
- Line 13: Fix the typos in the user-facing documentation by replacing the
misspelled tokens: change "reasoons" to "reasons", "sucess" to "success", and
"defaut" to "default" in the n8n.md content so the sentences read correctly and
clearly (search for those exact tokens to locate where to update).
- Around line 376-384: The GraphQL mutation in LaunchRun uses the outdated
signature launchRun(selector: $s) and JobOrPipelineSelector; update it to the
current Dagster shape by changing the mutation to accept executionParams (e.g.,
mutation LaunchRun($p: ExecutionParams!) { launchRun(executionParams: $p) {
__typename ... on LaunchRunSuccess { run { runId } } ... on PythonError {
message } } }) so callers use $p/ExecutionParams instead of
$s/JobOrPipelineSelector and the documented launchRun(executionParams: $p)
pattern.
---
Outside diff comments:
In `@cmd/utils/validateargs.go`:
- Around line 77-86: The loop over args[2:] accesses arg[0] without checking for
an empty string, which can panic; modify the loop in validateargs.go to first
guard empty arguments (e.g., if arg == "" or len(arg) == 0) and return a
formatted error via FormatError (similar style to the existing empty-flag check)
before any access to arg[0]; keep the existing checks for "-" / "--" and the
flag-character check afterwards so invalid or empty positional values are
rejected safely.
In `@connections/connection.go`:
- Around line 157-168: The CallTCGData code reads resp.Body with io.ReadAll
unbounded; mirror the bounded-read hardening used earlier (the pattern around
the previous io.LimitReader usage) by wrapping resp.Body with io.LimitReader
using the same max size constant (e.g., maxResponseBodySize or maxPayloadSize),
read from that limited reader instead of resp.Body directly, and validate the
returned byte length (error if it equals the limit implying truncation or
exceeds allowed size). Update the logic in CallTCGData (around the
httpClient.Do(req), resp.Body and subsequent io.ReadAll call) to use
io.LimitReader and the same validation check used in the other function.
🪄 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: 90725948-e9df-44bd-8cea-1a7699054d18
📒 Files selected for processing (49)
.github/workflows/ci.yml.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/poke_cli_dbt/dbt_project.ymlcard_data/pyproject.tomlcli.gocli_test.gocmd/ability/ability.gocmd/ability/ability_test.gocmd/berry/berry.gocmd/berry/berry_test.gocmd/berry/berryinfo.gocmd/card/card.gocmd/card/card_test.gocmd/card/cardinfo.gocmd/item/item.gocmd/item/item_test.gocmd/move/move.gocmd/move/move_test.gocmd/natures/natures.gocmd/natures/natures_test.gocmd/pokemon/pokemon.gocmd/pokemon/pokemon_test.gocmd/search/search.gocmd/search/search_test.gocmd/speed/speed.gocmd/speed/speed_test.gocmd/tcg/tcg.gocmd/tcg/tcg_test.gocmd/types/types.gocmd/types/types_test.gocmd/utils/output.gocmd/utils/output_test.gocmd/utils/validateargs.gocmd/utils/validateargs_test.goconnections/connection.goconnections/connection_test.godocs/Infrastructure_Guide/cloudflare-tunnel.mddocs/Infrastructure_Guide/n8n.mddocs/installation.mddocs/nginx.confflags/pokemonflagset.goflags/version.goflags/version_test.gomkdocs.ymlnfpm.yamltestdata/main_latest_flag.goldenweb/pyproject.toml
💤 Files with no reviewable changes (1)
- cli_test.go
| defer func() { os.Args = originalArgs }() | ||
|
|
||
| output, _ := AbilityCommand() | ||
| output, _ := AbilityCommand(tt.args) |
There was a problem hiding this comment.
Preserve error-path assertions in table tests.
Line 64 drops the returned err, so wantError is never enforced and negative-path regressions can pass unnoticed.
Suggested fix
- output, _ := AbilityCommand(tt.args)
+ output, err := AbilityCommand(tt.args)
cleanOutput := styling.StripANSI(output)
+
+ if (err != nil) != tt.wantError {
+ t.Errorf("AbilityCommand() error = %v, wantErr %v", err, tt.wantError)
+ return
+ }
assert.Equal(t, tt.expectedOutput, cleanOutput, "Output should match expected")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output, _ := AbilityCommand(tt.args) | |
| output, err := AbilityCommand(tt.args) | |
| cleanOutput := styling.StripANSI(output) | |
| if (err != nil) != tt.wantError { | |
| t.Errorf("AbilityCommand() error = %v, wantErr %v", err, tt.wantError) | |
| return | |
| } | |
| assert.Equal(t, tt.expectedOutput, cleanOutput, "Output should match expected") |
🤖 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/ability/ability_test.go` at line 64, The test currently ignores the
returned error from AbilityCommand, so negative-path expectations (wantError)
aren't asserted; update the table-driven test to capture both outputs by
changing the call to retrieve (output, err) := AbilityCommand(tt.args) and then
assert err presence/absence matches tt.wantError (and optionally assert error
contents when wantError is true), referring to the AbilityCommand call and the
tt.wantError field in the test table to implement the check.
| defer func() { os.Args = originalArgs }() | ||
|
|
||
| output, _ := SpeedCommand() | ||
| output, _ := SpeedCommand(tt.args) |
There was a problem hiding this comment.
Check the returned error in this test path.
Line 32 discards err, which can mask command failures and let output assertions pass for the wrong reason.
Suggested fix
- output, _ := SpeedCommand(tt.args)
+ output, err := SpeedCommand(tt.args)
+ if tt.wantError {
+ require.Error(t, err)
+ } else {
+ require.NoError(t, err)
+ }
cleanOutput := styling.StripANSI(output)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output, _ := SpeedCommand(tt.args) | |
| output, err := SpeedCommand(tt.args) | |
| if tt.wantError { | |
| require.Error(t, err) | |
| } else { | |
| require.NoError(t, err) | |
| } | |
| cleanOutput := styling.StripANSI(output) |
🤖 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/speed/speed_test.go` at line 32, The test currently ignores the error
returned from SpeedCommand (call site: SpeedCommand in cmd/speed/speed_test.go)
which can hide failures; update the test to capture the error (e.g., output, err
:= SpeedCommand(tt.args)) and assert or fail on it — for example use
t.Fatalf/t.Errorf or the test helper being used (require.NoError/assert.NoError)
to fail the test when err != nil before asserting on output so command failures
don't produce false positives.
| defer func() { os.Args = originalArgs }() | ||
|
|
||
| output, _ := TypesCommand() | ||
| output, _ := TypesCommand(tt.args) |
There was a problem hiding this comment.
Assert command errors instead of discarding them in tests.
Line 38 ignores err, which can hide failures and make output-only checks flaky.
Suggested fix
- output, _ := TypesCommand(tt.args)
+ output, err := TypesCommand(tt.args)
+ require.NoError(t, err)
cleanOutput := styling.StripANSI(output)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output, _ := TypesCommand(tt.args) | |
| output, err := TypesCommand(tt.args) | |
| require.NoError(t, err) | |
| cleanOutput := styling.StripANSI(output) |
🤖 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/types/types_test.go` at line 38, The test currently discards the error
from TypesCommand (output, _ := TypesCommand(tt.args)); change this to capture
the error (output, err := TypesCommand(tt.args)) and assert it against the test
case expectation (tt.wantErr) instead of ignoring it: if tt.wantErr {
assert.Error(t, err) / require.Error(t, err) } else { assert.NoError(t, err) /
require.NoError(t, err) } and then continue asserting on output; update
references to TypesCommand, output, err and tt.wantErr in the test to perform
these checks.
| } | ||
|
|
||
| endpoint := strings.ToLower(os.Args[1])[0:4] | ||
| endpoint := strings.ToLower(args[0])[0:4] |
There was a problem hiding this comment.
Avoid brittle slicing when deriving the endpoint.
Line 41 can panic on short input and obscures intent. Since this endpoint is fixed for this command, use a constant.
Suggested fix
- endpoint := strings.ToLower(args[0])[0:4]
+ endpoint := "type"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| endpoint := strings.ToLower(args[0])[0:4] | |
| endpoint := "type" |
🤖 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/types/types.go` at line 41, The current derivation endpoint :=
strings.ToLower(args[0])[0:4] is brittle and can panic on short args; replace
this with a named constant (e.g., const endpoint = "xxxx") for the fixed
endpoint and stop slicing args[0]; additionally add a short input validation
that checks len(args) and len(args[0]) (or ensure args[0] exists) before using
it to avoid panics, updating any references to the local variable endpoint and
the args usage in the surrounding function to use the constant and the new
guard.
| ```bash | ||
| sudo curl -L \ | ||
| https://github.com/cloudflare/cloudflared/releases/latest/download/cloudflared-linux-arm64 \ | ||
| -o /usr/local/bin/cloudflared |
There was a problem hiding this comment.
Use architecture-neutral install commands.
Line 49 says both ARM64 and x86_64 are supported, but Line 53 hardcodes the ARM64 binary URL. This will fail on amd64 hosts.
Suggested doc fix
-sudo curl -L \
- https://github.com/cloudflare/cloudflared/releases/latest/download/cloudflared-linux-arm64 \
- -o /usr/local/bin/cloudflared
+ARCH="$(uname -m)"
+if [ "$ARCH" = "x86_64" ]; then
+ CF_ARCH="amd64"
+elif [ "$ARCH" = "aarch64" ] || [ "$ARCH" = "arm64" ]; then
+ CF_ARCH="arm64"
+else
+ echo "Unsupported architecture: $ARCH" >&2
+ exit 1
+fi
+
+sudo curl -L \
+ "https://github.com/cloudflare/cloudflared/releases/latest/download/cloudflared-linux-${CF_ARCH}" \
+ -o /usr/local/bin/cloudflared🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 51-51: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 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 `@docs/Infrastructure_Guide/cloudflare-tunnel.md` around lines 51 - 54, The doc
hardcodes the ARM64 binary URL (cloudflared-linux-arm64) in the curl command
which will fail on amd64 hosts; update the install snippet to be
architecture-neutral by detecting architecture (e.g., use curl
.../cloudflared-linux-$(uname -m) -o /usr/local/bin/cloudflared or map uname -m
to the correct release name) and ensure the target path
(/usr/local/bin/cloudflared) and curl -L usage remain the same so the same
command works for both x86_64 and aarch64 hosts.
|
|
||
| ## Overview | ||
|
|
||
| n8n is used in this project for a few different reasoons such as performing API status checks, sending sucess/failure notifications, or ingesting data from sources that are not in a friendly format like a REST API. |
There was a problem hiding this comment.
Fix spelling mistakes in user-facing docs.
Line 13 has reasoons and sucess; Line 59 has defaut. These should be corrected for clarity.
Also applies to: 59-59
🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: Ensure spelling is correct
Context: ...sed in this project for a few different reasoons such as performing API status checks, s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~13-~13: Ensure spelling is correct
Context: ...s performing API status checks, sending sucess/failure notifications, or ingesting dat...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/Infrastructure_Guide/n8n.md` at line 13, Fix the typos in the
user-facing documentation by replacing the misspelled tokens: change "reasoons"
to "reasons", "sucess" to "success", and "defaut" to "default" in the n8n.md
content so the sentences read correctly and clearly (search for those exact
tokens to locate where to update).
| ```graphql | ||
| mutation LaunchRun($s: JobOrPipelineSelector!) { | ||
| launchRun(selector: $s) { | ||
| __typename | ||
| ... on LaunchRunSuccess { run { runId } } | ||
| ... on PythonError { message } | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
GraphQL mutation shape conflicts with the Dagster API version documented in this PR.
This snippet uses launchRun(selector: $s), but the Cloudflare guide in this PR (Line 184 onward) documents launchRun(executionParams: $p) for the current Dagster version. Keeping both patterns will lead to failed trigger calls depending on which guide users follow.
Suggested doc fix
-mutation LaunchRun($s: JobOrPipelineSelector!) {
- launchRun(selector: $s) {
+mutation LaunchRun($p: ExecutionParams!) {
+ launchRun(executionParams: $p) {
__typename
... on LaunchRunSuccess { run { runId } }
... on PythonError { message }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```graphql | |
| mutation LaunchRun($s: JobOrPipelineSelector!) { | |
| launchRun(selector: $s) { | |
| __typename | |
| ... on LaunchRunSuccess { run { runId } } | |
| ... on PythonError { message } | |
| } | |
| } | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 376-376: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 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 `@docs/Infrastructure_Guide/n8n.md` around lines 376 - 384, The GraphQL
mutation in LaunchRun uses the outdated signature launchRun(selector: $s) and
JobOrPipelineSelector; update it to the current Dagster shape by changing the
mutation to accept executionParams (e.g., mutation LaunchRun($p:
ExecutionParams!) { launchRun(executionParams: $p) { __typename ... on
LaunchRunSuccess { run { runId } } ... on PythonError { message } } }) so
callers use $p/ExecutionParams instead of $s/JobOrPipelineSelector and the
documented launchRun(executionParams: $p) pattern.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores