Created dockerfile + updated streaming #7
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds containerization and orchestration (Dockerfile, docker-compose), an OAuth callback proxy, docs (CLAUDE.md, DOCKER_OAUTH_SETUP.md, FIXES_SUMMARY.md), streaming and non‑stream response handling changes, environment propagation and debug logging for Claude subprocesses, and updates .gitignore to ignore Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as Chat Endpoint
participant CM as Claude Manager
participant P as Claude Process
C->>API: POST /v1/chat/completions (payload, stream flag)
API->>CM: request process (payload, env propagated)
CM->>P: spawn Claude CLI (env=os.environ.copy())
P-->>CM: stdout SSE / JSON chunks
CM-->>API: parsed messages (streamed chunks or final messages)
alt stream = true
Note right of API #D0F0C0: SSE media_type: text/event-stream
API-->>C: SSE events (data chunks until None)
else
Note right of API #F0E68C: JSONResponse application/json
API-->>C: final JSONResponse (aggregated assistant messages joined with `---`)
end
CM->>CM: log exit code, stdout, stderr
sequenceDiagram
autonumber
participant Dev as Developer
participant Proxy as OAuth Proxy
participant Cont as Container (API)
Dev->>Proxy: POST /oauth/register (session_id, callback_port)
Proxy-->>Dev: { callback_url -> http://host:port/oauth/callback?state=... }
Dev->>Browser: open callback_url (user auth)
Browser->>Proxy: GET /oauth/callback?state=...&code=...
Proxy->>Cont: POST http://container_host:callback_port/oauth/callback (forwarded)
Cont-->>Proxy: 200 OK
Proxy-->>Browser: success HTML
Proxy->>Proxy: track active_sessions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||
Summary of ChangesHello @christag, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the claude-code-api by introducing full Docker containerization, making it easier to deploy and run the application. It also refines the API's streaming capabilities to align with modern event-stream standards, improving integration with front-end applications. Additionally, the changes include better debugging tools for the Claude process and more flexible API key management through environment variables. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Docker support for the application via a Dockerfile and docker-compose.yml, which is a great enhancement for deployment and development consistency. It also includes a fix for the streaming response media type and improves debugging logs. My review focuses on improving the Docker setup for security, best practices, and maintainability. I've identified a critical issue with how the source code is added to the image, a high-severity security concern with user permissions, and several medium-severity suggestions to improve the Dockerfile, logging, and development workflow with Docker Compose.
| WORKDIR /home/claudeuser/app | ||
|
|
||
| # Clone claude-code-api | ||
| RUN git clone https://github.com/christag/claude-code-api.git . |
There was a problem hiding this comment.
The Docker image is built by cloning the repository from GitHub using git clone. This is not a good practice as it makes the build dependent on network availability and the state of the remote repository, which harms reproducibility. It also prevents building local changes. The source code should be copied from the build context using the COPY instruction. You should also consider adding a .dockerignore file to exclude unnecessary files and directories (like .git, Dockerfile, etc.) from the build context.
COPY . .
| RUN useradd -m -s /bin/bash claudeuser && \ | ||
| echo "claudeuser ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers |
There was a problem hiding this comment.
The non-root user claudeuser is granted passwordless sudo access to all commands. This is a significant security risk and violates the principle of least privilege. The container should be designed so that the application user does not require sudo at all. Based on the rest of the Dockerfile and the application, sudo does not appear to be necessary for the user at runtime.
RUN useradd -m -s /bin/bash claudeuser
| RUN curl -fsSL https://deb.nodesource.com/setup_18.x | bash - && \ | ||
| apt-get install -y nodejs && \ | ||
| node --version && npm --version |
There was a problem hiding this comment.
The apt-get install command for nodejs does not clean up the apt cache afterwards. This increases the image size unnecessarily. It's a good practice to chain apt-get install with rm -rf /var/lib/apt/lists/* in the same RUN layer to keep the layer small.
RUN curl -fsSL https://deb.nodesource.com/setup_18.x | bash - && \
apt-get install -y nodejs && \
rm -rf /var/lib/apt/lists/* && \
node --version && npm --version
| RUN echo '#!/bin/bash\n\ | ||
| set -e\n\ | ||
| \n\ | ||
| # Only configure API key if explicitly provided and not using Claude Max\n\ | ||
| if [ -n "$ANTHROPIC_API_KEY" ] && [ "$USE_CLAUDE_MAX" != "true" ]; then\n\ | ||
| echo "Configuring Claude Code with API key..."\n\ | ||
| mkdir -p ~/.config/claude\n\ | ||
| cat > ~/.config/claude/config.json << EOF\n\ | ||
| {\n\ | ||
| "apiKey": "$ANTHROPIC_API_KEY",\n\ | ||
| "autoUpdate": false\n\ | ||
| }\n\ | ||
| EOF\n\ | ||
| echo "Claude Code configured with API key"\n\ | ||
| elif [ "$USE_CLAUDE_MAX" = "true" ]; then\n\ | ||
| echo "Using Claude Max subscription - please run: docker exec -it claude-code-api claude"\n\ | ||
| echo "Then authenticate via browser when prompted"\n\ | ||
| else\n\ | ||
| echo "No authentication configured. Set ANTHROPIC_API_KEY or USE_CLAUDE_MAX=true"\n\ | ||
| fi\n\ | ||
| \n\ | ||
| # Test Claude Code\n\ | ||
| echo "Testing Claude Code..."\n\ | ||
| claude --version || echo "Claude Code installed"\n\ | ||
| \n\ | ||
| echo "Starting API server..."\n\ | ||
| cd /home/claudeuser/app\n\ | ||
| exec python3 -m claude_code_api.main' > /home/claudeuser/entrypoint.sh && \ | ||
| chmod +x /home/claudeuser/entrypoint.sh |
There was a problem hiding this comment.
The entrypoint script is created using a long echo command. This makes the script difficult to read, maintain, and debug. It's better practice to have the entrypoint script as a separate file in the repository (e.g., entrypoint.sh) and use the COPY instruction to add it to the image.
| # Only configure API key if explicitly provided and not using Claude Max\n\ | ||
| if [ -n "$ANTHROPIC_API_KEY" ] && [ "$USE_CLAUDE_MAX" != "true" ]; then\n\ | ||
| echo "Configuring Claude Code with API key..."\n\ | ||
| mkdir -p ~/.config/claude\n\ |
There was a problem hiding this comment.
The command mkdir -p ~/.config/claude inside the entrypoint script is redundant because this directory is already created at line 40 of the Dockerfile. This line can be removed from the script to avoid redundancy.
| logger.error(f"=== DEBUG: Exit code: {self.process.returncode} ===") | ||
| if stderr: | ||
| logger.error(f"=== DEBUG: Claude stderr: {stderr.decode()} ===") | ||
| if stdout: | ||
| logger.info(f"=== DEBUG: Claude stdout: {stdout.decode()[:1000]} ===") |
There was a problem hiding this comment.
The new debug log messages are using f-strings. Since the project uses structlog, it's better to pass the dynamic parts as keyword arguments to the logger methods. This allows for better processing, filtering, and querying of logs in a structured format.
| logger.error(f"=== DEBUG: Exit code: {self.process.returncode} ===") | |
| if stderr: | |
| logger.error(f"=== DEBUG: Claude stderr: {stderr.decode()} ===") | |
| if stdout: | |
| logger.info(f"=== DEBUG: Claude stdout: {stdout.decode()[:1000]} ===") | |
| logger.error("=== DEBUG: Claude process finished ===", exit_code=self.process.returncode) | |
| if stderr: | |
| logger.error("=== DEBUG: Claude stderr ===", stderr=stderr.decode()) | |
| if stdout: | |
| logger.info("=== DEBUG: Claude stdout ===", stdout=stdout.decode()[:1000]) |
| services: | ||
| claude-code-api: | ||
| build: . | ||
| container_name: claude-code-api | ||
| ports: | ||
| - "127.0.0.1:8000:8000" # Only bind to localhost since tunnel will handle external access | ||
| environment: | ||
| # Use Claude Max subscription instead of API key | ||
| - USE_CLAUDE_MAX=true | ||
| - HOST=0.0.0.0 | ||
| - PORT=8000 | ||
| # Optional: Project root for Claude Code workspace | ||
| - CLAUDE_PROJECT_ROOT=/home/claudeuser/app/workspace | ||
| volumes: | ||
| # Mount workspace for persistent projects | ||
| - ./workspace:/home/claudeuser/app/workspace | ||
| # Mount Claude config to persist authentication | ||
| - ./claude-config:/home/claudeuser/.config/claude | ||
| # Optional: Mount custom config | ||
| - ./config:/home/claudeuser/app/config | ||
| restart: unless-stopped | ||
| healthcheck: | ||
| test: ["CMD", "curl", "-f", "http://localhost:8000/health"] | ||
| interval: 30s | ||
| timeout: 10s | ||
| retries: 3 | ||
| start_period: 10s No newline at end of file |
There was a problem hiding this comment.
This docker-compose.yml is suitable for running the application, but it's not optimized for a development workflow. Any code change requires rebuilding the Docker image. To improve the development experience, consider adding a volume mount for the application source code. This allows changes to be reflected immediately without rebuilding. This is typically done in a separate docker-compose.override.yml file for development, which would be used alongside this file.
For example, you could add:
services:
claude-code-api:
volumes:
- .:/home/claudeuser/appNote that this would be most effective if the Dockerfile is updated to COPY the source code instead of git clone.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
claude_code_api/core/claude_manager.py (1)
72-73: Consider filtering sensitive environment variables.Passing
os.environ.copy()propagates all environment variables to the Claude subprocess, including potentially sensitive keys likeANTHROPIC_API_KEY. If the Claude process logs or exposes its environment, this could leak secrets.Consider filtering the environment to pass only necessary variables:
+ # Filter environment to only pass necessary variables + safe_env = { + k: v for k, v in os.environ.items() + if not k.startswith(('SECRET_', 'PASSWORD_', 'TOKEN_')) + or k in ('ANTHROPIC_API_KEY',) # Explicitly allow needed keys + } + self.process = await asyncio.create_subprocess_exec( *cmd, cwd=src_dir, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, - env=os.environ.copy() + env=safe_env )claude_code_api/api/chat.py (1)
281-281: Redundant media_type parameter.
JSONResponsealready setsmedia_type="application/json"by default. Explicitly passing it is redundant but harmless.You can simplify to:
- return JSONResponse(content=response, media_type="application/json") + return JSONResponse(content=response)Dockerfile (1)
23-25: Pin Node.js version for reproducibility.Using
setup_18.xinstalls the latest Node.js 18.x version, which can change over time and affect build reproducibility.Pin to a specific Node.js version:
-RUN curl -fsSL https://deb.nodesource.com/setup_18.x | bash - && \ +# Pin to specific Node.js version for reproducibility +RUN curl -fsSL https://deb.nodesource.com/setup_18.x | bash - && \ apt-get install -y nodejs && \ + npm install -g npm@9.8.1 && \ node --version && npm --versionAlso consider pinning the
@anthropic-ai/claude-codepackage version on line 28.docker-compose.yml (1)
7-13: Remove unused CLAUDE_PROJECT_ROOT
TheCLAUDE_PROJECT_ROOTvariable is declared indocker-compose.ymlbut not referenced anywhere in the codebase. Remove it to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)Dockerfile(1 hunks)claude_code_api/api/chat.py(2 hunks)claude_code_api/core/claude_manager.py(1 hunks)docker-compose.yml(1 hunks)
🔇 Additional comments (6)
.gitignore (1)
331-332: LGTM!Adding
fix_env.patchto.gitignoreis appropriate for preventing accidental commits of environment-specific patches or temporary fixes.claude_code_api/api/chat.py (1)
207-207: LGTM! Correct MIME type for SSE.Changing from
text/plaintotext/event-streamaligns with the Server-Sent Events (SSE) standard and matches OpenAI's streaming API format. This improves compatibility with clients like OpenWebUI.Dockerfile (2)
68-69: LGTM! Appropriate healthcheck configuration.The healthcheck configuration is well-designed with reasonable intervals, timeout, retries, and start period. This will help orchestration systems detect when the API is ready and healthy.
76-85: Verify API key configuration logic.The entrypoint script only configures the API key if
ANTHROPIC_API_KEYis set ANDUSE_CLAUDE_MAXis not true. However, if neither condition is met, the API may fail to authenticate with Claude Code at runtime.Consider adding validation to fail fast if authentication is not properly configured:
elif [ "$USE_CLAUDE_MAX" = "true" ]; then\n\ echo "Using Claude Max subscription - please run: docker exec -it claude-code-api claude"\n\ echo "Then authenticate via browser when prompted"\n\ else\n\ - echo "No authentication configured. Set ANTHROPIC_API_KEY or USE_CLAUDE_MAX=true"\n\ + echo "ERROR: No authentication configured. Set ANTHROPIC_API_KEY or USE_CLAUDE_MAX=true"\n\ + exit 1\n\ fi\n\This ensures the container fails immediately if misconfigured rather than starting and failing later.
docker-compose.yml (2)
5-6: LGTM! Security-conscious port binding.Binding to
127.0.0.1:8000instead of0.0.0.0:8000ensures the API is only accessible from localhost, which is appropriate when using an external tunnel for access. This prevents unintended exposure of the API.
22-27: LGTM! Appropriate healthcheck configuration.The healthcheck configuration matches the Dockerfile and provides proper health monitoring for container orchestration. The start period allows sufficient time for the API to initialize.
| # Log stdout and stderr for debugging | ||
| # Debug logging MUST be here, before any error checking | ||
| logger.error(f"=== DEBUG: Exit code: {self.process.returncode} ===") | ||
| if stderr: | ||
| logger.error(f"=== DEBUG: Claude stderr: {stderr.decode()} ===") | ||
| if stdout: | ||
| logger.info(f"=== DEBUG: Claude stdout: {stdout.decode()[:1000]} ===") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use appropriate log level for debug messages.
Lines 80, 82, and 84 use logger.error() for debug output. Debug messages should use logger.debug() to avoid polluting error logs and triggering unnecessary alerts.
Apply this diff:
- # Log stdout and stderr for debugging
- # Debug logging MUST be here, before any error checking
- logger.error(f"=== DEBUG: Exit code: {self.process.returncode} ===")
+ # Debug logging before error checking
+ logger.debug(f"Exit code: {self.process.returncode}")
if stderr:
- logger.error(f"=== DEBUG: Claude stderr: {stderr.decode()} ===")
+ logger.debug(f"Claude stderr: {stderr.decode()}")
if stdout:
- logger.info(f"=== DEBUG: Claude stdout: {stdout.decode()[:1000]} ===")
+ logger.debug(f"Claude stdout (first 1000 chars): {stdout.decode()[:1000]}")📝 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.
| # Log stdout and stderr for debugging | |
| # Debug logging MUST be here, before any error checking | |
| logger.error(f"=== DEBUG: Exit code: {self.process.returncode} ===") | |
| if stderr: | |
| logger.error(f"=== DEBUG: Claude stderr: {stderr.decode()} ===") | |
| if stdout: | |
| logger.info(f"=== DEBUG: Claude stdout: {stdout.decode()[:1000]} ===") | |
| # Debug logging before error checking | |
| logger.debug(f"Exit code: {self.process.returncode}") | |
| if stderr: | |
| logger.debug(f"Claude stderr: {stderr.decode()}") | |
| if stdout: | |
| logger.debug(f"Claude stdout (first 1000 chars): {stdout.decode()[:1000]}") |
🤖 Prompt for AI Agents
In claude_code_api/core/claude_manager.py around lines 78 to 84, the debug
output is currently logged at non-debug levels; change the debug-level messages
(exit code, stderr and stdout debug lines) to use logger.debug() instead of
logger.error() or logger.info() so they don't pollute error logs or trigger
alerts; keep the same message text and decoding/truncation logic, only update
the logger method to logger.debug.
| RUN useradd -m -s /bin/bash claudeuser && \ | ||
| echo "claudeuser ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers |
There was a problem hiding this comment.
Overly permissive sudo access.
Granting claudeuser passwordless sudo with ALL=(ALL) NOPASSWD:ALL is a significant security risk. If the Claude Code process or API is compromised, an attacker gains full root access to the container.
Limit sudo privileges to only what's necessary, or remove sudo access entirely if not required:
RUN useradd -m -s /bin/bash claudeuser && \
- echo "claudeuser ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers
+ # Grant limited sudo only if needed, e.g., for specific commands
+ echo "claudeuser ALL=(ALL) NOPASSWD: /usr/bin/apt-get" >> /etc/sudoersOr remove the line entirely if sudo is not required at runtime.
🤖 Prompt for AI Agents
In Dockerfile around lines 32-33, the Dockerfile grants claudeuser unrestricted
passwordless sudo (ALL=(ALL) NOPASSWD:ALL), which is overly permissive; either
remove the sudoers line entirely if sudo is not required at runtime, or replace
it with a minimal sudoers entry that only permits the specific commands the
process needs (using a dedicated sudoers file under /etc/sudoers.d and
visudo-safe syntax), and avoid NOPASSWD unless absolutely necessary (require a
password or use capability-based alternatives); update the Dockerfile to create
a limited sudoers file or drop sudo usage accordingly.
| WORKDIR /home/claudeuser/app | ||
|
|
||
| # Clone claude-code-api | ||
| RUN git clone https://github.com/christag/claude-code-api.git . |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the repository source.
The Dockerfile clones from https://github.com/christag/claude-code-api.git, which appears to be a personal fork. Ensure this is the intended source and consider:
- Using the official/canonical repository if available
- Pinning to a specific commit SHA or tag for reproducibility and security
- Verifying the clone integrity
🏁 Script executed:
#!/bin/bash
# Check if this is a fork and identify the upstream repository
gh repo view christag/claude-code-api --json parent,isFork,nameWithOwnerLength of output: 169
Target official upstream and pin revision in Dockerfile
Replace the clone URL on line 49 to the canonical codingworkflow/claude-code-api (or your intended source) and pin to a specific commit SHA or tag for reproducible, secure builds.
🤖 Prompt for AI Agents
In Dockerfile around line 49, the repository is cloned from an unofficial URL;
replace the clone command to target the official upstream (e.g.,
https://github.com/codingworkflow/claude-code-api.git) and pin to a specific
commit SHA or tag for reproducible builds: update the clone to reference the
canonical repo and a fixed revision (use --branch <tag> or clone then git
checkout <commit-sha>), or add a build ARG for the REVISION and use that to
check out the exact tag/commit; ensure the final image build uses the pinned
revision rather than the branch head.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CLAUDE.md(1 hunks)DOCKER_OAUTH_SETUP.md(1 hunks)FIXES_SUMMARY.md(1 hunks)claude_code_api/api/chat.py(3 hunks)claude_code_api/utils/streaming.py(4 hunks)docker-compose.yml(1 hunks)oauth-proxy.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🧬 Code graph analysis (2)
claude_code_api/utils/streaming.py (1)
claude_code_api/core/claude_manager.py (1)
get_output(136-163)
claude_code_api/api/chat.py (1)
claude_code_api/core/claude_manager.py (1)
get_output(136-163)
🪛 markdownlint-cli2 (0.18.1)
FIXES_SUMMARY.md
170-170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
CLAUDE.md
41-41: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
DOCKER_OAUTH_SETUP.md
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
108-108: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.13.2)
oauth-proxy.py
72-72: Local variable response_text is assigned to but never used
Remove assignment to unused variable response_text
(F841)
84-84: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
122-122: Do not catch blind exception: Exception
(BLE001)
123-123: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
126-126: Unused method argument: request
(ARG002)
271-271: Unused function argument: request
(ARG001)
359-359: Possible binding to all interfaces
(S104)
| session_id = query_params.get('state', 'unknown') | ||
| callback_port = query_params.get('callback_port') | ||
|
|
||
| if not callback_port: | ||
| # Try to determine callback port from state or other params | ||
| logger.warning("No callback_port specified, using default container port") | ||
| callback_port = self.container_port | ||
| else: | ||
| callback_port = int(callback_port) | ||
|
|
||
| # Forward to container | ||
| target_url = f"http://{self.container_host}:{callback_port}/oauth/callback" | ||
|
|
||
| logger.info(f"Forwarding OAuth callback to: {target_url}") | ||
|
|
||
| async with ClientSession() as session: | ||
| try: | ||
| # Forward the callback with all query parameters | ||
| async with session.get(target_url, params=query_params, timeout=10) as resp: | ||
| response_text = await resp.text() | ||
|
|
||
| logger.info(f"Container response: {resp.status}") | ||
|
|
||
| # Return success page to user |
There was a problem hiding this comment.
Use registered callback ports when forwarding OAuth callbacks
handle_register_callback records session→port mappings, but handle_oauth_callback ignores that data whenever the provider redirect lacks an explicit callback_port query param. In the normal OAuth flow the provider only echoes state=...; as a result every callback is forwarded to the default container port (8000), so session-specific callback listeners never receive the code, breaking authentication. Please resolve the port via active_sessions when the query omits callback_port (and keep the default as a final fallback).
- callback_port = query_params.get('callback_port')
-
- if not callback_port:
- # Try to determine callback port from state or other params
- logger.warning("No callback_port specified, using default container port")
- callback_port = self.container_port
- else:
- callback_port = int(callback_port)
+ callback_port_param = query_params.get('callback_port')
+
+ if callback_port_param is not None:
+ try:
+ callback_port = int(callback_port_param)
+ except ValueError:
+ logger.error("Invalid callback_port '%s', falling back to default", callback_port_param)
+ callback_port = self.container_port
+ else:
+ callback_port = self.active_sessions.get(session_id, self.container_port)
+ if session_id in self.active_sessions:
+ logger.info(
+ "Resolved callback port %s from registered session %s",
+ callback_port,
+ session_id,
+ )
+ else:
+ logger.warning(
+ "No callback_port provided and session %s not registered; using default %s",
+ session_id,
+ self.container_port,
+ )📝 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.
| session_id = query_params.get('state', 'unknown') | |
| callback_port = query_params.get('callback_port') | |
| if not callback_port: | |
| # Try to determine callback port from state or other params | |
| logger.warning("No callback_port specified, using default container port") | |
| callback_port = self.container_port | |
| else: | |
| callback_port = int(callback_port) | |
| # Forward to container | |
| target_url = f"http://{self.container_host}:{callback_port}/oauth/callback" | |
| logger.info(f"Forwarding OAuth callback to: {target_url}") | |
| async with ClientSession() as session: | |
| try: | |
| # Forward the callback with all query parameters | |
| async with session.get(target_url, params=query_params, timeout=10) as resp: | |
| response_text = await resp.text() | |
| logger.info(f"Container response: {resp.status}") | |
| # Return success page to user | |
| session_id = query_params.get('state', 'unknown') | |
| callback_port_param = query_params.get('callback_port') | |
| if callback_port_param is not None: | |
| try: | |
| callback_port = int(callback_port_param) | |
| except ValueError: | |
| logger.error("Invalid callback_port '%s', falling back to default", callback_port_param) | |
| callback_port = self.container_port | |
| else: | |
| callback_port = self.active_sessions.get(session_id, self.container_port) | |
| if session_id in self.active_sessions: | |
| logger.info( | |
| "Resolved callback port %s from registered session %s", | |
| callback_port, | |
| session_id, | |
| ) | |
| else: | |
| logger.warning( | |
| "No callback_port provided and session %s not registered; using default %s", | |
| session_id, | |
| self.container_port, | |
| ) | |
| # Forward to container | |
| target_url = f"http://{self.container_host}:{callback_port}/oauth/callback" | |
| logger.info(f"Forwarding OAuth callback to: {target_url}") | |
| async with ClientSession() as session: | |
| try: | |
| # Forward the callback with all query parameters | |
| async with session.get(target_url, params=query_params, timeout=10) as resp: | |
| response_text = await resp.text() | |
| logger.info(f"Container response: {resp.status}") | |
| # Return success page to user |
🧰 Tools
🪛 Ruff (0.13.2)
72-72: Local variable response_text is assigned to but never used
Remove assignment to unused variable response_text
(F841)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
CLAUDE.md (1)
1-151: Excellent comprehensive documentation for the project.This documentation provides clear architectural guidance, setup instructions, and debugging tips. It accurately reflects the codebase structure and will be valuable for developers working with or extending the project.
Optional: Address the static analysis hint.
Line 41 uses bold text for a heading. Consider using a proper markdown heading instead:
-**ClaudeProcess (claude_manager.py:18-228)** +#### ClaudeProcess (claude_manager.py:18-228)This applies to other bold "headings" at lines 28, 34, 42, and 47 as well.
oauth-proxy.py (4)
68-89: Address unused variable and improve error logging.The code has minor issues flagged by static analysis:
- Line 72:
response_textis assigned but never used- Line 84: Should use
logging.exceptionfor exception contextApply these improvements:
try: # Forward the callback with all query parameters async with session.get(target_url, params=query_params, timeout=10) as resp: - response_text = await resp.text() - logger.info(f"Container response: {resp.status}") # Return success page to user return web.Response( text=self._success_html(session_id), content_type='text/html', status=200 ) except ClientError as e: - logger.error(f"Failed to forward to container: {e}") + logger.exception(f"Failed to forward to container: {e}") return web.Response( text=self._error_html(str(e)), content_type='text/html', status=502 )
99-124: Improve error handling specificity.Lines 122-123 catch bare
Exceptionand should uselogging.exceptionfor better debugging context.Apply this improvement:
except Exception as e: - logger.error(f"Error registering callback: {e}") + logger.exception("Error registering callback") return web.json_response({'error': str(e)}, status=500)
126-134: Remove unused request parameter.The
requestparameter inhandle_healthis unused (line 126).Either use it for logging or mark it as unused:
- async def handle_health(self, request: web.Request) -> web.Response: + async def handle_health(self, _request: web.Request) -> web.Response: """Health check endpoint."""
271-316: Remove unused request parameter in handle_root.Line 271's
requestparameter is unused.Mark as unused:
- async def handle_root(request): + async def handle_root(_request): return web.Response(FIXES_SUMMARY.md (1)
1-406: Comprehensive documentation of fixes and testing procedures.This document accurately describes the streaming improvements and OAuth proxy implementation, provides clear testing procedures, and includes helpful troubleshooting guidance. It aligns well with the code changes in the PR.
Optional: Add language specifier to code block.
Line 170 has a fenced code block without a language specifier (flagged by markdownlint):
-``` +```text Starting OAuth Proxy on port 8888 Forwarding to container at localhost:8000 OAuth callback URL: http://localhost:8888/oauth/callback Press Ctrl+C to stop</blockquote></details> <details> <summary>DOCKER_OAUTH_SETUP.md (1)</summary><blockquote> `1-303`: **Excellent OAuth proxy setup documentation.** This guide provides clear, step-by-step instructions for setting up the OAuth proxy, including troubleshooting, advanced configuration, and production deployment guidance. It accurately reflects the oauth-proxy.py implementation and will help users successfully configure MCP server authentication in Docker. **Optional: Add language specifiers to code blocks.** Lines 47 and 108 have fenced code blocks without language specifiers (flagged by markdownlint): ```diff # Line 47 -``` +```text Starting OAuth Proxy on port 8888 Forwarding to container at localhost:8000 OAuth callback URL: http://localhost:8888/oauth/callback Press Ctrl+C to stopLine 108
-
+text
┌─────────────────┐
│ Your Browser │
│ (on host) │
...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CLAUDE.md(1 hunks)DOCKER_OAUTH_SETUP.md(1 hunks)FIXES_SUMMARY.md(1 hunks)claude_code_api/api/chat.py(3 hunks)claude_code_api/utils/streaming.py(4 hunks)docker-compose.yml(1 hunks)oauth-proxy.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🧬 Code graph analysis (2)
claude_code_api/api/chat.py (1)
claude_code_api/core/claude_manager.py (1)
get_output(136-163)
claude_code_api/utils/streaming.py (1)
claude_code_api/core/claude_manager.py (1)
get_output(136-163)
🪛 markdownlint-cli2 (0.18.1)
DOCKER_OAUTH_SETUP.md
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
108-108: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
FIXES_SUMMARY.md
170-170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
CLAUDE.md
41-41: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.13.2)
oauth-proxy.py
72-72: Local variable response_text is assigned to but never used
Remove assignment to unused variable response_text
(F841)
84-84: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
122-122: Do not catch blind exception: Exception
(BLE001)
123-123: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
126-126: Unused method argument: request
(ARG002)
271-271: Unused function argument: request
(ARG001)
359-359: Possible binding to all interfaces
(S104)
🔇 Additional comments (7)
claude_code_api/utils/streaming.py (2)
87-131: LGTM! Streaming improvements enable complete agentic workflows.The removal of artificial limits (
max_chunks = 5) and early termination logic allows streaming to capture all assistant responses and tool execution results. The changes correctly rely on theNoneend signal fromget_output()rather than arbitrary cutoffs.
346-405: Verify separator choice for multi-message aggregation.The aggregation logic correctly collects all assistant messages and uses
\n\n---\n\nas a separator when multiple messages exist. This is a reasonable choice for readability.Consider whether the separator pattern might conflict with actual content. For example, if assistant messages naturally contain
---markdown separators, this could create ambiguity. You may want to:
- Use a more distinctive separator (e.g.,
\n\n=== Response {n} ===\n\n)- Document the separator choice for API consumers
Run this to check if existing content uses similar patterns:
claude_code_api/api/chat.py (3)
207-207: Correct media type for Server-Sent Events.The change from
"text/plain"to"text/event-stream"is the proper media type for SSE streaming responses, ensuring compatibility with SSE clients and the OpenAI SDK.
219-235: LGTM! Message collection now captures complete workflows.Removing the artificial safety limit and early termination allows the endpoint to collect all messages until
get_output()naturally completes. This aligns with the streaming improvements and ensures complete agentic responses.
276-276: Explicit JSONResponse improves consistency.Returning
JSONResponse(content=response, media_type="application/json")makes the response type explicit and ensures the correct Content-Type header, improving consistency with the streaming path's explicit media type.oauth-proxy.py (2)
358-359: Binding to all interfaces requires careful consideration.Line 359 binds to
0.0.0.0, exposing the OAuth proxy to all network interfaces. This is flagged by static analysis (S104) as a potential security risk.For a localhost-only OAuth proxy, binding to
0.0.0.0is unnecessary and increases attack surface. Consider:
- If localhost-only is intended: Change to
127.0.0.1- If remote access is needed: Document the security implications and recommend firewall rules
Current code:
web.run_app(app, host='0.0.0.0', port=args.port)Safer default for local development:
web.run_app(app, host='127.0.0.1', port=args.port)Or make it configurable:
parser.add_argument('--host', default='127.0.0.1', help='Host to bind to') # ... web.run_app(app, host=args.host, port=args.port)Based on the PR description ("localhost redirect does not work cleanly"), binding to
127.0.0.1seems more appropriate.
38-62: Verify callback_port extraction and default behavior.The logic defaults to
self.container_portwhencallback_portis missing from query params. However, line 54 extractscallback_portfrom query params, which may not be the intended source for dynamic port forwarding.The PR description mentions MCP servers redirect to
localhost:[random-port], but this implementation:
- Extracts
callback_portfrom query params (line 54)- Falls back to the fixed
container_port(line 59)This means the MCP server must include
callback_portin its OAuth redirect URL. Verify this matches the actual OAuth flow:
1 participant
User description
I created a Dockerfile and compose file, as well as updated the type to text/event-stream for use with OpenWebUI.
You can set an ANTHROPIC_API_KEY in environement OR leave the max sub setting on and then youll just have to exec into the container to authenticate once.
One issue right now is being able to authenticate MCP servers from within claude code since localhost redirect doesnt work nicely.
PR Type
Enhancement
Description
Added Docker containerization with Dockerfile and compose
Fixed streaming response media type to text/event-stream
Enhanced Claude process debugging and error logging
Added environment variable support for API configuration
Diagram Walkthrough
File Walkthrough
Dockerfile
Complete Docker containerization setupDockerfile
claude_manager.py
Enhanced Claude process debugging and environment supportclaude_code_api/core/claude_manager.py
docker-compose.yml
Docker Compose service configurationdocker-compose.yml
chat.py
Fix streaming and JSON response media typesclaude_code_api/api/chat.py
text/event-stream
application/json
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores