Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
313 changes: 313 additions & 0 deletions issues/PLAN20_cross-review-worktree-and-result-schema-fix.md

Large diffs are not rendered by default.

245 changes: 245 additions & 0 deletions issues/i17.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
# [cross-review] worktree デフォルトパスと gemini result.json スキーマの不整合

- 対象 plugin: `ndf` v4.7.2
- 対象 skill: `ndf:cross-review`
- 報告者: takemi-ohama
- 検出経緯: `devbasex/devbase#14` に対する `/ndf:cross-review 14` 実行中

`/ndf:cross-review` を macOS ホストで実行したところ、独立した 2 件の不具合に遭遇しました。
どちらも回避手順は確立できたものの、毎回手動介入が必要になっており、
プラグイン側で対応していただきたいです。

---

## 1. worktree デフォルトパスが `/work/worktrees/prN` 固定で macOS で破綻する

### 現象

`scripts/state.py init <PR>` を引数省略で実行すると、worktree 作成が以下で失敗する:

```
❌ command failed (git worktree add --detach /work/worktrees/pr14 origin/feature/...):
fatal: could not create leading directories of '/work/worktrees/pr14/.git':
Read-only file system
```

macOS では `/work` がそもそも存在せず(root 直下は SIP で書き込み不可)、
コンテナ/Linux ホスト前提のパスが固定値として埋め込まれている。

### 該当箇所

`skills/cross-review/scripts/state.py:122`

```python
worktree = args.worktree or f"/work/worktrees/pr{pr}"
```

SKILL.md / docs 側でも `/work/worktrees/pr<PR>` を前提として記述されている:

- `skills/cross-review/SKILL.md`「事前確認」表 #2「worktree 分離」
- `docs/01-state-and-review.md` Step 0 解説

### 影響

- ホスト OS が macOS / 非コンテナ環境の場合、毎回 `--worktree` を明示指定しなければ init できない。
- 初回ユーザは「`/work` がない」エラーメッセージから worktree 引数の存在に気付きにくい。
- skill 内 prompt (`Agent(...)` に渡される prompt) や docs に `/work/worktrees/pr<PR>` が
ハードコードされているため、サブエージェント側も明示置換が必要。

### 回避策(当方で実施)

```bash
mkdir -p $HOME/work/worktrees
state.py init 14 --worktree $HOME/work/worktrees/pr14 ...
```

### 修正提案

優先度の高い順に 2 案:

1. **デフォルト値を環境変数 + OS 判定で導出する**
```python
def _default_worktree_base() -> pathlib.Path:
env = os.environ.get("NDF_WORKTREE_BASE")
if env:
return pathlib.Path(env)
# /work が書き込み可能ならそれを優先(既存 Linux コンテナ環境互換)
work = pathlib.Path("/work/worktrees")
try:
work.mkdir(parents=True, exist_ok=True)
return work
except OSError:
pass
return pathlib.Path.home() / "work" / "worktrees"

worktree = args.worktree or str(_default_worktree_base() / f"pr{pr}")
```
- 既存のコンテナ環境 (`/work` 書込可) は挙動を変えない
- macOS / WSL / その他は `$HOME/work/worktrees/prN` にフォールバック
- `NDF_WORKTREE_BASE` でユーザがオーバーライド可能

2. **SKILL.md / docs / launcher prompt 内のハードコード `/work/worktrees/pr<PR>` を、
state.json の `worktree_path` から読むようテンプレート化**
- サブエージェント prompt 側で `state.json` を参照させる
- もしくはメインから `WORKTREE_PATH` 変数を埋め込むテンプレートにする

---

## 2. gemini の `result.json` スキーマが launcher 間で揺れて `state.py read-result` が intent を欠落させる

### 現象

`launch-gemini.sh` 起動後、gemini が書き出した
`$TMP_DIR/gemini-review-pr<PR>-result.json` を `state.py read-result <pr> gemini`
にかけると、`intent=None / posted_as=None / comments=None` で state に取り込まれる:

```
✅ gemini: intent=None posted_as=None comments=None
```

その結果 `state.py judge` で
`gemini=None` 扱いになり、両者 APPROVE 状態でも収束判定されずに次ラウンドへ進んでしまう
(最悪 max_rounds 到達まで空回りする)。

### 実例

PR #14 round 2 で gemini が書き出した result.json:

```json
{
"repo": "devbasex/devbase",
"pr": 14,
"round": 2,
"reviewer": "gemini",
"intent": "APPROVE",
"comment_count": 3,
"summary": "全体的に堅牢で..."
}
```

一方 codex が書き出した(および `read-result` が期待している)スキーマ:

```json
{
"event": "APPROVE",
"posted_as": "COMMENT",
"comments_count": 0,
"review_url": "https://.../pull/14#pullrequestreview-...",
"by_severity": {"critical": 0, "major": 0, "minor": 0, "nit": 0}
}
```

主要フィールドが全てズレている (`event` vs `intent`、`comments_count` vs `comment_count`、
`review_url` 欠落、`posted_as` 欠落、`by_severity` 欠落)。

### 原因

`launch-gemini.sh:90` が **フィールドリストを列挙していない**:

```
- 投稿後、サマリを **$TMP_DIR/gemini-review-pr$STATE_PR-result.json** に書く
(フォーマットは launch-codex.sh と同じ)
```

一方 `launch-codex.sh:81-88` は具体的なスキーマを明示している:

```
- 投稿後、サマリを **$TMP_DIR/codex-review-pr$STATE_PR-result.json** に書く:
{
"event": "REQUEST_CHANGES",
"posted_as": "COMMENT",
"comments_count": 5,
"review_url": "https://.../pull/$PR#pullrequestreview-...",
"by_severity": {"critical": 0, "major": 3, "minor": 2, "nit": 0}
}
```

gemini は launcher prompt のみを文脈にもつため、「同じフォーマット」だけでは
launch-codex.sh の中身を読みに行けず、自分流のスキーマで書き出してしまう
(特にコンテキスト窓内に launch-codex.sh の本文が無い場合)。

加えて `state.py:244-265` `cmd_read_result` は `event` フィールドしか見ない:

```python
st["rounds"][-1][agent] = {
"intent": r.get("event"),
"posted_as": r.get("posted_as", r.get("event")),
"comments": r.get("comments_count"),
...
}
```

そのため `intent` / `comment_count` で書かれても拾えない。

### 影響

- gemini の APPROVE が認識されず、判定が「片方 None」状態で次ラウンドへ
- 最悪、無駄な round と CI / レビューコメントを増やしながら `max_rounds` 到達で `final=max_rounds` 終了
- ユーザが state.json を手で書き換えるまで気付きにくい(read-result は exit 0 で成功扱い)

### 回避策(当方で実施)

`state.json` の `rounds[-1].gemini` を `jq` で直接パッチして再度 `judge` を回した:

```bash
jq '.rounds[-1].gemini = {
intent: "APPROVE", posted_as: "COMMENT",
comments: 3,
review_url: "https://github.com/...",
by_severity: {}
}' "$STATE_FILE" > "$STATE_FILE.tmp" && mv "$STATE_FILE.tmp" "$STATE_FILE"
```

### 修正提案

両側から手当てするのが堅実です:

1. **`launch-gemini.sh` 側に codex と同じフィールドリストを明示**
```
- 投稿後、サマリを $TMP_DIR/gemini-review-pr$STATE_PR-result.json に **必ず以下のキーで** 書く:
{
"event": "APPROVE" | "REQUEST_CHANGES" | "COMMENT",
"posted_as": "APPROVE" | "REQUEST_CHANGES" | "COMMENT",
"comments_count": <int>,
"review_url": "<URL>",
"by_severity": {"critical": 0, "major": 0, "minor": 0, "nit": 0}
}
`intent` / `comment_count` 等の別名は使わないこと。
```
- 単に「codex と同じ」と書いただけでは LLM がスキーマを再現できない実例なので、
**両 launcher に同一のフィールドブロックをコピペで持たせる**のが安全。
- もしくは共通プロンプト断片を `scripts/_result_schema.txt` 等に切り出して両方が
ヒアドキュメントで埋め込む構成に。

2. **`state.py read-result` 側で別名フィールドにフォールバックする**
```python
intent = r.get("event") or r.get("intent")
posted_as = r.get("posted_as") or r.get("event") or intent
comments = r.get("comments_count")
if comments is None:
comments = r.get("comment_count")
review_url = r.get("review_url")
```
- 後方互換のための保険。スキーマ違反でも壊れずに取り込めるようにする。
- さらに「`event` も `intent` も無ければ exit 1 + 警告」にして
**無言で None を入れる挙動をやめる**のが望ましい
(現状は judge まで進んでから初めて発覚するため発見が遅れる)。

3. **テスト追加**
- `tests/test_state_py.py` 等で「gemini が `intent`/`comment_count` を使う変則 JSON」を
入力にした read-result の挙動を pin する

---

## 補足: 再現環境

| 項目 | 値 |
|---|---|
| OS | macOS 25.5.0 (Darwin) |
| plugin | `ndf` v4.7.2 (`~/.claude/plugins/cache/ai-plugins/ndf/4.7.2`) |
| codex CLI | `/opt/homebrew/bin/codex` |
| gemini CLI | `/opt/homebrew/bin/gemini` |
| 対象 PR | https://github.com/devbasex/devbase/pull/14 |
| TMP_DIR | `/Users/takemi_ohama/.gemini/tmp/pr14` |

参考: 当該 run の state.json / result.json は上記 TMP_DIR に残っており、必要なら共有可能。
2 changes: 1 addition & 1 deletion plugins/ndf/.claude-plugin/plugin.json
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"name": "ndf",
"version": "4.7.2",
"version": "4.7.3",
"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",
Expand Down
53 changes: 53 additions & 0 deletions plugins/ndf/CHANGELOG.md
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,58 @@
# NDF Plugin CHANGELOG

### v4.7.3 (cross-review: macOS 対応 worktree base + result.json スキーマ堅牢化)

`/ndf:cross-review` を非 Linux コンテナ環境 (macOS / WSL 等) でも素直に動かせるよう、
worktree のデフォルトパス解決を環境適応型に変更。あわせて gemini が変則スキーマで
result.json を書き出すケースで intent が silent に None マージされて judge が空回り
する不具合を恒久対応する PATCH リリース。

- **worktree デフォルトパスの環境適応** (`skills/cross-review/scripts/state.py`):
- `state.py init` が以下の優先順で worktree 親ディレクトリを解決:
1. `NDF_WORKTREE_BASE` 環境変数 (明示オーバーライド)
2. `/work/worktrees` (Linux コンテナ環境互換。書込可ならこちらを使用)
3. `$HOME/work/worktrees` (macOS / WSL 等のフォールバック)
- 既存の Linux コンテナ環境では `/work/worktrees` が引き続き使われ挙動不変。
- SKILL.md / docs から `/work/worktrees/pr<PR>` のハードコードを除去し、
`<worktree-base>/pr<PR>` 表記に統一 (state.json サンプル中の解決例 1 箇所のみ残存)。
- **gemini result.json スキーマの明示化** (`skills/cross-review/scripts/launch-gemini.sh`):
- 「フォーマットは launch-codex.sh と同じ」という曖昧指示を、codex と同一の
フィールド列挙ブロック (`event` / `posted_as` / `comments_count` / `review_url` /
`by_severity`) に置き換え。`intent` / `comment_count` 等の別名を使わないことを明記。
- **`state.py read-result` の堅牢化**:
- 仕様 (`event` / `comments_count`) を優先しつつ、別名 (`intent` / `comment_count`)
も拾えるようフォールバックを追加。
- `event` / `intent` いずれも欠落している場合は `die()` (exit 1) で fail する。
旧挙動 (silent な `intent=None` マージで judge が空回り) は **破壊的に修正**。
- **monitor.py EARLY_ERROR 誤検知の修正** (`skills/cross-review/scripts/monitor.py`):
- SKILL.md / `docs/01-state-and-review.md` の Markdown 表セル内で FATAL キーワード
(`「quota exceeded」`「sandbox error」等) を列挙しており、codex がレビュー時に
それを echo すると err.log 上で `_scan_early_fatal()` が誤発火してプロセスを
kill していた。以下 2 段の防御で恒久対応:
1. `EARLY_ERROR_BENIGN` に Markdown 表セル行 (`^\|`) を追加。
2. マッチ位置が backtick / 日本語「」で引用されている場合に benign 扱いする
`_match_is_quoted()` ヘルパを追加し、`_scan_patterns()` から呼ぶ。
- FATAL パターンから `^.*` プレフィックスを外し、`m.start()` をキーワード位置に
合わせて引用判定が機能するように修正。
- **pytest 追加** (`skills/cross-review/tests/`):
- `test_state_read_result.py` — 正規/変則/欠落スキーマ 4 ケース。
- `test_default_worktree_base.py` — env / legacy / fallback の 3 ケース。
- `test_monitor_early_error.py` — Markdown 表 / backtick / 日本語クォート引用の
benign 判定と、本物 fatal が依然検知される回帰テスト 7 ケース。
- ローカル実行: `uv run --with pytest pytest plugins/ndf/skills/cross-review/tests`。
- **関連 issue / plan**:
- `issues/i17.md` (再現報告) / `issues/PLAN20_cross-review-worktree-and-result-schema-fix.md` (実装プラン)。

#### 既存ユーザへの影響

- `/work/worktrees` が書ける環境 (大半の Linux コンテナ環境): **挙動不変**。
- macOS / WSL 等で `/work` が書けない環境: `--worktree` 引数なしでも
`$HOME/work/worktrees/pr<PR>` に自動フォールバックして init が成功する。
- gemini が変則スキーマ (`intent` / `comment_count`) で result.json を書く現象を
観測していたユーザ: フォールバックで自動的に取り込まれるようになる。
- `result.json` から `event` / `intent` が両方欠落しているケースは exit 1 で
早期 fail する (旧: judge 段階まで silent に None が伝播)。

### v4.7.0 (fix / cross-review: 修正ポリシー刷新 + CI 完了待ち廃止)

`/ndf:fix` と `/ndf:cross-review` の修正方針を見直し、PR の最終的なコード品質を
Expand Down
15 changes: 13 additions & 2 deletions plugins/ndf/skills/cross-review/SKILL.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -75,10 +75,21 @@ state.json の読み書きや AI launcher 起動・完了待ちは全て委譲
| # | 対策 | スクリプト側で何をするか |
|---|---|---|
| 1 | 自分の PR 判定(422 回避) | `gh api user` と `gh pr view --json author` を比較し `is_own_pr` / `event_downgrade` を state.json に書く |
| 2 | worktree 分離 | `git worktree add /work/worktrees/pr<PR> <head>` を冪等実行 |
| 2 | worktree 分離 | `git worktree add <worktree-base>/pr<PR> <head>` を冪等実行(`<worktree-base>` は `NDF_WORKTREE_BASE` env > `/work/worktrees` > `$HOME/work/worktrees` の優先順で解決) |
| 3 | gemini trusted directory | `launch-gemini.sh` が `GEMINI_CLI_TRUST_WORKSPACE=true` + `--skip-trust` を必ず併用。さらに **tmp dir は `~/.gemini/tmp/<workspace>/`** を採用し、gemini の workspace 制約 (workspace 外の `read_file` / `write_file` がブロックされる) を回避 |
| 4 | 既存コメント差分 | `gh api .../comments --paginate` を `$TMP_DIR/cross-review-pr<PR>-existing-comments.txt` に保存し、gemini プロンプトには **内容をインライン埋め込み**、codex プロンプトには path を渡す |

### `<worktree-base>` の解決順

`state.py init` は worktree の親ディレクトリを以下の優先順で解決する:

1. `NDF_WORKTREE_BASE` 環境変数(明示オーバーライド)
2. `/work/worktrees`(Linux コンテナ環境互換。書き込み可能ならこちらを使用)
3. `$HOME/work/worktrees`(macOS / WSL 等のフォールバック)

解決した実パスは `state.json` の `worktree_path` に書かれるため、後続スクリプトや
サブエージェント prompt は state.json から読めば追従できる。

### intent / posted_as の両保持(最重要)

GitHub は **自分の PR には `REQUEST_CHANGES` でレビューを投稿できない**
Expand All@@ -98,7 +109,7 @@ GitHub は **自分の PR には `REQUEST_CHANGES` でレビューを投稿で

```mermaid
flowchart TD
Start([事前確認 / loop 開始前に 1 回だけ]):::phase --> Init["worktree 作成 + state.json 初期化<br/>・自分の PR 判定 → event downgrade 設定<br/>・/work/worktrees/pr&lt;PR&gt; を用意<br/>・既存コメントスナップショット保存"]
Start([事前確認 / loop 開始前に 1 回だけ]):::phase --> Init["worktree 作成 + state.json 初期化<br/>・自分の PR 判定 → event downgrade 設定<br/>・&lt;worktree-base&gt;/pr&lt;PR&gt; を用意<br/>・既存コメントスナップショット保存"]
Init --> Round["Round N start<br/>current_pr = PR#"]:::phase

Round -.並列バックグラウンド.-> Codex["/ndf:review &lt;PR&gt; codex<br/>(AI が gh api で直接投稿)<br/>body 先頭: cross-review / round N / codex / intent<br/>→ result.json (intent + posted_as)"]
Expand Down
4 changes: 2 additions & 2 deletions plugins/ndf/skills/cross-review/docs/01-state-and-review.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,7 +94,7 @@ cd "$WORKTREE"

1. 既存 state.json があり `final == null` なら再開
2. 自分の PR 判定(`gh api user` と `gh pr view --json author` を比較)
3. worktree 作成(`/work/worktrees/pr<PR>`)
3. worktree 作成(`<worktree-base>/pr<PR>`。`<worktree-base>` は `NDF_WORKTREE_BASE` env > `/work/worktrees` > `$HOME/work/worktrees` の優先順で解決。実 path は state.json の `worktree_path` を参照
4. 既存コメントスナップショット → `$TMP_DIR/cross-review-pr<PR>-existing-comments.txt`
5. state.json 書き出し

Expand DownExpand Up@@ -163,7 +163,7 @@ fi
launcher が生成するプロンプトに以下を強制している:

- **headRefOid (commit_id) を明示**: AI が自前で取得すると baseRefOid を誤って入れる事故が多発
- **作業 worktree の絶対パス**: 「ファイル読み取りは必ず `/work/worktrees/pr<PR>/` 配下の絶対パスを使う」
- **作業 worktree の絶対パス**: 「ファイル読み取りは必ず worktree 配下の絶対パスを使う」(実 path は state.json の `worktree_path` を参照。`<worktree-base>` は `NDF_WORKTREE_BASE` env > `/work/worktrees` > `$HOME/work/worktrees` の優先順で解決)
- **event ダウングレード警告**: `event_downgrade=true` のときは payload の `event` を `COMMENT` に
- **既存コメント差分**: `$TMP_DIR/cross-review-pr<PR>-existing-comments.txt` を読んで重複指摘禁止
- **review body 先頭 prefix**:
Expand Down
Loading