Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
- [ADR-040: Local LLM Context Size と Resource Trade-off](docs/adr/adr-040-local-llm-context-size.md) *(試験運用)*
- [ADR-041: Test Isolation Patterns for Multi-Condition Guards](docs/adr/adr-041-test-isolation-patterns.md) *(試験運用)*
- [ADR-042: ルール vs 仕組み化の境界基準](docs/adr/adr-042-rule-vs-mechanism-boundary.md) *(試験運用)*
- [ADR-043: Security/Quality Gate での Fail-Closed 原則](docs/adr/adr-043-security-gates-fail-closed.md) *(試験運用)*

## Build

Expand Down
69 changes: 69 additions & 0 deletions docs/adr/adr-021-jj-change-detection-principles.md
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,72 @@ select : BOOKMARK_SEARCH_REVSETS を近い順に走査し、
- **cli-merge-pipeline の post_steps 実装時に流用**: ADR-013 の merge 後 AI ステップで、merge の副作用を検出する際も同パターン
- **lib-jj-helpers の利用徹底**: 新規 jj 連携クレートでは `src/lib-jj-helpers/` を依存に追加し、本 ADR 原則 5 の共通 API (`get_jj_bookmarks` 等) を利用する。`capture_commit_id` / `diff_is_empty` は 2 つ目の使用例出現時に段階的移設予定 (ADR-024 本採用)
- **他の jj コマンド差異の文書化**: `jj bookmark` / `jj new` / `jj describe` も git と意味が違う箇所が多い。必要に応じて追記

## Revset Composability (PR #194 T3-#3 追記、2026-06-04)

PR #194 で `sweep_empty_commits_in_pr_range` の初版が「全 empty commit を Rust 側で取得 → for ループで `description` を check」という generate-then-filter 設計だったが、`description(substring:"fix(review):")` を **revset 側で filter** することで output が最初から絞られる方が efficient + reviewer cost 低 + scope 明確という改善が takt-fix iteration で観測された。本 section は jj revset の composability 原則と設計チェックリストを codify する。

### 原則: revset で「何を取得するか」を最小化

jj の revset は AND / OR / 範囲 / metadata filter を表現できる小さな DSL。**「Rust に返す前に revset で絞れるものは全て絞る」** ことで、以下が同時に実現する:

- **token / IO 効率**: 出力量が最小化され、後続 processing コストが下がる
- **review cost 低**: revset 自体が「何を対象にするか」の宣言になり、Rust 側の filter ロジックを読まなくても scope が読み取れる
- **scope drift 防止**: filter を Rust の for ループに書くと、後の修正で条件追加が漏れる risk があるが、revset で 1 箇所にまとまっていれば変更追跡が容易

### 典型 filter 関数

| revset 関数 | 用途 |
|---|---|
| `empty()` | file change なし (`jj diff --stat` 空相当) |
| `description(substring:"...")` | description 部分一致 (parens / 記号を含む文字列も safe) |
| `description(exact:"...")` | description 完全一致 |
| `description(regex:"...")` | description 正規表現 |
| `(branch..@)` | branch を除いた `@` までの範囲 |
| `author(...)` / `mine()` | author 絞り込み |
| `bookmarks()` | bookmark がついている commit のみ |
| `<expr1> & <expr2>` | AND (両方満たす) |
| <code>&lt;expr1&gt; \| &lt;expr2&gt;</code> | OR (どちらかを満たす) |
| `~<expr>` | NOT |

### 設計チェックリスト (新規 jj 操作コードを書く前に)

- [ ] **取得目的は明示**: revset が何を表現しているかコメント / 関数名で書く (例: "fix(review): empty commits in PR range")
- [ ] **各条件を revset で表現できないか検討**: Rust の for ループに filter を書く前に revset operators で代替できないか確認
- [ ] **`&` / `|` で組合せ可能か**: 複数条件は revset 内で AND/OR 結合する
- [ ] **description マッチは `substring:` 修飾子を必須化**: parens / 記号を含む文字列で default `exact:` が 0 hit する bug を防ぐ (例: `description("fix(review):")` は完全一致になり失敗、`description(substring:"fix(review):")` が正解)
- [ ] **fail-open に注意**: jj log 失敗時の警告ログのみで継続するか、副作用処理を block するかは ADR-043 § 適用範囲 で判断 (sweep 系は fail-open、gate 系は fail-closed)
- [ ] **integration test の不変式は description ベース**: `~/.claude/rules/common/testing.md` § "jj 操作コードの integration test pattern" の `assert_descriptions_absent_in_pr_range` パターンを使う (count NG / description OK)

### Anti-pattern: generate-then-filter

```rust
// BAD: 全 empty を取得して Rust で filter
let all_empty_change_ids = jj_log("empty() & (master..@)");
for cid in all_empty_change_ids {
let desc = jj_log_description(&cid);
if desc.starts_with("fix(review):") {
jj_abandon(&cid);
}
}

// GOOD: revset 側で filter 済
let target_change_ids = jj_log("empty() & description(substring:\"fix(review):\") & (master..@)");
for cid in target_change_ids {
jj_abandon(&cid);
}
```

### 実装事例

PR #194 の `sweep_empty_commits_in_pr_range` (`src/cli-pr-monitor/src/fix_commit.rs:217-218`) で本原則を適用済。

### 試験運用判断基準

本 section は試験運用とする。今後の jj 操作コード PR で本チェックリストが reviewer / Claude の判断 anchor として参照されるかを観測。3 PR 以上で「revset filter で書き直し」の review iteration が発生しなければ stable 昇格、再発があれば原則に不足がないか分析。

### 参照

- `~/.claude/rules/common/testing.md` § "jj 操作コードの integration test pattern": 本 section と相補関係 (revset 設計 + test 設計の対)
- ADR-043 (Security/Quality Gate Fail-Closed 原則): gate 系の fail-closed と sweep 系の fail-open の使い分け
- PR #194 commit (`src/cli-pr-monitor/src/fix_commit.rs:217-218`): 実装事例
20 changes: 20 additions & 0 deletions docs/adr/adr-039-experimental-feature-standard-pattern.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,26 @@ decision trigger は **config (TOML コメント) / code comment (module doc) /

本 checklist は **新規 feature 追加時** の self-review 手順であり、既存 grandfathered case (例: `[session_start.staleness]` の pre-existing な `enabled = true`) の retro-cleanup は scope 外 (別 PR で個別判断)。

### 設計段階 pre-check: config struct 設計時の 6 点 (PR #194 T3-#1 採用、2026-06-04)

PR #194 で `SweepConfig` の初版が 3 点セット (config opt-in / kill-switch / bounded lifetime) のうち kill-switch + bounded lifetime の **設計時考慮** を欠いた状態で実装され、CodeRabbit Major #4 で指摘 → takt-fix で `enabled = false` default + config-driven gate を追加して修正された。前 section の self-review checklist (4 点) は code 完成後の整合確認だが、本 section は **config struct を書く前** に確認する設計段階チェックリスト。両者の関係は「設計時 6 点 (本 section)」→「実装後 4 点 (前 section)」の sequential gate。

新規 experimental feature の config struct を書く前に以下 6 点を確認する:

1. **`enabled: bool` field の存在**: feature 有効化フラグ。`#[serde(default)]` で default = false に明示。型は `Option<bool>` か `bool` のどちらでも可だが、`Option` は「未指定 = OFF」の意図を明示できて self-review 4 点目との整合が取りやすい
2. **`Default` impl の明示**: `Default::default()` で `enabled = false` が確実に出ることを `impl Default` で書く。`#[derive(Default)]` だと bool default が false なので結果は同じだが、`impl Default` の方が後の field 追加時に明示性が保たれる
3. **kill-switch 経路**: 即時停止が必要なとき、(a) config の `enabled = false` toggle で停止できるか、(b) feature を呼び出す上位 module で early-return できるか、(c) 別 process (daemon 等) なら kill signal で停止できるか — のいずれかを ADR / PR body で **明文化**。新規 config field (`kill_switch: bool`) を追加する代わりに既存 `enabled = false` toggle を kill-switch として併用する場合は、その明示が必要
4. **bounded lifetime decision trigger**: 「N PR 後 / YYYY-MM-DD / 条件 X」のいずれかで採否判定タイミングを明文化。形式不明の「いずれ判断する」は不可 (§ 3 § "明示的 decision trigger の必須化" 参照)
5. **3 段 gate の単一箇所集約**: 実行経路で `config.enabled && !is_kill_switched() && !is_expired(&config)` のような 3 段 check を **単一関数** に集約。call site で 3 段をバラバラに書くと条件追加時に漏れる risk あり。SweepConfig の場合は call site が 1 箇所のみのため `if !config.enabled { return; }` で十分だが、複数 call site がある feature は `fn should_run(config: &Self) -> bool` 関数を生やす
6. **off-state integration test の事前計画**: `enabled = false` でのバイパス test を **config struct 実装と同 commit** で書く。後追いで test を書くと、disable path の絶縁が確認されずに 本採用昇格 PR で初めて気づく risk あり

実例 (PR #194 SweepConfig):

- **NG** (初版): `enabled` field 不在 → 常時 run → CodeRabbit Major #4 指摘
- **OK** (takt-fix 後): `pub(crate) enabled: bool` with `#[serde(default)]` + `impl Default { enabled: false }` + `if !config.enabled { return; }` 単一 gate + integration test (本 PR 同梱の `integration_sweep_*` 系で `enabled = false` skip を assert する追加 test は PR #194 T2-#2 で完了)

本 6 点は **設計段階の** 確認手順であり、code 完成後は前 section の 4 点 self-review に進む。両 section が「設計 → 実装」の 2 段 gate を成す。

## 帰結

### 利点
Expand Down
141 changes: 141 additions & 0 deletions docs/adr/adr-043-security-gates-fail-closed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
# ADR-043: Security/Quality Gate での Fail-Closed 原則

## ステータス

試験運用 (2026-06-04)

> 本 ADR は PR #194 で観測した `behind?` (Option<usize>) を使った fail-open bug の根因を一般化し、security/quality gate 関数で Rust の `?` 演算子と早期 return の意味的衝突 (semantic mismatch) を構造的に避けるための設計原則を codify する。`~/.claude/rules/common/security.md` の Mandatory Security Checks の補完層として、判定不能時の挙動を明示する。

## コンテキスト

PR #194 で `src/hooks-pre-tool-validate/src/main.rs` の `check_todo_staleness` 経路に以下のような書き方が含まれていた:

```rust
fn build_todo_staleness_message(
file_path: &str,
behind: Option<usize>,
...,
) -> Option<String> {
let stale = behind? > 0; // ← BAD: None で関数全体が早期 return
...
}
```

CodeRabbit Major #5 が指摘した問題:

- `behind` は `count_commits_branch_ahead(branch)` の戻り値で、jj log 失敗 / branch 未取得 / fetch エラー等で `None` になる
- `Option::?` は `None` のとき関数全体を早期 return する Rust の便利 syntax だが、本関数はその時点で **gate を bypass** することになる
- 直感的には「判定不能なら **念のため block (stale=true 扱い)**」がセキュアな選択 (fail-closed)
- しかし `?` で書くと「判定不能なら **OK 扱いで通過**」になり、本来 stale な docs/todo*.md edit が untracked のまま通ってしまう (fail-open)

takt-fix で以下のイディオムに修正:

```rust
let stale = behind.is_none_or(|n| n > 0); // ← GOOD: None で stale=true (fail-closed)
```

`is_none_or` (Rust 1.82+) は `None` の場合 `true` を返し、`Some(v)` の場合は closure 適用結果を返す。これにより「判定不能 → block」が semantic に揃う。

## 決定

security / quality gate 関数では、以下の **Fail-Closed 原則** を遵守する。

### 原則 1: 判定不能 (None / Err / timeout) はデフォルト blocking

gate 関数の戻り値 (block すべきか / 通過してよいか) を計算する際、入力データが `None` / `Err` / timeout 等で確定不能な場合は、**block 側にデフォルトする**:

| 判定対象 | 入力が確定 | 入力が不確定 |
|---|---|---|
| 「stale か?」 | `Some(n) > 0` で判定 | **stale=true** で扱う |
| 「safe か?」 | 検証 pass / fail で判定 | **safe=false** で扱う |
| 「許可済か?」 | allow-list lookup | **不許可** で扱う |

### 原則 2: Rust idiom — `Option::?` は gate 関数で禁止

`Option<T>::?` は `None` で関数全体を早期 return する。これは gate 関数の semantics と衝突する (None = bypass = fail-open):

```rust
// BAD: fail-open
let stale = behind? > 0; // None → 関数全体 return → gate bypass

// GOOD: fail-closed
let stale = behind.is_none_or(|n| n > 0);

// GOOD: fail-closed (代替)
let stale = behind.map_or(true, |n| n > 0);
```

`is_none_or` は Rust 1.82 で stabilize (`std::option`)。1.82 未満の MSRV では `map_or(true, ...)` を使う。`unwrap_or(0)` 系は「`None` を `0` と扱う」= 「不確定を OK と扱う」ため gate には不適 (PR #194 同型の fail-open)。

### 原則 3: 反例 — gate 関数で禁止される pattern

以下は全て fail-open になるため、gate 関数では使ってはいけない:

```rust
// BAD 1: ? early-return
fn is_stale(behind: Option<usize>) -> Option<bool> {
Some(behind? > 0) // None で None 返却 = caller は gate を skip
}

// BAD 2: unwrap_or(0) で確定値化
fn is_stale(behind: Option<usize>) -> bool {
behind.unwrap_or(0) > 0 // None で 0 扱い = fail-open
}

// BAD 3: if let Some/else { false }
fn is_stale(behind: Option<usize>) -> bool {
if let Some(b) = behind { b > 0 } else { false }
// None で false = fail-open
}
```

正しい代替:

```rust
// GOOD
fn is_stale(behind: Option<usize>) -> bool {
behind.is_none_or(|n| n > 0)
}
```

### 原則 4: 適用範囲

本原則は以下のような gate 関数群に適用される:

- `hooks-pre-tool-validate` の各 staleness / matching / safety check
- `hooks-stop-quality` の test 結果集約
- `cli-push-runner` の各 stage gate (lint / clippy / test 結果判定)
- `cli-pr-monitor` の retry / circuit breaker 判断
- 一般に「block / allow を決める」関数全般

ただし **non-gate な計算関数** (純粋に数値を計算 / 表示用文字列を作る等) は本原則の対象外。`?` は通常通り使ってよい。

## 反例の判別ヒント

関数が gate 関数か non-gate 関数かは、以下の質問で判別する:

1. 戻り値が「block / allow」「stale / fresh」「safe / unsafe」等の二値判断か?
2. 戻り値が `Some(message)` のときに caller が action を取る (block 表示など) か?
3. 戻り値が `None` だと caller は「何もせず通過」するか?

3 が yes なら gate 関数 → 本原則を適用。

## 実装事例

PR #194 commit `dfad56ff` で `build_todo_staleness_message` 内の `let stale = behind.is_none_or(|n| n > 0);` 修正が実装。test は PR #194 T2-#1 (`build_todo_staleness_message_returns_some_when_behind_is_none` / `build_todo_staleness_message_behind_none_with_matches_includes_both_sections`) で fail-closed contract を検証。

## 試験運用判断基準

本 ADR は試験運用とする。3 つ以上の独立 gate 関数で本原則を適用し、同型 fail-open bug が再発しないか観測。

- 観測点: `hooks-pre-tool-validate` / `hooks-stop-quality` / `cli-push-runner` stages の各 gate 関数
- 期間: 2026-06-04 から最低 3 PR の review
- 本採用判断: 3 PR の review で fail-open 指摘が CR / reviewer から再発しなければ stable 昇格、再発があれば本 ADR の不足を分析して原則追加

## 参照

- PR #194 (`feat(hooks): merge 前 mechanical gate 強化 (clippy + 空 commit sweep)`) commit `dfad56ff`: `behind?` → `is_none_or` 修正
- CodeRabbit Major #5 (PR #194 review): 「security gate は判定不能時 fail-closed であるべき」
- ADR-021 (`jj 変更検出ロジックの設計原則`) § Revset Composability: jj 操作の fail-safe 方向との対比
- `~/.claude/rules/common/security.md` § Mandatory Security Checks: 本 ADR が補完する global checklist
- Rust 公式 doc: [`Option::is_none_or`](https://doc.rust-lang.org/std/option/enum.Option.html#method.is_none_or) (1.82+ stable)
Loading