-
Notifications
You must be signed in to change notification settings - Fork 667
[BREAKING] FEAT add TAP to content harms scenario#1378
[BREAKING] FEAT add TAP to content harms scenario #1378hannahwestra25 wants to merge 13 commits intoAzure:main from
Conversation
...dd_tap_to_content_harms
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.
If these were released that way it could be breaking.
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.
Is this a display name or something else? Do we have an established casing rule? Asking because I want to know, not to criticize 🙂
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.
Yep for display. I changed it to one word because that's what the red team agent scenario does (which is the only other 2+ word scenario) but it also matches the scenario class name. I'm indifferent about whether its camelcase or spaces between words but want it to be 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.
If you want it to match class name why don't we use that rather than setting custom strings that could diverge?
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.
Having this as list rather than types to the strategy seems worse? "harm strategy" sounds dangerous 😆
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.
what do you mean "this as a list rather than types to the strategy seems worse"? are you talking about the strategy type or the seed_groups type ? this is just a comment change but strategy is just the name of the harm which then is used for the atomic attack name so maybe I rename the variable to strategy_name at least if that's what you're referring to 😅
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.
Generally, having it typed is better than not (or just "string" when you actually just want one from a specific set of literals). That's essentially the point.
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.
does this break people who are using it in all caps from the last release?
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.
yes good call!
...dd_tap_to_content_harms
...dd_tap_to_content_harms
Uh oh!
There was an error while loading. Please reload this page.
Description
Tests and Documentation
added / updated tests