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 ef7ec42aaa tinker-review: implement functional testing for PR review
ef7ec42aaa is described below
commit ef7ec42aaab69348651de9af135bb71693632a06
Author: Stephen Mallette <[email protected]>
AuthorDate: Thu Jul 9 17:08:59 2026 +0000
tinker-review: implement functional testing for PR review
Adds the deferred functional-test component: a blind subagent exercises the
built PR as a minimally-experienced user against a Gremlin Server compiled
from
source, and its findings feed the report.
- functional/{setup,cli}.js build the PR and launch a server from its own
artifacts; net.js shares port/readiness helpers with docker.js
- a sixth 'Verify' playbook section carries the gate and per-change battery
- the blind agent labels every scenario in code; the report surfaces themes
and
cites those labels, with full code in the appendix
- renderer strips stray code wrappers so raw-text fields render cleanly
Assisted-by: Claude Code:claude-opus-4-8
---
.skills/tinker-review/DESIGN.md | 24 ++-
.skills/tinker-review/SKILL.md | 101 +++++++---
.skills/tinker-review/playbooks/bug-fix.md | 11 ++
.skills/tinker-review/playbooks/driver-server.md | 13 ++
.skills/tinker-review/playbooks/general.md | 30 +++
.skills/tinker-review/playbooks/glv.md | 10 +
.skills/tinker-review/playbooks/grammar.md | 11 ++
.skills/tinker-review/playbooks/new-step.md | 11 ++
.skills/tinker-review/playbooks/removal.md | 11 ++
.../tinker-review/references/functional-testing.md | 96 +++++++++
.skills/tinker-review/references/interfaces.md | 4 +
.skills/tinker-review/scripts/functional/cli.js | 92 +++++++++
.skills/tinker-review/scripts/functional/setup.js | 218 +++++++++++++++++++++
.../tinker-review/scripts/infrastructure/docker.js | 41 +---
.../tinker-review/scripts/infrastructure/net.js | 78 ++++++++
.skills/tinker-review/scripts/renderer/render.js | 34 +++-
.skills/tinker-review/scripts/review.js | 17 +-
.../tinker-review/test/playbook-sections.test.js | 9 +-
18 files changed, 736 insertions(+), 75 deletions(-)
diff --git a/.skills/tinker-review/DESIGN.md b/.skills/tinker-review/DESIGN.md
index 72b345596a..130a3c655c 100644
--- a/.skills/tinker-review/DESIGN.md
+++ b/.skills/tinker-review/DESIGN.md
@@ -16,7 +16,10 @@ Gremlin Server container stays up.
**Phase 2 — agent-driven** does everything that needs judgment: enrich the
graph
via the enrichment CLI, an optional functional test, then the report. It ends
by
-tearing down the container and worktree.
+tearing down the container and worktree. The functional test itself splits
along
+the same mechanical/judgment line: `functional/setup.js` builds the PR and
starts
+a server from its artifacts (mechanical), while a blind subagent designs and
runs
+the test battery per the playbooks' Verify sections (judgment).
Data flows one direction through three artifacts:
@@ -38,6 +41,8 @@ has already exited.
| `scripts/graph/*.js` | populate the graph; `confidence.js` / `externals.js`
/ `references.js` hold the data-model vocabularies and shared edge helpers |
| `scripts/patterns/*.js` | one structural check per file; each defines its
own result `@typedef` |
| `scripts/enrichment/{api,cli}.js` | Phase 2 read/write commands over the
live graph |
+| `scripts/functional/{setup,cli}.js` | Phase 2 functional test — build the
PR, launch a server from its artifacts (mechanical half of step 4) |
+| `scripts/infrastructure/{docker,net}.js` | Phase 1 stock-image server
(`docker.js`); shared port/readiness helpers (`net.js`) |
| `scripts/renderer/{render.js,template.html}` | `report.json` → HTML |
| `playbooks/*.md` | domain review guidance — prompt scaffolds, not code |
| `references/{schema,interfaces}.md` | the graph schema and the evidence
composite |
@@ -77,11 +82,11 @@ 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** — five sections. **Context** is prose stating when the
+- **Add a playbook** — six 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:
+ four working sections — **Enrich**, **Inspect**, **Verify**, **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
@@ -92,6 +97,13 @@ has already exited.
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.
+ - **Verify** — each bullet shapes the optional functional test's battery for
+ this class of change: which GLVs/layers to exercise and what adversarial
cases
+ matter. The gate (whether to test at all) and the shared battery framework
+ live once in `general.md`'s Verify and must not be repeated per playbook; a
+ domain playbook adds only its specialization. If a change class rarely has
a
+ black-box surface, say when to skip in one line rather than inventing
items.
+ The mechanical build+start is `functional/setup.js`, not a playbook
concern.
- **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` /
@@ -101,7 +113,7 @@ has already exited.
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
+ that every playbook carries the six 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 6eaf5f5e6c..a1287a16ab 100644
--- a/.skills/tinker-review/SKILL.md
+++ b/.skills/tinker-review/SKILL.md
@@ -26,6 +26,7 @@ metadata:
- 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
@@ -83,12 +84,14 @@ checklists — one item per action.
| **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. |
-| **Interpret** | writing the report (step 5) | Weigh the named
`evidence.json` fields together with the Inspect candidates into `findings` /
`openQuestions`. |
+| **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 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
+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.
@@ -143,38 +146,75 @@ 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
-running Gremlin Server and submit traversals. Try to use the feature based
-only on what the documentation says. Try to break it with adversarial inputs.
+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:
-**Build the full project:**
```bash
-git worktree add /tmp/pr-review-<pr>/build pr-review/<pr>
-mvn -f /tmp/pr-review-<pr>/build/pom.xml clean install -DskipTests
+node .skills/tinker-review/scripts/functional/cli.js start --workDir
/tmp/pr-review-<pr>
```
-**Start Gremlin Server from built artifacts** on a random port (different from
-the knowledge graph server). Use the built assembly at:
-`/tmp/pr-review-<pr>/build/gremlin-server/target/apache-tinkerpop-gremlin-server-*-standalone/`
+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 Gremlin Server URL (the `url` from the command above)
The subagent does NOT get: source code, the knowledge graph, code review
findings,
-or access to the analysis worktree. It tests blind — like a user who read the
docs.
+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.
+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:**
+**Layer decision** (the Verify sections say which applies to this PR):
- **Layer 1 (embedded):** Gremlin Console with TinkerGraph. For core logic
changes.
- **Layer 2 (per-GLV wire):** Connect from each GLV to the server. For
serialization/type changes. Skip if purely computational.
-The subagent returns: test plan, results, adversarial findings, exact test
code.
+**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
@@ -200,11 +240,24 @@ produce a complete evidence-with-narrative JSON file.
Write it to
- `guidedWalk` — array of `{ title, badge, badgeText, 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)
-
-All `body` fields are HTML. Use `<code>`, `<strong>`, `<ul>`, `<p>` as needed.
-The renderer handles all layout, CSS, and structure.
+- `functionalTest` — `{ plan, results: [{name, pass, output}], observations }`
(if testing was done).
+ `plan` and `observations` are HTML 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` 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:
@@ -231,7 +284,9 @@ git branch -D pr-review/<pr>
```
This stops the knowledge graph server, removes worktrees, deletes the branch.
-Call this ONLY after all phases are complete.
+If a functional test ran (step 4), teardown also stops that server (via its
+`pid` in `functional.json`) and removes the `build/` worktree. Call this ONLY
+after all phases are complete.
## Important Notes
diff --git a/.skills/tinker-review/playbooks/bug-fix.md
b/.skills/tinker-review/playbooks/bug-fix.md
index 74290dfb81..8c95e63eb4 100644
--- a/.skills/tinker-review/playbooks/bug-fix.md
+++ b/.skills/tinker-review/playbooks/bug-fix.md
@@ -20,6 +20,17 @@ address the root cause, not just the symptom.
- Resource cleanup on error paths — if the bug involves connection/channel
handling, no leak when the fix triggers.
+## Verify
+- Reproduce the reported symptom first, then confirm the fix resolves it:
derive
+ the failing scenario from the linked issue and run it against the built
server.
+- Pick the layer by where the bug lives — an embedded Console/TinkerGraph
+ exercise for core logic; the affected GLV's native client for a driver/wire
bug.
+- Adversarial: nearby inputs the fix might have missed (the boundary just past
+ the reported case, the empty/null variant) — a fix that only patches the
exact
+ reported value is a finding.
+- If the bug has no black-box surface (e.g. an internal-only refactor of the
+ fix), state that; rely on the author's regression test instead.
+
## Interpret
- `checks.blastRadius` — high on a bug fix is a warning: verify the fix doesn't
subtly change behavior for existing callers.
diff --git a/.skills/tinker-review/playbooks/driver-server.md
b/.skills/tinker-review/playbooks/driver-server.md
index cdb92df3eb..55fba6045c 100644
--- a/.skills/tinker-review/playbooks/driver-server.md
+++ b/.skills/tinker-review/playbooks/driver-server.md
@@ -39,6 +39,19 @@ serialization / server init / auth) first, then check the
matching group.
- Server configuration uses gremlin-lang expressions, not Groovy scripts.
- No commented-out old code left behind — remove it cleanly.
+## Verify
+- Focus on the wire: connect a GLV client to the built server and round-trip
the
+ serializers or protocol paths the PR touches. Confirm the bytes survive both
+ directions (GraphBinary and GraphSON if both are affected).
+- If a type code was added or removed, confirm the new code path works and the
+ removed one fails cleanly rather than mis-deserializing.
+- Adversarial: a payload larger than a batch (exercise result streaming), a
+ server-side error (confirm it comes back as a usable error, per the
GraphBinary
+ → JSON error fallback), and a malformed request.
+- Pure connection-lifecycle or concurrency changes have no single-query
surface —
+ say so; the confidence here comes from the author's concurrency tests, not
this
+ black-box pass.
+
## Interpret
- `checks.blastRadius` — inherently high (shared infrastructure); don't flag
the
reach itself, name the specific callers most affected.
diff --git a/.skills/tinker-review/playbooks/general.md
b/.skills/tinker-review/playbooks/general.md
index 2f91d976a0..ce4f197ea5 100644
--- a/.skills/tinker-review/playbooks/general.md
+++ b/.skills/tinker-review/playbooks/general.md
@@ -41,9 +41,39 @@ inherits it. Run in order:
- Concurrency-implicated data structures (`CopyOnWriteArraySet`, synchronized
collections) introduced without profiling justification
+## Verify
+Context: this is the shared gate and battery-design framework for the optional
+functional test (SKILL.md step 4). Domain playbooks add their own Verify
bullets;
+they do not repeat this framework.
+
+**Gate — does functional testing run at all?** Run it only when the change has
a
+user-facing runtime surface. Skip (state why in `functionalTest`, or omit the
+field) when the change is tests-only, docs-only, a pure internal refactor with
no
+observable behavior change, or build/CI plumbing.
+
+**Design the battery to match the change** — exercise what changed, then try
the
+mistakes a real user would make:
+- New or changed **step / API surface** → submit native queries against the
built
+ server in every affected GLV. A step that spans grammar + core + all GLVs is
+ tested per language.
+- **Semantics changed, API stable** → a small embedded exercise (Layer 1:
Gremlin
+ Console / TinkerGraph, or a short Java snippet) that drives the feature is
+ enough; per-GLV wire tests add nothing.
+- **Serialization / type / protocol** → round-trip the affected types over the
+ wire from at least one GLV (Layer 2).
+- Always include adversarial cases: wrong argument types, empty/null inputs,
+ boundary values, and the feature used against the grain of the docs.
+
+The blind subagent designs and runs this battery from the docs alone — see
+SKILL.md step 4 for how it is briefed and isolated.
+
## Interpret
- `checks.coverageGaps` / `checks.orphans` — missing tests on changed code; a
test-quality concern, weighed alongside the Inspect smells.
+- `functionalTest` observations (if testing ran) — a failing or surprising
result
+ is a finding graded by severity; a documented-but-unusable feature is
blocking.
+ Adversarial gaps the subagent found (unclear errors, silent wrong answers)
are
+ high. If testing was skipped, the gate reason is not itself a finding.
- 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.
diff --git a/.skills/tinker-review/playbooks/glv.md
b/.skills/tinker-review/playbooks/glv.md
index 34f6c991c4..03f9ee5cec 100644
--- a/.skills/tinker-review/playbooks/glv.md
+++ b/.skills/tinker-review/playbooks/glv.md
@@ -26,6 +26,16 @@ recent accepted GLV).
error paths; the common GLV bug is leaking a connection when a traversal
fails
mid-execution.
+## Verify
+- Test from the GLV under review by connecting its native client to the built
+ server — this is the language whose wire behavior the PR changes.
+- Round-trip the value types the GLV serializes (numbers, lists, maps,
vertices,
+ the language's date/UUID types) and confirm they survive the trip unchanged.
+- Adversarial: submit a traversal that errors server-side and confirm the GLV
+ surfaces a usable error rather than hanging or leaking the connection.
+- If the change is idiomatic-only (no wire/serialization impact), an embedded
+ Java exercise is not required — state that in `functionalTest`.
+
## Interpret
- `checks.completeness` — distinguish genuinely missing steps from steps
present
under a language-specific name (Python `addV` vs Go `AddV` — same step).
diff --git a/.skills/tinker-review/playbooks/grammar.md
b/.skills/tinker-review/playbooks/grammar.md
index f402817cc0..03b476ca43 100644
--- a/.skills/tinker-review/playbooks/grammar.md
+++ b/.skills/tinker-review/playbooks/grammar.md
@@ -20,6 +20,17 @@ downstream tooling. Backwards compatibility is critical.
- New keywords — TinkerPop has special handling for keywords as map keys
(#3091);
a new keyword can break queries that use it as an identifier.
+## Verify
+- Submit a query using the new syntax against the built server and confirm it
+ parses and returns the intended result.
+- Backwards-compat is the priority: run a handful of pre-existing query forms
and
+ confirm the grammar change did not break them — especially if a rule was
+ modified rather than added.
+- If a new keyword was introduced, submit a query using that word as an
+ identifier / map key (the #3091 hazard) and confirm it still parses.
+- Test across the ANTLR targets the PR updates (Java, Python, Go) — the same
+ query should parse the same in each.
+
## Interpret
- `checks.blastRadius` / `checks.centrality` — grammar touches everything;
don't
flag the reach, focus on backwards compatibility.
diff --git a/.skills/tinker-review/playbooks/new-step.md
b/.skills/tinker-review/playbooks/new-step.md
index 6db570c2d7..fecbb64876 100644
--- a/.skills/tinker-review/playbooks/new-step.md
+++ b/.skills/tinker-review/playbooks/new-step.md
@@ -25,6 +25,17 @@ semantics.
- Cross-GLV signatures — parameter count should match; types differ by
language.
Judge semantic equivalence, not syntactic identity.
+## Verify
+- Test the step in every active GLV, not just Java — a new step must work
+ end-to-end per language. Submit native queries against the built server from
+ each GLV client.
+- Drive the step's documented signature: required args, optional args, and the
+ no-arg form if the docs describe one.
+- Adversarial: wrong argument types, the step in an illegal position (start vs
+ mid-traversal), and composition with a common neighbor step (`by`, `as`).
+- Cross-check observed behavior against the step's Gherkin feature — the
results
+ should agree with what the feature asserts.
+
## Interpret
- `checks.completeness` over `implements_step` / `has_rule` / `covers` /
`documents` / `proposed_in` — what's missing. Missing from some GLVs is
diff --git a/.skills/tinker-review/playbooks/removal.md
b/.skills/tinker-review/playbooks/removal.md
index d834f16e71..73ba2a6296 100644
--- a/.skills/tinker-review/playbooks/removal.md
+++ b/.skills/tinker-review/playbooks/removal.md
@@ -29,6 +29,17 @@ 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.
+## Verify
+- The build worktree compiling at all is the first signal — a removal that
left a
+ dangling reference fails `mvn install`, and `buildAndStart` reports it.
+- Confirm the removed surface is actually gone: submit a query using the
removed
+ step / feature against the built server and confirm it now errors rather than
+ silently working.
+- Confirm what remains still works: exercise a neighboring feature that shared
+ code with the removed one, to catch an over-broad deletion.
+- A removal with no user-facing runtime surface (internal class, build-only
+ dependency) needs no query test — say so; the reference analysis carries it.
+
## Interpret
The `references` edges in `checks.removalRefs` (plus any you added) and
`checks.coverageGaps` on surviving code are the primary outputs.
diff --git a/.skills/tinker-review/references/functional-testing.md
b/.skills/tinker-review/references/functional-testing.md
new file mode 100644
index 0000000000..9455146193
--- /dev/null
+++ b/.skills/tinker-review/references/functional-testing.md
@@ -0,0 +1,96 @@
+<!--
+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.
+-->
+
+# Functional Testing — Reference
+
+The mechanics behind SKILL.md step 4. *Whether* to run a functional test and
+*what* to test are judgment calls that live in the playbooks' **Verify**
sections
+(`general.md` holds the shared gate and battery framework; domain playbooks
+specialize). This file covers only the reproducible half: building the PR and
+standing up a server to test against.
+
+## Why a second server
+
+Phase 1's knowledge-graph server (`infrastructure/docker.js`) runs the **stock
+published image** — it holds the review graph, not the PR's code. Functional
+testing must exercise the PR's **own compiled artifacts**, so it builds from
+source and launches natively. The two servers run on different ports and never
+share state.
+
+## `functional/cli.js`
+
+| Command | What it does |
+|---------|--------------|
+| `start --workDir <dir> [--port <n>]` | Reads `pr`/`repoPath` from
`session.json`; adds a `build/` worktree on `pr-review/<pr>`; runs `mvn clean
install -DskipTests` over the full reactor; locates the `*-standalone`
assembly; writes a TinkerGraph config, an init script (binding `g` and `a`),
and a server yaml; launches `bin/gremlin-server.sh` on a free port; polls until
ready. Prints the handle as JSON and persists it to `functional.json`. |
+| `stop --workDir <dir>` | Reads `functional.json`, kills the server JVM, and
removes the `build/` worktree. Also invoked automatically by `review.js`
teardown. |
+
+The handle: `{ port, url, pid, buildWorktree, assemblyDir, logFile }`. `url` is
+the HTTP endpoint (`http://localhost:<port>/gremlin`) to hand the subagent.
+
+The build is the slow step (a full reactor build, minutes). If readiness times
+out, the error names `functional-server.log` in the work dir — inspect it for
the
+JVM's own startup errors.
+
+## What the built server exposes
+
+Mirrors the Phase-1 server so a reviewer's queries look the same against
either:
+
+- `g` — a standard traversal source over an empty TinkerGraph
+- `a` — the same graph `withComputer()`, for OLAP steps
(`connectedComponent()`, …)
+
+Serializers are the project defaults (GraphSON V4 + GraphBinary V4) over the
+`HttpChannelizer`, so any current GLV client connects normally.
+
+## Driving it from the subagent
+
+The subagent connects to `url` and submits traversals — via a GLV client or the
+Gremlin Console — exactly as a user reading the docs would. It gets the server
+URL, the PR title/description, the relevant docs, and the relevant Gherkin
+features; it does **not** get source, the graph, or the review findings. See
+SKILL.md step 4 for the full briefing contract and the Verify sections for the
+per-change battery.
+
+## What the subagent must return (report inputs)
+
+The test code is the source of truth for *what* was tested, so the subagent
+returns it **complete and unabbreviated**, with every scenario labeled in a
+comment carrying a stable id and one-line intent:
+
+```groovy
+// Scenario A1: group().by(T.id) — id key survives when its reduction is
productive
+g.V().group().by(id).by(out().count())
+```
+
+Those labels are the anchor between the report's two functional sections:
+
+- `appendixFunctional.testCode` — the full labeled battery (raw text; the
+ renderer wraps it — the subagent must not pre-wrap in `<pre>`/`<code>`).
+- `functionalTest` — **themes, not a scenario dump**. `results` rows are
+ theme-level, each citing the labels it spans (e.g. "Barrier family
(A4–A12)");
+ `plan`/`observations` surface what was exercised and what was learned. The
row
+ count must not imply more tests than the appendix lists.
+
+The renderer defensively strips a stray wrapper from the raw-text fields, but
the
+contract is raw text — relying on the guard is a slip, not a plan.
+
+## Isolation notes
+
+- The `build/` worktree is separate from the enrichment worktree (`src/`) so
+ Maven's `target/` output never pollutes the tree the agent reads.
+- Teardown removes the `build/` worktree and stops the JVM. If a run is
+ interrupted, `functional/cli.js stop` (or the next `start`, which prunes a
+ stale worktree first) cleans up.
diff --git a/.skills/tinker-review/references/interfaces.md
b/.skills/tinker-review/references/interfaces.md
index 8d0fa4239d..93f290c542 100644
--- a/.skills/tinker-review/references/interfaces.md
+++ b/.skills/tinker-review/references/interfaces.md
@@ -136,7 +136,11 @@ interface ReportPackage extends Evidence {
findings: { title; snippet; body }[];
openQuestions: { title; body; meta }[];
functionalTest?: { plan; results: { name; pass; output }[]; observations };
+ // plan/observations: HTML, theme-level. results rows are THEMES, each
`name`
+ // naming the scenario labels it spans — not one row per scenario.
appendixFunctional?: { environment; testCode; fullOutput };
+ // environment: HTML. testCode/fullOutput: RAW TEXT (renderer wraps in
+ // <pre><code>; do not pre-wrap). testCode is the COMPLETE labeled battery.
}
```
diff --git a/.skills/tinker-review/scripts/functional/cli.js
b/.skills/tinker-review/scripts/functional/cli.js
new file mode 100644
index 0000000000..7d8006418c
--- /dev/null
+++ b/.skills/tinker-review/scripts/functional/cli.js
@@ -0,0 +1,92 @@
+/*
+ * 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.
+ */
+
+// Thin CLI over functional/setup.js so SKILL.md step 4 can drive the
mechanical
+// build+start from one command, the way enrichment/cli.js drives the graph.
+// Reads session.json from --workDir for the pr/repo, prints the server handle
as
+// JSON, and persists it to functional.json so `stop` can find it later.
+//
+// node scripts/functional/cli.js start --workDir /tmp/pr-review-<pr>
+// node scripts/functional/cli.js stop --workDir /tmp/pr-review-<pr>
+
+import { readFile, writeFile } from "node:fs/promises";
+import { join } from "node:path";
+
+import { buildAndStart, stop } from "./setup.js";
+
+function parseArgs(argv) {
+ // A leading `--flag` means no subcommand was given (e.g. `--help`).
+ const command = argv[0] && !argv[0].startsWith("--") ? argv[0] : undefined;
+ const opts = {};
+ for (let i = command ? 1 : 0; i < argv.length; i++) {
+ if (argv[i].startsWith("--")) {
+ const key = argv[i].slice(2);
+ const val = argv[i + 1] && !argv[i + 1].startsWith("--") ? argv[++i] :
true;
+ opts[key] = val;
+ }
+ }
+ return { command, opts };
+}
+
+async function readSession(workDir) {
+ return JSON.parse(await readFile(join(workDir, "session.json"), "utf-8"));
+}
+
+async function main() {
+ const { command, opts } = parseArgs(process.argv.slice(2));
+ const workDir = opts.workDir;
+
+ if (!command || opts.help) {
+ process.stdout.write(
+ "Usage:\n" +
+ " cli.js start --workDir <dir> [--port <n>] build the PR and start
the functional server\n" +
+ " cli.js stop --workDir <dir> stop it and remove the
build worktree\n",
+ );
+ return;
+ }
+ if (!workDir) throw new Error("--workDir is required");
+
+ const session = await readSession(workDir);
+ const handlePath = join(workDir, "functional.json");
+
+ if (command === "start") {
+ const handle = await buildAndStart(workDir, {
+ pr: session.pr,
+ repoPath: session.repoPath,
+ port: opts.port ? Number(opts.port) : undefined,
+ });
+ await writeFile(handlePath, JSON.stringify(handle, null, 2));
+ process.stdout.write(JSON.stringify(handle, null, 2) + "\n");
+ return;
+ }
+
+ if (command === "stop") {
+ const handle = JSON.parse(await readFile(handlePath, "utf-8").catch(() =>
"null"));
+ if (handle) await stop(handle, { repoPath: session.repoPath });
+ process.stdout.write(JSON.stringify({ stopped: Boolean(handle) }) + "\n");
+ return;
+ }
+
+ throw new Error(`Unknown command: ${command}`);
+}
+
+main().catch((err) => {
+ process.stderr.write(`${err.message}\n`);
+ process.exit(1);
+});
diff --git a/.skills/tinker-review/scripts/functional/setup.js
b/.skills/tinker-review/scripts/functional/setup.js
new file mode 100644
index 0000000000..6a07592879
--- /dev/null
+++ b/.skills/tinker-review/scripts/functional/setup.js
@@ -0,0 +1,218 @@
+/*
+ * 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.
+ */
+
+// Mechanical setup for Phase-2 functional testing: build the PR from source
and
+// stand up a Gremlin Server from the freshly built assembly, so a blind
subagent
+// can exercise the change as a user would. This is the reproducible half of
+// step 4 — the judgment (what to test) lives in the playbooks' Verify
sections.
+//
+// Distinct from infrastructure/docker.js, which runs the STOCK published image
+// for the Phase-1 knowledge graph. Functional testing must run the PR's OWN
+// compiled code, so it builds and launches natively from `target/`.
+//
+// The build worktree is separate from the enrichment worktree
(`<workDir>/src`)
+// so Maven's `target/` output never pollutes the tree the agent reads during
+// enrichment.
+
+import { execFile, spawn } from "node:child_process";
+import { promisify } from "node:util";
+import { mkdir, writeFile, readdir } from "node:fs/promises";
+import { join } from "node:path";
+import { existsSync } from "node:fs";
+import { createWriteStream } from "node:fs";
+
+import { findAvailablePort, waitForHttp } from "../infrastructure/net.js";
+
+const exec = promisify(execFile);
+
+const DEFAULT_BUILD_TIMEOUT_MS = 30 * 60 * 1000; // full reactor build is slow
+const DEFAULT_READY_TIMEOUT_MS = 60 * 1000; // native JVM start + graph
load
+
+// TinkerGraph config for the functional server. Mirrors the stock empty graph.
+const GRAPH_PROPERTIES =
`gremlin.graph=org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph
+gremlin.tinkergraph.vertexIdManager=LONG
+`;
+
+// Init script binding the two traversal sources the reviewer may need — kept
in
+// lockstep with docker.js's INIT_GROOVY so both servers expose the same
surface:
+// 'g' — standard traversal
+// 'a' — withComputer() for OLAP steps like connectedComponent()
+const INIT_GROOVY = `def globals = [:]
+globals << [g : traversal().withEmbedded(graph)]
+globals << [a : traversal().withEmbedded(graph).withComputer()]
+`;
+
+/**
+ * Build a self-contained server yaml for the functional test. Based on the
+ * project's shipped default (HttpChannelizer + GraphSON V4 / GraphBinary V4)
+ * with host/port pinned and the g/a bindings wired via an init script. Written
+ * fresh rather than parsed from the assembly so there is no YAML dependency
and
+ * no textual patching of the shipped file.
+ */
+function serverYaml(port) {
+ return `host: localhost
+port: ${port}
+evaluationTimeout: 30000
+channelizer: org.apache.tinkerpop.gremlin.server.channel.HttpChannelizer
+graphs: {
+ graph: conf/tinkergraph-review.properties}
+scriptEngines: {
+ gremlin-lang: {},
+ gremlin-groovy: {
+ plugins: {
org.apache.tinkerpop.gremlin.server.jsr223.GremlinServerGremlinPlugin: {},
+ org.apache.tinkerpop.gremlin.jsr223.ImportGremlinPlugin:
{classImports: [java.lang.Math], methodImports: [java.lang.Math#*]},
+ org.apache.tinkerpop.gremlin.jsr223.ScriptFileGremlinPlugin:
{files: [scripts/review-init.groovy]}}}}
+serializers:
+ - { className:
org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV4, config: {
ioRegistries:
[org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerIoRegistryV3] }}
+ - { className:
org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV4 }
+ - { className:
org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV4, config: {
serializeResultToString: true }}
+metrics: {
+ slf4jReporter: {enabled: true, interval: 180000}}
+strictTransactionManagement: false
+`;
+}
+
+/**
+ * Locate the built standalone server assembly under the build worktree. The
+ * assembly directory carries the project version in its name, so it is globbed
+ * rather than hard-coded.
+ *
+ * @param {string} buildWorktree
+ * @returns {Promise<string>} absolute path to the *-standalone directory
+ */
+async function findAssembly(buildWorktree) {
+ const targetDir = join(buildWorktree, "gremlin-server", "target");
+ if (!existsSync(targetDir)) {
+ throw new Error(`No gremlin-server/target under ${buildWorktree} — did the
build succeed?`);
+ }
+ const entries = await readdir(targetDir);
+ const match = entries.find(
+ (e) => e.startsWith("apache-tinkerpop-gremlin-server-") &&
e.endsWith("-standalone"),
+ );
+ if (!match) {
+ throw new Error(`No *-standalone assembly in ${targetDir} (found:
${entries.join(", ") || "nothing"})`);
+ }
+ return join(targetDir, match);
+}
+
+/**
+ * @typedef {object} FunctionalHandle
+ * @property {number} port - localhost port the server listens on
+ * @property {string} url - base HTTP endpoint
(`http://localhost:<port>/gremlin`)
+ * @property {number} pid - PID of the server JVM (for teardown)
+ * @property {string} buildWorktree - the `<workDir>/build` worktree to remove
on teardown
+ * @property {string} assemblyDir - the built `*-standalone` directory
+ * @property {string} logFile - server stdout/stderr log for diagnosis
+ */
+
+/**
+ * Build the PR and start a Gremlin Server from the built artifacts.
+ *
+ * Steps: add a `build` worktree on the PR branch → `mvn clean install
+ * -DskipTests` over the full reactor → locate the server assembly → write the
+ * TinkerGraph config + init script + yaml → launch `bin/gremlin-server.sh`
+ * natively on a free port → poll until ready.
+ *
+ * The caller (SKILL.md step 4) decides whether to run this at all, per the
+ * playbooks' Verify gate. Skipped changes never pay the build cost.
+ *
+ * @param {string} workDir - the review work dir, e.g. `/tmp/pr-review-<pr>`
+ * @param {object} opts
+ * @param {number} opts.pr - PR number (the branch is `pr-review/<pr>`)
+ * @param {string} opts.repoPath - the git repo the worktree is added against
+ * @param {number} [opts.port] - fixed port (default: an OS-assigned free port)
+ * @param {number} [opts.buildTimeoutMs]
+ * @param {number} [opts.readyTimeoutMs]
+ * @returns {Promise<FunctionalHandle>}
+ */
+export async function buildAndStart(workDir, opts) {
+ const { pr, repoPath } = opts;
+ if (!pr) throw new Error("buildAndStart requires opts.pr");
+ if (!repoPath) throw new Error("buildAndStart requires opts.repoPath");
+
+ const prBranch = `pr-review/${pr}`;
+ const buildWorktree = join(workDir, "build");
+
+ // Add the build worktree (idempotent — remove a stale one first).
+ if (existsSync(buildWorktree)) {
+ await exec("git", ["worktree", "remove", "--force", buildWorktree], { cwd:
repoPath }).catch(() => {});
+ }
+ await exec("git", ["worktree", "prune"], { cwd: repoPath }).catch(() => {});
+ await exec("git", ["worktree", "add", buildWorktree, prBranch], { cwd:
repoPath });
+
+ // Full-reactor build without tests, so every module reflects the PR.
+ await exec(
+ "mvn",
+ ["-f", join(buildWorktree, "pom.xml"), "clean", "install", "-DskipTests"],
+ { cwd: buildWorktree, timeout: opts.buildTimeoutMs ||
DEFAULT_BUILD_TIMEOUT_MS, maxBuffer: 64 * 1024 * 1024 },
+ );
+
+ const assemblyDir = await findAssembly(buildWorktree);
+ const port = opts.port || await findAvailablePort();
+
+ // Write config into the assembly's conf/scripts dirs.
+ await mkdir(join(assemblyDir, "conf"), { recursive: true });
+ await mkdir(join(assemblyDir, "scripts"), { recursive: true });
+ await writeFile(join(assemblyDir, "conf", "tinkergraph-review.properties"),
GRAPH_PROPERTIES);
+ await writeFile(join(assemblyDir, "scripts", "review-init.groovy"),
INIT_GROOVY);
+ const yamlPath = join(assemblyDir, "conf", "gremlin-server-review.yaml");
+ await writeFile(yamlPath, serverYaml(port));
+
+ // Launch natively in the foreground ("console" mode reads the yaml arg via
the
+ // catch-all case) and detach, capturing output to a log for diagnosis.
+ const logFile = join(workDir, "functional-server.log");
+ const out = createWriteStream(logFile);
+ await new Promise((resolve) => out.once("open", resolve));
+
+ const child = spawn(
+ join(assemblyDir, "bin", "gremlin-server.sh"),
+ [yamlPath],
+ { cwd: assemblyDir, stdio: ["ignore", out, out], detached: true },
+ );
+ child.unref();
+
+ const handle = { port, url: `http://localhost:${port}/gremlin`, pid:
child.pid, buildWorktree, assemblyDir, logFile };
+
+ try {
+ await waitForHttp(port, opts.readyTimeoutMs || DEFAULT_READY_TIMEOUT_MS);
+ } catch (err) {
+ await stop(handle).catch(() => {});
+ throw new Error(`${err.message} — see ${logFile}`);
+ }
+
+ return handle;
+}
+
+/**
+ * Stop the functional server and remove its build worktree.
+ *
+ * @param {FunctionalHandle} handle
+ * @param {object} [opts]
+ * @param {string} [opts.repoPath] - repo to prune the worktree against
(defaults to none)
+ * @returns {Promise<void>}
+ */
+export async function stop(handle, opts = {}) {
+ if (handle?.pid) {
+ try { process.kill(handle.pid, "SIGTERM"); } catch { /* already gone */ }
+ }
+ if (handle?.buildWorktree && opts.repoPath) {
+ await exec("git", ["worktree", "remove", "--force", handle.buildWorktree],
{ cwd: opts.repoPath }).catch(() => {});
+ await exec("git", ["worktree", "prune"], { cwd: opts.repoPath }).catch(()
=> {});
+ }
+}
diff --git a/.skills/tinker-review/scripts/infrastructure/docker.js
b/.skills/tinker-review/scripts/infrastructure/docker.js
index 372072332b..a1e6023a18 100644
--- a/.skills/tinker-review/scripts/infrastructure/docker.js
+++ b/.skills/tinker-review/scripts/infrastructure/docker.js
@@ -19,16 +19,15 @@
import { execFile } from "node:child_process";
import { promisify } from "node:util";
-import { createServer } from "node:net";
-import { get } from "node:http";
import { mkdir, writeFile } from "node:fs/promises";
import { join } from "node:path";
+import { findAvailablePort, waitForHttp } from "./net.js";
+
const exec = promisify(execFile);
const DEFAULT_IMAGE = "tinkerpop/gremlin-server:3.8.1";
const DEFAULT_TIMEOUT_MS = 30000;
-const POLL_INTERVAL_MS = 500;
const INIT_GROOVY = `def globals = [:]
globals << [g : traversal().withEmbedded(graph)]
@@ -75,7 +74,7 @@ export async function startServer(options = {}) {
const handle = { port, containerId, url, configDir };
try {
- await waitForReady(port, timeoutMs);
+ await waitForHttp(port, timeoutMs);
} catch (err) {
await stopServer(handle).catch(() => {});
throw err;
@@ -94,37 +93,3 @@ export async function stopServer(handle) {
await exec("docker", ["stop", handle.containerId]).catch(() => {});
await exec("docker", ["rm", handle.containerId]).catch(() => {});
}
-
-function findAvailablePort() {
- return new Promise((resolve, reject) => {
- const srv = createServer();
- srv.listen(0, () => {
- const { port } = srv.address();
- srv.close(() => resolve(port));
- });
- srv.on("error", reject);
- });
-}
-
-function waitForReady(port, timeoutMs) {
- const deadline = Date.now() + timeoutMs;
-
- return new Promise((resolve, reject) => {
- function poll() {
- if (Date.now() > deadline) {
- reject(new Error(`Gremlin Server did not become ready within
${timeoutMs}ms`));
- return;
- }
-
- const req = get(`http://localhost:${port}/gremlin`, (res) => {
- res.resume();
- resolve();
- });
- req.on("error", () => {
- setTimeout(poll, POLL_INTERVAL_MS);
- });
- }
-
- poll();
- });
-}
diff --git a/.skills/tinker-review/scripts/infrastructure/net.js
b/.skills/tinker-review/scripts/infrastructure/net.js
new file mode 100644
index 0000000000..ead214e4a2
--- /dev/null
+++ b/.skills/tinker-review/scripts/infrastructure/net.js
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Shared networking helpers for the two Gremlin Servers a review stands up:
the
+// Phase-1 knowledge-graph server (docker.js) and the Phase-2 functional-test
+// server built from PR source (functional/setup.js). Both pick a free port and
+// poll the same HTTP readiness endpoint, so that logic lives here once.
+
+import { createServer } from "node:net";
+import { get } from "node:http";
+
+const POLL_INTERVAL_MS = 500;
+
+/**
+ * Ask the OS for a free TCP port by binding to 0 and reading it back.
+ * @returns {Promise<number>}
+ */
+export function findAvailablePort() {
+ return new Promise((resolve, reject) => {
+ const srv = createServer();
+ srv.listen(0, () => {
+ const { port } = srv.address();
+ srv.close(() => resolve(port));
+ });
+ srv.on("error", reject);
+ });
+}
+
+/**
+ * Poll an HTTP endpoint until it responds or the timeout elapses. Gremlin
+ * Server answers a plain GET on `/gremlin` once its WebSocket listener is up,
+ * so a successful response of any status means "ready".
+ *
+ * @param {number} port - localhost port to poll
+ * @param {number} timeoutMs - max wait before rejecting
+ * @param {object} [options]
+ * @param {string} [options.path] - request path (default "/gremlin")
+ * @returns {Promise<void>}
+ */
+export function waitForHttp(port, timeoutMs, options = {}) {
+ const path = options.path || "/gremlin";
+ const deadline = Date.now() + timeoutMs;
+
+ return new Promise((resolve, reject) => {
+ function poll() {
+ if (Date.now() > deadline) {
+ reject(new Error(`Server on port ${port} did not become ready within
${timeoutMs}ms`));
+ return;
+ }
+
+ const req = get(`http://localhost:${port}${path}`, (res) => {
+ res.resume();
+ resolve();
+ });
+ req.on("error", () => {
+ setTimeout(poll, POLL_INTERVAL_MS);
+ });
+ }
+
+ poll();
+ });
+}
diff --git a/.skills/tinker-review/scripts/renderer/render.js
b/.skills/tinker-review/scripts/renderer/render.js
index 58935effda..3fadcb096e 100644
--- a/.skills/tinker-review/scripts/renderer/render.js
+++ b/.skills/tinker-review/scripts/renderer/render.js
@@ -32,6 +32,31 @@ function esc(str) {
.replace(/"/g, """);
}
+/**
+ * Fields typed as raw *code* (appendixFunctional.testCode / .fullOutput) are
+ * wrapped by the renderer in its own `<pre><code>` and escaped. Agents
+ * sometimes pre-wrap the value in `<pre>`, `<code>`, or `<p>` anyway; escaping
+ * that renders the tags literally. Strip a single leading/trailing wrapper of
+ * those kinds so a slip degrades to clean text instead of visible markup. This
+ * is a lenient guard, not an HTML sanitizer — the field contract is raw text.
+ */
+function stripCodeWrapper(str) {
+ let s = String(str || "").trim();
+ const wrapper = /^<(pre|code|p)(?:\s[^>]*)?>([\s\S]*)<\/\1>$/i;
+ // Peel nested wrappers (e.g. `<pre><code>…</code></pre>`), up to two layers.
+ // Only peel an *unambiguous* single wrapper: if the inner content still
holds
+ // the same tag, the string is not one wrapper (e.g. two sibling `<code>`
+ // blocks), so leave it alone rather than over-strip.
+ for (let i = 0; i < 2; i++) {
+ const m = s.match(wrapper);
+ if (!m) break;
+ const inner = m[2].trim();
+ if (new RegExp(`</${m[1]}>`, "i").test(inner)) break;
+ s = inner;
+ }
+ return s;
+}
+
/**
* Render the full report from evidence data + agent narrative.
* Output matches the structure of reference-report.html exactly.
@@ -48,9 +73,11 @@ function esc(str) {
* clusters: { svg: "<svg>...</svg>", assessment: "HTML string" }
* guidedWalk: [{ title, badge: "attention|info|safe", badgeText, body:
"HTML" }]
* functionalTest: { plan: "HTML", results: [{name, pass, output}],
observations: ["HTML"] }
+ * (results rows are THEME-level, each naming the scenario labels it
covers)
* findings: [{ title, snippet: "code", body: "HTML" }]
* openQuestions: [{ title, body: "HTML", meta: "string" }]
- * appendixFunctional: { environment: "HTML", testCode: "code", fullOutput:
"code" }
+ * appendixFunctional: { environment: "HTML", testCode: "raw text",
fullOutput: "raw text" }
+ * (testCode = the COMPLETE labeled battery; renderer wraps it in
<pre><code>, do NOT pre-wrap)
*/
function notProvided(sectionId, title) {
return `<section id="${sectionId}">\n <h2>${esc(title)}</h2>\n <p
class="section-intro" style="color: var(--danger);">Section not
provided.</p>\n</section>`;
@@ -548,10 +575,11 @@ function renderAppendixFunctional(af) {
<div class="card">\n ${af.environment}\n </div>
<h3>Test Code</h3>
- <pre><code>${esc(af.testCode)}</code></pre>
+ <p class="section-intro">Complete, unabbreviated test battery. Each scenario
is labeled in a comment; the Functional Test section refers to these labels.</p>
+ <pre><code>${esc(stripCodeWrapper(af.testCode))}</code></pre>
<h3>Full Output</h3>
- <pre><code>${esc(af.fullOutput)}</code></pre>
+ <pre><code>${esc(stripCodeWrapper(af.fullOutput))}</code></pre>
</section>`;
}
diff --git a/.skills/tinker-review/scripts/review.js
b/.skills/tinker-review/scripts/review.js
index 21df3054d3..badcf72939 100644
--- a/.skills/tinker-review/scripts/review.js
+++ b/.skills/tinker-review/scripts/review.js
@@ -455,11 +455,12 @@ export async function phase1(session) {
// ============================================================
export async function teardown(sessionOrWorkDir) {
- let repoPath, worktreePath, prBranch, containerId;
+ let repoPath, worktreePath, prBranch, containerId, workDir;
if (typeof sessionOrWorkDir === "string") {
// Called with just a workDir path — read session.json
const { readFile: rf } = await import("node:fs/promises");
+ workDir = sessionOrWorkDir;
const sessionData = JSON.parse(await rf(join(sessionOrWorkDir,
"session.json"), "utf-8"));
repoPath = sessionData.repoPath;
worktreePath = sessionData.worktreePath;
@@ -468,6 +469,7 @@ export async function teardown(sessionOrWorkDir) {
} else {
// Called with a live session object
const session = sessionOrWorkDir;
+ workDir = session.workDir;
repoPath = session.repoPath;
worktreePath = session.worktreePath;
prBranch = session.prBranch;
@@ -476,6 +478,19 @@ export async function teardown(sessionOrWorkDir) {
if (session.connection) await session.connection.close().catch(() => {});
}
+ // Stop the functional-test server and remove its build worktree, if one was
+ // stood up (step 4 persists its handle to functional.json).
+ if (workDir) {
+ const { readFile: rf } = await import("node:fs/promises");
+ const handle = await rf(join(workDir, "functional.json"), "utf-8")
+ .then((s) => JSON.parse(s))
+ .catch(() => null);
+ if (handle) {
+ const { stop: stopFunctional } = await import("./functional/setup.js");
+ await stopFunctional(handle, { repoPath }).catch(() => {});
+ }
+ }
+
if (containerId) {
await exec("docker", ["stop", containerId]).catch(() => {});
await exec("docker", ["rm", containerId]).catch(() => {});
diff --git a/.skills/tinker-review/test/playbook-sections.test.js
b/.skills/tinker-review/test/playbook-sections.test.js
index 32a1f2976b..8f89cd345e 100644
--- a/.skills/tinker-review/test/playbook-sections.test.js
+++ b/.skills/tinker-review/test/playbook-sections.test.js
@@ -18,12 +18,13 @@
*/
// Structural conformance guard for the playbooks. Every playbook must carry
the
-// same five sections in the same order, split by data flow:
+// same six 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
+// Verify — functional-test battery + the gate deciding whether to run it
+// Interpret — weighs evidence.json checks (+ Inspect/Verify notes) into the
report
// Escape — stop/escalate gates
//
// It also fails if an Enrich section names no registered command (and doesn't
@@ -41,7 +42,7 @@ 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"];
+const CANONICAL = ["Context", "Enrich", "Inspect", "Verify", "Interpret",
"Escape"];
// An Enrich section that legitimately has no domain-specific graph write must
// say so with this phrase rather than silently naming nothing.
@@ -71,7 +72,7 @@ function sectionBody(md, name) {
return md.slice(start, end);
}
-test("every playbook carries the five canonical sections in order", async ()
=> {
+test("every playbook carries the six canonical sections in order", async () =>
{
for (const file of await playbookFiles()) {
const md = await readFile(join(playbooksDir, file), "utf-8");
const headers = sectionHeaders(md);
