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 e24bd427c63a2456150ffe747abfa6ef3b8e2fc1 Author: Stephen Mallette <[email protected]> AuthorDate: Fri Jul 3 15:18:28 2026 -0400 tinker-review: document evidence.json schema and simplify playbooks Each check's result type is now a @typedef with per-field meaning in the pattern file that produces it (fixing four @returns that referenced types defined nowhere); interfaces.md keeps only the composite Evidence/ReportPackage shape and points at those typedefs. Wire orphans into Phase 1 so checks.orphans is real. Delete the pseudo-code Checks section from every playbook and fold its emphasis into Interpret, which now cites real evidence.json fields. SKILL.md states the section-to-phase contract so the agent knows what to do with each section. Assisted-by: Claude Code:claude-opus-4-8 --- .skills/tinker-review/SKILL.md | 18 +++++ .skills/tinker-review/playbooks/bug-fix.md | 39 ++++++----- .skills/tinker-review/playbooks/driver-server.md | 27 ++++---- .skills/tinker-review/playbooks/general.md | 9 +-- .skills/tinker-review/playbooks/glv.md | 18 +++-- .skills/tinker-review/playbooks/grammar.md | 13 ++-- .skills/tinker-review/playbooks/new-step.md | 29 ++++---- .skills/tinker-review/playbooks/removal.md | 9 +-- .skills/tinker-review/references/interfaces.md | 79 ++++++++++++++-------- .../tinker-review/scripts/patterns/architecture.js | 14 +++- .../tinker-review/scripts/patterns/blast-radius.js | 19 ++++++ .../tinker-review/scripts/patterns/centrality.js | 21 ++++++ .../scripts/patterns/classify-externals.js | 10 ++- .../scripts/patterns/cluster-analysis.js | 13 ++++ .../tinker-review/scripts/patterns/completeness.js | 8 +++ .../scripts/patterns/confidence-audit.js | 21 +++++- .../scripts/patterns/coverage-gaps.js | 14 ++++ .skills/tinker-review/scripts/patterns/orphans.js | 13 ++++ .skills/tinker-review/scripts/review.js | 4 ++ 19 files changed, 275 insertions(+), 103 deletions(-) diff --git a/.skills/tinker-review/SKILL.md b/.skills/tinker-review/SKILL.md index b29d148251..b4291613d8 100644 --- a/.skills/tinker-review/SKILL.md +++ b/.skills/tinker-review/SKILL.md @@ -64,6 +64,24 @@ Then determine which domain-specific playbooks apply from changed file paths: Load ALL matching playbooks. Execute enrichment for each in sequence. +**How a playbook is applied.** Each playbook has four sections, and each maps to +a phase of this run — this is the contract for what to do with the content: + +| Section | When | What you do with it | +|---------|------|---------------------| +| **Context** | framing | Orient to the change type and its risks; not actioned directly. | +| **Enrich** | Phase 2 | Execute the listed steps using the enrichment CLI commands. | +| **Interpret** | Phase 5 | When writing the report, weigh the named `evidence.json` fields into `findings` / `openQuestions`. | +| **Escape** | any phase | Check the stop/escalate conditions; halt or flag when one holds. | + +Phase 1 already computes every structural check — completeness, coverageGaps, +centrality, blastRadius, clusters, confidence, externals, orphans — into +`evidence.json`. Playbooks' Interpret sections reference those results **by field +name** (e.g. `checks.blastRadius`); they do not re-run checks. The shape and +meaning of every field is documented in +[references/interfaces.md](references/interfaces.md) (`Evidence`), which points +to the per-check `@typedef`s in `scripts/patterns/*.js`. + ### 3. Phase 2 — Enrichment (agent-driven) The Gremlin Server is still running. Use `scripts/enrichment/cli.js` to diff --git a/.skills/tinker-review/playbooks/bug-fix.md b/.skills/tinker-review/playbooks/bug-fix.md index 20be16eb98..fce7496a07 100644 --- a/.skills/tinker-review/playbooks/bug-fix.md +++ b/.skills/tinker-review/playbooks/bug-fix.md @@ -22,25 +22,28 @@ Look for: If the PR references a JIRA ticket (TINKERPOP-XXXX), link it as a discussion. -## Checks -- completeness(pr, ["addresses"]) -- coverage_gaps(pr.tests(), pr.modified()) -- blast_radius(pr.modified(), 3) -- high_centrality(pr.modified()) - ## Interpret -High blast radius on a bug fix is a warning signal — the fix touches -something many callers depend on. This doesn't mean it's wrong, but it -means the reviewer should verify the fix doesn't subtly change behavior -for existing callers. - -Changes outside the issue scope aren't automatically bad — sometimes -fixing a bug requires touching adjacent code. But they should be -explainable. Flag them with "necessary for fix?" not "wrong." - -If high-centrality functions are modified, emphasize that the reviewer -should check all callers for behavioral changes. A fix in a hot function -can silently break things far from the fix site. +Read the structural signals from evidence.json (schema in +[references/interfaces.md](../references/interfaces.md)). + +High blast radius (checks.blastRadius) on a bug fix is a warning signal — the +fix touches something many callers depend on. It isn't wrong, but verify it +doesn't subtly change behavior for existing callers. If high-centrality +functions (checks.centrality) are modified, say explicitly that every caller +needs a behavioral-change check — a fix in a hot function breaks things far +from the fix site. + +A bug fix with no new or modified test is the biggest red flag: an untested fix +(checks.coverageGaps, checks.orphans) can't be shown to prevent regression. +Call it out prominently. + +Confirm the fix is tied to its issue — the PR should have an `addresses` edge to +the JIRA/discussion (checks.completeness on `addresses`). No linked issue means +correctness can't be assessed. + +Changes outside the issue scope aren't automatically bad — sometimes a fix needs +adjacent code. But they should be explainable. Flag them "necessary for fix?" +not "wrong." ## Escape - if no linked issue — "Cannot assess whether fix is correct without knowing the bug" diff --git a/.skills/tinker-review/playbooks/driver-server.md b/.skills/tinker-review/playbooks/driver-server.md index 5f1d2d1018..38d4cf8705 100644 --- a/.skills/tinker-review/playbooks/driver-server.md +++ b/.skills/tinker-review/playbooks/driver-server.md @@ -40,23 +40,22 @@ For each layer, look for: - Server configuration: gremlin-lang expressions, not Groovy scripts - Don't leave commented-out old code — remove it cleanly -## Checks -- blast_radius(pr.modified(), 3) -- high_centrality(pr.modified()) -- coverage_gaps(pr.tests(), pr.modified()) -- orphans("Function", "tests", { changedOnly: true }) - ## 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. 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. +Read the structural signals from evidence.json (schema in +[references/interfaces.md](../references/interfaces.md)). + +Driver/server changes have inherently high blast radius (checks.blastRadius) — +they're shared infrastructure. Don't flag the reach as surprising, but DO +highlight which specific callers are most affected (checks.centrality). 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 — -these are the hardest bugs to reproduce and the most impactful in production. +If coverage gaps exist in connection lifecycle code (checks.coverageGaps, +checks.orphans), flag prominently — these are the hardest bugs to reproduce +and the most impactful in production. Serialization changes that add/remove type codes need upgrade documentation. Check if the PR includes corresponding upgrade doc entries. diff --git a/.skills/tinker-review/playbooks/general.md b/.skills/tinker-review/playbooks/general.md index 3cee125cd5..c13eba4ffc 100644 --- a/.skills/tinker-review/playbooks/general.md +++ b/.skills/tinker-review/playbooks/general.md @@ -40,11 +40,12 @@ 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 }) - ## Interpret +Read the structural signals from evidence.json (schema in +[references/interfaces.md](../references/interfaces.md)). Missing tests on +changed code (checks.coverageGaps, checks.orphans) are a test-quality concern — +weigh them alongside the code smells below. + Style nits and unused variables are low severity — note them but don't make them the focus of the report. Prioritize safety concerns (resource leaks, concurrency risks, missing error handling) and test quality issues. diff --git a/.skills/tinker-review/playbooks/glv.md b/.skills/tinker-review/playbooks/glv.md index 4937150a58..ba9ce3308e 100644 --- a/.skills/tinker-review/playbooks/glv.md +++ b/.skills/tinker-review/playbooks/glv.md @@ -40,23 +40,21 @@ For the driver layer, identify connection acquisition and release points. Trace resource lifecycle through error paths — the common GLV bug is leaking connections when a traversal fails mid-execution. -## Checks -- completeness(glv, canonical_step_list()) -- coverage_gaps(pr.tests(), pr.modified()) -- high_centrality(pr.modified()) - ## Interpret -When reporting completeness gaps, distinguish between missing steps and -steps that exist but use a language-specific name (e.g., Python uses -`addV` but Go uses `AddV` — same step, different convention). +Read the structural signals from evidence.json (schema in +[references/interfaces.md](../references/interfaces.md)). + +When reporting completeness gaps (checks.completeness), distinguish between +missing steps and steps that exist but use a language-specific name (e.g., +Python uses `addV` but Go uses `AddV` — same step, different convention). When reporting divergence from the reference GLV, the question isn't "is it different?" — it's "is the difference justified by the host language?" A Go GLV using goroutines where Python uses asyncio is fine. A Go GLV using a different serialization format is a concern. -Coverage gaps in a GLV are expected for driver internals (connection -management, serialization) — but traversal step methods should have +Coverage gaps in a GLV (checks.coverageGaps) are expected for driver internals +(connection management, serialization) — but traversal step methods should have corresponding test coverage. ## Escape diff --git a/.skills/tinker-review/playbooks/grammar.md b/.skills/tinker-review/playbooks/grammar.md index dd8bb704b0..9a796e33a2 100644 --- a/.skills/tinker-review/playbooks/grammar.md +++ b/.skills/tinker-review/playbooks/grammar.md @@ -15,14 +15,13 @@ Identify which grammar rules were added or modified. Check: Link the proposal/discussion — grammar changes should always have prior community discussion. -## Checks -- completeness(grammarRule, ["in:has_rule"]) -- blast_radius(pr.modified(), 2) -- high_centrality(pr.modified()) - ## Interpret -Grammar changes have outsized blast radius by nature — the grammar -touches everything. Focus on whether the change is backwards compatible. +Read the structural signals from evidence.json (schema in +[references/interfaces.md](../references/interfaces.md)). Grammar changes have +outsized blast radius by nature (checks.blastRadius, checks.centrality) — the +grammar touches everything, so don't flag the reach itself; focus on backwards +compatibility. Completeness (checks.completeness on has_rule) shows whether new +rules are wired to a step. A new rule that adds syntax (existing queries still work) is low risk. A modified rule that changes parsing of existing syntax is high risk and diff --git a/.skills/tinker-review/playbooks/new-step.md b/.skills/tinker-review/playbooks/new-step.md index eb66769fb9..3cd04e5aca 100644 --- a/.skills/tinker-review/playbooks/new-step.md +++ b/.skills/tinker-review/playbooks/new-step.md @@ -24,24 +24,29 @@ For API design concerns (mined from TinkerPop reviewer patterns): - Class design: wrapping + extending the same parent is suspicious - Type restrictions should not be too narrow (provider implementations vary) -## Checks -- completeness(step, ["in:implements_step", "out:has_rule", "in:covers", "in:documents", "out:proposed_in"]) -- coverage_gaps(pr.tests(), pr.modified()) -- high_centrality(pr.modified()) -- blast_radius(pr.modified(), 3) - ## Interpret -A step missing from some GLVs is acceptable if tracked in a follow-up -issue — check if the PR or linked JIRA mentions phased rollout. Missing -documentation is not acceptable — a step without docs is undiscoverable. +Read the structural signals from evidence.json (schema in +[references/interfaces.md](../references/interfaces.md)). + +A new step's completeness (checks.completeness over implements_step / has_rule / +covers / documents / proposed_in) tells you what's missing. Missing from some +GLVs is acceptable if a follow-up issue tracks it — check the PR or linked JIRA +for phased rollout. Missing docs (no `documents` edge) is not acceptable — an +undocumented step is undiscoverable. Missing tests (checks.coverageGaps, no +`covers` edge) is a blocking gap — a new step must be exercised. + +A new step usually has low blast radius (checks.blastRadius) since nothing calls +it yet; a high value means it hooks into shared infrastructure — verify those +integration points. When comparing signatures across GLVs, parameter count should match but parameter types will differ by language. Focus on semantic equivalence, not syntactic identity. -High centrality in step infrastructure (TraversalStrategy, Step interface -implementations) is expected — these are shared abstractions. Flag it -for attention but don't treat it as a problem. +High centrality (checks.centrality) in step infrastructure (TraversalStrategy, +Step interface implementations) is expected — these are shared abstractions. +Flag it for attention but don't treat it as a problem. Ignore out-degree that's +just library calls (checks.externals, origin=library). ## Escape - if missing: proposal — "Cannot assess intent — need human to confirm expected semantics" diff --git a/.skills/tinker-review/playbooks/removal.md b/.skills/tinker-review/playbooks/removal.md index 19e45c2d73..a40dcd0d63 100644 --- a/.skills/tinker-review/playbooks/removal.md +++ b/.skills/tinker-review/playbooks/removal.md @@ -36,11 +36,12 @@ deletions). It runs in addition to `general.md` and any module playbook. 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 +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. + 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. diff --git a/.skills/tinker-review/references/interfaces.md b/.skills/tinker-review/references/interfaces.md index 33df7fd202..8d0fa4239d 100644 --- a/.skills/tinker-review/references/interfaces.md +++ b/.skills/tinker-review/references/interfaces.md @@ -68,54 +68,75 @@ interface ServerHandle { // === Graph Population === interface PopulationSummary { - vertices: number; - edges: number; - breakdown: { + vertices: number; // true count, queried from the graph after population + edges: number; // true count, queried from the graph after population + breakdown: { // per-type ATTEMPTED inserts (may exceed the real totals above) files: number; functions: number; types: number; + tests: number; calls: number; defines: number; - dependsOn: number; + testsEdges: number; + externalFunctions: number; // stub Functions minted for unresolved callees + stubFiles: number; // stub Files minted for changed files that weren't parsed }; } // === Pattern Results === - -interface CompletenessResult { - node: string; // vertex id or identifier checked - present: string[]; // edge labels that exist - missing: string[]; // edge labels that are absent - score: number; // present.length / (present.length + missing.length) -} - -interface CoverageGapResult { - uncovered: { - name: string; - signature: string; - filePath: string; - linesStart: number; - linesEnd: number; - }[]; - totalChanged: number; - totalCovered: number; -} - -// === Evidence Package (renderer input) === - -interface EvidencePackage { +// +// Each check's result type is defined — WITH per-field meaning — as a @typedef +// in the pattern file that produces it. Those typedefs are canonical; read them +// when interpreting a field. Do not re-declare them here (that is what drifts). +// +// CompletenessResult scripts/patterns/completeness.js +// CoverageGapResult scripts/patterns/coverage-gaps.js +// CentralityResult scripts/patterns/centrality.js +// BlastRadiusResult scripts/patterns/blast-radius.js +// ClusterResult scripts/patterns/cluster-analysis.js +// ConfidenceResult scripts/patterns/confidence-audit.js +// ExternalsResult scripts/patterns/classify-externals.js +// OrphanResult scripts/patterns/orphans.js +// ArchitectureResult scripts/patterns/architecture.js + +// === Evidence (evidence.json — what Phase 1 writes; the fields Interpret cites) === + +interface Evidence { meta: { pr: number; title: string; - domain: string; + domains: string[]; // e.g. ["general", "glv", "driver-server"] + language: string; + changedFileCount: number; timestamp: string; }; - summary: string; graphStats: PopulationSummary; + architecture: ArchitectureResult; checks: { completeness: CompletenessResult[]; coverageGaps: CoverageGapResult; + centrality: CentralityResult; + blastRadius: BlastRadiusResult; + clusters: ClusterResult; + confidence: ConfidenceResult; + externals: ExternalsResult; + orphans: OrphanResult; }; + discussions: DiscussionsResult; // jiras[], devList[], secondary[], proposals[], prComments{} + changedFiles: string[]; +} + +// === ReportPackage (report.json — renderer input; Evidence + agent narrative) === +// The agent adds these fields in Phase 5; render.js consumes the whole thing. + +interface ReportPackage extends Evidence { + summary: string; // HTML + clusters: { assessment: string }; // narrative prose — distinct from checks.clusters + guidedWalk: { title; badge; badgeText; body }[]; + findings: { title; snippet; body }[]; + openQuestions: { title; body; meta }[]; + functionalTest?: { plan; results: { name; pass; output }[]; observations }; + appendixFunctional?: { environment; testCode; fullOutput }; } ``` diff --git a/.skills/tinker-review/scripts/patterns/architecture.js b/.skills/tinker-review/scripts/patterns/architecture.js index 1a510ad414..9c772d881d 100644 --- a/.skills/tinker-review/scripts/patterns/architecture.js +++ b/.skills/tinker-review/scripts/patterns/architecture.js @@ -33,7 +33,19 @@ const { process: { t } } = gremlin; * @param {object} [params.clusterResult] - Output from clusterAnalysis() (connectedComponent clusters) * @param {boolean} [params.changedOnly] - Only include changed files (default: false) * @param {number} [params.maxNodes] - Cap on nodes to render (default: 40) - * @returns {Promise<{nodes: Array, edges: Array}>} + * @returns {Promise<ArchitectureResult>} + */ + +/** + * @typedef {Object} ArchitectureNode + * @property {string} id + * @property {string} label + * @property {string} cluster the cluster/community the node belongs to + * @property {boolean} changed whether the PR modified it + * + * @typedef {Object} ArchitectureResult + * @property {ArchitectureNode[]} nodes + * @property {{from: string, to: string}[]} edges directed dependency edges between nodes */ export async function architecture(g, params = {}) { const { clusterResult, changedOnly = false, maxNodes = 40 } = params; diff --git a/.skills/tinker-review/scripts/patterns/blast-radius.js b/.skills/tinker-review/scripts/patterns/blast-radius.js index be96bda19e..9e823e56bd 100644 --- a/.skills/tinker-review/scripts/patterns/blast-radius.js +++ b/.skills/tinker-review/scripts/patterns/blast-radius.js @@ -40,6 +40,25 @@ const INHERENTLY_CENTRAL = new Set([ * @param {boolean} [params.changedOnly] - Start from changed functions only (default: true) * @returns {Promise<BlastRadiusResult>} */ + +/** + * @typedef {Object} BlastRadiusFn + * @property {string} name + * @property {string} filePath + * @property {string} signature + * @property {number} linesStart + * @property {number} linesEnd + * @property {boolean} changed whether the PR modified this function + * @property {number} reachableCount callers reachable within `depth` hops upstream; high = the + * change ripples widely (for driver/server this is expected) + * @property {number} depth hop limit used for this row + * + * @typedef {Object} BlastRadiusResult + * @property {BlastRadiusFn[]} functions changed functions and how far each one's change reaches + * @property {number} maxReachable largest reachableCount across changed functions + * @property {number} totalWithCallers changed functions that have any upstream callers + * @property {number} depth hop limit applied + */ export async function blastRadius(g, params = {}) { const depth = params.depth || 3; const changedOnly = params.changedOnly !== false; diff --git a/.skills/tinker-review/scripts/patterns/centrality.js b/.skills/tinker-review/scripts/patterns/centrality.js index d891533270..2e67be304c 100644 --- a/.skills/tinker-review/scripts/patterns/centrality.js +++ b/.skills/tinker-review/scripts/patterns/centrality.js @@ -46,6 +46,27 @@ const INHERENTLY_CENTRAL = new Set([ * Only takes effect once classifyExternals has tagged `origin`. * @returns {Promise<CentralityResult>} */ + +/** + * @typedef {Object} Hotspot + * @property {string} name + * @property {string} filePath + * @property {string} signature + * @property {number} linesStart + * @property {number} linesEnd + * @property {boolean} changed whether the PR modified this function + * @property {number} inDegree incoming call edges (how many functions call it) + * @property {number} outDegree outgoing call edges, excluding origin:library targets + * @property {number} totalDegree inDegree + outDegree — the centrality score + * @property {boolean} inherentlyCentral a boilerplate method (equals/toString/…) central by + * nature; surfaced only when the PR modified it + * + * @typedef {Object} CentralityResult + * @property {Hotspot[]} hotspots top functions by totalDegree (>= minDegree) + * @property {number} totalAnalyzed functions considered + * @property {number} aboveThreshold how many cleared minDegree + * @property {number} filteredAsBoilerplate inherently-central, unchanged functions dropped + */ export async function highCentrality(g, params = {}) { const changedOnly = params.changedOnly !== false; const topN = params.topN || 10; diff --git a/.skills/tinker-review/scripts/patterns/classify-externals.js b/.skills/tinker-review/scripts/patterns/classify-externals.js index 20562362f1..e242d3f753 100644 --- a/.skills/tinker-review/scripts/patterns/classify-externals.js +++ b/.skills/tinker-review/scripts/patterns/classify-externals.js @@ -50,7 +50,15 @@ async function definedAsProjectType(name, repoPath) { * * @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[]}>} + * @returns {Promise<ExternalsResult>} + */ + +/** + * @typedef {Object} ExternalsResult + * @property {number} total external-callee stubs classified + * @property {string[]} library names judged JDK/accessor noise (dropped from centrality out-degree) + * @property {{name: string, definedIn: string}[]} project names a repo type declares, with the file + * @property {string[]} unresolved neither a known library name nor a project type — unknown */ export async function classifyExternals(g, repoPath) { const names = await g.V().hasLabel("Function").has("external", true).values("name").toList(); diff --git a/.skills/tinker-review/scripts/patterns/cluster-analysis.js b/.skills/tinker-review/scripts/patterns/cluster-analysis.js index f446dc754e..2017e3d69a 100644 --- a/.skills/tinker-review/scripts/patterns/cluster-analysis.js +++ b/.skills/tinker-review/scripts/patterns/cluster-analysis.js @@ -29,6 +29,19 @@ * @param {boolean} [params.changedOnly] - Only analyze changed files (default: true) * @returns {Promise<ClusterResult>} */ + +/** + * @typedef {Object} Cluster + * @property {number} id 1-based cluster index (largest first) + * @property {string[]} files file paths in this connected component + * @property {number} size files.length + * + * @typedef {Object} ClusterResult + * @property {number} clusterCount disconnected components among changed files + * @property {boolean} coherent true when the change is one logical unit (<= 1 cluster) + * @property {Cluster[]} clusters components, largest first + * @property {number} totalFiles changed files placed into clusters + */ export async function clusterAnalysis(a, params = {}) { const changedOnly = params.changedOnly !== false; diff --git a/.skills/tinker-review/scripts/patterns/completeness.js b/.skills/tinker-review/scripts/patterns/completeness.js index 788d6faf7c..a4630edaed 100644 --- a/.skills/tinker-review/scripts/patterns/completeness.js +++ b/.skills/tinker-review/scripts/patterns/completeness.js @@ -31,6 +31,14 @@ import gremlin from "gremlin"; * e.g., ["out:has_rule", "in:implements_step", "in:covers", "in:documents"] * @returns {Promise<CompletenessResult[]>} */ + +/** + * @typedef {Object} CompletenessResult + * @property {string} node the vertex checked (name or identifier) + * @property {string[]} present expected edge specs that exist (e.g. "out:defines") + * @property {string[]} missing expected edge specs that are absent — the gaps to weigh + * @property {number} score present / (present + missing), 0..1 + */ export async function completeness(g, params) { const { vertexLabel, vertexName, expectedEdges } = params; diff --git a/.skills/tinker-review/scripts/patterns/confidence-audit.js b/.skills/tinker-review/scripts/patterns/confidence-audit.js index db8442694f..29d6cb326f 100644 --- a/.skills/tinker-review/scripts/patterns/confidence-audit.js +++ b/.skills/tinker-review/scripts/patterns/confidence-audit.js @@ -47,7 +47,22 @@ function describeVertex(elementMap) { * @param {object} g - gremlin-js GraphTraversalSource (already connected) * @param {object} [params] * @param {number} [params.maxAmbiguous] - Cap the AMBIGUOUS edge list (default 50) - * @returns {Promise<{distribution: object, total: number, ambiguous: object[]}>} + * @returns {Promise<ConfidenceResult>} + */ + +/** + * @typedef {Object} EdgeConfidenceRow + * @property {string} relation edge label (calls, implements_step, addresses, …) + * @property {string} [confidence] the edge's confidence value + * @property {string} from described source vertex, e.g. "Function(foo)" + * @property {string} to described target vertex + * @property {string} [foundIn] provenance for discussion links (pr, search, …) + * @property {string} [foundVia] + * + * @typedef {Object} ConfidenceResult + * @property {{EXTRACTED:number, INFERRED:number, AMBIGUOUS:number, UNTAGGED:number}} distribution edge counts by confidence + * @property {number} total count of all edges + * @property {EdgeConfidenceRow[]} ambiguous AMBIGUOUS edges — flagged for human review */ export async function confidenceAudit(g, params = {}) { const maxAmbiguous = params.maxAmbiguous || 50; @@ -89,7 +104,7 @@ export async function confidenceAudit(g, params = {}) { * @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 } + * @returns {Promise<EdgeConfidenceRow[]>} */ export async function listEdgesByConfidence(g, opts = {}) { const { confidence, relation, limit = 100 } = opts; @@ -128,7 +143,7 @@ export async function listEdgesByConfidence(g, opts = {}) { * @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[]>} + * @returns {Promise<EdgeConfidenceRow[]>} */ export async function listInferred(g, params = {}) { return listEdgesByConfidence(g, { diff --git a/.skills/tinker-review/scripts/patterns/coverage-gaps.js b/.skills/tinker-review/scripts/patterns/coverage-gaps.js index b570fea2e7..20cee1b7de 100644 --- a/.skills/tinker-review/scripts/patterns/coverage-gaps.js +++ b/.skills/tinker-review/scripts/patterns/coverage-gaps.js @@ -27,6 +27,20 @@ import gremlin from "gremlin"; * @param {boolean} [params.changedOnly] - Only check functions with changed=true (default: true) * @returns {Promise<CoverageGapResult>} */ + +/** + * @typedef {Object} UncoveredFunction + * @property {string} name + * @property {string} signature + * @property {string} filePath + * @property {number} linesStart + * @property {number} linesEnd + * + * @typedef {Object} CoverageGapResult + * @property {UncoveredFunction[]} uncovered changed functions with no incoming `tests` edge + * @property {number} totalChanged changed functions considered + * @property {number} totalCovered changed functions that do have a test + */ export async function coverageGaps(g, params = {}) { const changedOnly = params.changedOnly !== false; diff --git a/.skills/tinker-review/scripts/patterns/orphans.js b/.skills/tinker-review/scripts/patterns/orphans.js index 2cafaa7021..b9ef966dd1 100644 --- a/.skills/tinker-review/scripts/patterns/orphans.js +++ b/.skills/tinker-review/scripts/patterns/orphans.js @@ -32,6 +32,19 @@ import gremlin from "gremlin"; * @param {boolean} [params.changedOnly] - Only check changed vertices (default: false) * @returns {Promise<OrphanResult>} */ + +/** + * @typedef {Object} Orphan + * @property {string} name + * @property {string} label vertex label checked (Function, Step, …) + * @property {string} filePath + * @property {string} missingEdge the expected relationship that is absent (e.g. "in:tests") + * + * @typedef {Object} OrphanResult + * @property {Orphan[]} orphaned vertices missing the expected relationship + * @property {number} totalChecked vertices examined + * @property {number} totalOrphaned orphaned.length + */ export async function orphans(g, params) { const { vertexLabel, expectedEdge } = params; const direction = params.direction || "in"; diff --git a/.skills/tinker-review/scripts/review.js b/.skills/tinker-review/scripts/review.js index 65863f08e5..09c42c491b 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 { orphans } from "./patterns/orphans.js"; import { createPrDiscussion } from "./enrichment/api.js"; import { discoverDiscussions } from "./discovery/discussions.js"; @@ -311,12 +312,14 @@ export async function phase1(session) { const blastResult = await blastRadius(g, { depth: 3, changedOnly: true }); const clusterResult = await clusterAnalysis(a, { changedOnly: true }); const confidenceResult = await confidenceAudit(g); + const orphansResult = await orphans(g, { vertexLabel: "Function", expectedEdge: "tests", direction: "in", changedOnly: true }); log(` completeness: ${completenessResults.filter(r => r.missing.length > 0).length} gaps found`); log(` coverage_gaps: ${coverageResult.uncovered.length} functions without tests`); log(` centrality: ${centralityResult.aboveThreshold} hotspots`); log(` blast_radius: max ${blastResult.maxReachable} reachable`); log(` clusters: ${clusterResult.clusterCount} (${clusterResult.coherent ? "coherent" : "fragmented"})`); log(` confidence: ${confidenceResult.distribution.EXTRACTED} extracted / ${confidenceResult.distribution.INFERRED} inferred / ${confidenceResult.distribution.AMBIGUOUS} ambiguous`); + log(` orphans: ${orphansResult.totalOrphaned} functions with no test`); log(`Generating architecture map...`); const architectureResult = await architecture(g, { clusterResult, changedOnly: true }); @@ -341,6 +344,7 @@ export async function phase1(session) { clusters: clusterResult, confidence: confidenceResult, externals: externalsResult, + orphans: orphansResult, }, discussions, changedFiles,
