fix(deriver): normalize PromptRepresentation.explicit string shapes#538
Open
AmilGael wants to merge 1 commit intoplastic-labs:mainfrom
Open
fix(deriver): normalize PromptRepresentation.explicit string shapes#538AmilGael wants to merge 1 commit intoplastic-labs:mainfrom
AmilGael wants to merge 1 commit intoplastic-labs:mainfrom
Conversation
Some OpenAI-compatible structured-output backends occasionally emit
`explicit` as a bare string or a list of strings instead of the declared
list of `{"content": str}` objects. Today this triggers Pydantic
validation failure in the deriver path and the entire batch falls
through to the empty-fallback representation, dropping otherwise usable
observations.
Extend the existing before-validator on `PromptRepresentation.explicit`
to normalize:
- `"User works remotely"` -> `[{"content": "User works remotely"}]`
- `["A", "B"]` -> `[{"content": "A"}, {"content": "B"}]`
- `["A", {"content": "B"}]` -> mixed strings and dicts coexist
- blank / whitespace-only strings are dropped
- `None` -> `[]` (preserved)
Anything else is passed through unchanged so Pydantic still raises its
normal validation error for genuinely malformed input.
Adds seven unit tests covering each variant plus a passthrough case for
already well-formed dict input.
Closes plastic-labs#524
By Gamaliel — coworking with Claude Code
Contributor
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/deriver/test_representation_crud.py (1)
111-169: Add one negative test to lock malformed-input behavior.These tests cover accepted shapes well, but there isn’t a regression test confirming unsupported shapes still raise validation errors (e.g.,
{"explicit": 123}).✅ Suggested test addition
import datetime +import pytest +from pydantic import ValidationError + from src.utils.representation import ( DeductiveObservation, ExplicitObservation, ExplicitObservationBase, @@ def test_prompt_representation_proper_dicts_explicit_pass_through(): """Existing behavior: a well-formed dict list still validates unchanged.""" pr = PromptRepresentation.model_validate( {"explicit": [{"content": "A"}, {"content": "B"}]} ) assert [e.content for e in pr.explicit] == ["A", "B"] + + +def test_prompt_representation_invalid_explicit_shape_raises(): + """Unsupported top-level shapes should still fail normal validation.""" + with pytest.raises(ValidationError): + PromptRepresentation.model_validate({"explicit": 123})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/deriver/test_representation_crud.py` around lines 111 - 169, Add a negative test that ensures invalid shapes for the explicit field raise validation errors: create a new test (e.g., test_prompt_representation_rejects_invalid_type_explicit) that calls PromptRepresentation.model_validate({"explicit": 123}) and asserts that a validation exception is raised (use pytest.raises with the appropriate ValidationError type used in the project). Place the test alongside the other test_* functions in tests/deriver/test_representation_crud.py so malformed-input behavior (unsupported types like integers) is locked in.src/utils/representation.py (1)
124-142: Consider converting this validator docstring to Google style sections.The content is clear, but it does not follow the repository’s preferred Google docstring structure.
As per coding guidelines, "Use Google style docstrings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/representation.py` around lines 124 - 142, Update the validator docstring in src/utils/representation.py (the validator that "Normalize loose shapes emitted by OpenAI-compatible providers") to use Google style sections: add a short summary line, then separate "Args:", "Returns:" (or "Yields:" if appropriate), and "Raises:" as needed, and convert the bullet examples into a "Examples:" or an "Examples" subsection under the description; preserve all behavioral details (None -> [], string -> list of dicts, list coercions, dropping blank strings, and pass-through behavior) and keep the explanatory note about when non-str/list types are left for Pydantic to validate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/representation.py`:
- Around line 151-152: The code currently swallows None entries by doing "if
item is None: continue" when building the explicit list; instead, treat None as
a schema validation error: locate the block containing the "if item is None:
continue" check (working with the explicit list construction) and replace the
silent continue with an explicit raise (e.g., ValueError or a custom
ValidationError) that includes context about the offending item and the list
name ("explicit"); ensure the function that constructs/validates "explicit"
propagates this error so malformed payloads fail fast.
---
Nitpick comments:
In `@src/utils/representation.py`:
- Around line 124-142: Update the validator docstring in
src/utils/representation.py (the validator that "Normalize loose shapes emitted
by OpenAI-compatible providers") to use Google style sections: add a short
summary line, then separate "Args:", "Returns:" (or "Yields:" if appropriate),
and "Raises:" as needed, and convert the bullet examples into a "Examples:" or
an "Examples" subsection under the description; preserve all behavioral details
(None -> [], string -> list of dicts, list coercions, dropping blank strings,
and pass-through behavior) and keep the explanatory note about when non-str/list
types are left for Pydantic to validate.
In `@tests/deriver/test_representation_crud.py`:
- Around line 111-169: Add a negative test that ensures invalid shapes for the
explicit field raise validation errors: create a new test (e.g.,
test_prompt_representation_rejects_invalid_type_explicit) that calls
PromptRepresentation.model_validate({"explicit": 123}) and asserts that a
validation exception is raised (use pytest.raises with the appropriate
ValidationError type used in the project). Place the test alongside the other
test_* functions in tests/deriver/test_representation_crud.py so malformed-input
behavior (unsupported types like integers) is locked in.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ca9c7d1-c510-4813-9a2e-070a54ff1a99
📒 Files selected for processing (2)
src/utils/representation.pytests/deriver/test_representation_crud.py
Comment on lines
+151
to
+152
| if item is None: | ||
| continue |
Contributor
There was a problem hiding this comment.
Don’t silently drop None entries inside explicit lists.
At Line 151/Line 152, None items are skipped, which broadens tolerance and can hide malformed payloads that should fail normal schema validation.
🔧 Proposed fix
if isinstance(v, list):
normalized: list[Any] = []
for item in cast(list[Any], v):
- if item is None:
- continue
if isinstance(item, str):
stripped = item.strip()
if stripped:
normalized.append({"content": stripped})
continue
# dicts, ExplicitObservationBase instances, etc. pass through
# to normal Pydantic validation.
normalized.append(item)
return normalized🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/representation.py` around lines 151 - 152, The code currently
swallows None entries by doing "if item is None: continue" when building the
explicit list; instead, treat None as a schema validation error: locate the
block containing the "if item is None: continue" check (working with the
explicit list construction) and replace the silent continue with an explicit
raise (e.g., ValueError or a custom ValidationError) that includes context about
the offending item and the list name ("explicit"); ensure the function that
constructs/validates "explicit" propagates this error so malformed payloads fail
fast.
Author
|
@ajspig, I forgot to tag you, apologies - Gamaliel from Pursuit |
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.
Summary
Closes #524.
Some OpenAI-compatible structured-output backends occasionally emit
PromptRepresentation.explicitas a bare string or a list of strings instead of the declaredlist[{"content": str}]. Today that causes Pydantic validation to fail and the entire deriver batch falls through to the empty-fallback representation, silently dropping otherwise usable observations.This PR extends the existing before-validator on
PromptRepresentation.explicit(insrc/utils/representation.py) to coerce the loose shapes called out in the issue:"User works remotely"[{"content": "User works remotely"}]["A", "B"][{"content": "A"}, {"content": "B"}]["A", {"content": "B"}]" "/""in a listNone[](preserved)Anything that is neither
None,str, norliststill passes through to normal Pydantic validation, so genuinely malformed input continues to raise.Why this, why now
Issue #524 is tightly scoped by its author ("not about vector dimensions, embedding provider config, queue flushing, token accounting, or broad JSON repair policy changes"), and the failure mode is concrete and user-visible: self-hosted Honcho users on OpenAI-compatible providers lose a whole batch of observations whenever the model drifts into a string-valued emission. The fix is a narrow Pydantic normalization at the validator boundary — it doesn't widen the declared schema and doesn't touch any other code path.
What changed
src/utils/representation.py— renamedconvert_none_to_empty_listtonormalize_explicitand extended it with the string / list-of-string / mixed-list / blank-string rules described above.None -> []behavior is preserved. Addscastto thetypingimport to satisfybasedpyright.tests/deriver/test_representation_crud.py— adds seven unit tests covering each new variant plus a pass-through test for already well-formed dict input, and a test confirming the existingNone -> []behavior still holds.Out of scope (intentionally)
Per the issue, this PR does not touch: vector dimensions, embedding provider configuration, queue flushing / backlog claiming, token usage accounting, or broad JSON repair policy changes. All of those deserve their own discussions.
Test plan
uv run pytest tests/deriver/test_representation_crud.py— 10 passed (3 pre-existing + 7 new)uv run pytest tests/deriver/test_representation_crud.py tests/integration/test_representation.py— 19 passeduv run ruff check src/utils/representation.py tests/deriver/test_representation_crud.py— cleanuv run ruff format --check src/utils/representation.py tests/deriver/test_representation_crud.py— already formatteduv run basedpyright src/utils/representation.py tests/deriver/test_representation_crud.py— 0 errors, 0 warningsBy Gamaliel — coworking with Claude Code
Summary by CodeRabbit
New Features
Tests