docs(messaging): clarify Teams group allowlists (Fixes #5853)#5863
Conversation
Fixes NVIDIA#5853 Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdates the Microsoft Teams messaging docs to clarify that ChangesTeams allowlist documentation
Estimated review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/messaging/compiler/manifest-compiler.test.ts (1)
556-582: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the empty-allowlist normalization branch.
This case covers
TEAMS_GROUP_POLICY=allowlistwith a populated list, but the template resolver's key normalization path —allowlistwith an emptyTEAMS_GROUP_ALLOWED_USERScollapsing togroupPolicy:"open"and omittinggroupAllowFrom— is not exercised here. That branch directly encodes the PR objective that groups stay open unless the allowlist is explicitly populated, so a regression there would be silent.🧪 Suggested additional test
it("normalizes empty Microsoft Teams group allowlist to open policy", async () => { const plan = await withEnv( { ...TEST_TEAMS_ENV, TEAMS_GROUP_POLICY: "allowlist", TEAMS_GROUP_ALLOWED_USERS: undefined, }, () => compiler().compile({ sandboxName: "demo", agent: "openclaw", workflow: "rebuild", isInteractive: false, configuredChannels: ["teams"], credentialAvailability: { MSTEAMS_APP_PASSWORD: true }, }), ); const rendered = JSON.stringify(plan.agentRender); expect(rendered).toContain('"groupPolicy":"open"'); expect(rendered).not.toContain('"groupAllowFrom"'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/messaging/compiler/manifest-compiler.test.ts` around lines 556 - 582, Add a test in manifest-compiler.test.ts to cover the empty Microsoft Teams allowlist normalization path. Extend the existing OpenClaw Teams group policy coverage by adding a case that uses compiler().compile with TEAMS_GROUP_POLICY set to allowlist and TEAMS_GROUP_ALLOWED_USERS unset/empty, then assert the rendered plan normalizes to groupPolicy:"open" and does not include groupAllowFrom. Reuse withEnv, TEST_TEAMS_ENV, and the same compile inputs so the new test targets the template resolver behavior directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/messaging/compiler/manifest-compiler.test.ts`:
- Around line 556-582: Add a test in manifest-compiler.test.ts to cover the
empty Microsoft Teams allowlist normalization path. Extend the existing OpenClaw
Teams group policy coverage by adding a case that uses compiler().compile with
TEAMS_GROUP_POLICY set to allowlist and TEAMS_GROUP_ALLOWED_USERS unset/empty,
then assert the rendered plan normalizes to groupPolicy:"open" and does not
include groupAllowFrom. Reuse withEnv, TEST_TEAMS_ENV, and the same compile
inputs so the new test targets the template resolver behavior directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a311aca4-ec0e-4206-b909-6b095455a808
📒 Files selected for processing (5)
docs/manage-sandboxes/messaging-channels.mdxsrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/teams/manifest.tssrc/lib/messaging/channels/teams/template-resolver.tssrc/lib/messaging/compiler/manifest-compiler.test.ts
wscurran
added
area: docs
|
✨ Thanks for the proposed fix clarifying that TEAMS_ALLOWED_USERS and MSTEAMS_ALLOWED_USERS only gate direct messages and for adding the TEAMS_GROUP_POLICY and TEAMS_GROUP_ALLOWED_USERS onboarding inputs. This proposes a way to render the OpenClaw channels.msteams.groupPolicy and groupAllowFrom fields only when a non-empty group allowlist is configured, keeping the default group behavior open. Related open issues: |
|
Thanks for working on this and for digging into the Teams group/channel allowlist behavior. The distinction you found is real and important: That said, I’d like to narrow this PR back to the issue scope before we continue review. The original issue explicitly calls this out as not a code defect and frames the fix as a documentation plus onboarding-UX gap. The source changes here go beyond documenting the existing behavior: they add new NemoClaw env vars, add manifest inputs, change persisted/rebuild-hydrated state, modify onboarding prompts, and render new OpenClaw config fields. That is a product/UX behavior change, even if the underlying OpenClaw knobs are valid. Could you please revise this PR to be docs-only for now? The docs should clearly state that:
I’m not rejecting the idea of first-class |
Signed-off-by: Deepak Jain <deepujain@gmail.com>
deepujain
changed the title
|
Narrowed this back to docs-only. I removed the Teams runtime/test changes from the branch, kept the DM vs group/channel allowlist guidance, and corrected the empty group allowlist wording to match OpenClaw behavior.\n\nValidated with
docs/_build/agent-variants/about/how-it-works.hermes.generated.mdx is already up to date
docs/_build/agent-variants/about/how-it-works.hermes.generated.mdx is already up to date |
|
@deepujain would you please make sure to every commit has verified signature? |
|
Merging for now, but please make sure to keep all commits have verified signature from now on. |
## Summary Refreshes the v0.0.70 release docs from the release announcement and the `v0.0.69..v0.0.70` commit range. It also documents the `channels start` policy restoration behavior that was missing from the shared OpenClaw and Hermes command references, and bumps the Fern CLI version used for docs validation. ## Changes - Replaced the stale `v0.0.70` release-notes entry with the actual release themes, including CLI, onboarding, inference, messaging, Windows, documentation, and release-validation changes. - Documented that `channels start` reapplies the matching built-in network policy preset before rebuild and rolls back to disabled if policy restoration fails. - Bumped `fern/fern.config.json` from `5.55.0` to `5.59.0` for the docs refresh. - Source summary: - #5754 -> `docs/about/release-notes.mdx`: Notes Docker Desktop gateway bridge retry behavior during onboarding. - #5930 -> `docs/about/release-notes.mdx`: Links `nemoclaw use` default sandbox selection to the command reference. - #5948 -> `docs/about/release-notes.mdx`: Links reasoning-compatible endpoint validation to inference documentation. - #5950 -> `docs/about/release-notes.mdx`: Links Windows bootstrap WSL recovery behavior to Windows preparation and troubleshooting docs. - #5856 -> `docs/about/release-notes.mdx`: Notes rebuilt policy preset registry repair. - #5882 and #5949 -> `docs/about/release-notes.mdx`: Notes Hermes stale base-image state repair. - #6016 -> `docs/reference/commands.mdx`, `docs/reference/commands-nemohermes.mdx`, and `docs/manage-sandboxes/messaging-channels.mdx`: Documents channel policy restoration and rollback on `channels start`. - #5859 -> `docs/about/release-notes.mdx`: Links quickstart network approval guidance. - #5863 -> `docs/about/release-notes.mdx`: Links Teams allowlist guidance in the messaging page. - #5756, #5926, #6010, and #6011 -> `docs/about/release-notes.mdx`: Summarizes the Vitest E2E validation cutover. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: doc-only prose refresh with no runtime behavior change. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [ ] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) `npm run docs` exited 0 and Fern reported one existing light-mode accent contrast warning. `fern check --warnings` confirmed the warning is the site theme contrast ratio, not content introduced by this PR. --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
4 participants
Summary
TEAMS_ALLOWED_USERS/MSTEAMS_ALLOWED_USERSare direct-message allowlists, not group or channel sender restrictions.TEAMS_GROUP_POLICYandTEAMS_GROUP_ALLOWED_USERS.channels.msteams.groupPolicyandgroupAllowFromonly when a non-empty group allowlist is configured, keeping the default group behavior open.Fixes #5853
Evidence it works
npx vitest run src/lib/messaging/channels/manifests.test.ts src/lib/messaging/compiler/manifest-compiler.test.tspassed, 34 tests.npm run build:clipassed.npm run typecheck:clipassed.npm run docs:sync-agent-variantspassed.npm run docspassed with 0 errors; Fern reported 2 warnings without printing warning details.npx @biomejs/biome format src/lib/messaging/channels/teams/template-resolver.ts src/lib/messaging/channels/teams/manifest.ts src/lib/messaging/channels/manifests.test.ts src/lib/messaging/compiler/manifest-compiler.test.ts docs/manage-sandboxes/messaging-channels.mdxpassed.git diff --checkpassed.npx prek run --files ...reached and passed the relevant touched-file hooks through gitleaks, then timed out while handling invalid cached Python hook environments. No touched-file hook failure appeared before the timeout.npm testwas attempted and timed out at 300s with unrelated environment/tooling failures that require local gateway, docker, portablestat, net probe, and CLI log setup. The focused Teams manifest/compiler tests above passed.Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit