fix(tests): keep email-list hermetic; don't delete real secure-store credentials#25735
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 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); | ||
| }); |
There was a problem hiding this comment.
🟡 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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); | ||
| }); |
There was a problem hiding this comment.
🚩 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Addressed in #26266 — restored |
1 participant
Addresses Codex P2 feedback on #25714.
The
beforeEachadded in #25714 calleddeleteSecureKeyAsync(ASSISTANT_ID_CREDENTIAL)to guarantee the "missing assistant ID" fallback path was exercised. At the time,test-preloaddid not redirect the encrypted store, so this delete ran against the real backend at~/.vellum/protected/keys.encand wiped the developer's platform assistant ID on every run.Since then, #25717 isolated the encrypted store per test file via
_setStorePathintest-preload, so every test file gets a fresh empty store. The beforeEach delete is now redundant — the missing-ID fallback is already exercised by the empty temp store — and removing it eliminates the stale hazard entirely.