Rebase & Review Summary

Changes made during rebase:

• Rebased onto main to remove merge commit (now single clean commit)
• Updated commit message to follow conventional commits format:

  fix(oauth): populate expires_at from client_secret_expires_at in DCR registration
  

Code Review: ✅ Approved

RFC 7591 Compliance: The implementation correctly uses client_secret_expires_at from the DCR response:

• Per RFC 7591 Section 3.2.1, this is an optional field containing a Unix timestamp
• Value of 0 means "never expires" - correctly handled by the > 0 check
• The original TODO was misleading (suggested summing two values, but client_secret_expires_at is already an absolute timestamp)

Security: No concerns - this is informational metadata only

Performance: No impact - single dictionary lookup + conditional datetime conversion

Consistency: Uses the same datetime.fromtimestamp(..., tz=timezone.utc) pattern as elsewhere in the codebase

Tests: All 176 OAuth-related unit tests pass

Final Review & Fixes Summary

Rebased and significantly enhanced the original implementation with multiple rounds of review.

Key Improvements Made:

1. Type Safety & Coercion

• Added int() coercion for string timestamps from non-strict AS implementations
• Added proper exception handling for ValueError, TypeError, OSError, OverflowError

2. Millisecond Timestamp Detection

• Detects millisecond timestamps using digit count (>= 13 digits)
• Automatically converts to seconds instead of failing silently
• Uses logger.debug to avoid log noise

3. Parse Failure Handling

• Changed _parse_expires_at_timestamp() to return tuple[datetime | None, bool]
• On parse failure: preserves existing expires_at instead of clearing to None
• On explicit 0: sets to None (RFC 7591 "never expires")
• On valid timestamp: sets to parsed datetime

4. Update Path Enhancement

• update_client_registration now updates expires_at independent of secret rotation
• An AS can refresh expiry without rotating the secret

5. Comprehensive Test Coverage (11 new tests)

• Valid timestamp parsing
• Zero value (never expires)
• Missing field
• String coercion
• Invalid value handling
• Millisecond conversion
• Secret rotation scenarios
• Expiry-only updates
• Preserve on invalid/negative values

Files Changed

• mcpgateway/services/dcr_service.py: +64 lines (helper method + integration)
• tests/unit/mcpgateway/services/test_dcr_service.py: +540 lines (11 new tests)

Test Results

✅ 41 DCR tests pass
✅ 183 OAuth tests pass (full suite)

Complete Change Summary

Original PR

Added expires_at population from client_secret_expires_at in DCR registration.

Enhancements Made During Review

1. Type Safety & Error Handling

• Added int() coercion for string timestamps from non-strict AS implementations
• Comprehensive exception handling: ValueError, TypeError, OSError, OverflowError
• Negative value rejection with warning log

2. Millisecond Timestamp Detection

• Detects millisecond timestamps using digit count (>= 13 digits)
• Automatically converts ms → seconds instead of failing
• Changed from logger.info to logger.debug to reduce noise

3. Tuple Return Pattern for Safe Updates
Created _parse_expires_at_timestamp() returning tuple[datetime | None, bool]:

(datetime, True)  → Valid timestamp, update expires_at
(None, True)      → Explicit 0 ("never expires"), set to None  
(None, False)     → Parse failed, preserve existing expires_at

4. Update Path Enhancement

• update_client_registration now updates expires_at independent of secret rotation
• Only updates when should_update=True, preserving existing value on parse failure
• Handles AS implementations that refresh expiry without rotating secret

5. Test Coverage (11 new tests)

Files Changed

mcpgateway/services/dcr_service.py                 |  64 ++-
tests/unit/mcpgateway/services/test_dcr_service.py | 540 +++++++++++++++++++++
2 files changed, 603 insertions(+), 1 deletion(-)

Test Results

• ✅ 41 DCR service tests pass
• ✅ 183 OAuth tests pass (full suite)
• ✅ All existing tests unaffected