feat(rollout): surface approver interventions to policy via observati… - #217
feat(rollout): surface approver interventions to policy via observati…#217Adityakk9031 wants to merge 2 commits into
Conversation
jeqcho
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the mechanism is the right shape (rollout mirrors approval events into the store, snapshots them into extra before next_action, agent policy renders a line), the per-trial store created at src/inspect_robots/rollout.py:192 means nothing leaks across trials/epochs, and both new tests are well targeted (nice touch asserting the first inference sees [] in tests/test_rollout_hardening.py::test_rollout_surfaces_approvals_in_observation_extra). Both touched test files pass locally (152 passed). A few things need addressing before merge:
1. The rendered approvals line grows unboundedly — plugins/inspect-robots-agent/src/inspect_robots_agent/policy.py:922-929
_approvals_line joins one detail string per intervention with no aggregation. In exactly the scenario #187 describes — a policy repeatedly issuing out-of-bounds actions against a ClampApprover — a few hundred clamped steps produce approver: 400 step(s) modified (clamped, clamped, clamped, …) with hundreds of repeated tokens, injected into every subsequent observation. Please aggregate by flag, e.g. collections.Counter over the details, rendering something like approver: 400 step(s) modified (clamped ×398, delta_clamped ×2). — and extend test_observation_content_renders_approver_interventions to cover duplicate flags.
2. Cumulative-since-reset semantics diverge from the issue and dilute the signal — src/inspect_robots/rollout.py:220
#187 proposes surfacing interventions "from the steps since the last act()". The PR instead passes the full trial history on every step. Beyond size, this weakens the actionable signal: what the model needs to know is "was my latest motion clamped?", and under cumulative semantics that is drowned out once early interventions accumulate (the line eventually reads the same whether or not the most recent chunk was touched). The rollout already tracks inference boundaries via prev_inferences / _INFER_KEY (lines 219, 232), so windowing is cheap — e.g. record the approvals-list length at each inference and pass only the tail since the previous one. If you have a deliberate reason to prefer cumulative (t fields do let a sophisticated policy diff), please say so on the issue and document the chosen semantics; but for the agent-policy rendering, "since your last decision" is the more useful contract.
3. Missing CHANGELOG entry
CONTRIBUTING.md (step 4) asks for a CHANGELOG.md entry under "Unreleased". This is a policy-visible contract addition, so it belongs under "Added", referencing #187/#217 in the style of the existing entries.
4. Document the newly reserved extra key — src/inspect_robots/types.py:32-35
The Observation docstring currently says the rollout "injects the current step into the policy-facing observation's extra["env_step"] and reserves that key; embodiments should not set it." Since rollout.py:223 now also unconditionally sets (and would silently overwrite) extra["approvals"], please extend that docstring to reserve "approvals" the same way, including its shape (list of {"t": int, "detail": str | None}) and the fact that it is always present, possibly empty.
Minor (optional):
approvals = list(store.get(_APPROVALS_KEY, []))atrollout.py:220shallow-copies the list, but the entry dicts are shared by reference across every observation for the rest of the trial (and with the store). A policy that mutates an entry would corrupt what later inferences see. Copying the dicts (or storing immutable tuples) would harden this cheaply._approvals_line's defensiveisinstancechecks are welcome givenextrais an open channel; no change needed there.
Nothing here is structural — the core wiring is sound and matches the issue's intent. Happy to re-review once the rendering aggregation, windowing decision, changelog, and docstring are in. Thanks again for the contribution! 🙏
Description
Fixes #187 by surfacing safety approver interventions (such as
clampedanddelta_clampedrewrites) directly to policies inobservation.extra["approvals"].Previously, when an approver rewritten an action, the modification was recorded into the trial's event stream in
EvalLog, but never surfaced topolicy.act(). This meant LLM/VLA policies had to infer clamps from proprioceptive drift alone, often misattributing undershoot to dynamics/stiction and re-issuing the same out-of-bounds action.Proposed Changes
Core (
inspect_robots):rollout.py, track approval events in trialstoreunder_APPROVALS_KEY.{"t": t, "detail": detail}) intoobs.extra["approvals"]for every step when callingcontroller.next_action(...).test_rollout_surfaces_approvals_in_observation_extraintests/test_rollout_hardening.py.Agent Policy (
inspect_robots-agent):_observation_content(), renderobservation.extra.get("approvals")as a prompt text line (e.g.approver: 3 step(s) modified (clamped, delta_clamped).).test_observation_content_renders_approver_interventionsinplugins/inspect-robots-agent/tests/test_policy_e2e.py.Verification
pytest tests/(861 passed).