Skip to content

fix(msteams): keep delegated auth healthy when an expired token can auto-refresh#106923

Open
lonexreb wants to merge 1 commit into
openclaw:mainfrom
lonexreb:fix/106566-msteams-delegated-health
Open

fix(msteams): keep delegated auth healthy when an expired token can auto-refresh#106923
lonexreb wants to merge 1 commit into
openclaw:mainfrom
lonexreb:fix/106566-msteams-delegated-health

Conversation

@lonexreb

@lonexreb lonexreb commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Resubmit of #106916, which was auto-closed by the active-PR-limit bot. Same tested commit; unchanged.

What Problem This Solves

probeMSTeams marks the MS Teams channel unhealthy (delegatedAuth.ok: false) whenever the cached delegated access token is expired (extensions/msteams/src/probe.ts). But an expired access token is not a broken connection: the delegated-token path (token.ts) refreshes it from the stored refresh token on the next API call. The result is false-positive health failures — alert spam and channel restarts — for a connector that is actually fine. The existing code even labels it "will auto-refresh on next use" while still reporting a failure. (Reported in #106566.)

Why This Change Was Made

loadDelegatedTokens() returns a refreshToken alongside the access-token expiresAt, and the delegated path already auto-refreshes an expired access token using it. So the probe now treats an expired access token as healthy when a refresh token is stored (ok: !isExpired || canAutoRefresh). The genuine-failure signal is preserved: expired with no refresh token still reports ok: false with a clearer "re-run setup wizard" message. The probe intentionally does not attempt a network refresh to verify the token server-side — that would turn a cheap local health check into an API call; presence is the issue-specified proxy.

User Impact

MS Teams channel health checks stop firing false alarms (and stop triggering needless restarts) for connectors whose access token has simply aged out but can auto-refresh. Operators are still alerted when delegated auth is truly unrecoverable.

Evidence

  • extensions/msteams/src/probe.test.ts: the prior test that asserted ok: false for an expired-but-refreshable token is corrected to expect ok: true with no error; a new test covers expired + no refresh token → ok: false with the re-auth message. Full file green (5 passed) via node scripts/run-vitest.mjs extensions/msteams/src/probe.test.ts.
  • git diff --check clean; 2 files, +45/−4; no CHANGELOG.md edit.
  • Source-reproducible: the false positive is a local health computation over stored token fields; no live tenant required.

Fixes #106566

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: XS labels Jul 14, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 14, 2026
@clawsweeper

clawsweeper Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 13, 2026, 8:20 PM ET / July 14, 2026, 00:20 UTC.

Summary
Updates the Microsoft Teams probe to treat an expired delegated access token as healthy when a stored refresh token permits automatic renewal, with regression coverage for refreshable and unrecoverable states.

PR surface: Source +6, Tests +35. Total +41 across 2 files.

Reproducibility: yes. at source level: current main deterministically marks every non-future delegated access token unhealthy, while the runtime retains a refresh token for next-use renewal. No live current-main tenant reproduction was supplied.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #106566
Summary: The open issue is the canonical bug report, this PR is its candidate fix, and the earlier identical PR was administratively superseded by this resubmission.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real-setup output showing an expired access token with a stored refresh token remains healthy and refreshes successfully on the next Teams API use.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR provides focused mocked tests but no after-fix installed-connector output showing the healthy probe and subsequent refresh; add redacted terminal output, logs, or a recording, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] The PR body supplies source-level and mocked-test evidence only; it does not show an installed Teams connector reporting healthy after access-token expiry and successfully refreshing on the next real API use.
  • [P1] A stored but revoked or server-expired refresh token remains locally indistinguishable from a usable one, so the new status means locally refresh-capable rather than server-verified authentication.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the plugin-owned predicate and explicit no-refresh-token recovery message, then merge after redacted real-setup evidence demonstrates the intended health result and successful next-use refresh behavior.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Wait for the contributor's required real-behavior proof, then perform ordinary maintainer merge review; no automated code repair is currently identified.

Security
Cleared: The focused predicate and test changes neither expose token material nor alter credential storage, permissions, dependencies, or executable supply-chain inputs.

Review details

Best possible solution:

Keep the plugin-owned predicate and explicit no-refresh-token recovery message, then merge after redacted real-setup evidence demonstrates the intended health result and successful next-use refresh behavior.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current main deterministically marks every non-future delegated access token unhealthy, while the runtime retains a refresh token for next-use renewal. No live current-main tenant reproduction was supplied.

Is this the best way to solve the issue?

Yes—the owning Teams probe is the narrowest maintainable boundary, and checking refresh-token availability matches the existing local auto-refresh contract while preserving the explicit unrecoverable case.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against b4c311e51bc9.

Label changes

Label changes:

  • add P2: This is a bounded Microsoft Teams health-reporting bug that can cause false operator alerts without breaking the core runtime.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR provides focused mocked tests but no after-fix installed-connector output showing the healthy probe and subsequent refresh; add redacted terminal output, logs, or a recording, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.

Label justifications:

  • P2: This is a bounded Microsoft Teams health-reporting bug that can cause false operator alerts without breaking the core runtime.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR provides focused mocked tests but no after-fix installed-connector output showing the healthy probe and subsequent refresh; add redacted terminal output, logs, or a recording, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +6, Tests +35. Total +41 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 8 2 +6
Tests 1 37 2 +35
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 45 4 +41

What I checked:

Likely related people:

  • HDYA: Introduced the substantial Microsoft Teams federated-authentication work and related delegated-auth gating, making them closely connected to current Teams credential behavior. (role: recent auth-area contributor; confidence: high; commits: 26f633b604; files: extensions/msteams/src/token.ts, extensions/msteams/src/probe.ts)
  • BradGroux: Co-authored the major Microsoft Teams authentication expansion and has relevant history around the plugin's credential and configuration paths. (role: adjacent Teams auth contributor; confidence: medium; commits: 26f633b604; files: extensions/msteams/src/token.ts, extensions/msteams/src/probe.ts)
  • steipete: Moved Microsoft Teams into its plugin boundary and has repeatedly maintained the surrounding plugin and test architecture. (role: plugin boundary and recent area contributor; confidence: medium; commits: d9f9e93dee, 26763d1910; files: extensions/msteams/src/probe.ts, extensions/msteams/src/probe.test.ts, extensions/msteams/src/token.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@BradGroux BradGroux mentioned this pull request Jul 14, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: msteams Channel integration: msteams P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] False-Positive Channel Health Failure for MS Teams on Expired Delegated Token

1 participant

@lonexreb