feat(providers): wire IOJournalRecorder into AnthropicAdapter (M3 / slice 3 of #517)#535
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
31047e8for PR #535
Review record:
984da14d-fc59-4d25-95fb-176c059cc527
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/providers/anthropic_adapter.py:216 | BLOCKING | The Anthropic journal emission does not record top_p or stop_sequences, even though both are sent to client.messages.create() and can materially change model behavior. As written, two different outbound requests can produce indistinguishable llm.call.requested events because those fields are neither persisted nor included in prompt_hash, which breaks the PR’s stated “reconstructable from the journal alone” contract. |
Follow-up Findings
src/ouroboros/providers/anthropic_adapter.py:44[warning]_serialise_prompt_for_hash()hashessystem_partsas a list, but the actual Anthropic request normalizes them into a single string via\"\\n\\n\".join(system_parts). That means identical wire requests can hash differently depending only on how callers split system text acrossMessageRole.SYSTEMentries, soprompt_hashis not stable for the real outbound payload.
Non-blocking Suggestions
None.
Design Notes
The recorder/event-factory split is clean and the adapter integration stays additive, but the current Anthropic wiring does not yet preserve enough of the real request shape to make the journal a reliable replay/debug artifact.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
c404890for PR #535
Review record:
bb3d1900-ea52-4a30-b8fa-a92a4e85847a
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/events/io.py:282,390,452 | BLOCKING | The event factories reshape previews even when callers have already applied recorder-specific privacy/cap settings. IOJournalRecorder precomputes previews with its explicit privacy, preview_cap, and preview_hard_cap overrides before calling these factories (src/ouroboros/events/io_recorder.py:196-200, 233-237, 285-289, 320-324), but the factories call shape_preview() again with default settings. That means a valid override like privacy=PrivacyMode.ON with OUROBOROS_IO_JOURNAL_PREVIEWS=off still loses the preview, and any preview_cap larger than 256 is silently truncated back to 256 on persistence. This breaks the recorder API contract and makes the emitted journal data differ from the caller’s requested policy. |
Non-blocking Suggestions
None.
Design Notes
The split between pure event factories and an async recorder is a good direction, and the Anthropic integration keeps the old call path intact when no recorder is configured. The main design issue is that preview policy is enforced in two layers, so the factory/recorder boundary is currently not a single source of truth.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Autopilot follow-up: pushed review-addressing commits and confirmed the GitHub check suite is green on the latest head. Current merge state is clean; remaining CHANGES_REQUESTED status appears to be awaiting fresh bot/human re-review rather than failing CI. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
6aa0380for PR #535
Review record:
4215e59d-c018-49f9-98a9-3bd5321f2c5f
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
src/ouroboros/providers/anthropic_adapter.py:30[warning]_serialise_prompt_for_hash()claims to hash the request, but the payload it builds excludes core request parameters likemodel,max_tokens, andtemperaturewhile the caller only passestop_pandstop_sequencesat src/ouroboros/providers/anthropic_adapter.py:244. That means two materially different Anthropic calls can persist the sameprompt_hash, which makes the journal misleading for replay/debugging and breaks the “same request => same hash” invariant this PR is introducing.
Non-blocking Suggestions
None.
Design Notes
The recorder/factory split is clean and the best-effort append behavior is sensible. The main weakness is that the adapter-specific prompt serialization does not yet preserve full request identity, which undercuts the value of the new journal fields.
Policy Notes
- No in-scope blocking findings remained after policy filtering; downgraded verdict accordingly.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Q00
left a comment
There was a problem hiding this comment.
Reviewed the AnthropicAdapter IOJournalRecorder wiring. The optional recorder path preserves existing behavior and records both success and provider error paths cleanly.
52b61cd to
44a017e
Compare
Summary
Third slice of #517: wires the
IOJournalRecorder(#534) intoAnthropicAdapter. The constructor now accepts an optionalio_recorderkeyword; when provided, everyclient.messages.createcall is wrapped in the recorder so pairedllm.call.requested/llm.call.returnedevents land on the EventStore.The change is purely additive: legacy constructor calls without
io_recorder=continue to work byte-for-byte (defaultNoneskips emission entirely).Changes
src/ouroboros/providers/anthropic_adapter.pyio_recorder: IOJournalRecorder | None = None(last keyword, defaultNone).complete()wrapsclient.messages.createinrecorder.record_llm_call(...)only when a recorder is configured. The legacy code path is preserved verbatim for the no-recorder case._serialise_prompt_for_hash,_record_completion) keep the recorder wiring tidy.tests/unit/providers/test_anthropic_adapter_io_recorder.py— 8 cases covering both branches plus the exception path.Verification
uv run ruff check ...uv run ruff format ...uv run pytest tests/unit/providers/test_anthropic_adapter_io_recorder.pyPre-merge checklist
io_recorderkwarg; legacy callers unchangedcomplete()emits paired requested/returned events with sharedcall_idwhen recorder presentreturnedwithis_error=True+error_kindResult.errvia the existing_handle_erroron exception_parse_responseor response shapePost-merge checklist
record_tool_callIOJournalRecorderinto adapters where wantedRollback
Constructor kwarg is optional with a
Nonedefault. Rollback removes the kwarg + helpers; no caller depends on the new behaviour yet.Stack: depends on #534 → #532.