From 74c54a649fb8cf858269a6908208b01013ec87d2 Mon Sep 17 00:00:00 2001 From: Peter Kieltyka Date: Thu, 24 Sep 2026 15:07:58 -0400 Subject: [PATCH] fix(github): deduplicate unchanged prose comments across shifted anchors Address the repeated documentation threads reported in PR #28 at publication rather than assigning every finding in a document the same location identity. Composer grouping and existing code fingerprints retain their current semantics, so unrelated same-category findings and executable examples under docs/ remain separate. For Markdown, MDX, reStructuredText and text files, match the complete sanitized comment body on the same path and diff side. Store its content fingerprint before the inline size cap and read existing comments without that marker by their body. Changed wording, missing content and ambiguous truncated legacy comments remain eligible for posting instead of suppressing a potentially different issue. Use the validated publication anchor during duplicate detection, including anchors supplied only by the posting plan. Require a known anchor for prose matching. Adapt both PR #28 regression fixtures: preserve unchanged comment identity across a seven-line shift while explicitly keeping distinct documentation findings apart. Add coverage for code under docs/, existing markers, missing anchors, proximity, changed content and full-body identity beyond the display truncation boundary. Introduce evals/synthetics and make evals with 13 deterministic scenarios covering real diff parsing, publication, persisted markers, GitHub response parsing and duplicate detection through a scripted transport. Include these scenarios in the normal test suite and TypeScript checks; document their scope and conservative wording policy. Validation: pnpm test (1,390 tests across 63 files), make evals (13 scenarios), pnpm run check, pnpm build and git diff --check all passed. --- Makefile | 6 +- README.md | 4 + evals/synthetics/README.md | 22 +++ .../synthetics/docs-comment-identity.test.ts | 98 +++++++++++ src/github/duplicate-detector.ts | 34 +++- src/github/github-client.ts | 2 + src/github/publisher.ts | 11 +- src/types.ts | 5 +- src/util/path-roles.ts | 5 + tests/github-client.test.ts | 3 +- tests/github-duplicate-detector.test.ts | 51 +++++- tests/pipeline-phase5.test.ts | 166 +++++++++++++++++- tsconfig.json | 2 +- vitest.config.ts | 2 +- 14 files changed, 393 insertions(+), 18 deletions(-) create mode 100644 evals/synthetics/README.md create mode 100644 evals/synthetics/docs-comment-identity.test.ts diff --git a/Makefile b/Makefile index d61da1e..b274025 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: init build run install uninstall check check-workflows typecheck test clean help models-list +.PHONY: init build run install uninstall check check-workflows typecheck test evals clean help models-list # Default target help: @@ -13,6 +13,7 @@ help: @echo " make check-workflows Validate workflows with actionlint" @echo " make typecheck Run TypeScript type checking" @echo " make test Run tests" + @echo " make evals Run deterministic synthetic harness evals (no inference or network)" @echo " make models-list Regenerate models.md from the model registry" @echo " make clean Remove dist/" @echo "" @@ -60,6 +61,9 @@ check: test: pnpm test +evals: + pnpm exec vitest run evals/synthetics + models-list: build pnpm run models-list diff --git a/README.md b/README.md index 23c3ff6..4ad52b9 100644 --- a/README.md +++ b/README.md @@ -76,6 +76,8 @@ codegenie review --pr 123 --post-github-comments # publish inline comments (ex Posting to GitHub is a single `COMMENT`-type review with inline comments anchored to changed lines — it never approves or requests changes, and only happens when you pass the flag. Interactive runs show a stderr progress spinner (auto-disabled in CI; `--no-progress` disables it explicitly). Non-posting Markdown/JSON runs emit the full report to stdout; posting runs emit a concise posting summary instead. Action mode separately renders the full report into the status comment, step summary, and report artifact. +For prose files (`.md`, `.mdx`, `.rst`, `.txt`), unchanged inline comment content is deduplicated across shifted anchors on the same file and diff side. Matching uses the complete sanitized body before truncation; changed wording remains eligible for posting. Executable examples under `docs/` retain code fingerprint matching. Separate findings are not merged merely because they occur in the same document. + ## GitHub Action codegenie ships as a reusable GitHub Action: reviews run automatically on PR open/update, or on demand when a collaborator comments `codegenie review` on a PR. The run posts a single status comment ("Reviewing ...") that live-updates through the pipeline stages and finishes as the full markdown report; inline finding comments post as a PR review alongside it (on by default, `post-inline-comments: "false"` disables). @@ -280,6 +282,8 @@ Verification assesses proposed fixes and tests independently from the defect, us Run `pnpm run check` for TypeScript and GitHub workflow validation, then `pnpm test` and `pnpm build` for the full suite. Workflow validation uses [`actionlint`](https://github.com/rhysd/actionlint); `pnpm test` runs it automatically so GitHub expression/context errors cannot pass while unit tests remain green. +Run `make evals` for the deterministic [synthetic harness evals](evals/synthetics/README.md). These scenarios exercise harness behavior without network requests or model inference, and also run in `pnpm test`. + ## Status codegenie is a pre-1.0 CLI being hardened through live evals. Full specifications live in [`specs/project/`](specs/project/): diff --git a/evals/synthetics/README.md b/evals/synthetics/README.md new file mode 100644 index 0000000..693ef70 --- /dev/null +++ b/evals/synthetics/README.md @@ -0,0 +1,22 @@ +# Synthetic harness evals + +Run `make evals` from the repository root. These deterministic scenarios use real +harness components with scripted model/GitHub boundaries: no credentials, network +requests, or paid inference. They also run under `pnpm test`. + +The documentation comment suite simulates successive pushes through diff parsing, +publication, persisted comment markers, GitHub response parsing, and duplicate +matching. It checks shifted lines, changed anchor text, legacy comments, unrelated +issues on nearby and distant lines, different files, and capped comment bodies. +It also checks anchors supplied by the posting plan and makes the conservative +wording policy explicit: paraphrased findings remain eligible for posting. + +The original PR #28 cases also live in `tests/pipeline-phase5.test.ts`: an unchanged +finding survives a seven-line shift at publication, while distinct same-category +findings in a document remain separate. The latter deliberately reverses PR #28's +file-wide merging tradeoff. Existing code fingerprint tests, including executable +examples under `docs/`, retain symbol-based identity. + +These evaluate harness behavior, not whether a model identifies or phrases a +finding consistently. Use `codegenie eval --eval-dir ...` for live model comparisons. +Add additional `*.test.ts` scenarios here as harness failure patterns are found. diff --git a/evals/synthetics/docs-comment-identity.test.ts b/evals/synthetics/docs-comment-identity.test.ts new file mode 100644 index 0000000..10034b6 --- /dev/null +++ b/evals/synthetics/docs-comment-identity.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from "vitest"; +import { defaultConfig } from "../../src/config/schema.js"; +import { parseDiff } from "../../src/git/diff-parser.js"; +import { createGitHubClient } from "../../src/github/github-client.js"; +import { parseCodegenieMarker } from "../../src/github/duplicate-detector.js"; +import { maybePublishToGitHub } from "../../src/github/publisher.js"; +import { sha256Hex } from "../../src/util/hashing.js"; +import type { FinalFinding, InlineCommentInput, ResolvedReviewInput, ReviewResult } from "../../src/types.js"; +import type { runGh } from "../../src/git/subprocess.js"; +import { nullTelemetry } from "../../tests/helpers/git.js"; + +const enumAdvice = "The table lists `widgetCreated`, but `EventKind` does not define it. Use a supported event value."; +const retryAdvice = "These retry instructions can submit the same order twice. Require an idempotency key before retrying."; + +// Exercise the real diff parser, publisher, marker serialization, GitHub client, +// and duplicate detector across pushes. Only the GitHub transport is scripted. +describe("synthetic: documentation comment identity across pushes", () => { + it.each([ + { name: "unchanged finding after a seven-line shift", first: enumAdvice, second: enumAdvice, line: 74, expected: 0 }, + { name: "same finding on a table row instead of its prose bullet", first: enumAdvice, second: enumAdvice, line: 40, expected: 0 }, + { name: "unchanged finding using its validated posting-plan anchor", first: enumAdvice, second: enumAdvice, line: 74, expected: 0, plannedAnchor: true }, + { name: "unrelated finding far away in the same document", first: enumAdvice, second: retryAdvice, line: 180, expected: 1 }, + { name: "unrelated finding within the old five-line fuzzy window", first: enumAdvice, second: retryAdvice, line: 82, expected: 1 }, + { name: "changed advice at the same location", first: enumAdvice, second: retryAdvice, line: 81, expected: 1 }, + { name: "paraphrased finding stays eligible without a proven content match", first: enumAdvice, second: "EventKind has no widgetCreated member; correct the documented event value.", line: 74, expected: 1 }, + { name: "legacy marker and an unchanged body", first: enumAdvice, second: enumAdvice, line: 74, expected: 0, legacy: true }, + { name: "legacy sanitized body with a mention", first: `${enumAdvice} Ask @maintainer.`, second: `${enumAdvice} Ask @maintainer.`, line: 74, expected: 0, legacy: true }, + { name: "same published text in a different file", first: enumAdvice, second: enumAdvice, line: 74, expected: 1, path: "docs/other.md" }, + { name: "unchanged long report despite the inline body cap", first: enumAdvice.repeat(130), second: enumAdvice.repeat(130), line: 74, expected: 0 }, + { name: "truncated legacy report cannot prove a full-content match", first: enumAdvice.repeat(130), second: enumAdvice.repeat(130), line: 74, expected: 1, legacy: true }, + { name: "different conclusions after an identical capped prefix", first: enumAdvice.repeat(130) + "\nFirst conclusion.", second: enumAdvice.repeat(130) + "\nDifferent conclusion.", line: 81, expected: 1 } + ])("$name", async scenario => { + const remote = scriptedGitHub(); + const first = snapshot("docs/api.md", 81, "- The event is `widgetCreated`.", scenario.first); + const firstResult = await maybePublishToGitHub(first.review, first.resolved, defaultConfig, nullTelemetry(), { github: remote.client }); + expect(firstResult?.inlinePosted).toBe(1); + expect(remote.comments).toHaveLength(1); + expect(parseCodegenieMarker(remote.comments[0]!.body)?.contentFingerprint).toMatch(/^[0-9a-f]{64}$/u); + if (scenario.legacy) remote.comments[0]!.body = remote.comments[0]!.body.replace(/;content=[0-9a-f]{64}/u, ""); + // GitHub returns original_line for an outdated anchor after another push. + remote.comments[0]!.line = null; + remote.advance(); + const second = snapshot(scenario.path ?? "docs/api.md", scenario.line, "| event | `widgetCreated` |", scenario.second, "2".repeat(40)); + if (scenario.plannedAnchor) delete second.review.findings[0]!.anchor; + const result = await maybePublishToGitHub(second.review, second.resolved, defaultConfig, nullTelemetry(), { github: remote.client }); + expect(result?.inlinePosted).toBe(scenario.expected); + expect(result?.skippedDuplicates).toBe(1 - scenario.expected); + expect(remote.comments).toHaveLength(1 + scenario.expected); + expect(result?.duplicateDecisions?.[0]?.action).toBe(scenario.expected ? "post" : "skip_unchanged_content"); + }); +}); + +function snapshot(path: string, line: number, changedText: string, body: string, headSha = "1".repeat(40)) { + const rawDiff = `diff --git a/${path} b/${path}\n--- a/${path}\n+++ b/${path}\n@@ -${line},1 +${line},1 @@\n-old text\n+${changedText}\n`; + const hunk = parseDiff(rawDiff).files[0]!.hunks[0]!; + const finding: FinalFinding = { + id: "f1", path, title: "Documentation contradicts the supported behavior", severity: "medium", confidence: "high", + category: "correctness", changedLine: true, anchor: { path, line, side: "RIGHT", hunkId: hunk.id }, + evidence: { changedCode: changedText }, failureMode: body, whyThisMatters: body, verification: "Checked against the source contract.", + producedBy: { kind: "packet", stage: 7, packetId: `packet-${hunk.id}`, lensId: "core/code-review", skillIds: [] }, + fingerprint: sha256Hex([path, hunk.id, "correctness", "core/code-review"].join("\0")), + finalBody: body, publication: "inline", mergedCandidateIds: ["f1"] + }; + const review: ReviewResult = { + summary: "One finding.", findings: [finding], summaryOnlyFindings: [], needsHumanAttention: [], noFindings: false, + coverage: { totalHunks: 1, reviewedHunks: 1, skippedHunks: 0, failedHunks: 0, + coverageByLevel: { deep: 0, normal: 1, light: 0, skip: 0 }, degradedPlanning: false, budgetStopped: false, + verificationIncompleteCount: 0, partial: false, reasons: [] }, + postingPlan: { inline: [{ findingId: finding.id, anchor: finding.anchor! }], reviewBody: "One finding." } + }; + const resolved: ResolvedReviewInput = { + mode: "github_pr", repoRoot: "/synthetic", commits: [], rawDiff, + pr: { owner: "fixture", repo: "fixture", number: 1, title: "Docs update", body: "", url: "https://example.invalid/pr/1", + baseRefName: "main", baseSha: "b".repeat(40), headRefName: "feature", headSha } + }; + return { review, resolved }; +} + +function scriptedGitHub() { + type Comment = { id: number; path: string; side: "RIGHT" | "LEFT"; line: number | null; original_line: number; body: string; user: { login: string } }; + const comments: Comment[] = []; + let headSha = "1".repeat(40); + const gh: typeof runGh = async (_cwd, args, opts) => { + if (args[0] === "--version" || args.join(" ") === "auth status") return ""; + if (args[0] === "repo") return JSON.stringify({ owner: { login: "fixture" }, name: "fixture" }); + if (args[0] === "pr") return JSON.stringify({ number: 1, baseRefOid: "b".repeat(40), headRefOid: headSha }); + if (args[1] === "user") return "codegenie-bot"; + if (args[1]?.includes("/comments?")) return JSON.stringify(comments); + if (args[1]?.endsWith("/reviews")) { + const payload = JSON.parse(String(opts?.input)) as { comments: InlineCommentInput[] }; + for (const input of payload.comments) comments.push({ id: comments.length + 1, path: input.path, side: input.side, + line: input.line, original_line: input.line, body: input.body, user: { login: "codegenie-bot" } }); + return "{}"; + } + throw new Error(`Unexpected GitHub transport request: ${args.join(" ")}`); + }; + return { comments, client: createGitHubClient("/synthetic", { runGh: gh }), advance: () => { headSha = "2".repeat(40); } }; +} diff --git a/src/github/duplicate-detector.ts b/src/github/duplicate-detector.ts index c6a6a11..2243b9d 100644 --- a/src/github/duplicate-detector.ts +++ b/src/github/duplicate-detector.ts @@ -3,13 +3,17 @@ import type { FinalFinding, FindingDuplicateDecision } from "../types.js"; +import { isProsePath } from "../util/path-roles.js"; +import { sha256Hex } from "../util/hashing.js"; +import { sanitizeGitHubCommentBody } from "./comment-sanitizer.js"; export const CODEGENIE_MARKER_PATTERN = - //u; + //u; export type CodegenieMarker = { fingerprint: string; runId: string; + contentFingerprint?: string; }; export function parseCodegenieMarker(body: string): CodegenieMarker | undefined { @@ -19,12 +23,19 @@ export function parseCodegenieMarker(body: string): CodegenieMarker | undefined } return { fingerprint: match[1] ?? "", - runId: match[2] ?? "" + runId: match[2] ?? "", + ...(match[3] ? { contentFingerprint: match[3] } : {}) }; } -export function formatCodegenieMarker(fingerprint: string, runId: string): string { - return ``; +export function formatCodegenieMarker(fingerprint: string, runId: string, contentFingerprint?: string): string { + return ``; +} + +/** Match unchanged published text, not model wording similarity or diff geometry. */ +export function proseContentFingerprint(body: string): string | undefined { + const content = sanitizeGitHubCommentBody(body).replace(/\r\n?/gu, "\n").trim(); + return content ? sha256Hex(content) : undefined; } export function detectDuplicateFindings( @@ -38,6 +49,21 @@ export function detectDuplicateFindings( .map((comment) => [comment.fingerprint as string, comment]) ); return findings.map((finding): FindingDuplicateDecision => { + if (isProsePath(finding.path)) { + const contentFingerprint = proseContentFingerprint(finding.finalBody); + const unchanged = finding.anchor !== undefined && contentFingerprint && codegenieComments.find(comment => + comment.path === finding.path && comment.side === finding.anchor?.side && + (comment.contentFingerprint ?? proseContentFingerprint(comment.body ?? "")) === contentFingerprint + ); + // Geometry-based fingerprints and proximity alone cannot distinguish prose defects. + // Legacy comments can match by their full body; missing or changed content stays eligible. + return unchanged ? { + findingId: finding.id, + action: "skip_unchanged_content", + matchedCommentId: unchanged.id, + reason: "unchanged codegenie comment content already exists on the same path and side" + } : { findingId: finding.id, action: "post", reason: "no unchanged codegenie prose comment found" }; + } const exact = fingerprints.get(finding.fingerprint); if (exact !== undefined) { return { diff --git a/src/github/github-client.ts b/src/github/github-client.ts index 6b72869..ed8f20f 100644 --- a/src/github/github-client.ts +++ b/src/github/github-client.ts @@ -217,6 +217,8 @@ export function createGitHubClient(repoRoot: string, opts: CreateGitHubClientOpt } if (marker !== undefined) { thread.fingerprint = marker.fingerprint; + if (marker.contentFingerprint !== undefined) thread.contentFingerprint = marker.contentFingerprint; + if (comment.body !== undefined) thread.body = comment.body; } own.push(thread); } diff --git a/src/github/publisher.ts b/src/github/publisher.ts index 3e7827f..6c35c05 100644 --- a/src/github/publisher.ts +++ b/src/github/publisher.ts @@ -17,7 +17,8 @@ import { inlineCode, severityBadge } from "../util/markdown.js"; import { codegenieVersionInfo, workflowRunUrl } from "../util/version-info.js"; import { sanitizeGitHubCommentBody } from "./comment-sanitizer.js"; import { createGitHubClient } from "./github-client.js"; -import { detectDuplicateFindings, formatCodegenieMarker } from "./duplicate-detector.js"; +import { detectDuplicateFindings, formatCodegenieMarker, proseContentFingerprint } from "./duplicate-detector.js"; +import { isProsePath } from "../util/path-roles.js"; type PublishOptions = { github?: GitHubClient; @@ -116,7 +117,9 @@ export async function maybePublishToGitHub( } const comments = await github.listOwnComments(resolved.pr.number); - const duplicateDecisions = detectDuplicateFindings(inlineCandidates.map((candidate) => candidate.finding), comments); + const duplicateDecisions = detectDuplicateFindings( + inlineCandidates.map(({ finding, anchor }) => ({ ...finding, anchor })), comments + ); const duplicateById = new Map(duplicateDecisions.map((decision) => [decision.findingId, decision])); const prepared = inlineCandidates .filter(({ finding }) => duplicateById.get(finding.id)?.action === "post") @@ -280,11 +283,13 @@ function prepareInlineComment( runId: string, deletedFileAnchor: boolean ): PreparedInlineComment { + const contentFingerprint = isProsePath(finding.path) ? proseContentFingerprint(finding.finalBody) : undefined; + const marker = formatCodegenieMarker(finding.fingerprint, runId, contentFingerprint); const input: InlineCommentInput = { path: anchor.path, line: anchor.line, side: anchor.side, - body: `${capBody(sanitizeGitHubCommentBody(finding.finalBody), INLINE_BODY_CAP)}\n\n${formatCodegenieMarker(finding.fingerprint, runId)}` + body: `${capBody(sanitizeGitHubCommentBody(finding.finalBody), INLINE_BODY_CAP)}\n\n${marker}` }; if (anchor.startLine !== undefined && anchor.startLine !== anchor.line) { input.start_line = anchor.startLine; diff --git a/src/types.ts b/src/types.ts index 5a73502..69da0d9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -171,6 +171,9 @@ export type ExistingReviewThread = { author: string; isCodegenie: boolean; fingerprint?: string; + /** Stable identity of the complete published prose, before the inline size cap. */ + contentFingerprint?: string; + body?: string; }; export interface GitHubClient { @@ -1553,7 +1556,7 @@ export type EvalCompareReport = { export type FindingDuplicateDecision = { findingId: string; - action: "post" | "skip_exact_fingerprint" | "skip_fuzzy_proximity"; + action: "post" | "skip_exact_fingerprint" | "skip_fuzzy_proximity" | "skip_unchanged_content"; matchedCommentId?: string; reason: string; }; diff --git a/src/util/path-roles.ts b/src/util/path-roles.ts index 9813619..19727ef 100644 --- a/src/util/path-roles.ts +++ b/src/util/path-roles.ts @@ -45,6 +45,11 @@ export function isDocsPath(filePath: string): boolean { return /(?:^|\/)(?:docs?|documentation|postmortems?)(?:\/|$)|\.(?:md|mdx|rst|txt)$/u.test(normalized); } +/** Prose extensions, not directory roles: executable examples under docs/ remain code. */ +export function isProsePath(filePath: string): boolean { + return /\.(?:md|mdx|rst|txt)$/u.test(normalizePathRoleInput(filePath)); +} + function normalizePathRoleInput(filePath: string): string { return filePath.toLowerCase().replace(/\\/gu, "/"); } diff --git a/tests/github-client.test.ts b/tests/github-client.test.ts index b13805b..2d376cc 100644 --- a/tests/github-client.test.ts +++ b/tests/github-client.test.ts @@ -91,7 +91,8 @@ describe("GitHub client", () => { side: "RIGHT", author: "codebot", isCodegenie: true, - fingerprint + fingerprint, + body: `` } ]); }); diff --git a/tests/github-duplicate-detector.test.ts b/tests/github-duplicate-detector.test.ts index 4737721..52a6bea 100644 --- a/tests/github-duplicate-detector.test.ts +++ b/tests/github-duplicate-detector.test.ts @@ -2,7 +2,8 @@ import { describe, expect, it } from "vitest"; import { detectDuplicateFindings, formatCodegenieMarker, - parseCodegenieMarker + parseCodegenieMarker, + proseContentFingerprint } from "../src/github/duplicate-detector.js"; import type { ExistingReviewThread, FinalFinding } from "../src/types.js"; @@ -15,6 +16,54 @@ describe("GitHub duplicate detector", () => { expect(parseCodegenieMarker("body only")).toBeUndefined(); }); + it("round trips content markers while retaining legacy marker support", () => { + const fingerprint = "a".repeat(64), contentFingerprint = "b".repeat(64); + expect(parseCodegenieMarker(formatCodegenieMarker(fingerprint, "run-1", contentFingerprint))) + .toEqual({ fingerprint, runId: "run-1", contentFingerprint }); + expect(proseContentFingerprint("\r\nSome unchanged advice.\r\n")).toBe(proseContentFingerprint("Some unchanged advice.")); + expect(proseContentFingerprint(" ")).toBeUndefined(); + }); + + it.each(["docs/design.md", "README.MD", "guide.mdx", "guide.rst", "notes.txt"])("matches legacy prose content across distant or different anchors in %s", path => { + const body = "The table lists widgetCreated, but EventKind does not define it."; + const f = { ...finding({ line: 74 }), path, finalBody: body, + anchor: { path, side: "RIGHT" as const, hunkId: "new-hunk", line: 74 } }; + expect(detectDuplicateFindings([f], [{ id: "previous", path: f.path, line: 81, side: "RIGHT", author: "bot", isCodegenie: true, + fingerprint: "b".repeat(64), body: `${body}\n\n${formatCodegenieMarker("b".repeat(64), "old-run")}` }])) + .toEqual([expect.objectContaining({ action: "skip_unchanged_content" })]); + }); + + it.each([20, 21, 180])("does not hide a different prose defect at line %i under an old fingerprint or proximity", line => { + const f = { ...finding({ line }), path: "docs/api.md", finalBody: "Retrying this request creates duplicate orders.", + anchor: { path: "docs/api.md", side: "RIGHT" as const, hunkId: "new", line } }; + expect(detectDuplicateFindings([f], [{ id: "old", path: f.path, line: 20, side: "RIGHT", author: "bot", isCodegenie: true, + fingerprint: f.fingerprint, body: "The enum value is not defined." }])[0]?.action).toBe("post"); + }); + + it("does not match absent content, other files, other sides, or foreign comments", () => { + const f = { ...finding(), path: "docs/api.md", finalBody: "An actionable issue.", + anchor: { path: "docs/api.md", side: "RIGHT" as const, hunkId: "h1", line: 1 } }; + const base = { id: "old", path: f.path, side: "RIGHT" as const, author: "bot", isCodegenie: true, + fingerprint: f.fingerprint, body: f.finalBody }; + for (const comment of [{ ...base, body: "" }, { ...base, path: "docs/other.md" }, + { ...base, side: "LEFT" as const }, { ...base, isCodegenie: false }]) { + expect(detectDuplicateFindings([f], [comment])[0]?.action).toBe("post"); + } + }); + + it("preserves code fingerprint behavior for executable examples under docs", () => { + const f = { ...finding(), path: "docs/examples/server.ts" }; + expect(detectDuplicateFindings([f], [{ id: "old", author: "bot", isCodegenie: true, fingerprint: f.fingerprint }])[0]?.action) + .toBe("skip_exact_fingerprint"); + }); + + it("does not treat two missing anchor sides as a proven prose match", () => { + const { anchor: _anchor, ...unanchored } = finding(); + const f = { ...unanchored, path: "docs/api.md" }; + expect(detectDuplicateFindings([f], [{ id: "old", path: f.path, author: "bot", isCodegenie: true, + body: f.finalBody }])[0]?.action).toBe("post"); + }); + it("skips exact fingerprint and fuzzy nearby codegenie comments only", () => { const exact = finding({ id: "exact", fingerprint: "b".repeat(64), line: 10 }); const nearby = finding({ id: "nearby", fingerprint: "c".repeat(64), line: 105 }); diff --git a/tests/pipeline-phase5.test.ts b/tests/pipeline-phase5.test.ts index 9dede83..3385b58 100644 --- a/tests/pipeline-phase5.test.ts +++ b/tests/pipeline-phase5.test.ts @@ -10438,11 +10438,11 @@ describe("phase 5 pipeline regressions", () => { expect(result.findings[0]?.mergedCandidateIds.sort()).toEqual(["finding-1", "finding-2", "finding-3"]); }); - it("uses the canonical normalized delimiter-based final fingerprint", async () => { + it.each(["APP.ts", "docs/examples/server.ts"])("uses the canonical normalized delimiter-based final fingerprint for %s", async filePath => { const finding: CandidateFinding = { ...fakeFinding(), - path: "APP.ts", - anchor: { path: "APP.ts", line: 1, side: "RIGHT", hunkId: "h1" }, + path: filePath, + anchor: { path: filePath, line: 1, side: "RIGHT", hunkId: "h1" }, producedBy: { ...fakeFinding().producedBy, lensId: "CORE/Code-Review" } }; const result = await dedupeRankAndComposeReview( @@ -10475,11 +10475,12 @@ describe("phase 5 pipeline regressions", () => { } }, promptBuilder: fakePromptBuilder(), - packets: [packetWithSymbol("packet-1", " Checkout Flow ")] + packets: [{ ...packetWithSymbol("packet-1", " Checkout Flow "), path: filePath, + symbolFacts: packetWithSymbol("packet-1", " Checkout Flow ").symbolFacts.map(fact => ({ ...fact, path: filePath })) }] } ); - const expected = sha256Hex(["app.ts", "checkout flow", "correctness", "core/code-review"].join("\0")); + const expected = sha256Hex([filePath.toLowerCase(), "checkout flow", "correctness", "core/code-review"].join("\0")); const [final] = [...result.findings, ...result.summaryOnlyFindings]; expect(final?.fingerprint).toBe(expected); }); @@ -10566,6 +10567,161 @@ describe("phase 5 pipeline regressions", () => { expect(secondFinal?.fingerprint).toBe(expected); }); + it("keeps docs comment identity stable when a later push shifts the hunk", async () => { + const docsPath = "docs/plans/service-rollout/design.md"; + const tableRow = "| `POST /widgets` | write | `widgetCreated` |"; + const docsPacket = (hunkId: string, line: number): ReviewPacket => ({ + ...fakePacket({ path: docsPath }), + id: `packet-${hunkId}`, + language: "markdown", + hunks: [ + { + hunkId, + oldStart: line, + oldLines: 1, + newStart: line, + newLines: 1, + contentWithLineNumbers: ` ${line} ${line} +${tableRow}`, + lines: [{ kind: "add", content: tableRow, newLine: line }], + changedNewLineNumbers: [line], + changedOldLineNumbers: [] + } + ] + }); + const docsFinding = (hunkId: string, line: number): CandidateFinding => ({ + ...fakeFinding(), + path: docsPath, + anchor: { path: docsPath, line, side: "RIGHT", hunkId }, + evidence: { changedCode: tableRow }, + producedBy: { ...fakeFinding().producedBy, packetId: `packet-${hunkId}` } + }); + const compose = async (hunkId: string, line: number) => dedupeRankAndComposeReview( + { verified: [docsFinding(hunkId, line)], verdicts: [] }, + fakePlanForHunks([hunkId], docsPath), + { + mode: "branch", + repoRoot: "/tmp/repo", + commits: [], + rawDiff: "" + }, + { + totalHunks: 1, + reviewedHunks: 1, + skippedHunks: 0, + failedHunks: 0, + coverageByLevel: { deep: 0, normal: 1, light: 0, skip: 0 }, + degradedPlanning: false, + budgetStopped: false, + verificationIncompleteCount: 0, + partial: false, + reasons: [] + }, + config(), + nullTelemetry(), + { + runner: { + runStructured: async () => { + throw composerTransientError(); + } + }, + promptBuilder: fakePromptBuilder(), + packets: [docsPacket(hunkId, line)], + diff: { files: [{ ...fakeDiffFile(docsPath), language: "markdown", hunks: [{ + ...fakeDiffFile(docsPath).hunks[0]!, id: hunkId, oldStart: line, newStart: line, + lines: [{ kind: "add", content: tableRow, newLineNumber: line }] + }] }] } + } + ); + + const beforePush = await compose("hunk-at-81", 81); + const afterPush = await compose("hunk-at-74", 74); + + const [before] = [...beforePush.findings, ...beforePush.summaryOnlyFindings]; + const [after] = [...afterPush.findings, ...afterPush.summaryOnlyFindings]; + expect(before).toBeDefined(); + expect(after).toBeDefined(); + expect(after!.anchor).toEqual(docsFinding("hunk-at-74", 74).anchor); + // Keep grouping identities distinct; publication identity survives shifted geometry. + expect(before!.fingerprint).not.toBe(after!.fingerprint); + const { detectDuplicateFindings, proseContentFingerprint } = await import("../src/github/duplicate-detector.js"); + expect(proseContentFingerprint(before!.finalBody)).toBe(proseContentFingerprint(after!.finalBody)); + expect(detectDuplicateFindings([after!], [{ id: "old-comment", path: docsPath, side: "RIGHT", line: 81, + author: "bot", isCodegenie: true, fingerprint: before!.fingerprint, body: before!.finalBody }])) + .toEqual([expect.objectContaining({ action: "skip_unchanged_content", matchedCommentId: "old-comment" })]); + }); + + it("keeps distinct same-category docs findings in one file separate", async () => { + const docsPath = "docs/design.md"; + const docsPacket: ReviewPacket = { + ...fakePacket({ path: docsPath }), + kind: "coalesced-hunks", + language: "markdown", + hunks: [ + { + hunkId: "h1", + oldStart: 1, + oldLines: 1, + newStart: 1, + newLines: 1, + contentWithLineNumbers: " 1 1 +alpha claim", + lines: [{ kind: "add", content: "alpha claim", newLine: 1 }], + changedNewLineNumbers: [1], + changedOldLineNumbers: [] + }, + { + hunkId: "h2", + oldStart: 20, + oldLines: 1, + newStart: 20, + newLines: 1, + contentWithLineNumbers: " 20 20 +beta claim", + lines: [{ kind: "add", content: "beta claim", newLine: 20 }], + changedNewLineNumbers: [20], + changedOldLineNumbers: [] + } + ] + }; + const { anchor: _anchor, ...base } = fakeFinding(); + const first = { ...base, path: docsPath, changedLine: false, evidence: { changedCode: "alpha claim" } }; + const second = { ...first, id: "finding-2", title: "second docs claim", evidence: { changedCode: "beta claim" } }; + const result = await dedupeRankAndComposeReview( + { verified: [first, second], verdicts: [] }, + fakePlanForHunks(["h1", "h2"], docsPath), + { + mode: "branch", + repoRoot: "/tmp/repo", + commits: [], + rawDiff: "" + }, + { + totalHunks: 2, + reviewedHunks: 2, + skippedHunks: 0, + failedHunks: 0, + coverageByLevel: { deep: 0, normal: 2, light: 0, skip: 0 }, + degradedPlanning: false, + budgetStopped: false, + verificationIncompleteCount: 0, + partial: false, + reasons: [] + }, + { ...config(), review: { ...config().review, maxFindings: 100, softCommentCap: 100 } }, + nullTelemetry(), + { + runner: { + runStructured: async () => { + throw composerTransientError(); + } + }, + promptBuilder: fakePromptBuilder(), + packets: [docsPacket] + } + ); + + expect(result.summaryOnlyFindings).toHaveLength(2); + expect(result.summaryOnlyFindings.map(finding => finding.mergedCandidateIds)).toEqual([["finding-1"], ["finding-2"]]); + }); + it("does not merge unanchored findings from different hunks in one coalesced packet", async () => { const coalescedPacket: ReviewPacket = { ...fakePacket(), diff --git a/tsconfig.json b/tsconfig.json index 8439cb9..c7d877e 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -17,6 +17,6 @@ "noUncheckedIndexedAccess": true, "exactOptionalPropertyTypes": true }, - "include": ["src/**/*.ts", "tests/**/*.ts", "vitest.config.ts"], + "include": ["src/**/*.ts", "tests/**/*.ts", "evals/synthetics/**/*.ts", "vitest.config.ts"], "exclude": ["dist", "node_modules"] } diff --git a/vitest.config.ts b/vitest.config.ts index 556728c..4032d9a 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -3,7 +3,7 @@ import { defineConfig } from "vitest/config"; export default defineConfig({ test: { globals: true, - include: ["tests/**/*.test.ts"], + include: ["tests/**/*.test.ts", "evals/synthetics/**/*.test.ts"], restoreMocks: true, clearMocks: true }