Repository navigation
fix(redteam): carry run stats to the report via environment_state part of RedTeam Graduation Part 1 - #415
fix(redteam): carry run stats to the report via environment_state part of RedTeam Graduation Part 1#415nhungbi wants to merge 4 commits into
Conversation
|
@strandly-the-agent review the changes |
There was a problem hiding this comment.
Approach checks out: environment_state is the only task-output channel the base Experiment keeps (experiment.py:285-293), and test_cached_rerun_keeps_run_stats drives a real strategy through the real task, store and report — cache replay now keeps run stats, which main loses. Three 🟡s, no blockers; not approving while it's draft / do-not-merge.
Headline (inline):
report.py:367—"run_results"is reserved in the user'senvironment_statenamespace with no type guard; a custom task using that name crashesattack_results()after the paid attack run (repro'd). One-line fix inline.strategies/base.py:38—to_metadata()allowlists 3 keys, soGoatStrategy(store_reasoning=True)(documented to emitmetadata["reasoning_trace"]) becomes a no-op from the user's seat. Passself.metadatathrough.task.py:152— the disclosed trade-off turns a loud error (mainraises foruses_environment_state=True) into a silent judgment overpruned_branches, and nothing tests it.
Design question (non-blocking, but #316 says the custom task becomes the primary path): a custom task today must hand-write EnvironmentState(name="run_results", state={"turns_used": …, "backtracks": …, "pruned_branches": …}) from a string literal that lives only in a warning message. Would an exported constant + one helper on AttackRunResult be simpler — details below?
Proposed shape (fixes 1–3's root cause together; no import cycle — strands_evals.types imports no redteam module)
# strategies/base.py
RUN_RESULTS = "run_results" # export from strands_evals.experimental.redteam
class AttackRunResult:
def to_environment_state(self) -> EnvironmentState:
"""The task-output entry `RedTeamReport` reads run stats from."""
return EnvironmentState(name=RUN_RESULTS, state={**self.metadata, "pruned_branches": self.pruned_branches})A custom task then becomes return {"output": result.conversation, "trajectory": list(session.trace), "environment_state": [result.to_environment_state()]}; report.py:366 compares against RUN_RESULTS, and the deprecation message points at the helper instead of spelling out the literal. to_X siblings in this repo all return an X (to_dict, to_bytes, to_data_url, to_file); to_metadata() is the only one returning a subset of a field.
✅ What I verified at bce80b19 (vs base 9de7900)
python -m pytest tests/strands_evals/experimental/redteam/ -q→ 358 passed (two independent runs, 83s / 103s).-W error::DeprecationWarningon the four touched test files: no unexpected warnings.ruff check src tests+ruff format --checkclean.mypy -p src→ 5 errors, all pre-existing and outsideredteam.actual_environment_statereaches report rows as plainname/statedicts (experiment.py:593→model_dump()), sostate.get("name")is safe on the live path.- Error path holds: when
run_attackraises, the row comes fromcase.model_dump()(experiment.py:615) with noactual_environment_state→ fallback tometadata→ statsNone,errored=True, no crash (repro, not reading). - Cache round-trip holds:
LocalFileTaskResultStoreismodel_dump_json/model_validate_json;to_metadata()output is JSON-safe.mainreplay givesturns_used=None; this branch retains it. run_metawarns and still merges;run_resultswins when both are present. Deprecation idiom matches the house one (mappers/cloudwatch_session_mapper.py:60-66, AGENTS.md).AttackSuccessEvaluatornever readsactual_environment_state(zero hits); judge exposure is opt-in viauses_environment_state=True(defaultFalse).- No stale
run_metaoutsidereport.py/test_report.py; README has notask=section yet (author says follow-up);tests_integ/has no redteam coverage (tracked in #316). - Artifacts uploaded under
strands-agents/evals/pr/415/:pytest-redteam.log,repro_name_collision.py/.out,probe_env_state_prompt.py/.out,repro_error_path.py/.out,repro_415.py/repro_415_output.txt.
Questions (non-blocking)
- With #410 (
TaskOutput.metadata) closed unmerged, isenvironment_statethe deliberate long-term home for run stats or a stopgap? The ~2-line base alternative is readingtask_output.get("metadata")intoEvaluationData.metadataatexperiment.py:285-293, which keeps bookkeeping out of the evaluator-input channel. Worth deciding before graduation rather than after; not asking you to grow this PR. - The "unnamed cases" motivation doesn't hold on the
RedTeamExperimentpath:_expand_cross_product(experiment.py:137) always setsitem.name = f"{case.name}__{label}", socase.nameis the string"None__crescendo", neverNone. The custom-task motivation stands alone; flagging so it doesn't land in a changelog. turns_used/backtracks/pruned_branchesleavecases[i]["metadata"]into_dict()/to_file()output with no shim — worth one line in the deprecation message or README?- This repo has no
needs-api-reviewlabels;designis the nearest fit for a change to what a custom task must return and what a report row carries.AttackRunResultis in__all__, soto_metadata()is public surface.
Reading order for human reviewers
strategies/base.py:38 (what gets captured) → task.py:152 (where it's attached) → report.py:363 (where it's read, and the one reachable defect) → experiment.py (pure deletion).
Appendix — non-blocking (3)
report.py:368— themetadatafallback means a user-authoredRedTeamCase(metadata={"turns_used": 99})is reported as a measured stat when the task returns norun_results. Pre-existing (mainread the same key); noting it because the fallback is now load-bearing for every custom task.report.py:99—run_meta={}warns on a no-op empty dict (is not Nonevs truthiness). Cosmetic.base.py:41-42—to_metadata()emits explicitNones for strategies that record neither key; they land in the serialized report and (per the trade-off) in judge prompts. Harmless, slightly noisy.
|
@strandly-the-agent review the changes |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Follow-up on 1d04b66 (+ main merge, 9db588f): all three earlier findings are fixed and I couldn't break the fixes. Approving the code; still held per your do-not-merge banner.
- Fixed
report.py:367—isinstanceguard, and the reserved name is nowredteam_run_resultsso a user's ownrun_resultsstate can't collide. Repro'd: both a same-named foreign state and a non-dict state under the reserved name yield empty stats, no crash. - Fixed
strategies/base.py:38—to_environment_state()passesself.metadatathrough;GoatStrategy(store_reasoning=True)'sreasoning_tracereaches the report row again (repro'd). - Fixed
task.py:152—test_environment_state_evaluator_sees_run_resultspins the trade-off. RUN_RESULTSexported fromstrands_evals.experimental.redteam; deprecation message now points at the helper. Resolves the magic-string question.
✅ Verified at 9db588f
python -m pytest tests/strands_evals/experimental/redteam -q -W error::DeprecationWarning→ 360 passed.ruff check+ruff format --checkclean;mypy src/strands_evals/experimental/redteam→ no issues (35 files).- New import edge
report.py → strategies/base.py → strands_evals.typeshas no cycle: importingreportfirst andstrategies.basefirst both succeed. - Artifacts:
pytest-redteam-followup-9db588f.log,repro_followup_415.py/.out.
Still open, non-blocking: whether environment_state is the long-term home vs a TaskOutput.metadata channel (#410) — a decision for #316, not this PR. run_meta={} still warns on a no-op empty dict (cosmetic).
|
|
||
| The state holds the strategy's `metadata` plus `pruned_branches`. | ||
| """ | ||
| return EnvironmentState(name=RUN_RESULTS, state={**self.metadata, "pruned_branches": self.pruned_branches}) |
There was a problem hiding this comment.
Issue: to_environment_state() emits the entire metadata dict ({**self.metadata, "pruned_branches": ...}), but RedTeamReport._run_results() only reads three keys: turns_used, backtracks, pruned_branches. For GOAT/Crescendo, metadata also carries target_calls, parse_failures, attacks_used, and — when store_reasoning=True — the full per-turn reasoning_trace (the attacker's chain-of-thought).
Because the base Experiment copies this into actual_environment_state, and compose_test_prompt(..., uses_environment_state=True) stringifies the whole thing into the judge prompt, any environment-state-aware evaluator (e.g. OutputEvaluator) now sees the attacker's internal reasoning. That risks biasing the judge (it reveals how the attack was constructed) and inflates token usage. The PR's trade-off note only mentions "run stats" reaching the judge, not reasoning traces.
Suggestion: Emit only what the report consumes, e.g. state={"turns_used": self.metadata.get("turns_used"), "backtracks": self.metadata.get("backtracks"), "pruned_branches": self.pruned_branches}. If passing full metadata through is intentional, please document the judge-prompt exposure explicitly in the docstring and the PR trade-off section so downstream users of uses_environment_state=True evaluators aren't surprised.
There was a problem hiding this comment.
We pass the full metadata through because it's documented as free-form, and public options like GoatStrategy(store_reasoning=True) rely on their keys reaching the report and to_file(). An allowlist turns those options into silent no-ops and forces an edit to to_environment_state() for every new strategy field. Filtering also no longer protects anything: the old key-collision risk went away with the case/run metadata merge, and the judge only sees this data when an evaluator opts in with uses_environment_state=True, which we'll document.
|
Issue: This PR adds new public symbols to Suggestion: Since this establishes the public contract for how custom tasks feed run stats into the report, consider adding |
|
Issue: The PR description is out of sync with the implementation. It states " Suggestion: Update the description to reference |
|
Assessment: Comment Solid, well-tested refactor that removes the fragile Review themes
Nice simplification overall — dropping the keyed side channel in favor of the task's own |
|
@strandly-the-agent the issue / comments sounds trivial right? |
|
Mostly, yes. Two of the three bot comments are stale or don't apply here, and the third is a real design point that's already been decided and documented.
My approval at |
Important
Review only — please do not merge yet. This is part of the run-pattern work tracked in #316. Holding the merge.
Description
RedTeamReportgets each case's turns, backtracks and blocked branches from arun_metadict. Only the built-in attacker task writes to it, keyed by case name, andRedTeamExperimentthen merges it into the report. That side channel loses the stats in these cases:task=passed toRedTeamExperimentcan't reachrun_meta, so its turns and blocked columns come out empty.case.namewas set.This PR moves the stats into the task's own output, using the base
Experiment's existingenvironment_statechannel, so no core change is needed:RUN_RESULTSconstant ("redteam_run_results"), exported fromexperimental.redteamandexperimental.redteam.strategies. It names theenvironment_stateentry the report reads.AttackRunResult.to_environment_state()returnsEnvironmentState(name=RUN_RESULTS, state={**metadata, "pruned_branches": pruned_branches}). Strategy-specific keys (such as GOAT'sreasoning_trace) are kept as they are. A custom task that returns"environment_state": [result.to_environment_state()]gets the same report columns as the built-in task._run_attackreturns that entry. The baseExperimentcopies it intoEvaluationData.actual_environment_state, so it reaches every report row and the evaluation data cache.RedTeamReport.attack_results()readsturns_used,backtracksandpruned_branchesfrom the row'sRUN_RESULTSentry and ignores other entries:metadata. This covers older serialized reports and callers that still passrun_meta.RedTeamExperimentno longer keepsself._run_meta, and therun_metaparameter is removed from the private task builders.RedTeamReport.from_evaluation_report(run_meta=...)still works but emits aDeprecationWarningthat points toto_environment_state(). This follows the deprecation-cycle rule forexperimental.redteam. Calls withoutrun_metadon't warn.Trade-off: evaluators with
uses_environment_state=True(such asOutputEvaluator) now get the wholeRUN_RESULTSentry in their judge prompt. That includes the fullpruned_branchesand any strategy-specific metadata. Before this PR, such an evaluator raised on a red team run because there was no environment state.AttackSuccessEvaluatordoesn't read environment state, so it isn't affected.Related Issues
Part of #316 (update public run pattern: a custom task should get the same report as the built-in one).
Documentation PR
None yet. The custom-task README section that uses this will come in a follow-up PR.
Type of Change
Bug fix
Testing
test_strategies.py:to_environment_state()usesRUN_RESULTS, keeps strategy-specific metadata, and addspruned_branches(empty when none were recorded).test_task.py: both task paths (shared target and per-case factory) return theRUN_RESULTSentry; attack errors still propagate.test_report.py: stats come from theRUN_RESULTSentry, not same-named metadata or other entries; a non-dict state gives empty stats; fallback tometadatawhen the entry is absent;run_metawarns and still merges; no warning withoutrun_meta.test_experiment.py: a custom task that returnsto_environment_state()fills the turns and blocked columns; a cached rerun keeps them; an evaluator withuses_environment_state=Truesees the run stats in its prompt (pins the trade-off above).I ran
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.