1.10.2#287
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR refactors CLI commands to accept explicit args instead of reading global os.Args, centralizes HTTP clients with response-size limits, bumps the project version to v1.10.2 across manifests and docs, and adds infrastructure guides for Cloudflare Tunnel and n8n. ChangesCLI Arguments Refactoring with HTTP Client Centralization
Documentation and Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
This was
linked to
issues
May 13, 2026
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmd/natures/natures_test.go (1)
44-47: ⚡ Quick winPrefer
require.Errorfor expected-error branches.Using
require.Errorhere keeps behavior consistent with strict testifylint configurations and fails fast in the subtest.Suggested change
if tt.wantError { - assert.Error(t, err) + require.Error(t, err) } else { require.NoError(t, err) }🤖 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/natures/natures_test.go` around lines 44 - 47, In the test subtest branch that checks for expected errors (where tt.wantError is true), replace assert.Error(t, err) with require.Error(t, err) so the subtest fails fast and complies with strict testifylint rules; update the conditional in the test loop that currently reads "if tt.wantError { assert.Error(t, err) } else { require.NoError(t, err) }" to use require.Error for the true branch while keeping require.NoError in the false branch.cmd/pokemon/pokemon_test.go (1)
106-109: ⚡ Quick winUse
require.Errorin expected-error branch for consistency.This keeps table tests consistent with
require-first error assertion style and stricter testifylint setups.Suggested change
if tt.expectedError { - assert.Error(t, err) + require.Error(t, err) } else { require.NoError(t, err) }🤖 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/pokemon/pokemon_test.go` around lines 106 - 109, Replace the assert-based error assertion with a require one for consistency: in the test table branch that checks tt.expectedError, call require.Error(t, err) instead of assert.Error(t, err) so it matches the require.NoError(t, err) usage and satisfies stricter testifylint rules; update the assertion in the same test function where tt.expectedError and err are used.cmd/speed/speed_test.go (1)
33-36: ⚡ Quick winAlign expected-error assertion with
requirestyle.Switching to
require.Errorkeeps assertion style consistent with lint-enforced tests in this PR.Suggested change
if tt.wantError { - assert.Error(t, err) + require.Error(t, err) } else { require.NoError(t, err) }🤖 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` around lines 33 - 36, Replace the assert-style error check with the require-style to match the rest of the test: in the test block that inspects tt.wantError, change the call from assert.Error(t, err) to require.Error(t, err) so it aligns with require.NoError(t, err) and the lint-enforced assertion style used in these tests.
🤖 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/item/item_test.go`:
- Around line 63-66: Replace the non-fatal assertion with a fatal one: in the
test branch that checks tt.expectedError, change the call that uses assert.Error
to require.Error so failures abort the test immediately; locate the assertion
referencing tt.expectedError and replace assert.Error(...) with
require.Error(...) to satisfy the testifylint rule and prevent subsequent test
code from running on error.
In `@cmd/move/move_test.go`:
- Around line 51-54: Replace the non-fatal assertion with a fatal one so the
test halts on the expected-error path: change the call that currently uses
assert.Error(t, err) (inside the test table branch checking tt.expectedError) to
require.Error(t, err) so it matches the existing require.NoError path and
satisfies the testifylint require-error rule; ensure the surrounding if
tt.expectedError { ... } else { require.NoError(t, err) } structure remains
unchanged and still references the same variables (tt.expectedError, err).
---
Nitpick comments:
In `@cmd/natures/natures_test.go`:
- Around line 44-47: In the test subtest branch that checks for expected errors
(where tt.wantError is true), replace assert.Error(t, err) with require.Error(t,
err) so the subtest fails fast and complies with strict testifylint rules;
update the conditional in the test loop that currently reads "if tt.wantError {
assert.Error(t, err) } else { require.NoError(t, err) }" to use require.Error
for the true branch while keeping require.NoError in the false branch.
In `@cmd/pokemon/pokemon_test.go`:
- Around line 106-109: Replace the assert-based error assertion with a require
one for consistency: in the test table branch that checks tt.expectedError, call
require.Error(t, err) instead of assert.Error(t, err) so it matches the
require.NoError(t, err) usage and satisfies stricter testifylint rules; update
the assertion in the same test function where tt.expectedError and err are used.
In `@cmd/speed/speed_test.go`:
- Around line 33-36: Replace the assert-style error check with the require-style
to match the rest of the test: in the test block that inspects tt.wantError,
change the call from assert.Error(t, err) to require.Error(t, err) so it aligns
with require.NoError(t, err) and the lint-enforced assertion style used in these
tests.
🪄 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: f55e2a0a-fa57-427f-984b-44bd91aa6984
📒 Files selected for processing (8)
cmd/ability/ability_test.gocmd/item/item_test.gocmd/move/move_test.gocmd/natures/natures_test.gocmd/pokemon/pokemon_test.gocmd/speed/speed_test.gocmd/types/types_test.godocs/Infrastructure_Guide/n8n.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/types/types_test.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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores / Refactor