refactor(messaging): migrate credential providers to manifest plans#4937
Draft
sandl99 wants to merge 45 commits into
Draft
refactor(messaging): migrate credential providers to manifest plans#4937sandl99 wants to merge 45 commits into
sandl99 wants to merge 45 commits into
Conversation
Stores the compiled SandboxMessagingPlan in the onboard session so that resume runs can restore the plan to env without re-running enrollment hooks (token paste, QR pairing). Fixes the gap where the registry entry lost its `messaging` field on rebuild because the plan was only held in a process env var that didn't survive across process restarts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…el-setup Exports readMessagingPlanFromEnv and writePlanToEnv from messaging-channel-setup.ts (which already owns MessagingSetupApplier) to keep src/lib/onboard.ts from growing. Collapses the one-name messaging-channel-setup require into a single line to free headroom. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…an persistence - Extract parseSandboxMessagingPlan to messaging-plan-session.ts to keep onboard-session.ts growth under the monolith threshold - Guard plan restoration with sandbox-name + agent identity check so stale plans from renamed sandboxes or agent switches are not reused - Add three behavior assertions in sandbox.test.ts: fresh setup persists env plan to session; matching plan is restored to env on non-interactive resume; mismatched sandbox name skips restoration Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tive resume rebuild.ts stages an authoritative plan from the registry (which reflects post-stop/-start channel mutations) before calling onboard --resume. Previously, the session plan restoration was unconditionally overwriting that staged plan, causing stopped channels to reappear as active after rebuild. Now the handler checks the env first: if a plan is already staged (rebuild path), it is used as-is. The session plan is only restored when the env is empty, covering the plain process-restart resume case this PR was originally targeting. Also adds a test asserting the rebuild-path preference. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- On non-interactive resume, restore plan from registry (always current after stop/start/add/remove) instead of stale session snapshot; env-first priority preserved so rebuild.ts staging still wins - In rebuild.ts, persist the staged plan to the session alongside messagingChannels/disabledChannels/messagingChannelConfig so the session is fully consistent during the rebuild window - Add getChannelsFromPlan, getDisabledChannelsFromPlan, and getMessagingChannelConfigFromPlan helpers in messaging-plan-session.ts so the next PR can replace the three individual session fields with plan-derived reads - Move MessagingHostStateApplier re-export to messaging-channel-setup and getRegistrySandboxMessagingPlan helper to keep onboard.ts net-neutral Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…itecture (phase 4a) - Add src/lib/messaging/applier/conflict-detection.ts with all core logic: ConflictRegistryEntry minimal interface, createMessagingConflictProbe factory (consolidates the duplicated tri-state probe pattern from onboard.ts, policy-channel.ts, and status-command-deps.ts), plan-to-request helpers (getActiveChannelIdsFromPlan, getCredentialHashesFromPlan, planToConflictChannelRequests), plan-preferred/legacy-fallback entry resolution (resolveActiveChannelsFromEntry, resolveCredentialHashesFromEntry), and pure detection functions (findConflictsInEntries, detectAllOverlapsInEntries, backfillLegacyEntryChannels) - Rewrite messaging-conflict.ts as a thin public adapter: no detection logic inside, all three public exports delegate to applier functions, re-exports createMessagingConflictProbe so callers don't need to import from applier directly; removes getChannelDef/getChannelTokenKeys import (stored hashes are self-describing — no channel-constant lookup needed) - Update onboard.ts conflict-check block: remove makeConflictProbe() and the MESSAGING_CHANNELS/hashCredential block; use createMessagingConflictProbe with injected openshell deps and findChannelConflictsFromPlan from plan - Add plan-path tests: planToConflictChannelRequests grouping and exclusions, findChannelConflictsFromPlan matching/disabled/no-hash cases, plan-only registry entries in findChannelConflicts and findAllOverlaps Closes #4392 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Remove redundant createMessagingConflictProbe named import from messaging-conflict.ts (symbol is already re-exported on the line below; flagged by CodeQL unused-import rule) - Fix checkGatewayLiveness in onboard.ts to use runOpenshell exit status instead of stdout length; a healthy gateway with no sandboxes returns empty output with status 0, which the previous check incorrectly treated as "down" Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Fix credentialHash gate: gate on credentialAvailable (set by the compiler for all real onboarding flows) instead of credentialHash (not yet populated), which previously caused the conflict check to be silently skipped for all manifest-plan onboarding; planToConflictChannelRequests now includes channels with credentialAvailable=true and no hash, falling through to unknown-token conservative detection - Fix cross-channel hash contamination: getCredentialHashesFromPlan now accepts an optional channelId filter; conflictReasonForRequest and conflictReasonForPair use the new resolveChannelHashesFromEntry helper so Slack or Discord hashes cannot produce false positive unknown-token results when checking a Telegram request - Fix legacy hash fallback: when a plan-backed entry exists but carries no credentialHash for the requested channel (migration in flight), fall back to providerCredentialHashes so matching-token detection is not silently lost - Align active-channel semantics with plan-filter.ts: getActiveChannelIdsFromPlan now also checks channel.active && !channel.disabled, not only disabledChannels - Split plan-driven tests into src/lib/messaging/applier/conflict-detection.test.ts to offset the messaging-conflict.test.ts monolith growth; adds WhatsApp no-op test, cross-channel isolation, credentialAvailable-gate, and legacy-fallback cases Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ng-conflict-plan-driven
…e and populate credentialHash in plan bindings Remove hardcoded PROVIDER_SUFFIXES/KNOWN_CHANNEL_IDS from conflict-detection.ts; backfillLegacyEntryChannels now receives the suffix map as an injected parameter. The adapter layer (messaging-conflict.ts) derives it from BUILT_IN_CHANNEL_MANIFESTS credentials so adding a channel to the manifest registry propagates automatically. Also fix credential-binding-engine to hash the env var value into credentialHash when credentialAvailable is true, enabling matching-token conflict detection on the plan-driven path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove legacy providerCredentialHashes fallback from resolveChannelHashesFromEntry — entries without a plan now return {} and fall through to unknown-token (conservative). Fix PROVIDER_SUFFIXES to collect all credential suffixes per manifest rather than only credentials[0], so multi-credential channels like Slack probe all their provider names during backfill (channel active if any present). Split 473-line conflict-detection.test.ts monolith into conflict-detection-plan.test.ts (plan helpers) and conflict-detection-entry.test.ts (detection functions) to stay under the monolith growth threshold.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…omments Reject the env-staged plan when its sandboxName does not match the current sandbox being created, preventing a stale plan from a prior run from gating or bypassing conflict detection for a different sandbox. Update stale comments that described the credential-binding engine as not yet emitting credentialHash — it does now. Rephrase to reflect the remaining no-hash case (registry-only resume without a compiler re-run). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… test fixtures Restore the providerCredentialHashes fallback in resolveChannelHashesFromEntry for entries whose plan carries no channel hashes (e.g. registry-only resume without a compiler re-run), preserving safety coverage during migration. Fix tgChannel(false, true) → tgChannel(true, true) in disabled-channel test cases so assertions isolate plan.disabledChannels as the only gate rather than passing via the inactive-channel path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dings with active channels Address PR Review Advisor findings on phase-4a conflict detection: - resolveChannelHashesFromEntry now filters legacy providerCredentialHashes to only the providerEnvKey values declared for the requested channel in BUILT_IN_CHANNEL_MANIFESTS, preventing unrelated Slack hashes from producing false Telegram conflicts and vice versa. Unknown channels retain the full map. - planToConflictChannelRequests now intersects credentialBindings with getActiveChannelIdsFromPlan so stale bindings for inactive or absent channels cannot generate false conflict requests. - Fix misleading JSDoc on findChannelConflictsFromPlan: available bindings with no credentialHash are included (not excluded) and fall through to conservative unknown-token detection. - Add regression tests: cross-channel legacy hash scoping, active-channel binding intersection (absent channel, active=false), and slackChannel fixture for plan test file. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…rison and add no-plan fallback Address second round of PR Review Advisor findings: - conflictReasonForRequest and conflictReasonForPair now use manifest-declared CHANNEL_CREDENTIAL_ENV_KEYS as the primary comparison set. For multi-credential channels like Slack (SLACK_BOT_TOKEN + SLACK_APP_TOKEN), a differing bot token with a missing app token previously returned null; it now returns unknown-token because the missing manifest key marks the comparison as incomplete. - onboard.ts conflict gate: when hasPlanCredentials is false but token-backed channels were selected (enabledChannels contains non-QR channels), fall back to findChannelConflicts with just the channel names to preserve conservative unknown-token warnings even when the compiled plan has no credential data (e.g. credential-store-backed availability or stale env plan). - policy-channel.ts: add explicit comment scoping the add-channel path as a known limitation — no compiled plan is available at channels-add time, so findChannelConflicts is used with caller-built hashes; plan-driven migration is follow-up work. - Tests: four Slack partial-hash suppression cases covering the false-negative regression (conflictReasonForRequest with missing app token, both differ, conflictReasonForPair with missing app token from both sides, both differ). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…nifest-driven hashing Address third round of PR Review Advisor findings: - messaging-conflict.ts: add findChannelConflictsForOnboarding() that unions the plan-based hash-precise check with a channel-name-only fallback for any selectedTokenChannels not covered by the plan's credentialAvailable bindings. This ensures a partial or stale same-sandbox plan cannot silently skip channels it omits. onboard.ts now delegates to this single function rather than inlining the union logic. - onboard.ts conflict gate: replace the multi-branch plan/fallback block with a single call to findChannelConflictsForOnboarding(sandboxName, currentPlan, selectedTokenChannels, registry), keeping all conflict logic in messaging-conflict.ts. - policy-channel.ts checkChannelAddConflict: replace raw acquired-map iteration with manifest-driven hash building via createBuiltInChannelManifestRegistry(). Iterates channelManifest.credentials[].providerEnvKey to build credentialHashes, mirroring planToConflictChannelRequests without requiring a compiled plan. QR-only channels (WhatsApp: credentials=[]) exit early with no conflict possible. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…tection
Legacy entries no longer carry hash metadata into conflict comparisons.
resolveChannelHashesFromEntry returns {} for entries without a plan, which
falls through to conservative unknown-token detection in all callers.
- ConflictRegistryEntry: drop providerCredentialHashes field
- resolveChannelHashesFromEntry: plan path only; legacy entries → {}
- CHANNEL_CREDENTIAL_ENV_KEYS comment updated (now solely for manifest-key
comparison set, not legacy scoping)
- Section comment: "plan-preferred, legacy-fallback" → "Entry resolution"
- Remove findChannelConflictsForOnboarding and fallback logic; onboard.ts
calls findChannelConflictsFromPlan directly (rationale in issue #4392)
Test updates:
- conflict-detection-entry.test.ts: remove legacy cross-channel hash scoping
describe block; migrate Slack pair tests from providerCredentialHashes
fixtures to plan-backed entries
- messaging-conflict.test.ts: remove hash-comparison tests (covered by
plan-entry tests); update remaining fixtures to drop providerCredentialHashes;
update file header comment
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…aths Completes the providerCredentialHashes deprecation started in the phase 4a conflict-detection migration. The field was written by onboard.ts, policy-channel.ts, and rebuild.ts, and read by messaging-credentials.ts and workflow-planner.ts for rotation detection and credential-availability inference. All write paths are removed. Reads are replaced with plan-backed equivalents: detectMessagingCredentialRotation now reads credentialBindings.credentialHash from the compiled SandboxMessagingPlan, and credentialAvailabilityFromSandboxEntry reads plan.credentialBindings[].credentialAvailable. The field is removed from SandboxEntry and MessagingWorkflowPlannerSandboxEntry entirely; existing sandboxes.json files with stale providerCredentialHashes entries are unaffected at runtime (unknown JSON fields are ignored). Tests updated to use plan-backed fixtures for matching-token conflict scenarios; credential-rotation tests use messaging.plan.credentialBindings fixtures. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…-messaging Both files shrank after removing providerCredentialHashes fixtures and assertions in the preceding refactor commit. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Take nemoclaw-start budget reduction from main (5310→5300) and keep onboard-messaging reduction from this branch (2122→2110). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 2 needs attention, 8 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
…ial-provider-plan # Conflicts: # src/lib/verify-deployment.ts
…-plan-driven' into feat/phase-4b-credential-provider-plan
wscurran
added
area: messaging
Contributor
|
✨ Related open issues: |
Base automatically changed from
feat/phase-4a-messaging-conflict-plan-driven
to
main
June 8, 2026 15:04
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
2 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
Stacked on #4908, this PR migrates messaging credential/provider setup from legacy token definitions to manifest-backed
SandboxMessagingPlan.credentialBindings. It keeps provider names, env keys, placeholders, and credential hashes plan-driven so create, reuse, rebuild, add, remove, rollback, and rotation checks all consume the compiled plan.Related Issue
Closes #4393. Part of #3896
Changes
buildMessagingPlanForSandboxfallback;createSandboxnow reads the setup-staged env plan or the persisted registry plan only.channels add/removeand rollback to use plan credential bindings and provider names instead of legacy channel token metadata.Type of Change
Verification
Focused checks run locally:
npm run build:clipassednpx vitest run --project cli src/lib/onboard/providers.test.ts src/lib/onboard/extra-placeholder-keys.test.ts src/lib/messaging/applier/setup-applier.test.ts src/lib/onboard/messaging-channel-setup.test.ts src/lib/actions/sandbox/policy-channel-conflict.test.ts test/credential-rotation.test.ts test/channels-add-preset.test.ts test/channels-remove-full-teardown.test.ts test/onboard-messaging.test.tspassed: 154 testsnpm run typecheck:clipassednpm run test-size:checkpassednpx prek run --files ...did not pass because the broadTest (CLI)hook still fails in unrelated timeout/assertion cases outside these changed files (test/nemoclaw-start.test.ts,test/policies.test.ts,test/secret-redaction.test.ts,test/cli/connect-recovery.test.ts,test/cli/doctor-gateway-token.test.ts)npx prek run --all-filespassesnpm testpassesTests added or updated for new or changed behavior
No secrets, API keys, or credentials committed
Docs updated for user-facing behavior changes
npm run docsbuilds 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)
Signed-off-by: San Dang sdang@nvidia.com