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 f4222773666e1c01af706a723adaf44232f6ad8e Author: Stephen Mallette <[email protected]> AuthorDate: Thu Oct 8 14:29:11 2026 -0400 Guide tinker-review walk with questions and code links Each Guided Walk item now poses the questions a reviewer should ask, with chips that open the code answering each one at the reviewed commit. Assisted-by: Claude Code:claude-opus-5-5 --- .skills/tinker-review/SKILL.md | 27 +++- .skills/tinker-review/references/interfaces.md | 50 ++++++- .../tinker-review/scripts/extraction/code-index.js | 99 +++++++++++++ .../scripts/extraction/tree-sitter.js | 2 + .../tinker-review/scripts/renderer/code-links.js | 124 ++++++++++++++++ .skills/tinker-review/scripts/renderer/render.js | 41 +++++- .../tinker-review/scripts/renderer/template.html | 6 + .skills/tinker-review/scripts/review.js | 7 + .skills/tinker-review/test/code-links.test.js | 161 +++++++++++++++++++++ 9 files changed, 504 insertions(+), 13 deletions(-) diff --git a/.skills/tinker-review/SKILL.md b/.skills/tinker-review/SKILL.md index eec76e278d..34564b25e5 100644 --- a/.skills/tinker-review/SKILL.md +++ b/.skills/tinker-review/SKILL.md @@ -245,11 +245,32 @@ produce a complete evidence-with-narrative JSON file. Write it to 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 +- `guidedWalk` — array of `{ title, badge, badgeText, intro, questions, refs }` objects, one + per area of the change, that **orient a reviewer who does not know the code**. Write for a + first-time reader: say what the area does in plain terms, not as a tour of class and method + names. + - `intro` — HTML, one or two plain sentences on what this part of the PR does and why. + - `questions` — `[{ text, refs }]`, the questions a reviewer should hold while reading this + part. Each `text` (HTML) asks something the reviewer can check or judge (*"Can two graphs + ever write to the same directory?"*, *"Is keeping index definitions outside the log + 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 + 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 + refs per question is plenty. + - `refs` on the item — optional "Also see" chips for code that belongs to the whole area. + + `render.js` prints any ref it cannot resolve; fix the `file`/`symbol` and re-render. An + item with only an HTML `body` still renders (older reports). - `findings` — array of `{ title, snippet, body }` objects, ordered most-severe-first (Interpret grades each blocking / high / low) - `openQuestions` — array of `{ title, body, meta }` objects - `functionalTest` — `{ plan, results: [{name, pass, output}], observations }` (if testing was done). - `plan` and `observations` are HTML and surface **themes and insights** — what + `plan` is HTML and `observations` is an array of HTML strings (one per insight); both and surface **themes and insights** — what families of behavior were exercised and what was learned — not a per-scenario list. `results` rows are **theme-level**: each `name` names a theme and the scenario labels it spans (e.g. `"Barrier family (A4–A12)"`), so the PASS/FAIL @@ -263,7 +284,7 @@ produce a complete evidence-with-narrative JSON file. Write it to subagent (see step 4); it is the source of truth the Functional Test section summarizes by label. -All `body` fields (and `functionalTest.plan`/`observations`, `appendixFunctional.environment`) +All `body`, `intro` and question `text` fields (and `functionalTest.plan`/`observations`, `appendixFunctional.environment`) are HTML — use `<code>`, `<strong>`, `<ul>`, `<p>` as needed. The raw-text fields noted above are the exception. The renderer handles all layout, CSS, and structure. diff --git a/.skills/tinker-review/references/interfaces.md b/.skills/tinker-review/references/interfaces.md index b3644c73cb..60043ef934 100644 --- a/.skills/tinker-review/references/interfaces.md +++ b/.skills/tinker-review/references/interfaces.md @@ -103,12 +103,15 @@ interface PopulationSummary { // ExternalsResult scripts/patterns/classify-externals.js // OrphanResult scripts/patterns/orphans.js // ArchitectureResult scripts/patterns/architecture.js +// CodeIndex scripts/extraction/code-index.js +// CodeRef scripts/renderer/code-links.js // === Evidence (evidence.json — what Phase 1 writes; the fields Interpret cites) === interface Evidence { meta: { pr: number; + headSha: string | null; // PR head commit reviewed; pins report code links title: string; domains: string[]; // e.g. ["general", "glv", "driver-server"] language: string; @@ -129,6 +132,7 @@ interface Evidence { }; discussions: DiscussionsResult; // jiras[], devList[], secondary[], proposals[], prComments{} changedFiles: string[]; + codeIndex: CodeIndex; // changed files → hunks + symbol line ranges (scripts/extraction/code-index.js) } // === ReportPackage (report.json — renderer input; Evidence + agent narrative) === @@ -141,11 +145,19 @@ interface ReportPackage extends Evidence { 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 }[]; + guidedWalk: { + title; badge; badgeText; // badge: "attention" | "info" | "safe" + intro: string; // HTML — what this area does, in plain terms + questions: { text: string; refs?: CodeRef[] }[]; // text: HTML + refs?: CodeRef[]; // optional item-level "Also see" + }[]; + // CodeRef = { file; symbol?; lines?: [number, number]; label? } — see + // scripts/renderer/code-links.js. The renderer resolves it against codeIndex; + // the agent never writes line numbers or URLs. Legacy { body } items still render. findings: { title; snippet; body }[]; openQuestions: { title; body; meta }[]; - functionalTest?: { plan; results: { name; pass; output }[]; observations }; - // plan/observations: HTML, theme-level. results rows are THEMES, each `name` + functionalTest?: { plan; results: { name; pass; output }[]; observations: string[] }; + // plan: HTML; observations: array of HTML strings; both theme-level. results rows are THEMES, each `name` // naming the scenario labels it spans — not one row per scenario. appendixFunctional?: { environment; testCode; fullOutput }; // environment: HTML. testCode/fullOutput: RAW TEXT (renderer wraps in @@ -311,9 +323,39 @@ export async function coverageGaps(g, params = {}) {} * Render an evidence package to a self-contained HTML page. * * @param {EvidencePackage} evidence - Structured evidence data + * @param {object} [options] + * @param {string[]} [options.warnings] - receives one entry per Guided Walk code ref that + * could not be resolved (the CLI prints them to stderr) * @returns {string} - Complete HTML document as a string */ -export function render(evidence) {} +export function render(evidence, options = {}) {} +``` + +### renderer/code-links.js + +```javascript +/** + * Resolve a {file, symbol?, lines?, label?} reference against evidence.codeIndex to a + * link: the file pinned at meta.headSha with the symbol's lines highlighted (the whole + * file when no symbol or lines). Without headSha it falls back to the PR "Files changed" + * anchor at the first changed lines, else the file in the diff. url is null (with + * the reason in title) when the reference does not resolve. + * + * @returns {{label, url: string|null, title, target: "diff"|"blob"|"file"|null}} + */ +export function resolveRef(ref, { pr, headSha, codeIndex }) {} +``` + +### extraction/code-index.js + +```javascript +/** + * Build evidence.codeIndex from the extraction and `git diff --unified=0`: for each + * changed file, its changed line ranges on the PR head side and the line range of every + * function and type in it. Called by Phase 1. + */ +export function buildCodeIndex({ changedFiles, extraction, diffText }) {} +export function parseHunks(diffText) {} ``` **Responsibilities:** diff --git a/.skills/tinker-review/scripts/extraction/code-index.js b/.skills/tinker-review/scripts/extraction/code-index.js new file mode 100644 index 0000000000..7e330490d2 --- /dev/null +++ b/.skills/tinker-review/scripts/extraction/code-index.js @@ -0,0 +1,99 @@ +/* + * 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. + */ + +/** + * @typedef {object} CodeSymbol + * @property {string} name - function or type name as extracted + * @property {"function"|"type"} kind + * @property {number} start - first line (1-based, PR head side) + * @property {number} end - last line (inclusive) + */ + +/** + * @typedef {object} CodeIndexFile + * @property {Array<[number, number]>} hunks - changed line ranges on the PR head side, ascending + * @property {CodeSymbol[]} symbols + */ + +/** + * @typedef {object} CodeIndex + * @property {Object<string, CodeIndexFile>} files - keyed by repo-relative path + */ + +/** + * Parse `git diff --unified=0` output into the changed line ranges on the new + * (PR head) side of each file. A pure deletion (`+N,0`) has no new-side lines, so it + * is recorded as `[N, N]`, the head line it follows, which GitHub shows as context + * beside the removed lines; that lets a symbol whose only change is a removal still + * link into the diff. A file deleted by the PR has no entry. + * + * @param {string} diffText + * @returns {Object<string, Array<[number, number]>>} + */ +export function parseHunks(diffText) { + const hunks = {}; + let current = null; + for (const line of (diffText || "").split("\n")) { + if (line.startsWith("+++ ")) { + const target = line.slice(4).trim(); + current = target === "/dev/null" ? null : target.replace(/^b\//, ""); + if (current && !hunks[current]) hunks[current] = []; + continue; + } + if (!current || !line.startsWith("@@")) continue; + const m = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (!m) continue; + const start = Number(m[1]); + const count = m[2] === undefined ? 1 : Number(m[2]); + if (count > 0) hunks[current].push([start, start + count - 1]); + else hunks[current].push([Math.max(start, 1), Math.max(start, 1)]); + } + for (const ranges of Object.values(hunks)) ranges.sort((a, b) => a[0] - b[0]); + return hunks; +} + +/** + * 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 + * hierarchy neighborhood parsed for context) are left out. + * + * @param {object} params + * @param {string[]} params.changedFiles + * @param {{functions: object[], types: object[]}} params.extraction + * @param {string} params.diffText - `git diff --unified=0` of the PR against its merge base + * @returns {CodeIndex} + */ +export function buildCodeIndex({ changedFiles, extraction, diffText }) { + const hunks = parseHunks(diffText); + const files = {}; + for (const path of changedFiles) { + if (!hunks[path]) continue; + files[path] = { hunks: hunks[path], symbols: [] }; + } + const add = (record, kind) => { + const entry = files[record.filePath]; + if (!entry || !record.linesStart || !record.linesEnd) return; + entry.symbols.push({ name: record.name, kind, start: record.linesStart, end: record.linesEnd }); + }; + for (const t of extraction.types || []) add(t, "type"); + for (const f of extraction.functions || []) add(f, "function"); + 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 ba4dc9bd48..c8f7d4bfca 100644 --- a/.skills/tinker-review/scripts/extraction/tree-sitter.js +++ b/.skills/tinker-review/scripts/extraction/tree-sitter.js @@ -476,6 +476,8 @@ function extractTypesFromTree(tree, filePath, language) { language, supertypes, signature: typeSignature(node, language, kind, visibility, supertypes), + linesStart: node.startPosition.row + 1, + linesEnd: node.endPosition.row + 1, rawHash: rawHash(node), normHash: normHash(node), }); diff --git a/.skills/tinker-review/scripts/renderer/code-links.js b/.skills/tinker-review/scripts/renderer/code-links.js new file mode 100644 index 0000000000..2a1e9001f3 --- /dev/null +++ b/.skills/tinker-review/scripts/renderer/code-links.js @@ -0,0 +1,124 @@ +/* + * 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. + */ + +import { createHash } from "node:crypto"; + +const REPO = "https://github.com/apache/tinkerpop";; + +/** + * A reference from the report narrative to code in the PR. The agent names the + * code; the renderer finds the lines, so no line number or URL is ever written by + * hand. + * + * @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 + * (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 + */ + +/** + * @typedef {object} ResolvedRef + * @property {string} label + * @property {string|null} url - null when the reference could not be resolved + * @property {string} title - tooltip: path and lines, or why it did not resolve + * @property {"diff"|"blob"|"file"|null} target - which kind of link was produced + */ + +/** + * The anchor GitHub gives a file in a pull request's "Files changed" view. + */ +export function diffAnchor(path) { + return "diff-" + createHash("sha256").update(path).digest("hex"); +} + +function findFile(codeIndex, file) { + const files = (codeIndex && codeIndex.files) || {}; + if (files[file]) return { path: file }; + const matches = Object.keys(files).filter((p) => p.endsWith("/" + file)); + if (matches.length === 1) return { path: matches[0] }; + return { error: matches.length === 0 ? `no changed file matches "${file}"` : `"${file}" matches ${matches.length} changed files` }; +} + +function findSymbol(entry, symbol) { + const dot = symbol.lastIndexOf("."); + if (dot > 0) { + const owner = symbol.slice(0, dot); + 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; + } + return null; + } + // a bare name means the type when one exists (a constructor shares its class's name), else the first function + return entry.symbols.find((s) => s.kind === "type" && s.name === symbol) + || entry.symbols.find((s) => s.kind === "function" && s.name === symbol) + || null; +} + +/** + * Resolve a reference to a link. With the reviewed commit recorded, every reference + * links to the file pinned at that commit, highlighting the symbol's lines (or the + * whole file when there is no symbol or lines). The PR's "Files changed" view is not + * used because GitHub loads most files there only after the page opens, so its line + * anchors rarely land. Without a recorded commit, links fall back to that view: at the + * first changed lines inside the range, else the file. + * + * @param {CodeRef} ref + * @param {{pr: number|string, headSha?: string|null, codeIndex?: object}} ctx + * @returns {ResolvedRef} + */ +export function resolveRef(ref, ctx) { + const label = ref.label || ref.symbol || (ref.file || "").split("/").pop() || "code"; + const fail = (why) => ({ label, url: null, title: `unresolved: ${why}`, target: null }); + if (!ref.file) return fail("reference has no file"); + + const found = findFile(ctx.codeIndex, ref.file); + if (found.error) return fail(found.error); + const path = found.path; + const entry = ctx.codeIndex.files[path]; + const filesUrl = `${REPO}/pull/${ctx.pr}/files#${diffAnchor(path)}`; + const blobUrl = ctx.headSha ? `${REPO}/blob/${ctx.headSha}/${path}` : null; + + let range = null; + if (Array.isArray(ref.lines) && ref.lines.length === 2) { + range = [Number(ref.lines[0]), Number(ref.lines[1])]; + } else if (ref.symbol) { + const sym = findSymbol(entry, ref.symbol); + if (!sym) return fail(`no symbol "${ref.symbol}" in ${path}`); + range = [sym.start, sym.end]; + } + if (!range) return { label, url: blobUrl || filesUrl, title: path, target: "file" }; + + const [start, end] = range; + const hunk = entry.hunks.find(([hs, he]) => hs <= end && he >= start); + const changed = hunk ? `changed at ${Math.max(start, hunk[0])}` : "unchanged"; + if (blobUrl) { + return { label, url: `${blobUrl}#L${start}-L${end}`, title: `${path}:${start}-${end} (${changed})`, target: "blob" }; + } + if (hunk) { + const a = Math.max(start, hunk[0]); + const b = Math.min(end, hunk[1]); + return { label, url: `${filesUrl}R${a}${b > a ? `-R${b}` : ""}`, title: `${path}:${a}-${b} (changed lines)`, target: "diff" }; + } + return { label, url: filesUrl, title: `${path}:${start}-${end} (no commit recorded; linking the file)`, target: "file" }; +} diff --git a/.skills/tinker-review/scripts/renderer/render.js b/.skills/tinker-review/scripts/renderer/render.js index cb221255ae..aeafc0ea27 100644 --- a/.skills/tinker-review/scripts/renderer/render.js +++ b/.skills/tinker-review/scripts/renderer/render.js @@ -20,6 +20,7 @@ import { readFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; +import { resolveRef } from "./code-links.js"; const __dirname = dirname(fileURLToPath(import.meta.url)); const TEMPLATE = readFileSync(join(__dirname, "template.html"), "utf-8"); @@ -83,7 +84,10 @@ function stripCodeWrapper(str) { * Input contract — narrative fields (from agent/synthesize.js): * summary: "HTML string" * clusters: { svg: "<svg>...</svg>", assessment: "HTML string" } - * guidedWalk: [{ title, badge: "attention|info|safe", badgeText, body: "HTML" }] + * guidedWalk: [{ title, badge: "attention|info|safe", badgeText, + * intro: "HTML", questions: [{ text: "HTML", refs: [CodeRef] }], refs: [CodeRef] }] + * (CodeRef = { file, symbol?, lines?, label? } — see code-links.js; the renderer resolves it + * against evidence.codeIndex into a link. A legacy item with only body: "HTML" still renders.) * functionalTest: { plan: "HTML", results: [{name, pass, output}], observations: ["HTML"] } * (results rows are THEME-level, each naming the scenario labels it covers) * findings: [{ title, snippet: "code", body: "HTML" }] @@ -95,7 +99,8 @@ function notProvided(sectionId, title) { return `<section id="${sectionId}">\n <h2>${esc(title)}</h2>\n <p class="section-intro" style="color: var(--danger);">Section not provided.</p>\n</section>`; } -export function render(evidence) { +export function render(evidence, options = {}) { + const warnings = options.warnings || []; const { meta, graphStats, checks, discussions, summary, clusters, communityAssessment, communityNames, guidedWalk, functionalTest, findings, openQuestions, appendixFunctional } = evidence; @@ -106,7 +111,7 @@ export function render(evidence) { 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, communityNames)); - parts.push(guidedWalk && guidedWalk.length > 0 ? renderGuidedWalk(guidedWalk) : notProvided("guided-walk", "Guided Walk")); + parts.push(guidedWalk && guidedWalk.length > 0 ? renderGuidedWalk(guidedWalk, { pr: meta.pr, headSha: meta.headSha, codeIndex: evidence.codeIndex }, warnings) : 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")); @@ -412,14 +417,36 @@ function generateCommunitySvg(communityData, names) { </svg>`; } -function renderGuidedWalk(walk) { +function renderCodeRefs(refs, ctx, warnings, where, cls = "code-refs") { + if (!Array.isArray(refs) || refs.length === 0) return ""; + const chips = refs.map((ref) => { + const r = resolveRef(ref, ctx); + if (!r.url) { + warnings.push(`${where}: ${r.title} (${JSON.stringify(ref)})`); + return `<span class="code-ref code-ref-unresolved" title="${esc(r.title)}">${esc(r.label)}</span>`; + } + return `<a class="code-ref code-ref-${r.target}" href="${esc(r.url)}" target="_blank" rel="noopener" title="${esc(r.title)}">${esc(r.label)}</a>`; + }).join(" "); + return `<div class="${cls}">${chips}</div>`; +} + +function renderGuidedWalk(walk, ctx, warnings) { const steps = walk.map((step, i) => { const cls = step.badge === "attention" ? "card-attention" : step.badge === "safe" ? "card-safe" : "card-info"; const badgeCls = step.badge === "attention" ? "badge-attention" : step.badge === "safe" ? "badge-safe" : ""; const badge = step.badgeText ? ` <span class="badge ${badgeCls}">${esc(step.badgeText)}</span>` : ""; + const where = `guidedWalk[${i}]`; + const intro = step.intro || step.body || ""; + const questions = Array.isArray(step.questions) && step.questions.length > 0 + ? `<ul class="walk-questions">\n ${step.questions.map((q, j) => + `<li>${q.text}${renderCodeRefs(q.refs, ctx, warnings, `${where}.questions[${j}]`)}</li>`).join("\n ")}\n </ul>` + : ""; + const alsoSee = renderCodeRefs(step.refs, ctx, warnings, `${where}.refs`, "code-refs walk-refs"); return `<div class="card ${cls}"> <h3><span class="walk-number">${i + 1}</span> ${step.title}${badge}</h3> - ${step.body} + ${intro} + ${questions} + ${alsoSee} </div>`; }).join("\n "); return `<section id="guided-walk">\n <h2>Guided Walk</h2>\n ${steps}\n</section>`; @@ -622,7 +649,9 @@ if (process.argv[1] && basename(process.argv[1]) === "render.js") { const outputPath = process.argv[3] || inputPath.replace(/\.json$/, ".html"); const { readFileSync, writeFileSync } = await import("node:fs"); const evidence = JSON.parse(readFileSync(inputPath, "utf-8")); - const html = render(evidence); + const warnings = []; + const html = render(evidence, { warnings }); writeFileSync(outputPath, html, "utf-8"); console.log(`Report written: ${outputPath}`); + for (const w of warnings) console.error(`[render] unresolved code ref — ${w}`); } diff --git a/.skills/tinker-review/scripts/renderer/template.html b/.skills/tinker-review/scripts/renderer/template.html index 53901292c2..0e2a6da0e6 100644 --- a/.skills/tinker-review/scripts/renderer/template.html +++ b/.skills/tinker-review/scripts/renderer/template.html @@ -51,6 +51,12 @@ code { background: var(--bg-alt); padding: 0.1rem 0.4rem; border-radius: 3px; fo pre { background: var(--bg-alt); border: 1px solid var(--border); border-radius: var(--radius); padding: 0.75rem; overflow-x: auto; font-size: 0.8rem; line-height: 1.5; margin: 0.75rem 0; } pre code { background: none; padding: 0; } .walk-number { display: inline-flex; align-items: center; justify-content: center; width: 1.75rem; height: 1.75rem; border-radius: 50%; background: var(--primary); color: #fff; font-size: 0.8rem; font-weight: 700; margin-right: 0.5rem; } +.walk-questions { margin: 0.5rem 0 0.25rem 1.1rem; padding: 0; } .walk-questions li { margin-bottom: 0.6rem; } +.code-refs { display: flex; flex-wrap: wrap; gap: 0.35rem; margin-top: 0.3rem; } +.walk-refs { margin-top: 0.6rem; } .walk-refs::before { content: "Also see"; font-size: 0.75rem; color: var(--text-muted); align-self: center; margin-right: 0.15rem; } +.code-ref { display: inline-block; padding: 0.1rem 0.5rem; border: 1px solid var(--primary); border-radius: 3px; font-family: ui-monospace, SFMono-Regular, Menlo, monospace; font-size: 0.75rem; color: var(--primary); text-decoration: none; } +.code-ref::after { content: " \2197"; } .code-ref:hover { background: var(--primary); color: #fff; } +.code-ref-unresolved { border-color: var(--border); color: var(--text-muted); } .code-ref-unresolved::after { content: " ?"; } .finding { border-left: 3px solid var(--warning); } .finding-title { font-weight: 600; margin-bottom: 0.25rem; } .test-pass { color: var(--success); font-weight: 600; } .test-fail { color: var(--danger); font-weight: 600; } .test-grid { display: grid; grid-template-columns: auto 1fr auto; gap: 0.25rem 0.75rem; font-size: 0.875rem; align-items: baseline; } diff --git a/.skills/tinker-review/scripts/review.js b/.skills/tinker-review/scripts/review.js index 3aa600785a..4ac6f24c4c 100644 --- a/.skills/tinker-review/scripts/review.js +++ b/.skills/tinker-review/scripts/review.js @@ -39,6 +39,7 @@ import { confidenceAudit } from "./patterns/confidence-audit.js"; import { classifyExternals } from "./patterns/classify-externals.js"; import { findRemovalRefs } from "./patterns/removal-refs.js"; import { orphans } from "./patterns/orphans.js"; +import { buildCodeIndex } from "./extraction/code-index.js"; import { createPrDiscussion } from "./enrichment/api.js"; import { discoverDiscussions } from "./discovery/discussions.js"; @@ -441,9 +442,14 @@ export async function phase1(session) { const architectureResult = await architecture(g, { clusterResult, changedOnly: true }); log(` architecture: ${architectureResult.nodes.length} nodes, ${architectureResult.edges.length} edges`); + // pin code links in the report to the exact commit reviewed, so they don't drift when the PR is pushed again + const { stdout: headShaOut } = await exec("git", ["rev-parse", prBranch], { cwd: repoPath }).catch(() => ({ stdout: "" })); + const codeIndex = buildCodeIndex({ changedFiles, extraction, diffText }); + const evidence = { meta: { pr, + headSha: headShaOut.trim() || null, title: prTitle.trim(), domains, language, @@ -467,6 +473,7 @@ export async function phase1(session) { }, discussions, changedFiles, + codeIndex, }; const jsonPath = join(workDir, "evidence.json"); diff --git a/.skills/tinker-review/test/code-links.test.js b/.skills/tinker-review/test/code-links.test.js new file mode 100644 index 0000000000..6993790562 --- /dev/null +++ b/.skills/tinker-review/test/code-links.test.js @@ -0,0 +1,161 @@ +/* + * 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. + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { createHash } from "node:crypto"; + +import { parseHunks, buildCodeIndex } from "../scripts/extraction/code-index.js"; +import { resolveRef, diffAnchor } from "../scripts/renderer/code-links.js"; +import { render } from "../scripts/renderer/render.js"; + +const TX = "tinkergraph-gremlin/src/main/java/org/x/TinkerTransaction.java"; +const GRAPH = "tinkergraph-gremlin/src/main/java/org/x/TinkerStorageGraph.java"; +const OTHER_GRAPH = "gremlin-core/src/main/java/org/y/TinkerStorageGraph.java"; + +const DIFF = [ + `diff --git a/${TX} b/${TX}`, + `--- a/${TX}`, + `+++ b/${TX}`, + "@@ -174,3 +181,40 @@ class X {", + "@@ -300 +340 @@", + "@@ -400,5 +379,0 @@", + `diff --git a/${GRAPH} b/${GRAPH}`, + "--- /dev/null", + `+++ b/${GRAPH}`, + "@@ -0,0 +1,120 @@", + "diff --git a/gone.java b/gone.java", + "--- a/gone.java", + "+++ /dev/null", + "@@ -1,10 +0,0 @@", +].join("\n"); + +const extraction = { + types: [ + { name: "TinkerTransaction", filePath: TX, linesStart: 30, linesEnd: 400 }, + { name: "TinkerStorageGraph", filePath: GRAPH, linesStart: 10, linesEnd: 120 }, + { name: "Neighbor", filePath: "not/changed/Neighbor.java", linesStart: 1, linesEnd: 9 }, + ], + functions: [ + { name: "doCommit", filePath: TX, linesStart: 147, linesEnd: 245 }, + { name: "doRollback", filePath: TX, linesStart: 290, linesEnd: 320 }, + { name: "TinkerStorageGraph", filePath: GRAPH, linesStart: 40, linesEnd: 60 }, + ], +}; + +const codeIndex = buildCodeIndex({ changedFiles: [TX, GRAPH, "gone.java"], extraction, diffText: DIFF }); +const ctx = { pr: 3639, headSha: "abc123", codeIndex }; + +test("parseHunks keeps new-side ranges, marks pure deletions at the line they follow, and drops deleted files", () => { + const h = parseHunks(DIFF); + assert.deepEqual(h[TX], [[181, 220], [340, 340], [379, 379]]); + assert.deepEqual(h[GRAPH], [[1, 120]]); + assert.equal(h["gone.java"], undefined); +}); + +test("buildCodeIndex indexes only changed files, with sorted symbols", () => { + assert.deepEqual(Object.keys(codeIndex.files).sort(), [GRAPH, TX].sort()); + assert.deepEqual(codeIndex.files[TX].symbols.map((s) => s.name), ["TinkerTransaction", "doCommit", "doRollback"]); + assert.equal(codeIndex.files[TX].symbols[0].kind, "type"); +}); + +test("diffAnchor matches GitHub's sha256-of-path anchor", () => { + assert.equal(diffAnchor(TX), "diff-" + createHash("sha256").update(TX).digest("hex")); +}); + +test("a symbol links to the file pinned at the reviewed commit with its lines highlighted", () => { + const r = resolveRef({ file: "TinkerTransaction.java", symbol: "doCommit" }, ctx); + assert.equal(r.target, "blob"); + assert.equal(r.url, `https://github.com/apache/tinkerpop/blob/abc123/${TX}#L147-L245`); + assert.equal(r.label, "doCommit"); + assert.match(r.title, /changed at 181/); + const unchanged = resolveRef({ file: TX, symbol: "doRollback", label: "rollback" }, ctx); + assert.equal(unchanged.url, `https://github.com/apache/tinkerpop/blob/abc123/${TX}#L290-L320`); + assert.equal(unchanged.label, "rollback"); + assert.match(unchanged.title, /unchanged/); +}); + +test("without a recorded head commit links fall back to the PR diff", () => { + const noSha = { ...ctx, headSha: null }; + const changed = resolveRef({ file: TX, symbol: "doCommit" }, noSha); + assert.equal(changed.target, "diff"); + assert.equal(changed.url, `https://github.com/apache/tinkerpop/pull/3639/files#${diffAnchor(TX)}R181-R220`); + const unchanged = resolveRef({ file: TX, symbol: "doRollback" }, noSha); + assert.equal(unchanged.target, "file"); + assert.equal(unchanged.url, `https://github.com/apache/tinkerpop/pull/3639/files#${diffAnchor(TX)}`); +}); + +test("a bare name means the type; Type.member picks the member inside it", () => { + const type = resolveRef({ file: GRAPH, symbol: "TinkerStorageGraph" }, ctx); + assert.match(type.url, /#L10-L120$/); + const ctor = resolveRef({ file: GRAPH, symbol: "TinkerStorageGraph.TinkerStorageGraph" }, ctx); + assert.match(ctor.url, /#L40-L60$/); +}); + +test("a symbol whose only change is a removal links into the diff at the removal when no commit is recorded", () => { + const index = buildCodeIndex({ + changedFiles: ["A.java"], + extraction: { types: [], functions: [{ name: "ctor", filePath: "A.java", linesStart: 89, linesEnd: 107 }] }, + diffText: "+++ b/A.java\n@@ -96,9 +95,0 @@", + }); + const r = resolveRef({ file: "A.java", symbol: "ctor" }, { pr: 1, headSha: null, codeIndex: index }); + assert.equal(r.target, "diff"); + assert.match(r.url, /R95$/); +}); + +test("explicit lines and file-only references resolve", () => { + assert.match(resolveRef({ file: TX, lines: [340, 345] }, ctx).url, /#L340-L345$/); + const whole = resolveRef({ file: GRAPH }, ctx); + assert.equal(whole.target, "file"); + assert.equal(whole.url, `https://github.com/apache/tinkerpop/blob/abc123/${GRAPH}`); + assert.equal(whole.label, "TinkerStorageGraph.java"); +}); + +test("unknown, ambiguous and symbol-less references come back unresolved with a reason", () => { + assert.match(resolveRef({ file: "Nope.java" }, ctx).title, /no changed file/); + assert.match(resolveRef({ file: TX, symbol: "missing" }, ctx).title, /no symbol "missing"/); + const ambiguous = { pr: 1, codeIndex: { files: { [GRAPH]: { hunks: [], symbols: [] }, [OTHER_GRAPH]: { hunks: [], symbols: [] } } } }; + const r = resolveRef({ file: "TinkerStorageGraph.java" }, ambiguous); + assert.equal(r.url, null); + assert.match(r.title, /matches 2 changed files/); +}); + +test("the guided walk renders questions with chips and reports unresolved refs", () => { + const warnings = []; + const html = render({ + meta: { pr: 3639, headSha: "abc123", title: "t", domains: [], timestamp: "now" }, + codeIndex, + guidedWalk: [ + { + title: "Commit path", badge: "attention", badgeText: "defect", + intro: "<p>What a commit does now.</p>", + questions: [{ text: "Is the index restored?", refs: [{ file: TX, symbol: "doCommit" }, { file: TX, symbol: "ghost" }] }], + refs: [{ file: GRAPH }], + }, + { title: "Legacy", badge: "info", body: "<p>Old-style body still renders.</p>" }, + ], + }, { warnings }); + assert.match(html, /<ul class="walk-questions">/); + assert.match(html, /class="code-ref code-ref-blob" href="https:\/\/github\.com\/apache\/tinkerpop\/blob\/abc123\/[^"]*TinkerTransaction\.java#L147-L245"/); + assert.match(html, /class="code-ref code-ref-unresolved"[^>]*>ghost</); + assert.match(html, /class="code-refs walk-refs"/); + assert.match(html, /Old-style body still renders/); + assert.equal(warnings.length, 1); + assert.match(warnings[0], /guidedWalk\[0\]\.questions\[0\]: unresolved: no symbol "ghost"/); +});
