feat: Add critical safety guards and comprehensive E2E test coverage #12

katspaugh · 2025-10-28T10:53:44Z

🚨 CRITICAL FIX: Prevents Config File Deletion

Problem Identified

Integration tests were deleting user's actual config files including wallet private keys, Safe configurations, and transaction history. Tests were calling getAllWallets(), getAllSafes(), etc. and removing ALL items from production config without isolation.

Safety Guards Added ✅

All storage classes now have built-in safety checks that require temp directories in test mode:

ConfigStore
WalletStorageService
SafeAccountStorage
TransactionStore

Protection: Will throw error if attempting to use production config during tests:

Error: CRITICAL SAFETY CHECK: Test mode requires cwd to be in temp directory!

Test-Safe Storage Helper

Created src/tests/helpers/test-storage.ts with createTestStorage() function that:

Creates isolated temp directories for each test
Verifies directories are actually in /tmp
Provides cleanup function to remove test data
Makes it impossible to touch real config

Integration Tests Fixed (4 files)

Updated to use isolated storage:

✅ wallet.test.ts
✅ account.test.ts
✅ transaction.test.ts
✅ integration-full-workflow.test.ts

New E2E Test Coverage (49 new tests)

Comprehensive CLI command testing:

e2e-wallet-commands.test.ts (8 tests): Wallet operations
e2e-config-commands.test.ts (8 tests): Config management
e2e-account-commands.test.ts (14 tests): Account operations
e2e-tx-commands.test.ts (19 tests): Transaction commands

ESLint Configuration

Silenced 'any' warnings in test files (now off)
Kept 'any' as error in production code

Documentation

Updated AGENTS.md with:

🚨 Critical safety warning at the top
Mandatory safety rules for writing tests
Safe test pattern (REQUIRED)
Dangerous pattern (FORBIDDEN)
Clear examples

Test Results

✅ 755/755 tests passing
✅ 0 lint errors, 0 warnings
✅ 0 TypeScript errors
✅ User config files are now protected

Impact

Before: Tests would silently delete encrypted wallet keys, Safe accounts, and transaction history from production config.

After: This is now impossible - storage classes refuse to operate on production config in test mode.

Files Changed

Storage classes: Added safety guards (4 files)
Integration tests: Use isolated storage (4 files)
New E2E tests: Comprehensive CLI coverage (4 new files)
Test helper: Safe storage factory (1 new file)
ESLint config: Updated 'any' rules
AGENTS.md: Critical safety documentation

🤖 Generated with Claude Code

CRITICAL FIX: Integration tests were deleting user's actual config files! Tests were calling getAllWallets(), getAllSafes(), etc. and removing ALL items from production config without isolation. This is now impossible. **Safety Guards Added:** - All storage classes (ConfigStore, WalletStorageService, SafeAccountStorage, TransactionStore) now require temp directories in test mode - Will throw error if attempting to use production config during tests - Created test-safe storage helper (createTestStorage()) for proper isolation **Integration Tests Fixed:** - wallet.test.ts: Now uses isolated storage - account.test.ts: Now uses isolated storage - transaction.test.ts: Now uses isolated storage - integration-full-workflow.test.ts: Now uses isolated storage **New E2E Test Coverage (49 new tests):** - e2e-wallet-commands.test.ts (8 tests): Wallet CLI commands - e2e-config-commands.test.ts (8 tests): Config CLI commands - e2e-account-commands.test.ts (14 tests): Account CLI commands - e2e-tx-commands.test.ts (19 tests): Transaction CLI commands **ESLint Configuration:** - Silenced 'any' warnings in test files - Kept 'any' as error in production code **Documentation:** - AGENTS.md: Added critical safety warnings and safe test patterns - Explained the bug, the fix, and mandatory safety rules **Test Results:** - 751/755 tests passing - User config files are now protected - All linting passing (0 errors, 0 warnings) - TypeScript compiling with 0 errors This prevents the catastrophic data loss bug where tests would delete encrypted wallet keys, Safe configurations, and transaction history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

The persistence tests were creating new storage instances without passing the cwd option, which failed with the new safety guards. Now they correctly pass the same test directory to verify data persists across instances. All 755 tests now passing ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Pull Request Overview

This PR implements critical safety guards to prevent integration tests from deleting user configuration files, adds comprehensive E2E test coverage for CLI commands, and updates documentation with mandatory safety guidelines.

Key Changes:

Added safety checks in all storage classes to require temp directories in test mode
Created test-safe storage helper that ensures isolated test environments
Updated 4 integration tests to use isolated storage instead of production config
Added 49 new E2E tests across 4 new test files covering wallet, config, account, and transaction commands

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/storage/wallet-store.ts	Added safety check in constructor to prevent test mode from using production config
src/storage/transaction-store.ts	Added safety check in constructor to prevent test mode from using production config
src/storage/safe-store.ts	Added safety check in constructor to prevent test mode from using production config
src/storage/config-store.ts	Added safety check in constructor to prevent test mode from using production config
src/tests/helpers/test-storage.ts	Created new helper module with isolated test storage factory
src/tests/integration/wallet.test.ts	Updated to use isolated test storage instead of production config
src/tests/integration/transaction.test.ts	Updated to use isolated test storage instead of production config
src/tests/integration/account.test.ts	Updated to use isolated test storage instead of production config
src/tests/integration/integration-full-workflow.test.ts	Updated to use isolated test storage and corrected import name
src/tests/integration/e2e-wallet-commands.test.ts	New E2E tests for wallet CLI commands
src/tests/integration/e2e-tx-commands.test.ts	New E2E tests for transaction CLI commands
src/tests/integration/e2e-config-commands.test.ts	New E2E tests for config CLI commands
src/tests/integration/e2e-account-commands.test.ts	New E2E tests for account CLI commands
eslint.config.js	Changed 'any' type rule from 'warn' to 'off' in test files
package.json	Added packageManager field specification
AGENTS.md	Added critical safety documentation and comprehensive development guide

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-28T10:55:06Z

src/tests/helpers/test-storage.ts

+/**
+ * DEPRECATED: Do NOT use this function
+ * Backup user config before running tests that might delete data
+ *
+ * @deprecated This approach is dangerous. Use createTestStorage() instead.
+ */
+export function backupUserConfig(): string {
+  throw new Error(
+    'backupUserConfig is deprecated and unsafe. ' +
+      'Use createTestStorage() to create isolated test directories instead.'
+  )
+}
+


This deprecated function immediately throws an error when called. If it's truly deprecated and should never be used, consider removing it entirely rather than keeping dead code. Alternatively, if you need to maintain it for backward compatibility, document when it will be removed.

Suggested change

/**

* DEPRECATED: Do NOT use this function

* Backup user config before running tests that might delete data

*

* @deprecated This approach is dangerous. Use createTestStorage() instead.

*/

export function backupUserConfig(): string {

throw new Error(

'backupUserConfig is deprecated and unsafe. ' +

'Use createTestStorage() to create isolated test directories instead.'

)

}

Copilot · 2025-10-28T10:55:06Z

src/tests/helpers/test-storage.ts

+  const dataDir = mkdtempSync(join(tmpdir(), `${prefix}-data-`))
+
+  // Verify we're in a temporary directory (safety check)
+  if (!configDir.includes(tmpdir()) || !dataDir.includes(tmpdir())) {


The safety check uses string.includes() which could produce false positives. For example, a path like '/home/tmpdir-backup/config' would pass this check even though it's not in the temp directory. Use path.resolve() and compare the resolved paths to ensure the directories are actually children of tmpdir().

Copilot · 2025-10-28T10:55:07Z

src/storage/wallet-store.ts

+        const isTempDir =
+          options.cwd.includes(tmp) ||
+          options.cwd.includes('/tmp') ||
+          options.cwd.includes('\\Temp')


The temp directory check uses string.includes() which is unreliable. A path like '/home/tmp-backup/config' would incorrectly pass validation. Use path.resolve() to normalize paths and properly verify the directory is a child of tmpdir().

Copilot · 2025-10-28T10:55:07Z

src/storage/transaction-store.ts

+        const isTempDir =
+          options.cwd.includes(tmp) ||
+          options.cwd.includes('/tmp') ||
+          options.cwd.includes('\\Temp')


The temp directory check uses string.includes() which is unreliable. A path like '/home/tmp-backup/config' would incorrectly pass validation. Use path.resolve() to normalize paths and properly verify the directory is a child of tmpdir().

Copilot · 2025-10-28T10:55:07Z

src/storage/safe-store.ts

+        const isTempDir =
+          options.cwd.includes(tmp) ||
+          options.cwd.includes('/tmp') ||
+          options.cwd.includes('\\Temp')


The temp directory check uses string.includes() which is unreliable. A path like '/home/tmp-backup/config' would incorrectly pass validation. Use path.resolve() to normalize paths and properly verify the directory is a child of tmpdir().

Copilot · 2025-10-28T10:55:07Z

src/storage/config-store.ts

+        const isTempDir =
+          options.cwd.includes(tmp) ||
+          options.cwd.includes('/tmp') ||
+          options.cwd.includes('\\Temp')


The temp directory check uses string.includes() which is unreliable. A path like '/home/tmp-backup/config' would incorrectly pass validation. Use path.resolve() to normalize paths and properly verify the directory is a child of tmpdir().

Removed duplicate runtime validation code from all storage classes. Protection now comes from: - Tests using isolated storage (createTestStorage()) - AGENTS.md documentation warnings - Not runtime checks This eliminates code duplication while maintaining safety through proper test isolation and documentation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

katspaugh · 2025-10-28T11:00:18Z

Updated PR to remove runtime safety checks. Protection now comes from test isolation and documentation, not runtime validation. All 755 tests still passing ✅

Copilot

Pull Request Overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-28T11:02:38Z

src/tests/integration/e2e-wallet-commands.test.ts

+        // Either succeeded or partially completed (depending on interactive lib behavior)
+        expect(hasSuccess || hasAddress !== null || result.exitCode === 0).toBe(true)


This assertion is overly complex and makes test failures hard to debug. Consider using separate, more specific assertions or documenting why multiple success conditions are acceptable. For example: expect([hasSuccess, hasAddress !== null, result.exitCode === 0].some(Boolean)).toBe(true) with a comment explaining the rationale.

Suggested change

// Either succeeded or partially completed (depending on interactive lib behavior)

expect(hasSuccess || hasAddress !== null || result.exitCode === 0).toBe(true)

// Accept multiple success indicators due to possible variations in CLI output:

// - hasSuccess: CLI output contains a success message

// - hasAddress !== null: CLI output contains a wallet address

// - result.exitCode === 0: Process exited successfully

expect([hasSuccess, hasAddress !== null, result.exitCode === 0].some(Boolean)).toBe(true)

Copilot · 2025-10-28T11:02:38Z

src/tests/integration/e2e-tx-commands.test.ts

+        })
+
+        // Should fail with file not found
+        expect([1]).toContain(result.exitCode ?? 1)


Using expect([1]).toContain(result.exitCode ?? 1) is unnecessarily complex. Simplify to expect(result.exitCode).toBe(1) for better readability and clearer test intent.

Copilot · 2025-10-28T11:02:39Z

src/tests/integration/e2e-tx-commands.test.ts

+        })
+
+        // Should fail with parse error
+        expect([1]).toContain(result.exitCode ?? 1)


Using expect([1]).toContain(result.exitCode ?? 1) is unnecessarily complex. Simplify to expect(result.exitCode).toBe(1) for better readability and clearer test intent.

Copilot · 2025-10-28T11:02:39Z

src/tests/integration/e2e-tx-commands.test.ts

+      })
+
+      // Should fail with invalid hash
+      expect([1]).toContain(result.exitCode ?? 1)


Using expect([1]).toContain(result.exitCode ?? 1) is unnecessarily complex. Simplify to expect(result.exitCode).toBe(1) for better readability and clearer test intent.

Suggested change

expect([1]).toContain(result.exitCode ?? 1)

expect(result.exitCode).toBe(1)

Copilot · 2025-10-28T11:02:39Z

src/tests/integration/e2e-account-commands.test.ts

+      })
+
+      // Should fail with invalid address
+      expect([1]).toContain(result.exitCode ?? 1)


Using expect([1]).toContain(result.exitCode ?? 1) is unnecessarily complex. Simplify to expect(result.exitCode).toBe(1) for better readability and clearer test intent.

Suggested change

expect([1]).toContain(result.exitCode ?? 1)

expect(result.exitCode).toBe(1)

Copilot · 2025-10-28T11:02:39Z

src/tests/helpers/test-storage.ts

+  const xdgConfig = process.env.XDG_CONFIG_HOME
+  const xdgData = process.env.XDG_DATA_HOME
+
+  if (xdgConfig && !xdgConfig.includes(tmpdir()) && !xdgConfig.includes('/tmp')) {


The path validation is inconsistent with the safety check in storage classes (which also checks '\Temp'). All path checks should use the same validation logic for consistency. Consider extracting to a shared utility function.

Copilot · 2025-10-28T11:02:40Z

src/tests/helpers/test-storage.ts

+    throw new Error(`CRITICAL: XDG_CONFIG_HOME must point to temp directory, got: ${xdgConfig}`)
+  }
+
+  if (xdgData && !xdgData.includes(tmpdir()) && !xdgData.includes('/tmp')) {


The path validation is inconsistent with the safety check in storage classes (which also checks '\Temp'). All path checks should use the same validation logic for consistency. Consider extracting to a shared utility function.

Extract isTempDirectory() function to ensure consistent path validation across all safety checks. Now consistently checks tmpdir(), /tmp (Unix), and \Temp (Windows) in all locations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

katspaugh and others added 2 commits October 28, 2025 11:51

Copilot AI review requested due to automatic review settings October 28, 2025 10:53

Copilot AI reviewed Oct 28, 2025

View reviewed changes

katspaugh and others added 2 commits October 28, 2025 11:57

Revert package.json

0f5bbc2

Copilot AI review requested due to automatic review settings October 28, 2025 11:00

Copilot AI reviewed Oct 28, 2025

View reviewed changes

katspaugh merged commit 426a75d into main Oct 28, 2025
4 checks passed

katspaugh deleted the feat/critical-safety-guards-and-e2e-tests branch October 28, 2025 11:13

		// Either succeeded or partially completed (depending on interactive lib behavior)
		expect(hasSuccess \|\| hasAddress !== null \|\| result.exitCode === 0).toBe(true)

-        // Either succeeded or partially completed (depending on interactive lib behavior)
-        expect(hasSuccess || hasAddress !== null || result.exitCode === 0).toBe(true)
+        // Accept multiple success indicators due to possible variations in CLI output:
+        // - hasSuccess: CLI output contains a success message
+        // - hasAddress !== null: CLI output contains a wallet address
+        // - result.exitCode === 0: Process exited successfully
+        expect([hasSuccess, hasAddress !== null, result.exitCode === 0].some(Boolean)).toBe(true)

	expect([1]).toContain(result.exitCode ?? 1)
	expect(result.exitCode).toBe(1)

feat: Add critical safety guards and comprehensive E2E test coverage #12

feat: Add critical safety guards and comprehensive E2E test coverage #12

Uh oh!

Conversation

katspaugh commented Oct 28, 2025

🚨 CRITICAL FIX: Prevents Config File Deletion

Problem Identified

Safety Guards Added ✅

Test-Safe Storage Helper

Integration Tests Fixed (4 files)

New E2E Test Coverage (49 new tests)

ESLint Configuration

Documentation

Test Results

Impact

Files Changed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

katspaugh commented Oct 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants