Persist local agent chat hotfix in agent-api template#809
Open
rgranadosd wants to merge 3 commits into
Open
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughHelm templates and docs updated: agent-api CORS origin changed and its container startup replaced with an inline Python hotfix that relaxes Pydantic Question fields and adjusts chat route validation/error handling; a workflow template gains a structured OpenAPI v3 parameters schema; README adds a note about external repository import testing. ChangesAgent-API Configuration & Startup Patch
Workflow Template Parameters Schema
Documentation Note
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
README.md (1)
9-15: ⚡ Quick winConsider relocating internal testing notes out of the public README.
This section documents a transient testing artifact (a specific commit of an external third-party repo) and is not relevant to end-users or contributors browsing the README. Placing it before "## Overview" makes it the first substantive content readers see. Consider moving it to
CONTRIBUTING.md, a developer wiki page, or a comment in the relevant CI/test config instead.Additionally, the referenced external repo (
kooljo/poc-agent-sdk) is a third-party resource that could be deleted or made private at any time with no notice, making this reference stale.🤖 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 `@README.md` around lines 9 - 15, Move the transient testing note under the "## Note About External Repository Import Testing" header out of the public README (so it no longer appears before "## Overview"); remove the specific third‑party repo/commit reference from README.md and instead add it to a non-public developer document such as CONTRIBUTING.md, a developer wiki page, or as a comment in the CI/test configuration that runs import/deployment checks; update README.md to contain only stable, user-facing content and, if needed, add a brief generic sentence linking to the developer guide for testing details.
🤖 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
`@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml`:
- Around line 162-177: The startup hotfix script calls p.read_text() on p =
Path('/workspace/main.py') without checking existence, which causes a
FileNotFoundError for agents that don't include that file; update the script
around the p = Path('/workspace/main.py') / p.read_text() usage to first check
p.exists() (or wrap read_text() in a try/except FileNotFoundError) and skip the
patch/early-return (and print a short message) when the file is absent so the
component doesn't crash on startup.
- Line 179: The container command hardcodes the GCP Buildpack interpreter path
"/layers/google.python.runtime/python/bin/python3" which will fail for non-GCP
workflows listed in allowedWorkflows (e.g., amp-docker,
amp-ballerina-buildpack); replace that hardcoded path with a PATH-resolved
fallback so non-GCP containers simply skip/noop: change the command that runs
"/tmp/amp_chat_hotfix.py" to invoke python via a PATH resolution (e.g., prefer
`python3` if available, fall back to `python` or skip) or wrap invocation in a
shell that checks for a python interpreter before executing the script, ensuring
exec /cnb/process/web still runs when no suitable interpreter exists.
- Around line 168-177: The script unconditionally calls p.write_text(text) and
print("startup hotfix applied") even when no replacement occurred; modify the
flow to detect whether any replacement happened (e.g., track a boolean changed
flag or compare original_text vs text) after attempting both if old in text and
if old2 in text replacements, and only call p.write_text(text) and
print("startup hotfix applied") when changed is true; otherwise skip
writing/printing (or emit a distinct message) so p.write_text, print and the
variables old, old2, new, new2, text reflect actual changes.
---
Nitpick comments:
In `@README.md`:
- Around line 9-15: Move the transient testing note under the "## Note About
External Repository Import Testing" header out of the public README (so it no
longer appears before "## Overview"); remove the specific third‑party
repo/commit reference from README.md and instead add it to a non-public
developer document such as CONTRIBUTING.md, a developer wiki page, or as a
comment in the CI/test configuration that runs import/deployment checks; update
README.md to contain only stable, user-facing content and, if needed, add a
brief generic sentence linking to the developer guide for testing details.
🪄 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: 58ae2b03-b785-47ad-bfee-ca5bf2097c29
📒 Files selected for processing (3)
README.mddeployments/helm-charts/wso2-amp-evaluation-extension/templates/workflow-templates/workflow-monitor-evaluation.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml
Comment on lines
+162
to
+177
| from pathlib import Path | ||
|
|
||
| p = Path('/workspace/main.py') | ||
| text = p.read_text() | ||
| old = """# Pydantic model for handling chat input\nclass Question(BaseModel):\n prompt: str\n""" | ||
| new = """# Pydantic model for handling chat input\nclass Question(BaseModel):\n prompt: str | None = None\n message: str | None = None\n session_id: str | None = None\n context: dict | None = None\n""" | ||
| if old in text: | ||
| text = text.replace(old, new, 1) | ||
|
|
||
| old2 = """# Route to chat with OpenAI\n@app.post(\"/chat/\")\ndef ask_openai(question: Question):\n try:\n response = client.chat.completions.create(\n model=\"gpt-4o\",\n messages=[{\"role\": \"user\", \"content\": question.prompt}]\n )\n result = response.choices[0].message.content\n\n # Store chat history in database\n db = SessionLocal()\n db.add(QueryHistory(question=question.prompt, response=result))\n db.commit()\n\n return {\"response\": result}\n except Exception as e:\n raise HTTPException(status_code=500, detail=str(e))\n""" | ||
| new2 = """# Route to chat with OpenAI\n@app.post(\"/chat/\")\ndef ask_openai(question: Question):\n try:\n user_prompt = question.message or question.prompt\n if not user_prompt:\n raise HTTPException(status_code=422, detail=\"message or prompt is required\")\n\n response = client.chat.completions.create(\n model=\"gpt-4o\",\n messages=[{\"role\": \"user\", \"content\": user_prompt}]\n )\n result = response.choices[0].message.content\n\n # Store chat history in database\n db = SessionLocal()\n db.add(QueryHistory(question=user_prompt, response=result))\n db.commit()\n\n return {\"response\": result}\n except HTTPException:\n raise\n except Exception as e:\n raise HTTPException(status_code=500, detail=str(e))\n""" | ||
| if old2 in text: | ||
| text = text.replace(old2, new2, 1) | ||
|
|
||
| p.write_text(text) | ||
| print("startup hotfix applied") |
Contributor
There was a problem hiding this comment.
Critical: unguarded read_text() crashes startup for any agent-api without /workspace/main.py.
p.read_text() raises FileNotFoundError if the target file doesn't exist. Since this ClusterComponentType is shared across all agent-api workloads — including those built with amp-docker or amp-ballerina-buildpack (see allowedWorkflows, lines 12–17) — any agent that doesn't happen to have /workspace/main.py will CrashLoopBackOff because exec /cnb/process/web never runs.
🛡️ Proposed fix — add an existence guard at the top of the patch script
- p = Path('/workspace/main.py')
- text = p.read_text()
+ p = Path('/workspace/main.py')
+ if not p.exists():
+ print("startup hotfix: /workspace/main.py not found, skipping")
+ exit(0)
+ text = p.read_text()🤖 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
`@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml`
around lines 162 - 177, The startup hotfix script calls p.read_text() on p =
Path('/workspace/main.py') without checking existence, which causes a
FileNotFoundError for agents that don't include that file; update the script
around the p = Path('/workspace/main.py') / p.read_text() usage to first check
p.exists() (or wrap read_text() in a try/except FileNotFoundError) and skip the
patch/early-return (and print a short message) when the file is absent so the
component doesn't crash on startup.
Comment on lines
+168
to
+177
| if old in text: | ||
| text = text.replace(old, new, 1) | ||
|
|
||
| old2 = """# Route to chat with OpenAI\n@app.post(\"/chat/\")\ndef ask_openai(question: Question):\n try:\n response = client.chat.completions.create(\n model=\"gpt-4o\",\n messages=[{\"role\": \"user\", \"content\": question.prompt}]\n )\n result = response.choices[0].message.content\n\n # Store chat history in database\n db = SessionLocal()\n db.add(QueryHistory(question=question.prompt, response=result))\n db.commit()\n\n return {\"response\": result}\n except Exception as e:\n raise HTTPException(status_code=500, detail=str(e))\n""" | ||
| new2 = """# Route to chat with OpenAI\n@app.post(\"/chat/\")\ndef ask_openai(question: Question):\n try:\n user_prompt = question.message or question.prompt\n if not user_prompt:\n raise HTTPException(status_code=422, detail=\"message or prompt is required\")\n\n response = client.chat.completions.create(\n model=\"gpt-4o\",\n messages=[{\"role\": \"user\", \"content\": user_prompt}]\n )\n result = response.choices[0].message.content\n\n # Store chat history in database\n db = SessionLocal()\n db.add(QueryHistory(question=user_prompt, response=result))\n db.commit()\n\n return {\"response\": result}\n except HTTPException:\n raise\n except Exception as e:\n raise HTTPException(status_code=500, detail=str(e))\n""" | ||
| if old2 in text: | ||
| text = text.replace(old2, new2, 1) | ||
|
|
||
| p.write_text(text) | ||
| print("startup hotfix applied") |
Contributor
There was a problem hiding this comment.
Major: print("startup hotfix applied") is unconditional — masks silent no-ops.
p.write_text(text) and print("startup hotfix applied") execute regardless of whether either if old in text: / if old2 in text: branch matched. If the source code changes and neither pattern matches, the script silently does nothing but still reports success, making it invisible in logs that the hotfix was never applied.
🛡️ Proposed fix — conditional reporting
+ applied = False
if old in text:
text = text.replace(old, new, 1)
+ applied = True
...
if old2 in text:
text = text.replace(old2, new2, 1)
+ applied = True
p.write_text(text)
- print("startup hotfix applied")
+ if applied:
+ print("startup hotfix applied")
+ else:
+ print("startup hotfix: no patterns matched — source may have changed")📝 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
| if old in text: | |
| text = text.replace(old, new, 1) | |
| old2 = """# Route to chat with OpenAI\n@app.post(\"/chat/\")\ndef ask_openai(question: Question):\n try:\n response = client.chat.completions.create(\n model=\"gpt-4o\",\n messages=[{\"role\": \"user\", \"content\": question.prompt}]\n )\n result = response.choices[0].message.content\n\n # Store chat history in database\n db = SessionLocal()\n db.add(QueryHistory(question=question.prompt, response=result))\n db.commit()\n\n return {\"response\": result}\n except Exception as e:\n raise HTTPException(status_code=500, detail=str(e))\n""" | |
| new2 = """# Route to chat with OpenAI\n@app.post(\"/chat/\")\ndef ask_openai(question: Question):\n try:\n user_prompt = question.message or question.prompt\n if not user_prompt:\n raise HTTPException(status_code=422, detail=\"message or prompt is required\")\n\n response = client.chat.completions.create(\n model=\"gpt-4o\",\n messages=[{\"role\": \"user\", \"content\": user_prompt}]\n )\n result = response.choices[0].message.content\n\n # Store chat history in database\n db = SessionLocal()\n db.add(QueryHistory(question=user_prompt, response=result))\n db.commit()\n\n return {\"response\": result}\n except HTTPException:\n raise\n except Exception as e:\n raise HTTPException(status_code=500, detail=str(e))\n""" | |
| if old2 in text: | |
| text = text.replace(old2, new2, 1) | |
| p.write_text(text) | |
| print("startup hotfix applied") | |
| applied = False | |
| if old in text: | |
| text = text.replace(old, new, 1) | |
| applied = True | |
| old2 = """# Route to chat with OpenAI\n@app.post(\"/chat/\")\ndef ask_openai(question: Question):\n try:\n response = client.chat.completions.create(\n model=\"gpt-4o\",\n messages=[{\"role\": \"user\", \"content\": question.prompt}]\n )\n result = response.choices[0].message.content\n\n # Store chat history in database\n db = SessionLocal()\n db.add(QueryHistory(question=question.prompt, response=result))\n db.commit()\n\n return {\"response\": result}\n except Exception as e:\n raise HTTPException(status_code=500, detail=str(e))\n""" | |
| new2 = """# Route to chat with OpenAI\n@app.post(\"/chat/\")\ndef ask_openai(question: Question):\n try:\n user_prompt = question.message or question.prompt\n if not user_prompt:\n raise HTTPException(status_code=422, detail=\"message or prompt is required\")\n\n response = client.chat.completions.create(\n model=\"gpt-4o\",\n messages=[{\"role\": \"user\", \"content\": user_prompt}]\n )\n result = response.choices[0].message.content\n\n # Store chat history in database\n db = SessionLocal()\n db.add(QueryHistory(question=user_prompt, response=result))\n db.commit()\n\n return {\"response\": result}\n except HTTPException:\n raise\n except Exception as e:\n raise HTTPException(status_code=500, detail=str(e))\n""" | |
| if old2 in text: | |
| text = text.replace(old2, new2, 1) | |
| applied = True | |
| p.write_text(text) | |
| if applied: | |
| print("startup hotfix applied") | |
| else: | |
| print("startup hotfix: no patterns matched — source may have changed") |
🤖 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
`@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml`
around lines 168 - 177, The script unconditionally calls p.write_text(text) and
print("startup hotfix applied") even when no replacement occurred; modify the
flow to detect whether any replacement happened (e.g., track a boolean changed
flag or compare original_text vs text) after attempting both if old in text and
if old2 in text replacements, and only call p.write_text(text) and
print("startup hotfix applied") when changed is true; otherwise skip
writing/printing (or emit a distinct message) so p.write_text, print and the
variables old, old2, new, new2, text reflect actual changes.
| p.write_text(text) | ||
| print("startup hotfix applied") | ||
| PY | ||
| /layers/google.python.runtime/python/bin/python3 /tmp/amp_chat_hotfix.py |
Contributor
There was a problem hiding this comment.
Major: hardcoded GCP Buildpack interpreter path breaks non-GCP agent-api deployments.
/layers/google.python.runtime/python/bin/python3 only exists inside containers built with the Google Cloud Python Buildpack. The allowedWorkflows list on this same ClusterComponentType includes amp-docker and amp-ballerina-buildpack, neither of which will have this interpreter at that path. The python3 invocation will fail before any existence check in the script can run, again preventing exec /cnb/process/web from executing.
Consider falling back to a PATH-resolved interpreter so the script is a no-op for non-matching containers:
🛡️ Proposed fix — use PATH-resolved python3 with fallback
- /layers/google.python.runtime/python/bin/python3 /tmp/amp_chat_hotfix.py
+ PYTHON3=$(command -v python3 2>/dev/null || echo /layers/google.python.runtime/python/bin/python3)
+ "$PYTHON3" /tmp/amp_chat_hotfix.pyNote that this alone is still insufficient if /workspace/main.py is absent (see the preceding comment).
📝 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
| /layers/google.python.runtime/python/bin/python3 /tmp/amp_chat_hotfix.py | |
| PYTHON3=$(command -v python3 2>/dev/null || echo /layers/google.python.runtime/python/bin/python3) | |
| "$PYTHON3" /tmp/amp_chat_hotfix.py |
🤖 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
`@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml`
at line 179, The container command hardcodes the GCP Buildpack interpreter path
"/layers/google.python.runtime/python/bin/python3" which will fail for non-GCP
workflows listed in allowedWorkflows (e.g., amp-docker,
amp-ballerina-buildpack); replace that hardcoded path with a PATH-resolved
fallback so non-GCP containers simply skip/noop: change the command that runs
"/tmp/amp_chat_hotfix.py" to invoke python via a PATH resolution (e.g., prefer
`python3` if available, fall back to `python` or skip) or wrap invocation in a
shell that checks for a python interpreter before executing the script, ensuring
exec /cnb/process/web still runs when no suitable interpreter exists.
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
2 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.
Summary
Summary by CodeRabbit
Bug Fixes
Documentation
Chores