Repository navigation
Conversation
|
@strandly-the-agent review the changes |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Changes requested — no blockers; two API-shape gaps in the new surface and some doc drift. Both features work as claimed (pure move of _build_session, _turn_cap resolves correctly, 4/4 mutants killed). The two things worth fixing before un-drafting: as_target_session can't capture a baseline, so a custom task that shares one target gets a reset() that leaks agent.state between cases (repro below); and the runner now passes strategy.MAX_ALLOWED_TURNS, so a subclass ClassVar lifts the "hard" 50 cap the README still promises (runner passed 10 000 in my repro). Inline threads carry suggestions. Also: this changes a public ABC signature and adds root exports, with no needs-api-review label.
✅ Verified (head ecdfc89)
pytest tests/strands_evals/experimental/redteam→ 359 passed;ruff check/format --checkclean;mypy src/strands_evals/experimental/redteamclean.- Mutation checks on the new tests — all 4 killed:
_turn_capignoresown;_turn_capdoesn't clamp; runner passesmax_turns=None;as_target_sessionreturns the target unwrapped. _build_sessionbody intarget_session.py:366-395is textually identical to the block removed fromtask.py;as_target_session(make_target())≡ old_build_session(make_target(), baseline=None).- Repro, shared target:
agent.state.set("mode","x")afteras_target_session(agent), thensession.reset()→state.modestill"x", messages cleared. Same withStrandsAgentSession(agent, baseline=agent.take_snapshot(preset="session"))→ restored. - Repro, cap: subclass with
MAX_ALLOWED_TURNS = 10_000driven throughtask._run_attack→ strategy receivedmax_turns=10000; onmainthe runner always passed 50. - mypy on a user subclass still declaring
max_turns: int(MYPYPATH=src): new Liskov error on this branch, none onmain.
Questions (non-blocking)
- Default on an abstract method.
base.py:78givesrun_attacka default that Python won't inherit into overrides, andbase.py:96documents an underscore method (_turn_cap) as the thing implementers must call. Would a template method be simpler — public non-abstractrun_attackthat resolves the cap and calls an abstract_run_attack(..., max_turns: int)? Subclasses then get a resolvedintand can't skip the clamp. It's a bigger rename for existing subclasses, so a maintainer call — but if the subclass contract breaks anyway (mypy Liskov already does), once in the final shape beats twice. - Silent clamp of an explicit value.
run_attack(..., max_turns=100)was honoured and now silently becomes 50.AGENTS.md:491asks for aDeprecationWarningcycle on changed defaults; alogger.warningwhenmax_turns is not None and max_turns > cap(matchingtarget_session.py:46) would be the cheap version. TheNonepath should stay silent. - Why keep
_build_sessionat all?as_target_sessionisreturn _build_session(target)plus a duplicated docstring, andtask.py:16imports the private name across modules for one call. Oneas_target_session(target, *, baseline=None)would remove both (and answers the first inline thread).
Reading order
strategies/base.py (constant, ClassVar, _turn_cap) → task.py:150 → strategies/target_session.py:344-395 → one strategy (pair/__init__.py:148-156) as representative of the six → tests.
Appendix — non-blocking (7)
README.md:76still showsrun_attack(case, target_session, *, max_turns, model)as ifmax_turnswere required;:87says it can be omitted. Suggestmax_turns=None, model=None.SKILL.md:429still says "task.pyenforcesMAX_ALLOWED_TURNS = 50";SKILL.md:421lists the session symbols withoutas_target_session. README/AGENTS.md were updated, SKILL.md wasn't.README.md:229still describestask.pyas "wraps Agent / MultiAgentBase into a TargetSession" — that moved.as_target_sessionhas zero README mentions despite being the custom-task entry point.task.py:15re-export alias:MAX_ALLOWED_TURNSwasn't in any__all__before this PR and everything else intask.pyis private — the alias (andtest_strategies.py:103pinning it) is maintenance for a path nobody public had. Experimental module; dropping it is cheaper.- Only
PromptStrategyis tested withmax_turnsomitted (test_strategies.py:109-121); the other five changed signatures aren't. A parametrize overBUILTIN_STRATEGIEScloses it. bad_likert_judge/__init__.py:116rebinds the parameter (max_turns = self._turn_cap(max_turns)) where the other five assignturn_cap; works, just the odd one out.- User subclasses declaring
max_turns: intnow fail mypy (Liskov) — the package ships nopy.typed, so most users won't see it; worth one line in the PR's "Known limitation".
|
@strandly-the-agent review the changes |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Approve — all three round-1 findings fixed at 20d153d, and the fix is simpler than what I suggested. Dropping the ClassVar and _turn_cap entirely (plain max_turns: int = MAX_ALLOWED_TURNS, runner passes the module constant) removes the cap escape, the silent-clamp behaviour change, and the private-method-as-contract question in one move. The as_target_session shared-target leak is handled by the docstring Note rather than a capture_baseline knob — fine for Part 3, and the README now points custom tasks at a fresh target per case.
Still no needs-api-review label for a public ABC signature change + two new root exports; not blocking from me, just flagging for whoever merges.
✅ Verified (head 20d153d, delta vs ecdfc89 is fixes-only)
- Round-1 🟡 cap escape — fixed: subclass with
MAX_ALLOWED_TURNS = 10_000driven throughtask._run_attacknow receivesmax_turns=50(was 10 000). - Round-1 🟡 shared-target
reset()— documented (target_session.py:352-353); behaviour unchanged by design, README/docstring example build a fresh target per case. - Round-1 ⚪ error message — fixed:
TypeError: target must be …; test tightened tomatch="^target must be". - Appendix items also picked up:
README.md:76/87/229,SKILL.md:421/429updated; parametrized default test over all six strategies + the ABC (test_strategies.py); explicitmax_turns=80is now honoured and tested. pytest tests/strands_evals/experimental/redteam→ 365 passed; ruff check/format clean; mypy clean.- Mutation: changing one strategy's default to 49 fails the new parametrized test, so it discriminates.
- Attacked the fix: removing the clamp restores pre-PR semantics for direct callers (no behaviour change left to deprecate);
bad_likert_judgemax_turns < 2guard still reached. Nothing new found.
Description
Writing a custom red-team task, for example one that builds a fresh target per case or drives
strategies outside
RedTeamExperiment, needed two things that were private or manual:as_target_session(target)(new public helper). It wraps astrands.AgentinStrandsAgentSessionand aMultiAgentBase(Graph/Swarm) inStrandsMultiAgentSession,and returns a ready
TargetSessionunchanged. Anything else raisesTypeError. The wrappinglogic (
_build_session) moves fromtask.pytostrategies/target_session.py, and theparallel task runner now uses the public helper. The helper is exported from
strategiesandfrom
strands_evals.experimental.redteam.run_attack'smax_turnsdefaults toMAX_ALLOWED_TURNS. Custom tasks previously had topass
max_turns=MAX_ALLOWED_TURNSon every call, and nothing enforced the cap.MAX_ALLOWED_TURNS = 50moves fromtask.pytostrategies/base.pyand is exported fromstrategiesand the package root. It has to live inbase.pybecausetask.pyimportsstrategies.task.MAX_ALLOWED_TURNSis still re-exported, so existing imports work.AttackStrategy.MAX_ALLOWED_TURNS(aClassVar) and a_turn_cap(max_turns, own=None)helper. The helper uses the cap when
max_turnsisNone, clamps larger values to it, andthen applies the strategy's own budget when that is smaller.
max_turns: int | None = Noneand call_turn_cap.max_turnsexplicitly, now asstrategy.MAX_ALLOWED_TURNS.Custom strategies written against the old contract declare
max_turnswith no default, andomitting it would raise a
TypeErrorfor them.Behavior change
Calling a built-in strategy's
run_attackdirectly withmax_turns > 50now clamps to 50 insteadof honoring the value. Experiments run through
RedTeamExperimentare unaffected, because therunner already passed 50.
Known limitation
Python does not inherit a parameter's default into an override. A user's custom strategy gets the
default and the clamp only if it declares
max_turns: int | None = Noneand callsself._turn_cap(...). Making this automatic for every subclass, for example by wrappingrun_attackin__init_subclass__, is left out of this PR to keep it small.Related Issues
Documentation PR
N/A. The module README, the
as_target_sessiondocstring example and AGENTS.md are updated inthis PR.
Type of Change
New feature
Testing
New
test_target_session.pytests foras_target_session: wrapping anAgentor aMultiAgentBase, returning aTargetSessionsubclass unchanged instead of re-wrapping it, andraising
TypeErrorfor an object without atracelist.New
test_strategies.pytests: omittingmax_turnsgives 50,max_turns=100is clamped to 50,_turn_capresolution cases, andMAX_ALLOWED_TURNSimportable frombase,strategies,taskand the package root.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.