fix(agent): permit re-revealing on_demand cameras when image_horizon …

[Adityakk9031]

Description

Fixes #192 where in images="on_demand" mode, taking multiple single-camera pictures under an image_horizon evicted earlier revealed frames from the active context window, but left their camera names registered in self._revealed.

When the model subsequently attempted to re-reveal an evicted frame, LLMAgentPolicy refused the request with "already shown for this observation ... the view cannot change until the robot moves", leaving the model with an untrue refusal text and no way to recover the frame until taking a motion step.

Proposed Changes

• Agent Policy (plugins/inspect-robots-agent/src/inspect_robots_agent/policy.py):

  • In LLMAgentPolicy.act(), inspect the active outgoing view (post _evicted_view()) before processing duplicate camera capture refusals.
  • Update self._revealed by keeping only camera names whose frame messages are still present in the outgoing payload.
  • If a frame message was stubbed/evicted due to image_horizon, re-requesting take_pic for that camera is now permitted.

• Tests (plugins/inspect-robots-agent/tests/test_policy_eviction.py):

  • Added test_on_demand_allows_re_reveal_after_eviction() to verify that sequential single-camera reveals exceeding image_horizon permit re-revealing aged-out frames.

Verification

• Ran pytest plugins/inspect-robots-agent/tests/test_policy_eviction.py (17 passed).

[Adityakk9031]

@aris-zhu and @jeqcho have a look

[jeqcho]

Thanks for taking this on — the direction is right (deriving what's still visible from the same outgoing view the model saw, and choosing the "permit re-reveal" option from #192), and I confirmed the fix does work in the normal eval() path: three sequential single-camera reveals under image_horizon=2 followed by a re-request of the aged-out camera now re-captures it, while a still-visible duplicate is still refused. However, there are a few issues that need addressing before this can merge.

1. Quote-stripping order bug silently disables the duplicate-capture refusal when there is no step label (policy.py:762-766)

Frame labels are f"camera {name!r}{suffix}:" (policy.py:978), where suffix is empty when observation.extra["env_step"] is not an int (policy.py:937-940). In that case the label is camera 'top':, the whitespace-split term is 'top':, and

term.strip("'\"").rstrip(":")  # "'top':" -> "top':" -> "top'"

yields top' (the trailing quote survives because strip stops at the :). active_camera_names then never matches any real name, self._revealed.intersection_update(...) empties the set, and every duplicate take_pic is re-captured instead of refused — even when nothing was evicted. Reproduced: with a CubePickEmbodiment observation (no env_step, one image message, horizon 2, nothing evicted), two consecutive take_pic ["top"] calls both return captured 1 frame(s): 'top'; on main the second is correctly refused. CI stays green only because every fixture in test_policy_e2e.py passes env_step=0, so the suffix hides the bug. At minimum the order should be .rstrip(":").strip("'\"") — but see the next point.

2. Parsing rendered label text is fragile; consider tracking eviction structurally (policy.py:748-767)

Even with the strip order fixed, camera names containing whitespace break the token-based parse (camera 'left cam' (step 0): splits into 'left / cam', so left cam is never recognized as active and its refusal is permanently disabled). Nothing in the codebase constrains camera names to be identifier-like. A format-independent approach avoids this class of bug entirely: _evicted_view() already computes exactly which message indices get stubbed (image_message_indices[:-horizon], policy.py:137-143). You could have it also report those indices (or recompute them in act()), and record which message index each revealed camera's frame landed in — the policy appends those messages itself at policy.py:651 and policy.py:845 — then drop revealed names whose frame message was stubbed. That also matches the module's style of factoring logic into helpers (_evicted_view, _image_parts) rather than a ~20-line block at five levels of nesting inside act().

3. The new test does not exercise the fix (tests/test_policy_eviction.py:312-451)

• CubePickEmbodiment has only a top camera (src/inspect_robots/mock/cubepick.py:58), so calls 2 and 3 (front, left) fail with unknown or unavailable camera ... and no frames are ever appended — nothing is ever evicted, despite the test's name.
• The only assertion is chunk is not None, which holds regardless: the test passes unmodified against origin/main's policy.py. A refusal on call 4 doesn't error the turn (first rejection isn't counted as a failure), so the move_by response still yields a chunk.

Suggest rebuilding the test on a multi-camera observation with extra={"env_step": 0} (see _vision_observation in test_policy_e2e.py:368), and asserting the transcript's tool results directly: the 4th result is captured 1 frame(s): '<evicted-camera>', plus a companion case asserting a still-visible duplicate is still refused with already shown for this observation: .... Without the second assertion, an over-permissive implementation (like the current strip bug) sails through.

4. Missing CHANGELOG entry

CONTRIBUTING.md (step 4) asks for a CHANGELOG.md entry under "Unreleased" — a short "Fixed" note referencing #192 would do.

5. Verification scope

The PR notes running only test_policy_eviction.py; please also run plugins/inspect-robots-agent/tests/test_policy_e2e.py (it holds the existing already shown contract tests around lines 1965-2028) before pushing the revision.

Happy to re-review once these are addressed — the core idea is sound, it's the name-recovery mechanism and the test's teeth that need work. Thanks again for the contribution! 🙏