Benchmarking workflow setup - #1
Open
NitishDevzy wants to merge 3 commits into
Open
Conversation
|
| Severity | File | Description |
|---|---|---|
| 🔴 Critical | …/code_review_benchmark/bench_naming.py |
🔒 LDAP Injection |
| 🔴 Critical | …/code_review_benchmark/bench_naming.py |
🔒 LDAP Injection |
📂 File Changes
| Files | Summary |
|---|---|
offline/code_review_benchmark/bench_naming.py |
Adds shared parsing utilities for benchmark repository names and PR metadata extraction. |
🛡️ Security Analysis Report📊 Issue Summary
🔍 Issues by Category💉 Injection Vulnerabilities (1) - 🚨 1 critical issue(s) requiring immediate attention
💡 Recommendations
Powered by Devzy Security Scanner - Detecting 90+ security patterns across OWASP Top 10, cloud secrets, and infrastructure security. security-summary |
![devzyai-develop[bot]](https://avatars.githubusercontent.com/u/211546431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (7)
- .github/workflows/benchmark-offline.yml (1)
- README.md (1)
- offline/README.md (6)
- offline/analysis/merge_neatcode_into_openai_gpt52.py (1)
- offline/analysis/report_all_tools.py (1)
- offline/analysis/report_scores_by_repo.py (1)
- offline/code_review_benchmark/step5_label_prs.py (1)
Files ignored due to filter (11)
- offline/analysis/benchmark_dashboard.json
- offline/golden_comments/discourse.json
- offline/results/anthropic_claude-opus-4-5-20251101/candidates.json
- offline/results/anthropic_claude-opus-4-5-20251101/evaluations.json
- offline/results/anthropic_claude-sonnet-4-5-20250929/candidates.json
- offline/results/anthropic_claude-sonnet-4-5-20250929/evaluations.json
- offline/results/benchmark_data.json
- offline/results/openai_gpt-5.2/candidates.json
- offline/results/openai_gpt-5.2/evaluations.json
- offline/results/pr_labels.json
- offline/uv.lock
⚠️ Architecture Analysis (2 issues)
🟠 High: 2 issue(s)
- signature_mismatch: Function "load_dotenv" accepts at most 0 argument(s) but is called with 1 at line 12 in "online/etl/config.py".
- signature_mismatch: Function "load_dotenv" accepts at most 0 argument(s) but is called with 1 at line 17 in "online/etl/dashboard/app.py".
📊 Impact Analysis (3 high-impact changes)
load_dotenv: 27 dependents (high)sanitize_model_name: 25 dependents (high)process_batch: 21 dependents (high)
Review comments generated (3)
- Review: 3
- LGTM: 0
Priority ranking: Showing top 3 of 14 most impactful findings (5 cross-file duplicates, 6 lower-value findings suppressed).
Additional findings (outside changed lines) (1)
These were referenced by the model but are outside the PR diff hunks, so they cannot be posted as inline review comments.
offline/code_review_benchmark/step5_label_prs.py:9-19[medium]⚠️ Behavioral Regression: Removing "discourse" mapping will causederive_language()to return None for Discourse PRs TheUPSTREAM_REPO_TO_LANGUAGEmapping is used by `derive_la...
Tips
Chat with DevzyAi Bot (@DevzyAi)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@DevzyAiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@DevzyAi: ignoreanywhere in the PR description to pause further reviews from the bot.
Comment on lines
+22
to
+122
| default: false | ||
| step1_force: | ||
| description: "Pass --force to step 1 (refetch reviews)" | ||
| required: false | ||
| type: boolean | ||
| default: false | ||
| step1_test: | ||
| description: "Pass --test to step 1 (one repo per tool)" | ||
| required: false | ||
| type: boolean | ||
| default: false | ||
| judge_model: | ||
| description: "Override MARTIAN_MODEL for the judge (leave empty to use MARTIAN_MODEL secret)" | ||
| required: false | ||
| type: string | ||
| default: "" | ||
| limit: | ||
| description: "Optional --limit for steps 2 and 3 (smoke runs); leave empty for full run" | ||
| required: false | ||
| type: string | ||
| default: "" | ||
|
|
||
| concurrency: | ||
| group: benchmark-offline-${{ github.repository }} | ||
| cancel-in-progress: false | ||
|
|
||
| defaults: | ||
| run: | ||
| working-directory: offline | ||
|
|
||
| jobs: | ||
| benchmark: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 360 | ||
| permissions: | ||
| contents: read | ||
|
|
||
| env: | ||
| GH_TOKEN: ${{ secrets.BENCHMARK_GH_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.BENCHMARK_GH_TOKEN }} | ||
| MARTIAN_API_KEY: ${{ secrets.MARTIAN_API_KEY }} | ||
| MARTIAN_MODEL: ${{ inputs.judge_model || secrets.MARTIAN_MODEL }} | ||
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ inputs.ref }} | ||
|
|
||
| - name: Verify secrets | ||
| run: | | ||
| set -euo pipefail | ||
| if [ -z "${GH_TOKEN:-}" ]; then echo "Missing secret BENCHMARK_GH_TOKEN"; exit 1; fi | ||
| if [ -z "${MARTIAN_API_KEY:-}" ]; then echo "Missing secret MARTIAN_API_KEY"; exit 1; fi | ||
| if [ -z "${MARTIAN_MODEL:-}" ]; then echo "Set secret MARTIAN_MODEL or pass judge_model input"; exit 1; fi | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v4 | ||
| with: | ||
| version: "latest" | ||
|
|
||
| - name: Install dependencies | ||
| run: uv sync | ||
|
|
||
| - name: Step 0 — orchestrate forks | ||
| if: ${{ inputs.run_step0 }} | ||
| timeout-minutes: 180 | ||
| run: >- | ||
| uv run python -m code_review_benchmark.step0_orchestrate_forks | ||
| --org "${{ inputs.benchmark_org }}" | ||
| --name "${{ inputs.tool }}" | ||
| --golden-dir golden_comments | ||
|
|
||
| - name: Step 1 — download PR data | ||
| timeout-minutes: 60 | ||
| run: | | ||
| set -euo pipefail | ||
| ARGS=(--org "${{ inputs.benchmark_org }}" --output results/benchmark_data.json --tool "${{ inputs.tool }}") | ||
| if [[ "${{ inputs.step1_force }}" == "true" ]]; then ARGS+=(--force); fi | ||
| if [[ "${{ inputs.step1_test }}" == "true" ]]; then ARGS+=(--test); fi | ||
| uv run python -m code_review_benchmark.step1_download_prs "${ARGS[@]}" | ||
|
|
||
| - name: Step 2 — extract comments | ||
| timeout-minutes: 120 | ||
| run: | | ||
| set -euo pipefail | ||
| CMD=(uv run python -m code_review_benchmark.step2_extract_comments --tool "${{ inputs.tool }}") | ||
| if [[ -n "${{ inputs.limit }}" ]]; then CMD+=(--limit "${{ inputs.limit }}"); fi | ||
| "${CMD[@]}" | ||
|
|
||
| - name: Step 2.5 — deduplicate candidates | ||
| timeout-minutes: 120 | ||
| run: >- | ||
| uv run python -m code_review_benchmark.step2_5_dedup_candidates | ||
| --tool "${{ inputs.tool }}" | ||
|
|
||
| - name: Step 3 — judge comments | ||
| timeout-minutes: 300 | ||
| run: | | ||
| set -euo pipefail | ||
| MODEL_SLUG="${MARTIAN_MODEL//\//_}" |
There was a problem hiding this comment.
🟡 Medium
🐛 Bug: Shell parameter expansion will fail if MARTIAN_MODEL contains special characters
The ${MARTIAN_MODEL//\//_} expansion replaces forward slashes with underscores, but if MARTIAN_MODEL contains other shell metacharacters (spaces, quotes, etc.), the unquoted expansion will cause word splitting or glob expansion failures.
🛠️ Code Suggestions
- MODEL_SLUG="${MARTIAN_MODEL//\//_}"
+ MODEL_SLUG="${MARTIAN_MODEL//\//_}"
+ MODEL_SLUG="${MODEL_SLUG// /_}"Actually, a safer approach is to quote the variable and handle all special characters:
🛠️ Code Suggestions
- MODEL_SLUG="${MARTIAN_MODEL//\//_}"
+ MODEL_SLUG="$(echo "${MARTIAN_MODEL}" | tr '/' '_' | tr -cd '[:alnum:]_-')"🤖 AI Fix Prompt - Copy this into your AI coding agent
Validation: Read .github/workflows/benchmark-offline.yml and check the MODEL_SLUG assignment around line 60. Confirm that MARTIAN_MODEL is used in a shell parameter expansion without quotes and that the expansion only handles forward slashes. If the variable is already quoted or if the workflow guarantees MARTIAN_MODEL contains only alphanumeric and slash characters, skip this fix.
Problem & Location: In .github/workflows/benchmark-offline.yml, the line MODEL_SLUG="${MARTIAN_MODEL//\//_}" uses unquoted parameter expansion. If MARTIAN_MODEL contains spaces, quotes, or other shell metacharacters, the expansion will trigger word splitting or glob expansion, causing the workflow step to fail or produce incorrect slugs.
Fix: Replace the shell parameter expansion with a sanitization approach that strips all non-alphanumeric characters except underscores and hyphens. Reference the sanitize_model_name function in offline/code_review_benchmark/step5_label_prs.py (and step3, step4) which uses .replace("/", "_") and .strip(). Adapt that logic to shell by using tr to replace slashes with underscores and then delete any character that is not alphanumeric, underscore, or hyphen. The corrected line should be MODEL_SLUG="$(echo "${MARTIAN_MODEL}" | tr '/' '_' | tr -cd '[:alnum:]_-')".
Verify: Check that the benchmark-offline workflow runs successfully with model names containing spaces or special characters. If the workflow has integration tests or example runs, execute them to confirm MODEL_SLUG is correctly sanitized.
| GH_TOKEN: ${{ secrets.BENCHMARK_GH_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.BENCHMARK_GH_TOKEN }} | ||
| MARTIAN_API_KEY: ${{ secrets.MARTIAN_API_KEY }} | ||
| MARTIAN_MODEL: ${{ inputs.judge_model || secrets.MARTIAN_MODEL }} |
There was a problem hiding this comment.
🟠 High
🐛 Bug: Empty string from judge_model input will override MARTIAN_MODEL secret
When inputs.judge_model is an empty string (the default), the expression ${{ inputs.judge_model || secrets.MARTIAN_MODEL }} evaluates to empty string, not the secret. GitHub Actions treats empty strings as truthy in the || operator.
🛠️ Code Suggestions
- MARTIAN_MODEL: ${{ inputs.judge_model || secrets.MARTIAN_MODEL }}
+ MARTIAN_MODEL: ${{ inputs.judge_model != '' && inputs.judge_model || secrets.MARTIAN_MODEL }}🤖 AI Fix Prompt - Copy this into your AI coding agent
Validation: Read .github/workflows/benchmark-offline.yml and confirm that the MARTIAN_MODEL environment variable assignment uses the expression `${{ inputs.judge_model || secrets.MARTIAN_MODEL }}` without checking if judge_model is an empty string. If the expression already includes an empty-string check, skip this fix.
Problem & Location: In .github/workflows/benchmark-offline.yml, the env section sets MARTIAN_MODEL using `${{ inputs.judge_model || secrets.MARTIAN_MODEL }}`. Because judge_model has a default value of empty string, and GitHub Actions treats empty strings as truthy in the OR operator, an empty string input will override the secret instead of falling back to it. This breaks the workflow when triggered without explicitly setting judge_model, causing the benchmark to fail with no model configured.
Fix: Change the MARTIAN_MODEL assignment to check if inputs.judge_model is not empty before using it. Use the expression `${{ inputs.judge_model != '' && inputs.judge_model || secrets.MARTIAN_MODEL }}` so that only non-empty judge_model inputs override the secret, and empty strings fall through to secrets.MARTIAN_MODEL.
Verify: Run the benchmark-offline workflow twice: once with no judge_model input (should use MARTIAN_MODEL secret) and once with an explicit judge_model value (should use that value). Check that offline/code_review_benchmark/step3_judge_comments.py successfully initializes the LLM client in both cases.
Comment on lines
+115
to
+124
| for golden_url, tools in data.items(): | ||
| if not golden_url_in_repo(golden_url, canonical): | ||
| continue | ||
| result = tools.get(tool) | ||
| if not result or result.get("skipped"): | ||
| continue | ||
| tp += int(result.get("tp", 0)) | ||
| fp += int(result.get("fp", 0)) | ||
| fn += int(result.get("fn", 0)) | ||
| n_reviews += 1 |
There was a problem hiding this comment.
🟡 Medium
🐛 Bug: Silent data loss when result dictionary contains non-numeric values
The int() conversions will raise ValueError if the JSON contains non-numeric strings (e.g., "tp": "N/A" or "tp": null). This will crash the script instead of skipping the malformed entry or reporting it.
🛠️ Code Suggestions
result = tools.get(tool)
if not result or result.get("skipped"):
continue
- tp += int(result.get("tp", 0))
- fp += int(result.get("fp", 0))
- fn += int(result.get("fn", 0))
+ try:
+ tp += int(result.get("tp", 0))
+ fp += int(result.get("fp", 0))
+ fn += int(result.get("fn", 0))
+ except (ValueError, TypeError) as e:
+ print(f"Warning: Skipping {golden_url} due to invalid metrics: {e}", file=sys.stderr)
+ continue
n_reviews += 1Note: This requires adding import sys at the top of the file.
🤖 AI Fix Prompt - Copy this into your AI coding agent
Validation: Read offline/analysis/report_scores_by_repo.py and check whether the JSON data loaded from the benchmark results can ever contain non-numeric values for "tp", "fp", or "fn" keys. If the upstream data pipeline guarantees these are always integers or the script is only used with known-good data, this is a false positive and no fix is needed.
Problem & Location: In offline/analysis/report_scores_by_repo.py, the code that accumulates tp, fp, fn metrics directly calls int() on dictionary values without handling cases where the JSON might contain null, non-numeric strings, or missing keys that default to 0 but are then cast. The lines are:
tp += int(result.get("tp", 0))
fp += int(result.get("fp", 0))
fn += int(result.get("fn", 0))
If the benchmark JSON ever contains malformed data (e.g., "tp": "N/A" or "tp": null), the script will crash with ValueError or TypeError instead of skipping the bad entry or reporting it.
Fix: Wrap the three int() conversions in a try-except block that catches ValueError and TypeError. On exception, print a warning to stderr identifying the golden_url and the error, then continue to the next iteration. Add "import sys" at the top of the file if not already present. Reference offline/code_review_benchmark/step_speed_analysis.py for similar error handling patterns if needed.
Verify: Check whether any other scripts in offline/analysis/ or offline/code_review_benchmark/ consume the same JSON format and confirm they handle malformed data consistently. Run the script against a sample JSON file with intentionally malformed metrics to confirm the warning is printed and the script continues.
NitishDevzy
force-pushed
the
workflow/benchmarking
branch
2 times, most recently
from
April 15, 2026 13:17
e0693ea to
e48ba75
Compare
| rest = head_ref[3:] | ||
| if rest.isdigit(): | ||
| return int(rest) | ||
| m = re.search(r"-(\d+)$", rest) |
There was a problem hiding this comment.
🔴 Critical
🔒 LDAP Injection
🚨 LDAP Injection: User input in LDAP query without proper escaping.
Recommendation: Use parameterized LDAP queries or properly escape special characters (* , \ ( ) etc).
🤖 AI Fix Prompt - Copy this into your AI coding agent
|
|
||
| def source_repo_from_github_pr_url(url: str) -> str | None: | ||
| """Return repo segment from a ``https://github.com/{owner}/{repo}/pull/{n}`` URL.""" | ||
| m = re.search(r"github\.com/[^/]+/([^/]+)/pull/\d+", url) |
There was a problem hiding this comment.
🔴 Critical
🔒 LDAP Injection
🚨 LDAP Injection: User input in LDAP query without proper escaping.
Recommendation: Use parameterized LDAP queries or properly escape special characters (* , \ ( ) etc).
🤖 AI Fix Prompt - Copy this into your AI coding agent
📋 Additional Findings (Outside Changed Lines)The following issues were detected in areas related to your changes but are outside the diff range. These cannot be added as inline comments but may be relevant to your PR. 📍
|
NitishDevzy
force-pushed
the
workflow/benchmarking
branch
from
April 15, 2026 13:36
e48ba75 to
af8b6f4
Compare
|
✅ Review cycle complete for this PR (max 2 rounds reached). To request another review, comment |
NitishDevzy
force-pushed
the
workflow/benchmarking
branch
from
April 15, 2026 16:40
af8b6f4 to
91eaeed
Compare
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
1 participant
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 by DevzyAi
Release Notes
New Feature
Bug Fix
.json.bakformatDocumentation
Chore