tinker-review

# Graph Review Skill ## Prerequisites - Docker running (for Gremlin Server) - `upstream` remote pointing to `git@github.com:apache/tinkerpop.git` (fetch only) - Node.js 20+ with dependencies installed in `.skills/tinker-review/` - Beads (`bd`) installed, with the repo's beads database available (`bin/agent-setup.sh --contributor`). The review stops before fetching the PR when `bd` is missing. It runs `bd dolt pull` first and falls back to local state if the pull fails; it never writes to beads. ## References (load on demand) - Read [references/schema.md](references/schema.md) when you need to understand what vertices, edges, or properties exist in the knowledge graph (typically during enrichment or when writing raw Gremlin) - Read [references/interfaces.md](references/interfaces.md) when you need the exact function signatures or data type definitions for a module - Read [references/enrichment-cli.md](references/enrichment-cli.md) when you need to know what an enrichment CLI command *does* and when to reach for it (the command names below are terse; this is where their meaning lives) - Read [references/functional-testing.md](references/functional-testing.md) when you run the optional functional test (step 4) — what `functional/cli.js` does, how the built server is configured, and how to drive it from the subagent ## Execution Sequence When invoked with `/review <pr-number>`: The run has two phases (see [DESIGN.md](DESIGN.md)). **Phase 1** (step 1) is deterministic and builds the graph. **Phase 2** (steps 3–5) is agent-driven — enrichment, an optional functional test, then the report. Step 2 chooses the playbooks that guide Phase 2; step 6 tears down. ### 1. Setup + Phase 1 (deterministic) Run the review script. This handles everything mechanical: ```bash npm install --prefix .skills/tinker-review # only needed once node .skills/tinker-review/scripts/review.js <pr-number> <repo-path> ``` This performs: check for beads and pull it → fetch PR → create worktree → start Gremlin Server → extract structure via Tree-sitter → populate knowledge graph → discover discussions (JIRA, dev list, proposals, PR comments) → discover the PR's beads (records naming the PR or those discussions, their roots and subtrees) → run pattern checks (completeness, coverage gaps, centrality, blast radius, cluster analysis) → write evidence JSON. **Output:** `/tmp/pr-review-<pr>/evidence.json` **Server:** remains running (the agent needs it for enrichment) **Worktree:** available at `/tmp/pr-review-<pr>/src/` If re-running, the script clears the earlier run first (container, functional server, worktrees, and the work dir with its old evidence and report). ### 2. Choose the playbooks Playbooks carry the domain judgment for Phase 2. `general.md` always applies; select the domain playbooks in two passes: 1. **Orient** — from the changed file paths, gather the candidates: - `gremlin-dart/`, `gremlin-go/`, `gremlin-python/`, `gremlin-dotnet/`, `gremlin-js/`, `gremlin-javascript/` → `playbooks/glv.md` - `gremlin-core/` with new step patterns → `playbooks/new-step.md` - `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` 2. **Confirm** — read each candidate's **Context** and keep only the ones that truly fit this PR. A path can match a playbook that doesn't apply: a `gremlin-core/` change that adds no step, or a `gremlin-driver/` fix that's really a bug-fix. 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 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. | | **Verify** | functional testing (step 4) | Apply the gate, then shape the blind subagent's test battery. `general.md` holds the shared gate/framework; domains specialize it. | | **Interpret** | writing the report (step 5) | Weigh the named `evidence.json` fields together with the Inspect and Verify candidates into `findings` / `openQuestions`. | | **Escape** | any time | Honor its stop/escalate gates; halt or flag when one holds. | The working sections split by data flow: **Enrich** writes to the graph, **Inspect** reads the changed source into candidate findings, **Verify** exercises the built feature as a user would, and **Interpret** reads the computed `evidence.json` checks and weighs the Inspect and Verify 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 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 The Gremlin Server is still running. Use `scripts/enrichment/cli.js` to read from and write to the knowledge graph. Do NOT write your own connection code or import gremlin-js directly — the CLI handles connections for you. For what each command does and when to use it, see [references/enrichment-cli.md](references/enrichment-cli.md). Every command takes `--workDir /tmp/pr-review-<pr>`, connects using that dir's `session.json`, prints its JSON result to stdout, and disconnects: ```bash node scripts/enrichment/cli.js <command> --workDir /tmp/pr-review-<pr> [--options...] ``` The command catalog — what each does and when to reach for it — lives in [references/enrichment-cli.md](references/enrichment-cli.md); run `node scripts/enrichment/cli.js --help` for exact flags. In brief: **read commands** (`listFunctions`, `listDeleted`, `listExternalRefs`, …) orient you and feed the playbooks; **write commands** (`mapStep`, `addReference`, `setEdgeConfidence`, …) add the semantic edges. Edges you create default to `INFERRED` — pass `--confidence AMBIGUOUS` for a flagged guess or `EXTRACTED` 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`. 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 (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) **IMPORTANT: Functional testing is NOT running the project's unit tests.** DO NOT use `mvn test`. DO NOT run TreeTest, DO NOT run any existing test class. The project's own tests are the author's responsibility — running them tells the reviewer nothing new. Functional testing means: test the feature AS A USER WOULD. Connect to a Gremlin Server built from the PR and submit traversals. Try to use the feature based only on what the documentation says. Try to break it with adversarial inputs. **Gate — decide whether to run it.** Consult the applicable playbooks' **Verify** sections. `general.md`'s Verify holds the shared gate: skip functional testing when the change has no user-facing runtime surface (tests-only, docs-only, a pure internal refactor, build/CI plumbing) and either omit `functionalTest` or record the skip reason. Otherwise continue. **Build the PR and start the server (mechanical).** The `functional/cli.js` command builds the full reactor, locates the assembly, and launches a Gremlin Server from the built artifacts on a fresh port (distinct from the knowledge graph server). It reads `pr`/`repoPath` from `session.json` and writes the handle to `functional.json` so teardown can find it: ```bash node .skills/tinker-review/scripts/functional/cli.js start --workDir /tmp/pr-review-<pr> ``` This prints `{ url, port, pid, assemblyDir, logFile, ... }`. The build is slow (full reactor); on failure, the error names the server log to inspect. See [references/functional-testing.md](references/functional-testing.md) for what the command does and how to drive it. **Test from the outside.** Spawn a subagent that receives ONLY: - PR title/description (what the change claims to do) - Relevant documentation sections - Relevant Gherkin test features (for expected behavior reference) - The Gremlin Server URL (the `url` from the command above), or for Layer 1 the built console's `gremlin.sh` The subagent does NOT get: source code, the knowledge graph, the PR's beads, code review findings, or access to the analysis worktree. Brief it as a **minimally experienced TinkerPop user** who has only the docs — it tests blind, and its stumbles are signal about how usable the feature is. **The subagent tests by submitting Gremlin traversals** via the Gremlin Console or a GLV client. It devises its own test plan from the docs, executes traversals, and attempts adversarial edge cases. The **shape** of the battery comes from the applicable playbooks' Verify sections — which languages/layers to exercise and what adversarial cases matter for this class of change. **Layer decision** (the Verify sections say which applies to this PR): - **Layer 1 (embedded):** Gremlin Console with TinkerGraph. For core logic changes. Use the console `start` built; [references/functional-testing.md](references/functional-testing.md) gives its path and the script gotchas to pass to the subagent. - **Layer 2 (per-GLV wire):** Connect from each GLV to the server. For serialization/type changes. Skip if purely computational. **Label every scenario in the code (required).** Instruct the subagent to keep the test code as the single source of truth for *what* was tested, and to label each scenario with a comment carrying a stable id and a one-line intent, e.g.: ```groovy // Scenario A1: group().by(T.id) — validates the id key survives when its reduction is productive g.V().group().by(id).by(out().count()) ``` The subagent returns: - **Complete, unabbreviated test code**, every scenario labeled as above — this is `appendixFunctional.testCode` (raw text; the renderer wraps it — do NOT add `<pre>`/`<code>`). No `...`, no "(abbrev)"; if the battery is large, that is what the appendix is for. - **Themes, not a scenario dump** — group the scenarios into a handful of themes, each citing the labels it spans (e.g. "Barrier family (A4–A12): …"). - Results, adversarial findings, and observations keyed to those labels. Fold these into the `functionalTest` / `appendixFunctional` report fields (step 5) and weigh them in Interpret. The count and granularity of `functionalTest.results` rows must match the themes, not inflate to imply more tests than the appendix actually lists. ### 5. Phase 2 — Produce Report Write the narrative JSON, then render. **Step A:** Read `/tmp/pr-review-<pr>/evidence.json`. Add narrative fields to produce a complete evidence-with-narrative JSON file. Write it to `/tmp/pr-review-<pr>/report.json`. The narrative fields you must provide: - `summary` — HTML paragraph describing the PR - `clusters.assessment` — HTML prose about what the connected-component clusters mean (`evidence.json` carries `clusters: { assessment: null }` for it; the computed result is `checks.clusters`) - `communityAssessment` — HTML, **light by default**: usually one or two sentences on whether the change is coherent/localized and what its dominant theme is. Expand *only* when the community **structure itself** shows something non-obvious that no other section captures — e.g. disconnected communities hinting at bundled unrelated changes, a community bridging subsystems that shouldn't be coupled, a changed file stranded in a community about an unrelated concern, or diffuse modularity on a supposedly focused change. Absent such a structural surprise, keep it short and instead use the communities as *supporting evidence* cited in other sections (Guided Walk, Findings, Change Coherence). Do NOT restate the graph literally (vertices/edges/labels/counts), do NOT re-narrate what Removal References, Coverage, or Blast Radius already report, and do NOT pad. The renderer draws the diagram and the appendix lists membership; you supply only the judgment. - `communityNames` — object mapping each community's `id` (from `checks.communities.communities`) to a short name for what it is about, e.g. `{ "0": "Barrier step reduction", "1": "GraphBinary type IO" }`. Derive each name from the community's `keyMembers` and `files`; if those leave it unclear, query its members with `g.V().has("community", id)`. Aim for 2–5 words and at most 40 characters; the box truncates anything longer. Name the concern, not the structure: the renderer already shows the kind (`implementation`, `test code`, `type hierarchy`, `file group`) next to the name. Any community left unnamed renders as "Community N". - `guidedWalk` — array of `{ title, badge, badgeText, intro, questions, refs }` objects, one per area of the change, that **orient a reviewer who does not know the code**. Write for a first-time reader: say what the area does in plain terms, not as a tour of class and method names. - `intro` — HTML, one or two plain sentences on what this part of the PR does and why. - `questions` — `[{ text, refs }]`, the questions a reviewer should hold while reading this part. Each `text` (HTML) asks something the reviewer can check or judge (*"Can two graphs ever write to the same directory?"*, *"Is keeping index definitions outside the log reasonable?"*) and may cite what you found (a finding, a functional-test label). Invite judgment of trade-offs rather than asserting them. - `refs` on a question — the code that answers it, as `{ file, symbol?, lines?, label? }`: `file` is a repo path or unique suffix (`"TinkerTransaction.java"`); `symbol` a function, type or field name, or `"Type.member"` for a method or field of that type (`"TinkerStorageGraph.TinkerStorageGraph"` is the constructor; bare `"TinkerStorageGraph"` the class); `label` the chip text. Name the code — **never write line numbers or URLs**: the renderer resolves each ref against `evidence.codeIndex` and links to the file pinned at the reviewed commit with the symbol's lines highlighted. Two or three refs per question is plenty. - `refs` on the item — optional "Also see" chips for code that belongs to the whole area. - A ref can cite a bead instead of code: `{ bead: "tp-abc.3", label? }` links to that bead in the report's Project Memory. Use it where a question or finding rests on a recorded decision or task; it must be a bead in `discussions.beads.beads`. `render.js` prints any ref it cannot resolve; fix the `file`/`symbol` and re-render. An item with only an HTML `body` still renders (older reports). - `findings` — array of `{ title, snippet, body, refs? }` objects, ordered most-severe-first (Interpret grades each blocking / high / low); `refs` take the same code or bead refs as `guidedWalk` - `openQuestions` — array of `{ title, body, meta, refs? }` objects - `functionalTest` — `{ plan, results: [{name, pass, output}], observations }` (if testing was done). `plan` is HTML and `observations` is an array of HTML strings (one per insight); both and surface **themes and insights** — what families of behavior were exercised and what was learned — not a per-scenario list. `results` rows are **theme-level**: each `name` names a theme and the scenario labels it spans (e.g. `"Barrier family (A4–A12)"`), so the PASS/FAIL grid stays scannable and every row is backed by labeled code in the appendix. Do not enumerate every scenario here and do not let the row count imply more tests than the appendix lists. - `appendixFunctional` — `{ environment, testCode, fullOutput }` (if testing was done). `environment` is HTML. `testCode` and `fullOutput` are **raw text** — the renderer wraps them in `<pre><code>`, so do NOT add `<pre>`/`<code>`/`<p>` yourself. `testCode` is the **complete, unabbreviated** labeled battery from the subagent (see step 4); it is the source of truth the Functional Test section summarizes by label. All `body`, `intro` and question `text` fields (and `functionalTest.plan`/`observations`, `appendixFunctional.environment`) are HTML — use `<code>`, `<strong>`, `<ul>`, `<p>` as needed. The raw-text fields noted above are the exception. The renderer handles all layout, CSS, and structure. **Step B:** Render the report: ```bash