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
15 changes: 0 additions & 15 deletions .takt/facets/instructions/review-simplicity.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,6 @@ The diff has been pre-collected by push-runner (Rust exe) and saved to `.takt/re
**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.

### Optional: lint-screen pre-pass (Phase c §8.E, ADR-038 試験運用)

If `.takt/lint-screen-report.md` exists, push-runner has already run a mistral:7b lint pre-pass on the diff. Read this file as **supplementary context** (treat as advisory, not authoritative):

- Coverage: rule names from a fixed canonical list (`unused-import` / `no-var` / `no-unused-vars` / `magic-number` / `dead-code` / `deep-nesting` / `complexity`)
- Quality: agreement 75% with Claude baseline (Phase b' conditional GO) — false positives and recall misses are expected
- Use it to:
- Cross-check anomalies you already noticed (consensus signal)
- **Skip dimensions** the lint-screen already covered (avoid duplicate findings of unused-import / magic-number etc.)
- Do NOT use it to:
- Adopt findings verbatim without diff verification
- Override your own judgment on subjective anomalies (deep-nesting boundary, complexity)

If the report shows `screen_decision: informational` and zero findings, that is a **weak signal of a clean diff** — still review yourself, but you can be more concise in approval rationale.

## 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:
Expand Down
112 changes: 112 additions & 0 deletions .takt/facets/policies/review-anomaly.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
<!-- Used by: pre-push-review / pre-push-review-refute (all review steps). Shadows the takt builtin `review` policy for pre-push only. See ADR-056. -->

# Review Policy (anomaly mode)

Shared judgment criteria for the pre-push review steps (reviewers / refutation /
supervisor). This policy deliberately defines **no REJECT checklist of its own**.
What counts as a blocking problem is owned by each step's instruction facet; this
policy only constrains **how** a finding must be evidenced, scoped, and tracked.

Rationale: the deterministic layer intercepts structural violations at write time
(PostToolUse lint hooks) and during fix iterations (`fix-metrics-check.ps1`), so a
reviewer-side checklist duplicates a layer that already ran and turns unremarkable
code into fix iterations. See ADR-036 (three-layer review) and ADR-056 (this policy).

## Principles

| Principle | Criteria |
|-----------|----------|
| Fact-check | Verify against the actual code before raising anything. Never speculate |
| Eliminate ambiguity | "Clean this up a bit" is prohibited. Give file, line, and a proposed fix |
| Practical fixes | Propose implementable changes, not theoretical ideals |
| Articulable concern | If you cannot state what looks unusual and why, it is not a finding |

## Where REJECT criteria come from

Each step's instruction facet defines what qualifies as blocking:

- `review-simplicity` — an articulable anomaly (unexplained complexity, hidden coupling, dead-on-arrival code, ...)
- `review-security` — a concrete exploit path (who controls the input, what newly becomes possible)
- `refute-finding` — a finding that survives adversarial refutation
- `supervise` — the current iteration's blocking findings are resolved

This policy adds none of its own. In particular there is **no list of "REJECT
without exception" patterns**: DRY violations, TODO comments, unused code, fallback
values, and similar named patterns are grounds for a finding only when the step's
own criteria are met and the evidence rules below are satisfied. Structural metrics
alone (file length, duplication count, nesting depth, comment style) are never
sufficient grounds — the deterministic layer owns them.

## Scope Determination

| Situation | Verdict |
|-----------|---------|
| Problem introduced by this change | Blocking |
| Code made unused *by this change* (arguments, imports, branches) | Blocking — change-induced |
| Pre-existing problem in a changed file | Non-blocking — record only |
| Problem in an unchanged file | Non-blocking — record only |
| Refactoring beyond the task scope | Non-blocking — note as a suggestion |

"It sits in a file this change touched" does not make a pre-existing problem
blocking. Opportunistic cleanup of surrounding code is out of scope for pre-push
review: that judgment needs whole-PR context and belongs to the post-PR layer
(ADR-019 / ADR-027).

## APPROVE

Approve when no blocking finding remains. **Non-blocking warnings do not block
approval** — record them and approve. Warnings ride downstream to the post-PR
layer rather than gating the push.

## Fact-Checking

| Do | Do Not |
|----|--------|
| Open the file and read the actual code | Assume "it should be fixed already" |
| Grep for call sites before calling code dead | Raise issues from memory |
| Cross-reference type definitions and schemas | Guess that a premise holds |
| Distinguish generated files (reports) from source | Review generated files as if they were source |

## Writing Specific Feedback

Every finding must state:

- **Which file and line**
- **What the problem is**, and why it read as unusual
- **How to fix it**
- **If proposing consolidation or abstraction, why that placement is the natural one**

```text
❌ "Review the structure"
❌ "Refactoring is needed"

✅ "src/auth/service.ts:45 — validateUser() is duplicated in 3 places.
Extract into a shared function."
```

## Finding ID Tracking (`finding_id`)

Findings are tracked by ID so that iterations converge instead of circling.

- Every blocking finding carries a `finding_id`
- The same problem reuses the same `finding_id` across iterations — status is conveyed by the table a finding appears in, not by its id
- A `finding_id` means one and only one problem. If the problem, its evidence, or its reproduction conditions change, issue a new id
- Findings without a `finding_id` are invalid and cannot be used as REJECT grounds
- REJECT is valid only when at least one finding is `new`, `persists`, or `reopened`

## Reopen Conditions (`resolved` -> open)

Reopening a resolved finding requires all three:

1. Reproduction steps (command / input)
2. Expected result vs. actual result
3. Failing file / line evidence

If any of the three is missing, the reopen is invalid. If the reproduction
conditions changed, it is a different problem — issue a new `finding_id`.

## Recurring Findings

If the same finding keeps recurring across iterations, the fix instruction itself
is likely the problem. Propose a different approach rather than repeating the same
instruction.
8 changes: 4 additions & 4 deletions .takt/workflows/pre-push-review-refute.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ steps:
edit: false
persona: simplicity-reviewer
model: sonnet
policy: review
policy: review-anomaly
provider_options:
claude:
allowed_tools:
Expand All @@ -70,7 +70,7 @@ steps:
edit: false
persona: security-reviewer
model: sonnet
policy: review
policy: review-anomaly
knowledge: security
provider_options:
claude:
Expand Down Expand Up @@ -106,7 +106,7 @@ steps:
edit: false
persona: code-reviewer
model: haiku
policy: review
policy: review-anomaly
provider_options:
claude:
allowed_tools:
Expand Down Expand Up @@ -181,7 +181,7 @@ steps:
edit: false
persona: supervisor
model: sonnet
policy: review
policy: review-anomaly
provider_options:
claude:
allowed_tools:
Expand Down
6 changes: 3 additions & 3 deletions .takt/workflows/pre-push-review.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ steps:
edit: false
persona: simplicity-reviewer
model: sonnet
policy: review
policy: review-anomaly
provider_options:
claude:
allowed_tools:
Expand All @@ -62,7 +62,7 @@ steps:
edit: false
persona: security-reviewer
model: sonnet
policy: review
policy: review-anomaly
knowledge: security
provider_options:
claude:
Expand Down Expand Up @@ -136,7 +136,7 @@ steps:
edit: false
persona: supervisor
model: sonnet
policy: review
policy: review-anomaly
provider_options:
claude:
allowed_tools:
Expand Down
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
- [ADR-053: Stop hook による tool call leak 検知](docs/adr/adr-053-stop-tool-call-leak-detection.md) *(試験運用)*
- [ADR-054: prompt injection 信頼境界の 3 層防御](docs/adr/adr-054-prompt-injection-trust-boundary-defense.md) *(試験運用)*
- [ADR-055: 発火テレメトリ収集層 — ハーネス ROI 棚卸しの決定論的観測基盤](docs/adr/adr-055-firing-telemetry-collection.md) *(試験運用)*
- [ADR-056: takt builtin review policy の shadow — policy 層を anomaly 設計に整合させる](docs/adr/adr-056-review-policy-anomaly-shadow.md) *(試験運用)*

## 開発 convention / チェックリスト

Expand Down
Loading