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 4bf44b7802cd14410479e4dd05fd04c59de897fa Author: Stephen Mallette <[email protected]> AuthorDate: Fri Jul 3 17:19:22 2026 -0400 tinker-review: move removal-ref discovery into Phase 1 Finding references to removed code is mechanical — grep the worktree for each deleted code symbol — so it belongs in the deterministic phase, mirroring classifyExternals. A new removal-refs pass auto-creates `references` edges (checks.removalRefs); addReference becomes the manual escape hatch for the non-code cases (config strings, ports) the pass can't see. Shared symbol/edge helpers move to graph/references.js so both paths write the edge one way. The boundary audit that prompted this also found mapStep and grammar-rule population do NOT qualify: step-name matching is a weak signal that needs the judgment the GLV playbook describes, so those stay Phase-2 commands. DESIGN.md records the distinction. Assisted-by: Claude Code:claude-opus-4-8 --- .skills/tinker-review/DESIGN.md | 10 +- .skills/tinker-review/playbooks/removal.md | 34 ++++--- .skills/tinker-review/scripts/enrichment/api.js | 47 ++------- .skills/tinker-review/scripts/graph/references.js | 85 +++++++++++++++++ .../tinker-review/scripts/patterns/removal-refs.js | 106 +++++++++++++++++++++ .skills/tinker-review/scripts/review.js | 6 ++ 6 files changed, 229 insertions(+), 59 deletions(-) diff --git a/.skills/tinker-review/DESIGN.md b/.skills/tinker-review/DESIGN.md index de4cde0700..4adda0eb51 100644 --- a/.skills/tinker-review/DESIGN.md +++ b/.skills/tinker-review/DESIGN.md @@ -35,7 +35,7 @@ has already exited. |------|------| | `scripts/review.js` | Phase 1 orchestrator — `setup` / `phase1` / `teardown` | | `scripts/extraction/tree-sitter.js` | source → structural extraction | -| `scripts/graph/*.js` | populate the graph; `confidence.js` / `externals.js` hold the data-model vocabularies | +| `scripts/graph/*.js` | populate the graph; `confidence.js` / `externals.js` / `references.js` hold the data-model vocabularies and shared edge helpers | | `scripts/patterns/*.js` | one structural check per file; each defines its own result `@typedef` | | `scripts/enrichment/{api,cli}.js` | Phase 2 read/write commands over the live graph | | `scripts/renderer/{render.js,template.html}` | `report.json` → HTML | @@ -60,7 +60,13 @@ has already exited. silently vanish. A new edge must follow both. - **Mechanical vs judgment decides where code goes.** Anything reproducible is a Phase-1 module; anything needing judgment is a Phase-2 command the agent drives - from a playbook. That boundary tells you where a new capability belongs. + from a playbook. "Reproducible" means the signal is strong enough to act on + without judgment — not merely that a script *could* run. Finding references to + removed code is mechanical (grep a known symbol), so it is a Phase-1 pass + (`removal-refs.js`); `addReference` stays as the manual escape hatch for the + non-code cases the pass can't see. Mapping a method to a Gremlin step is *not* + mechanical — step names collide with ordinary method names, so it needs the + judgment the GLV playbook describes, and stays a Phase-2 command. ## How to change it diff --git a/.skills/tinker-review/playbooks/removal.md b/.skills/tinker-review/playbooks/removal.md index a40dcd0d63..b68574cf9c 100644 --- a/.skills/tinker-review/playbooks/removal.md +++ b/.skills/tinker-review/playbooks/removal.md @@ -16,31 +16,29 @@ deletions). It runs in addition to `general.md` and any module playbook. (e.g. `Krb5Authenticator.java` -> `Krb5Authenticator`). Deleted files are already in the graph as `File { deleted: true }` markers. -2. Run `listExternalRefs`. Any external callee whose `matchesDeletedSymbol` is - true is a dangling reference the changed code itself still makes — a - smoking gun visible in the graph with no grep needed. Record each with - `addReference` and treat as a finding. - -3. **Grep the surviving worktree** (`/tmp/pr-review-<pr>/src`) for every removed - symbol, excluding the deleted files themselves. Search code *and* the - supporting cast that removals commonly miss: - - source (`*.java`, GLV sources) and build files (`pom.xml`, `*.gradle`) - - config/resources (`*.yaml`, `*.conf`, `*.properties`) +2. **Phase 1 already found the code-symbol references.** For every deleted *code* + file it grepped the surviving worktree for that class/method name and wrote a + `references` edge per hit — read them from `checks.removalRefs` (and + `checks.removalRefs.externalCallers` for changed code still calling a removed + name). Your job on these is judgment, not discovery: classify each (see + Interpret). They are `INFERRED`; confirm or downgrade with `setEdgeConfidence`. + +3. **Grep for what the automatic pass skips** — the non-code supporting cast that + removals commonly leave behind, keyed off `listDeleted`: + - config/resources (`*.yaml`, `*.conf`, `*.properties`), ports, feature flags + - build files (`pom.xml`, `*.gradle`) — was the dependency actually dropped? - docs (`docs/src/**/*.asciidoc`) and `CHANGELOG.asciidoc` - Docker/CI setup (compose files, `*.sh`) - For each surviving hit, record it with `addReference --fromPath <file> - --toPath <deletedFile> --symbol <name> --location <where>`. - -4. Confirm the removal is complete on the *other* side too: was the dependency - dropped from `pom.xml`? Were the config keys, ports, and doc sections that - described the feature removed, not just the classes? + Record any surviving hit with `addReference --fromPath <file> --toPath + <deletedFile> --symbol <name> --location <where>` — the escape hatch for the + cases the code-symbol pass cannot see. ## Interpret Read the structural signals from evidence.json (schema in [references/interfaces.md](../references/interfaces.md)); the `references` edges -you added above and checks.coverageGaps on any surviving code are the primary -structural outputs here. +in checks.removalRefs (plus any you added by hand) and checks.coverageGaps on +any surviving code are the primary structural outputs here. Not every surviving reference is a defect — classify each: - **Active code / build / config / live docs** referencing a removed symbol is a diff --git a/.skills/tinker-review/scripts/enrichment/api.js b/.skills/tinker-review/scripts/enrichment/api.js index 9915c1c088..b90b78757c 100644 --- a/.skills/tinker-review/scripts/enrichment/api.js +++ b/.skills/tinker-review/scripts/enrichment/api.js @@ -21,6 +21,7 @@ import { readFile } from "node:fs/promises"; import { join } from "node:path"; import gremlin from "gremlin"; import { CONFIDENCE, normalizeConfidence, isValidConfidence } from "../graph/confidence.js"; +import { symbolFromPath, createReferenceEdge } from "../graph/references.js"; const { process: { statics: __ } } = gremlin; @@ -80,19 +81,6 @@ export async function getCanonicalSteps(repoPath) { return cachedSteps; } -// The likely symbol name a source file defined — its basename without extension -// (Krb5Authenticator.java -> Krb5Authenticator). Used to turn deleted-file paths -// into names the agent can grep the rest of the repo for. -function symbolFromPath(path) { - const base = path.split("/").pop() || path; - const dot = base.indexOf("."); - return dot > 0 ? base.slice(0, dot) : base; -} - -function extOf(path) { - return path.includes(".") ? path.split(".").pop() : ""; -} - // === Removal-impact reads === /** @@ -206,12 +194,11 @@ export async function setEdgeConfidence(g, params = {}) { } /** - * Record a lingering reference to removed code: a surviving file (fromPath) that - * still mentions a symbol defined by a deleted file (toPath). Creates a - * `references` edge File -> File(deleted), carrying the matched symbol and - * location. The source file is often outside the changed set and has no vertex - * yet, so it's find-or-created as an unparsed marker. This is the payoff of a - * removal review — "the PR deleted X, but these places still use it." + * Manual escape hatch for recording a lingering reference to removed code that + * the Phase-1 removal-refs pass didn't catch — e.g. a config-string or + * non-code-symbol reference, which that pass deliberately skips. The automatic + * pass (patterns/removal-refs.js) handles code symbols; use this for the cases + * that need a human/agent to spot. Creates the same `references` edge. * * @param {object} g - gremlin-js GraphTraversalSource (already connected) * @param {object} params @@ -219,7 +206,7 @@ export async function setEdgeConfidence(g, params = {}) { * @param {string} params.toPath - Deleted file path (must be an existing deleted File) * @param {string} [params.symbol] - The removed symbol found in fromPath * @param {string} [params.location] - Where (e.g. "L42" or a line snippet) - * @param {string} [params.confidence] - default INFERRED (a textual grep match) + * @param {string} [params.confidence] - default INFERRED (a textual match) * @returns {Promise<object>} */ export async function addReference(g, params = {}) { @@ -234,25 +221,7 @@ export async function addReference(g, params = {}) { return { error: `no deleted File vertex for toPath "${toPath}" (use listDeleted for valid targets)` }; } - const srcExists = await g.V().hasLabel("File").has("path", fromPath).hasNext(); - if (!srcExists) { - await g.addV("File") - .property("path", fromPath) - .property("language", extOf(fromPath)) - .property("changed", false) - .property("parsed", false) - .property("deleted", false) - .next(); - } - - await g.V().hasLabel("File").has("path", fromPath) - .addE("references") - .property("confidence", conf) - .property("symbol", symbol || "") - .property("location", location || "") - .to(__.V().hasLabel("File").has("path", toPath)) - .next(); - + await createReferenceEdge(g, { fromPath, toPath, symbol, location, confidence: conf }); return { referenced: `${fromPath} -> ${toPath}`, symbol: symbol || "", confidence: conf }; } diff --git a/.skills/tinker-review/scripts/graph/references.js b/.skills/tinker-review/scripts/graph/references.js new file mode 100644 index 0000000000..21f84793bf --- /dev/null +++ b/.skills/tinker-review/scripts/graph/references.js @@ -0,0 +1,85 @@ +/* + * 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. + */ + +// Shared helpers for symbols and `references` edges. Used by both the Phase-1 +// removal-refs pass (patterns/removal-refs.js) and the manual addReference +// escape hatch (enrichment/api.js), so the edge shape lives in one place. + +import gremlin from "gremlin"; +import { CONFIDENCE } from "./confidence.js"; + +const { process: { statics: __ } } = gremlin; + +const CODE_EXTENSIONS = new Set([ + "java", "py", "js", "ts", "jsx", "tsx", "mjs", "go", "cs", "groovy", + "kt", "scala", "rb", "rs", "c", "cpp", "h", "hpp", +]); + +// The likely symbol a source file defined — its basename without extension +// (Krb5Authenticator.java -> Krb5Authenticator). +export function symbolFromPath(path) { + const base = path.split("/").pop() || path; + const dot = base.indexOf("."); + return dot > 0 ? base.slice(0, dot) : base; +} + +export function extOf(path) { + return path.includes(".") ? path.split(".").pop() : ""; +} + +export function isCodeFile(path) { + return CODE_EXTENSIONS.has(extOf(path)); +} + +/** + * Create a `references` edge: a surviving file (fromPath) still mentions a symbol + * from a deleted file (toPath). The source file is often outside the changed set + * and has no vertex yet, so it is find-or-created as an unparsed marker. Assumes + * the target (deleted) File vertex already exists. + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @param {object} params + * @param {string} params.fromPath + * @param {string} params.toPath + * @param {string} [params.symbol] + * @param {string} [params.location] + * @param {string} [params.confidence] - default INFERRED + */ +export async function createReferenceEdge(g, params) { + const { fromPath, toPath, symbol, location, confidence = CONFIDENCE.INFERRED } = params; + + const srcExists = await g.V().hasLabel("File").has("path", fromPath).hasNext(); + if (!srcExists) { + await g.addV("File") + .property("path", fromPath) + .property("language", extOf(fromPath)) + .property("changed", false) + .property("parsed", false) + .property("deleted", false) + .next(); + } + + await g.V().hasLabel("File").has("path", fromPath) + .addE("references") + .property("confidence", confidence) + .property("symbol", symbol || "") + .property("location", location || "") + .to(__.V().hasLabel("File").has("path", toPath)) + .next(); +} diff --git a/.skills/tinker-review/scripts/patterns/removal-refs.js b/.skills/tinker-review/scripts/patterns/removal-refs.js new file mode 100644 index 0000000000..9d4792ccbe --- /dev/null +++ b/.skills/tinker-review/scripts/patterns/removal-refs.js @@ -0,0 +1,106 @@ +/* + * 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 { execFile } from "node:child_process"; +import { promisify } from "node:util"; +import { CONFIDENCE } from "../graph/confidence.js"; +import { symbolFromPath, isCodeFile, createReferenceEdge } from "../graph/references.js"; + +const exec = promisify(execFile); + +// Whole-word search for a symbol across the PR-head worktree. Returns one hit +// per file (first line), since deleted files no longer exist in the worktree the +// results are surviving references only. `git grep` exits non-zero on no match. +async function gitGrepWord(symbol, repoPath) { + try { + const { stdout } = await exec("git", ["grep", "-nw", symbol], { + cwd: repoPath, + maxBuffer: 16 * 1024 * 1024, + }); + const firstLineByFile = new Map(); + for (const line of stdout.split("\n")) { + const m = line.match(/^([^:]+):(\d+):/); + if (m && !firstLineByFile.has(m[1])) firstLineByFile.set(m[1], m[2]); + } + return [...firstLineByFile.entries()].map(([file, ln]) => ({ file, line: `L${ln}` })); + } catch { + return []; + } +} + +/** + * Phase-1 removal-impact pass. Mechanical mirror of classifyExternals: for every + * deleted *code* file, grep the surviving worktree for its symbol and auto-create + * a `references` edge (INFERRED) for each hit. Finding the references is + * reproducible; classifying them (blocking vs. benign historical note) is the + * agent's judgment in the report. Config/non-code symbols are skipped here and + * left to the addReference escape hatch. + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @param {string} repoPath - a git worktree (PR-head) to grep + * @returns {Promise<RemovalRefsResult>} + */ + +/** + * @typedef {Object} RemovalReference + * @property {string} from surviving file that still names the removed symbol + * @property {string} to the deleted file path + * @property {string} symbol the removed symbol matched + * @property {string} location first line in `from` (e.g. "L42") + * + * @typedef {Object} RemovalRefsResult + * @property {string[]} deletedCodeSymbols symbols of deleted code files that were searched + * @property {RemovalReference[]} references surviving references found (also written as edges) + * @property {string[]} externalCallers external-callee stubs whose name matches a deleted + * symbol — changed code still calling removed code + * @property {number} total references.length + */ +export async function findRemovalRefs(g, repoPath) { + const deletedPaths = await g.V().hasLabel("File").has("deleted", true).values("path").toList(); + const result = { deletedCodeSymbols: [], references: [], externalCallers: [], total: 0 }; + if (deletedPaths.length === 0) return result; + + const deletedSymbolSet = new Set(deletedPaths.map(symbolFromPath)); + + for (const path of deletedPaths) { + if (!isCodeFile(path)) continue; + const symbol = symbolFromPath(path); + if (!/^\w+$/.test(symbol)) continue; + result.deletedCodeSymbols.push(symbol); + + for (const { file, line } of await gitGrepWord(symbol, repoPath)) { + await createReferenceEdge(g, { + fromPath: file, + toPath: path, + symbol, + location: line, + confidence: CONFIDENCE.INFERRED, + }); + result.references.push({ from: file, to: path, symbol, location: line }); + } + } + + // In-graph dangling references: a changed function still calls a name that a + // deleted file defined (surfaced from the external-callee stubs, no grep). + const externalStubs = await g.V().hasLabel("Function").has("external", true).values("name").toList(); + result.externalCallers = externalStubs.filter((n) => deletedSymbolSet.has(n)); + + result.total = result.references.length; + return result; +} diff --git a/.skills/tinker-review/scripts/review.js b/.skills/tinker-review/scripts/review.js index 09c42c491b..848bbd4fde 100644 --- a/.skills/tinker-review/scripts/review.js +++ b/.skills/tinker-review/scripts/review.js @@ -36,6 +36,7 @@ import { clusterAnalysis } from "./patterns/cluster-analysis.js"; import { architecture } from "./patterns/architecture.js"; 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 { createPrDiscussion } from "./enrichment/api.js"; import { discoverDiscussions } from "./discovery/discussions.js"; @@ -301,6 +302,10 @@ export async function phase1(session) { const externalsResult = await classifyExternals(g, worktreePath); log(` externals: ${externalsResult.library.length} library / ${externalsResult.project.length} project / ${externalsResult.unresolved.length} unresolved`); + log(`Finding references to removed code...`); + const removalRefsResult = await findRemovalRefs(g, worktreePath); + log(` removal_refs: ${removalRefsResult.total} surviving references to ${removalRefsResult.deletedCodeSymbols.length} removed symbols`); + log(`Running checks...`); const completenessResults = await completeness(g, { vertexLabel: "File", @@ -344,6 +349,7 @@ export async function phase1(session) { clusters: clusterResult, confidence: confidenceResult, externals: externalsResult, + removalRefs: removalRefsResult, orphans: orphansResult, }, discussions,
