Skip to content

docs(messaging): add architecture README#5586

Open
sandl99 wants to merge 2 commits into
feat/ms-teams-messaging-onboardfrom
docs/messaging-readme
Open

docs(messaging): add architecture README#5586
sandl99 wants to merge 2 commits into
feat/ms-teams-messaging-onboardfrom
docs/messaging-readme

Conversation

@sandl99

@sandl99 sandl99 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a package-level README for src/lib/messaging that documents the manifest-first messaging architecture introduced by the Teams onboarding work.
The README gives contributors a single reference for the plan flow, hook phases, manifest shape, host-forward behavior, and channel registration path.

Related Issue

Part of #5492.

Changes

  • Add src/lib/messaging/README.md with architecture flow and class diagrams.
  • Document hook registration, hook inputs, hook phases, and hook failure behavior.
  • Document manifest fields, a manifest skeleton, template resolution, host forwarding, planner usage, and plan application.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • 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)
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Verification run:

  • npx prek run --from-ref origin/feat/ms-teams-messaging-onboard --to-ref HEAD
  • npm run docs passed with 0 errors and 2 existing Fern warnings
  • GitHub commit verification: 06aada998aba787a1d43b619279332a753bbda18 verified=true reason=valid

Signed-off-by: San Dang sdang@nvidia.com

Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99 sandl99 self-assigned this Jun 22, 2026
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 02a7a6c5-618d-47a7-8ae2-e75c02576aeb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/messaging-readme

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/feat/ms-teams-messaging-onboard
Head: HEAD
Confidence: high

Required E2E

  • None. No E2E is recommended because the only changed file is documentation and cannot affect NemoClaw runtime or user flows.

Optional E2E

  • None.

New E2E recommendations

  • None.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: None
Optional Vitest E2E scenarios: None

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/feat/ms-teams-messaging-onboard
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • None. Docs-only README change outside the Vitest scenario workflow, registry, runtime support, fixtures, manifests, and live test surfaces; it cannot affect Vitest E2E scenario behavior.

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • None.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

Review posture: No blocking advisor findings
Action expectation: Address required items before merge. Resolve or explicitly justify warnings. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up.
Findings: 0 required fixes, 0 items to resolve/justify, 1 in-scope improvement
Since last review: 3 prior items resolved, 0 still apply, 1 new item found

Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 1 in-scope improvement

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

  • None.

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

  • None.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • Mark optional ChannelManifest fields in the class diagram (src/lib/messaging/README.md:66): The `ChannelManifest` class diagram lists `policyPresets`, `hostForward`, `runtime`, and `agentPackages` like required properties, while `manifest/types.ts` marks those fields optional. The later prose does call `hostForward` optional, but the diagram is the first contract summary readers see.
    • Impact: Contributors may over-specify new manifests or misunderstand which manifest sections are required, especially when adding lightweight channels that do not need policy presets, host forwarding, runtime metadata, or agent packages.
    • Recommendation: Update the diagram to show optional fields consistently, for example `policyPresets?: ...`, `hostForward?: ...`, `runtime?: ...`, and `agentPackages?: ...`.
    • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
    • Verification hint: Compare README lines 66-74 with `src/lib/messaging/manifest/types.ts` where those four `ChannelManifest` properties are declared with `?`.
    • Missing regression test: No runtime regression test is needed for this prose-only correction. A docs/type-contract lint that checks README diagrams against TypeScript interfaces would catch this class of drift if the project has or adds such tooling.
    • Evidence: `manifest/types.ts` declares `readonly policyPresets?`, `readonly hostForward?`, `readonly runtime?`, and `readonly agentPackages?`, while the README diagram omits optional markers for those fields.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

Since last review details

Current findings, using the urgency labels above:

  • Mark optional ChannelManifest fields in the class diagram (src/lib/messaging/README.md:66): The `ChannelManifest` class diagram lists `policyPresets`, `hostForward`, `runtime`, and `agentPackages` like required properties, while `manifest/types.ts` marks those fields optional. The later prose does call `hostForward` optional, but the diagram is the first contract summary readers see.
    • Impact: Contributors may over-specify new manifests or misunderstand which manifest sections are required, especially when adding lightweight channels that do not need policy presets, host forwarding, runtime metadata, or agent packages.
    • Recommendation: Update the diagram to show optional fields consistently, for example `policyPresets?: ...`, `hostForward?: ...`, `runtime?: ...`, and `agentPackages?: ...`.
    • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
    • Verification hint: Compare README lines 66-74 with `src/lib/messaging/manifest/types.ts` where those four `ChannelManifest` properties are declared with `?`.
    • Missing regression test: No runtime regression test is needed for this prose-only correction. A docs/type-contract lint that checks README diagrams against TypeScript interfaces would catch this class of drift if the project has or adds such tooling.
    • Evidence: `manifest/types.ts` declares `readonly policyPresets?`, `readonly hostForward?`, `readonly runtime?`, and `readonly agentPackages?`, while the README diagram omits optional markers for those fields.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each finding. A human maintainer must make the final merge decision.

Signed-off-by: San Dang <sdang@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant