[Recipe] add Claude Code recipe agent loop - #1695
Conversation
2b1519e to
eb229a6
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| kwargs = { | ||
| "return_token_ids": True, | ||
| "return_logprob": False, | ||
| "return_logprob": True, |
There was a problem hiding this comment.
Claude: Warning — Unconditional return_logprob=True is a behavioral change for all callers.
This changes from False to True for every request going through LocalRolloutBackend, not just Claude Code agent loops. Returning logprobs requires additional GPU memory and computation. If the only consumer that needs logprobs is the new ClaudeCodeAgentLoop (which reads them in chat_trace_records_to_rollout_states), consider making this request-driven instead — e.g., respect a flag on the canonical request or the SampleParams, so existing callers aren't impacted.
| "return_logprob": True, | |
| "return_logprob": canonical_request.return_logprob if hasattr(canonical_request, "return_logprob") else True, |
If all callers on rl_design actually do need logprobs, please add a brief comment explaining why the default was flipped.
| XUTNER_DIR="${XUTNER_DIR:-${REPO_ROOT}}" | ||
| XTUNER_DIR="${XTUNER_DIR:-${XUTNER_DIR}}" |
There was a problem hiding this comment.
Claude: Warning — Typo XUTNER_DIR is confusing and fragile.
Lines 7-8 define XUTNER_DIR (typo) then use it as the fallback for XTUNER_DIR. Later, PYTHONPATH (line 33) is set from XUTNER_DIR (the typo'd version) and both are exported. While the fallback chain makes this functionally equivalent to using XTUNER_DIR directly, anyone reading this script will be confused about whether XUTNER_DIR is an intentional legacy env var or a typo.
If it's not an intentional backwards-compat alias, consider simplifying to:
XTUNER_DIR="${XTUNER_DIR:-${REPO_ROOT}}"and updating the PYTHONPATH and export lines accordingly.
| blocks.append({"type": "thinking", "thinking": reasoning_text}) | ||
| return blocks or [{"type": "text", "text": ""}] | ||
|
|
||
| def _sanitize_tool_call_for_request( | ||
| self, | ||
| tool_call: CanonicalToolCall, | ||
| *, | ||
| tools: list[CanonicalToolDefinition], | ||
| ) -> CanonicalToolCall: | ||
| tool_definition = next((tool for tool in tools if tool.name == tool_call.name), None) | ||
| if tool_definition is None: | ||
| return tool_call | ||
|
|
||
| properties = tool_definition.parameters_json_schema.get("properties") | ||
| if not isinstance(properties, dict): | ||
| return tool_call | ||
|
|
||
| arguments = tool_call.arguments | ||
| normalized_arguments = False | ||
| if not isinstance(arguments, dict): | ||
| normalized_arguments = True | ||
| if tool_call.raw_arguments_text is not None: | ||
| try: | ||
| decoded = json.loads(tool_call.raw_arguments_text) | ||
| except Exception: | ||
| decoded = {"raw": tool_call.raw_arguments_text} | ||
| arguments = decoded if isinstance(decoded, dict) else {"value": decoded} | ||
| elif arguments is None: | ||
| arguments = {} |
There was a problem hiding this comment.
Claude: Nit — Consider logging when arguments are dropped.
The sanitization logic correctly strips unknown keys and records them in metadata["dropped_arguments"], which is great for post-hoc analysis. However, silently dropping tool call arguments in a training pipeline could be hard to debug when things go wrong.
Consider adding a logger.debug(...) or logger.warning(...) when dropped_keys is non-empty, so operators can see this happening in real time:
if dropped_keys:
metadata["dropped_arguments"] = dropped_keys
logger.debug("Dropped unknown arguments %s from tool call %s", dropped_keys, tool_call.name)
| class ClaudeCodeAgentLoop(AgentLoop): | ||
| def __init__( | ||
| self, | ||
| claude_command: list[str], | ||
| cwd: str | None, | ||
| timeout_s: float, |
There was a problem hiding this comment.
Claude: Nit — Missing type annotations on build_local parameters.
Per project standards in CLAUDE.md: "All new code must include type hints for function signatures." The rollout_controller and logger parameters lack type annotations.
| class ClaudeCodeAgentLoop(AgentLoop): | |
| def __init__( | |
| self, | |
| claude_command: list[str], | |
| cwd: str | None, | |
| timeout_s: float, | |
| def build_local( | |
| self, | |
| rollout_controller: RolloutController, | |
| judger: Judger | None = None, | |
| logger: Any = None, | |
| ) -> ClaudeCodeAgentLoop: |
| self._failed_state( | ||
| rollout_state, | ||
| "Judger completed without setting reward.", | ||
| extra_fields=rollout_extra_fields, |
There was a problem hiding this comment.
Claude: Nit — api_key used for both auth and trace store correlation is a security anti-pattern.
The api_key is a random UUID hex used simultaneously as: (1) the ANTHROPIC_API_KEY / Authorization: Bearer token, and (2) the key to look up trace store records via /trace_store/pop. This means the trace store endpoint effectively becomes an unauthenticated data exfiltration path — anyone who can guess or observe the key can pop another session's training data.
In the current setup (localhost gateway, single-machine), this is low risk. But if the gateway is ever exposed to a network, consider separating the auth token from the trace correlation ID.
| </parameter> | ||
| </function> | ||
| </tool_call> |
There was a problem hiding this comment.
Claude: Nit — CALCULATOR_PROMPT embeds model-specific XML syntax.
The prompt hardcodes Qwen-style <tool_call>/<function=...> XML syntax, which is specific to the Qwen tool-calling format. If this recipe is meant to work with other models, this would break. If it's intentionally Qwen-only, a comment noting that would help future readers.
Same applies to CALCULATOR_SYSTEM_PROMPT below (line 56).
|
Claude: ## Summary Adds a IssuesWarning
Nit
VerdictCOMMENT — The recipe and utility code are well-structured. The main concern is the unconditional |
d0db86d to
9e05dd1
Compare
9e05dd1 to
d2f2f87
Compare
|
@claude review, if you think this pr is ready, give it an approve |
| except Exception as exc: | ||
| return [self._failed_state(rollout_state, f"ClaudeCodeAgentLoop failed: {exc}")] |
There was a problem hiding this comment.
Claude: Warning — Broad except Exception swallows programming bugs.
This catch-all turns every error (including KeyError, AttributeError, TypeError from coding bugs) into a FAILED rollout state with only a string message. During development this makes debugging extremely difficult — a typo in a field name silently becomes "ClaudeCodeAgentLoop failed: 'foo'" instead of a traceback.
Consider narrowing to expected failure modes (e.g., httpx.HTTPError, asyncio.TimeoutError, RuntimeError) and letting unexpected exceptions propagate, or at minimum logging the full traceback before converting to a failed state:
except Exception as exc:
logger.exception("ClaudeCodeAgentLoop failed unexpectedly")
return [self._failed_state(rollout_state, f"ClaudeCodeAgentLoop failed: {exc}")]
| last_error = repr(exc) | ||
| time.sleep(1.0) | ||
| raise AssertionError(f"Gateway did not become ready at {base_url}: {last_error}") | ||
|
|
There was a problem hiding this comment.
Claude: Nit — AssertionError is semantically wrong for a runtime timeout.
AssertionError conventionally signals a violated program invariant (and is suppressed by python -O). A gateway failing to become ready within a timeout is a runtime condition, not an assertion. Consider TimeoutError or RuntimeError instead:
| raise TimeoutError(f"Gateway did not become ready at {base_url}: {last_error}") |
| if bound_ports is not None: | ||
| return bound_ports | ||
| else: | ||
| assert end_port is not None |
There was a problem hiding this comment.
Claude: Nit — Contiguous port search uses fixed 100 random attempts, which is fragile for large nums.
With random.randint(20000, 60000 - nums), the search space is ~40k candidates but only 100 are tried. If the machine has scattered port reservations, this can fail even when a valid contiguous range exists. A sequential scan from a random start point would be more reliable:
start = random.randint(20000, 60000 - nums)
for offset in range(60000 - nums - 20000):
candidate = 20000 + (start - 20000 + offset) % (60000 - nums - 20000)
bound_ports = try_bind_ports(list(range(candidate, candidate + nums)))
if bound_ports is not None:
return bound_portsNot critical since 100 attempts is usually sufficient in practice, but worth noting.
|
Claude: ## Review (round 2) The PR was force-pushed since the last review but the substance is unchanged — none of the previous inline comments were addressed. SummaryAdds a Outstanding issues from the previous review (not yet addressed)
New findings (this review)
VerdictCOMMENT — The architecture is solid and the recipe is well-structured, but issue #1 ( |
add Claude Code recipe agent loop
add Claude Code recipe agent loop
add Claude Code recipe agent loop
2 participants
No description provided.