Skip to content

fix: include logo_uri in OAuth client metadata - #759

Open
zerone0x wants to merge 2 commits into
evalstate:mainfrom
zerone0x:fix/logo-uri-metadata-710
Open

fix: include logo_uri in OAuth client metadata#759
zerone0x wants to merge 2 commits into
evalstate:mainfrom
zerone0x:fix/logo-uri-metadata-710

Conversation

@zerone0x

Copy link
Copy Markdown

Summary

  • include logo_uri in default OAuth client metadata

Fixes #710

Changes

  • add logo_uri to OAuthClientMetadata defaults

@MohammedAlkindi MohammedAlkindi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This has been sitting since April with no response, so here is a review with the checks done — the change is still wanted, and the only thing standing between it and mergeable is a rebase.

Still needed at tip. logo_uri is absent from metadata_kwargs on current main, so nothing has superseded this in the intervening months.

The test is well-formed and passes. Run on Windows 11 (10.0.26200), CPython 3.12/3.13:

pytest tests/unit/fast_agent/mcp/test_cimd.py -k logo_uri   ->  1 passed

Monkeypatching OAuthClientProvider and asserting on captured kwargs is the right shape for this — no network, no keys, and it pins the field rather than the whole metadata blob. Whole-file run is 1 failed, 25 passed on the branch and 1 failed, 27 passed on main; the single failure is TestCallbackServerPortFallback::test_callback_server_port_fallback on both sides, so it is pre-existing and unrelated. The pass-count difference is just the branch's age — main has gained tests since April — not a regression.

Why it will not merge as-is. GitHub reports CONFLICTING, and the reason is that the block this patch edits has moved substantially:

  • On main the metadata_kwargs literal is now at roughly line 1460, not 728.
  • redirect_uris is no longer a plain local — it is now built by a helper:
metadata_kwargs: dict[str, Any] = {
    "client_name": "fast-agent",
    "redirect_uris": _oauth_redirect_uris(
        selected_redirect_port=selected_redirect_port,
        configured_redirect_port=settings.redirect_port,
        redirect_path=settings.redirect_path,
    ),
    "grant_types": ["authorization_code", "refresh_token"],
    "response_types": ["code"],

So the one-line addition still applies cleanly in spirit — insert "logo_uri": "https://fast-agent.ai/logo.png", after "client_name" — but the surrounding context in the patch no longer matches, which is what git is objecting to. A rebase onto current main should be mechanical.

One substantive question for a maintainer rather than the author: logo_uri becomes part of the registered OAuth client metadata that authorization servers may display to users at consent time, so the URL needs to stay live and stable for as long as any registered client exists. https://fast-agent.ai/logo.png resolving today is not quite the same commitment as it resolving indefinitely. Worth a deliberate yes rather than an incidental one — I have no view on whether that domain is under the project's control.

Nothing here is a defect in the patch; it is a good small change that got stale waiting.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include logo_uri in fast-agent CIMD metadata document

3 participants