Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the lightweight streaming dashboard and refactors the logging system to use a centralized LogContext and log IDs instead of run IDs. It also introduces a log redaction module to hide sensitive credentials, removes log_dir from TaskConfig, updates the Claude Code agent to support native installation when npm is missing, and enhances argument parsing for the file editor tool. The review feedback highlights a few critical issues: a class-level Semaphore in parallel_verify_swe.py that could raise a RuntimeError outside of an active event loop, a regex pattern in the redaction module that fails to handle escaped quotes, and unhandled command failures in the SWE-bench task execution when applying gold patches.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def __init__(self, log_dir: str | None): | ||
| self.log_dir = log_dir |
There was a problem hiding this comment.
Declaring _semaphore = asyncio.Semaphore(...) as a class-level attribute causes it to be instantiated at import time. In Python 3.10+, this will raise a RuntimeError: no running event loop if the module is imported outside of an active event loop (which is typical for CLI entry points).
To fix this, remove the class-level _semaphore attribute and initialize it inside __init__ where the event loop is guaranteed to be running.
| def __init__(self, log_dir: str | None): | |
| self.log_dir = log_dir | |
| def __init__(self, log_dir: str | None): | |
| self.log_dir = log_dir | |
| self._semaphore = asyncio.Semaphore(max(1, GLOBAL_CONCURRENCY // NUM_WORKERS)) |
|
|
||
| _QUOTED_VALUE_RE = re.compile( | ||
| rf"(?P<prefix>[\"']?{_SENSITIVE_NAME}[\"']?\s*[:=]\s*)" | ||
| rf"(?P<quote>[\"'])(?P<value>.*?)(?P=quote)", |
There was a problem hiding this comment.
The non-greedy pattern (?P<value>.*?) followed by (?P=quote) will stop matching at the first occurrence of the quote character, even if it is escaped (e.g., \" inside a double-quoted string). This can result in incomplete redaction of sensitive values that contain escaped quotes.
To handle escaped quotes correctly, use a pattern that explicitly matches escaped characters, such as:
rf"(?P<quote>[\"'])(?P<value>(?:(?!(?P=quote))[^\]|\.)*)(?P=quote)"
| rf"(?P<quote>[\"'])(?P<value>.*?)(?P=quote)", | |
| rf"(?P<quote>[\"'])(?P<value>(?:(?!(?P=quote))[^\\]|\\.)*)(?P=quote)", |
| if cfg.run_gold_patch: | ||
| logger.info("applying gold patch to /testbed") | ||
| await sandbox.write_file("/tmp/gold_patch.patch", sample["patch"]) | ||
| await sandbox.exec(["git", "apply", "--whitespace=fix", "/tmp/gold_patch.patch"], workdir="/testbed") |
There was a problem hiding this comment.
sandbox.exec returns an ExecResult and does not raise an exception if the command fails (returns a non-zero exit code). If git apply fails to apply the gold patch, the task will silently proceed to evaluate the unpatched codebase, leading to incorrect evaluation results.
Consider checking the exit code of the git apply command and raising an error if it fails.
| if cfg.run_gold_patch: | |
| logger.info("applying gold patch to /testbed") | |
| await sandbox.write_file("/tmp/gold_patch.patch", sample["patch"]) | |
| await sandbox.exec(["git", "apply", "--whitespace=fix", "/tmp/gold_patch.patch"], workdir="/testbed") | |
| if cfg.run_gold_patch: | |
| logger.info("applying gold patch to /testbed") | |
| await sandbox.write_file("/tmp/gold_patch.patch", sample["patch"]) | |
| res = await sandbox.exec(["git", "apply", "--whitespace=fix", "/tmp/gold_patch.patch"], workdir="/testbed") | |
| if res.exit_code != 0: | |
| raise RuntimeError(f"Failed to apply gold patch: {res.stderr}") |
| if cfg.run_gold_patch: | ||
| logger.info("applying gold patch to /testbed") | ||
| await sandbox.write_file("/tmp/gold_patch.patch", sample["patch"]) | ||
| await sandbox.exec(["git", "apply", "--whitespace=fix", "/tmp/gold_patch.patch"], workdir="/testbed") |
There was a problem hiding this comment.
sandbox.exec returns an ExecResult and does not raise an exception if the command fails. If git apply fails to apply the gold patch, the task will silently proceed to evaluate the unpatched codebase, leading to incorrect evaluation results.
Consider checking the exit code of the git apply command and raising an error if it fails.
| if cfg.run_gold_patch: | |
| logger.info("applying gold patch to /testbed") | |
| await sandbox.write_file("/tmp/gold_patch.patch", sample["patch"]) | |
| await sandbox.exec(["git", "apply", "--whitespace=fix", "/tmp/gold_patch.patch"], workdir="/testbed") | |
| if cfg.run_gold_patch: | |
| logger.info("applying gold patch to /testbed") | |
| await sandbox.write_file("/tmp/gold_patch.patch", sample["patch"]) | |
| res = await sandbox.exec(["git", "apply", "--whitespace=fix", "/tmp/gold_patch.patch"], workdir="/testbed") | |
| if res.exit_code != 0: | |
| raise RuntimeError(f"Failed to apply gold patch: {res.stderr}") |
) ### What does this PR do? This PR makes Gateway sessions reuse an existing chain when a client rewrites only that chain's latest Assistant message. The Gateway records the token/message/media boundary immediately before each Assistant turn. When the next request preserves the earlier prefix but replaces that Assistant turn, it rolls the chain back to the recorded boundary, re-encodes the replacement suffix, and continues generation on the same chain. This avoids materializing a redundant trajectory while preserving token IDs, response masks, rollout log probabilities, and multimodal inputs. Last-Assistant rollback is enabled by default. Set `actor_rollout_ref.rollout.custom.agent_framework.enable_last_assistant_rollback=false` to retain the previous split-on-rewrite behavior. ### Checklist Before Starting - [x] Search for similar PRs or issues and paste at least one relevant link here: #83, #85, and #87 above - [x] Format the PR title as `[{modules}] {type}: {description}` (checked by CI) ### Test Automated validation on `upstream/main` at `ecdddea` (#87): ```text PYTHONPATH=/tmp/uni-agent-python-isolation:/tmp/uni-agent-verl-78bba31 \ python -m pytest \ tests/uni_agent/gateway \ tests/uni_agent/framework/test_generate_sequences_on_cpu.py -q 166 passed, 4 warnings in 110.78s ``` All hooks pass for files changed by this PR: ```text pre-commit run --from-ref upstream/main --to-ref HEAD --show-diff-on-failure ruff: pass ruff-format: pass mypy: pass compileall: pass ``` `pre-commit run --all-files` additionally reformats `examples/blackbox_recipes/sandbox_client.py`, an unchanged file introduced by #87. That unrelated baseline formatting change is intentionally excluded from this focused PR. #### CC rollout experiment Full rollout configuration: ```text model: Qwen/Qwen3.5-4B samples: 8 rollouts per sample: 4 sessions: 32 max concurrent sessions: 4 max agent turns: 100 prompt length: 90112 response length: 49152 tensor parallelism: 4 GPU memory utilization: 0.90 ``` Results: | Metric | Result | |---|---:| | Successful sessions | 32 / 32 | | Failed session UIDs | 0 | | Sessions that triggered rollback | 32 / 32 (100%) | | Total rollbacks | 229 | | Mean rollbacks per session | 7.16 | | Rollback count P25 / median / P75 / max | 4 / 5.5 / 7 / 27 | | Dropped trainable tokens | 165,733 | | Mean dropped trainable tokens per rollback | 723.7 | | Output trajectories | 50 | | Extra trajectories | 18 | | `max_response_length` materializations | 18 | | Non-length split trajectories | 0 | | Resolved SWE-bench samples | 3 / 8 (37.5%) | All 50 trajectory records satisfied: ```text len(response_ids) == len(response_mask) == len(response_logprobs) ``` The full 8x4 run was collected on the #83-rebased rollback implementation. After rebasing onto #85/#87, the rollback implementation in `session.py` is byte-for-byte identical except that the feature default changes from `false` to `true` in this PR. A fresh #87 smoke run used `MAX_SAMPLES=2`, `N=1`, and the same model/token budgets. It completed 2/2 sessions, produced three trajectories (one `max_response_length` materialization), and all response ID/mask/log-probability arrays remained aligned. The CC recipe currently needs a separate post-#83 reward-wiring compatibility fix. That fix was applied only in the local rollout workspace and is not part of this PR. Observed task-level warnings included agents reaching the configured 100-turn limit, one malformed Tool-call parse, and one 143,813-token prompt exceeding the 140,288-token model context. These were not rollback or token-alignment failures; the full experiment still finalized all 32 sessions. ### API and Usage Example Rollback is enabled when the option is omitted: ```yaml actor_rollout_ref: rollout: custom: agent_framework: gateway_count: 1 ``` To preserve the previous behavior and split a rewritten latest Assistant into a new chain: ```yaml actor_rollout_ref: rollout: custom: agent_framework: enable_last_assistant_rollback: false ``` ### Design & Code Changes - Capture a stable `LastAssistantStart` boundary before every Assistant generation, including response IDs, response masks, log probabilities, message-history length, media lengths, and the prefix hash. - Rank exact-prefix and rollback candidates by reusable service depth, preferring exact continuation on ties and refusing ambiguous deepest rollback matches. - On rollback, truncate the selected chain to its recorded boundary and re-encode only the incoming replacement suffix. - Preserve response-log-probability alignment and fail loudly if stored token truth is already inconsistent. - Track `rollback_count` and `rollback_dropped_trainable_tokens_total` in the session snapshot. - Forward the configuration through framework entry -> Gateway actor -> Gateway session. The default is `true`; explicit `false` remains supported. - Cover exact matches, ambiguous candidates, reserved chains, multimodal suffix re-encoding, response-budget materialization, log-probability alignment, and explicit opt-out behavior. This PR intentionally does not include the independent CC reward compatibility fix or rollout snapshot logging used to collect the experiment metrics. ### Checklist Before Submitting - [x] Read the [Contribute Guide](../CONTRIBUTING.md) - [ ] Run `pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always` - All PR files pass; the all-files command finds the unrelated #87 baseline formatting issue documented above. - [x] Add or update docs/examples for user-facing changes - [x] Add tests or explain why tests are not practical - [x] Confirm the PR title matches the required format - [x] Confirm the placeholder text in this template has been replaced with real content
What does this PR do?
This PR centralizes logging ownership at execution entry points and makes per-session logging reliable across inline and Ray-dispatched runners.
Major Changes
LogContextpropagation across process boundaries.task.log.framework.logfor Ray-dispatched sessions.<log_dir>/step_<global_step>/<session_id>/<log_dir>/<session_id>/whenglobal_stepsisNonesession-sample-0-rollout-1-<uuid>.TaskConfig.log_dir, andTask.episode_logging().--log-dirand global--concurrencysupport to inference scripts.ANTHROPIC_AUTH_TOKEN.view_rangevalues instr_replace_editor.API and Usage Example
Framework logging is configured through:
Set
log_dirto an empty value to disable file logging.Breaking Changes
run.logis renamed totask.log.current_run_idget_loggeradd_file_handlercleanup_handlersinstall_console_sinkTaskConfig.log_dirandTask.episode_logging()are removed.Test