fix(acquisition): align managed Doubao runtime - #52
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds Doubao capability support, authenticated-profile and session readiness checks, target-aware validation, structured OpenCLI errors, trace hashing, adapter-specific runtime lineage, and verification of multiple pinned capability commits. ChangesOpenCLI capability integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CapabilityAPI
participant AcquisitionRunner
participant BrowserPool
participant OpenCLI
Client->>CapabilityAPI: submit target and prompt
CapabilityAPI->>AcquisitionRunner: dispatch registered capability
AcquisitionRunner->>BrowserPool: select and probe authenticated endpoint
AcquisitionRunner->>OpenCLI: execute Doubao capture
OpenCLI-->>AcquisitionRunner: payload, error, and trace hash
AcquisitionRunner-->>Client: execution result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
✅ Health: 7.3 📋 At a glance Files & modules (2)
📌 Before you merge
🗺️ Change map flowchart LR
subgraph PR ["Changed in this PR (1 with dependents)"]
f_backend_channels_opencli_channel_py["backend/channels/opencli_channel.py 🔥"]:::changed
end
f_backend_api_v1___init___py[".../v1/__init__.py"]
f_backend_channels_opencli_channel_py --> f_backend_api_v1___init___py
f_backend_api_v1_browsers_py[".../v1/browsers.py"]
f_backend_channels_opencli_channel_py --> f_backend_api_v1_browsers_py
f_backend_api_v1_nodes_py[".../v1/nodes.py"]
f_backend_channels_opencli_channel_py --> f_backend_api_v1_nodes_py
f_backend_api_v1_workers_py[".../v1/workers.py"]
f_backend_channels_opencli_channel_py --> f_backend_api_v1_workers_py
more(["+1 more dependent"])
PR --> more
t_tests_integration_test_opencli_channel_api_py(["✅ tests/integration/test_opencli_channel_api.py"]):::guard
t_tests_integration_test_opencli_channel_api_py -.-> f_backend_channels_opencli_channel_py
classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d
Solid arrows: code that imports the changed files (5 direct dependents, from the last indexed snapshot). Dashed: history/tests. 🚨 Change risk: 9.6/10 (high)
🔎 More signals (2)🔥 Hotspots touched (3)
💀 Dead code (1 finding)
👀 Suggested reviewers @xujinghua 📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-29 16:32 UTC |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
tests/unit/test_agent_image_runtime_packaging.py (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the official-site pin too.
The test now pins
OHMYOPENCLI_COMMITandDOUBAO_CAPABILITY_COMMIT, butARG OFFICIAL_SITE_CAPABILITY_COMMIT=73cc60c8...inagent/Dockerfileis no longer covered, so it can drift silently.💚 Proposed addition
assert ( "ARG DOUBAO_CAPABILITY_COMMIT=" "b0fdd513f64899b068103ddd7ff0de957d778b5c" ) in dockerfile + assert ( + "ARG OFFICIAL_SITE_CAPABILITY_COMMIT=" + "73cc60c83586ef2c95469b3b70d6cfc80fa5bc53" + ) in dockerfile🤖 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 `@tests/unit/test_agent_image_runtime_packaging.py` around lines 18 - 25, Extend the Dockerfile pin assertions in the test covering agent image runtime packaging to also verify the exact OFFICIAL_SITE_CAPABILITY_COMMIT value currently defined in agent/Dockerfile, alongside the existing OHMYOPENCLI_COMMIT and DOUBAO_CAPABILITY_COMMIT checks.backend/acquisition/runner.py (2)
439-442: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImporting a private helper across module boundaries.
_session_is_readyis module-private tobackend.acquisition.capabilitiesbut is now part of the runner's execution path. Consider exposing a publicsession_is_ready(...)wrapper there and calling that instead.🤖 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 `@backend/acquisition/runner.py` around lines 439 - 442, Replace the cross-module use of private helper _session_is_ready in the runner’s session_probe_args path with a public session_is_ready wrapper exposed by backend.acquisition.capabilities, preserving the existing arguments, await behavior, and readiness check.
490-498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated profile-failure-code derivation.
The same
no_clean_profile/no_{kind}_profileexpression appears at Lines 415-419 and again here. Extract a small helper (e.g._no_profile_code(kind)) so the two paths cannot drift.🤖 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 `@backend/acquisition/runner.py` around lines 490 - 498, Extract the duplicated profile-failure-code expression into a shared helper such as _no_profile_code(kind), then update both the earlier profile-failure path and the NoCleanProfileError handler in the execution flow to call it. Preserve the existing anonymous-to-no_clean_profile mapping and the no_{kind}_profile behavior for other profile kinds.backend/acquisition/registry.py (1)
81-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoubao dead-route probe prompt is duplicated with two different values. The registry and the runtime verifier each hardcode their own probe token, so the verifier does not exercise the exact route the registry probes.
backend/acquisition/registry.py#L81-L88: keepruntime-route-probeas the single source of truth (or export a shared constant) for the doubaoroute_probe_args.scripts/verify_managed_opencli_runtime.py#L106-L113: replace the divergentmanaged-runtime-route-probeliteral with the registry's token/constant.🤖 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 `@backend/acquisition/registry.py` around lines 81 - 88, The Doubao dead-route probe token is duplicated inconsistently between the registry and runtime verifier. Keep “runtime-route-probe” as the single source of truth in backend/acquisition/registry.py lines 81-88, optionally exposing a shared constant, and update scripts/verify_managed_opencli_runtime.py lines 106-113 to reuse that token instead of “managed-runtime-route-probe”.backend/acquisition/capabilities.py (1)
146-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared daemon-port value for bridge-mode env setup.
_browser_environmentstill setsOPENCLI_DAEMON_PORT = "19825"directly, while the bridge startup path is driven by_DAEMON_PORTinagent_server.py. If the default is changed there or viaOPENCLI_DAEMON_PORT, this session probe can use a different bridge port than the daemon the process launches. Reuse the same source-of-truth rather than duplicating"19825".🤖 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 `@backend/acquisition/capabilities.py` around lines 146 - 156, Update _browser_environment’s bridge-mode setup to derive OPENCLI_DAEMON_PORT from the shared _DAEMON_PORT source used by agent_server.py, rather than hardcoding "19825". Preserve the existing environment cleanup and non-bridge behavior.
🤖 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 `@backend/acquisition/capabilities.py`:
- Around line 241-252: Update the endpoint readiness logic in probe_capabilities
to avoid eagerly evaluating every _session_is_ready call: iterate through
endpoints, await each probe sequentially, and stop immediately when one returns
true. Preserve ready=false when no endpoint succeeds and keep the existing
unavailable_reason handling unchanged.
In `@backend/acquisition/runner.py`:
- Around line 655-663: Update the artifact reference condition in the runner
flow to require valid_trace in addition to trace_artifact and trace_sha256.
Reuse the existing valid_trace result from the trace validation logic,
preserving the current artifact_refs append behavior only when the digest passes
the established 64-character hexadecimal validation.
- Around line 61-69: In the error classification logic, update the marker tuple
used by the refusal check in the runner’s classification function to remove the
broad "refused" token. Preserve only markers that specifically indicate model
refusal, so connection errors such as ECONNREFUSED or "connection refused"
continue to flow to the transport/timeout/page-drift classification branches.
In `@scripts/install-agent.sh`:
- Around line 339-347: The installation guard in the OhMyOpenCLI setup must not
trigger `set -e` when `$OHMYOPENCLI_ROOT` is absent: replace the `[[ -e ... ]]
&& die` form with an explicit conditional that only calls `die` when the target
exists. In both `git -C "$OHMYOPENCLI_ROOT" merge-base --is-ancestor` checks,
append failure handling that calls `die` so capability ancestry validation
failures stop installation.
---
Nitpick comments:
In `@backend/acquisition/capabilities.py`:
- Around line 146-156: Update _browser_environment’s bridge-mode setup to derive
OPENCLI_DAEMON_PORT from the shared _DAEMON_PORT source used by agent_server.py,
rather than hardcoding "19825". Preserve the existing environment cleanup and
non-bridge behavior.
In `@backend/acquisition/registry.py`:
- Around line 81-88: The Doubao dead-route probe token is duplicated
inconsistently between the registry and runtime verifier. Keep
“runtime-route-probe” as the single source of truth in
backend/acquisition/registry.py lines 81-88, optionally exposing a shared
constant, and update scripts/verify_managed_opencli_runtime.py lines 106-113 to
reuse that token instead of “managed-runtime-route-probe”.
In `@backend/acquisition/runner.py`:
- Around line 439-442: Replace the cross-module use of private helper
_session_is_ready in the runner’s session_probe_args path with a public
session_is_ready wrapper exposed by backend.acquisition.capabilities, preserving
the existing arguments, await behavior, and readiness check.
- Around line 490-498: Extract the duplicated profile-failure-code expression
into a shared helper such as _no_profile_code(kind), then update both the
earlier profile-failure path and the NoCleanProfileError handler in the
execution flow to call it. Preserve the existing anonymous-to-no_clean_profile
mapping and the no_{kind}_profile behavior for other profile kinds.
In `@tests/unit/test_agent_image_runtime_packaging.py`:
- Around line 18-25: Extend the Dockerfile pin assertions in the test covering
agent image runtime packaging to also verify the exact
OFFICIAL_SITE_CAPABILITY_COMMIT value currently defined in agent/Dockerfile,
alongside the existing OHMYOPENCLI_COMMIT and DOUBAO_CAPABILITY_COMMIT checks.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7c0d375-7d2c-4014-a3a9-be0c9860f739
📒 Files selected for processing (21)
Dockerfileagent/Dockerfilebackend/acquisition/capabilities.pybackend/acquisition/registry.pybackend/acquisition/runner.pybackend/agent_server.pybackend/api/v1/geo_acquisition.pybackend/channels/opencli_channel.pybackend/channels/opencli_support.pybackend/schemas/acquisition.pyscripts/install-agent.shscripts/install-managed-opencli.ps1scripts/verify_managed_opencli_runtime.pytests/unit/channels/test_opencli_channel.pytests/unit/test_acquisition_capabilities.pytests/unit/test_acquisition_runner.pytests/unit/test_agent_image_runtime_packaging.pytests/unit/test_agent_server.pytests/unit/test_doubao_acquisition_runner.pytests/unit/test_geo_acquisition_api.pytests/unit/test_managed_opencli_verifier.py
| if ready and registration.session_probe_args: | ||
| ready = any( | ||
| [ | ||
| await _session_is_ready(registration, pool, endpoint) | ||
| for endpoint in endpoints | ||
| ] | ||
| ) | ||
| if not ready: | ||
| unavailable_reason = ( | ||
| registration.session_unavailable_reason | ||
| or "browser_session_not_ready" | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
any([... for endpoint in endpoints]) probes every endpoint even after one succeeds.
ready = any(
[
await _session_is_ready(registration, pool, endpoint)
for endpoint in endpoints
]
)The list comprehension eagerly awaits _session_is_ready for every endpoint before any() is applied — there's no short-circuiting once a ready endpoint is found. probe_capabilities() runs on the hot submission path (_validate_capability in backend/api/v1/geo_acquisition.py calls it per request), so with multiple authenticated endpoints this needlessly serializes extra subprocess/HTTP round-trips (each probe can take up to COMMAND_TIMEOUT_SECONDS) on every capability check.
⚡ Proposed fix: short-circuit with an explicit loop
if ready and registration.session_probe_args:
- ready = any(
- [
- await _session_is_ready(registration, pool, endpoint)
- for endpoint in endpoints
- ]
- )
+ ready = False
+ for endpoint in endpoints:
+ if await _session_is_ready(registration, pool, endpoint):
+ ready = True
+ break🤖 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 `@backend/acquisition/capabilities.py` around lines 241 - 252, Update the
endpoint readiness logic in probe_capabilities to avoid eagerly evaluating every
_session_is_ready call: iterate through endpoints, await each probe
sequentially, and stop immediately when one returns true. Preserve ready=false
when no endpoint succeeds and keep the existing unavailable_reason handling
unchanged.
| if any( | ||
| marker in normalized | ||
| for marker in ( | ||
| "refusal", | ||
| "refused", | ||
| "doubao_capture_refusal", | ||
| ) | ||
| ): | ||
| return "model_refusal" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"refused" will misclassify connection errors as model_refusal.
Messages like connect ECONNREFUSED 127.0.0.1:9222 or connection refused contain refused and are mapped to model_refusal, which mislabels a transport failure as a model-side refusal (and is checked before the timeout/page-drift branches). Tighten the marker set.
🐛 Proposed fix
if any(
marker in normalized
for marker in (
- "refusal",
- "refused",
+ "refusal",
+ "model refused",
+ "refused to answer",
"doubao_capture_refusal",
)
):
return "model_refusal"📝 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 any( | |
| marker in normalized | |
| for marker in ( | |
| "refusal", | |
| "refused", | |
| "doubao_capture_refusal", | |
| ) | |
| ): | |
| return "model_refusal" | |
| if any( | |
| marker in normalized | |
| for marker in ( | |
| "refusal", | |
| "model refused", | |
| "refused to answer", | |
| "doubao_capture_refusal", | |
| ) | |
| ): | |
| return "model_refusal" |
🤖 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 `@backend/acquisition/runner.py` around lines 61 - 69, In the error
classification logic, update the marker tuple used by the refusal check in the
runner’s classification function to remove the broad "refused" token. Preserve
only markers that specifically indicate model refusal, so connection errors such
as ECONNREFUSED or "connection refused" continue to flow to the
transport/timeout/page-drift classification branches.
| if trace_artifact and trace_sha256: | ||
| artifact_refs = [ | ||
| *artifact_refs, | ||
| {"kind": "trace", "ref": result.metadata["trace_artifact"]}, | ||
| { | ||
| "kind": "trace", | ||
| "ref": trace_artifact, | ||
| "sha256": trace_sha256, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Gate the trace artifact ref on valid_trace, not just truthiness.
valid_trace (Lines 610-615) requires a 64-char hex digest, but the artifact ref here only requires both values to be non-empty. When required_artifacts is empty, a malformed trace_sha256 is still persisted as an artifact hash, so stored artifact refs can carry a digest the runner itself rejected.
🐛 Proposed fix
- if trace_artifact and trace_sha256:
+ if valid_trace:📝 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 trace_artifact and trace_sha256: | |
| artifact_refs = [ | |
| *artifact_refs, | |
| {"kind": "trace", "ref": result.metadata["trace_artifact"]}, | |
| { | |
| "kind": "trace", | |
| "ref": trace_artifact, | |
| "sha256": trace_sha256, | |
| }, | |
| ] | |
| if valid_trace: | |
| artifact_refs = [ | |
| *artifact_refs, | |
| { | |
| "kind": "trace", | |
| "ref": trace_artifact, | |
| "sha256": trace_sha256, | |
| }, | |
| ] |
🤖 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 `@backend/acquisition/runner.py` around lines 655 - 663, Update the artifact
reference condition in the runner flow to require valid_trace in addition to
trace_artifact and trace_sha256. Reuse the existing valid_trace result from the
trace validation logic, preserving the current artifact_refs append behavior
only when the digest passes the established 64-character hexadecimal validation.
| command -v git >/dev/null 2>&1 || die "git is required to install OhMyOpenCLI" | ||
| [[ -e "$OHMYOPENCLI_ROOT" ]] && die \ | ||
| "Managed OhMyOpenCLI target already exists; archive it explicitly before reinstalling: $OHMYOPENCLI_ROOT" | ||
| git clone "$OHMYOPENCLI_REPO" "$OHMYOPENCLI_ROOT" | ||
| git -C "$OHMYOPENCLI_ROOT" checkout --detach "$OHMYOPENCLI_COMMIT" | ||
| git -C "$OHMYOPENCLI_ROOT" merge-base --is-ancestor \ | ||
| "$OFFICIAL_SITE_CAPABILITY_COMMIT" HEAD | ||
| git -C "$OHMYOPENCLI_ROOT" merge-base --is-ancestor \ | ||
| "$DOUBAO_CAPABILITY_COMMIT" HEAD |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether errexit is enabled in the installer and how `die` is used elsewhere.
rg -n 'set -[a-z]*e|set -o errexit|^die\(\)|die\(\) *\{' scripts/install-agent.sh
rg -n -C2 '\]\] && die' scripts/install-agent.shRepository: 2233admin/opencli-admin
Length of output: 784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant git block and surrounding function, then probe the bash errexit AND-list behavior
# for the exact pattern used in line 340 under a subshell.
sed -n '1,45p' scripts/install-agent.sh
sed -n '330,360p' scripts/install-agent.sh
bash -c 'set -e; [[ -e /nonexistent ]] && { echo unreachable; }; echo after' || echo "subshell exited due errexit on absent [[ -e ]]"
bash -c 'set -e; [[ -e /tmp ]] && { echo exists; }; echo after'Repository: 2233admin/opencli-admin
Length of output: 4512
🌐 Web query:
Bash man errexit AND lists && lists set -e non-zero exit status pipeline exit commands
💡 Result:
In Bash, the errexit option (enabled via set -e or set -o errexit) causes the shell to exit immediately if a command exits with a non-zero status [1]. However, there are specific exceptions to this rule where non-zero exit statuses do not trigger an immediate exit [2][1]. Exceptions for errexit: The errexit option is ignored for commands that are part of the following constructs [2][1]: 1. The condition list following if, while, or until keywords [2][1]. 2. Any command in an AND (&&) or OR (||) list, except for the command following the final operator [2]. 3. Any command in a pipeline, except for the last command [2]. 4. Any command whose return status is being inverted via the! reserved word [3][2]. For pipelines specifically, the return status is normally the exit status of the last command [3]. If the pipefail option is enabled (set -o pipefail), the pipeline's return status is the value of the last (rightmost) command to exit with a non-zero status, or zero if all commands exit successfully [3][4]. When using errexit, if a command that would normally trigger an exit is placed in one of these exempted positions, the shell continues execution [2][1]. Note that if a command is the final command in an AND/OR list or the last command in a pipeline, its non-zero exit status will still cause the shell to exit (unless otherwise protected by an if, while, or until condition) [2].
Citations:
- 1: https://stratus3d.com/blog/2019/11/29/bash-errexit-inconsistency
- 2: https://unix.stackexchange.com/questions/699108/why-does-a-command-with-a-non-zero-exit-status-send-an-err-signal-even-when-it-i
- 3: https://man7.org/linux/man-pages/man1/bash.1.html
- 4: https://man.archlinux.org/man/bash.1
Handle the missing OhMyOpenCLI target and pin ancestry checks explicitly.
With set -euo pipefail, [[ -e "$OHMYOPENCLI_ROOT" ]] && die will exit when the target path is absent before git clone; use an if guard unless missing is an error. Add || die to both merge-base --is-ancestor checks so unsupported or absent capability commits stop installation instead of continuing with an unverified checkout.
🤖 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 `@scripts/install-agent.sh` around lines 339 - 347, The installation guard in
the OhMyOpenCLI setup must not trigger `set -e` when `$OHMYOPENCLI_ROOT` is
absent: replace the `[[ -e ... ]] && die` form with an explicit conditional that
only calls `die` when the target exists. In both `git -C "$OHMYOPENCLI_ROOT"
merge-base --is-ancestor` checks, append failure handling that calls `die` so
capability ancestry validation failures stop installation.
What changed
Why
The authenticated browser acquisition path must use the published Doubao adapter and a clean local CDP environment before geo-xi can retain real answers, citations, traces, and screenshots.
Validation