From 2b613cb2386c2ceafa22a1f5c677b5c75530d82e Mon Sep 17 00:00:00 2001
From: Ralf Anton Beier <ralf_beier@me.com>
Date: Mon, 27 Apr 2026 05:54:59 +0200
Subject: [PATCH] fix: rivet binary path resolution + drop fragile .issues
 guard
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Two production bugs surfaced by today's live test

### Bug 1 — rivet binary path looked up in the wrong directory
Logs from netcup at 20:14:31 UTC:

  err: "spawn data/rivet/rivet ENOENT"
  msg: "rivet validate failed to spawn"

\`runRivetOracle\` calls \`execFile(binary, args, { cwd: repoPath })\` where
\`repoPath\` is the freshly-extracted PR tarball. A *relative* binary_path
is then resolved against THAT cwd — not the temper deploy root. So Node
looked for \`<tmpdir>/data/rivet/rivet\` and never finds the real binary at
\`/opt/temper/data/rivet/rivet\`.

Fix: in \`src/ai-review.js\`, resolve a relative \`binary_path\` against
\`__dirname/..\` (the temper repo root) before passing it to the oracle.
Absolute paths are passed through unchanged.

### Bug 3 — \`/review-pr\` silently dropped on every comment
The \`issue_comment.created\` handler had a defensive early-exit:

    if (!comment?.body || !context.octokit?.issues) {
      return;
    }

In Probot v14, \`context.octokit.issues\` is sometimes \`undefined\`
(same root cause as PR #22 but at a different call site). When that
happens, the entire ChatOps system silently breaks — every \`/review-pr\`,
\`/configure-repo\`, \`/sync-all-repos\` etc. evaporates with no log,
no comment, no error. Today's \`/review-pr\` triggers on rivet#213 and
temper#28 hit exactly this.

Fix: drop the \`.issues\` portion of the guard. Keep \`!context.octokit\`
because we genuinely need an octokit. The downstream \`.issues.X\` calls
that already work (e.g., the existing \`/configure-repo\` ChatOps in
production) keep working; if any individual one fails, it'll throw and
the Probot \`onError\` handler logs it — *much* better signal than silent
skipping every command.

## Test plan
- [x] All 766 tests pass
- [x] eslint clean
- [ ] After merge + self-update: comment \`/review-pr\` on a recent PR.
      The bot should post a "🔍 AI review in progress..." comment within
      a couple of seconds (whereas today nothing appears at all).
- [ ] Trigger \`/review-pr\` on a rivet-instrumented PR. Logs should now
      show successful \`rivet validate\` execution (no more ENOENT). If
      the binary path is correct, the oracle's findings prepend the
      review comment.

## Risk & rollout
- Risk: low. Two narrow changes. Path resolution is purely additive (new
  branch only fires when binary_path is relative). Guard removal restores
  behaviour to what the rest of the org's chatops have been depending on.
- Rollout: self-update on merge.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 src/ai-review.js | 9 ++++++++-
 src/app.js       | 8 +++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/ai-review.js b/src/ai-review.js
index 9e298a9..f4ccd0f 100644
--- a/src/ai-review.js
+++ b/src/ai-review.js
@@ -382,6 +382,13 @@ async function reviewPullRequest(octokit, owner, repo, prNumber) {
     if (rivetCfg?.enabled && rivetCfg?.binary_path) {
       const headSha = prData.data.head?.sha;
       const baseSha = prData.data.base?.sha;
+      // Resolve binary path against the temper deploy root, NOT the tmpdir
+      // cwd that runRivetOracle uses. Without this, a relative binary_path
+      // like "data/rivet/rivet" gets looked up inside the freshly-extracted
+      // PR tarball — which obviously doesn't have the rivet binary in it.
+      const resolvedBinary = path.isAbsolute(rivetCfg.binary_path)
+        ? rivetCfg.binary_path
+        : path.resolve(__dirname, '..', rivetCfg.binary_path);
       try {
         if (headSha && (await hasRivetYaml(octokit, owner, repo, headSha))) {
           oracleSummary = await withTempRepoCheckout(
@@ -389,7 +396,7 @@ async function reviewPullRequest(octokit, owner, repo, prNumber) {
             owner,
             repo,
             headSha,
-            (repoPath) => runRivetOracle(rivetCfg.binary_path, repoPath, {
+            (repoPath) => runRivetOracle(resolvedBinary, repoPath, {
               baseRef: baseSha,
               timeout: rivetCfg.timeout_ms || 60000
             })
diff --git a/src/app.js b/src/app.js
index 7d9ef92..8db4fe7 100644
--- a/src/app.js
+++ b/src/app.js
@@ -276,7 +276,13 @@ function registerApp(app, options = {}) {
     const config = getConfig();
     const { comment, repository, sender } = context.payload;
 
-    if (!comment?.body || !context.octokit?.issues) {
+    // Only check that the body is present. We previously also gated on
+    // `context.octokit?.issues` being defined, but Probot v14 doesn't
+    // *always* expose the `.issues` namespace — and silently skipping every
+    // ChatOps command when it's missing is worse than letting individual
+    // calls fail loudly (which they don't, in practice — `octokit.request()`
+    // is always available and the existing call sites use it).
+    if (!comment?.body || !context.octokit) {
       return;
     }