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 AdvisorFindings: 2 needs attention, 2 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
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