-
Notifications
You must be signed in to change notification settings - Fork 0
feat(takt): Bundle Z Phase 3 — #B-γ reviewer 異常検知 + #C-2 fix-trust 連帯 (PR 3) #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,43 +1,60 @@ | ||
| Focus on reviewing **code simplicity** within the changed diff only. | ||
| Focus on **anomaly detection** in the changed diff -- patterns that look unusual, unexplained, or out of step with the surrounding codebase. Do NOT enumerate against a fixed checklist; the deterministic layer already handles structural metrics. | ||
|
|
||
| ## Obtaining the diff | ||
|
|
||
| The diff has been pre-collected by push-runner (Rust exe) and saved to `.takt/review-diff.txt`. | ||
| **Read this file first** using the Read tool. This is the authoritative review target. | ||
| Do NOT run `git diff` or `jj diff` yourself -- the file already contains the correct diff scope. | ||
|
|
||
| ## Scope constraint | ||
| ## Determinism layer guarantees (do NOT duplicate) | ||
|
|
||
| The following dimensions are enforced by deterministic hooks at write time and by `fix-metrics-check.ps1` during fix iterations. Skip them — flagging them duplicates the deterministic layer and produces noise: | ||
|
|
||
| - **Comment policy** (Bundle Z #B-α / `hooks-post-tool-comment-lint-rust`): Non-doc comments are blocked at PostToolUse. Existing comments in the diff have already passed the allowlist (`// SAFETY:` / `// TODO:` / rustdoc etc.). | ||
| - **Function length** (順位 48, same hook): Functions >50 lines are blocked at write time (touch-trigger ratchet, grandfathered until touched). New >50 functions or growth past 50 cannot land in changed regions. | ||
| - **Function metrics during fix** (Bundle Z #B-β / `fix-metrics-check.ps1`): non-doc comment count, function length, max nesting depth cannot increase per function during fix iterations. Pre/post comparison enforces this structurally. | ||
|
|
||
| Reviewing these dimensions is duplicative. Skip them. | ||
|
|
||
| ## Anomaly criteria (subjective judgment required) | ||
|
|
||
| Review ONLY the lines changed in the diff. Do NOT explore cross-file dependencies, call chains, or project-wide architecture. Every finding must be traceable to a specific hunk in the diff. | ||
| Read the diff straight through. Note any pattern that prompted "this looks unusual / unexpected / hard to explain" — patterns deterministic checks cannot catch: | ||
|
|
||
| ## Review criteria (all diff-local) | ||
| - **Unexplained complexity**: Logic choices with no obvious motivation given the surrounding code; algorithm complexity that seems disproportionate to the problem | ||
| - **Inconsistent style**: Naming or structural patterns that diverge from neighboring code without rationale | ||
| - **Dead-on-arrival code**: Branches, parameters, or abstractions with no apparent caller or use site | ||
| - **Hidden coupling**: Changes that silently depend on global state, environment, ordering, or undocumented invariants | ||
| - **Missing failure paths**: Operations that can fail (I/O, parse, network, optional unwrap) with no visible error handling | ||
| - **Non-obvious magic values**: Numeric or string literals whose meaning isn't clear from context | ||
|
|
||
| - **Nesting depth**: Flag blocks nested >4 levels; suggest flattening via early returns or extraction | ||
| - **Function length**: Flag functions exceeding 50 lines | ||
| - **Early return opportunities**: Identify guard clauses that would reduce nesting | ||
| - **Redundant / duplicate code**: Flag copy-paste patterns or unnecessarily verbose logic within the diff | ||
| - **Magic numbers**: Flag unexplained numeric or string literals; suggest named constants | ||
| - **YAGNI violations**: Flag speculative abstractions, unused parameters, or over-engineered patterns that serve no current need | ||
| - **Naming clarity**: Flag ambiguous variable/function names that obscure intent | ||
| For each anomaly, articulate **what looks unusual**, **why it caught your attention**, and **what alternative would be expected**. If you cannot articulate the "why", it likely isn't an anomaly worth flagging. | ||
|
|
||
| ## Scope constraint | ||
|
|
||
| Review primarily within the changed diff. **Limited** cross-file lookups are permitted only to *verify* an anomaly already raised by the diff (e.g., confirming a hidden coupling, checking whether a referenced symbol exists, distinguishing dead-on-arrival code from a legitimate caller elsewhere). Do NOT use this allowance to expand into project-wide architecture review, unrelated call chains, or speculative exploration. Every anomaly finding must still be traceable to a specific hunk in the diff — cross-file evidence supports the finding, it does not become its own finding. | ||
|
|
||
| ## Scope of DRY / YAGNI (do NOT raise findings outside this scope) | ||
|
|
||
| The DRY and YAGNI criteria above apply **only to executable code logic**. | ||
| The DRY and YAGNI dimensions in anomaly detection apply **only to executable code logic**. | ||
|
|
||
| - **DRY scope**: Flag duplicated *code logic* (copy-paste functions, repeated control flow, redundant computations). Do NOT flag: | ||
| - Documentation hierarchies that intentionally restate context (e.g., a summary table followed by detailed bullet points) | ||
| - Repetition between docs and code (docs explain, code executes — they serve different audiences) | ||
| - Test code mirroring production code structure (test independence > test DRY) | ||
| - **YAGNI scope**: Flag *speculative code abstractions* (unused parameters, premature interfaces, over-engineered patterns in production code). Do NOT flag: | ||
| - Planning documents listing "future candidates", "Phase 2 検討", or "out of scope but worth considering" sections — these capture design intent for shared understanding, not speculative implementation | ||
| - ADR alternatives sections describing rejected options — these document the decision rationale | ||
| - Comments documenting *known constraints or limitations* of the current implementation (these are not speculation; they are recorded reality) | ||
| - Planning documents listing "future candidates", "Phase 2 検討", or "out of scope but worth considering" sections | ||
| - ADR alternatives sections describing rejected options | ||
| - Comments documenting *known constraints or limitations* of the current implementation | ||
|
|
||
| If a finding cannot be tied to executable code logic, it is out of scope. | ||
|
|
||
| ## Calibration: avoid over-narrowing | ||
|
|
||
| If a finding cannot be tied to executable code logic, it is out of scope — do not raise it. | ||
| The shift to anomaly detection is meant to remove the duplicative checklist work, not to skip review. If reading the diff leaves you with a concrete unease that you can articulate, raise it — even if it doesn't fit a named criterion. Conversely, if you can only flag something by mechanically applying a rule, the deterministic layer already handles that case. | ||
|
|
||
| ## Judgment procedure | ||
|
|
||
| 1. Read the diff from `.takt/review-diff.txt` | ||
| 2. For each changed hunk, check against the 7 criteria above | ||
| 3. For each detected issue, classify as blocking (significantly harms readability/maintainability) or non-blocking (minor suggestion) | ||
| 4. If there is even one blocking issue, judge as REJECT | ||
| 2. Read straight through. After the first pass, list any pattern that read as "unusual / unexpected / hard to explain" | ||
| 3. For each anomaly, classify as blocking (significant unexplained risk) or non-blocking (worth raising but not a blocker) | ||
| 4. If there is even one blocking anomaly, judge as REJECT | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,8 @@ | |
| | 54 | 🔧 Tier 2 | **review 完了待ちの CronCreate 化 + observer 廃止 (Bundle b PR-2) ★ Bundle b** | todo5.md | M | 順位 53 land 後 (Bb-1 で導入する Cron 機構を review 完了待ちにも展開、45s polling + 5s observer polling を完全排除、固定値 wakeup 化) | | ||
| | 55 | 💎 Tier 3 | **config 拡張 + SessionStart catch-up (Bundle b PR-3) ★ Bundle b** | todo5.md | S | 順位 53 / 54 land 後 (固定値の `monitor.toml` 化 + Claude Code 不在時に発火した wakeup を SessionStart で catch-up、AI 不在時の silent loss 防止) | | ||
| | 56 | 🔧 Tier 2 | **comment-lint hook test 拡充 (PR #104 T2-1+T2-2 bundle)** | todo5.md | S | なし (UTF-8 multi-byte 5 パターン + block comment boundary 6 パターンを `locate_string_line_ranges` / `span_overlaps_ranges` の回帰テストとして体系化、PR #104 Critical/Minor fix の固定化) | | ||
| | 57 | 🔧 Tier 2 | **Aggregation cap integration test (PR #105 T2-1 採用)** | todo5.md | S | なし (`collect_all_violations` の MAX_VIOLATIONS contract を test 化、将来の lint 追加時に `truncate(MAX)` 削除 regression を防止する explicit 安全網) | | ||
| | 58 | 🔧 Tier 2 | **post-merge-feedback findings table format 拡張 (Severity / Frequency / Adoption Risk / Recommendation を必須列化)** | todo5.md | S | なし (PR #105 評価で Effort + Rationale のみでは AI 採用判定が安定しないことを確認、rubric ベースの format 固定化で評価コスト削減 + 卻下根拠の言語化) | | ||
|
Comment on lines
+72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 新規タスクの追加先がずれています。 このファイルの運用ルールでは新規タスクは追加しない前提なので、72-73行の追加は役割と衝突しています。実体は 🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 本 finding は採用しません。docs/todo.md の運用ルール「新規タスクは追加しない」は task body (詳細セクション) を todoN.md (現在 todo5.md) に置く規約で、priority table 行の追加は既存運用です (順位 1-56 すべて同パターン、本 PR の 順位 57/58 もユーザー指示で同様に追加)。文言の曖昧性自体は既知で、順位 32 (★ Bundle V) として「docs/todo.md ヘッダの「新規タスクは追加しない」表記を実態整合 (PR #95 T3-3)」が登録済み。本 PR では変更せず、順位 32 の land 時に文言を更新します。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
文言の曖昧性は順位 32(★ Bundle V)の land 時に整合される予定とのこと、承知しました。 ✏️ Learnings added
🧠 Learnings used |
||
|
|
||
| **戦略**: Tier 1 を 2〜3 セッションで片付け → Tier 2 で ADR-032 の前提 + rate-limit + convergence cost 削減を進める → Tier 3 で ADR-032 を land + ドキュメント整備。Tier 4-5 は cleanup / 外部展開で daily efficiency への直接効果は小さい。 | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.