fix(api)!: reject publishes that carry no extractable content - #384
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughChangesPublish interaction validation now identifies content-bearing fields, rejects contradictory shapes, removes empty rows, and rejects fully empty batches. Route and generation-service logging sanitizes request IDs and withholds sensitive response or exception messages. Regression tests cover validation, payload shapes, background failures, and log safety. Interaction publish flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PublishUserInteractionRequest
participant PreconditionChecks
participant publish_user_interaction
participant publisher_api
Client->>PublishUserInteractionRequest: Submit interaction payload
PublishUserInteractionRequest->>PublishUserInteractionRequest: Validate shapes and filter empty rows
PublishUserInteractionRequest->>PreconditionChecks: Pass validated interactions
PreconditionChecks->>PreconditionChecks: Check content and indexed shape errors
PreconditionChecks-->>publish_user_interaction: Validation result
publish_user_interaction->>publisher_api: Add user interaction
publisher_api-->>publish_user_interaction: Success or content-free rejection
publish_user_interaction->>publish_user_interaction: Emit sanitized logs
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/api_endpoints/precondition_checks.py`:
- Around line 29-37: Update the explanatory comment above the interaction-data
loop to refer to InteractionData.shape_error() instead of the nonexistent
InteractionData.validation_error(), keeping the rest of the comment unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62528380-9827-497e-bafc-b46199cbe2ca
📒 Files selected for processing (7)
reflexio/models/api_schema/common.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/api_endpoints/precondition_checks.pyreflexio/server/routes/interactions.pytests/server/api_endpoints/test_api_routes.pytests/server/api_endpoints/test_precondition_checks.pytests/server/api_endpoints/test_publish_validation.py
guangyu-reflexio
force-pushed
the
fix/publish-contentless-core
branch
from
July 27, 2026 19:52
484788b to
29bfe8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reflexio/server/services/generation_service.py (1)
804-838: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
emit_deferred_learning_side_effectsstill logs rawrequest_id.Three
logger.exception(...)calls here (profile side-effects at 810-814, playbook side-effects at 820-824, schedule-tagging at 834-837) logplan.request_idunsanitized.plan.request_idis the same caller-supplied, unbounded, unrestricted-character value that is sanitized everywhere else in this file — including the near-identical schedule-tagging failure log in_run_learning_stepsa few hundred lines below (already fixed tosanitise_for_log(request_id)). This method sits on the deferred-learning path this PR's layer explicitly claims to cover, so it appears to be a missed call site rather than an intentional exclusion.Proposed fix
if plan.profile is not None: profile_service, profile_plan = plan.profile try: profile_service.emit_generation_side_effects(profile_plan) except Exception: logger.exception( "Failed to emit profile side effects for deferred " "learning request %s", - plan.request_id, + sanitise_for_log(plan.request_id), ) if plan.playbook is not None: playbook_service, playbook_plan = plan.playbook try: playbook_service.emit_generation_side_effects(playbook_plan) except Exception: logger.exception( "Failed to emit playbook side effects for deferred " "learning request %s", - plan.request_id, + sanitise_for_log(plan.request_id), ) try: schedule_tagging(...) except Exception: logger.exception( "Failed to schedule tagging for deferred learning request %s", - plan.request_id, + sanitise_for_log(plan.request_id), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/generation_service.py` around lines 804 - 838, Update all three logger.exception calls in emit_deferred_learning_side_effects to pass the sanitized form of plan.request_id, matching the existing sanitise_for_log usage elsewhere in the file. Apply this consistently to the profile side-effects, playbook side-effects, and schedule-tagging failure messages while preserving their current message text and control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/routes/interactions.py`:
- Around line 137-159: Add the repository-standard BLE001 suppression with a
concise justification to the intentional `except Exception as exc` handler in
the background publish error path of `interactions.py`, matching the established
style used in `generation_service.py`.
In `@tests/server/api_endpoints/test_publish_validation.py`:
- Around line 219-253: Update test_no_log_call_interpolates_a_raw_request_id so
its logger-call scan flags both bare request_id arguments and attribute accesses
ending in .request_id, such as plan.request_id,. Preserve the existing scope and
offender reporting while broadening the match beyond the exact line.strip() ==
"request_id," check.
---
Outside diff comments:
In `@reflexio/server/services/generation_service.py`:
- Around line 804-838: Update all three logger.exception calls in
emit_deferred_learning_side_effects to pass the sanitized form of
plan.request_id, matching the existing sanitise_for_log usage elsewhere in the
file. Apply this consistently to the profile side-effects, playbook
side-effects, and schedule-tagging failure messages while preserving their
current message text and control flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 468eb844-2170-4672-b332-a8402b830fe0
📒 Files selected for processing (8)
reflexio/models/api_schema/common.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/api_endpoints/precondition_checks.pyreflexio/server/routes/interactions.pyreflexio/server/services/generation_service.pytests/server/api_endpoints/test_api_routes.pytests/server/api_endpoints/test_precondition_checks.pytests/server/api_endpoints/test_publish_validation.py
🚧 Files skipped from review as they are similar to previous changes (5)
- reflexio/models/api_schema/common.py
- reflexio/server/api_endpoints/precondition_checks.py
- reflexio/models/api_schema/domain/entities.py
- tests/server/api_endpoints/test_api_routes.py
- tests/server/api_endpoints/test_precondition_checks.py
A publish of 50 interactions returned 200 and stored 50 rows with content=''. No profiles were generated and nothing reported an error. Three defects combined to hide it. 1. The precondition guard written to catch exactly this was dead code. UserActionType is a StrEnum whose NONE member is the truthy string "none", so `not interaction_data.user_action` was always False and the four-way "all empty" chain could never fire. Ten lines above, the same function already used the correct `!= UserActionType.NONE`. A test pinned the broken behaviour as correct, which is why it survived; that test is now inverted. 2. Two sibling rules in the same guard (user_action needs a description; interacted_image_url xor image_encoding) existed ONLY on a path whose result is discarded, so neither was ever reportable either. 3. On the default async path the rejection is discarded -- add_user_interaction runs inside a BackgroundTask whose return value was dropped, after the caller already got 200 "queued". A success=False is not an exception, so it was not logged either. The rules therefore live in a model_validator on PublishUserInteractionRequest, which runs during request parsing and so applies on both the sync and the background-task path. InteractionData.shape_error() holds the contradictions and precondition_checks delegates to it, so the two layers cannot diverge. An individual empty interaction is skipped, not fatal. Failing the batch was implemented and reverted: both first-party plugins append an empty Assistant placeholder unconditionally, so one empty row rejected the batch containing the real user turn, and their adapters swallow the error without advancing the publish watermark -- retrying the same doomed batch forever. A batch where *every* interaction is empty is still a 422, and that is precisely the incident (50 of 50 rows). Skips are logged at INFO with the caller's original indices so the drop is not silent to an operator. carries_content() counts every content-bearing field -- tools_used, shadow/expert content, citations, retrieved_learnings -- because a narrower list would reject legitimate tool-call-only and shadow-mode turns, and it strips text so " " is not content. Caller-supplied request_id is sanitised at every logger call that interpolates it -- 9 sites across the publish route and generation_service, not just the one that prompted the fix. It is a NonEmptyStr with no length cap and no character restrictions, so a newline in it forges a line in a shared multi-tenant stream that Sentry ingests as an event body. A test scans for any logger call passing it raw, so a new site fails the suite. The two sites that pass request_id as a LOCK OWNER TOKEN are deliberately left raw: sanitising there would truncate the token and break ownership comparison. Both background-task log lines withhold their reason string -- on the storage path it is an unbounded str(e) from a catch-all -- while still logging the exception type and raising file:line, which are content-free and the only way to locate a background failure. Scope: unknown fields stay silently ignored, as on main. Rejecting them broke every first-party plugin publish, and *reporting* them to the caller is a separate, larger change this deliberately leaves out. Also fixes a route-test fixture that was itself posting user_message/ agent_message/interaction_type -- none of which are InteractionData fields -- and asserting 200 on the resulting empty interaction. BREAKING CHANGE: a publish where every interaction is empty, an interaction whose user_action has no user_action_description, or one setting both interacted_image_url and image_encoding, now returns 422 instead of being accepted and silently dropped.
guangyu-reflexio
force-pushed
the
fix/publish-contentless-core
branch
from
July 27, 2026 22:10
29bfe8a to
f987c8a
Compare
Contributor
Author
|
Both comments valid, fixed in Major — Widened to Widening also surfaced 10 further pre-existing sites in two modules this PR does not touch ( One thing I checked before applying the fix, having nearly got this wrong earlier in the branch: every changed line is genuinely a logger argument. Minor — missing Gates: 4893 passed / 0 failed, |
This was referenced Jul 27, 2026
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 28, 2026
## Why A publish of 50 interactions to prod org 54 returned `200 OK` and stored 50 rows with `content = ''`. No profiles were ever produced. The caller had sent `Content` instead of `content`; every `InteractionData` field has a default and pydantic's `extra="ignore"` dropped the unknown key, so nothing bound and nothing was reported. Diagnosing it took hours. #384 made the all-empty case a `422`. This adds the missing half: **telling the caller what was dropped**, on the paths that still return 200. An earlier attempt used `extra="forbid"` plus per-interaction 422s. Both wedged the first-party plugins — reproduced, then reverted. Warn-don't-forbid is a deliberate decision, not an oversight. ## What - Unrecognised fields are captured and reported in `warnings` with the **caller's own** interaction index (computed before empty-row filtering, so indices are not renumbered), including nested paths like `tools_used[0].zzz`. - Interactions skipped as empty are summarised. - The all-empty `422` now names the mis-keyed field — previously the warnings were computed and then thrown away by the raise, so the incident's own scenario produced the least informative message available. - Top-level request typos (`forceExtraction`) are reported too; a dropped `force_extraction` silently changes behaviour rather than losing one row. - `ToolUsed.status` is now declared and coerced leniently. Plugins send it constantly; declaring it strictly turned five values into whole-batch 422s during development. - The openclaw plugin builds its wire payload from an **allowlist** pinned against the real model, replacing a denylist that had already let `user_id` onto the wire. Its adapter logs the warnings. ## Design notes **`warnings` is appended to, never assigned.** It already carries extraction-stall warnings that the CLI renders. The sync path had zero test coverage — deleting the append left the whole suite green — so that is now pinned with a test that seeds both kinds. **The client merges locally-dropped fields.** `publish_interaction` builds `InteractionData` before `model_dump()`, so unknown keys never reach the server. Without the merge the feature works over raw HTTP and is invisible through the SDK, which is the path almost everyone uses. A test pins that warnings cannot double. **The adapter reads warnings outside the try that guards the publish.** `publish_unpublished` advances the buffer watermark only on `True`, so a raise while reading diagnostics would report an accepted batch as failed and re-send it on every later hook — duplicates forever, caused by the observability code. Review found the helper's "total by construction" claim was false in three ways; the whole block is now guarded and tested against shapes that actually escape. **A correct 50-turn plugin batch produces zero warnings.** `user_id`/`session_id` are suppressed at both levels — warning on the routine case is how you train operators to ignore a channel. ## Testing - 4930 passed, 123 skipped in the root suite; 158 in the plugin suite; ruff and pyright clean. - Every new test was mutation-verified: the behaviour was reverted and the test confirmed to fail. - The openclaw plugin tests previously ran in **no** CI workflow — including the drift guard that exists to fail when `InteractionData` grows a field. Fixed in the paired enterprise PR, since the workflows live in the superproject. ## Deferred - The claude-smart adapter does not read `warnings` — same sibling site, separate repo, follow-up PR. - Pre-existing unsanitised `identifier`/`user_id` and `str(exc)` log sites in `base_generation/` (merged code, not this branch). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Publish responses now include warnings for skipped empty interactions and unrecognized fields. * SDK clients can access both server- and locally detected publish warnings. * Tool usage data now supports a normalized, length-limited status value. * **Bug Fixes** * Prevented internal bookkeeping and unknown fields from being sent in interaction payloads. * Successful publishes are no longer reported as failures when warning display encounters malformed responses. * **Documentation** * Expanded guidance on payload construction, warning interpretation, and non-retryable validation failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the incident: a publish of 50 interactions returned 200 OK and stored 50 rows with
content = ''. No profiles were ever generated, and nothing on any layer reported a problem.This is the minimal fix, split out of #383 so the incident fix can land on its own. Everything about reporting dropped fields back to the caller stays in #383.
Three defects combined to hide it
1. The guard written for exactly this case was dead code.
UserActionTypeis aStrEnumwhoseNONEmember is the truthy string"none", sonot interaction_data.user_actionwas alwaysFalseand the four-way "all empty" chain could never fire:Ten lines above, the same function already used the correct
!= UserActionType.NONE. It survived because a test pinned it —test_all_fields_empty_with_none_action_passesdiagnosed the dead branch in its own comment, then assertedvalid is True. That test is now inverted.2. Two sibling rules lived only on a discarded path.
user_actionneeds a description;interacted_image_urlxorimage_encoding. Both existed only in the precondition guard, so neither was ever reportable either.3. On the default async path the rejection is thrown away.
add_user_interactionruns inside aBackgroundTaskwhose return value was dropped, after the caller already got200 "queued". Asuccess=Falseisn't an exception, so it wasn't logged either.The fix
The rules live in a
model_validatoronPublishUserInteractionRequest— it runs during request parsing, so it applies on both the sync and background-task path.InteractionData.shape_error()holds the contradictions andprecondition_checksdelegates to it, so the two layers cannot diverge.An individual empty interaction is skipped, not fatal. Failing the batch was implemented and reverted: both first-party plugins append an empty
Assistantplaceholder unconditionally, so one empty row rejected the batch containing the real user turn — and their adapters swallow the error without advancing the publish watermark, retrying the same doomed batch forever. Reproduced end to end. A batch where every interaction is empty is still a 422, which is precisely the incident (50 of 50). Skips are logged server-side with the caller's original indices.carries_content()counts every content-bearing field —tools_used, shadow/expert content,citations,retrieved_learnings— because a narrower list rejects legitimate tool-call-only and shadow-mode turns. Text is stripped, so" "is not content.Both background log lines withhold their reason string (on the storage path it is an unbounded
str(e)from a catch-all, and Sentry ingests ERROR records as event bodies unscrubbed), andrequest_idis sanitised before reaching them — it is aNonEmptyStrwith no length cap and no character restrictions, so a newline could forge a line in a shared multi-tenant log stream.Deliberately out of scope
Unknown fields stay silently ignored, exactly as on
main. Rejecting them (extra="forbid") broke every first-party plugin publish into the same silent retry loop, and reporting them to the caller is a larger feature — capture/strip, nested models, volume caps, sanitisation, SDK propagation. That is #383, reviewed on its own merits.Tests
Written RED first.
TestEmptyInteractionsandTestSiblingRulesEnforcedAtBoundarypin the 422s on the async path — the one that previously returned 200 "queued" and then silently refused the write.TestLegitimateTurnsStillAcceptedis driven fromCONTENT_BEARING_FIELD_NAMESplus an independent pinned-set assertion, because a parametrize driven only by that tuple deletes its own coverage when a field is removed (proven by mutation).A route-test fixture was itself an instance of this bug — posting
user_message/agent_message/interaction_type, none of which areInteractionDatafields, and asserting 200. Fixed the data, not the assertion.ruffclean,pyright0 errors, full OSS + enterprise suites green.Summary by CodeRabbit