Fix duplicate failed-step log attribution

src/tools/diagnose/pipeline.ts

@@ -95,6 +95,7 @@ interface StageSummary {
 }
 
 interface FailedNodeDetail {
+  node_id: string;
   stage: string;
   step: string;
   failure_message: string;
@@ -231,7 +232,7 @@ function findFailedNodes(nodeMap: Record<string, ExecGraphNode>): FailedNodeDeta
   const stepNodes: FailedNodeDetail[] = [];
   const stageNodes: FailedNodeDetail[] = [];
 
-  for (const node of Object.values(nodeMap)) {
+  for (const [nodeId, node] of Object.entries(nodeMap)) {
     if (node.status !== "Failed" && node.status !== "Errored" && node.status !== "Aborted") continue;
 
     const msg = node.failureInfo?.message;
@@ -248,6 +249,7 @@ function findFailedNodes(nodeMap: Record<string, ExecGraphNode>): FailedNodeDeta
     const delegate = node.delegateInfoList?.[0]?.name;
 
     const detail: FailedNodeDetail = {
+      node_id: nodeId,
       stage: stageId,
       step: node.identifier ?? node.name ?? "unknown",
       failure_message: msg,
@@ -614,7 +616,8 @@ 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 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,26 +637,32 @@ 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; nodeId: string; logKey?: string; value: unknown }[] = [];
       for (let i = 0; i < capped.length; i += logFetchConcurrency) {
         signal?.throwIfAborted();
         const batch = capped.slice(i, i + logFetchConcurrency);
         const batchResults = await Promise.all(
           batch.map(async (fn) => {
             const key = `${fn.stage}/${fn.step}`;
             const prefix = fn.log_key;
-            if (!prefix) return { key, value: { error: "No log key available for this step" } };
+            if (!prefix) {
+              return {
+                key,
+                nodeId: fn.node_id,
+                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, nodeId: fn.node_id, 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, nodeId: fn.node_id, logKey: prefix, value: { error: String(err) } };
             }
           }),
         );
@@ -662,7 +671,13 @@ 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);
+        }
+        const outputKey = Object.hasOwn(stepLogs, entry.key)
+          ? `${entry.key} [${entry.nodeId}]`
+          : entry.key;
+        stepLogs[outputKey] = entry.value;
       }
       diagnostic.failed_step_logs = stepLogs;
     }
@@ -843,16 +858,10 @@ 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;
+          // Already fetched — use the log key rather than the display name so
+          // matrix/loop steps with duplicate identifiers cannot receive each
+          // other's logs.
+          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
 
+## Execution Log Identity Must Survive Response Reshaping
+- **Issue**: Failed pipeline logs were keyed by the display pair `stage/step`, then reused in node-ID-keyed all-step output through an identifier suffix match. Matrix and loop nodes can share both display identifiers, causing one log to overwrite another and be attributed to the wrong execution node.
+- **Fix**: Carry the execution node ID through failed-node discovery, preserve colliding display keys, and keep an internal map keyed by the unique `logBaseKey` for merges and deduplication.
+- **Rule**: When fetching or reshaping execution logs, use stable execution identity (`nodeId` or `logBaseKey`) internally. Human-readable stage/step names are presentation only and must never drive deduplication or attribution.
+
 ## 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,24 @@
 # Harness MCP Server — Task Tracking
 
+## Critical Bug Investigation Automation (2026-07-23)
+- [x] Baseline branch state 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
+- [x] Open a PR and report the outcome in Slack
+
+### Plan
+- Compare current main history with the previous investigation window and prioritize newly landed behavioral changes.
+- Trace candidate issues through public tool handlers, registry dispatch, and downstream request/response shaping.
+- Require a concrete severe trigger and a narrow high-confidence regression test before changing runtime code.
+
+### Review
+- Found a failed-log attribution bug in the new `include_all_step_logs` diagnosis path. Failed logs were first keyed by the display pair `stage/identifier`, which collides for matrix and loop nodes, then merged into node-ID-keyed results via an identifier suffix search.
+- A pipeline with two failed matrix nodes sharing identifier `run` could overwrite one `failed_step_logs` entry and attach the surviving log to both nodes. The pipeline summarizer treats `all_step_logs` as its single source of truth, so it could report the wrong failure evidence for a deployment or test shard.
+- Fixed failed-node tracking to retain node IDs, preserve colliding failed-log outputs, and merge already-fetched content by each node's unique `logBaseKey`.
+- Verification passed: focused pipeline diagnosis tests (47), `pnpm typecheck`, `pnpm build`, full `pnpm test` (120 files / 2615 tests), `pnpm standards:check` (80 tests), `pnpm docs:check`, 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

@@ -1323,6 +1323,54 @@ describe("pipelineHandler", () => {
       expect(Object.keys(logs)).toHaveLength(2);
     });
 
+    it("preserves and correctly merges failed logs for duplicate step identifiers", 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: "matrix-1", name: "Run 1", status: "Failed" },
+          { id: "matrix-2", name: "Run 2", status: "Failed" },
+        ]}],
+        nodeMapEntries: {
+          "matrix-1": {
+            uuid: "matrix-1", identifier: "run", name: "Run",
+            baseFqn: "pipeline.stages.s1.spec.execution.steps.matrix-1",
+            status: "Failed", failureInfo: { message: "first failed" },
+            logBaseKey: "log/matrix-1", startTs: NOW, endTs: NOW + 5000,
+          },
+          "matrix-2": {
+            uuid: "matrix-2", identifier: "run", name: "Run",
+            baseFqn: "pipeline.stages.s1.spec.execution.steps.matrix-2",
+            status: "Failed", failureInfo: { message: "second failed" },
+            logBaseKey: "log/matrix-2", 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);
+      expect(Object.values(result.failed_step_logs as Record<string, unknown>)).toEqual([
+        "log for log/matrix-1",
+        "log for log/matrix-2",
+      ]);
+
+      const logs = result.all_step_logs as Record<string, Record<string, unknown>>;
+      expect(logs["matrix-1"].log).toBe("log for log/matrix-1");
+      expect(logs["matrix-2"].log).toBe("log for log/matrix-2");
+    });
+
     it("includes steps without logBaseKey with a note", async () => {
       const { resolveLogContent } = await import("../../../src/utils/log-resolver.js");
       (resolveLogContent as ReturnType<typeof vi.fn>).mockResolvedValue("resolved log line 1\nresolved log line 2");