fix(api)!: report contentless and mis-keyed publish payloads - #383
fix(api)!: report contentless and mis-keyed publish payloads#383guangyu-reflexio wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughInteraction publishing now captures and strips unknown fields while returning warnings, validates content and sibling-field rules with indexed errors, filters plugin payloads through an allowlist, and reports background publish failures without exposing response contents. ChangesInteraction publish contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant PublishEndpoint
participant InteractionData
participant PublisherAPI
Client->>PublishEndpoint: Submit interaction_data_list
PublishEndpoint->>InteractionData: Validate shapes and capture unknown fields
InteractionData-->>PublishEndpoint: Return filtered data and warnings
PublishEndpoint->>PublisherAPI: Queue valid interaction
PublisherAPI-->>PublishEndpoint: Return publish status
PublishEndpoint-->>Client: Return response and warnings
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/server/api_endpoints/test_publish_validation.py (1)
102-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
citationsandretrieved_learningsas valid content. Both fields are accepted byInteractionData.carries_content(), but neither validation path has a regression case for them. A future predicate edit could reject these payloads unnoticed.
tests/server/api_endpoints/test_publish_validation.py#L102-L124: add accepted parameter cases with non-empty validcitationsandretrieved_learnings.tests/server/api_endpoints/test_precondition_checks.py#L89-L115: add precondition-guard acceptance cases for the same two fields.🤖 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 `@tests/server/api_endpoints/test_publish_validation.py` around lines 102 - 124, Extend the parameterized valid-content cases around InteractionData.carries_content() in tests/server/api_endpoints/test_publish_validation.py:102-124 with non-empty citations and retrieved_learnings payloads. Also add corresponding precondition-guard acceptance cases in tests/server/api_endpoints/test_precondition_checks.py:89-115, ensuring both fields are verified as valid independently.
🤖 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.
Nitpick comments:
In `@tests/server/api_endpoints/test_publish_validation.py`:
- Around line 102-124: Extend the parameterized valid-content cases around
InteractionData.carries_content() in
tests/server/api_endpoints/test_publish_validation.py:102-124 with non-empty
citations and retrieved_learnings payloads. Also add corresponding
precondition-guard acceptance cases in
tests/server/api_endpoints/test_precondition_checks.py:89-115, ensuring both
fields are verified as valid independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 135add30-612e-467b-bc27-458a8a965202
📒 Files selected for processing (7)
reflexio/models/api_schema/domain/entities.pyreflexio/server/api_endpoints/precondition_checks.pyreflexio/server/routes/interactions.pytests/cli/test_helpers.pytests/server/api_endpoints/test_api_routes.pytests/server/api_endpoints/test_precondition_checks.pytests/server/api_endpoints/test_publish_validation.py
9ef48f3 to
df5eb84
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
reflexio/models/api_schema/domain/entities.py (2)
799-818: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider surfacing unknown field names in the "is empty" reason.
When an interaction is empty because its only intended content landed on a mis-keyed field (e.g.
{"Content": "..."}),validate_interaction_shapesraises beforeunknown_field_warnings()is ever consulted, so the 422 the caller receives says only"is empty"— the most likely root cause (a typo'd key) is silently dropped even thoughunknown_field_names()already has it. This is exactly the debugging gap this PR is built to close.♻️ Proposed enrichment of the empty-interaction message
if not self.carries_content(): - return f'is empty: set "content" (or any of: {CONTENT_BEARING_FIELDS})' + hint = ( + f" (ignored unrecognised field(s): {', '.join(self.unknown_field_names())})" + if self.unknown_field_names() + else "" + ) + return ( + f'is empty: set "content" (or any of: {CONTENT_BEARING_FIELDS}){hint}' + ) return None🤖 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/models/api_schema/domain/entities.py` around lines 799 - 818, Update validation_error so the empty-interaction response incorporates unknown field names from unknown_field_names(). When carries_content() is false and unknown fields exist, include those names in the returned reason while preserving the current generic message when none exist; ensure validate_interaction_shapes surfaces this enriched message before unknown_field_warnings() is consulted.
746-764: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
model_extra.clear()instead of reassigning__pydantic_extra__.Pydantic 2.x exposes
model_extraas the documented public API for extra fields; clearing that live dictionary drains the stored extra fields without depending on the private attribute name.♻️ Proposed refactor using the public API
`@model_validator`(mode="after") def capture_and_strip_unknown_fields(self) -> Self: - if extra := self.__pydantic_extra__: + if extra := self.model_extra: self._unknown_field_names = sorted(extra) - self.__pydantic_extra__ = {} + extra.clear() return self🤖 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/models/api_schema/domain/entities.py` around lines 746 - 764, Update capture_and_strip_unknown_fields to use the public model_extra mapping: collect and sort its keys, then clear the live dictionary instead of assigning to the private __pydantic_extra__ attribute. Preserve the existing behavior of recording only unknown field names and leaving model_dump() without extras.
🤖 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 `@AI_AGENT_INTEGRATION.md`:
- Around line 502-507: Update the publish watermark guidance to use
status-specific handling rather than treating all 4xx responses as terminal.
Isolate or dead-letter invalid rows, including indexed 422 failures without
discarding valid siblings; retry transient 408/429 responses; and hold the
watermark while escalating 401/403 authentication or configuration failures.
Preserve retry behavior for 5xx, timeouts, and connection errors, and document
that only safely handled rejected rows may advance the watermark.
In `@reflexio/integrations/openclaw/plugin/tests/test_state.py`:
- Around line 285-305: Update test_allowlist_matches_the_server_model and
test_wire_turn_validates_against_the_server_model to require the server
InteractionData model instead of using pytest.importorskip. Use an
installed-model fixture or otherwise make an unavailable reflexio import fail
the tests, ensuring both assertions detect publish-field drift rather than being
silently skipped.
In `@reflexio/models/api_schema/domain/entities.py`:
- Around line 746-764: Update capture_and_strip_unknown_fields to sanitize each
unknown key before storing it in _unknown_field_names: remove control characters
and cap each name at a reasonable maximum length, while preserving deterministic
sorting and excluding raw values. Ensure the sanitized names are the only values
later available to logging and client-facing warnings.
---
Nitpick comments:
In `@reflexio/models/api_schema/domain/entities.py`:
- Around line 799-818: Update validation_error so the empty-interaction response
incorporates unknown field names from unknown_field_names(). When
carries_content() is false and unknown fields exist, include those names in the
returned reason while preserving the current generic message when none exist;
ensure validate_interaction_shapes surfaces this enriched message before
unknown_field_warnings() is consulted.
- Around line 746-764: Update capture_and_strip_unknown_fields to use the public
model_extra mapping: collect and sort its keys, then clear the live dictionary
instead of assigning to the private __pydantic_extra__ attribute. Preserve the
existing behavior of recording only unknown field names and leaving model_dump()
without extras.
🪄 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 Plus
Run ID: 5ff2e96e-067d-4e14-85c4-b0ed82de9f56
📒 Files selected for processing (11)
AI_AGENT_INTEGRATION.mdreflexio/client/client.pyreflexio/integrations/openclaw/plugin/src/openclaw_smart/state.pyreflexio/integrations/openclaw/plugin/tests/test_state.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/api_endpoints/precondition_checks.pyreflexio/server/routes/interactions.pytests/cli/test_helpers.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 (1)
- tests/server/api_endpoints/test_precondition_checks.py
| def test_allowlist_matches_the_server_model(self): | ||
| """Pin the literal field set against the real InteractionData.""" | ||
| interaction_data = pytest.importorskip( | ||
| "reflexio.models.api_schema.domain.entities" | ||
| ).InteractionData | ||
| assert set(interaction_data.model_fields) == state._INTERACTION_DATA_FIELDS | ||
|
|
||
| def test_wire_turn_validates_against_the_server_model(self): | ||
| """Every dict the slicer emits must construct a real InteractionData.""" | ||
| interaction_data = pytest.importorskip( | ||
| "reflexio.models.api_schema.domain.entities" | ||
| ).InteractionData | ||
| _, turns = state.unpublished_slice( | ||
| [ | ||
| {"ts": 1, "role": "User", "content": "hi", "user_id": "p"}, | ||
| {"ts": 2, "role": "Assistant", "content": "hello", "user_id": "p"}, | ||
| ] | ||
| ) | ||
| for turn in turns: | ||
| built = interaction_data(**turn) | ||
| assert built.unknown_field_names() == [] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the plugin's test job installs the reflexio package (so importorskip actually exercises these tests).
fd -e yml -e yaml . .github/workflows 2>/dev/null | xargs -I{} rg -n -i 'openclaw|reflexio' {}
fd pyproject.toml requirements.txt setup.cfg reflexio/integrations/openclawRepository: ReflexioAI/reflexio
Length of output: 329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow matches =="
fd -e yml -e yaml . .github/workflows 2>/dev/null | sort | while read -r f; do
if rg -n -i 'openclaw|reflexio|pytest|pip install|uv install|poetry' "$f" >/dev/null; then
echo "--- $f"
rg -n -i 'openclaw|reflexio|pytest|pip install|uv install|poetry' "$f" || true
fi
done
echo "== openclaw pyproject =="
cat -n reflexio/integrations/openclaw/plugin/pyproject.toml | sed -n '1,220p'
echo "== nearby test lines =="
cat -n reflexio/integrations/openclaw/plugin/tests/test_state.py | sed -n '250,315p'Repository: ReflexioAI/reflexio
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
fd -e yml -e yaml . .github/workflows 2>/dev/null | sort | sed -n '1,120p'
echo "== any workflow mentioning openclaw/reflexio/pip/pytest =="
while IFS= read -r f; do
if rg -n -i 'openclaw|reflexio|pip install|uv install|pytest|pipx install|pdm|poetry|tox' "$f" >/dev/null; then
echo "--- $f"
rg -n -i 'openclaw|reflexio|pip install|uv install|pytest|pipx install|pdm|poetry|tox' "$f" || true
fi
done < <(fd -e yml -e yaml . .github/workflows 2>/dev/null)
echo "== openclaw plugin metadata and dependencies =="
cat -n reflexio/integrations/openclaw/plugin/pyproject.toml | sed -n '1,220p'
echo "== target tests =="
git ls-files 'reflexio/integrations/openclaw/plugin/tests/*' 'reflexio/integrations/openclaw/plugin/**' | sort
cat -n reflexio/integrations/openclaw/plugin/tests/test_state.py | sed -n '250,315p'
echo "== references to imported InteractionData constants/model in openclaw plugin =="
rg -n '_INTERACTION_DATA_FIELDS|InteractionData|unknown_field_names|pydantic' reflexio/integrations/openclaw/plugin -S --glob '*.py' || trueRepository: ReflexioAI/reflexio
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo root files =="
git ls-files | sed -n '1,160p'
echo "== workflows (if present) =="
if [ -d .github/workflows ]; then
fd -e yml -e yaml . .github/workflows -0 | sort -z | tr '\0' '\n'
else
echo ".github/workflows not present"
fi
echo "== openclaw plugin tree =="
git ls-files 'reflexio/integrations/openclaw/plugin/**' | sort | sed -n '1,200p'
echo "== pyproject/lock/dependency files mentioning openclaw/reflexio =="
for f in $(git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|setup\.cfg|poetry\.lock|uv\.lock|Pipfile|Pipfile\.lock|tox\.ini|environment\.yaml|environment\.yml)$'); do
if rg -n -i 'openclaw|reflexio|pytest|pip install|uv install|poetry|pdm|tox' "$f" >/dev/null 2>&1; then
echo "--- $f"
fi
doneRepository: ReflexioAI/reflexio
Length of output: 10794
Make the _INTERACTION_DATA_FIELDS assertions fail when the server model changes.
test_allowlist_matches_the_server_model and test_wire_turn_validates_against_the_server_model currently use pytest.importorskip; if reflexio is unavailable, these drift detectors skip instead of failing. Use installed-model fixtures or make missing imports a hard test failure before relying on these assertions to catch publish-field drift.
🤖 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/integrations/openclaw/plugin/tests/test_state.py` around lines 285 -
305, Update test_allowlist_matches_the_server_model and
test_wire_turn_validates_against_the_server_model to require the server
InteractionData model instead of using pytest.importorskip. Use an
installed-model fixture or otherwise make an unavailable reflexio import fail
the tests, ensuring both assertions detect publish-field drift rather than being
silently skipped.
df5eb84 to
d0232db
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/models/api_schema/common.py`:
- Around line 40-45: Sanitize caller-controlled names at the rendering choke
point before applying length limits in the shown-name construction. Add a local
_bounded helper that replaces non-printable characters with “?”, truncates the
cleaned value to _MAX_NAME_LEN, and use it for each name while preserving the
existing _MAX_REPORTED_NAMES and remaining-count behavior.
- Around line 13-20: Reorder the entries in the module’s __all__ declaration so
the public symbols follow isort-style ordering, placing BlockingIssue before
BlockingIssueKind while leaving the remaining exports unchanged.
In `@reflexio/models/api_schema/domain/entities.py`:
- Around line 901-923: The payload_warnings method’s per-interaction messages
can still produce an unbounded total response and log size. Cap the number of
warning entries returned by payload_warnings, preserving the existing ordering
and adding a final “+N more interactions” summary when entries are omitted;
ensure the skipped-empty summary remains represented appropriately within the
bounded result.
- Around line 887-899: In the validator containing the interaction_data_list
filtering, compute payload_warnings() from the original interaction_data_list
before replacing it with kept, and stash the result for later callers. Preserve
the original list indices and include warnings from wholly mis-keyed
interactions, while retaining the existing shape validation, empty-list
rejection, filtering, and skipped-count behavior.
In `@reflexio/server/routes/interactions.py`:
- Around line 72-82: Sanitize caller-controlled unknown field names in
summarise_unknown_names() before payload_warnings() formats them for the logger.
Remove or encode CR/LF and other log-injection characters while preserving the
existing truncation and count limits, so the warning emitted by the interaction
publish route remains a single safe log record.
🪄 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 Plus
Run ID: b5bb8c81-9fdb-4b12-b117-7b4671fc9811
📒 Files selected for processing (12)
AI_AGENT_INTEGRATION.mdreflexio/client/client.pyreflexio/integrations/openclaw/plugin/src/openclaw_smart/state.pyreflexio/integrations/openclaw/plugin/tests/test_state.pyreflexio/models/api_schema/common.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/api_endpoints/precondition_checks.pyreflexio/server/routes/interactions.pytests/cli/test_helpers.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 (8)
- reflexio/server/api_endpoints/precondition_checks.py
- reflexio/client/client.py
- reflexio/integrations/openclaw/plugin/src/openclaw_smart/state.py
- reflexio/integrations/openclaw/plugin/tests/test_state.py
- tests/server/api_endpoints/test_publish_validation.py
- tests/server/api_endpoints/test_api_routes.py
- AI_AGENT_INTEGRATION.md
- tests/server/api_endpoints/test_precondition_checks.py
d0232db to
5c14c50
Compare
|
All 8 comments triaged — all valid, none dismissed. Two were real bugs, one of which defeated the feature's primary purpose. Pushed as
Not changed, deliberately: nested models use Gates: 4905 OSS + 4368 enterprise tests pass, ruff clean, pyright 0 errors. The one OSS failure ( |
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/client/client.py`:
- Around line 468-470: Update publish_interaction() to collect
request.payload_warnings() after converting the interaction data and merge those
SDK-local dropped-field names into the returned result’s warnings, preserving
any server-provided warnings. Add a regression test covering a valid interaction
with an extra field and assert that the warning identifies the dropped field.
🪄 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 Plus
Run ID: ee0bd2f3-51b7-45ce-b0c1-72fce39c693c
📒 Files selected for processing (13)
AI_AGENT_INTEGRATION.mdreflexio/client/client.pyreflexio/integrations/openclaw/plugin/src/openclaw_smart/state.pyreflexio/integrations/openclaw/plugin/tests/test_state.pyreflexio/models/api_schema/common.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/api_endpoints/precondition_checks.pyreflexio/server/routes/interactions.pytests/cli/test_helpers.pytests/models/api_schema/test_plugin_wire_contract.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 (8)
- reflexio/integrations/openclaw/plugin/src/openclaw_smart/state.py
- AI_AGENT_INTEGRATION.md
- reflexio/models/api_schema/common.py
- reflexio/integrations/openclaw/plugin/tests/test_state.py
- tests/server/api_endpoints/test_api_routes.py
- reflexio/server/api_endpoints/precondition_checks.py
- reflexio/models/api_schema/domain/entities.py
- tests/server/api_endpoints/test_precondition_checks.py
5c14c50 to
507a735
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/client/client.py`:
- Around line 468-470: Update the warnings documentation near payload_warnings()
to state that warnings cover both unrecognized interaction fields whose values
were discarded and empty interactions that were skipped; remove the claim that
every warning represents an unrecognized key.
In `@reflexio/server/routes/interactions.py`:
- Around line 131-147: Validate and sanitize
PublishUserInteractionRequest.request_id before the background publish task logs
it: enforce a bounded, newline-free value and use a safe fallback when
validation fails. Update the request-handling flow and both background publish
logger calls around the existing request_id usage, preserving the identifier for
valid inputs while preventing untrusted content from reaching Sentry logs.
🪄 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 Plus
Run ID: 07fc4e5a-1ce1-4d35-99da-8faa1b3868d3
📒 Files selected for processing (13)
AI_AGENT_INTEGRATION.mdreflexio/client/client.pyreflexio/integrations/openclaw/plugin/src/openclaw_smart/state.pyreflexio/integrations/openclaw/plugin/tests/test_state.pyreflexio/models/api_schema/common.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/api_endpoints/precondition_checks.pyreflexio/server/routes/interactions.pytests/cli/test_helpers.pytests/models/api_schema/test_plugin_wire_contract.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 (11)
- reflexio/integrations/openclaw/plugin/tests/test_state.py
- tests/cli/test_helpers.py
- tests/server/api_endpoints/test_publish_validation.py
- reflexio/models/api_schema/common.py
- reflexio/integrations/openclaw/plugin/src/openclaw_smart/state.py
- tests/server/api_endpoints/test_api_routes.py
- reflexio/server/api_endpoints/precondition_checks.py
- tests/models/api_schema/test_plugin_wire_contract.py
- tests/server/api_endpoints/test_precondition_checks.py
- AI_AGENT_INTEGRATION.md
- reflexio/models/api_schema/domain/entities.py
unpublished_slice dropped four known keys and passed everything else through, so buffer-internal bookkeeping rode onto the wire on every turn. A denylist also rots every time a hook adds a record key, and a server that rejected rather than reported unknown fields would turn that rot into a publish failure this plugin's adapter swallows without advancing its watermark -- so the same batch would retry forever. Scoped honestly: reflexio treats `user_id` as a benign request-level key and never warned about it, so the warning this actually removes is `synthesised_by`, emitted once per session by the SessionEnd anchor. The larger noise source was `tools_used[*].status`, fixed on the server by declaring the field (ReflexioAI/reflexio#383) rather than here. Also carries the buffer's own `ts` across as `created_at`. Without it the server defaults created_at to *parse* time, so a buffer drained hours later -- exactly the offline-resilience case this buffer exists for -- stamped every turn with the drain time instead of when it happened. Contract tests pin the allowlist against the real InteractionData model and assert a literal wire key set. The literal matters: asserting `set(turn) <= _INTERACTION_DATA_FIELDS` compares against the same constant the slicer filters by, so it could never fail -- verified tautological by runtime mutation, and now verified to bite.
507a735 to
6b1e95f
Compare
unpublished_slice dropped four known keys and passed everything else through, so buffer-internal bookkeeping rode onto the wire on every turn. A denylist also rots every time a hook adds a record key, and a server that rejected rather than reported unknown fields would turn that rot into a publish failure this plugin's adapter swallows without advancing its watermark -- so the same batch would retry forever. Scoped honestly: reflexio treats `user_id` as a benign request-level key and never warned about it, so the warning this actually removes is `synthesised_by`, emitted once per session by the SessionEnd anchor. The larger noise source was `tools_used[*].status`, fixed on the server by declaring the field (ReflexioAI/reflexio#383) rather than here. Also carries the buffer's own `ts` across as `created_at`. Without it the server defaults created_at to *parse* time, so a buffer drained hours later -- exactly the offline-resilience case this buffer exists for -- stamped every turn with the drain time instead of when it happened. Contract tests pin the allowlist against the real InteractionData model and assert a literal wire key set. The literal matters: asserting `set(turn) <= _INTERACTION_DATA_FIELDS` compares against the same constant the slicer filters by, so it could never fail -- verified tautological by runtime mutation, and now verified to bite.
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. InteractionData defaults every field and unknown keys were dropped with no trace, so a mis-keyed field yielded a *valid* interaction carrying nothing. 2. 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. 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 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 (user_action-needs-description, image-url-xor-encoding) that previously existed only on the discarded path, and precondition_checks delegates to it so the layers cannot diverge. Two rules deliberately do NOT hard-fail. Stricter versions of both were implemented, reproduced as catastrophic, and reverted -- in each case because the first-party plugins build their wire payload with a denylist and buffer an empty Assistant placeholder unconditionally, while their adapters swallow the error and never advance the publish watermark. The same batch then retries forever and nothing is ever published, for every installed plugin, the moment the server deploys: - Unknown keys are reported, not rejected. They are captured, stripped so they cannot reach storage, and their NAMES (never values, per #377) returned in warnings[]. Nested payload models capture the same way, which surfaces ToolUsed.status -- a field both plugins send on every tool call that was being dropped silently. - An individual empty interaction is skipped, not fatal. A batch where *every* interaction is empty is still a 422, and that is precisely the incident (50 of 50 rows). Warnings are built against the caller's ORIGINAL list, before empty rows are filtered out. Computing them afterwards defeated the feature in its primary case: a mis-keyed content yields an empty interaction, so the row carrying the typo was exactly the row removed and its warning vanished, leaving the caller "skipped 1 empty interaction" with no idea which field was wrong -- and every surviving index was renumbered. The client merges its own locally-detected warnings into the response. publish_interaction builds InteractionData before model_dump(), so the unknown keys are already stripped and the server never sees them; without the merge, unrecognised fields were reported over raw HTTP but invisible through the SDK, which is the primary integration path. Warning output is bounded on three axes (per-name length, names per interaction, total entries) and control characters are stripped, because the names are caller-controlled and reach both the response body and a shared multi-tenant log stream. Unbounded, 1000 interactions produced a ~350 KB warning and one enormous log record; now ~2.5 KB. 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. Also: - Request-level keys callers duplicate per-interaction (user_id, session_id) are stripped but not warned about, so a correct claude-smart batch emits 0 warnings rather than one per turn. - Both background-task log lines withhold their reason string. On the storage path that is an unbounded str(e) from a catch-all, and Sentry ingests ERROR records as event bodies without scrubbing them. - openclaw plugin builds its wire dict from an allowlist of InteractionData fields. The drift guard lives in tests/, not the plugin's own suite: testpaths excludes that directory and no workflow runs it, so an assertion there would never have executed. - AI_AGENT_INTEGRATION.md replaces "hold the watermark on every failure" with a per-status table -- hold on 5xx/timeout/408/429, hold and escalate on 401/403, advance only on 400/422 -- since a blanket hold is what turns a rejection into a permanent wedge. - 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. - ToolUsed gains a declared `status` field. Both plugins already send it ("success"/"error", derived from the tool response); it was being discarded, losing real signal, and once nested capture began reporting unknown keys it became the single largest warning source -- one per tool-bearing turn. Declaring it recovers the data and removes the noise. - The skipped-empty summary is appended AFTER the entry cap, so the cap can never drop it. Capping the combined list swallowed "N interactions were dropped" whenever there were >= 20 unknown-field warnings, which is the one thing the caller most needs to know about such a batch. - The openclaw adapter captures publish_interaction's return value and logs any warnings. It previously discarded the result, so the integration that motivated this work could not see its own dropped fields. - The SDK-side test moved to tests/client/, its mirror location, using the real ReflexioClient constructor instead of __new__ plus stubs; plus a test pinning the premise the client-side merge rests on (the wire payload carries no unknown keys, so warnings cannot double). - ToolUsed gains a declared `status` field, coerced rather than validated. Both plugins already send it ("success"/"error"); it was being discarded, and once nested capture began reporting unknown keys it was the largest single warning source. Declaring it strictly turned five previously- harmless values (int, None, bool, dict, >100 chars) into a 422 for the WHOLE batch, which the plugin adapters swallow without advancing their watermark -- reintroducing the exact stall that made extra="forbid" unacceptable. A mode="before" validator coerces and truncates instead. - request_id is sanitised before it reaches a log line. It is a NonEmptyStr with no length cap and no character restrictions, so a newline in it could forge a line in a shared multi-tenant log stream that Sentry ingests -- the same hazard as the unknown field names, on the value beside them. - The openclaw adapter's warning diagnostics sit outside the try that decides the publish result, and cannot raise. Inside it, a failure while merely formatting a warning would be read as a publish failure and stall the watermark on an already-successful publish. - cap_warning_list returns a copy; the caller appends to it. 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. Unknown fields and individually-empty interactions are still accepted, and are now reported in the response's warnings[].
6b1e95f to
24b2bf8
Compare
unpublished_slice dropped four known keys and passed everything else through, so buffer-internal bookkeeping rode onto the wire on every turn. A denylist rots every time a hook adds a record key, and a server that rejected rather than reported unknown fields would turn that rot into a publish failure this plugin's adapter swallows without advancing its watermark -- so the same batch would retry forever. Scoped honestly: reflexio treats `user_id` as a benign request-level key and never warned about it, so the warning this actually removes is `synthesised_by`, emitted once per session by the SessionEnd anchor. The larger noise source was `tools_used[*].status`, fixed on the server by declaring the field (ReflexioAI/reflexio#383) rather than here. Deliberately does NOT send `created_at`. Carrying the buffer's own `ts` across was implemented and reverted: the extractor's bookmark is keyed on interaction `created_at` (`last_processed_timestamp`, compared with `created_at >= ?`), so a batch recovered after the bookmark had moved was stored and then never seen by the extractor. Reproduced end to end as permanent, silent loss of learning data -- on exactly the offline-recovery path this buffer exists to protect. The server stamping drain time is the lesser evil until ingest ordering stops depending on caller-supplied event time. Contract tests pin the allowlist against the real InteractionData model and assert a literal wire key set. Both details matter: asserting `set(turn) <= _INTERACTION_DATA_FIELDS` compares against the same constant the slicer filters by and could never fail (verified tautological by mutation), and the model check is a subset assertion because the plugin is deliberately forward-compatible -- it may know fields the pinned `reflexio-ai` release has not caught up to, which is why equality broke CI.
unpublished_slice dropped four known keys and passed everything else through, so buffer-internal bookkeeping rode onto the wire on every turn. A denylist rots every time a hook adds a record key, and a server that rejected rather than reported unknown fields would turn that rot into a publish failure this plugin's adapter swallows without advancing its watermark -- so the same batch would retry forever. Scoped honestly: reflexio treats `user_id` as a benign request-level key and never warned about it, so the warning this actually removes is `synthesised_by`, emitted once per session by the SessionEnd anchor. The larger noise source was `tools_used[*].status`, fixed on the server by declaring the field (ReflexioAI/reflexio#383) rather than here. Deliberately does NOT send `created_at`. Carrying the buffer's own `ts` across was implemented and reverted: the extractor's bookmark is keyed on interaction `created_at` (`last_processed_timestamp`, compared with `created_at >= ?`), so a batch recovered after the bookmark had moved was stored and then never seen by the extractor. Reproduced end to end as permanent, silent loss of learning data -- on exactly the offline-recovery path this buffer exists to protect. The server stamping drain time is the lesser evil until ingest ordering stops depending on caller-supplied event time. Contract tests pin the allowlist against the real InteractionData model and assert a literal wire key set. Both details matter: asserting `set(turn) <= _INTERACTION_DATA_FIELDS` compares against the same constant the slicer filters by and could never fail (verified tautological by mutation), and the model check is a subset assertion because the plugin is deliberately forward-compatible -- it may know fields the pinned `reflexio-ai` release has not caught up to, which is why equality broke CI.
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.** `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: ``` bool(UserActionType.NONE) = True validate_publish_user_interaction_request(50 empty) -> (True, '') ``` 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_passes` diagnosed the dead branch in its own comment, then asserted `valid is True`. That test is now inverted. **2. Two sibling rules lived only on a discarded path.** `user_action` needs a description; `interacted_image_url` xor `image_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_interaction` runs inside a `BackgroundTask` whose return value was dropped, after the caller already got `200 "queued"`. A `success=False` isn't an exception, so it wasn't logged either. ## The fix The rules live in a `model_validator` on `PublishUserInteractionRequest` — it runs during request parsing, so it applies on **both** the sync and 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. 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), and `request_id` is sanitised before reaching them — it is a `NonEmptyStr` with 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. `TestEmptyInteractions` and `TestSiblingRulesEnforcedAtBoundary` pin the 422s on the async path — the one that previously returned 200 "queued" and then silently refused the write. `TestLegitimateTurnsStillAccepted` is driven from `CONTENT_BEARING_FIELD_NAMES` **plus 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 are `InteractionData` fields, and asserting 200. Fixed the data, not the assertion. `ruff` clean, `pyright` 0 errors, full OSS + enterprise suites green. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved publish interaction validation: detects contradictory field combinations and reports the failing interaction index. - Empty placeholder interactions are now skipped; requests with fully empty interaction batches are rejected. - **Bug Fixes** - Publish endpoint now correctly accepts plugin-style per-turn payloads (extra keys alongside role/content). - Async publish now rejects contentless interaction turns with HTTP 422. - Background publishing failures are logged with safer, non-sensitive messages. - **Tests** - Added regressions for boundary validation, empty-interaction skipping/rejection, plugin wire shape, and request-id log sanitization. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
Closing in favour of two focused PRs. This branch had grown to +1290/−68 across 15 files, mixing an incident fix with a new feature. Reviewing it as one change is what let five consecutive rounds each find a real regression — The incident fix shipped as #384 (merged, The feature — telling callers which field they mis-keyed — is being rebuilt fresh off Nothing is lost — every finding from this branch's review history is carried into either #384 or the rebuild. Keeping it open would just leave a diff that appears to revert merged fixes. |
…rity) (ReflexioAI#300) ## Summary **SEC-016** — cross-backend GDPR-erase parity on `lineage_event`. On user-data erase, **SQLite hard-DELETEd** `lineage_event` rows referencing erased entities, while **Supabase retains** the content-free lineage skeleton (its erase path never enumerates `lineage_event` — it's in neither the purge nor the delete set; the ReflexioAI#219/ReflexioAI#383 content-purge design keeps a content-free skeleton and purges PII on the entity tables). This converges SQLite onto the intended model. ## Change - **Stop deleting `lineage_event` on erase** (`sqlite_storage/governance/_erase_execution.py`). Safe because `lineage_event` has **no PII/content column, no foreign keys, no FTS/vec shadow tables** — retaining the row is exactly the content-free skeleton the design intends, and lineage reconstruction is *helped* (the retained `status_change` signal survives). Removed the delete block + now-dead `erased_entity_ids`/`request_ids`/`import json`. Entity-table deletes + `purge_content` calls are untouched. - **Inverted the test** that codified the old behavior (`test_governance_storage.py`): `test_apply_governance_user_data_delete_retains_lineage_skeleton` now asserts the pre-seeded skeleton row **still exists** (`== 1`) after erase, instead of `== 0`. ## Note on scope The retained values are opaque surrogate IDs (`profile_id`/`request_id`/`entity_id`) in a content-free table — this matches the **already-shipped Supabase behavior**, so this is a convergence, not a new retention decision. (If those internal IDs should ever be treated as PII, that's a separate cross-backend change requiring Supabase to scrub too.) ## Verification `governance/erase/lineage/purge` tests: **473 passed**; full sqlite storage suite: **390 passed**; ruff + pyright clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated data-deletion behavior so non-sensitive lineage “skeleton” records are preserved after governance erasure, while content-bearing user data is still removed. * Aligned deletion handling with expected behavior to avoid removing valid lineage context tied to unrelated records. * **Tests** * Revised coverage to verify that retained lineage records remain after user data deletion. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
The bug
A publish of 50 interactions returned 200 OK and stored 50 rows with
content = ''. No profiles were generated, and nothing on any layer reported a problem. Fleet-wide check: exactly one org affected; every other org with interactions had zero empty content and non-zero profiles. A validation gap, not a pipeline bug.Three defects combined to hide it
1. Unknown keys vanished without trace. Every
InteractionDatafield has a default, so a mis-keyed field produced a valid interaction carrying nothing. The tell-tale in storage:rolecame back as the literal default"User"though"user"was sent. When a field you set reads back as its default, nothing in that object bound.2. 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. Now inverted.3. On the async path the rejection is discarded.
add_user_interactionruns in 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 per-interaction rules live in a
model_validatoronPublishUserInteractionRequest— it runs during request parsing, so it 422s on both the sync and background-task path.InteractionData.validation_error()holds all three per-interaction rules in one place andprecondition_checksdelegates to it. That closes the class: the other two rules (user_actionneeds a description;interacted_image_urlxorimage_encoding) previously existed only on the discarded path, so they returned 200 and then silently refused the write.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. Text is stripped, so" "is not content.extra="forbid"was implemented, then reverted — this is the important partThe first revision forbade unknown keys. Review reproduced that this breaks 100% of publishes from both first-party plugins, and does so in the worst possible way.
Both plugins build their wire payload with a denylist (drop three known keys, pass the rest through), so every turn carries request-level bookkeeping such as
user_id:Under
forbidthat raises — and the failure is invisible and self-perpetuating:claude-smart POSTs raw JSON, so already-installed plugins would break the moment the server deployed, before any upgrade. That converts partial data loss into total, permanent loss — strictly worse than the bug being fixed.
So unknown keys are reported, not rejected: captured, stripped so they cannot reach storage, and their names (never values, per the #377 redaction invariant) returned in
warnings[]and logged once server-side. Nested payload models capture the same way, which surfacesToolUsed.status— a field both plugins send on every tool call that was being dropped silently.The emptiness rule also had to soften — same wedge, different rule
Verification then reproduced that a per-interaction emptiness 422 wedges the plugins identically. Both append an empty
Assistantplaceholder unconditionally, so one empty row rejected the batch containing the real user turn:So an individual empty interaction is skipped and reported; a batch where every interaction is empty is still a 422 — which is exactly the incident (50 of 50). Verified: the real turn now publishes, with
warnings: ["skipped 1 empty interaction(s)…"].Bounded and de-noised
user_id,session_id) are stripped but not warned about. A correct 50-turn claude-smart batch went from 50 warnings to 0.The contentless 422 alone still catches the original incident — that payload was fully-defaulted empty objects.
Also in this PR
InteractionDatafields, with a contract test pinning the allowlist to the real model and asserting every emitted turn validates. The denylist rots every time a hook adds a record key.response.message— on the storage path that is an unboundedstr(e)from a catch-all, and Sentry ingests ERROR records as event bodies without scrubbing them.AI_AGENT_INTEGRATION.mdno longer tells integrators to hold the watermark on every failure. That guidance is precisely what turns a 4xx into a permanent wedge; it now splits retryable from non-retryable and documents the allowlist rule.fix(api)!:+BREAKING CHANGE:footer. Verified against the installed parser that the previousBreaking:line yieldedbump: patch, breaking_descriptions: []— it would have shipped a breaking change as 0.2.29 with no release-note signal. There is no CHANGELOG in this repo, so the footer is the only place it gets recorded.Tests
Written RED first (8 failures against the old source), including the inverted characterization test.
TestLegitimateTurnsStillAcceptedis driven fromCONTENT_BEARING_FIELD_NAMES, so a field cannot be added to the predicate without acceptance coverage —citationsandretrieved_learningspreviously had none (a mutation dropping both killed zero tests).input.user_message/agent_message/interaction_typeand asserting 200. Fixed the data, not the assertion.4898 OSS + 4368 enterprise tests pass, 0 failures. 146 openclaw plugin tests pass. ruff clean, pyright 0 errors across all changed files including tests.
Coverage hole the verifier caught
The
citations/retrieved_learningsacceptance test was self-referential — parametrized from the same tuple that drives the predicate, so deleting a field deleted its own coverage. A mutation dropping both killed zero tests. There is now an independent pinned-set assertion as the anchor.Companion PRs: ReflexioAI/claude-smart#144 (same denylist→allowlist fix for the other plugin) and enterprise #849 (public docs + submodule repin).
Summary by CodeRabbit
warnings(field names only), including nested cases, with bounded and sanitized output.