Skip to content

Commit 1763792

Browse files
prekshivyascv
andauthored
fix(messaging): retain stopped channel providers (NVIDIA#10052)
<!-- markdownlint-disable MD041 --> ## Summary PR NVIDIA#10047 retained a stopped Discord provider only when Discord also appeared in the active channel list. Sandbox rebuilds exclude stopped channels from that list, so this change retains the exact existing static provider for the actual rebuild state. ## Changes - Include retained static provider definitions when a channel is stopped and absent from `enabledChannels`. - Continue to exclude stopped-channel token definitions and runtime activation. - Test the production state with `enabledChannels: []` and `disabledChannels: ["discord"]`. - Confirm that active and stopped channels reject a provider with the wrong type. ## Type of Change - [x] 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Exact provider name, type, and credential-key matching remains required. The regression test confirms that mismatched active and stopped providers remain detached. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli src/lib/onboard/messaging-prep.test.ts src/lib/onboard/sandbox-messaging-preflight.test.ts` (29 passed); `npx vitest run --project integration test/hermes-discord-credential-binding.test.ts` (7 passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Preserved static provider credentials when a messaging channel is disabled but retained in the selected configuration. * Improved provider matching for disabled channels while preventing reuse of incompatible or untyped providers. * Added coverage for both active and stopped channel states. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
1 parent b6e5936 commit 1763792

2 files changed

Lines changed: 16 additions & 5 deletions

File tree

src/lib/onboard/messaging-prep.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,12 @@ export function prepareCreateSandboxMessaging(
102102
retainWhileDisabled: staticProviderType !== null,
103103
};
104104
})
105-
.filter(({ envKey }) => !enabledEnvKeys || enabledEnvKeys.has(envKey));
105+
.filter(
106+
({ envKey, retainWhileDisabled }) =>
107+
!enabledEnvKeys ||
108+
enabledEnvKeys.has(envKey) ||
109+
(retainWhileDisabled && disabledEnvKeys.has(envKey)),
110+
);
106111
const messagingTokenDefs: MessagingTokenDef[] = messagingCredentialDefs
107112
.filter(({ envKey }) => !disabledEnvKeys.has(envKey))
108113
.map(({ retainWhileDisabled: _retainWhileDisabled, ...definition }) => definition);
@@ -196,8 +201,11 @@ export function prepareCreateSandboxMessaging(
196201
retainWhileDisabled,
197202
} of messagingCredentialDefs) {
198203
const channel = input.getMessagingChannelForEnvKey(envKey);
199-
if (!channel || !input.enabledChannels.includes(channel)) continue;
204+
if (!channel) continue;
200205
const channelDisabled = disabledChannelNames.has(channel);
206+
if (!input.enabledChannels.includes(channel) && !(channelDisabled && retainWhileDisabled)) {
207+
continue;
208+
}
201209
if (channelDisabled && !retainWhileDisabled) continue;
202210
// Disabled definitions are intentionally absent from messagingTokenDefs,
203211
// so even a still-readable source token cannot recreate their provider.

test/hermes-discord-credential-binding.test.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ function prepareDiscord(
2525
sandboxName: SANDBOX_NAME,
2626
agentName: "hermes",
2727
channels: discord,
28-
enabledChannels: ["discord"],
28+
enabledChannels: disabled ? [] : ["discord"],
2929
disabledChannels: disabled ? ["discord"] : [],
3030
webSearchConfig: null,
3131
env: token ? { DISCORD_BOT_TOKEN: token } : {},
@@ -66,9 +66,12 @@ describe("Hermes Discord credential endpoint binding", () => {
6666
expect(profileYaml.endpoints).toEqual([]);
6767
});
6868

69-
it("does not reuse an untyped provider for the endpoint-bound credential", () => {
69+
it.each([
70+
{ state: "active", disabled: false },
71+
{ state: "stopped", disabled: true },
72+
])("does not reuse an untyped provider for a $state channel", ({ disabled }) => {
7073
const providerMatches = vi.fn(() => false);
71-
const result = prepareDiscord(null, providerMatches);
74+
const result = prepareDiscord(null, providerMatches, disabled);
7275

7376
expect(result.reusableMessagingProviders).toEqual([]);
7477
expect(result.reusableMessagingChannels).toEqual([]);

0 commit comments

Comments
 (0)