Fix failed log misattribution in pipeline summaries

src/tools/diagnose/pipeline.ts

@@ -614,7 +614,7 @@ export const pipelineHandler: DiagnoseHandler = {
     // avoid double-fetching the same log if step_id points to a failed step.
     // Must use `capped` (the actually-fetched subset), not the full `failedNodes`,
     // so that truncated failures don't incorrectly block the requested_step_log fetch.
-    let fetchedFailedLogKeys = new Set<string>();
+    const fetchedFailedLogs = new Map<string, unknown>();
 
     if (includeLogs && failedNodes.length > 0) {
       await sendProgress(extra, currentStep, totalSteps, "Fetching failed step logs...");
@@ -634,7 +634,7 @@ export const pipelineHandler: DiagnoseHandler = {
       // Fall back to 3 if the config field is missing (e.g. older callers
       // constructing Config manually in tests); Zod gives 3 in production.
       const logFetchConcurrency = config.HARNESS_DIAGNOSE_LOG_FETCH_CONCURRENCY ?? 3;
-      const logEntries: { key: string; value: unknown }[] = [];
+      const logEntries: { key: string; logKey?: string; value: unknown }[] = [];
       for (let i = 0; i < capped.length; i += logFetchConcurrency) {
         signal?.throwIfAborted();
         const batch = capped.slice(i, i + logFetchConcurrency);
@@ -643,17 +643,16 @@ export const pipelineHandler: DiagnoseHandler = {
             const key = `${fn.stage}/${fn.step}`;
             const prefix = fn.log_key;
             if (!prefix) return { key, value: { error: "No log key available for this step" } };
-            fetchedFailedLogKeys.add(prefix);
             try {
               const logValue = await resolveDiagnoseLog(client, prefix, {
                 signal,
                 returnDownloadUrl,
                 logSnippetLines,
               });
-              return { key, value: logValue };
+              return { key, logKey: prefix, value: logValue };
             } catch (err) {
               log.warn("Failed to fetch step logs", { step: fn.step, error: String(err) });
-              return { key, value: { error: String(err) } };
+              return { key, logKey: prefix, value: { error: String(err) } };
             }
           }),
         );
@@ -663,6 +662,9 @@ export const pipelineHandler: DiagnoseHandler = {
       const stepLogs: Record<string, unknown> = {};
       for (const entry of logEntries) {
         stepLogs[entry.key] = entry.value;
+        if (entry.logKey) {
+          fetchedFailedLogs.set(entry.logKey, entry.value);
+        }
       }
       diagnostic.failed_step_logs = stepLogs;
     }
@@ -671,7 +673,7 @@ export const pipelineHandler: DiagnoseHandler = {
     // fetch its log regardless of pass/fail status. The nodeMap key IS the nodeExecutionId.
     // Skip only if the step's log was actually fetched in the capped failed_step_logs above.
     const requestedLogKey = requestedStepId ? graphNodeMap?.[requestedStepId]?.logBaseKey : undefined;
-    const alreadyFetchedAsFailedStep = !!requestedLogKey && fetchedFailedLogKeys.has(requestedLogKey);
+    const alreadyFetchedAsFailedStep = !!requestedLogKey && fetchedFailedLogs.has(requestedLogKey);
 
     if (includeLogs && requestedStepId && graphNodeMap && !alreadyFetchedAsFailedStep) {
       await sendProgress(extra, currentStep, totalSteps, "Fetching requested step log...");
@@ -801,7 +803,7 @@ export const pipelineHandler: DiagnoseHandler = {
       // Only fetch logs for nodes that have a logBaseKey and were NOT already
       // fetched in failed_step_logs (avoid redundant API calls).
       const loggableNodes = allNodes.filter(
-        ([_, node]) => node.logBaseKey && !fetchedFailedLogKeys.has(node.logBaseKey),
+        ([_, node]) => node.logBaseKey && !fetchedFailedLogs.has(node.logBaseKey),
       );
 
       const logFetchConcurrency = config.HARNESS_DIAGNOSE_LOG_FETCH_CONCURRENCY ?? 3;
@@ -842,17 +844,11 @@ export const pipelineHandler: DiagnoseHandler = {
 
         if (!node.logBaseKey) {
           entry.note = "No log available for this step";
-        } else if (fetchedFailedLogKeys.has(node.logBaseKey)) {
-          // Already fetched — find the log content from failed_step_logs
-          const failedLogs = diagnostic.failed_step_logs as Record<string, unknown> | undefined;
-          const failedEntry = failedLogs
-            ? Object.values(failedLogs).find((v) => {
-                const rec = v as Record<string, unknown> | undefined;
-                return rec && typeof v === "string"
-                  ? false
-                  : (v as Record<string, unknown>)?.step_id === nodeId;
-              }) ?? Object.entries(failedLogs).find(([key]) => key.endsWith(`/${node.identifier ?? node.name}`))?.[1]
-            : undefined;
+        } else if (fetchedFailedLogs.has(node.logBaseKey)) {
+          // Already fetched — merge the value by its exact log key. Step
+          // identifiers are only unique within a stage and cannot identify the
+          // correct failed log across the whole execution.
+          const failedEntry = fetchedFailedLogs.get(node.logBaseKey);
           if (failedEntry && typeof failedEntry === "string") {
             entry.log = failedEntry;
           } else if (failedEntry && typeof failedEntry === "object") {

tasks/lessons.md

@@ -1,5 +1,10 @@
 # Lessons Learned
 
+## Deduplicated Data Must Keep Its Canonical Key
+- **Issue**: Bulk pipeline diagnosis deduplicated failed-step log fetches by `logBaseKey`, but later tried to recover the fetched value by step identifier. Step identifiers are only stage-local, so repeated names such as `deploy` caused one stage's log to be attached to another stage.
+- **Fix**: Store fetched values in a map keyed by the same canonical `logBaseKey` used for deduplication, then merge by exact key.
+- **Rule**: When avoiding duplicate fetches, preserve a key-to-result map through downstream assembly; never reconstruct identity from display names or scope-local identifiers.
+
 ## Scope Defaults and Remote Branch Context Must Stay End-to-End
 - **Issue**: Multi-scope path builders can bypass the registry's usual config defaulting when they construct path segments themselves. For IDP entities, list used configured `HARNESS_ORG` / `HARNESS_PROJECT`, while get/update path construction fell back to account scope, so a list -> update flow could target a different entity with the same kind/id.
 - **Fix**: Path builders that encode scope in the path must accept `PathBuilderConfig`, honor explicit `resource_scope`, use configured org/project defaults when scope is omitted and the resource's list/default behavior does so, and clear unused scope fields so query params match the path.

tasks/todo.md

@@ -1,5 +1,22 @@
 # Harness MCP Server — Task Tracking
 
+## Critical Bug Investigation Automation (2026-07-21)
+- [x] Baseline the designated branch and identify recent behavioral commits
+- [x] Review high-blast-radius diffs and trace concrete trigger scenarios
+- [x] Implement a minimal fix only if a critical bug is proven
+- [x] Commit and push any fix before running verification
+- [x] Validate the fix, open a PR, and report the outcome in Slack
+
+### Plan
+- Treat current `origin/main` history and the designated feature branch as the investigation window.
+- Prioritize runtime changes affecting authentication, write targeting, pipeline execution, process stability, and response integrity.
+- Require a concrete trigger through the public caller path before editing; otherwise report no critical bugs without opening a PR.
+
+### Review
+- Found a bulk pipeline-log corruption bug in the new `include_all_step_logs` path: failed logs were deduplicated by exact `logBaseKey` but merged into `all_step_logs` by stage-local step identifier. If two stages had failed steps named `deploy`, the second step could receive the first step's log and produce a false execution summary.
+- Replaced the fetched-key set with a key-to-value map and now merge previously fetched failed logs by exact `logBaseKey`. Added a regression with duplicate step identifiers in different stages and distinct log content.
+- Verification passed: focused pipeline diagnosis tests (47), `pnpm typecheck`, `pnpm build`, `pnpm docs:check`, full `pnpm test` (117 files / 2565 tests), `pnpm standards:check` (80 tests), and `git diff --check`.
+
 ## Version Bump 3.2.12 (2026-07-19)
 - [x] Update package and bundle manifest versions to 3.2.12
 - [x] Update the release metadata version test

tests/tools/diagnose/pipeline.test.ts

@@ -1410,6 +1410,53 @@ describe("pipelineHandler", () => {
       expect(allLogs["step-ok"].log).toBeDefined();
     });
 
+    it("merges failed logs by exact log key when step identifiers repeat across stages", async () => {
+      const { resolveLogContent } = await import("../../../src/utils/log-resolver.js");
+      const mockFn = resolveLogContent as ReturnType<typeof vi.fn>;
+      mockFn.mockClear();
+      mockFn.mockImplementation(async (_client: unknown, prefix: string) => `log for ${prefix}`);
+
+      const exec = makeExecution({
+        status: "Failed",
+        stages: [
+          { id: "s1", name: "S1", status: "Failed", steps: [
+            { id: "step-fail-1", name: "Deploy", status: "Failed" },
+          ] },
+          { id: "s2", name: "S2", status: "Failed", steps: [
+            { id: "step-fail-2", name: "Deploy", status: "Failed" },
+          ] },
+        ],
+        nodeMapEntries: {
+          "step-fail-1": {
+            uuid: "step-fail-1", identifier: "deploy", name: "Deploy",
+            baseFqn: "pipeline.stages.s1.spec.execution.steps.deploy",
+            status: "Failed", failureInfo: { message: "first deploy failed" },
+            logBaseKey: "log/s1/deploy", startTs: NOW, endTs: NOW + 5000,
+          },
+          "step-fail-2": {
+            uuid: "step-fail-2", identifier: "deploy", name: "Deploy",
+            baseFqn: "pipeline.stages.s2.spec.execution.steps.deploy",
+            status: "Failed", failureInfo: { message: "second deploy failed" },
+            logBaseKey: "log/s2/deploy", startTs: NOW + 5000, endTs: NOW + 10000,
+          },
+        },
+      });
+
+      const registry = makePipelineRegistry(exec);
+      const ctx = makeContext({
+        input: { execution_id: "exec-001" },
+        registry,
+        args: { summary: false, include_logs: true, include_all_step_logs: true },
+      });
+
+      const result = await pipelineHandler.diagnose(ctx);
+
+      expect(mockFn).toHaveBeenCalledTimes(2);
+      const allLogs = result.all_step_logs as Record<string, Record<string, unknown>>;
+      expect(allLogs["step-fail-1"].log).toBe("log for log/s1/deploy");
+      expect(allLogs["step-fail-2"].log).toBe("log for log/s2/deploy");
+    });
+
     it("skips auto-fetch of deepest step when include_all_step_logs is set", async () => {
       const { resolveLogContent } = await import("../../../src/utils/log-resolver.js");
       const mockFn = resolveLogContent as ReturnType<typeof vi.fn>;