This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit b7728784a9b3468a37ee5c4fe7bb2f1ff10367b2 Author: Stephen Mallette <[email protected]> AuthorDate: Fri Oct 9 15:44:16 2026 -0400 Fix the tinker-review functional server on master The server config no longer sets metrics, whose setting names differ by branch. A failed build or start removes its build worktree, and embedded testing through the built console is documented. Assisted-by: Claude Code:claude-opus-5-5 --- .skills/tinker-review/SKILL.md | 7 ++-- .../tinker-review/references/functional-testing.md | 39 ++++++++++++++++++++-- .skills/tinker-review/scripts/functional/cli.js | 3 ++ .skills/tinker-review/scripts/functional/setup.js | 30 ++++++++++------- 4 files changed, 62 insertions(+), 17 deletions(-) diff --git a/.skills/tinker-review/SKILL.md b/.skills/tinker-review/SKILL.md index 80b0c41d93..4f370c86c6 100644 --- a/.skills/tinker-review/SKILL.md +++ b/.skills/tinker-review/SKILL.md @@ -180,7 +180,8 @@ command does and how to drive it. - 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) +- 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 @@ -194,7 +195,9 @@ 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. +- **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. diff --git a/.skills/tinker-review/references/functional-testing.md b/.skills/tinker-review/references/functional-testing.md index 9455146193..a69d915f6b 100644 --- a/.skills/tinker-review/references/functional-testing.md +++ b/.skills/tinker-review/references/functional-testing.md @@ -64,6 +64,34 @@ 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. +## Layer 1 — embedded, through the built console + +When the Verify sections call for an embedded exercise (semantics changed, API +stable), there is no server to start: the subagent drives the PR's own console +against an in-process TinkerGraph. `start` has already built it — the console +assembly sits next to the server's in the build worktree: + +```bash +<buildWorktree>/gremlin-console/target/apache-tinkerpop-gremlin-console-*-standalone/bin/gremlin.sh -e scenarios.groovy +``` + +`-e` runs the script in execution mode and returns. Hand the subagent the path +to `gremlin.sh` (with the `*` resolved) in place of a server URL, and these +gotchas, which otherwise cost it a few failed runs: + +- **No top-level `def`.** The console evaluates a script statement by statement, + so a `def`-declared variable is local to its statement and gone by the next + line. Assign without `def` (`g = TinkerGraph.open().traversal()`), as the reference + docs' execution-mode examples do. `def` inside a closure or method is fine. +- **End with `System.exit(0)`** if the console is still running after the last + statement — a non-daemon thread (a remote connection, an executor) keeps the + JVM alive. +- **Print what you assert.** Output is the only record: `println` each result, + or use `assert`, which stops the script on the first failure — so label every + scenario and print before asserting. + +The same scenario labels and complete-code rules below apply to a console script. + ## What the subagent must return (report inputs) The test code is the source of truth for *what* was tested, so the subagent @@ -91,6 +119,11 @@ contract is raw text — relying on the guard is a slip, not a plan. - 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. +- Teardown removes the `build/` worktree and stops the JVM. `start` records the + worktree in `functional.json` before it builds, and removes it itself when the + build or server start fails, so a failed or interrupted start never leaves it + behind for teardown to miss. +- The build worktree is checked out from the PR, so the server config written + into it must suit the PR's branch. It sets only what a review needs; metrics + are left out because their setting names differ between branches (master + renamed `interval` to `intervalMillis`). diff --git a/.skills/tinker-review/scripts/functional/cli.js b/.skills/tinker-review/scripts/functional/cli.js index 7d8006418c..88c06ad49c 100644 --- a/.skills/tinker-review/scripts/functional/cli.js +++ b/.skills/tinker-review/scripts/functional/cli.js @@ -66,6 +66,9 @@ async function main() { const handlePath = join(workDir, "functional.json"); if (command === "start") { + // Record the build worktree before building, so teardown can remove it even + // if this process dies mid-build. + await writeFile(handlePath, JSON.stringify({ buildWorktree: join(workDir, "build") }, null, 2)); const handle = await buildAndStart(workDir, { pr: session.pr, repoPath: session.repoPath, diff --git a/.skills/tinker-review/scripts/functional/setup.js b/.skills/tinker-review/scripts/functional/setup.js index 566d26d5e9..7f3232afa8 100644 --- a/.skills/tinker-review/scripts/functional/setup.js +++ b/.skills/tinker-review/scripts/functional/setup.js @@ -82,8 +82,6 @@ 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 `; } @@ -158,6 +156,23 @@ export async function buildAndStart(workDir, opts) { // refuses to check out one branch in two worktrees. await exec("git", ["worktree", "add", "--detach", buildWorktree, prBranch], { cwd: repoPath }); + // From here on, any failure stops the server (if it started) and removes the + // build worktree, so a failed start leaves nothing for teardown to miss. + let handle = { buildWorktree }; + try { + handle = await buildAndLaunch(workDir, buildWorktree, opts); + await waitForHttp(handle.port, opts.readyTimeoutMs || DEFAULT_READY_TIMEOUT_MS) + .catch((err) => { throw new Error(`${err.message} — see ${handle.logFile}`); }); + } catch (err) { + await stop(handle, { repoPath }).catch(() => {}); + throw err; + } + return handle; +} + +// Build the reactor in the build worktree, write the server config into the +// assembly, and launch it. Returns the handle; readiness is the caller's job. +async function buildAndLaunch(workDir, buildWorktree, opts) { // Full-reactor build without tests, so every module reflects the PR. await exec( "mvn", @@ -189,16 +204,7 @@ export async function buildAndStart(workDir, opts) { ); 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; + return { port, url: `http://localhost:${port}/gremlin`, pid: child.pid, buildWorktree, assemblyDir, logFile }; } /**
