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
The following commit(s) were added to refs/heads/master by this push:
new a77173fa13 tinker-review: add the Inspect phase and single-source its
boilerplate
a77173fa13 is described below
commit a77173fa131206c48cc7b256af3f52c84815cf7b
Author: Stephen Mallette <[email protected]>
AuthorDate: Sat Jul 4 17:23:12 2026 -0400
tinker-review: add the Inspect phase and single-source its boilerplate
Playbooks gain a fifth section, Inspect — a bullet checklist of source-read
concerns recorded as candidate findings for Interpret to weigh at report
time,
splitting the working sections cleanly by data flow (Enrich writes the
graph,
Inspect reads the source, Interpret reads evidence.json). The repeated
"read the
changed source, record each as a candidate finding" and "weigh the signals
into
findings / openQuestions" preambles are now stated once in SKILL.md's
section
table instead of atop every playbook; each Inspect/Interpret section
carries only
its distinctive concerns, with an optional `Context:` lens line where
framing
helps. test/playbook-sections.test.js enforces the five sections in order
and
that Enrich names a registered command.
Assisted-by: Claude Code:claude-opus-4-8
---
.skills/tinker-review/DESIGN.md | 30 ++++++-
.skills/tinker-review/SKILL.md | 23 +++--
.skills/tinker-review/playbooks/bug-fix.md | 58 +++++-------
.skills/tinker-review/playbooks/driver-server.md | 82 ++++++++---------
.skills/tinker-review/playbooks/general.md | 66 +++++++-------
.skills/tinker-review/playbooks/glv.md | 70 +++++----------
.skills/tinker-review/playbooks/grammar.md | 38 ++++----
.skills/tinker-review/playbooks/new-step.md | 64 ++++++-------
.skills/tinker-review/playbooks/removal.md | 62 ++++++-------
.../tinker-review/test/playbook-sections.test.js | 100 +++++++++++++++++++++
10 files changed, 324 insertions(+), 269 deletions(-)
diff --git a/.skills/tinker-review/DESIGN.md b/.skills/tinker-review/DESIGN.md
index 07c493485f..72b345596a 100644
--- a/.skills/tinker-review/DESIGN.md
+++ b/.skills/tinker-review/DESIGN.md
@@ -77,9 +77,31 @@ has already exited.
- **Add an enrichment command** — a function in `scripts/enrichment/api.js`
(or a
pattern module), wired into `cli.js` (COMMANDS + help + switch), documented
in
`SKILL.md`. Edge-creating commands take a `confidence`, default `INFERRED`.
-- **Add a playbook** — four sections, each with a job: **Context** states when
- the playbook applies (an applicability gate, read while choosing playbooks),
- **Enrich** uses real enrichment commands, **Interpret** cites `evidence.json`
- fields, **Escape** sets stop/escalate gates. Add an orient rule in
`SKILL.md`.
+- **Add a playbook** — five sections. **Context** is prose stating when the
+ playbook applies (an applicability gate, read while choosing playbooks). The
+ three working sections — **Enrich**, **Inspect**, **Interpret** — are bullet
+ checklists, one item per action so the agent runs them dependably rather than
+ parsing prose. They split by data flow:
+ - **Enrich** — each bullet names a registered enrichment command and mutates
+ the graph only (name at least one command, or state that none applies).
+ - **Inspect** — each bullet names a source-read concern to record as a
+ *candidate finding* for Interpret. List only the concerns; the "read the
+ changed source, record each as a candidate finding" instruction is stated
+ once in `SKILL.md` and must not be repeated per playbook. An Inspect
section
+ may open with a single `Context:` line naming its lens (e.g. `Context:
+ API-design concerns mined from TinkerPop reviewer patterns`) when that
framing
+ guides the read. If a playbook has no source-read work (its judgment is
+ structural), say so in one line rather than inventing items.
+ - **Interpret** — each bullet weighs an `evidence.json` field or an Inspect
+ candidate into `findings` or `openQuestions`. List only the weighing
bullets;
+ the "weigh the evidence.json signals and Inspect candidates into
`findings` /
+ `openQuestions`" instruction is stated once in `SKILL.md` and must not be
+ repeated per playbook. `findings` is an ordered list: rank it
most-severe-first
+ and grade each entry blocking / high / low. Prominence is expressed by that
+ ordering, not a per-finding field.
+
+ **Escape** sets stop/escalate gates. `test/playbook-sections.test.js`
enforces
+ that every playbook carries the five sections in order and that Enrich names
a
+ command. Add an orient rule in `SKILL.md`.
- **Add an edge or vertex type** — document it in `references/schema.md`; tag
new
edges with `confidence`; use find-or-create for cross-boundary endpoints.
diff --git a/.skills/tinker-review/SKILL.md b/.skills/tinker-review/SKILL.md
index 1ea01dac48..d0479963ac 100644
--- a/.skills/tinker-review/SKILL.md
+++ b/.skills/tinker-review/SKILL.md
@@ -75,13 +75,23 @@ select the domain playbooks in two passes:
Then apply each kept playbook's sections at the point each is used:
+The three working sections (**Enrich**, **Inspect**, **Interpret**) are bullet
+checklists — one item per action.
+
| Section | Used when | What you do |
|---------|-----------|-------------|
| **Context** | choosing playbooks (above) | Confirm the path-matched playbook
fits this PR; set aside the ones that don't. |
-| **Enrich** | improving the graph (step 3) | Run its enrichment CLI commands
to add semantic edges. |
-| **Interpret** | writing the report (step 5) | Weigh the named
`evidence.json` fields into `findings` / `openQuestions`. |
+| **Enrich** | improving the graph (step 3) | Run each command bullet to add
or re-grade semantic edges. Graph mutation only. |
+| **Inspect** | reading the changed source (step 3) | Check each bullet
against the source; record what you find as a *candidate finding* for
Interpret. |
+| **Interpret** | writing the report (step 5) | Weigh the named
`evidence.json` fields together with the Inspect candidates into `findings` /
`openQuestions`. |
| **Escape** | any time | Honor its stop/escalate gates; halt or flag when one
holds. |
+The three working sections split by data flow: **Enrich** writes to the graph,
+**Inspect** reads the changed source into candidate findings, **Interpret**
reads
+the computed `evidence.json` checks and weighs the Inspect candidates into the
+report. `findings` is an ordered list — Interpret ranks it most-severe-first,
+grading each entry blocking / high / low.
+
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
@@ -116,11 +126,14 @@ when the source states it directly.
The confidence loop is the backbone of enrichment: `auditConfidence` →
`listInferred` (your verification worklist) → read the source →
`setEdgeConfidence`
to promote or downgrade, then re-run `auditConfidence` and reflect anything
still
-`AMBIGUOUS` in `openQuestions`. See `general.md`'s **Verify confidence**
section.
+`AMBIGUOUS` in `openQuestions`. It is defined once in `general.md`'s **Enrich**
+section and inherited by every playbook.
**Read source files:** worktree at `/tmp/pr-review-<pr>/src/`
-Follow the playbook's Enrich section. Check Escape conditions.
+Follow the playbook's Enrich section (graph writes) and its Inspect section
(read
+the changed source, recording concerns as candidate findings for Interpret to
+weigh at report time). Check Escape conditions.
### 4. Phase 2 — Functional Testing (optional, subagent)
@@ -174,7 +187,7 @@ produce a complete evidence-with-narrative JSON file. Write
it to
- `summary` — HTML paragraph describing the PR
- `clusters.assessment` — HTML prose about what the clusters mean
- `guidedWalk` — array of `{ title, badge, badgeText, body }` objects
-- `findings` — array of `{ title, snippet, body }` objects
+- `findings` — array of `{ title, snippet, body }` objects, ordered
most-severe-first (Interpret grades each blocking / high / low)
- `openQuestions` — array of `{ title, body, meta }` objects
- `functionalTest` — `{ plan, results: [{name, pass, output}], observations }`
(if testing was done)
- `appendixFunctional` — `{ environment, testCode, fullOutput }` (if testing
was done)
diff --git a/.skills/tinker-review/playbooks/bug-fix.md
b/.skills/tinker-review/playbooks/bug-fix.md
index fce7496a07..74290dfb81 100644
--- a/.skills/tinker-review/playbooks/bug-fix.md
+++ b/.skills/tinker-review/playbooks/bug-fix.md
@@ -6,44 +6,30 @@ regression test, and not introduce new API surface. The fix
should
address the root cause, not just the symptom.
## Enrich
-Identify the fix location — which functions were modified to address
-the bug. Trace from the PR title/description to understand the reported
-symptom. Check if the regression test actually reproduces that symptom.
-
-Look for:
-- Changes outside the issue's scope — modifications to functions not
- related to the reported bug may indicate scope creep
-- Error messages: are they meaningful to users, not just developers?
- (A common reviewer concern at TinkerPop)
-- Log level changes: error should remain error for unexpected failures,
- don't downgrade to info without justification
-- Resource cleanup on error paths: if the bug involves connection/channel
- handling, verify resources aren't leaked when the fix triggers
-
-If the PR references a JIRA ticket (TINKERPOP-XXXX), link it as a discussion.
+- `linkDiscussion` — record the referenced JIRA (TINKERPOP-XXXX) or dev-list
+ thread (`--source jira|devlist`). Creates the `addresses` edge Interpret
checks.
+
+## Inspect
+- Fix location — which functions changed to address the bug; does the
regression
+ test reproduce the reported symptom?
+- Scope creep — changes to functions unrelated to the reported bug.
+- Error messages — meaningful to users, not just developers? (a common
TinkerPop
+ reviewer concern)
+- Log levels — error stays error for unexpected failures; not downgraded to
info
+ without justification.
+- Resource cleanup on error paths — if the bug involves connection/channel
+ handling, no leak when the fix triggers.
## Interpret
-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."
+- `checks.blastRadius` — high on a bug fix is a warning: verify the fix doesn't
+ subtly change behavior for existing callers.
+- `checks.centrality` — if a hot function changed, say explicitly that every
+ caller needs a behavioral-change check.
+- `checks.coverageGaps` / `checks.orphans` — a fix with no new or modified test
+ is blocking; it can't be shown to prevent regression.
+- `checks.completeness` on `addresses` — no linked issue means correctness
can't
+ be assessed.
+- Out-of-scope changes — flag as "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 38d4cf8705..cdb92df3eb 100644
--- a/.skills/tinker-review/playbooks/driver-server.md
+++ b/.skills/tinker-review/playbooks/driver-server.md
@@ -7,62 +7,52 @@ here affect all users and all GLV drivers. Correctness under
concurrency
and backwards compatibility are critical concerns.
## Enrich
-Identify what layer of the driver/server stack is being modified:
-- Connection lifecycle (pooling, creation, cleanup)
-- Protocol handling (HTTP, WebSocket, request/response framing)
-- Serialization (GraphBinary, GraphSON, type registration)
-- Server initialization (configuration, script engines)
-- Authentication/authorization
+- `linkDoc` — if the change adds or removes a serializer type code, record the
+ documentation that covers that format (`--entity File --name <doc>`).
+- `linkDiscussion` — record a referenced proposal or JIRA.
-For each layer, look for:
+## Inspect
+Context: layered — identify the layer (connection lifecycle / protocol /
+serialization / server init / auth) first, then check the matching group.
**Connection management:**
-- Are HttpClient/WebSocket instances shared or created per-connection?
- (Shared is correct for pooling; per-connection defeats the pool)
+- HttpClient/WebSocket instances shared or created per-connection? (shared is
+ correct for pooling; per-connection defeats the pool)
- Could pool size = 1 cause deadlock?
-- Data structure choices for connection tracking: CopyOnWriteArraySet has
- write overhead, ConcurrentLinkedQueue is better for frequent insert/remove
-- Are settings kept as a cohesive object or extracted into individual fields?
- (Prefer the object — individual fields drift from the source of truth)
+- Connection-tracking data structures: `CopyOnWriteArraySet` has write
overhead;
+ `ConcurrentLinkedQueue` is better for frequent insert/remove.
+- Settings kept as a cohesive object, or extracted into individual fields that
+ drift from the source of truth? (prefer the object)
**Serialization:**
-- De-bulking: should happen lazily at traversal iteration, not eagerly when
- responses arrive (eager de-bulking puts all objects in memory at once)
-- Removed type IDs: if a serializer type code is removed, leave a comment
- documenting what it was ("122 was Bytecode until removed in 4.x")
-- Error response fallback: GraphBinary errors may come back as JSON if the
- server can't serialize the error in binary
-- Numeric types: longs need 'L' suffix in GremlinLang text format
+- De-bulking happens lazily at traversal iteration, not eagerly on response
+ arrival (eager puts all objects in memory at once).
+- Removed type IDs leave a comment documenting what they were ("122 was
Bytecode
+ until removed in 4.x").
+- Error-response fallback: GraphBinary errors may return as JSON when the
server
+ can't serialize the error in binary.
+- Numeric types: longs need an `L` suffix in GremlinLang text format.
**API migration:**
-- New code must use GremlinLang, not Bytecode (removed in 4.x)
-- New code must use with_(), not withRemote (deprecated)
-- Server configuration: gremlin-lang expressions, not Groovy scripts
-- Don't leave commented-out old code — remove it cleanly
+- New code uses GremlinLang, not Bytecode (removed in 4.x).
+- New code uses `with_()`, not `withRemote` (deprecated).
+- Server configuration uses gremlin-lang expressions, not Groovy scripts.
+- No commented-out old code left behind — remove it cleanly.
## Interpret
-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 (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.
-
-For "good enough for now" patterns (strategy handling, migration scaffolding),
-note them as acceptable if they're clearly marked as temporary, but flag if
-they look like they'll become permanent debt.
+- `checks.blastRadius` — inherently high (shared infrastructure); don't flag
the
+ reach itself, name the specific callers most affected.
+- `checks.centrality` with `listExternalRefs` — separate project coupling
+ (`origin: project`/`unresolved`) from library noise (`origin: library`);
+ centrality already drops library calls, so a function ranking high is
+ genuinely central.
+- `checks.coverageGaps` / `checks.orphans` in connection-lifecycle or
+ concurrency code — blocking; these are the hardest bugs to reproduce and the
+ most impactful in production.
+- Serialization type-code change with no `documents` edge from Enrich — high;
+ flag the missing IO/upgrade doc.
+- "Good enough for now" patterns (strategy handling, migration scaffolding) —
+ acceptable if clearly marked temporary; flag if they look like permanent
debt.
## Escape
- if connection pool logic modified without concurrency tests — "Pool changes
need concurrency testing — flag for manual review of deadlock/race conditions"
diff --git a/.skills/tinker-review/playbooks/general.md
b/.skills/tinker-review/playbooks/general.md
index c13eba4ffc..2f91d976a0 100644
--- a/.skills/tinker-review/playbooks/general.md
+++ b/.skills/tinker-review/playbooks/general.md
@@ -6,57 +6,51 @@ the type of change. This playbook always applies in addition
to any
domain-specific playbook.
## Enrich
-Look for these patterns in the changed code and annotate them:
-
-**Style violations:**
-- Wildcard imports in Java (import foo.*)
+The confidence pass runs on every review; it lives here and every playbook
+inherits it. Run in order:
+- `auditConfidence` — read the edge-confidence distribution and the
`AMBIGUOUS` list.
+- `listInferred` — pull the verification worklist (`--relation implements_step`
+ first, then any `calls` edges your findings lean on); read each against the
+ worktree source.
+- `setEdgeConfidence` — re-grade what you verified: promote a confirmed edge to
+ `EXTRACTED`, downgrade a wrong name-resolution to `AMBIGUOUS`.
+- `auditConfidence` again — anything still `AMBIGUOUS` goes to `openQuestions`,
+ never asserted as fact.
+
+## Inspect
+**Style:**
+- Wildcard imports in Java (`import foo.*`)
- Formatting/indentation changes mixed with functional changes
- Unused variables or imports
- Non-final variables that should be final
-**Deprecated API usage:**
-- Use of withRemote (deprecated in 4.0, use with_())
+**Deprecated API:**
+- `withRemote` (deprecated in 4.0, use `with_()`)
- Groovy script strings where gremlin-lang should be used
-- Any API marked @Deprecated being used in new code
+- Any `@Deprecated` API used in new code
-**Test concerns:**
+**Tests:**
- Tests that drop/clear all data instead of isolating with specialized labels
- Assertions that don't clearly explain what they verify
- Error/exception paths that aren't tested
- Test helpers without guard clauses (missing else/throw for invalid input)
**Resource safety:**
-- Connections, channels, or streams opened without clear cleanup paths
+- Connections, channels, or streams opened without a clear cleanup path
- Log levels: error for unexpected failures, info for expected lifecycle events
-- 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.
+- Concurrency-implicated data structures (`CopyOnWriteArraySet`, synchronized
+ collections) introduced without profiling justification
## 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.
-
-Formatting changes mixed with functional changes are worth flagging
-prominently — they make the PR harder to review and should ideally be
-separate commits.
-
-Deprecated API usage in NEW code is always a concern. Deprecated usage
-in MODIFIED code (that was already there) is lower priority unless the
-PR is specifically about migrating away from the deprecated API.
+- `checks.coverageGaps` / `checks.orphans` — missing tests on changed code; a
+ test-quality concern, weighed alongside the Inspect smells.
+- Safety concerns (resource leaks, concurrency risks, missing error handling)
+ and test-quality issues — high; make these the focus.
+- Style nits and unused variables — low; note them, don't let them dominate.
+- Formatting mixed with functional changes — high; it makes the PR harder to
+ review and should ideally be separate commits.
+- Deprecated API in new code — high. Deprecated API already present in modified
+ code — low, unless the PR is specifically a migration away from it.
## Escape
None — this playbook always completes. No conditions warrant stopping.
diff --git a/.skills/tinker-review/playbooks/glv.md
b/.skills/tinker-review/playbooks/glv.md
index ba9ce3308e..34f6c991c4 100644
--- a/.skills/tinker-review/playbooks/glv.md
+++ b/.skills/tinker-review/playbooks/glv.md
@@ -8,54 +8,32 @@ reference GLV for structural comparison is typically
gremlin-go (most
recent accepted GLV).
## Enrich
-Map public methods in the GLV to their canonical Gremlin steps. Only map
-methods that are actual traversal step implementations — the methods a user
-calls to build a traversal. Do NOT map inherited language boilerplate
-(toString, hashCode, equals, clone, close, etc.) or internal helper methods
-(getLocalChildren, setTraversal, getRequirements, etc.) to steps.
-
-A step implementation is typically:
-- A method on a traversal class that returns the traversal (fluent API)
-- Named to match the canonical step (cased per language convention)
-- Part of the public traversal DSL, not internal plumbing
-
-In Java specifically, the step *class* (e.g., TreeStep) contains internal
-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.
-Trace resource lifecycle through error paths — the common GLV bug is
-leaking connections when a traversal fails mid-execution.
+- `getCanonicalSteps` — pull the authoritative step vocabulary from
`Gremlin.g4`;
+ validate every step name against it before mapping.
+- `mapStep` — map each real traversal-step method to its canonical step. A step
+ method is on a traversal class, returns the traversal (fluent), is named to
+ match the canonical step (cased per language), and is public DSL — not
internal
+ plumbing. Never map boilerplate (`toString`, `equals`, `close`) or helpers
+ (`getLocalChildren`, `setTraversal`). Pass `--confidence AMBIGUOUS` when you
+ can't tell whether a method is a real step (it surfaces in the review list
+ instead of being asserted).
+- `listInferred --relation implements_step` then `setEdgeConfidence` — promote
a
+ mapping to `EXTRACTED` once confirmed against the reference GLV or grammar.
+- `linkDiscussion` — record a referenced JIRA or dev-list thread.
+
+## Inspect
+- Connection acquisition and release points — trace resource lifecycle through
+ error paths; the common GLV bug is leaking a connection when a traversal
fails
+ mid-execution.
## Interpret
-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 (checks.coverageGaps) are expected for driver internals
-(connection management, serialization) — but traversal step methods should have
-corresponding test coverage.
+- `checks.completeness` — distinguish genuinely missing steps from steps
present
+ under a language-specific name (Python `addV` vs Go `AddV` — same step).
+- Divergence from the reference GLV — judge whether the host language justifies
+ it. Goroutines where Python uses asyncio is fine; a different serialization
+ format is a concern.
+- `checks.coverageGaps` — expected for driver internals (connection management,
+ serialization); traversal-step methods should have test coverage.
## Escape
- if not test_suite_passes(glv): stop("Cannot proceed — GLV must pass test
suite first")
diff --git a/.skills/tinker-review/playbooks/grammar.md
b/.skills/tinker-review/playbooks/grammar.md
index 9a796e33a2..5aad50efe1 100644
--- a/.skills/tinker-review/playbooks/grammar.md
+++ b/.skills/tinker-review/playbooks/grammar.md
@@ -6,30 +6,26 @@ are inherently high-risk — they affect all parsers, all GLVs,
and all
downstream tooling. Backwards compatibility is critical.
## Enrich
-Identify which grammar rules were added or modified. Check:
-- Is this adding new syntax or modifying existing syntax?
-- If modifying: could existing valid Gremlin become invalid?
-- Are all ANTLR targets updated? (Java, Python, Go parsers)
-- Is there a corresponding step implementation for new grammar rules?
+- `addGrammarRule` — record each grammar rule the PR adds, so completeness can
+ check it's wired to a step.
+- `linkDiscussion --source proposal` — record the proposal or dev-list thread
+ (grammar changes need prior community consensus).
-Link the proposal/discussion — grammar changes should always have prior
-community discussion.
+## Inspect
+- New vs modified syntax — a modified rule that changes the parse of existing
+ syntax is the high-risk case: could valid Gremlin become invalid?
+- ANTLR targets — Java, Python, and Go parsers all updated?
+- Step wiring — is there a step implementation for each new rule?
+- New keywords — TinkerPop has special handling for keywords as map keys
(#3091);
+ a new keyword can break queries that use it as an identifier.
## Interpret
-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
-needs explicit backwards-compatibility analysis.
-
-Look for keywords being added — TinkerPop has specific handling for
-allowing keywords as map keys (#3091). New keywords can break existing
-queries that use them as identifiers.
+- `checks.blastRadius` / `checks.centrality` — grammar touches everything;
don't
+ flag the reach, focus on backwards compatibility.
+- `checks.completeness` on `has_rule` — shows whether the recorded rules are
+ wired to a step.
+- New-syntax rule (existing queries still parse) — low. Modified rule (changes
+ the parse of existing syntax) — high; needs explicit backwards-compat
analysis.
## Escape
- if no proposal or dev-list discussion found — "Grammar changes require
community consensus — flagging for discussion"
diff --git a/.skills/tinker-review/playbooks/new-step.md
b/.skills/tinker-review/playbooks/new-step.md
index 3cd04e5aca..75a4393ce6 100644
--- a/.skills/tinker-review/playbooks/new-step.md
+++ b/.skills/tinker-review/playbooks/new-step.md
@@ -7,46 +7,34 @@ The proposal (dev list or JIRA) is the source of truth for
intended
semantics.
## Enrich
-Link the step's core implementation to its canonical name. The step
-class (e.g., TreeStep) contains internal methods — only the method on
-GraphTraversal/GraphTraversalSource that users call should map to the step.
-
-Check for:
-- A linked proposal (TINKERPOP-XXXX in title/description, or dev list thread)
-- Whether the step appears in Gremlin.g4 (grammar rule)
-- Whether GLV implementations exist for the step
-- Whether documentation references the step
-
-For API design concerns (mined from TinkerPop reviewer patterns):
-- Default implementations on interfaces need justification — if something
- implements an interface, should it have a proper implementation?
-- Naming should be consistent with existing patterns (look at sibling steps)
-- Class design: wrapping + extending the same parent is suspicious
-- Type restrictions should not be too narrow (provider implementations vary)
+- `getCanonicalSteps` — validate the step name before mapping.
+- `mapStep` — link the core implementation to its canonical name. Map the
+ `GraphTraversal`/`GraphTraversalSource` method users call, not the internal
+ methods on the Step class (e.g., `TreeStep`).
+- `linkDoc` — record the documentation that references the step.
+- `linkDiscussion` — record the proposal or JIRA that defines the step's
semantics.
+
+## Inspect
+- Default interface implementations — justified? If something implements an
+ interface, should it have a proper implementation?
+- Naming — consistent with sibling steps?
+- Class design — wrapping and extending the same parent is suspicious.
+- Type restrictions — not too narrow (provider implementations vary).
+- Cross-GLV signatures — parameter count should match; types differ by
language.
+ Judge semantic equivalence, not syntactic identity.
## Interpret
-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 (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).
+- `checks.completeness` over `implements_step` / `has_rule` / `covers` /
+ `documents` / `proposed_in` — 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`) — blocking; an undocumented
+ step is undiscoverable. Missing tests (`checks.coverageGaps`, no `covers`) —
+ blocking; a new step must be exercised.
+- `checks.blastRadius` — usually low (nothing calls a new step yet); high means
+ it hooks into shared infrastructure — verify those integration points.
+- `checks.centrality` — high in step infrastructure (TraversalStrategy, Step
+ implementations) is expected; note it, 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 b68574cf9c..d834f16e71 100644
--- a/.skills/tinker-review/playbooks/removal.md
+++ b/.skills/tinker-review/playbooks/removal.md
@@ -12,45 +12,33 @@ 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. **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`)
-
- 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.
+- `listDeleted` — the removed files and the symbol each likely defined
+ (`Krb5Authenticator.java` -> `Krb5Authenticator`). The entry point; its
results
+ are the valid `--toPath` targets for `addReference`.
+- Read `checks.removalRefs` (and `checks.removalRefs.externalCallers`) — Phase
1
+ already grepped the surviving worktree for each removed code symbol and
wrote a
+ `references` edge per hit. Once you've classified each (see Interpret),
confirm
+ or downgrade it with `setEdgeConfidence`.
+- `addReference` — record the non-code hits the automatic pass can't see,
grepped
+ off `listDeleted`: config/resources (`*.yaml`, `*.conf`, `*.properties`),
build
+ files (`pom.xml`, `*.gradle`), docs (`docs/src/**/*.asciidoc`,
`CHANGELOG.asciidoc`),
+ Docker/CI setup (`*.sh`, compose files).
+
+## Inspect
+None specific to removal — the review judgment here is structural (classifying
+the `references` edges recorded in Enrich) and is handled in Interpret rather
+than by reading changed source.
## Interpret
-Read the structural signals from evidence.json (schema in
-[references/interfaces.md](../references/interfaces.md)); the `references`
edges
-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
- **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.
+The `references` edges in `checks.removalRefs` (plus any you added) and
+`checks.coverageGaps` on surviving code are the primary outputs.
+Classify each surviving reference and state where it lives:
+- Active code / build / config / live docs referencing a removed symbol —
+ blocking; 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 — expected and correct; note as
verified-benign.
+- No current upgrade/CHANGELOG line announcing the removal — a finding; users
+ need to know the feature is gone.
## Escape
- if a removed symbol is still referenced by active source or build files —
diff --git a/.skills/tinker-review/test/playbook-sections.test.js
b/.skills/tinker-review/test/playbook-sections.test.js
new file mode 100644
index 0000000000..32a1f2976b
--- /dev/null
+++ b/.skills/tinker-review/test/playbook-sections.test.js
@@ -0,0 +1,100 @@
+/*
+ * 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.
+ */
+
+// Structural conformance guard for the playbooks. Every playbook must carry
the
+// same five sections in the same order, split by data flow:
+//
+// Context — applicability gate
+// Enrich — graph mutation only; names real enrichment CLI commands
+// Inspect — reads changed source into findings (no graph representation)
+// Interpret — weighs evidence.json checks (+ Inspect notes) into the report
+// Escape — stop/escalate gates
+//
+// It also fails if an Enrich section names no registered command (and doesn't
+// explicitly declare that none applies) — the drift that let playbooks gesture
+// at "link it as a discussion" without ever naming `linkDiscussion`.
+
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { readFile, readdir } from "node:fs/promises";
+import { fileURLToPath } from "node:url";
+import { dirname, join } from "node:path";
+
+import { COMMANDS } from "../scripts/enrichment/cli.js";
+
+const here = dirname(fileURLToPath(import.meta.url));
+const playbooksDir = join(here, "..", "playbooks");
+
+const CANONICAL = ["Context", "Enrich", "Inspect", "Interpret", "Escape"];
+
+// An Enrich section that legitimately has no domain-specific graph write must
+// say so with this phrase rather than silently naming nothing.
+const NO_ENRICHMENT = /\bnone\b.*\bappl/i;
+
+const agentCommands = Object.keys(COMMANDS).filter(
+ (c) => COMMANDS[c].facing === "agent",
+);
+
+async function playbookFiles() {
+ const entries = await readdir(playbooksDir);
+ return entries.filter((f) => f.endsWith(".md"));
+}
+
+/** The level-2 headers of a markdown doc, in document order. */
+function sectionHeaders(md) {
+ return [...md.matchAll(/^##\s+(.+?)\s*$/gm)].map((m) => m[1]);
+}
+
+/** The body of one `## <name>` section, up to the next `## ` header or EOF. */
+function sectionBody(md, name) {
+ const headers = [...md.matchAll(/^##\s+(.+?)\s*$/gm)];
+ const idx = headers.findIndex((h) => h[1] === name);
+ if (idx === -1) return "";
+ const start = headers[idx].index + headers[idx][0].length;
+ const end = idx + 1 < headers.length ? headers[idx + 1].index : md.length;
+ return md.slice(start, end);
+}
+
+test("every playbook carries the five canonical sections in order", async ()
=> {
+ for (const file of await playbookFiles()) {
+ const md = await readFile(join(playbooksDir, file), "utf-8");
+ const headers = sectionHeaders(md);
+ assert.deepEqual(
+ headers,
+ CANONICAL,
+ `${file} must have exactly these level-2 sections in order:
${CANONICAL.join(", ")} (found: ${headers.join(", ") || "none"})`,
+ );
+ }
+});
+
+test("every Enrich section names a registered command or declares none
applies", async () => {
+ for (const file of await playbookFiles()) {
+ const md = await readFile(join(playbooksDir, file), "utf-8");
+ const enrich = sectionBody(md, "Enrich");
+ assert.ok(enrich.trim(), `${file} has an empty Enrich section`);
+
+ const named = agentCommands.filter((c) =>
+ new RegExp(`\\b${c}\\b`).test(enrich),
+ );
+ assert.ok(
+ named.length > 0 || NO_ENRICHMENT.test(enrich),
+ `${file} Enrich names no registered enrichment command and doesn't state
that none applies`,
+ );
+ }
+});
