Add UNAVAILABLE_WITHOUT_CONTAINER_NAME feature flag - #1335
Conversation
Add UNAVAILABLE_WITHOUT_CONTAINER_NAME env var that, when truthy, causes the server to reject every HTTP and WebSocket request with a configurable status code (UNAVAILABLE_WITHOUT_CONTAINER_NAME_RESPONSE_STATUS_CODE, default 503) whenever CONTAINER_NAME is not set -- replacing the silent pass-through to local development mode. Implemented as a single ASGI middleware so the gate applies uniformly across /ws, /cmd, /pty/*, /responses, /playwright_exec, and /status, instead of duplicating an inline check at each auth site. Backwards-compatible: with neither env var set, behavior is unchanged. Integration tests cover the helper, HTTP endpoints, PTY endpoints, the WebSocket handshake, custom status codes, invalid status codes, multiple truthy value variants, and CONTAINER_NAME taking precedence over the flag. https://claude.ai/code/session_01VUPZ5JsZUpYKVKxgaSwYXt
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📦 Publishable packages changed
Add |
📝 WalkthroughWalkthroughAdded ASGI middleware that gates incoming requests based on environment configuration, returning an unavailable status code when Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as UnavailableWithoutContainer<br/>Middleware
participant App as Application Handler
alt Container Name Missing & Flag Enabled
Client->>Middleware: HTTP Request
Middleware->>Middleware: Check CONTAINER_NAME & flag
Middleware->>Client: 503/Custom Status + JSON Error
Client->>Middleware: WebSocket Upgrade
Middleware->>Middleware: Check CONTAINER_NAME & flag
Middleware->>Client: Accept Connection
Middleware->>Client: Send Error JSON
Middleware->>Client: Close (1008)
else Normal Operation
Client->>Middleware: Request
Middleware->>App: Pass Through
App->>Client: Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/python/computer-server/computer_server/main.py (1)
129-139: Middleware also gates/statusand any health/readiness endpoints.The middleware uniformly rejects every HTTP request (including
/status) when the flag is enabled. If this server runs behind an orchestrator (K8s liveness/readiness, load balancer health checks, uptime monitors), those probes will start failing with 503 as soon asUNAVAILABLE_WITHOUT_CONTAINER_NAME=1is set withoutCONTAINER_NAME, which could trigger pod restarts or remove the instance from rotation — even though "unconfigured" is exactly the state you want the probe to surface differently from "dead".Consider exempting a small allowlist (e.g.
/status, or an explicit/healthz) from the gate so health endpoints can still report configuration state, or document this behavior prominently so operators configure their probes accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/computer-server/computer_server/main.py` around lines 129 - 139, The middleware in __call__ uses _unavailable_status_code() to reject all "http" and "websocket" requests (via _reject_http/_reject_websocket), which currently blocks health endpoints like /status; update __call__ to exempt a small allowlist of health paths (e.g., "/status" or "/healthz") by checking scope.get("path") before applying _unavailable_status_code() and, if the path is in the allowlist, call await self.app(scope, receive, send) instead of rejecting; ensure both http and websocket flows consult the same allowlist and keep references to _unavailable_status_code, _reject_http and _reject_websocket to preserve existing behavior for other paths.libs/python/computer-server/tests/test_auth_availability.py (1)
137-145:test_backwards_compat_local_dev_accepts_authis brittle — depends on browser_manager behavior.This test issues
{"command": "noop", ...}and only assertsresp.status_code not in (401, 503). If a future refactor causesbrowser_manager.execute_commandto raise and the handler returns 500, the test still passes (vacuously confirming "not gated"); but if the handler ever starts returning 401/503 for unknown commands (e.g., a config-related 503 from the browser subsystem), the test fails for reasons unrelated to the middleware under test.Consider asserting on a more specific outcome — e.g.
resp.status_code in (200, 400, 500)— or (better) point this test at a lightweight endpoint like/commandsthat has no external dependency, to isolate the assertion to "middleware did not intercept".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/computer-server/tests/test_auth_availability.py` around lines 137 - 145, The test test_backwards_compat_local_dev_accepts_auth is brittle because it hits /playwright_exec which depends on browser_manager.execute_command; change it to assert a specific allowed set or call a lightweight handler without external deps. Either replace client.post("/playwright_exec", ...) with a request to a simple endpoint (e.g. "/commands" or an equivalent no-external-deps route) to ensure only middleware is exercised, or tighten the assertion to assert resp.status_code in (200, 400, 500) so future browser_manager failures (500) don’t cause false negatives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/python/computer-server/computer_server/main.py`:
- Around line 129-139: The middleware in __call__ uses
_unavailable_status_code() to reject all "http" and "websocket" requests (via
_reject_http/_reject_websocket), which currently blocks health endpoints like
/status; update __call__ to exempt a small allowlist of health paths (e.g.,
"/status" or "/healthz") by checking scope.get("path") before applying
_unavailable_status_code() and, if the path is in the allowlist, call await
self.app(scope, receive, send) instead of rejecting; ensure both http and
websocket flows consult the same allowlist and keep references to
_unavailable_status_code, _reject_http and _reject_websocket to preserve
existing behavior for other paths.
In `@libs/python/computer-server/tests/test_auth_availability.py`:
- Around line 137-145: The test test_backwards_compat_local_dev_accepts_auth is
brittle because it hits /playwright_exec which depends on
browser_manager.execute_command; change it to assert a specific allowed set or
call a lightweight handler without external deps. Either replace
client.post("/playwright_exec", ...) with a request to a simple endpoint (e.g.
"/commands" or an equivalent no-external-deps route) to ensure only middleware
is exercised, or tighten the assertion to assert resp.status_code in (200, 400,
500) so future browser_manager failures (500) don’t cause false negatives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c7373be-8f9a-484f-815e-5989fef0e3a2
📒 Files selected for processing (2)
libs/python/computer-server/computer_server/main.pylibs/python/computer-server/tests/test_auth_availability.py
r33drichards
added
the
release:pypi/computer-server
📦 Publishable packages changed
|
3 participants
Summary
Adds a new feature flag
UNAVAILABLE_WITHOUT_CONTAINER_NAMEthat allows the server to reject all requests with a configurable HTTP status code whenCONTAINER_NAMEis required but not set, while maintaining backwards compatibility with local development mode.Key Changes
_parse_bool_env()and_unavailable_status_code()to parse environment variables and determine when the server should be unavailableUnavailableWithoutContainerMiddlewarethat intercepts all HTTP and WebSocket requests and rejects them when the flag is enabled andCONTAINER_NAMEis unsetUNAVAILABLE_WITHOUT_CONTAINER_NAME: Feature flag (accepts various truthy values: "1", "true", "yes", "y", "on")UNAVAILABLE_WITHOUT_CONTAINER_NAME_RESPONSE_STATUS_CODE: Configurable HTTP status code for rejections (defaults to 503)/cmd,/status)Implementation Details
CONTAINER_NAMEis set, the unavailable flag is ignored, allowing normal auth flow to proceedhttps://claude.ai/code/session_01VUPZ5JsZUpYKVKxgaSwYXt
Summary by CodeRabbit
New Features
Tests