From 3ad707a41391964f7ba6839c9e02d00edde1dbba Mon Sep 17 00:00:00 2001 From: Jaydeep Dave Date: Fri, 14 Aug 2026 01:15:14 +0530 Subject: [PATCH 1/4] feat(tfa): code-enforced PR validation gate, author-merge fix, report error-gating Fix A: New lib/pr-validation.mjs validates every coordinator's related_prs against evidence file ground truth at the loop.mjs out() choke point. Invalid PRs are downgraded to verdict: "ruled-out ()", never dropped. Fix B: foldGithub in evidence-file.mjs now does field-level merge on PR dedup collisions instead of whole-object replacement, preserving author (and all other fields) across shards. Fix C: SKILL.md Step 6 now unambiguously gates completion glimpse and cleanupBuildArtifacts on triggerRcaReport success, with explicit try/catch and failure-path instructions. Fix D: Prompt contracts updated (ai-tfa-coordinator.md, github-evidence.md, suspect-packet.md, SKILL.md) to describe the code-enforced validation gate as primary enforcement, with LLM falsification as defense-in-depth. --- agents/ai-tfa-coordinator.md | 12 +- lib/evidence-file.mjs | 25 +++- lib/loop.mjs | 9 +- lib/pr-validation.mjs | 121 ++++++++++++++++ skills/rca-build/SKILL.md | 28 +++- .../rca-build/references/github-evidence.md | 8 ++ skills/rca-build/templates/suspect-packet.md | 6 + tests/evidence-file-author-merge.test.mjs | 69 +++++++++ tests/loop-pr-validation.test.mjs | 101 ++++++++++++++ tests/pr-validation.test.mjs | 131 ++++++++++++++++++ 10 files changed, 504 insertions(+), 6 deletions(-) create mode 100644 lib/pr-validation.mjs create mode 100644 tests/evidence-file-author-merge.test.mjs create mode 100644 tests/loop-pr-validation.test.mjs create mode 100644 tests/pr-validation.test.mjs diff --git a/agents/ai-tfa-coordinator.md b/agents/ai-tfa-coordinator.md index f0284af..fb2a3af 100644 --- a/agents/ai-tfa-coordinator.md +++ b/agents/ai-tfa-coordinator.md @@ -387,7 +387,17 @@ failing path, blame, deploy timing) via **GitHub MCP → `gh` → degrade**, and each candidate suspect **try to disprove it** (path overlap? shipped before the failure window? behind an OFF flag?). Feed both supporting *and* disconfirming evidence back as a structured suspect packet; only `verdict: supported` suspects -belong in `related_prs`. Reuse the pre-computed build-level evidence — do not +belong in `related_prs`. + +**Code-enforced validation gate (primary enforcement).** After the loop resolves, +`lib/loop.mjs`'s `out()` runs `validateAndDeduplicatePRs` (`lib/pr-validation.mjs`) +on every coordinator's `related_prs`, cross-checking each claimed PR against the +evidence file's `prsInWindow` ground truth. A PR absent from the evidence or +merged after `started_at` is automatically downgraded to +`verdict: "ruled-out ()"`. The LLM falsification protocol above is +**defense-in-depth** — it catches issues the code gate cannot (path-overlap, +behind-an-OFF-flag), but is not the sole enforcer of merge-window or existence +checks. Reuse the pre-computed build-level evidence — do not re-fetch per test (the `evidenceFile`'s `github` section, if present and not `gap`-marked for this repo; otherwise the live github connector). A culprit hunt often needs to go deeper than the file's summary — a full diff, a diff --git a/lib/evidence-file.mjs b/lib/evidence-file.mjs index e7e42d5..a931235 100644 --- a/lib/evidence-file.mjs +++ b/lib/evidence-file.mjs @@ -202,6 +202,19 @@ function prKey(pr, index) { return u ? `url:${u}` : t ? `title:${t}` : `anon:${index}`; } +// Field-level merge for PR dedup collisions: keep the first non-null value +// per field across shards. Fixes author-loss when a later shard lacks a field +// an earlier one had (Fix B). +function mergePrEntry(existing, incoming) { + const merged = { ...incoming }; + for (const key of Object.keys(existing)) { + if (merged[key] == null && existing[key] != null) { + merged[key] = existing[key]; + } + } + return merged; +} + function foldGithub(target, repo, entry) { const cur = target[repo] ?? { deployState: null, prsInWindow: [], gap: null }; const next = { @@ -215,8 +228,18 @@ function foldGithub(target, repo, entry) { }; if (Array.isArray(entry.prsInWindow)) { const byPr = new Map((next.prsInWindow ?? []).map((p, i) => [prKey(p, i), p])); - entry.prsInWindow.forEach((pr, i) => byPr.set(prKey(pr, `in-${i}`), pr)); + entry.prsInWindow.forEach((pr, i) => { + const key = prKey(pr, `in-${i}`); + const existing = byPr.get(key); + byPr.set(key, existing ? mergePrEntry(existing, pr) : pr); + }); next.prsInWindow = [...byPr.values()]; + // FR-6: warn when a PR completes folding with no author from any shard. + for (const pr of next.prsInWindow) { + if (pr.author == null) { + console.warn(`[evidence-file] PR ${pr.pr ?? "?"} has author: null after fold — no shard provided an author`); + } + } } // A contributor supplying real content clears the pre-fetch's gap. if (entry.gap === null || entry.gap === undefined) { diff --git a/lib/loop.mjs b/lib/loop.mjs index e0ef66b..d9abbcd 100644 --- a/lib/loop.mjs +++ b/lib/loop.mjs @@ -28,6 +28,7 @@ // PENDING instead would stack a second turn on one still in flight. import { routeAsks } from "./routing.mjs"; +import { validateAndDeduplicatePRs } from "./pr-validation.mjs"; // Drain budget for one soft-PENDING. Bounded so a wedged turn can never hang the // batch — on exhaustion the loop still ends `PENDING` (resumable via the CSV's @@ -145,6 +146,7 @@ export async function runRcaLoop({ config = {}, manifest = {}, gather = async () => "", + evidenceDoc, turnCap = config?.turnCap ?? 6, drain = config?.softPendingDrain, sleep = defaultSleep, @@ -172,13 +174,18 @@ export async function runRcaLoop({ const out = (status, turn, note) => { const glimpse = turn?.glimpse ?? {}; + const rawPrs = glimpse.related_prs ?? []; + const validatedPrs = + status === "RESOLVED" && evidenceDoc + ? validateAndDeduplicatePRs(rawPrs, evidenceDoc) + : rawPrs; return { testRunId: String(testRunId), status, confidence: turn?.confidence ?? "unknown", root_cause: status === "RESOLVED" ? (glimpse.root_cause ?? "") : (note ?? ""), failure_type: glimpse.failure_type ?? "", - related_prs: glimpse.related_prs ?? [], + related_prs: validatedPrs, view_rca: turn?.viewRca ?? "", threadId: threadId ?? null, turnId: turnId ?? null, diff --git a/lib/pr-validation.mjs b/lib/pr-validation.mjs new file mode 100644 index 0000000..916c35f --- /dev/null +++ b/lib/pr-validation.mjs @@ -0,0 +1,121 @@ +// Code-enforced PR validation gate (Fix A). +// +// Every coordinator's `related_prs` exits through `loop.mjs` `out()`, which +// calls `validateAndDeduplicatePRs` here to cross-check each claimed PR +// against the evidence file's ground truth. A PR that fails validation is +// downgraded to `verdict: "ruled-out ()"` — never silently dropped. + +/** + * Extract owner/repo from a PR entry's link or url field. + * Handles GitHub PR URLs like "https://github.com/owner/repo/pull/123". + */ +function extractOwnerRepo(entry) { + const raw = entry?.link || entry?.url || ""; + const m = String(raw).match(/github\.com\/([^/]+\/[^/]+)\/pull\//); + return m ? m[1] : null; +} + +/** + * Build the lookup key for a PR: "owner/repo" string. + * Prefers explicit owner+repo fields; falls back to parsing link/url. + */ +function resolveRepo(entry) { + if (entry?.owner && entry?.repo) return `${entry.owner}/${entry.repo}`; + return extractOwnerRepo(entry); +} + +/** + * Validate a single suspect PR entry against the evidence file. + * + * @param {object} entry - PR entry with owner, repo, number, merged_at, link/url + * @param {object} evidenceDoc - folded evidence file from readEvidenceFile + * @returns {{valid: boolean, reason?: "not-in-evidence" | "shipped-after"}} + */ +export function validateSuspectPR(entry, evidenceDoc) { + const repoKey = resolveRepo(entry); + if (!repoKey) return { valid: false, reason: "not-in-evidence" }; + + const repoEvidence = evidenceDoc?.github?.[repoKey]; + const prsInWindow = repoEvidence?.prsInWindow ?? []; + + const num = entry?.number ?? entry?.pr; + if (num == null) return { valid: false, reason: "not-in-evidence" }; + + const normalise = (n) => String(n).replace(/^#/, ""); + const match = prsInWindow.find( + (p) => normalise(p.pr) === normalise(num), + ); + if (!match) return { valid: false, reason: "not-in-evidence" }; + + // Merge-window check: merged_at must not be after the build's started_at. + const startedAt = evidenceDoc?.suspectWindow?.startedAt; + if (startedAt) { + const mergedMs = Date.parse(entry.merged_at ?? match.mergedAt ?? ""); + const startedMs = Date.parse(startedAt); + if (!Number.isNaN(mergedMs) && !Number.isNaN(startedMs) && mergedMs > startedMs) { + return { valid: false, reason: "shipped-after" }; + } + } else if (startedAt === undefined || startedAt === null) { + // Cannot falsify on window — log but allow. + console.warn(`[pr-validation] suspectWindow.startedAt absent — skipping merge-window check`); + } + + return { valid: true }; +} + +/** + * Deduplicate and validate an array of suspect PR entries against the evidence file. + * + * - Deduplicates by (owner, repo, number). + * - Enriches each entry with ground-truth fields from the evidence file. + * - Valid entries keep their existing verdict. + * - Invalid entries are downgraded to `verdict: "ruled-out ()"`. + * - All entries (valid + ruled-out) are returned — nothing is silently dropped. + * + * @param {Array} relatedPrs - the TFA agent's claimed suspect PRs + * @param {object} evidenceDoc - folded evidence file from readEvidenceFile + * @returns {Array} deduplicated, validated PR entries + */ +export function validateAndDeduplicatePRs(relatedPrs, evidenceDoc) { + if (!Array.isArray(relatedPrs) || relatedPrs.length === 0) return []; + + // Deduplicate by (repo, number). + const seen = new Map(); + for (const entry of relatedPrs) { + const repoKey = resolveRepo(entry); + const num = entry?.number ?? entry?.pr; + const dedup = repoKey && num != null + ? `${repoKey}#${String(num).replace(/^#/, "")}` + : `anon-${seen.size}`; + if (!seen.has(dedup)) seen.set(dedup, entry); + } + + const results = []; + for (const entry of seen.values()) { + const repoKey = resolveRepo(entry); + const num = entry?.number ?? entry?.pr; + + // Enrich from evidence ground truth when possible. + const normalise = (n) => String(n).replace(/^#/, ""); + const repoEvidence = repoKey ? evidenceDoc?.github?.[repoKey] : null; + const groundTruth = (repoEvidence?.prsInWindow ?? []).find( + (p) => num != null && normalise(p.pr) === normalise(num), + ); + + // Build the output entry: ground-truth fields override LLM transcription. + const enriched = groundTruth + ? { ...entry, ...Object.fromEntries(Object.entries(groundTruth).filter(([, v]) => v != null)) } + : { ...entry }; + + const { valid, reason } = validateSuspectPR(entry, evidenceDoc); + if (!valid) { + enriched.verdict = `ruled-out (${reason})`; + console.warn(`[pr-validation] PR ${repoKey ?? "?"}#${num ?? "?"} downgraded: ${reason}`); + } + // Valid entries keep their existing verdict (typically "supported"). + + results.push(enriched); + } + + return results; +} diff --git a/skills/rca-build/SKILL.md b/skills/rca-build/SKILL.md index c65d414..14581d0 100644 --- a/skills/rca-build/SKILL.md +++ b/skills/rca-build/SKILL.md @@ -83,6 +83,13 @@ deployShas(pathOrDoc) → {pins:{repo:sha}, source} recomputeCoverage(path, {r readEvidenceFile(path) folds base+shards · readBaseFile(path) is base ONLY ``` +**PR ground-truth validation — `lib/pr-validation.mjs`** +``` +validateSuspectPR(entry, evidenceDoc) → {valid} | {valid:false, reason: "not-in-evidence"|"shipped-after"} +validateAndDeduplicatePRs(relatedPrs, evidenceDoc) → deduped, validated entries — invalid ones get + verdict: "ruled-out ()", never dropped. Called from lib/loop.mjs's out() choke point. +``` + **Local repo reads — `lib/repo-source.mjs`** ``` discoverWorkspaceRoot({repos, from, explicit, maxTries=3}) → {root, matched, tried, reason} @@ -1054,7 +1061,12 @@ re-running the same live search. This is already baked into `agents/ai-tfa-coordinator.md`'s Operating Principle 0 for any dispatch of that agent type — no need to repeat the mechanics in the prompt, just don't omit `evidenceFilePath` (above), since write-back has nothing to write to -without it. +without it. Note: `evidenceFilePath` is also used for **post-loop PR +validation** — `lib/loop.mjs`'s `out()` cross-checks every coordinator's +`related_prs` against the evidence file's `prsInWindow` ground truth +(`lib/pr-validation.mjs`). A dispatch that omits the evidence file path +disables this code-enforced validation gate, falling back to unvalidated +LLM output only. **Pre-seed the MCP cache with the queries you just ran.** Step 4's log sweeps are MCP calls, and a coordinator will often want the same ones. Deposit each @@ -1112,8 +1124,18 @@ link — that is all. When every row is terminal: `renderGlimpse`): `RCA analysis complete — build ` + a status count line (` tests · resolved ·

pending · failed`). **Nothing per-test.** 2. Call **`triggerRcaReport(buildUuid=)`** (add `force=true` only to - re-run over an existing completed report). -3. **Only once that call succeeds**, call + re-run over an existing completed report). **Wrap this call in try/catch** + and inspect the result: + - **Success** (no exception AND the response does not indicate failure): + proceed to step 3. + - **Failure** (exception thrown OR the response indicates failure — e.g. an + `error` field, a non-success status, or a falsy/missing result): print the + error message clearly, do **NOT** call `cleanupBuildArtifacts`, do **NOT** + print the completion glimpse or link line, and **DO** print: + `"triggerRcaReport failed — build artifacts preserved for retry."` Then + stop Step 6; the build's CSV, evidence file, and tool cache remain intact + so a subsequent `/rca-build` invocation can resume. +3. **Only once step 2 succeeds**, call `cleanupBuildArtifacts(buildId, config.paths.stateDir)` (`lib/build-cleanup.mjs`) to delete THIS build's own CSV, evidence file + `.contrib/` shards, tool cache, and turn1 registry. Never call this before diff --git a/skills/rca-build/references/github-evidence.md b/skills/rca-build/references/github-evidence.md index 2960a9c..1211c5e 100644 --- a/skills/rca-build/references/github-evidence.md +++ b/skills/rca-build/references/github-evidence.md @@ -134,6 +134,14 @@ Feed **both supporting and disconfirming** evidence back to TFA. A suspect that survives 1–3 is a real candidate; one that fails any is reported as ruled-out (with the reason), **not** dropped silently. +**Code-level validation runs after this protocol.** Even if the LLM marks a +suspect as `supported`, `lib/pr-validation.mjs`'s code gate cross-checks every +entry in `related_prs` against the evidence file's `prsInWindow` (existence) and +`suspectWindow.startedAt` (merge window). A PR that fails either check is +automatically downgraded to `ruled-out`. This protocol remains valuable as +defense-in-depth — it catches path-overlap and flag-gating issues that the code +gate cannot — but is not the sole enforcer of existence or merge-window checks. + ## The suspect packet (structured, not free text) Each surviving/ruled-out suspect is one structured block so `related_prs` diff --git a/skills/rca-build/templates/suspect-packet.md b/skills/rca-build/templates/suspect-packet.md index 676fcf4..49d26b8 100644 --- a/skills/rca-build/templates/suspect-packet.md +++ b/skills/rca-build/templates/suspect-packet.md @@ -15,6 +15,12 @@ SUSPECT: link: ``` +**Note:** `verdict: supported` entries are subject to a code-level validation +gate (`lib/pr-validation.mjs`) that cross-checks each PR against the evidence +file's `prsInWindow` and merge window. A PR absent from evidence or merged after +`started_at` will be automatically downgraded to `ruled-out` regardless of the +LLM's verdict. + If the hunt ends empty after a real search (never fabricate): ``` diff --git a/tests/evidence-file-author-merge.test.mjs b/tests/evidence-file-author-merge.test.mjs new file mode 100644 index 0000000..e8d7192 --- /dev/null +++ b/tests/evidence-file-author-merge.test.mjs @@ -0,0 +1,69 @@ +import { test, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + readEvidenceFile, + writeEvidenceFile, + contributeGithubEvidence, + initEvidenceFile, +} from "../lib/evidence-file.mjs"; + +let dir; +let file; + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "rca-ev-author-")); + file = join(dir, "evidence.json"); +}); +afterEach(() => rmSync(dir, { recursive: true, force: true })); + +test("foldGithub: author preserved when later shard lacks it (FR-5)", () => { + // Base has author. + initEvidenceFile(file, "b1", 1000); + const base = readEvidenceFile(file); + base.github = { "org/repo": { prsInWindow: [{ pr: 42, title: "fix", author: "alice" }] } }; + writeEvidenceFile(file, base); + + // Shard overwrites same PR without author. + contributeGithubEvidence(file, "writer1", "org/repo", { + prsInWindow: [{ pr: 42, title: "fix v2" }], + }, 2000); + + const folded = readEvidenceFile(file); + const pr = folded.github["org/repo"].prsInWindow.find((p) => String(p.pr) === "42"); + assert.equal(pr.author, "alice", "author from base should survive shard without author"); + assert.equal(pr.title, "fix v2", "later shard's non-null title wins"); +}); + +test("foldGithub: author from shard preserved when base lacks it (FR-5)", () => { + initEvidenceFile(file, "b1", 1000); + const base = readEvidenceFile(file); + base.github = { "org/repo": { prsInWindow: [{ pr: 42, title: "fix" }] } }; + writeEvidenceFile(file, base); + + contributeGithubEvidence(file, "writer1", "org/repo", { + prsInWindow: [{ pr: 42, author: "bob" }], + }, 2000); + + const folded = readEvidenceFile(file); + const pr = folded.github["org/repo"].prsInWindow.find((p) => String(p.pr) === "42"); + assert.equal(pr.author, "bob"); + assert.equal(pr.title, "fix", "base title preserved when shard lacks it"); +}); + +test("foldGithub: author null when absent from all shards (FR-6)", () => { + initEvidenceFile(file, "b1", 1000); + const base = readEvidenceFile(file); + base.github = { "org/repo": { prsInWindow: [{ pr: 42, title: "fix" }] } }; + writeEvidenceFile(file, base); + + contributeGithubEvidence(file, "writer1", "org/repo", { + prsInWindow: [{ pr: 42, title: "fix updated" }], + }, 2000); + + const folded = readEvidenceFile(file); + const pr = folded.github["org/repo"].prsInWindow.find((p) => String(p.pr) === "42"); + assert.equal(pr.author, undefined); // neither shard had author +}); diff --git a/tests/loop-pr-validation.test.mjs b/tests/loop-pr-validation.test.mjs new file mode 100644 index 0000000..b8119ea --- /dev/null +++ b/tests/loop-pr-validation.test.mjs @@ -0,0 +1,101 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { runRcaLoop } from "../lib/loop.mjs"; + +const CONFIG = { + turnCap: 6, + evidenceRouting: { test_logs: { owner: "tfa", skip: true } }, +}; + +test("loop out(): PR not in evidence is ruled-out when evidenceDoc provided", async () => { + const submit = async () => ({ + status: "RESOLVED", + threadId: "t1", + confidence: "high", + glimpse: { + root_cause: "code change", + failure_type: "product_regression", + related_prs: [ + { owner: "org", repo: "app", number: 42, verdict: "supported" }, + { owner: "org", repo: "app", number: 999, verdict: "supported" }, + ], + }, + viewRca: "https://example.com", + }); + + const evidenceDoc = { + github: { "org/app": { prsInWindow: [{ pr: 42, author: "alice" }] } }, + suspectWindow: { startedAt: "2026-08-10T00:00:00Z" }, + }; + + const result = await runRcaLoop({ + testRunId: "1", + submit, + config: CONFIG, + evidenceDoc, + }); + + assert.equal(result.status, "RESOLVED"); + assert.equal(result.related_prs.length, 2, "both entries should be present (ruled-out, not dropped)"); + const valid = result.related_prs.find((p) => p.number === 42); + const invalid = result.related_prs.find((p) => p.number === 999); + assert.ok(valid); + assert.ok(invalid); + assert.ok(!String(valid.verdict ?? "").startsWith("ruled-out")); + assert.equal(invalid.verdict, "ruled-out (not-in-evidence)"); +}); + +test("loop out(): without evidenceDoc, PRs pass through unvalidated", async () => { + const submit = async () => ({ + status: "RESOLVED", + threadId: "t1", + confidence: "high", + glimpse: { + root_cause: "code change", + failure_type: "product_regression", + related_prs: [{ owner: "org", repo: "app", number: 999, verdict: "supported" }], + }, + viewRca: "https://example.com", + }); + + const result = await runRcaLoop({ + testRunId: "2", + submit, + config: CONFIG, + }); + + assert.equal(result.related_prs.length, 1); + assert.equal(result.related_prs[0].verdict, "supported"); +}); + +test("loop out(): deduplicates PRs with same repo+number", async () => { + const submit = async () => ({ + status: "RESOLVED", + threadId: "t1", + confidence: "high", + glimpse: { + root_cause: "code change", + failure_type: "product_regression", + related_prs: [ + { owner: "org", repo: "app", number: 42, title: "title A", verdict: "supported" }, + { owner: "org", repo: "app", number: 42, title: "title B", verdict: "supported" }, + ], + }, + viewRca: "https://example.com", + }); + + const evidenceDoc = { + github: { "org/app": { prsInWindow: [{ pr: 42, title: "real title" }] } }, + suspectWindow: { startedAt: "2026-08-10T00:00:00Z" }, + }; + + const result = await runRcaLoop({ + testRunId: "3", + submit, + config: CONFIG, + evidenceDoc, + }); + + assert.equal(result.related_prs.length, 1); + assert.equal(result.related_prs[0].title, "real title"); +}); diff --git a/tests/pr-validation.test.mjs b/tests/pr-validation.test.mjs new file mode 100644 index 0000000..3f30bab --- /dev/null +++ b/tests/pr-validation.test.mjs @@ -0,0 +1,131 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { validateSuspectPR, validateAndDeduplicatePRs } from "../lib/pr-validation.mjs"; + +// --- validateSuspectPR --- + +test("validateSuspectPR: valid PR in evidence with merged_at before started_at", () => { + const entry = { owner: "org", repo: "app", number: 42, merged_at: "2026-08-01T10:00:00Z" }; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42, title: "fix", mergedAt: "2026-08-01T10:00:00Z" }] } }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: true }); +}); + +test("validateSuspectPR: PR not in evidence returns not-in-evidence", () => { + const entry = { owner: "org", repo: "app", number: 999 }; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42 }] } }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: false, reason: "not-in-evidence" }); +}); + +test("validateSuspectPR: PR merged after started_at returns shipped-after", () => { + const entry = { owner: "org", repo: "app", number: 42, merged_at: "2026-08-03T00:00:00Z" }; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42 }] } }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: false, reason: "shipped-after" }); +}); + +test("validateSuspectPR: owner/repo extracted from link when fields absent", () => { + const entry = { link: "https://github.com/org/app/pull/42", number: 42 }; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42 }] } }, + suspectWindow: { startedAt: "2026-08-10T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: true }); +}); + +test("validateSuspectPR: missing startedAt skips window check (valid)", () => { + const entry = { owner: "org", repo: "app", number: 42 }; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42 }] } }, + suspectWindow: {}, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: true }); +}); + +test("validateSuspectPR: no repo key at all returns not-in-evidence", () => { + const entry = { number: 42 }; + const doc = { github: {}, suspectWindow: {} }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: false, reason: "not-in-evidence" }); +}); + +test("validateSuspectPR: repo in evidence but empty prsInWindow returns not-in-evidence", () => { + const entry = { owner: "org", repo: "app", number: 42 }; + const doc = { + github: { "org/app": { prsInWindow: [] } }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: false, reason: "not-in-evidence" }); +}); + +test("validateSuspectPR: PR number with # prefix matches", () => { + const entry = { owner: "org", repo: "app", number: "#42" }; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: "42" }] } }, + suspectWindow: { startedAt: "2026-08-10T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: true }); +}); + +test("validateSuspectPR: uses mergedAt from evidence when entry lacks merged_at", () => { + const entry = { owner: "org", repo: "app", number: 42 }; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42, mergedAt: "2026-08-03T00:00:00Z" }] } }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: false, reason: "shipped-after" }); +}); + +// --- validateAndDeduplicatePRs --- + +test("validateAndDeduplicatePRs: deduplicates by repo+number, enriches from evidence", () => { + const prs = [ + { owner: "org", repo: "app", number: 42, title: "wrong title", verdict: "supported" }, + { owner: "org", repo: "app", number: 42, title: "also wrong" }, + ]; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42, title: "correct title", author: "alice" }] } }, + suspectWindow: { startedAt: "2026-08-10T00:00:00Z" }, + }; + const result = validateAndDeduplicatePRs(prs, doc); + assert.equal(result.length, 1); + assert.equal(result[0].title, "correct title"); + assert.equal(result[0].author, "alice"); +}); + +test("validateAndDeduplicatePRs: invalid entry is ruled-out, not dropped", () => { + const prs = [ + { owner: "org", repo: "app", number: 999, verdict: "supported" }, + { owner: "org", repo: "app", number: 42, verdict: "supported" }, + ]; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42 }] } }, + suspectWindow: { startedAt: "2026-08-10T00:00:00Z" }, + }; + const result = validateAndDeduplicatePRs(prs, doc); + assert.equal(result.length, 2); + assert.equal(result[0].verdict, "ruled-out (not-in-evidence)"); + assert.equal(result[1].number, 42); +}); + +test("validateAndDeduplicatePRs: empty input returns empty", () => { + assert.deepStrictEqual(validateAndDeduplicatePRs([], {}), []); + assert.deepStrictEqual(validateAndDeduplicatePRs(null, {}), []); +}); + +test("validateAndDeduplicatePRs: shipped-after verdict", () => { + const prs = [{ owner: "org", repo: "app", number: 42, merged_at: "2026-08-05T00:00:00Z", verdict: "supported" }]; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42 }] } }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + const result = validateAndDeduplicatePRs(prs, doc); + assert.equal(result.length, 1); + assert.equal(result[0].verdict, "ruled-out (shipped-after)"); +}); From 98a8c0a39151aa261746cc48420a9c61d5c71024 Mon Sep 17 00:00:00 2001 From: Jaydeep Dave Date: Fri, 14 Aug 2026 02:08:23 +0530 Subject: [PATCH 2/4] fix(tfa): reject PRs merged into the wrong branch, not just wrong timing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The validation gate checked existence and merge timing but not which branch a candidate PR actually merged into — a PR merged to main/master (or any branch other than the one this build actually ran on) could still pass if it happened to be in-window and pre-existing in evidence. - lib/evidence-file.mjs: track workingBranch per repo (the build's real branch, resolved once at Step 4 from fetchBuildInsights — never assumed main/master), sticky across shard folds like deployState. - lib/pr-validation.mjs: reject a candidate whose ground-truth baseRefName doesn't match workingBranch (reason: wrong-base-branch). Also hoists the duplicated normalise() helper and excludes `verdict` from the ground-truth field spread (both flagged in PR #3 review). - SKILL.md / github-evidence.md: instruct capturing baseRefName on every gathered PR and setting workingBranch at the pre-fetch, since a live coordinator gather later in the loop may omit --base and default to the repo's default branch. 5 new tests (218/218 passing). --- lib/evidence-file.mjs | 21 +++++--- lib/pr-validation.mjs | 24 +++++++-- skills/rca-build/SKILL.md | 20 +++++-- .../rca-build/references/github-evidence.md | 6 ++- tests/evidence-file-author-merge.test.mjs | 14 +++++ tests/pr-validation.test.mjs | 53 +++++++++++++++++++ 6 files changed, 121 insertions(+), 17 deletions(-) diff --git a/lib/evidence-file.mjs b/lib/evidence-file.mjs index a931235..2fdb70c 100644 --- a/lib/evidence-file.mjs +++ b/lib/evidence-file.mjs @@ -216,9 +216,13 @@ function mergePrEntry(existing, incoming) { } function foldGithub(target, repo, entry) { - const cur = target[repo] ?? { deployState: null, prsInWindow: [], gap: null }; + const cur = target[repo] ?? { deployState: null, prsInWindow: [], gap: null, workingBranch: null }; const next = { deployState: pickLeaf(cur.deployState, entry.deployState), + // Sticky, same as deployState: the build's actual working branch (from + // fetchBuildInsights), resolved once at Step 4. Lets pr-validation.mjs + // reject a PR merged into the wrong branch instead of assuming main/master. + workingBranch: pickLeaf(cur.workingBranch, entry.workingBranch), prsInWindow: cur.prsInWindow ?? [], // Sticky: once ANY writer has genuinely run the PR search, the entry stays // trustworthy — a later contributor that didn't search must not silently @@ -349,11 +353,13 @@ export function setBaseline(filePath, baseline, suspectWindow, nowMs) { } /** Read-modify-write merge into `doc.github[repo]`. `entry` shape: - * `{ deployState: {block, gap}, prsInWindow: [{pr, files, block, verdict}], - * gap }` — `gap` (top-level, on the repo entry) is what `recomputeCoverage` - * checks; a repo present with a non-null `gap` is NOT counted as covered. - * Only ever touches this one repo's key — every other repo/workload already - * in the file is untouched. */ + * `{ deployState: {block, gap}, workingBranch, prsInWindow: [{pr, files, + * block, verdict, baseRefName, mergedAt}], gap }` — `workingBranch` is the + * build's actual branch (from fetchBuildInsights, resolved once at Step 4); + * `gap` (top-level, on the repo entry) is what `recomputeCoverage` checks; a + * repo present with a non-null `gap` is NOT counted as covered. Only ever + * touches this one repo's key — every other repo/workload already in the + * file is untouched. */ export function setGithubEvidence(filePath, repo, entry, nowMs) { const doc = loadOrInit(filePath, nowMs); doc.github[repo] = entry; @@ -412,8 +418,9 @@ function writeShard(path, doc) { * orchestrator's base pre-fetch. */ export function contributeGithubEvidence(basePath, writerId, repo, patch, nowMs) { const { path, doc } = loadOwnShard(basePath, writerId, nowMs); - const entry = doc.github[repo] ?? { deployState: null, prsInWindow: [], gap: null }; + const entry = doc.github[repo] ?? { deployState: null, prsInWindow: [], gap: null, workingBranch: null }; if (patch.deployState !== undefined) entry.deployState = patch.deployState; + if (patch.workingBranch !== undefined) entry.workingBranch = patch.workingBranch; if (Array.isArray(patch.prsInWindow)) { const byPr = new Map((entry.prsInWindow ?? []).map((p, i) => [prKey(p, i), p])); patch.prsInWindow.forEach((pr, i) => byPr.set(prKey(pr, `in-${i}`), pr)); diff --git a/lib/pr-validation.mjs b/lib/pr-validation.mjs index 916c35f..93a0e15 100644 --- a/lib/pr-validation.mjs +++ b/lib/pr-validation.mjs @@ -24,12 +24,17 @@ function resolveRepo(entry) { return extractOwnerRepo(entry); } +/** Strip a leading "#" so "#123" and 123 compare equal. */ +function normalise(n) { + return String(n).replace(/^#/, ""); +} + /** * Validate a single suspect PR entry against the evidence file. * * @param {object} entry - PR entry with owner, repo, number, merged_at, link/url * @param {object} evidenceDoc - folded evidence file from readEvidenceFile - * @returns {{valid: boolean, reason?: "not-in-evidence" | "shipped-after"}} + * @returns {{valid: boolean, reason?: "not-in-evidence" | "shipped-after" | "wrong-base-branch"}} */ export function validateSuspectPR(entry, evidenceDoc) { const repoKey = resolveRepo(entry); @@ -41,12 +46,20 @@ export function validateSuspectPR(entry, evidenceDoc) { const num = entry?.number ?? entry?.pr; if (num == null) return { valid: false, reason: "not-in-evidence" }; - const normalise = (n) => String(n).replace(/^#/, ""); const match = prsInWindow.find( (p) => normalise(p.pr) === normalise(num), ); if (!match) return { valid: false, reason: "not-in-evidence" }; + // Branch check: the PR's ground-truth baseRefName must match the build's + // ACTUAL working branch (resolved once at Step 4 from fetchBuildInsights) — + // never assumed to be main/master. A PR merged into the wrong branch could + // not have shipped to this build regardless of timing. + const workingBranch = repoEvidence?.workingBranch; + if (workingBranch && match.baseRefName && match.baseRefName !== workingBranch) { + return { valid: false, reason: "wrong-base-branch" }; + } + // Merge-window check: merged_at must not be after the build's started_at. const startedAt = evidenceDoc?.suspectWindow?.startedAt; if (startedAt) { @@ -55,7 +68,7 @@ export function validateSuspectPR(entry, evidenceDoc) { if (!Number.isNaN(mergedMs) && !Number.isNaN(startedMs) && mergedMs > startedMs) { return { valid: false, reason: "shipped-after" }; } - } else if (startedAt === undefined || startedAt === null) { + } else { // Cannot falsify on window — log but allow. console.warn(`[pr-validation] suspectWindow.startedAt absent — skipping merge-window check`); } @@ -96,15 +109,16 @@ export function validateAndDeduplicatePRs(relatedPrs, evidenceDoc) { const num = entry?.number ?? entry?.pr; // Enrich from evidence ground truth when possible. - const normalise = (n) => String(n).replace(/^#/, ""); const repoEvidence = repoKey ? evidenceDoc?.github?.[repoKey] : null; const groundTruth = (repoEvidence?.prsInWindow ?? []).find( (p) => num != null && normalise(p.pr) === normalise(num), ); // Build the output entry: ground-truth fields override LLM transcription. + // Exclude `verdict` explicitly — a ground-truth PR entry has no business + // setting this coordinator-owned field, even if one ever appeared there. const enriched = groundTruth - ? { ...entry, ...Object.fromEntries(Object.entries(groundTruth).filter(([, v]) => v != null)) } + ? { ...entry, ...Object.fromEntries(Object.entries(groundTruth).filter(([k, v]) => v != null && k !== "verdict")) } : { ...entry }; const { valid, reason } = validateSuspectPR(entry, evidenceDoc); diff --git a/skills/rca-build/SKILL.md b/skills/rca-build/SKILL.md index 14581d0..276568a 100644 --- a/skills/rca-build/SKILL.md +++ b/skills/rca-build/SKILL.md @@ -85,7 +85,7 @@ readEvidenceFile(path) folds base+shards · readBaseFile(path) is base ONLY **PR ground-truth validation — `lib/pr-validation.mjs`** ``` -validateSuspectPR(entry, evidenceDoc) → {valid} | {valid:false, reason: "not-in-evidence"|"shipped-after"} +validateSuspectPR(entry, evidenceDoc) → {valid} | {valid:false, reason: "not-in-evidence"|"shipped-after"|"wrong-base-branch"} validateAndDeduplicatePRs(relatedPrs, evidenceDoc) → deduped, validated entries — invalid ones get verdict: "ruled-out ()", never dropped. Called from lib/loop.mjs's out() choke point. ``` @@ -592,7 +592,12 @@ leaves the other free to reintroduce the bug. recipes **once**, using `lib/evidence-cache.mjs`'s `compute(repo, range, evidenceType, fn)` to dedupe if two steps need the same `(repo, range)`. Digest the result into the `evidence-block.md` shape, then persist via - `setGithubEvidence(path, repo, {deployState, prsInWindow, gap}, nowMs)`. + `setGithubEvidence(path, repo, {deployState, workingBranch, prsInWindow, gap}, nowMs)`. + **Always set `workingBranch` to the exact `` resolved in Gate Part + B (`fetchBuildInsights(buildId).branch`, never assumed main/master)** — it + is what `lib/pr-validation.mjs` checks a candidate PR's `baseRefName` + against, so a PR that shipped to a different branch than this build + actually ran on is rejected instead of trusted on timing alone. A repo the connector can't reach records `{gap: ""}` — never blocks the rest of the pre-fetch. @@ -618,9 +623,18 @@ evidenceType, fn)` to dedupe if two steps need the same `(repo, range)`. ```bash gh pr list -R / --state merged --base \ - --search 'merged:..' --json number,title,mergedAt,url,files --limit 100 + --search 'merged:..' --json number,title,mergedAt,url,files,baseRefName --limit 100 ``` + `--base ` already scopes this call to the build's real working + branch — but store `baseRefName` from the response on each PR anyway + (`match.baseRefName` in `lib/pr-validation.mjs`), not just trust the query + filter. A live coordinator gather run later in the loop (widening a hunt + outside this pre-fetch) may omit `--base` and default to the repo's + default branch — recording `baseRefName` per PR is what lets the + validation gate catch that PR before it reaches a user, rather than + relying on every gather call remembering the flag. + `--json files` returns every PR's changed paths in the SAME call, so one request per repo replaces one `gh pr view --json files` per PR across every coordinator. Across real runs, per-PR file-list fetches have been a diff --git a/skills/rca-build/references/github-evidence.md b/skills/rca-build/references/github-evidence.md index 1211c5e..372cefb 100644 --- a/skills/rca-build/references/github-evidence.md +++ b/skills/rca-build/references/github-evidence.md @@ -103,7 +103,7 @@ manifest resolved to), since the failure mode is identical. | Repo exists / default branch | `gh api repos/OWNER/REPO` | `gh api repos/OWNER/REPO --jq '.default_branch'` | | Branch exists on the shipping branch | `gh api repos/OWNER/REPO/branches/BRANCH` | `gh api repos/OWNER/REPO/branches/BRANCH --jq '.name'` | | Commit history / PR-window search | `gh api "repos/OWNER/REPO/commits?sha=BRANCH&per_page=100"` | add `--jq '[.[] | {sha: .sha[0:8], date: .commit.committer.date, msg: (.commit.message | split("\n")[0])}]'` | -| PR metadata | `gh pr view N --repo OWNER/REPO` (full payload) | `gh pr view N --repo OWNER/REPO --json state,mergedAt,baseRefName,headRefOid,files,author` — `--json` is itself a field allowlist; list only the fields this ask uses | +| PR metadata | `gh pr view N --repo OWNER/REPO` (full payload) | `gh pr view N --repo OWNER/REPO --json state,mergedAt,baseRefName,headRefOid,files,author` — `--json` is itself a field allowlist; list only the fields this ask uses. **Always keep `baseRefName`** — the code-level validation gate rejects a PR whose base branch doesn't match the build's actual working branch, so dropping this field silently disables that check | | Pod / workload listing | `kubectl get pods -n NS -o wide` | `kubectl get pods -n NS -o custom-columns='NAME:.metadata.name,STATUS:.status.phase'` | | Deploy / image state | `kubectl get deploy -n NS -o yaml` | `kubectl get deploy -n NS -o custom-columns='NAME:.metadata.name,IMAGE:.spec.template.spec.containers[0].image'` | | Log sweep | a raw `--tail` dump | `kubectl logs POD --since= --tail=2000 \| grep -E '\|ERROR\|Exception'` — filter by the correlation token, never a raw tail | @@ -124,7 +124,9 @@ For **each** candidate suspect PR, try to **break** the hypothesis: 1. **Path overlap.** Do the PR's changed hunks actually touch the failing code path (the function/line in the stack)? No overlap → **ruled out**. 2. **Deployment-state guard.** Was the PR's code actually **live** in the run's - env at `started_at`? If it shipped *after* the failure window, or sits behind + env at `started_at`? If it shipped *after* the failure window, merged into a + **different branch** than the one this build actually ran on (never assume + main/master — use the resolved `` from Gate Part B), or sits behind an **OFF** flag, it could not have caused this failure → **ruled out**. 3. **Direction.** Does the change plausibly produce *this* error (e.g. a validator tightened to reject the input the test sends)? If the change is unrelated to diff --git a/tests/evidence-file-author-merge.test.mjs b/tests/evidence-file-author-merge.test.mjs index e8d7192..d69bc38 100644 --- a/tests/evidence-file-author-merge.test.mjs +++ b/tests/evidence-file-author-merge.test.mjs @@ -67,3 +67,17 @@ test("foldGithub: author null when absent from all shards (FR-6)", () => { const pr = folded.github["org/repo"].prsInWindow.find((p) => String(p.pr) === "42"); assert.equal(pr.author, undefined); // neither shard had author }); + +test("foldGithub: workingBranch set at base survives a shard that omits it", () => { + initEvidenceFile(file, "b1", 1000); + const base = readEvidenceFile(file); + base.github = { "org/repo": { workingBranch: "regression_run", prsInWindow: [] } }; + writeEvidenceFile(file, base); + + contributeGithubEvidence(file, "writer1", "org/repo", { + prsInWindow: [{ pr: 42, baseRefName: "regression_run" }], + }, 2000); + + const folded = readEvidenceFile(file); + assert.equal(folded.github["org/repo"].workingBranch, "regression_run"); +}); diff --git a/tests/pr-validation.test.mjs b/tests/pr-validation.test.mjs index 3f30bab..b5eff3e 100644 --- a/tests/pr-validation.test.mjs +++ b/tests/pr-validation.test.mjs @@ -129,3 +129,56 @@ test("validateAndDeduplicatePRs: shipped-after verdict", () => { assert.equal(result.length, 1); assert.equal(result[0].verdict, "ruled-out (shipped-after)"); }); + +// --- branch check --- + +test("validateSuspectPR: PR merged into the wrong branch returns wrong-base-branch", () => { + const entry = { owner: "org", repo: "app", number: 42, merged_at: "2026-08-01T00:00:00Z" }; + const doc = { + github: { + "org/app": { + workingBranch: "regression_run", + prsInWindow: [{ pr: 42, baseRefName: "main", mergedAt: "2026-08-01T00:00:00Z" }], + }, + }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: false, reason: "wrong-base-branch" }); +}); + +test("validateSuspectPR: PR merged into the correct working branch is valid", () => { + const entry = { owner: "org", repo: "app", number: 42, merged_at: "2026-08-01T00:00:00Z" }; + const doc = { + github: { + "org/app": { + workingBranch: "regression_run", + prsInWindow: [{ pr: 42, baseRefName: "regression_run", mergedAt: "2026-08-01T00:00:00Z" }], + }, + }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: true }); +}); + +test("validateSuspectPR: missing workingBranch or baseRefName skips the branch check (fail-open)", () => { + const entry = { owner: "org", repo: "app", number: 42, merged_at: "2026-08-01T00:00:00Z" }; + const doc = { + github: { "org/app": { prsInWindow: [{ pr: 42, mergedAt: "2026-08-01T00:00:00Z" }] } }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + assert.deepStrictEqual(validateSuspectPR(entry, doc), { valid: true }); +}); + +test("validateAndDeduplicatePRs: never overwrites verdict from ground truth", () => { + const prs = [{ owner: "org", repo: "app", number: 42, verdict: "supported" }]; + const doc = { + github: { + "org/app": { + prsInWindow: [{ pr: 42, verdict: "some-ground-truth-value", mergedAt: "2026-08-01T00:00:00Z" }], + }, + }, + suspectWindow: { startedAt: "2026-08-02T00:00:00Z" }, + }; + const result = validateAndDeduplicatePRs(prs, doc); + assert.equal(result[0].verdict, "supported"); +}); From 57b4ea9ba797145ed5e2c4d305064b932056451d Mon Sep 17 00:00:00 2001 From: Jaydeep Dave Date: Fri, 14 Aug 2026 02:10:32 +0530 Subject: [PATCH 3/4] fix(tfa): resolve workingBranch per repo, not from a single build-level value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior instruction told the orchestrator to stamp Part B's single resolved "working branch" onto every repo's evidence entry via setGithubEvidence. Two problems: fetchBuildInsights(buildId).branch is SDK-build metadata and describes only the build's own trigger repo, not every dependency; and a multi-repo build routinely ships different repos off different branches (product repo on main, automation repo on release/2026.08, etc). Applying one branch across all of them would make the new wrong-base-branch check reject legitimate PRs. workingBranch is already stored per-repo in the evidence file (no code change needed) — this fixes only the SKILL.md instruction to resolve it independently per repo, falling back through fetchBuildInsights (when it describes that repo) -> the connector's own per-repo intake-defaults (the right authority for non-SDK builds) -> that repo's default branch as a last resort, guess-flagged. Leaves workingBranch: null (fail-open, same as the existing suspectWindow.startedAt gap) when nothing resolves for a repo, rather than guessing main/master. --- skills/rca-build/SKILL.md | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/skills/rca-build/SKILL.md b/skills/rca-build/SKILL.md index 276568a..f82fd6b 100644 --- a/skills/rca-build/SKILL.md +++ b/skills/rca-build/SKILL.md @@ -358,6 +358,9 @@ is the point: 3. Only if neither is available, fall through to the connector's intake-defaults, then the current git branch, per the existing order below. + This resolves the branch for the build's own trigger only — a multi-repo + build's OTHER repos each need their own branch resolved independently at + Step 4 (`workingBranch` per repo), not this single value. - cheap inference (e.g. the automation repo is the cwd if it holds the tests). **Check the selected connector skill's own intake-defaults section FIRST — before @@ -593,11 +596,36 @@ leaves the other free to reintroduce the bug. evidenceType, fn)` to dedupe if two steps need the same `(repo, range)`. Digest the result into the `evidence-block.md` shape, then persist via `setGithubEvidence(path, repo, {deployState, workingBranch, prsInWindow, gap}, nowMs)`. - **Always set `workingBranch` to the exact `` resolved in Gate Part - B (`fetchBuildInsights(buildId).branch`, never assumed main/master)** — it - is what `lib/pr-validation.mjs` checks a candidate PR's `baseRefName` - against, so a PR that shipped to a different branch than this build - actually ran on is rejected instead of trusted on timing alone. + + **Resolve `workingBranch` for THIS repo, independently — never copy Part + B's single "working branch" across every repo in the union.** Part B + resolves one branch value for the build's own trigger (the automation/test + repo it named); a multi-repo build routinely ships different repos off + different branches (e.g. the product repo on `main`, the automation repo on + `release/2026.08`), and `fetchBuildInsights` describes only the build's own + branch, not every dependency's. Per repo, in this order: + 1. `fetchBuildInsights(buildId).branch` — **only valid for the repo it + actually describes** (the SDK/automation repo the build ran); do not + apply it to a different repo in the same union. It is also SDK-build + metadata: absent entirely on non-SDK builds. + 2. The active connector skill's own intake-defaults, when it declares a + branch/lane per repo (the customer's own skill is the right authority + for non-SDK builds and for every repo `fetchBuildInsights` doesn't + cover). + 3. That repo's default branch (`gh api repos/OWNER/REPO --jq + '.default_branch'`) — **only as a last resort, and never silently + treated as authoritative**: it is a guess, not the branch this build + verified against. + If none resolves for a repo, leave `workingBranch: null` for it — this is + the SAME fail-open behavior `lib/pr-validation.mjs` already has for a + missing `suspectWindow.startedAt`: the branch check is skipped for that + repo rather than rejecting every candidate on an unresolved guess. Never + assume main/master when nothing resolves. + + `workingBranch` is what `lib/pr-validation.mjs` checks a candidate PR's + `baseRefName` against, so a PR that shipped to a different branch than + this build actually ran on is rejected instead of trusted on timing alone + — but only for repos where the branch was genuinely resolved, not guessed. A repo the connector can't reach records `{gap: ""}` — never blocks the rest of the pre-fetch. From 263456adb7f431d5a9b62c32e04c8f563e9eb05e Mon Sep 17 00:00:00 2001 From: Jaydeep Dave Date: Fri, 14 Aug 2026 02:19:38 +0530 Subject: [PATCH 4/4] feat(tfa): local commit-history and blame helpers, no gh call needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lib/repo-source.mjs already replaced file-content gh calls (31% of traffic) with local git show at a pinned sha. Commit history was the next-largest slice (12%, per this module's own measured breakdown) and had no local path; blame had none at all. - commitHistoryAt({repo, fromSha, toSha, path?, workspaceRoot}): local `git log fromSha..toSha -- path`, same "shas only, never a branch" rule as readFileAt (a branch-scoped range silently drifts as the branch moves). - blameAt({repo, sha, path, lineRange?, workspaceRoot}): local `git blame --line-porcelain`, scoped to a failing stack frame's lines. Unlike the other helpers this isn't just faster than a gh call — there's no gh blame equivalent to replace; it removes a GitHub dependency for this ask entirely once the commit is present. Both share the same fail-open contract as readFileAt: report `remote-needed` rather than fetch or fall back themselves, so a caller without a usable local clone falls through to the gh-based path unchanged. Wired into github-evidence.md's evidence table and field-filtering table, documented in SKILL.md's API reference. 11 new tests against a real throwaway git repo (228/228 passing). --- lib/repo-source.mjs | 118 +++++++++++++++++- skills/rca-build/SKILL.md | 6 + .../rca-build/references/github-evidence.md | 4 +- tests/repo-source.test.mjs | 82 +++++++++++- 4 files changed, 205 insertions(+), 5 deletions(-) diff --git a/lib/repo-source.mjs b/lib/repo-source.mjs index 865430e..74da3d5 100644 --- a/lib/repo-source.mjs +++ b/lib/repo-source.mjs @@ -4,8 +4,10 @@ // Measured: `gh api .../contents/?ref=` ~1022ms; the same read as // `git show :` from a local clone ~37ms — 27x faster, and // byte-identical. Across three real runs, file CONTENTS were 126 of 407 gh -// calls (31%) and commit history another 48 (12%), so this is the largest -// remaining slice of github traffic. +// calls (31%) and commit history another 48 (12%) — `commitHistoryAt` below +// closes that second slice the same way. `blameAt` has no `gh` equivalent to +// measure against at all: blame requires no GitHub dependency once the +// commit is present, not just a faster path to the same answer. // // THE CORRECTNESS RULE: ALWAYS PIN TO A COMMIT SHA, NEVER A BRANCH NAME. // @@ -171,3 +173,115 @@ export function readFileAt({ repo, sha, path, workspaceRoot, branch, allowFetch return { ok: false, source: "remote-needed", reason: msg.slice(0, 200) }; } } + +/** Ensure `sha` is present locally, respecting `allowFetch` exactly like + * `readFileAt`. Returns `null` on success (nothing to report) or a + * `remote-needed` envelope on failure — shared by every helper below so the + * "not present, and don't fetch unless asked" contract stays in one place. */ +function ensurePresent(dir, sha, branch, allowFetch) { + if (hasCommit(dir, sha)) return null; + if (!allowFetch) return { ok: false, source: "remote-needed", reason: `commit ${sha} not present in ${dir} (pass allowFetch to fetch it once)` }; + if (!ensureCommit(dir, sha, branch)) return { ok: false, source: "remote-needed", reason: `commit ${sha} still absent after fetch` }; + return null; +} + +/** + * Commit history touching `path` in the range `fromSha..toSha` (exclusive of + * `fromSha`, inclusive of `toSha` — same semantics as `git log A..B`). Local + * equivalent of `gh api "repos/.../commits?sha="`, the second-largest + * slice of `gh` traffic per this module's header (12% across three real + * runs) with no local-clone path until now. + * + * Returns `{ ok, source: "local"|"remote-needed", commits: [{sha, date, + * subject}], reason }`. Omit `path` to get the repo-wide history for the + * range. Both `fromSha` and `toSha` must be commit shas — same "never a + * branch name" rule as `readFileAt`, for the same reason: a branch-scoped + * range silently drifts as the branch moves. + */ +export function commitHistoryAt({ repo, fromSha, toSha, path, workspaceRoot, branch, allowFetch = false }) { + if (!SHA.test(String(fromSha ?? "")) || !SHA.test(String(toSha ?? ""))) { + return { ok: false, source: "remote-needed", reason: `fromSha and toSha must both be commit shas, got ${JSON.stringify(fromSha)}..${JSON.stringify(toSha)}` }; + } + const dir = localCloneFor(repo, workspaceRoot); + if (!dir) return { ok: false, source: "remote-needed", reason: `no local clone of ${repo} under ${workspaceRoot}` }; + + for (const sha of [fromSha, toSha]) { + const missing = ensurePresent(dir, sha, branch, allowFetch); + if (missing) return missing; + } + + try { + // \x1f (unit separator) can't appear in a commit subject, so it is a safe + // field delimiter without needing to escape/parse quoted output. + const args = ["log", "--pretty=format:%H\x1f%cI\x1f%s", `${fromSha}..${toSha}`]; + if (path) args.push("--", path); + const raw = git(dir, args); + const commits = raw + ? raw.split("\n").filter(Boolean).map((line) => { + const [sha, date, subject] = line.split("\x1f"); + return { sha, date, subject }; + }) + : []; + return { ok: true, source: "local", commits, dir }; + } catch (err) { + return { ok: false, source: "remote-needed", reason: String(err.stderr ?? err.message ?? "").slice(0, 200) }; + } +} + +function parseLinePorcelainBlame(raw) { + const out = []; + let cur = null; + for (const line of raw.split("\n")) { + const header = line.match(/^([0-9a-f]{40}) \d+ (\d+)(?: \d+)?$/); + if (header) { + if (cur) out.push(cur); + cur = { sha: header[1], line: Number(header[2]), author: null, date: null, content: "" }; + continue; + } + if (!cur) continue; + if (line.startsWith("author ")) cur.author = line.slice("author ".length); + else if (line.startsWith("author-time ")) cur.date = new Date(Number(line.slice("author-time ".length)) * 1000).toISOString(); + else if (line.startsWith("\t")) cur.content = line.slice(1); + } + if (cur) out.push(cur); + return out; +} + +/** + * Blame `path` at a pinned commit — "who/what last changed the failing + * line," the first ask in `references/github-evidence.md`'s evidence table. + * Pure local git plumbing: unlike every other helper here this isn't just + * faster than the network, there IS no equivalent single `gh` call — blame + * requires no GitHub dependency at all once the commit is present. + * + * `lineRange` is 1-indexed and inclusive, e.g. `{start: 40, end: 55}` from a + * stack frame; omit it to blame the whole file (expensive on a large file — + * always scope to the failing lines when you have them). + * + * Returns `{ ok, source: "local"|"remote-needed", lines: [{sha, author, + * date, line, content}], reason }`. + */ +export function blameAt({ repo, sha, path, lineRange, workspaceRoot, branch, allowFetch = false }) { + if (!SHA.test(String(sha ?? ""))) { + return { ok: false, source: "remote-needed", reason: `ref must be a commit sha, got ${JSON.stringify(sha)} (a branch name can silently blame stale code)` }; + } + const dir = localCloneFor(repo, workspaceRoot); + if (!dir) return { ok: false, source: "remote-needed", reason: `no local clone of ${repo} under ${workspaceRoot}` }; + + const missing = ensurePresent(dir, sha, branch, allowFetch); + if (missing) return missing; + + const args = ["blame", "--line-porcelain"]; + if (lineRange?.start && lineRange?.end) args.push("-L", `${lineRange.start},${lineRange.end}`); + args.push(sha, "--", path); + + try { + return { ok: true, source: "local", lines: parseLinePorcelainBlame(git(dir, args)), dir }; + } catch (err) { + const msg = String(err.stderr ?? err.message ?? ""); + if (/no such path|does not exist|is outside repository/i.test(msg)) { + return { ok: false, source: "local", reason: `path not present at ${sha}: ${path}` }; + } + return { ok: false, source: "remote-needed", reason: msg.slice(0, 200) }; + } +} diff --git a/skills/rca-build/SKILL.md b/skills/rca-build/SKILL.md index f82fd6b..b971266 100644 --- a/skills/rca-build/SKILL.md +++ b/skills/rca-build/SKILL.md @@ -95,7 +95,13 @@ validateAndDeduplicatePRs(relatedPrs, evidenceDoc) → deduped, validated entrie discoverWorkspaceRoot({repos, from, explicit, maxTries=3}) → {root, matched, tried, reason} resolveLocalRepos({repos, pins, workspaceRoot}) → {repo:{usable, sha|reason}} readFileAt({repo, sha, path, workspaceRoot}) → sha ONLY; a branch name is refused +commitHistoryAt({repo, fromSha, toSha, path?, workspaceRoot}) → {ok, source, commits:[{sha,date,subject}]} + local `git log fromSha..toSha` — both must be shas, never a branch; omit `path` for repo-wide history +blameAt({repo, sha, path, lineRange?, workspaceRoot}) → {ok, source, lines:[{sha,author,date,line,content}]} + scope lineRange:{start,end} to the failing stack frame — no gh equivalent needed once the commit is local ``` +All four `{ok, source: "local"|"remote-needed", reason}`-shaped calls never fall back to the network +themselves — a `remote-needed` result means the caller falls through to the discovered `github` connector. **Housekeeping — `lib/state-dir.mjs`** ``` diff --git a/skills/rca-build/references/github-evidence.md b/skills/rca-build/references/github-evidence.md index 372cefb..803d31c 100644 --- a/skills/rca-build/references/github-evidence.md +++ b/skills/rca-build/references/github-evidence.md @@ -80,7 +80,7 @@ input-from-output dependency. |---|---| | "Did `` change since the last passing run?" | the diff of ``'s file/function between the **baseline ref** (last-green, or the configured fallback) and the build's commit — not the whole repo diff | | "Which PRs are suspect?" | PRs **merged in the window** `(baselineRef, build commit]` that **touch the failing code path** — intersect changed files with the failing file/function | -| "Who/what last changed the failing line?" | `blame` on the specific failing lines (from the test's `file_path` + the error) | +| "Who/what last changed the failing line?" | `blame` on the specific failing lines (from the test's `file_path` + the error) — **prefer `blameAt` (`lib/repo-source.mjs`)** when `localRepos` shows this repo usable at the pinned sha: pure local git, no GitHub dependency at all, not just faster | | "What shipped to the run's env before the failure?" | deploy timeline (`gh` releases/tags + the env's deploy record); compare deploy time vs. the run's `started_at` | | "Did CI change?" | the workflow-file diff + recent `gh run` history for the failing job | @@ -102,7 +102,7 @@ manifest resolved to), since the failure mode is identical. |---|---|---| | Repo exists / default branch | `gh api repos/OWNER/REPO` | `gh api repos/OWNER/REPO --jq '.default_branch'` | | Branch exists on the shipping branch | `gh api repos/OWNER/REPO/branches/BRANCH` | `gh api repos/OWNER/REPO/branches/BRANCH --jq '.name'` | -| Commit history / PR-window search | `gh api "repos/OWNER/REPO/commits?sha=BRANCH&per_page=100"` | add `--jq '[.[] | {sha: .sha[0:8], date: .commit.committer.date, msg: (.commit.message | split("\n")[0])}]'` | +| Commit history / PR-window search | `gh api "repos/OWNER/REPO/commits?sha=BRANCH&per_page=100"` | add `--jq '[.[] | {sha: .sha[0:8], date: .commit.committer.date, msg: (.commit.message | split("\n")[0])}]'`. **Prefer `commitHistoryAt` (`lib/repo-source.mjs`)** when `localRepos` shows this repo usable — same answer, no network call, and it takes two pinned shas (never a branch) so it can't drift the way a `BRANCH`-scoped query can | | PR metadata | `gh pr view N --repo OWNER/REPO` (full payload) | `gh pr view N --repo OWNER/REPO --json state,mergedAt,baseRefName,headRefOid,files,author` — `--json` is itself a field allowlist; list only the fields this ask uses. **Always keep `baseRefName`** — the code-level validation gate rejects a PR whose base branch doesn't match the build's actual working branch, so dropping this field silently disables that check | | Pod / workload listing | `kubectl get pods -n NS -o wide` | `kubectl get pods -n NS -o custom-columns='NAME:.metadata.name,STATUS:.status.phase'` | | Deploy / image state | `kubectl get deploy -n NS -o yaml` | `kubectl get deploy -n NS -o custom-columns='NAME:.metadata.name,IMAGE:.spec.template.spec.containers[0].image'` | diff --git a/tests/repo-source.test.mjs b/tests/repo-source.test.mjs index 08cfb40..2f0ce93 100644 --- a/tests/repo-source.test.mjs +++ b/tests/repo-source.test.mjs @@ -4,7 +4,7 @@ import { mkdtempSync, rmSync, mkdirSync, writeFileSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { localCloneFor, hasCommit, readFileAt, discoverWorkspaceRoot, resolveLocalRepos } from "../lib/repo-source.mjs"; +import { localCloneFor, hasCommit, readFileAt, discoverWorkspaceRoot, resolveLocalRepos, commitHistoryAt, blameAt } from "../lib/repo-source.mjs"; let ws, repoDir, sha1, sha2; @@ -146,3 +146,83 @@ test("path missing at that commit is a local answer, not a remote fallback", () assert.equal(r.source, "local"); assert.match(r.reason, /path not present/); }); + +// --- commitHistoryAt --- + +test("commitHistoryAt lists commits touching a path between two shas", () => { + const r = commitHistoryAt({ repo: "browserstack/testrepo", fromSha: sha1, toSha: sha2, path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, true); + assert.equal(r.source, "local"); + assert.equal(r.commits.length, 1); + assert.equal(r.commits[0].sha, sha2); + assert.equal(r.commits[0].subject, "two"); + assert.ok(r.commits[0].date, "date should be populated"); +}); + +test("commitHistoryAt refuses a branch name for either endpoint", () => { + const r = commitHistoryAt({ repo: "browserstack/testrepo", fromSha: "main", toSha: sha2, path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "remote-needed"); + assert.match(r.reason, /must both be commit shas/); +}); + +test("commitHistoryAt: empty range (fromSha === toSha) returns no commits, not an error", () => { + const r = commitHistoryAt({ repo: "browserstack/testrepo", fromSha: sha2, toSha: sha2, path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, true); + assert.deepStrictEqual(r.commits, []); +}); + +test("commitHistoryAt: an absent sha defers to the caller, no fetch unless asked", () => { + const r = commitHistoryAt({ repo: "browserstack/testrepo", fromSha: sha1, toSha: "0".repeat(40), path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "remote-needed"); + assert.match(r.reason, /not present|allowFetch/); +}); + +test("commitHistoryAt: no local clone defers to the caller", () => { + const r = commitHistoryAt({ repo: "browserstack/absent", fromSha: sha1, toSha: sha2, workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "remote-needed"); + assert.match(r.reason, /no local clone/); +}); + +// --- blameAt --- + +test("blameAt attributes each line to the commit that introduced it", () => { + const r = blameAt({ repo: "browserstack/testrepo", sha: sha2, path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, true); + assert.equal(r.source, "local"); + assert.equal(r.lines.length, 1); + assert.equal(r.lines[0].sha, sha2); + assert.equal(r.lines[0].content, "VERSION_TWO"); + assert.ok(r.lines[0].author, "author should be populated"); + assert.ok(r.lines[0].date, "date should be populated"); +}); + +test("blameAt scoped to a lineRange narrows the result", () => { + const r = blameAt({ repo: "browserstack/testrepo", sha: sha2, path: "app.js", lineRange: { start: 1, end: 1 }, workspaceRoot: ws }); + assert.equal(r.ok, true); + assert.equal(r.lines.length, 1); + assert.equal(r.lines[0].line, 1); +}); + +test("blameAt refuses a branch name", () => { + const r = blameAt({ repo: "browserstack/testrepo", sha: "main", path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "remote-needed"); + assert.match(r.reason, /must be a commit sha/); +}); + +test("blameAt: path missing at that commit is a local answer, not a remote fallback", () => { + const r = blameAt({ repo: "browserstack/testrepo", sha: sha1, path: "nope.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "local"); + assert.match(r.reason, /path not present/); +}); + +test("blameAt: no local clone defers to the caller", () => { + const r = blameAt({ repo: "browserstack/absent", sha: sha1, path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "remote-needed"); + assert.match(r.reason, /no local clone/); +});