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 01790f6b0694adabb90b211263a9b75a582e284c Author: Stephen Mallette <[email protected]> AuthorDate: Fri Jul 3 11:39:45 2026 -0400 tinker-review: agent tools for confidence, removal impact, and callee origin listInferred + setEdgeConfidence let the agent verify name-resolved / mapped edges and re-grade them (promote a confirmed edge to EXTRACTED, downgrade a wrong one to AMBIGUOUS). listDeleted, listExternalRefs, addReference and a removal.md playbook surface lingering references to removed code. classifyExternals tags external-callee stubs library/project/unresolved so centrality drops library noise. Playbooks now point at these real commands. Assisted-by: Claude Code:claude-opus-4-8 --- .skills/tinker-review/SKILL.md | 11 ++ .skills/tinker-review/playbooks/driver-server.md | 5 +- .skills/tinker-review/playbooks/general.md | 9 ++ .skills/tinker-review/playbooks/glv.md | 10 ++ .skills/tinker-review/playbooks/removal.md | 61 ++++++++ .skills/tinker-review/references/schema.md | 11 +- .skills/tinker-review/scripts/enrichment/api.js | 155 ++++++++++++++++++++- .skills/tinker-review/scripts/enrichment/cli.js | 49 ++++++- .skills/tinker-review/scripts/graph/externals.js | 87 ++++++++++++ .../tinker-review/scripts/patterns/centrality.js | 14 +- .../scripts/patterns/classify-externals.js | 75 ++++++++++ .../scripts/patterns/confidence-audit.js | 57 +++++++- .skills/tinker-review/scripts/review.js | 6 + 13 files changed, 537 insertions(+), 13 deletions(-) diff --git a/.skills/tinker-review/SKILL.md b/.skills/tinker-review/SKILL.md index 30f028258c..b29d148251 100644 --- a/.skills/tinker-review/SKILL.md +++ b/.skills/tinker-review/SKILL.md @@ -60,6 +60,7 @@ Then determine which domain-specific playbooks apply from changed file paths: - `gremlin-driver/`, `gremlin-server/`, `gremlin-util/` → `playbooks/driver-server.md` - Small change set with linked issue → `playbooks/bug-fix.md` - `gremlin-language/` or `*.g4` → `playbooks/grammar.md` +- Deletion-heavy change set (removes a feature/module/dependency; `listDeleted` returns entries) → `playbooks/removal.md` Load ALL matching playbooks. Execute enrichment for each in sequence. @@ -81,17 +82,27 @@ node scripts/enrichment/cli.js listTypes --workDir /tmp/pr-review-<pr> --kind cl node scripts/enrichment/cli.js getCallsFrom --workDir /tmp/pr-review-<pr> --function <name> --file <path> node scripts/enrichment/cli.js getCanonicalSteps --workDir /tmp/pr-review-<pr> node scripts/enrichment/cli.js auditConfidence --workDir /tmp/pr-review-<pr> +node scripts/enrichment/cli.js listInferred --workDir /tmp/pr-review-<pr> --relation implements_step +node scripts/enrichment/cli.js listDeleted --workDir /tmp/pr-review-<pr> # removal PRs: files the PR deleted + their symbols +node scripts/enrichment/cli.js listExternalRefs --workDir /tmp/pr-review-<pr> # unresolved callees; flags any matching a deleted symbol ``` `auditConfidence` returns the edge confidence distribution and the list of AMBIGUOUS edges. Re-run it after enrichment to refresh the audit with the edges you added, then reflect any remaining AMBIGUOUS links in `openQuestions`. +`listInferred` is your **verification worklist** — the name-resolved / agent-mapped +edges worth a source check (optionally narrowed with `--relation`). After reading +the source, use `setEdgeConfidence` (below) to promote a confirmed edge to +`EXTRACTED` or downgrade a wrong resolution to `AMBIGUOUS`. + **Write commands** (edges you create default to `INFERRED`; pass `--confidence AMBIGUOUS` for a guess you want flagged, or `EXTRACTED` when the source states it directly): ```bash node scripts/enrichment/cli.js mapStep --workDir /tmp/pr-review-<pr> --function <name> --file <path> --step <canonicalName> [--confidence INFERRED|AMBIGUOUS|EXTRACTED] +node scripts/enrichment/cli.js setEdgeConfidence --workDir /tmp/pr-review-<pr> --relation <label> --fromName <name> [--fromFile <path>] [--toName <name>] --confidence <EXTRACTED|INFERRED|AMBIGUOUS> +node scripts/enrichment/cli.js addReference --workDir /tmp/pr-review-<pr> --fromPath <survivingFile> --toPath <deletedFile> --symbol <name> [--location <where>] [--confidence ...] node scripts/enrichment/cli.js linkDiscussion --workDir /tmp/pr-review-<pr> --url <url> --source jira --title <title> [--confidence ...] node scripts/enrichment/cli.js linkDoc --workDir /tmp/pr-review-<pr> --entity Step --name <name> --doc <path> [--confidence ...] node scripts/enrichment/cli.js addGrammarRule --workDir /tmp/pr-review-<pr> --name <name> diff --git a/.skills/tinker-review/playbooks/driver-server.md b/.skills/tinker-review/playbooks/driver-server.md index 177fbfdf5e..5f1d2d1018 100644 --- a/.skills/tinker-review/playbooks/driver-server.md +++ b/.skills/tinker-review/playbooks/driver-server.md @@ -49,7 +49,10 @@ For each layer, look for: ## Interpret Driver/server changes have inherently high blast radius — they're shared infrastructure. Don't flag blast radius as surprising, but DO highlight -which specific callers are most affected. +which specific callers are most affected. Use `listExternalRefs` to separate +real project coupling (`origin: project`/`unresolved`) from library noise +(`origin: library`) when judging a changed function's reach — centrality already +drops the library calls, so a function still ranking high is genuinely central. Connection pooling and concurrency code should have explicit test coverage. If coverage gaps exist in connection lifecycle code, flag prominently — diff --git a/.skills/tinker-review/playbooks/general.md b/.skills/tinker-review/playbooks/general.md index 91ea97253c..3cee125cd5 100644 --- a/.skills/tinker-review/playbooks/general.md +++ b/.skills/tinker-review/playbooks/general.md @@ -31,6 +31,15 @@ Look for these patterns in the changed code and annotate them: - Data structures with concurrency implications (note if CopyOnWriteArraySet, synchronized collections, etc. are introduced without profiling justification) +## Verify confidence +Before writing the report, run `auditConfidence`. Then pull the verification +worklist with `listInferred` (start with `--relation implements_step`, then any +`calls` edges that matter to your findings) and spot-check the ones your +conclusions lean on against the source in the worktree. Re-grade what you +verify with `setEdgeConfidence`: promote a confirmed edge to `EXTRACTED`, or +downgrade a wrong name-resolution to `AMBIGUOUS`. Anything left `AMBIGUOUS` +after this pass belongs in `openQuestions` — don't assert it as fact. + ## Checks - coverage_gaps(pr.tests(), pr.modified()) - orphans("Function", "tests", { changedOnly: true }) diff --git a/.skills/tinker-review/playbooks/glv.md b/.skills/tinker-review/playbooks/glv.md index 8c862a7ddc..4937150a58 100644 --- a/.skills/tinker-review/playbooks/glv.md +++ b/.skills/tinker-review/playbooks/glv.md @@ -24,6 +24,16 @@ methods — only the method on GraphTraversal/GraphTraversalSource that users call (e.g., `tree()`) should map to the step. In a GLV, the equivalent is the method on the traversal DSL class. +Record your confidence in each mapping via `mapStep --confidence`: use the +default `INFERRED` for a solid match, and `AMBIGUOUS` when you can't reliably +tell whether a method is a real step implementation (this is the graph form of +the `step_mapping_confidence < 0.7` escape below — AMBIGUOUS mappings surface in +the report's review list instead of being asserted as fact). Once you've mapped +methods, run `listInferred --relation implements_step` and, for any mapping you +then confirm against the reference GLV or grammar, promote it with +`setEdgeConfidence --relation implements_step --fromName <method> --toName <step> +--confidence EXTRACTED`. + If the PR references a JIRA ticket (TINKERPOP-XXXX), link it as a discussion. For the driver layer, identify connection acquisition and release points. diff --git a/.skills/tinker-review/playbooks/removal.md b/.skills/tinker-review/playbooks/removal.md new file mode 100644 index 0000000000..19e45c2d73 --- /dev/null +++ b/.skills/tinker-review/playbooks/removal.md @@ -0,0 +1,61 @@ +# Playbook: Removal / Deprecation + +## Context +The PR deletes code — a feature, a module, a dependency, or a deprecated API. +The change set is deletion-heavy (many removed files, net-negative diff). The +central review question is not "is the new code correct?" but **"does anything +left behind still depend on what was removed?"** A removal that leaves dangling +references, stale config, or orphaned docs is worse than no removal — it breaks +the build or misleads users. + +Load this playbook when `listDeleted` returns entries (or the PR is dominated by +deletions). It runs in addition to `general.md` and any module playbook. + +## Enrich +1. Run `listDeleted` to get the removed files and the symbol each likely defined + (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`) + - 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? + +## Checks +- coverage_gaps(pr.tests(), pr.modified()) +- (references edges created above are the primary structural output) + +## Interpret +Not every surviving reference is a defect — classify each: +- **Active code / build / config / live docs** referencing a removed symbol is a + **blocking finding**: the build breaks or the feature is half-removed. +- **Historical release notes** (e.g. `docs/src/upgrade/release-3.x.asciidoc`) + mentioning the removed symbol are **expected and correct** — they record when + the feature existed. Note them as verified-benign, not as a problem. +- A **current** upgrade/CHANGELOG entry should *gain* a line announcing the + removal. Its absence is a finding (users need to know the feature is gone). + +Weight findings by where the reference lives, and say so explicitly in the +report so the reviewer isn't left guessing whether a hit matters. + +## Escape +- if a removed symbol is still referenced by active source or build files — + "Removal is incomplete: <symbol> still referenced in <file>; the build or + feature is broken until this is resolved." +- if the removal drops a dependency or public API without an upgrade-doc entry — + "User-facing removal needs an upgrade/CHANGELOG note." diff --git a/.skills/tinker-review/references/schema.md b/.skills/tinker-review/references/schema.md index 22facca8d1..38564f8aa9 100644 --- a/.skills/tinker-review/references/schema.md +++ b/.skills/tinker-review/references/schema.md @@ -18,7 +18,7 @@ select materialized files with `.hasNot("parsed")` and markers with **Function** `{ name, signature, visibility, filePath, lines_start, lines_end, changed }` A function or method. The primary unit of analysis. -*External stub Functions* `{ name, external: true, resolved: false, changed: false }` +*External stub Functions* `{ name, external: true, resolved: false, changed: false, origin?, definedIn? }` are markers created when a `calls`/`tests` edge targets a function by name that wasn't extracted (a library/JDK call, or a function in a file this PR didn't change). They keep the edge from vanishing and let blast-radius/centrality see @@ -26,6 +26,12 @@ the call; they lack `filePath`/`signature`/`visibility`. Filter them out with `.has("external", false)` — or, since real Functions have no `external` property, `.hasNot("external")` — when you only want materialized code. +`classifyExternals` tags each stub with `origin`: `library` (a known JDK/accessor +name — noise), `project` (a repo source declares a type with this name; +`definedIn` records the file), or `unresolved` (unknown). Centrality drops +`origin: library` calls from out-degree so ubiquitous accessor calls don't +inflate hotspots. + **Type** `{ name, kind, visibility, filePath }` A class, interface, struct, or enum. `kind` is one of: class, interface, struct, enum. @@ -57,7 +63,7 @@ The PR itself is a Discussion with `source: "pr"`. **Comment** `{ author, body, timestamp }` A comment on a Discussion. -## Edges (14 labels) +## Edges (15 labels) ### Edge confidence (every edge) @@ -84,6 +90,7 @@ renders this as the **Signal Confidence** panel. | `defines` | File | Function or Type | File contains this definition | | `implements` | Function | Type | Function implements an interface | | `depends_on` | File | File | File imports/requires another file | +| `references` | File | File (deleted) | A surviving file still mentions a symbol from a file the PR deleted. Added during a removal review via `addReference`; carries `symbol` and `location` properties. | ### Domain relationships diff --git a/.skills/tinker-review/scripts/enrichment/api.js b/.skills/tinker-review/scripts/enrichment/api.js index 5508ed563b..9915c1c088 100644 --- a/.skills/tinker-review/scripts/enrichment/api.js +++ b/.skills/tinker-review/scripts/enrichment/api.js @@ -20,7 +20,7 @@ import { readFile } from "node:fs/promises"; import { join } from "node:path"; import gremlin from "gremlin"; -import { CONFIDENCE, normalizeConfidence } from "../graph/confidence.js"; +import { CONFIDENCE, normalizeConfidence, isValidConfidence } from "../graph/confidence.js"; const { process: { statics: __ } } = gremlin; @@ -80,6 +80,66 @@ 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 === + +/** + * Files the PR deleted (stub Files marked deleted). Each entry pairs the path + * with the symbol name it likely defined, so the agent can grep the surviving + * tree for lingering references to removed code. + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @returns {Promise<{path: string, symbol: string}[]>} + */ +export async function listDeleted(g) { + const paths = await g.V().hasLabel("File").has("deleted", true).values("path").toList(); + return paths.map((path) => ({ path, symbol: symbolFromPath(path) })); +} + +/** + * Unresolved external callees (external Function stubs) — names the changed code + * calls that weren't defined in the changed set. Includes each stub's `origin` + * (library/project/unresolved, once classifyExternals has run) and flags any + * whose name matches a deleted file's symbol: a changed file still calling a + * just-removed name is a dangling reference visible in the graph alone. + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @returns {Promise<{name: string, origin: string, matchesDeletedSymbol: boolean}[]>} + */ +export async function listExternalRefs(g) { + const rows = await g.V().hasLabel("Function").has("external", true) + .project("name", "origin") + .by("name") + .by(__.coalesce(__.values("origin"), __.constant("unclassified"))) + .toList(); + const deletedPaths = await g.V().hasLabel("File").has("deleted", true).values("path").toList(); + const deletedSymbols = new Set(deletedPaths.map(symbolFromPath)); + + const rank = { project: 0, unresolved: 1, unclassified: 2, library: 3 }; + return rows + .map((r) => ({ + name: r.get("name"), + origin: r.get("origin"), + matchesDeletedSymbol: deletedSymbols.has(r.get("name")), + })) + .sort((a, b) => { + if (a.matchesDeletedSymbol !== b.matchesDeletedSymbol) return a.matchesDeletedSymbol ? -1 : 1; + return (rank[a.origin] ?? 9) - (rank[b.origin] ?? 9); + }); +} + // === Write operations === export async function mapStep(g, functionName, filePath, canonicalStepName, confidence) { @@ -103,6 +163,99 @@ export async function mapStep(g, functionName, filePath, canonicalStepName, conf return { mapped: `${functionName} -> ${canonicalStepName}`, confidence: conf }; } +/** + * Re-grade the confidence of existing edge(s) after the agent verifies them + * against source. Identifies edges by their source vertex (name, optionally + * pinned to a file) and relation, optionally narrowed to a named target — + * covering the edges agents actually verify (`calls`, `implements_step`). + * Promote a confirmed edge to EXTRACTED, or downgrade a wrong resolution to + * AMBIGUOUS so it surfaces in the report's review list. + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @param {object} params + * @param {string} params.relation - Edge label (e.g. calls, implements_step) + * @param {string} params.fromName - Source vertex name + * @param {string} [params.fromFile] - Pin the source to this filePath + * @param {string} [params.toName] - Narrow to edges whose target has this name + * @param {string} [params.fromLabel] - Source vertex label (default Function) + * @param {string} params.confidence - EXTRACTED | INFERRED | AMBIGUOUS + * @returns {Promise<object>} + */ +export async function setEdgeConfidence(g, params = {}) { + const { relation, fromName, fromFile, toName, fromLabel = "Function", confidence } = params; + if (!isValidConfidence(confidence)) { + return { error: `invalid confidence "${confidence}" — use EXTRACTED, INFERRED, or AMBIGUOUS` }; + } + if (!relation || !fromName) { + return { error: "relation and fromName are required" }; + } + + let t = g.V().hasLabel(fromLabel).has("name", fromName); + if (fromFile) t = t.has("filePath", fromFile); + t = t.outE(relation); + if (toName) t = t.where(__.inV().has("name", toName)); + + const updated = await t.property("confidence", confidence).toList(); + return { + relation, + from: fromName, + to: toName || "*", + confidence, + updated: updated.length, + }; +} + +/** + * 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." + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @param {object} params + * @param {string} params.fromPath - Surviving file that references the removed symbol + * @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) + * @returns {Promise<object>} + */ +export async function addReference(g, params = {}) { + const { fromPath, toPath, symbol, location, confidence } = params; + const conf = normalizeConfidence(confidence, CONFIDENCE.INFERRED); + if (!fromPath || !toPath) { + return { error: "fromPath and toPath are required" }; + } + + const targetExists = await g.V().hasLabel("File").has("path", toPath).hasNext(); + if (!targetExists) { + 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(); + + return { referenced: `${fromPath} -> ${toPath}`, symbol: symbol || "", confidence: conf }; +} + export async function linkDiscussion(g, url, source, title, body, confidence) { const conf = normalizeConfidence(confidence, CONFIDENCE.INFERRED); await g.addV("Discussion") diff --git a/.skills/tinker-review/scripts/enrichment/cli.js b/.skills/tinker-review/scripts/enrichment/cli.js index c5f5099c22..49d078e853 100644 --- a/.skills/tinker-review/scripts/enrichment/cli.js +++ b/.skills/tinker-review/scripts/enrichment/cli.js @@ -22,10 +22,12 @@ import { join } from "node:path"; import gremlin from "gremlin"; import { listFunctions, listTypes, getCallsFrom, getCanonicalSteps, - mapStep, linkDiscussion, linkDoc, addGrammarRule, annotate, + listDeleted, listExternalRefs, addReference, + mapStep, setEdgeConfidence, linkDiscussion, linkDoc, addGrammarRule, annotate, createPrDiscussion, } from "./api.js"; -import { confidenceAudit } from "../patterns/confidence-audit.js"; +import { confidenceAudit, listInferred } from "../patterns/confidence-audit.js"; +import { classifyExternals } from "../patterns/classify-externals.js"; const COMMANDS = { listFunctions: { fn: listFunctions, needsG: true }, @@ -33,7 +35,13 @@ const COMMANDS = { getCallsFrom: { fn: getCallsFrom, needsG: true }, getCanonicalSteps: { fn: getCanonicalSteps, needsG: false }, auditConfidence: { fn: confidenceAudit, needsG: true }, + listInferred: { fn: listInferred, needsG: true }, + listDeleted: { fn: listDeleted, needsG: true }, + listExternalRefs: { fn: listExternalRefs, needsG: true }, + classifyExternals: { fn: classifyExternals, needsG: true }, + addReference: { fn: addReference, needsG: true }, mapStep: { fn: mapStep, needsG: true }, + setEdgeConfidence: { fn: setEdgeConfidence, needsG: true }, linkDiscussion: { fn: linkDiscussion, needsG: true }, linkDoc: { fn: linkDoc, needsG: true }, addGrammarRule: { fn: addGrammarRule, needsG: true }, @@ -76,7 +84,13 @@ async function main() { console.log(" getCallsFrom --function <name> --file <path>"); console.log(" getCanonicalSteps"); console.log(" auditConfidence [--maxAmbiguous 50]"); + console.log(" listInferred [--relation implements_step|calls|...] [--limit 100]"); + console.log(" listDeleted"); + console.log(" listExternalRefs"); + console.log(" classifyExternals (tags external stubs origin=library|project|unresolved)"); + console.log(" addReference --fromPath <path> --toPath <deletedPath> [--symbol <name>] [--location <L42>] [--confidence ...]"); console.log(" mapStep --function <name> --file <path> --step <canonicalName> [--confidence INFERRED|AMBIGUOUS|EXTRACTED]"); + console.log(" setEdgeConfidence --relation <label> --fromName <name> [--fromFile <path>] [--toName <name>] --confidence <EXTRACTED|INFERRED|AMBIGUOUS>"); console.log(" linkDiscussion --url <url> --source <jira|devlist|proposal> --title <title> [--body <body>] [--confidence ...]"); console.log(" linkDoc --entity <label> --name <name> --doc <path> [--section <section>] [--confidence ...]"); console.log(" addGrammarRule --name <name> [--production <production>]"); @@ -130,9 +144,40 @@ async function main() { case "auditConfidence": result = await fn(g, { maxAmbiguous: args.maxAmbiguous }); break; + case "listInferred": + result = await fn(g, { relation: args.relation, limit: args.limit }); + break; + case "listDeleted": + result = await fn(g); + break; + case "listExternalRefs": + result = await fn(g); + break; + case "classifyExternals": + result = await fn(g, session.worktreePath || session.repoPath); + break; + case "addReference": + result = await fn(g, { + fromPath: args.fromPath, + toPath: args.toPath, + symbol: args.symbol, + location: args.location, + confidence: args.confidence, + }); + break; case "mapStep": result = await fn(g, args.function, args.file, args.step, args.confidence); break; + case "setEdgeConfidence": + result = await fn(g, { + relation: args.relation, + fromName: args.fromName, + fromFile: args.fromFile, + toName: args.toName, + fromLabel: args.fromLabel, + confidence: args.confidence, + }); + break; case "linkDiscussion": result = await fn(g, args.url, args.source, args.title, args.body, args.confidence); break; diff --git a/.skills/tinker-review/scripts/graph/externals.js b/.skills/tinker-review/scripts/graph/externals.js new file mode 100644 index 0000000000..01c6080e7c --- /dev/null +++ b/.skills/tinker-review/scripts/graph/externals.js @@ -0,0 +1,87 @@ +/* + * 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. + */ + +/** + * Names that, when they show up as unresolved callees (external Function stubs), + * are almost certainly JDK / standard-library / ubiquitous-accessor calls rather + * than project functions. Classifying these as `library` lets structural checks + * (centrality, blast radius) drop the noise they'd otherwise inflate. + * + * Deliberately conservative: it lists object/collection/string/IO/logging/stream + * staples and common getX/isX accessors, and avoids ambiguous verbs (build, + * create, start, run, call…) that are just as likely to name a real project + * method. A false "library" on a genuinely ubiquitous name like `get` is + * acceptable; a false "library" on a distinctive project method is not. + */ +export const COMMON_LIBRARY_NAMES = new Set([ + // java.lang.Object + "equals", "hashCode", "toString", "clone", "finalize", "getClass", + "wait", "notify", "notifyAll", + // Comparable / iterator + "compareTo", "iterator", "hasNext", "next", "remove", + // Collection / Map / List + "size", "isEmpty", "contains", "containsKey", "containsValue", + "add", "addAll", "get", "set", "put", "putAll", "clear", "remove", + "keySet", "values", "entrySet", "toArray", "stream", "getKey", "getValue", + // String / CharSequence + "length", "charAt", "substring", "trim", "split", "replace", "indexOf", + "startsWith", "endsWith", "matches", "toLowerCase", "toUpperCase", + "getBytes", "valueOf", "format", + // common accessors + "getName", "getId", "getType", "getMessage", "getValue", "getStatusCode", + "getCause", "getClassName", "name", "ordinal", + // IO / lifecycle + "close", "flush", "read", "write", "open", + // logging + "log", "info", "debug", "warn", "error", "trace", "printStackTrace", + "print", "println", + // Optional / functional / stream + "of", "empty", "orElse", "orElseGet", "orElseThrow", "ifPresent", "isPresent", + "forEach", "map", "filter", "collect", "count", "findFirst", "anyMatch", + "allMatch", "noneMatch", "apply", "accept", "test", +]); + +/** + * @param {string} name + * @returns {boolean} true if the name is a well-known library/JDK/accessor call + */ +export function isLibraryName(name) { + return COMMON_LIBRARY_NAMES.has(name); +} + +/** + * Origin classification for an external stub, given whether a project type with + * this name was found in the repo. + * + * library — a known JDK/standard-library/accessor name (noise) + * project — a project source file declares a type with this name (signal) + * unresolved — neither; unknown, treat with caution + * + * Method-name project detection isn't attempted (name-only grep can't tell a + * definition from a use reliably); that's the domain of precise resolution. + * + * @param {string} name + * @param {boolean} definedAsProjectType + * @returns {"library"|"project"|"unresolved"} + */ +export function classifyExternalName(name, definedAsProjectType) { + if (isLibraryName(name)) return "library"; + if (definedAsProjectType) return "project"; + return "unresolved"; +} diff --git a/.skills/tinker-review/scripts/patterns/centrality.js b/.skills/tinker-review/scripts/patterns/centrality.js index 11aa0ca259..d891533270 100644 --- a/.skills/tinker-review/scripts/patterns/centrality.js +++ b/.skills/tinker-review/scripts/patterns/centrality.js @@ -19,6 +19,8 @@ import gremlin from "gremlin"; +const { process: { statics: __ } } = gremlin; + const INHERENTLY_CENTRAL = new Set([ "equals", "hashCode", "toString", "clone", "close", "compareTo", "iterator", "hasNext", "next", "get", "set", "size", "isEmpty", @@ -39,12 +41,16 @@ const INHERENTLY_CENTRAL = new Set([ * @param {boolean} [params.changedOnly] - Only check changed functions (default: true) * @param {number} [params.topN] - Return top N results (default: 10) * @param {number} [params.minDegree] - Minimum combined in+out degree to include (default: 3) + * @param {boolean} [params.excludeLibrary] - Drop calls to library-origin external + * stubs from out-degree so JDK/accessor noise doesn't inflate hotspots (default: true). + * Only takes effect once classifyExternals has tagged `origin`. * @returns {Promise<CentralityResult>} */ export async function highCentrality(g, params = {}) { const changedOnly = params.changedOnly !== false; const topN = params.topN || 10; const minDegree = params.minDegree || 3; + const excludeLibrary = params.excludeLibrary !== false; let traversal = g.V().hasLabel("Function"); if (changedOnly) { @@ -61,7 +67,13 @@ export async function highCentrality(g, params = {}) { const changed = fnMap.get("changed"); const inDegree = await g.V(vertexId).inE("calls").count().next(); - const outDegree = await g.V(vertexId).outE("calls").count().next(); + // Out-degree optionally skips calls to library-origin externals (getName, + // toString, …) so ubiquitous JDK/accessor calls don't inflate the hotspot. + let outTraversal = g.V(vertexId).outE("calls"); + if (excludeLibrary) { + outTraversal = outTraversal.where(__.inV().not(__.has("origin", "library"))); + } + const outDegree = await outTraversal.count().next(); const inCount = inDegree.value; const outCount = outDegree.value; diff --git a/.skills/tinker-review/scripts/patterns/classify-externals.js b/.skills/tinker-review/scripts/patterns/classify-externals.js new file mode 100644 index 0000000000..20562362f1 --- /dev/null +++ b/.skills/tinker-review/scripts/patterns/classify-externals.js @@ -0,0 +1,75 @@ +/* + * 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 { classifyExternalName } from "../graph/externals.js"; + +const exec = promisify(execFile); + +// A precise, low-false-positive check: does a Java type declaration with this +// exact name exist anywhere in the repo? Uses `git grep` in the worktree (fast, +// respects the tree). Returns the first defining file, or null. +async function definedAsProjectType(name, repoPath) { + if (!repoPath || !/^\w+$/.test(name)) return null; + try { + const { stdout } = await exec( + "git", + ["grep", "-lE", `(class|interface|enum|@interface)[[:space:]]+${name}\\b`], + { cwd: repoPath }, + ); + const files = stdout.split("\n").filter(Boolean); + return files[0] || null; + } catch { + // git grep exits non-zero when there is no match — treat as "not found". + return null; + } +} + +/** + * Classify every external Function stub as library / project / unresolved and + * annotate the vertex with `origin` (and `definedIn` for project types). This + * lets structural checks drop library noise. Run it after populate and BEFORE + * centrality so the metric can exclude `origin: library`. + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @param {string} repoPath - a git worktree to grep for project type definitions + * @returns {Promise<{total: number, library: string[], project: object[], unresolved: string[]}>} + */ +export async function classifyExternals(g, repoPath) { + const names = await g.V().hasLabel("Function").has("external", true).values("name").toList(); + + const classified = await Promise.all(names.map(async (name) => { + const definedIn = await definedAsProjectType(name, repoPath); + return { name, origin: classifyExternalName(name, Boolean(definedIn)), definedIn }; + })); + + const summary = { total: names.length, library: [], project: [], unresolved: [] }; + for (const { name, origin, definedIn } of classified) { + let t = g.V().hasLabel("Function").has("external", true).has("name", name) + .property("origin", origin); + if (origin === "project" && definedIn) t = t.property("definedIn", definedIn); + await t.iterate(); + + if (origin === "project") summary.project.push({ name, definedIn }); + else summary[origin].push(name); + } + + return summary; +} diff --git a/.skills/tinker-review/scripts/patterns/confidence-audit.js b/.skills/tinker-review/scripts/patterns/confidence-audit.js index ddb5c6e30d..db8442694f 100644 --- a/.skills/tinker-review/scripts/patterns/confidence-audit.js +++ b/.skills/tinker-review/scripts/patterns/confidence-audit.js @@ -71,24 +71,69 @@ export async function confidenceAudit(g, params = {}) { } } - const ambiguousRows = await g.E() - .has("confidence", CONFIDENCE.AMBIGUOUS) - .project("relation", "found_in", "found_via", "from", "to") + const ambiguous = await listEdgesByConfidence(g, { + confidence: CONFIDENCE.AMBIGUOUS, + limit: maxAmbiguous, + }); + + return { distribution, total, ambiguous }; +} + +/** + * List edges at a given confidence, optionally narrowed to one relation, with + * both endpoints described. The building block behind confidenceAudit's + * AMBIGUOUS list and the agent-facing listInferred worklist. + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @param {object} [opts] + * @param {string} [opts.confidence] - Filter to this confidence value + * @param {string} [opts.relation] - Filter to this edge label (e.g. implements_step) + * @param {number} [opts.limit] - Cap the result count (default 100) + * @returns {Promise<object[]>} rows of { relation, confidence, from, to, foundIn, foundVia } + */ +export async function listEdgesByConfidence(g, opts = {}) { + const { confidence, relation, limit = 100 } = opts; + let t = g.E(); + if (relation) t = t.hasLabel(relation); + if (confidence) t = t.has("confidence", confidence); + + const rows = await t + .limit(limit) + .project("relation", "confidence", "found_in", "found_via", "from", "to") .by(__.label()) + .by(__.coalesce(__.values("confidence"), __.constant(""))) .by(__.coalesce(__.values("found_in"), __.constant(""))) .by(__.coalesce(__.values("found_via"), __.constant(""))) .by(__.outV().elementMap()) .by(__.inV().elementMap()) - .limit(maxAmbiguous) .toList(); - const ambiguous = ambiguousRows.map((row) => ({ + return rows.map((row) => ({ relation: row.get("relation"), + confidence: row.get("confidence") || undefined, from: describeVertex(row.get("from")), to: describeVertex(row.get("to")), foundIn: row.get("found_in") || undefined, foundVia: row.get("found_via") || undefined, })); +} - return { distribution, total, ambiguous }; +/** + * Agent verification worklist: the INFERRED edges (name-resolved calls, agent + * step/doc mappings) that warrant a source check. After verifying, the agent + * uses setEdgeConfidence to promote a confirmed edge to EXTRACTED or downgrade a + * wrong one to AMBIGUOUS. + * + * @param {object} g - gremlin-js GraphTraversalSource (already connected) + * @param {object} [params] + * @param {string} [params.relation] - Narrow to one relation (e.g. implements_step) + * @param {number} [params.limit] - Cap results (default 100) + * @returns {Promise<object[]>} + */ +export async function listInferred(g, params = {}) { + return listEdgesByConfidence(g, { + confidence: CONFIDENCE.INFERRED, + relation: params.relation, + limit: params.limit || 100, + }); } diff --git a/.skills/tinker-review/scripts/review.js b/.skills/tinker-review/scripts/review.js index bda97cd7b8..65863f08e5 100644 --- a/.skills/tinker-review/scripts/review.js +++ b/.skills/tinker-review/scripts/review.js @@ -35,6 +35,7 @@ import { blastRadius } from "./patterns/blast-radius.js"; 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 { createPrDiscussion } from "./enrichment/api.js"; import { discoverDiscussions } from "./discovery/discussions.js"; @@ -295,6 +296,10 @@ export async function phase1(session) { log(`Loading discussions into graph...`); await populateDiscussions(g, discussions, { pr, prTitle: prTitle.trim(), changedFiles }); + log(`Classifying external callees...`); + const externalsResult = await classifyExternals(g, worktreePath); + log(` externals: ${externalsResult.library.length} library / ${externalsResult.project.length} project / ${externalsResult.unresolved.length} unresolved`); + log(`Running checks...`); const completenessResults = await completeness(g, { vertexLabel: "File", @@ -335,6 +340,7 @@ export async function phase1(session) { blastRadius: blastResult, clusters: clusterResult, confidence: confidenceResult, + externals: externalsResult, }, discussions, changedFiles,
