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.
FEAT Embed schema in SelfAskRefusalScorer #1432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
FEAT Embed schema in SelfAskRefusalScorer #1432
Changes from all commits
f2041d8de6309f7e926dda137629760211fedda2ea8ce6a54e0fef7304795e5369fc3664a0aa2ea36b755a9ce129790be1b50086fa882071f0b43ed1504ce322f6dbc217708fefa5675c6aecafa57f06879b7fee089f7935871f2bd5e55999a6492e73b97993f9a51e5f15fc4ee2c6cd78d44accdc3f2ebd78f828ff8bdb47c607d2cf6df16ba5210b07847ecd0b330587121f23d7ed0dde653724cb5eb960ed2ec9f8cb776530a61417f5b2b3a225f7daf8aFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so really it's a
dict[str, Any] | None?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using
Optionalto follow L38 above. For the actual dict, I was following:PyRIT/pyrit/common/json_helper.py
Line 15 in 6f591a8
which forces a
json.loadcall to adict[str, str]. I would agree thatdict[str, Any]might be more appropriate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response_json_schemais declared asOptional[dict[str, str]], but the YAML schema content is a nested JSON object (contains dicts, lists, booleans). Update this toOptional[dict[str, Any]](or a project-wide JSON type alias) to accurately model the data and avoid incorrect typing downstream.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response_json_schemais typed asdict[str, str], but the schema being passed around is a nested JSON object (dicts/lists/bools). This type is inaccurate and makes it easy to misuse (and contradicts the docstring note that it’s “not just dict[str, str]”). Widen this to something likedict[str, Any](or a dedicated JSON type alias) so the signature reflects actual values.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response_json_schemais typed asOptional[dict[str, str]], but JSON Schemas (and the YAML being added) contain nested dict/list/bool values. This type hint is incorrect and will either force unsafe casts or break type checking; useOptional[dict[str, Any]](or a shared JSON type alias) for this parameter and update the docstring accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring says the schema is used "to validate the response against", but
_score_value_with_llmonly forwards schema metadata to the target (which may constrain generation) and does not perform any local JSON Schema validation of the returned payload. Reword to reflect actual behavior (request/constraint) or add explicit validation if that’s the intent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check
if response_json_schema:will skip schemas that are valid but falsey (e.g., an empty{}), which can make behavior depend on schema contents rather than whether it was provided. Prefer an explicitis not Nonecheck so “provided but empty” is still passed through consistently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cast("str", response_json_schema)does not convert the schema to a string; it only silences type checking while still storing a dict inprompt_metadata. This is misleading and makes the metadata type contract unclear. Prefer serializing withjson.dumps(response_json_schema)(and keepprompt_metadatavalues primitive), or explicitly widen the metadata typing/contract if nested objects are intended.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prompt_metadata["json_schema"] = cast("str", response_json_schema)doesn’t actually serialize the schema to a string (it remains a dict at runtime), and it also makes the metadata dict’s declared value type (str | int) inaccurate. If you want metadata to remain scalar, JSON-serialize the schema (e.g.,json.dumps(...)) and store it as a string; otherwise, widenprompt_metadata’s type to allow JSON objects and avoid the misleading cast.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces new behavior (passing a JSON schema via
prompt_metadata) but there’s no unit test asserting that the outgoingMessagePiece.prompt_metadataincludes the expectedjson_schemawhenresponse_json_schemais provided. Adding a test intests/unit/score/test_scorer.py(or the refusal scorer tests) that inspectschat_target.send_prompt_asynccall args would prevent regressions in schema plumbing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment notes the schema is a “full JSON object”, but the type flowing from
SeedPrompt.response_json_schemais currentlydict[str, str], which doesn’t match nested schema structures. Once the SeedPrompt field type is widened, consider annotatingself._response_json_schemaaccordingly (e.g.,Optional[dict[str, Any]]) to keepSelfAskRefusalScorer’s internal state and identifier params consistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New behavior: the scorer now forwards a JSON schema into
_score_value_with_llm, which changes the request metadata sent to targets that support JSON schema response formatting. Add/extend a unit test (e.g., intests/unit/score/test_self_ask_refusal.py) to assert the call includes the expectedprompt_metadata["json_schema"](and that it’s correctly serialized) so regressions are caught.Uh oh!
There was an error while loading. Please reload this page.