fix: address all external audit weaknesses with comprehensive improvements

This commit fixes all remaining weaknesses identified in the external audit:

1. **Fixed Dangerous process.env = {} Pattern**
   - Changed from wiping entire environment to preserving PATH, HOME, etc.
   - Now creates fresh copy and deletes only CI-specific vars
   - Prevents git/system tools from breaking in tests
   - Added afterAll() to restore original env
   - Files: engine.spec.ts, engine.gh-pages-integration.spec.ts,
     engine.prepare-options-helpers.spec.ts

2. **Consolidated Redundant getRemoteUrl Tests**
   - Merged 3 redundant success tests into 1 comprehensive test
   - Now verifies actual git config output matches (uses execSync)
   - Tests consistency across multiple calls
   - Reduced from 6 tests to 3 focused tests
   - File: engine.prepare-options-helpers.spec.ts

3. **Added Error Message Assertions**
   - "non-existent remote" now asserts: 'Failed to get remote.'
   - "not in git repo" now asserts: 'run in a git repository'
   - Pins error message contract, not just that errors occur
   - File: engine.prepare-options-helpers.spec.ts

4. **Strengthened CI Metadata Tests**
   - Single-CI tests now assert FULL expected message with .toBe()
   - Multi-CI test verifies ordering (Travis → CircleCI → GitHub Actions)
   - Replaced weak .toContain() checks with precise assertions
   - File: engine.prepare-options-helpers.spec.ts

5. **Updated CLAUDE.md Testing Policy**
   - Softened "ONLY use .toBe()" rule to match reality
   - Now allows .toContain() for:
     - Array membership checks
     - Substrings in long messages (where full equality is brittle)
   - Documents sensible middle ground we actually follow
   - File: CLAUDE.md

Result: All 351 tests passing, much safer and more precise test suite.

Author: JohannesHoppe

.claude/settings.local.json

@@ -32,7 +32,8 @@
       "Bash(cat:*)",
       "Bash(npm view:*)",
       "WebFetch(domain:raw.githubusercontent.com)",
-      "Bash(gh pr list:*)"
+      "Bash(gh pr list:*)",
+      "Bash(git add:*)"
     ],
     "deny": [],
     "ask": []

CLAUDE.md

@@ -205,46 +205,47 @@ Snapshot tests are stored in `__snapshots__/` directory.
 
 ### Testing Philosophy: Explicit Assertions
 
-**CRITICAL: All tests MUST use explicit assertions. Never use `.toContain()` for value testing.**
+**Prefer precise assertions with `.toBe()` for scalar values, but allow `.toContain()` where appropriate.**
 
 **Rules:**
 1. **Explicit expected values** - Always write out the exact expected value
-2. **Use `.toBe()` for exact equality** - Not `.toContain()` unless testing substring presence
-3. **Variable reuse for passthrough** - If input should equal output unchanged, use the same variable for both
-4. **Separate variables for transformations** - If input is transformed, use distinct `input` and `expected` variables
+2. **Use `.toBe()` for exact equality of scalars** - Strings, numbers, booleans
+3. **Allow `.toContain()` for:**
+   - **Array membership checks** - Verifying an item is in an array
+   - **Substring assertions in long messages** - Where full string equality would be brittle or unmaintainable
+4. **Variable reuse for passthrough** - If input should equal output unchanged, use the same variable for both
+5. **Separate variables for transformations** - If input is transformed, use distinct `input` and `expected` variables
 
 **Good examples:**
 ```typescript
-// ✅ Passthrough - same variable shows value doesn't change
+// ✅ Scalar equality - use .toBe()
 const repoUrl = 'https://github.com/test/repo.git';
-const options = { repo: repoUrl };
-const result = await prepareOptions(options, logger);
+const result = await prepareOptions({ repo: repoUrl }, logger);
 expect(result.repo).toBe(repoUrl);
 
-// ✅ Transformation - separate variables show what changes
-const inputUrl = 'https://github.com/test/repo.git';
-const token = 'secret_token';
-const expectedUrl = 'https://x-access-token:secret_token@github.com/test/repo.git';
-expect(result.repo).toBe(expectedUrl);
+// ✅ Array membership - .toContain() is appropriate
+const files = ['index.html', 'main.js', '.htaccess'];
+expect(files).toContain('.htaccess');
+
+// ✅ Substring in long CI message - .toContain() is appropriate
+expect(options.message).toContain('Travis CI build: https://travis-ci.org/');
+
+// ✅ Exact message for simple case - use .toBe()
+const expected = 'Deploy to gh-pages';
+expect(options.message).toBe(expected);
 ```
 
 **Bad examples:**
 ```typescript
-// ❌ Weak - doesn't verify exact value
-expect(result.repo).toContain('github.com');
+// ❌ Weak - doesn't verify exact value when you could
+expect(result.repo).toContain('github.com'); // Could use .toBe() with full URL
 
 // ❌ Unclear - is this supposed to change or not?
 const options = { branch: 'main' };
 expect(result.branch).toBe('main'); // Should use same variable!
-
-// ❌ Cheating - use .toBe() instead
-expect(result.message).toContain('Deploy');
-expect(result.message).toMatch(/Deploy/);
-expect(result.message).toStartWith('Deploy');
-expect(result.message.startsWith('Deploy')).toBe(true);
 ```
 
-**ONLY use `.toBe()` for value assertions.** Other matchers like `.toContain()`, `.toMatch()`, `.toStartWith()` are forbidden for value testing.
+**Guideline:** Use `.toBe()` for exact equality when the full value is known and reasonable to assert. Use `.toContain()` for array membership or when asserting the full value would make tests brittle (e.g., long autogenerated messages).
 
 ### TypeScript: NEVER Use `any`
 

src/engine/engine.gh-pages-integration.spec.ts

@@ -29,6 +29,7 @@ jest.mock('gh-pages/lib/git', () => {
 
 describe('engine - gh-pages integration', () => {
   const logger = new logging.NullLogger();
+  const originalEnv = process.env;
 
   // Only spy on gh-pages methods
   let ghpagesCleanSpy: jest.SpyInstance;
@@ -58,8 +59,21 @@ describe('engine - gh-pages integration', () => {
       }
     );
 
-    // Reset environment
-    process.env = {};
+    // Create fresh copy of environment for each test
+    // This preserves PATH, HOME, etc. needed by git
+    process.env = { ...originalEnv };
+    // Clear only CI-specific vars we're testing
+    delete process.env.TRAVIS;
+    delete process.env.CIRCLECI;
+    delete process.env.GITHUB_ACTIONS;
+    delete process.env.GH_TOKEN;
+    delete process.env.PERSONAL_TOKEN;
+    delete process.env.GITHUB_TOKEN;
+  });
+
+  afterAll(() => {
+    // Restore original environment for other test files
+    process.env = originalEnv;
   });
 
   describe('gh-pages.clean() behavior', () => {

src/engine/engine.prepare-options-helpers.spec.ts

@@ -15,20 +15,44 @@ describe('prepareOptions helpers - intensive tests', () => {
   let testLogger: logging.Logger;
   let infoSpy: jest.SpyInstance;
   let warnSpy: jest.SpyInstance;
+  const originalEnv = process.env;
 
   beforeEach(() => {
     testLogger = new logging.Logger('test');
     infoSpy = jest.spyOn(testLogger, 'info');
     warnSpy = jest.spyOn(testLogger, 'warn');
-    // Reset environment for each test
-    process.env = {};
+    // Create fresh copy of environment for each test
+    // This preserves PATH, HOME, etc. needed by git
+    process.env = { ...originalEnv };
+    // Clear only CI-specific vars we're testing
+    delete process.env.TRAVIS;
+    delete process.env.TRAVIS_COMMIT;
+    delete process.env.TRAVIS_COMMIT_MESSAGE;
+    delete process.env.TRAVIS_REPO_SLUG;
+    delete process.env.TRAVIS_BUILD_ID;
+    delete process.env.CIRCLECI;
+    delete process.env.CIRCLE_PROJECT_USERNAME;
+    delete process.env.CIRCLE_PROJECT_REPONAME;
+    delete process.env.CIRCLE_SHA1;
+    delete process.env.CIRCLE_BUILD_URL;
+    delete process.env.GITHUB_ACTIONS;
+    delete process.env.GITHUB_REPOSITORY;
+    delete process.env.GITHUB_SHA;
+    delete process.env.GH_TOKEN;
+    delete process.env.PERSONAL_TOKEN;
+    delete process.env.GITHUB_TOKEN;
   });
 
   afterEach(() => {
     infoSpy.mockRestore();
     warnSpy.mockRestore();
   });
 
+  afterAll(() => {
+    // Restore original environment for other test files
+    process.env = originalEnv;
+  });
+
   describe('setupMonkeypatch', () => {
     let originalDebuglog: typeof import('util').debuglog;
 
@@ -280,7 +304,7 @@ describe('prepareOptions helpers - intensive tests', () => {
   describe('appendCIMetadata', () => {
     const baseMessage = 'Deploy to gh-pages';
 
-    it('should append Travis CI metadata when TRAVIS env is set', () => {
+    it('should append Travis CI metadata with exact format', () => {
       process.env.TRAVIS = 'true';
       process.env.TRAVIS_COMMIT_MESSAGE = 'Fix bug in component';
       process.env.TRAVIS_REPO_SLUG = 'user/repo';
@@ -296,12 +320,15 @@ describe('prepareOptions helpers - intensive tests', () => {
 
       helpers.appendCIMetadata(options);
 
-      expect(options.message).toContain(' -- Fix bug in component');
-      expect(options.message).toContain('Triggered by commit: https://github.com/user/repo/commit/abc123def456');
-      expect(options.message).toContain('Travis CI build: https://travis-ci.org/user/repo/builds/987654321');
+      const expectedMessage =
+        'Deploy to gh-pages -- Fix bug in component \n\n' +
+        'Triggered by commit: https://github.com/user/repo/commit/abc123def456\n' +
+        'Travis CI build: https://travis-ci.org/user/repo/builds/987654321';
+
+      expect(options.message).toBe(expectedMessage);
     });
 
-    it('should append CircleCI metadata when CIRCLECI env is set', () => {
+    it('should append CircleCI metadata with exact format', () => {
       process.env.CIRCLECI = 'true';
       process.env.CIRCLE_PROJECT_USERNAME = 'johndoe';
       process.env.CIRCLE_PROJECT_REPONAME = 'myproject';
@@ -317,11 +344,15 @@ describe('prepareOptions helpers - intensive tests', () => {
 
       helpers.appendCIMetadata(options);
 
-      expect(options.message).toContain('Triggered by commit: https://github.com/johndoe/myproject/commit/fedcba987654');
-      expect(options.message).toContain('CircleCI build: https://circleci.com/gh/johndoe/myproject/123');
+      const expectedMessage =
+        'Deploy to gh-pages\n\n' +
+        'Triggered by commit: https://github.com/johndoe/myproject/commit/fedcba987654\n' +
+        'CircleCI build: https://circleci.com/gh/johndoe/myproject/123';
+
+      expect(options.message).toBe(expectedMessage);
     });
 
-    it('should append GitHub Actions metadata when GITHUB_ACTIONS env is set', () => {
+    it('should append GitHub Actions metadata with exact format', () => {
       process.env.GITHUB_ACTIONS = 'true';
       process.env.GITHUB_REPOSITORY = 'angular-schule/angular-cli-ghpages';
       process.env.GITHUB_SHA = '1234567890abcdef';
@@ -335,7 +366,11 @@ describe('prepareOptions helpers - intensive tests', () => {
 
       helpers.appendCIMetadata(options);
 
-      expect(options.message).toContain('Triggered by commit: https://github.com/angular-schule/angular-cli-ghpages/commit/1234567890abcdef');
+      const expectedMessage =
+        'Deploy to gh-pages\n\n' +
+        'Triggered by commit: https://github.com/angular-schule/angular-cli-ghpages/commit/1234567890abcdef';
+
+      expect(options.message).toBe(expectedMessage);
     });
 
     it('should NOT modify message when no CI env is set', () => {
@@ -351,16 +386,23 @@ describe('prepareOptions helpers - intensive tests', () => {
       expect(options.message).toBe(baseMessage);
     });
 
-    it('should append metadata from multiple CI environments if present', () => {
+    it('should append metadata from multiple CI environments in correct order', () => {
+      // Set up multiple CI environments (Travis, CircleCI, GitHub Actions)
       process.env.TRAVIS = 'true';
       process.env.TRAVIS_COMMIT_MESSAGE = 'Update docs';
       process.env.TRAVIS_REPO_SLUG = 'org/repo';
       process.env.TRAVIS_COMMIT = 'abc123';
       process.env.TRAVIS_BUILD_ID = '111';
 
+      process.env.CIRCLECI = 'true';
+      process.env.CIRCLE_PROJECT_USERNAME = 'org';
+      process.env.CIRCLE_PROJECT_REPONAME = 'repo';
+      process.env.CIRCLE_SHA1 = 'def456';
+      process.env.CIRCLE_BUILD_URL = 'https://circleci.com/gh/org/repo/222';
+
       process.env.GITHUB_ACTIONS = 'true';
       process.env.GITHUB_REPOSITORY = 'org/repo';
-      process.env.GITHUB_SHA = 'def456';
+      process.env.GITHUB_SHA = 'ghi789';
 
       const options: helpers.PreparedOptions = {
         message: baseMessage,
@@ -371,8 +413,15 @@ describe('prepareOptions helpers - intensive tests', () => {
 
       helpers.appendCIMetadata(options);
 
-      expect(options.message).toContain('Travis CI build');
-      expect(options.message).toContain('Triggered by commit: https://github.com/org/repo/commit/def456');
+      // Verify Travis appears first, then CircleCI, then GitHub Actions
+      expect(options.message).toBeDefined();
+      const travisIndex = options.message!.indexOf('Travis CI build');
+      const circleIndex = options.message!.indexOf('CircleCI build');
+      const ghActionsIndex = options.message!.indexOf('Triggered by commit: https://github.com/org/repo/commit/ghi789');
+
+      expect(travisIndex).toBeGreaterThan(-1);
+      expect(circleIndex).toBeGreaterThan(travisIndex);
+      expect(ghActionsIndex).toBeGreaterThan(circleIndex);
     });
   });
 
@@ -541,30 +590,33 @@ describe('prepareOptions helpers - intensive tests', () => {
      * comment in engine.prepare-options-helpers.ts for fallback options.
      */
 
-    it('should successfully call gh-pages/lib/git Git class and return URL', async () => {
-      // This test verifies the internal API still exists and is callable
+    it('should return correct URL from git config and be consistent', async () => {
+      // This test verifies the internal API returns ACTUAL git config values
       const options = { remote: 'origin' };
 
-      // Should successfully return the git remote URL
-      const result = await helpers.getRemoteUrl(options);
+      // Get what git actually says
+      const { execSync } = require('child_process');
+      const expectedUrl = execSync('git config --get remote.origin.url', { encoding: 'utf8' }).trim();
 
-      // Verify it returns a string URL
-      expect(typeof result).toBe('string');
-      expect(result.length).toBeGreaterThan(0);
+      // Verify getRemoteUrl returns the same value
+      const result = await helpers.getRemoteUrl(options);
+      expect(result).toBe(expectedUrl);
 
-      // Verify it looks like a git URL (either SSH or HTTPS)
-      const isGitUrl = result.includes('github.com') || result.startsWith('git@') || result.startsWith('https://');
-      expect(isGitUrl).toBe(true);
+      // Verify consistency across multiple calls
+      const result2 = await helpers.getRemoteUrl(options);
+      expect(result2).toBe(expectedUrl);
+      expect(result).toBe(result2);
     });
 
-    it('should pass remote option to getRemoteUrl method', async () => {
+    it('should throw helpful error for non-existent remote', async () => {
       const options = {
         remote: 'nonexistent-remote-12345' // Remote that definitely doesn't exist
       };
 
-      // Should throw because this remote doesn't exist in our test repo
-      // This verifies the remote option is passed to getRemoteUrl method
-      await expect(helpers.getRemoteUrl(options)).rejects.toThrow();
+      // Should throw with specific error message about the remote
+      await expect(helpers.getRemoteUrl(options))
+        .rejects
+        .toThrow('Failed to get remote.');
     });
 
     it('should throw helpful error when not in a git repository', async () => {
@@ -575,41 +627,15 @@ describe('prepareOptions helpers - intensive tests', () => {
 
       try {
         process.chdir(tempDir);
-        const options = {};
+        const options = { remote: 'origin' };
 
         await expect(helpers.getRemoteUrl(options))
           .rejects
-          .toThrow(); // Will throw error about not being in a git repository
+          .toThrow('run in a git repository');
       } finally {
         process.chdir(originalCwd);
         await require('fs-extra').remove(tempDir);
       }
     });
-
-    it('should work when remote is provided from defaults', async () => {
-      // In real usage, options.remote is ALWAYS set because prepareOptions merges defaults
-      // Our defaults.ts has remote: 'origin', so options.remote is never undefined
-      const options = { remote: 'origin' };
-
-      // Should successfully return the remote URL
-      const result = await helpers.getRemoteUrl(options);
-      expect(typeof result).toBe('string');
-      expect(result.length).toBeGreaterThan(0);
-
-      // Calling again with same remote should return same URL
-      const result2 = await helpers.getRemoteUrl(options);
-      expect(result).toBe(result2);
-    });
-
-    it('should return consistent URL for same remote', async () => {
-      const options1 = { remote: 'origin' };
-      const options2 = { remote: 'origin' };
-
-      const result1 = await helpers.getRemoteUrl(options1);
-      const result2 = await helpers.getRemoteUrl(options2);
-
-      // Same remote should return same URL
-      expect(result1).toBe(result2);
-    });
   });
 });

src/engine/engine.spec.ts

@@ -5,9 +5,24 @@ import * as engine from './engine';
 describe('engine', () => {
   describe('prepareOptions', () => {
     const logger = new logging.NullLogger();
+    const originalEnv = process.env;
 
     beforeEach(() => {
-      process.env = {};
+      // Create fresh copy of environment for each test
+      // This preserves PATH, HOME, etc. needed by git
+      process.env = { ...originalEnv };
+      // Clear only CI-specific vars we're testing
+      delete process.env.TRAVIS;
+      delete process.env.CIRCLECI;
+      delete process.env.GITHUB_ACTIONS;
+      delete process.env.GH_TOKEN;
+      delete process.env.PERSONAL_TOKEN;
+      delete process.env.GITHUB_TOKEN;
+    });
+
+    afterAll(() => {
+      // Restore original environment for other test files
+      process.env = originalEnv;
     });
 
     it('should replace the string GH_TOKEN in the repo url (for backwards compatibility)', async () => {