Skip to content

Conversation

@evanscastonguay
Copy link
Contributor

@evanscastonguay evanscastonguay commented Jan 10, 2026

User description

Summary

  • Allow overriding the GitLab webhook server port via PORT env var
  • Add unit test for PORT override

Testing

  • python3 -m pytest tests/unittest/test_gitlab_webhook_port.py

PR Type

Enhancement, Tests


Description

  • Allow PORT environment variable override for GitLab webhook server

  • Default port remains 3000 if PORT env var not set

  • Add unit test verifying PORT override functionality


Diagram Walkthrough

flowchart LR
  A["PORT env var"] -- "read via os.environ.get" --> B["port variable"]
  B -- "default to 3000" --> C["uvicorn.run"]
  D["GITLAB_WEBHOOK"] -- "start function" --> C
Loading

File Walkthrough

Relevant files
Enhancement
gitlab_webhook.py
Add PORT environment variable support                                       

pr_agent/servers/gitlab_webhook.py

  • Import os module for environment variable access
  • Modify start() function to read PORT from environment
  • Use default port 3000 if PORT env var not provided
  • Pass dynamic port value to uvicorn.run()
+3/-1     
Tests
test_gitlab_webhook_port.py
Unit test for PORT environment override                                   

tests/unittest/test_gitlab_webhook_port.py

  • New test file for GitLab webhook PORT override functionality
  • Test verifies PORT env var is correctly passed to uvicorn.run
  • Test confirms host remains "0.0.0.0" when PORT is overridden
  • Uses module reload and mock patching for isolated testing
+22/-0   

@qodo-free-for-open-source-projects
Copy link
Contributor

qodo-free-for-open-source-projects bot commented Jan 10, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Unvalidated port input

Description: The PORT environment variable is converted to integer without validation, allowing
potential DoS via invalid port values (e.g., negative numbers, values > 65535) or
ValueError crash with non-numeric input.
gitlab_webhook.py [313-313]

Referred Code
port = int(os.environ.get("PORT", "3000"))
uvicorn.run(app, host="0.0.0.0", port=port)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Consistent Naming Conventions

Objective: All new variables, functions, and classes must follow the project's established naming
standards

Status: Passed

No Dead or Commented-Out Code

Objective: Keep the codebase clean by ensuring all submitted code is active and necessary

Status: Passed

Single Responsibility for Functions

Objective: Each function should have a single, well-defined responsibility

Status: Passed

When relevant, utilize early return

Objective: In a code snippet containing multiple logic conditions (such as 'if-else'), prefer an
early return on edge cases than deep nesting

Status: Passed

🔴
Robust Error Handling

Objective: Ensure potential errors and edge cases are anticipated and handled gracefully throughout
the code

Status:
Missing error handling: The PORT environment variable conversion to int lacks error handling for invalid
non-numeric values

Referred Code
port = int(os.environ.get("PORT", "3000"))
uvicorn.run(app, host="0.0.0.0", port=port)
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects
Copy link
Contributor

qodo-free-for-open-source-projects bot commented Jan 10, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle invalid port environment variable
Suggestion Impact:The suggestion was directly implemented in the commit. The exact code changes suggested were applied, including the try-except block around the port parsing, the ValueError exception handling, the warning log message, and the fallback to port 3000.

code diff:

+    try:
+        port = int(os.environ.get("PORT", "3000"))
+    except ValueError:
+        get_logger().warning("Invalid PORT environment variable, using default port 3000")
+        port = 3000

Add a try-except block to handle non-integer values for the PORT environment
variable, preventing a potential application crash and falling back to the
default port.

pr_agent/servers/gitlab_webhook.py [313]

-port = int(os.environ.get("PORT", "3000"))
+try:
+    port = int(os.environ.get("PORT", "3000"))
+except ValueError:
+    get_logger().warning("Invalid PORT environment variable, using default port 3000")
+    port = 3000

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential ValueError that would crash the application if the PORT environment variable is misconfigured, and proposes a robust solution with error handling and a fallback mechanism.

Medium
  • Update
  • Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

@evanscastonguay
Copy link
Contributor Author

/help

@qodo-free-for-open-source-projects
Copy link
Contributor

Qodo Merge Walkthrough 🤖

Welcome to Qodo Merge, an AI-powered tool for automated pull request analysis, feedback, suggestions and more.

Here is a list of tools you can use to interact with Qodo Merge:

ToolDescriptionTrigger Interactively 💎

DESCRIBE

Generates PR description - title, type, summary, code walkthrough and labels
  • Run

REVIEW

Adjustable feedback about the PR, possible issues, security concerns, review effort and more
  • Run

IMPROVE

Code suggestions for improving the PR
  • Run

UPDATE CHANGELOG

Automatically updating the CHANGELOG.md file with the PR changes
  • Run

ADD DOCS 💎

Generates documentation to methods/functions/classes that changed in the PR
  • Run

ANALYZE 💎

Identifies code components that changed in the PR, and enables to interactively generate tests, docs, and code suggestions for each component
  • Run

COMPLIANCE 💎

Comprehensive compliance checks for security, ticket requirements, and custom organizational rules
  • Run

GENERATE TESTS 💎

Generates unit tests for a specific component, based on the PR code change
  • Run

IMPROVE COMPONENT 💎

Code suggestions for a specific component that changed in the PR
  • Run

PR TO TICKET 💎

Generates ticket in the ticket tracking systems (Jira, Linear, or Git provider issues) based on PR content
  • Run

SCAN REPO DISCUSSIONS 💎

Generates best_practices.md file based on previous discussions in the repository
  • Run

ASK

Answering free-text questions about the PR

[*]

HELP DOCS

Answers a question regarding this repository, or a given one, based on given documentation path

[*]

CI FEEDBACK 💎

Generates feedback and analysis for a failed CI job

[*]

CUSTOM PROMPT 💎

Generates custom suggestions for improving the PR code, derived only from a specific guidelines prompt defined by the user

[*]

GENERATE CUSTOM LABELS 💎

Generates custom labels for the PR, based on specific guidelines defined by the user

[*]

IMPLEMENT 💎

Generates implementation code from review suggestions

[*]

SIMILAR CODE 💎

Retrieves the most similar code components from inside the organization's codebase, or from open-source code.

[*]

(1) Note that each tool can be triggered automatically when a new PR is opened, or called manually by commenting on a PR.

(2) Tools marked with [*] require additional parameters to be passed. For example, to invoke the /ask tool, you need to comment on a PR: /ask "<question content>". See the relevant documentation for each tool for more details.

@evanscastonguay
Copy link
Contributor Author

/ask could you check the pr and let me know if there is other change I could do make it merge

@qodo-free-for-open-source-projects
Copy link
Contributor

Ask

could you check the pr and let me know if there is other change I could do make it merge

Answer:

Great work on this PR! The implementation is solid and well-tested. Here are some suggestions to make it merge-ready:

Code Quality Improvements

  1. Add logging for successful port override: Currently, you only log when there's an invalid PORT value. Consider adding an info log when a custom port is successfully used:
def start():
    try:
        port = int(os.environ.get("PORT", "3000"))
        if os.environ.get("PORT"):
            get_logger().info(f"Using custom PORT from environment: {port}")
    except ValueError:
        get_logger().warning("Invalid PORT environment variable, using default port 3000")
        port = 3000
    uvicorn.run(app, host="0.0.0.0", port=port)
  1. Consider port range validation: Add validation to ensure the port is within valid range (1-65535):
try:
    port = int(os.environ.get("PORT", "3000"))
    if not (1 <= port <= 65535):
        raise ValueError(f"Port {port} is out of valid range")
except ValueError as e:
    get_logger().warning(f"Invalid PORT environment variable ({e}), using default port 3000")
    port = 3000

Test Improvements

  1. Add test for default port behavior: Add a test case that verifies the default port (3000) is used when PORT env var is not set:
def test_start_default_port(monkeypatch):
    monkeypatch.setenv("GITLAB__URL", "https://gitlab.example.com")
    monkeypatch.delenv("PORT", raising=False)
    module = _load_module()

    with mock.patch.object(module.uvicorn, "run") as mock_run:
        module.start()

    _, kwargs = mock_run.call_args
    assert kwargs["port"] == 3000
  1. Add test for port range validation (if you implement suggestion Add support for OpenAI organization in the secrets file #2)

Documentation

  1. Update README or documentation: If there's a README or documentation file explaining how to run the GitLab webhook server, add information about the PORT environment variable override capability.

  2. Add docstring to start() function: Document the PORT environment variable:

def start():
    """
    Start the GitLab webhook server.
    
    The server port can be configured via the PORT environment variable.
    Defaults to 3000 if PORT is not set or invalid.
    """

Minor Fixes

  1. Test cleanup: In your test file, consider using pytest.fixture for the module loading to avoid repetition and ensure proper cleanup.

Overall, this is a clean and focused PR. The main suggestions are around robustness (port validation), observability (logging), and completeness (additional test cases). These are optional improvements - the current implementation is functional and ready for review!

@evanscastonguay
Copy link
Contributor Author

/review

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
✅ No TODO sections
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Logger Import

The code calls get_logger() in the exception handler but there's no visible import for this function. Verify that get_logger is properly imported from the appropriate module.

get_logger().warning("Invalid PORT environment variable, using default port 3000")
port = 3000
Test Isolation Issue

The test uses importlib.reload() which may cause side effects and doesn't properly isolate the module state. Consider using dependency injection or mocking os.environ.get directly instead of reloading the entire module.

def _load_module():
    import pr_agent.servers.gitlab_webhook as gitlab_webhook

    return importlib.reload(gitlab_webhook)

@evanscastonguay
Copy link
Contributor Author

Final update:

  • Added PORT validation + logging + docstring
  • Documented PORT override in GitLab install docs
  • Improved tests (no reloads, added default/invalid/range cases)
  • Tests: python3 -m pytest tests/unittest/test_gitlab_webhook_port.py

Note: get_logger is already imported in pr_agent/servers/gitlab_webhook.py (false positive from the bot).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant