Respect AlwaysValidateCommits everywhere

[myieye]

[Claude, autonomous]

Sync and AddManyChanges validated even with the flag off, so the perf tests and benchmarks that turn it off still timed a full commit-history scan.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EfCKXPsc7q812jbjhiXXi6

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 75b3aec5-4cc0-4449-8c7c-cca5724b77d6

📥 Commits

Reviewing files that changed from the base of the PR and between 0c2072b and 40ee699.

📒 Files selected for processing (2)

• src/SIL.Harmony/Config/HarmonyConfig.cs
• src/SIL.Harmony/DataModel.cs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Commit-history validation now defaults to disabled. AddManyChanges and ISyncable.AddRangeFromSync call ValidateCommits only when validation is enabled.

Changes

Commit validation

Layer / File(s)	Summary
Configure and apply conditional validation src/SIL.Harmony/Config/HarmonyConfig.cs, src/SIL.Harmony/DataModel.cs	AlwaysValidateCommits now defaults to false. The two data-model paths validate commit history only when AlwaysValidate is enabled.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Suggested reviewers: hahn-kev

Merge Risk: ⚪ Minimal · up to f0cb0

Local edits and sync imports no longer revalidate the full commit history by default, as intended. No concrete issue in the changed paths remains to resolve before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 40ee6

Commit-history checks become opt-in. Existing corruption can therefore remain undetected during later edits or syncs, although new commits are still linked and saved transactionally.

Retained concerns

• Medium · security · inferred: Default-configured local batch writes and sync imports can commit without detecting corruption in an existing commit-history prefix. Previously, both paths checked the whole history before committing.

Security review details

Security Blast Radius

• inferred — The weakened check applies to default-configured Harmony data stores receiving local batches or sync imports. Sync can bring commits from another model, but the evidence does not establish which peers are attacker-controlled or how they authenticate.

Security Findings and Attack Paths

• inferred — If an actor can corrupt stored commit history, a subsequent default-configured batch add or sync import no longer detects that corrupt prefix at the write boundary. This does not establish unauthenticated access or acceptance of a forged incoming hash: newly inserted commits are rehashed locally.

Trust Boundaries and Controls

• observed — The receiver deduplicates by commit ID, chooses a local predecessor without trusting the incoming parent hash, and rebuilds links for the affected range. Those controls remain, but they do not perform the skipped whole-history check.

Resilience and Maintainability Implications

• inferred — Locking and transactions limit duplicate or partial database writes, including when enabled validation throws before commit. They do not restore detection of a corrupt earlier commit when validation is disabled.

Hardening Proposals

• proposed — If whole-history checks remain opt-in for performance, define a separate integrity-check schedule or explicit check at applicable trust boundaries, and exercise default-off behavior alongside the enabled fixture.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: making all relevant paths respect AlwaysValidateCommits. It is concise and specific, although it does not mention the related default change.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}