Skip to content

fix(tests): keep email-list hermetic; don't delete real secure-store credentials #25735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
siddseethepalli merged 1 commit into from
Apr 15, 2026
Merged

fix(tests): keep email-list hermetic; don't delete real secure-store credentials #25735

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions assistant/src/cli/commands/__tests__/email-list.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { runAssistantCommand } from "../../__tests__/run-assistant-command.js";

const ASSISTANT_ID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee";
const API_KEY_CREDENTIAL = credentialKey("vellum", "assistant_api_key");
const ASSISTANT_ID_CREDENTIAL = credentialKey("vellum", "platform_assistant_id");

/**
* Return the recorded fetch calls, excluding the feature-flag fetch that
Expand Down Expand Up @@ -84,10 +83,6 @@ beforeEach(async () => {
_setOverridesForTesting({ "email-channel": true });
setPlatformAssistantId(ASSISTANT_ID);
await setSecureKeyAsync(API_KEY_CREDENTIAL, "test-api-key");
// Ensure the credential store does not contain a stray platform_assistant_id
// from dev machine state — the "missing assistant ID" test relies on the
// fallback lookup returning empty.
await deleteSecureKeyAsync(ASSISTANT_ID_CREDENTIAL);
});
Comment on lines 85 to 86

@devin-ai-integration devin-ai-integration Bot Apr 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Removing deleteSecureKeyAsync for platform_assistant_id breaks test isolation on developer machines

The removed deleteSecureKeyAsync(ASSISTANT_ID_CREDENTIAL) call in beforeEach was specifically guarding the "missing assistant ID returns error" test (line 201–209). When setPlatformAssistantId("") is called, getPlatformAssistantId() returns "". In VellumPlatformClient.create() (assistant/src/platform/client.ts:60-67), !assistantId evaluates to true for "", triggering a fallback that reads platform_assistant_id from the encrypted credential store on disk. On developer machines that have a real platform_assistant_id stored in ~/.vellum/protected/keys.enc, this fallback returns the real value, the client gets a non-empty platformAssistantId, the !client.platformAssistantId check at assistant/src/cli/commands/email.ts:364 passes, and the test fails because the expected error is never thrown. The removed comment explicitly documented this: "the 'missing assistant ID' test relies on the fallback lookup returning empty."

Prompt for agents 
The deleteSecureKeyAsync call for platform_assistant_id was removed from beforeEach, but VellumPlatformClient.create() in assistant/src/platform/client.ts:60-67 falls back to reading platform_assistant_id from the encrypted credential store when getPlatformAssistantId() returns an empty string. On developer machines with real credentials stored, this breaks the 'missing assistant ID returns error' test at line 201.

To fix this while respecting the PR's goal of not deleting real credentials, consider one of:
1. Restore the deleteSecureKeyAsync call but add a corresponding restore in afterEach (read before delete, write back after).
2. Mock the credential store backend for this test file so it never hits the real encrypted store.
3. Within the 'missing assistant ID' test specifically, delete the platform_assistant_id credential and restore it after the assertion.

The other email test files (e.g. email-send.test.ts:61-63) still delete platform_assistant_id in their beforeEach, so this file is now inconsistent with the established pattern.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 85 to 86

@devin-ai-integration devin-ai-integration Bot Apr 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Inconsistency with sibling email test files

The email-send.test.ts:61-63 still explicitly deletes PLATFORM_ASSISTANT_ID_CREDENTIAL (along with API_KEY_CREDENTIAL and PLATFORM_BASE_URL_CREDENTIAL) in its beforeEach, and restores/cleans up in afterEach (email-send.test.ts:74-76). This PR makes email-list.test.ts inconsistent with that pattern. If the goal is to stop deleting real credentials, the approach should be applied uniformly across all email test files — or, if the other files need the deletion for correctness (as argued in the bug report), this file should retain it too.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


afterEach(() => {
Expand Down
Loading