feat(adapter): surface the publish warnings the server returns - #147
Conversation
The openclaw plugin logs reflexio's `warnings` — fields it could not bind, interactions skipped as empty. This adapter ignored the response entirely, so half the first-party install base still got a 200 and no signal for the exact defect the channel was built for: 50 mis-keyed interactions stored as 50 empty rows. It matters more here than in openclaw. The pinned-request_id path posts the payload straight through `_make_request`, so unknown keys really do reach the server rather than being stripped client-side first — the server's warnings are the only signal that path has. `publish` is split so the diagnostic read sits outside `_attempt_publish` and every try inside it. `publish_unpublished` advances the buffer watermark only on a truthy result, so a raise while reading warnings would report an ACCEPTED publish as failed and re-send the same batch on every later hook — duplicates forever, caused by the observability code. The three success paths now return their response alongside the result so the read has something to work with after the try has closed. `_publish_warnings` is defensive but not total, and says so: `getattr` absorbs only AttributeError, a mapping can override `get`, and `str` runs a caller's `__str__`. The caller's guard covers those and the logging handler. Both shapes are handled — the raw path returns parsed JSON, the client path a response object. Tests cover both publish paths and the three shapes that actually escape the isinstance guard; removing the guard fails exactly those three. One test drives the real Adapter through `publish_unpublished` and asserts the watermark still advances, which is the invariant rather than a proxy for it. Pre-existing and untouched: 9 suite failures (install-scripts, opencode-dist) that also fail on a clean main, and ruff-format drift across 15 files.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 participant
Follow-up to ReflexioAI/reflexio#387, which added a
warningschannel to publish responses and wired the openclaw adapter to log it. This adapter ignored the response entirely, so half the first-party install base still got a200and no signal.Why it matters more here
The pinned-
request_idpath posts the payload straight through_make_request, bypassingInteractionDataconstruction — so unknown keys genuinely reach the server instead of being stripped client-side first. On that path the server'swarningsare the only signal. The defect being caught: 50 interactions keyedContentinstead ofcontentstored as 50 rows withcontent = '', returning200.The structural bit
publishhad three success returns nested inside onetrywhoseexceptreturnsPublishResult(False).publish_unpublishedadvances the buffer watermark only on a truthy result — so putting the warning read inside thattrywould mean a raise reports an accepted publish as failed, and the same batch is re-sent on every later hook. Duplicates forever, caused by the code meant to improve observability.So the body moved to
_attempt_publish, which returns(result, response), and the read happens inpublishafter it returns — outside thatexceptand every nested one. A second guard covers the logging handler itself, not just the extraction._publish_warningsis deliberately documented as defensive, not total:getattrabsorbs onlyAttributeError, a mapping can overrideget, andstrruns a caller-supplied__str__. That claim was wrong in the openclaw version and review caught it; not repeating it here. Both response shapes are handled — parsed JSON dict on the raw path, response object on the client path.Testing
Covers both publish paths, the quiet case, and seven response shapes. Four of those are absorbed by the
isinstanceguard; three actually escape — awarningsproperty raisingRuntimeError, adictsubclass overridingget, and an item whose__str__raises. Removing the guard fails exactly those three and nothing else, which is the check the openclaw tests originally missed: they asserted totality while only exercising shapes that could never violate it.One test drives the real
Adapterthroughpublish_unpublishedwith a hostile response and assertspublished_up_tois still written — the invariant itself rather than a proxy for it. It also fails with the guard removed.71 passedacross the two touched files.Pre-existing, untouched
9 suite failures (
test_install_scripts,test_opencode_support) reproduce on a cleanmainwith this branch stashed.ruff-formatdrift spans 15 files repo-wide; CI runs pytest only. Reformatting two of fifteen inside a focused PR would be arbitrary noise.