[WIP]Feat: normalize anthropic messages - #49
Conversation
… construct provider trace;
Adopt the PR #45 native Anthropic relay so Claude Code reaches Gateway without LiteLLM, and align Docker/RJob runtime configuration with v4. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe PR replaces the Claude adapter path with native Anthropic gateway support. It adds Anthropic request normalization, streaming and telemetry handling, provider request persistence, SQLite migration, cloud serialization updates, and Claude runtime configuration changes. ChangesNative Anthropic Gateway Flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cve-2025-12266__bc9acb2a-2ec8-4e18-8cd2-9b3445799e4b.json (1)
1-1047: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDelete this trajectory dump from the repository.
This 1047-line file is a captured RL session artifact at the repository root. No configuration or code in this PR references it. It appears to be committed by accident during S3 testing.
It also discloses internal infrastructure:
upstream_base_urlandrequest_urlexpose an internal provider endpoint35.220.164.252:3888.job_idembeds an operator identifier.env_stateembeds absolute shared-storage mount paths.Delete the file and add a root-level ignore rule for these session dumps. The
rl/examples/patcheval/.gitignorerules added in this PR do not cover the repository root.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cve-2025-12266__bc9acb2a-2ec8-4e18-8cd2-9b3445799e4b.json` around lines 1 - 1047, Delete the accidentally committed root-level trajectory dump file, and add a repository-root .gitignore rule matching these RL session dump artifacts. Do not rely on the existing rl/examples/patcheval/.gitignore rules, which do not cover files at the repository root.
🧹 Nitpick comments (8)
gateway/anthropic_messages.py (1)
194-197: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUnknown block types abort the whole conversion.
Anthropic adds content block types over time, for example
document,server_tool_use, andweb_search_tool_result. Any one of them raises here, andgateway/storage.py:612-625then discards the normalized result for the entire step and falls back to the raw request messages.A stricter-than-necessary allowlist turns a new provider feature into silent normalization loss across every step of a session. Consider preserving unknown blocks as passthrough content instead of raising, and reserving the error for structurally invalid input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/anthropic_messages.py` around lines 194 - 197, Update the unsupported-content branch in the Anthropic message conversion logic to preserve unknown block types as passthrough content rather than raising AnthropicMessageConversionError. Continue raising only for structurally invalid blocks, while retaining the existing normalization behavior for recognized block types.core/data_manager/strategy/cloud_strategy_impl.py (1)
678-685: 🚀 Performance & Scalability | 🔵 TrivialConsider the size growth of
meta_json.
meta_jsonnow embeds the complete provider request JSON. For Anthropic multi-turn sessions this payload can reach hundreds of kilobytes per step, and it duplicates the conversation already stored inmessages. Two follow-on effects exist:
_normalize_session_step_updates_for_cloudroutesrequestthroughmeta_fields, so_load_existing_meta_jsonreads and rewrites the entire blob on every metadata-only update.list_session_stepsandmark_latest_session_completedselectmeta_json, so every read pulls the full request.A dedicated column for the request would keep metadata small. If the landing schema cannot change, confirm that the column type and the ingest path tolerate large values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data_manager/strategy/cloud_strategy_impl.py` around lines 678 - 685, The meta_json construction in the cloud strategy embeds the full request and causes oversized metadata reads and rewrites. Store the provider request in a dedicated request column and keep meta_json limited to lightweight metadata, then update _normalize_session_step_updates_for_cloud, _load_existing_meta_json, list_session_steps, and mark_latest_session_completed to use that column without duplicating the request; if schema changes are unavailable, verify the existing column type and ingest path safely support the required payload size.rl/examples/patcheval/.gitignore (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope the ignore patterns to the generated output.
*.jsonhides every JSON file in this directory tree, including configuration files a contributor may add later. Git reports no warning when it silently skips them. Restrict the patterns to the generated dataset and result paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rl/examples/patcheval/.gitignore` around lines 4 - 5, Scope the .gitignore patterns to the generated dataset and result paths instead of ignoring all .json and .jsonl files recursively. Update the existing patterns so contributor configuration files remain visible to Git.rl/examples/patcheval/README.md (1)
159-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new environment variables.
run_eval.shaddsPATCH_EVAL_CLAUDE_MAX_THINKING_TOKENSandPATCH_EVAL_STORAGE_TYPE(sqliteorcloud). This section describes the native Anthropic route but does not mention either variable. Readers who follow the README cannot discover the thinking-token limit or the cloud storage backend.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rl/examples/patcheval/README.md` around lines 159 - 161, Update the README section describing the native Anthropic route to document the PATCH_EVAL_CLAUDE_MAX_THINKING_TOKENS and PATCH_EVAL_STORAGE_TYPE environment variables, including that storage type accepts sqlite or cloud and explaining the thinking-token limit’s purpose.env/patcheval/generate_full_config.py (2)
347-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCall
write_configswith keyword arguments.
write_configsnow takes 18 positional parameters. Three new string and int parameters were appended at the end. A future reordering silently swaps values of the same type without an error. Pass the arguments by keyword.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@env/patcheval/generate_full_config.py` around lines 347 - 349, Update the write_configs call to pass all arguments by their parameter names instead of position, including claude_gateway_base_url, claude_model, and claude_max_thinking_tokens, so future parameter reordering cannot silently swap same-typed values.
172-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the Claude arguments in
main()withSystemExit.
write_configsraisesValueErrorfor a missing gateway URL or model.main()does not catch it, so the CLI prints a traceback. Every other argument check inmain()usesSystemExitand runs before dataset loading. Move these two checks next to the check at lines 296-297 so the failure is early and the message is clean.♻️ Proposed validation move
if args.claude_max_thinking_tokens < 0: raise SystemExit("--claude-max-thinking-tokens must be non-negative") + if args.baseline == "claudecode": + if not str(args.claude_gateway_base_url).strip(): + raise SystemExit("--claude-gateway-base-url is required for the Claude Code baseline") + if not str(args.claude_model).strip(): + raise SystemExit("--claude-model is required for the Claude Code baseline")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@env/patcheval/generate_full_config.py` around lines 172 - 175, Move the missing `claude_gateway_base_url` and `claude_model` validations from `write_configs` into `main()`, alongside the existing argument checks before dataset loading, and raise `SystemExit` with the same messages. Remove the corresponding `ValueError` checks from `write_configs` so CLI validation produces clean failures without tracebacks.env/exploitgym/runner.py (1)
485-490: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDetect an early socat exit before the readiness timeout expires.
_wait_httpruns first. Ifsocatexits immediately, the call blocks for the fullready_timeout_sand then reports a timeout instead of the process exit. The nesteddockerdstartup at lines 339-346 already polls the process inside the wait loop. Apply the same pattern here.♻️ Proposed fail-fast readiness check
- _wait_http( - f"http://{bind}:{port}{ready_path}", - timeout=ready_timeout_s, - ) - if relay.poll() is not None: - raise RuntimeError(f"relay exited during startup: {bind}:{port}") + deadline = time.monotonic() + ready_timeout_s + while True: + if relay.poll() is not None: + raise RuntimeError(f"relay exited during startup: {bind}:{port}") + remaining = deadline - time.monotonic() + if remaining <= 0: + raise RuntimeError(f"relay did not become ready: {bind}:{port}") + try: + _wait_http(f"http://{bind}:{port}{ready_path}", timeout=min(2.0, remaining)) + break + except Exception: + continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@env/exploitgym/runner.py` around lines 485 - 490, Update the readiness flow around _wait_http and relay so it polls the socat process during the HTTP wait, matching the nested dockerd startup pattern. Detect and raise the relay-exit RuntimeError as soon as relay.poll() indicates termination, rather than waiting for the full ready_timeout_s before reporting failure.rl/examples/patcheval/run_eval.sh (1)
118-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport an unknown model route with a clear message.
If
PATCH_EVAL_MODELis not a key inllm_routes, this line raises a bareKeyErrorand prints a Python traceback. Claude Code baseline users must set this variable explicitly, so a wrong value is likely. Emit an explicit error that lists the available routes.♻️ Proposed error message
-route = config["llm_routes"][os.environ["PATCH_EVAL_MODEL"]] +model = os.environ["PATCH_EVAL_MODEL"] +if model not in config["llm_routes"]: + raise SystemExit( + f"PATCH_EVAL_MODEL '{model}' is not a configured route; " + f"available: {', '.join(sorted(config['llm_routes']))}" + ) +route = config["llm_routes"][model]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rl/examples/patcheval/run_eval.sh` at line 118, Update the route lookup using PATCH_EVAL_MODEL so an unknown value is handled explicitly instead of exposing a bare KeyError traceback. Validate the model key against config["llm_routes"], emit a clear error that includes the invalid value and available route names, and preserve the existing route selection for valid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/data_manager/strategy/cloud_strategy_impl.py`:
- Around line 1064-1074: Update _response_to_landing_value so a string
containing valid JSON is parsed and reassigned to the resulting JSON value
before the final json.dumps call. Preserve the existing assistant-message
wrapping for invalid JSON strings and the handling of ChatMessage and other
value types.
- Around line 656-668: Update the create-path serialization around full_messages
and response to reuse _messages_to_landing_value and _response_to_landing_value,
preserving compact JSON and default=str behavior. Confirm the response helper’s
JSON-string passthrough does not bypass the intended
{"role":"assistant","content":response} wrapper; if necessary, construct that
assistant payload before invoking the helper.
In `@core/data_manager/strategy/sqlite_strategy_impl.py`:
- Around line 564-565: Update the field-value handling in update_session_step so
None remains None for nullable messages or request columns and is passed to
SQLite as SQL NULL; only JSON-serialize non-None values that are not already
strings.
- Around line 106-125: Update _ensure_runtime_schema’s add_missing_columns
helper to set a busy timeout on its SQLite connection before inspecting the
schema, and make the request-column migration safe when concurrent initializers
race by handling an already-existing column without failing. Preserve the
existing connection cleanup and commit behavior so the idempotent migration
tolerates database locks and duplicate-column races.
In `@env/exploitgym/.env`:
- Around line 1-3: Remove env/exploitgym/.env from version control, revoke and
rotate the exposed EXPLOITGYM_UPSTREAM_API_KEY, and purge the secret from git
history. Add env/exploitgym/.env to .gitignore and create
env/exploitgym/.env.example containing placeholders for the upstream URL, API
key, and runtime-supplied POD_IP.
In `@gateway/anthropic_messages.py`:
- Around line 103-110: Update the signature handling in the Anthropic message
conversion flow to support multiple signatures from interleaved thinking blocks
instead of raising AnthropicMessageConversionError. Preserve the association
between each signature and its reasoning segment, using a list or equivalent
normalized representation compatible with downstream consumers such as storage
conversion, while retaining the existing single-signature behavior.
- Line 102: Update the text-block assembly in the message content handling to
join text_parts with the same "\n\n" separator used by _normalize_system and
reasoning_parts, preserving clear boundaries between consecutive Anthropic text
blocks.
In `@gateway/app.py`:
- Around line 1144-1154: Bound native stream response capture in the streaming
flow that builds stream_text_parts: only accumulate response content when
telemetry sampling requires it, enforce the configured byte limit, and stop
retaining additional data once the limit is reached. Track whether truncation
occurred and pass that state through stream_truncated in both telemetry response
construction sites, including the messages path, without retaining the complete
SSE response when logging is disabled or capped.
In `@gateway/telemetry.py`:
- Around line 590-595: Update the telemetry request metadata around request_url
to accept the filtered non-beta query string forwarded by gateway/app.py, and
append it to the effective upstream URL when present. Ensure the persisted
request_url matches the actual Anthropic Messages request while retaining the
existing base URL and endpoint behavior when no query parameters exist.
- Around line 22-28: Update telemetry redaction around SENSITIVE_KEY_PARTS and
safe_request_body to include signature and encrypted_content, matching
gateway/request_logger.py. When redact_sensitive_fields=True, redact request
payloads before assigning record.request, sampled response bodies before
record.messages/record.response, and raw SSE response_text before serialization.
Ensure all telemetry paths use the shared sensitive-key policy.
---
Outside diff comments:
In `@cve-2025-12266__bc9acb2a-2ec8-4e18-8cd2-9b3445799e4b.json`:
- Around line 1-1047: Delete the accidentally committed root-level trajectory
dump file, and add a repository-root .gitignore rule matching these RL session
dump artifacts. Do not rely on the existing rl/examples/patcheval/.gitignore
rules, which do not cover files at the repository root.
---
Nitpick comments:
In `@core/data_manager/strategy/cloud_strategy_impl.py`:
- Around line 678-685: The meta_json construction in the cloud strategy embeds
the full request and causes oversized metadata reads and rewrites. Store the
provider request in a dedicated request column and keep meta_json limited to
lightweight metadata, then update _normalize_session_step_updates_for_cloud,
_load_existing_meta_json, list_session_steps, and mark_latest_session_completed
to use that column without duplicating the request; if schema changes are
unavailable, verify the existing column type and ingest path safely support the
required payload size.
In `@env/exploitgym/runner.py`:
- Around line 485-490: Update the readiness flow around _wait_http and relay so
it polls the socat process during the HTTP wait, matching the nested dockerd
startup pattern. Detect and raise the relay-exit RuntimeError as soon as
relay.poll() indicates termination, rather than waiting for the full
ready_timeout_s before reporting failure.
In `@env/patcheval/generate_full_config.py`:
- Around line 347-349: Update the write_configs call to pass all arguments by
their parameter names instead of position, including claude_gateway_base_url,
claude_model, and claude_max_thinking_tokens, so future parameter reordering
cannot silently swap same-typed values.
- Around line 172-175: Move the missing `claude_gateway_base_url` and
`claude_model` validations from `write_configs` into `main()`, alongside the
existing argument checks before dataset loading, and raise `SystemExit` with the
same messages. Remove the corresponding `ValueError` checks from `write_configs`
so CLI validation produces clean failures without tracebacks.
In `@gateway/anthropic_messages.py`:
- Around line 194-197: Update the unsupported-content branch in the Anthropic
message conversion logic to preserve unknown block types as passthrough content
rather than raising AnthropicMessageConversionError. Continue raising only for
structurally invalid blocks, while retaining the existing normalization behavior
for recognized block types.
In `@rl/examples/patcheval/.gitignore`:
- Around line 4-5: Scope the .gitignore patterns to the generated dataset and
result paths instead of ignoring all .json and .jsonl files recursively. Update
the existing patterns so contributor configuration files remain visible to Git.
In `@rl/examples/patcheval/README.md`:
- Around line 159-161: Update the README section describing the native Anthropic
route to document the PATCH_EVAL_CLAUDE_MAX_THINKING_TOKENS and
PATCH_EVAL_STORAGE_TYPE environment variables, including that storage type
accepts sqlite or cloud and explaining the thinking-token limit’s purpose.
In `@rl/examples/patcheval/run_eval.sh`:
- Line 118: Update the route lookup using PATCH_EVAL_MODEL so an unknown value
is handled explicitly instead of exposing a bare KeyError traceback. Validate
the model key against config["llm_routes"], emit a clear error that includes the
invalid value and available route names, and preserve the existing route
selection for valid values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b9c8dbe-1b56-4418-a1bf-0b3db7e79f59
📒 Files selected for processing (30)
core/data_manager/manager.pycore/data_manager/models.pycore/data_manager/strategy/base_strategy.pycore/data_manager/strategy/cloud_strategy_impl.pycore/data_manager/strategy/sqlite_strategy_impl.pycve-2025-12266__bc9acb2a-2ec8-4e18-8cd2-9b3445799e4b.jsonenv/exploitgym/.envenv/exploitgym/exploitgym_config.rjob.yamlenv/exploitgym/exploitgym_config.yamlenv/exploitgym/exploitgym_start.rjob.yamlenv/exploitgym/runner.pyenv/patcheval/claude_adapter/__init__.pyenv/patcheval/claude_adapter/__main__.pyenv/patcheval/claude_adapter/app.pyenv/patcheval/claude_adapter/conversion.pyenv/patcheval/claudecode_runner.pyenv/patcheval/generate_full_config.pygateway/anthropic_messages.pygateway/app.pygateway/config.pygateway/inference_forwarder.pygateway/models.pygateway/request_logger.pygateway/storage.pygateway/telemetry.pyrl/examples/patcheval/.gitignorerl/examples/patcheval/README.mdrl/examples/patcheval/run_eval.shtests/test_anthropic_messages.pytests/test_patcheval_claudecode.py
💤 Files with no reviewable changes (5)
- env/patcheval/claude_adapter/init.py
- env/patcheval/claude_adapter/main.py
- env/patcheval/claude_adapter/app.py
- env/patcheval/claude_adapter/conversion.py
- gateway/config.py
| # The S3 landing schema stores opaque JSON payloads. Preserve the | ||
| # extended Chat Completions document instead of coercing it through | ||
| # legacy ChatMessage/ContentItem models. | ||
| landing_messages = json.dumps( | ||
| full_messages, | ||
| ensure_ascii=False, | ||
| separators=(",", ":"), | ||
| ) | ||
| landing_response = json.dumps( | ||
| {"role": "assistant", "content": response}, | ||
| ensure_ascii=False, | ||
| separators=(",", ":"), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reuse the landing-value helpers so the create path matches the update path.
_messages_to_landing_value (Line 1057) and _response_to_landing_value (Line 1064) produce the same compact JSON, but they pass default=str. These two inline calls do not. A value that json cannot encode (for example a datetime or a numpy scalar left in a message) raises TypeError here and aborts the whole step write, while the same value is tolerated on the update path.
Reusing the helpers removes the duplication and the divergence.
♻️ Proposed refactor
- landing_messages = json.dumps(
- full_messages,
- ensure_ascii=False,
- separators=(",", ":"),
- )
- landing_response = json.dumps(
- {"role": "assistant", "content": response},
- ensure_ascii=False,
- separators=(",", ":"),
- )
+ landing_messages = self._messages_to_landing_value(full_messages)
+ landing_response = self._response_to_landing_value(
+ {"role": "assistant", "content": response}
+ )Note: _response_to_landing_value returns a JSON string unchanged when the input is a JSON-parsable string. Confirm that response is always intended to be wrapped as assistant content before you adopt the second line.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # The S3 landing schema stores opaque JSON payloads. Preserve the | |
| # extended Chat Completions document instead of coercing it through | |
| # legacy ChatMessage/ContentItem models. | |
| landing_messages = json.dumps( | |
| full_messages, | |
| ensure_ascii=False, | |
| separators=(",", ":"), | |
| ) | |
| landing_response = json.dumps( | |
| {"role": "assistant", "content": response}, | |
| ensure_ascii=False, | |
| separators=(",", ":"), | |
| ) | |
| # The S3 landing schema stores opaque JSON payloads. Preserve the | |
| # extended Chat Completions document instead of coercing it through | |
| # legacy ChatMessage/ContentItem models. | |
| landing_messages = self._messages_to_landing_value(full_messages) | |
| landing_response = self._response_to_landing_value( | |
| {"role": "assistant", "content": response} | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 658-662: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
full_messages,
ensure_ascii=False,
separators=(",", ":"),
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 663-667: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{"role": "assistant", "content": response},
ensure_ascii=False,
separators=(",", ":"),
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/data_manager/strategy/cloud_strategy_impl.py` around lines 656 - 668,
Update the create-path serialization around full_messages and response to reuse
_messages_to_landing_value and _response_to_landing_value, preserving compact
JSON and default=str behavior. Confirm the response helper’s JSON-string
passthrough does not bypass the intended {"role":"assistant","content":response}
wrapper; if necessary, construct that assistant payload before invoking the
helper.
| def _response_to_landing_value(self, value: Any) -> str: | ||
| if isinstance(value, str): | ||
| try: | ||
| json.loads(value) | ||
| except json.JSONDecodeError: | ||
| value = {"role": "assistant", "content": value} | ||
| elif isinstance(value, ChatMessage): | ||
| value = self._chat_message_to_landing_value(value) | ||
| elif not isinstance(value, dict): | ||
| value = {"role": "assistant", "content": str(value)} | ||
| return json.dumps(value, ensure_ascii=False, separators=(",", ":"), default=str) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
_response_to_landing_value double-encodes a JSON string input.
When value is a string that parses as JSON, the try block succeeds and value is never reassigned. Line 1074 then calls json.dumps on that string, which wraps it in quotes and escapes it. The stored column receives "{\"role\":\"assistant\",...}" instead of the intended object.
Only the JSONDecodeError branch reassigns value, so the pass-through case is the broken one. The existing tests cover _messages_to_landing_value only, so this path has no coverage.
🐛 Proposed fix
def _response_to_landing_value(self, value: Any) -> str:
if isinstance(value, str):
try:
json.loads(value)
+ return value
except json.JSONDecodeError:
value = {"role": "assistant", "content": value}
elif isinstance(value, ChatMessage):
value = self._chat_message_to_landing_value(value)
elif not isinstance(value, dict):
value = {"role": "assistant", "content": str(value)}
return json.dumps(value, ensure_ascii=False, separators=(",", ":"), default=str)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _response_to_landing_value(self, value: Any) -> str: | |
| if isinstance(value, str): | |
| try: | |
| json.loads(value) | |
| except json.JSONDecodeError: | |
| value = {"role": "assistant", "content": value} | |
| elif isinstance(value, ChatMessage): | |
| value = self._chat_message_to_landing_value(value) | |
| elif not isinstance(value, dict): | |
| value = {"role": "assistant", "content": str(value)} | |
| return json.dumps(value, ensure_ascii=False, separators=(",", ":"), default=str) | |
| def _response_to_landing_value(self, value: Any) -> str: | |
| if isinstance(value, str): | |
| try: | |
| json.loads(value) | |
| return value | |
| except json.JSONDecodeError: | |
| value = {"role": "assistant", "content": value} | |
| elif isinstance(value, ChatMessage): | |
| value = self._chat_message_to_landing_value(value) | |
| elif not isinstance(value, dict): | |
| value = {"role": "assistant", "content": str(value)} | |
| return json.dumps(value, ensure_ascii=False, separators=(",", ":"), default=str) |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 1073-1073: use jsonify instead of json.dumps for JSON output
Context: json.dumps(value, ensure_ascii=False, separators=(",", ":"), default=str)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/data_manager/strategy/cloud_strategy_impl.py` around lines 1064 - 1074,
Update _response_to_landing_value so a string containing valid JSON is parsed
and reassigned to the resulting JSON value before the final json.dumps call.
Preserve the existing assistant-message wrapping for invalid JSON strings and
the handling of ChatMessage and other value types.
| async def _ensure_runtime_schema(self) -> None: | ||
| if not self.db_url.startswith("sqlite://"): | ||
| raise ValueError("Only sqlite:// protocol is supported") | ||
|
|
||
| file_path = self.db_url[9:].split("?", 1)[0] | ||
|
|
||
| def add_missing_columns() -> None: | ||
| conn = sqlite3.connect(file_path) | ||
| try: | ||
| columns = { | ||
| str(row[1]) | ||
| for row in conn.execute("PRAGMA table_info(session_steps)") | ||
| } | ||
| if "request" not in columns: | ||
| conn.execute("ALTER TABLE session_steps ADD COLUMN request TEXT") | ||
| conn.commit() | ||
| finally: | ||
| conn.close() | ||
|
|
||
| await asyncio.to_thread(add_missing_columns) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the runtime migration resilient to concurrent initialization.
Two gaps exist in add_missing_columns:
- The connection does not set
PRAGMA busy_timeout._ensure_runtime_indexessetsbusy_timeout=5000on its own connection for the same file. If another writer holds the lock,ALTER TABLEfails immediately withdatabase is locked, andinit()raises. - The column check and the
ALTER TABLEare not atomic. If two processes initialize the same database file at the same time, both can read the column list before either alters the table. The secondALTER TABLEthen fails withduplicate column name: request.
Both failures abort startup for a purely idempotent migration.
🛡️ Proposed fix
def add_missing_columns() -> None:
conn = sqlite3.connect(file_path)
try:
+ conn.execute("PRAGMA busy_timeout=5000")
columns = {
str(row[1])
for row in conn.execute("PRAGMA table_info(session_steps)")
}
if "request" not in columns:
- conn.execute("ALTER TABLE session_steps ADD COLUMN request TEXT")
+ try:
+ conn.execute("ALTER TABLE session_steps ADD COLUMN request TEXT")
+ except sqlite3.OperationalError as exc:
+ if "duplicate column name" not in str(exc).lower():
+ raise
conn.commit()
finally:
conn.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def _ensure_runtime_schema(self) -> None: | |
| if not self.db_url.startswith("sqlite://"): | |
| raise ValueError("Only sqlite:// protocol is supported") | |
| file_path = self.db_url[9:].split("?", 1)[0] | |
| def add_missing_columns() -> None: | |
| conn = sqlite3.connect(file_path) | |
| try: | |
| columns = { | |
| str(row[1]) | |
| for row in conn.execute("PRAGMA table_info(session_steps)") | |
| } | |
| if "request" not in columns: | |
| conn.execute("ALTER TABLE session_steps ADD COLUMN request TEXT") | |
| conn.commit() | |
| finally: | |
| conn.close() | |
| await asyncio.to_thread(add_missing_columns) | |
| async def _ensure_runtime_schema(self) -> None: | |
| if not self.db_url.startswith("sqlite://"): | |
| raise ValueError("Only sqlite:// protocol is supported") | |
| file_path = self.db_url[9:].split("?", 1)[0] | |
| def add_missing_columns() -> None: | |
| conn = sqlite3.connect(file_path) | |
| try: | |
| conn.execute("PRAGMA busy_timeout=5000") | |
| columns = { | |
| str(row[1]) | |
| for row in conn.execute("PRAGMA table_info(session_steps)") | |
| } | |
| if "request" not in columns: | |
| try: | |
| conn.execute("ALTER TABLE session_steps ADD COLUMN request TEXT") | |
| except sqlite3.OperationalError as exc: | |
| if "duplicate column name" not in str(exc).lower(): | |
| raise | |
| conn.commit() | |
| finally: | |
| conn.close() | |
| await asyncio.to_thread(add_missing_columns) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/data_manager/strategy/sqlite_strategy_impl.py` around lines 106 - 125,
Update _ensure_runtime_schema’s add_missing_columns helper to set a busy timeout
on its SQLite connection before inspecting the schema, and make the
request-column migration safe when concurrent initializers race by handling an
already-existing column without failing. Preserve the existing connection
cleanup and commit behavior so the idempotent migration tolerates database locks
and duplicate-column races.
| if field in {"messages", "request"} and not isinstance(value, str): | ||
| value = json.dumps(value, ensure_ascii=False) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Handle None before JSON serialization.
None is not a str, so update_session_step(..., {"request": None}) stores the literal string "null" instead of SQL NULL. The column is nullable, so a caller can reasonably expect None to clear it.
🛠️ Proposed fix
- if field in {"messages", "request"} and not isinstance(value, str):
+ if field in {"messages", "request"} and value is not None and not isinstance(value, str):
value = json.dumps(value, ensure_ascii=False)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if field in {"messages", "request"} and not isinstance(value, str): | |
| value = json.dumps(value, ensure_ascii=False) | |
| if field in {"messages", "request"} and value is not None and not isinstance(value, str): | |
| value = json.dumps(value, ensure_ascii=False) |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 564-564: use jsonify instead of json.dumps for JSON output
Context: json.dumps(value, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/data_manager/strategy/sqlite_strategy_impl.py` around lines 564 - 565,
Update the field-value handling in update_session_step so None remains None for
nullable messages or request columns and is passed to SQLite as SQL NULL; only
JSON-serialize non-None values that are not already strings.
| EXPLOITGYM_UPSTREAM_BASE_URL="https://api.shanhaiengine.ai/v1" | ||
| EXPLOITGYM_UPSTREAM_API_KEY="sk-JdmL0UHBC4K0nP6OGqKsfUsHZjPi3IapBMTMKNjF5Ttt52xU" | ||
| POD_IP="100.103.199.47" No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Remove this file from version control and rotate the leaked API key.
Line 2 contains a live upstream API key. Anyone with repository access can read it. Static analysis also flags it.
Required actions:
- Revoke and rotate
EXPLOITGYM_UPSTREAM_API_KEYat the provider now. - Delete
env/exploitgym/.envfrom the repository and add it to.gitignore. - Purge the value from git history.
- Provide
env/exploitgym/.env.examplewith placeholder values instead.
POD_IP is also node-specific. Supply it at runtime rather than committing it.
🔒 Proposed replacement (`env/exploitgym/.env.example`)
-EXPLOITGYM_UPSTREAM_BASE_URL="https://api.shanhaiengine.ai/v1"
-EXPLOITGYM_UPSTREAM_API_KEY="sk-JdmL0UHBC4K0nP6OGqKsfUsHZjPi3IapBMTMKNjF5Ttt52xU"
-POD_IP="100.103.199.47"
+EXPLOITGYM_UPSTREAM_BASE_URL=https://api.example.com/v1
+EXPLOITGYM_UPSTREAM_API_KEY=
+POD_IP=🧰 Tools
🪛 Betterleaks (1.7.3)
[high] 2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 dotenv-linter (4.0.0)
[warning] 1-1: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 2-2: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 2-2: [UnorderedKey] The EXPLOITGYM_UPSTREAM_API_KEY key should go before the EXPLOITGYM_UPSTREAM_BASE_URL key
(UnorderedKey)
[warning] 3-3: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 3-3: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@env/exploitgym/.env` around lines 1 - 3, Remove env/exploitgym/.env from
version control, revoke and rotate the exposed EXPLOITGYM_UPSTREAM_API_KEY, and
purge the secret from git history. Add env/exploitgym/.env to .gitignore and
create env/exploitgym/.env.example containing placeholders for the upstream URL,
API key, and runtime-supplied POD_IP.
Source: Linters/SAST tools
| content_parts.extend({"type": "text", "text": text} for text in text_parts) | ||
| message["content"] = list(content_parts) | ||
| else: | ||
| message["content"] = "".join(text_parts) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Text blocks are concatenated without a separator.
"".join(text_parts) merges consecutive Anthropic text blocks with no delimiter. _normalize_system (Line 59) joins text blocks with "\n\n", and reasoning_parts (Line 104) uses the same separator. A message with two text blocks such as "first sentence" and "second sentence" becomes "first sentencesecond sentence".
Use the same separator for consistency, unless a caller depends on exact concatenation.
🛠️ Proposed fix
- message["content"] = "".join(text_parts)
+ message["content"] = "\n\n".join(text_parts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message["content"] = "".join(text_parts) | |
| message["content"] = "\n\n".join(text_parts) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/anthropic_messages.py` at line 102, Update the text-block assembly in
the message content handling to join text_parts with the same "\n\n" separator
used by _normalize_system and reasoning_parts, preserving clear boundaries
between consecutive Anthropic text blocks.
| if reasoning_parts: | ||
| message["reasoning_content"] = "\n\n".join(reasoning_parts) | ||
| if signatures: | ||
| if len(signatures) != 1: | ||
| raise AnthropicMessageConversionError( | ||
| "multiple thinking signatures in one Anthropic message are unsupported" | ||
| ) | ||
| message["encrypted_content"] = signatures[0] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Multiple thinking signatures abort the whole conversion.
Anthropic returns one signature per thinking block. With interleaved thinking enabled, one assistant message can contain several thinking blocks, so several signatures. This code then raises AnthropicMessageConversionError.
The consumer at gateway/storage.py:612-625 catches that error and falls back to record.messages, so the entire step loses its normalized form, not only the extra signature. That defeats the stated goal of preserving thinking signatures for multi-turn tool context.
The gateway configuration in this PR enables anthropic-beta: interleaved-thinking-2025-05-14, which makes multiple thinking blocks per message the expected shape rather than an edge case.
Consider keeping the signatures aligned with their reasoning segments, for example by storing a list, or by emitting one assistant message per thinking block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/anthropic_messages.py` around lines 103 - 110, Update the signature
handling in the Anthropic message conversion flow to support multiple signatures
from interleaved thinking blocks instead of raising
AnthropicMessageConversionError. Preserve the association between each signature
and its reasoning segment, using a list or equivalent normalized representation
compatible with downstream consumers such as storage conversion, while retaining
the existing single-signature behavior.
| stream_text = "".join(stream_text_parts) | ||
| telemetry_response_body = ( | ||
| None | ||
| if ctx.endpoint == "messages" | ||
| else _stream_response_body_for_telemetry( | ||
| summary=stream_response_body, | ||
| choice_states=stream_choice_states, | ||
| stream_text=stream_text, | ||
| stream_total_bytes=stream_total_bytes, | ||
| stream_truncated=False, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound native stream response retention.
The messages path now retains the complete SSE response and passes it to telemetry. A long-running or large stream can grow stream_text_parts without a limit, even when request logging is disabled or capped.
Capture only when telemetry sampling requires it. Apply a byte limit and mark truncated captures.
Also applies to: 1185-1186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/app.py` around lines 1144 - 1154, Bound native stream response
capture in the streaming flow that builds stream_text_parts: only accumulate
response content when telemetry sampling requires it, enforce the configured
byte limit, and stop retaining additional data once the limit is reached. Track
whether truncation occurred and pass that state through stream_truncated in both
telemetry response construction sites, including the messages path, without
retaining the complete SSE response when logging is disabled or capped.
| SENSITIVE_KEY_PARTS = ( | ||
| "authorization", | ||
| "api_key", | ||
| "token", | ||
| "password", | ||
| "secret", | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Apply redaction before telemetry serialization.
safe_request_body is created but record.request serializes the unredacted request_body. Sampled response bodies and raw SSE response_text also bypass redaction. The telemetry policy also omits signature and encrypted_content, although gateway/request_logger.py redacts both.
When redact_sensitive_fields=True, redact request and response payloads before assigning record.request, record.messages, and record.response. Use the same sensitive-key policy in both logging paths.
Also applies to: 542-542, 589-605, 733-743
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/telemetry.py` around lines 22 - 28, Update telemetry redaction around
SENSITIVE_KEY_PARTS and safe_request_body to include signature and
encrypted_content, matching gateway/request_logger.py. When
redact_sensitive_fields=True, redact request payloads before assigning
record.request, sampled response bodies before record.messages/record.response,
and raw SSE response_text before serialization. Ensure all telemetry paths use
the shared sensitive-key policy.
| request_method="POST" if target else None, | ||
| request_url=( | ||
| f"{target.base_url.rstrip('/')}/{ctx.endpoint}" | ||
| if target | ||
| else None | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the effective upstream query string.
gateway/app.py forwards non-beta query parameters for Anthropic Messages requests. request_url records only target.base_url and ctx.endpoint, so persisted metadata loses those parameters. The stored request metadata cannot reproduce the effective provider request.
Pass the filtered query string into telemetry and append it to request_url.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/telemetry.py` around lines 590 - 595, Update the telemetry request
metadata around request_url to accept the filtered non-beta query string
forwarded by gateway/app.py, and append it to the effective upstream URL when
present. Ensure the persisted request_url matches the actual Anthropic Messages
request while retaining the existing base URL and endpoint behavior when no
query parameters exist.
normalize anthropic messages to openai chat complete type.
testing in s3.
Summary by CodeRabbit
New Features
Bug Fixes
Configuration