Merged
Conversation
Co-authored-by: HappyOnigiri <nodemeld+happy-onigiri@gmail.com>
|
Cursor Agent can help with this pull request. Just |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefix にオプション Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
📝 Coding Plan for PR comments
Comment |
![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: 2
🤖 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 1648-1654: The early return in _mark_pr_merged_label_if_needed
currently aborts when "merged" is not enabled; change the logic to instead check
whether any of the post-merge labels {"running", "auto_merge_requested",
"merged"} are enabled via _resolve_enabled_pr_label_keys — if none are enabled
return False, otherwise continue and call _set_pr_merged_label(repo, pr_number,
...) but pass whether "merged" specifically should be added; this ensures
removal of refix:running / refix:auto-merge-requested still runs even when
"merged" is disabled.
- Around line 1716-1721: The current backfill early-return requires both "done"
and "auto_merge_requested" when "merged" is enabled, which prevents valid combos
like ["done","merged"] or ["auto_merge_requested","merged"] from matching;
change the logic in the block that calls _resolve_enabled_pr_label_keys so that
after confirming "merged" is present you require at least one of "done" or
"auto_merge_requested" (i.e. if neither is present return 0), and build
search_query dynamically to include label filters only for the labels present in
enabled (use REFIX_DONE_LABEL and REFIX_AUTO_MERGE_REQUESTED_LABEL
conditionally, always exclude REFIX_MERGED_LABEL) so the candidate selection
matches the same assumptions used by _mark_pr_merged_label_if_needed().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1b26440-c88c-4889-8bdc-1c625d87ba36
📒 Files selected for processing (5)
.refix.yaml.sampleREADME.ja.mdREADME.mdsrc/auto_fixer.pytests/test_auto_fixer.py
…g/auto_merge_requested を除去できるよう修正 「merged」が無効でも running/auto_merge_requested が有効なら _set_pr_merged_label() を呼び出す。
…の片方だけ有効な場合も動作するよう修正 条件を OR に変更し、search_query を有効なラベルのみで動的構築する。
Owner
Author
🤖 Refix Status対応済みレビュー一覧
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/auto_fixer.py (1)
1787-1819:⚠️ Potential issue | 🟠 Major
auto_merge_requested単独系の構成で、ラベル未作成のまま付与を試みています。Line 1558-1559 の early return により
_set_pr_done_label()から_ensure_refix_labels()が一度も走らないため、enabled_pr_labels: ["auto_merge_requested"]や["auto_merge_requested", "merged"]では Line 1800-1806 / Line 1813-1819 が未作成のrefix:auto-merge-requestedをgh pr edit --add-labelしに行きます。auto-merge 自体は成功していても、成功パスではmerge_requested=Falseになり、already merged パスでもauto_merge_requested前提の merged 判定が崩れるので、この経路でも先に_ensure_refix_labels()を呼ぶ必要があります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1787 - 1819, The function _trigger_pr_auto_merge skips _ensure_refix_labels() due to an early return so it can attempt to add REFIX_AUTO_MERGE_REQUESTED_LABEL before that label exists (breaking auto_merge_requested-only configs and causing merge_requested to be false on success); fix by calling _ensure_refix_labels(repo, enabled_pr_label_keys) before any _edit_pr_label calls (e.g. at the start of _trigger_pr_auto_merge or immediately after a successful gh merge and in the "already merged" branch) so labels are created first, then perform the _edit_pr_label(...) and return its result so merge_requested reflects the real state.
🤖 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 1716-1727: The current backfill gate in the block using
_resolve_enabled_pr_label_keys(enabled_pr_label_keys) incorrectly requires
"merged" and only builds search_parts when "done" or "auto_merge_requested" are
present, which mismatches _mark_pr_merged_label_if_needed and misses valid
configs like ["auto_merge_requested"] or ["merged"] alone; update this block to
follow the same preconditions as _mark_pr_merged_label_if_needed (i.e., do not
require "merged" to be present before proceeding) or alternatively enforce and
reject invalid combinations earlier in load_config(); specifically, align the
enabled checks around enabled_pr_label_keys/enabled, build search_parts based on
the presence of REFIX_DONE_LABEL and REFIX_AUTO_MERGE_REQUESTED_LABEL regardless
of "merged", always include the negative REFIX_MERGED_LABEL filter, and/or add
validation in load_config() to disallow unsupported label combinations so search
and backfill logic and _mark_pr_merged_label_if_needed() share the same
assumptions.
- Around line 1506-1510: The code currently lets setter calls that become no-ops
still cause callers to unconditionally increment modified_prs (via
modified_prs.add(...)), wasting max_modified_prs_per_run; change the
setter/update methods (the ones that were adjusted to return early in the
enabled-label path around the _resolve_enabled_pr_label_keys logic) so they
return a boolean or truthy value indicating whether they actually changed the
PR, then update every site that currently does modified_prs.add(...)
unconditionally to capture that return value and only add/count the PR when the
setter returned True; in short, make the setter explicitly return "modified" and
guard the modified_prs.add(...) calls with that flag at the call sites that
currently call modified_prs.add(...) unconditionally.
---
Outside diff comments:
In `@src/auto_fixer.py`:
- Around line 1787-1819: The function _trigger_pr_auto_merge skips
_ensure_refix_labels() due to an early return so it can attempt to add
REFIX_AUTO_MERGE_REQUESTED_LABEL before that label exists (breaking
auto_merge_requested-only configs and causing merge_requested to be false on
success); fix by calling _ensure_refix_labels(repo, enabled_pr_label_keys)
before any _edit_pr_label calls (e.g. at the start of _trigger_pr_auto_merge or
immediately after a successful gh merge and in the "already merged" branch) so
labels are created first, then perform the _edit_pr_label(...) and return its
result so merge_requested reflects the real state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2b7afb5-af73-475a-bfe3-860e692417c6
📒 Files selected for processing (1)
src/auto_fixer.py
…_edit_pr_label を実行していた問題を修正 auto_merge_requested 単独など merged が無効な設定で refix:auto-merge-requested ラベルが 未作成のまま gh pr edit --add-label を実行し失敗していた。 merge 成功・already merged の各パスで _ensure_refix_labels を呼ぶよう修正した。
…ed_prs_per_run を消費する問題を修正 _set_pr_running_label / _set_pr_done_label / _update_done_label_if_completed が no-op 早期リターンする場合に False を返すよう変更し、各呼び出し元で戻り値が True のときのみ modified_prs.add() を実行するようにした。 enabled_pr_labels: [] など一部ラベルが無効な設定で上限が不当に消費される問題を解消する。
…requested 無効の設定でスキップされる問題を修正
line 1717 の guard を "merged" 必須から {"auto_merge_requested","merged"} いずれか存在に緩和し、
["auto_merge_requested"] や ["running","auto_merge_requested"] でも backfill が動作するようにした。
line 1719 の guard に "running" を追加し ["running","merged"] でも動作するよう修正した。
search_parts の構築にも "running" ラベルを fallback として追加した。
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/auto_fixer.py (1)
2957-2975:⚠️ Potential issue | 🟠 Majorこの分岐だけ
modified_prs集計が抜けています
_update_done_label_if_completed()は変更有無を返すようになっていますが、Line 2957 の分岐では戻り値を捨てています。レビュー対象がなく、CI 修復や base 取り込みだけを処理した PR でrunning/doneが変わってもmax_modified_prs_per_runに計上されず、上限を超えて後続 PR のラベル更新が走ります。修正案
- _update_done_label_if_completed( + if _update_done_label_if_completed( repo=repo, pr_number=pr_number, has_review_targets=False, review_fix_started=review_fix_started, review_fix_added_commits=review_fix_added_commits, @@ summarize_only=summarize_only, auto_merge_enabled=auto_merge_enabled, coderabbit_rate_limit_active=bool(active_rate_limit), coderabbit_review_failed_active=bool(active_review_failed), enabled_pr_label_keys=enabled_pr_label_keys, - ) + ): + modified_prs.add((repo, pr_number))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 2957 - 2975, The call to _update_done_label_if_completed currently discards its boolean/changed return value; capture that return (e.g., changed = _update_done_label_if_completed(...)) and, when True, increment the modified_prs counter (the same counter used to enforce max_modified_prs_per_run) so this branch contributes to the modified PRs tally; update any surrounding logic that checks modified_prs to respect the increment and ensure the auto_merge_enabled/dry_run flags are still passed through unchanged.
♻️ Duplicate comments (1)
src/auto_fixer.py (1)
1718-1729:⚠️ Potential issue | 🟠 Major
enabled_pr_labels: ["running"]で merge 後の cleanup が止まります
load_config()は["running"]を受け付けていて、_mark_pr_merged_label_if_needed()もrunning単独を後処理対象にしていますが、Line 1719 の gate からrunningが漏れているため、この構成では_backfill_merged_labels()が常に 0 件で終わります。結果として、auto-merge 済み PR のrefix:runningが次回実行でも外れません。修正案
- if not ({"auto_merge_requested", "merged"} & enabled): + if not ({"running", "auto_merge_requested", "merged"} & enabled): return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1718 - 1729, The initial gate incorrectly requires {"auto_merge_requested","merged"} to intersect enabled, which excludes the case where enabled_pr_label_keys == ["running"]; update the gate in the beginning of the block (around the callsites of _resolve_enabled_pr_label_keys and before building search_parts used by _backfill_merged_labels) to also allow when "running" is present (i.e., check that enabled contains at least one of "auto_merge_requested","merged","running"), so that _backfill_merged_labels will proceed for configurations that only enable "running" and will use REFIX_RUNNING_LABEL / REFIX_AUTO_MERGE_REQUESTED_LABEL /REFIX_DONE_LABEL as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/auto_fixer.py`:
- Around line 2957-2975: The call to _update_done_label_if_completed currently
discards its boolean/changed return value; capture that return (e.g., changed =
_update_done_label_if_completed(...)) and, when True, increment the modified_prs
counter (the same counter used to enforce max_modified_prs_per_run) so this
branch contributes to the modified PRs tally; update any surrounding logic that
checks modified_prs to respect the increment and ensure the
auto_merge_enabled/dry_run flags are still passed through unchanged.
---
Duplicate comments:
In `@src/auto_fixer.py`:
- Around line 1718-1729: The initial gate incorrectly requires
{"auto_merge_requested","merged"} to intersect enabled, which excludes the case
where enabled_pr_label_keys == ["running"]; update the gate in the beginning of
the block (around the callsites of _resolve_enabled_pr_label_keys and before
building search_parts used by _backfill_merged_labels) to also allow when
"running" is present (i.e., check that enabled contains at least one of
"auto_merge_requested","merged","running"), so that _backfill_merged_labels will
proceed for configurations that only enable "running" and will use
REFIX_RUNNING_LABEL / REFIX_AUTO_MERGE_REQUESTED_LABEL /REFIX_DONE_LABEL as
before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08cf115f-c747-48f1-b5d7-3fd53a69cfb1
📒 Files selected for processing (1)
src/auto_fixer.py
…ng のみ有効な設定でもラベルクリーンアップが動作するよう修正
…ubprocess.run を呼ぶ問題を修正
第1ゲートチェックを {running, auto_merge_requested, merged} & enabled から
'merged' not in enabled に変更し、merged ラベルが無効な場合は即座にスキップするよう修正。
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/auto_fixer.py (1)
1718-1722:⚠️ Potential issue | 🟠 Major
merged無効時に merge 後のラベル掃除まで止まっています。
enabled_pr_labels: ["auto_merge_requested"]や["running", "auto_merge_requested"]では、PR が後で merge された後に open PR 側の処理へ戻れないので、この backfill だけがrefix:auto-merge-requested/refix:runningを外せます。ここで"merged"を必須にすると、stale ラベルが merged PR に残り続けます。修正案
- if "merged" not in enabled: + if not ({"running", "auto_merge_requested", "merged"} & enabled): return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1718 - 1722, Remove the unconditional "merged" requirement that blocks the backfill: in src/auto_fixer.py where enabled = _resolve_enabled_pr_label_keys(enabled_pr_label_keys) is computed, delete (or stop enforcing) the if "merged" not in enabled: return 0 check so that the subsequent logic (the check that returns 0 when none of "done", "auto_merge_requested", or "running" are enabled) remains the only early-exit; this lets backfill run for configs like ["auto_merge_requested"] or ["running","auto_merge_requested"] and still clean labels on merged PRs.
🤖 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 1506-1545: The flow currently treats any attempted label edit as
success and returns True even if underlying gh pr edit failed; change
_edit_pr_label to return a boolean success and update this function to call
_edit_pr_label and track whether any call actually succeeded (for both removing
REFIX_DONE_LABEL and adding REFIX_RUNNING_LABEL). Only return True if at least
one _edit_pr_label call returned True; propagate failures (do not increment
modified_prs when no edits succeeded). Apply the same pattern to the other
similar blocks referenced (lines around 1556-1597, 1603-1637, 1703-1708) and use
the existing symbols _resolve_enabled_pr_label_keys, _pr_has_label,
_ensure_refix_labels, REFIX_RUNNING_LABEL, REFIX_DONE_LABEL to locate and update
the logic.
---
Duplicate comments:
In `@src/auto_fixer.py`:
- Around line 1718-1722: Remove the unconditional "merged" requirement that
blocks the backfill: in src/auto_fixer.py where enabled =
_resolve_enabled_pr_label_keys(enabled_pr_label_keys) is computed, delete (or
stop enforcing) the if "merged" not in enabled: return 0 check so that the
subsequent logic (the check that returns 0 when none of "done",
"auto_merge_requested", or "running" are enabled) remains the only early-exit;
this lets backfill run for configs like ["auto_merge_requested"] or
["running","auto_merge_requested"] and still clean labels on merged PRs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc3be148-53ee-409e-8831-fb8b2a6d2206
📒 Files selected for processing (1)
src/auto_fixer.py
Both branches added new tests at the same location after test_auto_merge_requires_boolean. Keep all four tests: - test_enabled_pr_labels_can_be_subset (HEAD) - test_enabled_pr_labels_can_be_empty (HEAD) - test_enabled_pr_labels_must_be_known_values (HEAD) - test_execution_report_requires_boolean (origin/main)
enabled_pr_labels に "running" や "auto_merge_requested" のみが含まれる場合も バックフィルが動作し、マージ済み PR のステールラベルを削除できるようにする。 Fixes: r3935883603
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/auto_fixer.py (1)
1622-1646:⚠️ Potential issue | 🟠 Majorラベル更新失敗を成功扱いしないでください。
Line 1646 と Line 1698 は
_edit_pr_label()の成否を見ずにTrueを返し、Line 1809 も_set_pr_merged_label()の結果を見ずにTrueを返しています。これだとgh pr editが失敗して実際には何も変わっていなくても、呼び出し側がmodified_prsや backfill 件数を加算してmax_modified_prs_per_runを誤消費します。ここは各 helper が「変更を試みたか」ではなく「少なくとも 1 件のラベル変更に成功したか」を返すようにそろえる必要があります。Also applies to: 1672-1698, 1701-1738, 1804-1809
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdd51440-fc2e-4468-ae3f-c8b3a03e8e1c
📒 Files selected for processing (5)
.refix.yaml.sampleREADME.ja.mdREADME.mdsrc/auto_fixer.pytests/test_auto_fixer.py
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- .refix.yaml.sample
- README.ja.md
_set_pr_running_label / _set_pr_done_label / _set_pr_merged_label が _edit_pr_label の戻り値を無視して常に True を返していた問題を修正。 少なくとも 1 件のラベル変更に成功した場合のみ True を返すようにし、 modified_prs / backfill カウントの誤加算を防ぐ。 Fixes: discussion_r2924023703
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/auto_fixer.py (3)
1908-1951:⚠️ Potential issue | 🟠 Major
_trigger_pr_auto_merge()の戻り値が 1 つの bool では足りません。この値を「merge 要求が通った/既に merged」と「この run で PR を変更した」の両方に使っているため、
auto_merge_requested無効時のalready mergedは無変更でも modified 扱いになり、逆に merge 要求自体は成功したのにラベル付与だけ失敗したケースはFalseになって caller 側の件数集計と merged 後処理判定が両方ずれます。merge_state_reachedとmodifiedは分けて返さないと、この分岐は正しく扱えません。Also applies to: 2468-2487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1908 - 1951, The function _trigger_pr_auto_merge currently returns a single bool combining "merge_state_reached" (auto-merge requested or PR already merged) and "modified" (we changed PR labels), which causes caller miscounts; change _trigger_pr_auto_merge to return a tuple (merge_state_reached: bool, modified: bool), where merge_state_reached is True when the GH merge command succeeded or the PR is already merged, and modified is True only when _edit_pr_label actually changed labels (i.e., when you call _edit_pr_label and it returns True); update all callers (places that call _trigger_pr_auto_merge, including the duplicate site referenced) to unpack both values and use merge_state_reached for merged-state logic and modified for label-change counting/side effects.
1557-1560:⚠️ Potential issue | 🟠 Major
_set_pr_merged_label()が無効ラベルの skip をchanged=Trueに数えています。
_edit_pr_label()は無効ラベルをTrueで short-circuit するので、ここで 3 ラベルを無条件に流すとenabled_pr_label_keys={"auto_merge_requested"}などで実際の remove/add が失敗してもchangedだけが立ちます。結果として backfill 件数やmodified_prsが進み、ラベルは残ったままになります。Also applies to: 1711-1756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1557 - 1560, The _set_pr_merged_label() path is counting skips for disabled labels as changed, causing _edit_pr_label() short-circuiting to report changed=True even when no remove/add occurred; update the logic around enabled = _resolve_enabled_pr_label_keys(enabled_pr_label_keys) and the PR_LABEL_NAME_TO_KEY lookup so that when label_key is not in enabled you do not set or propagate changed=True (i.e., return False or skip without marking change), and mirror the same fix in the duplicate block (lines covering the 1711-1756 logic) so disabled labels are skipped without incrementing changed/modified_prs.
340-366:⚠️ Potential issue | 🟠 Major
enabled_pr_labels: ["merged"]はこのままだと実質サポートされません。ここでは
merged単独構成を受け入れていますが、後段の_backfill_merged_labels()はrunning / done / auto_merge_requestedが 1 つも無いと候補抽出前に0を返します。遅延 merge の主経路ではrefix:mergedを付ける導線がなく、設定だけ通って実際には動かない状態になります。ここで不正構成として弾かないと、利用者が壊れた設定を保存できてしまいます。🔧 最小修正案
config["enabled_pr_labels"] = normalized_enabled_labels + enabled_label_set = set(normalized_enabled_labels) + if config["auto_merge"] and "merged" in enabled_label_set and not ( + {"running", "done", "auto_merge_requested"} & enabled_label_set + ): + print( + "Error: enabled_pr_labels must include at least one of running, done, or auto_merge_requested when merged is enabled with auto_merge.", + file=sys.stderr, + ) + sys.exit(1)Also applies to: 1835-1839
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 340 - 366, The current parsing for config["enabled_pr_labels"] accepts "merged" alone but that combination is effectively unsupported because _backfill_merged_labels() returns zero when none of "running","done","auto_merge_requested" are present; update the validation in the enabled_pr_labels parsing (where PR_LABEL_KEY_TO_NAME and normalized_enabled_labels / seen_enabled_labels are handled) to detect the case where "merged" is present but none of the required merged sub-labels ("running","done","auto_merge_requested") are also included, and treat it as invalid: print the existing stderr error and exit (same style as other checks) or require the user to include at least one of those merged sub-label keys when "merged" is specified. Ensure the error message names the allowed merged sub-keys so users can fix the config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/auto_fixer.py`:
- Around line 1908-1951: The function _trigger_pr_auto_merge currently returns a
single bool combining "merge_state_reached" (auto-merge requested or PR already
merged) and "modified" (we changed PR labels), which causes caller miscounts;
change _trigger_pr_auto_merge to return a tuple (merge_state_reached: bool,
modified: bool), where merge_state_reached is True when the GH merge command
succeeded or the PR is already merged, and modified is True only when
_edit_pr_label actually changed labels (i.e., when you call _edit_pr_label and
it returns True); update all callers (places that call _trigger_pr_auto_merge,
including the duplicate site referenced) to unpack both values and use
merge_state_reached for merged-state logic and modified for label-change
counting/side effects.
- Around line 1557-1560: The _set_pr_merged_label() path is counting skips for
disabled labels as changed, causing _edit_pr_label() short-circuiting to report
changed=True even when no remove/add occurred; update the logic around enabled =
_resolve_enabled_pr_label_keys(enabled_pr_label_keys) and the
PR_LABEL_NAME_TO_KEY lookup so that when label_key is not in enabled you do not
set or propagate changed=True (i.e., return False or skip without marking
change), and mirror the same fix in the duplicate block (lines covering the
1711-1756 logic) so disabled labels are skipped without incrementing
changed/modified_prs.
- Around line 340-366: The current parsing for config["enabled_pr_labels"]
accepts "merged" alone but that combination is effectively unsupported because
_backfill_merged_labels() returns zero when none of
"running","done","auto_merge_requested" are present; update the validation in
the enabled_pr_labels parsing (where PR_LABEL_KEY_TO_NAME and
normalized_enabled_labels / seen_enabled_labels are handled) to detect the case
where "merged" is present but none of the required merged sub-labels
("running","done","auto_merge_requested") are also included, and treat it as
invalid: print the existing stderr error and exit (same style as other checks)
or require the user to include at least one of those merged sub-label keys when
"merged" is specified. Ensure the error message names the allowed merged
sub-keys so users can fix the config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03866110-b718-4ea9-8b9a-6d571bbfb0c5
📒 Files selected for processing (2)
src/auto_fixer.pytests/test_auto_fixer.py
`_update_done_label_if_completed` 呼び出し箇所 3 点で発生したコンフリクトを解消する。 - HEAD (PR #68) の `enabled_pr_label_keys` パラメータ追加と条件付き `modified_prs.add()` を維持 - origin/main の `_cacheable` 変数と 4 値目のリターン値を取り込む
…ied) タプルに変更 merge コマンドが成功した状態判定とラベル変更有無を分離することで、 auto_merge_requested ラベル操作の失敗時も _mark_pr_merged_label_if_needed が 呼ばれるよう修正。呼び出し元の merge_triggered もラベル実変更のみカウントする。
enabled に含まれないラベルをスキップする際に True を返すと、呼び出し元 (_set_pr_merged_label など) が changed=True と誤カウントしていた。 False を返すことで、実際の add/remove が発生した場合のみ changed が立つよう修正。
"merged" のみを指定した場合、_backfill_merged_labels が running/done/ auto_merge_requested のいずれも無いため 0 を返し、実質サポートされない。 設定読み込み時に不正構成として検出し、エラーメッセージと exit(1) で終了する。
_trigger_pr_auto_merge が (merge_state_reached, modified) タプルを返すよう変更されたが、 テスト側が bool を期待したままだったため CI が失敗していた。
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/auto_fixer.py (1)
1738-1762:⚠️ Potential issue | 🟠 Major無効ラベルのスキップが
_set_pr_merged_label()の成功判定を壊しています。この分岐は 3 つの
_edit_pr_label()を無条件で呼んでいますが、無効ラベルは_edit_pr_label()側でTrue扱いです。enabled_pr_label_keys={"auto_merge_requested"}のような構成だと、唯一の実編集であるrefix:auto-merge-requestedの削除が失敗しても、running/mergedのスキップだけでchanged=Trueになり、backfilled_countやmodified_prsを誤加算します。各呼び出し自体を enable 判定でガードしたほうが安全です。💡 修正案
- if _edit_pr_label( + if "running" in enabled and _edit_pr_label( repo, pr_number, add=False, label=REFIX_RUNNING_LABEL, enabled_pr_label_keys=enabled, ): changed = True - if _edit_pr_label( + if "auto_merge_requested" in enabled and _edit_pr_label( repo, pr_number, add=False, label=REFIX_AUTO_MERGE_REQUESTED_LABEL, enabled_pr_label_keys=enabled, ): changed = True - if _edit_pr_label( + if "merged" in enabled and _edit_pr_label( repo, pr_number, add=True, label=REFIX_MERGED_LABEL, enabled_pr_label_keys=enabled, ): changed = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1738 - 1762, The current sequence unconditionally calls three _edit_pr_label(...) invocations which return True even when a label is disabled, causing changed=True to be set incorrectly; update the code inside _set_pr_merged_label (or the surrounding function shown) to first check enabled_pr_label_keys (the enabled dict passed as enabled) before calling each _edit_pr_label for REFIX_RUNNING_LABEL, REFIX_AUTO_MERGE_REQUESTED_LABEL, and REFIX_MERGED_LABEL so that you only invoke _edit_pr_label when that specific label key is enabled; keep _ensure_refix_labels(...) as-is but wrap each _edit_pr_label call with an if enabled[...] guard (using the appropriate key names like "running", "auto_merge_requested", "merged") so changed is only set when a real edit occurs.
🤖 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 2465-2473: The bug is that an outdated pr_data (snapshot from
before review-fix started) is being passed into _set_pr_done_label after
_set_pr_running_label has already added the "refix:running" label, so the
removal logic misses "refix:running" and both running and done remain; to fix,
ensure you use the updated PR state when calling _set_pr_done_label (e.g.,
refresh/retrieve current pr_data after calling _set_pr_running_label or pass
None/omit pr_data so _set_pr_done_label fetches fresh data) and apply this same
change to the other occurrence at lines ~3320-3325; reference functions:
_set_pr_running_label and _set_pr_done_label.
---
Duplicate comments:
In `@src/auto_fixer.py`:
- Around line 1738-1762: The current sequence unconditionally calls three
_edit_pr_label(...) invocations which return True even when a label is disabled,
causing changed=True to be set incorrectly; update the code inside
_set_pr_merged_label (or the surrounding function shown) to first check
enabled_pr_label_keys (the enabled dict passed as enabled) before calling each
_edit_pr_label for REFIX_RUNNING_LABEL, REFIX_AUTO_MERGE_REQUESTED_LABEL, and
REFIX_MERGED_LABEL so that you only invoke _edit_pr_label when that specific
label key is enabled; keep _ensure_refix_labels(...) as-is but wrap each
_edit_pr_label call with an if enabled[...] guard (using the appropriate key
names like "running", "auto_merge_requested", "merged") so changed is only set
when a real edit occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bcd2ef42-92f0-4bae-874b-8c6279bc63af
📒 Files selected for processing (2)
src/auto_fixer.pytests/test_auto_fixer.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/auto_fixer.py (1)
2481-2489:⚠️ Potential issue | 🟠 Major古い
pr_dataを渡しているのでrefix:runningが残ります。ここで使っている
pr_dataは review-fix 開始前のスナップショットです。先に_set_pr_running_label()でrefix:runningを付けたあとでも、_set_pr_done_label()側は running 不在と誤認するので、成功時にrefix:runningとrefix:doneが同時に残ります。review_fix_started済みの経路ではpr_data=Noneで最新状態を使う必要があります。💡 修正案
if is_completed: print( f"PR #{pr_number} meets completion conditions; switching label to {REFIX_DONE_LABEL}." ) + current_pr_data = None if review_fix_started else pr_data if enabled_pr_label_keys is None: - done_changed = _set_pr_done_label(repo, pr_number, pr_data=pr_data) + done_changed = _set_pr_done_label( + repo, pr_number, pr_data=current_pr_data + ) else: done_changed = _set_pr_done_label( repo, pr_number, - pr_data=pr_data, + pr_data=current_pr_data, enabled_pr_label_keys=enabled_pr_label_keys, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 2481 - 2489, The code passes the stale pr_data into _set_pr_done_label causing refix:running to be considered absent; update the call sites around _set_pr_done_label so that when the PR has already been marked running (i.e. the review-fix started path / when review_fix_started is True), you pass pr_data=None to force fetching the latest PR state instead of pr_data; ensure both branches that call _set_pr_done_label (the enabled_pr_label_keys is None and non-None branches) use pr_data=None when review_fix_started is set, otherwise keep passing the existing pr_data.
🤖 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 1852-1864: The code allows backfill to run even when "merged" is
disabled because the initial gate checks only for any of
{"running","auto_merge_requested","merged"}; change the logic in the block that
computes enabled = _resolve_enabled_pr_label_keys(enabled_pr_label_keys) so that
if "merged" not in enabled you immediately return 0 (i.e., add an explicit check
like if "merged" not in enabled: return 0) before proceeding to build
search_parts (affecting how _resolve_enabled_pr_label_keys,
enabled_pr_label_keys, enabled and the subsequent use of
REFIX_DONE_LABEL/REFIX_AUTO_MERGE_REQUESTED_LABEL/REFIX_RUNNING_LABEL/REFIX_MERGED_LABEL
behave).
---
Duplicate comments:
In `@src/auto_fixer.py`:
- Around line 2481-2489: The code passes the stale pr_data into
_set_pr_done_label causing refix:running to be considered absent; update the
call sites around _set_pr_done_label so that when the PR has already been marked
running (i.e. the review-fix started path / when review_fix_started is True),
you pass pr_data=None to force fetching the latest PR state instead of pr_data;
ensure both branches that call _set_pr_done_label (the enabled_pr_label_keys is
None and non-None branches) use pr_data=None when review_fix_started is set,
otherwise keep passing the existing pr_data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20f60162-a1ea-4484-a1af-d1c1d67250e8
📒 Files selected for processing (2)
src/auto_fixer.pytests/test_auto_fixer.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/auto_fixer.py (1)
1852-1858: Lines 1855-1856 のチェックは到達不能です。Line 1853-1854 で
"merged" not in enabledの場合に早期リターンしているため、この時点でenabledには必ず"merged"が含まれています。そのため、Line 1855-1856 の{"running", "auto_merge_requested", "merged"} & enabledは常に真（少なくともmergedが含まれる）となり、このブロックは実行されません。♻️ 冗長なチェックの削除提案
def _backfill_merged_labels( repo: str, *, limit: int = 100, enabled_pr_label_keys: set[str] | None = None, ) -> int: """Backfill refix:merged label for merged PRs already marked refix:done.""" enabled = _resolve_enabled_pr_label_keys(enabled_pr_label_keys) if "merged" not in enabled: return 0 - if not ({"running", "auto_merge_requested", "merged"} & enabled): - return 0 if "done" not in enabled and "auto_merge_requested" not in enabled and "running" not in enabled: return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto_fixer.py` around lines 1852 - 1858, The second check is unreachable because we already return when "merged" is not in enabled; remove the redundant block that tests if not ({"running", "auto_merge_requested", "merged"} & enabled) and its return, leaving only the initial "merged" check and the subsequent conditional that verifies "done"/"auto_merge_requested"/"running". Locate the enabled = _resolve_enabled_pr_label_keys(enabled_pr_label_keys) assignment and delete the if-block that intersects {"running", "auto_merge_requested", "merged"} with enabled.
🤖 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 1852-1858: The second check is unreachable because we already
return when "merged" is not in enabled; remove the redundant block that tests if
not ({"running", "auto_merge_requested", "merged"} & enabled) and its return,
leaving only the initial "merged" check and the subsequent conditional that
verifies "done"/"auto_merge_requested"/"running". Locate the enabled =
_resolve_enabled_pr_label_keys(enabled_pr_label_keys) assignment and delete the
if-block that intersects {"running", "auto_merge_requested", "merged"} with
enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6c5f083-60c3-49f4-aaa5-240feb974723
📒 Files selected for processing (1)
src/auto_fixer.py
HappyOnigiri
added
the
refix:auto-merge-requested
HappyOnigiri
added
refix:merged
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
2 participants
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
PRラベルの付与を
enabled_pr_labels設定で制御できるようにしました。デフォルトは現状通りすべてのラベルを付与し、一部または全てのラベルを無効化できます。変更内容 / Changes
.refix.yamlにenabled_pr_labels設定を追加。running,done,merged,auto_merge_requestedの中から有効化するラベルを指定可能。[]を指定するとPRラベル操作を全て無効化。auto_merge時のauto-merge-requested付与、mergedbackfill 処理がenabled_pr_labelsの設定に従うよう変更。テスト / Testing
make ci(pytestを含む) にて、enabled_pr_labelsの設定パース（正常系、異常系、デフォルト、空リスト）および、有効化されたラベルのみが操作されることを検証済み。関連 issues / Related issues
なし
Summary by CodeRabbit
新機能
ドキュメント
テスト
変更