fix(tfa): code-enforced PR validation gate for build-level causation - #3
Draft
Dave3130 wants to merge 4 commits into
Draft
fix(tfa): code-enforced PR validation gate for build-level causation#3Dave3130 wants to merge 4 commits into
Dave3130 wants to merge 4 commits into
Conversation
… 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 (<reason>)", 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.
Dave3130
commented
Aug 13, 2026
Dave3130
left a comment
Dave3130
left a comment
Author
There was a problem hiding this comment.
Claude Code Review (automated) — 4 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| export function validateAndDeduplicatePRs(relatedPrs, evidenceDoc) { | ||
| if (!Array.isArray(relatedPrs) || relatedPrs.length === 0) return []; | ||
|
|
||
| // Deduplicate by (repo, number). |
Author
There was a problem hiding this comment.
[Medium] Dedup keeps first entry, silently drops fields from later duplicates
The dedup loop keeps the first entry for a given repo+number key. If a later duplicate carries fields the first lacks (e.g. merged_at), those are silently lost. Ground-truth enrichment partially compensates but only for evidence-file fields.
Suggestion: Merge duplicates using a field-level merge (similar to mergePrEntry in evidence-file.mjs) rather than discarding later entries.
Reviewer: fallback
| const match = prsInWindow.find( | ||
| (p) => normalise(p.pr) === normalise(num), | ||
| ); | ||
| if (!match) return { valid: false, reason: "not-in-evidence" }; |
Author
There was a problem hiding this comment.
[Medium] normalise helper defined identically in two places
This same arrow function is also defined at line 90. Extract to module scope.
Suggested change
| if (!match) return { valid: false, reason: "not-in-evidence" }; | |
| const normalise = (n) => String(n).replace(/^#/, ""); |
Reviewer: fallback
| const results = []; | ||
| for (const entry of seen.values()) { | ||
| const repoKey = resolveRepo(entry); | ||
| const num = entry?.number ?? entry?.pr; |
Author
There was a problem hiding this comment.
[Low] Ground-truth enrichment can silently overwrite verdict
This spreads all non-null evidence fields over the entry. Currently safe only because evidence PRs lack a verdict field. Make the guarantee explicit.
Suggestion: Object.entries(groundTruth).filter(([k, v]) => v != null && k !== 'verdict')
Reviewer: fallback
|
|
||
| const out = (status, turn, note) => { | ||
| const glimpse = turn?.glimpse ?? {}; | ||
| const rawPrs = glimpse.related_prs ?? []; |
Author
There was a problem hiding this comment.
[Low] Validation only runs for RESOLVED status
A FAILED or PENDING result with hallucinated related_prs skips validation. Likely intentional but worth documenting the design choice.
Reviewer: fallback
Author
Claude Code PR ReviewPR: #3 • Head: 3ad707a • Reviewers: fallback inline checklist SummaryAdds a code-enforced PR validation gate ( Review Table
Findings
Over-engineering pass (ponytail)
net: -5 lines possible. Verdict: PASS |
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).
…el value 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.
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes four production issues in the TFA Build-Level PR causation flow (
/rca-build), reported by two BrowserStack QA teams against real regression builds:Hallucinated/inconsistent PR attribution — same PR number reported with 3 different titles; non-existent PRs; wrong-repo cross-wiring. Fixed via a new code-enforced validation gate (
lib/pr-validation.mjs) at thelib/loop.mjsout()choke point, cross-checking every claimed PR against the evidence file's ground truth (owner+repo+number). Invalid PRs are downgraded toverdict: "ruled-out (<reason>)", never silently dropped.PRs merged outside the regression window reported as failures' cause — fixed by the validation gate's merge-window check (
merged_atvs the build'sstarted_at).PR authors frequently reported as "unknown" — root-caused to
evidence-file.mjs'sfoldGithubdoing whole-object replacement on PR-dedup collisions, silently dropping fields likeauthorfrom an earlier shard. Fixed via field-level merge (keep first non-null value per field).Reports silently not published to the Test Observability dashboard despite the run claiming "analysis complete" — fixed by making
SKILL.mdStep 6'striggerRcaReportcall success-gated: completion glimpse andcleanupBuildArtifactsnow only run after confirmed success, with explicit failure-path handling.Prompt-contract updates
Updated 4 docs (
agents/ai-tfa-coordinator.md,skills/rca-build/references/github-evidence.md,skills/rca-build/templates/suspect-packet.md,skills/rca-build/SKILL.md) to describe the new code-enforced validation as primary enforcement (LLM falsification protocol is now defense-in-depth).Files changed
lib/pr-validation.mjslib/evidence-file.mjsfoldGithubPR deduplib/loop.mjsout()choke pointskills/rca-build/SKILL.mdtriggerRcaReport, failure-path handlingagents/ai-tfa-coordinator.mdskills/rca-build/references/github-evidence.mdskills/rca-build/templates/suspect-packet.mdtests/pr-validation.test.mjstests/evidence-file-author-merge.test.mjstests/loop-pr-validation.test.mjsTest plan
References
.claude/dev/prds/fix-tfa-build-level-pr-causation.mddocs/prd/fix-tfa-build-level-pr-causation-tech-spec.md