From 6e93397e7a8af80371a335008be9bec319a209c3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 21 Jul 2026 11:14:16 +0000 Subject: [PATCH 1/2] fix: preserve failed log identity in pipeline summaries Co-authored-by: Rohan Gupta --- src/tools/diagnose/pipeline.ts | 32 ++++++++---------- tasks/lessons.md | 5 +++ tasks/todo.md | 12 +++++++ tests/tools/diagnose/pipeline.test.ts | 47 +++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 18 deletions(-) diff --git a/src/tools/diagnose/pipeline.ts b/src/tools/diagnose/pipeline.ts index 933030a42..98bc8c2ab 100644 --- a/src/tools/diagnose/pipeline.ts +++ b/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(); + const fetchedFailedLogs = new Map(); 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 = {}; 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 | 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; + } 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") { diff --git a/tasks/lessons.md b/tasks/lessons.md index 52365db88..14b66416f 100644 --- a/tasks/lessons.md +++ b/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. diff --git a/tasks/todo.md b/tasks/todo.md index ca802b154..e7e67c848 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -1,5 +1,17 @@ # 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 +- [ ] Commit and push any fix before running verification +- [ ] 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. + ## 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 cfe45431d..bf34b59ae 100644 --- a/tests/tools/diagnose/pipeline.test.ts +++ b/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; + 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>; + 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; From 4d065e5cb67b5245a941827ebe29845f962400f4 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 21 Jul 2026 11:15:13 +0000 Subject: [PATCH 2/2] docs: record critical bug investigation results Co-authored-by: Rohan Gupta --- tasks/todo.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tasks/todo.md b/tasks/todo.md index e7e67c848..8776ce1a1 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -4,14 +4,19 @@ - [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 -- [ ] 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 - 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