Add go support - #2
Conversation
|
Files changed in last commit (d3ff091): Review ReportSummaryThe provided code changes for the Critical Issues
|
|
Files changed in last commit (426af68): Summary: The provided CI/CD workflow file appears to be well-structured and covers a range of tasks for linting, testing, and security checks. However, there are several areas that can be improved for better maintainability, readability, and security. Critical Issues:
Suggestions:
- name: Cache pip dependencies
uses: cache@v3
with:
path: ~/.cache/pip
key: ${{ runner.os }}-pip-${{ hashFiles('.github/workflows/lint-and-test.yml') }}
restore-keys: |
${{ runner.os }}-pip-
- name: Run pip-audit
run: |
if [ -n "$PIP_AUDIT Enabled" ]; then
pip-audit
else
echo "pip-audit is disabled"
fi
jobs:
lint-and-test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Cache pip dependencies
uses: cache@v3
with:
path: ~/.cache/pip
key: ${{ runner.os }}-pip-${{ hashFiles('.github/workflows/lint-and-test.yml') }}
restore-keys: |
${{ runner.os }}-pip-
- name: Install tools
run: pip install ruff black isort bandit safety pip-audit
- name: Run Linters
run: |
ruff check .
black --check .
isort --check-only .
bandit -r .
- name: Run Tests
run: |
# add test commands herePositive Notes:
Architecture: The workflow file follows a modular structure, with each step performing a specific task. However, consider reorganizing the workflow to group related tasks together. Maintainability: The workflow file is relatively easy to maintain, but there are a few areas where improvements could be made. For example, consider adding more comments or documentation to explain the purpose and behavior of each step. Readability: The workflow file is well-formatted and easy to read. However, consider using more descriptive variable names and comments to explain the purpose and behavior of each step. Testing: The workflow file includes some testing tasks, but there are a few areas where improvements could be made. For example, consider adding more test cases or using a testing framework like |
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request adds a Go HTTP server, a natural-language MySQL query assistant, Docker service wiring, CI checks, AI-assisted pull request reviews, and repository templates. ChangesDatabase assistant
Repository automation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant ChatUI
participant DatabaseService
participant LLM
participant MySQL
User->>ChatUI: Submit natural-language query
ChatUI->>DatabaseService: POST /chat/database
DatabaseService->>LLM: Generate SQL with database prompt
LLM-->>DatabaseService: Return SQL response
DatabaseService->>MySQL: Validate and execute read-only SQL
MySQL-->>DatabaseService: Return rows and metadata
DatabaseService-->>ChatUI: Return SQL and formatted results
ChatUI-->>User: Render SQL and result table
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
… generation prompt, and enhanced model execution logging.
|
Files changed in last commit (4b7483b): Summary: Critical Issues:
Suggestions:
Positive Notes:
Additional Recommendations:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (13)
.github/ISSUE_TEMPLATE/feature_request.md (1)
1-9: LGTM!The feature request template provides a good starting point with clear sections for describing requests.
Optional: Consider enhancing the template structure
You might consider adding sections like:
- Priority/Impact (High/Medium/Low)
- Acceptance Criteria (what defines "done")
- Alternatives Considered
- Additional Context
This would provide more structure for feature discussions, but the current template is perfectly functional.
.github/workflows/pr-review.yml (1)
23-28: Remove redundant Docker installation.GitHub Actions
ubuntu-latestrunners already have Docker pre-installed. This step is unnecessary and adds execution time.🔎 Proposed fix
- - name: Install Docker - run: | - curl -fsSL https://get.docker.com -o get-docker.sh - sh get-docker.sh - sudo usermod -aG docker $USER - docker --version -review_diff.sh (2)
21-21: Add error handling for file read operations.The
cat "$FILE"command can fail if the file is deleted between detection and reading, or if it's a binary file that causes issues. Add error handling to skip problematic files gracefully.🔎 Suggested enhancement
for FILE in $FILES; do if [ -f "$FILE" ]; then - FILE_CONTENT=$(cat "$FILE") + if ! FILE_CONTENT=$(cat "$FILE" 2>/dev/null); then + echo "WARNING: Could not read $FILE, skipping..." + continue + fi AGGREGATED_CODE+="\n\n### File: $FILE\n\`\`\`\n$FILE_CONTENT\n\`\`\`" fi done
56-60: Remove commented-out code.The commented implementation (lines 56-60) should be removed to improve maintainability. Rely on version control history if you need to reference the old approach.
🔎 Proposed cleanup
- -# # Run the model -# RESPONSE=$(docker model run "$MODEL" "$PROMPT" 2>/dev/null) - -# # Write to review file -# echo -e "$RESPONSE" >> "$REPORT_FILE" - -services/database_service.py (1)
106-126: Hardcoded API endpoint and temperature parameter.Line 109 hardcodes the MODEL_RUNNER_API URL, duplicating the constant from
llama_runner.py. Line 125 sets temperature to 0.1 without explanation.Consider:
- Import MODEL_RUNNER_API from a shared config module
- Make temperature configurable or document why 0.1 is chosen
- Consider making the system prompt (lines 116-119) reference the one in
config/prompts.pyto maintain consistencydocker-compose.yml (1)
3-23: Consider adding health checks and restart policy.The service lacks health checks and restart policy, which are important for production reliability.
🔎 Suggested additions
services: chatbot: # ... existing config ... restart: unless-stopped healthcheck: test: ["CMD", "curl", "-f", "http://localhost:12345/health", "http://localhost:12346/health"] interval: 30s timeout: 10s retries: 3 start_period: 40sNote: This requires implementing
/healthendpoints in both the Python and Go servers.templates/chat.html (1)
116-119: Duplicate function definition.
looksLikeDbJsonis defined twice (lines 116-119 and 356-358). The second definition shadows the first. While both are identical, this is dead code that should be removed.🔎 Proposed fix
Remove the duplicate definition at lines 356-358:
- function looksLikeDbJson(obj) { - return obj && (obj.generated_sql || obj.columns || obj.rows || typeof obj.success === "boolean"); - } - // ---------- Submit ----------Also applies to: 356-358
Dockerfile (3)
6-8: Redundant pip install steps.Dependencies are installed twice: first in the
python-apistage (line 7 fromrequirements.txt) and again in the final stage (line 25 with hardcoded packages). This creates potential version inconsistencies and increases build time.🔎 Proposed fix
Keep only the
requirements.txtinstall and ensure all dependencies are listed there:# Final combined container FROM python:3.12-slim WORKDIR /app COPY --from=python-api /app /app COPY --from=go-builder /go-server /app/go-server -# Install Python dependencies -RUN pip install --no-cache-dir flask requests python-dotenv pymysql +# Copy installed packages from python-api stage +COPY --from=python-api /usr/local/lib/python3.12/site-packages /usr/local/lib/python3.12/site-packages +COPY --from=python-api /usr/local/bin /usr/local/bin RUN apt update && apt install -y curlAlternatively, ensure
requirements.txtcontains all needed packages and copy them from the build stage.Also applies to: 24-25
31-31: Fragile process management with background operator.Running
python app.py &in background means if the Flask app crashes, the container continues running with only the Go server. Partial failures won't be detected by container orchestration.For production, consider:
- Separate containers for each service (recommended for microservices)
- Process supervisor like
supervisordortiniif co-location is required- Health checks to detect partial failures
Current approach is acceptable for development but may cause debugging headaches.
27-27: Add apt cache cleanup to reduce image size.🔎 Proposed fix
-RUN apt update && apt install -y curl +RUN apt-get update && apt-get install -y --no-install-recommends curl \ + && rm -rf /var/lib/apt/lists/*routes/chat_routes.py (1)
45-46: Consider using logging instead of print statements.Debug
loggingmodule provides better control over log levels and output formatting in production.🔎 Proposed fix
+import logging + +logger = logging.getLogger(__name__) + @chat_bp.route("/chat/<mode>", methods=["POST"]) def dynamic_chat_handler(mode): - print(f"DEBUG: Handler called with mode: {mode}", flush=True) + logger.debug(f"Handler called with mode: {mode}") # ... - print("DEBUG: Calling run_database_chat", flush=True) + logger.debug("Calling run_database_chat") # ... - print(f"DEBUG: Calling run_chat with mode: {mode}", flush=True) + logger.debug(f"Calling run_chat with mode: {mode}")Also applies to: 61-61, 64-64
main.go (2)
108-112: Unchecked error fromjson.Marshal.While marshaling a
map[string]interface{}is unlikely to fail, silently ignoring the error violates Go best practices and could mask issues with message content.🔎 Proposed fix
bodyMap := map[string]interface{}{ "model": payload.Model, "messages": messages, } - bodyBytes, _ := json.Marshal(bodyMap) + bodyBytes, err := json.Marshal(bodyMap) + if err != nil { + http.Error(w, "Failed to encode request", 500) + return + }
129-130: Unchecked error fromio.Copy.The response streaming via
io.Copysilently ignores errors, which could result in partial responses being sent without any indication of failure.🔎 Proposed fix
w.Header().Set("Content-Type", "application/json") - io.Copy(w, resp.Body) + if _, err := io.Copy(w, resp.Body); err != nil { + log.Printf("Error streaming response: %v", err) + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.env.example.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.md.github/pull_request_template.md.github/workflows/ci.yml.github/workflows/pr-review.ymlDATABASE_ASSISTANT.mdDockerfileapp.pyconfig/prompts.pydocker-compose.ymlgo.modmain.gomodel_review.mdold/old_app.pyrequirements.txtreview_diff.shroutes/chat_routes.pyservices/database_service.pyservices/llama_runner.pytemplates/chat.html
🧰 Additional context used
🧬 Code graph analysis (1)
services/llama_runner.py (1)
old/old_app.py (1)
run_chat(33-58)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
17-17: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/pr-review.yml
15-15: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
24-24: shellcheck reported issue in this script: SC2086:info:3:25: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 GitHub Actions: Lint and Test
services/database_service.py
[error] 87-87: F541: f-string without any placeholders. Remove extraneous 'f' prefix.
[error] 92-92: F541: f-string without any placeholders. Remove extraneous 'f' prefix.
[error] 214-214: F541: f-string without any placeholders. Remove extraneous 'f' prefix.
🪛 LanguageTool
DATABASE_ASSISTANT.md
[uncategorized] ~99-~99: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...nst MySQL 4. Results are formatted as a markdown table 5. Response includes both the SQL...
(MARKDOWN_NNP)
🪛 markdownlint-cli2 (0.18.1)
DATABASE_ASSISTANT.md
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (23)
.github/ISSUE_TEMPLATE/config.yml (1)
1-5: LGTM!The issue template configuration is well-structured. Disabling blank issues and directing questions to Discussions helps maintain organized issue tracking.
.github/pull_request_template.md (1)
1-41: LGTM!The PR template is comprehensive and well-structured. It covers all essential aspects including clear descriptions, test documentation, and thorough checklists for both developers and reviewers. The inclusion of security considerations (no hardcoded secrets) and the emphasis on test coverage are particularly valuable.
.github/workflows/pr-review.yml (3)
50-59: API-based comment posting approach is sound.The use of
jq -Rsto properly escape the review content as JSON and post via the GitHub API is a robust approach. The authentication viaGITHUB_TOKENand endpoint construction are correct.
37-37: The model reference format is correct for Docker model pull.The format
ai/llama3.2:latestfollows the standard Docker model pull syntax (Docker Hub style:ai/model:tag). However, verify that the modelai/llama3.2exists in the Docker model registry and is publicly accessible, as the pull will fail if the model cannot be found or if authentication is missing.Likely an incorrect or invalid review comment.
30-34: I don't have a review comment to rewrite. Please provide the review comment within<review_comment>tags so I can proceed with the verification and rewriting task.review_diff.sh (3)
5-16: Git operations correctly handle empty changesets.The logic to detect the latest commit, compute the diff range, filter relevant file extensions, and exit early when no files match is well-structured.
82-90: Output handling and cleanup are well-implemented.The use of a temporary file with proper cleanup, success logging with byte count, and appending to the report file demonstrate good practice.
70-80: This review comment is incorrect. Thedocker modelcommand is not an undocumented or mysterious tool—it is a legitimate Docker extension (docker/model) that is properly documented and configured in this project.The project explicitly installs and uses this extension:
- Workflow (pr-review.yml) installs it via
sudo apt-get install docker-model-plugin- README documents installation:
docker extension install docker/model:latest- README documents usage:
docker model pullanddocker model install-runnerThe command syntax in review_diff.sh (lines 74-75) is correct:
docker model run "$MODEL" "$PROMPT". No issues exist with this code.Likely an incorrect or invalid review comment.
services/llama_runner.py (1)
6-20: LGTM!The formatting improvements (trailing commas, blank line) follow Python best practices and improve code maintainability.
config/prompts.py (2)
2-103: LGTM! Quote style consistency improved.The standardization to double-quoted keys improves consistency across the dictionary.
104-151: New prompts are well-structured.Both new prompts ("phpunit_worker_testgen" and "database") provide clear instructions and constraints. The database prompt appropriately emphasizes SQL-only output and includes safety measures (LIMIT clauses).
.env.example (1)
2-2: LGTM!Adding the HOST configuration variable is appropriate for deployment flexibility.
go.mod (1)
5-8: Dependencies are secure and current.Both versions are the latest stable releases and are free from known vulnerabilities. In particular, rs/cors v1.11.1 includes the fix for CVE-2025-47908 (GO-2024-2883), a DoS vulnerability that affected versions <1.11.0.
app.py (2)
1-11: LGTM!Clean Flask app setup with proper environment loading. The blueprint registration and early
load_dotenv()call follow best practices.
13-18: LGTM!Environment-driven configuration is appropriate for containerized deployment. The port separation (Flask: 12346, Go: 12345) correctly supports the multi-service architecture defined in the Dockerfile.
templates/chat.html (3)
19-56: LGTM!The CSS additions for database table styling are well-structured with sticky headers, proper border handling, and horizontal scroll support for wide result sets.
256-354: LGTM!The
renderDbCardfunction properly sanitizes user content with DOMPurify and handles both array and object row formats gracefully. Null value handling is appropriate.
360-446: LGTM!The form submission logic correctly handles database mode with JSON format enforcement and provides multiple fallback paths for different response shapes. Error handling is appropriate.
routes/chat_routes.py (2)
36-42: LGTM!The
ALIASESconstant provides a user-friendly way to accept various mode inputs while normalizing to canonical values.
43-65: LGTM!The dynamic chat handler cleanly separates database mode handling from other chat modes, with proper input validation and normalization.
DATABASE_ASSISTANT.md (1)
146-151: Good security awareness, but consider stronger warnings.The security notes appropriately warn about the default passwordless root configuration. Consider adding a more prominent warning or using admonition syntax to highlight the production risk.
main.go (2)
17-35: LGTM!The type definitions are clean and properly tagged for JSON serialization.
46-62: LGTM!The main function properly initializes the server with CORS support and environment-driven configuration. Route registration is clean and follows a consistent pattern.
| name: Lint and Test | ||
|
|
||
| on: [push] | ||
|
|
||
| jobs: | ||
| lint-and-test: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.11' | ||
|
|
||
| - name: Cache pip dependencies | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: ~/.cache/pip | ||
| key: ${{ runner.os }}-pip-${{ hashFiles('.github/workflows/lint-and-test.yml') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-pip- | ||
|
|
||
| - name: Install tools | ||
| run: pip install ruff black isort bandit safety pip-audit | ||
|
|
||
| - name: Run Ruff | ||
| run: ruff check . | ||
|
|
||
| - name: Run Black | ||
| run: black --check . | ||
|
|
||
| - name: Run isort | ||
| run: isort --check-only . | ||
|
|
||
| - name: Run Bandit | ||
| run: bandit -r . | ||
|
|
||
| - name: Run Safety | ||
| run: safety check | ||
|
|
||
| - name: Run pip-audit | ||
| run: pip-audit |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add Go tooling checks and actual tests.
The workflow only lints Python code, but this PR introduces Go code. Consider adding:
- Go setup and linting (golangci-lint)
- Go tests (
go test) - Actual Python tests (currently only linting tools run, no
pytestor similar)
🔎 Suggested additions
Add after the Python setup steps:
- uses: actions/setup-go@v5
with:
go-version: '1.22'
- name: Run Go lint
run: |
go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest
golangci-lint run
- name: Run Go tests
run: go test ./...
- name: Run Python tests
run: pytest🧰 Tools
🪛 actionlint (1.7.9)
17-17: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 1-43 the workflow only lints Python but
the PR adds Go code and no Python tests are run; update the CI to install and
set up Go (actions/setup-go@v5 with go-version 1.22), run golangci-lint (install
via go install and run golangci-lint run), run Go unit tests (go test ./...),
and add a Python test step (pytest) after the existing linting steps; ensure
steps run on ubuntu-latest and any caching keys remain valid.
| ``` | ||
| Query Executed Successfully | ||
|
|
||
| Generated SQL: | ||
| USE test1; SELECT * FROM users LIMIT 100; | ||
|
|
||
| Result: 15 row(s) returned | ||
|
|
||
| | id | username | email | | ||
| |---|---|---| | ||
| | 1 | john | john@example.com | | ||
| ... | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block starting at line 71 is missing a language identifier, which affects syntax highlighting and accessibility.
🔎 Proposed fix
-```
+```text
Query Executed Successfully
Generated SQL:📝 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.
| ``` | |
| Query Executed Successfully | |
| Generated SQL: | |
| USE test1; SELECT * FROM users LIMIT 100; | |
| Result: 15 row(s) returned | |
| | id | username | email | | |
| |---|---|---| | |
| | 1 | john | john@example.com | | |
| ... | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In DATABASE_ASSISTANT.md around lines 71 to 83, the fenced code block opening
lacks a language specifier; change the opening fence from ``` to ```text (or
```sql if you prefer SQL highlighting) so the block reads ```text and retains
the same content and closing fence, ensuring proper syntax highlighting and
improved accessibility.
| 1. User enters natural language query | ||
| 2. LLM converts query to SQL using specialized prompt | ||
| 3. Generated SQL is cleaned and executed against MySQL | ||
| 4. Results are formatted as a markdown table |
There was a problem hiding this comment.
Minor: Capitalize "Markdown" as a proper noun.
Per static analysis, "markdown" should be capitalized as "Markdown" when referring to the formatting language.
🔎 Proposed fix
-4. Results are formatted as a markdown table
+4. Results are formatted as a Markdown table📝 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.
| 4. Results are formatted as a markdown table | |
| 4. Results are formatted as a Markdown table |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~99-~99: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...nst MySQL 4. Results are formatted as a markdown table 5. Response includes both the SQL...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
In DATABASE_ASSISTANT.md at line 99, the word "markdown" should be capitalized
as the proper noun "Markdown"; update the line to read "Results are formatted as
a Markdown table" (only change the casing).
| # Aggregate code for prompt | ||
| AGGREGATED_CODE="" | ||
| for FILE in $FILES; do | ||
| if [ -f "$FILE" ]; then | ||
| FILE_CONTENT=$(cat "$FILE") | ||
| AGGREGATED_CODE+="\n\n### File: $FILE\n\`\`\`\n$FILE_CONTENT\n\`\`\`" | ||
| fi | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add size limit check to prevent context window overflow.
The script aggregates all changed file contents without checking total size. Large changesets could exceed the LLM's context window (typically 4K-128K tokens depending on the model), causing truncation or failure.
🔎 Suggested enhancement
Add a size check and warning after aggregation:
# After the aggregation loop, add:
AGGREGATED_SIZE=$(printf '%s' "$AGGREGATED_CODE" | wc -c)
MAX_SIZE=$((1024 * 1024)) # 1MB limit (adjust based on your model's context window)
if [ "$AGGREGATED_SIZE" -gt "$MAX_SIZE" ]; then
echo "WARNING: Aggregated code size ($AGGREGATED_SIZE bytes) exceeds limit ($MAX_SIZE bytes)."
echo "Review may be truncated or fail. Consider reviewing files in batches."
fi🤖 Prompt for AI Agents
In review_diff.sh around lines 17 to 24 the script concatenates file contents
into AGGREGATED_CODE without any size check which can overflow an LLM context;
add a post-aggregation size check that computes the byte length of
AGGREGATED_CODE, compare it against a configurable MAX_SIZE (e.g., via env var
defaulting to ~1MB or a model-appropriate limit), and if it exceeds the limit
emit a clear warning message and either exit with non-zero status or
truncate/split the aggregated content and advise the user to review files in
batches; ensure the warning includes both the aggregated size and the configured
limit so maintainers can adjust MAX_SIZE.
| for FILE in $FILES; do | ||
| if [ -f "$FILE" ]; then | ||
| FILE_CONTENT=$(cat "$FILE") | ||
| AGGREGATED_CODE+="\n\n### File: $FILE\n\`\`\`\n$FILE_CONTENT\n\`\`\`" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Critical: Unquoted variables cause word splitting on filenames with spaces.
Line 19 uses for FILE in $FILES without quotes, which will break on filenames containing spaces or special characters. Line 22 also has unquoted variable expansion. This is a serious correctness issue that will cause the script to fail or process filenames incorrectly.
🔎 Proposed fix
# Aggregate code for prompt
AGGREGATED_CODE=""
-for FILE in $FILES; do
+while IFS= read -r FILE; do
if [ -f "$FILE" ]; then
FILE_CONTENT=$(cat "$FILE")
- AGGREGATED_CODE+="\n\n### File: $FILE\n\`\`\`\n$FILE_CONTENT\n\`\`\`"
+ AGGREGATED_CODE+="\n\n### File: ${FILE}\n\`\`\`\n${FILE_CONTENT}\n\`\`\`"
fi
-done
+done <<< "$FILES"Alternatively, use a safer array-based approach:
# Aggregate code for prompt
AGGREGATED_CODE=""
+mapfile -t FILE_ARRAY <<< "$FILES"
-for FILE in $FILES; do
+for FILE in "${FILE_ARRAY[@]}"; do
if [ -f "$FILE" ]; then
FILE_CONTENT=$(cat "$FILE")
- AGGREGATED_CODE+="\n\n### File: $FILE\n\`\`\`\n$FILE_CONTENT\n\`\`\`"
+ AGGREGATED_CODE+="\n\n### File: ${FILE}\n\`\`\`\n${FILE_CONTENT}\n\`\`\`"
fi
done📝 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.
| for FILE in $FILES; do | |
| if [ -f "$FILE" ]; then | |
| FILE_CONTENT=$(cat "$FILE") | |
| AGGREGATED_CODE+="\n\n### File: $FILE\n\`\`\`\n$FILE_CONTENT\n\`\`\`" | |
| fi | |
| done | |
| while IFS= read -r FILE; do | |
| if [ -f "$FILE" ]; then | |
| FILE_CONTENT=$(cat "$FILE") | |
| AGGREGATED_CODE+="\n\n### File: ${FILE}\n\`\`\`\n${FILE_CONTENT}\n\`\`\`" | |
| fi | |
| done <<< "$FILES" |
🤖 Prompt for AI Agents
In review_diff.sh around lines 19 to 24, the for-loop and variable expansions
are unquoted causing word-splitting on filenames with spaces; fix by iterating
safely (either convert FILES into a proper array and loop: for FILE in
"${FILES[@]}" or use a while-read loop reading filenames null-delimited), and
ensure all variable expansions are quoted (e.g., "$FILE" and use printf or read
-r) so filenames with spaces or special chars are handled correctly.
| def run_database_chat(prompt, model, response_format): | ||
| """Main function to process natural language database queries.""" | ||
| print(f"DEBUG: Starting database chat with prompt: {prompt}") | ||
| MODEL_RUNNER_API = "http://host.docker.internal:12434" | ||
|
|
||
| # First, use LLM to convert natural language to SQL | ||
| # Use a very strict prompt to get only SQL | ||
| payload = { | ||
| "model": model, | ||
| "messages": [ | ||
| { | ||
| "role": "system", | ||
| "content": "Generate SQL queries ONLY. Output format: USE database_name; SQL_QUERY;\n\nDatabases: onlinepayment1, onlinepayment2, onlinepayment3\n\nDo NOT output explanations, descriptions, or example data.\n\nOutput ONLY this format:\nUSE onlinepayment1; SELECT * FROM table LIMIT 100;" | ||
| }, | ||
| { | ||
| "role": "user", | ||
| "content": "Convert this to SQL (output only the SQL query, no explanations): " + prompt | ||
| } | ||
| ], | ||
| "temperature": 0.1 # Lower temperature for more deterministic output | ||
| } | ||
|
|
||
| try: | ||
| # Get SQL from LLM | ||
| response = requests.post( | ||
| f"{MODEL_RUNNER_API}/engines/llama.cpp/v1/chat/completions", | ||
| json=payload, | ||
| timeout=60 | ||
| ) | ||
| response.raise_for_status() | ||
| llm_response = response.json() | ||
|
|
||
| # Extract SQL from LLM response | ||
| raw_sql = llm_response["choices"][0]["message"]["content"].strip() | ||
|
|
||
| # Debug: Print what we got from LLM | ||
| import sys | ||
| print(f"DEBUG: Raw LLM response: {raw_sql}", file=sys.stderr) | ||
| sys.stderr.flush() | ||
|
|
||
| # Extract SQL - look for SELECT statements first | ||
| sql = None | ||
|
|
||
| # Method 1: Look for SELECT statement with database.table format | ||
| match = re.search(r'SELECT\s+.*\s+FROM\s+[\w\.`]+\s+WHERE\s+.*;', raw_sql, re.IGNORECASE | re.DOTALL) | ||
| if match: | ||
| sql = match.group(0) | ||
| print(f"DEBUG: Found SQL via method 1: {sql}", file=sys.stderr) | ||
| sys.stderr.flush() | ||
|
|
||
| # Method 2: Look for full SELECT statement (with multi-line support) | ||
| if not sql: | ||
| # Match SELECT ... FROM ... (rest of query) until semicolon | ||
| match = re.search(r'SELECT\s+[^;]+;', raw_sql, re.IGNORECASE | re.DOTALL) | ||
| if match: | ||
| sql = match.group(0) | ||
| print(f"DEBUG: Found SQL via method 2: {sql}", file=sys.stderr) | ||
| sys.stderr.flush() | ||
|
|
||
| # Method 3: If still no SQL, try to extract from code blocks | ||
| if not sql: | ||
| # Look for content between ```sql and ``` | ||
| code_match = re.search(r'```sql\s*(.*?)\s*```', raw_sql, re.IGNORECASE | re.DOTALL) | ||
| if code_match: | ||
| sql = code_match.group(1).strip() | ||
| print(f"DEBUG: Found SQL via method 3: {sql}") | ||
|
|
||
| # Method 4: Last resort - find any line that looks like SQL | ||
| if not sql: | ||
| lines = raw_sql.split('\n') | ||
| for line in lines: | ||
| line = line.strip() | ||
| if re.match(r'SELECT\s+.*?\s+FROM\s+', line, re.IGNORECASE): | ||
| sql = line | ||
| if not sql.endswith(';'): | ||
| sql += ';' | ||
| print(f"DEBUG: Found SQL via method 4: {sql}") | ||
| break | ||
|
|
||
| # Clean up the SQL | ||
| if sql: | ||
| sql = sql.strip() | ||
| # Check if USE statement is present in raw_sql and prepend it if not already in sql | ||
| use_match = re.search(r'USE\s+\w+;', raw_sql, re.IGNORECASE) | ||
| if use_match and 'USE' not in sql.upper(): | ||
| use_statement = use_match.group(0) | ||
| sql = f"{use_statement} {sql}" | ||
| elif 'onlinepayment1' in raw_sql.lower() and 'USE' not in sql.upper(): | ||
| sql = f"USE onlinepayment1; {sql}" | ||
| elif 'onlinepayment2' in raw_sql.lower() and 'USE' not in sql.upper(): | ||
| sql = f"USE onlinepayment2; {sql}" | ||
| elif 'onlinepayment3' in raw_sql.lower() and 'USE' not in sql.upper(): | ||
| sql = f"USE onlinepayment3; {sql}" | ||
|
|
||
| print(f"DEBUG: Final SQL to execute: {sql}", file=sys.stderr) | ||
| sys.stderr.flush() | ||
|
|
||
| # Check if we found SQL | ||
| if not sql: | ||
| return jsonify({ | ||
| 'choices': [{ | ||
| 'message': { | ||
| 'content': f'**Error:** Could not extract SQL from LLM response.\n\n**Raw response:**\n```\n{raw_sql}\n```\n\nPlease try rephrasing your query.' | ||
| } | ||
| }] | ||
| }), 200 | ||
|
|
||
| # Execute the SQL query | ||
| print(f"DEBUG: About to call execute_query", file=sys.stderr) | ||
| sys.stderr.flush() | ||
| query_result = execute_query(sql) | ||
| print(f"DEBUG: Query result: {query_result}", file=sys.stderr) | ||
| sys.stderr.flush() | ||
| print(f"DEBUG: query_result keys: {query_result.keys() if isinstance(query_result, dict) else 'not a dict'}", file=sys.stderr) | ||
| sys.stderr.flush() | ||
|
|
||
| # Format the response | ||
| if 'error' in query_result: | ||
| error_message = f"**Database Query Error**\n\n**Generated SQL:**\n```sql\n{query_result.get('query', '')}\n```\n\n**Error:** {query_result['error']}" | ||
| return jsonify({ | ||
| 'choices': [{ | ||
| 'message': { | ||
| 'content': error_message | ||
| } | ||
| }] | ||
| }), 200 | ||
|
|
||
| # Format successful response | ||
| response_text = f"**Query Executed Successfully**\n\n**Generated SQL:**\n```sql\n{query_result.get('query', '')}\n```\n\n" | ||
|
|
||
| if 'columns' in query_result and query_result['columns']: | ||
| response_text += f"**Result:**\n{query_result.get('row_count', 0)} row(s) returned\n\n" | ||
|
|
||
| # Format as table | ||
| if query_result.get('rows'): | ||
| response_text += "| " + " | ".join(query_result['columns']) + " |\n" | ||
| response_text += "| " + " | ".join(["---"] * len(query_result['columns'])) + " |\n" | ||
| for row in query_result['rows'][:50]: # Limit to 50 rows for display | ||
| values = [str(row.get(col, '')) for col in query_result['columns']] | ||
| response_text += "| " + " | ".join(values) + " |\n" | ||
| if len(query_result['rows']) > 50: | ||
| response_text += f"\n*Showing first 50 of {len(query_result['rows'])} rows*" | ||
| elif 'affected_rows' in query_result: | ||
| response_text += f"**Result:** {query_result.get('message', 'Query executed')} - {query_result.get('affected_rows', 0)} row(s) affected" | ||
|
|
||
| return jsonify({ | ||
| 'choices': [{ | ||
| 'message': { | ||
| 'content': response_text | ||
| } | ||
| }] | ||
| }), 200 | ||
|
|
||
| except requests.exceptions.RequestException as e: | ||
| return jsonify({ | ||
| 'choices': [{ | ||
| 'message': { | ||
| 'content': f'**Error:** Failed to communicate with LLM: {str(e)}' | ||
| } | ||
| }] | ||
| }), 200 | ||
| except Exception as e: | ||
| return jsonify({ | ||
| 'choices': [{ | ||
| 'message': { | ||
| 'content': f'**Error:** Database query failed: {str(e)}\n\nMake sure the MySQL container (rds_payment) is running and the chatbot is connected to the aws_molpay network.' | ||
| } | ||
| }] | ||
| }), 200 |
There was a problem hiding this comment.
Multiple issues in database chat function.
Issues identified:
- Pipeline failure (Line 214): f-string without placeholder
- Brittle SQL extraction: Lines 146-199 use multiple regex patterns to extract SQL - this is fragile and may fail with valid queries
- Error handling: Returns 200 status for errors (lines 211, 231, 257, 266, 274) - should use appropriate HTTP error codes
- Debug logging: Lines 108, 141-201, 214-220 use print statements instead of proper logging
🔎 Fixes
Fix pipeline failure:
- print(f"DEBUG: About to call execute_query", file=sys.stderr)
+ print("DEBUG: About to call execute_query", file=sys.stderr)Improve error responses:
- }), 200
+ }), 400 # Bad Request for extraction failureUse proper logging:
import logging
logger = logging.getLogger(__name__)
# Replace print statements with:
logger.debug(f"Starting database chat with prompt: {prompt}")📝 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.
| def run_database_chat(prompt, model, response_format): | |
| """Main function to process natural language database queries.""" | |
| print(f"DEBUG: Starting database chat with prompt: {prompt}") | |
| MODEL_RUNNER_API = "http://host.docker.internal:12434" | |
| # First, use LLM to convert natural language to SQL | |
| # Use a very strict prompt to get only SQL | |
| payload = { | |
| "model": model, | |
| "messages": [ | |
| { | |
| "role": "system", | |
| "content": "Generate SQL queries ONLY. Output format: USE database_name; SQL_QUERY;\n\nDatabases: onlinepayment1, onlinepayment2, onlinepayment3\n\nDo NOT output explanations, descriptions, or example data.\n\nOutput ONLY this format:\nUSE onlinepayment1; SELECT * FROM table LIMIT 100;" | |
| }, | |
| { | |
| "role": "user", | |
| "content": "Convert this to SQL (output only the SQL query, no explanations): " + prompt | |
| } | |
| ], | |
| "temperature": 0.1 # Lower temperature for more deterministic output | |
| } | |
| try: | |
| # Get SQL from LLM | |
| response = requests.post( | |
| f"{MODEL_RUNNER_API}/engines/llama.cpp/v1/chat/completions", | |
| json=payload, | |
| timeout=60 | |
| ) | |
| response.raise_for_status() | |
| llm_response = response.json() | |
| # Extract SQL from LLM response | |
| raw_sql = llm_response["choices"][0]["message"]["content"].strip() | |
| # Debug: Print what we got from LLM | |
| import sys | |
| print(f"DEBUG: Raw LLM response: {raw_sql}", file=sys.stderr) | |
| sys.stderr.flush() | |
| # Extract SQL - look for SELECT statements first | |
| sql = None | |
| # Method 1: Look for SELECT statement with database.table format | |
| match = re.search(r'SELECT\s+.*\s+FROM\s+[\w\.`]+\s+WHERE\s+.*;', raw_sql, re.IGNORECASE | re.DOTALL) | |
| if match: | |
| sql = match.group(0) | |
| print(f"DEBUG: Found SQL via method 1: {sql}", file=sys.stderr) | |
| sys.stderr.flush() | |
| # Method 2: Look for full SELECT statement (with multi-line support) | |
| if not sql: | |
| # Match SELECT ... FROM ... (rest of query) until semicolon | |
| match = re.search(r'SELECT\s+[^;]+;', raw_sql, re.IGNORECASE | re.DOTALL) | |
| if match: | |
| sql = match.group(0) | |
| print(f"DEBUG: Found SQL via method 2: {sql}", file=sys.stderr) | |
| sys.stderr.flush() | |
| # Method 3: If still no SQL, try to extract from code blocks | |
| if not sql: | |
| # Look for content between ```sql and ``` | |
| code_match = re.search(r'```sql\s*(.*?)\s*```', raw_sql, re.IGNORECASE | re.DOTALL) | |
| if code_match: | |
| sql = code_match.group(1).strip() | |
| print(f"DEBUG: Found SQL via method 3: {sql}") | |
| # Method 4: Last resort - find any line that looks like SQL | |
| if not sql: | |
| lines = raw_sql.split('\n') | |
| for line in lines: | |
| line = line.strip() | |
| if re.match(r'SELECT\s+.*?\s+FROM\s+', line, re.IGNORECASE): | |
| sql = line | |
| if not sql.endswith(';'): | |
| sql += ';' | |
| print(f"DEBUG: Found SQL via method 4: {sql}") | |
| break | |
| # Clean up the SQL | |
| if sql: | |
| sql = sql.strip() | |
| # Check if USE statement is present in raw_sql and prepend it if not already in sql | |
| use_match = re.search(r'USE\s+\w+;', raw_sql, re.IGNORECASE) | |
| if use_match and 'USE' not in sql.upper(): | |
| use_statement = use_match.group(0) | |
| sql = f"{use_statement} {sql}" | |
| elif 'onlinepayment1' in raw_sql.lower() and 'USE' not in sql.upper(): | |
| sql = f"USE onlinepayment1; {sql}" | |
| elif 'onlinepayment2' in raw_sql.lower() and 'USE' not in sql.upper(): | |
| sql = f"USE onlinepayment2; {sql}" | |
| elif 'onlinepayment3' in raw_sql.lower() and 'USE' not in sql.upper(): | |
| sql = f"USE onlinepayment3; {sql}" | |
| print(f"DEBUG: Final SQL to execute: {sql}", file=sys.stderr) | |
| sys.stderr.flush() | |
| # Check if we found SQL | |
| if not sql: | |
| return jsonify({ | |
| 'choices': [{ | |
| 'message': { | |
| 'content': f'**Error:** Could not extract SQL from LLM response.\n\n**Raw response:**\n```\n{raw_sql}\n```\n\nPlease try rephrasing your query.' | |
| } | |
| }] | |
| }), 200 | |
| # Execute the SQL query | |
| print(f"DEBUG: About to call execute_query", file=sys.stderr) | |
| sys.stderr.flush() | |
| query_result = execute_query(sql) | |
| print(f"DEBUG: Query result: {query_result}", file=sys.stderr) | |
| sys.stderr.flush() | |
| print(f"DEBUG: query_result keys: {query_result.keys() if isinstance(query_result, dict) else 'not a dict'}", file=sys.stderr) | |
| sys.stderr.flush() | |
| # Format the response | |
| if 'error' in query_result: | |
| error_message = f"**Database Query Error**\n\n**Generated SQL:**\n```sql\n{query_result.get('query', '')}\n```\n\n**Error:** {query_result['error']}" | |
| return jsonify({ | |
| 'choices': [{ | |
| 'message': { | |
| 'content': error_message | |
| } | |
| }] | |
| }), 200 | |
| # Format successful response | |
| response_text = f"**Query Executed Successfully**\n\n**Generated SQL:**\n```sql\n{query_result.get('query', '')}\n```\n\n" | |
| if 'columns' in query_result and query_result['columns']: | |
| response_text += f"**Result:**\n{query_result.get('row_count', 0)} row(s) returned\n\n" | |
| # Format as table | |
| if query_result.get('rows'): | |
| response_text += "| " + " | ".join(query_result['columns']) + " |\n" | |
| response_text += "| " + " | ".join(["---"] * len(query_result['columns'])) + " |\n" | |
| for row in query_result['rows'][:50]: # Limit to 50 rows for display | |
| values = [str(row.get(col, '')) for col in query_result['columns']] | |
| response_text += "| " + " | ".join(values) + " |\n" | |
| if len(query_result['rows']) > 50: | |
| response_text += f"\n*Showing first 50 of {len(query_result['rows'])} rows*" | |
| elif 'affected_rows' in query_result: | |
| response_text += f"**Result:** {query_result.get('message', 'Query executed')} - {query_result.get('affected_rows', 0)} row(s) affected" | |
| return jsonify({ | |
| 'choices': [{ | |
| 'message': { | |
| 'content': response_text | |
| } | |
| }] | |
| }), 200 | |
| except requests.exceptions.RequestException as e: | |
| return jsonify({ | |
| 'choices': [{ | |
| 'message': { | |
| 'content': f'**Error:** Failed to communicate with LLM: {str(e)}' | |
| } | |
| }] | |
| }), 200 | |
| except Exception as e: | |
| return jsonify({ | |
| 'choices': [{ | |
| 'message': { | |
| 'content': f'**Error:** Database query failed: {str(e)}\n\nMake sure the MySQL container (rds_payment) is running and the chatbot is connected to the aws_molpay network.' | |
| } | |
| }] | |
| }), 200 | |
| def run_database_chat(prompt, model, response_format): | |
| """Main function to process natural language database queries.""" | |
| print(f"DEBUG: Starting database chat with prompt: {prompt}") | |
| MODEL_RUNNER_API = "http://host.docker.internal:12434" | |
| # First, use LLM to convert natural language to SQL | |
| # Use a very strict prompt to get only SQL | |
| payload = { | |
| "model": model, | |
| "messages": [ | |
| { | |
| "role": "system", | |
| "content": "Generate SQL queries ONLY. Output format: USE database_name; SQL_QUERY;\n\nDatabases: onlinepayment1, onlinepayment2, onlinepayment3\n\nDo NOT output explanations, descriptions, or example data.\n\nOutput ONLY this format:\nUSE onlinepayment1; SELECT * FROM table LIMIT 100;" | |
| }, | |
| { | |
| "role": "user", | |
| "content": "Convert this to SQL (output only the SQL query, no explanations): " + prompt | |
| } | |
| ], | |
| "temperature": 0.1 # Lower temperature for more deterministic output | |
| } | |
| try: | |
| # Get SQL from LLM | |
| response = requests.post( | |
| f"{MODEL_RUNNER_API}/engines/llama.cpp/v1/chat/completions", | |
| json=payload, | |
| timeout=60 | |
| ) | |
| response.raise_for_status() | |
| llm_response = response.json() | |
| # Extract SQL from LLM response | |
| raw_sql = llm_response["choices"][0]["message"]["content"].strip() | |
| # Debug: Print what we got from LLM | |
| import sys | |
| print(f"DEBUG: Raw LLM response: {raw_sql}", file=sys.stderr) | |
| sys.stderr.flush() | |
| # Extract SQL - look for SELECT statements first | |
| sql = None | |
| # Method 1: Look for SELECT statement with database.table format | |
| match = re.search(r'SELECT\s+.*\s+FROM\s+[\w\.`]+\s+WHERE\s+.*;', raw_sql, re.IGNORECASE | re.DOTALL) | |
| if match: | |
| sql = match.group(0) | |
| print(f"DEBUG: Found SQL via method 1: {sql}", file=sys.stderr) | |
| sys.stderr.flush() | |
| # Method 2: Look for full SELECT statement (with multi-line support) | |
| if not sql: | |
| # Match SELECT ... FROM ... (rest of query) until semicolon | |
| match = re.search(r'SELECT\s+[^;]+;', raw_sql, re.IGNORECASE | re.DOTALL) | |
| if match: | |
| sql = match.group(0) | |
| print(f"DEBUG: Found SQL via method 2: {sql}", file=sys.stderr) | |
| sys.stderr.flush() | |
| # Method 3: If still no SQL, try to extract from code blocks | |
| if not sql: | |
| # Look for content between |
🧰 Tools
🪛 GitHub Actions: Lint and Test
[error] 214-214: F541: f-string without any placeholders. Remove extraneous 'f' prefix.
🤖 Prompt for AI Agents
In services/database_service.py around lines 106–274, the run_database_chat
function has multiple problems: a stray f-string without placeholders around
line ~214 causing a pipeline failure, brittle regex-based SQL extraction (lines
~146–199), improper error handling returning HTTP 200 on failures (lines ~211,
~231, ~257, ~266, ~274), and scattered print debug statements (lines ~108,
~141–201, ~214–220). Fix by (1) removing or correcting the empty f-string so no
runtime FormatError occurs; (2) replace the fragile multi-regex extraction with
a deterministic parser: first strip code fences, prefer explicit USE detection
then capture the first SQL statement up to the terminating semicolon using a
single robust pattern and validate with a simple whitelist
(SELECT/INSERT/UPDATE/DELETE) before execution; (3) change responses on failures
to return appropriate HTTP status codes (400 for user/parse errors, 502/503 for
upstream/LLM/network errors, 500 for internal/execution errors) instead of 200;
and (4) replace all print statements with standard logging (get a module logger)
using logger.debug/info/error and remove sys.stderr prints. Ensure sql is
sanitized/validated before calling execute_query and keep returned JSON
structure intact while adjusting HTTP status codes.
Assisted-by: GPT-5.6
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
.github/workflows/pr-review.yml (5)
39-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the competing report-file redirection.
Line 42 redirects standard output to
model_review.md, whilereview_diff.shalso truncates and appends to that same file. These independent file descriptors can overwrite report content. Remove the redirect, or changereview_diff.shto write only to standard output.Proposed fix
- ./review_diff.sh > model_review.md + ./review_diff.sh🤖 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 @.github/workflows/pr-review.yml around lines 39 - 42, Remove the standard-output redirection from the “Run LLM Code Review” workflow step so it invokes review_diff.sh without writing concurrently to model_review.md; preserve review_diff.sh’s existing report-file handling.
1-12: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd
contents: readto the workflow permissions.GitHub sets unspecified permissions to
nonewhen a workflow-levelpermissionsblock is present here, soactions/checkout@v4can fail without repository read access. Addcontents: readalongsidepull-requests: write.🤖 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 @.github/workflows/pr-review.yml around lines 1 - 12, Update the workflow-level permissions block in the llama-review job configuration to add contents: read alongside pull-requests: write, ensuring actions/checkout@v4 retains repository read access.
15-21: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheckout the immutable PR commit from the head repository.
ref: ${{ github.event.pull_request.head.ref }}checks out a mutable branch name from the default repository. Fork pull requests can fail when that branch is not in the base repository, and a new push can change the branch before the job runs. Specify the PR head repository and head SHA.Proposed fix
with: fetch-depth: 0 - ref: ${{ github.event.pull_request.head.ref }} + repository: ${{ github.event.pull_request.head.repo.full_name }} + ref: ${{ github.event.pull_request.head.sha }}🤖 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 @.github/workflows/pr-review.yml around lines 15 - 21, Update the actions/checkout step to use the pull request’s head repository and immutable head SHA from github.event.pull_request rather than head.ref. Preserve fetch-depth: 0 and ensure fork pull requests check out the exact commit submitted for review.
30-34: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the package installation fully non-interactive.
sudo apt-get install docker-model-plugincan block on confirmation, and-ymay still leave interactive configuration dialogs in action pipelines. Use a non-interactive frontend plus-ybefore runningdocker model version.Proposed fix
- sudo apt-get update - sudo apt-get install docker-model-plugin + sudo DEBIAN_FRONTEND=noninteractive apt-get update + sudo DEBIAN_FRONTEND=noninteractive apt-get install -y docker-model-plugin🤖 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 @.github/workflows/pr-review.yml around lines 30 - 34, Update the “Install Docker Model Runner (DMR)” workflow step to set the package manager frontend to noninteractive and pass the automatic-confirmation flag to apt-get install, ensuring installation completes without prompts before running docker model version.
50-59: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake the review comment creation idempotent and fail on HTTP errors.
This workflow runs on
pull_requestopenedandsynchronize, and every run posts to/issues/{pull_request.number}/comments. GitHub issue comments are not created idempotently throughPOST, so repeated events can create duplicate AI review comments. Make publish/update use an existing comment or a stable unique marker such as a predefined header/body signature, and make thecurlrequest fail on 4xx/5xx responses with--fail-with-body.🤖 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 @.github/workflows/pr-review.yml around lines 50 - 59, The “Post AI Comment via API” step must avoid duplicate comments across repeated workflow runs by locating and updating an existing AI review comment using a stable marker, or otherwise using an equivalent idempotent publish/update flow. Also add curl’s --fail-with-body option so HTTP 4xx/5xx responses fail the workflow while preserving the response body.
🧹 Nitpick comments (1)
.github/workflows/pr-review.yml (1)
36-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin the model used by CI.
Line 37 uses
ai/llama3.2:latest. The review result can change without a repository change. Use a fixed model tag or digest, and pass the same value toreview_diff.shinstead of duplicating thelatestvalue. Docker supports explicit model tags. (docs.docker.com)🤖 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 @.github/workflows/pr-review.yml around lines 36 - 37, Update the “Pull LLM model” workflow step to use a fixed Docker model tag or digest instead of ai/llama3.2:latest, then pass that same pinned value to review_diff.sh so the model reference is defined consistently and not duplicated.
🤖 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 @.github/workflows/pr-review.yml:
- Around line 15-18: Update the workflow around the PR checkout and review
execution so PR-controlled `review_diff.sh` code never runs with `pull-requests:
write` or `GITHUB_TOKEN` access. Run the review script from a trusted
base-branch revision, or separate execution from comment publication with
read-only and write-only permissions, and set `persist-credentials: false` on
any untrusted checkout.
In `@docker-compose.yml`:
- Line 12: Update the Go service configuration around the compose port mapping
and Go server startup settings to keep the exposed host/container port aligned
with the default PORT value of 12345. Remove or adjust the 12346:12346 mapping
unless the compose command explicitly sets PORT=12346 consistently for the Go
server.
In `@services/database_service.py`:
- Around line 28-29: Update the query validation and execution flow in
execute_query to require a validated maximum LIMIT for accepted SELECT
statements, configure a database-side statement timeout before fetchall(), and
detect when the enforced limit is reached so the response includes a truncation
indicator. Preserve the existing read-only statement restrictions for SHOW,
DESCRIBE, and EXPLAIN queries.
- Around line 28-29: Harden execute_query beyond the leading-keyword check:
validate parsed SQL before execution, reject LOAD_FILE and other file functions,
SELECT ... INTO OUTFILE/DUMPFILE, and locking clauses, and reject every
qualified table/schema reference outside ALLOWED_DATABASES. Ensure the database
principal used by execute_query has only SELECT grants for the three approved
schemas and no FILE privilege.
---
Outside diff comments:
In @.github/workflows/pr-review.yml:
- Around line 39-42: Remove the standard-output redirection from the “Run LLM
Code Review” workflow step so it invokes review_diff.sh without writing
concurrently to model_review.md; preserve review_diff.sh’s existing report-file
handling.
- Around line 1-12: Update the workflow-level permissions block in the
llama-review job configuration to add contents: read alongside pull-requests:
write, ensuring actions/checkout@v4 retains repository read access.
- Around line 15-21: Update the actions/checkout step to use the pull request’s
head repository and immutable head SHA from github.event.pull_request rather
than head.ref. Preserve fetch-depth: 0 and ensure fork pull requests check out
the exact commit submitted for review.
- Around line 30-34: Update the “Install Docker Model Runner (DMR)” workflow
step to set the package manager frontend to noninteractive and pass the
automatic-confirmation flag to apt-get install, ensuring installation completes
without prompts before running docker model version.
- Around line 50-59: The “Post AI Comment via API” step must avoid duplicate
comments across repeated workflow runs by locating and updating an existing AI
review comment using a stable marker, or otherwise using an equivalent
idempotent publish/update flow. Also add curl’s --fail-with-body option so HTTP
4xx/5xx responses fail the workflow while preserving the response body.
---
Nitpick comments:
In @.github/workflows/pr-review.yml:
- Around line 36-37: Update the “Pull LLM model” workflow step to use a fixed
Docker model tag or digest instead of ai/llama3.2:latest, then pass that same
pinned value to review_diff.sh so the model reference is defined consistently
and not duplicated.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0a5ceee-475e-42d5-a33d-0956cc7f049c
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/pr-review.ymldocker-compose.ymlrequirements.txtservices/database_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- requirements.txt
- .github/workflows/ci.yml
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| ref: ${{ github.event.pull_request.head.ref }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
git ls-files .github/workflows || true
echo "== pr-review.yml =="
if [ -f .github/workflows/pr-review.yml ]; then
cat -n .github/workflows/pr-review.yml
else
echo "missing .github/workflows/pr-review.yml"
fi
echo "== search review_diff and api/comment usage =="
rg -n "review_diff|persist-credentials|GITHUB_TOKEN|permissions:|pull-request|write|contents" .github/workflows . 2>/dev/null | head -n 200Repository: PuvaanRaaj/local-chatbot
Length of output: 3287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files named review_diff.sh or containing review_diff =="
fd -a '^review_diff\.sh$|review_diff' . || true
git ls-files '*review_diff*' || true
echo "== inspect action README availability (if cached deps exist) =="
for d in node_modules/actions/checkout node_modules/actions-checkout; do
[ -d "$d" ] && { echo "$d"; rg -n "persist-credentials|persist" "$d" 2>/dev/null | head -n 40 || true; }
done
echo "== deterministic facts from action metadata if present offline =="
python3 - <<'PY'
from pathlib import Path
for p in [Path('node_modules/actions/checkout/action.yml'), Path('.git/modules/actions/checkout/action.yml'), Path('node_modules/.cache')]:
print(p, p.exists())
PYRepository: PuvaanRaaj/local-chatbot
Length of output: 520
🌐 Web query:
actions/checkout v4 persist-credentials default true persist-credentials false
💡 Result:
In the actions/checkout v4 action, the persist-credentials parameter determines whether the authentication token (or SSH key) used for the checkout is configured in the local git configuration of the runner [1][2]. The default value for persist-credentials is true [1][3]. When set to true, the authentication token is persisted in the local git configuration [2][4]. This allows subsequent steps in the same job to perform authenticated git commands using that token without needing to re-authenticate [2][4]. The action ensures the token is removed during the post-job cleanup phase [2][4]. When set to false, the token is not persisted in the git configuration [2][4]. This is often used as a security best practice to prevent the GitHub token from remaining available to subsequent steps within the job, thereby reducing the potential blast radius if a later step is compromised [5][6]. If you wish to opt-out of this behavior, you should explicitly set persist-credentials: false in your workflow file [2][4]: - uses: actions/checkout@v4 with: persist-credentials: false
Citations:
- 1: https://github.com/actions/checkout/blob/v4/action.yml
- 2: https://github.com/actions/checkout/blob/v4/README.md
- 3: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 4: https://www.github.com/actions/checkout
- 5: Always emit
persist-credentials: falsewith actions/checkout github/gh-aw#2641 - 6: [security] Escalate concerning default
persist-credentials=trueplease actions/checkout#2312
Do not execute PR-controlled code with write-token permissions.
This job checks out the pull request head, runs ./review_diff.sh from that ref, then posts with pull-requests: write and GITHUB_TOKEN. A modified PR script can access the persisted actions/checkout credentials and read repository contents or write PR comments. Run review code from a trusted base-branch script, or split PR review execution and comment publication into separate jobs with read-only and write-only permissions. Set persist-credentials: false for the untrusted checkout.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 14-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/pr-review.yml around lines 15 - 18, Update the workflow
around the PR checkout and review execution so PR-controlled `review_diff.sh`
code never runs with `pull-requests: write` or `GITHUB_TOKEN` access. Run the
review script from a trusted base-branch revision, or separate execution from
comment publication with read-only and write-only permissions, and set
`persist-credentials: false` on any untrusted checkout.
| - "12345:12345" | ||
| volumes: | ||
| - .:/app | ||
| - "12346:12346" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'go-server|go build|EXPOSE|12346' Dockerfile docker-compose.yml
fd -t f -E .git . | rg '(^|/)(go\.mod|.*\.go)$'Repository: PuvaanRaaj/local-chatbot
Length of output: 1543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf -- '--- files ---\n'
git ls-files | sed -n '1,120p'
printf -- '\n--- docker-related files ---\n'
fd -t f -i '^(dockerfile|docker-compose.*\.ya?ml|compose.*\.ya?ml)$' .
printf -- '\n--- go files ---\n'
fd -t f -i '\.go$' .
printf -- '\n--- Dockerfile ---\n'
cat -n Dockerfile
printf -- '\n--- docker-compose.yml ---\n'
cat -n docker-compose.yml
printf -- '\n--- main.go relevant sections ---\n'
wc -l main.go
sed -n '1,220p' main.go | cat -nRepository: PuvaanRaaj/local-chatbot
Length of output: 7978
Keep the Go port mapping aligned with the Go server default.
main.go binds the Go server to the PORT environment variable and defaults it to 12345; the Go compose command and build copy establish /app/go-server, but the extra 12346:12346 mapping will only help if PORT=12346 is set consistently for the Go server.
🤖 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 `@docker-compose.yml` at line 12, Update the Go service configuration around
the compose port mapping and Go server startup settings to keep the exposed
host/container port aligned with the default PORT value of 12345. Remove or
adjust the 12346:12346 mapping unless the compose command explicitly sets
PORT=12346 consistently for the Go server.
| if not READ_ONLY_STATEMENT.match(query): | ||
| raise ValueError('Only read-only SELECT, SHOW, DESCRIBE, or EXPLAIN queries are allowed') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound query work before fetching results.
The validator accepts SELECT statements without a LIMIT. execute_query then calls fetchall() at Line 81, so one generated query can materialize an entire large table before the response formatter limits display rows.
Require a validated maximum LIMIT and configure a database-side statement timeout. Return a truncation indicator when the enforced limit is reached.
🤖 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 `@services/database_service.py` around lines 28 - 29, Update the query
validation and execution flow in execute_query to require a validated maximum
LIMIT for accepted SELECT statements, configure a database-side statement
timeout before fetchall(), and detect when the enforced limit is reached so the
response includes a truncation indicator. Preserve the existing read-only
statement restrictions for SHOW, DESCRIBE, and EXPLAIN queries.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce database scope and non-mutating SELECT semantics.
The leading-keyword check accepts SELECT LOAD_FILE(...), SELECT ... INTO OUTFILE, and queries against schemas outside ALLOWED_DATABASES. A prompt-controlled LLM response can reach execute_query with these statements. If the configured account has broad privileges, it can read database-host files, write files, or read unapproved schemas.
Use a dedicated database principal with SELECT grants only for the three approved schemas and no FILE privilege. Parse and reject file clauses, file functions, locking clauses, and out-of-scope qualified schema names before execution.
🤖 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 `@services/database_service.py` around lines 28 - 29, Harden execute_query
beyond the leading-keyword check: validate parsed SQL before execution, reject
LOAD_FILE and other file functions, SELECT ... INTO OUTFILE/DUMPFILE, and
locking clauses, and reject every qualified table/schema reference outside
ALLOWED_DATABASES. Ensure the database principal used by execute_query has only
SELECT grants for the three approved schemas and no FILE privilege.
|
Files changed in last commit (23d2d3f): Review Guidelines:
Review Areas:
Output Format:For each file, provide:
Code to Review:\n\n### File: .github/workflows/ci.yml\n```\nname: Lint and Test on: [push] ---- end preview ---- Critical Issues
Suggestions
def execute_query(sql):
def process_llm_response(llm_response): def extract_sql(llm_response): def format_response(query_result):
def handle_database_connection(connection): def execute_query(sql): |
Assisted-by: GPT-5.6
Assisted-by: GPT-5.6
Assisted-by: GPT-5.6
|
No relevant files changed in the last commit (d926c85). |
|
Files changed in last commit (f09c923): Review Guidelines:
Review Areas:
Output Format:For each file, provide:
Code to Review:\n\n### File: .github/workflows/ci.yml\n```\nname: Lint and Test on: [push] ---- end preview ---- Summary: The provided CI/CD workflow is well-structured and covers essential tools for code quality checks. However, there are some areas that can be improved for better maintainability, security, and performance. Critical Issues:
Suggestions:
- name: Cache pip dependencies
uses: actions/cache@v4
with:
path: ${{ env.PIP_CACHE_PATH }}
key: ${{ runner.os }}-pip-${{ hashFiles('requirements.txt') }}
restore-keys: |
${{ runner.os }}-pip-
- name: Compile dependencies
run: |
pip-compile --no-deps --requirement requirements.txt
Positive Notes:
Additional Recommendations:
|
|
Files changed in last commit (4457b50): Review Guidelines:
Review Areas:
Output Format:For each file, provide:
Code to Review:\n\n### File: app.py\n```\nimport os from dotenv import load_dotenv The code provided appears to be a complex Flask application that handles natural language database queries. The application uses a LLM to convert natural language prompts into SQL queries and then executes the queries on a MySQL database. While the code is well-structured, there are several areas that require improvement to enhance security, performance, and maintainability. Critical Issues
Suggestions
query_result = execute_query("SELECT * FROM table LIMIT 100")
import os host = os.getenv("DB_HOST", "localhost") connection = pymysql.connect(
def main(): if name == "main": |
1 participant
📌 What Does This PR Do?
Closes #1
Test Cases
Logs / Screenshots
Developer Checklist
Reviewer Checklist
Summary by CodeRabbit
New Features
Documentation
Chores