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 4e79ba292594e366da6c856cd990e74e1f7ae4a2 Author: Stephen Mallette <[email protected]> AuthorDate: Thu Oct 8 12:17:08 2026 -0400 Name tinker-review communities by their content The agent names each Thematic Community from its key members; kind labels become "implementation" and "test code". The stamped community property now matches the reported community id. Assisted-by: Claude Code:claude-opus-5-5 --- .skills/tinker-review/SKILL.md | 8 +++ .skills/tinker-review/references/interfaces.md | 4 ++ .../scripts/patterns/community-detection.js | 84 +++++++++++++++++----- .skills/tinker-review/scripts/renderer/render.js | 42 +++++++---- .../tinker-review/test/community-detection.test.js | 40 ++++++++++- 5 files changed, 145 insertions(+), 33 deletions(-) diff --git a/.skills/tinker-review/SKILL.md b/.skills/tinker-review/SKILL.md index b165f05fc4..eec76e278d 100644 --- a/.skills/tinker-review/SKILL.md +++ b/.skills/tinker-review/SKILL.md @@ -237,6 +237,14 @@ produce a complete evidence-with-narrative JSON file. Write it to literally (vertices/edges/labels/counts), do NOT re-narrate what Removal References, Coverage, or Blast Radius already report, and do NOT pad. The renderer draws the diagram and the appendix lists membership; you supply only the judgment. +- `communityNames` — object mapping each community's `id` (from + `checks.communities.communities`) to a short name for what it is about, e.g. + `{ "0": "Barrier step reduction", "1": "GraphBinary type IO" }`. Derive each name + from the community's `keyMembers` and `files`; if those leave it unclear, query its + members with `g.V().has("community", id)`. Aim for 2–5 words and at most 40 + characters; the box truncates anything longer. Name the concern, not the structure: + the renderer already shows the kind (`implementation`, `test code`, `type hierarchy`, + `file group`) next to the name. Any community left unnamed renders as "Community N". - `guidedWalk` — array of `{ title, badge, badgeText, body }` objects - `findings` — array of `{ title, snippet, body }` objects, ordered most-severe-first (Interpret grades each blocking / high / low) - `openQuestions` — array of `{ title, body, meta }` objects diff --git a/.skills/tinker-review/references/interfaces.md b/.skills/tinker-review/references/interfaces.md index 282e06adf1..b3644c73cb 100644 --- a/.skills/tinker-review/references/interfaces.md +++ b/.skills/tinker-review/references/interfaces.md @@ -137,6 +137,10 @@ interface Evidence { interface ReportPackage extends Evidence { summary: string; // HTML clusters: { assessment: string }; // narrative prose — distinct from checks.clusters + communityAssessment: string; // HTML — judgment on checks.communities + communityNames?: { [id: string]: string }; + // content-derived name per community, keyed by checks.communities.communities[].id + // (0-based; also the stamped `community` vertex property). Unnamed → "Community N". guidedWalk: { title; badge; badgeText; body }[]; findings: { title; snippet; body }[]; openQuestions: { title; body; meta }[]; diff --git a/.skills/tinker-review/scripts/patterns/community-detection.js b/.skills/tinker-review/scripts/patterns/community-detection.js index 51bc9d8d02..cc9ec5da2a 100644 --- a/.skills/tinker-review/scripts/patterns/community-detection.js +++ b/.skills/tinker-review/scripts/patterns/community-detection.js @@ -62,6 +62,9 @@ const WEIGHT = { EXTRACTED: 3, INFERRED: 2, AMBIGUOUS: 1 }; const NODE_LABELS = ["Function", "Type", "File", "Test"]; const EDGE_LABELS = ["calls", "defines", "declares", "extends", "implements", "overrides", "tests"]; +/** How many member names each community carries for the agent to name it from. */ +const KEY_MEMBER_LIMIT = 8; + /** * @typedef {Object} Community * @property {number} id 0-based community index (largest first) @@ -73,7 +76,10 @@ const EDGE_LABELS = ["calls", "defines", "declares", "extends", "implements", "o * @property {string} dominantLabel most common vertex label * @property {number} testShare fraction of members living in test files — drives the role * @property {?Object} churn `{ added, removed, mode }` for the community's files, when churn is supplied - * @property {string} role human descriptor, e.g. "test scaffolding, reduced (−240/+12), spanning 3 files" + * @property {string} kind structural category: "test code", "type hierarchy", "file group" or "implementation" + * @property {string[]} keyMembers up to KEY_MEMBER_LIMIT member names, changed first then most-connected — + * the material the agent reads to give the community a descriptive name + * @property {string} role human descriptor, e.g. "test code, reduced (−240/+12), spanning 3 files" * * @typedef {Object} CommunityResult * @property {number} communityCount communities with size >= minCommunitySize @@ -165,21 +171,28 @@ export function detectCommunities(nodes, edges, params = {}) { graph.forEachNode((n, _attr) => { communities[n] = graph.nodes().indexOf(n); }); } - const { assignments, splitCount } = splitDisconnected(graph, communities); + const { assignments: pieces, splitCount } = splitDisconnected(graph, communities); // Group members by final community id. const members = new Map(); - for (const [id, comm] of assignments) { + for (const [id, comm] of pieces) { if (!members.has(comm)) members.set(comm, []); members.get(comm).push(id); } - const built = [...members.entries()] - .map(([, ids]) => rollUp(ids, nodeById, churn)) - .sort((a, b) => b.size - a.size); + const built = [...members.values()] + .map((ids) => ({ ids, c: rollUp(ids, nodeById, churn, graph) })) + .sort((a, b) => b.c.size - a.c.size); - const communityList = built.filter((c) => c.size >= minSize).map((c, i) => ({ ...c, id: i })); - const isolatedCount = built.filter((c) => c.size < minSize).length; + const reported = built.filter(({ c }) => c.size >= minSize); + const communityList = reported.map(({ c }, i) => ({ ...c, id: i })); + const isolatedCount = built.length - reported.length; + + // Write-back stamps the reported id, so `has("community", n)` returns exactly + // the members of the community the report shows as n; vertices below + // minCommunitySize stay unstamped. + const assignments = new Map(); + reported.forEach(({ ids }, i) => { for (const id of ids) assignments.set(id, i); }); return { communityCount: communityList.length, @@ -252,16 +265,12 @@ function interpret(communities, modularity, churn) { } /** - * Describe what a community is: its kind (test scaffolding / call cluster / …), - * how it changed, and its file span. When churn is known it drives the change + * Describe what a community is: its kind (see communityKind), how it changed, + * and its file span. When churn is known it drives the change * clause (real line counts beat the changed/unchanged vertex-share heuristic); * otherwise we fall back to that share. */ -function describeRole(label, changedShare, fileCount, testShare, churnSummary) { - const kind = testShare >= 0.6 ? "test scaffolding" - : label === "Type" ? "type hierarchy" - : label === "File" ? "file group" - : "call cluster"; +function describeRole(kind, changedShare, fileCount, churnSummary) { const change = churnSummary ? `${churnSummary.mode} (−${churnSummary.removed}/+${churnSummary.added})` : changedShare >= 0.6 ? "mostly changed" @@ -312,8 +321,44 @@ function splitDisconnected(graph, communities) { return { assignments, splitCount }; } -/** Roll a community's member vertices up to files, label mix, churn, and a role descriptor. */ -function rollUp(ids, nodeById, churn) { +/** + * The structural category of a community, from what its members are. This is + * only the coarse kind; the agent names each community by its content during + * enrichment (report.json `communityNames`), working from `keyMembers`. + */ +function communityKind(label, testShare) { + return testShare >= 0.6 ? "test code" + : label === "Type" ? "type hierarchy" + : label === "File" ? "file group" + : "implementation"; +} + +/** + * The member names most telling of what a community is about: changed members + * first (they are why the community matters to the PR), then by weighted degree + * within the community (the hubs the rest hang off). File vertices are skipped — + * the file list already carries them. + */ +function keyMembers(ids, nodeById, graph) { + const inComm = new Set(ids); + const internalDegree = (id) => { + let d = 0; + graph.forEachEdge(id, (_e, attr, src, tgt) => { + if (inComm.has(src === id ? tgt : src)) d += attr.weight || 1; + }); + return d; + }; + return ids + .map((id) => nodeById.get(id)) + .filter((n) => n && n.name && n.label !== "File") + .map((n) => ({ n, degree: internalDegree(n.id) })) + .sort((a, b) => (b.n.changed ? 1 : 0) - (a.n.changed ? 1 : 0) || b.degree - a.degree) + .slice(0, KEY_MEMBER_LIMIT) + .map(({ n }) => n.name); +} + +/** Roll a community's member vertices up to files, label mix, churn, key members, and a role descriptor. */ +function rollUp(ids, nodeById, churn, graph) { const fileCounts = new Map(); const labelCounts = {}; let changedCount = 0; @@ -344,6 +389,7 @@ function rollUp(ids, nodeById, churn) { if (withChurn > 0) churnSummary = { added, removed, mode: churnMode(added, removed) }; } + const kind = communityKind(dominantLabel, testShare); return { size: ids.length, files, @@ -353,7 +399,9 @@ function rollUp(ids, nodeById, churn) { dominantLabel, testShare, churn: churnSummary, - role: describeRole(dominantLabel, ids.length ? changedCount / ids.length : 0, files.length, testShare, churnSummary), + kind, + keyMembers: keyMembers(ids, nodeById, graph), + role: describeRole(kind, ids.length ? changedCount / ids.length : 0, files.length, churnSummary), }; } diff --git a/.skills/tinker-review/scripts/renderer/render.js b/.skills/tinker-review/scripts/renderer/render.js index 6b4e3fe36f..cb221255ae 100644 --- a/.skills/tinker-review/scripts/renderer/render.js +++ b/.skills/tinker-review/scripts/renderer/render.js @@ -96,7 +96,7 @@ function notProvided(sectionId, title) { } export function render(evidence) { - const { meta, graphStats, checks, discussions, summary, clusters, communityAssessment, + const { meta, graphStats, checks, discussions, summary, clusters, communityAssessment, communityNames, guidedWalk, functionalTest, findings, openQuestions, appendixFunctional } = evidence; const parts = []; @@ -105,12 +105,12 @@ export function render(evidence) { parts.push(summary ? renderSummary(summary) : notProvided("summary", "Summary")); parts.push(discussions ? renderContext(discussions) : notProvided("context", "Discovered Context")); parts.push(renderClusters(clusters, checks && checks.clusters, evidence.architecture)); - parts.push(renderCommunitySection(checks && checks.communities, communityAssessment)); + parts.push(renderCommunitySection(checks && checks.communities, communityAssessment, communityNames)); parts.push(guidedWalk && guidedWalk.length > 0 ? renderGuidedWalk(guidedWalk) : notProvided("guided-walk", "Guided Walk")); parts.push(functionalTest ? renderFunctionalTest(functionalTest) : notProvided("functional-test", "Functional Test")); parts.push(findings && findings.length > 0 ? renderFindings(findings) : notProvided("findings", "Findings")); parts.push(openQuestions && openQuestions.length > 0 ? renderOpenQuestions(openQuestions) : notProvided("open-questions", "Open Questions")); - parts.push(renderAppendixStructural(checks, graphStats)); + parts.push(renderAppendixStructural(checks, graphStats, communityNames)); parts.push(appendixFunctional ? renderAppendixFunctional(appendixFunctional) : notProvided("appendix-functional", "Appendix: Functional Test Details")); parts.push(`<footer>Graph Review — Apache TinkerPop | PR #${esc(String(meta.pr))} | Generated ${esc(meta.timestamp)}</footer>`); @@ -322,11 +322,25 @@ function renderClusters(clusters, clusterData, architecture) { </section>`; } -function renderCommunitySection(communityData, assessment) { +/** + * A community's display name: the agent's content-derived name from report.json + * `communityNames` (keyed by the community's 0-based `id`), else "Community N". + */ +function communityName(c, names) { + const named = names && names[c.id]; + return named ? String(named) : `Community ${c.id + 1}`; +} + +/** Structural kind; older evidence carries it only as the role's first clause. */ +function communityKindOf(c) { + return c.kind || c.role.split(",")[0]; +} + +function renderCommunitySection(communityData, assessment, names) { if (!communityData || !communityData.interpretation) return notProvided("communities", "Thematic Communities"); const interp = communityData.interpretation; - const svg = generateCommunitySvg(communityData); + const svg = generateCommunitySvg(communityData, names); // The main section is the analyst's meaning-making (enrichment). Until that runs, // fall back to the deterministic reading so the section is never empty. @@ -348,7 +362,7 @@ function renderCommunitySection(communityData, assessment) { </section>`; } -function generateCommunitySvg(communityData) { +function generateCommunitySvg(communityData, names) { const communities = (communityData && communityData.communities) || []; if (communities.length === 0) return ""; @@ -380,11 +394,12 @@ function generateCommunitySvg(communityData) { const y = rowY[row]; const h = rowH[row]; const cc = modeColor(c); - const kind = c.role.split(",")[0]; + const name = communityName(c, names); + const title = name.length > 40 ? `${name.slice(0, 39)}…` : name; // fits the 270px box const churnBadge = c.churn ? `−${c.churn.removed}/+${c.churn.added}` : "unchanged context"; content += `<rect x="${x}" y="${y}" width="${boxW}" height="${h}" rx="8" fill="${cc.fill}" stroke="${cc.stroke}" stroke-width="1.5"/>`; - content += `<text x="${x + 12}" y="${y + 19}" font-size="11" font-weight="600" fill="${cc.label}">Community ${c.id + 1} — ${esc(kind)}</text>`; - content += `<text x="${x + 12}" y="${y + 33}" font-size="9" fill="#555">${c.size} vertices · ${esc(churnBadge)}</text>`; + content += `<text x="${x + 12}" y="${y + 19}" font-size="11" font-weight="600" fill="${cc.label}"><title>${esc(name)}</title>${esc(title)}</text>`; + content += `<text x="${x + 12}" y="${y + 33}" font-size="9" fill="#555">${esc(communityKindOf(c))} · ${c.size} vertices · ${esc(churnBadge)}</text>`; const files = c.files.slice(0, 4).map((f) => f.split("/").pop()); for (let j = 0; j < files.length; j++) { content += `<text x="${x + 12}" y="${y + 48 + j * 13}" font-size="8" fill="#333">${esc(files[j])}</text>`; @@ -502,7 +517,7 @@ function renderConfidence(confidence) { `; } -function renderAppendixStructural(checks, graphStats) { +function renderAppendixStructural(checks, graphStats, communityNames) { const hotspots = checks?.centrality?.hotspots || []; const blast = checks?.blastRadius?.functions || []; const stats = graphStats || {}; @@ -528,14 +543,15 @@ function renderAppendixStructural(checks, graphStats) { const communityRows = communities.map(c => { const labels = Object.entries(c.labelCounts || {}).sort((a, b) => b[1] - a[1]).map(([l, n]) => `${l}:${n}`).join(", "); const files = c.files.map(f => `<code>${esc(f)}</code>`).join("<br>"); - return `<tr><td>${c.id + 1}</td><td>${esc(c.role)}</td><td class="num">${c.size}</td><td>${esc(labels)}</td><td>${files}</td></tr>`; + const keys = (c.keyMembers || []).map(m => `<code>${esc(m)}</code>`).join(", "); + return `<tr><td>${esc(communityName(c, communityNames))}<br><code>community=${c.id}</code></td><td>${esc(c.role)}</td><td class="num">${c.size}</td><td>${esc(labels)}</td><td>${keys}</td><td>${files}</td></tr>`; }).join("\n "); const isolated = checks?.communities?.isolatedCount ?? 0; const communitiesHtml = communities.length === 0 ? "" : ` <h3>Community Membership</h3> - <p class="section-intro">Louvain communities over the code subgraph, largest first (modularity ${checks?.communities?.modularity}). The reading is in <a href="#communities">Thematic Communities</a>; this is the raw membership. Role carries the change mode and line churn (−removed/+added). ${isolated} vertices have no in-subgraph edge and are omitted; every clustered vertex carries a <code>community</code> property for direct querying.</p> + <p class="section-intro">Louvain communities over the code subgraph, largest first (modularity ${checks?.communities?.modularity}). The reading is in <a href="#communities">Thematic Communities</a>; this is the raw membership. Key members are changed first, then most-connected. Role carries the change mode and line churn (−removed/+added). ${isolated} vertices have no in-subgraph edge and are omitted; every clustered vertex carries the <code>community</code> property shown, for direct [...] <table class="gap-table"> - <thead><tr><th>#</th><th>Role</th><th>Vertices</th><th>Label mix</th><th>Files</th></tr></thead> + <thead><tr><th>Community</th><th>Role</th><th>Vertices</th><th>Label mix</th><th>Key members</th><th>Files</th></tr></thead> <tbody>\n ${communityRows}\n </tbody> </table>`; diff --git a/.skills/tinker-review/test/community-detection.test.js b/.skills/tinker-review/test/community-detection.test.js index ef147c21d4..e28502eb87 100644 --- a/.skills/tinker-review/test/community-detection.test.js +++ b/.skills/tinker-review/test/community-detection.test.js @@ -87,9 +87,45 @@ test("the assignments map covers every node so write-back can stamp them all", ( assert.equal(r.assignments.size, nodes.length, "one community assignment per vertex"); }); +test("assignments carry the reported community id and skip isolated vertices", () => { + const nodes = [ + fn("a", "big.java"), fn("b", "big.java"), fn("c", "big.java"), + fn("x", "small.java"), fn("y", "small.java"), + fn("orphan", "o.java"), + ]; + const edges = [edge("a", "b"), edge("b", "c"), edge("a", "c"), edge("x", "y")]; + const r = detectCommunities(nodes, edges, {}); + for (const c of r.communities) { + const stamped = [...r.assignments].filter(([, comm]) => comm === c.id).map(([id]) => id).sort(); + assert.equal(stamped.length, c.size, `community ${c.id} stamps exactly its members`); + } + assert.equal(r.assignments.get("a"), 0, "largest community is id 0"); + assert.ok(!r.assignments.has("orphan"), "isolated vertex is not stamped"); +}); + +test("key members list changed members first, then the most-connected", () => { + const nodes = [ + { ...fn("hub", "f.java") }, fn("leaf1", "f.java"), fn("leaf2", "f.java"), + { ...fn("edited", "f.java"), changed: true }, + { id: "file", label: "File", name: "f.java", filePath: "f.java" }, + ]; + const edges = [ + // a 4-clique where every hub edge pulls hardest, plus the file defining the hub + edge("hub", "leaf1", "EXTRACTED"), edge("hub", "leaf2", "EXTRACTED"), edge("hub", "edited", "EXTRACTED"), + edge("leaf1", "leaf2", "AMBIGUOUS"), edge("leaf1", "edited", "AMBIGUOUS"), edge("leaf2", "edited", "AMBIGUOUS"), + edge("file", "hub", "EXTRACTED"), + ]; + const r = detectCommunities(nodes, edges, {}); + assert.equal(r.communityCount, 1); + const km = r.communities[0].keyMembers; + assert.deepEqual(km.slice(0, 2), ["edited", "hub"], "changed first, then the hub"); + assert.ok(!km.includes("f.java"), "File vertices are left to the file list"); + assert.equal(r.communities[0].kind, "implementation"); +}); + const testNode = (id, file, changed = false) => ({ id, label: "Test", name: id, filePath: file, changed }); -test("an all-test partition reads as test scaffolding, not new functional groupings", () => { +test("an all-test partition reads as test code, not new functional groupings", () => { const nodes = [ testNode("t1", "AbstractIT.java"), testNode("t2", "AbstractIT.java"), testNode("t3", "AbstractIT.java"), testNode("u1", "AuditLogIT.java"), testNode("u2", "AuditLogIT.java"), testNode("u3", "AuditLogIT.java"), @@ -100,7 +136,7 @@ test("an all-test partition reads as test scaffolding, not new functional groupi edge("t3", "u1"), ]; const r = detectCommunities(nodes, edges, {}); - assert.ok(r.communities.every((c) => c.role.startsWith("test scaffolding")), "each community typed as test scaffolding"); + assert.ok(r.communities.every((c) => c.kind === "test code" && c.role.startsWith("test code")), "each community typed as test code"); assert.match(r.interpretation.headline, /modularity/); assert.ok( r.interpretation.reading.some((line) => /test infrastructure/i.test(line)),
