add_ids_to_model silently regenerates existing oifm_id when model is round-tripped through FindingModelBase

[talkasab]

Summary

Using findingmodel.tools.add_ids_to_model(FindingModelBase.model_validate(data), source) on an existing model that already has oifm_id / oifma_id / value_code silently strips and regenerates all of them. This breaks external references to those IDs.

Reproduction

import json
from findingmodel import FindingModelBase
from findingmodel.tools import add_ids_to_model

data = json.loads(open("defs/some_existing_model.fm.json").read())
assert data["oifm_id"] == "OIFM_MSFT_440456"  # existing, published ID

model = FindingModelBase.model_validate(data)  # silently drops oifm_id and all oifma_ids
model_full = add_ids_to_model(model, "MSFT")   # allocates brand-new IDs

assert model_full.oifm_id == "OIFM_MSFT_440456"  # FAILS — now e.g. "OIFM_MSFT_266428"

Root cause

FindingModelBase has no oifm_id field. model_validate(data) discards unknown keys by default, so the existing ID is gone before add_ids_to_model runs. add_ids_to_model's body checks if \"oifm_id\" not in finding_model_dict: — which is true because it was just stripped — and generates a new one. Same happens per-attribute for oifma_id.

Impact

This is a silent data-integrity hazard for any workflow that:

• Loads an existing .fm.json as a dict
• Needs to mutate it (e.g., add a missing attribute)
• Wants ID allocation for just the new part

We hit it in a review flow that needed to add a change from prior attribute to a legacy model that only had presence. The naive implementation looked correct but quietly regenerated the model's published OIFM ID and the existing attribute's OIFMA ID. External references to those IDs would have silently become dangling.

Workaround we shipped

Avoid FindingModelBase.model_validate / add_ids_to_model entirely for in-place mutation. Operate on the dict directly, and use Index().generate_attribute_id(model_oifm_id=..., source=...) to allocate only the new IDs needed. Build value_code from the oifma_id.N pattern directly. See scripts/finding_authoring/add_change_from_prior.py in findingmodels-headcts for the pattern we landed on.

Suggested fixes (any of these would help)

1. Raise if a pre-existing ID would be regenerated. add_ids_to_model could accept the original dict alongside the model, or the caller could indicate "these IDs are authoritative, do not touch." At minimum, detect the mismatch and raise.
2. Provide an in-place / dict-based API for the common case of "this model exists and has IDs; allocate new IDs only for items that literally don't have them." The existing function is framed around new model creation; the existing-model path has no safe helper.
3. Preserve oifm_id on FindingModelBase via Extra config, or provide a FindingModelWithIds intermediate class that model_validate won't strip.
4. Docstring warning. At minimum, the docstring should call out that FindingModelBase.model_validate(existing_dict_with_ids) strips those IDs, and recommend alternatives.

Happy to submit a PR if there's agreement on which direction to take.