From 3ea9018dd9125167bd75dcfab5786fa4cccbaad4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 18:20:06 -0700 Subject: [PATCH 1/4] fix(review): explain validation and guardrail holds --- src/queue/processors.ts | 13 +-- src/settings/agent-actions.ts | 16 +++- src/signals/engine.ts | 6 +- src/signals/focus-manifest.ts | 10 ++- src/signals/test-evidence.ts | 4 + test/unit/agent-actions.test.ts | 13 +++ test/unit/focus-manifest.test.ts | 8 +- test/unit/gate-check-policy.test.ts | 2 +- .../unit/public-safe-manifest-finding.test.ts | 4 +- test/unit/queue.test.ts | 86 +++++++++++++++++++ test/unit/test-evidence.test.ts | 8 +- 11 files changed, 147 insertions(+), 23 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 1b142370ee..6b54ed6c62 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -182,7 +182,7 @@ import { buildPullRequestAdvisory, evaluateGateCheck, } from "../rules/advisory"; -import { isTestPath } from "../signals/test-evidence"; +import { hasValidationNote, isTestPath } from "../signals/test-evidence"; import { detectNotificationEvents } from "../notifications/events"; import { deliverNotification, @@ -832,13 +832,14 @@ export async function runRetentionPrune( const PUBLIC_MANIFEST_POLICY_FINDING_OVERRIDES: Partial< Record< FocusManifestFinding["code"], - Pick + Pick > > = { manifest_missing_tests: { - detail: "Maintainer test expectations are not satisfied by this PR.", + title: "Configured validation evidence missing", + detail: "No changed test files or passing validation evidence were detected for this PR.", action: - "Add or update tests, or attach passing validation output that satisfies the maintainer's test expectations.", + "Add regression/invariant coverage, update relevant tests, or attach passing validation output that satisfies the repo's configured expectations.", }, }; @@ -852,7 +853,7 @@ export function publicSafeManifestPolicyFinding( detail: finding.detail, /* v8 ignore next -- the three manifest policy findings always carry an action; the no-action arm is unreachable. */ ...(finding.action !== undefined ? { action: finding.action } : {}), - // Override the leaky detail/action with a static, public-safe version for the codes whose raw text would echo + // Override the leaky title/detail/action with static, public-safe text for codes whose raw text would echo // private blocked-path globs / test expectations; codes absent from the table keep their already-generic text. ...PUBLIC_MANIFEST_POLICY_FINDING_OVERRIDES[finding.code], }; @@ -7610,7 +7611,7 @@ async function maybePublishPrPublicSurface( linkedIssueCount: pr.linkedIssues.length, testFileCount: manifestFiles.filter((file) => isTestPath(file.path)) .length, - passedValidationCount: 0, + passedValidationCount: hasValidationNote(pr.body ?? "") ? 1 : 0, }); const policyCodes = new Set([ "manifest_blocked_path", diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index db87814e0f..03deea9f37 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -1,7 +1,7 @@ import type { AgentActionClass, AutoMaintainPolicy, AutoMergeMethod, AutonomyPolicy } from "../types"; import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckConclusion } from "../rules/advisory"; import { DEFAULT_AUTO_MAINTAIN_POLICY, autonomyRequiresApproval, isActingAutonomyLevel, resolveAutonomy } from "./autonomy"; -import { isGuardrailHit } from "../signals/change-guardrail"; +import { changedPathsHittingGuardrail, isGuardrailHit } from "../signals/change-guardrail"; import { AGENT_LABEL_PENDING_CLOSURE } from "../review/linked-issue-hard-rules"; import { sanitizePublicComment } from "../github/commands"; @@ -357,6 +357,13 @@ function resolveAgentDispositionLabels(settings: AgentDispositionLabelSettings): }; } +function guardrailHoldReason(changedPaths: string[], hardGuardrailGlobs: string[]): string { + const matches = changedPathsHittingGuardrail(changedPaths, hardGuardrailGlobs); + if (matches.length === 0) return "guarded path -> manual review (changed-file list unavailable)"; + const visible = matches.slice(0, 3).map((path) => `\`${path}\``).join(", "); + return `guarded path -> manual review (${visible}${matches.length > 3 ? `, and ${matches.length - 3} more` : ""})`; +} + /** * Accuracy circuit-breaker (#self-improve / GAP-4): when auto-merge is DISABLED for a repo (the auto-tuner * engaged the holdonly flag after merge precision dropped, or a human set it), DOWNGRADE a would-MERGE into a @@ -620,6 +627,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // never auto-merge, auto-approve, or auto-close a PR whose diff we don't know. Repos with no guardrails // configured stay permissive. const guardrailHit = isGuardrailHit(input.changedPaths, input.hardGuardrailGlobs); + const guardrailReason = guardrailHit ? guardrailHoldReason(input.changedPaths, input.hardGuardrailGlobs) : null; // Manual review is the RARE exception (the operator's minimize-manual goal): the ONLY things that hold a PR // for a human instead of merge/close are an auto-merge-ready PR that touches a hard-guardrail path, or a // live migration-number collision detected against the CURRENT tip of the base branch (#2550 — a sibling PR @@ -702,7 +710,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne actionClass: "label", autonomyClass: "merge", requiresApproval: false, - reason: `verdict=${conclusion}; guarded path → manual review`, + reason: `verdict=${conclusion}; ${guardrailReason ?? "guarded path -> manual review"}`, label: labels.manualReview, labelOp: "add", }); @@ -727,7 +735,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne : input.migrationCollisionHold !== undefined ? `verdict=${conclusion}; ${input.migrationCollisionHold.reason}` : heldForManualReview - ? `verdict=${conclusion}; guarded path → manual review` + ? `verdict=${conclusion}; ${guardrailReason ?? "guarded path -> manual review"}` : `verdict=${conclusion}; CI green`; if (label !== null && !hasLabelOrPlanned(input.pr.labels, actions, label)) { actions.push({ @@ -913,7 +921,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // review-good-but-not-yet-mergeable → held briefly (rebase/approve resolves it next pass). const manualHoldReason = guardrailHit - ? `verdict=${conclusion}; guarded path → manual review` + ? `verdict=${conclusion}; ${guardrailReason ?? "guarded path -> manual review"}` : ciUnverified ? "CI could not be verified" : conclusion === "action_required" diff --git a/src/signals/engine.ts b/src/signals/engine.ts index 5611655f99..e3fd3336be 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -26,7 +26,7 @@ import type { GittensorContributorSnapshot } from "../gittensor/api"; import { nowIso } from "../utils/json"; import { sanitizePublicComment } from "../queue-intelligence"; import { labelMatchesPattern, projectLinkedIssueMultiplierForPlannedSolve, type LinkedIssueMultiplierStatus } from "../scoring/preview"; -import { hasLocalTestEvidence, isTestPath } from "./test-evidence"; +import { hasLocalTestEvidence, hasValidationNote, isTestPath } from "./test-evidence"; import { isFailingCheckSummary } from "./local-branch"; import { isDuplicateClusterWinnerByClaim } from "./duplicate-winner"; import { PREFLIGHT_LIMITS } from "./preflight-limits"; @@ -5012,10 +5012,6 @@ export function hasClearNoIssueRationale(pr: Pick 0 && testFileCount === 0 && passedValidationCount === 0) { + const safeExpectations = manifest.testExpectations.filter(isFocusManifestPublicSafe).slice(0, 3); + const expectationDetail = safeExpectations.length > 0 ? ` Expected evidence: ${safeExpectations.join("; ")}.` : ""; findings.push({ code: "manifest_missing_tests", severity: "warning", - title: "Maintainer test expectations unmet", - detail: `Maintainer expects test evidence: ${manifest.testExpectations.slice(0, 3).join("; ")}.`, - action: "Add or update tests, or attach passing validation output that satisfies the maintainer's test expectations.", + title: "Configured validation evidence missing", + detail: `No changed test files or passing validation evidence were detected for this PR.${expectationDetail}`, + action: "Add regression/invariant coverage, update relevant tests, or attach passing validation output that satisfies the repo's configured expectations.", }); - publicNextSteps.push("Add tests or attach passing validation that meets the maintainer's test expectations."); + publicNextSteps.push("Add relevant tests or passing validation evidence that matches the repo's configured expectations."); } if (manifest.issueDiscoveryPolicy === "discouraged") { diff --git a/src/signals/test-evidence.ts b/src/signals/test-evidence.ts index b89457fc26..edfa515abc 100644 --- a/src/signals/test-evidence.ts +++ b/src/signals/test-evidence.ts @@ -20,6 +20,10 @@ export function hasLocalTestEvidence(input: { tests?: string[] | undefined; test return (input.tests ?? []).length > 0 || (input.testFiles ?? []).some((file) => isTestPath(file)); } +export function hasValidationNote(value: string): boolean { + return /\b(test(?:ed|s|ing)?|validation|validated|verified|manual check|smoke|pytest|vitest|npm test|pnpm test|cargo test|go test)\b/i.test(value); +} + /** * Coarse classification of how much test coverage accompanies a set of changed paths. * Used by slop signals to weight diffs that touch source but include no tests differently diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 6c88203890..97fb58972b 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -387,9 +387,22 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(label?.label).toBe(AGENT_LABEL_NEEDS_REVIEW); expect(label?.label).not.toBe(AGENT_LABEL_READY); expect(label?.reason).toContain("guarded path"); + expect(label?.reason).toContain("src/scoring/model.ts"); expect(classes(plan)).not.toContain("merge"); }); + it("explains when guardrail hold is fail-closed because changed files are unavailable", () => { + const label = planAgentMaintenanceActions(input({ + conclusion: "success", + autonomy: { merge: "auto" }, + changedPaths: [], + hardGuardrailGlobs: ["src/scoring/**"], + pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" }, + })).find((a) => a.actionClass === "label"); + expect(label?.label).toBe(AGENT_LABEL_NEEDS_REVIEW); + expect(label?.reason).toContain("changed-file list unavailable"); + }); + it("does not re-plan the manual-review label when the guarded PR already carries it (idempotent)", () => { const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, ...guarded, pr: { labels: [AGENT_LABEL_NEEDS_REVIEW] } }))); expect(plan).not.toContain("label"); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 028f48f680..91b1d67455 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -339,7 +339,13 @@ describe("buildFocusManifestGuidance", () => { it("surfaces missing preferred labels and test expectations", () => { const guidance = buildFocusManifestGuidance({ manifest: wanted, changedPaths: ["src/x.ts"], labels: [], linkedIssueCount: 1, testFileCount: 0, passedValidationCount: 0 }); expect(guidance.findings.some((finding) => finding.code === "manifest_missing_preferred_label")).toBe(true); - expect(guidance.findings.some((finding) => finding.code === "manifest_missing_tests")).toBe(true); + const missingTests = guidance.findings.find((finding) => finding.code === "manifest_missing_tests"); + expect(missingTests).toMatchObject({ + title: "Configured validation evidence missing", + detail: expect.stringContaining("No changed test files or passing validation evidence were detected"), + action: "Add regression/invariant coverage, update relevant tests, or attach passing validation output that satisfies the repo's configured expectations.", + }); + expect(missingTests?.detail).toContain("unit tests for new branches."); }); it("treats passing validation as satisfying test expectations", () => { diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index 6ef3b5f927..fd3bc73648 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -601,7 +601,7 @@ describe("focus-manifest policy gate (#555)", () => { // Path-based manual review lives in settings.hardGuardrailGlobs, not manifest policy. const POLICY_FINDINGS = { manifest_linked_issue_required: { code: "manifest_linked_issue_required", title: "Maintainer requires a linked issue", severity: "warning" as const, detail: "Manifest requires a linked issue.", action: "Link the issue." }, - manifest_missing_tests: { code: "manifest_missing_tests", title: "Maintainer test expectations unmet", severity: "warning" as const, detail: "Manifest expects test evidence.", action: "Add tests." }, + manifest_missing_tests: { code: "manifest_missing_tests", title: "Configured validation evidence missing", severity: "warning" as const, detail: "No changed test files or passing validation evidence were detected.", action: "Add regression/invariant coverage, update relevant tests, or attach passing validation output." }, }; function manifestAdvisory(code: keyof typeof POLICY_FINDINGS): Advisory { diff --git a/test/unit/public-safe-manifest-finding.test.ts b/test/unit/public-safe-manifest-finding.test.ts index d5f1971bf5..cafce050cc 100644 --- a/test/unit/public-safe-manifest-finding.test.ts +++ b/test/unit/public-safe-manifest-finding.test.ts @@ -14,9 +14,11 @@ describe("publicSafeManifestPolicyFinding", () => { action: "Add or update tests for the private fuzz suite.", }; const safe = publicSafeManifestPolicyFinding(finding); + expect(safe.title).toBe("Configured validation evidence missing"); expect(safe.detail).not.toContain("private fuzz suite"); expect(safe.action).not.toContain("private fuzz suite"); - expect(safe.detail).toBe("Maintainer test expectations are not satisfied by this PR."); + expect(safe.detail).toBe("No changed test files or passing validation evidence were detected for this PR."); + expect(safe.action).toBe("Add regression/invariant coverage, update relevant tests, or attach passing validation output that satisfies the repo's configured expectations."); }); it("passes through a finding whose detail is already generic (no override)", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 916c73bf16..0bac68d48f 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6266,6 +6266,92 @@ describe("queue processors", () => { }); }); + it("accepts PR-body validation evidence for configured manifest test expectations", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertInstallation(env, { + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "write", issues: "write" }, + events: ["pull_request"], + }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "off", + manifestPolicyGateMode: "block", + requireLinkedIssue: false, + typeLabelsEnabled: false, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { testExpectations: ["Run npm run test:ci."] }); + await upsertPullRequestFile(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 43, + path: "src/feature.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }); + + const gatePatches: Array> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/commits/gate-validation/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 901 }, { status: 201 }); + if (url.includes("/check-runs/901") && method === "PATCH") { + gatePatches.push(JSON.parse(String(init?.body ?? "{}")) as Record); + return Response.json({ id: 901, html_url: "https://github.com/checks/901" }); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "gate-validation-evidence", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 43, + title: "Validated change", + state: "open", + user: { login: "contributor" }, + head: { sha: "gate-validation" }, + labels: [], + body: "Validated with npm run test:ci.", + }, + }, + }); + + expect(gatePatches).toHaveLength(1); + expect(gatePatches[0]).toMatchObject({ status: "completed", conclusion: "success" }); + expect(JSON.stringify(gatePatches[0])).not.toContain("Configured validation evidence missing"); + expect(JSON.stringify(gatePatches[0])).not.toContain("manifest_missing_tests"); + }); + it("stamps a gate-only surface even when local Gate check-summary persistence fails", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( diff --git a/test/unit/test-evidence.test.ts b/test/unit/test-evidence.test.ts index beb91f2a42..41c3f9aa78 100644 --- a/test/unit/test-evidence.test.ts +++ b/test/unit/test-evidence.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { classifyTestCoverage, hasLocalTestEvidence, isTestPath } from "../../src/signals/test-evidence"; +import { classifyTestCoverage, hasLocalTestEvidence, hasValidationNote, isTestPath } from "../../src/signals/test-evidence"; describe("test evidence helpers", () => { it("detects common test path conventions", () => { @@ -76,6 +76,12 @@ describe("test evidence helpers", () => { expect(hasLocalTestEvidence({ tests: [] })).toBe(false); expect(hasLocalTestEvidence({})).toBe(false); }); + + it("detects PR-body validation notes used by review and manifest-policy gates", () => { + expect(hasValidationNote("Validated with npm run test:ci and a smoke run.")).toBe(true); + expect(hasValidationNote("Manual check passed for the dashboard.")).toBe(true); + expect(hasValidationNote("Refactors the route naming only.")).toBe(false); + }); }); describe("classifyTestCoverage", () => { From 03fbb7177c74778083d552837e17672ae0cae777 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 19:32:37 -0700 Subject: [PATCH 2/4] fix(review): reject negative test/validation evidence, close coverage gaps hasValidationNote matched any mention of testing regardless of polarity, so a PR body like "No tests run" or "Tests not run" satisfied a configured manifest test expectation and let manifest_missing_tests disappear on a reachable live gate path. Add a negation guard covering both word orders before falling through to the existing affirmative match. Thread the same helper into the pre-submission predictor so it stops predicting stricter than the live gate on this exact finding, and close the codecov/patch gap: cover the truncated guardrail-path list, the all-unsafe test-expectations case, and a null PR body through the live webhook path. Simplify the redundant guardrailReason fallback in agent-actions.ts to a single non-nullable default instead of three unreachable `?? fallback` arms. --- src/rules/predicted-gate.ts | 7 +- src/settings/agent-actions.ts | 11 +- src/signals/test-evidence.ts | 8 ++ test/unit/agent-actions.test.ts | 14 +++ test/unit/focus-manifest.test.ts | 11 ++ test/unit/predicted-gate.test.ts | 33 ++++++ test/unit/queue.test.ts | 174 +++++++++++++++++++++++++++++++ test/unit/test-evidence.test.ts | 14 +++ 8 files changed, 266 insertions(+), 6 deletions(-) diff --git a/src/rules/predicted-gate.ts b/src/rules/predicted-gate.ts index da3a87b1ca..b2b5630356 100644 --- a/src/rules/predicted-gate.ts +++ b/src/rules/predicted-gate.ts @@ -20,7 +20,7 @@ const OSS_ANTI_SLOP_FUNNEL = { registerUrl: GITTENSOR_HOME_URL, } as const; import { buildPullRequestAdvisory, evaluateGateCheck, type GateCheckConclusion } from "./advisory"; -import { isTestPath } from "../signals/test-evidence"; +import { hasValidationNote, isTestPath } from "../signals/test-evidence"; import { evaluateClaCheck } from "../review/cla-check"; import { evaluatePreMergeChecks } from "../review/pre-merge-checks"; @@ -236,7 +236,10 @@ export function buildPredictedGateVerdict(args: { labels: syntheticPr.labels, linkedIssueCount: syntheticPr.linkedIssues.length, testFileCount: changedPaths.filter((path) => isTestPath(path)).length, - passedValidationCount: 0, + // Parity with the live gate (queue/processors.ts's manifestPolicyGateMode block): the predictor + // already has the same PR body available via input.body, so a manifest_missing_tests prediction must + // not stay stuck at "no validation evidence" when the real gate would already treat the body as evidence. + passedValidationCount: hasValidationNote(input.body ?? "") ? 1 : 0, }); const policyCodes = new Set(["manifest_linked_issue_required", "manifest_missing_tests"]); for (const finding of guidance.findings) { diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 03deea9f37..9eeb0c05e2 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -627,7 +627,10 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // never auto-merge, auto-approve, or auto-close a PR whose diff we don't know. Repos with no guardrails // configured stay permissive. const guardrailHit = isGuardrailHit(input.changedPaths, input.hardGuardrailGlobs); - const guardrailReason = guardrailHit ? guardrailHoldReason(input.changedPaths, input.hardGuardrailGlobs) : null; + // Every read site below is itself gated on guardrailHit being true, so this default is never actually + // read -- it exists only so guardrailReason stays a plain string instead of forcing a `?? fallback` at + // every call site (each of which would be an untestable, permanently-unreachable branch). + const guardrailReason = guardrailHit ? guardrailHoldReason(input.changedPaths, input.hardGuardrailGlobs) : "guarded path -> manual review"; // Manual review is the RARE exception (the operator's minimize-manual goal): the ONLY things that hold a PR // for a human instead of merge/close are an auto-merge-ready PR that touches a hard-guardrail path, or a // live migration-number collision detected against the CURRENT tip of the base branch (#2550 — a sibling PR @@ -710,7 +713,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne actionClass: "label", autonomyClass: "merge", requiresApproval: false, - reason: `verdict=${conclusion}; ${guardrailReason ?? "guarded path -> manual review"}`, + reason: `verdict=${conclusion}; ${guardrailReason}`, label: labels.manualReview, labelOp: "add", }); @@ -735,7 +738,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne : input.migrationCollisionHold !== undefined ? `verdict=${conclusion}; ${input.migrationCollisionHold.reason}` : heldForManualReview - ? `verdict=${conclusion}; ${guardrailReason ?? "guarded path -> manual review"}` + ? `verdict=${conclusion}; ${guardrailReason}` : `verdict=${conclusion}; CI green`; if (label !== null && !hasLabelOrPlanned(input.pr.labels, actions, label)) { actions.push({ @@ -921,7 +924,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // review-good-but-not-yet-mergeable → held briefly (rebase/approve resolves it next pass). const manualHoldReason = guardrailHit - ? `verdict=${conclusion}; ${guardrailReason ?? "guarded path -> manual review"}` + ? `verdict=${conclusion}; ${guardrailReason}` : ciUnverified ? "CI could not be verified" : conclusion === "action_required" diff --git a/src/signals/test-evidence.ts b/src/signals/test-evidence.ts index edfa515abc..7e93399c83 100644 --- a/src/signals/test-evidence.ts +++ b/src/signals/test-evidence.ts @@ -20,7 +20,15 @@ export function hasLocalTestEvidence(input: { tests?: string[] | undefined; test return (input.tests ?? []).length > 0 || (input.testFiles ?? []).some((file) => isTestPath(file)); } +// A body can mention testing without having actually done it ("No tests run", "Tests not run", "did not +// run tests") -- the affirmative keyword match below would otherwise treat that as passing evidence and +// let a configured manifest test expectation silently disappear. Reject both negation-before-noun and +// noun-before-negation orderings before falling through to the affirmative match. +const NEGATES_BEFORE_TEST_NOUN = /\b(?:no|not|without|skip(?:ped)?|did not|haven't|have not|never)\s+(?:run\s+|passing\s+|passed\s+)?(?:tests?|validation|manual check|smoke(?: tests?)?)\b/i; +const NEGATES_AFTER_TEST_NOUN = /\b(?:tests?|validation|manual check|smoke(?: tests?)?)\s+(?:not\s+run|not\s+passed|not\s+passing|not\s+included|failed|failing|skipped|were\s+not\s+run|was\s+not\s+run)\b/i; + export function hasValidationNote(value: string): boolean { + if (NEGATES_BEFORE_TEST_NOUN.test(value) || NEGATES_AFTER_TEST_NOUN.test(value)) return false; return /\b(test(?:ed|s|ing)?|validation|validated|verified|manual check|smoke|pytest|vitest|npm test|pnpm test|cargo test|go test)\b/i.test(value); } diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 97fb58972b..9b2e318369 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -391,6 +391,20 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(classes(plan)).not.toContain("merge"); }); + it("truncates the guardrail hold reason to 3 visible paths and counts the rest (#3304)", () => { + const label = planAgentMaintenanceActions(input({ + conclusion: "success", + autonomy: { merge: "auto" }, + changedPaths: ["src/scoring/a.ts", "src/scoring/b.ts", "src/scoring/c.ts", "src/scoring/d.ts", "src/scoring/e.ts"], + hardGuardrailGlobs: ["src/scoring/**"], + pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" }, + })).find((a) => a.actionClass === "label"); + expect(label?.reason).toContain("src/scoring/a.ts"); + expect(label?.reason).toContain("src/scoring/c.ts"); + expect(label?.reason).not.toContain("src/scoring/d.ts"); + expect(label?.reason).toContain("and 2 more"); + }); + it("explains when guardrail hold is fail-closed because changed files are unavailable", () => { const label = planAgentMaintenanceActions(input({ conclusion: "success", diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 91b1d67455..6fb15a5dd9 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -348,6 +348,17 @@ describe("buildFocusManifestGuidance", () => { expect(missingTests?.detail).toContain("unit tests for new branches."); }); + it("omits the 'Expected evidence' detail when every test expectation is public-unsafe (#3304)", () => { + // testExpectations.length > 0 still trips the finding, but the public-safe filter drops the only entry, + // so the detail must fall back to the base sentence with no "Expected evidence: ..." suffix appended. + const unsafeManifest = parseFocusManifest({ testExpectations: ["Submit your wallet seed phrase"] }); + const guidance = buildFocusManifestGuidance({ manifest: unsafeManifest, changedPaths: ["src/x.ts"], linkedIssueCount: 1, testFileCount: 0, passedValidationCount: 0 }); + const missingTests = guidance.findings.find((finding) => finding.code === "manifest_missing_tests"); + expect(missingTests?.detail).toBe("No changed test files or passing validation evidence were detected for this PR."); + expect(missingTests?.detail).not.toContain("Expected evidence"); + expect(missingTests?.detail).not.toContain("wallet"); + }); + it("treats passing validation as satisfying test expectations", () => { const guidance = buildFocusManifestGuidance({ manifest: wanted, changedPaths: ["src/x.ts"], linkedIssueCount: 1, testFileCount: 0, passedValidationCount: 2 }); expect(guidance.findings.some((finding) => finding.code === "manifest_missing_tests")).toBe(false); diff --git a/test/unit/predicted-gate.test.ts b/test/unit/predicted-gate.test.ts index d58528d9b1..e3c8cf79ba 100644 --- a/test/unit/predicted-gate.test.ts +++ b/test/unit/predicted-gate.test.ts @@ -371,6 +371,39 @@ describe("buildPredictedGateVerdict", () => { expect(result.warnings.some((w) => w.code === "manifest_blocked_path")).toBe(false); }); + it("predicts manifest_missing_tests when testExpectations are configured and neither test files nor a validation note are present (#3304)", () => { + const result = verdict({ + gate: { manifestPolicy: "block" }, + manifestExtra: { testExpectations: ["Run npm run test:ci."] }, + changedPaths: ["src/feature.ts"], + input: { body: "Closes #7" }, + }); + expect(result.conclusion).toBe("failure"); + expect(result.blockers.some((b) => b.code === "manifest_missing_tests")).toBe(true); + }); + + it("REGRESSION (#3304): does NOT predict manifest_missing_tests when the PR body includes validation evidence (parity with the live gate)", () => { + const result = verdict({ + gate: { manifestPolicy: "block" }, + manifestExtra: { testExpectations: ["Run npm run test:ci."] }, + changedPaths: ["src/feature.ts"], + input: { body: "Closes #7\n\nValidated with npm run test:ci." }, + }); + expect(result.conclusion).not.toBe("failure"); + expect(result.blockers.some((b) => b.code === "manifest_missing_tests")).toBe(false); + }); + + it("still predicts manifest_missing_tests when the PR body only claims NOT to have tested (negative evidence) (#3304)", () => { + const result = verdict({ + gate: { manifestPolicy: "block" }, + manifestExtra: { testExpectations: ["Run npm run test:ci."] }, + changedPaths: ["src/feature.ts"], + input: { body: "Closes #7\n\nNo tests run." }, + }); + expect(result.conclusion).toBe("failure"); + expect(result.blockers.some((b) => b.code === "manifest_missing_tests")).toBe(true); + }); + it("ignores non-policy guidance findings (e.g. off-focus) — only enforceable policy codes are threaded (#12)", () => { // The path isn't blocked but it's outside the wanted areas → guidance emits the NON-policy `manifest_off_focus`. // The predictor must skip it (only linked-issue-required / missing-tests are gateable). diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 0bac68d48f..63f3193ec2 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6352,6 +6352,180 @@ describe("queue processors", () => { expect(JSON.stringify(gatePatches[0])).not.toContain("manifest_missing_tests"); }); + // REGRESSION (#3304): a PR body that merely MENTIONS testing without affirming it was done ("No tests + // run.") must not satisfy a configured manifest test expectation on the live webhook gate path. + it("still flags manifest_missing_tests for a PR body that only claims tests were NOT run", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertInstallation(env, { + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "write", issues: "write" }, + events: ["pull_request"], + }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "off", + manifestPolicyGateMode: "block", + requireLinkedIssue: false, + typeLabelsEnabled: false, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { testExpectations: ["Run npm run test:ci."] }); + await upsertPullRequestFile(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 44, + path: "src/feature.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }); + + const gatePatches: Array> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/commits/gate-no-validation/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 902 }, { status: 201 }); + if (url.includes("/check-runs/902") && method === "PATCH") { + gatePatches.push(JSON.parse(String(init?.body ?? "{}")) as Record); + return Response.json({ id: 902, html_url: "https://github.com/checks/902" }); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "gate-no-validation-evidence", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 44, + title: "Unvalidated change", + state: "open", + user: { login: "contributor" }, + head: { sha: "gate-no-validation" }, + labels: [], + body: "No tests run.", + }, + }, + }); + + expect(gatePatches).toHaveLength(1); + expect(gatePatches[0]).toMatchObject({ status: "completed", conclusion: "failure" }); + expect(JSON.stringify(gatePatches[0])).toContain("Configured validation evidence missing"); + }); + + // REGRESSION (#3304): a PR with no body at all (GitHub sends `body: null` for an empty description) must + // fall back to treating validation evidence as absent, not throw or silently pass the manifest gate. + it("still flags manifest_missing_tests for a PR with a null body", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertInstallation(env, { + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "write", issues: "write" }, + events: ["pull_request"], + }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "off", + manifestPolicyGateMode: "block", + requireLinkedIssue: false, + typeLabelsEnabled: false, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { testExpectations: ["Run npm run test:ci."] }); + await upsertPullRequestFile(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 45, + path: "src/feature.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }); + + const gatePatches: Array> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/commits/gate-null-body/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 903 }, { status: 201 }); + if (url.includes("/check-runs/903") && method === "PATCH") { + gatePatches.push(JSON.parse(String(init?.body ?? "{}")) as Record); + return Response.json({ id: 903, html_url: "https://github.com/checks/903" }); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "gate-null-body-evidence", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 45, + title: "No-description change", + state: "open", + user: { login: "contributor" }, + head: { sha: "gate-null-body" }, + labels: [], + body: null, + }, + }, + }); + + expect(gatePatches).toHaveLength(1); + expect(gatePatches[0]).toMatchObject({ status: "completed", conclusion: "failure" }); + expect(JSON.stringify(gatePatches[0])).toContain("Configured validation evidence missing"); + }); + it("stamps a gate-only surface even when local Gate check-summary persistence fails", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( diff --git a/test/unit/test-evidence.test.ts b/test/unit/test-evidence.test.ts index 41c3f9aa78..65cf46bb1e 100644 --- a/test/unit/test-evidence.test.ts +++ b/test/unit/test-evidence.test.ts @@ -81,6 +81,20 @@ describe("test evidence helpers", () => { expect(hasValidationNote("Validated with npm run test:ci and a smoke run.")).toBe(true); expect(hasValidationNote("Manual check passed for the dashboard.")).toBe(true); expect(hasValidationNote("Refactors the route naming only.")).toBe(false); + expect(hasValidationNote("Adds retry logic to the fetch helper. Tested with npm run test:ci — all 142 tests pass.")).toBe(true); + }); + + // REGRESSION (#3304): a body that merely MENTIONS testing without affirming it was actually done must not + // satisfy a configured manifest test expectation. Covers both negation-before-noun and noun-before-negation + // word orders, since only guarding one direction still let the other slip through. + it("rejects PR-body text that mentions tests/validation without affirming they passed", () => { + expect(hasValidationNote("No tests run.")).toBe(false); + expect(hasValidationNote("Tests not run.")).toBe(false); + expect(hasValidationNote("I did not run tests for this change.")).toBe(false); + expect(hasValidationNote("Tests were not run due to a broken CI runner.")).toBe(false); + expect(hasValidationNote("Skipped tests for this one.")).toBe(false); + expect(hasValidationNote("Tests failed locally but I'm opening this anyway.")).toBe(false); + expect(hasValidationNote("No validation was performed.")).toBe(false); }); }); From d0896bac8f4cf73744abdb1165a1ae2fc2755c47 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 21:43:32 -0700 Subject: [PATCH 3/4] fix(review): detect negated test evidence by proximity, not literal phrases The first fix enumerated literal negation phrases keyed on the noun forms "test"/"tests", which missed "Not tested locally." -- the verb form "tested" only matched the affirmative branch, so a body stating tests were NOT run could still satisfy a configured manifest test expectation. Redesign hasValidationNote around a shared test/validation stem definition and a word-proximity negation check (a negation word within a bounded window of the stem, in either order) instead of enumerating more literal templates, so the same class of miss cannot recur for a different tense/form. The window is clause-bounded (stops at ,.!?;) so an unrelated negation earlier in the body cannot suppress a later, real validation note. --- src/signals/test-evidence.ts | 30 +++++++++++++++++++++++------- test/unit/test-evidence.test.ts | 21 +++++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/signals/test-evidence.ts b/src/signals/test-evidence.ts index 7e93399c83..0e6229abd0 100644 --- a/src/signals/test-evidence.ts +++ b/src/signals/test-evidence.ts @@ -20,15 +20,31 @@ export function hasLocalTestEvidence(input: { tests?: string[] | undefined; test return (input.tests ?? []).length > 0 || (input.testFiles ?? []).some((file) => isTestPath(file)); } -// A body can mention testing without having actually done it ("No tests run", "Tests not run", "did not -// run tests") -- the affirmative keyword match below would otherwise treat that as passing evidence and -// let a configured manifest test expectation silently disappear. Reject both negation-before-noun and -// noun-before-negation orderings before falling through to the affirmative match. -const NEGATES_BEFORE_TEST_NOUN = /\b(?:no|not|without|skip(?:ped)?|did not|haven't|have not|never)\s+(?:run\s+|passing\s+|passed\s+)?(?:tests?|validation|manual check|smoke(?: tests?)?)\b/i; -const NEGATES_AFTER_TEST_NOUN = /\b(?:tests?|validation|manual check|smoke(?: tests?)?)\s+(?:not\s+run|not\s+passed|not\s+passing|not\s+included|failed|failing|skipped|were\s+not\s+run|was\s+not\s+run)\b/i; +// A body can mention testing without having actually done it ("No tests run", "Tests not run", "Not +// tested locally", "did not run any tests") -- the affirmative keyword match below would otherwise treat +// that as passing evidence and let a configured manifest test expectation silently disappear. Rather than +// enumerate ever more literal phrase templates (which a previous version of this function tried, and which +// still missed "Not tested" because its test-noun list didn't include the verb form "tested"), detect +// negation by PROXIMITY: a negation word within a few words of a test/validation stem, in either order, +// with a shared stem definition so the "is this a test/validation mention at all" question is answered +// exactly once. The filler between the negation word and the stem may not cross a clause/sentence boundary +// (a comma/period/exclamation/question mark/semicolon), so an unrelated "not" earlier in the body (e.g. +// "This is not a breaking change. Tested with npm run test:ci.") cannot suppress a later, unrelated +// affirmative note. +const TEST_STEM = "(?:test(?:ed|s|ing)?|validat(?:ion|ed)|verif(?:y|ied|ying)|manual check|smoke(?:\\s+tests?)?)"; +const NEGATION_WORD = "(?:no|not|never|without|skip(?:ped)?|didn't|doesn't|isn't|wasn't|weren't|haven't|hasn't)"; +const NEGATION_CONTINUATION = "(?:not|never|failed|failing|skipped|incomplete)"; +const SAME_SENTENCE_FILLER_WORD = "[^\\s.,!?;]+"; + +const NEGATES_BEFORE_TEST_STEM = new RegExp(`\\b${NEGATION_WORD}\\b(?:\\s+${SAME_SENTENCE_FILLER_WORD}){0,3}\\s+${TEST_STEM}\\b`, "i"); +const NEGATES_AFTER_TEST_STEM = new RegExp(`\\b${TEST_STEM}\\b(?:\\s+${SAME_SENTENCE_FILLER_WORD}){0,2}\\s+${NEGATION_CONTINUATION}\\b`, "i"); +// A compound negated adjective with no separating whitespace at all ("untested", "unvalidated", "unverified"). +const NEGATES_TEST_STEM_PREFIX = /\bun(?:tested|validated|verified)\b/i; export function hasValidationNote(value: string): boolean { - if (NEGATES_BEFORE_TEST_NOUN.test(value) || NEGATES_AFTER_TEST_NOUN.test(value)) return false; + if (NEGATES_TEST_STEM_PREFIX.test(value) || NEGATES_BEFORE_TEST_STEM.test(value) || NEGATES_AFTER_TEST_STEM.test(value)) { + return false; + } return /\b(test(?:ed|s|ing)?|validation|validated|verified|manual check|smoke|pytest|vitest|npm test|pnpm test|cargo test|go test)\b/i.test(value); } diff --git a/test/unit/test-evidence.test.ts b/test/unit/test-evidence.test.ts index 65cf46bb1e..6f84153b79 100644 --- a/test/unit/test-evidence.test.ts +++ b/test/unit/test-evidence.test.ts @@ -96,6 +96,27 @@ describe("test evidence helpers", () => { expect(hasValidationNote("Tests failed locally but I'm opening this anyway.")).toBe(false); expect(hasValidationNote("No validation was performed.")).toBe(false); }); + + // REGRESSION (#3304, round 2): the first negation fix used a literal `tests?` noun, which missed the past- + // tense verb form "tested" -- "Not tested locally." slipped through as affirmative evidence because only + // the (unrelated) positive "tested" keyword matched. The proximity-based redesign shares one stem + // definition between the negation and affirmative checks, so this class of miss cannot recur for any + // stem/tense combination. + it("rejects negated verb forms the noun-only check missed, including compounds and interposed words", () => { + expect(hasValidationNote("Not tested locally.")).toBe(false); + expect(hasValidationNote("Untested change, opening as a draft-adjacent PR.")).toBe(false); + expect(hasValidationNote("Unvalidated — please review carefully.")).toBe(false); + expect(hasValidationNote("Testing skipped for this draft.")).toBe(false); + expect(hasValidationNote("Have not run any tests yet.")).toBe(false); + }); + + // REGRESSION (#3304, round 2): a negation word elsewhere in the body must not suppress an unrelated, + // later affirmative note -- the proximity check is bounded to the same sentence specifically so this + // cannot happen (a prior naive "any negation word anywhere" design would wrongly return false here). + it("does not let an unrelated negation elsewhere in the body suppress a real validation note", () => { + expect(hasValidationNote("This is not a breaking change. Tested with npm run test:ci.")).toBe(true); + expect(hasValidationNote("Not a big deal, tested with npm test.")).toBe(true); + }); }); describe("classifyTestCoverage", () => { From abed0572993e01b837dccc699c0e34b3587e24a9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 22:06:17 -0700 Subject: [PATCH 4/4] fix(review): judge test-evidence clauses independently, not the whole body hasValidationNote ran its negation checks against the entire PR body and returned false on the first match, so a genuine negated clause ("No tests run locally.") vetoed a separate, later clause with real affirmative evidence ("Validated with npm run test:ci."). Split on clause-boundary punctuation and require only one clause to be an affirmative, non-negated mention. --- src/signals/test-evidence.ts | 21 +++++++++++++++++---- test/unit/test-evidence.test.ts | 14 ++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/signals/test-evidence.ts b/src/signals/test-evidence.ts index 0e6229abd0..6b74eda343 100644 --- a/src/signals/test-evidence.ts +++ b/src/signals/test-evidence.ts @@ -41,11 +41,24 @@ const NEGATES_AFTER_TEST_STEM = new RegExp(`\\b${TEST_STEM}\\b(?:\\s+${SAME_SENT // A compound negated adjective with no separating whitespace at all ("untested", "unvalidated", "unverified"). const NEGATES_TEST_STEM_PREFIX = /\bun(?:tested|validated|verified)\b/i; +const AFFIRMATIVE_TEST_MENTION = /\b(test(?:ed|s|ing)?|validation|validated|verified|manual check|smoke|pytest|vitest|npm test|pnpm test|cargo test|go test)\b/i; + +// A body can contain BOTH a genuine negated clause ("No tests run locally.") and a separate, later clause +// with real affirmative evidence ("Validated with npm run test:ci.") -- evaluating the negation checks +// against the WHOLE body would let the first clause veto the second, discarding real evidence the manifest +// gate is specifically trying to detect (#3304, round 3). Split on the same clause-boundary punctuation the +// proximity checks already treat as a hard stop, and require at least one clause to be an affirmative, +// non-negated mention -- so an earlier honest "no tests" disclosure can no longer suppress later evidence. export function hasValidationNote(value: string): boolean { - if (NEGATES_TEST_STEM_PREFIX.test(value) || NEGATES_BEFORE_TEST_STEM.test(value) || NEGATES_AFTER_TEST_STEM.test(value)) { - return false; - } - return /\b(test(?:ed|s|ing)?|validation|validated|verified|manual check|smoke|pytest|vitest|npm test|pnpm test|cargo test|go test)\b/i.test(value); + return value + .split(/[.,!?;]+/) + .some( + (clause) => + !NEGATES_TEST_STEM_PREFIX.test(clause) && + !NEGATES_BEFORE_TEST_STEM.test(clause) && + !NEGATES_AFTER_TEST_STEM.test(clause) && + AFFIRMATIVE_TEST_MENTION.test(clause), + ); } /** diff --git a/test/unit/test-evidence.test.ts b/test/unit/test-evidence.test.ts index 6f84153b79..6cfaac3259 100644 --- a/test/unit/test-evidence.test.ts +++ b/test/unit/test-evidence.test.ts @@ -117,6 +117,20 @@ describe("test evidence helpers", () => { expect(hasValidationNote("This is not a breaking change. Tested with npm run test:ci.")).toBe(true); expect(hasValidationNote("Not a big deal, tested with npm test.")).toBe(true); }); + + // REGRESSION (#3304, round 3): the negation checks previously ran against the WHOLE body, so a genuine + // negated clause ("No tests run locally.") vetoed the whole result even when a separate, later clause + // provided real affirmative evidence -- discarding evidence the manifest gate is specifically trying to + // detect. Each clause must now be judged independently. + it("does not let an earlier genuine test-negation suppress later real affirmative evidence", () => { + expect(hasValidationNote("No tests run locally. Validated with npm run test:ci.")).toBe(true); + expect(hasValidationNote("Not tested on staging, but ran the full suite locally with npm test.")).toBe(true); + expect(hasValidationNote("Skipped tests for the docs change. Verified the build output manually.")).toBe(true); + }); + + it("still rejects a body whose only test/validation mentions are all negated across clauses", () => { + expect(hasValidationNote("No tests run. Not validated. Untested change.")).toBe(false); + }); }); describe("classifyTestCoverage", () => {