feat(messaging): onboard Microsoft Teams channel#5585
Open
sandl99 wants to merge 16 commits into
Open
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Microsoft Teams as a fully supported messaging channel across NemoClaw: Teams channel manifest with inputs, credentials, render fragments for OpenClaw and Hermes, and lifecycle hooks; host-forward planning engine integrated into the compiler and workflow planner; network policy preset with Microsoft/Azure endpoint allowlists; Hermes uv-pip package installer for Teams Python dependencies; port-conflict detection hooks with overlap status reporting; onboarding orchestration with port preservation and rollback; process recovery for webhook forwards; and comprehensive test coverage across compiler, manifests, policies, and integration scenarios. ChangesMicrosoft Teams Messaging Channel Rollout
Sequence Diagram(s)sequenceDiagram
participant ManifestCompiler
participant planHostForward
participant MessagingWorkflowPlanner
participant refreshDerivedPlanFields
ManifestCompiler->>planHostForward: compileChannel(manifest, inputs, active, resolver)
planHostForward-->>ManifestCompiler: SandboxMessagingHostForwardPlan | undefined
ManifestCompiler-->>MessagingWorkflowPlanner: SandboxMessagingChannelPlan with hostForward
MessagingWorkflowPlanner->>refreshDerivedPlanFields: stop/start/rebuild plan update
refreshDerivedPlanFields->>planHostForward: recompute hostForward per channel
planHostForward-->>refreshDerivedPlanFields: updated SandboxMessagingHostForwardPlan
refreshDerivedPlanFields-->>MessagingWorkflowPlanner: updated plan with hostForward and runtimeSetup
sequenceDiagram
participant ensureDashboardForward
participant resolveMessagingHostForwardForSandbox
participant ensureMessagingHostForwardForSandbox
participant ensureForward
participant abortMessagingHostForwardFailure
ensureDashboardForward->>resolveMessagingHostForwardForSandbox: resolve active hostForward
resolveMessagingHostForwardForSandbox-->>ensureDashboardForward: port to preserve
ensureDashboardForward->>ensureDashboardForward: add port to preservedPorts
ensureDashboardForward->>ensureMessagingHostForwardForSandbox: ensure webhook forward
ensureMessagingHostForwardForSandbox->>ensureForward: start forward(sandboxName, port, label)
ensureForward-->>ensureMessagingHostForwardForSandbox: true / false
alt forward fails with rollback enabled
ensureMessagingHostForwardForSandbox->>abortMessagingHostForwardFailure: stop ports, delete sandbox
abortMessagingHostForwardFailure-->>abortMessagingHostForwardFailure: exit(1)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 46%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-5585.docs.buildwithfern.com/nemoclaw |
2 tasks
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
nemoclaw-blueprint/policies/tiers.yaml (1)
47-47: 🧹 Nitpick | 🔵 TrivialRun
network-policy-e2efor the Open-tier preset expansion.Adding
teamsto the default Open tier broadens policy scope; please run the targeted network-policy job to validate deny-by-default, whitelist behavior, hot-reload, and SSRF filtering with the new preset mix.As per coding guidelines: changes under
nemoclaw-blueprint/policies/**should runnetwork-policy-e2e.🤖 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 `@nemoclaw-blueprint/policies/tiers.yaml` at line 47, You have modified the network policies in the tiers.yaml file by adding teams with read-write access to the Open tier. Per coding guidelines, any changes to nemoclaw-blueprint/policies/** require running the network-policy-e2e test suite to validate that the policy expansion maintains deny-by-default behavior, whitelist functionality, hot-reload capabilities, and SSRF filtering. Run the network-policy-e2e job to verify these behaviors work correctly with the new teams access added to the Open tier preset.Source: Coding guidelines
agents/hermes/policy-additions.yaml (1)
258-340: 🧹 Nitpick | 🔵 TrivialRun the Hermes E2E set for this channel-policy addition.
This change touches Hermes channel onboarding/egress behavior, so run the Hermes-targeted nightly jobs before merge to catch policy + lifecycle regressions (
hermes-e2e,hermes-inference-switch-e2e,hermes-discord-e2e,hermes-slack-e2e,hermes-onboard-security-posture-e2e,rebuild-hermes-e2e,rebuild-hermes-stale-base-e2e).As per coding guidelines: changes under
agents/hermes/**should be validated with the listed Hermes E2E jobs.🤖 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 `@agents/hermes/policy-additions.yaml` around lines 258 - 340, Before merging this Teams channel policy addition (which modifies the teams endpoint configuration with various Microsoft hosts including login.microsoftonline.com, api.botframework.com, graph.microsoft.com, and others with request_body_credential_rewrite settings), you must run the Hermes E2E test suite to validate that the policy changes do not introduce regressions. Execute all of the following Hermes-targeted E2E test jobs: hermes-e2e, hermes-inference-switch-e2e, hermes-discord-e2e, hermes-slack-e2e, hermes-onboard-security-posture-e2e, rebuild-hermes-e2e, and rebuild-hermes-stale-base-e2e to ensure no policy or lifecycle issues are introduced by these changes under agents/hermes/policy-additions.yaml.Source: Coding guidelines
🤖 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.
Inline comments:
In `@docs/_components/StarterPromptButton.tsx`:
- Around line 149-153: The shell code block in StarterPromptButton.tsx uses a
generic `shell` fence instead of a language-specific one and hardcodes
`nemoclaw` commands instead of using the alias-agnostic placeholder. Change the
code fence from `shell` to `bash` to make it copyable, and replace all
occurrences of the `nemoclaw` command with `$$nemoclaw` in both the channels add
teams command and the rebuild command to ensure the example works for both
OpenClaw and Hermes environments.
In `@docs/about/overview.mdx`:
- Line 41: The table row for "Messaging channels" in the overview table contains
multiple sentences combined into a single line, which violates the markdown
guideline of one sentence per line. Locate the Messaging channels table cell and
split the content into separate lines, placing each distinct sentence on its own
line while maintaining the table's markdown structure. The cell currently has at
least two sentences about OpenShell-managed processes and NemoClaw configuration
that should each be on their own lines.
In `@src/lib/messaging/applier/build/messaging-build-applier.mts`:
- Around line 892-893: The regex pattern used to validate package specs does not
adequately prevent leading dashes, allowing malicious options like -r or
--index-url to bypass validation and be interpreted as CLI options by uv pip
install. Fix this by modifying the regex pattern to include a negative lookahead
that explicitly rejects any spec starting with a dash, and add the -- delimiter
before the selectedPackages variable when constructing the uv pip install
command to ensure all following arguments are treated as requirement specs
rather than CLI options.
In `@src/lib/onboard/messaging-host-forward.ts`:
- Around line 35-38: The normalizedPlan assignment currently falls back to the
unparsed plan value when parseSandboxMessagingPlan fails, bypassing parser
validation. Remove the ?? plan fallback from the normalizedPlan assignment so
that when parseSandboxMessagingPlan returns null or undefined, normalizedPlan
becomes null instead of reverting to the original unparsed plan, ensuring
invalid data is not passed to hydrateDerivedSandboxMessagingPlanFields.
In `@test/policies-teams.test.ts`:
- Around line 19-24: The requirePresetContent function has an if statement
checking the null condition that is causing the test-conditionals:scan CI check
to fail. Remove the if statement that tests !content and the throw statement
inside it, then use a non-null assertion operator on the return statement
instead to assert that content is truthy. Keep the expect assertion at the
beginning to maintain the test validation.
---
Nitpick comments:
In `@agents/hermes/policy-additions.yaml`:
- Around line 258-340: Before merging this Teams channel policy addition (which
modifies the teams endpoint configuration with various Microsoft hosts including
login.microsoftonline.com, api.botframework.com, graph.microsoft.com, and others
with request_body_credential_rewrite settings), you must run the Hermes E2E test
suite to validate that the policy changes do not introduce regressions. Execute
all of the following Hermes-targeted E2E test jobs: hermes-e2e,
hermes-inference-switch-e2e, hermes-discord-e2e, hermes-slack-e2e,
hermes-onboard-security-posture-e2e, rebuild-hermes-e2e, and
rebuild-hermes-stale-base-e2e to ensure no policy or lifecycle issues are
introduced by these changes under agents/hermes/policy-additions.yaml.
In `@nemoclaw-blueprint/policies/tiers.yaml`:
- Line 47: You have modified the network policies in the tiers.yaml file by
adding teams with read-write access to the Open tier. Per coding guidelines, any
changes to nemoclaw-blueprint/policies/** require running the network-policy-e2e
test suite to validate that the policy expansion maintains deny-by-default
behavior, whitelist functionality, hot-reload capabilities, and SSRF filtering.
Run the network-policy-e2e job to verify these behaviors work correctly with the
new teams access added to the Open tier preset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: af41e3bd-1aaa-4379-930c-8a19bf19304e
📒 Files selected for processing (76)
agents/hermes/Dockerfileagents/hermes/Dockerfile.baseagents/hermes/manifest.yamlagents/hermes/policy-additions.yamlagents/openclaw/manifest.yamldocs/_components/StarterPromptButton.tsxdocs/about/overview.mdxdocs/deployment/brev-web-ui.mdxdocs/deployment/deploy-to-remote-gpu.mdxdocs/get-started/quickstart.mdxdocs/manage-sandboxes/lifecycle.mdxdocs/manage-sandboxes/messaging-channels.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/network-policy/customize-network-policy.mdxdocs/network-policy/integration-policy-examples.mdxdocs/reference/architecture.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxdocs/reference/network-policies.mdxdocs/reference/troubleshooting.mdxdocs/security/best-practices.mdxnemoclaw-blueprint/policies/presets/teams.yamlnemoclaw-blueprint/policies/tiers.yamlsrc/lib/actions/sandbox/channel-status.test.tssrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/agent/defs.test.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging-channel-config.test.tssrc/lib/messaging/AGENTS.mdsrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/messaging/applier/setup-applier.tssrc/lib/messaging/channels/built-ins.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/metadata.test.tssrc/lib/messaging/channels/slack/manifest.tssrc/lib/messaging/channels/teams/hooks/host-forward-port-conflict.test.tssrc/lib/messaging/channels/teams/hooks/host-forward-port-conflict.tssrc/lib/messaging/channels/teams/hooks/index.tssrc/lib/messaging/channels/teams/manifest.tssrc/lib/messaging/channels/teams/template-resolver.tssrc/lib/messaging/channels/template-resolver.tssrc/lib/messaging/compiler/engines/host-forward-engine.tssrc/lib/messaging/compiler/manifest-compiler.test.tssrc/lib/messaging/compiler/manifest-compiler.tssrc/lib/messaging/compiler/workflow-planner.test.tssrc/lib/messaging/compiler/workflow-planner.tssrc/lib/messaging/diagnostics.test.tssrc/lib/messaging/hooks/builtins.tssrc/lib/messaging/hooks/common/config-prompt.tssrc/lib/messaging/hooks/hook-runner.test.tssrc/lib/messaging/host-forward.tssrc/lib/messaging/index.tssrc/lib/messaging/manifest/types.tssrc/lib/messaging/persistence.tssrc/lib/messaging/plan-validation.test.tssrc/lib/messaging/plan-validation.tssrc/lib/onboard/agent-dashboard-forward.test.tssrc/lib/onboard/agent-dashboard-forward.tssrc/lib/onboard/dashboard.tssrc/lib/onboard/initial-policy.test.tssrc/lib/onboard/messaging-host-forward.test.tssrc/lib/onboard/messaging-host-forward.tssrc/lib/onboard/messaging-prep.test.tssrc/lib/sandbox/channels.test.tssrc/lib/status-command-deps.tssrc/lib/tunnel/services.tstest/channels-add-preset.test.tstest/messaging-build-applier.test.tstest/messaging-plan-test-helper.tstest/policies-teams.test.tstest/policies.test.tstest/sandbox-provider-cleanup.test.ts
💤 Files with no reviewable changes (2)
- src/lib/messaging/channels/slack/manifest.ts
- src/lib/messaging/hooks/common/config-prompt.ts
sandl99
added
area: messaging
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27931132475
|
cv
approved these changes
Jun 22, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Around line 268-271: The fallback logic in parseSandboxCommandResult that uses
the nullish coalescing operator (??), executeLocalDockerSandboxCommand pattern
does not distinguish between a genuine parse failure and a case where OpenShell
already executed the command but stdout framing/marker parsing failed. This
causes non-idempotent commands to execute twice. Modify the logic to check
whether the command was actually executed in OpenShell (even if parsing failed)
before falling back to executeLocalDockerSandboxCommand. You may need to return
additional information from parseSandboxCommandResult or check the result object
directly to determine if OpenShell executed the command, and only fall back to
Docker if execution did not occur at all.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 04f358a6-508a-4ff0-a07e-e57eeddc9da4
📒 Files selected for processing (2)
src/lib/actions/sandbox/process-recovery.tstest/process-recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/process-recovery.test.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Around line 268-271: The fallback logic in parseSandboxCommandResult that uses
the nullish coalescing operator (??), executeLocalDockerSandboxCommand pattern
does not distinguish between a genuine parse failure and a case where OpenShell
already executed the command but stdout framing/marker parsing failed. This
causes non-idempotent commands to execute twice. Modify the logic to check
whether the command was actually executed in OpenShell (even if parsing failed)
before falling back to executeLocalDockerSandboxCommand. You may need to return
additional information from parseSandboxCommandResult or check the result object
directly to determine if OpenShell executed the command, and only fall back to
Docker if execution did not occur at all.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 04f358a6-508a-4ff0-a07e-e57eeddc9da4
📒 Files selected for processing (2)
src/lib/actions/sandbox/process-recovery.tstest/process-recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/process-recovery.test.ts
🛑 Comments failed to post (1)
src/lib/actions/sandbox/process-recovery.ts (1)
268-271: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent duplicate command execution on parse-only failure.
At Line 268, Docker fallback runs whenever parsing returns
null. Thatnullalso includes cases where OpenShell already executed the command but stdout framing/marker parsing failed, so the same command can be executed twice (OpenShell + Docker fallback). For non-idempotent commands this can duplicate side effects.⚙️ Proposed fix
- return ( - parseSandboxCommandResult(result) ?? - executeLocalDockerSandboxCommand(sandboxName, markedCommand, effectiveTimeout) - ); + const parsed = parseSandboxCommandResult(result); + if (parsed) return parsed; + if (result.error || (result.status ?? 1) !== 0) { + return executeLocalDockerSandboxCommand(sandboxName, markedCommand, effectiveTimeout); + } + return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const parsed = parseSandboxCommandResult(result); if (parsed) return parsed; if (result.error || (result.status ?? 1) !== 0) { return executeLocalDockerSandboxCommand(sandboxName, markedCommand, effectiveTimeout); } return null;🤖 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/actions/sandbox/process-recovery.ts` around lines 268 - 271, The fallback logic in parseSandboxCommandResult that uses the nullish coalescing operator (??), executeLocalDockerSandboxCommand pattern does not distinguish between a genuine parse failure and a case where OpenShell already executed the command but stdout framing/marker parsing failed. This causes non-idempotent commands to execute twice. Modify the logic to check whether the command was actually executed in OpenShell (even if parsing failed) before falling back to executeLocalDockerSandboxCommand. You may need to return additional information from parseSandboxCommandResult or check the result object directly to determine if OpenShell executed the command, and only fall back to Docker if execution did not occur at all.
…ng-onboard # Conflicts: # src/lib/actions/sandbox/process-recovery.ts # src/lib/onboard/agent-dashboard-forward.ts
… into feat/ms-teams-messaging-onboard
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28003939444
|
14 tasks
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
3 participants
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.
Summary
Adds experimental Microsoft Teams channel onboarding through NemoClaw's manifest-first messaging architecture for OpenClaw and Hermes. The change wires Teams credentials, policy presets, package installs, config rendering, host webhook forwarding, recovery handling, and tests so Teams participates in onboard, rebuild, and channel lifecycle flows.
Key features
Inputs
MSTEAMS_APP_ID: ClientID - Required
MSTEAMS_APP_PASSWORD: ClientSecret - Required
MSTEAMS_TENANT_ID: TenantId - Required
MSTEAMS_ALLOWED_USERS: Optional - OpenClaw allows to pairing after setup.
Related Issue
Part of #5492
Result
Changes
MSTEAMS_PORT.PR Review Advisor justification
PR Review Advisor follow-up: #5585 (comment)
PRA-1:TEAMS_ALLOWED_USERSis intentionally optional. Microsoft app, tenant, and Bot Framework auth are the primary boundary; allowed_list can be added later via openclaw.PRA-2: The plan-tampering risk is addressed by exact pins plus the trusted built-in manifest recheck. Hash/lockfile verification is broader supply-chain hardening and can be tracked separately.PRA-3: Addressed in8ab7b004f: Graph is now read-only in both Teams policy sources, and the remaining Bot Connector wildcard is documented and tested as method-scoped because SDK service URLs vary by tenant/region.PRA-4: Deferred for now. Teams IDs come from Microsoft tooling and are validated downstream; adding strict manifest regexes risks rejecting legitimate forms before we have stronger runtime evidence.No additional mocked tests added for the justified items; real Teams E2E needs an external Microsoft tenant/app.
Type of Change
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Verification run:
npm run build:clinpx vitest run src/lib/onboard/messaging-host-forward.test.ts src/lib/messaging/channels/manifests.test.ts src/lib/messaging/channels/metadata.test.ts src/lib/messaging/compiler/manifest-compiler.test.ts src/lib/messaging/compiler/workflow-planner.test.ts test/messaging-build-applier.test.ts test/policies-teams.test.ts test/process-recovery.test.tsnpx vitest run test/sandbox-connect-inference/auto-pair-approval.test.tsnpm run typecheck:clinpm run test-conditionals:scan -- --top 25git push --no-verifyas requestedSigned-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Summary of Changes
New Features
Improvements