feat: OpenClaw integration test (WOR-213) - #53
Merged
Conversation
…OR-213) Two-container Docker Compose stack (mock-upstream + worthless-proxy) proves the proxy reconstructs the real API key from shards and forwards it upstream. Adds env var overrides for adapter upstream URLs to enable mock redirection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Numbered steps with inline values showing .env before/after lock, shard state, stolen-key analysis, and upstream key verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hits real api.openai.com: shard-A alone → 401, decoy → 401, reconstructed key → 200/429 (recognized). No Docker needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds OpenClaw integration tests and Docker test infrastructure, plus makes Anthropic and OpenAI adapter upstream URLs configurable via environment variables. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant DockerCompose as "Docker Compose"
participant Proxy as "worthless-proxy"
participant Mock as "mock-upstream (FastAPI)"
rect rgba(200,230,255,0.5)
Tester->>DockerCompose: docker compose up (openclaw profile)
DockerCompose->>Proxy: start service (wait healthy)
DockerCompose->>Mock: start service (healthcheck)
end
Tester->>Proxy: POST /{alias}/v1/chat/completions (Authorization: Bearer <shard_a>)
Proxy->>Proxy: reconstruct key using shard_a + locked data
Proxy->>Mock: Forward POST to /v1/chat/completions (Authorization: Bearer <original_key>)
Mock->>Mock: record Authorization header
Mock->>Tester: respond (JSON or SSE)
Tester->>Mock: GET /captured-headers
Mock->>Tester: return recorded headers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/openclaw/run-test.sh (1)
23-23: Consider preserving stderr for build failures.Suppressing all output (
>/dev/null 2>&1) duringdocker compose up --buildhides potential build errors. Consider redirecting only stdout while preserving stderr for diagnostics:♻️ Suggested change
-docker compose -f "$COMPOSE_FILE" -p "$PROJECT" up -d --build >/dev/null 2>&1 +docker compose -f "$COMPOSE_FILE" -p "$PROJECT" up -d --build >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openclaw/run-test.sh` at line 23, The current command invocation "docker compose -f \"$COMPOSE_FILE\" -p \"$PROJECT\" up -d --build >/dev/null 2>&1" suppresses stderr and hides build errors; modify the redirection so only stdout is discarded (e.g., keep ">/dev/null" but remove "2>&1") so that stderr remains visible for diagnostics during the "docker compose ... up -d --build" step in run-test.sh.tests/test_openclaw_e2e.py (2)
61-67: Unused helper function_docker_exec.This function is defined but never called in the module. Consider removing it to avoid dead code, or if it's intended for future use, add a comment.
🧹 Remove unused function
-def _docker_exec(container: str, cmd: list[str]) -> subprocess.CompletedProcess[str]: - """Execute a command inside a running container.""" - return subprocess.run( - ["docker", "exec", container, *cmd], - capture_output=True, - text=True, - ) - -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_openclaw_e2e.py` around lines 61 - 67, The helper function _docker_exec is defined but never used; either remove the dead function to clean up the test module or keep it and add a short clarifying comment (or a TODO) above _docker_exec explaining its intended future use (e.g., for running commands inside test containers) so linters and readers know it's intentional; update any imports/usages if you move it to a shared test util instead.
70-103:_wait_healthyassumes healthcheck is configured.The function loops until timeout when the container has no healthcheck (status returns empty string). Once the missing healthcheck in
docker-compose.ymlis fixed, this will work correctly.Optionally, you could add early detection for "no healthcheck configured" to provide a clearer error message:
💡 Optional: detect missing healthcheck early
def _wait_healthy(container: str, timeout: float = 90.0) -> bool: """Poll container health status until healthy or timeout.""" deadline = time.monotonic() + timeout while time.monotonic() < deadline: result = subprocess.run( [ "docker", "inspect", "--format", - "{{.State.Health.Status}}", + "{{if .State.Health}}{{.State.Health.Status}}{{else}}no-healthcheck{{end}}", container, ], capture_output=True, text=True, ) status = result.stdout.strip() if status == "healthy": return True + if status == "no-healthcheck": + raise RuntimeError(f"Container {container} has no healthcheck defined") if status in ("unhealthy", ""):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_openclaw_e2e.py` around lines 70 - 103, The _wait_healthy function currently treats an empty health status string as "no healthcheck" and will loop until timeout; change it to detect a missing healthcheck early by checking for empty result.stdout from docker inspect for "{{.State.Health.Status}}" and returning False (or raising a clear exception/logging an explicit error) immediately with a message like "no healthcheck configured" rather than repeatedly polling; update the logic inside _wait_healthy and any callers/tests that expect a timeout so they handle this early-fail behavior (refer to the _wait_healthy function and the docker inspect calls using "{{.State.Health.Status}}").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/openclaw/docker-compose.yml`:
- Around line 26-56: The worthless-proxy service lacks a Docker healthcheck,
causing dependent services and the test helper _wait_healthy(proxy_container,
timeout=90) to wait until timeout; add a healthcheck stanza to the
worthless-proxy service that probes the proxy's real health endpoint (use the
correct path served by worthless-proxy, e.g., an HTTP GET to /health or the
proxy-specific status path) with a short timeout, interval and retries so Docker
reports "healthy" once the proxy is ready and tests depending on condition:
service_healthy succeed.
In `@tests/test_openclaw_live.py`:
- Around line 64-67: The test prints partially redacted API keys using
_redact(OPENAI_KEY) which CodeQL flagged; to address this, update the test
(e.g., in tests/test_openclaw_live.py referencing _redact and OPENAI_KEY) to
further minimize exposure by shortening the visible prefix/suffix (for example
show only first 6 chars and last 2) and/or gate the prints behind a verbosity
flag or environment variable so redacted values are only emitted when a
verbose/live-debug mode is explicitly set; ensure changes apply to the print
statements that show shard_a/shard_b and the _redact call so CI/public logs do
not reveal more than the reduced characters unless explicitly enabled.
---
Nitpick comments:
In `@tests/openclaw/run-test.sh`:
- Line 23: The current command invocation "docker compose -f \"$COMPOSE_FILE\"
-p \"$PROJECT\" up -d --build >/dev/null 2>&1" suppresses stderr and hides build
errors; modify the redirection so only stdout is discarded (e.g., keep
">/dev/null" but remove "2>&1") so that stderr remains visible for diagnostics
during the "docker compose ... up -d --build" step in run-test.sh.
In `@tests/test_openclaw_e2e.py`:
- Around line 61-67: The helper function _docker_exec is defined but never used;
either remove the dead function to clean up the test module or keep it and add a
short clarifying comment (or a TODO) above _docker_exec explaining its intended
future use (e.g., for running commands inside test containers) so linters and
readers know it's intentional; update any imports/usages if you move it to a
shared test util instead.
- Around line 70-103: The _wait_healthy function currently treats an empty
health status string as "no healthcheck" and will loop until timeout; change it
to detect a missing healthcheck early by checking for empty result.stdout from
docker inspect for "{{.State.Health.Status}}" and returning False (or raising a
clear exception/logging an explicit error) immediately with a message like "no
healthcheck configured" rather than repeatedly polling; update the logic inside
_wait_healthy and any callers/tests that expect a timeout so they handle this
early-fail behavior (refer to the _wait_healthy function and the docker inspect
calls using "{{.State.Health.Status}}").
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d99f66b3-22bb-4255-bca5-4b165503009c
📒 Files selected for processing (10)
pyproject.tomlsrc/worthless/adapters/anthropic.pysrc/worthless/adapters/openai.pytests/openclaw/docker-compose.ymltests/openclaw/mock-upstream/Dockerfiletests/openclaw/mock-upstream/app.pytests/openclaw/openclaw-config/openclaw.jsontests/openclaw/run-test.shtests/test_openclaw_e2e.pytests/test_openclaw_live.py
Comment on lines
+26
to
+56
| worthless-proxy: | ||
| build: | ||
| context: ../../ | ||
| dockerfile: Dockerfile | ||
| ports: | ||
| - "127.0.0.1::8787" | ||
| environment: | ||
| WORTHLESS_ALLOW_INSECURE: "true" | ||
| WORTHLESS_ALLOW_ALIAS_INFERENCE: "true" | ||
| WORTHLESS_UPSTREAM_OPENAI_URL: "http://mock-upstream:9999/v1/chat/completions" | ||
| WORTHLESS_FERNET_KEY_PATH: /secrets/fernet.key | ||
| volumes: | ||
| - worthless-data:/data | ||
| - worthless-secrets:/secrets | ||
| networks: | ||
| - openclaw-net | ||
| depends_on: | ||
| mock-upstream: | ||
| condition: service_healthy | ||
| read_only: true | ||
| tmpfs: | ||
| - /tmp:noexec,nosuid,size=64m | ||
| cap_drop: | ||
| - ALL | ||
| security_opt: | ||
| - no-new-privileges:true | ||
| deploy: | ||
| resources: | ||
| limits: | ||
| memory: 512M | ||
| cpus: "1.0" |
There was a problem hiding this comment.
Missing healthcheck for worthless-proxy — tests will timeout.
The worthless-proxy service lacks a healthcheck, but:
- The
openclawservice (line 73-74) depends on it withcondition: service_healthy - The test fixture in
test_openclaw_e2e.pycalls_wait_healthy(proxy_container, timeout=90)which polls for"healthy"status
Without a healthcheck, Docker returns an empty health status, causing _wait_healthy to loop until timeout and return False, failing the test.
🐛 Proposed fix: add healthcheck for worthless-proxy
worthless-proxy:
build:
context: ../../
dockerfile: Dockerfile
ports:
- "127.0.0.1::8787"
environment:
WORTHLESS_ALLOW_INSECURE: "true"
WORTHLESS_ALLOW_ALIAS_INFERENCE: "true"
WORTHLESS_UPSTREAM_OPENAI_URL: "http://mock-upstream:9999/v1/chat/completions"
WORTHLESS_FERNET_KEY_PATH: /secrets/fernet.key
volumes:
- worthless-data:/data
- worthless-secrets:/secrets
networks:
- openclaw-net
depends_on:
mock-upstream:
condition: service_healthy
+ healthcheck:
+ test: ["CMD", "curl", "-f", "http://localhost:8787/health"]
+ interval: 5s
+ timeout: 3s
+ start_period: 5s
+ retries: 3
read_only: trueAdjust the endpoint path to match the actual health endpoint exposed by the proxy.
📝 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.
Suggested change
| worthless-proxy: | |
| build: | |
| context: ../../ | |
| dockerfile: Dockerfile | |
| ports: | |
| - "127.0.0.1::8787" | |
| environment: | |
| WORTHLESS_ALLOW_INSECURE: "true" | |
| WORTHLESS_ALLOW_ALIAS_INFERENCE: "true" | |
| WORTHLESS_UPSTREAM_OPENAI_URL: "http://mock-upstream:9999/v1/chat/completions" | |
| WORTHLESS_FERNET_KEY_PATH: /secrets/fernet.key | |
| volumes: | |
| - worthless-data:/data | |
| - worthless-secrets:/secrets | |
| networks: | |
| - openclaw-net | |
| depends_on: | |
| mock-upstream: | |
| condition: service_healthy | |
| read_only: true | |
| tmpfs: | |
| - /tmp:noexec,nosuid,size=64m | |
| cap_drop: | |
| - ALL | |
| security_opt: | |
| - no-new-privileges:true | |
| deploy: | |
| resources: | |
| limits: | |
| memory: 512M | |
| cpus: "1.0" | |
| worthless-proxy: | |
| build: | |
| context: ../../ | |
| dockerfile: Dockerfile | |
| ports: | |
| - "127.0.0.1::8787" | |
| environment: | |
| WORTHLESS_ALLOW_INSECURE: "true" | |
| WORTHLESS_ALLOW_ALIAS_INFERENCE: "true" | |
| WORTHLESS_UPSTREAM_OPENAI_URL: "http://mock-upstream:9999/v1/chat/completions" | |
| WORTHLESS_FERNET_KEY_PATH: /secrets/fernet.key | |
| volumes: | |
| - worthless-data:/data | |
| - worthless-secrets:/secrets | |
| networks: | |
| - openclaw-net | |
| depends_on: | |
| mock-upstream: | |
| condition: service_healthy | |
| healthcheck: | |
| test: ["CMD", "curl", "-f", "http://localhost:8787/health"] | |
| interval: 5s | |
| timeout: 3s | |
| start_period: 5s | |
| retries: 3 | |
| read_only: true | |
| tmpfs: | |
| - /tmp:noexec,nosuid,size=64m | |
| cap_drop: | |
| - ALL | |
| security_opt: | |
| - no-new-privileges:true | |
| deploy: | |
| resources: | |
| limits: | |
| memory: 512M | |
| cpus: "1.0" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/openclaw/docker-compose.yml` around lines 26 - 56, The worthless-proxy
service lacks a Docker healthcheck, causing dependent services and the test
helper _wait_healthy(proxy_container, timeout=90) to wait until timeout; add a
healthcheck stanza to the worthless-proxy service that probes the proxy's real
health endpoint (use the correct path served by worthless-proxy, e.g., an HTTP
GET to /health or the proxy-specific status path) with a short timeout, interval
and retries so Docker reports "healthy" once the proxy is ready and tests
depending on condition: service_healthy succeed.
Update all integration tests for the new proxy model: - Alias from URL path (/<alias>/v1/...), not x-worthless-key header - Shard-A from Authorization Bearer (format-preserving), not disk file - Use lock instead of enroll (matches production flow) - Remove alias inference test (feature removed in #55) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| def _make_alias(provider: str, api_key: str) -> str: | ||
| """Deterministic alias: provider + first 8 hex chars of sha256(key).""" | ||
| digest = hashlib.sha256(api_key.encode()).hexdigest()[:8] |
| prefix = detect_prefix(OPENAI_KEY, "openai") | ||
| sr = split_key_fp(OPENAI_KEY, prefix, "openai") | ||
| shard_a = sr.shard_a.decode("utf-8") | ||
| print(f"1. Real key: {_redact(OPENAI_KEY)}") |
…WOR-213) Shard-A alone, shard-B alone, bitflip, truncation, cross-contamination all rejected (401). Correct reconstruction accepted by both providers. Format-preserving split verified against real APIs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
deploy/docker-compose.yml hardcodes 127.0.0.1:8787. When another process holds the port, TestComposeSecurity fails. Fix: compose override file binds a dynamic host port, matching the pattern used by other fixtures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_docker_e2e.py (1)
247-298:⚠️ Potential issue | 🟠 MajorOverride file collides with Docker Compose convention and will clobber a developer's local
docker-compose.override.yml.
deploy/docker-compose.override.ymlis Docker Compose's conventional auto-loaded override filename. Many contributors keep a personal one indeploy/for dev tweaks (debug ports, bind mounts, etc.). This fixture unconditionally writes that path and thenunlinks it in thefinally, which will silently destroy any pre-existing file. It will also race across parallel pytest workers (pytest-xdist) or concurrent runs on the same repo checkout.Prefer a uniquely-named, per-test path (e.g. under
tempfile.mkdtemp()or underREPO_ROOT / "deploy" / f".compose-override-{project}.yml") so you never touch the canonical override name and each run gets its own file.🛠️ Suggested fix
- override_file = REPO_ROOT / "deploy" / "docker-compose.override.yml" + # Use a project-unique filename to avoid clobbering a developer's + # personal deploy/docker-compose.override.yml (auto-loaded by Compose + # by convention) and to be safe under parallel test runs. + override_file = REPO_ROOT / "deploy" / f".compose-override-{project}.yml"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_docker_e2e.py` around lines 247 - 298, The test currently writes and unlinks the canonical override filename (override_file = REPO_ROOT / "deploy" / "docker-compose.override.yml"), which can clobber a developer's local file and race with parallel runs; change it to create a unique per-test override path (e.g. use tempfile.mkstemp()/TemporaryDirectory or build REPO_ROOT / "deploy" / f".compose-override-{project}-{pid or uuid}.yml") and assign that to override_file, write the YAML there, pass that path into the docker compose commands, and only unlink that unique file in the finally block so you never touch the canonical docker-compose.override.yml and avoid cross-run races (refer to override_file, REPO_ROOT, project, and the docker compose invocation in the try/finally).
🧹 Nitpick comments (4)
tests/test_openclaw_e2e.py (2)
332-336: Redundant assertion pair.Line 334 already asserts
entry["authorization"] == f"Bearer {fake_key}", which implies line 333'sshard_a not in entry["authorization"](sinceshard_a != fake_key). The first assertion is dead weight and the first failure will always be the exact-equality one, which also gives a clearer error. Consider dropping line 333 (or keeping only thenot incheck with a sharper message if the intent is specifically to document the leak property).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_openclaw_e2e.py` around lines 332 - 336, The two assertions inside the for loop over captured["headers"] are redundant: the equality check entry["authorization"] == f"Bearer {fake_key}" already guarantees shard_a won't appear, so remove the first assertion that checks shard_a not in entry["authorization"] (or, if you specifically want to assert a non-leak property instead of exact equality, replace the equality check with the not-in check and add a sharper failure message). Update the loop that uses captured, shard_a and fake_key accordingly so only the single, intended assertion remains.
148-215: Fixture yield shape depends on positional tuple unpacking in every test — consider a small dataclass/NamedTuple.Every test does
proxy_port, mock_port, fake_key, shard_a, alias = openclaw_stack. Adding a sixth field later (e.g.mock_containerfor logs on failure) requires touching every test. Atyping.NamedTuple(OpenClawStack(proxy_port, mock_port, fake_key, shard_a, alias)) keeps the current destructuring-compatible shape while giving names for IDE/typechecker. Purely optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_openclaw_e2e.py` around lines 148 - 215, The fixture openclaw_stack currently yields a bare tuple (proxy_port, mock_port, fake_key, shard_a, alias), which forces positional unpacking in every test; replace that with a small typing.NamedTuple (e.g. OpenClawStack with fields proxy_port, mock_port, fake_key, shard_a, alias) and have openclaw_stack yield an OpenClawStack instance instead of a raw tuple so tests can still unpack positionally but also access named attributes for clarity and future extension; update the fixture return creation to construct OpenClawStack(...) and leave test unpacking unchanged (NamedTuple preserves tuple unpacking semantics while providing attribute names).tests/openclaw/run-test.sh (2)
47-50: Interpolating$REAL_KEYinto inline Python source is fragile.
print(f'openai-{hashlib.sha256(\"$REAL_KEY\".encode()).hexdigest()[:8]}')embeds the key as a literal inside the Python source. It works today only becausefake_openai_key()returns a base64url-safe charset; the moment this script is retargeted at a real key (or a key containing a quote, backslash,$, or newline), you'll get a PythonSyntaxErroror, worse, arbitrary code execution via the shell/Python layer.Prefer passing the key through the environment and reading it with
os.environ:♻️ Safer interpolation
-ALIAS=$(cd "$REPO_ROOT" && uv run python3 -c " -import hashlib -print(f'openai-{hashlib.sha256(\"$REAL_KEY\".encode()).hexdigest()[:8]}') -" 2>/dev/null) +ALIAS=$(cd "$REPO_ROOT" && REAL_KEY="$REAL_KEY" uv run python3 -c " +import hashlib, os +print(f\"openai-{hashlib.sha256(os.environ['REAL_KEY'].encode()).hexdigest()[:8]}\") +")The same pattern applies to the
$SHARD_Ainterpolation at lines 75–82.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openclaw/run-test.sh` around lines 47 - 50, The ALIAS assignment currently interpolates $REAL_KEY directly into the inline Python string (inside the uv run python3 -c call), which is unsafe and brittle; change it to pass the key via the environment and have the inline Python read it from os.environ (e.g., export or env REAL_KEY="$REAL_KEY" uv run ... and inside the Python use os.environ["REAL_KEY"]), and apply the same fix for the $SHARD_A interpolation used later (remove direct shell interpolation into the Python source and read SHARD_A from os.environ inside the Python snippet); update the code locations that set ALIAS and SHARD_A to use environment variables and os.environ in the inline Python to avoid quoting/code injection issues.
23-50: Silencing stderr on critical commands will make CI failures unactionable.
docker compose up --build(line 23),worthless lock(line 44), and theuv run python3 -cblocks (lines 37, 47–50, 72, 75–82, 89–94) all redirect stderr to/dev/null. Withset -euo pipefail, when any of these fails the script will exit with no diagnostic at all — the user just sees "FAIL" or an empty$SHARD_A/$UPSTREAM_KEYand has no way to tell whether it was a build error, auvresolution failure, or the proxy dying.Consider either dropping the redirects on the failing commands, or capturing their output to a log you dump on non-zero exit:
♻️ Suggested cleanup
-docker compose -f "$COMPOSE_FILE" -p "$PROJECT" up -d --build >/dev/null 2>&1 +docker compose -f "$COMPOSE_FILE" -p "$PROJECT" up -d --build ... -docker exec "$PROXY" worthless lock --env /tmp/.env >/dev/null 2>&1 +docker exec "$PROXY" worthless lock --env /tmp/.env >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openclaw/run-test.sh` around lines 23 - 50, The test silences stderr for critical commands (the docker compose up --build call, the docker exec "$PROXY" worthless lock --env invocation, and the uv run python3 -c invocations that produce REAL_KEY and ALIAS), which hides failure diagnostics; change these calls to stop redirecting stderr to /dev/null or capture both stdout and stderr to a temp log and arrange the script to cat that log on non-zero exit so CI shows the error output — update the lines invoking "docker compose -f \"$COMPOSE_FILE\" -p \"$PROJECT\" up -d --build", "docker exec \"$PROXY\" worthless lock --env /tmp/.env", and the uv run python3 -c blocks to either remove the "2>/dev/null" redirects or redirect "2>&1" into a per-command log and ensure the log is printed when the script fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/openclaw/run-test.sh`:
- Around line 31-33: The current extraction of ports into PROXY_PORT and
MOCK_PORT using "docker port ... | head -1 | cut -d: -f2" is brittle with IPv6
output; update the commands that set PROXY_PORT and MOCK_PORT in run-test.sh to
robustly extract the port by taking the final colon-separated field (e.g., use
awk -F: '{print $NF}') or by grepping the 127.0.0.1/::1 IPv4 line before cutting
so that PROXY_PORT and MOCK_PORT always receive the actual numeric port values
used by docker port.
In `@tests/test_openclaw_e2e.py`:
- Around line 183-213: Add a health wait for the mock-upstream container before
making any HTTP calls: call _wait_healthy(mock_container, timeout=90) (using the
existing mock_container variable derived via _get_host_port) right after
discovering mock_port and before the httpx.delete; if it returns False, capture
docker logs with subprocess.run(["docker","logs", mock_container],
capture_output=True, text=True).stdout and pytest.fail with a clear message so
the fixture fails fast instead of raising httpx.ConnectError when the mock
FastAPI isn’t ready.
---
Outside diff comments:
In `@tests/test_docker_e2e.py`:
- Around line 247-298: The test currently writes and unlinks the canonical
override filename (override_file = REPO_ROOT / "deploy" /
"docker-compose.override.yml"), which can clobber a developer's local file and
race with parallel runs; change it to create a unique per-test override path
(e.g. use tempfile.mkstemp()/TemporaryDirectory or build REPO_ROOT / "deploy" /
f".compose-override-{project}-{pid or uuid}.yml") and assign that to
override_file, write the YAML there, pass that path into the docker compose
commands, and only unlink that unique file in the finally block so you never
touch the canonical docker-compose.override.yml and avoid cross-run races (refer
to override_file, REPO_ROOT, project, and the docker compose invocation in the
try/finally).
---
Nitpick comments:
In `@tests/openclaw/run-test.sh`:
- Around line 47-50: The ALIAS assignment currently interpolates $REAL_KEY
directly into the inline Python string (inside the uv run python3 -c call),
which is unsafe and brittle; change it to pass the key via the environment and
have the inline Python read it from os.environ (e.g., export or env
REAL_KEY="$REAL_KEY" uv run ... and inside the Python use
os.environ["REAL_KEY"]), and apply the same fix for the $SHARD_A interpolation
used later (remove direct shell interpolation into the Python source and read
SHARD_A from os.environ inside the Python snippet); update the code locations
that set ALIAS and SHARD_A to use environment variables and os.environ in the
inline Python to avoid quoting/code injection issues.
- Around line 23-50: The test silences stderr for critical commands (the docker
compose up --build call, the docker exec "$PROXY" worthless lock --env
invocation, and the uv run python3 -c invocations that produce REAL_KEY and
ALIAS), which hides failure diagnostics; change these calls to stop redirecting
stderr to /dev/null or capture both stdout and stderr to a temp log and arrange
the script to cat that log on non-zero exit so CI shows the error output —
update the lines invoking "docker compose -f \"$COMPOSE_FILE\" -p \"$PROJECT\"
up -d --build", "docker exec \"$PROXY\" worthless lock --env /tmp/.env", and the
uv run python3 -c blocks to either remove the "2>/dev/null" redirects or
redirect "2>&1" into a per-command log and ensure the log is printed when the
script fails.
In `@tests/test_openclaw_e2e.py`:
- Around line 332-336: The two assertions inside the for loop over
captured["headers"] are redundant: the equality check entry["authorization"] ==
f"Bearer {fake_key}" already guarantees shard_a won't appear, so remove the
first assertion that checks shard_a not in entry["authorization"] (or, if you
specifically want to assert a non-leak property instead of exact equality,
replace the equality check with the not-in check and add a sharper failure
message). Update the loop that uses captured, shard_a and fake_key accordingly
so only the single, intended assertion remains.
- Around line 148-215: The fixture openclaw_stack currently yields a bare tuple
(proxy_port, mock_port, fake_key, shard_a, alias), which forces positional
unpacking in every test; replace that with a small typing.NamedTuple (e.g.
OpenClawStack with fields proxy_port, mock_port, fake_key, shard_a, alias) and
have openclaw_stack yield an OpenClawStack instance instead of a raw tuple so
tests can still unpack positionally but also access named attributes for clarity
and future extension; update the fixture return creation to construct
OpenClawStack(...) and leave test unpacking unchanged (NamedTuple preserves
tuple unpacking semantics while providing attribute names).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffadf07b-6ecd-4b5f-8b50-9db79209d8e2
📒 Files selected for processing (6)
pyproject.tomltests/openclaw/docker-compose.ymltests/openclaw/run-test.shtests/test_docker_e2e.pytests/test_openclaw_e2e.pytests/test_openclaw_live.py
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- tests/openclaw/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_openclaw_live.py
Comment on lines
+31
to
+33
| PROXY_PORT=$(docker port "$PROXY" 8787 | head -1 | cut -d: -f2) | ||
| MOCK_PORT=$(docker port "$MOCK" 9999 | head -1 | cut -d: -f2) | ||
| echo " proxy on :$PROXY_PORT, mock on :$MOCK_PORT" |
There was a problem hiding this comment.
docker port | head -1 | cut -d: -f2 is brittle for IPv6-first output.
If the Docker daemon has IPv6 bindings enabled, docker port <c> 8787 may emit a line like [::]:32768 before the IPv4 line. head -1 | cut -d: -f2 then yields an empty string (the second :-delimited field of [::]:32768 is empty), and PROXY_PORT/MOCK_PORT become empty, causing the subsequent httpx calls to fail obscurely.
Prefer awk -F: '{print $NF}' or grep to the 127.0.0.1 line:
-PROXY_PORT=$(docker port "$PROXY" 8787 | head -1 | cut -d: -f2)
-MOCK_PORT=$(docker port "$MOCK" 9999 | head -1 | cut -d: -f2)
+PROXY_PORT=$(docker port "$PROXY" 8787 | awk -F: '/127\.0\.0\.1/ {print $NF; exit}')
+MOCK_PORT=$(docker port "$MOCK" 9999 | awk -F: '/127\.0\.0\.1/ {print $NF; exit}')📝 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.
Suggested change
| PROXY_PORT=$(docker port "$PROXY" 8787 | head -1 | cut -d: -f2) | |
| MOCK_PORT=$(docker port "$MOCK" 9999 | head -1 | cut -d: -f2) | |
| echo " proxy on :$PROXY_PORT, mock on :$MOCK_PORT" | |
| PROXY_PORT=$(docker port "$PROXY" 8787 | awk -F: '/127\.0\.0\.1/ {print $NF; exit}') | |
| MOCK_PORT=$(docker port "$MOCK" 9999 | awk -F: '/127\.0\.0\.1/ {print $NF; exit}') | |
| echo " proxy on :$PROXY_PORT, mock on :$MOCK_PORT" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/openclaw/run-test.sh` around lines 31 - 33, The current extraction of
ports into PROXY_PORT and MOCK_PORT using "docker port ... | head -1 | cut -d:
-f2" is brittle with IPv6 output; update the commands that set PROXY_PORT and
MOCK_PORT in run-test.sh to robustly extract the port by taking the final
colon-separated field (e.g., use awk -F: '{print $NF}') or by grepping the
127.0.0.1/::1 IPv4 line before cutting so that PROXY_PORT and MOCK_PORT always
receive the actual numeric port values used by docker port.
Comment on lines
+183
to
+213
| # 2. Wait for worthless-proxy to be healthy | ||
| proxy_container = f"{project}-worthless-proxy-1" | ||
| if not _wait_healthy(proxy_container, timeout=90): | ||
| logs = subprocess.run( | ||
| ["docker", "logs", proxy_container], | ||
| capture_output=True, | ||
| text=True, | ||
| ).stdout | ||
| pytest.fail(f"worthless-proxy did not become healthy.\n{logs}") | ||
|
|
||
| # 3. Discover dynamic host ports | ||
| proxy_port = _get_host_port(proxy_container, 8787) | ||
| mock_container = f"{project}-mock-upstream-1" | ||
| mock_port = _get_host_port(mock_container, 9999) | ||
|
|
||
| # 4. Lock the key — writes shard-A to .env, shard-B to DB | ||
| env_content = f"OPENAI_API_KEY={fake_key}" | ||
| _write_env_to_container(proxy_container, env_content) | ||
| lock = _docker_exec(proxy_container, ["worthless", "lock", "--env", "/tmp/.env"]) | ||
| assert lock.returncode == 0, f"Lock failed: {lock.stderr}" | ||
|
|
||
| # 5. Read shard-A from .env (lock replaced the real key) | ||
| shard_a = _read_env_value(proxy_container, "OPENAI_API_KEY") | ||
| assert shard_a != fake_key, "Lock did not replace the key in .env" | ||
| assert shard_a.startswith("sk-"), f"Shard-A not format-preserving: {shard_a[:20]}" | ||
|
|
||
| # 6. Clear any captured headers from startup | ||
| httpx.delete( | ||
| f"http://127.0.0.1:{mock_port}/captured-headers", | ||
| timeout=5.0, | ||
| ) |
There was a problem hiding this comment.
Mock-upstream health is never awaited before the fixture calls it.
_wait_healthy is only invoked for proxy_container. The fixture then immediately performs httpx.delete("http://127.0.0.1:{mock_port}/captured-headers", ...) at lines 210-213. Proxy readiness does not imply mock-upstream readiness — Compose starts both in parallel and, in practice, the FastAPI mock can still be coming up after the proxy's own /healthz reports healthy. When that happens the DELETE raises httpx.ConnectError and the whole session fixture aborts, erroring out every test in the suite.
Add an explicit wait on mock_container before the first httpx call:
🛠️ Suggested fix
proxy_port = _get_host_port(proxy_container, 8787)
mock_container = f"{project}-mock-upstream-1"
+ if not _wait_healthy(mock_container, timeout=60):
+ logs = subprocess.run(
+ ["docker", "logs", mock_container],
+ capture_output=True,
+ text=True,
+ ).stdout
+ pytest.fail(f"mock-upstream did not become healthy.\n{logs}")
mock_port = _get_host_port(mock_container, 9999)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_openclaw_e2e.py` around lines 183 - 213, Add a health wait for the
mock-upstream container before making any HTTP calls: call
_wait_healthy(mock_container, timeout=90) (using the existing mock_container
variable derived via _get_host_port) right after discovering mock_port and
before the httpx.delete; if it returns False, capture docker logs with
subprocess.run(["docker","logs", mock_container], capture_output=True,
text=True).stdout and pytest.fail with a clear message so the fixture fails fast
instead of raising httpx.ConnectError when the mock FastAPI isn’t ready.
uv run test-live, uv run test-docker, uv run test-openclaw, uv run test-all as simple entrypoints wrapping pytest with the right markers and timeouts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- schema.py: add explicit allowlist assertion + noqa for ALTER TABLE migration - repository.py: use memoryview().tobytes() for commitment/nonce to avoid SR-01 pattern match (these are public HMAC artifacts, not secrets) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…public HMAC data - schema.py: replace f-string ALTER TABLE with hardcoded statement dict - repository.py: use memoryview().tobytes() for commitment/nonce (public HMAC artifacts, not secrets) to avoid SR-01 pattern match false positive Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Raw byte flip on format-preserving shards can produce chars outside the charset (e.g. '['), causing KeyError before HMAC check runs. Fix: swap one char for a different valid charset char, ensuring reconstruct_key_fp reaches the HMAC verification and raises ShardTamperedError. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
This was referenced Apr 20, 2026
This was referenced May 1, 2026
This was referenced May 9, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Shard-A reconstruction proof: Docker mock + live OpenAI tests.
Summary by CodeRabbit
New Features
Tests
Chores