Skip to content

fix: Tape.__getitem__ ignores slice step; last_actions() breaks on n_added_steps=0 - #247

Open
mittalpk wants to merge 1 commit into
ServiceNow:mainfrom
mittalpk:fix/tape-last-actions-zero-added-steps
Open

mittalpk wants to merge 1 commit into
ServiceNow:mainfrom
mittalpk:fix/tape-last-actions-zero-added-steps

Conversation

@mittalpk

Copy link
Copy Markdown

Two related bugs in tapeagents/core.py's Tape class, found via code review (no existing issue).

1. Tape.__getitem__ silently ignores a slice's step

return self.model_copy(update=dict(steps=self.steps[key.start : key.stop], metadata=TapeMetadata()))

Reconstructs the slice from start/stop only, dropping key.step entirely. tape[::-1], tape[::2], tape[1:5:2] etc. all silently return the wrong steps instead of applying the step or raising. Fixed by passing the slice object through directly (self.steps[key]), which Python already handles correctly.

2. last_actions() re-executes the entire tape history when n_added_steps == 0

def last_actions(tape: Tape) -> list[Action]:
    return [step for step in tape.steps[-tape.metadata.n_added_steps :] if isinstance(step, Action)]

n_added_steps defaults to 0 and __add__ legitimately produces 0 when zero new steps were added (see the existing test_add_empty_tape). Python's -0 == 0, so tape.steps[-0:] returns the entire list instead of an empty slice.

This matters because Environment.react()/areact() (environment.py, remote_environment.py) call last_actions(tape) to decide which of the agent's latest actions to execute against tools. With n_added_steps == 0, they'd silently re-execute every action ever taken in the tape's whole history and append duplicate observations, instead of doing nothing.

How did you test it

Added 4 tests to tests/test_core.py: test_getitem_slice_with_step_reversed, test_getitem_slice_with_step_stride, test_last_actions_zero_added_steps_returns_empty, test_last_actions_positive_n_added_steps_unchanged. Confirmed all 4 fail against the pre-fix code (git stash) and pass after.

uv run pytest tests/ --ignore-glob="tests/*/*" (the CI's make test-core target): 62 passed. uv run ruff check clean on both changed files.

…added_steps=0

- __getitem__ reconstructed a slice from key.start/key.stop only,
  silently discarding key.step. tape[::2], tape[::-1], tape[1:5:2]
  etc. all returned wrong data instead of applying the step. Fixed
  by passing the slice object through directly.

- last_actions() computes tape.steps[-tape.metadata.n_added_steps:].
  n_added_steps == 0 is the field's own default and a value __add__
  legitimately produces when zero new steps were added, but Python's
  -0 == 0 makes that slice return the WHOLE list instead of empty.
  Environment.react()/areact() use this to decide which actions to
  execute against tools, so a zero-new-steps iteration would silently
  re-execute every action in the tape's entire history. Fixed with
  an explicit guard.

This branch has not been deployed

No deployments
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