feat: Smarter retries and better error feedback

- Modified `fetchWithRetry` to stop retrying on 4xx client errors (excluding 429 Too Many Requests). This prevents unnecessary waiting for errors that will not resolve (e.g., 404 Not Found, 400 Bad Request).
- Updated `saveError` to print the specific error message to `stderr` in addition to logging the file path. This gives the user immediate context on why the request failed.
- Added comprehensive tests in `tests/commands/unipept/retry_logic.test.ts` to verify the retry behavior across various scenarios (5xx, 404, 400, 429, network error).

Author: google-labs-jules[bot]

lib/commands/unipept/unipept_subcommand.ts

@@ -208,9 +208,10 @@ export abstract class UnipeptSubcommand {
    */
   async saveError(message: string) {
     const errorPath = this.errorFilePath();
-    mkdir(path.dirname(errorPath), { recursive: true });
+    await mkdir(path.dirname(errorPath), { recursive: true });
     await appendFile(errorPath, `${message}\n`);
-    console.error(`API request failed! log can be found in ${errorPath}`);
+    console.error(`API request failed: ${message}`);
+    console.error(`Log can be found in ${errorPath}`);
   }
 
   /**
@@ -228,7 +229,17 @@ export abstract class UnipeptSubcommand {
         }
       })
       .catch(async error => {
-        if (retries > 0) {
+        let shouldRetry = retries > 0;
+
+        // check if we should stop retrying based on error message (from the reject above)
+        if (typeof error === 'string') {
+          const status = parseInt(error.split(' ')[0]);
+          if (status >= 400 && status < 500 && status !== 429) {
+            shouldRetry = false;
+          }
+        }
+
+        if (shouldRetry) {
           // retry with delay
           // console.error("retrying");
           const delay = 5000 * Math.random();

tests/commands/unipept/retry_logic.test.ts

@@ -0,0 +1,103 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { UnipeptSubcommand } from '../../../lib/commands/unipept/unipept_subcommand';
+
+// Concrete implementation for testing abstract class
+class TestSubcommand extends UnipeptSubcommand {
+    constructor() {
+        super('test');
+    }
+    defaultBatchSize(): number { return 10; }
+}
+
+describe('UnipeptSubcommand Retry Logic', () => {
+    let command: TestSubcommand;
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    let fetchSpy: any;
+
+    beforeEach(() => {
+        command = new TestSubcommand();
+        // @ts-ignore
+        fetchSpy = vi.spyOn(global, 'fetch');
+    });
+
+    afterEach(() => {
+        vi.restoreAllMocks();
+    });
+
+    it('should retry on 500 error', async () => {
+        // Mock fetch to fail with 500 once, then succeed
+        fetchSpy
+            .mockResolvedValueOnce({
+                ok: false,
+                status: 500,
+                statusText: 'Internal Server Error'
+            } as Response)
+            .mockResolvedValueOnce({
+                ok: true,
+                status: 200,
+                statusText: 'OK'
+            } as Response);
+
+        const result = await command.fetchWithRetry('http://example.com', {}, 3);
+        expect(result.ok).toBe(true);
+        expect(fetchSpy).toHaveBeenCalledTimes(2);
+    });
+
+    it('should retry on network error', async () => {
+        fetchSpy
+            .mockRejectedValueOnce(new Error('Network error'))
+            .mockResolvedValueOnce({
+                ok: true,
+                status: 200,
+                statusText: 'OK'
+            } as Response);
+
+        const result = await command.fetchWithRetry('http://example.com', {}, 3);
+        expect(result.ok).toBe(true);
+        expect(fetchSpy).toHaveBeenCalledTimes(2);
+    });
+
+    it('should NOT retry on 404 error', async () => {
+        fetchSpy.mockResolvedValue({
+            ok: false,
+            status: 404,
+            statusText: 'Not Found'
+        } as Response);
+
+        await expect(command.fetchWithRetry('http://example.com', {}, 3))
+            .rejects.toMatch(/Failed to fetch data from the Unipept API: 404 Not Found/);
+
+        expect(fetchSpy).toHaveBeenCalledTimes(1);
+    });
+
+    it('should NOT retry on 400 error', async () => {
+        fetchSpy.mockResolvedValue({
+            ok: false,
+            status: 400,
+            statusText: 'Bad Request'
+        } as Response);
+
+        await expect(command.fetchWithRetry('http://example.com', {}, 3))
+            .rejects.toMatch(/Failed to fetch data from the Unipept API: 400 Bad Request/);
+
+        expect(fetchSpy).toHaveBeenCalledTimes(1);
+    });
+
+    it('should retry on 429 error', async () => {
+        fetchSpy
+            .mockResolvedValueOnce({
+                ok: false,
+                status: 429,
+                statusText: 'Too Many Requests'
+            } as Response)
+            .mockResolvedValueOnce({
+                ok: true,
+                status: 200,
+                statusText: 'OK'
+            } as Response);
+
+        const result = await command.fetchWithRetry('http://example.com', {}, 3);
+        expect(result.ok).toBe(true);
+        expect(fetchSpy).toHaveBeenCalledTimes(2);
+    });
+});