fix: validate build_mode in CodeQL plan repair loop and CLI check#42
Merged
Merged
Conversation
_validate_codeql_plan_for_repair() previously only validated build_command portability (shell syntax, /tmp/ paths, etc.) but did not check whether build_mode was supported for the target language. This meant the model could write build_mode: 'none' for c-cpp, pass self-validation via 'check-codeql-plan', exit the retry loop, and only be caught later by the post-loop gate check — with no feedback path to the model. Now the function also validates: - build_mode against supported_build_modes(language_id) - manual build_mode requires a non-empty build_command Additionally, _codeql_repair_needed() now detects plan-level unsupported build_modes as a trigger for the CodeQL repair loop, not just 'Database create failed' runtime errors.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPhase 1 adds CodeQL capability-aware validation to enforce per-language ChangesCodeQL Plan Validation and Repair Trigger
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage Report
Generated by pytest-cov on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_phase_1_codeql_plan_repair.py (1)
44-44: ⚡ Quick winAdd a focused regression test for the new
build_modechecks.This fixture update keeps the existing resume-path test valid, but this file still doesn't directly assert the new behavior from the PR: rejecting unsupported/blank
build_modevalues and rejectingmanualwithout abuild_command. A dedicated test for those cases would pin the exact regression this change is fixing.🤖 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 `@tests/test_phase_1_codeql_plan_repair.py` at line 44, Add a focused regression test in tests/test_phase_1_codeql_plan_repair.py that asserts the new build_mode checks: create two tests (e.g., test_reject_unsupported_or_blank_build_mode and test_reject_manual_without_build_command) that construct minimal plan YAMLs modifying the build_mode field and verify the validator/repair function rejects unsupported or blank build_mode values and that setting build_mode: manual without a build_command triggers an error; reference the build_mode and build_command fields and call the same validation/repair entrypoint used by existing tests to assert an exception or error message is produced.
🤖 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 `@tools/codecome/phase_1.py`:
- Around line 277-291: The code currently lets missing, blank or non-string
build_mode values slip through; update the validation around build_mode in
phase_1.py so you normalize and validate language["build_mode"] once (e.g.,
coerce to a single variable like build_mode = language.get("build_mode") and
treat None, non-str, or str.strip() == "" as invalid) before calling
is_supported_language(language_id) or supported_build_modes(language_id); if
invalid, append an error using the existing context message (same style as the
other errors) and ensure this normalized check is used consistently wherever
build_mode is validated (including the other similar block around lines 411-419
and any callers like _codeql_repair_needed()).
---
Nitpick comments:
In `@tests/test_phase_1_codeql_plan_repair.py`:
- Line 44: Add a focused regression test in
tests/test_phase_1_codeql_plan_repair.py that asserts the new build_mode checks:
create two tests (e.g., test_reject_unsupported_or_blank_build_mode and
test_reject_manual_without_build_command) that construct minimal plan YAMLs
modifying the build_mode field and verify the validator/repair function rejects
unsupported or blank build_mode values and that setting build_mode: manual
without a build_command triggers an error; reference the build_mode and
build_command fields and call the same validation/repair entrypoint used by
existing tests to assert an exception or error message is produced.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2cdaeef-ef82-4d09-bfcc-162ffe10ee53
📒 Files selected for processing (2)
tests/test_phase_1_codeql_plan_repair.pytools/codecome/phase_1.py
…ssion tests - _validate_codeql_plan_for_repair(): extract build_mode without default, reject None/blank/non-string values explicitly before checking supported_build_modes(), instead of silently skipping validation. - _codeql_repair_needed(): use effective build_mode (defaulting to 'none' as the runner does) so missing/blank build_mode for unsupported languages also triggers the repair loop. - Add three regression tests: unsupported build_mode, missing build_mode, and manual without build_command.
Owner
Author
|
Addressed review comments in 53d8d91: Re: blank/non-string build_mode (Major, tools/codecome/phase_1.py)
Re: Regression tests (Nitpick, tests/test_phase_1_codeql_plan_repair.py)
All 621 tests pass. |
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.
Problem
Phase 1 failed because the model wrote
build_mode: "none"for c-cpp initemdb/notes/codeql-plan.yml, which is not a valid build mode for C/C++ (onlyautobuildandmanualare supported).The root cause:
_validate_codeql_plan_for_repair()— called by both the phase retry loop and thecheck-codeql-planCLI command — only validated build_command portability but never checked whetherbuild_modewas supported for the target language. The gate check correctly caught the error, but only after the retry loop had already exited.Changes
tools/codecome/phase_1.py_validate_codeql_plan_for_repair()— Addedbuild_modevalidation againstsupported_build_modes(language_id)for each language entry. Now:nonefor c-cpp)manualwithout abuild_commandcheck-codeql-planself-validation command_codeql_repair_needed()— Added plan-level detection of unsupported build_modes as a trigger for the CodeQL repair loop (defense-in-depth: covers the case where the runner catches the error instead of the validator).tests/test_phase_1_codeql_plan_repair.py_write_valid_planto includebuild_mode: autobuildfor c-cpp, since the old fixture had no build_mode (which now correctly fails validation).Summary by CodeRabbit
Bug Fixes
Tests