Skip to content

[patch] Stop the merge-strategy cache letting one call decide a key for every later call - #155

Merged
matt-edmondson merged 2 commits into
mainfrom
fix/130-merge-cache-per-strategy
Sep 28, 2026
Merged

matt-edmondson merged 2 commits into
mainfrom
fix/130-merge-cache-per-strategy

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #130

Problem

PropertyMerger.PropertyMergeCache was a process-wide cache keyed only on the property name. But a key's canonical name also depends on the strategy and, for Aggressive and Maximum, on the other keys in the same document. So the first call to reach a key decided how every later call treated it:

  • After an Aggressive run, a Conservative run dropped custom_title.
  • After a Conservative run, an Aggressive run stopped merging.
  • A key merged in one document was renamed in a later document that had nothing to merge it with.

Change

  • Cache removed: GetCanonicalName works out the name on every call. The work is a dictionary lookup plus a small normalization over a handful of keys, which is the lower-risk option the triage recommended.
  • Test hack removed: PropertyMergerTests, SingleCharacterKeyTests and DecorationOnlyKeyTests no longer clear the cache by reflection. That clearing was what hid the bug.

Tests

  • Added MergeStrategyIsolationTests, which runs strategies and documents back to back without clearing anything. It covers:
    • Aggressive then Conservative: the acceptance case from the issue, checked at both the merger level and the CombineFrontmatter level
    • Conservative then Aggressive
    • a key merged in one document, then appearing alone in a later one
  • With PropertyMerger.cs reverted, 3 of the 4 new tests fail. The cross-document test passes either way.
  • With the fix, all 188 tests pass on net10.0.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KrGyxYxnoFgAkP1ENResCJ


Generated by Claude Code

…or every later call

PropertyMerger cached each key's canonical name process-wide, keyed on the
property name alone. The name also depends on the strategy and, for
Aggressive and Maximum, on the other keys in the document, so an Aggressive
run made a later Conservative run drop custom_title, and a key merged in one
document was renamed in another that had nothing to merge it with.

Drop the cache: the per-call work is a dictionary lookup and a small
normalization over a handful of keys. The tests no longer clear it by
reflection, and new tests run the strategies back to back without clearing
anything.

Fixes #130

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KrGyxYxnoFgAkP1ENResCJ
Comment thread Frontmatter.Test/MergeStrategyIsolationTests.cs Fixed
Comment thread Frontmatter.Test/MergeStrategyIsolationTests.cs Fixed
@sonarqubecloud

Copy link
Copy Markdown

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.

Merge-strategy cache is keyed only on the property name, so an Aggressive run makes a later Conservative run drop properties

1 participant