From e9ab3594b3e06c97ad98dc33697680855ce18dee Mon Sep 17 00:00:00 2001 From: ytnobody Date: Tue, 28 Jul 2026 10:39:32 +0900 Subject: [PATCH] =?UTF-8?q?fix(requirements):=20stop=20review-test=20from?= =?UTF-8?q?=20re-firing=20on=20=E5=AE=9F=E8=A3=85=E7=8A=B6=E6=B3=81=20edit?= =?UTF-8?q?s=20(Closes=20#182)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reconcile sweep hashed the entire requirement block, including the 実装状況 (implementation status) field. Resolving a review-test issue writes findings into that field, which changed the hash and made the next sweep think the requirement text had changed again — a self-reinforcing loop with zero real regression detections. Hash is now computed only from AcceptanceCriteria and Verify (the actual spec), via specHash. HashStore gained a scheme version (HashSchemeVersion) so upgrading doesn't cause every requirement to look "changed" at once: a version mismatch is treated as "no change" for one sweep, hashes are just recomputed and persisted under the new scheme. Also fixed a related gap where flipping verify: test<->manual never triggered review-test, since the manual short-circuit ran before the hash comparison. Assumption: HashSchemeVersion starts at 2 (implicit prior scheme is 1), and legacy on-disk hash files with no version envelope are treated as version 0 so they go through the same migration path. --- REQUIREMENTS.md | 8 + internal/requirements/hashstore.go | 110 +++++++++--- internal/requirements/hashstore_test.go | 42 ++++- internal/requirements/requirements.go | 29 ++- internal/requirements/requirements_test.go | 95 ++++++++++ internal/requirements/sweep.go | 33 +++- internal/requirements/sweep_test.go | 200 ++++++++++++++++++++- 7 files changed, 465 insertions(+), 52 deletions(-) diff --git a/REQUIREMENTS.md b/REQUIREMENTS.md index 02dfdc0..c7d963c 100644 --- a/REQUIREMENTS.md +++ b/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` を追加 diff --git a/internal/requirements/hashstore.go b/internal/requirements/hashstore.go index aac215c..242657d 100644 --- a/internal/requirements/hashstore.go +++ b/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 diff --git a/internal/requirements/hashstore_test.go b/internal/requirements/hashstore_test.go index 31a4563..ec1fcdc 100644 --- a/internal/requirements/hashstore_test.go +++ b/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) } diff --git a/internal/requirements/requirements.go b/internal/requirements/requirements.go index 3d19b2c..d03b86e 100644 --- a/internal/requirements/requirements.go +++ b/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[:]) } diff --git a/internal/requirements/requirements_test.go b/internal/requirements/requirements_test.go index a59d23b..76fb5a0 100644 --- a/internal/requirements/requirements_test.go +++ b/internal/requirements/requirements_test.go @@ -114,6 +114,101 @@ func TestParse_HashChangesWithText(t *testing.T) { } } +// TestParse_HashUnaffectedByImplementationStatusField is the regression test +// for Issue #182: hashing the whole requirement block (including a +// "- 実装状況:" progress-note field) meant that *resolving* a review-test +// issue — which involves writing findings into 実装状況 — changed the +// block's hash, which made the next sweep think the requirement text had +// changed again, re-opening review-test forever. The hash must depend only +// on 受け入れ条件 and verify, not on 実装状況. +func TestParse_HashUnaffectedByImplementationStatusField(t *testing.T) { + before := `# 要件定義書 + +## REQ-200: 自己増殖しない要件 +- 受け入れ条件: review-test が自己増殖しないこと +- verify: test +` + reqsBefore, err := Parse(before) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + + // Simulate resolving a review-test issue: an engineer/human appends an + // "- 実装状況:" progress note to the block, without touching the + // acceptance criteria or verify mode at all. + after := `# 要件定義書 + +## REQ-200: 自己増殖しない要件 +- 受け入れ条件: review-test が自己増殖しないこと +- verify: test +- 実装状況: #182 で調査済み。テストは要件を正しく検証している。 +` + reqsAfter, err := Parse(after) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + + if reqsBefore[0].Hash != reqsAfter[0].Hash { + t.Errorf("hash changed after only 実装状況 was added: before=%q after=%q — this is the #182 self-reinforcing loop", + reqsBefore[0].Hash, reqsAfter[0].Hash) + } +} + +// TestParse_HashUnaffectedByTitleOrDescriptionOnly covers the acceptance +// criterion that editing only the requirement heading text or free-form +// description prose (neither 受け入れ条件 nor verify) must not change Hash. +func TestParse_HashUnaffectedByTitleOrDescriptionOnly(t *testing.T) { + before := `## REQ-201: 元のタイトル +補足説明の文章です。 + +- 受け入れ条件: 変わらない条件 +- verify: test +` + after := `## REQ-201: 書き直したタイトル +補足説明の文章を書き直しました。もっと詳しく説明します。 + +- 受け入れ条件: 変わらない条件 +- verify: test +` + reqsBefore, err := Parse(before) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + reqsAfter, err := Parse(after) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if reqsBefore[0].Hash != reqsAfter[0].Hash { + t.Errorf("hash changed after only title/description prose changed: before=%q after=%q", + reqsBefore[0].Hash, reqsAfter[0].Hash) + } +} + +// TestParse_HashChangesWithVerifyMode covers the acceptance criterion that +// switching verify: test <-> manual must still change Hash, since that's a +// genuine change to how the requirement is judged. +func TestParse_HashChangesWithVerifyMode(t *testing.T) { + testDoc := `## REQ-202: verify 切り替え +- 受け入れ条件: 同じ条件文 +- verify: test +` + manualDoc := `## REQ-202: verify 切り替え +- 受け入れ条件: 同じ条件文 +- verify: manual +` + reqsTest, err := Parse(testDoc) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + reqsManual, err := Parse(manualDoc) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if reqsTest[0].Hash == reqsManual[0].Hash { + t.Errorf("hash should change when verify mode changes between test and manual") + } +} + func TestParse_CRLFNormalized(t *testing.T) { crlfDoc := strings.ReplaceAll(sampleDoc, "\n", "\r\n") reqsLF, _ := Parse(sampleDoc) diff --git a/internal/requirements/sweep.go b/internal/requirements/sweep.go index a0afc56..5754ddc 100644 --- a/internal/requirements/sweep.go +++ b/internal/requirements/sweep.go @@ -54,7 +54,7 @@ type SweepOptions struct { // Sweep is safe to call repeatedly against an unchanged requirements doc and // unchanged test results: it will not create duplicate issues. func Sweep(reqs []Requirement, opts SweepOptions) ([]ReqResult, error) { - prevHashes, err := opts.Hashes.Load() + prevVersion, prevHashes, err := opts.Hashes.Load() if err != nil { return nil, fmt.Errorf("loading requirement hashes: %w", err) } @@ -62,6 +62,16 @@ func Sweep(reqs []Requirement, opts SweepOptions) ([]ReqResult, error) { prevHashes = map[string]string{} } + // schemeChanged is true when the stored hashes were computed under a + // different (or no prior) HashSchemeVersion. In that case the raw hash + // values are not comparable to the ones we're about to compute — every + // requirement would spuriously look "changed" even if its spec is + // identical. Per Issue #182's migration requirement, treat this as "no + // change" for review-test purposes on this one sweep: just recompute + // and persist hashes under the current scheme, without opening any + // review-test issues. + schemeChanged := prevVersion != HashSchemeVersion + newHashes := make(map[string]string, len(prevHashes)) // Preserve hashes for requirements not present in this sweep (e.g. a doc // that only contains a subset), so a partial sweep doesn't erase memory. @@ -71,13 +81,14 @@ func Sweep(reqs []Requirement, opts SweepOptions) ([]ReqResult, error) { var results []ReqResult for _, req := range reqs { - if req.Verify == VerifyManual { - results = append(results, ReqResult{ReqID: req.ID, Title: req.Title, Status: Skipped}) - continue - } - + // Hash comparison (and the resulting review-test issue) happens + // before the verify:manual short-circuit below, so that a + // requirement's verify mode flipping between test and manual — a + // genuine spec change that specHash captures — still triggers + // review-test even though the requirement isn't otherwise judged by + // running a test. oldHash, hadHash := prevHashes[req.ID] - hashChanged := hadHash && oldHash != req.Hash + hashChanged := !schemeChanged && hadHash && oldHash != req.Hash newHashes[req.ID] = req.Hash result := ReqResult{ReqID: req.ID, Title: req.Title, HashChanged: hashChanged} @@ -95,6 +106,12 @@ func Sweep(reqs []Requirement, opts SweepOptions) ([]ReqResult, error) { } } + if req.Verify == VerifyManual { + result.Status = Skipped + results = append(results, result) + continue + } + status, output, runErr := opts.Runner.Run(req.ID) result.TestOutput = output if runErr != nil { @@ -133,7 +150,7 @@ func Sweep(reqs []Requirement, opts SweepOptions) ([]ReqResult, error) { results = append(results, result) } - if err := opts.Hashes.Save(newHashes); err != nil { + if err := opts.Hashes.Save(HashSchemeVersion, newHashes); err != nil { return nil, fmt.Errorf("saving requirement hashes: %w", err) } diff --git a/internal/requirements/sweep_test.go b/internal/requirements/sweep_test.go index a9c03df..5f076c6 100644 --- a/internal/requirements/sweep_test.go +++ b/internal/requirements/sweep_test.go @@ -57,11 +57,15 @@ func (f *fakeIssueClient) CreateIssue(reqID string, kind IssueKind, title, body } func reqTest(id, title string) Requirement { - return Requirement{ID: id, Title: title, AcceptanceCriteria: "criteria for " + id, Verify: VerifyTest, Body: id + " body v1", Hash: hashText(id + " body v1")} + r := Requirement{ID: id, Title: title, AcceptanceCriteria: "criteria for " + id, Verify: VerifyTest, Body: id + " body v1"} + r.Hash = specHash(r) + return r } func reqManual(id, title string) Requirement { - return Requirement{ID: id, Title: title, AcceptanceCriteria: "criteria for " + id, Verify: VerifyManual, Body: id + " manual body", Hash: hashText(id + " manual body")} + r := Requirement{ID: id, Title: title, AcceptanceCriteria: "criteria for " + id, Verify: VerifyManual, Body: id + " manual body"} + r.Hash = specHash(r) + return r } // --- tests ----------------------------------------------------------------- @@ -179,9 +183,10 @@ func TestSweep_HashChange_CreatesReviewIssue_ThenIdempotent(t *testing.T) { runner := &fakeRunner{statuses: map[string]TestStatus{"REQ-040": TestPassed}} issues := &fakeIssueClient{} hashes := NewMemHashStore() - // Seed the hash store with a stale hash to simulate a text change since - // the last sweep. - if err := hashes.Save(map[string]string{"REQ-040": "some-stale-hash-value"}); err != nil { + // Seed the hash store with a stale hash (under the *current* scheme + // version, so this simulates a genuine spec change since the last + // sweep rather than a scheme migration). + if err := hashes.Save(HashSchemeVersion, map[string]string{"REQ-040": "some-stale-hash-value"}); err != nil { t.Fatalf("seeding hash store: %v", err) } @@ -266,6 +271,191 @@ func TestSweep_PropagatesIssueClientError(t *testing.T) { } } +// TestSweep_ImplementationStatusOnlyChange_DoesNotFireReviewTest is the +// end-to-end regression test for Issue #182's self-reinforcing loop: +// resolving a review-test issue by recording findings in the requirement's +// "- 実装状況:" field must not, by itself, cause the next sweep to fire +// review-test again. +func TestSweep_ImplementationStatusOnlyChange_DoesNotFireReviewTest(t *testing.T) { + runner := &fakeRunner{statuses: map[string]TestStatus{"REQ-180": TestPassed}} + issues := &fakeIssueClient{} + hashes := NewMemHashStore() + + before := reqTest("REQ-180", "自己増殖しない") + + // First sweep: establish a baseline hash (no prior hash yet, so no + // review-test fires — this mirrors TestSweep_FirstRunEver...). + if _, err := Sweep([]Requirement{before}, SweepOptions{Runner: runner, Issues: issues, Hashes: hashes}); err != nil { + t.Fatalf("Sweep() (baseline run) error = %v", err) + } + if len(issues.created) != 0 { + t.Fatalf("baseline run should not create any issue, got %+v", issues.created) + } + + // Simulate an engineer resolving a (hypothetical) review-test issue by + // writing a finding into 実装状況 — same AcceptanceCriteria and Verify, + // only the progress-note field differs, so Body differs but Hash must + // not. + after := before + after.Body = before.Body + "\n- 実装状況: #182 の調査により、テストは要件を満たしていることを確認済み。" + after.Hash = specHash(after) + if after.Hash != before.Hash { + t.Fatalf("precondition failed: specHash must be unaffected by Body/実装状況 change") + } + + results, err := Sweep([]Requirement{after}, SweepOptions{Runner: runner, Issues: issues, Hashes: hashes}) + if err != nil { + t.Fatalf("Sweep() (post-resolution run) error = %v", err) + } + if results[0].HashChanged { + t.Errorf("HashChanged = true after only 実装状況 changed — this is the #182 self-reinforcing loop") + } + if results[0].IssueCreated { + t.Errorf("a review-test issue was re-created after only 実装状況 changed: %+v", results[0]) + } + if len(issues.created) != 0 { + t.Errorf("expected no issues created across both sweeps, got %+v", issues.created) + } +} + +// TestSweep_HashSchemeMigration_DoesNotFireReviewTest_JustRecomputesAndSaves +// covers the migration requirement: when the stored hashes were computed +// under an older/unknown HashSchemeVersion (e.g. a store from before Issue +// #182, or bumped again in the future), the first sweep afterward must not +// treat every requirement as "changed" (which would fire review-test for +// the entire document at once) — it should just silently recompute and +// persist hashes under the current scheme. +func TestSweep_HashSchemeMigration_DoesNotFireReviewTest_JustRecomputesAndSaves(t *testing.T) { + req := reqTest("REQ-190", "migration safe") + runner := &fakeRunner{statuses: map[string]TestStatus{"REQ-190": TestPassed}} + issues := &fakeIssueClient{} + hashes := NewMemHashStore() + + // Seed the store as if it were written by an older hash scheme: some + // unrelated hash value, saved under version 1 (not HashSchemeVersion). + if err := hashes.Save(1, map[string]string{"REQ-190": "old-scheme-hash-unrelated-to-spec"}); err != nil { + t.Fatalf("seeding hash store: %v", err) + } + + results, err := Sweep([]Requirement{req}, SweepOptions{Runner: runner, Issues: issues, Hashes: hashes}) + if err != nil { + t.Fatalf("Sweep() error = %v", err) + } + if results[0].HashChanged { + t.Errorf("HashChanged = true on the first sweep after a hash-scheme version change; want false (migration, not a real change)") + } + if results[0].IssueCreated { + t.Errorf("review-test issue created on scheme-migration sweep: %+v", results[0]) + } + if len(issues.created) != 0 { + t.Errorf("expected no issues created on scheme-migration sweep, got %+v", issues.created) + } + + version, saved, err := hashes.Load() + if err != nil { + t.Fatalf("Load() after migration sweep: %v", err) + } + if version != HashSchemeVersion { + t.Errorf("stored version = %d after migration sweep, want %d", version, HashSchemeVersion) + } + if saved["REQ-190"] != req.Hash { + t.Errorf("stored hash = %q, want recomputed current-scheme hash %q", saved["REQ-190"], req.Hash) + } + + // A subsequent sweep with the identical requirement must now be a true + // no-op (no HashChanged), since the store has caught up to the current + // scheme. + results2, err := Sweep([]Requirement{req}, SweepOptions{Runner: runner, Issues: issues, Hashes: hashes}) + if err != nil { + t.Fatalf("Sweep() (2nd run) error = %v", err) + } + if results2[0].HashChanged { + t.Errorf("2nd run after migration: expected HashChanged = false") + } +} + +// TestREQ016_ReviewTestHashIgnoresImplementationStatus is the REQ-016 +// acceptance test (see REQUIREMENTS.md), matching the test_command naming +// convention (test_command runs "^TestREQ016" to judge REQ-016). It +// exercises every clause of REQ-016's 受け入れ条件 end-to-end via Sweep: +// resolving a review-test issue (実装状況 edit only) must not re-fire +// review-test, while a genuine 受け入れ条件 or verify-mode change must. +func TestREQ016_ReviewTestHashIgnoresImplementationStatus(t *testing.T) { + docV1 := `## REQ-300: 自己増殖しない +- 受け入れ条件: 元の条件文 +- verify: test +` + // "実装状況" is added, spec fields (受け入れ条件/verify) untouched: this + // simulates resolving a review-test issue. + docV1PlusStatus := `## REQ-300: 自己増殖しない +- 受け入れ条件: 元の条件文 +- verify: test +- 実装状況: 調査済み。テストは要件を正しく検証している。 +` + // A genuine spec change: 受け入れ条件 text itself changes. + docV2 := `## REQ-300: 自己増殖しない +- 受け入れ条件: 変更された条件文 +- verify: test +- 実装状況: 調査済み。テストは要件を正しく検証している。 +` + // A genuine spec change: verify mode flips to manual. + docV3 := `## REQ-300: 自己増殖しない +- 受け入れ条件: 変更された条件文 +- verify: manual +- 実装状況: 調査済み。テストは要件を正しく検証している。 +` + + runner := &fakeRunner{statuses: map[string]TestStatus{"REQ-300": TestPassed}} + issues := &fakeIssueClient{} + hashes := NewMemHashStore() + + mustParse := func(doc string) Requirement { + reqs, err := Parse(doc) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if len(reqs) != 1 { + t.Fatalf("expected 1 requirement, got %d", len(reqs)) + } + return reqs[0] + } + + // Sweep 1: baseline, no prior hash. + if _, err := Sweep([]Requirement{mustParse(docV1)}, SweepOptions{Runner: runner, Issues: issues, Hashes: hashes}); err != nil { + t.Fatalf("Sweep 1 error = %v", err) + } + if len(issues.created) != 0 { + t.Fatalf("Sweep 1: expected no issues, got %+v", issues.created) + } + + // Sweep 2: only 実装状況 added -> must NOT fire review-test. + res2, err := Sweep([]Requirement{mustParse(docV1PlusStatus)}, SweepOptions{Runner: runner, Issues: issues, Hashes: hashes}) + if err != nil { + t.Fatalf("Sweep 2 error = %v", err) + } + if res2[0].HashChanged || res2[0].IssueCreated { + t.Errorf("Sweep 2 (実装状況 only): HashChanged=%v IssueCreated=%v, want both false", res2[0].HashChanged, res2[0].IssueCreated) + } + + // Sweep 3: 受け入れ条件 changes -> must fire review-test. + res3, err := Sweep([]Requirement{mustParse(docV2)}, SweepOptions{Runner: runner, Issues: issues, Hashes: hashes}) + if err != nil { + t.Fatalf("Sweep 3 error = %v", err) + } + if !res3[0].HashChanged || !res3[0].IssueCreated || res3[0].IssueKind != KindReviewTest { + t.Errorf("Sweep 3 (受け入れ条件 changed): got %+v, want HashChanged/IssueCreated=true, IssueKind=%q", res3[0], KindReviewTest) + } + + // Sweep 4: verify flips test -> manual -> must fire review-test again. + res4, err := Sweep([]Requirement{mustParse(docV3)}, SweepOptions{Runner: runner, Issues: issues, Hashes: hashes}) + if err != nil { + t.Fatalf("Sweep 4 error = %v", err) + } + if !res4[0].HashChanged { + t.Errorf("Sweep 4 (verify test->manual): HashChanged = false, want true") + } +} + func TestSweep_MixedRequirements(t *testing.T) { reqs := []Requirement{ reqTest("REQ-100", "ok"),