fix(experiments): preserve run metadata precedence

[marliessophie]

What changed

• Add one regression test covering conflicting run and item metadata.
• Merge item metadata first, then run metadata, with reserved experiment fields taking precedence.

Why

Item metadata could overwrite run-level metadata on the experiment item span, including experiment_run_name. Run metadata now takes precedence while item-only metadata is preserved.

Validation

Local tests and lint were not run by request; CI should validate the targeted regression test and SDK checks.

Refs LFE-8595

Greptile Summary

This PR fixes a metadata precedence bug in run_experiment: item-level metadata was applied last (via .update()), allowing it to silently clobber experiment_run_name and other reserved fields set by the run. The fix merges metadata in the correct order — item → run → reserved — inside a single dict literal.

• The reordering correctly makes experiment_name/experiment_run_name unconditionally take the highest precedence, with run metadata overriding item metadata for shared keys.
• The old code had an explicit isinstance(item_metadata, dict) guard before merging; the new code uses **(item_metadata or {}), which silently drops None but raises TypeError for any other non-dict type (e.g. a string or list metadata value), introducing a new crash path for dataset items with non-object metadata.
• A targeted regression test is added that covers the primary conflict scenario and verifies item-only keys are preserved.

Confidence Score: 3/5

Safe to merge only after restoring the isinstance guard for item_metadata; without it, any dataset item whose metadata field is a non-object JSON value (string, list, number) will cause the experiment run to crash mid-execution.

The merge-order fix and the reserved-field pinning are correct, and the regression test validates the intended scenario well. However, removing the isinstance(item_metadata, dict) check introduces a new crash path: if a dataset item carries non-dict metadata (a valid JSON state), the ** unpack will raise a TypeError that propagates out of the item processing loop, silently dropping results for that item and potentially aborting the run depending on error handling upstream.

langfuse/_client/client.py lines 2879–2884 — the metadata dict literal needs isinstance(item_metadata, dict) restored before the ** unpack.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[run_experiment called] --> B[Extract item_metadata from dataset item]
    B --> C{item_metadata is dict?}
    C -- "Yes (new: via ** unpack)" --> D["Merge: item_metadata → experiment_metadata → reserved fields"]
    C -- "None (handled by 'or {}')" --> D
    C -- "Non-dict truthy value\n(string, list, etc.)" --> E["❌ TypeError raised\n(regression from removed isinstance guard)"]
    D --> F["final_observation_metadata =\n{**item_metadata, **experiment_metadata,\nexperiment_name, experiment_run_name}"]
    F --> G{Is dataset item?}
    G -- Yes --> H["Update with dataset_id, dataset_item_id"]
    G -- No --> I[Set span attributes]
    H --> I
    I --> J[Span recorded with correct precedence]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[run_experiment called] --> B[Extract item_metadata from dataset item]
    B --> C{item_metadata is dict?}
    C -- "Yes (new: via ** unpack)" --> D["Merge: item_metadata → experiment_metadata → reserved fields"]
    C -- "None (handled by 'or {}')" --> D
    C -- "Non-dict truthy value\n(string, list, etc.)" --> E["❌ TypeError raised\n(regression from removed isinstance guard)"]
    D --> F["final_observation_metadata =\n{**item_metadata, **experiment_metadata,\nexperiment_name, experiment_run_name}"]
    F --> G{Is dataset item?}
    G -- Yes --> H["Update with dataset_id, dataset_item_id"]
    G -- No --> I[Set span attributes]
    H --> I
    I --> J[Span recorded with correct precedence]

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
langfuse/_client/client.py:2879-2884
**Missing isinstance guard drops non-dict metadata safety net**

The old code used `if isinstance(item_metadata, dict)` before applying item metadata, intentionally ignoring non-dict values. The new code replaces that with `**(item_metadata or {})`, which raises `TypeError: argument after ** must be a mapping` whenever `item_metadata` is truthy but not a dict (e.g., a string `"notes"`, a list, or any other JSON-serializable scalar). A dataset item whose `metadata` field is set to a non-object JSON value will now cause the entire experiment item run to crash rather than silently skipping the bad value as before.

```suggestion
                final_observation_metadata = {
                    **(item_metadata if isinstance(item_metadata, dict) else {}),
                    **(experiment_metadata or {}),
                    "experiment_name": experiment_name,
                    "experiment_run_name": experiment_run_name,
                }
```

_{Reviews (1): Last reviewed commit: "fix(experiments): preserve run metadata ..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[github-actions]

@claude review

[claude]

LGTM — small, well-scoped fix to metadata precedence with a targeted regression test. The one inline nit about removing the isinstance(item_metadata, dict) guard is minor: impact is bounded to users passing an invalid type, and the identical guard still exists ~70 lines below at line 2951, so it's a consistency cleanup rather than a blocker.

Extended reasoning...

Overview

Two-file change: swap the order of keys in the final_observation_metadata dict-literal in _process_experiment_item so item metadata is spread first, then run/experiment metadata, then the reserved experiment_name/experiment_run_name keys. Also removes a now-redundant post-hoc .update(item_metadata) and adds one regression test verifying that a run-level metadata key overrides a conflicting item-level key while item-only keys survive and experiment_run_name is preserved.

Security risks

None. Change is purely about key ordering in an internal metadata dict on an OTel experiment span. No auth, crypto, input parsing, or network paths touched.

Level of scrutiny

Low. The diff is ~5 lines of production code inside a single async helper, with a matching regression test. Behavior change is a precedence fix (run metadata wins over item metadata), which matches the PR description and title, and the new test locks it in.

Other factors

The bug hunting pass surfaced one nit: dropping the isinstance(item_metadata, dict) guard means a truthy non-mapping item_metadata (e.g. a string in a dict item's metadata field) now raises TypeError inside the item's try, gets caught by the outer handler at line 2965, and drops that item from results where it previously ran to completion with the malformed metadata silently ignored. That is a real behavior change but bounded to users violating the documented type. The inline comment records the concrete fix (mirror the surviving isinstance check at line 2951). Not a blocker for approval.