diff --git a/.takt/facets/instructions/fix.md b/.takt/facets/instructions/fix.md index 037f78c2..4b5f18be 100644 --- a/.takt/facets/instructions/fix.md +++ b/.takt/facets/instructions/fix.md @@ -25,7 +25,7 @@ If you catch yourself about to run a Bash command that writes into a read-only z - Use the latest review reports in the Report Directory as primary evidence. - Past iteration reports are saved as `{filename}.{timestamp}` in the same directory. For each report, run Glob with a `{report-name}.*` pattern, read up to 2 files in descending timestamp order, and understand persists / reopened trends before starting fixes. -### Optional: refutation-report.md filter (pre-push-review-refute only) +### Optional: refutation-report.md filter (applies whenever refutation-report.md is present) If the Report Directory contains `refutation-report.md`, a verify (refute) step has already adversarially filtered the reviewer findings. In that case: diff --git a/.takt/facets/instructions/supervise.md b/.takt/facets/instructions/supervise.md index 93e5d5f3..ee215d1d 100644 --- a/.takt/facets/instructions/supervise.md +++ b/.takt/facets/instructions/supervise.md @@ -7,7 +7,7 @@ You are the supervisor. The review-fix cycle has either completed or been escala 3. Check that fixes did not introduce new issues 4. Verify read-only zone compliance (no writes to .takt/, docs/adr/, templates/, .claude/hooks-config.toml) -## Optional: refutation-report.md (pre-push-review-refute only) +## Optional: refutation-report.md (applies whenever refutation-report.md is present) If the Report Directory contains `refutation-report.md` with Verdict `ALL_REFUTED` and there is no fix report, the verify (refute) step rejected every reviewer finding as a false positive, so there is intentionally no code change. Confirm the rejection reasons are sound (each should cite concrete code-level evidence). If they are, this is **ready to push** -- any wrongly-rejected finding is recaught by the post-pr CodeRabbit layer. If a rejection reason looks unsound, treat it as **issues detected**. diff --git a/CLAUDE.md b/CLAUDE.md index 387d289a..112278b0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -50,6 +50,7 @@ - [ADR-047: pre-push review の反証(refute)facet](docs/adr/adr-047-prepush-refute-facet.md) *(試験運用)* - [ADR-048: reviewers→fix findings handoff の output-contract 標準化(markdown 統一・JSON 却下)](docs/adr/adr-048-facet-findings-handoff-markdown-contract.md) *(試験運用)* - [ADR-049: incident→eval 回帰スイート(カスタムルールの由来 incident 再現テスト)](docs/adr/adr-049-incident-eval-regression-suite.md) *(試験運用)* +- [ADR-050: multi-iteration workflow の decision criteria scope 明示](docs/adr/adr-050-iteration-aware-decision-criteria.md) *(試験運用)* ## 開発 convention / チェックリスト diff --git a/docs/adr/adr-043-security-gates-fail-closed.md b/docs/adr/adr-043-security-gates-fail-closed.md index 22e85cec..f78bf24f 100644 --- a/docs/adr/adr-043-security-gates-fail-closed.md +++ b/docs/adr/adr-043-security-gates-fail-closed.md @@ -118,6 +118,12 @@ fn is_stale(behind: Option) -> bool { 一般原則として、model/heuristic の評価では **accuracy と安全軸 (安全側デフォルトを破らないこと) を独立指標として測り、両者が trade-off するときは安全軸を優先**する。accuracy 改善が安全後退を隠しうる (WP-04 の qwen3-coder) 点に注意する。gate 関数における「判定不能 → block」と、助言層における「不確実 → 保守側 (human_review)」は同一の設計思想の別レイヤーへの適用である。 +### 原則 6: read-only zone への reviewer 提案は silently skip せず escalate (2026-07-07 追記、WP-06/07 feedback) + +レビュアー (CodeRabbit 等) の提案が read-only zone (`.takt/facets/**` / `docs/adr/**` / `templates/**` 等、fix step が編集不可の領域) を対象とする場合、fix step はそれを適用できない。このとき **silently skip せず、misdirected として明示 escalate** する (fix report の `### Misdirected finding` 記載 + analyze step での `not_applicable` 明示分類)。silently drop は valid finding の取りこぼし = 「自動対応不能 → 楽観的にスルー」であり、原則 5 の逆で fail-open 的リスクだからである。read-only zone の対応は human (project owner) が直接編集するか、analyze が理由付きで `not_applicable` に分類して監査可能な形で残す。 + +**観測事例**: PR #250 (supervise.md への CodeRabbit Major) / PR #252 (simplicity-review.md への CodeRabbit Major) の 2 件で、`.takt/facets/**` への提案が fix step で read-only zone として misdirected 分類された。両件とも post-pr-review pipeline が misdirected → not_applicable → approved と正しく escalate/分類し silently drop しなかった。この既存の望ましい挙動を原則として明文化する。 + ## 反例の判別ヒント 関数が gate 関数か non-gate 関数かは、以下の質問で判別する: diff --git a/docs/adr/adr-048-facet-findings-handoff-markdown-contract.md b/docs/adr/adr-048-facet-findings-handoff-markdown-contract.md index 8a3f7be2..5be196ae 100644 --- a/docs/adr/adr-048-facet-findings-handoff-markdown-contract.md +++ b/docs/adr/adr-048-facet-findings-handoff-markdown-contract.md @@ -76,6 +76,13 @@ takt に同梱の公式スタイルガイド・スキーマ・builtins を精査 4. 契約は公式スタイルガイド準拠 (` ```markdown ` ブロック + 認知負荷軽減ルール + `finding_id` 必須の Rejection Gate)。 +### output-contract 設計原則 (次回 contract 追加・編集時の参照基準、2026-07-07 追記 / WP-07 feedback) + +新規 output-contract を追加・編集する際は以下の 2 原則を守る (WP-07 で列不整合が CodeRabbit Major 指摘となった再発防止): + +1. **全 finding セクションで同一列セット**: 1 contract 内の Current Iteration / Carry-over / Reopened 等の finding テーブルは同じ finding 列 (`finding_id` / `family_tag` / ...) を共有する。セクション間で列が漂流すると下流の fix / refute facet の parse を壊す。 +2. **builtin `security-review` を mirror**: reviewer contract は takt builtin `security-review` の列構造・casing を踏襲する。casing の混在 (snake_case の field id `finding_id`/`family_tag` + Title Case の表示ラベル `Severity`/`Type`/...) は builtin 由来の**意図的な区別**であり「不整合」ではない (PR #252 で CodeRabbit が誤指摘)。意図的に mirror しない contract を作る場合は、その設計意図を contract 冒頭コメントに明記する。 + 適用範囲は **pre-push review の reviewers** (通常 `pre-push-review.yaml` と refute variant `pre-push-review-refute.yaml` の両方が同じ `format:` 名を共有するため自動的に裨益)。post-pr-review は simplicity/security reviewer を持たず (CodeRabbit findings 駆動の analyze/fix 構成) 対象外。 diff --git a/docs/adr/adr-050-iteration-aware-decision-criteria.md b/docs/adr/adr-050-iteration-aware-decision-criteria.md new file mode 100644 index 00000000..989a360a --- /dev/null +++ b/docs/adr/adr-050-iteration-aware-decision-criteria.md @@ -0,0 +1,59 @@ +# ADR-050: multi-iteration workflow の decision criteria scope 明示 + +## ステータス + +試験運用 (2026-07-07) + +## コンテキスト + +takt の review-fix workflow は `Iteration N/max` の反復構造を持ち、各 step (analyze / fix / supervise 等) が +Report Directory に**複数 iteration 分の report** を蓄積する (最新は `{filename}`、過去は `{filename}.{timestamp}`)。 + +PR #250 (WP-06) で `supervise.md` の Decision Criteria が「all blocking findings が解決したら push」とだけ書かれ、 +その「all blocking findings」が **全 iteration の累積か、現 iteration のみか** が曖昧だった。この scope 曖昧性は +「前 iteration で解決済みの finding を再判定して不要な reject」または「現 iteration の未解決を見落として過剰 approve」 +という判定ミスに直結する。CodeRabbit Major 指摘となり、supervise.md に scope 宣言を追記して解消した。 + +この曖昧性は supervise 固有ではなく、**Report Directory を読む全ての multi-iteration step に共通の systemic pattern** +であり、今後の複数 iteration workflow 実装で再発しうる。 + +## 決定 + +multi-iteration workflow の各判定 step の decision criteria は、**評価 scope を明示**する。 + +### scope の 3 分類 + +| scope | 意味 | 典型例 | +|---|---|---| +| `current-iteration-only` | 現 iteration の report のみを判定対象とする (既定) | fix / supervise の per-iteration 判定 | +| `cumulative` | 全 iteration の累積状態を判定対象とする | convergence 全体の最終判定 | +| `sliding-window` | 直近 N iteration を対象とする | 膠着 (stall) 検出 | + +### 原則 + +1. **既定は `current-iteration-only`** — 特に明示しない限り、判定は現 iteration の report (`{filename}` suffix なし、 + 最新) のみを対象とする。過去 iteration の archived report (`{filename}.{timestamp}`) は参照しない + ([dev-conventions.md](../dev-conventions.md) の Report Directory アクセスパターンと整合)。 +2. **scope 宣言を instruction に明記** — decision criteria セクション冒頭で、どの scope で評価するかを 1-2 行の + negative/positive specification で宣言する (例: supervise.md「本セクションは current iteration の report のみを + 判定対象とし、前 iteration の convergence_verdict とは比較しない」)。 +3. **cumulative / sliding-window を使う場合は理由を明記** — 既定から外れる scope を採る step は、なぜ全履歴 / 窓が + 必要かを instruction に書く (膠着検出は反復の history が本質的に必要、等)。 + +## 帰結 + +### 利点 + +- multi-iteration step の判定 scope が暗黙にならず、scope 曖昧性由来の判定ミスを設計段階で排除。 +- 新規 iteration workflow / facet 追加時の設計チェック項目として再利用可能。 + +### 留意点 + +- takt は scope を機械強制しない (instruction prompt レベルの規約)。宣言忘れの検出は reviewer / dogfood に依存する。 + +### 関連 ADR + +- [ADR-020](adr-020-takt-facets-sharing.md) — facet 共有 (fix/supervise は複数 workflow で共有、scope 宣言も共有される) +- [ADR-030](adr-030-deterministic-post-merge-feedback.md) — 決定論的 post-merge feedback (iteration 構造の文脈) +- [ADR-037](adr-037-takt-fix-trust-shortcut.md) — fix-trust shortcut (convergence_verdict による iteration 短絡) +- [ADR-047](adr-047-prepush-refute-facet.md) — refute facet (loop_monitor の cycle = sliding-window scope の例) diff --git a/docs/dev-conventions.md b/docs/dev-conventions.md index 49d0eaea..41916d33 100644 --- a/docs/dev-conventions.md +++ b/docs/dev-conventions.md @@ -28,3 +28,30 @@ spike・実験タスクを見送る (採用しない) と判断したときは 5. **public リポ特典の適用範囲** — public リポで無償化される「機能」と、緩和されない「rate limit」を区別する。 **由来** (WP-03、[ADR-019](adr/adr-019-coderabbit-review-hybrid-policy.md) § CodeRabbit クォータ設計): CodeRabbit の「public リポ向け Pro 機能無償提供」を「rate limit 撤廃」と誤解しかけたが、実際には月間上限と時間単位 rate limit は別次元で、時間単位上限 (3〜4 回 / 時) は残存していた (2026-07-04 ユーザー確認)。この誤解は LLM API・CI 等の他 SaaS 統合でも再発しうる汎用パターン。 + +## facet の Report Directory アクセスパターン (WP-06 feedback) + +takt facet が Report Directory から report を読む際は、**現 iteration の report のみを対象**とする: + +1. **archived timestamped ファイルを除外** — 過去 iteration の report は `{filename}.{timestamp}` として同ディレクトリに残る。読み取り対象は suffix なしの `{filename}` (最新) に限る。 +2. **既存パターンの踏襲** — `fix.md` は「`{report-name}.*` を Glob し descending timestamp 順で最新 2 件のみ読む」パターンを確立済み。新規 facet が Report Directory を読む場合はこれに揃える。 + +**由来** (PR #250 / WP-06): `supervise.md` が Report Directory のフィルタを持たず全履歴を読みうる曖昧性が CodeRabbit Major 指摘となった。facet が「全履歴」か「current-iteration-only」かを暗黙にせず明示する (scope 曖昧さは判定ミスに直結)。 + +## 見出し ⇔ 実装スコープの整合 (WP-06/07 feedback) + +見出し (section heading / WP heading) は実装の条件スコープと 1:1 対応させ、実装変更時は見出しも追随させる: + +1. **takt instruction / output-contract の section 見出し** — `if` ガードや file-existence check の条件を反映する。「X-variant only」ではなく「Applies when \」形式で実際の適用条件を表す (例: 「pre-push-review-refute only」→「applies whenever refutation-report.md is present」)。 +2. **計画文書の WP 見出し** — `docs/harness-improvement-plan.md` 等の WP status を「実装済」に更新する際は、WP 見出しが実装内容を正確に反映しているか確認する。方針転換した場合は見出しも更新する。 + +**由来** (PR #252 / WP-07): WP 見出し「JSON 化」が markdown 契約標準化への方針転換後も未更新で CodeRabbit 指摘。同 PR で `fix.md` の section 見出しが本文の適用条件より狭い (「refute only」だが実際は file 存在時) ことも simplicity review で観測。 + +## 外部 exe を spawn する integration test の bounded wait (WP-08 feedback) + +integration test で外部バイナリを spawn する場合、**無期限 wait を避け bounded duration の wait を必須**とする: + +1. **timeout 付き wait** — `child.wait_with_output()` / `child.wait()` は子プロセスが hang すると CI を無期限ブロックする。代わりに `lib-subprocess::wait_with_timeout_safe(label, &mut child, 30)` 等の timeout 付き wait を使い、超過時は kill + test 失敗させる。 +2. **出力捕捉との両立** — 出力が必要なら stdout/stderr を `lib-subprocess::drain_pipe_unlimited` で別スレッド drain してから timeout wait する (pipe バッファ充填による deadlock 回避)。 + +**由来** (PR #254 / WP-08、[ADR-049](adr/adr-049-incident-eval-regression-suite.md)): codebase 初の exe-spawn E2E テスト (`incident_eval.rs`) パターンを確立したが timeout 境界が欠落し CodeRabbit nitpick。WP-16 CI smoke test 等で同パターン流用が見込まれるため convention 化する。 diff --git a/src/hooks-post-tool-linter/Cargo.toml b/src/hooks-post-tool-linter/Cargo.toml index 2ba0d0ca..d968998a 100644 --- a/src/hooks-post-tool-linter/Cargo.toml +++ b/src/hooks-post-tool-linter/Cargo.toml @@ -19,5 +19,8 @@ tempfile = "3" # stdout HookOutput/LintViolation JSON. serde_json is also a [dependencies] entry but # integration tests only see [dev-dependencies], so it is re-declared here. serde_json = "1.0" +# lib-subprocess: bounded-wait + pipe drain for the exe-spawn E2E test (dev-conventions.md +# § bounded wait). Re-declared here because integration tests only see [dev-dependencies]. +lib-subprocess = { path = "../lib-subprocess" } # [profile.release] は workspace root (Cargo.toml) に集約 (ADR-026) diff --git a/src/hooks-post-tool-linter/tests/incident_eval.rs b/src/hooks-post-tool-linter/tests/incident_eval.rs index f4ab9998..5f97e7d9 100644 --- a/src/hooks-post-tool-linter/tests/incident_eval.rs +++ b/src/hooks-post-tool-linter/tests/incident_eval.rs @@ -14,10 +14,15 @@ //! data reproducing the real incident each rule was created for (see the fixture //! headers and `[rules.incident]` in `.claude/custom-lint-rules.toml`). +use lib_subprocess::{drain_pipe_unlimited, wait_with_timeout_safe}; use std::io::Write; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; +/// Bounded wait for the spawned linter exe (dev-conventions.md § bounded wait): a hung +/// child is killed and the test fails rather than blocking CI indefinitely. +const LINTER_TIMEOUT_SECS: u64 = 30; + /// One incident-derived rule's E2E expectation. struct Case { /// Expected `LintViolation.type` (rule id upper-cased, hyphens -> underscores). @@ -119,14 +124,22 @@ fn run_linter(cwd: &Path, invoke_path: &str) -> Vec { .stderr(Stdio::piped()) .spawn() .expect("spawn hooks-post-tool-linter"); + let stdout_drain = drain_pipe_unlimited(child.stdout.take().expect("child stdout")); + let stderr_drain = drain_pipe_unlimited(child.stderr.take().expect("child stderr")); child .stdin .take() .expect("child stdin") .write_all(payload.as_bytes()) .expect("write stdin payload"); - let out = child.wait_with_output().expect("wait for linter exe"); - let stdout = String::from_utf8_lossy(&out.stdout); + let status = wait_with_timeout_safe("hooks-post-tool-linter", &mut child, LINTER_TIMEOUT_SECS) + .expect("wait_with_timeout_safe errored"); + let stdout = stdout_drain.join().expect("stdout drain thread panicked"); + let _stderr = stderr_drain.join().expect("stderr drain thread panicked"); + assert!( + status.is_some(), + "hooks-post-tool-linter hung > {LINTER_TIMEOUT_SECS}s on {invoke_path} (killed) — investigate", + ); parse_custom_lint_violations(&stdout) } @@ -222,3 +235,31 @@ fn incident_eval_all_incident_rules() { assert_good_fixture_clean(case); } } + +/// fail-closed: `CASES` must have one entry per incident-derived rule. This closes the +/// asymmetry where `incident_fixture_coverage_check` gates the *fixture* dimension +/// dynamically but the E2E `CASES` array was manually synced — a new `[rules.incident]` +/// rule added without a CASES entry would silently skip E2E coverage (ADR-043 / ADR-049). +#[test] +fn cases_cover_every_incident_rule() { + let toml = std::fs::read_to_string( + repo_root().join(".claude").join("custom-lint-rules.toml"), + ) + .expect("read deployed custom-lint-rules.toml"); + let incident_rule_count = toml + .lines() + .filter(|l| l.trim() == "[rules.incident]") + .count(); + assert!( + incident_rule_count > 0, + "false-green guard: no `[rules.incident]` sections found in deployed toml" + ); + assert_eq!( + CASES.len(), + incident_rule_count, + "CASES ({}) must have one entry per `[rules.incident]` rule ({}) — a new incident \ + rule was added without an E2E case (fail-closed)", + CASES.len(), + incident_rule_count + ); +}