chore(security): scope CodeQL to product code and clear false-positive alerts#426
Merged
Conversation
…e alerts CodeQL runs +security-extended,security-and-quality across the whole repo, so the quality queries fire heavily in tests, build scripts, and the static site. The result is ~330 open alerts in which the real findings are invisible. - Add .github/codeql/codeql-config.yml with paths-ignore for test suites, build/release tooling, the marketing site, and vendored/generated trees, and wire it into the CodeQL init step. - Re-declare the lazy exports of agents/__init__.py in a TYPE_CHECKING block. The names in __all__ resolve through __getattr__ at runtime, which CodeQL cannot follow, producing 20 false 'py/undefined-export' errors. The block is never executed, so lazy loading is unchanged. - Replace the root-level test_verify_subtask_2_2.py verification script from #154 with tests/test_error_classification.py. The script was never collected by pytest and had gone stale: it asserted the string-based error detection that has since been removed, so it would fail if run. The new tests pin the current typed-only contract, which had no coverage at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Oleg Miagkov <mrobenner@gmail.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
📝 WalkthroughWalkthroughChangesCodeQL static analysis
Error classification verification
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_error_classification.py`:
- Around line 22-31: Update the pytest.warns calls in _is_rate_limit and
_is_auth to include match patterns for the expected deprecation warning
messages, ensuring each assertion targets the specific deprecated helper warning
rather than unrelated warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1b62220-7a26-4134-81f7-53adee9536ac
📒 Files selected for processing (5)
.github/codeql/codeql-config.yml.github/workflows/quality-security.ymlapps/backend/agents/__init__.pytest_verify_subtask_2_2.pytests/test_error_classification.py
💤 Files with no reviewable changes (1)
- test_verify_subtask_2_2.py
Comment on lines
+22
to
+31
| def _is_rate_limit(error: Exception) -> bool: | ||
| """Call the deprecated helper, asserting it still warns.""" | ||
| with pytest.warns(DeprecationWarning): | ||
| return is_rate_limit_error(error) | ||
|
|
||
|
|
||
| def _is_auth(error: Exception) -> bool: | ||
| """Call the deprecated helper, asserting it still warns.""" | ||
| with pytest.warns(DeprecationWarning): | ||
| return is_authentication_error(error) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the deprecation warning assertions stricter.
It is a recommended pytest convention to use the match parameter with pytest.warns to ensure you are catching the specific warning intended, rather than unrelated deprecations that might bubble up from internal dependencies. As per path instructions, ensure tests follow pytest conventions.
♻️ Proposed refactor to add `match` parameters
def _is_rate_limit(error: Exception) -> bool:
"""Call the deprecated helper, asserting it still warns."""
- with pytest.warns(DeprecationWarning):
+ with pytest.warns(DeprecationWarning, match=r"is_rate_limit_error\(\) is deprecated"):
return is_rate_limit_error(error)
def _is_auth(error: Exception) -> bool:
"""Call the deprecated helper, asserting it still warns."""
- with pytest.warns(DeprecationWarning):
+ with pytest.warns(DeprecationWarning, match=r"is_authentication_error\(\) is deprecated"):
return is_authentication_error(error)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _is_rate_limit(error: Exception) -> bool: | |
| """Call the deprecated helper, asserting it still warns.""" | |
| with pytest.warns(DeprecationWarning): | |
| return is_rate_limit_error(error) | |
| def _is_auth(error: Exception) -> bool: | |
| """Call the deprecated helper, asserting it still warns.""" | |
| with pytest.warns(DeprecationWarning): | |
| return is_authentication_error(error) | |
| def _is_rate_limit(error: Exception) -> bool: | |
| """Call the deprecated helper, asserting it still warns.""" | |
| with pytest.warns(DeprecationWarning, match=r"is_rate_limit_error\(\) is deprecated"): | |
| return is_rate_limit_error(error) | |
| def _is_auth(error: Exception) -> bool: | |
| """Call the deprecated helper, asserting it still warns.""" | |
| with pytest.warns(DeprecationWarning, match=r"is_authentication_error\(\) is deprecated"): | |
| return is_authentication_error(error) |
🤖 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_error_classification.py` around lines 22 - 31, Update the
pytest.warns calls in _is_rate_limit and _is_auth to include match patterns for
the expected deprecation warning messages, ensuring each assertion targets the
specific deprecated helper warning rather than unrelated warnings.
Source: Path instructions
This was referenced Jul 17, 2026
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.
First of three PRs from a triage of the code scanning and Dependabot dashboards. This one is the noise-reduction slice: no behaviour changes.
The problem
330 open code-scanning alerts (17 high, 20 error, 45 medium, 230 note). Triaging the serious-looking ones by hand, almost every high/error is a false positive:
py/undefined-export(agents/__init__.py)__getattr__/_LAZY_EXPORTS; CodeQL can't follow lazy importsjs/insufficient-password-hash(credential-utils.ts)SHA256(configDir)[:8]namespaces keychain entries; it isn't a password hashpy/clear-text-logging(security_scanner.py)pattern_type/file/lineonly, never secret valuesThe rest is overwhelmingly hygiene (87 unused-local-variable, 64 unused-import, 23 cyclic-import, …) concentrated in test suites, build scripts, and
site/. Real findings are invisible in that pile.What this changes
1. Scope CodeQL to product code. New
.github/codeql/codeql-config.ymlwithpaths-ignorefor test suites, build/release tooling, the static marketing site, and vendored/generated trees; wired into the CodeQL init step. Query suites are unchanged —apps/,libs/and the rest of the shipped code are still scanned by+security-extended,security-and-quality.2. Clear the 20 false
py/undefined-exporterrors properly.agents/__init__.pygains aTYPE_CHECKINGblock re-declaring each lazy export, so static analysers can resolve__all__. It never executes, so__getattr__still does the work at runtime. Verified: all 22 names in__all__resolve and still load from their original modules.3. Replace a stale root-level script with real tests.
test_verify_subtask_2_2.py(left at the repo root by #154) generated 6 alerts and was never collected by pytest — wrong location, and its imports only resolve fromapps/backend.Worth flagging: it had also gone stale. It asserts that string-based error detection works, but
is_rate_limit_error/is_authentication_errorare now typed-only (the string fallback was deliberately removed, and both are deprecated) — so the script would fail if anyone ran it. Rather than delete it, I turned it intotests/test_error_classification.py, which pins the current typed-only contract and the deprecation warnings. Those helpers previously had no test coverage anywhere, so this is a net gain rather than a removal.Verification
pytest tests/test_error_classification.py→ 11 passedagents.__all__resolves;run_autonomous_agent→agents.coder,find_subtask_in_plan→agents.utilsThe effect on the alert count lands once CodeQL re-runs on
develop. The remaining real items (sanitizer regex, log-injection, file-system-race) come in a later PR.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Quality Improvements