Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion verifiers/v1/trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,9 @@ class Trace(StrictBaseModel, Generic[TaskT, StateT]):
_head_index: dict = PrivateAttr(default_factory=dict)
"""`(parent, msg_hash) -> node_id` for the graph builder (`graph.prepare_turn` / `commit`);
rebuilt lazily from `nodes` after deserialization."""
_branch_cache: list[Branch] = PrivateAttr(default_factory=list)
_branch_cache_node_count: int = PrivateAttr(default=-1)
"""The derived branch view, rebuilt whenever the graph grows."""

@property
def reward(self) -> float:
Expand Down Expand Up @@ -295,6 +298,10 @@ def branches(self) -> list[Branch]:
"""The conversation segmented into linear branches — a view over the graph: each
leaf's root→leaf path is a branch (one when linear, several under compaction or
subagents). Branching falls out of walking each leaf's parents back to its root."""
node_count = len(self.nodes)
if self._branch_cache_node_count == node_count:
return self._branch_cache.copy()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return defensive Branch copies from the cache

Fresh evidence after the prior review: this returns only a shallow copy of the cached list, so callers still receive the exact cached Branch instances. If any post-processing mutates a branch object or its public nodes list, such as trace.branches[0].nodes.pop(), later reads while len(self.nodes) is unchanged reuse the corrupted branch and can make prompt_len, completion_len, or tool_messages wrong; before this change each access rebuilt new Branch objects. Cache immutable data or return defensive Branch copies.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is fine. dont wanna copy for that. branches should be read-only. if users mutate, thats on them

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait acc why do we copy here? this is expensive, no?


branches: list[Branch] = []
for i, leaf in enumerate(graph.leaves(self)):
path: list[int] = []
Expand All @@ -304,11 +311,15 @@ def branches(self) -> list[Branch]:
nid = self.nodes[nid].parent
path.reverse()
branches.append(Branch(index=i, nodes=[self.nodes[n] for n in path]))
return branches
self._branch_cache = branches
self._branch_cache_node_count = node_count
return branches.copy()

@property
def num_branches(self) -> int:
"""How many branches (1 = linear; >1 = compaction/subagents)."""
if self._branch_cache_node_count == len(self.nodes):
return len(self._branch_cache)
return len(graph.leaves(self))
Comment on lines +321 to 323

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt we compute this as len(self.branches), then we have the caching only in one place


@property
Expand Down
Loading