refactor(messaging): migrate conflict detection to manifest-plan architecture (phase 4a, #4392)#4908
Open
sandl99 wants to merge 31 commits into
Open
refactor(messaging): migrate conflict detection to manifest-plan architecture (phase 4a, #4392)#4908sandl99 wants to merge 31 commits into
sandl99 wants to merge 31 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>
|
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. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors messaging conflict detection from legacy hardcoded channel metadata to manifest/plan-driven credential hashing. It removes provider-hash field persistence, introduces new conflict-detection modules with request/entry/pair reasoning, rewires runtime conflict gates and channel management to use compiled plan data, and migrates all unit and integration tests to plan-backed fixtures and assertions. ChangesMessaging Conflict Detection Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
PR Review AdvisorFindings: 0 needs attention, 2 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. |
![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: 1
♻️ Duplicate comments (1)
src/lib/messaging-conflict.ts (1)
17-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import
createMessagingConflictProbefrom the import statement.The import of
createMessagingConflictProbeon line 19 is unused because line 28 performs a direct re-export from the same module. The static analysis correctly flags this redundancy.🧹 Proposed fix
import { backfillLegacyEntryChannels, - createMessagingConflictProbe, detectAllOverlapsInEntries, findConflictsInEntries, planToConflictChannelRequests, type ConflictMatch, type ConflictReason, type MessagingConflictProbe, } from "./messaging/applier";🤖 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/messaging-conflict.ts` around lines 17 - 28, The import list in src/lib/messaging-conflict.ts unnecessarily includes createMessagingConflictProbe even though that symbol is re-exported later; remove createMessagingConflictProbe from the named imports (the import that currently includes backfillLegacyEntryChannels, detectAllOverlapsInEntries, findConflictsInEntries, planToConflictChannelRequests, type ConflictMatch, type ConflictReason, type MessagingConflictProbe) and leave the explicit export export { createMessagingConflictProbe } from "./messaging/applier"; unchanged.Source: Linters/SAST tools
🤖 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/onboard.ts`:
- Around line 2808-2811: The checkGatewayLiveness implementation is using stdout
length to decide liveness (treating empty output as "down"); change
checkGatewayLiveness to determine success from the command exit status or thrown
error from runCaptureOpenshell instead of checking typeof result === "string" &&
result.length > 0. Locate the checkGatewayLiveness function and replace the
stdout-content check with a check of runCaptureOpenshell's success (e.g., handle
non-zero exit by catch/return false or inspect a returned status field) so a
successful exit with empty output still yields true; remove the string/length
condition and rely on the command exit semantics exposed by runCaptureOpenshell.
---
Duplicate comments:
In `@src/lib/messaging-conflict.ts`:
- Around line 17-28: The import list in src/lib/messaging-conflict.ts
unnecessarily includes createMessagingConflictProbe even though that symbol is
re-exported later; remove createMessagingConflictProbe from the named imports
(the import that currently includes backfillLegacyEntryChannels,
detectAllOverlapsInEntries, findConflictsInEntries,
planToConflictChannelRequests, type ConflictMatch, type ConflictReason, type
MessagingConflictProbe) and leave the explicit export export {
createMessagingConflictProbe } from "./messaging/applier"; unchanged.
🪄 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: b1a03bce-d211-43d0-a561-8b38d5167f2b
📒 Files selected for processing (5)
src/lib/messaging-conflict.test.tssrc/lib/messaging-conflict.tssrc/lib/messaging/applier/conflict-detection.tssrc/lib/messaging/applier/index.tssrc/lib/onboard.ts
- 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>
sandl99
added
refactor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/messaging/applier/conflict-detection.test.ts (1)
59-70: 💤 Low valueSimplify redundant boolean expression.
Line 67 contains
hash !== undefined || true, which always evaluates totrue. This is logically correct but unnecessarily complex. Since tests that needcredentialAvailable: falseexplicitly override it (line 195), and tests needingcredentialAvailable: truewith no hash also override it (line 176), consider simplifying to justcredentialAvailable: truefor clarity.♻️ Proposed simplification
function tgBinding(hash?: string): SandboxMessagingPlan["credentialBindings"][number] { return { channelId: "telegram", credentialId: "telegramBotToken", sourceInput: "botToken", providerName: "sb-telegram-bridge", providerEnvKey: "TELEGRAM_BOT_TOKEN", placeholder: "openshell:resolve:env:TELEGRAM_BOT_TOKEN", - credentialAvailable: hash !== undefined || true, + credentialAvailable: true, ...(hash !== undefined ? { credentialHash: hash } : {}), }; }🤖 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/messaging/applier/conflict-detection.test.ts` around lines 59 - 70, The helper tgBinding currently sets credentialAvailable using the redundant expression `hash !== undefined || true`; replace that with the simple literal `credentialAvailable: true` in the tgBinding function so the value is explicit and clearer (tests that need false/true already override credentialAvailable where needed).
🤖 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/onboard.ts`:
- Around line 2882-2884: The reuse path currently returns after
upsertMessagingProviders() and never applies extraPlaceholderKeys from
registerExtraPlaceholderProviders(sandboxName,...), so Brave Search (and similar
providers) are registered in the gateway but the sandbox env never receives the
placeholder declarations; fix by adding a reuse-time update: after
upsertMessagingProviders() in the ready-sandbox reuse branch, if
extraPlaceholderKeys.length > 0 either call the same sandbox-env update routine
used by create (e.g. the function that writes env vars/placeholders — add/update
sandbox env variables) to merge extraPlaceholderKeys into the sandbox
environment, or if no such update routine exists, treat non-empty
extraPlaceholderKeys as rebuild drift and force a sandbox rebuild/recreate
instead of returning early; reference extraPlaceholderKeys,
registerExtraPlaceholderProviders and upsertMessagingProviders when locating
where to apply this change.
---
Nitpick comments:
In `@src/lib/messaging/applier/conflict-detection.test.ts`:
- Around line 59-70: The helper tgBinding currently sets credentialAvailable
using the redundant expression `hash !== undefined || true`; replace that with
the simple literal `credentialAvailable: true` in the tgBinding function so the
value is explicit and clearer (tests that need false/true already override
credentialAvailable where needed).
🪄 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: c3dd64f4-8cd7-4ddc-955a-7c53b8aa680a
📒 Files selected for processing (6)
src/lib/messaging-conflict.test.tssrc/lib/messaging-conflict.tssrc/lib/messaging/applier/conflict-detection.test.tssrc/lib/messaging/applier/conflict-detection.tssrc/lib/messaging/compiler/engines/credential-binding-engine.tssrc/lib/onboard.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/messaging-conflict.test.ts
- src/lib/messaging-conflict.ts
…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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard.ts (1)
2777-2787:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't gate plan-driven conflict detection on
credentialAvailable.This skips
findChannelConflictsFromPlan()entirely unless at least one binding already has a resolved hash. That drops the conservative unknown-token warning path for plans that enable token-backed channels but could not resolve hashes yet, so a conflicting sandbox can slip past pre-commit checks.Suggested change
- const hasPlanCredentials = currentPlan?.credentialBindings.some((b) => b.credentialAvailable) ?? false; - if (hasPlanCredentials) { + if (currentPlan) { const { backfillMessagingChannels, findChannelConflictsFromPlan, createMessagingConflictProbe } = require("./messaging/applier") as typeof import("./messaging/applier");If you want to keep a fast path, gate on whether the plan yields any active conflict requests, not on whether a hash is already present. As per coding guidelines,
src/lib/onboard.tschanges affect the full sandbox creation and configuration flow.🤖 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/onboard.ts` around lines 2777 - 2787, The code currently skips calling backfillMessagingChannels and findChannelConflictsFromPlan when hasPlanCredentials (computed from currentPlan?.credentialBindings.some(b => b.credentialAvailable)) is false, which suppresses conservative unknown-token conflict detection; change the gating so conflict detection runs regardless of credentialAvailable — either remove the hasPlanCredentials conditional and always construct the probe and call backfillMessagingChannels(registry, probe) and findChannelConflictsFromPlan(sandboxName, currentPlan!, registry), or replace the condition with a check that the plan actually declares any token-backed channel requests (not whether hashes are already resolved) so that findChannelConflictsFromPlan and createMessagingConflictProbe (and their dependencies runOpenshell and providerExistsInGateway) still execute when the plan could yield conflicts.Source: Coding guidelines
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
2779-2809: Run the onboarding E2E slice before merge.This file now changes both messaging conflict checks and resume-state bookkeeping. I'd run at least
cloud-e2e,sandbox-operations-e2e,channels-stop-start-e2e,messaging-compatible-endpoint-e2e, andissue-3600-gpu-proof-optional-e2eon this branch.As per coding guidelines,
src/lib/onboard.tscontains core onboarding logic and the listed E2Es are the recommended selective coverage.Also applies to: 5794-5799, 6053-6099, 6365-6383
🤖 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/onboard.ts` around lines 2779 - 2809, The onboarding change touches messaging conflict checks (createMessagingConflictProbe, backfillMessagingChannels, findChannelConflictsFromPlan and related calls like runOpenshell and providerExistsInGateway) and resume-state bookkeeping; before merging, run the onboarding E2E slice and the specific tests listed (cloud-e2e, sandbox-operations-e2e, channels-stop-start-e2e, messaging-compatible-endpoint-e2e, issue-3600-gpu-proof-optional-e2e) to validate behavior, fix any failures uncovered (adjust probe logic, conflict messaging, or resume-state handling), and add/adjust E2E coverage where missing to ensure backfillMessagingChannels, conflict reporting, promptYesNoOrDefault/isNonInteractive flows, and cliName guidance behave correctly under CI.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.
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 2777-2787: The code currently skips calling
backfillMessagingChannels and findChannelConflictsFromPlan when
hasPlanCredentials (computed from currentPlan?.credentialBindings.some(b =>
b.credentialAvailable)) is false, which suppresses conservative unknown-token
conflict detection; change the gating so conflict detection runs regardless of
credentialAvailable — either remove the hasPlanCredentials conditional and
always construct the probe and call backfillMessagingChannels(registry, probe)
and findChannelConflictsFromPlan(sandboxName, currentPlan!, registry), or
replace the condition with a check that the plan actually declares any
token-backed channel requests (not whether hashes are already resolved) so that
findChannelConflictsFromPlan and createMessagingConflictProbe (and their
dependencies runOpenshell and providerExistsInGateway) still execute when the
plan could yield conflicts.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 2779-2809: The onboarding change touches messaging conflict checks
(createMessagingConflictProbe, backfillMessagingChannels,
findChannelConflictsFromPlan and related calls like runOpenshell and
providerExistsInGateway) and resume-state bookkeeping; before merging, run the
onboarding E2E slice and the specific tests listed (cloud-e2e,
sandbox-operations-e2e, channels-stop-start-e2e,
messaging-compatible-endpoint-e2e, issue-3600-gpu-proof-optional-e2e) to
validate behavior, fix any failures uncovered (adjust probe logic, conflict
messaging, or resume-state handling), and add/adjust E2E coverage where missing
to ensure backfillMessagingChannels, conflict reporting,
promptYesNoOrDefault/isNonInteractive flows, and cliName guidance behave
correctly under CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 557fd409-bfb3-45cf-9fe2-82d24983069f
📒 Files selected for processing (23)
ci/test-file-size-budget.jsonsrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/inventory/index.tssrc/lib/messaging-conflict.tssrc/lib/messaging/applier/conflict-detection-entry.test.tssrc/lib/messaging/applier/conflict-detection-legacy.test.tssrc/lib/messaging/applier/conflict-detection-plan.test.tssrc/lib/messaging/applier/conflict-detection.tssrc/lib/messaging/compiler/workflow-planner.test.tssrc/lib/messaging/compiler/workflow-planner.tssrc/lib/onboard.tssrc/lib/onboard/messaging-credentials.tssrc/lib/state/registry.tssrc/lib/status-command-deps.tstest/channels-add-preset.test.tstest/channels-remove-full-teardown.test.tstest/cli/status-health.test.tstest/credential-rotation.test.tstest/e2e/test-token-rotation.shtest/onboard-messaging.test.tstest/rebuild-credential-preflight.test.ts
💤 Files with no reviewable changes (9)
- src/lib/inventory/index.ts
- src/lib/messaging/compiler/workflow-planner.test.ts
- test/rebuild-credential-preflight.test.ts
- src/lib/actions/sandbox/rebuild.ts
- src/lib/messaging-conflict.ts
- src/lib/state/registry.ts
- test/channels-remove-full-teardown.test.ts
- test/onboard-messaging.test.ts
- test/channels-add-preset.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/status-command-deps.ts
- ci/test-file-size-budget.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/messaging/applier/conflict-detection-plan.test.ts
12 tasks
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27115849905
|
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
cv
approved these changes
Jun 8, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/actions/sandbox/policy-channel.ts (1)
401-403: ⚡ Quick winUse the existing module-level registry instead of creating a new instance.
messagingManifestRegistryis already defined at module scope (line 73) and used byresolveChannelManifest(). Creating a new registry here is redundant and inconsistent with the rest of the file.♻️ Suggested fix
- const channelManifest = createBuiltInChannelManifestRegistry() - .list() - .find((m) => m.id === channelName); + const channelManifest = messagingManifestRegistry.get(channelName);🤖 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/policy-channel.ts` around lines 401 - 403, The code creates a new registry via createBuiltInChannelManifestRegistry() to find channelManifest; instead use the existing module-level messagingManifestRegistry (already used by resolveChannelManifest()) to avoid redundancy and keep consistency—replace the createBuiltInChannelManifestRegistry().list().find(...) lookup with a lookup against messagingManifestRegistry (e.g., use its .list()/.find() or its existing resolver method) so channelManifest is obtained from the shared messagingManifestRegistry.
🤖 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/applier/conflict-detection-entry.ts`:
- Around line 94-95: Zero-key credential comparisons in the
conflict-detection-entry logic are currently treated as "unknown-token", which
causes false conflicts for credential-less channels. Update both branches that
return "unknown-token" when keys.length === 0 so that the no-credential case is
handled as a no-op/non-conflicting result instead of producing a synthetic
token; make the same change in both occurrences called out in this file so
overlap/conflict detection skips channels with no comparison keys.
---
Nitpick comments:
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 401-403: The code creates a new registry via
createBuiltInChannelManifestRegistry() to find channelManifest; instead use the
existing module-level messagingManifestRegistry (already used by
resolveChannelManifest()) to avoid redundancy and keep consistency—replace the
createBuiltInChannelManifestRegistry().list().find(...) lookup with a lookup
against messagingManifestRegistry (e.g., use its .list()/.find() or its existing
resolver method) so channelManifest is obtained from the shared
messagingManifestRegistry.
🪄 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: d04c186f-f936-4a91-8eab-64d19e66296a
📒 Files selected for processing (31)
ci/test-file-size-budget.jsonsrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/inventory/index.tssrc/lib/messaging-conflict.tssrc/lib/messaging/applier/conflict-detection-backfill.tssrc/lib/messaging/applier/conflict-detection-entry.test.tssrc/lib/messaging/applier/conflict-detection-entry.tssrc/lib/messaging/applier/conflict-detection-legacy.test.tssrc/lib/messaging/applier/conflict-detection-manifest.tssrc/lib/messaging/applier/conflict-detection-multi-credential.test.tssrc/lib/messaging/applier/conflict-detection-overlap.test.tssrc/lib/messaging/applier/conflict-detection-plan.test.tssrc/lib/messaging/applier/conflict-detection-plan.tssrc/lib/messaging/applier/conflict-detection-types.tssrc/lib/messaging/applier/conflict-detection.tssrc/lib/messaging/compiler/workflow-planner.test.tssrc/lib/messaging/compiler/workflow-planner.tssrc/lib/onboard.tssrc/lib/onboard/messaging-credentials.tssrc/lib/state/registry.tssrc/lib/status-command-deps.tstest/channels-add-preset.test.tstest/channels-remove-full-teardown.test.tstest/cli/status-health.test.tstest/credential-rotation.test.tstest/e2e/test-token-rotation.shtest/helpers/messaging-conflict-fixtures.tstest/onboard-messaging.test.tstest/rebuild-credential-preflight.test.ts
💤 Files with no reviewable changes (9)
- src/lib/actions/sandbox/rebuild.ts
- src/lib/messaging-conflict.ts
- test/channels-remove-full-teardown.test.ts
- src/lib/messaging/compiler/workflow-planner.test.ts
- test/rebuild-credential-preflight.test.ts
- test/onboard-messaging.test.ts
- src/lib/inventory/index.ts
- test/channels-add-preset.test.ts
- src/lib/state/registry.ts
✅ Files skipped from review due to trivial changes (1)
- ci/test-file-size-budget.json
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27118468378
|
Signed-off-by: San Dang <sdang@nvidia.com>
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
Migrates cross-sandbox messaging credential conflict detection to the manifest-plan architecture (phase 4a of #3896). Core detection logic now lives in
src/lib/messaging/applier/conflict-detection.ts; the legacysrc/lib/messaging-conflict.tsadapter has been removed; and current callers import the conflict helpers fromsrc/lib/messaging/applier.onboard.tsnow reads conflict requests directly from the compiledSandboxMessagingPlan, so conflict detection is plan-driven rather than built from legacy channel constants.Related Issue
Closes #4392. Part of #3896.
Changes
src/lib/messaging/applier/conflict-detection.tscontains the conflict detection surface: tri-state gateway probe factory, plan-to-request helpers, active-channel resolution, registry-backed conflict/overlap helpers, and legacy channel-presence backfill.src/lib/messaging/applier/index.tsre-exports the conflict-detection helpers for callers.src/lib/messaging-conflict.tswas deleted;onboard.ts,status-command-deps.ts, andpolicy-channel.tsnow import frommessaging/applier.src/lib/messaging/applier/conflict-detection-legacy.test.ts; plan and entry behavior remain covered by the applier tests.Type of Change
Verification
npm run typecheck:clinpx vitest run --project cli src/lib/messaging/applier/conflict-detection-entry.test.ts src/lib/messaging/applier/conflict-detection-plan.test.ts src/lib/messaging/applier/conflict-detection-legacy.test.ts src/lib/actions/sandbox/policy-channel-conflict.test.ts src/lib/status-command-deps.test.tsbash -n test/e2e/test-token-rotation.shgit diff --checknpm test/ full CLI suite: unrelatedtest/cli/list-inference.test.tstimeout reproduced under suite loadSigned-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit