feat(messaging): add channel enrollment manifests#4050
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
📝 WalkthroughWalkthroughAdds hook type contracts, an in-memory hook registry, a hook-runner with strict output validation/serialization checks, fake/common hook implementations and tests, complete channel manifests for Telegram/Discord/Slack/WeChat/WhatsApp, manifest validation tests, and built-in manifest registry/index exports. ChangesMessaging hooks and built-in channel manifests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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/messaging/hooks/hook-runner.ts`:
- Around line 82-105: The function isMessagingSerializableValue currently treats
any repeated object as a cycle by using a single WeakSet; change it to track the
current recursion path (visiting) so shared references like [sameObj, sameObj]
are allowed while real reference cycles still fail. In
isMessagingSerializableValue, rename the second parameter to something like
visiting (WeakSet<object>), replace the early "if (seen.has(objectValue)) return
false" with "if (visiting.has(objectValue)) return false" to detect cycles only
on the current stack, add the object to visiting before recursing into arrays or
object values, and remove it from visiting after recursion returns; do not
reject objects just because they were seen earlier in a different branch. Keep
all other logic (primitive checks, prototype check using Object.getPrototypeOf)
the same and ensure recursive calls pass the visiting set.
🪄 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: f26d3f6a-bab2-4b28-aa67-eb5273dd95be
📒 Files selected for processing (14)
src/lib/messaging/channels/discord/manifest.tssrc/lib/messaging/channels/index.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/slack/manifest.tssrc/lib/messaging/channels/telegram/manifest.tssrc/lib/messaging/channels/whatsapp/manifest.tssrc/lib/messaging/hooks/hook-runner.test.tssrc/lib/messaging/hooks/hook-runner.tssrc/lib/messaging/hooks/index.tssrc/lib/messaging/hooks/registry.tssrc/lib/messaging/hooks/types.tssrc/lib/messaging/index.tssrc/lib/messaging/manifest/registry.test.tssrc/lib/messaging/manifest/types.test.ts
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review is based on trusted deterministic PR metadata, linked issue text, review-thread data, E2E Advisor comment, repository read-only inspection of selected files, and the provided diff; no commands, scripts, tests, or package-manager operations were executed.; The provided git diff is truncated; assessment assumes the supplied changedFiles list is complete for head SHA ab94308.; Linked issue #3993 has no comments in the provided context; acceptance mapping therefore uses the issue body and PR/E2E comments only.; Required E2E job pass/fail state was inferred from the provided status rollup and E2E Advisor comment; no separate workflow artifacts were inspected.; This advisory result does not approve, merge, request changes, label, dispatch workflows, or assert that human review is unnecessary. Full advisor summaryPR Review AdvisorBase: Declarative messaging manifest and hook scaffolding is well isolated and has substantial unit coverage, but mergeability is blocked and required E2E jobs for this messaging/runtime surface are not shown as passed for head SHA ab94308. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Summary
Adds phase-1 manifest-first messaging architecture scaffolding with typed channel manifests, hook registry/runner contracts, explicit enrollment hooks, and fake WeChat/common hook implementations for design review. The declarations stay isolated from production workflows so current behavior does not change.
Related Issue
Fixes #3993
Part of #3896
Changes
enrollhooks for token-paste channels throughcommon.tokenPasteand WeChat host-QR throughwechat.ilinkLogin.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional verification run locally:
npm test -- --project cli src/lib/messaging/channels src/lib/messaging/manifest src/lib/messaging/hookspassed.npm run typecheck:clipassed.npm run lint -- src/lib/messagingpassed with one unrelated existing Biome warning insrc/lib/onboard/child-exit-tracker.test.ts.git diff --checkpassed.npx prek run --all-fileswas run but did not pass because existing broader CLI tests failed outside this messaging diff.git commitandgit pushhooks were blocked by the same broad CLI test failure, so the signed commit and push used--no-verify.npm test -- --project cli test/cli.test.ts test/sandbox-connect-inference.test.tspassedtest/sandbox-connect-inference.test.ts;test/cli.test.tsstill failed the existing macOS diagnostic-output assertion fordebug --quick explains restricted dmesg.Signed-off-by: San Dang sdang@nvidia.com