test: add regression guards for review fixes; harden restoreCache

docker-agent · docker-agent · commit 505413252022 · 2026-05-06T20:02:24.000Z
FIX A (integration test — security pipeline ordering):
  Replace vacuous conversational-only content with a real Anthropic API key
  (matching /sk-ant-[a-zA-Z0-9_-]{30,}/) in the verbose log before a clean
  docker-agent-output block. Test now asserts secrets-detected=true and
  security-blocked=true, which ONLY passes under the correct order
  (filter→sanitize→extract). Also asserts outputFile retains the leaked
  key (Step 9b is skipped on leak detection).

FIX B (auth.test.ts — Tier 3 fallthrough is distinguishable):
  Add 'falls through to author_association when org check throws (FIX 3)'.
  Uses OWNER association so old hard-deny code would return denied but
  new fallthrough code reaches Tier 4 and returns authorized=true,
  outcome='author-association'. This test fails if the FIX 3 catch block
  is reverted to return { authorized: false }.

FIX C (outputs.test.ts — blank-line inTool exit):
  Add 'drops blank line that terminates an inTool block (matches awk next)'.
  Checks that the line immediately before 'Clean output' is not blank,
  which fails if the continue is reverted to fall-through.

FIX D (exec.test.ts — SIGTERM kill path):
  Add 'kills process with SIGTERM when timeout fires'. Uses a 50ms action
  timeout with a child that would naturally exit in 5s. Updated makeMockChild
  so kill() emits close via setImmediate (simulates OS killing the process),
  allowing the timer path to complete within the 2s test timeout.
  Asserts child.kill was called with 'SIGTERM'.

FIX E (binary.ts — restoreCache non-fatal):
  Wrap both restoreCache calls (docker-agent and mcp-gateway) in try/catch
  with core.warning + fallthrough to download, matching the existing
  saveCache non-fatal pattern. A network error in restoreCache no longer
  fails the action.

Assisted-By: docker-agent
diff --git a/src/main/__tests__/auth.test.ts b/src/main/__tests__/auth.test.ts
@@ -233,6 +233,27 @@ describe('Tier 3: org membership', () => {
     expect(result.authorized).toBe(false);
     expect(result.outcome).toBe('denied');
   });
+
+  it('falls through to author_association when org check throws (FIX 3)', async () => {
+    // Old code: exception → hard-deny. New code: exception → warn + fall through to Tier 4.
+    // Using OWNER association so Tier 4 authorizes — this distinguishes the two behaviors.
+    await writePayload({
+      comment: { author_association: 'OWNER', user: { login: 'repo-owner' } },
+    });
+    mockGetAuthenticated.mockResolvedValue({ data: { login: 'bot' } });
+    mockCheckOrgMembership.mockRejectedValue(new Error('Network timeout'));
+
+    const result = await checkAuthorization({
+      ...BASE_OPTS,
+      skipAuth: false,
+      orgMembershipToken: 'org-token',
+      authOrg: 'my-org',
+      eventPayloadPath,
+    });
+    // New behavior: falls through to Tier 4 → OWNER is authorized
+    expect(result.authorized).toBe(true);
+    expect(result.outcome).toBe('author-association');
+  });
 });
 
 // ── Tier 4: author_association fallback ──────────────────────────────────────
diff --git a/src/main/__tests__/exec.test.ts b/src/main/__tests__/exec.test.ts
@@ -40,8 +40,13 @@ function makeMockChild(exitCode: number, delayMs = 0) {
     kill: ReturnType<typeof vi.fn>;
   };
   emitter.stdin = { write: vi.fn(), end: vi.fn() };
-  emitter.kill = vi.fn();
 
+  // When killed (SIGTERM/SIGKILL), emit close shortly after — simulates real process dying.
+  emitter.kill = vi.fn().mockImplementation(() => {
+    setImmediate(() => emitter.emit('close', null));
+  });
+
+  // Natural exit after delayMs (ignored if kill fires first)
   setTimeout(() => emitter.emit('close', exitCode), delayMs);
 
   return emitter;
@@ -251,6 +256,18 @@ describe('runAgent', () => {
     expect(mockSpawn).toHaveBeenCalledOnce();
   });
 
+  it('kills process with SIGTERM when timeout fires (FIX D)', async () => {
+    // Child exits naturally in 5 s; action timeout is 50 ms.
+    // The real timer path fires SIGTERM, then the child is killed.
+    const child = makeMockChild(0, 5000);
+    mockSpawn.mockReturnValue(child);
+
+    const result = await runAgent(baseOpts({ timeout: 0.05, maxRetries: 0 }));
+
+    expect(result.exitCode).toBe(TIMEOUT_EXIT_CODE);
+    expect(child.kill).toHaveBeenCalledWith('SIGTERM');
+  }, 2000);
+
   it('retries on non-zero exit code up to maxRetries times', async () => {
     // Each spawn call must have its own close event
     mockSpawn
diff --git a/src/main/__tests__/main.integration.test.ts b/src/main/__tests__/main.integration.test.ts
@@ -446,13 +446,22 @@ describe('agent exit code propagation', () => {
 
 // ── Wiring — sanitizeOutput runs before block extraction ─────────────────────
 
+// ── Wiring — sanitizeOutput runs before block extraction ─────────────────────
+
 describe('security pipeline ordering (FIX 1)', () => {
   it('sanitizeOutput scans full filtered output before block extraction narrows it', async () => {
+    // This test MUST fail if FIX 1 is reverted (sanitize-after-extract order).
+    // Strategy: verbose log contains a real Anthropic API key (matching
+    // /sk-ant-[a-zA-Z0-9_-]{30,}/) in conversational text BEFORE a clean
+    // docker-agent-output block.  Under the correct order (filter → sanitize →
+    // extract), sanitizeOutput sees the key → secrets-detected=true.  Under the
+    // wrong order (filter → extract → sanitize) the outputFile contains only the
+    // clean block and the key is never scanned.
+    const LEAKED_KEY =
+      'sk-ant-api03-AAAABBBBCCCCDDDDEEEEFFFFGGGGHHHHIIIIJJJJKKKKLLLLMMMMNNNNOOOOPPPPQQQQRRRR';
+
     setupInputs({ prompt: 'Please review this PR' });
 
-    // Capture the paths that run() will assign to output/verbose files.
-    // setOutput('verbose-log-file', ...) is called BEFORE runAgent(), so
-    // capturedVerboseLog is populated before mockSpawn is invoked.
     let capturedVerboseLog: string | undefined;
     let capturedOutputFile: string | undefined;
 
@@ -461,13 +470,21 @@ describe('security pipeline ordering (FIX 1)', () => {
       if (name === 'output-file') capturedOutputFile = value;
     });
 
-    // Write verbose log content SYNCHRONOUSLY inside the spawn mock so it is
-    // present when run() reads the file immediately after spawn completes.
+    // Write verbose log content SYNCHRONOUSLY so it is present when run() reads
+    // the file immediately after spawn completes.
     mockSpawn.mockImplementation(() => {
       if (capturedVerboseLog) {
         fsSync.appendFileSync(
           capturedVerboseLog,
-          '--- Agent: root ---\nConversational text.\n```docker-agent-output\n## Result\n\nClean output.\n```\n',
+          [
+            'Here is my analysis.',
+            `Oops I leaked: ${LEAKED_KEY}`,
+            '```docker-agent-output',
+            '## Result',
+            '',
+            'Clean output with no secrets.',
+            '```',
+          ].join('\n'),
           'utf-8',
         );
       }
@@ -476,19 +493,16 @@ describe('security pipeline ordering (FIX 1)', () => {
 
     await run();
 
-    // outputFile should contain the block-extracted clean content
+    // The key must have been detected BEFORE block extraction narrowed the file.
+    const outputCalls = Object.fromEntries(mockSetOutput.mock.calls.map(([n, v]) => [n, v]));
+    expect(outputCalls['secrets-detected']).toBe('true');
+    expect(outputCalls['security-blocked']).toBe('true');
+
+    // When a leak is detected, Step 9b is skipped — outputFile retains full
+    // filtered text so the incident path can see the leaked key.
     if (capturedOutputFile && fsSync.existsSync(capturedOutputFile)) {
-      const outputContent = fsSync.readFileSync(capturedOutputFile, 'utf-8');
-      // Block extraction should have run: output is the docker-agent-output block
-      expect(outputContent.trim()).toBe('## Result\n\nClean output.');
-    } else {
-      // If output file wasn\'t created, ensure run() completed without failure
-      expect(mockSetFailed).not.toHaveBeenCalled();
+      const content = fsSync.readFileSync(capturedOutputFile, 'utf-8');
+      expect(content).toContain(LEAKED_KEY);
     }
-
-    // Security outputs are correct (no leak in test content)
-    const outputCalls = Object.fromEntries(mockSetOutput.mock.calls.map(([n, v]) => [n, v]));
-    expect(outputCalls['secrets-detected']).toBe('false');
-    expect(outputCalls['security-blocked']).toBe('false');
   });
 });
diff --git a/src/main/__tests__/outputs.test.ts b/src/main/__tests__/outputs.test.ts
@@ -81,6 +81,18 @@ describe('filterAgentOutput', () => {
     expect(result).toContain('Clean output');
   });
 
+  it('drops blank line that terminates an inTool block (matches awk `next`)', () => {
+    // FIX 4: the blank line that closes an inTool block must be dropped (continue),
+    // not re-emitted. Reverting to fall-through would emit an extra blank line.
+    const input = ['--- Tool: bash ---', 'some tool output', '', 'Clean output'].join('\n');
+    const result = filterAgentOutput(input);
+    const lines = result.split('\n');
+    const cleanIdx = lines.findIndex((l) => l === 'Clean output');
+    expect(cleanIdx).toBeGreaterThan(-1);
+    // The line immediately before 'Clean output' must not be blank
+    expect(lines[cleanIdx - 1]).not.toBe('');
+  });
+
   it('strips Calling <fn>( … ) blocks', () => {
     const input =
       'Calling read_multiple_files(\n  paths: ["pr.diff"]\n)\n\n## Summary\n\nThis PR adds a greeting.\n';
diff --git a/src/main/binary.ts b/src/main/binary.ts
@@ -93,7 +93,14 @@ async function ensureDockerAgent(version: string, githubToken?: string): Promise
   // ── 2. Remote @actions/cache hit (cross-run persistence) ───────────────
   const tmpBinDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'docker-agent-'));
   const cacheKey = `docker-agent-${toolName}-${version}-${platform}-${arch}`;
-  const restoredKey = await actionsCache.restoreCache([tmpBinDir], cacheKey);
+  let restoredKey: string | undefined;
+  try {
+    restoredKey = await actionsCache.restoreCache([tmpBinDir], cacheKey);
+  } catch (err: unknown) {
+    core.warning(
+      `Remote cache restore failed (${(err as Error).message}); falling back to download`,
+    );
+  }
 
   if (restoredKey) {
     core.info(`Restored docker-agent ${version} from remote cache (key: ${restoredKey})`);
@@ -161,7 +168,14 @@ async function ensureMcpGateway(version: string, githubToken?: string): Promise<
   // ── 2. Remote @actions/cache hit ───────────────────────────────────────
   const tmpPluginDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'docker-mcp-'));
   const cacheKey = `docker-agent-${toolName}-${version}-${platform}-${arch}`;
-  const restoredKey = await actionsCache.restoreCache([tmpPluginDir], cacheKey);
+  let restoredKey: string | undefined;
+  try {
+    restoredKey = await actionsCache.restoreCache([tmpPluginDir], cacheKey);
+  } catch (err: unknown) {
+    core.warning(
+      `Remote cache restore failed (${(err as Error).message}); falling back to download`,
+    );
+  }
 
   if (restoredKey) {
     core.info(`Restored mcp-gateway ${version} from remote cache (key: ${restoredKey})`);
