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
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