Uh oh!
There was an error while loading. Please reload this page.
Fix: cross-review gemini stall 既定 480s + fix 戻り値 fallback (PLAN21, v4.7.4) - #6
Conversation
….7.4) `/ndf:cross-review` 実走時の運用不整合 2 件を恒久対応する PATCH リリース (i18 報告 / PLAN21)。 - monitor.py: per-agent stall timeout 既定の導入 - 既定を agent 別ビルトイン化: codex=180s (不変) / gemini=480s - err.log にほぼ進捗を出さない gemini を 1 度目に毎回 STALLED kill する誤検知を解消 - 解決順: CLI --stall-timeout > env MONITOR_STALL_<AGENT> > env MONITOR_STALL > builtin - --stall-timeout argparse default を int → None に変更 - state.py merge-fix: fix 戻り値ファイル探索 fallback + key alias - 探索順を (1) --file > (2) $TMP_DIR/ > (3) /tmp/ の 3 段に - 全候補不在時の die メッセージに探索 path 一覧を含める - commit_sha / fixed の別名 key も fix_commit / fixed_count として受理 - SKILL.md / docs のパス記述統一 - SKILL.md の /tmp/fix-pr<#>-result.json ハードコード 3 箇所を $TMP_DIR/ に - 「すべて /tmp/ に置き」表現も $TMP_DIR/ (= _tmp_dir() 解決先) に - docs/01: monitor 説明表に per-agent stall 既定の解決順を追記 - docs/02: $TMP_DIR の解決順を 1 行明示 - pytest 追加 (全 24 ケース pass) - test_monitor_stall_default.py (6 ケース) - test_state_merge_fix.py (5 ケース) - plugin version 4.7.3 → 4.7.4 + CHANGELOG entry 関連: issues/i18-issue-gemini.md, issues/PLAN21_cross-review-gemini-stall-and-fix-merge.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 1 | gemini | APPROVE
PLAN21に基づき、Geminiのstall判定緩和とfix戻り値マージの堅牢化が正しく実装されています。追加されたテストにより、fallbackロジックの正常動作も確認されています。
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 1 | codex | REQUEST_CHANGES
/tmp fallback はPR番号だけの非スコープファイルを無条件に採用するため、同一PR番号の再実行や別リポジトリの古い戻り値を state にマージし得ます。fallback候補は現在round開始後に生成されたことを検証し、古い候補を無視してください。
Uh oh!
There was an error while loading. Please reload this page.
codex review 指摘 (PR #6): `/tmp/fix-pr<PR>-result.json` は PR 番号のみで命名された共有 namespace のため、別 round / 別リポジトリの同番号 PR の戻り値を 無条件に拾ってしまう major バグを修正。 - 冒頭で state を 1 回だけ load し、round 開始時刻 (UNIX time) を取得 - fallback 候補 (TMP_DIR / /tmp) の採用前に以下を検証: 1. ファイル mtime が round.started_at 以降 2. JSON 内 `pr` フィールドがあれば対象 PR と一致 - 古い候補は警告を stderr に出して skip - 明示 `--file` 指定はユーザー意図優先で stale 検証バイパス - 重複していた後段の `_load(pr)` を削除 tests/test_state_merge_fix.py: - stale mtime (TMP_DIR / /tmp 両方) / pr 不一致 / fresh 採用 / --file バイパスの 5 ケースを追加 (29 tests 全て pass) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 2 | gemini | APPROVE
fallback 指定の fix 結果ファイルに対する stale 検証（mtime および PR 番号のチェック）が導入され、共有ディレクトリにおける競合や古い実行結果の誤マージのリスクが適切に低減されています。テストコードも網羅的で、正常系・異常系ともに期待通り動作することが確認できました。
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 2 | codex | REQUEST_CHANGES
fallback候補の検証で、古い候補・PR不一致と、正規戻り値ファイルの読み取り失敗を分けて扱ってください。
Uh oh!
There was an error while loading. Please reload this page.
cross-review round 2 のクロスレビュー指摘 4 件に対応 (plugins/ndf/skills/cross-review/scripts/state.py)。 - (codex major, line 151) 正規パス ($TMP_DIR/fix-pr<PR>-result.json) の JSON parse 失敗を後続 fallback に流すと別 PR の戻り値を誤マージする事故が 起こるため、is_canonical=True の候補の OSError / JSONDecodeError は 即時 die(code=3) で中断する。stale mtime / pr 不一致は従来通り次の候補へ。 - (gemini minor, line 150) _is_fresh_fix_result の read_text() に encoding="utf-8" を明示。 - (gemini minor, line 155) file_pr の int() を try/except で囲み、 ValueError / TypeError なら warn + invalid 扱い (fresh ではない)。 - (gemini minor, line 508) _is_fresh_fix_result を (is_fresh, parsed_payload) のタプル返却に変更し、呼び出し側 cmd_merge_fix で再パースを省略。 明示 --file パスのみ別途 read_text(encoding="utf-8") で読み込む。 tests/test_state_merge_fix.py に下記 4 ケース追加: - 正規パス JSON parse 失敗 → 即時 die(code=3)、/tmp fallback に流れないこと - fallback の pr フィールドが int 化不能 → skip - _is_fresh_fix_result が (True, dict) を返すこと - stale 時に (False, None) を返すこと 全 33 テスト pass。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 3 | gemini | REQUEST_CHANGES
正規パスの読み取り失敗時の die 処理や再パース回避による性能改善は適切です。一方で、JSON パース後のデータが dict であることを検証していないため、リスト形式などの不正な JSON が与えられた場合に AttributeError でプロセスがクラッシュする脆弱性が残っています。
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 3 | codex | COMMENT
MONITOR_STALL の共通 env 解決と、その回帰テストの期待値を実際の上書き値に合わせてください。
Uh oh!
There was an error while loading. Please reload this page.
…L 動的解決 (PLAN21 round 4) PR #6 round 3 のクロスレビューで指摘された 3 件を修正: 1. state.py:_is_fresh_fix_result (major, gemini 指摘) json.loads は dict 以外 (list 等) も返しうるため、payload.get(...) 呼び出し前に isinstance(payload, dict) を検証し、dict でなければ warn + (False, None) を返す。 2. state.py:cmd_merge_fix (major, gemini 指摘) --file 明示時の JSON も同様に isinstance(fix, dict) を検証。dict でない場合は 後続の fix.get(...) クラッシュを防ぐため即時 die(code=3)。 3. monitor.py:_agent_stall_default (minor, codex 指摘) MONITOR_STALL が設定されているとき、import 時固定の DEFAULT_STALL ではなく int(os.environ["MONITOR_STALL"]) を呼び出し時に再評価する。 これにより共通 env が実際に反映されるようになり、テストでも検証可能になる。 テスト: - test_is_fresh_fix_result_non_dict_json_is_skipped (新規) - test_is_fresh_fix_result_non_dict_json_canonical_also_skipped (新規) - test_explicit_file_non_dict_json_dies (新規) - test_shared_env_applies_to_both: 期待値を 240 に修正 (共通 env 反映の検証) - test_per_agent_env_takes_precedence_over_shared: codex 側の期待値を 240 に修正 全 36 tests pass。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 4 | gemini | REQUEST_CHANGES
PLAN21 round 3 の指摘の反映を確認しました。fix 結果マージへの防御と MONITOR_STALL の動的解決により堅牢性が向上しています。一方で、review result や round payload のパース箇所に同様の non-dict JSON によるクラッシュリスクが残っています。これらへの水平展開を推奨します。
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 4 | codex | REQUEST_CHANGES
fix 戻り値の明示/正規パスが不正な場合に共有 fallback へ継続する経路が残っているため、誤マージ防止のガードを追加してください。
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
… round 5) PR #6 round 4 クロスレビュー指摘 4 件への対応。 1. state.py `_is_fresh_fix_result(is_canonical=True)` の非 dict は parse 失敗と 同様に即時 die(code=3) する (codex)。skip すると後続 /tmp/ fallback で別実行の 戻り値を誤マージする経路が残るため。 2. state.py `cmd_merge_fix` の `--file` 明示時は fallback 探索に進まず即時失敗 させる (codex)。存在しない / 空 / JSON 不正 / 非 dict のすべてのケースで die(code=3) する。 3. state.py `cmd_read_result` / `cmd_check_oscillation` でも JSON 読み込み直後に `isinstance(..., dict)` 検証を追加し、不正な JSON によるクラッシュを防止 (gemini)。`cmd_check_oscillation` は comments のエントリも dict 検証する。 4. monitor.py `_agent_stall_default()` で env が非数値だった場合、ValueError / TypeError を warn 付きで catch して builtin 既定にフォールバック (gemini)。 監視プロセスを env 設定ミスでクラッシュさせない。 テスト: - test_state_merge_fix.py: 正規パス非 dict / --file 不在 / 空 / 不正 JSON の die(code=3) 確認 + 既存 round 4 テストを round 5 挙動に追従 - test_state_read_result.py: result.json 非 dict / parse 不能の die(code=3) - test_state_check_oscillation.py (新規): payload 非 dict / comments エントリ 非 dict の die(code=3) + 正常 dict が誤検知されない regression guard - test_monitor_stall_default.py: MONITOR_STALL=abc 等で builtin フォールバック 全 48 テスト pass。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 5 | gemini | APPROVE
環境変数のパース失敗時や JSON ファイルが不正な型（dict 以外）である場合の防御策が網羅的に追加されており、システムの堅牢性が大幅に向上しています。
特に cmd_merge_fix において --file 指定時のバリデーションを厳格化した点、および正規パスでの不正データ検出時にフォールバックせず即時停止（die）するようにした点は、意図しないデータのマージを防ぐ上で非常に重要な改善です。テストコードも追加された各エラーパスを的確に検証しており、品質面で問題ありません。
Uh oh!
There was an error while loading. Please reload this page.
… / 最終) codex round 5 指摘: `DEFAULT_TIMEOUT` / `DEFAULT_STALL` / `DEFAULT_POLL` を `int(os.environ.get(name, ...))` で import 時に評価すると、`MONITOR_STALL=abc` のような非数値 env 設定で `_agent_stall_default()` に到達する前に `int()` の `ValueError` で monitor.py の import そのものが落ち、 監視プロセスが起動できなくなる。 修正: - `DEFAULT_TIMEOUT=420` / `DEFAULT_STALL=180` / `DEFAULT_POLL=15` を import 時に固定数値で保持 - env の解釈は `_agent_stall_default()` / 新規 `_safe_int_env()` 内で try/except 付きで行う (非数値時は warn を出して fallback) - argparse 既定値も `_safe_int_env()` 経由で env を呼び出し時に読む テスト追加 (`tests/test_monitor_import_safety.py`): - `MONITOR_STALL=abc` / `MONITOR_TIMEOUT=xxx` / `MONITOR_POLL=foo` を 個別 / 同時に設定しても monitor.py の fresh import が成功すること - `_safe_int_env()` ヘルパの正常系 / fallback / warn 出力 cross-review test suite: 55 passed (新規 7 件含む、既存テスト全 pass)。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 6 | gemini | APPROVE
環境変数の非数値設定によるクラッシュを修正し、網羅的なテストが追加されています。修正内容は適切で、堅牢性が向上しています。
Uh oh!
There was an error while loading. Please reload this page.
takemi-ohama
left a comment
There was a problem hiding this comment.
🤖 cross-review | round 6 | codex | APPROVE
修正提案なし。pytest /work/worktrees/pr6/plugins/ndf/skills/cross-review/tests -q は 55 passed。
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- conftest.py.template: browser_type_launch_args を依存に追加し local モードで --headed/slow_mo/channel 等が反映されるよう修正 (#1, #6) - conftest.py.template: 変数名 b → browser に改名 (#2) - conftest.py.template: CDP 接続時に非 Chromium ブラウザの fail-fast チェック追加 (#5) - config.py: BrowserConfig.cdp_endpoint の既定値を ws:// → http:// に変更 (#4) - CHANGELOG.md: v4.10.0 エントリを追加 (#3) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
/ndf:cross-review実走時の運用不整合 2 件を恒久対応する PATCH リリース (i18 報告 / PLAN21)。--stall-timeout> envMONITOR_STALL_<AGENT>> envMONITOR_STALL> builtin。--file> (2)\$TMP_DIR/> (3)/tmp/の 3 段 fallback に。全候補不在時の die メッセージに探索 path 一覧を含める。commit_sha/fixedの別名 key もfix_commit/fixed_countとして受理 (silent なfixed=0記録を救済)。/tmp/fix-pr<#>-result.jsonハードコード 3 箇所を\$TMP_DIR/に。docs/01 monitor 説明と docs/02 $TMP_DIR 解決順を追記。差分は 11 files / +928 -17 (うち issues/PLAN21*.md と issues/i18-issue-gemini.md の plan ファイル分が大半。コード変更自体は ~130 行)。
Plan
issues/PLAN21_cross-review-gemini-stall-and-fix-merge.md を参照。
関連:
互換性
Test plan