Skip to content

refactor(agent-email): address review nits from #1829 (connector routes)#1830

Merged
itomek merged 2 commits into
mainfrom
fix/1829-connector-review-nits
Jun 23, 2026
Merged

refactor(agent-email): address review nits from #1829 (connector routes)#1830
itomek merged 2 commits into
mainfrom
fix/1829-connector-review-nits

Conversation

@itomek

@itomek itomek commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Small follow-up to the merged #1829 — addresses the two actionable (non-blocking) nits from the code review on the new connector routes:

  • Wire the unused module logger into the configure / complete / grant error handlers, so the live OAuth path is debuggable (it was defined-but-unused).
  • Centralize the DEFAULT_ACCOUNT ("default") sentinel handling server-side — a new _account_email helper applied on both GET /v1/email/connectors and /complete — and drop the redundant client-side !== "default" magic string, so the store's sentinel value stays server-side only.

The third review nit (empty-scope grant) was explicitly flagged as needing no action; left as-is.

Evidence

This change is backend-only (logger + server-side sentinel normalization), so the playground UI is unchanged — verified below straight from this PR's build.

Behavioral proof of the new normalization — GET /v1/email/connectors now maps the store's "default" no-email sentinel to null:

$ curl -s :8135/v1/email/connectors | jq '.providers[] | {provider, connected, account_email}'
{ "provider": "google",    "connected": true, "account_email": "…@gmail.com" }
{ "provider": "microsoft", "connected": true, "account_email": null }   # was "default" before this PR

UI still works — screenshots captured from this PR's sidecar (md5-identical to the #1829 baseline, confirming the UI did not change):

Not connected — per-provider client_id/secret connect forms; Send disabled:

Playground — not connected

Connected — a Disconnect button on each account; Send picks the mailbox from a dropdown and is ready:

Playground — connected

Test plan

  • PYTHONPATH=hub/agents/python/email python -m pytest tests/unit/agents/email/test_connector_routes.py tests/unit/agents/email/test_playground.py -q — includes a new test asserting /complete normalizes the "default" sentinel to null.

- Wire the module logger into the configure/complete/grant except handlers so
  the live OAuth path is debuggable (was defined-but-unused).
- Centralize the DEFAULT_ACCOUNT sentinel normalization server-side (new
  _account_email helper, applied on /connectors and /complete) and drop the
  redundant client-side 'default' magic string.
@itomek itomek marked this pull request as ready for review June 23, 2026 16:06
@itomek itomek requested a review from kovtcharov-amd as a code owner June 23, 2026 16:06
@github-actions github-actions Bot added the tests Test changes label Jun 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve

This is a tight, scope-clean follow-up to #1829 that addresses the two actionable review nits on the new email connector routes: it wires the previously-unused module logger into the OAuth error handlers so live failures are debuggable, and it centralizes the "default" no-email sentinel handling on the server side behind one helper — dropping the duplicated magic string from the client JS. A new test confirms the /complete endpoint normalizes the sentinel to null.

The bottom line: the sentinel is now translated in exactly one place and applied on both the list and complete endpoints, so the client no longer needs to know the store's internal "default" value. No security, compatibility, or architecture concerns — safe to merge.

🔍 Technical details

Issues

None blocking.

🟢 Per-call deferred import in the new helper (connector_routes.py:77) — _account_email imports DEFAULT_ACCOUNT on every invocation. This is harmless and actually consistent with the module's deliberate lazy-import convention (every gaia.connectors.* import in this file is function-local to keep the frozen sidecar's import graph lazy), so no change is needed — just noting it's an intentional trade-off, not an oversight.

Verified

  • Logger is real and correctly used. log is defined at connector_routes.py:33 (logging.getLogger("gaia_agent_email.connectors")); the three new log.warning(...) calls all sit on the error paths and re-raise, so this is added observability with no swallowed exceptions — consistent with the no-silent-fallback rule.
  • Sentinel normalization is complete and consistent. _account_email is applied on both GET /connectors (:96) and /complete (:157); the only remaining "default" reference in connector_routes.py is the docstring. On the client side the !== "default" check is fully gone — the JS now reads st.account_email || provider (playground_html.py:607) and the list-render paths (:533, :636) rely on the server-normalized value. No orphaned DEFAULT_ACCOUNT reference after the top-level import was removed from the list endpoint.
  • Return ordering is correct. {"connected": True, **state, "account_email": _account_email(state.get("account_email"))} puts the explicit key after the **state spread, so the normalized value wins even when state already carries a raw account_email.
  • Test exercises the real path. test_complete_hides_default_account_sentinel monkeypatches gaia.connectors.api.complete_authorization/grant_agent; since the route imports those names at call-time from that module, the patch is picked up, and the assertion (account_email is None) covers exactly the sentinel-leak the helper closes.

Strengths

  • Removes a cross-layer magic string ("default") the client shouldn't have known about — store-internal knowledge now stays server-side, behind a single named helper applied at both surfaces.
  • The new test asserts the normalized output shape of /complete, not just that the endpoint was called — it would actually catch a regression where the sentinel leaks again.
  • Genuinely minimal diff: only the three files the stated task requires, no drive-by changes.

@itomek itomek enabled auto-merge June 23, 2026 16:18
@itomek itomek self-assigned this Jun 23, 2026
@itomek itomek added this pull request to the merge queue Jun 23, 2026
Merged via the queue into main with commit bfb7d15 Jun 23, 2026
32 checks passed
@itomek itomek deleted the fix/1829-connector-review-nits branch June 23, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants