fix(requirements): stop review-test self-reinforcing loop (Closes #182)

REQUIREMENTS.md

@@ -196,3 +196,11 @@ HERMIT 自身を制約する制御面 (`internal/risk/`・`internal/permissions/
 - 受け入れ条件: `internal/risk/`・`internal/permissions/`・`internal/readiness/`・`harness.toml`・`.claude/`・`CLAUDE.md` のいずれかのみを 1 ファイル 1 行変更しても HIGH と判定されること。制御面以外の `internal/` 配下の変更や `cmd/hermit/templates/` 配下のみの変更は従来通りの判定 (MEDIUM・LOW) を維持すること
 - verify: test
 - 実装状況: 実装済み — `internal/risk/evaluator.go` の `DefaultConfig()`。`internal/risk/req_test.go` の `TestREQ015_ControlPlanePathsAreHighRisk` で検証
+
+## REQ-016: review-test のハッシュ判定は仕様(受け入れ条件・verify)のみを対象とする
+
+reconcile sweep の review-test は、要件の「仕様」が変わったときにのみ発火しなければならない。要件ブロック全体 (見出し・説明文・`実装状況` 進捗メモを含む) をハッシュ対象にすると、review-test を解決する作業自体が `実装状況` 行を書き換えるため、次の sweep で再び「テキストが変化した」と判定され review-test が無限に再発火する自己増殖ループになる (Issue #182)。ハッシュは `受け入れ条件` と `verify` の値のみから計算し、`実装状況` を含む残りのブロックは対象外とする。
+
+- 受け入れ条件: `Requirement.Hash` が `受け入れ条件` と `verify` のみから計算され、要件ブロック全体からは計算されないこと。`実装状況` 行のみを変更しても次の sweep で review-test が発火しないこと。`受け入れ条件` の変更、および `verify` の `test` ↔ `manual` の切り替えは従来どおり発火すること。見出しや説明文のみの変更では発火しないこと。ハッシュストアに計算方式のバージョンが記録され、方式変更後の初回 sweep は全件を再計算・保存するのみで Issue を起票しないこと
+- verify: test
+- 実装状況: 実装済み — `internal/requirements/requirements.go` の `specHash` が `AcceptanceCriteria` と `Verify` のみからハッシュを計算するように変更 (旧 `hashText(block)` を置き換え)。`internal/requirements/hashstore.go` の `HashStore` インターフェースを `Load() (version int, hashes map[string]string, err error)` / `Save(version int, hashes map[string]string) error` に拡張し、`HashSchemeVersion` 定数 (現在値 2) を導入。旧形式 (バージョン無しの素の map) のファイルは version 0 として扱われ後方互換。`internal/requirements/sweep.go` の `Sweep` は読み込んだバージョンが `HashSchemeVersion` と異なる場合 `schemeChanged` として HashChanged 判定を強制的に false にし (review-test を発火させず)、sweep 終了時に現行バージョンでハッシュを保存し直すことで移行を1回のsweepで完了させる。自己増殖ループの回帰テストは `internal/requirements/sweep_test.go` の `TestSweep_ImplementationStatusOnlyChange_DoesNotFireReviewTest`、スキーマ移行の回帰テストは同ファイルの `TestSweep_HashSchemeMigration_DoesNotFireReviewTest_JustRecomputesAndSaves`、ハッシュ計算自体の単体テストは `internal/requirements/requirements_test.go` の `TestParse_HashUnaffectedByImplementationStatusField` / `TestParse_HashUnaffectedByTitleOrDescriptionOnly` / `TestParse_HashChangesWithVerifyMode` で検証。REQ-ID 命名規約に沿った `TestREQ016_ReviewTestHashIgnoresImplementationStatus` を追加

internal/requirements/hashstore.go

@@ -6,21 +6,51 @@ import (
 	"path/filepath"
 )
 
-// HashStore persists the last-seen content hash of each requirement so the
-// sweep can detect when a requirement's text has changed since the previous
-// run. This is *not* a satisfaction record — it never says whether a
-// requirement is "done"; it only remembers enough to avoid re-firing a
-// "review the test" issue every single sweep for a change that was already
-// reported.
+// HashSchemeVersion identifies the algorithm used to compute Requirement.Hash
+// (see specHash). It must be bumped whenever the set of fields that feed the
+// hash changes, so HashStore can tell a genuine spec change apart from a
+// hash produced by a since-retired scheme.
+//
+// Bumping this on its own is intentionally *not* enough to make old stored
+// hashes compare unequal to new ones and fire review-test: Sweep checks the
+// stored version against this constant and, on a mismatch, treats it as
+// "nothing changed" for review-test purposes — it only recomputes and
+// persists hashes under the new scheme (Issue #182's migration
+// requirement). This avoids a scheme change (like #182's fix itself, which
+// narrowed the hash to exclude 実装状況) causing every requirement to look
+// "changed" and firing review-test for the entire document in one sweep.
+const HashSchemeVersion = 2
+
+// HashStore persists the last-seen content hash of each requirement (plus
+// the scheme version those hashes were computed under) so the sweep can
+// detect when a requirement's *spec* has changed since the previous run.
+// This is *not* a satisfaction record — it never says whether a requirement
+// is "done"; it only remembers enough to avoid re-firing a "review the test"
+// issue every single sweep for a change that was already reported.
 type HashStore interface {
-	Load() (map[string]string, error)
-	Save(map[string]string) error
+	// Load returns the previously stored scheme version and hash map. A
+	// store that has never been written returns version 0 (which never
+	// equals a real HashSchemeVersion, so callers can detect "no prior
+	// data" the same way they detect "old scheme") and an empty map.
+	Load() (version int, hashes map[string]string, err error)
+	// Save persists hashes under the given scheme version.
+	Save(version int, hashes map[string]string) error
 }
 
 // DefaultHashStorePath is the path, relative to the project root, where
 // FileHashStore persists requirement hashes by default.
 const DefaultHashStorePath = ".hermit/requirements-hashes.json"
 
+// fileHashStoreData is the on-disk JSON shape used by FileHashStore.
+type fileHashStoreData struct {
+	// Version is the HashSchemeVersion the Hashes below were computed
+	// under. Absent/zero in files written before Issue #182 introduced
+	// versioning, which is exactly the "unknown/old scheme" sentinel value
+	// callers need.
+	Version int               `json:"version"`
+	Hashes  map[string]string `json:"hashes"`
+}
+
 // FileHashStore persists requirement hashes as JSON on disk.
 type FileHashStore struct {
 	Path string
@@ -32,33 +62,54 @@ func NewFileHashStore(dir string) FileHashStore {
 	return FileHashStore{Path: filepath.Join(dir, DefaultHashStorePath)}
 }
 
-// Load reads the stored hash map. A missing file is not an error — it
-// returns an empty map, since that's the expected state before the first
-// sweep has ever run.
-func (f FileHashStore) Load() (map[string]string, error) {
+// Load reads the stored version and hash map. A missing file is not an
+// error — it returns version 0 and an empty map, since that's the expected
+// state before the first sweep has ever run.
+func (f FileHashStore) Load() (int, map[string]string, error) {
 	data, err := os.ReadFile(f.Path)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return map[string]string{}, nil
+			return 0, map[string]string{}, nil
 		}
-		return nil, err
+		return 0, nil, err
 	}
-	var m map[string]string
-	if err := json.Unmarshal(data, &m); err != nil {
-		return nil, err
+
+	// Backward compatibility: files written before Issue #182 are a bare
+	// {"REQ-001": "hash", ...} map with no "version"/"hashes" envelope.
+	// Detect that shape and treat it as version 0 (unknown/old scheme) so
+	// it goes through the same "recompute, don't fire" migration path as
+	// any other scheme mismatch, instead of failing to unmarshal.
+	var legacy map[string]string
+	if err := json.Unmarshal(data, &legacy); err == nil {
+		if _, isEnvelope := legacy["version"]; !isEnvelope {
+			if legacy == nil {
+				legacy = map[string]string{}
+			}
+			// Note: real envelope data can never reach this branch — its
+			// "hashes" field is a JSON object, not a string, so unmarshaling
+			// an envelope into map[string]string fails above and we never
+			// get here with err == nil for that shape.
+			return 0, legacy, nil
+		}
+	}
+
+	var d fileHashStoreData
+	if err := json.Unmarshal(data, &d); err != nil {
+		return 0, nil, err
 	}
-	if m == nil {
-		m = map[string]string{}
+	if d.Hashes == nil {
+		d.Hashes = map[string]string{}
 	}
-	return m, nil
+	return d.Version, d.Hashes, nil
 }
 
-// Save writes the hash map to disk, creating parent directories as needed.
-func (f FileHashStore) Save(hashes map[string]string) error {
+// Save writes the version and hash map to disk, creating parent directories
+// as needed.
+func (f FileHashStore) Save(version int, hashes map[string]string) error {
 	if err := os.MkdirAll(filepath.Dir(f.Path), 0o755); err != nil {
 		return err
 	}
-	data, err := json.MarshalIndent(hashes, "", "  ")
+	data, err := json.MarshalIndent(fileHashStoreData{Version: version, Hashes: hashes}, "", "  ")
 	if err != nil {
 		return err
 	}
@@ -68,23 +119,26 @@ func (f FileHashStore) Save(hashes map[string]string) error {
 // memHashStore is a trivial in-memory HashStore, useful for tests and for
 // callers that intentionally don't want cross-run persistence.
 type memHashStore struct {
-	data map[string]string
+	version int
+	data    map[string]string
 }
 
-// NewMemHashStore returns an in-memory HashStore starting empty.
+// NewMemHashStore returns an in-memory HashStore starting empty (version 0,
+// as if never written).
 func NewMemHashStore() HashStore {
 	return &memHashStore{data: map[string]string{}}
 }
 
-func (m *memHashStore) Load() (map[string]string, error) {
+func (m *memHashStore) Load() (int, map[string]string, error) {
 	out := make(map[string]string, len(m.data))
 	for k, v := range m.data {
 		out[k] = v
 	}
-	return out, nil
+	return m.version, out, nil
 }
 
-func (m *memHashStore) Save(hashes map[string]string) error {
+func (m *memHashStore) Save(version int, hashes map[string]string) error {
+	m.version = version
 	m.data = make(map[string]string, len(hashes))
 	for k, v := range hashes {
 		m.data[k] = v

internal/requirements/hashstore_test.go

@@ -1,16 +1,20 @@
 package requirements
 
 import (
+	"os"
 	"path/filepath"
 	"testing"
 )
 
 func TestFileHashStore_LoadMissingFileReturnsEmptyMap(t *testing.T) {
 	store := FileHashStore{Path: filepath.Join(t.TempDir(), "does-not-exist.json")}
-	m, err := store.Load()
+	version, m, err := store.Load()
 	if err != nil {
 		t.Fatalf("Load() error = %v", err)
 	}
+	if version != 0 {
+		t.Errorf("expected version 0 for a never-written store, got %d", version)
+	}
 	if len(m) != 0 {
 		t.Errorf("expected empty map, got %v", m)
 	}
@@ -20,13 +24,16 @@ func TestFileHashStore_SaveThenLoadRoundTrip(t *testing.T) {
 	store := FileHashStore{Path: filepath.Join(t.TempDir(), "nested", "hashes.json")}
 	want := map[string]string{"REQ-001": "abc123", "REQ-002": "def456"}
 
-	if err := store.Save(want); err != nil {
+	if err := store.Save(HashSchemeVersion, want); err != nil {
 		t.Fatalf("Save() error = %v", err)
 	}
-	got, err := store.Load()
+	version, got, err := store.Load()
 	if err != nil {
 		t.Fatalf("Load() error = %v", err)
 	}
+	if version != HashSchemeVersion {
+		t.Errorf("version = %d, want %d", version, HashSchemeVersion)
+	}
 	if len(got) != len(want) {
 		t.Fatalf("got %v, want %v", got, want)
 	}
@@ -37,6 +44,28 @@ func TestFileHashStore_SaveThenLoadRoundTrip(t *testing.T) {
 	}
 }
 
+func TestFileHashStore_LoadLegacyBareMap_TreatedAsVersionZero(t *testing.T) {
+	// Files written before Issue #182 introduced the version envelope are a
+	// bare {"REQ-001": "hash"} map. Load must recognize this shape and
+	// report version 0 (unknown/old scheme), not fail to parse it.
+	path := filepath.Join(t.TempDir(), "legacy.json")
+	if err := os.WriteFile(path, []byte(`{"REQ-001":"abc123"}`), 0o644); err != nil {
+		t.Fatalf("writing legacy fixture: %v", err)
+	}
+	store := FileHashStore{Path: path}
+
+	version, got, err := store.Load()
+	if err != nil {
+		t.Fatalf("Load() error = %v", err)
+	}
+	if version != 0 {
+		t.Errorf("version = %d, want 0 for legacy bare-map file", version)
+	}
+	if got["REQ-001"] != "abc123" {
+		t.Errorf("got %v, want legacy hash preserved", got)
+	}
+}
+
 func TestNewFileHashStore_UsesDefaultPath(t *testing.T) {
 	dir := t.TempDir()
 	store := NewFileHashStore(dir)
@@ -48,13 +77,16 @@ func TestNewFileHashStore_UsesDefaultPath(t *testing.T) {
 
 func TestMemHashStore_RoundTrip(t *testing.T) {
 	store := NewMemHashStore()
-	if err := store.Save(map[string]string{"REQ-001": "x"}); err != nil {
+	if err := store.Save(HashSchemeVersion, map[string]string{"REQ-001": "x"}); err != nil {
 		t.Fatalf("Save() error = %v", err)
 	}
-	got, err := store.Load()
+	version, got, err := store.Load()
 	if err != nil {
 		t.Fatalf("Load() error = %v", err)
 	}
+	if version != HashSchemeVersion {
+		t.Errorf("version = %d, want %d", version, HashSchemeVersion)
+	}
 	if got["REQ-001"] != "x" {
 		t.Errorf("got %v", got)
 	}

internal/requirements/requirements.go

@@ -41,10 +41,24 @@ type Requirement struct {
 	// Verify is "test" (default) or "manual".
 	Verify VerifyMode
 	// Body is the full raw text of this requirement's block (header plus
-	// fields), used to compute Hash.
+	// fields). It is kept for diagnostics/issue bodies, but is deliberately
+	// NOT used to compute Hash (see Hash's doc comment).
 	Body string
-	// Hash is a stable content hash of Body, used to detect requirement-text
+	// Hash is a stable content hash of only the spec-bearing fields
+	// (AcceptanceCriteria and Verify), used to detect requirement-*spec*
 	// changes across sweeps (e.g. to trigger a "review the test" issue).
+	//
+	// It deliberately excludes the rest of Body — most notably a
+	// "- 実装状況:" (implementation status) progress-note field, which the
+	// review-test workflow itself writes into when a human/engineer resolves
+	// a review-test issue. Hashing the whole block created a self-sustaining
+	// loop (Issue #182): resolving a review-test issue edited the
+	// 実装状況 line, which changed Body's hash, which made the next sweep
+	// think the requirement text had changed again, which re-opened
+	// review-test forever — even though the actual spec (acceptance
+	// criteria / verify mode) never changed. Also excludes the header
+	// title and any free-form description text, so wording-only edits to
+	// those don't spuriously trigger review-test either.
 	Hash string
 }
 
@@ -115,15 +129,18 @@ func Parse(doc string) ([]Requirement, error) {
 			AcceptanceCriteria: criteria,
 			Verify:             verify,
 			Body:               block,
-			Hash:               hashText(block),
 		})
+		reqs[len(reqs)-1].Hash = specHash(reqs[len(reqs)-1])
 	}
 	return reqs, nil
 }
 
-// hashText returns a stable hex-encoded sha256 hash of s, used to detect
-// requirement-text changes between sweeps.
-func hashText(s string) string {
+// specHash returns a stable hex-encoded sha256 hash of just the spec-bearing
+// fields of req (AcceptanceCriteria and Verify) — see Requirement.Hash for
+// why the rest of the block (title, description, 実装状況 progress notes,
+// etc.) must NOT be included.
+func specHash(req Requirement) string {
+	s := "verify:" + string(req.Verify) + "\n" + "criteria:" + req.AcceptanceCriteria
 	sum := sha256.Sum256([]byte(strings.TrimSpace(s)))
 	return hex.EncodeToString(sum[:])
 }