feat: replace UserOverrideModel with ContextualUserModel in KeyHandler

[tianjianjiang]

Summary

Stacked on #780. This PR makes the replacement of UserOverrideModel explicit: ContextualUserModel becomes the user adaptation mechanism. Because the two models expose the same walk-based observe/suggest interface, the diff is small and touches only the existing call sites — the suggestion at insert time still applies through overrideCandidate() + re-walk, and the observation in fixNodeWithReading still uses the walk captured before the override (so the recorded context is what the user actually saw — no left-context timing issue).

Behavior changes:

• Learning persists across restarts — loaded once at startup (AppDelegate), saved after each observed selection
• Suggestions generalize to unseen contexts once a candidate is confirmed in ≥2 distinct contexts
• Plain Bopomofo no longer feeds the model — persistence makes single-character selections there pollution

Review items from the previous iteration

Item	Resolution
Aliasing shared_ptr wrapping a stack global (UB / use-after-free)	Gone — the model holds no base-LM reference; plain static object like gUserOverrideModel was
Blocking file save on the main thread per selection	Saves snapshot via serialize() on the input thread (µs), then write atomically on a serial background queue
Left context captured after fixSpan()/walk	Not applicable — observation uses prevWalk captured before the re-walk, same as UOM semantics
Missing file I/O error handling	Load logs unreadable files and reports skipped malformed-line counts; save logs write failures
Iterator underflow / unchecked optional	Not applicable — that code path (fixSpan flow) no longer exists; existing findNodeAt guards retained
Unrelated upstream changes in the diff	Rebuilt from a clean base; diff is 5 files, +69/−13
Dual-model non-determinism	Only ContextualUserModel is wired; UserOverrideModel sources stay in tree but nothing calls them (removal deferred until baked)

Data safety

The model reads/writes only contextual-user-model.txt in the user data folder. User phrase files are untouched. UserOverrideModel was in-memory only, so there is no legacy learned data to migrate.

Verification

• xcodebuild build: BUILD SUCCEEDED (the previous iteration's CI failure does not reproduce)
• xcodebuild test: all suites pass (24 cases, 0 failures — KeyHandlerBopomofo, KeyHandlerPlainBopomofo, UTF8Helper)
• Engine suite: 12 ContextualUserModel tests pass (feat(engine): add ContextualUserModel with interpolated Kneser-Ney scoring #780)

🤖 Generated with Claude Code

[claude]

Critical Issues Found

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

Memory Safety

1. Dangling pointer risk (LanguageModelManager.mm:40) - Aliasing shared_ptr wrapping stack object creates unsafe lifetime semantics
2. Iterator underflow (KeyHandler.mm:211) - Undefined behavior when accessing nodeIter - 1 at container begin
3. Unchecked optional (KeyHandler.mm:192) - nodeOpt may be empty but is used without validation

Performance & Reliability

4. Blocking I/O on main thread (KeyHandler.mm:219) - Synchronous file save on every selection will cause UI lag
5. Missing error handling (LanguageModelManager.mm:83) - File I/O failures are silently ignored

These issues affect correctness, crash safety, and user experience. The memory safety issues in particular need immediate attention.

[gemini-code-assist]

Code Review

This pull request effectively integrates the new ContextualUserModel into the KeyHandler, simplifying the adaptation logic by replacing the two-walk override mechanism with a more efficient single-walk approach. The changes are well-structured and align with the goals outlined in the description.

My review focuses on two main areas of concern, which are also noted as future work in the PR description but are important enough to highlight as high-severity issues:

1. Synchronous I/O: The user model is saved synchronously on every selection, which can block the main thread and impact UI responsiveness.
2. Thread Safety: The global language models are accessed without synchronization, creating a risk of race conditions and data corruption in a multi-threaded environment.

Addressing these will be critical for ensuring the stability and performance of the input method.

[Copilot]

Pull request overview

Integrates the C++ ContextualUserModel into the macOS input method plumbing (via LanguageModelManager and KeyHandler) so user selections can be observed, persisted, and influence subsequent Viterbi walks without the prior post-walk override flow.

Changes:

• Add a global ContextualUserModel instance in LanguageModelManager, load it on startup, and provide save/load accessors.
• Wire the user model into KeyHandler’s ReadingGrid via setUserModel(), and record selections via observe() + immediate persistence.
• Update engine tests around fixed-span clearing behavior; project file updates to include new engine sources and adjust build settings.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
Source/LanguageModelManager.mm	Creates/loads/saves a global ContextualUserModel and exposes it to ObjC++ callers.
Source/LanguageModelManager+Privates.h	Declares private accessors for the contextual user model and its persistence path.
Source/KeyHandler.mm	Sets the contextual model on the grid and records user selections; also includes unrelated state-machine refactors.
Source/Engine/gramambular2/reading_grid_test.cpp	Adds a regression test ensuring ReadingGrid::clear() resets fixed spans.
McBopomofo.xcodeproj/project.pbxproj	Adds new engine sources to the Xcode build and modifies resources/deployment targets.

Comments suppressed due to low confidence (1)

Source/KeyHandler.mm:499

• The “tone-marker-only changes prior reading” logic gated by Preferences.allowChangingPriorTone (Issue 753) was removed from this method. The preference still exists, but there is no remaining code path that applies it, so enabling the setting will no longer have any effect and the feature regresses.

    BOOL composeReading = isValidKey && _bpmfReadingBuffer->hasToneMarker() && !_bpmfReadingBuffer->hasToneMarkerOnly();

    // see if we have composition if Enter/Space is hit and buffer is not empty
    // this is bit-OR'ed so that the tone marker key is also taken into account
    composeReading |= (!_bpmfReadingBuffer->isEmpty() && (charCode == 32 || charCode == 13));

[claude]

Critical Issues

Found 5 critical/high-priority issues that should be addressed:

1. Thread safety - Global gContextualUserModel accessed without synchronization (data race risk)
2. Performance - Synchronous file I/O on every candidate selection (UI jank on slow storage)
3. Memory safety - Aliasing shared_ptr to stack-allocated global creates fragile lifetime dependency
4. Correctness - Optional dereferencing in fixNodeWithReading needs null check
5. Robustness - Silent failure on file I/O errors loses user data

See inline comments for details and suggested fixes.

[claude]

Critical Issues

Three critical issues that should be addressed before merge:

1. Stack object lifetime with shared_ptr alias (LanguageModelManager.mm:37-40) - Potential use-after-free if destruction order is wrong
2. Synchronous I/O on main thread (KeyHandler.mm:219) - Blocks UI on every candidate selection, causing user-visible lag
3. Thread safety violation (KeyHandler.mm:219) - If save becomes async, concurrent access to gContextualUserModel from multiple threads/contexts will corrupt state

The synchronous I/O issue is marked as "future work" in the PR description but represents a UX regression that users will experience immediately. At minimum, the save should be dispatched to a background queue with proper synchronization.

Additional concerns about null pointer handling and failed override observation logic warrant verification but may be correct as-is depending on API contracts.

[claude]

Critical Issues

1. Left context extraction timing (Source/KeyHandler.mm:210)
The left context is extracted from _latestWalk after fixSpan() and _walk(), but it should capture the state before the user's selection. This may pass incorrect context to observe() if the structural fix changes the previous node.

2. Synchronous I/O on main thread (Source/LanguageModelManager.mm:455)
saveContextualUserModel performs synchronous disk I/O on the main thread during every candidate selection, causing input lag on slow disks. Additionally, there's no thread safety for gContextualUserModel, blocking future async save work.

3. Missing file I/O error handling (Source/KeyHandler.mm:219)
saveContextualUserModel can silently fail (disk full, permissions), causing unnoticed user data loss. Needs at minimum error logging, ideally retry or user notification.

[claude]

Critical Issues

Identified 5 critical issues requiring attention before merge:

1. Global initialization order UB (LanguageModelManager.mm:37-40): gContextualUserModel initialization with aliasing shared_ptr has undefined behavior due to C++ global initialization order rules

2. Synchronous I/O on main thread (LanguageModelManager.mm:455): File save after every selection will cause UI stuttering

3. Potential stale pointer issue (KeyHandler.mm:195): fixSpan may receive invalidated node pointer after modifications

4. Silent learning failure (KeyHandler.mm:186): Early return on override failure bypasses context recording

5. Thread safety (KeyHandler.mm:217): Unprotected concurrent access to global model from multiple call sites

The global initialization order issue is a correctness/safety bug that could cause crashes. The synchronous I/O issue will impact user experience noticeably during typing.

[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]

Critical Issues Summary

High Priority:

1. Synchronous I/O on main thread (LanguageModelManager.mm:453) - Will cause UI freezes on every candidate selection
2. Aliasing shared_ptr lifetime issue (LanguageModelManager.mm:38) - Potential use-after-free during static destruction
3. OverrideType mismatch (KeyHandler.mm:184) - May prevent fixSpan from finding the overridden node

Medium Priority:
4. Silent failure in fixSpan (KeyHandler.mm:199) - No error handling if node lookup fails
5. Confusing control flow (KeyHandler.mm:209) - Redundant nullptr check with unclear intent

The synchronous I/O issue is the most user-visible problem and should be addressed before merge.

[claude]

Critical Issues

1. Synchronous I/O blocking main thread (LanguageModelManager.mm:455) - saveContextualUserModel performs file I/O on the main thread during every candidate selection, risking UI freezes

2. Thread safety (LanguageModelManager.mm:40) - Global gContextualUserModel lacks synchronization for concurrent observe()/saveToFile() calls

3. Value matching bug (KeyHandler.mm:189) - findInSpan predicate should check both reading AND value to avoid fixing the wrong node when multiple overridden nodes share the same reading

4. Aliasing shared_ptr fragility (KeyHandler.mm:65) - The gEmptySharedPtr pattern creates a dangling pointer risk if refactored

Recommendations

• Add value check to the findInSpan predicate (see inline suggestion)
• Consider deferring or batching file saves to avoid blocking input
• Add synchronization for the global contextual user model
• Document the lifetime dependency between gLmPtr and gLanguageModelMcBopomofo

[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]

Three critical issues found in this PR:

[claude]

Three issues found, ordered by severity:

1. observe() called when fixSpan was skipped (KeyHandler.mm ~line 217) — correctness bug introduced by this PR. If findInSpan doesn't find the overridden node, fixSpan is silently omitted but observe() still fires, recording a preference the walk isn't constrained to honor.

2. Aliasing shared_ptr with null owner (LanguageModelManager.mm ~line 39) — gLmPtr has use_count() == 0 and any weak_ptr derived from it would be immediately expired. Safe only if ContextualUserModel never constructs weak_ptrs or inspects use_count().

3. Missing setCursor on cend early return (KeyHandler.mm ~line 202) — pre-existing gap made more visible now that the nullptr branch below it explicitly resets the cursor. Minor but worth fixing for consistency.

[claude]

Four issues found, one of which is a correctness bug:

1. Logic error (KeyHandler.mm): _contextualUserModel->observe() is called for all input modes including plain Bopomofo, but the user model is only installed on the grid for InputModeBopomofo. This causes plain Bopomofo selections to train predictions that influence only Bopomofo mode — a regression from the old code which explicitly guarded if (_inputMode != InputModePlainBopomofo).

2. Sentinel inconsistency (KeyHandler.mm): For sentence-initial position, leftReading is set to kStartSentinel but leftValue is left as "". Both should use the same sentinel to ensure consistent bigram key construction in observe() and suggest().

3. Null dereference in test (reading_grid_test.cpp): grid.spans()[6].nodeOf(2) return value is used immediately without a null check.

4. Non-standard shared_ptr construction (LanguageModelManager.mm): The aliasing-from-null pattern (shared_ptr(emptySharedPtr, rawPtr)) produces a shared_ptr where get() != nullptr but use_count() == 0, which is a subtle invariant violation. A no-op deleter is clearer and safer.

[claude]

Four issues found, two of which are critical for correctness/UX:

1. Aliasing shared_ptr with null control block (LanguageModelManager.mm:37) — produces a shared_ptr with use_count()==0 and a valid stored pointer, which violates normal shared_ptr invariants. A no-op-deleter shared_ptr is semantically cleaner.

2. Synchronous file I/O on the main thread (KeyHandler.mm:220) — blocks the entire IME input pipeline on every candidate selection. Should be dispatched to a background queue.

3. Missing null-guard in new test (reading_grid_test.cpp:990) — nodeOf() can return null; dereferencing without checking is UB.

4. User model applied to plain Bopomofo walks without a mode guard (KeyHandler.mm:143) — the standard-mode model's suggestions flow into plain Bopomofo grid walks even though observations are correctly mode-gated.

[claude]

Synchronous I/O on the main thread on every candidate selection.

saveContextualUserModel calls gContextualUserModel.saveToFile(...) synchronously. For an input method that must remain responsive at all times, a blocking filesystem write on every selection will cause perceptible UI stalls whenever the system is under I/O pressure or when the model file grows large.

At minimum, dispatch the save to a background queue:

Suggested change

	[LanguageModelManager saveContextualUserModel];
	_contextualUserModel->observe(leftReading, leftValue,
	currentNode->reading(), currentNode->value(),
	[NSDate date].timeIntervalSince1970);
	dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
	[LanguageModelManager saveContextualUserModel];
	});

(A coalescing/debounce approach would be even better, but async dispatch already prevents main-thread blocking.)

[claude]

Silent fixSpan no-op when overrideCandidate succeeds but findInSpan returns nothing.

overrideCandidate uses cursor-position semantics to locate and override a node, but findInSpan may use span-start-position semantics. If actualCursor lands in the middle of a multi-syllable candidate span, findInSpan could return nullopt even though the override succeeded, silently skipping fixSpan. The walk then uses only the soft override (not the structural pin), so subsequent keystrokes can un-fix the selection.

If overrideCandidate succeeded there must always be a matching overridden node at that cursor. An assertion here would catch the mismatch early:

Suggested change

	auto nodeOpt = _grid->findInSpan(actualCursor, [&readingStr](const auto& node) {
	auto nodeOpt = _grid->findInSpan(actualCursor, [&readingStr](const auto& node) {
	return node->reading() == readingStr && node->isOverridden();
	});
	// findInSpan must find the node we just overrode; a miss indicates a
	// cursor-position mismatch between overrideCandidate and findInSpan.
	NSCAssert(nodeOpt.has_value(), @"fixNodeWithReading: overrideCandidate succeeded but findInSpan found no overridden node at cursor %zu", actualCursor);
	if (nodeOpt) {
	_grid->fixSpan(actualCursor, *nodeOpt);
	}

[claude]

Aliasing shared_ptr from an empty shared_ptr produces use_count() == 0.

The aliasing constructor shared_ptr<T>(r, ptr) shares the control block of r. Since gEmptySharedPtr has no control block, gLmPtr.use_count() is 0 even though gLmPtr.get() is non-null. Any future code in ContextualUserModel or its callers that checks use_count() >= 1 or calls unique() will see incorrect values and could draw wrong conclusions (e.g., thinking the LM is unowned and safe to delete).

A no-op-deleter shared_ptr gives the conventional use_count() == 1 and is clearer about intent:

Suggested change

	static std::shared_ptr<Formosa::Gramambular2::LanguageModel> gLmPtr(gEmptySharedPtr, &gLanguageModelMcBopomofo);
	static std::shared_ptr<Formosa::Gramambular2::LanguageModel> gLmPtr(
	&gLanguageModelMcBopomofo, [](Formosa::Gramambular2::LanguageModel*) {});
	static Formosa::Gramambular2::ContextualUserModel gContextualUserModel(gLmPtr);

(gEmptySharedPtr can then be removed.)

[claude]

Aliasing shared_ptr with null control block — gLmPtr is constructed via the aliasing constructor from a default-constructed (null) gEmptySharedPtr. This produces a shared_ptr where use_count() == 0 and get() != nullptr — semantics that differ from every normal shared_ptr and are easy to misuse. If ContextualUserModel ever stores this shared_ptr and any caller later checks use_count() or assumes reference counting is active, it will silently misbehave.

A no-op-deleter avoids the footgun while still preventing deletion of the stack-allocated object:

Suggested change

	// Aliasing shared_ptr: wraps the stack-allocated LM for ContextualUserModel.
	static std::shared_ptr<Formosa::Gramambular2::LanguageModel> gLmPtr(&gLanguageModelMcBopomofo, [](Formosa::Gramambular2::LanguageModel*){});

[claude]

Synchronous file I/O on the main thread on every candidate selection — saveContextualUserModel writes to disk synchronously during key event processing. For an IME this blocks the text input pipeline on every candidate pick, causing perceptible lag especially on slower or busier systems.

The PR description acknowledges this as deferred future work, but it should be tracked explicitly. At minimum, consider dispatching the save to a background queue:

Suggested change

	[NSDate date].timeIntervalSince1970);
	dispatch_async(dispatch_get_global_queue(QOS_CLASS_UTILITY, 0), ^{
	[LanguageModelManager saveContextualUserModel];
	});

(If saveToFile is not thread-safe, a dedicated serial queue is needed instead.)

[claude]

Missing null-guard on nodeOf() return — nodeOf(2) can return nullptr if the span at index 6 has no 2-syllable node. Dereferencing it immediately after is undefined behaviour and will crash the test process. Other tests in this file guard with EXPECT_TRUE(nzNode != nullptr) before use; this one should too.

Suggested change

	auto result = grid.walk();
	auto nzNode = grid.spans()[6].nodeOf(2);
	ASSERT_TRUE(nzNode != nullptr);
	nzNode->selectOverrideUnigram(

[claude]

User model applied to plain Bopomofo walks — setUserModel(_contextualUserModel) is called unconditionally in -init, so the global ContextualUserModel (trained exclusively on standard Bopomofo selections) also influences plain Bopomofo grid walks. Observations are correctly gated (_inputMode != InputModePlainBopomofo), but suggestions from the standard-mode model can still surface in plain-mode walks. Consider guarding the setUserModel call as well, or using a separate model instance per mode.

[claude]

The substitution of ContextualUserModel for UserOverrideModel is clean and the thread-safety reasoning in the comments is correct. One critical performance issue found: saveContextualUserModel enqueues a new serialize+write on every candidate selection with no coalescing, which could queue up redundant writes under rapid input. See inline comment for a simple fix with an atomic flag.

[claude]

Redundant queued writes under rapid input. saveContextualUserModel is called on every confirmed candidate selection. Each call calls serialize() on the input thread and unconditionally queues a disk write, even when a previous write hasn't completed yet. Under fast candidate navigation, multiple identical (or near-identical) snapshots pile up in the serial queue — all but the last are wasted work.

A simple fix is an atomic "dirty" flag so only one write is ever pending at a time:

Suggested change

	});
	dispatch_async(LTContextualUserModelQueue(), ^{
	NSData *data = [NSData dataWithBytes:snapshot.data() length:snapshot.size()];
	NSError *error = nil;
	if (![data writeToFile:path options:NSDataWritingAtomic error:&error]) {
	NSLog(@"Error: Could not save contextual user model to %@: %@", path, error);
	}
	});
	}

Suggested replacement for the whole method:

static std::atomic<bool> gContextualUserModelSavePending{false};

+ (void)saveContextualUserModel
{
    // Only snapshot and enqueue if no write is already pending.
    bool expected = false;
    if (!gContextualUserModelSavePending.compare_exchange_strong(expected, true)) {
        return;
    }
    std::string snapshot = gContextualUserModel.serialize();
    NSString *path = [self contextualUserModelDataPath];
    dispatch_async(LTContextualUserModelQueue(), ^{
        gContextualUserModelSavePending.store(false);
        NSData *data = [NSData dataWithBytes:snapshot.data() length:snapshot.size()];
        NSError *error = nil;
        if (![data writeToFile:path options:NSDataWritingAtomic error:&error]) {
            NSLog(@"Error: Could not save contextual user model to %@: %@", path, error);
        }
    });
}

This ensures at most one snapshot + write is ever in flight. The tradeoff is that observations made between enqueue and write completion aren't persisted immediately — but those will be captured by the next save triggered by the next selection.