From fe02e5896e49a6d8813c8fc3d637a7de64957932 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 23 Jul 2026 11:07:53 +0000 Subject: [PATCH 1/3] fix: keep duplicate failed step logs correctly attributed Co-authored-by: Rohan Gupta --- src/tools/diagnose/pipeline.ts | 43 ++++++++++++++---------- tasks/todo.md | 12 +++++++ tests/tools/diagnose/pipeline.test.ts | 48 +++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 17 deletions(-) diff --git a/src/tools/diagnose/pipeline.ts b/src/tools/diagnose/pipeline.ts index 933030a4..7bef6785 100644 --- a/src/tools/diagnose/pipeline.ts +++ b/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): 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): 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(); + const fetchedFailedLogKeys = new Set(); + const fetchedFailedLogs = new Map(); if (includeLogs && failedNodes.length > 0) { await sendProgress(extra, currentStep, totalSteps, "Fetching failed step logs..."); @@ -634,7 +637,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; nodeId: string; logKey?: string; value: unknown }[] = []; for (let i = 0; i < capped.length; i += logFetchConcurrency) { signal?.throwIfAborted(); const batch = capped.slice(i, i + logFetchConcurrency); @@ -642,7 +645,13 @@ export const pipelineHandler: DiagnoseHandler = { 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, { @@ -650,10 +659,10 @@ export const pipelineHandler: DiagnoseHandler = { 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 = {}; 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 | undefined; - const failedEntry = failedLogs - ? Object.values(failedLogs).find((v) => { - const rec = v as Record | undefined; - return rec && typeof v === "string" - ? false - : (v as Record)?.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") { diff --git a/tasks/todo.md b/tasks/todo.md index ca802b15..3adcf4af 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -1,5 +1,17 @@ # 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 +- [ ] Commit and push any fix before running verification +- [ ] Validate the fix, 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. + ## 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 diff --git a/tests/tools/diagnose/pipeline.test.ts b/tests/tools/diagnose/pipeline.test.ts index cfe45431..de306ce4 100644 --- a/tests/tools/diagnose/pipeline.test.ts +++ b/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; + 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)).toEqual([ + "log for log/matrix-1", + "log for log/matrix-2", + ]); + + const logs = result.all_step_logs as Record>; + 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).mockResolvedValue("resolved log line 1\nresolved log line 2"); From ecc17763229444e846e9b9630e1550903d5f87ce Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 23 Jul 2026 11:09:13 +0000 Subject: [PATCH 2/3] docs: record diagnosis log investigation Co-authored-by: Rohan Gupta --- tasks/lessons.md | 5 +++++ tasks/todo.md | 11 +++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/tasks/lessons.md b/tasks/lessons.md index 52365db8..9da036f7 100644 --- a/tasks/lessons.md +++ b/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. diff --git a/tasks/todo.md b/tasks/todo.md index 3adcf4af..db893d35 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -4,14 +4,21 @@ - [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 -- [ ] Commit and push any fix before running verification -- [ ] Validate the fix, open a PR, and report the outcome in Slack +- [x] Commit and push any fix before running verification +- [x] Validate the fix +- [ ] 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 From 88673204a902e184019c6feb9c5104e8fe570b2f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 23 Jul 2026 11:10:29 +0000 Subject: [PATCH 3/3] docs: complete critical bug investigation Co-authored-by: Rohan Gupta --- tasks/todo.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tasks/todo.md b/tasks/todo.md index db893d35..64b6f786 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -6,7 +6,7 @@ - [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 +- [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.