feat(gitlab): comprehensive integration alignment with GitHub #1413
+17,222
−68
Conversation
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
Implement full GitLab integration parity with GitHub, including multi-pass MR review, bot detection, CI/CD checking, file locking, rate limiting, and comprehensive test suite. Implementation (7 new files, ~2,600 lines): - providers/gitlab_provider.py: GitProvider protocol implementation - services/context_gatherer.py: MR context gathering with AI bot comment detection - services/ci_checker.py: GitLab CI/CD pipeline status checking - bot_detection.py: Bot detection with cooling-off period - utils/file_lock.py: Concurrent-safe file operations - utils/rate_limiter.py: Token bucket rate limiting Enhanced files (5 files): - glab_client.py: Added async methods and new API endpoints - models.py: Added 6-pass review, evidence-based findings, structural issues - orchestrator.py: Integrated bot detection, CI checking, multi-pass review - runner.py: Added triage, auto-fix, batch-issues CLI commands - services/__init__.py: Export new services Test suite (8 new files, ~2,400 lines): - test_gitlab_provider.py: GitProvider protocol tests - test_gitlab_bot_detection.py: Bot detection tests - test_gitlab_ci_checker.py: CI checker tests - test_gitlab_mr_review.py: MR review models tests - test_gitlab_mr_e2e.py: End-to-end review lifecycle tests - test_gitlab_file_lock.py: File locking tests - test_gitlab_rate_limiter.py: Rate limiter tests - test_glab_client.py: Client timeout/retry tests - fixtures/gitlab.py: Test fixtures Features implemented: - Phase 1: GitProvider protocol for GitLab - Phase 2: 6-pass MR review (quick_scan, security, quality, deep_analysis, structural, ai_comment_triage) - Phase 3: Bot detection with cooling-off period - Phase 4: CI/CD pipeline integration - Phase 5: File locking for concurrent safety - Phase 6: Token bucket rate limiting - Phase 7: Enhanced orchestrator with all integrations - Phase 8: Comprehensive test suite - Phase 9: CLI commands (triage, auto-fix, batch-issues) - Phase 10: Frontend parity (IPC handlers already complete) All code follows Python best practices with type hints, error handling, logging, and cross-platform compatibility.
- Add params parameter to GitLabClient._fetch() method - Update runners/__init__.py to comment out missing roadmap_runner import - Update .gitignore to exclude gitlab-integration-tests directory
- Fix check_encoding.py to handle nested parentheses in open() calls - Add encoding="utf-8" to file operations in GitLab test files - Add encoding to bot_detection.py and file_lock.py - Fix trailing whitespace and end-of-file issues (auto-fixed)
…, and tests This commit completes the GitLab integration implementation with full feature parity to GitHub, including: Core API Extensions (glab_client.py): - Merge Request CRUD: create_mr, list_mrs, update_mr - Branch Management: list_branches, get_branch, create_branch, delete_branch, compare_branches - File Operations: get_file_contents, create_file, update_file, delete_file - Webhook Management: list_webhooks, get_webhook, create_webhook, update_webhook, delete_webhook - Enhanced _fetch method with params support and retry logic New Service Modules: - autofix_processor.py: Auto-fix workflow with permission verification - batch_issues.py: Issue batching with Claude AI similarity detection - permissions.py: GitLab permission checker for automation triggers - services/triage_engine.py: AI-driven issue triage and categorization - services/followup_reviewer.py: Automated MR review for follow-up commits - services/context_gatherer.py: Enhanced context gathering with monorepo support - services/prompt_manager.py: System prompt management - services/response_parsers.py: Parse Claude API responses - services/batch_processor.py: Batch issue processing Enhanced Modules: - models.py: AutoFixState, GitLabRunnerConfig, and other data models - bot_detection.py: Bot comment detection with import fixes - utils/file_lock.py: Added encoding parameter to atomic_write - utils/rate_limiter.py: Updated for GitLab (not GitHub) - types.py: TypedDict type definitions for API responses Test Files (12 new test files, 122 tests): - test_gitlab_client_extensions.py: MR CRUD, branch, file, webhook tests - test_gitlab_client_errors.py: Retry logic, rate limiting, error handling - test_gitlab_branch_operations.py: Branch management tests - test_gitlab_file_operations.py: File operations tests - test_gitlab_webhook_operations.py: Webhook management tests - test_gitlab_autofix_processor.py: Auto-fix workflow tests - test_gitlab_batch_issues.py: Issue batching tests - test_gitlab_triage_engine.py: Triage engine tests - test_gitlab_followup_reviewer.py: Follow-up reviewer tests - test_gitlab_context_gatherer.py: Context gathering tests - test_gitlab_permissions.py: Permission checker tests - test_gitlab_types.py: TypedDict type tests All tests passing (122/122).
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces comprehensive GitLab integration infrastructure, including a full-featured GitLab client, MR review orchestration with bot detection and CI/CD pipeline checking, auto-fix workflow for issues, issue batching and triage engines, permission verification, file locking utilities, and rate limiting—with extensive test coverage. Changes
Sequence DiagramssequenceDiagram
participant Client as GitLab API
participant Orch as Orchestrator
participant Bot as BotDetector
participant CI as CIChecker
participant Gatherer as MRContextGatherer
participant Engine as MRReviewEngine
participant Result as MRReviewResult
Client->>Orch: MR data
Orch->>Bot: should_skip_mr_review(mr_data, commits)
Bot-->>Orch: skip_decision + reason
alt Bot detected, skip
Orch-->>Client: return skip result
else Proceed
Orch->>CI: check_mr_pipeline(mr_iid)
CI->>Client: fetch pipelines & jobs
CI-->>Orch: PipelineInfo (status, blocking reason)
alt CI blocking
Orch-->>Client: return blocked result
else Proceed
Orch->>Gatherer: gather()
Gatherer->>Client: fetch MR changes, commits, notes
Gatherer-->>Orch: MRContext (files, diffs, ai_comments)
Orch->>Engine: review(context)
Engine-->>Result: findings + verdict
Orch->>Bot: mark_reviewed(mr_iid, commit_sha)
Orch-->>Client: return review + CI summary
end
end
sequenceDiagram
participant CLI as CLI/Runner
participant Proc as AutoFixProcessor
participant Checker as PermissionChecker
participant State as AutoFixState
participant FileStore as Disk (JSON)
CLI->>Proc: process_issue(issue_iid, trigger_label)
Proc->>State: load or create AutoFixState
State->>FileStore: load_async()
FileStore-->>State: existing state or new
alt State exists and not terminal
Proc-->>CLI: return existing state
else New/resumable state
Proc->>Checker: verify_automation_trigger(issue_iid, label)
Checker-->>Proc: PermissionCheckResult
alt Permission denied
Proc->>State: update_status(FAILED)
State->>FileStore: save()
Proc-->>CLI: PermissionError
else Allowed
Proc->>State: update_status(ANALYZING)
State->>FileStore: save()
Proc->>Proc: construct issue URL
Proc->>State: update_status(CREATING_SPEC)
State->>FileStore: save()
Proc-->>CLI: return state
end
end
sequenceDiagram
participant Runner as CLI Runner
participant Batcher as GitlabIssueBatcher
participant Analyzer as ClaudeGitlabBatchAnalyzer
participant Claude as Claude AI
participant FileStore as Disk (JSON)
Runner->>Batcher: create_batches(issues)
Batcher->>Analyzer: analyze_and_batch_issues(issues)
Analyzer->>Claude: build prompt from all issues, call AI
Claude-->>Analyzer: batch suggestions (JSON)
Analyzer-->>Batcher: list of batch dicts
loop For each batch
Batcher->>Batcher: assign batch ID, create GitlabIssueBatch
Batcher->>FileStore: save_batch()
end
Batcher-->>Runner: list[GitlabIssueBatch]
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
StillKnotKnown
added
feature
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ci_status = None | ||
| ci_pipeline_id = None | ||
| try: | ||
| pipeline = await self.client.get_mr_pipeline_async(self.mr_iid) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Summary of ChangesHello @StillKnotKnown, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a comprehensive upgrade to the GitLab integration, bringing it to full feature parity with the existing GitHub integration. It significantly expands the range of automated actions available, from detailed merge request and branch management to advanced file operations and webhook control. The core purpose is to empower users with a more robust and intelligent automation toolkit within GitLab, leveraging AI for tasks like issue resolution, code review, and project insights, thereby streamlining development workflows and enhancing overall productivity. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive GitLab integration, bringing feature parity with GitHub. The changes are extensive, covering core API extensions, new service modules for auto-fix, batching, permissions, CI checking, context gathering, and prompt management. The new functionalities are well-tested with dedicated unit and integration tests, demonstrating a strong focus on correctness, robustness, and maintainability. The GitLab client has been significantly enhanced with improved error handling, retry logic, and asynchronous capabilities. Overall, this is a substantial and well-executed set of changes.
| wait_time = max( | ||
| 0, | ||
| ( | ||
| retry_date - datetime.now(timezone.utc) |
Contributor
There was a problem hiding this comment.
The parsedate_to_datetime function is used here but does not appear to be imported. Please add from email.utils import parsedate_to_datetime to the imports section of this file.
| elif name_lower in [ | ||
| n.lower() for n in CONFIG_FILE_NAMES | ||
| ] or name_lower.endswith((".config.js", ".config.ts", "rc", "rc.json")): | ||
| config_files.append(f) |
Contributor
There was a problem hiding this comment.
The pattern rc in name_lower.endswith((".config.js", ".config.ts", "rc", "rc.json")) might be too broad and could lead to unintended matches (e.g., a file named myrc). Consider making this more specific, perhaps by explicitly listing common RC files like .eslintrc, .prettierrc, etc., or by ensuring it's always prefixed with a dot if that's the intention.
- Add get_mr_pipeline() to get the latest pipeline for an MR - Add get_mr_pipeline_async() - async version of get_mr_pipeline - Add get_mr_notes_async() - async version of get_mr_notes - Add get_pipeline_jobs_async() - async version of get_pipeline_jobs These methods were being called by context_gatherer.py and ci_checker.py but were not implemented, which would cause AttributeError at runtime. Fixes: AndyMik90#1413 (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: 100
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/check_encoding.py (3)
17-23: Avoid direct platform checks per coding guidelines.Line 17 checks
sys.platform == "win32"directly. Per the coding guidelines, platform detection should use the platform abstraction module rather than checking platform identifiers directly.Suggested approach
Import and use the platform abstraction module's detection function instead of checking
sys.platformdirectly:-if sys.platform == "win32": +from your_platform_module import is_windows + +if is_windows():Based on coding guidelines: "Do not check
process.platformdirectly in code - always import platform detection functions from the platform abstraction module."
169-179: Inconsistent handling of nested parentheses in json.load pattern.The
json.loadpattern still uses[^)]+which cannot handle nested parentheses, while theopen()detection above was updated to track parenthesis depth. This creates inconsistency and will fail on cases likejson.load(open(get_path())).Consider extracting the parenthesis-tracking logic into a helper function and reusing it for all checks to ensure consistent behavior.
181-192: Same inconsistency in json.dump pattern.The
json.dumppattern at line 182 has the same issue asjson.load- it uses[^)]+instead of proper parenthesis depth tracking, causing it to fail on nested calls.
🤖 Fix all issues with AI agents
In `@apps/backend/__tests__/fixtures/gitlab.py`:
- Around line 177-182: The fixture uses a PAT-shaped token value in
MOCK_GITLAB_CONFIG which can trigger secret scanners; update the "token" value
in MOCK_GITLAB_CONFIG to a clearly fake placeholder that won't match PAT
patterns (e.g., "gitlab-test-token" or "TEST_GITLAB_TOKEN") so tests remain
meaningful without emitting token-shaped secrets; change only the token string
in the MOCK_GITLAB_CONFIG dict.
- Around line 213-218: mock_pipeline_jobs currently does a shallow copy of
SAMPLE_PIPELINE_JOBS so updating data[0] mutates the global fixture; change the
function to clone the inner dicts (e.g., use copy.deepcopy(SAMPLE_PIPELINE_JOBS)
or build a new list via [dict(item) for item in SAMPLE_PIPELINE_JOBS]) before
applying overrides so SAMPLE_PIPELINE_JOBS remains untouched when
mock_pipeline_jobs updates data[0].
In `@apps/backend/__tests__/test_gitlab_autofix_processor.py`:
- Around line 243-251: The side_effect function make_permission_result is
synchronous but is used with mock_permission_checker.verify_automation_trigger
which is an AsyncMock; change make_permission_result to be async (async def
make_permission_result(issue_iid, trigger_label): ...) and return the same
MagicMock instances so the AsyncMock awaits the coroutine correctly, or
alternatively replace side_effect with an AsyncMock that returns the appropriate
MagicMock per input; update references to make_permission_result accordingly.
- Around line 13-20: The fallback imports are inconsistent: in the except block
replace the lone "from models import AutoFixState, AutoFixStatus,
GitLabRunnerConfig" with the matching package path used above so all imports
come from runners.gitlab; ensure AutoFixState, AutoFixStatus, and
GitLabRunnerConfig are imported from runners.gitlab.models while
AutoFixProcessor and GitLabPermissionChecker remain imported from
runners.gitlab.autofix_processor and runners.gitlab.permissions respectively so
the except block mirrors the try block's module paths.
- Around line 305-324: The test function test_state_save_and_load uses
asyncio.run() to call the async AutoFixState.save method; change the test to an
async test (async def test_state_save_and_load(...)) and replace
asyncio.run(state.save(tmp_gitlab_dir)) with await state.save(tmp_gitlab_dir),
adding the appropriate pytest-asyncio marker or ensuring the test runner
supports async tests; keep the rest of the assertions and the call to
AutoFixState.load unchanged.
- Around line 119-121: Replace inline uses of __import__("datetime") in the test
module with top-level imports: add "from datetime import datetime, timedelta,
timezone" to the imports in
apps/backend/__tests__/test_gitlab_autofix_processor.py and update any code that
currently does __import__("datetime").datetime, .timedelta or .timezone to use
the directly imported names (datetime, timedelta, timezone) instead.
In `@apps/backend/__tests__/test_gitlab_batch_issues.py`:
- Around line 13-32: The current try/except duplicates imports for
ClaudeGitlabBatchAnalyzer, GitlabBatchStatus, GitlabIssueBatch,
GitlabIssueBatcher, GitlabIssueBatchItem, and format_batch_summary; keep those
common imports once and only wrap the GitLabConfig import in the try/except.
Specifically, import the six batch-related symbols unconditionally from
runners.gitlab.batch_issues (ClaudeGitlabBatchAnalyzer, GitlabBatchStatus,
GitlabIssueBatch, GitlabIssueBatcher, GitlabIssueBatchItem,
format_batch_summary), then use a try/except solely around importing
GitLabConfig (first attempt from runners.gitlab.glab_client, fallback to
glab_client) to eliminate the redundant branch.
- Around line 315-348: The test_list_batches is flaky because it relies on
implicit timestamps; modify the test to set explicit created_at values on the
two GitlabIssueBatch instances so ordering is deterministic: when constructing
batch1 and batch2 (GitlabIssueBatch in test_list_batches) set created_at such
that batch2 has a later timestamp than batch1, then save them with
batcher.save_batch and assert list_batches() returns batch2 before batch1; this
targets the GitlabIssueBatch created_at field and the
GitlabIssueBatcher.list_batches/save_batch behavior.
In `@apps/backend/__tests__/test_gitlab_bot_detection.py`:
- Around line 119-121: The test uses unnecessary __import__("datetime") when
datetime is already imported; update the expressions that construct timedeltas
(e.g., the assignment to detector.state.last_review_times["123"] and the similar
occurrences around lines 176-178) to use datetime.timedelta (or import timedelta
directly and use timedelta) instead of __import__("datetime").timedelta so the
code is simpler and consistent with the existing imports.
In `@apps/backend/__tests__/test_gitlab_branch_operations.py`:
- Around line 100-106: The async tests (e.g., test_list_branches_async) are
patching client._fetch but the code under test awaits client._fetch_async;
change the tests to patch client._fetch_async using an AsyncMock and set its
return_value to sample_branches so the async path is mocked (e.g.,
patch.object(client, "_fetch_async",
new=AsyncMock(return_value=sample_branches))). Apply the same change to the
other async tests noted (around the other ranges) so they mock _fetch_async
instead of _fetch.
In `@apps/backend/__tests__/test_gitlab_ci_checker.py`:
- Around line 46-83: Replace the asyncio.run wrapping with pytest-asyncio
fixtures by making the test coroutine itself async and decorating it with
`@pytest.mark.asyncio`; specifically change test_check_mr_pipeline_success to be
"async def" (instead of defining inner async test + asyncio.run) and add
`@pytest.mark.asyncio` above it, keeping the existing async mocks and patch.object
calls targeting checker.client.get_mr_pipelines_async,
get_pipeline_status_async, and get_pipeline_jobs_async, then await
checker.check_mr_pipeline(123) directly; apply the same refactor to
test_check_mr_pipeline_failed and test_check_mr_pipeline_no_pipeline so all
async tests are top-level async test functions using pytest.mark.asyncio.
- Around line 8-18: Remove the unused imports datetime, timezone, Path,
MagicMock, and Mock from the import block in the test file; leave only the
imports actually used by the tests (e.g., pytest, patch if used, and the
fixtures MOCK_GITLAB_CONFIG, mock_mr_data, mock_pipeline_data,
mock_pipeline_jobs) so the top-of-file import list matches what functions and
fixtures in this test reference.
In `@apps/backend/__tests__/test_gitlab_context_gatherer.py`:
- Around line 15-26: The test contains a non-functional fallback import: remove
the except block that imports from runners.gitlab.context_gatherer because that
module does not exist; keep the direct import from
runners.gitlab.services.context_gatherer and update the import statement to only
import CONFIG_FILE_NAMES, GITLAB_AI_BOT_PATTERNS, and MRContextGatherer from
runners.gitlab.services.context_gatherer so the test reliably imports the
correct symbols.
- Around line 348-364: The test function test_gather_handles_missing_ci is
defined at module level but uses self, so move/indent this async test method
into the TestGatherIntegration class (keeping the `@pytest.mark.asyncio` decorator
and the signature async def test_gather_handles_missing_ci(self, gatherer,
mock_client, sample_mr_data, sample_changes_data, sample_commits): intact) so
self is bound; ensure it is properly indented with the other tests in
TestGatherIntegration and that no duplicate names or fixtures are broken by the
repositioning.
In `@apps/backend/__tests__/test_gitlab_file_lock.py`:
- Around line 21-63: The tests assert the wrong path and call a non-existent
acquire() — update assertions to check the actual lock file created by FileLock
(it appends ".lock" to the given filename) by using lock_path =
lock_file.with_name(lock_file.name + ".lock") and assert on lock_path.exists() /
not lock_path.exists() in test_acquire_lock and test_lock_release; in
test_lock_timeout replace the direct .acquire() call with the context-manager
entry to trigger timeout, i.e. wrap "with FileLock(lock_file, timeout=0.1):"
inside pytest.raises(FileLockTimeout) so the __enter__ raises while another
thread holds the lock (keep references to FileLock and FileLockTimeout).
- Around line 167-298: Tests are incorrectly treating async helpers as sync;
update tests to use the async API: mark affected tests with
`@pytest.mark.asyncio`, change calls to await locked_json_write(...), result =
await locked_json_read(...), await locked_json_update(...), and use async with
locked_write(...) / async with locked_read(...); also adjust the missing-file
expectation to await locked_json_read(...) and assert None, and remove the
invalid lock=None arg from locked_write(...) or supply a proper FileLock
instance; alternatively, if you prefer sync semantics, add synchronous wrapper
functions in runners.gitlab.utils.file_lock (e.g.,
sync_locked_write/sync_locked_read/sync_locked_json_write/sync_locked_json_read/sync_locked_json_update)
that internally run the async implementations via asyncio.run or context-managed
event loops and update tests to call those sync wrappers.
In `@apps/backend/__tests__/test_gitlab_followup_reviewer.py`:
- Around line 72-81: The reviewer fixture instantiates FollowupReviewer with
hardcoded "/tmp/project" paths; change the fixture to accept pytest's tmp_path
and use tmp_path / "project" and tmp_path / "project" / ".auto-claude" /
"gitlab" (or .joinpath) so the paths are OS-safe and isolated; update the
fixture signature to def reviewer(tmp_path, sample_previous_review): and pass
project_dir=str(tmp_path / "project") and gitlab_dir=str(tmp_path / "project" /
".auto-claude" / "gitlab") when calling FollowupReviewer.
- Around line 11-19: The import list in the test imports unused symbols
AutoFixState and AutoFixStatus; remove AutoFixState and AutoFixStatus from the
import tuple in the runners.gitlab.models import statement so only used symbols
(MergeVerdict, MRReviewFinding, MRReviewResult, ReviewCategory, ReviewSeverity)
remain, and run tests to confirm no other references to
AutoFixState/AutoFixStatus exist in the file (test_gitlab_followup_reviewer.py).
- Around line 366-378: The test references a non-existent enum member
ReviewCategory.DOCUMENTATION; update the test to use the actual enum value
(likely ReviewCategory.DOCS) wherever ReviewCategory.DOCUMENTATION appears
(e.g., in the MRReviewFinding instantiation) and rerun tests to confirm; check
the ReviewCategory definition to pick the correct member name and replace
ReviewCategory.DOCUMENTATION in this file and any similar tests such as
test_gitlab_mr_review.py, ensuring reviewer._is_finding_addressed and
MRReviewFinding usage remain unchanged.
In `@apps/backend/__tests__/test_gitlab_mr_e2e.py`:
- Around line 523-531: The test incorrectly sets gatherer_class =
MRContextGatherer.__class__ (which yields the metaclass) causing an
AttributeError; change it to reference the actual class or instance (e.g., use
MRContextGatherer directly or instantiate it) so you call the real
_parse_ai_comment on MRContextGatherer (or an MRContextGatherer instance) with
the same arguments; update the gatherer_class assignment and subsequent call to
_parse_ai_comment accordingly.
- Around line 14-21: Replace the incorrect absolute import "from
tests.fixtures.gitlab import ..." with a relative import "from .fixtures.gitlab
import ..." in the test modules that reference the fixtures; locate occurrences
of that import line in test files such as test_gitlab_mr_e2e.py,
test_gitlab_provider.py, test_gitlab_mr_review.py, test_gitlab_ci_checker.py and
test_gitlab_bot_detection.py and update them to import MOCK_GITLAB_CONFIG,
mock_mr_changes, mock_mr_commits, mock_mr_data, mock_pipeline_data, and
mock_pipeline_jobs from .fixtures.gitlab so the tests correctly reference the
local fixtures module.
In `@apps/backend/__tests__/test_gitlab_mr_review.py`:
- Around line 14-18: Remove the unused fixture imports by deleting the three
names MOCK_GITLAB_CONFIG, mock_issue_data, and mock_mr_data from the import
statement in the test file; update the import from tests.fixtures.gitlab to only
import the fixtures actually used in this file (leave any other needed names
intact) so that the import line no longer contains MOCK_GITLAB_CONFIG,
mock_issue_data, or mock_mr_data.
- Around line 8-11: Remove the unused imports from the test module: delete the
top-level imports for json, datetime, timezone, Path, Mock, and patch while
keeping any mocks actually used (e.g., retain MagicMock if referenced
elsewhere). Locate the import statements at the top of the file (the lines with
"import json", "from datetime import datetime, timezone", "from pathlib import
Path", and "from unittest.mock import MagicMock, Mock, patch") and remove the
unused names so only required imports remain.
In `@apps/backend/__tests__/test_gitlab_permissions.py`:
- Around line 322-333: The test test_log_permission_denial currently calls
permission_checker.log_permission_denial but has no assertions; update it to
capture and assert log output using caplog: set caplog.set_level(logging.INFO)
(or the correct level), call
permission_checker.log_permission_denial(action="auto-fix", username="bob",
role=GitLabRole.REPORTER, issue_iid=123), and assert caplog.records or
caplog.text contains the expected pieces (e.g., "auto-fix", "bob", "REPORTER" or
GitLabRole.REPORTER.name, and "123") to verify the log includes full context
from log_permission_denial.
- Around line 11-16: The test imports are shadowing Python's built-in
PermissionError; update the import line that currently brings in PermissionError
from runners.gitlab.permissions to import it as GitLabPermissionError (e.g.,
change the imported symbol name for the exported error), and then update all
usages in this test file to reference GitLabPermissionError instead of
PermissionError; ensure references to GitLabPermissionChecker, GitLabRole, and
PermissionCheckResult remain unchanged.
- Around line 154-177: The mock for test_get_user_role_owner_via_namespace is
missing the separate namespace response expected by
permission_checker.get_user_role; update
mock_glab_client._fetch_async.side_effect to include an additional response for
the namespace lookup (e.g. a dict with "id" and "owner_id") between the project
info and the user info so the namespace owner_id is returned from the dedicated
/namespaces/{full_path} call rather than nested inside the project response.
- Around line 76-104: The test only provides one mocked return for
mock_glab_client._fetch_async but check_label_adder calls the API twice (label
events then get_user_role -> get_project_members); update the test to use
mock_glab_client._fetch_async.side_effect to return two sequential responses:
first the list of label event dicts (as already provided) and then a list of
project member dicts matching get_project_members format (include a member entry
for "alice" with an appropriate access_level) so get_user_role can map it to a
GitLabRole; adjust the assertions if needed to reflect the role produced by that
access_level.
In `@apps/backend/__tests__/test_gitlab_provider.py`:
- Around line 123-139: The test_post_review doesn't assert that the MR approval
path is invoked; when ReviewData.event == "approve" provider.post_review should
call provider._glab_client.approve_mr in addition to post_mr_note. Update the
test_post_review to assert that provider._glab_client.approve_mr was called once
with the MR id (e.g., 123) and any expected args/kwargs, while keeping the
existing assertion for post_mr_note (referencing test_post_review,
provider.post_review, and _glab_client.approve_mr/_glab_client.post_mr_note).
- Around line 149-157: The test test_close_pr calls provider.close_pr with a
comment but never asserts that the provider's GitLab client posted that comment;
update the test to assert that provider._glab_client.post_mr_note was called
with the MR id and the comment before/when close_pr returns (e.g., assert
provider._glab_client.post_mr_note was called once with 123 and "Closing as not
needed"), while keeping the existing final assert result is True.
- Around line 8-11: Remove the unused imports to clean up the test file: delete
json, datetime, timezone, and Mock from the import block and keep only the used
symbols (e.g., Path, MagicMock, patch) so the import line reads only the
necessary names; ensure no other parts of the file reference json, datetime,
timezone, or Mock before removing.
- Around line 230-236: The function await_if_needed currently imports asyncio
inside the function on every call; move the import to the module level (top of
the file) and remove the inline import from await_if_needed so the function
simply uses asyncio.run for awaitable inputs. Update the module imports to
include "import asyncio" and keep the helper logic in await_if_needed (check
hasattr(..., "__await__") and return asyncio.run(...) or the original result).
- Around line 14-19: The test import path is wrong: update the import in
apps/backend/__tests__/test_gitlab_provider.py to import the fixtures from the
package namespace used in this repo by changing the module path from
tests.fixtures.gitlab to __tests__.fixtures.gitlab so that MOCK_GITLAB_CONFIG,
mock_issue_data, mock_mr_data, and mock_pipeline_data are imported from
__tests__.fixtures.gitlab and the ModuleNotFoundError is resolved.
In `@apps/backend/__tests__/test_gitlab_rate_limiter.py`:
- Around line 244-263: The test_concurrent_requests has a race on modifying the
shared results list; replace results with a thread-safe collection (e.g.,
queue.Queue) or protect appends with a threading.Lock inside make_request so
concurrent threads can record outcomes safely; update references in the test
(results, make_request, and threads) to push/pop from the thread-safe structure
and assert by draining or collecting all items after threads join to preserve
the existing assertion that all requests succeeded.
- Around line 121-130: The test test_token_bucket_get_available calls a
non-existent method get_available() on TokenBucket; change the calls to use the
actual method name available() on the TokenBucket instance (e.g., replace
bucket.get_available() with bucket.available()) so the test uses the implemented
API in TokenBucket.
- Around line 62-77: The timing in test_token_bucket_refill is flaky; make it
deterministic by either (A) replacing the real sleep with a controlled time
advance (preferred) — monkeypatch time.time (or refactor TokenBucket to accept a
time provider) and advance the virtual clock instead of time.sleep before
checking bucket.tokens, or (B) if you prefer minimal change, increase the sleep
margin (e.g., sleep 0.2–0.3s) and assert bucket.tokens >= 1 after calling
bucket.consume(10). Update the test referencing TokenBucket,
test_token_bucket_refill, bucket.consume, and bucket.tokens accordingly.
In `@apps/backend/__tests__/test_gitlab_triage_engine.py`:
- Around line 185-199: Remove the redundant test_triage_categories_exist test
which duplicates assertions in test_supported_categories; locate the test
function named test_triage_categories_exist in the test file and delete it (or
replace it with a unique assertion if you need different coverage), ensuring
TriageCategory enum checks remain covered by test_supported_categories so there
is no duplicate verification of TriageCategory.BUG, TriageCategory.FEATURE, etc.
- Around line 13-20: The fallback ImportError block uses inconsistent module
paths which will fail; make the except imports mirror the same package structure
as the try block (or consistently relative), e.g., import GitLabConfig,
TriageCategory, TriageResult, and TriageEngine from the same module prefix used
in the try block; update the except block so all imports reference the same
package style as the successful imports (ensure references to GitLabConfig,
TriageCategory, TriageResult, and TriageEngine are imported from matching module
paths).
- Around line 111-180: Rename the test class ResponseParserTests to follow
pytest discovery rules (e.g., TestResponseParser) so pytest will discover and
run its test methods; update the class definition name in the test file (the
class that contains test_parse_findings_valid_json,
test_parse_findings_with_duplicate, test_parse_findings_with_question,
test_parse_findings_markdown_only, and test_parse_findings_invalid_json) to the
new name and ensure there are no other references to the old class name.
In `@apps/backend/__tests__/test_gitlab_webhook_operations.py`:
- Around line 145-211: The four tests in TestCreateWebhook use an async pytest
marker but call the synchronous method create_webhook
(test_create_webhook_basic, test_create_webhook_with_events,
test_create_webhook_with_all_events) and the async test
(test_create_webhook_async) patches the wrong helper; fix by making the three
synchronous tests regular (remove `@pytest.mark.asyncio` and make them normal def)
and keep test_create_webhook_async async but patch client._fetch_async (not
client._fetch); reference create_webhook, create_webhook_async, client._fetch
and client._fetch_async when updating the tests.
- Around line 294-319: The three tests test_get_invalid_webhook_id,
test_create_webhook_invalid_url, and test_delete_nonexistent_webhook have an
async/sync mismatch: remove the `@pytest.mark.asyncio` decorators and change the
async test functions to regular synchronous defs so they run as normal pytest
tests; keep the patch.object(client, "_fetch") mocks and mock_fetch.side_effect
assignments the same and ensure you call client.get_webhook(0),
client.create_webhook(url="not-a-url"), and client.delete_webhook(999) directly
inside pytest.raises(Exception) without awaiting.
- Line 9: The test file imports AsyncMock but does not use it; update the tests
to replace or supplement the current patching of _fetch_async with AsyncMock so
the import is used: locate the test cases that patch or mock _fetch_async
(search for "_fetch_async" or uses of patch/patch.object in
apps/backend/__tests__/test_gitlab_webhook_operations.py) and change the patch
target to use AsyncMock for the async function behavior (or remove the unused
AsyncMock import if you intentionally prefer a different mock), ensuring
AsyncMock is applied when mocking asynchronous calls.
- Around line 110-140: test_get_existing_webhook and
test_get_nonexistent_webhook are synchronous but decorated with
`@pytest.mark.asyncio` and test_get_webhook_async patches client._fetch while
exercising get_webhook_async; fix by removing the `@pytest.mark.asyncio` decorator
from the two sync tests (or convert them to async and await client.get_webhook),
and change the async test to patch client._fetch_async (or patch
_fetch_async.side_effect and await the call) so that test_get_webhook_async uses
client._fetch_async and test_get_nonexistent_webhook uses the matching
_fetch/_fetch_async consistent with whether you make it sync or async; update
assertions to await results if you convert tests to async.
- Around line 216-263: The async tests in TestUpdateWebhook are patching the
synchronous _fetch instead of the async _fetch_async; update
test_update_webhook_async to patch client._fetch_async (use
new_callable=AsyncMock or an AsyncMock instance) and have the mock return the
expected dict so awaiting client.update_webhook_async works correctly, and
ensure AsyncMock is imported from unittest.mock; keep the synchronous tests
patching client._fetch unchanged.
- Around line 268-289: The async test currently patches client._fetch (sync)
causing an async/sync mismatch; change test_delete_webhook_async to patch
client._fetch_async with an AsyncMock that returns None and then await
client.delete_webhook_async(2); leave test_delete_webhook as-is patching
client._fetch (regular mock) and calling client.delete_webhook(1) so the
sync/async calls and mocks match (reference: test_delete_webhook,
test_delete_webhook_async, client.delete_webhook, client.delete_webhook_async,
_fetch, _fetch_async).
- Line 8: Remove the unused import "Path" from the test module: delete the "from
pathlib import Path" import statement (the unused symbol Path) in apps backend
tests so the module no longer imports Path that isn't referenced by any test
functions or fixtures (clean up the unused import to satisfy linters).
- Around line 73-104: Remove the unnecessary `@pytest.mark.asyncio` from
test_list_all_webhooks and test_list_webhooks_empty since they call the
synchronous client.list_webhooks() method; make them plain synchronous tests. In
test_list_webhooks_async, patch client._fetch_async (not client._fetch) because
list_webhooks_async calls _fetch_async in glab_client.py, and ensure the test
awaits client.list_webhooks_async() as it already does.
In `@apps/backend/__tests__/test_glab_client.py`:
- Around line 568-576: The test currently assumes requests.request received the
URL as the first positional argument (mock_request.call_args[0][0]); change the
assertion to robustly extract the URL from the recorded call by inspecting
mock_request.call_args (args, kwargs) and preferring kwargs['url'] (falling back
to args[0] only if kwargs has no 'url') before asserting "gitlab.custom.com" is
in the URL — update the assertion in the test that calls client.get_mr(123) to
use this args/kwargs extraction so it doesn't break if the call uses keyword
args.
- Around line 144-164: The test has an inline "import time" inside
test_retry_with_backoff; move the import to the module's top-level imports (add
"import time" with the other imports) and remove the inline import from inside
test_retry_with_backoff so mock_request and the client.get_mr call use the
top-level time module consistently.
- Around line 8-15: The top-of-file imports include unused names—remove json,
datetime, timezone, Path, AsyncMock, and MagicMock from the import list so only
used symbols remain (e.g., keep asyncio, Mock, patch, pytest, and the requests
exceptions); update the import statements (from datetime, pathlib, and
unittest.mock) to delete those unused entries or collapse imports into only the
needed names to satisfy linting.
- Around line 21-30: The fixture uses GitLabClient with incorrect constructor
args; update the client fixture to construct a GitLabConfig instance and pass a
project_dir Path to GitLabClient instead of token/project/instance_url keywords:
import GitLabClient and GitLabConfig, build config = GitLabConfig(token=...,
project=..., instance_url=...), and call
GitLabClient(project_dir=Path("/tmp/test_project"), config=config) (optionally
set default_timeout if needed) so it matches GitLabClient.__init__ signature.
In `@apps/backend/runners/__init__.py`:
- Around line 12-18: Remove the misleading commented import and commented
__all__ entry for roadmap_runner: delete the line "from .roadmap_runner import
main as run_roadmap # Temporarily disabled - missing module" and the commented
"# "run_roadmap", # Temporarily disabled" so only run_spec and "run_spec"
remain; if the roadmap import truly must be disabled, replace the commented code
with a one-line TODO comment stating the precise reason (e.g., "TODO: disabled
due to circular dependency with X" or "disabled for GitLab integration") or
document the rationale in the PR description instead, but do not leave dead
commented code in __init__.py.
In `@apps/backend/runners/gitlab/autofix_processor.py`:
- Around line 150-155: The except block sets state.status directly which
bypasses transition validation; change it to call
state.update_status(AutoFixStatus.FAILED) (or the appropriate update_status
method) to enforce validation, then set state.error = str(e) and await
state.save(self.gitlab_dir); if update_status can raise, wrap the update_status
call in its own try/except to still set error and save the state without masking
the original exception before re-raising.
- Around line 126-143: Existing state loaded earlier may be overwritten by
creating a fresh AutoFixState instance (AutoFixState(...)) which loses prior
history; instead detect and reuse the previously loaded state object (or merge
its history) before changing status: if an existing state was loaded and its
status is FAILED or COMPLETED, copy its history/events/timestamps into the new
state or preferably call
existing_state.update_status(AutoFixStatus.CREATING_SPEC) and await
existing_state.save(self.gitlab_dir) rather than constructing a new AutoFixState
and saving it, ensuring you preserve previous state's fields and logs when
transitioning.
In `@apps/backend/runners/gitlab/batch_issues.py`:
- Around line 492-498: The expression building the "Theme" line in the lines
list uses a terse chained or/if that is hard to read; replace it with an
explicit computation of theme (e.g., compute theme = batch.theme; if not theme
and batch.common_themes: theme = batch.common_themes[0]; theme = theme or 'N/A')
and then use f"Theme: {theme}" when constructing the lines list so the intent is
unambiguous; update the block that creates lines (the variable lines and
reference to batch.theme / batch.common_themes) accordingly.
- Around line 191-202: The code is mutating sys.path to import
ensure_claude_code_oauth_token which is fragile; remove the runtime sys.path
manipulation and local import block in the method and instead perform a guarded
module-level import (e.g., try: from apps.backend.core.auth import
ensure_claude_code_oauth_token except ImportError:
ensure_claude_code_oauth_token = None) so the method can check that symbol and
fall back to self._fallback_batches(issues) when it's not available; update
references in this function (the try/except around claude_agent_sdk and the call
to ensure_claude_code_oauth_token) to use the module-level symbol and eliminate
backend_path/sys.path insertion.
In `@apps/backend/runners/gitlab/bot_detection.py`:
- Around line 47-92: The BotDetectionState uses dict[int,...] but persists keys
as strings, causing type drift; update serialization to normalize keys: change
to_dict to convert reviewed_commits and last_review_times keys to str, and
change from_dict (and load) to convert keys back to int (e.g., iterate items and
cast keys with int(key)) before constructing the BotDetectionState so the
in-memory attributes reviewed_commits and last_review_times remain dict[int,
...]; adjust save/load/to_dict/from_dict accordingly (use the class
BotDetectionState and fields reviewed_commits/last_review_times to locate the
code).
In `@apps/backend/runners/gitlab/glab_client.py`:
- Around line 1137-1170: The async create_mr_async duplicates payload-building
logic found in the sync create_mr; extract that shared logic into a helper
(e.g., _build_mr_payload or similar) that accepts source_branch, target_branch,
title, description, assignee_ids, reviewer_ids, labels, remove_source_branch,
squash and returns the data dict, then call that helper from both create_mr and
create_mr_async before calling _fetch or _fetch_async; alternatively, adapt the
existing _async_method decorator to wrap the sync create_mr so create_mr_async
delegates to create_mr via the decorator — update references to create_mr_async,
create_mr, _fetch_async, _fetch, and _async_method accordingly.
- Around line 1002-1009: The update_webhook method accepts arbitrary kwargs and
forwards them to _fetch without validation; add explicit validation in
update_webhook by defining an allowed_fields set of permitted webhook parameters
(e.g., url, push_events, issues_events, merge_requests_events, token,
enable_ssl_verification, etc.), filter the incoming kwargs to only those keys
present in allowed_fields, and either raise a ValueError for unknown keys or log
and ignore them; also update the update_webhook docstring to list the accepted
parameter names and reference encode_project_path and _fetch in the comment so
reviewers can find the call sites.
- Around line 1023-1042: The _fetch_async coroutine uses the deprecated
asyncio.get_event_loop(); replace that call with asyncio.get_running_loop()
inside _fetch_async so the code obtains the current running loop before calling
loop.run_in_executor to invoke the synchronous _fetch; update the reference in
the lambda invocation to continue passing endpoint, method, data, params,
timeout to _fetch unchanged.
- Around line 40-60: The _async_method decorator is dead code in glab_client.py:
remove the entire _async_method definition to avoid unused helpers since the
file already implements _fetch_async and explicit async methods; search for and
delete the function named _async_method and its inner async_wrapper/runner
helpers, and run tests/lint to ensure no other references remain.
- Around line 662-668: CIChecker.check_mr_pipeline calls a missing asynchronous
helper; add an async counterpart to the existing synchronous get_pipeline_jobs
by implementing get_pipeline_jobs_async(self, pipeline_id: int) -> list[dict]
that mirrors get_pipeline_jobs but uses the async fetch path (use the same
encoded project logic with encode_project_path(self.config.project) and call
self._fetch_async(f"/projects/{encoded_project}/pipelines/{pipeline_id}/jobs",
params={"per_page": 100})). Place this new method near the other async helpers
(e.g., after get_pipeline_status_async) so CIChecker.check_mr_pipeline can call
client.get_pipeline_jobs_async(pipeline_id) successfully.
In `@apps/backend/runners/gitlab/orchestrator.py`:
- Around line 293-301: The conditional on pipeline_info.status is dead code (all
branches are pass); either delete the entire if/elif/else block or implement the
intended behavior: for example, continue normal review when pipeline_info.status
== "success", mark or annotate the review with CI failure metadata when
pipeline_info.status == "failed", and handle in-progress states
("running"/"pending") as a soft-review path. Locate the block referencing
pipeline_info.status in orchestrator.py and either remove it or replace each
branch with the appropriate actions (e.g., call the review continuation
function, add a CI-failure note/label, or enqueue a deferred review) so the
control flow has visible effects.
- Line 294: The conditional is comparing an enum to a string — replace the
string comparison with the enum constant (use PipelineStatus.SUCCESS from
ci_checker.py) when checking pipeline_info.status, or better yet remove this
conditional block entirely since it's dead code; locate the conditional that
reads like if pipeline_info.status == "success": in the orchestrator logic (the
pipeline_info status handling) and either change the RHS to
PipelineStatus.SUCCESS or delete the whole block per the earlier note.
- Around line 353-358: The code is making a redundant API call to
ci_checker.check_mr_pipeline(mr_iid) here; reuse the already-defined
pipeline_info variable (from the earlier call) instead of calling
check_mr_pipeline again, and only call
ci_checker.format_pipeline_summary(pipeline_info) when pipeline_info is truthy
to build ci_section and prepend it to full_summary using the existing variable
names (pipeline_info, ci_section, format_pipeline_summary, full_summary).
In `@apps/backend/runners/gitlab/permissions.py`:
- Around line 24-27: The top-of-file comment incorrectly lists GitLab numeric
access levels; update or replace that comment to match the real mapping used in
ACCESS_LEVELS (and used by the code): 50 = OWNER, 40 = MAINTAINER, 30 =
DEVELOPER, 20 = REPORTER, 10 = GUEST (or simply remove the incorrect line and
reference the ACCESS_LEVELS dict). Ensure the comment references the GitLabRole
Literal and ACCESS_LEVELS to avoid future drift.
- Around line 213-233: The code assumes members[0] is the requested user when
calling self.glab_client.get_project_members(query=username); instead, filter
the returned list for an exact username match before using access_level: iterate
over members from get_project_members(query=username) to find member where
member.get("username") == username (or other canonical identifier), then compute
access_level against self.ACCESS_LEVELS and set self._role_cache[username] only
for that matched member; if no exact match, fall back to treating the user as
GUEST or return None per existing semantics.
In `@apps/backend/runners/gitlab/providers/gitlab_provider.py`:
- Around line 128-180: The async method fetch_prs is calling the synchronous
_glab_client._fetch(), which blocks the event loop; replace that call with the
async counterpart and await it (i.e., use await
self._glab_client._fetch_async(endpoint, params=params)) and update any
surrounding code to handle the awaited result; apply the same systematic fix to
other async methods mentioned (fetch_pr_diff, post_review, merge_pr, close_pr,
fetch_issue, fetch_issues, create_issue, close_issue, add_comment and all
label/repo operations) by converting synchronous _glab_client._fetch() calls to
awaitable _fetch_async() calls and ensuring callers use await and are declared
async.
- Around line 11-12: Remove the unused import "urllib.request" from the top of
the module: keep "import urllib.parse" but delete the "import urllib.request"
line to eliminate the unused import; search for any references to urllib.request
in the file to confirm it's safe to remove and run linting/tests after the
change.
- Around line 379-389: The lookup embeds the username into the path string
(f"/users?username={username}") which can break for special characters; update
the assignee resolution in the assignees handling block to call
self._glab_client._fetch with a params dict (e.g. {"username": username}) or
otherwise URL-encode the username instead of interpolating it, preserving the
existing try/except and appending user_data[0]["id"] to assignee_ids and setting
data["assignee_ids"] when non-empty; target the code that builds assignee_ids
and calls self._glab_client._fetch to apply this change.
- Around line 743-750: The chained boolean expression that sets "status" using
change.get("new_file") and "added" or ... is hard to read; replace it with an
explicit if/elif/else block that checks change.get("new_file"),
change.get("deleted_file"), and change.get("renamed_file") in order and assigns
"added", "deleted", "renamed", or falls back to "modified" to preserve the same
semantics; update the assignment that currently builds the dict with the
"status" key so it uses this new variable (refer to the "status" assignment and
the "change" dict in this code).
- Around line 102-126: The async fetch_pr method is calling synchronous client
methods and blocking the event loop; change the calls in fetch_pr to await the
async client variants: replace self._glab_client.get_mr(number) with await
self._glab_client.get_mr_async(number) and
self._glab_client.get_mr_changes(number) with await
self._glab_client.get_mr_changes_async(number), keep the rest of the
diff-building logic the same, and return self._parse_mr_data(mr_data, diff,
changes_data) as before so the method remains non-blocking.
- Around line 451-465: The code currently swallows exceptions when posting MR or
issue notes (calling post_mr_note and _fetch) and returns 0 which is ambiguous;
modify the two except blocks to capture the exceptions (except Exception as e),
log the errors with context using the provider's logger (e.g.,
self._logger.exception or self._logger.error including the exception and a
message mentioning encode_project_path(self._repo) and issue_or_pr_number), and
change the final failure return value to None (or another sentinel) so callers
can distinguish a real 0 ID from a failure.
- Around line 241-246: The merge result check is wrong and exceptions are being
swallowed; update the logic after calling self._glab_client.merge_mr(pr_number,
squash=squash) to check result.get("state") == "merged" (not "status") to
determine success, and change the except block to catch Exception as e and log
the exception details (e.g. using self._logger.error or the provider's logger)
before returning False so failures are visible for debugging rather than
silently ignored.
In `@apps/backend/runners/gitlab/runner.py`:
- Around line 270-277: The labels argument passed to client.list_issues is
currently a comma-separated string (args.labels) but list_issues expects
list[str]; split args.labels on commas, trim whitespace and remove empty entries
(e.g., labels_list = [s.strip() for s in args.labels.split(",") if s.strip()])
and pass labels=labels_list when non-empty, otherwise pass labels=None; update
the call around list_issues to use this parsed labels list.
- Around line 514-516: Update the triage command shown in the batch print so it
matches the parser and doesn't print "None": replace the hardcoded "--label
{args.label}" with the actual parsed value "--labels" using args.labels (e.g.,
include "--labels <comma-separated>" only when args.labels is truthy, joining
list elements if necessary) in the print that references largest_issues; ensure
the printed flag name is "--labels" to match the triage parser and omit the
labels portion entirely when args.labels is empty or None.
In `@apps/backend/runners/gitlab/services/batch_processor.py`:
- Around line 151-157: The call GitlabIssueBatcher.save_batch(batch) is
incorrect because save_batch is an instance method (defined as def
save_batch(self, batch: GitlabIssueBatch)) — either instantiate
GitlabIssueBatcher and call instance.save_batch(batch) or change save_batch to a
`@classmethod/`@staticmethod and update its signature; apply the same change for
the other invocations of save_batch in this file (the calls around where batch
status is set and at the other two call sites mentioned) so all calls match the
method definition.
- Around line 41-60: The dynamic import in _report_progress uses
sys.modules["orchestrator"] which is brittle and likely wrong; replace this
ad-hoc logic by importing the ProgressCallback type at module level (or under
TYPE_CHECKING) and avoid runtime module lookup: add a top-level import for
ProgressCallback from the correct orchestrator module (e.g.,
runners.gitlab.orchestrator) or declare a Protocol/ABC for the callback type,
then annotate progress_callback accordingly and construct ProgressCallback
instances directly in _report_progress without dynamic sys.modules checks.
In `@apps/backend/runners/gitlab/services/ci_checker.py`:
- Around line 337-347: The status_emoji mapping in ci_checker.py omits
PipelineStatus.MANUAL; update the status_emoji dict (where it's defined as
status_emoji) to include PipelineStatus.MANUAL mapped to an appropriate emoji
(e.g., "👤" or "⏸️") so that the lookup for pipeline.status explicitly handles
MANUAL rather than relying on the fallback used by emoji =
status_emoji.get(pipeline.status, "⚪").
- Around line 415-441: The loop using the `elapsed` counter undercounts time
because it only increments by `check_interval` and ignores time spent in
`check_mr_pipeline()`, so replace the incremental `elapsed` with wall-clock
timing: capture a monotonic `start = time.monotonic()` before the loop and
compute `elapsed = time.monotonic() - start` each iteration to enforce
`timeout_seconds` correctly (update the log messages that reference `elapsed`
and remaining time accordingly); keep using `await
self.check_mr_pipeline(mr_iid)` and `await asyncio.sleep(check_interval)` but
base timeout checks on the monotonic elapsed value so the loop exits on real
elapsed time.
- Around line 68-71: The dataclass fields jobs, failed_jobs, blocked_jobs, and
security_issues currently use None as mutable defaults; change them to use
dataclasses.field(default_factory=list) (import field from dataclasses) so each
instance gets its own list instead of sharing a single mutable default—update
the dataclass definition in ci_checker.py to replace those None defaults with
field(default_factory=list) for JobStatus lists and for the list[dict]
security_issues.
- Around line 188-190: The code picks the oldest pipeline by using
pipelines[-1]; change it to select the newest pipeline with pipelines[0] and add
an explicit empty-list guard (e.g., check if not pipelines then
handle/return/raise) so latest_pipeline_data is only set when pipelines contains
elements; update references around latest_pipeline_data in ci_checker.py to rely
on the new selection logic.
In `@apps/backend/runners/gitlab/services/context_gatherer.py`:
- Around line 278-289: The try/except around client.get_mr_pipeline_async
silently swallows all errors; update the except block in the context_gatherer
(around get_mr_pipeline_async usage) to catch exceptions but log them instead of
pass — e.g., catch Exception as e and call safe_print or the module logger to
emit a clear message including e and context (mentioning
ci_status/ci_pipeline_id and mr_iid) so failures retrieving CI pipeline are
visible while preserving optional behavior.
- Around line 33-51: Apply the existing validation helpers in this GitLab
context_gatherer: when you extract source_branch and target_branch from MR data,
run _validate_git_ref(ref) on each and handle invalid refs (log and skip
processing or raise a safe error) before using them in commands; likewise, when
extracting new_path and old_path run _validate_file_path(path) and skip or
reject any invalid paths. Update any usage sites that assume these values (e.g.,
places that call git operations with source_branch/target_branch or
new_path/old_path) to only proceed when the corresponding validator returns True
to prevent unsafe inputs.
- Around line 946-962: The code misidentifies the current head SHA by using
commits[-1]; update the current head SHA assignment so it uses the newest-first
commit at commits[0] when commits is non-empty (adjust the current_sha
calculation that currently reads commits[-1].get("id") or commits[-1].get("sha",
"") to use commits[0] instead), ensuring consistency with how new_commits and
previous_sha are handled; keep the existing safe_print and empty-string default
behavior intact.
In `@apps/backend/runners/gitlab/services/followup_reviewer.py`:
- Around line 22-56: The file currently imports safe_print (from .io_utils or
services.io_utils) and then redefines a local safe_print function, shadowing the
import; remove the local redefinition of safe_print (the def safe_print(...)
near the bottom) so the module uses the imported safe_print implementation, and
update any local references to safe_print if needed to rely on the imported
function (keep the try/except import block as-is and delete the duplicate def to
eliminate redundancy).
- Around line 288-295: The code checks finding.category against a non-existent
enum member ReviewCategory.DOCUMENTATION which will raise AttributeError; update
the condition in the followup_reviewer logic to use the correct enum member
ReviewCategory.DOCS (e.g., change "elif finding.category ==
ReviewCategory.DOCUMENTATION:" to "elif finding.category ==
ReviewCategory.DOCS:") and keep the existing docstring/comment detection logic
intact so finding.category comparisons use valid ReviewCategory values.
In `@apps/backend/runners/gitlab/services/prompt_manager.py`:
- Around line 38-43: Wrap file reads in a small helper that catches OSError and
falls back to the default prompt to avoid crashes: implement a private helper
(e.g., _safe_read_prompt(path, fallback_callable)) that tries
Path.read_text(encoding="utf-8") inside a try/except OSError and returns the
fallback_callable() on error, then replace direct reads in get_mr_review_prompt,
get_mr_summarizer_prompt and the other prompt-reading methods to call this
helper and pass the existing default-returning methods (like
_get_default_mr_review_prompt) as the fallback.
- Around line 33-36: Implement get_review_pass_prompt to resolve a pass-specific
prompt instead of returning an empty string: look up a prompt for the given
ReviewPass (e.g., by pass.name or pass.id) from your prompt store/ filesystem or
configuration, and return it; if no pass-specific prompt exists, fall back to
the main MR prompt (or explicitly raise a descriptive error) so multi-pass
reviews never receive a blank instruction—change the body of
get_review_pass_prompt to perform the lookup/fallback using ReviewPass and the
MR prompt source.
In `@apps/backend/runners/gitlab/services/triage_engine.py`:
- Around line 137-142: The current duplicate detection computes overlap with
title_words, other_words and appends potential_dupes, which yields false
positives for very short titles; introduce a minimum word-count threshold (e.g.,
MIN_TITLE_WORDS = 3) and only compute overlap and append to potential_dupes when
both len(title_words) and len(other_words) are >= MIN_TITLE_WORDS; add the
MIN_TITLE_WORDS constant (or parameter) near the function and update the
conditional that computes overlap (using title_words, other_words, overlap,
potential_dupes) to guard by the minimum-word check before dividing.
- Around line 112-119: The except block in triage_engine.py currently swallows
all exceptions, prints a short message, and returns a default
TriageCategory.FEATURE with 0.0 confidence; update the error handling in the
function containing this except to (1) log the full exception and traceback
using the project's logger rather than print (include the exception object and
traceback), (2) return a distinct error/unknown category (e.g.,
TriageCategory.ERROR or TriageCategory.UNKNOWN) or propagate critical exceptions
instead of always defaulting to FEATURE, and (3) ensure the returned
TriageResult (constructed via TriageResult(...)) clearly reflects the error
state (non-zero confidence only if appropriate) so callers can distinguish true
FEATURE decisions from failure cases.
- Around line 42-60: The dynamic import logic in _report_progress is fragile;
replace it by importing ProgressCallback at module scope (or from a shared
utilities module) and remove the sys.modules/fallback logic inside
_report_progress; update this file's top-level imports to `from orchestrator
import ProgressCallback` (or point to the centralized module) and do the same
refactor in batch_processor.py and autofix_processor.py so all usages of
ProgressCallback reference the same imported symbol rather than performing
runtime resolution.
In `@apps/backend/runners/gitlab/types.py`:
- Around line 239-259: The GitLabWebhook TypedDict contains a duplicate key
"wiki_page_events" which should be removed to avoid overriding and confusing
type checkers; edit the GitLabWebhook definition to delete the redundant
"wiki_page_events" line so the TypedDict only declares that key once (keep the
single correct declaration alongside the other fields in the GitLabWebhook
class).
In `@apps/backend/runners/gitlab/utils/file_lock.py`:
- Around line 330-365: The async locked_read currently opens the file on the
event loop thread; change it to avoid blocking by offloading file I/O to a
thread (e.g., use asyncio.to_thread or loop.run_in_executor) or alternatively
add a separate locked_read_sync wrapper for synchronous use. Specifically, keep
the FileLock usage (FileLock(..., exclusive=False)) but acquire/release the lock
in async context as now and perform the actual open/read inside an executor
(call a helper that opens the file and returns the file contents or a
io.StringIO wrapper) so the context manager yields non-blocking data instead of
performing blocking open/read on the event loop; if you implement
locked_read_sync, expose the same locking behavior but perform open/read
synchronously there. Ensure you reference and use the existing locked_read name
(or create locked_read_sync) and the FileLock.__aenter__/__aexit__ calls when
adjusting behavior.
- Around line 152-173: The timeout loop uses time.time() which can be affected
by system clock changes; switch to time.monotonic() for measuring elapsed time
in the lock acquisition loop: initialize start_time = time.monotonic(), compute
elapsed = time.monotonic() - start_time, and keep the existing retry/timeout
behavior (including closing self._fd, setting self._fd = None, and raising
FileLockTimeout with self.filepath and self.timeout) while calling
_try_lock(self._fd, self.exclusive) in the loop.
- Around line 174-190: In _release_lock, stop unlinking the lock file to avoid
inode-replacement races: remove the block that checks self._lock_file.exists()
and calls self._lock_file.unlink(), leaving only the
_unlock(self._fd)/os.close(self._fd) cleanup and setting self._fd = None; keep
the lock file persistent so file-based locking (inode identity) remains valid
across processes (references: _release_lock, self._fd, self._lock_file).
In `@apps/backend/runners/gitlab/utils/rate_limiter.py`:
- Around line 40-60: The _async_method decorator is defined but never used;
either remove the unused function or mark it as intentionally reserved by adding
a clear TODO comment and docstring referencing its name _async_method so future
maintainers know it's deliberate; if you remove it, also delete any imports only
used by it (e.g., functools, asyncio) and update the module docstring, otherwise
add a one-line TODO above the _async_method definition explaining intended
future use and keep its current implementation.
- Around line 295-356: The singleton in get_instance is not thread-safe; add a
class-level threading.Lock (e.g., RateLimiter._init_lock) and use it to guard
instance creation in get_instance using double-checked locking: acquire the
lock, re-check if cls._instance is None, then construct RateLimiter and assign
cls._instance (ensuring __init__ can run once or remove reliance on
RateLimiter._initialized); release the lock and return cls._instance. Ensure the
new lock is initialized at class scope and that get_instance uses it to prevent
concurrent constructions.
- Around line 598-607: The sync_wrapper currently calls
asyncio.run(async_wrapper(...)) which creates a new loop and crashes if there's
an existing running loop; change sync_wrapper to first check for a running loop
(use asyncio.get_running_loop() inside a try/except): if no running loop, call
asyncio.run(async_wrapper(*args, **kwargs)); if a running loop exists, raise a
clear RuntimeError instructing the caller to await the decorated function (i.e.,
use the async_wrapper / await the rate_limited function) instead of calling the
sync wrapper. Update the error message in sync_wrapper to reference rate_limited
and async_wrapper so callers know to use the async form.
In `@scripts/check_encoding.py`:
- Around line 53-67: The parenthesis-depth scanner that extracts the full
open(...) call (the loop starting with "for match in re.finditer(...open\s*\()")
fails when parentheses appear inside string literals; update the while-loop that
advances end_pos (the paren_depth tracking) to detect and skip over quoted
string literals (both single and double quotes), honoring escape sequences, so
parentheses inside strings do not affect paren_depth and the resulting call
variable contains the full call; apply the same fix to the equivalent scanners
used for read_text() and write_text() checks (the analogous re.finditer loops
and their paren_depth loops).
Comment on lines
+177
to
+182
| # Mock GitLab config | ||
| MOCK_GITLAB_CONFIG = { | ||
| "token": "glpat-test-token-12345", | ||
| "project": "group/project", | ||
| "instance_url": "https://gitlab.example.com", | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid token-shaped secrets in fixtures.
The glpat-... pattern can trip secret scanners and leak into logs. Consider a clearly fake placeholder (e.g., gitlab-test-token) that won’t match PAT patterns.
🔧 Suggested tweak
- "token": "glpat-test-token-12345",
+ "token": "gitlab-test-token",🤖 Prompt for AI Agents
In `@apps/backend/__tests__/fixtures/gitlab.py` around lines 177 - 182, The
fixture uses a PAT-shaped token value in MOCK_GITLAB_CONFIG which can trigger
secret scanners; update the "token" value in MOCK_GITLAB_CONFIG to a clearly
fake placeholder that won't match PAT patterns (e.g., "gitlab-test-token" or
"TEST_GITLAB_TOKEN") so tests remain meaningful without emitting token-shaped
secrets; change only the token string in the MOCK_GITLAB_CONFIG dict.
Comment on lines
+213
to
+218
| def mock_pipeline_jobs(**overrides): | ||
| """Create mock pipeline jobs with optional overrides.""" | ||
| data = SAMPLE_PIPELINE_JOBS.copy() | ||
| if overrides: | ||
| data[0].update(overrides) | ||
| return data |
Contributor
There was a problem hiding this comment.
Fix shallow copy to prevent cross-test contamination.
list.copy() keeps the inner dicts shared; data[0].update() mutates the global fixture. Use a deep copy or copy each dict.
🐛 Suggested fix
+from copy import deepcopy
@@
def mock_pipeline_jobs(**overrides):
"""Create mock pipeline jobs with optional overrides."""
- data = SAMPLE_PIPELINE_JOBS.copy()
+ data = deepcopy(SAMPLE_PIPELINE_JOBS)
if overrides:
data[0].update(overrides)
return data📝 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 mock_pipeline_jobs(**overrides): | |
| """Create mock pipeline jobs with optional overrides.""" | |
| data = SAMPLE_PIPELINE_JOBS.copy() | |
| if overrides: | |
| data[0].update(overrides) | |
| return data | |
| from copy import deepcopy | |
| def mock_pipeline_jobs(**overrides): | |
| """Create mock pipeline jobs with optional overrides.""" | |
| data = deepcopy(SAMPLE_PIPELINE_JOBS) | |
| if overrides: | |
| data[0].update(overrides) | |
| return data |
🤖 Prompt for AI Agents
In `@apps/backend/__tests__/fixtures/gitlab.py` around lines 213 - 218,
mock_pipeline_jobs currently does a shallow copy of SAMPLE_PIPELINE_JOBS so
updating data[0] mutates the global fixture; change the function to clone the
inner dicts (e.g., use copy.deepcopy(SAMPLE_PIPELINE_JOBS) or build a new list
via [dict(item) for item in SAMPLE_PIPELINE_JOBS]) before applying overrides so
SAMPLE_PIPELINE_JOBS remains untouched when mock_pipeline_jobs updates data[0].
Comment on lines
+243
to
+251
| def make_permission_result(issue_iid, trigger_label): | ||
| if issue_iid == 1: | ||
| return MagicMock(allowed=True) | ||
| else: | ||
| return MagicMock(allowed=False, reason="Not authorized") | ||
|
|
||
| mock_permission_checker.verify_automation_trigger.side_effect = ( | ||
| make_permission_result | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Side effect function returns sync value for an AsyncMock.
verify_automation_trigger is configured as AsyncMock, but make_permission_result returns a sync MagicMock. When using side_effect with an AsyncMock, the function should be async or the mock should handle it correctly. This works because AsyncMock wraps the return value, but using AsyncMock(side_effect=...) with an async function would be more explicit.
🤖 Prompt for AI Agents
In `@apps/backend/__tests__/test_gitlab_autofix_processor.py` around lines 243 -
251, The side_effect function make_permission_result is synchronous but is used
with mock_permission_checker.verify_automation_trigger which is an AsyncMock;
change make_permission_result to be async (async def
make_permission_result(issue_iid, trigger_label): ...) and return the same
MagicMock instances so the AsyncMock awaits the coroutine correctly, or
alternatively replace side_effect with an AsyncMock that returns the appropriate
MagicMock per input; update references to make_permission_result accordingly.
Comment on lines
+13
to
+32
| try: | ||
| from runners.gitlab.batch_issues import ( | ||
| ClaudeGitlabBatchAnalyzer, | ||
| GitlabBatchStatus, | ||
| GitlabIssueBatch, | ||
| GitlabIssueBatcher, | ||
| GitlabIssueBatchItem, | ||
| format_batch_summary, | ||
| ) | ||
| from runners.gitlab.glab_client import GitLabConfig | ||
| except ImportError: | ||
| from glab_client import GitLabConfig | ||
| from runners.gitlab.batch_issues import ( | ||
| ClaudeGitlabBatchAnalyzer, | ||
| GitlabBatchStatus, | ||
| GitlabIssueBatch, | ||
| GitlabIssueBatcher, | ||
| GitlabIssueBatchItem, | ||
| format_batch_summary, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant import fallback pattern.
Both branches of the try/except import the same modules from runners.gitlab.batch_issues. The only difference is the GitLabConfig import source. This can be simplified:
Proposed fix
+from runners.gitlab.batch_issues import (
+ ClaudeGitlabBatchAnalyzer,
+ GitlabBatchStatus,
+ GitlabIssueBatch,
+ GitlabIssueBatcher,
+ GitlabIssueBatchItem,
+ format_batch_summary,
+)
+
try:
- from runners.gitlab.batch_issues import (
- ClaudeGitlabBatchAnalyzer,
- GitlabBatchStatus,
- GitlabIssueBatch,
- GitlabIssueBatcher,
- GitlabIssueBatchItem,
- format_batch_summary,
- )
from runners.gitlab.glab_client import GitLabConfig
except ImportError:
from glab_client import GitLabConfig
- from runners.gitlab.batch_issues import (
- ClaudeGitlabBatchAnalyzer,
- GitlabBatchStatus,
- GitlabIssueBatch,
- GitlabIssueBatcher,
- GitlabIssueBatchItem,
- format_batch_summary,
- )🤖 Prompt for AI Agents
In `@apps/backend/__tests__/test_gitlab_batch_issues.py` around lines 13 - 32, The
current try/except duplicates imports for ClaudeGitlabBatchAnalyzer,
GitlabBatchStatus, GitlabIssueBatch, GitlabIssueBatcher, GitlabIssueBatchItem,
and format_batch_summary; keep those common imports once and only wrap the
GitLabConfig import in the try/except. Specifically, import the six
batch-related symbols unconditionally from runners.gitlab.batch_issues
(ClaudeGitlabBatchAnalyzer, GitlabBatchStatus, GitlabIssueBatch,
GitlabIssueBatcher, GitlabIssueBatchItem, format_batch_summary), then use a
try/except solely around importing GitLabConfig (first attempt from
runners.gitlab.glab_client, fallback to glab_client) to eliminate the redundant
branch.
Comment on lines
+315
to
+348
| def test_list_batches(self, mock_config, tmp_path): | ||
| """Test listing all batches.""" | ||
| batcher = GitlabIssueBatcher( | ||
| gitlab_dir=tmp_path / ".auto-claude" / "gitlab", | ||
| project="namespace/project", | ||
| project_dir=tmp_path, | ||
| ) | ||
|
|
||
| # Create a couple of batches | ||
| batch1 = GitlabIssueBatch( | ||
| batch_id="batch-1", | ||
| project="namespace/project", | ||
| primary_issue=1, | ||
| issues=[], | ||
| status=GitlabBatchStatus.PENDING, | ||
| ) | ||
| batch2 = GitlabIssueBatch( | ||
| batch_id="batch-2", | ||
| project="namespace/project", | ||
| primary_issue=2, | ||
| issues=[], | ||
| status=GitlabBatchStatus.COMPLETED, | ||
| ) | ||
|
|
||
| batcher.save_batch(batch1) | ||
| batcher.save_batch(batch2) | ||
|
|
||
| # List | ||
| batches = batcher.list_batches() | ||
|
|
||
| assert len(batches) == 2 | ||
| # Should be sorted by created_at descending | ||
| assert batches[0].batch_id == "batch-2" | ||
| assert batches[1].batch_id == "batch-1" |
Contributor
There was a problem hiding this comment.
Potentially flaky test due to file timestamp ordering.
test_list_batches assumes batches are sorted by created_at descending and expects batch-2 before batch-1. However, both batches are saved in quick succession without explicit created_at values, so the ordering depends on filesystem timestamps or default datetime generation, which may not be deterministic.
Consider explicitly setting created_at values to ensure deterministic ordering:
Proposed fix
+from datetime import datetime, timezone
+
def test_list_batches(self, mock_config, tmp_path):
"""Test listing all batches."""
batcher = GitlabIssueBatcher(
gitlab_dir=tmp_path / ".auto-claude" / "gitlab",
project="namespace/project",
project_dir=tmp_path,
)
# Create a couple of batches
batch1 = GitlabIssueBatch(
batch_id="batch-1",
project="namespace/project",
primary_issue=1,
issues=[],
status=GitlabBatchStatus.PENDING,
+ created_at="2024-01-01T00:00:00Z",
)
batch2 = GitlabIssueBatch(
batch_id="batch-2",
project="namespace/project",
primary_issue=2,
issues=[],
status=GitlabBatchStatus.COMPLETED,
+ created_at="2024-01-02T00:00:00Z",
)🤖 Prompt for AI Agents
In `@apps/backend/__tests__/test_gitlab_batch_issues.py` around lines 315 - 348,
The test_list_batches is flaky because it relies on implicit timestamps; modify
the test to set explicit created_at values on the two GitlabIssueBatch instances
so ordering is deterministic: when constructing batch1 and batch2
(GitlabIssueBatch in test_list_batches) set created_at such that batch2 has a
later timestamp than batch1, then save them with batcher.save_batch and assert
list_batches() returns batch2 before batch1; this targets the GitlabIssueBatch
created_at field and the GitlabIssueBatcher.list_batches/save_batch behavior.
Comment on lines
+239
to
+259
| class GitLabWebhook(TypedDict, total=False): | ||
| """Webhook data.""" | ||
|
|
||
| id: int | ||
| url: str | ||
| project_id: int | ||
| push_events: bool | ||
| issues_events: bool | ||
| merge_request_events: bool | ||
| wiki_page_events: bool | ||
| deployment_events: bool | ||
| job_events: bool | ||
| pipeline_events: bool | ||
| releases_events: bool | ||
| tag_push_events: bool | ||
| note_events: bool | ||
| confidential_note_events: bool | ||
| wiki_page_events: bool | ||
| custom_webhook_url: str | ||
| enable_ssl_verification: bool | ||
|
|
Contributor
There was a problem hiding this comment.
Remove duplicate wiki_page_events entry.
The field appears twice in GitLabWebhook, which overrides the earlier definition and can confuse type checkers.
🧹 Suggested fix
class GitLabWebhook(TypedDict, total=False):
@@
wiki_page_events: bool
@@
- wiki_page_events: bool
custom_webhook_url: str🤖 Prompt for AI Agents
In `@apps/backend/runners/gitlab/types.py` around lines 239 - 259, The
GitLabWebhook TypedDict contains a duplicate key "wiki_page_events" which should
be removed to avoid overriding and confusing type checkers; edit the
GitLabWebhook definition to delete the redundant "wiki_page_events" line so the
TypedDict only declares that key once (keep the single correct declaration
alongside the other fields in the GitLabWebhook class).
Comment on lines
+152
to
+173
| # Try to acquire lock with timeout | ||
| start_time = time.time() | ||
|
|
||
| while True: | ||
| try: | ||
| # Non-blocking lock attempt | ||
| _try_lock(self._fd, self.exclusive) | ||
| return # Lock acquired | ||
| except (BlockingIOError, OSError): | ||
| # Lock held by another process | ||
| elapsed = time.time() - start_time | ||
| if elapsed >= self.timeout: | ||
| os.close(self._fd) | ||
| self._fd = None | ||
| raise FileLockTimeout( | ||
| f"Failed to acquire lock on {self.filepath} within " | ||
| f"{self.timeout}s" | ||
| ) | ||
|
|
||
| # Wait a bit before retrying | ||
| time.sleep(0.01) | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a monotonic clock for the timeout loop.
time.time() can move backwards/forwards with system clock changes; time.monotonic() is safer for elapsed timeouts.
🤖 Prompt for AI Agents
In `@apps/backend/runners/gitlab/utils/file_lock.py` around lines 152 - 173, The
timeout loop uses time.time() which can be affected by system clock changes;
switch to time.monotonic() for measuring elapsed time in the lock acquisition
loop: initialize start_time = time.monotonic(), compute elapsed =
time.monotonic() - start_time, and keep the existing retry/timeout behavior
(including closing self._fd, setting self._fd = None, and raising
FileLockTimeout with self.filepath and self.timeout) while calling
_try_lock(self._fd, self.exclusive) in the loop.
Comment on lines
+174
to
+190
| def _release_lock(self) -> None: | ||
| """Release the file lock.""" | ||
| if self._fd is not None: | ||
| try: | ||
| _unlock(self._fd) | ||
| os.close(self._fd) | ||
| except Exception: | ||
| pass # Best effort cleanup | ||
| finally: | ||
| self._fd = None | ||
|
|
||
| # Clean up lock file | ||
| if self._lock_file and self._lock_file.exists(): | ||
| try: | ||
| self._lock_file.unlink() | ||
| except Exception: | ||
| pass # Best effort cleanup |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
file locking inode replacement race cross-process exclusion unlink lock file
💡 Result:
Summary — problem
- POSIX locks (fcntl/flock) are associated with the open file / inode (and the process holding the fd), not the pathname. Replacing or unlinking a pathname can leave a different inode at that name while an existing descriptor still refers to the old inode, so a pathname-based “lock” can race with unlink/rename operations. [1][2][3]
Why races happen (concise)
- Locks are by-inode/advisory: two pathnames that refer to the same inode share the same advisory lock state; but creating a new file (unlink + create, or rename over) produces a new inode that has no locks. A process that looks up the pathname (open after replacement) may get the new inode and miss the other process’s lock — a classic inode-replacement race. Unlink/rename themselves ignore advisory locks. [1][2][3]
Practical consequences
- You cannot prevent another process from unlinking/renaming your pathname with advisory locks.
- Mandatory locking is available on some systems but is unreliable, filesystem-limited, and generally discouraged. [1][2]
How to avoid or mitigate the race (recommended patterns)
- Lock the open file descriptor (fcntl or flock) and operate via the fd (not by pathname). Open the file first, then acquire the lock on that fd; because fds reference a specific inode, the lock protects that inode while you use it. [2][5]
- If you must guard creation/replacement by pathname, use atomic filesystem primitives:
- create with O_CREAT|O_EXCL to avoid racing creations where appropriate; or
- prepare the new content in a temporary file and atomically rename(2) it over the target (rename is atomic), together with an agreed protocol so readers open by pathname and then obtain an fd-lock. But note: rename replaces the pathname atomically, so readers that open after the rename will see the new inode (no global lock across rename). [5]
- Use a separate lock object that isn’t replaced: e.g., a stable lockfile (a dedicated filename) where processes open and lock that file’s inode (and do not allow that lockfile to be unlinked while in use). Keep lockfile and data file separate so replacing the data file doesn’t remove the lock object. Use O_EXCL for creating lockfiles if you need “first-creator” semantics. [2][5]
- For robust inter-process coordination, prefer an explicit coordinator (daemon, socket, fcntl locks on a stable inode, or kernel mechanisms like file leases) rather than relying on pathname semantics alone. [5][2]
Short checklist
- Open the target (or a separate, long-lived lockfile) and lock the resulting fd via fcntl/flock. Operate on the fd. [2]
- Avoid relying on mandatory locks; they’re filesystem- and kernel-dependent and error-prone. [1][2]
- Use atomic rename and O_CREAT|O_EXCL where appropriate to avoid create/replace races. [5]
- If you must coordinate by pathname, design a protocol that prevents inode-replacement races (e.g., lock a stable lockfile that is never swapped). [2]
References
[1] Linux kernel docs — Mandatory File Locking (explanation and warnings). (kernel.org)
[2] Gavv — File locking in Linux (advisory vs mandatory, locks are inode-based; unlink/rename ignored). (gavv.net)
[3] StackOverflow discussion on fcntl/lockf and inode semantics. (stackoverflow.com)
[4] “Things UNIX can do atomically” (practical notes on fcntl, rename, races). (rcrowley.org)
If you want, tell me your exact use-case (language, filesystem, whether NFS is involved) and I’ll give a concrete code/protocol example.
Citations:
- 1: https://www.kernel.org/doc/html/v5.14/filesystems/mandatory-locking.html?utm_source=openai
- 2: https://gavv.net/articles/file-locks/?utm_source=openai
- 3: https://stackoverflow.com/questions/575328/fcntl-lockf-which-is-better-to-use-for-file-locking?utm_source=openai
- 4: https://rcrowley.org/2010/01/06/things-unix-can-do-atomically.html?utm_source=openai
Remove the lock file unlink operation; it breaks cross-process file locking via inode-replacement race.
Unlinking the lock file at line 185 allows another process to create a new file at the same path with a different inode while the original process still holds a file descriptor to the old inode. This defeats mutual exclusion—new processes opening the pathname get the new inode without locks, while the prior process thinks it holds the lock on the old inode.
File-based locks work via inode identity, not pathname; replacing the inode invalidates all protections. Keep the lock file persistent across releases.
🔧 Proposed fix
- # Clean up lock file
- if self._lock_file and self._lock_file.exists():
- try:
- self._lock_file.unlink()
- except Exception:
- pass # Best effort cleanup
+ # Do not unlink the lock file; removing it allows concurrent locks
+ # on different inodes, breaking cross-process exclusion.📝 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 _release_lock(self) -> None: | |
| """Release the file lock.""" | |
| if self._fd is not None: | |
| try: | |
| _unlock(self._fd) | |
| os.close(self._fd) | |
| except Exception: | |
| pass # Best effort cleanup | |
| finally: | |
| self._fd = None | |
| # Clean up lock file | |
| if self._lock_file and self._lock_file.exists(): | |
| try: | |
| self._lock_file.unlink() | |
| except Exception: | |
| pass # Best effort cleanup | |
| def _release_lock(self) -> None: | |
| """Release the file lock.""" | |
| if self._fd is not None: | |
| try: | |
| _unlock(self._fd) | |
| os.close(self._fd) | |
| except Exception: | |
| pass # Best effort cleanup | |
| finally: | |
| self._fd = None | |
| # Do not unlink the lock file; removing it allows concurrent locks | |
| # on different inodes, breaking cross-process exclusion. |
🤖 Prompt for AI Agents
In `@apps/backend/runners/gitlab/utils/file_lock.py` around lines 174 - 190, In
_release_lock, stop unlinking the lock file to avoid inode-replacement races:
remove the block that checks self._lock_file.exists() and calls
self._lock_file.unlink(), leaving only the _unlock(self._fd)/os.close(self._fd)
cleanup and setting self._fd = None; keep the lock file persistent so file-based
locking (inode identity) remains valid across processes (references:
_release_lock, self._fd, self._lock_file).
Comment on lines
+330
to
+365
| @asynccontextmanager | ||
| async def locked_read(filepath: str | Path, timeout: float = 5.0) -> Any: | ||
| """ | ||
| Async context manager for locked file reading. | ||
| Acquires shared lock for reading, allowing multiple concurrent readers | ||
| but blocking writers. | ||
| Args: | ||
| filepath: File path to read | ||
| timeout: Lock timeout in seconds (default: 5.0) | ||
| Example: | ||
| async with locked_read("/path/to/file.json", timeout=5.0) as f: | ||
| data = json.load(f) | ||
| Raises: | ||
| FileLockTimeout: If lock cannot be acquired within timeout | ||
| FileNotFoundError: If file doesn't exist | ||
| """ | ||
| filepath = Path(filepath) | ||
|
|
||
| if not filepath.exists(): | ||
| raise FileNotFoundError(f"File not found: {filepath}") | ||
|
|
||
| # Acquire shared lock (allows multiple readers) | ||
| lock = FileLock(filepath, timeout=timeout, exclusive=False) | ||
| await lock.__aenter__() | ||
|
|
||
| try: | ||
| # Open file for reading | ||
| with open(filepath, encoding="utf-8") as f: | ||
| yield f | ||
| finally: | ||
| # Release lock | ||
| await lock.__aexit__(None, None, None) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid blocking file I/O in async context.
locked_read opens/reads files on the event loop thread. Consider offloading open/read to an executor or adding a sync locked_read_sync wrapper to prevent event‑loop stalls.
🤖 Prompt for AI Agents
In `@apps/backend/runners/gitlab/utils/file_lock.py` around lines 330 - 365, The
async locked_read currently opens the file on the event loop thread; change it
to avoid blocking by offloading file I/O to a thread (e.g., use
asyncio.to_thread or loop.run_in_executor) or alternatively add a separate
locked_read_sync wrapper for synchronous use. Specifically, keep the FileLock
usage (FileLock(..., exclusive=False)) but acquire/release the lock in async
context as now and perform the actual open/read inside an executor (call a
helper that opens the file and returns the file contents or a io.StringIO
wrapper) so the context manager yields non-blocking data instead of performing
blocking open/read on the event loop; if you implement locked_read_sync, expose
the same locking behavior but perform open/read synchronously there. Ensure you
reference and use the existing locked_read name (or create locked_read_sync) and
the FileLock.__aenter__/__aexit__ calls when adjusting behavior.
Comment on lines
+53
to
+67
| # We need to find the full open() call including nested parentheses | ||
| for match in re.finditer(r'(?<![a-zA-Z_\.])open\s*\(', content): | ||
| start_pos = match.end() | ||
|
|
||
| # Find the matching closing parenthesis (handle nesting) | ||
| paren_depth = 1 | ||
| end_pos = start_pos | ||
| while end_pos < len(content) and paren_depth > 0: | ||
| if content[end_pos] == '(': | ||
| paren_depth += 1 | ||
| elif content[end_pos] == ')': | ||
| paren_depth -= 1 | ||
| end_pos += 1 | ||
|
|
||
| call = content[match.start():end_pos] |
Contributor
There was a problem hiding this comment.
Parenthesis tracking doesn't handle string literals.
The new parenthesis-depth tracking is an improvement for handling nested calls like open(get_path()), but it doesn't account for parentheses inside string literals. For example, open("file).txt") would incorrectly match the ) inside the string, causing the extracted call to be truncated.
Suggested fix
Add logic to skip characters inside string literals:
# Find the matching closing parenthesis (handle nesting)
paren_depth = 1
end_pos = start_pos
+ in_string = None # None, or the quote character (' or ")
while end_pos < len(content) and paren_depth > 0:
+ char = content[end_pos]
+ # Handle string boundaries
+ if in_string is None and char in ('"', "'"):
+ in_string = char
+ elif in_string == char and (end_pos == 0 or content[end_pos - 1] != '\\'):
+ in_string = None
+ elif in_string is None:
+ if char == '(':
+ paren_depth += 1
+ elif char == ')':
+ paren_depth -= 1
- if content[end_pos] == '(':
- paren_depth += 1
- elif content[end_pos] == ')':
- paren_depth -= 1
end_pos += 1Note: The same issue exists in the read_text() and write_text() checks (lines 90-98 and 132-140).
🤖 Prompt for AI Agents
In `@scripts/check_encoding.py` around lines 53 - 67, The parenthesis-depth
scanner that extracts the full open(...) call (the loop starting with "for match
in re.finditer(...open\s*\()") fails when parentheses appear inside string
literals; update the while-loop that advances end_pos (the paren_depth tracking)
to detect and skip over quoted string literals (both single and double quotes),
honoring escape sequences, so parentheses inside strings do not affect
paren_depth and the resulting call variable contains the full call; apply the
same fix to the equivalent scanners used for read_text() and write_text() checks
(the analogous re.finditer loops and their paren_depth loops).
- Fix unnecessary __import__(datetime) calls in test_gitlab_bot_detection.py The datetime module is already imported, so use datetime.timedelta directly - Fix incorrect fixture import path in test_gitlab_provider.py Change from tests.fixtures.gitlab to __tests__.fixtures.gitlab These changes address CodeRabbit review comments on PR AndyMik90#1413.
…avior The encoding checker now detects 3 issues instead of 2 in the test_multiple_issues_in_single_file test: - 2 open() calls (one in comment, one actual) - 1 write_text() call Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Add io_utils.py module with safe_print function for subprocess output - Fix ReviewCategory.DOCUMENTATION -> DOCS in followup_reviewer.py - Fix doc pattern matching to include '+ """' variant - Fix test assertion to use lowercase 'todo' instead of uppercase - Fix test diff format to include proper unified diff headers Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com>
Comment on lines
+54
to
+56
| def safe_print(msg: str, **kwargs) -> None: | ||
| """Thread-safe print helper.""" | ||
| print(msg, **kwargs) |
There was a problem hiding this comment.
Bug: The local redefinition of safe_print shadows the imported version from io_utils, removing critical BrokenPipeError handling needed for subprocesses, which will cause a crash.
Severity: HIGH
Suggested Fix
Remove the local redefinition of the safe_print function on lines 54-56. This will ensure that the imported version from io_utils, which contains the necessary error handling for subprocess communication, is used instead.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/backend/runners/gitlab/services/followup_reviewer.py#L54-L56
Potential issue: The function `safe_print` is imported from `services/io_utils.py` but
is immediately redefined as a simple wrapper around `print()`. The original
implementation in `io_utils` is designed to run in a subprocess and gracefully handles
`BrokenPipeError` when the parent process closes the communication pipe. The local
redefinition lacks this error handling. When the parent process (e.g., an Electron app)
closes the pipe, the call to the redefined `safe_print` will raise an unhandled
`BrokenPipeError`, causing the subprocess to crash.
Did we get this right? 👍 / 👎 to inform future reviews.
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
This PR implements comprehensive GitLab integration with full feature parity to GitHub, completing all HIGH and MEDIUM priority recommendations from the code review.
Changes
Core API Extensions
create_mr,list_mrs(with filters),update_mrlist_branches,get_branch,create_branch,delete_branch,compare_branchesget_file_contents,create_file,update_file,delete_filelist_webhooks,get_webhook,create_webhook,update_webhook,delete_webhook_fetchmethod with query parameters support and retry logicNew Service Modules (ported from GitHub)
autofix_processor.py: Auto-fix workflow with permission verificationbatch_issues.py: Issue batching with Claude AI similarity detectionpermissions.py: GitLab permission checker for automation triggersservices/triage_engine.py: AI-driven issue triage and categorizationservices/followup_reviewer.py: Automated MR review for follow-up commitsservices/context_gatherer.py: Enhanced context gathering with monorepo supportservices/prompt_manager.py: System prompt managementservices/response_parsers.py: Parse Claude API responsesservices/batch_processor.py: Batch issue processingType Safety & Code Quality
types.py: TypedDict type definitions for API responsesConfiguration Fixes
utils/file_lock.py: Addedencodingparameter toatomic_writeutils/rate_limiter.py: Updated documentation from GitHub to GitLabTests
Test Plan
Checklist
developbranchSummary by CodeRabbit
Release Notes
New Features
triage,auto-fix, andbatch-issuesTests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.