Conversation
Co-authored-by: HappyOnigiri <nodemeld+happy-onigiri@gmail.com>
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthrough実行単位での Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/auto_fixer.py (1)
1525-1537:max_per_run正規化処理はヘルパー化すると保守しやすくなります。
process_repoとmainで近い処理を持っているため、共通化しておくと将来の仕様変更時にズレを防げます。♻️ 例: 正規化ヘルパーに集約
+def _normalize_auto_resume_max_per_run(value: Any) -> int: + try: + parsed = int(value) + except (TypeError, ValueError): + return DEFAULT_CONFIG["coderabbit_auto_resume_max_per_run"] + return parsed if parsed >= 1 else DEFAULT_CONFIG["coderabbit_auto_resume_max_per_run"] + def process_repo( @@ - coderabbit_auto_resume_max_per_run = int( - runtime_config.get( - "coderabbit_auto_resume_max_per_run", - DEFAULT_CONFIG["coderabbit_auto_resume_max_per_run"], - ) - ) - if coderabbit_auto_resume_max_per_run < 1: - coderabbit_auto_resume_max_per_run = DEFAULT_CONFIG["coderabbit_auto_resume_max_per_run"] + coderabbit_auto_resume_max_per_run = _normalize_auto_resume_max_per_run( + runtime_config.get( + "coderabbit_auto_resume_max_per_run", + DEFAULT_CONFIG["coderabbit_auto_resume_max_per_run"], + ) + ) @@ - auto_resume_run_state = { - "posted": 0, - "max_per_run": int( - config.get( - "coderabbit_auto_resume_max_per_run", - DEFAULT_CONFIG["coderabbit_auto_resume_max_per_run"], - ) - ), - } + auto_resume_run_state = { + "posted": 0, + "max_per_run": _normalize_auto_resume_max_per_run( + config.get( + "coderabbit_auto_resume_max_per_run", + DEFAULT_CONFIG["coderabbit_auto_resume_max_per_run"], + ) + ), + }Also applies to: 2327-2335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1525 - 1537, The repeated normalization logic for coderabbit_auto_resume_max_per_run and auto_resume_run_state should be extracted into a small helper (e.g., normalize_auto_resume_state) and used from both process_repo and main to avoid duplication and drift; implement a function that accepts runtime_config, DEFAULT_CONFIG and optional auto_resume_run_state, enforces a minimum of 1 for coderabbit_auto_resume_max_per_run, fills defaults for "posted" and "max_per_run", and returns the normalized state and computed process_draft_prs flag (or compute process_draft_prs separately using runtime_config.get with DEFAULT_CONFIG["process_draft_prs"] as shown), then replace the inline block that uses coderabbit_auto_resume_max_per_run, auto_resume_run_state, and process_draft_prs with calls to this helper in both locations.tests/test_auto_fixer.py (1)
286-297:bool入力拒否の回帰テストも1本追加するとさらに堅牢です。実装側で
boolを明示的に弾いているため、その仕様をテストで固定化しておくのがおすすめです。🧪 追加例
+ def test_coderabbit_auto_resume_max_per_run_rejects_boolean(self, tmp_path): + config_file = tmp_path / "config.yaml" + config_file.write_text( + """ +coderabbit_auto_resume_max_per_run: true +repositories: + - repo: owner/repo1 +""".strip() + ) + with pytest.raises(SystemExit) as exc_info: + auto_fixer.load_config(str(config_file)) + assert exc_info.value.code == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auto_fixer.py` around lines 286 - 297, Add a regression test to assert that boolean values are rejected for coderabbit_auto_resume_max_per_run by creating a config file where coderabbit_auto_resume_max_per_run is set to a YAML boolean (e.g., true), calling auto_fixer.load_config(...) and asserting it raises SystemExit with code 1; place the new test alongside test_coderabbit_auto_resume_max_per_run_requires_positive_integer and reference the same load_config call to lock in the boolean-rejection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/auto_fixer.py`:
- Around line 1525-1537: The repeated normalization logic for
coderabbit_auto_resume_max_per_run and auto_resume_run_state should be extracted
into a small helper (e.g., normalize_auto_resume_state) and used from both
process_repo and main to avoid duplication and drift; implement a function that
accepts runtime_config, DEFAULT_CONFIG and optional auto_resume_run_state,
enforces a minimum of 1 for coderabbit_auto_resume_max_per_run, fills defaults
for "posted" and "max_per_run", and returns the normalized state and computed
process_draft_prs flag (or compute process_draft_prs separately using
runtime_config.get with DEFAULT_CONFIG["process_draft_prs"] as shown), then
replace the inline block that uses coderabbit_auto_resume_max_per_run,
auto_resume_run_state, and process_draft_prs with calls to this helper in both
locations.
In `@tests/test_auto_fixer.py`:
- Around line 286-297: Add a regression test to assert that boolean values are
rejected for coderabbit_auto_resume_max_per_run by creating a config file where
coderabbit_auto_resume_max_per_run is set to a YAML boolean (e.g., true),
calling auto_fixer.load_config(...) and asserting it raises SystemExit with code
1; place the new test alongside
test_coderabbit_auto_resume_max_per_run_requires_positive_integer and reference
the same load_config call to lock in the boolean-rejection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3698551b-36fc-43e4-b047-a0ed4f77a0f7
📒 Files selected for processing (5)
.refix.yaml.sampleREADME.ja.mdREADME.mdsrc/auto_fixer.pytests/test_auto_fixer.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/auto_fixer.py (2)
1707-1722: 冗長な.get()呼び出しについて(軽微な指摘)
_normalize_auto_resume_state呼び出し後はauto_resume_run_stateに"posted"と"max_per_run"キーが存在することが保証されています。そのため、Lines 1715-1716 と 1722 の.get(..., default)は冗長です。動作に問題はありませんが、より簡潔に書くことも可能です:
♻️ 簡潔な記述への提案
remaining_resume_posts=max( 0, - int(auto_resume_run_state.get("max_per_run", 1)) - - int(auto_resume_run_state.get("posted", 0)), + auto_resume_run_state["max_per_run"] + - auto_resume_run_state["posted"], ),if posted_resume_comment: - auto_resume_run_state["posted"] = int(auto_resume_run_state.get("posted", 0)) + 1 + auto_resume_run_state["posted"] += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1707 - 1722, auto_resume_run_state is guaranteed to contain "posted" and "max_per_run" after _normalize_auto_resume_state, so remove the redundant .get(...) defaults: in the call to _maybe_auto_resume_coderabbit_review replace int(auto_resume_run_state.get("max_per_run", 1)) and int(auto_resume_run_state.get("posted", 0)) with direct accesses like int(auto_resume_run_state["max_per_run"]) and int(auto_resume_run_state["posted"]), and when updating posted use auto_resume_run_state["posted"] = int(auto_resume_run_state["posted"]) + 1 instead of using .get(...).
137-162: 関数の責務の混在について(任意のリファクタ候補)
_normalize_auto_resume_stateは auto-resume state の正規化とprocess_draft_prsフラグの取得という2つの異なる責務を持っています。関数名から期待される動作と実際の戻り値に若干の乖離があります。現状でも動作に問題はありませんが、将来的にメンテナンス性を高めるため、以下のいずれかを検討してください:
- 関数名を
_normalize_pr_processing_stateなどに変更process_draft_prsの取得を別の場所に移動🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 137 - 162, The function _normalize_auto_resume_state presently mixes normalizing auto_resume_run_state and fetching process_draft_prs; either rename it to _normalize_pr_processing_state to reflect the dual responsibility, or split responsibilities by keeping _normalize_auto_resume_state to only return the normalized auto_resume_run_state (ensuring it still sets defaults for posted and max_per_run) and moving the process_draft_prs extraction into a new helper (e.g., get_process_draft_prs) that reads runtime_config/default_config; if you split, update all callers to handle the new return value (only auto_resume_run_state) and call the new helper for process_draft_prs, and adjust any type hints and tests that expect the old tuple return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/auto_fixer.py`:
- Around line 1707-1722: auto_resume_run_state is guaranteed to contain "posted"
and "max_per_run" after _normalize_auto_resume_state, so remove the redundant
.get(...) defaults: in the call to _maybe_auto_resume_coderabbit_review replace
int(auto_resume_run_state.get("max_per_run", 1)) and
int(auto_resume_run_state.get("posted", 0)) with direct accesses like
int(auto_resume_run_state["max_per_run"]) and
int(auto_resume_run_state["posted"]), and when updating posted use
auto_resume_run_state["posted"] = int(auto_resume_run_state["posted"]) + 1
instead of using .get(...).
- Around line 137-162: The function _normalize_auto_resume_state presently mixes
normalizing auto_resume_run_state and fetching process_draft_prs; either rename
it to _normalize_pr_processing_state to reflect the dual responsibility, or
split responsibilities by keeping _normalize_auto_resume_state to only return
the normalized auto_resume_run_state (ensuring it still sets defaults for posted
and max_per_run) and moving the process_draft_prs extraction into a new helper
(e.g., get_process_draft_prs) that reads runtime_config/default_config; if you
split, update all callers to handle the new return value (only
auto_resume_run_state) and call the new helper for process_draft_prs, and adjust
any type hints and tests that expect the old tuple return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb85e97e-21b8-4dbc-a0f0-5d7386d6c95f
📒 Files selected for processing (2)
src/auto_fixer.pytests/test_auto_fixer.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auto_fixer.py`:
- Around line 137-158: The code currently coerces the
"coderabbit_auto_resume_max_per_run" setting with int(), which silently rounds
floats like 1.5 to 1; update both load_config and _normalize_auto_resume_state
to perform strict validation instead: in load_config validate that the raw value
for "coderabbit_auto_resume_max_per_run" (if present) is an int and >= 1 and
raise a clear ValueError (or return a config error) for any other type/value (do
not coerce floats/strings); in _normalize_auto_resume_state (function
_normalize_auto_resume_state) remove the int(...) coercion, accept only an int
>= 1 for max_per_run (or if invalid/absent use
default_config["coderabbit_auto_resume_max_per_run"] after validating that
default is an int >= 1), and set auto_resume_run_state["max_per_run"] to the
validated integer; reference the key "coderabbit_auto_resume_max_per_run",
function name _normalize_auto_resume_state and load_config when applying these
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3ff5ebf-c73a-4ada-947e-118e09e2ca67
📒 Files selected for processing (1)
src/auto_fixer.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/auto_fixer.py (1)
149-154:⚠️ Potential issue | 🟠 Major
coderabbit_auto_resume_max_per_runの型検証が仕様とズレています
int(...)への強制変換で1.5や"2"が受理され、設定仕様(integer >= 1)に反して無効値を黙って通します。load_configと_normalize_auto_resume_stateの両方で、boolかつ非intを拒否する厳密検証に揃えてください。💡 修正案(厳密な整数検証)
def _normalize_auto_resume_state( runtime_config: dict[str, Any], default_config: dict[str, Any], auto_resume_run_state: dict[str, int] | None = None, ) -> dict[str, int]: """Normalize CodeRabbit auto-resume state.""" - max_per_run = int( - runtime_config.get( - "coderabbit_auto_resume_max_per_run", - default_config["coderabbit_auto_resume_max_per_run"], - ) - ) + max_per_run = runtime_config.get( + "coderabbit_auto_resume_max_per_run", + default_config["coderabbit_auto_resume_max_per_run"], + ) + if isinstance(max_per_run, bool) or not isinstance(max_per_run, int): + max_per_run = default_config["coderabbit_auto_resume_max_per_run"] if max_per_run < 1: max_per_run = default_config["coderabbit_auto_resume_max_per_run"] @@ coderabbit_auto_resume_max_per_run = parsed.get("coderabbit_auto_resume_max_per_run") if coderabbit_auto_resume_max_per_run is not None: - if isinstance(coderabbit_auto_resume_max_per_run, bool): + if ( + isinstance(coderabbit_auto_resume_max_per_run, bool) + or not isinstance(coderabbit_auto_resume_max_per_run, int) + ): print("Error: coderabbit_auto_resume_max_per_run must be an integer >= 1.", file=sys.stderr) sys.exit(1) - try: - parsed_max_per_run = int(coderabbit_auto_resume_max_per_run) - except (TypeError, ValueError): - print("Error: coderabbit_auto_resume_max_per_run must be an integer >= 1.", file=sys.stderr) - sys.exit(1) - if parsed_max_per_run < 1: + if coderabbit_auto_resume_max_per_run < 1: print("Error: coderabbit_auto_resume_max_per_run must be an integer >= 1.", file=sys.stderr) sys.exit(1) - config["coderabbit_auto_resume_max_per_run"] = parsed_max_per_run + config["coderabbit_auto_resume_max_per_run"] = coderabbit_auto_resume_max_per_run#!/bin/bash set -euo pipefail # 1) 実装上、int() 強制変換が残っている箇所を確認 rg -n -C2 'coderabbit_auto_resume_max_per_run|max_per_run = int\(|parsed_max_per_run = int\(' src/auto_fixer.py # 2) PyYAML + int() で float が丸め受理される挙動を再現 python - <<'PY' import yaml samples = [ "coderabbit_auto_resume_max_per_run: 1.5", "coderabbit_auto_resume_max_per_run: 1.0", ] for raw in samples: v = yaml.safe_load(raw)["coderabbit_auto_resume_max_per_run"] print(raw, "=>", repr(v), type(v).__name__, "int(v)=", int(v)) PYAlso applies to: 255-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 149 - 154, max_per_run の値を int(...) で強制変換する現在の実装は 1.5 や "2" を受け入れてしまうので、load_config と _normalize_auto_resume_state 内で行っている検証と同様に厳密な整数検証に変更してください: coderabbit_auto_resume_max_per_run を受け取る箇所で int(...) を使わず、値が int 型かつ not isinstance(value, bool) であることを確認し、かつ value >= 1 を満たさない場合は例外を投げるかデフォルトにフォールバックするようにし、max_per_run と parsed_max_per_run の割当はその検証済み整数を使うように修正してください。
🧹 Nitpick comments (1)
.refix.yaml.sample (1)
15-17: サンプル設定に最小値制約(min: 1)も明記してください実装は
coderabbit_auto_resume_max_per_run >= 1を必須にしているため、コメントにも最小値を明記しておくと誤設定を減らせます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.refix.yaml.sample around lines 15 - 17, Update the sample config comment for coderabbit_auto_resume_max_per_run to state the minimum allowed value (min: 1) so it matches the implementation requirement; locate the entry named coderabbit_auto_resume_max_per_run in the .refix.yaml.sample and modify the adjacent comment lines to include "min: 1" (and optionally a short note that the code enforces coderabbit_auto_resume_max_per_run >= 1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auto_fixer.py`:
- Around line 160-163: auto_resume_run_state の既存値を setdefault
で放置せず毎回再正規化してください:auto_resume_run_state の "posted" は存在しなければ 0 を、"max_per_run"
は現在の max_per_run の検証後に必ず代入するように置き換え(例: auto_resume_run_state["posted"] =
int(auto_resume_run_state.get("posted", 0)) と
auto_resume_run_state["max_per_run"] = validated_max_per_run のように)して、古い
max_per_run が残らないようにしてください。
---
Duplicate comments:
In `@src/auto_fixer.py`:
- Around line 149-154: max_per_run の値を int(...) で強制変換する現在の実装は 1.5 や "2"
を受け入れてしまうので、load_config と _normalize_auto_resume_state
内で行っている検証と同様に厳密な整数検証に変更してください: coderabbit_auto_resume_max_per_run を受け取る箇所で
int(...) を使わず、値が int 型かつ not isinstance(value, bool) であることを確認し、かつ value >= 1
を満たさない場合は例外を投げるかデフォルトにフォールバックするようにし、max_per_run と parsed_max_per_run
の割当はその検証済み整数を使うように修正してください。
---
Nitpick comments:
In @.refix.yaml.sample:
- Around line 15-17: Update the sample config comment for
coderabbit_auto_resume_max_per_run to state the minimum allowed value (min: 1)
so it matches the implementation requirement; locate the entry named
coderabbit_auto_resume_max_per_run in the .refix.yaml.sample and modify the
adjacent comment lines to include "min: 1" (and optionally a short note that the
code enforces coderabbit_auto_resume_max_per_run >= 1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc2158fe-da4c-4b41-a733-486866e91130
📒 Files selected for processing (3)
.refix.yaml.samplesrc/auto_fixer.pytests/test_auto_fixer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_auto_fixer.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/auto_fixer.py (2)
158-163:postedフィールドの型検証がmax_per_runと一貫していません
max_per_runには厳密な型チェック(Line 153)を適用していますが、postedにはint()強制変換を使用しています。postedは内部状態であり YAML から直接設定されないためリスクは低いですが、一貫性のために同様の検証を適用することを推奨します。♻️ 修正案(型検証の一貫性)
if auto_resume_run_state is None: auto_resume_run_state = {"posted": 0, "max_per_run": max_per_run} else: - auto_resume_run_state["posted"] = int(auto_resume_run_state.get("posted", 0)) + posted = auto_resume_run_state.get("posted", 0) + if isinstance(posted, bool) or not isinstance(posted, int) or posted < 0: + posted = 0 + auto_resume_run_state["posted"] = posted auto_resume_run_state["max_per_run"] = max_per_run🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 158 - 163, The code sets auto_resume_run_state["posted"] via int() coercion while max_per_run uses stricter validation; update the auto_resume_run_state handling so posted is validated the same way as max_per_run: if auto_resume_run_state is None initialize with posted=0 and max_per_run, otherwise explicitly check that auto_resume_run_state.get("posted") is an integer (or a numeric string that can be parsed) and within the allowed range (e.g. >= 0), raise a ValueError on invalid types/values, and then assign auto_resume_run_state["posted"] and auto_resume_run_state["max_per_run"] accordingly; reference the auto_resume_run_state dict and the keys "posted" and "max_per_run" when making the change.
1755-1764: 冗長なint()呼び出し
_normalize_auto_resume_stateが既に整数型を保証しているため、ここでのint()呼び出しは冗長です。防御的コーディングとして問題はありませんが、削除することでコードの明確さが向上します。♻️ 修正案
posted_resume_comment = _maybe_auto_resume_coderabbit_review( repo=repo, pr_number=pr_number, issue_comments=issue_comments, rate_limit_status=active_rate_limit, auto_resume_enabled=coderabbit_auto_resume_enabled, remaining_resume_posts=max( 0, - int(auto_resume_run_state["max_per_run"]) - - int(auto_resume_run_state["posted"]), + auto_resume_run_state["max_per_run"] + - auto_resume_run_state["posted"], ), dry_run=dry_run, summarize_only=summarize_only, ) if posted_resume_comment: - auto_resume_run_state["posted"] = int(auto_resume_run_state["posted"]) + 1 + auto_resume_run_state["posted"] += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1755 - 1764, The code redundantly calls int() on auto_resume_run_state fields that _normalize_auto_resume_state already ensures are ints; remove the int() wrappers in the remaining_resume_posts calculation and in the increment when a resume is posted (i.e., compute remaining_resume_posts with max(0, auto_resume_run_state["max_per_run"] - auto_resume_run_state["posted"]) and update auto_resume_run_state["posted"] = auto_resume_run_state["posted"] + 1), keeping the rest of the call-site (remaining_resume_posts, dry_run, summarize_only, and posted_resume_comment check) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/auto_fixer.py`:
- Around line 158-163: The code sets auto_resume_run_state["posted"] via int()
coercion while max_per_run uses stricter validation; update the
auto_resume_run_state handling so posted is validated the same way as
max_per_run: if auto_resume_run_state is None initialize with posted=0 and
max_per_run, otherwise explicitly check that auto_resume_run_state.get("posted")
is an integer (or a numeric string that can be parsed) and within the allowed
range (e.g. >= 0), raise a ValueError on invalid types/values, and then assign
auto_resume_run_state["posted"] and auto_resume_run_state["max_per_run"]
accordingly; reference the auto_resume_run_state dict and the keys "posted" and
"max_per_run" when making the change.
- Around line 1755-1764: The code redundantly calls int() on
auto_resume_run_state fields that _normalize_auto_resume_state already ensures
are ints; remove the int() wrappers in the remaining_resume_posts calculation
and in the increment when a resume is posted (i.e., compute
remaining_resume_posts with max(0, auto_resume_run_state["max_per_run"] -
auto_resume_run_state["posted"]) and update auto_resume_run_state["posted"] =
auto_resume_run_state["posted"] + 1), keeping the rest of the call-site
(remaining_resume_posts, dry_run, summarize_only, and posted_resume_comment
check) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1a8b23a-b787-471c-b7be-56598239d22a
📒 Files selected for processing (2)
.refix.yaml.samplesrc/auto_fixer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .refix.yaml.sample
Summary
@coderabbitai resumecomments posted perrefixexecution to prevent rate limit issues.Related issues
Changes
coderabbit_auto_resume_max_per_runconfiguration (default: 1, min: 1) to cap resume comments per run..refix.yaml.sample,README.md,README.ja.md).Testing
coderabbit_auto_resume_max_per_runsetting and its enforcement.make ci.Checklist
Summary by CodeRabbit
新機能
ドキュメント
テスト