This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit a88f5010a80619dae7b52d62b3a841112a94565e Author: Stephen Mallette <[email protected]> AuthorDate: Mon Oct 12 12:11:31 2026 -0400 Stop tinker-review linking unrelated proposals Proposal matching now needs an identifier-like keyword or a changed module's name in a proposal's title, so ordinary words from file names no longer link proposals. Proposals that start at a level-2 heading get their real title. Assisted-by: Claude Code:claude-opus-5-5 --- .skills/tinker-review/references/schema.md | 4 +- .../tinker-review/scripts/discovery/discussions.js | 65 ++++++++---- .../scripts/graph/populate-discussions.js | 5 +- .skills/tinker-review/scripts/review.js | 116 +++++++++++++++++---- .skills/tinker-review/test/discovery.test.js | 99 +++++++++++++++++- 5 files changed, 248 insertions(+), 41 deletions(-) diff --git a/.skills/tinker-review/references/schema.md b/.skills/tinker-review/references/schema.md index 026df0e9a7..5fdb3f0f75 100644 --- a/.skills/tinker-review/references/schema.md +++ b/.skills/tinker-review/references/schema.md @@ -201,8 +201,8 @@ renders this as the **Signal Confidence** panel. | Property | Values | Meaning | |----------|--------|---------| -| `matched_in` | `reference`, `title`, `body` | How the proposal was linked: an explicit path reference in the PR (`EXTRACTED`), a keyword in the proposal's title/heading (`INFERRED`), or keywords in its body (`AMBIGUOUS`). | -| `matched_keywords` | comma-separated terms | The keywords that matched, so the link is self-explanatory. | +| `matched_in` | `reference`, `title`, `body`, `location` | How the proposal was linked: an explicit path reference in the PR (`EXTRACTED`), a strong keyword in the proposal's title or a heading (`INFERRED`), two strong keywords in its body (`AMBIGUOUS`), or the name of a changed module in its title (`AMBIGUOUS`). Strong keywords are identifier-shaped: camelCase, a step written `name()` in the PR title, or letters mixed with digits. | +| `matched_keywords` | comma-separated terms | The keywords that matched, so the link is self-explanatory. Includes weak keywords (plain words from the PR title or split from file names), which are listed on a proposal another keyword matched but never match one themselves. | ### Project memory diff --git a/.skills/tinker-review/scripts/discovery/discussions.js b/.skills/tinker-review/scripts/discovery/discussions.js index aff7c158b5..4f22b92d53 100644 --- a/.skills/tinker-review/scripts/discovery/discussions.js +++ b/.skills/tinker-review/scripts/discovery/discussions.js @@ -148,15 +148,19 @@ function keywordRegex(kw) { } // Split an asciidoc proposal into { title, headings, body }, dropping the ASF -// license preamble by starting at the first level-1 title (`= Title`). Without -// this, keyword matching hits the license header ("...the NOTICE file...") that -// every ASF-licensed doc carries. +// license preamble by starting at the first heading. Without this, keyword +// matching hits the license header ("...the NOTICE file...") that every +// ASF-licensed doc carries. The first heading is the title at whatever level it +// is: most proposals are included into the future index and start at `==`. +const HEADING = /^=+\s+\S/; +const headingText = (line) => line.replace(/^=+\s+/, "").replace(/\s+=+\s*$/, "").trim(); + function splitProposal(raw) { const lines = raw.split("\n"); - const titleIdx = lines.findIndex((l) => /^=\s+\S/.test(l)); + const titleIdx = lines.findIndex((l) => HEADING.test(l)); const content = titleIdx >= 0 ? lines.slice(titleIdx) : lines; - const title = titleIdx >= 0 ? content[0].replace(/^=\s+/, "").trim() : ""; - const headings = content.filter((l) => /^=+\s+\S/.test(l)).join("\n"); + const title = titleIdx >= 0 ? headingText(content[0]) : ""; + const headings = content.filter((l) => HEADING.test(l)).map(headingText).join("\n"); return { title, headings, body: content.join("\n") }; } @@ -165,7 +169,25 @@ function splitProposal(raw) { // keywords spuriously. Exclude it from proposal discovery. const isProposalIndex = (file) => /^index\.(asciidoc|adoc)$/i.test(basename(file)); +/** + * Find proposals under docs/src/dev/future/ that the PR's keywords point at. + * Keywords come graded (see proposalKeywords() in review.js); a plain array is + * treated as all strong. A proposal is kept when: + * - a strong keyword is in its title or a heading (matchedIn "title", INFERRED); + * - otherwise two distinct strong keywords are in its body ("body", AMBIGUOUS). + * One is not enough: a changed class every proposal about its component + * names, such as TinkerGraph, would link them all; + * - otherwise a location keyword (a module name) is in its title ("location", + * AMBIGUOUS). Not its headings: section headings use ordinary words that are + * also module names (language, examples). + * Weak keywords never keep a proposal; they are reported and scored on one that + * is already kept. + * + * @param {string} repoPath + * @param {string[]|{strong: string[], weak?: string[], location?: string[]}} keywords + */ export async function findMatchingProposals(repoPath, keywords) { + const { strong = [], weak = [], location = [] } = Array.isArray(keywords) ? { strong: keywords } : keywords; const proposalDir = join(repoPath, PROPOSAL_DIR); try { const files = await readdir(proposalDir); @@ -176,23 +198,27 @@ export async function findMatchingProposals(repoPath, keywords) { if (isProposalIndex(file)) continue; const { title, headings, body } = splitProposal(await readFile(join(proposalDir, file), "utf-8")); - const matches = (text) => keywords.filter((k) => keywordRegex(k).test(text)); - const titleHits = new Set([...matches(title), ...matches(headings)]); - const bodyHits = new Set(matches(body)); - // Keep a proposal only on real signal: a keyword in its title/heading, or - // at least two distinct keywords in the body. A lone body mention is noise. - const strong = titleHits.size > 0; - if (!strong && bodyHits.size < 2) continue; + const top = `${title}\n${headings}`; + const hits = (list, text) => list.filter((k) => keywordRegex(k).test(text)); + const strongTop = hits(strong, top); + const strongBody = hits(strong, body); + const locationTop = hits(location, title); + const matchedIn = strongTop.length > 0 ? "title" + : strongBody.length >= 2 ? "body" + : locationTop.length > 0 ? "location" + : null; + if (!matchedIn) continue; + const weakHits = hits(weak, body); proposals.push({ file, path: `${PROPOSAL_DIR}/${file}`, source: "proposal", title: title || file, - matchedKeywords: [...new Set([...titleHits, ...bodyHits])], - matchedIn: strong ? "title" : "body", + matchedKeywords: [...new Set([...strongTop, ...strongBody, ...locationTop, ...weakHits])], + matchedIn, found_in: "search", - score: titleHits.size * 2 + bodyHits.size, + score: strongTop.length * 3 + strongBody.length * 2 + locationTop.length + weakHits.length, snippet: body.slice(0, 500), }); } @@ -309,11 +335,13 @@ async function followLinks(discussions) { * @param {string} params.prBody - PR body/description * @param {string} params.diff - The full diff text * @param {string[]} params.keywords - Keywords for search (class names, step names, etc.) + * @param {{strong: string[], weak: string[], location: string[]}} [params.proposalKeywords] - Graded + * keywords for proposal matching (see findMatchingProposals); `keywords` is used when absent. * @param {string} params.repoPath - Path to the repo (for proposal discovery) * @returns {Promise<DiscoveryResult>} */ export async function discoverDiscussions(params) { - const { pr, prTitle = "", prBody = "", diff = "", keywords = [], repoPath = "" } = params; + const { pr, prTitle = "", prBody = "", diff = "", keywords = [], proposalKeywords = null, repoPath = "" } = params; // Fetch PR comments from GitHub (issue comments + inline review comments) const prCommentErrors = []; @@ -360,7 +388,7 @@ export async function discoverDiscussions(params) { if (repoPath) { const explicit = await resolveExplicitProposals(repoPath, proposalLinks); const explicitPaths = new Set(explicit.map((p) => p.path)); - const keywordMatched = (await findMatchingProposals(repoPath, keywords)) + const keywordMatched = (await findMatchingProposals(repoPath, proposalKeywords || keywords)) .filter((p) => !explicitPaths.has(p.path)); proposals = [...explicit, ...keywordMatched]; } @@ -373,6 +401,7 @@ export async function discoverDiscussions(params) { devListMissing: explicitDevList.length === 0 && searchedDevList.length === 0, devListSearchPerformed, devListSearchKeywords: devListSearchPerformed ? keywords : [], + proposalKeywords, secondary: secondaryDiscussions, diff --git a/.skills/tinker-review/scripts/graph/populate-discussions.js b/.skills/tinker-review/scripts/graph/populate-discussions.js index 3dea9da566..b31e1059ad 100644 --- a/.skills/tinker-review/scripts/graph/populate-discussions.js +++ b/.skills/tinker-review/scripts/graph/populate-discussions.js @@ -262,8 +262,9 @@ export async function populateDiscussions(g, discussions, context) { } // proposed_in: link proposals to the PR Discussion. Confidence tracks how the - // link was found: an explicit reference is EXTRACTED, a title/heading keyword - // match is INFERRED, a body-only match is AMBIGUOUS (so it surfaces for human + // link was found: an explicit reference is EXTRACTED, a strong keyword in the + // title or a heading is INFERRED, and a strong keyword only in the body or a + // module/package name in a heading is AMBIGUOUS (so it surfaces for human // review). The matched terms and location ride on the edge so the link is // self-explanatory — mirroring `addresses`' found_in/found_via. const confForMatch = (matchedIn) => diff --git a/.skills/tinker-review/scripts/review.js b/.skills/tinker-review/scripts/review.js index 1d0e672a21..b4321080d2 100644 --- a/.skills/tinker-review/scripts/review.js +++ b/.skills/tinker-review/scripts/review.js @@ -206,42 +206,121 @@ function tokenizeName(name) { .filter((t) => t.length >= 3); } -// Keywords for discovery search (dev-list + proposals). Topic first: PR-title -// words (the human statement of intent) lead so they can't be truncated by the -// cap, then distinctive tokens from changed-file basenames. Structural names, -// generic verbs, and repo-ubiquitous tokens are filtered out so we don't search -// on noise like "NOTICE" or "gremlin-server". So are location words: any token -// of a changed file's directory path (a module such as tinkergraph-gremlin, a -// package such as structure/storage) says where the change is, not what it is -// about, and matches every proposal that mentions the module. Exported for testing. -export function extractKeywords(changedFiles, prTitle) { +// Every token of a changed file's directory path: a module such as +// tinkergraph-gremlin or a package such as structure/storage. These say where a +// change is, not what it is about. +function locationTokens(changedFiles) { const location = new Set(); for (const file of changedFiles) { for (const dir of file.split("/").slice(0, -1)) { for (const tok of dir.toLowerCase().split(/[^a-z0-9]+/)) location.add(tok); } } + return location; +} + +// Top-level directories that are not modules. +const NON_MODULE_TOKENS = new Set(["docs", "bin", "data", "docker", "licenses", "github", "beads", "skills"]); + +// The names of the modules changed files belong to, from the top-level directory +// (tinkergraph-gremlin -> tinkergraph). Package directories below it are left +// out: gremlin-core's (process, traversal, step, map, graph) are the vocabulary +// every proposal's headings use. +function moduleTokens(changedFiles) { + const modules = new Set(); + for (const file of changedFiles) { + const parts = file.split("/"); + if (parts.length < 2) continue; + for (const tok of parts[0].toLowerCase().split(/[^a-z0-9]+/)) { + if (tok && !NON_MODULE_TOKENS.has(tok)) modules.add(tok); + } + } + return modules; +} + +const usableKeyword = (k) => k.length >= 3 && !GENERIC_WORDS.has(k) && !UBIQUITOUS_TOKENS.has(k); +const changedBasenames = (changedFiles) => changedFiles + .map((file) => file.split("/").pop().replace(/\.\w+$/, "")) + .filter((base) => !STRUCTURAL_NAMES.has(base.toLowerCase())); + +// Keywords for discovery search (dev-list + proposals). Topic first: PR-title +// words (the human statement of intent) lead so they can't be truncated by the +// cap, then distinctive tokens from changed-file basenames. Structural names, +// generic verbs, and repo-ubiquitous tokens are filtered out so we don't search +// on noise like "NOTICE" or "gremlin-server". So are location words taken from +// basenames (see locationTokens); the title is the author's own statement of the +// topic, so its words are kept even when they also name a directory. Exported +// for testing. +export function extractKeywords(changedFiles, prTitle) { + const location = locationTokens(changedFiles); const keywords = []; const seen = new Set(); - const push = (word) => { + const push = (word, isTitle) => { const k = word.toLowerCase(); - if (k.length < 3 || seen.has(k)) return; - if (GENERIC_WORDS.has(k) || UBIQUITOUS_TOKENS.has(k) || location.has(k)) return; + if (!usableKeyword(k) || seen.has(k)) return; + if (!isTitle && location.has(k)) return; seen.add(k); keywords.push(k); }; for (const w of prTitle.split(/[\s\-_:,.()]+/)) { - if (!/^\d+$/.test(w)) push(w); + if (!/^\d+$/.test(w)) push(w, true); } - for (const file of changedFiles) { - const base = file.split("/").pop().replace(/\.\w+$/, ""); - if (STRUCTURAL_NAMES.has(base.toLowerCase())) continue; - for (const tok of tokenizeName(base)) push(tok); + for (const base of changedBasenames(changedFiles)) { + for (const tok of tokenizeName(base)) push(tok, false); } return keywords.slice(0, 8); } +// A word whose shape marks it as an identifier: camelCase (TinkerStorageGraph, +// asNumber) or letters mixed with digits (krb5). Plain words are not. +const IDENTIFIER_SHAPED = (w) => /[a-z][A-Z]/.test(w) || /[A-Za-z]\d|\d[A-Za-z]/.test(w); + +/** + * Keywords for proposal matching, graded by how much a hit means. + * - strong: identifier-shaped words. These are words in the PR title that are + * camelCase, mix letters and digits, or are written as a step call + * (`select()`), plus whole camelCase basenames of changed files. They are the + * only grade that can make a match on its own. + * - weak: plain words from the title and the words a basename splits into + * (THREAT_MODEL -> threat, model). They are ordinary English often enough that + * they only add to a match a strong or location keyword already made. + * - location: the names of the modules changed files belong to (tinkergraph). + * They count only on a proposal's title. + * Exported for testing. + * + * @param {string[]} changedFiles + * @param {string} prTitle + * @returns {{strong: string[], weak: string[], location: string[]}} + */ +export function proposalKeywords(changedFiles, prTitle) { + const location = locationTokens(changedFiles); + const strong = []; + const weak = []; + const seen = new Set(); + const add = (list, word) => { + const k = word.toLowerCase(); + if (!usableKeyword(k) || seen.has(k)) return; + seen.add(k); + list.push(k); + }; + + const titleWords = [...prTitle.matchAll(/([A-Za-z][A-Za-z0-9]*)(\(\))?/g)] + .filter(([w]) => w.toUpperCase() !== "TINKERPOP"); + for (const [, w, call] of titleWords) if (call || IDENTIFIER_SHAPED(w)) add(strong, w); + const bases = changedBasenames(changedFiles); + for (const base of bases) if (/^[A-Za-z0-9]+$/.test(base) && IDENTIFIER_SHAPED(base)) add(strong, base); + // Module names before weak words: a module named by a plain title word must + // still be able to match a proposal's title. + const loc = []; + for (const tok of moduleTokens(changedFiles)) add(loc, tok); + for (const [, w] of titleWords) add(weak, w); + for (const base of bases) { + for (const tok of tokenizeName(base)) if (!location.has(tok)) add(weak, tok); + } + return { strong, weak, location: loc }; +} + async function cleanupWorktree(repoPath, worktreePath, prBranch) { if (existsSync(worktreePath)) { await exec("git", ["worktree", "remove", "--force", worktreePath], { cwd: repoPath }).catch(() => {}); @@ -442,12 +521,13 @@ export async function phase1(session) { prBody, diff: diffText, keywords: prKeywords, + proposalKeywords: proposalKeywords(changedFiles, prTitle.trim()), repoPath, }); log(` jiras: ${discussions.jiras.length} found${discussions.jiraMissing ? " (none referenced)" : ""}`); log(` pr comments: ${discussions.prComments.issue.length} issue + ${discussions.prComments.review.length} review${discussions.prCommentsError ? ` (FETCH FAILED — ${discussions.prCommentsError})` : ""}`); log(` dev list: ${discussions.devList.length} found${discussions.devListSearchPerformed ? ` (searched: ${prKeywords.join(", ")})` : ""}`); - log(` proposals: ${discussions.proposals.length} found`); + log(` proposals: ${discussions.proposals.length} found (strong: ${discussions.proposalKeywords.strong.slice(0, 8).join(", ") || "none"})`); log(`Loading discussions into graph...`); await populateDiscussions(g, discussions, { pr, prTitle: prTitle.trim(), changedFiles }); diff --git a/.skills/tinker-review/test/discovery.test.js b/.skills/tinker-review/test/discovery.test.js index 67208bc84f..8259d63b0b 100644 --- a/.skills/tinker-review/test/discovery.test.js +++ b/.skills/tinker-review/test/discovery.test.js @@ -26,7 +26,7 @@ import { mkdtemp, writeFile, mkdir, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { extractKeywords } from "../scripts/review.js"; +import { extractKeywords, proposalKeywords } from "../scripts/review.js"; import { findMatchingProposals } from "../scripts/discovery/discussions.js"; const ASF_HEADER = `//// @@ -109,3 +109,100 @@ test("extractKeywords drops words that only name where the change is (module and assert.ok(kws.includes("tinkerstoragegraph"), "title topic kept"); assert.ok(kws.includes("binary"), "a basename token that is not a location is kept"); }); + +test("extractKeywords keeps a PR-title word even when it also names a changed directory", () => { + const kws = extractKeywords( + ["tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransaction.java"], + "Fix TinkerGraph commit ordering", + ); + assert.ok(kws.includes("tinkergraph"), "the author's own topic word survives the location filter"); +}); + +// The changed files of PR 3639 that drove its keywords, less TinkerGraph.java so +// that tinkergraph stays a module name rather than a changed class. +const PR_3639_FILES = [ + "THREAT_MODEL.md", + "docs/src/reference/implementations-tinkergraph.asciidoc", + "docs/src/upgrade/release-4.x.x.asciidoc", + "gremlin-server/conf/tinkergraph-credentials.properties", + "gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/auth/SimpleAuthenticator.java", + "tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerStorageGraph.java", + "tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorage.java", +]; +const PR_3639_TITLE = "TINKERPOP-3282 TinkerStorageGraph"; + +test("proposalKeywords grades identifiers strong, plain words weak and module names as location", () => { + const { strong, weak, location } = proposalKeywords(PR_3639_FILES, PR_3639_TITLE); + for (const k of ["tinkerstoragegraph", "simpleauthenticator", "graphbinarystorage"]) assert.ok(strong.includes(k), `${k} is strong`); + for (const k of ["threat", "model", "implementations", "simple"]) assert.ok(weak.includes(k), `${k} is weak`); + assert.ok(location.includes("tinkergraph"), "the module name is a location keyword"); + for (const k of ["storage", "structure", "auth", "docs", "reference"]) assert.ok(!location.includes(k), `${k} is not a module name`); + assert.ok(!strong.includes("tinkerpop"), "the JIRA prefix is not a keyword"); +}); + +test("proposalKeywords keeps a module named by a plain title word as a location keyword", () => { + const { location, weak } = proposalKeywords( + ["tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransaction.java"], + "Fix tinkergraph commit ordering", + ); + assert.ok(location.includes("tinkergraph") && !weak.includes("tinkergraph")); +}); + +test("proposalKeywords makes a step written as a call in the title strong, and plain title words weak", () => { + const { strong, weak } = proposalKeywords([], "Clarify local() side-effect state semantics"); + assert.deepEqual(strong, ["local"]); + assert.ok(weak.includes("semantics") && weak.includes("clarify")); +}); + +test("PR 3639: dictionary words split from file names link nothing; the module name finds the transaction proposal", async () => { + await withProposals( + { + // Real proposals start at ==, since they are included into the future index. + "proposal-scoping-5.asciidoc": `${ASF_HEADER}== Lazy vs. Eager Evaluation in TP4 ==\n\nA simple model that implementations may follow.\n`, + "proposal-type-predicate-8.asciidoc": `${ASF_HEADER}== Type Predicate\n\n=== Motivation\n\nSimple for implementations.\n`, + "proposal-transaction-4.asciidoc": `${ASF_HEADER}== TinkerGraph Transaction Support\n\nTransactions for the in-memory graph.\n`, + "proposal-equality-1.asciidoc": `${ASF_HEADER}= Equality Semantics\n\n== Implementations\n\nTinkerGraph storage behaves the same.\n`, + }, + async (dir) => { + const matched = await findMatchingProposals(dir, proposalKeywords(PR_3639_FILES, PR_3639_TITLE)); + assert.deepEqual(matched.map((m) => m.file), ["proposal-transaction-4.asciidoc"]); + assert.equal(matched[0].matchedIn, "location", "a module-name match is graded AMBIGUOUS"); + assert.equal(matched[0].title, "TinkerGraph Transaction Support", "a title at == is still the title"); + }, + ); +}); + +test("a body-only match needs two distinct strong keywords, and weak keywords never make one", async () => { + await withProposals( + { + "proposal-two.asciidoc": `${ASF_HEADER}== Two\n\nMentions FooStep and BarStep.\n`, + "proposal-one.asciidoc": `${ASF_HEADER}== One\n\nMentions FooStep only.\n`, + "proposal-weak.asciidoc": `${ASF_HEADER}== Weak\n\n=== Model\n\nA simple model.\n`, + }, + async (dir) => { + const matched = await findMatchingProposals(dir, { strong: ["foostep", "barstep"], weak: ["model", "simple"], location: [] }); + assert.deepEqual(matched.map((m) => [m.file, m.matchedIn]), [["proposal-two.asciidoc", "body"]]); + }, + ); +}); + +test("a module name on a section heading, not the title, does not match", async () => { + await withProposals( + { "proposal-x.asciidoc": `${ASF_HEADER}== Removing Closures\n\n=== The Gremlin Language\n\nText.\n` }, + async (dir) => { + assert.equal((await findMatchingProposals(dir, { strong: [], weak: [], location: ["language"] })).length, 0); + }, + ); +}); + +test("a proposal starting at == takes that heading as its title and never matches its license header", async () => { + await withProposals( + { "proposal-scoping.asciidoc": `${ASF_HEADER}== Lazy vs. Eager Evaluation ==\n\nBody.\n` }, + async (dir) => { + const [m] = await findMatchingProposals(dir, ["eager"]); + assert.equal(m.title, "Lazy vs. Eager Evaluation"); + assert.ok(!m.snippet.includes("Licensed"), "the snippet starts at the title"); + assert.equal((await findMatchingProposals(dir, ["notice"])).length, 0); + }, + ); +});
