Skip to content

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

Merged
ytnobody merged 1 commit into
developfrom
hermit/ytnobody/issue-182
Jul 28, 2026
Merged

fix(requirements): stop review-test self-reinforcing loop (Closes #182)#186
ytnobody merged 1 commit into
developfrom
hermit/ytnobody/issue-182

Conversation

@ytnobody

@ytnobody ytnobody commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • Fixes Issue review-test が自己増殖する — ハッシュ対象に実装状況が含まれ、解決作業自体が次の発火を確定させる #182: review-test was re-firing forever because Requirement.Hash was computed from the entire requirement block, which includes the - 実装状況: (implementation status) field. Resolving a review-test issue means writing findings into 実装状況, which changed the hash and made the next sweep think the requirement text had changed again — a closed self-reinforcing loop with zero real regression detections.
  • Requirement.Hash is now computed only from AcceptanceCriteria and Verify via a new specHash function (internal/requirements/requirements.go), excluding the header title, free-form description, and 実装状況.
  • HashStore (internal/requirements/hashstore.go) now carries a HashSchemeVersion (currently 2). Sweep (internal/requirements/sweep.go) treats a stored-version mismatch as "no change" for one sweep — it recomputes and persists hashes under the current scheme without firing review-test for the whole document, per the Issue's migration requirement. Pre-review-test が自己増殖する — ハッシュ対象に実装状況が含まれ、解決作業自体が次の発火を確定させる #182 on-disk hash files (a bare map, no version envelope) are detected and treated as version 0 so they go through the same migration path.
  • Also fixed a related gap surfaced while writing the acceptance test: flipping verify: testmanual never triggered review-test, because the verify:manual short-circuit ran before the hash comparison. Hash comparison (and the resulting review-test issue) now happens first; the manual short-circuit only skips test execution and implement/regression issue logic.

Assumptions (documented per Human Input Policy — no ambiguity was escalated to chat)

  • HashSchemeVersion starts at 2 (implicit prior/unversioned scheme is 1); legacy on-disk files with no version envelope are parsed as version 0 so they're always treated as "old scheme" regardless of exact prior value.
  • HashStore.Load/Save signatures changed to Load() (version int, hashes map[string]string, err error) / Save(version int, hashes map[string]string) error. This is an internal-package interface (HashStore, FileHashStore, memHashStore are all within internal/requirements), so no external API surface is affected.

Test plan

  • go test ./... passes
  • go vet ./... passes, gofmt -l . clean
  • New regression test TestSweep_ImplementationStatusOnlyChange_DoesNotFireReviewTest reproduces the exact review-test が自己増殖する — ハッシュ対象に実装状況が含まれ、解決作業自体が次の発火を確定させる #182 loop scenario (baseline sweep, then a sweep where only 実装状況 was added) and asserts review-test does not re-fire
  • New regression test TestSweep_HashSchemeMigration_DoesNotFireReviewTest_JustRecomputesAndSaves verifies the version-mismatch migration path fires no issues and persists hashes under the current scheme
  • New unit tests in requirements_test.go: TestParse_HashUnaffectedByImplementationStatusField, TestParse_HashUnaffectedByTitleOrDescriptionOnly, TestParse_HashChangesWithVerifyMode
  • TestREQ016_ReviewTestHashIgnoresImplementationStatus added per REQ-ID naming convention (internal/requirements/sweep_test.go), matches test_command's ^TestREQ016 pattern, and covers every clause of REQ-016's acceptance criteria end-to-end
  • REQUIREMENTS.md updated with new REQ-016

Closes #182

@ytnobody

Copy link
Copy Markdown
Owner Author

Superintendentによるレビュー(PR #186, Issue #182)

evaluate_risk の判定は HIGH(500行以上の変更)。自動マージ対象外のため、人間の判断を待ちます。以下、実質的なレビュー内容です。

差分の内容確認

  • 根本原因の特定は正確: internal/requirements/requirements.go の旧 Hash: hashText(block) が要件ブロック全体(実装状況行を含む)をハッシュ化していたため、review-test解決作業自体がハッシュを変化させ無限ループを生んでいた、という分析は妥当
  • 修正方針: specHash() を新設し、AcceptanceCriteriaVerify のみからハッシュを計算するよう変更。Issueの要求通り
  • 移行対応: HashStore インターフェースを Load() (version int, hashes map[string]string, err error) / Save(version int, hashes map[string]string) error に拡張し、HashSchemeVersion(現在値2)を導入。旧形式(バージョン無しの素の map)ファイルはversion 0として検出され後方互換が保たれている
  • Sweep はロードしたバージョンが現行と異なる場合、review-testを発火させずに再計算・保存のみ行う設計になっており、Issue本文の「移行時の注意」の要求(初回sweepで一斉発火させない)を満たしている
  • 実装中に追加で発見した関連バグ(verify:manual への切り替えチェックが元々ハッシュ比較より先に短絡し、testmanual切り替えでreview-testが発火しなかった問題)も修正し、受け入れ条件の該当項目を満たすようにしている。スコープ外に記載された「implement/regressionの判定ロジック変更」には踏み込んでおらず、範囲は適切
  • 回帰テスト(TestSweep_ImplementationStatusOnlyChange_DoesNotFireReviewTest)を含め、要求された受け入れ条件をカバーするテストが揃っている
  • CIは全て成功(Test/Lint/Security Scan)

懸念点

推奨

実装の正確性・テストカバレッジともに良好で、Issueの受け入れ条件を満たしています。人間による承認後のマージを推奨します。マージ時はREQ-015番号衝突の解消をお願いします。

 #182)

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.
@ytnobody
ytnobody force-pushed the hermit/ytnobody/issue-182 branch from bf11c52 to e9ab359 Compare July 28, 2026 02:52
@ytnobody

Copy link
Copy Markdown
Owner Author

⚠️ HERMIT: HIGH risk detected.
Reasons: [500 or more lines changed]

@ytnobody
ytnobody merged commit 908f80c into develop Jul 28, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

review-test が自己増殖する — ハッシュ対象に実装状況が含まれ、解決作業自体が次の発火を確定させる

1 participant