feat(engine): add ContextualUserModel with interpolated Kneser-Ney scoring

[tianjianjiang]

Summary

Reworked from the original proposal, following the review direction on #779 ("work with what's in HEAD"): the model no longer hooks into the walk. It is a self-contained, persistent, context-sensitive successor to UserOverrideModel, applied through the existing overrideCandidate/selectOverrideUnigram machinery. ReadingGrid is untouched by this PR.

• observe()/suggest() are walk-based and signature-compatible with UserOverrideModel, including the three override cases (same-length, phrase-building with force-high-score, phrase-breaking based on the post-override walk) and punctuation/sentence-start context handling
• Scoring: absolute-discounting interpolated Kneser-Ney over bigram contexts; continuation probability is normalized per reading (Σ N₁₊ over that reading's candidates). A candidate confirmed in ≥2 distinct contexts generalizes to unseen contexts — the capability exact-key UOM lacks
• Persistence: atomic save (write-to-temp + rename); load validates every field, skips malformed lines with a reported count, rejects NaN/Inf/negative values
• No base-LM reference: with insufficient evidence suggest() returns empty and the walk falls back to base scores naturally (the lower KN backoff levels are implicit) — no aliasing-shared_ptr lifetime concerns by construction
• Wall-clock exponential decay (half-life seconds) + LRU capacity bound, mirroring UOM defaults

Review items from the previous iteration, and where they went

Item	Resolution
User-model score replaced edge weights instead of blending	Eliminated by design — no walk hook; overrides use existing soft-override scoring
Log-prob vs probability space mixing	Eliminated — model is internally probability-space only; API returns a candidate, not a score
KN continuation count normalization	Fixed — normalized per reading, not by total unique bigrams
Dangling userModel_ raw pointer in grid	Eliminated — grid holds no model pointer
Division by zero	Guarded — discounting only applies when total evidence ≥ discount
Silent load corruption	Fixed — per-field validation, skipped-line count surfaced via LoadStats
UTF-8 decomposition splitting	Removed — the decomposed backoff level no longer exists
No capacity/eviction	Fixed — LRU bound like UOM

Verification

• 11 new tests in ContextualUserModelTest.cpp (KN behavior, generalization threshold, decay, eviction, persistence round-trip, malformed-load, walk-based observe/suggest)
• Full McBopomofoLMLibTest suite passes; pbxproj lints OK
• Base: master (no dependency on other PRs)

KeyHandler integration (replacing UserOverrideModel and adding load/save wiring) follows in #781.

🤖 Generated with Claude Code

[claude]

Critical Issues Found

Reviewed the contextual user model implementation. Found several critical issues:

Priority 1 - Correctness Bugs

1. Log probability handling inconsistency (contextual_user_model.cpp:97) - Mixing probability and log-probability spaces
2. Score replacement logic error (walk_strategy.cpp:63) - User model unconditionally replaces edge weights instead of blending
3. Base score probability conversion (contextual_user_model.cpp:153) - Verify whether base LM returns probabilities or log-probabilities

Priority 2 - Robustness

4. UTF-8 parsing safety (contextual_user_model.cpp:250) - Potential buffer overflow with malformed input
5. Null pointer safety (reading_grid.cpp:194) - Missing error handling for failed overrides
6. Division by zero (contextual_user_model.cpp:107) - Constructor allows discount=0
7. Thread safety (contextual_user_model.cpp:35) - No synchronization for mutable state

The most critical is #1 (log probability) and #2 (score blending) as they directly affect the correctness of the Viterbi walk results.

[gemini-code-assist]

Code Review

This pull request introduces a ContextualUserModel to provide probabilistic scoring based on user history, which is a significant improvement over the previous heuristic-based approach. The implementation of the Kneser-Ney backoff model and its integration into the Viterbi walk is well-structured. I've found two significant issues that need to be addressed: a critical bug that disables the temporal decay feature, and a high-severity issue in the model's persistence logic that could lead to data corruption. My review includes detailed comments and suggestions to fix these problems.

[Copilot]

Pull request overview

Introduces a new contextual adaptation layer in the C++ engine by adding a ContextualUserModel and threading it into the existing Gramambular2 Viterbi walk pipeline so user evidence can influence path selection and surfaced node values.

Changes:

• Added ContextualUserModel (KN-style backoff + temporal decay) with persistence and explicit phrase support.
• Extended WalkStrategy::WalkInput and Viterbi relaxation to optionally consult the user model during scoring.
• Updated ReadingGrid to accept a user model pointer and apply post-walk “soft overrides”, plus added extensive unit/integration tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
Source/Engine/gramambular2/contextual_user_model.h	Declares the new contextual user model API and scoring/persistence helpers.
Source/Engine/gramambular2/contextual_user_model.cpp	Implements observation, KN backoff scoring, decay, and save/load.
Source/Engine/gramambular2/walk_strategy.h	Extends WalkInput to carry optional user model + timestamp.
Source/Engine/gramambular2/walk_strategy.cpp	Integrates user model suggestion scoring into Viterbi relaxation.
Source/Engine/gramambular2/reading_grid.h	Adds setUserModel() and tracking for user-model-applied overrides.
Source/Engine/gramambular2/reading_grid.cpp	Resets and applies post-walk user model overrides; threads user model into the walk input.
Source/Engine/gramambular2/reading_grid_test.cpp	Adds unit tests for ContextualUserModel and integrated walk behavior.
Source/Engine/gramambular2/CMakeLists.txt	Adds the new user model sources to the gramambular2 library target.

[claude]

Critical Issues Review

I've identified several critical issues that should be addressed:

High Priority

1. Numerical stability (contextual_user_model.cpp:99): The log conversion can produce -inf for very small probabilities, potentially breaking Viterbi score comparisons. Add finite-value guards.

2. Buffer overrun risk (contextual_user_model.cpp:268): UTF-8 decoding doesn't validate continuation bytes. Malformed UTF-8 input could cause out-of-bounds reads and crashes.

3. File I/O robustness (contextual_user_model.cpp:283): loadFromFile silently skips malformed lines, potentially loading corrupt/incomplete state with no error indication. Consider fail-fast or validation of loaded state consistency.

Medium Priority

4. Thread safety (reading_grid.cpp:191): Post-walk node modification lacks documentation about thread-safety guarantees. If walk() can be called concurrently or nodes are shared, this could cause data races.

5. Variable naming (walk_strategy.cpp:50): Using distance for maximization (not minimization) is confusing and error-prone for future maintainers.

Note

The Kneser-Ney continuation count logic (contextual_user_model.cpp:54-59) is actually correct as-is, but the implementation is subtle and could benefit from clarifying comments.

All other aspects of the implementation appear sound. The test coverage is comprehensive and the architecture integrates cleanly with the existing Viterbi walk.

[claude]

Critical Issues Found

Reviewed PR #780 focusing on correctness, bugs, and security. Found 6 critical/high-priority issues:

Correctness Issues

1. Continuation count logic error (contextual_user_model.cpp:53) - Breaks Kneser-Ney backoff by not counting distinct left contexts correctly
2. User model score unconditionally replaces node score (walk_strategy.cpp:63) - Contradicts stated priority (fixed spans > user model > base LM) and discards original scores entirely
3. Invalid UTF-8 handling (contextual_user_model.cpp:264) - Can cause incorrect character splits and wrong decomposition scores

Data Safety Issues

4. Silent data corruption during load (contextual_user_model.cpp:299) - Failed parse lines are silently skipped, no validation of data integrity
5. Unsafe raw pointer lifetime (reading_grid.cpp:277) - No lifetime validation for externally-set user model pointer, potential use-after-free

Performance

6. Redundant O(n) reset operations (reading_grid.cpp:42) - Nodes reset twice per walk cycle unnecessarily

The continuation count bug (#1) and unconditional score replacement (#2) are the most critical as they affect core algorithm correctness.

[claude]

Critical Issues Summary

Reviewed PR #780 focusing on potential bugs, correctness, and safety issues. Found several critical areas requiring attention:

Memory Safety

• Dangling pointer risk in ReadingGrid::userModel_ - raw pointer with no lifetime guarantees could cause use-after-free
• Unsafe pointer dereference in Relax() - missing null checks before accessing u->node and v->node

Arithmetic Safety

• Division by zero possible in bigramScore() when cTotal == 0 and discount_ == 0
• Division by zero potential in continuationScore() if totalUniqueBigrams_ becomes 0
• Integer overflow theoretical risk in observe() for long-running systems (counts use size_t)

Data Integrity

• Partial load corruption in loadFromFile() - returns true even when individual line parsing fails, leaving inconsistent state
• Partial write corruption in saveToFile() - doesn't check write success, could silently produce truncated files
• UTF-8 validation missing in splitValue() - malformed sequences could cause incorrect parsing

Concurrency (Future Risk)

• Race condition on totalUniqueBigrams_ if class is ever used multi-threaded - no synchronization or thread-safety documentation

All issues have inline comments with suggested fixes.

[claude]

Critical Issues Found

Reviewed focusing on bugs, correctness, and performance. Found 7 critical issues:

Bugs (must fix):

1. Division by zero in bigramScore() - contextual_user_model.cpp:128
2. Incorrect continuation count reconstruction in loadFromFile() - breaks save/load round-trip
3. Node override tracking bug in post-walk processing - reading_grid.cpp:196

Correctness issues:

4. Missing data validation in loadFromFile() - allows NaN/Inf/negative values
5. Negative elapsed time not properly handled when timestamps go backward
6. UTF-8 validation missing in splitValue() - can produce wrong character counts

Performance concern:

7. Quadratic post-walk overhead for user model suggestions on long sentences

All issues have inline comments with specific line references and suggested fixes.

[claude]

Critical Issues Found

Reviewed PR #780 focusing on correctness and potential bugs. Found 6 critical issues that need attention:

High Priority

1. Viterbi score replacement bug (walk_strategy.cpp:48) - User model score completely replaces base LM weight instead of blending, breaking the algorithm's probability space
2. Log-probability conversion error (contextual_user_model.cpp:99) - Applying log to already-linear probability produces incorrect scores
3. Post-walk override bypasses fixed spans (reading_grid.cpp:196) - User model can override structural constraints

Medium Priority

4. Division by zero risk (contextual_user_model.cpp:112) - No validation for discount_ before division
5. UTF-8 validation missing (contextual_user_model.cpp:252) - Malformed input causes incorrect parsing
6. Incomplete error handling (contextual_user_model.cpp:290) - loadFromFile returns success on empty/invalid files

All issues have suggested fixes in inline comments.

[claude]

Critical Issues Summary

Found 3 critical correctness/functionality issues and 5 additional concerns:

Critical:

1. Hardcoded timestamp (reading_grid.cpp:173) - Temporal decay is completely disabled in production. All user model observations are treated as fresh, defeating the decay mechanism that tests verify works.

2. Score replacement vs. blending (walk_strategy.cpp:63) - Code replaces base LM scores entirely instead of blending. Description claims "blending" but implementation does full replacement, potentially losing structural information.

3. Duplicate reset logic (reading_grid.cpp:173-177) - Override reset happens both in clear() and walk(), but mutations between walks could leave stale overrides if grid isn't cleared.

High Priority:

4. Memory safety concern (reading_grid.cpp:42-45) - userModelOverriddenNodes_ could contain dangling pointers if grid modifications occur between walks.

5. Performance O(n²) (contextual_user_model.cpp:67-82) - Double iteration in suggest() creates redundant scoring calls.

Documentation/Robustness:

6. File I/O lacks bounds checking (contextual_user_model.cpp:295)
7. Missing documentation on continuation count invariant (contextual_user_model.cpp:112)
8. Decomposition assumptions undocumented (contextual_user_model.cpp:165)

The timestamp issue is the most critical - it needs a real timestamp from the caller to make temporal decay functional in production.

[claude]

Critical Issues Found

I've identified several critical bugs that need to be addressed before merging:

1. Bigram counting logic error (contextual_user_model.cpp:48-58)

The observe() method incorrectly tracks unique bigrams and continuation counts. When the same (reading, value) appears in a new context, it updates the existing entry but fails to:

• Increment totalUniqueBigrams_ for the NEW bigram key
• Correctly track continuation counts across contexts

2. Incorrect Kneser-Ney implementation (contextual_user_model.cpp:142)

The continuation score uses totalUniqueBigrams_ as denominator, but KN requires the sum of continuation counts. This breaks probability normalization.

3. User model score replaces instead of combines (walk_strategy.cpp:63)

The Viterbi relaxation replaces the node's score with the user model score, completely discarding the base LM. This creates discontinuities and doesn't properly interpolate between models.

4. UTF-8 decoding lacks validation (contextual_user_model.cpp:254-265)

The splitValue() function doesn't validate UTF-8 byte sequences, risking buffer overruns and silent corruption on invalid input.

5. Double user model lookup inefficiency (reading_grid.cpp:186-199)

User model is queried during Viterbi walk AND in post-walk override pass, doubling the cost for each node.

Please address these issues, particularly #1-3 which affect correctness.

[claude]

Critical Issues Found

This PR introduces a sophisticated contextual user model with Kneser-Ney backoff, but there are 8 critical issues that need addressing:

High Priority

1. Score replacement vs blending (walk_strategy.cpp:63) - User model score completely replaces LM score instead of blending, breaking probabilistic composition
2. Division by zero (contextual_user_model.cpp:125) - Missing check for cTotal == 0.0 before division
3. Incorrect totalUniqueBigrams_ after load (contextual_user_model.cpp:290) - Counts all bigram entries instead of unique (reading, value) pairs, breaking continuation scores

Medium Priority

4. UTF-8 validation (contextual_user_model.cpp:252) - splitValue doesn't validate multibyte sequences, potential out-of-bounds read
5. Dangling pointer risk (reading_grid.cpp:201) - Raw pointer to ContextualUserModel without lifetime guarantees
6. Redundant post-walk overrides (reading_grid.cpp:186) - Double application of user preferences may cause inconsistency

Low Priority

7. Integer overflow (contextual_user_model.cpp:51) - totalUniqueBigrams_ unbounded
8. Linear search performance (contextual_user_model.cpp:151) - O(n) lookup per baseScore call

The most critical issue is #1 (score replacement), as it fundamentally changes the intended probabilistic model behavior.

[claude]

Critical Issues Found

Found 7 critical issues requiring attention:

Correctness & Logic Errors:

1. walk_strategy.cpp:48-64 - User model score unconditionally replaces base LM score, discarding probabilistic information
2. contextual_user_model.cpp:99 - Log of zero produces -infinity instead of floor value
3. reading_grid.cpp:186-201 - Stale override nodes not properly reset between walks

Data Integrity:
4. contextual_user_model.cpp:283 - loadFromFile() corrupts model state on partial load failure
5. contextual_user_model.cpp:267 - saveToFile() doesn't verify write operations succeed

Robustness:
6. contextual_user_model.cpp:34 - Temporal decay calculation vulnerable to extreme elapsed times and negative values
7. contextual_user_model.cpp:38 - observe() has potential race conditions if called from multiple threads

All issues have suggested fixes in inline comments. The most critical are #1 (affects scoring correctness) and #4 (data corruption risk).

[github-actions]

Claude Code Review Failed

The automated Claude review encountered an error and could not complete. You can:

• Check the workflow logs for details
• Trigger a manual review by commenting @claude on this PR
• The review will be retried automatically on the next push

This does not affect the PR approval process.

[github-actions]

Claude Code Review Failed

The automated Claude review encountered an error and could not complete. You can:

• Check the workflow logs for details
• Trigger a manual review by commenting @claude on this PR
• The review will be retried automatically on the next push

This does not affect the PR approval process.

[github-actions]

Claude Code Review Failed

The automated Claude review encountered an error and could not complete. You can:

• Check the workflow logs for details
• Trigger a manual review by commenting @claude on this PR
• The review will be retried automatically on the next push

This does not affect the PR approval process.

[github-actions]

Claude Code Review Failed

The automated Claude review encountered an error and could not complete. You can:

• Check the workflow logs for details
• Trigger a manual review by commenting @claude on this PR
• The review will be retried automatically on the next push

This does not affect the PR approval process.

[github-actions]

Claude Code Review Failed

The automated Claude review encountered an error and could not complete. You can:

• Check the workflow logs for details
• Trigger a manual review by commenting @claude on this PR
• The review will be retried automatically on the next push

This does not affect the PR approval process.

[claude]

Two correctness bugs in the new ContextualUserModel:

1. continuationFor() ignores temporal decay — the raw stats.count > 0 guard means any observation, no matter how old, permanently contributes to cross-context generalization. Fix: pass timestamp into continuationFor() and use decayedCount(stats, timestamp) > 0.

2. serialize() truncates timestamps to 6 significant digits — the default ostringstream precision silently rounds a 9-digit Unix timestamp down by up to ~1000 seconds, corrupting the decay calculation on every restart. Fix: set std::setprecision(std::numeric_limits<double>::max_digits10) before writing numeric fields, and add <iomanip>/<limits> includes.

Both issues have inline suggestions attached.

[claude]

Bug: continuationFor() ignores temporal decay — stale observations permanently inflate generalization

stats.count is the stored value, not the decayed value. A candidate observed once long ago (raw count = 1.0) will still pass this check even after its effective probability has decayed to near zero, so it permanently counts as a valid distinct context for generalization. This means candidates that would otherwise be excluded by the kMinSuggestionProbability threshold at the exact-context level continue to trigger cross-context generalization indefinitely.

continuationFor() needs to receive the current timestamp and use decayedCount(stats, timestamp):

Suggested change

	if (stats.count > 0) {
	if (decayedCount(stats, timestamp) > 0) {

The method signature (declaration + definition) also needs the timestamp parameter:

// in header:
[[nodiscard]] Continuation continuationFor(const std::string& reading, double timestamp) const;

// call sites in suggest():
Continuation continuation = continuationFor(reading, timestamp);

[claude]

Bug: default ostringstream precision silently corrupts timestamps on save/load

std::ostringstream uses 6 significant digits by default (equivalent to %g). A Unix timestamp like 1657772432 (9 digits) is written as 1.65777e+09 = 1657770000, losing ~2432 seconds. After a round-trip through saveToFile/loadFromFile, every stored timestamp is off by up to hundreds of seconds. Since decay is computed as count * exp2(-elapsed / halfLife), a 2000-second error with a 5400-second half-life changes the decayed weight by ~exp2(-2000/5400) ≈ 0.77×, enough to alter suggestions immediately after startup.

Fix by setting maximum precision before the numeric fields:

Suggested change

	out << context << "\t" << reading << "\t" << candidate << "\t"
	out << std::setprecision(std::numeric_limits<double>::max_digits10)
	<< context << "\t" << reading << "\t" << candidate << "\t"
	<< stats.count << "\t" << stats.timestamp << "\t"

Also add #include <iomanip> and #include <limits> to the include block at the top of the file.