From 24bbb5263e01b119a19f7c5f0353fb39f75c8079 Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Wed, 20 May 2026 03:50:19 +0000 Subject: [PATCH 1/2] =?UTF-8?q?Fix:=20cross-review=20monitor.py=20?= =?UTF-8?q?=E3=81=AE=20EARLY=5FERROR=20=E8=AA=A4=E6=A4=9C=E7=9F=A5?= =?UTF-8?q?=E3=82=92=E4=BF=AE=E6=AD=A3=20(ndf=20v4.7.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issues/REPORT01.md の不具合報告 ("EARLY_ERROR 誤検知で codex/gemini が無条件に kill される") に対応。中規模以上の PR で cross-review がほぼ毎ラウンド失敗扱いに なる High バグの修正。 ## 修正内容 ### monitor.py: EARLY_ERROR を FATAL/WARN に分離 (報告書 提案1) - `EARLY_ERROR_PATTERNS` を `EARLY_ERROR_FATAL` (auth/quota/sandbox/HTTP 401-403-429 /gemini YOLO 降格) と `EARLY_ERROR_WARN` (生 `Error:` / `Traceback` の曖昧パターン) に分離 - FATAL 検知時のみ kill。WARN は警告ログだけ出して通常判定 (sentinel / result.json / timeout) を継続する - `_scan_early_errors` → `_scan_patterns` / `_scan_early_fatal` / `_scan_early_warn` に分割 ### monitor.py: BENIGN フィルタ強化 (報告書 提案2) - gemini-cli の `Error in: mcpServers.` config validation 警告を benign 扱い に追加。`Error in: ` 単独パターンも除外 ### monitor.py: --no-early-error フラグ追加 (報告書 提案3) - `--no-early-error` / `MONITOR_NO_EARLY_ERROR=1` で EARLY_ERROR 検知自体を無効化 する escape hatch を追加。誤検知が継続する場合の最終回避策 ### launch-gemini.sh: settings.json sanitize ロジック追加 (報告書 提案4) - worktree の `.gemini/settings.json` に gemini-cli 最新版で Unrecognized 扱いに なるキー (`disabled` 等) があると毎回 `Error in: mcpServers.` 警告が出る 問題を回避 - launcher が起動時に jq で `disabled` キーを再帰削除した sanitize 版に差し替え、 gemini が settings を読み終えた頃 (2 秒後) に元のファイルを復元する ### SKILL.md: アンチパターン節を更新 (報告書 提案4) - EARLY_ERROR の曖昧パターンで kill するのを禁止事項として明記 - `monitor.py` が誤って kill する場合の切り分け手順を追加 (err.log 確認 → launcher の sanitize 確認 → --no-early-error 切替 → 新パターンを FATAL に追記) ### plugin.json: バージョン更新 - 4.7.1 → 4.7.2 (パッチ) Co-Authored-By: Claude Opus 4.7 (1M context) --- issues/REPORT01.md | 341 ++++++++++++++++++ plugins/ndf/.claude-plugin/plugin.json | 2 +- plugins/ndf/skills/cross-review/SKILL.md | 17 + .../cross-review/scripts/launch-gemini.sh | 35 +- .../skills/cross-review/scripts/monitor.py | 114 ++++-- 5 files changed, 479 insertions(+), 30 deletions(-) create mode 100644 issues/REPORT01.md diff --git a/issues/REPORT01.md b/issues/REPORT01.md new file mode 100644 index 0000000..fd7d7f2 --- /dev/null +++ b/issues/REPORT01.md @@ -0,0 +1,341 @@ +# [ndf:cross-review] 不具合報告 — monitor.py の早期エラー誤検知で codex / gemini が無条件に kill される + +- 報告日: 2026-05-19 +- 報告者: takemi-ohama +- 対象 plugin: `ndf` (cache パス: `/home/ubuntu/.claude/plugins/cache/ai-plugins/ndf/4.7.1/`) +- 対象 skill: `ndf:cross-review` +- 対象スクリプト: `skills/cross-review/scripts/monitor.py`、`skills/cross-review/scripts/launch-gemini.sh` +- 確認バージョン: 4.7.1 (`/home/ubuntu/.claude/plugins/cache/ai-plugins/ndf/4.7.1`) +- 影響度: **High** — 中規模以上の PR で `/ndf:cross-review` がほぼ毎ラウンド失敗扱いになり、ループが回らない + +## 概要 + +`/ndf:cross-review` でレビューラウンドを回したところ、`monitor.py` の **EARLY_ERROR 検知** が +正常実行中の codex / gemini を「致命的エラー」と誤判定して **SIGTERM → SIGKILL** で +強制終了させる事象が複数のラウンドで再現した。 + +`monitor.py` は EARLY_ERROR を検知すると対象プロセスを kill するため、AI 側はレビューを +完了できず `result.json` も書き出せない。結果としてラウンド全体が `EARLY_ERROR` で +中断され、cross-review ループが進まない。 + +実運用では監視を介さずに `result.json` の生成有無で進捗判定するカスタムポーリングに +切替えて回避したが、本来は plugin 側で修正されるべき。 + +## 影響範囲 + +| 観測 | 影響 | +|---|---| +| PR #276 round 1 | gemini を 30s 時点で誤 kill (`Error in: mcpServers.github`) | +| PR #276 round 1 (codex 再現) | codex を 105s 時点で誤 kill (`Traceback (most recent call last):` ※diff 本文の引用) | +| PR #276 round 2 | codex / gemini 両方が **同様の誤検知で kill される** ことを再確認 | +| 全 round 共通 | 1 度誤検知が起きると round 全体が EARLY_ERROR 終了し、`/ndf:cross-review` 本体は `final=error` 一歩手前まで進む | + +PR #275 (小規模 PR, diff 量が少ない) では再現しなかったため、**diff 量が増えて +err.log にコード片 (Traceback を含む test コード等) や MCP 警告が混在しやすい +中〜大規模 PR** で実害が出る。 + +## 再現条件 + +1. `/ndf:cross-review ` を **自分の PR** に対して実行（`event_downgrade=true`） +2. PR の diff にテストコード等が含まれ、codex / gemini の err.log にコード片や + Traceback 引用が出力される +3. または worktree 内 `.gemini/settings.json` が gemini-cli 最新仕様と非互換で + 起動直後に `Error in: mcpServers.` 警告が出る + +## 観測されたログ (代表例) + +### 例 1: gemini を `Error in: mcpServers.github` で kill + +``` +=== /home/ubuntu/.gemini/tmp/pr276/gemini-review-pr276-err.log (Round 1 初回起動) === +Invalid configuration in /work/worktrees/pr276/.gemini/settings.json: + +Error in: mcpServers.github + Unrecognized key(s) in object: 'disabled' + +Error in: mcpServers.awslabs.core-mcp-server + Unrecognized key(s) in object: 'disabled' +... +Please fix the configuration. +See: https://geminicli.com/docs/reference/configuration/ +YOLO mode is enabled. All tool calls will be automatically approved. +YOLO mode is enabled. All tool calls will be automatically approved. +Ripgrep is not available. Falling back to GrepTool. +MCP issues detected. Run /mcp list for status. +``` + +monitor.py の判定: + +``` +[gemini] 💥 gemini EARLY_ERROR (15s) — early error in err.log: Error in: mcpServers.github +{"agent": "gemini", "status": "EARLY_ERROR", "exit_code": 4, ...} +``` + +gemini-cli はこのメッセージを出した後も **YOLO mode is enabled** と表示して +実行を継続する仕様（無効キーは無視するだけで起動失敗ではない）だが、 +monitor.py は致命扱いで kill する。 + +### 例 2: codex を `Traceback (most recent call last):` で kill + +PR の diff にテストコード (例: `test_lease_judgment_skipped_when_lease_tier_disabled` の +シミュレートエラー処理) が含まれていると、codex はその diff を err.log に echo する。 + +``` ++ def test_lease_judgment_skipped_when_lease_tier_disabled(self, client, monkeypatch): ++ """lease_tier 失敗時に lease_judgment が自動 skip + error=dependency 'lease_tier' not available""" +... ++ def _failing_execute(self, ctx): ++ raise RuntimeError("simulated lease_tier failure") +... +Traceback (most recent call last): +``` + +monitor.py の判定: + +``` +[codex] 💥 codex EARLY_ERROR (105s) — early error in err.log: Traceback (most recent call last): +``` + +codex は diff 表示の一部としてレビュー中に `Traceback` 文字列を出力しているだけで、 +実際には正常稼働中。150〜260 秒後には正常に `tokens used` sentinel に達し、 +`result.json` を書く挙動 (実環境で確認済) なのに、105 秒時点で kill されてしまう。 + +## 根本原因 + +### A. EARLY_ERROR パターンが広すぎる + +`monitor.py:67` + +```python +re.compile(r"^(?:Error|FATAL|fatal|panic|PANIC|Traceback)[: ]", re.MULTILINE), +``` + +このパターンは: + +- `Error in: mcpServers.github` — gemini 起動時の **無害な設定警告** にマッチ +- `Traceback (most recent call last):` — **codex がレビュー対象の test コード片 + を echo した行** にマッチ (test の例外処理コードに合法的に含まれる) + +### B. BENIGN フィルタが弱い + +`monitor.py:81` + +```python +EARLY_ERROR_BENIGN = [ + re.compile(r"^[ +-].*[\|`]", re.MULTILINE), # diff context with `|` or backtick + re.compile(r"^\s*[-*>]\s", re.MULTILINE), # markdown list/quote + re.compile(r"^warning: ", re.IGNORECASE | re.MULTILINE), +] +``` + +`Traceback (most recent call last):` のような行は **行頭 `T`** で始まり、 +benign フィルタの「diff context (` +-`)」「markdown list (`-*>`)」「warning:」の +いずれにもマッチしないため、誤検知を回避できない。 + +また `Error in: mcpServers.github` も行頭 `E` で benign フィルタにかからない。 + +### C. EARLY_ERROR 判定が kill とセットになっている + +`monitor.py:347-355` + +```python +err = _scan_early_errors(paths.err_log) +if err: + if alive: + _kill_pid(pid) + status.status = "EARLY_ERROR" + status.exit_code = 4 +``` + +EARLY_ERROR 検知時に **必ず対象プロセスを kill** する設計のため、 +誤検知 1 回で AI のレビュー実行が完全に断たれ、`result.json` も生成できない。 + +「致命と確証が取れたパターンのみ kill する」「曖昧なパターンは警告に +留めて待機を続ける」といった段階制御が無い。 + +### D. 早期エラー検知を無効化するフラグが無い + +`monitor.py` には: + +- `--timeout` / `MONITOR_TIMEOUT` +- `--stall-timeout` / `MONITOR_STALL` +- `--no-require-result` + +のオプションがあるが、**早期エラー検知を無効化する手段が無い** ため、 +誤検知が出続けると待機側でも対処できない。 + +### E. (副次的) `.gemini/settings.json` が gemini-cli 最新仕様で非互換 + +このリポジトリでは `.gemini/settings.json` の各 mcpServer エントリに +`disabled: false` を書き込んでいたが、gemini-cli 最新版はこのキーを +Unrecognized 扱いにする。 + +`launch-gemini.sh` 側で「`cd $WORKTREE` してから gemini を起動」する設計上、 +worktree の `.gemini/settings.json` を必ず読みに行くため、リポジトリ側に +非互換ファイルがあると毎回 `Error in: mcpServers.*` が出る。 + +これ自体はリポジトリ側で直すべき問題だが、上記 A〜D と組み合わさることで +**cross-review が起動しないほど影響が大きくなる**。 + +## 影響の連鎖 + +``` +.gemini/settings.json の disabled key (リポ側の非互換) + ↓ +gemini 起動時に err.log に "Error in: mcpServers.github" を出力 (実害なし、続行可能) + ↓ +monitor.py の EARLY_ERROR_PATTERNS が "Error in:" にマッチ (誤検知) + ↓ +_kill_pid() で gemini を強制終了 (result.json 未生成) + ↓ +ラウンド全体が EARLY_ERROR (exit 4) で終了 + ↓ +/ndf:cross-review が進まない / 全 round が同じ理由で失敗 +``` + +## 再現手順 + +1. 中規模以上の PR (diff にテストコードを含む) を対象に `/ndf:cross-review ` を実行 +2. もしくは worktree 内 `.gemini/settings.json` に gemini-cli 最新版で + 非互換のキー (例: `disabled: false`) を含めて実行 +3. `monitor.py` が `^Error[: ]` / `^Traceback[: ]` を err.log で見つけて kill する + +## 暫定回避策 (今回適用したもの) + +1. `.gemini/settings.json` の `disabled` キーをラウンド開始前にローカルで削除し、 + ラウンド終了後 `git restore` で戻す +2. `monitor.py` を使わず、`result.json` ファイルの生成有無を直接ポーリングする + bash ループに切替え (EARLY_ERROR 検知をバイパス) + +```bash +# 暫定: result.json の生成を 600 秒まで待つだけのポーリング +for i in $(seq 1 30); do + sleep 20 + CODEX_RESULT=$([ -s "$TMP/codex-review-pr$PR-result.json" ] && echo 1 || echo 0) + GEMINI_RESULT=$([ -s "$TMP/gemini-review-pr$PR-result.json" ] && echo 1 || echo 0) + if [ "$CODEX_RESULT" = 1 ] && [ "$GEMINI_RESULT" = 1 ]; then + break + fi +done +``` + +## 修正提案 + +### 提案 1 (最優先): EARLY_ERROR 検知に「致命度」を導入し kill を抑制する + +「明確な致命パターン (auth 失敗 / quota / sandbox)」と「曖昧パターン (生の +`Error:` / `Traceback`)」を区別し、後者は **kill せず警告ログのみ** に留める。 +プロセスが自走中なら sentinel / result.json / hard timeout の通常経路で判定する。 + +```python +EARLY_ERROR_FATAL = [ + re.compile(r"^Authentication failed", re.MULTILINE), + re.compile(r"^.*\b(quota exceeded|rate limit exceeded)\b", re.IGNORECASE | re.MULTILINE), + re.compile(r"^.*\bAPI key (not found|missing|invalid)", re.IGNORECASE | re.MULTILINE), + re.compile(r"^.*\bsandbox error\b", re.IGNORECASE | re.MULTILINE), + re.compile(r"^HTTP/\d\S* (?:401|403|429) ", re.MULTILINE), + re.compile(r'^Approval mode overridden to "default"', re.MULTILINE), # gemini 固有 +] + +EARLY_ERROR_WARN = [ + # 行頭 Error: / Traceback: は警告のみ (kill しない) + re.compile(r"^(?:Error|FATAL|fatal|panic|PANIC|Traceback)[: ]", re.MULTILINE), +] +``` + +検知ロジック: + +```python +fatal_err = _scan_patterns(err_log, EARLY_ERROR_FATAL) +warn_err = _scan_patterns(err_log, EARLY_ERROR_WARN) +if fatal_err: + _kill_pid(pid) + return EARLY_ERROR +elif warn_err and not warned: + log.warning(f"[{agent}] early-error pattern detected (non-fatal): {warn_err}") + warned = True +# kill せず通常の sentinel / result.json / timeout 判定を続行 +``` + +### 提案 2: BENIGN フィルタを強化 + +- gemini の `Error in: mcpServers.\S+` 警告を明示的に除外 +- codex がレビュー本文として echo する diff の一部 (前後数行に `+`/`-` で + 始まる行が連続) を benign 判定 + +```python +EARLY_ERROR_BENIGN += [ + # gemini config validation warning + re.compile(r"^Error in: mcpServers\.", re.MULTILINE), + # 直前 3 行以内に diff hunk header (`@@ ...`) があれば diff body と判定 + # (実装イメージ — _scan_early_errors 内で前後行を見て判定) +] +``` + +### 提案 3: `--no-early-error` フラグを追加 + +既存の `--no-require-result` と同様、EARLY_ERROR 検知自体を無効化する +フラグを `monitor.py` に追加し、メイン側で誤検知を回避できるようにする。 + +```python +p.add_argument("--no-early-error", action="store_true", + help="EARLY_ERROR 検知を無効化 (hard timeout / stall / sentinel のみで判定)") +``` + +### 提案 4: `launch-gemini.sh` を改善 + +worktree の `.gemini/settings.json` を読まないよう、`GEMINI_CONFIG_DIR` +等の専用環境変数で gemini-cli の config を上書きする（gemini-cli が対応する +場合）。または launcher が worktree の `.gemini/settings.json` を一時的に +退避してから gemini を起動する仕組みを提供する。 + +```bash +# 案: launcher が worktree の .gemini/settings.json を mv で退避し、 +# 最低限のフォールバックを置く +if [ -f "$WORKTREE/.gemini/settings.json" ]; then + cp "$WORKTREE/.gemini/settings.json" "$TMP_DIR/.gemini-settings-backup.json" + echo '{"context_files":["AGENTS.md"]}' > "$WORKTREE/.gemini/settings.json" + trap 'mv "$TMP_DIR/.gemini-settings-backup.json" "$WORKTREE/.gemini/settings.json"' EXIT +fi +``` + +### 提案 5: 各ラウンド開始前の env / config 健全性チェック + +cross-review 開始時に gemini-cli を `gemini --help` 程度で 1 度起動して +err.log のフォーマットを確認し、`Error in: mcpServers.*` 等の既知警告が +出るなら自動的にパッチを当てるか、ユーザに通知する。 + +## 関連ファイル + +| ファイル | 行 | 内容 | +|---|---|---| +| `skills/cross-review/scripts/monitor.py` | 65-78 | EARLY_ERROR_PATTERNS 定義 | +| `skills/cross-review/scripts/monitor.py` | 81-88 | EARLY_ERROR_BENIGN 定義 | +| `skills/cross-review/scripts/monitor.py` | 208-236 | `_scan_early_errors` 実装 | +| `skills/cross-review/scripts/monitor.py` | 347-355 | EARLY_ERROR 検知 → `_kill_pid` の呼び出し | +| `skills/cross-review/scripts/launch-gemini.sh` | 99-107 | worktree CWD で gemini 起動 (settings.json を読みに行く) | +| `skills/cross-review/SKILL.md` | (アンチパターン節) | "`monitor.py` の多軸判定を使うこと" と書かれているが、その多軸判定そのものが今回 kill の原因 | + +## 補足: 実害が出たラウンド一覧 + +| PR | round | agent | 観測時刻 | 検知パターン | 実際の状態 | +|---|---|---|---|---|---| +| #276 | 1 | gemini | 30s | `Error in: mcpServers.github` | 起動 1 警告のみ、続行可能 | +| #276 | 1 | codex | 105s | `Traceback (most recent call last):` | レビュー実行中 (sentinel 未到達)、150s で正常完了 | +| #276 | 2 | gemini | 30s | `Error executing tool mcp_github_get_pull_request_files: ...` | 一部 MCP 失敗だが gh CLI でリカバリ可能、続行中 | +| #276 | 2 | codex | 90s | `Traceback (most recent call last):` | 同上 | + +すべて kill 後にカスタムポーリングで再起動 → `result.json` が **160〜260 秒** +程度で正常生成されたことを確認しているため、誤検知であることは確定的。 + +## 期待する対応 + +1. **(最優先)** `monitor.py` の EARLY_ERROR 検知を「警告のみ」と「kill する致命」に + 分離し、`Error:` / `Traceback` のような曖昧パターンは kill 対象から外す +2. `--no-early-error` フラグの追加 (escape hatch として) +3. (推奨) `launch-gemini.sh` が worktree の `.gemini/settings.json` を読みに + 行く副作用を抑制する仕組みを提供 +4. (推奨) SKILL.md の「アンチパターン」節に **「`monitor.py` が + false-positive で kill する場合の手順」** を追記 + +ご検討よろしくお願いします。 diff --git a/plugins/ndf/.claude-plugin/plugin.json b/plugins/ndf/.claude-plugin/plugin.json index afac962..6494357 100644 --- a/plugins/ndf/.claude-plugin/plugin.json +++ b/plugins/ndf/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "ndf", - "version": "4.7.1", + "version": "4.7.2", "description": "Integrated plugin with 8 specialized agents (model-tiered: opus/sonnet/haiku), 39 skills including official mcp-builder, on-demand loader for Anthropic official skills, generic workflow/principle skills, skill usage statistics, pytest-playwright based scenario E2E testing (v0.5.0: BREAKING — package renamed scenario_test → playwright_kit, fixtures/CLI ndf_*/--ndf-* → pwk_*/--pwk-*, all-in-one runtime layout enabling Skill-independent operation via init_project.sh + run.sh, accessibility/web vitals autouse, overlay (formerly HUD), report.md, Drive integration, body_check autouse enabled by default to detect server-rendered PHP/SSR errors leaked into HTML), Google Drive/Chat integration, and Codex CLI integration via /ndf:codex skill. Transcript retention is automatically kept at >= 90 days. BREAKING (v4.0.0): Codex MCP server is removed (use /ndf:codex skill); legacy CLAUDE.ndf.md detection hook and /ndf:cleanup skill are removed (obsolete since v3.0.0). Serena MCP is a separate plugin (mcp-serena).", "author": { "name": "takemi-ohama", diff --git a/plugins/ndf/skills/cross-review/SKILL.md b/plugins/ndf/skills/cross-review/SKILL.md index f7e3478..ffa3c3c 100644 --- a/plugins/ndf/skills/cross-review/SKILL.md +++ b/plugins/ndf/skills/cross-review/SKILL.md @@ -269,6 +269,23 @@ pint / larastan / test / build などは **中断** を原則とする。 - ❌ **`pgrep -fa ` で完了判定** — gemini は long prompt が引数に乗り検知失敗。pidfile 必須 - ❌ **sentinel 単独で完了判定** — codex がクラッシュすると永遠に出ない。`monitor.py` の多軸判定 (pidfile / sentinel / 早期エラー / stall / hard timeout / result.json) を使うこと - ❌ **タイムアウトなしで wait** — ハング検知不能。`monitor.py` の hard timeout (30 分既定) + stall timeout (10 分既定) を必ず効かせる +- ❌ **EARLY_ERROR の曖昧パターンで kill する** — 行頭の生 `Error:` / `Traceback` は codex がレビュー対象 diff の test コード片を echo するケースで誤検知する。明確な致命 (auth / quota / sandbox / HTTP 401-403-429 / gemini の YOLO 降格) **のみ** kill 対象とし、曖昧パターンは警告ログに留める。誤検知が再発する場合は `--no-early-error` / `MONITOR_NO_EARLY_ERROR=1` で検知自体を無効化する (sentinel / result.json / timeout で十分判定可能) + +## monitor.py が誤って kill する場合の手順 + +`monitor.py` が EARLY_ERROR で codex / gemini を即時 kill してしまい、`result.json` が +生成されないケースは以下で切り分け・回避できる: + +1. **err.log の冒頭を確認**: 検知パターン (`fatal_err` の `early error (fatal) in err.log: ...`) が + 本当に致命なのか、それとも diff body の echo / config validation 警告なのかを判別 +2. **gemini の `Error in: mcpServers.` 警告**: `.gemini/settings.json` に `disabled: false` + 等の非互換キーがあると毎回出る。`launch-gemini.sh` の sanitize ロジック (v4.7.2+) で + 自動退避するため、最新版にアップデートすれば解消する +3. **誤検知が継続する場合**: `monitor.py --no-early-error` (もしくは `MONITOR_NO_EARLY_ERROR=1` + 環境変数) で EARLY_ERROR 検知自体を無効化し、hard timeout / stall / sentinel / result.json + のみで判定するモードに切り替える +4. **新しい致命パターンを観測した場合**: `EARLY_ERROR_FATAL` に追記する (PR で plugin に反映)。 + 曖昧パターンは `EARLY_ERROR_WARN` 側に置き、kill 対象にはしない - ❌ **fix サブエージェントが Resolve をスキップ** — reply だけでは未対応扱い。Resolve まで実行 - ❌ **review body に identifier prefix を付け忘れる** — GitHub UI 上で誰のレビューか不明になる diff --git a/plugins/ndf/skills/cross-review/scripts/launch-gemini.sh b/plugins/ndf/skills/cross-review/scripts/launch-gemini.sh index fd9e1c6..adcdbe7 100755 --- a/plugins/ndf/skills/cross-review/scripts/launch-gemini.sh +++ b/plugins/ndf/skills/cross-review/scripts/launch-gemini.sh @@ -97,11 +97,42 @@ $EXISTING_INLINE EOF cd "$WORKTREE" + +# gemini-cli 最新版は mcpServers エントリの `disabled` キーを Unrecognized 扱いし、 +# 起動時に `Error in: mcpServers.` 警告を err.log に出す。文字列としては +# `Error: ...` ではなく `Error in: ...` だが、monitor.py 旧版が誤検知して +# プロセスを kill する原因になっていた (REPORT01 参照)。 +# launcher 側でも sanitize して警告自体を抑制する: `disabled` キーを再帰的に +# 削除した settings.json を起動時のみ差し込み、gemini が読み終わったら復元する。 +SETTINGS=$WORKTREE/.gemini/settings.json +SETTINGS_BACKUP= +if [ -f "$SETTINGS" ] && command -v jq >/dev/null 2>&1; then + SETTINGS_BACKUP=$TMP_DIR/gemini-review-pr$STATE_PR-settings-backup.json + cp "$SETTINGS" "$SETTINGS_BACKUP" + SANITIZED=$TMP_DIR/gemini-review-pr$STATE_PR-settings-sanitized.json + if jq 'walk(if type == "object" then del(.disabled) else . end)' "$SETTINGS_BACKUP" > "$SANITIZED" 2>/dev/null; then + cp "$SANITIZED" "$SETTINGS" + else + # jq が失敗したら sanitize を諦め、バックアップも破棄して元のまま起動 + rm -f "$SETTINGS_BACKUP" + SETTINGS_BACKUP= + fi +fi + # ⚠ --skip-trust と GEMINI_CLI_TRUST_WORKSPACE=true は両方必須 GEMINI_CLI_TRUST_WORKSPACE=true nohup gemini --yolo --skip-trust --output-format text \ -p "$(cat "$PROMPT")" \ > $TMP_DIR/gemini-review-pr$STATE_PR-stdout.log \ 2> $TMP_DIR/gemini-review-pr$STATE_PR-err.log & -echo $! > $TMP_DIR/gemini-review-pr$STATE_PR.pid +GEMINI_PID=$! +echo $GEMINI_PID > $TMP_DIR/gemini-review-pr$STATE_PR.pid disown -echo "🚀 gemini launched (pid=$(cat $TMP_DIR/gemini-review-pr$STATE_PR.pid))" >&2 + +# gemini は起動時に 1 度 settings.json を読む。読み込み完了を待ってから元の +# ファイルを復元する (worktree を dirty なままにしないため)。 +if [ -n "$SETTINGS_BACKUP" ] && [ -f "$SETTINGS_BACKUP" ]; then + sleep 2 + mv "$SETTINGS_BACKUP" "$SETTINGS" +fi + +echo "🚀 gemini launched (pid=$GEMINI_PID)" >&2 diff --git a/plugins/ndf/skills/cross-review/scripts/monitor.py b/plugins/ndf/skills/cross-review/scripts/monitor.py index 12aaf2f..feafd09 100755 --- a/plugins/ndf/skills/cross-review/scripts/monitor.py +++ b/plugins/ndf/skills/cross-review/scripts/monitor.py @@ -14,17 +14,21 @@ - 可能なら `/proc//cmdline` で codex/gemini であることを再確認 (PID 再利用対策) 2. **sentinel** (codex のみ): err.log に `^tokens used$` 出現 3. **early-error pattern**: err.log に既知の致命的キーワードが出たら即中断 + - **FATAL** (auth/quota/sandbox 等の明確な致命): 検知時に kill + - **WARN** (生の `Error:` / `Traceback` 等の曖昧パターン): 警告ログのみ、kill せず通常判定を継続 + - `--no-early-error` / `MONITOR_NO_EARLY_ERROR=1` で検知自体を無効化可 4. **result.json**: プロセス終了後に `/tmp/-review-pr-result.json` が 生成されていなければ失敗扱い 5. **hard timeout**: 既定 7 分。`--timeout` または `MONITOR_TIMEOUT` で上書き可 6. **stall timeout**: err.log + stdout.log の合計サイズが既定 3 分変化しなければ STALLED として中断 (`--stall-timeout` または `MONITOR_STALL` で上書き可) - 7. **失敗時 kill**: TIMEOUT / STALLED / EARLY_ERROR / PIDFILE_BAD で返るとき、 - 対象プロセスを SIGTERM (3 秒後に SIGKILL) で停止する + 7. **失敗時 kill**: TIMEOUT / STALLED / EARLY_ERROR (FATAL のみ) / PIDFILE_BAD で + 返るとき、対象プロセスを SIGTERM (3 秒後に SIGKILL) で停止する Usage: monitor.py target ∈ {codex, gemini, both} monitor.py both --timeout 1200 --stall-timeout 600 + monitor.py both --no-early-error # EARLY_ERROR 検知を完全無効化 Exit codes (target=both は最悪値を返す): 0 OK プロセス正常終了 + result.json 確認 @@ -58,27 +62,45 @@ DEFAULT_TIMEOUT = int(os.environ.get("MONITOR_TIMEOUT", "420")) # 7 min DEFAULT_STALL = int(os.environ.get("MONITOR_STALL", "180")) # 3 min no progress DEFAULT_POLL = int(os.environ.get("MONITOR_POLL", "15")) # 15 sec - -# err.log の行頭に近い形で出る致命的パターン（substring 検索ではない）。 -# `^` (行頭) を必須として、diff のコード本文 / doc の引用 / インラインコメント本文に -# 同じキーワードが出ても誤検知しないようにする。 -EARLY_ERROR_PATTERNS = [ - # 行頭または `: ` の直後など、典型的な error 出力フォーマット - re.compile(r"^(?:Error|FATAL|fatal|panic|PANIC|Traceback)[: ]", re.MULTILINE), +# `MONITOR_NO_EARLY_ERROR=1` で EARLY_ERROR 検知を無効化 (escape hatch) +DEFAULT_NO_EARLY_ERROR = os.environ.get("MONITOR_NO_EARLY_ERROR", "").lower() in { + "1", "true", "yes", "on", +} + +# err.log の行頭に近い形で出る **明確な致命** パターン (kill 対象)。 +# auth / quota / sandbox / HTTP 401-403-429 / gemini の YOLO 降格など、 +# プロセスが続行しても result を生成できないと判明しているケースだけを入れる。 +EARLY_ERROR_FATAL = [ # HTTP エラーステータス行 (`HTTP/1.1 401 Unauthorized` 等) re.compile(r"^HTTP/\d\S* (?:401|403|429) ", re.MULTILINE), # gemini 固有: untrusted directory で YOLO が降格される re.compile(r'^Approval mode overridden to "default"', re.MULTILINE), - # 認証 / クオータ系（行頭限定） + # 認証 / 権限系（行頭限定） re.compile(r"^(?:Authentication failed|Permission denied)", re.MULTILINE), + # quota / rate limit re.compile(r"^.*\b(?:quota exceeded|rate limit exceeded)\b", re.MULTILINE | re.IGNORECASE), + # API key 系 re.compile(r"^.*\bAPI key (?:not found|missing|invalid)", re.MULTILINE | re.IGNORECASE), - # codex 固有: sandbox エラー（行頭 + 末尾近辺） + # codex 固有: sandbox エラー re.compile(r"^.*\bsandbox error\b", re.MULTILINE | re.IGNORECASE), ] -# 文脈に含まれていたら benign（doc 引用 / コードレビューコメント等）と見なし誤検知扱い +# 行頭の生 `Error:` / `Traceback` 系は **kill しない警告のみ** に降格。 +# - gemini-cli の `Error in: mcpServers.` 警告 (起動時の config 検証) +# - codex がレビュー対象 diff の test コード片を echo して `Traceback` が混入するケース +# など、続行可能な誤検知が頻発するため。プロセスは sentinel / result.json / timeout +# で別途判定する。 +EARLY_ERROR_WARN = [ + re.compile(r"^(?:Error|FATAL|fatal|panic|PANIC|Traceback)[: ]", re.MULTILINE), +] + +# 文脈に含まれていたら benign（doc 引用 / コードレビューコメント等）と見なし誤検知扱い。 +# FATAL / WARN 双方のスキャンに適用される。 EARLY_ERROR_BENIGN = [ + # gemini-cli の config validation 警告 (`Error in: mcpServers.` / `Error in: ...`): + # 設定の Unrecognized キーを通知するだけで、gemini 本体は起動継続する。 + re.compile(r"^Error in: mcpServers\.", re.MULTILINE), + re.compile(r"^Error in: \S+\s*$", re.MULTILINE), # diff のコンテキスト行 (` `, `+`, `-` で始まり、その後 markdown 表記が来る) re.compile(r"^[ +-].*[\|`]", re.MULTILINE), # markdown のリスト / 引用 @@ -205,11 +227,17 @@ def _pid_cmdline_matches(pid: int, expected: str) -> Optional[bool]: return None -def _scan_early_errors(path: pathlib.Path) -> Optional[str]: +def _scan_patterns( + path: pathlib.Path, patterns: list[re.Pattern[str]] +) -> Optional[str]: + """err.log を末尾 200KB だけ読み、`patterns` の最初のヒット行を返す。 + + BENIGN フィルタは前後 40 文字の文脈で適用し、誤検知 (markdown 引用 / diff body / + gemini の config validation 警告) を除外する。 + """ if not path.exists(): return None try: - # 末尾 200KB のみ読む（巨大化対策） sz = path.stat().st_size with path.open("rb") as f: if sz > 200 * 1024: @@ -218,17 +246,15 @@ def _scan_early_errors(path: pathlib.Path) -> Optional[str]: except OSError: return None - for pat in EARLY_ERROR_PATTERNS: + for pat in patterns: m = pat.search(data) if not m: continue - # 直前後 80 文字を見て benign パターンと重なってないか確認 start = max(0, m.start() - 40) end = min(len(data), m.end() + 40) context = data[start:end] if any(b.search(context) for b in EARLY_ERROR_BENIGN): continue - # 周辺 1 行を返す line_start = data.rfind("\n", 0, m.start()) + 1 line_end = data.find("\n", m.end()) line_end = line_end if line_end != -1 else len(data) @@ -236,6 +262,14 @@ def _scan_early_errors(path: pathlib.Path) -> Optional[str]: return None +def _scan_early_fatal(path: pathlib.Path) -> Optional[str]: + return _scan_patterns(path, EARLY_ERROR_FATAL) + + +def _scan_early_warn(path: pathlib.Path) -> Optional[str]: + return _scan_patterns(path, EARLY_ERROR_WARN) + + def _scan_codex_sentinel(path: pathlib.Path) -> bool: if not path.exists(): return False @@ -258,9 +292,14 @@ def monitor_agent( stall_timeout: int, poll: int, require_result: bool, + no_early_error: bool = False, log_prefix: str = "", ) -> AgentStatus: - """1 agent を監視する。""" + """1 agent を監視する。 + + `no_early_error=True` のとき、EARLY_ERROR 検知 (FATAL/WARN とも) を完全に無効化し、 + hard timeout / stall / sentinel / result.json のみで判定する。 + """ paths = AgentPaths.for_(agent, pr) status = AgentStatus(agent=agent) started = time.monotonic() @@ -288,6 +327,7 @@ def monitor_agent( last_err_size = paths.err_log.stat().st_size if paths.err_log.exists() else 0 last_progress = time.monotonic() cmdline_validated = False + warned_early_error = False while True: elapsed = time.monotonic() - started @@ -344,15 +384,29 @@ def monitor_agent( return status # 3. early error - err = _scan_early_errors(paths.err_log) - if err: - if alive: - _kill_pid(pid) - status.status = "EARLY_ERROR" - status.exit_code = 4 - status.detail = f"early error in err.log: {err[:200]}" - _emit_log(log_prefix, agent, status) - return status + # 明確な致命 (FATAL) のみ kill する。曖昧パターン (生 Error: / Traceback) は + # WARN として警告ログのみ。codex がレビュー対象 diff の test コード片を + # echo するケースや gemini の config validation 警告で誤 kill されるのを防ぐ。 + if not no_early_error: + fatal_err = _scan_early_fatal(paths.err_log) + if fatal_err: + if alive: + _kill_pid(pid) + status.status = "EARLY_ERROR" + status.exit_code = 4 + status.detail = f"early error (fatal) in err.log: {fatal_err[:200]}" + _emit_log(log_prefix, agent, status) + return status + + if not warned_early_error: + warn_err = _scan_early_warn(paths.err_log) + if warn_err: + print( + f"{log_prefix}⚠️ {agent} early-error WARN " + f"(non-fatal, not killing): {warn_err[:200]}", + file=sys.stderr, flush=True, + ) + warned_early_error = True if not alive: # プロセス終了 — result.json を確認 @@ -432,6 +486,11 @@ def main() -> None: help=f"poll interval in seconds (default: {DEFAULT_POLL})") p.add_argument("--no-require-result", action="store_true", help="プロセス終了後に result.json が無くても OK 扱い") + p.add_argument("--no-early-error", action="store_true", + default=DEFAULT_NO_EARLY_ERROR, + help="EARLY_ERROR 検知を無効化 " + "(hard timeout / stall / sentinel / result.json のみで判定) " + f"[env: MONITOR_NO_EARLY_ERROR; default: {DEFAULT_NO_EARLY_ERROR}]") args = p.parse_args() agents = ["codex", "gemini"] if args.target == "both" else [args.target] @@ -444,6 +503,7 @@ def run(agent: str) -> None: agent=agent, pr=args.pr, timeout=args.timeout, stall_timeout=args.stall_timeout, poll=args.poll, require_result=require_result, + no_early_error=args.no_early_error, log_prefix=f"[{agent}] ", ) From 32f23084f13c5124ea6aeba672e8c6dfbd56f86b Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Wed, 20 May 2026 04:31:32 +0000 Subject: [PATCH 2/2] =?UTF-8?q?Fix:=20cross-review=20monitor.py=20/=20laun?= =?UTF-8?q?ch-gemini.sh=20=E3=81=AE=E3=83=AC=E3=83=93=E3=83=A5=E3=83=BC?= =?UTF-8?q?=E6=8C=87=E6=91=98=E5=AF=BE=E5=BF=9C?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #3 round 1 のレビュー指摘 2 件に対応する。 1. monitor.py `_scan_patterns` の見逃しバグ (gemini major) - `pat.search()` は最初の一致しか返さないため、最初の一致が benign で 除外されると後続の本物エラーを見逃していた - `pat.finditer()` に変更し、benign で除外された場合も後続を継続走査 - さらに benign 判定を「マッチ行そのもの」に対して行うように変更 (以前は前後 40 文字の文脈窓を使っていたが、benign 行が直前にある だけで後続の本物エラーが誤って benign 扱いされる別バグがあった) 2. launch-gemini.sh の sanitize 済み settings.json 残留リスク (codex major / gemini minor) - sleep 2 中 / 復元前に SIGTERM/Ctrl-C/シェル終了で止まると、sanitize 済み settings.json が worktree に残る問題 - `trap restore_settings EXIT INT TERM HUP` を登録して、どの経路で終了 しても必ずバックアップから復元するように修正 - restore_settings は冪等 (SETTINGS_BACKUP が未設定 or バックアップ不在 なら何もしない) なので多重実行されても安全 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../cross-review/scripts/launch-gemini.sh | 17 ++++++++- .../skills/cross-review/scripts/monitor.py | 38 +++++++++++-------- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/plugins/ndf/skills/cross-review/scripts/launch-gemini.sh b/plugins/ndf/skills/cross-review/scripts/launch-gemini.sh index adcdbe7..c688c05 100755 --- a/plugins/ndf/skills/cross-review/scripts/launch-gemini.sh +++ b/plugins/ndf/skills/cross-review/scripts/launch-gemini.sh @@ -106,6 +106,20 @@ cd "$WORKTREE" # 削除した settings.json を起動時のみ差し込み、gemini が読み終わったら復元する。 SETTINGS=$WORKTREE/.gemini/settings.json SETTINGS_BACKUP= + +# trap で EXIT / INT / TERM / HUP のいずれでも必ず settings.json を復元する。 +# sleep 2 中や復元前に Ctrl-C / SIGTERM / シェル終了で止まっても、sanitize 済み +# settings.json が worktree に残らないようにするため。冪等に書いてあるので +# 多重実行されても安全。 +restore_settings() { + # SETTINGS_BACKUP が未設定 or バックアップ不在なら何もしない + if [ -n "${SETTINGS_BACKUP:-}" ] && [ -f "$SETTINGS_BACKUP" ]; then + mv -f "$SETTINGS_BACKUP" "$SETTINGS" 2>/dev/null || true + SETTINGS_BACKUP= + fi +} +trap restore_settings EXIT INT TERM HUP + if [ -f "$SETTINGS" ] && command -v jq >/dev/null 2>&1; then SETTINGS_BACKUP=$TMP_DIR/gemini-review-pr$STATE_PR-settings-backup.json cp "$SETTINGS" "$SETTINGS_BACKUP" @@ -130,9 +144,10 @@ disown # gemini は起動時に 1 度 settings.json を読む。読み込み完了を待ってから元の # ファイルを復元する (worktree を dirty なままにしないため)。 +# sleep 中に signal が来ても trap restore_settings が必ず復元するため安全。 if [ -n "$SETTINGS_BACKUP" ] && [ -f "$SETTINGS_BACKUP" ]; then sleep 2 - mv "$SETTINGS_BACKUP" "$SETTINGS" + restore_settings fi echo "🚀 gemini launched (pid=$GEMINI_PID)" >&2 diff --git a/plugins/ndf/skills/cross-review/scripts/monitor.py b/plugins/ndf/skills/cross-review/scripts/monitor.py index feafd09..89cb1d8 100755 --- a/plugins/ndf/skills/cross-review/scripts/monitor.py +++ b/plugins/ndf/skills/cross-review/scripts/monitor.py @@ -230,10 +230,19 @@ def _pid_cmdline_matches(pid: int, expected: str) -> Optional[bool]: def _scan_patterns( path: pathlib.Path, patterns: list[re.Pattern[str]] ) -> Optional[str]: - """err.log を末尾 200KB だけ読み、`patterns` の最初のヒット行を返す。 + """err.log を末尾 200KB だけ読み、`patterns` の最初の **non-benign** ヒット行を返す。 - BENIGN フィルタは前後 40 文字の文脈で適用し、誤検知 (markdown 引用 / diff body / - gemini の config validation 警告) を除外する。 + BENIGN フィルタは **マッチ行そのもの** に対して適用し、誤検知 (markdown 引用 / + diff body / gemini の config validation 警告) を除外する。 + + 重要 1: `pat.search()` ではなく `pat.finditer()` を使い、benign で除外された場合も + 後続の一致を継続して走査する。これにより benign な先行ヒットの後ろにある本物の + エラーを見逃さない。 + + 重要 2: benign 判定は「マッチ行」だけを対象にする。以前は前後 40 文字の文脈窓を + 使っていたが、それだと benign 行が直前にあるだけで後続の本物エラーを誤って benign + 扱いしてしまった (例: `Error in: mcpServers.serena\\n...\\nTraceback ...` で + Traceback が誤抑制された)。 """ if not path.exists(): return None @@ -247,18 +256,17 @@ def _scan_patterns( return None for pat in patterns: - m = pat.search(data) - if not m: - continue - start = max(0, m.start() - 40) - end = min(len(data), m.end() + 40) - context = data[start:end] - if any(b.search(context) for b in EARLY_ERROR_BENIGN): - continue - line_start = data.rfind("\n", 0, m.start()) + 1 - line_end = data.find("\n", m.end()) - line_end = line_end if line_end != -1 else len(data) - return data[line_start:line_end].strip() + for m in pat.finditer(data): + line_start = data.rfind("\n", 0, m.start()) + 1 + line_end = data.find("\n", m.end()) + line_end = line_end if line_end != -1 else len(data) + line = data[line_start:line_end] + # benign パターンはマッチ行そのものに当てる。markdown 引用や + # `Error in: mcpServers.X` のような行単位パターンは「その行」だけを + # 評価すれば判定可能で、文脈窓を広げると誤判定の原因になる。 + if any(b.search(line) for b in EARLY_ERROR_BENIGN): + continue + return line.strip() return None