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 fc7533165d0b2d9395e89533318513d895e7f1a3 Author: Stephen Mallette <[email protected]> AuthorDate: Fri Oct 9 15:44:18 2026 -0400 Let tinker-review code refs name fields Field declarations are indexed, so a ref such as Type.field links to the field. Assisted-by: Claude Code:claude-opus-5-5 --- .skills/tinker-review/SKILL.md | 7 +-- .../tinker-review/scripts/extraction/code-index.js | 9 ++-- .../scripts/extraction/tree-sitter.js | 51 +++++++++++++++++++++- .../tinker-review/scripts/renderer/code-links.js | 11 +++-- .skills/tinker-review/test/code-links.test.js | 16 +++++++ .skills/tinker-review/test/extraction.test.js | 15 +++++++ 6 files changed, 96 insertions(+), 13 deletions(-) diff --git a/.skills/tinker-review/SKILL.md b/.skills/tinker-review/SKILL.md index 87e65c27ce..05556edbeb 100644 --- a/.skills/tinker-review/SKILL.md +++ b/.skills/tinker-review/SKILL.md @@ -267,9 +267,10 @@ produce a complete evidence-with-narrative JSON file. Write it to reasonable?"*) and may cite what you found (a finding, a functional-test label). Invite judgment of trade-offs rather than asserting them. - `refs` on a question — the code that answers it, as `{ file, symbol?, lines?, label? }`: - `file` is a repo path or unique suffix (`"TinkerTransaction.java"`); `symbol` a function - or type name, or `"Type.member"` for a member (`"TinkerStorageGraph.TinkerStorageGraph"` - is the constructor; bare `"TinkerStorageGraph"` the class); `label` the chip text. Name + `file` is a repo path or unique suffix (`"TinkerTransaction.java"`); `symbol` a function, + type or field name, or `"Type.member"` for a method or field of that type + (`"TinkerStorageGraph.TinkerStorageGraph"` is the constructor; bare `"TinkerStorageGraph"` + the class); `label` the chip text. Name the code — **never write line numbers or URLs**: the renderer resolves each ref against `evidence.codeIndex` and links to the file pinned at the reviewed commit with the symbol's lines highlighted. Two or three diff --git a/.skills/tinker-review/scripts/extraction/code-index.js b/.skills/tinker-review/scripts/extraction/code-index.js index 7e330490d2..fd1d296598 100644 --- a/.skills/tinker-review/scripts/extraction/code-index.js +++ b/.skills/tinker-review/scripts/extraction/code-index.js @@ -19,8 +19,8 @@ /** * @typedef {object} CodeSymbol - * @property {string} name - function or type name as extracted - * @property {"function"|"type"} kind + * @property {string} name - function, type or field name as extracted + * @property {"function"|"type"|"field"} kind * @property {number} start - first line (1-based, PR head side) * @property {number} end - last line (inclusive) */ @@ -71,12 +71,12 @@ export function parseHunks(diffText) { /** * Build the code index the renderer uses to turn a `{file, symbol}` reference into * a link: for every changed file, its changed hunks and the line range of each - * function and type extracted from it. Files outside `changedFiles` (such as the + * function, type and field extracted from it. Files outside `changedFiles` (such as the * hierarchy neighborhood parsed for context) are left out. * * @param {object} params * @param {string[]} params.changedFiles - * @param {{functions: object[], types: object[]}} params.extraction + * @param {{functions: object[], types: object[], fields?: object[]}} params.extraction * @param {string} params.diffText - `git diff --unified=0` of the PR against its merge base * @returns {CodeIndex} */ @@ -94,6 +94,7 @@ export function buildCodeIndex({ changedFiles, extraction, diffText }) { }; for (const t of extraction.types || []) add(t, "type"); for (const f of extraction.functions || []) add(f, "function"); + for (const f of extraction.fields || []) add(f, "field"); for (const entry of Object.values(files)) entry.symbols.sort((a, b) => a.start - b.start); return { files }; } diff --git a/.skills/tinker-review/scripts/extraction/tree-sitter.js b/.skills/tinker-review/scripts/extraction/tree-sitter.js index fddd85cc60..24a7247281 100644 --- a/.skills/tinker-review/scripts/extraction/tree-sitter.js +++ b/.skills/tinker-review/scripts/extraction/tree-sitter.js @@ -492,6 +492,51 @@ function extractTypesFromTree(tree, filePath, language) { return types; } +// Field declarations, so a report ref can name a field (`Type.field`). They feed +// only the code index; the graph has no Field vertex. A declaration such as +// `int a, b;` yields one entry per name, each spanning the whole declaration. +const FIELD_NODES = { + java: "field_declaration", + csharp: "field_declaration", + go: "field_declaration", + javascript: "field_definition", +}; + +function fieldNames(node) { + // Java puts the declarators directly under the declaration, C# one level down. + const holder = childByType(node, "variable_declaration") || node; + const declarators = childrenByType(holder, "variable_declarator"); + if (declarators.length > 0) { + return declarators + .map((d) => (d.childForFieldName("name") || childByType(d, "identifier"))?.text) + .filter(Boolean); + } + // Go struct fields carry field_identifiers; JS class fields a property name. + const names = childrenByType(node, "field_identifier").map((n) => n.text); + const prop = node.childForFieldName("property") || childByType(node, "property_identifier"); + if (prop) names.push(prop.text); + return names; +} + +function extractFieldsFromTree(tree, filePath, language) { + const nodeType = FIELD_NODES[language]; + if (!nodeType) return []; + const fields = []; + (function visit(node) { + if (node.type === nodeType) { + for (const name of fieldNames(node)) { + fields.push({ + name, filePath, language, + linesStart: node.startPosition.row + 1, + linesEnd: node.endPosition.row + 1, + }); + } + } + for (let i = 0; i < node.childCount; i++) visit(node.child(i)); + })(tree.rootNode); + return fields; +} + // Which type declares each function, for `declares` (Type -> Function) edges. // Walks the tree carrying the innermost enclosing type so a method maps to the // class/interface whose body it sits in (both keyed within the same file). @@ -778,6 +823,7 @@ function parseSourceFile(parser, file, language, result, baseContents = {}) { result.functions.push(...fileFunctions); result.types.push(...fileTypes); result.declares.push(...extractDeclaresFromTree(tree, file.path, language)); + result.fields.push(...extractFieldsFromTree(tree, file.path, language)); const fileCalls = extractCallsFromTree(tree, file.path, language, fileFunctions); result.calls.push(...fileCalls); @@ -944,6 +990,7 @@ export async function extract(directory, language, options = {}) { imports: [], tests: [], declares: [], + fields: [], }; for (const file of sourceFiles) { @@ -987,7 +1034,7 @@ export async function extract(directory, language, options = {}) { export async function extractMulti(directory, languages, options = {}) { const merged = { languages: [], - files: [], functions: [], types: [], calls: [], imports: [], tests: [], declares: [], + files: [], functions: [], types: [], calls: [], imports: [], tests: [], declares: [], fields: [], hierarchyNeighborhood: { files: 0, truncated: false }, }; @@ -995,7 +1042,7 @@ export async function extractMulti(directory, languages, options = {}) { if (!LANGUAGE_EXTENSIONS[language]) continue; const one = await extract(directory, language, options); merged.languages.push(language); - for (const key of ["files", "functions", "types", "calls", "imports", "tests", "declares"]) { + for (const key of ["files", "functions", "types", "calls", "imports", "tests", "declares", "fields"]) { // Not push(...one[key]): spreading a large array exceeds the call stack. for (const item of one[key]) merged[key].push(item); } diff --git a/.skills/tinker-review/scripts/renderer/code-links.js b/.skills/tinker-review/scripts/renderer/code-links.js index 2a1e9001f3..6c7519fb19 100644 --- a/.skills/tinker-review/scripts/renderer/code-links.js +++ b/.skills/tinker-review/scripts/renderer/code-links.js @@ -28,7 +28,7 @@ const REPO = "https://github.com/apache/tinkerpop";; * * @typedef {object} CodeRef * @property {string} file - repo-relative path, or a unique suffix of one (e.g. "TinkerTransaction.java") - * @property {string} [symbol] - function or type name; "Type.member" picks a member of that type + * @property {string} [symbol] - function, type or field name; "Type.member" picks a method or field of that type * (so "TinkerStorageGraph.TinkerStorageGraph" is the constructor, bare "TinkerStorageGraph" the class) * @property {[number, number]} [lines] - explicit line range on the PR head side, used instead of symbol * @property {string} [label] - chip text; defaults to the symbol, else the file name @@ -64,14 +64,17 @@ function findSymbol(entry, symbol) { const member = symbol.slice(dot + 1); const types = entry.symbols.filter((s) => s.kind === "type" && s.name === owner); for (const t of types) { - const fn = entry.symbols.find((s) => s.kind === "function" && s.name === member && s.start >= t.start && s.end <= t.end); - if (fn) return fn; + const inside = (s) => s.name === member && s.start >= t.start && s.end <= t.end; + const found = entry.symbols.find((s) => s.kind === "function" && inside(s)) + || entry.symbols.find((s) => s.kind === "field" && inside(s)); + if (found) return found; } return null; } - // a bare name means the type when one exists (a constructor shares its class's name), else the first function + // a bare name means the type when one exists (a constructor shares its class's name), else the first function, else a field return entry.symbols.find((s) => s.kind === "type" && s.name === symbol) || entry.symbols.find((s) => s.kind === "function" && s.name === symbol) + || entry.symbols.find((s) => s.kind === "field" && s.name === symbol) || null; } diff --git a/.skills/tinker-review/test/code-links.test.js b/.skills/tinker-review/test/code-links.test.js index 6993790562..05272383d7 100644 --- a/.skills/tinker-review/test/code-links.test.js +++ b/.skills/tinker-review/test/code-links.test.js @@ -159,3 +159,19 @@ test("the guided walk renders questions with chips and reports unresolved refs", assert.equal(warnings.length, 1); assert.match(warnings[0], /guidedWalk\[0\]\.questions\[0\]: unresolved: no symbol "ghost"/); }); + +test("a Type.field ref resolves to the field declaration", () => { + const index = buildCodeIndex({ + changedFiles: [TX], + extraction: { + types: [{ name: "TinkerTransaction", filePath: TX, linesStart: 30, linesEnd: 400 }], + functions: [], + fields: [{ name: "storageCommitLock", filePath: TX, linesStart: 42, linesEnd: 42 }], + }, + diffText: DIFF, + }); + const ctx = { pr: 3639, headSha: "abc123", codeIndex: index }; + const r = resolveRef({ file: TX, symbol: "TinkerTransaction.storageCommitLock" }, ctx); + assert.match(r.url, /#L42-L42$/); + assert.match(resolveRef({ file: TX, symbol: "storageCommitLock" }, ctx).url, /#L42-L42$/, "bare field name"); +}); diff --git a/.skills/tinker-review/test/extraction.test.js b/.skills/tinker-review/test/extraction.test.js index cde766ba73..ca8c1754aa 100644 --- a/.skills/tinker-review/test/extraction.test.js +++ b/.skills/tinker-review/test/extraction.test.js @@ -171,3 +171,18 @@ test("expandHierarchy:false keeps extraction to changed files only", async () => await rm(dir, { recursive: true, force: true }); } }); + +test("Java fields are extracted per name, spanning their declaration", async () => { + await withJavaSources( + { + "Graph.java": + "package x;\npublic class Graph {\n private final Object storageCommitLock = new Object();\n int a,\n b;\n void run() { int local = 1; }\n}\n", + }, + (r) => { + const byName = Object.fromEntries(r.fields.map((f) => [f.name, f])); + assert.deepEqual(Object.keys(byName).sort(), ["a", "b", "storageCommitLock"], "locals are not fields"); + assert.deepEqual([byName.storageCommitLock.linesStart, byName.storageCommitLock.linesEnd], [3, 3]); + assert.deepEqual([byName.b.linesStart, byName.b.linesEnd], [4, 5]); + }, + ); +});
