fix(tracing): forward Codex reasoning effort from turn_context - #85
Conversation
There was a problem hiding this comment.
Looks good — small, well-tested, additive change.
What was reviewed: the turn_context parsing in parse.ts (effort preferred over reasoning_effort, matching the measured field usage described in the PR), the new reasoningEffort propagation into root metadata, generation modelParameters, and generation metadata in trace.ts, and the corresponding new tests in parse.test.ts/trace.test.ts. Checked that the comment removals are purely cosmetic (diffed logic lines only, no behavior change). Also confirmed the pre-existing convertRollout dedup test failure noted in the PR description is unrelated to this change (it stems from an earlier commit, #78) and not newly introduced here.
Extended reasoning...
Overview
The diff adds an optional reasoningEffort field threading from turn_context payloads (parse.ts/types.ts) through to Langfuse observation metadata and generation modelParameters (trace.ts). It also strips a large number of pre-existing explanatory comments with no logic changes, verified by diffing only non-comment lines and confirming behavior is unchanged.
Security risks
None. This is a read-only metadata enrichment: it reads a string field (effort/reasoning_effort) already present in Codex rollout JSON and forwards it as observation metadata/model parameters to Langfuse. No new external input parsing beyond an existing trusted-format field, no auth/permissions/crypto code touched, no injection surface introduced.
Level of scrutiny
Low-to-moderate is appropriate: the change is small, additive, mechanical, and confined to an internal tracing/observability plugin (not a critical path like auth or payment). The precedence logic (effort over reasoning_effort) is simple and directly tested. I verified the fallback logic manually: typeof p.effort === "string" ? p.effort : p.reasoning_effort, then only assigns if the result is a string — this correctly preserves the previous turn's value when a later turn_context lacks the field, consistent with the existing t.model = p.model ?? t.model pattern already in the file.
Other factors
New test coverage in parse.test.ts directly exercises the two claimed properties (turn_context-per-turn vs. thread_settings between turns, and the effort/reasoning_effort alias precedence), and trace.test.ts asserts the metadata and modelParameters propagation end-to-end. The PR description's claim about a pre-existing unrelated test failure (does not re-export a settings event...) is plausible and traces to an earlier commit (#78), not this diff. The bug-hunting system reported no findings, and my own read of the diff found no logic issues, so this is a solid candidate for approval without requiring further human review.
a034fcd to
e32fc79
Compare
Linear: LFE-16502
Summary
Export Codex's per-turn reasoning effort to Langfuse.
turn_context. Codex writes it aseffort;reasoning_effortis accepted as a forward-compatible alias.modelParameters.reasoning_effort, pluscodex.reasoning_effortmetadata on the turn and on each generation.Supersedes #43, which targets the same gap but was written before #78 landed.
Why #43's
thread_settings_appliedbranch is not carried overMeasured across all 119 local rollouts (Codex up to 0.155.0):
turn_context.effortturn_contextlinesturn_context.reasoning_effortthread_settings_applied→thread_settings.reasoning_effortturn_context.effort)Since #78,
parseSessionguards non-turn-opening events withelse if (!turn) continue;, andfinishTurnsetsturn = null. Codex emitsthread_settings_appliedaftertask_completewithturn_id: null, so 42 of the 44 never reach a handler at all. #43's branch is dead code against currentmain, and the 2 occurrences that would fire carry a valueturn_contextalready provided.turn_contextis also the per-turn record of what was actually sent to the model, which makes it the authoritative source.Note that #43 prefers
reasoning_effortovereffort— i.e. it prefers the key that never appears in practice. This PR inverts that precedence.Verification
Two fresh
codex execruns on Codex 0.155.0 (-c model_reasoning_effort=highand=low), each exported through the realStophook bundle:modelParameterscodex.reasoning_effortmain{}{"reasoning_effort":"high"}high{"reasoning_effort":"low"}lowThe low/high pair confirms the value is read per turn rather than set to a constant.
tsc --noEmitclean,prettier --checkclean, 57 of 58 tests pass.Pre-existing failure, not introduced here
trace.test.ts > convertRollout > "does not re-export a settings event between turns on every invocation"fails on this branch and onmain—main's CI has been red since a1eb683. The test callsconvertRollout, which only returns the exported turn ids; the dedup sidecar is written by the hook insrc/index.ts. The second-pass dedup it asserts therefore cannot happen. Left untouched here so the fix stays reviewable on its own.Commits
refactor: removed unnecessary comments— removes 94 comment lines fromsrc/, no code changeadded effort to be traced— the feature itselfThe branch is one commit behind
main(the OpenTelemetry bump in #19). GitHub diffs against the merge base, so that bump does not appear here; use "Update branch" before merging.