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 f947aa5bb6a99afedb2a7b3641682c3b0cb065aa Author: Stephen Mallette <[email protected]> AuthorDate: Sun Jul 5 19:39:15 2026 -0400 tinker-review: tighten discussion discovery precision Fuzzy discovery over-linked on noise — a Kerberos-removal PR drew proposed_in edges to all ten future proposals because the keyword "NOTICE" (from the changed NOTICE file) matched the ASF license header every asciidoc carries, while the real topic "Kerberos" never made the keyword list. Keyword extraction now leads with PR-title words and drops structural filenames (NOTICE, Dockerfile), generic verbs, and repo-ubiquitous tokens — so the dev-list search and proposal matching both stop searching on noise. Proposal matching strips the license preamble, matches whole words weighted by location, and keeps a proposal only on real signal (a title/heading hit or two body keywords); the table-of-contents index page is no longer treated as a proposal. Explicit proposal references in the PR now land as EXTRACTED edges instead of being discarded, and each proposed_in edge records its confidence, matched terms, and where they matched so the link explains itself. startServer gains an optional fixed port for reproducible reruns. Assisted-by: Claude Code:claude-opus-4-8 --- .skills/tinker-review/references/schema.md | 9 +- .../tinker-review/scripts/discovery/discussions.js | 102 +++++++++++++++++---- .../scripts/graph/populate-discussions.js | 14 ++- .../tinker-review/scripts/infrastructure/docker.js | 4 +- .skills/tinker-review/scripts/review.js | 68 +++++++++++--- .skills/tinker-review/test/discovery.test.js | 97 ++++++++++++++++++++ 6 files changed, 259 insertions(+), 35 deletions(-) diff --git a/.skills/tinker-review/references/schema.md b/.skills/tinker-review/references/schema.md index f0ffcb5ae4..31da9ec935 100644 --- a/.skills/tinker-review/references/schema.md +++ b/.skills/tinker-review/references/schema.md @@ -130,7 +130,7 @@ renders this as the **Signal Confidence** panel. |------|------|----|---------| | `has_comment` | Discussion | Comment | Discussion contains this comment | | `addresses` | Discussion | Discussion | One discussion references another (e.g., PR addresses a JIRA) | -| `proposed_in` | Step | Discussion | This step was proposed/discussed here | +| `proposed_in` | Discussion(proposal) | Discussion(pr) | A `docs/src/dev/future` proposal that this PR appears to relate to. Confidence tracks how it was found (see properties below). | | `modifies` | Discussion(pr) | Function or File | The PR modifies this code | #### `addresses` edge properties @@ -140,3 +140,10 @@ renders this as the **Signal Confidence** panel. | `found_in` | `pr`, `diff`, `search`, `jira_body`, `devlist_body` | Where the link was discovered | | `found_via` | JIRA ID or URL | Which discussion contained the reference (for secondary links) | +#### `proposed_in` edge properties + +| 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. | + diff --git a/.skills/tinker-review/scripts/discovery/discussions.js b/.skills/tinker-review/scripts/discovery/discussions.js index ff05a3e87a..218158abd8 100644 --- a/.skills/tinker-review/scripts/discovery/discussions.js +++ b/.skills/tinker-review/scripts/discovery/discussions.js @@ -19,7 +19,7 @@ import { get } from "node:https"; import { readdir, readFile } from "node:fs/promises"; -import { join } from "node:path"; +import { join, basename } from "node:path"; const JIRA_BASE = "https://issues.apache.org/jira";; const DEV_LIST_API = "https://lists.apache.org/api/stats.lua";; @@ -123,7 +123,32 @@ async function searchDevList(keywords) { } } -async function findMatchingProposals(repoPath, keywords) { +// Whole-word, case-insensitive match for a keyword (so "krb5" doesn't hit inside +// another token, and a keyword must stand on word boundaries). +function keywordRegex(kw) { + const esc = kw.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + return new RegExp(`(?:^|[^A-Za-z0-9])${esc}(?:[^A-Za-z0-9]|$)`, "i"); +} + +// 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. +function splitProposal(raw) { + const lines = raw.split("\n"); + const titleIdx = lines.findIndex((l) => /^=\s+\S/.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"); + return { title, headings, body: content.join("\n") }; +} + +// The proposals index (docs/src/dev/future/index.asciidoc) is a table of +// contents, not a proposal — its headings name every topic, so it matches +// keywords spuriously. Exclude it from proposal discovery. +const isProposalIndex = (file) => /^index\.(asciidoc|adoc)$/i.test(basename(file)); + +export async function findMatchingProposals(repoPath, keywords) { const proposalDir = join(repoPath, PROPOSAL_DIR); try { const files = await readdir(proposalDir); @@ -131,28 +156,62 @@ async function findMatchingProposals(repoPath, keywords) { for (const file of files) { if (!file.endsWith(".asciidoc") && !file.endsWith(".adoc")) continue; - const content = await readFile(join(proposalDir, file), "utf-8"); - const lowerContent = content.toLowerCase(); - const matchedKeywords = keywords.filter((k) => lowerContent.includes(k.toLowerCase())); - if (matchedKeywords.length > 0) { - const titleMatch = content.match(/^=+\s*(.+)$/m); - proposals.push({ - file, - path: `${PROPOSAL_DIR}/${file}`, - source: "proposal", - title: titleMatch ? titleMatch[1].trim() : file, - matchedKeywords, - snippet: content.slice(0, 500), - }); - } + 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; + + proposals.push({ + file, + path: `${PROPOSAL_DIR}/${file}`, + source: "proposal", + title: title || file, + matchedKeywords: [...new Set([...titleHits, ...bodyHits])], + matchedIn: strong ? "title" : "body", + found_in: "search", + score: titleHits.size * 2 + bodyHits.size, + snippet: body.slice(0, 500), + }); } - return proposals.sort((a, b) => b.matchedKeywords.length - a.matchedKeywords.length); + return proposals.sort((a, b) => b.score - a.score); } catch { return []; } } +// Turn explicit proposal paths referenced in the PR text/diff into proposal +// records at EXTRACTED confidence — a named reference is a fact, not a guess. +async function resolveExplicitProposals(repoPath, proposalPaths) { + const out = []; + for (const rel of new Set(proposalPaths.map((p) => p.replace(/[.,)\]]+$/, "")))) { + if (isProposalIndex(rel)) continue; + try { + const { title } = splitProposal(await readFile(join(repoPath, rel), "utf-8")); + out.push({ + file: basename(rel), + path: rel, + source: "proposal", + title: title || rel, + matchedKeywords: [], + matchedIn: "reference", + found_in: "pr", + score: 100, + snippet: "", + }); + } catch { + // Referenced path may not exist in this worktree — skip it. + } + } + return out; +} + function extractLinksFromText(text) { const jiraRefs = [...new Set([...text.matchAll(TINKERPOP_JIRA_PATTERN)].map((m) => m[0]))]; const devListRefs = [...new Set([...text.matchAll(DEV_LIST_LINK_PATTERN)].map((m) => m[0]))]; @@ -254,10 +313,17 @@ export async function discoverDiscussions(params) { const secondaryDiscussions = await followLinks(directDiscussions); // --- Proposals --- + // Explicit references in the PR text/diff are EXTRACTED facts; keyword matches + // are the graded fuzzy fallback. Merge them, letting an explicit reference win + // over a keyword hit for the same proposal. const proposalLinks = [...allText.matchAll(/docs\/src\/dev\/future\/[^\s)\]>"]+/g)].map((m) => m[0]); let proposals = []; if (repoPath) { - proposals = await findMatchingProposals(repoPath, keywords); + const explicit = await resolveExplicitProposals(repoPath, proposalLinks); + const explicitPaths = new Set(explicit.map((p) => p.path)); + const keywordMatched = (await findMatchingProposals(repoPath, keywords)) + .filter((p) => !explicitPaths.has(p.path)); + proposals = [...explicit, ...keywordMatched]; } return { diff --git a/.skills/tinker-review/scripts/graph/populate-discussions.js b/.skills/tinker-review/scripts/graph/populate-discussions.js index ea95c57f9d..3dea9da566 100644 --- a/.skills/tinker-review/scripts/graph/populate-discussions.js +++ b/.skills/tinker-review/scripts/graph/populate-discussions.js @@ -261,12 +261,22 @@ export async function populateDiscussions(g, discussions, context) { if (batch.length >= BATCH_SIZE) { await submitBatch(batch); batch = []; } } - // proposed_in: link proposals to the PR Discussion + // 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 + // 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) => + matchedIn === "reference" ? CONFIDENCE.EXTRACTED + : matchedIn === "title" ? CONFIDENCE.INFERRED + : CONFIDENCE.AMBIGUOUS; for (const proposal of (discussions.proposals || [])) { batch.push( g.V().hasLabel("Discussion").has("source", "proposal").has("title", proposal.title) .addE("proposed_in") - .property("confidence", CONFIDENCE.INFERRED) + .property("confidence", confForMatch(proposal.matchedIn)) + .property("matched_in", proposal.matchedIn || "search") + .property("matched_keywords", (proposal.matchedKeywords || []).join(", ")) .to(__.V().hasLabel("Discussion").has("url", prUrl)) ); counts.edges++; diff --git a/.skills/tinker-review/scripts/infrastructure/docker.js b/.skills/tinker-review/scripts/infrastructure/docker.js index 085d9d4608..372072332b 100644 --- a/.skills/tinker-review/scripts/infrastructure/docker.js +++ b/.skills/tinker-review/scripts/infrastructure/docker.js @@ -53,7 +53,9 @@ export async function startServer(options = {}) { const image = options.image || DEFAULT_IMAGE; const timeoutMs = options.timeoutMs || DEFAULT_TIMEOUT_MS; - const port = await findAvailablePort(); + // A fixed port lets a review be regenerated on the same endpoint; otherwise an + // OS-assigned free port is used. + const port = options.port || await findAvailablePort(); const configDir = `/tmp/gremlin-review-${port}`; await mkdir(join(configDir, "scripts"), { recursive: true }); diff --git a/.skills/tinker-review/scripts/review.js b/.skills/tinker-review/scripts/review.js index be546d29a8..82ebc070ea 100644 --- a/.skills/tinker-review/scripts/review.js +++ b/.skills/tinker-review/scripts/review.js @@ -110,20 +110,62 @@ function classifyDomains(changedFiles) { return domains; } -function extractKeywords(changedFiles, prTitle) { - const keywords = new Set(); - for (const file of changedFiles) { - const parts = file.split("/"); - const filename = parts[parts.length - 1].replace(/\.\w+$/, ""); - if (filename.length > 3 && !["index", "package", "pom", "build"].includes(filename.toLowerCase())) { - keywords.add(filename); - } +// Structural/infra filenames that describe packaging, not a topic — they match +// boilerplate (e.g. NOTICE lives in every ASF license header) and must never +// become search keywords. +const STRUCTURAL_NAMES = new Set([ + "index", "package", "pom", "build", "notice", "license", "dockerfile", + "docker-compose", "readme", "changelog", "makefile", "setup", "config", +]); +// Generic verbs/nouns in PR titles that carry no topic signal. +const GENERIC_WORDS = new Set([ + "remove", "removes", "removed", "removal", "add", "adds", "added", "fix", + "fixes", "fixed", "update", "updates", "updated", "support", "refactor", + "improve", "improves", "cleanup", "bump", "upgrade", "migrate", "implement", + "introduce", "enable", "disable", "allow", "the", "and", "for", "with", +]); +// Tokens so common across the repo that matching on them finds everything. +const UBIQUITOUS_TOKENS = new Set([ + "gremlin", "server", "client", "test", "tests", "docker", "tinkerpop", + "apache", "core", "impl", "util", "utils", "common", "base", "default", + "abstract", "main", "java", "python", +]); + +// Split an identifier/filename into lowercased word tokens (camelCase and +// non-alphanumeric boundaries), e.g. "Krb5Authenticator" -> ["krb5","authenticator"]. +function tokenizeName(name) { + return name + .replace(/([a-z0-9])([A-Z])/g, "$1 $2") + .split(/[^A-Za-z0-9]+/) + .map((t) => t.toLowerCase()) + .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". Exported for testing. +export function extractKeywords(changedFiles, prTitle) { + const keywords = []; + const seen = new Set(); + const push = (word) => { + const k = word.toLowerCase(); + if (k.length < 3 || seen.has(k)) return; + if (GENERIC_WORDS.has(k) || UBIQUITOUS_TOKENS.has(k)) return; + seen.add(k); + keywords.push(k); + }; + + for (const w of prTitle.split(/[\s\-_:,.()]+/)) { + if (!/^\d+$/.test(w)) push(w); } - const titleWords = prTitle.split(/[\s\-_:]+/).filter((w) => w.length > 3 && !/^\d+$/.test(w)); - for (const w of titleWords.slice(0, 3)) { - keywords.add(w); + 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); } - return [...keywords].slice(0, 6); + return keywords.slice(0, 8); } async function cleanupWorktree(repoPath, worktreePath, prBranch) { @@ -194,7 +236,7 @@ export async function setup(params) { log(`PR #${pr} — classified as: ${domains.join(", ")} (${languages.join("+")}, ${changedFiles.length} files changed)`); log(`Starting Gremlin Server...`); - const handle = await startServer(); + const handle = await startServer({ port: options.port }); log(`Gremlin Server ready on port ${handle.port}`); const connection = new gremlin.driver.DriverRemoteConnection(handle.url); diff --git a/.skills/tinker-review/test/discovery.test.js b/.skills/tinker-review/test/discovery.test.js new file mode 100644 index 0000000000..b2fd2b4950 --- /dev/null +++ b/.skills/tinker-review/test/discovery.test.js @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Precision guards for discussion discovery — the keyword extraction and the +// proposal matcher that over-linked on ASF license boilerplate (PR #3502). + +import { test } from "node:test"; +import assert from "node:assert/strict"; +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 { findMatchingProposals } from "../scripts/discovery/discussions.js"; + +const ASF_HEADER = `//// +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information. +//// +`; + +test("extractKeywords drops structural filenames and keeps the title topic", () => { + const kws = extractKeywords( + ["NOTICE", "gremlin-server/src/main/java/.../Krb5Authenticator.java", "pom.xml"], + "Remove Kerberos support", + ); + assert.ok(kws.includes("kerberos"), "title topic retained"); + assert.ok(!kws.includes("notice"), "NOTICE excluded (structural)"); + assert.ok(!kws.includes("remove") && !kws.includes("support"), "generic verbs dropped"); + assert.ok(kws.includes("krb5"), "distinctive identifier token kept"); + assert.equal(kws[0], "kerberos", "title topic leads so it can't be truncated"); +}); + +async function withProposals(files, fn) { + const dir = await mkdtemp(join(tmpdir(), "tinker-prop-")); + const pdir = join(dir, "docs/src/dev/future"); + await mkdir(pdir, { recursive: true }); + try { + for (const [name, body] of Object.entries(files)) await writeFile(join(pdir, name), body); + return await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +test("a keyword that only appears in the ASF license header does not match (the #3502 bug)", async () => { + await withProposals( + { "proposal-equality.asciidoc": `${ASF_HEADER}= Equality Semantics\n\nNothing about auth here.\n` }, + async (dir) => { + const matched = await findMatchingProposals(dir, ["notice"]); + assert.equal(matched.length, 0, "NOTICE in the license header must not link the proposal"); + }, + ); +}); + +test("the proposals index (index.asciidoc) is excluded even on a title match", async () => { + await withProposals( + { "index.asciidoc": `${ASF_HEADER}= TinkerPop Future\n\n== Gremlin Console\n\nLinks to everything.\n` }, + async (dir) => { + const matched = await findMatchingProposals(dir, ["console"]); + assert.equal(matched.length, 0, "the TOC index page must not be treated as a proposal"); + }, + ); +}); + +test("a title keyword matches at INFERRED strength; a lone body mention does not", async () => { + await withProposals( + { + "proposal-asbool.asciidoc": `${ASF_HEADER}= asBool() Step\n\nDefines the asBool step.\n`, + "proposal-other.asciidoc": `${ASF_HEADER}= Transactions\n\nOne passing mention of asbool somewhere.\n`, + }, + async (dir) => { + const matched = await findMatchingProposals(dir, ["asbool"]); + const titles = matched.map((m) => m.title); + assert.ok(titles.includes("asBool() Step"), "title match kept"); + assert.equal(matched.find((m) => m.title === "asBool() Step").matchedIn, "title"); + assert.ok(!titles.includes("Transactions"), "single body mention below threshold is dropped"); + }, + ); +});
