Skip to content

Add per-domain RewardAdapter seam (Refs #199)#360

Open
larstalian wants to merge 1 commit into
mainfrom
feat/199-reward-adapter
Open

Add per-domain RewardAdapter seam (Refs #199)#360
larstalian wants to merge 1 commit into
mainfrom
feat/199-reward-adapter

Conversation

@larstalian

Copy link
Copy Markdown
Collaborator

Summary

Refs #199. Adds a RewardAdapter seam so each pack owns its EpisodeResult -> reward mapping and training loops stay domain-agnostic — replacing the hardcoded success -> 1.0 / else 0.0 shaping. The point of this increment is the seam, not a perfect reward function.

  • SDK (openrange-pack-sdk, _protocols.py): new RewardAdapter ABC — reward(report: EpisodeReportLike) -> float | Sequence[float] — consuming the SDK's EpisodeReportLike, never openrange. Adds a generic SubgoalFractionRewardAdapter that reproduces today's dense mapping exactly (1.0 on success, else fraction of subgoals passed, else 0.0), returned by a concrete Pack.default_reward_adapter() hook so every existing pack keeps working (trading/swe inherit it unchanged). EpisodeReportLike is widened with an episode_result: EpisodeResult property so adapters can read subgoals for partial credit; concrete EpisodeReport already satisfies it. Both new symbols exported from the SDK __init__.
  • Cyber (packs/cyber_webapp/.../reward.py): CyberPentestRewardAdapter gives a monotone exploit-chain ladder from the subgoals the pentest grader already emits — reached_endpoint (0.2) < extracted_anything (0.6) < matched_flag (1.0), 1.0 on success — wired via WebappPack.default_reward_adapter().
  • Trainer (src/openrange/training.py): episode_reward / episode_trajectory take an optional adapter. A float output is the scalar (keeping per-subgoal components); a Sequence[float] becomes an indexed components map whose mean is the scalar. adapter=None preserves current behavior byte-for-byte.
  • Example (examples/reward_adapter_demo.py): builds miss / reached / extracted / solved reports by hand and prints the generic default vs the cyber ladder, then feeds episode_trajectory(report, adapter=...). No LLM, no network.

The protocol widening required adding episode_result to a handful of existing EpisodeReportLike test shims (curriculum / mutation / dashboard tests); no core behavior changed.

Test plan

  • uv run ruff check . — clean
  • uv run ruff format --check . — clean
  • bash scripts/check_boundary.sh — clean (cyber reward.py imports only the SDK)
  • uv run mypy src tests examples main.py packages/openrange-trl/src packages/openrange-rllm/src — Success, no issues (89 files)
  • uv run mypy packages/openrange-pack-sdk/src packages/openrange-pack-sdk/tests/test_reward.py packs/cyber_webapp/cyber_webapp/reward.py — clean
  • Impacted tests pass: uv run pytest packages/openrange-pack-sdk/tests/test_reward.py tests/test_cyber_reward.py tests/test_training.py tests/test_curriculum.py tests/test_cyber_auto_curriculum.py tests/test_v1_llm_generation.py tests/test_dashboard_runs.py tests/test_import_boundaries.py packages/openrange-pack-sdk/tests/test_sdk.py (220 passed)
  • 100% branch coverage on new/changed lines (training.py + cyber reward.py report complete; new _protocols.py lines all covered). Tests use real concrete subclasses + real EpisodeReport/EpisodeResult — no mocks/patches/fakes.
  • uv run python -m examples.reward_adapter_demo runs (no LLM/network)

🤖 Generated with Claude Code

Packs emit a structured EpisodeResult and never a scalar; today the
result->reward mapping is hardcoded harness-side. Introduce a
RewardAdapter ABC on the pack SDK so each pack owns its natural reward
shape and training loops stay domain-agnostic.

- SDK: RewardAdapter ABC + generic SubgoalFractionRewardAdapter (1.0 on
  success, else subgoal fraction, else 0.0), exposed via a concrete
  Pack.default_reward_adapter() hook so every existing pack keeps working.
  Widen EpisodeReportLike with episode_result so adapters can read
  subgoals for partial credit; concrete EpisodeReport already satisfies it.
- Cyber: CyberPentestRewardAdapter gives a monotone exploit-chain ladder
  (reached_endpoint 0.2 < extracted_anything 0.6 < matched_flag 1.0),
  wired through WebappPack.default_reward_adapter().
- Trainer: episode_reward/episode_trajectory take an optional adapter that
  sources the scalar; adapter=None preserves current behavior exactly.
- Example: examples/reward_adapter_demo.py contrasts the generic default
  against the cyber ladder and feeds episode_trajectory (no LLM/network).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@larstalian

Copy link
Copy Markdown
Collaborator Author

Code review — RewardAdapter seam (Refs #199)

Reviewed against the repo's own bar: .rules, CONTRIBUTING.md, the PR template's self-review checklist, DESIGN.md/CONTRACTS.md/docs/start_here.md, and the acceptance criteria on #199. I checked out the branch and re-ran the checks locally: ruff check clean, ruff format --check clean, mypy clean, scripts/check_boundary.sh clean, the impacted tests pass (190 in my subset), and python -m examples.reward_adapter_demo runs and prints the expected generic-vs-cyber table. Nice, focused increment that matches #199's "the point is the seam" framing.

What's good

  • SDK boundary is respected. The adapter consumes EpisodeReportLike, never openrange; packs/cyber_webapp/.../reward.py imports only the SDK; check_boundary.sh stays clean (verified). This is exactly what Add per-domain RewardAdapter #199's acceptance asked for.
  • Backward compatible by construction. adapter=None is the default and preserves episode_reward behavior byte-for-byte; test_none_adapter_preserves_default_behavior locks it in.
  • The cyber ladder is wired to the real grader. webapp.pentest.check_success emits reached_endpoint / extracted_anything / matched_flag (confirmed in families/pentest.py), and the ladder keys off those exact names — so the rungs are live, not aspirational. Monotone, and tested for it.
  • Tests follow .rules. Real EpisodeReport/EpisodeResult + real RewardAdapter subclasses, no mocks/patches/fakes; the new _protocols.py reward lines are covered. Docstrings on the public symbols carry real WHY, per the docstring rule.

Must address before merge

  • Doc drift contradicts a documented architecture boundary. DESIGN.md ("No reward function anywhere" in the env; "Reward shaping is the harness's job", ~L483–486, L633–634), CONTRACTS.md (L53, L777 "never a scalar reward … reward shaping is harness-side"), and docs/start_here.md (L197 "It does not define the training reward") all state that reward shaping lives on the harness side. This PR moves a default reward shape onto Pack.default_reward_adapter() in the SDK — i.e. env-side. That's sanctioned by roadmap issue Add per-domain RewardAdapter #199, and README.md L24 ("swap the pack … without rewriting your … reward policy") already leans that way — but the design docs now actively contradict the code. CONTRIBUTING.md and the PR template's self-review both require doc updates when architecture/public behavior changes. Please reconcile: update DESIGN.md/CONTRACTS.md/start_here.md to describe the pack-owned default reward seam (and that a harness may still override it), so the docs and the code agree. This is the one item I'd block merge on.

Should address

  • Duplicated subgoal-fraction logic (.rules: "No redundant abstractions or duplicate code"). The "1.0 / fraction-of-subgoals / 0.0" mapping now exists twice: the adapter=None path in episode_reward (training.py) and SubgoalFractionRewardAdapter.reward (_protocols.py). They have to be kept in lockstep by hand. Either have the None path delegate to SubgoalFractionRewardAdapter(), or add a one-line WHY that the duplication is deliberate to preserve the byte-for-byte compat guarantee without coupling training.py to the SDK adapter.

Notes / follow-up (non-blocking)

  • EpisodeReportLike widening is a breaking protocol change. It's a @runtime_checkable public Protocol; any external pack implementing it must now provide episode_result. Handled for all in-repo shims here, but worth a line in the PR/release notes for downstream pack authors. (Minor: passed and episode_result.success are now both on the protocol and are redundant.)
  • No production caller uses the seam yet. episode_reward/episode_trajectory accept the adapter, but the actual eval pipeline the issue named (examples/codex_eval.py) still doesn't pass one — the adapter is exercised only by the demo and tests. Fine for a seam-only increment; flagging it as the obvious next step so Add per-domain RewardAdapter #199's "replace the ad-hoc mapping" intent actually lands.
  • _adapter_reward branches on isinstance(value, Sequence). Correct for float vs list/tuple, but a numpy array (a common multi-objective return) is not a collections.abc.Sequence and would fall into the scalar branch and raise on float(...). The contract is Sequence[float], so this is acceptable — just be explicit if numpy vectors are expected.

Overall: solid, in-scope, well-tested implementation of #199. Reconcile the design docs (must) and de-dup the fraction logic (should), and this is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant