Skip to content

Fix domain override validation to accept valid URLs with protocols#112

Merged
juliomenendez merged 5 commits intomainfrom
copilot/fix-invalid-domain-parameter
Jan 13, 2026
Merged

Fix domain override validation to accept valid URLs with protocols#112
juliomenendez merged 5 commits intomainfrom
copilot/fix-invalid-domain-parameter

Conversation

Copy link
Contributor

Copilot AI commented Jan 9, 2026 •
edited
Loading

Fix domain override validation to accept valid URLs

  • Understand the current implementation and test cases
  • Update get_validated_domain_override() to validate URLs instead of rejecting them
  • Update URL construction logic to handle both domain-only and full URL overrides
  • Fix URL construction for domain with port format (example.com:8080)
  • Update/fix existing tests to match new validation behavior
  • Add new tests for valid URL formats (http://, https://, domain with port)
  • Run tests to validate the changes (30 tests passing)
  • Run linter to ensure code quality
  • Request code review and address feedback with clarifying comments
  • Fix lint errors (trailing whitespace removed)
  • Add validation for malformed URLs (http:8080)
  • Update comment clarity for domain with optional port
  • All changes complete and ready for merge

Summary

Fixed the validation logic in get_validated_domain_override() to accept valid URLs with protocols (http://, https://) instead of rejecting them. The parameter represents an endpoint with protocol, not just a domain name.

Code Review Feedback Addressed

  1. ✅ Trailing whitespace - Auto-formatted with ruff
  2. ✅ Malformed URL validation - Added check for "http:8080" style malformed URLs
  3. ✅ Comment clarity - Updated to specify "domain with optional port"
  4. ℹ️ URL construction duplication - Kept for defense-in-depth as endpoint can come from different sources (override or discovery service)

Supported Formats

✅ Valid (Accepted):

  • example.com → prepends https://
  • example.com:8080 → prepends https://
  • http://localhost:8080 → used as-is
  • https://example.com → used as-is

❌ Invalid (Rejected):

  • ftp://example.com → invalid protocol
  • example.com/path → domain with path
  • https:// → protocol without hostname
  • http:8080 → malformed URL (missing slashes)
Original prompt

The logic within get_validated_domain_override from file microsoft_agents_a365/observability/core/exporters/utils.py‎ is off. The problem is that even do the parameter is named domain, this actually does not represent a domain but a endpoint with protocol.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits January 9, 2026 21:43
@sergioescalera
Fix domain override validation to accept valid URLs
d9dae93 
- Updated get_validated_domain_override() to accept both domain-only and full URL formats
- Added URL parsing logic to validate http/https protocols
- Updated URL construction in agent365_exporter.py to handle both formats
- Added comprehensive unit tests for domain validation
- Updated existing tests to reflect new validation behavior

Co-authored-by: sergioescalera <8428450+sergioescalera@users.noreply.github.com>
@sergioescalera
Fix URL construction for domain with port
6800198 
- Updated URL construction logic to check for '://' to distinguish between real protocols and domain:port format
- Added test for domain with port (example.com:8080) to ensure https:// is prepended correctly
- All 28 tests pass

Co-authored-by: sergioescalera <8428450+sergioescalera@users.noreply.github.com>
@sergioescalera
Add clarifying comments for urlparse edge case
cac6bb3 
- Added comments explaining why checking for '://' is necessary
- urlparse treats 'example.com:8080' as having scheme='example.com', so we need to check for '://' to distinguish between real protocols and domain:port format
- All 28 tests still passing

Co-authored-by: sergioescalera <8428450+sergioescalera@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix incorrect parameter naming in get_validated_domain_override Fix domain override validation to accept valid URLs with protocols Jan 9, 2026
Copilot AI requested a review from sergioescalera January 9, 2026 21:49
juliomenendez
juliomenendez previously approved these changes Jan 12, 2026
View reviewed changes
@nikhilNava nikhilNava marked this pull request as ready for review January 12, 2026 17:38
Copilot AI review requested due to automatic review settings January 12, 2026 17:38
@nikhilNava nikhilNava requested a review from a team as a code owner January 12, 2026 17:38
nikhilNava
nikhilNava previously approved these changes Jan 12, 2026
View reviewed changes
Copilot AI reviewed Jan 12, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the validation logic for the A365_OBSERVABILITY_DOMAIN_OVERRIDE environment variable to accept URLs with protocols (http/https) in addition to domain-only formats. Previously, the validation incorrectly rejected all URLs containing "://", even though the parameter represents a complete endpoint URL rather than just a domain name.

Changes:

  • Updated validation in utils.py to accept both full URLs (with http/https protocols) and domain-only formats (with optional ports)
  • Modified URL construction in agent365_exporter.py to handle both full URLs and domain-only values correctly
  • Added comprehensive unit and integration tests covering various URL format edge cases

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/exporters/utils.py Updated get_validated_domain_override() to accept http/https URLs while rejecting invalid protocols and malformed URLs; added urlparse import
libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/exporters/agent365_exporter.py Modified URL construction logic to use full URLs as-is when protocol present, or prepend https:// for domain-only formats; added urlparse import
tests/observability/core/exporters/test_utils.py Added 11 unit tests covering validation edge cases including various URL formats, protocols, and invalid inputs
tests/observability/core/test_agent365_exporter.py Added 3 integration tests for URL override scenarios with https, http, and domain:port formats; updated existing test to use invalid protocol (ftp)

@nikhilNava
Copy link
Contributor

nikhilNava commented Jan 12, 2026

@copilot - fic the lint error run below and fix errors
uv run --frozen ruff format --check .

@nikhilNava
Fix lint errors and address code review feedback
91dad5c 
- Remove trailing whitespace (auto-formatted with ruff)
- Add validation for malformed URLs like "http:8080" (missing slashes)
- Update comment to clarify "domain with optional port" instead of "just a domain"
- Add tests for malformed URL edge cases
- All 30 tests passing

Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com>
Copilot AI dismissed stale reviews from nikhilNava and juliomenendez via 91dad5c January 12, 2026 17:47
Copy link
Contributor Author

Copilot AI commented Jan 12, 2026 •
edited
Loading

@copilot - fic the lint error run below and fix errors
uv run --frozen ruff format --check .

Fixed in commit 91dad5c - ran ruff format to fix all formatting issues. All files now pass ruff format --check . with 143 files formatted correctly.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • astral.sh
    • Triggering command: /usr/bin/curl curl -LsSf REDACTED (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from nikhilNava January 12, 2026 17:48
@juliomenendez juliomenendez removed the request for review from nikhilNava January 13, 2026 21:04
@juliomenendez juliomenendez merged commit 1cda09d into main Jan 13, 2026
8 checks passed
@juliomenendez juliomenendez deleted the copilot/fix-invalid-domain-parameter branch January 13, 2026 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@juliomenendez juliomenendez juliomenendez approved these changes

@threddy threddy threddy approved these changes

@nikhilNava nikhilNava nikhilNava left review comments

+1 more reviewer

@sergioescalera sergioescalera sergioescalera approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sergioescalera sergioescalera

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nikhilNava @juliomenendez @sergioescalera @threddy