Skip to content

feat(msgraph): add auth and client foundation#21408

Closed
dlkakbs wants to merge 2 commits into
NousResearch:mainfrom
dlkakbs:pr/msgraph-auth-client
Closed

feat(msgraph): add auth and client foundation#21408
dlkakbs wants to merge 2 commits into
NousResearch:mainfrom
dlkakbs:pr/msgraph-auth-client

Conversation

@dlkakbs

@dlkakbs dlkakbs commented May 7, 2026

Copy link
Copy Markdown
Contributor

References PR #19815.

This is the first part of the Microsoft Teams meeting pipeline stack, split out in response to maintainer review on the original PR.

This slice is intentionally scoped to:

  • Microsoft Graph auth foundation
  • shared Microsoft Graph client utilities
  • token cache concurrency coverage

The goal here is to keep the review surface small and establish the Graph auth/client layer before the webhook listener, pipeline runtime, outbound delivery, and docs follow-up PRs in the stack.

Review and merge in stack order.

@dlkakbs dlkakbs marked this pull request as draft May 7, 2026 16:30
@alt-glitch alt-glitch added type/feature New feature or request comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have labels May 7, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Resubmission of closed #21389 (same branch pr/msgraph-auth-client, same author). First slice of the #19815 Teams meeting pipeline stack.

@dlkakbs dlkakbs marked this pull request as ready for review May 7, 2026 21:03
@teknium1

teknium1 commented May 8, 2026

Copy link
Copy Markdown
Contributor

Thanks for splitting this out, and for taking the earlier review direction on #19815 seriously — the plugin + CLI + skill shape is exactly right.

One thing that'll make these much easier to review: the 5 PRs are currently all based on main with cumulative diffs (#21412 contains everything from #21408-#21411). That means we can't see what each slice actually adds on top of the previous one, and merging the bottom of the stack would leave the upper PRs thousands of lines behind.

Could you retarget them as a proper stack so diffs are incremental?

gh pr edit <N> --base <branch> does it in place — no close/reopen needed, comments and CI history are preserved. Once #21408 merges, GitHub auto-retargets #21409 to main, and so on up the stack.

Planning to review bottom-up starting with this one. Thanks for working through the split.

@dlkakbs

dlkakbs commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

gh pr edit is failing with "branch not found" because the intermediate branches don't exist upstream. I don't have write access to push them. Could you advise on how to proceed? @teknium1

@teknium1

teknium1 commented May 8, 2026

Copy link
Copy Markdown
Contributor

Merged via PR #21922. Your two commits (feat(msgraph): add auth and client foundation and test(msgraph): cover concurrent token cache reuse) landed on main with your authorship preserved via rebase-merge — see commits a152c706b and b878f89f6.

Two small follow-ups added on top during salvage:

  1. fix(msgraph): stream download_to_file body instead of buffering — the original download_to_file routed through the shared _request() path, which reads the body inside an httpx.AsyncClient context that closes before aiter_bytes() iterates. Tests passed with the 17-byte payload because small bodies are indistinguishable. On real Teams meeting recordings (hundreds of MB) the body would load fully into memory per download. Rewrote to open a dedicated client.stream() and keep the context open across the iteration, preserving retry/token-refresh/Retry-After semantics on the streaming path. Added a discriminating test (512 KiB at chunk_size=65536 → 8 chunks under streaming, 1 under buffering).

  2. docs(msgraph): add Azure app registration walkthrough + env var reference — new /docs/guides/microsoft-graph-app-registration covering app registration, client secret, per-capability Graph permissions (transcript-first / recording fallback / Graph-mode delivery), admin consent, New-CsApplicationAccessPolicy for tenant-scoping, token-flow smoke test, AADSTS troubleshooting, and secret rotation. Env var reference gained a Microsoft Graph subsection. Wired into the sidebar.

The remaining PRs in the stack (#21409#21412) are still open and will be reviewed next against current main. Thanks for splitting the stack up — it made the foundation much easier to land cleanly.

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

Labels

comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants