diff --git a/issues/PLAN21_cross-review-gemini-stall-and-fix-merge.md b/issues/PLAN21_cross-review-gemini-stall-and-fix-merge.md new file mode 100644 index 0000000..ccfc803 --- /dev/null +++ b/issues/PLAN21_cross-review-gemini-stall-and-fix-merge.md @@ -0,0 +1,415 @@ +# PLAN21: cross-review の gemini stall タイムアウト分岐 と fix サブエージェント戻り値マージの堅牢化 + +- 起票日: 2026-05-23 +- 対象 plugin: `ndf` v4.7.3 +- 対象 skill: `ndf:cross-review` +- 関連 issue: [issues/i18-issue-gemini.md](./i18-issue-gemini.md) +- 関連 plan (先行): [issues/PLAN20_cross-review-worktree-and-result-schema-fix.md](./PLAN20_cross-review-worktree-and-result-schema-fix.md) (PR #4 で merge 済み) +- 報告者: takemi-ohama (`devbasex/devbase#19` の `/ndf:cross-review 19` 実行中に検出) + +## 背景・課題 + +`/ndf:cross-review` を v4.7.2 (PLAN20 適用前) で実走させた際に、cross-review ループ +自体は完走したが、メインセッション側で手動補正が必要だった運用不整合が i18 に 3 点 +報告されている。PLAN20 (v4.7.3 で merge) によって以下が解消済み: + +- i18 #2 (gemini `result.json` スキーマ揺れ) — `launch-gemini.sh` の明示 JSON ブロック化と + `state.py cmd_read_result` の `event`/`intent` フォールバック + 欠落 die で対応済 + +本 PLAN21 は、PLAN20 では扱われなかった **残り 2 件** を対象とする: + +### 問題1: gemini の stall タイムアウト既定 (180s) が gemini に対して短すぎる + +`scripts/monitor.py:63` の以下が原因: + +```python +DEFAULT_STALL = int(os.environ.get("MONITOR_STALL", "180")) # 3 min no progress +``` + +- err.log + stdout.log の合計サイズが 180 秒変化しなければ STALLED 扱いで kill する設計 +- codex は推論ステップを逐次 err.log に書き出すため stall 検知が機能する +- gemini はリクエスト送信〜レスポンス受領まで err.log に**ほぼ何も書かない** (起動直後の + `YOLO mode is enabled.` 等 340B のみ)。「サイズ非変化 = stall」前提と相性が悪い +- 結果として gemini プロセスを 1 度目に毎回 STALLED で kill してしまい、孤児プロセスも + 残る (skill 既知の挙動: `pkill` 後も子プロセスが生き残るケースあり) +- 再実行で `--stall-timeout 480 --timeout 900` を明示すると 195 秒で正常終了する実績あり + +ユーザはこの挙動を学習して毎回 `--stall-timeout 480` を明示しているが、本来は plugin が +agent ごとの特性に応じた既定値を持つべき。 + +### 問題2: fix サブエージェント戻り値ファイルのパス指定が散逸し、merge-fix が +silent に fail する + +`scripts/state.py:403` (`cmd_merge_fix`) は以下のみを参照する: + +```python +ffile = pathlib.Path(args.file or _tmp_dir() / f"fix-pr{pr}-result.json") +``` + +`_tmp_dir()` は環境変数 `CROSS_REVIEW_TMP_DIR` または `~/.gemini/tmp//` を返す +(例: `/Users/takemi_ohama/.gemini/tmp/pr19/`)。 + +一方、SKILL.md の Step 5 例文や mermaid 図には `/tmp/fix-pr<#>-result.json` がハードコード +されており、メインセッションが Agent tool に渡すプロンプトでも `/tmp/fix-pr19-result.json` +が使われた結果、サブエージェントは `/tmp/` に書き込み、`state.py merge-fix` は +`~/.gemini/tmp/pr19/fix-pr19-result.json` を見て **「戻り値ファイルが生成されなかった」 +で exit 3 (ci-code-fail 扱い)** になる事象が発生した。 + +加えて、サブエージェント側が `commit_sha`/`fixed` のキーで戻り値を作ってしまうケースが +あり、`cmd_merge_fix` が期待する `fix_commit`/`fixed_count` キーが取れず `fixed=0` で +silent に記録される事象も観測されている。 + +両方とも i18 issue 第 3 節で実証されており、SKILL.md / docs と `cmd_merge_fix` の二面で +対応する必要がある。 + +### 補足: i18 issue #2 (gemini `result.json` スキーマ) は PLAN20 で対応済み + +PLAN20 で以下が入っているため、本 PLAN21 では再対応しない: + +- `scripts/launch-gemini.sh:90` に codex と同等の JSON スキーマブロックを明示 +- `scripts/state.py cmd_read_result` で `event`/`intent` フォールバック + 欠落 die +- ユニットテスト (`tests/test_state_read_result.py`) + +i18 issue #2 の報告は v4.7.2 環境での観測であり、v4.7.3 では `intent` 別名でも state に +正しく取り込まれる。万一 v4.7.3 でも何らかのスキーマ漏れが残るようなら別 issue として +切り出す。 + +## ゴール + +1. gemini プロセスを 1 度目に STALLED で kill する誤検知を解消する (per-agent 既定の + stall タイムアウト) +2. fix サブエージェントの戻り値ファイル受け渡しでパス指定ズレや key 名ズレが起きても + `state.py merge-fix` が **適切な fallback で拾い上げる**か、もしくは明示的に fail + して原因が分かるようにする +3. SKILL.md / docs から `/tmp/fix-pr<#>-result.json` ハードコードを排除し、 + `$TMP_DIR/fix-pr-result.json` で統一する +4. 既存の Linux コンテナ環境 / `CROSS_REVIEW_TMP_DIR` 明示環境の挙動は変えない (後方互換) + +## 設計方針 + +### 1. per-agent DEFAULT_STALL の導入 (問題1) + +`monitor.py` に agent 別の既定値 env を追加し、`monitor_agent()` 呼び出し前の解決時に +agent 名で切り替える。 + +`scripts/monitor.py` 設定セクションを以下に変更: + +```python +DEFAULT_TIMEOUT = int(os.environ.get("MONITOR_TIMEOUT", "420")) # 7 min +# 既定 stall timeout (後方互換のため env MONITOR_STALL は残す)。 +# `MONITOR_STALL` は両 agent 共通のデフォルトとして引き続き受け付ける。 +DEFAULT_STALL = int(os.environ.get("MONITOR_STALL", "180")) # 3 min no progress +# per-agent 上書き (gemini は stdout/stderr に進捗を出さないため大きめ)。 +# `MONITOR_STALL_` が指定されていなければ DEFAULT_STALL_AGENT_BUILTIN を使う。 +DEFAULT_STALL_AGENT_BUILTIN = { + "codex": 180, # 推論ログを逐次出すので 3 min で十分 + "gemini": 480, # err.log が静かなため 8 min まで許容 +} + + +def _agent_stall_default(agent: str) -> int: + env_key = f"MONITOR_STALL_{agent.upper()}" + if env_key in os.environ: + return int(os.environ[env_key]) + if "MONITOR_STALL" in os.environ: + return DEFAULT_STALL + return DEFAULT_STALL_AGENT_BUILTIN.get(agent, DEFAULT_STALL) +``` + +`main()` の per-agent ループで、`--stall-timeout` が CLI 引数で明示されていなければ +agent ごとの既定を採用する形に変更: + +```python +# argparse 側: default は文字列 "auto" を入れ、明示時のみ int として上書き +p.add_argument("--stall-timeout", default=None, + help="stall timeout (err.log no progress) in seconds. " + "未指定時は agent 別既定 (codex=180, gemini=480) または " + "env MONITOR_STALL_ / MONITOR_STALL") + +# ループ内: +def run(agent: str) -> None: + stall = int(args.stall_timeout) if args.stall_timeout is not None \ + else _agent_stall_default(agent) + results[agent] = monitor_agent( + agent=agent, pr=args.pr, + timeout=args.timeout, stall_timeout=stall, + ... + ) +``` + +**ポイント**: + +- CLI 引数 `--stall-timeout` の明示優先 (既存ユーザの上書きを尊重) +- env `MONITOR_STALL_CODEX` / `MONITOR_STALL_GEMINI` で per-agent 上書き +- env `MONITOR_STALL` 共通指定も後方互換で残す (両 agent に同じ値) +- いずれも未指定なら agent 別ビルトイン (codex=180s, gemini=480s) +- 既定が変わるのは gemini のみ (180 → 480)。codex は不変 + +### 2. ドキュメント追記 (問題1 派生) + +`docs/01-state-and-review.md` の monitor 説明 (Step 0〜2 周辺) に以下を追記: + +- 「**gemini は err.log がほぼ無音のため、stall timeout の既定は 480 秒** にしている」 +- 「上書きしたい場合は `MONITOR_STALL_GEMINI` env か `--stall-timeout` を使う」 +- 既存の「`--stall-timeout` または `MONITOR_STALL` で上書き可」記述は per-agent + 既定の説明と整合させる + +`SKILL.md` Step 2 の monitor.py 呼び出し例には現状 `--stall-timeout` が無いため +追記不要。 + +### 3. `cmd_merge_fix` の fallback と key alias (問題2 本体) + +`state.py cmd_merge_fix` を以下に変更: + +```python +def cmd_merge_fix(args: argparse.Namespace) -> None: + """Step 5 後段 — fix サブエージェント戻り値を state にマージ + CI 分類。 + + Exit code: 0=continue, 3=ci-code-fail (final=error) + """ + pr = args.pr + + # 戻り値ファイルの探索順: + # 1. --file 明示 + # 2. $TMP_DIR/fix-pr-result.json (正規) + # 3. /tmp/fix-pr-result.json (メインセッションが旧プロンプトで /tmp を指定した場合の救済) + candidates: list[pathlib.Path] = [] + if args.file: + candidates.append(pathlib.Path(args.file)) + candidates.append(_tmp_dir() / f"fix-pr{pr}-result.json") + candidates.append(pathlib.Path(f"/tmp/fix-pr{pr}-result.json")) + + ffile: pathlib.Path | None = None + for c in candidates: + if c.exists() and c.stat().st_size > 0: + ffile = c + break + if ffile is None: + die( + "fix サブエージェントが戻り値ファイルを生成しなかった " + f"(checked: {[str(c) for c in candidates]})", + code=3, + ) + + fix = json.loads(ffile.read_text()) + + # key 名 fallback (サブエージェントが別名で書いた場合の救済。仕様としては + # fix_commit / fixed_count が正、commit_sha / fixed は別名) + fix_commit = fix.get("fix_commit") or fix.get("commit_sha") + fixed_count = fix.get("fixed_count") + if fixed_count is None: + fixed_count = fix.get("fixed", 0) + + st = _load(pr) + if not st.get("rounds"): + die("state.rounds が空。`state.py start-round` を先に呼んでください", code=3) + round_no = st["rounds"][-1]["round"] + + st["rounds"][-1]["fix"] = { + "commit": fix_commit, + "fixed": fixed_count, + "deferred": len(fix.get("deferred", []) or []), + "rejected": len(fix.get("rejected", []) or []), + "resolved_threads": len(fix.get("resolved_threads", []) or []), + "ci": fix.get("ci_status"), + "ci_failed_checks": fix.get("ci_failed_checks", []) or [], + "ci_note": fix.get("ci_note"), + "by_severity": fix.get("by_severity", {}), + } + st["rounds"][-1]["ended_at"] = _now() + for d in (fix.get("deferred") or []): + st["deferred_nits"].append({**d, "pr": pr, "round": round_no}) + _save(pr, st) + + # 以降 CI 分類は既存のままだが、ローカル変数で書き換え: + if (fix.get("ci_status") or "").upper() != "FAILURE": + info(f"✅ fix マージ完了 (commit={fix_commit} fixed={fixed_count})") + return + ... +``` + +**ポイント**: + +- 探索順は (1) `--file` 明示 → (2) `_tmp_dir()` → (3) `/tmp/` の 3 段 +- 全候補が無ければ list を含めて die (どこを見たか分かる) +- key 別名は `commit_sha` / `fixed` の 1 段のみ。それ以外は将来別名が出てきた時に追加 +- 既存仕様 (`fix_commit` / `fixed_count` で書く) はそのまま動く + +### 4. SKILL.md / docs のパス記述統一 (問題2 派生) + +以下のハードコードを `$TMP_DIR/fix-pr-result.json` に統一する: + +- `SKILL.md:129` (mermaid 図中の `→ /tmp/fix-pr<#>-result.json`) +- `SKILL.md:193` (Step 5 説明コメント `# Step 5: 修正サブエージェント起動 (Agent tool) → /tmp/fix-pr-result.json`) +- `SKILL.md:322` (fix 戻り値ファイル節 `(/tmp/fix-pr-result.json)`) +- `SKILL.md:374` の「`/tmp/` に置き」表現は **「`$TMP_DIR/` (= `_tmp_dir()` 解決先) に置き」** に + 書き換え (general な「すべて tmp dir」の意味のため、ハードコードは避ける) + +`docs/02-fix-and-rotation.md` は既に `$TMP_DIR/fix-pr-result.json` 表現になっており修正不要。 + +**追加**: `docs/02-fix-and-rotation.md` の Step 5 手順節に **`$TMP_DIR` の解決順 +(env `CROSS_REVIEW_TMP_DIR` > `~/.gemini/tmp//` > `/tmp/`) を 1 行で明示** +してメインセッション側がパスを書き出す際の参考になるようにする。 + +### 5. テスト (軽量) + +`plugins/ndf/skills/cross-review/tests/` 配下に以下を追加: + +**`tests/test_monitor_stall_default.py`** (新規): + +- `_agent_stall_default("codex")` がビルトイン 180 を返すこと +- `_agent_stall_default("gemini")` がビルトイン 480 を返すこと +- env `MONITOR_STALL_GEMINI=600` 設定時に 600 が返ること +- env `MONITOR_STALL=240` 設定時に両 agent ともに 240 が返ること (env 上書き） +- `MONITOR_STALL_GEMINI` と `MONITOR_STALL` の両方が設定された場合は per-agent 優先 + +**`tests/test_state_merge_fix.py`** (新規): + +- 正規パス (`_tmp_dir()/fix-prN-result.json`) + 正規 key (`fix_commit` / `fixed_count`) + → state に正しくマージされる +- 正規パスに無く `/tmp/fix-prN-result.json` にある場合 → fallback で拾える +- key が `commit_sha` / `fixed` の別名でも fix.commit / fix.fixed が埋まる +- 戻り値ファイルが 1 箇所も無い場合は `SystemExit(3)` で die + 探索 path 一覧が + stderr に出る + +`pytest` は PLAN20 同様 CI 強制せず、開発者ローカルで +`uv run pytest plugins/ndf/skills/cross-review/tests` で全 pass を確認する。 + +### 6. バージョン更新 + +- `plugins/ndf/.claude-plugin/plugin.json` を `4.7.3` → `4.7.4` (patch bump、バグ修正のみ) +- 開発履歴 (`CHANGELOG.md` または `README.md` の該当節) に v4.7.4 のエントリ追加 + +## 実装タスク + +### Phase 1: monitor.py per-agent stall (問題1 本体) +- [ ] `scripts/monitor.py` 設定セクションに `DEFAULT_STALL_AGENT_BUILTIN` 辞書と + `_agent_stall_default(agent)` ヘルパを追加 +- [ ] `main()` の `--stall-timeout` argparse default を `None` に変更し、help を更新 +- [ ] `main()` `run(agent)` 内で per-agent 既定を解決して `monitor_agent()` に渡す +- [ ] 既存の `DEFAULT_STALL` 定数は残し、`MONITOR_STALL` env 共通指定の挙動を維持 + +### Phase 2: ドキュメント追記 +- [ ] `docs/01-state-and-review.md` の monitor 説明節 (Step 0〜2 周辺) に gemini の + stall 既定 480 秒 + 上書き方法を追記 +- [ ] 既存「`--stall-timeout` または `MONITOR_STALL` で上書き可」記述に per-agent + 既定 + `MONITOR_STALL_` env を追記 +- [ ] codex 側既定 180 秒は不変である旨も明記 (gemini だけ調整した意図を残す) + +### Phase 3: state.py cmd_merge_fix の堅牢化 (問題2 本体) +- [ ] `cmd_merge_fix` に candidates list ベースの探索ロジックを実装 (3 段 fallback) +- [ ] 全候補不在時に die メッセージへ探索 path 一覧を含める +- [ ] key 別名 (`commit_sha`, `fixed`) フォールバックを `fix_commit` / `fixed_count` に + 対して追加 +- [ ] info ログを新しい変数 (`fix_commit`, `fixed_count`) ベースに書き換え +- [ ] CI 分類ロジック以降 (line 430〜) は既存挙動を保つ + +### Phase 4: SKILL.md / docs のパス記述統一 (問題2 派生) +- [ ] `SKILL.md:129` の mermaid 中 `→ /tmp/fix-pr<#>-result.json` を + `→ $TMP_DIR/fix-pr<#>-result.json` に +- [ ] `SKILL.md:193` の Step 5 コメント `→ /tmp/fix-pr-result.json` を + `→ $TMP_DIR/fix-pr-result.json` に +- [ ] `SKILL.md:322` `(/tmp/fix-pr-result.json)` を `($TMP_DIR/fix-pr-result.json)` に +- [ ] `SKILL.md:374` 周辺の「すべて `/tmp/` に置き」表現を「すべて `$TMP_DIR/` + (= `_tmp_dir()` 解決先) に置き」に +- [ ] `docs/02-fix-and-rotation.md` Step 5 手順節に `$TMP_DIR` の解決順を 1 行追記 + +### Phase 5: テスト追加 +- [ ] `tests/test_monitor_stall_default.py`: + - [ ] codex / gemini ビルトイン値ケース + - [ ] env `MONITOR_STALL_GEMINI` 上書きケース + - [ ] env `MONITOR_STALL` 共通指定ケース (両 agent に効く) + - [ ] per-agent env と共通 env 併用時に per-agent 優先となること +- [ ] `tests/test_state_merge_fix.py`: + - [ ] 正規パス + 正規 key ケース + - [ ] `/tmp/` fallback パスケース + - [ ] `commit_sha` / `fixed` 別名 key ケース + - [ ] 全候補不在で `SystemExit(3)` + 探索 path 一覧 stderr ケース +- [ ] ローカルで `uv run pytest plugins/ndf/skills/cross-review/tests` を回し全 pass + +### Phase 6: バージョン更新 +- [ ] `plugins/ndf/.claude-plugin/plugin.json` `version` を `4.7.4` に +- [ ] 開発履歴 (CHANGELOG / README の該当節) に v4.7.4 エントリ追加 +- [ ] エントリ内容: monitor per-agent stall, merge-fix fallback / key alias, docs 統一 + +### Phase 7: 検証 (手動) +- [ ] `monitor.py both` を `--stall-timeout` 引数なしで実行し、gemini 側が 480s + まで stall を待つことを log で確認 (例: 200s 経過時点で stall 判定されない) +- [ ] codex 側は引き続き 180s で stall になることを確認 +- [ ] サブエージェントが `/tmp/fix-pr-result.json` に書いた状態で + `state.py merge-fix ` を呼び、`$TMP_DIR` ではなく `/tmp/` の戻り値を + 拾って state にマージされることを確認 +- [ ] サブエージェントが `{"commit_sha":"abc","fixed":3,...}` で戻り値を書いた状態で + `state.py merge-fix` を呼び、state.rounds[-1].fix.commit / fixed に値が入る + ことを確認 +- [ ] 全候補に戻り値ファイルが無い状態で `state.py merge-fix` を呼び、exit 3 と + 探索 path 一覧の stderr を確認 + +## PR 構成 + +**単一 PR で実装する** (合計差分は ~180 行以内の見込み、release branch 不要): + +- branch: `fix/PLAN21-cross-review-gemini-stall-and-fix-merge` +- base: `main` +- 流れ: + 1. 上記 Phase 1〜6 を順次 commit (Phase 単位で分けると review しやすい) + 2. `/ndf:review-branch` でセルフレビュー + 3. `/ndf:pr` で PR 作成 (本 plan を `## Plan` セクションで参照) + 4. レビュー対応後 squash merge + +multi-PR 化が必要になりそうな兆候 (例: monitor 側の per-agent 化が大きく波及する、 +fixture が増える) が出てきたら、Phase 1〜2 (monitor) と Phase 3〜4 (state + docs) で +分割する余地はあるが、現状の差分規模では単一 PR で十分とみなす。 + +## 互換性方針 + +- **後方互換あり**: + - `--stall-timeout` を CLI で明示しているユーザの挙動は不変 + - `MONITOR_STALL` env 指定ユーザは両 agent ともその値が適用される (既存と同じ) + - 既存の正規 fix 戻り値 (`fix_commit` / `fixed_count` キー、`$TMP_DIR` 配下) も + 引き続き正規系 +- **新規挙動**: + - 何も指定していない場合の gemini 既定が 180 → 480 秒に増加 (kill されにくくなる) + - `cmd_merge_fix` が `/tmp/` fallback で戻り値を拾う + - `commit_sha` / `fixed` 別名 key も受理 + - 戻り値不在時のエラーメッセージに探索 path 一覧が含まれる +- 旧挙動でユーザが暗黙依存していた可能性のあるもの (gemini 既定が 180s であること) は、 + 挙動として **緩める方向**のため破壊的ではないと判断。CHANGELOG で言及する。 + +## リスクと対策 + +| リスク | 対策 | +|---|---| +| gemini 既定 480s で実際にハングしている時の検出が 5 分遅れる | `DEFAULT_TIMEOUT=420` は据え置きのため hard timeout は変わらない (5 min 経過時点でハード kill) | +| `/tmp/` fallback で他プロセスの古い fix-prN-result.json を拾う | 候補は **size > 0 かつ exists** で確認、かつ 1 番目に `--file` 明示と `_tmp_dir()` を優先するため fallback は最終手段 | +| 別名 key 対応が将来の本物のスキーマ違反を隠す | 別名は `commit_sha` / `fixed` の 1 段のみ。それ以外は silent に None になる現挙動と同じ (悪化しない) | +| per-agent 既定変更でテストや CI が回らなくなる | 既定変更は gemini のみ。codex 利用箇所は影響なし | +| `--stall-timeout` argparse の default 変更 (`int → None`) で他から呼ばれている script が壊れる | `monitor.py` は CLI 経由でのみ呼ばれており、helper として import している箇所は無いことを `grep` で確認の上で対応 | + +## 完了の定義 + +- [ ] `monitor.py both` を引数なしで実行したとき、gemini 側の stall timeout が + 480 秒で適用される +- [ ] codex 側の stall timeout は引き続き 180 秒 +- [ ] env `MONITOR_STALL_GEMINI=600` 指定でその値が gemini にだけ反映される +- [ ] `state.py merge-fix ` がサブエージェント戻り値を `$TMP_DIR` → `/tmp/` の + 順で探索し、どちらにあっても拾える +- [ ] サブエージェントが `commit_sha` / `fixed` 別名で書いても fix.commit / + fix.fixed に値が入る +- [ ] 全候補不在時に exit 3 + 探索 path 一覧の stderr が出る +- [ ] 追加した pytest が全て pass する +- [ ] plugin version が `4.7.4` に上がり、CHANGELOG / 開発履歴に v4.7.4 の節がある +- [ ] SKILL.md から `/tmp/fix-pr<#>-result.json` ハードコードが取り除かれている + (mermaid / Step 5 例 / 戻り値節 / 「すべて `/tmp/`」記述) + +## 参考 + +- [issues/i18-issue-gemini.md](./i18-issue-gemini.md) — 元 issue (再現手順 / 回避策 / 修正提案を含む) +- [issues/PLAN20_cross-review-worktree-and-result-schema-fix.md](./PLAN20_cross-review-worktree-and-result-schema-fix.md) — 先行 plan (#2 を解消) +- 該当コード: + - `plugins/ndf/skills/cross-review/scripts/monitor.py:63` (DEFAULT_STALL) + - `plugins/ndf/skills/cross-review/scripts/monitor.py:510-540` (main / argparse) + - `plugins/ndf/skills/cross-review/scripts/state.py:397-432` (cmd_merge_fix) + - `plugins/ndf/skills/cross-review/SKILL.md:129,193,322,374` (`/tmp/fix-pr...` ハードコード) + - `plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md:28,121,159` (既に `$TMP_DIR`) +- 関連 PR: `devbasex/devbase#19` (再現発生 PR、検証参考。v4.7.2 で観測) diff --git a/issues/i18-issue-gemini.md b/issues/i18-issue-gemini.md new file mode 100644 index 0000000..a22833d --- /dev/null +++ b/issues/i18-issue-gemini.md @@ -0,0 +1,158 @@ +# ndf plugin (4.7.2) cross-review skill 修正提案 + +> 報告者: takemi-ohama +> 発見日: 2026-05-23 +> 対象 plugin: `ai-plugins/ndf/4.7.2` — `skills/cross-review/` +> 検証 PR: devbasex/devbase #19 (Round 1 で収束。両 AI とも COMMENT/0 critical/0 major) + +PR #19 で `/ndf:cross-review` を実走させた際に観測した、運用上の不整合 3 点をまとめる。 +いずれも cross-review ループ自体は完走したが、メインセッション側で手動補正が必要だった。 + +## 1. gemini の stall-timeout デフォルト (180s) が短すぎる + +### 現状 + +- `skills/cross-review/scripts/monitor.py:63` + ```python + DEFAULT_STALL = int(os.environ.get("MONITOR_STALL", "180")) # 3 min no progress + ``` +- 既定 stall = 3 分。err.log + stdout.log の合計サイズに変化が無いと STALLED 扱いで kill される。 + +### 観測した事象 (PR #19 Round 1) + +- gemini プロセス pid=26149 を起動 → 180 秒の間 err.log は 340B から増えず STALLED 判定 → kill +- 1 度目の launch で gemini node プロセスが「子側」だったため、`pkill` 後も実プロセス (pid 26155) が + 生き残っていた (skill `アンチパターン` でも触れている既知の挙動) +- `--stall-timeout 480 --timeout 900` で再起動したところ、**195 秒**で正常終了 (`result.json` 生成済) + - sentinel は最後まで出ず (`sentinel_seen: false`)、`result_exists=True` のみで判定された +- 観測ログ (抜粋): + ``` + [gemini] ⏳ gemini elapsed=180s pid=26968 err_log=340B sentinel=- + [gemini] ✅ gemini OK (195s) — process exited; sentinel=False; result_exists=True + ``` + +### 原因の推定 + +- codex は推論ステップを逐次 err.log に書き出すため `err_log_size` が増え続け stall 検知に引っかからない +- gemini はリクエスト送信〜レスポンス受領まで **err.log にほぼ何も書かない** + (起動直後の `YOLO mode is enabled.` 等の 340B のみ)。「サイズ非変化 = stall」前提と相性が悪い + +### 提案 + +1. **agent 別に DEFAULT_STALL を分岐**: + ```python + DEFAULT_STALL_CODEX = int(os.environ.get("MONITOR_STALL_CODEX", "180")) + DEFAULT_STALL_GEMINI = int(os.environ.get("MONITOR_STALL_GEMINI", "480")) + ``` + `monitor.py` の per-agent ループで切り替える。 +2. 上記が大きすぎる変更なら、**gemini については stall 判定を緩める fallback** を入れる: + - `result.json` 不在 + プロセス生存 + sentinel 無し のときは hard timeout のみで判定 +3. docs (`docs/01-state-and-review.md` 周辺) に「**gemini は err.log が静かなため stall-timeout を + ≥ 480 秒推奨**」と明記 + +## 2. launch-gemini.sh プロンプトに result.json スキーマが明示されていない + +### 現状 + +- `skills/cross-review/scripts/launch-gemini.sh:90` + ``` + - 投稿後、サマリを **$TMP_DIR/gemini-review-pr$STATE_PR-result.json** に書く + （フォーマットは launch-codex.sh と同じ） + ``` +- codex 側 (`launch-codex.sh:81-90`) は JSON フィールドを inline で例示している: + ```json + { + "event": "REQUEST_CHANGES", + "posted_as": "COMMENT", + "comments_count": 5, + "review_url": "...", + "by_severity": {"critical": 0, "major": 3, "minor": 2, "nit": 0} + } + ``` +- `state.py:248` の `cmd_read_result` は `r.get("event")` 等を **codex の field 名前提で** 読む + +### 観測した事象 (PR #19 Round 1) + +- gemini が以下の **独自スキーマ**で result.json を生成: + ```json + { + "status": "success", + "intent": "COMMENT", + "summary": "S3 URIのパース仕様と依存関係の案内メッセージについて、2点修正を提案します。" + } + ``` +- `state.py read-result 19 gemini` を実行すると `intent=None posted_as=None comments=None` で + state に登録され、judge 段階で `GEMINI_INTENT=None` (= SKIP 扱い) になりかけた +- 実際には gemini は GitHub にレビュー (id 4350631675) と inline コメント 2 件を正しく投稿していた + ため、メインセッション側で gh api から拾い直して result.json を手書きする羽目になった + +### 提案 + +1. **launch-gemini.sh のプロンプトに codex と同じ JSON 例を inline 展開**する + (「launch-codex.sh と同じ」は LLM 側からは読めない参照のため不十分) +2. もしくは `state.py read-result` に **後方互換 field マップ** を入れる: + - `event` が無く `intent` がある → `intent` を採用 + - `comments_count` が無ければ `review_url` から `gh api` で実数を拾う +3. 最小修正としては (1) で十分。長期的には (1) + (2) 両方が望ましい + +## 3. fix サブエージェント戻り値ファイルのパス指定が散逸している + +### 現状 + +- `state.py:368` (merge-fix) + ```python + ffile = pathlib.Path(args.file or _tmp_dir() / f"fix-pr{pr}-result.json") + ``` +- `_tmp_dir()` は環境変数 `CROSS_REVIEW_TMP_DIR` または `~/.gemini/tmp//` を返す + (本検証では `/Users/takemi_ohama/.gemini/tmp/pr19/`) +- 一方、skill 本文 (`docs/02-fix-and-rotation.md` 周辺) の例や、メインセッションが Agent tool に + 渡すプロンプトでは **`/tmp/fix-pr<#>-result.json`** が登場する箇所がある (要監査) + +### 観測した事象 (PR #19 Round 1) + +- general-purpose サブエージェントが `/tmp/fix-pr19-result.json` に書き込み完了 +- メイン側で `state.py merge-fix 19` を実行 → `❌ fix サブエージェントが戻り値ファイルを生成しなかった` + で exit=3 (= ci-code-fail 扱い) になった +- 手動で `cp /tmp/fix-pr19-result.json $CROSS_REVIEW_TMP_DIR/` してから再実行で復旧 + +### 提案 + +1. skill docs (`docs/02-fix-and-rotation.md` および `SKILL.md` の Step 5 例) に + **「サブエージェントに渡すパスは必ず `$CROSS_REVIEW_TMP_DIR/fix-pr-result.json`」** + と明記し、`/tmp/...` の例を排除する +2. `state.py merge-fix` に **fallback** を 1 段追加: + ```python + for candidate in [args.file, _tmp_dir() / f"fix-pr{pr}-result.json", + pathlib.Path(f"/tmp/fix-pr{pr}-result.json")]: + if candidate and candidate.exists() and candidate.stat().st_size > 0: + ffile = candidate; break + ``` + メインセッションのプロンプト誤りを救済できる +3. さらに、サブエージェントのキー名も `commit_sha`/`fixed` で来てしまうと + `state.py merge-fix` が `fix_commit`/`fixed_count` を期待して fixed=0 と記録される + (本検証で発生)。同じく後方互換 map を入れるのが望ましい + +## 影響まとめ + +| # | 影響 | 回避策 (現状) | 提案後の効果 | +|---|---|---|---| +| 1 | gemini が毎回 1 度目に STALLED で kill される | `--stall-timeout 480` を明示 | 既定で 1 発成功、孤児プロセスも減る | +| 2 | gemini 結果が state に正しく載らず judge 誤動作の恐れ | 手動で result.json を書き直す | スキーマ統一で手動補正不要 | +| 3 | merge-fix が失敗し final=error 扱いになるリスク | 手動 cp で復旧 | fallback または docs 統一で防止 | + +## 関連ファイル + +- `skills/cross-review/scripts/monitor.py` (#1) +- `skills/cross-review/scripts/launch-gemini.sh` (#2) +- `skills/cross-review/scripts/launch-codex.sh` (#2 参照元) +- `skills/cross-review/scripts/state.py` (#2, #3) +- `skills/cross-review/docs/01-state-and-review.md` (#1 ドキュメント追記) +- `skills/cross-review/docs/02-fix-and-rotation.md` (#3 ドキュメント追記) +- `skills/cross-review/SKILL.md` (#3 メインプロンプト例の修正) + +## 補足: 本検証の環境 + +- macOS Darwin 25.5.0 / bash / claude-opus-4-7[1m] +- ndf plugin: `~/.claude/plugins/cache/ai-plugins/ndf/4.7.2/` +- worktree path: `/work/` が read-only のため `--worktree $HOME/cross-review-worktrees/pr19` を明示指定 + (これは別途 docs 側に「`/work/` 不在時の代替パス例」を追記すると親切) diff --git a/plugins/ndf/.claude-plugin/plugin.json b/plugins/ndf/.claude-plugin/plugin.json index 1fb2f11..e2709cc 100644 --- a/plugins/ndf/.claude-plugin/plugin.json +++ b/plugins/ndf/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "ndf", - "version": "4.7.3", + "version": "4.7.4", "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/CHANGELOG.md b/plugins/ndf/CHANGELOG.md index ff7737c..bcd75bd 100644 --- a/plugins/ndf/CHANGELOG.md +++ b/plugins/ndf/CHANGELOG.md @@ -1,5 +1,56 @@ # NDF Plugin CHANGELOG +### v4.7.4 (cross-review: gemini stall 既定の per-agent 化 + fix 戻り値マージの堅牢化) + +`/ndf:cross-review` を実走させた際に、cross-review ループ自体は完走するが +メインセッション側で手動補正が必要だった運用不整合の残り 2 件を恒久対応する +PATCH リリース。 + +- **monitor.py: per-agent stall timeout 既定の導入** (`skills/cross-review/scripts/monitor.py`): + - 従来は `MONITOR_STALL=180s` の単一既定値だったため、err.log にほぼ進捗を出さない + gemini を 1 度目に毎回 STALLED 扱いで kill していた (孤児プロセスも残存)。 + - 既定を agent 別ビルトインに変更: **codex=180s (変更なし) / gemini=480s**。 + - 解決順: CLI `--stall-timeout` (明示優先) > env `MONITOR_STALL_` (per-agent) > + env `MONITOR_STALL` (両 agent 共通、後方互換) > agent 別ビルトイン。 + - `--stall-timeout` の argparse default を `int` → `None` に変更し、未指定時のみ + per-agent 解決を行う。 +- **state.py merge-fix: fix 戻り値ファイルの探索 fallback と key 別名対応** + (`skills/cross-review/scripts/state.py`): + - サブエージェントが `/tmp/fix-pr-result.json` に書き、`merge-fix` は + `$TMP_DIR` (= `~/.gemini/tmp//`) のみ参照する不整合で + exit 3 (ci-code-fail 扱い) になる事象を解消。探索順を **(1) `--file` 明示 + → (2) `$TMP_DIR/` → (3) `/tmp/`** の 3 段に変更。 + - 全候補不在時の die メッセージに探索 path 一覧を含め、原因切り分け可能にした。 + - サブエージェントが `commit_sha` / `fixed` 別名キーで書き出した場合も + `fix_commit` / `fixed_count` と同等に受理する key alias を追加 (silent な + `fixed=0` 記録の救済)。 +- **SKILL.md / docs のパス記述統一**: + - `SKILL.md` 内の `/tmp/fix-pr<#>-result.json` ハードコード 3 箇所を + `$TMP_DIR/fix-pr<#>-result.json` に統一。 + - 「すべて `/tmp/` に置き」の汎用記述も `$TMP_DIR/` (= `_tmp_dir()` 解決先) に修正。 + - `docs/02-fix-and-rotation.md` に `$TMP_DIR` の解決順 + (env `CROSS_REVIEW_TMP_DIR` > `~/.gemini/tmp//` > `/tmp/`) を 1 行明示。 + - `docs/01-state-and-review.md` の monitor 説明表に per-agent stall 既定の + 解決順を追記。 +- **pytest 追加** (`skills/cross-review/tests/`): + - `test_monitor_stall_default.py` — codex/gemini ビルトイン + env 上書き 6 ケース。 + - `test_state_merge_fix.py` — 正規 / `/tmp/` fallback / key 別名 / 全候補不在 / + `--file` 明示の 5 ケース。 +- **関連 issue / plan**: + - `issues/i18-issue-gemini.md` (再現報告) / + `issues/PLAN21_cross-review-gemini-stall-and-fix-merge.md` (実装プラン)。 + +#### 既存ユーザへの影響 + +- `--stall-timeout` を CLI で明示しているユーザ: **挙動不変**。 +- `MONITOR_STALL` env を指定しているユーザ: **挙動不変** (両 agent に同じ値が適用)。 +- 何も指定していないユーザ: gemini 側の既定 stall timeout が 180s → 480s に + **緩和**される (kill されにくくなる方向のため非破壊)。 +- 正規パス (`$TMP_DIR/fix-pr-result.json`) + 正規 key (`fix_commit` / + `fixed_count`) で書き出していたユーザ: **挙動不変**。 +- 旧プロンプトで `/tmp/` ハードコードや別名 key を使っていたユーザ: **silent fail + していたケースが拾われるようになる** (修復方向の変更)。 + ### v4.7.3 (cross-review: macOS 対応 worktree base + result.json スキーマ堅牢化) `/ndf:cross-review` を非 Linux コンテナ環境 (macOS / WSL 等) でも素直に動かせるよう、 diff --git a/plugins/ndf/skills/cross-review/SKILL.md b/plugins/ndf/skills/cross-review/SKILL.md index 009e3dd..90dfd0f 100644 --- a/plugins/ndf/skills/cross-review/SKILL.md +++ b/plugins/ndf/skills/cross-review/SKILL.md @@ -126,7 +126,7 @@ flowchart TD Gemini --> Decide Decide -->|両方 APPROVE / SKIP| Approved([final = approved]):::ok - Decide -->|一方でも REQUEST_CHANGES| Fix["Agent (general-purpose)
/ndf:fix <PR> --defer-nit を worktree 内で実行
・critical/major/minor 修正 + push
・reply + resolveReviewThread
・deferred/rejected は reply のみ
→ /tmp/fix-pr<#>-result.json"] + Decide -->|一方でも REQUEST_CHANGES| Fix["Agent (general-purpose)
/ndf:fix <PR> --defer-nit を worktree 内で実行
・critical/major/minor 修正 + push
・reply + resolveReviewThread
・deferred/rejected は reply のみ
→ $TMP_DIR/fix-pr<#>-result.json"] Fix --> Check{収束チェック} Check -->|max-rounds 到達| MaxR([final = max_rounds]):::stop @@ -190,7 +190,7 @@ while :; do # Step 4: 振動検知 (4=oscillation) "$SCRIPTS/state.py" check-oscillation "$STATE_PR" || [ $? -eq 2 ] || exit 4 - # Step 5: 修正サブエージェント起動 (Agent tool) → /tmp/fix-pr-result.json + # Step 5: 修正サブエージェント起動 (Agent tool) → $TMP_DIR/fix-pr-result.json # - メインで Agent(subagent_type=general-purpose, ...) を呼ぶ。docs/02 参照 # - tmp パスは launcher / monitor.py と同じく **STATE_PR ベース** で統一 # Step 5 後段: fix 戻り値マージ + CI 分類 (3=code-fail で中断) @@ -319,7 +319,7 @@ body に書くのは **設計レベル・PR 横断の修正提案** のみ。 ## CI failure の分類（誤中断防止） 「CI 失敗 → 即 `final=error`」は乱暴。`scripts/state.py merge-fix` が -fix 戻り値ファイル (`/tmp/fix-pr-result.json`) を受け取った際に +fix 戻り値ファイル (`$TMP_DIR/fix-pr-result.json`) を受け取った際に `ci_failed_checks` を以下で分類する: | 分類 | パターン | 振る舞い | @@ -371,7 +371,8 @@ pint / larastan / test / build などは **中断** を原則とする。 ## メイン context 節約の工夫 1. **大きいファイルはメイン context に載せない**: payload / err.log / diff は - すべて `/tmp/` に置き、メインは state.json と result.json だけ読む + すべて `$TMP_DIR/` (= `state.py _tmp_dir()` の解決先) に置き、メインは + state.json と result.json だけ読む 2. **サブエージェント分離**: 修正は別 context window で実行 3. **PR ローテーション**: 1 PR あたりの会話履歴を抑える 4. **AI 直接投稿**: 中間ペイロードがメインを通らない diff --git a/plugins/ndf/skills/cross-review/docs/01-state-and-review.md b/plugins/ndf/skills/cross-review/docs/01-state-and-review.md index e18ae55..0f724c7 100644 --- a/plugins/ndf/skills/cross-review/docs/01-state-and-review.md +++ b/plugins/ndf/skills/cross-review/docs/01-state-and-review.md @@ -143,7 +143,7 @@ fi | pidfile + `kill -0` | プロセス生存確認。alive 確認後に `/proc//cmdline` で agent 名一致も検証 (PID 再利用対策)。**プロセスが既に死んでいる場合は result.json の有無のみで OK 判定**する (死亡直後 cmdline 不一致で誤検知しないため) | | codex sentinel | err.log に `^tokens used$` 出現で正常完了マーク | | early-error | **行頭限定** で `^Error:` / `^FATAL:` / `^panic:` / `^Traceback ` / `^HTTP/1.1 401\|403\|429` / `^Approval mode overridden to "default"` / `^Authentication failed` / 「quota exceeded」「rate limit exceeded」「API key not found/missing/invalid」「sandbox error」を含む行を検出 (diff/doc 引用文中の同語句は誤検知しないよう anchor + benign フィルタ併用) | -| stall timeout | err.log + stdout.log の合計サイズが既定 **3 分** 変化しなければ STALLED で中断 (`--stall-timeout` or `MONITOR_STALL` env) | +| stall timeout | err.log + stdout.log の合計サイズが一定時間変化しなければ STALLED で中断。既定は **agent 別** (codex=**180s** / gemini=**480s**)。gemini は err.log がほぼ無音のため大きめに取る。codex 側既定は不変。上書き方法: CLI `--stall-timeout` (明示優先) > env `MONITOR_STALL_` (per-agent) > env `MONITOR_STALL` (両 agent 共通) > agent 別ビルトイン | | hard timeout | 既定 **7 分**。`--timeout` or `MONITOR_TIMEOUT` env で上書き | | result.json 存在 | プロセス終了後、result.json が無ければ NO_RESULT (exit 3) | | **失敗時 kill** | TIMEOUT / STALLED / EARLY_ERROR / PIDFILE_BAD で返るときは対象プロセスに SIGTERM → 3 秒後 SIGKILL。残存プロセスが後から `gh api` 投稿や result.json 書き込みを行うのを防ぐ | diff --git a/plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md b/plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md index f758503..40e9239 100644 --- a/plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md +++ b/plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md @@ -26,6 +26,8 @@ 4. nit / 判断が割れる minor は **修正せず deferred 記録**（reply は `[deferred / nit]` ラベル付き、Resolve しない） 5. **PR レベルの Summary コメントを `gh pr comment` で投稿**（対応件数 / 重要度別 / deferred 件数 / rejected 件数 / commit SHA を含む） 6. 戻り値ファイル `$TMP_DIR/fix-pr-result.json` を必ず書き出す + （`$TMP_DIR` は env `CROSS_REVIEW_TMP_DIR` > `~/.gemini/tmp//` > `/tmp/` の順で解決。 + 詳細は `scripts/state.py _tmp_dir()` 参照。`/tmp/` 直書きでも `state.py merge-fix` は fallback で拾う） > ⚠ inline thread への reply + Resolve **だけでは不十分**。PR ページの > conversation タブに表示される **PR レベルコメント** がレビュアーへの diff --git a/plugins/ndf/skills/cross-review/scripts/monitor.py b/plugins/ndf/skills/cross-review/scripts/monitor.py index 3c25e7a..ae57bb3 100755 --- a/plugins/ndf/skills/cross-review/scripts/monitor.py +++ b/plugins/ndf/skills/cross-review/scripts/monitor.py @@ -20,8 +20,10 @@ 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` で上書き可) + 6. **stall timeout**: err.log + stdout.log の合計サイズが一定時間変化しなければ + STALLED として中断。既定は agent 別 (codex=180s, gemini=480s。gemini は err.log + にほぼ進捗を出さないため大きめ)。`--stall-timeout` で CLI 明示、 + `MONITOR_STALL_` env で per-agent 上書き、`MONITOR_STALL` env で共通上書き可 7. **失敗時 kill**: TIMEOUT / STALLED / EARLY_ERROR (FATAL のみ) / PIDFILE_BAD で 返るとき、対象プロセスを SIGTERM (3 秒後に SIGKILL) で停止する @@ -59,9 +61,23 @@ # ---------- 設定 ---------- -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 +# 既定値は import 時に **固定数値** で保持する。env (`MONITOR_TIMEOUT` / +# `MONITOR_STALL` / `MONITOR_POLL`) の解釈は **呼び出し時** に try/except +# 付きで行い、非数値 env でも import / 監視プロセスがクラッシュしないようにする。 +# (codex round 5 指摘: import 時の `int(os.environ.get(...))` は +# `MONITOR_STALL=abc` のような誤設定で `_agent_stall_default()` に到達する前に +# ValueError で落ちてしまうため) +DEFAULT_TIMEOUT = 420 # 7 min — `--timeout` / env `MONITOR_TIMEOUT` で上書き可 +# 既定 stall timeout (後方互換のため env MONITOR_STALL は残す)。 +# 両 agent 共通のデフォルトとして引き続き受け付ける (解釈は `_agent_stall_default()` 内)。 +DEFAULT_STALL = 180 # 3 min no progress +# per-agent 上書き: gemini は err.log がほぼ無音なため大きめに取る。 +# 解決順は `_agent_stall_default()` 参照。 +DEFAULT_STALL_AGENT_BUILTIN = { + "codex": 180, # 推論ログを逐次出すので 3 min で十分 + "gemini": 480, # err.log が静かなため 8 min まで許容 +} +DEFAULT_POLL = 15 # 15 sec — env `MONITOR_POLL` で上書き可 # `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", @@ -134,6 +150,56 @@ def _match_is_quoted(line: str, match_start: int, match_end: int) -> bool: CODEX_SENTINEL = re.compile(r"^tokens used$", re.MULTILINE) +def _safe_int_env(name: str, fallback: int) -> int: + """env を safe に int parse する。 + + 非数値時は warn を stderr に出して fallback 値を返す。 + `_agent_stall_default()` と同じく、env 設定ミスでプロセスを落とさないため。 + + Note (codex round 5 指摘): `MONITOR_STALL` 等の env を import 時 / 呼び出し時に + 生の `int(...)` で読むと、非数値設定で監視プロセスが起動できなくなる。 + `DEFAULT_TIMEOUT` / `DEFAULT_POLL` も同じ問題を持つため、共通ヘルパに集約。 + """ + if name not in os.environ: + return fallback + raw = os.environ[name] + try: + return int(raw) + except (ValueError, TypeError): + print( + f"⚠ env {name}={raw!r} が int に変換できません — {fallback} を使用", + file=sys.stderr, flush=True, + ) + return fallback + + +def _agent_stall_default(agent: str) -> int: + """agent ごとの既定 stall timeout を解決する。 + + 解決順: + 1. env `MONITOR_STALL_` (per-agent 明示) + 2. env `MONITOR_STALL` (両 agent 共通) + 3. `DEFAULT_STALL_AGENT_BUILTIN[agent]` (codex=180, gemini=480) + 4. `DEFAULT_STALL` (フォールバック) + + Note (codex round 3 指摘): 2 は **呼び出し時** に `os.environ["MONITOR_STALL"]` + を再評価する。`DEFAULT_STALL` モジュール定数は import 時に env を読むため + プロセス起動後の env 変更に追随できず、テストの monkeypatch も効かない。 + + Note (gemini round 4 指摘): env が非数値だった場合 (`int(...)` で + `ValueError` / `TypeError` が裸で上がる) は warn を出して + `DEFAULT_STALL_AGENT_BUILTIN` / `DEFAULT_STALL` にフォールバックする。 + 監視プロセスを env 設定ミスでクラッシュさせない。 + """ + builtin = DEFAULT_STALL_AGENT_BUILTIN.get(agent, DEFAULT_STALL) + env_key = f"MONITOR_STALL_{agent.upper()}" + if env_key in os.environ: + return _safe_int_env(env_key, builtin) + if "MONITOR_STALL" in os.environ: + return _safe_int_env("MONITOR_STALL", builtin) + return builtin + + def _tmp_dir() -> pathlib.Path: """cross-review 用 tmp ディレクトリ。 @@ -511,12 +577,18 @@ def main() -> None: p = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) p.add_argument("pr", type=int) p.add_argument("target", choices=["codex", "gemini", "both"]) - p.add_argument("--timeout", type=int, default=DEFAULT_TIMEOUT, - help=f"hard timeout in seconds (default: {DEFAULT_TIMEOUT})") - p.add_argument("--stall-timeout", type=int, default=DEFAULT_STALL, - help=f"stall timeout (err.log no progress) in seconds (default: {DEFAULT_STALL})") - p.add_argument("--poll", type=int, default=DEFAULT_POLL, - help=f"poll interval in seconds (default: {DEFAULT_POLL})") + # env (MONITOR_TIMEOUT / MONITOR_POLL) は呼び出し時に safe parse で読む。 + # 非数値設定でも fixed default (`DEFAULT_TIMEOUT` / `DEFAULT_POLL`) に戻す。 + timeout_default = _safe_int_env("MONITOR_TIMEOUT", DEFAULT_TIMEOUT) + poll_default = _safe_int_env("MONITOR_POLL", DEFAULT_POLL) + p.add_argument("--timeout", type=int, default=timeout_default, + help=f"hard timeout in seconds (default: {timeout_default})") + p.add_argument("--stall-timeout", type=int, default=None, + help="stall timeout (err.log no progress) in seconds. " + "未指定時は agent 別既定 (codex=180, gemini=480) または " + "env MONITOR_STALL_ / MONITOR_STALL を参照") + p.add_argument("--poll", type=int, default=poll_default, + help=f"poll interval in seconds (default: {poll_default})") p.add_argument("--no-require-result", action="store_true", help="プロセス終了後に result.json が無くても OK 扱い") p.add_argument("--no-early-error", action="store_true", @@ -532,9 +604,11 @@ def main() -> None: results: dict[str, AgentStatus] = {} def run(agent: str) -> None: + stall = args.stall_timeout if args.stall_timeout is not None \ + else _agent_stall_default(agent) results[agent] = monitor_agent( agent=agent, pr=args.pr, - timeout=args.timeout, stall_timeout=args.stall_timeout, + timeout=args.timeout, stall_timeout=stall, poll=args.poll, require_result=require_result, no_early_error=args.no_early_error, log_prefix=f"[{agent}] ", diff --git a/plugins/ndf/skills/cross-review/scripts/state.py b/plugins/ndf/skills/cross-review/scripts/state.py index 0fa9bbe..3a3df80 100755 --- a/plugins/ndf/skills/cross-review/scripts/state.py +++ b/plugins/ndf/skills/cross-review/scripts/state.py @@ -98,6 +98,116 @@ def _now() -> str: return _dt.datetime.now(_dt.timezone.utc).astimezone().isoformat(timespec="seconds") +def _round_started_unixtime(round_entry: dict[str, Any]) -> float | None: + """``round.started_at`` (ISO 8601) を UNIX time (秒) に変換する。 + + fix 戻り値ファイルの mtime と比較して、round 開始前に書かれた古い + ファイルを fallback から除外するために使う。 + パース失敗時は None を返し、呼び出し側で「検証スキップ」を選ばせる。 + """ + started = round_entry.get("started_at") + if not started: + return None + try: + return _dt.datetime.fromisoformat(started).timestamp() + except (TypeError, ValueError): + return None + + +def _is_fresh_fix_result( + path: pathlib.Path, + pr: int, + round_started_ts: float | None, + is_canonical: bool = False, +) -> tuple[bool, dict[str, Any] | None]: + """fallback 候補の fix 戻り値ファイルを採用してよいか判定する。 + + 検証項目: + 1. ファイル mtime が `round_started_ts` 以降であること + (round 開始前に作られた = 古い実行 / 別リポジトリの同番号 PR の残骸) + 2. JSON 内に `pr` フィールドがある場合は対象 PR と一致すること + (`pr` フィールドが無い場合は 1 のみで判定) + + Returns: + ``(is_fresh, parsed_payload)`` のタプル。``is_fresh=True`` の場合のみ + ``parsed_payload`` (dict) が返る。呼び出し側はこれを使って再パースを省略できる。 + + 挙動: + - 古い候補・`pr` 不一致は警告を stderr に出して ``(False, None)`` を返し、 + 呼び出し側で次の候補へ進む。 + - `is_canonical=True` (= ``$TMP_DIR/fix-pr-result.json`` 正規パス) で + **読み取り失敗 (OSError / JSONDecodeError)** が発生した場合のみ + 即時 ``die(code=3)`` する (codex round 2 指摘: 正規パスが壊れているのに + 後続候補へ流れて別 PR の戻り値を誤マージする事故を防ぐ)。 + 正規パスでも `pr` 不一致 / stale mtime は fallback 継続対象とする。 + """ + # 1. mtime チェック + if round_started_ts is not None: + try: + mtime = path.stat().st_mtime + except OSError as exc: + info(f"⚠ fallback 候補 stat 失敗 ({path}): {exc} — skip") + return False, None + if mtime < round_started_ts: + info( + f"⚠ fallback 候補が round 開始前の古いファイル ({path}, " + f"mtime={_dt.datetime.fromtimestamp(mtime).isoformat(timespec='seconds')} " + f"< round_started={_dt.datetime.fromtimestamp(round_started_ts).isoformat(timespec='seconds')}) " + "— skip" + ) + return False, None + + # 2. JSON 内 `pr` フィールドの一致 (任意) + try: + payload = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + if is_canonical: + die( + f"正規パスの fix 戻り値ファイルの読み取りに失敗 ({path}): {exc}。" + " 後続 fallback への流れ込みを防ぐため即時中断。", + code=3, + ) + info(f"⚠ fallback 候補 JSON 解析失敗 ({path}): {exc} — skip") + return False, None + # gemini round 3 指摘: `json.loads` は dict 以外 (list 等) も返す。 + # 後続の `payload.get(...)` や cmd_merge_fix 側の `.get()` でクラッシュしないよう、 + # dict でない場合は warn を出して fallback 不採用 ((False, None)) として扱う。 + # + # codex round 4 指摘: ただし `is_canonical=True` (= 正規パス) で非 dict が返った + # 場合は parse 失敗と同じく即時 die(code=3) する。skip で後続 `/tmp/` fallback に + # 流れると、壊れた正規出力を無視して別実行の戻り値を誤マージする経路が残るため。 + if not isinstance(payload, dict): + if is_canonical: + die( + f"正規パスの fix 戻り値ファイルが dict ではない " + f"({path}, type={type(payload).__name__})。" + " 後続 fallback への流れ込みを防ぐため即時中断。", + code=3, + ) + info( + f"⚠ fallback 候補 JSON が dict ではない ({path}, type={type(payload).__name__}) " + "— skip" + ) + return False, None + file_pr = payload.get("pr") + if file_pr is not None: + try: + file_pr_int = int(file_pr) + except (TypeError, ValueError): + info( + f"⚠ fallback 候補の pr フィールドが数値として解釈できない " + f"({path}, file_pr={file_pr!r}) — skip" + ) + return False, None + if file_pr_int != int(pr): + info( + f"⚠ fallback 候補の pr 不一致 ({path}, file_pr={file_pr} != pr={pr}) " + "— 別 PR の戻り値の可能性。skip" + ) + return False, None + return True, payload + + def _load(pr: int) -> dict[str, Any]: p = _state_path(pr) if not p.exists(): @@ -270,7 +380,20 @@ def cmd_read_result(args: argparse.Namespace) -> None: if not rfile.exists() or rfile.stat().st_size == 0: die(f"{agent}: result 未生成 ({rfile})") - r = json.loads(rfile.read_text()) + try: + r = json.loads(rfile.read_text()) + except json.JSONDecodeError as exc: + die(f"{agent}: result.json の parse に失敗 ({rfile}): {exc}", code=3) + + # gemini round 4 指摘: result.json は本来 dict だが、launcher の出力バグや + # 別実行の残骸で list / str が入り込むと `r.get(...)` で AttributeError になる。 + # 不正な review result はバグなので即時 die(code=3) で停止させる。 + if not isinstance(r, dict): + die( + f"{agent}: result.json が dict ではない " + f"({rfile}, type={type(r).__name__})。review launcher の出力形式不正。", + code=3, + ) # 別名フィールドへのフォールバック (gemini が `intent` / `comment_count` を使う変則 JSON を # 書き出す既知のケースに対応する。仕様としては `event` / `comments_count` が正) @@ -370,7 +493,25 @@ def collect_keys(round_no: int) -> set[str]: payload = json.loads(p.read_text()) except json.JSONDecodeError: continue + # gemini round 4 指摘: payload は本来 dict (comments: [...]) だが、 + # launcher のバグで list / str が入り込むと `payload.get(...)` で + # AttributeError になる。不正な review payload はバグなので + # 即時 die(code=3) で停止させる。 + if not isinstance(payload, dict): + die( + f"{agent}: payload.json が dict ではない " + f"({p}, type={type(payload).__name__})。" + " review launcher の出力形式不正。", + code=3, + ) for c in payload.get("comments", []): + if not isinstance(c, dict): + # comments エントリが dict でない場合も同様に致命扱い + die( + f"{agent}: payload.comments のエントリが dict ではない " + f"({p}, type={type(c).__name__})。", + code=3, + ) path = c.get("path") line = c.get("line") or c.get("start_line") if path and line is not None: @@ -400,19 +541,97 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: Exit code: 0=continue, 3=ci-code-fail (final=error) """ pr = args.pr - ffile = pathlib.Path(args.file or _tmp_dir() / f"fix-pr{pr}-result.json") - if not ffile.exists() or ffile.stat().st_size == 0: - die("fix サブエージェントが戻り値ファイルを生成しなかった", code=3) - fix = json.loads(ffile.read_text()) + # state を先に読み、fallback 検証用の round 開始時刻を取得する + # (round 開始前の古いファイルや、別リポジトリの同番号 PR の戻り値を + # 誤マージするのを防ぐ)。 st = _load(pr) if not st.get("rounds"): die("state.rounds が空。`state.py start-round` を先に呼んでください", code=3) + round_started_ts = _round_started_unixtime(st["rounds"][-1]) + + # 戻り値ファイルの探索順: + # 1. --file 明示 (ユーザー指定なので mtime/pr 検証はスキップ) + # 2. $TMP_DIR/fix-pr-result.json (正規; _tmp_dir() 解決先) + # 3. /tmp/fix-pr-result.json (旧プロンプトで /tmp を指定したサブエージェント救済) + # 2, 3 は PR 番号だけで命名されているため、別 round / 別リポジトリの + # 古い結果を拾わないよう mtime と (あれば) JSON 内の `pr` で検証する。 + explicit = pathlib.Path(args.file) if args.file else None + canonical_path = _tmp_dir() / f"fix-pr{pr}-result.json" + legacy_tmp_path = pathlib.Path(f"/tmp/fix-pr{pr}-result.json") + # (path, is_canonical) のタプル: 正規パス (canonical) の parse 失敗は die(code=3) する + fallback_candidates: list[tuple[pathlib.Path, bool]] = [ + (canonical_path, True), + (legacy_tmp_path, False), + ] + + ffile: pathlib.Path | None = None + fix: dict[str, Any] | None = None + if explicit is not None: + # codex round 4 指摘: `--file` 明示時は fallback 探索に進まず即時失敗させる。 + # ユーザーが特定ファイルを指定しているのに、それが存在しない / 空 / JSON 不正 + # だった場合、無言で fallback に流れて別実行の戻り値を誤マージすると事故になる。 + if not explicit.exists(): + die( + f"--file で指定されたパスが存在しません: {explicit}", + code=3, + ) + if explicit.stat().st_size == 0: + die( + f"--file で指定されたファイルが空です: {explicit}", + code=3, + ) + try: + fix = json.loads(explicit.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + die( + f"--file 指定の fix 戻り値ファイルの読み取り / parse に失敗 " + f"({explicit}): {exc}", + code=3, + ) + # gemini round 3 指摘: `--file` で `list` 等の non-dict JSON が渡されると + # 後続の `fix.get(...)` でクラッシュする。即時 die(code=3) で中断。 + if not isinstance(fix, dict): + die( + f"--file 指定の fix 戻り値ファイルが dict ではない " + f"({explicit}, type={type(fix).__name__})。" + " fix サブエージェント出力の形式不正。", + code=3, + ) + ffile = explicit + # 明示指定は stale 検証スキップ + else: + for c, is_canonical in fallback_candidates: + if not (c.exists() and c.stat().st_size > 0): + continue + is_fresh, parsed = _is_fresh_fix_result(c, pr, round_started_ts, is_canonical=is_canonical) + if not is_fresh: + continue + ffile = c + fix = parsed # 既にパース済みのデータを再利用 (gemini round 2 指摘の性能改善) + break + + if ffile is None or fix is None: + checked = ([str(explicit)] if explicit else []) + [str(c) for c, _ in fallback_candidates] + die( + "fix サブエージェントが戻り値ファイルを生成しなかった " + f"(checked: {checked})", + code=3, + ) + + # key 名 fallback (サブエージェントが別名で書いた場合の救済)。 + # 正規は fix_commit / fixed_count、別名は commit_sha / fixed のみ受理する。 + fix_commit = fix.get("fix_commit") or fix.get("commit_sha") + fixed_count = fix.get("fixed_count") + if fixed_count is None: + fixed_count = fix.get("fixed", 0) + + # `st` は冒頭の fallback 検証で既に load 済み。 round_no = st["rounds"][-1]["round"] st["rounds"][-1]["fix"] = { - "commit": fix.get("fix_commit"), - "fixed": fix.get("fixed_count", 0), + "commit": fix_commit, + "fixed": fixed_count, "deferred": len(fix.get("deferred", []) or []), "rejected": len(fix.get("rejected", []) or []), "resolved_threads": len(fix.get("resolved_threads", []) or []), @@ -428,7 +647,7 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: # CI 分類 if (fix.get("ci_status") or "").upper() != "FAILURE": - info(f"✅ fix マージ完了 (commit={fix.get('fix_commit')} fixed={fix.get('fixed_count', 0)})") + info(f"✅ fix マージ完了 (commit={fix_commit} fixed={fixed_count})") return code_patterns = ("pint", "larastan", "phpstan", "test", "lint", "type", diff --git a/plugins/ndf/skills/cross-review/tests/test_monitor_import_safety.py b/plugins/ndf/skills/cross-review/tests/test_monitor_import_safety.py new file mode 100644 index 0000000..36efc48 --- /dev/null +++ b/plugins/ndf/skills/cross-review/tests/test_monitor_import_safety.py @@ -0,0 +1,105 @@ +"""monitor.py の import 時 env 評価 safety テスト (PLAN21 round 6 / 最終). + +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` / `DEFAULT_STALL` / `DEFAULT_POLL` は import 時に + **固定数値** (420 / 180 / 15) で保持する + - env の解釈は `_agent_stall_default()` / `_safe_int_env()` 内で + try/except 付きで行う (既に round 4 で実装済み) + +本テストは fixture 経由ではなく **毎回 fresh に source loader で import** し、 +非数値 env が設定された状態でも import が成功することを確認する。 +""" +from __future__ import annotations + +import importlib.util +import pathlib +import sys +import types + +import pytest + + +_HERE = pathlib.Path(__file__).resolve().parent +_MONITOR = _HERE.parent / "scripts" / "monitor.py" + + +def _fresh_import_monitor(name: str = "cross_review_monitor_fresh") -> types.ModuleType: + """毎回 fresh に monitor.py を import する (sys.modules キャッシュを使わない)。 + + session-scoped fixture (`monitor_mod`) は最初の import 時の env を保持してしまい + 本テストでは「非数値 env がセットされた状態での import が安全か」を検証できない。 + """ + sys.modules.pop(name, None) + spec = importlib.util.spec_from_file_location(name, _MONITOR) + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + sys.modules[name] = mod + spec.loader.exec_module(mod) + return mod + + +def test_import_succeeds_with_non_numeric_monitor_stall(monkeypatch): + """MONITOR_STALL=abc でも monitor.py の import が成功すること (codex round 5).""" + monkeypatch.setenv("MONITOR_STALL", "abc") + mod = _fresh_import_monitor("cross_review_monitor_fresh_stall") + # 固定 default が保たれる + assert mod.DEFAULT_STALL == 180 + # 呼び出し時の解決も builtin にフォールバック (既存 round 4 挙動と同じ) + assert mod._agent_stall_default("codex") == 180 + assert mod._agent_stall_default("gemini") == 480 + + +def test_import_succeeds_with_non_numeric_monitor_timeout(monkeypatch): + """MONITOR_TIMEOUT=xxx でも monitor.py の import が成功すること.""" + monkeypatch.setenv("MONITOR_TIMEOUT", "xxx") + mod = _fresh_import_monitor("cross_review_monitor_fresh_timeout") + # 固定 default が保たれる + assert mod.DEFAULT_TIMEOUT == 420 + # safe parse ヘルパも fallback を返す + assert mod._safe_int_env("MONITOR_TIMEOUT", mod.DEFAULT_TIMEOUT) == 420 + + +def test_import_succeeds_with_non_numeric_monitor_poll(monkeypatch): + """MONITOR_POLL=foo でも monitor.py の import が成功すること.""" + monkeypatch.setenv("MONITOR_POLL", "foo") + mod = _fresh_import_monitor("cross_review_monitor_fresh_poll") + assert mod.DEFAULT_POLL == 15 + assert mod._safe_int_env("MONITOR_POLL", mod.DEFAULT_POLL) == 15 + + +def test_import_succeeds_with_all_non_numeric_envs(monkeypatch): + """3 つ全て非数値でも import に失敗しないこと (組み合わせ防御).""" + monkeypatch.setenv("MONITOR_STALL", "abc") + monkeypatch.setenv("MONITOR_TIMEOUT", "xxx") + monkeypatch.setenv("MONITOR_POLL", "???") + mod = _fresh_import_monitor("cross_review_monitor_fresh_all") + assert mod.DEFAULT_STALL == 180 + assert mod.DEFAULT_TIMEOUT == 420 + assert mod.DEFAULT_POLL == 15 + + +def test_safe_int_env_returns_value_when_numeric(monkeypatch, monitor_mod): + """数値が設定されている場合は素直に int を返す.""" + monkeypatch.setenv("SOME_TEST_INT_ENV", "42") + assert monitor_mod._safe_int_env("SOME_TEST_INT_ENV", 1) == 42 + + +def test_safe_int_env_returns_fallback_when_missing(monkeypatch, monitor_mod): + """env が未設定なら fallback を返す.""" + monkeypatch.delenv("SOME_TEST_INT_ENV", raising=False) + assert monitor_mod._safe_int_env("SOME_TEST_INT_ENV", 99) == 99 + + +def test_safe_int_env_warns_on_non_numeric(monkeypatch, monitor_mod, capsys): + """非数値時は stderr に警告を出して fallback を返す.""" + monkeypatch.setenv("SOME_TEST_INT_ENV", "not-a-number") + assert monitor_mod._safe_int_env("SOME_TEST_INT_ENV", 7) == 7 + captured = capsys.readouterr() + assert "SOME_TEST_INT_ENV" in captured.err + assert "int に変換できません" in captured.err diff --git a/plugins/ndf/skills/cross-review/tests/test_monitor_stall_default.py b/plugins/ndf/skills/cross-review/tests/test_monitor_stall_default.py new file mode 100644 index 0000000..77a4f6b --- /dev/null +++ b/plugins/ndf/skills/cross-review/tests/test_monitor_stall_default.py @@ -0,0 +1,119 @@ +"""monitor.py の per-agent stall timeout 既定値テスト (PLAN21). + +`_agent_stall_default(agent)` の解決優先度: + 1. env `MONITOR_STALL_` (per-agent 明示) + 2. env `MONITOR_STALL` (両 agent 共通) + 3. `DEFAULT_STALL_AGENT_BUILTIN[agent]` (codex=180, gemini=480) + +gemini は err.log にほぼ進捗を出さないため、ビルトイン既定を 480s と大きめに +取って 1 度目の STALLED 誤検知を避ける。codex は従来通り 180s で変更なし。 +""" +from __future__ import annotations + +import pytest + + +def test_builtin_default_codex(monkeypatch, monitor_mod): + """codex のビルトイン既定は 180s。""" + monkeypatch.delenv("MONITOR_STALL", raising=False) + monkeypatch.delenv("MONITOR_STALL_CODEX", raising=False) + monkeypatch.delenv("MONITOR_STALL_GEMINI", raising=False) + assert monitor_mod._agent_stall_default("codex") == 180 + + +def test_builtin_default_gemini(monkeypatch, monitor_mod): + """gemini のビルトイン既定は 480s (codex より大きい)。""" + monkeypatch.delenv("MONITOR_STALL", raising=False) + monkeypatch.delenv("MONITOR_STALL_CODEX", raising=False) + monkeypatch.delenv("MONITOR_STALL_GEMINI", raising=False) + assert monitor_mod._agent_stall_default("gemini") == 480 + + +def test_per_agent_env_overrides_builtin(monkeypatch, monitor_mod): + """env `MONITOR_STALL_GEMINI` 設定で gemini 既定が上書きされる。""" + monkeypatch.delenv("MONITOR_STALL", raising=False) + monkeypatch.setenv("MONITOR_STALL_GEMINI", "600") + assert monitor_mod._agent_stall_default("gemini") == 600 + # codex は影響を受けない + assert monitor_mod._agent_stall_default("codex") == 180 + + +def test_shared_env_applies_to_both(monkeypatch, monitor_mod): + """env `MONITOR_STALL` 共通指定は両 agent に同じ値が適用される (旧挙動互換)。 + + codex round 3 指摘: `_agent_stall_default()` は呼び出し時に env を再評価する + (import 時固定の `DEFAULT_STALL` ではなく `int(os.environ["MONITOR_STALL"])` を返す)。 + 本テストは monkeypatch で `MONITOR_STALL=240` に書き換え、両 agent が 240 を + 返すことを確認する (= 共通 env が実際に反映されることの検証)。 + """ + monkeypatch.delenv("MONITOR_STALL_CODEX", raising=False) + monkeypatch.delenv("MONITOR_STALL_GEMINI", raising=False) + monkeypatch.setenv("MONITOR_STALL", "240") + # 共通 env が両 agent に効く (per-agent 上書きなしの場合) + assert monitor_mod._agent_stall_default("codex") == 240 + assert monitor_mod._agent_stall_default("gemini") == 240 + + +def test_per_agent_env_takes_precedence_over_shared(monkeypatch, monitor_mod): + """per-agent env > 共通 env の優先順位を確認する。""" + monkeypatch.setenv("MONITOR_STALL", "240") + monkeypatch.setenv("MONITOR_STALL_GEMINI", "777") + monkeypatch.delenv("MONITOR_STALL_CODEX", raising=False) + assert monitor_mod._agent_stall_default("gemini") == 777 + # codex 側は per-agent env が無いので 共通 env (= 240) にフォールバック + assert monitor_mod._agent_stall_default("codex") == 240 + + +def test_unknown_agent_falls_back_to_default_stall(monkeypatch, monitor_mod): + """ビルトインに無い agent 名は `DEFAULT_STALL` にフォールバックする。""" + monkeypatch.delenv("MONITOR_STALL", raising=False) + monkeypatch.delenv("MONITOR_STALL_UNKNOWN", raising=False) + assert monitor_mod._agent_stall_default("unknown") == monitor_mod.DEFAULT_STALL + + +# ---------------- PLAN21 round 5: env が非数値だった場合のフォールバック ---------------- + + +def test_shared_env_non_numeric_falls_back_to_builtin(monkeypatch, monitor_mod, capsys): + """env `MONITOR_STALL` が非数値なら builtin にフォールバック (ValueError で落ちない)。 + + gemini round 4 指摘: `int(os.environ[...])` は非数値で ValueError を出す。 + 監視プロセスを env 設定ミスでクラッシュさせないため、try/except で builtin に戻す。 + """ + monkeypatch.delenv("MONITOR_STALL_CODEX", raising=False) + monkeypatch.delenv("MONITOR_STALL_GEMINI", raising=False) + monkeypatch.setenv("MONITOR_STALL", "abc") + # codex / gemini とも builtin 既定 (180 / 480) に戻る + assert monitor_mod._agent_stall_default("codex") == 180 + assert monitor_mod._agent_stall_default("gemini") == 480 + captured = capsys.readouterr() + # 警告メッセージが stderr に出る + assert "MONITOR_STALL" in captured.err + assert "int に変換できません" in captured.err + + +def test_per_agent_env_non_numeric_falls_back_to_builtin( + monkeypatch, monitor_mod, capsys +): + """env `MONITOR_STALL_` が非数値なら builtin にフォールバック。""" + monkeypatch.delenv("MONITOR_STALL", raising=False) + monkeypatch.setenv("MONITOR_STALL_GEMINI", "not-a-number") + monkeypatch.delenv("MONITOR_STALL_CODEX", raising=False) + # gemini は builtin (480) にフォールバック + assert monitor_mod._agent_stall_default("gemini") == 480 + # codex は env 未設定なので builtin (180) + assert monitor_mod._agent_stall_default("codex") == 180 + captured = capsys.readouterr() + assert "MONITOR_STALL_GEMINI" in captured.err + assert "int に変換できません" in captured.err + + +def test_per_agent_env_non_numeric_does_not_affect_other_agent( + monkeypatch, monitor_mod +): + """non-numeric な per-agent env は対象 agent だけに影響する。""" + monkeypatch.delenv("MONITOR_STALL", raising=False) + monkeypatch.setenv("MONITOR_STALL_GEMINI", "xxx") + monkeypatch.setenv("MONITOR_STALL_CODEX", "200") # codex 側は正常 + assert monitor_mod._agent_stall_default("codex") == 200 + assert monitor_mod._agent_stall_default("gemini") == 480 diff --git a/plugins/ndf/skills/cross-review/tests/test_state_check_oscillation.py b/plugins/ndf/skills/cross-review/tests/test_state_check_oscillation.py new file mode 100644 index 0000000..f00d189 --- /dev/null +++ b/plugins/ndf/skills/cross-review/tests/test_state_check_oscillation.py @@ -0,0 +1,101 @@ +"""state.py cmd_check_oscillation の non-dict 防御テスト (PLAN21 round 5). + +gemini round 4 指摘: payload.json が dict 以外 (list 等) の場合、 +`payload.get("comments", [])` で AttributeError になる前に明示的に止める。 +""" +from __future__ import annotations + +import argparse +import json +import pathlib + +import pytest + + +PR = 7777 + + +def _seed_state_two_rounds(tmp_dir: pathlib.Path) -> None: + """同一 PR で 2 round 分のエントリを seed する (oscillation 判定の前提)。""" + state = { + "current_pr": PR, + "rounds": [ + {"round": 1, "pr": PR, "started_at": "2026-05-23T00:00:00+00:00"}, + {"round": 2, "pr": PR, "started_at": "2026-05-23T00:10:00+00:00"}, + ], + "deferred_nits": [], + "final": None, + } + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) + + +def _make_args() -> argparse.Namespace: + return argparse.Namespace(pr=PR) + + +@pytest.fixture() +def patched_tmp_dir(monkeypatch, tmp_path, state_mod): + monkeypatch.setenv("CROSS_REVIEW_TMP_DIR", str(tmp_path)) + return tmp_path + + +def _write_payload( + tmp_dir: pathlib.Path, agent: str, round_no: int, data: object +) -> pathlib.Path: + p = tmp_dir / f"{agent}-review-pr{PR}-round{round_no}-payload.json" + p.write_text(json.dumps(data)) + return p + + +def test_non_dict_payload_dies(patched_tmp_dir, state_mod, capsys): + """payload.json が list 等の非 dict なら die(code=3)。 + + `payload.get(...)` を呼ぶ前に明示的に止めることで、launcher 出力バグや + 別実行の残骸を黙って見逃さない。 + """ + tmp_dir = patched_tmp_dir + _seed_state_two_rounds(tmp_dir) + + # round 1 / round 2 の codex payload を dict で置く (正常 = 比較ベース) + _write_payload(tmp_dir, "codex", 1, {"comments": [{"path": "a.py", "line": 1}]}) + _write_payload(tmp_dir, "codex", 2, {"comments": [{"path": "a.py", "line": 1}]}) + # round 2 の gemini payload は不正 (list) + _write_payload(tmp_dir, "gemini", 2, [{"comments": "should_be_ignored"}]) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_check_oscillation(_make_args()) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "dict ではない" in captured.err + + +def test_non_dict_comment_entry_dies(patched_tmp_dir, state_mod, capsys): + """payload.comments のエントリが dict ではない場合も die(code=3)。""" + tmp_dir = patched_tmp_dir + _seed_state_two_rounds(tmp_dir) + + # round 1 / round 2 とも codex の payload は dict だが、 + # round 2 の comments エントリが str (本来 dict) + _write_payload(tmp_dir, "codex", 1, {"comments": [{"path": "a.py", "line": 1}]}) + _write_payload(tmp_dir, "codex", 2, {"comments": ["not-a-dict-entry"]}) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_check_oscillation(_make_args()) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "dict ではない" in captured.err + + +def test_valid_dict_payloads_dont_die(patched_tmp_dir, state_mod): + """正常な dict payload (regression guard): non-dict 検査が誤検知しないこと。""" + tmp_dir = patched_tmp_dir + _seed_state_two_rounds(tmp_dir) + + # 全 agent の payload を正規の dict で置く (overlap < 50% で continue 期待) + _write_payload(tmp_dir, "codex", 1, {"comments": [{"path": "a.py", "line": 1}]}) + _write_payload(tmp_dir, "codex", 2, {"comments": [{"path": "b.py", "line": 2}]}) + + # exit code 2 (continue) で正常終了 + with pytest.raises(SystemExit) as e: + state_mod.cmd_check_oscillation(_make_args()) + assert e.value.code == 2 diff --git a/plugins/ndf/skills/cross-review/tests/test_state_merge_fix.py b/plugins/ndf/skills/cross-review/tests/test_state_merge_fix.py new file mode 100644 index 0000000..0d7e784 --- /dev/null +++ b/plugins/ndf/skills/cross-review/tests/test_state_merge_fix.py @@ -0,0 +1,565 @@ +"""state.py cmd_merge_fix の堅牢化テスト (PLAN21). + +カバー範囲: + 1. 正規パス + 正規 key (`fix_commit` / `fixed_count`) + 2. 正規パス不在 → `/tmp/` fallback で拾える + 3. 戻り値 key 別名 (`commit_sha` / `fixed`) も受理 + 4. 全候補不在 → exit 3 で die + 探索 path 一覧が stderr に出る + 5. (PLAN21 round 2) fallback の stale 検証 + - mtime が round 開始前 → skip + - JSON 内 `pr` 不一致 → skip + - 明示 `--file` は stale 検証スキップ + 6. (PLAN21 round 3) 正規パス JSON parse 失敗 → 即時 die(code=3) + 7. (PLAN21 round 3) `pr` フィールドが数値として解釈できない → skip + 8. (PLAN21 round 3) `_is_fresh_fix_result` 戻り値のタプル化追従 +""" +from __future__ import annotations + +import argparse +import datetime as _dt +import json +import os +import pathlib + +import pytest + + +PR = 9919 + + +def _seed_state(tmp_dir: pathlib.Path) -> None: + state = { + "current_pr": PR, + "rounds": [ + {"round": 1, "pr": PR, "started_at": "2026-05-23T00:00:00+00:00"} + ], + "deferred_nits": [], + "final": None, + } + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) + + +def _make_args(file_path: pathlib.Path | None = None) -> argparse.Namespace: + return argparse.Namespace(pr=PR, file=str(file_path) if file_path else None) + + +def _read_state(tmp_dir: pathlib.Path) -> dict: + return json.loads((tmp_dir / f"cross-review-pr{PR}-state.json").read_text()) + + +def _canonical_fix() -> dict: + return { + "pr": PR, + "fix_commit": "abc1234", + "ci_status": "SUCCESS", + "ci_failed_checks": [], + "fixed_count": 5, + "by_severity": {"critical": 0, "major": 3, "minor": 2, "nit": 0}, + "resolved_threads": [{"thread_id": "T1"}], + "deferred": [], + "rejected": [], + } + + +@pytest.fixture() +def patched_tmp_dir(monkeypatch, tmp_path, state_mod): + """`CROSS_REVIEW_TMP_DIR` を tmp_path に向けて `_tmp_dir()` を決定的にする。""" + monkeypatch.setenv("CROSS_REVIEW_TMP_DIR", str(tmp_path)) + return tmp_path + + +def test_canonical_path_and_key(patched_tmp_dir, state_mod): + """正規パス ($TMP_DIR/fix-prN-result.json) + 正規 key で state にマージされる。""" + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + fix = _canonical_fix() + (tmp_dir / f"fix-pr{PR}-result.json").write_text(json.dumps(fix)) + + state_mod.cmd_merge_fix(_make_args()) + + st = _read_state(tmp_dir) + merged = st["rounds"][-1]["fix"] + assert merged["commit"] == "abc1234" + assert merged["fixed"] == 5 + assert merged["ci"] == "SUCCESS" + assert merged["by_severity"]["major"] == 3 + + +def test_tmp_fallback_path(patched_tmp_dir, state_mod, tmp_path): + """正規パス不在で /tmp/fix-prN-result.json にある場合は fallback で拾う。 + + 旧プロンプトでサブエージェントが `/tmp/` を指定したケースの救済。 + """ + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + # 正規パスには書かず、/tmp/ のみに置く + fix = _canonical_fix() + fix["fix_commit"] = "tmp_path_commit" + legacy = pathlib.Path(f"/tmp/fix-pr{PR}-result.json") + legacy.write_text(json.dumps(fix)) + try: + state_mod.cmd_merge_fix(_make_args()) + st = _read_state(tmp_dir) + assert st["rounds"][-1]["fix"]["commit"] == "tmp_path_commit" + finally: + legacy.unlink(missing_ok=True) + + +def test_key_alias_commit_sha_and_fixed(patched_tmp_dir, state_mod): + """サブエージェントが別名 (`commit_sha` / `fixed`) で書いても受理する。""" + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + fix = { + "pr": PR, + "commit_sha": "alias_sha", + "fixed": 3, + "ci_status": "SUCCESS", + "deferred": [], + "rejected": [], + "resolved_threads": [], + } + (tmp_dir / f"fix-pr{PR}-result.json").write_text(json.dumps(fix)) + + state_mod.cmd_merge_fix(_make_args()) + + st = _read_state(tmp_dir) + merged = st["rounds"][-1]["fix"] + assert merged["commit"] == "alias_sha" + assert merged["fixed"] == 3 + + +def test_missing_all_candidates_dies_with_paths(patched_tmp_dir, state_mod, capsys): + """どの候補にもファイルが無い場合は exit 3 + 探索 path 一覧が stderr に出る。""" + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + # /tmp/ にも置かない (clean up to be safe) + legacy = pathlib.Path(f"/tmp/fix-pr{PR}-result.json") + legacy.unlink(missing_ok=True) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args()) + assert e.value.code == 3 + + captured = capsys.readouterr() + # 探索 path 一覧 (どこを見たか) がメッセージに含まれる + assert str(tmp_dir / f"fix-pr{PR}-result.json") in captured.err + assert f"/tmp/fix-pr{PR}-result.json" in captured.err + + +def test_explicit_file_arg_wins(patched_tmp_dir, state_mod, tmp_path): + """`--file` 明示時はそれを最優先で読む (正規パスや /tmp/ より優先)。""" + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + # 正規パスにも置くが、--file で別ファイルを指定する + canonical = _canonical_fix() + canonical["fix_commit"] = "canonical_should_not_be_used" + (tmp_dir / f"fix-pr{PR}-result.json").write_text(json.dumps(canonical)) + + explicit_fix = _canonical_fix() + explicit_fix["fix_commit"] = "explicit_wins" + explicit_path = tmp_path / "custom-fix.json" + explicit_path.write_text(json.dumps(explicit_fix)) + + state_mod.cmd_merge_fix(_make_args(explicit_path)) + + st = _read_state(tmp_dir) + assert st["rounds"][-1]["fix"]["commit"] == "explicit_wins" + + +# ---------------- PLAN21 round 2: fallback stale 検証 ---------------- + + +def _seed_state_with_started_at(tmp_dir: pathlib.Path, started_at: str) -> None: + """`started_at` を任意のタイムスタンプで seed する。""" + state = { + "current_pr": PR, + "rounds": [{"round": 1, "pr": PR, "started_at": started_at}], + "deferred_nits": [], + "final": None, + } + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) + + +def test_fallback_stale_mtime_is_ignored(patched_tmp_dir, state_mod, capsys): + """正規 fallback ファイルの mtime が round 開始前なら skip。 + + 別 round / 別実行の残骸ファイルを誤マージしないことを確認する。 + """ + tmp_dir = patched_tmp_dir + # round 開始時刻 = 現在 (これより古いファイルは skip 対象) + now_dt = _dt.datetime.now(_dt.timezone.utc).astimezone() + _seed_state_with_started_at(tmp_dir, now_dt.isoformat(timespec="seconds")) + + stale = tmp_dir / f"fix-pr{PR}-result.json" + stale.write_text(json.dumps(_canonical_fix())) + # mtime を 1 時間前に巻き戻す (round 開始より明確に古い) + stale_ts = now_dt.timestamp() - 3600 + os.utime(stale, (stale_ts, stale_ts)) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args()) + assert e.value.code == 3 + + captured = capsys.readouterr() + # 古いファイルとして skip された旨が stderr に出る + assert "round 開始前" in captured.err or "古いファイル" in captured.err + + +def test_fallback_tmp_stale_is_ignored(patched_tmp_dir, state_mod, capsys): + """`/tmp/fix-prN-result.json` の古い残骸 (別リポジトリの同番号 PR 想定) は skip。 + + codex review 指摘の本丸: PR 番号だけで命名された共有 namespace の + 古いファイルを無条件に拾わないこと。 + """ + tmp_dir = patched_tmp_dir + now_dt = _dt.datetime.now(_dt.timezone.utc).astimezone() + _seed_state_with_started_at(tmp_dir, now_dt.isoformat(timespec="seconds")) + + legacy = pathlib.Path(f"/tmp/fix-pr{PR}-result.json") + legacy.write_text(json.dumps(_canonical_fix())) + # mtime を 2 時間前に巻き戻す (= 過去の別実行で残った想定) + stale_ts = now_dt.timestamp() - 7200 + os.utime(legacy, (stale_ts, stale_ts)) + try: + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args()) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "round 開始前" in captured.err or "古いファイル" in captured.err + finally: + legacy.unlink(missing_ok=True) + + +def test_fallback_pr_mismatch_is_ignored(patched_tmp_dir, state_mod, capsys): + """fallback ファイル内の `pr` が対象 PR と一致しない場合は skip。 + + 別リポジトリの同番号 PR の戻り値が `/tmp` 共有 namespace に + 残っていたケースを想定する。 + """ + tmp_dir = patched_tmp_dir + # round 開始は十分過去にして mtime チェックでは弾かれないようにする + past = _dt.datetime(2000, 1, 1, tzinfo=_dt.timezone.utc) + _seed_state_with_started_at(tmp_dir, past.isoformat(timespec="seconds")) + + fix = _canonical_fix() + fix["pr"] = PR + 1 # 別 PR の戻り値 + (tmp_dir / f"fix-pr{PR}-result.json").write_text(json.dumps(fix)) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args()) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "pr 不一致" in captured.err or "別 PR" in captured.err + + +def test_fallback_fresh_mtime_and_matching_pr_is_accepted(patched_tmp_dir, state_mod): + """mtime が round 開始後 & `pr` 一致なら採用される (regression guard)。""" + tmp_dir = patched_tmp_dir + # round 開始を 1 時間前に置く → 直後に書いたファイルは "fresh" + past_dt = _dt.datetime.now(_dt.timezone.utc).astimezone() - _dt.timedelta(hours=1) + _seed_state_with_started_at(tmp_dir, past_dt.isoformat(timespec="seconds")) + + fix = _canonical_fix() + fix["fix_commit"] = "fresh_ok" + (tmp_dir / f"fix-pr{PR}-result.json").write_text(json.dumps(fix)) + + state_mod.cmd_merge_fix(_make_args()) + + st = _read_state(tmp_dir) + assert st["rounds"][-1]["fix"]["commit"] == "fresh_ok" + + +def test_explicit_file_bypasses_stale_check(patched_tmp_dir, state_mod, tmp_path): + """`--file` 明示時は stale 検証をスキップする (ユーザー指定優先)。""" + tmp_dir = patched_tmp_dir + now_dt = _dt.datetime.now(_dt.timezone.utc).astimezone() + _seed_state_with_started_at(tmp_dir, now_dt.isoformat(timespec="seconds")) + + explicit_fix = _canonical_fix() + explicit_fix["fix_commit"] = "explicit_stale_ok" + explicit_path = tmp_path / "explicit.json" + explicit_path.write_text(json.dumps(explicit_fix)) + # mtime を round 開始前に巻き戻しても、--file 指定なら採用される + stale_ts = now_dt.timestamp() - 3600 + os.utime(explicit_path, (stale_ts, stale_ts)) + + state_mod.cmd_merge_fix(_make_args(explicit_path)) + + st = _read_state(tmp_dir) + assert st["rounds"][-1]["fix"]["commit"] == "explicit_stale_ok" + + +# ---------------- PLAN21 round 3: 正規パス parse 失敗 / pr 型不正 / 戻り値タプル化 ---------------- + + +def test_canonical_path_json_parse_failure_dies_immediately( + patched_tmp_dir, state_mod, capsys +): + """正規パス ($TMP_DIR/fix-prN-result.json) の JSON parse 失敗は即時 die(code=3)。 + + codex round 2 指摘: 壊れた正規パスをスキップして /tmp/ fallback に流れると + 別 PR の戻り値を誤マージする事故が起こるため、正規パスの parse 失敗は致命扱い。 + """ + tmp_dir = patched_tmp_dir + # round 開始を十分過去にして mtime チェックでは弾かれないようにする + past = _dt.datetime(2000, 1, 1, tzinfo=_dt.timezone.utc) + _seed_state_with_started_at(tmp_dir, past.isoformat(timespec="seconds")) + + # 正規パスに壊れた JSON を置く (size>0 だが parse 不能) + canonical = tmp_dir / f"fix-pr{PR}-result.json" + canonical.write_text("{ this is not valid json") + + # /tmp/ 側には別 PR を装う偽の正規 JSON を置く (これに流れ込んだら事故) + legacy = pathlib.Path(f"/tmp/fix-pr{PR}-result.json") + legacy.write_text(json.dumps(_canonical_fix())) + try: + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args()) + assert e.value.code == 3 + captured = capsys.readouterr() + # 正規パスの読み取り失敗である旨が stderr に出る + assert "正規パス" in captured.err + # /tmp 側の fix がマージされていないこと (= state.rounds[-1].fix が無い) + st = _read_state(tmp_dir) + assert "fix" not in st["rounds"][-1] + finally: + legacy.unlink(missing_ok=True) + + +def test_fallback_pr_field_non_numeric_is_skipped(patched_tmp_dir, state_mod, capsys): + """fallback ファイルの `pr` フィールドが int 変換不能なら skip (ValueError 防止)。 + + gemini round 2 指摘: `int(file_pr)` で ValueError が裸で上がるとプロセスごと落ちる。 + """ + tmp_dir = patched_tmp_dir + past = _dt.datetime(2000, 1, 1, tzinfo=_dt.timezone.utc) + _seed_state_with_started_at(tmp_dir, past.isoformat(timespec="seconds")) + + # 正規パスに `pr` が int 化できない値の JSON を置く + canonical = tmp_dir / f"fix-pr{PR}-result.json" + bad = _canonical_fix() + bad["pr"] = "not-a-number" + canonical.write_text(json.dumps(bad)) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args()) + # /tmp/ 側にも候補が無いので最終的には "戻り値ファイル無し" で die(3) + assert e.value.code == 3 + captured = capsys.readouterr() + # 数値として解釈できない旨が stderr に出る + assert "数値として解釈できない" in captured.err + # state にマージされていないこと + st = _read_state(tmp_dir) + assert "fix" not in st["rounds"][-1] + + +def test_is_fresh_fix_result_returns_tuple_with_parsed_payload( + patched_tmp_dir, state_mod +): + """`_is_fresh_fix_result` は (is_fresh, parsed_payload) を返す。 + + gemini round 2 指摘の性能改善: cmd_merge_fix 側で再パースしないよう、 + fresh な場合は parse 済み dict を返す。 + """ + tmp_dir = patched_tmp_dir + past_dt = _dt.datetime.now(_dt.timezone.utc).astimezone() - _dt.timedelta(hours=1) + past_ts = past_dt.timestamp() + + fix = _canonical_fix() + fix["fix_commit"] = "tuple_return_ok" + p = tmp_dir / f"fix-pr{PR}-result.json" + p.write_text(json.dumps(fix)) + + is_fresh, parsed = state_mod._is_fresh_fix_result(p, PR, past_ts, is_canonical=True) + assert is_fresh is True + assert isinstance(parsed, dict) + assert parsed["fix_commit"] == "tuple_return_ok" + + +def test_is_fresh_fix_result_returns_none_when_stale(patched_tmp_dir, state_mod): + """stale な場合は (False, None) を返す。""" + tmp_dir = patched_tmp_dir + now_dt = _dt.datetime.now(_dt.timezone.utc).astimezone() + + p = tmp_dir / f"fix-pr{PR}-result.json" + p.write_text(json.dumps(_canonical_fix())) + stale_ts = now_dt.timestamp() - 3600 + os.utime(p, (stale_ts, stale_ts)) + + is_fresh, parsed = state_mod._is_fresh_fix_result( + p, PR, now_dt.timestamp(), is_canonical=False + ) + assert is_fresh is False + assert parsed is None + + +# ---------------- PLAN21 round 4: non-dict JSON 防御 ---------------- + + +def test_is_fresh_fix_result_non_dict_json_is_skipped(patched_tmp_dir, state_mod, capsys): + """`_is_fresh_fix_result` は dict 以外 (list 等) の JSON を読んだら (False, None) を返す。 + + gemini round 3 指摘: `json.loads` は list / int / str も返しうるため、 + `payload.get(...)` 呼び出し前に `isinstance(payload, dict)` で防御する必要がある。 + """ + tmp_dir = patched_tmp_dir + # round 開始は十分過去にして mtime チェックを通す + past_dt = _dt.datetime.now(_dt.timezone.utc).astimezone() - _dt.timedelta(hours=1) + past_ts = past_dt.timestamp() + + p = tmp_dir / f"fix-pr{PR}-result.json" + # JSON として valid だが dict ではない (list) + p.write_text(json.dumps([{"fix_commit": "should_be_ignored"}])) + + is_fresh, parsed = state_mod._is_fresh_fix_result( + p, PR, past_ts, is_canonical=False + ) + assert is_fresh is False + assert parsed is None + captured = capsys.readouterr() + # dict ではない旨が stderr に出る + assert "dict ではない" in captured.err + + +def test_is_fresh_fix_result_non_dict_json_canonical_dies_round5( + patched_tmp_dir, state_mod, capsys +): + """PLAN21 round 5 で挙動変更: 正規パス (is_canonical=True) で非 dict なら die(code=3)。 + + 旧 round 4 では skip (False, None) だったが、codex round 4 指摘により + `/tmp/` fallback への流れ込みを防ぐため parse 失敗と同様に即時中断する。 + """ + tmp_dir = patched_tmp_dir + past_dt = _dt.datetime.now(_dt.timezone.utc).astimezone() - _dt.timedelta(hours=1) + past_ts = past_dt.timestamp() + + p = tmp_dir / f"fix-pr{PR}-result.json" + p.write_text(json.dumps(["not", "a", "dict"])) + + with pytest.raises(SystemExit) as e: + state_mod._is_fresh_fix_result(p, PR, past_ts, is_canonical=True) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "dict ではない" in captured.err + assert "正規パス" in captured.err + + +def test_explicit_file_non_dict_json_dies(patched_tmp_dir, state_mod, capsys, tmp_path): + """`--file` 明示時に non-dict JSON が渡されたら die(code=3) で即時中断。 + + gemini round 3 指摘: list 等が渡されると後続 `fix.get(...)` でクラッシュするため、 + 明示指定の場合も dict 検証を行ってから fix に代入する。 + """ + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + + explicit_path = tmp_path / "bad-explicit.json" + # JSON として valid だが dict ではない (list) + explicit_path.write_text(json.dumps([_canonical_fix()])) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args(explicit_path)) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "dict ではない" in captured.err + # state は更新されていない + st = _read_state(tmp_dir) + assert "fix" not in st["rounds"][-1] + + +# ---------------- PLAN21 round 5: 正規パス non-dict 即時 die / --file 厳格化 ---------------- + + +def test_canonical_path_non_dict_json_dies_immediately( + patched_tmp_dir, state_mod, capsys +): + """正規パス ($TMP_DIR/fix-prN-result.json) の JSON が dict 以外なら即時 die(code=3)。 + + codex round 4 指摘: parse 失敗と同様、非 dict も「壊れた正規出力」として扱う。 + skip で /tmp/ fallback に流れると別実行の戻り値を誤マージする経路が残るため、 + list / str / int 等が返ったら fallback には進まず即時中断する。 + """ + tmp_dir = patched_tmp_dir + past = _dt.datetime(2000, 1, 1, tzinfo=_dt.timezone.utc) + _seed_state_with_started_at(tmp_dir, past.isoformat(timespec="seconds")) + + # 正規パスに list (非 dict) を置く + canonical = tmp_dir / f"fix-pr{PR}-result.json" + canonical.write_text(json.dumps([{"fix_commit": "should_not_be_read"}])) + + # /tmp/ 側には別 PR を装う正規 JSON を置く (これに流れ込んだら事故) + legacy = pathlib.Path(f"/tmp/fix-pr{PR}-result.json") + legacy.write_text(json.dumps(_canonical_fix())) + try: + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args()) + assert e.value.code == 3 + captured = capsys.readouterr() + # 正規パスで非 dict であった旨が stderr に出る + assert "正規パス" in captured.err + assert "dict ではない" in captured.err + # /tmp 側の fix がマージされていないこと + st = _read_state(tmp_dir) + assert "fix" not in st["rounds"][-1] + finally: + legacy.unlink(missing_ok=True) + + +def test_explicit_file_nonexistent_dies(patched_tmp_dir, state_mod, capsys, tmp_path): + """`--file` 指定パスが存在しなければ fallback に進まず die(code=3)。 + + codex round 4 指摘: ユーザー明示指定なのに無言で fallback に流れて + 別実行の戻り値を誤マージするのを防ぐ。 + """ + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + # 正規パスにも置いて、fallback に流れたら事故と判別できるようにする + (tmp_dir / f"fix-pr{PR}-result.json").write_text(json.dumps(_canonical_fix())) + + nonexistent = tmp_path / "does-not-exist.json" + assert not nonexistent.exists() + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args(nonexistent)) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "存在しません" in captured.err + # fallback が読まれていない (state にマージされていない) + st = _read_state(tmp_dir) + assert "fix" not in st["rounds"][-1] + + +def test_explicit_file_empty_dies(patched_tmp_dir, state_mod, capsys, tmp_path): + """`--file` 指定パスが空ファイルなら fallback に進まず die(code=3)。""" + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + (tmp_dir / f"fix-pr{PR}-result.json").write_text(json.dumps(_canonical_fix())) + + empty = tmp_path / "empty.json" + empty.write_text("") + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args(empty)) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "空です" in captured.err + st = _read_state(tmp_dir) + assert "fix" not in st["rounds"][-1] + + +def test_explicit_file_invalid_json_dies(patched_tmp_dir, state_mod, capsys, tmp_path): + """`--file` 指定パスが JSON 不正なら fallback に進まず die(code=3)。""" + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + (tmp_dir / f"fix-pr{PR}-result.json").write_text(json.dumps(_canonical_fix())) + + bad = tmp_path / "bad.json" + bad.write_text("{ this is not valid json") + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(_make_args(bad)) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "parse に失敗" in captured.err or "読み取り" in captured.err + st = _read_state(tmp_dir) + assert "fix" not in st["rounds"][-1] diff --git a/plugins/ndf/skills/cross-review/tests/test_state_read_result.py b/plugins/ndf/skills/cross-review/tests/test_state_read_result.py index 4cea082..a243fc2 100644 --- a/plugins/ndf/skills/cross-review/tests/test_state_read_result.py +++ b/plugins/ndf/skills/cross-review/tests/test_state_read_result.py @@ -120,3 +120,41 @@ def test_empty_result_file_dies(patched_tmp_dir, state_mod): with pytest.raises(SystemExit) as e: state_mod.cmd_read_result(_make_args(rfile)) assert e.value.code == 1 + + +# ---------------- PLAN21 round 5: non-dict / 不正 JSON 防御 ---------------- + + +def test_non_dict_result_json_dies(patched_tmp_dir, state_mod, capsys): + """result.json が list 等の非 dict なら die(code=3)。 + + gemini round 4 指摘: `r.get(...)` で AttributeError になる前に明示的に止める。 + """ + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + rfile = tmp_dir / "result.json" + # JSON valid だが dict ではない + rfile.write_text(json.dumps([{"event": "APPROVE"}])) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_read_result(_make_args(rfile)) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "dict ではない" in captured.err + # state は更新されていない + st = _read_state(tmp_dir) + assert AGENT not in st["rounds"][-1] + + +def test_invalid_json_result_file_dies(patched_tmp_dir, state_mod, capsys): + """JSON parse 不能なら die(code=3)。""" + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + rfile = tmp_dir / "result.json" + rfile.write_text("{ this is not valid json") + + with pytest.raises(SystemExit) as e: + state_mod.cmd_read_result(_make_args(rfile)) + assert e.value.code == 3 + captured = capsys.readouterr() + assert "parse" in captured.err.lower() or "parse" in captured.err