feat(msgraph): add webhook listener platform - #1
Closed
dlkakbs wants to merge 2 commits into
Closed
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41c98b23e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+83
to
+95
| def _build_receipt_key(notification: Dict[str, Any]) -> str: | ||
| explicit_id = str(notification.get("id") or "").strip() | ||
| if explicit_id: | ||
| return f"id:{explicit_id}" | ||
| payload = "|".join( | ||
| [ | ||
| str(notification.get("subscriptionId") or ""), | ||
| str(notification.get("changeType") or ""), | ||
| str(notification.get("resource") or ""), | ||
| json.dumps(notification.get("resourceData") or {}, sort_keys=True), | ||
| ] | ||
| ) | ||
| return f"sha1:{sha1(payload.encode('utf-8')).hexdigest()}" |
There was a problem hiding this comment.
_build_receipt_key() falls back to hashing only subscriptionId, changeType, resource, and resourceData when notification.id is absent. Because Graph marks id as optional, repeated legitimate updates for the same resource (for example multiple updated events with the same resource path/resourceData) produce the same key and are dropped as duplicates, which silently loses real events until eviction from the 5,000-entry window.
Useful? React with 👍 / 👎.
Comment on lines
+211
to
+212
| if resource == pattern or resource.startswith(f"{pattern}/"): | ||
| return True |
There was a problem hiding this comment.
Resource filtering compares raw strings, but Graph subscription resources are commonly configured with a leading / while delivered notification resource values are relative paths without it. With accepted_resources set from subscription-style values (for example /communications/...), _resource_accepted() rejects every valid notification and the adapter returns no accepted work.
Useful? React with 👍 / 👎.
dlkakbs
pushed a commit
that referenced
this pull request
May 20, 2026
…registries
Both web_search_registry._resolve() and image_gen_registry.get_active_provider()
walked their registered providers and returned the first one matching the
capability flag — without checking whether that provider was actually
usable. On a fresh install with no credentials at all, this meant
get_active_search_provider() returned `brave-free` (legacy preference
order) even though BRAVE_SEARCH_API_KEY was unset, leading the
dispatcher to surface a "BRAVE_SEARCH_API_KEY is not set" error for a
provider the user never chose. Same bug shape in image_gen for FAL.
Resolution semantics now match tools.web_tools._get_backend():
1. Explicit config name wins, ignoring is_available() — the dispatcher
surfaces a precise "X_API_KEY is not set" error rather than silently
switching backends. Matches user expectation: "I configured X, tell
me what's wrong with X."
2. Fallback (no explicit config) walks the legacy preference order
filtered by is_available() — pick the highest-priority backend the
user actually has credentials for.
is_available() is wrapped in a try/except so a buggy provider doesn't
brick resolution.
E2E verified:
- No creds + no config: get_active_search_provider() -> None
- Explicit brave-free + no key: get_active_search_provider() -> brave-free
(and .is_available() correctly reports False)
This fix was identified during the spike (NousResearch#25182 finding #1) and is
fold-in to the same PR rather than a follow-up.
dlkakbs
pushed a commit
that referenced
this pull request
May 20, 2026
…sResearch#26672) The #1 confusing cause of the xAI 403 (per Teknium): X Premium+ subscribers see Grok inside the X app and assume API access is included. It is NOT — only standalone SuperGrok subscribers can use xai-oauth with Hermes today. Without calling this out, every Premium+ user hits the 403 with no idea why. PR NousResearch#26666's neutral 4-cause list was correct but buried the most common cause. Lead with the Premium+ gotcha, then list the other possibilities (no subscription, wrong tier, exhausted quota) as fallbacks. Same neutral framing — does not accuse anyone of being unsubscribed.
dlkakbs
pushed a commit
that referenced
this pull request
May 20, 2026
Three issues flagged by the Copilot review on this PR: 1. Double JSON emit on stage failure (Copilot #1, #2). When -Stage <name> ran a worker that threw, Invoke-Stage's finally emitted a JSON result frame AND the entry-point catch emitted a second error frame -- producing two concatenated JSON objects on stdout and breaking the one-line-per-invocation contract that drivers parse against. Same issue applied to -Json mode on a full install (every stage's finally plus a final error frame missing duration_ms/skipped). Fix: Invoke-Stage's finally now sets $script:_StageEmittedErrorFrame when it emits a failure frame; the entry-point catch checks the flag and skips its own emit, still exit 1. 2. $prevEAP uninitialized on early try-block throw (Copilot #3). In Install-Uv, Test-Python, Test-Node's winget fallback, _Run-NpmInstall, and the playwright block, '$prevEAP = $ErrorActionPreference' lived as the first statement INSIDE the try. If anything between 'try {' and that line threw (Write-Info on an unusual host, the npx-finding loop, etc.), the catch's 'if ($prevEAP) { ... }' restore was a no-op and EAP could remain relaxed. Fix: hoist '$prevEAP = $ErrorActionPreference' to the line immediately before 'try {' in all five sites. Catch's restore is now always meaningful regardless of where in the try the throw originated. No change to Invoke-Stage's success path or to the four lint-clean EAP sites (Test-Node was the only winget-related catch). All 19 metadata smoke tests still pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
References PR NousResearch#19815.
This PR is the second 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:
The goal here is to keep the review surface small and land the webhook ingestion layer before the Teams pipeline runtime, outbound delivery, and docs follow-up PRs in the stack.